From e2d6b4b2cacecd5f088c3ee701bf4e8c59b9f082 Mon Sep 17 00:00:00 2001 From: review Date: Thu, 24 Sep 2026 15:23:22 +0800 Subject: [PATCH 1/2] =?UTF-8?q?chore(review):=20scope=20e2e=20requirement,?= =?UTF-8?q?=20drop=20provider=C3=97agent=20matrix,=20add=20P3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-review gate over-flagged: it required a full provider × agent e2e matrix as [P1 blocking] on every PR, raised theoretical/low-confidence risks at blocking severity, and repeated already-resolved findings. - e2e record is [P1 blocking] only for behavior-changing PRs; docs-only / tests-only diffs need none. One representative real-CLI run is enough; missing extra provider/agent coverage the author flagged as untestable here or deferred to CI is at most [P3 nit], never blocking. - Add a third severity [P3 nit] (minor/optional polish, theoretical edge case, coverage deferred to CI). - Tell the reviewer not to over-review: report only high-confidence findings, prefer few high-signal over exhaustive, never restate a resolved finding. Kept in sync across AGENTS.md (## Code Review Rules, the trusted base criteria) and the workflow prompt in codex-review-on-assign.yml. --- .github/workflows/codex-review-on-assign.yml | 26 ++++++++++----- AGENTS.md | 33 ++++++++++++++------ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/.github/workflows/codex-review-on-assign.yml b/.github/workflows/codex-review-on-assign.yml index 840817b44..c2ee7add2 100644 --- a/.github/workflows/codex-review-on-assign.yml +++ b/.github/workflows/codex-review-on-assign.yml @@ -199,14 +199,24 @@ jobs: 'you. Do not run, build, or install anything from the PR; only read', 'the diff.', '', - 'Flag every bug, risk, and rule violation that is present in the', - 'current diff. Keep each finding short and specific, but never cap', - 'how many findings you report or leave a finding for a later pass.', - 'The severity label ranks findings; the list does not need to.', - 'Also judge whether the PR description contains sufficient testing /', - 'end-to-end test notes; if it lacks a test plan or an e2e record,', - 'flag it as a blocking issue. Reply in the same language the PR', - 'author used in the PR description.', + 'Report only findings you are confident are real in the current', + 'diff. Do not over-review: prefer a few high-signal findings over', + 'an exhaustive list, and keep each one short and specific. A', + 'speculative or theoretical risk that needs an unlikely', + 'precondition to trigger is at most [P3 nit], not blocking. Use', + 'three severities, spelled out inline in the author\'s language:', + '[P1 blocking] (must fix before merge), [P2 non-blocking]', + '(suggestion), [P3 nit] (minor/optional polish, theoretical edge', + 'case, or coverage deferred to CI).', + 'Also judge whether the PR description documents sufficient', + 'testing. A PR that changes runtime behavior (beyond a docs-only', + 'or tests-only diff) needs an end-to-end / real-CLI record; flag', + 'its absence as [P1 blocking]. A docs-only or tests-only PR needs', + 'no e2e record — do not flag it for that. Do NOT require a full', + 'provider x agent matrix: one representative real-CLI run suffices,', + 'and missing extra-provider/agent coverage the author flagged as', + 'untestable here or deferred to CI is at most [P3 nit]. Reply in', + 'the same language the PR author used in the PR description.', '', 'Earlier passes of this review raised the findings below. A finding', 'already fixed in the current diff is resolved: say so, but do not', diff --git a/AGENTS.md b/AGENTS.md index d159c3713..b09293ba6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,11 +34,23 @@ shared state, list every reader and every writer. ## Code Review Rules -- The PR description must document sufficient testing, including an - end-to-end / real-CLI verification record — not only unit tests or type - checks. Flag a PR whose description lacks a test plan or an e2e record. - A test record naming an older commit than the head is a note, not a blocking - finding; the body may have been edited after the review pass started. +- The PR description must document sufficient testing. For a PR that changes + runtime behavior (anything beyond a docs-only or tests-only diff), that + includes an end-to-end / real-CLI verification record, not only unit tests + or type checks — flag such a PR that lacks one as `[P1 blocking]`. A + docs-only or tests-only PR needs no e2e record; do not flag it for that. + Do NOT require a full provider × agent matrix — one representative real-CLI + run is enough. Missing coverage of extra providers (`gitlab`/`github`) or + agents (`Codex`/`CodeBuddy`/`OpenCode`) is at most `[P3 nit]` when the author + has flagged it as untestable in this environment or deferred to CI, never + `[P1 blocking]`. A test record naming an older commit than the head is a + note, not a blocking finding; the body may have been edited after the review + pass started. +- Do not over-review. Report only findings you are confident are real in the + current diff, and prefer a few high-signal findings over an exhaustive list. + A speculative or theoretical risk that needs an unlikely precondition to + trigger is at most `[P3 nit]`, not `[P1 blocking]`; never restate a finding + already resolved in the current diff. - Reject over-engineering. Favor the smallest code that solves the problem; flag speculative abstractions, unused flexibility or config, error handling for cases that cannot occur, and new CLI commands added where an existing @@ -46,8 +58,9 @@ shared state, list every reader and every writer. - Changes must be surgical. Every changed line should trace directly to the PR's stated goal; flag unrelated drive-by edits. - Label every finding with an explicit severity a first-time reader can - understand — never a bare `P1`/`P2` code. Keep the `P` marker but spell out - what it means inline on each finding, using the PR author's language: - `[P1 blocking]` for issues that must be fixed before merge, and - `[P2 non-blocking]` for suggestions that do not block merge. (In Chinese, - `[P1 阻断]` / `[P2 非阻断]`.) + understand — never a bare `P1`/`P2`/`P3` code. Keep the `P` marker but spell + out what it means inline on each finding, using the PR author's language: + `[P1 blocking]` for issues that must be fixed before merge, + `[P2 non-blocking]` for suggestions that do not block merge, and `[P3 nit]` + for minor or optional polish, theoretical edge cases, and coverage deferred + to CI. (In Chinese, `[P1 阻断]` / `[P2 非阻断]` / `[P3 可选]`.) From 93110f636382590ad086036053fa52deedab5398 Mon Sep 17 00:00:00 2001 From: review Date: Thu, 24 Sep 2026 17:36:56 +0800 Subject: [PATCH 2/2] =?UTF-8?q?chore(review):=20relax=20PR=20=E5=89=8D?= =?UTF-8?q?=E6=B5=8B=E8=AF=95=20matrix,=20gate=20P1=20by=20evidence=20not?= =?UTF-8?q?=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address SaulMoro's changes-requested on #781: - The provider × agent matrix requirement still lived in `## PR 前测试` (AGENTS.md/CLAUDE.md), which the Codex reviewer loads whole — so the matrix could return as a P1 despite `## Code Review Rules` relaxing it. Relax `## PR 前测试` to match: one representative real-CLI run suffices, docs/tests-only needs no e2e, extra provider/agent coverage defers to CI. - Replace "prefer a few high-signal findings" (which caps real bugs and pushes them to later passes) with evidence-gated severity: every [P1 blocking] must cite a concrete failure scenario or the exact rule it breaks, else it drops to P2/P3. Count is uncapped; real bugs surface in one pass. Synced into the workflow prompt. --- .github/workflows/codex-review-on-assign.yml | 12 ++++++++---- AGENTS.md | 16 +++++++++++----- CLAUDE.md | 4 +++- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/.github/workflows/codex-review-on-assign.yml b/.github/workflows/codex-review-on-assign.yml index c2ee7add2..01b1fe43b 100644 --- a/.github/workflows/codex-review-on-assign.yml +++ b/.github/workflows/codex-review-on-assign.yml @@ -200,10 +200,14 @@ jobs: 'the diff.', '', 'Report only findings you are confident are real in the current', - 'diff. Do not over-review: prefer a few high-signal findings over', - 'an exhaustive list, and keep each one short and specific. A', - 'speculative or theoretical risk that needs an unlikely', - 'precondition to trigger is at most [P3 nit], not blocking. Use', + 'diff, and keep each one short and specific. Do not cap how many', + 'you report: surface every real bug in one pass. Gate severity by', + 'evidence instead — every [P1 blocking] must cite either a', + 'concrete failure scenario (the input or state that triggers it', + 'and the resulting misbehavior) or the exact Code Review Rules', + 'item it breaks. A finding that can cite neither is at most', + '[P2 non-blocking]; a speculative or theoretical risk that needs', + 'an unlikely precondition to trigger is at most [P3 nit]. Use', 'three severities, spelled out inline in the author\'s language:', '[P1 blocking] (must fix before merge), [P2 non-blocking]', '(suggestion), [P3 nit] (minor/optional polish, theoretical edge', diff --git a/AGENTS.md b/AGENTS.md index b09293ba6..08e569325 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,7 +22,9 @@ TypeScript, Node 20+, tsup (ESM), Vitest. Commands: `npm run build`, `npx tsc -- ## PR 前测试 -`npm run build` 后用真实 CLI 对本次改动做完整端到端验证（不能只跑 type check / unit test）。Test Plan 每一项必须实际通过，测试报告贴进 PR。 +改动运行时行为的 PR（docs-only / tests-only 之外），`npm run build` 后必须用真实 CLI 对本次改动做端到端验证，不能只跑 type check / unit test；**一次代表性的 real-CLI 运行即可**，把实际通过的验证记录贴进 PR。docs-only / tests-only 的改动无需 e2e 记录。 + +不要求覆盖下面的完整 provider × agent 矩阵——额外 provider / agent 的覆盖交给 CI，或在本地环境不具备时说明即可： - Agent：Claude、Codex、CodeBuddy、OpenCode - Provider：`git`、`gitlab`、`github` @@ -47,10 +49,14 @@ shared state, list every reader and every writer. note, not a blocking finding; the body may have been edited after the review pass started. - Do not over-review. Report only findings you are confident are real in the - current diff, and prefer a few high-signal findings over an exhaustive list. - A speculative or theoretical risk that needs an unlikely precondition to - trigger is at most `[P3 nit]`, not `[P1 blocking]`; never restate a finding - already resolved in the current diff. + current diff. Do not cap how many findings you report: every real bug should + surface in one pass, not be deferred to a later one. Instead, gate severity + by evidence — every `[P1 blocking]` must cite either a concrete failure + scenario (the input or state that triggers it and the resulting misbehavior) + or the exact `## Code Review Rules` item it breaks. A finding that can cite + neither is at most `[P2 non-blocking]`; a speculative or theoretical risk + that needs an unlikely precondition to trigger is at most `[P3 nit]`. Never + restate a finding already resolved in the current diff. - Reject over-engineering. Favor the smallest code that solves the problem; flag speculative abstractions, unused flexibility or config, error handling for cases that cannot occur, and new CLI commands added where an existing diff --git a/CLAUDE.md b/CLAUDE.md index e93ec1fd0..8ad520d3d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,7 +22,9 @@ TypeScript, Node 20+, tsup (ESM), Vitest. Commands: `npm run build`, `npx tsc -- ## PR 前测试 -`npm run build` 后用真实 CLI 对本次改动做完整端到端验证（不能只跑 type check / unit test）。Test Plan 每一项必须实际通过，测试报告贴进 PR。 +改动运行时行为的 PR（docs-only / tests-only 之外），`npm run build` 后必须用真实 CLI 对本次改动做端到端验证，不能只跑 type check / unit test；**一次代表性的 real-CLI 运行即可**，把实际通过的验证记录贴进 PR。docs-only / tests-only 的改动无需 e2e 记录。 + +不要求覆盖下面的完整 provider × agent 矩阵——额外 provider / agent 的覆盖交给 CI，或在本地环境不具备时说明即可： - Agent：Claude、Codex、CodeBuddy、OpenCode - Provider：`git`、`gitlab`、`github`