feat(cli): download from sandbox into different directory

[rh-hemartin]

fullsend run was wiping out --target-repo and downloading the sandbox target repo into it. On the workflows, this was OK, but locally this meant wiping out the target directory. If someone used a valuable target repository that would mean that they could lose local information (in case the agent decided to wipe out branches within the sandbox, or something similar). Because of that this PR introduces hostDownloadDir to download the sandbox contents and prevent wiping out the host --target-repo. This is not configurable, so overriding the --target-repo is not possible, users that want their contents modified will need to manually copy changes into their --target-repo.

Closes #2075

[github-actions]

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

[github-actions]

Site preview

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

Commit: 368a4ec39eff70fecd7ec9683d23352b4088e46c

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:59 AM UTC · Completed 8:08 AM UTC
Commit: c0243a6 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [logic-error] internal/cli/run.go:563 — REPO_DIR and TARGET_REPO_DIR are appended to the post-script env after envToList(h.RunnerEnv), so any user-specified REPO_DIR in runner_env is silently overridden. The customizing-agents.md example harness still shows REPO_DIR in runner_env, which is now dead configuration. The override is intentional by design, but the silent replacement with no warning may confuse users who follow the existing documentation.
  Remediation: Either (a) emit a log warning when REPO_DIR already exists in h.RunnerEnv, or (b) do not override if already present, or (c) update documentation examples to remove the REPO_DIR runner_env entry and document that REPO_DIR is now set by the CLI.

• [resource-leak] internal/cli/run.go:527 — hostDownloadDir (os.TempDir()/fullsend-downloads/<sandboxName>) is never cleaned up after the function completes. Each fullsend run invocation creates a new directory (sandboxName includes PID and timestamp). Previous iterations are cleaned within the loop, but the parent directory and the final iteration's download persist indefinitely. For the local-use scenario this PR targets, repeated runs will accumulate repository-sized directories under /tmp/fullsend-downloads/.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) after the post-script defer (so LIFO ordering ensures the post-script runs first), or document that cleanup is the user's responsibility.

• [user-contract-change] internal/cli/run.go:527 — This PR changes the --target-repo contract from input+output (modified in place) to input-only, with output going to a separate temp directory. The PR body acknowledges this. The doc changes in this PR update REPO_DIR references and add clarifying notes, but the user-facing workflow guidance in running-agents-locally.md could more explicitly explain the new manual-copy requirement and where to find the downloaded output.
  Remediation: Add a note in running-agents-locally.md explaining that --target-repo is no longer modified in place, documenting the output location and how to copy changes back if desired.

Low

• [error-handling-pattern] internal/cli/run.go:957 — os.RemoveAll(lastDownloadDir) ignores errors silently when cleaning the previous iteration's download. Best-effort cleanup is reasonable here (unlike the pre-PR extraction-destination cleanup which was correctness-critical), but a warning log would be consistent with nearby printer.StepWarn usage.

• [directory-permissions] internal/cli/run.go:955 — iterDownloadDir is created with 0o755 (world-readable). This is consistent with existing directory permissions in the codebase (runDir, findingsDir), but 0o700 would be more restrictive for a temp directory containing extracted source code.

• [stale-environment-variable-reference] docs/guides/user/building-custom-agents.md:56 — TARGET_REPO_DIR is described as "path to target repository checkout" without noting that for post-scripts and validation scripts, it now points to a per-iteration download directory rather than the original --target-repo path.

Previous run

Review

Findings

High

• [scope-divergence] internal/cli/run.go:527 — Issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 explicitly suggested the atomic write-to-temp-then-os.Rename pattern (already used in internal/fetch/). This PR instead downloads to a completely separate directory tree (os.TempDir()/fullsend-downloads/) and leaves --target-repo unchanged, fundamentally changing the user contract from "run agent and update target-repo" to "run agent and leave results in a temp directory." The PR body acknowledges this: "users that want their contents modified will need to manually copy changes into their --target-repo." This is a valid design choice but diverges from the issue's authorized approach — the behavioral change should be explicitly discussed and approved.
  Remediation: Align implementation with issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075's suggested approach: download to a sibling temp directory then atomically os.Rename() it to hostRepositoryDir. Alternatively, amend the issue to explicitly authorize this alternative design and document the new contract.

