Browse Source

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.
MHSanaei 23 hours ago
parent
commit
532392850f
3 changed files with 29 additions and 16 deletions
  1. 0 2
      .github/workflows/claude-issue-analyst.yml
  2. 3 3
      .github/workflows/claude-pr-review.yml
  3. 26 11
      REVIEW.md

+ 0 - 2
.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

+ 3 - 3
.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

+ 26 - 11
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