Conversation
|
Updated 10:05 PM PT - Jul 6th, 2026
❌ @robobun, your commit b0e29b3 has some failures in 🧪 To try this PR locally: bunx bun-pr 33567That installs a local version of the PR into your bun-33567 --bun |
|
Warning Review limit reached
Next review available in: 23 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)
WalkthroughThis PR simplifies Node.js's Changesassert.fail Simplification
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fail as assert.fail
participant AssertionError
Caller->>fail: fail(message?)
alt message is Error
fail->>Caller: throw message
else message is string or undefined
fail->>AssertionError: new AssertionError({actual: undefined, expected: undefined, operator: "fail", stackStartFn: fail})
AssertionError-->>fail: error instance
fail->>Caller: throw error
end
Related PRs: None identified. Suggested labels: Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/assert.ts (1)
120-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForce
generatedMessageto reflect argument presence, not message truthiness.
AssertionErrorcurrently setsgeneratedMessage = !message, so explicit falsy messages like"",0, orfalsestill become generated. That violates the new contract that only zero-argumentassert.fail()is generated.🐛 Proposed fix
- const err = new AssertionError(errArgs); - if (internalMessage) { - err.generatedMessage = true; - } + const err = new AssertionError(errArgs); + err.generatedMessage = internalMessage;🤖 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 `@src/js/node/assert.ts` around lines 120 - 123, The generatedMessage flag in AssertionError creation should depend on whether a message argument was actually provided, not on the message value’s truthiness. Update the assert path around AssertionError and the internalMessage check in assert.ts so generatedMessage is set only for zero-argument fail/assert cases, while explicit falsy messages like empty string, 0, or false remain non-generated.
🤖 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 `@src/js/node/assert.ts`:
- Line 110: Update the message-type checks in assert.ts so they use
Error.isError instead of instanceof Error in the assert.fail and innerFail
paths, since instanceof misses cross-realm errors. Make sure the logic still
rethrows real Error objects directly, and add coverage for a node:vm-created
error case so assert.fail(vmError) is handled correctly.
In `@test/js/node/assert/assert-fail.test.ts`:
- Around line 1-93: This Bun-only assert.fail coverage should be moved into the
existing assert test suite instead of living in a standalone file. Relocate the
cases into the current assert tests alongside the other assert.fail coverage,
keeping the same capture helper and the assert.fail-related checks so the
behavior remains discoverable in the main suite. Use the existing assert test
file and the assert.fail symbol to place these new expectations where the rest
of the assert API tests already live.
- Around line 68-70: Tighten the `assert.fail` test so it verifies object
identity rather than matcher behavior: in the `throws the first argument when it
is an Error` case, catch the thrown value from `assert.fail(err)` and assert it
is the exact same `Error` instance with `toBe(err)`. Keep the change localized
to the `assert-fail.test.ts` test block using the existing `assert.fail` and
`err` symbols.
- Around line 79-89: The test in assert-fail.test.ts is using a broad stderr
sentinel by asserting that stderr does not contain “WARNING”, which can fail on
unrelated benign output. Update the warning-checking logic around the
assert.fail(1, 2) process case to use a more specific sentinel for process
warnings (or rely on the nonzero exit behavior together with the existing
DEP0094 assertion), and keep the assertion tied to the relevant warning path
rather than generic stderr content.
---
Outside diff comments:
In `@src/js/node/assert.ts`:
- Around line 120-123: The generatedMessage flag in AssertionError creation
should depend on whether a message argument was actually provided, not on the
message value’s truthiness. Update the assert path around AssertionError and the
internalMessage check in assert.ts so generatedMessage is set only for
zero-argument fail/assert cases, while explicit falsy messages like empty
string, 0, or false remain non-generated.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52faf149-877c-4447-b35a-8f65ac4cedad
📒 Files selected for processing (2)
src/js/node/assert.tstest/js/node/assert/assert-fail.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…y deprecation test
There was a problem hiding this comment.
LGTM — all prior feedback addressed; implementation now matches Node 26's assert.fail exactly.
Extended reasoning...
Overview
This PR updates assert.fail in src/js/node/assert.ts to match Node 26's removal of the end-of-life DEP0094 multi-argument form. The function now takes a single optional message argument: if it's an Error (checked via cross-realm-safe Error.isError), it's thrown directly; otherwise it becomes the AssertionError message with operator: 'fail' and actual/expected undefined. The legacy warned flag, process.emitWarning('DEP0094') call, and multi-argument message synthesis are removed. innerFail was also updated to use Error.isError. A new test file covers the full contract, and the now-obsolete vendored test-assert-fail-deprecation.js is deleted.
Security risks
None. Pure JavaScript, no native code, no I/O, no auth/crypto/permissions surface. The change only affects error-object construction in node:assert.
Level of scrutiny
Moderate — this is a user-facing behavioral change to a Node.js compat API, but it's a straightforward alignment with an upstream Node semver-major removal (DEP0094 went end-of-life). The final implementation is a near-verbatim port of Node's lib/assert.js fail().
Other factors
This PR went through three rounds of my review feedback plus CodeRabbit's, and all issues are now resolved and verified in the current diff:
message === undefinedcheck (notarguments.length) soassert.fail(undefined)yields'Failed'— fixed and tested.- Conditional
if (internalMessage) err.generatedMessage = truerestored so falsy messages ('',0,null) keepgeneratedMessage: trueper the AssertionError constructor — fixed and tested. - Vendored
test-assert-fail-deprecation.jsdeleted so CI doesn't break — done. Error.isErrorfor cross-realm errors, identity-preserving Error throw, unique stderr sentinel — all applied.
The new test file has comprehensive coverage (no-arg, explicit undefined, empty string, string message, multi-arg-ignored, Error identity, cross-realm Error, no-DEP0094-warning subprocess check) and follows harness conventions. No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; fail() now mirrors Node 26 exactly.
Extended reasoning...
Overview
This PR removes the end-of-life DEP0094 multi-argument form of assert.fail from src/js/node/assert.ts, replacing it with the Node 26 single-argument implementation: Error.isError(message) → rethrow; message === undefined → default "Failed"; fixed operator: "fail", actual/expected: undefined; conditional if (internalMessage) err.generatedMessage = true. It also switches innerFail's Error check to Error.isError, adds a focused test file test/js/node/assert/assert-fail.test.ts, and deletes the vendored test-assert-fail-deprecation.js (removed upstream when DEP0094 went EOL).
Security risks
None. This is a pure Node-compat behavioral sync in the assert module — no auth, crypto, filesystem, or network paths are touched. No untrusted input parsing.
Level of scrutiny
Low-to-moderate. The runtime change is ~15 lines that now match Node's lib/assert.js line-for-line. It went through three rounds of review here (the arguments.length vs === undefined regression, the stale vendored deprecation test, and the unconditional generatedMessage overwrite for falsy messages) — all fixed with regression tests. CodeRabbit's cross-realm Error.isError, identity assertion, and stderr-sentinel feedback were also applied.
Other factors
Test coverage is thorough: no-arg, explicit undefined, empty-string (falsy → generatedMessage: true), string message, same-realm Error identity, cross-realm vm Error identity, multi-arg ignored (three variants), and a spawned subprocess asserting no DEP0094 warning. The remaining vendored test-assert-fail.js only exercises single-arg forms, which remain correct. No outstanding reviewer comments; all threads resolved. The bug hunter found nothing on the latest revision.
|
Diff is green. All 257 test jobs that ran passed, including the new The only CI failure on build 69475 is unrelated infra: the This change is JS-only ( |
This branch has not been deployed
1 participant
What
Node 26 removed the end-of-life
DEP0094multi-argument behaviour ofassert.fail. Bun still implemented the legacy form, so every observable property of the thrownAssertionErrordiverged for calls with 2+ arguments, and Bun emitted aDeprecationWarningNode no longer emits.Repro
Node v26.3.0 (first arg is the message, rest ignored, no warning):
Bun before this change (legacy multi-arg form):
Cause
failinsrc/js/node/assert.tskept the deprecated branch: it synthesized"actual operator expected"messages, setactual/expected/operatorfrom the extra arguments, and calledprocess.emitWarning(..., "DEP0094").Fix
Mirror Node 26:
fail(message)uses only the first argument. If it is anErrorit is thrown; otherwise it becomes the message.operatoris always"fail",actual/expectedareundefined, andgeneratedMessageistrueonly when no argument is given. The legacy multi-argument synthesis and theDEP0094warning are removed.Verification
Added tests in
test/js/node/assert/assert.spec.ts. They pass with the fix and fail on the released build (legacyactual/expected/operatorand theDEP0094warning).