• [architectural-coherence] internal/cli/run.go:560 — Post-script now receives both REPO_DIR and TARGET_REPO_DIR pointing to lastDownloadDir (a per-iteration temp directory). This overrides any REPO_DIR set in the harness runner_env (e.g., ${GITHUB_WORKSPACE}/target-repo). The actual --target-repo location is no longer accessible to post-scripts. Post-scripts (post-code.sh, post-fix.sh) cd into REPO_DIR and perform git operations (clone setup, push) — verify that SafeDownload preserves .git/config with remotes so git push works from the temp download directory.
  Remediation: Pass both the download dir and the original --target-repo path as separate environment variables so post-scripts can choose, or verify that SafeDownload preserves git remote configuration and document this guarantee.

Medium

• [data-exposure] internal/cli/run.go:527 — hostDownloadDir (os.TempDir()/fullsend-downloads/<sandboxName>) is never cleaned up after the function completes. On shared CI runners, this leaves the final iteration's extracted repository contents — which may include agent-generated secrets or sensitive code — on disk indefinitely under a world-readable (0o755) temp directory. The docs acknowledge this ("persists until OS clears /tmp"), but explicit cleanup would be safer.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) (respecting keepSandbox) after the hostDownloadDir assignment, or document that cleanup is the caller's responsibility.

• [stale-environment-variable-default] docs/plans/adr-0045-forge-portable-harness-phase2.md:129 — The plan document states REPO_DIR should be set to ${GITHUB_WORKSPACE}/target-repo in forge.github.runner_env, but this PR changes the effective REPO_DIR to point to a host temporary download path and changes the post-script default from "repo" to ".". The plan no longer reflects the new implementation.
  Remediation: Update the plan document to reflect that REPO_DIR is now explicitly set by the runner to the host download directory.

Low

• [architectural-coherence] internal/cli/run.go:527 — hostDownloadDir is placed outside outputBase to avoid including source code in build artifacts. This introduces a new architectural constraint (artifact-content separation) documented only in an inline code comment. The split means some run outputs live in runDir and the repo lives elsewhere, which may complicate troubleshooting.

• [edge-case] internal/cli/run.go:560 — No guard in the post-script defer for lastDownloadDir being empty. Existing flow guards (runErr != nil, !validationPassed) prevent most early-exit scenarios from reaching the post-script, but an explicit guard would be more defensive.

• [variable-declaration-placement] internal/cli/run.go:527 — hostDownloadDir and lastDownloadDir are declared at function level. This is necessary because they are captured by the post-script defer closure, but the relationship could be clearer with a comment linking the declaration to the defer.

• [directory-permissions] internal/cli/run.go:955 — iterDownloadDir is created with 0o755 (world-readable). On shared CI runners, 0o700 would limit access. Consistent with existing directory permissions in the codebase but worth tightening for temp directories containing extracted source.

• [error-handling] internal/cli/run.go:955 — os.RemoveAll(lastDownloadDir) call ignores errors silently. Best-effort cleanup is reasonable, but a warning log would be consistent with the codebase pattern (see nearby printer.StepWarn usage).

• [env-var-injection-pattern] internal/cli/run.go:560 — REPO_DIR and TARGET_REPO_DIR are added using a second append statement. The established pattern (see validation command at lines 968–973) uses a single nested append chain.

• [documentation-divergence] docs/guides/dev/cli-internals.md — The extraction flow documentation still describes updating --target-repo but the PR changes this behavior. The PR updates the post-script box in the diagram but does not update the extraction section.

Previous run (2)

Review

Findings

Medium

• [scope-divergence] internal/cli/run.go — Issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 explicitly suggested the atomic write-to-temp-then-os.Rename pattern (already used in internal/fetch/). This PR instead downloads to a completely separate directory and leaves --target-repo unchanged, changing the user contract from "run agent and update target-repo" to "run agent and leave results in a temp directory." The PR body acknowledges this ("users that want their contents modified will need to manually copy changes"). This is a valid design choice but diverges from the issue's authorized approach — consider documenting the rationale or filing a follow-up issue proposing the behavioral change.

