From 249899e42c09364578df004212b2372a53016b20 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 04:27:59 -0400 Subject: [PATCH 1/3] Record old Security review findings privately (#552) --- README.md | 2 +- prompts/security-review.md | 11 +- site/prompts/index.html | 11 +- src/delivery.rs | 13 +- src/prompt.rs | 14 +- src/prompts_page.rs | 4 +- src/security/audit.rs | 2 +- src/security/review.rs | 122 ++++++++++++++- tests/security_review.rs | 304 ++++++++++++++++++++++++++++++++++--- 9 files changed, 439 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 59e9b8d4..1d8c1f78 100644 --- a/README.md +++ b/README.md @@ -776,7 +776,7 @@ A finding's title becomes the advisory summary. Its description preserves its wr `security-review`, or `review = true` under `[security]`, enables one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. `no-security-review` overrides the setting. Both words, with or without dashes, are accepted on commands that start Runs and follow dispatched Runs and Base fixes. Setup asks about the setting; it is off by default. -The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run that is not a Merge run records the outcome and continues. Pre-existing vulnerability details stay out of the PR. Spec PR review and private recording of old findings are tracked separately in [#551](https://github.com/JacobStephens2/thirdshift/issues/551) and [#552](https://github.com/JacobStephens2/thirdshift/issues/552). The separate Security audit retains its full audit workflow. +The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run that is not a Merge run records the outcome and continues. An old finding, whose proof-of-concept also fails at the pinned merge base, is reported in a private file under the repository's logs (`security-reviews/`). thirdshift records it as a draft advisory, or a `security-finding`/`needs-triage` issue on a private repository, reusing existing fingerprints and preserving the Day shift's grades. Its details stay out of the PR and it does not hold Self-merge; fixing belongs to a separate Security run. Spec PR review is tracked separately in [#551](https://github.com/JacobStephens2/thirdshift/issues/551). The separate Security audit retains its full audit workflow. ### Fencing diff --git a/prompts/security-review.md b/prompts/security-review.md index aaf36b34..c61a9dc3 100644 --- a/prompts/security-review.md +++ b/prompts/security-review.md @@ -6,16 +6,19 @@ When enabled, runs one guidance review after a Run's opening session and before ``` Use the `thirdshift-security-audit` skill in guidance mode. -Review the change for on branch with `git diff ...HEAD`, including supporting code. +Review the change for on branch against Base branch with `git diff ...HEAD`, including supporting code. Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present. Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts. Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix. -Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here. +Merge base commit: ``. +Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here. +Private report file: ``. +Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session. If refused, incomplete, or missing a prerequisite, say so and do not claim completion. After a complete review, end your final message with exactly one line of the form: -Security review: {"unaddressed_count":0,"findings":[]} -Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it. +Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0} +unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line. 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 86a7690d..32281cf4 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -163,16 +163,19 @@

Security review

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

Sent by 3 · Y · Review

Use the `thirdshift-security-audit` skill in guidance mode.
-Review the change for <Issue URL> on branch <branch> with `git diff <base>...HEAD`, including supporting code.
+Review the change for <Issue URL> on branch <branch> against Base branch <base> with `git diff <merge base commit>...HEAD`, including supporting code.
 Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.
 Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.
 Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.
-Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.
+Merge base commit: `<merge base commit>`.
+Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here.
+Private report file: `<private report file>`.
+Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.
 Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.
 If refused, incomplete, or missing a prerequisite, say so and do not claim completion.
 After a complete review, end your final message with exactly one line of the form:
-Security review: {"unaddressed_count":0,"findings":[]}
-Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.
+Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0}
+unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line.
 
 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/delivery.rs b/src/delivery.rs index 2194855b..25406b40 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -376,12 +376,13 @@ impl Outside for InWorktree<'_> { } fn security_review(&mut self, prompt: &str) -> Result { - let message = self.sessions.run_to_final_message( - crate::session::Purpose::Security, - "security-review", + crate::security::review::run( + self.sessions, + self.worktree.path(), + self.issue, + self.base, prompt, - )?; - Review::from_final_message(message.as_deref()) + ) } fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> { @@ -634,7 +635,7 @@ mod tests { } fn security_review(&mut self, _: &str) -> Result { Review::from_final_message(Some( - "Security review: {\"unaddressed_count\":0,\"findings\":[]}", + "Security review: {\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}", )) } fn catch_up(&mut self) -> Result<()> { diff --git a/src/prompt.rs b/src/prompt.rs index 1383f7a9..8f98f8fb 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -316,19 +316,25 @@ pub fn security_audit( /// One guidance session on the Run's change, before Delivery pushes and /// enters the Repair loop. The separate Security audit remains a full audit. +pub const SECURITY_REVIEW_REPORT_FILE: &str = ""; +pub const SECURITY_REVIEW_MERGE_BASE: &str = ""; + pub fn security_review(issue: &IssueUrl, base: &str, branch: &str) -> String { format!( "Use the `thirdshift-security-audit` skill in guidance mode.\n\ - Review the change for {url} on branch {branch} with `git diff {base}...HEAD`, including supporting code.\n\ + Review the change for {url} on branch {branch} against Base branch {base} with `git diff {SECURITY_REVIEW_MERGE_BASE}...HEAD`, including supporting code.\n\ Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.\n\ Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.\n\ Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.\n\ - Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.\n\ + Merge base commit: `{SECURITY_REVIEW_MERGE_BASE}`.\n\ + Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here.\n\ + Private report file: `{SECURITY_REVIEW_REPORT_FILE}`.\n\ + Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.\n\ Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.\n\ If refused, incomplete, or missing a prerequisite, say so and do not claim completion.\n\ After a complete review, end your final message with exactly one line of the form:\n\ - Security review: {{\"unaddressed_count\":0,\"findings\":[]}}\n\ - Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.\n\n\ + Security review: {{\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}}\n\ + unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line.\n\n\ {HEADLESS}", url = issue.url, ) diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 468fe5ab..33993604 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -60,7 +60,7 @@ const SESSION_ID: &str = ""; const PROMPT: &str = ""; const MODEL: &str = ""; const EFFORT: &str = ""; -const PLACEHOLDERS: [&str; 18] = [ +const PLACEHOLDERS: [&str; 20] = [ ISSUE_URL, SPEC_URL, NUMBER, @@ -79,6 +79,8 @@ const PLACEHOLDERS: [&str; 18] = [ MODEL, EFFORT, prompt::REVIEW_REPORTS_DIRECTORY, + prompt::SECURITY_REVIEW_REPORT_FILE, + prompt::SECURITY_REVIEW_MERGE_BASE, ]; /// Who sends a prompt. diff --git a/src/security/audit.rs b/src/security/audit.rs index 4f7565a6..539b52c5 100644 --- a/src/security/audit.rs +++ b/src/security/audit.rs @@ -219,7 +219,7 @@ fn node(args: &[&str]) -> Result<()> { } /// Manifest identity only: no affected or patched versions are inferred. -fn package_in(worktree: &Path) -> Package { +pub(super) fn package_in(worktree: &Path) -> Package { for (file, section, ecosystem) in [ ("Cargo.toml", "package", "rust"), ("pyproject.toml", "project", "pip"), diff --git a/src/security/review.rs b/src/security/review.rs index b888fa7d..587ac70a 100644 --- a/src/security/review.rs +++ b/src/security/review.rs @@ -2,10 +2,127 @@ //! appear here; pre-existing vulnerability details stay out of public text. use anyhow::{Context, Result, bail}; -use serde::Deserialize; -use std::fmt; +use serde::{Deserialize, Serialize}; +use std::{fmt, fs, path::Path}; +use crate::git::Git; +use crate::github::{DraftAdvisory, GitHub}; use crate::harness::interpretation::SafeguardRefusal; +use crate::issue::IssueUrl; +use crate::session::{Purpose, Sessions}; +use crate::{logs, progress, prompt}; + +/// A fresh report outside the worktree, retained even when the session or +/// private recording fails. Only the introduced titles reach Delivery. +pub(crate) fn run( + sessions: &Sessions<'_>, + worktree: &Path, + issue: &IssueUrl, + base: &str, + text: &str, +) -> Result { + let merge_base = Git::new(worktree).run(&["merge-base", &format!("origin/{base}"), "HEAD"])?; + let root = logs::root(&issue.repo()).join("security-reviews"); + fs::create_dir_all(&root)?; + let directory = tempfile::Builder::new() + .prefix(&format!("{}-{}-", issue.number, logs::stamp())) + .tempdir_in(root)? + .keep(); + let report = directory.join("pre-existing.json"); + let text = text + .replace( + prompt::SECURITY_REVIEW_REPORT_FILE, + &report.to_string_lossy(), + ) + .replace(prompt::SECURITY_REVIEW_MERGE_BASE, &merge_base); + progress::step(format!( + "Security review private report: {}", + report.display() + )); + let message = sessions.run_to_final_message(Purpose::Security, "security-review", &text)?; + let review = Review::from_final_message(message.as_deref())?; + let findings: Vec = serde_json::from_slice(&fs::read(report)?)?; + if findings.len() != review.pre_existing_count { + bail!("Security review's private report does not match its final count"); + } + // Validate the entire report before creating any record. Malformed report + // diagnostics are never passed into the public outcome. + for finding in &findings { + finding.validate()?; + } + if !findings.is_empty() { + let github = GitHub::new(); + let repo = issue.repo_slug(); + let mut known = github.security_records(&repo)?; + let package = super::audit::package_in(worktree); + for finding in findings { + let draft = DraftAdvisory { + fingerprint: finding.fingerprint.clone(), + summary: finding.title.clone(), + description: format!( + "Found by thirdshift's Security review.\n\nFingerprint: `{}`\nAudited commit: `{merge_base}`\nReview issue: {}\n\n{}\n\n```json\n{}\n```\n", + finding.fingerprint, + issue.url, + finding.description, + serde_json::to_string_pretty(&finding)? + ), + package: package.clone(), + }; + // Reuse every record state and preserve the Day shift's grade. + if known.finding(&draft).is_none() { + let record = github.create_security_record(&repo, &known, &draft)?; + known.remember(record); + progress::step("recorded a pre-existing Security finding privately"); + } + } + } + Ok(review) +} + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct OldFinding { + fingerprint: String, + title: String, + description: String, + proof_of_concept: ProofOfConcept, +} + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct ProofOfConcept { + test: String, + command: String, + head_exit_code: i32, + merge_base_exit_code: i32, + notes: String, +} + +impl OldFinding { + fn validate(&self) -> Result<()> { + let proof = &self.proof_of_concept; + if [ + &self.fingerprint, + &self.title, + &self.description, + &proof.test, + &proof.command, + &proof.notes, + ] + .iter() + .any(|text| text.trim().is_empty()) + || self.fingerprint.contains(['\r', '\n', '`']) + || self.title.contains(['\r', '\n']) + || proof.head_exit_code <= 0 + || proof.merge_base_exit_code <= 0 + { + bail!( + "Security review's private report lacks a valid old finding and failing proof-of-concept runs" + ); + } + Ok(()) + } +} #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Ask { @@ -73,6 +190,7 @@ impl std::error::Error for Hold {} pub(crate) struct Review { unaddressed_count: usize, pub findings: Vec, + pre_existing_count: usize, } impl Review { diff --git a/tests/security_review.rs b/tests/security_review.rs index 7b5d118a..edd02298 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -1,5 +1,6 @@ mod support; +use serde_json::json; use support::Scenario; const OPENS_PR: &str = r#" @@ -9,7 +10,270 @@ git commit -q -m 'Add feature' gh pr create --base main --head issue-7 --title 'Add feature' --body 'Closes #7' "#; -const CLEAN: &str = r#"printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#; +fn old_finding() -> serde_json::Value { + json!({ + "fingerprint": "old-input-bound", + "title": "Old unchecked bound", + "description": "Private old vulnerability evidence.", + "proof_of_concept": { + "test": "assert_bounded_input();", + "command": "cargo test bounded_input", + "head_exit_code": 101, + "merge_base_exit_code": 101, + "notes": "The bounded local fixture fails at both commits." + } + }) +} + +fn review_script(old: &serde_json::Value, introduced: &[&str]) -> String { + format!( + r#" +printf '%s' "$FAKE_CLAUDE_PROMPT" > review-prompt.txt +report=$(sed -n 's/^Private report file: `\(.*\)`\.$/\1/p' review-prompt.txt) +test -n "$report" +printf '%s\n' '{old}' > "$report" +printf '%s\n' 'Security review: {outcome}' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + outcome = json!({ + "unaddressed_count": introduced.len(), + "findings": introduced, + "pre_existing_count": old.as_array().unwrap().len() + }) + ) +} + +#[test] +fn an_old_finding_is_recorded_privately_without_holding_self_merge() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([old_finding()]), &[])); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + "merge", + "security-fix", + ]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + let records = state["advisories"] + .as_array() + .expect("private draft advisory"); + assert_eq!(records.len(), 1); + assert_eq!(records[0]["state"], "draft"); + assert!(records[0]["severity"].is_null()); + let description = records[0]["description"].as_str().unwrap(); + assert!(description.contains("Fingerprint: `old-input-bound`")); + assert!(description.contains("Private old vulnerability evidence.")); + assert!(description.contains("assert_bounded_input();")); + assert!(description.contains(scenario.launch_git(&["rev-parse", "main"]).trim())); + assert_eq!(state["prs"][0]["state"], "MERGED"); + assert_eq!( + scenario.claude_calls().len(), + 2, + "old findings belong to a Security run" + ); + for private in [ + "Old unchecked bound", + "old-input-bound", + "Private old vulnerability evidence.", + "assert_bounded_input();", + ] { + assert!(!state["prs"][0]["body"].as_str().unwrap().contains(private)); + assert!(!result.stderr.contains(private)); + assert!(!result.stdout.contains(private)); + } + let reports = scenario.entries("home/.thirdshift/logs/acme/widgets/security-reviews"); + assert_eq!(reports.len(), 1); + let kept: serde_json::Value = serde_json::from_slice( + &std::fs::read(scenario.path(&format!( + "home/.thirdshift/logs/acme/widgets/security-reviews/{}/pre-existing.json", + reports[0] + ))) + .unwrap(), + ) + .unwrap(); + assert_eq!(kept, json!([old_finding()])); +} + +#[test] +fn mixed_old_and_introduced_findings_keep_the_introduced_hold_and_old_details_private() { + for harness in ["claude", "codex"] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + &review_script(&json!([old_finding()]), &["Introduced cross-tenant read"]), + ); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + "merge", + "harness", + harness, + ]); + assert_eq!(result.code, Some(1), "{harness}: {}", result.stderr); + let state = scenario.gh_state(); + assert_eq!(state["advisories"].as_array().unwrap().len(), 1); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); + let body = state["prs"][0]["body"].as_str().unwrap(); + assert!(body.contains("Introduced cross-tenant read")); + assert!(result.stderr.contains("Introduced cross-tenant read")); + for private in [ + "Old unchecked bound", + "old-input-bound", + "Private old vulnerability evidence.", + "assert_bounded_input();", + ] { + assert!(!body.contains(private)); + assert!(!result.stderr.contains(private)); + } + } +} + +#[test] +fn repeated_fingerprints_reuse_private_records_and_preserve_day_shift_grades() { + for (private, record_state) in [ + (false, "draft"), + (false, "published"), + (false, "closed"), + (true, "OPEN"), + (true, "CLOSED"), + ] { + let scenario = Scenario::new(); + let mut state = scenario.gh_state(); + state["private"] = json!(private); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, OPENS_PR); + // Duplicates within one report must match just as later reviews do. + scenario.agent_does_in_session( + 2, + &review_script(&json!([old_finding(), old_finding()]), &[]), + ); + let first = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); + assert_eq!( + first.code, + Some(0), + "{private} {record_state}: {}", + first.stderr + ); + let mut state = scenario.gh_state(); + if private { + assert!(state["advisories"].is_null()); + assert_eq!(state["issues"].as_object().unwrap().len(), 2); + assert_eq!( + state["labels"]["8"], + json!(["security-finding", "needs-triage"]) + ); + assert!( + state["bodies"]["8"] + .as_str() + .unwrap() + .contains("Private old vulnerability evidence.") + ); + state["issues"]["8"] = json!(record_state); + state["labels"]["8"] = json!(["security-finding"]); + } else { + assert_eq!(state["advisories"].as_array().unwrap().len(), 1); + state["advisories"][0]["state"] = json!(record_state); + state["advisories"][0]["severity"] = json!("low"); + } + scenario.write_gh_state(&state); + scenario.agent_does_in_session(3, "true"); + let mut repeated = old_finding(); + repeated["description"] = json!("A later review must not replace the grade or write-up."); + scenario.agent_does_in_session(4, &review_script(&json!([repeated]), &[])); + let second = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!( + second.code, + Some(0), + "{private} {record_state}: {}", + second.stderr + ); + let after = scenario.gh_state(); + assert_eq!(after["prs"][0]["state"], "MERGED"); + if private { + assert_eq!(after["issues"]["8"], state["issues"]["8"]); + assert_eq!(after["labels"]["8"], state["labels"]["8"]); + assert_eq!(after["bodies"]["8"], state["bodies"]["8"]); + assert_eq!(after["issues"].as_object().unwrap().len(), 2); + } else { + assert_eq!(after["advisories"], state["advisories"]); + } + let body = after["prs"][0]["body"].as_str().unwrap(); + assert!(!body.contains("Old unchecked bound")); + assert!(!body.contains("old-input-bound")); + } +} + +#[test] +fn incomplete_private_reports_hold_self_merge_without_publishing_report_diagnostics() { + let valid = review_script(&json!([old_finding()]), &[]); + let mut passes_at_base = old_finding(); + passes_at_base["proof_of_concept"]["merge_base_exit_code"] = json!(0); + let mut no_test = old_finding(); + no_test["proof_of_concept"]["test"] = json!(""); + for script in [ + valid.replace("\"pre_existing_count\":1", "\"pre_existing_count\":0"), + format!("{valid}\nrm \"$report\"\n"), + format!("{valid}\nprintf '%s' 'Private old vulnerability evidence.' > \"$report\"\n"), + review_script(&json!([passes_at_base]), &[]), + review_script(&json!([no_test]), &[]), + ] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &script); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let state = scenario.gh_state(); + assert!(state["advisories"].is_null()); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); + let body = state["prs"][0]["body"].as_str().unwrap(); + assert!(body.contains("Security review incomplete")); + assert!(!body.contains("Private old vulnerability evidence.")); + assert!( + !result + .stderr + .contains("Private old vulnerability evidence.") + ); + } +} + +#[test] +fn review_uses_the_published_base_when_the_local_base_is_stale() { + let scenario = Scenario::new(); + scenario.origin_has_commit( + "main", + "base-new.txt", + "base change", + "Advance the Base branch", + ); + let base = scenario.origin_git(&["rev-parse", "main"]); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + &format!( + r#" +printf '%s' "$FAKE_CLAUDE_PROMPT" > review-prompt.txt +merge_base=$(sed -n 's/^Merge base commit: `\(.*\)`\.$/\1/p' review-prompt.txt) +test "$merge_base" = "$(git rev-parse origin/main)" +{} +"#, + review_script(&json!([old_finding()]), &[]) + ), + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + assert!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .contains(base.trim()) + ); + assert_eq!(state["prs"][0]["state"], "MERGED"); +} #[test] fn command_words_override_the_config_and_review_is_off_by_default() { @@ -30,7 +294,7 @@ fn command_words_override_the_config_and_review_is_off_by_default() { let scenario = Scenario::new(); scenario.user_config_is(config); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session(2, CLEAN); + scenario.agent_does_in_session(2, &review_script(&json!([]), &[])); let url = scenario.issue_url(7); let mut args = vec![url.as_str(), "merge"]; args.extend(word); @@ -46,9 +310,9 @@ fn invalid_or_missing_final_lines_hold_self_merge_but_report_only_runs_continue( for final_message in [ "Review incomplete", "Security review: {}", - r#"Security review: {"unaddressed_count":0,"findings":["Cross-tenant read"]}"#, - r#"Security review: {"unaddressed_count":1,"findings":[""]}"#, - r#"Security review: {"unaddressed_count":0,"findings":[]}\nMore text"#, + r#"Security review: {"unaddressed_count":0,"findings":["Cross-tenant read"],"pre_existing_count":0}"#, + r#"Security review: {"unaddressed_count":1,"findings":[""],"pre_existing_count":0}"#, + r#"Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0}\nMore text"#, ] { for merge in [true, false] { let scenario = Scenario::new(); @@ -134,7 +398,7 @@ gh fake checks "$(git rev-parse origin/main)" '[{{"name":"test","conclusion":"fa "# ), ); - scenario.agent_does_for_in_session(7, 2, CLEAN); + scenario.agent_does_for_in_session(7, 2, &review_script(&json!([]), &[])); scenario.agent_does_for_in_session( 8, 1, @@ -146,7 +410,7 @@ gh pr create --base main --head issue-8 --title 'Fix base CI' --body 'Closes #8' gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}]' "#, ); - scenario.agent_does_for_in_session(8, 2, CLEAN); + scenario.agent_does_for_in_session(8, 2, &review_script(&json!([]), &[])); let result = scenario.run(&[&scenario.issue_url(7), "security-review", "base-fix"]); assert_eq!(result.code, Some(0), "{}", result.stderr); let calls = scenario.claude_calls(); @@ -155,7 +419,7 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] calls[3]["prompt"] .as_str() .unwrap() - .contains("git diff main...HEAD") + .contains("against Base branch main") ); assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); } @@ -164,7 +428,7 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] fn an_unaddressed_introduced_finding_holds_self_merge_with_the_pr_ready() { let scenario = Scenario::new(); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session(2, r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); assert_eq!(result.code, Some(1), "{}", result.stderr); assert!( @@ -190,8 +454,9 @@ fn enabled_review_fixes_reach_origin_before_delivery_finishes() { echo checked > security-fix.txt git add security-fix.txt git commit -q -m 'Fix reproduced finding' -{CLEAN} -"# +{clean} +"#, + clean = review_script(&json!([]), &[]) ), ); let result = scenario.run(&[&scenario.issue_url(7), "security-review"]); @@ -207,7 +472,10 @@ git commit -q -m 'Fix reproduced finding' let prompt = calls[1]["prompt"].as_str().unwrap(); assert!(prompt.contains("guidance mode")); assert!(prompt.contains("Do not delegate")); - assert!(prompt.contains("git diff main...HEAD")); + assert!(prompt.contains(&format!( + "git diff {}...HEAD", + scenario.launch_git(&["rev-parse", "main"]).trim() + ))); } // Regression A. @@ -224,10 +492,7 @@ gh fake checks "$(git rev-parse origin/main)" '[{"name":"test","conclusion":"suc "# ), ); - scenario.agent_does_in_session( - 2, - r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, - ); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); scenario.agent_does_in_session( 3, r#" @@ -292,10 +557,7 @@ printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINA fn review_regression_c_continuation_replaces_the_previous_security_outcome() { let scenario = Scenario::new(); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session( - 2, - r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, - ); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); let first = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); assert_eq!(first.code, Some(0), "{}", first.stderr); assert!( @@ -312,7 +574,7 @@ git add feature.txt git commit -q -m 'Fix the previously reported finding' "#, ); - scenario.agent_does_in_session(4, CLEAN); + scenario.agent_does_in_session(4, &review_script(&json!([]), &[])); let second = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); assert_eq!(second.code, Some(0), "{}", second.stderr); let state = scenario.gh_state(); From ec0eef2a221ac1787a34bc5ad9c896e18b9766b0 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 04:39:54 -0400 Subject: [PATCH 2/3] Address Standards and Spec findings for #552 --- README.md | 2 +- prompts/security-review.md | 2 +- site/prompts/index.html | 2 +- src/delivery.rs | 8 +--- src/prompt.rs | 2 +- src/security/review.rs | 50 +++++++++++++++------ src/worktree.rs | 8 ++++ tests/security_review.rs | 92 +++++++++++++++++++++++++++++++++++++- 8 files changed, 141 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 1d8c1f78..40c6a353 100644 --- a/README.md +++ b/README.md @@ -776,7 +776,7 @@ A finding's title becomes the advisory summary. Its description preserves its wr `security-review`, or `review = true` under `[security]`, enables one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. `no-security-review` overrides the setting. Both words, with or without dashes, are accepted on commands that start Runs and follow dispatched Runs and Base fixes. Setup asks about the setting; it is off by default. -The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run that is not a Merge run records the outcome and continues. An old finding, whose proof-of-concept also fails at the pinned merge base, is reported in a private file under the repository's logs (`security-reviews/`). thirdshift records it as a draft advisory, or a `security-finding`/`needs-triage` issue on a private repository, reusing existing fingerprints and preserving the Day shift's grades. Its details stay out of the PR and it does not hold Self-merge; fixing belongs to a separate Security run. Spec PR review is tracked separately in [#551](https://github.com/JacobStephens2/thirdshift/issues/551). The separate Security audit retains its full audit workflow. +The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run that is not a Merge run records the outcome and continues. An old finding, whose proof-of-concept also fails at the pinned merge base, is reported in a private file under the repository's logs (`security-reviews/`). thirdshift records it as a draft advisory, or a `security-finding`/`needs-triage` issue on a private repository, reusing existing fingerprints and preserving the Day shift's grades. New records include scored reproduction and fix-size metadata so a later Security run can take the fix when allowed. Its details stay out of the PR and it does not hold Self-merge; fixing belongs to a separate Security run. Spec PR review is tracked separately in [#551](https://github.com/JacobStephens2/thirdshift/issues/551). The separate Security audit retains its full audit workflow. ### Fencing diff --git a/prompts/security-review.md b/prompts/security-review.md index c61a9dc3..92fc97bf 100644 --- a/prompts/security-review.md +++ b/prompts/security-review.md @@ -13,7 +13,7 @@ Fix only a Security finding you can show with a failing proof-of-concept test; r Merge base commit: ``. Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here. Private report file: ``. -Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. +Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session. If refused, incomplete, or missing a prerequisite, say so and do not claim completion. After a complete review, end your final message with exactly one line of the form: diff --git a/site/prompts/index.html b/site/prompts/index.html index 32281cf4..7e31ba77 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -170,7 +170,7 @@

