Deepen Recovery so each layer adds its facts and Recovery owns the rules for its lines

[JacobStephens2]

Summary

Recovery's fields are now private. Each layer of a Failed run adds the fact it knows, and Recovery decides how that fact changes the lines.

 Delivery::run → fail(outside, stopped_at, worktree, log, error)
-  Recovery::new(stage, preservation)            # then patched: recovery.worktree = path
+  Recovery::new(stage, preservation).record_worktree(worktree)
 spec_run::run_through
-  progress.reviewed = !matches!(recovery.stage, Tickets | SpecReview) && all_accounted
-  recovery.tickets = Some(progress); recovery.local_child_work = local_work
+  recovery.add_tickets(progress, all_accounted)   # Recovery derives "final review complete"
+  recovery.add_child_work(local_work)             # extends, never replaces
 run::start
-  if preservation == None && Continuation { preservation = Remote }; recovery.continuation = ..
+  recovery.add_command_context(context, is_continuation)   # Recovery applies "preserved remotely"
 run::run_to_end
-  recovery.local_child_work.extend_from_slice(base_fix.local_work())
+  recovery.add_child_work(base_fix.local_work())

• TicketProgress no longer has a reviewed field. The Ticket board reports only landed and total.
• GLOSSARY.md gains a Recovery entry, and the Failed run entry links to it.
• docs/code-navigation.md gains a Recovery row.

Evidence

• Before: src/recovery.rs had no tests. Its rules were tested only end to end, through the Spec run and Run notification fakes.
• After: five new unit tests in recovery::tests, each asserting only on lines():
  • a Continuation with nothing pushed reads preserved remotely, with the Continuing line;
  • a fresh Run with nothing pushed reads no work preserved, with the Retry line;
  • the final review reads complete only past the Spec review, with every Ticket accounted for;
  • retained work added by two layers gives both Retained work lines in order, plus Before retrying;
  • local preservation names the recorded worktree, or the Launch directory when none is recorded.

Test runs:

• Full cargo test on the first commit: every suite passed (lib: 1108 passed, 2 ignored).
• After the review fixes, these pass:
  • cargo test --bin thirdshift -- recovery:: run_ending:: delivery::tests spec_run:: run::: 136 passed.
  • tests/failed_run.rs, tests/spec_run.rs, tests/run_notification.rs and tests/continuation.rs: all passed.
• No file under tests/ changed, so the end-to-end tests check that the lines are unchanged.

Merge Danger

Door: two-way

It is an in-process refactor with no change in behaviour, and it is easy to revert.

Blast Radius: small

It affects the lines a Failed run prints: in the terminal, the Activity log, the Command record and the Run notification. The unchanged end-to-end tests cover each of them.

Review coverage

Both reviewers read every changed file. No generated output was excluded.

Unaddressed findings

Standards

• The Recovery::stage() getter exists only for tests (judgement call).
  • Declined because the spec's Decisions list "read the stage" as one of Recovery's methods. It stays #[cfg(test)], because no production code reads it any more.
  • Delivery's tests compare Stage values (src/delivery.rs, fn stopped). Rewriting them to match Stopped at: text would only couple them to the line format.
• Possible Primitive Obsession in the bool flags on add_tickets and add_command_context (smell).
  • Declined because the spec's Decisions define each method as taking the fact "together with whether every Ticket was accounted for" or "whether the Issue branch was already on origin".
  • Each flag is named in its doc comment. In src/spec_run.rs and src/run.rs the flag is passed as a named expression; only the hand-built run_ending.rs test passes literals.
• record_worktree breaks the add_* naming (smell).
  • Declined because the worktree is set once, not accumulated, so add_ would misdescribe it. The spec's Decisions word it as "record the worktree".
• fail takes the worktree path as an extra parameter (weak smell).
  • Declined because the spec's Decisions say "The Delivery's fail is given the worktree path." Getting it from FailedOutside instead would add a method to every test double for no change in behaviour.

Spec

• stage() is test-only rather than part of the production interface (noted as a deviation, not a defect).
  • Declined because of the acceptance criterion "The Spec run no longer reads Recovery's stage". No production caller remains, and clippy's dead_code would reject an unused public method.

None of these needs a Day shift decision, so no needs-triage issue was filed.

Closes #867

🤖 Generated with Claude Code

Built with claude · claude-opus-5-5 · medium

Security review outcome

No unaddressed introduced findings.