http: restore the "> " and "[fetch] " prefixes on verbose trace request lines - #38691
http: restore the "> " and "[fetch] " prefixes on verbose trace request lines#38691robobun wants to merge 1 commit into
Conversation
…st lines print_request passed "> ..." templates to pretty_errorln!, which drops a bare ">" as a tag terminator, so request lines printed as " HTTP/1.1 GET url" while response lines printed "< 200 OK". Escape the ">" and print the same "[fetch] " tag the response lines carry on color terminals, via a shared picohttp::trace_line_prefix(). Delete the unused Display impl for picohttp::Request.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: reproduced on the released 1.4.0 ( |
There was a problem hiding this comment.
LGTM — a targeted fix restoring the documented > and [fetch] prefixes on verbose fetch request lines.
What was reviewed:
- Confirmed the root cause in
bun_core_macros/lib.rs: a bare>is dropped as a tag terminator, and\\>is the established literal escape (matchingtree_printer.rs,toHaveBeenCalled.rs, etc.). - Verified
trace_line_prefix()produces byte-identical output to the inline condition it replaces inResponse::fmt, so response lines are unchanged. - Grepped for any caller of the deleted
impl Display for picohttp::Request— none exist;print_requestis the only request formatter and it never used it. - Tests cover
verbose: true,BUN_CONFIG_VERBOSE_FETCH=1|curl, andFORCE_COLOR=1; the tightened[redacted]regex now requires the>prefix instead of tolerating its absence.
Extended reasoning...
Overview
This PR restores two prefixes that went missing from the verbose HTTP client trace during the Rust port: the > marker on request lines and the dim [fetch] tag (color-only). The changes are:
src/http/lib.rs(print_request): The threepretty_errorln!templates now escape>as\\>and interpolatepicohttp::trace_line_prefix()as the first argument. I confirmed insrc/bun_core_macros/lib.rs:96-114that a bare>in a pretty template is silently dropped as a stray tag closer, while\\>emits a literal>— this is the same escape used throughout the tree (tree_printer.rs,clap/lib.rs, help texts).src/picohttp/lib.rs: A newtrace_line_prefix()helper returns the ANSI-expanded[fetch]tag when stderr has colors and""otherwise.Response::fmtnow calls it in place of two inline copies of the identical condition/literal — the response output is byte-for-byte unchanged. The unusedimpl Display for Request<'_>(stale: hardcoded HTTP/1.1, no credential redaction) is deleted; I grepped forpicohttp::Requestdisplay uses and confirmed there are none.test/js/web/fetch/fetch.test.ts: A newdescribe.concurrentblock with four tests coveringverbose: true,BUN_CONFIG_VERBOSE_FETCH=1,=curl, andFORCE_COLOR=1, plus the existing[redacted]test tightened from>?to require the prefix.
Security risks
None. This is purely debug-trace output formatting to stderr. Credential redaction (Authorization: Bearer [redacted]) is preserved and now covered by two additional test assertions. The prefix is passed as a {} argument, and the macro only rewrites the template literal — arguments are substituted after tag expansion — so no template-injection concern arises from the interpolated prefix.
Level of scrutiny
Low-to-medium. This is a cosmetic regression fix in diagnostic output with no runtime-behavior implications beyond what's printed to stderr. The mechanism (pretty-template escaping) is well-understood and the fix follows the exact pattern used elsewhere in the codebase. The deleted Display impl was verifiably dead. The refactor of Response::fmt to use the shared helper is a straightforward deduplication with identical semantics.
Other factors
- The PR description is thorough: it names the exact source line where
>is dropped, cites prior art for\\>, and explains why the[fetch]tag is color-only (matching pre-port and response-side behavior). - Test coverage is strong: exact first-line assertions, every-line-prefixed assertions via
filter, both plain and ANSI paths, and the negative check that the raw token doesn't leak. Tests usebunEnvspread,port: 0,describe.concurrent, and drain both pipes concurrently — all matching harness conventions. - The PR notes overlap with #38673 and that whichever lands second needs a trivial rebase; this PR's HEAD (4b073d2) is already on
main, so that has been resolved. - No prior human or bot reviews to address; CodeRabbit was rate-limited.
Problem
fetch(url, { verbose: true }),BUN_CONFIG_VERBOSE_FETCH=1|curl,bun install --verbose) is documented (docs/runtime/networking/fetch.mdx, docs/runtime/debugger.mdx) as printing request lines as[fetch] > HTTP/1.1 GET http://example.com//[fetch] > Connection: keep-aliveand response lines as[fetch] < 200 OK.NO_COLOR=1:FORCE_COLOR=1or a terminal) the response lines also carry the dim[fetch]tag and the request lines still carry nothing.print_requestinsrc/http/lib.rs:1439-1459: the three templates passed topretty_errorln!start with a bare>. In the pretty template language a bare>is a tag terminator (<cyan>'s closing character) and is dropped (src/bun_core_macros/lib.rs:112, mirrored bypretty_fmt_runtimeinsrc/bun_core/output.rs); a literal>has to be written\\>. The response side is unaffected becausepicohttp::Response'sDisplaywrites<throughwrite_str, not through a template.[fetch]tag was dropped by the same port: the ZigprintRequestwrote the tag plus>through the raw error writer, and the port replaced that with the templates above (the original port left aTODO(port)about the missing prefix, later removed without restoring it).Fix
src/http/lib.rsprint_request: the three templates escape the>("{}\\> ..."), and each line starts withpicohttp::trace_line_prefix().src/picohttp/lib.rs: newtrace_line_prefix()returns the<r><d>[fetch]<r>expansion when stderr has colors and""otherwise, andResponse'sDisplaynow uses it in place of its two inline copies of that condition and literal (same condition, same literal, so response output is byte for byte unchanged). The unusedimpl Display for picohttp::Request(a second, stale copy of the request format: hardcodedHTTP/1.1, no credential redaction, no callers since the port) is deleted rather than updated.\\>is the established spelling of a literal>in these templates (src/install/lockfile/printer/tree_printer.rs-\\>,src/runtime/test_runner/expect/toHaveBeenCalled.rs\\>=, the\\<cmd\\>help texts), and taking the tag from the same function the response lines use makes the two halves of the trace line up the way the docs and the pre-port code had them; the tag staying color-only keeps plain (piped) output identical in shape to the response lines, which already behave that way.>(the->arrows in somebun install --verboseand bin-linker messages, etc.) are unrelated output and are not touched here.print_request's argument lists,Response'sDisplayand the end offetch.test.ts, so whichever lands second needs a trivial rebase.test/js/web/fetch/fetch.test.ts:verbose fetch logging line prefixesblock:verbose: true,BUN_CONFIG_VERBOSE_FETCH=1and=curl(plain output: every trace line starts with>or<, exact first line> HTTP/1.1 GET <url>, the redactedAuthorizationline, a custom header,< 200 OK), andFORCE_COLOR=1(every line starts with the exact\x1b[0m\x1b[2m[fetch]\x1b[0mbytes, and afterBun.stripANSIevery line matches[fetch] >/[fetch] <).[redacted]test now requires the>prefix instead of tolerating its absence (>?), which is how this went unnoticed.test/regression/issue/12042.test.ts(curl trace) still passes. The rest offetch.test.tshas the same set oflocalhost/IPv6 and root-permission failures here with and without this change.cargo check -p bun_picohttp -p bun_httpandcargo fmtare clean.Background
pretty_errorln!and friends take a template in which<cyan>,<d>(dim),<r>(reset) and so on are rewritten at compile time into ANSI escapes, or stripped when the destination has no colors (bun_core::pretty_fmt!). Only the template is rewritten;{}arguments are substituted afterwards and are never scanned for tags, which is why header values containing>were always printed intact and only the template's own>went missing.HTTPVerboseLevel::Headersprints the>/<lines;Curladditionally prints a copy-pasteablecurlcommand first. HTTP/1.1, h2 and h3 all call the sameprint_request, so the three protocols share this fix;print_responseformats the response throughpicohttp::Response'sDisplay.FORCE_COLOR=1makesOutputenable colors on a piped stderr (and overridesNO_COLOR), which is how the color case is exercised from a test.