xinyin025/3x-ui
1
0
Fork 0
mirror of https://github.com/MHSanaei/3x-ui.git synced 2026-09-16 15:17:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(ci): review pull requests with one senior-engineer role

Browse Source 
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
2026-09-08 18:46:30 +02:00
parent 47d2303334
commit acf3603dc8
4 changed files with 146 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files
REVIEW.md
+43 -35
View File
@@ -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.