[JacobStephens2]

Summary

A failure report now gives the Session log of the session its cause came from, not just the last session that ran. The terminal report, the Command log's closing session log: line and the Run notification all read FailedRun.log, so they agree.

 Delivery
   implement session
   Security review                    → Outcome { ended, log: security-review.jsonl }
   Repair loop                        → repair-1.jsonl …
   Security hold raised (Merge run)
   fail(log = most recent)
-    report log = most recent              (repair-2.jsonl, as in #776)
+    report log = hold.log() or most recent (security-review.jsonl)

 Spec run, Tickets not done
-  report log = none
+  report log = the one failed Ticket's log; none if several failed

Here is the rule for each cause. The guide documents it under Logs.

Cause	Session log reported
A failed implement session, Spec review or Repair	that session's own log (it is the most recent)
A Security hold: a refused, incomplete or finding-holding review	the Security review's log (or its Resume's), even after later Repairs
A Spec run whose Tickets are not done	the one failed Ticket's log; none if several failed
No session: Repairs used up, a Declined CI fix, the upstream-move budget, a Harness check, git/GitHub errors	the most recent Session log

Sessions::last_log_of(kind) returns the last session's log only if that session was of kind. A Security review that started no session therefore carries no log, and its hold falls back to the most recent one.

Evidence

• Before: the new tests fail with the hold's log choice disabled:

  • security_review::a_refused_security_review_followed_by_a_repair_reports_the_reviews_session_log panics because the reported log is …-repair-1.jsonl.
  • delivery::tests::a_security_hold_after_a_repair_reports_the_reviews_session_log fails.
  • On main, spec_run::tests::a_spec_run_failed_by_one_tickets_session_names_that_sessions_log fails with left: None, right: Some("/logs/session.jsonl").

  After: all of these pass. The full cargo test run passes (2204 tests), and so do cargo clippy --all-targets -- -D warnings and cargo fmt --check.

The acceptance criteria are covered by these tests:

refused Security review → Repair → hold      stderr "session log:" and email "Session log:" both …-security-review.jsonl
failing Repair after a Security review       stderr and email both …-repair-1.jsonl
no-session cause (Repairs exhausted)         most recent log (delivery unit tests)
Spec run: one failed / failed + blocked / two failed Tickets   that Ticket's log / that Ticket's log / none

Merge Danger

Door: two-way. This changes which path is reported. Nothing is stored or published differently, and the notification format is unchanged.

Blast Radius: reporting. It touches the Failed run's Session log line on stderr, in the Command log and in the Run notification. A Spec run failed by Tickets now prints a session log: line it did not print before.

Unaddressed findings

Standards

All of these are judgement-call smells. None says behaviour is wrong.

• The tuple returned by Outside::security_review. It follows the existing (Result<T>, Option<PathBuf>) shape of Sessions::within/checked_within (src/session.rs:141, :156). The Err must stay inspectable for UntrustedEdit before it becomes an Outcome (src/delivery.rs:231).
• Three checks for SecurityHold in fail/stopped_at. They answer different questions: whether to keep the PR ready (src/delivery.rs:372), which log to report (:374) and where the Run stopped (:414). Merging them would hide that.
• The test calling the private fail directly. This follows the precedent failed_run_finishing_errors_warn_and_preserve_the_primary_error_and_log (src/delivery.rs:1144). The routed cases are tested through deliver and end to end.
• last_log_of repeating the exists() filter. It is read in the middle of the steps, before checked_within's filter runs at the end (src/session.rs:178), so it has to filter for itself (:329).
• last_log_of has no direct unit test. tests/security_review.rs:621 exercises it end to end, and that test goes red without the fix.
• Outcome.log exists only to feed Hold. The issue's Key interfaces ask for exactly this: "Where a Security review's outcome becomes a hold (the Security review Outcome): it needs to carry its Session log with it."
• Landing carrying several board facts. That struct shape predates this change (src/spec_run.rs:368). This PR adds one field to it and does not restructure it.
• A String log becoming a PathBuf. The cause is Ended::Failed { log: Option<String> } (src/child_run.rs:382), which predates this change and is parsed from a child Run's output. Changing it is outside A failure report links the most recent session's log, not the one that caused the failure #911.
• TicketBoard::failed_log duplicating the query in lines(). The predicates differ. lines() matches only failures with a log (src/spec_run/ticket_board.rs:178). failed_log (:140) must also count failures without a log, so that two failures never name one.

Spec

None. Findings (a)1, (a)2 and the follow-up's interrupt filter were all addressed.

Security review outcome

No unaddressed introduced findings.

Review coverage

Both axes read every changed file. There were no generated-output exclusions. Commit 0632e12 came after both review passes. It edits only src/spec_run.rs, which both axes read in their follow-up, to resolve the follow-up findings.

Closes #911

🤖 Generated with Claude Code

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