This is an automated email from the ASF dual-hosted git repository.
shuke987 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/doris-skills.git
The following commit(s) were added to refs/heads/main by this push:
new 0476f3b feat(doris-repo-review): flag regressions per finding and
floor them at Major (#12)
0476f3b is described below
commit 0476f3b84adeee4df75bc7ed694980306f8c6ab8
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Mon Sep 14 14:26:39 2026 +0800
feat(doris-repo-review): flag regressions per finding and floor them at
Major (#12)
* feat(doris-repo-review): flag regressions per finding and floor them at
Major
The 2026-09-13 review of apache/doris#67900 found that the refactor had
dropped
the per-attempt reset of the Flight result-location flag, wrote it up
correctly,
and rated it Minor because the trigger was narrow and the fix was one line.
The
verdict stayed APPROVE, a PASS receipt was posted with the regression as its
first note, and the team's "fix only Blocker/Major" rule dropped it; the
next
reviewer sent it back as a must-fix.
---
README.md | 2 +-
skills/doris-repo-review/SKILL.md | 116 +++++++++++++++----
.../doris-repo-review/references/doc-templates.md | 102 ++++++++++++++++-
.../references/pr-comment-format.md | 19 +++-
skills/doris-repo-review/references/prompts.md | 89 +++++++++++++--
.../doris-repo-review/scripts/post-pass-comment.sh | 25 +++++
.../scripts/prepare-review-context.sh | 16 ++-
.../scripts/verify-review-docs.py | 123 ++++++++++++++++++++-
verify/README.md | 2 +-
verify/repo-review/post-comment.sh | 44 +++++++-
verify/repo-review/review-documents.sh | 98 +++++++++++++++-
11 files changed, 587 insertions(+), 49 deletions(-)
diff --git a/README.md b/README.md
index 4082c3c..094da27 100644
--- a/README.md
+++ b/README.md
@@ -34,7 +34,7 @@ skill from its `description`, so in practice you describe the
problem and the ri
| Skill | What it does | Use it when |
|---|---|---|
-| [`doris-repo-review`](skills/doris-repo-review/) | Contributor-side PR
review of a local `apache/doris` clone, running the same flow as the CI Code
Review Runner: worktree alignment, risk scan with premise checks, subagent
fan-out, shared ledger, mechanical coverage report, anchored EN + ZH review
documents kept per reviewed head, dismissals carried forward from earlier
reviews of the same PR, and — on a passing review — one machine-readable PASS
comment on the PR | Reviewing an `apach [...]
+| [`doris-repo-review`](skills/doris-repo-review/) | Contributor-side PR
review of a local `apache/doris` clone, running the same flow as the CI Code
Review Runner: worktree alignment, risk scan with premise checks, subagent
fan-out, shared ledger, mechanical coverage report, anchored EN + ZH review
documents kept per reviewed head with a per-finding category and a regression
flag judged against the merge base that floors an undeclared functional / data
/ resource / performance change at [...]
These skills read a Doris **source** checkout and never touch a cluster; the
cluster-side skills
never touch repository source. Nothing here builds Doris or runs tests. The
one GitHub write in
diff --git a/skills/doris-repo-review/SKILL.md
b/skills/doris-repo-review/SKILL.md
index 186b39a..6a3b148 100644
--- a/skills/doris-repo-review/SKILL.md
+++ b/skills/doris-repo-review/SKILL.md
@@ -47,10 +47,10 @@ $S/save-run-state.sh --ctx "$CTX" --verdict ... --findings
b,m,mi,n # step 11:
| `scripts/align-to-pr.sh` | Resolve the PR, diagnose how the current
directory relates to it, align it to the PR head |
| `scripts/prepare-review-context.sh` | Produce the authoritative diff,
new-side line ranges, required AGENTS.md list, existing comments, ledger
skeleton |
| `scripts/coverage-report.sh` | Mechanical check of which changed files no
ledger file has mentioned yet |
-| `scripts/verify-review-docs.py` | Validate commit, anchors, EN/ZH agreement,
verdict, findings, rounds, and convergence |
+| `scripts/verify-review-docs.py` | Validate commit, anchors, EN/ZH agreement,
verdict, findings, per-finding category and regression flag with the
category-aware `Major` floor, rounds, and convergence |
| `scripts/record-review-runtime.sh` | Record the qualified reviewer model,
effort, and exact commit |
| `scripts/review-runtime-policy.sh` | The exact model and effort allowlist
for a pipeline-equivalent review |
-| `scripts/post-pass-comment.sh` | Render and post the machine-readable PASS
comment; refuses everything that is not a pass |
+| `scripts/post-pass-comment.sh` | Render and post the machine-readable PASS
comment; refuses everything that is not a converged pass free of floored
regressions, and refuses to leave any other regression undisclosed |
| `scripts/save-run-state.sh` | Persist this run's merged ledger under the
stable per-PR state directory, so the next review inherits its dismissals |
| `references/prompts.md` | Subagent prompt templates (CI wording, carried
over verbatim) |
| `references/doc-templates.md` | Templates for both documents, anchor format,
verdict rule |
@@ -171,7 +171,7 @@ Output (under `$CTX`):
| File | Contents |
|---|---|
-| `meta.env` | PR info, base/head sha, diff range, `DOCS_ROOT`, `ALIGN_MODE`,
date, dirty-file count |
+| `meta.env` | PR info, `BASE_SHA` / `HEAD_SHA` / `MERGE_BASE`, `TARGET_AHEAD`
(target-branch commits since the merge base), diff range, `DOCS_ROOT`,
`ALIGN_MODE`, date, dirty-file count |
| `pr.diff` / `pr_changed_files.txt` / `pr_changed_files_status.txt` | The
authoritative diff and change list |
| `changed_line_ranges.txt` / `.tsv` | **New-side (post-change) line ranges**
- the source for anchors |
| `pr_commits.txt` / `pr_diffstat.txt` | Commit list and change size |
@@ -184,7 +184,11 @@ Output (under `$CTX`):
| `prior_runs/` | What earlier reviews of **this same PR** concluded - see
below |
If `BASE_SOURCE` is not `PR base sha (matches CI)`, the baseline differs from
CI's and the
-documents must say so.
+documents must say so. Two different "before" commits come out of this step
and they are not
+interchangeable: `BASE_SHA` is the target-branch tip the diff was cut against
and the commit the
+PASS receipt binds to; `MERGE_BASE` is where the PR actually branched off, and
it is the only
+correct answer to "what did this code do before the PR" (steps 4.1 and 6).
When `TARGET_AHEAD`
+is non-zero the two differ, and the script says so.
Record the qualified runtime selected in step 0 before reading source:
@@ -284,11 +288,13 @@ So each entry gets one more field:
Premise result: <confirmed | FALSE - item dismissed | cannot be checked
cheaply>
```
-Run every one of them **before** step 5. The check is nearly always a
one-liner against the base:
+Run every one of them **before** step 5. The check is nearly always a
one-liner against the merge
+base (`MERGE_BASE` in `meta.env` — not `BASE_SHA`, which is the target-branch
tip and may hold
+commits the PR never saw; `TARGET_AHEAD` says how many):
```bash
-git show "$BASE_SHA:path/to/File.java" | grep -n 'thing I think is new'
-git show "$BASE_SHA:path/to/File.java" | sed -n '120,140p'
+git show "$MERGE_BASE:path/to/File.java" | grep -n 'thing I think is new'
+git show "$MERGE_BASE:path/to/File.java" | sed -n '120,140p'
rg -n 'symbol' --files-with-matches # "only one caller" claims
```
@@ -369,10 +375,41 @@ candidate into `$CTX/ledger/main-merged.md`:
`pr_review_threads.md`.
- **Assign a status**: `accepted` / `dismissed_with_evidence` / `duplicated`.
A dismissal must come
with concrete code evidence, which goes verbatim into the document's
"Considered and Dismissed".
+- **Classify**: give every accepted candidate exactly one `Category` from the
closed vocabulary
+ of `references/doc-templates.md` — `functional-bug`, `functional-loss`,
`data-error`,
+ `resource-leak`, `performance`, `observability`, `test-coverage`, `wording`,
`maintainability`
+ — naming its *consequence*; the domain (concurrency, lifecycle,
compatibility, config, …) goes
+ in a parenthesis after it. The verifier rejects any other value.
+- **Flag regressions**: for every accepted candidate decide `Regression: yes |
no` against the
+ **merge base** — does HEAD behave differently from `git show
$MERGE_BASE:<path>` in a way the PR
+ body does not declare as intended? Not `$BASE_SHA`: when `TARGET_AHEAD` is
non-zero the target
+ branch has commits the PR never saw, and a fix that landed there is absent
at HEAD without the
+ PR having removed anything (a normal merge keeps it) — that is not a
regression, and a semantic
+ merge concern, if there is one, is its own row. Record the flag and its
evidence in
+ `main-merged.md`; it becomes the mandatory `Regression` line of the finding.
Then apply the
+ severity floors of `references/doc-templates.md`: a regression in
`functional-bug`,
+ `functional-loss`, `data-error`, `resource-leak`, or `performance` is **at
least `Major`**, and
+ for a PR presented as behaviour-preserving every undeclared differing cell
of the differential
+ table is such a regression. A regression in the other four categories is
judged by its
+ consequence, but below `Major` it needs a written *Severity rationale*
saying why the behaviour
+ change is acceptable. Severity is the consequence when it triggers, not the
probability; "narrow
+ trigger", "cloud only", "one-line fix" and "no wrong result" are not
discounts.
+ `verify-review-docs.py` rejects a floored regression rated `Minor`/`Nit` and
a non-floored one
+ without a rationale, so decide it here, not at document time.
+- **Severity challenge before a downgrade**: when you intend to rate a
candidate below what its
+ subagent proposed, or to dismiss a `functional-bug` / `functional-loss` /
`data-error` /
+ `resource-leak` candidate, first write the
+ *Severity rationale* (consequence when it triggers, who notices, what stays
leaked or wrong, why
+ that is not `Major`) into `main-merged.md`, then dispatch **one**
severity-challenge subagent with
+ the prompt of `references/prompts.md` section E — its only job is to argue
the higher severity
+ from primary sources. Finalize the severity only after it returns, and carry
the rationale into the
+ finding. It is one short agent; skipping it is how apache/doris#67900
shipped a `Minor` that the
+ next reviewer had to send back as a must-fix.
- **Fill the risk items back in**: update `Status` and `Final conclusion` for
every entry in
`00-main-risk-scan.md`.
-When this step ends, **no candidate may be left without a status**.
+When this step ends, **no candidate may be left without a status, a category,
a regression flag,
+or — where it was downgraded or is a non-floored regression below `Major` — a
severity rationale**.
### 6a. Run the coverage report at the end of every round
@@ -472,9 +509,13 @@ already exists, you are re-running against an unchanged
head: overwrite that one
- The document header must state: the PR link and state, the **head sha
actually reviewed**, the
diff range, the review directory (`WORKDIR`), and the `branch check` /
`commit check` results
from step 1.
-- Every finding must carry: a severity, a `path:line` anchor, a verbatim
snippet, and the four
- parts **what is wrong / why it happens / when it bites / suggested fix**.
Give a diff patch when
- the fix is small and self-contained; use prose only for architectural
problems.
+- Every finding must carry: a severity, a `Category` from the closed
vocabulary, a
+ `Regression: yes | no` line (with the merge-base evidence), a `path:line`
anchor, a verbatim
+ snippet, and the four parts **what is wrong / why it happens / when it bites
/ suggested fix** —
+ plus a **severity rationale** whenever the severity is below what a subagent
proposed, or a
+ regression in `observability` / `test-coverage` / `wording` /
`maintainability` is rated below
+ `Major`. Give a diff patch when the fix is small and self-contained; use
prose only for
+ architectural problems.
- The "Critical Checkpoints" table needs a conclusion per row; mark a
non-applicable one `n/a` with
a half-line reason - **never delete the row**.
- "Response to Review Focus" answers each of the user's focus points.
@@ -495,22 +536,31 @@ python3 $S/verify-review-docs.py --ctx "$CTX" \
--doc review-docs/pr-<N>-review.zh.md
```
-It validates the reviewed head, anchors, EN/ZH finding order and severity,
verdict, rounds, and
-convergence. Fix every error before continuing.
+It validates the reviewed head, anchors, EN/ZH finding order, severities,
categories and
+regression flags, the category-aware regression floor, the rationale a
non-floored regression
+below `Major` needs, verdict, rounds, and convergence. Fix every error before
continuing — and if
+the error is "is a regression in category … but rated Minor", the fix is the
severity (and with it
+the verdict), not the flag and not the category.
---
## 10. Post the PASS comment to the PR
**Only when the verdict is APPROVE**, the review converged, and step 9 passed.
A
-REQUEST_CHANGES or non-converged review posts nothing - say so in the closing
report and stop.
+REQUEST_CHANGES or non-converged review posts nothing - say so in the closing
report and stop. A
+regression in a floored category can only exist inside a REQUEST_CHANGES, so
it never reaches the
+poster; a `Regression: yes` in `observability` / `test-coverage` / `wording` /
`maintainability`
+rated `Minor` / `Nit` does reach it, and the receipt must disclose every one
of them.
-Write optional notes and run the poster once:
+Write the notes and run the poster once:
```bash
-# At most 5 bullets, each anchored where it can be. Skip the file when there
is nothing to say.
+# At most 5 bullets, each anchored where it can be. Every finding flagged
`Regression: yes`
+# comes first, named with its ID and category; the poster refuses to post
without notes when
+# such a finding exists. Skip the file only when there is nothing to say and
no regression.
cat > "$CTX/pr-comment-notes.md" <<'EOF'
-- `fe/fe-core/src/main/java/org/apache/doris/X.java:214` — <what the
maintainer should know>
+- F-01 (observability, undeclared behaviour change)
`fe/fe-core/src/main/java/org/apache/doris/X.java:214` — <what changed and why
it is acceptable>
+- `fe/fe-core/src/main/java/org/apache/doris/Y.java:88` — <what the maintainer
should know>
EOF
$S/post-pass-comment.sh --ctx "$CTX" \
@@ -518,8 +568,8 @@ $S/post-pass-comment.sh --ctx "$CTX" \
```
The poster reads model, effort, and commit from `review-runtime.json`; it
invokes
-`verify-review-docs.py` itself for verdict, findings, rounds, and convergence;
then it rechecks the
-live PR head. A refusal is final. The normal flow posts immediately;
`--dry-run` exists only for
+`verify-review-docs.py` itself for verdict, findings, regression counts,
rounds, and convergence;
+then it rechecks the live PR head. A refusal is final. The normal flow posts
immediately; `--dry-run` exists only for
maintainer testing. Same account plus same commit updates the existing
comment, while a new commit
creates a new one. Keep the schema in `references/pr-comment-format.md`
machine-generated.
@@ -530,16 +580,19 @@ creates a new one. Keep the schema in
`references/pr-comment-format.md` machine-
Tell the user:
1. The paths of both documents, the verdict (REQUEST_CHANGES / APPROVE), the
finding count per
- severity, and whether the rounds converged in the sense of step 7 - "the
verdict was settled at
- round N" if they did, and which condition failed if they did not. A run
that ended with only
- Minor/Nit still arriving **converged**; do not report it as a failure.
+ severity, every finding flagged `Regression: yes` with its category (these
are the behaviour
+ changes the PR body never declared; the author should declare or fix them),
and whether the
+ rounds converged in the sense of step 7 - "the verdict was settled at round
N" if they did, and
+ which condition failed if they did not. A run that ended with only
Minor/Nit still arriving
+ **converged**; do not report it as a failure.
1a. **What this run inherited**, when `prior_runs/` was not empty: which heads
were reviewed
before, how many of their dismissals were carried forward, and which of
their accepted findings
are fixed at this head. If no prior run existed, say that this is the first
review of this PR.
2. The `branch check` / `commit check` results - especially `ahead:N`
(unpushed commits that were
not reviewed).
3. **What happened to the PASS comment**: the URL when one was posted or
updated, or the reason
- nothing was posted (REQUEST_CHANGES, non-convergence, unqualified runtime,
or moved PR head).
+ nothing was posted (REQUEST_CHANGES, non-convergence, unqualified runtime,
an undisclosed
+ regression, or moved PR head).
4. **Where the current directory now stands**: with `ALIGN_MODE=switched` it
is detached on the PR
head, and `git checkout <PREV_REF>` restores it. **Do not switch back
automatically** - the user
may still want to read the code.
@@ -573,7 +626,7 @@ Tell the user:
| Fetch existing inline threads (30 threads / 1200 chars) | Same jq |
| Text after `/review` = review focus | Free text after the PR URL → `--focus`
|
| Single-file ledger with sections | A `ledger/` directory, one file per owner
|
-| Main risk scan → 1-3 full-review subagents + risk-focused → merge → ≤3
rounds | Same shape, three local additions: every risk item carries a premise
check the main agent runs before dispatch (4.1); round 1 goes out in two waves
so the ledger can deduplicate (5.1); a round converges on **severity plus
coverage**, not on "no new candidates at all" (7) |
+| Main risk scan → 1-3 full-review subagents + risk-focused → merge → ≤3
rounds | Same shape, four local additions: every risk item carries a premise
check the main agent runs before dispatch (4.1); round 1 goes out in two waves
so the ledger can deduplicate (5.1); every finding carries a category and a
regression flag against the merge base, a regression in a functional / data /
resource / performance category is floored at `Major`, and a downgrade needs a
written rationale plus a sever [...]
| CI reviews one push in isolation | Earlier runs of the same PR are loaded
from a stable state directory and read as input (2.1); documents are named by
head sha and never overwritten (9) |
| `gh pr review` / Reviews API posting inline comments | **Two `review-docs/`
documents (EN + ZH) with `path:line` anchors** |
| CI's review verdict is visible on the PR itself | On a qualified converged
pass, one automatic `doris-repo-review/v1` comment bound to the exact commit;
otherwise nothing |
@@ -587,6 +640,21 @@ Tell the user:
for as long as you keep looking. What has to stop moving is the *verdict* -
see step 7. Two runs
in a row reporting "did not converge" while the blocking findings were
settled in round 1 is a
broken criterion, not a deep PR.
+- **Discounting a regression because its trigger is narrow or its fix is one
line.** Severity is
+ what happens when it triggers, not how often. A per-attempt reset that a
refactor dropped is a
+ `functional-loss` regression whether it needs "Flight + replan + a failing
retry" or not; rate it
+ `Major`, or the PASS receipt will sit on top of a known behaviour change and
the fix threshold of
+ the team ("only Blocker/Major") will silently drop it — apache/doris#67900,
2026-09-13.
+- **Filing a real behaviour change under a soft category to dodge the floor.**
A metric that
+ stopped being emitted is `observability`; a reset that stopped happening is
`functional-loss`
+ even when the only visible symptom today is a log line. The category names
the consequence,
+ not the file the code lives in.
+- **Comparing HEAD with the target-branch tip instead of the merge base.**
When `TARGET_AHEAD` is
+ non-zero, `git show $BASE_SHA:<path>` shows code the PR branch has never
contained; a fix that
+ landed there after the PR branched off is "missing" at HEAD without the PR
having removed
+ anything, and a normal merge keeps it. That is not a regression and must not
become a `Major`.
+ Regression evidence is `git show $MERGE_BASE:<path>`; `BASE_SHA` is for the
diff range and the
+ receipt.
- **Dispatching a subagent on a premise you never checked.** The premise check
of step 4.1 costs
one command; skipping it costs a whole agent, and the agent comes back
having proved you wrong
rather than having reviewed anything.
diff --git a/skills/doris-repo-review/references/doc-templates.md
b/skills/doris-repo-review/references/doc-templates.md
index a9ce34a..6a96e51 100644
--- a/skills/doris-repo-review/references/doc-templates.md
+++ b/skills/doris-repo-review/references/doc-templates.md
@@ -13,9 +13,9 @@ permanent, and what it destroys is the only record of what an
earlier review alr
and dismissed. Re-running against the *same* head overwrites that head's pair,
and only that one.
The ZH document is a real Chinese review, not a machine translation of the EN
one: same facts,
-same anchors, same IDs, but idiomatic Chinese. Identifiers, file paths, log
messages, config
-names, code snippets, and the severity words (`Blocker` / `Major` / `Minor` /
`Nit`) stay in
-their original form in both documents.
+same anchors, same IDs, same severities, categories and regression flags, but
idiomatic Chinese.
+Identifiers, file paths, log messages, config names, code snippets, and the
severity words
+(`Blocker` / `Major` / `Minor` / `Nit`) stay in their original form in both
documents.
## Anchor format (mandatory, both documents)
@@ -50,6 +50,88 @@ without a compat path, or a broken build/test contract.
`Major` = real defect or
guarantee that will bite in production or during upgrade. `Minor` = worth
fixing, not urgent.
`Nit` = style or wording.
+## Category, regression flag, and severity floors
+
+Every finding carries two more mandatory lines right after its severity — its
**category** and
+whether it is a **regression**:
+
+```markdown
+- **Category**: functional-loss (lifecycle) <!-- EN; ZH: -
**类别**：功能缺失（生命周期） -->
+- **Regression**: yes <!-- EN; ZH: - **回归**：是 /
否 -->
+```
+
+### Category
+
+The category names the *consequence* of the finding, from this closed
vocabulary. The
+verifier rejects any other value, so an unrecognised category cannot slip past
the floor. One
+class per finding; the domain that produced it (concurrency, lifecycle,
compatibility, config,
+…) goes in an optional parenthetical after the class.
+
+| Category (EN / ZH) | What belongs here | Regression floor |
+|---|---|---|
+| `functional-bug` / `功能缺陷` | the code does the wrong thing: crash, exception,
hang, deadlock, wrong branch, wrong control flow, a check that fires when it
should not | at least `Major` |
+| `functional-loss` / `功能缺失` | something the base did, or the PR claims, no
longer happens: a dropped reset, a removed compat path, a check that no longer
runs, a sibling path left unchanged, a rolling-upgrade path that breaks | at
least `Major` |
+| `data-error` / `数据错误` | wrong query results, lost, duplicated or corrupted
data, an edit log or persisted state that does not replay | at least `Major` |
+| `resource-leak` / `资源泄露` | memory, file descriptors, threads, connections,
locks, temp files, tablets or statements not released | at least `Major` |
+| `performance` / `性能降低` | slower, more memory / CPU / IO, a lost
optimisation, an unbounded loop or allocation | at least `Major` |
+| `observability` / `可观测性` | logs, VLOG, metrics, profile fields, the
diagnostic content of an error message | judged normally |
+| `test-coverage` / `测试覆盖` | missing, weakened, or non-negative tests; a gate
a change would leave green | judged normally |
+| `wording` / `措辞` | comments, docs, messages, naming | judged normally |
+| `maintainability` / `可维护性` | duplication, dead code, structure, an
unexplained special case | judged normally |
+
+Spelling is compared case-insensitively with spaces, hyphens and underscores
removed, and the
+ZH forms `功能性bug` / `功能性缺失` / `资源泄漏` / `性能` are accepted as aliases. EN and ZH
+documents must agree on the category of every finding.
+
+### Regression
+
+`yes` means the behaviour at HEAD differs from the **merge base** in a way the
PR body does not
+declare as intended — the cell of the differential table that the author did
not ask for. `no`
+means the defect was already there (the PR merely exposes, documents, or moves
it) or the change
+is the one the PR set out to make. The evidence is `git show
<MERGE_BASE>:<path>`; name it in
+the finding.
+
+The comparison point is `MERGE_BASE` (from `meta.env`), **not** `BASE_SHA`.
`BASE_SHA` is the
+tip of the target branch — the snapshot the diff was cut against and the
commit the PASS receipt
+binds to — and it may carry commits the PR branch has never seen
(`TARGET_AHEAD` in `meta.env`
+counts them). A fix that landed on the target branch after the PR branched off
is absent at
+HEAD without the PR having removed anything, and a normal merge keeps it;
comparing against
+`BASE_SHA` would report it as a "dropped" behaviour and a Major regression
that does not exist.
+Whether the PR still merges cleanly *semantically* against a target branch
that has moved on is
+a real question, but a different one — raise it, when it applies, as its own
finding or
+dismissal row, never as `Regression: yes`.
+
+`verify-review-docs.py` rejects a finding without the line and rejects EN/ZH
documents whose
+flags disagree.
+
+### Floors
+
+The flag and the category together drive a floor that the verifier enforces
and that no
+amount of "but it is narrow" overrides:
+
+| Condition | Floor |
+|---|---|
+| `Regression: yes` in `functional-bug`, `functional-loss`, `data-error`,
`resource-leak`, or `performance` | at least `Major` — the verifier rejects
`Minor` / `Nit` |
+| `Regression: yes` in `observability`, `test-coverage`, `wording`, or
`maintainability` | judged normally by consequence; below `Major` the finding
**must** carry a **Severity rationale** paragraph saying why the behaviour
change is acceptable (the verifier checks that it is there), and the PASS
receipt must name it in its notes |
+| a PR presented as behaviour-preserving (`[refactor]`, `[chore]`, "Behavior
changed: No", or any wording that claims equivalence): **every** differing cell
of the D1 differential table that the PR body does not name as intended |
`Regression: yes`, classified by its consequence like any other finding — a
cell that differs in what the code *does* (as opposed to what it logs or says)
is `functional-loss` / `functional-bug` / `data-error`, hence at least `Major`.
The reviewer's job is to f [...]
+
+Severity is the consequence *when* the finding triggers — what is wrong,
leaked, lost, or
+silently skipped — never the probability that it triggers. A narrow trigger
(one protocol, cloud
+only, a retry that has to fail twice), a rare environment, or a one-line fix
is not a discount. The
+review of apache/doris#67900 rated a per-attempt reset that the PR had dropped
as `Minor` because
+the trigger was "Flight + replan + failing retry" and the fix was one line;
the connector statement
+scope it leaked was raised again by the next reviewer as a must-fix. Under
this table that finding
+is `functional-loss` (the reset the base performed no longer happens),
`Regression: yes`, and
+therefore at least `Major` — the case this section exists to prevent.
+
+**Downgrading needs a written rationale.** When the main agent rates a
candidate below what the
+subagent proposed, or dismisses a `functional-bug` / `functional-loss` /
`data-error` /
+`resource-leak` candidate, the finding (or the "Considered and Dismissed" row)
must carry a
+**Severity rationale** paragraph: the consequence analysis — what happens when
it triggers, who
+notices, what is left leaked or wrong — and explicitly why that is not
`Major`. "Narrow",
+"one-line fix", or "no wrong result" alone is not a rationale. SKILL.md step 6
additionally
+dispatches a severity-challenge subagent before such a downgrade is final.
+
---
## English template
@@ -84,8 +166,9 @@ guarantee that will bite in production or during upgrade.
`Minor` = worth fixing
### F-01 · <one-line title>
- **Severity**: Blocker
+- **Category**: functional-bug (concurrency) <!-- one of functional-bug /
functional-loss / data-error / resource-leak / performance / observability /
test-coverage / wording / maintainability; the domain goes in the parenthesis.
Mandatory. -->
+- **Regression**: yes <!-- yes = behaviour differs from the merge base and the
PR body does not declare it intended; no = pre-existing or intended. Mandatory;
yes in the first five categories is at least Major. -->
- **Where**: `path/to/File.java:412-430`
-- **Category**: correctness / concurrency / lifecycle / compatibility / config
/ performance / observability / test coverage
```java
<verbatim snippet from the anchor>
@@ -97,6 +180,12 @@ guarantee that will bite in production or during upgrade.
`Minor` = worth fixing
**When it bites.** <a concrete trigger scenario — who calls what, in which
order, with which data>
+**Severity rationale.** <required whenever the severity is below what a
subagent proposed, a
+regression is rated at the floor rather than Blocker, or a regression in
observability /
+test-coverage / wording / maintainability is rated below Major (say why the
behaviour change is
+acceptable): the consequence when it triggers and why that is (not) worse.
Omit for findings rated
+as proposed.>
+
**Suggested fix.**
```diff
@@ -184,8 +273,9 @@ out. This is what makes the review auditable — do not
silently drop a concern.
### F-01 · <一句话标题>
- **等级**：Blocker
+- **类别**：功能缺陷（并发） <!-- 功能缺陷 / 功能缺失 / 数据错误 / 资源泄露 / 性能降低 / 可观测性 / 测试覆盖 / 措辞 /
可维护性 之一；所属领域写在括号里。必填。 -->
+- **回归**：是 <!-- 是 = HEAD 的行为与 merge base 不同且 PR 描述没有声明这是有意的；否 =
既有问题或有意改动。必填；前五类里的回归至少是 Major。 -->
- **位置**：`path/to/File.java:412-430`
-- **类别**：正确性 / 并发 / 生命周期 / 兼容性 / 配置 / 性能 / 可观测性 / 测试覆盖
```java
<锚点处原样摘录的代码>
@@ -197,6 +287,8 @@ out. This is what makes the review auditable — do not
silently drop a concern.
**什么时候会踩到。** <具体触发场景：谁在什么顺序上调用了什么、数据长什么样>
+**定级理由。** <当等级低于子 agent 提议、回归只定在 Major 下限而非 Blocker、或可观测性 / 测试覆盖 / 措辞 /
可维护性类的回归定在 Major 以下（说明为什么这个行为变化可以接受）时必填：触发后的后果是什么、为什么（不）更严重。等级与提议一致时可省略。>
+
**修改建议。**
```diff
diff --git a/skills/doris-repo-review/references/pr-comment-format.md
b/skills/doris-repo-review/references/pr-comment-format.md
index edd51d2..c16d755 100644
--- a/skills/doris-repo-review/references/pr-comment-format.md
+++ b/skills/doris-repo-review/references/pr-comment-format.md
@@ -1,7 +1,10 @@
# PASS comment format (`doris-repo-review/v1`)
-When a review passes, the skill posts **one comment** on the PR from the
locally authenticated
-`gh` account. The comment is written by `scripts/post-pass-comment.sh`, never
typed by hand: the
+When a review passes — a converged `APPROVE` with no Blocker/Major finding and
no regression in
+a category that carries the Major floor (`functional-bug`, `functional-loss`,
`data-error`,
+`resource-leak`, `performance`) — the skill posts **one comment** on the PR
from the locally
+authenticated `gh` account. A regression in any other category can stay
`Minor`/`Nit` and still
+pass, but the receipt then has to name it in the notes; the poster refuses to
post without them. The comment is written by `scripts/post-pass-comment.sh`,
never typed by hand: the
agent supplies optional notes, while verified documents supply the structured
result. Anything that reads the
comment back — a script, a dashboard, another agent — depends on that layout
being fixed.
@@ -55,7 +58,7 @@ receipt for the matching commit; it is not a human Apache
approval.</sub>
| `status` | `PASS` | the only value ever posted; `CHANGES_REQUESTED` is
reserved, not used |
| `pr` | `owner/repo#N` | `meta.env` |
| `commit` | 40-hex | `HEAD_SHA` — the exact commit reviewed |
-| `base` | 40-hex | `BASE_SHA` — with `commit` this reproduces the reviewed
diff |
+| `base` | 40-hex | `BASE_SHA` — with `commit` this reproduces the reviewed
diff (`git diff base...commit`); the regression flags were judged against the
merge base of the two |
| `reviewed_at` | ISO-8601, minute precision, with offset | when the comment
was rendered |
| `reviewer` | GitHub login | `gh api user`, falling back to GraphQL `viewer`
and `gh auth status` |
| `model` | exact eligible model id | `review-runtime.json`, recorded before
source review |
@@ -115,12 +118,16 @@ gh api repos/apache/doris/issues/66807/comments
--paginate \
The comment is not a substitute for the review documents; it is the receipt. A
note earns its
place only when a maintainer would act differently without it:
+- **every finding flagged `Regression: yes`** — an undeclared behaviour change
in
+ `observability` / `test-coverage` / `wording` / `maintainability` that
stayed below `Major` —
+ first, named by ID and category with its `path:line` anchor and why it is
acceptable; the poster
+ refuses a receipt that would leave one undisclosed;
- a residual risk that did not reach `Minor`, with a `path:line` anchor;
- coverage the review could not reach (no build, no cluster, no test run);
- a backport or upgrade consideration the PR itself does not state;
A non-converged review records what remained open in the local documents and
posts no receipt.
-Not this: restating what the PR does, listing every `Minor`/`Nit` (they live
in the documents),
-praise, or anything that reads as an official Apache sign-off. `_None._` is a
perfectly good
-notes section.
+Not this: restating what the PR does, listing every `Minor`/`Nit` (they live
in the documents;
+only the ones flagged as regressions belong here), praise, or anything that
reads as an official
+Apache sign-off. `_None._` is a perfectly good notes section when no finding
is a regression.
diff --git a/skills/doris-repo-review/references/prompts.md
b/skills/doris-repo-review/references/prompts.md
index d3ae4b8..18f9664 100644
--- a/skills/doris-repo-review/references/prompts.md
+++ b/skills/doris-repo-review/references/prompts.md
@@ -11,7 +11,7 @@ Substitute before use:
|---|---|
| `{CTX}` | absolute review context directory |
| `{REPO_ROOT}` | absolute repo root |
-| `{BASE_SHA}` / `{HEAD_SHA}` | from `{CTX}/meta.env` |
+| `{BASE_SHA}` / `{HEAD_SHA}` / `{MERGE_BASE}` | from `{CTX}/meta.env` |
| `{ROUND}` | 1-based round number |
| `{AGENT_ID}` | short slug, e.g. `r1-fe-spi` |
| `{FOCUS}` | this subagent's assigned coverage |
@@ -40,9 +40,14 @@ Authoritative PR context (do not obtain the diff or the
changed-path list any ot
- Earlier reviews of this PR : {CTX}/prior_runs/ (may be absent - then
this is the first)
- Coverage checklist : {CTX}/coverage_checklist.tsv
- Diff range : {BASE_SHA}...{HEAD_SHA} (three-dot, from
the merge base)
+- Merge base : {MERGE_BASE}
These were generated with `git diff {BASE_SHA}...{HEAD_SHA}` in this worktree.
The base SHA
-identifies the target-branch snapshot and is not necessarily the diff's left
endpoint.
+identifies the target-branch snapshot and is not necessarily the diff's left
endpoint; the
+diff's left endpoint is the merge base. Whenever you need "what did this file
look like before
+the PR", read `git show {MERGE_BASE}:<path>` - never `git show
{BASE_SHA}:<path>`: the target
+branch may carry commits the PR branch has not seen, and their absence at HEAD
is not something
+the PR did.
Before reading any file whose exact path is not already confirmed by
pr_changed_files.txt,
pr.diff, or a previous successful command output, you MUST first run `rg
--files` to confirm
@@ -65,6 +70,17 @@ Ledger rules:
globally unique: prefix them with your agent id, e.g. `{AGENT_ID}-01`.
- If a candidate overlaps one that already exists in the ledger, record it in
your own file
with a duplicate note naming the existing candidate ID instead of restating
it.
+- Every candidate carries a `Category:` - exactly one of functional-bug,
functional-loss,
+ data-error, resource-leak, performance, observability, test-coverage,
wording,
+ maintainability - naming the consequence, with the domain (concurrency,
lifecycle,
+ compatibility, config, ...) in a parenthesis after it.
+- Every candidate carries `Regression: yes | no` — does HEAD behave
differently from the merge
+ base (`git show {MERGE_BASE}:<path>`) in a way the PR body does not declare
as intended? Cite
+ the merge-base lines. Propose the severity from the consequence when it
triggers, never from
+ how narrow the trigger is: a regression in functional-bug, functional-loss,
data-error,
+ resource-leak, or performance is at least Major, and for a PR presented as
+ behaviour-preserving every undeclared differing cell of your differential
table is such a
+ regression.
Line-number rule (this run has no GitHub inline comments, so anchors are the
only pointer):
- Every candidate MUST carry `Path:` plus `Line:` using NEW-SIDE (post-change)
line numbers,
@@ -176,12 +192,15 @@ A subagent told only "review your slice" invents a
method, and the method it inv
techniques below are the ones that actually produced findings. Name the one
you want, and say what
it should be applied to.
-**D1. Differential against the base.** *Do not read the diff. Reconstruct both
sides.* Extract the
-pre-change files with `git show {BASE_SHA}:<path>`, build the old and the new
behaviour tables
-yourself - one row per (input, condition) the code distinguishes - and list
every cell that
-differs. Then classify each differing cell as intended (name the commit that
says so) or as a
-regression. This is what catches a defect whose changed line is *correct*: the
line is right, the
-comment explaining it is right, and the consequence two modules away is wrong.
+**D1. Differential against the merge base.** *Do not read the diff.
Reconstruct both sides.*
+Extract the pre-change files with `git show {MERGE_BASE}:<path>` (the merge
base, not the
+target-branch tip: a commit the target branch gained after the PR branched off
is not a cell the
+PR changed), build the old and the new behaviour tables yourself - one row per
(input, condition)
+the code distinguishes - and list every cell that differs. Then classify each
differing cell as
+intended (name the commit that says so) or as a regression, and name the
consequence of each
+regression cell (functional-loss, functional-bug, data-error, ...). This is
what catches a defect
+whose changed line is *correct*: the line is right, the comment explaining it
is right, and the
+consequence two modules away is wrong.
**D2. What did this switch turn on?** When a change fixes something that was
silently not working,
the path it revives has never been exercised. Ask: what else is on that path,
what has never run,
@@ -216,3 +235,57 @@ find the two or three most load-bearing "not a bug"
conclusions in the ledger an
from primary sources. Then ask which *kinds* of check were never run at all -
persistence and
replay, upgrade *and downgrade*, CI gates, licence obligations, the thrift
surface - and run the
ones that apply.
+
+---
+
+## E. Severity-challenge subagent (one per intended downgrade, SKILL.md step 6)
+
+Append to the shared preamble. Dispatch it **before** finalizing a severity
that is lower than the
+subagent proposed, or before dismissing a functional-bug / functional-loss /
data-error /
+resource-leak candidate. It is
+deliberately one-sided: its job is to make the strongest case for the higher
severity so the main
+agent's rationale has been argued against by someone before it is written into
the documents.
+
+```
+You are a SEVERITY-CHALLENGE subagent. You are not reviewing a slice and you
are not looking for
+new findings. One candidate is about to be rated lower than proposed, or
dismissed, and your only
+job is to argue the higher severity from primary sources - then say honestly
whether the argument
+holds.
+
+Candidate ID : {CANDIDATE_ID}
+Claim : {CLAIM}
+Anchor(s) : {ANCHORS}
+Proposed by subagent : {PROPOSED_SEVERITY}
+Main agent intends : {INTENDED_SEVERITY_OR_DISMISSAL}
+Main agent's rationale: {RATIONALE}
+Category : {CATEGORY}
+Regression flag : {REGRESSION_FLAG} (evidence: {REGRESSION_EVIDENCE})
+
+Do this, in order:
+1. Re-derive the consequence from the code, not from the ledger: when the
trigger happens, what is
+ wrong, leaked, lost, or silently skipped? Who notices, and when? What is
the blast radius (one
+ session, one query, the FE, persisted state)? Cite `path:line`.
+2. Check the regression flag yourself with `git show {MERGE_BASE}:<path>`: did
the merge base
+ behave differently? If HEAD differs and the PR body does not declare it,
the flag is `yes`. Then
+ check the category: is the consequence really observability / test-coverage
/ wording /
+ maintainability, or is something the base *did* no longer happening
(functional-loss), or
+ wrong (functional-bug / data-error), leaked (resource-leak), or slower
(performance)? A `yes` in
+ one of those five is at least Major - say so even if the main agent's
rationale never mentions
+ it. A difference that exists only because the target branch moved on after
the PR branched off
+ is not a regression; say that too.
+3. Attack the rationale: is it about probability ("narrow", "cloud only",
"needs two failures") or
+ about cost ("one-line fix") rather than about consequence? Those are not
severity arguments.
+ Is "no wrong result" true for every consumer of the leaked or skipped state?
+4. Name the strongest counter-argument to your own case and say whether it
survives.
+
+Write to {CTX}/ledger/sub-{ROUND}-challenge-{CANDIDATE_ID}.md and return
exactly:
+- the severity you would assign, and the category and regression flag you
verified,
+- the one-paragraph consequence analysis with citations,
+- UPHOLD_DOWNGRADE when the main agent's rationale survives your best case,
otherwise
+ RAISE_SEVERITY with the floor that applies.
+```
+
+The main agent records the outcome in `main-merged.md` under the candidate
(`Severity challenge:
+UPHOLD_DOWNGRADE | RAISE_SEVERITY, <one line>`), and the finding's **Severity
rationale** paragraph
+must survive that challenge, not merely precede it.
+
diff --git a/skills/doris-repo-review/scripts/post-pass-comment.sh
b/skills/doris-repo-review/scripts/post-pass-comment.sh
index 0e769e7..60a3224 100755
--- a/skills/doris-repo-review/scripts/post-pass-comment.sh
+++ b/skills/doris-repo-review/scripts/post-pass-comment.sh
@@ -10,6 +10,9 @@
#
# Runtime fields come from review-runtime.json. Review fields come directly
from
# verify-review-docs.py. The agent supplies notes, never receipt fields or
format.
+# Refuses on anything that is not a converged APPROVE with zero Blocker/Major
findings and
+# zero regressions in a category that carries the Major floor; a regression in
any other
+# category (rated Minor/Nit) is allowed only when the notes disclose it.
#
# Rendered body: <ctx>/pr-comment.md Posted URL: <ctx>/pr-comment.url
set -euo pipefail
@@ -82,6 +85,14 @@ F_BLOCKER="$(jq -er '.findings.blocker | numbers'
<<<"$RESULT_JSON")"
F_MAJOR="$(jq -er '.findings.major | numbers' <<<"$RESULT_JSON")"
F_MINOR="$(jq -er '.findings.minor | numbers' <<<"$RESULT_JSON")"
F_NIT="$(jq -er '.findings.nit | numbers' <<<"$RESULT_JSON")"
+F_REGRESSIONS="$(jq -er '.regressions | numbers' <<<"$RESULT_JSON")" || {
+ echo "ERROR: the verifier did not report a regression count; update
verify-review-docs.py." >&2
+ exit 2
+}
+F_FLOORED_REGRESSIONS="$(jq -er '.floored_regressions | numbers'
<<<"$RESULT_JSON")" || {
+ echo "ERROR: the verifier did not report a floored-regression count;
update verify-review-docs.py." >&2
+ exit 2
+}
[ "$RESULT_COMMIT" = "$NORMALIZED_HEAD_SHA" ] || { echo "ERROR: review
documents target another commit." >&2; exit 2; }
[ "$VERDICT" = "APPROVE" ] || { echo "ERROR: review verdict is $VERDICT.
Nothing was posted." >&2; exit 1; }
@@ -90,6 +101,13 @@ F_NIT="$(jq -er '.findings.nit | numbers'
<<<"$RESULT_JSON")"
echo "ERROR: Blocker or Major findings cannot produce a PASS comment." >&2
exit 1
}
+# The verifier already floors a functional / data / resource / performance
regression at Major,
+# so this only fires when the two scripts disagree; a PASS receipt must never
sit on top of an
+# undeclared behaviour change in one of those categories.
+[ "$F_FLOORED_REGRESSIONS" -eq 0 ] || {
+ echo "ERROR: $F_FLOORED_REGRESSIONS finding(s) are regressions in a
category that is at least Major; a PASS receipt cannot be issued. Nothing was
posted." >&2
+ exit 1
+}
# -------------------------------------------------------------------------
notes
NOTES_BODY="_None._"
@@ -116,6 +134,12 @@ if [ -n "$NOTES_FILE" ]; then
NOTES_BODY="$(cat "$NOTES_FILE")"
fi
fi
+# A regression outside the floored categories stays Minor/Nit, so the verdict
is still APPROVE -
+# but the receipt must not be silent about a behaviour change the PR body
never declared.
+if [ "$F_REGRESSIONS" -gt 0 ] && [ "$NOTE_COUNT" -eq 0 ]; then
+ echo "ERROR: $F_REGRESSIONS finding(s) are undeclared behaviour changes
(Regression: yes); the receipt must disclose each of them - list them as
--notes-file bullets. Nothing was posted." >&2
+ exit 1
+fi
# ------------------------------------------------------------------ live PR
state
PR_TSV="$(gh api "repos/${UPSTREAM_REPO}/pulls/${PR_NUMBER}" --jq '[.head.sha,
.state] | @tsv')" || {
echo "ERROR: cannot read ${UPSTREAM_REPO}#${PR_NUMBER}." >&2; exit 1; }
@@ -226,6 +250,7 @@ echo "reviewer : ${REVIEWER}"
echo "model : ${MODEL} (effort ${EFFORT})"
echo "findings : blocker=${F_BLOCKER} major=${F_MAJOR} minor=${F_MINOR}
nit=${F_NIT}"
echo "notes : ${NOTE_COUNT}"
+[ "$F_REGRESSIONS" -eq 0 ] || echo "disclosed : ${F_REGRESSIONS} undeclared
behaviour change(s) in non-blocking categories, named in the notes"
if [ "$ACTION" = "update" ]; then
echo "action : UPDATE the existing comment ${EXISTING_ID} (same commit)"
elif [ -n "$LAST_ID" ]; then
diff --git a/skills/doris-repo-review/scripts/prepare-review-context.sh
b/skills/doris-repo-review/scripts/prepare-review-context.sh
index b75ff36..c0101a4 100755
--- a/skills/doris-repo-review/scripts/prepare-review-context.sh
+++ b/skills/doris-repo-review/scripts/prepare-review-context.sh
@@ -154,6 +154,10 @@ fi
MERGE_BASE="$(git merge-base "$BASE_SHA" "$HEAD_SHA")"
DIFF_RANGE="${BASE_SHA}...${HEAD_SHA}"
+# Target-branch commits the PR branch has not seen. When this is non-zero,
HEAD differs from
+# BASE_SHA in places the PR never touched; "what did the PR change" questions
- the regression
+# flag above all - must be answered against MERGE_BASE, not BASE_SHA.
+TARGET_AHEAD="$(git rev-list --count "${MERGE_BASE}..${BASE_SHA}")"
# ------------------------------------------------------------------ worktree
state
git status --porcelain > "$CTX/worktree_status.txt"
@@ -350,7 +354,9 @@ Candidate format:
Status:
Path:
Line: <new-side line or start-end, must match
changed_line_ranges.txt or be justified>
- Severity: Blocker | Major | Minor | Nit
+ Severity: Blocker | Major | Minor | Nit <from the consequence when it
triggers, not its probability>
+ Category: functional-bug | functional-loss | data-error | resource-leak
| performance | observability | test-coverage | wording | maintainability
<one class, optionally "(domain note)">
+ Regression: yes | no <does HEAD differ from `git show
MERGE_BASE:<path>` in a way the PR body does not declare? cite merge-base
lines; yes in the first five categories is at least Major>
Claim:
Evidence: <call chain / concrete trigger scenario / file:line citations>
Duplicate relationship:
@@ -387,6 +393,10 @@ Owned by the main agent.
Source IDs:
Status: <accepted | dismissed_with_evidence
| duplicated>
Severity:
+ Category: <functional-bug | functional-loss |
data-error | resource-leak | performance | observability | test-coverage |
wording | maintainability>
+ Regression: <yes | no, with the merge-base
evidence (`git show MERGE_BASE:<path>`); yes in a
functional/data/resource/performance category is at least Major>
+ Severity rationale: <required when rated below the
subagent's proposal, and for a regression in any other category rated below
Major: consequence when it triggers, why not Major>
+ Severity challenge: <UPHOLD_DOWNGRADE | RAISE_SEVERITY
| n/a - from the section-E subagent>
Path:
Line:
Claim:
@@ -447,6 +457,7 @@ fi
echo "BASE_SOURCE=$BASE_SOURCE"
echo "HEAD_SHA=$HEAD_SHA"
echo "MERGE_BASE=$MERGE_BASE"
+ echo "TARGET_AHEAD=$TARGET_AHEAD"
echo "DIFF_RANGE=$DIFF_RANGE"
echo "DIRTY_FILES=$DIRTY_COUNT"
echo "STATE_DIR=${STATE_DIR:-}"
@@ -461,6 +472,9 @@ echo "changed files : $(grep -c .
"$CTX/pr_changed_files.txt" || true)"
echo "commits : $(grep -c . "$CTX/pr_commits.txt" || true)"
echo "diff lines : $(wc -l < "$CTX/pr.diff" | tr -d ' ')"
echo "diffstat : $(tail -n 1 "$CTX/pr_diffstat.txt")"
+if [ "${TARGET_AHEAD:-0}" -gt 0 ]; then
+ echo "target ahead : $TARGET_AHEAD commit(s) on $BASE_REF after the merge
base - judge regressions against MERGE_BASE, not BASE_SHA"
+fi
echo
if [ "${PRIOR_RUNS:-0}" -gt 0 ]; then
echo "prior runs of this PR (read them, see SKILL.md 2.1): $PRIOR_RUNS"
diff --git a/skills/doris-repo-review/scripts/verify-review-docs.py
b/skills/doris-repo-review/scripts/verify-review-docs.py
index bcebccf..037c411 100755
--- a/skills/doris-repo-review/scripts/verify-review-docs.py
+++ b/skills/doris-repo-review/scripts/verify-review-docs.py
@@ -1,5 +1,10 @@
#!/usr/bin/env python3
-"""Validate both doris-repo-review documents and emit their agreed result."""
+"""Validate both doris-repo-review documents and emit their agreed result.
+
+Besides head, anchors, verdict, rounds and convergence, every finding must
name its category
+and state whether it is a regression against the merge base. A regression in a
category whose
+consequence is functional, data, resource, or performance (see CATEGORIES) is
at least Major; a
+regression anywhere else may stay Minor/Nit only with a written severity
rationale."""
from __future__ import annotations
@@ -17,6 +22,47 @@ SEVERITY_RE = re.compile(
r"^-\s+\*\*(?:Severity|等级)\*\*\s*[:：]\s*(Blocker|Major|Minor|Nit)\s*$",
re.IGNORECASE,
)
+# `- **Regression**: yes` / `- **回归**：是`, optionally followed by one
parenthetical note or
+# the template's HTML comment.
+REGRESSION_RE = re.compile(
+ r"^-\s+\*\*(?:Regression|回归)\*\*\s*[:：]\s*(yes|no|是|否)\s*"
+ r"(?:[(（][^()（）]*[)）]|<!--.*?-->)?\s*$",
+ re.IGNORECASE,
+)
+REGRESSION_YES = ("yes", "是")
+# `- **Category**: functional-bug (concurrency: lock order)` / `-
**类别**：功能缺陷（并发）`.
+# The class comes first; one parenthetical domain note or the template's HTML
comment may follow.
+CATEGORY_RE = re.compile(
+ r"^-\s+\*\*(?:Category|类别)\*\*\s*[:：]\s*(.+?)\s*"
+ r"(?:[(（][^()（）]*[)）]|<!--.*?-->)?\s*$",
+ re.IGNORECASE,
+)
+# Canonical category -> (regression floor applies?, accepted spellings).
Spellings are compared
+# case-insensitively with spaces, hyphens and underscores removed, so
`functional bug`,
+# `functional_bug` and `功能性 bug` all resolve. Anything else is rejected: an
unknown category
+# would silently escape the floor.
+CATEGORIES: dict[str, tuple[bool, tuple[str, ...]]] = {
+ "functional-bug": (True, ("functional-bug", "功能缺陷", "功能性bug", "功能bug")),
+ "functional-loss": (True, ("functional-loss", "功能缺失", "功能性缺失")),
+ "data-error": (True, ("data-error", "数据错误")),
+ "resource-leak": (True, ("resource-leak", "资源泄露", "资源泄漏")),
+ "performance": (True, ("performance", "性能降低", "性能")),
+ "observability": (False, ("observability", "可观测性")),
+ "test-coverage": (False, ("test-coverage", "测试覆盖")),
+ "wording": (False, ("wording", "措辞")),
+ "maintainability": (False, ("maintainability", "可维护性")),
+}
+CATEGORY_ALIASES: dict[str, str] = {
+ re.sub(r"[\s_-]", "", alias).casefold(): canonical
+ for canonical, (_, aliases) in CATEGORIES.items()
+ for alias in aliases
+}
+FLOORED_CATEGORIES = tuple(name for name, (floored, _) in CATEGORIES.items()
if floored)
+# A regression - behaviour that differs from the merge base without the PR
declaring it
+# intended - in a floored category is at least Major, whatever the width of
its trigger or the
+# size of its fix. In any other category it may stay below Major only with a
written rationale.
+REGRESSION_SEVERITY_FLOOR = ("Blocker", "Major")
+RATIONALE_RE = re.compile(r"^\*\*(?:Severity rationale|定级理由)\s*[.。:：]?\s*\*\*")
HEAD_RE = re.compile(r"^\|\s*PR head\s*\|\s*`([0-9a-fA-F]{40})`")
VERDICT_RE =
re.compile(r"^\|\s*(?:Verdict|结论)\s*\|\s*\*\*(APPROVE|REQUEST_CHANGES)\*\*\s*\|")
ROUNDS_RE = re.compile(r"^\|\s*(?:Rounds|轮次)\s*\|(.*?)\|\s*$")
@@ -63,6 +109,9 @@ class Document:
converged: bool | None = None
finding_ids: list[str] = field(default_factory=list)
severities: dict[str, str] = field(default_factory=dict)
+ regressions: dict[str, bool] = field(default_factory=dict)
+ categories: dict[str, str] = field(default_factory=dict)
+ rationales: set[str] = field(default_factory=set)
anchors: list[Anchor] = field(default_factory=list)
finding_anchors: dict[str, list[Anchor]] = field(default_factory=lambda:
defaultdict(list))
@@ -149,6 +198,31 @@ def parse_document(path: Path, errors: list[str]) ->
Document:
else:
document.severities[current_finding] = match.group(1).title()
+ if match := REGRESSION_RE.match(line):
+ if current_finding is None:
+ errors.append(f"{path}:{lineno}: regression flag is not under
a finding")
+ elif current_finding in document.regressions:
+ errors.append(f"{path}:{lineno}: duplicate regression flag for
{current_finding}")
+ else:
+ document.regressions[current_finding] = match.group(1).lower()
in REGRESSION_YES
+
+ if match := CATEGORY_RE.match(line):
+ category = canonical_category(match.group(1))
+ if current_finding is None:
+ errors.append(f"{path}:{lineno}: category is not under a
finding")
+ elif category is None:
+ errors.append(
+ f"{path}:{lineno}: unknown category
{match.group(1).strip()!r} for "
+ f"{current_finding}; use one of {', '.join(CATEGORIES)}"
+ )
+ elif current_finding in document.categories:
+ errors.append(f"{path}:{lineno}: duplicate category for
{current_finding}")
+ else:
+ document.categories[current_finding] = category
+
+ if current_finding is not None and RATIONALE_RE.match(line):
+ document.rationales.add(current_finding)
+
for match in ANCHOR_RE.finditer(line):
anchor = Anchor(
path=match.group(1),
@@ -174,6 +248,30 @@ def parse_document(path: Path, errors: list[str]) ->
Document:
for finding in document.finding_ids:
if finding not in document.severities:
errors.append(f"{path}: finding {finding} has no severity")
+ if finding not in document.categories:
+ errors.append(
+ f"{path}: finding {finding} has no Category/类别 line (one of
{', '.join(CATEGORIES)})"
+ )
+ if finding not in document.regressions:
+ errors.append(
+ f"{path}: finding {finding} has no Regression/回归 line (yes/no
against the merge base)"
+ )
+ elif document.regressions[finding] and finding in document.categories:
+ severity = document.severities.get(finding)
+ category = document.categories[finding]
+ if severity in (None, *REGRESSION_SEVERITY_FLOOR):
+ pass
+ elif category in FLOORED_CATEGORIES:
+ errors.append(
+ f"{path}: finding {finding} is a regression in category
{category} but rated "
+ f"{severity}; a {category} regression against the merge
base is at least Major"
+ )
+ elif finding not in document.rationales:
+ errors.append(
+ f"{path}: finding {finding} is a regression in category
{category} rated "
+ f"{severity}; add a **Severity rationale** paragraph
saying why the behaviour "
+ "change is acceptable at that severity"
+ )
if not document.finding_anchors.get(finding):
errors.append(f"{path}: finding {finding} has no `path:line`
anchor")
@@ -191,6 +289,23 @@ def severity_counts(document: Document) -> dict[str, int]:
}
+def canonical_category(value: str) -> str | None:
+ return CATEGORY_ALIASES.get(re.sub(r"[\s_-]", "", value).casefold())
+
+
+def regression_count(document: Document) -> int:
+ return sum(1 for value in document.regressions.values() if value)
+
+
+def floored_regression_count(document: Document) -> int:
+ """Regressions whose category carries the Major floor - never present in a
valid APPROVE."""
+ return sum(
+ 1
+ for finding, value in document.regressions.items()
+ if value and document.categories.get(finding) in FLOORED_CATEGORIES
+ )
+
+
def finding_anchor_keys(document: Document, finding: str) -> list[tuple[str,
int, int]]:
"""Return stable semantic anchors, excluding document source-line
metadata."""
return sorted(
@@ -286,6 +401,10 @@ def main() -> int:
errors.append("EN and ZH finding IDs or order differ")
if left.severities != right.severities:
errors.append("EN and ZH finding severities differ")
+ if left.regressions != right.regressions:
+ errors.append("EN and ZH regression flags differ")
+ if left.categories != right.categories:
+ errors.append("EN and ZH finding categories differ")
for finding in sorted(set(left.finding_ids) & set(right.finding_ids)):
if finding_anchor_keys(left, finding) !=
finding_anchor_keys(right, finding):
errors.append(f"EN and ZH anchors differ for {finding}")
@@ -314,6 +433,8 @@ def main() -> int:
"commit": head_sha,
"verdict": documents[0].verdict,
"findings": severity_counts(documents[0]),
+ "regressions": regression_count(documents[0]),
+ "floored_regressions": floored_regression_count(documents[0]),
"rounds": documents[0].rounds,
"converged": documents[0].converged,
}
diff --git a/verify/README.md b/verify/README.md
index 150bbfb..ee02418 100644
--- a/verify/README.md
+++ b/verify/README.md
@@ -8,7 +8,7 @@ real Apache Doris cluster and against `doriscli`. Three layers:
| **L1 — knowledge** | Every DDL template (T1–T5) and DDL gotcha in
`doris-best-practices/SKILL.md` is accepted / rejected exactly as claimed |
`mysql` client | ✅ `run.sh` |
| **L2 — CLI contract** | Every command + JSON field in `CLI-CONTRACT.md`
really exists in `doriscli` | `doriscli --format json` + `jq` | ✅ `cli/run.sh` |
| **L3 — behavior** | Triggering, evidence-first / safety guardrails,
end-to-end DDL that loops back through L1 | nested `claude -p` | ✅ `behavior/`
(`run.sh` + `e2e-advisor-ddl.sh` + `triggering.sh`) |
-| **Repo review** | Runtime policy, verified review result, commit binding,
and automatic comment behavior | shell + Python + mocked `gh` | ✅
`repo-review/run.sh` |
+| **Repo review** | Runtime policy, verified review result (incl. the
per-finding category, the regression flag and its category-aware Major floor),
commit binding, and automatic comment behavior | shell + Python + mocked `gh` |
✅ `repo-review/run.sh` |
Run the repository-review contract tests without a cluster or GitHub access:
diff --git a/verify/repo-review/post-comment.sh
b/verify/repo-review/post-comment.sh
index d03723d..56bcac9 100755
--- a/verify/repo-review/post-comment.sh
+++ b/verify/repo-review/post-comment.sh
@@ -42,10 +42,23 @@ DOCS_ROOT=$REPO
EOF
}
+# write_docs <head> <verdict> <severity> <rounds> <convergence>
[regression=no] [category=functional-loss]
+# A non-floored category (observability, ...) gets a severity rationale so the
verifier accepts
+# the regression below Major; the poster is then the only remaining gate.
write_docs() {
local head="$1" verdict="$2" severity="$3" rounds="$4" convergence="$5"
- local zh_convergence=已收敛
+ local regression="${6:-no}" category="${7:-functional-loss}"
+ local zh_convergence=已收敛 zh_regression=否 zh_category=功能缺失
+ local en_rationale="" zh_rationale=""
[ "$convergence" = converged ] || zh_convergence=未收敛
+ [ "$regression" = no ] || zh_regression=是
+ case "$category" in
+ functional-loss) ;;
+ observability) zh_category=可观测性
+ en_rationale='**Severity rationale.** only a debug line lost
detail.'
+ zh_rationale='**定级理由。** 只是一条 debug 日志少了细节。' ;;
+ *) fail "fixture has no ZH form for category $category" ;;
+ esac
cat > "$REPO/review-docs/pr-123-review.en.md" <<EOF
# Code Review — PR #123: fixture
| | |
@@ -55,7 +68,11 @@ write_docs() {
| Rounds | $rounds of max 3, $convergence |
### F-01 · fixture
- **Severity**: $severity
+- **Category**: $category
+- **Regression**: $regression
- **Where**: \`src/Foo.java:1\`
+
+$en_rationale
EOF
cat > "$REPO/review-docs/pr-123-review.zh.md" <<EOF
# 代码评审 — PR #123：fixture
@@ -66,7 +83,11 @@ EOF
| 轮次 | 共 $rounds 轮（上限 3），$zh_convergence |
### F-01 · fixture
- **等级**：$severity
+- **类别**：$zh_category
+- **回归**：$zh_regression
- **位置**：\`src/Foo.java:1\`
+
+$zh_rationale
EOF
}
@@ -134,6 +155,27 @@ write_docs "$HEAD_SHA" REQUEST_CHANGES Major 2 converged
expect_failure "REQUEST_CHANGES never posts" "verdict is REQUEST_CHANGES" \
"$S/post-pass-comment.sh" --ctx "$CTX" --dry-run
+write_docs "$HEAD_SHA" APPROVE Minor 2 converged yes
+expect_failure "a floored regression rated Minor never posts" "failed
verification" \
+ "$S/post-pass-comment.sh" --ctx "$CTX" --dry-run
+
+write_docs "$HEAD_SHA" REQUEST_CHANGES Major 2 converged yes
+expect_failure "a floored regression finding never posts" "verdict is
REQUEST_CHANGES" \
+ "$S/post-pass-comment.sh" --ctx "$CTX" --dry-run
+
+write_docs "$HEAD_SHA" APPROVE Minor 2 converged yes observability
+expect_failure "an undisclosed non-floored regression never posts" "the
receipt must disclose" \
+ "$S/post-pass-comment.sh" --ctx "$CTX" --dry-run
+
+DISCLOSURE_NOTES="$TMP_ROOT/disclosure-notes.md"
+printf -- '- F-01 (observability, undeclared behaviour change)
`src/Foo.java:1` — a debug line lost detail; acceptable.\n' >
"$DISCLOSURE_NOTES"
+"$S/post-pass-comment.sh" --ctx "$CTX" --notes-file "$DISCLOSURE_NOTES"
--dry-run > "$TMP_ROOT/disclosed"
+grep -Fq "F-01 (observability, undeclared behaviour change)"
"$CTX/pr-comment.md" \
+ || fail "disclosed regression note is missing from the receipt"
+grep -Fq "findings: {blocker: 0, major: 0, minor: 1, nit: 0}"
"$CTX/pr-comment.md" \
+ || fail "receipt findings are wrong for a disclosed regression"
+pass "a disclosed non-floored regression rated Minor still posts"
+
write_docs "$HEAD_SHA" APPROVE Minor 3 'did not converge'
expect_failure "non-converged review never posts" "did not converge" \
"$S/post-pass-comment.sh" --ctx "$CTX" --dry-run
diff --git a/verify/repo-review/review-documents.sh
b/verify/repo-review/review-documents.sh
index 1544774..cb87629 100755
--- a/verify/repo-review/review-documents.sh
+++ b/verify/repo-review/review-documents.sh
@@ -36,6 +36,27 @@ DOCS_ROOT=$REPO
EOF
}
+# EN_REGRESSION / ZH_REGRESSION: the value of the mandatory regression line
(default no / 否);
+# EN_CATEGORY / ZH_CATEGORY: the value of the mandatory category line (default
a floored one);
+# EN_RATIONALE / ZH_RATIONALE: an optional "Severity rationale" paragraph.
+# The literal word `omit` leaves the line out entirely.
+EN_REGRESSION="${EN_REGRESSION:-no}"
+ZH_REGRESSION="${ZH_REGRESSION:-否}"
+EN_CATEGORY="${EN_CATEGORY:-functional-loss}"
+ZH_CATEGORY="${ZH_CATEGORY:-功能缺失}"
+EN_RATIONALE="${EN_RATIONALE:-omit}"
+ZH_RATIONALE="${ZH_RATIONALE:-omit}"
+
+finding_line() {
+ local label="$1" value="$2"
+ [ "$value" = omit ] || printf -- '- **%s**%s\n' "$label" "$value"
+}
+
+rationale_paragraph() {
+ local label="$1" value="$2"
+ [ "$value" = omit ] || printf -- '\n**%s** %s\n' "$label" "$value"
+}
+
write_docs() {
local head="$1" verdict="$2" en_severity="$3" zh_severity="$4"
local rounds="$5" en_convergence="$6" zh_convergence="$7"
@@ -57,7 +78,10 @@ write_docs() {
### F-01 · fixture finding
- **Severity**: $en_severity
+$(finding_line Category ": $EN_CATEGORY")
+$(finding_line Regression ": $EN_REGRESSION")
- **Where**: \`src/Foo.java:$en_anchor\`
+$(rationale_paragraph "Severity rationale." "$EN_RATIONALE")
EOF
cat > "$REPO/review-docs/pr-123-review.zh.md" <<EOF
# 代码评审 — PR #123：fixture
@@ -73,7 +97,10 @@ EOF
### F-01 · fixture finding
- **等级**：$zh_severity
+$(finding_line 类别 "：$ZH_CATEGORY")
+$(finding_line 回归 "：$ZH_REGRESSION")
- **位置**：\`src/Foo.java:$zh_anchor\`
+$(rationale_paragraph "定级理由。" "$ZH_RATIONALE")
EOF
}
@@ -87,10 +114,79 @@ write_meta "$HEAD_SHA"
write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
RESULT="$(verify_json)"
jq -e --arg head "$HEAD_SHA" \
- '.commit == $head and .verdict == "APPROVE" and .findings.minor == 1 and
.rounds == 2 and .converged' \
+ '.commit == $head and .verdict == "APPROVE" and .findings.minor == 1 and
.regressions == 0 and .floored_regressions == 0 and .rounds == 2 and
.converged' \
<<<"$RESULT" >/dev/null || fail "verified result JSON is wrong"
pass "matching documents produce one verified result"
+# ---- category-aware regression floor
+for category in "functional-bug 功能缺陷" "functional-loss 功能缺失" "data-error 数据错误"
\
+ "resource-leak 资源泄露" "performance 性能降低"; do
+ set -- $category
+ EN_CATEGORY="$1" ZH_CATEGORY="$2" EN_REGRESSION=yes ZH_REGRESSION=是 \
+ write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+ expect_failure "a $1 regression cannot be rated Minor" "a $1 regression
against the merge base is at least Major" verify_json
+done
+
+EN_REGRESSION=yes ZH_REGRESSION=是 write_docs "$HEAD_SHA" APPROVE Nit Nit 2
converged 已收敛
+expect_failure "a floored regression cannot be rated Nit" "at least Major"
verify_json
+
+EN_REGRESSION=yes ZH_REGRESSION=是 write_docs "$HEAD_SHA" REQUEST_CHANGES Major
Major 2 converged 已收敛
+RESULT="$(verify_json)"
+jq -e '.verdict == "REQUEST_CHANGES" and .findings.major == 1 and .regressions
== 1 and .floored_regressions == 1' <<<"$RESULT" >/dev/null \
+ || fail "regression counts are not reported"
+pass "a Major floored regression is counted in the verified result"
+
+EN_CATEGORY=observability ZH_CATEGORY=可观测性 EN_REGRESSION=yes ZH_REGRESSION=是 \
+ write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+expect_failure "a non-floored regression below Major needs a rationale" \
+ "add a **Severity rationale** paragraph" verify_json
+
+for category in "observability 可观测性" "test-coverage 测试覆盖" "wording 措辞"
"maintainability 可维护性"; do
+ set -- $category
+ EN_CATEGORY="$1" ZH_CATEGORY="$2" EN_REGRESSION=yes ZH_REGRESSION=是 \
+ EN_RATIONALE="only troubleshooting convenience is affected."
ZH_RATIONALE="只影响排障便利性。" \
+ write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+ RESULT="$(verify_json)"
+ jq -e '.verdict == "APPROVE" and .findings.minor == 1 and .regressions ==
1 and .floored_regressions == 0' <<<"$RESULT" >/dev/null \
+ || fail "a $1 regression with a rationale was not accepted at Minor"
+ pass "a $1 regression with a rationale may stay Minor"
+done
+
+EN_CATEGORY='functional-bug (concurrency: lock order)'
ZH_CATEGORY='功能缺陷（并发：锁序）' \
+ EN_REGRESSION=yes ZH_REGRESSION=是 write_docs "$HEAD_SHA" REQUEST_CHANGES
Major Major 2 converged 已收敛
+RESULT="$(verify_json)"
+jq -e '.floored_regressions == 1' <<<"$RESULT" >/dev/null || fail "a domain
note after the category was not accepted"
+pass "a parenthetical domain note after the category is accepted"
+
+EN_CATEGORY='Functional Bug' ZH_CATEGORY='功能性 bug' EN_REGRESSION=yes
ZH_REGRESSION=是 \
+ write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+expect_failure "category spelling variants still resolve to the floored class"
\
+ "a functional-bug regression against the merge base is at least Major"
verify_json
+
+EN_CATEGORY=omit write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+expect_failure "the category line is mandatory" "has no Category/类别 line"
verify_json
+
+EN_CATEGORY=security ZH_CATEGORY=安全 write_docs "$HEAD_SHA" APPROVE Minor Minor
2 converged 已收敛
+expect_failure "an unknown category is rejected" "unknown category 'security'"
verify_json
+
+EN_CATEGORY=observability ZH_CATEGORY=功能缺失 write_docs "$HEAD_SHA" APPROVE
Minor Minor 2 converged 已收敛
+expect_failure "EN and ZH categories must agree" "categories differ"
verify_json
+
+EN_REGRESSION='no (base 699-701 already lacked the reset)'
ZH_REGRESSION='否（base 本来就没有）' \
+ write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+RESULT="$(verify_json)"
+jq -e '.regressions == 0' <<<"$RESULT" >/dev/null || fail "annotated
regression flag was not accepted"
+pass "a parenthetical note after the regression flag is accepted"
+
+EN_REGRESSION=omit write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+expect_failure "the regression line is mandatory" "has no Regression/回归 line"
verify_json
+
+EN_REGRESSION=yes ZH_REGRESSION=否 write_docs "$HEAD_SHA" REQUEST_CHANGES Major
Major 2 converged 已收敛
+expect_failure "EN and ZH regression flags must agree" "regression flags
differ" verify_json
+
+EN_REGRESSION=maybe write_docs "$HEAD_SHA" APPROVE Minor Minor 2 converged 已收敛
+expect_failure "an unparseable regression value is rejected" "has no
Regression/回归 line" verify_json
+
write_docs "$OTHER_SHA" APPROVE Minor Minor 2 converged 已收敛
expect_failure "document commit must match context" "context head" verify_json
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
