Code Review: PR #488

Commit: 4a584c9c41d3a279a161bdf5cac9551fd63b157e

Summary

Adds a release-gated security-scan workflow (.github/workflows/security-scan.yml) so the framework dog-foods the Shield pipeline, and refreshes the shipped golden-path template (golden-paths/pipelines/security.yml) to swap two token-gated/unpinned tools for free, pinned equivalents. I verified every claim in the PR body against the worktree at ci/GH-487-security-scan (HEAD 4a584c9c).

Checklist Results

• ✅ Architecture & Design: Pass — release-gated triggers correct, gate logic sound
• ✅ Code Quality: Pass
• ✅ Testing: Pass — test_site_counts.sh green
• ✅ Security: Pass — least-privilege permissions: contents: read, no pull_request_target, pinned actions
• ✅ Performance: Pass
• ✅ PR Description & Glossary: Pass — narrative Summary, full Glossary, Closes #487
• ✅ Summary Bullet Narrative: Pass
• ⛔ Technical Decisions (AgDR): Fail — no AgDR linked for the CI security-tooling selection
• ✅ Adopter Handbooks: N/A — no handbook applies to a CI-YAML-only diff

What I verified (all confirmed accurate)

1. Triggers / gate logic — Valid YAML both files. release: published + push tags: v* + workflow_dispatch; no pull_request trigger as specified. security-gate job needs: [secrets-scan, sast-shell, dep-audit-na, sbom-na] with if: always(), and fails iff secrets-scan or sast-shell result is not success — correctly ignores the always-green N/A jobs while still blocking on real secret/SAST failures.
2. Free-tools claim holds — gitleaks/gitleaks-action@v2 (no GITLEAKS_LICENSE set → OSS mode), Semgrep OSS via pip install semgrep with --config=r/bash (no auto, no SEMGREP_APP_TOKEN). The base version on origin/dev genuinely carried returntocorp/semgrep-action@v1 + SEMGREP_APP_TOKEN + trufflehog@main; all three are removed and now only appear in explanatory comments. The swaps really do remove the token requirements. Only GITHUB_TOKEN is used.
3. N/A honesty — Confirmed: zero dependency manifests in the repo (find for package.json / requirements.txt / pyproject.toml / Cargo.toml / go.mod / Gemfile → none). The dep-audit + SBOM N/A jobs are honest, not fabricated coverage.
4. Findings report accuracy — Spot-checked _lib-multi-repo-trace.sh: IFS_save="$IFS" captured at line 128 before the loop; flagged lines 143/159/170/181/189 are all the safe IFS="$IFS_save" restore before an early return. Genuine ifs-tampering false positives, no exploitable surface. No ERROR-severity or secret findings.
5. Supply-chain / pinning — All actions pinned to ≥ major (checkout@v4, gitleaks-action@v2, upload-artifact@v4, setup-node@v4, anchore/sbom-action@v0, github-script@v7). No @main/@master.
6. Counts — test_site_counts.sh PASSES (skills 59, hooks 38, roles 20; LLM payload meta-tags within 5%).
7. Scanner-introduces-risk check — permissions: contents: read (least-privilege; the self-scan grants nothing beyond read). No pull_request_target. The release-gated triggers run only on trusted refs. The scanners themselves add no risk surface.

Issues Found

⛔ AgDR Required — CI security-tooling selection

This PR makes tool-selection decisions that meet the AgDR "library choice" detection pattern, and no AgDR is linked:

• gitleaks chosen over the previously-shipped trufflehog for secrets scanning
• Semgrep OSS (pip + semgrep scan) chosen over the token-gated returntocorp/semgrep-action@v1 for SAST
• anchore/sbom-action chosen for CycloneDX SBOM generation
• The release-gated vs per-PR trigger split (self-scan release-only; adopter template per-PR) is itself an architecture call worth recording

The "why did you choose X over Y?" test applies squarely here — these are exactly the trade-offs (free vs token-gated, pinned vs unstable ref, per-PR vs release latency) that an AgDR exists to capture for the next person who touches the framework's security tooling.

Action required:

1. Run /decide to create an AgDR for the security-tooling selection (one AgDR covering the secrets/SAST/SBOM tool choices + the trigger-cadence split is fine).
2. Add the AgDR link to the PR description.

AgDR: docs/agdr/AgDR-NNNN-security-scan-tooling.md

The inline header comments and the PR-body narrative already articulate most of the rationale, so promoting that to an AgDR is low-effort — but the framework's own rule (.claude/rules/agdr-decisions.md) requires the record before Build/merge for tooling choices, and an AgDR is the durable, searchable home (/agdr search) that a PR body is not.

Suggestions (non-blocking)

