Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
57 changes: 57 additions & 0 deletions .github/scripts/qwen-triage-workflow.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -259,3 +270,49 @@ 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'));
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
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('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` 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('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('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: <if Stage 1e signals matched'));
});
});
42 changes: 40 additions & 2 deletions .qwen/skills/triage/references/pr-workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -227,6 +227,38 @@ 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 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 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
Comment thread
yiliang114 marked this conversation as resolved.
Outdated
```

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

- For non-maintainer PRs: do not skip any Stage 2 enrichment (2a-bis); require Stage 2b CI evidence before approving.
- 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.
- 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]'
```

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/**`):

- 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.
Comment thread
yiliang114 marked this conversation as resolved.
Outdated
- 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 (this feeds the Stage 3 approval guardrail — see below).

**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.
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 All @@ -244,6 +276,8 @@ Size: <if core paths are touched, report production lines vs. test lines vs. gen

Approach: <state your honest assessment — the scope feels right / feels like it could be much simpler / here's what I'd consider cutting>. <If you see a simpler path, name it: "Have you considered just X? It might cover most of the use case with a fraction of the complexity."> <If the diff carries unrelated changes or drive-by refactors, name them and suggest splitting them out.>

Risk: <if Stage 1e signals matched, list the matched high-risk paths and/or contested-merge pattern, the recommended review depth, and whether maintainer sign-off is recommended. Otherwise say "no elevated risk signals".>

<If passing:> Moving on to code review. 🔍
<If concerns:> Flagging these for discussion before diving deeper.

Expand All @@ -262,6 +296,8 @@ Approach: <state your honest assessment — the scope feels right / feels like i

方案:<范围合理 / 感觉可以大幅简化 / 建议砍掉的部分>。<如果看到更简路径,点名:有没有考虑过直接 X?可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构,点名并建议拆成单独 PR。>

风险:<如果 Stage 1e 信号命中,列出匹配的高风险路径和/或 contested-merge 模式、建议的 review 深度、是否需要维护者签字。否则写"无升级风险信号"。>

<如果通过:> 进入代码审查 🔍
<如果有顾虑:> 先提出来讨论,再深入看代码。

Expand Down Expand Up @@ -593,7 +629,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`, and Stage 0 raised no maintainer escalation. 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 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 `<HEAD_SHA>`" — and include it on its own line (full OID, the same one the footer attests):

Expand All @@ -611,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.
Comment thread
yiliang114 marked this conversation as resolved.
Outdated

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

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