mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-09-09 11:48:18 +00:00
refactor(ci): review pull requests with one senior-engineer role
The review job ran the official code-review plugin, which fans a pull request out to five Sonnet reviewers plus a Haiku scorer per finding and drops everything scored under 80, and the briefing file spent most of its lines overriding that plugin. Both are gone: the job hands one Senior Software Engineer prompt to the action inline, the way the issue analyst does, and denies the Agent tool so the single role is mechanical rather than a request. REVIEW.md moves from the emoji markers to CRITICAL/HIGH/MEDIUM/LOW with a pre-existing qualifier. The uncapped rule is scoped to findings the pull request introduced or worsened so it cannot collide with the cap of three pre-existing ones. "A finding is a report, not a patch" stays as it was. Workflow housekeeping: GH_TOKEN, REPO and PR live in the job env instead of six step copies; the skip gate is per pull request, so a head pushed after the automatic review is reviewed only on @claude review; the comment counters sum gh's per-page jq output, which read "0\n0" as a review on a pull request with more than 100 comments; --max-turns rises to 300 because every read now costs the single agent a turn instead of a subagent.
This commit is contained in:
Sanaei
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user