ci: gated per-PR security review (Trail of Bits differential-review)

[GTC6244]

Adds .github/workflows/security-review.yml, a security review that runs on every PR and gates merge: the job fails on any CRITICAL/HIGH finding at MEDIUM+ confidence, computed deterministically from a verdict JSON via jq (and fails closed if the review produces no verdict). It uses pull_request rather than pull_request_target, so fork PRs run with a read-only token and can't reach secrets. The Trail of Bits differential-review skill and its adversarial-modeler agent are vendored into .claude/ (pinned v1.1.1, attributed CC BY-SA 4.0) so the reviewed content is in-repo with no CI-time supply-chain fetch, and .gitignore is adjusted to track those paths while keeping local Claude state ignored. To turn this into a true merge blocker, mark the security-review check as required in branch protection for main.

[Copilot]

Pull request overview

Adds an automated, merge-gating security review workflow that runs on pull requests and vendors the Trail of Bits “differential-review” skill + an adversarial modeling agent into the repo for deterministic, in-repo review content.

Changes:

• Introduces a new Security Review (gated) GitHub Actions workflow that runs Claude-based differential security review and fails the check based on a jq-evaluated verdict JSON.
• Vendors the differential-review skill documentation/assets plus the adversarial-modeler agent under .claude/ for in-repo pinning/attribution.
• Updates .gitignore to ignore local Claude state while attempting to track the vendored .claude/skills/ and .claude/agents/ content.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
.gitignore	Adjusts ignore rules to (intended) track vendored .claude skills/agents while ignoring local Claude state.
.github/workflows/security-review.yml	Adds the gated, per-PR security review workflow and jq-based merge-blocking logic.
.claude/skills/differential-review/SKILL.md	Adds the core “differential-review” skill definition and overview.
.claude/skills/differential-review/reporting.md	Adds detailed reporting format guidance for the skill.
.claude/skills/differential-review/patterns.md	Adds a vulnerability pattern checklist/reference.
.claude/skills/differential-review/methodology.md	Adds the phase-by-phase differential review methodology.
.claude/skills/differential-review/ATTRIBUTION.md	Adds upstream attribution/license/version pin statement.
.claude/skills/differential-review/assets/trail-of-bits-mark.svg	Adds vendored branding asset referenced by the skill.
.claude/skills/differential-review/agents/openai.yaml	Adds UI/branding metadata for the skill’s agent interface.
.claude/skills/differential-review/adversarial.md	Adds adversarial analysis methodology used for high-risk changes.
.claude/agents/adversarial-modeler.md	Adds the adversarial modeling agent definition used by the skill.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[GTC6244]

Good catch — fixed in 2d8297c. You're right that pull_request withholds secrets from forked-repo PRs, so the token is empty and the review cannot run there, and the header comment claiming the gate "still works" on forks was wrong.

Changes:

• The review step now has if: github.event.pull_request.head.repo.full_name == github.repository, so it is skipped (not failed) on fork PRs.
• The gate step detects the fork case (IS_FORK) up front and exits 0 with a ::notice + job-summary explanation, so fork PRs are not blocked. A maintainer can re-run the review from a same-repo branch to get a gated review.
• Corrected the header security note to describe this accurately.

[GTC6244]

Agreed — fixed in 2d8297c. jq -e . only checked JSON validity, so {} passed and a missing .findings was silently read as "0 findings" (false pass). The gate now fails closed unless the verdict is an object with a findings array and every entry has a recognized severity (CRITICAL|HIGH|MEDIUM|LOW|INFO) and confidence (HIGH|MEDIUM|LOW) enum:

(type == "object")
and ((.findings | type) == "array")
and (all(.findings[]?;
      (.severity   as $s | ["CRITICAL","HIGH","MEDIUM","LOW","INFO"] | index($s))
  and (.confidence as $c | ["HIGH","MEDIUM","LOW"] | index($c))))

This also closes a subtler hole you hinted at: a finding with a bogus/missing severity would have evaded the blocking filter, so the whole verdict is now rejected in that case. A legitimately empty findings: [] (clean review) still passes. Verified across 9 good/bad cases locally.

