diff --git a/.github/claude/repo-context.md b/.github/claude/repo-context.md index f19fbb117..b91db5dba 100644 --- a/.github/claude/repo-context.md +++ b/.github/claude/repo-context.md @@ -3,9 +3,8 @@ Briefing for the issue analyst in `.github/workflows/claude-issue-analyst.yml`. It exists so these facts live in ONE place next to the code instead of being restated in the prompt, where they went stale silently. (Pull-request review is -separate: the code-review skill in `.github/workflows/claude-pr-review.yml` is -briefed with `CLAUDE.md`, `REVIEW.md` and `.github/claude/review-job.md`, not -this.) +separate: the reviewer in `.github/workflows/claude-pr-review.yml` is briefed by +its own prompt, `CLAUDE.md` and `REVIEW.md`, not this.) `CLAUDE.md`, `frontend/CLAUDE.md` and `docs/architecture.md` outrank this file. Where they disagree with it, they win and this file is the thing to fix. diff --git a/.github/claude/review-job.md b/.github/claude/review-job.md deleted file mode 100644 index 1886d9248..000000000 --- a/.github/claude/review-job.md +++ /dev/null @@ -1,81 +0,0 @@ -# Review job briefing - -Appended to the system prompt of the pull-request review job in -`.github/workflows/claude-pr-review.yml`. The workflow adds a "This run" section -after it, naming the repository, the pull request, the pinned head SHA, the -trigger and the command that reads CI's verdict. `REVIEW.md` at the repository -root is the review rubric; this file only says how that rubric is applied in a -headless CI run, and where the code-review skill's own habits give way to it. - -## Read REVIEW.md first - -Before reviewing, read `REVIEW.md` and follow it: the severity marker every -finding carries, what counts as Important in this repository, what not to -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. 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 - -An existing review comment justifies skipping only when its `Reviewed head:` -line names the head SHA of this run. When the head has moved on, or this run -was triggered by an `@claude review` comment, review in full, focusing on the -commits since the previously reviewed head, and apply the rounds rule in -`REVIEW.md`: after the first review of a pull request, 🔴 findings only. - -## Headless run - -This run ends the moment you end your turn. Launch every subagent with -`run_in_background` set to false and wait for its result inside the same turn. -Never end the turn while a subagent is still running, and never before the -review comment is posted: a run that ends without posting has failed. - -## What is checked out where - -The working tree is the BASE branch. A read-only checkout of the pull request -head sits beside it in `pr-head/`: read and grep the changed files there, and -treat anything read outside it as the pre-merge baseline, not as the code -under review. Never build, install or execute anything from `pr-head/`. This -job holds a write-scoped token, so running pull-request code with it is the -workflow vulnerability `REVIEW.md` calls blocking. - -## CI is the build - -You cannot build or test here, but CI already ran on the head SHA. Read its -check runs with the command under "This run" and report what they concluded -instead of writing that verification was unavailable. A required check that -failed, or that never ran on this head, is itself a finding. - -## The comment - -The comment you post is the only part of this run anyone sees. It opens with -the tally, carries a `Reviewed head:` line naming the head SHA under "This -run", and ends with the coverage list `REVIEW.md` asks for, whether or not you -found anything. Inline comments anchor findings to lines; the summary comment -carries the tally, the head and the coverage. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 09384bf41..82be90ff1 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -33,6 +33,10 @@ jobs: pull-requests: write issues: read id-token: write + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number || github.event.issue.number }} steps: - name: Record when this run started id: started @@ -43,8 +47,6 @@ jobs: if: github.event_name == 'issue_comment' continue-on-error: true env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} COMMENT_ID: ${{ github.event.comment.id }} run: gh api "repos/${REPO}/issues/comments/${COMMENT_ID}/reactions" -f content=eyes - uses: actions/checkout@v7 @@ -55,9 +57,6 @@ jobs: - name: Pin the head this run reviews id: pinned-sha env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number || github.event.issue.number }} PAYLOAD_SHA: ${{ github.event.pull_request.head.sha }} COMMENT_AT: ${{ github.event.comment.created_at }} run: | @@ -80,23 +79,19 @@ jobs: exit 1 fi echo "sha=${HEAD_SHA}" >> "$GITHUB_OUTPUT" - # An automatic re-review of a head that already has one spends a whole run - # to reach the same conclusion, so settle it here rather than in the model. - - name: Skip a head that already has a review + # One automatic review per pull request: a later push is reviewed only + # when a maintainer asks for it with `@claude review`. + - name: Skip a pull request that already has a review id: reviewed if: github.event_name == 'pull_request_target' - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number }} - HEAD_SHA: ${{ steps.pinned-sha.outputs.sha }} run: | set -euo pipefail posted=$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - --jq "[.[] | select(.user.login == \"github-actions[bot]\") | select((.body | contains(\"Reviewed head:\")) and (.body | contains(\"${HEAD_SHA}\")))] | length") + --jq '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("Reviewed head:"))] | length' \ + | awk '{n += $1} END {print n + 0}') if [ "$posted" != "0" ]; then echo "done=true" >> "$GITHUB_OUTPUT" - echo "::notice::#${PR} already carries a review of ${HEAD_SHA}; nothing to review." + echo "::notice::#${PR} already carries a review; nothing to review." fi # Read-only, and pinned to one immutable commit: this job holds a # write-scoped token, so running anything out of pr-head/ would be a pwn-request. @@ -107,26 +102,6 @@ jobs: path: pr-head persist-credentials: false allow-unsafe-pr-checkout: true - # The skill reads CLAUDE.md on its own but not REVIEW.md, and knows nothing - # of pr-head/ or this run's head: the brief is the only way both reach it. - - name: Brief the reviewer - if: steps.reviewed.outputs.done != 'true' - env: - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number || github.event.issue.number }} - HEAD_SHA: ${{ steps.pinned-sha.outputs.sha }} - TRIGGER: ${{ github.event_name }} / ${{ github.event.action }} - run: | - set -euo pipefail - { - cat .github/claude/review-job.md - printf '\n## This run\n\n' - printf -- '- Repository: %s\n' "$REPO" - printf -- '- Pull request: #%s\n' "$PR" - printf -- '- Head under review, checked out read-only in pr-head/: %s\n' "$HEAD_SHA" - printf -- '- Trigger: %s\n' "$TRIGGER" - printf -- '- CI on that head: gh api repos/%s/commits/%s/check-runs\n' "$REPO" "$HEAD_SHA" - } > "$RUNNER_TEMP/review-brief.md" - uses: anthropics/claude-code-action@v1 id: review if: steps.reviewed.outputs.done != 'true' @@ -137,25 +112,99 @@ jobs: github_token: ${{ secrets.GITHUB_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} allowed_non_write_users: "*" - plugin_marketplaces: "https://github.com/anthropics/claude-code.git" - plugins: "code-review@claude-code-plugins" - prompt: "/code-review:code-review max --comment ${{ github.repository }}/pull/${{ github.event.pull_request.number || github.event.issue.number }}" - # allowedTools only pre-approves; it denies nothing. Only the deny - # list stops the review executing what it just checked out. + # Claude Code loads a CLAUDE.md or .claude/rules/ file the moment a file + # beside it is read, so a fork's copy under pr-head/ would brief its own review. + settings: '{"claudeMdExcludes": ["**/pr-head/**"]}' + # allowedTools only pre-approves; it denies nothing. Only the deny list + # stops the review executing what it just checked out, or delegating. claude_args: | --model claude-opus-5 --effort xhigh - --max-turns 100 - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh api:*),Bash(gh pr diff:*),Bash(grep:*),Bash(rg:*),Bash(ls:*),Bash(find:*),Bash(sed:*),Bash(git log:*),Bash(git show:*),Bash(git diff:*),Bash(go doc:*),Bash(go env:*),Read,Glob,Grep,WebFetch,WebSearch" - --disallowedTools "Bash(go build:*),Bash(go run:*),Bash(go test:*),Bash(go generate:*),Bash(go install:*),Bash(make:*),Bash(npm:*),Bash(npx:*),Bash(pnpm:*),Bash(yarn:*),Bash(node:*),Bash(bash:*),Bash(sh:*),Bash(docker:*),Bash(chmod:*),Edit,Write,NotebookEdit" - --append-system-prompt-file ${{ runner.temp }}/review-brief.md + --max-turns 300 + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh api:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment ${{ env.PR }}:*),Bash(grep:*),Bash(rg:*),Bash(ls:*),Bash(find:*),Bash(sed:*),Bash(git log:*),Bash(git show:*),Bash(git diff:*),Bash(git blame:*),Bash(go doc:*),Bash(go env:*),Read,Glob,Grep,WebFetch,WebSearch" + --disallowedTools "Agent,Bash(go build:*),Bash(go run:*),Bash(go test:*),Bash(go generate:*),Bash(go install:*),Bash(make:*),Bash(npm:*),Bash(npx:*),Bash(pnpm:*),Bash(yarn:*),Bash(node:*),Bash(bash:*),Bash(sh:*),Bash(docker:*),Bash(chmod:*),Edit,Write,NotebookEdit" + prompt: | + You are a Senior Software Engineer performing a production-grade code + review of pull request #${{ env.PR }} in ${{ env.REPO }}. You are the + only reviewer: no other role, no subagent, no second pass. What you + post is the whole review. + + Your goal is to identify real defects and meaningful risks, not to + criticise style or suggest refactoring nobody needs. Review the entire + change in the context of the existing codebase, not the hunks alone. + + Prioritise, in this order: + 1. Correctness + 2. Bugs and edge cases + 3. Security + 4. Concurrency and race conditions + 5. Performance + 6. Data integrity + 7. API and backward compatibility + 8. Error handling + 9. Maintainability + 10. Test coverage + + Report only what is actionable and supported by evidence from the + code. Do not invent hypothetical problems. Do not nitpick formatting + or personal style. Do not request tests merely to raise coverage. + If the implementation is correct, say so. Do not manufacture findings. + + For every finding, explain the problem, why it can happen, which code + is affected (`file:line`), and the impact. Mark it with one severity: + CRITICAL - security, data loss, corruption, or severe production failure + HIGH - a significant functional or production issue + MEDIUM - a real bug or a meaningful reliability or performance problem + LOW - a minor but legitimate issue + + THE RUBRIC + Read `REVIEW.md` at the repository root before the diff, and follow it: + what is HIGH in this repository, the checks to always run, what not to + report, the verification bar, the volume cap and the shape of the + comment. It also settles the one thing a finding never carries: the + fix. Name where the fix belongs, never what it is - no patch, no + snippet, no suggestion block, no rewrite in prose. The maintainer + decides the change. + + WHAT IS CHECKED OUT WHERE + The working tree is the BASE branch. The head under review, + ${{ steps.pinned-sha.outputs.sha }}, is checked out read-only in + `pr-head/`: read and grep the changed files there, and treat anything + outside it as the pre-merge baseline. Never build, install or execute + anything from `pr-head/`. This job holds a write-scoped token, and + running pull-request code with it is the workflow vulnerability + `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 + `gh api repos/${{ env.REPO }}/commits/${{ steps.pinned-sha.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. + + ROUNDS + Trigger: ${{ github.event_name }} / ${{ github.event.action }}. On an + `@claude review`, review in full even when an earlier comment of yours + exists, focusing on the commits since the head it names, and apply the + rounds rule in `REVIEW.md`: after the first review of a pull request, + MEDIUM and above only. + + THE COMMENT + This run ends the moment you end your turn, and a run that ends + without posting has failed. Anchor each finding to its line with an + inline comment, then post the summary with + `gh pr comment ${{ env.PR }} --repo ${{ env.REPO }}`. The summary opens + with the tally, carries the line + `Reviewed head: ${{ steps.pinned-sha.outputs.sha }}`, and ends with the + coverage list `REVIEW.md` asks for, whether or not you found anything. - name: Upload the run transcript if: always() env: NODE_OPTIONS: "" uses: actions/upload-artifact@v7 with: - name: claude-review-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.run_id }}-${{ github.run_attempt }} + name: claude-review-${{ env.PR }}-${{ github.run_id }}-${{ github.run_attempt }} path: ${{ runner.temp }}/claude-execution-output.json if-no-files-found: ignore retention-days: 7 @@ -165,9 +214,6 @@ jobs: id: throttled if: ${{ !cancelled() && steps.review.outcome == 'failure' }} env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number || github.event.issue.number }} TRANSCRIPT: ${{ runner.temp }}/claude-execution-output.json run: | set -euo pipefail @@ -182,23 +228,19 @@ jobs: echo "skipped=true" >> "$GITHUB_OUTPUT" echo "::notice::No review of #${PR}: ${reason}." gh pr comment "$PR" --repo "$REPO" --body "No review ran on this head: ${reason}. Nothing in this pull request was examined. A maintainer can ask for one with \`@claude review\`." + # updated_at, not created_at: a re-review may edit its earlier comment. + # --paginate prints one jq count per page, so the pages are summed. - name: Fail if the review posted nothing if: ${{ !cancelled() && steps.pinned-sha.outcome == 'success' && steps.reviewed.outputs.done != 'true' && steps.throttled.outputs.skipped != 'true' }} env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number || github.event.issue.number }} + HEAD_SHA: ${{ steps.pinned-sha.outputs.sha }} STARTED_AT: ${{ steps.started.outputs.at }} run: | set -euo pipefail - head=$(gh api "repos/${REPO}/pulls/${PR}" --jq '.head.sha') - # updated_at, not created_at: the skill may update its existing sticky comment. - # "Reviewed head:" as well as the SHA — the bot's other comments quote SHAs too. - posted=$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - --jq "[.[] | select(.user.login == \"github-actions[bot]\") | select((.updated_at >= \"${STARTED_AT}\") or ((.body | contains(\"Reviewed head:\")) and (.body | contains(\"${head}\"))))] | length") - inline=$(gh api "repos/${REPO}/pulls/${PR}/comments" --paginate \ - --jq "[.[] | select(.user.login == \"github-actions[bot]\") | select(.updated_at >= \"${STARTED_AT}\")] | length") + since="[.[] | select(.user.login == \"github-actions[bot]\") | select(.updated_at >= \"${STARTED_AT}\")] | length" + posted=$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate --jq "$since" | awk '{n += $1} END {print n + 0}') + inline=$(gh api "repos/${REPO}/pulls/${PR}/comments" --paginate --jq "$since" | awk '{n += $1} END {print n + 0}') if [ "$posted" = "0" ] && [ "$inline" = "0" ]; then - echo "::error::The review run ended without posting a review of ${head} on #${PR}. Read the uploaded transcript before re-running." + echo "::error::The review run ended without posting a review of ${HEAD_SHA} on #${PR}. Read the uploaded transcript before re-running." exit 1 fi diff --git a/REVIEW.md b/REVIEW.md index 648756b8d..85cb87530 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -9,28 +9,33 @@ breaks for those consumers and operators, not by style. Mark every finding with exactly one of these, at the start of the finding: -| Marker | Severity | Use it for | -| --- | --- | --- | -| 🔴 | Important | A defect this pull request introduces or makes worse, in one of the classes under "What Important means here". Worth fixing before it merges. | -| 🟡 | Nit | Style, naming, refactoring, and an ordinary `CLAUDE.md` violation the change introduces — a source comment block over two lines, a fix larger than the bug it removes, a test `CLAUDE.md` rejects outright. | -| 🟣 | Pre-existing | A real bug you hit while reading that this pull request neither introduced nor made worse. | +| Severity | Use it for | +| --- | --- | +| CRITICAL | Security, data loss, corruption, or a severe production failure. | +| HIGH | A significant functional or production issue. Everything under "What HIGH means here" is at least this. | +| MEDIUM | A real bug, or a meaningful reliability or performance problem. | +| LOW | A minor but legitimate issue, including an ordinary `CLAUDE.md` violation the change introduces — a source comment block over two lines, a fix larger than the bug it removes, a test `CLAUDE.md` rejects outright. Never formatting or personal style. | -Not every `CLAUDE.md` rule is a nit. The three listed below — the dispatch -rule, the migration rule, the endpoint chain — are Important, because each one -passes every local test and breaks a real deployment. +A CRITICAL or HIGH finding this pull request introduced or made worse is +blocking: worth fixing before it merges. Not every `CLAUDE.md` rule is LOW. +The three listed below — the dispatch rule, the migration rule, the endpoint +chain — are HIGH, because each one passes every local test and breaks a real +deployment. -Severity follows what this pull request did, not how alarming the defect looks -on its own. One the change worsens is 🔴 for the regression it added, not for -the whole defect; one it merely brought into view is 🟣. +Severity rates the defect; a second word says whose it is. A real bug you hit +while reading that this pull request neither introduced nor made worse is +marked pre-existing after its severity — `MEDIUM pre-existing` — and is never +blocking. One the change worsens is rated for the regression it added, not for +the whole defect. Checking what this panel emits means reading far more code than the diff changes, so pre-existing bugs surface on every review. One already on the base -branch stays 🟣 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 🟣, but open the summary with it. +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. -## What Important means here +## What HIGH means here - Security on the exposed surfaces: `internal/web/controller/`, session and middleware code, the PUBLIC `internal/sub/` subscription server, and Xray @@ -64,7 +69,7 @@ surface — still 🟣, but open the summary with it. in `tools/openapigen/main.go`, and `frontend/public/openapi.json` copied to `docs/public/openapi.json` with the docs MDX regenerated (`cd docs && pnpm gen:api`). CI checks the first three; the docs copy is - checked by nothing — a missed copy is Important, not a nit. + checked by nothing — a missed copy is HIGH, not LOW. - A bug fix carries a test that would fail without the fix. A test that cannot tell the broken behaviour from the fixed one passes before and after, so it certifies nothing and is itself the finding — asserting only `err != nil` or @@ -92,7 +97,7 @@ surface — still 🟣, but open the summary with it. ## A higher bar, not silence -Everything named under "What Important means here" gets full scrutiny. Two +Everything named under "What HIGH means here" gets full scrutiny. Two areas do not — they earn review, but report there only what you are near-certain about and that actually breaks something: @@ -125,24 +130,27 @@ near-certain about and that actually breaks something: ## Cap the volume -🔴 findings are never capped. Report every one. +CRITICAL, HIGH and MEDIUM findings this pull request introduced or made worse +are never capped. Report every one. -Report at most five 🟡 nits and at most three 🟣 pre-existing bugs. Past that, -say "plus N similar" in the summary instead of posting them. +Report at most five LOW and at most three pre-existing findings, whatever +their severity. Past that, say "plus N similar" in the summary instead of +posting them. -A cap decides WHICH ones survive, so choose rather than truncate: the same nit -repeated across files is ONE finding with a count, not five slots; a nit in -code this pull request wrote outranks one in code it only moved; and a nit -nobody would act on does not deserve a slot at all. +A cap decides WHICH ones survive, so choose rather than truncate: the same LOW +repeated across files is ONE finding with a count, not five slots; one in code +this pull request wrote outranks one in code it only moved; and one nobody +would act on does not deserve a slot at all. -After the first review of a pull request, report 🔴 findings only: a one-line -fix must not reach round seven on style. +After the first review of a pull request, report MEDIUM and above only: a +one-line fix must not reach round seven on style. ## What the comment must show -Open with a one-line tally — `2 🔴 / 4 🟡 / 1 🟣` — so the author sees the -shape of the review before the detail. When nothing is 🔴, lead with -`No blocking issues` and put the tally after it. +Open with 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. Nothing pads the comment: no "Strengths" section, no restatement of what the pull request does, no praise, no closing pleasantry. Padding is not neutral — @@ -172,8 +180,8 @@ 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. +A finding that is not pre-existing 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 pre-existing one 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.