feat(cmd): make destroy safe by default#37
Merged
Conversation
Destroy was dangerous: `destroy --all --force` deleted immediately with no preview, --all's scope was unclear, and --force overrode every protection at once (this wiped leased/reserved homes in a real incident). Bring destroy up to prune's safety bar while keeping it the deliberate "remove this even though it has unlanded work" tool. - Narrow, explicit targets: `destroy <path>` removes exactly one worktree; `destroy <pool> --all` removes that pool only. There is no cross-pool or global destroy, and --all with no pool target is an error. - Dry-run by default, like prune. --yes is required to execute. - Replace the blunt --force with per-risk opt-in flags: --include-unlanded (uncommitted or unmerged), --include-in-use (running process; terminated cleanly first), and --include-leased (leased, only when the exact path is named, NEVER via --all). A bare `--all --yes` removes only the disposable set and skips the rest, naming the flag that would include each. - Risk-revealing preview: a status tag, path, and size per worktree, plus an honest summary of what was destroyed and skipped (never a blind "all worktrees destroyed"). - A single named target that is skipped for lack of a flag exits non-zero. - Destroy and prune share classification primitives (classifyForDestroy) so they agree on leased / in-use / unlanded / disposable. BREAKING CHANGE: the `treehouse destroy --force` flag is removed. Replace it with the specific --include-unlanded / --include-in-use / --include-leased flag(s) for the risk you intend to override, plus --yes, and pass an explicit pool path to --all.
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
Redesign the 'treehouse destroy' command to be safe-by-default as a BREAKING CHANGE that release-please bumps to v2.0.0 (committed with a 'feat!:' subject + 'BREAKING CHANGE:' footer; versions are managed by release-please and must NOT be hand-edited). Background: the old destroy was dangerous - 'destroy --all --force' deleted immediately with no preview, --all's scope was unclear, and --force overrode every protection at once, which wiped leased/reserved homes in a real incident. The goal was to bring destroy up to prune's safety bar while keeping it the deliberate 'remove this even though it has unlanded work' tool.
Decisions implemented (all intentional):
Architecture choices: destroy and prune share classification primitives (new classifyForDestroy in internal/pool/destroy.go reuses prune's ownerAlive/process scan/backingRepositoryMissing/git.IsDirty/git.IsHeadMergedIntoRef against the same resolvePruneDefaultRef ref) so they agree on leased/in-use/unlanded/disposable. New entry points pool.DestroyWorktree (single path, allowLeased=true) and pool.DestroyPool (bulk, allowLeased=false) intentionally REPLACE the old exported pool.Destroy/pool.DestroyAll and the now-unused worktreeInUse helper, which were removed; the well-tested two-phase reservation + pre_destroy hook safety (a worktree re-acquired during its hook is never deleted) is preserved. The pool-level functions return classified results and never error on skips; only the cmd layer maps a single-target no-op to a non-zero exit. The 'unverified' class (orphaned/cannot-verify) is intentionally gated like unlanded. The pool/cmd destroy tests were rewritten for the new API and e2e tests added for dry-run default, lease protection under --all, per-risk flags, scoped --all, and confirmation gating. Docs (README + AGENTS.md) updated with the new surface and a --force -> --include-* migration table.
What Changed
treehouse destroyaround dry-run-by-default execution, explicit single-worktree or pool-scoped--alltargets, and honest skip behavior for unsafe candidates.--forcepath and adds per-risk opt-ins for unlanded, in-use, and leased worktrees, with leased destruction limited to exact path targets.internal/pool, sharing prune-style safety classification and reservation checks, and updates the CLI, docs, and tests for the new destroy surface.Risk Assessment
Testing
Verified the branch has a
feat!commit with aBREAKING CHANGEfooter, ran the full Go test suite successfully, and captured a manual CLI transcript showing rejected global--all, removed--force, dry-run preservation, lease protection in bulk destroy, named leased removal, dirty gating with non-zero single-target exit,--include-unlandedremoval, and pool-scoped--allpreserving another repo's worktree.Evidence: Release-please breaking-change signal
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (4) ✅
internal/pool/destroy.go:218- Removal is authorized from a single DestroyClass, but classifyForDestroy returns at the first risk. A dirty or unmerged worktree that is also leased or in-use can be destroyed with only --include-leased or --include-in-use, bypassing --include-unlanded; a leased worktree with live processes also bypasses process termination. Track independent risks and require every matching --include-* flag before planning removal.internal/pool/destroy.go:335- The reservation pass trusts the stale plan and overwrites the current state entry without rechecking owner, lease, dirtiness, or merge state. If another get or lease acquires a planned disposable worktree before this lock is taken, destroy can replace that reservation and delete the active worktree. Reclassify under the lock before setting Destroying and skip anything no longer allowed.internal/pool/destroy.go:384- After pre_destroy hooks, deletion only checks sameDestroyReservation. A hook or concurrent process can add uncommitted files, move HEAD, or start a process after planning, and a bare --yes still force-removes the worktree. Mirror prune's final safety check just before removal and skip with the reservation cleared when the current risks are no longer allowed.internal/pool/destroy.go:412- removeManagedWorktree drops both git worktree remove and os.RemoveAll errors, while executeDestroy still reports the target as destroyed and removes it from state. A failed deletion, especially from locked files on Windows, can leave content on disk with no managed state entry. Return cleanup errors and keep the entry with its reservation cleared as a skipped cleanup failure.🔧 Fix: Harden destroy risk checks
1 warning still open:
internal/pool/destroy.go:432- The destroy path treats a nil return from TerminateWorktreeProcesses as proof that the worktree was drained, but that helper only reports scan errors and intentionally swallows kill failures while filtering the current process and its parents. A surviving process can therefore remain in the worktree while destroy removes it and reports success; re-scan after termination and skip removal if any processes are still present.🔧 Fix: Guard destroy against process survivors
1 error still open:
internal/pool/destroy.go:373- Destroy reservations overwrite an existing live owner reservation, but the later skip paths only clear the destroy reservation. If--include-in-usereserves an owner-held worktree and a hook dirties it, a process survives, or removal fails, the worktree is left in state withOwnerPID/OwnerStartedAtcleared, so a latergetcan hand out a worktree still owned by the original agent. Preserve the previous owner reservation and restore it whenever destruction does not complete.🔧 Fix: Restore destroy owner reservations on skips
3 warnings still open:
cmd/destroy.go:163---allfalls back togit.FindRepoRootFrom, which returns the linked worktree root when the target is inside a managed worktree. For local-only repos or relativerootconfigs,config.ResolvePoolDirthen computes a different pool under that worktree, so documented commands liketreehouse destroy . --all --yesfrom a subdirectory can report nothing destroyed while locking the wrong pool. Usegit.FindMainRepoRootFromhere, matching return's resolver.internal/pool/destroy.go:272- A process enumeration failure is classified only asDestroyUnverified, so--include-unlanded --yesauthorizes deletion and the final removal path never enters the in-use termination branch. If process scanning fails, destroy can remove a worktree without proving it is idle or draining processes. Treat process scan failure as an in-use blocker, or skip unless the process state can be rechecked successfully.internal/pool/destroy.go:512- WhenrepoRootis empty but the backing repository is present,removeManagedWorktreeskipsgit worktree removeand still deletes the container directory. This can happen if a pool is planned while all entries look orphaned and the backing repo reappears before final deletion, leaving stale worktree registrations in git. Re-resolve the repo root at removal time or refuse non-orphan removal without one.🔧 Fix: Harden destroy safety resolution
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
git log --format='%H%n%s%n%b%n---ENDCOMMIT---' 67f31cecf29e42a40ff8565336f4d8d5d4b6b2d2..973bcccac4244e9f9734ac4c164c83e9f41f3d85go test ./...Manual CLI E2E using a builttreehousebinary against isolated git repos, recorded in.no-mistakes/evidence/fm/th-destroy-v2/destroy-v2-cli-transcript.txt✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.