[GTC6244]

Thanks for the review @copilot-pull-request-reviewer. Both inline comments were valid and are addressed in 2d8297c:

1. Fork PRs — the review step now skips on forked-repo PRs (no secrets available) and the gate passes with a notice instead of failing closed, so fork PRs are not blocked. Header comment corrected.
2. Verdict schema — the gate now fails closed unless the verdict is an object with a findings[] array whose every entry has a valid severity + confidence enum, so {} / missing-findings no longer produce a false pass.

Note: the security-review check itself is expected to report "skipped (not gating yet)" on this PR, because GitHub's claude-code-action workflow validation blocks the action from running until the workflow file is on the default branch — it will gate normally on subsequent PRs once merged.

[Copilot]

@GTC6244 I'm unable to start working on this because of repository rules that prevent me from pushing to the branch:

• Changes must be made through a pull request due to repository rules

See the documentation for more details.

[clawdbot-glitch003]

🤖 Code review (Claude)

This PR adds .github/workflows/security-review.yml, a per-PR merge gate that runs the Trail of Bits differential-review skill via anthropics/claude-code-action@v1, has the model emit a machine-readable verdict.json, and fails the check via deterministic jq filtering on CRITICAL/HIGH findings at MEDIUM+ confidence. The skill and its adversarial-modeler agent are vendored under .claude/ (pinned v1.1.1, attributed), and .gitignore is adjusted to track them. The fundamentals are right: pull_request (not pull_request_target) so fork PRs never see secrets, no untrusted PR title/body interpolated into the prompt (only PR number, repo, and SHAs), minimal permissions block, fail-closed on missing/invalid/schema-violating verdicts, and gate policy computed in CI rather than trusted from model prose. The findings below are about holes in the fail-closed story.

Findings

1. Pre-seeded verdict.json in the PR defeats fail-closed (HIGH)

.github/workflows/security-review.yml:175-183 — the gate reads $VERDICT_FILE (.security-review/verdict.json) from the working tree, which is the PR head checkout. Nothing removes a committed .security-review/verdict.json before the review step. So a PR that commits a benign verdict ({"findings":[],...}) converts every failure mode of the review step — action error, timeout, model not writing the file — into a silent pass: the gate finds a valid, schema-conformant file and exits 0. Since the review step already has a 30-minute timeout and can legitimately fail, this is a realistic path, and it also means a stale verdict accidentally committed by a developer running the flow locally would satisfy the gate.

Fix (two lines): add a rm -rf .security-review step before "Run differential security review" (or fail the gate if git ls-files --error-unmatch "$VERDICT_FILE" says the file is tracked), and add .security-review/ to .gitignore so local runs never commit it.

2. Workflow-file "not gating yet" check compares the working tree, not HEAD (MEDIUM)

