fix(runner): extract sandbox output to temp dir instead of overwriting target repo

[rh-hemartin]

Summary

Breaking change: fullsend run code no longer writes sandbox output directly into the --target-repo directory. Results are extracted to a temporary directory ($TMPDIR/<sandbox>) that is cleaned up after the run. Pass --keep-sandbox to retain it for inspection.

• os.RemoveAll(hostRepositoryDir) ran before each SafeDownload, creating a window where the target repo was missing or partially populated. Concurrent readers (IDEs, file watchers, other agents) would hit ENOENT or see partial state.
• Post-script and validation-loop env vars (REPO_DIR, TARGET_REPO_DIR) now point to the temp download location.
• Results output now notes the download directory is ephemeral unless --keep-sandbox is set.
• --keep-sandbox help text updated to mention it also retains the download directory.
• Added TODO for removing stale REPO_DIR from harness RunnerEnv (Remove REPO_DIR from harness RunnerEnv agents#191).

Migration

Users relying on fullsend run code to modify their local checkout in place need to copy results from the printed download directory, or pass --keep-sandbox and use the temp path.

Closes #2075

Test plan

• Run a local agent with a post-script that reads $REPO_DIR — verify it points to temp dir
• Run a local agent with a validation loop — verify $TARGET_REPO_DIR points to temp dir
• Confirm original target repo directory is untouched after agent run
• Verify results output shows ephemeral hint without --keep-sandbox
• Verify results output shows plain path with --keep-sandbox

[qodo-code-review]

PR Summary by Qodo

Runner: extract sandbox repo to temp dir to avoid clobbering target repo

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Stop deleting the host target repo before each sandbox extraction.
• Download extracted repo content into a per-sandbox temp directory instead.
• Point post-script and validation env vars at the temp download location.

Diagram

graph TD
  A["runAgent (runner)"] --> B["Sandbox"] --> C["Repo output (sandbox)"] --> D["Download dir (tmp)"] --> E["Post/validation scripts"] --> F["Results output"]
  G["Target repo (host)"] --> B
  A -. "does not delete" .-> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Extract to temp dir, then atomically swap into target repo

• ➕ Keeps target repo path stable for consumers that require it
• ➕ Avoids partial states via atomic rename on the same filesystem
• ➖ Still modifies the user's target repo directory (undesirable in this issue)
• ➖ Rename semantics vary across filesystems and can fail cross-device

2. Use os.MkdirTemp for a unique download directory

• ➕ Avoids any theoretical name collisions in $TMPDIR
• ➕ Makes it explicit that this directory is ephemeral
• ➖ Requires plumbing the generated path through logging/env consistently
• ➖ May complicate reproducibility/debugging if names are random

3. Place download dir under runDir (outputBase)

• ➕ Keeps all artifacts grouped together for debugging/cleanup
• ➕ Avoids scattering temp directories across the system temp root
• ➖ If outputBase is user-controlled, may reintroduce interactions with watched paths
• ➖ May increase disk usage under the run output tree

Recommendation: The PR’s approach (extract to a per-sandbox temp directory and never touch the host repo) directly addresses the ENOENT/partial-state window and is the safest default for concurrent readers. Consider a follow-up improvement to use os.MkdirTemp (or document/clean up the chosen temp path) to make uniqueness and lifecycle explicit, but the current change is a solid fix for #2075.

Files changed (1) +9 / -5

Bug fix (1) +9 / -5

run.goExtract sandbox repo into temp downloadDir and export env vars +9/-5

Extract sandbox repo into temp downloadDir and export env vars

• Introduces a per-sandbox download directory under the system temp dir and uses it as the SafeDownload destination instead of the user’s target repo path. Updates post-script and validation-loop environments to point at the extracted temp repo (REPO_DIR/TARGET_REPO_DIR) and prints the download directory in the final results summary.

internal/cli/run.go

[github-actions]

Site preview

Preview: https://c6ac5e20-site.fullsend-ai.workers.dev

Commit: d539ddafd4d92306f7ac6e96ad0e027a6bc4a8bc

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:58 AM UTC · Completed 8:10 AM UTC
Commit: 63dc6cc · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 fullsend run: os.RemoveAll creates race condition with concurrent directory access

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Temp repo not cleaned ✓ Resolved 🐞 Bug ☼ Reliability

Description

runAgent now extracts the target repo into a global temp location (downloadDir) that is not under
the run output directory and is never removed after the run completes. This can leave large per-run
repo copies behind (and ignores --output-dir), leading to disk bloat and confusing cleanup.

Code

internal/cli/run.go[601]

+	downloadDir := filepath.Join(os.TempDir(), sandboxName)

Relevance

⭐⭐ Medium

No direct precedent on cleaning os.TempDir; team adds cleanup elsewhere but not consistently
enforced.

PR-#555
PR-#1595
PR-#1045

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code sets runDir under outputBase but sets downloadDir under os.TempDir() and uses it
for repo extraction; there is no corresponding cleanup of downloadDir after the run.

internal/cli/run.go[584-622]
internal/cli/run.go[1008-1023]
internal/cli/run.go[1103-1108]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`downloadDir` is set to `filepath.Join(os.TempDir(), sandboxName)` and used as the host extraction destination for the full repo. Unlike `runDir` (which is controlled by `--output-dir`), `downloadDir` is always in the process temp dir and is never cleaned up, so repeated runs can leave behind large extracted repositories.

## Issue Context
- `runDir` is derived from `outputBase` (which can be user-configured).
- `downloadDir` is derived from `os.TempDir()` and is printed in Results, but there is no lifecycle management (no cleanup defer, and it is not nested under `runDir`).

## Fix Focus Areas
- internal/cli/run.go[584-622]
- internal/cli/run.go[1008-1023]
- internal/cli/run.go[1103-1108]

### Suggested implementation approach
- Prefer placing the extracted repo under the run directory, e.g. `downloadDir := filepath.Join(runDir, "repo")` (or `filepath.Join(runDir, "download", repoName)`), so `--output-dir` controls all artifacts and a single directory deletion cleans everything.
- Optionally: if the intent is to keep it temporary, add a `defer os.RemoveAll(downloadDir)` gated behind a flag (e.g. `--keep-download`) or only when no post-script/validation needs it after printing results.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. runAgent repo refresh untested ✓ Resolved 📘 Rule violation ▣ Testability

Description

The PR changes runAgent to extract the target repo into a new temp downloadDir and threads that
path via REPO_DIR/TARGET_REPO_DIR, but the existing tests shown only cover early harness-loading
paths and fail before reaching the extraction/validation logic. This violates the requirement to
add/adjust tests for modified Go logic, risking regressions in repo extraction and
post-script/validation behavior.

Code

internal/cli/run.go[R1008-1025]

		// 9d. Extract target repo back to host. SafeDownload removes dangerous
		// symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape.
-		if clearErr := os.RemoveAll(hostRepositoryDir); clearErr != nil {
-			return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDir, clearErr)
+		if clearErr := os.RemoveAll(downloadDir); clearErr != nil {
+			return fmt.Errorf("clearing local repo %s before extraction: %w", downloadDir, clearErr)
		}
+
		repoExtractStart := time.Now()
		printer.StepStart("Extracting target repo")
-		if err := sandbox.SafeDownload(sandboxName, remoteRepositoryDir, hostRepositoryDir); err != nil {
+		if err := sandbox.SafeDownload(sandboxName, remoteRepositoryDir, downloadDir); err != nil {
			if es := tx.ParseTranscriptErrors(iterTranscriptDir); len(es) > 0 {
				tx.EmitTranscriptErrors(os.Stderr, es)
			}
			return fmt.Errorf("extracting target repo (iteration %d): %w", iteration, err)
		}
-		printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", hostRepositoryDir, time.Since(repoExtractStart).Seconds()))
+		printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", downloadDir, time.Since(repoExtractStart).Seconds()))

		// 9e. Run validation.
		if h.ValidationLoop == nil {

Relevance

⭐ Low

Requests to add tests for new run.go paths repeatedly rejected as too integration-heavy.

PR-#1238
PR-#2428
PR-#1627

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff shows new logic that deletes and extracts into downloadDir and passes downloadDir via
REPO_DIR/TARGET_REPO_DIR. The existing runAgent test explicitly states it fails earlier (at
sandbox availability) and thus does not cover the modified extraction path, indicating the new
behavior lacks corresponding test coverage.

Rule 1062049: Require tests for new or modified Go logic
internal/cli/run.go[601-622]
internal/cli/run.go[1008-1036]
internal/cli/run_test.go[142-168]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`runAgent` now downloads/extracts the sandbox repo into a temp `downloadDir` and exports it via `REPO_DIR`/`TARGET_REPO_DIR`, but current tests do not exercise or assert this changed behavior.

## Issue Context
The extraction destination and env var threading are behavioral changes that can silently break post-scripts/validation loops or reintroduce race conditions if refactored later.

## Fix Focus Areas
- internal/cli/run.go[601-622]
- internal/cli/run.go[1008-1036]
- internal/cli/run_test.go[142-168]

## Notes for implementation
- Add/extend tests to assert that the extraction destination is `downloadDir` (not `hostRepositoryDir`) and that `REPO_DIR` / `TARGET_REPO_DIR` are set accordingly.
- If current design makes this hard to test, introduce a narrow seam (e.g., interface or function injection) around the sandbox download/extract and command env construction so tests can verify the arguments/env without requiring an actual sandbox.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Re-review of d539dda (prior review: de48094, provenance: app-verified).

This is a new commit since the prior review. All 3 files changed. The core change — extracting sandbox output to $TMPDIR/<sandbox> instead of overwriting the target repo — remains sound and correctly addresses the race condition described in #2075. The download-dir cleanup defer is registered before the post-script defer, ensuring correct LIFO ordering. All affected call sites (SafeDownload, validationEnv, post-script REPO_DIR, results output) consistently use hostRepositoryDownloadDir.

Findings

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative from the bug fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. This PR removes 41 lines of tests for the deleted clearDirContents function and adds no replacement tests; the ignore entry suppresses codecov's 80% patch coverage threshold on a 3000+ line file containing critical runner logic, masking the coverage reduction.

  Remediation: Remove the .codecov.yml change from this PR. When clearDirContents tests are removed, either add equivalent tests for the new temp-directory approach, or defer test removal to issue refactor: break up runAgent for testability #2831's testability refactor.

• [breaking-change-marker] PR title — The PR body declares a "Breaking change:" but the title fix(runner): extract sandbox output to temp dir instead of overwriting target repo lacks the required ! suffix per AGENTS.md. GoReleaser builds release notes from merged PR titles; a missing ! makes the breaking change invisible in release notes.

  Remediation: Change the PR title to fix(runner)!: extract sandbox output to temp dir instead of overwriting target repo.

Low

• [env-override-fragility] internal/cli/run.go:914 — REPO_DIR is appended to postCmd.Env after childScriptEnv has already placed a REPO_DIR entry from h.RunnerEnv. This relies on Go's exec.Cmd passing duplicate env keys where the OS resolves to the last value. The inline comment added in this PR documents the intent, mitigating the prior concern.

• [test-adequacy] internal/cli/run_test.go — 41 lines of TestClearDirContents tests are removed (legitimate — the function is deleted) but no replacement tests are added for the new temp-dir extraction behavior. Key untested paths: the REPO_DIR append override, the download-dir cleanup defer ordering, and the pre-extraction os.RemoveAll on a non-existent vs. existing directory.

• [intent-coherence] internal/cli/run.go — The implementation (extract to temp dir, clean up after run) deviates from the atomic write pattern suggested in issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 (write-to-temp-then-os.Rename). The current approach is a larger behavioral change but is a valid design trade-off.

• [variable-naming] internal/cli/run.go:869 — hostRepositoryDownloadDir introduces a second host-path variable alongside the existing hostRepositoryDir, creating two similarly-named variables with different semantics (original target repo vs. temporary download location).

• [stale-flag-description] docs/guides/user/running-agents-locally.md:121 — The --keep-sandbox usage tip does not mention the new download directory preservation behavior. The tip remains factually correct but is incomplete.

• [missing-behavioral-change] docs/guides/user/running-agents-locally.md — The breaking behavioral change (sandbox output goes to temp dir) is not reflected in the user guide. The CLI output now prints the download directory path, but docs users may not discover this.

Previous run

Review

Re-review of de48094 (prior review: db67317, provenance: app-verified).

This is a rebase onto the latest main — the PR's code changes are identical to the prior review. All prior findings carry forward with anchored severities. The prior high-severity [resource-ordering] finding remains resolved: the download-dir cleanup defer is registered before the post-script defer, ensuring correct LIFO ordering.

The core change — extracting sandbox output to $TMPDIR/<sandbox> instead of overwriting the target repo — is sound and correctly addresses the race condition described in #2075. All affected call sites (SafeDownload, validationEnv, post-script REPO_DIR, results output) consistently use hostRepositoryDownloadDir.

Findings

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative from the bug fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. This PR removes 41 lines of tests for the deleted clearDirContents function and adds no replacement tests; the ignore entry suppresses codecov's 80% patch coverage threshold on a 3000+ line file containing critical runner logic, masking the coverage reduction.

  Remediation: Remove the .codecov.yml change from this PR. When clearDirContents tests are removed, either add equivalent tests for the new temp-directory approach, or defer test removal to issue refactor: break up runAgent for testability #2831's testability refactor.

Low

• [env-override-fragility] internal/cli/run.go:912 — REPO_DIR is appended to postCmd.Env after childScriptEnv has already placed a REPO_DIR entry from h.RunnerEnv (harness files code.yaml and fix.yaml define REPO_DIR in env.runner). This relies on Go's exec.Cmd passing duplicate env keys where the OS resolves to the last value. The behavior is correct on Linux but a brief inline comment would prevent future maintainers from removing the "duplicate" entry.

• [test-adequacy] internal/cli/run_test.go — 41 lines of TestClearDirContents tests are removed (legitimate — the function is deleted) but no replacement tests are added for the new temp-dir extraction behavior. Key untested paths: the REPO_DIR append override of the harness-defined value, and the download-dir cleanup defer running after the post-script.

Previous run (2)

Review

Re-review of db67317 (prior review: 264b65e, provenance: app-verified).

The prior review's high-severity [resource-ordering] finding has been resolved. The download-dir cleanup defer is now registered before the post-script defer (line 820), ensuring Go's LIFO defer ordering runs: sandbox cleanup → post-script → download-dir cleanup. The post-script's REPO_DIR correctly points to a directory that still exists during execution.

Findings

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative from the bug fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075.

Low

• [test-adequacy] internal/cli/run_test.go — 41 lines of TestClearDirContents tests are removed but no replacement tests are added.

• [env-override-fragility] internal/cli/run.go:864 — REPO_DIR is appended to postCmd.Env after childScriptEnv has already placed a REPO_DIR entry from h.RunnerEnv. The override is correct but undocumented.

Previous run (3)

Review

Findings

High

• [resource-ordering] internal/cli/run.go:878 — The download directory cleanup was placed inside the sandbox cleanup defer, causing it to run before the post-script. Resolved in db67317.

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831, separate from fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075.

Previous run

Review

Re-review of de48094 (prior review: db67317, provenance: app-verified).

This is a rebase onto the latest main — the PR's code changes are identical to the prior review. All prior findings carry forward with anchored severities. The prior high-severity [resource-ordering] finding remains resolved: the download-dir cleanup defer is registered before the post-script defer, ensuring correct LIFO ordering.

The core change — extracting sandbox output to $TMPDIR/<sandbox> instead of overwriting the target repo — is sound and correctly addresses the race condition described in #2075. All affected call sites (SafeDownload, validationEnv, post-script REPO_DIR, results output) consistently use hostRepositoryDownloadDir.

Findings

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative from the bug fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. This PR removes 41 lines of tests for the deleted clearDirContents function and adds no replacement tests; the ignore entry suppresses codecov's 80% patch coverage threshold on a 3000+ line file containing critical runner logic, masking the coverage reduction.

  Remediation: Remove the .codecov.yml change from this PR. When clearDirContents tests are removed, either add equivalent tests for the new temp-directory approach, or defer test removal to issue refactor: break up runAgent for testability #2831's testability refactor.

Low

• [env-override-fragility] internal/cli/run.go:912 — REPO_DIR is appended to postCmd.Env after childScriptEnv has already placed a REPO_DIR entry from h.RunnerEnv (harness files code.yaml and fix.yaml define REPO_DIR in env.runner). This relies on Go's exec.Cmd passing duplicate env keys where the OS resolves to the last value. The behavior is correct on Linux but a brief inline comment would prevent future maintainers from removing the "duplicate" entry.

• [test-adequacy] internal/cli/run_test.go — 41 lines of TestClearDirContents tests are removed (legitimate — the function is deleted) but no replacement tests are added for the new temp-dir extraction behavior. Key untested paths: the REPO_DIR append override of the harness-defined value, and the download-dir cleanup defer running after the post-script.

Previous run (2)

Review

Re-review of db67317 (prior review: 264b65e, provenance: app-verified).

The prior review's high-severity [resource-ordering] finding has been resolved. The download-dir cleanup defer is now registered before the post-script defer (line 820), ensuring Go's LIFO defer ordering runs: sandbox cleanup → post-script → download-dir cleanup. The post-script's REPO_DIR correctly points to a directory that still exists during execution.

The core change — extracting sandbox output to $TMPDIR/<sandbox> instead of overwriting the target repo — is sound and correctly addresses the race condition described in #2075. All affected call sites (SafeDownload, validationEnv, post-script REPO_DIR, results output) consistently use hostRepositoryDownloadDir.

Findings

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative from the bug fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. This PR removes 41 lines of tests for the deleted clearDirContents function and adds no replacement tests; the ignore entry suppresses codecov's 80% patch coverage threshold on a 3000+ line file containing critical runner logic, masking the coverage reduction.

  Remediation: Split the codecov change into a dedicated PR linked to issue refactor: break up runAgent for testability #2831. If the coverage gate is blocking this PR, add targeted tests for the new behavior (e.g., verify REPO_DIR override in post-script env, verify download-dir cleanup ordering).

Low

• [test-adequacy] internal/cli/run_test.go — 41 lines of TestClearDirContents tests are removed (legitimate — the function is deleted) but no replacement tests are added for the new temp-dir extraction behavior. Key untested paths: the REPO_DIR append override of the harness-defined value, and the download-dir cleanup defer running after the post-script.

• [env-override-fragility] internal/cli/run.go:864 — REPO_DIR is appended to postCmd.Env after childScriptEnv has already placed a REPO_DIR entry from h.RunnerEnv (harness defines REPO_DIR: "${GITHUB_WORKSPACE}/target-repo"). This relies on Go's exec.Cmd passing duplicate env keys where the OS resolves to the last value. The override is correct but undocumented — a brief inline comment would prevent future maintainers from removing the "duplicate" entry.

Previous run (3)

Review

Findings

High

• [resource-ordering] internal/cli/run.go:878 — The download directory cleanup was placed inside the sandbox cleanup defer, causing it to run before the post-script (which needs the download dir). Resolved in db67317 — cleanup is now a separate defer registered before the post-script.

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list references issue refactor: break up runAgent for testability #2831, separate from fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075.

Previous run (4)

Review

Re-review of 264b65e (prior review: 760c642, provenance: app-verified).

The prior review flagged a high-severity resource leak: hostRepositoryDownloadDir was never cleaned up. This revision adds cleanup inside the sandbox cleanup defer — fixing the leak but introducing a new ordering bug that breaks post-script execution.

Findings

High

• [resource-ordering] internal/cli/run.go:878 — The download directory cleanup (os.RemoveAll(hostRepositoryDownloadDir)) is placed inside the sandbox cleanup defer (registered at line 862), which runs before the post-script defer (registered at line 828) due to Go's LIFO defer ordering. The post-script receives REPO_DIR pointing to hostRepositoryDownloadDir (line 850), but that directory is deleted before the post-script executes. post-code.sh (line 53) checks [ ! -d "${REPO_DIR}" ] and exits with error 1 when the directory is missing. This will cause post-script failures for all agents that have a PostScript configured (code, fix agents).

  The source code comment at line 821 confirms the intentional ordering: "Post-script runs after sandbox cleanup (defers are LIFO)." The download dir cleanup was placed in the same defer as sandbox deletion, so it also runs before the post-script — but unlike the sandbox container, the download dir is still needed by the post-script.

  Remediation: Register the download directory cleanup as a separate defer before the post-script defer block (i.e., earlier in the source code). LIFO ensures it runs last — after both the post-script and sandbox cleanup complete:

  hostRepositoryDownloadDir := filepath.Join(os.TempDir(), sandboxName)
  // Registered first → runs last (LIFO). Cleans up after post-script completes.
  defer func() {
      if keepSandbox {
          return
      }
      if err := os.RemoveAll(hostRepositoryDownloadDir); err != nil {
          printer.StepWarn("Failed to remove download dir: " + err.Error())
      } else {
          printer.StepDone(fmt.Sprintf("Download directory removed: %s", hostRepositoryDownloadDir))
      }
  }()
  // Then register the post-script defer (runs second).
  if h.PostScript != "" {
      defer func() { /* ... */ }()
  }
  // Then register sandbox cleanup defer (runs first).
  defer func() { /* ... sandbox.Delete ... */ }()

  This gives the execution order: sandbox deleted → post-script runs (REPO_DIR exists) → download dir cleaned up.

Medium

• [scope-creep] .codecov.yml:28 — Adding internal/cli/run.go to the codecov ignore list is a project-wide coverage policy change unrelated to the race-condition fix in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. The comment references issue refactor: break up runAgent for testability #2831 (refactoring runAgent for testability), which is a separate initiative. This PR removes 41 lines of tests for the deleted clearDirContents function and adds no replacement tests, so the ignore entry masks the coverage reduction. Bundling the codecov change here conflates a policy decision with a bug fix.
  Remediation: Split the codecov change into issue refactor: break up runAgent for testability #2831 or a dedicated PR. If the coverage gate is blocking this PR, add targeted tests for the new extraction path instead (e.g., verify REPO_DIR is set correctly in the post-script env, verify download dir cleanup ordering).

Previous run (5)

Review

Findings

High

• [resource-leak] internal/cli/run.go:649 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. The sandbox cleanup defer only calls sandbox.Delete() to remove the container — it does not touch host-side directories. Each run leaves a full copy of the extracted repository in the system temp directory, and since sandbox names are unique (agent-<name>-<pid>-<timestamp>), these directories accumulate without bound. For local fullsend run usage (the explicit use case in issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075), these temp directories will silently consume disk over repeated runs.
  Remediation: Register a deferred cleanup after the post-script defer block so it runs after the post-script completes (LIFO ordering). For example:

  defer func() {
      if keepSandbox {
          return
      }
      _ = os.RemoveAll(hostRepositoryDownloadDir)
  }()

Previous run (6)

Review

Findings

High

• [resource-leak] internal/cli/run.go:649 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. The sandbox cleanup defer only calls sandbox.Delete() to remove the container — it does not touch host-side directories. Each run leaves a full copy of the extracted repository in the system temp directory, and since sandbox names are unique (agent-<name>-<pid>-<timestamp>), these directories accumulate without bound. For local fullsend run usage (the explicit use case in issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075), these temp directories will silently consume disk over repeated runs.
  Remediation: Register a deferred cleanup after the post-script defer block so it runs after the post-script completes (LIFO ordering). For example:

  defer func() {
      if keepSandbox {
          return
      }
      _ = os.RemoveAll(hostRepositoryDownloadDir)
  }()

Previous run (7)

Review

Findings

High

• [resource-leak] internal/cli/run.go:601 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(hostRepositoryDownloadDir) or any other cleanup mechanism. Each run of runAgent leaves a full copy of the extracted repository in the system temp directory. These temp directories use unique sandbox names (agent-<name>-<pid>-<timestamp>) and accumulate without bound. The sandbox cleanup defer only deletes the sandbox container via sandbox.Delete(), not host-side directories.
  Remediation: Register a deferred os.RemoveAll(hostRepositoryDownloadDir) after the post-script defer so it runs after the post-script completes (LIFO ordering). The cleanup should be conditional on !keepSandbox to match the existing sandbox retention logic.

Medium

• [incomplete-scope] internal/cli/run.go:618 — The PR branch adds REPO_DIR to the post-script env by manually appending after the old append(os.Environ(), envToList(h.RunnerEnv)...) pattern. On main (line 667), the post-script already uses childScriptEnv() which wraps environment construction and adds TRACEPARENT. The PR will need adaptation during rebase to add REPO_DIR through or after childScriptEnv rather than reconstructing the env from scratch.
  Remediation: After rebasing, append REPO_DIR to the result of childScriptEnv(), or add REPO_DIR to RunnerEnv so it flows through automatically.

• [scope-creep] .codecov.yml:27 — The codecov.yml change adds internal/cli/run.go to the ignore list with a reference to issue refactor: break up runAgent for testability #2831, but this PR claims to close issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. The PR body makes no mention of issue refactor: break up runAgent for testability #2831. Adding a file to codecov ignore is a policy decision that appears unrelated to the race-condition fix described in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075.
  Remediation: Remove the codecov.yml change from this PR, or update the PR description to explain why issue refactor: break up runAgent for testability #2831 is relevant. If refactor: break up runAgent for testability #2831 is unrelated, file it as a separate PR.

Previous run (8)

Review

Findings

High

• [resource-leak] internal/cli/run.go:601 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(hostRepositoryDownloadDir) or any other cleanup mechanism. Each run of runAgent leaves a full copy of the extracted repository in the system temp directory. These temp directories use unique sandbox names (agent-<name>-<pid>-<timestamp>) and accumulate without bound.
  Remediation: Register a deferred os.RemoveAll(hostRepositoryDownloadDir) after the post-script defer so it runs after the post-script completes (LIFO ordering), or add cleanup at the end of the function.

Low

• [edge-case] internal/cli/run.go:601 — If the function returns early (before extraction at step 9d), the post-script will receive REPO_DIR pointing to a directory that does not exist. The post-script (post-code.sh) does check for directory existence and exits with an error, so this is handled — but the error message will be confusing. This is not a regression from the prior behavior.

• [test-adequacy] internal/cli/run.go — No tests are added or modified to cover the new extraction path. The change alters the destination of SafeDownload, the environment variables passed to post-scripts and validation loops, and the results output.

• [env-var-naming] internal/cli/run.go:621 — REPO_DIR does not follow the agent-specific naming convention from ADR 0049. However, REPO_DIR is an existing contract defined by post-code.sh (line 49: REPO_DIR="${REPO_DIR:-repo}"), so this is consistent with the established interface.

---

Labels: Bug fix in the agent runner's extraction path (internal/cli/run.go) addressing race condition #2075, submitted by an external contributor.

Previous run (9)

Review

Findings

Critical

• [logic-error] internal/cli/run.go — The PR replaces hostRepositoryDir with downloadDir (a temp directory) for extraction but never copies or renames the extracted repo back to hostRepositoryDir. The original target repo on the host is never updated with the agent's changes. For local fullsend run users, this silently discards all agent modifications — the user's working directory is left with the pre-agent state. Issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 recommended the write-to-temp-then-os.Rename pattern (as used in internal/fetch/cache.go atomicWrite), but this PR only implements the first half (write to temp) and omits the atomic swap.
  Remediation: After SafeDownload into downloadDir succeeds, atomically rename it into place: os.Rename(downloadDir, hostRepositoryDir). This is the pattern already used in internal/fetch/cache.go (line 345) and recommended in the issue.

High

• [resource-leak] internal/cli/run.go — downloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(downloadDir) or any other cleanup. Each run leaves a full copy of the extracted repository in the system temp directory. If the atomic rename fix is adopted, the temp dir would be consumed by the rename and cleanup becomes automatic.
  Remediation: Add a defer os.RemoveAll(downloadDir) after the final use of the directory, or clean it up as part of the existing sandbox cleanup defer.

Medium

• [contract-violation] internal/cli/run.go:620 — Post-script receives REPO_DIR pointing to downloadDir (temp directory) instead of the user's target repo directory. Post-scripts (post-code.sh, post-fix.sh) cd into REPO_DIR and perform git operations. While the temp directory will contain a valid git repo, this is an undocumented behavioral change that should be explicitly tracked.

• [contract-violation] internal/cli/run.go:1034 — Validation loop receives TARGET_REPO_DIR pointing to downloadDir instead of hostRepositoryDir. The extracted repo at downloadDir is valid, but the path change is undocumented and could break scripts that make assumptions about the directory location.

• [test-adequacy] internal/cli/run.go — This PR changes the extraction target directory and modifies environment variables for post-scripts and validation. No tests are added or modified. At minimum, tests verifying that downloadDir is correctly propagated via REPO_DIR and TARGET_REPO_DIR should be included.

• [filesystem-assumption] internal/cli/run.go:601 — Using os.TempDir() for downloadDir may place it on a different filesystem than hostRepositoryDir, which would prevent atomic os.Rename() if the swap is later added. internal/fetch/cache.go's atomicWrite creates temp files in the same directory as the target (via os.CreateTemp(dir, ...)) specifically to enable atomic rename.
  Remediation: Use filepath.Join(filepath.Dir(hostRepositoryDir), ...) to create a temp sibling directory on the same filesystem.

Low

• [environment-variable-conflict] internal/cli/run.go:620 — Appending REPO_DIR=<downloadDir> after os.Environ() and envToList(h.RunnerEnv) creates a duplicate entry when REPO_DIR is already set. While Linux honors the last value, the same append pattern is used elsewhere in this file for TARGET_REPO_DIR and FULLSEND_RUN_DIR, so this is consistent.

• [naming-consistency] internal/cli/run.go:601 — Variable name downloadDir breaks from the established naming pattern (hostRepositoryDir, runDir, iterDir, remoteRepositoryDir). Consider tempRepositoryDir or extractedRepoDir.

Previous run (10)

Review

Findings

High

• [resource-leak] internal/cli/run.go:601 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(hostRepositoryDownloadDir) or any other cleanup mechanism. Each run of runAgent leaves a full copy of the extracted repository in the system temp directory. These temp directories use unique sandbox names (agent-<name>-<pid>-<timestamp>) and accumulate without bound. The sandbox cleanup defer only deletes the sandbox container via sandbox.Delete(), not host-side directories.
  Remediation: Register a deferred os.RemoveAll(hostRepositoryDownloadDir) after the post-script defer so it runs after the post-script completes (LIFO ordering). The cleanup should be conditional on !keepSandbox to match the existing sandbox retention logic.

Medium

• [incomplete-scope] internal/cli/run.go:618 — The PR branch adds REPO_DIR to the post-script env by manually appending after the old append(os.Environ(), envToList(h.RunnerEnv)...) pattern. On main (line 667), the post-script already uses childScriptEnv() which wraps environment construction and adds TRACEPARENT. The PR will need adaptation during rebase to add REPO_DIR through or after childScriptEnv rather than reconstructing the env from scratch.
  Remediation: After rebasing, append REPO_DIR to the result of childScriptEnv(), or add REPO_DIR to RunnerEnv so it flows through automatically.

• [scope-creep] .codecov.yml:27 — The codecov.yml change adds internal/cli/run.go to the ignore list with a reference to issue refactor: break up runAgent for testability #2831, but this PR claims to close issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075. The PR body makes no mention of issue refactor: break up runAgent for testability #2831. Adding a file to codecov ignore is a policy decision that appears unrelated to the race-condition fix described in fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075.
  Remediation: Remove the codecov.yml change from this PR, or update the PR description to explain why issue refactor: break up runAgent for testability #2831 is relevant. If refactor: break up runAgent for testability #2831 is unrelated, file it as a separate PR.

Previous run (11)

Review

Findings

High

• [resource-leak] internal/cli/run.go:601 — hostRepositoryDownloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(hostRepositoryDownloadDir) or any other cleanup mechanism. Each run of runAgent leaves a full copy of the extracted repository in the system temp directory. These temp directories use unique sandbox names (agent-<name>-<pid>-<timestamp>) and accumulate without bound.
  Remediation: Register a deferred os.RemoveAll(hostRepositoryDownloadDir) after the post-script defer so it runs after the post-script completes (LIFO ordering), or add cleanup at the end of the function.

Low

• [edge-case] internal/cli/run.go:601 — If the function returns early (before extraction at step 9d), the post-script will receive REPO_DIR pointing to a directory that does not exist. The post-script (post-code.sh) does check for directory existence and exits with an error, so this is handled — but the error message will be confusing. This is not a regression from the prior behavior.

• [test-adequacy] internal/cli/run.go — No tests are added or modified to cover the new extraction path. The change alters the destination of SafeDownload, the environment variables passed to post-scripts and validation loops, and the results output.

• [env-var-naming] internal/cli/run.go:621 — REPO_DIR does not follow the agent-specific naming convention from ADR 0049. However, REPO_DIR is an existing contract defined by post-code.sh (line 49: REPO_DIR="${REPO_DIR:-repo}"), so this is consistent with the established interface.

---

Labels: Bug fix in the agent runner's extraction path (internal/cli/run.go) addressing race condition #2075, submitted by an external contributor.

Previous run (12)

Review

Findings

Critical

• [logic-error] internal/cli/run.go — The PR replaces hostRepositoryDir with downloadDir (a temp directory) for extraction but never copies or renames the extracted repo back to hostRepositoryDir. The original target repo on the host is never updated with the agent's changes. For local fullsend run users, this silently discards all agent modifications — the user's working directory is left with the pre-agent state. Issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 recommended the write-to-temp-then-os.Rename pattern (as used in internal/fetch/cache.go atomicWrite), but this PR only implements the first half (write to temp) and omits the atomic swap.
  Remediation: After SafeDownload into downloadDir succeeds, atomically rename it into place: os.Rename(downloadDir, hostRepositoryDir). This is the pattern already used in internal/fetch/cache.go (line 345) and recommended in the issue.

High

• [resource-leak] internal/cli/run.go — downloadDir (filepath.Join(os.TempDir(), sandboxName)) is never cleaned up. There is no defer os.RemoveAll(downloadDir) or any other cleanup. Each run leaves a full copy of the extracted repository in the system temp directory. If the atomic rename fix is adopted, the temp dir would be consumed by the rename and cleanup becomes automatic.
  Remediation: Add a defer os.RemoveAll(downloadDir) after the final use of the directory, or clean it up as part of the existing sandbox cleanup defer.

Medium

• [contract-violation] internal/cli/run.go:620 — Post-script receives REPO_DIR pointing to downloadDir (temp directory) instead of the user's target repo directory. Post-scripts (post-code.sh, post-fix.sh) cd into REPO_DIR and perform git operations. While the temp directory will contain a valid git repo, this is an undocumented behavioral change that should be explicitly tracked.

• [contract-violation] internal/cli/run.go:1034 — Validation loop receives TARGET_REPO_DIR pointing to downloadDir instead of hostRepositoryDir. The extracted repo at downloadDir is valid, but the path change is undocumented and could break scripts that make assumptions about the directory location.

• [test-adequacy] internal/cli/run.go — This PR changes the extraction target directory and modifies environment variables for post-scripts and validation. No tests are added or modified. At minimum, tests verifying that downloadDir is correctly propagated via REPO_DIR and TARGET_REPO_DIR should be included.

• [filesystem-assumption] internal/cli/run.go:601 — Using os.TempDir() for downloadDir may place it on a different filesystem than hostRepositoryDir, which would prevent atomic os.Rename() if the swap is later added. internal/fetch/cache.go's atomicWrite creates temp files in the same directory as the target (via os.CreateTemp(dir, ...)) specifically to enable atomic rename.
  Remediation: Use filepath.Join(filepath.Dir(hostRepositoryDir), ...) to create a temp sibling directory on the same filesystem.

Low

• [environment-variable-conflict] internal/cli/run.go:620 — Appending REPO_DIR=<downloadDir> after os.Environ() and envToList(h.RunnerEnv) creates a duplicate entry when REPO_DIR is already set. While Linux honors the last value, the same append pattern is used elsewhere in this file for TARGET_REPO_DIR and FULLSEND_RUN_DIR, so this is consistent.

• [naming-consistency] internal/cli/run.go:601 — Variable name downloadDir breaks from the established naming pattern (hostRepositoryDir, runDir, iterDir, remoteRepositoryDir). Consider tempRepositoryDir or extractedRepoDir.

[rh-hemartin]

Won't address the critical given by the review agent, we won't replace the directory, what if something bad happens and all is wiped out?

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:30 AM UTC · Completed 8:41 AM UTC
Commit: c22aaba · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:44 AM UTC · Completed 7:58 AM UTC
Commit: e55c6e3 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:36 AM UTC · Completed 8:48 AM UTC
Commit: 760c642 · View workflow run →

[ben-alkov]

LGTM, but why are the changes to ".codecov.yml" here?

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:10 AM UTC · Completed 7:26 AM UTC
Commit: de48094 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:42 AM UTC · Completed 7:59 AM UTC
Commit: d539dda · View workflow run →

[ben-alkov]

LGTM

[waynesun09]

CI green, previously-blocking merge conflict resolved by rebase, and the flagged findings (results-output lifetime, REPO_DIR override documentation) have been fixed. Approving.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 10:03 PM UTC · Completed 10:10 PM UTC
Commit: d539dda · View workflow run →

[fullsend-ai-retro]

PR #2774 was a well-scoped bug fix (3 files, +33/-69 lines) that took 16 days to merge, with 9 review agent runs producing 43 review comments. The review agent caught genuinely valuable findings (resource leak on temp dir, defer LIFO ordering bug) but also re-raised the same codecov scope-creep finding 4+ times after the author explicitly dismissed it — reinforcing the pattern documented in agents#106. Human reviewers caught two issues the review agent missed: the stale branch/conflicting fix on main (waynesun09, day 13) and a results-output-lifetime bug where a printed download path would be deleted before the user could use it (waynesun09, day 15). The repeated findings and high review-run count represent significant token waste, with agents#108 and fullsend#5139 already tracking related optimizations.

Proposals filed

• Review agent should detect ephemeral-path-in-user-output as a correctness finding (in fullsend-ai/agents)
• Review agent should detect significant base-branch divergence early and recommend rebase (in fullsend-ai/agents)

Evidence notes (not filed as issues)

• Additional evidence for agents#106: PR fix(runner): extract sandbox output to temp dir instead of overwriting target repo #2774 scope-creep finding re-raised 4+ times after explicit author dismissal (fullsend-ai/agents): On PR #2774, the review agent raised a medium scope-creep finding about .codecov.yml being unrelated to the bug fix. This finding appeared in review runs on 6/30, 7/1, 7/14 (twice), and 7/16 (twice) — at least 6 times across 9 review runs. The author responded with terse dismissals ('no', 'No.') on 7/16 after having already explained the rationale on 7/10. The prior review context mechanism was active (PRIOR_REVIEW_FILE was populated in all runs after the first, confirmed via workflow logs showing PRIOR_REVIEW_SHA), but the agent continued re-raising the identical finding verbatim.