diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index cd22107bf..e669f9765 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -222,7 +222,13 @@ jobs: 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. + 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 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`.