fix(ci): surface blocked autofix takeover admission - #8410
Conversation
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR — re-run gate pass on the current head. Template looks good ✓ Problem: observed, not theoretical. Issue #8409 documents a concrete occurrence (PR #8320, autofix run 30778039590, 2026-08-03): a trusted Critical review routed into AutoFix, the forced scan silently rejected the takeover PR, and the stale "round 3 finished" status stayed visible. Maintainer real-stack verification on this PR independently reproduced the base behavior (silent reject, exit 0, stale status left in place). Direction: aligned — this makes the repo's own automation fail-closed and visible instead of silently green, which is exactly the direction the autofix workflow should move. The scope tracks the issue's acceptance criteria one-for-one. Size: not a core-module path. Production logic 241 lines (workflow YAML +223/−18); test lines 836 (+798/−38). No thresholds involved. Approach: focused — one workflow file and its test, no drive-by changes. Bounded retries with reason codes, a blocked-status writer that proves visibility or fails the run, and behavioral tests that extract the shell verbatim. The three genuinely debatable design points (shared per-PR lock trade-offs, PAT-identity mismatch failing the run, Risk: no elevated risk signals (no high-risk-path matches; the change is confined to the autofix workflow and its test). Moving on to code review. 🔍 中文说明感谢贡献 —— 针对当前 head 的 re-run 门检。 模板完整 ✓ 问题：已观测到，非理论性问题。Issue #8409 记录了具体发生场景（PR #8320、autofix 运行 30778039590，2026-08-03）：受信任的 Critical 评审路由进 AutoFix 后，强制扫描静默拒绝了该 takeover PR，旧的 "round 3 finished" 状态仍然可见。维护者在本 PR 上的真实环境验证也独立复现了 base 行为（静默拒绝、exit 0、旧状态残留）。 方向：对齐 —— 让仓库自身自动化从"静默绿色"变为 fail-closed 且可见，正是 autofix 工作流应有的方向。范围与 issue 验收标准逐条对应。 规模：非核心模块路径。生产逻辑 241 行（workflow YAML +223/−18）；测试 836 行（+798/−38）。不涉及任何阈值。 方案：聚焦 —— 只改一个 workflow 文件及其测试，无夹带改动。有界重试 + 原因码、要么证明可见要么让运行失败的 blocked 状态写入器、以及逐字节抽取 shell 的行为测试。三个真正有争议的设计点（共享 per-PR 锁的取舍、PAT 身份不匹配使运行变红、 风险：无升级风险信号（未命中高风险路径；改动仅限 autofix workflow 及其测试）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Re-review at Code reviewWhat I verified this pass rather than trusting the prior one:
Since the last pass, the maintainer also re-ran his full real-stack harness against exactly this head — including a correction to his own earlier report: the Items the maintainer accepted as follow-ups by approving (previously deferred to his judgment, unchanged in the code): (1) the shared per-PR lock trade-off — a forced scan can queue behind a long address round, and the single pending slot per group is now contended between scan/address legs; (2) PAT-identity mismatch fails the run on the terminal-rejection path while sibling writers warn-and-continue; (3) The forced path, failure routing included: sequenceDiagram
participant P1 as review-scan forced path
participant P2 as read_forced_pr_meta
participant P3 as read_live_permission
participant P4 as blocked-status reporter
participant P5 as GitHub API
P1->>P2: metadata, up to 3 tries
P2->>P5: gh pr view
P5-->>P2: metadata ok
P2-->>P1: eligible so far
P1->>P3: fork author permission, up to 3 tries
P3->>P5: gh api collaborator permission
P5-->>P3: 502 on every try
P3-->>P1: permission_lookup_failed
P1->>P4: blocked reason
P4->>P5: read status comments
P4->>P5: PATCH to blocked status
P4-->>P1: delivered
P1-->>P1: exit 1, run red
Test evidence — the PR's own CIPlainly, again: hosted CI has never run on the reviewed head. Every earlier push on this branch created a
The last fully green run on the branch was Sandboxed verification is already in flight for exactly this head: a fresh sponsored Real-scenario testing
中文说明在 本轮亲自核验（而非沿用上一轮结论）：分类器与旧 上一轮之后，维护者又对同一 head 重跑了整套真实环境 harness，并更正了自己先前报告的定性： 维护者以批准方式接受为 follow-up 的未决项（此前留给其取舍、代码未变）：1）共享 per-PR 锁取舍 —— 强制 scan 可能排在长时间 address 轮之后，且每组唯一 pending 名额由 scan/address 分片争抢；2）PAT 身份不匹配在终态拒绝路径上让运行变红，而兄弟写入点是 warning 后继续；3） 测试证据 —— PR 自己的 CI：直说：被审 head 上仍然没有托管 CI。 本分支此前每次推送都生成了 沙箱验证已在针对同一 head 进行中：本次 triage 运行已派发一次新的赞助 真实场景测试：N/A —— CLI 无用户可见变化，改动完全在 bot 自己的 GitHub workflow 里。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a proven, exhaustively reviewed change that the maintainer has approved on this exact head; the one honest reservation is that hosted CI never ran on the merge commit, and it is named below, not waved away. Stepping back: if I'd been handed issue #8409 cold, I'd have proposed exactly this shape — bounded retries that distinguish transient from terminal answers, stable reason codes, and a blocked-status writer that either proves the blocker is visible or fails the run. The implementation matches, and the evidence stack around it is now unusually deep: a behavioral suite that extracts the shell verbatim and kills the mutations a lazier one would ship, the maintainer's real-stack harness re-run under faithful runner options with every one of 31 scenarios byte-identical on this head, and nine review rounds whose findings were answered precisely rather than waved away. The previous pass deferred on two conditions: no CI on the head, and three design calls waiting on the maintainer. Both are now resolved by the maintainer himself — he re-verified at this head, ruled the open items follow-up material, and approved this exact commit. Continuing to defer would be the gate substituting its judgment for an informed human decision, which is not what the gate is for. Why not 5/5: the merge commit still has no hosted CI — it never triggered a run (likely the fork update-branch quirk), so the merged tree's build/typecheck is attested only by the maintainer's local suite run and by the branch's last green at Approving — pinned to the reviewed commit. ✅ 中文说明Confidence: 4/5 —— 一个已被充分证明、经过彻底审查的改动，维护者已在该 head 上批准；唯一诚实的保留意见是托管 CI 从未在合并提交上运行过，下面点名说明，不作掩饰。 退一步看：如果把 issue #8409 交给我从零设计，我会提出完全相同的形状 —— 区分瞬时与终态响应的有界重试、稳定原因码、以及"要么证明阻塞可见、要么让运行失败"的状态写入器。实现与之吻合，且其证据深度不同寻常：逐字节抽取 shell、能杀死懒惰套件会放行的变异的行为测试；维护者的真实环境 harness 在与 runner 一致的 shell 选项下重跑、31 个场景在该 head 上全部字节级一致；九轮评审的发现均被精确回应而非敷衍。 上一轮压在两点上：被审 head 无 CI、三处设计取舍待维护者定夺。如今两点都由维护者本人解决 —— 他在该 head 复验、裁定未决项为 follow-up、并批准了这个确切的提交。继续压住不放，就是门禁用自己的判断替代知情的人类决定 —— 那不是门禁的职责。 为何不是 5/5：合并提交仍无托管 CI —— 它从未触发运行（大概率是 fork update-branch 的特性），合并树的 build/typecheck 只有维护者本地套件与分支在 批准 —— 钉死在被审提交。✅ — Qwen Code · qwen3.8-max 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 — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| actor="$(gh api user --jq '.login' 2> /dev/null || echo '')" | ||
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | ||
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'" |
There was a problem hiding this comment.
[Suggestion] The gh api user identity probe is the only new live lookup in this diff without the bounded retry the rest of the diff adds, and it conflates a failed lookup with a wrong-identity PAT. — Failure scenario: a transient 5xx/rate-limit on gh api user while the comments API is healthy yields actor='' ≠ AUTOFIX_BOT → return 1 → at the terminal-blocker call site the run turns red and the blocked-status comment is not posted — losing exactly the visibility this PR adds — with a warning that points at a PAT misconfiguration that does not exist. The house pattern in this file ('Check bot credentials', ~line 704) splits lookup-failure from mismatch.
| actor="$(gh api user --jq '.login' 2> /dev/null || echo '')" | |
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | |
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'" | |
| if ! actor="$(gh api user --jq '.login' 2> /dev/null)"; then | |
| echo "::warning::Blocked takeover status skipped: could not verify PAT identity" | |
| return 1 | |
| fi | |
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | |
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor}'" |
中文说明
gh api user 身份探测是本 diff 新增实时查询中唯一没有有界重试的，且把“查询失败”与“PAT 身份不符”混为一谈。— 失败场景：gh api user 出现临时 5xx/限流（评论 API 正常）时 actor='' ≠ AUTOFIX_BOT → return 1 → 在终态阻塞调用点整个运行变红，且 blocked 状态评论不会发出——恰好丢失本 PR 要提供的可见性——同时警告会把问题误导为并不存在的 PAT 配置错误。本文件的既有模式（'Check bot credentials'，约 704 行）将查询失败与身份不符分开处理。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| status_id="$(tail -1 <<< "${status_ids}")" | ||
| if [[ -n "${status_id}" ]]; then | ||
| if ! gh api --method PATCH "repos/${REPO}/issues/comments/${status_id}" -f body="${body}" > /dev/null; then |
There was a problem hiding this comment.
[Suggestion] The blocked-state PATCH targets the single <!-- autofix-status --> marker comment without participating in the address job's lifecycle coordination (post_status captures a comment_id at post time; finalize PATCHes that captured id under always()). — Failure scenario: while a review-address round is running on a takeover fork PR, a review event force-dispatches and admission hits a terminal blocker the fork author can cause mid-round (unchecking "Allow edits from maintainers", losing write access): this reporter PATCHes the marker to "⛔ AutoFix blocked"; when the in-flight round ends, its finalize PATCHes the same comment back to "round finished" — the PR page no longer says blocked, the newest review event stays unaddressed, and nothing re-posts the blocked state until a future forced dispatch. The race window spans the whole round (job cap 300 min).
Suggested fix: before PATCHing, skip when the latest marker comment is a live "working" status (let the red run carry the signal), or skip the blocked report while a review-address run for this PR is queued/running.
中文说明
blocked 状态 PATCH 写入的是唯一的 <!-- autofix-status --> 标记评论，但没有参与 address job 的生命周期协调（post_status 在发布时捕获 comment_id，finalize 在 always() 下 PATCH 该 id）。— 失败场景：takeover fork PR 上 review-address 轮次运行中，新评审事件触发强制分发，且准入遇到 fork 作者可在轮次中途造成的终态阻塞（取消 "Allow edits from maintainers"、失去 write 权限）：本函数把标记评论 PATCH 为 "⛔ AutoFix blocked"；运行中的轮次结束时其 finalize 会把同一评论 PATCH 回 "round finished" —— PR 页面不再显示 blocked，最新评审事件仍未处理，且在下一次强制分发前没有任何机制重新发出 blocked 状态。竞态窗口覆盖整个轮次（job 上限 300 分钟）。
建议修复：PATCH 前若最新标记评论是 "working" 状态则跳过（让红色运行本身传递信号），或在本 PR 有排队/运行中的 review-address 时跳过 blocked 上报。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| case "${reason}" in | ||
| permission_lookup_failed|author_permission_*|maintainer_edits_disabled|cross_repo_state_missing) ;; | ||
| *) return 0 ;; |
There was a problem hiding this comment.
[Suggestion] Test gap (pattern, instance 1/4): this terminal/non-terminal gate has no behavioral test — a confirmed surviving mutant proves it. Changing *) return 0 ;; to *) ;; keeps all 110 tests green, but then non-terminal reasons (wrong_base, not_open, …) would post the "⛔ AutoFix blocked … A later scheduled scan will retry" comment, promising a retry that can never succeed. Also unexercised: the actor guard, the PATCH-failure branch, the DRY_RUN/no-label early returns, and the gh pr comment create fallback. — Failure scenario: a future refactor silently breaks the gate and the suite stays green; blocked comments with false retry promises go out for non-terminal rejections.
Suggested fix: parameterize the existing reporter harness — one non-terminal reason (assert rc=0 and zero gh calls), one non-bot actor (rc=1, no PATCH), one PATCH failure (rc=1), one empty comment list (assert gh pr comment is invoked).
中文说明
测试缺口（模式，第 1/4 处）：该终态/非终态门没有行为测试——已确认的存活变异体证明了这一点。把 *) return 0 ;; 改为 *) ;; 后全部 110 个测试仍然通过，但非终态原因（wrong_base、not_open 等）将发出 "⛔ AutoFix blocked … A later scheduled scan will retry" 评论，承诺一次永远不会成功的重试。同样未覆盖：actor 守卫、PATCH 失败分支、DRY_RUN/无标签提前返回、gh pr comment 创建兜底。— 失败场景：未来重构悄悄破坏该门而测试套件仍然全绿；非终态拒绝会发出带有虚假重试承诺的 blocked 评论。
建议修复：参数化现有 reporter 测试装置——一个非终态原因（断言 rc=0 且零 gh 调用）、一个非 bot actor（rc=1、无 PATCH）、一次 PATCH 失败（rc=1）、一个空评论列表（断言调用 gh pr comment）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| and (.labels | type == "array") | ||
| and (.maintainerCanModify | type == "boolean")' > /dev/null <<< "${meta}"; then |
There was a problem hiding this comment.
[Suggestion] Test gap (pattern, instance 2/4): the shape-validation negative path of read_forced_pr_meta is never exercised — the retry tests feed only fully-valid payloads or an outright failing gh. Confirmed surviving mutant: dropping the and (.maintainerCanModify | type == "boolean") conjunct keeps the suite green, while a probe shows the original rejects a payload missing only that field (rc=1 → metadata_fetch_failed) and the mutant accepts it (rc=0). read_live_permission has the same gap: no test feeds a successful-but-unrecognized permission value, so widening its regex to accept anything also survives. — Failure scenario: during an API incident or field rename, partial metadata would flow into admission instead of a fail-closed fetch failure, and no test would catch the regression.
Suggested fix: add fake-gh cases that exit 0 with shape-invalid payloads (meta missing maintainerCanModify; permission "" or "unknown") and assert both readers retry to (attempt 3/3) and return 1.
中文说明
测试缺口（模式，第 2/4 处）：read_forced_pr_meta 的结构校验负路径从未被执行——重试测试只喂入完全合法的载荷或直接失败的 gh。已确认的存活变异体：删除 and (.maintainerCanModify | type == "boolean") 合取项后套件仍全绿，而探针表明原代码会拒绝仅缺该字段的载荷（rc=1 → metadata_fetch_failed），变异体却接受它（rc=0）。read_live_permission 存在同样缺口：没有测试喂入“成功但不可识别”的权限值，因此把正则放宽为接受任意值同样存活。— 失败场景：API 故障或字段改名期间，残缺元数据会流入准入而不是触发 fail-closed 的获取失败，且没有任何测试能发现该回归。
建议修复：新增退出码为 0 但载荷不合法的 fake-gh 用例（meta 缺 maintainerCanModify；权限为 "" 或 "unknown"），并断言两个读取器都重试到 (attempt 3/3) 且返回 1。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| if ! FPERM="$(read_live_permission "${FAUTHOR}")"; then | ||
| echo "::warning::Fork takeover candidate #${FPR} blocked: permission_lookup_failed" | ||
| fleet_row "${FPR}" 'blocked' 'permission_lookup_failed' |
There was a problem hiding this comment.
[Suggestion] Test gap (pattern, instance 3/4): this new scan-loop blocked branch has no test — deleting the entire five-line block keeps all 110 tests green (the generic toContain('permission_lookup_failed') assertion is satisfied by the forced-path and reporter occurrences; no fleet-row assertion covers the 'blocked' state). — Failure scenario: if the branch regresses, a transient permission-lookup failure on the scan path silently falls back to the old below-write skip, making a retryable infra failure indistinguishable in the fleet table from genuine permission revocation — losing exactly the blocked-vs-rejected distinction this PR introduces.
Suggested fix: add a static pin next to the existing fleet-row assertions (e.g. expect(reviewScanJob).toContain("fleet_row \"${FPR}\" 'blocked' 'permission_lookup_failed'")), or exercise the candidate loop with a failing fake gh.
中文说明
测试缺口（模式，第 3/4 处）：这个新的扫描循环 blocked 分支没有测试——删除整个五行代码块后全部 110 个测试仍全绿（通用的 toContain('permission_lookup_failed') 断言已被强制路径和 reporter 中的出现满足；没有任何 fleet-row 断言覆盖 'blocked' 状态）。— 失败场景：若该分支回归，扫描路径上的临时权限查询失败会悄悄退回旧的 below-write 跳过逻辑，使可重试的基础设施故障在 fleet 表中与真实的权限回收无法区分——恰好丢失本 PR 引入的 blocked 与 rejected 的区分。
建议修复：在现有 fleet-row 断言旁增加静态钉住（例如 expect(reviewScanJob).toContain("fleet_row \"${FPR}\" 'blocked' 'permission_lookup_failed'")），或用失败的 fake gh 实际执行候选循环。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| # review-address re-checks before pushing). In-repo PRs are gated | ||
| # by author/label alone. | ||
| if [[ "${OK}" == 'true' && "$(jq -r '.isCrossRepository == true' <<< "${META}")" == 'true' ]]; then | ||
| if [[ "${ADMISSION_REASON}" == 'eligible' && "$(jq -r '.isCrossRepository == true' <<< "${META}")" == 'true' ]]; then |
There was a problem hiding this comment.
[Suggestion] Test gap (pattern, instance 4/4): the isCrossRepository == true conjunct — the only thing exempting in-repo PRs from the live permission check — is a changed line with no behavioral test; a mutant deleting it survives the full suite (verified at this commit). — Failure scenario: the mutant permission-checks in-repo forced PRs, which the scheduled scan never does (its in-repo loop has no permission call): an in-repo takeover-labeled PR whose author holds only read/triage would be admitted by the scan but rejected by the mutant's forced path as author_permission_read — a terminal reason — so the reporter would post a factually wrong "A later scheduled scan will retry" comment while the real-time review event is silently degraded to throttled-schedule pickup.
Suggested fix: extend the existing forced-target test to replay this gate — assert that an eligible in-repo meta never reaches the permission call and an eligible cross-repo meta does (a small bash replay with a stubbed read_live_permission that records invocation, in the style of the suite's existing extractions).
中文说明
测试缺口（模式，第 4/4 处）：isCrossRepository == true 合取项——唯一使仓库内 PR 免于实时权限检查的条件——是被修改的行却没有行为测试；删除该合取项的变异体在整个套件中存活（已在本提交验证）。— 失败场景：变异体会对仓库内强制 PR 做权限检查，而定时扫描从不会这样做（其仓库内循环没有权限调用）：一个作者只有 read/triage 权限、带 takeover 标签的仓库内 PR，扫描会准入，而变异体的强制路径会以 author_permission_read（终态原因）拒绝——于是 reporter 会发出与事实不符的 "A later scheduled scan will retry" 评论，同时实时评审事件被悄悄降级为限流的定时拾取。
建议修复：扩展现有 forced-target 测试以重放该门——断言合法的仓库内 meta 永远不会到达权限调用、而合法的跨仓库 meta 会到达（用记录调用的桩 read_live_permission 做一段小型 bash 重放，沿用套件现有的函数提取风格）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'" | ||
| return 1 | ||
| fi | ||
| body="$(printf '<!-- autofix-status -->\n\n⛔ **AutoFix blocked** — the latest review event was not addressed because takeover admission stopped at `%s`. [View run](https://github.com/%s/actions/runs/%s). A later scheduled scan will retry without advancing the feedback watermark.\n\n<details>\n<summary>中文说明</summary>\n\n⛔ **AutoFix 已阻塞** —— 最新评审事件未被处理，takeover 准入停在 `%s`。[查看运行](https://github.com/%s/actions/runs/%s)。后续定时扫描会重试，本次不会推进反馈水位。\n\n</details>' "${reason}" "${REPO}" "${GITHUB_RUN_ID}" "${reason}" "${REPO}" "${GITHUB_RUN_ID}")" |
There was a problem hiding this comment.
[Suggestion] This body promises "A later scheduled scan will retry" for every terminal reason, but for maintainer_edits_disabled no scheduled scan ever re-checks the PR (pattern, instance 1/2). The scan's fork pre-filter (select(.maintainerCanModify == true), ~line 1928) drops such a PR before any permission call — corroborated by the suite's own candidate-selection assertion — and the forced path emits no fleet row before exiting. — Failure scenario: a maintainer manually dispatches on a takeover-labeled fork PR whose author disabled maintainer edits: admission stops at maintainer_edits_disabled, this comment promises a scheduled retry that never comes, the PR is absent from every fleet table, and the maintainer waits indefinitely — a small reintroduction of the invisible-stall class this PR exists to eliminate. (The sentence also presumes "the latest review event", though a manual dispatch may have been triggered without one.)
Suggested fix: make the retry wording conditional per reason — for maintainer_edits_disabled, say the loop resumes once the fork author re-enables maintainer edits (this blocker needs author action, not a scan retry).
中文说明
该评论正文对每个终态原因都承诺 "A later scheduled scan will retry"，但对 maintainer_edits_disabled，定时扫描永远不会重新检查该 PR（模式，第 1/2 处）。扫描的 fork 预过滤（select(.maintainerCanModify == true)，约 1928 行）在任何权限调用之前就丢弃此类 PR——套件自身的候选选择断言也证实了这一点——且强制路径在退出前不产生任何 fleet row。— 失败场景：维护者对一个作者已禁用 maintainer edits 的带 takeover 标签 fork PR 手动分发：准入停在 maintainer_edits_disabled，本评论承诺一次永远不会到来的定时重试，该 PR 从所有 fleet 表中缺席，维护者无限期等待——小型重现了本 PR 要消除的“隐形停摆”类别。（该句还假定存在 "the latest review event"，但手动分发可能并非由评审事件触发。）
建议修复：按原因条件化重试措辞——对 maintainer_edits_disabled，说明在 fork 作者重新允许 maintainer edits 后流程恢复（该阻塞需要作者行动，而不是扫描重试）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复 + 验证证据：maintainer_edits_disabled 现在明确提示重新开启 maintainer edits，并由回归用例确认不再承诺 scheduled retry。聚焦用例 1/1 通过；完整 workflow 测试 110/110 断言通过；Prettier 与 git diff --check 通过。
| if ! FPERM="$(read_live_permission "${FAUTHOR}")"; then | ||
| echo "::warning::Fork takeover candidate #${FPR} blocked: permission_lookup_failed" | ||
| fleet_row "${FPR}" 'blocked' 'permission_lookup_failed' |
There was a problem hiding this comment.
[Suggestion] For author_permission_triage/author_permission_read blockers, the blocked comment promises "A later scheduled scan will retry", but those retries are permanently invisible (pattern, instance 2/2). The scheduled scan's below-write branch — the unchanged *) case directly under this new blocked branch — emits a plain echo with no fleet row, while this new branch adds one for the transient case: the diff itself creates the asymmetry inside one loop. — Failure scenario: a takeover-labeled fork PR whose author held write+ is engaged; the author is later demoted to read/triage (offboarding, role change). A review event hits the forced path → author_permission_read → blocked comment posted, exit 0. Every subsequent scan passes the pre-filter, gets read, and logs "skipped … below write" with no fleet row: no fleet table ever shows the PR or why the retry keeps failing, and the blocked comment is never updated until a new forced event.
Suggested fix: mirror the lookup-failure branch in the below-write *) case: fleet_row "${FPR}" 'blocked' "author_permission_${FPERM}" so the promised retry leaves a trail (and/or qualify the comment's retry wording for permission reasons).
中文说明
对 author_permission_triage/author_permission_read 阻塞，blocked 评论承诺 "A later scheduled scan will retry"，但这些重试是永久不可见的（模式，第 2/2 处）。定时扫描的 below-write 分支——就在这个新 blocked 分支下方、未修改的 *) 分支——只输出一条普通 echo，没有 fleet row，而新分支却为临时失败情形添加了 fleet row：diff 本身在同一个循环里制造了不对称。— 失败场景：一个作者原本有 write+ 权限的带 takeover 标签 fork PR 被接管；作者后来被降为 read/triage（离职、角色调整）。评审事件触发强制路径 → author_permission_read → 发出 blocked 评论，exit 0。此后每次扫描都通过预过滤、得到 read、记录 "skipped … below write" 且没有 fleet row：任何 fleet 表都不会显示该 PR 或重试为何一直失败，blocked 评论在下一次强制事件前永远不会更新。
建议修复：在 below-write 的 *) 分支中镜像查询失败分支：fleet_row "${FPR}" 'blocked' "author_permission_${FPERM}"，让承诺的重试留下痕迹（并且/或者对权限类原因限定评论中的重试措辞）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复 + 验证证据：定时扫描在 fork 作者权限低于 write 时现在写入 blocked / author_permission_* fleet row，并新增回归断言。聚焦用例 1/1 通过；完整 workflow 测试 110/110 断言通过；Prettier 与 git diff --check 通过。
| ADMISSION_REASON='permission_lookup_failed' | ||
| report_forced_takeover_blocked "${ADMISSION_REASON}" \ | ||
| || echo "::error::Forced PR #${FORCED_PR} blocked status update failed" |
There was a problem hiding this comment.
[Suggestion] The exit-code contract for terminal blockers is asymmetric and undocumented: metadata_fetch_failed exits 1 unconditionally without invoking the reporter (~line 1818); permission_lookup_failed (here) exits 1 even when the blocked comment posts fine; deterministic rejections exit 1 only if the reporter fails (~line 1859); and the scan path adds a fourth behavior (fleet_row blocked, job green). — Failure scenario: oncall paged on a red run cannot map red run → action without reverse-engineering two call sites 20 lines apart; the identical PR is green or red depending on whether the comment API happened to be healthy; a future maintainer "normalizing" either path silently changes the alert semantics. If the split is intentional (unknown outcome = loud; deterministic rejection = quiet once visible), it needs saying.
Suggested fix: add a one-line comment at each exit 1 stating why that class fails the run, or unify the exit codes across blocker classes.
中文说明
终态阻塞的退出码契约不对称且无文档：metadata_fetch_failed 无条件 exit 1 且不调用 reporter（约 1818 行）；permission_lookup_failed（此处）即使 blocked 评论成功发出也 exit 1；确定性拒绝仅在 reporter 失败时 exit 1（约 1859 行）；扫描路径还有第四种行为（fleet_row blocked，job 保持绿色）。— 失败场景：oncall 因红色运行被告警时，必须逆向相距 20 行的两个调用点才能弄清红色运行 → 该如何处理；同一个 PR 会因评论 API 恰好是否健康而时绿时红；未来维护者 "统一" 任一路径都会悄悄改变告警语义。如果这种区分是有意为之（未知结果 = 高声失败；确定性拒绝 = 可见后即安静），需要注释说明。
建议修复：在每个 exit 1 处加一行注释说明该类为何使运行失败，或统一各阻塞类别的退出码。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| read_live_permission() { | ||
| local login="$1" attempt permission | ||
| for attempt in 1 2 3; do | ||
| if permission="$(gh api "repos/${REPO}/collaborators/${login}/permission" --jq '.permission // ""' 2> /dev/null)" \ |
There was a problem hiding this comment.
[Suggestion] All four new gh calls discard stderr via 2> /dev/null (also read_forced_pr_meta ~line 1720, the actor check ~line 1774, the status lookup ~line 1783), so the new retry warnings carry no root-cause detail — command substitution only binds stdout, so the redirect protects nothing in the variable; it only deletes diagnostic text. — Failure scenario: a red run "admission blocked: metadata_fetch_failed" shows three identical warnings, while the four plausible causes — expired PAT (401), secondary rate limit (403), bad PR number (404), GitHub incident (5xx) — produce byte-identical logs and need four different operator responses.
| if permission="$(gh api "repos/${REPO}/collaborators/${login}/permission" --jq '.permission // ""' 2> /dev/null)" \ | |
| if permission="$(gh api "repos/${REPO}/collaborators/${login}/permission" --jq '.permission // ""')" \ |
中文说明
四个新的 gh 调用都用 2> /dev/null 丢弃 stderr（另有 read_forced_pr_meta 约 1720 行、actor 检查约 1774 行、状态查询约 1783 行），因此新的重试警告不携带任何根因信息——命令替换只绑定 stdout，该重定向对变量内容毫无保护作用，只是删掉了诊断文本。— 失败场景：一次 "admission blocked: metadata_fetch_failed" 的红色运行只显示三条完全相同的警告，而四种可能的原因——PAT 过期（401）、次级限流（403）、PR 号错误（404）、GitHub 故障（5xx）——产生逐字节相同的日志，却需要四种不同的处置。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
Code reviewReviewed What I verified independently
Nice touches: the explicit Suggestions1. The blocked comment tells maintainers to wait, for blockers that will never clear (L1779-L1785) Only Worth a third branch for 2. The bot's own fork PRs get no blocked status at all (L1769) [[ "$(jq -r --arg take "${TAKEOVER_LABEL}" '[.labels[]?.name] | index($take) != null' <<< "${META}")" == 'true' ]] || return 0The workflow deliberately manages the bot's own fork PRs without a label (header L24-L29: "The bot's OWN fork PRs … are auto-managed WITHOUT a label when allow-edits is on"), and 3. Terminal API answers are retried and then reported as transient (L1740-L1752, L1717-L1738)
Either produces 3 sleeps, Splitting "the call failed" (retry) from "the call answered and the answer is terminal" (classify, 4. Both readers discard the API error ( The stated goal is making blockers visible, but the only trace left is 5.
6. jq program interpolation deviates from this file's own convention (L1791) --jq ".[] | select(.user.login == \"${AUTOFIX_BOT}\") | select(.body | contains(\"<!-- autofix-status -->\")) | .id"The identical lookup at L3466-L3470 uses 7. Dead After 8. Outputs written immediately before
Test coverageThe behavioural harness is a real step up from string assertions, and neutralising
SummaryNo Critical. The classifier rewrite is sound and I could not find an admission-widening path. The substantive concerns are all on the new reporting surface rather than the gate: #1 and #2 mean the "make terminal blockers visible" promise is only partly delivered (wrong guidance for permission blockers, no coverage for the bot's own fork PRs), and #3/#4 mean some terminal conditions are laundered into transient-looking red runs with no diagnostic in the log. Those three plus the |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| actor="$(gh api user --jq '.login' 2> /dev/null || echo '')" | ||
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then |
There was a problem hiding this comment.
[Suggestion] R1-1: The gh api user identity probe is the only new live lookup in this diff without the bounded retry the rest of the diff adds, and it conflates a failed lookup with a wrong-identity PAT. Still stands at this commit (re-checked; confirmed by probe — a failing api user stub skips the 3-attempt comment lookup and returns 1). — Failure scenario: a transient API error or secondary rate limit yields actor='' → the mismatch branch logs "PAT authenticates as 'unknown'" (misdiagnosing a network hiccup as a misconfigured token) → return 1 → the forced scan exits 1 red even though admission was already safely blocked and the comment lookup immediately below (which does get 3 attempts) could have succeeded.
| actor="$(gh api user --jq '.login' 2> /dev/null || echo '')" | |
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | |
| actor='' | |
| for attempt in 1 2 3; do | |
| if actor="$(gh api user --jq '.login' 2> /dev/null)" && [[ -n "${actor}" ]]; then | |
| break | |
| fi | |
| echo "::warning::PAT identity lookup failed (attempt ${attempt}/3)" >&2 | |
| [[ "${attempt}" -lt 3 ]] && sleep "${attempt}" | |
| done | |
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then |
中文说明
gh api user 身份探测是本 diff 新增的实时查询中唯一没有有界重试的，且把“查询失败”与“PAT 身份错误”混为一谈（本轮复查仍成立；探针确认：令 api user 失败时，下方有 3 次重试的评论查询被直接跳过并 return 1）。— 失败场景：临时 API 错误或次级限流导致 actor='' → 进入不匹配分支，日志打印 "PAT authenticates as 'unknown'"（把网络抖动误诊为 token 配置错误）→ return 1 → 强制扫描以红色 exit 1 结束——尽管准入本已安全阻塞，且紧接其下的评论查询本可成功。
建议修复：给身份探测同样的 3 次重试（如上 suggestion），并在警告文案中区分“重试后仍查询失败”与“认证为其他用户”。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复。验证：聚焦回归 1/1 通过；完整 workflow 测试 110/110 断言通过；Prettier 与 git diff --check 通过。
| status_id="$(tail -1 <<< "${status_ids}")" | ||
| if [[ -n "${status_id}" ]]; then |
There was a problem hiding this comment.
[Suggestion] R1-2: The blocked-state PATCH targets the single <!-- autofix-status --> marker comment without participating in the address job's lifecycle coordination (post_status captures a comment_id at post time; finalize PATCHes that id under if: always()). Still stands at this commit (re-checked). — Failure scenario: a takeover-labeled fork PR is mid-round in review-address; allow-edits is toggled off and a maintainer force-dispatches → this reporter PATCHes the shared marker to "⛔ blocked … no work was started". When the in-flight round ends, finalize PATCHes the same comment back to the round's terminal status — the block becomes invisible; if the round is cancelled before finalize, the false "no work was started" text persists until the next round.
Suggested fix: scope the claim or skip the write when work is in flight — e.g. soften the body to "this run started no work", or skip the PATCH when an address run is live for this PR (the scan step already queries check/run state elsewhere for BUSY_PRS).
中文说明
blocked 状态的 PATCH 直接写入唯一的 <!-- autofix-status --> 标记评论，没有参与 address job 的生命周期协调（post_status 在发布时记录 comment_id，finalize 在 if: always() 下 PATCH 该 id）（本轮复查仍成立）。— 失败场景：某个带 takeover 标签的 fork PR 正在 review-address 轮次中运行；此时 allow-edits 被关闭，maintainer 强制触发扫描 → 本 reporter 把共享标记评论 PATCH 成 "⛔ blocked … no work was started"。在途轮次结束时，finalize 会把同一条评论 PATCH 回该轮次的终态——阻塞状态从此不可见；若该轮次在 finalize 之前被取消，虚假的 "no work was started" 文案会一直保留到下一轮。
建议修复：限定文案范围或在有轮次在跑时跳过写入——例如把文案软化为 "this run started no work"，或在该 PR 存在进行中的 address 运行时跳过 PATCH（scan 步骤已在别处为 BUSY_PRS 查询 check/run 状态）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| for attempt in 1 2 3; do | ||
| if meta="$(gh pr view "${FORCED_PR}" --repo "${REPO}" \ | ||
| --json number,state,author,headRefName,isCrossRepository,baseRefName,labels,maintainerCanModify 2> /dev/null)" \ | ||
| && jq -e 'type == "object" |
There was a problem hiding this comment.
[Suggestion] R1-4: Test gap — the shape-validation negative path of the new readers is never exercised: the retry tests feed only fully-valid payloads or an outright failing gh. Still stands at this commit (mutants re-confirmed): deleting the jq -e shape check makes read_forced_pr_meta accept an exit-0 {}, and deleting the permission regex makes read_live_permission accept an exit-0 none — both mutants keep all 110 tests green. — Failure scenario: a future edit stripping either fail-closed validation ships green, and malformed-but-successful API responses flow into the classifier/admission path instead of being rejected after 3 attempts.
Suggested fix: add two reader scenarios to the existing harness — a gh stub that exits 0 printing {} (expect status 1 and 3 retry warnings) and a stub that exits 0 printing none (expect status 1).
中文说明
测试缺口——新读取器的结构校验负路径从未被执行：重试用例只喂完全合法的 payload 或直接失败的 gh（本轮复查仍成立，变异体再次确认）：删除 jq -e 结构检查后 read_forced_pr_meta 会接受 exit 0 的 {}；删除权限正则后 read_live_permission 会接受 exit 0 的 none——两种变异下全部 110 个测试仍为绿。— 失败场景：未来某次编辑删掉任一 fail-closed 校验后会静默通过 CI，畸形但成功的 API 响应将流入分类器/准入路径，而不是在 3 次尝试后被拒绝。
建议修复：在现有测试装置中新增两个读取器场景——exit 0 输出 {} 的 gh 桩（断言状态码 1 与 3 次重试警告）、exit 0 输出 none 的桩（断言状态码 1）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| ADMISSION_REASON='permission_lookup_failed' | ||
| report_forced_takeover_blocked "${ADMISSION_REASON}" \ |
There was a problem hiding this comment.
[Suggestion] R1-9: The exit-code contract for terminal blockers is asymmetric and undocumented in the code. Still stands at this commit (re-checked): metadata_fetch_failed exits 1 without invoking the reporter; permission_lookup_failed (here) exits 1 even when the blocked status was posted successfully; deterministic rejections exit 0 after reporting. — Failure scenario: with no comment pinning the intent, a future maintainer "normalizing" the exit codes can break the fleet loop's expectation (one candidate's lookup failure records a blocked row and moves on; a red run means visibility was lost) — nothing in the file warns them.
Suggested fix: document the contract where the paths diverge — lookup failures fail loud (exit 1) because visibility could not be guaranteed, while deterministic rejections exit 0 once the blocked status is posted or deliberately skipped.
中文说明
终态阻塞的退出码契约不对称且代码中无文档（本轮复查仍成立）：metadata_fetch_failed 不调用 reporter 直接 exit 1；permission_lookup_failed（此处）即使 blocked 状态已成功发布也 exit 1；确定性拒绝在上报后 exit 0。— 失败场景：由于没有注释钉住该意图，未来某位 maintainer “归一化”退出码时可能破坏舰队循环的预期（单个候选的查询失败记录一条 blocked 行后继续；红色运行代表可见性丢失）——文件里没有任何警告。
建议修复：在路径分叉处补充注释说明契约——查询失败因无法保证可见性而显式失败（exit 1），确定性拒绝在 blocked 状态成功发布或有意跳过时 exit 0。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| echo "::warning::Failed to update blocked takeover status for #${FORCED_PR}" | ||
| return 1 |
There was a problem hiding this comment.
[Suggestion] R2-1: The reporter's comment writes are single-attempt while every read this diff adds gets bounded retries — one transient write failure escalates a safely-resolved deterministic rejection to a red run and loses the blocked-status comment. Verified by probe (3 arms): PATCH fails once → exit 1 and no comment posted; the identical run with a successful PATCH → exit 0; a failing listing recovers on attempt 2 via its retry, while the write does not. Nothing re-attempts the comment: the scheduled scan writes only fleet rows for these reasons. — Failure scenario: a takeover-labeled fork PR is force-dispatched and deterministically rejected (e.g. author_permission_triage); the identity probe and 3-attempt status lookup succeed, then the one-shot PATCH hits a transient 5xx / secondary rate limit → return 1 → exit 1, and the PR-level visibility this feature exists to provide is absent until some future forced dispatch.
Suggested fix: wrap the PATCH/create attempts in the same for attempt in 1 2 3 + sleep "${attempt}" pattern used by the status lookup directly above (spans both write branches):
for attempt in 1 2 3; do
if gh api --method PATCH "repos/${REPO}/issues/comments/${status_id}" -f body="${body}" > /dev/null; then
break
fi
echo "::warning::Failed to update blocked takeover status for #${FORCED_PR} (attempt ${attempt}/3)"
[[ "${attempt}" -lt 3 ]] && sleep "${attempt}"
done中文说明
reporter 的评论写入是单次尝试，而本 diff 新增的所有读取都有有界重试——一次临时写入失败会把已安全确定的确定性拒绝升级为红色运行，并丢失 blocked 状态评论。已通过探针验证（三组对照）：PATCH 失败一次 → exit 1 且未发布任何评论；同场景 PATCH 成功 → exit 0；列表查询失败可在第 2 次重试恢复，而写入不能。没有任何机制重试该评论：定时扫描对这些原因只写 fleet 行。— 失败场景：带 takeover 标签的 fork PR 被强制触发且确定性拒绝（如 author_permission_triage）；身份探测与 3 次重试的状态查询均成功，随后单次 PATCH 遇到临时 5xx/次级限流 → return 1 → exit 1，本功能理应提供的 PR 级可见性在下一次强制触发之前一直缺失。
建议修复：把 PATCH/创建写入包进与上方状态查询相同的 for attempt in 1 2 3 + sleep "${attempt}" 模式（两个写入分支都需要）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复。验证：聚焦回归覆盖 PATCH 与新建评论的首失败重试；完整 workflow 测试 110/110 断言通过；Prettier 与 git diff --check 通过。
| if ! gh pr comment "${FORCED_PR}" --repo "${REPO}" --body "${body}" > /dev/null; then | ||
| echo "::warning::Failed to post blocked takeover status for #${FORCED_PR}" |
There was a problem hiding this comment.
[Suggestion] R2-2: Test gap — the reporter's comment-creation branch (no existing marker → gh pr comment) has zero behavioral coverage: every reporter scenario stubs the marker lookup to return id 123, and no assertion pins gh pr comment "${FORCED_PR}". Verified: deleting this else branch keeps all 110 tests green (an empty if returns 0, so both call sites treat the block as reported), and a reachability probe confirms the branch fires on an exit-0 empty lookup. — Failure scenario: a future edit deleting the branch ships green — green exit, no comment — exactly in the first-block case the feature targets: a freshly takeover-labeled fork PR with no prior address round (no marker exists) whose first forced pickup is blocked.
Suggested fix: add a fourth reporter scenario — the stub gh answers the comments lookup with exit 0 and empty output; assert the reporter exits 0 and the recorded calls contain pr comment 8320 with the blocked body and no PATCH.
中文说明
测试缺口——reporter 的评论创建分支（无现有标记 → gh pr comment）零行为覆盖：所有 reporter 场景都把标记查询桩成返回 id 123，且没有任何断言钉住 gh pr comment "${FORCED_PR}"。已验证：删除该 else 分支后全部 110 个测试仍为绿（空 if 返回 0，两个调用点都会认为阻塞已上报）；可达性探针确认 exit 0 的空查询结果会触发该分支。— 失败场景：未来某次编辑删除该分支后会静默通过 CI——绿色退出、无评论——恰好落在本功能针对的首次阻塞场景：刚打上 takeover 标签、尚无 address 轮次（不存在标记评论）的 fork PR 首次强制触发即被阻塞。
建议修复：新增第四个 reporter 场景——gh 桩对评论查询以 exit 0 返回空输出；断言 reporter 退出码为 0 且调用记录包含带 blocked 正文的 pr comment 8320、不包含 PATCH。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复。验证：无 marker 时的新建评论路径已纳入行为回归，并覆盖首失败后重试；聚焦回归 1/1 通过。
qwen-code-ci-bot
left a comment
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/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| status_id="$(tail -1 <<< "${status_ids}")" | ||
| if [[ -n "${status_id}" ]]; then | ||
| for attempt in 1 2 3; do |
There was a problem hiding this comment.
[Critical] R1-2: Still stands — escalated from Suggestion on this round's verification. The blocked-status reporter PATCHes the newest shared <!-- autofix-status --> marker comment with no coordination with the review-address job's lifecycle. Reachability verified at this commit: the route job dispatches review-scan on review/label/dispatch events without consulting address-run state; review-scan has no concurrency group; BUSY_PRS is computed below the forced block (~line 2027) and consulted only at target emission (~line 2091) — nothing gates this path. — Failure scenario: a takeover-labeled fork PR has an address round in flight (its announcement already PATCHed this same newest marker comment) when a forced scan blocks on a reportable reason (permission_lookup_failed during a permission-API outage — exactly what the new retries exist to absorb — maintainer_edits_disabled via workflow_dispatch, or author_permission_* after mid-round de-privileging) → this PATCH turns the live "AutoFix is working on this PR" announcement into "⛔ AutoFix blocked … no work was started" for up to review-address's 300-minute timeout; in the opposite ordering, finalize's id-based PATCH silently erases the blocked status. Both directions show a user-visible false status on the PR thread. The file already treats this hazard class as gate-worthy: the finalize step's comment says it must not overwrite another round's status comment, but this new writer is ungated.
Suggested fix: gate the blocked-status write on round state — skip the PATCH when the matched comment body is the in-flight round's live announcement (post a separate comment instead), or hoist the BUSY_PRS computation above the forced block; alternatively give the blocked status its own marker (e.g. <!-- autofix-blocked -->) so the two channels cannot overwrite each other.
中文说明
[Critical] R1-2：仍然存在——经本轮验证，由 Suggestion 升级为 Critical。blocked 状态报告函数会 PATCH 最新的共享 <!-- autofix-status --> 标记评论，且未与 review-address job 的生命周期做任何协调。本提交上已验证可达性：route job 在 review/label/dispatch 事件下分发 review-scan 时不查询 address 运行状态；review-scan 没有 concurrency group；BUSY_PRS 在强制准入块之后（约 2027 行）才计算，且只在输出 targets 时（约 2091 行）才被查询——这条路径没有任何门禁。— 失败场景：某个带 takeover 标签的 fork PR 正有一轮 address 在运行（其公告已 PATCH 了同一条最新标记评论），此时强制扫描因可报告原因被阻塞（权限 API 故障期间的 permission_lookup_failed——正是新增重试要吸收的场景、workflow_dispatch 触发的 maintainer_edits_disabled、或轮次中途作者被降权后的 author_permission_*）→ 这个 PATCH 会把"AutoFix 正在处理此 PR"的实时公告改成"⛔ AutoFix blocked … 未开始处理"，最长可持续 review-address 的 300 分钟超时；反过来的顺序下，finalize 基于 id 的 PATCH 又会悄悄抹掉 blocked 状态。两个方向都会在 PR 时间线上向用户展示与事实相反的状态。本文件已将这类危害视为需要设防：finalize 步骤的注释明确说不得覆盖其他轮次的状态评论，但这个新写入器没有门禁。
建议修复：按轮次状态为 blocked 写入设门——当匹配到的评论正文是进行中轮次的实时公告时跳过 PATCH（改为另发一条评论），或把 BUSY_PRS 的计算上移到强制块之前；也可以给 blocked 状态单独的标记（如 <!-- autofix-blocked -->），让两个通道互不覆盖。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
已修复：forced review-scan 现在与同一 PR 的 review-address 共享 qwen-pr-head-write-<PR> 并发锁，blocked reporter 与 address status/finalize 不再并发覆盖同一状态评论；定时扫描使用 run ID 保持独立。
验证证据：聚焦并发契约回归 1/1 通过；完整 scripts/tests/qwen-autofix-workflow.test.js 111/111 通过；Prettier 与 git diff --check 通过；commit 7170261765。
| && jq -e 'type == "object" | ||
| and (.number | type == "number") |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands — the shape-validation negative path of the new readers is never exercised: the retry tests feed only fully-valid payloads or an outright failing gh. Mutants re-confirmed at this commit: deleting this jq -e clause from read_forced_pr_meta (and separately the permission whitelist regex from read_live_permission) keeps all 110 tests green, while live comparators prove both mutants change behaviour (malformed payload: BASE exit=1 rejected vs mutant exit=0 accepted). — Failure scenario: gh pr view can exit 0 with a partial response; with validation in place that payload is retried and surfaces as metadata_fetch_failed. If a future edit drops the check, a transient API degradation flows into forced_admission_reason and is recorded as a permanent PR property with no retries — and this suite certifies the change green.
Suggested fix: in the existing runReader harness, add a fake gh that exits 0 but prints {} (and one printing a wrong-typed field, e.g. number as a string) — expect status 1 with (attempt 3/3); likewise for read_live_permission with an exit-0 payload of "" or "owner".
中文说明
[Suggestion] R1-4：仍然存在——新读取器的结构校验负路径从未被测试覆盖：重试测试要么喂完全合法的载荷，要么让 gh 直接失败。本提交上重新确认了变异体：删除 read_forced_pr_meta 中的这个 jq -e 子句（以及单独删除 read_live_permission 的权限白名单正则）后，全部 110 个测试仍然通过，而实时对照证明两个变异体都改变了行为（畸形载荷：BASE exit=1 拒绝 vs 变异体 exit=0 接受）。— 失败场景：gh pr view 可能以 exit 0 返回残缺响应；有校验时该载荷会被重试并最终表现为 metadata_fetch_failed。若未来某次编辑删掉了这个检查，一次瞬时 API 降级就会直接流入 forced_admission_reason，被当作 PR 的永久属性记录下来且不再重试——而本测试套件还会给这个改动开绿灯。
建议修复：在现有 runReader 测试框架中，加一个 exit 0 但输出 {} 的假 gh（再一个输出错误类型字段，如把 number 变成字符串）——断言 status 1 且带 (attempt 3/3)；对 read_live_permission 同理，喂 exit-0 的 "" 或 "owner"。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| report_forced_takeover_blocked "${ADMISSION_REASON}" \ | ||
| || echo "::error::Forced PR #${FORCED_PR} blocked status update failed" | ||
| echo "::error::Forced PR #${FORCED_PR} admission blocked: ${ADMISSION_REASON}" |
There was a problem hiding this comment.
[Suggestion] R1-9: Still stands — the exit-code contract for terminal blockers is asymmetric and undocumented in the code. metadata_fetch_failed exits 1 without invoking the reporter; permission_lookup_failed (here) exits 1 even when the blocked comment posts successfully; author_permission_* / maintainer_edits_disabled exit 0 when the comment posts and 1 when it fails. The asymmetry may well be intentional (transient-infrastructure failure → red, definitive policy rejection → green once reported), but no comment in the forced block states it. — Failure scenario: a future maintainer touching the forced path (or the scheduled loop's treatment of exit codes) has no in-code statement of which exits are deliberate: 'fixing' the asymmetry either way silently changes fleet-scan and route behaviour, and a reviewer cannot tell a deliberate contract from a bug — for exactly the exit semantics this PR's fail-loud design rests on.
Suggested fix: add a short comment above the forced-admission block stating the contract: lookup failures exit 1 unconditionally (transient, must alert); deterministic rejections exit 0 once the blocked status is posted (terminal, reported); report failure exits 1 (the blocker could not be made visible).
中文说明
[Suggestion] R1-9：仍然存在——终态阻塞的退出码契约不对称，且代码中没有任何文档。metadata_fetch_failed 不调用报告函数直接 exit 1；permission_lookup_failed（此处）即使 blocked 评论成功发出也 exit 1；author_permission_* / maintainer_edits_disabled 在评论发出后 exit 0，发不出则 exit 1。这种不对称很可能有意为之（瞬时基础设施失败 → 红色；确定性策略拒绝 → 报告后绿色），但强制块里没有任何注释说明。— 失败场景：未来改动强制路径（或定时循环对退出码的处理）的维护者，看不到任何关于哪些退出是有意的说明：无论朝哪个方向"修复"这个不对称，都会悄悄改变舰队扫描与 route 的行为，而审查者无法区分有意的契约和 bug——而这恰是本 PR fail-loud 设计所依赖的退出语义。
建议修复：在强制准入块上方加一条简短注释说明契约：查询失败无条件 exit 1（瞬时错误，必须告警）；确定性拒绝在 blocked 状态发出后 exit 0（终态，已报告）；报告失败 exit 1（阻塞原因无法被公开）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| echo "::warning::Failed to post blocked takeover status for #${FORCED_PR} (attempt ${attempt}/3)" | ||
| [[ "${attempt}" -lt 3 ]] && sleep "${attempt}" | ||
| done |
There was a problem hiding this comment.
[Suggestion] Test gap: the reporter's terminal WRITE-failure fall-through — this return 1 after both the PATCH loop and the create-comment loop exhaust all 3 attempts — has no behavioral test. Issue 8409's acceptance criterion requires 'inability to read or update that status also fails the run visibly', but only the READ side (FAIL_STATUS_LOOKUP) is behaviorally proven; every *_ONCE stub flag fails exactly once, so no scenario exhausts the write retries. Mutant proof: flipping this return 1 to return 0 keeps 110/110 tests green, while a probe shows BASE exit=1 vs mutant exit=0. The code at this commit is correct (both call sites exit 1 when the reporter fails) — only the behavioral proof is missing. — Failure scenario: if a future mutation makes the reporter return success after exhausting the write retries, a deterministic terminal blocker during a comment-API outage would exit 0 green with the stale successful-round status still visible — a re-ship of issue 8409's exact harm, certified green by this suite.
Suggested fix: add reporter scenarios with a stub gh that always fails the PATCH endpoint (and one always failing gh pr comment under NO_STATUS_MARKER), asserting status 1 — mirroring the existing FAIL_STATUS_LOOKUP assertion.
中文说明
[Suggestion] 测试缺口：报告函数的终态写失败兜底——即 PATCH 循环与新建评论循环都耗尽 3 次尝试后的这个 return 1——没有行为测试。Issue 8409 的验收条件要求"无法读取或更新该状态时运行必须明显失败"，但目前只有读侧（FAIL_STATUS_LOOKUP）被行为性证明；所有 *_ONCE 桩标志都只失败一次，没有任何场景会耗尽写重试。变异体证明：把这个 return 1 翻成 return 0，110/110 测试仍全绿，而探针显示 BASE exit=1 vs 变异体 exit=0。本提交的代码本身是正确的（两个调用点在报告函数失败时都会 exit 1）——缺的只是行为性证明。— 失败场景：若未来某次变异让报告函数在写重试耗尽后返回成功，那么评论 API 故障期间的一个确定性终态阻塞就会以 exit 0 绿掉，旧的成功轮次状态依旧可见——等于重新放出 issue 8409 的原生危害，而套件还会开绿灯。
建议修复：新增报告函数场景，用始终令 PATCH 端点失败的桩 gh（以及 NO_STATUS_MARKER 下始终令 gh pr comment 失败的桩），断言 status 1——与现有 FAIL_STATUS_LOOKUP 断言对称。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| local login="$1" attempt permission | ||
| for attempt in 1 2 3; do | ||
| if permission="$(gh api "repos/${REPO}/collaborators/${login}/permission" --jq '.permission // ""' 2> /dev/null)" \ | ||
| && [[ "${permission}" =~ ^(admin|maintain|write|triage|read)$ ]]; then |
There was a problem hiding this comment.
[Suggestion] Terminal API answers are retried and reported as transient: this whitelist rejects permission: "none", and live probing found that answer is returned on THIS repo for bot logins (dependabot[bot], github-actions[bot]). A bot-authored fork PR carrying the takeover label then gets a definitive no-permission answer retried 3x and classified permission_lookup_failed — forced path exits 1 red on every dispatch and posts a blocked comment promising a scheduled retry that can never succeed, where the pre-change code resolved the same input in one call with a clean exit-0 rejection; the scan path records fleet rows indistinguishable from a genuine API outage. This confirms, with an additional concrete trigger, the concern in @wenshao's unresolved review comment (suggestion 3). Write-safety is preserved (fail-closed, no work starts) — the harm is diagnostic quality and false-red runs. — Failure scenario: a bot-authored fork PR carrying autofix/takeover is force-dispatched or picked up by the scheduled scan → GET collaborators/{bot}/permission returns "none" → whitelist rejects it → 3 retries (~3 s) → permission_lookup_failed → a red forced run plus a blocked comment promising a retry that can never succeed, every time, until the label is removed.
Suggested fix: split 'the call failed' (retry) from 'the call answered and the answer is terminal': accept none as a definitive answer and classify it author_permission_none on the clean exit-0/comment path, reserving permission_lookup_failed for transport/5xx/rate-limit failures.
中文说明
[Suggestion] 终态 API 应答被当作瞬时错误重试：这个白名单拒绝 permission: "none"，而实测发现本仓库对 bot 账号（dependabot[bot]、github-actions[bot]）就会返回该值。于是带 takeover 标签的 bot 作者 fork PR 会把一个确定性的无权限应答重试 3 次并归类为 permission_lookup_failed——强制路径每次分发都 exit 1 变红，并发一条承诺"定时扫描会重试"的 blocked 评论（而这个重试永远不会成功）；改动前的代码对同样输入一次调用即可干净地 exit 0 拒绝；定时扫描路径记录的舰队行则与真实 API 故障无法区分。这以一个额外的具体触发场景，印证了 @wenshao 未解决评论中的第 3 条建议。写安全不受影响（fail-closed，不会开始处理）——危害在于诊断质量与假红色运行。— 失败场景：带 autofix/takeover 标签的 bot 作者 fork PR 被强制分发或定时扫描拾取 → GET collaborators/{bot}/permission 返回 "none" → 白名单拒绝 → 重试 3 次（约 3 秒）→ permission_lookup_failed → 每次都是一次红色强制运行加一条承诺永远不会成功的重试的 blocked 评论，直到标签被移除。
建议修复：把"调用失败"（重试）与"调用成功且应答是终态"（直接归类）分开：把 none 作为确定性应答接受下来，归类为 author_permission_none 走干净的 exit 0/评论路径，permission_lookup_failed 只留给传输错误/5xx/限流。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| for attempt in 1 2 3; do | ||
| if gh pr comment "${FORCED_PR}" --repo "${REPO}" --body "${body}" > /dev/null; then | ||
| return 0 |
There was a problem hiding this comment.
[Suggestion] report_forced_takeover_blocked re-implements the marker-based status-comment upsert that already exists in this same workflow's 'Post autofix status comment' step (~lines 3458-3495) — find the bot's newest <!-- autofix-status --> comment, PATCH it, else create — and the two copies already diverge: selection (slurped [ .[][] ... ] | last vs tail -1 over per-page --jq output — same result today, different mechanism), create path (gh api ... -f body --jq '.id' capturing the id for finalize vs gh pr comment with no id), retry policy (one-shot vs 3x), and marker handling (MARKER variable vs the literal hardcoded twice). — Failure scenario: both implementations manage the SAME per-PR status comment (same marker, same bot author). Any future change to the marker, the newest-comment selection rule, or the create path must now be applied in two places; applied to one, the writers stop agreeing on which comment is THE status comment and rounds start stacking duplicate status comments (the exact failure the existing step's comment warns about) or orphan the blocked comment. The duplication is structurally induced (review-scan is deliberately checkout-free/API-only), which limits the fix options but not the hazard.
Suggested fix: cheapest consistent option — make the scan-side copy semantically identical to the announcement step (same MARKER variable, same selection semantics, same gh api ... -f body create path). Durable option — extract the upsert into a .github/scripts/ helper used by both jobs; that requires giving review-scan a trusted-base checkout, so state that tradeoff explicitly if rejected.
中文说明
[Suggestion] report_forced_takeover_blocked 重新实现了本工作流 'Post autofix status comment' 步骤（约 3458-3495 行）已有的基于标记的状态评论 upsert——找到 bot 最新的 <!-- autofix-status --> 评论并 PATCH，否则新建——而且两份实现已经开始分叉：选择逻辑（slurp 后 [ .[][] ... ] | last vs 对每页 --jq 输出取 tail -1——今天结果相同，机制不同）、创建路径（gh api ... -f body --jq '.id' 捕获 id 供 finalize 使用 vs gh pr comment 不捕获 id）、重试策略（一次性 vs 3 次）、标记处理（MARKER 变量 vs 标记字面量硬编码两次）。— 失败场景：两份实现管理的是同一条每 PR 状态评论（同一标记、同一 bot 作者）。未来任何对标记、最新评论选择规则或创建路径的修改都必须同时落在两处；只改一处，两个写入器就会对"哪条评论才是状态评论"产生分歧，轮次开始堆叠重复的状态评论（正是现有步骤注释所警告的失败）或让 blocked 评论变成孤儿。这种重复是结构性的（review-scan 刻意不 checkout、纯 API），限制了修复选项，但不消除危害。
建议修复：最便宜的一致性选项——让扫描侧副本与公告步骤语义完全一致（同样的 MARKER 变量、同样的选择语义、同样的 gh api ... -f body 创建路径）。持久选项——把 upsert 抽成 .github/scripts/ 辅助脚本供两个 job 使用；这需要给 review-scan 一个可信 base 的 checkout，如拒绝请明确说明该取舍。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | ||
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'" | ||
| return 1 |
There was a problem hiding this comment.
[Suggestion] This PAT-identity guard warns ::warning::Blocked takeover status skipped: ... and returns 1 — but nothing is skipped (return 1 fails every caller: the permission_lookup_failed site adds ::error::...blocked status update failed + exit 1, the other site exits 1 directly), and every sibling PAT check in this file (lines 712-714, 1412-1414, 1599-1601, 1634-1636) uses ::error::CI_DEV_BOT_PAT authenticates as 'X'; expected ${AUTOFIX_BOT}. — error level, naming both actual and expected identity. — Failure scenario: CI_DEV_BOT_PAT is rotated to the wrong account (or vars.AUTOFIX_BOT_LOGIN mis-set); a forced dispatch of a blocked takeover PR reaches this check and the log shows a warning saying 'skipped' followed by a generic failure error — an operator reads 'skipped' as a deliberate no-op and investigates the comment API instead of the PAT configuration. For reasons that would otherwise exit 0 cleanly (author_permission_*, maintainer_edits_disabled), the misconfiguration turns the run red while the log still says 'skipped'.
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | |
| echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'" | |
| return 1 | |
| if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then | |
| echo "::error::CI_DEV_BOT_PAT authenticates as '${actor:-unknown}'; expected ${AUTOFIX_BOT}." | |
| return 1 |
中文说明
[Suggestion] 这个 PAT 身份守卫打出 ::warning::Blocked takeover status skipped: ... 并 return 1——但什么都没被"跳过"（return 1 会让每个调用方失败：permission_lookup_failed 调用点会追加 ::error::...blocked status update failed + exit 1，另一调用点直接 exit 1），而且本文件所有同族 PAT 检查（712-714、1412-1414、1599-1601、1634-1636 行）都用 ::error::CI_DEV_BOT_PAT authenticates as 'X'; expected ${AUTOFIX_BOT}.——error 级别，同时给出实际与期望身份。— 失败场景：CI_DEV_BOT_PAT 被轮换到错误账号（或 vars.AUTOFIX_BOT_LOGIN 配错）；一次对被阻塞 takeover PR 的强制分发走到这个检查，日志先显示一条说 "skipped" 的 warning，再跟一条笼统的失败 error——运维会把 "skipped" 读成有意的空操作，转而去查评论 API 而不是 PAT 配置。对原本可以干净 exit 0 的原因（author_permission_*、maintainer_edits_disabled），这个配置错误会把运行变红，而日志仍然写着 "skipped"。
（建议修复见上方 suggestion 代码块：改用 error 级别并同时输出实际与期望身份。）
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if ! report_forced_takeover_blocked "${ADMISSION_REASON}"; then | ||
| echo "::error::Forced PR #${FORCED_PR} blocked status update failed" |
There was a problem hiding this comment.
[Suggestion] On the report-failure exit, the admission reason is never logged for maintainer_edits_disabled: permission_lookup_failed is echoed after its report regardless of outcome (~line 1866), author_permission_* is echoed before this branch (~line 1878), but maintainer_edits_disabled reaches this if ! report_forced_takeover_blocked branch without ever having been echoed, and the branch exits 1 BEFORE the rejected: ${ADMISSION_REASON} line — unreachable on failure. — Failure scenario: comment API outage (three failed PATCH/post attempts) during a forced dispatch of a fork PR whose author disabled maintainer edits: the reason exists only in the comment body that failed to post; the log contains in-function retry warnings and then only ::error::Forced PR #N blocked status update failed, exiting 1 — the operator cannot tell from the run which terminal blocker fired, for exactly the blockers this PR exists to surface.
Suggested fix: emit the reason in the failure branch too, e.g. echo "❌ Forced PR #${FORCED_PR} rejected: ${ADMISSION_REASON}" before the report call (or inside its failure branch).
中文说明
[Suggestion] 在报告失败的退出路径上，maintainer_edits_disabled 的准入原因从未被记录到日志：permission_lookup_failed 无论报告结果如何都会在报告后回显（约 1866 行），author_permission_* 在进入本分支前已被回显（约 1878 行），但 maintainer_edits_disabled 到达这个 if ! report_forced_takeover_blocked 分支时从未被回显，而该分支在 rejected: ${ADMISSION_REASON} 那一行之前就 exit 1——失败时根本走不到那行。— 失败场景：对一个作者已关闭 maintainer edits 的 fork PR 强制分发时评论 API 故障（PATCH/发送三次全失败）：原因只存在于那条没能发出的评论正文里；日志只有函数内的重试 warning，然后是一句 ::error::Forced PR #N blocked status update failed，exit 1——运维无法从运行日志判断是哪个终态阻塞触发的，而这恰是本 PR 要暴露的那类阻塞。
建议修复：失败分支也输出原因，例如在报告调用之前（或其失败分支内）加 echo "❌ Forced PR #${FORCED_PR} rejected: ${ADMISSION_REASON}"。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| echo "::warning::Failed to read takeover status comments for #${FORCED_PR}" | ||
| return 1 | ||
| fi | ||
| status_id="$(tail -1 <<< "${status_ids}")" |
There was a problem hiding this comment.
[Suggestion] Test-oracle gap: the newest-status-comment selection tail -1 is pinned with a single-id oracle — the fake comments endpoint returns exactly 123 in every reporter scenario (or empty under NO_STATUS_MARKER), where tail -1 and head -1 are identical. Probed divergence: on payload 100\n123, tail -1 selects 123 (newest) while head -1 selects 100 (oldest). — Failure scenario: a PR can accumulate multiple <!-- autofix-status --> comments — the code itself creates that state when a PATCH attempt fails and the gh pr comment fallback later succeeds. A head -1 mutant PATCHes the OLDEST marker comment while the newest, most visible status stays stale — and the suite cannot tell them apart because its oracle has one line.
Suggested fix: have the fake comments endpoint print 100\n123 in a reporter scenario and assert the recorded PATCH hits repos/QwenLM/qwen-code/issues/comments/123.
中文说明
[Suggestion] 测试预言机缺口：最新评论选择 tail -1 被一个单 id 预言机钉住——每个报告函数场景里假评论端点都只返回 123（NO_STATUS_MARKER 时为空），此时 tail -1 与 head -1 完全等价。实测分歧：载荷为 100\n123 时，tail -1 选 123（最新），head -1 选 100（最旧）。— 失败场景：一个 PR 可以累积多条 <!-- autofix-status --> 评论——当 PATCH 尝试失败、随后 gh pr comment 兜底成功时，代码自己就会造成这种状态。head -1 变异体会去 PATCH 最旧的标记评论，而最新、最显眼的状态仍然过期——套件无法区分两者，因为预言机只有一行。
建议修复：让某个报告场景的假评论端点输出 100\n123，并断言记录的 PATCH 命中 repos/QwenLM/qwen-code/issues/comments/123。
— qwen3.8-max via Qwen Code /review (v0.21.4)
|
|
||
| report_forced_takeover_blocked() { | ||
| local reason="$1" actor status_ids status_id body attempt status_lookup_ok next_en next_zh | ||
| [[ "${DRY_RUN}" == 'true' ]] && return 0 |
There was a problem hiding this comment.
[Suggestion] Reverse audit round 3: the reporter's DRY_RUN short-circuit guard has no behavioral test — every reporter scenario passes DRY_RUN=false. Probe-verified: deleting this guard survives 110/110, while a three-arm probe flips on both axes — current code with DRY_RUN=true makes zero gh calls; the guard-deletion mutant with DRY_RUN=true makes 3 calls including the real PATCH. Reachability verified: the workflow_dispatch dry_run input propagates via the route job's dry_run output into this scan step's env, and a manual pr_number + dry_run: true dispatch of a takeover-labeled fork with maintainer_edits_disabled reaches the reporter with DRY_RUN=true — this guard is the sole protection against a write on that path. The code at this commit is correct; only the behavioral proof is missing. — Failure scenario: if a future edit deletes or breaks the guard, a manual dry-run dispatch — contractually "assess/verify, but do not claim, push, or comment" — of a takeover-labeled fork blocked on maintainer_edits_disabled would POST/PATCH a real "AutoFix blocked" status comment, a write in a mode forbidden from writing, while the full suite stays green.
Suggested fix: add one runReporter arm with DRY_RUN: 'true' (takeover-labelled META, terminal reason), asserting status 0 and an empty recorded-calls file — mirroring the existing FAIL_STATUS_LOOKUP assertion.
中文说明
[Suggestion] 反向审计第 3 轮：报告函数的 DRY_RUN 短路守卫没有行为测试——所有报告场景都传 DRY_RUN=false。经探针验证：删除这行守卫后 110/110 测试仍全绿，而三臂探针在两个轴上都能翻转——当前代码在 DRY_RUN=true 时发起 0 次 gh 调用；删除守卫的变异体在 DRY_RUN=true 时发起 3 次调用，包括真实的 PATCH。可达性已验证：workflow_dispatch 的 dry_run 输入经 route job 的 dry_run 输出传入本扫描步骤的 env；对带 takeover 标签且 maintainer_edits_disabled 的 fork 手动执行 pr_number + dry_run: true 分发时，会以 DRY_RUN=true 到达报告函数——这个守卫是该路径上防止写入的唯一防线。本提交的代码是正确的；缺的只是行为性证明。— 失败场景：若未来某次编辑删除或破坏了这个守卫，一次手动 dry-run 分发（按其契约"只评估/验证，不得 claim、push 或评论"）会对一个因 maintainer_edits_disabled 被阻塞的带 takeover 标签 fork POST/PATCH 一条真实的 "AutoFix blocked" 状态评论——在本禁止写入的模式下发生了写入，而整个测试套件仍是绿的。
建议修复：新增一个 DRY_RUN: 'true' 的 runReporter 分支（takeover 标签 META、终态原因），断言 status 0 且记录的调用文件为空——与现有 FAIL_STATUS_LOOKUP 断言对称。
— qwen3.8-max via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
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.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if meta="$(gh pr view "${FORCED_PR}" --repo "${REPO}" \ | ||
| --json number,state,author,headRefName,isCrossRepository,baseRefName,labels,maintainerCanModify 2> /dev/null)" \ |
There was a problem hiding this comment.
[Suggestion] R4-3: All four new gh reads discard stderr via 2> /dev/null (meta here, permission ~1748, actor probe ~1781, status lookup ~1803), so a terminal metadata_fetch_failed / permission_lookup_failed run goes red with no root-cause text — the log carries three identical attempt N/3 warnings and one error line, and the fallback jq -e on empty input exits 4 silently (measured). Nothing validates CI_DEV_BOT_PAT before review-scan spends it, so an operator-typo'd PR number, an expired PAT, and a GitHub outage are indistinguishable. — Failure scenario: a red forced scan pages a maintainer; gh's actual error (Could not resolve to a PullRequest…, HTTP 401: Bad credentials, secondary-rate-limit text) was discarded, so the cause cannot be determined without manually re-running the exact gh call. The route job already has the capture-stderr idiom (~470-490). Suggested fix: capture stderr to a temp file and include it in the final warning/error (the route job's api_error_file idiom), or drop the 2> /dev/null.
中文说明
四个新增的 gh 读取都通过 2> /dev/null 丢弃了 stderr（此处是元数据读取，其余在 ~1748 权限、~1781 身份探测、~1803 状态评论查找），因此终态的 metadata_fetch_failed / permission_lookup_failed 会把运行染红却没有任何根因文本——日志里只有三条相同的 attempt N/3 警告和一条错误行，且空输入上的兜底 jq -e 会静默以 4 退出（已实测）。review-scan 花用 CI_DEV_BOT_PAT 之前没有任何凭证校验，所以手滑输错的 PR 号、过期的 PAT 和 GitHub 故障完全无法区分。— 失败场景：一次染红的强制扫描把维护者叫醒；gh 的真实报错（Could not resolve to a PullRequest…、HTTP 401: Bad credentials、次级限流文本）已被丢弃，不手工重跑一模一样的 gh 调用就无法定位原因。route job 里已有捕获 stderr 的惯用写法（~470-490）。建议修复：把 stderr 捕获到临时文件并并入最终的 warning/error（route job 的 api_error_file 写法），或去掉 2> /dev/null。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| report_forced_takeover_blocked() { | ||
| local reason="$1" actor status_ids status_id body attempt status_lookup_ok next_en next_zh | ||
| [[ "${DRY_RUN}" == 'true' ]] && return 0 | ||
| [[ "$(jq -r --arg take "${TAKEOVER_LABEL}" '[.labels[]?.name] | index($take) != null' <<< "${META}")" == 'true' ]] || return 0 |
There was a problem hiding this comment.
[Suggestion] R4-2: The bot's own fork PRs — managed WITHOUT a label per the workflow header — get no blocked status at all: this takeover-label gate makes the reporter return 0 for them, so terminal blockers on that class exit silently (green for deterministic reasons, red-but-commentless for lookup failures). That is the original bug this PR sets out to fix, persisting for exactly the class the workflow explicitly manages (maintainer pre-landing item, unaddressed at this commit). — Failure scenario: a bot-authored fork PR with allow-edits off is force-dispatched → admission stops at maintainer_edits_disabled → the reporter returns 0 at this gate → exit 0 green with nothing posted; the PR thread keeps showing the last successful-looking state while the feedback is never addressed. Suggested fix: gate the reporter on "managed" (author == AUTOFIX_BOT or takeover label), mirroring how forced_admission_reason defines unmanaged_author.
中文说明
机器人自己的 fork PR——按工作流头部说明是不带标签自动托管的——完全拿不到 blocked 状态：这个 takeover 标签门会让报告函数对它们直接 return 0，于是该类别的终态阻塞会静默退出（确定性原因是绿色，查询失败是染红但无评论）。这正是本 PR 要修复的原始 bug，恰好残留在工作流明确托管的那个类别上（维护者 pre-landing 清单条目，截至本提交未处理）。— 失败场景：一个关闭了 allow-edits 的机器人 fork PR 被强制分发 → 准入停在 maintainer_edits_disabled → 报告函数在此门返回 0 → 绿色 exit 0 且什么都不发；PR 时间线继续显示上一次的成功状态，而反馈从未被处理。建议修复：把报告函数的门改为"托管"（author == AUTOFIX_BOT 或带 takeover 标签），与 forced_admission_reason 对 unmanaged_author 的定义保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
已修复 + 验证证据：blocked reporter 现在同时识别 autofix bot 作者与 takeover 标签，新增无标签 bot-managed fork 回归；聚焦用例 1/1 通过，完整 workflow 111/111 断言通过，Prettier 与 git diff --check 通过。
| else | ||
| next_en='A later scheduled scan will retry without advancing the feedback watermark.' | ||
| next_zh='后续定时扫描会重试，本次不会推进反馈水位。' |
There was a problem hiding this comment.
[Suggestion] R4-1: The blocked comment gives an actionable remedy only for maintainer_edits_disabled; a definitive author_permission_* block gets the generic "A later scheduled scan will retry" — a retry that can only succeed via a human action the text never names, while the sibling case spells its remedy out. The round-1 thread on this was fixed only for the fleet-row facet; the maintainer's explicit third-branch request ("grant the fork author write+, or drop autofix/takeover", listed as a pre-landing item) remains unaddressed at this commit — git diff ea34938..HEAD does not touch these text arms. — Failure scenario: a takeover-labeled fork PR whose author holds read/triage → green exit posts "⛔ AutoFix blocked — author_permission_read … A later scheduled scan will retry". Every scan re-derives the same block until someone grants write+ or removes the label; the maintainer is told to wait rather than act, and no writer updates the comment afterward. Suggested fix: add an arm for author_permission_*, e.g. "Grant the fork author write access (or remove the autofix/takeover label) to resume takeover; scheduled scans re-check permission." with a matching Chinese sentence; keep the generic retry sentence for the genuinely transient permission_lookup_failed.
中文说明
blocked 评论只为 maintainer_edits_disabled 给出可操作的补救指引；确定性的 author_permission_* 阻塞拿到的是笼统的"后续定时扫描会重试"——这个重试只有靠人采取行动才能成功，而文案并没有说明该行动，而相邻分支却写明了补救方式。第 1 轮的相关线程只修复了 fleet-row 可见性那一面；维护者明确提出的第三分支请求（"给 fork 作者 write+ 权限，或移除 autofix/takeover 标签"，列在 pre-landing 清单里）截至本提交仍未处理——git diff ea34938..HEAD 没有触碰这些文案分支。— 失败场景：一个作者只有 read/triage 权限的 takeover fork PR → 绿色退出并发布"⛔ AutoFix blocked — author_permission_read … 后续定时扫描会重试"。在有人授予 write+ 或移除标签之前，每次扫描都会重新推导出同样的阻塞；维护者被告知等待而不是行动，且之后没有任何写入器更新这条评论。建议修复：为 author_permission_* 增加一个分支，例如"授予 fork 作者 write 权限（或移除 autofix/takeover 标签）以恢复 takeover；定时扫描会重新检查权限。"并配对应中文；真正临时的 permission_lookup_failed 保留现有的笼体重试文案。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
已修复 + 验证证据：author_permission_* 现在明确提示授予 write 权限或移除 autofix/takeover 标签，新增文案分支回归；聚焦用例 1/1 通过，完整 workflow 111/111 断言通过，Prettier 与 git diff --check 通过。
| if status_ids="$(gh api "repos/${REPO}/issues/${FORCED_PR}/comments" --paginate \ | ||
| --jq ".[] | select(.user.login == \"${AUTOFIX_BOT}\") | select(.body | contains(\"<!-- autofix-status -->\")) | .id" 2> /dev/null)"; then |
There was a problem hiding this comment.
[Suggestion] R3-3: Still stands — the <!-- autofix-status --> marker-scan + PATCH-or-create upsert is re-implemented inline here; the existing implementation is review-address's 'Post autofix status comment' step (~3473-3498), and the copies already differ: the original parameterises the marker (MARKER variable, jq --arg m/--arg ab, null-guarded (.body // "")) and picks last; this copy hard-codes the marker literal inside the jq filter AND inside the body printf (~1798), interpolates ${AUTOFIX_BOT} into the jq program, and picks via tail -1. The marker must now stay in sync across four sites (1798, 1803, ~3473, ~4736). — Failure scenario: editing one writer without mirroring the other makes the two jobs target different comments, re-creating the comment-stacking the marker scheme (and this PR's new concurrency lock) exists to prevent; a null comment body also makes the new unguarded filter error and burn all 3 retries, where the original degrades gracefully. Suggested fix: at minimum mirror the original inside this function — assign MARKER='<!-- autofix-status -->' once, build the body from "${MARKER}", pass it to the filter with --arg; better, extract the lookup+PATCH/POST into a shared script both jobs call.
中文说明
仍然存在——<!-- autofix-status --> 标记扫描 + PATCH 或新建的 upsert 在此处被内联重新实现；既有实现是 review-address 的 'Post autofix status comment' 步骤（~3473-3498），而两份拷贝已经产生分歧：原版把标记参数化（MARKER 变量、jq --arg m/--arg ab、空值防护 (.body // "")）并用 last 选取；本拷贝把标记字面量硬编码进 jq filter 和正文 printf（~1798），把 ${AUTOFIX_BOT} 内插进 jq 程序，并用 tail -1 选取。标记现在必须在四处保持同步（1798、1803、~3473、~4736）。— 失败场景：只改其中一个写入器而不同步另一个，会让两个 job 瞄准不同的评论，重新制造出标记机制（以及本 PR 新增的并发锁）所要防止的评论堆叠；空值的评论正文还会让新的无防护 filter 报错并烧掉全部 3 次重试，而原版能优雅降级。建议修复：至少在本函数内对齐原版——一次性赋值 MARKER='<!-- autofix-status -->'，用 "${MARKER}" 构造正文，用 --arg 传入 filter；更好是把查找+PATCH/POST 抽成两个 job 共用的脚本。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-1: Still stands — the reporter's terminal WRITE-failure fall-through (this return 1 after both the PATCH loop and the create-comment loop exhaust all 3 attempts) has no behavioral test: the suite covers lookup failure (FAIL_STATUS_LOOKUP → status 1) and transient one-failure retries (FAIL_PATCH_ONCE/FAIL_COMMENT_ONCE → status 0 after retry), but no scenario fails all three write attempts. Additionally, the workflow-level chain (reporter returns 1 → branch exit 1) is pinned only by a job-wide toContain('exit 1'); a mutant flipping that branch to exit 0 keeps 111/111 green (executed). — Failure scenario: a regression making the reporter return 0 on terminal write failure — or the workflow branch exit 0 when the reporter fails — ships green: forced PR hits a terminal blocker, all three status writes fail, the run renders green, and the PR is left with neither work nor a blocked notification — the exact silent failure this PR exists to remove. Suggested fix: add a reporter scenario failing all 3 PATCH (and create) attempts → expect status 1; and anchor the workflow exit by asserting the block spanning if ! report_forced_takeover_blocked … through its fi contains exit 1.
中文说明
仍然存在——报告函数的终态写入失败 fall-through（PATCH 循环和新建评论循环都耗尽 3 次后的这个 return 1）没有行为测试：套件覆盖了查找失败（FAIL_STATUS_LOOKUP → status 1）和临时性单次失败重试（FAIL_PATCH_ONCE/FAIL_COMMENT_ONCE → 重试后 status 0），但没有任何场景让三次写入全部失败。此外，工作流层面的链条（报告函数返回 1 → 分支 exit 1）只被整个 job 范围的 toContain('exit 1') 钉住；把该分支翻转为 exit 0 的突变体依然 111/111 全绿（已执行验证）。— 失败场景：让报告函数在终态写入失败时返回 0 的回归——或让报告函数失败时工作流分支 exit 0 的回归——会在绿灯下上线：强制 PR 命中终态阻塞、三次状态写入全部失败、运行显示绿色，PR 上既没有处理也没有阻塞通知——正是本 PR 要消除的那种静默失败。建议修复：新增一个让 PATCH（和新建）三次全部失败的报告函数场景 → 断言 status 1；并断言从 if ! report_forced_takeover_blocked … 到其 fi 的代码块包含 exit 1，把工作流退出钉牢。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| writeFileSync(join(failingDir, 'gh'), '#!/bin/bash\nexit 1\n'); | ||
| chmodSync(join(failingDir, 'gh'), 0o755); |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands — the mock gh either exits 1 or prints fully valid output, so the jq schema validation in read_forced_pr_meta and the permission-value regex in read_live_permission are never exercised against a successful-but-malformed response. A mutant replacing the 9-clause schema check with jq -e 'true' keeps 111/111 green (executed at this commit). — Failure scenario: gh pr view exits 0 with degraded JSON (e.g. missing headRefName after an API hiccup). Today the schema check fails closed with metadata_fetch_failed; if the validation silently regresses, the garbage flows into forced_admission_reason, whose // defaults can classify missing-field meta as eligible, building a target from an empty head ref — no test would catch it. Suggested fix: add reader cases where the fake gh exits 0 printing '{}' and meta missing one required field → expect status 1 with (attempt 3/3); likewise read_live_permission with successful output 'owner' → status 1.
中文说明
仍然存在——mock gh 要么以 1 退出、要么打印完全合法的输出，因此 read_forced_pr_meta 的 jq 结构校验和 read_live_permission 的权限值正则从未在"成功但畸形"的响应上被演练。把 9 子句的结构检查替换为 jq -e 'true' 的突变体依然 111/111 全绿（已在本提交执行验证）。— 失败场景：gh pr view 以 0 退出但返回退化 JSON（例如 API 抖动后缺少 headRefName）。当前结构检查会以 metadata_fetch_failed fail-closed；若该校验静默退化，垃圾数据会流入 forced_admission_reason，其 // 默认值可把缺字段的元数据判为 eligible，用空的 head ref 构造 target——没有任何测试能发现。建议修复：新增 fake gh 以 0 退出并打印 '{}'、以及缺少某个必需字段的元数据的读取场景 → 断言 status 1 且带 (attempt 3/3)；同样为 read_live_permission 新增成功输出 'owner' → status 1 的场景。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const reporter = runReporter(); | ||
| expect({ status: reporter.status, stderr: reporter.stderr }).toEqual({ |
There was a problem hiding this comment.
[Suggestion] R3-7: Still stands — every guard in report_forced_takeover_blocked that decides WHETHER it may write is exercised only on its permissive path; five executed guard-removal mutants survive 111/111 at this commit: (1) deleting the case "${reason}" filter — skip_label/not_open/wrong_base/unmanaged_author dispatches then post blocked comments; (2) deleting the DRY_RUN short-circuit (the original R3-7 concern) — dry-run dispatches write real comments; (3) deleting the takeover-label requirement; (4) deleting the actor != AUTOFIX_BOT check; (5) dropping the author_permission_* arm from the case list. — Failure scenario: any of those regressions ships green — e.g. a PR labelled both takeover and skip gets a "⛔ AutoFix blocked" comment despite explicitly opting out, or a dry-run dispatch writes a real comment. Suggested fix: add negative-path scenarios: runReporter({}, 'skip_label') and 'unmanaged_author' → status 0, zero recorded gh calls; DRY_RUN: 'true' → zero gh calls; META without the takeover label → zero calls; actor mock returning 'some-human' → status 1, no PATCH; runReporter({}, 'author_permission_read') → PATCH happens.
中文说明
仍然存在——report_forced_takeover_blocked 中决定是否允许写入的每一个守卫都只在放行路径上被演练；五个已执行的守卫删除突变体在本提交 111/111 全绿下存活：(1) 删除 case "${reason}" 过滤器——skip_label/not_open/wrong_base/unmanaged_author 的分发也会发布 blocked 评论；(2) 删除 DRY_RUN 短路（即原 R3-7 的关注点）——dry-run 分发会写入真实评论；(3) 删除 takeover 标签要求；(4) 删除 actor != AUTOFIX_BOT 检查；(5) 从 case 列表中去掉 author_permission_* 分支。— 失败场景：其中任何一个回归都会在绿灯下上线——例如同时带 takeover 和 skip 标签的 PR 明明已明确退出仍收到"⛔ AutoFix blocked"评论，或 dry-run 分发写入真实评论。建议修复：新增负路径场景：runReporter({}, 'skip_label') 与 'unmanaged_author' → status 0 且记录的 gh 调用为零；DRY_RUN: 'true' → 零 gh 调用；META 不含 takeover 标签 → 零调用；actor mock 返回 'some-human' → status 1 且不 PATCH；runReporter({}, 'author_permission_read') → 发生 PATCH。
— qwen3.8-max via Qwen Code /review (v0.21.5)
qwen-code-ci-bot
left a comment
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.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| elif (.baseRefName // "") != "main" then "wrong_base" | ||
| elif ([.labels[]?.name] | index($skip) != null) then "skip_label" |
There was a problem hiding this comment.
[Suggestion] No classifier test case pins skip_label precedence over the fork checks (unmanaged_author, maintainer_edits_disabled), although the workflow comment documents "skip wins over takeover … excluded everywhere". Probe-verified mutant: moving the skip check below the maintainer_edits_disabled check flips a fork PR carrying both labels from skip_label to maintainer_edits_disabled, and all 11 existing assertions stay green (the only skip cases in the test are in-repo). — Failure scenario: after such a reorder, a fork PR carrying both autofix/takeover and autofix/skip with maintainerCanModify: false classifies as maintainer_edits_disabled; the reporter's case filter excludes skip_label but includes maintainer_edits_disabled, so it posts a "⛔ AutoFix blocked" status comment on a PR whose author explicitly opted out.
Suggested fix — one assertion in the classifier test:
expect(
reason(
meta('human', ['autofix/takeover', 'autofix/skip'], {
isCrossRepository: true,
maintainerCanModify: false,
}),
),
).toBe('skip_label');中文说明
分类器测试没有任何用例钉住 skip_label 相对 fork 检查（unmanaged_author、maintainer_edits_disabled）的优先级，而工作流注释明确写着 "skip wins over takeover … excluded everywhere"。已通过变异探针验证：把 skip 检查移到 maintainer_edits_disabled 检查之下，同时带两个标签的 fork PR 会从 skip_label 变为 maintainer_edits_disabled，而现有 11 条断言全部仍然通过（现有 skip 用例都是同仓库 PR）。— 失败场景：发生这种重排后，同时带 autofix/takeover 和 autofix/skip、maintainerCanModify: false 的 fork PR 会被分类为 maintainer_edits_disabled；报告函数的 case 过滤器排除 skip_label 但包含 maintainer_edits_disabled，于是会在作者已明确选择退出的 PR 上发布 "⛔ AutoFix blocked" 状态评论。
建议修复——在分类器测试中加一条断言（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if ! META="$(read_forced_pr_meta)"; then | ||
| echo "::error::Forced PR #${FORCED_PR} admission blocked: metadata_fetch_failed" |
There was a problem hiding this comment.
[Suggestion] read_forced_pr_meta classifies definitive terminal states as transient metadata_fetch_failed — the sibling, in the metadata reader, of the read_live_permission issue already on this PR (open round-4 comment at line 1749, maintainer review suggestion (3)). Probe-verified: gh pr view 999999999 exits 1 with the definitive GraphQL: Could not resolve to a PullRequest error; the verbatim reader retries it 3× (with sleeps) and returns 1, so a typo'd workflow_dispatch number produces ::error::...metadata_fetch_failed and a red run that re-dispatch can never cure. A deleted/ghost PR author (author: null fails the jq -e shape gate) dies the same death. Pre-diff, both inputs exited 0 with a clean rejection (|| echo '{}' → aggregate rejection → exit 0). Admission stays fail-closed; the defect is the failure-mode regression — permanent red runs and a transient-flavored reason string on conditions that never change. — Failure scenario: a maintainer force-dispatches with a typo'd PR number → 3 retries on a definitive answer → red workflow run implying a transient lookup problem; every retry of the dispatch repeats the red run, where pre-diff the same input exited 0 with an informative rejection.
Suggested fix: mirror the terminal/transient split proposed for read_live_permission — treat a definitive gh pr view failure (nonexistent PR) and a valid-but-unexpected shape (e.g. author: null) as terminal (clean exit 0 with a terminal reason such as pr_not_found / author_missing, which the reporter's case filter keeps silent), and reserve retry-then-exit 1 for genuinely transient transport errors.
中文说明
read_forced_pr_meta 把确定性的终态当成临时性的 metadata_fetch_failed 处理——与本 PR 上已在讨论的 read_live_permission 问题（1749 行的 round-4 开放评论、维护者评审建议 (3)）同族，发生在元数据读取器上。探针验证：gh pr view 999999999 以确定性错误 GraphQL: Could not resolve to a PullRequest 退出 1；逐字提取的读取函数会重试 3 次（含 sleep）后返回 1，因此 workflow_dispatch 输错 PR 号会得到 ::error::...metadata_fetch_failed 和一次重新分发也无法修复的红色运行。PR 作者账号已注销（author: null 过不了 jq -e 结构校验）也是同样的下场。改动前这两种输入都会 exit 0 并给出干净的拒绝（|| echo '{}' → 笼统拒绝 → exit 0）。准入本身仍是 fail-closed；缺陷是失败模式的回归——对永不改变的条件产生永久红色运行和听起来像临时故障的原因码。— 失败场景：维护者用错误的 PR 号手动触发 → 对确定性应答重试 3 次 → 红色运行暗示临时查询故障；每次重试分发都重复红色运行，而改动前同样的输入会 exit 0 并输出有意义的拒绝信息。
建议修复：与 read_live_permission 建议的终态/临时态拆分一致——把 gh pr view 的确定性失败（PR 不存在）和合法但意外的结构（如 author: null）作为终态处理（以 pr_not_found / author_missing 等终态原因干净地 exit 0，报告函数的 case 过滤器对其保持静默），仅对真正的临时传输错误保留重试后 exit 1。
— qwen3.8-max via Qwen Code /review (v0.21.5)
qwen-code-ci-bot
left a comment
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/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| status_ids='' | ||
| status_lookup_ok=false | ||
| for attempt in 1 2 3; do | ||
| if status_ids="$(gh api "repos/${REPO}/issues/${FORCED_PR}/comments" --paginate \ |
There was a problem hiding this comment.
[Critical] R6-1: This new blocked-status comment lookup is the 14th --paginate code site in the workflow, but the deliberate site-count pin at scripts/tests/qwen-autofix-workflow.test.js:2494 still says toBe(13) — the PR's own test suite fails deterministically at this commit. — Failure scenario: every run of npm run test:scripts (chained from npm run test:ci in CI) fails with AssertionError: expected 14 to be 13 in 'normalizes every paginated WORKDIR fetch to one flat array'. Measured 13 --paginate sites at merge base 5173052e3 vs 14 at this head; the pin line is untouched by this diff, so the diff itself turns the suite red. Fix: bump the pin to toBe(14) — and leave the adjacent toBe(9) normalizer pin unchanged, since this site is consumed inline via gh api --paginate --jq (an id stream into tail -1) and never lands in a WORKDIR JSON file, so it must NOT be piped through the jq -s 'add // []' normalizer. The pin's own comment mandates this deliberate update.
中文说明
R6-1：新增的 blocked 状态评论查询是本工作流中第 14 处 --paginate 代码点，但 scripts/tests/qwen-autofix-workflow.test.js:2494 处刻意设置的站点计数断言仍是 toBe(13) —— 本 PR 自己的测试套件在该提交上确定性失败。— 失败场景：每次运行 npm run test:scripts（CI 中由 npm run test:ci 串联执行）都会在 'normalizes every paginated WORKDIR fetch to one flat array' 用例上报 AssertionError: expected 14 to be 13。实测合并基 5173052e3 有 13 处 --paginate，本 HEAD 有 14 处；断言行本身未被本 diff 修改，因此正是本 diff 让测试套件变红。修复：把断言改为 toBe(14) —— 相邻的 toBe(9) normalizer 断言保持不变，因为该站点是通过 gh api --paginate --jq 内联消费的（id 流交给 tail -1），不会落入 WORKDIR JSON 文件，不应接入 jq -s 'add // []' normalizer。该断言自带的注释明确要求这种刻意更新。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expect(reviewScanJob).toContain('metadata_fetch_failed'); | ||
| expect(reviewScanJob).toContain('permission_lookup_failed'); |
There was a problem hiding this comment.
[Suggestion] R6-2: The forced-admission wiring (reader → classifier → live-permission gate → reporter → GITHUB_OUTPUT/exit code) is guarded only by toContain substring pins and a bash -n parse check; no test executes the forced dispatch block end-to-end, so exit-code regressions in the new branches are invisible to the suite. — Failure scenario: probe-verified mutant — deleting only the exit 1 inside the permission_lookup_failed branch makes the extracted run block fall through into the author_permission_none case, running the reporter a second time and exiting 0 (green) after writing both a permission_lookup_failed and an author_permission_none blocked comment (in production the second PATCH overwrites the first): a transient lookup failure is misreported as a permanent permission problem with no failing run to retry. The mutated workflow still satisfies every static assertion in this test file (suite result identical before and after the mutant). Suggested fix: add one end-to-end scenario per forced branch that drives the extracted scan run block with a stubbed gh (the suite already uses this technique for the route step) and asserts the exit status plus targets=[] / has_targets=false in GITHUB_OUTPUT.
中文说明
R6-2：强制准入的接线（reader → classifier → 实时权限门禁 → reporter → GITHUB_OUTPUT/退出码）目前只由 toContain 子串断言和 bash -n 语法检查守护；没有任何测试端到端执行强制分发代码块，因此新增分支中的退出码回归对测试套件不可见。— 失败场景：经探针验证的变异体——仅删除 permission_lookup_failed 分支内的 exit 1 后，提取出的 run 块会落入 author_permission_none 分支，第二次运行 reporter 并以退出码 0（绿色）结束，先后写入 permission_lookup_failed 和 author_permission_none 两条 blocked 评论（生产环境中第二个 PATCH 会覆盖第一个）：一次临时查询失败被误报为永久权限问题，且没有失败运行可供重试。变异后的工作流仍满足本测试文件的全部静态断言（变异前后套件结果完全一致）。建议修复：为每个强制分支增加一个端到端用例，用打桩的 gh 驱动提取出的扫描 run 块（套件已对 route 步骤使用同样技术），并断言退出码以及 GITHUB_OUTPUT 中的 targets=[] / has_targets=false。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Review:
|
qwen-code-ci-bot
left a comment
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/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
[Critical] R6-1 (re-checked at 339540a: still stands): the new blocked-status comment lookup at .github/workflows/qwen-autofix.yml:1810 is the 14th --paginate code site, but the deliberate site-count pin at scripts/tests/qwen-autofix-workflow.test.js:2494 still asserts toBe(13) — the PR's own suite fails deterministically at this commit. Re-executed at head: 1 failed | 113 passed (AssertionError: expected 14 to be 13 in 'normalizes every paginated WORKDIR fetch to one flat array'); base has 13 sites, head 14, and the pin line is untouched by the diff. CI Test (ubuntu-latest, Node 22.x) fails on exactly this assertion. Fix: bump the pin to toBe(14); leave the adjacent toBe(9) normalizer pin unchanged — this site is consumed inline via gh api --paginate --jq (an id stream into tail -1) and never lands in a WORKDIR JSON file, so it must NOT be piped through the jq -s 'add // []' normalizer; the pin's own comment mandates this deliberate update. Not re-posted inline: the resolved line overlaps an existing thread at that location.
[Critical] R7-1 — maintainer blocker (re-checked at 339540a: still stands): read_live_permission (.github/workflows/qwen-autofix.yml:1751-1752) still classifies definitive terminal permission answers as transient lookup failures. Live probes on THIS repo: bot-type logins (dependabot[bot], github-actions[bot], renovate[bot]) and org logins return HTTP 200 with permission: "none"; empty/nonexistent logins return 404 — and the whitelist regex accepts neither, so an executed replay of the verbatim function shows both burn 3 retries and return 1 (permission_lookup_failed) instead of the routine author_permission_none rejection; author_permission_${FPERM:-none} (~1888, ~1958) can never render none. Regression vs main: a terminal rejection now exits 1 (red run) instead of exit 0, the blocked comment promises "A later scheduled scan will retry" — a retry that can never succeed — and the actionable guidance ("Grant the fork author write access, or remove the autofix/takeover label") is unreachable; the scheduled loop re-pays 3 API calls + 3s sleep per such candidate per tick and records blocked / permission_lookup_failed permanently. Maintainer @wenshao requests this fixed before merge (issue-level review). Fix: whitelist none (^(admin|maintain|write|triage|read|none)$) so the existing case *) arms render author_permission_none, and treat HTTP 404 / empty answers as terminal none rather than retrying them; add fixtures for none, admin, and maintain. Reachability caveat (why this is the one residual uncertainty): all 161 observed fork authors in this repo are User-type and resolve to read/write, so the firing incidence on today's traffic is unproven — the mechanism, the dead none branch, and the regression vs main are confirmed regardless. Not re-posted inline: overlaps the round-4 thread at line 1752.
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
[Critical] R6-1 (re-checked at 339540a: still stands): the new blocked-status comment lookup at .github/workflows/qwen-autofix.yml:1810 is the 14th --paginate code site, but the deliberate site-count pin at scripts/tests/qwen-autofix-workflow.test.js:2494 still asserts toBe(13) — the PR's own suite fails deterministically at this commit. Re-executed at head: 1 failed | 113 passed (AssertionError: expected 14 to be 13 in 'normalizes every paginated WORKDIR fetch to one flat array'); base has 13 sites, head 14, and the pin line is untouched by the diff. CI Test (ubuntu-latest, Node 22.x) fails on exactly this assertion. Fix: bump the pin to toBe(14); leave the adjacent toBe(9) normalizer pin unchanged — this site is consumed inline via gh api --paginate --jq (an id stream into tail -1) and never lands in a WORKDIR JSON file, so it must NOT be piped through the jq -s 'add // []' normalizer; the pin's own comment mandates this deliberate update. Not re-posted inline: the resolved line overlaps an existing thread at that location.
[Critical] R7-1 — maintainer blocker (re-checked at 339540a: still stands): read_live_permission (.github/workflows/qwen-autofix.yml:1751-1752) still classifies definitive terminal permission answers as transient lookup failures. Live probes on THIS repo: bot-type logins (dependabot[bot], github-actions[bot], renovate[bot]) and org logins return HTTP 200 with permission: "none"; empty/nonexistent logins return 404 — and the whitelist regex accepts neither, so an executed replay of the verbatim function shows both burn 3 retries and return 1 (permission_lookup_failed) instead of the routine author_permission_none rejection; author_permission_${FPERM:-none} (~1888, ~1958) can never render none. Regression vs main: a terminal rejection now exits 1 (red run) instead of exit 0, the blocked comment promises "A later scheduled scan will retry" — a retry that can never succeed — and the actionable guidance ("Grant the fork author write access, or remove the autofix/takeover label") is unreachable; the scheduled loop re-pays 3 API calls + 3s sleep per such candidate per tick and records blocked / permission_lookup_failed permanently. Maintainer @wenshao requests this fixed before merge (issue-level review). Fix: whitelist none (^(admin|maintain|write|triage|read|none)$) so the existing case *) arms render author_permission_none, and treat HTTP 404 / empty answers as terminal none rather than retrying them; add fixtures for none, admin, and maintain. Reachability caveat (why this is the one residual uncertainty): all 161 observed fork authors in this repo are User-type and resolve to read/write, so the firing incidence on today's traffic is unproven — the mechanism, the dead none branch, and the regression vs main are confirmed regardless. Not re-posted inline: overlaps the round-4 thread at line 1752.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expect(reviewScanJob).toContain('AutoFix blocked'); | ||
| expect(reviewScanJob).toContain('exit 1'); | ||
|
|
||
| const runBlock = reviewScanJob.match(/run: \|-\n([\s\S]*)$/)?.[1]; |
There was a problem hiding this comment.
[Suggestion] R6-2: Still stands — the forced-admission wiring (reader → classifier → live-permission gate → reporter → GITHUB_OUTPUT/exit codes) is still guarded only by toContain substring pins and this bash -n parse check; no test executes the integrated forced-dispatch flow end-to-end. The new component-level replays (each helper against a stubbed gh) are real progress, but they exercise the pieces in isolation. — Failure scenario: a wiring bug between the components — ADMISSION_REASON assigned but never passed to the reporter, the reporter call moved outside the FORCED_PR guard, or an exit-code path reordered — keeps all 114 tests green, because each component passes in isolation and the glue is only substring-pinned; the silent no-op failure class this PR exists to eliminate would ship with a green suite. Fix: add one end-to-end case that executes the extracted forced block (from the FORCED_PR guard through the exit paths) against the stubbed gh, asserting exit status, ADMISSION_REASON routing, and GITHUB_OUTPUT contents for at least: eligible+write, a terminal permission block, and metadata_fetch_failed.
中文说明
[Suggestion] R6-2：仍然存在 —— 强制准入接线（reader → classifier → 实时权限门 → reporter → GITHUB_OUTPUT/退出码）仍然只由 toContain 子串断言和这个 bash -n 语法检查守护；没有任何测试端到端执行完整的强制分发流程。新增的组件级回放（各 helper 对打桩 gh 的执行）是实质进步，但它们只单独执行各个部件。— 失败场景：组件之间的接线错误——例如 ADMISSION_REASON 被赋值却从未传给 reporter、reporter 调用被移出 FORCED_PR 守卫、或退出码路径被重排——不会让任何测试变红，因为每个组件单独执行都能通过，而粘合部分只有子串断言；本 PR 要消灭的静默空跑故障类型会在测试套件全绿的情况下合入。修复：新增一个端到端用例，对打桩 gh 执行提取出的强制分发代码块（从 FORCED_PR 守卫到各退出路径），断言退出码、ADMISSION_REASON 路由和 GITHUB_OUTPUT 内容，至少覆盖：eligible+write、终态权限阻塞、metadata_fetch_failed 三种情况。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Two review blockers on the forced takeover admission gate.
R7-1: `read_live_permission` whitelisted only admin/maintain/write/triage/
read, so GitHub's definitive answers for "holds nothing here" never matched.
Bot-type logins (dependabot[bot], github-actions[bot], renovate[bot]) and org
logins return HTTP 200 with permission 'none'; nonexistent or empty logins
return 404. Both burned three API calls plus back-off and then returned
`permission_lookup_failed`, so the forced path exited 1 (a red run) instead of
the routine `author_permission_none` rejection, the blocked comment promised
"a later scheduled scan will retry" — a retry that can never succeed — and the
actionable "grant the fork author write access" guidance behind
author_permission_* was unreachable. `author_permission_${FPERM:-none}` could
never render `none`. The scheduled loop re-paid the same cost per candidate
per tick, permanently.
Accept 'none', answer HTTP 404 terminally, skip the call for an empty login,
and keep the retry budget for genuinely transient answers (5xx, network, auth)
so a legitimate write-holder is never silently rejected. gh's own stderr now
rides along in the warning instead of going to /dev/null — a rate limit, an
expired PAT and a 5xx were indistinguishable before.
R6-1: the new blocked-status comment lookup is the 14th `--paginate` code
site, but the deliberate site-count pin still asserted 13, failing
`Test (ubuntu-latest, Node 22.x)` deterministically. Bump it to 14 and record
why this site stays out of the `jq -s 'add // []'` normalizer: it consumes the
page stream inline via `--jq ... | .id` into `tail -1` and never lands in a
WORKDIR json file, so wrapping it in an array would break the tail-1 consumer.
R6-2: pin the forced-admission wiring (reader -> classifier -> live-permission
gate -> reporter) end to end, plus fixtures for none, 404, empty login, every
grant level, and a transient 5xx.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| read_forced_pr_meta() { | ||
| local attempt meta | ||
| for attempt in 1 2 3; do |
There was a problem hiding this comment.
[Suggestion] R8-1: The same 3-attempt / linear-backoff / per-attempt-warning retry skeleton is pasted six times inside the one shell block this diff adds (read_forced_pr_meta, read_live_permission, and four loops in report_forced_takeover_blocked: actor lookup, status lookup, PATCH, create-comment). Drift already exists within the diff: the two reader loops warn to stderr, the four reporter loops warn to stdout (both streams are test-pinned). — Failure scenario: any retry-policy change (attempt count, backoff curve, warning format) must be applied consistently in six copies inside one run block; a miss makes the metadata fetch retry differently from the status post that reports its failure, on the same failure path.
Suggested fix: extract one local retry helper beside the new functions and use it for the four plain-command reporter loops (the two reader loops keep their bespoke in-loop validation but share the backoff/warning shape):
retry3() { # <warning-what> <cmd...> — warns per attempt, linear backoff
local what="$1" attempt; shift
for attempt in 1 2 3; do
if "$@"; then return 0; fi
echo "::warning::${what} (attempt ${attempt}/3)" >&2
[[ "${attempt}" -lt 3 ]] && sleep "${attempt}"
done
return 1
}中文说明
R8-1：本 diff 新增的同一个 shell 块中，相同的"3 次尝试 / 线性退避 / 每次尝试告警"重试骨架被粘贴了六次（read_forced_pr_meta、read_live_permission，以及 report_forced_takeover_blocked 中的四个循环：actor 查询、状态评论查询、PATCH、新建评论）。diff 内部已出现漂移：两个读取器循环向 stderr 告警，四个报告器循环向 stdout 告警（两种流都被测试钉住）。— 失败场景：任何重试策略变更（尝试次数、退避曲线、告警格式）都必须在一个 run 块的六处副本中保持一致；漏改一处会使元数据获取与报告其失败的"状态评论发布"在同一条失败路径上采用不同的重试行为。建议修复：在新函数旁抽取一个本地重试助手（如上），四个纯命令的报告器循环直接复用；两个读取器循环保留各自的循环内校验，但共享退避/告警形状。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| [[ "$(jq -r --arg ab "${AUTOFIX_BOT}" --arg take "${TAKEOVER_LABEL}" ' | ||
| ((.author.login // "") == $ab) or ([.labels[]?.name] | index($take) != null) | ||
| ' <<< "${META}")" == 'true' ]] || return 0 |
There was a problem hiding this comment.
[Suggestion] R8-2: This re-derives "PR is managed" with a second copy of the classifier's unmanaged_author predicate — an invariant living in two places that is provably always true at every current call site (every reason reaching this function already passed the same check in forced_admission_reason, and the case filter below rejects the unmanaged reasons anyway). — Failure scenario: if the managed definition ever changes (a second opt-in label, a different author rule), the edit lands in forced_admission_reason and this buried copy keeps applying the old rule: the reporter silently returns 0, the gate exits 0 green, and the blocked-status comment this PR exists to deliver is never posted — with no red run to point at the loss.
| [[ "$(jq -r --arg ab "${AUTOFIX_BOT}" --arg take "${TAKEOVER_LABEL}" ' | |
| ((.author.login // "") == $ab) or ([.labels[]?.name] | index($take) != null) | |
| ' <<< "${META}")" == 'true' ]] || return 0 | |
| # Managedness already guaranteed by forced_admission_reason for every | |
| # reason the case filter below admits; do not re-derive it here. |
中文说明
R8-2：此处用分类器 unmanaged_author 谓词的第二份副本重新推导"该 PR 被托管"——这是一个存在于两处的不变量，且在当前所有调用点上恒为真（到达本函数的每个原因都已在 forced_admission_reason 中通过同样的检查，且下方 case 过滤器本来就会拒绝未托管的原因）。— 失败场景：若"托管"定义日后变更（新增第二个 opt-in 标签、更换作者规则），修改只会落在 forced_admission_reason，而这份深埋的副本仍按旧规则执行：报告器静默返回 0，门禁以绿色 exit 0 结束，本 PR 本应发布的 blocked 状态评论永远不会发出——且没有红色运行可以指向这一丢失。建议修复：删除该 jq 重复检查，以一行注释说明调用方已保证的不变量。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const runGate = (permission) => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'autofix-wiring-')); |
There was a problem hiding this comment.
[Suggestion] R8-3: The new end-to-end wiring test only replays a fork PR with a responsive permission API; the gate's in-repo pass-through and the permission_lookup_failed red exit are never executed. Probe-verified surviving mutants: deleting the fork-only conjunct (isCrossRepository == true) keeps the suite 116/116 green; flipping the permission_lookup_failed block's exit 1 to exit 0 also survives (the metadata_fetch_failed flip is killed only incidentally — it perturbs this test's lazy gate-extraction regex rather than a test that executes the branch). — Failure scenario: a regression deleting the conjunct ships green and an in-repo takeover-labelled PR whose author was demoted below write is rejected as author_permission_read, violating the documented rule that in-repo PRs are gated by author/label alone; a regression flipping the red exit ships green and the silent-green failure mode this PR exists to remove returns.
Suggested fix: add runGate scenarios — an in-repo meta (isCrossRepository: false) asserting no collaborator call and admission by label alone, plus a stub whose collaborators call returns 502 three times asserting status 1, permission_lookup_failed, and no ADMITTED: output.
中文说明
R8-3：新增的端到端接线测试只回放"权限 API 正常响应的 fork PR"；门禁的 in-repo 直通路径与 permission_lookup_failed 红色退出从未被执行。经突变体验证存活的突变体：删除 fork 专属合取条件（isCrossRepository == true）后套件仍 116/116 全绿；将 permission_lookup_failed 分支的 exit 1 翻转为 exit 0 同样存活（metadata_fetch_failed 的翻转只是被偶然杀死——它扰动的是本测试的惰性门禁提取正则，而不是某个真正执行该分支的测试）。— 失败场景：删除合取条件的回归会在绿灯下上线，作者被降到 write 以下的 in-repo takeover 标签 PR 将被以 author_permission_read 拒绝，违反"in-repo PR 仅按作者/标签设门"的成文规则；翻转红色退出的回归同样绿灯上线，本 PR 要消除的"静默绿灯"失败模式将复归。建议修复：新增 runGate 场景——in-repo 元数据（isCrossRepository: false）断言不发起 collaborator 调用且仅凭标签准入；以及 collaborators 调用连续三次返回 502 的桩，断言 status 1、permission_lookup_failed 且无 ADMITTED: 输出。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Review round 2 —
|
Addresses review round 2 on e49aade. - Status-comment lookup now uses the same jq filter as the sibling upsert in 'Post autofix status comment': `// ""` guards so one comment with a null body cannot abort the program (verified: the old filter exits 5, so all three attempts fail and the run reds out without posting the very status it exists to post), and --arg for AUTOFIX_BOT so a repo-configured login containing " or \ is a mismatch rather than a jq parse error. - The lookup no longer sends gh's stderr to /dev/null, matching the rule read_live_permission states 60 lines above. - The blocked body resolves its run link from GITHUB_SERVER_URL like every other status writer, instead of hardcoding github.com. - All four reporter warnings go to stderr like the two reader helpers, so the reporter stays safe to wrap in $( ). Tests: the reporter and reader harnesses now run under production shell options (`set -eo pipefail`) and call through `|| exit $?` so errexit is suspended inside the helper exactly as the `if !` call sites do. New cases cover the null-bodied comment page, the PAT-identity mismatch branch, the in-repo pass-through (no collaborator call), and the permission_lookup_failed red exit. Each kills a probe-verified mutant that previously survived: dropping the `// ""` guard, dropping the `isCrossRepository == true` conjunct, and flipping that `exit 1` to `exit 0`. Verified: npm run test:scripts 946 passed / 9 skipped, 46 files; prettier --check clean; eslint clean; git diff --check clean.
|
已修复（commit Fixed in Fixed
Tests
Each new case kills a mutant that previously survived — I re-ran the suite against all three:
Verification
Not changed, on purpose#1 (shared |
Maintainer verification — local real-stack run of
|
| Injected condition | main (before) |
PR #8410 (after) |
|---|---|---|
Fork permission read: 500 → write |
rejected, dispatch lost | admitted after 1 retry, exit 0 |
| Fork permission read: 500 ×3 | silent reject, exit 0, stale ✅ status stays | permission_lookup_failed, existing status comment PATCHed to the blocked body, exit 1 |
| Permission = HTTP 404 | rejected (leaks {"message":"Not Found"} into the log) |
author_permission_none, exactly 1 API call, routine exit 0 |
Permission = none / read (200) |
same aggregate message | author_permission_none / author_permission_read, 1 call |
| Maintainer edits off | generic rejection | maintainer_edits_disabled + "Re-enable maintainer edits…" guidance |
gh pr view 500 → 200 |
|| echo '{}' → silent green no-op |
retried, admitted |
gh pr view 500 ×3 |
silent green no-op | metadata_fetch_failed, exit 1 |
| Status PATCH 500 ×3 | n/a | 3 bounded attempts, ::error::blocked status update failed, exit 1 |
| Status comment absent | n/a | posts a new one (addComment) with the same marker |
| PAT authenticates as a human | n/a | refuses to write, ::error::, exit 1 |
DRY_RUN=true |
n/a | no write at all (ledger confirms), exit 1 |
not_open / wrong_base / skip_label / unmanaged_author |
one aggregate message | distinct reason codes, no status write (label/reason gate holds) |
| Scheduled scan, fork permission 500 ×3 | skipped … below write, no fleet row |
blocked / permission_lookup_failed fleet row in the run summary |
Scheduled scan, permission read / 404 |
no fleet row | blocked / author_permission_read / author_permission_none |
Edge probes that also passed: a null-body comment in the list does not abort the jq filter (the // "" guard holds); an AUTOFIX_BOT_LOGIN containing " and \ is a mismatch, not a jq parse error (the --arg guard holds); a status comment living on page 2 is found correctly through --paginate + .[][]; the run link is built from GITHUB_SERVER_URL, and the bilingual body renders as intended (screenshot 1).
The PR's own suite: 117/117 pass on the head commit.
🐞 Defect found — connection-level failure on the status read is neither retried nor surfaced
report_forced_takeover_blocked reads the status comments through a pipeline:
if status_ids="$(gh api ".../comments" --paginate 2> "${err}" |
jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' '…')"; then
status_lookup_ok=trueThe review-scan step does not set pipefail (the sibling Post autofix status comment step does — it starts with set -uo pipefail; this one only mentions pipefail in a comment). So the if tests jq's exit status, not gh's. When gh fails with an HTTP status it prints the error body to stdout and jq errors out, so the retry works — that path is fine. But when gh fails at the connection level (TCP reset / TLS abort / DNS blip) stdout is empty, jq -rs happily produces nothing and exits 0.
Verified end-to-end (scenario: terminal author_permission_none, status read answered with a TCP RST):
- 1 status read instead of 3 — the documented bounded retry never happens
status_lookup_ok=truealthough nothing was read- a duplicate
⛔ AutoFix blockedcomment is posted while the stale✅ round 3 pushedcomment stays — the exact two-status state this PR exists to remove - the run exits 0, green
Re-running the identical scenario with the one-line change below gives 3 bounded retries, gh's real diagnosis in the warning, Failed to read takeover status comments, and exit 1 — i.e. the contract the PR description states:
if status_ids="$(set -o pipefail; gh api ".../comments" --paginate 2> "${err}" |
jq -rs …)"; then
Smaller notes (non-blocking)
-
npm run test:scriptsexits 1 on macOS. On this head,scripts/tests/qwen-autofix-workflow.test.jsreports 117/117 passed but the process still exits 1 on an unhandled[vitest-worker]: Timeout calling "onTaskUpdate". Deterministic here — 3/3 runs on macOS (file runtime 62–86 s); the same file on the merge-base is exit 0, 112 passed, 55 s. Linux CI is green, and theTest (macos-latest)job wasskippedin that run, so this is currently unobserved by CI rather than proven safe there. Worth a look, since the file is not excluded on darwin (only onwin32). -
A 404 on the forced PR lookup is a terminal answer treated as transient.
read_forced_pr_metaretries anygh pr viewfailure three times, so a mistypedpr_numberin a manual dispatch burns 3 calls + back-off and then reds out withmetadata_fetch_failed— a transient-sounding code for a permanent condition. This is the same distinction the PR already draws for permissions (e49aade); mirroring it here (grep -q 'HTTP 404'→pr_not_found) would be consistent. -
The
jq -eschema gate on the metadata is effectively unreachable through realgh.gh pr view --jsonalways emits every requested field, so the "malformed metadata" branch cannot fire in practice — a deleted-account author comes through aslogin: "app/"(still a string), and the run then correctly ends atauthor_permission_none. Harmless belt-and-braces, but the transient-vs-malformed coverage it implies is theoretical.
Not covered
- The
concurrency:group change inbf6d7965is a job-level expression, evaluated by the Actions engine before the step runs — it is outside this harness. I only reviewed it by reading; its semantics (do_review != 'true'→github.run_id) look right, but it is untested here. - Everything downstream of admission (
review-address, the push path, the agent round itself). - Real GitHub rate-limit / secondary-limit behaviour.
Verdict: the admission changes do what the description says, before/after is clearly demonstrable, and the reason codes + status write behave correctly under every failure mode I could inject. I'd like the pipefail fix in before merge; the three notes can follow up.
中文版本
维护者验证 —— 在本地对 review-scan 做真实环境运行
我没有只读 diff，而是把这个 PR 的准入路径在本地真实跑了一遍。结论：PR 声称的行为属实且已验证；同时harness 也发现了新版 blocked-status 写入逻辑里的一个真实缺陷，外加两三个合并前值得看一眼的小问题。
验证方法（harness）
目标是：网络层以上的东西一律不打桩。
Scan for PRs with new feedback这一步的run:脚本块从.github/workflows/qwen-autofix.yml里逐字节抽出（不做任何改写，只去掉块缩进），交给bash执行。- 运行在 Linux 容器里（对齐
runs-on: ubuntu-latest：GNUdate -d、bash 5.2），使用真实ghCLI v2.96.0 与jq1.7.1。 - 容器内把
api.github.com指向127.0.0.1，由本地服务通过真实 TLS 提供（自签证书装进容器信任库）。因此gh自己的参数解析、--paginate的 Link 头翻页、--jq过滤、GraphQL 编码与错误处理全部原样执行 —— 只有源站的应答是我们控制的。 - workflow 顶层
env:也是从 YAML 抽取的，没有任何常量是手抄的。 - 每一次 HTTP 请求都记入 ledger，所以断言基于真正发出去的请求，而不是日志文本。
31 个场景 × {main 基线, PR head} = 64 次完整步骤执行，303 次真实 HTTP 请求。
验证的 head 为 bf6d7965（其 review-scan 的 run: 块与 5d45d9f3 逐字节相同）；基线为 merge-base 89b3d5ea。
确认有效的部分
| 注入条件 | main（修改前） |
PR #8410（修改后） |
|---|---|---|
fork 权限查询：500 → write |
拒绝，本次调度丢失 | 重试一次后准入，exit 0 |
| fork 权限查询：连续 3 次 500 | 静默拒绝、exit 0，旧的 ✅ 状态仍在 | permission_lookup_failed，现有状态评论被 PATCH 成 blocked 内容，exit 1 |
| 权限返回 HTTP 404 | 拒绝（且把 {"message":"Not Found"} 泄进日志） |
author_permission_none，只调用 1 次 API，常规拒绝 exit 0 |
权限为 none / read（200） |
同一句笼统信息 | author_permission_none / author_permission_read，1 次调用 |
| 未允许 maintainer edits | 笼统拒绝 | maintainer_edits_disabled + “请重新允许 maintainer edits” 指引 |
gh pr view 500 → 200 |
|| echo '{}' → 静默绿色空跑 |
重试后准入 |
gh pr view 连续 3 次 500 |
静默绿色空跑 | metadata_fetch_failed，exit 1 |
| 状态 PATCH 连续 3 次 500 | 无此路径 | 3 次有界重试，::error::blocked status update failed，exit 1 |
| 不存在状态评论 | 无此路径 | 以相同 marker 新建一条（addComment） |
| PAT 身份是人类账号 | 无此路径 | 拒绝写入，::error::，exit 1 |
DRY_RUN=true |
无此路径 | 完全不写（ledger 可证），exit 1 |
not_open / wrong_base / skip_label / unmanaged_author |
一句笼统信息 | 各自独立原因码，且不写状态评论（标签/原因门控生效） |
| 定时扫描，fork 权限连续 3 次 500 | skipped … below write，无 fleet 行 |
run summary 出现 blocked / permission_lookup_failed fleet 行 |
定时扫描，权限 read / 404 |
无 fleet 行 | blocked / author_permission_read / author_permission_none |
另外通过的边界探针：评论列表里存在 body 为 null 的评论不会让 jq 中断（// "" 护栏有效）；AUTOFIX_BOT_LOGIN 含 " 和 \ 时是不匹配而非 jq 解析错误（--arg 护栏有效）；状态评论位于第 2 页时经 --paginate + .[][] 也能正确定位；run 链接来自 GITHUB_SERVER_URL，中英双语正文渲染符合预期（见截图 1）。
PR 自带测试：head 提交上 117/117 通过。
🐞 发现的缺陷 —— 连接层失败时，状态评论读取既不重试也不暴露
report_forced_takeover_blocked 通过管道读取状态评论：
if status_ids="$(gh api ".../comments" --paginate 2> "${err}" |
jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' '…')"; then
status_lookup_ok=truereview-scan 这一步没有 pipefail（兄弟步骤 Post autofix status comment 有 —— 它以 set -uo pipefail 开头；本步骤里 pipefail 只出现在注释中）。因此 if 判断的是 jq 的退出码而非 gh 的。当 gh 因 HTTP 状态码失败时，它会把错误体打到 stdout，jq 随之报错，所以那条路径没问题。但当 gh 在连接层失败（TCP reset / TLS 中断 / DNS 抖动）时，stdout 是空的，jq -rs 什么都不输出并以 0 退出。
端到端验证结果（场景：终态 author_permission_none，状态评论读取被 TCP RST 打断）：
- 状态读取只发生 1 次而非 3 次 —— 文档承诺的有界重试根本没发生
- 明明什么都没读到，
status_lookup_ok却为true - 新发了一条重复的
⛔ AutoFix blocked评论，而旧的✅ round 3 pushed仍然留在 PR 上 —— 正是本 PR 想消除的“两条状态并存”状态 - 本次运行 exit 0，绿色
把同一场景换成下面这一行改动后重跑：3 次有界重试、warning 里带出 gh 的真实诊断、Failed to read takeover status comments、exit 1 —— 即 PR 描述所声明的契约：
if status_ids="$(set -o pipefail; gh api ".../comments" --paginate 2> "${err}" |
jq -rs …)"; then其他较小的问题（不阻塞）
-
npm run test:scripts在 macOS 上 exit 1。 在本 head 上，scripts/tests/qwen-autofix-workflow.test.js报告 117/117 通过，但进程仍以 1 退出，原因是未处理的[vitest-worker]: Timeout calling "onTaskUpdate"。本机可稳定复现 —— macOS 上 3/3（该文件耗时 62–86 秒）；同一文件在 merge-base 上是 exit 0、112 通过、55 秒。Linux CI 是绿的，且那次绿色运行里Test (macos-latest)是skipped，所以这属于“CI 目前没观察到”，而不是“已证明在 macOS 上没事”。该文件只在win32被排除、darwin 并未排除，值得看一下。 -
强制 PR 元数据查询遇到 404 是终态，却被当作临时错误。
read_forced_pr_meta对任何gh pr view失败都重试三次，因此手动 dispatch 时写错pr_number会白白消耗 3 次调用加退避，最后以metadata_fetch_failed变红 —— 用一个听起来像“临时”的原因码描述一个永久状态。这正是本 PR 已经为权限查询做出的区分（e49aade）；在这里同样处理（grep -q 'HTTP 404'→pr_not_found）会更一致。 -
元数据上的
jq -e结构校验，在真实gh下实际上不可达。gh pr view --json总会输出所请求的全部字段，所以“畸形元数据”分支在实践中不会触发 —— 账号已注销的作者会以login: "app/"出现（仍是字符串），随后流程正确地停在author_permission_none。作为兜底无害，但它所暗示的“临时失败 vs 结构畸形”的覆盖只是理论上的。
未覆盖范围
bf6d7965里的concurrency:改动属于 job 级表达式，由 Actions 引擎在步骤运行前求值，不在本 harness 范围内。我只是读过：其语义（do_review != 'true'→github.run_id）看起来正确，但这里没有实测。- 准入之后的一切（
review-address、推送路径、agent 轮次本身）。 - GitHub 真实的 rate limit / secondary limit 行为。
结论： 准入相关改动确实做到了描述所声称的事情，前后对比清晰可证，且在我能注入的每种失败模式下，原因码与状态写入行为都正确。希望合并前先修掉 pipefail 这一处；其余三点可以后续跟进。
|
Sandboxed verification: ❌ not passed — blocked (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 57 passed · 3 failed · 60 total 中文 — 判定：❌ 不通过 · 阻塞（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：57 通过 · 3 失败 · 60 总计 Verification reportPR #8410 deep verification —
|
| # | Scenario (identical stub on both arms) | HEAD (bf6d79656c) |
BASE (a833204038) |
|---|---|---|---|
| 0 | happy path: meta ok, author=write | admitted, exit 0 | admitted, exit 0 (validity control) |
| 1 | meta 5xx once → ok; author=write | admitted after 2 pr view calls + backoff, exit 0 |
silent reject after 1 call, exit 0 |
| 2 | permission 502 ×3 | permission_lookup_failed, blocked status PATCHed, exit 1 |
silent reject, status untouched, exit 0 |
| 3 | meta 5xx ×3 | metadata_fetch_failed, exit 1 |
silent reject, exit 0 |
| 4 | permission none (HTTP 200) |
author_permission_none, blocked PATCH, exit 0 |
reject, status untouched, exit 0 |
| 5 | permission 404 | author_permission_none in ONE call, PATCH, exit 0 |
silent reject, untouched, exit 0 |
| 6 | empty author login | author_permission_none, ZERO api calls, PATCH, exit 0 |
silent reject, burns a 404 call, exit 0 |
| 7 | blocked + PATCH fails ×3 | exit 1 + ::error:: blocked status update failed |
n/a (no reporter on base) |
| 8 | blocked + status lookup fails ×3 | exit 1, Failed to read takeover status comments |
n/a |
| 9 | no existing status comment | gh pr comment creates one, exit 0 |
n/a |
| 10 | page carries a null-bodied bot comment | // "" guard: PATCH targets id 123, exit 0 |
n/a |
| 11 | PAT authenticates as non-bot | exit 1, nothing written | n/a |
| 12 | DRY_RUN=true | reporter short-circuits, exit 0, no writes | exit 0 |
| 13 | maintainerCanModify=false |
maintainer_edits_disabled, remedy-specific PATCH, exit 0 |
silent reject |
Flips 1–3 are the core of issue #8409: base collapses every lookup failure into a silent green rejection that leaves the previous success status visible; head either recovers (transient) or makes the blocker visible and reds the run when it cannot. Classifier sibling matrix (8 shapes → exact reason codes) and reporter no-write pass-throughs (5 shapes → rc 0, zero gh calls) all pass on head (evidence/02-…). Concurrency group: predicate mirrors the job if: (do_review == 'true'), falls back to github.run_id, prefix byte-equal to review-address's qwen-pr-head-write-; base had no block at all. --paginate site count recomputed = 14, matching the bumped pin.
Corrections
- The PR description's "all 110 workflow assertions passed", "266/266 pass", and "ESLint clean" do not hold on the committed tree: the suite fails at collection (Finding 1) and ESLint exits 1 with
no-redeclareon the changed file. These claims were evidently verified against a pre-merge state. Stated as a correction to the description; the code fix is Finding 1.
Findings
1. Critical — duplicate const reviewAddressJob makes the PR's own regression suite (and test:scripts) fail at committed head
scripts/tests/qwen-autofix-workflow.test.js declares reviewAddressJob twice: line 50 (unbounded slice, added by this PR) and line 62 (the pre-existing bounded slice). Module-scope lexical redeclaration is a SyntaxError, so vitest never collects the file:
FAIL scripts/tests/qwen-autofix-workflow.test.js
SyntaxError: Identifier 'reviewAddressJob' has already been declared
Blast radius: all 120 tests in the suite die — the 5 new regression tests this PR adds and every pre-existing pin (paginate site count, sanitize-step parity, round caps, takeover command gating, …). package.json wires test:scripts into test:ci, so the Test job is red on this tree. ESLint on the file: 62:7 error 'reviewAddressJob' is already defined no-redeclare, exit 1.
Attribution: base runs the same suite 115/115 green in an identical container (tmp/base-tree); the duplicate is added by this PR's diff (+const reviewAddressJob = … hunk at @@ -47,6) without removing the original.
Measured fix (scratch worktree tmp/fix-tree, diff = 2 deleted lines): delete the added declaration at lines 50–51 → suite green 120/120 (base 115 + the 5 new tests; zero other failures). Suggested patch:
minimal fix
const reviewScanJob =
workflow.match(/\n {2}review-scan:[\s\S]*?(?=\n[ ]{2}# ==========)/)?.[0] ??
'';
-const reviewAddressJob =
- workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? '';
const issueAutofixJob =Witnesses: evidence/04-committed-suite-syntaxerror.png, evidence/02-assertion-suite-60-checks.png (gate section), evidence/03-mutation-matrix.png (control row = fixed tree green).
2. Suggestion — blocked-status allowlist leaves stale success comments on closed/renamed takeover PRs
report_forced_takeover_blocked's reason case covers permission_lookup_failed|author_permission_*|maintainer_edits_disabled|cross_repo_state_missing only. A takeover-labeled PR that is now CLOSED (not_open), retargeted (wrong_base), skip-labeled, or no longer managed exits the gate with exit 0 and no status update, so a prior "working/success" status comment remains. Defensible (a closed PR will never address feedback, so the stale comment is historical rather than misleading), but the PR body phrases the feature as "terminal blockers visible" without naming this boundary — a one-line rationale comment (or a test pinning the exclusion) would stop the next reader filing it as a bug. Verified non-blocking: the reporter's pass-through cells return rc 0 with zero gh calls for exactly these shapes.
Not covered
- Per-commit attribution: the checkout is shallow/grafted (
rev-list HEAD^1..HEAD^2reports 1 vs the snapshot's 10 commits), so the aggregateHEAD^1..HEADdiff was verified; individual commit claims (e.g. R7-1, R9-1 rationales) were not exercised separately. - yamllint:
scripts/lint.js --setupcould not install it in-container (pip3: Permission denied) — environmental; the YAML surface is covered by js-yaml parsing (extraction), actionlint (clean, liveness-proven with a planted expression error), and prettier (clean). - Scheduled-scan inspection surface past the forced block: admit-path cells cut the script at
CANDIDATES="${FORCED_PR}"(identical cut both arms); the downstream feedback-inspection loop was not exercised. - Live GitHub API semantics: the
ghstub encodes documented answers (200+nonefor bot/org logins, 404 for absent logins, 5xx transient); real-world drift (e.g. new permission levels) is not covered. - Concurrency group: evaluated statically and via the suite's pins (both mutation-killed), not by an Actions expression evaluator.
- Replay calibration: no production-emitted artifact of this step exists in this environment (first round, no token), so the bash replay is uncalibrated; a real
review-scanrun log containing a forced dispatch would calibrate it. - Event-route admission and the independent
review-addresswrite-safety recheck are unchanged by the PR and were not exercised; Windows/macOS lanes N/A.
Methodology
Environment: node:22-bookworm CI container; merge-ref checkout at depth 2 (HEAD merge, HEAD^1 base, HEAD^2 PR head); npm ci + build pre-run. Harnesses live in tmp/pr8410-verify-20260806-173434/: extract-scan-step.mjs (js-yaml verbatim step extraction, gate mode cuts at the forced-block terminus and closes the if), stub-template/gh (scenario-driven, logs every call), run-cells.sh (drives 13 head + 7 base full/gate cells under bash --noprofile --norc -eo pipefail), run-func-cells.sh (classifier/reporter sibling probes), assert.mjs (all 60 scripted checks incl. gate re-runs; writes assertions.json), mutation-matrix.sh (control + 3 mutants against the full suite in tmp/fix-tree). Scratch worktrees: tmp/base-tree (HEAD^1, A/B control) and tmp/fix-tree (HEAD + the 2-line deletion; mutation substrate) — recreate with git worktree add tmp/base-tree HEAD^1 / git worktree add tmp/fix-tree HEAD if re-running after cleanup. Raw per-cell stdout/stderr/call logs under cells/. Captures via scripts/verify-capture.mjs. Gates: vitest (head, base, fixed), ESLint, prettier, actionlint + shellcheck via scripts/lint.js (both liveness-proven with planted violations).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
…LM#8410) Maintainer verification of the forced-admission path found the one `if` in `report_forced_takeover_blocked` that tests a PIPELINE rather than a single command: if status_ids="$(gh api .../comments --paginate 2> "${err}" | jq -rs ...)" A gh failure carrying an HTTP status prints the error body to stdout, so jq chokes on it and the bounded retry fires. A CONNECTION-level failure (TCP reset, TLS abort, DNS blip) leaves stdout empty — `jq -rs` then prints nothing and exits 0. Absent pipefail that reads as success on nothing read: the loop breaks on attempt 1, `status_lookup_ok` goes true, and the empty id routes the writer to its "no status comment yet" branch, posting a DUPLICATE blocked comment beside the stale one, run green. Set the option locally on that command substitution. `defaults.run.shell: bash` expands to `bash --noprofile --norc -eo pipefail`, so the step is already pipefail on a real runner and this is redundant today; it is the only guard that survives that default changing or the helper moving to a step that sets its own options. Tests: replay the same connection-level failure with the ambient pipefail dropped and pin 3 bounded reads, exit 1, and no comment written; add the HTTP-status half so the two failure shapes stay distinguishable and the local option cannot be "simplified" away as carried by jq; pin both the `set -o pipefail` and `defaults.run.shell: 'bash'` textually, since the ambient half is what every other gh|jq writer in this file relies on without saying so. Both mutations verified to fail the suite.
|
已修复（commit Fixed the blocking The change
if status_ids="$(set -o pipefail; gh api "repos/${REPO}/issues/${FORCED_PR}/comments" --paginate 2> "${err}" |
jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' \
'.[][] | select((.user.login // "") == $ab) | select((.body // "") | contains($m)) | .id')"; thenOne factual note on severity, since it changes what the fix is for rather than whether to take it: this file sets VerificationBoth halves of the failure are now replayed against the extracted helper, and I checked each new assertion bites by mutating the source:
The second row is the new one: it is the case that fails without the local option. Mutation checks —
The third row is there so the Commands run on
Note 1 (macOS |
Picks up main so `Test (ubuntu-latest, Node 22.x)` stops failing. CI checks out `refs/pull/8410/head` but runs the base branch's `ci.yml`, so main's "Check voice guard mirror sync" step ran `npm run check:voice-guard-sync` against this branch's older `package.json`, which predates that script (added in QwenLM#8350) -> `npm error Missing script`, exit 1. Nothing in this PR caused it; the branch was simply 17 commits behind. Merge resolution: both sides had added a `const reviewAddressJob` in `scripts/tests/qwen-autofix-workflow.test.js`, in different hunks, so git merged them textually into a duplicate `const` (SyntaxError at import, whole suite unloadable). Kept main's bounded slice `(?=\n {2}[a-z][a-z0-9-]*:\n|$)` and dropped this branch's older unbounded `[\s\S]*$` form — main's is strictly more general (it still allows EOF, so it keeps working while review-address is the last job, and it shrinks correctly once a job is appended after it).
|
已修复（merge commit Fixed in merge commit 1.
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Re-verification after
|
bf6d7965 (no local pipefail) |
d8d66b5 ($(set -o pipefail; …)) |
|
|---|---|---|
runner shell -eo pipefail — production |
✔ 3/3 reads, nothing written, exit 1 | ✔ 3/3 reads, nothing written, exit 1 |
| ambient pipefail dropped — hypothetical | ✘ 1/3 reads, duplicate ⛔ comment, exit 0 | ✔ 3/3 reads, nothing written, exit 1 |
The failure lives only in the bottom-left cell. So: not a production bug, and I should not have called it one. The fix is still worth keeping for the reason the commit gives — it is the only guard that does not depend on a workflow-level default staying put or on this helper never moving to a step that sets its own options — and the two new tests (connection-level vs HTTP-level failure, with ambient pipefail dropped) pin exactly the right distinction. Pinning defaults.run.shell: 'bash' textually is a good call too: the other gh … | jq … writers in this file silently depend on it.
Also worth noting for anyone else building a harness against this file: -e is part of that same default, and I re-checked it — it changes nothing in this step, because every call site of the new helpers is an if ! / || context, which suspends errexit inside the call.
Full matrix re-run under the runner shell
All 31 scenarios × {merge-base 037b4d9, head dfc6ec5}, now with bash --noprofile --norc -eo pipefail. Every admission outcome, reason code, retry count and status write is byte-identical to my first run — so the whole "Confirmed working" table in my earlier comment stands as written, only now on a faithful shell:
- transient permission 500 →
write: admitted after one retry - permission 500 ×3:
permission_lookup_failed, existing status comment PATCHed to the blocked body, exit 1 - permission 404 /
none/read: terminal, 1 API call,author_permission_*, routine exit 0 maintainer_edits_disabled,metadata_fetch_failed, PAT-identity refusal,DRY_RUNwrites nothing, distinct reason codes fornot_open/wrong_base/skip_label/unmanaged_author, scheduled-scanblockedfleet rows — all unchanged- edge probes unchanged: null comment body,
AUTOFIX_BOT_LOGINcontaining"and\, status comment on page 2,GITHUB_SERVER_URL-derived run link
The PR's own suite on dfc6ec5: 120/120 pass.
Status of my other three notes
test:scriptsexit 1 on macOS — still there, but I have to soften how I described it. On this head it is intermittent: 2 of 6 runs exit 1 with the unhandled[vitest-worker]: Timeout calling "onTaskUpdate", always on the slower runs (~78 s file runtime vs ~58 s when green); tests are 120/120 either way. On the earlier head I saw 3/3, so "deterministic" was an artefact of a small sample. Still worth a look — the file is only excluded onwin32, and this is a process-level non-zero exit, not a test failure.- 404 on the forced PR lookup treated as transient — open.
read_forced_pr_metastill retries a nonexistentpr_numberthree times and reportsmetadata_fetch_failed. Verified again ondfc6ec5: 3pr_viewcalls, exit 1. jq -emetadata schema gate unreachable through realgh— open, and harmless. Informational only.
Verdict
Unchanged apart from the retraction: the admission changes do what the description says, and I could not find a production-reachable failure in the forced or scheduled path across 31 injected conditions. My earlier "I'd like the pipefail fix in before merge" was based on my harness bug, not on the code — d8d66b5 is a good defensive addition rather than a required fix. From my side this is ready to merge; notes 1–3 are follow-up material.
中文版本
d8d66b5 之后的复验 —— 以及对我自己报告的更正
针对 dfc6ec5 重跑了整套 harness（merge-base 现为 037b4d9）。先说更正，因为这是我的问题。
⚠️ 我对 pipefail 那条发现的严重性判断是错的
d8d66b5 的 commit message 是对的，我的报告不对：本 workflow 设置了 defaults.run.shell: 'bash'，GitHub 会把它展开成 bash --noprofile --norc -eo pipefail。而我的 harness 是用普通 bash 启动抽出来的步骤的。 正是这一处不忠实，让状态读取的问题看起来在生产环境可达。实际上并不可达 —— 环境里本来就有的 pipefail 在任何真实 runner 上都已经覆盖了它。
我把 harness 改成完全按 runner 的方式启动步骤，并把该场景重跑成一个 2×2：
bf6d7965（无局部 pipefail） |
d8d66b5（$(set -o pipefail; …)） |
|
|---|---|---|
runner shell -eo pipefail —— 生产 |
✔ 读 3/3 次，不写入，exit 1 | ✔ 读 3/3 次，不写入，exit 1 |
| 去掉环境 pipefail —— 假设 | ✘ 只读 1/3 次，重复 ⛔ 评论，exit 0 | ✔ 读 3/3 次，不写入，exit 1 |
失败只存在于左下角那一格。所以：这不是生产缺陷，我不该那样定性。 这个修复仍然值得保留，理由就是 commit 里写的 —— 它是唯一不依赖「workflow 级默认值保持不变」或「这个 helper 永远不被挪到自设选项的步骤里」的护栏；而新增的两个测试（连接层失败 vs HTTP 层失败，且都去掉环境 pipefail）正好钉住了这个区分。把 defaults.run.shell: 'bash' 做文本断言也是对的：本文件里其他 gh … | jq … 写入者都在无声地依赖它。
另外提醒任何要为这个文件搭 harness 的人：-e 也是同一个默认值的一部分，我一并复核过 —— 它在这一步没有任何影响，因为新 helper 的每一处调用点都是 if ! / || 上下文，会在调用内部挂起 errexit。
在 runner shell 下重跑完整矩阵
全部 31 个场景 × {merge-base 037b4d9，head dfc6ec5}，这次用 bash --noprofile --norc -eo pipefail。所有准入结果、原因码、重试次数、状态写入与我第一轮完全一致 —— 因此前一条评论里那张「确认有效」的表格原样成立，只是这次跑在忠实的 shell 上：
- 权限查询 500 →
write：重试一次后准入 - 权限连续 3 次 500：
permission_lookup_failed，现有状态评论被 PATCH 成 blocked 内容，exit 1 - 权限 404 /
none/read：终态，只调 1 次 API，author_permission_*，常规 exit 0 maintainer_edits_disabled、metadata_fetch_failed、PAT 身份拒写、DRY_RUN不写入、not_open/wrong_base/skip_label/unmanaged_author各自独立原因码、定时扫描的blockedfleet 行 —— 全部不变- 边界探针不变：body 为 null 的评论、含
"与\的AUTOFIX_BOT_LOGIN、位于第 2 页的状态评论、来自GITHUB_SERVER_URL的 run 链接
PR 自带测试在 dfc6ec5 上：120/120 通过。
其余三条的状态
- macOS 上
test:scriptsexit 1 —— 依然存在，但我得修正之前的描述。在这个 head 上是间歇性的：6 次里 2 次 exit 1（未处理的[vitest-worker]: Timeout calling "onTaskUpdate"），且都发生在较慢的那几次（文件耗时约 78 秒，绿色时约 58 秒）；两种情况下测试都是 120/120。之前的 head 上我看到 3/3，所以「稳定复现」是样本太小的假象。仍值得看一下 —— 该文件只在win32被排除，而这是进程级的非零退出，不是测试失败。 - 强制 PR 查询的 404 被当作临时错误 —— 仍未处理。
read_forced_pr_meta对不存在的pr_number仍重试三次并报metadata_fetch_failed。在dfc6ec5上再次验证：3 次pr_view调用，exit 1。 - 元数据的
jq -e结构校验在真实gh下不可达 —— 仍未处理，且无害，仅供参考。
结论
除了那处撤回之外结论不变：准入相关改动确实做到了描述所声称的事情，在 31 种注入条件下，我没能在强制路径或定时路径中找到生产可达的失败。我之前说「希望合并前先修掉 pipefail」是基于我自己的 harness bug，而不是代码本身 —— d8d66b5 属于良好的防御性补强，而非必需修复。就我这边而言可以合并；1–3 三条作为后续跟进即可。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 121 passed · 0 failed · 121 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：121 通过 · 0 失败 · 121 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed (agent verdict) — follow-up round Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 121 passed · 0 failed · 121 total 中文 — 判定：✅ 通过（agent 判定）· 后续验证轮
Verification reportPR #8410 deep verification (follow-up round) —
|
| # | Finding (previous round) | Severity | Status at dfc6ec551 |
|---|---|---|---|
| 1 | Duplicate const reviewAddressJob — suite (120 tests) fails at collection; test:scripts/test:ci red; ESLint no-redeclare |
Critical | Fixed by the merge commit. Re-measured: suite loads and runs 120/120 green at head (base 115/115, +5 new tests, zero regressions); ESLint on the file exits 0 (liveness-proven with a planted unused var); exactly one declaration remains, in main's bounded-slice form (`(?=\n {2}[a-z][a-z0-9-]*:\n |
| 2 | Blocked-status allowlist leaves stale success comments on not_open/wrong_base/skip_label/unmanaged_author |
Suggestion | Stands (accepted boundary, unchanged). Re-measured with reporter pass-through probes: all four shapes (plus eligible) return rc 0 with zero gh calls; the allowlist text is byte-identical to the previous round. Still not documented in the YAML; still defensible — these are terminal states that will never address feedback, so the old comment is historical rather than misleading. |
The previous round's Corrections (description claims "266/266 pass / ESLint clean" not holding at committed head) are now historical: at the new head both claims hold (suite green, ESLint clean). The description was evidently verified pre-merge; the merge fixed the tree.
Central claim + A/B
Central claim: forced review-scan admission is explicit and fail-closed — transient metadata/permission lookups retry up to 3× and recover; terminal answers (none, 404, empty login) are routine rejections with stable reason codes and remedy-specific bilingual blocked-status updates; when the blocker cannot be made visible, the run fails loudly (exit 1) without writing.
Harness: the scan step's run-block extracted verbatim from each arm's YAML via js-yaml, executed under the production shell contract (bash --noprofile --norc -eo pipefail) with a scenario-driven gh stub (documented GitHub semantics: 200+none, 404, 5xx) and a no-op sleep. Reject/block cells run the full step; admit cells run the gate cut at CANDIDATES="${FORCED_PR}" (identical cut both arms). Witness: evidence/01-ab-cells-head-vs-base.png; the full 121-check list in evidence/02-assertion-suite-final.png.
| # | Scenario (identical stub both arms) | HEAD | BASE |
|---|---|---|---|
| 0 | happy path: meta ok, author=write | admitted, exit 0 | admitted, exit 0 (validity control) |
| 1 | meta 502 once → ok | admitted after 2 pr view calls, exit 0 |
silent reject after 1 call, exit 0 |
| 15 | permission 502 once → write | admitted after 2 collaborator calls, exit 0 | silent reject, exit 0 |
| 2 | meta 502 ×3 | metadata_fetch_failed, exit 1, 3 calls |
silent reject, exit 0 |
| 3 | meta malformed ×3 | metadata_fetch_failed, exit 1, 3 calls |
silent reject, exit 0 |
| 4 | permission 502 ×3 | permission_lookup_failed, blocked PATCH, exit 1 |
silent reject, status untouched, exit 0 |
| 5 | permission none (200) |
author_permission_none, remedy PATCH, exit 0, 1 call |
reject, nothing written, exit 0 |
| 6 | permission 404 | author_permission_none, 1 call, PATCH, exit 0 |
silent reject, untouched, exit 0 |
| 7 | empty author login | author_permission_none, zero api calls, PATCH, exit 0 |
burns a 404 call, silent reject, exit 0 |
| 8 | blocked + PATCH fails ×3 | exit 1, blocked status update failed, 3 PATCH attempts |
n/a |
| 9 | blocked + status read conn-fail ×3 | exit 1, 3 reads, 0 writes, failure on stderr | n/a |
| 10 | no existing status marker | gh pr comment creates one, exit 0 |
n/a |
| 11 | page carries null-bodied bot comment | // "" guard: PATCH targets id 123, exit 0 |
n/a |
| 12 | PAT authenticates as non-bot | exit 1, nothing written | n/a |
| 13 | DRY_RUN=true | reporter short-circuits, 0 extra calls, exit 0 | exit 0 |
| 14 | maintainerCanModify=false |
maintainer_edits_disabled, remedy-specific PATCH, exit 0 |
silent reject |
Flips 1–4 are the core of issue #8409: base collapses every lookup failure into a silent green rejection that leaves the previous success status visible; head either recovers (transient) or makes the blocker visible and reds the run when it cannot. Classifier matrix (9 shapes → exact reason codes, skip-beats-takeover, missing isCrossRepository fails closed) and reporter pass-throughs (5 shapes → rc 0, zero calls; positive control author_permission_read writes) all pass on head.
Delta probes: the local pipefail guard (d8d66b5)
The status read is the one if in the reporter testing a pipeline: a connection-level gh failure leaves stdout empty, and jq -rs on empty input prints nothing and exits 0 — indistinguishable from success without pipefail. Production has ambient pipefail today (defaults.run.shell: 'bash', pinned by the suite), so the guard is redundant today and load-bearing tomorrow. Proven by reverting only the guard hunk in a scratch copy and dropping ambient pipefail (bash -e):
| cell | guard | ambient pipefail | result |
|---|---|---|---|
| h16/h21 | present | dropped | exit 1, 3 bounded reads, 0 writes, Failed to read takeover status comments on stderr |
| h17 (fail-closed site) | reverted | dropped | 1 read, blind write, failure never surfaced |
| h20 (routine site) | reverted | dropped | exit 0 (green) + duplicate ⛔ blocked comment posted beside the stale ✅ — the exact two-status state the function exists to prevent |
| h18 | present | dropped, HTTP-502 shape | exit 1, 3 reads, 0 writes (jq chokes on the stdout error body regardless — the HTTP half never depended on pipefail; pinned so it can't be "simplified" into the guard) |
Witness: evidence/03-mutation-matrix.png (suite-level kills) and the A/B capture.
Reviewer Test Plan walkthrough
| Plan step | Result |
|---|---|
| Takeover-labelled fork remains eligible after a transient metadata or permission failure | ✅ cells 1 and 15 (2 calls each, admitted) |
| Malformed or repeatedly unavailable metadata/permission stops after three attempts | ✅ cells 2–4 (3 bounded calls, stable reason, exit 1) |
| Terminal permission blocker updates the existing status with reason + run link | ✅ cells 5–7, 14 (PATCH to id 123, reason + GITHUB_SERVER_URL-resolved link + remedy) |
| Status comment unreadable/unupdatable ⇒ scan fails visibly | ✅ cells 8–9 (exit 1, ::error::/warning on stderr, nothing written) |
Mutation matrix (suite at committed head, single-hunk mutants)
| build | suite | killed by |
|---|---|---|
| control (unmutated head) | 120/120 green | — |
M1: local set -o pipefail; removed |
119/120 | recovers transient forced-target reads… — behavioral (expected +0 to be 1 on the no-pipefail case), not a collection error |
M2: do_review conjunct removed from scan group |
118/120 | the two lock tests (serializes…, keeps the forced-scan lock…) — matches the commit's "fails 2 tests" claim |
M3: // "" null-body guard removed |
119/120 | same test, behavioral mismatch (jq rc 5 → reporter status 1) |
No survivors among the guards the delta introduces. Concurrency group additionally verified statically: predicate carries the do_review == 'true' conjunct and falls back to github.run_id; prefix byte-equal to review-address's qwen-pr-head-write-; base had no group at all; actionlint (liveness-proven with a planted expression error) type-checks the group expression against route's declared outputs.
Findings
1. Suggestion (non-blocking, new) — terminal meta shapes are retried as transient and leave the stale status
read_forced_pr_meta validates response structure and treats any mismatch as a retryable lookup failure. A terminal shape — e.g. "author": null (deleted account; probed in cell h19) — burns all 3 pr view calls, then exits 1 with metadata_fetch_failed, and that path never invokes report_forced_takeover_blocked, so a previous success status stays visible: the issue-#8409 symptom in a rarer shape, and the same class R7-1 fixed for permissions (definitive answers answered terminally). Fail-closed and bounded, so non-blocking; base handles the same shape as a silent green reject (neither arm updates the status). A one-line rationale (structural mismatch = "cannot attribute the PR, therefore cannot safely report") or routing a parseable-but-unmanaged shape to the classifier instead of the retry loop would close it.
2. Suggestion (non-blocking, carried) — blocked-status allowlist boundary
Unchanged from the previous round and re-measured: not_open / wrong_base / skip_label / unmanaged_author exit the gate green with no status update, leaving any prior success comment. Defensible (terminal states never address feedback); a comment naming the boundary, or a test pinning the exclusion, would stop the next reader filing it as a bug.
Not covered
- Per-commit attribution: shallow checkout (
rev-list HEAD^1..HEAD^2= 1 vs 12 commits in the snapshot); aggregateHEAD^1..HEADverified, individual commits (R7-1, R9-1, d8d66b5 rationales) not exercised separately. - yamllint:
scripts/lint.js --setupfails onpip3: Permission deniedin this container (same as previous round — environmental); YAML covered by js-yaml parse (extraction), actionlint (clean, liveness-proven), prettier (clean). - Scheduled-scan inspection surface past the forced block: admit cells cut at
CANDIDATES="${FORCED_PR}"(identical cut both arms); the newfleet_rowblocked rows in the candidate loop are pinned textually by the suite and covered at function level, not end-to-end. - Live GitHub API semantics: the stub encodes documented answers; real-world drift (new permission levels, schema changes) not covered — see Finding 1 for the schema-drift consequence.
- Concurrency group: static + suite pins + actionlint expression typing, not an Actions expression evaluator.
- Replay calibration: no production-emitted artifact of this step exists here; the bash replay is uncalibrated (a real forced-dispatch run log would calibrate it). The A/B is calibrated against the base build instead.
- Event-route admission and the
review-addresswrite-safety recheck are unchanged by the PR and were not exercised; Windows/macOS lanes N/A.
Methodology
Environment: node:22-bookworm CI container; merge-ref checkout at depth 2 (HEAD merge 8087b69e2, HEAD^1 base 037b4d9c0, HEAD^2 PR head dfc6ec551); npm ci + build pre-run. Harnesses in tmp/pr8410-verify-20260806-231519/: extract-scan-step.mjs (js-yaml verbatim extraction, gate mode cuts at the forced-block terminus), stub-template/gh (scenario-driven, logs every call + PATCH/comment bodies), run-cells.sh (18 head + 9 base cells under bash --noprofile --norc -eo pipefail; delta cells under -e), func-probes.sh (classifier matrix + reporter pass-throughs), assert.mjs (all 121 scripted checks incl. gate/mutation log parsing; writes assertions.json), mutant runs in a scratch worktree (M1–M3, restored and removed). Raw per-cell stdout/stderr/call logs under head/cells/ and base/cells/; suite and mutant logs at the artifact root. Captures via scripts/verify-capture.mjs. Gates: vitest (head 120/120, base 115/115, full test:scripts 851 passed with the single pre-existing environmental install-script file failure reproduced identically at base), ESLint + prettier (both liveness-proven), bash -n on every extracted block, actionlint + shellcheck (pinned binaries, liveness-proven).
Evidence images
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
|
Released in v0.21.8. |
3 participants
What this PR does
This change makes forced AutoFix review admission explicit and fail-closed. Live pull-request metadata and fork-author permissions are validated and retried up to three times, each rejection receives a stable reason code, and takeover pull requests update their existing AutoFix status to a bilingual blocked state when admission cannot safely continue. Scheduled fork discovery reuses the same permission reader.
The regression coverage executes the admission classifier, transient and terminal lookup paths, the GitHub status-comment update, its failure path, and the complete review-scan shell syntax.
Why it's needed
A trusted Critical review on a managed takeover pull request could route into AutoFix but then be silently rejected by the forced scan after a transient metadata or permission lookup. The prior success status remained visible even though the feedback was not addressed. This keeps
review-scanauthoritative while making transient failures recoverable and terminal blockers visible without advancing the feedback watermark.Reviewer Test Plan
How to verify
Confirm that a takeover-labelled fork remains eligible when live metadata and write permission are available after a transient API failure. Confirm that malformed or repeatedly unavailable metadata and permission responses stop admission after three attempts. For an identifiable takeover PR with a terminal permission blocker, confirm that the existing AutoFix status comment is updated with the specific reason and run link; if that comment cannot be read or updated, the scan must fail visibly.
Evidence (Before & After)
Before: forced-scan lookup failures collapsed into one aggregate rejection and could leave the previous successful takeover status visible.
After: admission emits stable reason codes, retries bounded live reads, and behaviorally verifies the blocked-status PATCH and its failure semantics.
Tested on
Environment (optional)
Node.js 22 workspace. The focused new regression passes independently; all 110 workflow assertions passed in the complete local run, although the local Vitest worker reported an RPC timeout after publishing the passing results. Type checking, formatting, shell syntax, and diff checks pass.
Risk & Scope
review-addresswrite-safety recheck are intentionally unchanged; hosted GitHub Actions CI will provide Linux coverage.Linked Issues
Fixes #8409
中文说明
本 PR 的改动
本次修改让强制触发的 AutoFix 评审准入具备明确原因并保持写入侧 fail-closed。PR 实时元数据和 fork 作者权限都会校验响应结构并最多重试三次；每种拒绝都有稳定原因码；takeover PR 无法安全继续时，会把现有 AutoFix 状态更新为中英双语的 blocked 状态。定时扫描 fork 候选也复用同一权限读取逻辑。
回归测试会实际执行准入分类、临时和终态查询失败、GitHub 状态评论更新及其失败路径，并检查完整 review-scan shell 语法。
为什么需要
受信任的 Critical 评审可以把已接管 PR 路由进 AutoFix，但强制扫描可能因为一次临时元数据或权限查询失败而静默拒绝，旧的成功状态仍然保留，看起来像反馈已经被处理。本修改继续以
review-scan作为权威实时准入，同时让临时错误可恢复、终态阻塞可见，并且不推进反馈水位。Reviewer Test Plan
如何验证
确认带 takeover 标签的 fork PR 在元数据和 write 权限首次查询临时失败、后续成功时仍可准入。确认元数据或权限响应畸形、连续不可用时会在三次后停止。对于元数据可识别但权限查询最终失败的 takeover PR，确认现有 AutoFix 状态会更新为包含明确原因和运行链接的 blocked 状态；如果状态评论无法读取或更新，扫描必须明确失败。
证据（修改前后）
修改前：强制扫描的查询失败会合并成一个笼统拒绝，并可能继续显示上一轮成功的 takeover 状态。
修改后：准入输出稳定原因码，实时查询做有界重试，并通过行为测试验证 blocked 状态 PATCH 及其失败语义。
测试平台
环境（可选）
Node.js 22 工作区。新增聚焦回归可独立通过；完整本地运行的 110 个工作流断言全部通过，但本地 Vitest worker 在发布通过结果后报告了一次 RPC 超时。类型检查、格式检查、shell 语法和 diff 检查均通过。
风险与范围
review-address独立的最终写安全复核保持不变；由 GitHub Actions CI 提供 Linux 覆盖。关联 Issue
Fixes #8409