From b53a842e1394a4bea50970d51ecae21de30f832a Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 30 Jul 2026 16:58:19 +0800 Subject: [PATCH 1/6] feat(verify-pr): ship a one-command capture helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth attempt at the same failure, and the first one aimed at the real cause. The score so far: #8016 captures were "Optionally … when text cannot carry the oracle" -> 0 images / 14 runs #8104 captures became budget item 4 in Scope selection -> 0 images / 1 run That last run is the one that settles it. Same PR (#7975), browser installed and working ("Install evidence browser: success"), and the verification got DEEPER — 64 assertions against 40, 53 tables against 31 — while still producing zero images. An agent reading the instruction, doing more work than before, and still not capturing is not an agent that missed the instruction. The cause is one I should have checked when I wrote #8016: the skill sent the agent to build node-pty -> xterm.js -> Playwright itself, and `node-pty` is not a dependency of this repo. It needs a native build. The `playwright` package is not a declared dependency either. So the documented route did not exist, and the incentive was entirely against trying it: authoring that pipeline risks failing and eats budget, while skipping costs nothing and is invisible. `scripts/verify-capture.mjs` makes a capture one command using deps that are already installed: node scripts/verify-capture.mjs --out evidence/01-ab.png \ --title 'A/B: the gate flips' -- node my-harness.mjs Command (or stdin) -> @xterm/headless parses the ANSI into a cell grid with colour and bold -> SVG -> sharp rasterises. No browser, no pseudo-terminal. A non-zero exit from the captured command still produces an image, because capturing a failing base arm is the normal case. The skill's dead route is removed and replaced with that command, and the budget line drops from ~5 minutes to ~2 because there is no pipeline to author. QWEN_VERIFY_CHROMIUM and its install stay for a future web-UI capture, but the terminal route no longer depends on them — gating on a browser the route does not use is how an absent browser turns into a skipped capture. Mutation-verified 7/7 against the real helper and the real PNGs: bare LF (staircase render), no event-loop turn (blank capture), dropped bold, dropped colour, no blank-row trimming, tolerating an empty capture, and dropped geometry validation. Two of those initially SURVIVED. The colour test compared "coloured and bold" against plain and asserted the bytes differ — which passes while EITHER attribute survives. That is the wrong-reason trap this skill warns about, met in the skill's own test file. Each attribute is now isolated against the same plain baseline (green-no-bold, bold-no-colour), and both mutations kill. 119/119 across both suites; prettier and eslint clean. --- .qwen/skills/verify-pr/SKILL.md | 45 ++-- scripts/tests/qwen-triage-workflow.test.js | 20 +- scripts/tests/verify-capture.test.js | 232 ++++++++++++++++++++ scripts/verify-capture.mjs | 244 +++++++++++++++++++++ 4 files changed, 519 insertions(+), 22 deletions(-) create mode 100644 scripts/tests/verify-capture.test.js create mode 100644 scripts/verify-capture.mjs diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 2b75bfe1071..0018f3cdc53 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -104,13 +104,15 @@ secondary claims. Budget by value: 1. **A/B load-bearing proof of the central claim** (always, ~half the budget). 2. **One or two wire-oracle harnesses** on the changed surface. 3. **Targeted gates**: tests/typecheck of the affected workspace(s) only. -4. **Capture the A/B and the matrix as they print** (~5 minutes, whenever - `QWEN_VERIFY_CHROMIUM=1`). This is a budget line, not an afterthought: - two live runs with the browser installed and working produced **zero** - images, because the instruction lived in the artifact contract while the - plan the agent follows is this list. Decide here how many captures the - round needs — normally two, at most a handful — and reserve the time. - See the artifact contract for the mechanics and the naming rule. +4. **Capture the A/B and the matrix as they print** — one command each, + `node scripts/verify-capture.mjs --out …/01-ab.png -- `, so budget + ~2 minutes, not the ~5 an ad-hoc pipeline would need. This is a budget + line, not an afterthought: **four live runs produced zero images**, first + because the instruction was worded as optional, then because it lived in + the artifact contract while the plan the agent follows is this list, and + underneath both because the pipeline it named did not exist. Decide here + how many captures the round needs — normally two, at most a handful — and + reserve the time. Mechanics and the naming rule: artifact contract. Everything else is explicitly out of scope — and is **listed as not covered** in the report. Never let breadth eat the A/B: one proven load-bearing claim @@ -541,15 +543,28 @@ workflow globs). It must contain: headline number. One capture of the terminal showing `2999 → 0` is worth more than the sentence asserting it. - **Chromium is pre-installed for you** when `QWEN_VERIFY_CHROMIUM=1` is set; - `PLAYWRIGHT_BROWSERS_PATH` already points at it. Do **not** run - `playwright install` — you run as `node` with a fresh `HOME` and no apt - rights, so it downloads ~170 MB and then fails on system deps. If - `QWEN_VERIFY_CHROMIUM` is unset the capability is unavailable in this run: - ship the text-only report and note it under _Not covered_ in one line, do - not spend budget working around it. + **One command, already wired — do not build a capture pipeline.** + + ```bash + node scripts/verify-capture.mjs --out tmp/pr-verify-/evidence/01-ab.png \ + --title 'A/B: the gate flips on noisy data' -- node my-harness.mjs + # or pipe: my-harness | node scripts/verify-capture.mjs --out …/02-matrix.png + ``` + + It runs the command, parses its ANSI through `@xterm/headless`, and + rasterises the cell grid with `sharp` — colour and bold preserved, **no + browser and no pseudo-terminal**. A non-zero exit from the captured command + is fine and often the point: capturing a failing base arm is normal. Options + that matter: `--cols` (default 100) to stop wrapping, `--title` for the + caption, `--rows` to cap height. + + Earlier versions of this section sent you to build node-pty → xterm → + Playwright yourself. **That route did not exist** — `node-pty` is not a + dependency of this repo and needs a native build — and four live runs + produced zero images because of it. If `verify-capture.mjs` is missing or + fails, say so under _Not covered_ in one line and ship the text-only report; + do not reconstruct the pipeline by hand. - Route: `terminal-capture` skill (node-pty → xterm.js → Playwright PNG). The publish job hosts what you produce on a per-PR branch (`pr-assets/-verify`) and appends it below the report, capped at **8 images, 2 MB each**; anything diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index f7818381258..af0f2fa66f5 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -3164,13 +3164,17 @@ describe('qwen-triage verify round-3 hardening', () => { 'QWEN_VERIFY_CHROMIUM', ); - // And the skill must stop calling captures optional, must name the - // gate, and must forbid the install the agent cannot complete. + // And the skill must stop calling captures optional. const flat = verifySkill.replace(/\s+/g, ' '); expect(flat).toContain('Produce these whenever you ran a harness'); - expect(flat).toContain('QWEN_VERIFY_CHROMIUM=1'); - expect(flat).toContain('Do **not** run `playwright install`'); expect(flat).not.toContain('Optionally `evidence/*.png`'); + // The TERMINAL capture route no longer depends on this browser at all — + // it is @xterm/headless + sharp, so the skill must not gate captures on + // QWEN_VERIFY_CHROMIUM or the agent skips when the browser is absent. + // The variable and its install stay for a future web-UI capture; see + // scripts/verify-capture.mjs for why the terminal route needs neither. + expect(flat).toContain('node scripts/verify-capture.mjs --out'); + expect(flat).not.toContain('Route: `terminal-capture` skill'); }); // The browser step's require.resolve + cli.js join is otherwise guarded @@ -3395,11 +3399,13 @@ describe('qwen-triage verify round-3 hardening', () => { ); // A numbered budget item alongside the A/B, harnesses and gates. expect(scope).toMatch(/^4\. \*\*Capture/m); - expect(scope).toContain('QWEN_VERIFY_CHROMIUM=1'); + // The command, so budgeting does not mean budgeting for pipeline + // authoring — that is what made this cost ~5 minutes and never happen. + expect(scope).toContain('node scripts/verify-capture.mjs'); + expect(scope).toContain('~2 minutes'); // Time reserved, and the failure that motivated it named — a rule // stated without its failure reads as advice and gets skipped. - expect(scope).toContain('~5 minutes'); - expect(scope).toContain('produced **zero**'); + expect(scope).toContain('four live runs produced zero images'); // Bounded: the cap exists so "budget it" does not become eight images. expect(scope).toMatch(/normally two, at most a handful/); diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js new file mode 100644 index 00000000000..473e4b034ac --- /dev/null +++ b/scripts/tests/verify-capture.test.js @@ -0,0 +1,232 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../..', +); +const HELPER = path.join(repoRoot, 'scripts/verify-capture.mjs'); +const ESC = String.fromCharCode(27); + +// Every assertion here runs the real helper and inspects the real PNG. The +// point of this script is that the pipeline it replaces did NOT exist — four +// live /verify runs produced zero images because the skill named node-pty and +// Playwright, neither of which is installed. A test that only checked the +// skill's wording would have passed through all four of those rounds. +describe('verify-capture helper', () => { + const run = (args, opts = {}) => + spawnSync('node', [HELPER, ...args], { + cwd: repoRoot, + encoding: 'utf8', + ...opts, + }); + + /** PNG signature — proves sharp actually rasterised, not that a file exists. */ + const isPng = (file) => { + const head = readFileSync(file).subarray(0, 8); + return head.equals( + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + ); + }; + + const withDir = (fn) => { + const dir = mkdtempSync(path.join(tmpdir(), 'verify-capture-')); + try { + return fn(dir); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + + /** A harness that emits a coloured A/B table, like a real one would. */ + const harness = (dir) => { + const file = path.join(dir, 'harness.mjs'); + writeFileSync( + file, + [ + `const E = String.fromCharCode(27);`, + `const g = (s) => \`\${E}[32m\${s}\${E}[0m\`;`, + `const r = (s) => \`\${E}[1;31m\${s}\${E}[0m\`;`, + `console.log('cell base head');`, + `console.log(\`noisy \${r('FAIL')} \${g('PASS')}\`);`, + `console.log(\`clean \${g('PASS')} \${g('PASS')}\`);`, + ].join('\n'), + ); + return file; + }; + + it('captures a command to a real PNG', () => + withDir((dir) => { + const out = path.join(dir, 'evidence/01-ab.png'); + const res = run([ + '--out', + out, + '--cols', + '40', + '--title', + 'A/B: gate flips', + '--', + 'node', + harness(dir), + ]); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + // Geometry is reported so a caller can spot a blank or clipped capture. + expect(res.stdout).toMatch(/\d+x\d+ \d+B 3 rows/); + // Parent dirs are created — the skill tells the agent to write into + // evidence/, which will not exist yet. + expect(readFileSync(out).length).toBeGreaterThan(1000); + })); + + it('accepts piped input as well as a command', () => + withDir((dir) => { + const out = path.join(dir, 'piped.png'); + const res = run(['--out', out, '--cols', '40'], { + input: `${ESC}[32mPASS${ESC}[0m 64/64\n`, + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + })); + + // Capturing a failing base arm is the normal case for an A/B cell, so a + // non-zero exit from the captured command must still produce an image. + it('still captures when the captured command fails', () => + withDir((dir) => { + const out = path.join(dir, 'failing.png'); + const res = run([ + '--out', + out, + '--', + 'node', + '-e', + 'console.log("boom"); process.exit(3)', + ]); + expect(res.status).toBe(0); + expect(res.stderr).toContain('command exited 3'); + expect(isPng(out)).toBe(true); + })); + + // Colour and weight are the whole reason to render rather than paste text: + // a red FAIL beside a green PASS is what makes the cell readable at a + // glance. + // + // Each attribute is isolated against the SAME plain baseline. A single + // "coloured and bold vs plain" comparison passes while EITHER attribute + // survives — verified: mutating colour away, and mutating bold away, both + // left that version green. This is the wrong-reason trap the skill warns + // about, met in this file's own test. + it('preserves colour and bold independently', () => + withDir((dir) => { + const render = (name, input) => { + const out = path.join(dir, `${name}.png`); + expect(run(['--out', out, '--cols', '30'], { input }).status).toBe(0); + expect(isPng(out)).toBe(true); + return readFileSync(out); + }; + const plain = render('plain', 'FAIL PASS\n'); + // Green, no bold — differs from plain ONLY if colour is applied. + const colourOnly = render('colour', `${ESC}[32mFAIL PASS${ESC}[0m\n`); + // Bold, no colour — differs from plain ONLY if weight is applied. + const boldOnly = render('bold', `${ESC}[1mFAIL PASS${ESC}[0m\n`); + expect(colourOnly.equals(plain), 'colour was dropped').toBe(false); + expect(boldOnly.equals(plain), 'bold was dropped').toBe(false); + // ...and the two attributes must not collapse onto the same rendering. + expect(colourOnly.equals(boldOnly)).toBe(false); + })); + + // A bare LF leaves xterm's cursor in the old column, so line 2 renders + // indented by line 1's width and the capture looks like a staircase. The + // helper normalises to CRLF; assert the rendered height matches the line + // count rather than trusting that. + it('renders one row per line, not a staircase', () => + withDir((dir) => { + const out = path.join(dir, 'rows.png'); + const res = run(['--out', out, '--cols', '20'], { + input: 'aaa\nbbb\nccc\nddd\n', + }); + expect(res.status).toBe(0); + expect(res.stdout).toContain('4 rows'); + })); + + it('trims trailing blank rows instead of padding to --rows', () => + withDir((dir) => { + const tall = run(['--out', path.join(dir, 'a.png'), '--rows', '40'], { + input: 'one\ntwo\n', + }); + const short = run(['--out', path.join(dir, 'b.png'), '--rows', '4'], { + input: 'one\ntwo\n', + }); + expect(tall.stdout).toContain('2 rows'); + // Same content, same image, regardless of the row cap. + expect(tall.stdout.split(' ')[1]).toBe(short.stdout.split(' ')[1]); + })); + + describe('fails loudly rather than writing a broken image', () => { + it('rejects a missing --out', () => { + const res = run(['--', 'echo', 'hi']); + expect(res.status).toBe(1); + expect(res.stderr).toContain('--out is required'); + }); + + it('rejects nonsense geometry', () => + withDir((dir) => { + for (const bad of ['0', '-5', 'abc', '9999']) { + const res = run(['--out', path.join(dir, 'x.png'), '--cols', bad]); + expect(res.status, `--cols ${bad} was accepted`).toBe(1); + expect(res.stderr).toContain('must be an integer'); + } + })); + + it('rejects an unknown option instead of ignoring it', () => + withDir((dir) => { + const res = run(['--out', path.join(dir, 'x.png'), '--width', '80']); + expect(res.status).toBe(1); + expect(res.stderr).toContain('unknown option --width'); + })); + + // A blank capture is worse than none: it looks like evidence and shows + // nothing. Exit 1 so the agent notices rather than publishing it. + it('refuses to write an empty capture', () => + withDir((dir) => { + const res = run(['--out', path.join(dir, 'empty.png')], { input: '' }); + expect(res.status).toBe(1); + expect(res.stderr).toContain('nothing to render'); + })); + + it('reports a command that does not exist', () => + withDir((dir) => { + const res = run([ + '--out', + path.join(dir, 'x.png'), + '--', + 'definitely-not-a-real-binary-xyz', + ]); + expect(res.status).toBe(1); + })); + }); + + // The helper only helps if the skill points at it. Three rounds of + // rewording failed because the named pipeline did not exist; assert the + // dead route is gone and the live one is named. + it('is the route the skill actually names', () => { + const skill = readFileSync( + path.join(repoRoot, '.qwen/skills/verify-pr/SKILL.md'), + 'utf8', + ); + expect(skill).toContain('node scripts/verify-capture.mjs --out'); + expect(skill).toContain('no\n browser and no pseudo-terminal'); + // The route that never existed must not be recommended again. + expect(skill).not.toMatch(/Route: `terminal-capture` skill/); + expect(skill).not.toMatch(/node-pty → xterm\.js → Playwright PNG/); + }); +}); diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs new file mode 100644 index 00000000000..13b37f5ae35 --- /dev/null +++ b/scripts/verify-capture.mjs @@ -0,0 +1,244 @@ +#!/usr/bin/env node + +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Render a command's terminal output to a PNG, for `/verify` evidence images. + * + * Why this exists: the `verify-pr` skill asked the agent to build its own + * node-pty -> xterm.js -> Playwright pipeline. Neither `node-pty` nor the + * `playwright` package is a dependency of this repo, and node-pty needs a + * native build — so the documented route did not exist, and three rounds of + * rewording the instruction produced zero images across four live runs. This + * turns a capture into one command using deps that are already installed. + * + * Pipeline: run the command, feed its bytes to @xterm/headless (which parses + * ANSI into a cell grid with colour and bold attributes), emit that grid as + * SVG, and let sharp rasterise it. No browser, no pseudo-terminal. + * + * Usage: + * node scripts/verify-capture.mjs --out evidence/01-ab.png -- npm test -w pkg + * some-harness | node scripts/verify-capture.mjs --out evidence/02-matrix.png + * + * Options: + * --out required; parent dirs are created + * --cols terminal width (default 100) + * --rows max rows kept (default 40, trailing blanks trimmed) + * --title caption drawn above the output + * + * Exit codes: 0 on a written PNG, 1 on usage or render failure. The captured + * command's own exit code is reported on stderr but does NOT fail the capture — + * a failing command is usually exactly what is being captured. + */ + +import { spawnSync } from 'node:child_process'; +import { mkdirSync, readFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { dirname, resolve } from 'node:path'; + +const require = createRequire(import.meta.url); + +// xterm's headless build is CommonJS; a named ESM import of `Terminal` throws. +const { Terminal } = require('@xterm/headless'); +const sharp = require('sharp'); + +// The 16 ANSI colours as xterm reports them from getFgColor()/getBgColor(). +const ANSI = [ + '#1e1e1e', + '#cd3131', + '#0dbc79', + '#e5e510', + '#2472c8', + '#bc3fbc', + '#11a8cd', + '#e5e5e5', + '#666666', + '#f14c4c', + '#23d18b', + '#f5f543', + '#3b8eea', + '#d670d6', + '#29b8db', + '#ffffff', +]; +const FG_DEFAULT = '#d4d4d4'; +const BG = '#1e1e1e'; +const CELL_W = 8.4; +const CELL_H = 18; +const PAD = 12; +const FONT_SIZE = 14; + +function usage(message) { + process.stderr.write(`verify-capture: ${message}\n`); + process.stderr.write( + 'usage: verify-capture.mjs --out [--cols n] [--rows n] [--title s] [-- cmd ...]\n', + ); + process.exit(1); +} + +function parseArgs(argv) { + const opts = { cols: 100, rows: 40, out: '', title: '' }; + const cmd = []; + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i]; + if (arg === '--') { + cmd.push(...argv.slice(i + 1)); + break; + } + const next = () => { + i += 1; + if (i >= argv.length) usage(`${arg} needs a value`); + return argv[i]; + }; + switch (arg) { + case '--out': + opts.out = next(); + break; + case '--cols': + opts.cols = Number(next()); + break; + case '--rows': + opts.rows = Number(next()); + break; + case '--title': + opts.title = next(); + break; + default: + usage(`unknown option ${arg}`); + } + } + if (!opts.out) usage('--out is required'); + // Guard the geometry: a NaN or absurd value would otherwise reach sharp as a + // broken SVG and fail with something unrelated to the real mistake. + for (const key of ['cols', 'rows']) { + if (!Number.isInteger(opts[key]) || opts[key] < 1 || opts[key] > 500) { + usage(`--${key} must be an integer between 1 and 500`); + } + } + return { opts, cmd }; +} + +const escapeXml = (s) => + s.replace( + /[<>&"]/g, + (c) => ({ '<': '<', '>': '>', '&': '&', '"': '"' })[c], + ); + +/** Collect the bytes to render: either a child command's output, or stdin. */ +function collectOutput(cmd) { + if (cmd.length === 0) { + try { + return readFileSync(0, 'utf8'); + } catch { + usage('no command given and stdin is empty'); + } + } + const res = spawnSync(cmd[0], cmd.slice(1), { + encoding: 'utf8', + maxBuffer: 32 * 1024 * 1024, + // Ask for colour without a pty: most tools honour one of these, and a + // purpose-built harness emits ANSI unconditionally anyway. + env: { ...process.env, FORCE_COLOR: '1', CLICOLOR_FORCE: '1' }, + }); + if (res.error) { + process.stderr.write(`verify-capture: ${res.error.message}\n`); + process.exit(1); + } + // A non-zero exit is not a capture failure — capturing a failing base arm is + // the normal case for an A/B cell. + process.stderr.write(`verify-capture: command exited ${res.status}\n`); + return `${res.stdout ?? ''}${res.stderr ?? ''}`; +} + +/** Parse ANSI into a cell grid, then emit it as SVG. */ +async function render(raw, opts) { + const term = new Terminal({ + cols: opts.cols, + rows: opts.rows, + scrollback: 0, + allowProposedApi: true, + }); + // xterm needs CRLF; a bare LF leaves the cursor in the old column and every + // line after the first renders indented by the previous line's length. + term.write(raw.replace(/\r?\n/g, '\r\n')); + // write() is asynchronous internally; without a turn of the loop the buffer + // is still empty and the capture silently comes out blank. + await new Promise((r) => setTimeout(r, 120)); + + const buf = term.buffer.active; + const rows = []; + for (let y = 0; y < opts.rows; y += 1) { + const line = buf.getLine(y); + if (!line) break; + const cells = []; + for (let x = 0; x < opts.cols; x += 1) { + const cell = line.getCell(x); + const chars = cell?.getChars(); + if (!chars) continue; + cells.push({ + x, + chars, + fg: cell.getFgColor(), + bold: cell.isBold() !== 0, + blank: chars === ' ', + }); + } + rows.push(cells); + } + // Trim trailing blank rows so a 40-row default does not pad every capture + // with empty space. + while (rows.length > 0 && rows.at(-1).every((c) => c.blank)) rows.pop(); + if (rows.length === 0) { + process.stderr.write('verify-capture: nothing to render (empty output)\n'); + process.exit(1); + } + + const titleRows = opts.title ? 1 : 0; + const width = Math.round(PAD * 2 + opts.cols * CELL_W); + const height = PAD * 2 + (rows.length + titleRows) * CELL_H; + let body = ''; + if (opts.title) { + body += + `${escapeXml(opts.title)}`; + } + rows.forEach((cells, y) => { + const baseline = PAD + (y + titleRows + 1) * CELL_H - 5; + for (const cell of cells) { + if (cell.blank) continue; + const colour = + cell.fg >= 0 && cell.fg < ANSI.length ? ANSI[cell.fg] : FG_DEFAULT; + body += + `` + + `${escapeXml(cell.chars)}`; + } + }); + + const svg = + `` + + `` + + `${body}`; + + const out = resolve(opts.out); + mkdirSync(dirname(out), { recursive: true }); + const info = await sharp(Buffer.from(svg)) + .png({ compressionLevel: 9 }) + .toFile(out); + process.stdout.write( + `${out} ${info.width}x${info.height} ${info.size}B ${rows.length} rows\n`, + ); +} + +const { opts, cmd } = parseArgs(process.argv.slice(2)); +try { + await render(collectOutput(cmd), opts); +} catch (error) { + process.stderr.write(`verify-capture: ${error?.message ?? error}\n`); + process.exit(1); +} From 4c2206a9527cb24f40987728b0231845d05406b1 Mon Sep 17 00:00:00 2001 From: Qwen Code Autofix Date: Thu, 30 Jul 2026 11:44:56 +0000 Subject: [PATCH 2/6] fix(verify-pr): harden capture helper per review (#8114) - Strip U+FE0F before rasterising: the emoji variation selector made Pango abort() in native code (SIGTRAP, no PNG, no diagnostic) when no colour-emoji font exists; the base codepoint renders. Add a non-ASCII regression test. - Await xterm's write callback instead of a fixed 120ms sleep, so a large capture is not read mid-parse and silently come out blank. - Name a signal-killed child ("killed by SIGKILL") rather than "exited null". - Warn on stderr when input is taller than --rows and the top is dropped. - Correct the falsified "route did not exist" rationale: the browser pipeline's deps do resolve from this repo; the real fragility is that integration-tests/terminal-capture is not a root workspace. Keep a skill pointer to terminal-capture for TUI/web-UI captures this helper cannot do. - Qualify the colour claim (16 base ANSI colours; 256/truecolor fall back). - Tests: feed both --cols and --rows to the geometry guard, exercise escapeXml, flatten the SKILL.md assertion against reflow, and replace the platform-fragile PNG byte-length check (flaked at 846B on Linux vs >1000B on macOS) with the deterministic canvas geometry. --- .qwen/skills/verify-pr/SKILL.md | 20 ++++-- scripts/tests/verify-capture.test.js | 99 +++++++++++++++++++++------- scripts/verify-capture.mjs | 49 ++++++++++---- 3 files changed, 125 insertions(+), 43 deletions(-) diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 0018f3cdc53..a2af6f90ea8 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -552,16 +552,22 @@ workflow globs). It must contain: ``` It runs the command, parses its ANSI through `@xterm/headless`, and - rasterises the cell grid with `sharp` — colour and bold preserved, **no + rasterises the cell grid with `sharp` — the 16 base ANSI colours and bold + preserved (256-colour and truecolor fall back to the default grey), **no browser and no pseudo-terminal**. A non-zero exit from the captured command is fine and often the point: capturing a failing base arm is normal. Options that matter: `--cols` (default 100) to stop wrapping, `--title` for the - caption, `--rows` to cap height. - - Earlier versions of this section sent you to build node-pty → xterm → - Playwright yourself. **That route did not exist** — `node-pty` is not a - dependency of this repo and needs a native build — and four live runs - produced zero images because of it. If `verify-capture.mjs` is missing or + caption, `--rows` to cap height (output taller than `--rows` keeps the tail + and warns on stderr that the top was dropped). + + This helper covers flat command output only: it gives the captured command + no TTY, so it cannot render an ink TUI or a browser page; for a TUI or + web-UI capture, see the `terminal-capture` skill. Earlier versions of this + section sent you to build that browser pipeline yourself. Its dependencies + do resolve from this repo, but it needs a browser, is slower, and is wired + fragilely (integration-tests/terminal-capture is not a root workspace, so + its package.json is never installed as a unit), and four live runs produced + zero images. Prefer this one command. If `verify-capture.mjs` is missing or fails, say so under _Not covered_ in one line and ship the text-only report; do not reconstruct the pipeline by hand. diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js index 473e4b034ac..c394bcc0726 100644 --- a/scripts/tests/verify-capture.test.js +++ b/scripts/tests/verify-capture.test.js @@ -19,10 +19,10 @@ const HELPER = path.join(repoRoot, 'scripts/verify-capture.mjs'); const ESC = String.fromCharCode(27); // Every assertion here runs the real helper and inspects the real PNG. The -// point of this script is that the pipeline it replaces did NOT exist — four -// live /verify runs produced zero images because the skill named node-pty and -// Playwright, neither of which is installed. A test that only checked the -// skill's wording would have passed through all four of those rounds. +// point of this script is that the browser pipeline it replaces never produced +// an image in four live /verify runs — it needed a browser and a route that is +// not installed as a unit. A test that only checked the skill's wording would +// have passed through all four of those rounds. describe('verify-capture helper', () => { const run = (args, opts = {}) => spawnSync('node', [HELPER, ...args], { @@ -80,12 +80,15 @@ describe('verify-capture helper', () => { harness(dir), ]); expect(res.status).toBe(0); + // isPng reads from evidence/01-ab.png, so a pass also proves the helper + // created the parent dir the skill tells the agent to write into. expect(isPng(out)).toBe(true); // Geometry is reported so a caller can spot a blank or clipped capture. - expect(res.stdout).toMatch(/\d+x\d+ \d+B 3 rows/); - // Parent dirs are created — the skill tells the agent to write into - // evidence/, which will not exist yet. - expect(readFileSync(out).length).toBeGreaterThan(1000); + // The canvas size is deterministic — cols fix the width, the row count + // the height — so assert it exactly; the compressed byte length varies + // with the platform's font rasterisation and flaked on Linux (846B for a + // render that was >1000B on macOS). + expect(res.stdout).toMatch(/360x96 \d+B 3 rows/); })); it('accepts piped input as well as a command', () => @@ -117,8 +120,7 @@ describe('verify-capture helper', () => { })); // Colour and weight are the whole reason to render rather than paste text: - // a red FAIL beside a green PASS is what makes the cell readable at a - // glance. + // a red FAIL beside a green PASS is what makes the cell readable at a glance. // // Each attribute is isolated against the SAME plain baseline. A single // "coloured and bold vs plain" comparison passes while EITHER attribute @@ -171,6 +173,49 @@ describe('verify-capture helper', () => { expect(tall.stdout.split(' ')[1]).toBe(short.stdout.split(' ')[1]); })); + // Regression: U+FE0F (the emoji variation selector in ⚠️) sent Pango looking + // for a colour-emoji font and abort()ing in native code on macOS — no PNG, no + // diagnostic, exit 133. The helper strips it and the base codepoint renders. + // No other test fed the helper a non-ASCII byte. + it('renders non-ASCII and emoji without aborting', () => + withDir((dir) => { + const out = path.join(dir, 'emoji.png'); + const res = run(['--out', out, '--cols', '40'], { + input: 'warn ⚠️ history gap 中文 café\n', + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + })); + + // escapeXml guards every rendered cell and the title; feed it XML-hostile + // content (common in test output: expect(a < b), foo & bar) so a deleted + // escape rule cannot silently corrupt the SVG and ship a broken image. + it('escapes XML special characters in rendered text', () => + withDir((dir) => { + const out = path.join(dir, 'xml.png'); + const res = run( + ['--out', out, '--cols', '40', '--title', 'a < b & "c"'], + { input: 'expect(a < b) & foo > bar "q"\n' }, + ); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + })); + + // scrollback: 0 keeps only the last --rows lines, so a taller input loses its + // header with no visible sign. The helper must say so on stderr rather than + // ship an image that looks complete but starts halfway down. + it('warns when input is taller than --rows', () => + withDir((dir) => { + const out = path.join(dir, 'tall.png'); + const res = run(['--out', out, '--rows', '4'], { + input: 'l1\nl2\nl3\nl4\nl5\nl6\n', + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + expect(res.stderr).toContain('warning: input has 6 lines'); + expect(res.stderr).toContain('dropped the top 2'); + })); + describe('fails loudly rather than writing a broken image', () => { it('rejects a missing --out', () => { const res = run(['--', 'echo', 'hi']); @@ -180,10 +225,14 @@ describe('verify-capture helper', () => { it('rejects nonsense geometry', () => withDir((dir) => { - for (const bad of ['0', '-5', 'abc', '9999']) { - const res = run(['--out', path.join(dir, 'x.png'), '--cols', bad]); - expect(res.status, `--cols ${bad} was accepted`).toBe(1); - expect(res.stderr).toContain('must be an integer'); + // Both flags share one validation loop; feed both so a mutation + // dropping 'rows' from it cannot survive the suite. + for (const flag of ['--cols', '--rows']) { + for (const bad of ['0', '-5', 'abc', '9999']) { + const res = run(['--out', path.join(dir, 'x.png'), flag, bad]); + expect(res.status, `${flag} ${bad} was accepted`).toBe(1); + expect(res.stderr).toContain('must be an integer'); + } } })); @@ -215,18 +264,24 @@ describe('verify-capture helper', () => { })); }); - // The helper only helps if the skill points at it. Three rounds of - // rewording failed because the named pipeline did not exist; assert the - // dead route is gone and the live one is named. + // The helper only helps if the skill points at it. Three rounds of rewording + // failed because the named pipeline never produced an image; assert the slow + // browser route is no longer the recommendation and the live command is named. it('is the route the skill actually names', () => { const skill = readFileSync( path.join(repoRoot, '.qwen/skills/verify-pr/SKILL.md'), 'utf8', ); - expect(skill).toContain('node scripts/verify-capture.mjs --out'); - expect(skill).toContain('no\n browser and no pseudo-terminal'); - // The route that never existed must not be recommended again. - expect(skill).not.toMatch(/Route: `terminal-capture` skill/); - expect(skill).not.toMatch(/node-pty → xterm\.js → Playwright PNG/); + // Flatten whitespace so a prose reflow cannot break these, like the sibling + // test in qwen-triage-workflow.test.js. + const flat = skill.replace(/\s+/g, ' '); + expect(flat).toContain('node scripts/verify-capture.mjs --out'); + expect(flat).toContain('no browser and no pseudo-terminal'); + // The slow browser pipeline must not be the recommended route again... + expect(flat).not.toContain('Route: `terminal-capture` skill'); + expect(flat).not.toMatch(/node-pty → xterm\.js → Playwright PNG/); + // ...but the skill must still point at it for the captures this helper + // cannot do (an ink TUI or a browser page), so the route stays discoverable. + expect(flat).toContain('see the `terminal-capture` skill'); }); }); diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs index 13b37f5ae35..56c033b023e 100644 --- a/scripts/verify-capture.mjs +++ b/scripts/verify-capture.mjs @@ -9,12 +9,16 @@ /** * Render a command's terminal output to a PNG, for `/verify` evidence images. * - * Why this exists: the `verify-pr` skill asked the agent to build its own - * node-pty -> xterm.js -> Playwright pipeline. Neither `node-pty` nor the - * `playwright` package is a dependency of this repo, and node-pty needs a - * native build — so the documented route did not exist, and three rounds of - * rewording the instruction produced zero images across four live runs. This - * turns a capture into one command using deps that are already installed. + * Why this exists: the `verify-pr` skill asked the agent to assemble its own + * node-pty -> xterm.js -> Playwright pipeline. Those dependencies do resolve + * from this repo (node-pty is a root optionalDependency shipping prebuilt + * binaries; playwright is declared in packages/webui and + * integration-tests/terminal-capture), but the route needs a browser, is slow, + * and — the real fragility — integration-tests/terminal-capture is not a root + * workspace, so its package.json is never installed as a unit and resolves only + * because every dependency happens to be hoisted. Four live runs still produced + * zero images. This makes a capture one fast command on deps already installed: + * no browser, no pseudo-terminal. * * Pipeline: run the command, feed its bytes to @xterm/headless (which parses * ANSI into a cell grid with colour and bold attributes), emit that grid as @@ -149,8 +153,11 @@ function collectOutput(cmd) { process.exit(1); } // A non-zero exit is not a capture failure — capturing a failing base arm is - // the normal case for an A/B cell. - process.stderr.write(`verify-capture: command exited ${res.status}\n`); + // the normal case for an A/B cell. A signal-killed child has status === null, + // so name the signal rather than printing "exited null". + const how = + res.signal != null ? `killed by ${res.signal}` : `exited ${res.status}`; + process.stderr.write(`verify-capture: command ${how}\n`); return `${res.stdout ?? ''}${res.stderr ?? ''}`; } @@ -162,12 +169,15 @@ async function render(raw, opts) { scrollback: 0, allowProposedApi: true, }); - // xterm needs CRLF; a bare LF leaves the cursor in the old column and every - // line after the first renders indented by the previous line's length. - term.write(raw.replace(/\r?\n/g, '\r\n')); - // write() is asynchronous internally; without a turn of the loop the buffer - // is still empty and the capture silently comes out blank. - await new Promise((r) => setTimeout(r, 120)); + // U+FE0F (emoji variation selector) makes Pango abort() in native code when + // no colour-emoji font exists — uncatchable here — so strip it; the base + // codepoint still renders. CRLF is required or a bare LF leaves the cursor in + // the old column and indents every later line. Await xterm's write callback + // (fires once the parser has drained the input) rather than a fixed sleep, so + // a large capture is not read mid-parse and silently come out blank. + await new Promise((r) => + term.write(raw.replace(/\uFE0F/g, '').replace(/\r?\n/g, '\r\n'), r), + ); const buf = term.buffer.active; const rows = []; @@ -230,6 +240,17 @@ async function render(raw, opts) { const info = await sharp(Buffer.from(svg)) .png({ compressionLevel: 9 }) .toFile(out); + // scrollback: 0 keeps only the last --rows lines, so a taller input loses its + // top — often the header — with no visible sign; say so rather than shipping + // an image that looks complete but starts halfway down. + const inputLines = raw.replace(/\r?\n$/, '').split(/\r?\n/).length; + if (inputLines > opts.rows) { + process.stderr.write( + `verify-capture: warning: input has ${inputLines} lines; ` + + `--rows ${opts.rows} kept the last ${opts.rows} and dropped the top ` + + `${inputLines - opts.rows}\n`, + ); + } process.stdout.write( `${out} ${info.width}x${info.height} ${info.size}B ${rows.length} rows\n`, ); From 47fa5c1934175a49e1e0b4854384eb5604b3ef61 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Thu, 30 Jul 2026 13:35:20 +0000 Subject: [PATCH 3/6] fix(verify-pr): exercise colour fallback and fix wrap-aware truncation warning (#8114) --- scripts/tests/verify-capture.test.js | 32 ++++++++++++++++++++++++++-- scripts/verify-capture.mjs | 24 +++++++++++++++------ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js index c394bcc0726..e0c829394b6 100644 --- a/scripts/tests/verify-capture.test.js +++ b/scripts/tests/verify-capture.test.js @@ -146,6 +146,19 @@ describe('verify-capture helper', () => { expect(colourOnly.equals(boldOnly)).toBe(false); })); + // 256-colour and truecolor sequences produce getFgColor() values >= 16, + // which the bounds guard maps to FG_DEFAULT. Exercise that branch so a + // future simplification to ANSI[cell.fg] cannot ship fill="undefined". + it('renders 256-colour and truecolor via the default-grey fallback', () => + withDir((dir) => { + const out = path.join(dir, 'fallback.png'); + const res = run(['--out', out], { + input: `${ESC}[38;5;200mFAIL${ESC}[0m ${ESC}[38;2;255;100;0mFAIL${ESC}[0m\n`, + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + })); + // A bare LF leaves xterm's cursor in the old column, so line 2 renders // indented by line 1's width and the capture looks like a staircase. The // helper normalises to CRLF; assert the rendered height matches the line @@ -201,7 +214,7 @@ describe('verify-capture helper', () => { expect(isPng(out)).toBe(true); })); - // scrollback: 0 keeps only the last --rows lines, so a taller input loses its + // scrollback: 0 keeps only the last --rows rows, so a taller input loses its // header with no visible sign. The helper must say so on stderr rather than // ship an image that looks complete but starts halfway down. it('warns when input is taller than --rows', () => @@ -212,7 +225,22 @@ describe('verify-capture helper', () => { }); expect(res.status).toBe(0); expect(isPng(out)).toBe(true); - expect(res.stderr).toContain('warning: input has 6 lines'); + expect(res.stderr).toContain('warning: input occupies 6 terminal rows'); + expect(res.stderr).toContain('dropped the top 2'); + })); + + // A line wider than --cols wraps into ceil(len / cols) terminal rows, so + // the top can be dropped even when the newline count is under --rows. + it('warns when wrapped lines exceed --rows', () => + withDir((dir) => { + const out = path.join(dir, 'wrapped.png'); + const longLine = 'a'.repeat(20); + const res = run(['--out', out, '--cols', '10', '--rows', '4'], { + input: `${longLine}\n${longLine}\n${longLine}\n`, + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + expect(res.stderr).toContain('warning: input occupies 6 terminal rows'); expect(res.stderr).toContain('dropped the top 2'); })); diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs index 56c033b023e..36cfd1bc205 100644 --- a/scripts/verify-capture.mjs +++ b/scripts/verify-capture.mjs @@ -240,15 +240,27 @@ async function render(raw, opts) { const info = await sharp(Buffer.from(svg)) .png({ compressionLevel: 9 }) .toFile(out); - // scrollback: 0 keeps only the last --rows lines, so a taller input loses its + // scrollback: 0 keeps only the last --rows rows, so a taller input loses its // top — often the header — with no visible sign; say so rather than shipping - // an image that looks complete but starts halfway down. - const inputLines = raw.replace(/\r?\n$/, '').split(/\r?\n/).length; - if (inputLines > opts.rows) { + // an image that looks complete but starts halfway down. Count wrapped rows + // (a line wider than --cols occupies ceil(len / cols) terminal rows) so the + // warning also fires when wrapping, not just newlines, pushes past --rows. + const ESC = String.fromCharCode(27); + const stripAnsi = (s) => + s.replace(new RegExp(`${ESC}\\[[0-9;]*[a-zA-Z]`, 'g'), ''); + const wrappedRows = raw + .replace(/\r?\n$/, '') + .split(/\r?\n/) + .reduce( + (sum, line) => + sum + Math.max(1, Math.ceil(stripAnsi(line).length / opts.cols)), + 0, + ); + if (wrappedRows > opts.rows) { process.stderr.write( - `verify-capture: warning: input has ${inputLines} lines; ` + + `verify-capture: warning: input occupies ${wrappedRows} terminal rows; ` + `--rows ${opts.rows} kept the last ${opts.rows} and dropped the top ` + - `${inputLines - opts.rows}\n`, + `${wrappedRows - opts.rows}\n`, ); } process.stdout.write( From 4b2465f4f06613d1d994c21dede5b2daf8ac99f3 Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Thu, 30 Jul 2026 15:35:39 +0000 Subject: [PATCH 4/6] fix(verify-pr): separate stdout/stderr join and pin next() guard (#8114) --- scripts/tests/verify-capture.test.js | 30 ++++++++++++++++++++++++++++ scripts/verify-capture.mjs | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js index e0c829394b6..d88b0e0e62d 100644 --- a/scripts/tests/verify-capture.test.js +++ b/scripts/tests/verify-capture.test.js @@ -119,6 +119,28 @@ describe('verify-capture helper', () => { expect(isPng(out)).toBe(true); })); + // stdout and stderr are collected separately by spawnSync; joining them with + // an empty separator glues a partial stdout line to the first stderr line, + // producing a line that never appeared on the real terminal. + it('separates stdout and stderr with a newline', () => + withDir((dir) => { + const out = path.join(dir, 'both-streams.png'); + const res = run([ + '--out', + out, + '--cols', + '40', + '--', + 'node', + '-e', + 'process.stdout.write("result: 42"); process.stderr.write("warning: flaky")', + ]); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + // Two separate lines, not one glued "result: 42warning: flaky". + expect(res.stdout).toContain('2 rows'); + })); + // Colour and weight are the whole reason to render rather than paste text: // a red FAIL beside a green PASS is what makes the cell readable at a glance. // @@ -251,6 +273,14 @@ describe('verify-capture helper', () => { expect(res.stderr).toContain('--out is required'); }); + it('rejects a flag with no value', () => { + for (const flag of ['--out', '--cols', '--rows', '--title']) { + const res = run([flag]); + expect(res.status, `${flag} accepted without a value`).toBe(1); + expect(res.stderr).toContain('needs a value'); + } + }); + it('rejects nonsense geometry', () => withDir((dir) => { // Both flags share one validation loop; feed both so a mutation diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs index 36cfd1bc205..a79e98bd025 100644 --- a/scripts/verify-capture.mjs +++ b/scripts/verify-capture.mjs @@ -158,7 +158,7 @@ function collectOutput(cmd) { const how = res.signal != null ? `killed by ${res.signal}` : `exited ${res.status}`; process.stderr.write(`verify-capture: command ${how}\n`); - return `${res.stdout ?? ''}${res.stderr ?? ''}`; + return [res.stdout, res.stderr].filter(Boolean).join('\n'); } /** Parse ANSI into a cell grid, then emit it as SVG. */ From 451773a3ae25e084cafe3fa0d78d4c7cc7855d4a Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Thu, 30 Jul 2026 16:58:29 +0000 Subject: [PATCH 5/6] fix(scripts): correct verify-capture truncation guard and SGR 30 colour (#8114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The truncation guard compared wrapped rows against --rows, but newline-terminated output needs one row beyond its last line (the final CRLF scrolls it off the scrollback-less viewport), so input of exactly --rows lines — including the default 40 — lost its top line with no warning, and taller input under-reported the drop by one. Compare against a capacity of rows - 1 for newline-terminated input. Also lift SGR 30 foreground to the default grey: it mapped to #1e1e1e, identical to the canvas background, so black-foreground labels (e.g. vitest's project badge) vanished as black-on-black. --- scripts/tests/verify-capture.test.js | 54 ++++++++++++++++++++++++++-- scripts/verify-capture.mjs | 18 +++++++--- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js index d88b0e0e62d..4f09788dad0 100644 --- a/scripts/tests/verify-capture.test.js +++ b/scripts/tests/verify-capture.test.js @@ -6,6 +6,7 @@ import { spawnSync } from 'node:child_process'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -181,6 +182,39 @@ describe('verify-capture helper', () => { expect(isPng(out)).toBe(true); })); + // SGR 30 maps to #1e1e1e — identical to the canvas BG — so black-foreground + // text (the normal way to label a coloured badge, e.g. vitest's project + // badge) vanished as black-on-black. The helper lifts it to the default + // grey; assert the rendered pixels contain something other than the background + // colour, which a black-on-black render cannot. sharp's PNG bytes are not + // run-to-run deterministic, so compare pixels, not file bytes. + it('keeps black-foreground text readable instead of black-on-black', async () => { + let png; + withDir((dir) => { + const out = path.join(dir, 'black.png'); + expect( + run(['--out', out, '--cols', '30'], { + input: `${ESC}[30mFAIL PASS${ESC}[0m\n`, + }).status, + ).toBe(0); + // Read the bytes before withDir removes the dir; decode them below. + png = readFileSync(out); + }); + const sharp = createRequire(import.meta.url)('sharp'); + const { data, info } = await sharp(png) + .raw() + .toBuffer({ resolveWithObject: true }); + const BG = 0x1e; // #1e1e1e canvas background + let visible = false; + for (let i = 0; i + 2 < data.length; i += info.channels) { + if (data[i] !== BG || data[i + 1] !== BG || data[i + 2] !== BG) { + visible = true; + break; + } + } + expect(visible, 'SGR 30 rendered as black-on-black').toBe(true); + }); + // A bare LF leaves xterm's cursor in the old column, so line 2 renders // indented by line 1's width and the capture looks like a staircase. The // helper normalises to CRLF; assert the rendered height matches the line @@ -248,7 +282,7 @@ describe('verify-capture helper', () => { expect(res.status).toBe(0); expect(isPng(out)).toBe(true); expect(res.stderr).toContain('warning: input occupies 6 terminal rows'); - expect(res.stderr).toContain('dropped the top 2'); + expect(res.stderr).toContain('dropped the top 3'); })); // A line wider than --cols wraps into ceil(len / cols) terminal rows, so @@ -263,7 +297,23 @@ describe('verify-capture helper', () => { expect(res.status).toBe(0); expect(isPng(out)).toBe(true); expect(res.stderr).toContain('warning: input occupies 6 terminal rows'); - expect(res.stderr).toContain('dropped the top 2'); + expect(res.stderr).toContain('dropped the top 3'); + })); + + // The defect the guard exists for: at EXACTLY --rows newline-terminated lines + // the final CRLF still scrolls the top line off the scrollback-less viewport, + // yet the old `wrappedRows > opts.rows` test stayed silent. Usable capacity is + // rows - 1, so exactly --rows lines must warn that one was dropped. + it('warns at exactly --rows lines, where the top line is already gone', () => + withDir((dir) => { + const out = path.join(dir, 'boundary.png'); + const res = run(['--out', out, '--rows', '4'], { + input: 'l1\nl2\nl3\nl4\n', + }); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + expect(res.stderr).toContain('warning: input occupies 4 terminal rows'); + expect(res.stderr).toContain('dropped the top 1'); })); describe('fails loudly rather than writing a broken image', () => { diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs index a79e98bd025..5d306de628a 100644 --- a/scripts/verify-capture.mjs +++ b/scripts/verify-capture.mjs @@ -220,8 +220,12 @@ async function render(raw, opts) { const baseline = PAD + (y + titleRows + 1) * CELL_H - 5; for (const cell of cells) { if (cell.blank) continue; - const colour = + const mapped = cell.fg >= 0 && cell.fg < ANSI.length ? ANSI[cell.fg] : FG_DEFAULT; + // SGR 30 maps to #1e1e1e — identical to the canvas BG — so black-foreground + // text (the normal way to label a coloured badge, e.g. vitest's project + // badge) would vanish as black-on-black; lift it to the default grey. + const colour = mapped === BG ? FG_DEFAULT : mapped; body += `` + @@ -256,11 +260,17 @@ async function render(raw, opts) { sum + Math.max(1, Math.ceil(stripAnsi(line).length / opts.cols)), 0, ); - if (wrappedRows > opts.rows) { + // Newline-terminated output needs one row BEYOND its last line: the final + // CRLF advances the cursor off the viewport and scrolls one row away, so with + // scrollback: 0 the usable capacity is rows - 1, not rows. Comparing against + // opts.rows instead let input of exactly --rows lines lose its top silently — + // the very case this warning exists for. + const capacity = /\r?\n$/.test(raw) ? opts.rows - 1 : opts.rows; + if (wrappedRows > capacity) { process.stderr.write( `verify-capture: warning: input occupies ${wrappedRows} terminal rows; ` + - `--rows ${opts.rows} kept the last ${opts.rows} and dropped the top ` + - `${wrappedRows - opts.rows}\n`, + `--rows ${opts.rows} kept the last ${capacity} and dropped the top ` + + `${wrappedRows - capacity}\n`, ); } process.stdout.write( From 5ac69ae876e89c0df59373b71ab87202200b53ce Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Thu, 30 Jul 2026 19:10:46 +0000 Subject: [PATCH 6/6] fix(scripts): address review feedback on verify-capture helper (#8114) - Declare sharp as a root devDependency so the script does not rely on workspace hoisting from packages/core - Guard against TTY stdin hanging silently: check process.stdin.isTTY before readFileSync(0) and print usage immediately - Fix phantom blank row when stdout already ends with a newline: only insert a separator between stdout and stderr when stdout lacks a trailing newline - Strengthen 256-colour/truecolor test to decode pixels and assert the #d4d4d4 fallback grey is present - Add test for non-newline-terminated input that fits exactly --rows - Add test for the phantom blank row fix (console.log + stderr) --- package.json | 1 + scripts/tests/verify-capture.test.js | 57 ++++++++++++++++++++++++++-- scripts/verify-capture.mjs | 8 +++- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index b40542422bc..53c82021af0 100644 --- a/package.json +++ b/package.json @@ -149,6 +149,7 @@ "prettier": "^3.5.3", "react-devtools-core": "^6.1.5", "semver": "^7.7.2", + "sharp": "^0.34.5", "strip-ansi": "^7.1.2", "tsx": "^4.20.3", "typescript-eslint": "^8.30.1", diff --git a/scripts/tests/verify-capture.test.js b/scripts/tests/verify-capture.test.js index 4f09788dad0..7ea2ba0ace4 100644 --- a/scripts/tests/verify-capture.test.js +++ b/scripts/tests/verify-capture.test.js @@ -142,6 +142,26 @@ describe('verify-capture helper', () => { expect(res.stdout).toContain('2 rows'); })); + // console.log-terminated stdout already ends with \n; the old join('\n') + // inserted a phantom blank row between stdout and stderr. + it('does not insert a phantom blank row when stdout ends with a newline', () => + withDir((dir) => { + const out = path.join(dir, 'no-phantom.png'); + const res = run([ + '--out', + out, + '--cols', + '40', + '--', + 'node', + '-e', + 'console.log("result: 42"); process.stderr.write("warning: flaky\\n")', + ]); + expect(res.status).toBe(0); + expect(isPng(out)).toBe(true); + expect(res.stdout).toContain('2 rows'); + })); + // Colour and weight are the whole reason to render rather than paste text: // a red FAIL beside a green PASS is what makes the cell readable at a glance. // @@ -170,9 +190,11 @@ describe('verify-capture helper', () => { })); // 256-colour and truecolor sequences produce getFgColor() values >= 16, - // which the bounds guard maps to FG_DEFAULT. Exercise that branch so a - // future simplification to ANSI[cell.fg] cannot ship fill="undefined". - it('renders 256-colour and truecolor via the default-grey fallback', () => + // which the bounds guard maps to FG_DEFAULT (#d4d4d4). Decode pixels and + // assert the fallback grey is present, so deleting the guard cannot ship + // fill="undefined" (which librsvg paints black) while the test stays green. + it('renders 256-colour and truecolor via the default-grey fallback', async () => { + let png; withDir((dir) => { const out = path.join(dir, 'fallback.png'); const res = run(['--out', out], { @@ -180,7 +202,22 @@ describe('verify-capture helper', () => { }); expect(res.status).toBe(0); expect(isPng(out)).toBe(true); - })); + png = readFileSync(out); + }); + const sharp = createRequire(import.meta.url)('sharp'); + const { data, info } = await sharp(png) + .raw() + .toBuffer({ resolveWithObject: true }); + // FG_DEFAULT is #d4d4d4 — look for a pixel matching it. + let hasFallback = false; + for (let i = 0; i + 2 < data.length; i += info.channels) { + if (data[i] === 0xd4 && data[i + 1] === 0xd4 && data[i + 2] === 0xd4) { + hasFallback = true; + break; + } + } + expect(hasFallback, '256-colour text did not render as #d4d4d4').toBe(true); + }); // SGR 30 maps to #1e1e1e — identical to the canvas BG — so black-foreground // text (the normal way to label a coloured badge, e.g. vitest's project @@ -316,6 +353,18 @@ describe('verify-capture helper', () => { expect(res.stderr).toContain('dropped the top 1'); })); + // Non-newline-terminated input of exactly --rows lines fits: no final CRLF + // scrolls the viewport, so the usable capacity is rows, not rows - 1. + it('does not warn for non-newline-terminated input that fits --rows', () => + withDir((dir) => { + const out = path.join(dir, 'no-nl.png'); + const res = run(['--out', out, '--rows', '4'], { + input: 'l1\nl2\nl3\nl4', + }); + expect(res.status).toBe(0); + expect(res.stderr).not.toContain('warning'); + })); + describe('fails loudly rather than writing a broken image', () => { it('rejects a missing --out', () => { const res = run(['--', 'echo', 'hi']); diff --git a/scripts/verify-capture.mjs b/scripts/verify-capture.mjs index 5d306de628a..d148e224402 100644 --- a/scripts/verify-capture.mjs +++ b/scripts/verify-capture.mjs @@ -135,6 +135,7 @@ const escapeXml = (s) => /** Collect the bytes to render: either a child command's output, or stdin. */ function collectOutput(cmd) { if (cmd.length === 0) { + if (process.stdin.isTTY) usage('no command given and nothing piped to stdin'); try { return readFileSync(0, 'utf8'); } catch { @@ -158,7 +159,12 @@ function collectOutput(cmd) { const how = res.signal != null ? `killed by ${res.signal}` : `exited ${res.status}`; process.stderr.write(`verify-capture: command ${how}\n`); - return [res.stdout, res.stderr].filter(Boolean).join('\n'); + // Only insert a separator when stdout lacks a trailing newline; a + // console.log-terminated stdout already ends with \n, and join('\n') would + // add a phantom blank row that never appeared on the real terminal. + return res.stdout && res.stderr && !res.stdout.endsWith('\n') + ? res.stdout + '\n' + res.stderr + : res.stdout + res.stderr; } /** Parse ANSI into a cell grid, then emit it as SVG. */