Deepen Worktree ownership of stale Architecture review recovery #505

What to build

A repeated Architect run must preserve a checkout that an earlier failed Architecture review acquisition explicitly retained. Today a failing checkout hook can leave review work intact, but the next review treats the same pathname as disposable scratch and force-removes that work before the acquisition owner can protect it.

Deepen the Worktree module so successful detached review acquisition records disposal eligibility for that acquired instance. A later Architect run recovers only a positively identified, successfully acquired review checkout. Failed, unmarked, replaced or uncertain checkouts survive with a diagnostic. Successfully acquired review scratch remains disposable after its owning process dies, including edits and detached commits. Callers keep the existing create/use/drop interface.

Acceptance criteria

• A detached review add rejected by a checkout hook retains its tracked, staged, untracked and ignored work, or detached commits, as today. After removing the failing hook, another review acquisition refuses the retained checkout and preserves its files, commits and registration. Cover interruption during add too; retry must not erase retained work.
• Successful detached review acquisition publishes a private, versioned disposal record only after ownership capture succeeds, while acquisition recovery remains armed and the repository worktree lock is held. The record agrees with an excluded checkout token and identifies the captured checkout, administrative directory and common repository. No valid record is published by a failed add or failed ownership capture.
• Record publication is the final fallible acquisition step. Marker/exclusion/publication errors keep the original acquisition error and use existing guarded recovery: remove only verified attempt-owned marker artifacts, then remove a clean, unchanged attempt checkout; retain actual work or uncertainty. A process killed before final publication leaves an ineligible checkout, which the next review retains.
• A successfully acquired review whose factory process is killed is recovered by the next Architect run, and that next review can run normally and clean up. Verify disposable scratch and detached commits as well as the existing command-level crash regression.
• Stale recovery requires a complete registration inspection and matching disposal evidence. Same pathname, administrative pathname and HEAD alone do not authorize removal. Retain recreated checkouts or administrative directories, changed attached/detached identity, locked registrations, symlinks, mismatched tokens, missing or malformed records, unsupported record versions and unreadable or inconsistent evidence.
• An unmarked legacy or manually created checkout at the review pathname is retained and named; no ownership is inferred or retroactively granted. A dangling symlink or a registration whose checkout path is missing is uncertainty, not absence. Such a refusal occurs before a new add or agent session starts.
• Verification, stale disposal and new acquisition remain serialized by the existing repository worktree lock. Stale recovery reuses the private ownership inspection/removal implementation without recursively taking that lock. Ordinary interruption stops a new recovery/acquisition; existing completion cleanup and acquisition recovery still finish without clearing the recorded interruption.
• Removing an eligible stale review deletes only its verified detached checkout and registration, with its own marker artifacts. It deletes no local Issue branch and performs no commit, push or agent session. A removal failure retains the remaining resources and fails acquisition with the path, available head and reason.
• Markers remain outside commits through the existing locked exclusion mechanism and their administrative location. Existing marker files, links or tracked files are never overwritten. Normal live-owner cleanup keeps its established disposal policy and removes its markers with the owned checkout; missing stale-disposal markers do not negate a live owner's pinned identity.
• Preserve fresh Run and Continuation acquisition, Failed run preservation, origin sampling, branch synchronization and their existing tests. Both successful review cleanup and verified stale recovery use the same ownership evidence rules, with durable eligibility adding the cross-invocation check.
• Sharpen the Worktree entry in CONTEXT.md: failed-acquisition retention survives later review attempts, and stale review scratch is disposable only with evidence of successful acquisition and unchanged ownership. Record an ADR for durable review disposal eligibility, including conservative retention of unmarked legacy paths and the successful-acquisition publication point. These documentation changes belong to this Ticket's pull request.
• Tests cross the existing Worktree/ReviewWorktree acquisition and scope-exit interfaces with real local Git, a temporary origin, failing hooks and deterministic filesystem/fault fixtures. Assert retained file contents, exact commits, registrations, diagnostics and successful subsequent review behavior; avoid asserting private call order or the serialized representation.
• Run focused Worktree and Architect lifecycle tests, then formatting, clippy with warnings denied and the full test suite. Keep the supported Linux and macOS behavior green.

Blocked by

None (can start immediately).

Decisions

The design tree is settled in four rounds: disposal policy and scope → durable evidence and publication → recovery, interruption and cleanup → tests, documentation and plan size. Each decision below includes its reason; no branch requires a human decision.

Round 1: disposal policy and scope

