diff --git a/.github/claude/review-job.md b/.github/claude/review-job.md index d81f26255..5264d3fc2 100644 --- a/.github/claude/review-job.md +++ b/.github/claude/review-job.md @@ -15,15 +15,31 @@ report, the repo-specific checks, the verification bar and the volume cap. The skill loads `CLAUDE.md` on its own; it does not load `REVIEW.md`, which is why this briefing exists. +Where the skill and `REVIEW.md` disagree, `REVIEW.md` wins. The skill treats +a pre-existing issue as a false positive, and a real issue on a line the pull +request did not modify too; here severity follows what the change caused, not +which lines it touched — a defect it introduced a frame outside the diff is +🔴 when it lands in an Important class, one it did not cause is 🟣, up to +three of those get posted, and a live security hole on an exposed surface +opens the summary. + +It also filters out every issue its confidence pass scores under 80 and posts +nothing once that empties the list; that rubric scores a nitpick 50, +`REVIEW.md` allots five nits, and the comment goes up either way. It says to +avoid emojis, and the whole severity system is three of them. Its "Found N +issues" format gives way to the tally, findings and coverage list below, and +its rule against reading build signal gives way to "CI is the build". + ## A finding is a report, not a patch Never post a `suggestion` block, and never write the fix: no patch, no replacement snippet, no rewritten function, no "suggested fix" section, in the -summary and in an inline comment alike. This overrides the skill's `--comment` -step, which would otherwise attach a committable suggestion to any small fix. -A finding states what is wrong, the `file:line`, what triggers it and what -breaks; one clause on where the fix belongs is the most it may add. The -maintainer decides the change. +summary and in an inline comment alike. The prompt that launches this job +passes `--comment` after the command; the skill defines no such flag, and it +is not a licence to attach a suggestion to a small fix. How narrow the one +clause naming where the fix belongs has to be, and what a finding says +instead, is `REVIEW.md`'s "A finding is a report, not a patch" — read it +there rather than from memory. The maintainer decides the change. ## Skip gate diff --git a/REVIEW.md b/REVIEW.md index b587f9939..a34cebde9 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -103,6 +103,10 @@ near-certain about and that actually breaks something: - A claim about behaviour needs a `file:line` citation from this repository, not an inference from a name. +- 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 + `internal/xray/api.go` call sits a frame outside it. - A claim that a downstream client rejects or requires a wire-format detail — a config key, JSON tag, URI query parameter, YAML or TOML key, an encoding or hash choice — must name the upstream symbol that decides it (repository, @@ -137,6 +141,10 @@ Open with a one-line tally — `2 🔴 / 4 🟡 / 1 🟣` — so the author sees shape of the review before the detail. When nothing is 🔴, lead with `No blocking issues` and put the tally after it. +Nothing pads the comment: no "Strengths" section, no restatement of what the +pull request does, no praise, no closing pleasantry. Padding is not neutral — +it buries the two lines someone actually has to act on. + The posted comment is the only part of a review anyone sees, so a bare "no issues found" is a receipt, not a review: nothing in it says whether the diff was read or the run died early. Every comment therefore ends with a short @@ -151,6 +159,18 @@ evidence, not a retelling of the pull request. A finding says what is wrong, where (`file:line`), what triggers it and what breaks. It never carries the fix: no `suggestion` block, no patch, no replacement snippet, no rewritten function, no "suggested fix" section — in -the summary and in an inline comment alike. One clause naming where the fix -belongs is the most it may add. The maintainer decides the change; a review -that writes it out puts unreviewed code one click from the branch. +the summary and in an inline comment alike. One clause naming WHERE the fix +belongs is the most it may add — a file, a function, a symbol, a layer — and +nothing about what happens there. Prose is a patch too the moment a verb +describes the change: "move the lookup inside the body", "spend the comment +on the invariant instead" hand it over as surely as a diff would, and so does +holding up an existing symbol as the model to copy. A clause the maintainer +could apply as written is the fix, however it is punctuated. The maintainer +decides the change; a review that writes it out puts unreviewed code one +click from the branch. + +A 🔴 or 🟡 finding also says, in one clause, what this pull request did to +the code it is about — the line it added, the call it moved, the guard it +dropped — the way a 🟣 says that it predates the change. That clause reports +what the change did, never what it should have done. Nothing else in the +comment shows the marker was earned.