diff --git a/README.md b/README.md index 2da9e45..59e9b8d 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,7 @@ parallel = 2 # how many Tickets a Spec run runs at once, instead of 3 limit = 5 # how many open issues labelled in-progress stop a Pickup run taking another, instead of 3 [security] +review = true # review each Run's change for Security findings harness = "claude" # the Harness for Security runs; blank or missing follows harness.default [harness] @@ -252,6 +253,7 @@ limit = 3 # how many open issues labelled in-progress stop a Pickup run taking [security] fix = false # Security runs may fix reproduced findings; default false +review = false # Runs review their change for Security findings; default false harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code [harness] @@ -289,8 +291,9 @@ Every key holds its real value, so a Run reading it does exactly what it does wi 3. With that on, every Run may start a Base fix when the Base branch's CI is red? (`base.fix`). With it off, this is not asked, and `base.fix` is written at its default, `false`. 4. Every Run first fast-forwards your checkout of the Base branch? (`launch.pull`) 5. Security runs may fix reproduced findings? (`security.fix`), off by default. -6. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`). -7. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials `. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written. +6. Runs review their change for Security findings? (`security.review`), off by default. +7. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`). +8. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials `. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written. Pressing Enter takes the default shown, which is the file's current value, or else the setting's default, and for the address the suggested email above. `logs.dir`, `activity.quiet_skips`, `spec.parallel`, `pickup.limit` and `security.harness` are not asked about. Setup adds a missing `security.harness` at its default, blank, with a comment explaining it; a configured Harness is kept. The answers are written like everything else below: in place, keeping your comments. Ctrl-C, SIGTERM or SIGHUP during the questions or a Model/Effort check exits `1` with `interrupted` and writes nothing: neither the User config nor the Credentials. Ordinary and hidden questions stop without another line of input, and hidden input restores the saved terminal settings, including echo. With notifications off, nothing about a key is asked, and saved Credentials stay as they were, so `--email` on a single Run still works. Credentials a Run would refuse (see [Email](#email)) are refused before any question, exit `1`, and not touched. With no terminal, as from cron or `thirdshift setup ///audits/ A finding's title becomes the advisory summary. Its description preserves its write-up, trace, evidence and validation plan, with the fingerprint and audited commit. thirdshift claims no severity, CWE or affected version range. It uses the package in the repository's manifest, or the `other` ecosystem when none is identified. Matching fingerprints in any advisory state are kept rather than recorded again. Rejected candidates stay in the local report and produce no advisory. Draft advisories require GitHub repository security manager or administrator access. On a private repository whose advisory endpoint is unavailable, the private record is an issue labelled `security-finding` and `needs-triage`. It is never replaced by a public finding issue. +### Security review + +`security-review`, or `review = true` under `[security]`, enables one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. `no-security-review` overrides the setting. Both words, with or without dashes, are accepted on commands that start Runs and follow dispatched Runs and Base fixes. Setup asks about the setting; it is off by default. + +The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run that is not a Merge run records the outcome and continues. Pre-existing vulnerability details stay out of the PR. Spec PR review and private recording of old findings are tracked separately in [#551](https://github.com/JacobStephens2/thirdshift/issues/551) and [#552](https://github.com/JacobStephens2/thirdshift/issues/552). The separate Security audit retains its full audit workflow. + ### Fencing **Fencing** is the factory finding and fixing vulnerabilities with nobody starting it: Security runs started on a schedule with fixing allowed. thirdshift never schedules itself ([ADR-0009](docs/adr/0009-the-operating-system-schedules-thirdshift.md)). This Linux crontab entry starts one every five minutes; use your own paths, with `claude` and `gh` already logged in and Git able to push: diff --git a/prompts/README.md b/prompts/README.md index 87701f3..458adf4 100644 --- a/prompts/README.md +++ b/prompts/README.md @@ -25,6 +25,7 @@ With `harness codex`, each session runs `codex exec --json --dangerously-bypass- | Continuation, with an open pull request | Starts the implement session when the Run is a Continuation of an Issue branch whose pull request is open. | [continuation-pr.md](continuation-pr.md) | | Spec review | In a Spec run, starts the Spec review once every Ticket has landed on the Spec branch, before the Spec PR, a draft until then, is marked ready. | [spec-review.md](spec-review.md) | | Architecture review | Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none. | [architecture-review.md](architecture-review.md) | +| Security review | When enabled, runs one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. Base fixes get one too; introduced findings or incomplete reviews hold Self-merge. | [security-review.md](security-review.md) | | Security audit | Starts the report-only Security audit in a throwaway worktree at origin's Base branch head. The threat-model line is included when a conventional document exists; artifacts stay under the repository's audit root. | [security-audit.md](security-audit.md) | | Security reproduction | After a Security audit, tries to reproduce one recorded finding in a fresh throwaway worktree at its audited commit. The test is copied into the private record only after a complete outcome. | [security-reproduction.md](security-reproduction.md) | | Conflict Repair | Starts a Repair session when merging the Base branch into the Issue branch leaves conflicts. | [conflict-repair.md](conflict-repair.md) | diff --git a/prompts/security-review.md b/prompts/security-review.md new file mode 100644 index 0000000..aaf36b3 --- /dev/null +++ b/prompts/security-review.md @@ -0,0 +1,21 @@ + + +# Security review + +When enabled, runs one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. Base fixes get one too; introduced findings or incomplete reviews hold Self-merge. + +``` +Use the `thirdshift-security-audit` skill in guidance mode. +Review the change for on branch with `git diff ...HEAD`, including supporting code. +Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present. +Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts. +Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix. +Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here. +Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session. +If refused, incomplete, or missing a prerequisite, say so and do not claim completion. +After a complete review, end your final message with exactly one line of the form: +Security review: {"unaddressed_count":0,"findings":[]} +Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it. + +You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the command's timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. Before ending your turn, stop every background task you no longer need, by its task id (with the `TaskStop` tool, if you have it): a task still running when your turn ends is taken as work you were waiting on. +``` diff --git a/site/prompts/index.html b/site/prompts/index.html index 7a763d2..86a7690 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -155,6 +155,25 @@

Architecture review

Architecture review idea: <URL of the issue you filed> Architecture review already filed: <URL of the open issue that already covers it> +You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the command's timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. Before ending your turn, stop every background task you no longer need, by its task id (with the `TaskStop` tool, if you have it): a task still running when your turn ends is taken as work you were waiting on. + + +
+

Security review

+

When enabled, runs one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. Base fixes get one too; introduced findings or incomplete reviews hold Self-merge.

+

Sent by 3 · Y · Review

+ 
Use the `thirdshift-security-audit` skill in guidance mode.
+Review the change for <Issue URL> on branch <branch> with `git diff <base>...HEAD`, including supporting code.
+Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.
+Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.
+Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.
+Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.
+Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.
+If refused, incomplete, or missing a prerequisite, say so and do not claim completion.
+After a complete review, end your final message with exactly one line of the form:
+Security review: {"unaddressed_count":0,"findings":[]}
+Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.
+
 You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the command's timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. Before ending your turn, stop every background task you no longer need, by its task id (with the `TaskStop` tool, if you have it): a task still running when your turn ends is taken as work you were waiting on.
