From e07a2ee69f8ab7351c633443f996c901a02d64e7 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 04:30:12 -0400 Subject: [PATCH] Run one guidance Security review on Spec PRs (#551) --- README.md | 4 +- prompts/README.md | 2 +- prompts/security-review.md | 2 +- prompts/spec-review.md | 2 +- site/prompts/index.html | 4 +- src/prompt.rs | 2 +- src/prompts_page.rs | 2 +- src/spec_run.rs | 6 +- tests/spec_run.rs | 207 +++++++++++++++++++++++++++++++++++++ 9 files changed, 218 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 59e9b8d4..d7b57195 100644 --- a/README.md +++ b/README.md @@ -774,9 +774,9 @@ A finding's title becomes the advisory summary. Its description preserves its wr ### Security review -`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. +`security-review`, or `review = true` under `[security]`, enables one guidance review after a Run's opening session or a Spec run's Spec review, before Delivery pushes and enters the Repair loop. A Spec run reviews the whole Spec branch against its Base branch once; its Tickets' Runs get none. `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, including a Spec run asked to merge its Spec PR, 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 or Spec run not asked to merge records the outcome and continues. Pre-existing vulnerability details stay out of the PR; private recording of old findings is tracked in [#552](https://github.com/JacobStephens2/thirdshift/issues/552). The separate Security audit retains its full audit workflow. ### Fencing diff --git a/prompts/README.md b/prompts/README.md index 458adf46..1df3b83c 100644 --- a/prompts/README.md +++ b/prompts/README.md @@ -25,7 +25,7 @@ With `harness codex`, each session runs `codex exec --json --dangerously-bypass- | Continuation, with an open pull request | Starts the implement session when the Run is a Continuation of an Issue branch whose pull request is open. | [continuation-pr.md](continuation-pr.md) | | Spec review | In a Spec run, starts the Spec review once every Ticket has landed on the Spec branch, before the Spec PR, a draft until then, is marked ready. | [spec-review.md](spec-review.md) | | Architecture review | Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none. | [architecture-review.md](architecture-review.md) | -| 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. | [security-review.md](security-review.md) | +| Security review | 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. | [security-review.md](security-review.md) | | Security audit | Starts the report-only Security audit in a throwaway worktree at origin's Base branch head. The threat-model line is included when a conventional document exists; artifacts stay under the repository's audit root. | [security-audit.md](security-audit.md) | | Security reproduction | After a Security audit, tries to reproduce one recorded finding in a fresh throwaway worktree at its audited commit. The test is copied into the private record only after a complete outcome. | [security-reproduction.md](security-reproduction.md) | | Conflict Repair | Starts a Repair session when merging the Base branch into the Issue branch leaves conflicts. | [conflict-repair.md](conflict-repair.md) | diff --git a/prompts/security-review.md b/prompts/security-review.md index aaf36b34..21b87686 100644 --- a/prompts/security-review.md +++ b/prompts/security-review.md @@ -2,7 +2,7 @@ # 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. ``` Use the `thirdshift-security-audit` skill in guidance mode. diff --git a/prompts/spec-review.md b/prompts/spec-review.md index baa0f94a..70df5f9f 100644 --- a/prompts/spec-review.md +++ b/prompts/spec-review.md @@ -14,7 +14,7 @@ Review with the `thirdshift-code-review` skill using as the fixed point a For each Standards or Spec finding that says behaviour is wrong, run its test or command as written before deciding the finding. If it fails as the finding says, fix the code and keep the test. If it passes, you may decline the finding, citing the run. A passing run counts only when it exercises that finding; a green suite does not. Address the Standards and Spec findings you agree with, using the `thirdshift-tdd` skill where it fits, and commit. Write the reviewers' reports, with each axis's final files-read list, to `/standards.md` (Standards) and `/spec.md` (Spec). -Push branch . Do not rebase or force-push. +Do not push: thirdshift pushes branch after the optional Security review. Do not rebase or force-push. Update PR 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. diff --git a/site/prompts/index.html b/site/prompts/index.html index 86a7690d..8395e54a 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -121,7 +121,7 @@

Spec review

For each Standards or Spec finding that says behaviour is wrong, run its test or command as written before deciding the finding. If it fails as the finding says, fix the code and keep the test. If it passes, you may decline the finding, citing the run. A passing run counts only when it exercises that finding; a green suite does not. Address the Standards and Spec findings you agree with, using the `thirdshift-tdd` skill where it fits, and commit. Write the reviewers' reports, with each axis's final files-read list, to `<review reports directory>/standards.md` (Standards) and `<review reports directory>/spec.md` (Spec). -Push branch <Spec branch>. Do not rebase or force-push. +Do not push: thirdshift pushes branch <Spec branch> after the optional Security review. Do not rebase or force-push. Update PR <pull request 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. @@ -160,7 +160,7 @@

Architecture review

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();