3x-ui/.github/claude/review-rubric.md

Review rubric and lane map

Shared by the four pull-request review lanes in .github/workflows/claude-bot.yml. The lane map below is here so it exists ONCE: when each lane carried its own copy of "mine / not mine", the four copies could quietly contradict each other and the same defect got reported twice or not at all.

Read this from the workspace checkout, which is the base revision and is trusted. NEVER read it from /tmp/head — a pull request controls that tree, and a fork that could supply this file could rewrite the rubric it is judged by.

Lane map — who owns what

Ownership is decided by WHAT YOU WOULD HAVE TO BE RIGHT ABOUT for the finding to be true, not by how bad the consequence would be.

lane	owns
Senior Developer	Correctness, edge cases, nil and empty handling, regressions. Layering and the runtime.Runtime dispatch rule. Security in code: authn/authz, input validation, injection, XSS, CSRF, SSRF, path traversal, secrets, unsafe defaults — weighted at internal/web/controller/, session and middleware, the PUBLIC internal/sub/ surface, and Xray config generation. Concurrency: races, deadlocks, goroutine and task leaks around the Xray and mtg-multi children, the cron jobs, the eventbus, the websockets. Performance. Maintainability and the 2-line comment cap. Frontend code quality. Every client-facing field name, encoding and hash choice the change emits.
Senior QA	internal/database/**, internal/database/model/**, internal/config/, internal/web/translation/**, tools/openapigen/, frontend/src/pages/api-docs/endpoints.ts, .github/workflows/**, Dockerfile*, docker-compose.yml, install.sh, x-ui.sh, DockerInit.sh, Makefile, CLAUDE.md, frontend/CLAUDE.md, docs/**, README*, SECURITY.md. Plus intent, upgrade safety, blast radius, backward compatibility of those contracts, operational impact, and labels.
Senior Tester	Test quality and coverage, what CI proved and what it did not, weak assertions, vacuous tests, snapshot and golden-fixture abuse.
Arbiter	Reconciliation, upstream wire-format resolution, and divergence BETWEEN the three link implementations.

Boundaries that are easy to get wrong

• Field names are the Developer's, never QA's — a config key, JSON tag, URI parameter, YAML key, TOML key, value encoding, hash choice, or which of two variables a field is populated from. However large the blast radius. If your finding is only true when one of those is wrong, it is the Developer's.
• QA outside its own files may report exactly ONE thing: a configuration that works on the base branch today behaves differently after this ships, with no operator action — and only when it can state (a) the concrete existing configuration, (b) what it does today, (c) what it does after. Otherwise drop it; the Developer has it.
• Destroying data IS QA's, even outside its files: regenerating a live key or UUID, overwriting a stored secret, resetting a traffic counter or expiry. That is blast radius, not correctness.
• docs/lib/xray/: QA reports the process omission ("it was not updated"). The Arbiter reports semantic divergence between the three implementations. The Developer reports whether the one in front of it emits the right thing.
• The Tester never opines on architecture, naming or what the code emits, and never restates a green CI job as a finding.

Severity — exactly one per finding, plain text, no emoji

level	means
Critical	security hole, data corruption or loss, crash, privilege escalation, authentication bypass, unrecoverable migration, or a fleet-wide outage path
High	likely production bug, incorrect behaviour on a common path, a breaking API or subscription-format change, a missing migration, a guaranteed CI break, or a significant performance problem
Medium	missing validation, an unhandled edge case, an undeclared behaviour change, documentation or OpenAPI drift, a maintainability problem, or an untested new code path
Low	minor readability, consistency, operational or documentation improvement
Suggestion	optional improvement with no correctness or release impact

Confidence — exactly one per finding

High, Medium, or Low. Reserve High for something CONFIRMED in the source and citable as file:line, or observed in real command output. Anything inferred, or resting on a detail you could not check, is Medium or Low.

Verdict — exactly one

Approve, Comment, or Request changes.

Finding block

Fields on their own lines:

Severity / Confidence / Category
Location: file:line as plain text, not a Markdown link
Problem: what is wrong
Why it matters: the practical runtime, security, operational or upgrade impact
Recommendation: the preferred fix

The Tester replaces Why it matters with Evidence: the command or CI job and the real output it read. A code example is optional and, if included, must be a plain fenced code block — never a suggestion block, since the Arbiter republishes the text.

Reporting discipline

• Report every problem, including Low and Suggestion. Never drop a finding because you are unsure: report it at Confidence: Low and say what would confirm it. Severity and confidence ARE the filter.
• Dropping a finding because it is not YOURS is different, and is exactly what the lane map asks for. A duplicate only costs the Arbiter a merge.
• Do not report the same issue twice, do not bikeshed style, and ignore pure-formatting changes unless they reduce readability. Ignore lock files and true vendor code; do NOT ignore test fixtures or generated files.
• If the diff is too large to cover completely, say so and name the files you did NOT review. A truncated review that does not admit it is worse than none.