Skip to content

Deepen Worktree ownership of Failed run preservation #482

Closed

Description

@JacobStephens2

What to build

Keep a Failed run's work safe through one Worktree preservation operation. A Run or Spec run must commit and push unfinished work as it does today, retain the worktree and local Issue branch when preservation fails, and report the original failure with its Session log and pull request. Delivery should no longer coordinate the Git salvage sequence and the separate decision to retain the worktree.

Acceptance criteria

  • Worktree owns one mutating Failed run preservation operation taking the settled Base branch and the first line of the failure cause. It returns success or the preservation error and internally owns the cleanup decision.
  • Retention is armed before the first salvage operation. Any propagated salvage error, or unwinding during preservation, leaves the worktree and local Issue branch retained without another caller action. Successful preservation re-enables normal cleanup.
  • Preserve the existing sequence: abort an unfinished merge; stage all work; compare the index with the last-fetched Base branch on origin; do nothing further when there are no changes; otherwise create the existing failure marker commit, including its timestamp, host and uncommitted-work explanation, and push without local hooks. Keep the current commit flags and cached-diff probe semantics.
  • Preservation uses a completion Git view, succeeds after recorded Command interruption where Git operations allow it, and neither clears the interruption nor permits ordinary Git work to continue.
  • Delivery delegates preservation once, reports a preservation error as a secondary progress line, and retains its original failure cause, interruption status, Session log, PR URL and draft-conversion rules. A Policy refusal still skips preservation and leaves the checked ready PR unchanged.
  • Remove Worktree's public raw-Git accessor and separate retention setter once their only production caller has migrated. Remove Delivery's separate retention operation and its caller choreography; keep Worktree's Launch directory access for Base fix.
  • Real-Git tests exercise the Worktree interface and observe remote content, failure commit metadata, and worktree/local-branch existence after drop. Cover unchanged work, committed plus uncommitted work, unfinished-merge recovery, propagated merge-abort/staging/commit/push failures, and recorded interruption.
  • Replace Delivery's scripted assertions about a separate retention call with Worktree outcome tests. Keep Delivery tests for preservation delegation, secondary diagnostics, original cause, draft conversion and Policy refusal, plus existing Failed run end-to-end coverage and Claim behavior.
  • Formatting, linting and the repository test suite pass. No new domain term or ADR is required: the existing Failed run, Command interruption, Continuation and Policy refusal definitions already describe this behavior.

Blocked by

None (can start immediately).

Decisions

