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..f8a91fd2 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,13 @@ 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), + ..crate::child_run::ChildAsks::default() + }, } } diff --git a/src/base_fix.rs b/src/base_fix.rs index 7f8dff0a..cdfd4987 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"); @@ -113,10 +111,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 +134,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 +148,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,14 +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`, 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 - /// `issue` already names `base` and those checks, no issue is written: + /// `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 `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, @@ -209,20 +195,13 @@ impl BaseFix { /// [`BaseFix::into_advice`] to give after its cause. pub fn fix( &mut self, - launch: &Git, - issue: &IssueUrl, + mut outside: LaunchAndGitHub, pr_url: &str, base: &str, base_commit: &str, failed: &FailedChecks, ) -> Result<()> { - let harness = self.harness.clone(); - let mut outside = LaunchAndGitHub { - launch, - issue, - harness: &harness, - security: self.security, - }; + let issue = outside.issue; self.fix_through(&mut outside, issue, pr_url, base, base_commit, failed) } @@ -485,14 +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, its sessions on -/// `harness`, and the poll interval. -struct LaunchAndGitHub<'a> { - launch: &'a Git, - issue: &'a IssueUrl, - harness: &'a Choice, - security: Options, +/// 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<'_> { @@ -550,7 +529,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 +877,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 +1328,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..fb7b366d 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,7 @@ mod tests { stamp: STAMPED.to_string(), command: logs::CommandKind::Issue, base_fix: BaseFixAsk::Forbid, - security: Options::default(), - harness: Choice::default(), + asks: ChildAsks::default(), }; let Err(error) = start_from(executable, &issue, &given) else { panic!("started from {}", executable.display()); @@ -792,18 +796,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..eabbe55f --- /dev/null +++ b/src/child_run/asks.rs @@ -0,0 +1,76 @@ +//! 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, 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. + 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 { + security_review, + ..ChildAsks::default() + } + } + + #[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..a5458537 100644 --- a/src/child_run/execution_tests.rs +++ b/src/child_run/execution_tests.rs @@ -168,8 +168,7 @@ 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::default(), }, 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..2cbbe4f2 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::{self, BaseFix}; +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 outside = base_fix::LaunchAndGitHub { + launch: self.worktree.launch(), + issue: self.issue, + asks: self.child_asks, + }; + self.base_fix + .fix(outside, 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();