.github/workflows/security-review.yml:158-173 — the skip-path check does diff -q <(git show "FETCH_HEAD:$WORKFLOW_FILE") "$WORKFLOW_FILE", i.e. it compares against the working-tree copy of the workflow. The review step runs before the gate with Write,Edit,Bash allowed, so anything that steers the model (prompt injection embedded in the PR's diff content is the obvious vector) into touching .github/workflows/security-review.yml in the working tree flips the gate into the "skipped (not gating yet) … exit 0" path. Compare the committed copy instead: git show "HEAD:$WORKFLOW_FILE". That matches what claude-code-action actually validates and removes the writable-tree escape hatch.

(Related, worth stating in the file's security note: since the same steerable model writes verdict.json, a sufficiently adversarial same-repo PR can always talk the reviewer into an empty verdict. That's inherent to any LLM gate and same-repo authors are trusted, so it's an accepted risk — but the working-tree comparison above is a mechanical bypass and should still be closed.)

3. FETCH_HEAD fallback on failed default-branch fetch is silently wrong (LOW)

.github/workflows/security-review.yml:156 — git fetch --no-tags --depth=1 origin "$DEFAULT_BRANCH" 2>/dev/null || true. If that fetch fails (transient network, etc.), FETCH_HEAD still points at the base SHA from the earlier "Fetch base ref" step, so the workflow-file comparison runs against the wrong ref without any indication. Depending on drift between base and default branch, that can either wrongly skip the gate or wrongly enforce it. Since fetch-depth: 0 already gives you full history, drop the || true and fail loudly, or use origin/$DEFAULT_BRANCH and verify the ref exists before comparing.

Nits

• .gitignore:5-7 — !.claude/skills/ and !.claude/agents/ un-ignore all of every developer's local skills/agents, not just the vendored ones; personal local agents can now be committed by an inattentive git add -A. Scoping to !.claude/skills/differential-review/ and !.claude/agents/adversarial-modeler.md is safer.
• .github/workflows/security-review.yml:234-236 — model-authored title/file strings go straight into the step-summary markdown table; a | in a title breaks the table. Same for report.md inside the ``` fence (a fenced block in the report escapes it). Cosmetic — step summary only, no execution risk.
• The prompt's JSON schema requires a counts object but the gate never validates or uses it; either drop it from the schema or cross-check it against findings (drift between the two is a useful signal the model got confused).
• docs: fork PRs pass unreviewed by design (documented in the file header — good). Once this check is marked required, remember that means fork PRs get zero automated security review, so human review must cover them.

Verdict

Needs changes — the design is sound and the injection/secrets/permissions hygiene is genuinely good, but finding 1 is a real fail-open hole with a two-line fix (rm -rf .security-review pre-step + gitignore entry), and finding 2 is a one-token fix (HEAD: instead of the working-tree path). With those addressed this is good to merge.

[GTC6244]

Response to Claude code review

Thanks — genuinely good findings. All three + the nits are addressed in db8fe00.

Findings

1. Pre-seeded verdict.json defeats fail-closed (HIGH) — fixed.
Added a Clear any pre-committed verdict (prevent fail-open) step that runs before the review: it fails the job if .security-review/ is tracked (git ls-files) and rm -rf .security-review otherwise, so the gate can only ever read this run's freshly-generated output. Also added .security-review/ to .gitignore so local runs can't commit one. This closes the "review times out → committed benign verdict → silent pass" path.

2. Skip-check compared the working tree, not HEAD (MEDIUM) — fixed.
The comparison is now diff -q <(git show "FETCH_HEAD:$WORKFLOW_FILE") <(git show "HEAD:$WORKFLOW_FILE") — both git objects, so the review step's Write/Edit access can no longer mutate the on-disk workflow to force the "skipped (not gating yet)" path. This also matches what claude-code-action actually validates (committed content). Your related note — that a same-repo author can always talk the LLM into an empty verdict — is an accepted risk (same-repo authors are trusted); the mechanical working-tree bypass is what mattered and it's closed.

3. FETCH_HEAD fallback silently wrong on failed fetch (LOW) — fixed.
Dropped || true; the fetch now fails the job loudly ("Could not fetch default branch … Failing closed") instead of comparing against the stale base-ref FETCH_HEAD.

Nits

• gitignore scope — fixed. Negations are now scoped to exactly !.claude/skills/differential-review/ and !.claude/agents/adversarial-modeler.md (with .claude/skills/* / .claude/agents/* re-ignored), so a stray git add -A can't commit personal local skills/agents. Verified with git check-ignore.
• Table pipe injection — fixed. Model-authored cells are now gsub-escaped (| → \|) before going into the step-summary table.
• Unused counts — fixed. Dropped counts from the prompt schema (it was never validated or used).
• Fork PRs get zero review once required — documented. Added a CAUTION line to the workflow header noting human review must cover forked-repo contributions.

Not changed

• The report.md fenced-block escaping nit: left as-is. It's step-summary-only, no execution risk, and a full markdown-fence sanitizer for arbitrary report content is more complexity than the cosmetic payoff warrants. Happy to add ````-fence-length probing if you feel strongly.
• The inherent "LLM can be argued into a clean verdict by a trusted same-repo author" risk is accepted, per finding 2.

Verified: YAML parses, gitignore scoping, schema validation (9 cases), pipe-escaping, and the fetch/HEAD comparison logic all tested locally before pushing.