From 8fb48cbea88e25ba6402888e9740862287892670 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 14:07:34 -0400 Subject: [PATCH 1/3] Implement Spec-sized Security fixes and reuse private finding issues (#545) --- README.md | 4 +- skills/thirdshift-to-spec/SKILL.md | 4 +- skills/thirdshift-to-tickets/SKILL.md | 4 +- src/github/advisories.rs | 25 ++- src/pass.rs | 22 ++- src/prompt.rs | 12 +- src/prompts_page.rs | 2 +- src/security.rs | 2 + src/security/fixing.rs | 136 +++++++++++--- src/security/reproduction.rs | 1 + tests/security_run.rs | 248 ++++++++++++++++++++++---- 11 files changed, 375 insertions(+), 85 deletions(-) diff --git a/README.md b/README.md index 10fbec28..5bdc66e0 100644 --- a/README.md +++ b/README.md @@ -748,7 +748,9 @@ The command takes the Harness, Model and Effort words and the shared Pass words 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. +A publishing session reads the private record and follows the reproduction's fix-size decision: a single **Ticket**, or a **Spec** with **Tickets** through `thirdshift-to-spec` and `thirdshift-to-tickets`. Every new issue is terse, saying only what the fix changes and linking the private record. thirdshift checks all the issues, swaps the top issue's `needs-triage` for `ready-for-agent`, adds `security-fix` to every fix issue (creating the label when missing), records the fix link privately and dispatches its **Run** or **Spec run**. The Security run ends as that run ends, including a **Merge run** when asked by the command or the User config. + +On a private repository, the finding's own issue is the fix's Ticket or Spec, keeping its write-up and proof-of-concept. A one-session fix needs no publishing session or second issue: thirdshift marks the finding's issue ready and dispatches it. For a bigger fix, the publishing session adds terse Tickets as that issue's native sub-issues, with their blocking links. `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. diff --git a/skills/thirdshift-to-spec/SKILL.md b/skills/thirdshift-to-spec/SKILL.md index 56dc42ed..5990f1a8 100644 --- a/skills/thirdshift-to-spec/SKILL.md +++ b/skills/thirdshift-to-spec/SKILL.md @@ -1,6 +1,6 @@ --- name: thirdshift-to-spec -description: "Turn the current session into a spec and publish it to the project issue tracker: no interview, just synthesis of what you've already settled. Only for an Architecture review." +description: "Turn the current session into a spec and publish it to the project issue tracker: no interview, just synthesis of what you've already settled. For an Architecture review or Security fix publishing." disable-model-invocation: false --- @@ -10,6 +10,8 @@ The issue tracker and triage label vocabulary should have been provided to you. The spec is all this skill writes. Write nothing to the repository: no spec file, commits or branches. A `CONTEXT.md` or ADR change the spec needs is work for one of its tickets. +For **Security fix publishing**, the Session prompt overrides the template: keep the Spec terse, saying only what the fix changes and linking the private record. Keep all finding evidence in that record. On a private repository, use the finding's existing issue as the Spec and preserve its body and evidence; publish its Tickets with `thirdshift-to-tickets`. + ## Process 1. Explore the repo to understand the current state of the codebase, if you haven't already. Use the project's domain glossary vocabulary throughout the spec, and respect any ADRs in the area you're touching. diff --git a/skills/thirdshift-to-tickets/SKILL.md b/skills/thirdshift-to-tickets/SKILL.md index 2ed473f3..d9568644 100644 --- a/skills/thirdshift-to-tickets/SKILL.md +++ b/skills/thirdshift-to-tickets/SKILL.md @@ -1,6 +1,6 @@ --- name: thirdshift-to-tickets -description: Break a plan, spec, or the current session into a set of tracer-bullet tickets, each declaring its blocking edges, published to the issue tracker with native sub-issue and blocking links. Only for an Architecture review. +description: Break a plan, spec, or the current session into a set of tracer-bullet tickets, each declaring its blocking edges, published to the issue tracker with native sub-issue and blocking links. For an Architecture review or Security fix publishing. disable-model-invocation: false --- @@ -12,6 +12,8 @@ The issue tracker and triage label vocabulary should have been provided to you. The tickets are all this skill writes. Write nothing to the repository: no ticket files, commits or branches. A `CONTEXT.md` or ADR change the work needs is part of a ticket's work, named in its acceptance criteria. +For **Security fix publishing**, keep every Ticket terse: say only what the fix changes and link the private record. Keep all finding evidence in that record. A private finding's issue is the parent Spec, and its existing body and evidence stay unchanged. + ## Process ### 1. Gather context diff --git a/src/github/advisories.rs b/src/github/advisories.rs index 44a954af..56f45cea 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -8,7 +8,7 @@ use serde_json::{Value, json}; use super::GitHub; use crate::issue::IssueUrl; use crate::labels::{Label, NEEDS_TRIAGE}; -use crate::security::reproduction::{Reproduction, Severity}; +use crate::security::reproduction::{FixSize, Reproduction, Severity}; const SECURITY_FINDING: Label = Label::new( "security-finding", @@ -69,7 +69,9 @@ impl SecurityRecords { { continue; } - if let Some(severity) = reproduced_severity(&value[description]) { + if let Some((severity, _)) = + reproduced_outcome(value[description].as_str().unwrap_or_default()) + { // The Day shift's current advisory grade takes precedence // over the historical proof-of-concept's score. let severity = value["severity"] @@ -148,14 +150,18 @@ impl SecurityRecords { } fn reproduced_severity(description: &Value) -> Option { - let (_, reproduction) = description - .as_str()? - .split_once("\n\n")?; + reproduced_outcome(description.as_str()?).map(|(severity, _)| severity) +} + +fn reproduced_outcome(description: &str) -> Option<(Severity, FixSize)> { + let (_, reproduction) = + description.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"] => Severity::parse(severity), + ["reproduced", severity, "single"] => Some((Severity::parse(severity)?, FixSize::Single)), + ["reproduced", severity, "spec"] => Some((Severity::parse(severity)?, FixSize::Spec)), _ => None, } } @@ -181,6 +187,13 @@ pub enum SecurityRecord { } impl SecurityRecord { + /// The size judged by the completed reproduction, never the candidate write-up. + pub fn fix_size(&self) -> Result { + reproduced_outcome(self.description()) + .map(|(_, size)| size) + .context("the Security finding has no reproduced fix size") + } + /// A Day-shift decision is the finding's grade; a repeated fingerprint /// must not publish new proof-of-concept evidence or replace that grade. pub fn untriaged(&self) -> bool { diff --git a/src/pass.rs b/src/pass.rs index 21732ee2..d3ca7fe5 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -44,13 +44,11 @@ const REVIEW: &str = "architecture-review"; pub enum Dispatch<'a> { SecurityFix { ticket: &'a IssueUrl, + is_spec: bool, 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, @@ -279,9 +277,13 @@ 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 } => ( + Dispatch::SecurityFix { ticket, - Asks::of_ready_issue(ticket, false, self.flags, self.config), + is_spec, + base, + } => ( + ticket, + Asks::of_ready_issue(ticket, is_spec, self.flags, self.config), base, ), Dispatch::ArchitectPlan { plan, base } => ( @@ -391,6 +393,7 @@ mod in_memory { LinkSecurityFix(u64), DispatchSecurityFix { ticket: u64, + is_spec: bool, base: String, }, /// It recorded that it started work: on this issue, for a Pickup @@ -798,8 +801,13 @@ mod in_memory { fn dispatch(&mut self, dispatch: Dispatch) -> Ended { self.calls.push(match dispatch { - Dispatch::SecurityFix { ticket, base } => Call::DispatchSecurityFix { + Dispatch::SecurityFix { + ticket, + is_spec, + base, + } => Call::DispatchSecurityFix { ticket: ticket.number, + is_spec, base: base.into(), }, Dispatch::ArchitectPlan { plan, base } => Call::DispatchPlan { diff --git a/src/prompt.rs b/src/prompt.rs index e9cd5838..ee02c71b 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -330,16 +330,18 @@ pub fn security_reproduction(commit: &str, finding: &str, test: &std::path::Path } pub const SECURITY_FIX_LINE: &str = "Security fix Ticket: "; +pub const SECURITY_FIX_SPEC_LINE: &str = "Security fix Spec: "; 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\ + Follow the completed reproduction's fix size: single publishes one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`.\n\ + On a public repository, publish one new top issue labelled `needs-triage`. Create that label if missing. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues.\n\ + Every new issue is terse: it 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. This overrides the skills' templates.\n\ + A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. Read the links back before finishing.\n\ + This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks every issue, marks the top issue ready, labels all fix issues security-fix and dispatches its Run or Spec run.\n\ + End your final message with exactly `{SECURITY_FIX_LINE}` for single or `{SECURITY_FIX_SPEC_LINE}` for spec, naming the top issue.\n\n\ Private record:\n{finding}\n\n{HEADLESS}" ) } diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 736b69d8..d804f6cc 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -198,7 +198,7 @@ fn prompts() -> Vec { 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.", + when: "With fixing allowed, publishes a terse Ticket or Spec with Tickets for the most severe reproduced finding, reusing a private finding's issue. thirdshift checks and marks it ready before dispatching its Run or Spec run.", sender: SecurityRun, text: prompt::security_fix(BASE, "", ""), }, diff --git a/src/security.rs b/src/security.rs index 5e1a1ee3..e9dff4a6 100644 --- a/src/security.rs +++ b/src/security.rs @@ -228,6 +228,7 @@ fn fix( outside.link_security_fix(&record, &ticket)?; Ok(outside.dispatch(crate::pass::Dispatch::SecurityFix { ticket: &ticket, + is_spec: record.fix_size()? == reproduction::FixSize::Spec, base, })) })(); @@ -367,6 +368,7 @@ mod tests { ); assert!(outside.calls.contains(&Call::DispatchSecurityFix { ticket: 8, + is_spec: false, base: "main".into() })); assert!(!outside.calls.contains(&Call::AuditHistory)); diff --git a/src/security/fixing.rs b/src/security/fixing.rs index 8318e073..d386aa7c 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -1,4 +1,4 @@ -//! Publish and check a terse fix Ticket, without exposing the private write-up. +//! Publish and check a terse fix Ticket or Spec, without exposing the private write-up. use std::path::PathBuf; @@ -11,6 +11,7 @@ use crate::harness::Choice; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label, NEEDS_TRIAGE, READY_FOR_AGENT}; use crate::prompt; +use crate::security::reproduction::FixSize; use crate::session::{Logs, Purpose, Sessions}; use crate::worktree::ReviewWorktree; @@ -25,6 +26,44 @@ pub fn publish( url: &str, harness: &Choice, ) -> (Result, Option) { + let size = match record.fix_size() { + Ok(size) => size, + Err(error) => return (Err(error), None), + }; + if let SecurityRecord::Issue { number, .. } = record + && size == FixSize::Single + { + let ready = (|| -> Result { + let issue = IssueUrl::parse(url)?; + if issue.number != *number || !issue.repo_slug().eq_ignore_ascii_case(&repo.slug()) { + bail!("the private Security finding's issue does not match its record"); + } + let github = GitHub::new(); + let viewed = github.issue(&issue)?; + if !viewed.is_open + || viewed + .labels + .swapped(&[NEEDS_TRIAGE], &[]) + .unready() + .is_some() + || !github.tickets(&issue)?.is_empty() + { + bail!("the private Security finding must be an open, ready single Ticket"); + } + if crate::interrupt::requested() { + bail!("interrupted"); + } + Edit::of( + &issue, + viewed.labels, + &[NEEDS_TRIAGE], + &[READY_FOR_AGENT, SECURITY_FIX], + ) + .apply(&github)?; + Ok(issue) + })(); + return (ready, None); + } let worktree = match ReviewWorktree::create(launch, &repo.name, base) { Ok(worktree) => worktree, Err(error) => return (Err(error), None), @@ -42,19 +81,29 @@ pub fn publish( .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")?; + let prefix = match size { + FixSize::Single => prompt::SECURITY_FIX_LINE, + FixSize::Spec => prompt::SECURITY_FIX_SPEC_LINE, + }; + let ticket = line.strip_prefix(prefix).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 private = matches!(record, SecurityRecord::Issue { .. }); + if let SecurityRecord::Issue { number, .. } = record + && ticket.number != *number + { + bail!("a private Security fix must reuse the finding's own issue"); + } 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() { + if !private && viewed.created.timestamp() < started.timestamp() { bail!("the Security fix Ticket was created before this session"); } - if !viewed.labels.has(NEEDS_TRIAGE) + if !private && !viewed.labels.has(NEEDS_TRIAGE) || viewed .labels .swapped(&[NEEDS_TRIAGE], &[]) @@ -65,33 +114,30 @@ pub fn publish( "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 tickets = github.tickets(&ticket)?; + if (size == FixSize::Spec) == tickets.is_empty() { + bail!("the Security fix's sub-issues do not match the reproduced fix size"); } - let body = github.security_fix_text(&ticket)?; - if !body.contains(url) { - bail!("the Security fix Ticket does not link the private record"); + if !private { + check_issue_text(&github, &ticket, record, url)?; } - // 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(); - // 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:") - && !line.starts_with("Outcome:") - && !line.starts_with("Severity:") - && !line.starts_with("Fix size:") - && !line.starts_with('#') - && !line.starts_with("