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
282 changes: 282 additions & 0 deletions .github/scripts/web-shell-visuals-publish.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
/**
* @license
* Copyright 2025 Qwen
* SPDX-License-Identifier: Apache-2.0
*/

/**
* Staging + comment generation for the web-shell visuals publish workflow.
*
* Extracted from the inline workflow so the image validation and comment
* construction — the parts that consume UNTRUSTED PR output and were
* previously untested — have unit coverage. (A shell sanitizer bug once
* appended `_` to every filename and silently produced an empty preview; the
* pure functions here are covered by web-shell-visuals-publish.test.mjs.)
*
* The pure helpers (`sanitizeName`, `classifyMagic`, `selectImages`,
* `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>
*/

import {
closeSync,
copyFileSync,
mkdirSync,
openSync,
readdirSync,
readSync,
statSync,
writeFileSync,
} from 'node:fs';
import { basename, join } from 'node:path';
import { pathToFileURL } from 'node:url';

// Bounds on UNTRUSTED artifact content: cap files EXAMINED (so a flood of junk
// can't burn the budget before valid files), files ACCEPTED, and per-file size.
export const MAX_CANDIDATES = 200;
export const MAX_SCREENSHOTS = 20;
export const MAX_GIFS = 6;
export const MAX_BYTES = 3 * 1024 * 1024;

const PNG_MAGIC = '89504e470d0a1a0a';
const GIF_MAGICS = new Set(['474946383961', '474946383761']); // GIF89a / GIF87a

const FLOW_LABELS = {
'model-switch': 'Open the slash menu and switch model',
'prompt-stream': 'Submit a prompt and watch the reply stream in',
};

/**
* Sanitize to the hosted-filename charset WITHOUT corrupting the extension.
* (The shell version captured `basename` through a pipe, turning its trailing
* newline into `_` and breaking the `.png`/`.gif` filter — this cannot.)
*/
export function sanitizeName(name) {
return String(name).replace(/[^A-Za-z0-9._-]/g, '_');
}
Comment on lines +56 to +58

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 safe-character set [A-Za-z0-9._-] includes ., so a filename like .hidden-light.png passes through unchanged. The publish workflow copies staged files with cp "${STAGE}"/* (bash glob, dotglob off by default), which does not match leading-dot entries. The dotfile is silently dropped — never committed to the asset branch — but the comment body (built from the stage directory) still references it, producing a broken image. — Fix: strip or replace a leading . in sanitizeName, or add shopt -s dotglob before the cp glob.

Suggested change
export function sanitizeName(name) {
return String(name).replace(/[^A-Za-z0-9._-]/g, '_');
}
export function sanitizeName(name) {
const safe = String(name).replace(/[^A-Za-z0-9._-]/g, '_');
return safe.startsWith('.') ? '_' + safe : safe;
}

— qwen3.7-max via Qwen Code /review


/** Classify by first-bytes magic hex → 'png' | 'gif' | null. */
export function classifyMagic(ext, magicHex) {
const hex = String(magicHex).toLowerCase();
if (ext === 'png') return hex.slice(0, 16) === PNG_MAGIC ? 'png' : null;
if (ext === 'gif') return GIF_MAGICS.has(hex.slice(0, 12)) ? 'gif' : null;
return null;
}

/**
* Pure selection over candidates `[{ name, ext, size, magic }]` (in order):
* apply the examined/accepted/size caps and magic validation. Returns
* `{ accepted: [{ name, safeName, kind }], warnings: string[] }`.
*/
export function selectImages(candidates, opts = {}) {
const maxCandidates = opts.maxCandidates ?? MAX_CANDIDATES;
const maxBytes = opts.maxBytes ?? MAX_BYTES;
Comment on lines +73 to +75

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] All selectImages tests use default cap values. No test exercises the custom-opts override path (e.g., selectImages(candidates, { maxScreenshots: 2, maxGifs: 1 })). The override logic (opts.maxCandidates ?? MAX_CANDIDATES, etc.) is untested exported API. — A single test passing reduced caps and asserting the smaller limits are honored would close this gap.

— qwen3.7-max via Qwen Code /review

