install: stage git cache folders and rename on success; hit only tagged checkouts - #38274
Closed
Jarred-Sumner wants to merge 1 commit into
Closed
install: stage git cache folders and rename on success; hit only tagged checkouts#38274Jarred-Sumner wants to merge 1 commit into
Jarred-Sumner wants to merge 1 commit into
Claude / Claude Code Review
completed
Aug 13, 2026 in 19m 21s
Code review found 3 potential issues
Found 3 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/install/repository.rs:1018-1035 |
Repo with a .bun-tag directory hard-fails install |
| 🟡 Nit | src/install/repository.rs:424-429 |
CacheStaging cleanup is manual, not RAII |
| 🟡 Nit | test/cli/install/bun-install.test.ts:5428 |
Success-path installs assert only exitCode |
Annotations
Check warning on line 1035 in src/install/repository.rs
claude / Claude Code Review
Repo with a .bun-tag directory hard-fails install
`delete_file_z` is `unlinkat(.., 0)` (no `AT_REMOVEDIR`), so a repository that checks in a **directory** named `.bun-tag` survives the unlink and then `O_CREAT|O_EXCL` fails EEXIST — the install hard-fails with `writing ".bun-tag" failed: EEXIST`, whereas before this PR it installed (with degraded verify). `dir.delete_tree(b".bun-tag")` (matching `.git` two lines up) would make the comment "anything the repository checked in under that name is replaced" true; alternatively the error could name t
Check warning on line 429 in src/install/repository.rs
claude / Claude Code Review
CacheStaging cleanup is manual, not RAII
nit: `CacheStaging` names a temp directory that must be removed on every error path, and the six error returns between `CacheStaging::new()` and `publish()` each manually call `staging.discard()`. All current paths do so correctly, but per CLAUDE.md #8 ("Prefer RAII (`Drop`) over manual cleanup") a `Drop` impl that calls `discard()`, disarmed by a `published: bool` set in `publish()`, would collapse those calls and stay correct if a future `?` or early return is added. Not blocking — worst-case
Check warning on line 5428 in test/cli/install/bun-install.test.ts
claude / Claude Code Review
Success-path installs assert only exitCode
nit: the three success-path checks (`expect(await install()).toMatchObject({ exitCode: 0 })`, also 5444/5451) assert only `exitCode`. `toMatchObject`'s failure diff does print the full received object so `err` is not actually hidden, but destructuring and asserting `err` first — as the failure path at 5438-5440 already does — would surface git's stderr as the primary diff line rather than buried in the `+ Received` block. Worth it here since the PR description defers Windows rename behavior to C
Loading