fix(#5460): restore permissions before removing read-only download dir

[fullsend-ai-coder]

Summary

Fixes the harness cleanup failure where os.RemoveAll fails with unlinkat ... permission denied on directories extracted from a read-only sandbox repo. The readonly_repo enforcement removes write permissions from all files and directories (excluding .git/), and those permissions persist when the repo is extracted back to the host via SafeDownload. Adds a forceRemoveAll helper that restores owner-write permission on directories before deletion.

Related Issue

Fixes #5460

Changes

• Add forceRemoveAll() function in internal/cli/run.go that walks the directory tree restoring u+rwx on directories, then calls os.RemoveAll
• Replace os.RemoveAll(hostRepositoryDownloadDir) at both call sites: the defer cleanup (line ~910) and the pre-extraction clear in the validation loop (line ~1391)
• Add three tests: read-only tree removal, already-writable tree, and non-existent path

Testing

• TestForceRemoveAll_ReadOnlyTree — creates a nested directory tree, removes all write permissions (mirroring chmod a-w), verifies os.RemoveAll fails, then verifies forceRemoveAll succeeds
• TestForceRemoveAll_AlreadyWritable — verifies normal directories are cleaned up without issues
• TestForceRemoveAll_NonExistent — verifies non-existent paths return nil (matching os.RemoveAll behavior)
• go vet ./internal/cli/... passes
• All existing cli tests pass (4 pre-existing FetchService failures unrelated to this change)

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• I wrote this contribution myself and can explain all changes in it

---

Closes #5460

Post-script verification

• Branch is not main/master (agent/5460-force-remove-readonly-dirs)
• Secret scan passed (gitleaks — 98e92fa6db387b8e8eb751e687cfcc41e295821b..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 4:27 PM UTC · Ended 4:49 PM UTC
Commit: b15ecbb · View workflow run →

[github-actions]

Site preview

Preview: https://3d9d8a5c-site.fullsend-ai.workers.dev

Commit: 6e922fd30334a621c3b650a409a86a63db8f196d

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 4:27 PM UTC · Completed 4:49 PM UTC
Commit: b15ecbb · View workflow run →

[ggallen]

Review agent results (extracted from artifact)

The review agent completed successfully and produced an approve result, but the harness crashed during post-review cleanup before it could post the approval. The crash is the exact bug this PR fixes:

Error: clearing local repo /tmp/agent-review-7267-1784737665 before extraction:
  unlinkat /tmp/agent-review-7267-1784737665/website/.gitignore: permission denied

Review findings

One low-severity finding (scope-completeness at run.go:3207):

The forceRemoveAll function silently ignores os.Chmod errors during the permission-restoration walk. If chmod fails for an unexpected reason (e.g., a TOCTOU race on the directory tree), the subsequent os.RemoveAll error may not clearly indicate the root cause. Consider logging chmod failures for operator visibility.

Source

Results extracted from workflow run 29938002439, artifact fullsend-review → iteration-2/output/agent-result.json.

[ggallen]

/fs-fix fix the issue reported in #5474 (comment).

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:59 PM UTC · Completed 6:05 PM UTC
Commit: 7ada4e0 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 1 review finding: forceRemoveAll now logs chmod failures to stderr for operator visibility, matching the file's existing warning pattern.

Fixed (1):

1. forceRemoveAll silently ignores os.Chmod errors (internal/cli/run.go): Captured os.Chmod errors and logged them to stderr using the file's existing fmt.Fprintf(os.Stderr, "WARNING: ...") pattern. Both chmod call sites (parent directory restore and directory permission restore) now log failures with the path and error, giving operators visibility into the root cause when the subsequent os.RemoveAll fails.

Tests: passed

Decision points

• Chose fmt.Fprintf(os.Stderr) over structured logging (alternatives: slog/log package, Return chmod errors; rationale: The file consistently uses fmt.Fprintf(os.Stderr, "WARNING: ...") for diagnostic warnings (10+ existing instances). The function is best-effort cleanup, so returning chmod errors would change its contract unnecessarily.)

_{Updated by fullsend fix agent}

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:31 PM UTC · Completed 6:44 PM UTC
Commit: 7ada4e0 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [error-handling-coherence] internal/cli/run.go:3200 — forceRemoveAll uses fmt.Fprintf(os.Stderr) for chmod warning messages, while most user-facing diagnostics in run.go go through the printer.StepWarn abstraction. Both patterns are established in this file (30+ printer.StepWarn call sites vs 13+ fmt.Fprintf(os.Stderr, "WARNING: ...") call sites for infrastructure-level diagnostics), so this is consistent with the WARNING:-prefixed infrastructure diagnostic convention. Consider using printer.StepWarn for consistency if the function signature is ever refactored to accept a printer.

---

Labels: PR labels (type/bug, component/harness) correctly match the change area

Previous run

Review

Findings

Low

• [edge-case] internal/cli/run.go:3211 — WalkDir's single-pass traversal means that if a directory lacks read+execute permissions, its children are skipped without retry after the parent is fixed via os.Chmod. Currently unreachable because readonly_repo enforcement only strips write permission (chmod a-w), preserving read+execute. Consider documenting this assumption so future callers know the limitation.
• [path-traversal] internal/cli/run.go:3216 — In the error-path branch, os.Chmod(filepath.Dir(p), 0o755) operates on a string-computed parent path without an os.Lstat guard to confirm the target is a real directory (not a symlink). Mitigated by SafeDownload stripping dangerous symlinks before forceRemoveAll runs; an os.Lstat check would add defense-in-depth.
• [error-handling-idiom] internal/cli/run.go:3211 — The //nolint:errcheck directive is not used elsewhere in internal/cli/ but is consistent with internal/sandbox/ conventions for best-effort operations.

---

Labels: PR fixes a bug in the harness cleanup flow (internal/cli/run.go), matching component/harness and type/bug labels from the linked issue

[ggallen]

/fs-fix - Rebase and fix the issues in #5474 (review), replying to and resolving all conversations.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 6:56 PM UTC · Completed 7:04 PM UTC
Commit: 7ada4e0 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Addressed all 3 review findings: documented the WalkDir single-pass assumption, added os.Lstat defense-in-depth guard before chmod in the error path, and added inline nolint explanation matching sandbox conventions. All existing tests pass.

Fixed (3):

1. edge-case: WalkDir single-pass traversal assumption undocumented (internal/cli/run.go): Added doc comment to forceRemoveAll documenting the assumption that readonly_repo only strips write permission (chmod a-w), preserving read+execute bits. WalkDir cannot retry children after fixing a parent, so if a directory also lacked r+x its children would be silently skipped. This is currently unreachable but now explicitly documented for future callers.
2. path-traversal: os.Chmod(filepath.Dir(p)) without os.Lstat guard (internal/cli/run.go): Added os.Lstat check before os.Chmod in the error-path branch to verify the parent is a real directory, not a symlink. This provides defense-in-depth alongside SafeDownload's existing symlink stripping.
3. error-handling-idiom: //nolint:errcheck not used elsewhere in internal/cli/ (internal/cli/run.go): Added inline explanation after //nolint:errcheck matching the internal/sandbox/ convention for best-effort operations. The comment clarifies that errors are logged individually and the final os.RemoveAll is the authoritative result.

Tests: passed

_{Updated by fullsend fix agent}

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:10 PM UTC · Completed 7:23 PM UTC
Commit: 7ada4e0 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:20 PM UTC · Completed 8:43 PM UTC
Commit: 6e922fd · View workflow run →

[fullsend-ai-retro]

Retro: PR #5474 — fix(#5460): restore permissions before removing read-only download dir

Timeline

Time (UTC)	Event	Details
11:38	Issue #5460 filed	Retro agent identified cleanup permission denied bug from quay/quay-konflux-components PR #3362
16:09	Human adds second instance	ggallen confirms same failure on PR #5469 review run 29934396723
16:10	Triage dispatched	/fs-triage → run 29936763162 (success, ~5 min)
16:15	Code agent starts	Run 29937180639
16:26	PR #5474 created	Code agent adds forceRemoveAll helper + 3 tests (100 additions, 2 deletions)
16:26	Auto-review dispatched	Run 29938002439
16:28–16:38	Review iteration 1	Agent completed review but got sidetracked by issue-labels skill — never wrote agent-result.json
16:38–16:49	Review iteration 2	Agent wrote valid output (approve with 1 low finding), schema validation passed
16:49	Review crashed	Harness cleanup (os.RemoveAll) failed with permission denied on read-only website/.gitignore — the exact bug this PR fixes. Validation never ran for iteration 2. Post-script skipped. Review never delivered.
17:10	Human extracts results	ggallen manually reads artifacts, discovers the approve verdict + 1 finding, posts results as a comment
17:57	Fix #1 dispatched	/fs-fix → run 29944610686 — adds chmod error logging (~8 min)
18:30	Review #2 dispatched	/fs-review → run 29946949491 — approve with 3 low findings (~14 min)
18:55	Fix #2 dispatched	/fs-fix → run 29948779804 — addresses all 3 findings (~9 min)
19:08	Review #3 dispatched	/fs-review → run 29949729347 — clean approve, 0 actionable findings (~15 min)
19:26	Human approves	ggallen approves and merges
20:16	PR merged

What went well

Code agent quality was high. The initial implementation was correct — forceRemoveAll properly walks the directory tree restoring u+rwx on directories before calling os.RemoveAll, with appropriate error handling and 3 well-designed tests covering read-only, writable, and non-existent paths. Review findings were all low-severity hardening suggestions (documentation, symlink defense-in-depth, error logging), not correctness bugs.

Review agent findings were appropriate. All 3 findings from review #2 (WalkDir single-pass assumption, os.Lstat guard for symlinks, //nolint:errcheck convention) were genuine code quality improvements. The challenger agent in review #3 correctly filtered out 4 false positives (naming criticism, documentation formatting, test naming convention, implementation detail misunderstanding), demonstrating effective adversarial verification.

Fix agent executed cleanly. Both fix iterations addressed all findings correctly and quickly (~8–9 min each).

Autonomy signal: The human reviewer approved with zero additional findings beyond what the review agent identified. The human's role was entirely operational (triggering fix/review cycles, manually relaying crashed review results). For this class of change (well-tested Go bug fix with good coverage), the review agent demonstrated sufficient coverage.

What could improve

The first review's valid output was lost due to two interacting issues, adding ~1.5 hours and ~$3.50 of wasted compute:

1. Review iteration 1 failed to write output (#2886) — The transcript from run 29938002439 shows the review orchestrator dispatched the issue-labels skill at the end of its session and hit end_turn before writing agent-result.json. This is a new instance of the output-write reliability problem tracked in Review agent should guarantee output file is written before exit #2886.

2. Cleanup crash prevented iteration 2's validation (#5393) — After iteration 2 produced valid output, the harness attempted os.RemoveAll on the download directory (line 1391 of run.go) to clear it before repo re-extraction for validation. This failed with unlinkat ... permission denied because readonly_repo enforcement had stripped write permissions. The crash preempted validation, leaving validationPassed = false, which caused the post-script defer to skip delivery. This is the same class of issue as Harness should defer validation until all retry iterations complete #5393 — the iteration/validation loop losing valid agent output due to an intermediate failure — with a different trigger (cleanup crash vs. timeout race).

The root cause (permission denied on read-only dirs) is fixed by this PR. The systemic resilience issue (validation loop fragility) is tracked in #5393.

No new proposals

All improvement opportunities identified in this retro are covered by existing open issues. No new proposals are warranted.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!