rust: adopt bon for compile-checked struct and function builders by robobun · Pull Request #32845 · oven-sh/bun

robobun · 2026-06-27T11:36:08Z

Exploratory PR evaluating bon (compile-time typestate builders for structs and functions) as a workspace convention. It converts enough real code, across six crates, to judge the pattern on its own merits, and writes down what the conversion turned up. 16 functions (~77 call sites), 3 structs, all behavior-preserving.

The two bug classes this targets

1. Positional arguments of the same type

A positional call with several same-typed arguments type-checks after any transposition. A survey of the workspace (excluding FFI and #[repr(C)], see below) turned up, among others:

function	hazard
`CanonicalRequest::format` (`s3_signing`)	15 positional args building an AWS SigV4 canonical request: 7 are `&[u8]`, 6 are `Option<&[u8]>`. Transposing `hash` and `date` (adjacent, same type) signs garbage.
`S3Credentials::new_value`	6 consecutive `Box<[u8]>`. Transposing `access_key_id` and `secret_access_key` compiles.
`GlobWalker::init_with_cwd`	5 consecutive `bool`. One caller passed `false, false, false, false, false`; another passed `true, true, false, true, true`.
`HTTPContext::release_socket`	two transposable pairs in one signature: `hostname`/`target_hostname` and `port`/`target_port`.
`ShellCpTask::create`	`src: Vec<u8>, tgt: Vec<u8>, cwd_path: Vec<u8>`. Transposing `src`/`tgt` in a `cp` is data loss.
`AsyncHTTP::init`	`headers_buf`/`request_body` adjacent, both `&[u8]`. One caller passed `b"", response_buffer, b""`.
`Package::parse_dependency`	17 positional parameters, ending in `key_loc: Loc, value_loc: Loc`.

The clearest evidence this was already costing something is pm_view_command.rs, where the author had to write inline comments naming every positional argument:

PackageManifest::parse(
    &scope, &mut log, response_buf.list.as_slice(), name,
    b"",  // last_modified (not needed for view)
    b"",  // etag (not needed for view)
    0,    // public_max_age (not needed for view)
    true, // is_extended_manifest (view uses application/json Accept header)
)

Those comments are a builder, written by hand as dead text the compiler cannot check. After:

PackageManifest::parse()
    .scope(&scope)
    .log(&mut log)
    .json_buffer(response_buf.list.as_slice())
    .expected_name(name)
    .last_modified(b"")
    .etag(b"")
    .public_max_age(0)
    .is_extended_manifest(true)
    .call()

2. `impl Default` as a construction crutch

A plain struct literal already has compile-checked required fields. The bug is specifically ..Default::default() paired with a hand-written Default full of placeholder values, and every instance of it in this PR says so in its own words.

FetchOptions (FetchTasklet.rs):

// Zero-values for the required fields
// (method/headers/body/url/bools/unix_socket_path/globalThis) so
// callers can use `..Default::default()` struct-update syntax while
// still overriding the required fields explicitly.

S3HttpSimpleTask (simple_request.rs): the Default impl defines a local function as the callback placeholder whose body is

unreachable!("S3HttpSimpleTask.callback used before being set")

a runtime panic standing in for a compile error. Its http field defaults to MaybeUninit::uninit() while the Drop impl calls assume_init, so dropping a default() is UB.

S3HttpDownloadStreamingTask (download_stream.rs): callback_context: NonNull::dangling() and a silent no-op callback: |_, _, _, _| {}.

All three turned out to already be dead: no construction site used ..Default::default() on them any more. This PR deletes the three impl Defaults and derives a builder in which the non-Option fields are required at compile time.

The one I did not convert, and why it is the real argument

bun_spawn::WindowsOptions:

// Every `bun.spawnSync` call site sets `loop_` explicitly. A
// zeroed handle here keeps `..Default::default()` usable
// for the other fields. `spawn_process_windows` (the sole consumer)
// asserts non-null at the read site so a forgotten `loop_` panics
// with a pointed message instead of segfaulting at the `.uv_loop`
// field offset.
loop_: unsafe { bun_core::ffi::zeroed_unchecked() },

An unsafe block and a runtime non-null assertion exist only so that ..Default::default() keeps working. It has 28 construction sites. I did not convert it, because the trap turns out to be viral: those 28 sites build WindowsOptions inside a sync::Options { .., windows: WindowsOptions { loop_, ..Default::default() }, ..Default::default() } whose own impl Default requires WindowsOptions: Default. One placeholder Default forces the enclosing struct to grow one too, and you cannot remove the inner one without first removing the outer one.

That cascade is, I think, the real answer to why the workspace has 2060 ..Default::default() call sites and 535 hand-written impl Default blocks. It is the strongest case for making builders a convention, and it is also why WindowsOptions needs its own PR: it requires converting sync::Options first.

What was converted

Functions (16, ~77 call sites). All are pure positional-to-named conversions; no parameter gained or lost a default that it did not already have.

function	crate	call sites
`GlobWalker::init` (absorbs `init_with_cwd`)	`bun_glob`	6
`AsyncHTTP::init`	`bun_http`	7
`AsyncHTTP::init_sync`	`bun_http`	14
`HTTPContext::release_socket`	`bun_http`	4
`HTTPContext::existing_socket`	`bun_http`	1
`S3Credentials::new_value`	`bun_s3_signing`	1
`CanonicalRequest::format`	`bun_s3_signing`	1
`LinkerGraph::generate_symbol_import_and_use`	`bun_bundler`	13
`Package::parse_dependency`	`bun_install`	3
`PackageManifest::parse`	`bun_install`	2
`ServerWebSocket::do_publish`	`bun_runtime`	6
`s3::upload`	`bun_runtime`	2
`s3::upload_stream`	`bun_runtime`	5
`s3::writable_stream`	`bun_runtime`	2
`RunCommand::run_package_script_foreground` (absorbs `_with_shell_path`)	`bun_runtime`	9
`ShellCpTask::create`	`bun_runtime`	1

Two _with_x sibling variants became a single builder with an optional member and were deleted: GlobWalker::init_with_cwd folded into init (cwd is now optional, defaulting to the process top-level dir) and RunCommand::run_package_script_foreground_with_shell_path folded into run_package_script_foreground (shell_path is now optional). In api/glob.rs, folding init_with_cwd into init also collapsed two copy-pasted "has cwd" / "no cwd" branches into one call via the generated .maybe_cwd() setter. A bon builder replaces the _with_x constructor-name explosion, which is its own defect class.

Structs (3): FetchOptions, S3HttpSimpleTask, S3HttpDownloadStreamingTask. Each derives bon::Builder and has its hand-written impl Default deleted.

Several #[allow(clippy::too_many_arguments)] attributes on the converted functions are deleted with them.

Deliberately skipped

JSGlobalObject::gregorian_date_time_to_ms_utc: 7 consecutive i32, the longest same-type run found, but (year, month, day, hour, minute, second, ms) is the one signature where positional order is universal. A builder there is worse.
Repository::checkout: it is a trait method (impl RepositoryExt for Repository); bon only applies to inherent impls.
NodeFS::_copy_single_file_sync: has a #[cfg(windows)]-conditional parameter type.
write_package_info_object / write_workspace_deps: six byte-identical 12-argument call sites. Six byte-identical builder chains are no better; the right fix is hoisting the invariant context into a struct.
output_file::Options: 11 of its 14 construction sites are already full struct literals with no ..Default::default(), so they already get compile-checked required fields. The fix there is 3 call sites, not a 270-line conversion.
WindowsOptions and sync::Options: the viral-Default cascade above; needs its own PR.
Everything #[repr(C)], everything in the *_jsc and *_sys crates, every extern "C" and // HOST_EXPORT(..) function, and the fn-pointer hook tables: their layouts and signatures are fixed by the C++ side. That boundary, not caution, is the real ceiling on how much of the workspace this can reach.

Cost

3 new crates in Cargo.lock: bon, bon-macros, and strsim. bon-macros' heavy dependencies (syn, quote, proc-macro2, darling) were already in the tree via strum and enumset.
Zero runtime overhead. The required/optional state lives in the type parameters of the generated builder, so an optimized build inlines every setter and the finishing .call()/.build() into the same code as the positional call or struct literal. Upstream documents this with generated assembly at https://bon-rs.com/guide/benchmarks/runtime, and this workspace's lto = "fat" + codegen-units = 1 release profile is stronger than what those benchmarks assume.
No measurable change to cargo check --workspace wall time.

Verification

cargo check --workspace: clean
cargo clippy --workspace: clean. The workspace denies ~50 clippy lints; bon's generated code passes all of them, including needless_pass_by_value, large_types_passed_by_value, redundant_clone, and unreachable_pub.
cargo check --workspace --target x86_64-pc-windows-msvc: clean
cargo fmt --check: clean
bun bd (full debug + ASan build): succeeds. Bun.Glob(..).scanSync, Bun.serve + fetch, and the first test/js/bun/glob/scan.test.ts cases were exercised against the built binary.

Rebase onto main (2026-06-28)

Rebased onto a1c39ded9b (25 upstream commits) and squashed this branch's 13 commits, most of which were review fixups, into one. Two conflicts, both append/append:

src/runtime/server/ServerWebSocket.rs: Bun.serve websocket: make publish() return 0/-1 on subscriber backpressure #32889 rewrote do_publish's body (it now maps an aggregated SendStatus instead of a bool) and its doc comment. The signature is unchanged, so the resolution keeps Bun.serve websocket: make publish() return 0/-1 on subscriber backpressure #32889's body and doc and re-adds the #[builder] attribute; all six callers are unaffected.
test/js/bun/glob/scan.test.ts: glob: match explicitly-named dotfiles and resolve literal paths through symlinks #32853 appended a describe block at the same end-of-file position as the glob.scan option flags block this PR adds. Both are kept.

Because #32853 also changed glob dotfile and symlink semantics, the verification was rerun in full against the rebased tree rather than assumed: cargo check --workspace, cargo clippy --workspace, cargo fmt --check, and a complete bun bd all pass, and the merged test/js/bun/glob/scan.test.ts (#32853's new behavior tests plus this PR's flag tests, 197 tests) passes against the rebuilt binary with 0 failures. #32853's own new tests (wildcard pattern "**/*.txt" still hides dotfiles by default, wildcard segment still respects followSymlinks:false) assert the same semantics the flag tests here rely on.

Rebase onto main (2026-07-01)

Rebased again onto eba370b692 (75 upstream commits, including the JSON parser rewrite in #33032 and the WebKit bump in #33133). One conflicting file: src/install/lockfile/Package.rs, and this one was non-trivial.

#33032 changed parse_dependency itself: it deleted the value_loc: bun_ast::Loc parameter (the value location is now derived inside the body via a new value_loc_of() helper) and restructured the enclosing loop to iterate JsonObjectStringRows, so key_loc became a loop binding rather than an Expr field. The resolution takes main's new signature and control flow verbatim and removes the now-nonexistent .value_loc(..) setter from all three call sites; the doc comment this PR had added, whose rationale was "key_loc/value_loc are an adjacent same-typed pair", became false along with the parameter and was rewritten.

Two build prerequisites from main are worth knowing about if you build this branch: the new bun_parsers crate's build.rs needs bun bd --configure-only to have generated json_byte_class.rs, and generated_classes.rs must be regenerated for #33144's new BuildArtifact cached field. Neither is specific to this PR.

Verification was rerun in full on the rebased tree: cargo check --workspace, cargo clippy --workspace, cargo fmt --check, prettier, and a complete bun bd all pass; the merged test/js/bun/glob/scan.test.ts (197 tests) passes against the rebuilt binary. Because the only real conflict was inside the package.json dependency parser, it was also runtime-verified directly: a local workspace install (a workspaces: ["packages/*"] glob plus dependencies/peerDependencies/peerDependenciesMeta on a member) exercises this PR's converted GlobWalker::init and all three hand-merged parse_dependency call sites, including the peerDependenciesMeta-only synthesis path, and produces a correct lockfile on the rebuilt binary.

Rebase onto main (2026-07-09)

Rebased onto b05b4fab0e (104 upstream commits). Two conflicting files, both in the fetch() path, and both one change: main added a fetch(url, {timeout: ms}) option, threading a new idle_timeout_seconds: Option<c_uint> field through FetchOptions and into async_http::Options (alongside a new proxy_settings field there).

src/runtime/webcore/fetch/FetchTasklet.rs: the struct definition picked up idle_timeout_seconds in the auto-merge (it is Option<T>, so bon auto-generates .maybe_idle_timeout_seconds() with no attribute needed); the impl Default for FetchOptions this PR deletes conflicted because main added the new field there, so it stays deleted; and the AsyncHTTP::init() call picks up main's two new fields in the inner async_http::Options literal.
src/runtime/webcore/fetch.rs: the FetchOptions::builder() chain gains a .maybe_idle_timeout_seconds(idle_timeout_seconds) setter right where main put the field in the struct literal. Main's side also re-mentioned the global_this field this PR proves dead and deletes; that stays deleted.

Verification rerun in full on the rebased tree: cargo check --workspace, cargo clippy --workspace, cargo fmt --check, prettier, and a complete bun bd all pass. Because the only conflicts were in the fetch() path, runtime-verified directly: a fetch(url, {timeout: 5000}) roundtrip against a local Bun.serve exercises the exact merged code and succeeds. The workspace bun install smoke and the per-flag scan.test.ts tests also pass against the rebuilt binary.

no test proof · iteration 12 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/glob/scan.test.ts

robobun · 2026-06-27T11:36:17Z

^{Updated 3:07 AM PT - Jul 9th, 2026}

❌ @robobun, your commit 3a55eb1 has 3 failures in Build #70891 (All Failures):

test/cli/run/no-orphans.test.ts - code 1 on 🍎 14 x64
test/cli/run/no-orphans.test.ts - code 1 on 🍎 26 aarch64
test/napi/napi.test.ts - code 1 on 🪟 2019 x64-baseline
test/cli/install/bun-install-registry.test.ts - code 1 on 🪟 11 aarch64

🧪 To try this PR locally:

bunx bun-pr 32845

That installs a local version of the PR into your bun-32845 executable, so you can run:

bun-32845 --bun

coderabbitai · 2026-06-27T11:46:54Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

This PR adds bon as a workspace dependency and converts many constructors and call sites to builder-style APIs across glob handling, HTTP, shell execution, bundler wiring, install flows, S3 helpers, signing code, and glob tests.

Changes

bon builder migration

Layer / File(s)	Summary
Workspace and builder entrypoints `Cargo.toml`, `src/*/Cargo.toml`, `src/glob/GlobWalker.rs`, `src/http/AsyncHTTP.rs`, `src/http/HTTPContext.rs`	`bon` is added to workspace manifests, and `GlobWalker`, `AsyncHTTP`, and `HTTPContext` gain builder-annotated constructors and socket helpers.
Bundler symbol wiring `src/bundler/LinkerGraph.rs`, `src/bundler/LinkerContext.rs`, `src/bundler/linker_context/*`	`generate_symbol_import_and_use` call sites in bundler code move to chained setters, and `LinkerGraph` isolates the builder-generated helper in its own impl block.
Glob walker consumers and tests `src/install/lockfile/Package/WorkspaceMap.rs`, `src/runtime/api/glob.rs`, `src/runtime/cli/filter_arg.rs`, `src/runtime/shell/states/Expansion.rs`, `test/js/bun/glob/scan.test.ts`	Glob walker construction in install and runtime paths switches to `GlobWalker::init` builder chains, and glob option tests exercise the new flag wiring.
HTTP socket lifecycle `src/http/HTTPContext.rs`, `src/http/h2_client/ClientSession.rs`, `src/http/lib.rs`	`HTTPContext::connect`, `ClientSession::maybe_release`, and keep-alive release paths now use builder-style `release_socket` and `existing_socket` calls with the same connection metadata.
Request builders and runtime call sites `src/http/AsyncHTTP.rs`, `src/install/NetworkTask.rs`, `src/install/npm.rs`, `src/runtime/cli/*`, `src/standalone_graph/StandaloneModuleGraph.rs`, `src/runtime/server/ServerWebSocket.rs`, `src/runtime/shell/builtin/cp.rs`	HTTP request creation in install, runtime CLI, standalone graph, and preconnect/prefetch paths moves from positional constructors to fluent builder chains, and package-script and websocket call sites switch to the same pattern.
Package, signing, and S3 builders `src/install/npm.rs`, `src/install/lockfile/Package.rs`, `src/s3_signing/credentials.rs`, `src/runtime/webcore/fetch.rs`, `src/runtime/webcore/fetch/FetchTasklet.rs`, `src/runtime/webcore/s3/client.rs`, `src/runtime/webcore/s3/download_stream.rs`, `src/runtime/webcore/s3/simple_request.rs`, `src/runtime/webcore/Blob.rs`	`PackageManifest`, lockfile dependency parsing, `S3Credentials`, `CanonicalRequest`, fetch options, S3 client task allocation, Blob upload paths, fetch task setup, and S3 download state move to builder-backed construction.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the PR’s main change: adopting bon for compile-checked Rust builders.
Description check	✅ Passed	It includes the PR purpose and verification details, though it uses custom headings instead of the exact template.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 6

🤖 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 `@Cargo.toml`:
- Around line 350-356: Shorten the inline manifest comment to 3 lines max in
Cargo.toml. Keep the explanation brief and focused on the typestate builder
rationale, removing the longer justification about Default placeholders and
release-build inlining. Locate the existing multi-line comment near the
compile-time-checked builders section and condense it without changing meaning.

In `@src/glob/GlobWalker.rs`:
- Around line 1319-1322: Trim the constructor comment in GlobWalker so it keeps
only the durable invariant about named setters preventing boolean transposition,
and remove the migration-history/out-param note from the nearby documentation on
the GlobWalker constructor/init path. Keep the comment concise and limited to
the non-obvious behavior around GlobWalker::init and only_files, with no
PR-history context.

In `@src/http/AsyncHTTP.rs`:
- Around line 446-448: The builder comment in AsyncHTTP::request-style setup
still references migration history (“used to be” positional parameters); rewrite
it to state only the lasting invariant that named setters prevent accidental
transposition of adjacent &[u8] values like headers_buf and request_body. Keep
the comment durable and non-historical while preserving the rationale for the
API design.

In `@src/install/lockfile/Package.rs`:
- Around line 1677-1681: The inline comment near the lockfile body is too long
and should be compressed to stay within the 3-line guideline. Shorten the
rationale around the live StringBuilder and split-borrow invariant in
Package::... so it keeps only the essential point about reading string_bytes
through the builder while accepting workspace_paths and workspace_versions
directly.

In `@src/runtime/webcore/Blob.rs`:
- Around line 4979-4998: The upload stream branch is still using the store-level
S3 options instead of the parsed per-call upload options, so the request can
ignore caller-provided settings. In the upload path inside Blob.rs, update the
s3_client::upload_stream chain to use the options derived from aws_options (the
same parsed options used by get_credentials_with_options and sibling
upload-stream branches) rather than s3.options, keeping the rest of the
ACL/storage/metadata wiring unchanged.

In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 2487-2492: Shorten the doc comment attached to the FetchTasklet
builder rationale so it fits within 3 lines or fewer. Keep the explanation
focused on the builder behavior and the removal of the old Default
implementation, and trim the extra historical/detail sentences in the nearby
comment block.

🪄 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: 2ee27aa0-5438-474e-8b3a-4e98caaa29f9

📥 Commits

Reviewing files that changed from the base of the PR and between df92f8f and 70f81ad.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (41)

Cargo.toml
src/bundler/Cargo.toml
src/bundler/LinkerContext.rs
src/bundler/LinkerGraph.rs
src/bundler/linker_context/generateCodeForLazyExport.rs
src/bundler/linker_context/scanImportsAndExports.rs
src/glob/Cargo.toml
src/glob/GlobWalker.rs
src/http/AsyncHTTP.rs
src/http/Cargo.toml
src/http/HTTPContext.rs
src/http/h2_client/ClientSession.rs
src/http/lib.rs
src/install/Cargo.toml
src/install/NetworkTask.rs
src/install/lockfile/Package.rs
src/install/lockfile/Package/WorkspaceMap.rs
src/install/npm.rs
src/runtime/Cargo.toml
src/runtime/api/glob.rs
src/runtime/cli/audit_command.rs
src/runtime/cli/create_command.rs
src/runtime/cli/filter_arg.rs
src/runtime/cli/pack_command.rs
src/runtime/cli/pm_version_command.rs
src/runtime/cli/pm_view_command.rs
src/runtime/cli/publish_command.rs
src/runtime/cli/run_command.rs
src/runtime/cli/upgrade_command.rs
src/runtime/server/ServerWebSocket.rs
src/runtime/shell/builtin/cp.rs
src/runtime/shell/states/Expansion.rs
src/runtime/webcore/Blob.rs
src/runtime/webcore/fetch.rs
src/runtime/webcore/fetch/FetchTasklet.rs
src/runtime/webcore/s3/client.rs
src/runtime/webcore/s3/download_stream.rs
src/runtime/webcore/s3/simple_request.rs
src/s3_signing/Cargo.toml
src/s3_signing/credentials.rs
src/standalone_graph/StandaloneModuleGraph.rs

claude

Additional findings (outside current diff — PR may have been updated during review):

🟡 src/runtime/webcore/s3/simple_request.rs:110-117 — These struct doc comments (also on S3HttpDownloadStreamingTask and FetchOptions) narrate what this PR changed — "This replaces the hand-written impl Default this struct used to carry... Neither construction site used it any more." Per CLAUDE.md, change history belongs in the PR description (where it already appears verbatim); the first sentence ("Every non-Option field must be supplied at the construction site; forgetting one is a compile error") is the durable invariant worth keeping, the "This replaces..." tail should be dropped.
Extended reasoning...

What this is

The PR adds 6-7-line doc comments above #[derive(bon::Builder)] on three structs:
- S3HttpSimpleTask (src/runtime/webcore/s3/simple_request.rs:110-116)
- S3HttpDownloadStreamingTask (src/runtime/webcore/s3/download_stream.rs:18-24)
- FetchOptions (src/runtime/webcore/fetch/FetchTasklet.rs:2487-2492)
Each follows the same template, e.g. on S3HttpSimpleTask:

Every non-Option field must be supplied at the construction site; forgetting one is a compile error. This replaces the hand-written impl Default this struct used to carry, which handed out a MaybeUninit::uninit() http (UB to Drop) and a callback whose body was unreachable!("used before being set") purely so ..Default::default() struct-update syntax compiled. Neither construction site used it any more.

The bolded portion — five of the seven lines — describes what the deleted code looked like and why it was deleted. That is change history.

Why it violates the project convention

Root CLAUDE.md (line 239, "Code style & idioms") states:

Comments carry only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications... No narrating what the code does, no bug history — that belongs in the PR description.

The "This replaces the hand-written impl Default..." sentences describe a previous code state that no longer exists in the file. A reader six months from now does not need to know that there used to be an unreachable!("used before being set") placeholder callback — they need to know how to construct the struct today. The PR description already carries this rationale in detail (see "§2. impl Default as a construction crutch"), which is exactly where CLAUDE.md says it should live.

Step-by-step example

Take S3HttpDownloadStreamingTask. The comment says:
1. "Every non-Option field must be supplied at the construction site; forgetting one is a compile error." — durable: tells the reader what #[derive(bon::Builder)] enforces.
2. "This replaces the hand-written impl Default this struct used to carry," — history: there is no impl Default in the file any more; "replaces" only makes sense relative to the diff.
3. "which handed out a MaybeUninit::uninit() http, a dangling NonNull for callback_context, and a silent no-op callback" — history: describes the body of a deleted impl.
4. "purely so ..Default::default() struct-update syntax compiled." — history: describes the motivation for deleted code.
5. "The one construction site did not use it any more." — history: describes the state of call sites at the time of the PR.
Sentences 2-5 will be stale the moment this merges (the deleted Default is no longer visible to anyone reading the file), and will become actively misleading if a second construction site is later added.

Addressing the counterargument

One verifier argued this is a "deliberate deviation" warning — that it tells future maintainers not to add Default back because doing so was UB. That is a fair reading of the intent, but it is not what the comment says. The comment is phrased as "this replaces X, which did Y" — past-tense narration of the diff — not as "do not add impl Default: http is MaybeUninit and a default would be UB to Drop." If the goal is to warn against re-adding Default, a one-line forward-looking statement to that effect would serve it without the change-history framing. The same verifier conceded the "Neither construction site used it any more" sentence is purely historical and that the "This replaces..." framing "could be tightened" — which is the substance of this nit.

The "3 lines max" rule (CLAUDE.md line 268) is weaker grounds on its own — the adjacent http field comment in the same struct is 8 lines — but the no-change-history rule applies cleanly here.

Suggested fix

Trim each comment to its durable first sentence, optionally adding a forward-looking warning if desired:
```
/// Every non-`Option` field must be supplied at the construction site;
/// forgetting one is a compile error. No `Default` impl: `http` is
/// `MaybeUninit` and a defaulted instance would be UB to `Drop`.
#[derive(bon::Builder)]
pub struct S3HttpSimpleTask { ... }
```
This keeps the invariant and the do-not-regress warning while dropping the "used to carry" / "any more" narration that only makes sense relative to this PR's diff.

robobun · 2026-06-27T12:09:58Z

Both claude findings are handled:

The "This replaces the hand-written impl Default..." doc comments on FetchOptions, S3HttpSimpleTask, and S3HttpDownloadStreamingTask were already rewritten to the timeless, three-line-max form in 89e900d (pushed while that review was running). The change history lives in the PR description only.
The #[bon::bon] impl-block scoping is now uniform as of 12cc8f9 (see the resolved thread on HTTPContext.rs).

coderabbitai

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/bun/glob/scan.test.ts`:
- Around line 940-943: Shorten the explanatory comment in scan.test.ts to 3
lines max while preserving the named-setter invariant for GlobWalker::init. Keep
the core message that each boolean scan option maps to exactly one Rust setter
and that the one-at-a-time fixture tests catch incorrect wiring, but condense
the wording in place.

🪄 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: 011390fb-4906-47f8-a349-773455882555

📥 Commits

Reviewing files that changed from the base of the PR and between 70f81ad and eba62ca.

📒 Files selected for processing (17)

Cargo.toml
src/bundler/LinkerGraph.rs
src/glob/GlobWalker.rs
src/http/AsyncHTTP.rs
src/http/HTTPContext.rs
src/install/lockfile/Package.rs
src/install/npm.rs
src/runtime/api/glob.rs
src/runtime/cli/run_command.rs
src/runtime/server/ServerWebSocket.rs
src/runtime/shell/builtin/cp.rs
src/runtime/webcore/fetch/FetchTasklet.rs
src/runtime/webcore/s3/client.rs
src/runtime/webcore/s3/download_stream.rs
src/runtime/webcore/s3/simple_request.rs
src/s3_signing/credentials.rs
test/js/bun/glob/scan.test.ts

💤 Files with no reviewable changes (1)

src/runtime/api/glob.rs

robobun · 2026-06-27T13:35:21Z

CI status for head 3a55eb1d91 (third rebase), build 70891, now near-finished: 280 of 286 jobs passed, 2 still scheduled (macOS agents). The diff is green; all 16 build-rust targets pass.

Three test files have error annotations. All three were modified by #33622 ("make some tests faster", landed in the 104 main commits this rebase pulled in), and none is in code this PR touches:

test/cli/install/bun-install-registry.test.ts (Windows 11 aarch64, 267/274 tests in the file pass): hoisting > peers > it should hoist 1.0.1 when peer * got 1.0.9. This is the peer-dependency hoisting resolver; this PR's diff has zero files in resolver, hoist, or Tree.rs (the converted parse_dependency is the package.json parser). A sibling case in the same loop is already isFlaky-marked, and a comment in the same file reads verbatim: "this repeatedly re-resolves from a deleted lockfile to catch nondeterministic peer-dep hoisting."
test/napi/napi.test.ts (Windows 2019 x64-baseline): napi_wrap > has the right lifetime, a gcUntil "Condition was not met after 100 GC attempts" GC-timing assertion. This PR does not touch N-API.
test/cli/run/no-orphans.test.ts (2 darwin shards): the "(perl) fast-exit intermediate — daemon still reaped" 30s timeout. Proven on another PR right now: build 70888 (url-format-whatwg-options, a WHATWG URL formatting PR) has the identical annotation. make some tests faster #33622's own commit message lists no-orphans among the files it added concurrency to. The --no-orphans daemon-reaper code path is separate from the run_package_script_foreground function this PR converted.

As before: main's own builds run zero test shards, so #33622's timing and concurrency changes are being test-run for the first time in PR builds. test/js/bun/glob/scan.test.ts (the test this PR adds) passes on every lane. All review threads are resolved; CodeRabbit has auto-paused itself. Ready for a maintainer.

coderabbitai

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 `@src/runtime/cli/pm_view_command.rs`:
- Around line 161-164: The comment near pm_view_command::parse should be trimmed
to 3 lines max and remove transient wording like “now-public”; keep only
durable, non-obvious context about why the response is parsed into a
PackageManifest and why last_modified/etag/public_max_age are omitted for pm
view. Rewrite the block in place so it stays concise, references the parse flow
in pm_view_command::parse, and avoids implementation-status language.

🪄 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: 01d435ee-f30d-45a2-977f-9814491a0edd

📥 Commits

Reviewing files that changed from the base of the PR and between 70f87aa and 3ebd017.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (42)

Cargo.toml
src/bundler/Cargo.toml
src/bundler/LinkerContext.rs
src/bundler/LinkerGraph.rs
src/bundler/linker_context/generateCodeForLazyExport.rs
src/bundler/linker_context/scanImportsAndExports.rs
src/glob/Cargo.toml
src/glob/GlobWalker.rs
src/http/AsyncHTTP.rs
src/http/Cargo.toml
src/http/HTTPContext.rs
src/http/h2_client/ClientSession.rs
src/http/lib.rs
src/install/Cargo.toml
src/install/NetworkTask.rs
src/install/lockfile/Package.rs
src/install/lockfile/Package/WorkspaceMap.rs
src/install/npm.rs
src/runtime/Cargo.toml
src/runtime/api/glob.rs
src/runtime/cli/audit_command.rs
src/runtime/cli/create_command.rs
src/runtime/cli/filter_arg.rs
src/runtime/cli/pack_command.rs
src/runtime/cli/pm_version_command.rs
src/runtime/cli/pm_view_command.rs
src/runtime/cli/publish_command.rs
src/runtime/cli/run_command.rs
src/runtime/cli/upgrade_command.rs
src/runtime/server/ServerWebSocket.rs
src/runtime/shell/builtin/cp.rs
src/runtime/shell/states/Expansion.rs
src/runtime/webcore/Blob.rs
src/runtime/webcore/fetch.rs
src/runtime/webcore/fetch/FetchTasklet.rs
src/runtime/webcore/s3/client.rs
src/runtime/webcore/s3/download_stream.rs
src/runtime/webcore/s3/simple_request.rs
src/s3_signing/Cargo.toml
src/s3_signing/credentials.rs
src/standalone_graph/StandaloneModuleGraph.rs
test/js/bun/glob/scan.test.ts

claude

I didn't find any issues in this round, but this PR adds a new workspace dependency (bon) and proposes it as a workspace-wide convention across 43 files in six crates — that design decision (and the open CI build failures) warrant a human sign-off.

Extended reasoning...

Overview

This PR introduces the bon crate (compile-time typestate builders) as a workspace dependency and converts 16 functions (~77 call sites) and 3 structs across six crates — bun_glob, bun_http, bun_s3_signing, bun_bundler, bun_install, bun_runtime — from positional arguments / hand-written Default impls to builder chains. It also deletes three impl Default blocks, folds two _with_x sibling functions into their bases, removes a dead FetchOptions.global_this field, and adds a describe block of glob option-flag tests. The PR is explicitly framed as exploratory, evaluating whether bon should become a workspace convention.

Security risks

The diff touches AWS SigV4 canonical-request construction (CanonicalRequest::format in s3_signing/credentials.rs) and S3Credentials::new_value. The conversions are mechanical positional→named with no logic changes, and the parameter values at each call site are preserved verbatim, so I see no new exposure — but signing code is exactly where a transposed argument would be silent and security-relevant, so it merits a careful human pass.

Level of scrutiny

High. Three independent reasons: (1) this is explicitly a convention-setting PR — adopting bon workspace-wide is an architectural choice a maintainer should ratify, not something a bot should approve; (2) it adds three crates to Cargo.lock (bon, bon-macros, strsim); (3) it touches hot/critical paths (HTTP keep-alive socket pooling, fetch tasklet construction, bundler symbol import wiring, S3 signing) where a mis-mapped builder argument would be subtle.

Other factors

All prior review threads (mine and CodeRabbit's) are resolved, and the author has been thorough about verification (cargo check/clippy/fmt, full bun bd, glob test suite re-run post-rebase). However, the most recent robobun CI status (against dae0a97) shows build-rust failing on every platform, which is unresolved. Between the open CI failures, the new dependency, and the convention decision, this needs a human reviewer.

robobun · 2026-06-28T09:53:45Z

Agreed on the human sign-off: adopting a new workspace dependency as a convention is exactly the call this PR is asking a maintainer to make, and the PR description frames it as exploratory for that reason.

One correction, because it matters for whoever reviews: build-rust is not failing. The build you looked at (66329, for dae0a97) was canceled when it was superseded by the next push, and Buildkite reports a canceled build's in-flight jobs as canceled, which reads as red. Queried directly:

Build 66329 (dae0a97): build state canceled, all 16 build-rust jobs in state canceled, not failed.
Build 66332 (the current head, 2a165b34): all 16 build-rust jobs passed, across darwin (x64/aarch64), linux (gnu/musl/android/baseline/asan, x64 and aarch64), freebsd (x64/aarch64), and windows (x64/x64-baseline/aarch64).

The only genuinely failed job on the current build is :darwin: 26 aarch64 - test-bun, which for the third build in a row is the same agent (darwin-aarch64-26.5.1-1) dying with buildkite-agent artifact download timed out after 120s before any test runs. That lane needs a manual job retry, not a code change.

On the signing code: I share that concern, which is why CanonicalRequest::format and S3Credentials::new_value are named in the PR description's hazard table. The conversions there are strictly positional-to-named with the same values, and the 16 build-rust targets plus the x64-asan test shards are all green, but a human eye on those two diffs specifically is the right ask.

Convert 16 functions (~77 call sites) whose positional parameter lists have runs of same-typed arguments (transposable without a compile error) to named-setter builders, and 3 structs whose hand-written Default impls existed only so ..Default::default() would compile while required fields got inert placeholder values. bon's typestate builder makes forgetting a required field a compile error at zero runtime cost. Folds GlobWalker::init_with_cwd into init (cwd becomes an optional setter) and RunCommand::run_package_script_foreground_with_shell_path into run_package_script_foreground (shell_path becomes optional), deleting the _with_x variant pair. Deletes the three lying Default impls (FetchOptions, S3HttpSimpleTask, S3HttpDownloadStreamingTask) and the vestigial Option wrapper on the S3 task vm fields, plus the three .expect("vm set at task creation") panics that guarded it. Adds a per-flag Bun.Glob.scan option test: each boolean option maps to exactly one named setter, so flipping one at a time against a fixed result set catches a flag wired to the wrong setter.

github-actions Bot added the claude label Jun 27, 2026

coderabbitai Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread Cargo.toml Outdated

Comment thread src/glob/GlobWalker.rs Outdated

Comment thread src/http/AsyncHTTP.rs Outdated

Comment thread src/install/lockfile/Package.rs

Comment thread src/runtime/webcore/Blob.rs

Comment thread src/runtime/webcore/fetch/FetchTasklet.rs Outdated