diff --git a/src/args.rs b/src/args.rs index f059e36..e99b9bd 100644 --- a/src/args.rs +++ b/src/args.rs @@ -258,6 +258,18 @@ fn take_flag<'a>( arg, SECURITY_FIX_FLAGS, )?, + "security-review" | "--security-review" => ask_once( + &mut flags.security_review, + crate::security::review::Ask::Allow, + arg, + SECURITY_REVIEW_FLAGS, + )?, + "no-security-review" | "--no-security-review" => ask_once( + &mut flags.security_review, + crate::security::review::Ask::Forbid, + arg, + SECURITY_REVIEW_FLAGS, + )?, "harness" | "--harness" => ask_harness(&mut flags.harness.harness, arg, rest.next())?, "model" | "--model" => { let model = &mut flags.harness.model_and_effort.model; @@ -275,6 +287,7 @@ fn take_flag<'a>( const MERGE_FLAGS: &str = "merge and no-merge"; const EMAIL_FLAGS: &str = "email and no-email"; const BASE_FIX_FLAGS: &str = "base-fix and no-base-fix"; +const SECURITY_REVIEW_FLAGS: &str = "security-review and no-security-review"; const SECURITY_FIX_FLAGS: &str = "security-fix and no-security-fix"; /// Record in `given` what the flag `arg` asked for: the same kind of ask @@ -355,6 +368,22 @@ mod tests { const URL: &str = "https://github.com/acme/widgets/issues/7"; + #[test] + fn security_review_words_are_accepted_on_commands_that_start_runs() { + for command in [URL, "pickup", "architect", "secure"] { + for word in [ + "security-review", + "--security-review", + "no-security-review", + "--no-security-review", + ] { + assert!(parse_strs(&[command, word]).is_ok(), "{command} {word}"); + } + } + assert!(parse_strs(&[URL, "security-review", "no-security-review"]).is_err()); + assert!(parse_strs(&[URL, "security-review", "--security-review"]).is_err()); + } + fn parse_strs(args: &[&str]) -> Result { let args: Vec = args.iter().map(|arg| arg.to_string()).collect(); parse(&args) @@ -738,6 +767,7 @@ mod tests { base: Some("develop".to_string()), flags: Flags { security_fix: None, + security_review: None, goal: Some(Goal::Merged), email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))), parallel: NonZeroUsize::new(2), @@ -793,6 +823,7 @@ mod tests { base: None, flags: Flags { security_fix: None, + security_review: None, goal: Some(Goal::ReadyForReview), email: Some(NotificationAsk::Skip), parallel: None, diff --git a/src/asks.rs b/src/asks.rs index 56ec566..4742b91 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -28,6 +28,8 @@ pub struct Flags { pub base_fix: Option, /// What `security-fix` or `no-security-fix` asked for. pub security_fix: Option, + /// What security-review or no-security-review asked for. + pub security_review: Option, /// What `harness `, `model ` and `effort ` asked for. pub harness: harness::Asked, } @@ -48,6 +50,8 @@ pub struct Asks { pub base_fix: BaseFixAsk, /// Whether a Security run may fix reproduced findings, passed to child Runs. pub security_fix: bool, + /// Whether Delivery runs the optional guidance Security review. + pub security_review: bool, /// Whether it first brings the Launch directory's checkout of the Base /// branch up to date with origin. pub launch_pull: bool, @@ -82,6 +86,10 @@ impl Asks { parallel_asked: flags.parallel.is_some(), base_fix, security_fix: flags.security_fix_allowed(config), + security_review: flags + .security_review + .map(|ask| ask == crate::security::review::Ask::Allow) + .unwrap_or(config.security_review), launch_pull: config.launch_pull, harness: flags.harness(config), } @@ -101,7 +109,8 @@ impl Asks { tickets_at_once: config.spec_parallel, parallel_asked: false, base_fix: given.base_fix.clone(), - security_fix: given.security_fix, + security_fix: given.security.fix, + security_review: given.security.review, launch_pull: false, harness: given.harness.clone(), } @@ -226,6 +235,11 @@ fn retry_with_base_fix(issue: &IssueUrl, flags: &Flags) -> String { Some(crate::security::FixAsk::Forbid) => command += " no-security-fix", None => {} } + match flags.security_review { + Some(crate::security::review::Ask::Allow) => command += " security-review", + Some(crate::security::review::Ask::Forbid) => command += " no-security-review", + None => {} + } let asked = &flags.harness; if let Some(harness) = asked.harness { command += &format!(" harness {}", harness.name()); @@ -277,6 +291,7 @@ mod tests { harness: harness::Settings::default(), security_harness: None, security_fix: None, + security_review: false, } } @@ -357,6 +372,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: n(3), @@ -376,6 +392,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, notification: NotificationAsk::Send(None), tickets_at_once: n(5), @@ -391,6 +408,7 @@ mod tests { fn each_flag_decides_its_ask_whatever_the_user_config_says() { let against = Flags { security_fix: None, + security_review: None, goal: Some(Goal::ReadyForReview), email: Some(NotificationAsk::Skip), parallel: Some(n(2)), @@ -402,6 +420,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -415,6 +434,7 @@ mod tests { let for_it = Flags { security_fix: None, + security_review: None, goal: Some(Goal::Merged), email: Some(to("flag@example.com")), parallel: Some(n(2)), @@ -426,6 +446,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, notification: to("flag@example.com"), tickets_at_once: n(2), @@ -441,7 +462,7 @@ mod tests { /// fix, its sessions on Claude's `sonnet`. fn given(kind: Kind, base_fix: BaseFixAsk) -> Given { Given { - security_fix: false, + security: crate::security::Options::default(), kind, stamp: "20261003T120000-0400".to_string(), base_fix, @@ -506,6 +527,7 @@ mod tests { fn dispatch_flags() -> Flags { Flags { security_fix: None, + security_review: None, goal: Some(Goal::Merged), email: Some(to("flag@example.com")), parallel: Some(n(2)), @@ -521,6 +543,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -538,6 +561,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, // Whatever `email.always` says: the Architect run sends it. notification: NotificationAsk::Skip, @@ -557,6 +581,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -574,6 +599,7 @@ mod tests { asks, Asks { security_fix: false, + security_review: false, goal: Goal::Merged, // Whatever `email.always` says: the Pickup run sends it. notification: NotificationAsk::Skip, @@ -659,6 +685,7 @@ mod tests { fn the_retry_command_is_the_issue_url_and_the_flags_as_given_with_base_fix_added() { let flags = |goal, email, parallel| Flags { security_fix: None, + security_review: None, goal, email, parallel, diff --git a/src/base_fix.rs b/src/base_fix.rs index 82ac205..6225fc6 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -23,6 +23,7 @@ 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"); @@ -115,7 +116,7 @@ pub struct BaseFix { /// The Harness, Model and Effort a Base fix it starts runs its sessions /// on: the Run's. harness: Choice, - security_fix: bool, + security: Options, } /// The Base fix a Run took as its one: one it started, or one it found @@ -141,7 +142,7 @@ impl BaseFix { /// 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_fix: bool) -> Self { + pub fn new(child: Option<&Kind>, ask: BaseFixAsk, harness: Choice, security: Options) -> Self { let (on_inherited_failures, retry) = match (child, ask) { (Some(Kind::BaseFix { .. }), _) => (OnInheritedFailures::IsBaseFix, None), (_, BaseFixAsk::Allow) => (OnInheritedFailures::StartBaseFix, None), @@ -154,7 +155,7 @@ impl BaseFix { taken: None, advice: Vec::new(), harness, - security_fix, + security, } } @@ -219,7 +220,7 @@ impl BaseFix { launch, issue, harness: &harness, - security_fix: self.security_fix, + security: self.security, }; self.fix_through(&mut outside, issue, pr_url, base, base_commit, failed) } @@ -472,7 +473,7 @@ struct LaunchAndGitHub<'a> { launch: &'a Git, issue: &'a IssueUrl, harness: &'a Choice, - security_fix: bool, + security: Options, } impl Outside for LaunchAndGitHub<'_> { @@ -530,14 +531,7 @@ impl Outside for LaunchAndGitHub<'_> { let kind = Kind::BaseFix { base: base.to_string(), }; - child_run::start( - fix, - kind, - BaseFixAsk::Forbid, - self.security_fix, - self.harness, - )? - .wait() + child_run::start(fix, kind, BaseFixAsk::Forbid, self.security, self.harness)?.wait() } fn pause(&mut self) -> Result<()> { @@ -885,7 +879,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(), false) + BaseFix::new(None, ask, Choice::default(), Options::default()) } fn undecided() -> BaseFixAsk { @@ -1332,7 +1326,7 @@ mod tests { base: "main".to_string(), }; for ask in [BaseFixAsk::Allow, BaseFixAsk::Forbid, undecided()] { - let base_fix = BaseFix::new(Some(&child), ask, Choice::default(), false); + let base_fix = BaseFix::new(Some(&child), ask, Choice::default(), Options::default()); assert!(!base_fix.sees_inherited_failures()); assert_eq!(base_fix.ask_of_tickets(), BaseFixAsk::Forbid); } @@ -1340,8 +1334,13 @@ mod tests { spec_branch: "spec-3".to_string(), }; assert!( - BaseFix::new(Some(&ticket), BaseFixAsk::Allow, Choice::default(), false) - .sees_inherited_failures() + BaseFix::new( + Some(&ticket), + BaseFixAsk::Allow, + Choice::default(), + Options::default() + ) + .sees_inherited_failures() ); assert!(asked(BaseFixAsk::Forbid).sees_inherited_failures()); } diff --git a/src/child_run.rs b/src/child_run.rs index 8531165..4c5267d 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -24,6 +24,7 @@ use crate::issue::IssueUrl; use crate::logs; use crate::progress; use crate::run_ending; +use crate::security::Options; #[cfg(test)] use execution_tests::Fault; @@ -71,7 +72,7 @@ pub struct Given { pub stamp: String, /// What it is asked about a Base fix. pub base_fix: BaseFixAsk, - pub security_fix: bool, + pub security: Options, /// The Harness, Model and Effort its sessions run on: the command's /// that started it. pub harness: Choice, @@ -92,6 +93,7 @@ const STAMP: &str = "--stamp"; /// The hidden argument that asks a child Run [`BaseFixAsk::Allow`]. const ALLOW_BASE_FIX: &str = "--allow-base-fix"; +const SECURITY_REVIEW: &str = "--run-security-review"; const ALLOW_SECURITY_FIX: &str = "--allow-security-fix"; /// The hidden argument followed by the command that starts the Spec run @@ -127,9 +129,12 @@ impl Given { BaseFixAsk::Forbid => {} BaseFixAsk::Undecided { retry } => args.extend([OFFER_BASE_FIX, retry]), } - if self.security_fix { + if self.security.fix { args.push(ALLOW_SECURITY_FIX); } + if self.security.review { + args.push(SECURITY_REVIEW); + } args.extend([SESSIONS_HARNESS, self.harness.harness.name()]); if let Some(model) = &self.harness.model { args.extend([SESSIONS_MODEL, model]); @@ -149,7 +154,7 @@ pub struct Reader { kind: Option, stamp: Option, base_fix: Option, - security_fix: bool, + security: Options, harness: Option, model: Option, effort: Option, @@ -169,11 +174,17 @@ impl Reader { None => bail!("missing {missing}"), }; match arg { + SECURITY_REVIEW => { + if self.security.review { + bail!("repeated argument: {arg}"); + } + 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)?; @@ -230,7 +241,8 @@ impl Reader { let Some(kind) = self.kind else { if self.stamp.is_some() || self.base_fix.is_some() - || self.security_fix + || self.security.review + || self.security.fix || self.harness.is_some() || self.model.is_some() || self.effort.is_some() @@ -250,7 +262,7 @@ impl Reader { kind, stamp, base_fix: self.base_fix.unwrap_or(BaseFixAsk::Forbid), - security_fix: self.security_fix, + security: self.security, harness: Choice { harness, model: self.model, @@ -312,14 +324,14 @@ pub fn start( issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, - security_fix: bool, + security: Options, harness: &Choice, ) -> Result { let given = Given { kind, stamp: logs::stamp(), base_fix, - security_fix, + security, harness: harness.clone(), }; start_from(&own_executable()?, issue, &given) @@ -697,7 +709,7 @@ mod tests { kind: ticket(), stamp: STAMPED.to_string(), base_fix: BaseFixAsk::Forbid, - security_fix: false, + security: Options::default(), harness: Choice::default(), }; let Err(error) = start_from(executable, &issue, &given) else { @@ -755,7 +767,7 @@ mod tests { kind: kind.clone(), stamp: STAMPED.to_string(), base_fix, - security_fix: false, + security: Options::default(), harness: harness.clone(), }; let written = given.to_args(); diff --git a/src/child_run/execution_tests.rs b/src/child_run/execution_tests.rs index 1ce5b06..4681924 100644 --- a/src/child_run/execution_tests.rs +++ b/src/child_run/execution_tests.rs @@ -167,7 +167,7 @@ impl Fixture { }, stamp: "20261003T120000-0400".to_string(), base_fix: BaseFixAsk::Forbid, - security_fix: false, + security: crate::security::Options::default(), harness: Choice::default(), }, startup, diff --git a/src/child_run/runs.rs b/src/child_run/runs.rs index 2a378a4..e7c3d42 100644 --- a/src/child_run/runs.rs +++ b/src/child_run/runs.rs @@ -9,6 +9,7 @@ use super::{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)] @@ -26,11 +27,11 @@ impl Runs { issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, - security_fix: bool, + security: Options, harness: &Choice, ) -> Result<()> { self.start_using(issue.number, || { - super::start(issue, kind, base_fix, security_fix, harness) + super::start(issue, kind, base_fix, security, harness) }) } diff --git a/src/config.rs b/src/config.rs index 88f22d7..879351d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -47,6 +47,7 @@ pub struct UserConfig { pub security_harness: Option, /// `security.fix`: off by default; None leaves the decision unmade. pub security_fix: Option, + pub security_review: bool, } /// The `[email]` section: where email goes and who it comes from. The Resend @@ -90,6 +91,7 @@ impl UserConfig { harness: harness::Settings::default(), security_harness: None, security_fix: None, + security_review: false, } } @@ -159,6 +161,12 @@ impl UserConfig { ("pickup", "limit", value) => { config.pickup_limit = whole_number_from_1(value, "pickup.limit", &file)? } + ("security", "review", Value::Boolean(review)) => { + config.security_review = *review + } + ("security", "review", _) => { + bail!("security.review must be true or false in {file}") + } ("security", "fix", Value::Boolean(fix)) => config.security_fix = Some(*fix), ("security", "fix", _) => bail!("security.fix must be true or false in {file}"), ("security", "harness", Value::String(name)) => { @@ -213,6 +221,7 @@ pub struct UserConfigChanges { pub base_fix: bool, pub launch_pull: bool, pub security_fix: bool, + pub security_review: bool, /// None disables Run notifications while retaining their addresses. pub notifications: Option, /// None retains every Harness setting. Unset Model/Effort writes blank. @@ -262,6 +271,7 @@ impl UserConfigDocument { set(&mut document, "base", "fix", changes.base_fix); set(&mut document, "launch", "pull", changes.launch_pull); set(&mut document, "security", "fix", changes.security_fix); + set(&mut document, "security", "review", changes.security_review); set( &mut document, "email", @@ -677,6 +687,7 @@ limit = 3 # how many open issues labelled in-progress stop a Pickup run taking [security] fix = false # Security runs may fix reproduced findings; default false +review = false # Runs review their change for Security findings; default false harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code [harness] @@ -835,6 +846,7 @@ mod tests { "spec.parallel", "pickup.limit", "security.fix", + "security.review", "security.harness", "harness.default", "harness.claude.model", @@ -1188,6 +1200,7 @@ mod tests { base_fix: true, launch_pull: true, security_fix: false, + security_review: false, notifications: Some(NotificationAddresses { to: "me@example.com".to_string(), from: "ts@example.com".to_string(), @@ -1342,6 +1355,7 @@ mod tests { base_fix: false, launch_pull: false, security_fix: false, + security_review: false, notifications: Some(NotificationAddresses { to: to.to_string(), from: crate::email::DEFAULT_FROM.to_string(), @@ -1435,6 +1449,7 @@ mod tests { base_fix: false, launch_pull: false, security_fix: false, + security_review: false, notifications: None, harness: Some(( Harness::Codex, @@ -1479,6 +1494,7 @@ mod tests { base_fix: true, launch_pull: true, security_fix: false, + security_review: false, notifications: Some(NotificationAddresses { to: "o\"brien@example.com".to_string(), from: "ts@example.com".to_string(), @@ -1564,6 +1580,7 @@ mod tests { base_fix: false, launch_pull: true, security_fix: false, + security_review: false, notifications: None, harness: None, }; diff --git a/src/delivery.rs b/src/delivery.rs index 4fddf1b..2194855 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -1,5 +1,6 @@ //! Delivery: what a Run, or a Spec run for its Spec PR, does from its -//! worktree once its work begins. The opening session, the push after it, +//! worktree once its work begins. The opening session, the optional guidance +//! Security review, the push after it, //! marking the pull request ready, the Repair loop that keeps it mergeable //! and green, the Self-merge in a Merge run and its steps after the merge, //! and the Failed run path when any of these fails. @@ -29,6 +30,7 @@ use crate::progress; use crate::prompt; use crate::run::{Goal, Reached}; use crate::run_ending::Cause; +use crate::security::review::{Hold as SecurityHold, Outcome as SecurityOutcome, Review}; use crate::session::{Logs, Sessions}; use crate::worktree::{ForeignCommits, Merge, PendingMerge, Worktree}; @@ -37,6 +39,8 @@ use repair_loop::{Repair, Upstream}; /// 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, pub issue: &'a IssueUrl, pub base: &'a str, pub goal: Goal, @@ -64,7 +68,8 @@ pub struct Opening<'a> { impl Delivery<'_> { /// Take the pull request from the branch checked out in `worktree` to the /// goal. With `opening.catch_up_from_origin`, first fast-forward the - /// branch to origin. Then run the opening session, push the branch, for + /// branch to origin. Then run the opening session and optional Security + /// review, push the branch, for /// any commit the session left unpushed, write the line that says what it /// was built with in the pull request's body, only warning if that fails, /// restore the optional Tickets checklist, and mark the pull request ready, @@ -83,6 +88,7 @@ impl Delivery<'_> { checklist: Option<&str>, ) -> Result { let route = Route { + security_review: self.security_review, issue: self.issue, base: self.base, branch: worktree.branch(), @@ -124,6 +130,7 @@ impl Delivery<'_> { /// The steps of a Delivery of the pull request for `issue`, from `branch` /// into the Base branch `base`, to `goal`, its sessions run on `harness`. struct Route<'a> { + security_review: bool, issue: &'a IssueUrl, base: &'a str, branch: &'a str, @@ -147,10 +154,40 @@ impl Route<'_> { outside.catch_up()?; } outside.session(opening.kind, &opening.prompt)?; + let review = self.security_review.then(|| { + SecurityOutcome::of(outside.security_review(&prompt::security_review( + self.issue, + self.base, + self.branch, + ))) + }); outside.push()?; self.write_built_with(outside); let pr = outside.mark_pr_ready(checklist)?; - outside.take_to_goal(&pr.url, self.goal)?; + let hold = review.as_ref().and_then(SecurityOutcome::hold); + if let Some(review) = &review + && let Err(error) = outside.record_security_review(review) + { + outside.warn( + &error, + "could not write the Security review outcome in the pull request's body" + .to_string(), + ); + } + // A Security hold prevents Self-merge, while ordinary readiness still + // includes repairing conflicts and watching the branch's CI. + let goal = if hold.is_some() { + Goal::ReadyForReview + } else { + self.goal + }; + outside.take_to_goal(&pr.url, goal)?; + if let Some(hold) = hold { + outside.step(hold.to_string()); + if self.goal == Goal::Merged { + return Err(hold.into()); + } + } if self.goal == Goal::Merged { self.after_merge(outside, &pr); } @@ -237,7 +274,7 @@ fn retry_if_interrupted( /// sessions have ended with `error`: commit and push the work, and send an /// open PR back to draft. An interrupt, if one was requested, is the error /// instead: it can surface as some other error, such as a killed git. A -/// `PolicyRefusal` neither pushes nor converts, so the PR stays ready on the +/// `PolicyRefusal` or `SecurityHold` neither pushes nor converts, so the PR stays ready on the /// 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 @@ -249,7 +286,7 @@ fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::E // to its first line: the reason goes in the failure commit's subject. 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::(); + let keep_ready = error.is::() || error.is::(); if !keep_ready && let Err(problem) = outside.preserve_failed_run(&reason) { outside.step(format!( "could not push the failed run's work, so it may exist only locally: {problem:#}" @@ -278,6 +315,8 @@ trait Outside { fn catch_up(&mut self) -> Result<()>; /// Run the session `kind` given `prompt`. fn session(&mut self, kind: &str, prompt: &str) -> Result<()>; + fn security_review(&mut self, prompt: &str) -> Result; + fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()>; /// Push the Issue branch. fn push(&mut self) -> Result<()>; /// Write the annotation through the captured pull request module. @@ -336,6 +375,19 @@ impl Outside for InWorktree<'_> { self.sessions.run(kind, prompt) } + fn security_review(&mut self, prompt: &str) -> Result { + let message = self.sessions.run_to_final_message( + crate::session::Purpose::Security, + "security-review", + prompt, + )?; + Review::from_final_message(message.as_deref()) + } + + fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> { + self.pull_request.record_security_review(&review.entry()) + } + fn push(&mut self) -> Result<()> { self.worktree.push() } @@ -577,6 +629,14 @@ mod tests { } impl Outside for Scripted { + fn record_security_review(&mut self, _: &SecurityOutcome) -> Result<()> { + Ok(()) + } + fn security_review(&mut self, _: &str) -> Result { + Review::from_final_message(Some( + "Security review: {\"unaddressed_count\":0,\"findings\":[]}", + )) + } fn catch_up(&mut self) -> Result<()> { self.calls.push(Call::CatchUp); self.check(Fails::CatchUp) @@ -656,6 +716,7 @@ mod tests { fn deliver(outside: &mut Scripted, goal: Goal, spec: bool) -> Result { let issue = IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap(); let route = Route { + security_review: false, issue: &issue, base: "main", branch: "issue-7", diff --git a/src/main.rs b/src/main.rs index e388f6e..1fbf6b4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -317,6 +317,14 @@ known, title and private link, never its write-up. A skipped Security run sends The address and Resend API key checks come before skip checks or work; a failed send is only a warning and never changes the run's outcome. +security-review enables one guidance review after a Run's opening session, before Delivery +pushes and enters the Repair loop. [security] review = true also enables it; +no-security-review overrides the setting. Both words are accepted on commands that start +Runs and follow dispatched Runs and Base fixes. Setup asks; the default is off. +Only reproduced findings are fixed, with their failing tests kept. Unaddressed introduced +findings, a refused review or an incomplete review hold Self-merge, leaving the PR ready +for review and naming the reason. Other Runs record the Security outcome and continue. + security-fix allows a Security run to fix one reproduced finding: the most severe first, ties in private-record order, before auditing or after an audit reproduces a finding. The publishing session follows the reproduction's fix size: one terse Ticket, or a Spec diff --git a/src/prompt.rs b/src/prompt.rs index ee02c71..1383f7a 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -314,6 +314,26 @@ pub fn security_audit( ) } +/// One guidance session on the Run's change, before Delivery pushes and +/// enters the Repair loop. The separate Security audit remains a full audit. +pub fn security_review(issue: &IssueUrl, base: &str, branch: &str) -> String { + format!( + "Use the `thirdshift-security-audit` skill in guidance mode.\n\ + Review the change for {url} on branch {branch} with `git diff {base}...HEAD`, including supporting code.\n\ + Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.\n\ + Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.\n\ + Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.\n\ + Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.\n\ + Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.\n\ + If refused, incomplete, or missing a prerequisite, say so and do not claim completion.\n\ + After a complete review, end your final message with exactly one line of the form:\n\ + Security review: {{\"unaddressed_count\":0,\"findings\":[]}}\n\ + Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.\n\n\ + {HEADLESS}", + url = issue.url, + ) +} + pub fn security_reproduction(commit: &str, finding: &str, test: &std::path::Path) -> String { format!( "Reproduce this recorded Security finding at audited commit `{commit}`.\n\ diff --git a/src/prompts_page.rs b/src/prompts_page.rs index eb95a08..468fe5a 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -164,6 +164,13 @@ fn prompts() -> Vec { sender: ArchitectRun, text: prompt::architecture_review(BASE, Some(FOCUS)), }, + Prompt { + id: "prompt-security-review", + title: "Security review", + when: "When enabled, runs one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. Base fixes get one too; introduced findings or incomplete reviews hold Self-merge.", + sender: Units(&[REVIEW]), + text: prompt::security_review(&issue, BASE, BRANCH), + }, Prompt { id: "prompt-security-audit", title: "Security audit", diff --git a/src/pull_request.rs b/src/pull_request.rs index 882183b..61d1d57 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -179,6 +179,37 @@ impl PullRequest { Ok(()) } + /// Retain a deterministic review outcome even if the session refused + /// before it could edit the body. Keep the agent's detailed evidence. + pub fn record_security_review(&mut self, entry: &str) -> Result<()> { + let snapshot = self.observe(false)?.context("no PR found")?; + self.validate(&snapshot, true)?; + let body = self.adapter.body(&self.issue, snapshot.pr.number, false)?; + let section = "## Unaddressed findings"; + let start = ""; + let end = ""; + let summary = format!("{start}\n### Security review outcome\n\n{entry}\n{end}\n"); + let previous = body + .find(start) + .and_then(|at| body[at..].find(end).map(|last| (at, at + last + end.len()))); + let updated = if let Some((first, last)) = previous { + format!("{}{}{}", &body[..first], summary.trim_end(), &body[last..]) + } else if let Some(at) = body.find(section) { + let after = at + section.len(); + let end = body[after..] + .find("\n## ") + .map_or(body.len(), |next| after + next); + format!("{}\n\n{summary}\n{}", body[..end].trim_end(), &body[end..]) + } else { + format!("{}\n\n{section}\n\n{summary}", body.trim_end()) + }; + if updated != body { + self.adapter + .set_body(&self.issue, snapshot.pr.number, &updated, false)?; + } + Ok(()) + } + /// Observe afresh and validate the expected branches and open state /// before marking this number ready. Already-ready PRs need no request. pub fn mark_ready(&mut self, checklist: Option<&str>) -> Result { diff --git a/src/run.rs b/src/run.rs index 721c3b4..89d7b9a 100644 --- a/src/run.rs +++ b/src/run.rs @@ -118,7 +118,10 @@ pub fn run_to_end(issue: &IssueUrl, asks: &mut Asks, started_by: StartedBy) -> E started_by.child(), asks.base_fix.clone(), asks.harness.clone(), - asks.security_fix, + crate::security::Options { + fix: asks.security_fix, + review: asks.security_review, + }, ); let outcome = run(issue, asks, started_by, &mut base_fix); Ended { @@ -165,6 +168,8 @@ fn run( issue, goal: asks.goal, security_fix: asks.security_fix, + security_review: asks.security_review + && !matches!(started_by, StartedBy::Child(Kind::Ticket { .. })), base_fix, logs: Logs::of_run(issue), harness: &asks.harness, @@ -358,6 +363,7 @@ struct LaunchAndGitHub<'a> { issue: &'a IssueUrl, goal: Goal, security_fix: bool, + security_review: bool, base_fix: &'a mut BaseFix, logs: Logs, harness: &'a Choice, @@ -369,6 +375,7 @@ impl LaunchAndGitHub<'_> { fn delivery<'d>(&'d mut self, base: &'d str) -> Delivery<'d> { Delivery { security_fix: self.security_fix, + security_review: self.security_review, issue: self.issue, base, goal: self.goal, @@ -695,6 +702,7 @@ mod tests { fn asks() -> Asks { Asks { security_fix: false, + security_review: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: NonZeroUsize::new(1).unwrap(), diff --git a/src/security.rs b/src/security.rs index 4b2f48e..73673f2 100644 --- a/src/security.rs +++ b/src/security.rs @@ -20,6 +20,14 @@ use crate::pass::{LaunchAndGitHub, Outside}; pub mod audit; 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 { diff --git a/src/security/review.rs b/src/security/review.rs new file mode 100644 index 0000000..b888fa7 --- /dev/null +++ b/src/security/review.rs @@ -0,0 +1,96 @@ +//! The guidance review's final-line contract. Only introduced findings may +//! appear here; pre-existing vulnerability details stay out of public text. + +use anyhow::{Context, Result, bail}; +use serde::Deserialize; +use std::fmt; + +use crate::harness::interpretation::SafeguardRefusal; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Ask { + Allow, + Forbid, +} + +pub(crate) enum Outcome { + Complete(Review), + Incomplete(String), +} + +impl Outcome { + pub fn of(result: Result) -> Self { + match result { + Ok(review) => Self::Complete(review), + Err(error) => Self::Incomplete(match error.downcast_ref::() { + Some(refusal) => format!("Security review refused: {refusal}"), + None => "Security review incomplete: session failed, ended early or omitted a valid final line; see Session log".to_string(), + }), + } + } + + pub fn hold(&self) -> Option { + match self { + Self::Complete(review) if review.findings.is_empty() => None, + Self::Complete(review) => Some(Hold(format!( + "Security review left {} unaddressed introduced finding(s): {}", + review.findings.len(), + review.findings.join("; ") + ))), + Self::Incomplete(reason) => Some(Hold(reason.clone())), + } + } + + pub fn entry(&self) -> String { + match self { + Self::Complete(review) if review.findings.is_empty() => { + "No unaddressed introduced findings.".to_string() + } + Self::Complete(review) => review + .findings + .iter() + .map(|title| format!("- {title}\n")) + .collect(), + Self::Incomplete(reason) => format!("- {reason}\n"), + } + } +} + +/// Delivery has pushed and marked the PR ready before raising this hold. +#[derive(Debug)] +pub(crate) struct Hold(pub String); + +impl fmt::Display for Hold { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for Hold {} + +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct Review { + unaddressed_count: usize, + pub findings: Vec, +} + +impl Review { + pub fn from_final_message(message: Option<&str>) -> Result { + let json = message + .and_then(|message| message.lines().rfind(|line| !line.trim().is_empty())) + .and_then(|line| line.strip_prefix("Security review: ")) + .context("Security review ended without its required final line")?; + let review: Self = serde_json::from_str(json) + .context("Security review ended with an invalid final line")?; + if review.unaddressed_count != review.findings.len() + || review + .findings + .iter() + .any(|title| title.trim().is_empty() || title.contains(['\r', '\n'])) + { + bail!("Security review ended with an inconsistent finding count or title"); + } + Ok(review) + } +} diff --git a/src/session.rs b/src/session.rs index 9b763a9..0e1307a 100644 --- a/src/session.rs +++ b/src/session.rs @@ -9,7 +9,7 @@ use std::time::{Duration, Instant}; #[cfg(test)] use anyhow::bail; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; #[cfg(test)] pub(crate) use crate::harness::claude::claude_args; @@ -214,6 +214,11 @@ impl<'a> Sessions<'a> { } if !ended.killed.is_empty() { let ending = ending_with(&ended.killed_work()); + if matches!(purpose, Purpose::Security) { + return Err(anyhow!( + "{kind}: the {ended_last} {ending}; Security session incomplete" + )); + } self.step(format!( "{kind}: the {ended_last} {ending}; carrying on, as it may have been abandoned" )); @@ -586,6 +591,31 @@ mod tests { assert_eq!(log, Some(logs().path("implement-resume"))); } + #[test] + fn a_security_session_cannot_claim_completion_with_killed_work() { + for id in [None, Some("s1")] { + let mut endings = vec![ended( + id, + &["cargo test"], + "Security review: {\"unaddressed_count\":0,\"findings\":[]}", + )]; + if id.is_some() { + endings.push(ended( + id, + &["cargo test"], + "Security review: {\"unaddressed_count\":0,\"findings\":[]}", + )); + } + let (taken, _, _) = take(endings, |sessions| { + sessions.run_to_final_message(Purpose::Security, "security-review", "review") + }); + assert!( + taken.is_err(), + "unfinished security work must hold Self-merge" + ); + } + } + #[test] fn a_resume_that_ends_the_same_way_gets_no_second_and_a_later_failure_names_the_work() { let (taken, log, calls) = take( diff --git a/src/setup.rs b/src/setup.rs index 0407f1f..1ed4ce5 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -719,6 +719,7 @@ mod tests { const MERGE: &str = "Every Run a Merge run?"; const BASE_FIX: &str = "Every Run may start a Base fix when the Base branch's CI is red?"; const PULL: &str = "fast-forward"; + const SECURITY_REVIEW: &str = "Runs review their change for Security findings?"; const SECURITY_FIX: &str = "Security runs may fix reproduced findings?"; const NOTIFY: &str = "Run notifications, an email"; const TO: &str = "Send Run notifications to"; @@ -734,8 +735,13 @@ mod tests { const KEY: &str = "re_secret_123"; /// The answers that take every default and turn nothing on. - const ENTER_THROUGHOUT: [(&str, &str); 4] = - [(MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), (NOTIFY, "")]; + const ENTER_THROUGHOUT: [(&str, &str); 5] = [ + (MERGE, ""), + (PULL, ""), + (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), + (NOTIFY, ""), + ]; /// The answers that turn Run notifications on, to `me@example.com` from /// the default sender, then `rest`. @@ -746,6 +752,7 @@ mod tests { (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, ""), @@ -1048,6 +1055,7 @@ to = \"me@example.com\" # my inbox "Every Run a Merge run? [y/N] ", "Every Run first fast-forwards your checkout of the Base branch? [y/N] ", "Security runs may fix reproduced findings? [y/N] ", + "Runs review their change for Security findings? [y/N] ", "Run notifications, an email as each Run ends? [y/N] ", ] ); @@ -1062,6 +1070,7 @@ to = \"me@example.com\" # my inbox (BASE_FIX, "y"), (PULL, "yes"), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, "ts@acme.dev"), @@ -1133,6 +1142,7 @@ limit = 5 [security] harness = \"codex\" fix = false +review = false [harness] default = \"claude\" @@ -1170,6 +1180,7 @@ effort = \"\" (BASE_FIX, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), (TO, ""), (FROM, ""), @@ -1191,6 +1202,7 @@ effort = \"\" "Every Run may start a Base fix when the Base branch's CI is red? [Y/n] ", "Every Run first fast-forwards your checkout of the Base branch? [Y/n] ", "Security runs may fix reproduced findings? [y/N] ", + "Runs review their change for Security findings? [y/N] ", "Run notifications, an email as each Run ends? [Y/n] ", "Send Run notifications to [mine@example.net]: ", "Send them from [ts@acme.dev]: ", @@ -1203,7 +1215,13 @@ effort = \"\" fn on_a_terminal_changed_answers_are_written_over_the_user_config() { let mut outside = Scripted { user_config: Some(DEFAULTS.to_string()), - ..Scripted::answering(&[(MERGE, ""), (PULL, "y"), (SECURITY_FIX, ""), (NOTIFY, "")]) + ..Scripted::answering(&[ + (MERGE, ""), + (PULL, "y"), + (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), + (NOTIFY, ""), + ]) }; let done = setup(&mut outside).unwrap(); @@ -1235,6 +1253,7 @@ always = false # quiet, please (BASE_FIX, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, ""), @@ -1341,6 +1360,7 @@ always = false # quiet, please (BASE_FIX, answer), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ]); @@ -1366,6 +1386,7 @@ always = false # quiet, please (MERGE, answer), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ]); @@ -1386,6 +1407,7 @@ always = false # quiet, please (BASE_FIX, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ]) }; @@ -1407,7 +1429,13 @@ always = false # quiet, please fn turning_merging_off_writes_base_fix_at_its_default() { let mut outside = Scripted { user_config: Some("[merge]\nalways = true\n\n[base]\nfix = true # mine\n".to_string()), - ..Scripted::answering(&[(MERGE, "n"), (PULL, ""), (SECURITY_FIX, ""), (NOTIFY, "")]) + ..Scripted::answering(&[ + (MERGE, "n"), + (PULL, ""), + (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), + (NOTIFY, ""), + ]) }; setup(&mut outside).unwrap(); @@ -1425,6 +1453,7 @@ always = false # quiet, please (BASE_FIX, "No"), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ]); @@ -1439,6 +1468,7 @@ always = false # quiet, please (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, ""), (TO, "me.example.com"), @@ -1467,7 +1497,13 @@ always = false # quiet, please let mut outside = Scripted { key: Ok(Some(Source::Credentials(CREDENTIALS.into()))), github_email: Ok(Some("octo@example.com")), - ..Scripted::answering(&[(MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), (NOTIFY, "n")]) + ..Scripted::answering(&[ + (MERGE, ""), + (PULL, ""), + (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), + (NOTIFY, "n"), + ]) }; setup(&mut outside).unwrap(); @@ -1710,6 +1746,7 @@ always = false # quiet, please (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, ""), (FROM, ""), @@ -1811,6 +1848,7 @@ always = false # quiet, please (BASE_FIX, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ]); @@ -1830,6 +1868,7 @@ always = false # quiet, please (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, ""), @@ -1928,6 +1967,7 @@ always = false # quiet, please (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, ""), diff --git a/src/setup/questions.rs b/src/setup/questions.rs index f9a90be..0fb5c21 100644 --- a/src/setup/questions.rs +++ b/src/setup/questions.rs @@ -28,6 +28,7 @@ pub struct Answers { pub launch_pull: bool, /// `security.fix`: Security runs may fix reproduced findings. pub security_fix: bool, + pub security_review: bool, /// With Run notifications on, `email.always`, their settings; with them /// off, `None`, and the email settings stay as they were. pub notifications: Option, @@ -41,6 +42,7 @@ impl Answers { base_fix: self.base_fix, launch_pull: self.launch_pull, security_fix: self.security_fix, + security_review: self.security_review, notifications: self .notifications .as_ref() @@ -126,6 +128,11 @@ pub fn ask( "Security runs may fix reproduced findings?", current.security_fix.unwrap_or(false), )?; + let security_review = yes_or_no( + outside, + "Runs review their change for Security findings?", + current.security_review, + )?; if !yes_or_no( outside, "Run notifications, an email as each Run ends?", @@ -137,6 +144,7 @@ pub fn ask( base_fix, launch_pull, security_fix, + security_review, notifications: None, }); } @@ -179,6 +187,7 @@ pub fn ask( base_fix, launch_pull, security_fix, + security_review, notifications: Some(Notifications { to, from, diff --git a/src/spec_run.rs b/src/spec_run.rs index 6ce7b6a..97c7fcd 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -196,6 +196,7 @@ struct ChildRunsAndGitHub<'a> { delivery: Option>, base_fix: BaseFixAsk, security_fix: bool, + security_review: bool, harness: &'a Choice, spec_pr: PullRequest, } @@ -203,13 +204,17 @@ struct ChildRunsAndGitHub<'a> { 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 { + fn new(worktree: Worktree, mut delivery: Delivery<'a>, spec_pr: PullRequest) -> Self { + let security_review = delivery.security_review; + // The Spec PR review is implemented by #551; Base fixes inherit the choice. + delivery.security_review = false; Self { spec: delivery.issue, children: Runs::default(), worktree: Some(worktree), base_fix: delivery.base_fix.ask_of_tickets(), security_fix: delivery.security_fix, + security_review, harness: delivery.harness, delivery: Some(delivery), spec_pr, @@ -238,7 +243,10 @@ impl Outside for ChildRunsAndGitHub<'_> { &ticket, kind, self.base_fix.clone(), - self.security_fix, + crate::security::Options { + fix: self.security_fix, + review: self.security_review, + }, self.harness, ) } diff --git a/tests/muse_sessions.rs b/tests/muse_sessions.rs index 4a77d71..f30e6eb 100644 --- a/tests/muse_sessions.rs +++ b/tests/muse_sessions.rs @@ -298,6 +298,7 @@ fn setup_checks_muses_proposed_model_with_the_same_flags_and_environment() { ("Every Run a Merge run", ""), ("Every Run first fast-forwards", ""), ("Security runs may fix reproduced findings?", ""), + ("Runs review their change for Security findings?", ""), ("Run notifications", ""), ], ); diff --git a/tests/opencode_sessions.rs b/tests/opencode_sessions.rs index c14fb09..756d497 100644 --- a/tests/opencode_sessions.rs +++ b/tests/opencode_sessions.rs @@ -407,6 +407,7 @@ fn setup_proposes_no_model_and_checks_the_answer_standalone() { ("Every Run a Merge run", ""), ("Every Run first fast-forwards", ""), ("Security runs may fix reproduced findings?", ""), + ("Runs review their change for Security findings?", ""), ("Run notifications", ""), ], ); @@ -484,6 +485,7 @@ fn setup_retries_a_refused_model_and_effort_with_the_same_preflight_check() { ("Every Run a Merge run", ""), ("Every Run first fast-forwards", ""), ("Security runs may fix reproduced findings?", ""), + ("Runs review their change for Security findings?", ""), ("Run notifications", ""), ], ); diff --git a/tests/security_review.rs b/tests/security_review.rs new file mode 100644 index 0000000..7b5d118 --- /dev/null +++ b/tests/security_review.rs @@ -0,0 +1,330 @@ +mod support; + +use support::Scenario; + +const OPENS_PR: &str = r#" +echo feature > feature.txt +git add feature.txt +git commit -q -m 'Add feature' +gh pr create --base main --head issue-7 --title 'Add feature' --body 'Closes #7' +"#; + +const CLEAN: &str = r#"printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#; + +#[test] +fn command_words_override_the_config_and_review_is_off_by_default() { + for (config, word, enabled) in [ + ("", None, false), + ("[security]\nreview = true\n", None, true), + ( + "[security]\nreview = true\n", + Some("no-security-review"), + false, + ), + ( + "[security]\nreview = false\n", + Some("security-review"), + true, + ), + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, CLEAN); + let url = scenario.issue_url(7); + let mut args = vec![url.as_str(), "merge"]; + args.extend(word); + let result = scenario.run(&args); + assert_eq!(result.code, Some(0), "{config} {word:?}: {}", result.stderr); + assert_eq!(scenario.claude_calls().len(), if enabled { 2 } else { 1 }); + assert_eq!(scenario.gh_state()["prs"][0]["state"], "MERGED"); + } +} + +#[test] +fn invalid_or_missing_final_lines_hold_self_merge_but_report_only_runs_continue() { + for final_message in [ + "Review incomplete", + "Security review: {}", + r#"Security review: {"unaddressed_count":0,"findings":["Cross-tenant read"]}"#, + r#"Security review: {"unaddressed_count":1,"findings":[""]}"#, + r#"Security review: {"unaddressed_count":0,"findings":[]}\nMore text"#, + ] { + for merge in [true, false] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + // Write literal data as a file through the existing fixture. + std::fs::write(scenario.path("review-final.txt"), final_message).unwrap(); + scenario.agent_does_in_session(2, r#"cat "$(dirname "$FAKE_CLAUDE_SCRIPT")/review-final.txt" > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + if merge { "merge" } else { "no-merge" }, + ]); + assert_eq!( + result.code, + Some(if merge { 1 } else { 0 }), + "{final_message}: {}", + result.stderr + ); + assert!( + result.stderr.contains("Security review incomplete"), + "{}", + result.stderr + ); + let pr = &scenario.gh_state()["prs"][0]; + assert_eq!(pr["state"], "OPEN"); + assert_eq!(pr["isDraft"], false); + assert!( + pr["body"] + .as_str() + .unwrap() + .contains("Security review incomplete") + ); + } + } +} + +#[test] +fn safeguard_refusals_hold_self_merge_without_publishing_private_diagnostics() { + for (harness, script, cause) in [ + ( + "claude", + r#"printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, + "Claude Code's [cyber] safeguard refusal", + ), + ( + "codex", + r#"printf '%s\n' 'Cybersecurity safeguard refused: Private refusal evidence' > "$FAKE_CODEX_ERROR" +exit 1"#, + "Codex's cybersecurity safeguard refusal", + ), + ] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, script); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + "merge", + "harness", + harness, + ]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!(result.stderr.contains(cause), "{}", result.stderr); + let pr = &scenario.gh_state()["prs"][0]; + assert_eq!(pr["state"], "OPEN"); + assert_eq!(pr["isDraft"], false); + let body = pr["body"].as_str().unwrap(); + assert!(body.contains(cause), "{body}"); + assert!(!body.contains("Private refusal evidence")); + } +} + +#[test] +fn base_fixes_inherit_the_security_review_choice() { + let scenario = Scenario::new(); + scenario.agent_does_for_in_session( + 7, + 1, + &format!( + r#"{OPENS_PR} +gh fake checks "$(git rev-parse HEAD)" '[{{"name":"test","conclusion":"failure"}}]' +gh fake checks "$(git rev-parse origin/main)" '[{{"name":"test","conclusion":"failure"}}]' +"# + ), + ); + scenario.agent_does_for_in_session(7, 2, CLEAN); + scenario.agent_does_for_in_session( + 8, + 1, + r#" +echo fixed > ci-fix.txt +git add ci-fix.txt +git commit -q -m 'Fix base CI' +gh pr create --base main --head issue-8 --title 'Fix base CI' --body 'Closes #8' +gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}]' +"#, + ); + scenario.agent_does_for_in_session(8, 2, CLEAN); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "base-fix"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 4); + assert!( + calls[3]["prompt"] + .as_str() + .unwrap() + .contains("git diff main...HEAD") + ); + assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); +} + +#[test] +fn an_unaddressed_introduced_finding_holds_self_merge_with_the_pr_ready() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("Cross-tenant read"), + "{}", + result.stderr + ); + let pr = &scenario.gh_state()["prs"][0]; + assert_eq!(pr["state"], "OPEN"); + assert_eq!(pr["isDraft"], false); + assert!(pr["body"].as_str().unwrap().contains("### Security")); + assert!(pr["body"].as_str().unwrap().contains("Cross-tenant read")); +} + +#[test] +fn enabled_review_fixes_reach_origin_before_delivery_finishes() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + &format!( + r#" +echo checked > security-fix.txt +git add security-fix.txt +git commit -q -m 'Fix reproduced finding' +{CLEAN} +"# + ), + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!( + scenario + .origin_file("issue-7", "security-fix.txt") + .as_deref(), + Some("checked\n") + ); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2); + let prompt = calls[1]["prompt"].as_str().unwrap(); + assert!(prompt.contains("guidance mode")); + assert!(prompt.contains("Do not delegate")); + assert!(prompt.contains("git diff main...HEAD")); +} + +// Regression A. +#[test] +fn review_regression_a_security_hold_still_repairs_red_ci() { + 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' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$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 result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("Cross-tenant read"), + "{}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + 3, + "The held Merge run should repair red CI before handing over its PR: {}", + result.stderr + ); + assert_eq!( + scenario.origin_file("issue-7", "ci-fixed.txt").as_deref(), + Some("fixed\n") + ); + let state = scenario.gh_state(); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); +} + +// Regression B. +#[test] +fn review_regression_b_body_write_failure_preserves_refusal_and_readiness() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + r#" +gh fake fails 'api --method PATCH repos/acme/widgets/pulls/1' +printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let state = scenario.gh_state(); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!( + state["prs"][0]["isDraft"], false, + "A refused review must leave the already-ready PR ready when only its summary PATCH fails: {}", + result.stderr + ); + assert!( + result.stderr.contains("Security review refused"), + "The failure must retain the Security refusal cause: {}", + result.stderr + ); +} + +// Regression C. +#[test] +fn review_regression_c_continuation_replaces_the_previous_security_outcome() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, + ); + let first = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); + assert_eq!(first.code, Some(0), "{}", first.stderr); + assert!( + scenario.gh_state()["prs"][0]["body"] + .as_str() + .unwrap() + .contains("Cross-tenant read") + ); + scenario.agent_does_in_session( + 3, + r#" +echo fixed > feature.txt +git add feature.txt +git commit -q -m 'Fix the previously reported finding' +"#, + ); + scenario.agent_does_in_session(4, CLEAN); + let second = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(second.code, Some(0), "{}", second.stderr); + let state = scenario.gh_state(); + assert_eq!(state["prs"][0]["state"], "MERGED"); + let body = state["prs"][0]["body"].as_str().unwrap(); + assert!( + !body.contains("Cross-tenant read"), + "The fixed finding remains falsely listed as unaddressed after a clean review: {body}" + ); + assert_eq!( + body.matches("### Security review outcome").count(), + 1, + "{body}" + ); +} diff --git a/tests/setup.rs b/tests/setup.rs index d598b8a..5ce8334 100644 --- a/tests/setup.rs +++ b/tests/setup.rs @@ -188,6 +188,7 @@ const EFFORT: &str = "Effort for claude"; const MERGE: &str = "Merge run?"; const BASE_FIX: &str = "Every Run may start a Base fix when the Base branch's CI is red?"; const PULL: &str = "fast-forward"; +const SECURITY_REVIEW: &str = "Runs review their change for Security findings?"; const SECURITY_FIX: &str = "Security runs may fix reproduced findings?"; const NOTIFY: &str = "Run notifications, an email"; const TO: &str = "Send Run notifications to"; @@ -198,6 +199,34 @@ const KEPT: &str = "Resend API key (input hidden, Enter keeps the saved one):"; const WROTE_CREDENTIALS: &str = "wrote the Credentials"; const KEY: &str = "re_secret_123"; +#[test] +fn setup_can_enable_security_review_and_keeps_it_as_the_next_default() { + let scenario = Scenario::new(); + for (answer, choices) in [("y", "[y/N]"), ("", "[Y/n]")] { + let result = setup_on_terminal( + &scenario, + &[], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, ""), + (PULL, ""), + (SECURITY_FIX, ""), + (SECURITY_REVIEW, answer), + (NOTIFY, ""), + ], + ); + assert!( + result + .stderr + .contains(&format!("{SECURITY_REVIEW} {choices}")) + ); + let config: toml::Table = result.user_config.unwrap().parse().unwrap(); + assert_eq!(config["security"]["review"].as_bool(), Some(true)); + } +} + #[test] fn setup_asks_once_about_security_fixing_with_off_as_the_default_and_writes_yes() { let scenario = Scenario::new(); @@ -211,6 +240,7 @@ fn setup_asks_once_about_security_fixing_with_off_as_the_default_and_writes_yes( (MERGE, ""), (PULL, ""), (SECURITY_FIX, "y"), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ], ); @@ -244,6 +274,7 @@ fn rerunning_setup_keeps_security_answers_as_defaults_and_can_turn_fixing_off() (MERGE, ""), (PULL, ""), (SECURITY_FIX, answer), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ], ); @@ -407,6 +438,7 @@ fn cancelling_later_questions_after_successful_or_repeated_checks_restores_echo_ TerminalStep::line(MERGE, ""), TerminalStep::line(PULL, ""), TerminalStep::line(SECURITY_FIX, ""), + TerminalStep::line(SECURITY_REVIEW, ""), TerminalStep::line(NOTIFY, "y"), TerminalStep::line(TO, "me@example.com"), TerminalStep::line(FROM, ""), @@ -448,7 +480,10 @@ fn pasted_answers_are_left_available_to_later_terminal_questions() { let result = setup_on_terminal( &scenario, &[], - &[(HARNESS, " claude \n\n\n\n\n\ny\n café@example.com \n\n")], + &[( + HARNESS, + " claude \n\n\n\n\n\n\ny\n café@example.com \n\n", + )], ); let config: toml::Table = result.user_config.unwrap().parse().unwrap(); assert_eq!(config["harness"]["default"].as_str(), Some("claude")); @@ -490,6 +525,7 @@ fn a_partial_final_terminal_answer_is_trimmed_and_accepted_before_eof() { TerminalStep::line(MERGE, ""), TerminalStep::line(PULL, ""), TerminalStep::line(SECURITY_FIX, ""), + TerminalStep::line(SECURITY_REVIEW, ""), // Queue EOF for both the partial answer and the next question. // macOS can finish both reads before another input action runs. TerminalStep::bytes(NOTIFY, b" y \x04\x04\x04"), @@ -530,6 +566,7 @@ fn notifications_on<'a>(rest: &[Keystrokes<'a>]) -> Vec> { (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, "me@example.com"), (FROM, ""), @@ -556,6 +593,7 @@ fn on_a_terminal_pressing_enter_throughout_writes_what_setup_with_no_terminal_wr (MERGE, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ], ); @@ -671,6 +709,7 @@ fn ctrl_c_during_the_questions_leaves_an_existing_user_config_unchanged() { (MERGE, "n"), (PULL, "y"), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, "y"), (TO, CTRL_C), ], diff --git a/tests/setup_offer.rs b/tests/setup_offer.rs index bf6427f..e9c7bac 100644 --- a/tests/setup_offer.rs +++ b/tests/setup_offer.rs @@ -28,6 +28,7 @@ const EFFORT: &str = "Effort for claude"; const MERGE: &str = "Merge run?"; const BASE_FIX: &str = "Every Run may start a Base fix when the Base branch's CI is red?"; const PULL: &str = "fast-forward"; +const SECURITY_REVIEW: &str = "Runs review their change for Security findings?"; const SECURITY_FIX: &str = "Security runs may fix reproduced findings?"; const NOTIFY: &str = "Run notifications, an email"; const KEY: &str = "re_secret_123"; @@ -84,6 +85,7 @@ fn accepting_and_choosing_merge_always_makes_that_run_a_merge_run() { (BASE_FIX, ""), (PULL, ""), (SECURITY_FIX, ""), + (SECURITY_REVIEW, ""), (NOTIFY, ""), ], );