fix(teardown): refuse unsafe scratch returns - #1968
Draft
coreldh wants to merge 5 commits into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
--forceis supplied, while preserving task branch refs.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=0Pipeline
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 beforereap_task_worktree_processes; a task process can handle its TERM by writing an untracked file and then exit. The subsequent known-runtime cleanup andtreehouse return --forcerun 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 staleindex.lockinto an immediate generic refusal whenevergit statusneeds 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 reachestreehouse returnorfm_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_cleanlinessinvokesgit ... 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 --checkSelected end-to-end cases fromtests/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.