Add opt-in guidance Security reviews to Runs

README.md

@@ -203,6 +203,7 @@ parallel = 2   # how many Tickets a Spec run runs at once, instead of 3
 limit = 5   # how many open issues labelled in-progress stop a Pickup run taking another, instead of 3
 
 [security]
+review = true   # review each Run's change for Security findings
 harness = "claude"   # the Harness for Security runs; blank or missing follows harness.default
 
 [harness]
@@ -252,6 +253,7 @@ limit = 3   # how many open issues labelled in-progress stop a Pickup run taking
 
 [security]
 fix = false   # Security runs may fix reproduced findings; default false
+review = false   # Runs review their change for Security findings; default false
 harness = ""   # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code
 
 [harness]
@@ -289,8 +291,9 @@ Every key holds its real value, so a Run reading it does exactly what it does wi
 3. With that on, every Run may start a Base fix when the Base branch's CI is red? (`base.fix`). With it off, this is not asked, and `base.fix` is written at its default, `false`.
 4. Every Run first fast-forwards your checkout of the Base branch? (`launch.pull`)
 5. Security runs may fix reproduced findings? (`security.fix`), off by default.
-6. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`).
-7. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials <path>`. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written.
+6. Runs review their change for Security findings? (`security.review`), off by default.
+7. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`).
+8. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials <path>`. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written.
 
 Pressing Enter takes the default shown, which is the file's current value, or else the setting's default, and for the address the suggested email above. `logs.dir`, `activity.quiet_skips`, `spec.parallel`, `pickup.limit` and `security.harness` are not asked about. Setup adds a missing `security.harness` at its default, blank, with a comment explaining it; a configured Harness is kept. The answers are written like everything else below: in place, keeping your comments. Ctrl-C, SIGTERM or SIGHUP during the questions or a Model/Effort check exits `1` with `interrupted` and writes nothing: neither the User config nor the Credentials. Ordinary and hidden questions stop without another line of input, and hidden input restores the saved terminal settings, including echo. With notifications off, nothing about a key is asked, and saved Credentials stay as they were, so `--email` on a single Run still works. Credentials a Run would refuse (see [Email](#email)) are refused before any question, exit `1`, and not touched. With no terminal, as from cron or `thirdshift setup </dev/null`, `setup` asks nothing and never writes the Credentials. Either way it prints the file's path on stderr and exits `0` with stdout empty. Over a User config that is already there, `setup` edits it in place: its comments and key order stay, as do the values it didn't ask about, and each key it lacks is added at its default with its comment, so afterwards the file lists every setting this version knows. One that already does, down to the commented-out `email.to` line, is left byte for byte as it was. A key added to an inline table, such as `launch = { pull = true }`, gets no comment, since TOML has no place for one there. One a Run would refuse is refused the same way, exit `1`, and not touched. Any argument after `setup` is an argument error (exit `2`).
 
@@ -769,6 +772,12 @@ Each audit keeps a new output directory under `<logs.dir>/<owner>/<repo>/audits/
 
 A finding's title becomes the advisory summary. Its description preserves its write-up, trace, evidence and validation plan, with the fingerprint and audited commit. thirdshift claims no severity, CWE or affected version range. It uses the package in the repository's manifest, or the `other` ecosystem when none is identified. Matching fingerprints in any advisory state are kept rather than recorded again. Rejected candidates stay in the local report and produce no advisory. Draft advisories require GitHub repository security manager or administrator access. On a private repository whose advisory endpoint is unavailable, the private record is an issue labelled `security-finding` and `needs-triage`. It is never replaced by a public finding issue.
 
+### 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.
+
+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.
+
 ### Fencing
 
 **Fencing** is the factory finding and fixing vulnerabilities with nobody starting it: Security runs started on a schedule with fixing allowed. thirdshift never schedules itself ([ADR-0009](docs/adr/0009-the-operating-system-schedules-thirdshift.md)). This Linux crontab entry starts one every five minutes; use your own paths, with `claude` and `gh` already logged in and Git able to push:

prompts/README.md

@@ -25,6 +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 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) |

prompts/security-review.md

@@ -0,0 +1,21 @@
+<!-- Generated by src/prompts_page.rs from the prompts in src/prompt.rs and their titles and when sentences in src/prompts_page.rs; don't edit. Regenerate with UPDATE_PROMPTS=1 cargo test prompts_page -->
+
+# 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.
+
+```
+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.
+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.
+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.
+
+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.
+```

site/prompts/index.html

@@ -155,6 +155,25 @@ <h3 id="prompt-architecture-review-title">Architecture review</h3>
 Architecture review idea: &lt;URL of the issue you filed&gt;
 Architecture review already filed: &lt;URL of the open issue that already covers it&gt;
 
+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.
+</code></pre>
+      </article>
+      <article class="job-sheet" id="prompt-security-review" aria-labelledby="prompt-security-review-title">
+        <h3 id="prompt-security-review-title">Security review</h3>
+        <p>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.</p>
+        <p class="sent-by">Sent by <a href="/#unit-3">3 · Y · Review</a></p>
+        <pre class="job-text"><code>Use the `thirdshift-security-audit` skill in guidance mode.
+Review the change for <mark class="ph">&lt;Issue URL&gt;</mark> on branch <mark class="ph">&lt;branch&gt;</mark> with `git diff <mark class="ph">&lt;base&gt;</mark>...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.
+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.
+
 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.
 </code></pre>
       </article>

src/args.rs

@@ -258,6 +258,18 @@ fn take_flag<'a>(
             arg,
             SECURITY_FIX_FLAGS,
         )?,
+        "security-review" | "--security-review" => ask_once(
+            &mut flags.security_review,
+            crate::security::review::Ask::Allow,
+            arg,
+            SECURITY_REVIEW_FLAGS,
+        )?,
+        "no-security-review" | "--no-security-review" => ask_once(
+            &mut flags.security_review,
+            crate::security::review::Ask::Forbid,
+            arg,
+            SECURITY_REVIEW_FLAGS,
+        )?,
         "harness" | "--harness" => ask_harness(&mut flags.harness.harness, arg, rest.next())?,
         "model" | "--model" => {
             let model = &mut flags.harness.model_and_effort.model;
@@ -275,6 +287,7 @@ fn take_flag<'a>(
 const MERGE_FLAGS: &str = "merge and no-merge";
 const EMAIL_FLAGS: &str = "email and no-email";
 const BASE_FIX_FLAGS: &str = "base-fix and no-base-fix";
+const SECURITY_REVIEW_FLAGS: &str = "security-review and no-security-review";
 const SECURITY_FIX_FLAGS: &str = "security-fix and no-security-fix";
 
 /// Record in `given` what the flag `arg` asked for: the same kind of ask
@@ -355,6 +368,22 @@ mod tests {
 
     const URL: &str = "https://github.com/acme/widgets/issues/7";
 
+    #[test]
+    fn security_review_words_are_accepted_on_commands_that_start_runs() {
+        for command in [URL, "pickup", "architect", "secure"] {
+            for word in [
+                "security-review",
+                "--security-review",
+                "no-security-review",
+                "--no-security-review",
+            ] {
+                assert!(parse_strs(&[command, word]).is_ok(), "{command} {word}");
+            }
+        }
+        assert!(parse_strs(&[URL, "security-review", "no-security-review"]).is_err());
+        assert!(parse_strs(&[URL, "security-review", "--security-review"]).is_err());
+    }
+
     fn parse_strs(args: &[&str]) -> Result<Command> {
         let args: Vec<String> = args.iter().map(|arg| arg.to_string()).collect();
         parse(&args)
@@ -738,6 +767,7 @@ mod tests {
             base: Some("develop".to_string()),
             flags: Flags {
                 security_fix: None,
+                security_review: None,
                 goal: Some(Goal::Merged),
                 email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))),
                 parallel: NonZeroUsize::new(2),
@@ -793,6 +823,7 @@ mod tests {
             base: None,
             flags: Flags {
                 security_fix: None,
+                security_review: None,
                 goal: Some(Goal::ReadyForReview),
                 email: Some(NotificationAsk::Skip),
                 parallel: None,