fix(#605): retry sandbox cleanup with chmod on permission denied - #620
fix(#605): retry sandbox cleanup with chmod on permission denied#620fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
When sandboxed processes create files with restrictive permissions (e.g. different UID), os.RemoveAll fails during repo extraction cleanup. This caused the entire run to abort and skip the post-script, losing review results (observed on 4 of 7 review runs for PR fullsend-ai#3193). Add forceRemoveAll helper that retries removal after making all entries owner-writable (chmod 0700). Change the cleanup at step 9d from a hard failure to a warning so the post-script always has a chance to run. Closes #605
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
🤖 Finished Review · ✅ Success · Started 11:05 AM UTC · Completed 11:17 AM UTC |
Review — PR #620fix(#605): retry sandbox cleanup with chmod on permission denied SummaryThe PR adds a Findings🔴 HIGH — Stale file contamination when cleanup fails in validation loopFile: The pre-PR code hard-aborted when With this PR, if Remediation: Extract into a fresh temporary directory and atomically rename into place, so a partial cleanup never contaminates the result. Alternatively, only downgrade to a warning when the cleanup error is a permission error ( 🟡 MEDIUM — All cleanup errors downgraded, not just permission errorsFile:
🔵 LOW — chmod fallback ineffective for different-UID filesFile: The issue and doc comment describe the failure as "files created by a sandboxed process with a different UID." 🔵 LOW — Test does not cover different-UID scenarioFile: The test exercises same-UID restrictive permissions, which is the scenario the fix actually addresses. A comment noting that the different-UID case is a known limitation (cannot test without root) would clarify intent. 🔵 LOW — Symlink-following in chmod walkFile:
🔵 LOW — Behavior change broader than stated scopeFile: The PR title says "retry sandbox cleanup with chmod on permission denied" but the call-site change also converts all cleanup failures (not just permission-denied) from hard errors to warnings. This broader behavior change should be explicitly acknowledged. Labels: PR fixes sandbox cleanup in CLI runtime code |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // Use forceRemoveAll to handle files created by sandboxed processes with | ||
| // restrictive permissions (#605). Warn on failure instead of aborting so | ||
| // the post-script still runs (review results are more valuable than a | ||
| // clean temp directory). |
There was a problem hiding this comment.
[high] stale-file-contamination
SafeDownload overlays onto the target directory without clearing it. When forceRemoveAll fails and the call site continues with a warning, files from the prior iteration that the sandbox deleted persist as ghost artifacts, potentially corrupting validation results in the iteration loop.
Suggested fix: Extract into a fresh temporary directory and atomically rename into place. Alternatively, only downgrade to warning on permission errors (errors.Is(clearErr, fs.ErrPermission)) and keep the hard abort for other error types, restricting warn-and-continue to the final iteration.
| // Use forceRemoveAll to handle files created by sandboxed processes with | ||
| // restrictive permissions (#605). Warn on failure instead of aborting so | ||
| // the post-script still runs (review results are more valuable than a | ||
| // clean temp directory). |
There was a problem hiding this comment.
[medium] error-handling
The call site downgrades ALL forceRemoveAll errors to warnings, not just permission errors. forceRemoveAll correctly distinguishes permission errors from other error types, but the caller discards that distinction. Non-permission errors (I/O errors, EBUSY, filesystem corruption) should still hard-abort as the original code intended.
Suggested fix: Add an errors.Is(clearErr, fs.ErrPermission) check at the call site: warn-and-continue for permission errors, hard-abort for all other error types.
| // forceRemoveAll removes a directory tree. If the initial removal fails with a | ||
| // permission error (e.g. files created by a sandboxed process with a different | ||
| // UID), it makes all entries owner-writable and retries. See #605. | ||
| func forceRemoveAll(path string) error { |
There was a problem hiding this comment.
[low] incorrect-documentation
The doc comment and issue describe the failure as different-UID files, but os.Chmod on different-UID files requires CAP_FOWNER and will fail. The function fixes same-UID restrictive directory permissions. The doc comment should accurately describe the scope.
| assert.True(t, os.IsNotExist(err)) | ||
| }) | ||
|
|
||
| t.Run("handles restrictive directory permissions", func(t *testing.T) { |
There was a problem hiding this comment.
[low] test-coverage
The test exercises same-UID restrictive directory permissions but does not document that the different-UID scenario is a known limitation that cannot be tested without root.
| if err == nil || !errors.Is(err, fs.ErrPermission) { | ||
| return err | ||
| } | ||
| // Best-effort chmod: make every entry owner-rwx so the retry can unlink. |
There was a problem hiding this comment.
[low] symlink-following
os.Chmod follows symlinks. While prior sanitizeDownload strips dangerous symlinks, adding a symlink guard (d.Type() and fs.ModeSymlink != 0) in the WalkDir callback is a one-line defensive improvement.
| // Use forceRemoveAll to handle files created by sandboxed processes with | ||
| // restrictive permissions (#605). Warn on failure instead of aborting so | ||
| // the post-script still runs (review results are more valuable than a | ||
| // clean temp directory). |
There was a problem hiding this comment.
[low] scope-mismatch
The PR title says retry on permission denied but the call-site change converts all cleanup failures to warnings, not just permission-denied. This broader behavior change should be explicitly acknowledged.
|
Run B invalidated — scaffold was pulling from upstream. Re-running with workflow fix. |
1 participant
When sandboxed processes create files with restrictive permissions (e.g. different UID), os.RemoveAll fails during repo extraction cleanup. This caused the entire run to abort and skip the post-script, losing review results (observed on 4 of 7 review runs for PR fullsend-ai#3193).
Add forceRemoveAll helper that retries removal after making all entries owner-writable (chmod 0700). Change the cleanup at step 9d from a hard failure to a warning so the post-script always has a chance to run.
Closes #605
Post-script verification
agent/605-force-remove-sandbox-cleanup)f6d95485ad7c634f52e66a32573856b85534daca..HEAD)