1. Retention survives repeated acquisition. Failed review acquisition promises to retain work and uncertainty; a later invocation cannot silently revoke that promise by reusing the pathname. This follows the existing Worktree definition and failed-add tests rather than introducing a new product preference.
2. Keep automatic recovery of successfully acquired review scratch. The Architect crash-recovery test requires the next invocation to recover a review checkout after its factory process dies. Successful owned scratch, including detached commits, is already disposable. Refusing every leftover would break that behavior, so eligibility must distinguish successful acquisition from retained failure.
3. Deepen the Worktree module behind its existing acquisition seam. One private ownership implementation owns eligibility publication, stale inspection and verified disposal. Runs, Architecture reviews and their tests keep their current interface; they do not gain marker management or a separate recovery call. This creates depth and locality rather than exposing another ordering obligation.
4. Limit durable eligibility to detached Architecture review checkouts. Issue branch acquisition and Failed run salvage already have their own lifetime rules. They do not need persistent review disposal authority. Closed Deepen Worktree acquisition to own failed adds #501 and Deepen Worktree ownership through successful cleanup #503 explicitly left stale review recovery outside their work; this addresses that remaining gap and preserves the ADRs governing Continuations and delivery.

Round 2: durable evidence and publication

5. Use paired private evidence: a checkout token and an administrative record. An excluded root token carries a fresh random per-acquisition nonce generated from an exclusively created temporary filename; a record in that checkout's Git administrative directory carries the same nonce, a format version, canonical checkout/administrative/common-repository paths and their device/inode identities, and the required detached mode. A root token plus administrative evidence prevents ordinary same-path recreation or administrative-directory reuse from inheriting eligibility. A shared pathname or HEAD is insufficient.
6. Record instance identity, not the acquisition commit. Review experiments may create detached commits after acquisition. Recovery verifies the current HEAD and registration consistency but does not require the original HEAD, because successful scratch remains disposable as it changes.
7. Keep the record strict and private. Use a versioned JSON schema with required typed fields, rejection of duplicate or unknown fields, and rejection of unsupported versions. Add serde with derive support as a direct dependency using the already resolved crate, alongside the existing JSON dependency. This is simpler than hand-writing a strict parser and prevents ambiguous evidence from authorizing disposal. There is no migration that grants eligibility to an old unmarked checkout.
8. Publish eligibility only after successful ownership capture. The existing acquisition guard stays armed through capture, exclusion, root-token creation and record preparation. The root token and record use exclusive creation and regular files, never following or replacing existing links/files. Reuse the locked exclusion mechanism and existing temporary-file dependency; no new external adapter is needed.
9. Make atomic record publication the acquisition commit point. Prepare and sync file contents first, then publish the complete administrative record without replacement as the final fallible step. After publication, only infallible ownership transfer and guard disarming remain. The existing temporary-file implementation has no fallible step after successful destination publication. This prevents an ordinary failed acquisition from leaving a valid success record. The guarantee concerns process death; it does not promise survival of whole-machine power loss.
10. Keep partial marker artifacts inside failed-acquisition ownership. Before publication, errors remove only this attempt's positively identified token/temporary files, then use existing clean-checkout recovery. If artifact identity or removal is uncertain, retain and name it. This preserves clean failure recovery without treating an attempt-owned ignored marker as user work, and preserves user work when a hook actually left it. If the process dies before publication, missing/partial evidence remains ineligible.

Round 3: recovery, interruption and cleanup

11. Inspect stale resources before a new add. Use symlink-aware path inspection and the complete registration listing to distinguish true absence from missing paths, dangling links and damaged registrations. An occupied or registered path requires successful eligibility verification; an unknown resource is retained, not passed to a force-removal attempt. This is the missing ownership check in the current shallow stale recovery module.
12. Reopen and pin the recorded directories, then reuse ownership verification. Under the existing worktree lock, match the paired nonce and recorded directory identities to current non-symlink directories and their handles. Recheck common-repository identity, detached registration, root/admin links, available HEAD and lock state through the existing inspection implementation before removal. Keeping those handles through inspection/removal preserves the live-owner replacement checks during the recovery operation.
13. Keep one lock from stale inspection through new acquisition. The creation path already holds the repository worktree lock. Share an internal removal operation that assumes that lock is held; normal cleanup acquires its completion lock before calling the same operation. Calling the existing locking cleanup method from stale recovery would deadlock. The lock serializes cooperating factory writers; this does not introduce coordination with every unrelated Git process.
14. Use ordinary execution for starting recovery, completion execution for finishing owned work. A recorded interrupt prevents a new review recovery/add. Failed acquisition recovery and live scope-exit cleanup retain their established completion behavior. Check interruption before the final publication point, with no new fallible post-publication check. This preserves Command interruption semantics without publishing success and then reporting acquisition failure.
15. Retain uncertainty with an actionable identification, not disposal by name. Missing, malformed, mismatched or legacy evidence, inspection errors and removal errors fail review creation and identify the retained path, current head when readable and reason. Do not mint eligibility, clear locks or prune registrations to make an unknown checkout removable. The owned checkout's successful removal naturally removes its record and token; no sweep of other resources is added.
16. Keep live-owner authority independent of restart evidence. Existing pinned handles still authorize normal cleanup of an owned successful review, even if its scratch experiment removed a marker. The durable record is needed when that owner has died. A replacement rejected by live cleanup must also fail later stale verification, while successful original scratch with intact evidence remains recoverable after a crash. This adds cross-invocation depth without weakening existing cleanup.

