Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 203 additions & 0 deletions .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ concurrency:
format('qwen-pr-review-run-{0}', github.run_id) }}
cancel-in-progress: "${{ github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') }}"

env:
# Dedup marker for the review-failure fallback comments. The in-job step
# and the fallback-comment job both build their body from it, and the
# cross-job dedup matches it — the sites must stay byte-identical or the
# dedup silently posts duplicates, so the literal is defined once here.
FALLBACK_MARKER: '<!-- qwen-review-fallback -->'

jobs:
precheck-pr:
if: |-
Expand Down Expand Up @@ -412,6 +419,55 @@ jobs:
pull-requests: 'write'
issues: 'write'
steps:
# The runner worker dies in FinalizeJob with EACCES when it loses write
# access to its own directories (observed: '/home/github-runner' no
# longer creatable), taking the whole job down with no fallback comment
# and no cleanup — see the PR #8894 incident. The known trigger on this
# shared pool is a prior containerised job running as root. Probe every
# directory the review must create files in, repair single-directory
# ownership with the same sudo pattern as 'Restore workspace ownership',
# and fail fast with a clear message when repair is impossible — cheaper
# than burning hours of review budget to die at finalize. Only catches
# corruption already present at job start; mid-run corruption is covered
# by the fallback-comment job instead.
- name: 'Verify runner directory health'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The only self-healing repair for runner-pool corruption lives in review-pr, the last job of the chain — downstream of the self-hosted jobs (authorize / review-config) that the same corruption kills first, so the repair is usually unreachable while the failure persists. — Failure scenario: an ecs-qwen runner with a root-owned $HOME (the exact PR #8894 trigger) kills authorize/review-config (verified: authorize runs self-hosted for same-repo heads; review-config has no needs and schedules first); review-pr is skipped without starting, so this repair never runs and the corrupted runner stays in the pool to kill the retry. The diff's own rationale concedes the trigger "can kill the chain's earlier self-hosted jobs first" — it covers that with a comment, not with repair.

Suggested fix: run the same probe (at minimum probe+repair) in the other self-hosted jobs of this workflow (authorize, review-config), or extract it as a shared preflight for every ecs-qwen job in this file.

中文说明

针对 runner 池损坏的唯一自愈修复位于链条最末的 review-pr——处在同样会被该损坏杀死的自托管 job(authorize/review-config)下游,因此故障持续期间修复通常不可达。— 故障场景:$HOME 属主为 root 的 ecs-qwen runner(正是 PR #8894 的触发条件)杀死 authorize/review-config(已核实:同仓库 head 时 authorize 跑在自托管上;review-configneeds、最先被调度);review-pr 未启动即被跳过,本修复从不执行,损坏的 runner 留在池中继续杀死重试。diff 自身的理由也承认触发条件"可能先杀死链条上更早的自托管 job"——但只以注释覆盖,未以修复覆盖。

建议修复:在本 workflow 其他自托管 job(authorizereview-config)中运行同样的探测(至少探测+修复),或抽取为本文件所有 ecs-qwen job 共享的前置检查。

— qwen3.8-max via Qwen Code /review (v0.21.12)

run: |-
set -uo pipefail
RUNNER_UID="$(id -u)"
RUNNER_GID="$(id -g)"
# Three levels above the workspace (_work/owner/repo) is the runner
# root, whose _diag/pages dir is what FinalizeJob creates in.
RUNNER_ROOT="$(cd "$GITHUB_WORKSPACE/../../.." && pwd)"
dirs=("$HOME" "${RUNNER_TEMP:?}" "$RUNNER_ROOT")
# A writable runner root does not prove an existing _diag writable
# (ownership is per-directory), so probe it too; when absent, it is
# created by FinalizeJob, which only needs the runner root.
if [ -d "$RUNNER_ROOT/_diag" ]; then
dirs+=("$RUNNER_ROOT/_diag")
fi
status=0
for dir in "${dirs[@]}"; do
probe="$(mktemp -u "$dir/.qwen-health-XXXXXX")"
if touch "$probe" 2>/dev/null; then
rm -f "$probe"
continue
fi
echo "::warning::no write access to $dir; attempting single-directory repair"
sudo -n chown "$RUNNER_UID:$RUNNER_GID" "$dir" 2>/dev/null || true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The repair branch's chown is unverified by any test: the stub sudo no-ops chown (it only honours chmod) and no textual pin mentions it, so deleting the chown line leaves the whole suite green (verified by probe). — Failure scenario: on the step's own incident trigger (root-owned directories), chmod u+rwx without chown grants write to root, not the runner user, so repair can never succeed in exactly the case the step was written for — every incident degrades to fail-fast requiring manual runner repair instead of self-healing, while CI stays green. The harness cannot create root-owned fixtures without root, but the stub could still observe the invocation.

Suggested fix: have the stub sudo append "$@" to a log file (like the gh stub's $CALLS) and assert in the repair test that a chown <uid>:<gid> <broken-dir> invocation occurred.

中文说明

修复分支的 chown 未被任何测试验证:stub sudochown 不做任何事(只执行 chmod),也没有文本钉住它,因此删除 chown 行后整个套件仍为绿色(探针已验证)。— 故障场景:在该步骤自身针对的事故触发条件(root 属主目录)下,缺少 chownchmod u+rwx 把写权限给了 root 而非 runner 用户,修复在该步骤正是为之编写的场景中永远无法成功——每起事故都退化为需人工修复 runner 的快速失败而非自愈,CI 却保持绿色。框架在无 root 时无法构造 root 属主夹具,但 stub 仍可记录该调用。

建议修复:让 stub sudo"$@" 追加到日志文件(如 gh stub 的 $CALLS),并在修复测试中断言出现过 chown <uid>:<gid> <broken-dir> 调用。

— qwen3.8-max via Qwen Code /review (v0.21.12)

sudo -n chmod u+rwx "$dir" 2>/dev/null || true
if touch "$probe" 2>/dev/null; then
Comment on lines +456 to +458

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-3: The health probe's repair-vs-fail-fast decision (probe → repair → re-probe → status) is pinned only by static toContain/toMatch assertions; no test executes this step's real bash through the repair branches. — Failure scenario: named surviving mutants — moving status=1 to right after the first failed touch ships a false fail-fast (a repairable directory aborts review-pr on every run until manual runner repair); deleting the second touch re-check ships a missed fail-fast (an unusable directory is reported "repaired" and the job dies in FinalizeJob — the PR #8894 incident this PR exists to prevent). Witness: probe ran the step's real bash with a stub sudo against a chmod-555 directory — mutant (a): REPAIRABLE → status:1 "failing fast" (original: status:0 "repaired"); mutant (b): UNREPAIRABLE → status:0 "repaired" (original: status:1); the suite stays 135/135 green under both mutants.

Suggested fix: add an executed-shape test mirroring runFallbackStep's stub harness — stub sudo, a read-only tmp dir standing in for $HOME, a fake GITHUB_WORKSPACE nested three deep — covering healthy (exit 0) / repairable (exit 0 + "repaired") / unrepairable (exit 1 + ::error::) / _diag-absent.

中文说明

健康探测的"修复还是快速失败"决策(探测 → 修复 → 复探 → status)目前只被静态 toContain/toMatch 断言钉住,没有任何测试真正执行这段 bash 的修复分支。故障场景:已点名的可存活变异——把 status=1 挪到首次 touch 失败后会带来误报式快速失败(可修复的目录导致 review-pr 每次运行都中止,直到人工修 runner);删掉第二次 touch 复探则会漏掉快速失败(不可用目录被报告为"已修复",job 最终死在 FinalizeJob——正是本 PR 要防的 PR #8894 事故)。证据:探针用 stub sudo 对 chmod-555 目录真实执行了该步 bash——变异 (a):可修复 → status:1(原版 status:0);变异 (b):不可修复 → status:0(原版 status:1);两种变异下测试套件均为 135/135 全绿。建议修复:仿照 runFallbackStep 的 stub 框架补一个执行型测试,覆盖健康/可修复/不可修复/无 _diag 四种路径。

— qwen3.8-max via Qwen Code /review (v0.21.12)

rm -f "$probe"
echo "repaired write access to $dir"
else
echo "::error::runner directory still unusable after repair: $dir"
status=1
fi
done
if [ "$status" != 0 ]; then
echo "::error::runner directories unhealthy; failing fast instead of dying at job finalize"
fi
exit "$status"

# Self-hosted runners reuse the workspace; a prior containerised job can
# leave root-owned, read-only files anywhere in it. Restore ownership and
# write permission unconditionally before checkout — probing only .qwen
Expand Down Expand Up @@ -1624,6 +1680,27 @@ jobs:
echo "Skipping fallback comment: PR #${PR_NUMBER} moved from ${EXPECTED_HEAD_SHA} to ${current_head}." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi
# Re-runs of failed jobs keep the same run id: a prior attempt that
# died before reaching this step already got a fallback comment for
# this run from the fallback-comment job. Dedup on the marker plus
# this run's URL exactly as that job does; a FAILED lookup defers to
# it (it retries and fails closed) instead of risking a duplicate —
# posting on a failed listing is how a transient 5xx mints one.
bot_login="$(gh api user --jq '.login' 2>/dev/null)" || bot_login=""
fallback_bodies=""
if [ -n "$bot_login" ] \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The in-job dedup's error path "comments listing fails while gh api user succeeds" is never executed — the only in-job scenarios run are already, default, and lookup_fail; the fallback-job twin (comments_lookup_fail) IS tested. — Failure scenario: verified surviving mutant — decoupling the failed lookup from the condition (hoisted assignment + || true) leaves the suite green; the condition then succeeds with an empty body on a transient 5xx and the step posts without dedup, minting a permanent duplicate on the exact re-run case this block was written for (attempt 1 died, the fallback job already posted for this run id, attempt 2 hits a transient listing failure).

Suggested fix: add one scenario — runFallbackStep('comments_lookup_fail', { useInJobStep: true }) expecting status 0, posted '', and summary containing deferring to the fallback-comment job.

中文说明

in-job 去重的错误路径"gh api user 成功但评论列表失败"从未被执行——in-job 只跑 alreadydefaultlookup_fail 三个场景;其 fallback-job 孪生路径(comments_lookup_fail)却有测试。— 故障场景:已验证存活的变异体——把失败的查询与条件解耦(提升赋值 + || true)后套件仍全绿;瞬时 5xx 时条件以空 body 成功,步骤未经去重就发布,在正是本块为之编写的重跑场景上铸成永久重复(尝试 1 死亡、fallback job 已就该 run id 发布、尝试 2 遇到瞬时列表失败)。

建议修复:新增一个场景——runFallbackStep('comments_lookup_fail', { useInJobStep: true }),期望 status 0、posted 为 ''、summary 含 deferring to the fallback-comment job

— qwen3.8-max via Qwen Code /review (v0.21.12)

&& fallback_bodies="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json comments \
--jq ".comments[] | select(.author.login == \"$bot_login\") | select(.body | contains(\"$FALLBACK_MARKER\")) | .body")"; then
case "$fallback_bodies" in
*"actions/runs/${GITHUB_RUN_ID})"*)
echo "A fallback comment for this run already exists; skipping." >> "$GITHUB_STEP_SUMMARY"
exit 0
;;
esac
else
echo "Fallback comment dedup lookup failed; deferring to the fallback-comment job." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi
MAX_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"
if [ "$FAILURE_KIND" = "timeout" ]; then
if [ "$TIMEOUT_MINUTES" -lt "$MAX_TIMEOUT_MINUTES" ]; then
Expand All @@ -1638,6 +1715,10 @@ jobs:
else
body="**Qwen Code review did not complete successfully.** ${FAILURE_REASON} A transient error is retried automatically; if you are seeing this, retry with \`@qwen-code /review\`. See [workflow logs](${RUN_URL})."
fi
# Blank line after the marker or the prose renders as raw source —
# same HTML-block quirk as the ack marker. The fallback-comment job
# dedupes on this marker plus this run's URL.
body="$(printf '%s\n\n%s' "$FALLBACK_MARKER" "$body")"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The cross-job dedup is one-directional: the new fallback-comment job lists existing marker comments before posting, but this in-job step has no dedup check — so a re-run after a fallback post mints a duplicate failure comment for the same run, contrary to the job comment's claim that "the same check dedupes re-runs, which keep the same run id". (In-job→in-job re-run duplicates pre-date this PR; the fallback→in-job pair is newly introduced by this diff.) — Failure scenario: attempt 1 — the self-hosted runner dies mid-review (the incident class this PR targets); this step never runs; the fallback-comment job posts the marker-headed comment linking actions/runs/<id>. The maintainer re-runs failed jobs (same run id). Attempt 2 completes but the review step fails for an ordinary reason (timeout/quota/generic): this step's if: failure() fires and posts a second marker-headed comment linking the identical run URL, because nothing on its path lists existing comments. Probe-verified via extract-step (the step posts without any comment listing; a fail-open lookup flips it). Suggested fix: apply the same author-scoped marker+run-URL lookup before posting here (fail-open — posting wins over silence, the fallback job still catches a true miss), or correct the job comment claiming re-run dedup.

中文说明

跨 job 的去重是单向的:新的 fallback-comment job 在发布前会列出现有 marker 评论,但 job 内的这个步骤没有去重检查——于是在兜底评论发布之后重跑,会为同一个 run 制造重复的失败评论,与 job 注释中"the same check dedupes re-runs, which keep the same run id"(同一检查会为保留相同 run id 的重跑去重)的说法矛盾。(job 内→job 内的重跑重复在本 PR 之前就存在;fallback→job 内的这一对是本 diff 新引入的。)— 故障场景:第 1 次尝试——自托管 runner 在 review 中途死亡(本 PR 针对的事故类);此步骤从未运行;fallback-comment job 发布了带 marker、链接 actions/runs/<id> 的评论。维护者重跑失败的 jobs(run id 不变)。第 2 次尝试完成但 review 步骤因普通原因失败(超时/配额/通用):此步骤的 if: failure() 触发,由于其路径上没有任何东西列出已有评论,又发布了第二条带 marker、链接同一 run URL 的评论。已用 extract-step 探针验证(该步骤发布时不做任何评论列出;加上 fail-open 查找后可翻转)。建议修复:在此处发布前应用同样的按作者过滤的 marker+run-URL 查找(fail-open——发布优先于沉默,fallback job 仍会兜住真正的遗漏),或修正声称重跑去重的 job 注释。

— qwen3.8-max via Qwen Code /review (v0.21.12)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — this is a real duplication path. Deferred to the next round by this round's batch bound (eight findings implemented, Critical first). Planned fix: a fail-open, author-scoped marker + run-URL lookup before the in-job gh pr comment, evaluated together with the R2-3 consolidation question — adding a third inline copy of the lookup protocol this round would deepen exactly the drift R2-3 flags. The job comment's re-run-dedup claim is accurate for the fallback job's own check; the gap is the fallback→in-job pair, which the next round addresses.

中文说明

已确认——这是一条真实的重复路径。受本轮批次上限(实现了 8 个发现、Critical 优先)推迟到下一轮。计划中的修复:在 job 内 gh pr comment 之前加 fail-open、按作者过滤的 marker+run-URL 查找,并与 R2-3 的合并问题一并评估——本轮再加第三份内联查找拷贝会加深 R2-3 所指出的漂移。job 注释中关于重跑去重的说法对 fallback job 自身的检查是准确的;缺口在 fallback→job 内这一对,下一轮处理。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The ordering invariant that this step prepends FALLBACK_MARKER to the body BEFORE gh pr comment is unpinned — the tests assert the printf line exists (toContain) but not that it precedes the post, so a regression that strips the marker from the posted in-job comment ships green. — Failure scenario: a future edit moves the printf line below the gh pr comment block (mutating a dead variable); the suite passes 131/131 (verified mutant). At runtime the in-job comment then posts without the marker, so the fallback job's author+marker dedup filter never sees it — and every run where review-pr fails after this step already posted mints a duplicate "review did not complete" comment, the duplication the marker was introduced to prevent. Suggested fix: pin the order (expect(inJobStep.run.indexOf(printf)).toBeLessThan(inJobStep.run.indexOf('gh pr comment')) — an established pattern elsewhere in this file), or execute this step's script the way runFallbackStep executes the fallback job's and assert the posted body starts with the marker.

中文说明

该步骤在 gh pr comment 之前把 FALLBACK_MARKER 前置到正文的这一顺序不变量未被钉住——测试只断言 printf 行存在(toContain),未断言它先于发布,因此一个把 marker 从所发布的 job 内评论中去掉的回归可以绿着上线。— 故障场景:未来某次编辑把 printf 行移到 gh pr comment 块之下(变异一个死变量);套件 131/131 通过(变异体已验证)。运行时 job 内评论随后会不带 marker 发布,fallback job 的按作者+marker 去重筛选永远看不到它——于是每个 review-pr 在此步骤已发布之后才失败的 run 都会制造重复的 "review did not complete" 评论,正是 marker 被引入要防止的重复。建议修复:钉住顺序(expect(inJobStep.run.indexOf(printf)).toBeLessThan(inJobStep.run.indexOf('gh pr comment'))——本文件其他地方已有的既定模式),或像 runFallbackStep 执行 fallback job 脚本那样执行此步骤的脚本,并断言发布的正文以 marker 开头。

— qwen3.8-max via Qwen Code /review (v0.21.12)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — deferred to the next round by this round's batch bound. The fix is the suggested one-line ordering assertion (indexOf(printf) < indexOf('gh pr comment') on the in-job step, an established pattern in this suite) and ships with the R2-9 follow-up.

中文说明

已确认——受本轮批次上限推迟到下一轮。修复即所建议的一行顺序断言(对 job 内步骤断言 indexOf(printf) < indexOf('gh pr comment'),本套件中已有的既定模式),随 R2-9 的后续一并提交。

Comment on lines +1720 to +1721

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R2-9 (carried from round 2, ruled still standing at this commit): the cross-job dedup is one-directional — the new fallback-comment job lists existing marker comments before posting, but this in-job step posts with no marker/run-URL lookup (its body contains exactly one gh read, --json state,headRefOid, before gh pr comment). The fallback job's header claims "the same check dedupes re-runs", but re-runs re-fire THIS step (if: failure()), which has no check. — Failure scenario: run N dies mid-review (runner death; this step never runs) → fallback-comment posts comment #1 (marker + run N URL) → "Re-run failed jobs" (same run id) → review-pr fails again at step level → this step posts comment #2 with the same run URL → two near-identical comments for one run. Witness: not run — GitHub Actions re-run orchestration is the platform's; verdict rests on a full read of this step's body at HEAD. Acknowledged and deferred by the author automation in round 2; re-reporting because the mechanism still stands.

Suggested fix: give this step the sibling's guard before gh pr comment — the same bot-login-scoped, marker + actions/runs/${GITHUB_RUN_ID}) lookup (bounded retry optional here; on a failed lookup it can skip and let fallback-comment handle it).

中文说明

R2-9(第 2 轮携带,本轮代码核实仍然成立):跨 job 去重是单向的——新的 fallback-comment job 在发布前会列出已有标记评论,但 job 内的这一步发布前没有任何标记/run URL 查询(正文在 gh pr comment 之前只有一处 --json state,headRefOid 读取)。兜底 job 的头注释声称"同一检查为重跑去重",但重跑重新触发的是这一步(if: failure()),而它没有检查。故障场景:run N 在 review 中途死亡(runner 崩溃,此步未运行)→ fallback-comment 发出评论 #1(标记 + run N URL)→ 点击"Re-run failed jobs"(run id 不变)→ review-pr 再次在步骤级失败 → 此步带着同一 run URL 发出评论 #2 → 同一 run 出现两条几乎相同的评论。证据:未运行——重跑编排属于 GitHub Actions 平台;结论基于对 HEAD 处该步正文的完整阅读。第 2 轮作者自动化已确认并延期处理,因机制仍在,本轮继续报告。建议修复:在 gh pr comment 前给此步加上同款守卫(机器人登录作用域的标记 + run URL 查询;查询失败时可跳过、交给 fallback-comment 兜底)。

— qwen3.8-max via Qwen Code /review (v0.21.12)

gh pr comment "$PR_NUMBER" \
Comment on lines +1720 to 1722

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R2-11 (carried from round 2, ruled still standing at this commit): the ordering invariant that this step prepends $FALLBACK_MARKER to the body BEFORE gh pr comment is pinned only by existence assertions (toContain of the printf line) — grep across every test file at HEAD finds no ordering assertion for this step, despite the round-2 reply stating the one-line ordering assertion would ship. The fallback job's side IS pinned (the executed test asserts posted.startsWith(marker)), but this in-job step is not executed by the harness. — Failure scenario: a regression moving the printf below gh pr comment keeps every test green (toContain still matches) and ships fallback comments without the marker on the in-job path — breaking the cross-job and re-run dedup that keys on it. Witness: grep at HEAD — FALLBACK_MARKER appears only in existence assertions and env injection; no indexOf/toBeLessThan ordering assertion exists anywhere.

Suggested fix: add the one-line ordering assertion the round-2 thread already proposed — an established pattern in this suite:

expect(
  inJobStep.run.indexOf(`body="$(printf '%s\\n\\n%s' "$FALLBACK_MARKER" "$body")"`),
).toBeLessThan(inJobStep.run.indexOf('gh pr comment'));
中文说明

R2-11(第 2 轮携带,本轮代码核实仍然成立):这一步在 gh pr comment 之前把 $FALLBACK_MARKER 前置到正文的次序不变量,目前只被存在性断言(对 printf 行的 toContain)钉住——在 HEAD 处 grep 全部测试文件都找不到针对该步的次序断言,尽管第 2 轮回复声称这条一行次序断言会随本轮推送。兜底 job 一侧已有钉住(执行型测试断言 posted.startsWith(marker)),但 job 内这一步并未被执行型框架覆盖。故障场景:把 printf 挪到 gh pr comment 之后的回归能让所有测试保持绿色(toContain 依然匹配),并在 job 内路径发出没有标记的兜底评论——破坏以标记为键的跨 job 与重跑去重。证据:HEAD 处 grep——FALLBACK_MARKER 只出现在存在性断言与 env 注入中,任何地方都没有 indexOf/toBeLessThan 次序断言。建议修复:补上第 2 轮线程已提出的一行次序断言(本套件的既有模式,见代码块)。

— qwen3.8-max via Qwen Code /review (v0.21.12)

--repo "$GITHUB_REPOSITORY" \
--body "$body"
Expand Down Expand Up @@ -1692,6 +1773,128 @@ jobs:
rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true
echo "review worktrees cleaned"

# A review job that dies abnormally — runner crash, host loss, or the
# FinalizeJob EACCES from the PR #8894 incident — never reaches its in-job
# 'Post fallback comment on failure' step, leaving the PR with no review and
# no explanation. This dependent job runs on an ephemeral hosted runner, so
# it survives whatever killed the review job, and posts the retry guidance
# itself. Every upstream job whose failure marks review-pr 'skipped' opens
# the gate — the incident's trigger can kill the chain's earlier
# self-hosted jobs first (authorize / review-config), and a transient API
# failure can kill the hosted ones (precheck-pr / delay-automatic-review) —
# a skipped review is just as unexplained as a dead one. It skips when a
# fallback comment for this run already exists — matched by the
# qwen-review-fallback marker plus this run's URL, since the ack comment
# also links the run and must not suppress this one; the same check dedupes
# re-runs, which keep the same run id. A review-pr that dies to its own
# job-level timeout is auto-CANCELLED by GitHub — result 'cancelled' and
# failure() false — which opens neither a failure-only gate nor the in-job
# step, so the gate admits 'cancelled' too; a run-level cancel cancels this
# queued job with it, so a live gate evaluation seeing 'cancelled' is
# overwhelmingly the timeout case, and the residual manual single-job
# cancel just gets a benign retry-guidance comment. The PR number comes
# from the event payload, not the dead job's outputs, which do not survive
# a crash.
fallback-comment:
needs:
[
'precheck-pr',
'review-config',
'authorize',
'delay-automatic-review',
'review-pr',
]
if: |-
always() &&
(needs.review-pr.result == 'failure' ||
needs.review-pr.result == 'cancelled' ||
needs.authorize.result == 'failure' ||
Comment on lines +1809 to +1811

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-6: The gate disjunction admits only result == 'failure'. A review-pr that exceeds its job-level timeout-minutes (line 415, default 360) is auto-cancelled by GitHub with result 'cancelled' — which opens neither this gate nor the in-job step (failure() is false for a cancelled job). Both defenses miss on the timeout path. The workflow proves the authors hold this model elsewhere: Clean review worktrees uses if: 'always()' precisely because "a cancelled or timed-out review may not reach the CLI's process cleanup" (line 1705). — Failure scenario: a hang outside the CLI's own timeout enforcement (eight steps precede Run review and only Install capture tools has its own timeout) runs into the 360-min cap → review-pr result 'cancelled' → no in-job comment, gate closed → the PR is left with no review and no explanation — the same hole as the PR #8894 incident. Frequency is low (the cap sits far above the review budget by design), but the mechanism is deterministic once triggered. Witness: not run — GitHub Actions job-timeout semantics (conclusion cancelled, failure() false) only execute on GitHub's runtime; the quoted lines 415, 1653-1657, 1779-1784 and 1705-1711 carry the trace.

Suggested fix: add needs.review-pr.result == 'cancelled' to the disjunction — a run-level concurrency cancel cancels the whole run including the queued fallback job, so a fallback job that lives to evaluate the gate and sees 'cancelled' is overwhelmingly the job-timeout case (residual edge: manual single-job cancel, whose outcome is a benign retry-guidance comment) — and pin the chosen semantics in the gate test; or document in the gate comment that cancelled/timed-out runs are intentionally left silent.

中文说明

gate 的析取只接受 result == 'failure'review-pr 超过 job 级 timeout-minutes(415 行,默认 360)时会被 GitHub 自动取消、结果为 'cancelled'——既打不开这个 gate,也触发不了 job 内步骤(被取消的 job 上 failure() 为假)。超时路径上两层防御全部落空。workflow 在别处证明作者清楚这一模型:Clean review worktreesif: 'always()',理由正是"被取消/超时的 review 可能走不到 CLI 的进程清理"(1705 行)。故障场景:CLI 自身超时管辖之外的挂起(Run review 之前有八个步骤,只有 Install capture tools 有自己的超时)撞上 360 分钟上限 → review-pr 结果 'cancelled' → 无 job 内评论、gate 关闭 → PR 既无 review 也无解释——与 PR #8894 事故相同的空洞。频率低(上限按设计远高于 review 预算),但一旦触发机制是确定的。证据:未运行——job 超时语义只在 GitHub 运行时生效;引用 415、1653-1657、1779-1784、1705-1711 行为证。建议修复:在析取中加入 needs.review-pr.result == 'cancelled'(run 级并发取消会连同排队中的兜底 job 一起取消,因此活着评估 gate 并看到 'cancelled' 的兜底 job 几乎必然是 job 超时;残余边界是手动取消单个 job,其后果只是一条良性的重试指引评论),并在 gate 测试中钉住所选语义;或在 gate 注释中声明有意对被取消/超时的运行保持沉默。

— qwen3.8-max via Qwen Code /review (v0.21.12)

needs.review-config.result == 'failure' ||
needs.delay-automatic-review.result == 'failure' ||
needs.precheck-pr.result == 'failure') &&
github.event.inputs.command != 'resolve' &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The /resolve exclusion only covers workflow_dispatch runs (github.event.inputs.command != 'resolve'), but /resolve is also a first-class issue_comment command — authorize.if matches startsWith(github.event.comment.body, '@qwen-code /resolve') — and on issue_comment events github.event.inputs is empty, so this exclusion never fires there. — Failure scenario: a maintainer comments @qwen-code /resolve on a same-repo PR; authorize runs on the self-hosted ECS pool and fails (a permission-API 5xx under set -euo pipefail, or the incident-class runner corruption this PR hardens against). review-pr is skipped by design (its if has no /resolve branch), the gate opens via needs.authorize.result == 'failure', and the job posts "Qwen Code review did not complete successfully … retry with @qwen-code /review" on a run where no review was ever attempted — misdiagnosing a resolve run and recommending the wrong command. Suggested fix: mirror authorize's /resolve predicate for comment events, e.g. add !(github.event_name == 'issue_comment' && startsWith(github.event.comment.body, '@qwen-code /resolve')) && to the gate, and extend the gate test's resolve case to the comment-driven path.

中文说明

/resolve 排除项只覆盖了 workflow_dispatch 运行(github.event.inputs.command != 'resolve'),但 /resolve 也是一等的 issue_comment 命令——authorize.if 匹配 startsWith(github.event.comment.body, '@qwen-code /resolve')——而在 issue_comment 事件中 github.event.inputs 为空,该排除项在那里永远不会生效。— 故障场景:维护者在同仓库 PR 上评论 @qwen-code /resolveauthorize 在自托管 ECS 池上运行并失败(set -euo pipefail 下的权限 API 5xx,或本 PR 所要防御的事故类 runner 损坏)。review-pr 按设计被跳过(其 if 没有 /resolve 分支),gate 经 needs.authorize.result == 'failure' 打开,在一个从未尝试过 review 的运行上发布 "Qwen Code review did not complete successfully … retry with @qwen-code /review"——把 resolve 运行误诊为 review 失败,并推荐了错误的命令。建议修复:在 gate 中为评论事件镜像 authorize 的 /resolve 谓词,例如添加 !(github.event_name == 'issue_comment' && startsWith(github.event.comment.body, '@qwen-code /resolve')) &&,并在 gate 测试中把 resolve 用例扩展到评论驱动路径。

— qwen3.8-max via Qwen Code /review (v0.21.12)

!(github.event_name == 'issue_comment' &&
startsWith(github.event.comment.body, '@qwen-code /resolve')) &&
github.repository == 'QwenLM/qwen-code' &&
(github.event_name != 'workflow_dispatch' ||
github.event.inputs.review_mode == 'comment')
runs-on: 'ubuntu-latest'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This job's load-bearing placement invariant — it must run on an ephemeral hosted runner, never the self-hosted ECS pool whose failure mode it exists to survive — is pinned by no test; the suite pins only needs and if fragments. The job comment ("runs on an ephemeral hosted runner, so it survives whatever killed the review job") is the sole enforcement. — Failure scenario: a future edit routes fallback-comment onto the ECS pool (a routine-looking consolidation — the pool every other compute job in this file uses); all 131 tests stay green (verified mutant). On the next PR #8894-class incident the fallback job dies the same death as review-pr, and the PR is again left with no review and no comment — the exact state this PR exists to eliminate. Suggested fix: expect(job['runs-on']).toBe('ubuntu-latest') in the resilience suite, with a comment naming the survival requirement.

Suggested change
runs-on: 'ubuntu-latest'
runs-on: 'ubuntu-latest'
中文说明

该 job 承重的部署不变量——必须运行在临时托管 runner 上、绝不能运行在它所要幸存其故障模式的自托管 ECS 池上——没有任何测试钉住;套件只钉了 needsif 的片段。job 注释("runs on an ephemeral hosted runner, so it survives whatever killed the review job")是唯一的约束。— 故障场景:未来某次编辑把 fallback-comment 改到 ECS 池上(看似例行整合——该文件里其他所有计算 job 都用这个池);全部 131 个测试保持绿色(变异体已验证)。下一次 PR #8894 类事故发生时,fallback job 会与 review-pr 同样死亡,PR 再次落入既无 review 也无评论的境地——正是本 PR 要消灭的状态。建议修复:在 resilience 套件中加入 expect(job['runs-on']).toBe('ubuntu-latest'),并用注释写明存活要求。

— qwen3.8-max via Qwen Code /review (v0.21.12)

timeout-minutes: 5
Comment on lines +1821 to +1822

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The fallback job's timeout-minutes: 5 is pinned by no assertion, unlike the identical invariant this file already enforces for the capture-tools step (which pins both the value and the worst-case retry budget arithmetically). — Failure scenario: verified surviving mutant — shrinking it below the loop's wall-clock floor (3 × sleep 10 plus up to six gh calls with their own backoff) makes GitHub cancel the job mid-loop on exactly the degraded-API conditions the loop exists to survive — no comment, no fail-closed summary — while the suite stays green (probe: timeout-minutes: 0.4 left it 144/144; adding the pin flipped it red).

Suggested fix: add expect(job['timeout-minutes']).toBe(5); beside the existing runs-on pin (the timeout pin alone is the verifiable guard — the gh calls have no script-level timeout, so a budget formula has no finite inputs to compute from).

中文说明

兜底 job 的 timeout-minutes: 5 没有任何断言钉住,而本文件对 capture-tools 步骤已强制同样的不变量(既钉数值、又以算术钉最坏重试预算)。— 故障场景:已验证存活的变异体——把它缩小到循环墙钟下限(3 × sleep 10 加最多六次自带退避的 gh 调用)以下,GitHub 会在恰是该循环为之存在的 API 降级条件下于循环中途取消 job——无评论、无失败关闭摘要——而套件仍保持绿色(探针:timeout-minutes: 0.4 时仍 144/144;加上该钉住后转红)。

建议修复:在现有 runs-on 钉住旁添加 expect(job['timeout-minutes']).toBe(5);(仅超时钉住即为可验证的防线——gh 调用无脚本级超时,预算公式没有可计算的有限输入)。

— qwen3.8-max via Qwen Code /review (v0.21.12)

permissions:
pull-requests: 'write'
steps:
- name: 'Post fallback comment'
env:
GH_TOKEN: '${{ secrets.CI_BOT_PAT }}'
PR_NUMBER: '${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}'
RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
Comment on lines +1829 to +1830

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-5: This RUN_URL env expression is read by no test — the executed tests inject a literal RUN_URL — so the invariant the dedup depends on (the rendered link ends exactly at the run id, matching *"actions/runs/${GITHUB_RUN_ID})"*) is verified only for the injected literal, not for this YAML. The marker got byte-identical pinning tests for exactly this drift class; the URL shape did not. — Failure scenario: appending any suffix here (e.g. an /attempts/1 deep-link, plausibly added when someone improves the log link) makes the posted body contain runs/12345/attempts/1) → the dedup pattern no longer matches on re-run → every re-run of a dead review silently posts a duplicate fallback comment, with no error signal anywhere. Witness: probe (step's real bash, stub gh, GITHUB_RUN_ID=12345): base shape → NO POST (dedup matches); /attempts/1 mutant → POSTED (duplicate); mutated YAML suite run 135/135 green.

Suggested fix: assert this step env RUN_URL matches /\/actions\/runs\/\$\{\{ github\.run_id \}\}'$/ (and ideally equals the in-job step's RUN_URL expression).

中文说明

这个 RUN_URL env 表达式没有任何测试读取——执行型测试注入的是字面量 RUN_URL——因此 dedup 所依赖的不变量(渲染出的链接恰好在 run id 处结束,才能匹配 *"actions/runs/${GITHUB_RUN_ID})"*)只对被注入的字面量成立,对这段真实 YAML 并无验证。隐藏标记针对同类漂移已有逐字节钉住的测试,URL 形状却没有。故障场景:在此行追加任何后缀(例如有人改进日志链接时加上 /attempts/1 深链),正文将包含 runs/12345/attempts/1) → dedup 模式在重跑时不再匹配 → 死 review 的每次重跑都会静默多发一条兜底评论,且没有任何报错信号。证据:探针(真实 bash + stub gh,GITHUB_RUN_ID=12345):原始形状 → 不发布(dedup 命中);/attempts/1 变异 → 发布(重复);变异下套件 135/135 全绿。建议修复:断言该 env 以 run id 结尾,并与 job 内步骤的 RUN_URL 表达式一致。

— qwen3.8-max via Qwen Code /review (v0.21.12)

run: |-
set -uo pipefail
if [ -z "$PR_NUMBER" ]; then
echo "Could not determine the PR number; skipping fallback comment." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi
# A push landing mid-review leaves this comment pointing at a dead
# run while a fresh review of the new head already queues (per-run
# concurrency groups are not cancelled by pushes). The run's head
# is comparable only on pull_request_target events — comment and
# review runs report main's tip as headSha — so guard only there,
# and when the comparison is unavailable or fails, posting wins
# over silence.
if [ "${GITHUB_EVENT_NAME:-}" = "pull_request_target" ]; then
run_head="$(gh run view "${GITHUB_RUN_ID:?}" --repo "$GITHUB_REPOSITORY" --json headSha --jq '.headSha' 2>/dev/null)" || run_head=""
current_head="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json headRefOid --jq '.headRefOid' 2>/dev/null)" || current_head=""
if [ -n "$run_head" ] && [ -n "$current_head" ] && [ "$run_head" != "$current_head" ]; then
echo "Skipping fallback comment: PR #${PR_NUMBER} moved from ${run_head} to ${current_head} since this run started." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi
fi
# Dedup lookup with bounded retry: a FAILED lookup is never treated
# as an EMPTY result — posting on a failed listing is how a
# transient 5xx mints a permanent duplicate (same norm as
# upsert-bot-comment.sh). The author scope resolves the
# authenticated login dynamically so a participant posting the
# marker can never capture the lookup, and the filter cannot drift
# from the account CI_BOT_PAT posts as.
bot_login=""
fallback_bodies=""
for _attempt in 1 2 3; do

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This dedup lookup re-inlines the marker+author upsert protocol that .github/scripts/upsert-bot-comment.sh was created to consolidate — the 3-attempt retry with sleep 10, dynamic gh api user --jq '.login' resolution, author-scoped marker filter, and fail-closed-on-failed-listing. The script's header states it exists because "the previous per-step copies had already drifted", and the in-code comment here even says "same norm as upsert-bot-comment.sh". The ~10% that differs is deliberate (dedup on marker + this run's URL, skip instead of PATCH), but the prerequisite machinery is now maintained in two places, and the new tests pin the inline form textually, cementing the second implementation. — Concrete cost: when the shared script's protocol is next hardened (retry count, backoff, fail-closed semantics), nothing references it from this job — the inline copy silently stays stale and the two comment paths drift, the exact failure mode the shared script was extracted to prevent. Suggested fix: add a base-ref actions/checkout step to this job (it currently checks out no code) and route the lookup/post through upsert-bot-comment.sh — either accepting upsert semantics or extending the script with a post-if-absent mode plus a caller-supplied body predicate.

中文说明

该去重查找把 .github/scripts/upsert-bot-comment.sh 被抽出来统一的 marker+author upsert 协议重新内联了一遍——3 次重试 + sleep 10、动态 gh api user --jq '.login' 解析、按作者过滤的 marker 筛选、查询失败即拒绝发布(fail-closed)。脚本头部写明它存在的原因正是"先前各步骤的拷贝已经发生漂移",而这里的代码注释甚至写着 "same norm as upsert-bot-comment.sh"。约 10% 的差异是有意为之(以 marker + 本 run URL 去重、跳过而非 PATCH),但前置机制现在要在两处手工维护,且新测试以文本方式钉住了内联形式,把第二份实现固化了下来。— 具体代价:当共享脚本的协议下次被加固(重试次数、退避、fail-closed 语义)时,本 job 没有任何东西引用它——内联拷贝会悄悄停留在旧版本,两条评论路径随之漂移,正是共享脚本被抽取出来要防止的失败模式。建议修复:给该 job 增加一个 base-ref 的 actions/checkout 步骤(目前不 checkout 任何代码),并把查找/发布改走 upsert-bot-comment.sh——要么接受 upsert 语义,要么给脚本扩展一个"不存在才发布"的模式并支持调用方提供的正文谓词。

— qwen3.8-max via Qwen Code /review (v0.21.12)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined for this PR (not on the merits of consolidation). The suggested fix extends .github/scripts/upsert-bot-comment.sh with a post-if-absent mode plus a caller-supplied body predicate and adds a base-ref checkout to this job — that script is shared machinery used by the docs-only relay and the stale-badge supersede step, so a protocol change risks regressing both, and .github/scripts/ is outside this PR's footprint. The ~10% divergence here is deliberate (dedup on marker plus this run's URL, skip instead of PATCH), and the inline copy is pinned textually and executed by the suite, so it cannot silently rot within this PR. Recommended follow-up: a dedicated PR consolidating all three comment-protocol sites (the upsert script, this dedup, and the R2-9 in-job lookup) behind one script mode.

