fix(#5529): make pre-extraction cleanup non-fatal on permission errors - #5530
fix(#5529): make pre-extraction cleanup non-fatal on permission errors#5530fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
The harness cleanup of the host download directory (forceRemoveAll)
was a hard gate — if it failed with EACCES due to a UID mismatch
between the sandbox user and the host process, the entire run
returned an error. This cascaded: the post-script was skipped
("Skipping post-script: validation did not pass"), silently losing
review results. 4 of 7 review agent runs on PR #3193 failed this way.
Two changes:
1. Make the pre-extraction forceRemoveAll call best-effort: on failure,
log a warning and proceed with extraction. A stale download dir is
recoverable; lost post-script output is not.
2. Improve forceRemoveAll with a two-pass strategy: the first pass
chmod's directories only (fast path for readonly_repo enforcement);
if os.RemoveAll still fails with a permission error, a second pass
chmod's both directories and files before retrying. This handles
files created with restrictive modes (e.g. 0o000) by a sandboxed
process running as the same UID. When the tree is owned by a
different UID, chmod fails and the caller handles the error
gracefully via the non-fatal path.
Note: pre-commit could not run in sandbox (network error fetching
hooks). The post-script runs it authoritatively on the runner.
Closes #5529
|
🤖 Finished Review · ✅ Success · Started 1:52 PM UTC · Completed 2:06 PM UTC |
Site previewPreview: https://f91dbec9-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Labels: PR fixes a bug in the harness sandbox cleanup logic (internal/cli/run.go), directly referencing issue #5529 |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // Removing a path that does not exist should succeed (same as os.RemoveAll). | ||
| require.NoError(t, forceRemoveAll(filepath.Join(t.TempDir(), "does-not-exist"))) | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] test adequacy
TestForceRemoveAll_RestrictiveFilePerms does not actually exercise the second pass of forceRemoveAll. On Linux, deleting a file requires write permission on the parent directory, not on the file itself. After pass 1 restores directory permissions to 0o755, os.RemoveAll succeeds without needing the file-level chmod in pass 2. The test comment claims it exercises pass 2 but pass 1 alone is sufficient.
Suggested fix: Update the test comment to accurately describe what it tests (pass 1 directory-permission restoration), or add a separate test targeting pass 2 behavior on a platform where file-level permissions affect deletion.
| // the entire run and losing review results (#5529), we warn and let | ||
| // the extraction proceed — a stale download dir is recoverable, but | ||
| // lost post-script output is not. | ||
| if clearErr := forceRemoveAll(hostRepositoryDownloadDir); clearErr != nil { |
There was a problem hiding this comment.
[low] data-integrity
When forceRemoveAll fails and the pre-extraction cleanup is skipped, SafeDownload extracts the new sandbox content on top of the stale hostRepositoryDownloadDir. Files from a previous iteration that are not overwritten by the new download will persist and are passed to the post-script via REPO_DIR. This is an intentional design tradeoff.
Suggested fix: Consider logging which files remain in the stale directory so operators can audit whether the post-script was affected.
fullsend-ai-review
Bot
added
ready-for-merge
|
Nobody is tracking this down right now. It's been sitting in review for a month. |
|
🤖 Finished Retro · ❌ Failure · Started 8:57 PM UTC · Completed 8:57 PM UTC Commit: |
1 participant
Summary
forceRemoveAllcleanup call non-fatal: on permission errors (UID mismatch between sandbox and host), log a warning and proceed with extraction instead of failing the entire run. This prevents cascading failures that skip the post-script and silently lose review results.forceRemoveAllwith a two-pass chmod strategy: the first pass fixes directories (fast path for readonly_repo), and ifos.RemoveAllstill fails with EACCES, a second pass also chmod's files before retrying. This handles restrictive file modes from sandboxed processes.Context
On PR #3193, 4 of 7 review agent runs failed with
permission deniedduring cleanup of the host download directory. The sandbox created files owned by the sandbox UID, which the host process could not chmod or remove. The hard error caused the post-script to be skipped entirely, losing all review results.Testing
TestForceRemoveAll_RestrictiveFilePerms— verifies the two-pass strategy handles files with mode 0o000 in read-only directoriesTestForceRemoveAll_*tests continue to passgo vetpassesCloses #5529
Post-script verification
agent/5529-resilient-cleanup)e45db7d74f778894558047adfaef5c2d52adcbb9..HEAD)