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.
+When enabled, runs one guidance review after a Run's opening session or a Spec run's Spec review, before Delivery pushes and enters the Repair loop. Ticket Runs get none; Base fixes get one too. Introduced findings, refusals 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.
diff --git a/src/prompt.rs b/src/prompt.rs
index 1383f7a9..81c78823 100644
--- a/src/prompt.rs
+++ b/src/prompt.rs
@@ -145,7 +145,7 @@ pub fn spec_review(spec: &IssueUrl, base: &str, branch: &str, pr_url: &str) -> S
\n\
{address_findings}\n\
\n\
- Push branch {branch}. Do not rebase or force-push.\n\
+ Do not push: thirdshift pushes branch {branch} after the optional Security review. Do not rebase or force-push.\n\
\n\
Update PR {pr_url} using the `thirdshift-pr` skill, rewriting its body to cover the whole Spec. Leave out its Tickets checklist, or keep it between its markers as it is: thirdshift puts it back. Leave the PR a draft: thirdshift marks it ready once you are done.\n\
\n\
diff --git a/src/prompts_page.rs b/src/prompts_page.rs
index 468fe5ab..3c21e202 100644
--- a/src/prompts_page.rs
+++ b/src/prompts_page.rs
@@ -167,7 +167,7 @@ fn prompts() -> Vec {
Prompt {
id: "prompt-security-review",
title: "Security review",
- when: "When enabled, runs one guidance review after a Run's opening session and before Delivery pushes and enters the Repair loop. Base fixes get one too; introduced findings or incomplete reviews hold Self-merge.",
+ when: "When enabled, runs one guidance review after a Run's opening session or a Spec run's Spec review, before Delivery pushes and enters the Repair loop. Ticket Runs get none; Base fixes get one too. Introduced findings, refusals or incomplete reviews hold Self-merge.",
sender: Units(&[REVIEW]),
text: prompt::security_review(&issue, BASE, BRANCH),
},
diff --git a/src/spec_run.rs b/src/spec_run.rs
index 97c7fcd6..f71db799 100644
--- a/src/spec_run.rs
+++ b/src/spec_run.rs
@@ -1,6 +1,6 @@
//! A Spec run: each of a Spec's Tickets, in dependency order and several at
//! once, taken by a Ticket's Run, a Merge run into the Spec branch in a child `thirdshift`
-//! (ADR-0006), then the Spec review and the Spec PR from the Spec branch into
+//! (ADR-0006), then the Spec review, optional Security review and the Spec PR from the Spec branch into
//! the Base branch, kept mergeable and green like a Run's PR, and Self-merged
//! when the Spec run was asked to merge.
//!
@@ -204,10 +204,8 @@ struct ChildRunsAndGitHub<'a> {
impl<'a> ChildRunsAndGitHub<'a> {
/// The outside world of a Spec run, as the struct says, with no Ticket's
/// Run started yet.
- fn new(worktree: Worktree, mut delivery: Delivery<'a>, spec_pr: PullRequest) -> Self {
+ fn new(worktree: Worktree, delivery: Delivery<'a>, spec_pr: PullRequest) -> Self {
let security_review = delivery.security_review;
- // The Spec PR review is implemented by #551; Base fixes inherit the choice.
- delivery.security_review = false;
Self {
spec: delivery.issue,
children: Runs::default(),
diff --git a/tests/spec_run.rs b/tests/spec_run.rs
index 4775deeb..35f88394 100644
--- a/tests/spec_run.rs
+++ b/tests/spec_run.rs
@@ -151,6 +151,213 @@ fn a_linear_spec_lands_each_ticket_in_order_then_opens_a_ready_spec_pr() {
scenario.assert_cleaned_up("issue-20");
}
+#[test]
+fn security_review_runs_once_after_spec_review_over_the_whole_spec_branch() {
+ let scenario = linear_spec();
+ scenario.agent_does_for_in_session(
+ SPEC,
+ 1,
+ r#"
+echo reviewed > spec-reviewed.txt
+git add spec-reviewed.txt
+git commit -q -m 'Complete Spec review'
+"#,
+ );
+ scenario.agent_does_for_in_session(
+ SPEC,
+ 2,
+ r#"
+test -f first.txt
+test -f second.txt
+test -f spec-reviewed.txt
+gh pr view issue-20 --json isDraft | grep -q '"isDraft": true'
+! git cat-file -e origin/issue-20:spec-reviewed.txt
+git diff main...HEAD --name-only > changed.txt
+grep -qx first.txt changed.txt
+grep -qx second.txt changed.txt
+grep -qx spec-reviewed.txt changed.txt
+rm changed.txt
+echo fixed > security-fixed.txt
+git add security-fixed.txt
+git commit -q -m 'Fix reproduced Security finding'
+printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE"
+"#,
+ );
+
+ let result = scenario.run(&[&spec_url(&scenario), "security-review", "merge"]);
+
+ assert_eq!(result.code, Some(0), "{}", result.stderr);
+ assert_eq!(sessions_by_issue(&scenario), ["21", "22", "20", "20"]);
+ let calls = scenario.claude_calls();
+ assert_contains(
+ calls[2]["prompt"].as_str().unwrap(),
+ "/thirdshift-code-review main",
+ );
+ let prompt = calls[3]["prompt"].as_str().unwrap();
+ assert_contains(prompt, "guidance mode");
+ assert_contains(prompt, "Do not delegate auditors");
+ assert_contains(prompt, "https://github.com/acme/widgets/issues/20");
+ assert_contains(prompt, "branch issue-20 with `git diff main...HEAD`");
+ assert_eq!(calls[3]["branch"], "issue-20");
+ assert_eq!(spec_pr(&scenario)["state"], "MERGED");
+ for file in [
+ "first.txt",
+ "second.txt",
+ "spec-reviewed.txt",
+ "security-fixed.txt",
+ ] {
+ assert!(
+ scenario.origin_file("main", file).is_some(),
+ "missing {file}"
+ );
+ }
+ let reviewed = result.stderr.find("spec-review: session ended").unwrap();
+ let security = result
+ .stderr
+ .find("security-review: session started")
+ .unwrap();
+ assert!(reviewed < security, "{}", result.stderr);
+}
+
+#[test]
+fn spec_review_leaves_the_push_to_delivery_after_the_optional_security_review() {
+ let scenario = linear_spec();
+ let result = scenario.run(&[&spec_url(&scenario)]);
+ assert_eq!(result.code, Some(0), "{}", result.stderr);
+ let call = spec_review_call(&scenario);
+ assert_contains(
+ call["prompt"].as_str().unwrap(),
+ "Do not push: thirdshift pushes branch issue-20 after the optional Security review.",
+ );
+}
+
+#[test]
+fn security_review_on_a_spec_obeys_config_and_command_overrides() {
+ for (config, word, enabled) in [
+ ("", None, false),
+ ("[security]\nreview = true\n", None, true),
+ (
+ "[security]\nreview = true\n",
+ Some("no-security-review"),
+ false,
+ ),
+ (
+ "[security]\nreview = false\n",
+ Some("security-review"),
+ true,
+ ),
+ ] {
+ let scenario = linear_spec();
+ scenario.user_config_is(config);
+ scenario.agent_does_for_in_session(SPEC, 2, r#"printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#);
+ let url = spec_url(&scenario);
+ let mut args = vec![url.as_str(), "merge"];
+ args.extend(word);
+
+ let result = scenario.run(&args);
+
+ assert_eq!(result.code, Some(0), "{config} {word:?}: {}", result.stderr);
+ assert_eq!(
+ sessions_by_issue(&scenario),
+ if enabled {
+ vec!["21", "22", "20", "20"]
+ } else {
+ vec!["21", "22", "20"]
+ }
+ );
+ assert_eq!(spec_pr(&scenario)["state"], "MERGED");
+ }
+}
+
+#[test]
+fn security_findings_refusals_and_incomplete_reviews_hold_only_the_spec_self_merge() {
+ for (script, cause) in [
+ (
+ r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#,
+ "Security review left 1 unaddressed introduced finding(s): Cross-tenant read",
+ ),
+ (
+ r#"printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINAL_MESSAGE""#,
+ "Security review refused: Claude Code's [cyber] safeguard refusal",
+ ),
+ (
+ r#"printf '%s\n' 'Review incomplete' > "$FAKE_CLAUDE_FINAL_MESSAGE""#,
+ "Security review incomplete",
+ ),
+ ("exit 1\n", "Security review incomplete"),
+ ] {
+ for merge in [true, false] {
+ let scenario = linear_spec();
+ scenario.agent_does_for_in_session(SPEC, 2, script);
+
+ let result = scenario.run(&[
+ &spec_url(&scenario),
+ "security-review",
+ if merge { "merge" } else { "no-merge" },
+ ]);
+
+ assert_eq!(
+ result.code,
+ Some(i32::from(merge)),
+ "{cause}: {}",
+ result.stderr
+ );
+ assert_contains(&result.stderr, cause);
+ assert_eq!(sessions_by_issue(&scenario), ["21", "22", "20", "20"]);
+ let pr = spec_pr(&scenario);
+ assert_eq!(pr["state"], "OPEN");
+ assert_eq!(pr["isDraft"], false);
+ let body = pr["body"].as_str().unwrap();
+ assert_contains(body, "### Security");
+ // Introduced finding titles and sanitized refusal/incomplete causes.
+ assert_contains(
+ body,
+ if cause.contains("Cross-tenant read") {
+ "Cross-tenant read"
+ } else {
+ cause
+ },
+ );
+ assert!(!body.contains("Private refusal evidence"));
+ assert_contains(body, "#21 landed");
+ assert_contains(body, "#22 landed");
+ assert_eq!(scenario.gh_state()["issues"]["20"], "OPEN");
+ assert_eq!(scenario.origin_file("main", "first.txt"), None);
+ for ticket in ["issue-21", "issue-22"] {
+ assert_eq!(pr_from(&scenario, ticket).unwrap()["state"], "MERGED");
+ }
+ }
+ }
+}
+
+#[test]
+fn a_spec_security_hold_still_repairs_ci_without_repeating_the_review() {
+ let scenario = linear_spec();
+ scenario.agent_does_for_in_session(SPEC, 1, &checks_on_head(RED));
+ scenario.agent_does_for_in_session(SPEC, 2, r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#);
+ scenario.agent_does_for_in_session(
+ SPEC,
+ 3,
+ &format!("{}{}", commits_fix(1), checks_on_head(GREEN)),
+ );
+
+ let result = scenario.run(&[&spec_url(&scenario), "security-review", "merge"]);
+
+ assert_eq!(result.code, Some(1), "{}", result.stderr);
+ assert_contains(&result.stderr, "Cross-tenant read");
+ let prompts = spec_prompts(&scenario);
+ assert_eq!(prompts.len(), 3);
+ assert_contains(&prompts[0], "/thirdshift-code-review main");
+ assert_contains(&prompts[1], "guidance mode");
+ assert_contains(&prompts[2], "CI");
+ assert_eq!(
+ scenario.origin_file("issue-20", "fix-1.txt").as_deref(),
+ Some("fix\n")
+ );
+ assert_eq!(spec_pr(&scenario)["state"], "OPEN");
+ assert_eq!(spec_pr(&scenario)["isDraft"], false);
+}
+
#[test]
fn a_spec_run_claims_the_spec_and_its_tickets_runs_change_no_label() {
let scenario = linear_spec();