中文说明

对本 PR 予以拒绝(并非不认可合并本身)。建议的修复要给 .github/scripts/upsert-bot-comment.sh 扩展"不存在才发布"模式并支持调用方正文谓词,还要给该 job 增加 base-ref checkout——该脚本是 docs-only relay 与 stale-badge supersede 步骤共用的机制,改动其协议有让两者回归的风险,且 .github/scripts/ 不在本 PR 的足迹之内。此处约 10% 的差异是有意为之(以 marker 加本 run URL 去重、跳过而非 PATCH),且内联拷贝被套件以文本钉住并实际执行,在本 PR 生命周期内不会悄悄腐化。建议后续:用一个专门的 PR 把三处评论协议站点(upsert 脚本、此去重、以及 R2-9 的 job 内查找)统一到同一脚本模式之下。

if bot_login="$(gh api user --jq '.login')" \
&& [ -n "$bot_login" ] \
Comment on lines +1862 to +1863

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-7: The cross-job dedup's load-bearing identity agreement — the in-job comment must be posted by the same account this lookup resolves via gh api user — is pinned by no test. It works today because the in-job step (line 1660) happens to use the same CI_BOT_PAT; nothing asserts it. — Failure scenario: mutate line 1660 to github.token (a plausible edit — the adjacent ack step posts with GITHUB_TOKEN) → the in-job comment is authored by github-actions[bot], invisible to this author-scoped filter → on every ordinary failure (in-job step posts, job result stays failure, gate opens) this job posts a second "did not complete" comment for the same run. Witness: probe applied that mutant — all four workflow-parsing suites stayed green (164 + 178 tests); adding one assertion pinning inJobStep.env.GH_TOKEN to CI_BOT_PAT made the same mutant fail (expected '${{ secrets.GITHUB_TOKEN }}' to be '${{ secrets.CI_BOT_PAT }}'). Tree restored, baseline 135/135.

