Browse Source

docs(review): put the tally before Design and name who can store a trigger

MHSanaei 16 hours ago
parent
commit
a4699696ef
2 changed files with 47 additions and 22 deletions
  1. 27 13
      .github/workflows/claude-pr-review.yml
  2. 20 9
      REVIEW.md

+ 27 - 13
.github/workflows/claude-pr-review.yml

@@ -215,12 +215,20 @@ jobs:
             `REVIEW.md` calls blocking.
             `REVIEW.md` calls blocking.
 
 
             CI IS THE BUILD
             CI IS THE BUILD
-            You cannot build or test here, but CI already ran on the head. Read
-            its check runs with
+            You cannot build or test here; CI builds the head, and may still be
+            running or waiting for a maintainer to approve a fork's run. Read its
+            check runs once, near the end, with
             `gh api repos/${{ env.REPO }}/commits/${{ needs.prepare.outputs.sha }}/check-runs`
             `gh api repos/${{ env.REPO }}/commits/${{ needs.prepare.outputs.sha }}/check-runs`
-            and report what they concluded instead of writing that verification
-            was unavailable. A required check that failed, or never ran on this
-            head, is itself a finding.
+            and report each as it stands: concluded, still running, or never
+            started. Never wait or poll for CI - nothing resumes this run. A
+            check that concluded in failure is a finding; one still running is
+            unverified, and the coverage list says so. A fork's run that is
+            waiting for approval (`action_required`) is not a finding against
+            the pull request - nothing its author can do starts it - so the
+            coverage list says CI awaits maintainer approval and that nothing
+            it covers is verified until a maintainer approves the run. A
+            finding of yours from an earlier round that said only this is
+            withdrawn on that ground.
 
 
             UPSTREAM SOURCE
             UPSTREAM SOURCE
             The xray-core module the base `go.mod` pins is unpacked read-only at
             The xray-core module the base `go.mod` pins is unpacked read-only at
@@ -282,11 +290,14 @@ jobs:
             a maintainer, or withdrawn as wrong with the `file:line` that shows it.
             a maintainer, or withdrawn as wrong with the `file:line` that shows it.
 
 
             THE COMMENT
             THE COMMENT
-            You post nothing yourself, and your token cannot write to GitHub.
-            The review is ONE plain comment: write its body to `/tmp/review.md`
-            with the Write tool - the only file you may write - and end your
-            turn; the workflow posts that file on the pull request afterwards.
-            A run that ends without writing it has failed. The comment opens
+            This run ends the moment you end your turn: nothing waits, polls or
+            resumes it, so the whole review happens in this one turn. You post
+            nothing yourself, and your token cannot write to GitHub. The review
+            is ONE plain comment: write its finished body to `/tmp/review.md`
+            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
             with the verdict, then the tally, carries the line
             `Reviewed head: ${{ needs.prepare.outputs.sha }}`, gives each
             `Reviewed head: ${{ needs.prepare.outputs.sha }}`, gives each
             finding in full with its `file:line`, and ends with the coverage list
             finding in full with its `file:line`, and ends with the coverage list
@@ -378,8 +389,8 @@ jobs:
             || exit 0
             || exit 0
           echo "skipped=true" >> "$GITHUB_OUTPUT"
           echo "skipped=true" >> "$GITHUB_OUTPUT"
           echo "::warning::No review of #${PR}: the Claude credential was refused, so nothing in this pull request was examined."
           echo "::warning::No review of #${PR}: the Claude credential was refused, so nothing in this pull request was examined."
-      # The review never proposes code, so a fence is cut out before posting. The
-      # skip in `prepare` keys on the Reviewed head line, so it is never left out.
+      # A body without its own Reviewed head line is unfinished (run 38188509693
+      # posted a "placeholder"), and posting it would mark the PR as reviewed.
       - name: Post the review
       - name: Post the review
         if: ${{ !cancelled() && steps.throttled.outputs.skipped != 'true' && steps.refused.outputs.skipped != 'true' }}
         if: ${{ !cancelled() && steps.throttled.outputs.skipped != 'true' && steps.refused.outputs.skipped != 'true' }}
         env:
         env:
@@ -391,10 +402,13 @@ jobs:
             echo "::error::The review run ended without writing a review of ${HEAD_SHA} for #${PR}. Read the uploaded transcript before re-running."
             echo "::error::The review run ended without writing a review of ${HEAD_SHA} for #${PR}. Read the uploaded transcript before re-running."
             exit 1
             exit 1
           fi
           fi
+          if ! grep -qF "Reviewed head: ${HEAD_SHA}" "$BODY"; then
+            echo "::error::The review of ${HEAD_SHA} for #${PR} is unfinished: its body lacks the Reviewed head line, so nothing was posted. Read the uploaded transcript before re-running."
+            exit 1
+          fi
           out=$(mktemp)
           out=$(mktemp)
           awk '/^[ \t]*(```|~~~)/ { if (!open) print "_[Code removed: this review does not propose code.]_"; open = !open; next } !open' \
           awk '/^[ \t]*(```|~~~)/ { if (!open) print "_[Code removed: this review does not propose code.]_"; open = !open; next } !open' \
             "$BODY" > "$out"
             "$BODY" > "$out"
