feat(triage): stop in-agent CI polling, finalize evidence and approval after CI completes#7693
Conversation
…l after CI completes The triage agent's Stage 2b polled pending checks for up to 10 minutes, but this repo's unit suite runs ~30 minutes, so the poll always burned its full budget, gave up with 'CI still running', and Stage 3 could then approve before the suite finished (observed on a PR approved 12 minutes before its Test job completed). Split the wait out of the agent entirely: - pr-workflow.md Stage 2b now forbids polling: fetch check-runs once, report pending checks honestly, and wrap the CI table in qwen-triage-ci region markers keyed to the reviewed SHA. - Stage 3 defers a clean-verdict approval when checks are still pending: the comment carries an approve-on-green marker instead of an immediate APPROVE. - New qwen-triage-finalize.yml fires on workflow_run completion of 'Qwen Code CI' / 'E2E Tests' and, with plain bash over the API (no model, no checkout), rewrites the marked table region with the settled results and posts the commit-pinned approval only when every check landed green — failing closed on red checks, a moved head, or a closed/draft PR, and flipping the triage status comment to say which way it resolved. Markers are honored only in comments authored by the bot identity itself, and check names (attacker-influenced on fork PRs) go through the same HTML-escape chain the skill mandates for file paths. Stage comments now land ~10 minutes sooner and the approval, when deferred, lands at CI completion with full evidence instead of before it.
|
Thanks for the PR! Template looks good ✓ — all sections present with substantive content, including the bilingual translation. Problem: observed and measured, not theoretical. The #7680 timeline shows the poll loop burning 14:51→15:01 (10 minutes) and still giving up, with Stage 3 approving at 15:02:10 while Direction: aligned. Moving the "wait for CI" half out of the time-bounded agent run into a deterministic Size: not applicable — no core module paths touched. The 3 changed files are Approach: the scope feels right. Each of the three files is necessary: the new workflow (the core deliverable), the pr-workflow.md update (agent instructions to emit region markers and the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有章节齐全，包含中文翻译。 问题：已观测、已量化，非理论性问题。#7680 的时间线显示轮询循环在 14:51→15:01 空转 10 分钟后仍然放弃，Stage 3 于 15:02:10 审批，而 方向：对齐。将"等待 CI"这一半从有时间限制的 agent 运行中移出，交给确定性的 规模：不适用——未触及核心模块路径。3 个变更文件分别在 方案：范围合理。三个文件各有必要：新 workflow（核心交付物）、pr-workflow.md 更新（agent 指令，发出区域标记和 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (the agent's 10-minute poll budget structurally can't catch a ~30-minute unit suite), I would split the "wait for CI" responsibility out of the agent run entirely: fetch check-runs once, wrap the CI table in machine-readable markers, and let a deterministic Comparison with the diff: the PR's approach matches this almost exactly. The implementation is clean, well-commented (the "why" comments earn their place), and the security model is sound throughout:
The The pr-workflow.md changes integrate cleanly: the poll instruction is replaced with "never poll", the region marker template is added, the guardrail computation moves before the marker emission (closing the bypass), and the Stage 3 approval logic branches on the The tests cover the security-relevant structure (no checkout, bot-author filter, SHA-pinned approval, closed green set, self-check exclusion, per-SHA concurrency, guardrail re-assertion) and execute the extracted bash helpers against crafted inputs — No critical blockers or AGENTS.md violations found. sequenceDiagram
participant P1 as Triage Agent
participant P2 as GitHub CI
participant P3 as Finalize Workflow
participant P4 as PR Comments and Reviews
P1->>P4: Post Stage 2 with CI table in region markers
P1->>P4: Post Stage 3 with approve-on-green marker (if CI pending)
P2->>P3: workflow_run completed
P3->>P3: Fetch check-runs and workflow runs for reviewed SHA
P3->>P4: Rewrite CI table region in place
alt All PR CI runs green and head unchanged
P3->>P4: Post SHA-pinned approval
else Red runs, head moved, or PR closed
P3->>P4: Update status (withheld, stale, or deferred)
end
TestingCI status for Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Approval gate (workflow runs with This PR touches CI/triage infrastructure, not a TUI surface — no real-scenario testing needed. N/A. 中文说明代码审查独立方案： 鉴于问题（agent 的 10 分钟轮询预算在结构上无法等到约 30 分钟的单测套件），我会将"等待 CI"的职责完全移出 agent 运行：拉取一次 check-runs，用机器可读标记包裹 CI 表格，让一个确定性的 与 diff 的对比： PR 的方案几乎完全匹配。实现干净，注释到位，安全模型全程可靠：
未发现关键阻塞项或 AGENTS.md 违规。 测试
本 PR 触及 CI/triage 基础设施，非 TUI 界面——无需真实场景测试。N/A。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; the problem is measured, the architecture is right, the security model is thorough, and four rounds of self-review caught real bugs before they shipped. Stepping back: this PR fixes a structural flaw in the triage process — the agent's poll budget can never catch the unit suite, so the "CI evidence" section was always a shrug, and the approval once landed 12 minutes before the suite finished (#7680). The fix is the right split: the agent does what needs a model (the review), and a deterministic bash job does what doesn't (did CI land green?). No model tokens, no checkout, no sleep loops. What stands out across the four commits is the quality of the self-review. Each round caught a real issue: the original RED gate had a jq error that would have approved regardless of failures (d2cb023), the E2E Tests trigger was dead code (d2cb023), a stale finalize firing could clobber a newer review's status comment (cadf3f0), and the approve-on-green marker could bypass the fork-refactor guardrail because it was emitted before the check ran (75ee714). That last one is the most important — without it, a fork refactor PR could have been auto-approved on green CI despite the explicit policy against it. The fix moves the guardrail computation before the marker emission and re-asserts it structurally in the finalize job itself. The 75ee714 commit also aligns the finalize table with the gate's data source — Every change in the diff is necessary for the stated goal. No drive-by refactors, no scope creep. The pr-workflow.md changes integrate cleanly with the existing instructions, and the staleness direction is safe (a check that completed after the single fetch is merely treated as pending — defers, never approves early). CI is all green on LGTM, looks ready to ship. ✅ 中文说明置信度：5/5 —— 每个阶段都干净；问题已量化，架构正确，安全模型周全，四轮自审在发布前捕获了真实 bug。 退一步看：这个 PR 修复了 triage 流程中的结构性缺陷——agent 的轮询预算永远等不到单测套件，审批曾经在套件完成前 12 分钟就落地（#7680）。修复方案是正确的拆分：agent 做需要模型的事（审查），确定性 bash 任务做不需要模型的事（CI 是否全绿）。 四个提交中最突出的是自审的质量。每一轮都捕获了真实问题：原始 RED 门控有 jq 错误（d2cb023），E2E Tests 触发器是死代码（d2cb023），过期触发可能覆盖新 review 的状态评论（cadf3f0），以及 approve-on-green 标记可能绕过 fork-refactor 护栏（75ee714）。最后一个最重要——没有它，fork refactor PR 可能在 CI 全绿时被自动审批，尽管有明确的政策禁止。修复将护栏计算移到标记发出之前，并在 finalize 任务中结构性地重断言。 75ee714 还将 finalize 表格与门控的数据源对齐—— diff 中的每个改动都是目标所必需的。没有顺手重构，没有范围蔓延。
— 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.
LGTM, looks ready to ship. ✅
Review —
|
| check | in the rendered table? |
|---|---|
Test (ubuntu-latest, Node 22.x) |
❌ |
Test (macos-latest / windows-latest) |
❌ |
web-shell E2E Smoke |
❌ |
Serve A/B |
❌ |
Three compounding causes:
- No dedup by name.
filter=latestdedups per check suite, not per name. Everypull_request_target/issue_commentbot run mints a new suite with the same job names — on perf(web-shell): paint the composer git chip before git status completes #7680 that is 96 check-runs across only 35 distinct names (authorize×7,review-pr×6,resolve-pr×6, …). - No skip filter. 68 of 96 rows are
skippedconditional bot jobs. - Alphabetical
sort -f+head -n 60. The cut lands mid-review-config, so everything fromS/T/u/wonward — i.e. every real test job — is truncated away.
So on a typical PR, finalize-triage-ci overwrites the agent's curated, human-meaningful table with 60 rows of duplicated skipped bot jobs and silently drops the unit-test result. Worse, a red check that sorts late is dropped too, leaving a table that reads as clean.
Fix (verified against the same data — 96 rows → 16, and it contains exactly the checks that matter, Test (ubuntu-latest, Node 22.x) included):
jq -r 'group_by(.name) | map(max_by(.id))
| map(select(.conclusion != "skipped"))
| sort_by([ (.status == "completed"), ((.conclusion // "") | IN("success","neutral")), (.name | ascii_downcase) ])
| .[] | [.name, .status, (.conclusion // "")] | @tsv' /tmp/checks.jsonSorting non-green first is the important half: it guarantees truncation can never hide a failure. Keep skipped in the approval gate (it is correctly green there) — just out of the table.
🟠 Medium
3. E2E Tests is a dead trigger. e2e.yml has no pull_request trigger — only push / schedule / workflow_dispatch. So if: github.event.workflow_run.event == 'pull_request' drops 100% of its firings. The claimed second convergence path ("so a late E2E finish also converges the table") cannot happen; Qwen Code CI is the only workflow that can ever fire this job. The new test asserts the two-entry list, codifying it.
4. The deferred approval can be silently dropped forever. PENDING counts every check-run on the SHA, and this repo sprays them onto the PR head from pull_request_target / issue_comment workflows — on #7680 the head SHA carries triage, review-pr, route, remove-suspicious-attachments, and a whole batch created at 15:39, long after CI finished. If any of those is in flight when finalize reads check-runs, PENDING > 0 → continue — with no status-comment update — and, since only Qwen Code CI can re-trigger the job (finding 3), no further firing ever arrives. Suggested: list the other pull_request workflows (SDK Python, Serve A/B, Web-shell Visuals, Qwen Autofix) in workflows:, and have the still-deferred branch update the status comment so the outcome is never invisible.
5. The red gate runs before the head/state re-check. ci.yml sets cancel-in-progress for PR refs, so a force-push cancels the in-flight run → workflow_run fires on the now-stale SHA → cancelled is non-green → update_status "$PR" red posts "CI finished with N non-green check(s) on <stale sha>". Because STATUS_MARKER is not SHA-scoped, that clobbers the status comment belonging to the new head. Move the CURRENT_HEAD != HEAD_SHA / state / draft re-check above the RED branch (or additionally gate on github.event.workflow_run.conclusion != 'cancelled'). Currently masked by blocker 1; it goes live the moment that is fixed.
6. replace_region truncates the comment when the end marker precedes the begin marker. grep -qF only proves both strings exist, not their order; awk then opens the region and eats to EOF. Reproduced:
input : "quoted example: <!-- /qwen-triage-ci -->" / prose / <begin> / | old | / FOOTER reviewed commit
output: quoted example… / prose / <begin> / | new | / <end> ← exit=0, FOOTER gone
exit 0, so the caller PATCHes the truncated body — losing the signature and the reviewed-commit footer, which is the attestation the whole triage flow rests on. The skill doc's "do not quote the marker text elsewhere" is a soft guard on LLM-authored prose; make it structural: END { if (inr) exit 1 } in the awk and check its status. The existing test covers only "end marker entirely absent", which grep already catches.
🟡 Low / nits
- Empty check-runs response ⇒ approve.
TOTAL=0givesPENDING=0,RED=0→ approve path. Add[ "$TOTAL" -gt 0 ] || { echo "::warning::no check-runs; skipping"; exit 0; }. - The self-check exclusion is a no-op, and the comment states it as fact.
workflow_runruns are attributed to the default branch, so this job's check-run never lands on the PR head SHA. Evidence:Serve A/B PublishandWeb-shell Visuals Publish(bothworkflow_run) appear nowhere in perf(web-shell): paint the composer git chip before git status completes #7680's 96 head-SHA check-runs, while theirpull_requestcounterparts do. Harmless as defense-in-depth, but the workflow comment ("attaches to the SAME head SHA") and the skill's addedselect(.name != "finalize-triage-ci")clause should say "belt-and-braces" rather than assert it. - The
cmp -s"already current" branch is unreachable. The region embedsRUN_URL, which carriesgithub.run_idand differs on every firing, so the bodies never compare equal and every trigger PATCHes. Either drop the run link from inside the region or diff with it masked. "(fork?)"in the missing-token message is misleading —workflow_runalways runs in the base-repo context with secrets available.MAX_ROWSaccounting:TOTALis counted before truncation but after self-exclusion, so "…and N more" is consistent — worth keeping in mind if the dedup fix lands (recount after dedup).
✅ Verified good
- 26/26 tests pass locally (13 existing + 13 new),
bash -nclean on the extracted script, and yamllint-clean — the double-quotedif:is fine because.yamllintsetsallow-quoted-quotes: true. scripts/tests/is wired into CI viatest:scripts→test:ci, so the new suite really runs.- Fixed-string matching everywhere (
grep -F,index(),contains) — no regex over untrusted text. - Escape chain matches the skill's
sanitize_path(&first, control chars stripped, length-bounded,<code>-wrapped), and the helper test proves the ordering. - Per-SHA concurrency with
cancel-in-progress: false; approval idempotence via thestate == "APPROVED" and .commit_id == $shapre-check;commit_idpinning.
Test coverage
Executing the extracted bash helpers is the right instinct — please extend it to the decision logic, which is where both blockers live. expect(script).toContain('["success","neutral","skipped"]') passes on code that raises a jq error every single run: a string assertion that reads as a safety guarantee while asserting nothing about behavior. Concretely, feed a fixture array covering success / neutral / skipped / failure / cancelled / timed_out / action_required / null / queued through the actual TOTAL / PENDING / RED expressions and assert the counts — that single test would have caught blocker 1. A rendering test over a fixture with duplicate names, skipped rows, and >60 entries would have caught blocker 2. Also, expect(workflowText).not.toContain('npm ') is brittle — it breaks if a comment ever mentions npm.
Repro
gh api "repos/QwenLM/qwen-code/commits/f35fc8dd10e7290696a2941a40549faf2bf92b6d/check-runs?per_page=100" \
--paginate --jq '.check_runs' | jq -s 'add // []' > checks.json
# blocker 1
jq '[.[] | select(.status=="completed")
| select(["success","neutral","skipped"] | index(.conclusion // "") | not)] | length' checks.json
# blocker 2
jq -r '.[] | [.name,.status,(.conclusion // "")] | @tsv' checks.json | sort -f | head -60 | grep -c 'Test (ubuntu'中文说明
总体评价
问题诊断准确，架构方向正确：确定性的 workflow_run 任务、无模型、无 checkout、contents: read、仅用 PAT 写入、全程定长字符串匹配、通过 gh api user 校验评论作者——这确实比一个结构上不可能等到 30 分钟测试套件的 sleep 60 轮询更好。令牌（QWEN_CODE_BOT_TOKEN || CI_BOT_PAT）与 qwen-triage.yml 发评论所用的一致，作者过滤能对上，这一点我已验证。
但我拿真实数据实跑了该 workflow 自身的代码，发现两个阻塞问题，且都恰好推翻了本 PR 的核心安全声明。当前不可合并。
🔴 阻塞 1 —— 红灯判定永不生效，CI 失败反而会自动审批
jq 中 | 会重绑 .，因此 index(...) 内部的 .conclusion 是对数组 ["success","neutral","skipped"] 求值，直接报错 Cannot index array with string "conclusion"（exit=5）。脚本没有 set -e，于是 RED 为空串；[ "" -gt 0 ] 报 integer expression expected 并返回非零，直接落入审批分支。
端到端复现（输入：1 个 skipped + Test (ubuntu-latest, Node 22.x) 为 failure）：
total=2 pending=0 non-green=
[: : integer expression expected
>>> 到达 APPROVE 分支 <<<
A/B 对照（仅把 jq 换成正确写法）：non-green=1 → approval withheld.
即：单测已经红了，延迟审批仍会提交一个 pin 到该 commit 的 APPROVE。这比它所替代的现状更差。修复：select((.conclusion // "") | IN("success","neutral","skipped") | not)，并在分支前校验三个计数均为数字。
🔴 阻塞 2 —— finalize 会把它本该保全的 CI 证据删掉
用 #7680（本 PR 的动机样本，f35fc8dd，96 条 check-run）实跑渲染逻辑：渲染出的 60 行里 45 行是 skipped、15 行 success，而 Test (ubuntu-latest / macos / windows)、web-shell E2E Smoke、Serve A/B 一条都没有。
三个叠加原因：① 未按名字去重（filter=latest 只在 check suite 内去重，96 条仅 35 个不同名字，authorize 出现 7 次）；② 未过滤 skipped（96 条中 68 条）；③ sort -f 字母序 + head -n 60，截断点正好落在 review-config，S/T/u/w 开头的真实测试任务全被切掉。红灯若排序靠后同样会被切掉，留下一张"看着很干净"的表。
修复（同一数据验证：96 → 16 行，且包含 Test (ubuntu-latest, Node 22.x)）：按 .name 去重取最新、剔除 skipped、非绿优先排序（保证截断永远切不掉失败项）。skipped 在审批门禁里仍按绿计算，只是不进表格。
🟠 中等
E2E Tests触发器是死的：e2e.yml没有pull_request触发器，if条件会 100% 过滤掉它的所有触发；实际只有Qwen Code CI能触发本任务，"E2E 晚到也能收敛表格"不成立，且新测试把这个两元素列表固化了下来。- 延迟审批可能被永久静默丢弃：
PENDING统计该 SHA 上的所有 check-run，而本仓库的pull_request_target/issue_commentbot workflow 会不断往 head SHA 上追加 check（perf(web-shell): paint the composer git chip before git status completes #7680 上有triage、review-pr、route、remove-suspicious-attachments，甚至 15:39 还新增了一批）。只要读取时有任一在跑，就PENDING > 0→continue，且不更新 status 评论，而后续再无触发。建议把其余pull_requestworkflow 加入workflows:列表，并在"仍延迟"分支更新 status 评论。 - 红灯判定排在 head/状态复核之前：
ci.yml对 PR ref 开了cancel-in-progress，force-push 会取消旧 run → 在过期 SHA 上触发 →cancelled记为非绿 → 发出"CI 有 N 项未通过（过期 SHA）"，而STATUS_MARKER不带 SHA，会覆盖新 head 的 status 评论。应把CURRENT_HEAD != HEAD_SHA复核前移。 replace_region在结束标记先于开始标记出现时会截断评论：grep -qF只证明两个字符串存在、不保证顺序，awk 随后一路吃到 EOF，且返回 0，调用方会 PATCH 掉被截断的正文——签名与 reviewed-commit footer（整套 triage 的背书凭据）随之丢失。已复现。建议 awk 加END { if (inr) exit 1 }并检查其退出码；现有测试只覆盖"完全没有结束标记"，那种情况 grep 已经拦住了。
🟡 低
- check-runs 返回空数组会导致审批：
TOTAL=0→PENDING=0、RED=0→ 走审批分支。建议加TOTAL -gt 0兜底。 - 自身 check 排除是空操作，且注释把它当作事实陈述：
workflow_run运行归属默认分支，本任务的 check-run 不会落到 PR head SHA 上（证据：同为workflow_run的Serve A/B Publish、Web-shell Visuals Publish在 perf(web-shell): paint the composer git chip before git status completes #7680 的 96 条 head-SHA check 中一条都没有）。作为冗余防御无害，但注释与 skill 文档中的select(.name != "finalize-triage-ci")应改成"保险起见"的措辞。 cmp -s的"已是最新"分支不可达：区域内嵌了含github.run_id的RUN_URL，每次触发都不同，因此每次都会 PATCH。"(fork?)"提示有误导：workflow_run始终在 base 仓库上下文运行，secrets 可用。
✅ 已验证的优点
26/26 测试本地通过（13 旧 + 13 新）；bash -n 通过；yamllint 通过（.yamllint 设了 allow-quoted-quotes: true，双引号的 if: 合规）；scripts/tests/ 经 test:scripts → test:ci 确实在 CI 中运行；全程定长字符串匹配；转义链与 skill 的 sanitize_path 一致（& 最先、剔除控制字符、限长、<code> 包裹）；按 SHA 串行且不取消；审批幂等（state == "APPROVED" and .commit_id == $sha）与 commit_id pin。
测试覆盖
把 bash 辅助函数抽出来实跑是对的方向，建议把同样的做法延伸到判定逻辑——两个阻塞问题都在那里。expect(script).toContain('["success","neutral","skipped"]') 在一段每次运行都报 jq 错误的代码上依然通过：它读起来像一条安全保证，实际上没有断言任何行为。建议用覆盖 success / neutral / skipped / failure / cancelled / timed_out / action_required / null / queued 的 fixture 跑真实的 TOTAL / PENDING / RED 表达式并断言计数——仅这一条测试就能拦住阻塞 1；再加一条含重名、skipped、>60 条目的渲染测试即可拦住阻塞 2。另外 expect(workflowText).not.toContain('npm ') 过于脆弱。
…ion, dead trigger Review findings on the finalize workflow, all reproduced before fixing: - Blocker 1: the RED jq used the array-first membership form, where | rebinds . and .conclusion indexes an array — jq exits 5 every run, RED comes back empty, [ "" -gt 0 ] errors, and control falls through to the approve path: a red CI auto-approved. The gate now binds the conclusion before the membership test (IN(...)), and the counters are numeric-validated so any future jq failure reads as 'cannot attest', never 'approve'. - Blocker 2: the table rendered raw check-runs — on a real PR (96 runs, 35 names, 68 skipped) alphabetical sort + head -60 truncated away every actual test job. table_rows now dedups per name (latest run), drops skipped rows, and sorts running/non-green first so the cap can only cut green rows. Replayed against the same PR: 96 rows -> 16, unit suite present. - The approval gate now reads workflow runs filtered to event=pull_request (deduped per workflow) instead of head-SHA check-runs, which also carry long-running bot orchestration jobs that would wedge PENDING above zero at the exact moment the last CI workflow fires — silently dropping the deferred approval forever. The skill's Stage 3 PENDING count matches. - E2E Tests had no pull_request trigger (dead entry); the workflows list is now exactly the six pull_request-triggered workflows, so the last finisher always re-fires the job. - Head/state re-check moved before the red/deferred verdicts so a cancel-in-progress firing on a stale SHA cannot stamp a red status over the new head's comment; the still-deferred branch now updates the status comment instead of staying invisible. - replace_region fails closed when the end-marker text only precedes the begin marker (awk END guard) — previously that shape truncated the comment body, eating the signature and reviewed-commit footer. - Region content is deterministic (no run URL) so the no-op cmp works; empty run list or unavailable gate skips approval; comment wording fixed (workflow_run jobs are attributed to the default branch, so the self-check exclusion is belt-and-braces, not load-bearing). Tests now execute the decision logic, not just grep for it: gate_counts and table_rows run against fixtures covering every conclusion class, non-PR events, re-run dedup, skipped filtering, ordering, and both marker-order failure shapes. 30/30 passing.
|
Thanks for the review — both blockers reproduced exactly as described before I touched anything, and they're humbling ones: the jq form at the heart of the "fail-closed" claim errored on every single run. All findings addressed in d2cb023. Blocker 1 (red gate never fires → red CI auto-approves) — reproduced ( Blocker 2 (table truncates away the evidence) — reproduced on #7680's data: 96 check-runs, 35 names, 68 skipped, Gate data source (also closes finding 4) — the approval gate no longer reads head-SHA check-runs at all; it reads Finding 3 (dead E2E trigger) — confirmed ( Finding 5 (stale red clobbers the new head's status) — the head/state/draft re-check now runs before any red/deferred verdict; a cancel-in-progress firing on a stale SHA takes the Finding 6 (end-before-begin truncates the comment) — reproduced, fixed with the Lows — all taken: empty run list / unreadable gate → skip approval; self-exclusion comments reworded to belt-and-braces (your default-branch-attribution evidence is right); region content is now deterministic (no 30/30 tests passing (17 in the finalize suite, including the fixture-driven decision-logic tests you asked for — both would have caught the blockers). 中文说明感谢审查——两个阻塞问题在动手前均按描述完整复现，且都很扎心："fail-closed" 声明的核心 jq 每次运行都在报错。全部发现已在 d2cb023 处理。 阻塞 1（红灯判定失效 → 红 CI 自动审批）：已复现（jq exit 5）。判定改为先绑定 conclusion 再做成员测试（ 阻塞 2（表格截断证据）：在 #7680 数据上复现（96 条 check、35 个名字、68 条 skipped，字母序前 60 行没有 门禁数据源（同时解决发现 4）：审批门禁不再读 head SHA 的 check-runs，改读 发现 3（E2E 死触发器）：确认无 发现 5（过期红灯覆盖新 head 的状态）：head/状态/草稿复核前移至任何红/延迟判定之前；cancel-in-progress 在过期 SHA 上的触发走 发现 6（end 先于 begin 时截断评论）：已复现，awk 加 低优先级：全部采纳——空 run 列表/门禁不可读则跳过审批；自身 check 排除的注释改为"保险起见"（你的默认分支归属证据是对的）；区域内容改为确定性（去掉 30/30 测试通过（finalize 套件 17 条，含你点名要的判定逻辑 fixture 测试——两条均可拦住原阻塞问题）。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
…eview's status comment The status comment is deliberately not SHA-scoped (the triage workflow creates it unscoped; scoping only the finalize side would orphan the pairing), so a finalize firing for an old SHA that loses the race against a newer head's green approval would overwrite the ✅ status with a stale warning. Guard the stale path: when the current head already carries bot sha= markers (a re-review owns the status comment), stay silent; when the head moved with no re-review yet — triage does not auto-rerun on synchronize — the stale note is accurate and still posts. Closed/draft PRs now just log instead of flipping the status.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
ReviewStrong direction, and the security posture is the best part: no Four things I'd fix before merge. 1. The fork-refactor approval guardrail now runs after the marker is postedIn That marker is now an approval with a CI precondition attached. So for a fork Cheapest robust fix is to re-assert the guardrail in the finalize job, where it costs zero extra API calls — IS_FORK="$(jq -r '(.head.repo.full_name // "") != .base.repo.full_name' <<<"$PR_STATE_JSON")"
PR_TITLE="$(jq -r '.title' <<<"$PR_STATE_JSON")"
if [ "$IS_FORK" != false ] && printf '%s' "$PR_TITLE" | grep -qiE '^[[:space:]]*refactor'; then
echo "PR #$PR: fork refactor — approval guardrail blocks the deferred approval."
update_status "$PR" guarded
continue
fi(A deleted fork makes 2. The finalized table renders bot-orchestration check-runs as CI evidence
On this PR's own head (
Five of eight rows are bot plumbing. The agent's hand-written table quoted the checks that matter; the first finalize pass replaces it with one where Both are fixed by filtering the table to the suites the gate already trusts — table_rows() { # $1 checks-file $2 runs-file
jq -r --arg self "$SELF_CHECK_NAME" --slurpfile runs "$2" '
([$runs[0][] | select(.event == "pull_request") | .check_suite_id]) as $ok
| map(select(.name != $self))
| map(select(.check_suite.id as $s | $ok | index($s)))
| …' "$1"
}On 3. The status comment can get stuck at "⏳ Approval still deferred" after the approval has landed
Reachable whenever any firing observes 4. An empty
|
| 检查 | 属于该 PR 的 CI suite？ |
|---|---|
Classify PR |
✅ |
Test (ubuntu-latest, Node 22.x) |
✅ |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ |
Remind on force-push |
❌ |
label |
❌ |
precheck-pr / precheck |
❌ |
remove-suspicious-attachments |
❌ |
route |
❌ |
八行里五行是 bot 管道。Agent 手写的表格引用的是真正要紧的检查；第一次 finalize 会把它替换成一张把 Remind on force-push ✅ 当作测试证据呈现的表。这也让标题文案自相矛盾：PENDING 来自 workflow run，因此一个仍在运行的编排 job 会在"Final CI results … after CI completed"下面渲染成 ⏳ running。
两个问题都可以通过把表格过滤到门禁已经信任的 suite 来解决——/tmp/runs.json 就在磁盘上，且带有 check_suite_id：
table_rows() { # $1 checks-file $2 runs-file
jq -r --arg self "$SELF_CHECK_NAME" --slurpfile runs "$2" '
([$runs[0][] | select(.event == "pull_request") | .check_suite_id]) as $ok
| map(select(.name != $self))
| map(select(.check_suite.id as $s | $ok | index($s)))
| …' "$1"
}在 cadf3f0 上，这样恰好只剩那三行 CI。
3. 审批落地之后，状态评论可能永久卡在"⏳ 审批仍在延迟中"
PENDING > 0 会无条件调用 update_status "$PR" deferred——包括延迟审批已经发出之后。等一切转绿时，ALREADY = true 分支在 update_status … approved 之前就 continue 了，于是再也没有东西把它改回来。用打桩端到端复现：
>>> PR 上已有审批；某个受监听 workflow 正在重跑
PR #1: 1 PR CI workflow run(s) still in progress; approval stays deferred.
STATUS -> ⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for `aaaaaaa`
>>> 重跑绿色完成，finalize 再次触发
PR #1: already approved at aaaaaaa.
STATUS -> ⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for `aaaaaaa` ← 永久停留
只要审批之后任何一次触发观察到 PENDING > 0 就会命中——手动重跑某个 job、Qwen CI Failure Patrol、或者一次打标签事件启动 Qwen Autofix（它的 pull_request: [labeled, unlabeled] 触发器正好落在门禁集合里）。这是外观问题，但状态评论恰恰是本 PR 为了"不打开 Stage 2 也能看到结果"而新增的界面。一行即可：在 ALREADY 分支里调用 update_status "$PR" approved，或者当该 SHA 已有审批时跳过 deferred 更新。
4. check-runs 返回空数组时会清空表格，而不是保持原样
拉取失败有保护，返回空结果没有。jq -s 'add // []' 得到 []——或者该 SHA 上所有检查都是 skipped——会产生一张只有表头的表并覆盖掉 agent 的证据：
Final CI results for `aaaaaaa` (auto-updated by the triage finalize job after CI completed):
| Check | Result |
| ----- | ------ |
这个 workflow 在其他地方一律把"读不到"当作"什么都不做"（缺结束标记 → 不改动；计数不可解析 → 跳过审批）。这里也应同样处理：[ "$ROWS" -gt 0 ] || 跳过表格重写。相关地，replace_region 里 getline line < rfile 的返回值没有检查——/tmp/region.md 不可读或为空时会静默删除整个区域连同定界符，之后任何一次运行都无法修复。在循环前加 [ -s /tmp/region.md ] 可同时堵住这两条。
小问题
- 缺少
github.repository == 'QwenLM/qwen-code'守卫。.github/workflows/下有 25 个工作流带这个守卫，所有 bot 工作流都带。没有它的话，一个配置了CI_BOT_PAT的 fork 会对着 fork 自己跑完整的 finalize。今天无害，但这是仓库惯例。 - 表头漂移。 skill 模板写的是
| Check | Conclusion |，finalize 写的是| Check | Result |，第一次 finalize 后表头会悄悄变化。二者统一即可。 - PR 描述相对
d2cb023/cadf3f0已过期：描述说工作流在Qwen Code CI/E2E Tests上触发，但E2E Tests是被刻意排除的（这是对的——它没有pull_request触发器），而且有测试断言它不在列表里；测试方案里的数字写着"26 个测试通过（新增 13 个）"，实际现在是 30 个（新增 17 个）。
工作流清单本身没问题——Qwen Code CI、Qwen Autofix、SDK Java、SDK Python、Serve A/B、Web-shell Visuals 恰好就是仓库里全部六个带 pull_request 触发器的工作流，且 Qwen Code CI 没有路径过滤，因此长尾检查始终存在。并发控制确实是收敛的：GitHub 丢弃的是同组里 pending 的那个运行，从不丢弃正在运行的，所以最后一次触发总能读到最新状态。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
…with the gate Second review round, all four findings reproduced or confirmed before fixing: - The approve-on-green marker was emitted in Step 1 while the fork-refactor GUARD only ran in Step 2 — a marker that slipped out on a fork refactor would have been honored by the finalize job on green CI, bypassing the guardrail entirely. GUARD now computes in Step 1 and gates the marker's emission, and the finalize job re-asserts it structurally from the PR state it already fetched (null head.repo = deleted fork = blocked), with a 'guarded' status message instead of an approval. - table_rows now restricts check-runs to the suites of the same deduped event=pull_request workflow runs the gate trusts. Without it, 5 of 8 rendered rows on this PR's own head were bot plumbing presented as CI evidence; with it, 115 raw check-runs reduce to exactly the 3 CI rows. - A firing that saw PENDING>0 after the approval landed flipped the status comment back to 'deferred' with nothing to ever right it; the already-approved branch now repairs the status. - Zero surviving table rows (failed runs fetch, missing suite ids) skips the region rewrite instead of blanking the agent's table, and replace_region refuses an empty region file (an unchecked getline would have deleted the region and its markers unrecoverably). Nits: the house github.repository guard on the job, the table header matches the skill template, and the run-URL stays out of the region so the no-op cmp keeps working.
|
All four fixed in 75ee714, each verified the same way you found them. 1. Guardrail bypass — the worst of the round, and correctly diagnosed: the marker was an approval with a CI precondition, minted before the check that says "never approve this class of PR". Two-sided fix: 2. Table/gate universe mismatch — adopted your 3. Stuck "deferred" status — reproduced logic confirmed; the 4. Blank-table hazard — zero surviving rows now skips the region rewrite entirely (warning logged, agent's table left standing), and Nits: repository guard added to the job 中文四项均已在 75ee714 修复，并按你的方法逐一复验。 1. 护栏绕过——本轮最严重且诊断准确：标记本质是"附带 CI 前置条件的审批"，却在"这类 PR 永不审批"的检查之前发出。双侧修复： 2. 表格与门禁集合不一致——采纳 3. 状态卡在"延迟中"——逻辑复现确认； 4. 空表覆盖风险——存活行数为零时整体跳过区域重写（记录 warning、保留 agent 手写表格）， Nit：job |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
…n model injection (QwenLM#7703) * fix(triage): resolve stage comment ids by marker at patch time, harden model injection Two hardenings from shepherding QwenLM#7693, plus a wording fix: - Re-run comment updates now resolve the target comment id by its stage marker (bot-author-filtered, startswith match) immediately before each PATCH, instead of trusting remembered ids or list positions. Observed on a real re-run: the agent PATCHed the stage=3 comment with stage=1 content mid-run before self-correcting — with four bot comments in the thread, remembered-id bookkeeping is fragile. - The model-name injection step previously no-opped silently if the 'qwen3.7-max' literal ever left the skill (shipping the wrong signature in every comment), and corrupted the skill text on model names carrying sed metacharacters (/ & \). It now fails the job loudly when the target literal is missing and escapes the replacement. Covered by a behavioral test that runs the extracted step script against fixture files. - The finalize status text said 'stage comments above', but the status comment is created first, so the stage comments are below it — now 'in this thread'. * chore(triage): drop unrelated formatting churn from qwen-triage.yml The previous commit let prettier rewrite untouched lines (runs-on quoting, comment spacing) while formatting the edited step. Restore those lines to main's form; the diff now carries only the injection hardening and the status wording fix. * test(triage): pin the stage_comment_id recipe's load-bearing constraints Guards the startswith match and the bot-author filter in the skill's re-run comment-id recipe against silent regression — a contains match or a dropped author filter re-introduces the wrong-comment-overwrite bug this PR fixes. * test(triage): shim BSD sed only on darwin in the injection test The extracted step script uses GNU 'sed -i' (the step only runs on ubuntu runners), but this suite also runs in the macOS merge-queue job where BSD sed needs an extension argument after -i. Rewrite to sed -i '' on darwin only — on GNU sed a separated '' parses as the sed script, so the unconditional rewrite would break the Linux runs that mirror production.
…it association (QwenLM#7706) Live verification of the finalize loop (QwenLM#7693) on its first real fork PR caught the deferred approval being silently dropped: CI landed green, the approve-on-green marker was in place, but commits/:sha/pulls returned an empty list for the PR's current head — the association endpoint is not reliable for fork-branch commits (workflow_run.pull_requests is likewise empty for forks). The run logged 'No open PR; nothing to finalize' and exited, so the approval never posted. Resolve PRs by filtering the open-PR list on head.sha as the primary source — it cannot miss the PR a current-head firing belongs to — and keep the association endpoint as the second source (when it works it also surfaces PRs whose head moved past the SHA, which powers the stale note). Union both, deduped.
3 participants
What this PR does
Removes the triage agent's in-agent CI polling and moves the "wait for CI" half of the review to a new deterministic finalize workflow. Stage 2b now fetches check-runs exactly once, reports pending checks honestly, and wraps the CI table in
qwen-triage-ciregion markers keyed to the reviewed SHA. When Stage 3 reaches a clean approve verdict while checks are still running, it no longer approves immediately — the comment carries anapprove-on-greenmarker instead. A newqwen-triage-finalize.ymlworkflow fires onworkflow_runcompletion ofQwen Code CI/E2E Testsand, in plain bash over the API (no model, no checkout), rewrites the marked table region with the settled results and posts the commit-pinned approval only when every check on that SHA landed green — failing closed on red checks, a moved head, or a closed/draft PR, and flipping the triage status comment to say which way it resolved.Why it's needed
The Stage 2b poll loop (
sleep 60, up to ~10 minutes) can never catch this repo's unit suite, which takes ~30 minutes: the agent starts polling around minute 9 of a 32-minute check, waits its full budget, and always gives up with "CI still running at review time". Concretely on #7680: PR opened 14:42, triage burned 14:51→15:01 polling, Stage 3 approved at 15:02:10, andTest (ubuntu-latest)finished at 15:14:17 — the approval attested to a result that did not exist yet, and 10 minutes of ECS runner time plus per-poll model turns were spent to obtain nothing. With this change the stage comments land ~10 minutes sooner (roughly halving the triage job), and the deferred approval lands at CI completion — the theoretical minimum for a review that includes the unit-test signal — with full evidence instead of before it.Reviewer Test Plan
How to verify
npx vitest run --config ./scripts/tests/vitest.config.ts qwen-triage-finalize-workflow qwen-triage-workflow— 26 tests pass (13 new). The new suite asserts the security-relevant structure (no checkout, bot-author filter before honoring any marker, SHA-pinned approval, closed green set, self-check exclusion, per-SHA serialized concurrency) and executes the extractedhtml_escape/replace_regionbash helpers against crafted bodies (region replaced in place, prose preserved, missing end marker → fail closed, escape order&first)..qwen/skills/triage/references/pr-workflow.mdStage 2b/Stage 3: polling is now forbidden, the table template carries the region markers, and the approve path branches on thePENDINGcount computed from the existing single fetch (staleness in that direction can only defer, never approve early).gh api user), so a third party pastingapprove-on-greeninto a PR comment is ignored.commit_idand additionally re-checkshead.sha/state/draft at the last moment; any mismatch posts a "stale" status note instead of approving.Evidence (Before & After)
Before (#7680, run 30102148473): triage job 14:43→15:02 (~19.7 min, of which ~10 min was the poll loop), Stage 2 comment says "still running after ~10 minutes of polling", approval posted 15:02:10, unit suite finished 15:14:17.
After: the agent posts Stage 2/3 with the once-fetched table (pending rows marked) and no sleep loops;
finalize-triage-ciupdates the table and resolves the deferred approval when the last CI workflow completes. New-workflow behavior is covered by the structural/functional test suite above; it activates on merge (workflow_run listens on the default branch).Tested on
Environment (optional)
vitest via
scripts/tests/vitest.config.ts; YAML validated with the repo'syamlpackage and the embedded script withbash -n.Risk & Scope
workflow_runfiring after the marker is posted. The long pole (Qwen Code CI, ~30 min) always completes well after Stage 3 (~10 min), so the ordering holds in practice; if every listed workflow somehow finished before the marker landed, the approval simply stays deferred (visible in the status comment) until the next/triagere-run — fail-closed, never fail-open.workflow_runpath can only be exercised after merge (GitHub runs the default-branch version); the CI table region renders identically to the current hand-written table but row texts become mechanical (✅ successetc.) after the first finalize pass. Shortening the 30-minute unit suite itself (the real lower bound) is out of scope.Linked Issues
None — follows up on the triage-latency discussion around #7646 / #7648 (static unattended review) using #7680 as the measured example.
中文说明
本 PR 做了什么
移除 triage agent 的 CI 轮询，把"等待 CI"这一半职责移交给新的确定性 finalize workflow。Stage 2b 现在只拉取一次 check-runs，如实报告 pending 状态，并用绑定被审 SHA 的
qwen-triage-ci定界符包裹 CI 表格。Stage 3 得出干净的 approve 结论但检查仍在运行时，不再立即审批——评论中改为携带approve-on-green标记。新增的qwen-triage-finalize.yml在Qwen Code CI/E2E Tests完成时经workflow_run触发，以纯 bash 调 API（无模型、无 checkout）原地重写定界区内的表格，并且只有当该 SHA 上所有检查全绿时才提交 pin 到该 commit 的审批——红色检查、head 变更、PR 关闭/草稿一律 fail-closed，并同步更新 triage status 评论说明结果。为什么需要
Stage 2b 的轮询循环（
sleep 60，上限约 10 分钟）永远等不到本仓库约 30 分钟的单测套件：agent 在一个 32 分钟检查的第 9 分钟左右开始轮询，耗尽全部预算后必然以"CI still running at review time"放弃。以 #7680 为例：PR 14:42 打开，triage 在 14:51→15:01 空转轮询，Stage 3 于 15:02:10 审批，而Test (ubuntu-latest)15:14:17 才跑完——审批为一个尚不存在的结果背了书，10 分钟 ECS runner 时间和每轮轮询的模型调用一无所获。本改动使阶段评论提前约 10 分钟落地（triage job 时长约减半），延迟审批则在 CI 完成时（这是包含单测信号的审查的理论下界）带着完整证据落地。审查者验证方案
npx vitest run --config ./scripts/tests/vitest.config.ts qwen-triage-finalize-workflow qwen-triage-workflow—— 26 个测试通过（新增 13 个）。新测试断言安全关键结构（无 checkout、先验证评论作者是 bot 才认标记、审批 pin SHA、绿色集合封闭、排除自身 check、按 SHA 串行并发），并实际执行抽取出的html_escape/replace_regionbash 函数（区域原地替换、正文保留、缺失结束标记则 fail-closed、&最先转义）。.qwen/skills/triage/references/pr-workflow.mdStage 2b/Stage 3：禁止轮询，表格模板带定界符，approve 路径按既有单次拉取算出的PENDING分支（该方向的数据陈旧只会推迟审批，绝不会提前审批）。gh api user）的评论中查找标记，第三方粘贴approve-on-green无效。commit_idpin 定，且在最后时刻复核head.sha/状态/草稿；任何不一致都改为发布 "stale" 状态说明而非审批。证据（Before & After）
Before（#7680，run 30102148473）：triage job 14:43→15:02（约 19.7 分钟，其中约 10 分钟为轮询），Stage 2 评论写着 "still running after ~10 minutes of polling"，15:02:10 审批，单测 15:14:17 完成。
After：agent 以单次拉取的表格（pending 行如实标注）发布 Stage 2/3，无任何 sleep；
finalize-triage-ci在最后一个 CI workflow 完成时更新表格并落定延迟审批。新 workflow 行为由上述结构/功能测试覆盖；合并后生效（workflow_run监听默认分支版本）。风险与范围
workflow_run触发。长尾检查（约 30 分钟的Qwen Code CI）总是远晚于 Stage 3（约 10 分钟）完成，顺序在实践中成立；若所列 workflow 都在标记落地前完成，审批只会保持延迟（status 评论可见），等下次/triage重跑——只会 fail-closed，不会 fail-open。workflow_run实链路只能在合并后验证（GitHub 运行默认分支版本）；首次 finalize 后表格行文变为机械格式（✅ success等）。缩短 30 分钟单测本身（真正的下界）不在本 PR 范围。关联 Issue
无——是 #7646 / #7648（无人值守静态审查）之后关于 triage 延迟的后续，以 #7680 为实测样本。