Suggested fix: in the scopes the dedup to the authenticated bot login test, add:

expect(inJobStep.env.GH_TOKEN).toBe("'${{ secrets.CI_BOT_PAT }}'");

with a comment that the author-scoped dedup only sees comments posted by the same account.

中文说明

跨 job 去重的承重不变量——job 内评论必须由本查询经 gh api user 解析出的同一账户发布——没有任何测试钉住。它今天成立只是因为 job 内步骤(1660 行)恰好也用 CI_BOT_PAT,并无任何断言保证。故障场景:把 1660 行改成 github.token(完全可能的编辑——相邻的 ack 步骤就用 GITHUB_TOKEN)→ job 内评论的作者变成 github-actions[bot],对这个按作者过滤的查询不可见 → 每次普通失败(job 内步骤已发布、job 结果仍为 failure、gate 打开)本 job 都会为同一 run 再发一条 "did not complete" 评论。证据:探针应用该变异——四个解析此 workflow 的测试套件全部保持绿色(164 + 178);加上一行把 inJobStep.env.GH_TOKEN 钉到 CI_BOT_PAT 的断言后,同一变异即失败。树已还原,基线 135/135。建议修复:在 scopes the dedup... 测试中补上该断言,并注明作者作用域去重只能看到同一账户发布的评论。

