From 0b7e20f86d4d68bfdef81de60fba44f2afde0fa0 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 07:51:49 -0400 Subject: [PATCH 1/2] Hand every child Run one set of child asks, built once after the Harness check A Run now builds its ChildAsks (the Command's checked Harness, Model and Effort, its Security fix ask and its Security review ask) once, after its Harness check, and hands them unchanged to the Delivery, its Base fix and the Spec run's Tickets. ChildAsks also answers whether a Run of a given child kind runs its own Security review, replacing the Run's Ticket mask. The Base fix no longer keeps a Harness copy to re-sync, and the Delivery no longer carries a Security fix ask it never read. Co-Authored-By: Claude Opus 5.5 --- GLOSSARY.md | 2 +- docs/code-navigation.md | 2 +- src/asks.rs | 13 +++-- src/base_fix.rs | 77 ++++++++++++---------------- src/child_run.rs | 86 +++++++++++++++++++------------- src/child_run/asks.rs | 77 ++++++++++++++++++++++++++++ src/child_run/execution_tests.rs | 7 ++- src/child_run/runs.rs | 11 ++-- src/delivery.rs | 35 +++++++------ src/run.rs | 43 +++++++--------- src/security.rs | 7 --- src/spec_run.rs | 28 +++-------- tests/security_review.rs | 11 ++-- tests/spec_run.rs | 20 ++++++++ 14 files changed, 249 insertions(+), 170 deletions(-) create mode 100644 src/child_run/asks.rs diff --git a/GLOSSARY.md b/GLOSSARY.md index 93263b7c..5f1ee734 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -148,7 +148,7 @@ An open **Ticket** labelled with a triage role that says it is not agent work: ` The agent session a **Spec run** starts once every **Ticket** is done: it reviews the whole **Spec branch** against the **Base branch** and the **Spec**, fixes what it agrees with, and writes the **Spec PR**'s description, listing the rest as **Unaddressed findings**. **Security review**: -When a flag or the **User config** asks for one, the agent session a **Run** starts after its opening session, or a **Spec run** starts on its **Spec PR** after the **Spec review**: it reviews the branch's change against the **Base branch** for vulnerabilities, and fixes each **Security finding** it can show with a failing test. A **Ticket**'s Run gets none. A Security review that a model refused, or that ended early, holds a **Self-merge** as an unaddressed Security finding does. +When a flag or the **User config** asks for one, the agent session a **Run** starts after its opening session, or a **Spec run** starts on its **Spec PR** after the **Spec review**: it reviews the branch's change against the **Base branch** for vulnerabilities, and fixes each **Security finding** it can show with a failing test. A **Ticket**'s Run gets none. A **Base fix** gets one when its Command asked for one, whichever Run started it, a Ticket's Run included. A Security review that a model refused, or that ended early, holds a **Self-merge** as an unaddressed Security finding does. _Avoid_: security axis, security audit (a **Security audit** covers the whole Base branch) **Failed spec run**: diff --git a/docs/code-navigation.md b/docs/code-navigation.md index 1ea27ce0..9761832f 100644 --- a/docs/code-navigation.md +++ b/docs/code-navigation.md @@ -26,7 +26,7 @@ Start with the issue's terms in `GLOSSARY.md`, then use this index to find their | Stop | `src/logs/reading.rs` — `retained_work` reload by captured record; `src/logs/reading/selection.rs` — `Selection::stop_target`; `src/logs/stop.rs` — request, signaling and waiting; `src/claim.rs` — Stop endings | `src/logs/reading/tests.rs`, `tests/stop.rs`; inline Claim and Stop tests | | User config, Setup | `src/config.rs`; `src/setup.rs` | `tests/user_config.rs`, `tests/setup.rs` | | Architecture review, Architect run | `src/architect.rs` | `tests/architect_run.rs` | -| Security audit, Security review, Security run | `src/security.rs` and `src/security/` | `tests/security_review.rs`, `tests/security_run.rs` | +| Security audit, Security review, Security run | `src/security.rs` and `src/security/`; which Runs run a Security review, and what every child Run is handed: `src/child_run/asks.rs` — `ChildAsks` | Inline tests; `tests/security_review.rs`, `tests/security_run.rs` | For example, `rg -n 'Claim|TicketBoard|RunNotification' src/claim.rs src/spec_run/ticket_board.rs src/notification.rs` locates the owners of Claim, Ticket board and Run notification without reading the whole glossary or README. Ticket board is an implementation term; the glossary defines its Spec run and Ticket vocabulary. diff --git a/src/asks.rs b/src/asks.rs index ea29cfe6..e3cc871d 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -109,10 +109,10 @@ impl Asks { tickets_at_once: config.spec_parallel, parallel_asked: false, base_fix: given.base_fix.clone(), - security_fix: given.security.fix, - security_review: given.security.review, + security_fix: given.asks.security_fix, + security_review: given.asks.security_review, launch_pull: false, - harness: given.harness.clone(), + harness: given.asks.harness.clone(), } } @@ -424,11 +424,14 @@ mod tests { fn given(kind: Kind, base_fix: BaseFixAsk) -> Given { Given { command: crate::logs::CommandKind::Issue, - security: crate::security::Options::default(), kind, stamp: "20261003T120000-0400".to_string(), base_fix, - harness: sonnet(None), + asks: crate::child_run::ChildAsks { + harness: sonnet(None), + security_fix: false, + security_review: false, + }, } } diff --git a/src/base_fix.rs b/src/base_fix.rs index 7f8dff0a..dd2c24e0 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -14,16 +14,14 @@ use std::path::PathBuf; use anyhow::{Result, bail}; -use crate::child_run::{self, Ended, Kind}; +use crate::child_run::{self, ChildAsks, Ended, Kind}; use crate::ci::{self, FailedChecks}; use crate::git::Git; use crate::github::{Check, GitHub, ListedIssue}; -use crate::harness::Choice; use crate::issue::IssueUrl; use crate::labels::{Label, Labels, READY_FOR_AGENT}; use crate::poll; use crate::progress; -use crate::security::Options; /// The label that marks a Base fix issue, which a Run finds an open one by. pub const BASE_FIX: Label = Label::new("base-fix", "A Base fix: CI is red on a Base branch"); @@ -91,6 +89,14 @@ impl fmt::Display for Advice { } } +/// Where a Run starts a Base fix from: the Launch directory's git `launch`, +/// as the Run on `issue`, handing it `asks`, as the Run does every child Run. +pub struct StartedFrom<'a> { + pub launch: &'a Git, + pub issue: &'a IssueUrl, + pub asks: &'a ChildAsks, +} + /// What a Run does when its only red checks are Inherited failures. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum OnInheritedFailures { @@ -113,10 +119,6 @@ pub struct BaseFix { /// What the Run says after its cause, if Inherited failures failed it /// with no Base fix taken. advice: Vec, - /// The Harness, Model and Effort a Base fix it starts runs its sessions - /// on: the Run's. - harness: Choice, - security: Options, } /// The Base fix a Run took as its one: one it started, or one it found @@ -140,10 +142,9 @@ impl Taken { impl BaseFix { /// The Base fix of a Run that is the child Run `child`, if it is one, - /// that asked `ask` about a Base fix, and whose sessions run on - /// `harness`, as a Base fix it starts does. A Base fix starts none of - /// its own, whatever it asked. - pub fn new(child: Option<&Kind>, ask: BaseFixAsk, harness: Choice, security: Options) -> Self { + /// that asked `ask` about a Base fix. A Base fix starts none of its own, + /// whatever it asked. + pub fn new(child: Option<&Kind>, ask: BaseFixAsk) -> Self { let (on_inherited_failures, retry) = match (child, ask) { (Some(Kind::BaseFix { .. }), _) => (OnInheritedFailures::IsBaseFix, None), (_, BaseFixAsk::Allow) => (OnInheritedFailures::StartBaseFix, None), @@ -155,17 +156,9 @@ impl BaseFix { retry, taken: None, advice: Vec::new(), - harness, - security, } } - /// Have any Base fix it starts run its sessions on `harness`: the Run's, - /// once the Run has checked it. - pub fn runs_on(&mut self, harness: Choice) { - self.harness = harness; - } - /// Whether the Run compares its red checks with the Base branch's, so /// that some may be Inherited failures. pub fn sees_inherited_failures(&self) -> bool { @@ -193,11 +186,12 @@ impl BaseFix { } /// Fix the Inherited failures in `failed`, the only red checks on the PR - /// `pr_url` of the Run on `issue`, started from `launch`, which also fail - /// on `base` at `base_commit`, where `failed` holds that commit's checks - /// of their names, with a Base fix: write its issue, start it as a child - /// `thirdshift` and wait for it to merge, after which the Run is to merge - /// `base` in and watch CI again. If an open Base fix issue other than + /// `pr_url` of the Run on `issue`, started from `launch`, as `from` says, + /// which also fail on `base` at `base_commit`, where `failed` holds that + /// commit's checks of their names, with a Base fix: write its issue, + /// start it as a child `thirdshift` handed the `asks` in `from`, and wait + /// for it to merge, after which the Run is to merge `base` in and watch + /// CI again. If an open Base fix issue other than /// `issue` already names `base` and those checks, no issue is written: /// the Run waits for that one to close instead, as its one Base fix (see /// [`BaseFix::wait_on`]), or, if a Run from `launch` started that Base fix @@ -209,19 +203,21 @@ impl BaseFix { /// [`BaseFix::into_advice`] to give after its cause. pub fn fix( &mut self, - launch: &Git, - issue: &IssueUrl, + from: StartedFrom, pr_url: &str, base: &str, base_commit: &str, failed: &FailedChecks, ) -> Result<()> { - let harness = self.harness.clone(); + let StartedFrom { + launch, + issue, + asks, + } = from; let mut outside = LaunchAndGitHub { launch, issue, - harness: &harness, - security: self.security, + asks, }; self.fix_through(&mut outside, issue, pr_url, base, base_commit, failed) } @@ -486,13 +482,12 @@ impl Mark { } /// The outside world of a Run on `issue` started from `launch`: its git -/// directory's locks and marks, GitHub, the child Run, its sessions on -/// `harness`, and the poll interval. +/// directory's locks and marks, GitHub, the child Run, handed `asks`, and +/// the poll interval. struct LaunchAndGitHub<'a> { launch: &'a Git, issue: &'a IssueUrl, - harness: &'a Choice, - security: Options, + asks: &'a ChildAsks, } impl Outside for LaunchAndGitHub<'_> { @@ -550,7 +545,7 @@ impl Outside for LaunchAndGitHub<'_> { let kind = Kind::BaseFix { base: base.to_string(), }; - child_run::start(fix, kind, BaseFixAsk::Forbid, self.security, self.harness)?.wait() + child_run::start(fix, kind, BaseFixAsk::Forbid, self.asks)?.wait() } fn pause(&mut self) -> Result<()> { @@ -898,7 +893,7 @@ mod tests { /// A Run's Base fix as asked `ask`, not itself a Base fix. fn asked(ask: BaseFixAsk) -> BaseFix { - BaseFix::new(None, ask, Choice::default(), Options::default()) + BaseFix::new(None, ask) } fn undecided() -> BaseFixAsk { @@ -1349,22 +1344,14 @@ mod tests { base: "main".to_string(), }; for ask in [BaseFixAsk::Allow, BaseFixAsk::Forbid, undecided()] { - let base_fix = BaseFix::new(Some(&child), ask, Choice::default(), Options::default()); + let base_fix = BaseFix::new(Some(&child), ask); assert!(!base_fix.sees_inherited_failures()); assert_eq!(base_fix.ask_of_tickets(), BaseFixAsk::Forbid); } let ticket = Kind::Ticket { spec_branch: "spec-3".to_string(), }; - assert!( - BaseFix::new( - Some(&ticket), - BaseFixAsk::Allow, - Choice::default(), - Options::default() - ) - .sees_inherited_failures() - ); + assert!(BaseFix::new(Some(&ticket), BaseFixAsk::Allow).sees_inherited_failures()); assert!(asked(BaseFixAsk::Forbid).sees_inherited_failures()); } } diff --git a/src/child_run.rs b/src/child_run.rs index d706d541..bd291386 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -3,8 +3,10 @@ //! (ADR-0008). [`Runs`] owns concurrent children and their collective cleanup; //! standalone [`start`] and [`Handle::wait`] serve Base fix. +mod asks; mod runs; +pub use asks::ChildAsks; pub use runs::Runs; use std::ffi::OsString; @@ -24,7 +26,6 @@ use crate::issue::IssueUrl; use crate::logs; use crate::progress; use crate::run_ending; -use crate::security::Options; #[cfg(test)] use execution_tests::Fault; @@ -74,10 +75,8 @@ pub struct Given { pub command: logs::CommandKind, /// What it is asked about a Base fix. pub base_fix: BaseFixAsk, - pub security: Options, - /// The Harness, Model and Effort its sessions run on: the command's - /// that started it. - pub harness: Choice, + /// What the Run that started it hands on to every child Run. + pub asks: ChildAsks, } /// The hidden argument that makes a child Run a [`Kind::Ticket`], followed by @@ -98,6 +97,9 @@ const LOGS_COMMAND: &str = "--logs-command"; /// The hidden argument that asks a child Run [`BaseFixAsk::Allow`]. const ALLOW_BASE_FIX: &str = "--allow-base-fix"; + +/// The hidden arguments that give a child Run [`ChildAsks::security_review`] +/// and [`ChildAsks::security_fix`]. const SECURITY_REVIEW: &str = "--run-security-review"; const ALLOW_SECURITY_FIX: &str = "--allow-security-fix"; @@ -136,17 +138,18 @@ impl Given { BaseFixAsk::Forbid => {} BaseFixAsk::Undecided { retry } => args.extend([OFFER_BASE_FIX, retry]), } - if self.security.fix { + if self.asks.security_fix { args.push(ALLOW_SECURITY_FIX); } - if self.security.review { + if self.asks.security_review { args.push(SECURITY_REVIEW); } - args.extend([SESSIONS_HARNESS, self.harness.harness.name()]); - if let Some(model) = &self.harness.model { + let harness = &self.asks.harness; + args.extend([SESSIONS_HARNESS, harness.harness.name()]); + if let Some(model) = &harness.model { args.extend([SESSIONS_MODEL, model]); } - if let Some(effort) = &self.harness.effort { + if let Some(effort) = &harness.effort { args.extend([SESSIONS_EFFORT, effort]); } args @@ -162,7 +165,8 @@ pub struct Reader { stamp: Option, command: Option, base_fix: Option, - security: Options, + security_review: bool, + security_fix: bool, harness: Option, model: Option, effort: Option, @@ -183,16 +187,16 @@ impl Reader { }; match arg { SECURITY_REVIEW => { - if self.security.review { + if self.security_review { bail!("repeated argument: {arg}"); } - self.security.review = true; + self.security_review = true; } ALLOW_SECURITY_FIX => { - if self.security.fix { + if self.security_fix { bail!("repeated argument: {arg}"); } - self.security.fix = true; + self.security_fix = true; } SPEC_BRANCH | BASE_FIX_INTO => { not_yet_given(&self.kind, arg)?; @@ -257,8 +261,8 @@ impl Reader { if self.command.is_some() || self.stamp.is_some() || self.base_fix.is_some() - || self.security.review - || self.security.fix + || self.security_review + || self.security_fix || self.harness.is_some() || self.model.is_some() || self.effort.is_some() @@ -281,14 +285,17 @@ impl Reader { .command .with_context(|| format!("missing {LOGS_COMMAND}"))?, base_fix: self.base_fix.unwrap_or(BaseFixAsk::Forbid), - security: self.security, - harness: Choice { - harness, - model: self.model, - effort: self.effort, - // Chosen by the command that started it, whose checks it - // passed. - chosen_by: ChosenBy::Command, + asks: ChildAsks { + harness: Choice { + harness, + model: self.model, + effort: self.effort, + // Chosen by the command that started it, whose checks it + // passed. + chosen_by: ChosenBy::Command, + }, + security_fix: self.security_fix, + security_review: self.security_review, }, })) } @@ -342,21 +349,19 @@ pub struct Handle { /// Start a Run of `kind` on `issue` in a child `thirdshift`, from the same /// Launch directory, given this command's start stamp for its Session logs, -/// asked `base_fix` about a Base fix, and running its sessions on `harness`. +/// asked `base_fix` about a Base fix, and handed `asks`. pub fn start( issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, - security: Options, - harness: &Choice, + asks: &ChildAsks, ) -> Result { let given = Given { kind, stamp: logs::stamp(), command: logs::command_kind(), base_fix, - security, - harness: harness.clone(), + asks: asks.clone(), }; start_from(&own_executable()?, issue, &given) } @@ -743,8 +748,11 @@ mod tests { stamp: STAMPED.to_string(), command: logs::CommandKind::Issue, base_fix: BaseFixAsk::Forbid, - security: Options::default(), - harness: Choice::default(), + asks: ChildAsks { + harness: Choice::default(), + security_fix: false, + security_review: false, + }, }; let Err(error) = start_from(executable, &issue, &given) else { panic!("started from {}", executable.display()); @@ -792,18 +800,26 @@ mod tests { ..on_claude.clone() }; let harnesses = [on_claude, on_opus]; + let securities = [(false, false), (true, false), (false, true), (true, true)]; for (kind, harness) in [ticket(), base_fix()] .into_iter() .zip(harnesses.iter().cycle()) { - for base_fix in [BaseFixAsk::Allow, BaseFixAsk::Forbid, offer.clone()] { + for (base_fix, (security_fix, security_review)) in + [BaseFixAsk::Allow, BaseFixAsk::Forbid, offer.clone()] + .into_iter() + .flat_map(|base_fix| securities.map(|security| (base_fix.clone(), security))) + { let given = Given { kind: kind.clone(), stamp: STAMPED.to_string(), command: logs::CommandKind::Issue, base_fix, - security: Options::default(), - harness: harness.clone(), + asks: ChildAsks { + harness: harness.clone(), + security_fix, + security_review, + }, }; let written = given.to_args(); // Before, between and after the hidden arguments, each of diff --git a/src/child_run/asks.rs b/src/child_run/asks.rs new file mode 100644 index 00000000..429132d9 --- /dev/null +++ b/src/child_run/asks.rs @@ -0,0 +1,77 @@ +//! What every child Run a Run starts is asked, whichever kind it is: built +//! once in the Run, after its Harness check, and handed on unchanged. + +use super::Kind; +use crate::asks::Asks; +use crate::harness::Choice; + +/// The Command's checked Harness, Model and Effort, its Security fix ask and +/// its Security review ask, as every child Run is given them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ChildAsks { + /// The Harness, Model and Effort a child Run's sessions run on, as the + /// Command's checks settled them. + pub harness: Choice, + /// Whether a Security run may fix reproduced findings. + pub security_fix: bool, + /// Whether the Command asked for a Security review: its own ask, before + /// any Run of a kind that runs none drops it. See + /// [`ChildAsks::runs_security_review`]. + pub security_review: bool, +} + +impl ChildAsks { + /// What a Run asked `asks` hands on to every child Run it starts. Built + /// only once `asks.harness` has been checked: by this Run, or by the + /// command that started it. + pub fn of(asks: &Asks) -> ChildAsks { + ChildAsks { + harness: asks.harness.clone(), + security_fix: asks.security_fix, + security_review: asks.security_review, + } + } + + /// Whether a Run that is the child Run `child`, if it is one, runs its + /// own Security review. A Ticket's Run runs none: its Spec run reviews + /// the Spec PR instead. A Base fix runs one when the Command asked for + /// one, whichever Run started it. + pub fn runs_security_review(&self, child: Option<&Kind>) -> bool { + self.security_review && !matches!(child, Some(Kind::Ticket { .. })) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn asked(security_review: bool) -> ChildAsks { + ChildAsks { + harness: Choice::default(), + security_fix: false, + security_review, + } + } + + #[test] + fn only_a_ticket_s_run_skips_the_security_review_the_command_asked_for() { + let ticket = Kind::Ticket { + spec_branch: "issue-20".to_string(), + }; + let base_fix = Kind::BaseFix { + base: "main".to_string(), + }; + for (child, when_asked) in [ + (None, true), + (Some(&ticket), false), + (Some(&base_fix), true), + ] { + assert_eq!( + asked(true).runs_security_review(child), + when_asked, + "{child:?}" + ); + assert!(!asked(false).runs_security_review(child), "{child:?}"); + } + } +} diff --git a/src/child_run/execution_tests.rs b/src/child_run/execution_tests.rs index 510dd7c0..736cec6b 100644 --- a/src/child_run/execution_tests.rs +++ b/src/child_run/execution_tests.rs @@ -168,8 +168,11 @@ impl Fixture { stamp: "20261003T120000-0400".to_string(), command: logs::CommandKind::Issue, base_fix: BaseFixAsk::Forbid, - security: crate::security::Options::default(), - harness: Choice::default(), + asks: super::ChildAsks { + harness: Choice::default(), + security_fix: false, + security_review: false, + }, }, startup, ) diff --git a/src/child_run/runs.rs b/src/child_run/runs.rs index e7c3d420..044e293f 100644 --- a/src/child_run/runs.rs +++ b/src/child_run/runs.rs @@ -5,11 +5,9 @@ use std::thread; use anyhow::{Result, bail}; -use super::{Ended, Handle, Kind, POLL}; +use super::{ChildAsks, Ended, Handle, Kind, POLL}; use crate::base_fix::BaseFixAsk; -use crate::harness::Choice; use crate::issue::IssueUrl; -use crate::security::Options; /// Own child Runs until their numbered endings are delivered or cleanup finishes. #[derive(Default)] @@ -27,12 +25,9 @@ impl Runs { issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, - security: Options, - harness: &Choice, + asks: &ChildAsks, ) -> Result<()> { - self.start_using(issue.number, || { - super::start(issue, kind, base_fix, security, harness) - }) + self.start_using(issue.number, || super::start(issue, kind, base_fix, asks)) } /// Launch and retain ownership before returning to the caller. diff --git a/src/delivery.rs b/src/delivery.rs index c0391844..c33a3fd6 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -22,7 +22,8 @@ mod stage; use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; -use crate::base_fix::BaseFix; +use crate::base_fix::{BaseFix, StartedFrom}; +use crate::child_run::ChildAsks; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; use crate::git::Git; @@ -46,9 +47,11 @@ use stage::{CommandLog, CommandRecord, Stage, Tracker}; /// A Delivery of the pull request for `issue`, the Run's issue or the Spec, /// into the Base branch `base`, to `goal`. pub struct Delivery<'a> { - pub security_fix: bool, /// Whether the opening session is followed by a guidance Security review. pub security_review: bool, + /// What the Run hands on to every child Run, a Base fix or a Ticket's + /// Run, whose Harness, Model and Effort its own sessions run on too. + pub child_asks: &'a ChildAsks, pub issue: &'a IssueUrl, pub base: &'a str, pub goal: Goal, @@ -57,9 +60,6 @@ pub struct Delivery<'a> { pub base_fix: &'a mut BaseFix, /// Where its Session logs go. pub logs: &'a Logs, - /// The Harness, Model and Effort its sessions run on, and its child - /// Runs' too. - pub harness: &'a Choice, /// When the Run began: an untrusted edit to `issue` made since then /// starts no further session. pub started: DateTime, @@ -134,7 +134,7 @@ impl Delivery<'_> { base: self.base, branch: worktree.branch(), goal: self.goal, - harness: self.harness, + harness: &self.child_asks.harness, }; pull_request.begin_delivery(); let (issue, started, path) = (self.issue, self.started, worktree.path()); @@ -145,7 +145,7 @@ impl Delivery<'_> { let (delivered, log) = Sessions::checked_within( self.logs, worktree.path(), - self.harness, + &self.child_asks.harness, &before_each, |sessions| { let mut outside = InWorktree { @@ -154,6 +154,7 @@ impl Delivery<'_> { worktree: &worktree, base: self.base, base_fix: self.base_fix, + child_asks: self.child_asks, sessions, pull_request, }; @@ -511,6 +512,8 @@ struct InWorktree<'a> { worktree: &'a Worktree, base: &'a str, base_fix: &'a mut BaseFix, + /// What a Base fix it starts is handed. + child_asks: &'a ChildAsks, sessions: &'a Sessions<'a>, pull_request: &'a mut PullRequest, } @@ -568,6 +571,7 @@ impl Outside for InWorktree<'_> { base: self.base, pr_url, base_fix: self.base_fix, + child_asks: self.child_asks, sessions: self.sessions, pull_request: self.pull_request, }; @@ -641,6 +645,8 @@ struct RunOutside<'a> { base: &'a str, pr_url: &'a str, base_fix: &'a mut BaseFix, + /// What a Base fix it starts is handed. + child_asks: &'a ChildAsks, sessions: &'a Sessions<'a>, pull_request: &'a mut PullRequest, } @@ -728,14 +734,13 @@ impl repair_loop::Outside for RunOutside<'_> { } fn base_fix(&mut self, base_commit: &str, failed: &FailedChecks) -> Result<()> { - self.base_fix.fix( - self.worktree.launch(), - self.issue, - self.pr_url, - self.base, - base_commit, - failed, - ) + let from = StartedFrom { + launch: self.worktree.launch(), + issue: self.issue, + asks: self.child_asks, + }; + self.base_fix + .fix(from, self.pr_url, self.base, base_commit, failed) } fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()> { diff --git a/src/run.rs b/src/run.rs index 8b08d4f3..68353e71 100644 --- a/src/run.rs +++ b/src/run.rs @@ -10,12 +10,11 @@ use chrono::{DateTime, TimeDelta, Utc}; use crate::asks::Asks; use crate::base_fix::{Advice, BaseFix}; use crate::branch::{self, Selection}; -use crate::child_run::Kind; +use crate::child_run::{ChildAsks, Kind}; use crate::claim::{self, Claim}; use crate::delivery::{Delivery, Opening, OpeningSession}; use crate::failed_run::FailedRun; use crate::github::{GitHub, Ticket}; -use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; use crate::launch::LaunchDirectory; @@ -118,15 +117,7 @@ impl<'a> StartedBy<'a> { /// Harness's names for them once checked. Its Run notification, if `asks` /// has it send one, is for what started the Run to send, once it has ended. pub fn run_to_end(issue: &IssueUrl, asks: &mut Asks, started_by: StartedBy) -> Ended { - let mut base_fix = BaseFix::new( - started_by.child(), - asks.base_fix.clone(), - asks.harness.clone(), - crate::security::Options { - fix: asks.security_fix, - review: asks.security_review, - }, - ); + let mut base_fix = BaseFix::new(started_by.child(), asks.base_fix.clone()); let mut outcome = run(issue, asks, started_by, &mut base_fix); if let Err(failed) = &mut outcome && let Some(recovery) = &mut failed.recovery @@ -152,10 +143,11 @@ pub fn run_to_end(issue: &IssueUrl, asks: &mut Asks, started_by: StartedBy) -> E /// The Launch directory is opened first, with its Origin match, the check /// that `issue` is open and the git identity check. A Run started by its /// command then checks the Harness, Model and Effort its sessions run on, as -/// `asks` say, settling the Model and Effort in them, and in `base_fix`, on -/// the Harness's names: one started by another thirdshift, or dispatched by -/// a pass, runs on what that command checked. The rest is [`start`], through the -/// Launch directory, GitHub and the logs. +/// `asks` say, settling the Model and Effort in them on the Harness's names: +/// one started by another thirdshift, or dispatched by a pass, runs on what +/// that command checked. Only then is what it hands on to every child Run +/// built from `asks`. The rest is [`start`], through the Launch directory, +/// GitHub and the logs. /// /// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may /// start, or wait on, when its only red checks are Inherited failures: what @@ -177,19 +169,17 @@ fn run( )), } asks.harness.check()?; - base_fix.runs_on(asks.harness.clone()); } let asks = &*asks; + let child_asks = ChildAsks::of(asks); let mut outside = LaunchAndGitHub { directory: &directory, issue, goal: asks.goal, - security_fix: asks.security_fix, - security_review: asks.security_review - && !matches!(started_by, StartedBy::Child(Kind::Ticket { .. })), + security_review: child_asks.runs_security_review(started_by.child()), + child_asks: &child_asks, base_fix, logs: Logs::of_run(issue), - harness: &asks.harness, claim: None, issue_branch: None, child: started_by.child().is_some(), @@ -395,16 +385,17 @@ trait Outside { /// The outside world of a Run on `issue` from the opened Launch directory /// `directory`: GitHub, its git, the logs, the Claim it made, the worktree, /// the Spec run and the Delivery to `goal`, with `base_fix` and the Run's -/// Session `logs`, its sessions on `harness`. +/// Session `logs`, its sessions on the Harness in `child_asks`, which it +/// hands on to every child Run. struct LaunchAndGitHub<'a> { directory: &'a LaunchDirectory, issue: &'a IssueUrl, goal: Goal, - security_fix: bool, + /// Whether the Run runs its own Security review. security_review: bool, + child_asks: &'a ChildAsks, base_fix: &'a mut BaseFix, logs: Logs, - harness: &'a Choice, claim: Option>, issue_branch: Option, child: bool, @@ -446,14 +437,13 @@ impl LaunchAndGitHub<'_> { /// The Delivery of the issue into the Base branch `base`. fn delivery<'d>(&'d mut self, base: &'d str) -> Delivery<'d> { Delivery { - security_fix: self.security_fix, security_review: self.security_review, + child_asks: self.child_asks, issue: self.issue, base, goal: self.goal, base_fix: self.base_fix, logs: &self.logs, - harness: self.harness, started: self.started, } } @@ -471,7 +461,7 @@ impl<'a> Outside for LaunchAndGitHub<'a> { } fn started(&mut self, work: Work) { - logs::started(work, self.harness); + logs::started(work, &self.child_asks.harness); if self.child { logs::issue_title(GitHub::new().issue_title(self.issue).ok()); } @@ -560,6 +550,7 @@ mod tests { use super::*; use crate::base_fix::BaseFixAsk; use crate::github::{PrState, PullRequest}; + use crate::harness::Choice; use crate::labels::Labels; use crate::notification::NotificationAsk; diff --git a/src/security.rs b/src/security.rs index 2e95496e..6fd00172 100644 --- a/src/security.rs +++ b/src/security.rs @@ -21,13 +21,6 @@ pub(crate) mod fixing; pub mod reproduction; pub(crate) mod review; -/// Resolved security choices passed together to child Runs and Base fixes. -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] -pub struct Options { - pub fix: bool, - pub review: bool, -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum FixAsk { Allow, diff --git a/src/spec_run.rs b/src/spec_run.rs index bfbb1d37..3b64af98 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -16,11 +16,10 @@ use std::num::NonZeroUsize; use anyhow::{Result, bail}; use crate::base_fix::BaseFixAsk; -use crate::child_run::{Ended, Kind, Runs}; +use crate::child_run::{ChildAsks, Ended, Kind, Runs}; use crate::delivery::{Delivery, Opening, OpeningSession}; use crate::failed_run::{FailedRun, interrupted_or}; use crate::github::{GitHub, Ticket}; -use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; use crate::progress; @@ -201,8 +200,8 @@ trait Outside { /// The outside world of a Spec run on `delivery`'s Spec, from its Spec /// branch checked out in `worktree`: child `thirdshift` Runs, each of which -/// may start a Base fix if `base_fix` allows one, and runs its sessions on -/// `harness`, owned together in `children`, GitHub, the Spec PR +/// may start a Base fix if `base_fix` allows one, and is handed +/// `child_asks`, owned together in `children`, GitHub, the Spec PR /// `spec_pr`, and `delivery`, which takes the /// worktree when it starts. struct ChildRunsAndGitHub<'a> { @@ -217,9 +216,7 @@ struct ChildRunsAndGitHub<'a> { /// Held until it starts. delivery: Option>, base_fix: BaseFixAsk, - security_fix: bool, - security_review: bool, - harness: &'a Choice, + child_asks: &'a ChildAsks, spec_pr: PullRequest, } @@ -227,16 +224,13 @@ impl<'a> ChildRunsAndGitHub<'a> { /// The outside world of a Spec run, as the struct says, with no Ticket's /// Run started yet. fn new(worktree: Worktree, delivery: Delivery<'a>, spec_pr: PullRequest) -> Self { - let security_review = delivery.security_review; Self { spec: delivery.issue, children: Runs::default(), active: std::collections::BTreeMap::new(), worktree: Some(worktree), base_fix: delivery.base_fix.ask_of_tickets(), - security_fix: delivery.security_fix, - security_review, - harness: delivery.harness, + child_asks: delivery.child_asks, delivery: Some(delivery), spec_pr, } @@ -268,16 +262,8 @@ impl Outside for ChildRunsAndGitHub<'_> { if let Some(check) = trust::ticket_trust(&GitHub::new(), ticket, self.spec)? { bail!("Ticket #{}: {check}", ticket.number); } - self.children.start( - ticket, - kind, - self.base_fix.clone(), - crate::security::Options { - fix: self.security_fix, - review: self.security_review, - }, - self.harness, - )?; + self.children + .start(ticket, kind, self.base_fix.clone(), self.child_asks)?; self.active.insert(ticket.number, ticket.clone()); Ok(()) } diff --git a/tests/security_review.rs b/tests/security_review.rs index 06acfed9..cb643974 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -504,11 +504,14 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] assert_eq!(result.code, Some(0), "{}", result.stderr); let calls = scenario.claude_calls(); assert_eq!(calls.len(), 4); + // The Base fix's own Delivery runs the Security review its Command asked + // for, on its own branch. + assert_eq!(calls[3]["branch"], "issue-8"); + let prompt = calls[3]["prompt"].as_str().unwrap(); + assert!(prompt.contains("guidance mode"), "{prompt}"); assert!( - calls[3]["prompt"] - .as_str() - .unwrap() - .contains("against Base branch main") + prompt.contains("branch issue-8 against Base branch main"), + "{prompt}" ); assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); } diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 22465e78..2a4b111e 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1194,6 +1194,26 @@ gh pr create --base issue-20 --head issue-{fix} --title "Fix CI" --body "Closes ); } +#[test] +fn a_base_fix_a_tickets_run_starts_runs_the_security_review_its_ticket_skips() { + let scenario = spec_whose_ticket_inherits_a_failure(); + base_fix_lands_on_the_spec_branch(&scenario, 22); + scenario.agent_does_for_in_session(22, 2, &security_review_script(&[])); + scenario.agent_does_for_in_session(SPEC, 2, &security_review_script(&[])); + + let result = scenario.run(&[&spec_url(&scenario), "base-fix", "security-review"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(sessions_by_issue(&scenario), ["21", "22", "22", "20", "20"]); + let calls = scenario.claude_calls(); + assert_eq!(calls[2]["branch"], "issue-22"); + assert_contains( + calls[2]["prompt"].as_str().unwrap(), + "branch issue-22 against Base branch issue-20", + ); + assert_eq!(pr_from(&scenario, "issue-22").unwrap()["state"], "MERGED"); +} + #[test] fn with_base_fix_a_tickets_inherited_failure_gets_a_base_fix_into_the_spec_branch() { let scenario = spec_whose_ticket_inherits_a_failure(); From b42ebda6530de3ce14e10f13bac0f73bfe92d127 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 08:05:15 -0400 Subject: [PATCH 2/2] Start a Base fix through its own outside world and default child asks in tests The Base fix's LaunchAndGitHub now takes the launch, issue and child asks straight from the Delivery, in place of a StartedFrom copy of the same fields. ChildAsks derives Default, so test fixtures need not spell it out. Co-Authored-By: Claude Opus 5.5 --- src/asks.rs | 3 +- src/base_fix.rs | 48 +++++++++++--------------------- src/child_run.rs | 6 +--- src/child_run/asks.rs | 5 ++-- src/child_run/execution_tests.rs | 6 +--- src/delivery.rs | 6 ++-- 6 files changed, 24 insertions(+), 50 deletions(-) diff --git a/src/asks.rs b/src/asks.rs index e3cc871d..f8a91fd2 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -429,8 +429,7 @@ mod tests { base_fix, asks: crate::child_run::ChildAsks { harness: sonnet(None), - security_fix: false, - security_review: false, + ..crate::child_run::ChildAsks::default() }, } } diff --git a/src/base_fix.rs b/src/base_fix.rs index dd2c24e0..cdfd4987 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -89,14 +89,6 @@ impl fmt::Display for Advice { } } -/// Where a Run starts a Base fix from: the Launch directory's git `launch`, -/// as the Run on `issue`, handing it `asks`, as the Run does every child Run. -pub struct StartedFrom<'a> { - pub launch: &'a Git, - pub issue: &'a IssueUrl, - pub asks: &'a ChildAsks, -} - /// What a Run does when its only red checks are Inherited failures. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum OnInheritedFailures { @@ -186,15 +178,15 @@ impl BaseFix { } /// Fix the Inherited failures in `failed`, the only red checks on the PR - /// `pr_url` of the Run on `issue`, started from `launch`, as `from` says, + /// `pr_url` of the Run on `outside.issue`, started from `outside.launch`, /// which also fail on `base` at `base_commit`, where `failed` holds that /// commit's checks of their names, with a Base fix: write its issue, - /// start it as a child `thirdshift` handed the `asks` in `from`, and wait - /// for it to merge, after which the Run is to merge `base` in and watch - /// CI again. If an open Base fix issue other than - /// `issue` already names `base` and those checks, no issue is written: + /// start it as a child `thirdshift` handed `outside.asks`, and wait for + /// it to merge, after which the Run is to merge `base` in and watch CI + /// again. If an open Base fix issue other than the Run's issue already + /// names `base` and those checks, no issue is written: /// the Run waits for that one to close instead, as its one Base fix (see - /// [`BaseFix::wait_on`]), or, if a Run from `launch` started that Base fix + /// [`BaseFix::wait_on`]), or, if a Run from its Launch directory started that Base fix /// and it ended with the issue still open, starts it again on the same /// issue. Fails, with the Run's cause, if the Run was not asked to start /// one, if the Base fix fails, in which case the cause names its issue, @@ -203,22 +195,13 @@ impl BaseFix { /// [`BaseFix::into_advice`] to give after its cause. pub fn fix( &mut self, - from: StartedFrom, + mut outside: LaunchAndGitHub, pr_url: &str, base: &str, base_commit: &str, failed: &FailedChecks, ) -> Result<()> { - let StartedFrom { - launch, - issue, - asks, - } = from; - let mut outside = LaunchAndGitHub { - launch, - issue, - asks, - }; + let issue = outside.issue; self.fix_through(&mut outside, issue, pr_url, base, base_commit, failed) } @@ -481,13 +464,14 @@ impl Mark { } } -/// The outside world of a Run on `issue` started from `launch`: its git -/// directory's locks and marks, GitHub, the child Run, handed `asks`, and -/// the poll interval. -struct LaunchAndGitHub<'a> { - launch: &'a Git, - issue: &'a IssueUrl, - asks: &'a ChildAsks, +/// The outside world of a Run on `issue` started from `launch`, as it +/// starts a Base fix: its git directory's locks and marks, GitHub, the child +/// Run, handed `asks`, as the Run hands every child Run, and the poll +/// interval. +pub struct LaunchAndGitHub<'a> { + pub launch: &'a Git, + pub issue: &'a IssueUrl, + pub asks: &'a ChildAsks, } impl Outside for LaunchAndGitHub<'_> { diff --git a/src/child_run.rs b/src/child_run.rs index bd291386..fb7b366d 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -748,11 +748,7 @@ mod tests { stamp: STAMPED.to_string(), command: logs::CommandKind::Issue, base_fix: BaseFixAsk::Forbid, - asks: ChildAsks { - harness: Choice::default(), - security_fix: false, - security_review: false, - }, + asks: ChildAsks::default(), }; let Err(error) = start_from(executable, &issue, &given) else { panic!("started from {}", executable.display()); diff --git a/src/child_run/asks.rs b/src/child_run/asks.rs index 429132d9..eabbe55f 100644 --- a/src/child_run/asks.rs +++ b/src/child_run/asks.rs @@ -7,7 +7,7 @@ use crate::harness::Choice; /// The Command's checked Harness, Model and Effort, its Security fix ask and /// its Security review ask, as every child Run is given them. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct ChildAsks { /// The Harness, Model and Effort a child Run's sessions run on, as the /// Command's checks settled them. @@ -47,9 +47,8 @@ mod tests { fn asked(security_review: bool) -> ChildAsks { ChildAsks { - harness: Choice::default(), - security_fix: false, security_review, + ..ChildAsks::default() } } diff --git a/src/child_run/execution_tests.rs b/src/child_run/execution_tests.rs index 736cec6b..a5458537 100644 --- a/src/child_run/execution_tests.rs +++ b/src/child_run/execution_tests.rs @@ -168,11 +168,7 @@ impl Fixture { stamp: "20261003T120000-0400".to_string(), command: logs::CommandKind::Issue, base_fix: BaseFixAsk::Forbid, - asks: super::ChildAsks { - harness: Choice::default(), - security_fix: false, - security_review: false, - }, + asks: super::ChildAsks::default(), }, startup, ) diff --git a/src/delivery.rs b/src/delivery.rs index c33a3fd6..2cbbe4f2 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -22,7 +22,7 @@ mod stage; use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; -use crate::base_fix::{BaseFix, StartedFrom}; +use crate::base_fix::{self, BaseFix}; use crate::child_run::ChildAsks; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; @@ -734,13 +734,13 @@ impl repair_loop::Outside for RunOutside<'_> { } fn base_fix(&mut self, base_commit: &str, failed: &FailedChecks) -> Result<()> { - let from = StartedFrom { + let outside = base_fix::LaunchAndGitHub { launch: self.worktree.launch(), issue: self.issue, asks: self.child_asks, }; self.base_fix - .fix(from, self.pr_url, self.base, base_commit, failed) + .fix(outside, self.pr_url, self.base, base_commit, failed) } fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()> {