fix(ci): treat a PR closed during a triage re-run as a terminal action - #10334
Conversation
The `Notify silent triage re-run` step in qwen-triage.yml decides solely
from the review list (HAS_REVIEW / STANDING). A Stage 1-pre duplicate-close
exit terminates a run without leaving any review, so the step cannot tell it
apart from a re-run that did nothing: it POSTs the stage=rerun-summary
comment ("Triage re-run completed without a new review ... it did not") on
the PR the same run just closed, and emits ::warning title=Triage re-run
left no bot review, dispatching a human to a correctly-handled run.
Read the PR's own state before warning: if closedAt is at or after the
trigger comment, the close is this run's terminal action — exit quietly with
no summary comment and no warning. A close BEFORE the trigger keeps the
existing notify behaviour, and a failed `gh pr view` falls through to the
review-list check as before. Only the notify step changes; no other step is
touched.
Tests: scripts/tests/qwen-triage-workflow.test.js gains an executed test
driving the real step body with a stubbed gh in the close-exit shape (no
comment, no warning) and the closed-before-trigger boundary (old behaviour
preserved); the existing executed pin for open PRs is extended to serve the
new pr view call so the pinned no-review notification behaviour is guarded.
Fixes #10324
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, with a linked issue (#10324, Direction: aligned. This fixes the triage workflow's own notify step misreading a terminal close as a silent no-op — internal CI correctness, squarely in scope. Size: not applicable — no core paths. 21 production lines in one workflow step (~10 of them comments), 102 test lines. Approach: the scope feels right. Reading the PR's own Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,有关联 issue(#10324, 方向:对齐。修的是 triage 工作流自己的通知步骤把"终止性关闭"误读为"静默无动作"—— 纯内部 CI 正确性,完全在范围内。 规模:不适用 —— 未触及核心路径。单个工作流步骤 21 行生产代码(约 10 行是注释),102 行测试。 方案:范围合理。用既有的 风险:无升级风险信号(未命中高风险路径)。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent take before reading the diff: the notify step decides solely from the review list, so it needs to read the PR's own close state before deciding — one The implementation is clean:
No blockers, no convention violations. Test evidenceUnattended CI run — no PR code was built or executed here; the evidence below is the PR's own CI, fetched via the API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查:独立构思的方案与 PR 完全一致 —— 在步骤开头读一次 测试证据:无人值守 CI 运行 —— 此处未构建或执行任何 PR 代码,以下为通过 API 获取的 PR 自身 CI 结果。撰写时 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — observed bug with a reproduction, the minimal fail-safe fix, and an executed pin that goes red without it; nothing left to want. Stepping back: this is exactly the change the notify step was missing. The step reasons about "did this re-run do anything?" purely from the review list, and a terminal close is the one action that leaves no review — so reading the PR's own close state before warning is the right fix, placed at the right point, failing through to today's behavior when anything is unreadable. My independent proposal matched the PR's approach line for line; there was no simpler path to find. The scope is disciplined too — one step, no drive-by edits, and the check is shape-agnostic, which is what lets it land before #10292 introduces the Stage 1-pre close it was designed for. In six months this reads as an obvious invariant ("don't announce a missing review on a PR this run just closed"), not a special case. CI is still queued at the time of writing, so approval is deferred until CI lands green on 中文说明回顾全局:这正是通知步骤缺的那块。该步骤只靠评审列表判断"这次重跑是否做了任何事",而终止性关闭恰恰是不留评审的动作 —— 在告警前读取 PR 自身的关闭状态是正确的修复、放在正确的位置、任何读取失败都回落到现状。我的独立构思与 PR 的方案完全一致,没有更简的路径。范围也很克制 —— 只改一个步骤、无顺手改动,检查与形态无关,因此可以先于 #10292(引入 Stage 1-pre 关闭的 PR)合入。六个月后回看,这是一条显而易见的不变量("不要在本轮刚关闭的 PR 上宣告缺失评审"),而不是特例。撰写时 CI 仍在排队,因此批准推迟到 CI 在上述提交上全绿。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| '#!/usr/bin/env bash', | ||
| 'case "$*" in', | ||
| ` "api user --jq .login") echo bot ;;`, | ||
| ` "pr view 1 --repo QwenLM/qwen-code --json state,closedAt") cat "${join(dir, 'pr-state.json')}" ;;`, |
There was a problem hiding this comment.
[Suggestion] This is the third near-verbatim copy of the fake-gh stub + run() harness in this file (the adjacent notify test at ~1196-1244 is the second), even though makeGhHarness (line ~92) was created to eliminate pasted stubs — its header comment records that pasted copies "already diverged once". This very diff demonstrates the coupling: adding gh pr view to the step forced an edit to the OLD test's stub (line ~1208), whose catch-all *) exit 1 would otherwise have failed every old scenario. The two notify stubs already differ semantically (hard-coded {"state":"OPEN","closedAt":null} vs cat pr-state.json; $FAKE_HEAD failure injection vs fixed echo head), so a future fix to a shared arm (comment capture, login answer, pr-state shape) landing on only one copy silently ages the other test. Consider hoisting one parameterized notify harness to file scope — e.g. makeNotifyHarness({ prState, reviews, head }) returning { run, cleanup } — and driving both the existing no-review notify test and this new close-exit test from it, keeping the FAKE_HEAD failure arm parameterized. Refactor only: both notify-step tests must stay green across the extraction.
中文说明
这是本文件中第三份近乎逐字复制的 fake gh stub + run() harness(相邻 ~1196-1244 的通知测试是第二份),尽管 makeGhHarness(~92 行)正是为消除粘贴 stub 而创建的 —— 其头部注释记载粘贴的副本"已经分歧过一次"。本 diff 恰好演示了这种耦合:往步骤里新增 gh pr view 迫使旧测试的 stub(~1208 行)也被修改,否则其兜底分支 *) exit 1 会让所有旧场景失败。两个通知 stub 已经在语义上分歧(硬编码 {"state":"OPEN","closedAt":null} 对比 cat pr-state.json;$FAKE_HEAD 失败注入对比固定 echo head),未来对共享分支(评论捕获、login 应答、pr-state 形态)的修复若只落在其中一份上,会悄悄让另一份测试过时。建议提取一个参数化的通知 harness 到文件作用域 —— 例如 makeNotifyHarness({ prState, reviews, head }) 返回 { run, cleanup } —— 让既有的无评审通知测试和这个新的关闭退出测试共用,并保持 FAKE_HEAD 失败分支参数化。纯重构:提取过程中两个通知测试须保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| # exit quietly; a close BEFORE the trigger keeps the old behaviour, | ||
| # because this run still owes its summary. A failed `gh pr view` | ||
| # is not fatal: the review-list check below then decides, as before. |
There was a problem hiding this comment.
[Suggestion] The fallback this comment documents — a failed gh pr view falling through to the review-list check — is correct today only because PR_STATE="$(gh pr view ...)" sits inside the if condition, where set -euo pipefail is suppressed. No test pins that placement: every fake gh in scripts/tests/qwen-triage-workflow.test.js serves the pr view call successfully. A future edit that hoists the assignment out of the if makes a transient gh API failure (rate limit, network blip) abort the whole notify step under errexit — no stage=rerun-summary comment and no ::warning for a genuinely silent re-run — and nothing in the suite turns red. Verified by probe: with the assignment hoisted (mutant), a failing pr view makes the step exit 1 with no comment and no warning, while both shipped notify tests still pass; on the PR code the same failing call correctly falls through (exit 0, comment + warning posted).
Suggested fix: add a failure arm for the pr view case in the new test's fake gh (exit 1), plus a scenario asserting the declared fallback — with an empty review list the step still exits 0, the log contains Triage re-run left no bot review, and the posted comment contains <!-- qwen-triage stage=rerun-summary -->.
Witness:
PR prview-fail: status=0, marker=true, warning=true
MUTANT (assignment hoisted out of the if): status=1, commentPosted=false, warning=false
shipped notify tests against the mutant: 2 passed
Fix witness: that new scenario itself — hoisting PR_STATE="$(gh pr view ...)" out of the if condition makes the step exit non-zero under the failing arm, so the scenario's status assertion goes red (remove the guard's placement, run that test, confirm it fails).
中文说明
此评论所记载的兜底行为 —— gh pr view 失败时回落到评审列表检查 —— 目前之所以成立,仅仅因为 PR_STATE="$(gh pr view ...)" 位于 if 条件内部(set -euo pipefail 在此处被抑制)。没有任何测试钉住这一位置:scripts/tests/qwen-triage-workflow.test.js 中的所有 fake gh 都成功应答 pr view 调用。未来若有编辑把该赋值提出 if 条件,一次瞬时的 gh API 失败(限流、网络抖动)就会在 errexit 下中止整个通知步骤 —— 对一次真正静默的重跑既不发 stage=rerun-summary 评论、也不发 ::warning —— 且测试套件中没有任何测试变红。已通过探针验证:把赋值提出 if(突变体)后,pr view 失败会使步骤以 1 退出,无评论、无告警,而两个已装船的通知测试仍然通过;在 PR 代码上,同样的失败调用则正确回落(退出 0,发出评论和告警)。
建议修复:在新测试的 fake gh 中为 pr view 增加一个失败分支(exit 1),并新增一个场景断言所声明的兜底行为 —— 评审列表为空时步骤仍以 0 退出,日志包含 Triage re-run left no bot review,发出的评论包含 <!-- qwen-triage stage=rerun-summary -->。
修复见证:该新场景本身 —— 把 PR_STATE="$(gh pr view ...)" 提出 if 条件后,步骤在失败分支下以非 0 退出,场景的 status 断言变红(移除该守卫位置、运行该测试、确认其失败)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // The boundary: a close BEFORE the trigger comment is not this run's | ||
| // terminal action, so the old notify behaviour stays intact. | ||
| const closedBefore = run({ | ||
| state: 'CLOSED', | ||
| closedAt: '2026-01-01T00:00:00Z', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The equality boundary — the "at" of the documented "at/after the trigger" semantics, .closedAt >= $since — is exercised by no test: only strictly-after (2026-01-03) and strictly-before (2026-01-01) fixtures exist against TRIGGERED_AT 2026-01-02. GitHub timestamps are second-granularity, so a PR closed in the same second as the /triage comment is a real input; under a >= → > mutation it falls through to the review-list check and posts the contradictory stage=rerun-summary comment plus ::warning title=Triage re-run left no bot review on a just-closed PR — precisely the false alarm this PR exists to suppress — and nothing in the suite turns red. Verified by probe: the equality case exits quietly on the PR code, flips to warning + comment under the > mutant, both shipped notify tests pass against the mutant, and the fixture suggested below is red on the mutant and green on the PR.
Suggested fix: add a third fixture equal to the trigger time:
const closedAtTrigger = run({
state: 'CLOSED',
closedAt: '2026-01-02T00:00:00Z',
});
expect(closedAtTrigger.status).toBe(0);
expect(closedAtTrigger.comment).toBe('');
expect(closedAtTrigger.log).toContain('no re-run summary needed');Fix witness: that equality fixture — changing the jq filter's >= to > makes it fail while both existing fixtures stay green (apply the mutation, run the test, confirm it fails).
中文说明
等值边界 —— 所记载的"触发之时或之后(at/after)"语义中的"at",即 .closedAt >= $since —— 没有任何测试覆盖:相对 TRIGGERED_AT 2026-01-02,现有 fixture 只有严格之后(2026-01-03)和严格之前(2026-01-01)。GitHub 时间戳精确到秒,因此 PR 在与 /triage 评论同一秒被关闭是真实输入;若把 >= 突变为 >,该情形会回落到评审列表检查,在刚刚关闭的 PR 上发出自相矛盾的 stage=rerun-summary 评论和 ::warning title=Triage re-run left no bot review —— 恰是本 PR 要消除的误报 —— 而套件中没有任何测试变红。探针已验证:等值情形在 PR 代码上静默退出,在 > 突变体下翻转为告警 + 评论,两个已装船的通知测试对突变体仍然通过,且上面建议的 fixture 在突变体上为红、在 PR 代码上为绿。
建议修复:新增第三个与触发时间相等的 fixture(见上方代码块)。
修复见证:该等值 fixture —— 把 jq 过滤器的 >= 改为 > 会使其失败,而现有两个 fixture 保持为绿(施加该突变、运行测试、确认其失败)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // The close-exit shape: closed after the trigger, no review left. | ||
| // Neither the summary comment nor the warning may fire. | ||
| const closeExit = run({ | ||
| state: 'CLOSED', | ||
| closedAt: '2026-01-03T00:00:00Z', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The exemption's most common real-world input — a PR MERGED at/after the trigger — is pinned by no fixture: both fixtures use state: 'CLOSED', while the guard queries --json state,closedAt and keys solely on closedAt, never reading state. GitHub sets closedAt on merge as well (verified against the live API, e.g. #10329), so a PR merged mid-run gets the quiet exit today; but a future edit that narrows the guard to also require .state == "CLOSED" reintroduces the false-alarm summary comment and ::warning on merged PRs, and both current fixtures stay green. Verified by probe: under that mutant the MERGED case flips to warning + rerun-summary comment while both shipped CLOSED fixtures stay byte-identical and green.
Suggested fix: add a fixture run({ state: 'MERGED', closedAt: '2026-01-03T00:00:00Z' }) asserting the same quiet exit (status 0, empty comment, no warning); or drop state from the --json query if it is deliberately unused, so the guard cannot drift toward it.
Witness:
PR MERGED-after-trigger: quietExit=true, no warning, no comment
MUTANT `and .state == "CLOSED"`: MERGED flips to warning=true + rerun-summary comment
shipped CLOSED fixtures against the mutant: byte-identical, green
Fix witness: the MERGED fixture itself must go red if the guard is mutated to and .state == "CLOSED", while both CLOSED fixtures stay green (apply that mutation, run the test, confirm it fails).
中文说明
该豁免最常见的真实输入 —— 触发之时或之后被合并(MERGED)的 PR —— 没有任何 fixture 钉住:两个 fixture 都用 state: 'CLOSED',而守卫查询 --json state,closedAt 却只依据 closedAt、从不读取 state。GitHub 在合并时同样会设置 closedAt(已在真实 API 上确认,如 #10329),因此运行中被合并的 PR 今天会走静默退出;但未来若有编辑把守卫收窄为同时要求 .state == "CLOSED",就会在已合并的 PR 上重新引入自相矛盾的摘要评论和 ::warning,而现有两个 fixture 仍然为绿。探针已验证:该突变体下 MERGED 情形翻转为告警 + rerun-summary 评论,而两个已装船的 CLOSED fixture 逐字节不变且仍通过。
建议修复:新增 fixture run({ state: 'MERGED', closedAt: '2026-01-03T00:00:00Z' }),断言同样的静默退出(status 0、空评论、无告警);或者,如果 state 确属有意不用,就从 --json 查询中去掉它,使守卫无法向它漂移。
修复见证:该 MERGED fixture 本身必须在守卫被突变为 and .state == "CLOSED" 时变红,且两个 CLOSED fixture 保持为绿(施加该突变、运行测试、确认其失败)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
Scope: CI (head a7a83c6):
Implementation review: The new
The existing-pin test was correctly extended (line ~1208) to answer the new No blockers found. Cross-check vs. existing review (4 suggestions — all confirmed):
All four are suggestion-level (test quality and maintainability, not functionality). The implementation is correct and CI is green on Linux. I judge none must-fix for this PR. Reviewed with AI assistance. |
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at a7a83c6 — no findings. Verified: TRIGGERED_AT comes from github.event.comment.created_at and GraphQL closedAt is ISO-8601 Z as well, so the lexicographic >= comparison is chronologically valid; both sides fall inside the if condition, so a failed gh pr view or jq does not trip set -euo pipefail and the step falls through to the review-list check exactly as the comment claims. The pre-existing gh stand-in in the older notify test was extended for the new pr view call (so existing cases still run), and the new test pins both the quiet-exit shape (closed after trigger, no comment, no warning) and the boundary (closed before trigger keeps the rerun-summary behavior). Consistent with #10292 Stage 1-pre, whose close path is what this exemption serves.
Self-PR: GitHub blocks self-approval, so this is a comment review; merge approvals need to come from other maintainers.
|
Released in v0.22.3. |
What this PR does
Makes the
Notify silent triage re-runstep in.github/workflows/qwen-triage.ymlaware of the PR's own close state. Before deciding whether to warn about a missing bot review, the step now fetchesgh pr view "$NUMBER" --repo "$GITHUB_REPOSITORY" --json state,closedAt. If the PR was closed at or after the/triagetrigger comment, the close is treated as the run's terminal action and the step exits quietly — nostage=rerun-summarycomment, no::warning title=Triage re-run left no bot review. A close before the trigger keeps today's notify behavior, and a failedgh pr viewfalls through to the existing review-list check. Only this one step changes; no other step is touched.Why it's needed
Stage 1-pre's duplicate-close exit is the first terminal triage action that leaves no PR review, and this step decides solely from the review list (
HAS_REVIEW/STANDING). As reported in #10324 — and reproduced by driving the extracted step body with a stubbedghin the close-exit state — a/triagere-run that closes the PR as a duplicate then POSTs the contradictorystage=rerun-summarycomment ("Triage re-run completed without a new review … If this re-run was meant to review or approve, it did not") on the same PR it just closed, and emits::warning title=Triage re-run left no bot review, dispatching a human to a correctly-handled run. The check is shape-agnostic (it also covers a maintainer closing the PR mid-run), so it can land ahead of #10292.Reviewer Test Plan
How to verify
scripts/tests/qwen-triage-workflow.test.jsgains an executed test that extracts the real step body and drives it with a stubbedgh(same pattern as the existing pin): (1) the close-exit shape — PR closed after the trigger, empty review list — must exit 0 with no comment POSTed and no warning; (2) the boundary — closed before the trigger — must keep the existing warning plus thestage=rerun-summarycomment. The existing executed pin for open PRs is extended to serve the newpr viewcall, so the pinned no-review notification behavior is regression-guarded. Runvitest run --config scripts/tests/vitest.config.ts scripts/tests/qwen-triage-workflow.test.js; the new test goes red without the workflow change (verified).Evidence (Before & After)
Before (extracted step driven in the close-exit state): POSTs the 908-char
stage=rerun-summarycomment ending in "If this re-run was meant to review or approve, it did not" and emits::warning title=Triage re-run left no bot review. After: logsPR closed at/after the trigger comment; no re-run summary needed., POSTs nothing, warns nothing; an open PR in the same no-review state still receives the full bilingual summary and warning.Tested on
Environment (optional)
Workflow-script tests only: vitest with a stubbed
ghon PATH, real step body, GitHub's exact shell flags. No live workflow run was triggered.Risk & Scope
gh pr viewfailure falls through to the existing review-list check, so the worst case is today's behavior (notify), never a silent miss of a real problem.Linked Issues
Fixes #10324
中文说明
本 PR 做了什么
让
.github/workflows/qwen-triage.yml中的Notify silent triage re-run步骤感知 PR 自身的关闭状态。在判断是否就缺失的机器人评审发出告警之前,该步骤现在会调用gh pr view "$NUMBER" --repo "$GITHUB_REPOSITORY" --json state,closedAt。如果 PR 在/triage触发评论之时或之后被关闭,则把这次关闭视为本轮运行的终止动作,步骤安静退出 —— 不发stage=rerun-summary评论,也不发::warning title=Triage re-run left no bot review。触发之前的关闭保持现有通知行为;gh pr view调用失败则回落到既有的评审列表检查。只改了这一个步骤,其他步骤一律未动。为什么需要
Stage 1-pre 的重复关闭退出是第一个不留下任何 PR 评审的 triage 终止动作,而该步骤只依据评审列表(
HAS_REVIEW/STANDING)判断。正如 #10324 所报告 —— 并已复现:把逐字提取的步骤体用 stub 的gh在关闭退出态下驱动 —— 一次把 PR 作为重复关闭的/triage重跑,会在它刚刚关闭的同一个 PR 上发出自相矛盾的stage=rerun-summary评论("Triage re-run completed without a new review … If this re-run was meant to review or approve, it did not"),并输出::warning title=Triage re-run left no bot review,把人力派往一次本就正确处理了的运行。该检查与具体形态无关(也覆盖维护者在运行中途手动关闭 PR 的情况),因此可以先于 #10292 合入。评审者测试计划
如何验证
scripts/tests/qwen-triage-workflow.test.js新增一个执行型测试:逐字提取真实步骤体,用 stub 的gh驱动(与既有钉住测试同一模式):(1) 关闭退出形态 —— 触发后关闭、评审列表为空 —— 必须以 0 退出,不 POST 任何评论、不发告警;(2) 边界 —— 触发之前关闭 —— 必须保留现有告警和stage=rerun-summary评论。既有针对 open PR 的执行型钉住测试被扩展为同时应答新增的pr view调用,从而守住"无评审重跑仍要通知"这一被钉住行为的回归。运行vitest run --config scripts/tests/vitest.config.ts scripts/tests/qwen-triage-workflow.test.js;不带工作流改动时新测试为红(已验证)。证据(修复前 / 修复后)
修复前(关闭退出态下驱动提取的步骤体):POST 一条 908 字符的
stage=rerun-summary评论,结尾是 "If this re-run was meant to review or approve, it did not",并输出::warning title=Triage re-run left no bot review。修复后:日志输出PR closed at/after the trigger comment; no re-run summary needed.,不 POST、不告警;open PR 在同样的无评审状态下仍然收到完整的双语摘要和告警。测试环境
环境(可选)
仅工作流脚本测试:PATH 上放 stub
gh的 vitest,执行真实步骤体、使用与 GitHub 完全一致的 shell 参数。未触发任何真实工作流运行。风险与范围
gh pr view瞬时失败会回落到既有的评审列表检查,因此最坏情况就是现状(发通知),绝不会静默漏掉真实问题。关联 Issue
Fixes #10324