Fix visual failure reporting and unstable captures

.github/workflows/_static-checks.yaml

@@ -34,8 +34,8 @@ jobs:
       - name: Test release PR body renderer
         run: node --test scripts/upstream-release/render-pr-body.test.mjs
 
-      - name: Test screenshot summary updater
-        run: node --test scripts/update-pr-screenshot-summary.test.mjs
+      - name: Test screenshot summary helpers
+        run: node --test scripts/update-pr-screenshot-summary.test.mjs scripts/pr-screenshot-workflow.test.mjs
 
       - name: Test Helm reference generator
         run: node --test scripts/upstream-release/generate-helm-reference.test.mjs

.github/workflows/pr-screenshot-summary.yaml

@@ -95,16 +95,20 @@ jobs:
           JOBS_FILE="$RUNNER_TEMP/jobs.json"
           ARTIFACTS_FILE="$RUNNER_TEMP/artifacts.json"
           gh api "repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" > "$JOBS_FILE"
-          gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
           CONCLUSION="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].conclusion // "missing"' "$JOBS_FILE")"
           JOB_URL="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].html_url // ""' "$JOBS_FILE")"
-          jq '.artifacts |= ([.artifacts[] | select(.name == "visual-failure-details" and .expired == false)] + [.artifacts[] | select(.name == "playwright-report" and .expired == false)])' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
+          # Preserve the conclusion so the always() comment step can still
+          # post a fallback if artifact discovery or parsing fails.
+          {
+            echo "conclusion=$CONCLUSION"
+            echo "job_url=$JOB_URL"
+          } >> "$GITHUB_OUTPUT"
+          gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
+          jq '.artifacts = ([.artifacts[] | select(.name == "visual-failure-details" and .expired == false)] + [.artifacts[] | select(.name == "playwright-report" and .expired == false)])' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
           mv "$ARTIFACTS_FILE.selected" "$ARTIFACTS_FILE"
           ARTIFACT_ID="$(jq -r '.artifacts[0].id // ""' "$ARTIFACTS_FILE")"
           ARTIFACT_SIZE="$(jq -r '.artifacts[0].size_in_bytes // "0"' "$ARTIFACTS_FILE")"
           {
-            echo "conclusion=$CONCLUSION"
-            echo "job_url=$JOB_URL"
             echo "artifact_id=$ARTIFACT_ID"
             echo "artifact_size=$ARTIFACT_SIZE"
           } >> "$GITHUB_OUTPUT"

scripts/pr-screenshot-workflow.test.mjs

@@ -0,0 +1,137 @@
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+import assert from 'node:assert/strict';
+import { spawnSync } from 'node:child_process';
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import test from 'node:test';
+import { parse } from 'yaml';
+
+const workflow = parse(
+  fs.readFileSync(
+    new URL('../.github/workflows/pr-screenshot-summary.yaml', import.meta.url),
+    'utf8'
+  )
+);
+const resolveStep = workflow.jobs['update-description'].steps.find(
+  (step) => step.id === 'visual'
+);
+
+function resolveArtifacts(t, artifacts, { unavailable = false } = {}) {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-job-'));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const bin = path.join(root, 'bin');
+  fs.mkdirSync(bin);
+  fs.writeFileSync(
+    path.join(root, 'jobs.json.fixture'),
+    JSON.stringify({
+      jobs: [
+        {
+          name: 'Static checks / Lint and format checks',
+          conclusion: 'success',
+        },
+        {
+          name: 'Visual regression / Playwright visual snapshots',
+          conclusion: 'failure',
+          html_url: 'https://github.com/example/docs/actions/runs/1/job/2',
+        },
+      ],
+    })
+  );
+  fs.writeFileSync(
+    path.join(root, 'artifacts.json.fixture'),
+    typeof artifacts === 'string' ? artifacts : JSON.stringify({ artifacts })
+  );
+  fs.writeFileSync(
+    path.join(bin, 'gh'),
+    `#!/bin/sh
+case "$2" in
+  */jobs?*) cat "$RUNNER_TEMP/jobs.json.fixture" ;;
+  */artifacts?*)
+    if [ "$ARTIFACTS_UNAVAILABLE" = true ]; then exit 1; fi
+    cat "$RUNNER_TEMP/artifacts.json.fixture" ;;
+  *) exit 1 ;;
+esac
+`,
+    { mode: 0o755 }
+  );
+  const outputFile = path.join(root, 'outputs');
+  const result = spawnSync('bash', ['-eo', 'pipefail', '-c', resolveStep.run], {
+    encoding: 'utf8',
+    env: {
+      ...process.env,
+      PATH: `${bin}${path.delimiter}${process.env.PATH}`,
+      RUNNER_TEMP: root,
+      GITHUB_OUTPUT: outputFile,
+      REPO: 'example/docs',
+      RUN_ID: '1',
+      ARTIFACTS_UNAVAILABLE: String(unavailable),
+    },
+  });
+  const outputs = Object.fromEntries(
+    fs
+      .readFileSync(outputFile, 'utf8')
+      .trim()
+      .split('\n')
+      .map((line) => {
+        const separator = line.indexOf('=');
+        return [line.slice(0, separator), line.slice(separator + 1)];
+      })
+  );
+  assert.equal(outputs.conclusion, 'failure');
+  assert.equal(
+    outputs.job_url,
+    'https://github.com/example/docs/actions/runs/1/job/2'
+  );
+  return { result, outputs };
+}
+
+const fullReport = {
+  name: 'playwright-report',
+  expired: false,
+  id: 10,
+  size_in_bytes: 55_544_517,
+};
+const compactReport = {
+  name: 'visual-failure-details',
+  expired: false,
+  id: 20,
+  size_in_bytes: 21_006,
+};
+
+test('selects compact details ahead of a large full report', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [fullReport, compactReport]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '20');
+  assert.equal(outputs.artifact_size, '21006');
+});
+
+test('falls back to the full report when compact details are expired', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, expired: true },
+    fullReport,
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '10');
+});
+
+test('preserves the failure conclusion when no artifact is available', (t) => {
+  const { result, outputs } = resolveArtifacts(t, []);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '');
+  assert.equal(outputs.artifact_size, '0');
+});
+
+test('preserves the failure conclusion when artifact discovery fails', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [], { unavailable: true });
+  assert.notEqual(result.status, 0);
+  assert.equal(outputs.artifact_id, undefined);
+});
+
+test('preserves the failure conclusion when artifact parsing fails', (t) => {
+  const { result, outputs } = resolveArtifacts(t, '{malformed');
+  assert.notEqual(result.status, 0);
+  assert.equal(outputs.artifact_id, undefined);
+});