feat(ci): stop auto-triggering reviews on a loop that is not settling - #9711
feat(ci): stop auto-triggering reviews on a loop that is not settling#9711wenshao wants to merge 3 commits into
Conversation
The `/review` pipeline measures whether a pull request's review loop is converging and publishes that measurement; it owns no threshold and stops nothing. This is the caller's half of issue #9278's rule — the tool measures, the caller decides — and it decides exactly one thing: whether the review that fires on every push keeps firing. The gate lives in `delay-automatic-review`, which is reached only by `opened` and `synchronize`, so `@qwen-code /review` and a requested review go around it entirely. Stopping the treadmill is not refusing to review, and nothing already posted is withheld. The rule reads each posted marker's `round`, `fresh` and `floor` over a window of consecutive rounds, and stops only when the first-time-finding count fails to fall across the whole window. Every other shape keeps reviewing: too few readable rounds, a round with no fresh count, a gap in the round numbers, a settled newest round, or two different posting floors inside the window. Each of those mirrors a reading the pipeline itself refuses to call divergence — the only thing this repository adds is the window, as `REVIEW_AUTO_STOP_WINDOW` (default 3), with `REVIEW_AUTO_STOP_DISABLED` to turn it off. It fails OPEN everywhere. A listing that errors, telemetry that will not parse, a missing decision module, a missing Node — all keep reviewing. A caller that silences reviews when it cannot read its own evidence is worse than one with no rule at all. All four of those paths are exercised against the extracted step, not argued. The evidence is author-scoped: reading markers from any author would let a stranger's forged marker silence this repository's automatic reviews. The job checks out the base branch with credentials dropped — it runs on `pull_request_target`, and it reads none of the PR's tree. Why the window and not a round count: the obvious "stop after round N" is the rule the measured data rejects. The two pull requests that ran this feature's own loop to completion each took nine rounds, and #9461's rounds 6 and 7 still produced 5 and 2 Critical findings. Eleven unit tests over the decision, each isolating the clause it names from the shape that DOES stop; seven replays of the extracted workflow step covering both outcomes, the two repository variables, and the four fail-open paths.
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical — this repository's own automation has run review loops to nine rounds twice recently (#9461, #9623, the same line of work), #9461 still produced 5 and 2 Critical findings in rounds 6–7 with nothing in the pipeline allowed to intervene, and #9461 alone accumulated 143 reviews. The measurement half (#9623) is already on main; the decision half is what's missing. Direction: aligned — this is the caller's half of issue #9278's "the tool measures, the caller decides" rule, the threshold lives in repository variables where it belongs, and it continues an existing series rather than opening a new front. No CHANGELOG reference expected for CI infrastructure. Size: not applicable — no Approach: scope feels right. Pure decision function + one gate step + one upserted notice, fail-open on every unreadable state, and explicit requests bypass the gate by construction since it lives in a job only Risk: no elevated risk signals — no high-risk paths matched. The one area Stage 2 will look at closely is the Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,而非理论性的——本仓库自己的自动化最近两次把评审回路跑到九轮(#9461、#9623,同一条工作线),#9461 在第 6、7 轮仍分别产出 5 条和 2 条 Critical,而流水线中没有任何环节被允许介入;仅 #9461 就累积了 143 条 review。测量的一半(#9623)已在 main 上,缺的是决策的一半。 方向:对齐——这是 issue #9278 "工具测量、调用方决策"规则中属于调用方的一半,阈值放在它该在的仓库变量里,是既有系列的延续而非另起炉灶。CI 基础设施不期望有 CHANGELOG 参照。 规模:不适用——未触及 方案:范围合理。纯函数决策 + 一个门禁 step + 一条幂等更新的通知,所有读不出证据的状态都 fail-open;由于判据位于只有 风险:无升级风险信号——未命中高风险路径。Stage 2 会重点看的是 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe design holds up under scrutiny: author-scoped marker listing (a stranger's forged marker cannot feed the decision), integer-strict parsing (no marker content can ever reach the posted notice), fail-open on every unreadable state, and a gate unreachable from any explicit request. The decision module is a clean pure function, and the one-change-at-a-time test shapes are exactly how these arms should be proven. Two problems though — both verified against live data rather than inferred, and both sit on the feature's own happy path. 1. The review listing is broken for multi-page results — the rule can never engage at the scale it exists for. 2. The pause notice can never post — every stop will be silent on the PR. The upsert POSTs an issue comment with the step's Not blocking, noted in passing: the TestingUnattended CI run — no PR code was executed in this review. Evidence carried: the PR's own CI fetched via API (table below), plus two API-only replays I ran against live data (the pagination reproduction in finding 1, and a static read of the job's permission/credential wiring in finding 2).
CI at review time (the finalize workflow updates this table once CI settles): Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 No failing checks at review time. One operational note: 中文说明代码审查设计经得起推敲:按作者过滤的标记列举(陌生人的伪造标记无法进入决策)、整数级严格解析(标记内容不可能到达发布的评论)、所有读不出证据的状态都 fail-open、任何显式请求都触达不到判据。决策模块是干净的纯函数,测试"每次只改一处"的形态也正是这类分支该有的证法。但有两个问题——都是对着线上数据验证出来的,不是推断——而且都落在功能自身的正常路径上。 1. review 列举在多页结果下是坏的——规则在它为之存在的规模上永远无法生效。 2. 暂停通知永远发不出去——每次停下在 PR 上都是静默的。 upsert 用 step 的 不阻塞,顺带一提: 测试无人值守的 CI 运行——本次审查没有执行任何 PR 代码。承载的证据:经 API 获取的 PR 自身 CI(下表),以及我对线上数据做的两次纯 API 重放(问题 1 的分页复现,与问题 2 对 job 权限/凭据接线的静态读取)。
评审时的 CI 状态(CI 落定后 finalize workflow 会更新此表):表格见上,无失败项; — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — strong design and unusually good fail-open discipline, but both runtime paths the feature depends on are demonstrated broken: the rule cannot engage at flagship scale, and the pause notice cannot post at any scale. Stepping back: the independent baseline I'd write for this problem is almost exactly what this PR is — a pure decision function over bot-authored ledger markers, a gate only What I can't get past is that both defects are demonstrated, not speculated, and both sit on the feature's happy path:
Both fixes are small ( — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two demonstrated blockers on the feature's own path — details in my stage-2 comment above: (1) gh api --paginate --jq emits one array per page, so the decision reads zero rounds on any PR with 100+ reviews (verified live against #9461's 143 reviews); (2) the pause notice posts with a read-only GITHUB_TOKEN → 403 → every stop is silent on the PR. Both fixes are small; the rest reads ready. 🙏
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. |
Both defects were fail-open and therefore silent, and both sat on the feature's own happy path. The review listing used `gh api --paginate --jq`, which applies the filter per page and concatenates the outputs: a PR past 100 reviews emits two JSON documents, `JSON.parse` rejects them, and the fallback reads "no rounds carry a marker" — so the rule kept reviewing. Verified against #9461 (143 reviews): two arrays, 3 + 6 bodies. The gate was inert on exactly the long diverging loops it exists for, and worked only where the treadmill is still bearable. Now `--paginate` alone, slurped with `jq -s`, this repository's convention everywhere else. The pause notice was posted with the job's GITHUB_TOKEN, but the notice is an issue comment, the job holds no `issues: write`, and upsert-bot-comment.sh opens by resolving its author scope through `gh api user` — which a GITHUB_TOKEN cannot call at all. Every stop would have been silent on the PR, the one failure mode the notice exists to prevent. The write now carries CI_BOT_PAT, scoped to that one call; the two reads stay on the job token, and an absent PAT posts nothing rather than burning three retry rounds. The unit tests could not see either defect, because both live in the wiring around the decision. The test file now also replays the shipped `run:` block verbatim against a stubbed `gh` that paginates the way the real one does — including applying `--jq` per page, so the multi-page arm fails while the single-page arm passes when the old listing is restored. Mutation-verified, one named test each: restoring `--paginate --jq` reddens the multi-page arm only; dropping the PAT override reddens the credential arm only; dropping the absent-PAT guard reddens the fork arm only.
|
Both confirmed, both fixed in 1. The multi-page listing. Reproduced with your exact command against #9461 before changing anything: 2 documents, 2. The notice's credential. Confirmed, and the break is one step earlier than described: The fixture you asked for. The test file now replays the shipped Mutation-verified, one named test each:
17/17 in Both traps are now written down in On the non-blocking note: agreed, the second 中文说明两个问题都已确认并在 1. 多页列举。 动手改之前先用你给的原样命令对 #9461 复现:2 个文档、 2. 通知的凭据。 确认,且断点比描述的还早一步: 你要的夹具。 测试文件现在会把发布版的 变异验证,每条对应一个具名测试(表格见上)。 两个坑都已写进 不阻塞的那条:同意,第二处 |
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review
该 PR 实现了 #9278 治理规则中调用方的那一半——让本仓库在 PR 评审回路未收敛时停止自动触发评审。
无阻塞性问题。 代码结构清晰、fail-open 纪律完善、测试覆盖充分。先前一轮审查发现的两个运行时缺陷(多页列举、通知凭据)已在 abafd77 中修复,并通过 workflow 回放测试验证。
主要确认点
- 设计正确: 为纯函数,所有判据分支均 fail-open
- 授权范围正确:仅读取 账号的标记
- 安全模型正确:Checkout 基分支、丢弃凭据,PR 控制字节仅触及 JSON.parse 和整数检查
- 通知凭据正确:读取用 GITHUB_TOKEN,通知发布用 CI_BOT_PAT,二者分离
- 测试充分:11 个单元测试 + 4 个 workflow 回放测试,覆盖所有判据分支和运行时陷阱
- 文档清晰:设计文档记录了运行时陷阱和配置变量
--paginate陷阱已修复:使用不带 --jq 的 --paginate + jq -s
建议(非阻塞)
(1) 决策模块中的 qwen-code-ci-bot 账号名硬编码为字符串,而 workflow 注释提示要与 review-config 中的 bot_login 常量保持同步。建议将账号名提取为共享常量,避免后续修改时两处不同步。
Reviewed by Qwen Code (high effort).
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed.
Not explored to full depth (tool budget reached): You are review agent 6b — Agent 6b: Undirected audit — 3 AM oncall mindset.: None. All checks completed within the budget..
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
Test Plan (not a blocker): .github/scripts/check-workflow-size.sh — no such file or directory.
中文说明
已审查。
未探索到全部深度(达到工具调用预算):You are review agent 6b — Agent 6b: Undirected audit — 3 AM oncall mindset.:None. All checks completed within the budget.。
未审查:反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序,即便运行过,也缺失了 brief 承载的方法。
Test Plan(非阻断):.github/scripts/check-workflow-size.sh — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…fting it Two defects on the notice, and the wiring that let both hide. The notice body never carried `<!-- qwen-review-auto-stop -->`, the marker upsert-bot-comment.sh looks a prior comment up by. The lookup therefore never matched, so every stop POSTed a new comment — and a paused PR re-decides the same stop on every push, so the duplicates were unbounded, on exactly the long-diverging loops this rule targets. The marker is now one shell variable used for both the body and the lookup key. The notice also promised a resume that cannot happen: while the pause holds, `review-pr` never runs for opened/synchronize, so no round posts, so the evidence is frozen and every later push re-decides the identical stop. An author who batches fixes and pushes once waits forever — the same silent-stop confusion the notice exists to prevent. It now says what actually moves it: an explicit review. When the pause does lift, the same comment is superseded in place through the `--update-only` upsert, so a recovered PR stops advertising a pause that is over; the supersede is skipped when no round is readable, because such a PR has never been paused. Two silences closed with them. A listing that could not be READ was byte-identical to a PR with no reviews yet — it now warns with the captured stderr and still fails open. And `evidence.rounds` rides both answers now, so the gate has a real reason to read it and a test that pins it. Tests: the replay stub keeps a comment store across runs (an upsert is invisible without one), gained a listing-failure switch, derives the bot login from the review-config constant rather than retyping it, and cleans up its temp dirs like the sibling suite. A new vitest contract test feeds real `serializeLedger` output — including a volume-shed marker — through the real gate, the only coupling between the format this pipeline writes and its second, hand-copied reader. Mutation-verified, nine one-change mutations, each reddening the arm that names it: the marker dropped from the body, the supersede removed, its readable guard removed, `2>/dev/null` restored, the author filter deleted, a bot rename that misses the jq literal, `window > 0` relaxed to `>= 0`, `evidence.rounds` emptied, and the reader drifting off the producer's version pin.
|
Round 2: all 11 addressed in The two Criticals were both about the notice, and both were real: its body never carried the marker its own upsert looks a prior comment up by, so every stop minted a duplicate — unbounded, because a paused PR re-decides the same stop on every push; and it promised a resume by pushing, which the gate makes impossible, since a paused PR posts no round and its evidence is therefore frozen. The notice now says what actually moves it, and the pause supersedes itself in place when it lifts. What made all three rounds of defects invisible is the same thing, and it is now the shape of the test file: the stub answered One new lane: Nine mutations, each reddening the arm that names it
24/24 in 中文说明第 2 轮 11 条全部在 两条 Critical 都关于那条通知,且都成立:正文从不携带其自身 upsert 用来查找旧评论的标记,于是每次停止都新发一条——而且无上限,因为被暂停的 PR 每次推送都重新裁决出同样的停止;通知还承诺"推送即可恢复",而门禁使之不可能,因为暂停中的 PR 不发轮次,证据被冻结。通知现在说明真正推动它的是什么,暂停解除时也会原地替换自己。 让三轮缺陷都隐形的是同一件事,现在它成了测试文件的形状:桩对任何评论列举都回 新增一条腿: 九条变异各自只让点名的用例变红(表格见上)。 |
|
Closing this: the direction is wrong, and no amount of fixing the wiring makes it right. Two lines this crossed. It changes the GitHub trigger flow, when the work belongs in For the record, the thing this PR was reaching for already ships. The posted review body carries the non-convergence reminder on every event a signal fires ( What does remain is inside Thanks to the reviewer for the three rounds here — the pagination defect, the credential that could not post, the missing upsert marker, and the temp-dir leak were all real and all found by running the thing rather than reading it. None of that work is lost; it just has no business existing. 中文说明关闭本 PR:方向本身是错的,把接线修得再对也不成立。 越过了两条线。其一,它改的是 GitHub 触发流程,而这项工作应当落在 需要记录的是:本 PR 想要的东西其实早已发布。发布的评审正文在任何信号触发时都会带上未收敛的提醒( 真正还留在 感谢评审这三轮——分页缺陷、发不出去的凭据、缺失的 upsert 标记、临时目录泄漏,条条属实,而且都是跑出来的而不是读出来的。那些工作没有白费,只是这个 PR 本就不该存在。 |
What this PR does
Makes this repository stop auto-triggering reviews on a pull request whose own review history shows the loop is not settling. The
/reviewpipeline already measures that and publishes the measurement in every posted review's ledger marker; it owns no threshold and stops nothing. This is the caller's half of the rule that line of work is built on — the tool measures, the caller decides — and it decides exactly one thing: whether the review that fires on every push keeps firing.The gate lives in
delay-automatic-review, a job reached only byopenedandsynchronize.@qwen-code /reviewand a requested review go around it entirely, so stopping the treadmill is never refusing to review, and nothing already posted is withheld. When it does stop, the PR gets one upserted comment naming the measurement, the evidence behind it, and both ways to resume — a silent stop would be indistinguishable from a pipeline that broke.The rule reads each posted marker's
round,freshandflooracross a window of consecutive rounds and stops only when the first-time-finding count fails to fall across the whole window. Every other shape keeps reviewing: too few readable rounds, a round that recorded no fresh count, a gap in the round numbers, a settled newest round, or two different posting floors inside the window. Each of those mirrors a reading the pipeline itself refuses to call divergence — the only thing this repository adds is the window, asREVIEW_AUTO_STOP_WINDOW(default 3), withREVIEW_AUTO_STOP_DISABLEDto turn the rule off.It fails open everywhere: a listing that errors, telemetry that will not parse, a missing decision module, a missing Node runtime — all keep reviewing.
Why it's needed
Every piece of this work so far tells a human why a loop is not converging. None of it ever stops one. The failure mode that started it is an agent processing review comments across a dozen-plus rounds, generating new findings while fixing old ones, until hundreds of comments accumulate without converging — and nothing in the pipeline is allowed to intervene, by design, because a tool that owns a stop threshold is a tool that has to defend that number on repositories it knows nothing about.
The number belongs to the repository. This is where it goes, and the previous PR's
recommendationsoutput plus the ledger'sfresh/floorfields are the machine-readable evidence that makes it possible to write without asking a model anything.The obvious rule — stop after round N — is the one the measured data rejects, which is why this uses the trend instead. The two pull requests that ran this feature's own review loop to completion (#9461, #9623) each took nine rounds, and #9461's rounds 6 and 7 still produced 5 and 2 Critical findings; a round-count bar would have cut those off.
Reviewer Test Plan
How to verify
node --test .github/scripts/review-auto-stop.test.mjs— 11 pass. The decision is a pure function, and every test starts from the shape that DOES stop and changes one thing, so each assertion measures the clause it names rather than passing for a sibling's reason.The workflow step was replayed, not argued. The
run:block is extracted from the YAML and executed against fixtures withghand the upsert script stubbed, covering seven arms: a non-converging history (should_review=false, notice rendered with the evidence interpolated), a converging one (true, reason names the falling step), the window widened by repository variable so the same evidence keeps reviewing, the rule disabled by variable, the reviews listing failing, the decision module missing, and Node absent fromPATH. The last four all produceshould_review=true— the fail-open direction.node scripts/lint.js --actionlintpasses, and.github/scripts/check-workflow-size.shstill passes with the added step.Evidence (Before & After)
N/A — no user-visible product change. The observable effect is a workflow job's own output plus, on a stopped PR, one bot comment; both are covered by the replays above.
Tested on
Environment (optional)
Node's test runner and a replayed shell step; no daemon, no sandbox, no network.
Risk & Scope
REVIEW_AUTO_STOP_DISABLEDcovers that without a code change.recommendationscodes the previous PR added ride the composed result and the artifact, not the PR, so a caller that wants to key onland-and-deferneeds them relayed to the PR first — deliberately left for a separate change rather than widened here.REVIEW_AUTO_STOP_DISABLED=truerestores exactly the previous behaviour.Linked Issues
Part of #9278. Follows #9623.
中文说明
这个 PR 做了什么
让本仓库在"某个 PR 自身的评审历史显示回路没有收敛"时,停止在每次推送上自动触发评审。
/review流水线已经在测量这件事,并把测量结果发布在每一份已发布评审的台账标记里;它不拥有任何阈值,也不停止任何东西。这是那条规则里属于调用方的一半——工具测量、调用方决策——而它只决定一件事:那个"每次推送都跑一轮"的评审是否继续跑。判据落在
delay-automatic-review这个 job 里,而它只会被opened与synchronize触达。@qwen-code /review和 review_requested 完全绕开它,所以停下传送带从来不等于拒绝评审,已经发布的内容也不会被扣留。真的停下时,PR 上会收到一条幂等更新的评论,写明测量结果、支撑它的证据,以及两种恢复方式——静默地停下与"流水线坏了"在外部无法区分。规则读取每份已发布标记的
round、fresh与floor,在连续若干轮的窗口上考察,只有当"首次提出"的条数在整个窗口内都没有下降时才停止。其余所有形态都继续评审:可读轮次不足、某一轮没有记录 fresh、轮次不连续、最新一轮已经收敛、窗口内出现两种不同的发布下限。每一条都对应流水线自己拒绝称之为发散的读法——本仓库唯一新增的只有窗口大小,即REVIEW_AUTO_STOP_WINDOW(默认 3),另有REVIEW_AUTO_STOP_DISABLED可整体关闭。它在所有分支上都 fail-open:列举失败、遥测解析不了、决策模块缺失、Node 不存在——一律继续评审。
为什么需要
到目前为止这条线上的每一块,都只是告诉人类回路为什么没有收敛,没有任何一块会停下它。最初的失败形态正是:agent 处理评审评论、跨十几轮边修边产生新问题,直到累积上百条评论仍不收敛——而按设计,流水线不被允许介入,因为一个拥有停止阈值的工具,必须在它一无所知的仓库上为那个数字辩护。
那个数字属于仓库。这里就是它该在的地方;上一个 PR 的
recommendations输出与台账的fresh/floor字段,正是让这条规则无需询问任何模型就能写出来的机器可读证据。最直觉的规则——"第 N 轮之后就停"——恰恰是实测数据否决的那个,所以这里用的是趋势。把本功能自身评审回路跑完的两个 PR(#9461、#9623)各用了 9 轮,而 #9461 的第 6、7 轮仍然产出了 5 条和 2 条 Critical;按轮次设卡会把它们砍掉。
复核测试计划
如何验证
node --test .github/scripts/review-auto-stop.test.mjs—— 11 个通过。决策是纯函数,每个用例都从"会停"的形态出发、只改一处,因此每条断言测的是它所指名的那个子句,而不是因为另一个兄弟条件恰好也成立而通过。workflow 的 step 是回放验证而非论证:从 YAML 中抽出
run:块,用打桩的gh与 upsert 脚本对夹具执行,覆盖七条腿——不收敛的历史(should_review=false,通知渲染且证据被正确插值)、收敛的历史(true,理由指出下降的那一步)、用仓库变量放宽窗口后同一证据继续评审、用变量整体关闭、评审列举失败、决策模块缺失、PATH中没有 Node。后四条全部得到should_review=true,即 fail-open 方向。node scripts/lint.js --actionlint通过;加入新 step 后.github/scripts/check-workflow-size.sh仍然通过。证据(前后对比)
N/A —— 无用户可见的产品变化。可观察的效果是一个 workflow job 自身的输出,以及在被停止的 PR 上的一条机器人评论;两者都由上述回放覆盖。
测试环境
运行环境(可选)
Node 自带测试运行器 + 回放的 shell step;不涉及 daemon、沙箱或网络。
风险与范围
REVIEW_AUTO_STOP_DISABLED无需改代码即可覆盖。recommendations码随合成结果与 artifact 传出,不在 PR 上,因此若调用方想按land-and-defer之类的码来决策,需要先把它们中继到 PR——刻意留作独立改动,而不是在此扩大范围。REVIEW_AUTO_STOP_DISABLED=true即完全恢复此前行为。关联 Issue
属于 #9278。承接 #9623。