Security review

Merge base commit: `<merge base commit>`. Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here. Private report file: `<private report file>`. -Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. +Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session. If refused, incomplete, or missing a prerequisite, say so and do not claim completion. After a complete review, end your final message with exactly one line of the form: diff --git a/src/delivery.rs b/src/delivery.rs index 25406b40..534ad960 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -376,13 +376,7 @@ impl Outside for InWorktree<'_> { } fn security_review(&mut self, prompt: &str) -> Result { - crate::security::review::run( - self.sessions, - self.worktree.path(), - self.issue, - self.base, - prompt, - ) + crate::security::review::run(self.sessions, self.worktree, self.issue, self.base, prompt) } fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> { diff --git a/src/prompt.rs b/src/prompt.rs index 8f98f8fb..b54af21d 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -329,7 +329,7 @@ pub fn security_review(issue: &IssueUrl, base: &str, branch: &str) -> String { Merge base commit: `{SECURITY_REVIEW_MERGE_BASE}`.\n\ Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here.\n\ Private report file: `{SECURITY_REVIEW_REPORT_FILE}`.\n\ - Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.\n\ + Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.\n\ Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.\n\ If refused, incomplete, or missing a prerequisite, say so and do not claim completion.\n\ After a complete review, end your final message with exactly one line of the form:\n\ diff --git a/src/security/review.rs b/src/security/review.rs index 587ac70a..9defe469 100644 --- a/src/security/review.rs +++ b/src/security/review.rs @@ -3,25 +3,26 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -use std::{fmt, fs, path::Path}; +use std::{fmt, fs}; -use crate::git::Git; +use super::reproduction::{FixSize, Outcome as ReproductionOutcome, Reproduction, Severity}; use crate::github::{DraftAdvisory, GitHub}; use crate::harness::interpretation::SafeguardRefusal; use crate::issue::IssueUrl; use crate::session::{Purpose, Sessions}; +use crate::worktree::Worktree; use crate::{logs, progress, prompt}; /// A fresh report outside the worktree, retained even when the session or /// private recording fails. Only the introduced titles reach Delivery. pub(crate) fn run( sessions: &Sessions<'_>, - worktree: &Path, + worktree: &Worktree, issue: &IssueUrl, base: &str, text: &str, ) -> Result { - let merge_base = Git::new(worktree).run(&["merge-base", &format!("origin/{base}"), "HEAD"])?; + let merge_base = worktree.merge_base_commit(base)?; let root = logs::root(&issue.repo()).join("security-reviews"); fs::create_dir_all(&root)?; let directory = tempfile::Builder::new() @@ -40,6 +41,9 @@ pub(crate) fn run( report.display() )); let message = sessions.run_to_final_message(Purpose::Security, "security-review", &text)?; + // A session may change commits on the Issue branch, but it cannot hand + // us a replacement checkout to observe or publish findings from. + worktree.head()?; let review = Review::from_final_message(message.as_deref())?; let findings: Vec = serde_json::from_slice(&fs::read(report)?)?; if findings.len() != review.pre_existing_count { @@ -47,25 +51,29 @@ pub(crate) fn run( } // Validate the entire report before creating any record. Malformed report // diagnostics are never passed into the public outcome. - for finding in &findings { - finding.validate()?; - } + let findings = findings + .into_iter() + .map(|finding| { + let reproduction = finding.reproduction()?; + Ok((finding, reproduction)) + }) + .collect::>>()?; if !findings.is_empty() { let github = GitHub::new(); let repo = issue.repo_slug(); let mut known = github.security_records(&repo)?; - let package = super::audit::package_in(worktree); - for finding in findings { + let package = super::audit::package_in(worktree.path()); + for (finding, reproduction) in findings { let draft = DraftAdvisory { fingerprint: finding.fingerprint.clone(), summary: finding.title.clone(), - description: format!( + description: reproduction.description(&format!( "Found by thirdshift's Security review.\n\nFingerprint: `{}`\nAudited commit: `{merge_base}`\nReview issue: {}\n\n{}\n\n```json\n{}\n```\n", finding.fingerprint, issue.url, finding.description, serde_json::to_string_pretty(&finding)? - ), + )), package: package.clone(), }; // Reuse every record state and preserve the Day shift's grade. @@ -96,10 +104,12 @@ struct ProofOfConcept { head_exit_code: i32, merge_base_exit_code: i32, notes: String, + severity: String, + fix_size: String, } impl OldFinding { - fn validate(&self) -> Result<()> { + fn reproduction(&self) -> Result { let proof = &self.proof_of_concept; if [ &self.fingerprint, @@ -120,7 +130,21 @@ impl OldFinding { "Security review's private report lacks a valid old finding and failing proof-of-concept runs" ); } - Ok(()) + let severity = Severity::parse(&proof.severity) + .context("Security review's private report has an invalid reproduced severity")?; + let size = match proof.fix_size.as_str() { + "single" => FixSize::Single, + "spec" => FixSize::Spec, + _ => bail!("Security review's private report has an invalid reproduced fix size"), + }; + Ok(Reproduction { + outcome: ReproductionOutcome::Reproduced { severity, size }, + notes: format!( + "Command: {}\nHEAD exit code: {}\nMerge base exit code: {}\n\n{}", + proof.command, proof.head_exit_code, proof.merge_base_exit_code, proof.notes + ), + test: proof.test.clone(), + }) } } diff --git a/src/worktree.rs b/src/worktree.rs index 3f0dea0f..2472df2d 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -204,6 +204,14 @@ impl Worktree { .ordinary(&self.launch, |operation| operation.head()) } + /// Observe the merge base against the published Base branch only while + /// this is still the acquired checkout on the selected Issue branch. + pub fn merge_base_commit(&self, base: &str) -> Result { + self.checkout.ordinary(&self.launch, |operation| { + operation.run(&["merge-base", &format!("origin/{base}"), "HEAD"]) + }) + } + /// Fetch and sample `origin/`, then merge that commit into the /// Issue branch. A clean outcome includes the selected commit for CI /// comparison, including when the merge had nothing to do. diff --git a/tests/security_review.rs b/tests/security_review.rs index edd02298..fc7c561e 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -20,7 +20,9 @@ fn old_finding() -> serde_json::Value { "command": "cargo test bounded_input", "head_exit_code": 101, "merge_base_exit_code": 101, - "notes": "The bounded local fixture fails at both commits." + "notes": "The bounded local fixture fails at both commits. Low likelihood and low impact; one session can add a bound.", + "severity": "low", + "fix_size": "single" } }) } @@ -590,3 +592,91 @@ git commit -q -m 'Fix the previously reported finding' "{body}" ); } + +#[test] +fn review_old_finding_can_be_taken_by_next_security_run() { + for private in [false, true] { + let scenario = Scenario::new(); + let mut state = scenario.gh_state(); + state["private"] = json!(private); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([old_finding()]), &[])); + let first = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(first.code, Some(0), "{}", first.stderr); + let before = scenario.claude_calls().len(); + if !private { + let record = scenario.gh_state()["advisories"][0]["html_url"] + .as_str() + .unwrap() + .to_string(); + scenario.agent_does_in_session(3, &format!(r#" +gh label create needs-triage --description 'Needs triage' +issue=$(gh issue create --title 'Bound input' --body 'Add a bound. Private record: {record}' --label needs-triage) +printf '%s\n' "Security fix Ticket: $issue" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#)); + } + scenario.agent_does_for( + 8, + r#" +echo bounded > bounded.txt +git add bounded.txt +git commit -q -m 'Bound input' +gh pr create --base main --head issue-8 --title 'Bound input' --body 'Closes #8' +"#, + ); + let next = scenario.run(&["secure", "base", "main", "security-fix"]); + assert!( + scenario.claude_calls().len() > before, + "Security run must take the reproduced review finding when fixing is allowed; private={private}, stderr={}", + next.stderr + ); + assert_eq!(next.code, Some(0), "{private}: {}", next.stderr); + let state = scenario.gh_state(); + assert_eq!(state["prs"][1]["head"], "issue-8"); + assert_eq!(state["prs"][1]["isDraft"], false); + assert_eq!( + scenario.origin_file("issue-8", "bounded.txt").as_deref(), + Some("bounded\n") + ); + assert!( + !state["prs"][1]["body"] + .as_str() + .unwrap() + .contains("Private old vulnerability evidence.") + ); + } +} + +#[test] +fn switched_checkout_is_rejected_before_private_recording() { + for before_review in [true, false] { + let scenario = Scenario::new(); + let switch = "\ngit switch -c replacement-branch\n"; + scenario.agent_does_in_session( + 1, + &format!("{OPENS_PR}{}", if before_review { switch } else { "" }), + ); + scenario.agent_does_in_session( + 2, + &format!( + "{}{}", + review_script(&json!([old_finding()]), &[]), + if before_review { "" } else { switch } + ), + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let records = scenario.gh_state()["advisories"].clone(); + assert!( + records.is_null() || records.as_array().is_some_and(|records| records.is_empty()), + "A changed checkout must be rejected before private recording: {records}; {}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + if before_review { 1 } else { 2 }, + "A checkout replaced by the opening session must not start a Security review" + ); + } +} From 1319bbc0bb5a0f2309a526cf9d4dd1a6447fd182 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 04:44:20 -0400 Subject: [PATCH 3/3] Pin Security review to the full remote ref (#552) --- src/worktree.rs | 2 +- tests/security_review.rs | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/worktree.rs b/src/worktree.rs index 2472df2d..610face7 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -208,7 +208,7 @@ impl Worktree { /// this is still the acquired checkout on the selected Issue branch. pub fn merge_base_commit(&self, base: &str) -> Result { self.checkout.ordinary(&self.launch, |operation| { - operation.run(&["merge-base", &format!("origin/{base}"), "HEAD"]) + operation.run(&["merge-base", &format!("refs/remotes/origin/{base}"), "HEAD"]) }) } diff --git a/tests/security_review.rs b/tests/security_review.rs index fc7c561e..1fc324d0 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -680,3 +680,22 @@ fn switched_checkout_is_rejected_before_private_recording() { ); } } + +#[test] +fn review_merge_base_uses_remote_ref_when_local_tag_shadows_it() { + let scenario = Scenario::new(); + scenario.launch_git(&["tag", "origin/main"]); + scenario.origin_has_commit("main", "base-new.txt", "base change", "Advance Base branch"); + let published_base = scenario.origin_git(&["rev-parse", "refs/heads/main"]); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([]), &[])); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let calls = scenario.claude_calls(); + let prompt = calls[1]["prompt"].as_str().unwrap(); + let expected = format!("Merge base commit: `{}`.", published_base.trim()); + assert!( + prompt.contains(&expected), + "Security review used a local tag instead of published Base branch merge base; expected {expected}, got {prompt}" + ); +}