Round 4: verification, documentation and size

17. The interface is the test surface; dependencies are local-substitutable. Use the existing real-Git adapter and temporary repositories, hooks, isolated signal runner and Git fault fixtures. Test the two-invocation sequence, not a pure marker parser in isolation. Tests assert files, commits, registration state, diagnostics and the next review's result; they should survive changes to the private representation.
18. Protect both sides of the policy. Keep the existing successful crash-recovery regression. Add failed-hook and interrupted-add retries with retained work; failed capture/publication; death before final publication; missing/corrupt/unsupported evidence; token and directory substitution; locked/attached checkouts; and clean failure recovery. Use barriers or recorded child handles for interruption/interleaving, and release and drain every child the fixture starts. These cases prove that preservation and automatic recovery coexist.
19. Document the durable authority through this Ticket. Sharpen the existing Worktree term rather than inventing a domain term. Add an ADR because permanent eligibility affects recovery across invocations and old unmarked artifacts, and the trade-off between automatic scratch recovery and preserving unknown work would otherwise be surprising. No existing ADR is reopened.
20. Publish one standalone Ticket. The complete change is one disposal-eligibility rule, a private record protocol, shared inspection and its lifecycle tests. It fits one fresh context window, and separating publication from recovery would leave an unverifiable intermediate policy. It has no blockers; an open issue asking for better error guidance does not cover this deepening.

Validation from this Architecture review

Reviewed main at a2faf4f. A disposable test used the real review-acquisition interface: a failing post-checkout hook wrote a file, the acquisition error named the retained checkout, the hook was removed, and acquisition was retried. cargo test architecture_probe_review_retry_preserves_failed_acquisition_work --bin thirdshift -- --nocapture failed because the retry removed the retained checkout and returned a new successful owner. The diagnostic printed “removed the leftover worktree” between attempts. The probe was removed; no production change is proposed as an implementation in this review.

The existing Worktree suite passed: 89 tests. The existing Architect process-death recovery test passed: 1 test, confirming the successful-scratch behavior the design must preserve. Atomic publication behavior was checked against the installed temporary-file implementation for Linux and macOS. Implementation and its full CI checks are this Ticket's work.

Architecture review

Strength: Strong · Dependencies: local-substitutable

Files: src/worktree.rs, src/worktree/acquisition.rs, src/worktree/ownership.rs, a private review-recovery module under src/worktree/, src/worktree/acquisition_tests.rs, src/worktree/cleanup_tests.rs, tests/architect_run.rs, Cargo.toml, Cargo.lock, CONTEXT.md, a new ADR in docs/adr/.

Before

flowchart LR
  first["Architect run"] --> acquire["Review Worktree acquisition"]
  acquire --> failed["Failed acquisition recovery"]
  acquire --> live["Worktree lifetime cleanup"]
  next["Next Architect run"] --> stale["Stale review recovery"]
  failed -. "retained pathname reused" .-> stale
  classDef leak stroke:#dc2626,stroke-width:2px;
  class failed,stale leak

After

flowchart LR
  first["Architect run"] --> acquire
  next["Next Architect run"] --> stale
  subgraph owner["Worktree ownership"]
    acquire["Review Worktree acquisition"] --> failed["Failed acquisition recovery"]
    acquire --> live["Worktree lifetime cleanup"]
    acquire -- "publish eligibility" --> stale["Stale review recovery"]
    failed -- "retain across retries" --> stale
  end
  classDef deep stroke-width:3px;
  class owner deep

Problem: Stale review recovery is a shallow module whose pathname-only interface erases work retained by failed acquisition.

Solution: Deepen the Worktree module with durable disposal eligibility and shared ownership inspection behind the existing acquisition seam.

Wins

• Depth: acquisition hides retry policy.
• Locality: one ownership implementation.
• Leverage: every repeated Architect run.
• Tests cross one existing interface.
• Reuse the real Git adapter.

Deletion test: Removing the deepened ownership implementation would redistribute eligibility, retention and disposal ordering across acquisition, lifetime cleanup and stale recovery; that complexity would spread rather than disappear.

Coverage: All 49 open issue titles and bodies were read and checked again. None proposes this deepening. #120 and #53 concern error guidance; closed #501 and #503 cover failed acquisition and live cleanup respectively and explicitly exclude stale review recovery. Existing ADRs are preserved.