Skip to content

fix(teardown): refuse unsafe scratch returns - #1968

Draft
coreldh wants to merge 5 commits into
kunchenguid:mainfrom
coreldh:fm/c0808-fm-scratch-return
Draft

fix(teardown): refuse unsafe scratch returns#1968
coreldh wants to merge 5 commits into
kunchenguid:mainfrom
coreldh:fm/c0808-fm-scratch-return

Conversation

@coreldh

@coreldh coreldh commented Aug 8, 2026

Copy link
Copy Markdown

Intent

Bring the existing scratch-return pull request into repository pipeline compliance. Preserve the deliberate refuse-rather-than-clean behavior: unknown untracked or tracked worktree content may be genuine unlanded work, so teardown must name and refuse it rather than delete it. Keep managed runtime artifacts from poisoning the reusable pool, preserve task branch refs, and validate the added regression coverage. Adopt the already-open PR if possible; never force-push, merge, enable auto-merge, approve, close it, or create a duplicate pull request.

What Changed

  • Make teardown refuse and name unknown tracked or untracked worktree content, including when --force is supplied, while preserving task branch refs.
  • Allow only Firstmate-managed runtime artifacts to be cleaned before a pooled worktree is returned.
  • Revalidate return safety after cleanup and on retry paths, with provably stale Git-lock recovery before rerunning the checks.

Risk Assessment

✅ Low: The final diff closes the post-reap and stale-lock paths while preserving the intended refusal, managed-artifact cleanup, and branch-retention behavior.

Testing

Targeted CLI regression validation passed with reviewer-visible transcript evidence; no UI surface applies. The only test-related fix was aligning two legacy assertions with the new explicit tracked-change refusal message.

Evidence: Targeted teardown behavior transcript

ok - untracked scratch is named and refused before it can make a returned copy unavailable ok - forced return preserves untracked candidate work and its task branch ok - clean return preserves its task branch and makes the copy available ok - process-created unknown worktree content is named and preserved after reap ok - provably-stale worktree index.lock (old, no live holder) is cleared and teardown succeeds ok - stale lock cleanup rechecks and refuses dirty worktree before return ok - dirty worktree is refused even when its committed work has landed (dirty always wins) exit_code=0

ok - untracked scratch is named and refused before it can make a returned copy unavailable
ok - forced return preserves untracked candidate work and its task branch
ok - clean return preserves its task branch and makes the copy available
ok - process-created unknown worktree content is named and preserved after reap
ok - provably-stale worktree index.lock (old, no live holder) is cleared and teardown succeeds
ok - stale lock cleanup rechecks and refuses dirty worktree before return
ok - dirty worktree is refused even when its committed work has landed (dirty always wins)
exit_code=0

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (3) ✅
  • 🚨 bin/fm-teardown.sh:2235 - Required behavior is still bypassable: “unknown untracked or tracked worktree content may be genuine unlanded work, so teardown must name and refuse it rather than delete it.” Cleanliness is checked before reap_task_worktree_processes; a task process can handle its TERM by writing an untracked file and then exit. The subsequent known-runtime cleanup and treehouse return --force run without rechecking cleanliness, so that new file can be discarded or returned to the pool unnamed. Revalidate cleanliness immediately after reaping/runtime cleanup and on the stale-lock retry path, directly before each destructive return.

🔧 Fix: Revalidate worktree safety before every return attempt
1 warning still open:

  • ⚠️ bin/fm-teardown.sh:1200 - The new pre-return cleanliness check turns a provably stale index.lock into an immediate generic refusal whenever git status needs to refresh the index. This bypasses the existing stale-lock recovery path and breaks the documented/tested stale-lock teardown sequence (including --force): the wrapper never reaches treehouse return or fm_lock_is_provably_stale. Route a cleanliness-inspection failure caused by the worktree lock through the existing stale-lock proof, then rerun the full pre-return check.

🔧 Fix: Recover stale locks before final return validation
1 error still open:

  • 🚨 tests/fm-teardown.test.sh:526 - The new stale-lock regression fixture never intercepts the production command: validate_worktree_return_cleanliness invokes git ... status --porcelain=v1, but the fake only matches the literal --porcelain. It therefore delegates to real Git, so the test can pass through the old return-lock path without proving that a lock-caused final-cleanliness failure is recovered. Match --porcelain=v1 (or the supported porcelain form) and assert the injected status-failure path fired.

🔧 Fix: Exercise porcelain-v1 stale-lock recovery
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • git diff --check
  • Selected end-to-end cases from tests/fm-teardown.test.sh: untracked scratch refusal, forced preservation of candidate work and branch, clean pool return, post-reap content refusal, stale-lock cleanup, and dirty-worktree refusal.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@coreldh coreldh changed the title fix: refuse dirty pooled worktrees fix(teardown): refuse unsafe scratch returns Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant