From a4699696efffe483b94979973a542a3653131371 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Mon, 12 Oct 2026 02:41:02 +0200 Subject: [PATCH] docs(review): put the tally before Design and name who can store a trigger --- .github/workflows/claude-pr-review.yml | 40 +++++++++++++++++--------- REVIEW.md | 29 +++++++++++++------ 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index f7720db8e..e669f9765 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -215,12 +215,20 @@ jobs: `REVIEW.md` calls blocking. 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` - 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 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. 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 `Reviewed head: ${{ needs.prepare.outputs.sha }}`, gives each finding in full with its `file:line`, and ends with the coverage list @@ -378,8 +389,8 @@ jobs: || exit 0 echo "skipped=true" >> "$GITHUB_OUTPUT" 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 if: ${{ !cancelled() && steps.throttled.outputs.skipped != 'true' && steps.refused.outputs.skipped != 'true' }} 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." exit 1 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) awk '/^[ \t]*(```|~~~)/ { if (!open) print "_[Code removed: this review does not propose code.]_"; open = !open; next } !open' \ "$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" 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." diff --git a/REVIEW.md b/REVIEW.md index f512f6403..40635b8bc 100644 --- a/REVIEW.md +++ b/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 reaches the code, and what authorization, validation, escaping and 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 read, not inferred from the hunk. A dispatch-rule violation rarely shows 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 here" — multi-node, upgrade over an existing database, every link 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 the design does not cover, how an operator will meet it. Ideas, not instructions, and never code. @@ -272,9 +276,13 @@ evidence: | `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` | -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, 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 -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 once, in full, under its severity and `file:line`.