diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 6fc7d257b6..6804f51f10 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2247,6 +2247,25 @@ jobs: (cd "${RUNNER_TEMP:?}" && npm install -g --registry=https://registry.npmjs.org '@qwen-code/qwen-code@latest') qwen --version + # Chromium system dependencies (apt packages) for evidence + # screenshots. install-deps is unpinned so the apt list tracks + # current Playwright (a superset of the lockfile binary's needs); + # the version-sensitive BINARY is downloaded after checkout by the + # "Install evidence browser" step, using the checkout's own + # Playwright so it always matches the lockfile. Best-effort: a + # failure here must not fail the job. + # Record success in a marker the "Install evidence browser" step + # gates on: apt and the Playwright CDN are independent servers with + # no shared success signal, so a binary download alone must not + # promise chromium to the agent. Rewritten or removed every run so + # a stale success on the persistent pool cannot leak through. + if (cd "${RUNNER_TEMP:?}" && npx --yes playwright install-deps chromium); then + printf 'ok' > "${RUNNER_TEMP:?}/verify-chromium-deps-ok" + else + rm -f "${RUNNER_TEMP:?}/verify-chromium-deps-ok" + echo "::warning::Chromium system deps install failed; the verification agent will produce a text-only report." + fi + # This container mounts the persistent runner workspace, and a previous # run EXECUTED PR code as the node user with write access to .git — # hooks or config planted then would fire during the checkout below (as @@ -2444,8 +2463,11 @@ jobs: unset ACTIONS_RUNTIME_TOKEN ACTIONS_RUNTIME_URL ACTIONS_CACHE_URL # rm first: on the persistent pool a stale verify-results from an # EARLIER PR's run would otherwise ride along into this run's - # artifact upload and report selection. + # artifact upload and report selection. The chromium marker is + # cleared too, so a previous run's success cannot promise this run + # a browser it never installed. rm -rf "$RUNNER_TEMP/verify-results" + rm -f "$RUNNER_TEMP/verify-chromium-path" mkdir -p "$RUNNER_TEMP/verify-results" chown -R node:node "$GITHUB_WORKSPACE" prepare_log="$RUNNER_TEMP/verify-results/prepare.log" @@ -2536,6 +2558,46 @@ jobs: fi echo "Install/build completed before verification." >> "$GITHUB_STEP_SUMMARY" + - name: 'Install evidence browser' + if: "steps.pr.outputs.decision == 'run' && steps.prepare.outputs.verdict == ''" + env: + GITHUB_TOKEN: '' + GH_TOKEN: '' + run: |- + set -uo pipefail + # Download the browser binary for the Playwright package the + # capture harness actually imports. This lockfile has TWO + # Playwright trees: terminal-capture.ts imports `playwright`, but + # node_modules/.bin/playwright is @playwright/test's CLI, which + # pins a different chromium revision — so `npx playwright install` + # would download a browser the harness cannot launch. Resolve the + # CLI from the harness's own directory: require.resolve runs the + # same algorithm as its `from 'playwright'` import, so the binary + # matches the lockfile even if npm's hoist layout changes (nothing + # pins playwright to the root node_modules). cli.js is not in the + # package's exports map, so resolve the exported package.json and + # join. Runs as node so the tree is agent-readable without chmod. + # Best-effort: failure degrades to a text-only report. + PW_PATH="${RUNNER_TEMP:?}/pw-browsers" + mkdir -p "$PW_PATH" + chown node:node "$PW_PATH" + PW_CLI="$(runuser -u node -- node -p "require('path').join(require('path').dirname(require.resolve('playwright/package.json', { paths: ['./integration-tests/terminal-capture'] })), 'cli.js')")" + # The marker gates QWEN_VERIFY_CHROMIUM in the agent step, so it + # requires BOTH halves: the system deps installed as root in the + # tools step (which writes verify-chromium-deps-ok) AND the binary + # download below. A binary-only success would tell the agent + # chromium is ready when every launch still dies on a missing .so. + if [ -f "${RUNNER_TEMP:?}/verify-chromium-deps-ok" ] && + runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \ + -u ACTIONS_RUNTIME_TOKEN -u ACTIONS_RUNTIME_URL -u ACTIONS_CACHE_URL \ + PLAYWRIGHT_BROWSERS_PATH="$PW_PATH" \ + node "$PW_CLI" install chromium; then + printf '%s' "$PW_PATH" > "${RUNNER_TEMP:?}/verify-chromium-path" + echo "Chromium available for evidence screenshots." >> "$GITHUB_STEP_SUMMARY" + else + echo "::warning::Chromium unavailable (system deps or browser download failed); the verification agent will produce a text-only report." + fi + - name: 'Run verification agent' if: "steps.pr.outputs.decision == 'run' && steps.prepare.outputs.verdict == ''" id: 'run' @@ -2970,6 +3032,19 @@ jobs: "QWEN_CI_REAL_GH=${QWEN_CI_REAL_GH:-}" "QWEN_CI_REAL_GIT=${QWEN_CI_REAL_GIT:-}" ) + # Evidence screenshots: the "Install evidence browser" step + # downloaded chromium into a shared path using the checkout's + # Playwright. Both variables are set only when that install + # actually succeeded, so their ABSENCE is the agent's signal to + # skip captures rather than burn budget on a download it cannot + # complete. + if CHROMIUM_PATH="$(cat "${RUNNER_TEMP:?}/verify-chromium-path" 2>/dev/null)" && + [ -n "$CHROMIUM_PATH" ]; then + QWEN_ENV+=( + "PLAYWRIGHT_BROWSERS_PATH=$CHROMIUM_PATH" + "QWEN_VERIFY_CHROMIUM=1" + ) + fi if [ -n "${OPENAI_MODEL:-}" ]; then QWEN_ENV+=("OPENAI_MODEL=$OPENAI_MODEL") fi @@ -3251,8 +3326,8 @@ jobs: printf '\n\n\n\n' } - # Host the agent's evidence images (if any) on the pr-assets branch - # — the same convention hand-run verification rounds use — and build + # Host the agent's evidence images (if any) on a per-PR branch + # (pr-assets/-verify, matching hand-run convention) and build # a markdown section referencing them. Image bytes come from a run # that executed PR code: inert but untrusted, so filenames pass a # strict allowlist, count/size are capped (8 files, <2 MB each; the @@ -3263,6 +3338,7 @@ jobs: collect_and_host_evidence() { local imgs=() f base safe seen=' ' hosted=0 total=0 skipped=0 local dest_dir="verify/pr${PR_NUMBER}-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT:-1}" + local assets_branch="pr-assets/${PR_NUMBER}-verify" local clone_dir="${RUNNER_TEMP:-/tmp}/pr-assets" local remote="${VERIFY_ASSETS_REMOTE:-https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git}" # `|| true` on every find: the artifact download is @@ -3282,9 +3358,12 @@ jobs: return 0 fi rm -rf "$clone_dir" - if ! git clone -q --depth 1 --branch pr-assets "$remote" "$clone_dir" 2>/dev/null; then - echo "::warning::pr-assets branch unavailable; posting a text-only report." >&2 - return 0 + if ! git clone -q --depth 1 --branch "$assets_branch" "$remote" "$clone_dir" 2>/dev/null; then + rm -rf "$clone_dir" + mkdir -p "$clone_dir" + git -C "$clone_dir" init -q + git -C "$clone_dir" checkout -q --orphan "$assets_branch" + git -C "$clone_dir" remote add origin "$remote" fi # Rebase in the racing-push retry needs a committer identity, so # set it once on the clone instead of per-command -c flags. @@ -3320,11 +3399,11 @@ jobs: git add "$dest_dir" && git commit -q -m "verify evidence for PR #${PR_NUMBER} (run ${GITHUB_RUN_ID})" && { - git push -q origin HEAD:pr-assets 2>/dev/null || + git push -q origin "HEAD:$assets_branch" 2>/dev/null || { # One retry after a racing push from another assets job. - git pull -q --rebase origin pr-assets 2>/dev/null && - git push -q origin HEAD:pr-assets 2>/dev/null + git pull -q --rebase origin "$assets_branch" 2>/dev/null && + git push -q origin "HEAD:$assets_branch" 2>/dev/null } } ); then @@ -3332,7 +3411,7 @@ jobs: EVIDENCE_SECTION='' return 0 fi - local raw_base="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/pr-assets/${dest_dir}" + local raw_base="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${assets_branch}/${dest_dir}" EVIDENCE_SECTION=$'### Evidence images\n\n' for f in "$clone_dir/$dest_dir"/*.png; do [ -f "$f" ] || continue diff --git a/.github/workflows/web-shell-visuals-cleanup.yml b/.github/workflows/web-shell-visuals-cleanup.yml index ffee20ef37..68ee597829 100644 --- a/.github/workflows/web-shell-visuals-cleanup.yml +++ b/.github/workflows/web-shell-visuals-cleanup.yml @@ -1,9 +1,13 @@ -name: 'Web-shell Visuals Cleanup' +name: 'PR Asset Branch Cleanup' -# When a PR closes, delete its per-PR visuals asset branch so the `pr-assets/*` -# refs (one per PR that ever produced a preview) don't accumulate without bound -# in the base repository. Runs in the base context (pull_request_target) but -# never checks out or runs PR code — it only deletes one ref by name. +# When a PR closes, delete its per-PR asset branches so the `pr-assets/*` +# refs (one per PR that ever produced a preview or a verification report) +# don't accumulate without bound in the base repository. Runs in the base +# context (pull_request_target) but never checks out or runs PR code — it +# only deletes refs by name. +# +# Both producers are covered, and every new `pr-assets/*` producer must be +# added here: a branch nothing deletes is permanent. on: pull_request_target: types: @@ -18,17 +22,29 @@ jobs: runs-on: 'ubuntu-latest' timeout-minutes: 5 steps: - - name: 'Delete the PR asset branch' + - name: 'Delete the PR asset branches' env: # Deleting a ref needs contents:write, which the CI_BOT_PAT carries. GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ github.event.pull_request.number }}' run: |- - set -euo pipefail - branch="pr-assets/web-shell-visuals-${PR_NUMBER}" - if gh api "repos/${GITHUB_REPOSITORY}/git/refs/heads/${branch}" >/dev/null 2>&1; then - gh api -X DELETE "repos/${GITHUB_REPOSITORY}/git/refs/heads/${branch}" - echo "Deleted ${branch}." - else - echo "No asset branch ${branch}; nothing to delete." - fi + set -uo pipefail + # Not `set -e`: one branch missing, or one delete failing, must not + # stop the others. Each is independent and absence is normal — most + # PRs produce neither. + status=0 + for branch in \ + "pr-assets/web-shell-visuals-${PR_NUMBER}" \ + "pr-assets/${PR_NUMBER}-verify"; do + if ! gh api "repos/${GITHUB_REPOSITORY}/git/refs/heads/${branch}" >/dev/null 2>&1; then + echo "No asset branch ${branch}; nothing to delete." + continue + fi + if gh api -X DELETE "repos/${GITHUB_REPOSITORY}/git/refs/heads/${branch}"; then + echo "Deleted ${branch}." + else + echo "::warning::Failed to delete ${branch}; it will need removing by hand." + status=1 + fi + done + exit "$status" diff --git a/.qwen/skills/terminal-capture/SKILL.md b/.qwen/skills/terminal-capture/SKILL.md index 3fba541ab1..905fe8b8ef 100644 --- a/.qwen/skills/terminal-capture/SKILL.md +++ b/.qwen/skills/terminal-capture/SKILL.md @@ -17,9 +17,14 @@ Ensure the following dependencies are installed before running: ```bash npm install # Install project dependencies. -npx playwright install chromium # Install Playwright browser +npx playwright install chromium # Install Playwright browser (skip in CI: see note below) ``` +> **CI / verify context:** when `QWEN_VERIFY_CHROMIUM=1` is set, the browser +> is already installed and `PLAYWRIGHT_BROWSERS_PATH` points at it. Do **not** +> run `playwright install` — it downloads ~170 MB and fails on system deps +> the agent user cannot install. + ## Architecture ``` @@ -227,7 +232,8 @@ This tool is commonly used for visual verification during PR reviews. - Playwright error `browser not found` Cause: browser not installed. - Solution: `npx playwright install chromium`. + Solution: `npx playwright install chromium` (local dev only — in CI verify + runs, this means the pre-install step failed; report it, do not install). - Blank screenshot Cause: process starts slowly or build failed. Solution: check build success and the spawn command. diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 19b0abc8cf..fc1bad504f 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -525,17 +525,33 @@ workflow globs). It must contain: - `assertions.json` — `{"pass": , "fail": , "total": }`, counting **only scripted assertions that actually executed**. - Harness scripts and raw logs (per-cell stdout/stderr, build logs). -- Optionally `evidence/*.png` — rendered image evidence. The publish job - hosts these on the `pr-assets` branch and appends them below the report, - capped at **8 images, 2 MB each**; anything beyond stays in the run - artifacts only. Use them when text cannot carry the oracle: TUI rendering - (`terminal-capture` skill: node-pty → xterm → Playwright PNG; - `npx playwright install chromium` on demand) or a one-image harness - summary. Name each file as a kebab-case caption that binds image to claim - (`01-bundle-ab-base-vs-head.png`, `02-repaint-after-sigcont.png`) — the - filename becomes the published caption — and reference it from report.md - prose by that name. Before/after pairs beat single "after" shots; a - screenshot that does not name what to look at proves nothing. +- `evidence/*.png` — image evidence. **Produce these whenever you ran a + harness**, not only for TUI work. A table in the report is your _claim_ + about what happened; a capture of the run is a _witness_ that the numbers + came from a real execution, and it is the part a reviewer cannot get any + other way. The highest-value shots, in order: the A/B cells side by side, + the mutation matrix as it printed, and the raw harness output behind a + 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. + + 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 + beyond stays in the run artifacts. Name each file as a kebab-case caption + that binds image to claim (`01-bundle-ab-base-vs-head.png`, + `02-repaint-after-sigcont.png`) — the filename becomes the published + caption — and reference it from report.md prose by that name. Before/after + pairs beat single "after" shots; a screenshot that does not name what to + look at proves nothing. `verdict.txt` meanings: `merge-ready` = every executed assertion passed and no new blocking finding; `findings` = evidence produced concrete problems worth a diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 1df2cca45c..ed0e55cedb 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6,6 +6,7 @@ import { spawn, spawnSync } from 'node:child_process'; import { + existsSync, mkdirSync, mkdtempSync, readdirSync, @@ -1515,15 +1516,19 @@ describe('qwen-triage verify hardening', () => { // The lifecycle-script command-file guards must be asserted on the verify // job's own commands: a bare step() lookup returns the tmux job's // identically named step, so verify-side regressions would pass silently. - it('strips GitHub command files from both verify lifecycle commands', () => { - // Bound to the prepare step: the agent step's own `runuser` launches - // qwen under `env -i`, which needs no per-variable stripping. + it('strips GitHub command files from every node-run verify command', () => { + // Bound to the lifecycle commands that run as node before the agent: + // npm ci and npm run build in the prepare step, plus the evidence + // browser download. The slice stops at the agent step, whose own + // `runuser` launches qwen under `env -i` and needs no per-variable + // stripping. Covering all three by construction (not enumeration) is + // what catches a future node-run command added without the strip. const prepare = verifyJob.slice( verifyJob.indexOf('Install and build PR app'), verifyJob.indexOf('Run verification agent'), ); const commands = prepare.match(/runuser -u node -- env[\s\S]*?\n/g) ?? []; - expect(commands.length).toBe(2); + expect(commands.length).toBe(3); expect(step('Run verification agent')).toContain( 'runuser -u node -- env -i', ); @@ -1869,7 +1874,7 @@ describe('qwen-triage verify hardening round 2', () => { // A bare remote with a pr-assets branch, plus a gh stub. sh(`git init -q --bare "${dir}/assets.git"`); sh( - `mkdir -p "${dir}/seed" && cd "${dir}/seed" && git init -q && git checkout -q -b pr-assets && echo s > s.txt && git add . && git -c user.name=t -c user.email=t@t commit -qm s && git push -q "${dir}/assets.git" pr-assets`, + `mkdir -p "${dir}/seed" && cd "${dir}/seed" && git init -q && git checkout -q -b pr-assets/7999-verify && echo s > s.txt && git add . && git -c user.name=t -c user.email=t@t commit -qm s && git push -q "${dir}/assets.git" pr-assets/7999-verify`, ); writeFileSync( join(dir, 'gh'), @@ -1931,7 +1936,7 @@ describe('qwen-triage verify hardening round 2', () => { }); expect(res.status).toBe(0); const hosted = sh( - `git -C "${dir}/assets.git" ls-tree -r --name-only pr-assets | grep verify/ || true`, + `git -C "${dir}/assets.git" ls-tree -r --name-only pr-assets/7999-verify | grep verify/ || true`, ) .stdout.trim() .split('\n') @@ -1947,8 +1952,8 @@ describe('qwen-triage verify hardening round 2', () => { expect(comment).not.toContain('02-fake'); expect(comment).toContain('did not pass the hosting checks'); - // No reachable pr-assets branch -> text-only, never an aborted report. - sh(`git init -q --bare "${dir}/empty.git"`); + // Unreachable remote -> text-only, never an aborted report. + sh(`rm -rf "${dir}/empty.git" && mkdir -p "${dir}/empty.git"`); const out2 = join(dir, 'comment2.md'); const res2 = sh(script, { cwd: work, @@ -1976,6 +1981,62 @@ describe('qwen-triage verify hardening round 2', () => { const comment2 = readFileSync(out2, 'utf8'); expect(comment2).toContain('Sandboxed verification'); expect(comment2).not.toContain('Evidence images'); + + // FIRST RUN on a PR: the remote is valid but the per-PR branch does + // not exist yet, so the clone fails and the orphan-init path runs for + // real. Both scenarios above take the clone-failed branch too, but + // both then fail to push (one seeded the branch, the other has no + // remote), so neither proves orphan-init can actually DELIVER. Without + // this, a bug in `checkout --orphan` or a dropped `remote add origin` + // would silently discard every image on every PR's first run. + sh(`git -C "${dir}/assets.git" branch -D pr-assets/7999-verify`); + expect( + sh( + `git -C "${dir}/assets.git" branch --list pr-assets/7999-verify`, + ).stdout.trim(), + ).toBe(''); + const out3 = join(dir, 'comment3.md'); + const res3 = sh(script, { + cwd: work, + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_STUB_OUT: out3, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + RUNNER_TEMP: dir, + GITHUB_STEP_SUMMARY: '/dev/null', + GITHUB_RUN_ID: '79', + GITHUB_RUN_ATTEMPT: '1', + PR_NUMBER: '7999', + RUN_URL: 'u', + VERIFY_RESULT: 'success', + VERDICT: 'pass', + AGENT_VERDICT: 'findings', + SKIP_REASON: '', + PREPARE_FAILURE_PHASE: '', + VERIFY_ASSETS_REMOTE: `${dir}/assets.git`, + }, + }); + expect(res3.status).toBe(0); + // The branch was created by orphan-init and carries this run's images. + const hosted3 = sh( + `git -C "${dir}/assets.git" ls-tree -r --name-only pr-assets/7999-verify | grep verify/ || true`, + ) + .stdout.trim() + .split('\n') + .filter(Boolean); + expect(hosted3.map((p) => p.split('/').pop()).sort()).toEqual([ + '01-ab.png', + '04-edge.png', + ]); + // Orphan, not a graft onto unrelated history: exactly one commit. + expect( + sh( + `git -C "${dir}/assets.git" rev-list --count pr-assets/7999-verify`, + ).stdout.trim(), + ).toBe('1'); + expect(readFileSync(out3, 'utf8')).toContain('![01-ab]('); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -2996,6 +3057,269 @@ describe('qwen-triage verify round-3 hardening', () => { expect(group).toContain("vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true'"); }); + // Evidence images: the agent cannot install chromium (runs as `node`, + // `env -i`, fresh HOME, no apt). The tools step installs system deps + // as root; a post-checkout step downloads the browser binary using the + // checkout's own Playwright so the version always matches the lockfile. + it('pre-installs chromium and hands it to the agent', () => { + // System deps only — no browser binary, no version-sensitive download. + const tools = stepIn('verify', 'Install verify runner tools'); + expect(tools).toContain('install-deps chromium'); + expect(tools).not.toContain('install --with-deps'); + expect(tools).toContain('::warning::Chromium system deps install failed'); + // Deps success is recorded in a marker the browser step gates on: apt + // and the Playwright CDN are independent servers, so a binary download + // alone must not promise chromium to the agent. + expect(tools).toContain('verify-chromium-deps-ok'); + // No hardcoded Playwright pin here either: the apt list must track + // current Playwright so it covers the lockfile-matched binary below. + expect(tools).not.toMatch(/playwright@[\d.]/); + + // Browser binary: downloaded after npm ci, and by the CLI of the + // package the capture harness actually imports — never a hardcoded pin + // (M5). This lockfile has TWO Playwright trees: terminal-capture.ts + // imports `playwright`, but node_modules/.bin/playwright (what `npx + // playwright` resolves) is @playwright/test's CLI, which pins a + // different chromium revision. The install must therefore resolve the + // imported package's cli.js from the harness's own directory (the same + // algorithm as its import), not assume npm hoists `playwright` to the + // root — a hoist nothing pins. cli.js is absent from the package's + // exports map, so the workflow resolves the exported package.json and + // joins; binding this assertion to the harness's import keeps the two + // from drifting apart. + const capture = readFileSync( + 'integration-tests/terminal-capture/terminal-capture.ts', + 'utf8', + ); + expect(capture).toMatch(/from 'playwright'/); + const browser = stepIn('verify', 'Install evidence browser'); + expect(browser).toContain( + "require.resolve('playwright/package.json', { paths: ['./integration-tests/terminal-capture'] })", + ); + expect(browser).toContain('node "$PW_CLI" install chromium'); + expect(browser).not.toContain('./node_modules/playwright/cli.js'); + expect(browser).not.toContain('npx playwright install chromium'); + expect(browser).not.toMatch(/playwright@[\d.]/); + expect(browser).toContain('PLAYWRIGHT_BROWSERS_PATH'); + // The CLI runs PR-controlled code ($PW_CLI resolves from the PR's + // node_modules), so it must drop the same runner-injected cache + // credentials the prepare and agent steps unset — a doctored + // playwright package could otherwise write to the Actions cache. + expect(browser).toContain('-u ACTIONS_RUNTIME_TOKEN'); + expect(browser).toContain('-u ACTIONS_RUNTIME_URL'); + expect(browser).toContain('-u ACTIONS_CACHE_URL'); + // Marker is written ONLY inside the success branch (M6): the if + // condition must precede the marker write, and the else branch must + // carry the warning instead. The condition must ALSO require the deps + // marker (M6b): a binary download alone is not enough. + expect(browser).toContain('verify-chromium-deps-ok'); + const ifIdx = browser.indexOf( + 'if [ -f "${RUNNER_TEMP:?}/verify-chromium-deps-ok" ]', + ); + const markerIdx = browser.indexOf('verify-chromium-path'); + const elseIdx = browser.indexOf('::warning::Chromium unavailable'); + expect(ifIdx).toBeGreaterThan(-1); + expect(markerIdx).toBeGreaterThan(ifIdx); + expect(elseIdx).toBeGreaterThan(markerIdx); + // Best-effort: a failed download must not fail a verification. + expect(browser).not.toContain('exit 1'); + // A stale marker from an earlier run on the persistent pool must not + // ride along: prepare clears it before this step writes a fresh one, + // so absence stays a real signal even though RUNNER_TEMP outlives the + // run. + expect(stepIn('verify', 'Install and build PR app')).toContain( + 'rm -f "$RUNNER_TEMP/verify-chromium-path"', + ); + + // The agent is told ONLY when the install actually succeeded, so the + // variable's absence is a real signal rather than a stale promise. + // The guard must be CONDITIONAL (M1b): QWEN_VERIFY_CHROMIUM=1 must + // appear inside the if-block that reads the marker, not unconditionally. + const runStep = stepIn('verify', 'Run verification agent'); + expect(runStep).toContain('verify-chromium-path'); + expect(runStep).toContain('QWEN_VERIFY_CHROMIUM=1'); + // Both variables, not just the flag. Losing the path alone is the + // nastiest arm: the agent is TOLD chromium is available, Playwright + // then looks in the default ~/.cache/ms-playwright instead of the + // shared install, and captures degrade to text-only after a successful + // install — verified by mutation, this test passed without this line. + expect(runStep).toContain('PLAYWRIGHT_BROWSERS_PATH=$CHROMIUM_PATH'); + const guard = runStep.indexOf('verify-chromium-path'); + const marker = runStep.indexOf('QWEN_VERIFY_CHROMIUM=1'); + const path = runStep.indexOf('PLAYWRIGHT_BROWSERS_PATH=$CHROMIUM_PATH'); + expect(guard).toBeLessThan(marker); + expect(guard).toBeLessThan(path); + // The if-fi block must wrap the QWEN_ENV+= assignment. Anchor the + // slice at the real `if` keyword: `guard - 20` lands inside the marker + // path, where "ver-if-y-chromium-path" satisfies toContain('if') even + // with the guard deleted. + const runIfIdx = runStep.lastIndexOf('if CHROMIUM_PATH', guard); + expect(runIfIdx).toBeGreaterThan(-1); + const ifBlock = runStep.slice(runIfIdx, marker + 30); + expect(ifBlock).toMatch(/(^|\s)if\s/); + expect(ifBlock).toContain('then'); + + // The tmux lane is untouched: it has no evidence-image path. + expect(stepIn('tmux-testing', 'Run tmux real-user testing')).not.toContain( + 'QWEN_VERIFY_CHROMIUM', + ); + + // And the skill must stop calling captures optional, must name the + // gate, and must forbid the install the agent cannot complete. + 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 browser step's require.resolve + cli.js join is otherwise guarded + // only by a literal string match, so a Playwright bump that relocates + // cli.js would break the download at runtime while that assertion still + // passes. Execute the workflow's exact resolution against the installed + // tree and require it to land on a real cli.js. Skipped only when + // playwright is absent entirely (the require.resolve itself fails), + // mirroring the jq tool-availability guard above. + const resolveHarnessCli = + "process.stdout.write(require('path').join(require('path').dirname(" + + "require.resolve('playwright/package.json', { paths: ['./integration-tests/terminal-capture'] })" + + "), 'cli.js'))"; + it.skipIf(spawnSync('node', ['-e', resolveHarnessCli]).status !== 0)( + 'resolves the harness Playwright cli.js to a real file', + () => { + const cli = spawnSync('node', ['-e', resolveHarnessCli], { + encoding: 'utf8', + }); + expect(cli.status).toBe(0); + expect(cli.stdout).toMatch(/cli\.js$/); + expect(existsSync(cli.stdout)).toBe(true); + }, + ); + + // The publish job must host images on a per-PR branch that can coexist + // with the existing pr-assets/* namespace — a bare `pr-assets` leaf + // cannot be created while `pr-assets/…` children exist. + it('hosts evidence on a per-PR branch, not a bare pr-assets leaf', () => { + const publish = stepIn( + 'publish-verify', + 'Post verification report comment', + ); + expect(publish).toContain('pr-assets/${PR_NUMBER}-verify'); + expect(publish).toContain('checkout -q --orphan'); + expect(publish).not.toMatch(/--branch pr-assets["\s]/); + }); + + // Every `pr-assets/*` producer needs a deleter, or its branches are + // permanent: one single-commit branch per verified PR, forever, slowing + // `git ls-remote` and cluttering the branch list for every contributor. + // The verify lane became a second producer and was not added. + it('deletes both pr-assets producers when a PR closes', () => { + const cleanup = readFileSync( + '.github/workflows/web-shell-visuals-cleanup.yml', + 'utf8', + ); + // Both branch names, built from the same PR number. + expect(cleanup).toContain('pr-assets/web-shell-visuals-${PR_NUMBER}'); + expect(cleanup).toContain('pr-assets/${PR_NUMBER}-verify'); + // Runs in the base context and never touches PR code. + expect(cleanup).toContain('pull_request_target'); + expect(cleanup).not.toContain('actions/checkout'); + // One branch missing or one delete failing must not stop the other: + // most PRs produce neither, so absence is the normal case. + expect(cleanup).not.toContain('set -euo pipefail'); + expect(cleanup).toContain('continue'); + // ...but a real delete failure still has to be visible. + expect(cleanup).toContain('::warning::Failed to delete'); + + // Execute it against a stubbed gh: the loop must attempt both refs, and + // a missing first branch must not skip the second. + const script = cleanup.match(/run: \|-\n([\s\S]*?)$/)?.[1]; + expect(script).toBeTruthy(); + const dir = mkdtempSync(join(tmpdir(), 'assets-cleanup-')); + try { + const calls = join(dir, 'gh-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'gh'), + [ + '#!/usr/bin/env bash', + 'printf "%s\\n" "$*" >> "$GH_CALLS"', + // Only the verify branch exists; the visuals one 404s. + 'case "$*" in', + ' *web-shell-visuals*) exit 1 ;;', + 'esac', + 'exit 0', + ].join('\n'), + { mode: 0o755 }, + ); + const res = spawnSync('bash', ['-c', script.replace(/^ {10}/gm, '')], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_CALLS: calls, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + PR_NUMBER: '7999', + }, + }); + expect(res.status).toBe(0); + const lines = readFileSync(calls, 'utf8').trim().split('\n'); + const deletes = lines.filter((l) => l.includes('DELETE')); + // The missing visuals branch was probed but never deleted... + expect( + lines.some((l) => l.includes('pr-assets/web-shell-visuals-7999')), + ).toBe(true); + expect(deletes.some((l) => l.includes('web-shell-visuals'))).toBe(false); + // ...and the loop carried on to the verify branch and deleted it. + // This is the assertion the whole test exists for: a `set -e` script + // would have exited on the first 404 and never reached here. + expect(deletes.some((l) => l.includes('pr-assets/7999-verify'))).toBe( + true, + ); + expect(res.stdout).toContain('Deleted pr-assets/7999-verify'); + + // Delete-FAILURE path: the verify branch probes OK but its DELETE + // fails (transient 500, or the PAT lost contents:write). The loop + // must set a non-zero exit and surface the warning — without this, a + // future edit dropping `exit "$status"` leaves the job green while + // the orphaned branch this workflow exists to prevent accumulates. + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'gh'), + [ + '#!/usr/bin/env bash', + 'printf "%s\\n" "$*" >> "$GH_CALLS"', + // The verify branch probes OK but its DELETE fails; the visuals + // branch still 404s on the probe. + 'case "$*" in', + ' *web-shell-visuals*) exit 1 ;;', + ' *"-X DELETE"*) exit 1 ;;', + 'esac', + 'exit 0', + ].join('\n'), + { mode: 0o755 }, + ); + const res2 = spawnSync('bash', ['-c', script.replace(/^ {10}/gm, '')], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_CALLS: calls, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + PR_NUMBER: '7999', + }, + }); + expect(res2.status).toBe(1); + expect(res2.stdout).toContain( + '::warning::Failed to delete pr-assets/7999-verify', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + // One budget drives the agent's graceful kill and the watchdog threshold // that distinguishes that kill from an OOM — both derive from a single // AGENT_BUDGET_M, so they cannot drift apart. The job limit and the