mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
fix(triage): strengthen PR gate with batch detection, problem existence check, and red flag patterns (#5723)
* 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 <qwen-coder@alibabacloud.com> * 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 <noreply@qwen.ai> * 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 <noreply@qwen.ai> * fix(triage): remove dead problem-does-not-exist branch, clarify escalate/size wording - Drop the `<If problem does not exist:>` 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 <qwen-coder@alibabacloud.com> * 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 <!-- qwen-triage stage=1b --> 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 <qwen-coder@alibabacloud.com> Co-authored-by: Qwen-Coder <noreply@qwen.ai>
This commit is contained in:
parent
4e3fd29781
commit
a6a1258077
2 changed files with 120 additions and 10 deletions
|
|
@ -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'
|
||||
<!-- qwen-triage stage=1b -->
|
||||
|
||||
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.
|
||||
|
||||
<details>
|
||||
<summary>中文说明</summary>
|
||||
|
||||
这个 PR 解决的是一个理论性的问题——"理论上可能发生 X"——但没有复现证明它
|
||||
实际发生过。能否提供一个 before/after 复现,或者关联一个观测到此现象的 issue?
|
||||
|
||||
没有复现的 fix 只是一个假设——应该放在 issues 里,而不是 PR。
|
||||
如果作者在 re-run 时仍无法提供复现,请转交 maintainer 处理。
|
||||
|
||||
</details>
|
||||
|
||||
— _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: <state your honest assessment — aligned and why, or concerns and why>. CHANGELOG <reference if found, or "no direct reference but the area is relevant">.
|
||||
Problem: <state whether the problem is an observed bug with evidence, or theoretical hardening without reproduction. If no reproduction exists, say so plainly: "No before/after reproduction is provided. What scenario triggers this issue?">
|
||||
|
||||
On approach: <state your honest assessment — the scope feels right / feels like it could be much simpler / here's what I'd consider cutting>. <If you see a simpler path, name it: "Have you considered just X? It might cover most of the use case with a fraction of the complexity."> <If the diff carries unrelated changes or drive-by refactors, name them and suggest splitting them out.>
|
||||
Direction: <state your honest assessment — aligned and why, or concerns and why>. CHANGELOG <reference if found, or "no direct reference but the area is relevant">.
|
||||
|
||||
Approach: <state your honest assessment — the scope feels right / feels like it could be much simpler / here's what I'd consider cutting>. <If you see a simpler path, name it: "Have you considered just X? It might cover most of the use case with a fraction of the complexity."> <If the diff carries unrelated changes or drive-by refactors, name them and suggest splitting them out.>
|
||||
|
||||
<If passing:> Moving on to code review. 🔍
|
||||
<If concerns:> Flagging these for discussion before diving deeper.
|
||||
|
|
@ -112,6 +189,8 @@ On approach: <state your honest assessment — the scope feels right / feels lik
|
|||
|
||||
模板完整 ✓
|
||||
|
||||
问题:<说明问题是已观测到的 bug(有证据)还是理论性加固(无复现)。如果没有复现,直接说明:"未提供 before/after 复现。什么场景会触发这个问题?">
|
||||
|
||||
方向:<直接说判断——对齐的原因/担心的原因>。
|
||||
|
||||
方案:<范围合理 / 感觉可以大幅简化 / 建议砍掉的部分>。<如果看到更简路径,点名:有没有考虑过直接 X?可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构,点名并建议拆成单独 PR。>
|
||||
|
|
@ -124,8 +203,12 @@ On approach: <state your honest assessment — the scope feels right / feels lik
|
|||
— _Qwen Code · qwen3.7-max_
|
||||
```
|
||||
|
||||
Save this comment's ID. If direction is escalated → stop here. Template
|
||||
failures already stopped in Stage 1a.
|
||||
Save this comment's ID. Terminal exits — stop here if any applies:
|
||||
|
||||
- Core module hard block (Stage 0) → rejected, do not proceed.
|
||||
- Template failure (Stage 1a) → stopped.
|
||||
- Problem does not exist (Stage 1b) → request changes, do not proceed to Stage 2.
|
||||
- Direction escalated (Stage 1c) → stop here.
|
||||
|
||||
### Stage 2: Review + Test
|
||||
|
||||
|
|
@ -231,6 +314,9 @@ Step back and look at the whole picture — the motivation, the implementation,
|
|||
- After seeing it run, do the results match what the PR promised?
|
||||
- If I had to maintain this in six months, would I curse the author or thank them?
|
||||
- Am I approving this because it's genuinely good, or because I ran out of reasons to say no?
|
||||
- **Did I verify the problem actually exists?** Or did I accept the PR's framing ("this value could be passed") without asking "has this ever happened?" If the PR has no before/after reproduction, I should not be this far in the pipeline.
|
||||
- **Is this part of a pattern?** If the same author has multiple similar PRs open, am I evaluating each one on merit, or being worn down by volume?
|
||||
- **Am I being a pushover?** If I feel "this is probably fine but I'm not sure it's needed" — that feeling IS the signal. The gate's job is to say no to things that are not clearly needed.
|
||||
|
||||
If your independent proposal was materially simpler — say so. Not as a blocker, but as an honest question the contributor should think about.
|
||||
|
||||
|
|
|
|||
24
AGENTS.md
24
AGENTS.md
|
|
@ -21,6 +21,30 @@ simplify.
|
|||
|
||||
_Adapted from Andrej Karpathy's [CLAUDE.md](https://github.com/multica-ai/andrej-karpathy-skills/blob/main/CLAUDE.md)._
|
||||
|
||||
### Core Infrastructure Is Maintainer-Only (triage gate, two-tier rule)
|
||||
|
||||
Core modules — `packages/core/src/**`, `packages/*/src/auth/**`,
|
||||
`packages/*/src/providers/**`, `packages/*/src/models/**`,
|
||||
`packages/*/src/config/**`, `packages/*/src/tools/**`,
|
||||
`packages/*/src/services/**`, cross-package changes — are the architectural
|
||||
backbone. External PRs touching them face a two-tier gate (maintainer-authored
|
||||
PRs are exempt):
|
||||
|
||||
1. **Large-scope changes (500+ changed lines in core, additions +
|
||||
deletions combined) → hard block.**
|
||||
Skip evaluation entirely — the maintainer exemption above is the sole
|
||||
exception. Large-scale core refactors must be maintainer-initiated. Breadth alone is not size — a low-risk sweep that
|
||||
touches 10+ files but changes a line or two each is escalated to a
|
||||
maintainer for awareness and otherwise judged under Tier 2's
|
||||
100%-confidence bar, not auto-rejected on file count.
|
||||
2. **Small-scope changes → gate may evaluate, but must be 100% confident.**
|
||||
Any doubt at all → escalate to maintainer. "The direction looks correct"
|
||||
is not confidence. The gate must name every downstream consumer; if it
|
||||
cannot, escalate.
|
||||
|
||||
**When in doubt, escalate. Better to wrongly escalate than to wrongly
|
||||
approve.**
|
||||
|
||||
## Common Commands
|
||||
|
||||
### Building
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue