Stop the Go fixture stranding a read-only module cache, and pin review cleanup containment - #2973
Merged
jonathanKingston merged 2 commits intoSep 22, 2026
Merged
Conversation
#2945 reported review-ground cleanup failing with EACCES while unlinking a Go module-cache fixture inside the review cell's own temporary directory. The removal itself was hardened shortly afterwards in #2947 and #2954, but the negative coverage that issue asks for was never added, so nothing holds the containment properties in place. Add the three cases the acceptance criteria name: - a read-only, Go-shaped module cache is removed from the owned cell; - permission repair never touches a path outside that cell; - repair follows no symlink out of it — an outside sentinel keeps both its contents and its mode. Also fix a silent gap in the existing race regression. It relied on a real EACCES to drive its chmod interception, which root never sees: the mode bits stop nothing, the first `rm` succeeds, and the interception the test exists to exercise is skipped while the test still reports green. All four cases now force the recovery path explicitly, so they assert the same thing whoever runs them. Mutation-checked: removing the EACCES recovery fails all four; the symlink case fails once both containment guards (the `lstat` check and the `readdir` directory filter) are removed — either alone still holds the line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KtZSwBeY9GbEPkM3Tkb8f3
Both fixtures in this file create their temporary directory, then run setup — including `go mod tidy`, which populates a deliberately read-only module cache — before entering the try whose finally removes it. Any failure in that window (the zip helper, `go mod tidy`, or sandbox initialisation) strands the directory with read-only contents still in it. Observed locally: a full `pnpm run check` whose sandbox could not initialise left /tmp/go-preparation-test-*/author-cache/example.test/dep@v1.0.0/dep.go at mode 0444 under a 0555 parent — the same path shape #2945 reported the review cell failing to unlink with EACCES. When this fixture runs inside a review cell's own temporary directory, that stranded tree is what the cell's cleanup then trips over. Tie teardown to the directory's own lifetime with `t.after` instead of the try, so it runs wherever setup fails. Sandbox state stays in the finally, which is the only part that depends on the body having started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KtZSwBeY9GbEPkM3Tkb8f3
jonathanKingston
force-pushed
the
claude/open-issues-priority-review-yooklc
branch
from
September 22, 2026 19:41
b6f5340 to
1cad773
Compare
Contributor
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Copse Reviewer
Executed in the ephemeral-runner backend (container).
Checks: build ✓, typecheck ✓, lint ✓, test ✓.
Verification: 0 attempted — 0 confirmed by reproducer, 0 refuted, 0 survived challenge, 0 undetermined.
Reviewers: qwen3.8-27b.
Head: 1cad77304e5e.
No findings.
Advisory, not a gate. copse-review 0.1.0 ·
jonathanKingston
marked this pull request as ready for review
September 22, 2026 20:03
jonathanKingston
enabled auto-merge
September 22, 2026 20:04
jonathanKingston
deleted the
claude/open-issues-priority-review-yooklc
branch
September 22, 2026 20:18
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2945.
Outcome
The production cleanup recovery landed earlier in #2947 and #2954. This change closes the two remaining regression gaps:
t.after, so setup failures cannot strand a read-only module cache before the test body reaches itsfinallyremoveTreecontainment with deterministic coverage for a read-only Go-shaped tree, outside permission preservation, symlink escape prevention, and the root-runner recovery pathOnly test code changes. The reviewer implementation, permission policy, and benchmark specimens are unchanged.
Review
The complete diff was independently reviewed after rebasing onto current
main. The cleanup repair remains confined to the owned tree: it useslstat, traverses only directory entries reported as directories, and never follows the planted symlink. The test mock forces the first owned-root removal to fail withEACCES, so the recovery assertions execute even on a privileged runner.Validation
remove-treesuite: 4/4 passedprecheck,check, and aggregateCI Passedall green1cad77304e5e; build, typecheck, lint, and tests clean; cleanup, artifact upload, and handoff succeededThe original PR-specific baseline failure mentioned in #2945 was separate from cleanup. It is not reproduced by this rebased head and is not being treated as a reason to modify a specimen PR.