From 9e2818ae89fe1b2ff913a28f6c45b39684f7e855 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 27 Jul 2026 20:39:51 +0800 Subject: [PATCH 01/11] feat(triage): add revert-pattern high-risk path detection Replace the behavior-neutral PR filter (PR #7414 v1, ~2% hit rate) with a data-backed triage gate based on revert-history analysis of 111 revert commits and 46 unique reverted PRs in this repo. Stage 1e checks three signals identified by the analysis: - touches_high_risk (66.7% precision, 32.3% recall) - contested-merge pattern (50.0% precision, 19.4% recall) - non-maintainer + high-risk (58.3% precision, 22.6% recall) The gate escalates review depth and recommends maintainer sign-off; it never blocks or closes PRs. Design doc and analysis scripts included. --- .qwen/skills/triage/references/pr-workflow.md | 30 +++ .../2026-07-27-revert-pattern-triage-gate.md | 218 ++++++++++++++++++ scripts/tests/qwen-triage-workflow.test.js | 41 ++++ 3 files changed, 289 insertions(+) create mode 100644 docs/design/2026-07-27-revert-pattern-triage-gate.md diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 2b3f72b45db..2f96b22357f 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -227,6 +227,36 @@ If you spot a materially simpler path, or changes that go beyond the minimal set Implementation-level concerns (over-abstraction, code duplication, "10 lines vs 10 files") belong in Stage 2a code review — you need to see the code for those. +**1e. High-risk path and contested-merge detection (data-backed escalation):** + +A revert-history analysis of this repo (111 revert commits, 46 unique reverted PRs) found that certain file paths and review patterns are strongly correlated with post-merge reverts. Check for these signals before proceeding to Stage 2 — they do NOT block or close the PR, but they determine the review depth and whether a maintainer sign-off is recommended. + +**High-risk paths** — check the PR's changed files against these patterns: + +```bash +gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|ELECTRON_RUN_AS_NODE' +``` + +If any file matches (66.7% revert precision, 32.3% recall in the analysis): +- For non-maintainer PRs: escalate to the deepest review tier — do not skip any review stage, run full `/review` with maximum agent coverage. +- Flag the high-risk paths in the Stage 1 comment so the reviewer knows where to focus. +- Recommend E2E verification in tmux (Stage 2b) before approval. + +**Contested-merge pattern** — check the PR's review history for a CHANGES_REQUESTED → APPROVE cycle: + +```bash +gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | .state]' +``` + +If `CHANGES_REQUESTED` appears after the first review (not at position 0) AND the PR touches core paths (50.0% revert precision, 19.4% recall): +- Apply `status/on-hold` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label status/on-hold` (if the label exists). +- Recommend maintainer sign-off before merge in the Stage 1 comment. +- Do not auto-approve even if Stage 2 and Stage 3 are clean. + +**Non-maintainer + high-risk** (58.3% revert precision, 22.6% recall): the intersection of both signals above is the highest-risk tier. Apply both actions: deepest review depth AND recommend maintainer sign-off. + +These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. + Post a single Stage 1 comment. Be direct — say what you actually think, not what's polite: ```markdown diff --git a/docs/design/2026-07-27-revert-pattern-triage-gate.md b/docs/design/2026-07-27-revert-pattern-triage-gate.md new file mode 100644 index 00000000000..bc67211d099 --- /dev/null +++ b/docs/design/2026-07-27-revert-pattern-triage-gate.md @@ -0,0 +1,218 @@ +# Revert-pattern triage gate + +Date: 2026-07-27 +Status: Proposed +Area: CI triage — `.github/workflows/qwen-code-pr-review.yml`, `.qwen/skills/triage/` + +## Problem + +Small behavior-neutral maintenance PRs currently consume the same multi-stage +triage and model review capacity as behavior changes. The original proposal +(PR #7414) attempted to filter these out, but a maintainer measurement of the +live backlog showed only a ~2% hit rate — the feature targeted a near-nonexistent +problem. + +Meanwhile, the repo has **111 revert commits** across its history (19 in July +2026 alone), and **71% of reverts occur within 24 hours of merge** — meaning +the problem is caught quickly but after it's already on `main`. The real cost +is not reviewing harmless PRs; it's merging PRs that have to be rolled back. + +This design proposes a data-backed triage gate that targets the PRs that +actually cause reverts, not the ones that are already harmless. + +## Data + +### Methodology + +Three-phase analysis of the repo's full revert history: + +1. **Collection**: `git log --all --grep="^Revert "` found 111 revert commits. + Each revert's body was parsed for `This reverts commit `, then the + original commit was traced back to its PR via `gh api`. Result: 46 unique + reverted PRs (59 reverts traceable to a PR number; 52 reverts only had the + original commit title without a PR link). + +2. **Enrichment**: For each reverted PR, extracted triage-time observable + signals: touch scope (core/auth/providers/tools/services), diff size, review + round count, bot Critical findings, CHANGES_REQUESTED cycles, merge→revert + time gap, self-revert, E2E verification presence. 31 of 46 PRs were + successfully enriched; 15 are deleted and inaccessible (HTTP 404). + +3. **Control group comparison**: Sampled 60 recently-merged-but-not-reverted + PRs and extracted the same signals. Computed precision (TP / (TP + FP)) + and recall for each signal. + +Scripts and raw data: `.qwen/scripts/revert-analysis-*.mjs`, +`.qwen/scripts/revert-data-*.json`, `.qwen/scripts/revert-analysis-report-v2.json`. + +### Signal precision and recall + +| Signal | Precision | Recall | Reverted (n=31) | Control (n=60) | +| ------ | --------- | ------ | --------------- | -------------- | +| `touches_high_risk` | **66.7%** | 32.3% | 10 | 5 | +| `non_maintainer + high_risk` | **58.3%** | 22.6% | 7 | 5 | +| `core + contested` | **50.0%** | 19.4% | 6 | 6 | +| `non_maintainer + core` | 46.2% | 38.7% | 12 | 14 | +| `touches_core` | 44.7% | 54.8% | 17 | 21 | +| `has_contested_pattern` | 40.9% | 29.0% | 9 | 13 | +| `had_changes_requested` | 40.7% | 35.5% | 11 | 16 | +| `non_maintainer` | 39.6% | 67.7% | 21 | 32 | +| `large_diff_gt_200` | 37.0% | 54.8% | 17 | 29 | +| `critical_count > 0` | 28.6% | 12.9% | 4 | 10 | +| `fast_revert_24h` | 100.0% | 25.8% | 8 | 0 | +| `self_reverted` | 100.0% | 9.7% | 3 | 0 | + +`fast_revert_24h` and `self_reverted` have 100% precision but are +**post-merge signals** — they cannot be used as triage gates because they are +only observable after the PR is already merged and reverted. They confirm the +problem exists but don't help prevent it. + +`critical_count > 0` was initially thought to be a strong signal (the bot +flagged the exact root cause in case studies like PR #6866), but after +correcting the regex to only match `**[Critical]**` tags (not the bare word +"critical" in prose like "no critical blockers"), the precision dropped to +28.6%. The bot is too trigger-happy with Critical findings — 16.7% of +control-group PRs also have Critical tags. + +### High-risk path definition + +The `touches_high_risk` signal checks whether any changed file matches these +subsystem patterns: + +- `openaiContentGenerator` — streaming response parsing +- `streamingToolCallParser` — tool call stream parsing +- `geminiChat` — Gemini chat pipeline +- `acpConnection` — ACP process spawning +- `shell.ts` / `shellExecutionService` — shell tool execution +- `mcp-client` / `mcp-pool` — MCP server management +- `LspServer` — LSP server management +- `acp-integration` — ACP session integration +- `ELECTRON_RUN_AS_NODE` / `process.env` — environment variable propagation + +These are the paths where incorrect changes are most likely to cause +observable regressions that require reversion. + +### Merge→revert time gap + +Of 13 PRs with valid (non-negative, post-merge) gap data: + +- Median: 4 hours +- Within 24h: 61.5% +- Within 72h: 84.6% +- Max: 97 hours + +This confirms that revert-causing defects surface quickly after merge, but +the damage is already on `main`. + +### Flip-flop PRs + +10 PRs were reverted multiple times (revert → re-revert cycles), indicating +unresolved contention: + +- PR #6754 (3 reverts), PR #6751 (3 reverts), PR #3433 (3 reverts) +- PR #6869 (2 reverts), PR #5668 (2 reverts), PR #3567 (2 reverts), + PR #3478 (2 reverts), PR #5060 (2 reverts) + +These flip-flop PRs are the highest-cost outcomes — they consume multiple +review rounds, multiple merge/revert cycles, and often require patch releases. + +## Design + +### Rule 1: High-risk path escalation (precision 66.7%, recall 32.3%) + +When a non-maintainer PR touches any high-risk path (see definition above), +Stage 1 triage escalates the PR to the deepest review tier instead of the +normal path. This does **not** block or close the PR — it ensures the full +`/review` pipeline runs with maximum agent coverage. + +This is the strongest triage-time signal: 66.7% of PRs touching these paths +were reverted, vs 8.3% of control-group PRs. + +Implementation: the Stage 1e skill text instructs the triage model to run +`gh pr view --json files | grep -E '...'` against the high-risk path patterns. +No workflow YAML change is needed — the detection runs inside the skill, +not as a separate workflow step. + +### Rule 2: Contested-merge detection (precision 50.0%, recall 19.4%) + +When a PR has a CHANGES_REQUESTED → APPROVE cycle in its review history and +touches core paths, Stage 1 applies `status/on-hold` and posts a summary +recommending maintainer sign-off before merge. + +This targets the flip-flop pattern: PRs that went through multiple review +rounds with disagreements are 50% likely to be reverted (vs ~20% baseline for +contested PRs in the control group). + +Implementation: the Stage 1e skill text instructs the triage model to fetch +the PR's review state sequence via `gh pr view --json reviews` and check for +a `CHANGES_REQUESTED` entry after position 0. If found alongside core-path +changes, the classifier applies `status/on-hold` and recommends maintainer +sign-off. No workflow YAML change is needed. + +### Rule 3: Non-maintainer + high-risk tier (precision 58.3%, recall 22.6%) + +A non-maintainer PR touching high-risk paths gets the highest-risk tier: +full `/review` depth + `status/on-hold` until a maintainer reviews. This +is Rule 1 + Rule 2 combined, targeting the intersection that has 58.3% +precision. + +This replaces PR #7414's behavior-neutral filter (2% recall) with a signal +that has 22.6% recall — over 10× more effective at catching dangerous PRs. + +### What this design does NOT do + +- **Does not auto-close or auto-reject PRs.** The gate escalates review depth + and recommends maintainer attention; it never blocks merge or closes the PR. +- **Does not use bot Critical findings as a signal.** The data shows 28.6% + precision — the bot flags Criticals on 16.7% of safe PRs too. Criticals are + too noisy to gate on. +- **Does not filter by PR size alone.** `large_diff_gt_200` has 37.0% + precision — size without context is not predictive. +- **Does not require E2E verification for all PRs.** `no_e2e` has 13.0% + precision because 100% of the control group also lacks E2E comments. + +## Comparison to PR #7414 + +| | PR #7414 (behavior-neutral) | This design (revert-pattern) | +| --- | --- | --- | +| Signal | "diff is entirely behavior-neutral" | "touches high-risk paths" | +| Hit rate (reverted PRs) | ~2% (2/102 live backlog) | 32.3% (10/31) | +| Precision | unmeasured (no reverts to compare) | 66.7% | +| Targets | harmless PRs (cost: low) | dangerous PRs (cost: high) | +| False positive cost | skips review on a useful PR | escalates review depth (extra review time) | + +## Files changed + +- `.qwen/skills/triage/references/pr-workflow.md` — add Stage 1e high-risk path + checklist, contested-merge detection, and non-maintainer + high-risk tier + instructions. The detection runs inside the triage skill (the model runs + `gh pr view --json files | grep …` and `gh pr view --json reviews` itself), + so no workflow YAML change is needed. +- `scripts/tests/qwen-triage-workflow.test.js` — assert the high-risk path, + contested-merge, and non-maintainer + high-risk routing strings exist in + the triage skill markdown. + +## Non-goals / follow-ups + +- **Bot Critical refinement.** The current bot Critical detection is too noisy + (28.6% precision). If the bot could distinguish "unresolved Critical" from + "resolved Critical" (by checking whether the finding thread was marked + resolved), the signal might become useful. This is a separate bot + improvement, not a triage gate change. +- **Time-aligned control group.** The current control group is sampled from + the 200 most recently merged PRs, but reverted PRs span 2025–2026. A + time-aligned control group would give more precise false-positive rates. + The `gh pr list` API doesn't support deep pagination, so this requires a + GraphQL cursor-based fetch. +- **Recovering 15 deleted PRs.** 15 of 46 reverted PRs are deleted and + inaccessible via the GitHub API. Their patterns may differ from the 31 + we could enrich. No recovery path exists — GitHub permanently deletes + closed PRs in certain states. +- **Flip-flop detection as a real-time gate.** The current analysis detects + flip-flops retrospectively (after multiple reverts). A real-time version + would monitor for revert→re-revert patterns on `main` and alert maintainers. + This requires a separate monitoring workflow, not a triage gate. +- **Expanding the high-risk path list.** The current list is manually curated + from the reverted PR file paths. As the codebase evolves, new high-risk + paths may emerge. A periodic re-run of the analysis scripts would keep the + list current. diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index d7d9153d626..4a4102241fe 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -653,6 +653,47 @@ describe('qwen-triage tmux workflow', () => { expect(noHead.comment).toContain('could not be read'); }, ); + + it('includes high-risk path detection in the triage skill', () => { + const prWorkflow = readFileSync( + '.qwen/skills/triage/references/pr-workflow.md', + 'utf8', + ); + + expect(prWorkflow).toContain('1e. High-risk path'); + expect(prWorkflow).toContain('openaiContentGenerator'); + expect(prWorkflow).toContain('streamingToolCallParser'); + expect(prWorkflow).toContain('acpConnection'); + expect(prWorkflow).toContain('shellExecutionService'); + expect(prWorkflow).toContain('mcp-client'); + expect(prWorkflow).toContain('LspServer'); + expect(prWorkflow).toContain('66.7% revert precision'); + expect(prWorkflow).toContain('deepest review tier'); + }); + + it('includes contested-merge detection in the triage skill', () => { + const prWorkflow = readFileSync( + '.qwen/skills/triage/references/pr-workflow.md', + 'utf8', + ); + + expect(prWorkflow).toContain('Contested-merge pattern'); + expect(prWorkflow).toContain('CHANGES_REQUESTED'); + expect(prWorkflow).toContain('50.0% revert precision'); + expect(prWorkflow).toContain('status/on-hold'); + expect(prWorkflow).toContain('maintainer sign-off'); + }); + + it('includes non-maintainer high-risk tier in the triage skill', () => { + const prWorkflow = readFileSync( + '.qwen/skills/triage/references/pr-workflow.md', + 'utf8', + ); + + expect(prWorkflow).toContain('Non-maintainer + high-risk'); + expect(prWorkflow).toContain('58.3% revert precision'); + expect(prWorkflow).toContain('highest-risk tier'); + }); }); describe('qwen-triage verify workflow', () => { From 07870df8f0c263d748ed37d3f4881e6a19fa6fab Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 27 Jul 2026 20:53:35 +0800 Subject: [PATCH 02/11] fix(triage): avoid stale-exempt hold label --- .qwen/skills/triage/references/pr-workflow.md | 2 +- docs/design/2026-07-27-revert-pattern-triage-gate.md | 6 +++--- scripts/tests/qwen-triage-workflow.test.js | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 2f96b22357f..ebb3eb59cf7 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -249,7 +249,7 @@ gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select ``` If `CHANGES_REQUESTED` appears after the first review (not at position 0) AND the PR touches core paths (50.0% revert precision, 19.4% recall): -- Apply `status/on-hold` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label status/on-hold` (if the label exists). +- Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). - Recommend maintainer sign-off before merge in the Stage 1 comment. - Do not auto-approve even if Stage 2 and Stage 3 are clean. diff --git a/docs/design/2026-07-27-revert-pattern-triage-gate.md b/docs/design/2026-07-27-revert-pattern-triage-gate.md index bc67211d099..4b96cd52b14 100644 --- a/docs/design/2026-07-27-revert-pattern-triage-gate.md +++ b/docs/design/2026-07-27-revert-pattern-triage-gate.md @@ -136,7 +136,7 @@ not as a separate workflow step. ### Rule 2: Contested-merge detection (precision 50.0%, recall 19.4%) When a PR has a CHANGES_REQUESTED → APPROVE cycle in its review history and -touches core paths, Stage 1 applies `status/on-hold` and posts a summary +touches core paths, Stage 1 applies `need-discussion` and posts a summary recommending maintainer sign-off before merge. This targets the flip-flop pattern: PRs that went through multiple review @@ -146,13 +146,13 @@ contested PRs in the control group). Implementation: the Stage 1e skill text instructs the triage model to fetch the PR's review state sequence via `gh pr view --json reviews` and check for a `CHANGES_REQUESTED` entry after position 0. If found alongside core-path -changes, the classifier applies `status/on-hold` and recommends maintainer +changes, the classifier applies `need-discussion` and recommends maintainer sign-off. No workflow YAML change is needed. ### Rule 3: Non-maintainer + high-risk tier (precision 58.3%, recall 22.6%) A non-maintainer PR touching high-risk paths gets the highest-risk tier: -full `/review` depth + `status/on-hold` until a maintainer reviews. This +full `/review` depth + `need-discussion` until a maintainer reviews. This is Rule 1 + Rule 2 combined, targeting the intersection that has 58.3% precision. diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 4a4102241fe..061dcb3b7e1 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -680,7 +680,8 @@ describe('qwen-triage tmux workflow', () => { expect(prWorkflow).toContain('Contested-merge pattern'); expect(prWorkflow).toContain('CHANGES_REQUESTED'); expect(prWorkflow).toContain('50.0% revert precision'); - expect(prWorkflow).toContain('status/on-hold'); + expect(prWorkflow).toContain('need-discussion'); + expect(prWorkflow).not.toContain('status/on-hold'); expect(prWorkflow).toContain('maintainer sign-off'); }); From fe9b017e953bdf96226f40205dd31a3809f20085 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 27 Jul 2026 23:50:43 +0800 Subject: [PATCH 03/11] fix(triage): address review risk detection feedback --- .qwen/skills/triage/references/pr-workflow.md | 6 +- scripts/tests/qwen-triage-workflow.test.js | 56 ++++++++----------- 2 files changed, 26 insertions(+), 36 deletions(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index ebb3eb59cf7..f3e95b29b9d 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -234,7 +234,7 @@ A revert-history analysis of this repo (111 revert commits, 46 unique reverted P **High-risk paths** — check the PR's changed files against these patterns: ```bash -gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|ELECTRON_RUN_AS_NODE' +gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|relaunch\.ts|sandbox\.ts|electron-run-as-node' ``` If any file matches (66.7% revert precision, 32.3% recall in the analysis): @@ -248,12 +248,12 @@ If any file matches (66.7% revert precision, 32.3% recall in the analysis): gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | .state]' ``` -If `CHANGES_REQUESTED` appears after the first review (not at position 0) AND the PR touches core paths (50.0% revert precision, 19.4% recall): +If the review state array contains both `CHANGES_REQUESTED` and `APPROVE` entries (in any order) AND the PR touches core paths (50.0% revert precision, 19.4% recall): - Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). - Recommend maintainer sign-off before merge in the Stage 1 comment. - Do not auto-approve even if Stage 2 and Stage 3 are clean. -**Non-maintainer + high-risk** (58.3% revert precision, 22.6% recall): the intersection of both signals above is the highest-risk tier. Apply both actions: deepest review depth AND recommend maintainer sign-off. +**Non-maintainer + high-risk** (58.3% revert precision, 22.6% recall): a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions: deepest review depth, recommend E2E verification, apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 061dcb3b7e1..16e3078efca 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -655,45 +655,35 @@ describe('qwen-triage tmux workflow', () => { ); it('includes high-risk path detection in the triage skill', () => { - const prWorkflow = readFileSync( - '.qwen/skills/triage/references/pr-workflow.md', - 'utf8', - ); - - expect(prWorkflow).toContain('1e. High-risk path'); - expect(prWorkflow).toContain('openaiContentGenerator'); - expect(prWorkflow).toContain('streamingToolCallParser'); - expect(prWorkflow).toContain('acpConnection'); - expect(prWorkflow).toContain('shellExecutionService'); - expect(prWorkflow).toContain('mcp-client'); - expect(prWorkflow).toContain('LspServer'); - expect(prWorkflow).toContain('66.7% revert precision'); - expect(prWorkflow).toContain('deepest review tier'); + expect(prSkill).toContain('1e. High-risk path'); + expect(prSkill).toContain('openaiContentGenerator'); + expect(prSkill).toContain('streamingToolCallParser'); + expect(prSkill).toContain('acpConnection'); + expect(prSkill).toContain('shellExecutionService'); + expect(prSkill).toContain('mcp-client'); + expect(prSkill).toContain('LspServer'); + expect(prSkill).toContain('relaunch\\.ts'); + expect(prSkill).toContain('sandbox\\.ts'); + expect(prSkill).toContain('66.7% revert precision'); + expect(prSkill).toContain('deepest review tier'); }); it('includes contested-merge detection in the triage skill', () => { - const prWorkflow = readFileSync( - '.qwen/skills/triage/references/pr-workflow.md', - 'utf8', - ); - - expect(prWorkflow).toContain('Contested-merge pattern'); - expect(prWorkflow).toContain('CHANGES_REQUESTED'); - expect(prWorkflow).toContain('50.0% revert precision'); - expect(prWorkflow).toContain('need-discussion'); - expect(prWorkflow).not.toContain('status/on-hold'); - expect(prWorkflow).toContain('maintainer sign-off'); + expect(prSkill).toContain('Contested-merge pattern'); + expect(prSkill).toContain('CHANGES_REQUESTED'); + expect(prSkill).toContain('APPROVE'); + expect(prSkill).toContain('in any order'); + expect(prSkill).toContain('50.0% revert precision'); + expect(prSkill).toContain('need-discussion'); + expect(prSkill).not.toContain('status/on-hold'); + expect(prSkill).toContain('maintainer sign-off'); }); it('includes non-maintainer high-risk tier in the triage skill', () => { - const prWorkflow = readFileSync( - '.qwen/skills/triage/references/pr-workflow.md', - 'utf8', - ); - - expect(prWorkflow).toContain('Non-maintainer + high-risk'); - expect(prWorkflow).toContain('58.3% revert precision'); - expect(prWorkflow).toContain('highest-risk tier'); + expect(prSkill).toContain('Non-maintainer + high-risk'); + expect(prSkill).toContain('58.3% revert precision'); + expect(prSkill).toContain('highest-risk tier'); + expect(prSkill).toContain('do not auto-approve'); }); }); From 7cb1eddf49b3a7d6b72a5cce876a140bf6925bfc Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 28 Jul 2026 00:45:23 +0800 Subject: [PATCH 04/11] fix(triage): tighten high-risk path patterns --- .qwen/skills/triage/references/pr-workflow.md | 2 +- scripts/tests/qwen-triage-workflow.test.js | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index f3e95b29b9d..9415b86f366 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -234,7 +234,7 @@ A revert-history analysis of this repo (111 revert commits, 46 unique reverted P **High-risk paths** — check the PR's changed files against these patterns: ```bash -gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|relaunch\.ts|sandbox\.ts|electron-run-as-node' +gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|relaunch\.ts$|sandbox\.ts$|electron-run-as-node' ``` If any file matches (66.7% revert precision, 32.3% recall in the analysis): diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 16e3078efca..740741011fa 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -658,12 +658,17 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('1e. High-risk path'); expect(prSkill).toContain('openaiContentGenerator'); expect(prSkill).toContain('streamingToolCallParser'); + expect(prSkill).toContain('geminiChat'); expect(prSkill).toContain('acpConnection'); + expect(prSkill).toContain('shell\\.ts$'); expect(prSkill).toContain('shellExecutionService'); expect(prSkill).toContain('mcp-client'); + expect(prSkill).toContain('mcp-pool'); expect(prSkill).toContain('LspServer'); - expect(prSkill).toContain('relaunch\\.ts'); - expect(prSkill).toContain('sandbox\\.ts'); + expect(prSkill).toContain('acp-integration'); + expect(prSkill).toContain('relaunch\\.ts$'); + expect(prSkill).toContain('sandbox\\.ts$'); + expect(prSkill).toContain('electron-run-as-node'); expect(prSkill).toContain('66.7% revert precision'); expect(prSkill).toContain('deepest review tier'); }); From 7b022cfbdf83129a7f37960df384484385d0ac44 Mon Sep 17 00:00:00 2001 From: qwen-code-bot Date: Mon, 27 Jul 2026 18:16:21 +0000 Subject: [PATCH 05/11] fix(triage): address review feedback on Stage 1e revert-pattern gate (#7414) --- .github/scripts/qwen-triage-workflow.test.mjs | 48 +++++++++++++++++ .qwen/skills/triage/references/pr-workflow.md | 16 ++++-- .../2026-07-27-revert-pattern-triage-gate.md | 51 ++++++++++--------- scripts/tests/qwen-triage-workflow.test.js | 6 +-- 4 files changed, 88 insertions(+), 33 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 2a1f7220ca2..b5a3d759997 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -21,6 +21,17 @@ const workflowPath = join( 'qwen-triage.yml', ); const doc = parse(readFileSync(workflowPath, 'utf8')); +const prWorkflowPath = join( + dirname(fileURLToPath(import.meta.url)), + '..', + '..', + '.qwen', + 'skills', + 'triage', + 'references', + 'pr-workflow.md', +); +const prSkill = readFileSync(prWorkflowPath, 'utf8'); const triageJob = doc.jobs.triage; const steps = triageJob.steps; const triageStep = steps.find((s) => s.id === 'triage'); @@ -259,3 +270,40 @@ describe('qwen-triage: git exec-vector cleanup', () => { } }); }); + +describe('qwen-triage: Stage 1e revert-pattern signals', () => { + it('includes high-risk path detection', () => { + assert.ok(prSkill.includes('1e. High-risk path')); + assert.ok(prSkill.includes('openaiContentGenerator')); + assert.ok(prSkill.includes('streamingToolCallParser')); + assert.ok(prSkill.includes('geminiChat')); + assert.ok(prSkill.includes('acpConnection')); + assert.ok(prSkill.includes('shell\\.ts$')); + assert.ok(prSkill.includes('shellExecutionService')); + assert.ok(prSkill.includes('mcp-client')); + assert.ok(prSkill.includes('mcp-pool')); + assert.ok(prSkill.includes('LspServer')); + assert.ok(prSkill.includes('acp-integration')); + assert.ok(prSkill.includes('relaunch\\.ts$')); + assert.ok(prSkill.includes('sandbox\\.ts$')); + assert.ok(prSkill.includes('electron-run-as-node')); + assert.ok(prSkill.includes('66.7% revert precision')); + }); + + it('includes contested-merge detection', () => { + assert.ok(prSkill.includes('Contested-merge pattern')); + assert.ok(prSkill.includes('CHANGES_REQUESTED')); + assert.ok(prSkill.includes('after position 0')); + assert.ok(prSkill.includes('50.0% revert precision')); + assert.ok(prSkill.includes('need-discussion')); + assert.ok(!prSkill.includes('status/on-hold')); + assert.ok(prSkill.includes('maintainer sign-off')); + }); + + it('includes non-maintainer high-risk tier', () => { + assert.ok(prSkill.includes('Non-maintainer + high-risk')); + assert.ok(prSkill.includes('58.3% revert precision')); + assert.ok(prSkill.includes('highest-risk tier')); + assert.ok(prSkill.includes('do not auto-approve')); + }); +}); diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 9415b86f366..e7d145e9c64 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -86,7 +86,7 @@ gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \ **Approve once per commit — and only your OWN approval counts as already done.** Re-running triage three times must not stack three approvals, so check before posting. But "already approved" means **this bot's** `APPROVED` review on **this exact** `HEAD_SHA`, and nothing else: -- **Another account's approval is not yours.** `main` requires two approving reviews, so a maintainer's approval is a *different* vote — the whole reason the bot's is still needed. Never read it as "already approved". +- **Another account's approval is not yours.** `main` requires two approving reviews, so a maintainer's approval is a _different_ vote — the whole reason the bot's is still needed. Never read it as "already approved". - **A `DISMISSED` review is not an approval.** Branch protection runs with `dismiss_stale_reviews: true`, so every push dismisses the bot's prior approval. That is precisely when a fresh one is required. - **An approval on an earlier commit does not carry over**, for the same reason. @@ -238,22 +238,24 @@ gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files[].path' | grep ``` If any file matches (66.7% revert precision, 32.3% recall in the analysis): + - For non-maintainer PRs: escalate to the deepest review tier — do not skip any review stage, run full `/review` with maximum agent coverage. - Flag the high-risk paths in the Stage 1 comment so the reviewer knows where to focus. -- Recommend E2E verification in tmux (Stage 2b) before approval. +- Recommend E2E verification in tmux (Stage 2c) before approval. -**Contested-merge pattern** — check the PR's review history for a CHANGES_REQUESTED → APPROVE cycle: +**Contested-merge pattern** — check the PR's review history for reviewer disagreement (a CHANGES_REQUESTED entry that is not the first review): ```bash gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | .state]' ``` -If the review state array contains both `CHANGES_REQUESTED` and `APPROVE` entries (in any order) AND the PR touches core paths (50.0% revert precision, 19.4% recall): +If the review state array has a `CHANGES_REQUESTED` entry after position 0 (a prior review preceded the disagreement) AND the PR touches core paths as defined in Stage 0 (50.0% revert precision, 19.4% recall): + - Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). - Recommend maintainer sign-off before merge in the Stage 1 comment. - Do not auto-approve even if Stage 2 and Stage 3 are clean. -**Non-maintainer + high-risk** (58.3% revert precision, 22.6% recall): a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions: deepest review depth, recommend E2E verification, apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. +**Non-maintainer + high-risk** (58.3% revert precision, 22.6% recall): a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions: deepest review depth, flag the high-risk paths in the Stage 1 comment, recommend E2E verification, apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. @@ -274,6 +276,8 @@ Size: . +Risk: + Moving on to code review. 🔍 Flagging these for discussion before diving deeper. @@ -292,6 +296,8 @@ Approach: 。<如果看到更简路径，点名：有没有考虑过直接 X？可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构，点名并建议拆成单独 PR。> +风险：<如果 Stage 1e 信号命中，列出匹配的高风险路径和/或 contested-merge 模式、建议的 review 深度、是否需要维护者签字。否则写"无升级风险信号"。> + <如果通过：> 进入代码审查 🔍 <如果有顾虑：> 先提出来讨论，再深入看代码。 diff --git a/docs/design/2026-07-27-revert-pattern-triage-gate.md b/docs/design/2026-07-27-revert-pattern-triage-gate.md index 4b96cd52b14..476a7fc82d1 100644 --- a/docs/design/2026-07-27-revert-pattern-triage-gate.md +++ b/docs/design/2026-07-27-revert-pattern-triage-gate.md @@ -42,25 +42,26 @@ Three-phase analysis of the repo's full revert history: PRs and extracted the same signals. Computed precision (TP / (TP + FP)) and recall for each signal. -Scripts and raw data: `.qwen/scripts/revert-analysis-*.mjs`, -`.qwen/scripts/revert-data-*.json`, `.qwen/scripts/revert-analysis-report-v2.json`. +Scripts and raw data (local analysis artifacts, not committed): +`.qwen/scripts/revert-analysis-*.mjs`, `.qwen/scripts/revert-data-*.json`, +`.qwen/scripts/revert-analysis-report-v2.json`. ### Signal precision and recall -| Signal | Precision | Recall | Reverted (n=31) | Control (n=60) | -| ------ | --------- | ------ | --------------- | -------------- | -| `touches_high_risk` | **66.7%** | 32.3% | 10 | 5 | -| `non_maintainer + high_risk` | **58.3%** | 22.6% | 7 | 5 | -| `core + contested` | **50.0%** | 19.4% | 6 | 6 | -| `non_maintainer + core` | 46.2% | 38.7% | 12 | 14 | -| `touches_core` | 44.7% | 54.8% | 17 | 21 | -| `has_contested_pattern` | 40.9% | 29.0% | 9 | 13 | -| `had_changes_requested` | 40.7% | 35.5% | 11 | 16 | -| `non_maintainer` | 39.6% | 67.7% | 21 | 32 | -| `large_diff_gt_200` | 37.0% | 54.8% | 17 | 29 | -| `critical_count > 0` | 28.6% | 12.9% | 4 | 10 | -| `fast_revert_24h` | 100.0% | 25.8% | 8 | 0 | -| `self_reverted` | 100.0% | 9.7% | 3 | 0 | +| Signal | Precision | Recall | Reverted (n=31) | Control (n=60) | +| ---------------------------- | --------- | ------ | --------------- | -------------- | +| `touches_high_risk` | **66.7%** | 32.3% | 10 | 5 | +| `non_maintainer + high_risk` | **58.3%** | 22.6% | 7 | 5 | +| `core + contested` | **50.0%** | 19.4% | 6 | 6 | +| `non_maintainer + core` | 46.2% | 38.7% | 12 | 14 | +| `touches_core` | 44.7% | 54.8% | 17 | 21 | +| `has_contested_pattern` | 40.9% | 29.0% | 9 | 13 | +| `had_changes_requested` | 40.7% | 35.5% | 11 | 16 | +| `non_maintainer` | 39.6% | 67.7% | 21 | 32 | +| `large_diff_gt_200` | 37.0% | 54.8% | 17 | 29 | +| `critical_count > 0` | 28.6% | 12.9% | 4 | 10 | +| `fast_revert_24h` | 100.0% | 25.8% | 8 | 0 | +| `self_reverted` | 100.0% | 9.7% | 3 | 0 | `fast_revert_24h` and `self_reverted` have 100% precision but are **post-merge signals** — they cannot be used as triage gates because they are @@ -87,7 +88,9 @@ subsystem patterns: - `mcp-client` / `mcp-pool` — MCP server management - `LspServer` — LSP server management - `acp-integration` — ACP session integration -- `ELECTRON_RUN_AS_NODE` / `process.env` — environment variable propagation +- `relaunch.ts` — desktop app relaunch lifecycle +- `sandbox.ts` — sandbox process management +- `electron-run-as-node` — Electron node-mode entry point (path match) These are the paths where incorrect changes are most likely to cause observable regressions that require reversion. @@ -173,13 +176,13 @@ that has 22.6% recall — over 10× more effective at catching dangerous PRs. ## Comparison to PR #7414 -| | PR #7414 (behavior-neutral) | This design (revert-pattern) | -| --- | --- | --- | -| Signal | "diff is entirely behavior-neutral" | "touches high-risk paths" | -| Hit rate (reverted PRs) | ~2% (2/102 live backlog) | 32.3% (10/31) | -| Precision | unmeasured (no reverts to compare) | 66.7% | -| Targets | harmless PRs (cost: low) | dangerous PRs (cost: high) | -| False positive cost | skips review on a useful PR | escalates review depth (extra review time) | +| | PR #7414 (behavior-neutral) | This design (revert-pattern) | +| ----------------------- | ----------------------------------- | ------------------------------------------ | +| Signal | "diff is entirely behavior-neutral" | "touches high-risk paths" | +| Hit rate (reverted PRs) | ~2% (2/102 live backlog) | 32.3% (10/31) | +| Precision | unmeasured (no reverts to compare) | 66.7% | +| Targets | harmless PRs (cost: low) | dangerous PRs (cost: high) | +| False positive cost | skips review on a useful PR | escalates review depth (extra review time) | ## Files changed diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 740741011fa..19024d9dfd5 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -328,9 +328,7 @@ describe('qwen-triage tmux workflow', () => { const finalizeStep = step('Finalize triage status comment'); // Runs on both outcomes and edits the SAME marker comment (no second post). - expect(finalizeStep).toContain( - "MARKER=''", - ); + expect(finalizeStep).toContain("MARKER=''"); expect(finalizeStep).toContain( "LEGACY_MARKER=''", ); @@ -677,7 +675,7 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('Contested-merge pattern'); expect(prSkill).toContain('CHANGES_REQUESTED'); expect(prSkill).toContain('APPROVE'); - expect(prSkill).toContain('in any order'); + expect(prSkill).toContain('after position 0'); expect(prSkill).toContain('50.0% revert precision'); expect(prSkill).toContain('need-discussion'); expect(prSkill).not.toContain('status/on-hold'); From b596fafb343791d2040fbb041478ff370f53b959 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 27 Jul 2026 19:58:15 +0000 Subject: [PATCH 06/11] fix(triage): address round-2 review feedback on Stage 1e gate (#7414) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix APPROVE → APPROVED state name (GitHub API enum) - Use gh api --paginate for file listing (fixes 100-file truncation) - Anchor shell/relaunch/sandbox patterns with (^|/) to avoid false positives - Append || true to grep (exit 1 on no match is the 92% case) - Scope E2E recommendation to write-access authors per Stage 2c - Add bot author filter to contested-merge query - Define core paths explicitly in contested-merge condition - Wire Stage 1e do-not-auto-approve into Stage 3 guardrail - Replace precision percentages with p-values/raw counts in skill text - Add sampling caveat and statistical significance notes to design doc - Fix design doc errors: 71%→61.5%, 10→8 PRs, Rule 3 attribution, ~20% baseline→10% prevalence, Area field, no_e2e inconsistency - Make test assertions specific to Stage 1e (not vacuous) - Add Risk: template field assertion - Revert drive-by prettier reflow - Note need-discussion label removal by maintainer --- .github/scripts/qwen-triage-workflow.test.mjs | 23 +++++-- .qwen/skills/triage/references/pr-workflow.md | 26 +++---- .../2026-07-27-revert-pattern-triage-gate.md | 68 +++++++++++++------ scripts/tests/qwen-triage-workflow.test.js | 27 +++++--- 4 files changed, 95 insertions(+), 49 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index b5a3d759997..6d5c6fc4c2c 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -278,32 +278,41 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('streamingToolCallParser')); assert.ok(prSkill.includes('geminiChat')); assert.ok(prSkill.includes('acpConnection')); - assert.ok(prSkill.includes('shell\\.ts$')); + assert.ok(prSkill.includes('(^|/)shell\\.ts$')); assert.ok(prSkill.includes('shellExecutionService')); assert.ok(prSkill.includes('mcp-client')); assert.ok(prSkill.includes('mcp-pool')); assert.ok(prSkill.includes('LspServer')); assert.ok(prSkill.includes('acp-integration')); - assert.ok(prSkill.includes('relaunch\\.ts$')); - assert.ok(prSkill.includes('sandbox\\.ts$')); + assert.ok(prSkill.includes('(^|/)relaunch\\.ts$')); + assert.ok(prSkill.includes('(^|/)sandbox\\.ts$')); assert.ok(prSkill.includes('electron-run-as-node')); - assert.ok(prSkill.includes('66.7% revert precision')); + assert.ok(prSkill.includes('p = 0.006')); + assert.ok(prSkill.includes('do not skip any Stage 2 enrichment')); + assert.ok(prSkill.includes('gh api --paginate')); + assert.ok(prSkill.includes('|| true')); }); it('includes contested-merge detection', () => { assert.ok(prSkill.includes('Contested-merge pattern')); - assert.ok(prSkill.includes('CHANGES_REQUESTED')); + assert.ok(prSkill.includes('`CHANGES_REQUESTED` entry after position 0')); assert.ok(prSkill.includes('after position 0')); - assert.ok(prSkill.includes('50.0% revert precision')); assert.ok(prSkill.includes('need-discussion')); assert.ok(!prSkill.includes('status/on-hold')); assert.ok(prSkill.includes('maintainer sign-off')); + assert.ok(prSkill.includes('qwen-code-ci-bot')); + assert.ok(prSkill.includes('packages/core/src/**')); + assert.ok(prSkill.includes('A maintainer removes it once the discussion resolves')); }); it('includes non-maintainer high-risk tier', () => { assert.ok(prSkill.includes('Non-maintainer + high-risk')); - assert.ok(prSkill.includes('58.3% revert precision')); assert.ok(prSkill.includes('highest-risk tier')); assert.ok(prSkill.includes('do not auto-approve')); + assert.ok(prSkill.includes('Stage 3 approval guardrail')); + }); + + it('includes Risk field in the Stage 1 comment template', () => { + assert.ok(prSkill.includes('Risk: `" — and include it on its own line (full OID, the same one the footer attests): @@ -647,6 +647,8 @@ If `GUARD` is `block`: do **not** run `gh pr review --approve` no matter how cle If Stage 0 escalated the PR for maintainer awareness, do **not** approve automatically; use the "Genuinely unsure" path below. +If Stage 1e flagged a contested-merge or non-maintainer + high-risk signal, do **not** approve automatically; use the "Genuinely unsure" path below. A Stage 1e flag is a data-backed risk signal, not a hygiene concern — the re-run rule below does not override it. + **Re-runs (manually triggered via `@qwen-code /triage`):** hygiene concerns (scope mismatch, undocumented changes, naming) that don't block the PR are not a valid reason to defer. Note them in the comment and approve. Only defer if you have genuine blocking uncertainty — something you cannot resolve from the diff, tests, and PR description. All stages genuinely clean, `GUARD` is `ok`, and no Stage 0 maintainer escalation remains — how you approve depends on the `PENDING` count computed in Step 1: diff --git a/docs/design/2026-07-27-revert-pattern-triage-gate.md b/docs/design/2026-07-27-revert-pattern-triage-gate.md index 476a7fc82d1..b0917bed1d4 100644 --- a/docs/design/2026-07-27-revert-pattern-triage-gate.md +++ b/docs/design/2026-07-27-revert-pattern-triage-gate.md @@ -2,7 +2,7 @@ Date: 2026-07-27 Status: Proposed -Area: CI triage — `.github/workflows/qwen-code-pr-review.yml`, `.qwen/skills/triage/` +Area: CI triage — `.github/workflows/qwen-triage.yml`, `.qwen/skills/triage/` ## Problem @@ -13,7 +13,7 @@ live backlog showed only a ~2% hit rate — the feature targeted a near-nonexist problem. Meanwhile, the repo has **111 revert commits** across its history (19 in July -2026 alone), and **71% of reverts occur within 24 hours of merge** — meaning +2026 alone), and **61.5% of reverts occur within 24 hours of merge** — meaning the problem is caught quickly but after it's already on `main`. The real cost is not reviewing harmless PRs; it's merging PRs that have to be rolled back. @@ -63,6 +63,16 @@ Scripts and raw data (local analysis artifacts, not committed): | `fast_revert_24h` | 100.0% | 25.8% | 8 | 0 | | `self_reverted` | 100.0% | 9.7% | 3 | 0 | +**Sampling caveat:** precision is computed on a 1:1.9 case-control ratio (31 +reverted vs 60 control), while the repo's actual base rate is ~1.37% (46/3358). +Precision (PPV) is the metric most sensitive to this enrichment — the true +positive predictive value at the repo base rate is much lower (e.g. ~5% for +`touches_high_risk`). Sensitivity (recall) and specificity are invariant to +the sampling ratio and are the appropriate metrics for comparing signals. +The _ranking_ of signals by precision is still valid (it is monotone in the +likelihood ratio at fixed n), but the absolute values should not be quoted +to contributors as posterior probabilities. + `fast_revert_24h` and `self_reverted` have 100% precision but are **post-merge signals** — they cannot be used as triage gates because they are only observable after the PR is already merged and reverted. They confirm the @@ -109,7 +119,7 @@ the damage is already on `main`. ### Flip-flop PRs -10 PRs were reverted multiple times (revert → re-revert cycles), indicating +8 PRs were reverted multiple times (revert → re-revert cycles), indicating unresolved contention: - PR #6754 (3 reverts), PR #6751 (3 reverts), PR #3433 (3 reverts) @@ -128,8 +138,9 @@ Stage 1 triage escalates the PR to the deepest review tier instead of the normal path. This does **not** block or close the PR — it ensures the full `/review` pipeline runs with maximum agent coverage. -This is the strongest triage-time signal: 66.7% of PRs touching these paths -were reverted, vs 8.3% of control-group PRs. +This is the strongest triage-time signal: 10 of 31 reverted PRs (32.3% +sensitivity) touched these paths, vs 5 of 60 control PRs (91.7% specificity; +Fisher p = 0.006). Implementation: the Stage 1e skill text instructs the triage model to run `gh pr view --json files | grep -E '...'` against the high-risk path patterns. @@ -138,13 +149,15 @@ not as a separate workflow step. ### Rule 2: Contested-merge detection (precision 50.0%, recall 19.4%) -When a PR has a CHANGES_REQUESTED → APPROVE cycle in its review history and +When a PR has a CHANGES_REQUESTED → APPROVED cycle in its review history and touches core paths, Stage 1 applies `need-discussion` and posts a summary recommending maintainer sign-off before merge. This targets the flip-flop pattern: PRs that went through multiple review -rounds with disagreements are 50% likely to be reverted (vs ~20% baseline for -contested PRs in the control group). +rounds with disagreements have 50% precision (6 of 12 such PRs reverted), +versus a 10% prevalence of the same signal in the control group (6/60). +Note: this signal is not statistically significant at n = 31 (Fisher +p = 0.33) — see the follow-up note below. Implementation: the Stage 1e skill text instructs the triage model to fetch the PR's review state sequence via `gh pr view --json reviews` and check for @@ -156,11 +169,14 @@ sign-off. No workflow YAML change is needed. A non-maintainer PR touching high-risk paths gets the highest-risk tier: full `/review` depth + `need-discussion` until a maintainer reviews. This -is Rule 1 + Rule 2 combined, targeting the intersection that has 58.3% -precision. +combines the `non_maintainer` and `touches_high_risk` signals (Rule 1's +high-risk path escalation restricted to non-maintainer authors), the +intersection measured at 58.3% precision in the table above. Note: this +signal does not reach significance at n = 31 (Fisher p = 0.098). -This replaces PR #7414's behavior-neutral filter (2% recall) with a signal -that has 22.6% recall — over 10× more effective at catching dangerous PRs. +This replaces PR #7414's behavior-neutral filter (~2% live-backlog hit +rate; revert recall unmeasured) with a signal whose revert recall is +measured at 22.6% — targeting the PRs that actually cause reverts. ### What this design does NOT do @@ -171,18 +187,19 @@ that has 22.6% recall — over 10× more effective at catching dangerous PRs. too noisy to gate on. - **Does not filter by PR size alone.** `large_diff_gt_200` has 37.0% precision — size without context is not predictive. -- **Does not require E2E verification for all PRs.** `no_e2e` has 13.0% - precision because 100% of the control group also lacks E2E comments. +- **Does not require E2E verification for all PRs.** `no_e2e` is not + discriminative — 100% of the control group also lacks E2E comments, so + the signal cannot distinguish revert-prone PRs from safe ones. ## Comparison to PR #7414 -| | PR #7414 (behavior-neutral) | This design (revert-pattern) | -| ----------------------- | ----------------------------------- | ------------------------------------------ | -| Signal | "diff is entirely behavior-neutral" | "touches high-risk paths" | -| Hit rate (reverted PRs) | ~2% (2/102 live backlog) | 32.3% (10/31) | -| Precision | unmeasured (no reverts to compare) | 66.7% | -| Targets | harmless PRs (cost: low) | dangerous PRs (cost: high) | -| False positive cost | skips review on a useful PR | escalates review depth (extra review time) | +| | PR #7414 (behavior-neutral) | This design (revert-pattern) | +| ------------------- | ----------------------------------- | ------------------------------------------ | +| Signal | "diff is entirely behavior-neutral" | "touches high-risk paths" | +| Revert recall | unmeasured (no reverts to compare) | 32.3% (10/31) | +| Specificity | n/a | 91.7% (55/60) | +| Targets | harmless PRs (cost: low) | dangerous PRs (cost: high) | +| False positive cost | skips review on a useful PR | escalates review depth (extra review time) | ## Files changed @@ -194,6 +211,8 @@ that has 22.6% recall — over 10× more effective at catching dangerous PRs. - `scripts/tests/qwen-triage-workflow.test.js` — assert the high-risk path, contested-merge, and non-maintainer + high-risk routing strings exist in the triage skill markdown. +- `.github/scripts/qwen-triage-workflow.test.mjs` — the same assertions in + the node:test runner. ## Non-goals / follow-ups @@ -215,6 +234,13 @@ that has 22.6% recall — over 10× more effective at catching dangerous PRs. flip-flops retrospectively (after multiple reverts). A real-time version would monitor for revert→re-revert patterns on `main` and alert maintainers. This requires a separate monitoring workflow, not a triage gate. +- **Statistical significance of Rules 2–3.** `core + contested` (Rule 2) + has Fisher p = 0.33 and `non_maintainer + high_risk` (Rule 3) has + p = 0.098 at n = 31 — neither reaches the 0.05 threshold. Only Rule 1 + (`touches_high_risk`, p = 0.006) is well-supported. Rules 2–3 are + shipped as escalate-only signals (they add review friction but never + block), but if a future re-analysis with more data does not confirm + them, they should be removed. - **Expanding the high-risk path list.** The current list is manually curated from the reverted PR file paths. As the codebase evolves, new high-risk paths may emerge. A periodic re-run of the analysis scripts would keep the diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 19024d9dfd5..ca4719ffa59 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -658,35 +658,44 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('streamingToolCallParser'); expect(prSkill).toContain('geminiChat'); expect(prSkill).toContain('acpConnection'); - expect(prSkill).toContain('shell\\.ts$'); + expect(prSkill).toContain('(^|/)shell\\.ts$'); expect(prSkill).toContain('shellExecutionService'); expect(prSkill).toContain('mcp-client'); expect(prSkill).toContain('mcp-pool'); expect(prSkill).toContain('LspServer'); expect(prSkill).toContain('acp-integration'); - expect(prSkill).toContain('relaunch\\.ts$'); - expect(prSkill).toContain('sandbox\\.ts$'); + expect(prSkill).toContain('(^|/)relaunch\\.ts$'); + expect(prSkill).toContain('(^|/)sandbox\\.ts$'); expect(prSkill).toContain('electron-run-as-node'); - expect(prSkill).toContain('66.7% revert precision'); - expect(prSkill).toContain('deepest review tier'); + expect(prSkill).toContain('p = 0.006'); + expect(prSkill).toContain('do not skip any Stage 2 enrichment'); + expect(prSkill).toContain('gh api --paginate'); + expect(prSkill).toContain('|| true'); }); it('includes contested-merge detection in the triage skill', () => { expect(prSkill).toContain('Contested-merge pattern'); - expect(prSkill).toContain('CHANGES_REQUESTED'); - expect(prSkill).toContain('APPROVE'); + expect(prSkill).toContain('`CHANGES_REQUESTED` entry after position 0'); expect(prSkill).toContain('after position 0'); - expect(prSkill).toContain('50.0% revert precision'); expect(prSkill).toContain('need-discussion'); expect(prSkill).not.toContain('status/on-hold'); expect(prSkill).toContain('maintainer sign-off'); + expect(prSkill).toContain('qwen-code-ci-bot'); + expect(prSkill).toContain('packages/core/src/**'); + expect(prSkill).toContain( + 'A maintainer removes it once the discussion resolves', + ); }); it('includes non-maintainer high-risk tier in the triage skill', () => { expect(prSkill).toContain('Non-maintainer + high-risk'); - expect(prSkill).toContain('58.3% revert precision'); expect(prSkill).toContain('highest-risk tier'); expect(prSkill).toContain('do not auto-approve'); + expect(prSkill).toContain('Stage 3 approval guardrail'); + }); + + it('includes Risk field in the Stage 1 comment template', () => { + expect(prSkill).toContain('Risk: Date: Mon, 27 Jul 2026 21:02:34 +0000 Subject: [PATCH 07/11] fix(triage): address round-3 review feedback on Stage 1e gate (#7414) - Separate gh api call from grep so API failures are visible instead of being masked by || true (rc:3660753982) - Include author identity in contested-merge jq output and require different reviewers for the disagreement check, avoiding false positives from same-reviewer iteration (rc:3660753990) - Add Stage 1e to the approval summary checklist so it is not omitted from the pre-approval conditions (rc:3660753994) --- .github/scripts/qwen-triage-workflow.test.mjs | 3 +++ .qwen/skills/triage/references/pr-workflow.md | 13 +++++++++---- scripts/tests/qwen-triage-workflow.test.js | 3 +++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 6d5c6fc4c2c..0f559e46c96 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -291,6 +291,7 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('do not skip any Stage 2 enrichment')); assert.ok(prSkill.includes('gh api --paginate')); assert.ok(prSkill.includes('|| true')); + assert.ok(prSkill.includes('WARNING: could not fetch PR files')); }); it('includes contested-merge detection', () => { @@ -303,6 +304,7 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('qwen-code-ci-bot')); assert.ok(prSkill.includes('packages/core/src/**')); assert.ok(prSkill.includes('A maintainer removes it once the discussion resolves')); + assert.ok(prSkill.includes('different reviewer')); }); it('includes non-maintainer high-risk tier', () => { @@ -310,6 +312,7 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('highest-risk tier')); assert.ok(prSkill.includes('do not auto-approve')); assert.ok(prSkill.includes('Stage 3 approval guardrail')); + assert.ok(prSkill.includes('no Stage 1e do-not-auto-approve signal')); }); it('includes Risk field in the Stage 1 comment template', () => { diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 413a91fc090..1ce61a54799 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -234,7 +234,12 @@ A revert-history analysis of this repo (111 revert commits, 46 unique reverted P **High-risk paths** — check the PR's changed files against these patterns: ```bash -gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true +FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') +if [ -n "$FILES" ]; then + echo "$FILES" | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true +else + echo "WARNING: could not fetch PR files" +fi ``` If any file matches (the strongest triage-time signal — 10 of 31 reverted PRs touched these paths vs 5 of 60 control PRs, p = 0.006): @@ -246,10 +251,10 @@ If any file matches (the strongest triage-time signal — 10 of 31 reverted PRs **Contested-merge pattern** — check the PR's review history for human reviewer disagreement (a CHANGES_REQUESTED entry that is not the first review, excluding bot reviews): ```bash -gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | select(.author.login != "qwen-code-ci-bot") | .state]' +gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | select(.author.login != "qwen-code-ci-bot") | {state: .state, author: .author.login}]' ``` -If the review state array has a `CHANGES_REQUESTED` entry after position 0 (a prior review preceded the disagreement) AND the PR touches core paths (`packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`): +If the review array has a `CHANGES_REQUESTED` entry after position 0 from a different reviewer than the entries before it (genuine cross-reviewer disagreement, not same-reviewer iteration) AND the PR touches core paths (`packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`): - Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). A maintainer removes it once the discussion resolves. - Recommend maintainer sign-off before merge in the Stage 1 comment. @@ -651,7 +656,7 @@ If Stage 1e flagged a contested-merge or non-maintainer + high-risk signal, do * **Re-runs (manually triggered via `@qwen-code /triage`):** hygiene concerns (scope mismatch, undocumented changes, naming) that don't block the PR are not a valid reason to defer. Note them in the comment and approve. Only defer if you have genuine blocking uncertainty — something you cannot resolve from the diff, tests, and PR description. -All stages genuinely clean, `GUARD` is `ok`, and no Stage 0 maintainer escalation remains — how you approve depends on the `PENDING` count computed in Step 1: +All stages genuinely clean, `GUARD` is `ok`, no Stage 0 maintainer escalation remains, and no Stage 1e do-not-auto-approve signal — how you approve depends on the `PENDING` count computed in Step 1: - `PENDING` = 0 → approve now, pinned to the reviewed commit (see the Approval note above) — never `gh pr review --approve`, which binds to no SHA: diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index ca4719ffa59..f9a6ff67cce 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -671,6 +671,7 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('do not skip any Stage 2 enrichment'); expect(prSkill).toContain('gh api --paginate'); expect(prSkill).toContain('|| true'); + expect(prSkill).toContain('WARNING: could not fetch PR files'); }); it('includes contested-merge detection in the triage skill', () => { @@ -685,6 +686,7 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain( 'A maintainer removes it once the discussion resolves', ); + expect(prSkill).toContain('different reviewer'); }); it('includes non-maintainer high-risk tier in the triage skill', () => { @@ -692,6 +694,7 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('highest-risk tier'); expect(prSkill).toContain('do not auto-approve'); expect(prSkill).toContain('Stage 3 approval guardrail'); + expect(prSkill).toContain('no Stage 1e do-not-auto-approve signal'); }); it('includes Risk field in the Stage 1 comment template', () => { From 496e17f42d5965d844543cb578690d4e5d4ec347 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 27 Jul 2026 22:30:56 +0000 Subject: [PATCH 08/11] fix(triage): address round-4 review feedback on Stage 1e gate (#7414) --- .github/scripts/qwen-triage-workflow.test.mjs | 2 +- .qwen/skills/triage/references/pr-workflow.md | 8 +++++--- scripts/tests/qwen-triage-workflow.test.js | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 0f559e46c96..94d9e3b0a67 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -304,7 +304,7 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('qwen-code-ci-bot')); assert.ok(prSkill.includes('packages/core/src/**')); assert.ok(prSkill.includes('A maintainer removes it once the discussion resolves')); - assert.ok(prSkill.includes('different reviewer')); + assert.ok(prSkill.includes('does not appear in any earlier entry')); }); it('includes non-maintainer high-risk tier', () => { diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 1ce61a54799..b5637af3ac7 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -236,7 +236,7 @@ A revert-history analysis of this repo (111 revert commits, 46 unique reverted P ```bash FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') if [ -n "$FILES" ]; then - echo "$FILES" | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true + echo "$FILES" | grep -v '\.\(test\|spec\)\.' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true else echo "WARNING: could not fetch PR files" fi @@ -254,12 +254,14 @@ If any file matches (the strongest triage-time signal — 10 of 31 reverted PRs gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | select(.author.login != "qwen-code-ci-bot") | {state: .state, author: .author.login}]' ``` -If the review array has a `CHANGES_REQUESTED` entry after position 0 from a different reviewer than the entries before it (genuine cross-reviewer disagreement, not same-reviewer iteration) AND the PR touches core paths (`packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`): +If the review array has a `CHANGES_REQUESTED` entry after position 0 from a reviewer who does not appear in any earlier entry (genuine cross-reviewer disagreement, not same-reviewer iteration) AND the PR touches core paths (`packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`): - Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). A maintainer removes it once the discussion resolves. - Recommend maintainer sign-off before merge in the Stage 1 comment. - Do not auto-approve even if Stage 2 and Stage 3 are clean (this feeds the Stage 3 approval guardrail — see below). +**Re-triage clearing:** the review history is immutable — a `CHANGES_REQUESTED` entry from an earlier round remains even after the disagreement is resolved. On a re-triage (`@qwen-code /triage`), if the `need-discussion` label is absent, treat the contested-merge signal as resolved and do not re-apply it. The label is the clearing mechanism: a maintainer removes it once the discussion resolves, and its absence on re-run means the signal no longer fires. + **Non-maintainer + high-risk**: a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions: do not skip any Stage 2 enrichment, require Stage 2b CI evidence, flag the high-risk paths in the Stage 1 comment, recommend E2E verification (scoped to write-access authors per Stage 2c), apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. @@ -614,7 +616,7 @@ Open it with a one-line confidence score — `**Confidence: N/5** — { expect(prSkill).toContain( 'A maintainer removes it once the discussion resolves', ); - expect(prSkill).toContain('different reviewer'); + expect(prSkill).toContain('does not appear in any earlier entry'); }); it('includes non-maintainer high-risk tier in the triage skill', () => { From 5d918fb4b239d7eeaa8fbd24e25585a8fac34948 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Tue, 28 Jul 2026 02:26:22 +0000 Subject: [PATCH 09/11] fix(triage): use portable ERE grep for test-file exclusion (#7414) --- .qwen/skills/triage/references/pr-workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index b5637af3ac7..300c07d9504 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -236,7 +236,7 @@ A revert-history analysis of this repo (111 revert commits, 46 unique reverted P ```bash FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') if [ -n "$FILES" ]; then - echo "$FILES" | grep -v '\.\(test\|spec\)\.' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true + echo "$FILES" | grep -Ev '\.(test|spec)\.' | grep -E 'openaiContentGenerator|streamingToolCallParser|geminiChat|acpConnection|(^|/)shell\.ts$|shellExecutionService|mcp-client|mcp-pool|LspServer|acp-integration|(^|/)relaunch\.ts$|(^|/)sandbox\.ts$|electron-run-as-node' || true else echo "WARNING: could not fetch PR files" fi From a5cd27a1a837d5e62894ecc5a171cd0aa2024a55 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 28 Jul 2026 11:04:54 +0800 Subject: [PATCH 10/11] fix(triage): guard deferred approval on discussion label --- .github/scripts/qwen-triage-workflow.test.mjs | 24 +++++++++++++++---- .github/workflows/qwen-triage-finalize.yml | 5 ++++ .qwen/skills/triage/references/pr-workflow.md | 8 +++---- .../qwen-triage-finalize-workflow.test.js | 4 ++++ scripts/tests/qwen-triage-workflow.test.js | 5 +++- 5 files changed, 36 insertions(+), 10 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 94d9e3b0a67..412e4bae14b 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -54,7 +54,10 @@ describe('qwen-triage: agent tool/permission settings', () => { it('settings is valid JSON that restricts the toolset', () => { const settings = JSON.parse(triageStep.with.settings); const core = settings.tools?.core; - assert.ok(Array.isArray(core), 'tools.core must be an array (registration allowlist)'); + assert.ok( + Array.isArray(core), + 'tools.core must be an array (registration allowlist)', + ); for (const t of [ 'run_shell_command', 'read_file', @@ -233,9 +236,13 @@ describe('qwen-triage: git exec-vector cleanup', () => { after(() => dir && rmSync(dir, { recursive: true, force: true })); const remaining = () => - spawnSync('git', ['-C', dir, 'config', '--local', '--name-only', '--list'], { - encoding: 'utf8', - }).stdout.toLowerCase(); + spawnSync( + 'git', + ['-C', dir, 'config', '--local', '--name-only', '--list'], + { + encoding: 'utf8', + }, + ).stdout.toLowerCase(); for (const vec of [ 'hookspath', @@ -303,7 +310,11 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('maintainer sign-off')); assert.ok(prSkill.includes('qwen-code-ci-bot')); assert.ok(prSkill.includes('packages/core/src/**')); - assert.ok(prSkill.includes('A maintainer removes it once the discussion resolves')); + assert.ok( + prSkill.includes( + 'contested-merge and non-maintainer + high-risk signals as resolved', + ), + ); assert.ok(prSkill.includes('does not appear in any earlier entry')); }); @@ -313,6 +324,9 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('do not auto-approve')); assert.ok(prSkill.includes('Stage 3 approval guardrail')); assert.ok(prSkill.includes('no Stage 1e do-not-auto-approve signal')); + assert.ok( + prSkill.includes('unless the re-triage clearing rule above applies'), + ); }); it('includes Risk field in the Stage 1 comment template', () => { diff --git a/.github/workflows/qwen-triage-finalize.yml b/.github/workflows/qwen-triage-finalize.yml index aaae63b7672..dee96de0165 100644 --- a/.github/workflows/qwen-triage-finalize.yml +++ b/.github/workflows/qwen-triage-finalize.yml @@ -422,6 +422,11 @@ jobs: update_status "$PR" guarded continue fi + if jq -e 'any(.labels[]?; .name == "need-discussion")' >/dev/null <<<"$PR_STATE_JSON"; then + echo "PR #$PR: need-discussion label — approval guardrail blocks the deferred approval." + update_status "$PR" guarded + continue + fi # An unavailable gate or an empty run list is "cannot attest", not # "nothing failed" — skip without approving. if [ "$GATE_OK" != true ] || [ "$TOTAL" -eq 0 ]; then diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 300c07d9504..9cf89d8446a 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -260,9 +260,9 @@ If the review array has a `CHANGES_REQUESTED` entry after position 0 from a revi - Recommend maintainer sign-off before merge in the Stage 1 comment. - Do not auto-approve even if Stage 2 and Stage 3 are clean (this feeds the Stage 3 approval guardrail — see below). -**Re-triage clearing:** the review history is immutable — a `CHANGES_REQUESTED` entry from an earlier round remains even after the disagreement is resolved. On a re-triage (`@qwen-code /triage`), if the `need-discussion` label is absent, treat the contested-merge signal as resolved and do not re-apply it. The label is the clearing mechanism: a maintainer removes it once the discussion resolves, and its absence on re-run means the signal no longer fires. +**Re-triage clearing:** the review history and touched paths are immutable — a `CHANGES_REQUESTED` entry from an earlier round remains even after the disagreement is resolved, and high-risk paths remain in the diff. On a re-triage (`@qwen-code /triage`), if the `need-discussion` label is absent, treat contested-merge and non-maintainer + high-risk signals as resolved and do not re-apply the label. The label is the clearing mechanism: a maintainer removes it once the discussion resolves, and its absence on re-run means those signals no longer fire. -**Non-maintainer + high-risk**: a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions: do not skip any Stage 2 enrichment, require Stage 2b CI evidence, flag the high-risk paths in the Stage 1 comment, recommend E2E verification (scoped to write-access authors per Stage 2c), apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. +**Non-maintainer + high-risk**: a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions unless the re-triage clearing rule above applies: do not skip any Stage 2 enrichment, require Stage 2b CI evidence, flag the high-risk paths in the Stage 1 comment, recommend E2E verification (scoped to write-access authors per Stage 2c), apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. @@ -636,7 +636,7 @@ GUARD=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json isCrossRepository,title \ --jq 'if (.isCrossRepository and (.title | test("^\\s*refactor"; "i"))) then "block" else "ok" end') ``` -Emit the marker only when ALL of these hold: the verdict is approve, `PENDING` is greater than 0, `GUARD` is `ok`, Stage 0 raised no maintainer escalation, and Stage 1e raised no do-not-auto-approve signal. A fork `refactor` or an escalated PR never carries the marker — those cap at 3/5 and take the defer path, with or without CI. (The finalize workflow independently re-asserts the fork-refactor guardrail before approving, but that is a backstop, not the mechanism.) +Emit the marker only when ALL of these hold: the verdict is approve, `PENDING` is greater than 0, `GUARD` is `ok`, Stage 0 raised no maintainer escalation, and Stage 1e raised no do-not-auto-approve signal. A fork `refactor` or an escalated PR never carries the marker — those cap at 3/5 and take the defer path, with or without CI. (The finalize workflow independently re-asserts the fork-refactor guardrail and an active `need-discussion` label before approving, but that is a backstop, not the mechanism.) When the marker is warranted, state it plainly in the comment — "approval deferred until CI lands green on ``" — and include it on its own line (full OID, the same one the footer attests): @@ -654,7 +654,7 @@ If `GUARD` is `block`: do **not** run `gh pr review --approve` no matter how cle If Stage 0 escalated the PR for maintainer awareness, do **not** approve automatically; use the "Genuinely unsure" path below. -If Stage 1e flagged a contested-merge or non-maintainer + high-risk signal, do **not** approve automatically; use the "Genuinely unsure" path below. A Stage 1e flag is a data-backed risk signal, not a hygiene concern — the re-run rule below does not override it. +If Stage 1e flagged a contested-merge or non-maintainer + high-risk signal and `need-discussion` is still present, do **not** approve automatically; use the "Genuinely unsure" path below. A Stage 1e flag is a data-backed risk signal, not a hygiene concern. If a maintainer removed `need-discussion` and this is a re-triage, follow the clearing rule above. **Re-runs (manually triggered via `@qwen-code /triage`):** hygiene concerns (scope mismatch, undocumented changes, naming) that don't block the PR are not a valid reason to defer. Note them in the comment and approve. Only defer if you have genuine blocking uncertainty — something you cannot resolve from the diff, tests, and PR description. diff --git a/scripts/tests/qwen-triage-finalize-workflow.test.js b/scripts/tests/qwen-triage-finalize-workflow.test.js index b3dce76167e..674a305468f 100644 --- a/scripts/tests/qwen-triage-finalize-workflow.test.js +++ b/scripts/tests/qwen-triage-finalize-workflow.test.js @@ -131,6 +131,10 @@ describe('qwen-triage-finalize workflow', () => { `(.head.repo.full_name // "") != .base.repo.full_name`, ); expect(script).toContain("grep -qiE '^[[:space:]]*refactor'"); + expect(script).toContain('any(.labels[]?; .name == "need-discussion")'); + expect(script).toContain( + 'need-discussion label — approval guardrail blocks the deferred approval', + ); expect(script.indexOf('update_status "$PR" guarded')).toBeLessThan( script.indexOf('update_status "$PR" deferred'), ); diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index a8ad919b0bf..59e18682612 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -684,7 +684,7 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('qwen-code-ci-bot'); expect(prSkill).toContain('packages/core/src/**'); expect(prSkill).toContain( - 'A maintainer removes it once the discussion resolves', + 'contested-merge and non-maintainer + high-risk signals as resolved', ); expect(prSkill).toContain('does not appear in any earlier entry'); }); @@ -695,6 +695,9 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('do not auto-approve'); expect(prSkill).toContain('Stage 3 approval guardrail'); expect(prSkill).toContain('no Stage 1e do-not-auto-approve signal'); + expect(prSkill).toContain( + 'unless the re-triage clearing rule above applies', + ); }); it('includes Risk field in the Stage 1 comment template', () => { From 35178968cfa28500e14daf84bd73dfe41f220aa5 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 28 Jul 2026 11:31:18 +0800 Subject: [PATCH 11/11] fix(triage): keep only supported revert signal --- .github/scripts/qwen-triage-workflow.test.mjs | 30 +---------- .github/workflows/qwen-triage-finalize.yml | 5 -- .qwen/skills/triage/references/pr-workflow.md | 34 +++--------- .../2026-07-27-revert-pattern-triage-gate.md | 52 +++---------------- .../qwen-triage-finalize-workflow.test.js | 4 -- scripts/tests/qwen-triage-workflow.test.js | 28 +--------- 6 files changed, 16 insertions(+), 137 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 412e4bae14b..276999d305f 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -301,35 +301,7 @@ describe('qwen-triage: Stage 1e revert-pattern signals', () => { assert.ok(prSkill.includes('WARNING: could not fetch PR files')); }); - it('includes contested-merge detection', () => { - assert.ok(prSkill.includes('Contested-merge pattern')); - assert.ok(prSkill.includes('`CHANGES_REQUESTED` entry after position 0')); - assert.ok(prSkill.includes('after position 0')); - assert.ok(prSkill.includes('need-discussion')); - assert.ok(!prSkill.includes('status/on-hold')); - assert.ok(prSkill.includes('maintainer sign-off')); - assert.ok(prSkill.includes('qwen-code-ci-bot')); - assert.ok(prSkill.includes('packages/core/src/**')); - assert.ok( - prSkill.includes( - 'contested-merge and non-maintainer + high-risk signals as resolved', - ), - ); - assert.ok(prSkill.includes('does not appear in any earlier entry')); - }); - - it('includes non-maintainer high-risk tier', () => { - assert.ok(prSkill.includes('Non-maintainer + high-risk')); - assert.ok(prSkill.includes('highest-risk tier')); - assert.ok(prSkill.includes('do not auto-approve')); - assert.ok(prSkill.includes('Stage 3 approval guardrail')); - assert.ok(prSkill.includes('no Stage 1e do-not-auto-approve signal')); - assert.ok( - prSkill.includes('unless the re-triage clearing rule above applies'), - ); - }); - it('includes Risk field in the Stage 1 comment template', () => { - assert.ok(prSkill.includes('Risk: /dev/null <<<"$PR_STATE_JSON"; then - echo "PR #$PR: need-discussion label — approval guardrail blocks the deferred approval." - update_status "$PR" guarded - continue - fi # An unavailable gate or an empty run list is "cannot attest", not # "nothing failed" — skip without approving. if [ "$GATE_OK" != true ] || [ "$TOTAL" -eq 0 ]; then diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 9cf89d8446a..e6350f91312 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -227,9 +227,9 @@ If you spot a materially simpler path, or changes that go beyond the minimal set Implementation-level concerns (over-abstraction, code duplication, "10 lines vs 10 files") belong in Stage 2a code review — you need to see the code for those. -**1e. High-risk path and contested-merge detection (data-backed escalation):** +**1e. High-risk path detection (data-backed escalation):** -A revert-history analysis of this repo (111 revert commits, 46 unique reverted PRs) found that certain file paths and review patterns are correlated with post-merge reverts. Check for these signals before proceeding to Stage 2 — they do NOT block or close the PR, but they determine the review depth and whether a maintainer sign-off is recommended. +A revert-history analysis of this repo (111 revert commits, 46 unique reverted PRs) found that certain file paths are correlated with post-merge reverts. Check this signal before proceeding to Stage 2 — it does NOT block or close the PR, but it determines the review depth. **High-risk paths** — check the PR's changed files against these patterns: @@ -248,23 +248,7 @@ If any file matches (the strongest triage-time signal — 10 of 31 reverted PRs - Flag the high-risk paths in the Stage 1 comment so the reviewer knows where to focus. - If the PR author has write access, recommend E2E verification in tmux (Stage 2c) before approval. If the author lacks write access, the sandboxed lanes are unavailable — recommend that a maintainer check the PR out in a disposable container or reproduce the specific behavioural claim by hand (see Stage 2c). -**Contested-merge pattern** — check the PR's review history for human reviewer disagreement (a CHANGES_REQUESTED entry that is not the first review, excluding bot reviews): - -```bash -gh pr view "$PR_NUMBER" --repo "$REPO" --json reviews --jq '[.reviews[] | select(.state != "PENDING" and .state != "COMMENTED") | select(.author.login != "qwen-code-ci-bot") | {state: .state, author: .author.login}]' -``` - -If the review array has a `CHANGES_REQUESTED` entry after position 0 from a reviewer who does not appear in any earlier entry (genuine cross-reviewer disagreement, not same-reviewer iteration) AND the PR touches core paths (`packages/core/src/**`, `packages/*/src/auth/**`, `packages/*/src/providers/**`, `packages/*/src/models/**`, `packages/*/src/config/**`, `packages/*/src/tools/**`, `packages/*/src/services/**`): - -- Apply `need-discussion` label via `gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label need-discussion` (if the label exists). A maintainer removes it once the discussion resolves. -- Recommend maintainer sign-off before merge in the Stage 1 comment. -- Do not auto-approve even if Stage 2 and Stage 3 are clean (this feeds the Stage 3 approval guardrail — see below). - -**Re-triage clearing:** the review history and touched paths are immutable — a `CHANGES_REQUESTED` entry from an earlier round remains even after the disagreement is resolved, and high-risk paths remain in the diff. On a re-triage (`@qwen-code /triage`), if the `need-discussion` label is absent, treat contested-merge and non-maintainer + high-risk signals as resolved and do not re-apply the label. The label is the clearing mechanism: a maintainer removes it once the discussion resolves, and its absence on re-run means those signals no longer fire. - -**Non-maintainer + high-risk**: a non-maintainer PR that matches the high-risk path patterns above is the highest-risk tier. Apply all actions unless the re-triage clearing rule above applies: do not skip any Stage 2 enrichment, require Stage 2b CI evidence, flag the high-risk paths in the Stage 1 comment, recommend E2E verification (scoped to write-access authors per Stage 2c), apply `need-discussion` label (if it exists), recommend maintainer sign-off, and do not auto-approve even if Stage 2 and Stage 3 are clean. - -These signals are NOT terminal gates — they do not stop the review or close the PR. They escalate review depth and flag risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. +This signal is NOT a terminal gate — it does not stop the review or close the PR. It escalates review depth and flags risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. Post a single Stage 1 comment. Be direct — say what you actually think, not what's polite: @@ -283,7 +267,7 @@ Size: . -Risk: +Risk: Moving on to code review. 🔍 Flagging these for discussion before diving deeper. @@ -303,7 +287,7 @@ Risk: 。<如果看到更简路径，点名：有没有考虑过直接 X？可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构，点名并建议拆成单独 PR。> -风险：<如果 Stage 1e 信号命中，列出匹配的高风险路径和/或 contested-merge 模式、建议的 review 深度、是否需要维护者签字。否则写"无升级风险信号"。> +风险：<如果 Stage 1e 命中，列出匹配的高风险路径和建议的 review 深度。否则写"无升级风险信号"。> <如果通过：> 进入代码审查 🔍 <如果有顾虑：> 先提出来讨论，再深入看代码。 @@ -616,7 +600,7 @@ Open it with a one-line confidence score — `**Confidence: N/5** — `" — and include it on its own line (full OID, the same one the footer attests): @@ -654,11 +638,9 @@ If `GUARD` is `block`: do **not** run `gh pr review --approve` no matter how cle If Stage 0 escalated the PR for maintainer awareness, do **not** approve automatically; use the "Genuinely unsure" path below. -If Stage 1e flagged a contested-merge or non-maintainer + high-risk signal and `need-discussion` is still present, do **not** approve automatically; use the "Genuinely unsure" path below. A Stage 1e flag is a data-backed risk signal, not a hygiene concern. If a maintainer removed `need-discussion` and this is a re-triage, follow the clearing rule above. - **Re-runs (manually triggered via `@qwen-code /triage`):** hygiene concerns (scope mismatch, undocumented changes, naming) that don't block the PR are not a valid reason to defer. Note them in the comment and approve. Only defer if you have genuine blocking uncertainty — something you cannot resolve from the diff, tests, and PR description. -All stages genuinely clean, `GUARD` is `ok`, no Stage 0 maintainer escalation remains, and no Stage 1e do-not-auto-approve signal — how you approve depends on the `PENDING` count computed in Step 1: +All stages genuinely clean, `GUARD` is `ok`, and no Stage 0 maintainer escalation remains — how you approve depends on the `PENDING` count computed in Step 1: - `PENDING` = 0 → approve now, pinned to the reviewed commit (see the Approval note above) — never `gh pr review --approve`, which binds to no SHA: diff --git a/docs/design/2026-07-27-revert-pattern-triage-gate.md b/docs/design/2026-07-27-revert-pattern-triage-gate.md index b0917bed1d4..23502f83e44 100644 --- a/docs/design/2026-07-27-revert-pattern-triage-gate.md +++ b/docs/design/2026-07-27-revert-pattern-triage-gate.md @@ -131,7 +131,7 @@ review rounds, multiple merge/revert cycles, and often require patch releases. ## Design -### Rule 1: High-risk path escalation (precision 66.7%, recall 32.3%) +### High-risk path escalation When a non-maintainer PR touches any high-risk path (see definition above), Stage 1 triage escalates the PR to the deepest review tier instead of the @@ -147,37 +147,6 @@ Implementation: the Stage 1e skill text instructs the triage model to run No workflow YAML change is needed — the detection runs inside the skill, not as a separate workflow step. -### Rule 2: Contested-merge detection (precision 50.0%, recall 19.4%) - -When a PR has a CHANGES_REQUESTED → APPROVED cycle in its review history and -touches core paths, Stage 1 applies `need-discussion` and posts a summary -recommending maintainer sign-off before merge. - -This targets the flip-flop pattern: PRs that went through multiple review -rounds with disagreements have 50% precision (6 of 12 such PRs reverted), -versus a 10% prevalence of the same signal in the control group (6/60). -Note: this signal is not statistically significant at n = 31 (Fisher -p = 0.33) — see the follow-up note below. - -Implementation: the Stage 1e skill text instructs the triage model to fetch -the PR's review state sequence via `gh pr view --json reviews` and check for -a `CHANGES_REQUESTED` entry after position 0. If found alongside core-path -changes, the classifier applies `need-discussion` and recommends maintainer -sign-off. No workflow YAML change is needed. - -### Rule 3: Non-maintainer + high-risk tier (precision 58.3%, recall 22.6%) - -A non-maintainer PR touching high-risk paths gets the highest-risk tier: -full `/review` depth + `need-discussion` until a maintainer reviews. This -combines the `non_maintainer` and `touches_high_risk` signals (Rule 1's -high-risk path escalation restricted to non-maintainer authors), the -intersection measured at 58.3% precision in the table above. Note: this -signal does not reach significance at n = 31 (Fisher p = 0.098). - -This replaces PR #7414's behavior-neutral filter (~2% live-backlog hit -rate; revert recall unmeasured) with a signal whose revert recall is -measured at 22.6% — targeting the PRs that actually cause reverts. - ### What this design does NOT do - **Does not auto-close or auto-reject PRs.** The gate escalates review depth @@ -203,14 +172,12 @@ measured at 22.6% — targeting the PRs that actually cause reverts. ## Files changed -- `.qwen/skills/triage/references/pr-workflow.md` — add Stage 1e high-risk path - checklist, contested-merge detection, and non-maintainer + high-risk tier - instructions. The detection runs inside the triage skill (the model runs - `gh pr view --json files | grep …` and `gh pr view --json reviews` itself), +- `.qwen/skills/triage/references/pr-workflow.md` — add the Stage 1e high-risk + path checklist. The detection runs inside the triage skill (the model runs + `gh api --paginate … | grep …` itself), so no workflow YAML change is needed. -- `scripts/tests/qwen-triage-workflow.test.js` — assert the high-risk path, - contested-merge, and non-maintainer + high-risk routing strings exist in - the triage skill markdown. +- `scripts/tests/qwen-triage-workflow.test.js` — assert the high-risk path + routing strings exist in the triage skill markdown. - `.github/scripts/qwen-triage-workflow.test.mjs` — the same assertions in the node:test runner. @@ -234,13 +201,6 @@ measured at 22.6% — targeting the PRs that actually cause reverts. flip-flops retrospectively (after multiple reverts). A real-time version would monitor for revert→re-revert patterns on `main` and alert maintainers. This requires a separate monitoring workflow, not a triage gate. -- **Statistical significance of Rules 2–3.** `core + contested` (Rule 2) - has Fisher p = 0.33 and `non_maintainer + high_risk` (Rule 3) has - p = 0.098 at n = 31 — neither reaches the 0.05 threshold. Only Rule 1 - (`touches_high_risk`, p = 0.006) is well-supported. Rules 2–3 are - shipped as escalate-only signals (they add review friction but never - block), but if a future re-analysis with more data does not confirm - them, they should be removed. - **Expanding the high-risk path list.** The current list is manually curated from the reverted PR file paths. As the codebase evolves, new high-risk paths may emerge. A periodic re-run of the analysis scripts would keep the diff --git a/scripts/tests/qwen-triage-finalize-workflow.test.js b/scripts/tests/qwen-triage-finalize-workflow.test.js index 674a305468f..b3dce76167e 100644 --- a/scripts/tests/qwen-triage-finalize-workflow.test.js +++ b/scripts/tests/qwen-triage-finalize-workflow.test.js @@ -131,10 +131,6 @@ describe('qwen-triage-finalize workflow', () => { `(.head.repo.full_name // "") != .base.repo.full_name`, ); expect(script).toContain("grep -qiE '^[[:space:]]*refactor'"); - expect(script).toContain('any(.labels[]?; .name == "need-discussion")'); - expect(script).toContain( - 'need-discussion label — approval guardrail blocks the deferred approval', - ); expect(script.indexOf('update_status "$PR" guarded')).toBeLessThan( script.indexOf('update_status "$PR" deferred'), ); diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 59e18682612..8c784c292c9 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -674,34 +674,8 @@ describe('qwen-triage tmux workflow', () => { expect(prSkill).toContain('WARNING: could not fetch PR files'); }); - it('includes contested-merge detection in the triage skill', () => { - expect(prSkill).toContain('Contested-merge pattern'); - expect(prSkill).toContain('`CHANGES_REQUESTED` entry after position 0'); - expect(prSkill).toContain('after position 0'); - expect(prSkill).toContain('need-discussion'); - expect(prSkill).not.toContain('status/on-hold'); - expect(prSkill).toContain('maintainer sign-off'); - expect(prSkill).toContain('qwen-code-ci-bot'); - expect(prSkill).toContain('packages/core/src/**'); - expect(prSkill).toContain( - 'contested-merge and non-maintainer + high-risk signals as resolved', - ); - expect(prSkill).toContain('does not appear in any earlier entry'); - }); - - it('includes non-maintainer high-risk tier in the triage skill', () => { - expect(prSkill).toContain('Non-maintainer + high-risk'); - expect(prSkill).toContain('highest-risk tier'); - expect(prSkill).toContain('do not auto-approve'); - expect(prSkill).toContain('Stage 3 approval guardrail'); - expect(prSkill).toContain('no Stage 1e do-not-auto-approve signal'); - expect(prSkill).toContain( - 'unless the re-triage clearing rule above applies', - ); - }); - it('includes Risk field in the Stage 1 comment template', () => { - expect(prSkill).toContain('Risk: