fix(ci): keep web-shell visuals comment on upload failure - #10253
Conversation
Ensure the publish workflow still refreshes the PR marker comment when image hosting fails, so reviewers see a clear hosting-failure note instead of stale or missing preview output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @AaronZ345 — the direction here is genuinely right: #10240 is a real gap (a failed OSS upload aborts the step before the comment posts, so the PR gets no preview comment at all), and teaching the comment builder a third "images produced but not hosted" state is exactly what that issue asks for.
But I have to stop at the gate: the PR body doesn't follow the repository's pull request template. These required sections are missing (the body uses ## Summary / ## Test plan instead):
## What this PR does— describe the change in prose## Why it's needed— motivation; link #10240 and spell out the failure mode## Reviewer Test Plan— with### How to verify,### Evidence (Before & After),### Tested on## Risk & Scope## Linked Issues- A
<details>block carrying the Chinese translation
Please rewrite the description using the template. The substance is already in your Summary/Test plan and in #10240, so this should be a quick fill-in — the maintainer-facing "How to verify" and the bilingual block are the parts that matter most. Once the body matches, re-run with @qwen-code /triage and I'll take it through code review.
Template: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
中文说明
感谢贡献，@AaronZ345 ——这个 PR 的方向是对的：#10240 是一个真实存在的缺口（OSS 上传失败会在评论发出之前中止该步骤，导致 PR 完全收不到预览评论），而给评论构建器增加第三种"已生成截图但无法托管"的状态，正是该 issue 所要求的。
但我必须在门禁这一步停下来：PR 正文没有遵循本仓库的 pull request 模板。以下必需章节缺失（正文用了 ## Summary / ## Test plan）：
## What this PR does—— 用文字描述改动## Why it's needed—— 动机；关联 #10240 并说明失败场景## Reviewer Test Plan—— 包含### How to verify、### Evidence (Before & After)、### Tested on## Risk & Scope## Linked Issues- 一个包含中文翻译的
<details>块
请用模板重写描述。实质内容已经在你现有的 Summary/Test plan 和 #10240 里，所以这应该只是快速填写——最关键的是面向维护者的 "How to verify" 和中文对照部分。正文符合模板后，用 @qwen-code /triage 重新触发，我会继续代码审查。
模板：https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
— Qwen Code · qwen3.8-max
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@qwen-code /triage |
1 similar comment
|
@qwen-code /triage |
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/web-shell-visuals-publish.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/web-shell-visuals-publish.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (hostingFailed) { | ||
| out.push( | ||
| `⚠️ _Preview images rendered, but the publish workflow failed to host them on the PR assets branch${runLink}._ This comment was still refreshed so stale images from an older push do not remain attached to this SHA.`, |
There was a problem hiding this comment.
[Suggestion] This branch pre-empts the existing renderIncomplete branches instead of composing with them. When both flags are set — at least one render scenario crashes on the PR head (the capture step is continue-on-error, so the passing shots are still staged and render-status.txt reads failure) and the asset push then fails all three retries — the comment asserts "Preview images rendered, but … failed to host" while the "one or more scenarios failed to render" caveat is dropped. A view a reviewer expects but does not see then reads as "no change" instead of a render crash — exactly the ambiguity the renderIncomplete note exists to prevent. Probe-verified: buildComment(['a.png', 'b.png', 'c.gif'], { hostingFailed: true, renderIncomplete: true, runUrl }) mentions only the hosting failure; composing the caveat into this branch flips both checks. When renderIncomplete is also true, push the existing render-failure note (line 248) from this branch as well:
if (hostingFailed) {
if (renderIncomplete) {
out.push(
`⚠️ _One or more scenarios failed to render_ on this head, so this preview may be missing views${runLink}.`,
);
out.push('');
}
out.push(If you add the note, also add a buildComment test with { hostingFailed: true, renderIncomplete: true } asserting /failed to render/i, and confirm it goes red when the note is removed.
中文说明
此分支抢先于已有的 renderIncomplete 分支，而不是与之组合。当两个标志同时为真时——至少一个渲染场景在 PR head 上崩溃（capture 步骤设置了 continue-on-error，成功的截图仍会被暂存，render-status.txt 读作 failure），且资产推送三次重试全部失败——评论会断言 "Preview images rendered, but … failed to host"，而 "one or more scenarios failed to render" 的提示被丢弃。评审者预期看到却没有出现的视图会被读作"没有变化"，而不是渲染崩溃——这正是 renderIncomplete 提示要避免的歧义。已通过探针验证：buildComment(['a.png', 'b.png', 'c.gif'], { hostingFailed: true, renderIncomplete: true, runUrl }) 只提到托管失败；把该提示组合进此分支后两项检查均为真。当 renderIncomplete 也为真时，请在此分支中同时输出已有的渲染失败提示（第 248 行，见上方代码）。如果添加该提示，请同时新增一个 buildComment 测试，使用 { hostingFailed: true, renderIncomplete: true } 断言 /failed to render/i，并确认移除该提示后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| /HOSTING_STATUS_FILE="\$\{RUNNER_TEMP\}\/visuals-hosting-status\.txt"/, | ||
| ); | ||
| assert.match(workflow, /echo 'failure' > "\$\{HOSTING_STATUS_FILE\}"/); | ||
| assert.match(workflow, /"\$\{HOSTING_STATUS_FILE\}"/); |
There was a problem hiding this comment.
[Suggestion] This assertion does not pin the hand-off it names: the workflow contains three quoted ${HOSTING_STATUS_FILE} references — the two echo … > "${HOSTING_STATUS_FILE}" writes and the comment invocation — so the echo lines alone satisfy the regex. Mutation-verified at HEAD: deleting the "${HOSTING_STATUS_FILE}" argument from the node … comment invocation, or swapping it with "${RENDER_STATUS_FILE}", keeps the whole suite green (25/25 pass), while at runtime commentCli fails open to hostingFailed=false and buildComment emits broken <img src="/…"> links after a push failure — the exact #10240 regression this PR fixes, shipped under a green guard. The argv-8 seam has no executable coverage either, since the buildComment tests inject hostingFailed: true directly. Anchor the assertion on the invocation itself:
| assert.match(workflow, /"\$\{HOSTING_STATUS_FILE\}"/); | |
| assert.match( | |
| workflow, | |
| /"\$\{RENDER_STATUS_FILE\}" \\\n\s*"\$\{HOSTING_STATUS_FILE\}"/, | |
| ); |
Optionally add one seam test that spawns node .github/scripts/web-shell-visuals-publish.mjs comment … with a status file containing failure to pin the argv position, the trim/newline handling, and the 'failure' literal together. The strengthened assertion must go red when the "${HOSTING_STATUS_FILE}" argument is removed from the workflow invocation — please confirm that mutation.
中文说明
该断言没有固定它所命名的传递关系：workflow 中有三处带引号的 ${HOSTING_STATUS_FILE} 引用——两处 echo … > "${HOSTING_STATUS_FILE}" 写入和评论调用——因此仅有 echo 行就能满足该正则。已在 HEAD 上做变异验证：从 node … comment 调用中删除 "${HOSTING_STATUS_FILE}" 参数，或将其与 "${RENDER_STATUS_FILE}" 交换，整个测试套件仍为绿色（25/25 通过），而运行时 commentCli 会 fail-open 为 hostingFailed=false，推送失败后 buildComment 会输出失效的 <img src="/…"> 链接——正是本 PR 修复的 #10240 回归，却在绿色守卫下通过。argv-8 接缝也没有可执行覆盖——buildComment 测试直接注入 hostingFailed: true。请将断言锚定到调用本身（见 suggestion 块）。可选：再加一个接缝测试，用包含 failure 的状态文件实际运行 node .github/scripts/web-shell-visuals-publish.mjs comment …，一次性固定 argv 位置、trim/换行处理和 'failure' 字面量。加强后的断言必须在从 workflow 调用中移除 "${HOSTING_STATUS_FILE}" 参数时变红——请确认该变异。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| rest[7], | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This diff adds an 8th positional argument (hostingStatusFile, passed as rest[7]) but leaves the file-header usage line (line 20) documenting only seven: comment <stageDir> <rawBase> <shortSha> <runUrl> <bodyFile> [changedPathsFile] [renderStatusFile]. The header is the script's only prose statement of this positional contract, and the CLI has no argument-count validation. Probe-verified: invoking comment per the documented 7-arg signature with an empty rawBase produces broken <img src="/…"> output with no hosting note and no error, because commentCli fails open on the missing status file. Append [hostingStatusFile] to the usage line in the header comment.
中文说明
此 diff 新增了第 8 个位置参数（hostingStatusFile，以 rest[7] 传入），但文件头部的用法说明（第 20 行）仍只记录 7 个参数。头部是该 CLI 位置参数契约唯一的文字说明，且 CLI 没有参数个数校验。已通过探针验证：按文档中的 7 参数签名调用 comment 且 rawBase 为空时，会输出失效的 <img src="/…">，没有托管失败提示，也没有任何错误——因为 commentCli 对缺失的状态文件 fail-open。请在头部注释的用法行末尾补上 [hostingStatusFile]。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| assert.doesNotMatch( | ||
| workflow, | ||
| /Failed to push web-shell visuals to \$\{BRANCH\} after retries\."\n\s+exit 1/, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This guard requires the literal OLD error-message text immediately before exit 1 — text this PR deleted (the workflow now emits the ::warning::…after retries; updating the PR comment… variant). Grep confirms the old message survives only in the unrelated validate_pr line, so this guard can never fire again: re-adding exit 1 right after echo 'failure' > "${HOSTING_STATUS_FILE}" keeps the suite green (mutation-verified: 25/25 pass) while the job aborts before the comment step — resurrecting exactly the abort-before-comment behaviour this PR exists to remove. Pin the effect instead of the old message:
| assert.doesNotMatch( | |
| workflow, | |
| /Failed to push web-shell visuals to \$\{BRANCH\} after retries\."\n\s+exit 1/, | |
| ); | |
| assert.doesNotMatch( | |
| workflow, | |
| /echo 'failure' > "\$\{HOSTING_STATUS_FILE\}"\n\s+exit 1/, | |
| ); |
Confirm the strengthened assertion goes red when exit 1 is re-added after the failure write.
中文说明
这个守卫要求 exit 1 前面紧邻旧错误信息的字面文本——而该文本已被本 PR 删除（workflow 现在输出 ::warning::…after retries; updating the PR comment… 变体）。grep 确认旧信息只残留在无关的 validate_pr 行中，因此该守卫永远不会再触发：在 echo 'failure' > "${HOSTING_STATUS_FILE}" 之后重新加上 exit 1，测试套件仍为绿色（变异验证：25/25 通过），而作业会在评论步骤之前中止——正是本 PR 要移除的"评论前先中止"行为的复活。请固定效果而非旧信息（见 suggestion 块），并确认在失败写入之后重新加上 exit 1 时加强后的断言会变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| assert.match(body, /failed to host/i); | ||
| assert.match(body, /home-light\.png/); | ||
| assert.match(body, /model-switch\.gif/); | ||
| assert.match(body, /https:\/\/run\.example\/7/); |
There was a problem hiding this comment.
[Suggestion] This assertion is satisfied by the unconditional <sub>Full-resolution recordings…</sub> footer — which embeds the run URL whenever runUrl is truthy — not by the runLink interpolated into the hosting warning. Mutation-verified: removing ${runLink} from the warning template keeps the suite green (25/25 pass) while the warning loses its "see the workflow run" link. Assert the markdown-link shape the warning must produce:
| assert.match(body, /https:\/\/run\.example\/7/); | |
| assert.match(body, /\[workflow run\]\(https:\/\/run\.example\/7\)/); |
Confirm the anchored assertion goes red when ${runLink} is removed from the warning.
中文说明
该断言被无条件的 <sub>Full-resolution recordings…</sub> 页脚满足——只要 runUrl 为真值，页脚就会嵌入 run URL——而不是由插入到托管警告中的 runLink 满足。变异验证：从警告模板中移除 ${runLink}，测试套件仍为绿色（25/25 通过），而警告会失去 "see the workflow run" 链接。请断言警告必须产生的 markdown 链接形态（见 suggestion 块），并确认从警告中移除 ${runLink} 时该断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Preserve render-failure context when hosting fails and tighten tests around the hosting-status handoff so the preview comment cannot silently regress. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 8 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
.github/scripts/web-shell-visuals-publish.mjs:423 — [review] hosting-status read duplicates the render-status read block (opposite polarity).github/scripts/web-shell-visuals-publish.mjs:426 — [probe] hosting-status read untested for 'success' content and missing-file input classes
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 8 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (hostingFailed) { | ||
| if (renderIncomplete) { | ||
| out.push( |
There was a problem hiding this comment.
[Suggestion] R1-1: (fix-induced) The if (renderIncomplete) guard this round added inside the hostingFailed branch is pinned only for its presence: the companion test asserts the caveat appears when both flags are set, but nothing asserts it stays absent when hosting failed and the render completed. Dropping the guard (or hoisting the push out of it) ships green — the first hosting test (renderIncomplete unset) asserts only presence-style content and doesNotMatch(/<img /), and the two doesNotMatch(/failed to render/i) assertions further down run with hostingFailed unset, so no test sees the mutant. Every hosting failure with a complete render would then post a spurious "One or more scenarios failed to render" warning on a preview that rendered completely.
Probe at HEAD (scratch tree): unmodified 27/27 pass; guard-dropped mutant 27/27 pass (ships green); mutant + the fix below → not ok 12 — The input was expected to not match the regular expression /failed to render/i; unmodified + fix 27/27 pass.
Add the negative pin to buildComment: hosting failure reports rendered assets without broken image links:
assert.doesNotMatch(body, /failed to render/i);Fix witness: that assertion goes red when the if (renderIncomplete) guard is removed and stays green with it in place — please confirm the mutation (delete the guard, run the test, see it fail).
中文说明
[Suggestion] R1-1：（由本轮修复引入）这轮在 hostingFailed 分支中新增的 if (renderIncomplete) 守卫只被“存在性”测试钉住：配套测试断言两个标志同时为真时警告会出现，但没有任何测试断言“托管失败但渲染全部完成”时该警告不出现。删掉这个守卫（或把 push 提出条件）后所有测试仍然通过——第一个托管测试（未设置 renderIncomplete）只断言存在性内容和 doesNotMatch(/<img /)，而下方两处 doesNotMatch(/failed to render/i) 断言运行时未设置 hostingFailed，因此没有测试能看到这个突变体。届时每一次渲染完整的托管失败都会错误地发布 “One or more scenarios failed to render” 警告。
探针验证（临时树，HEAD）：未修改 27/27 通过；删除守卫的突变体 27/27 通过（可带病上线）；突变体 + 下方修复 → not ok 12 — The input was expected to not match the regular expression /failed to render/i；未修改 + 修复 27/27 通过。
建议在 buildComment: hosting failure reports rendered assets without broken image links 测试中补上反向断言：
assert.doesNotMatch(body, /failed to render/i);修复验收标准：删除 if (renderIncomplete) 守卫后该断言应变红，守卫存在时保持绿色——请通过突变验证（删除守卫、运行该测试、确认失败）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| assert.match( | ||
| workflow, | ||
| /"\$\{RENDER_STATUS_FILE\}" \\\n\s+"\$\{HOSTING_STATUS_FILE\}"/, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This test pins local regex fragments of the workflow rather than the hosting-failure contract it exists to protect — three probe-verified one-line workflow mutants keep all four assertions green and each breaks the #10240 guarantee:
- Reachability: inserting a diagnostic
echo "::error::…"+exit 1between the failure write and the comment invocation re-aborts the step before the comment builds — thedoesNotMatchonly checks the line immediately after the echo, so follow-up(ci): a failed visuals upload drops the whole web-shell preview comment (from #9985) #10240 (no refreshed comment on hosting failure) silently returns. Probe: mutant suite 27/27 green. - Position: moving
"${CHANGED_PATHS_FILE}"after"${HOSTING_STATUS_FILE}"(the pinned pair stays adjacent) shifts the positional CLI contract. Probe: driving the real comment CLI with reordered argv — correct order<img present: false | 'failed to host' present: true; reordered<img present: true | 'failed to host' present: false>— on a hosting failure the misparse emits broken<img src="/<name>">links built from the empty RAW_BASE. - Placement: moving
echo 'failure' > "${HOSTING_STATUS_FILE}"ahead of the retry loop turns every successful publish into a false "failed to host" comment with bare filenames — nothing pins that the failure write lives in the not-pushed branch, nor that theecho 'success'initializer exists. Probe: mutant suite 27/27 green.
This supersedes the round-1 threads R1-2 and R1-4 — the replacements written there closed the reported inputs but each opened one of the facets above at the same site. Patching the regexes fragment by fragment leaves further facets open; pin the contract structurally in one assertion set instead, e.g.:
// reachability: no abort between the failure write and the comment invocation
const fromFailure = workflow.slice(
workflow.indexOf(`echo 'failure' > "\${HOSTING_STATUS_FILE}"`),
);
const beforeComment = fromFailure.slice(
0,
fromFailure.indexOf('node .github/scripts/web-shell-visuals-publish.mjs comment'),
);
assert.ok(beforeComment.length > 0);
assert.doesNotMatch(beforeComment, /\bexit 1\b/);
// position: the whole positional tail, not just the adjacent pair
assert.match(
workflow,
/"\$\{BODY_FILE\}" \\\n\s+"\$\{CHANGED_PATHS_FILE\}" \\\n\s+"\$\{RENDER_STATUS_FILE\}" \\\n\s+"\$\{HOSTING_STATUS_FILE\}"/,
);
// placement: the failure write lives in the not-pushed branch, and the success seed exists
assert.match(workflow, /echo 'success' > "\$\{HOSTING_STATUS_FILE\}"/);
assert.match(
workflow,
/if \[ "\$\{pushed\}" -ne 1 \]; then\n\s+echo "::warning::[^\n]*"\n\s+echo 'failure' > "\$\{HOSTING_STATUS_FILE\}"/,
);Fix witness: each of the three mutants above must turn the strengthened assertions red — with the current fragment pins all three stay green (proven by the probe runs).
中文说明
[Suggestion] 这个测试钉住的是 workflow 中的局部正则片段，而不是它本应保护的托管失败契约——三个经过探针验证的单行 workflow 突变体都能让全部四条断言保持绿色，且每一个都会破坏 #10240 的保证：
- 可达性：在失败写入和评论调用之间插入一行诊断
echo "::error::…"+exit 1，步骤会在评论构建前再次中止——doesNotMatch只检查 echo 后紧邻的一行，因此 follow-up(ci): a failed visuals upload drops the whole web-shell preview comment (from #9985) #10240（托管失败时评论不刷新）会悄无声息地回归。探针：突变体下测试 27/27 全绿。 - 参数位置：把
"${CHANGED_PATHS_FILE}"移到"${HOSTING_STATUS_FILE}"之后（被钉住的相邻对保持不变）会错位 CLI 的位置参数契约。探针：用重排后的 argv 实际运行 comment CLI——正确顺序<img present: false | 'failed to host' present: true>；重排后<img present: true | 'failed to host' present: false>——托管失败时误解析会用空的 RAW_BASE 拼出失效的<img src="/<name>">链接。 - 写入位置：把
echo 'failure' > "${HOSTING_STATUS_FILE}"移到重试循环之前，会让每一次成功发布都变成带裸文件名的假 “failed to host” 评论——没有任何断言钉住失败写入位于“未推送成功”分支内，也没有断言echo 'success'初始化语句的存在。探针：突变体下测试 27/27 全绿。
本条取代第一轮评论 R1-2 与 R1-4——当时的替换写法关闭了各自报告的问题，但各自又在同一位置打开了上述一个侧面。逐片段修补正则仍会留下更多缺口；建议改为在一组断言中钉住整个契约，例如：
（代码示例见英文部分）
修复验收标准：上述三个突变体各自都必须让加强后的断言变红——在当前的片段式钉扎下三者都保持绿色（已由探针运行证实）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Pin the hosting-failure workflow contract so comments still refresh without false render-failure warnings or broken argument ordering. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
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.
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: .github/scripts/web-shell-visuals-publish.test.mjs (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：.github/scripts/web-shell-visuals-publish.test.mjs（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const beforeComment = fromFailureWrite.slice(0, commentIndex); | ||
| assert.doesNotMatch(beforeComment, /\bexit 1\b/); |
There was a problem hiding this comment.
[Suggestion] R2-1: (fix-induced) The structural rewrite landed this round closed the three mutants named in round 2 — all four of that round's mutations are now killed — but the replacement assertion set leaves seven probe-verified one-line workflow mutants at the same site, each keeping the suite 27/27 green while re-breaking the #10240 guarantee. Demonstrated entrances: (1) an exit 0 in the failure branch — the comment invocation is never reached; (2) deleting the "${STAGE}" \ line — every argument shifts up one slot, hosting status falls out of slot 8, and a runtime-confirmed misparse treats the hosting failure as a render failure; (3) deleting the failure branch's cd "${GITHUB_WORKSPACE}" — the repo-relative script path fails with MODULE_NOT_FOUND and set -e aborts before posting; (4) moving echo 'success' > "${HOSTING_STATUS_FILE}" after the fi — the failure status is overwritten and the real comment CLI emits the exact #10240 broken-image comment; (5) inverting the HAS_IMAGES gate — image PRs skip hosting entirely (root-relative <img> links) while no-image PRs abort on the unmatched cp glob; (6) deleting pushed=0 or pushed=1 — an unbound-variable abort of the failure branch under set -u, or a false hosting-failure comment on every successful publish; (7) the CLI test asserting nothing about the body payload — files = [] and a slot 2/3 dispatcher swap both survive. This family has now recurred across rounds 1, 2 and 3: text-shape pinning of a shell program embedded in YAML is an unbounded surface, and patching it mutant by mutant regenerates a round each time. Close it structurally — either drive the actual Publish visuals to the PR step body end-to-end against stubbed git/gh binaries in a test (a shell oracle covering reachability, argument order, write ordering and cwd restoration at once), or take the maintainer decision that text-shape pins deliberately cover only the core no-abort property (failure write → no exit → comment invocation → eight-argument tail) and stop filing one-line mutants.
Witness: all seven mutants probed in a scratch tree at 92a2229 — each leaves node --test .github/scripts/web-shell-visuals-publish.test.mjs at 27/27 green. Entrance (4), driving the real comment CLI with the mutated status-file state and an empty RAW_BASE:
body contains <img src="/home-light.png" width="900" alt="home-light before/after">,
no "failed to host" note; control arm (status 'failure') has no <img> and carries the warning
Fix witness: with a structural oracle (or a consolidated pin set) in place, weakening it back to the current assertion set must turn at least one of the seven confirmed mutants red — today all seven stay green.
中文说明
[Suggestion] R2-1：（由本轮修复引入）本轮落地的结构化重写关闭了第二轮点名的三个突变体——那一轮的四个突变现在全部被杀死——但替换后的断言组在同一位置留下了七个经探针验证的单行 workflow 突变体，每一个都能让测试套件保持 27/27 全绿，同时重新破坏 #10240 的保证。已演示的入口：(1) 在失败分支中放入 exit 0——评论调用永远不会被执行；(2) 删除 "${STAGE}" \ 行——所有参数上移一个槽位，托管状态移出第 8 槽，运行时确认的误解析会把托管失败当作渲染失败；(3) 删除失败分支的 cd "${GITHUB_WORKSPACE}"——仓库相对脚本路径以 MODULE_NOT_FOUND 失败，set -e 在发评论之前中止；(4) 把 echo 'success' > "${HOSTING_STATUS_FILE}" 移到 fi 之后——失败状态被覆盖，真实 comment CLI 输出的正是 #10240 的失效图片评论；(5) 反转 HAS_IMAGES 门——有图片的 PR 完全跳过托管（根相对 <img> 链接），无图片的 PR 在未匹配的 cp 通配符上中止；(6) 删除 pushed=0 或 pushed=1——set -u 下失败分支因未绑定变量中止，或每次成功发布都发出假的托管失败评论；(7) CLI 测试对正文负载不做任何断言——files = [] 与第 2/3 槽位交换两个突变都能存活。这个家族已在第 1、2、3 轮反复出现：对嵌入 YAML 的 shell 程序做文本形状钉扎是一个无界表面，逐个突变体修补每一轮都会重新生成同样的问题。请从结构上关闭它——要么在测试中用桩 git/gh 端到端驱动真实的 Publish visuals to the PR 步骤正文（一个同时覆盖可达性、参数顺序、写入顺序和 cwd 恢复的 shell 预言机），要么由维护者决定：文本形状钉扎有意只覆盖核心的“不中止”属性（失败写入 → 无 exit → 评论调用 → 八参数尾部），不再逐条提交单行突变体。
见证：全部七个突变体都在 92a2229 的临时树中做过探针——每一个都让 node --test .github/scripts/web-shell-visuals-publish.test.mjs 保持 27/27 全绿。入口 (4)：用突变后的状态文件内容和空 RAW_BASE 驱动真实 comment CLI：
正文包含 <img src="/home-light.png" width="900" alt="home-light before/after">，
没有 "failed to host" 提示；对照组（状态为 'failure'）没有 <img> 且带有警告
修复验收标准：结构化预言机（或合并后的钉扎集）就位后，把它削弱回当前断言组，必须让七个已确认突变体中至少一个变红——目前七个全部保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
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 |
…isuals-upload-comment
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- hosting-status read untested for 'success' content and missing-file inputs — already recorded as the round-2 deferral in review 5040260583
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/web-shell-visuals-publish.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/web-shell-visuals-publish.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const commentBlock = fromFailureWrite.slice( | ||
| commentIndex, | ||
| fromFailureWrite.indexOf(' # --- Post or update the PR comment'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-1: The round-3 class-level claim still stands at this head, and this round's fix opened one new facet at the same site. The workflow-wiring test's text-shape pinning of the shell program embedded in web-shell-visuals-publish.yml remains an unbounded surface: the fix landed this round closed R2-1 entrances (1) exit 0 in the failure branch and (2) deleting "${STAGE}" \ (the argument shift), but one-line workflow mutants still keep the suite 27/27 green while re-breaking the #10240 guarantee. Re-demonstrated at the current head: moving echo 'success' > "${HOSTING_STATUS_FILE}" below the hosting fi lets the failure status be overwritten with success, and driving the real comment CLI with that mutated state and an empty RAW_BASE emits the exact #10240 broken-image comment — <img src="/home-light.png" …> with no "failed to host" note — while every assertion stays green (the test matches the success write's existence, never its ordering before the failure write).
The facet this round's fix introduced: the assertion window now ends at fromFailureWrite.indexOf(' # --- Post or update the PR comment'). Rename or re-indent that unrelated section header (a formatting edit with zero effect on the hosting handoff) and indexOf returns -1, slice(commentIndex, -1) widens the window to end of file — which includes the comment-posting section's own legitimate exit 1 — and assert.doesNotMatch(commentBlock, /\bexit\s+[01]\b/) fails: a formatting edit false-reds the test. Both mutants were probed in a scratch tree at this head.
Witness:
mutant: `echo 'success' …` moved below the hosting fi — node --test: 27/27 pass
CLI on mutated state (empty RAW_BASE): <img src="/home-light.png" width="900" alt="home-light before/after">, no hosting note
control arm (status 'failure'): hosting-failure warning, no <img>
mutant: header renamed — 26/27 pass; not ok 27: expected to not match /\bexit\s+[01]\b/
The family has now recurred across rounds 1 through 4; the two ways out remain the ones R2-1 named: drive the actual Publish visuals to the PR step body end-to-end against stubbed git/gh binaries (one shell oracle covering reachability, argument order, write ordering, and cwd restoration at once), or take the maintainer decision that text-shape pins deliberately cover only the core no-abort property and stop filing one-line mutants. Whichever is chosen, the false-red coupling must go with it: bound the assertion window on the comment invocation itself, or parse the workflow structurally with the yaml package the way classify-release-notes.test.mjs does — never on an unrelated section header. With the structural oracle (or consolidated pin set) in place, weakening it back to the current assertion set must turn at least one of the confirmed surviving mutants red — today the success-write-ordering mutant stays green.
中文说明
[Suggestion] R2-1：第 3 轮的类级别结论在当前 head 上依然成立，且本轮修复在同一位置带来了一个新缺口。对嵌入 web-shell-visuals-publish.yml 的 shell 程序做文本形状钉扎仍是无界表面：本轮落地的修复关闭了 R2-1 的入口 (1) 失败分支中的 exit 0 和 (2) 删除 "${STAGE}" \（参数位移），但仍存在单行 workflow 突变体能让测试套件保持 27/27 全绿、同时重新破坏 #10240 的保证。已在当前 head 重新演示：把 echo 'success' > "${HOSTING_STATUS_FILE}" 移到 hosting fi 之后，失败状态会被 success 覆盖；用突变后的状态文件和空 RAW_BASE 驱动真实 comment CLI，输出的正是 #10240 的失效图片评论——<img src="/home-light.png" …> 且没有 "failed to host" 提示——而所有断言保持绿色（测试只匹配 success 写入的存在，从不校验它先于 failure 写入）。
本轮修复引入的新缺口：断言窗口的结束锚点现在是 fromFailureWrite.indexOf(' # --- Post or update the PR comment')。重命名或重新缩进这个无关的段落标题（对托管交接毫无影响的格式改动）时，indexOf 返回 -1，slice(commentIndex, -1) 会把窗口放宽到文件末尾——包含评论发布段落自己合法的 exit 1——于是 assert.doesNotMatch(commentBlock, /\bexit\s+[01]\b/) 失败：一次格式改动让测试误报变红。两个突变体均已在本 head 的临时树中做过探针验证。
见证（见英文部分的 witness 代码块）。
这个家族已在第 1 至第 4 轮反复出现；出路仍是 R2-1 给出的两条：要么用桩 git/gh 端到端驱动真实的 Publish visuals to the PR 步骤正文（一个同时覆盖可达性、参数顺序、写入顺序和 cwd 恢复的 shell 预言机），要么由维护者决定：文本形状钉扎有意只覆盖核心的"不中止"属性，不再逐条提交单行突变体。无论选哪条，误报耦合都必须一并消除：让断言窗口锚定在评论调用本身，或像 classify-release-notes.test.mjs 那样用 yaml 包结构化解析 workflow——不要锚定无关的段落标题。结构化预言机（或合并后的钉扎集）就位后，把它削弱回当前断言组，必须让至少一个已确认存活的突变体变红——目前"success 写入顺序"突变体仍保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Replace brittle text-shape assertions with an end-to-end shell harness that pins hosting status and comment behavior. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
Adapt the failure-comment handoff to the new Aliyun OSS publishing flow so upload failures still refresh the PR comment without broken image links. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the model risk screen failed or returned an unparseable answer. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定：
|
|
Thanks for the PR — and for rewriting the description to the template after the earlier gate; it reads well now. ✅ Problem: observed, not theoretical. #10240 — filed by a maintainer as a follow-up to #9985 — documents the exact gap: the OSS upload runs under Direction: aligned — this is precisely the fix the issue asks for: "teach the comment builder a third state" (images produced but not hosted), rather than "don't abort". CHANGELOG: not applicable (CI plumbing, no product surface). Size: not core infrastructure. 85 production lines (comment builder +48/−6, workflow +19/−12), 233 test lines — well under every threshold, no maintainer-awareness flag needed. Approach: scope feels right. One new state in Risk: no high-risk paths matched. One process note for the thread: the pre-check flagged this diff as Moving on to code review. 🔍 中文说明感谢这个 PR——也感谢在上一轮门禁之后按模板重写了 PR 描述，现在很规范。✅ 问题：已观测到，不是理论问题。#10240（维护者作为 #9985 的后续跟进提出）明确记录了这个缺口：OSS 上传在评论构建之前、于 方向：一致——这正是 issue 所要求的修复："给评论构建器增加第三种状态"（图片已生成但无法托管），而不是简单地"不要中止"。CHANGELOG：不适用（CI 管道改动，无产品面）。 规模：不涉及核心基础设施。生产代码 85 行（评论构建器 +48/−6，workflow +19/−12），测试 233 行——远低于所有阈值，无需提醒维护者关注。 方案：范围合理。 风险：未命中高风险路径。流程上的一点说明：pre-check 曾把这个 diff 标记为 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI sketched my own fix from #10240 before opening the diff — wrap the upload, record a hosting status, teach What I verified while reading:
Two non-blocking nits:
Testing — the PR's own CI (this run executes no PR code)
Evidence detail: the ubuntu job logged Two honesty notes. First, the suite does pin the change: the harness executes the real publish-step script extracted from the workflow YAML against a stubbed uploader on both arms, and the new assertions (exit 0 + 中文说明代码审查在看 diff 之前，我先根据 #10240 独立勾勒了自己的修复方案——把上传包进条件分支、记录托管状态、给 阅读中核实过的点：
两个非阻塞的小问题：
测试 —— 来自 PR 自身的 CI（本次运行不执行任何 PR 代码）CI 表格见英文部分。 证据细节：ubuntu 任务日志显示 两点如实说明。其一，这套测试确实"钉住"了改动：harness 直接执行从 workflow YAML 中提取的真实 publish 步骤脚本（上传器用 stub，成败两路都跑），新断言（退出码 0 + — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a focused, well-tested fix for a maintainer-filed gap; the two nits above are cosmetic. Stepping back: this is exactly the change #10240 scoped — not a larger rework. The third comment state is the design the issue argued for ("given the choice between silence and a lie, the PR chose silence" — this teaches the builder the truthful third option instead), and the implementation matches my independent sketch almost line for line: status file threaded from the workflow, failure state read defensively, no Thread bookkeeping: the earlier Verdict: approving, pinned to the reviewed commit. 👍 中文说明置信度：4/5 —— 针对维护者所提缺口的聚焦且测试充分的修复；上述两点均属外观层面。 整体来看：这正是 #10240 所界定的改动——没有更大的重构。"第三种评论状态"正是 issue 论证过的设计（issue 原话："在沉默与说谎之间，（当时的）PR 选择了沉默"——本 PR 改为让构建器学会如实的第三种状态），实现与我独立勾勒的方案几乎逐行一致：从 workflow 传入状态文件、防御式读取失败状态、 线程记录：本 PR 此前的 结论：批准，并固定在受审提交上。👍 — 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. ✅
Independent local verification — real publish-step harnessVerdict: the fix does what #10240 asks, end to end. With hosting broken, the real How it was verifiedNot via the PR's own tests (those also pass: 39/39 on head, 34/34 on base). I ran the real step script, extracted byte-for-byte from each arm's workflow YAML, against the real Results
Cross-pairing the two halves confirms both are load-bearing: the head script with the base workflow still aborts before commenting, and the base script with the head workflow posts a comment full of broken The head arm's gh call ordering is preserved: identity → bind gate ×3 (the TOCTOU re-checks) → changed files → comment listing → PATCH/POST. Base vs head under the same injected upload failure (real step, real uploader retries):
The hosting-failure comment, rendered through GitHub's own markdown pipeline (
Full matrix:
Asks for @AaronZ345 (description, not code)
For the record: the bot's 中文版（Chinese version）独立本地验证 — 真实 publish step harness结论：修复端到端达成 #10240 的要求。 注入托管故障时，真实的 验证方式不依赖 PR 自带测试（它们也通过：head 39/39，base 34/34）。我把两臂 workflow YAML 里的 step 脚本逐字节提取出来直接执行，链路上是真实的 结果
交叉配对证明两半改动缺一不可：head 脚本配 base workflow 依旧在发评论前中止；base 脚本配 head workflow 会发出满是失效 head 臂的 gh 调用顺序保持不变：identity → bind 门 ×3（TOCTOU 复查）→ changed files → 评论列表 → PATCH/POST。 （截图见上方英文部分：同一注入下 base vs head 的 A/B、经 GitHub 真实 markdown 管线渲染的失败评论、完整矩阵。） 请作者处理（描述问题，非代码）
另注：bot 的 🤖 Verified with Claude Code — Claude Fable 5 |
…) (QwenLM#10738) * test(ci): make web-shell visuals publish stub module-safe (QwenLM#10736) The two end-to-end publish-step tests added in QwenLM#10253 write an extensionless fake `gh` whose only module-system-dependent line uses `require('node:fs')`. Node resolves the module type of an extensionless script from the nearest ancestor package.json, so a stray `type: module` package.json anywhere above the temp root (shared CI hosts accumulate them) makes Node load the stub as an ES module. Every simulated `gh` call then fails, the publish step exits before writing the comment body the tests read, and both tests die on ENOENT — this was the main CI failure on 1804683, filed as QwenLM#10736. Switch the stub to `process.getBuiltinModule('node:fs')`, which works in both module scopes, exactly as QwenLM#10402 already did for the classify-release-notes stub. * test(ci): pin the gh stub module scope in the visuals publish fixture (QwenLM#10736) * test(ci): add a witness test for the gh stub ESM-scope pin (QwenLM#10736) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
|
Released in v0.23.0. |
4 participants
What this PR does
This PR keeps the web-shell visual preview comment alive when rendered screenshots or GIFs exist but publishing them to the PR assets branch fails. The comment builder now accepts a hosting-failure state and writes a warning with the rendered asset filenames instead of emitting broken image links or leaving an older preview attached to the current SHA. The publish workflow records whether hosting succeeded and passes that status into the comment builder.
Why it's needed
Issue #10240 reports that an upload failure aborts the visuals publish step before the PR comment is refreshed. That leaves reviewers with no current preview comment, or worse, stale images from an older commit. The useful behavior is to still refresh the comment so reviewers can see that rendering completed but hosting failed, then inspect the workflow run for the upload problem.
Reviewer Test Plan
How to verify
Reviewers can confirm the comment-builder behavior by running the helper test suite and checking the new hosting-failure fixture. They can also inspect
.github/workflows/web-shell-visuals-publish.ymlto verify that failed pushes writefailureto the hosting status file instead of exiting before comment generation.Evidence (Before & After)
Before: when
git pushtopr-assets/web-shell-visuals-<PR>failed, the workflow exited immediately and skipped theweb-shell-visuals-publish.mjs commentcall, so no refreshed comment was posted for the current SHA.After: failed hosting records
failure, returns to${GITHUB_WORKSPACE}, still calls the comment builder, and the generated body reports that rendered assets could not be hosted while listing their filenames without raw image URLs.Local verification:
Tested on
Environment (optional)
Node.js local workspace on macOS. The change is CI-script/comment-builder focused; Windows and Linux are covered by repository CI once the PR checks rerun.
Risk & Scope
Linked Issues
Fixes #10240
中文说明
What this PR does
这个 PR 让 web-shell 可视化预览在截图或 GIF 已经渲染出来、但推送到 PR assets 分支失败时，仍然刷新 PR 评论。评论构建器现在支持 hosting-failure 状态，会写入明确的警告和已渲染文件名，而不是输出失效图片链接，或者让旧提交的预览继续挂在当前 SHA 上。发布 workflow 会记录托管是否成功，并把状态传给评论构建器。
Why it's needed
#10240 指出：上传失败会让 visuals publish step 在发评论之前直接中止。这样 reviewer 要么看不到当前预览评论，要么看到旧提交留下的过期图片。更合理的行为是仍然刷新评论，说明渲染已经完成但托管失败，并引导 reviewer 查看 workflow run 里的上传错误。
Reviewer Test Plan
How to verify
Reviewer 可以运行 helper 测试，确认新增的 hosting-failure 场景会生成警告评论。也可以检查
.github/workflows/web-shell-visuals-publish.yml，确认 push 失败时会把 hosting status 写成failure，而不是在生成评论前直接退出。Evidence (Before & After)
Before：当
git push到pr-assets/web-shell-visuals-<PR>失败时，workflow 会立即退出，跳过web-shell-visuals-publish.mjs comment调用，因此当前 SHA 不会收到刷新后的预览评论。After：托管失败会记录
failure，返回${GITHUB_WORKSPACE}，继续调用评论构建器；生成的正文会说明已渲染资源无法托管，并列出文件名，不输出 raw 图片链接。本地验证：
Tested on
Environment (optional)
macOS 本地 Node.js 工作区。该改动集中在 CI 脚本和评论构建器；Windows 和 Linux 由 PR 重新跑 CI 覆盖。
Risk & Scope
Linked Issues
Fixes #10240