fix(runner): preserve target repo directory inode during extraction by ralphbean · Pull Request #3381 · fullsend-ai/fullsend

ralphbean · 2026-07-07T16:14:40Z

Summary

Replace os.RemoveAll(hostRepositoryDir) with clearDirContents() which removes directory contents but preserves the directory itself (and its inode)
Fixes the "getwd: no such file or directory" error when running fullsend run a second time from the same shell
Adds a StepInfo log line showing the full absolute path being cleared

Problem

When running fullsend run ... --target-repo . --fullsend-dir ., the extraction step (os.RemoveAll + SafeDownload) deletes and recreates the target repo directory. This destroys the directory inode, orphaning any shell whose CWD pointed at it. The next invocation from that shell fails immediately:

Error: resolving fullsend dir: getwd: no such file or directory

Fix

clearDirContents() iterates over directory entries and removes them individually, leaving the directory itself (and its inode) intact. Shells continue to work without needing cd ..

Relates-to: #2075

Test plan

TestClearDirContents verifies inode preservation and content removal
Manual: run fullsend run twice from the same shell with --target-repo . — second invocation should work without cd .

🤖 Generated with Claude Code

Replace os.RemoveAll(hostRepositoryDir) with clearDirContents(), which removes all entries inside the directory without removing the directory itself. This preserves the directory inode so that shells whose CWD points at the target repo continue to work after extraction. Previously, the delete-then-recreate cycle would orphan any shell sitting in the directory, causing subsequent commands to fail with "getwd: no such file or directory". Also adds a StepInfo log line showing the full absolute path being cleared, making it obvious what happened if something goes wrong. Relates-to: #2075 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

qodo-code-review · 2026-07-07T16:15:34Z

PR Summary by Qodo

fix(runner): preserve target repo directory inode during extraction

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Preserve target repo directory inode by clearing contents instead of removing the directory
• Prevent "getwd: no such file or directory" on repeated runs from the same shell CWD
• Add unit test covering inode preservation and full directory cleanup

Diagram

graph TD
  A["runAgent: extract"] --> B["clearDirContents()"] --> C[("Target repo dir")]
  C --> D["SafeDownload extract"]
  A --> E["StepInfo log"]

High-Level Assessment

This is the right approach for the stated failure mode: removing the directory breaks processes holding the directory inode (e.g., shells with CWD inside it). Clearing only the directory contents is the simplest, most portable fix and keeps semantics close to the original behavior while avoiding the inode invalidation. Alternatives like extracting to a new directory and swapping via rename would add complexity and can still surprise tooling via path changes.

Files changed (2) +48 / -1

Bug fix (1) +23 / -1

run.goClear target repo contents without deleting directory before extraction +23/-1

Clear target repo contents without deleting directory before extraction

• Replaces 'os.RemoveAll(hostRepositoryDir)' with a new 'clearDirContents()' helper that deletes all entries inside the directory while preserving the directory itself (and inode). Adds a StepInfo log line to make the cleared path explicit during the extraction step.

internal/cli/run.go

Tests (1) +25 / -0

run_test.goAdd unit test for inode-preserving directory clear helper +25/-0

Add unit test for inode-preserving directory clear helper

• Introduces 'TestClearDirContents' to assert that clearing removes all entries while the directory remains the same file (inode preserved via 'os.SameFile'). Covers nested directories and files to validate recursive removal.

internal/cli/run_test.go

fullsend-ai-review · 2026-07-07T16:15:40Z

🤖 Finished Review · ✅ Success · Started 4:15 PM UTC · Completed 4:24 PM UTC
Commit: 90a1a53 · View workflow run →

github-actions · 2026-07-07T16:16:49Z

Site preview

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

Commit: be97fb1821e5f53973eaf0e24d67557b07475418

codecov · 2026-07-07T16:19:56Z

Codecov Report

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

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

📢 Thoughts on this report? Let us know!

qodo-code-review · 2026-07-07T16:22:22Z

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): 54 rules

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

