fix(ci): gate the fork signal on fields the review payload delivers - #9469
Conversation
`qwen-autofix-fork-signal.yml` gated on `github.event.pull_request.maintainer_can_modify == true`. That field does not exist in a `pull_request_review` payload: the event carries the SIMPLE pull-request object, and `maintainer_can_modify` — like `mergeable`, `additions`, and `changed_files` — ships only on the full object the `pull_request` event sends. The expression evaluated to null on every delivery, `null == true` is false, and the job's `if` could never hold. Measured on the repository: across the 300 runs between the bridge shipping (QwenLM#8676, 2026-08-07) and this change, 290 skipped, 7 cancelled, 1 action_required, and 0 success. Not one signal ever reached its step, so the bridge behind it has never fired either — every fork-PR review has been served by the scheduled scan alone, which is exactly the throttled backstop this bridge exists to get ahead of. The consent check is not lost, and does not move: the bridge already re-reads it live (`gh pr view --json maintainerCanModify`, then `select(… .maintainerCanModify == true)`), and that read was always the authoritative one — consent can be withdrawn between the review and the dispatch, so a payload copy could only ever have been a stale early-out. The signal job cannot make that call itself: it holds `permissions: {}`, no secrets and no checkout, deliberately, because it runs on a fork-triggered event. What the removal does cost is one signal + bridge run and one PR read for a takeover-labeled fork PR whose author has turned maintainer edits off, where the gate previously intended to spend nothing. Ordinary contributor fork PRs are unaffected — the bot-authored-or-takeover-labeled conjunct still excludes them. Also adds a regression test asserting the gate references no full-object-only field. A gate like this fails silently: the job's entire body is one echo, so "never opens" and "no fork review happened to qualify" look identical from outside, which is why this went twelve days unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Re-run on the new head (
Moving on to code review. 🔍 中文说明应维护者请求,在新 head(
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy baseline is unchanged from the first pass: a gate conjunct that can never be true on this event has exactly two sensible fixes — fetch the field properly, or drop the conjunct. Fetching is off the table because the signal job is deliberately powerless ( What the new commit (
No blockers, no convention violations. Test evidence (this PR's own CI, read via API — unattended run)This is an unattended CI run, so I did not build or execute any PR code; the evidence below is the PR's own CI on
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified, explicitly:
中文说明代码审查我的基准判断与首轮一致:一个在此事件上永远不可能为真的合取项只有两种合理修法——正确读取字段,或删掉它。读取在这里不可行,因为 signal job 刻意不持凭证( 新提交(
无阻塞项,无规范违规。 测试证据(该 PR 自己的 CI,经 API 读取——无人值守运行)这是无人值守的 CI 运行,因此我未构建或执行任何 PR 代码;以下证据是 未验证项,明确列出:
— Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the one prior nit was addressed exactly, and I independently reproduced the dead gate twice over. Approval is pinned and deferred only because one CI leg is still in flight on the reviewed commit. Stepping back: this is what a good infrastructure fix looks like, and the second round made it sharper rather than broader. The problem was observed, not hypothesized — on this re-run I pulled the full run history (500 runs, zero The delta since the last review is exactly one test-only commit, and it does exactly what was asked: the full-object field guard now matches on a word boundary instead of a substring, with a two-directional self-test proving the matcher rejects What I'd want to know six months from now is all in the tree: the long job comment records why the conjunct can never live here (payload shape + Not approving in this run for one procedural reason: the 中文说明回顾全局:这是一次优秀的基础设施修复该有的样子,而且第二轮让它更锋利、而不是更膨胀。问题是观测到的,不是假设的——这次重跑我拉取了完整运行历史(500 次运行,零次 上次评审以来的差异恰好是一个纯测试提交,且它做的恰好是被要求的事:完整对象字段守卫现在按词边界匹配而非子串,并配有一个双向自测,证明匹配器拒绝 六个月后维护它时我需要知道的东西都已在树里:那段长 job 注释记录了该合取项为何永远不能存在于这里(负载形状 + 本次不批准,只出于一个程序性原因: — Qwen Code · qwen3.8-max Reviewed at |
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. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.14)
The guard asserted the signal gate references no full-object-only field via bare substring containment, but four deny-list entries are strict prefixes of fields the simple pull-request object DOES deliver: `merged` in `merged_at`, `commits` in `commits_url`, `comments` in `comments_url`, `review_comments` in `review_comments_url`. A future edit adding a legal conjunct such as `github.event.pull_request.merged_at == null` would turn the suite red with a message blaming a full-object-only field — pushing the author to drop the conjunct or weaken the guard itself. Anchor each check on a word boundary, and add a test that pins the matcher's discrimination on all four prefix pairs so the substring form cannot come back unnoticed.
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定:
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.14)
The full-object-only guard matched `pull_request.<field>` literally, so it only saw the `.` property de-reference. GitHub Actions reaches the same property through the documented `[ ]` index operator, on any segment of the path, and `github.event.pull_request['maintainer_can_modify'] == true` evaluates exactly as the dot form did: the field is absent from the simple `pull_request_review` payload, `null == true` is false, and the gate never opens. The guard stayed green through it — a silent replay of the incident this PR fixes, invisible to the test written to catch it. Rewrite the index form to the dot form before matching rather than enumerating spellings, so one matcher covers every combination of the two at any depth. The word-boundary anchor from R1-1 is unchanged, so the four delivered fields the deny-list names prefix (`merged_at`, `commits_url`, `comments_url`, `review_comments_url`) still pass in every spelling. A `fromJSON(toJSON(github.event.pull_request))` round-trip still evades this; no textual guard catches that one, and the comment says so. Mutation-verified, each mutant reddening the tests that pin it: | mutant | result | |---|---| | normalization removed (identity) | 2 failed — bracket and mixed spellings go unseen | | word boundary dropped | 1 failed — `merged_at` rejected as `merged` | `npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-fork-bridge-workflow.test.js` -> 12 passed (12). eslint and prettier clean.
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
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-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
scripts/tests/qwen-autofix-fork-bridge-workflow.test.js:295 — [probe] The field guard is a 12-name blocklist; an allowlist of delivered fields would fail closed on typos and unlisted fields
中文说明
已审查。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
R3-1: `asDotAccess` rewrites `pull_request['field']` to the dot form before
matching, and its regex deliberately tolerates whitespace inside the index
(`\[\s*…\s*\]`) because GitHub Actions accepts
`github.event.pull_request[ 'maintainer_can_modify' ]` as a legal expression.
No spelling in `referenceSpellings` carried that whitespace, so the tolerance
was unpinned: deleting both `\s*` left all 12 tests green, and a later gate
edit written in the spaced form would have reached the same absent field and
restored the always-false gate this PR fixes.
Add the spaced-bracket spelling, which both index-operator tests consume.
Mutation-verified: with the two `\s*` deleted from `asDotAccess`, this file
now fails 2 tests ("rejects a full-object field without rejecting the fields
it prefixes" and "sees a full-object field through the index operator");
before this commit the same mutation left 12 passed.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
scripts/tests/qwen-autofix-fork-bridge-workflow.test.js:105 — [probe] Guard misses the spaced-dot spelling pull_request . field , which the Actions lexer acceptsscripts/tests/qwen-autofix-fork-bridge-workflow.test.js:295 — [probe] Guard misses the camelCase alias maintainerCanModify , the spelling dominant in the surrounding code
中文说明
无阻断问题。LGTM!✅
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-autofix-fork-signal.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
Released in v0.22.2. |
What this PR does
Removes one conjunct from the
Qwen Autofix Fork Signaljob'sif, and adds a regression test that stops it — or anything like it — from coming back.The conjunct was
github.event.pull_request.maintainer_can_modify == true. That field is not present in apull_request_reviewpayload. The event carries the simple pull-request object;maintainer_can_modify— along withmergeable,mergeable_state,additions,deletions,changed_files, and the rest of that family — ships only on the full object thepull_requestevent sends. The expression therefore evaluated tonullon every delivery,null == trueisfalse, and the job's gate could never hold.Why it's needed
The gate has never once opened. Across the 300 runs of this workflow between the bridge shipping (#8676, 2026-08-07) and today:
skippedcancelledaction_requiredsuccessBecause the signal never completes
success, theworkflow_run-triggered half (qwen-autofix-fork-bridge.yml) has never fired either. The entire bridge has been inert for twelve days. Every fork-PR review in that window was served by the scheduled*/10scan alone — precisely the throttled backstop this bridge exists to get ahead of, and the one the design notes call out as insufficient for real-time pickup.This is not a theoretical cost. It surfaced because a scheduled-scan stall left takeover-managed fork PRs sitting on unaddressed Critical findings for hours with nothing to pick them up: the direct
pull_request_reviewlane declines fork events by design (no secrets —routelogsfork review noted … the next scheduled scan engages), the bridge that exists to cover that decline was inert, and the cron backstop was the only remaining path. Three paths, and the two that were supposed to provide redundancy were the same path.Why removing it is not a weakened check
The consent check does not move and is not lost — the bridge already re-reads it live, and always has:
That live read was always the authoritative one. Consent can be withdrawn between the review and the dispatch, so a value copied out of the review payload could only ever have been a stale early-out, never the decision. The bridge's own comment already frames these conjuncts as "cheap early-outs, not a security boundary: review-scan re-derives all of it", and the signal's own comment says its gate "decides WHEN, never whether".
scripts/tests/qwen-autofix-fork-bridge-workflow.test.jsalready pins both the live read (expect(bridgeScript).toContain('maintainerCanModify == true')) and a behavioral case for edits turned off since the review.The signal job also cannot make this call itself. It holds
permissions: {}, no secrets and no checkout, deliberately — it runs on a fork-triggered event, so it is built to hold nothing worth stealing. It has no way to ask the API for a field the payload does not carry. Removal is the only correct direction; there is no "read it properly here" option.Why a regression test, not just the deletion
This class of bug fails silently and is invisible from outside. The job's entire body is one
echo; when the gate holds, nothing observable happens beyond a run turning green. So "the gate never opens" and "no fork review happened to qualify this week" look identical in the Actions tab. That is how it survived twelve days and 300 runs. The verbatim gate pin already in this test file did not help — it pins the gate against drift, and this gate was wrong the day it was written.The new test asserts the gate references no full-object-only field, so a future conjunct reaching for
mergeableorchanged_filesfails here rather than silently closing the gate forever.Reviewer Test Plan
How to verify
1. Confirm the field is genuinely absent from the payload. From GitHub's published webhook types:
SimplePullRequestcarries exactly these fields, andmaintainer_can_modifyis not among them:The full
PullRequestinterface — what thepull_requestevent sends — does carry it, alongsidemergeableandadditions.2. Confirm the observed effect.
Qwen Autofix Fork Signal, all runs to date: 290skipped, 7cancelled, 1action_required, 0success. Includes deliveries where every other conjunct is verifiably true — e.g. run titledfork-signal: PR 9384 reviewed by qwen-code-ci-botat2026-08-19T01:47:08Z, on a PR that is cross-repo, targetsmain, is open, carriesautofix/takeover, does not carryautofix/skip, hasmaintainer_can_modify: trueon the REST object, and was reviewed byREVIEW_BOT. Skipped.3. Run the tests.
4. Mutation-verify the new test. Re-add the conjunct to the workflow and re-run:
2 failed | 8 passed— the verbatim gate pin and the new payload-field test both catch it. Restore, and it is10 passedagain.scripts/tests/qwen-autofix-workflow.test.jshas one failure,behaviorally replays the stale-duplicate revalidation, including the conflict-only transition, which is a 5sTest timed out— pre-existing and unrelated: it reproduces identically on a pristineupstream/mainwith these changes stashed (1 failed | 179 passed), and this PR does not touchqwen-autofix.yml.Evidence (Before & After)
N/A — CI plumbing, no user-visible surface.
Tested on
Environment (optional)
Unit tests only (
npx vitest run scripts/tests/…). The behavioral proof is the workflow-run history cited above; the gate itself can only be exercised by a real fork-PR review, which lands the first time this merges.Risk & Scope
gh pr viewbefore the bridge rejects it, where the gate previously intended to spend nothing. That is the whole cost, it is bounded at one review, and it buys back a path that has never worked. Ordinary contributor fork PRs are unaffected — the bot-authored-or-takeover-labeled conjunct still excludes them, which is where the run-burn concern actually bites.Linked Issues
None.
中文说明
这个 PR 做了什么
从
Qwen Autofix Fork Signal这个 job 的if条件中删掉一个合取项,并新增一个回归测试,防止它——或任何同类问题——再次出现。被删掉的是
github.event.pull_request.maintainer_can_modify == true。这个字段在pull_request_review的事件负载中并不存在:该事件携带的是简化版的 pull-request 对象;maintainer_can_modify——连同mergeable、mergeable_state、additions、deletions、changed_files等同一族字段——只出现在pull_request事件发送的完整对象上。因此该表达式在每一次投递中都求值为null,而null == true为false,这个 job 的门禁永远无法通过。为什么需要
这个门禁从来没有打开过一次。从该 bridge 上线(#8676,2026-08-07)到今天,这个 workflow 的全部 300 次运行中:
skippedcancelledaction_requiredsuccess由于 signal 从未以
success结束,其后由workflow_run触发的另一半(qwen-autofix-fork-bridge.yml)也从未启动过。整条 bridge 已经失效十二天。这段时间内所有 fork PR 的评审,都只由每 10 分钟一次的定时扫描承担——而那正是这条 bridge 为了抢在其前面而存在的、受限流的兜底路径,也正是设计说明中指出的、不足以支撑实时接管的那一条。这不是纸面上的代价。问题之所以浮现,是因为一次定时扫描停摆,导致处于 takeover 管理下的 fork PR 带着未处理的 Critical 发现搁置数小时无人接手:直连的
pull_request_review通道按设计拒绝 fork 事件(无密钥——route的日志写着fork review noted … the next scheduled scan engages),本该覆盖这一拒绝的 bridge 处于失效状态,于是定时兜底成了唯一剩下的路径。名义上三条路,而本应互为冗余的两条其实是同一条。为什么删除它并不等于放松检查
这项同意检查既没有移位,也没有丢失——bridge 一直在用实时数据重新读取它:
这次实时读取始终是权威判据。同意可能在评审与派发之间被撤回,因此从评审事件负载里抄来的值,充其量只能是一个陈旧的提前退出条件,从来都不是决策本身。bridge 自己的注释已经将这些合取项定性为「cheap early-outs, not a security boundary: review-scan re-derives all of it」,而 signal 自己的注释也写明其门禁「decides WHEN, never whether」。
scripts/tests/qwen-autofix-fork-bridge-workflow.test.js中已经同时钉住了这次实时读取(expect(bridgeScript).toContain('maintainerCanModify == true'))以及「评审之后关闭了 maintainer edits」这一行为用例。signal 这个 job 也没有能力自行做出该判断。它持有
permissions: {},无密钥、无 checkout,这是刻意为之——它运行在 fork 触发的事件上,因此被设计成不持有任何值得窃取的东西。它无从向 API 索取一个事件负载并未携带的字段。删除是唯一正确的方向;不存在「在这里正确地读取它」这个选项。为什么要加回归测试,而不只是删掉
这一类缺陷是静默失败的,从外部完全不可见。该 job 的全部主体只有一条
echo;当门禁通过时,除了一次运行变绿之外不会发生任何可观测的事情。因此「门禁永远不开」与「这周恰好没有符合条件的 fork 评审」在 Actions 页面上看起来一模一样。它正是这样存活了十二天、300 次运行。该测试文件中已有的逐字门禁钉死并没有帮上忙——它钉的是门禁的漂移,而这个门禁在写下的那天就是错的。新增的测试断言该门禁不引用任何「仅存在于完整对象」的字段,这样将来若有合取项去取
mergeable或changed_files,会在此处失败,而不是悄无声息地把门永久关死。评审者测试计划
如何验证
1. 确认该字段在事件负载中确实不存在。 取自 GitHub 公布的 webhook 类型定义:
SimplePullRequest携带的字段恰好是以下这些,其中并无maintainer_can_modify:而完整的
PullRequest接口——即pull_request事件所发送的那个——确实携带该字段,同时也携带mergeable与additions。2. 确认实际观测到的效果。
Qwen Autofix Fork Signal至今的全部运行:290 次skipped、7 次cancelled、1 次action_required、0 次success。其中包括其余每一个合取项都可验证为真的投递——例如2026-08-19T01:47:08Z那次标题为fork-signal: PR 9384 reviewed by qwen-code-ci-bot的运行,对应的 PR 跨仓库、目标为main、处于 open、带有autofix/takeover、不带autofix/skip、REST 对象上maintainer_can_modify: true、评审者为REVIEW_BOT。结果仍是 skipped。3. 运行测试。
4. 对新测试做变异验证。 把该合取项加回 workflow 后重跑:
2 failed | 8 passed——逐字门禁钉死与新增的负载字段测试都会捕获它。恢复后重新变为10 passed。scripts/tests/qwen-autofix-workflow.test.js中有一处失败,behaviorally replays the stale-duplicate revalidation, including the conflict-only transition,报的是 5 秒Test timed out——既有问题且与本 PR 无关:将本次改动 stash 后在干净的upstream/main上可原样复现(1 failed | 179 passed),且本 PR 并未改动qwen-autofix.yml。证据(改动前后)
N/A —— CI 管道改动,无用户可见界面。
测试环境
运行环境(可选)
仅单元测试(
npx vitest run scripts/tests/…)。行为层面的证据是上文引用的 workflow 运行历史;门禁本身只能由一次真实的 fork PR 评审来触发,即本 PR 合入后的第一次。风险与范围
autofix/takeover标签的 fork PR,其作者已关闭 maintainer edits,那么现在会先消耗一次 signal 运行、一次 bridge 运行和一次gh pr view,随后才被 bridge 拒绝——而原门禁的意图是在此情形下不消耗任何资源。这就是全部代价,其上界为每次评审一次,换回的是一条从未生效过的通路。普通贡献者的 fork PR 不受影响——「由 bot 创建或带 takeover 标签」这一合取项仍将其排除在外,而这才是「消耗运行次数」这一顾虑真正起作用的地方。关联 Issue
无。