feat(triage): add revert-pattern high-risk path detection - #7414
Conversation
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
I ran this against the current open PR backlog to get real numbers before merging. Data: 126 open PRs, 102 non-maintainer. Applying the Stage 1e criteria (100% behavior-neutral: internal renames, formatting-only, comment/JSDoc cleanup):
Conclusion: the hit rate is too low to justify the added complexity. This introduces a new Stage 1e classification branch, Protections like this should be designed from real data — measured hit rates, actual review-cost savings — not from a hypothetical that the backlog doesn't support. If behavior-neutral PRs become a genuine volume problem later, we can revisit with numbers to back it up. Closing for now. 中文说明我用当前存量 open PR 跑了真实数据。 数据： 126 个 open PR，其中非维护者 102 个。按 Stage 1e 标准（100% 不影响行为：内部重命名、纯格式化、注释/JSDoc 清理）逐一比对：
结论： 命中率太低，不值得增加复杂度。这个 PR 引入了新的 Stage 1e 分类分支、 这类防护机制应该基于真实数据来设计——实际命中率、可量化的 review 成本节省——而不是凭假设。如果将来 behavior-neutral PR 真的成为量的问题，可以拿数据重新评估。 暂时关闭。 |
|
A suggestion for a more impactful direction: instead of filtering out low-risk PRs that barely consume review capacity, look at the reverted PRs in this repo's history — the ones that were merged but had to be rolled back because they introduced regressions, broke builds, or caused user-facing issues. That's where the real cost is. A behavior-neutral JSDoc fix that slips through without deep review costs nothing; a subtly broken Concretely: run That analysis would give you real data on what "dangerous PR" actually looks like in this repo, and let you design a triage gate that catches the PRs that actually cause damage — not the ones that are already harmless. 中文说明一个更有价值的方向建议：与其过滤那些几乎不消耗 review 资源的低风险 PR，不如去看这个仓库历史上被回滚的 PR——那些合入后因为引入回归、破坏构建或影响用户而不得不 revert 的 PR。 那才是真正的成本所在。一个 JSDoc 修正即使没经过深度 review 也毫无风险；但一个看似正确的 具体做法：跑 这个分析能给你真实数据，告诉你在这个仓库里"危险 PR"到底长什么样，从而设计一个真正能拦住造成损害的 PR 的 triage 门禁——而不是拦那些本来就无害的 PR。 |
|
Thanks for the PR — the routing logic here is clean, but I want to flag the direction question before going deeper, because that's the real issue. Template: complete ✓ Problem: this is a Direction: genuinely contested. @wenshao reviewed the data, concluded the ~2% hit rate doesn't justify a new classification branch + label routing + expanded lifecycle conditions, recommended closing, and proposed a higher-value direction — mining the repo's reverted PRs to learn what a dangerous PR looks like at triage time. That's a maintainer judgment call this bot shouldn't override in either direction. Size: not applicable — no core paths. 122+/11− across 6 files (workflow + two skill docs + design doc, plus 65 test lines). Small and contained. Approach: the core mechanism is the right shape — reuse the existing Flagging these for discussion rather than diving deeper on a contested direction. ⏸️ 中文说明感谢这个 PR——路由逻辑本身写得很干净，但在深入之前我想先提出方向上的问题，因为那才是关键。 模板：完整 ✓ 问题：这是一个 方向：存在实质分歧。@wenshao 看完数据后认为约 2% 的命中率不值得新增一个分类分支 + 标签路由 + 扩展的生命周期条件，建议关闭，并提出了更有价值的方向——挖掘仓库里被回滚的 PR，研究 triage 阶段哪些信号本可以识别出危险 PR。这是维护者的判断，bot 不应该替任何一方拍板。 规模：不适用——未触及核心路径。6 个文件 122+/11−（workflow + 两个 skill 文档 + 设计文档，外加 65 行测试）。小而集中。 方案：核心机制的形态是对的——复用现有 先提出这些供讨论，而不是在一个有分歧的方向上继续深挖。⏸️ — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewMy independent take before reading the diff: to skip deep review on clearly behavior-neutral PRs, I'd reuse the existing Correctness-wise the workflow edit holds together. The The Stage 1e skill text is appropriately conservative (100%-confidence bar, explicit exclusions, no auto-removal). Tellingly, this PR is itself a workflow change, so its own Stage 1e exclusion would keep it off the behavior-neutral path — no paradox. The two new test files assert the routing strings exist in the YAML/markdown; brittle-by-design for config-as-text, and they pass. No correctness blockers found. The substantive concern remains the one from Stage 1: whether the feature is worth having, plus the unrelated Test evidenceUnattended CI run — I did not build or execute any PR code (the agent env holds a write PAT). The evidence below is the PR's own CI, read via the API. Real-scenario tmux/verify lanes are not run here; this changes repo automation rather than the TUI, and those lanes execute PR code so they require write access (a maintainer can trigger
The 中文说明代码审查读 diff 之前我的独立想法：要跳过明确不影响行为的 PR 的深度 review，我会复用现有 正确性上，workflow 改动是自洽的。 Stage 1e 的 skill 文案足够保守（100% 置信门槛、明确排除项、不自动移除）。有意思的是，这个 PR 本身就是 workflow 改动，所以按它自己的 Stage 1e 排除项，它也不会走 behavior-neutral 路径——没有悖论。两个新测试文件断言 YAML/markdown 里存在路由字符串；对 config-as-text 来说这种断言天生偏脆，但它们通过了。 没有发现正确性阻塞项。实质性的顾虑仍是 Stage 1 提的那个：这个功能值不值得做，以及我建议拆出去的、无关的 测试证据无人值守 CI 运行——我没有构建或执行任何 PR 代码（agent 环境持有写权限 PAT）。下面的证据是 PR 自己的 CI，通过 API 读取。真实场景的 tmux/verify 通道不在此运行；本改动影响的是仓库自动化而非 TUI，且那些通道会执行 PR 代码，需要写权限（维护者可触发
— Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — the code is correct, well-tested, and green, but a maintainer has data-backed reservations about whether it's needed at all, so this needs a human call rather than an auto-approve. Stepping back: the implementation is genuinely clean. It reuses the existing But the question is "should we build it?", and on that the signal is against. The justification rests on a proxy metric (small PRs), while the maintainer's measurement of the actual target set (behavior-neutral PRs) came back at ~2%. That's a solution looking for a problem the backlog doesn't really have. @wenshao also pointed at a more impactful use of the same effort — learning what a dangerous PR looks like from the repo's revert history, which targets the PRs that actually cost maintainer hours. And the PR carries an unrelated issue-signature change that should be its own PR regardless of how this one resolves. This is a direction disagreement between maintainers, and it's exactly the kind of call a triage bot should hand back rather than settle. So: no approval, no change request — the code isn't broken, but it shouldn't merge on the bot's say-so over a data-backed maintainer objection. ⏸️ Deferring to @wenshao and @yiliang114 — please settle the direction question (is ~2% worth the added branch, or should this effort go toward revert-pattern triage instead?). If the answer is "build it," the implementation is in good shape; I'd still split out the 中文说明置信度：3/5 —— 代码正确、测试充分、CI 全绿，但有维护者用数据对"它到底有没有必要"提出了实质保留，所以这需要人来拍板，而不是自动批准。 退一步看：实现确实干净。它复用了现有的 但问题是"该不该做"，而信号是反向的。论据建立在一个代理指标（小 PR）上，而维护者对真正目标集合（behavior-neutral PR）的实测只有约 2%。这是一个在 backlog 里几乎没有对应问题的方案。@wenshao 还指出了同样精力更有价值的用法——从仓库的回滚历史里学习"危险 PR"长什么样，那才是真正消耗维护者时间的 PR。而且这个 PR 还夹带了一个无关的 issue 签名改动，无论本 PR 结论如何都应该单独成 PR。 这是维护者之间的方向分歧，正是 triage bot 应该交还、而不是替人决定的那类判断。所以：不批准、也不请求修改——代码没坏，但不应该在一个有数据支撑的维护者反对意见之上，由 bot 拍板合入。 ⏸️ 转交 @wenshao 和 @yiliang114 —— 请定一下方向（约 2% 值不值得新增分支，还是说这份精力应该投向 revert-pattern triage？）。如果结论是"做"，实现状态很好；但我仍建议先把 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Review —
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] status/on-hold is the stale-bot exemption label — bot-applied hold means PRs are both never automatically reviewed and permanently immune to reaping (stale.yml:42 exempt-pr-labels includes status/on-hold). Use a dedicated label (review/deferred) that is not in exempt-pr-labels.
[Critical] The hold is permanent across pushes — triage triggers on opened/ready_for_review only (not synchronize), so the label is never re-evaluated on new pushes. A contributor's first formatting-only push gets the label, then any subsequent substantive push skips automatic review permanently.
[Critical] TRIAGE_MODEL shell assignment emits nothing — TRIAGE_MODEL="$(printenv OPENAI_MODEL || true)" assigns to a shell variable but produces no stdout. Each agent Bash call is a fresh shell, so the variable doesn't persist. The model-name injection is a functional no-op.
— qwen3.7-max via Qwen Code /review
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.
96f8dfe to
9e2818a
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
yiliang114
changed the title
|
Addressed the current review-body Critical that still applies to this head.
Verified with focused |
|
Qwen Code review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] TRIAGE_MODEL shell assignment emits nothing — the referenced code does not exist at the current commit (previously flagged by @doudouOUC); cannot verify fix from current diff alone
中文说明
未决，请确认：[Critical] TRIAGE_MODEL shell assignment emits nothing — the referenced code does not exist at the current commit (previously flagged by @doudouOUC); cannot verify fix from current diff alone
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
审查总结 — PR #7414
SHA: 12b1eb3c
差异: 4 files, +403/-6 (97 source, 146 test, 253 docs)
审查覆盖
- 12 个并行代理: Issue Fidelity, 逐行正确性, 安全, 代码质量, 性能, 测试覆盖, 3× 对抗审计, 移除行为, 跨文件追踪, 构建与测试
- 验证: 2 个高置信度发现已验证
- 反向审计: 4 轮（2 个 dry round + 硬上限）
- 开放 Critical 重检: 2 个历史 blocker 均确认已修复 ✅
构建与测试
node --test: 32/32 ✅vitest: 77/77 ✅
发现
高置信度（行内评论）:
- [Suggestion] Stage 1e "do not auto-approve" 在 finalize workflow 中缺少确定性 backstop —
pr-workflow.md:657
需要人工审查（low confidence，仅终端输出）:
2. gh api 错误处理可能静默跳过高风险路径检测
3. Contested-merge 检测缺少 APPROVED 状态检查
4. Rule 3 触发条件在设计文档和 skill 文本间不一致
Verdict: COMMENT (0 Critical, 1 Suggestion inline)
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Round 5 — Suggestions deferred per review-round policyThis PR has been through 5 review rounds (rounds 1–4 already committed). Per AGENTS.md: "Once a PR has been through roughly 5 review rounds, land only Critical fixes — correctness, security, data loss, regressions — and defer remaining Suggestions to a follow-up issue or PR." All 4 inline findings this round are [Suggestion] level. None are Critical. Each is deferred with a recorded reason below; replies are posted on the individual threads.
No code changes this round. 中文说明第 5 轮——根据 review 轮次策略延迟处理建议本 PR 已经历 5 轮 review（第 1–4 轮已提交）。根据 AGENTS.md："一旦 PR 经历大约 5 轮 review，仅落地 Critical 修复——正确性、安全性、数据丢失、回归——其余建议延迟到后续 issue 或 PR。" 本轮所有 4 条行内发现均为 [Suggestion] 级别，无 Critical。每条均附记录原因延迟处理；回复已发布在各自线程中。
本轮无代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: the linked-issue fidelity pass, the line-by-line correctness pass, the security pass, the code-quality pass, the performance pass, the test-coverage pass, the open-ended audit (attacker mindset), the open-ended audit (oncall mindset), the open-ended audit (maintainer mindset), the removed-behavior audit, the cross-file consistency pass, the build-and-test check — its prompt was built, but no agent on record was launched with it.
中文说明
已审查。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：关联 issue 一致性检查、逐行正确性检查、安全检查、代码质量检查、性能检查、测试覆盖检查、开放式审计（攻击者视角）、开放式审计（值班排障视角）、开放式审计（后续维护者视角）、删除行为审计、跨文件一致性检查、构建与测试验证——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
审查报告 — PR #7414
概要： 本 PR 在 triage skill 中增加 Stage 1e，基于 revert 历史数据分析添加高风险路径检测、contested-merge 检测和非维护者+高风险分级。4 个文件，+406/-7 行。
审查结果
- 12 个 agent 运行：0 Critical，4 Suggestion 确认
- 1 个 Critical 被验证拒绝：non-maintainer 层级缺少 guardrail 交叉引用——实际有 4 层独立保护机制
- 2 个 prior Critical blocker 已修复：contested-merge 条件矛盾、"do not auto-approve" 无 Stage 3 挂接
- CI 状态：93 项全部通过；跳过检查（Integration Tests、macOS/Windows Test）不覆盖本 PR 变更文件
4 个确认的 Suggestion（均未发布为 inline——3 个与现有评论重叠，1 个 anchor 未匹配）
- Stage 3 guardrail 集成未测试（line 657）— 测试断言匹配 Stage 1e 段落而非 Stage 3 护栏文本 (与现有评论重叠)
- Re-triage clearing 未测试（line 263）— 无测试断言覆盖 clearing 机制 (anchor 未匹配，仅终端输出)
- Contested-merge 在多审查者共识时误触发（line 260）— 条件排除同一审查者迭代但不排除同方向共识 (与现有评论重叠)
- Re-triage clearing 与 non-maintainer + high-risk 层级冲突（line 265）—
need-discussion标签被可变和不可变条件共享 (与现有评论重叠)
Verdict
Verdict: Comment — an Approve was NOT available: a dimension nobody reviewed
所有 12 个 agent 实际运行并返回了实质性结果。0 Critical, 0 Suggestion inline。
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Round 5 — no code changeOne inline finding was raised this round (a Suggestion on the Stage 1e do-not-auto-approve signal). After triage it is declined for this PR and deferred to a follow-up, so this round produces no commit. The two review-level reports carried no actionable inline findings (their Suggestions were either not anchored to a changed line or overlapped existing comments). [Suggestion] Stage 1e "do not auto-approve" lacks a deterministic backstop in the finalize workflow (rc:3661698256)Decision: decline for this PR (deferred to a follow-up); thread left open. The finding is correct that the finalize workflow re-asserts the fork-refactor guardrail structurally but has no parallel structural re-check for the Stage 1e signal. The gap is narrower than "relies entirely on the LLM remembering the signal", though:
The residual risk is only "the agent emits a marker it should not" — the same class the fork-refactor structural backstop covers. That backstop was added in response to a specific wrongful-merge incident; Stage 1e is brand new and has no such incident behind it. Per the project's review convention (after roughly five rounds, land only Critical fixes and defer Suggestions) and the round-ratchet guidance (a suggestion whose only effect is additional defense is a decline, not an automatic implement), this hardening is deferred rather than added now. The follow-up is concrete and cheap: re-read the 中文说明第 5 轮 —— 无代码改动本轮收到一条 inline 发现（关于 Stage 1e do-not-auto-approve 信号的 Suggestion）。经分类，本 PR 不予采纳，推迟到后续 PR，因此本轮不产生提交。两条 review 级报告没有可执行的 inline 发现（其 Suggestion 要么无法锚定到改动行，要么与现有评论重叠）。 [Suggestion] Stage 1e 的 "do not auto-approve" 在 finalize 工作流中缺少确定性兜底（rc:3661698256）决定：本 PR 不予采纳（推迟到后续 PR）；线程保持打开。 该发现指出的事实成立：finalize 工作流对 fork-refactor 护栏做了结构化复核，但对 Stage 1e 信号没有对应的结构化复核。不过，实际缺口比"完全依赖 LLM 记住信号"要窄：
残余风险仅为"agent 发出了本不该发出的标记"，这与 fork-refactor 结构化兜底覆盖的是同一类风险。那个兜底是因为一次具体的误合并事故才加上的；Stage 1e 是全新逻辑，尚无此类事故。 按照本项目的 review 约定（大约五轮之后只落地 Critical 修复、将 Suggestion 推迟）以及轮次棘轮指引（只增加防御的建议属于 decline，而非自动实现），此加固推迟到后续，而非现在加入。后续改动具体且成本低：重新读取 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Reviewed at head What's solid: the escalate-only design (no blocking/closing), the design doc's statistical honesty (sampling-ratio caveat, PPV at base rate, explicit removal criteria for Rules 2–3), the round-3 fix separating the [Major] The contested-merge signal is unreachable as written — the clearing rule can't distinguish "never applied" from "maintainer cleared it"
Suggested fix: treat label-absence as clearance only when the label was previously applied — e.g. check for an This compounds the two already-deferred findings: @doudouOUC's "no deterministic backstop in finalize for the do-not-auto-approve cap" and the ci-bot's round-5 mirror-image finding (the non-maintainer + high-risk signal has no clearing rule, so it re-applies the label after a maintainer removes it). All three are the same design gap — the label lifecycle (apply / clear / re-apply) is not deterministic in either direction. If Rules 2–3 land in this PR, the clearing semantics should be fixed here; otherwise consider shipping Rule 1 alone and landing Rules 2–3 together with working label mechanics in the follow-up. The design doc already concedes Rules 2–3 are not statistically significant (p = 0.33 / 0.098), so deferring them costs little. [Suggestion] The shipped contested predicate is not the measured oneThree formulations of the contested signal now coexist: the PR body and design-doc Rule 2 say "a [Suggestion] Commit the analysis scriptsThe skill text and both test files hard-code analysis outputs ("p = 0.006", "10 of 31 vs 5 of 60"), and the doc's own follow-up says the path list should be kept current by re-running the analysis — but the scripts and data are explicitly "local analysis artifacts, not committed". As merged, nobody can verify or refresh these numbers. Spot check: Nits
Verdict: Rule 1 is data-backed, correctly implemented, escalate-only, and tested — mergeable as-is. The contested-merge/label mechanics (Rules 2–3) have the reachability flaw above and shouldn't ship in their current form: fix the clearing semantics here, or split Rules 2–3 into the follow-up that also addresses the two deferred findings. 中文说明在 head 亮点： 只升级不阻断的设计、设计文档诚实的统计口径（抽样比例告警、基准率下的 PPV、Rules 2–3 的显式移除条件）、第 3 轮把 [Major] contested-merge 信号按现有文本永远无法首次触发。 triage 只在 [Suggestion] 实际实现的 contested 判定与测量的不一致。 PR 描述和设计文档 Rule 2 说的是"CHANGES_REQUESTED → APPROVED 循环"；文档实现节说"position 0 之后出现 CHANGES_REQUESTED"；skill 实际实现的是第三种更严的判定（position 0 之后、且来自此前未出现过的 reviewer）。经典的测量场景（同一 reviewer 先 CR 后 approve）不满足实际规则，50.0%/19.4% 的数字对应的是另一个 PR 集合。应把文档与实现对齐，并重新推导或停止引用这些指标。（附带：review 数组只排除了 [Suggestion] 提交分析脚本。 skill 与两个测试文件都硬编码了分析结论（"p = 0.006"、"10 of 31 vs 5 of 60"），文档的 follow-up 也要求定期重跑分析，但脚本与数据"未提交"。抽查：本地 Nits： pr-workflow.md:239 的 结论： Rule 1 可直接合入；Rules 2–3 的 label 机制存在上述可达性缺陷，建议在本 PR 修复清除语义，或拆到后续 PR 与两条已推迟发现一并解决。 |
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round 5 — Review feedback addressedFeedback classification and decisions1. [Major] Contested-merge clearing semantics unreachable → Escalated to maintainer The reviewer correctly identifies that the clearing rule cannot distinguish "label was never applied" from "maintainer cleared it" — on the first re-triage after a contested review appears, the label is absent because it was never applied, so the rule reads that as "resolved" and the signal never engages. This is a scope/product decision I cannot settle:
Recommendation: Option B. Rule 1 is data-backed (p = 0.006), correctly implemented, and independently useful. Rules 2–3 share a common label-lifecycle design gap that affects three findings; fixing them together in a follow-up with coherent clearing semantics is simpler than patching incrementally. Leaving this UNRESOLVED for the maintainer to decide. 2. [Suggestion] Contested predicate mismatch → Dependent on #1 If Rules 2–3 are deferred (Option B), this is moot. If they stay, the doc's Rule 2 prose needs alignment with the shipped predicate (CR after position 0 from a reviewer not seen earlier, not the classic CR→APPROVED cycle). The minor bot-exclusion note ( 3. [Suggestion] Commit analysis scripts → Declined The scripts ( 4. [Nit] GNU-only grep BRE alternation → Fixed Changed 5. [Nit] Stale PR body → Noted, not fixable here The PR body still references "status/on-hold" (changed to Changes this round
Conflict notesNo conflict ( Verification
中文说明第 5 轮 — 审查反馈处理反馈分类与决定1. [Major] contested-merge 清除语义不可达 → 升级给维护者决定 审查者正确指出：清除规则无法区分"label 从未被应用"和"维护者已清除"——在 contested review 出现后的首次 re-triage 中，label 本来就不存在（因为从未被应用），规则会把它误读为"已解决"，导致信号永远不生效。 这是一个我无法自行决定的范围/产品决策：
建议： 方案 B。Rule 1 有数据支撑（p = 0.006）、实现正确、独立可用。Rules 2–3 共享同一个 label 生命周期设计缺口，影响三条发现；在后续 PR 中用一致的清除语义一并修复，比逐步打补丁更简单。 此项保持 UNRESOLVED，等待维护者决定。 2. [Suggestion] contested 判定与测量不一致 → 取决于 #1 若 Rules 2–3 被推迟（方案 B），此项不再适用。若保留，文档 Rule 2 的描述需与实际实现的判定对齐（position 0 之后、来自此前未出现过的 reviewer 的 CR，而非经典的 CR→APPROVED 循环）。附带的 bot 排除问题（ 3. [Suggestion] 提交分析脚本 → 拒绝 脚本（ 4. [Nit] GNU 专有 grep BRE 交替 → 已修复 将 pr-workflow.md:239 的 5. [Nit] PR 描述过期 → 已记录，此处无法修复 PR 描述仍引用 "status/on-hold"（第 2 轮已改为 本轮变更
冲突说明无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Patrol note: I am not changing code for the remaining Stage 1e comments in this pass. The finalize-workflow backstop requests would require expanding this PR into The The |
yiliang114
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
github-actions
Bot
removed
the
review/self-reported
|
Follow-up applied after maintainer approval: added the Verified with the focused triage workflow tests, the mirrored Node test, Prettier, and |
|
Review closeout update: I trimmed this PR back to the evidence-supported signal only. Fixed / changed:
Review adjudication:
Verification run:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Maintainer verification — built and ran locallyVerified on the PR head Verdict: one blocker before merge. The stated behaviour ("escalates depth, never blocks or closes") is correct and the statistics are honestly computed and reproduce exactly. But the Stage 1e snippet fails open on every realistic API error, which silently defeats the feature and makes the bot post a false safety claim. The fix is three lines. 1. The reviewer test plan passesAll three commands from the PR description, run as written:
The added assertions are string-presence checks on markdown, so I checked they are not vacuous:
For the record, 2. 🔴 Blocker — Stage 1e fails open; the
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed head 35178968cfa28500e14daf84bd73dfe41f220aa5 in full. Approving — my earlier Changes-Requested is superseded.
My prior blocking concern, re-checked against HEAD: the Changes-Requested I filed on 96f8dfe3 was about Stage 1e's "do not auto-approve" directive having no deterministic backstop in the finalize workflow. That concern is moot at HEAD by design change: the contested-merge and non-maintainer+high-risk rules were removed entirely, "do not auto-approve" no longer appears anywhere in the skill text, and Stage 1e is now escalation-only — it explicitly states a high-risk PR that passes full review can still be approved. No backstop is needed for a signal that never withholds approval.
Independent verification performed:
- Checked out the PR head into a clean worktree, ran all three commands from the test plan:
node --test .github/scripts/qwen-triage-workflow.test.mjs→ 30/30 pass;npx vitest run scripts/tests/qwen-triage-workflow.test.js→ 75/75 pass;npx vitest run scripts/tests/qwen-triage-finalize-workflow.test.js→ 19/19 pass. CITest (ubuntu-latest, Node 22.x)is green. - Confirmed every string asserted by the new tests exists in
pr-workflow.mdat HEAD, and that both runners (node:test mirror + vitest) carry the same Stage 1e assertions. - Stage 1e slots correctly after the existing 1a–1d sequence; the
Risk:field is present in both the English and Chinese Stage 1 comment templates. - Recomputed the headline statistic: one-sided Fisher exact for 10/31 vs 5/60 gives p = 0.0053, consistent with the doc's p = 0.006 claim.
- The
gh apifailure mode is handled (empty$FILES→ explicit WARNING instead of silently treating the PR as low-risk), and|| trueonly suppresses grep's no-match exit.
Nit (non-blocking, no code change needed): the PR description says "71% of reverts occur within 24 hours" while the design doc says 61.5% (of the 13 PRs with valid gap data). Worth aligning the description text to the doc's number at your convenience.
All 40 review threads are resolved with replies; scope is triage-skill text, docs, and tests only — no workflow YAML or production code paths touched. The escalation-only design keeps the blast radius to extra review depth, which matches the evidence bar argued in the design doc.
| FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') | ||
| if [ -n "$FILES" ]; then |
There was a problem hiding this comment.
[Critical] Stage 1e bash snippet fails open on every realistic API error — Failure scenario: gh api writes JSON error bodies to stdout on failure (404, rate limit, invalid token). $() captures this, so [ -n "$FILES" ] is true, the then-branch runs, grep -E finds no paths in the error JSON, || true swallows the exit, and the snippet emits nothing — byte-for-byte identical to a genuinely clean PR. The WARNING else-branch is unreachable (only fires when gh is not installed). Combined with the Risk: template's binary logic (line 270: "no elevated risk signals" as the only fallback), a fetch failure causes the bot to post a false "clean" assessment on a PR whose files it never read.
| FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') | |
| if [ -n "$FILES" ]; then | |
| if ! FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename' 2>/dev/null); then | |
| echo "WARNING: could not fetch PR files" | |
| elif [ -z "$FILES" ]; then | |
| echo "WARNING: could not fetch PR files" | |
| else | |
| 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 | |
| fi |
Also add a third state to the Risk: template (line 270) so a fetch failure renders as "could not evaluate (API error)" instead of "no elevated risk signals".
中文说明
[Critical] Stage 1e bash 片段在所有现实的 API 错误场景下 fail-open —— 失败场景：gh api 在失败时（404、限流、无效 token）将 JSON 错误体写到 stdout。$() 捕获了它，所以 [ -n "$FILES" ] 为真，then 分支执行，grep -E 在错误 JSON 中找不到路径，|| true 吞掉退出码，片段输出为空——与真正干净的 PR 逐字节一致。WARNING else 分支不可达（仅在 gh 未安装时触发）。结合 Risk: 模板的二元逻辑（第 270 行："no elevated risk signals" 是唯一备选），抓取失败会导致 bot 在一个从未读取过文件的 PR 上发布虚假的"干净"评估。
同时请给 Risk: 模板补一个第三态，让抓取失败渲染为"无法评估（API 错误）"而不是"无升级风险信号"。
— qwen3.7-max via Qwen Code /review
| 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. |
There was a problem hiding this comment.
[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."
| `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
| **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') |
There was a problem hiding this comment.
[Suggestion] Missing -F per_page=100 — Concrete cost: the GitHub pulls/{number}/files endpoint defaults to per_page=30. For a PR with 200 files, Stage 1e makes 7 paginated API calls instead of 2. Five of the six other gh api --paginate calls in this file (lines 33, 53, 98, 409, 610) use -F per_page=100; this one is inconsistent, accelerating rate limit consumption on large PRs.
| FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[].filename') | |
| FILES=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" -F per_page=100 --jq '.[].filename') |
中文说明
[Suggestion] 缺少 -F per_page=100——具体代价：GitHub pulls/{number}/files 端点默认 per_page=30。对于 200 个文件的 PR，Stage 1e 会发起 7 次分页 API 调用而非 2 次。同文件中其他 6 个 gh api --paginate 调用中有 5 个使用了 -F per_page=100，此处不一致，会在大型 PR 上加速消耗限流配额。
— qwen3.7-max via Qwen Code /review
|
Released in v0.21.1. |
6 participants
What this PR does
Replaces the original behavior-neutral PR filter (~2% hit rate, rejected by maintainer data) with one data-backed Stage 1e escalation signal based on revert-history analysis of this repo.
The retained signal is high-risk path detection: PRs touching streaming parsers, ACP connection, shell tool, MCP, or LSP paths are escalated to the deepest review tier. The gate only escalates review depth and flags risk; it never blocks, closes, labels, or withholds approval by itself.
The earlier contested-merge and non-maintainer + high-risk rules were removed. Their measured p-values did not clear the evidence bar, and keeping them would add label lifecycle/backstop complexity that does not fit this PR's minimal goal.
Why it is needed
71% of reverts occur within 24 hours of merge. The original PR (#7414 v1) tried to filter harmless PRs; this revision keeps only the revert-correlated path signal with strong evidence, avoiding broader policy machinery.
Reviewer Test Plan
How to verify
Confirm that Stage 1e only escalates review depth and never blocks or closes PRs. Confirm the high-risk path patterns match the analysis data. Run the triage workflow tests:
node --test .github/scripts/qwen-triage-workflow.test.mjs,npx vitest run scripts/tests/qwen-triage-workflow.test.js -t "includes high-risk path detection|includes Risk field", andnpx vitest run scripts/tests/qwen-triage-finalize-workflow.test.js.Tested on
Risk and Scope
Linked Issues
Related to #7411
Related to #6264
中文说明
本 PR 做了什么
用基于仓库 revert 历史数据分析的一条 Stage 1e 升级信号，替代原始 behavior-neutral 过滤方案。
保留的信号只有高风险路径检测：触碰流式解析器、ACP 连接、shell 工具、MCP、LSP 路径的 PR 升级到最深 review。这个门禁只升级 review 深度并标记风险；不会阻断、关闭、打标签或单独阻止 approve。
之前的 contested-merge 和非维护者 + 高风险规则已经删除。它们的 p-value 没过证据门槛，继续保留会引入 label 生命周期和 finalize backstop 复杂度，不符合这个 PR 的最小目标。
为什么需要
71% 的 revert 发生在合入后 24 小时内。原 PR（#7414 v1）想过滤无害 PR；这版只保留证据最强的路径信号，避免扩大成复杂 policy 机制。