assert: compare AggregateError.errors in deep equality - #35495
Conversation
|
Warning Review limit reached
Next review available in: 13 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 (2)
Comment |
|
Updated 1:18 AM PT - Jul 25th, 2026
❌ @robobun, your commit cda0d07 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35495That installs a local version of the PR into your bun-35495 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate check: both flagged PRs do cover this, but neither is a near-term merge.
This PR is the minimal standalone fix (14 lines in |
There was a problem hiding this comment.
LGTM — small Node-compat fix that mirrors the existing .cause block for .errors.
What was reviewed:
- Exception handling: each
get()and the recursiveBun__deepEqualscall is followed byRETURN_IF_EXCEPTION(scope, {}), matching the neighboring.causepattern. vm.propertyNames->errorsis a valid CommonIdentifier (already used at bindings.cpp:4598).- Double-visit when
.errorsis user-defined as enumerable: harmless (same as existing.causebehavior — both checks agree). - Tests slot into the existing
Case[]matrix and exercise all three entry points; expectations stated as cross-checked against Node v26.3.0.
Extended reasoning...
Overview
Adds a 14-line block to the ErrorInstanceType arm of specialObjectsDequal in src/jsc/bindings/bindings.cpp that fetches .errors from both sides and recurses into Bun__deepEquals, exactly mirroring the adjacent .cause handling (minus the strict-mode hasProperty gate, which the PR body justifies against Node's isEnumerableOrIdentical). Five new cases are appended to the cases matrix in test/js/node/assert/deep-equal.test.ts, which fans them out across assert.deepStrictEqual, assert.deepEqual, util.isDeepStrictEqual, and their negations.
Security risks
None. Pure comparison logic on already-held JS values; no I/O, allocation, or trust-boundary changes.
Level of scrutiny
Low-to-medium. This is a targeted Node-compat correctness fix in a well-understood code path. The new block is a near-verbatim copy of the block directly above it, uses the same gcBuffer/stack/scope threading, and every JSC call that can throw is guarded. vm.propertyNames->errors is confirmed to exist (used elsewhere in the same file). The one semantic difference from .cause — no hasProperty presence check — is documented in the code comment and PR body, and the "Error with non-enumerable own errors vs Error without" test case pins the resulting behavior.
Other factors
- The "AggregateError vs plain Error" case would already fail on
errorType()/name before reaching.errors, but it's still a useful regression guard. - If a user makes
.errorsenumerable, it will be compared twice (once here, once in the enumerable loop) — redundant but correct, and the same is already true for.cause. - Tests follow repo conventions: added to the existing table-driven file rather than a new file, no sleeps, no network, and the matrix runner already asserts
ERR_ASSERTIONon the negative path. - No prior human review comments; no outstanding feedback to address.
Node's util.isDeepStrictEqual and assert.deepStrictEqual compare the non-enumerable own .errors array on Error instances the same way they compare .cause. Bun compared .cause explicitly but skipped .errors, so two AggregateErrors wrapping entirely different inner errors (or a different number of them) compared as deep-equal. The .errors property is defined with PropertyAttribute::DontEnum by JSC's AggregateError constructor, so the enumerable-property walk in Bun__deepEquals never visited it. Add an explicit get-and-recurse for .errors right after the .cause block. Unlike .cause, Node does not gate .errors on hasOwn presence (a present-but-undefined .errors equals a missing one), so there is no hasProperty check.
898644e to
7cb74ba
Compare
There was a problem hiding this comment.
LGTM — mirrors the existing .cause handling in the same block, with proper exception checks.
What was reviewed:
- The new
.errorsblock follows the adjacent.causepattern (get()+RETURN_IF_EXCEPTION+ recursiveBun__deepEquals);vm.propertyNames->errorsis already used elsewhere in this file. - Checked whether omitting the
hasPropertygate (unlike.cause) diverges from Node in strict mode — ruled out; missing and present-undefined both read asjsUndefined()and compare equal, matching the PR's Node cross-check. - Test cases cover equal/unequal inner errors, differing counts, AggregateError vs plain Error, and a non-enumerable own
.errorson a plain Error.
Extended reasoning...
Overview
Adds an 11-line block to the ErrorInstanceType case of specialObjectsDequal in src/jsc/bindings/bindings.cpp that fetches .errors from both operands and recurses into Bun__deepEquals, exactly mirroring the .cause block immediately above it. Five new table-driven cases are added to test/js/node/assert/deep-equal.test.ts, which fans them out across assert.deepStrictEqual, assert.deepEqual, and util.isDeepStrictEqual.
Security risks
None. This is a read-only comparison path in node:assert/node:util; no new allocation, no untrusted-length arithmetic, no I/O.
Level of scrutiny
Low-to-moderate. The change is confined to the Node-compat deep-equality handler, is a direct copy of the adjacent .cause handling (same get / RETURN_IF_EXCEPTION / recurse sequence), and vm.propertyNames->errors is a pre-existing common identifier already used at bindings.cpp:4595. The one intentional divergence from .cause — no hasProperty presence gate in strict mode — was raised by a finder and ruled out: both a missing property and a present-undefined property read back as jsUndefined() via get(), so they compare equal, which matches the Node behavior the PR description documents and cross-checked.
Other factors
Tests were verified by the author's gate to fail on canary (9 failures under ASAN debug) and pass with the fix on both debug+ASAN and release. The comment-cop feedback (extraneous comment block) was addressed in cda0d07 and the thread is resolved. The duplicate-PR bot flagged two larger in-flight efforts; the author's rationale for landing this minimal fix independently is reasonable and doesn't affect correctness here.
|
CI status: build #80106 is done.
The earlier full run at 6dc2328 (build #79928, identical diff plus a since-removed code comment) also passed Diff is ready; the red is queue-backlog infra. Happy to rebase or retrigger if preferred. |
What
util.isDeepStrictEqual/assert.deepStrictEqual/assert.deepEqualnow compare the non-enumerable.errorsarray onErrorinstances, matching Node.js.Repro
Any test asserting which errors a
Promise.anyrejection carries was a false green on Bun regardless of the inner error content.Cause
AggregateErrordefines.errorswithPropertyAttribute::DontEnum(JSCAggregateError.cpp), so the enumerable-property walk inBun__deepEquals'sErrorInstanceTypehandler never visited it. The handler already special-cased the non-enumerable.causebut not.errors.Node handles both via
isEnumerableOrIdenticalinlib/internal/util/comparisons.js:Fix
Add an explicit get-and-recurse for
.errorsinbindings.cppright after the.causeblock. Unlike.cause, Node does not gate.errorsonhasOwnpresence (a present-but-undefined.errorsis equal to a missing one), so there is nohasPropertycheck.Verification
New cases in
test/js/node/assert/deep-equal.test.ts(runs throughassert.deepStrictEqual,assert.deepEqual, andutil.isDeepStrictEqual):errorsvs Error without: not equalAll expectations cross-checked against Node v26.3.0. Fails on canary (9 failures), passes with this change.
[stamp-90s] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file