From 3e0e0ec08bc544a0b21207fe367eed45143a81ee Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 21 Oct 2026 08:09:39 -0400 Subject: [PATCH 1/4] Report a Security hold with its review's Session log, not the latest one A Security hold is raised only after the Repair loop, whose sessions may run after the review, so the failure report, the Command log's closing line and the Run notification named a later Repair's log. The review's Outcome now carries its Session log and the Failed run path reports it for a hold. A cause from no session keeps the most recent Session log, as the guide now says. Co-Authored-By: Claude Opus 5.5 --- docs/guide.md | 4 +- src/delivery.rs | 116 ++++++++++++++++++++++++++++------- src/failed_run.rs | 3 +- src/run_ending.rs | 3 +- src/security/review.rs | 56 +++++++++++------ src/security/review/tests.rs | 4 +- src/session.rs | 19 ++++-- tests/security_review.rs | 71 +++++++++++++++++++++ 8 files changed, 225 insertions(+), 51 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index 0967d8ed..022991db 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -609,7 +609,7 @@ The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only A Run that asks for a notification, by the flag or by `email.always`, makes the same checks as `email-test` before any other work: an address is known, and a key is found, in `RESEND_API_KEY` or else the Credentials. If either fails, or the Credentials are broken, the Run stops, exits `1` naming what's wrong (with no key, the message above listing every way to give one), and sends nothing. A Run that asks for no notification never reads the Credentials, so a broken file can't stop it, and `update`, `version` and `help` never read it either. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a [Failed run](#failed-runs) (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal. - **Subject**: `[thirdshift] /# : `, where the outcome is `ready for review`, `merged`, `failed` or `interrupted`. The title is left out if it can't be read from GitHub. -- **Body**, plain text: a `Result:` line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; immediately followed by one `Issue:` line with the full original Issue URL, including after failure or interruption with no pull request, and a `Title:` line with the issue title when it was read; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the [Command log](#logs) path as a `Command log:` line, a `Sent by: thirdshift` line, the hostname and how long the Run took. The body tells everything the subject does, so copying the body alone is enough. +- **Body**, plain text: a `Result:` line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; immediately followed by one `Issue:` line with the full original Issue URL, including after failure or interruption with no pull request, and a `Title:` line with the issue title when it was read; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the same one stderr gives, the [Command log](#logs) path as a `Command log:` line, a `Sent by: thirdshift` line, the hostname and how long the Run took. The body tells everything the subject does, so copying the body alone is enough. The email's `Built with` line names the command's configured Harness, Model and Effort and is marked `(configured)`. A `Session models` section lists sessions whose Harness was launched, including failed or interrupted sessions, Resumes and child Runs belonging to that Command. Models evidenced by the Harness's output or retained records are marked `(reported)`, including model switches and delegated Claude answers. When the Harness supplies no model evidence, the requested Model is labelled `(requested; actual model not reported)`, or its default is labelled `Harness default (actual model not reported)`. A listed session proves the Harness was launched; an unconfirmed request does not prove that Model executed. Effort is the session's configured Effort; it does not describe a delegated answer's Effort. Attribution records (`*.models.json`) are kept beside the Session logs; a failure to write or read them is a warning and never changes the Run's outcome. @@ -679,7 +679,7 @@ A Resume is logged as its session's kind plus `-resume`, e.g. `implement-resume. A **Command log** is everything one command printed, stderr and stdout in the order printed, while the terminal still shows all of it. Its folder is the command typed: `thirdshift `, a Run or a Spec run, in `issue/`, `thirdshift pickup` in `pickup/`, named for the issue it took, and `thirdshift architect` in `architect/`. A Spec run's covers its Tickets' Runs, a Run's covers its [Base fix](#base-fix), and a Pass's covers the Spec run or Run it dispatched: none of those keeps one of its own. So `ls -t ~/.thirdshift/logs/acme/widgets/commands/pickup/*.log | head` lists the recent passes on acme/widgets that took an issue. A command keeps its Command log once it starts work: a Pass skipped before doing any work keeps none, nor does a Run that fails before it starts work, as on the [Origin match](#what-a-run-does), and `setup`, `email-test`, `update`, `version` and `help` never keep one. Lines printed before the Command log's name is known, such as a Pickup run's lines on the issues it [passed over](#why-an-issue-was-passed-over) before it took one, are written first, and once it is created a progress line says `logging this command to `. A Command log that can't be written produces one `warning:` line on stderr. A failure confined to the Command log leaves the outcome, stdout and exit code unchanged; if the shared folder also prevents writing a Session log, the Run fails before starting that session. -The stamp is the local time the command started, with its UTC offset, as in `20261003T120000-0400`, in the machine's time zone, or `TZ`'s if set, so it agrees with `date` and `ls -l`. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of its most recent Session log, the place to start looking, then that of its Command log. +The stamp is the local time the command started, with its UTC offset, as in `20261003T120000-0400`, in the machine's time zone, or `TZ`'s if set, so it agrees with `date` and `ls -l`. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of the Session log of the session its cause came from, the place to start looking, then that of its Command log. A failed implement session, Spec review or Repair names its own log, as does a [Security review](#security-review) whose hold is reported only after later Repairs. A cause that came from no session, such as red CI with the Repairs used up, a Declined CI fix, the upstream-move budget, a Harness check or a git or GitHub error, names the most recent Session log. The Command log's closing `session log:` line and the [Run notification](#run-notifications) give the same path. New Command and Session logs insert `~` immediately after the stamp. The random token gives each Command its own storage even when start times agree; its Command record uses the exact Command-log stem followed by `.command.json`. Model sidecars (`.models.json`) and review reports (`.standards` and `.spec`) use the exact Session-log stem. Human Command names stay the same, and start stamps remain display and grouping facts. A matching stamp alone does not attach a tokenless Session to a new Command: a record reference or exact Model evidence must establish that ownership. Legacy logs and records remain readable without renaming. A failed Command-log writer retains its allocated owner; if allocation itself failed, a Session fails with that cause instead of choosing a shared timestamp-only filename. diff --git a/src/delivery.rs b/src/delivery.rs index 605d7c77..18f163d2 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -228,8 +228,8 @@ impl Route<'_> { outside.security_review(&prompt::security_review(self.issue, self.base, self.branch)) }) { // Not a Security review outcome: the Run itself must stop. - Some(Err(error)) if error.is::() => return Err(error), - review => review.map(SecurityOutcome::of), + Some((Err(error), _)) if error.is::() => return Err(error), + review => review.map(|(result, log)| SecurityOutcome::of(result, log)), }; outside.stage(Stage::Push); outside.push()?; @@ -351,8 +351,11 @@ fn retry_if_interrupted( /// head whose CI was watched. Problems along the way are reported, not /// raised, so the error is what the Run fails with. Worktree owns preservation /// and retains work that may not have reached origin. `log` is the most recent -/// Session log, if a session created one; `stopped_at`, the stage its -/// recovery lines report; `worktree`, where work preserved only locally is. +/// Session log, if a session created one, which the Failed run reports unless +/// the error is a Security hold with its review's own Session log: a hold is +/// raised only after the Repair loop, whose sessions may have run since. +/// `stopped_at` is the stage its recovery lines report; `worktree`, where work +/// preserved only locally is. fn fail( outside: &mut impl FailedOutside, stopped_at: recovery::Stage, @@ -367,6 +370,10 @@ fn fail( let reason = Cause::of(&error).first_line().to_string(); // Everything is already on origin: the Repair loop pushed the head. let keep_ready = error.is::() || error.is::(); + let log = error + .downcast_ref::() + .and_then(|hold| hold.log.clone()) + .or(log); let preservation = if keep_ready { Preservation::Remote } else { @@ -419,7 +426,9 @@ trait Outside { fn catch_up(&mut self) -> Result<()>; /// Run a session of `kind` given `prompt`. fn session(&mut self, kind: Kind, prompt: &str) -> Result<()>; - fn security_review(&mut self, prompt: &str) -> Result; + /// Run the Security review given `prompt`, with its Session log, its + /// Resume's if it got one, if it started a session. + fn security_review(&mut self, prompt: &str) -> (Result, Option); fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()>; /// Push the Issue branch. fn push(&mut self) -> Result<()>; @@ -533,17 +542,20 @@ impl Outside for InWorktree<'_> { self.pull_request.ensure_actionable(false) } - fn security_review(&mut self, prompt: &str) -> Result { - self.pull_request.ensure_actionable(false)?; - let review = crate::security::review::run( - self.sessions, - self.worktree, - self.issue, - self.base, - prompt, - )?; - self.pull_request.ensure_actionable(false)?; - Ok(review) + fn security_review(&mut self, prompt: &str) -> (Result, Option) { + let review = (|| { + self.pull_request.ensure_actionable(false)?; + let review = crate::security::review::run( + self.sessions, + self.worktree, + self.issue, + self.base, + prompt, + )?; + self.pull_request.ensure_actionable(false)?; + Ok(review) + })(); + (review, self.sessions.last_log_of(Kind::SecurityReview)) } fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> { @@ -827,6 +839,8 @@ mod tests { /// review ends incomplete, a Security hold. reviews_security: bool, security_held: bool, + /// Whether taking the PR to its goal runs a Repair session first. + repairs: bool, failing: Option, interrupted: bool, refused: bool, @@ -850,13 +864,17 @@ mod tests { fn stage(&mut self, stage: Stage) { self.stages.push(stage); } - fn security_review(&mut self, _: &str) -> Result { - if self.security_held { - return Review::from_final_message(Some("Security review: incomplete: timed out")); - } - Review::from_final_message(Some( - "Security review: {\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}", - )) + fn security_review(&mut self, _: &str) -> (Result, Option) { + let log = PathBuf::from("/logs/security-review.jsonl"); + self.log = Some(log.clone()); + let review = if self.security_held { + Review::from_final_message(Some("Security review: incomplete: timed out")) + } else { + Review::from_final_message(Some( + "Security review: {\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}", + )) + }; + (review, Some(log)) } fn catch_up(&mut self) -> Result<()> { self.calls.push(Call::CatchUp); @@ -889,6 +907,9 @@ mod tests { } fn take_to_goal(&mut self, _: &str, goal: Goal) -> Result<()> { self.calls.push(Call::Goal(goal)); + if self.repairs { + self.log = Some(PathBuf::from("/logs/repair-1.jsonl")); + } if self.refused { return Err(anyhow!("merge refused").context(PolicyRefusal::NothingToFix)); } @@ -1232,4 +1253,53 @@ mod tests { ] ); } + + #[test] + fn a_security_hold_after_a_repair_reports_the_reviews_session_log() { + let mut outside = Scripted { + reviews_security: true, + security_held: true, + repairs: true, + ..Default::default() + }; + let failed = deliver(&mut outside, Goal::Merged, false).unwrap_err(); + assert!(failed.error.is::(), "{:#}", failed.error); + assert_eq!(outside.log, Some(PathBuf::from("/logs/repair-1.jsonl"))); + assert_eq!( + failed.log, + Some(PathBuf::from("/logs/security-review.jsonl")) + ); + } + + #[test] + fn a_failure_after_a_repair_reports_the_most_recent_session_log() { + // A failing Repair session, or a cause no session gave, as with + // Repairs exhausted, is reported with the most recent Session log, + // whether or not a Security review ran before it. + for reviews_security in [false, true] { + let mut outside = Scripted { + reviews_security, + repairs: true, + failing: Some(Fails::Goal), + ..Default::default() + }; + let failed = deliver(&mut outside, Goal::Merged, false).unwrap_err(); + assert_eq!(failed.log, Some(PathBuf::from("/logs/repair-1.jsonl"))); + } + } + + #[test] + fn a_security_hold_whose_review_started_no_session_reports_the_most_recent_session_log() { + let hold = SecurityOutcome::of(Err(anyhow!("no merge base")), None) + .hold() + .unwrap(); + let failed = fail( + &mut Scripted::default(), + recovery::Stage::SecurityReview, + PathBuf::from("/worktree"), + Some(PathBuf::from("/logs/repair-1.jsonl")), + hold.into(), + ); + assert_eq!(failed.log, Some(PathBuf::from("/logs/repair-1.jsonl"))); + } } diff --git a/src/failed_run.rs b/src/failed_run.rs index 36da6fd4..32955dcd 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -14,7 +14,8 @@ pub struct FailedRun { pub error: anyhow::Error, /// The Issue branch's open PR, if it has one. pub pr_url: Option, - /// The most recent session log, if a session was started. + /// The Session log of the session the failure came from, else the most + /// recent one, if a session was started. pub log: Option, /// Whether the Run was interrupted, as it was when the Run failed. pub interrupted: bool, diff --git a/src/run_ending.rs b/src/run_ending.rs index bf9b0f96..cff713e9 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -122,7 +122,8 @@ pub struct Account<'a> { pub advice: &'a [Advice], /// What became of the Base fix the Run started or waited on, if any. pub base_fix: Option<&'a str>, - /// The most recent session log, if a session was started. + /// The Session log of the session a failure came from, else the most + /// recent one, if a session was started. pub log: Option<&'a Path>, /// In a Spec run, a line on each Ticket, as in its summary on stderr. pub ticket_lines: &'a [String], diff --git a/src/security/review.rs b/src/security/review.rs index 596a7bc3..8a00b3b4 100644 --- a/src/security/review.rs +++ b/src/security/review.rs @@ -3,6 +3,7 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; +use std::path::PathBuf; use std::{fmt, fs}; use super::reproduction::{FixSize, Outcome as ReproductionOutcome, Reproduction, Severity}; @@ -165,16 +166,25 @@ pub(crate) enum Ask { Forbid, } -pub(crate) enum Outcome { +/// How a Security review ended, with the Session log of the session it ran, +/// which any Security hold it raises is reported with. +pub(crate) struct Outcome { + ended: Ended, + log: Option, +} + +enum Ended { Complete(Review), Incomplete(String), } impl Outcome { - pub fn of(result: Result) -> Self { - match result { - Ok(review) => Self::Complete(review), - Err(error) => Self::Incomplete( + /// The review that came to `result`, its session logged to `log`, its + /// Resume's if it got one, if it started one. + pub fn of(result: Result, log: Option) -> Self { + let ended = match result { + Ok(review) => Ended::Complete(review), + Err(error) => Ended::Incomplete( if let Some(refusal) = error.downcast_ref::() { format!("Security review refused: {refusal}") } else if let Some(incomplete) = error.downcast_ref::() { @@ -183,43 +193,53 @@ impl Outcome { "Security review incomplete: session failed, ended early or omitted a valid final line; see Session log".to_string() }, ), - } + }; + Self { ended, log } } pub fn hold(&self) -> Option { - match self { - Self::Complete(review) if review.findings.is_empty() => None, - Self::Complete(review) => Some(Hold(format!( + let reason = match &self.ended { + Ended::Complete(review) if review.findings.is_empty() => return None, + Ended::Complete(review) => format!( "Security review left {} unaddressed introduced finding(s): {}", review.findings.len(), review.findings.join("; ") - ))), - Self::Incomplete(reason) => Some(Hold(reason.clone())), - } + ), + Ended::Incomplete(reason) => reason.clone(), + }; + Some(Hold { + reason, + log: self.log.clone(), + }) } pub fn entry(&self) -> String { - match self { - Self::Complete(review) if review.findings.is_empty() => { + match &self.ended { + Ended::Complete(review) if review.findings.is_empty() => { "No unaddressed introduced findings.".to_string() } - Self::Complete(review) => review + Ended::Complete(review) => review .findings .iter() .map(|title| format!("- {title}\n")) .collect(), - Self::Incomplete(reason) => format!("- {reason}\n"), + Ended::Incomplete(reason) => format!("- {reason}\n"), } } } /// Delivery has pushed and marked the PR ready before raising this hold. +/// It is reported with `log`, the Security review's Session log, however +/// many sessions ran after the review. #[derive(Debug)] -pub(crate) struct Hold(pub String); +pub(crate) struct Hold { + reason: String, + pub log: Option, +} impl fmt::Display for Hold { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) + f.write_str(&self.reason) } } diff --git a/src/security/review/tests.rs b/src/security/review/tests.rs index 14a8c473..2efe5e49 100644 --- a/src/security/review/tests.rs +++ b/src/security/review/tests.rs @@ -19,11 +19,11 @@ fn malformed_final_messages_produce_a_public_incomplete_hold() { ] { let result = Review::from_final_message(Some(final_message)); assert!(result.is_err(), "accepted {final_message:?}"); - let outcome = Outcome::of(result); + let outcome = Outcome::of(result, None); let expected = "Security review incomplete: session failed, ended early or omitted a valid final line; see Session log"; assert_eq!(outcome.hold().unwrap().to_string(), expected); assert_eq!(outcome.entry(), format!("- {expected}\n")); } - let missing = Outcome::of(Review::from_final_message(None)); + let missing = Outcome::of(Review::from_final_message(None), None); assert!(missing.hold().is_some()); } diff --git a/src/session.rs b/src/session.rs index ea403cd5..23678e5f 100644 --- a/src/session.rs +++ b/src/session.rs @@ -116,8 +116,8 @@ pub struct Sessions<'a> { /// What must still hold before each session starts, a Resume included: /// a session it fails is not started, and the step fails with it. before_each: &'a dyn Fn() -> Result<()>, - /// The log of the session started last, a Resume included. - last_log: RefCell>, + /// The session started last, a Resume included, and its log. + last_log: RefCell>, /// How each session ended whose last ending, its Resume's if it got one, /// left background work running, as in "implement session ended with a /// background task still running (cargo test), which was killed", oldest @@ -197,7 +197,7 @@ impl<'a> Sessions<'a> { endings_with_killed_work: RefCell::default(), }; let taken = steps(&sessions); - let log = sessions.last_log.into_inner(); + let log = sessions.last_log.into_inner().map(|(_, log)| log); let endings = sessions.endings_with_killed_work.into_inner(); if endings.is_empty() { return (taken, log); @@ -306,7 +306,7 @@ impl<'a> Sessions<'a> { logs::session(session, &log); let label = &session.token(); self.step(format!("logging the session to {}", log.display())); - *self.last_log.borrow_mut() = Some(log.clone()); + *self.last_log.borrow_mut() = Some((session, log.clone())); let prompt = reports.map_or_else( || prompt.replace(prompt::REVIEW_REPORTS, ""), |reports| reports.prompt(prompt), @@ -323,6 +323,17 @@ impl<'a> Sessions<'a> { ended } + /// The log of the session started last, its Resume's if it got one, if + /// that session is of `kind` and the log exists: the log a cause from + /// that session is reported with. + pub fn last_log_of(&self, kind: Kind) -> Option { + self.last_log + .borrow() + .as_ref() + .filter(|(session, log)| session.kind == kind && log.exists()) + .map(|(_, log)| log.clone()) + } + /// Hand on the progress line `line`. fn step(&self, line: String) { self.outside.borrow_mut().step(line); diff --git a/tests/security_review.rs b/tests/security_review.rs index cf13c4cb..a73d4bc2 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -617,6 +617,77 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] assert_eq!(state["prs"][0]["isDraft"], false); } +#[test] +fn a_refused_security_review_followed_by_a_repair_reports_the_reviews_session_log() { + let scenario = Scenario::new(); + scenario.agent_does_in_session( + 1, + &format!( + "{OPENS_PR}\n{}", + r#" +gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"failure"}]' +gh fake checks "$(git rev-parse origin/main)" '[{"name":"test","conclusion":"success"}]' +"# + ), + ); + scenario.agent_does_in_session( + 2, + r#"printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, + ); + scenario.agent_does_in_session( + 3, + r#" +echo fixed > ci-fixed.txt +git add ci-fixed.txt +git commit -q -m 'Fix branch CI' +gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}]' +"#, + ); + let resend = ResendStandIn::replying(200, r#"{"id":"sent"}"#); + let result = scenario.run_with_env( + &[ + &scenario.issue_url(7), + "security-review", + "merge", + "email", + "you@example.com", + ], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "re_test"), + ], + ); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("Security review refused"), + "{}", + result.stderr + ); + // The Repair ran after the review, so it is the most recent session. + assert!( + result.stderr.contains("-repair-1.jsonl"), + "{}", + result.stderr + ); + let reported = result + .stderr + .lines() + .find_map(|line| line.split_once("session log: ").map(|(_, log)| log)) + .unwrap_or_else(|| panic!("no session log line: {}", result.stderr)); + assert!( + reported.ends_with("-security-review.jsonl"), + "{}", + result.stderr + ); + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + let text = requests[0].body["text"].as_str().unwrap(); + assert!( + text.contains(&format!("Session log: {reported}\n")), + "{text}" + ); +} + // Regression B. #[test] fn review_regression_b_body_write_failure_preserves_refusal_and_readiness() { From 729b2bbd98dd45668d7afee093e4334e6de3d10a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 21 Oct 2026 08:23:57 -0400 Subject: [PATCH 2/4] Name the failed Ticket's Session log, and keep a Security hold's log private A Spec run whose Tickets are not done reported no Session log, though one Ticket's session can be the whole cause. It now names that Ticket's log when just one Ticket's Run failed, and none when several did, as the guide says. A failing Repair after a Security review is tested end to end, the Security hold's log is read through an accessor, and the review's actionable checks move to a named helper. Co-Authored-By: Claude Opus 5.5 --- docs/guide.md | 2 +- src/delivery.rs | 33 +++++++++++++++----------- src/security/review.rs | 11 +++++++-- src/spec_run.rs | 39 +++++++++++++++++++++++++++++-- src/spec_run/ticket_board.rs | 20 ++++++++++++++++ tests/security_review.rs | 45 ++++++++++++++++++++++++++++++++++++ 6 files changed, 132 insertions(+), 18 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index 022991db..adb1267c 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -679,7 +679,7 @@ A Resume is logged as its session's kind plus `-resume`, e.g. `implement-resume. A **Command log** is everything one command printed, stderr and stdout in the order printed, while the terminal still shows all of it. Its folder is the command typed: `thirdshift `, a Run or a Spec run, in `issue/`, `thirdshift pickup` in `pickup/`, named for the issue it took, and `thirdshift architect` in `architect/`. A Spec run's covers its Tickets' Runs, a Run's covers its [Base fix](#base-fix), and a Pass's covers the Spec run or Run it dispatched: none of those keeps one of its own. So `ls -t ~/.thirdshift/logs/acme/widgets/commands/pickup/*.log | head` lists the recent passes on acme/widgets that took an issue. A command keeps its Command log once it starts work: a Pass skipped before doing any work keeps none, nor does a Run that fails before it starts work, as on the [Origin match](#what-a-run-does), and `setup`, `email-test`, `update`, `version` and `help` never keep one. Lines printed before the Command log's name is known, such as a Pickup run's lines on the issues it [passed over](#why-an-issue-was-passed-over) before it took one, are written first, and once it is created a progress line says `logging this command to `. A Command log that can't be written produces one `warning:` line on stderr. A failure confined to the Command log leaves the outcome, stdout and exit code unchanged; if the shared folder also prevents writing a Session log, the Run fails before starting that session. -The stamp is the local time the command started, with its UTC offset, as in `20261003T120000-0400`, in the machine's time zone, or `TZ`'s if set, so it agrees with `date` and `ls -l`. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of the Session log of the session its cause came from, the place to start looking, then that of its Command log. A failed implement session, Spec review or Repair names its own log, as does a [Security review](#security-review) whose hold is reported only after later Repairs. A cause that came from no session, such as red CI with the Repairs used up, a Declined CI fix, the upstream-move budget, a Harness check or a git or GitHub error, names the most recent Session log. The Command log's closing `session log:` line and the [Run notification](#run-notifications) give the same path. +The stamp is the local time the command started, with its UTC offset, as in `20261003T120000-0400`, in the machine's time zone, or `TZ`'s if set, so it agrees with `date` and `ls -l`. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of the Session log of the session its cause came from, the place to start looking, then that of its Command log. A failed implement session, Spec review or Repair names its own log, as does a [Security review](#security-review) whose hold is reported only after later Repairs. A cause that came from no session, such as red CI with the Repairs used up, a Declined CI fix, the upstream-move budget, a Harness check or a git or GitHub error, names the most recent Session log. A Spec run whose Tickets are not done names the Session log of the one Ticket whose Run failed, and none if several did, as no single session caused it; each failed Ticket's line gives its own. The Command log's closing `session log:` line and the [Run notification](#run-notifications) give the same path. New Command and Session logs insert `~` immediately after the stamp. The random token gives each Command its own storage even when start times agree; its Command record uses the exact Command-log stem followed by `.command.json`. Model sidecars (`.models.json`) and review reports (`.standards` and `.spec`) use the exact Session-log stem. Human Command names stay the same, and start stamps remain display and grouping facts. A matching stamp alone does not attach a tokenless Session to a new Command: a record reference or exact Model evidence must establish that ownership. Legacy logs and records remain readable without renaming. A failed Command-log writer retains its allocated owner; if allocation itself failed, a Session fails with that cause instead of choosing a shared timestamp-only filename. diff --git a/src/delivery.rs b/src/delivery.rs index 18f163d2..a08ed500 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -372,7 +372,8 @@ fn fail( let keep_ready = error.is::() || error.is::(); let log = error .downcast_ref::() - .and_then(|hold| hold.log.clone()) + .and_then(SecurityHold::log) + .map(Path::to_path_buf) .or(log); let preservation = if keep_ready { Preservation::Remote @@ -543,18 +544,7 @@ impl Outside for InWorktree<'_> { } fn security_review(&mut self, prompt: &str) -> (Result, Option) { - let review = (|| { - self.pull_request.ensure_actionable(false)?; - let review = crate::security::review::run( - self.sessions, - self.worktree, - self.issue, - self.base, - prompt, - )?; - self.pull_request.ensure_actionable(false)?; - Ok(review) - })(); + let review = self.review_while_actionable(prompt); (review, self.sessions.last_log_of(Kind::SecurityReview)) } @@ -616,6 +606,23 @@ impl Outside for InWorktree<'_> { } } +impl InWorktree<'_> { + /// Run the Security review given `prompt`, failing unless the pull + /// request is still actionable both before and after it. + fn review_while_actionable(&mut self, prompt: &str) -> Result { + self.pull_request.ensure_actionable(false)?; + let review = crate::security::review::run( + self.sessions, + self.worktree, + self.issue, + self.base, + prompt, + )?; + self.pull_request.ensure_actionable(false)?; + Ok(review) + } +} + /// A failed Delivery's worktree and its pull request on GitHub. Worktree /// owns preservation and the cleanup decision when this is dropped. struct OnFailure<'a> { diff --git a/src/security/review.rs b/src/security/review.rs index 8a00b3b4..7a3b6c9d 100644 --- a/src/security/review.rs +++ b/src/security/review.rs @@ -3,7 +3,7 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::{fmt, fs}; use super::reproduction::{FixSize, Outcome as ReproductionOutcome, Reproduction, Severity}; @@ -234,7 +234,14 @@ impl Outcome { #[derive(Debug)] pub(crate) struct Hold { reason: String, - pub log: Option, + log: Option, +} + +impl Hold { + /// The Security review's Session log, if it started a session. + pub fn log(&self) -> Option<&Path> { + self.log.as_deref() + } } impl fmt::Display for Hold { diff --git a/src/spec_run.rs b/src/spec_run.rs index 067efc16..f6dce7ff 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -12,6 +12,7 @@ mod ticket_board; use std::num::NonZeroUsize; +use std::path::PathBuf; use anyhow::{Result, bail}; @@ -122,6 +123,7 @@ fn run_through( ) -> Result { let Landing { lines: ticket_lines, + failed_log, progress, pushed, landed, @@ -142,7 +144,7 @@ fn run_through( Err(FailedRun { error: interrupted_or(error, interrupted), pr_url: outside.spec_pr_url(), - log: None, + log: failed_log.filter(|_| !interrupted), interrupted, ticket_lines: Vec::new(), recovery: Some(Box::new(Recovery::new( @@ -365,6 +367,9 @@ impl Outside for ChildRunsAndGitHub<'_> { /// another error ended it first. struct Landing { lines: Vec, + /// The Session log of the one failed Ticket's session the landing + /// failed on, if just one failed. + failed_log: Option, progress: TicketProgress, pushed: bool, landed: Result, @@ -382,6 +387,7 @@ fn land_tickets( if let Err(error) = outside.push() { return Landing { lines: Vec::new(), + failed_log: None, progress: board.progress(), pushed: false, landed: Err(error), @@ -390,6 +396,7 @@ fn land_tickets( } let landing = run_ready_tickets(outside, &mut board); let lines = board.lines(); + let mut failed_log = None; let landed = landing.and_then(|()| { let not_done = board.not_done(); if not_done.is_empty() { @@ -398,10 +405,12 @@ fn land_tickets( for line in &lines { outside.step(line.clone()); } + failed_log = board.failed_log().map(PathBuf::from); bail!("Tickets not done: {}", not_done.join(", ")); }); Landing { lines, + failed_log, progress: board.progress(), local_work: board.local_work(), pushed: true, @@ -1468,7 +1477,7 @@ mod tests { assert_eq!(format!("{:#}", failed.error), "Tickets not done: #21"); assert_eq!(failed.pr_url.as_deref(), Some(SPEC_PR)); - assert_eq!(failed.log, None); + assert_eq!(failed.log, Some(PathBuf::from("/logs/session.jsonl"))); assert!(!failed.interrupted); assert_eq!( failed.ticket_lines, @@ -1618,4 +1627,30 @@ mod tests { ] ); } + + #[test] + fn a_spec_run_failed_by_one_tickets_session_names_that_sessions_log() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[])], + endings: VecDeque::from([(21, Ok(failed("claude exited 1")))]), + ..Script::default() + }); + let failed = failed_of(spec_run(&mut outside, 1)); + assert_eq!(failed.log, Some(PathBuf::from("/logs/session.jsonl"))); + } + + #[test] + fn a_spec_run_failed_by_several_tickets_sessions_names_no_single_log() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + endings: VecDeque::from([ + (21, Ok(failed("claude exited 1"))), + (22, Ok(failed("claude exited 1"))), + ]), + ..Script::default() + }); + let failed = failed_of(spec_run(&mut outside, 1)); + assert_eq!(format!("{:#}", failed.error), "Tickets not done: #21, #22"); + assert_eq!(failed.log, None); + } } diff --git a/src/spec_run/ticket_board.rs b/src/spec_run/ticket_board.rs index 1e7ff0e9..cbcc0f05 100644 --- a/src/spec_run/ticket_board.rs +++ b/src/spec_run/ticket_board.rs @@ -134,6 +134,26 @@ impl TicketBoard { .collect() } + /// The Session log of the one Ticket not done whose Run failed, if just + /// one did and it named its log: the session the Spec run's failure came + /// from. With several, no single session caused it. + pub(super) fn failed_log(&self) -> Option { + let mut failed = self + .tickets + .iter() + .filter(|ticket| ticket.is_open) + .filter_map( + |ticket| match self.standings.get(&ticket.issue.identity()) { + Some(Standing::Ended(Ended::Failed { log, .. })) => Some(log), + _ => None, + }, + ); + match (failed.next(), failed.next()) { + (Some(log), None) => log.clone(), + _ => None, + } + } + /// The Tickets not done, the open ones, in the order they were read. pub(super) fn not_done(&self) -> Vec { self.tickets diff --git a/tests/security_review.rs b/tests/security_review.rs index a73d4bc2..18d576ed 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -688,6 +688,51 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] ); } +#[test] +fn a_failing_repair_after_a_security_review_reports_the_repairs_session_log() { + let scenario = Scenario::new(); + scenario.agent_does_in_session( + 1, + &format!( + "{OPENS_PR}\n{}", + r#" +gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"failure"}]' +gh fake checks "$(git rev-parse origin/main)" '[{"name":"test","conclusion":"success"}]' +"# + ), + ); + scenario.agent_does_in_session(2, &review_script(&json!([]), &[])); + scenario.agent_does_in_session(3, "exit 3\n"); + let resend = ResendStandIn::replying(200, r#"{"id":"sent"}"#); + let result = scenario.run_with_env( + &[ + &scenario.issue_url(7), + "security-review", + "merge", + "email", + "you@example.com", + ], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "re_test"), + ], + ); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let reported = result + .stderr + .lines() + .find_map(|line| line.split_once("session log: ").map(|(_, log)| log)) + .unwrap_or_else(|| panic!("no session log line: {}", result.stderr)); + assert!(reported.ends_with("-repair-1.jsonl"), "{}", result.stderr); + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + let text = requests[0].body["text"].as_str().unwrap(); + assert!( + text.contains(&format!("Session log: {reported}\n")), + "{text}" + ); +} + // Regression B. #[test] fn review_regression_b_body_write_failure_preserves_refusal_and_readiness() { From 0632e125c3698e07e449beab0f195eede25c2076 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 21 Oct 2026 08:44:48 -0400 Subject: [PATCH 3/4] Name a failed Ticket's Session log in an interrupted Spec run too As a Run does, an interrupted Spec run keeps the Session log it would report. The log is read from the board once the landing ran to its end, not set inside the failure's closure, and a Ticket blocked by the failed one leaves the failed Ticket's log named. Co-Authored-By: Claude Opus 5.5 --- src/spec_run.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/spec_run.rs b/src/spec_run.rs index f6dce7ff..6aea7e75 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -144,7 +144,7 @@ fn run_through( Err(FailedRun { error: interrupted_or(error, interrupted), pr_url: outside.spec_pr_url(), - log: failed_log.filter(|_| !interrupted), + log: failed_log, interrupted, ticket_lines: Vec::new(), recovery: Some(Box::new(Recovery::new( @@ -396,7 +396,12 @@ fn land_tickets( } let landing = run_ready_tickets(outside, &mut board); let lines = board.lines(); - let mut failed_log = None; + // Only a landing that ran to its end failed on its Tickets alone. + let failed_log = landing + .is_ok() + .then(|| board.failed_log()) + .flatten() + .map(PathBuf::from); let landed = landing.and_then(|()| { let not_done = board.not_done(); if not_done.is_empty() { @@ -405,7 +410,6 @@ fn land_tickets( for line in &lines { outside.step(line.clone()); } - failed_log = board.failed_log().map(PathBuf::from); bail!("Tickets not done: {}", not_done.join(", ")); }); Landing { @@ -1639,6 +1643,18 @@ mod tests { assert_eq!(failed.log, Some(PathBuf::from("/logs/session.jsonl"))); } + #[test] + fn a_spec_run_whose_other_ticket_is_blocked_names_the_failed_tickets_log() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[21])], + endings: VecDeque::from([(21, Ok(failed("claude exited 1")))]), + ..Script::default() + }); + let failed = failed_of(spec_run(&mut outside, 1)); + assert_eq!(format!("{:#}", failed.error), "Tickets not done: #21, #22"); + assert_eq!(failed.log, Some(PathBuf::from("/logs/session.jsonl"))); + } + #[test] fn a_spec_run_failed_by_several_tickets_sessions_names_no_single_log() { let mut outside = Scripted::new(Script { From a8ac5d5c84f454211971d4b6ea3a395856165f57 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 21 Oct 2026 09:28:45 -0400 Subject: [PATCH 4/4] Regenerate the site manual from docs/guide.md Co-Authored-By: Claude Opus 5.5 --- site/docs/index.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/docs/index.html b/site/docs/index.html index 7712fc3c..e8006de9 100644 --- a/site/docs/index.html +++ b/site/docs/index.html @@ -561,7 +561,7 @@

A Run that asks for a notification, by the flag or by email.always, makes the same checks as email-test before any other work: an address is known, and a key is found, in RESEND_API_KEY or else the Credentials. If either fails, or the Credentials are broken, the Run stops, exits 1 naming what's wrong (with no key, the message above listing every way to give one), and sends nothing. A Run that asks for no notification never reads the Credentials, so a broken file can't stop it, and update, version and help never read it either. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a Failed run (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal.

  • Subject: [thirdshift] <owner>/<repo>#<n> <issue title>: <outcome>, where the outcome is ready for review, merged, failed or interrupted. The title is left out if it can't be read from GitHub.
    • -
  • Body, plain text: a Result: line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; immediately followed by one Issue: line with the full original Issue URL, including after failure or interruption with no pull request, and a Title: line with the issue title when it was read; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its Base check:, Retry with: and Or set: lines, the session log path (if any), the Command log path as a Command log: line, a Sent by: thirdshift line, the hostname and how long the Run took. The body tells everything the subject does, so copying the body alone is enough.
    • +
  • Body, plain text: a Result: line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; immediately followed by one Issue: line with the full original Issue URL, including after failure or interruption with no pull request, and a Title: line with the issue title when it was read; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its Base check:, Retry with: and Or set: lines, the session log path (if any), the same one stderr gives, the Command log path as a Command log: line, a Sent by: thirdshift line, the hostname and how long the Run took. The body tells everything the subject does, so copying the body alone is enough.

The email's Built with line names the command's configured Harness, Model and Effort and is marked (configured). A Session models section lists sessions whose Harness was launched, including failed or interrupted sessions, Resumes and child Runs belonging to that Command. Models evidenced by the Harness's output or retained records are marked (reported), including model switches and delegated Claude answers. When the Harness supplies no model evidence, the requested Model is labelled (requested; actual model not reported), or its default is labelled Harness default (actual model not reported). A listed session proves the Harness was launched; an unconfirmed request does not prove that Model executed. Effort is the session's configured Effort; it does not describe a delegated answer's Effort. Attribution records (*.models.json) are kept beside the Session logs; a failure to write or read them is a warning and never changes the Run's outcome.

A Spec run sends at most one notification for the whole Spec, under the same rules, with its checks made once before any Ticket starts. Its subject names the Spec, its Issue: line links the parent Spec rather than a Ticket or pull request, its outcome is the Spec run's, and its body, after what a Run's holds (the Spec PR, if any), lists each Ticket's outcome, one line per Ticket as in the summary on stderr, such as #21 landed with https://github.com/acme/widgets/pull/1 or #22 blocked by #21. The Ticket Runs inside it never send a notification of their own, whatever the User config says.

@@ -615,7 +615,7 @@

Session logs share the owning command's folder, stamp and owner token: a Pickup run's dispatched Run logs in commands/pickup/, an Architect run's in commands/architect/, and a Security run's in commands/secure/. Tickets and nested Base fixes inherit that same ownership too.

A Resume is logged as its session's kind plus -resume, e.g. implement-resume.jsonl.

A Command log is everything one command printed, stderr and stdout in the order printed, while the terminal still shows all of it. Its folder is the command typed: thirdshift <Issue URL>, a Run or a Spec run, in issue/, thirdshift pickup in pickup/, named for the issue it took, and thirdshift architect in architect/. A Spec run's covers its Tickets' Runs, a Run's covers its Base fix, and a Pass's covers the Spec run or Run it dispatched: none of those keeps one of its own. So ls -t ~/.thirdshift/logs/acme/widgets/commands/pickup/*.log | head lists the recent passes on acme/widgets that took an issue. A command keeps its Command log once it starts work: a Pass skipped before doing any work keeps none, nor does a Run that fails before it starts work, as on the Origin match, and setup, email-test, update, version and help never keep one. Lines printed before the Command log's name is known, such as a Pickup run's lines on the issues it passed over before it took one, are written first, and once it is created a progress line says logging this command to <path>. A Command log that can't be written produces one warning: line on stderr. A failure confined to the Command log leaves the outcome, stdout and exit code unchanged; if the shared folder also prevents writing a Session log, the Run fails before starting that session.

-

The stamp is the local time the command started, with its UTC offset, as in 20261003T120000-0400, in the machine's time zone, or TZ's if set, so it agrees with date and ls -l. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of its most recent Session log, the place to start looking, then that of its Command log.

+

The stamp is the local time the command started, with its UTC offset, as in 20261003T120000-0400, in the machine's time zone, or TZ's if set, so it agrees with date and ls -l. A command's Command log and all of its Session logs, its Tickets' Runs' and its Base fix's included, share that one stamp, so they sort together and each can be found from the other. When a Run fails, stderr ends with the path of the Session log of the session its cause came from, the place to start looking, then that of its Command log. A failed implement session, Spec review or Repair names its own log, as does a Security review whose hold is reported only after later Repairs. A cause that came from no session, such as red CI with the Repairs used up, a Declined CI fix, the upstream-move budget, a Harness check or a git or GitHub error, names the most recent Session log. A Spec run whose Tickets are not done names the Session log of the one Ticket whose Run failed, and none if several did, as no single session caused it; each failed Ticket's line gives its own. The Command log's closing session log: line and the Run notification give the same path.

New Command and Session logs insert ~<token> immediately after the stamp. The random token gives each Command its own storage even when start times agree; its Command record uses the exact Command-log stem followed by .command.json. Model sidecars (.models.json) and review reports (.standards and .spec) use the exact Session-log stem. Human Command names stay the same, and start stamps remain display and grouping facts. A matching stamp alone does not attach a tokenless Session to a new Command: a record reference or exact Model evidence must establish that ownership. Legacy logs and records remain readable without renaming. A failed Command-log writer retains its allocated owner; if allocation itself failed, a Session fails with that cause instead of choosing a shared timestamp-only filename.

The Activity log, activity.log, is a short running record of what the factory did on the repository (ADR-0011). A Run, a Spec run or a Pass writes a line when it starts work, naming its Command log, and one when it ends, with its outcome; a Run you type by hand writes them too, and the run a Pass dispatched, a Spec run's Tickets and a Base fix write none of their own. A skipped Pass writes a line only when its reason differs from the last line of its own kind, so a repository that sits idle shows one line, not one per pass, and the first skip after a pass that did work always shows: that is when the repository went idle. Every line starts with the local date and time:

2026-10-03 02:00:01 Pickup run skipped: no Ready issue on acme/widgets