fix(ci): keep web-shell visuals comment on upload failure (#10253)

* fix(ci): keep web-shell visuals comment on upload failure

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 <[email protected]>

* fix(core): remove duplicate telemetry registry stub

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(ci): address web-shell visuals review feedback

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 [email protected]

* test(ci): harden visuals hosting failure coverage

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 [email protected]

* test(ci): pin visuals hosting failure comment handoff

Co-Authored-By: Claude Sonnet 4.6 [email protected]

* test(ci): execute visual publishing workflow

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 [email protected]

---------

Co-authored-by: zhangyu.34 <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>

.github/scripts/web-shell-visuals-publish.mjs

@@ -17,7 +17,7 @@
  * `buildComment`) are exported and tested. The file also runs as a CLI for the
  * workflow:
  *   node web-shell-visuals-publish.mjs stage   <screenshotsDir> <gifsDir> <stageDir>
- *   node web-shell-visuals-publish.mjs comment <stageDir> <rawBase> <shortSha> <runUrl> <bodyFile> [changedPathsFile] [renderStatusFile]
+ *   node web-shell-visuals-publish.mjs comment <stageDir> <rawBase> <shortSha> <runUrl> <bodyFile> [changedPathsFile] [renderStatusFile] [hostingStatusFile]
  */
 
 import {
@@ -190,18 +190,22 @@ const codePath = (p) => `\`${esc(String(p).replace(/[`\r\n]/g, ''))}\``;
 
 /**
  * Pure comment builder. `files` is the list of staged filenames (png + gif).
- * `ctx` is `{ rawBase, shortSha, runUrl, changedPaths, renderIncomplete }`:
+ * `ctx` is
+ * `{ rawBase, shortSha, runUrl, changedPaths, renderIncomplete, hostingFailed }`:
  * `changedPaths` is the PR's full changed-file list (used only to triage an
  * empty preview), and `renderIncomplete` is true when >=1 scenario failed to
  * render on the PR head — in which case a missing view may be a render failure,
  * not "no change", and the comment must say so instead of a clean bill of
- * health. Returns the markdown body.
+ * health. `hostingFailed` is true when assets rendered but could not be hosted;
+ * in that state the comment must refresh without broken inline image URLs.
+ * Returns the markdown body.
  */
 export function buildComment(files, ctx = {}) {
   const rawBase = ctx.rawBase ?? '';
   const shortSha = ctx.shortSha ?? '';
   const runUrl = ctx.runUrl ?? '';
   const renderIncomplete = ctx.renderIncomplete === true;
+  const hostingFailed = ctx.hostingFailed === true;
   const url = (name) => `${rawBase}/${encodeURIComponent(name)}`;
   // A "see the run" pointer for the render-failure notes (omitted if unknown).
   const runLink = runUrl ? ` — see the [workflow run](${esc(runUrl)})` : '';
@@ -223,7 +227,25 @@ export function buildComment(files, ctx = {}) {
 
   out.push('#### Screenshots · before / after');
   out.push('');
-  if (shots.length > 0) {
+  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(
+      `⚠️ _Preview images rendered, but the publish workflow failed to host them${runLink}._ This comment was still refreshed so stale images from an older push do not remain attached to this SHA.`,
+    );
+    out.push('');
+    const rendered = [...shots, ...gifs].sort();
+    if (rendered.length > 0) {
+      out.push('Rendered asset filenames:');
+      out.push('');
+      for (const f of rendered) out.push(`- ${codePath(f)}`);
+      out.push('');
+    }
+  } else if (shots.length > 0) {
     // Some views rendered. If others FAILED, say so first: the set below is
     // partial, and a view a reviewer expects but doesn't see may have crashed
     // rather than stayed unchanged.
@@ -276,7 +298,7 @@ export function buildComment(files, ctx = {}) {
     }
   }
 
-  if (gifs.length > 0) {
+  if (!hostingFailed && gifs.length > 0) {
     out.push('#### Flows');
     out.push('');
     for (const g of gifs) {
@@ -365,6 +387,7 @@ function commentCli(
   bodyFile,
   pathsFile,
   renderStatusFile,
+  hostingStatusFile,
 ) {
   let files = [];
   try {
@@ -397,12 +420,22 @@ function commentCli(
       // Unreadable → leave complete (fail open toward the normal preview).
     }
   }
+  let hostingFailed = false;
+  if (hostingStatusFile) {
+    try {
+      hostingFailed =
+        readFileSync(hostingStatusFile, 'utf8').trim() === 'failure';
+    } catch {
+      // Missing/older workflow input → normal hosted/no-image behavior.
+    }
+  }
   const body = buildComment(files, {
     rawBase,
     shortSha,
     runUrl,
     changedPaths,
     renderIncomplete,
+    hostingFailed,
   });
   writeFileSync(bodyFile, body);
   process.stderr.write(`Comment body: ${body.split('\n').length} lines.\n`);
@@ -413,7 +446,16 @@ if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) {
   if (cmd === 'stage') {
     stageCli(rest[0], rest[1], rest[2]);
   } else if (cmd === 'comment') {
-    commentCli(rest[0], rest[1], rest[2], rest[3], rest[4], rest[5], rest[6]);
+    commentCli(
+      rest[0],
+      rest[1],
+      rest[2],
+      rest[3],
+      rest[4],
+      rest[5],
+      rest[6],
+      rest[7],
+    );
   } else {
     process.stderr.write(`unknown command: ${cmd ?? '(none)'}\n`);
     process.exit(2);

.github/scripts/web-shell-visuals-publish.test.mjs

@@ -5,8 +5,10 @@
  */
 
 import assert from 'node:assert/strict';
-import { spawnSync } from 'node:child_process';
+import { execFileSync, spawnSync } from 'node:child_process';
 import {
+  chmodSync,
+  copyFileSync,
   existsSync,
   mkdirSync,
   mkdtempSync,
@@ -18,6 +20,8 @@ import {
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import test from 'node:test';
+import { fileURLToPath } from 'node:url';
+import { parse } from 'yaml';
 
 import {
   buildComment,
@@ -312,10 +316,15 @@ function runHostingBlockIn(
     ? JSON.parse(readFileSync(recordPath, 'utf8'))
     : null;
   const hostedFiles = listFiles(stubRoot).sort();
+  const hostingStatusPath = join(runnerTemp, 'visuals-hosting-status.txt');
+  const hostingStatus = existsSync(hostingStatusPath)
+    ? readFileSync(hostingStatusPath, 'utf8').trim()
+    : null;
   return {
     res,
     record,
     hostedFiles,
+    hostingStatus,
     pr,
     headSha,
     runId,
@@ -329,6 +338,7 @@ test('hosting block uploads staged images to the exact prefix RAW_BASE promises'
     res,
     record,
     hostedFiles,
+    hostingStatus,
     pr,
     headSha,
     runId,
@@ -336,6 +346,7 @@ test('hosting block uploads staged images to the exact prefix RAW_BASE promises'
     runnerTemp,
   } = runHostingBlock(true);
   assert.equal(res.status, 0, res.stderr);
+  assert.equal(hostingStatus, 'success');
   // Flag wiring: the uploader gets the bucket, the credential file the
   // configure step writes, and the prefix the comment URLs are built from.
   const prefix = `pr-assets/web-shell-visuals/${pr}/${headSha}/${runId}/${runAttempt}`;
@@ -414,18 +425,23 @@ test('hosting block gives re-run attempts of the SAME run distinct prefixes', ()
 });
 
 test('hosting block skips the uploader entirely on the no-change arm', () => {
-  const { res, record } = runHostingBlock(false);
+  const { res, record, hostingStatus } = runHostingBlock(false);
   assert.equal(res.status, 0, res.stderr);
   assert.match(res.stdout, /^RAW_BASE=$/m); // empty -> image-less comment
   assert.doesNotMatch(res.stdout, /hosted at/);
   assert.equal(record, null); // the uploader never ran
+  assert.equal(hostingStatus, 'success');
 });
 
-test('hosting block aborts without publishing a URL when upload fails', () => {
-  const { res, record } = runHostingBlock(true, { uploadFails: true });
-  assert.notEqual(res.status, 0);
-  assert.doesNotMatch(res.stdout, /^RAW_BASE=/m);
+test('hosting block records failure and continues without publishing a URL', () => {
+  const { res, record, hostingStatus } = runHostingBlock(true, {
+    uploadFails: true,
+  });
+  assert.equal(res.status, 0, res.stderr);
+  assert.match(res.stdout, /^RAW_BASE=$/m);
+  assert.match(res.stdout, /Failed to host web-shell visuals/);
   assert.equal(record, null);
+  assert.equal(hostingStatus, 'failure');
 });
 
 test('hosting block strips a trailing slash from the public base URL', () => {
@@ -443,6 +459,117 @@ test('hosting block strips a trailing slash from the public base URL', () => {
   assert.doesNotMatch(res.stdout, /assets\.example\.test\/\/pr-assets/);
 });
 
+const publishWorkflow = () =>
+  readFileSync(
+    new URL('../workflows/web-shell-visuals-publish.yml', import.meta.url),
+    'utf8',
+  );
+const publishScriptPath = fileURLToPath(
+  new URL('./web-shell-visuals-publish.mjs', import.meta.url),
+);
+const publishStep = () => {
+  const workflow = parse(publishWorkflow());
+  return workflow.jobs.publish.steps.find(
+    (step) => step.name === 'Publish visuals to the PR',
+  ).run;
+};
+
+const writeExecutable = (path, source) => {
+  writeFileSync(path, source);
+  chmodSync(path, 0o755);
+};
+
+const runPublishStep = ({ uploadSucceeds, script = publishStep() }) => {
+  const root = mkdtempSync(join(tmpdir(), 'web-shell-visuals-workflow-'));
+  try {
+    const workspace = join(root, 'workspace');
+    const runnerTemp = join(root, 'runner');
+    const bin = join(root, 'bin');
+    const callLog = join(root, 'calls.log');
+    const scriptDir = join(workspace, '.github', 'scripts');
+    mkdirSync(join(workspace, 'visuals', 'screenshots'), { recursive: true });
+    mkdirSync(join(workspace, 'visuals', 'gifs'), { recursive: true });
+    mkdirSync(scriptDir, { recursive: true });
+    mkdirSync(runnerTemp);
+    mkdirSync(bin);
+    writeFileSync(join(workspace, 'visuals', 'pr.txt'), '7\n');
+    writeFileSync(
+      join(workspace, 'visuals', 'screenshots', 'home-light.png'),
+      Buffer.from(PNG, 'hex'),
+    );
+    writeFileSync(join(workspace, 'visuals', 'render-status.txt'), 'success\n');
+    copyFileSync(
+      publishScriptPath,
+      join(scriptDir, 'web-shell-visuals-publish.mjs'),
+    );
+    mkdirSync(join(workspace, 'scripts'), { recursive: true });
+    writeFileSync(
+      join(workspace, 'scripts', 'upload-aliyun-oss-assets.js'),
+      "process.exit(process.env.UPLOAD_SUCCEEDS === '1' ? 0 : 1);\n",
+    );
+    writeExecutable(join(runnerTemp, 'ossutil'), '#!/bin/sh\nexit 0\n');
+    writeExecutable(
+      join(bin, 'gh'),
+      `#!/usr/bin/env node
+const args = process.argv.slice(2);
+require('node:fs').appendFileSync(process.env.CALL_LOG, args.join(' ') + '\\n');
+const endpoint = args[1] ?? '';
+if (endpoint === 'user') {
+  process.stdout.write('qwen-code-bot\\n');
+} else if (endpoint.endsWith('/pulls/7')) {
+  process.stdout.write(JSON.stringify({
+    state: 'open',
+    head: {
+      sha: process.env.RUN_HEAD_SHA,
+      repo: { full_name: process.env.RUN_HEAD_REPO },
+      ref: process.env.RUN_HEAD_BRANCH,
+    },
+  }));
+} else if (endpoint.endsWith('/issues/7/comments') && args.includes('--method')) {
+  process.stdout.write('[]\\n');
+}
+`,
+    );
+    writeExecutable(join(bin, 'sleep'), '#!/bin/sh\nexit 0\n');
+
+    const result = spawnSync('bash', ['-c', script], {
+      cwd: workspace,
+      encoding: 'utf8',
+      env: {
+        ...process.env,
+        PATH: `${bin}:${process.env.PATH}`,
+        CALL_LOG: callLog,
+        UPLOAD_SUCCEEDS: uploadSucceeds ? '1' : '0',
+        GH_TOKEN: 'test-token',
+        GITHUB_REPOSITORY: 'QwenLM/qwen-code',
+        GITHUB_WORKSPACE: workspace,
+        RUNNER_TEMP: runnerTemp,
+        RUN_ID: '99',
+        RUN_ATTEMPT: '2',
+        RUN_URL: 'https://run.example/99',
+        RUN_HEAD_SHA: '1234567890abcdef',
+        RUN_HEAD_REPO: 'fork/qwen-code',
+        RUN_HEAD_BRANCH: 'fix/visuals',
+        ALIYUN_OSS_BUCKET: 'assets-bucket',
+        ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test',
+        OSS_UPLOAD_ATTEMPT_TIMEOUT_MS: '120000',
+      },
+    });
+    const bodyPath = join(runnerTemp, 'visuals-comment.md');
+    return {
+      ...result,
+      body: readFileSync(bodyPath, 'utf8'),
+      calls: readFileSync(callLog, 'utf8'),
+      hostingStatus: readFileSync(
+        join(runnerTemp, 'visuals-hosting-status.txt'),
+        'utf8',
+      ).trim(),
+    };
+  } finally {
+    rmSync(root, { recursive: true, force: true });
+  }
+};
+
 test('sanitizeName preserves the extension (regression: a trailing char broke the .png filter)', () => {
   assert.equal(
     sanitizeName('session-transcript-light.png'),
@@ -582,6 +709,66 @@ test('buildComment lists a lone composite as one wide image (no light/dark table
   assert.doesNotMatch(body, /<td>/);
 });
 
+test('buildComment: hosting failure reports rendered assets without broken image links', () => {
+  const body = buildComment(['home-light.png', 'model-switch.gif'], {
+    hostingFailed: true,
+    runUrl: 'https://run.example/7',
+  });
+  assert.match(body, /failed to host/i);
+  assert.match(body, /home-light\.png/);
+  assert.match(body, /model-switch\.gif/);
+  assert.match(body, /\[workflow run\]\(https:\/\/run\.example\/7\)/);
+  assert.doesNotMatch(body, /failed to render/i);
+  assert.doesNotMatch(body, /<img /);
+});
+
+test('buildComment: hosting failure preserves render-failure caveat', () => {
+  const body = buildComment(['home-light.png', 'model-switch.gif'], {
+    hostingFailed: true,
+    renderIncomplete: true,
+    runUrl: 'https://run.example/7',
+  });
+  assert.match(body, /failed to render/i);
+  assert.match(body, /failed to host/i);
+  assert.match(body, /\[workflow run\]\(https:\/\/run\.example\/7\)/);
+  assert.doesNotMatch(body, /<img /);
+});
+
+test('comment CLI reads hosting failure status from the eighth argument', () => {
+  const root = mkdtempSync(join(tmpdir(), 'web-shell-visuals-'));
+  try {
+    const stageDir = join(root, 'stage');
+    const bodyFile = join(root, 'body.md');
+    const changedPathsFile = join(root, 'paths.txt');
+    const renderStatusFile = join(root, 'render-status.txt');
+    const hostingStatusFile = join(root, 'hosting-status.txt');
+    mkdirSync(stageDir);
+    writeFileSync(join(stageDir, 'home-light.png'), '');
+    writeFileSync(changedPathsFile, '');
+    writeFileSync(renderStatusFile, 'success\n');
+    writeFileSync(hostingStatusFile, 'failure\n');
+
+    execFileSync(process.execPath, [
+      publishScriptPath,
+      'comment',
+      stageDir,
+      'https://assets.example/pr',
+      'abc1234',
+      'https://run.example/7',
+      bodyFile,
+      changedPathsFile,
+      renderStatusFile,
+      hostingStatusFile,
+    ]);
+
+    const body = readFileSync(bodyFile, 'utf8');
+    assert.match(body, /failed to host/i);
+    assert.doesNotMatch(body, /<img /);
+  } finally {
+    rmSync(root, { recursive: true, force: true });
+  }
+});
+
 // --- Empty-preview triage (coverage gap vs. genuinely no visual effect) ---
 
 test('selectRenderShapingFiles keeps rendered .tsx/.css/.svg and drops logic/test/other-package edits', () => {
@@ -759,3 +946,37 @@ test('buildComment: render-failure note omits the run link when runUrl is absent
   assert.match(body, /failed to render/i);
   assert.doesNotMatch(body, /\[workflow run\]/); // no dangling empty link
 });
+
+test('publish workflow carries a hosting failure into the posted body', () => {
+  const renamedSection = publishStep().replace(
+    '# --- Post or update the PR comment',
+    '# Write the PR comment',
+  );
+  const result = runPublishStep({
+    uploadSucceeds: false,
+    script: renamedSection,
+  });
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(result.hostingStatus, 'failure');
+  assert.match(result.body, /failed to host/i);
+  assert.doesNotMatch(result.body, /<img /);
+  assert.match(
+    result.calls,
+    /api repos\/QwenLM\/qwen-code\/issues\/7\/comments -F body=@/,
+  );
+});
+
+test('publish workflow keeps successful hosting out of the failure path', () => {
+  const result = runPublishStep({ uploadSucceeds: true });
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(result.hostingStatus, 'success');
+  assert.doesNotMatch(result.body, /failed to host/i);
+  assert.match(
+    result.body,
+    /https:\/\/assets\.example\.test\/pr-assets\/web-shell-visuals\/7\/1234567890abcdef\/99\/2\/home-light\.png/,
+  );
+  assert.match(
+    result.calls,
+    /api repos\/QwenLM\/qwen-code\/issues\/7\/comments -F body=@/,
+  );
+});

.github/workflows/web-shell-visuals-publish.yml

@@ -67,9 +67,8 @@ jobs:
       - name: 'Install ossutil'
         id: 'install-ossutil'
         # Evidence hosting is best-effort: a setup failure must not block the
-        # no-image marker-comment path, which has no external dependency
-        # (mirrors the qwen-triage.yml twins). The upload below stays loud:
-        # with images present it still aborts the publish step.
+        # marker-comment path. When images exist, the publish step records the
+        # hosting failure and refreshes the comment without broken image URLs.
         continue-on-error: true
         env:
           OSSUTIL_URL: "${{ vars.OSSUTIL_URL || 'https://gosspublic.alicdn.com/ossutil/1.7.19/ossutil-v1.7.19-linux-amd64.zip' }}"
@@ -272,6 +271,8 @@ jobs:
           # skips the upload entirely; RAW_BASE stays empty and the
           # comment below is image-less ("no screenshot changes").
           RAW_BASE=''
+          HOSTING_STATUS_FILE="${RUNNER_TEMP}/visuals-hosting-status.txt"
+          echo 'success' > "${HOSTING_STATUS_FILE}"
           if [ "${HAS_IMAGES}" = "1" ]; then
             ASSET_PREFIX="pr-assets/web-shell-visuals/${PR}/${RUN_HEAD_SHA}/${RUN_ID}/${RUN_ATTEMPT}"
             # Same PATH discipline as qwen-triage.yml's publish-verify: never
@@ -283,15 +284,20 @@ jobs:
             # pool.
             node_bin="$(command -v node)"
             trusted_bin="$(mktemp -d)"
-            install -m 0755 "${RUNNER_TEMP}/ossutil" "$trusted_bin/ossutil"
-            PATH="$trusted_bin" "$node_bin" scripts/upload-aliyun-oss-assets.js \
-              --bucket "${ALIYUN_OSS_BUCKET}" \
-              --config "${RUNNER_TEMP}/.ossutilconfig" \
-              --prefix "${ASSET_PREFIX}" \
-              "${STAGE}"/*
+            if install -m 0755 "${RUNNER_TEMP}/ossutil" "$trusted_bin/ossutil" &&
+              PATH="$trusted_bin" "$node_bin" scripts/upload-aliyun-oss-assets.js \
+                --bucket "${ALIYUN_OSS_BUCKET}" \
+                --config "${RUNNER_TEMP}/.ossutilconfig" \
+                --prefix "${ASSET_PREFIX}" \
+                "${STAGE}"/*
+            then
+              RAW_BASE="${ALIYUN_OSS_PUBLIC_BASE_URL%/}/${ASSET_PREFIX}"
+              echo "Web-shell visuals hosted at ${RAW_BASE}."
+            else
+              echo "::warning::Failed to host web-shell visuals; updating the PR comment with a hosting-failure note."
+              echo 'failure' > "${HOSTING_STATUS_FILE}"
+            fi
             rm -rf "$trusted_bin"
-            RAW_BASE="${ALIYUN_OSS_PUBLIC_BASE_URL%/}/${ASSET_PREFIX}"
-            echo "Web-shell visuals hosted at ${RAW_BASE}."
           fi
 
           # --- Changed paths, for triaging an EMPTY preview ------------------
@@ -325,7 +331,8 @@ jobs:
             "${RAW_BASE}" \
             "${SHORT_SHA}" "${RUN_URL}" "${BODY_FILE}" \
             "${CHANGED_PATHS_FILE}" \
-            "${RENDER_STATUS_FILE}"
+            "${RENDER_STATUS_FILE}" \
+            "${HOSTING_STATUS_FILE}"
 
           # --- Post or update the PR comment --------------------------------
           # Dedup only against OUR OWN prior comment (bot author + marker), and