install: fix isolated-linker deadlock installing patched git/github dependencies in workspaces - #37136
Closed
robobun wants to merge 5 commits into
Closed
install: fix isolated-linker deadlock installing patched git/github dependencies in workspaces#37136robobun wants to merge 5 commits into
robobun wants to merge 5 commits into
Claude / Claude Code Review
completed
Aug 7, 2026 in 26m 28s
Code review found 4 potential issues
Found 2 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 4 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/install/isolated_install.rs:2407 |
Dead set_length(full_len) restore that would yield a NUL-embedded path if ever live |
| 🟡 Nit | src/install/isolated_install.rs:2359-2369 |
Multi-line explanatory comments should collapse to one-liner + issue link |
Annotations
Check warning on line 2407 in src/install/isolated_install.rs
claude / Claude Code Review
Dead set_length(full_len) restore that would yield a NUL-embedded path if ever live
The `full_len` capture (line 2370) and `pkg_cache_dir_subpath.set_length(full_len)` restore (line 2407) are dead — grep confirms nothing reads `pkg_cache_dir_subpath` after 2407. Worse, the restore is subtly wrong: in the `PatchInfo::Patch` case the `_ =>` arm's `slice_z()` has already written `buf[idx] = 0` (Path.rs:878), so "restoring" to `full_len` yields `<prefix>\0patch_hash=<hex>`, not the original patched subpath. Either drop both lines, or copy the truncated prefix into a separate `PathB
Check warning on line 2369 in src/install/isolated_install.rs
claude / Claude Code Review
Multi-line explanatory comments should collapse to one-liner + issue link
The 11-line comment at 2359-2369 (and the 3-line "`Remove` also lands here" at 2386-2388) violate REVIEW.md's "Only comment what the code cannot say. One line. … Prefer links to GitHub issues." The comment-cop bot's generic "the code is wrong — fix the code" message is misleading here: the code *is* the correct fix (it mirrors the hoisted installer's `package_missing_from_cache`), so the remedy is just to collapse the comment to a one-liner + link, e.g. `// Check the *unpatched* cache folder — r
Loading