[JacobStephens2]

Summary

The Delivery now runs its steps and the Failed run path in its own code, through two private seams. Each seam has a production adapter and a scripted fake.

 Delivery::deliver
-  worktree.fast_forward_to_origin()          # outside the sessions' scope
   Sessions::within
-    sessions.run / worktree.push / mark_pr_ready(gh) / take_to_goal / after_merge(gh, git)
+    Route::steps(InWorktree: Outside)         # catch-up, session, push, before_ready,
+                                              # mark ready, Repair loop, after the merge
-  failed_run::fail(issue, worktree, ...)     # in src/failed_run.rs
+  fail(OnFailure: FailedOutside)             # owns the worktree, so it can keep it

src/
├── delivery.rs     # steps + Failed run path, Outside / FailedOutside, InWorktree / OnFailure, Scripted fake + tests
└── failed_run.rs   # only FailedRun, PolicyRefusal, From<anyhow::Error> (+ interrupted_or, shared with it)

• Interrupts reach these rules as a seam question (interrupted()), not as the process-wide flag. That covers both "an interrupt wins over the error" and "a step after the merge is retried if interrupted". interrupt::retry_if_interrupted is gone.
• The Repair loop is a single step behind the seam. Its own seam and tests are unchanged.
• Worktree::keep now takes &mut self, so the adapter that owns the worktree can keep it.
• End-to-end tests that checked only one rule are replaced by unit tests. One test per wiring path stays. The two "when and where" failure-commit tests are merged into one, and the SIGTERM/SIGHUP variants are dropped: interrupt.rs unit-tests all three signals.

Evidence

• Before: marking ready, the steps after the merge and the Failed run path had no unit tests. They were reached only by driving whole Runs against the fake gh, git and claude.
  After: 22 new in-process tests in delivery::tests cover:

  • the order of the steps;
  • marking ready;
  • the steps after a merge, including the retry when interrupted;
  • each Failed run rule.

  cargo test passes in full, and cargo clippy --all-targets and cargo fmt --check are clean.

a_failure_at_each_step_goes_down_the_failed_run_path_with_the_right_session_log
  for step in [catch-up, session, push, before-ready, read PR, mark ready, Repair loop]
    fail it → FailedRun with that error, log None for catch-up (no session), else the session's
            → failure commit for that reason, no steps after a merge

Merge Danger

Door: two-way

This moves code inside the binary. Nothing persisted and no interface changes.

Blast Radius: Delivery

Every Run and Spec PR goes through this path. The progress lines, warnings, requests and git effects are meant to be byte-identical. One difference is that the Factory skills plugin is now written before a Spec PR's catch-up rather than after it.

Unaddressed findings

Standards

• retry_if_interrupted duplicated in claim.rs and delivery.rs: each is a few lines over its own module's seam, as the Claim seam set the pattern. A cross-module trait to share them would couple two private seams.
• Outside and FailedOutside share pull_request / interrupted / step; suggested a supertrait: the spec chose two seams on purpose. A supertrait adds a third abstraction to save three one-line methods.
• Data clump (issue, base, branch) across Route / InWorktree / OnFailure / RunOutside: each adapter needs a different subset and different ownership (borrowed vs. owned worktree), so bundling them would not remove any parameters.
• before_ready: FnOnce(&mut O) shaped by tests: passing the outside lets the fake record the step in order with the others. The public deliver keeps its FnOnce() parameter.
• Adapter names (InWorktree, OnFailure, Scripted) vs. OnGitHub / InMemory: Scripted matches the Repair loop's fake. The adapter names say where each runs: within the worktree's sessions, and on a failure.
• interrupted_or made pub in failed_run.rs: the From conversion the spec keeps there uses it too. Sharing it keeps one definition of "an interrupt wins".
• Only SIGINT stays end to end: interrupt.rs's unit test still records SIGINT, SIGTERM and SIGHUP.
• Worktree::keep takes &mut self: needed so the Failed run adapter can own the worktree, as the spec asks.

Spec

• interrupted_or left in the Failed run module: the conversion from a plain error, which the spec keeps there, depends on it.
• Some merge_run.rs end-to-end tests may check a single rule (closing into develop, Ctrl-C after the merge, branch already deleted by GitHub): each checks wiring the unit tests can't. They are the non-default base on the real merge, the real signal flag through the adapter, and the adapter's own already-deleted check.
• The plugin is now written before the catch-up: the spec asks for the catch-up to move inside the sessions' scope. The only difference is in which failure is reported when writing the plugin fails.

Closes #384

🤖 Generated with Claude Code