Uh oh!
There was an error while loading. Please reload this page.
fix(interceptor): surgical fixes for Request body bytes + http.request overload edges (#10, #12) - #35
Merged
Merged
Conversation
Empirical testing on Node 26 showed Request.headers.get("content-length")
returns null for undici-backed fetch — undici sets the header on the wire
but never on the Request.headers object. The sync recovery path the plan
originally specified was rejected.
Switching to async Request.clone() + arrayBuffer() (user-approved Option A).
Deliberately changes the contract for stream-bodied Requests: was 0, now
actual bytes — which is what the issue explicitly asks for.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements Wave 3 of the interceptor roadmap: three targeted correctness fixes to request body byte estimation for fetch with Request objects, support for options.path overrides in http.request with URL first arguments, and defensive hostname normalization to strip embedded ports. ChangesInterceptor Surgical Fixes
Sequence DiagramssequenceDiagram
participant HttpRequestWrapper as http.request wrapper
participant ExtractUrl as extractUrl
participant Telemetry as Telemetry event
HttpRequestWrapper->>HttpRequestWrapper: Detect URL-like first arg<br/>and RequestOptions.path
HttpRequestWrapper->>ExtractUrl: Call with url and pathOverride
ExtractUrl->>ExtractUrl: Parse URL, normalize hostname<br/>(strip embedded port)
ExtractUrl->>ExtractUrl: Apply pathOverride to pathname<br/>if provided
ExtractUrl->>HttpRequestWrapper: Return normalized url/path
HttpRequestWrapper->>Telemetry: Record with corrected path
sequenceDiagram
participant PatchedFetch as patchedFetch
participant EstimateRequestBytes as estimateRequestBytes
participant Telemetry as Telemetry event
PatchedFetch->>PatchedFetch: Validate instrumentation<br/>will be recorded
PatchedFetch->>EstimateRequestBytes: Start requestBytesPromise<br/>(async)
PatchedFetch->>PatchedFetch: Call fetch(input, init)
alt Success or HTTP error
PatchedFetch->>EstimateRequestBytes: Await promise
EstimateRequestBytes-->>PatchedFetch: Return requestBytes
PatchedFetch->>Telemetry: Emit success event<br/>with requestBytes
else Fetch fails
PatchedFetch->>EstimateRequestBytes: Await promise in catch
EstimateRequestBytes-->>PatchedFetch: Return requestBytes (0 if clone failed)
PatchedFetch->>Telemetry: Emit error event<br/>with requestBytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t count Final-review pass found three stale references inside the plan doc that still described the abandoned content-length-header approach (Task 2 Step 7 commit-message template, Task 5 Step 2 expected-log block, Task 5 Step 4 PR-body template). Updates them to match the async clone+arrayBuffer approach that actually shipped, and corrects the test count from 239 to 240 (one extra test landed during Task 2's review fix-up). No code change. PR description (#35) was already correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.md (1)
946-946: 💤 Low valueClarify or remove stale line-number reference.
"caller at line 151 updated" appears to reference the original source file, but the plan doesn't mention line 151 elsewhere. If this refers to
patchedFetchinsrc/core/interceptor.ts, consider clarifying or removing the stale reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.md` at line 946, The plan contains a stale line-number reference ("caller at line 151 updated") that likely refers to the patchedFetch caller; update the text to either remove the line-number mention or replace it with a clear symbol-based reference such as "the caller in patchedFetch" and note that estimateRequestBytes' signature was changed and that patchedFetch was updated accordingly; ensure you reference the function names estimateRequestBytes and patchedFetch (not a line number) so readers can find the change.tests/interceptor.test.ts (1)
251-321: ⚡ Quick winAdd a regression for prompt resolve/reject with slow
Requeststreams.These cases validate byte accounting, but they do not protect against the bigger risk here:
fetch(req)resolving or rejecting only after the cloned body finishes draining. A slow-stream + early-failure/early-response case would lock that down. As per coding guidelines, "tests/**/*.test.ts: Use vitest for unit testing with a target of comprehensive coverage across all core modules".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/interceptor.test.ts` around lines 251 - 321, Add a regression test in interceptor.test.ts that verifies fetch(req) resolves/rejects without waiting for a slow Request body clone to finish draining: create a Request with a ReadableStream that yields data slowly (use async delays in start/controller.enqueue), send it to a server route that responds immediately (e.g., "/early-response" or "/early-error"), call fetch(req) and assert that the Promise resolves/rejects promptly (don’t await the full stream drain) and that the interceptor still records an event with a sane requestBytes (use the existing events array and estimateRequestBytes behavior). Reference Request, fetch, the ReadableStream start/controller.enqueue, the server route handler, and the interceptor/event capture to locate where to add the test; implement the delay with a small await/timeout and use vitest assertions/timers consistent with the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.md`:
- Around line 891-923: The PR body incorrectly states the rejected approach for
`#12` (saying the fix reads Request.headers.get("content-length")); update the PR
body text for issue `#12` to describe the actual implementation: state that
estimateRequestBytes became async, and when init.body is absent and input is a
Request with a body it clones the Request (input.clone()) and awaits the clone's
arrayBuffer() (cloned.arrayBuffer()) to measure bytes so the original
Request/body stream is preserved; replace the existing one-line claim about
content-length with this corrected explanation.
- Line 5: The plan text miscounts fixes: it claims "two narrow correctness bugs"
but the interceptor changes actually address three issues—estimateRequestBytes
failing to read Request bodies, the http.request overload behavior that drops
options.path when the first argument is a URL, and the host-with-embedded-port
vs opts.port collision that strips the port; update the description and PR title
to list all three fixes and adjust any changelog/commit messages accordingly,
and ensure references to src/core/interceptor.ts and the functions/variables
estimateRequestBytes, fetch handling for new Request(url, { body }), and the
http.request code path that uses options.path, opts.host, and opts.port are
mentioned so reviewers can verify each fix is covered.
In `@src/core/interceptor.ts`:
- Around line 218-230: The code currently awaits requestBytesPromise (variables
requestBytesPromise and requestBytes) on the caller-visible fetch path which can
block resolution for slow/streaming Request bodies; instead remove the await
from the success and error return paths and handle request-byte measurement
asynchronously by attaching a background continuation (e.g.
requestBytesPromise.then(bytes => { requestBytes = bytes; emitTelemetry(...);
}).catch(() => {/* swallow or log safe default */})); ensure
estimateRequestBytes is invoked as before but do not await it on either the
normal return or rethrow path so the fetch resolution is not delayed.
- Around line 73-77: The hostname extraction currently truncates bracketed IPv6
addresses by using indexOf(":"), so update the logic that computes hostname from
hostRaw (derived from opts.hostname ?? opts.host ?? "localhost") to be
bracket-aware: if hostRaw starts with "[" find the matching "]" and take the
substring through that bracket (so "[::1]:8080" -> "[::1]"); otherwise, strip a
trailing numeric port by looking for the last ":" and only slicing there if the
substring after the last ":" is all digits (so "host:8080" -> "host" but
"user@host:abc" stays unchanged). Apply this change where hostname is defined
(and used by extractUrl()/related URL construction) so IPv6 bracketed hosts and
non-port colons are handled correctly.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.md`:
- Line 946: The plan contains a stale line-number reference ("caller at line 151
updated") that likely refers to the patchedFetch caller; update the text to
either remove the line-number mention or replace it with a clear symbol-based
reference such as "the caller in patchedFetch" and note that
estimateRequestBytes' signature was changed and that patchedFetch was updated
accordingly; ensure you reference the function names estimateRequestBytes and
patchedFetch (not a line number) so readers can find the change.
In `@tests/interceptor.test.ts`:
- Around line 251-321: Add a regression test in interceptor.test.ts that
verifies fetch(req) resolves/rejects without waiting for a slow Request body
clone to finish draining: create a Request with a ReadableStream that yields
data slowly (use async delays in start/controller.enqueue), send it to a server
route that responds immediately (e.g., "/early-response" or "/early-error"),
call fetch(req) and assert that the Promise resolves/rejects promptly (don’t
await the full stream drain) and that the interceptor still records an event
with a sane requestBytes (use the existing events array and estimateRequestBytes
behavior). Reference Request, fetch, the ReadableStream
start/controller.enqueue, the server route handler, and the interceptor/event
capture to locate where to add the test; implement the delay with a small
await/timeout and use vitest assertions/timers consistent with the other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c01bf62-6531-4de7-a46c-6beec1ccf437
📒 Files selected for processing (4)
docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.mddocs/superpowers/roadmap-2026-05-13-issue-waves.mdsrc/core/interceptor.tstests/interceptor.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
4 tasks
The original 10b strip used first-colon split on the host string, which
broke IPv6:
- `[::1]:8080` → first colon at index 1 (inside `[::1]`) → "["
- `::1` → first colon at index 0 → ""
Rewrite the strip to be bracket-aware:
- Bracketed IPv6 ("[::1]:8080"): strip ":port" after the closing "]"
- Bare IPv6 ("::1"): preserve as-is (multi-colon → no strip). URL
reconstruction still fails downstream for unbracketed forms (URL
spec requires brackets), but that's a graceful null-return rather
than silent data corruption.
- Regular host/IPv4 ("host" or "host:port"): strip when exactly one
colon, leave alone otherwise.
Test: bracketed IPv6 with collision (`[::1]:PORT` + `port: PORT`) now
captures a correctly-attributed event via the error-path callback.
WHATWG `URL.hostname` retains brackets for IPv6 literals, so the
captured `event.host` is `[::1]` (matches Node's URL semantics).
CodeRabbit comment 3246546663 on PR #35 — flagged Major. Pre-Wave-3,
this was a silent null-return; post-Wave-3 the strip introduced silent
wrong data. This commit restores graceful behavior for IPv6.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>…nt doesn't block fetch caller (#12) Move the `requestBytesPromise` await off the caller-visible fetch path. Previously, `patchedFetch` awaited the cloned-body materialization before returning the response (success path) or rethrowing (error path), which could delay large/streaming uploads' fetch resolution by the body-materialization time. Now: - Bodyless response path: schedule a deferred IIFE that awaits the request-byte measurement, then fires telemetry. Return the response immediately. - Streaming response path: `fireTelemetry` is async and awaits the request-byte measurement inside the existing body-counter IIFE, which only delays the eventual `_callback`, not the caller's fetch resolution or response stream consumption. - Error path: schedule a deferred IIFE for the error event after awaiting the request-byte measurement; rethrow immediately. `latencyMs` is computed synchronously at response-arrival (or error) time, so latency reflects server response time rather than request-body materialization time. Event contents and ordering are unchanged. Tests: added `flushDeferred()` helper (one `setImmediate` cycle) before event-array assertions in the affected interceptor tests so the deferred IIFE has fired before assertions. No assertion weakened. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wave 3 — two narrow correctness bugs in
src/core/interceptor.ts, plus the leftover roadmap docs maintenance from Wave 2.fetch(new Request(url, { body }))reportedrequestBytes: 0becauseestimateRequestBytesonly inspectedinit.body. The body on a constructed Request is a ReadableStream by then, andRequest.headers.get("content-length")is unreliable on Node's undici fetch (empirically verified — undici sets the header on the wire but not onRequest.headers). Fix:estimateRequestBytesbecomes async, clones the Request, and readscloned.arrayBuffer()for the byte count. The clone tees the body stream, so the original Request still feeds the actual outgoing HTTP request. Deliberate contract change: stream-bodied Requests now report actual bytes (the issue's intent) at the cost of materializing the body in memory (~2× peak for the body).http.request(URL, { path })silently droppedoptions.path. Fix:extractUrlgains an optionalpathOverride; the wrapper computes it from second-argoptions.pathwhen the first arg is a URL/string.opts.hostcontaining an embedded:portplus a separateopts.portproduced an unparseable URL (host:port:port), silently skipping instrumentation. Fix: strip any embedded port fromopts.hostbefore appendingopts.port.No public API changes. No new exports. No changes to the install/uninstall lifecycle or the double-count guard.
Roadmap
First commit also marks Waves 1 + 2 as done (links to merged PRs #33 and #34) and flips Wave 3 to in-progress — leftover docs maintenance bundled into this PR.
Commits
docs: mark waves 1+2 done; add wave 3 surgical fixes plan (#10, #12)— roadmap + plan doc.docs(plans): revise #12 approach to async clone+arrayBuffer— plan revision after empirically discovering the original sync content-length-header approach didn't work on undici-backed fetch.fix(interceptor): measure Request body via clone+arrayBuffer for fetch (#12)— Task 2 initial fix.fix(interceptor): skip body measurement on parse failure; expand #12 comments+tests— Task 2 review fix-up (avoid orphaning the clone-and-measure promise on URL parse failure; tighten worst-case-cost comment; add regression test for already-consumed Request).fix(interceptor): honor options.path when first arg is URL (#10)— Task 3 (#10a impl + #10b impl bundled, regression tests for 10a only).test(interceptor): strip embedded port from opts.host (#10)— Task 4 (#10b regression tests).Test plan
npm run lintcleannpm run buildemits ESM + CJS + DTSnpm run test— 240/240 vitest (baseline 230 + 10 new; total includes the 7 dist smoke tests;test:distre-runs those 7 separately after build)clone()throws → records 0)Plan
See
docs/superpowers/plans/2026-05-15-interceptor-surgical-fixes.md.Closes#10, closes#12.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
fetchand HTTP requests.Tests
Documentation