From a6a1258077ed5863ca9229eb65d4bdcd4e4db849 Mon Sep 17 00:00:00 2001 From: pomelo Date: Sat, 4 Jul 2026 00:38:37 +0800 Subject: [PATCH] fix(triage): strengthen PR gate with batch detection, problem existence check, and red flag patterns (#5723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(triage): strengthen PR gate with batch detection, problem existence check, and red flag patterns The PR triage gate was too permissive — it approved 20 validation-noise PRs from an AI bot in a single day without blocking any. Root cause: the gate asked "is the direction correct?" (easy to pass) but never asked "does this problem actually exist?" (the real question). Three new checks added to pr-workflow.md: 1. Gate Philosophy — explicit stance: the gate says no by default, burden of proof is on the author. AI-bot volume does not equal value. 2. Stage 0: Batch Pattern Detection — before individual review, check if the same author has 3+ similar PRs in 7 days. If so, evaluate as a group and close the noise batch together. 3. Stage 1b: Problem Existence Check — mandatory before direction review. Distinguishes observed bugs (with reproduction) from theoretical hardening (without). Includes red flag phrases that signal "no real problem" (e.g. "the runtime validators already enforce..."). Also updated Stage 1 comment template and Stage 3 reflection questions. * fix(triage): fix broken jq filter, add missing stop instructions, and tighten prompt - Fix jq date comparison: string '7 days ago' always false, use fromdateiso8601 - Fix --state all to --state open to match '3+ open PRs' prose - Add terminal exit list after Stage 1 comment (1b problem-existence was missing) - Fix Stage 3 batch threshold (10+) to reference Stage 0 (3+) - Compress Gate Philosophy, Stage 0, and 1b for conciseness * feat(triage): add core module scope gate to reject unsolicited refactors Add Stage 0b that flatly rejects community-contributed refactor PRs touching core infrastructure (packages/core, auth, providers, models, config, tools, services). No exceptions — core refactors must be maintainer-initiated with prior design discussion. Triggered by PR #5089: 75-file refactor across core/auth/providers/models that should never have been accepted from a community contributor. * docs(agents): add core infrastructure maintainer-only policy to Working Principles * feat(triage): gate must verify it truly understands PR impact before approving The meta-principle: 'the direction looks correct' is the most dangerous sentence in triage — it means the gate understood intent but not impact. For core module changes, the gate must name every downstream consumer; if it cannot, escalate to maintainer. 100% confidence or escalate. Updated both pr-workflow.md (Stage 0b) and AGENTS.md (Working Principles). * fix(triage): make core module gate a hard block with no judgment allowed Previous version asked the gate to 'assess whether it understands the impact' — too soft, AI will always rationalize that it understands enough. New version: non-maintainer + core paths = instant reject. No evaluation, no thinking, no exceptions. The gate is not qualified to judge core refactors. Period. * fix(triage): two-tier core module gate — hard block large, 100% confidence for small Large-scope core changes (10+ files / 500+ lines) are a hard block — no evaluation, no exceptions. Small-scope core changes may proceed only if the gate is 100% confident; any doubt triggers maintainer escalation. Previous versions were either too soft ('assess your understanding') or too blunt ('all non-maintainer core PRs rejected'). Two-tier approach allows legitimate small bugfixes through while walling off refactors. * refactor(triage): remove Stage 0 batch pattern detection — low ROI, always returns empty for human contributors Co-authored-by: Qwen-Coder * fix(triage): address review feedback on pr-workflow and AGENTS.md - Fix "Closing" vs "request changes" inconsistency in Stage 1b template (both EN and CN) — align with terminal exits list - Add Stage 0b to terminal exits list and terminal gate exception - Add gh pr review command to Tier 1 hard block (was missing delivery mechanism) - Clarify Tier 1 "500+ lines" as additions + deletions combined - Add maintainer-authored exception to Tier 1 hard block - Use concrete path patterns for core infrastructure definition - Fix dangling Stage 0 reference in Stage 3 reflection (Stage 0 was removed) - Scope AGENTS.md core rule to triage gate, add cross-package changes to definition * fix(triage): rename Stage 0b→0, fix close ambiguity, add stage labels to exits - Rename "Stage 0b" to "Stage 0" (no more Stage 0a since batch detection was removed) - Fix ambiguous "close" in Stage 1b → "leave for maintainer to decide" (agent recommends, maintainer executes gh pr close) - Add explicit stage labels to all terminal exits (Stage 0, 1a, 1b, 1c) * fix(triage): hard-block on net size, not file breadth wenshao: a low-risk sweep (rename, import-path update, lint/format autofix, a repeated null-guard) can touch 10+ core files while changing a line or two each, yet the 'OR 10+ files' trigger hard-blocked it with 'open an issue to discuss', while a deep risky rewrite under 10 files slipped into the lenient Tier 2 path. Breadth was treated as risk; depth was ignored. Make the hard block fire on size alone (500+ changed lines), which still catches a deep rewrite concentrated in a few files. Route pure file-count breadth to maintainer escalation / Tier 2 judgement on the actual diff instead of an auto-reject. Co-Authored-By: Qwen-Coder * fix(triage): make re-run escalation concrete; align breadth wording across files - Stage 1b: replace vague 'leave for maintainer to decide' with concrete escalate-to-maintainer + stop (no Stage 2), matching the escalation vocabulary used in Stage 1c/Stage 3. - AGENTS.md: clarify that a file-breadth sweep is escalated for awareness and otherwise judged under Tier 2's 100%-confidence bar, aligning with pr-workflow.md's Breadth-ne-size paragraph (removes terminal-stop ambiguity). Co-Authored-By: Qwen-Coder * fix(triage): remove dead problem-does-not-exist branch, clarify escalate/size wording - Drop the `` Stage 1 comment branch: it is a terminal exit that submits a CHANGES_REQUESTED review and must not also post a Stage 1 comment, so the template branch was unreachable. - Reword the breadth-sweep case to "flag for the maintainer's awareness" so "escalate" consistently means stop/hand-off across the file. - Clarify AGENTS.md 500-line threshold as additions + deletions combined, matching pr-workflow.md. Co-authored-by: Qwen-Coder * fix(triage): address 4 review threads — Stage 1b command, re-run carve-out, Tier 2 wording, AGENTS.md exemption - Add concrete gh pr review --request-changes command block with body template and marker to Stage 1b, matching every other terminal gate's invocation pattern. - Add terminal-exit review carve-out to re-runs section: PR reviews cannot be edited via PATCH API, so on re-run check for existing CHANGES_REQUESTED review before re-submitting. - Fix Tier 2 precondition wording: 'Small-scope' → 'below 500-line threshold' and 'fewer files' → 'few files, or a breadth-sweep flagged above' so the breadth carve-out PRs are not excluded by the Tier 2 description alone. - Fix AGENTS.md 'No evaluation, no exceptions' → 'Skip evaluation entirely — the maintainer exemption above is the sole exception' to eliminate the apparent contradiction with the parenthetical maintainer exemption. --------- Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder --- .qwen/skills/triage/references/pr-workflow.md | 106 ++++++++++++++++-- AGENTS.md | 24 ++++ 2 files changed, 120 insertions(+), 10 deletions(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 7c3b1edc8d..179f4d47e8 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -19,9 +19,11 @@ COMMENT_ID=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" -F body=@/tmp/stage | Stage 2 | Code review + test results (with screenshots) | | Stage 3 | Reflection + verdict | -**Terminal gate exception:** if Stage 1a template check fails, submit exactly -one `CHANGES_REQUESTED` review and stop. Do not also post or update a Stage 1 -issue comment, and do not continue to Stage 2, Stage 3, or approval. +**Terminal gate exception:** if any terminal exit triggers (Stage 0 core +module hard block, Stage 1a template failure, Stage 1b problem-does-not-exist, +or Stage 1c direction escalation), submit exactly one `CHANGES_REQUESTED` +review and stop. Do not also post or update a Stage 1 issue comment, and do not +continue to Stage 2, Stage 3, or approval. **Re-runs:** if the triage runs again on the same PR, update each comment in place: @@ -29,7 +31,19 @@ issue comment, and do not continue to Stage 2, Stage 3, or approval. gh api -X PATCH "/repos/$REPO/issues/comments/$COMMENT_ID" -F body=@/tmp/stage-N-updated.md ``` -Never create duplicates. +Never create duplicates. For terminal-exit reviews (submitted via +`gh pr review --request-changes`), the GitHub API does not support editing PR +reviews. On re-run: check if a `CHANGES_REQUESTED` review from the bot already +exists — if it does, skip re-submitting (the existing review already gates the +PR). Only update issue comments, not PR reviews. + +```bash +# Check for existing terminal-exit review before re-submitting +EXISTING=$(gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \ + --jq '[.[] | select(.user.login=="qwen-code-ci-bot" and .state=="CHANGES_REQUESTED")] | length') +# Only submit if no existing terminal review +if [ "$EXISTING" -eq 0 ]; then gh pr review ... ; fi +``` **Signature:** every comment ends with: @@ -39,6 +53,28 @@ Never create duplicates. **Approval:** the `gh pr review --approve` command is a separate step that runs **after** Stage 3 comment is posted. Comment first, then approve only when genuinely confident. +### Gate Philosophy + +Default posture: **skepticism**. Burden of proof is on the author. Distinguish **observed failures** (linked issue, reproduction, before/after) from **theoretical hardening** ("could theoretically send X" with no evidence it ever has). Volume ≠ value — an AI bot can produce 20 plausible PRs in a day. If being "too strict" feels uncomfortable, that is the gate working correctly. + +### Stage 0: Core Module Protection (two-tier check) + +Core infrastructure: files matching `packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`, or cross-package changes spanning multiple `packages/*/`. + +**Tier 1 — Large-scope changes to core → HARD BLOCK.** Applies to non-maintainer PRs only (skip this check if the author is a known maintainer). Hard-block on _size_, not breadth: if a core-path change totals **500+ lines** (additions + deletions combined) → reject immediately. No evaluation, no Stage 1. + +```bash +gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body "This change touches core infrastructure at scale. Core refactors must be maintainer-initiated — please open an issue to discuss the design first." +``` + +Then **stop**. This is a wall, not a guideline. + +**Breadth ≠ size.** A uniform, low-risk sweep — renaming a symbol, updating an import path, a lint/format autofix, the same null-guard at many call sites — can touch **10+ files** while changing only a line or two each. Don't auto-reject on file count alone: **flag it for the maintainer's awareness**, and otherwise let it proceed to Stage 1 under Tier 2's 100%-confidence bar, judged on the actual diff rather than the file count. (A deep rewrite concentrated in a few files still trips the 500-line threshold above, so depth isn't ignored.) + +**Tier 2 — Changes to core below the 500-line threshold → evaluate with 100% confidence.** If the PR hits core paths but stays under Tier 1 (either few files, or a breadth-sweep flagged above), you MAY proceed to Stage 1 — but only if you are **100% confident** the change is correct and safe. If there is any doubt at all — "the direction looks correct" is NOT 100% confidence — escalate to maintainer before proceeding. You must be able to name every downstream consumer affected; if you cannot, escalate. + +**Why two tiers:** A one-line bugfix in `packages/core/src/providers/install.ts` with a clear reproduction is different from a 75-file refactor of the provider system. The gate can handle the former; the latter requires maintainer architectural context. But for any core change, **when in doubt, escalate. Better to wrongly escalate than to wrongly approve.** + ### Stage 1: Gate (Template + Direction + Solution Review) **⛔ Before anything else: create a worktree.** This is the #1 forgotten step. @@ -59,7 +95,46 @@ PR body missing required headings from `.github/pull_request_template.md` (read gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body-file /tmp/pr-gate-template.md ``` -**1b. Product direction:** +**1b. Problem existence check (MANDATORY):** + +Before "is the direction right?", ask **"does this problem actually exist?"** + +- **Observed bug** (linked issue, reproduction, before/after) → proceed. +- **Theoretical hardening** ("could theoretically send X" with no evidence) → **request changes.** Ask for a reproduction: + +```bash +cat > /tmp/stage-1b-reproduction.md <<'EOF' + + +This PR addresses a theoretical concern — "could theoretically send X" — but +no reproduction demonstrates it has actually happened. Could you provide a +before/after reproduction or link an issue where this was observed? + +Without a reproduction, this is a hypothesis that belongs in issues, not PRs. +If the author cannot provide one on re-run, escalate to the maintainer and stop. + +
+中文说明 + +这个 PR 解决的是一个理论性的问题——"理论上可能发生 X"——但没有复现证明它 +实际发生过。能否提供一个 before/after 复现，或者关联一个观测到此现象的 issue？ + +没有复现的 fix 只是一个假设——应该放在 issues 里，而不是 PR。 +如果作者在 re-run 时仍无法提供复现，请转交 maintainer 处理。 + +
+ +— _Qwen Code · qwen3.7-max_ +EOF +gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body-file /tmp/stage-1b-reproduction.md +``` + +If the author cannot provide a reproduction on re-run, escalate to the maintainer (use `$QWEN_MAINTAINER_HANDLE` if set) and stop — do not proceed to Stage 2. +- **No reproduction = no fix.** A `fix:` PR without reproduction is a hypothesis — belongs in issues, not PRs. + +**"direction is correct" ≠ "problem exists."** If the runtime already handles the case correctly, there is no bug — only code hygiene. Code hygiene does not warrant a PR. + +**1c. Product direction:** Ask the hard questions before reading a single line of code: @@ -78,7 +153,7 @@ curl -s https://raw.githubusercontent.com/anthropics/claude-code/main/CHANGELOG. **Escalate to maintainer** (never auto-reject): touches auth/sandbox/model selection/telemetry/release/public contract, or direction is genuinely unclear. -**1c. Solution review** (never skip — judge from the PR description and a skim of the diff structure, before reading code in detail): +**1d. Solution review** (never skip — judge from the PR description and a skim of the diff structure, before reading code in detail): - If we cut 80% of the scope, would the remaining 20% already solve the problem? - Could we achieve the same goal by modifying something that already exists, instead of adding something new? @@ -98,9 +173,11 @@ Thanks for the PR! Template looks good ✓ -On direction: . CHANGELOG . +Problem: -On approach: . +Direction: . CHANGELOG . + +Approach: . Moving on to code review. 🔍 Flagging these for discussion before diving deeper. @@ -112,6 +189,8 @@ On approach: + 方向：<直接说判断——对齐的原因/担心的原因>。 方案：<范围合理 / 感觉可以大幅简化 / 建议砍掉的部分>。<如果看到更简路径，点名：有没有考虑过直接 X？可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构，点名并建议拆成单独 PR。> @@ -124,8 +203,12 @@ On approach: