From 532392850feefe29a906164251073304d4714c2e Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Mon, 12 Oct 2026 13:39:29 +0200 Subject: [PATCH] fix(ci): drop id-token from the Claude workflows and tighten the review comment CodeQL alert #121 flags the pr-head checkout in the PR review job. The checkout is deliberate and nothing in the job executes it, but the job still carried `id-token: write`, and so did the issue analyst. claude-code-action only uses OIDC to trade for a Claude GitHub App token when no github_token is passed - that app token asks for contents, pull_requests and issues write - or for workload identity, which neither workflow uses. Both pass github_token, so the permission did nothing for them, yet any code running in either job could have minted that write token, undoing the review's read-only split. Three reviews posted on one morning (#6666, #6675, #6752) also showed rubric gaps. REVIEW.md said both "open with the verdict" and "lead with No blocking issues", so the comments opened three ways and two tallies dropped severities; the opening is now a fixed order - tag line only when blocking, verdict paragraph, one status line whose tally names all three severities. #6675 filed a spoofable X-Forwarded-For IP, which that PR first shows to an admin approving a login, under Design; Design now holds approaches and trade-offs only, and a concrete consequence the change makes reachable or worse is a finding. The coverage list ran to some 25 nested lines on #6752; it is now ten lines at most, one per area, no nested bullets. --- .github/workflows/claude-issue-analyst.yml | 2 -- .github/workflows/claude-pr-review.yml | 6 ++-- REVIEW.md | 37 +++++++++++++++------- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/.github/workflows/claude-issue-analyst.yml b/.github/workflows/claude-issue-analyst.yml index 55e9e6e64..b0d1d2051 100644 --- a/.github/workflows/claude-issue-analyst.yml +++ b/.github/workflows/claude-issue-analyst.yml @@ -9,7 +9,6 @@ on: permissions: contents: read issues: write - id-token: write jobs: issue-analyst: @@ -29,7 +28,6 @@ jobs: permissions: contents: read issues: write - id-token: write steps: - name: Record when this run started id: started diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index e669f9765..bca3ffa15 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -99,7 +99,6 @@ jobs: contents: read pull-requests: read issues: read - id-token: write env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} @@ -297,8 +296,9 @@ jobs: with the Write tool - the only file you may write - as your last action, never a placeholder or a draft. Whatever that file holds when your turn ends is what the workflow posts; a run that ends - without the finished review in it has failed. The comment opens - with the verdict, then the tally, carries the line + without the finished review in it has failed. The comment follows + the opening order in `REVIEW.md` - verdict paragraph, then the + status line with the full tally - carries the line `Reviewed head: ${{ needs.prepare.outputs.sha }}`, gives each finding in full with its `file:line`, and ends with the coverage list `REVIEW.md` asks for, whether or not you found anything. When there is diff --git a/REVIEW.md b/REVIEW.md index 40635b8bc..47fda3527 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -33,7 +33,8 @@ changes, so pre-existing bugs surface on every review. One already on the base branch stays pre-existing however bad it is: this pull request did not cause it, so it cannot be a reason to hold this pull request. Say in one clause that it predates the change. The exception is a live security hole on an exposed -surface — still pre-existing, but open the summary with it. +surface — still pre-existing, but it gets the maintainer tag line that opens +the comment. ## What HIGH means here @@ -263,7 +264,12 @@ implementation — and which ones it leaves out. Then give one verdict: `Fits`, **Design** section: in prose only, the ideas a senior engineer would raise about the approach — a simpler model, an existing mechanism it could build on, a case the design does not cover, how an operator will meet it. Ideas, not -instructions, and never code. +instructions, and never code. Design holds approaches and trade-offs only. A +concrete consequence — something an attacker can exploit, or that breaks for +an operator — is a finding, even when its root predates the pull request, +whenever the change makes it reachable or worse: rated by what the change +added, on the surface it now sits on. A pre-existing spoofable value the pull +request first puts in front of someone making a trust decision is that case. Every other kind gets one question and one verdict, with the same `file:line` evidence: @@ -291,13 +297,20 @@ full base-to-head trace is for changes to what the panel does. Anything under ## What the comment must show -Open with the verdict paragraph, then — before the evidence, the Design -section and everything else — a one-line tally: `1 HIGH / 2 MEDIUM / 1 LOW -open (1 new), 1 pre-existing`. It counts every finding still open, those -carried from earlier rounds included, and says how many are new this round; -a pre-existing finding counts only in its own bucket. The author sees the -shape of the review before the detail. When nothing is blocking, lead with -`No blocking issues` and put the tally after it. +Every comment opens in the same order, so a reader can rely on where things +sit: + +1. Only when something is blocking, the line that tags the maintainer. +2. The verdict paragraph. +3. One status line: `No blocking issues` or `Blocking`, then the tally — + `No blocking issues — 0 HIGH / 1 MEDIUM / 2 LOW open (1 new), + 0 pre-existing`. All three severities appear even when zero. The tally + counts every finding still open, those carried from earlier rounds + included, and says how many are new this round; a pre-existing finding + counts only in its own bucket. + +Then `Reviewed head:`, the evidence, the Design section, earlier rounds, the +findings and the coverage list. The review is one plain comment, with no inline comments. Say each finding once, in full, under its severity and `file:line`. @@ -312,8 +325,10 @@ was read or the run died early. Every comment therefore ends with a short coverage list — one line per area actually checked, naming what was examined and what it turned out to be, plus the head SHA and the size of the diff it covers. Say which claims could not be verified and why, including a check -this environment blocked. Keep that coverage list under ten lines; it is -evidence, not a retelling of the pull request. +this environment blocked. Keep that coverage list to ten lines at most, one +line per area and no nested bullets; it is evidence, not a retelling of the +pull request. Only the list of unread files a partial review names sits +outside that cap. ## A finding is a report, not a patch