-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(triage): add revert-pattern high-risk path detection #7414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
9e2818a
07870df
a17a335
13e8f62
fe9b017
7cb1edd
7b022cf
61a801f
b596faf
12b1eb3
496e17f
5d918fb
a5cd27a
6b2b759
3517896
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -227,6 +227,45 @@ 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):** | ||||||||||||||||||||
|
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 | ||||||||||||||||||||
| FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Missing
Suggested change
中文说明[Suggestion] 缺少 — qwen3.7-max via Qwen Code /review |
||||||||||||||||||||
| if [ -n "$FILES" ]; then | ||||||||||||||||||||
|
Comment on lines
+237
to
+238
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Stage 1e bash snippet fails open on every realistic API error — Failure scenario:
Suggested change
Also add a third state to the 中文说明[Critical] Stage 1e bash 片段在所有现实的 API 错误场景下 fail-open —— 失败场景: 同时请给 — qwen3.7-max via Qwen Code /review |
||||||||||||||||||||
| 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 | ||||||||||||||||||||
| ``` | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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): | ||||||||||||||||||||
|
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. | ||||||||||||||||||||
|
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: .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. | ||||||||||||||||||||
|
yiliang114 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||
| - Recommend maintainer sign-off before merge in the Stage 1 comment. | ||||||||||||||||||||
|
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). | ||||||||||||||||||||
|
|
||||||||||||||||||||
| **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. | ||||||||||||||||||||
|
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 | ||||||||||||||||||||
|
|
@@ -244,6 +283,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. | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -262,6 +303,8 @@ Approach: <state your honest assessment — the scope feels right / feels like i | |||||||||||||||||||
|
|
||||||||||||||||||||
| 方案:<范围合理 / 感觉可以大幅简化 / 建议砍掉的部分>。<如果看到更简路径,点名:有没有考虑过直接 X?可能用很小的复杂度覆盖大部分场景。><如果 diff 夹带了无关改动或顺手重构,点名并建议拆成单独 PR。> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 风险:<如果 Stage 1e 信号命中,列出匹配的高风险路径和/或 contested-merge 模式、建议的 review 深度、是否需要维护者签字。否则写"无升级风险信号"。> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| <如果通过:> 进入代码审查 🔍 | ||||||||||||||||||||
| <如果有顾虑:> 先提出来讨论,再深入看代码。 | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -573,7 +616,7 @@ Open it with a one-line confidence score — `**Confidence: N/5** — <one hones | |||||||||||||||||||
| | 2/5 | Significant concerns; leaning against as-is | request changes | | ||||||||||||||||||||
| | 1/5 | Should not merge in its current form | request changes | | ||||||||||||||||||||
|
|
||||||||||||||||||||
| A fork `refactor` that hits the approval guardrail below, **or a PR that Stage 0 escalated for maintainer awareness**, caps at 3/5 no matter how clean every stage looked — the guardrail drives the action, not the score. At 3/5 the action is always the **defer path** (a comment, never `--request-changes`): name any concerns in the defer comment for the maintainer's attention without approving, and @mention the maintainer for an unresolvable question or when the cap is pure policy. When the cap is pure policy on an otherwise-clean PR, say so in the one-line score so 3/5 doesn't read as real doubt — e.g. `Confidence: 3/5 — clean review, but the fork-refactor guardrail needs a maintainer's sign-off`. Never post a 4–5/5 alongside a `--request-changes`, or a 1–2/5 alongside an `--approve`: the score and the verdict tell the same story. | ||||||||||||||||||||
| A fork `refactor` that hits the approval guardrail below, **a PR that Stage 0 escalated for maintainer awareness, or a PR with an active Stage 1e do-not-auto-approve signal**, caps at 3/5 no matter how clean every stage looked — the guardrail drives the action, not the score. At 3/5 the action is always the **defer path** (a comment, never `--request-changes`): name any concerns in the defer comment for the maintainer's attention without approving, and @mention the maintainer for an unresolvable question or when the cap is pure policy. When the cap is pure policy on an otherwise-clean PR, say so in the one-line score so 3/5 doesn't read as real doubt — e.g. `Confidence: 3/5 — clean review, but the fork-refactor guardrail needs a maintainer's sign-off`. Never post a 4–5/5 alongside a `--request-changes`, or a 1–2/5 alongside an `--approve`: the score and the verdict tell the same story. | ||||||||||||||||||||
|
yiliang114 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| Then write what you're actually thinking. "Looks good, ships the feature cleanly, the before/after shows it works" — not a five-bullet summary of the stages. If you have reservations, say them plainly. If you're approving with mild concerns, name them. Sign with `— _Qwen Code · qwen3.7-max_`, add the reviewed-commit footer (empty `HEAD_SHA` → fail closed, as above — don't blank a prior footer), and save this comment's ID. | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -593,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`, 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): | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -611,9 +654,11 @@ 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. | ||||||||||||||||||||
|
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. | ||||||||||||||||||||
|
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: | ||||||||||||||||||||
| 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: | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.