Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .qwen/skills/triage/references/pr-workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -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):**
Comment thread
yiliang114 marked this conversation as resolved.
Outdated

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'
Comment thread
yiliang114 marked this conversation as resolved.
Outdated
```

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.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
- Recommend E2E verification in tmux (Stage 2b) before approval.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

**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):
Comment thread
yiliang114 marked this conversation as resolved.
Outdated
- 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.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated
- Do not auto-approve even if Stage 2 and Stage 3 are clean.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

**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.
Comment thread
yiliang114 marked this conversation as resolved.
Outdated

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
Expand Down
218 changes: 218 additions & 0 deletions docs/design/2026-07-27-revert-pattern-triage-gate.md
Original file line number Diff line number Diff line change
@@ -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
Comment thread
yiliang114 marked this conversation as resolved.
(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 <hash>`, 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`.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

### 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
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

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:
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

- 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Design doc describes wrong command — Concrete cost: this paragraph says gh pr view --json files | grep -E '...' but the actual skill text implements gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename'. The doc's own "Files changed" section (line ~183) correctly uses gh api --paginate, making the doc internally inconsistent. A future maintainer reading the Implementation section wastes time tracing the discrepancy. Additionally, pr-workflow.md:369 explicitly warns against gh pr view --json files as it "caps at the first 100 files and silently drops the rest."

Suggested change
`gh pr view --json files | grep -E '...'` against the high-risk path patterns.
`gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename' | grep -E '...'` against the high-risk path patterns.
中文说明

[Suggestion] 设计文档描述了错误的命令——具体代价:本段写的是 gh pr view --json files | grep -E '...',但实际 skill 文本实现的是 gh api --paginate。文档自身的"Files changed"部分(约第 183 行)正确使用了 gh api --paginate,使文档内部不一致。未来维护者阅读 Implementation 部分时会浪费时间追踪差异。

— qwen3.7-max via Qwen Code /review

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 `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).
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

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
is Rule 1 + Rule 2 combined, targeting the intersection that has 58.3%
precision.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

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.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Outdated

### 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.
42 changes: 42 additions & 0 deletions scripts/tests/qwen-triage-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,48 @@ 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',
);
Comment thread
yiliang114 marked this conversation as resolved.
Outdated

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('need-discussion');
expect(prWorkflow).not.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', () => {
Expand Down
Loading