The design tree was settled in prerequisite order: behavior and ownership; interface and dependencies; salvage and reporting; verification. Every branch below is resolved. Today OnFailure::commit_and_push owns the Git sequence, fail calls keep after PR accounting, and Worktree drop owns destructive cleanup; recent Git/GitHub interruption changes touched this path.

  1. Deepen the existing Worktree module, with Delivery as its caller. Worktree already owns destructive cleanup, so placing the preservation decision there gives locality to the invariant that local-only work survives. Deleting the caller's Git sequence and retention call concentrates their complexity beside cleanup; it does not move that complexity into another caller.
  2. Use one mutable operation, named preserve_failed_run, returning Result<()>. The caller needs only to report success or a secondary error. Separate outcomes for unchanged work and pushed work would add interface without leverage. Accept only the Base branch and failure reason: Worktree already knows its Git directory and Issue branch.
  3. Arm retention before any fallible work; disarm it only on successful completion. This keeps the checkout safe even when failure happens before the push or the caller unwinds while handling the error. The existing Worktree drop implementation remains responsible for cleanup or the line naming the retained path, branch and head.
  4. Move the entire existing salvage sequence and failure-commit construction into Worktree. This includes merge abort, staging, the cached-diff probe, timestamp/host formatting, marker commit and push. It makes the real implementation testable through the same interface Delivery uses. Preserve the last-fetched comparison, --allow-empty marker commit and hook-skipping behavior; do not add fetches, force pushes, retries or a new preservation policy. The existing cached-diff boolean probe treats a completed nonzero exit as changes; changing that interpretation is outside this refactor.
  5. Select the completion Git view inside preservation. Failed run salvage is finishing work under the existing Command contract. The caller must not supply an interruption flag, and preservation must not change the recorded interruption or any ordinary Git view.
  6. Keep failure reporting and PR changes in Delivery. Delivery still chooses the first line of the normalized original cause, skips preservation for Policy refusal, emits the existing secondary salvage diagnostic, and performs PR lookup/draft conversion independently of preservation success. Those concern the Run's outcome; Worktree owns the retained work before they run.
  7. Replace the old interface rather than adding an alternative path. The raw-Git accessor and retention setter have only the Failed run production caller, so remove them and the scripted retention operation after migration. Launch directory access has a separate Base fix caller and remains. No public trait or new dependency adapter is needed: real Git with a temporary bare origin is already local-substitutable.
  8. Test preservation at the Worktree seam with real temporary Git repositories. Capture the checkout path and branch before the call, drop the owner, then inspect existence and remote content; do not inspect its retention flag or assert exact Git command lists. Configure fixture-local identity, signing and hooks so these tests do not depend on the machine's Git preferences.
  9. Use natural repository failures for retention tests. Editing an automatically merged file after a merge conflict makes merge abort fail; an obstructed index, empty identity, rejecting bare-origin hook and unreachable local origin exercise the other propagated errors without a production fault-injection interface. Each failed attempt must leave available local work intact after drop; verify the failure commit when commit succeeded before push failed. This covers the branches hidden by today's one scripted preservation answer.
  10. Reuse isolated recorded-signal tests. Create the fixture before recording SIGINT, SIGTERM or SIGHUP, then preserve, verify the remote work and allowed cleanup, and confirm the interruption is still recorded and ordinary Git execution still fails. Subprocess isolation prevents signals from contaminating other tests.
  11. Retain verification at both relevant seams. Worktree tests replace scripted retention assertions; Delivery tests retain outcome/reporting behavior, and the existing binary Failed run tests continue checking PR drafting, notifications/log output and Claim behavior. Run the normal formatting, lint and full test checks when implementing. This avoids duplicating command-by-command tests of the moved implementation.
  12. Publish one standalone Ticket with no blockers and only needs-triage. The change is one local ownership refactor across Worktree, its sole preservation caller and their tests; it fits a single fresh context window. It introduces no domain concept or hard-to-reverse trade-off, so no Spec, glossary edit or ADR is needed.

Review validation on Base branch main at ca366e0: a throwaway prototype passed three real-Git checks for successful salvage/cleanup, commit failure retention and push failure retention. The existing Failed run binary suite passed all 12 tests. Both before/after diagrams passed Mermaid parsing. The prototype was removed and the worktree is clean; this issue specifies the implementation still to do.

Architecture review

Strength: Strong · Dependencies: local-substitutable · Covered by: None; all 46 open issue titles and bodies checked.

Files: src/worktree.rs, src/delivery.rs, src/test_support.rs, tests/failed_run.rs

Before

flowchart LR
  subgraph Delivery[Delivery]
    direction TB
    Failed[Failed run] --> Salvage[Git salvage]
    Failed --> Retention[Retention]
  end
  subgraph Worktree[Worktree]
    Cleanup[Cleanup]
  end
  Salvage -. completion execution .-> Git[Git]
  Retention -. keep after failure .-> Cleanup
  Cleanup --> Git
  classDef leak stroke:#dc2626,stroke-width:2px;
  class Salvage,Retention leak;
Loading

After

flowchart LR
  subgraph Delivery[Delivery]
    Failed[Failed run]
  end
  subgraph Worktree[Worktree]
    direction TB
    Retention[Retention] --> Salvage[Git salvage]
    Retention --> Cleanup[Cleanup]
  end
  Failed -->|preserve| Retention
  Salvage --> Git[Git]
  Cleanup --> Git
  classDef deep stroke-width:3px;
  class Worktree deep;
Loading

Problem: Worktree's shallow preservation interface makes Delivery coordinate Git salvage, retention and cleanup across the seam.

Solution: Deepen the Worktree module: its implementation owns preservation and retention behind one interface using the existing Git adapter.

Wins

  • depth: one preservation operation
  • locality: salvage beside cleanup
  • leverage: Run and Spec run
  • Tests cross the ownership seam

Activity

  1. added
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    architect-planAn Architect plan: the Spec or Ticket an Architecture review published
    in-progressA Claim: thirdshift has taken this issue
    and removed
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    on Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    architect-planAn Architect plan: the Spec or Ticket an Architecture review published

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions