From 9f5c908175628fda57b56f1e84bcf230c4297739 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sat, 10 Oct 2026 12:45:43 -0400 Subject: [PATCH 1/5] Stage spec proposal text and validate before creating public issues Spec-sized public Security fixes now stage their proposed Spec text (top issue plus Tickets with blocking edges) for validation, and thirdshift creates the issues and their native sub-issue and blocked-by links from the staged text. A rejection creates nothing, and a publishing session that creates a public issue on the side fails loudly with the leak closed, as the single-fix staged path does. Private bigger fixes keep the session-created path. Closes #647 --- prompts/security-fix.md | 21 +++- site/prompts/index.html | 21 +++- src/github.rs | 47 +++++++++ src/prompt.rs | 11 ++- src/security/fixing.rs | 209 +++++++++++++++++++++++++++++++++++++--- tests/fakes/gh.rs | 94 ++++++++++++++++++ tests/security_run.rs | 132 +++++++++++++++++++++---- 7 files changed, 487 insertions(+), 48 deletions(-) diff --git a/prompts/security-fix.md b/prompts/security-fix.md index 82d9c42..1fcc6cf 100644 --- a/prompts/security-fix.md +++ b/prompts/security-fix.md @@ -7,17 +7,30 @@ With fixing allowed, publishes a terse Ticket or Spec with Tickets for the most ``` Publish the fix for this reproduced Security finding on Base branch ``. Read the private record at and the record below. -Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`. -On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. 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. A spec still publishes its issues itself, as before. +Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`. +On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. 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. Only a private bigger fix publishes its issues itself. 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. -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. +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. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges back before finishing. 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. End your final message for a single public fix with exactly: Proposed public issue: Title: Body: <body> -For a spec, end with exactly `Security fix Spec: <Issue URL>`, naming the top issue. +For a public spec, end with exactly: +Proposed public spec: +Title: <spec title> +Body: +<spec body> +Ticket: <ticket 1 title> +Body: +<ticket 1 body> +Blocked by: none +Ticket: <ticket 2 title> +Body: +<ticket 2 body> +Blocked by: 1 +with each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `Security fix Spec: <Issue URL>`, naming the top issue. Private record: <recorded Security finding> diff --git a/site/prompts/index.html b/site/prompts/index.html index d9b84fb..c317b8d 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -240,17 +240,30 @@ <h3 id="prompt-security-fix-title">Security fix publishing</h3> <p class="sent-by">Sent by a Security run, before any unit</p> <pre class="job-text"><code>Publish the fix for this reproduced Security finding on Base branch `<mark class="ph"><base></mark>`. Read the private record at <private record URL> and the record below. -Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`. -On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. 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. A spec still publishes its issues itself, as before. +Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`. +On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. 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. Only a private bigger fix publishes its issues itself. 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. -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. +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. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges back before finishing. 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. End your final message for a single public fix with exactly: Proposed public issue: Title: <title> Body: <body> -For a spec, end with exactly `Security fix Spec: <mark class="ph"><Issue URL></mark>`, naming the top issue. +For a public spec, end with exactly: +Proposed public spec: +Title: <spec title> +Body: +<spec body> +Ticket: <ticket 1 title> +Body: +<ticket 1 body> +Blocked by: none +Ticket: <ticket 2 title> +Body: +<ticket 2 body> +Blocked by: 1 +with each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `Security fix Spec: <mark class="ph"><Issue URL></mark>`, naming the top issue. Private record: <recorded Security finding> diff --git a/src/github.rs b/src/github.rs index d7a918b..fa26cc2 100644 --- a/src/github.rs +++ b/src/github.rs @@ -177,6 +177,53 @@ impl GitHub { ]) } + /// The numeric database id of `issue`, for API calls that take one + /// instead of the issue number (sub-issues, dependencies). + fn issue_db_id(&self, issue: &IssueUrl) -> Result<u64> { + let json = self.gh_json(&[ + "api", + &format!("repos/{}/issues/{}", issue.repo_slug(), issue.number), + "--jq", + ".id", + ])?; + json.as_u64() + .context("gh api did not return the issue's database id") + } + + /// Link `child` as a native sub-issue of `parent`. + pub fn add_sub_issue(&self, parent: &IssueUrl, child: &IssueUrl) -> Result<()> { + let child_id = self.issue_db_id(child)?; + self.gh(&[ + "api", + "--method", + "POST", + &format!( + "repos/{}/issues/{}/sub_issues", + parent.repo_slug(), + parent.number + ), + "-F", + &format!("sub_issue_id={child_id}"), + ]) + } + + /// Add a native "blocked by" edge: `ticket` is blocked by `blocker`. + pub fn add_blocked_by(&self, ticket: &IssueUrl, blocker: &IssueUrl) -> Result<()> { + let blocker_id = self.issue_db_id(blocker)?; + self.gh(&[ + "api", + "--method", + "POST", + &format!( + "repos/{}/issues/{}/dependencies/blocked_by", + ticket.repo_slug(), + ticket.number + ), + "-F", + &format!("issue_id={blocker_id}"), + ]) + } + /// Every open issue labelled `label` in the repository `repo`, an /// `owner/repo`, newest first. They come from GitHub's issue list, not its /// search, whose index can be a while behind an issue just opened. diff --git a/src/prompt.rs b/src/prompt.rs index b0d8001..a76397e 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -360,19 +360,22 @@ 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 const STAGED_PROPOSAL_MARKER: &str = "Proposed public issue:"; +pub const STAGED_SPEC_MARKER: &str = "Proposed public spec:"; pub const STAGED_TITLE_MARKER: &str = "Title:"; pub const STAGED_BODY_MARKER: &str = "Body:"; +pub const STAGED_TICKET_MARKER: &str = "Ticket:"; +pub const STAGED_BLOCKED_BY_MARKER: &str = "Blocked by:"; 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\ - Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`.\n\ - On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. 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. A spec still publishes its issues itself, as before.\n\ + Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`.\n\ + On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. 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. Only a private bigger fix publishes its issues itself.\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\ + 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. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges 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 for a single public fix with exactly:\n{STAGED_PROPOSAL_MARKER}\n{STAGED_TITLE_MARKER} <title>\n{STAGED_BODY_MARKER}\n<body>\nFor a spec, end with exactly `{SECURITY_FIX_SPEC_LINE}<Issue URL>`, naming the top issue.\n\n\ + End your final message for a single public fix with exactly:\n{STAGED_PROPOSAL_MARKER}\n{STAGED_TITLE_MARKER} <title>\n{STAGED_BODY_MARKER}\n<body>\nFor a public spec, end with exactly:\n{STAGED_SPEC_MARKER}\n{STAGED_TITLE_MARKER} <spec title>\n{STAGED_BODY_MARKER}\n<spec body>\n{STAGED_TICKET_MARKER} <ticket 1 title>\n{STAGED_BODY_MARKER}\n<ticket 1 body>\n{STAGED_BLOCKED_BY_MARKER} none\n{STAGED_TICKET_MARKER} <ticket 2 title>\n{STAGED_BODY_MARKER}\n<ticket 2 body>\n{STAGED_BLOCKED_BY_MARKER} 1\nwith each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `{SECURITY_FIX_SPEC_LINE}<Issue URL>`, naming the top issue.\n\n\ Private record:\n{finding}\n\n{HEADLESS}" ) } diff --git a/src/security/fixing.rs b/src/security/fixing.rs index f8e98fe..ab60079 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -75,11 +75,12 @@ pub fn publish( let message = sessions.run_to_final_message(Purpose::Security, "security-fix-publishing", &prompt)?; let github = GitHub::new(); - // A single public fix stages its proposed text for validation; - // thirdshift creates the issue from it, and the session has no - // independent public-issue path. Anything else keeps the - // session-created issue path until staged specs land. - let issue = if size == FixSize::Single && record.private_issue_number().is_none() { + // A public fix stages its proposed text for validation; + // thirdshift creates the issues from it, and the session has no + // independent public-issue path. A private bigger fix keeps the + // session-created issue path: its issues stay private with the + // repository. + let issue = if record.private_issue_number().is_none() { // The session has no independent public-issue path: anything // created during its window is a side-effect leak outside the // validated staged path. Close it to stop further interaction, @@ -98,16 +99,20 @@ pub fn publish( "the Security fix publishing session created public issue(s) outside the staged path: {numbers}" ); } - let Some((title, body)) = message.as_deref().and_then(parse_staged_proposal) else { - bail!( - "the Security fix publishing session did not stage its proposed public issue text" - ); - }; - // Validate the staged text before any public issue exists; - // a rejection blocks creation, so no public issue leaks - // private write-up text. - record.check_public_fix_text(&format!("{title}\n\n{body}"), url)?; - github.create_issue_in(repo, &title, &body, &[NEEDS_TRIAGE])? + if size == FixSize::Single { + let Some((title, body)) = message.as_deref().and_then(parse_staged_proposal) else { + bail!( + "the Security fix publishing session did not stage its proposed public issue text" + ); + }; + // Validate the staged text before any public issue exists; + // a rejection blocks creation, so no public issue leaks + // private write-up text. + record.check_public_fix_text(&format!("{title}\n\n{body}"), url)?; + github.create_issue_in(repo, &title, &body, &[NEEDS_TRIAGE])? + } else { + publish_staged_spec(&github, repo, record, url, message.as_deref())? + } } else { let line = message .as_deref() @@ -189,6 +194,147 @@ pub fn publish( }) } +/// One staged Spec Ticket the publishing session proposed: its title, its +/// body, and the 0-based indices of the Tickets it is blocked by. +struct StagedTicket { + title: String, + body: String, + blocked_by: Vec<usize>, +} + +/// The public Spec the publishing session staged for validation: its +/// proposed top-issue `(title, body)` and its Tickets, instead of +/// session-created public issues. +struct StagedSpec { + title: String, + body: String, + tickets: Vec<StagedTicket>, +} + +/// The staged Spec the publishing session proposed, with every Ticket's +/// blocking edges as 0-based indices. `None` when the final message holds +/// no well-formed staged spec: no marker, no top title or body, no Ticket, +/// an empty title or body, or a blocking edge that is not `none` or a list +/// of 1-based Ticket positions naming another Ticket. +fn parse_staged_spec_proposal(message: &str) -> Option<StagedSpec> { + let (_, staged) = message.rsplit_once(prompt::STAGED_SPEC_MARKER)?; + let staged = staged.trim_start_matches(['\n', '\r']); + let (title_line, rest) = staged.split_once('\n')?; + let title = title_line.strip_prefix(prompt::STAGED_TITLE_MARKER)?.trim(); + let rest = rest.strip_prefix(prompt::STAGED_BODY_MARKER)?; + let (spec_body, tickets_text) = match rest.split_once(prompt::STAGED_TICKET_MARKER) { + Some((body, tickets)) => (body, Some(tickets)), + None => (rest, None), + }; + let tickets_text = tickets_text?; + if title.is_empty() || spec_body.trim().is_empty() { + return None; + } + let mut tickets = Vec::new(); + for section in tickets_text.split(prompt::STAGED_TICKET_MARKER) { + let (title_line, rest) = section.split_once('\n')?; + let title = title_line.trim(); + if title.is_empty() { + return None; + } + let rest = rest.strip_prefix(prompt::STAGED_BODY_MARKER)?; + let (body, blocked) = rest.split_once(prompt::STAGED_BLOCKED_BY_MARKER)?; + if body.trim().is_empty() { + return None; + } + tickets.push((title.to_string(), body.trim().to_string(), blocked)); + } + if tickets.is_empty() { + return None; + } + let count = tickets.len(); + let mut parsed = Vec::with_capacity(count); + for (index, (title, body, blocked)) in tickets.into_iter().enumerate() { + parsed.push(StagedTicket { + title, + body, + blocked_by: parse_blocked_by(blocked, index, count)?, + }); + } + Some(StagedSpec { + title: title.to_string(), + body: spec_body.trim().to_string(), + tickets: parsed, + }) +} + +/// The `Blocked by:` trailer of one staged Ticket as 0-based indices into +/// its Spec's Tickets: `none` is empty, else comma-separated 1-based +/// positions. Reading stops at the next Ticket section or the message end, +/// so only the trailer's first line counts. `None` unless every position +/// names another Ticket of the `count` staged. +fn parse_blocked_by(blocked: &str, index: usize, count: usize) -> Option<Vec<usize>> { + let line = blocked.lines().next().unwrap_or_default().trim(); + let line = line.trim_end_matches(['.', ';']); + if line.eq_ignore_ascii_case("none") { + return Some(Vec::new()); + } + let mut edges = Vec::new(); + for position in line + .split(',') + .map(str::trim) + .filter(|edge| !edge.is_empty()) + { + let position: usize = position.parse().ok()?; + if position == 0 || position > count || position - 1 == index { + return None; + } + let edge = position - 1; + if !edges.contains(&edge) { + edges.push(edge); + } + } + if edges.is_empty() { + return None; + } + Some(edges) +} + +/// Validate the staged Spec proposal and create its public issues: the +/// `needs-triage` top issue and one `ready-for-agent` Ticket per staged +/// Ticket, linked as native sub-issues with their staged blocking edges. +/// Every staged text is validated before any public issue exists, so a +/// rejection creates nothing and leaks no private write-up text. +fn publish_staged_spec( + github: &GitHub, + repo: &Repo, + record: &SecurityRecord, + url: &str, + message: Option<&str>, +) -> Result<IssueUrl> { + let Some(staged) = message.and_then(parse_staged_spec_proposal) else { + bail!("the Security fix publishing session did not stage its proposed public spec text"); + }; + record.check_public_fix_text(&format!("{}\n\n{}", staged.title, staged.body), url)?; + for ticket in &staged.tickets { + record.check_public_fix_text(&format!("{}\n\n{}", ticket.title, ticket.body), url)?; + } + let spec = github.create_issue_in(repo, &staged.title, &staged.body, &[NEEDS_TRIAGE])?; + let mut tickets = Vec::with_capacity(staged.tickets.len()); + for ticket in &staged.tickets { + tickets.push(github.create_issue_in( + repo, + &ticket.title, + &ticket.body, + &[READY_FOR_AGENT], + )?); + } + for ticket in &tickets { + github.add_sub_issue(&spec, ticket)?; + } + for (ticket, staged) in tickets.iter().zip(&staged.tickets) { + for edge in &staged.blocked_by { + github.add_blocked_by(ticket, &tickets[*edge])?; + } + } + Ok(spec) +} + /// The single public fix the publishing session staged for validation: /// its proposed `(title, body)`, instead of a session-created public issue. fn parse_staged_proposal(message: &str) -> Option<(String, String)> { @@ -232,4 +378,37 @@ mod tests { let message = "Security fix Ticket: https://github.com/acme/widgets/issues/8\n"; assert!(parse_staged_proposal(message).is_none()); } + + #[test] + fn staged_spec_parses_top_issue_and_tickets_with_blocking() { + let message = "Reasoning\nProposed public spec:\nTitle: Bound accepted input\nBody:\nBound input across storage and transport. Private record: https://example.invalid/advisory\nTicket: Bound storage input\nBody:\nBound storage input. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Bound transport input\nBody:\nBound transport input. Private record: https://example.invalid/advisory\nBlocked by: 1\n"; + let spec = parse_staged_spec_proposal(message).expect("staged spec parses"); + assert_eq!(spec.title, "Bound accepted input"); + assert!(spec.body.contains("storage and transport")); + assert_eq!(spec.tickets.len(), 2); + assert_eq!(spec.tickets[0].title, "Bound storage input"); + assert!(spec.tickets[0].blocked_by.is_empty()); + assert_eq!(spec.tickets[1].title, "Bound transport input"); + assert_eq!(spec.tickets[1].blocked_by, vec![0]); + } + + #[test] + fn staged_spec_rejects_a_bare_url_final_line() { + let message = "Security fix Spec: https://github.com/acme/widgets/issues/8\n"; + assert!(parse_staged_spec_proposal(message).is_none()); + } + + #[test] + fn staged_spec_rejects_bad_blocking_edges() { + for message in [ + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 1\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 2\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\n", + ] { + assert!( + parse_staged_spec_proposal(message).is_none(), + "bad staged spec parses: {message}" + ); + } + } } diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 21bd3dd..1b83ed9 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -53,6 +53,12 @@ //! `gh api --method DELETE repos/<repo>/issues/<number>/labels/<label>` takes //! that label, whatever its case, off the issue, failing if the issue does //! not have it, as GitHub does. +//! `gh api --method POST repos/<repo>/issues/<parent>/sub_issues -F +//! sub_issue_id=<id>` links the issue as a native sub-issue of the parent. +//! `gh api --method POST repos/<repo>/issues/<ticket>/dependencies/blocked_by +//! -F issue_id=<id>` links the ticket as blocked by the issue. Database ids +//! are the issue numbers, and `gh api repos/<repo>/issues/<number> --jq .id` +//! answers with one. //! //! `gh api user` answers with the signed-in user's profile, whose public //! `email` is `user_email`, else null. `gh fake fails 'api user'` makes it @@ -649,6 +655,8 @@ fn issue_fields(state: &Json, n: &str, issue_state: &Json) -> Json { }; object([ ("number", number(n.parse::<i64>().unwrap())), + // The fake's database ids are the issue numbers. + ("id", number(n.parse::<i64>().unwrap())), ("state", issue_state.clone()), ("title", title), ("url", string(url)), @@ -710,6 +718,74 @@ fn issue_labels_put(state: &mut Json, args: &[&str]) { save(state); } +/// `gh api --method POST repos/<repo>/issues/<parent>/sub_issues -F +/// sub_issue_id=<id>`: link the issue as a native sub-issue of the parent. +/// The fake's database ids are the issue numbers. +fn api_add_sub_issue(state: &mut Json, args: &[&str]) { + let (positional, _) = parse(args); + let parent = positional.first().and_then(|path| { + let (repo, rest) = repo_prefix(path)?; + check_repo_is(state, Some(repo)); + rest.strip_prefix("issues/")? + .strip_suffix("/sub_issues") + .filter(|parent| state.at("issues").has(parent)) + }); + let child = args + .windows(2) + .filter(|pair| pair[0] == "-F") + .filter_map(|pair| pair[1].strip_prefix("sub_issue_id=")) + .next() + .filter(|child| state.at("issues").has(child)); + let (Some(parent), Some(child)) = (parent, child) else { + die( + &format!("fake gh: unsupported api POST {}", python_list(args)), + 2, + ) + }; + let mut sub = listed(state, "sub_issues", parent); + if !sub.iter().any(|ticket| ticket.python().as_str() == child) { + sub.push(number(child.parse::<i64>().unwrap())); + } + state + .entry("sub_issues", object([])) + .set(parent, Array(sub)); + save(state); +} + +/// `gh api --method POST repos/<repo>/issues/<ticket>/dependencies/blocked_by +/// -F issue_id=<id>`: link the ticket as blocked by the issue. The fake's +/// database ids are the issue numbers. +fn api_add_blocked_by(state: &mut Json, args: &[&str]) { + let (positional, _) = parse(args); + let ticket = positional.first().and_then(|path| { + let (repo, rest) = repo_prefix(path)?; + check_repo_is(state, Some(repo)); + rest.strip_prefix("issues/")? + .strip_suffix("/dependencies/blocked_by") + .filter(|ticket| state.at("issues").has(ticket)) + }); + let blocker = args + .windows(2) + .filter(|pair| pair[0] == "-F") + .filter_map(|pair| pair[1].strip_prefix("issue_id=")) + .next() + .filter(|blocker| state.at("issues").has(blocker)); + let (Some(ticket), Some(blocker)) = (ticket, blocker) else { + die( + &format!("fake gh: unsupported api POST {}", python_list(args)), + 2, + ) + }; + let mut blocked = listed(state, "blocked_by", ticket); + if !blocked.iter().any(|edge| edge.python().as_str() == blocker) { + blocked.push(number(blocker.parse::<i64>().unwrap())); + } + state + .entry("blocked_by", object([])) + .set(ticket, Array(blocked)); + save(state); +} + /// `gh api --method DELETE repos/<repo>/issues/<number>/labels/<label>`: take /// the label, whatever its case, off the issue. fn issue_label_delete(state: &mut Json, args: &[&str]) { @@ -1089,6 +1165,10 @@ fn api(state: &mut Json, positional: &[String], flags: &Flags) { && n.parse::<u64>().is_ok() { check_repo_is(state, Some(repo)); + if flag(flags, "jq") == Some(".id") { + println!("{n}"); + return; + } println!("{}", issue_fields(state, n, state.at("issues").at(n))); return; } @@ -1974,6 +2054,20 @@ pub fn main(args: Vec<String>) { } ["api", "--method", "PUT", rest @ ..] => issue_labels_put(&mut state, rest), ["api", "--method", "DELETE", rest @ ..] => issue_label_delete(&mut state, rest), + ["api", "--method", "POST", rest @ ..] + if rest + .first() + .is_some_and(|path| path.ends_with("/sub_issues")) => + { + api_add_sub_issue(&mut state, rest) + } + ["api", "--method", "POST", rest @ ..] + if rest + .first() + .is_some_and(|path| path.contains("/dependencies/blocked_by")) => + { + api_add_blocked_by(&mut state, rest) + } ["api", path, ..] if generate_notes_repo(path).is_some() => { generate_notes(&state, &args[1..]); } diff --git a/tests/security_run.rs b/tests/security_run.rs index 0318609..d129417 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -414,14 +414,13 @@ printf 'Proposed public issue:\nTitle: Bound accepted input\nBody:\nReject overs ) } -fn publish_spec(record: &str) -> String { +/// The staged spec path: the session returns its proposed public spec text +/// without running `gh issue create` itself. thirdshift validates every +/// staged text before creating any public issue. +fn stage_spec(record: &str) -> String { format!( r#" -url=$(gh issue create --title "Bound accepted input" --body "Bound input across storage and transport. Private record: {record}" --label needs-triage) -gh issue create --title "Bound storage input" --body "Bound storage input. Private record: {record}" --label ready-for-agent -gh issue create --title "Bound transport input" --body "Bound transport input. Private record: {record}" --label ready-for-agent -gh fake sub-issues 8 '[9,10]' -printf 'Security fix Spec: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +printf 'Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input across storage and transport. Private record: {record}\nTicket: Bound storage input\nBody:\nBound storage input. Private record: {record}\nBlocked by: none\nTicket: Bound transport input\nBody:\nBound transport input. Private record: {record}\nBlocked by: 1\n' > "$FAKE_CLAUDE_FINAL_MESSAGE" "# ) } @@ -448,11 +447,10 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { .replace("high single", "high spec") .replace("Fix size: single", "Fix size: spec") ); - state["blocked_by"]["10"] = json!([9]); scenario.write_gh_state(&state); scenario.agent_does_in_session( 1, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), ); scenario.agent_does_for(9, &implement_spec_ticket(9, 8)); scenario.agent_does_for( @@ -483,6 +481,12 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { ); } assert_eq!(state["labels"]["8"], json!(["security-fix", "in-progress"])); + // thirdshift created the Spec's native links from the staged text. + assert_eq!(state["sub_issues"]["8"], json!([9, 10])); + assert_eq!(state["blocked_by"]["10"], json!([9])); + let spec_body = state["bodies"]["8"].as_str().unwrap(); + assert!(spec_body.contains("storage and transport"), "{spec_body}"); + assert!(!spec_body.contains("Private candidate write-up")); let calls = scenario.claude_calls(); assert!( calls[0]["prompt"] @@ -496,21 +500,53 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { .unwrap() .contains("thirdshift-to-tickets") ); + assert!( + calls[0]["prompt"] + .as_str() + .unwrap() + .contains("instead of creating any public issue directly"), + "{}", + calls[0]["prompt"].as_str().unwrap() + ); assert_eq!(calls.len(), 4); } #[test] fn invalid_security_fix_specs_are_not_marked_ready_or_dispatched() { + // A public spec has no session-created path: every entry here is either + // a missing staged spec or staged text that validation rejects before + // thirdshift creates anything. let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let staged = stage_spec(record); for script in [ - format!("{}gh fake sub-issues 8 '[]'\n", publish_spec(record)), - publish_spec(record).replace("Security fix Spec:", "Security fix Ticket:"), - publish_spec(record).replace("Bound storage input. Private record:", "Private candidate write-up. Private record:"), - publish_spec(record).replace("Bound storage input. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", "Bound storage input."), - format!("{}gh fake created 9 '2020-01-01T00:00:00Z'\n", publish_spec(record)), - format!("{}gh issue close 9\n", publish_spec(record)), - format!("{}gh fake labels 9 '[\"ready-for-human\"]'\n", publish_spec(record)), - format!("{}gh fake sub-issues 9 '[7]'\n", publish_spec(record)), + // No staged spec at all. + "printf '%s\\n' 'No protocol block' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n".to_string(), + // A session-created URL final line stages nothing. + "printf 'Security fix Spec: https://github.com/acme/widgets/issues/8\\n' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n".to_string(), + // The top issue copies private write-up text. + staged.replace( + "Bound input across storage and transport. Private record:", + "Private candidate write-up. Private record:", + ), + // A Ticket copies private write-up text. + staged.replace( + "Bound storage input. Private record:", + "Private candidate write-up. Private record:", + ), + // The top issue omits the private record link. + staged.replace( + "Bound input across storage and transport. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", + "Bound input across storage and transport.", + ), + // A Ticket omits the private record link. + staged.replace( + "Bound transport input. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", + "Bound transport input.", + ), + // A Ticket blocked by itself. + staged.replace("Blocked by: 1", "Blocked by: 2"), + // A Ticket blocked by no such Ticket. + staged.replace("Blocked by: 1", "Blocked by: 3"), ] { let scenario = with_reproduced_findings(&["high"]); let mut state = scenario.gh_state(); @@ -519,12 +555,62 @@ fn invalid_security_fix_specs_are_not_marked_ready_or_dispatched() { 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.gh_state()["labels"]["8"], json!(["needs-triage"])); + assert!( + scenario.gh_state()["bodies"].get("8").is_none(), + "{script}: a rejected staged spec must not create a public issue: {}", + scenario.gh_state()["bodies"] + ); assert!(scenario.gh_state()["prs"].as_array().unwrap().is_empty()); assert_eq!(scenario.claude_calls().len(), 1); } } +#[test] +fn a_spec_session_side_effect_public_issue_fails_the_fix_and_is_closed() { + // The staged spec path validates before creating: a session that stages + // valid spec text but also creates a public issue on the side must fail + // loudly, and thirdshift closes the leak instead of reporting success. + let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["advisories"][0]["description"] = json!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("high single", "high spec") + ); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &format!( + "{}\ngh issue create --title 'Leaked write-up' --body 'Private candidate write-up. Private record: {record}' --label needs-triage > /dev/null\n", + stage_spec(record), + ), + ); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result + .stderr + .contains("created public issue(s) outside the staged path: #8"), + "{}", + result.stderr + ); + let state = scenario.gh_state(); + assert_eq!( + state["issues"]["8"], "CLOSED", + "the session-created side-effect issue must be closed: {}", + state["issues"] + ); + assert!( + state["bodies"].get("9").is_none(), + "no validated issue may follow a side-effect leak: {}", + state["bodies"] + ); + assert!(state["prs"].as_array().unwrap().is_empty()); + assert_eq!(scenario.claude_calls().len(), 1); +} + #[test] fn fixing_selects_the_most_severe_record_then_dispatches_one_ticket_before_auditing() { let scenario = with_reproduced_findings(&["low", "critical", "critical", "high"]); @@ -670,7 +756,7 @@ fn a_staged_proposal_is_created_by_thirdshift_not_the_session() { let calls = scenario.claude_calls(); let prompt = calls[0]["prompt"].as_str().unwrap(); assert!( - prompt.contains("instead of creating the public issue directly"), + prompt.contains("instead of creating any public issue directly"), "{prompt}" ); } @@ -907,7 +993,7 @@ fn an_audit_goes_on_to_one_reproduced_fix_when_fixing_is_allowed() { scenario.agent_does_in_session(3, &reproduction_script("reproduced critical spec")); scenario.agent_does_in_session( 4, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-test-test-0002"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-test-test-0002"), ); scenario.agent_does_for(9, &implement_spec_ticket(9, 8)); scenario.agent_does_for(10, &implement_spec_ticket(10, 8)); @@ -1087,10 +1173,14 @@ printf '%s\n' 'Security fix Spec: https://github.com/acme/widgets/issues/7' > "$ state["advisories"][0]["description"] = json!(evidence); scenario.agent_does_in_session( 1, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), ); } - state["blocked_by"][(ticket + 1).to_string()] = json!([ticket]); + if private { + // The private session links its own Tickets; the public staged + // path has thirdshift create the blocking edge itself. + state["blocked_by"][(ticket + 1).to_string()] = json!([ticket]); + } scenario.write_gh_state(&state); scenario.agent_does_for(ticket, "exit 1\n"); let failed = scenario.run(&["secure", "security-fix", "parallel", "1"]); From f25eed3de4eb888ccb67340199205aac29c816ba Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Sat, 10 Oct 2026 12:48:58 -0400 Subject: [PATCH 2/5] Share the by-id POST shape behind staged Spec issue links Standards review finding: add_sub_issue and add_blocked_by repeated the database-id lookup plus single-field POST shape. Both delegate to a private post_by_id helper now. --- src/github.rs | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/github.rs b/src/github.rs index fa26cc2..4a01614 100644 --- a/src/github.rs +++ b/src/github.rs @@ -193,34 +193,36 @@ impl GitHub { /// Link `child` as a native sub-issue of `parent`. pub fn add_sub_issue(&self, parent: &IssueUrl, child: &IssueUrl) -> Result<()> { let child_id = self.issue_db_id(child)?; - self.gh(&[ - "api", - "--method", - "POST", - &format!( - "repos/{}/issues/{}/sub_issues", - parent.repo_slug(), - parent.number - ), - "-F", - &format!("sub_issue_id={child_id}"), - ]) + self.post_by_id( + parent, + &format!("issues/{}/sub_issues", parent.number), + "sub_issue_id", + child_id, + ) } /// Add a native "blocked by" edge: `ticket` is blocked by `blocker`. pub fn add_blocked_by(&self, ticket: &IssueUrl, blocker: &IssueUrl) -> Result<()> { let blocker_id = self.issue_db_id(blocker)?; + self.post_by_id( + ticket, + &format!("issues/{}/dependencies/blocked_by", ticket.number), + "issue_id", + blocker_id, + ) + } + + /// `POST` one database-id field to the `endpoint` below + /// `repos/<owner>/<repo>`: the shared shape behind issue links that take + /// an id instead of an issue number. + fn post_by_id(&self, issue: &IssueUrl, endpoint: &str, field: &str, id: u64) -> Result<()> { self.gh(&[ "api", "--method", "POST", - &format!( - "repos/{}/issues/{}/dependencies/blocked_by", - ticket.repo_slug(), - ticket.number - ), + &format!("repos/{}/{}", issue.repo_slug(), endpoint), "-F", - &format!("issue_id={blocker_id}"), + &format!("{field}={id}"), ]) } From 0c067363c80f2ebf7eec1f6b91e11d6f238c0665 Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Sat, 10 Oct 2026 14:26:11 -0400 Subject: [PATCH 3/5] Sweep the spec publish window for unlinked public issues (#648) After the staged spec and its Tickets are created and linked, re-sweep the publishing session window. Anything in it that is neither the spec nor one of its Tickets is a side-effect leak outside the validated staged path: close it and fail loudly, as the pre-creation sweep does. --- src/security/fixing.rs | 73 ++++++++++++++++++++++++++++++++++-------- tests/security_run.rs | 11 +++++++ 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/security/fixing.rs b/src/security/fixing.rs index ab60079..a67d43c 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use anyhow::{Context, Result, bail}; -use chrono::Utc; +use chrono::{DateTime, Utc}; use crate::git::Git; use crate::github::{GitHub, SecurityRecord}; @@ -87,17 +87,7 @@ pub fn publish( // then fail loudly so the Day shift triages the exposure. let leaked = github.issues_created_since(&repo.slug(), started)?; if !leaked.is_empty() { - for side in &leaked { - github.close_issue(side, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; - } - let numbers = leaked - .iter() - .map(|side| format!("#{}", side.number)) - .collect::<Vec<_>>() - .join(", "); - bail!( - "the Security fix publishing session created public issue(s) outside the staged path: {numbers}" - ); + return fail_on_side_effects(&github, &leaked); } if size == FixSize::Single { let Some((title, body)) = message.as_deref().and_then(parse_staged_proposal) else { @@ -111,7 +101,7 @@ pub fn publish( record.check_public_fix_text(&format!("{title}\n\n{body}"), url)?; github.create_issue_in(repo, &title, &body, &[NEEDS_TRIAGE])? } else { - publish_staged_spec(&github, repo, record, url, message.as_deref())? + publish_staged_spec(&github, repo, record, url, message.as_deref(), started)? } } else { let line = message @@ -306,6 +296,7 @@ fn publish_staged_spec( record: &SecurityRecord, url: &str, message: Option<&str>, + started: DateTime<Utc>, ) -> Result<IssueUrl> { let Some(staged) = message.and_then(parse_staged_spec_proposal) else { bail!("the Security fix publishing session did not stage its proposed public spec text"); @@ -332,9 +323,52 @@ fn publish_staged_spec( github.add_blocked_by(ticket, &tickets[*edge])?; } } + // The session has no independent public-issue path, so anything in its + // window that is neither the staged spec nor one of its Tickets is a + // side-effect leak outside the validated staged path. The pre-creation + // sweep runs before any staged issue exists; this one runs after + // linking, so the staged issues themselves must not count. + let unlinked = unlinked_issues( + github.issues_created_since(&repo.slug(), started)?, + &spec, + &tickets, + ); + if !unlinked.is_empty() { + return fail_on_side_effects(github, &unlinked); + } Ok(spec) } +/// The issues in the publishing session's `window` that are neither the +/// staged `spec` nor one of its `tickets`: side-effect leaks outside the +/// validated staged path. +fn unlinked_issues(window: Vec<IssueUrl>, spec: &IssueUrl, tickets: &[IssueUrl]) -> Vec<IssueUrl> { + window + .into_iter() + .filter(|issue| { + issue.number != spec.number + && !tickets.iter().any(|ticket| ticket.number == issue.number) + }) + .collect() +} + +/// Close `sides`, public issues a Security fix publishing session created +/// outside the validated staged path, then fail loudly so the Day shift +/// triages the exposure. +fn fail_on_side_effects(github: &GitHub, sides: &[IssueUrl]) -> Result<IssueUrl> { + for side in sides { + github.close_issue(side, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; + } + let numbers = sides + .iter() + .map(|side| format!("#{}", side.number)) + .collect::<Vec<_>>() + .join(", "); + bail!( + "the Security fix publishing session created public issue(s) outside the staged path: {numbers}" + ) +} + /// The single public fix the publishing session staged for validation: /// its proposed `(title, body)`, instead of a session-created public issue. fn parse_staged_proposal(message: &str) -> Option<(String, String)> { @@ -398,6 +432,19 @@ mod tests { assert!(parse_staged_spec_proposal(message).is_none()); } + #[test] + fn unlinked_issues_keeps_only_issues_outside_the_staged_spec() { + let issue = |number: u64| { + IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")).unwrap() + }; + let spec = issue(8); + let tickets = vec![issue(9), issue(10)]; + let window = vec![issue(8), issue(9), issue(10), issue(11)]; + assert_eq!(unlinked_issues(window, &spec, &tickets), vec![issue(11)]); + let window = vec![issue(8), issue(9), issue(10)]; + assert!(unlinked_issues(window, &spec, &tickets).is_empty()); + } + #[test] fn staged_spec_rejects_bad_blocking_edges() { for message in [ diff --git a/tests/security_run.rs b/tests/security_run.rs index d129417..40ab6fe 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -484,6 +484,17 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { // thirdshift created the Spec's native links from the staged text. assert_eq!(state["sub_issues"]["8"], json!([9, 10])); assert_eq!(state["blocked_by"]["10"], json!([9])); + // The post-publish window sweep must leave the staged issues alone. + assert_eq!( + state["issues"]["8"], "OPEN", + "the staged spec must survive the window sweep: {}", + state["issues"] + ); + assert!( + !result.stderr.contains("outside the staged path"), + "{}", + result.stderr + ); let spec_body = state["bodies"]["8"].as_str().unwrap(); assert!(spec_body.contains("storage and transport"), "{spec_body}"); assert!(!spec_body.contains("Private candidate write-up")); From 611ae31da9268027061a47e9700acc04857367eb Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Sat, 10 Oct 2026 14:34:48 -0400 Subject: [PATCH 4/5] Fail closed on multiline Blocked by trailers; name side-effect leaks (#648) The staged-spec Blocked by trailer parsed only its first line, silently dropping a stated ordering edge while every other malformation rejects the staged text. Reject non-blank lines after the trailer's first line. Rename fail_on_side_effects sides to leaks, the term the surrounding comments use. --- src/security/fixing.rs | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/security/fixing.rs b/src/security/fixing.rs index a67d43c..a847b7b 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -255,11 +255,16 @@ fn parse_staged_spec_proposal(message: &str) -> Option<StagedSpec> { /// The `Blocked by:` trailer of one staged Ticket as 0-based indices into /// its Spec's Tickets: `none` is empty, else comma-separated 1-based -/// positions. Reading stops at the next Ticket section or the message end, -/// so only the trailer's first line counts. `None` unless every position -/// names another Ticket of the `count` staged. +/// positions on a single line. Anything but blank lines after that first +/// line is malformed staged text: ignoring it would silently drop a stated +/// ordering edge, so validation fails closed instead. `None` unless every +/// position names another Ticket of the `count` staged. fn parse_blocked_by(blocked: &str, index: usize, count: usize) -> Option<Vec<usize>> { - let line = blocked.lines().next().unwrap_or_default().trim(); + let mut lines = blocked.lines(); + let line = lines.next().unwrap_or_default().trim(); + if lines.any(|line| !line.trim().is_empty()) { + return None; + } let line = line.trim_end_matches(['.', ';']); if line.eq_ignore_ascii_case("none") { return Some(Vec::new()); @@ -352,16 +357,16 @@ fn unlinked_issues(window: Vec<IssueUrl>, spec: &IssueUrl, tickets: &[IssueUrl]) .collect() } -/// Close `sides`, public issues a Security fix publishing session created +/// Close `leaks`, public issues a Security fix publishing session created /// outside the validated staged path, then fail loudly so the Day shift /// triages the exposure. -fn fail_on_side_effects(github: &GitHub, sides: &[IssueUrl]) -> Result<IssueUrl> { - for side in sides { - github.close_issue(side, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; +fn fail_on_side_effects(github: &GitHub, leaks: &[IssueUrl]) -> Result<IssueUrl> { + for leak in leaks { + github.close_issue(leak, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; } - let numbers = sides + let numbers = leaks .iter() - .map(|side| format!("#{}", side.number)) + .map(|leak| format!("#{}", leak.number)) .collect::<Vec<_>>() .join(", "); bail!( @@ -445,6 +450,15 @@ mod tests { assert!(unlinked_issues(window, &spec, &tickets).is_empty()); } + #[test] + fn staged_spec_rejects_a_trailing_blocked_by_edge_on_its_own_line() { + let message = "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: First ticket\nBody:\nFirst ticket. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Second ticket\nBody:\nSecond ticket. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Third ticket\nBody:\nThird ticket. Private record: https://example.invalid/advisory\nBlocked by: 1\n2\n"; + assert!( + parse_staged_spec_proposal(message).is_none(), + "a second Blocked by edge on its own line was silently dropped instead of rejected" + ); + } + #[test] fn staged_spec_rejects_bad_blocking_edges() { for message in [ From af7e77100addad97a326f494565529df53eccc3b Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Sat, 10 Oct 2026 15:43:57 -0400 Subject: [PATCH 5/5] Reject forward and cyclic Blocked by edges in staged specs (#646) parse_blocked_by accepted any in-range Ticket position except self-edges, so a staged spec could name later Tickets as blockers or form a cycle and still publish. A cyclic spec dispatches Tickets that can never unblock. Validation now fails closed unless every edge names an earlier Ticket, as the staged format ('each later Ticket naming the positions of the Tickets that block it') requires. Keeps the Spec review's cyclic-rejection integration test and a forward-edge unit case. --- src/security/fixing.rs | 13 ++++++++----- tests/security_run.rs | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/security/fixing.rs b/src/security/fixing.rs index a847b7b..5853194 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -255,10 +255,12 @@ fn parse_staged_spec_proposal(message: &str) -> Option<StagedSpec> { /// The `Blocked by:` trailer of one staged Ticket as 0-based indices into /// its Spec's Tickets: `none` is empty, else comma-separated 1-based -/// positions on a single line. Anything but blank lines after that first -/// line is malformed staged text: ignoring it would silently drop a stated -/// ordering edge, so validation fails closed instead. `None` unless every -/// position names another Ticket of the `count` staged. +/// positions of earlier Tickets on a single line, since each later Ticket +/// names the Tickets that block it. Anything but blank lines after that +/// first line is malformed staged text: ignoring it would silently drop a +/// stated ordering edge, so validation fails closed instead. `None` unless +/// every position names an earlier Ticket of the `count` staged: a forward +/// or cyclic edge would dispatch Tickets that can never unblock. fn parse_blocked_by(blocked: &str, index: usize, count: usize) -> Option<Vec<usize>> { let mut lines = blocked.lines(); let line = lines.next().unwrap_or_default().trim(); @@ -276,7 +278,7 @@ fn parse_blocked_by(blocked: &str, index: usize, count: usize) -> Option<Vec<usi .filter(|edge| !edge.is_empty()) { let position: usize = position.parse().ok()?; - if position == 0 || position > count || position - 1 == index { + if position == 0 || position > count || position > index { return None; } let edge = position - 1; @@ -464,6 +466,7 @@ mod tests { for message in [ "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 1\n", "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 2\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: First ticket\nBody:\nFirst ticket. Private record: https://example.invalid/advisory\nBlocked by: 2\nTicket: Second ticket\nBody:\nSecond ticket. Private record: https://example.invalid/advisory\nBlocked by: none\n", "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\n", ] { assert!( diff --git a/tests/security_run.rs b/tests/security_run.rs index 40ab6fe..e6714e5 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -622,6 +622,41 @@ fn a_spec_session_side_effect_public_issue_fails_the_fix_and_is_closed() { assert_eq!(scenario.claude_calls().len(), 1); } +#[test] +fn a_staged_spec_with_cyclic_blocking_edges_is_rejected_before_creating_anything() { + // Ticket 1 names a later Ticket as its blocker and vice versa: no + // execution order satisfies both edges, so validation must reject the + // staged text before thirdshift creates any public issue. + let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["advisories"][0]["description"] = json!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("high single", "high spec") + ); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &format!( + "printf 'Proposed public spec:\\nTitle: Bound accepted input\\nBody:\\nBound input across storage and transport. Private record: {record}\\nTicket: Bound storage input\\nBody:\\nBound storage input. Private record: {record}\\nBlocked by: 2\\nTicket: Bound transport input\\nBody:\\nBound transport input. Private record: {record}\\nBlocked by: 1\\n' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + ), + ); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!( + result.code, + Some(1), + "cyclic staged spec must be rejected: {}", + result.stderr + ); + assert!( + scenario.gh_state()["bodies"].get("8").is_none(), + "a rejected staged spec must not create a public issue: {}", + scenario.gh_state()["bodies"] + ); +} + #[test] fn fixing_selects_the_most_severe_record_then_dispatches_one_ticket_before_auditing() { let scenario = with_reproduced_findings(&["low", "critical", "critical", "high"]);