Conversation
…f, drop CallTracker Node 26 reworked the assert module around an Assert class whose prototype carries every assertion method, a richer message argument (lazy function messages, printf-style format args, type validation), and an AssertionError `diff` field, while removing CallTracker (DEP0173) and the legacy multi-argument fail (DEP0094). Bun's assert still implemented the older surface. - internal/assert/utils: add Node 26's innerFail (message is the caller's trailing rest array: string + format args, Error, lazy function, or ERR_INVALID_ARG_TYPE for anything else) and update innerOk to match. - node/assert: every comparison method now takes `...message` and forwards it to innerFail; add the Assert(options) constructor (validates options.diff, stores options behind a symbol, aliases strict methods when options.strict) and expose it on assert and assert.strict; thread the per-instance diff through to AssertionError; replace the legacy fail with Node 26's single-argument form; remove the CallTracker accessor. - AssertionError: record options.diff (default 'simple') on the error. - node/test: stop excluding CallTracker from t.assert, exclude Assert, and update the innerOk call signature. Sync the affected upstream tests to their Node 26 versions: drop the CallTracker and fail-deprecation parallel tests (removed upstream), update test-runner-assert's uncopied key list, and update the Symbol-message case in test-assert.js to the new ERR_INVALID_ARG_TYPE expectation.
|
Warning Review limit reached
Next review available in: 6 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 (15)
Comment |
|
Updated 7:06 PM PT - Jul 22nd, 2026
❌ @robobun, your commit bf9d397 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35181That installs a local version of the PR into your bun-35181 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flags:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/assert.ts:1013— Removing theassert.CallTrackeraccessor makessrc/js/internal/assert/calltracker.tsunreachable, but the file is left in the tree andsrc/jsc/bindings/ProcessBindingNatives.cpp:100still listsinternal/assert/calltrackerin the natives table. Per REVIEW.md ("Delete dead code in the same PR that makes it dead — required scope"), the orphaned module and its natives-table entry should be removed here as well.Extended reasoning...
What was left behind
This PR deletes the lazy accessor in
src/js/node/assert.tsthat was the sole runtime consumer ofinternal/assert/calltracker:// removed by this PR Object.defineProperty(assert, "CallTracker", { get() { ... CallTracker = deprecate(require("internal/assert/calltracker"), ...); ... }, });
With that gone, two pieces of infrastructure are now dead but still present:
src/js/internal/assert/calltracker.ts(3337 bytes) — the CallTracker class implementation.rgconfirms no remainingrequire("internal/assert/calltracker")anywhere insrc/, so this module is bundled into the binary but can never be loaded.src/jsc/bindings/ProcessBindingNatives.cpp:100— theinternal/assert/calltrackerentry in theprocess.binding('natives')lookup table, which now points at a module nothing requires.
(
src/node-fallbacks/assert.js:5also destructures/re-exportsCallTracker, but that file imports from the npmassertbrowser-polyfill package rather than Bun's runtime, so it isn't strictly "made dead" by this change — worth a look but a weaker point.)Why this matters
REVIEW.md's code-style section is explicit:
Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): superseded implementations, helpers whose last caller you rewired, ...
calltracker.tsis exactly "a helper whose last caller you rewired" — its onlyrequire()was the accessor this PR removes. Leaving it behind ships an unused module in the bundled builtins and leaves a stale row in the hand-maintained natives table that the next person to touch either file has to reason about.Step-by-step proof
- Before this PR,
grep -rn "internal/assert/calltracker" src/jsreturns exactly one hit: therequire()inside theCallTrackeraccessor insrc/js/node/assert.ts. - This PR's diff removes that accessor block entirely (the
-var CallTracker; -Object.defineProperty(assert, "CallTracker", { ... })hunk). - After applying the PR,
rg calltracker src/yields onlyProcessBindingNatives.cpp:100— no JS-side consumer remains. ls src/js/internal/assert/calltracker.tsstill succeeds: the implementation file is untouched by the diff.- Therefore the module is compiled into the builtins bundle but is unreachable at runtime, and the natives-table row references an orphaned module.
Impact
None at runtime — nothing breaks, this is purely dead weight in the binary and a stale registration. Hence nit severity.
Fix
git rm src/js/internal/assert/calltracker.ts- Remove the
internal/assert/calltrackerline from the lookup table insrc/jsc/bindings/ProcessBindingNatives.cpp - Optionally tidy
src/node-fallbacks/assert.jsto stop re-exportingCallTrackerfrom the browser polyfill - Mention the deletions in the PR description per the REVIEW.md rule
|
Addressed the review points in 7fc7f28:
|
Node 26 threads this into its deep-equal comparison; Bun's Bun.deepEquals- backed implementation does not consume it yet. Accept the option but do not default it, so the stored options object carries only what is read.
|
CI at 7cb7f5b is green on the diff. Remaining reds are all unrelated flakes ( Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-22, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
Node 26 reworked
node:assertaround anAssertclass and a richermessageargument. Bun still implemented the older surface, so four observable behaviors diverged:messagewas stringified (assert.strictEqual(1, 2, () => "lazy")producedAssertionError: () => "lazy"); Node evaluates it lazily on failure and uses the returned string.message(e.g.42, or an explicitundefined) was accepted and stringified; Node rejects it withERR_INVALID_ARG_TYPE.assert.Assertand theAssertionError#difffield were absent.assert.CallTracker(removed in Node 26, DEP0173 end-of-life) was still exported.Repro
Node v26.3.0:
Bun before:
Fix
internal/assert/utils.ts: port Node 26'sinnerFail(themessageargument is always the caller's trailing rest array:[string, ...formatArgs]is formatted,[Error]is thrown,[function]is called with(actual, expected)and its string result used, trailing args after anError/function throwERR_AMBIGUOUS_ARGUMENT, anything else throwsERR_INVALID_ARG_TYPE) and updateinnerOkto forward through it.node/assert.ts: every comparison method now takes...messageand forwards toinnerFail; add theAssert(options)constructor (validatesoptions.diff, aliases strict methods whenoptions.strict, and stores options behind a symbol read viagetDiff(this)so destructured methods fall back to defaults); attach every method toAssert.prototypeand to the module export; drop theCallTrackeraccessor; replacefail()with Node 26's single-argument form. Because functions defined in this builtin context do not automatically get a.prototype, one is assigned explicitly.internal/assert/assertion_error.ts: recordoptions.diff(default'simple') on the error.node/test.ts: excludeAssertinstead ofCallTrackerfromt.assert, and update theinnerOksignature.The upstream parallel tests that cover removed behavior (
test-assert-calltracker-*.js,test-assert-fail-deprecation.js) are dropped to match Node 26's test tree, andtest-assert.js/test-runner-assert.jsare synced to their Node 26 expectations.This subsumes #33567 (the
fail()signature change is part of the same Node 26 restructure).Verification
test/js/node/assert/assert-node26-surface.test.tsexercises lazy/typed message handling across every comparison method plusok/assert()/fail(), theAssertclass shape anddiffpropagation, theAssertionError#difffield, andCallTrackerremoval. 42/47 cases fail on the released build; all 47 pass with this change, as do the existing 360test/js/node/asserttests and Node'stest-assert*.js/test-runner-assert.js.[review] gate passed · iteration 2 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file