Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 2 remain after this review. WalkthroughChangesMacro logging and regression coverage
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change routes macro debug output through the existing scoped logging behavior and tightens related test assertions; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed explanation of the problem, fix, behavior, related test updates, and verification results. It does not use the template headings exactly, but it includes the required information and is substantially complete. Comment |
|
Status: ready for review. How it was checked: on a local debug build, Scope after self-review: the unawaited case of CI (build #107085, rebased onto the merged #40614): PR: #40658 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap to the documented scoped-logging convention, plus test tightening.
What was reviewed:
declare_scope!(Macro, visible)+scoped_log!matches the pattern used across the tree (AWS, fetch, AsyncHTTP, Postgres); the macro still gates onenv::IS_DEBUGand dead-strips in release, and the[macro]prefix is preserved via_LowerTag.- Test assertions are strictly stronger than before (
toBeovertoContain; sortedtoEqualnow also bounds the line count), andbunEnvsetsBUN_DEBUG_QUIET_LOGS=1so exact stdout is safe to assert. - The intentionally-untouched filters in
macro-test.test.tsare called out in the PR body with a reason.
Extended reasoning...
Overview
The PR replaces a hand-rolled if IS_DEBUG { prettyln!(...) } in src/js_parser_jsc/Macro.rs's Runner::run with the repo-standard bun_core::declare_scope!(Macro, visible) + bun_core::scoped_log!(Macro, ...) pair, and tightens two assertions in test/regression/issue/39900.test.ts that previously had to tolerate the stray [macro] call ... line on debug builds. Net diff is one added static, one log-line rewrite, and three assertion lines changed.
Security risks
None. This is debug-build-only diagnostic output; the scoped_log! macro body is gated on env::IS_DEBUG and dead-strips in release exactly as the old if IS_DEBUG branch did. No user input handling, no auth/crypto, no data exposure.
Level of scrutiny
Low. The change is a textbook application of the convention documented in src/CLAUDE.md ("use bun_core::declare_scope! + scoped_log! instead of ad-hoc prints"), and I verified the macro definitions in src/bun_core/output.rs — the expansion still gates on IS_DEBUG, adds an is_visible() check (which is what makes BUN_DEBUG_QUIET_LOGS work), and emits the same [macro] call <name> shape via the lowercased tag. Fifteen-plus other call sites use the identical pattern, so there is no novelty here.
Other factors
The test edits move in the direction REVIEW.md asks for: toBe over toContain, and a sorted toEqual that both handles the legitimate ordering nondeterminism of the unawaited digest and asserts a stronger invariant (exactly two lines, not merely "contains both"). The PR body explicitly names why the now-dead filters in macro-test.test.ts are left in place (conflict avoidance with two in-flight PRs), satisfying the "intentionally excluded — say so" rule. No prior reviews or objections on the timeline; hunt exited on dry_streak with no candidates.
There was a problem hiding this comment.
LGTM — the scope-name point from the earlier pass is addressed, and the follow-up commit drops the now-dead [macro] stdout workarounds.
Checked that scoped_log! gates on env::IS_DEBUG so release still dead-strips, and that all three tightened spawn sites pass bunEnv (which sets BUN_DEBUG_QUIET_LOGS=1 and strips other BUN_DEBUG_*), so the exact stdout/stderr: "" assertions hold on debug builds. The macros scope name is unique across src/ and avoids the Macro struct in the same module.
Extended reasoning...
Overview
The PR replaces a bare bun_core::prettyln! gated on IS_DEBUG in src/js_parser_jsc/Macro.rs::Runner::run with the standard scoped-logger convention: bun_core::declare_scope!(macros, visible) at module scope plus bun_core::scoped_log!(macros, ...) at the call site. Three tests that had loosened stdout matching to tolerate the old unconditional debug line are tightened back to exact assertions (toEqual/toBe on full stdout, stderr: ""), and the .filter(line => !line.startsWith("[macro]")) / regex-tail workarounds are removed.
Security risks
None. This is a debug-build-only diagnostic print being routed through the existing logging machinery. No user-controlled input reaches new code paths, no auth/crypto/permissions surface is touched, and the body still dead-strips in release via the env::IS_DEBUG gate inside scoped_log! (src/bun_core/output.rs:1537).
Level of scrutiny
Low. The change is mechanical and follows the pattern src/CLAUDE.md explicitly documents for debug logging. I verified: (1) scoped_log! is gated on IS_DEBUG && scope.is_visible(), preserving the release dead-strip; (2) bunEnv in test/harness.ts sets BUN_DEBUG_QUIET_LOGS: "1" and deletes any inherited BUN_DEBUG_* keys, so every tightened test's child process has the scope silenced and the exact-match assertions are safe; (3) all three modified spawn sites use bunEnv or { ...bunEnv, ... }; (4) no other declare_scope!(macros, ...) exists in src/, so the new static doesn't collide.
Other factors
My earlier nit (scope name macros vs. the description's Macro/[macro]) was addressed — the author kept macros to avoid shadowing the struct Macro in the same file and updated the description to match ([macros], BUN_DEBUG_macros=1). The subsequent commit 392cde9 removed the test workarounds rather than leaving them as dead code, which is the right call per REVIEW.md's "delete dead code in the same PR that makes it dead". The test tightening moves in the direction REVIEW.md asks for (exact values over toContain/regex tails).
392cde9 to
8fb1ffe
Compare
There was a problem hiding this comment.
LGTM — the scope-name concern from the earlier review is addressed (description now documents [macros]/BUN_DEBUG_macros, and the commit explains Macro collides with the struct name).
What was reviewed:
declare_scope!/scoped_log!expansion insrc/bun_core/output.rs— gated onenv::IS_DEBUG && is_visible(), so release builds still dead-strip andBUN_DEBUG_QUIET_LOGSnow applies.- Test edits only strengthen assertions (
toEqual/toBeon full stdout, addedstderr: ""); no existing coverage weakened. - The unawaited case in
39900.test.tsintentionally left withtoContain, matching the PR's stated timing rationale.
Extended reasoning...
Overview
The PR replaces a raw bun_core::prettyln! gated on IS_DEBUG in src/js_parser_jsc/Macro.rs::Runner::run with the repo's scoped-logger convention (declare_scope!(macros, visible) + scoped_log!). This routes the debug trace through the ScopedLogger machinery so BUN_DEBUG_QUIET_LOGS=1 (which bunEnv sets) suppresses it, and BUN_DEBUG_macros=1 re-enables it selectively. Three tests that previously worked around the stray [macro] call ... line in child stdout are tightened to exact-match assertions.
Security risks
None. This is a debug-only logging refactor with no user-facing behavior, no input parsing, and no auth/crypto/permission surface. The scoped_log! body is dead-stripped in release builds via the env::IS_DEBUG gate, identical to the previous behavior.
Level of scrutiny
Low. The change is small (one 6-line hunk in native code, three test-assertion tightenings), mechanical, and follows the exact convention documented in src/CLAUDE.md ("use bun_core::declare_scope! + scoped_log! instead of eprintln!/prettyln! for debug logging"). I verified the macro definitions in src/bun_core/output.rs:1507-1547 to confirm the visibility gating and release dead-stripping behave as described.
Other factors
My prior inline nit (scope named macros producing tag [macros] rather than [macro], contradicting the then-current PR description) has been addressed: the description now correctly documents [macros] and BUN_DEBUG_macros=1, and commit 04dcb39b records the reason (Macro is the struct name in this module, so the scope uses the plural). The test edits are pure strengthenings per REVIEW.md ("toBe over toContain", combined {stdout, stderr, exitCode} object assertion) — the first macro-test.test.ts hunk moves from toMatchObject without a stderr key to toEqual with stderr: "", which is strictly tighter. No assertions were weakened; the unawaited 39900 case is untouched. No outstanding CHANGES_REQUESTED reviews from other reviewers.
|
Updated 4:24 PM PT - Aug 27th, 2026
❌ @robobun, your commit 010f058 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40658That installs a local version of the PR into your bun-40658 --bun |
The debug-only print of every macro call was a plain prettyln! gated on IS_DEBUG, so BUN_DEBUG_QUIET_LOGS did not silence it and it landed on stdout of every macro test. It is now scoped_log!(Macro, ...): same text by default in debug builds, off under BUN_DEBUG_QUIET_LOGS, and BUN_DEBUG_Macro=1 turns it on alone. Release builds are unchanged. The 39900 test asserts the child's exact stdout instead of a substring.
…ruct in this module
…kes dead macro-test.test.ts asserted a stdout tail, a last line, and filtered "[macro]" lines out because debug builds printed the macro call log to stdout. With the log behind BUN_DEBUG_QUIET_LOGS the three tests assert the exact stdout. The unawaited case of 39900.test.ts goes back to main's two toContain checks, since whether "settled" prints is timing.
…e positive The linear-sweep scan of the linux-x64 baseline build decoded one RDPMC out of the opcode id embedded at the start of that LLInt handler. The layout shift that moved the desync onto this symbol is this branch's Rust change. Same class as the two llint_op_* entries already listed.
There was a problem hiding this comment.
LGTM — the earlier scope-name nit was addressed (description now documents macros/BUN_DEBUG_macros and why it isn't Macro), and the new llint_op_jmp_wide32 allowlist entry follows the documented data-in-.text blanket-pass convention.
Checked the scoped_log! swap against declare_scope!(name, visible) semantics — silenced by BUN_DEBUG_QUIET_LOGS=1 (which bunEnv sets), dead-strips in release, so the tightened toEqual/toBe assertions hold on both build profiles.
Checked the allowlist entry: bare symbol with no [FEAT] ceiling for a linear-sweep misdecode, reason noted in the group comment, (N symbols) count bumped 2→3 — matches scripts/verify-baseline-static/CLAUDE.md.
Confirmed the dropped stderr key in the first macro-test.test.ts assertion is recovered by switching toMatchObject → toEqual with stderr: "".
Extended reasoning...
Overview
Two independent, mechanical changes. First, src/js_parser_jsc/Macro.rs replaces an unconditional IS_DEBUG-gated prettyln!("[macro] call ...") with declare_scope!(macros, visible) + scoped_log!, which routes the same debug line through the standard scoped-logger machinery so BUN_DEBUG_QUIET_LOGS=1 silences it and release builds dead-strip it. Three tests that previously worked around the stray stdout line (macro-test.test.ts regex-tail match, last-line pop, [macro]-prefix filter; 39900.test.ts toContain) are tightened to exact toEqual/toBe assertions. Second, scripts/verify-baseline-static/allowlist-x64.txt adds llint_op_jmp_wide32 as a blanket-pass entry to the existing LLInt data-in-.text false-positive group after this branch's layout shift moved the linear-sweep desync onto that symbol.
Security risks
None. The change touches debug-only logging (dead-stripped in release), test assertions, and a static-analysis allowlist for a known-benign LLInt embedded-opcode-id misdecode. No auth, crypto, network, or user-input parsing paths are involved. The allowlist entry is a blanket pass, but per scripts/verify-baseline-static/CLAUDE.md that is the correct form for confirmed data-in-.text misdecodes — a feature ceiling on layout-dependent garbage bytes would just re-flake, and a real -march leak in LLInt would surface across the other llint_op_* symbols and in the SDE emulator step the group comment references.
Level of scrutiny
Low. Every piece follows an explicitly documented repo convention: src/CLAUDE.md prescribes declare_scope! + scoped_log! over ad-hoc prettyln! for debug output; test/CLAUDE.md prescribes exact toEqual on composite objects and notes bunEnv sets BUN_DEBUG_QUIET_LOGS=1; scripts/verify-baseline-static/CLAUDE.md prescribes bare-name blanket passes with a group-comment reason and updated count for data-in-.text false positives. The PR's evidence block shows the tightened tests fail on the unfixed debug build (with [macro] call sha in the diff) and pass on both debug and release with the fix, satisfying the "test fails for the right reason" bar.
Other factors
My earlier inline nit (scope name macros vs the description's claim of [macro]/BUN_DEBUG_Macro) was addressed by updating the PR description to match the code and explain the naming choice (Macro is already the struct name in that module) — a reasonable resolution for a description/code mismatch. The only commit since the previous review round is the allowlist addition, which is a three-line data change in the exact shape the neighboring entries and CLAUDE.md prescribe. No CODEOWNERS cover these paths. No outstanding third-party CHANGES_REQUESTED reviews.
7cca1d2 to
010f058
Compare
There was a problem hiding this comment.
LGTM — the earlier [macro]/[macros] naming nit is settled (description now matches the code, and Macro colliding with the struct at Macro.rs:378 is a fair reason to keep the scope as macros).
What was reviewed:
declare_scope!(macros, visible)+scoped_log!matches the src/CLAUDE.md convention; release builds still dead-strip the body.- Test edits are strict tightenings only (
toEqual/toBereplacing tail-regex, last-line, and[macro]-filter workarounds); no assertion was weakened. llint_op_jmp_wide32allowlist entry follows the verify-baseline-static triage guide — blanket pass for a decode-desync false positive, comment updated, count bumped 2→3.
Extended reasoning...
Overview
Four files. src/js_parser_jsc/Macro.rs swaps an if IS_DEBUG { prettyln!("[macro] call ...") } for declare_scope!(macros, visible) + scoped_log!, so BUN_DEBUG_QUIET_LOGS (which bunEnv sets) now silences it. Three tests that had workarounds for that stdout noise — a /200000\n$/ tail match, a lastLine pop, a .filter(line => !line.startsWith("[macro]")), and a toContain — are converted to exact toEqual/toBe assertions. scripts/verify-baseline-static/allowlist-x64.txt gains llint_op_jmp_wide32 as a blanket pass for a linear-sweep decode false positive (stray RDPMC out of LLInt handler bytes), with the group comment and count updated.
Security risks
None. The Rust change touches a debug-build-only log line that release builds dead-strip; no user-facing behavior, no input parsing, no auth/crypto/permissions. The test edits only strengthen assertions. The allowlist entry suppresses a static-scan false positive on JSC's LLInt interpreter bytes (not gated code, not compiled from a -march flag), per the tool's own documented triage procedure.
Level of scrutiny
Low. This is a logging-convention alignment plus test-workaround cleanup. The declare_scope!/scoped_log! pattern is the exact one src/CLAUDE.md prescribes for debug logging. The allowlist edit follows scripts/verify-baseline-static/CLAUDE.md to the letter: confirmed data-in-.text decode desyncs get a bare-name blanket pass (no [...] ceiling) because the misdecoded bytes shift with link layout and there is no gate to bound; the added comment names the reason and the (N symbols) count is bumped. No CODEOWNERS cover the touched paths.
Other factors
The earlier inline nit about the scope name (macros vs Macro) was addressed by updating the PR description and giving a reason — pub struct Macro already exists at line 378 of the same file, so reusing that identifier for the logger static would shadow it. That's a reasonable resolution. The stamp evidence shows the tightened tests fail on a debug build without the fix (the [macro] call line lands in the diff) and pass with it, and pass on release both ways — which is the expected shape for a debug-only logging change. No third-party CHANGES_REQUESTED reviews are outstanding.
|
The |
This branch has not been deployed
1 participant
Problem
[macro] call <name>to stdout for every macro call. The print insrc/js_parser_jsc/Macro.rs(Runner::run) is a plainprettyln!gated onIS_DEBUG, not a scoped logger, soBUN_DEBUG_QUIET_LOGS=1(set bybunEnvand bybun bd) does not silence it./200000\n$/tail match, alastLinepop and a[macro]line filter intest/bundler/transpiler/macro-test.test.ts, and atoContainintest/regression/issue/39900.test.ts.Fix
declare_scope!(macros, visible)plusscoped_log!(macros, "call <b>{}<r>", ...)replace theIS_DEBUGbranch. The line reads[macros] call identityin a debug build with logs on.BUN_DEBUG_QUIET_LOGS=1turns it off,BUN_DEBUG_macros=1turns it on alone. Release builds dead-strip the body as before. The scope is not namedMacrobecause that is already the struct in this module.src/CLAUDE.mdnames for debug output, and the print carries no information outside debugging.macro-test.test.tsand thetoContainof the awaited case in39900.test.tsbecome exact stdout assertions. On a debug build the old binary fails each of them with the[macro] callline in the diff.bun bd teston39900.test.tsandmacro-test.test.ts, plustranspiler.test.js,scope-mismatch-panic.test.ts, thebun-build-apiandbundler_edgecasemacro cases, and03830,22656,26360.Background
declare_scope!(Name, visible|hidden).scoped_log!(Name, ...)prints[name] ...when the scope is visible.BUN_DEBUG_<Name>,BUN_DEBUG_ALLandBUN_DEBUG_QUIET_LOGSdecide visibility at runtime, and the whole body is gone in release builds.bunEnvintest/harness.tssetsBUN_DEBUG_QUIET_LOGS=1, so tests never see scoped logs from the bun they spawn.Notes
39900.test.tskeeps main's twotoContainchecks. Whether itssettledline prints before exit is timing, and macros: run every macro on one dedicated VM thread #40059 relaxes that assertion for the same reason.Macro.rsand keeps it aprettyln!with anOutput::flush(), and adds one more[macro]filter tomacro-test.test.ts. On rebase it replaces the moved print with thescoped_log!and drops its filter. test: run the serial macro-test spawn cases concurrently and assert exact output #40623 rewrites the two standalone tests ofmacro-test.test.tswith a helper that carries the same filter, which it drops on rebase.src/runtime/cli/upgrade_command.rshas three moreIS_DEBUG-gatedprettyln!sites, on thebun upgradepath no test reaches. They are left for a separate change.scripts/verify-baseline-static/allowlist-x64.txtgainsllint_op_jmp_wide32. The static ISA scan of this branch's linux-x64 build decoded oneRDPMCout of the opcode id embedded at the start of that LLInt handler (build #107047). The layout shift from the Rust change moved the known decode desync onto this symbol. It joins the twollint_op_*entries already listed for the same reason, as a blanket pass because misdecoded bytes have no feature to bound.[stamp-90s] gate passed · iteration 5 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 5
evidence per changed file