-          grep -qF "Reviewed head: ${HEAD_SHA}" "$out" || printf '\nReviewed head: %s\n' "$HEAD_SHA" >> "$out"
           gh pr comment "$PR" --repo "$REPO" --body-file "$out"
           gh pr comment "$PR" --repo "$REPO" --body-file "$out"
           if grep -qE '^[[:space:]]*(```|~~~)' "$BODY"; then
           if grep -qE '^[[:space:]]*(```|~~~)' "$BODY"; then
             echo "::error::Removed fenced code from the review of #${PR} before posting it: the review must not propose code."
             echo "::error::Removed fenced code from the review of #${PR} before posting it: the review must not propose code."

+ 20 - 9
REVIEW.md

@@ -161,6 +161,10 @@ near-certain about and that actually breaks something:
 - A security finding traces the trust boundary the change sits on — who
 - A security finding traces the trust boundary the change sits on — who
   reaches the code, and what authorization, validation, escaping and
   reaches the code, and what authorization, validation, escaping and
   privilege it assumes — against the existing code, not the hunk.
   privilege it assumes — against the existing code, not the hunk.
+- A finding triggered by a stored value — a setting, a host field, a client
+  limit — names who can store it: an unauthenticated caller, a client, or
+  only an authenticated admin or API token. "Any API client" when only an
+  admin can write the value overstates the risk.
 - A claim about what the change does to a caller or a callee needs that file
 - A claim about what the change does to a caller or a callee needs that file
   read, not inferred from the hunk. A dispatch-rule violation rarely shows
   read, not inferred from the hunk. A dispatch-rule violation rarely shows
   inside the diff — the changed line calls an innocuous helper and the
   inside the diff — the changed line calls an innocuous helper and the
@@ -255,8 +259,8 @@ the panel already offers it or most of it (setting, inbound option, existing
 endpoint); whether it holds across the deployment shapes in "What HIGH means
 endpoint); whether it holds across the deployment shapes in "What HIGH means
 here" — multi-node, upgrade over an existing database, every link
 here" — multi-node, upgrade over an existing database, every link
 implementation — and which ones it leaves out. Then give one verdict: `Fits`,
 implementation — and which ones it leaves out. Then give one verdict: `Fits`,
-`Fits with gaps` or `Overlaps existing` — and under it a short **Design**
-section: in prose only, the ideas a senior engineer would raise about the
+`Fits with gaps` or `Overlaps existing` — and, after the tally, a short
+**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
 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
 the design does not cover, how an operator will meet it. Ideas, not
 instructions, and never code.
 instructions, and never code.
@@ -272,9 +276,13 @@ evidence:
 | `docs` | Does it match what the code does? | `Accurate` / `Inaccurate` (cite the line it contradicts) |
 | `docs` | Does it match what the code does? | `Accurate` / `Inaccurate` (cite the line it contradicts) |
 | `ci`, workflow | Does it keep every guard named under "What HIGH means here"? | `No guard weakened` / `Weakens a guard` |
 | `ci`, workflow | Does it keep every guard named under "What HIGH means here"? | `No guard weakened` / `Weakens a guard` |
 
 
-Write the verdict for someone who has not read the diff: two to six sentences
-on what was broken or missing and what now changes, then the evidence. On a
-re-review, update it in light of the new commits.
+Write the verdict paragraph for someone who has not read the diff: two to six
+plain sentences, with no `file:line`, symbol or library internals, on what an
+operator or end user ran into — the client that failed, the traffic that was
+dropped, the setting that was ignored — and what changes for them now. The
+evidence the questions above ask for — traces, citations, test reasoning —
+goes in a separate paragraph after it. On a re-review, update both in light of the new
+commits.
 
 
 Scale the trace to the change. One with no runtime effect — a typo, a comment,
 Scale the trace to the change. One with no runtime effect — a typo, a comment,
 a docs wording — gets a one- or two-line verdict and a short coverage list; the
 a docs wording — gets a one- or two-line verdict and a short coverage list; the
@@ -283,10 +291,13 @@ full base-to-head trace is for changes to what the panel does. Anything under
 
 
 ## What the comment must show
 ## What the comment must show
 
 
-Open with the verdict, then a one-line tally — `1 HIGH / 2 MEDIUM / 1 LOW, 1 pre-existing`,
-where a pre-existing finding counts only in its own bucket — so 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.
+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.
 
 
 The review is one plain comment, with no inline comments. Say each finding
 The review is one plain comment, with no inline comments. Say each finding
 once, in full, under its severity and `file:line`.
 once, in full, under its severity and `file:line`.