Repository navigation
Conversation
The concurrent git worker thread printed its "[Xs] git" timing line the moment git finished, racing with postinstall output streaming on the main thread and landing mid-stream. Record the elapsed time instead and print the line from the main thread after GitHandler::wait(), once postinstall has completed. The synchronous path (--no-install or no dependencies) still prints inline. Also fix bun-create postinstall tasks starting with "bun " on POSIX: stripping the "<exe> run" prefix left the bare string "bun" as argv[0], which posix spawn does not resolve against PATH, so the task silently never ran (ENOENT was swallowed). Spawn with the absolute self path and report spawn failures instead of discarding them. Fixes #36953
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WalkthroughChanges
bun create runtime behavior
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: 2
🤖 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/create_command.rs`:
- Around line 149-157: Update exec_task to return crate::Result<()> and convert
both spawn and task-execution failures into returned errors that include the
task text, rather than only logging them. Propagate exec_task errors from both
preinstall and postinstall task loops through CreateCommand::exec so bun create
exits nonzero on failure, and add a regression test covering a nonzero
postinstall task.
- Around line 1117-1125: Invert the assignment to create_options.skip_git in the
Git setup block so it reflects whether Git creation failed rather than
succeeded. Keep the existing created result and timing behavior unchanged,
ensuring successful Git setup reports its status and failed setup retains the
skip behavior.
🪄 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: 5233ec99-6ee3-47bf-9fcb-3a60dc81f8ef
📒 Files selected for processing (2)
src/runtime/cli/create_command.rstest/cli/install/bun-create.test.ts
The no-dependencies path set skip_git to the git success instead of its negation, suppressing the final message on success and printing it when git was missing. Also tighten two comments.
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)
src/runtime/cli/create_command.rs (1)
91-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParse the task before matching
bun.The current split recognizes only literal spaces and preserves empty fields. Therefore,
bun\t scripts/install.tsbypasses thebunpath, and repeated spaces can add emptyargventries. Parse the task into arguments, match the first argument, and add tab and repeated-whitespace cases to the postinstall test matrix.🤖 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/cli/create_command.rs` around lines 91 - 99, Update the task parsing in the npm client handling around the bun detection and argv construction: tokenize task using whitespace-aware parsing that ignores empty fields, then inspect the first parsed argument rather than checking the raw task prefix. Preserve the existing bun execution behavior and add postinstall coverage for tab-separated and repeated-whitespace bun commands.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 `@src/runtime/cli/create_command.rs`:
- Around line 91-99: Update the task parsing in the npm client handling around
the bun detection and argv construction: tokenize task using whitespace-aware
parsing that ignores empty fields, then inspect the first parsed argument rather
than checking the raw task prefix. Preserve the existing bun execution behavior
and add postinstall coverage for tab-separated and repeated-whitespace bun
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d787d7bf-c5cd-48c9-ba57-1106c31b1bff
📒 Files selected for processing (2)
src/runtime/cli/create_command.rstest/cli/install/bun-create.test.ts
|
Re the outside-diff note about tokenizing the task before matching |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The argv[0] substitution was gated on the npm client being present, but a template with a bun-create postinstall and no dependencies reaches exec_task without one, leaving the bare string "bun" as argv[0] and the same posix ENOENT. Gate on the task prefix alone and resolve the self path directly.
There was a problem hiding this comment.
LGTM — both earlier findings are addressed in 3215e1a: the bun -prefixed argv[0] substitution now gates on the task prefix alone via self_exe_path() (with a dependency-less-template regression test), and the fixture's sleep 0.5 carries the required comment.
Extended reasoning...
Overview
This PR touches src/runtime/cli/create_command.rs (~50 net lines) and adds four tests to test/cli/install/bun-create.test.ts. It fixes three related bun create defects: (1) the [Xs] git timing line was printed from the git worker thread and interleaved with postinstall output — the elapsed time is now stored in a static AtomicU64 and printed from the main thread after GitHandler::wait() (or inline on the synchronous path); (2) bun -prefixed bun-create postinstall tasks silently ENOENT'd on POSIX because posix spawn does no PATH lookup — argv[0] is now the absolute self path while the printed line keeps bun, and spawn errors are reported instead of discarded; (3) the synchronous git path assigned skip_git to the success value instead of its negation, suppressing the "A local git repository was created" message.
Prior review follow-up
I previously flagged that the argv[0] fix was gated inside if let Some(ref client) = npm_client, leaving the dependency-less-template path (which reaches exec_task with npm_client = None) unfixed. Commit 3215e1a moves the gate to strings::starts_with(task, b"bun ") alone, resolves the path via bun_core::self_exe_path() (a &'static ZStr), and adds a test with no dependencies field that asserts the marker file was written. I also asked for a comment on the bare sleep 0.5 in the git-timing fixture; that comment is present. CodeRabbit's skip_git inversion note and the comment-cop long-comment nits were addressed in 2d2ddbb; robobun's decline of the "propagate task exit code" suggestion is a reasonable scope call (pre-existing policy).
Security risks
None. This is CLI scaffolding output ordering plus argv[0] resolution to the current process's own executable path. No user-controlled data reaches a new sink; the task string parsing (single-space split, bun prefix match) is unchanged pre-existing behavior.
Level of scrutiny
Low-to-moderate. CLI-only, no JSC/GC interaction, no allocator or lifetime changes. The one cross-thread piece — a Release store of elapsed nanoseconds paired with an Acquire load after join() — is straightforward and mirrors the existing SUCCESS atomic. print_start_end(0, elapsed as i128) computes the same delta the original inline call did.
Other factors
Four new tests cover the interleave (POSIX-only via a shell-script git stub with a two-file handshake), both bun -prefixed postinstall variants (with and without dependencies), and the skip_git message. The PR body's evidence shows the file failing on both the debug-ASAN main and release 1.3.14 and passing with the fix. The last test relies on system git rather than a stub — consistent with other tests in this file, and bunEnv spreads process.env so PATH is inherited; the finder/verifier pass ruled this out as a hermeticity concern.
|
Re-verified against current main (7d276b9): the branch merges cleanly, a debug build of the merge passes #38889 was a later PR for the same issue and has been closed in favor of this one. |
|
Heads up on the |
1 participant
Fixes #36953
Repro
A template with dependencies and a
bun-create.postinstalltask that streams output.bun createrunsgit init/add/commiton a worker thread concurrently with install + postinstall, and the worker printed its timing line the moment git finished:Cause
GitHandler::runprinted[Xs] gitdirectly from the git worker thread, racing with the postinstall child process writing to the same inherited stderr on the main thread.Fix
GitHandler::runnow records the elapsed time in an atomic, and the line is printed from the main thread afterGitHandler::wait()returns, i.e. after postinstall has completed. The synchronous path (--no-installor a dependency-less template) still prints inline as before.While reproducing, a second bug in the same code path surfaced: on POSIX, a
bun-create.postinstalltask starting withbun(exactly the reporter's template,"postinstall": "bun scripts/install.ts") silently never ran.exec_taskstrips the<exe> runprefix, leaving the bare stringbunas argv[0], and posix spawn does no PATH lookup, so the spawn failed with ENOENT and the error was discarded. The task now spawns with the absolute self path (display still showsbun), and spawn failures are reported instead of swallowed. This was present before the Rust port (verified on bun 1.3.14); Windows was unaffected because uv_spawn resolves argv[0] against PATH.Verification
Two tests in
test/cli/install/bun-create.test.ts:gitwhosecommitis sequenced against the postinstall task so the unfixed binary reliably prints[Xs] gitbetween postinstall lines; asserts the timing line comes after the last postinstall line"postinstall": "bun scripts/marker.ts"; asserts the task actually ranBoth fail on the unfixed binary and pass with the fix; the full file (23 tests) passes.
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The git initialization runs on a background thread concurrently with dependency installation, and that thread printed its timing line directly to the terminal as soon as it finished, so the log raced with a postinstall task writing output on the main thread and landed mid-stream. The fix stops the worker thread from printing, instead recording the elapsed git time and emitting the timing line from the main thread only after the git work has completed and the postinstall output is done. As a related hardening, bun-prefixed postinstall commands now spawn with the resolved absolute Bun executa…