Record old Security review findings privately

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 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, 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.
+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. 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. The separate Security audit retains its full audit workflow.
 
 ### Fencing
 

prompts/security-review.md

@@ -6,16 +6,19 @@ When enabled, runs one guidance review after a Run's opening session or a Spec r
 
 ```
 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 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:
-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.
 ```

site/prompts/index.html

@@ -163,16 +163,19 @@ <h3 id="prompt-security-review-title">Security review</h3>
         <p>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.</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.
+Review the change for <mark class="ph">&lt;Issue URL&gt;</mark> on branch <mark class="ph">&lt;branch&gt;</mark> against Base branch <mark class="ph">&lt;base&gt;</mark> with `git diff <mark class="ph">&lt;merge base commit&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.
+Merge base commit: `<mark class="ph">&lt;merge base commit&gt;</mark>`.
+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: `<mark class="ph">&lt;private report file&gt;</mark>`.
+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:
-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.
 </code></pre>

src/delivery.rs

@@ -376,12 +376,7 @@ impl Outside for InWorktree<'_> {
     }
 
     fn security_review(&mut self, prompt: &str) -> Result<Review> {
-        let message = self.sessions.run_to_final_message(
-            crate::session::Purpose::Security,
-            "security-review",
-            prompt,
-        )?;
-        Review::from_final_message(message.as_deref())
+        crate::security::review::run(self.sessions, self.worktree, self.issue, self.base, prompt)
     }
 
     fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> {
@@ -634,7 +629,7 @@ mod tests {
         }
         fn security_review(&mut self, _: &str) -> Result<Review> {
             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<()> {

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 = "<private report file>";
+pub const SECURITY_REVIEW_MERGE_BASE: &str = "<merge base commit>";
+
 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 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\
-         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,
     )

src/prompts_page.rs

@@ -60,7 +60,7 @@ const SESSION_ID: &str = "<session id>";
 const PROMPT: &str = "<prompt>";
 const MODEL: &str = "<model>";
 const EFFORT: &str = "<effort>";
-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.

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"),