• nit: The SBOM N/A job copy says "A CycloneDX SBOM would be empty" — strictly it'd be near-empty (a root component, no dependencies). Minor wording.
• nit: Consider adding inline # nosemgrep: bash.lang.security.ifs-tampering suppressions on the five restore lines in _lib-multi-repo-trace.sh in a follow-up, so the self-scan reaches a true-zero WARNING state and a future real IFS finding isn't lost in known noise. The PR already notes this as a possible follow-up — agreed it's out of scope here.

Verdict

CHANGES REQUESTED — the security review itself is clean and every technical claim in the PR body checks out; the only blocker is the missing AgDR for the CI security-tooling selection. Add the AgDR link and this is an approve.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 4a584c9c41d3a279a161bdf5cac9551fd63b157e

Code Review: PR #488 — AgDR-gap re-review

Reviewed local commit: ea4989736139f7597710a6b423d7b1625e6e23e9
PR HEAD on GitHub: 4a584c9c41d3a279a161bdf5cac9551fd63b157e

Summary

Re-review to clear the single prior blocker: missing AgDR for the security-scan tooling selection. All technical aspects passed in the prior review and are unchanged on this commit.

AgDR-gap clearance — all content checks pass

• ✅ docs/agdr/AgDR-0061-security-scan-tooling.md exists and follows the AgDR template (summary line + Context / Options Considered / Decision / Consequences / Artifacts).
• ✅ Records the actual decisions: gitleaks-over-trufflehog, Semgrep-OSS-over-paid-action/CodeQL, release-gated cadence, honest N/A for dep-audit + SBOM (no manifests), least-privilege permissions: contents: read.
• ✅ <!-- agdr: docs/agdr/AgDR-0061-security-scan-tooling.md --> marker present in the PR body.
• ✅ git diff 4a584c9..ea49897 is only the AgDR addition (1 file, +39). The workflow + golden-path files are unchanged from the prior approval-modulo-AgDR review.
• ✅ test_site_counts.sh green (exit 0) — AgDR files aren't counted, no drift.

⚠ Blocker: remediating commit is not on GitHub

The AgDR commit ea49897 exists only in the local worktree. The remote branch ci/GH-487-security-scan is still at 4a584c9 (git ls-remote confirms), so GitHub's PR HEAD is 4a584c9 — a tree that contains no AgDR.

I cannot write the approval marker until the AgDR commit is pushed:

1. git push origin ci/GH-487-security-scan (from the worktree) so GitHub HEAD becomes ea49897.
2. Re-run /code-review 488 (or this clearance) against the pushed HEAD — the marker SHA must match GitHub's HEAD for the merge gate to accept it.

Approving the current GitHub HEAD (4a584c9) would re-approve the exact un-remediated state the prior review blocked on. Once ea49897 is pushed and confirmed as HEAD, the verdict flips to APPROVED.

Verdict

APPROVED pending push — content is clean; marker withheld until the AgDR commit is the PR's HEAD on GitHub.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Content verified at commit ea4989736139f7597710a6b423d7b1625e6e23e9 (local; not yet pushed)

Code Review: PR #488 (re-review) — APPROVED

Commit: ea4989736139f7597710a6b423d7b1625e6e23e9

Note: GitHub blocks --approve on own PR; recording APPROVED verdict via comment. Rex approval marker written for the merge gate.

Summary

Release-gated security-scan workflow that dog-foods the shipped Shield pipeline, plus a refresh of the golden-paths/pipelines/security.yml adopter template to free/pinned tooling. Third pass — the only change since my clean technical review is the addition of AgDR-0061.

Re-review conditions

• ✅ GitHub PR HEAD == ea49897 (pushed)
• ✅ AgDR-0061 present, template-conformant, records the tooling decisions
• ✅ <!-- agdr: docs/agdr/AgDR-0061-security-scan-tooling.md --> marker present in PR body
• ✅ Diff since last technical review is ONLY the AgDR file (commit ea49897 touches docs/agdr/AgDR-0061-security-scan-tooling.md exclusively)

Checklist Results

• ✅ Architecture & Design: Pass
• ✅ Code Quality: Pass
• ✅ Testing: Pass (one-time clearance run documented; manual + tag triggers in Testing section)
• ✅ Security: Pass (least-privilege contents: read, no pull_request_target, free token-free tooling, pinned actions)
• ✅ Performance: Pass
• ✅ PR Description & Glossary: Pass (narrative summary + full glossary)
• ✅ Technical Decisions (AgDR):Pass — AgDR-0061 now documents gitleaks-over-trufflehog, Semgrep-OSS-over-paid/CodeQL, release-gated cadence, and honest N/A for dep-audit/SBOM

Issues Found

None. The prior blocker (missing AgDR for the tooling/cadence decisions) is resolved.

Verdict

APPROVED

The AgDR captures exactly the decisions the diff embodies (tool selection, cadence, N/A coverage), conforms to the template, and the agdr: marker links it from the PR body. No new code changes to re-review.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: ea4989736139f7597710a6b423d7b1625e6e23e9