Deduplicate jsc bindings, runtime api, test matchers, and shell builtins - #32023
Deduplicate jsc bindings, runtime api, test matchers, and shell builtins#32023alii wants to merge 2 commits into
Conversation
|
@robobun adopt |
|
Retargeted at main by alii; rebased and mergeable at 306781e. Build 92360: green except an R2-side |
|
Updated 11:35 AM PT - Aug 11th, 2026
✅ @robobun, your commit d474a28fcb3df45e40c18b0e5a77305307a8646c passed in 🧪 To try this PR locally: bunx bun-pr 32023That installs a local version of the PR into your bun-32023 --bun |
fbe8f1a to
a1c67b5
Compare
4a6537d to
b6193e2
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a large refactor (53 files, net −5k lines) touching unsafe runtime internals across the VM event loop, IPC, NAPI, DNS, crypto, shell, and cron — worth a human pass to confirm the behavioral-equivalence claims hold across all the consolidations.
Extended reasoning...
Overview
This PR consolidates duplicated code across ~53 files in the Bun runtime: collapsing the TagPayload/Tag split in ConsoleObject, unifying the four PendingCacheKey types in DNS into one generic, merging auto_tick/auto_tick_active via a const generic, factoring shell builtins' OutputTaskVTable into a macro, restructuring CronRegisterJob/CronRemoveJob around a shared CronJobCommon base, deduping the CryptoHasher digest-output parsing, deleting the dead draft FFI implementation, and turning jsc::URL into a re-export of bun_url::whatwg::URL plus a UrlJsc extension trait. The diff is ~525k chars (truncated) with a net −5k lines.
Security risks
Several touched files are security-adjacent: csrf_jsc.rs (CSRF token generation/verification option parsing), CryptoHasher.rs and PasswordObject.rs (digest output handling, algorithm validation), and napi_body.rs (unsafe pointer/length handling for caller-supplied buffers). The changes look like straight extractions of existing logic into helpers with identical semantics, and the bug-hunting pass found nothing, but crypto/CSRF argument parsing and unsafe NAPI slice construction are exactly the kind of code where a subtle reordering or changed early-return can matter.
Level of scrutiny
High. Even granting the "zero intended behavior change" framing, this PR rewrites substantial unsafe Rust across the event loop (jsc_hooks.rs), intrusive-list DNS draining (drain_chain_ok/drain_chain_err), raw-pointer self-freeing state machines in cron.rs, and Stacked-Borrows-sensitive ManuallyDrop/ptr::read paths in subprocess Writable. The cron rework in particular merges two separate state enums and maybe_finished/finish paths into one generic — the kind of consolidation where a per-variant edge case (e.g. which states tolerate nonzero exit, which Drop runs) is easy to lose. These are production-critical hot paths, not sandbox code.
Other factors
CI is green across all lanes per the robobun comment, and the existing test suite is cited as regression coverage. The PR is also stacked (foundations → install-cli → this), so merge ordering matters. No prior human reviews are on the timeline. Given the breadth, the unsafe-heavy surface, and the security-adjacent files, this should get a human reviewer's eyes even if mechanically clean.
|
Re the specific high-scrutiny areas called out above, on the rebased head (b6193e2):
Build 64472 is at 281 jobs green with zero test-failure annotations; the only red is a darwin 26 lane that exited on a buildkite artifact-download timeout before running anything. Agreed a human pass is warranted given the scope; flagging @alii since this was handed over via adopt. |
a1c67b5 to
2f5dd36
Compare
b6193e2 to
f29ce8f
Compare
There was a problem hiding this comment.
The automated bug hunt came back clean, but this needs a human pass — 52 files across the event loop, JSC GC/formatter, DNS, IPC, NAPI, sockets, cron, and shell, with two rounds of non-trivial conflict resolution against main.
Checked and ruled out: the merged auto_tick<ACTIVE> preserves both original tails (GC-timer / rejected-promises skipped only when ACTIVE); drain_chain_ok keeps the ensure_still_alive bracketing and per-global re-materialization from every original drain_pending_*; the cron CronJobCommon extraction preserves the raw-ptr / no-FnEntry-protector discipline and per-job accepts_nonzero_exit differences; digest_to_bytes still returns the freshly-written digest bytes after the output_digest extraction.
Extended reasoning...
Overview
Net −5k line deduplication refactor across 52 runtime files: collapses TagPayload→Tag in the console formatter (shared with macro evaluator and test-runner pretty-printer), unifies four near-identical PendingCacheKey types in DNS behind a generic + HasTail/NameKeyed traits, extracts drain_chain_{ok,err} from five drain_pending_* copies, merges auto_tick/auto_tick_active into one const-generic fn, folds CronRegisterJob/CronRemoveJob state into a CronJobCommon base, macro-izes the shell builtins' OutputTaskVTable impl, extracts shared IPC drain loops, deletes the dead draft FFI implementation, and moves URL to a re-export with a UrlJsc extension trait. Two rebases onto main with 11+5 documented conflict resolutions.
Security risks
No new attack surface — this is code motion and helper extraction. The touched areas include NAPI string creation (unsafe slice construction from C pointers) and CSRF option parsing, but both are straight extractions of existing validation into helpers with identical guard order. No parsing/auth/crypto logic changed.
Level of scrutiny
High. This is production-critical native runtime code with pervasive unsafe, raw-pointer receivers whose contracts depend on Stacked-Borrows reasoning (cron's "local reborrow, no FnEntry protector" pattern), GC-rooting order (ensure_still_alive placement in DNS drain chains), and event-loop hot paths. The refactor's correctness hinges on every extracted helper preserving side-effect order and every conflict resolution correctly threading main's concurrent semantic changes (e.g. Rc<Handlers> in socket_body, transfer_to_js in napi) through the moved code. A subtle divergence in any one of the ~30 extractions could ship a UAF or a lost error path that the test suite doesn't cover.
Other factors
CI is green (281+ jobs) and the PR description documents per-file test verification, but the author's own bot already flagged that a human pass is warranted. The diff is stacked on another PR (install-cli) with a required merge order. Several extractions change control-flow shape in ways that are equivalent but not mechanically obvious — e.g. PasswordObject's verify_sync previously returned JSValue::ZERO on one dead-code branch and now returns Err(Thrown), and Mkdir::on_io_writer_chunk previously panicked on Idle/Done and now falls through to next(). These looked benign on inspection but are exactly the kind of thing a maintainer should confirm.
|
On the two specific control-flow shape changes called out above: PasswordObject Mkdir Build 72447 on the rebased head: 82 passed so far, one |
|
@robobun split this into a few prs. it's too big to land safely. |
|
On it. Splitting by subsystem into separate PRs, each stacked on |
|
Split into eight PRs, all stacked on
Each body carries the rebase-resolution notes relevant to its files and the suites run against it. Converting this one to a draft and leaving it open as the validated union for reference; happy to close it outright once the splits start landing. |
2f5dd36 to
9ae3ff5
Compare
f29ce8f to
1aba1ea
Compare
|
Re-landed directly on The PR description now lists what was dropped because main already has it or it depended on #32022, and which parts were re-derived against main (matchers on the #32024 is still stacked on this branch and needs the same treatment on its own. |
|
@robobun get this mergeable — main moved and it conflicts again. |
|
On it. Checking whether the base moved under the splits too; will rebase whatever conflicts and report back here. |
Consolidates duplicated code across the jsc and runtime crates without changing behavior: - ConsoleObject: drop the TagPayload mirror of Tag; TagResult carries the custom-formatter payload as an Option. Shared with the macro evaluator and the test-runner pretty printer. WrappedWriter::new replaces 32 struct-literal constructions; the per-function pf! macros go away. - URL: bun_jsc::URL becomes a re-export of bun_url::whatwg::URL (which gains the getters and destroy() that only the jsc copy had); the JSValue entry points move to the UrlJsc extension trait. - VirtualMachine: one wait loop for load_entry_point and the test-runner variant. jsc_hooks: auto_tick and auto_tick_active become one const-generic function. - bun:test: toEqual/toStrictEqual share one implementation; the toHave*With matcher family shares argument comparison, mock.results parsing and the failure epilogues, written against the throw! macro so user data is never passed through the color-tag rewrite. - Shell: basename/dirname share PathBuiltin; the OutputTaskVTable impls of cp/ls/mkdir/touch come from one macro; the lex/parse testing APIs share their argument prologue; subproc Writable/Readable use one match with per-arm cfg instead of two platform copies, and the Blob/ArrayBuffer stdin writer is shared with Bun.spawn. - cron: the register and remove jobs share CronJobCommon and CronJobBase. - dns: one generic PendingCacheKey and one pending-chain drain instead of a copy per record kind; CacheHit is now an alias of LookupCacheHit. - valkey: the seven command-shape macros become cmd! and cmd_varargs!. - Smaller dedups in AsyncModule, CryptoHasher, PasswordObject, csrf, HashObject, MarkdownObject, JSON5Object, napi string creation, bake transpiler setup and DevServer field exhaustiveness checks, udp setsockopt helpers, ipc message draining, socket event callbacks, ServerWebSocket publish prologue and CodeCoverage line mapping.
1aba1ea to
306781e
Compare
|
Mergeable again at 306781e. Only One question: the eight splits (#37358-#37366) are still based on |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given the scope — 53 files across the event loop (auto_tick), DNS pending-cache drains, cron process lifecycle, NAPI FFI, shell subprocess/builtin vtables, and the test-runner matchers, much of it restructuring unsafe raw-pointer code — a human review is still warranted, which lines up with the earlier call to split it.
What was reviewed: the auto_tick<const ACTIVE> merge preserves the three ACTIVE-gated skips and the debug sleep-timer logging path; the shared wait_for_entry_point_promise keeps the test-runner-only wakeup() outside the loop; the DNS drain_chain_ok/err skeletons preserve the head/tail consumption order and per-node next snapshotting; the cron CronJobCommon keeps Drop inside the enter/exit scope and the raw-ptr "borrow ends before free" discipline; the verify_sync Ok(ZERO)→Err(Thrown) and mkdir Idle/Done shape changes were checked against the host_fn boundary and next() respectively.
Extended reasoning...
Overview
This PR is a large deduplication pass across 53 files (+3294 / −5031): ConsoleObject TagPayload→Tag, bun_jsc::URL re-exported from bun_url, auto_tick/auto_tick_active merged behind a const generic, load_entry_point wait loop shared, the toHave*With matcher family sharing failure epilogues and mock-result parsing, shell builtins sharing PathBuiltin/impl_output_task_vtable!, subproc Writable/Readable::init collapsed to one match with per-arm cfg, cron register/remove sharing CronJobCommon, DNS PendingCacheKey<Req> + drain_chain_ok/err, valkey command macros collapsed to two, plus a long tail of smaller extractions (AsyncModule error builders, CryptoHasher digest args, csrf option parsing, napi string prologue, bake init_transpiler_impl, udp setsockopt helpers, socket callback tail, ServerWebSocket publish prologue). It has been rebased across ~800 main commits with 30 conflicting files re-derived.
Security risks
No new attack surface is introduced; this is a refactor. The security-adjacent files (csrf_jsc.rs, CryptoHasher.rs, PasswordObject.rs) only extract shared argument-parsing helpers with identical validation. No TLS/auth/permission logic is touched.
Level of scrutiny
High. The bulk of the change is in memory-safety-sensitive native code governed by the repo's most-blocked review category: intrusive raw-pointer chains in DNS (drain_chain_ok/err, HasTail::append_node), the cron *mut Self → heap::take state machines with their "borrow ends before free" invariants, NAPI FFI slice construction, subprocess Stdio payload extraction via ManuallyDrop+ptr::read, and the per-thread event-loop tick. Each of these was two nearly-identical copies before; merging them means any subtle divergence between the copies (intentional or accidental) is now a single behavior. The PR description documents byte-for-byte differential checks for the user-visible outputs, and the bug-hunting pass found nothing, but the sheer breadth of unsafe-code restructuring across a 30-file conflict rebase is exactly the kind of change the review guidelines say to treat as guilty until proven behavior-preserving.
Other factors
The maintainer explicitly said on this thread that it is "too big to land safely" and requested a split (eight split PRs exist at #37358–#37366), then later asked to get it mergeable again — so there is an unresolved process question about whether this lands as one PR or via the splits. There is also an open question from robobun to the maintainer about closing vs. retargeting those splits. No new tests are added (by design, since no behavior change is intended), so regression coverage is entirely the existing suites plus the differential scripts described in the PR body. Given all of this, deferring to human review is the right call; approving would bypass a decision the maintainer has not yet made.
|
Agreed on all counts, and the process question is the real blocker here, not the code. To keep it in one place for @alii:
Either way the code is the same bytes; it is just a question of review granularity. |
|
Warning Review limit reached
Next review available in: 20 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 (1)
WalkthroughThis PR consolidates shared helpers across URL handling, console formatting, runtime APIs, networking, shell execution, test matchers, Valkey commands, and coverage reporting. ChangesRuntime refactors
Possibly related PRs
Suggested reviewers: 🚥 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/runtime/shell/shell_body.rs (1)
782-796: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the argument-count error text identify the calling API.
shell_cmd_args_from_jsis now shared byshell_lexandshell_parse. Line 785 hard-codesshell_parsein the message, so ashell_lexcall with no arguments reports the wrong API name. Line 794 uses a third wording,shell, for the same failure class in the same function.Pass the entry-point name into the helper and use one wording for both missing arguments.
As per coding guidelines: "Error messages must identify the failed resource, violated constraint, rejected value, cause, and concrete remedy while preserving rich underlying errors."
🐛 Proposed fix to name the calling API
fn shell_cmd_args_from_js( global: &JSGlobalObject, callframe: &CallFrame, + api_name: &str, marked_argument_buffer: &mut MarkedArgumentBuffer, ) -> JsResult<(Bump, JsStrings, Vec<JSValue>, Vec<u8>)> { // SAFETY: bun_vm() is non-null for a Bun-owned global. let vm = global.bun_vm(); let mut arguments = jsc::ArgumentsSlice::init(vm, callframe.arguments()); let string_args: JSValue = match arguments.next_eat() { Some(s) => s, None => { - return Err(global.throw(format_args!("shell_parse: expected 2 arguments, got 0"))); + return Err(global.throw(format_args!( + "{api_name}: expected 2 arguments, got 0" + ))); } }; let arena = Bump::new(); let template_args_js: JSValue = match arguments.next_eat() { Some(s) => s, None => { - return Err(global.throw(format_args!("shell: expected 2 arguments, got 0"))); + return Err(global.throw(format_args!( + "{api_name}: expected 2 arguments, got 1" + ))); } };Update both call sites:
- let (arena, mut jsstrings, jsobjs, script) = - shell_cmd_args_from_js(global, callframe, marked_argument_buffer)?; + let (arena, mut jsstrings, jsobjs, script) = + shell_cmd_args_from_js(global, callframe, "shell_lex", marked_argument_buffer)?;- let (arena, mut jsstrings, mut jsobjs, script) = - shell_cmd_args_from_js(global, callframe, marked_argument_buffer)?; + let (arena, mut jsstrings, mut jsobjs, script) = + shell_cmd_args_from_js(global, callframe, "shell_parse", marked_argument_buffer)?;🤖 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/runtime/shell/shell_body.rs` around lines 782 - 796, Update shell_cmd_args_from_js to accept the calling entry-point name from shell_lex and shell_parse, then use that name in both missing-argument errors. Replace the inconsistent shell_parse and shell wording with one shared message that identifies the API, expected argument count, and concrete remedy.Source: Coding guidelines
src/runtime/valkey_jsc/js_valkey_functions.rs (2)
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the rejected argument position in the
cmd_varargs!error.The error reports
"additional arguments"for every rejected element. The caller cannot tell which argument failed. The repository guideline requires that error messages identify the rejected value and the violated constraint.Include the zero-based index of the failing argument in the message.
♻️ Proposed change to report the argument position
- for arg in arguments { + for (arg_index, arg) in arguments.iter().enumerate() { if $skip_null { if arg.is_undefined_or_null() { continue; } } let Some(another) = from_js(global, *arg)? else { return Err(global.throw_invalid_argument_type( bname($name), - "additional arguments", + &format!("argument {arg_index}"), "string or buffer", )); }; args.push(another); }Adjust the second parameter to match the exact type
throw_invalid_argument_typeaccepts; if it requires&'static str, add an overload or pass a formatted message through the existing error path.As per coding guidelines: "Error messages must identify the failed resource, violated constraint, rejected value, cause, and concrete remedy while preserving rich underlying errors."
🤖 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/runtime/valkey_jsc/js_valkey_functions.rs` around lines 255 - 261, Update the rejected-argument handling in the cmd_varargs! expansion around from_js so throw_invalid_argument_type identifies the failing zero-based argument index instead of using the generic "additional arguments" label. Preserve the existing string-or-buffer constraint and ensure the message uses the exact parameter type accepted by throw_invalid_argument_type, extending the existing error path only if needed to format the indexed label.Source: Coding guidelines
215-263: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe
cmd_varargs!mode selection changes JS-visible null handling and error shape across the migrated Valkey commands. The root cause is the new three-mode contract (required/skip_null/strict) in the macro definition: it fixes both howundefined/nullarguments are treated and which error the caller receives, and every registration below inherits that behavior from the mode it selects. The PR states no intended behavior change, so each mode assignment needs verification against the macro it replaced.
src/runtime/valkey_jsc/js_valkey_functions.rs#L215-L263: confirm thatskip_null'scontinueon line 251 matches the previousbreaksemantics used by the hand-written equivalents (set,srem,sadd,hmget), and thatstrictrejecting trailingundefined/nullthrough thefrom_jsNonebranch on line 255 matches the prior behavior for commands with optional trailing parameters.src/runtime/valkey_jsc/js_valkey_functions.rs#L1041-L1641: audit each registration's mode choice; confirm therequiredmode'sthrow_missing_arguments_valueerror replaces the previousthrow_invalid_argument_typewithout breaking tests, and re-checkhscanon line 1049, whereskip_nullcan drop a required cursor argument.🤖 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/runtime/valkey_jsc/js_valkey_functions.rs` around lines 215 - 263, The cmd_varargs! mode choices may change null handling and error behavior from the migrated hand-written commands. In src/runtime/valkey_jsc/js_valkey_functions.rs:215-263, verify skip_null preserves prior break semantics for set, srem, sadd, and hmget, while strict preserves trailing null/undefined rejection for commands with optional parameters. In src/runtime/valkey_jsc/js_valkey_functions.rs:1041-1641, audit every registration’s mode, preserve required-argument validation behavior and error compatibility, and ensure hscan does not use skip_null in a way that drops its required cursor argument.src/runtime/api/cron.rs (1)
2161-2212: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInstall process ownership before reader startup
When reader startup fails,
spawn_cmd_preparereturns beforebase!().processis set.T::finishthen has no process to detach. On POSIX, the child is not watched or reaped. On Windows,WindowsSpawnResult::Dropcleans pipes but does not release itsprocesspointer. Create and store theProcessbefore reader startup, while extracting POSIX stdio fields before the consumingto_processcall.🤖 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/runtime/api/cron.rs` around lines 2161 - 2212, Update spawn_cmd_prepare to construct and assign base!().process before starting either stdout_reader or stderr_reader, ensuring T::finish can detach the process if reader startup fails. For POSIX, extract the required spawned stdio fields before the consuming to_process call, then store the resulting Process before invoking reader startup; preserve the existing Windows pipe extraction and startup flow while installing ownership beforehand.Source: Coding guidelines
src/runtime/socket/udp_socket.rs (1)
944-959: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winDo not retain a native socket reference across
parse_addr.
parse_addrcallsto_bun_string, which can run user JavaScript. That code can close the UDP socket afterrequire_socketreturns. The later membership call can then use a closed native socket. Parse all optional addresses first. Then callrequire_socketimmediately before the native socket operation.
src/runtime/socket/udp_socket.rs#L944-L959: acquire the socket after parsing and validating the optional interface.src/runtime/socket/udp_socket.rs#L1030-L1045: acquire the socket after parsing and validating the optional interface.As per coding guidelines, “Assume any operation that can run user JavaScript can synchronously free state; perform coercions first, revalidate liveness after callbacks, and protect teardown-sensitive entry points.”
🤖 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/runtime/socket/udp_socket.rs` around lines 944 - 959, In the UDP membership operation, move require_socket in the relevant method at src/runtime/socket/udp_socket.rs lines 944-959 until after parse_addr and interface-family validation, immediately before set_membership; apply the same ordering in the corresponding method at src/runtime/socket/udp_socket.rs lines 1030-1045. Ensure no native socket reference is retained across parse_addr’s JavaScript coercion, while preserving existing validation and error handling.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.
Inline comments:
In `@src/runtime/api/bun/subprocess/Writable.rs`:
- Around line 19-48: Update the documentation for buffered_stdin_writer to state
that consuming Stdio::ArrayBuffer leaves
Stdio::ArrayBuffer(ArrayBufferStrong::default()), while Stdio::Blob is replaced
with Stdio::Ignore. Keep the implementation unchanged and explicitly note that
callers do not inspect the postcondition after Writable::init.
In `@src/runtime/server/ServerWebSocket.rs`:
- Around line 311-314: Update the closed-server log in publish_prologue to use
the log_name parameter instead of hardcoding “publish() closed”, preserving the
existing formatted per-method logging convention for publish and publishBinary.
In `@src/runtime/socket/udp_socket.rs`:
- Around line 838-844: Update require_socket to return
throw_setsockopt_errno(global_this, SystemErrno::EBADF) when self.socket.get()
is None, replacing the generic “Socket is closed” error; preserve the existing
socket dereference path for present sockets so all setsockopt callers retain the
required errno contract.
In `@src/runtime/valkey_jsc/js_valkey_functions.rs`:
- Around line 186-201: Update the argument handling in the cmd! macro to
validate frame.arguments().len() against the declared parameters and reject
extra arguments with the established arity error. For commands such as BITCOUNT
that support optional arguments, use cmd_varargs! so start, end, and BYTE|BIT
options are forwarded instead of silently discarded.
- Around line 1041-1079: Update the hscan command definition to require and
validate the cursor argument before dispatch, rather than relying on required
"key" alone, so missing or null cursors are rejected locally. Preserve the
existing key requirement and add coverage for both missing and null cursor
inputs.
In `@src/url/lib.rs`:
- Around line 187-200: Change URL::deinit to require unsafe cleanup semantics or
consume self, preventing callers from retaining a usable reference after C++
deletion. Keep destroy as the raw-pointer cleanup entry point with the same
live, uniquely owned, no-use-after-destruction contract, and update its call to
match the chosen deinit signature.
---
Outside diff comments:
In `@src/runtime/api/cron.rs`:
- Around line 2161-2212: Update spawn_cmd_prepare to construct and assign
base!().process before starting either stdout_reader or stderr_reader, ensuring
T::finish can detach the process if reader startup fails. For POSIX, extract the
required spawned stdio fields before the consuming to_process call, then store
the resulting Process before invoking reader startup; preserve the existing
Windows pipe extraction and startup flow while installing ownership beforehand.
In `@src/runtime/shell/shell_body.rs`:
- Around line 782-796: Update shell_cmd_args_from_js to accept the calling
entry-point name from shell_lex and shell_parse, then use that name in both
missing-argument errors. Replace the inconsistent shell_parse and shell wording
with one shared message that identifies the API, expected argument count, and
concrete remedy.
In `@src/runtime/socket/udp_socket.rs`:
- Around line 944-959: In the UDP membership operation, move require_socket in
the relevant method at src/runtime/socket/udp_socket.rs lines 944-959 until
after parse_addr and interface-family validation, immediately before
set_membership; apply the same ordering in the corresponding method at
src/runtime/socket/udp_socket.rs lines 1030-1045. Ensure no native socket
reference is retained across parse_addr’s JavaScript coercion, while preserving
existing validation and error handling.
In `@src/runtime/valkey_jsc/js_valkey_functions.rs`:
- Around line 255-261: Update the rejected-argument handling in the cmd_varargs!
expansion around from_js so throw_invalid_argument_type identifies the failing
zero-based argument index instead of using the generic "additional arguments"
label. Preserve the existing string-or-buffer constraint and ensure the message
uses the exact parameter type accepted by throw_invalid_argument_type, extending
the existing error path only if needed to format the indexed label.
- Around line 215-263: The cmd_varargs! mode choices may change null handling
and error behavior from the migrated hand-written commands. In
src/runtime/valkey_jsc/js_valkey_functions.rs:215-263, verify skip_null
preserves prior break semantics for set, srem, sadd, and hmget, while strict
preserves trailing null/undefined rejection for commands with optional
parameters. In src/runtime/valkey_jsc/js_valkey_functions.rs:1041-1641, audit
every registration’s mode, preserve required-argument validation behavior and
error compatibility, and ensure hscan does not use skip_null in a way that drops
its required cursor argument.
🪄 Autofix
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: aca3c9ec-d6f6-4d57-a64b-f684edd8eefe
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
src/CLAUDE.mdsrc/js_parser_jsc/Macro.rssrc/jsc/AsyncModule.rssrc/jsc/ConsoleObject.rssrc/jsc/URL.rssrc/jsc/VirtualMachine.rssrc/jsc/lib.rssrc/runtime/api/HashObject.rssrc/runtime/api/MarkdownObject.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/api/csrf_jsc.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/bake_body.rssrc/runtime/bake/dev_server/memory_cost.rssrc/runtime/bake/mod.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dns_jsc/dns.rssrc/runtime/dns_jsc/dns_sd.rssrc/runtime/dns_jsc/mod.rssrc/runtime/ipc.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/server/ServerWebSocket.rssrc/runtime/shell/builtin/basename.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/builtin/dirname.rssrc/runtime/shell/builtin/ls.rssrc/runtime/shell/builtin/mkdir.rssrc/runtime/shell/builtin/touch.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/subproc.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/expect/toEqual.rssrc/runtime/test_runner/expect/toHaveBeenCalledWith.rssrc/runtime/test_runner/expect/toHaveBeenLastCalledWith.rssrc/runtime/test_runner/expect/toHaveBeenNthCalledWith.rssrc/runtime/test_runner/expect/toHaveLastReturnedWith.rssrc/runtime/test_runner/expect/toHaveNthReturnedWith.rssrc/runtime/test_runner/expect/toHaveReturnedWith.rssrc/runtime/test_runner/expect/toStrictEqual.rssrc/runtime/test_runner/mod.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/valkey_jsc/js_valkey_functions.rssrc/runtime/webcore/fetch.rssrc/sourcemap_jsc/CodeCoverage.rssrc/url/Cargo.tomlsrc/url/lib.rs
💤 Files with no reviewable changes (2)
- src/runtime/test_runner/mod.rs
- src/runtime/test_runner/expect/toStrictEqual.rs
|
Review pass summary, so the thread history is skimmable: coderabbit raised six line findings. One was ours and is fixed (d474a28, a doc comment on the new The three "possibly related" PRs in the walkthrough (#36051, #36404, #36603) are all still open, so there is nothing here to have mis-merged; they are just future rebase surface for whichever side lands second, which mildly favors landing this sooner. Net: mergeable at d474a28, zero open threads, CI green apart from the R2 outage noted above. Still waiting on the one-PR-vs-splits call. |
2 participants
What this does
Dedup pass over the jsc bindings, runtime APIs,
bun:testmatchers and shell builtins. Only moves and removes code; no intended behavior change. Net -1.7k lines (53 files, +3294 / -5031).TagPayload(a full mirror ofTagthat existed to carry one variant's payload) is gone;TagResultcarries the custom-formatter payload as anOption. Shared with the macro evaluator,VirtualMachine::print_error_instanceand the test-runner pretty printer.WrappedWriter::newreplaces 32 struct literals and the eight per-functionpf!macros go away.bun_jsc::URLis now a re-export ofbun_url::whatwg::URL. The getters anddestroy()that only the jsc copy had move down intobun_url(whose copy is nowopaque_ffi!like the jsc one was), and the two entry points that need aJSValue(href_from_js,from_js) become theUrlJscextension trait.src/CLAUDE.mdupdated accordingly.load_entry_pointandload_entry_point_for_test_runner;auto_tick/auto_tick_activebecome oneauto_tick<const ACTIVE: bool>.toEqual/toStrictEqualshare one implementation (toStrictEqual.rsdeleted); thetoHave*Withfamily shares argument comparison,mock.resultsparsing and its failure epilogues. The shared epilogues are written against thethrow!macro from bun:test: stop expect() failure messages from consuming <...> spans in user data #34343, so user data still never goes through the color-tag rewrite.basename/dirnamesharePathBuiltin; the identicalOutputTaskVTableimpls of cp/ls/mkdir/touch come fromimpl_output_task_vtable!; the lex/parse testing APIs share their argument prologue;subprocWritable::init/Readable::initare one match with per-armcfginstead of two platform copies, and the Blob/ArrayBuffer stdin writer is shared withBun.spawn'sWritable.CronRegisterJob/CronRemoveJobshareCronJobCommonplus aCronJobBasetrait with hooks for the two places they actually differ (Windows non-zero-exit acceptance, the SID error message).PendingCacheKey<Req>and one pending-chain drain (drain_chain_ok/drain_chain_err) replace the per-record-kind copies;CacheHitbecomes an alias ofLookupCacheHit<GetAddrInfoRequest>and the twoget_or_put_into_pending_cachevariants merge.cmd!andcmd_varargs!(required/skip_null/strictselect the three varargs behaviors). All 133 commands keep their name, wire command, argument names, state requirement and meta.AsyncModuleerror builders,CryptoHasherdigest argument handling,PasswordObjectverify algorithm parsing, csrf option parsing,HashObject/ pretty_format / ConsoleObject usingJSType::is_array_buffer_like,MarkdownObjectinput pinning,JSON5Objectdelegating to the sharedexpr_to_js, napicreate_string_*prologue, bakeinit_transpiler_impland theDevServerfield exhaustiveness macro, udp setsockopt helpers, ipc message draining, socket event-callback tail,ServerWebSocketpublish prologue,CodeCoverageline mapping.Rebase onto main
This was stacked on #32022 / #32000; #32000 is closed as superseded, so the PR now targets
maindirectly and carries the one piece it needed from there (thewhatwg::URLadditions). Re-applied on top of ~790 commits of main; 30 of the files conflicted. Where main had changed one of the copies being merged, main's behavior is what the shared code does now. Notable:ffi/host_fns.rs, already removed on main by Remove ~39k lines of dead Rust across the workspace #35002), the pretty_formatMessageTypecleanup (already on main), the ipc decode-failure helper (main'sfinish_decodecovers it; only the four drain loops are shared now), and thebake/production.rshunk, which called a helper that only exists in Deduplicate package manager and CLI command helpers #32022.throw!macro,arguments()), cron (local-time /{ tz }rewrite in cron: interpret Bun.cron.parse() and in-process schedules in local time; add { tz } option #35122 and the&self-only restructuring in Make re-entrant runtime objects &self-only; delete AnyTask #36571; the consolidation now follows main's "confine&mutto one prepare call, then free" shape), dns (dns(macOS): replace getaddrinfo_async_start with DNSServiceGetAddrInfo over a shared connection #36619 dns_sd backend, Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075Outcome/keep_alivein the shared drain), ConsoleObject / URL / matchers / dns visibility per Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184,VirtualMachinekeeps the test-runner-onlywakeup()from bun test: one pending ref'd timer adds ~100ms to every subsequent test file that loads a module (1.4.0-canary.1, not in 1.3.14) #36450 outside the shared loop, subproc keeps theSocketFdarms,Writablehelper adapted toNewStaticPipeWriter<P>.da3851e(14 more main commits): onlycron.rsconflicted, against cron: remove the Done and Failed states of RegisterState and RemoveState #37411, which removed theDone/Failedvariants from both state enums and thestate =assignment in bothfinishimpls. Applied the same removal once to the unifiedCronJobStateandCronJobCommon::finish;rust:check-all(cron has macOS/Windows-only variants) and the cron suite (134 pass) confirm it.CacheHitbecoming an alias forcesLookupCacheHit,HasPendingCacheKey,HasTail,PendingCacheKey,NameKeyedandDNSLookupto bepub(they are reachable through the existingpub useofCacheHit/PendingCache);CAresRecordTypegoes topub(crate)since the module that made it reachable is gone.Verification
cargo checkon all 10 CI targets,cargo clippy --workspace,cargo fmt --check, andtest/internal/source-lints(75 lints) pass.console.table),expect.test.js,spyMatchers,toHaveReturnedWith,mock-fn,expect-failure-message-angle-brackets, shell commands +bunshell+ lex/parse, cron, CryptoHasher, csrf, hash, password, JSON5, coverage, macros, markdown, SocketAddress, sourcemap, napi, spawn + ipc, websocket server, udp + dgram, dns, bake dev/prod, auto-install, valkey unit tests against a local redis. The only failures were environment or debug-build timing (root can't hit EACCES, no outbound DNS, the 300k-message websocket benchmark at the 30s limit) and fail identically on the release build.publish*argument error, mkdir/touch/ls/basename/dirname output and errors,Bun.resolveSyncoffile:URLs,fetch()offile:URLs, 14 redis URL shapes, dns pass/fail set, and a script verifying all 133 valkey commands map to the equivalent macro variant.#32024 is still stacked on this branch and needs the same treatment separately.