• [architectural-coherence] internal/cli/run.go:560 — Post-script now receives both REPO_DIR and TARGET_REPO_DIR pointing to lastDownloadDir (a per-iteration temp directory). Previously, post-scripts used REPO_DIR from runner_env and TARGET_REPO_DIR pointed to hostRepositoryDir. The actual --target-repo location is no longer accessible to post-scripts. Post-scripts (post-code.sh, post-fix.sh) cd into REPO_DIR and perform git operations — verify that git push works from the temp download directory (SafeDownload may not preserve remotes).
  Remediation: Pass both the download dir and the original --target-repo path as separate variables so post-scripts can choose, or verify that SafeDownload preserves git remote configuration.

• [data-exposure] internal/cli/run.go:527 — hostDownloadDir is created under os.TempDir()/fullsend-downloads/<sandboxName> but is never cleaned up. On shared CI runners, this leaves extracted repository contents (which may contain agent-added secrets) on disk indefinitely. The docs acknowledge this ("persists until OS clears /tmp"), but explicit cleanup would be safer.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) after the post-script defer (so LIFO ordering ensures post-script runs first), or document that the caller is responsible for cleanup.

• [scope-alignment] internal/cli/run.go:950 — Per-iteration download directories (iteration-N) accumulate under hostDownloadDir but only the last is referenced via lastDownloadDir. For validation loops with max_iterations: 10, this creates 10 full copies of the target repo in /tmp with no cleanup. The old behavior (extract-in-place to hostRepositoryDir) avoided this by overwriting the same location.
  Remediation: Clean up prior iteration download directories when starting a new iteration, or document why per-iteration snapshots are retained.

• [architectural-coherence] internal/cli/run.go:527 — hostDownloadDir is placed outside outputBase to avoid including source code in build artifacts. This introduces a new architectural constraint (artifact-content separation) with no design document coverage. The split means some run outputs live in runDir and the repo lives elsewhere, complicating troubleshooting and breaking the expectation that runDir is the complete record of a run.
  Remediation: Document the artifact-separation constraint in a design document or ADR.

Low

• [contract-violation] internal/scaffold/fullsend-repo/scripts/post-code.sh:51 — Fallback default changes from REPO_DIR:-repo to REPO_DIR:-.. The runner now always injects REPO_DIR explicitly, but manual invocations outside the runner will silently change behavior (operating in cwd instead of a repo subdirectory).

• [edge-case] internal/cli/run.go:560 — No guard in the post-script defer for lastDownloadDir being empty. The current code flow ensures lastDownloadDir is set before the defer runs, but adding a guard (if lastDownloadDir == "" { return }) would be defensive against future code changes.

• [directory-permissions] internal/cli/run.go:955 — iterDownloadDir is created with 0o755 (world-readable). On shared CI runners, 0o700 would limit access to the extracted repo contents. Consistent with existing runDir permissions but worth tightening for the new temp directory.

• [TOCTOU] internal/cli/run.go:527 — hostDownloadDir uses a predictable path under os.TempDir(). Risk is low because sandboxName includes PID and timestamp, making path prediction impractical.

• [error-handling] internal/cli/run.go:1020 — scanOutputFiles findings in hostDownloadDir are not included in the verified audit chain (only runDir findings are chain-verified).

• [error-message-consistency] internal/cli/run.go:956 — Error message uses "creating download dir" — both "dir" and "directory" forms appear in the codebase, but nearby code prefers the full word.

• [path-traversal] internal/cli/run.go:560 — agentName is embedded in path construction without explicit validation. Risk is mitigated by filepath.Join normalization and the fact that agentName must match a harness config key.

• [logging-consistency] internal/cli/run.go:1053 — printer.KeyValue("Extracted repo", ...) label differs from the "X directory" pattern used by "Run directory". Consider "Download directory" for consistency.

• [comment-style] internal/cli/run.go:527 — lastDownloadDir lacks an explanatory comment despite being captured by the post-script defer closure. Its purpose is clear from usage but a brief comment would match the style of nearby validationPassed.

• [error-handling-consistency] internal/cli/run.go:1023 — The "(download dir)" suffix on the scan error message is a useful disambiguator but creates a slightly different format from the existing scan error message.

---

Labels: PR modifies the CLI runner's sandbox extraction and post-script contract.

Previous run (3)

Review

Findings

Medium

