From a41f804cff6e9aa28b1bfbb03382851e334b7fea Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 11:20:53 -0400 Subject: [PATCH 1/4] Allow Security runs to dispatch one reproduced fix (#543) --- README.md | 13 +- prompts/README.md | 1 + prompts/security-fix.md | 20 ++ site/prompts/index.html | 18 ++ src/args.rs | 17 +- src/asks.rs | 32 +++ src/base_fix.rs | 21 +- src/child_run.rs | 18 ++ src/child_run/execution_tests.rs | 1 + src/child_run/runs.rs | 3 +- src/command.rs | 4 + src/config.rs | 9 + src/delivery.rs | 1 + src/github/advisories.rs | 164 +++++++++++++-- src/main.rs | 14 +- src/pass.rs | 79 ++++++- src/prompt.rs | 15 ++ src/prompts_page.rs | 7 + src/run.rs | 5 + src/run_ending.rs | 4 + src/security.rs | 152 ++++++++++++-- src/security/fixing.rs | 104 ++++++++++ src/spec_run.rs | 11 +- tests/security_run.rs | 346 +++++++++++++++++++++++++++++++ 24 files changed, 1017 insertions(+), 42 deletions(-) create mode 100644 prompts/security-fix.md create mode 100644 src/security/fixing.rs diff --git a/README.md b/README.md index b54655e4..10fbec28 100644 --- a/README.md +++ b/README.md @@ -251,6 +251,7 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 [security] +fix = false # Security runs may fix reproduced findings; default false harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code [harness] @@ -735,7 +736,7 @@ On a repository that also has a [Pickup line](#a-pickup-run-on-a-schedule), put ## Security runs -`thirdshift secure` starts a report-only **Security run**: it audits the **Base branch** at its head on `origin` and records each **Security finding** privately as a draft repository security advisory. Start it from a clone on the Base branch, or name that branch from any checkout: +`thirdshift secure` starts a **Security run**, report-only by default: it audits the **Base branch** at its head on `origin` and records each **Security finding** privately as a draft repository security advisory. Start it from a clone on the Base branch, or name that branch from any checkout: ```sh thirdshift secure @@ -743,7 +744,11 @@ thirdshift secure base main harness claude thirdshift secure base main harness codex model gpt-6.1-sol effort high ``` -The command takes the Harness, Model and Effort words and the shared Pass words (`merge`, `no-merge`, `base-fix`, `no-base-fix`, `parallel`, `email` and `no-email`, with or without dashes). After recording findings, it tries to reproduce each untriaged one in a fresh throwaway worktree at the recorded audited commit, and keeps the outcome, severity when reproduced, notes and test in the private record. It preserves findings already triaged by the Day shift. It dispatches no fix and never commits, pushes, publishes or closes an advisory; the fix-related options are reserved for later Security run work. +The command takes the Harness, Model and Effort words and the shared Pass words (`merge`, `no-merge`, `base-fix`, `no-base-fix`, `parallel`, `email`, `no-email`, `security-fix` and `no-security-fix`, with or without dashes). After recording findings, it tries to reproduce each untriaged one in a fresh throwaway worktree at the recorded audited commit, and keeps the outcome, severity when reproduced, notes and test in the private record. It preserves findings already triaged by the Day shift. It never publishes or closes an advisory. + +Fixing is off by default. `security-fix` on the command or `fix = true` under `[security]` allows one fix; `no-security-fix` overrides the setting. Both words are accepted on every command that starts Runs and passed to those Runs. With fixing allowed, a Security run takes the most severe reproduced finding still awaiting a fix before auditing, with ties in private-record order. After an audit reproduces findings, it goes on to the most severe one. + +A publishing session reads the private record and publishes one terse **Ticket**, labelled `needs-triage`, saying only what the fix changes and linking the record. thirdshift checks the Ticket, swaps `needs-triage` for `ready-for-agent`, adds `security-fix` (creating the label when missing), records the Ticket link privately and dispatches its **Run**. The Security run ends as that Run ends, including a **Merge run** when asked by the command or the User config. Every fix is a single Ticket for now; the write-up and proof-of-concept stay in the private record. `email`, optionally followed by an address, or `email.always` in the User config asks for one **Run notification** when the Security run ends, whether the audit succeeded, failed or was interrupted. `no-email` overrides the default. The notification says how the audit ended and lists each finding it recorded or matched to an existing private record: its severity when known, title and private link. It includes no finding write-up, trace or evidence, since it passes through Resend. Detailed failure causes stay in the local logs; the notification gives the audit's status and log paths. A recording failure still lists the records reached before it failed. A skipped Security run sends none. The usual address and Resend API key checks run before the skip checks or any work; a failed send is a warning and never changes the run's outcome. @@ -751,13 +756,13 @@ A Security run chooses its Harness from the command's `harness` word, then `[sec Codex security sessions and their Resumes set `agents.max_concurrent_threads_per_session=8` and ask for fresh sub-agents with `fork_turns: "none"`, so the skill's verifiers stay independent. -A Security run checks its gates in order: another **Pass** on the repository running on the machine, a **Ready issue**, a **Security finding** waiting for the **Day shift**, then an unchanged **Base branch** since the last completed Security audit. A draft advisory with no severity waits; closing it, publishing it or assigning severity ends that wait. On a private repository, an open finding issue with `needs-triage` waits until that label is removed or the issue is closed. The audited commit comes from the skill's own `run-metadata.json` under the audit root, with no separate state file. A skip exits `0`, starts no session, keeps no Command log and sends no Run notification; its reason goes into the repository's **Activity log** only when it changes, with the usual `activity.quiet_skips` behavior. Before starting work, thirdshift checks the chosen Harness and that **Node.js** is on `PATH`, since the embedded skill's report validators require it. +A Security run checks its gates in order: another **Pass** on the repository running on the machine, a **Ready issue**, a **Security finding** waiting for the **Day shift**, then an unchanged **Base branch** since the last completed Security audit. An unreproduced draft advisory with no severity waits; closing it, publishing it or assigning severity ends that wait. A reproduced finding waits while fixing is disabled until its record is closed or published, or its fix Ticket is closed. On a private repository, an open finding issue with `needs-triage` waits until that label is removed or the issue is closed. The audited commit comes from the skill's own `run-metadata.json` under the audit root, with no separate state file. A skip exits `0`, starts no session, keeps no Command log and sends no Run notification; its reason goes into the repository's **Activity log** only when it changes, with the usual `activity.quiet_skips` behavior. Before starting work, thirdshift checks the chosen Harness and that **Node.js** is on `PATH`, since the embedded skill's report validators require it. The Security audit runs in a throwaway worktree detached at origin's Base branch head, leaving the Launch directory and local work untouched, including when `launch.pull` is set. Its Session prompt runs `thirdshift-security-audit` in full audit mode with the `quick` profile, auditing the whole repository except vendored and third-party code. It names a `SECURITY.md` or conventional threat-model document when present, reads compatible earlier runs and ends `incomplete` instead of asking for input. Each audit keeps a new output directory under `///audits/`, outside the worktree. The audit artifacts and private advisory descriptions contain evidence; keep the logs directory private. thirdshift runs both embedded validators before recording anything. A missing final line, an incomplete session or an invalid report fails the run and names its Session log. Command logs are in `commands/secure/`; Session logs are named `secure--security-audit.jsonl`; the Activity log records the start and ending as for other Passes. -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. Private-repository fallback is later work; this command fails if the advisory endpoint is unavailable, without publishing a finding as an issue. +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. ## Pickup runs diff --git a/prompts/README.md b/prompts/README.md index 07907f3a..c5bbc825 100644 --- a/prompts/README.md +++ b/prompts/README.md @@ -28,6 +28,7 @@ With `harness codex`, each session runs `codex exec --json --dangerously-bypass- | 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) | +| Security fix publishing | With fixing allowed, publishes one terse Ticket for the most severe reproduced finding. thirdshift checks and marks it ready before dispatching its Run. | [security-fix.md](security-fix.md) | | Conflict Repair, on Foreign commits | In a Merge run, starts a Repair session when merging Foreign commits from the Issue branch on origin into the local one leaves conflicts. | [foreign-conflict-repair.md](foreign-conflict-repair.md) | | Review Repair | In a Merge run, starts a Repair session once Foreign commits are merged into the Issue branch, to review them from the head the Run last knew as its own before they can be merged. | [review-repair.md](review-repair.md) | | CI-fix Repair | Starts a Repair session when CI fails on the pull request's head commit, listing each failed check. | [ci-fix-repair.md](ci-fix-repair.md) | diff --git a/prompts/security-fix.md b/prompts/security-fix.md new file mode 100644 index 00000000..efc86495 --- /dev/null +++ b/prompts/security-fix.md @@ -0,0 +1,20 @@ + + +# Security fix publishing + +With fixing allowed, publishes one terse Ticket for the most severe reproduced finding. thirdshift checks and marks it ready before dispatching its Run. + +``` +Publish the fix for this reproduced Security finding on Base branch ``. +Read the private record at and the record below. +Publish exactly one new, terse Ticket in this repository, labelled `needs-triage`. Create that label if missing. +The Ticket says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record. +For now every fix is a single Ticket, even if the reproduction suggested a Spec. Publish no Spec or sub-issues. +This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks the Ticket, marks it ready, labels it security-fix and dispatches its Run. +End your final message with exactly `Security fix Ticket: `. + +Private record: + + +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 94d9a73e..a4c34334 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -207,6 +207,24 @@

Conflict Repair

Resolve the conflicts, finish the merge, and push <branch>. Do not rebase or force-push. +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 fix publishing

+

With fixing allowed, publishes one terse Ticket for the most severe reproduced finding. thirdshift checks and marks it ready before dispatching its Run.

+

Sent by a Security run, before any unit

+ 
Publish the fix for this reproduced Security finding on Base branch `<base>`.
+Read the private record at <private record URL> and the record below.
+Publish exactly one new, terse Ticket in this repository, labelled `needs-triage`. Create that label if missing.
+The Ticket says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record.
+For now every fix is a single Ticket, even if the reproduction suggested a Spec. Publish no Spec or sub-issues.
+This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks the Ticket, marks it ready, labels it security-fix and dispatches its Run.
+End your final message with exactly `Security fix Ticket: <Issue URL>`.
+
+Private record:
+<recorded Security finding>
+
 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 a3a4abd1..f059e364 100644 --- a/src/args.rs +++ b/src/args.rs @@ -58,7 +58,7 @@ pub struct PassArgs { pub base: Option, /// The flags it shares with a Run: `email` and `no-email` for its own /// Run notification, the rest for a run the Pass dispatches. The - /// report-only Security run reserves them until those stages are built. + /// Security run also uses `security-fix` for its own choice of work. pub flags: Flags, } @@ -246,6 +246,18 @@ fn take_flag<'a>( "no-base-fix" | "--no-base-fix" => { ask_once(&mut flags.base_fix, BaseFixAsk::Forbid, arg, BASE_FIX_FLAGS)? } + "security-fix" | "--security-fix" => ask_once( + &mut flags.security_fix, + crate::security::FixAsk::Allow, + arg, + SECURITY_FIX_FLAGS, + )?, + "no-security-fix" | "--no-security-fix" => ask_once( + &mut flags.security_fix, + crate::security::FixAsk::Forbid, + arg, + SECURITY_FIX_FLAGS, + )?, "harness" | "--harness" => ask_harness(&mut flags.harness.harness, arg, rest.next())?, "model" | "--model" => { let model = &mut flags.harness.model_and_effort.model; @@ -263,6 +275,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_FIX_FLAGS: &str = "security-fix and no-security-fix"; /// Record in `given` what the flag `arg` asked for: the same kind of ask /// twice is a repeated argument, and a different one contradicts the first, @@ -724,6 +737,7 @@ mod tests { let all = PassArgs { base: Some("develop".to_string()), flags: Flags { + security_fix: None, goal: Some(Goal::Merged), email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))), parallel: NonZeroUsize::new(2), @@ -778,6 +792,7 @@ mod tests { let cautious = PassArgs { base: None, flags: Flags { + security_fix: None, goal: Some(Goal::ReadyForReview), email: Some(NotificationAsk::Skip), parallel: None, diff --git a/src/asks.rs b/src/asks.rs index 42ba8c3f..56ec5661 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -26,6 +26,8 @@ pub struct Flags { pub parallel: Option, /// What `base-fix` or `no-base-fix` asked for. pub base_fix: Option, + /// What `security-fix` or `no-security-fix` asked for. + pub security_fix: Option, /// What `harness `, `model ` and `effort ` asked for. pub harness: harness::Asked, } @@ -44,6 +46,8 @@ pub struct Asks { pub parallel_asked: bool, /// What it is asked about a Base fix. pub base_fix: BaseFixAsk, + /// Whether a Security run may fix reproduced findings, passed to child Runs. + pub security_fix: bool, /// Whether it first brings the Launch directory's checkout of the Base /// branch up to date with origin. pub launch_pull: bool, @@ -77,6 +81,7 @@ impl Asks { tickets_at_once: flags.parallel.unwrap_or(config.spec_parallel), parallel_asked: flags.parallel.is_some(), base_fix, + security_fix: flags.security_fix_allowed(config), launch_pull: config.launch_pull, harness: flags.harness(config), } @@ -96,6 +101,7 @@ impl Asks { tickets_at_once: config.spec_parallel, parallel_asked: false, base_fix: given.base_fix.clone(), + security_fix: given.security_fix, launch_pull: false, harness: given.harness.clone(), } @@ -144,6 +150,13 @@ impl Asks { } impl Flags { + pub fn security_fix_allowed(&self, config: &UserConfig) -> bool { + match self.security_fix { + Some(crate::security::FixAsk::Allow) => true, + Some(crate::security::FixAsk::Forbid) => false, + None => config.security_fix.unwrap_or(false), + } + } /// Whether any flag here is for the Spec run or Run that a Pass /// dispatches: every one but `email` and `no-email`, /// which ask that run for its own Run notification. @@ -208,6 +221,11 @@ fn retry_with_base_fix(issue: &IssueUrl, flags: &Flags) -> String { if let Some(parallel) = flags.parallel { command += &format!(" parallel {parallel}"); } + match flags.security_fix { + Some(crate::security::FixAsk::Allow) => command += " security-fix", + Some(crate::security::FixAsk::Forbid) => command += " no-security-fix", + None => {} + } let asked = &flags.harness; if let Some(harness) = asked.harness { command += &format!(" harness {}", harness.name()); @@ -258,6 +276,7 @@ mod tests { pickup_limit: n(3), harness: harness::Settings::default(), security_harness: None, + security_fix: None, } } @@ -337,6 +356,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: n(3), @@ -355,6 +375,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, notification: NotificationAsk::Send(None), tickets_at_once: n(5), @@ -369,6 +390,7 @@ mod tests { #[test] fn each_flag_decides_its_ask_whatever_the_user_config_says() { let against = Flags { + security_fix: None, goal: Some(Goal::ReadyForReview), email: Some(NotificationAsk::Skip), parallel: Some(n(2)), @@ -379,6 +401,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -391,6 +414,7 @@ mod tests { ); let for_it = Flags { + security_fix: None, goal: Some(Goal::Merged), email: Some(to("flag@example.com")), parallel: Some(n(2)), @@ -401,6 +425,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, notification: to("flag@example.com"), tickets_at_once: n(2), @@ -416,6 +441,7 @@ mod tests { /// fix, its sessions on Claude's `sonnet`. fn given(kind: Kind, base_fix: BaseFixAsk) -> Given { Given { + security_fix: false, kind, stamp: "20261003T120000-0400".to_string(), base_fix, @@ -479,6 +505,7 @@ mod tests { /// dispatches, with `email` for its own Run notification. fn dispatch_flags() -> Flags { Flags { + security_fix: None, goal: Some(Goal::Merged), email: Some(to("flag@example.com")), parallel: Some(n(2)), @@ -493,6 +520,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -509,6 +537,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, // Whatever `email.always` says: the Architect run sends it. notification: NotificationAsk::Skip, @@ -527,6 +556,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, notification: NotificationAsk::Skip, tickets_at_once: n(2), @@ -543,6 +573,7 @@ mod tests { assert_eq!( asks, Asks { + security_fix: false, goal: Goal::Merged, // Whatever `email.always` says: the Pickup run sends it. notification: NotificationAsk::Skip, @@ -627,6 +658,7 @@ mod tests { #[test] 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, goal, email, parallel, diff --git a/src/base_fix.rs b/src/base_fix.rs index 61e9d801..78d1e277 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -116,6 +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, } /// 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) -> Self { + pub fn new(child: Option<&Kind>, ask: BaseFixAsk, harness: Choice, security_fix: bool) -> Self { let (on_inherited_failures, retry) = match (child, ask) { (Some(Kind::BaseFix { .. }), _) => (OnInheritedFailures::IsBaseFix, None), (_, BaseFixAsk::Allow) => (OnInheritedFailures::StartBaseFix, None), @@ -154,6 +155,7 @@ impl BaseFix { taken: None, advice: Vec::new(), harness, + security_fix, } } @@ -218,6 +220,7 @@ impl BaseFix { launch, issue, harness: &harness, + security_fix: self.security_fix, }; self.fix_through(&mut outside, issue, pr_url, base, base_commit, failed) } @@ -470,6 +473,7 @@ struct LaunchAndGitHub<'a> { launch: &'a Git, issue: &'a IssueUrl, harness: &'a Choice, + security_fix: bool, } impl Outside for LaunchAndGitHub<'_> { @@ -527,7 +531,14 @@ impl Outside for LaunchAndGitHub<'_> { let kind = Kind::BaseFix { base: base.to_string(), }; - child_run::start(fix, kind, BaseFixAsk::Forbid, self.harness)?.wait() + child_run::start( + fix, + kind, + BaseFixAsk::Forbid, + self.security_fix, + self.harness, + )? + .wait() } fn pause(&mut self) -> Result<()> { @@ -875,7 +886,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()) + BaseFix::new(None, ask, Choice::default(), false) } fn undecided() -> BaseFixAsk { @@ -1322,7 +1333,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()); + let base_fix = BaseFix::new(Some(&child), ask, Choice::default(), false); assert!(!base_fix.sees_inherited_failures()); assert_eq!(base_fix.ask_of_tickets(), BaseFixAsk::Forbid); } @@ -1330,7 +1341,7 @@ mod tests { spec_branch: "spec-3".to_string(), }; assert!( - BaseFix::new(Some(&ticket), BaseFixAsk::Allow, Choice::default()) + BaseFix::new(Some(&ticket), BaseFixAsk::Allow, Choice::default(), false) .sees_inherited_failures() ); assert!(asked(BaseFixAsk::Forbid).sees_inherited_failures()); diff --git a/src/child_run.rs b/src/child_run.rs index 8a9d0b7b..85311656 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -71,6 +71,7 @@ pub struct Given { pub stamp: String, /// What it is asked about a Base fix. pub base_fix: BaseFixAsk, + pub security_fix: bool, /// The Harness, Model and Effort its sessions run on: the command's /// that started it. pub harness: Choice, @@ -91,6 +92,7 @@ const STAMP: &str = "--stamp"; /// The hidden argument that asks a child Run [`BaseFixAsk::Allow`]. const ALLOW_BASE_FIX: &str = "--allow-base-fix"; +const ALLOW_SECURITY_FIX: &str = "--allow-security-fix"; /// The hidden argument followed by the command that starts the Spec run /// again with a Base fix allowed, for the child Run to offer: it asks @@ -125,6 +127,9 @@ impl Given { BaseFixAsk::Forbid => {} BaseFixAsk::Undecided { retry } => args.extend([OFFER_BASE_FIX, retry]), } + if self.security_fix { + args.push(ALLOW_SECURITY_FIX); + } args.extend([SESSIONS_HARNESS, self.harness.harness.name()]); if let Some(model) = &self.harness.model { args.extend([SESSIONS_MODEL, model]); @@ -144,6 +149,7 @@ pub struct Reader { kind: Option, stamp: Option, base_fix: Option, + security_fix: bool, harness: Option, model: Option, effort: Option, @@ -163,6 +169,12 @@ impl Reader { None => bail!("missing {missing}"), }; match arg { + ALLOW_SECURITY_FIX => { + if self.security_fix { + bail!("repeated argument: {arg}"); + } + self.security_fix = true; + } SPEC_BRANCH | BASE_FIX_INTO => { not_yet_given(&self.kind, arg)?; let base = value("Base branch")?; @@ -218,6 +230,7 @@ impl Reader { let Some(kind) = self.kind else { if self.stamp.is_some() || self.base_fix.is_some() + || self.security_fix || self.harness.is_some() || self.model.is_some() || self.effort.is_some() @@ -237,6 +250,7 @@ impl Reader { kind, stamp, base_fix: self.base_fix.unwrap_or(BaseFixAsk::Forbid), + security_fix: self.security_fix, harness: Choice { harness, model: self.model, @@ -298,12 +312,14 @@ pub fn start( issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, + security_fix: bool, harness: &Choice, ) -> Result { let given = Given { kind, stamp: logs::stamp(), base_fix, + security_fix, harness: harness.clone(), }; start_from(&own_executable()?, issue, &given) @@ -681,6 +697,7 @@ mod tests { kind: ticket(), stamp: STAMPED.to_string(), base_fix: BaseFixAsk::Forbid, + security_fix: false, harness: Choice::default(), }; let Err(error) = start_from(executable, &issue, &given) else { @@ -738,6 +755,7 @@ mod tests { kind: kind.clone(), stamp: STAMPED.to_string(), base_fix, + security_fix: false, 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 eb1996a8..1ce5b06a 100644 --- a/src/child_run/execution_tests.rs +++ b/src/child_run/execution_tests.rs @@ -167,6 +167,7 @@ impl Fixture { }, stamp: "20261003T120000-0400".to_string(), base_fix: BaseFixAsk::Forbid, + security_fix: false, harness: Choice::default(), }, startup, diff --git a/src/child_run/runs.rs b/src/child_run/runs.rs index da58eb5d..2a378a40 100644 --- a/src/child_run/runs.rs +++ b/src/child_run/runs.rs @@ -26,10 +26,11 @@ impl Runs { issue: &IssueUrl, kind: Kind, base_fix: BaseFixAsk, + security_fix: bool, harness: &Choice, ) -> Result<()> { self.start_using(issue.number, || { - super::start(issue, kind, base_fix, harness) + super::start(issue, kind, base_fix, security_fix, harness) }) } diff --git a/src/command.rs b/src/command.rs index b4117825..bce4ba1c 100644 --- a/src/command.rs +++ b/src/command.rs @@ -42,6 +42,10 @@ pub enum Ending { }, /// A Security run completed its audit, or failed. Security(crate::security::Ended), + SecurityFix { + ended: Ended, + findings: Vec, + }, /// A Pass was skipped before any work. Skipped(Skip), } diff --git a/src/config.rs b/src/config.rs index 4054782f..42778aee 100644 --- a/src/config.rs +++ b/src/config.rs @@ -45,6 +45,8 @@ pub struct UserConfig { /// `security.harness`: the Harness a Security run uses ahead of /// `harness.default`, unless its command names one. pub security_harness: Option, + /// `security.fix`: off by default; None leaves the decision unmade. + pub security_fix: Option, } /// The `[email]` section: where email goes and who it comes from. The Resend @@ -87,6 +89,7 @@ impl UserConfig { pickup_limit: NonZeroUsize::new(3).unwrap(), harness: harness::Settings::default(), security_harness: None, + security_fix: None, } } @@ -156,6 +159,8 @@ impl UserConfig { ("pickup", "limit", value) => { config.pickup_limit = whole_number_from_1(value, "pickup.limit", &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)) => { let name = name.trim(); if !name.is_empty() { @@ -669,6 +674,7 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 [security] +fix = false # Security runs may fix reproduced findings; default false harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code [harness] @@ -792,6 +798,8 @@ mod tests { config.email.from = None; assert_eq!(config.harness.default, Some(Harness::Claude)); config.harness.default = None; + assert_eq!(config.security_fix, Some(false)); + config.security_fix = None; assert_eq!(config, UserConfig::defaults(Path::new("/home/me"))); } @@ -824,6 +832,7 @@ mod tests { "activity.quiet_skips", "spec.parallel", "pickup.limit", + "security.fix", "security.harness", "harness.default", "harness.claude.model", diff --git a/src/delivery.rs b/src/delivery.rs index e1c0a05f..4fddf1b0 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -36,6 +36,7 @@ use repair_loop::{Repair, Upstream}; /// 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, pub issue: &'a IssueUrl, pub base: &'a str, pub goal: Goal, diff --git a/src/github/advisories.rs b/src/github/advisories.rs index 93a9a995..0fc790eb 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -22,26 +22,72 @@ pub enum SecurityRecords { impl SecurityRecords { /// An unreproduced, untriaged finding still needs the Day shift's call. - pub fn waiting_for_day_shift(&self) -> bool { + pub fn waiting_for_day_shift(&self, fixing: bool) -> bool { match self { - Self::Advisories(records) => records - .iter() - .any(|record| record["state"] == "draft" && record["severity"].is_null()), + Self::Advisories(records) => records.iter().any(|record| { + record["state"] == "draft" + && (reproduced_severity(&record["description"]).is_some() + && !fixing + && record["security_fix_closed"] != true + || reproduced_severity(&record["description"]).is_none() + && record["severity"].is_null()) + }), Self::Issues(records) => records.iter().any(|record| { record["state"] .as_str() .is_some_and(|state| state.eq_ignore_ascii_case("open")) - && record["labels"].as_array().is_some_and(|labels| { - labels.iter().any(|label| { - label["name"] - .as_str() - .is_some_and(|name| NEEDS_TRIAGE.is_named(name)) - }) - }) + && (reproduced_severity(&record["body"]).is_some() + && !fixing + && record["security_fix_closed"] != true + || reproduced_severity(&record["body"]).is_none() + && record["labels"].as_array().is_some_and(|labels| { + labels.iter().any(|label| { + label["name"] + .as_str() + .is_some_and(|name| NEEDS_TRIAGE.is_named(name)) + }) + })) }), } } + /// Most severe reproduced finding still awaiting a fix, ties in record order. + /// A recorded Ticket has already been dispatched; its Run owns that fix. + pub fn next_fix(&self) -> Result> { + let (records, description, state) = match self { + Self::Advisories(records) => (records, "description", "draft"), + Self::Issues(records) => (records, "body", "open"), + }; + let mut next = None; + let mut highest = 0; + for value in records { + if !value["state"] + .as_str() + .is_some_and(|s| s.eq_ignore_ascii_case(state)) + || has_fix_ticket(&value[description]) + { + continue; + } + if let Some(severity) = reproduced_severity(&value[description]) { + let rank = match severity { + "critical" => 5, + "high" => 4, + "medium" => 3, + "low" => 2, + "informational" => 1, + _ => continue, + }; + if rank > highest { + let mut metadata = crate::security::RecordedFinding::of_record(value)?; + metadata.severity = Some(severity.into()); + next = Some((self.record(value)?, metadata)); + highest = rank; + } + } + } + Ok(next) + } + pub fn remember(&mut self, record: Value) { match self { Self::Advisories(records) | Self::Issues(records) => records.push(record), @@ -97,6 +143,25 @@ impl SecurityRecords { } } +fn reproduced_severity(description: &Value) -> Option<&str> { + let (_, reproduction) = description + .as_str()? + .split_once("\n\n")?; + let outcome = reproduction + .lines() + .find_map(|line| line.strip_prefix("Outcome: "))?; + match outcome.split_whitespace().collect::>().as_slice() { + ["reproduced", severity, "single" | "spec"] => Some(*severity), + _ => None, + } +} + +fn has_fix_ticket(description: &Value) -> bool { + description + .as_str() + .is_some_and(|text| text.contains("\n\nFix Ticket: ")) +} + /// A finding read from the private storage selected for this repository. pub enum SecurityRecord { Advisory { @@ -149,10 +214,67 @@ pub struct DraftAdvisory { } impl GitHub { + /// Link the dispatched Ticket without changing the private record's grade. + pub fn link_security_fix( + &self, + repo: &str, + record: &SecurityRecord, + ticket: &IssueUrl, + ) -> Result<()> { + let (path, field) = match record { + SecurityRecord::Advisory { id, .. } => ( + format!("repos/{repo}/security-advisories/{id}"), + "description", + ), + SecurityRecord::Issue { number, .. } => { + (format!("repos/{repo}/issues/{number}"), "body") + } + }; + let output = self.output_with_input(&["api", &path], None)?; + if !output.status.success() { + bail!( + "reading the private record before linking its fix failed ({})", + output.status + ); + } + let current: Value = + serde_json::from_slice(&output.stdout).context("private record is invalid JSON")?; + if current[field].as_str() != Some(record.description()) { + bail!("the private record changed while publishing its fix; leaving it unchanged"); + } + let description = format!( + "{}\n\nFix Ticket: {}\n", + record.description().trim_end(), + ticket.url + ); + let body = serde_json::to_vec(&json!({field: description}))?; + let output = self.output_with_input( + &["api", "--method", "PATCH", &path, "--input", "-"], + Some(&body), + )?; + if !output.status.success() { + bail!( + "linking the private record's fix failed ({})", + output.status + ); + } + Ok(()) + } + + pub fn security_fix_text(&self, ticket: &IssueUrl) -> Result { + let value = self.issue_view(ticket, "title,body")?; + let title = value["title"] + .as_str() + .context("the Security fix Ticket has no title")?; + let body = value["body"] + .as_str() + .context("the Security fix Ticket has no body")?; + Ok(format!("{title}\n\n{body}")) + } /// Every state and every page. A 404 selects issues only when repository /// metadata confirms they will be private. pub fn security_records(&self, repo: &str) -> Result { - match self.gh_api_items( + let records = match self.gh_api_items( &format!("repos/{repo}/security-advisories?per_page=100"), "", ) { @@ -176,7 +298,25 @@ impl GitHub { )) } Err(error) => Err(error), + }; + let mut records = records?; + let (values, field) = match &mut records { + SecurityRecords::Advisories(values) => (values, "description"), + SecurityRecords::Issues(values) => (values, "body"), + }; + for record in values { + if let Some((_, link)) = record[field].as_str().and_then(|text| { + text.rsplit_once("\n\nFix Ticket: ") + }) { + let ticket = IssueUrl::parse(link.lines().next().unwrap_or_default()) + .context("the private record has an invalid fix Ticket link")?; + if !ticket.repo_slug().eq_ignore_ascii_case(repo) { + bail!("the private record's fix Ticket belongs to another repository"); + } + record["security_fix_closed"] = json!(!self.issue_is_open(&ticket)?); + } } + Ok(records) } /// Use the storage selected while reading the repository's records. diff --git a/src/main.rs b/src/main.rs index 8bdbd82b..aaeef623 100644 --- a/src/main.rs +++ b/src/main.rs @@ -315,6 +315,17 @@ 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-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 creates one terse Ticket that says what the fix changes and links +the private record; thirdshift checks it, swaps needs-triage for ready-for-agent, adds +security-fix and dispatches its Run. The Security run ends as that Run ends, including +merge when requested. Fixing is off by default. [security] fix = true also allows it; +no-security-fix overrides the setting. Both words, with or without dashes, are accepted +on Run, Spec run, Architect run, Pickup run and Security run commands, and passed to their +Runs. Without permission, a reproduced finding waits until its record is closed or +published, or its fix Ticket is closed. + The User config, ~/.thirdshift/config.toml, sets defaults for every Run on this machine; thirdshift setup asks for your defaults and writes one listing every setting, to edit. With merge.always set, every Run is a Merge run unless given --no-merge: @@ -510,7 +521,7 @@ fn pickup(args: PassArgs) -> ExitCode { } } -/// A report-only Security run, with one notification when asked, unless skipped. +/// A Security run, with one notification when asked, unless skipped. fn secure(args: PassArgs) -> ExitCode { let (config, mut started) = match command::start( Begin::SecurityRun, @@ -529,6 +540,7 @@ fn secure(args: PassArgs) -> ExitCode { urls: Vec::new(), }), security::Outcome::Audited(audited) => Ending::Security(audited), + security::Outcome::Fixed { ended, findings } => Ending::SecurityFix { ended, findings }, }) } diff --git a/src/pass.rs b/src/pass.rs index 9aaec33e..21732ee2 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -42,8 +42,15 @@ const REVIEW: &str = "architecture-review"; /// the pass's Base branch `base`, whatever the Launch directory has checked /// out. pub enum Dispatch<'a> { + SecurityFix { + ticket: &'a IssueUrl, + base: &'a str, + }, /// An Architect run's Architect plan. - ArchitectPlan { plan: &'a IssueUrl, base: &'a str }, + ArchitectPlan { + plan: &'a IssueUrl, + base: &'a str, + }, /// The Ready issue a Pickup run took, a Spec or not, as `is_spec` says. ReadyIssue { issue: &'a IssueUrl, @@ -123,6 +130,13 @@ pub trait Outside { record: &SecurityRecord, reproduction: &Reproduction, ) -> Result<()>; + fn publish_security_fix( + &mut self, + base: &str, + record: &SecurityRecord, + url: &str, + ) -> (Result, Option); + fn link_security_fix(&mut self, record: &SecurityRecord, ticket: &IssueUrl) -> Result<()>; /// Run `dispatch` to its end, on the Harness the pass checked. fn dispatch(&mut self, dispatch: Dispatch) -> Ended; } @@ -265,6 +279,11 @@ impl Outside for LaunchAndGitHub<'_> { /// the command's flags and the User config, on the checked Harness. fn dispatch(&mut self, dispatch: Dispatch) -> Ended { let (issue, asks, base) = match dispatch { + Dispatch::SecurityFix { ticket, base } => ( + ticket, + Asks::of_ready_issue(ticket, false, self.flags, self.config), + base, + ), Dispatch::ArchitectPlan { plan, base } => ( plan, Asks::of_architect_plan(plan, self.flags, self.config), @@ -286,6 +305,26 @@ impl Outside for LaunchAndGitHub<'_> { }; run::run_to_end(issue, &mut asks, StartedBy::Dispatch { base }) } + + fn publish_security_fix( + &mut self, + base: &str, + record: &SecurityRecord, + url: &str, + ) -> (Result, Option) { + crate::security::fixing::publish( + self.launch.git(), + self.repo, + base, + record, + url, + self.harness, + ) + } + + fn link_security_fix(&mut self, record: &SecurityRecord, ticket: &IssueUrl) -> Result<()> { + GitHub::new().link_security_fix(&self.repo.slug(), record, ticket) + } } #[cfg(test)] @@ -348,6 +387,12 @@ mod in_memory { Reproduce(String), /// It wrote a completed reproduction to this private record. UpdateSecurityRecord(String), + PublishSecurityFix(String), + LinkSecurityFix(u64), + DispatchSecurityFix { + ticket: u64, + base: String, + }, /// It recorded that it started work: on this issue, for a Pickup /// run, or on its repository, for an Architect run. Started(Option), @@ -355,7 +400,10 @@ mod in_memory { Pull, /// It ran the Architecture review session of this Base branch on /// this prompt, handing on its starting line next. - Review { base: String, prompt: String }, + Review { + base: String, + prompt: String, + }, /// It dispatched this Ready issue, a Spec or not, on this Base /// branch. DispatchReadyIssue { @@ -364,7 +412,10 @@ mod in_memory { base: String, }, /// It dispatched this Architect plan on this Base branch. - DispatchPlan { plan: u64, base: String }, + DispatchPlan { + plan: u64, + base: String, + }, } /// A repository in memory: its issues, filed by label, open or @@ -747,6 +798,10 @@ mod in_memory { fn dispatch(&mut self, dispatch: Dispatch) -> Ended { self.calls.push(match dispatch { + Dispatch::SecurityFix { ticket, base } => Call::DispatchSecurityFix { + ticket: ticket.number, + base: base.into(), + }, Dispatch::ArchitectPlan { plan, base } => Call::DispatchPlan { plan: plan.number, base: base.to_string(), @@ -765,6 +820,24 @@ mod in_memory { .take() .expect("a dispatch, with no ending scripted for it") } + + fn publish_security_fix( + &mut self, + _base: &str, + record: &SecurityRecord, + _url: &str, + ) -> (Result, Option) { + self.calls.push(Call::PublishSecurityFix(record.name())); + ( + Ok(IssueUrl::parse("https://github.com/acme/widgets/issues/8").unwrap()), + self.session_log.clone(), + ) + } + + fn link_security_fix(&mut self, _record: &SecurityRecord, ticket: &IssueUrl) -> Result<()> { + self.calls.push(Call::LinkSecurityFix(ticket.number)); + Ok(()) + } } /// Issue `number` in `acme/widgets` as a listing gives it, labelled diff --git a/src/prompt.rs b/src/prompt.rs index aa306b81..e9cd5838 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -328,3 +328,18 @@ pub fn security_reproduction(commit: &str, finding: &str, test: &std::path::Path test = test.display(), ) } + +pub const SECURITY_FIX_LINE: &str = "Security fix Ticket: "; + +pub fn security_fix(base: &str, url: &str, finding: &str) -> String { + format!( + "Publish the fix for this reproduced Security finding on Base branch `{base}`.\n\ + Read the private record at {url} and the record below.\n\ + Publish exactly one new, terse Ticket in this repository, labelled `needs-triage`. Create that label if missing.\n\ + The Ticket says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record.\n\ + For now every fix is a single Ticket, even if the reproduction suggested a Spec. Publish no Spec or sub-issues.\n\ + This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks the Ticket, marks it ready, labels it security-fix and dispatches its Run.\n\ + End your final message with exactly `{SECURITY_FIX_LINE}`.\n\n\ + Private record:\n{finding}\n\n{HEADLESS}" + ) +} diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 5fdc9371..736b69d8 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -195,6 +195,13 @@ fn prompts() -> Vec { sender: Units(&[FINISH]), text: prompt::conflict_repair(&issue, BASE, BRANCH, PR_URL), }, + Prompt { + id: "prompt-security-fix", + title: "Security fix publishing", + when: "With fixing allowed, publishes one terse Ticket for the most severe reproduced finding. thirdshift checks and marks it ready before dispatching its Run.", + sender: SecurityRun, + text: prompt::security_fix(BASE, "", ""), + }, Prompt { id: "prompt-foreign-conflict-repair", title: "Conflict Repair, on Foreign commits", diff --git a/src/run.rs b/src/run.rs index a2dce098..721c3b46 100644 --- a/src/run.rs +++ b/src/run.rs @@ -118,6 +118,7 @@ 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, ); let outcome = run(issue, asks, started_by, &mut base_fix); Ended { @@ -163,6 +164,7 @@ fn run( directory: &directory, issue, goal: asks.goal, + security_fix: asks.security_fix, base_fix, logs: Logs::of_run(issue), harness: &asks.harness, @@ -355,6 +357,7 @@ struct LaunchAndGitHub<'a> { directory: &'a LaunchDirectory, issue: &'a IssueUrl, goal: Goal, + security_fix: bool, base_fix: &'a mut BaseFix, logs: Logs, harness: &'a Choice, @@ -365,6 +368,7 @@ 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, issue: self.issue, base, goal: self.goal, @@ -690,6 +694,7 @@ mod tests { /// What a Run started by its command is asked, by default. fn asks() -> Asks { Asks { + security_fix: false, goal: Goal::ReadyForReview, notification: NotificationAsk::Skip, tickets_at_once: NonZeroUsize::new(1).unwrap(), diff --git a/src/run_ending.rs b/src/run_ending.rs index 722bc5ab..bce3a0bc 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -33,6 +33,10 @@ const BASE_FIX: &str = "Base fix: "; pub fn read(ending: &Ending) -> Result, &Skip> { match ending { Ending::Run(ended) => Ok(Account::of_run(ended)), + Ending::SecurityFix { ended, findings } => Ok(Account { + security_findings: Some(findings), + ..Account::of_run(ended) + }), Ending::Architect { review, dispatched } => { Ok(Account::of_architect(review, dispatched.as_ref())) } diff --git a/src/security.rs b/src/security.rs index c97c440f..5e1a1ee3 100644 --- a/src/security.rs +++ b/src/security.rs @@ -17,11 +17,22 @@ use crate::logs::{self, Pass, Work}; use crate::pass::{LaunchAndGitHub, Outside}; pub mod audit; +pub(crate) mod fixing; pub mod reproduction; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FixAsk { + Allow, + Forbid, +} + pub enum Outcome { Skipped(Skipped), Audited(Ended), + Fixed { + ended: crate::run::Ended, + findings: Vec, + }, } /// The audit's outcome and the private records it reached, including before a failure. @@ -48,7 +59,7 @@ pub struct RecordedFinding { } impl RecordedFinding { - fn of_record(record: &Value) -> Result { + pub(crate) fn of_record(record: &Value) -> Result { Ok(Self { severity: record["severity"].as_str().map(String::from), title: record["summary"] @@ -133,10 +144,11 @@ pub fn run( }, &repo, base.name(), + flags.security_fix_allowed(config), ) } -fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str) -> Outcome { +fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str, fixing: bool) -> Outcome { match outside.ready_issue() { Ok(Some(ready)) => { let skipped = Skipped::ReadyIssue(ready.listed); @@ -150,7 +162,14 @@ fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str) -> Outcome { Ok(records) => records, Err(error) => return Outcome::Audited(error.into()), }; - if records.waiting_for_day_shift() { + if fixing { + match records.next_fix() { + Ok(Some((record, metadata))) => return fix(outside, repo, base, record, metadata), + Ok(None) => {} + Err(error) => return Outcome::Audited(error.into()), + } + } + if records.waiting_for_day_shift(fixing) { let skipped = Skipped::FindingWaiting; outside.skipped(Pass::Security, &skipped); return Outcome::Skipped(skipped); @@ -164,7 +183,65 @@ fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str) -> Outcome { Ok(false) => {} Err(error) => return Outcome::Audited(error.into()), } - Outcome::Audited(audit_and_record(outside, repo, base)) + let audited = audit_and_record(outside, repo, base); + if fixing && audited.outcome.is_ok() { + match outside + .security_records() + .and_then(|records| records.next_fix()) + { + Ok(Some((record, metadata))) => { + let outcome = fix(outside, repo, base, record, metadata); + if let Outcome::Fixed { ended, .. } = outcome { + return Outcome::Fixed { + ended, + findings: audited.findings, + }; + } + return outcome; + } + Ok(None) => {} + Err(error) => { + return Outcome::Audited(Ended { + outcome: Err(error.into()), + ..audited + }); + } + } + } + Outcome::Audited(audited) +} + +fn fix( + outside: &mut impl Outside, + repo: &Repo, + base: &str, + record: crate::github::SecurityRecord, + metadata: RecordedFinding, +) -> Outcome { + let mut log = None; + let ended = (|| -> Result { + outside.check_harness()?; + outside.started(Work::SecurityRun(repo)); + let (published, session_log) = outside.publish_security_fix(base, &record, &metadata.url); + log = session_log; + let ticket = published?; + outside.link_security_fix(&record, &ticket)?; + Ok(outside.dispatch(crate::pass::Dispatch::SecurityFix { + ticket: &ticket, + base, + })) + })(); + Outcome::Fixed { + ended: ended.unwrap_or_else(|error| crate::run::Ended { + outcome: Err(FailedRun { + log, + ..error.into() + }), + base_fix: None, + advice: Vec::new(), + }), + findings: vec![metadata], + } } fn audit_and_record(outside: &mut impl Outside, repo: &Repo, base: &str) -> Ended { @@ -249,6 +326,55 @@ mod tests { use super::*; use crate::pass::{Call, InMemory, widgets}; + #[test] + fn a_reproduced_fix_precedes_waiting_and_unchanged_but_yields_to_a_ready_issue() { + for ready in [false, true] { + let mut outside = InMemory::default().advisories(vec![ + serde_json::json!({"state":"draft", "severity":null}), + serde_json::json!({ + "state":"draft", "severity":"critical", "ghsa_id":"GHSA-first", + "summary":"Bound input", "html_url":"https://github.com/acme/widgets/security/advisories/GHSA-first", + "description":"Private record\n\nOutcome: reproduced critical single\n" + }), + serde_json::json!({ + "state":"draft", "severity":"critical", "ghsa_id":"GHSA-second", + "summary":"Other bound", "html_url":"https://github.com/acme/widgets/security/advisories/GHSA-second", + "description":"Private record\n\nOutcome: reproduced critical single\n" + }), + ]).unchanged_base().dispatched_ending(crate::pass::ready_for_review()); + if ready { + outside = outside.ready(7, false); + } + let outcome = run_through(&mut outside, &widgets(), "main", true); + if ready { + assert!(matches!(outcome, Outcome::Skipped(Skipped::ReadyIssue(_)))); + assert!(matches!( + &outside.calls[..], + [Call::ReadySearch, Call::Skipped(_)] + )); + } else { + assert!(matches!( + outcome, + Outcome::Fixed { + ended: crate::run::Ended { outcome: Ok(_), .. }, + .. + } + )); + assert!( + outside + .calls + .contains(&Call::PublishSecurityFix("GHSA-first".into())) + ); + assert!(outside.calls.contains(&Call::DispatchSecurityFix { + ticket: 8, + base: "main".into() + })); + assert!(!outside.calls.contains(&Call::AuditHistory)); + assert!(!outside.calls.contains(&Call::NodeCheck)); + } + } + } + #[test] fn a_draft_without_severity_waits_before_prerequisites_or_audit() { let mut outside = InMemory::default() @@ -257,7 +383,7 @@ mod tests { "ghsa_id": "GHSA-test", "summary": "Unchecked input size" })]) .harness_failing(); - let outcome = run_through(&mut outside, &widgets(), "main"); + let outcome = run_through(&mut outside, &widgets(), "main", false); assert!(matches!(outcome, Outcome::Skipped(_))); assert_eq!( outside.calls, @@ -282,7 +408,7 @@ mod tests { let mut outside = InMemory::default().advisories(vec![serde_json::json!({ "state": state, "severity": severity })]); - let outcome = run_through(&mut outside, &widgets(), "main"); + let outcome = run_through(&mut outside, &widgets(), "main", false); assert_eq!( matches!(outcome, Outcome::Skipped(_)), waits, @@ -308,7 +434,7 @@ mod tests { "state": state, "labels": labels.iter().map(|name| serde_json::json!({"name":name})).collect::>() })]); - let outcome = run_through(&mut outside, &widgets(), "main"); + let outcome = run_through(&mut outside, &widgets(), "main", false); assert_eq!( matches!(outcome, Outcome::Skipped(_)), waits, @@ -324,7 +450,7 @@ mod tests { #[test] fn checks_prerequisites_then_audits_without_pulling() { let mut outside = InMemory::default(); - let outcome = run_through(&mut outside, &widgets(), "main"); + let outcome = run_through(&mut outside, &widgets(), "main", false); assert!(matches!( outcome, Outcome::Audited(Ended { outcome: Ok(_), .. }) @@ -351,7 +477,7 @@ mod tests { .node_failing() .harness_failing(); assert!(matches!( - run_through(&mut outside, &widgets(), "main"), + run_through(&mut outside, &widgets(), "main", false), Outcome::Skipped(Skipped::ReadyIssue(_)) )); assert!(matches!( @@ -377,7 +503,7 @@ mod tests { if unchanged { outside = outside.unchanged_base(); } - let outcome = run_through(&mut outside, &widgets(), "main"); + let outcome = run_through(&mut outside, &widgets(), "main", false); let expected = if ready { "Ready issue #7" } else if waiting { @@ -431,7 +557,7 @@ mod tests { ), ] { assert!(matches!( - run_through(&mut outside, &widgets(), "main"), + run_through(&mut outside, &widgets(), "main", false), Outcome::Audited(Ended { outcome: Err(_), .. @@ -449,7 +575,7 @@ mod tests { let Outcome::Audited(Ended { outcome: Err(failed), .. - }) = run_through(&mut outside, &widgets(), "main") + }) = run_through(&mut outside, &widgets(), "main", false) else { panic!("audit should fail"); }; @@ -491,7 +617,7 @@ mod tests { let Outcome::Audited(Ended { outcome: Ok(recorded), .. - }) = run_through(&mut outside, &widgets(), "main") + }) = run_through(&mut outside, &widgets(), "main", false) else { panic!("audit should succeed"); }; diff --git a/src/security/fixing.rs b/src/security/fixing.rs new file mode 100644 index 00000000..1e27a685 --- /dev/null +++ b/src/security/fixing.rs @@ -0,0 +1,104 @@ +//! Publish and check a terse fix Ticket, without exposing the private write-up. + +use std::path::PathBuf; + +use anyhow::{Context, Result, bail}; +use chrono::Utc; + +use crate::git::Git; +use crate::github::{GitHub, SecurityRecord}; +use crate::harness::Choice; +use crate::issue::{IssueUrl, Repo}; +use crate::labels::{Edit, Label, NEEDS_TRIAGE, READY_FOR_AGENT}; +use crate::prompt; +use crate::session::{Logs, Purpose, Sessions}; +use crate::worktree::ReviewWorktree; + +pub const SECURITY_FIX: Label = + Label::new("security-fix", "A fix for a reproduced Security finding"); + +pub fn publish( + launch: &Git, + repo: &Repo, + base: &str, + record: &SecurityRecord, + url: &str, + harness: &Choice, +) -> (Result, Option) { + let worktree = match ReviewWorktree::create(launch, &repo.name, base) { + Ok(worktree) => worktree, + Err(error) => return (Err(error), None), + }; + let logs = Logs::of_security_run(repo); + let started = Utc::now(); + let prompt = prompt::security_fix(base, url, record.description()); + Sessions::within(&logs, worktree.path(), harness, |sessions| { + let message = + sessions.run_to_final_message(Purpose::Security, "security-fix-publishing", &prompt)?; + let line = message + .as_deref() + .unwrap_or_default() + .lines() + .map(str::trim) + .rfind(|line| !line.is_empty()) + .unwrap_or_default(); + let ticket = line.strip_prefix(prompt::SECURITY_FIX_LINE).and_then(|url| IssueUrl::parse(url).ok()).context("the Security fix publishing session ended without the final line its prompt asks for")?; + if !ticket.repo_slug().eq_ignore_ascii_case(&repo.slug()) { + bail!("the Security fix Ticket is not in this repository"); + } + let github = GitHub::new(); + let viewed = github.issue(&ticket)?; + if !viewed.is_open { + bail!("the Security fix Ticket is closed"); + } + if viewed.created.timestamp() < started.timestamp() { + bail!("the Security fix Ticket was created before this session"); + } + if !viewed.labels.has(NEEDS_TRIAGE) + || viewed + .labels + .swapped(&[NEEDS_TRIAGE], &[]) + .unready() + .is_some() + { + bail!( + "the Security fix Ticket must be labelled needs-triage with no other Unready Ticket label" + ); + } + if github.candidate(&ticket, NEEDS_TRIAGE)?.has_sub_issues() { + bail!("the Security fix must be a single Ticket"); + } + let body = github.security_fix_text(&ticket)?; + if !body.contains(url) { + bail!("the Security fix Ticket does not link the private record"); + } + // Reject copied write-up lines and test text. The session is also + // instructed to publish only what the fix changes, never a paraphrase. + for line in record.description().lines().filter(|line| { + let line = line.trim(); + line.len() >= 16 + && !line.starts_with("Fingerprint:") + && !line.starts_with("Audited commit:") + && !line.starts_with("Outcome:") + && !line.starts_with("Severity:") + && !line.starts_with("Fix size:") + && !line.starts_with('#') + && !line.starts_with("\n## Reproduction\n\nOutcome: reproduced {severity} single\nSeverity: {severity}\nFix size: single\n\nPrivate reproduction notes.\n\n### Proof-of-concept test\n\n```\nbounded_fixture();\n```\n") + }) + }).collect::>()); + scenario.write_gh_state(&state); + scenario +} + +fn publish_fix(record: &str) -> String { + format!( + r#" +url=$(gh issue create --title "Bound accepted input" --body "Reject oversized input. Private record: https://github.com/acme/widgets/security/advisories/{record}" --label needs-triage) +printf 'Security fix Ticket: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"# + ) +} + +fn implement_fix() -> &'static str { + r#" +echo bounded > bounded.txt +git add bounded.txt +git commit -q -m 'Bound accepted input' +gh pr create --base main --head issue-8 --title 'Bound accepted input' --body 'Closes #8' +"# +} + +#[test] +fn fixing_selects_the_most_severe_record_then_dispatches_one_ticket_before_auditing() { + let scenario = with_reproduced_findings(&["low", "critical", "critical", "high"]); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-1")); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert!( + result + .stderr + .ends_with("PR https://github.com/acme/widgets/pull/1 is ready for review\n"), + "{}", + result.stderr + ); + let state = scenario.gh_state(); + assert_eq!(state["labels"]["8"], json!(["security-fix", "in-progress"])); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2); + let prompt = calls[0]["prompt"].as_str().unwrap(); + assert!(prompt.contains("GHSA-finding-1"), "{prompt}"); + assert!(!prompt.contains("GHSA-finding-2")); + assert!(!prompt.contains("Security audit: complete")); + let ticket = state["bodies"]["8"].as_str().unwrap(); + assert!(ticket.contains("GHSA-finding-1")); + assert!(!ticket.contains("Private candidate write-up")); + assert!( + state["advisories"][1]["description"] + .as_str() + .unwrap() + .contains("https://github.com/acme/widgets/issues/8") + ); +} + +#[test] +fn a_fix_ticket_title_cannot_copy_the_private_write_up() { + let scenario = with_reproduced_findings(&["high"]); + scenario.agent_does_in_session( + 1, + &publish_fix("GHSA-finding-0") + .replace("Bound accepted input", "Private candidate write-up."), + ); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("includes private write-up text"), + "{}", + result.stderr + ); + assert_eq!(scenario.gh_state()["labels"]["8"], json!(["needs-triage"])); + assert_eq!(scenario.claude_calls().len(), 1); +} + +#[test] +fn a_dispatched_fix_still_waits_without_permission_until_its_ticket_is_closed() { + let scenario = with_reproduced_findings(&["high"]); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-0")); + scenario.agent_does_for(8, implement_fix()); + let fixed = scenario.run(&["secure", "security-fix"]); + assert_eq!(fixed.code, Some(0), "{}", fixed.stderr); + let waiting = scenario.run(&["secure"]); + assert_eq!(waiting.code, Some(0), "{}", waiting.stderr); + assert!( + waiting + .stderr + .contains("a Security finding is waiting for the Day shift"), + "{}", + waiting.stderr + ); + assert_eq!(scenario.claude_calls().len(), 2); + let mut state = scenario.gh_state(); + state["issues"]["8"] = json!("CLOSED"); + scenario.write_gh_state(&state); + // Once fixed, the next run may audit the Base branch again. + scenario.agent_does(&audit_script("[]")); + let after = scenario.run(&["secure"]); + assert_eq!(after.code, Some(0), "{}", after.stderr); + assert!( + !after + .stderr + .contains("a Security finding is waiting for the Day shift") + ); +} + +#[test] +fn reproduced_findings_wait_without_permission_even_after_severity_is_written() { + for config in ["", "[security]\nfix = false\n", "[security]\nfix = true\n"] { + let scenario = with_reproduced_findings(&["high"]); + scenario.user_config_is(config); + let args = if config.contains("true") { + vec!["secure", "no-security-fix"] + } else { + vec!["secure"] + }; + let result = scenario.run(&args); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert!( + result + .stderr + .contains("a Security finding is waiting for the Day shift"), + "{}", + result.stderr + ); + assert!(scenario.claude_calls().is_empty()); + assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); + } +} + +#[test] +fn a_private_finding_dispatches_a_terse_ticket_and_keeps_its_write_up_in_the_record() { + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["private"] = json!(true); + state["bodies"]["7"] = state["advisories"][0]["description"].clone(); + state["labels"]["7"] = json!(["security-finding", "needs-triage"]); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &publish_fix("GHSA-finding-0").replace("security/advisories/GHSA-finding-0", "issues/7"), + ); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + assert!( + state["bodies"]["8"] + .as_str() + .unwrap() + .contains("https://github.com/acme/widgets/issues/7") + ); + assert!( + !state["bodies"]["8"] + .as_str() + .unwrap() + .contains("Private candidate write-up") + ); + assert!( + state["bodies"]["7"] + .as_str() + .unwrap() + .contains("Private candidate write-up") + ); + assert!( + state["bodies"]["7"] + .as_str() + .unwrap() + .contains("Fix Ticket: https://github.com/acme/widgets/issues/8") + ); +} + +#[test] +fn the_setting_allows_a_merge_fix_and_the_command_can_override_either_setting() { + for (config, word, merged) in [ + ( + "[security]\nfix = true\n[merge]\nalways = true\n", + None, + true, + ), + ("[security]\nfix = false\n", Some("security-fix"), false), + ( + "[security]\nfix = true\n[merge]\nalways = true\n", + Some("no-merge"), + false, + ), + ] { + let scenario = with_reproduced_findings(&["medium"]); + scenario.user_config_is(config); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-0")); + scenario.agent_does_for(8, implement_fix()); + let mut args = vec!["secure"]; + args.extend(word); + let result = scenario.run(&args); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!( + scenario.gh_state()["prs"][0]["state"], + if merged { "MERGED" } else { "OPEN" } + ); + assert_eq!(scenario.claude_calls().len(), 2); + } +} + +#[test] +fn an_audit_goes_on_to_one_reproduced_fix_when_fixing_is_allowed() { + let scenario = Scenario::new(); + scenario.repo_has_labels(&["needs-triage", "ready-for-agent"]); + scenario.agent_does_in_session( + 1, + &audit_script(&json!([finding("first"), finding("second")]).to_string()), + ); + scenario.agent_does_in_session(2, &reproduction_script("reproduced high single")); + scenario.agent_does_in_session(3, &reproduction_script("reproduced critical spec")); + scenario.agent_does_in_session(4, &publish_fix("GHSA-test-test-0002")); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert_eq!(scenario.claude_calls().len(), 5); + assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 2); + assert!( + scenario.gh_state()["bodies"]["8"] + .as_str() + .unwrap() + .contains("GHSA-test-test-0002") + ); +} + +#[test] +fn the_security_run_ends_as_a_failed_fix_and_sends_one_private_metadata_notification() { + let scenario = with_reproduced_findings(&["critical"]); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-0")); + scenario.agent_does_for(8, &format!("{}exit 1\n", implement_fix())); + let resend = ResendStandIn::replying(200, r#"{"id":"sent"}"#); + let result = scenario.run_with_env( + &["secure", "security-fix", "email", "day@example.com"], + &resend_env(&resend), + ); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("claude exited 1"), + "{}", + result.stderr + ); + assert_eq!( + scenario.gh_state()["labels"]["8"], + json!(["security-fix", "in-progress"]) + ); + let (subject, body) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Security run: failed"); + assert!(body.contains("critical")); + assert!(body.contains("GHSA-finding-0")); + assert!(!body.contains("Private candidate write-up")); + assert!(!body.contains("bounded_fixture")); +} + +#[test] +fn invalid_fix_tickets_are_not_marked_ready_or_dispatched() { + for script in [ + "printf '%s\\n' 'No protocol line' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n".to_string(), + publish_fix("GHSA-finding-0") + .replace("\"$url\"", "\"https://github.com/other/widgets/issues/8\""), + publish_fix("GHSA-finding-0") + .replace("Reject oversized input.", "Private candidate write-up."), + publish_fix("GHSA-finding-0").replace("GHSA-finding-0", "GHSA-other"), + format!( + "{}gh fake sub-issues 8 '[7]'\n", + publish_fix("GHSA-finding-0") + ), + format!("{}gh issue close 8\n", publish_fix("GHSA-finding-0")), + format!( + "{}gh fake created 8 '2020-01-01T00:00:00Z'\n", + publish_fix("GHSA-finding-0") + ), + ] { + let scenario = with_reproduced_findings(&["high"]); + scenario.agent_does_in_session(1, &script); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(1), "{script}: {}", result.stderr); + assert_eq!(scenario.claude_calls().len(), 1); + assert!( + !scenario.gh_state()["labels"]["8"] + .as_array() + .is_some_and(|labels| labels.contains(&json!("ready-for-agent"))) + ); + assert!(scenario.gh_state()["prs"].as_array().unwrap().is_empty()); + } +} + +#[test] +fn security_fix_words_are_accepted_once_on_every_command_that_starts_runs() { + for prefix in [ + vec!["https://github.com/acme/widgets/issues/7"], + vec!["architect"], + vec!["pickup"], + vec!["secure"], + ] { + for word in [ + "security-fix", + "--security-fix", + "no-security-fix", + "--no-security-fix", + ] { + let scenario = Scenario::new(); + scenario.user_config_is("[security]\nfix = 'invalid'\n"); + let mut args = prefix.clone(); + args.push(word); + let result = scenario.run(&args); + assert_eq!(result.code, Some(1), "{args:?}: {}", result.stderr); + assert!( + result.stderr.contains("security.fix must be true or false"), + "{}", + result.stderr + ); + } + for (words, error) in [ + ( + vec!["security-fix", "no-security-fix"], + "can't be used together", + ), + (vec!["security-fix", "--security-fix"], "repeated argument"), + ] { + let scenario = Scenario::new(); + let mut args = prefix.clone(); + args.extend(words); + let result = scenario.run(&args); + assert_eq!(result.code, Some(2), "{args:?}: {}", result.stderr); + assert!(result.stderr.contains(error)); + } + } +} + #[test] fn reproductions_are_sequential_fresh_and_pinned_to_the_audited_commit() { let scenario = Scenario::new(); From 755371d08ece6d4184aa743d71b575b4b32d1d87 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 11:21:30 -0400 Subject: [PATCH 2/4] Keep config completion assertions aligned with the fixing default --- src/config.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/config.rs b/src/config.rs index 42778aee..cbd51aae 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1228,6 +1228,10 @@ mod tests { assert_eq!(after.harness.default, Some(Harness::Claude)); after.harness.default = None; } + if before.security_fix.is_none() { + assert_eq!(after.security_fix, Some(false)); + after.security_fix = None; + } assert_eq!(after, before, "{completed}"); let again = document(&completed).render(None); assert_eq!(again, completed, "completing twice changed it"); From 78afbc7a403b4326d63c7bb8ed3325459b1bd2e5 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 11:37:57 -0400 Subject: [PATCH 3/4] Address Standards and Spec findings with retained regressions --- src/github.rs | 1 + src/github/advisories.rs | 49 +++++++------- src/security/fixing.rs | 3 +- src/security/reproduction.rs | 36 +++++++---- tests/security_run.rs | 121 +++++++++++++++++++++++++++++++++++ 5 files changed, 175 insertions(+), 35 deletions(-) diff --git a/src/github.rs b/src/github.rs index 741a1233..bcd543b2 100644 --- a/src/github.rs +++ b/src/github.rs @@ -13,6 +13,7 @@ use crate::labels::{Label, Labels}; use crate::process::{self, Control, Interruption}; mod advisories; +pub(crate) use advisories::FIX_TICKET_MARKER; pub use advisories::{DraftAdvisory, Package, SecurityRecord, SecurityRecords}; #[cfg(test)] diff --git a/src/github/advisories.rs b/src/github/advisories.rs index 0fc790eb..44a954af 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -15,6 +15,8 @@ const SECURITY_FINDING: Label = Label::new( "A Security finding recorded privately for the Day shift", ); +pub(crate) const FIX_TICKET_MARKER: &str = "\n\nFix Ticket: "; + pub enum SecurityRecords { Advisories(Vec), Issues(Vec), @@ -59,7 +61,6 @@ impl SecurityRecords { Self::Issues(records) => (records, "body", "open"), }; let mut next = None; - let mut highest = 0; for value in records { if !value["state"] .as_str() @@ -69,23 +70,26 @@ impl SecurityRecords { continue; } if let Some(severity) = reproduced_severity(&value[description]) { - let rank = match severity { - "critical" => 5, - "high" => 4, - "medium" => 3, - "low" => 2, - "informational" => 1, - _ => continue, - }; - if rank > highest { - let mut metadata = crate::security::RecordedFinding::of_record(value)?; - metadata.severity = Some(severity.into()); - next = Some((self.record(value)?, metadata)); - highest = rank; + // The Day shift's current advisory grade takes precedence + // over the historical proof-of-concept's score. + let severity = value["severity"] + .as_str() + .and_then(Severity::parse) + .unwrap_or(severity); + if next + .as_ref() + .is_none_or(|(most_severe, _)| severity < *most_severe) + { + next = Some((severity, value)); } } } - Ok(next) + next.map(|(severity, value)| { + let mut metadata = crate::security::RecordedFinding::of_record(value)?; + metadata.severity = Some(severity.name().into()); + Ok((self.record(value)?, metadata)) + }) + .transpose() } pub fn remember(&mut self, record: Value) { @@ -143,7 +147,7 @@ impl SecurityRecords { } } -fn reproduced_severity(description: &Value) -> Option<&str> { +fn reproduced_severity(description: &Value) -> Option { let (_, reproduction) = description .as_str()? .split_once("\n\n")?; @@ -151,7 +155,7 @@ fn reproduced_severity(description: &Value) -> Option<&str> { .lines() .find_map(|line| line.strip_prefix("Outcome: "))?; match outcome.split_whitespace().collect::>().as_slice() { - ["reproduced", severity, "single" | "spec"] => Some(*severity), + ["reproduced", severity, "single" | "spec"] => Severity::parse(severity), _ => None, } } @@ -159,7 +163,7 @@ fn reproduced_severity(description: &Value) -> Option<&str> { fn has_fix_ticket(description: &Value) -> bool { description .as_str() - .is_some_and(|text| text.contains("\n\nFix Ticket: ")) + .is_some_and(|text| text.contains(FIX_TICKET_MARKER)) } /// A finding read from the private storage selected for this repository. @@ -243,7 +247,7 @@ impl GitHub { bail!("the private record changed while publishing its fix; leaving it unchanged"); } let description = format!( - "{}\n\nFix Ticket: {}\n", + "{}{FIX_TICKET_MARKER}{}\n", record.description().trim_end(), ticket.url ); @@ -305,9 +309,10 @@ impl GitHub { SecurityRecords::Issues(values) => (values, "body"), }; for record in values { - if let Some((_, link)) = record[field].as_str().and_then(|text| { - text.rsplit_once("\n\nFix Ticket: ") - }) { + if let Some((_, link)) = record[field] + .as_str() + .and_then(|text| text.rsplit_once(FIX_TICKET_MARKER)) + { let ticket = IssueUrl::parse(link.lines().next().unwrap_or_default()) .context("the private record has an invalid fix Ticket link")?; if !ticket.repo_slug().eq_ignore_ascii_case(repo) { diff --git a/src/security/fixing.rs b/src/security/fixing.rs index 1e27a685..28eda33e 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -76,7 +76,8 @@ pub fn publish( // instructed to publish only what the fix changes, never a paraphrase. for line in record.description().lines().filter(|line| { let line = line.trim(); - line.len() >= 16 + line.chars().any(char::is_alphanumeric) + && !line.starts_with("```") && !line.starts_with("Fingerprint:") && !line.starts_with("Audited commit:") && !line.starts_with("Outcome:") diff --git a/src/security/reproduction.rs b/src/security/reproduction.rs index ab31ee4a..f420113a 100644 --- a/src/security/reproduction.rs +++ b/src/security/reproduction.rs @@ -9,7 +9,7 @@ use anyhow::{Context, Result, bail}; use serde::Serialize; use crate::git::Git; -use crate::github::SecurityRecord; +use crate::github::{FIX_TICKET_MARKER, SecurityRecord}; use crate::harness::Choice; use crate::issue::Repo; use crate::logs; @@ -17,7 +17,8 @@ use crate::prompt; use crate::session::{Logs, Purpose, Sessions}; use crate::worktree::ReviewWorktree; -#[derive(Clone, Copy, Serialize)] +// Declaration order is severity order, most severe first. +#[derive(Clone, Copy, Serialize, PartialEq, Eq, PartialOrd, Ord)] #[serde(rename_all = "lowercase")] pub enum Severity { Critical, @@ -28,7 +29,18 @@ pub enum Severity { } impl Severity { - pub(super) fn name(self) -> &'static str { + pub(crate) fn parse(name: &str) -> Option { + match name { + "critical" => Some(Self::Critical), + "high" => Some(Self::High), + "medium" => Some(Self::Medium), + "low" => Some(Self::Low), + "informational" => Some(Self::Informational), + _ => None, + } + } + + pub(crate) fn name(self) -> &'static str { match self { Self::Critical => "critical", Self::High => "high", @@ -84,6 +96,12 @@ impl Reproduction { } pub fn description(&self, original: &str) -> String { + // Replacing reproduction evidence must keep the dispatched fix's link. + let (original, fix) = original + .rsplit_once(FIX_TICKET_MARKER) + .map_or((original, String::new()), |(original, ticket)| { + (original, format!("{FIX_TICKET_MARKER}{ticket}")) + }); let original = original .split_once("\n\n") .map_or(original, |(original, _)| original); @@ -107,7 +125,7 @@ impl Reproduction { + 1, ); format!( - "{}\n\n\n## Reproduction\n\nOutcome: {}\n{severity}{size}\n{}\n\n### Proof-of-concept test\n\n{fence}\n{}{fence}\n", + "{}\n\n\n## Reproduction\n\nOutcome: {}\n{severity}{size}\n{}\n\n### Proof-of-concept test\n\n{fence}\n{}{fence}\n{fix}", original.trim_end(), self.outcome, self.notes, @@ -166,14 +184,8 @@ pub fn run( let outcome = match line.split_whitespace().collect::>().as_slice() { ["Security", "reproduction:", "not", "reproduced"] => Outcome::NotReproduced, ["Security", "reproduction:", "reproduced", severity, size] => { - let severity = match *severity { - "critical" => Severity::Critical, - "high" => Severity::High, - "medium" => Severity::Medium, - "low" => Severity::Low, - "informational" => Severity::Informational, - _ => bail!("the Security reproduction ended with an invalid severity"), - }; + let severity = Severity::parse(severity) + .context("the Security reproduction ended with an invalid severity")?; let size = match *size { "single" => FixSize::Single, "spec" => FixSize::Spec, diff --git a/tests/security_run.rs b/tests/security_run.rs index 9fa69b50..bc3a00b3 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -122,6 +122,7 @@ fn with_reproduced_findings(severities: &[&str]) -> Scenario { let scenario = Scenario::new(); scenario.repo_has_labels(&["needs-triage", "ready-for-agent"]); let commit = scenario.origin_git(&["rev-parse", "main"]); + let commit = commit.trim_end(); let mut state = scenario.gh_state(); state["advisories"] = json!(severities.iter().enumerate().map(|(index, severity)| { json!({ @@ -2166,3 +2167,123 @@ fn a_reproduction_failure_notifies_recorded_metadata_without_private_evidence() assert_eq!(scenario.claude_calls().len(), 3); } } + +#[test] +fn spec_review_short_private_poc_must_not_be_dispatched() { + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + let description = state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("bounded_fixture();", "bypass_login();"); + state["advisories"][0]["description"] = json!(description); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &publish_fix("GHSA-finding-0").replace( + "Reject oversized input.", + "Reject oversized input. Proof-of-concept: bypass_login();", + ), + ); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + let state = scenario.gh_state(); + assert!( + state["bodies"]["8"] + .as_str() + .unwrap() + .contains("bypass_login();") + ); + assert_eq!( + result.code, + Some(1), + "a Ticket copying the private proof-of-concept was dispatched: {}", + result.stderr, + ); + assert_eq!(scenario.claude_calls().len(), 1); + assert!(state["prs"].as_array().unwrap().is_empty()); +} + +#[test] +fn spec_review_current_day_shift_severity_must_decide_fix_priority() { + let scenario = with_reproduced_findings(&["critical", "high"]); + let mut state = scenario.gh_state(); + // This models changing severity in GitHub without rewriting the historical + // reproduction outcome, which must remain useful private evidence. + state["advisories"][0]["severity"] = json!("low"); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, r#" +case "$FAKE_CLAUDE_PROMPT" in + *GHSA-finding-1*) record=GHSA-finding-1 ;; + *GHSA-finding-0*) record=GHSA-finding-0 ;; + *) exit 1 ;; +esac +url=$(gh issue create --title "Bound accepted input" --body "Reject oversized input. Private record: https://github.com/acme/widgets/security/advisories/$record" --label needs-triage) +printf 'Security fix Ticket: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let calls = scenario.claude_calls(); + let prompt = calls[0]["prompt"].as_str().unwrap(); + assert!( + prompt.contains("GHSA-finding-1"), + "the current high finding must precede the finding regraded low: {prompt}", + ); +} + +#[test] +fn a_later_audit_keeps_the_link_to_a_successfully_dispatched_private_fix() { + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["private"] = json!(true); + state["bodies"]["7"] = state["advisories"][0]["description"].clone(); + state["labels"]["7"] = json!(["security-finding", "needs-triage"]); + scenario.write_gh_state(&state); + let publish = + publish_fix("GHSA-finding-0").replace("security/advisories/GHSA-finding-0", "issues/7"); + scenario.agent_does_in_session(1, &publish); + scenario.agent_does_for(8, implement_fix()); + let first = scenario.run(&["secure", "security-fix"]); + assert_eq!(first.code, Some(0), "{}", first.stderr); + let state = scenario.gh_state(); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert!( + state["bodies"]["7"] + .as_str() + .unwrap() + .contains("Fix Ticket: https://github.com/acme/widgets/issues/8") + ); + assert_eq!(state["issues"].as_object().unwrap().len(), 2); + + scenario.origin_has_commit( + "main", + "other-work.txt", + "other work", + "Advance Base branch", + ); + scenario.agent_does_in_session(3, &audit_script(&json!([finding("finding-0")]).to_string())); + scenario.agent_does_in_session(4, &reproduction_script("reproduced high single")); + scenario.agent_does_for_in_session(8, 2, &reproduction_script("reproduced high single")); + scenario.agent_does_in_session(5, &publish); + scenario.agent_does_for( + 9, + &implement_fix() + .replace("issue-8", "issue-9") + .replace("#8", "#9"), + ); + let second = scenario.run(&["secure", "security-fix"]); + assert_eq!(second.code, Some(0), "{}", second.stderr); + let state = scenario.gh_state(); + assert_eq!( + state["issues"].as_object().unwrap().len(), + 2, + "another fix Ticket was published for a finding whose first fix succeeded: {state}" + ); + assert!( + state["bodies"]["7"] + .as_str() + .unwrap() + .contains("Fix Ticket: https://github.com/acme/widgets/issues/8") + ); +} From 21f81e3ed84f57d567e2ffac31bbe97b2b6c6f87 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 11:48:03 -0400 Subject: [PATCH 4/4] Allow ordinary fix prose and complete the Setup config fixture --- src/security/fixing.rs | 3 +++ src/setup.rs | 1 + tests/security_run.rs | 30 ++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/src/security/fixing.rs b/src/security/fixing.rs index 28eda33e..8318e073 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -76,7 +76,10 @@ pub fn publish( // instructed to publish only what the fix changes, never a paraphrase. for line in record.description().lines().filter(|line| { let line = line.trim(); + // A bare identifier can also be an ordinary word in fix prose. + // Complete short statements such as bypass_login(); stay checked. line.chars().any(char::is_alphanumeric) + && !line.chars().all(|ch| ch.is_alphanumeric() || ch == '_') && !line.starts_with("```") && !line.starts_with("Fingerprint:") && !line.starts_with("Audited commit:") diff --git a/src/setup.rs b/src/setup.rs index 29724b02..dbb93102 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -1127,6 +1127,7 @@ limit = 5 [security] harness = \"codex\" +fix = false [harness] default = \"claude\" diff --git a/tests/security_run.rs b/tests/security_run.rs index bc3a00b3..f798d67f 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -2287,3 +2287,33 @@ fn a_later_audit_keeps_the_link_to_a_successfully_dispatched_private_fix() { .contains("Fix Ticket: https://github.com/acme/widgets/issues/8") ); } + +#[test] +fn spec_followup_common_poc_token_must_not_block_a_terse_fix_ticket() { + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + let description = state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("bounded_fixture();", "assert (\n input\n) == 'overflow'"); + state["advisories"][0]["description"] = json!(description); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-0")); + scenario.agent_does_for(8, implement_fix()); + let result = scenario.run(&["secure", "security-fix"]); + let state = scenario.gh_state(); + let ticket = state["bodies"]["8"].as_str().unwrap(); + assert_eq!( + ticket, + "Reject oversized input. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", + ); + assert!(!ticket.contains("assert (") && !ticket.contains("overflow")); + assert_eq!( + result.code, + Some(0), + "a valid terse Ticket was rejected because it shares an ordinary token with the private test: {}", + result.stderr, + ); + assert_eq!(scenario.claude_calls().len(), 2); + assert_eq!(state["prs"].as_array().unwrap().len(), 1); +}