Sanaei
5321665d5b
REVIEW.md said what blocks and what does not, but never how to mark a finding, so every review invented its own shape and none carried a severity. It now names the three markers the hosted Code Review service uses - Important, Nit, Pre-existing - and keys them to what the pull request did rather than to how alarming a defect looks alone: a defect it introduces or worsens is Important, one it merely brought into view is Pre-existing and cannot be a reason to hold it. Pre-existing was missing entirely, and checking what this panel emits means reading far outside the diff, so those findings had nowhere to go except a wrong Important or silence. The volume cap said how many and never which. It now collapses a nit repeated across files into one finding, prefers a nit in code the pull request wrote over one in code it only moved, caps pre-existing findings at three, and states that Important findings are never capped - a section listing two caps otherwise reads as licence to trim what matters. The review opens with a tally so the author sees the shape before the detail. Two contradictions went with it. The file told the reviewer to skip what CI enforces and then to check that a new i18n key reaches all 13 locales, which i18n-dead-keys.test.ts pins in both directions - the rule moves to "Do not report" with the reason. "Anything CI already enforces: npm audit" overstated what runs; CI audits production dependencies at high and above, so a dev-dependency advisory is out of scope by design. The reviewer could not read its own CI. Only postgres-durable-first runs against PostgreSQL, and XRAY_E2E_BINARY and XUI_SCALE_TEST are set by no job, so a dialect or migration change can carry a wall of green while the paths it touches never executed. That belongs to the verification bar, next to the rule that a behaviour claim needs a file:line citation, and "CI passed" now needs a run actually read. Also names the two house choices no linter defends: neither golangci-lint nor oxlint rejects a testify or Tailwind import. Both kinds of claim rot on a rename, so a test pins them the way repo-context.md's claims are already pinned - the CI jobs REVIEW.md names must exist in ci.yml, the skip gates it calls unset must stay unset, and the locale count must match the directory. The review itself moves from high to max effort, and the prompt records why it names REVIEW.md at all: the code-review skill reads CLAUDE.md on its own but not REVIEW.md, so dropping that clause would silently stop the file applying. Drops a CLAUDE.md reference to tools/seedperf/, which no longer exists - the review reads that file as project context, so a stale path there misleads it.
7.9 KiB
Review instructions
3x-ui is a Go (Gin + GORM) web panel that generates configuration, share links and subscriptions for other programs — Xray-core, mihomo, sing-box, mtg-multi — and is deployed by operators who upgrade in place. Judge findings by what breaks for those consumers and operators, not by style.
Severity
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. |
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.
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 🟣.
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.
What Important means here
- Security on the exposed surfaces:
internal/web/controller/, session and middleware code, the PUBLICinternal/sub/subscription server, and Xray config generation ininternal/xray/. - A state-changing inbound or client operation that bypasses
runtime.Runtime(internal/web/runtime/) and callsinternal/xray/api.godirectly, or dispatches from a controller or cron job. It passes every local test and silently breaks every multi-node deployment. - A schema or model change without a matching hand-written migration in
internal/database/db.go, one that behaves differently on SQLite and PostgreSQL, or one that loses or overwrites operator data on upgrade or rollback. There are no migration files and no down-migrations. - A change to what the panel emits on the wire — Xray config JSON, share
links, subscription/Clash YAML, mtg-multi TOML — that a downstream client
would reject or read differently, or that makes the three independent link
implementations (Go
internal/util/link/+internal/sub/, TSfrontend/src/lib/xray/, TSdocs/lib/xray/) diverge from one another. - Any edit to
.github/workflows/: this repository runs workflows with secrets against a public fork stream. Untrusted expression interpolation intorun:blocks, broadened permissions, weakened guards, or a job that executes pull-request code.
Always check
- A new
g.POST/g.GETininternal/web/controller/needs the whole chain: an entry infrontend/src/pages/api-docs/endpoints.ts, regenerated artefacts (make gen), any new API-boundary struct added toStructAllowintools/openapigen/main.go, andfrontend/public/openapi.jsoncopied todocs/public/openapi.jsonwith 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. - 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 != nilorlen(x) > 0, or going green by regenerating golden fixtures or Vitest snapshots. - No second way to do a thing already decided: Go tests are stdlib
testing(never testify), the panel is Ant Design (never Tailwind or shadcn). Neither golangci-lint nor oxlint forbids the import, so it passes CI clean.
Do not report
- Anything CI already enforces: golangci-lint and gofumpt, oxlint, format
and typecheck, govulncheck, and
npm audit --omit=dev --audit-level=high. A dev-dependency advisory is out of scope on purpose: it ships to nobody. - The contents of generated files (
frontend/src/generated/,frontend/public/openapi.json,docs/public/openapi.json) or lock files. Those files being STALE after a source change is reportable; their style is not. - Missing tests for getters, constants, renames or pure map lookups —
CLAUDE.mdrejects such tests outright. - A missing or unreferenced i18n key.
frontend/src/test/i18n-dead-keys.test.tspins the 13 locale files ininternal/web/translation/in both directions, so thefrontendjob is already red. Report the failing check, not the key.
A higher bar, not silence
Everything named under "What Important 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:
docs/— the standalone Fumadocs site, with its own CI and its own dependency tree.docs/lib/xray/is the exception and gets full scrutiny: it is the third link implementation.internal/web/translation/— the key set is CI's job and the wording of a translation is nobody's here.
Verification bar
- A claim about behaviour needs a
file:linecitation from this repository, not an inference from a name. - 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, file, identifier). If you cannot verify it, keep the finding but say explicitly that it is unverified instead of asserting it.
- "CI passed" is a claim too, and needs the same evidence: say it only of a
run you actually read. A green one proves less here than it looks — only
postgres-durable-firstruns against PostgreSQL,go-testandraceare SQLite, andXRAY_E2E_BINARYandXUI_SCALE_TESTare set by no job, so those tests have never run in CI at all. Where a change touches dialect, migration or Xray gRPC code that no job exercised, say it is unverified rather than repeating a green tick as proof.
Cap the volume
🔴 findings 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.
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.
After the first review of a pull request, report 🔴 findings 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.
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 coverage list — one line per area actually checked, naming what was examined and what it turned out to be, plus the head SHA and the size of the diff it covers. Say which claims could not be verified and why, including a check this environment blocked. Keep that coverage list under ten lines; it is evidence, not a retelling of the pull request.