— qwen3.8-max via Qwen Code /review (v0.21.12)

&& fallback_bodies="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json comments \
--jq ".comments[] | select(.author.login == \"$bot_login\") | select(.body | contains(\"$FALLBACK_MARKER\")) | .body")"; then
break
fi
bot_login=""
fallback_bodies=""
sleep 10
Comment on lines +1868 to +1870

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R3-4: The retry loop's post-failure reset lines are unobservable in every test scenario — the only lookup-failure scenario (lookup_fail) fails BOTH gh api user and the comments listing, so bot_login ends up empty via the failed command substitution itself and these resets never matter. A mutant deleting them survives. — Failure scenario: gh api user succeeds while the comments listing returns 5xx on all three attempts → in the mutant, bot_login stays non-empty with empty fallback_bodies, the post-loop [ -z "$bot_login" ] guard passes, the dedup case matches nothing, and the job posts on a failed listing — the exact "transient 5xx mints a permanent duplicate" outcome the step's own comment forbids. Witness: probe ran the step's real bash under partial failure — original exits 1 with ::error:: and posts nothing; mutant (resets deleted) exits 0 and POSTED a comment; suite 135/135 green under the mutant.

Suggested fix: add a comments_lookup_fail scenario (user lookup succeeds, *comments* exits 1) asserting status === 1 and posted === ''.

