Repository navigation
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 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)
WalkthroughAdds a new ChangesWorker execArgv validation
Sequence Diagram(s)sequenceDiagram
participant JSWorkerDOMConstructor
participant Bun__Worker__validateExecArgv
participant ErrorCode
participant WorkerTests
JSWorkerDOMConstructor->>Bun__Worker__validateExecArgv: validate execArgv StringImpl pointers
Bun__Worker__validateExecArgv->>ErrorCode: throw ERR_WORKER_INVALID_EXEC_ARGV on invalid entries
WorkerTests->>JSWorkerDOMConstructor: construct workers with valid/invalid execArgv
JSWorkerDOMConstructor-->>WorkerTests: accept or reject constructor
Compact Metadata
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:25 PM PT - Jul 10th, 2026
❌ @autofix-ci[bot], your commit 5434286 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33363That installs a local version of the PR into your bun-33363 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All my earlier nits are addressed (including --no-inspect in 4e83e3e) and I found nothing new this pass, but this introduces a behavioural tightening — previously-accepted execArgv values now throw — backed by ~120 hand-maintained Node flag-name entries and an unsafe C++→Rust FFI boundary, so it's worth a human sign-off on the accept/reject policy and the table contents.
Extended reasoning...
Overview
Adds ERR_WORKER_INVALID_EXEC_ARGV validation at new Worker(f, { execArgv }) construction to match Node.js. The bulk of the change is a new 605-line src/runtime/cli/worker_exec_argv.rs containing a flag classifier (lookup_long/lookup_exact/invalid_entries) plus four hand-maintained tables (PER_PROCESS_LONG, NODE_ONLY_BOOLEAN ~80 entries, NODE_ONLY_WITH_VALUE ~40 entries, NEGATABLE_WITH_OPTIONAL_VALUE) derived from Node v26.3.0's node_options.cc. JSWorker.cpp gains an extern "C" call into Bun__Worker__validateExecArgv via a reinterpret_cast<WTF::StringImpl**> over the local Vector<String>. ErrorCode.ts/ErrorCode.rs get the new error code appended. ~120 lines of new tests in worker_threads.test.ts plus Rust unit tests, and an unrelated debug-build timeout bump on the neighbouring "eval does not leak source code" test.
Security risks
None identified. Input is same-process JS (not a trust boundary), the classifier only reads static tables and byte slices, and the earlier stack-overflow-on-pathological-input I flagged was fixed (single non-recursive no- strip, covered by a 200k-repeat regression test). The FFI unsafe block dereferences WTF::StringImpl* from a caller-owned stack Vector<String> that outlives the call; the pattern mirrors Worker::create() and is guarded by a static_assert.
Level of scrutiny
Medium-high. This is a behavioural tightening: execArgv values Bun previously accepted silently now throw synchronously, so real user code (worker pools forwarding filtered process.execArgv, packages passing --max-old-space-size) can start failing. The accept/reject policy encodes several judgment calls a maintainer should ratify — accepting Bun-only flags Node rejects (--smol, --no-macros), the --use-system-ca per-environment carve-out, treating --inspect as boolean-negatable via a special-case list — and the NODE_ONLY_* tables are a new hand-maintained mirror of upstream Node source that will drift. The five rounds of edge-case fixes on this PR (positive boolean spellings, _→- canonicalisation, recursive no-, Bun-only negative flags, OneOptional negation) show the correctness surface is subtle.
Other factors
Test coverage is thorough (Rust unit tests including sortedness checks, JS it.each tables for both accept and reject paths, the differential against Node described in the PR body). All six of my prior inline findings were addressed with commits and pinned by tests; the last one (--no-inspect) is fixed in 4e83e3e via NEGATABLE_WITH_OPTIONAL_VALUE though the thread isn't yet marked resolved. No CODEOWNERS hit that I can see. The unrelated timeout bump on "eval does not leak source code" is reasonable and explained in the PR body.
|
All six inline findings are addressed and their threads are resolved (the snapshot above predates the last resolve). Summarising the judgment calls so they are easy to ratify or overrule, since they are the part that needs a human:
On table drift: a flag a later Node adds and The behavioural tightening is the point of the PR rather than a side effect — Not covered, for the record: Node also validates a worker's Rebased onto 55ba6e4 (main moved 115 commits since I branched). Two things touched this diff:
Post-rebase verification: CI status (5434286, build #71786): The only red is Ready for a maintainer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 384-394: The Worker test currently waits only for the "message"
event in the execArgv case, so startup regressions can hang until timeout
instead of failing fast. Update the test around the Worker creation and
once(worker, "message") await to also listen for the worker "error" event and
reject immediately on failure, so the assertion fails with the startup error
rather than waiting for a message that never arrives.
🪄 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: 8a0df085-7d68-4938-8bf1-1da84066e13b
📒 Files selected for processing (6)
src/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/webcore/JSWorker.cppsrc/runtime/cli/mod.rssrc/runtime/cli/worker_exec_argv.rstest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/worker_threads/worker_threads.test.ts (1)
353-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
Buffer.alloc(...).toString()instead of.repeat()for the large string.
"no-".repeat(200_000)builds a ~600KB string via repeated string ops, which the repo's own testing guideline calls out as slow in debug JSC builds. This string is only used as an array element (not passed to a byte-encoding API), so no exception applies here.As per coding guidelines: "To create repetitive strings in tests, use
Buffer.alloc(count, fill).toString()instead of"string".repeat(count)for better performance in debug builds."♻️ Proposed fix
- await expectRejected(["--" + "no-".repeat(200_000)], { + await expectRejected(["--" + Buffer.alloc(200_000 * "no-".length, "no-").toString()], {🤖 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 `@test/js/node/worker_threads/worker_threads.test.ts` around lines 353 - 357, The test in worker_threads.test.ts builds a very large repeated string with the negation argument, which should be changed to the repo-preferred Buffer-based approach for faster debug JSC performance. Update the repeated value used in expectRejected in the worker_threads test to use Buffer.alloc(...).toString() instead of the current repeat-based construction, keeping the same string content and preserving the existing assertion behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 353-357: The test in worker_threads.test.ts builds a very large
repeated string with the negation argument, which should be changed to the
repo-preferred Buffer-based approach for faster debug JSC performance. Update
the repeated value used in expectRejected in the worker_threads test to use
Buffer.alloc(...).toString() instead of the current repeat-based construction,
keeping the same string content and preserving the existing assertion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08b045fe-ab0a-4ca0-826f-4a4a47a43efd
📒 Files selected for processing (1)
test/js/node/worker_threads/worker_threads.test.ts
d3603d4 to
f7a05a4
Compare
new Worker(file, { execArgv }) accepted any array of strings and echoed it
back through the worker's process.execArgv, even though Bun reads only
--no-addons out of it. A typo, or a flag that can never apply to a thread,
passed silently.
Parse execArgv the way Node does and throw ERR_WORKER_INVALID_EXEC_ARGV for
unrecognised names, per-process flags and engine flags. Flags Bun's CLI
accepts stay accepted, and so do the Node flags Bun ignores, so nothing that
Node allows in execArgv is rejected.
clippy's disallowed-methods bans String::from_utf8_lossy; format the joined names through bstr::BStr instead.
…n, and single `no-` strip Three divergences from Node, all found by review: - Node registers one canonical name per boolean option and derives the other spelling, so `--warnings` and `--no-warnings` both name the same option. Our tables carry the documented spelling, so look up the negation too. - Node canonicalises `_` to `-` in a long option's name before looking it up. - Node strips `no-` exactly once. Recursing per prefix also overflowed the stack on `--` + "no-".repeat(200000), crashing the process instead of reporting the flag. The ERR_WORKER_INVALID_EXEC_ARGV tests now terminate the worker that is born when the constructor fails to throw, so a regression fails the assertion instead of leaking a thread into every later test. The heavy eval-source leak test gets a debug-build timeout: its fixture hands 600 MiB of source to six workers, which takes ~30s under a debug build and always blew the 5s default.
…e-only flags The reverse `no-` lookup went through AUTO_PARAMS, so Bun flags that only have a negative name (`--no-macros`, `--no-clear-screen`, `--no-orphans`, `--no-exit-on-error`) made up positive spellings that neither Bun nor Node knows. Spell NODE_ONLY_BOOLEAN with the name Node registers each option under instead, which is the positive one for a default-true option, so the existing `no-` strip derives the other spelling and no reverse lookup is needed.
It shares Bun's CA-store group with --use-bundled-ca and --use-openssl-ca, but Node registers it in EnvironmentOptionsParser rather than PerProcessOptionsParser, so it is the one of the three Node accepts in a worker's execArgv. Pin all three in the tests so the asymmetry is not mistaken for an oversight.
Bun declares --inspect, --inspect-brk and --inspect-wait as <STR>?, so the no- strip classified --no-inspect as negating a non-boolean and threw. Node registers all three as plain booleans whose address is a separate implied --inspect-port, and accepts --no-inspect. Only those three, not every <STR>? flag: Bun's --config really does name a value, and Node rejects --no-config just as it rejects --no-inspect-port.
Awaiting only "message" meant an accepted flag that broke worker startup hung until the test timed out. Reject on "error" so the failure names itself.
…nd heap-dump tests Seven tests from #31216 spawn a subprocess that builds a chain of workers (or take a full V8-format heap snapshot) and need 5.6-11.2s under `bun bd`, well past the 5s default. They fail the same way on clean main. Same treatment as the `eval does not leak source code` timeout earlier in this PR; no assertions changed.
f7a05a4 to
b1b4e2f
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/worker_threads/worker_threads.test.ts:1-8— Theconst subprocessTimeout = ...declaration (and its comment) sits between theharnessimport and thenode:events/node:fs/worker_threadsimports, breaking the file's contiguous-imports-at-top convention. Purely cosmetic (ES imports hoist, soisDebugis defined regardless) — moving the const below the last import restores it.Extended reasoning...
What this is
The diff changes the top of
worker_threads.test.tsfrom a single contiguous block of imports to:import { bunEnv, bunExe, isDebug, tmpdirSync } from "harness"; // A debug build starts a worker VM about an order of magnitude more slowly, so // the tests that spawn a subprocess that then builds a chain of workers need // headroom past the 5s default to finish under `bun bd`. const subprocessTimeout = isDebug ? 60_000 : 20_000; import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; import { Readable } from "node:stream"; import wt, { ... } from "worker_threads";
Before this PR, all
importstatements were contiguous at the top of the file with no code interleaved. The newconst(with its 3-line comment) is wedged between the first import and the remaining five.Why it's flagged
Root CLAUDE.md's "Code style & idioms reviewers enforce" section says: "Match the exact file's local conventions: the namespace aliases the neighboring lines actually use, import placement, canonical parameter names, ...". This file's local convention was contiguous imports at the top, and the new line is a fresh addition in this PR (not pre-existing), so it's fair to point at.
Why it doesn't matter behaviorally
ES module
importdeclarations are hoisted: all module bindings are established before any top-level statement executes. So even thoughconst subprocessTimeout = isDebug ? ...textually precedesimport { once } from "node:events", at runtimeisDebugis already bound (from the hoistedharnessimport) when the initializer runs, and the later imports are already resolved too. There is zero behavioral difference between the current placement and the conventional one.Step-by-step
- Module linking phase: all six
importdeclarations are hoisted and their bindings (bunEnv,bunExe,isDebug,once,fs, ...) established. - Module evaluation phase: top-level statements execute in source order. The first non-import statement is
const subprocessTimeout = isDebug ? 60_000 : 20_000;. isDebugis a live binding from step 1 → evaluates fine →subprocessTimeoutis assigned.- All subsequent uses of
subprocessTimeout(as the third argument totest(...)/it(...)at lines ~334, ~638, ~1495, etc.) read the same value they would if the const were below the last import.
Identical observable behavior either way.
Impact & fix
Purely cosmetic. The placement has a defensible rationale — proximity to the
isDebugimport it depends on — but it reads oddly (a blank line, a comment, a const, then five more imports with no separating blank line) and diverges from both the file's prior state and the repo-wide norm.Fix: move the comment and
const subprocessTimeout = ...to immediately after thefrom "worker_threads"import block, so all imports stay contiguous:import { bunEnv, bunExe, isDebug, tmpdirSync } from "harness"; import { once } from "node:events"; ... import wt, { ... } from "worker_threads"; // A debug build starts a worker VM about an order of magnitude more slowly, ... const subprocessTimeout = isDebug ? 60_000 : 20_000;
Non-blocking; nothing breaks if merged as-is.
- Module linking phase: all six
There was a problem hiding this comment.
No new issues found — the six earlier inline findings are all addressed and resolved. Deferring to a human because this is a ~600-line new validator that tightens user-visible behaviour (previously-accepted execArgv entries now throw), and the author's own summary explicitly lists design judgment calls for a maintainer to ratify.
Checked: the FFI boundary in JSWorker.cpp (StringImpl* lifetime, RETURN_IF_EXCEPTION placement, null-impl guard); lookup_canonical_long's single-strip no- handling and _→- canonicalisation against Node's parser rules; that the sorted tables' binary_search invariant is unit-tested; and that the timeout-only edits to eight existing tests leave their assertions unchanged.
Extended reasoning...
Overview
Adds Node-compatible ERR_WORKER_INVALID_EXEC_ARGV validation to new Worker(_, { execArgv }). A new 606-line src/runtime/cli/worker_exec_argv.rs classifies each flag against Bun's own AUTO_PARAMS table plus three hand-maintained tables (~150 entries) mirroring Node v26.3.0's PerIsolateOptionsParser. JSWorker.cpp calls into it over FFI before moving the strings into WorkerOptions. One-line ErrorCode.ts append; ~130 new test lines plus debug-build timeout headroom on eight existing subprocess/heap-dump tests.
Security risks
None material. Input is same-process JS constructing a Worker (not a trust boundary). The one previously-identified DoS shape — unbounded no- recursion overflowing the stack — was fixed and has both a Rust unit test and a JS regression test. The reinterpret_cast<WTF::StringImpl**> matches the existing pattern in Worker::create and is guarded by a static_assert; the Rust side null-checks each impl and owns its transcoded copies.
Level of scrutiny
High. This is a behavioural tightening: execArgv values Bun silently accepted now throw synchronously at construction. That is the point (Node parity), but it can break code that today forwards flags Bun ignores. The classification logic is subtle — no- derivation direction, _ canonicalisation, Values::OneOptional negation, per-process vs per-environment split — and took six review rounds to converge. The three static tables mirror an external source of truth (Node's option registry) and will drift; the author documented that drift fails loud, but the maintenance burden is real.
Other factors
The author's own summary comment explicitly tables four design decisions "so they are easy to ratify or overrule, since they are the part that needs a human" — accepting Bun-only flags, tolerating Node-only flags Bun ignores, the --use-system-ca split, and the --no-inspect/--no-config asymmetry. That is a direct request for maintainer sign-off on policy, which puts this outside auto-approval scope regardless of implementation quality. Test coverage is strong (Rust unit tests + it.each matrices on both accept and reject sides, differential-tested against live Node), and all prior inline findings are resolved with pinning tests.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-11, 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. |
1 participant
Repro
Node throws
ERR_WORKER_INVALID_EXEC_ARGVsynchronously. Bun starts the worker and the bogus flags show up verbatim in itsprocess.execArgv.Cause
JSWorker.cppcoerced everyexecArgventry to a string and stored the array unvalidated. Bun reads only--no-addonsback out of it (parse_worker_exec_argv_allow_addons), so everything else is inert but still echoed back through the worker'sprocess.execArgv. Code that relies on the constructor throwing to detect unsupported runtime flags (pools that forward a filteredprocess.execArgv) silently proceeds: the classic "my--max-old-space-sizeisn't being applied in workers" report, with no error anywhere.Fix
Validate
execArgvat construction and throwERR_WORKER_INVALID_EXEC_ARGV, as Node does inWorker::New. Only an explicitly passedexecArgvis checked; an inherited one is not, matching Node.New
src/runtime/cli/worker_exec_argv.rsclassifies each entry against:AUTO_PARAMStable — the same tableprocess.execArgv's re-parser derives its value-consuming set from — so every flagbun <entrypoint>accepts keeps working.--experimental-vm-modules,--warnings,--tls-min-v1.2, …), taken from Node v26.3.0'sPerIsolateOptionsParser. Bun ignores them, but Node allows them inexecArgv, so rejecting them would be stricter than Node.--title,--zero-fill-buffers,--use-openssl-ca,--expose-gc,--stack-trace-limit, …). Node rejects these fromexecArgvtoo.Node's parsing rules are reproduced: parsing stops at the first positional; a value-taking flag consumes the next entry only when that entry is not itself a flag;
_is canonicalised to-in a long flag's name; every boolean is registered under one canonical name, with--no-<name>derived from it by strippingno-exactly once (including--no-inspect, which Node registers as a boolean); and parse errors ("--conditionsrequires an argument") are reported in preference to unrecognised names.Bun-only flags (
--smol,--hot,--port,--no-macros) stay accepted. Node rejects them only because it has no such flag, and users forward them throughprocess.execArgvtoday.Verification
A differential of the validator against real Node v26.3.0 over 105 inputs (the repro, engine flags, per-process flags, both boolean spellings, underscore spellings, doubled
no-prefixes, negations, missing values, positionals,--, short flags) produces zero differences. The remaining deliberate divergences are flags Bun's CLI has and Node does not (--no-macros,--install,--no-env-file, …), and:["--stack-trace-limit", "7", "--bogus"]names only--stack-trace-limitwhere Node names both. Both throw the same error.With
src/reverted to the merge-base, 19 of those fail.test/js/web/workers/worker.test.ts,test-no-addons-resolution-condition.jsandtest-trace-exit.js(all of which passexecArgvto aWorker) still pass.Also in this PR
ERR_WORKER_INVALID_EXEC_ARGVtests terminate the worker that gets born when the constructor fails to throw, so a regression fails the assertion instead of leaking a thread into every test that follows. The accepting tests wire the worker'serrorevent torejectso a startup failure names itself instead of hanging until the timeout.worker_threads.test.tsget a debug-build timeout. Each spawns a subprocess that builds a chain of workers (or takes a full V8-format heap dump) and needs 5.6-11.2s underbun bd, well past the 5s default; they fail identically on clean main. Same treatment asfetch-leak.test.ts. No assertions changed.Rebase note
#33947 made
ErrorCode.rsgenerated fromErrorCode.ts, so the error code is now a one-line append there; no Rust-side edit remains.Not covered
Node also throws
ERR_WORKER_INVALID_EXEC_ARGVfor a badenv.NODE_OPTIONSpassed to a worker. Bun does not readNODE_OPTIONSat all, so that half oftest-worker-execargv-invalid.jsis out of scope here; theexecArgvcases from that file are covered inworker_threads.test.ts.