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.