// Per-kind caps so a large screenshot set can't starve the flow GIFs: a
// shared total cap over PNG-first candidates would let >=N screenshots
// silently drop every GIF from the preview.
const maxPerKind = {
png: opts.maxScreenshots ?? MAX_SCREENSHOTS,
gif: opts.maxGifs ?? MAX_GIFS,
};
const kindCount = { png: 0, gif: 0 };
const accepted = [];
const warnings = [];
let examined = 0;
for (const c of candidates) {
examined += 1;
if (examined > maxCandidates) {
warnings.push(`examined ${maxCandidates} candidate files; stopping`);
break;
}
if (c.size > maxBytes) {
warnings.push(`${c.name} exceeds ${maxBytes} bytes; skipping`);
continue;
}
const kind = classifyMagic(c.ext, c.magic);
if (!kind) {
warnings.push(`${c.name} is not a valid ${c.ext}; skipping`);
continue;
}
if (kindCount[kind] >= maxPerKind[kind]) {
warnings.push(
`reached the ${kind} cap (${maxPerKind[kind]}); skipping ${c.name}`,
);
continue;
}
kindCount[kind] += 1;
accepted.push({
name: c.name,
safeName: sanitizeName(basename(c.name)),
kind,
});
Comment on lines +109 to +113

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] selectImages does not de-duplicate by safeName. Two candidates whose names differ only in characters that sanitizeName collapses to _ (e.g., foo bar.png and foo_bar.png) both pass all caps and magic checks and enter accepted. In stageCli, the second copyFileSync silently overwrites the first. The accepted count is inflated by one, while the comment lists only the surviving file. — Concrete cost: the workflow's "should I post?" decision is based on the inflated count, and the overwrite is invisible.

Suggested change
accepted.push({
name: c.name,
safeName: sanitizeName(basename(c.name)),
kind,
});
const safeName = sanitizeName(basename(c.name));
if (seenSafeNames.has(safeName)) {
warnings.push(`skipped duplicate safeName: ${c.name}${safeName}`);
continue;
}
seenSafeNames.add(safeName);
accepted.push({
name: c.name,
safeName,
kind,
});

— qwen3.7-max via Qwen Code /review

}
return { accepted, warnings };
}

/** Self-defending HTML escaping for interpolated values. */
export const esc = (s) =>
String(s)
.replace(/&/g, '&amp;')
.replace(/"/g, '&quot;')
.replace(/</g, '&lt;')
.replace(/>/g, '&gt;');

export const pretty = (s) =>
s.replace(/[-_]+/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase());

/**
* Pure comment builder. `files` is the list of staged filenames (png + gif).
* `ctx` is `{ rawBase, shortSha, runUrl }`. Returns the markdown body.
*/
export function buildComment(files, ctx = {}) {
const rawBase = ctx.rawBase ?? '';
const shortSha = ctx.shortSha ?? '';
const runUrl = ctx.runUrl ?? '';
const url = (name) => `${rawBase}/${encodeURIComponent(name)}`;

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] encodeURIComponent is applied to filenames in the url() helper, but no test verifies that a filename with URL-unsafe characters is correctly encoded in the generated <img src> attributes. All test filenames are already URL-safe after sanitization. — Concrete cost: if sanitizeName were changed to allow a character that needs URL encoding (e.g., +), the generated image URLs would break silently.

— qwen3.7-max via Qwen Code /review


const shots = files.filter((f) => /\.png$/i.test(f));
const views = new Map();
for (const f of shots) {
const m = f.match(/^(.*)-(light|dark)\.png$/i);
if (!m) continue;
Comment on lines +142 to +143

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] PNGs without a -light or -dark suffix are silently dropped from the comment output. The file passes selectImages (valid magic, within caps), gets staged and uploaded, but buildComment's regex /^(.*)-(light|dark)\.png$/i skips it with continue. — Failure scenario: a contributor renames a screenshot spec so its output filename loses the theme suffix. The image is staged and pushed to the asset branch, but the PR comment silently omits it. The publisher reports success with no signal that an image was invisible.

Suggested change
const m = f.match(/^(.*)-(light|dark)\.png$/i);
if (!m) continue;
const m = f.match(/^(.*)-(light|dark)\.png$/i);
if (!m) {
warnings.push(`skipped unmatched PNG: ${f} (expected <view>-(light|dark).png)`);
continue;
}

— qwen3.7-max via Qwen Code /review

const [, view, theme] = m;
const entry = views.get(view) || {};
entry[theme.toLowerCase()] = f;
views.set(view, entry);
}
const gifs = files.filter((f) => /\.gif$/i.test(f)).sort();

