From ac0dc485c48dbe0b6cf28fe3cb23c01af13eea09 Mon Sep 17 00:00:00 2001
From: A <258483684+la14-1@users.noreply.github.com>
Date: Tue, 17 Feb 2026 20:08:20 -0800
Subject: [PATCH] fix: prevent contradictory security PR reviews via dedup gate
 (#1427)

The security reviewer agent could post contradictory reviews (CHANGES_REQUESTED
then APPROVED) on the same commit because each review_all cycle spawned a fresh
reviewer with no memory of prior runs. Reviews posted via `gh pr review` don't
appear in `gh pr view --comments`, so the existing comment-based dedup missed them.

Adds a review dedup step that fetches existing reviews via the GitHub API and
stops if a prior security review exists on the current HEAD commit. Also adds
commit SHA to the review body format for traceability.

Co-authored-by: lab <6723574+louisgv@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 .claude/skills/setup-agent-team/security.sh | 29 +++++++++++++--------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/.claude/skills/setup-agent-team/security.sh b/.claude/skills/setup-agent-team/security.sh
index 5e7ce276..539932e1 100644
--- a/.claude/skills/setup-agent-team/security.sh
+++ b/.claude/skills/setup-agent-team/security.sh
@@ -358,40 +358,47 @@ Each pr-reviewer MUST:
 
 1. **Fetch full context**:
    \`\`\`bash
-   gh pr view NUMBER --repo OpenRouterTeam/spawn --json updatedAt,mergeable,title,headRefName
+   gh pr view NUMBER --repo OpenRouterTeam/spawn --json updatedAt,mergeable,title,headRefName,headRefOid
    gh pr diff NUMBER --repo OpenRouterTeam/spawn
    gh pr view NUMBER --repo OpenRouterTeam/spawn --comments
    gh api repos/OpenRouterTeam/spawn/pulls/NUMBER/comments --jq '.[] | "\(.user.login): \(.body)"'
+   gh api repos/OpenRouterTeam/spawn/pulls/NUMBER/reviews --jq '.[] | {state: .state, submitted_at: .submitted_at, commit_id: .commit_id, user: .user.login, bodySnippet: (.body[:200])}'
    \`\`\`
-   Read ALL comments — prior discussion contains decisions, rejected approaches, and scope changes.
+   Read ALL comments AND reviews — prior discussion contains decisions, rejected approaches, and scope changes. Reviews (approve/request-changes) are separate from comments and must be checked independently.
 
-2. **Comment-based triage** — Close if comments indicate superseded/duplicate/abandoned:
+2. **Review dedup** — If ANY prior review from \`louisgv\` OR containing \`-- security/pr-reviewer\` already exists:
+   - If prior review is **CHANGES_REQUESTED** → Do NOT post a new review. Report "already flagged by prior security review, skipping" and STOP.
+   - If prior review is **APPROVED** and PR is not yet merged → The prior approval stands. Do NOT post another review. Report "already approved, skipping" and STOP.
+   - Only proceed if there are **NEW COMMITS** pushed after the latest security review (compare the review's \`commit_id\` with the PR's current HEAD \`headRefOid\`). If the commit SHAs match, STOP — no new code to review.
+
+3. **Comment-based triage** — Close if comments indicate superseded/duplicate/abandoned:
    \`gh pr close NUMBER --repo OpenRouterTeam/spawn --delete-branch --comment "Closing: [reason].\n\n-- security/pr-reviewer"\`
    Report and STOP.
 
-3. **Staleness check** — If \`updatedAt\` > 48h AND \`mergeable\` is CONFLICTING:
+4. **Staleness check** — If \`updatedAt\` > 48h AND \`mergeable\` is CONFLICTING:
    - If PR contains valid work: file follow-up issue, then close PR referencing the new issue
    - If trivial/outdated: close without follow-up
    - Delete branch via \`--delete-branch\`. Report and STOP.
    - If > 48h but no conflicts: proceed to review. If fresh: proceed normally.
 
-4. **Set up worktree**: \`git worktree add WORKTREE_BASE_PLACEHOLDER/pr-NUMBER -b review-pr-NUMBER origin/main\` → \`cd\` → \`gh pr checkout NUMBER\`
+5. **Set up worktree**: \`git worktree add WORKTREE_BASE_PLACEHOLDER/pr-NUMBER -b review-pr-NUMBER origin/main\` → \`cd\` → \`gh pr checkout NUMBER\`
 
-5. **Security review** of every changed file:
+6. **Security review** of every changed file:
    - Command injection, credential leaks, path traversal, XSS/injection, unsafe eval/source, curl|bash safety, macOS bash 3.x compat
 
-6. **Test** (in worktree): \`bash -n\` on .sh files, \`bun test\` for .ts files, verify source fallback pattern
+7. **Test** (in worktree): \`bash -n\` on .sh files, \`bun test\` for .ts files, verify source fallback pattern
 
-7. **Decision**:
+8. **Decision** — Before posting any review, verify it applies to the **current HEAD commit**:
    - CRITICAL/HIGH found → \`gh pr review NUMBER --request-changes\` + label \`security-review-required\`
    - MEDIUM/LOW or clean → \`gh pr review NUMBER --approve\` + label \`security-approved\` + \`gh pr merge NUMBER --repo OpenRouterTeam/spawn --squash --delete-branch\`
 
-8. **Clean up**: \`cd REPO_ROOT_PLACEHOLDER && git worktree remove WORKTREE_BASE_PLACEHOLDER/pr-NUMBER --force\`
+9. **Clean up**: \`cd REPO_ROOT_PLACEHOLDER && git worktree remove WORKTREE_BASE_PLACEHOLDER/pr-NUMBER --force\`
 
-9. **Review body format**:
+10. **Review body format** — MUST include the HEAD commit SHA for traceability:
    \`\`\`
    ## Security Review
    **Verdict**: [APPROVED / CHANGES REQUESTED]
+   **Commit**: [HEAD_COMMIT_SHA]
    ### Findings
    - [SEVERITY] file:line — description
    ### Tests
@@ -400,7 +407,7 @@ Each pr-reviewer MUST:
    *-- security/pr-reviewer*
    \`\`\`
 
-10. Report: PR number, verdict, finding count, merge status.
+11. Report: PR number, verdict, finding count, merge status.
 
 ## Step 3 — Branch Cleanup