• [contract-violation] internal/cli/run.go:911 — TARGET_REPO_DIR passed to the validation script now points to iterDownloadDir (a per-iteration temp directory) instead of hostRepositoryDir (the user's --target-repo path). Custom validation scripts that rely on TARGET_REPO_DIR pointing to a stable, user-controlled path will see a different directory. This is a semantic change to the validation script contract.
  Remediation: Document this contract change. Consider passing both TARGET_REPO_DIR and a new HOST_REPO_DIR.

• [stale-identifier-reference] docs/guides/user/building-custom-agents.md:56 — Documentation describes TARGET_REPO_DIR as "path to target repository checkout". After this PR, TARGET_REPO_DIR for validation scripts points to a per-iteration download directory, not the original checkout. The description may still be accurate in spirit (it is where the repo was extracted) but should be reviewed to ensure users aren't confused.
  Remediation: Review whether the TARGET_REPO_DIR description accurately reflects post-PR semantics.

Low

• [resource-leak] internal/cli/run.go:493 — hostDownloadDir is created under os.TempDir()/fullsend-downloads/<sandboxName> but is never cleaned up. Each invocation leaves a copy of the downloaded repo on disk until the OS clears /tmp. The directory lives under os.TempDir() which is ephemeral and cleaned by the OS, and sandbox names are unique per run, so this is a nice-to-have cleanup rather than a critical leak.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) after the post-script defer, or add a --cleanup-downloads flag.

• [comment-accuracy] internal/cli/run.go:891 — The comment "Extract target repo back to host" no longer accurately describes the behavior — extraction goes to iterDownloadDir, not back to the original target repo location.
  Remediation: Update the comment to reflect the new behavior.

• [env-variable-naming] internal/cli/run.go:527 — The post-script receives REPO_DIR while the validation script uses TARGET_REPO_DIR. Both point to a download directory containing the extracted repo. The naming inconsistency may confuse script authors.
  Remediation: Consider aligning naming or documenting the distinction.

Info

• [scope-alignment] internal/cli/run.go:496 — The PR diverges from issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075's suggested atomic-swap approach, instead downloading to a separate directory tree. This is a valid design choice that avoids the race entirely by never touching the original directory.

• [forward-plan-discrepancy] docs/plans/adr-0045-forge-portable-harness-phase2.md:129 — Phase 2 plan document specifies REPO_DIR should be ${GITHUB_WORKSPACE}/target-repo but the current PR changes REPO_DIR default and introduces separate download directory handling.

• [output-clarity] internal/cli/run.go:969 — The new "Download directory" output label may not clearly convey the distinction from "Run directory" to users.

Previous run (4)

Review

Findings

High