const out = [];
out.push('<!-- qwen:web-shell-visuals -->');
out.push('### 🖼️ web-shell visual preview');
out.push(
`Auto-rendered from this PR head \`${esc(shortSha)}\` against a mock daemon (no real backend). Refreshes on every push.`,
);
out.push('');

if (views.size > 0) {
out.push('#### Screenshots · light / dark');
out.push('');
out.push('<table>');
out.push('<tr><th align="left">view</th><th>light</th><th>dark</th></tr>');
for (const [view, pair] of [...views.entries()].sort()) {
const light = pair.light
? `<img src="${url(pair.light)}" width="360" alt="${esc(view)} light">`
: '—';
const dark = pair.dark
? `<img src="${url(pair.dark)}" width="360" alt="${esc(view)} dark">`
: '—';
out.push(
`<tr><td valign="top"><sub>${esc(pretty(view))}</sub></td><td>${light}</td><td>${dark}</td></tr>`,
);
}
out.push('</table>');
out.push('');
}

if (gifs.length > 0) {
out.push('#### Flows');
out.push('');
for (const g of gifs) {
const key = g.replace(/\.gif$/i, '');
// Own-property only: `FLOW_LABELS[key]` would otherwise inherit
// Object.prototype members, so a `toString.gif` would render the function
// source as the label.
const label = Object.hasOwn(FLOW_LABELS, key)
? FLOW_LABELS[key]
: pretty(key);
out.push(`**${esc(label)}**`);
out.push('');
out.push(`<img src="${url(g)}" width="640" alt="${esc(key)} flow">`);
out.push('');
}
}

if (runUrl) {
out.push(
`<sub>Full-resolution recordings (.webm) are attached to the <a href="${esc(runUrl)}">workflow run</a>.</sub>`,
);
}
out.push('');
out.push('— _Qwen Code · web-shell visuals_');
return out.join('\n') + '\n';
}

// --- I/O layer (exercised by the CLI; not part of the unit-tested surface) ---

function readMagicHex(path, n = 8) {
const fd = openSync(path, 'r');
try {
const buf = Buffer.alloc(n);
const read = readSync(fd, buf, 0, n, 0);
return buf.subarray(0, read).toString('hex');
} finally {
closeSync(fd);
}
}

function gatherCandidates(dir, ext) {
let names;
try {
names = readdirSync(dir);
} catch {
return [];
}
return names
.filter((n) => n.toLowerCase().endsWith(`.${ext}`))
.sort()
.map((n) => {
const path = join(dir, n);
let size = Infinity;
let magic = '';
try {
size = statSync(path).size;
magic = readMagicHex(path);
} catch {
// Unreadable entry: leave size=Infinity/magic='' so it is skipped.
}
return { name: n, ext, size, magic, path };
});
}

function stageCli(screenshotsDir, gifsDir, stageDir) {
const candidates = [
...gatherCandidates(screenshotsDir, 'png'),
...gatherCandidates(gifsDir, 'gif'),
];
const { accepted, warnings } = selectImages(candidates);
for (const w of warnings) process.stderr.write(`::warning::${w}\n`);
mkdirSync(stageDir, { recursive: true });
const byName = new Map(candidates.map((c) => [c.name, c.path]));
for (const a of accepted) {
copyFileSync(byName.get(a.name), join(stageDir, a.safeName));
}
// stdout = accepted count (the workflow reads it to decide whether to post).
process.stdout.write(`${accepted.length}\n`);
}

function commentCli(stageDir, rawBase, shortSha, runUrl, bodyFile) {
let files = [];
try {
files = readdirSync(stageDir);
} catch {
// Missing stage dir → empty preview body.
}
const body = buildComment(files, { rawBase, shortSha, runUrl });
writeFileSync(bodyFile, body);
process.stderr.write(`Comment body: ${body.split('\n').length} lines.\n`);
}

if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) {
const [cmd, ...rest] = process.argv.slice(2);
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]);
} else {
process.stderr.write(`unknown command: ${cmd ?? '(none)'}\n`);
process.exit(2);
}
}
Loading
Loading