Repository navigation
ci: run the ai-scan model session with a read-only token - #580
Conversation
Split ai-scan.yml into gate/scan/label, the same author/publish shape claude-repro.yml already uses: the gate charges the daily budget marker with issues:write, the Claude session runs in a job whose grants are contents/issues/pull-requests read, and a separate fail-closed labeler holds the write scopes. Only the coarse verdict enum crosses a job boundary. The execution file stays on the scan runner — publishing it as an artifact would hand out the scanner's reasoning, which is the evasion oracle show_full_output: false exists to prevent — so parse-scan-verdict.mjs runs in the scan job and its output feeds the labeler's unchanged enum. The session's Bash allowlist is now defense in depth rather than the one control between an injected session and repository write. Fail directions are preserved: an empty verdict output (crash, cancellation, parser never ran) hits the labeler's flagged/other default, and a gate error still leaves the item unscanned and unlabeled. One deliberate delta: a cancelled scan now flags, where cancellation used to kill the labeler too. Fixes #455.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe AI scan workflow now uses separate ChangesAI scan security split
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The workflow now runs the scan session with read-only permissions while keeping write operations in separate jobs. No actionable merge-blocking risk remains, so the PR is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GitHubEvent
participant gate
participant scan
participant label
participant GitHubIssues
GitHubEvent->>gate: trigger workflow
gate->>GitHubIssues: charge daily budget marker
gate->>scan: provide run, is_pr, and number
scan->>scan: run Claude with read-only token scope
scan->>label: provide coarse verdict
label->>GitHubIssues: apply needs-security-review
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description provides a detailed summary, links issue Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://4413b950.bestax.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
The security-critical permission and failure-path changes warrant final human review despite no identified blocking defect.
Pull request overview
Splits AI security scanning into least-privilege jobs so the model session has read-only repository access.
Changes:
- Separates budget gating, scanning, and labeling.
- Preserves fail-closed verdict handling.
- Updates security documentation to reflect token boundaries.
File summaries
| File | Description |
|---|---|
.github/workflows/ai-scan.yml |
Implements the three-job permission split. |
.github/CLAUDE.md |
Documents the revised security boundary. |
SECURITY.md |
Updates the repository security posture. |
docs/docs/guides/getting-started/ai-development.md |
Updates user-facing AI workflow documentation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review of the split found three real defects and several overstated claims. The defects: Re-running a failed `scan` bypassed the kill switches. AI_LOOP_ENABLED and AI_SCAN_MODE lived only on `gate`, and "Re-run failed jobs" does not re-run an already-green gate — it replays its cached run=true — so a maintainer who set AI_SCAN_MODE=off could still have a billed session start on re-run. Rule 3 wants the opt-in on the job that spends usage, so both variables are repeated on `scan`. `label` could flag an item nothing ever scanned. Its condition checked only `needs.gate.outputs.run`, so a gate that wrote run=true and then failed would skip `scan` while `label` ran against an empty verdict and applied needs-security-review — fail-closed on a path rule 4 mandates fail-open. It now also requires the gate to have succeeded and `scan` not to have been skipped; a scan that failed or was cancelled still flags. harden-runner ahead of the labeling call could lose a flag. If the action fails to download, the `gh api` labels call never runs and a genuinely flagged item goes unlabeled — fail-open on the verdict side, which the pre-split design could not do. It is continue-on-error now; the policy is not enforced today regardless (#487). The claims: the header asserted that a cancelled scan now flags "where cancellation used to kill the labeler too". That is backwards — the old labeler step also carried always() — and calling it a deliberate delta implied a run-state matrix that had not been enumerated, which is how the `label` gap above went unexamined. Corrected to describe the behavior without claiming a change. `.github/CLAUDE.md`'s rule 2 row said the ai-scan allowlist now holds back "nothing", which would license widening it: the job still runs Bash beside CLAUDE_CODE_OAUTH_TOKEN, so by I1 the allowlist is still the only control on that credential. The same overstatement is scoped down in the workflow and in SECURITY.md, whose "a separate job it cannot reach" ignored that the session's own verdict is the label job's only input. Also: drop the unused pull-requests:write from `label` (the labels call is the issues endpoint), restore the repo-uniform egress endpoint list on the two new jobs, refresh parse-scan-verdict.mjs's header and its test's for the new topology, and record the budget-charge and check-run visibility consequences the split introduced.
Preview DeploymentPreview URL: https://47e8f0c4.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Two security documentation claims conflict with the implemented credential boundaries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| output, never a public comment). The scan holds no write tools, no PAT, and no write-scoped | ||
| token — the labeling happens in a separate job, holding its own credentials, that the session | ||
| never runs in — so it has no channel to post or leak anything; it fails **closed** (a crashed or inconclusive scan flags rather than | ||
| passes). It is **opt-in**: it runs only when `AI_SCAN_MODE` is `on` (or `y`). Every other |
| # defense in depth rather than the ONE control standing between an | ||
| # injected session and repository write. It is still | ||
| # security-critical (rule 2) — keep it narrow. | ||
| # label — issues + pull-requests: write. Applies needs-security-review from |
The header's job topology still described `label` as holding issues + pull-requests write after the scope was cut to issues only, and the user-facing docs claimed the session 'has no channel to post or leak anything'. The leak half does not hold: the scan job still carries CLAUDE_CODE_OAUTH_TOKEN beside Bash and egress is not enforced (#487), which the same PR says plainly two files over. Both now claim only what the credential split delivers — no write to this repository.
There was a problem hiding this comment.
🔵 Needs a closer look
The security-critical workflow permission and failure-path changes warrant final human review despite no identified blocking defect.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Preview DeploymentPreview URL: https://4cc9666a.bestax.pages.dev |
|
Both of Copilot's comments were correct, and both were stale claims my own earlier fixes left behind. Addressed in ec056da.
This is the second round of the same class of error in this PR — a security comment claiming more than the mechanism delivers — so it is worth naming the pattern: the split genuinely removed repository write from the session, and each time I have overstated it, I have written it as though it removed the boundary. It did not. Rule 2's table now spells out which credential the allowlist still guards. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | gate charges the daily budget then finishes; a scan that is never scheduled (runner outage / inter-job cancellation) burns quota for a session that never ran. On record; the author documents it. |
.github/workflows/ai-scan.yml:74 |
| 2 | 🔵 Advisory | Security | Three named jobs are three public check runs, so scan's conclusion (skipped vs success) now leaks whether an item was scanned — and a budget-exhausted, fail-open day is exactly the unscanned/unflagged window. No reasoning leaks (show_full_output:false still holds). Author-documented and judged worth the credential split. |
.github/workflows/ai-scan.yml:79 |
| 3 | 🔵 Advisory | Robustness | always() on label means cancelling a run after the gate charges applies needs-security-review (empty verdict → fail-closed default). Safe direction, matches the pre-split always() labeler step, but worth knowing operationally. |
.github/workflows/ai-scan.yml:401 |
Overall: The change is sound and unusually well-reasoned. The core security property — the model session's job drops to contents/issues/pull-requests: read while the two write-scoped jobs (gate, label) run zero repository code and no model — is achieved exactly as issue #455 asks, and the --allowedTools allowlist is genuinely untouched. I traced every fail direction (gate skip, run=false, gate-fail-after-run=true, scan crash/cancel/skip, kill-switch-on-rerun) against the actual if: expressions and job-result semantics: the verdict path fails closed (every crash mode routes to the label job's flagged/other enum default), and the gate/counter path fails open, with no new fail-open hole where a real flag is lost. The riskiest part is the label job's if: (always() && gate.result=='success' && gate.outputs.run=='true' && scan.result != 'skipped') — that one expression carries both directions at once — so a human should focus there, but each clause is individually correct and the two guards added in commit 2 close the "flag an unscanned item" and "skipped-on-rerun reads as failure" gaps precisely.
Residual risk: the failure class this PR addresses is the session gaining write via a silently-widened allowlist and a flag being lost or spuriously applied across the new job boundary.
- Session write via allowlist widening — refuted: the write scopes now live only on
gate/label, which hold noCLAUDE_CODE_OAUTH_TOKENand run no checked-out code;scan's token is read-only, so wideningallowedToolscan no longer reach issue/PR write. The header and.github/CLAUDE.mdcorrectly keep the narrower claim that the allowlist still guards the OAuth token in-environment (I1). - Lost flag (fail-open on verdict) — refuted: I enumerated scan
success(flagged/empty verdict),failure, andcancelled; all satisfyscan.result != 'skipped'and reach theflagged/otherdefault. The only skip paths (gate.outputs.run != true, kill-switch off) are cases where no scan was warranted.label'sharden-runneriscontinue-on-error: true, so it cannot swallow a computed flag. - Spurious flag / double-label — refuted:
gate.result=='success'blocks flagging an item the gate never cleared to scan;scan.result != 'skipped'blocks the kill-switch-on-rerun path;addLabelsis idempotent. Theissues: write-only scope onlabelis correct — the/issues/{n}/labelsREST endpoint covers PRs, so droppingpull-requests: writeloses nothing.
🏄 Righteous refactor, dude — took one gnarly do-everything job and carved it into three clean breaks, and the write token never once shares a wave with the model. Fail-closed still holds all the way to the beach. Ship it.
|
🎉 This PR is included in version 5.11.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #455.
This is a security change (
.github/CLAUDE.mdrule 2)The
--allowedToolsallowlist is unchanged. What changes is that it is no longer the only thing standing behind it: the model session's job now holds no write scope at all.Credential per job, after the split:
gateGITHUB_TOKENissues: write(budget marker)scanGITHUB_TOKENcontents/issues/pull-requestsreadlabelGITHUB_TOKENissues: write(the labels API covers PRs too)Two properties worth stating plainly, because neither held before:
gateandlabelare dependency-free shell; the parser runs in the read-onlyscanjob.Why the parser stays in the
scanjobexecution_fileis a runner-local path, so it cannot cross a job boundary, and uploading it as an artifact would publish the scanner's reasoning to anyone with read access — precisely the evasion oracleshow_full_output: falseexists to prevent. Soscripts/parse-scan-verdict.mjs(unchanged) runs inscan, and only its coarse enum (clean/flagged <category>) becomes a job output. The labeler'scaseenum, itsflagged/otherdefaults, the step-summary lines, and theaddLabelscall are moved verbatim — the only edit is reading$SCAN_VERDICTinstead of invoking node.The
gateshell is byte-identical to the original (verified by diff).Fail directions
SCAN_VERDICTempty → enum default → flaggedrunnevertrue→ scan and label skip → unscanned, unlabeledif:false → nothing runsalways()→ flaggedAn earlier revision of this PR claimed the cancelled row was a behavior change introduced by the split. That was wrong — the pre-split labeler was a step carrying
always(), which also runs through cancellation — and the claim has been removed. Cancelling a run does not prevent the flag, before or after; that is now stated as a property rather than a delta.One deviation from the issue text
The issue sketches
scanascontents: read. Declaring only that sets every other scope tonone, so the session'sgh issue view/gh pr diffwould work solely because this repository is public — an undocumented dependency, invisible in review, that breaks under a private fork. The job therefore declaresissues: readandpull-requests: readas well. That reads as more permissions than the issue asked for while being strictly read-only, and it satisfies the acceptance box ("no write permission of any kind").Review round (commit 2)
A review of the first commit found three real defects, all fixed in
a551303:AI_LOOP_ENABLED/AI_SCAN_MODEwere only ongate. "Re-run failed jobs" replays a green gate's cachedrun=truerather than re-running it, so a billed session could start after a maintainer setAI_SCAN_MODE=off. Both variables are now repeated onscan, the job that spends the usage (rule 3).labelcould flag an unscanned item. Its condition read onlyneeds.gate.outputs.run, so a gate that wroterun=trueand then failed would skipscanwhilelabelran against an empty verdict and appliedneeds-security-review— fail-closed on a path rule 4 mandates fail-open. It now also requiresneeds.gate.result == 'success'andneeds.scan.result != 'skipped'.flaggedverdict. It iscontinue-on-error: truenow; the policy is not enforced anyway ([Security] harden-runner silently downgrades egress-policy: block to audit — no AI job has ever enforced egress #487).Plus several claims scoped back to what the mechanism delivers: the
.github/CLAUDE.mdrow no longer says the ai-scan allowlist holds back "nothing" (the job still runs Bash besideCLAUDE_CODE_OAUTH_TOKEN, so by I1 the allowlist is still the only control on that credential — repository write is what the split removed),SECURITY.mdno longer says the label job is one the session "cannot reach" (the session's verdict is that job's only input), andparse-scan-verdict.mjsand its test no longer describe the pre-split topology. Also dropped the unusedpull-requests: writefromlabel, and restored the repo-uniform egress endpoint list on the two new jobs.Empirically retired risk
A reviewer flagged that read-only permissions plus
claude-code-actionon apull_requesttrigger had no precedent in this repo, with a repo-wide false-positive as the failure mode. This PR's own ai-scan run settles it:gate→scan→labelall passed on PR #580 itself, and the label job loggedscan clean for #580 — no label. The action works at read scope on this trigger.Known follow-ups (not blocking)
harden-runner is now declared three times in this one file; the gate's budget shell is a near-verbatim copy of
ai-triage.yml's against the same tracking issue with opposite fail directions and no test on either; the verdict category enum lives in three places; andscandoes a full checkout for one 160-line script thatsparse-checkoutwould trim.Acceptance
writepermission of any kindis_errorstill flag, now via an empty verdict output)Review checklist
--allowedToolsand--disallowedToolsare untouched.GITHUB_TOKENthroughout; no PAT anywhere in this file.grep -rho "actions/checkout@[a-f0-9]\{40\}" .github/workflows/ | sort | uniq -c→ one line).blockper rule 10, and the header still states plainly that nothing in this repo enforces egress until [Security] harden-runner silently downgrades egress-policy: block to audit — no AI job has ever enforced egress #487 closes. No comment cites egress as a control.Verification
pnpm allgreen locally on both commits.parse-scan-verdict.mjsand its test sibling have header-only edits (their topology description was stale after the split); no behavior changed, and all 453 script tests pass.Worth watching on the first live run (needs
AI_SCAN_MODE=on): the three jobs chaining, the gate charging the marker on #290, and the label job loggingscan clean … — no label.Summary by CodeRabbit
Security Improvements
Documentation