• [resource-leak] internal/cli/run.go:493 — hostDownloadDir is created under os.TempDir()/fullsend-downloads/<sandboxName> but is never cleaned up. There is no defer os.RemoveAll(hostDownloadDir) and no other cleanup path. Each invocation leaves a full copy of the target repo on disk. The baseline code did not have this problem because it wrote back to hostRepositoryDir (the user's own directory). This is a genuine new resource leak introduced by the PR.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) after the hostDownloadDir assignment, or document intentional retention and add cleanup in a wrapper script.

Medium

• [logic-error] internal/cli/run.go:945 — scanOutputFiles only scans runDir, which does not include hostDownloadDir (the new location for extracted source). If the agent embeds secrets in source files, those secrets persist on disk unredacted. Note: this is a pre-existing gap (the previous hostRepositoryDir was also outside runDir), not a regression introduced by this PR, but this change makes the gap more operationally significant since the unscanned directory is now a temp location the user may not notice.

• [scope-exceeded] internal/cli/run.go:496 — The PR claims to close fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075, which requested atomic directory swapping to eliminate the race condition window. The PR instead downloads to a completely separate directory tree and never updates hostRepositoryDir, changing the behavioral contract from "run agent and update target-repo in-place" to "run agent and leave results in a separate download directory." The PR body acknowledges this: "users that want their contents modified will need to manually copy changes into their --target-repo." This is a valid approach but diverges from the issue's suggested atomic-swap pattern — the PR description should clarify this is an intentional design choice.

• [contract-violation] internal/cli/run.go:914 — TARGET_REPO_DIR now points to iterDownloadDir instead of hostRepositoryDir. Validation scripts that expect TARGET_REPO_DIR to be the user's --target-repo path will receive a different (per-iteration temporary) directory. This changes the contract for existing validation and post-scripts.
  Remediation: Document the changed semantics of TARGET_REPO_DIR or provide both the original and new paths as separate env vars.

Low

• [design-misalignment] internal/cli/run.go:496 — The comment explains hostDownloadDir is outside outputBase to avoid uploading source code as a GitHub Actions artifact. This couples CLI behavior rationale to the CI/CD workflow layout.

• [comment-accuracy] internal/cli/run.go:885 — The comment "Extract target repo back to host" no longer accurately describes the behavior — extraction goes to iterDownloadDir, not back to the original target repo location.

Info

• [output-clarity] internal/cli/run.go:965 — The new "Download directory" output label may not clearly convey the distinction from "Run directory" to users.

Previous run (5)

Review

Findings

High

• [resource-leak] internal/cli/run.go:493 — hostDownloadDir is created under os.TempDir()/fullsend-downloads/<sandboxName> but is never cleaned up. There is no defer os.RemoveAll(hostDownloadDir) and no other cleanup path. Each invocation leaves a full copy of the target repo on disk. The baseline code did not have this problem because it wrote back to hostRepositoryDir (the user's own directory). This is a genuine new resource leak introduced by the PR.
  Remediation: Add defer os.RemoveAll(hostDownloadDir) after the hostDownloadDir assignment, or document intentional retention and add cleanup in a wrapper script.

Medium

• [logic-error] internal/cli/run.go:945 — scanOutputFiles only scans runDir, which does not include hostDownloadDir (the new location for extracted source). If the agent embeds secrets in source files, those secrets persist on disk unredacted. Note: this is a pre-existing gap (the previous hostRepositoryDir was also outside runDir), not a regression introduced by this PR, but this change makes the gap more operationally significant since the unscanned directory is now a temp location the user may not notice.

• [scope-exceeded] internal/cli/run.go:496 — The PR claims to close fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075, which requested atomic directory swapping to eliminate the race condition window. The PR instead downloads to a completely separate directory tree and never updates hostRepositoryDir, changing the behavioral contract from "run agent and update target-repo in-place" to "run agent and leave results in a separate download directory." The PR body acknowledges this: "users that want their contents modified will need to manually copy changes into their --target-repo." This is a valid approach but diverges from the issue's suggested atomic-swap pattern — the PR description should clarify this is an intentional design choice.

• [contract-violation] internal/cli/run.go:914 — TARGET_REPO_DIR now points to iterDownloadDir instead of hostRepositoryDir. Validation scripts that expect TARGET_REPO_DIR to be the user's --target-repo path will receive a different (per-iteration temporary) directory. This changes the contract for existing validation and post-scripts.
  Remediation: Document the changed semantics of TARGET_REPO_DIR or provide both the original and new paths as separate env vars.

Low

• [design-misalignment] internal/cli/run.go:496 — The comment explains hostDownloadDir is outside outputBase to avoid uploading source code as a GitHub Actions artifact. This couples CLI behavior rationale to the CI/CD workflow layout.

• [comment-accuracy] internal/cli/run.go:885 — The comment "Extract target repo back to host" no longer accurately describes the behavior — extraction goes to iterDownloadDir, not back to the original target repo location.

Info

• [output-clarity] internal/cli/run.go:965 — The new "Download directory" output label may not clearly convey the distinction from "Run directory" to users.

Previous run (6)

Review

Findings

High

• [logic-error] internal/cli/run.go:848 — The post-agent security scan (scanOutputFiles) walks runDir to redact secrets from extracted output. With this change, the downloaded repo content now lives under hostDownloadDir (os.TempDir()/fullsend-downloads/...), completely outside runDir. The security pipeline therefore no longer scans any of the extracted source-code files. If the agent embeds secrets in source files, those secrets will persist on disk unredacted.
  Remediation: Either (a) run scanOutputFiles against hostDownloadDir as well, or (b) change the download location to be a subdirectory of runDir.

• [resource-leak] internal/cli/run.go:448 — hostDownloadDir is created under os.TempDir()/fullsend-downloads/<sandboxName> but is never cleaned up. There is no defer os.RemoveAll(hostDownloadDir) and no other cleanup path. Each invocation of fullsend run leaves a full copy of the target repo (per iteration) on disk indefinitely. For local use — the exact scenario this PR targets — repeated runs will accumulate potentially large directory trees in the system temp directory.
  Remediation: Add a defer os.RemoveAll(hostDownloadDir) after the directory is first referenced, or document that cleanup is the user's responsibility.

• [scope-exceeded] internal/cli/run.go:793 — Issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 requested atomic directory swapping (write-to-temp-then-os.Rename) to prevent race conditions with concurrent IDE/git access. The PR instead downloads to a completely separate directory and never updates hostRepositoryDir, fundamentally changing the contract of fullsend run from "run agent and update target-repo with results" to "run agent and leave results in a separate download directory." The PR body states "users that want their contents modified will need to manually copy changes into their --target-repo" — this is a breaking behavioral change the issue did not authorize.
  Remediation: Follow the issue's suggested approach: download to a temporary directory, then use os.Rename() for atomic swap into hostRepositoryDir. The fetch cache implementation at internal/fetch/cache.go provides the exact pattern.

Medium

• [design-direction] internal/cli/run.go:817 — The validation script now receives TARGET_REPO_DIR pointing to iterDownloadDir (/tmp/fullsend-downloads/.../iteration-N) instead of hostRepositoryDir. This changes the contract where TARGET_REPO_DIR should point to the --target-repo path. Validation scripts may rely on this path being the actual repository directory.
  Remediation: If atomic swap is implemented, TARGET_REPO_DIR should continue pointing to hostRepositoryDir. If the download directory is kept separate, introduce a new env var and document it.

Low

• [misplaced-abstraction] internal/cli/run.go:446 — The comment states hostDownloadDir is outside outputBase to avoid uploading source code as an artifact. This couples the CLI to GitHub Actions artifact upload behavior rather than keeping platform-agnostic behavior.

• [error-handling-idiom] internal/cli/run.go:791 — The comment at lines 791–792 still describes "Extract target repo back to host" but the behavior has changed — it no longer extracts back to the original target-repo location but to a new temp download location per iteration.

• [edge-case] internal/cli/run.go:795 — iterDownloadDir is passed to sandbox.SafeDownload without ensuring parent directories exist via os.MkdirAll. This likely works because openshell sandbox download creates the destination, but the implicit dependency is undocumented.

Info

• [naming-convention] internal/cli/run.go:871 — The new "Download directory" output label may confuse users about the distinction between "Run directory" and "Download directory." Consider a more descriptive label.

[fullsend-ai-review]

See the review comment for full details.

[ralphbean]

One question inline about how this interacts with the post-scripts.

[ralphbean]

I might be misreading the flow — in code.yaml, REPO_DIR points to ${GITHUB_WORKSPACE}/target-repo. post-code.sh cds there and looks for the agent's feature branch to push. With SafeDownload now targeting the temp dir instead, wouldn't that path still have the original checkout? If so, the post-script would see no branch and exit with "nothing to do."

[ralphbean]

Btw — this is a good example of why we're pursuing functional tests (#1682). The unit tests and lint all pass here, but the integration between the download path, REPO_DIR, and the post-script is only exercised end-to-end. A functional test that asserts "after runAgent completes, the post-script's REPO_DIR contains the agent's feature branch" would catch this class of regression before review.

[rh-hemartin]

Btw — this is a good example of why we're pursuing functional tests (#1682). The unit tests and lint all pass here, but the integration between the download path, REPO_DIR, and the post-script is only exercised end-to-end. A functional test that asserts "after runAgent completes, the post-script's REPO_DIR contains the agent's feature branch" would catch this class of regression before review.

Ah, damn.. What about we pass the folder to the post-script?

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 8:04 AM UTC · Ended 8:18 AM UTC
Commit: 4e21a60 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/run.go	0.00%	18 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:04 AM UTC · Completed 8:18 AM UTC
Commit: f97d57e · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:32 PM UTC · Completed 1:44 PM UTC
Commit: 9956a8b · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:07 AM UTC · Ended 6:17 AM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:20 AM UTC · Completed 6:37 AM UTC
Commit: e629bd2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:06 AM UTC · Completed 7:20 AM UTC
Commit: d40eb41 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:39 AM UTC · Completed 8:57 AM UTC
Commit: 368a4ec · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] logic-error

REPO_DIR and TARGET_REPO_DIR are appended to the post-script env after envToList(h.RunnerEnv), so any user-specified REPO_DIR in runner_env is silently overridden. The customizing-agents.md example harness shows REPO_DIR in runner_env, which is now dead configuration.

Suggested fix: Either (a) emit a log warning when REPO_DIR already exists in h.RunnerEnv, or (b) do not override if already present, or (c) update documentation examples to remove the REPO_DIR runner_env entry.

[fullsend-ai-review]

[medium] resource-leak

hostDownloadDir (os.TempDir()/fullsend-downloads/) is never cleaned up after the function completes. Repeated local runs accumulate repository-sized directories under /tmp/fullsend-downloads/.

Suggested fix: Add defer os.RemoveAll(hostDownloadDir) after the post-script defer, or document that cleanup is the user's responsibility.

[fullsend-ai-review]

[medium] user-contract-change

This PR changes the --target-repo contract from input+output (modified in place) to input-only. The PR body acknowledges this but the user-facing workflow guidance could more explicitly explain the new manual-copy requirement.

Suggested fix: Update running-agents-locally.md to explain new behavior, output location, and how to copy changes back.

[fullsend-ai-review]

[low] error-handling-pattern

os.RemoveAll(lastDownloadDir) ignores errors silently when cleaning the previous iteration's download. A warning log would be consistent with nearby printer.StepWarn usage.

[fullsend-ai-review]

[low] directory-permissions

iterDownloadDir is created with 0o755 (world-readable). Consistent with existing codebase pattern but 0o700 would be more restrictive for temp directories containing source code.

[rh-hemartin]

Superseded by #2774

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:00 AM UTC · Completed 8:19 AM UTC
Commit: 368a4ec · View workflow run →

[fullsend-ai-retro]

Retro: PR #2154 — feat(cli): download from sandbox into different directory

Timeline

1. Jun 9 — Issue #2075 triaged and coded by agents. The code agent produced a PR that introduced a separate hostDownloadDir for sandbox downloads.
2. Jun 11 — PR opened. Review agent requested changes on first pass, correctly identifying: resource leak (hostDownloadDir never cleaned up), scope divergence from issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075's atomic-swap approach, and post-script contract breakage.
3. Jun 12 — Human reviewer (ralphbean) left a substantive inline comment about the post-script interaction — the same concern the review agent had flagged.
4. Jun 15 — Second review run, same findings repeated. Codecov reported 0% patch coverage on 18 lines.
5. Jun 23 — Third review run. Author responded to findings with "Won't fix" (4×), "Intended" (1×), "Done" (1×). Each reply triggered a separate pull_request_review dispatch — 8+ shim runs in 5 minutes (08:30–08:35), leading to additional review runs.
6. Jun 30 — PR closed without merge, superseded by #2774.

Total: 9 review dispatches (7 completed, 2 cancelled), 0 fix agent runs, 19 days open, ultimately abandoned.

What went well

• Review agent quality was high. Its three highest-severity findings (resource leak, scope divergence, post-script contract violation) were all legitimate. The human reviewer independently raised the same post-script concern, validating the agent's analysis.
• The review agent correctly predicted the PR was fundamentally misaligned with the issue requirements. The PR was ultimately abandoned in favor of a different approach (fix(runner): extract sandbox output to temp dir instead of overwriting target repo #2774).

Patterns observed (all covered by existing issues)

No new proposals are filed because every improvement opportunity maps to existing open issues:

Pattern	Existing Issues
Burst of review dispatches from rapid comment replies	#1125, #893, #1014
Same findings repeated across review rounds	#1013, #1583
Code agent diverged from issue's recommended approach	#2185
No escalation when author dismisses high-severity findings	#2111, #1672
Per-PR review budget to cap runaway dispatches	#2599

Autonomy assessment

The review agent's findings aligned with (and in some cases exceeded) human review quality on this PR. The human reviewer raised one concern; the review agent raised the same concern plus five additional valid findings. This PR is evidence that the review agent is providing high-quality architectural feedback on CLI/runner changes.