中文说明

重试循环在失败后的两行重置(bot_login="" / fallback_bodies="")在所有测试场景中都不可观测——唯一的查询失败场景 lookup_fail 同时令 gh api user 与评论列表失败,bot_login 本来就因命令替换失败而为空,重置行从未起作用,删掉它们的变异可以存活。故障场景:gh api user 成功而评论列表三次尝试全部 5xx → 变异体中 bot_login 非空而 fallback_bodies 为空,循环后的 [ -z "$bot_login" ] 守卫通过、dedup case 匹配不到任何内容,job 在一次失败的列表之上发布评论——正是该步注释明令禁止的"瞬时 5xx 铸成永久重复评论"。证据:探针在部分失败下真实执行该步 bash——原版 exit 1 且未发布;变异体 exit 0 且发布了评论;变异下套件 135/135 全绿。建议修复:新增 comments_lookup_fail 场景(user 查询成功、评论列表失败),断言 status === 1posted === ''

— qwen3.8-max via Qwen Code /review (v0.21.12)

done
if [ -z "$bot_login" ]; then
echo "::error::fallback comment dedup lookup failed after retries; refusing to post on a failed listing"
echo "Fallback comment lookup failed after retries; skipping to avoid a duplicate." >> "$GITHUB_STEP_SUMMARY"
exit 1
fi
case "$fallback_bodies" in
*"actions/runs/${GITHUB_RUN_ID})"*)
echo "A fallback comment for this run already exists; skipping." >> "$GITHUB_STEP_SUMMARY"
exit 0
;;
esac
pr_state="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state --jq '.state')" || {
echo "::error::could not verify PR #${PR_NUMBER} state; refusing to post on a failed lookup"
echo "Could not verify PR #${PR_NUMBER} (API error); failing instead of guessing." >> "$GITHUB_STEP_SUMMARY"
exit 1
}
Comment on lines +1883 to +1887

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The twin failure mode of the lookup above: if this gh pr view call fails (expired/revoked CI_BOT_PAT, API outage), the job exits 0 GREEN with no comment posted and one ambiguous step-summary line — "Could not verify PR #N" reads identically for a deleted PR and for dead credentials. The run page shows fallback-comment ✓ for the one job whose sole purpose went unachieved, and the repo's flaky-rerun automation keys on failure, so green hides from it too: the failure-notifier fails silently.