1. ~~Non-atomic repo refresh remains~~ ✗ Dismissed 📎 Requirement gap ☼ Reliability

Description

runAgent refreshes hostRepositoryDir by clearing it in-place and then repopulating it, which can
expose an empty or partially-populated directory to concurrent observers and also risk leaving
behind stale/foreign files created after the initial delete snapshot. This violates the compliance
requirement to update the target repo directory via an atomic swap so tools never observe an
incomplete or mixed state.

Code

internal/cli/run.go[R1120-1126]

+		//
+		// Clear the contents of the directory rather than removing the directory
+		// itself. Removing the directory would invalidate the inode, which breaks
+		// any shell whose CWD points at it (the next command in that shell would
+		// fail with "getwd: no such file or directory").
+		printer.StepInfo(fmt.Sprintf("Clearing %s before extraction", hostRepositoryDir))
+		if clearErr := clearDirContents(hostRepositoryDir); clearErr != nil {

Relevance

⭐⭐ Medium
Team accepted “atomic commit” consistency fix (#1954), but run.go repo extraction historically uses
RemoveAll clearing (#1038,#1045).
PR-#1954
PR-#1045
PR-#1038

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1525850 requires an atomic swap update strategy and flags in-place
clearing/repopulation as a failure mode; the PR changes runAgent from
os.RemoveAll(hostRepositoryDir) to clearDirContents(hostRepositoryDir), but clearDirContents
still performs an in-place transition by calling os.ReadDir once and then removing those entries
one-by-one via os.RemoveAll. Because there is no re-scan/retry, entries created after the initial
os.ReadDir snapshot can remain, and since sandbox.SafeDownload() then runs Download +
sanitizeDownload without purging arbitrary pre-existing contents, any leftovers can persist and be
mixed into the newly extracted repo while concurrent tools/watchers interact with the directory.

Avoid delete-then-recreate repo directory in fullsend run to prevent race conditions
internal/cli/run.go[1120-1127]
internal/cli/run.go[2816-2828]
internal/cli/run.go[2816-2830]
internal/cli/run.go[1118-1137]
internal/sandbox/sandbox.go[586-593]

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` refreshes the repo in `hostRepositoryDir` by clearing the directory in-place via `clearDirContents(hostRepositoryDir)` and then downloading/extracting into that same directory. This creates (1) a non-atomic window where external observers can see an empty or partially-populated repo, and (2) a correctness risk where `clearDirContents()` may leave behind entries created after its single `os.ReadDir` snapshot, which then persist through `sandbox.SafeDownload()` and can contaminate the extracted repo with stale/foreign files.

## Issue Context
Compliance requires `hostRepositoryDir` to be updated atomically (e.g., extract into a sibling temp dir and then atomically swap into place) so concurrent tools (IDEs, file watchers, git) always observe either the previous complete state or the new complete state, never an incomplete/mixed state. This PR also intentionally keeps the target repo directory inode stable, meaning the directory remains present and writable throughout the clear+download window, increasing exposure to concurrent mutations.

## Fix Focus Areas
- internal/cli/run.go[1120-1127]
- internal/cli/run.go[1125-1132]
- internal/cli/run.go[2816-2829]
- internal/cli/run.go[2819-2829]

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

fullsend-ai-review · 2026-07-07T16:24:18Z

Review — PR #3381

Verdict: approve ✅

Re-review note: The new commit (be97fb1) adds edge-case sub-tests for clearDirContents (empty directory, non-existent directory) — exactly the edge cases the prior review suggested. The production code in run.go is unchanged since the prior review (78963a0). All findings below are independently verified against the current head.

Clean, well-scoped fix for the inode invalidation that causes getwd: no such file or directory when running fullsend run twice from the same shell. The clearDirContents function is correct, the test suite now covers the key invariant (inode preservation via os.SameFile) plus edge cases, and the change is security-neutral.

Correctness

clearDirContents correctly iterates over directory entries and removes each individually via os.RemoveAll, preserving the parent directory inode. Error handling is correct — returns on first failure, caller treats it as fatal.
SafeDownload works correctly with a pre-existing (now empty) target directory — extraction overwrites into it.
Test (TestClearDirContents) validates three cases: (1) populated directory — inode preservation via os.SameFile and complete content removal via os.ReadDir returning empty, (2) empty directory — no-op without error, (3) non-existent directory — returns error, documenting the contract difference from os.RemoveAll's no-op behavior on missing paths.
No stale references to the old os.RemoveAll(hostRepositoryDir) pattern exist elsewhere in the codebase.
No existing tests were weakened or had assertions loosened.

Security

Path traversal: os.ReadDir in Go never returns . or .. entries, and POSIX filenames cannot contain /, so filepath.Join(dir, e.Name()) always resolves to a direct child of dir. No traversal risk.
Symlink handling: os.RemoveAll on individual entries removes symlinks without following them — identical behavior to the original os.RemoveAll on the parent directory.
Post-extraction sanitization: SafeDownload's symlink sanitization and .git/hooks/ removal run after extraction and are unaffected by this change.
Log injection: hostRepositoryDir is logged via printer.StepInfo using %s formatting with no shell execution surface.

Intent & scope

Change traces to authorized work (fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075) and is appropriately scoped as a bug fix.
PR correctly uses Relates-to (not Fixes) since issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 encompasses both the inode invalidation and a broader atomicity concern; this PR addresses only the former.
PR title fix(runner): ... follows COMMITS.md conventional commit format correctly.
DCO sign-off present on all commits.

Style

Function naming (clearDirContents), godoc comment format, error handling idioms, and test structure all follow established patterns in internal/cli/run.go and internal/cli/run_test.go.

Documentation

No documentation staleness. The change is an internal implementation detail; existing docs describe the extraction step at the appropriate abstraction level (SafeDownload with security sanitization). No docs reference the pre-extraction directory cleanup step.

Previous run

Review — PR #3381

Verdict: approve ✅

Re-review note: The new commit (78963a0) is a merge resolving conflicts with main in run_test.go. No substantive changes to the PR's own logic since the prior review (90a1a53). All findings below are independently verified against the current head.

Clean, well-scoped fix for the inode invalidation that causes getwd: no such file or directory when running fullsend run twice from the same shell. The clearDirContents function is correct, the test verifies the key invariant (inode preservation via os.SameFile), and the change is security-neutral.

Correctness

clearDirContents correctly iterates over directory entries and removes each individually via os.RemoveAll, preserving the parent directory inode. Error handling is correct — returns on first failure, caller treats it as fatal.
SafeDownload is routinely called with an existing target directory elsewhere in the codebase (e.g., security findings download), confirming extraction works correctly with a pre-existing directory.
Test (TestClearDirContents) validates both invariants: inode preservation via os.SameFile and complete content removal via os.ReadDir returning empty.
No stale references to the old os.RemoveAll(hostRepositoryDir) pattern exist elsewhere.

Security

Path traversal: os.ReadDir in Go never returns . or .. entries, and POSIX filenames cannot contain /, so filepath.Join(dir, e.Name()) always resolves to a direct child of dir. No traversal risk.
Symlink handling: os.RemoveAll on individual entries removes symlinks without following them — identical behavior to the original os.RemoveAll on the parent directory.
Post-extraction sanitization: SafeDownload's symlink sanitization and .git/hooks/ removal run after extraction and are unaffected by this change.
Log injection: hostRepositoryDir is logged via printer.StepInfo using %s formatting with no shell execution surface. Consistent with existing logging at line 1280.

Intent & scope

Change traces to authorized work (fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075) and is appropriately scoped as a bug fix.
PR correctly uses Relates-to (not Fixes) since issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 encompasses both the inode invalidation and a broader atomicity concern; this PR addresses only the former.
PR title fix(runner): ... follows COMMITS.md conventional commit format correctly.

Style

Function naming (clearDirContents), error handling idioms, godoc comment format, and test structure all follow established patterns in internal/cli/run.go and internal/cli/run_test.go.

Documentation

No documentation staleness. The change is an internal implementation detail; existing docs describe the extraction step at the appropriate abstraction level (SafeDownload with security sanitization).

Previous run

Review — PR #3381

Verdict: approve ✅

Clean, well-scoped fix for the inode invalidation that causes getwd: no such file or directory when running fullsend run twice from the same shell. The clearDirContents function is correct, the test verifies the key invariant (inode preservation via os.SameFile), and the change is security-neutral.

Security notes

os.ReadDir never returns .. entries (Go filters them in Readdirent), so filepath.Join(dir, e.Name()) cannot escape the directory.
os.RemoveAll on individual entries removes symlinks without following them — same behavior as the previous os.RemoveAll on the parent.
SafeDownload's symlink sanitization and .git/hooks/ removal run after extraction and are unaffected by this change.

Observations

Remaining race window (relates to #2075): The linked issue describes both inode invalidation and a race condition where concurrent readers see an empty/partial directory during the clear→extract window. This PR fixes the inode problem but the atomicity concern remains — the directory goes through an empty state between clearDirContents and SafeDownload. The PR correctly uses Relates-to (not Fixes), so this is properly scoped as an incremental improvement. A follow-up could implement the atomic swap approach suggested in #2075 to close the remaining window.

Test edge cases: The test covers the happy path well (files + subdirectory removed, inode preserved). Consider adding sub-tests for: (1) an empty directory (documents the no-op case), and (2) a non-existent directory (documents the error contract, which differs from os.RemoveAll's no-op behavior on missing paths).

Labels: PR modifies runner CLI code (internal/cli/run.go) with a bug fix for inode preservation

Previous run

Review — PR #3381

Verdict: approve ✅

Re-review note: The new commit (78963a0) is a merge resolving conflicts with main in run_test.go. No substantive changes to the PR's own logic since the prior review (90a1a53). All findings below are independently verified against the current head.

Clean, well-scoped fix for the inode invalidation that causes getwd: no such file or directory when running fullsend run twice from the same shell. The clearDirContents function is correct, the test verifies the key invariant (inode preservation via os.SameFile), and the change is security-neutral.

Correctness

clearDirContents correctly iterates over directory entries and removes each individually via os.RemoveAll, preserving the parent directory inode. Error handling is correct — returns on first failure, caller treats it as fatal.
SafeDownload is routinely called with an existing target directory elsewhere in the codebase (e.g., security findings download), confirming extraction works correctly with a pre-existing directory.
Test (TestClearDirContents) validates both invariants: inode preservation via os.SameFile and complete content removal via os.ReadDir returning empty.
No stale references to the old os.RemoveAll(hostRepositoryDir) pattern exist elsewhere.

Security

Path traversal: os.ReadDir in Go never returns . or .. entries, and POSIX filenames cannot contain /, so filepath.Join(dir, e.Name()) always resolves to a direct child of dir. No traversal risk.
Symlink handling: os.RemoveAll on individual entries removes symlinks without following them — identical behavior to the original os.RemoveAll on the parent directory.
Post-extraction sanitization: SafeDownload's symlink sanitization and .git/hooks/ removal run after extraction and are unaffected by this change.
Log injection: hostRepositoryDir is logged via printer.StepInfo using %s formatting with no shell execution surface. Consistent with existing logging at line 1280.

Intent & scope

Change traces to authorized work (fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075) and is appropriately scoped as a bug fix.
PR correctly uses Relates-to (not Fixes) since issue fullsend run: os.RemoveAll creates race condition with concurrent directory access #2075 encompasses both the inode invalidation and a broader atomicity concern; this PR addresses only the former.
PR title fix(runner): ... follows COMMITS.md conventional commit format correctly.

Style

Function naming (clearDirContents), error handling idioms, godoc comment format, and test structure all follow established patterns in internal/cli/run.go and internal/cli/run_test.go.

Documentation

No documentation staleness. The change is an internal implementation detail; existing docs describe the extraction step at the appropriate abstraction level (SafeDownload with security sanitization).

Previous run (2)

Review — PR #3381

Verdict: approve ✅

Clean, well-scoped fix for the inode invalidation that causes getwd: no such file or directory when running fullsend run twice from the same shell. The clearDirContents function is correct, the test verifies the key invariant (inode preservation via os.SameFile), and the change is security-neutral.

Security notes

os.ReadDir never returns .. entries (Go filters them in Readdirent), so filepath.Join(dir, e.Name()) cannot escape the directory.
os.RemoveAll on individual entries removes symlinks without following them — same behavior as the previous os.RemoveAll on the parent.
SafeDownload's symlink sanitization and .git/hooks/ removal run after extraction and are unaffected by this change.

Observations

Remaining race window (relates to #2075): The linked issue describes both inode invalidation and a race condition where concurrent readers see an empty/partial directory during the clear→extract window. This PR fixes the inode problem but the atomicity concern remains — the directory goes through an empty state between clearDirContents and SafeDownload. The PR correctly uses Relates-to (not Fixes), so this is properly scoped as an incremental improvement. A follow-up could implement the atomic swap approach suggested in #2075 to close the remaining window.

Test edge cases: The test covers the happy path well (files + subdirectory removed, inode preserved). Consider adding sub-tests for: (1) an empty directory (documents the no-op case), and (2) a non-existent directory (documents the error contract, which differs from os.RemoveAll's no-op behavior on missing paths).

Labels: PR modifies runner CLI code (internal/cli/run.go) with a bug fix for inode preservation

Both branches added independent test functions at the end of internal/cli/run_test.go. Keep both: TestClearDirContents from this branch and the config/backend/allowlist tests from main. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

fullsend-ai-review · 2026-07-09T12:17:40Z

🤖 Finished Review · ✅ Success · Started 12:17 PM UTC · Completed 12:25 PM UTC
Commit: 78963a0 · View workflow run →

Add sub-tests for empty directory (no-op) and non-existent directory (returns error) to document the function's contract. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

fullsend-ai-review · 2026-07-09T20:00:58Z

🤖 Finished Review · ✅ Success · Started 8:00 PM UTC · Completed 8:09 PM UTC
Commit: be97fb1 · View workflow run →

ralphbean requested a review from a team as a code owner July 7, 2026 16:14

github-actions Bot deployed to site-preview July 7, 2026 16:16 View deployment

qodo-code-review Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/cli/run.go

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/runner Agent runner behavior and lifecycle type/bug Confirmed defect in existing behavior go Pull requests that update go code Review effort 2/5 labels Jul 7, 2026

github-actions Bot deployed to site-preview July 9, 2026 12:17 View deployment

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jul 9, 2026

test(runner): add edge-case sub-tests for clearDirContents

be97fb1

Add sub-tests for empty directory (no-op) and non-existent directory (returns error) to document the function's contract. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

github-actions Bot deployed to site-preview July 9, 2026 20:02 View deployment

fullsend-ai-review Bot approved these changes Jul 9, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jul 9, 2026

Uh oh!

Conversation

ralphbean commented Jul 7, 2026

Summary

Problem

Fix

Test plan

Uh oh!

qodo-code-review Bot commented Jul 7, 2026

PR Summary by Qodo

Uh oh!

fullsend-ai-review Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

codecov Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qodo-code-review Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review — PR #3381

Correctness

Security

Intent & scope

Style

Documentation

Review — PR #3381

Correctness

Security

Intent & scope

Style

Documentation

Review — PR #3381

Security notes

Observations

Review — PR #3381

Correctness

Security

Intent & scope

Style

Documentation

Review — PR #3381

Security notes

Observations

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Jul 7, 2026 •

edited

Loading

github-actions Bot commented Jul 7, 2026 •

edited

Loading

codecov Bot commented Jul 7, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 7, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 7, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 9, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 9, 2026 •

edited

Loading