Red-on-own-failure is safe here: the gate already requires review-pr.result == 'failure', so the run is red anyway. Emit a ::error:: annotation (annotations surface at run level; step summaries don't) and exit non-zero when the state check itself cannot complete; reserve exit 0 for a positively-determined non-OPEN PR.

中文说明

上面查找的对称失效模式:如果这个 gh pr view 调用失败(CI_BOT_PAT 过期/被吊销、API 故障),job 以 0 绿色退出,评论没发,只在 step summary 留下一句含糊的话——"Could not verify PR #N" 对"PR 已删除"和"凭证已失效"读起来一模一样。运行页面上,唯一职责没达成的 job 却显示 fallback-comment ✓;仓库的 flaky 重跑自动化以 failure 为键,绿色也同样躲过它:失败通知者自己静默失败了。

自身失败时变红在这里是安全的:门控已要求 review-pr.result == 'failure',整个 run 本来就是红的。状态检查本身无法完成时,发一条 ::error:: 注解(注解在 run 级别可见,step summary 不可见)并以非零退出;exit 0 保留给确认为非 OPEN 的 PR。

— qwen3.8-max via Qwen Code /review (v0.21.12)

if [ "$pr_state" != "OPEN" ]; then
echo "Skipping fallback comment: PR #${PR_NUMBER} is ${pr_state}." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi
Comment on lines +1888 to +1891

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This job drops the in-job step's moved-head guard (~line 1665 compares EXPECTED_HEAD_SHA with the current head): comment/review-event runs use per-run concurrency groups that a new push does NOT cancel (cancel-in-progress is only set for pull_request_target synchronize/closed). So a commit landing mid-review, followed by an abnormal review death, makes this job post stale "retry with @qwen-code /review" pointing at a dead run while a fresh lifecycle review of the new head is already queued — noise, and potentially provoking a redundant manual /review.

⚠️ Fix carefully: for issue_comment runs, gh run view --json headSha returns the tip of main, not the PR head (verified: 8 recent issue_comment runs all show head_branch=main) — a naive headSha vs headRefOid comparison would mismatch on essentially every comment-triggered run and silently suppress the fallback on the very path this job exists for. Apply the guard only where a comparable head exists (pull_request_target events), and degrade to POSTING, never suppressing, when the comparison cannot be made.

中文说明

本 job 丢掉了 job 内步骤的"head 已移动"防护(约 1665 行会比较 EXPECTED_HEAD_SHA 与当前 head):评论/review 事件的运行使用按 run 分组的并发组,新 push 不会取消它们(cancel-in-progress 只对 pull_request_target 的 synchronize/closed 生效)。因此 review 进行中有新提交落入、随后 review 异常死亡时,本 job 会发出一条指向已死 run 的过期"重试 @qwen-code /review",而新 head 的自动 review 已经排队——噪音,还可能诱发多余的手动 /review。

⚠️ 修复需谨慎:对 issue_comment 运行,gh run view --json headSha 返回的是 main 的尖端而不是 PR 的 head(已验证:最近 8 个 issue_comment 运行全部 head_branch=main)——朴素的 headShaheadRefOid 比较几乎在每个评论触发的 run 上都会失配,从而在本 job 存在的核心路径上静默抑制兜底评论。只在 head 可比较的事件(pull_request_target)上加该防护;无法比较时,退化为发布,绝不抑制。

— qwen3.8-max via Qwen Code /review (v0.21.12)

body="**Qwen Code review did not complete successfully.** The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with \`@qwen-code /review\`. See [workflow logs](${RUN_URL})."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Fallback comments are fire-and-forget: nothing updates, retracts, or consolidates a marker comment after posting (repo-wide, the marker's only consumers are the two dedup filters). — Failure scenario: a run dies, the fallback posts "retry with @qwen-code /review", the retry succeeds and the real review posts — a stale, still-actionable failure claim persists beside the completed review, and a contributor following its advice mints another full review run; on a persistently unhealthy pool each instructed retry and each synchronize run posts one more near-identical comment. This file already upserts the ack and the docs-only badge (upsert-bot-comment.sh --update-only); the fallback marker is the only singleton bot comment with no such path.

Suggested fix: route the post through upsert-bot-comment.sh (one marker comment per PR, updated with the latest dead run's URL), and/or supersede a stale fallback comment on the success path — at minimum, decide and document that staleness/accumulation is accepted.

中文说明

兜底评论是"发后即忘":发布后没有任何机制更新、撤回或合并标记评论(全仓库范围内,该标记的唯一消费者是两处去重过滤器)。— 故障场景:某 run 死亡,兜底发布"请用 @qwen-code /review 重试",重试成功、真正的 review 发出——一条仍然可操作的陈旧失败声明与已完成的 review 并存,贡献者照做又会触发一次完整 review;在持续不健康的 runner 池上,每次按其指引的重试、每次 synchronize 运行都会再发一条几乎相同的评论。本文件对确认评论与仅文档徽章都已采用 upsert(upsert-bot-comment.sh --update-only);兜底标记是唯一没有此路径的单例 bot 评论。

建议修复:经由 upsert-bot-comment.sh 发布(每 PR 一条标记评论,更新为最新死亡 run 的 URL),和/或在成功路径上取代陈旧的兜底评论——至少明确并记录接受陈旧/累积。

— qwen3.8-max via Qwen Code /review (v0.21.12)

body="$(printf '%s\n\n%s' "$FALLBACK_MARKER" "$body")"
gh pr comment "$PR_NUMBER" \
--repo "$GITHUB_REPOSITORY" \
--body "$body"

resolve-pr:
needs: ['authorize']
# The /resolve shape match uses the same fromJSON newline/CR pair as
Expand Down
Loading
Loading