From 029beba3eff66d31575b09a37338a27ab96f47e3 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Wed, 29 Jul 2026 09:36:14 +0000 Subject: [PATCH] fix(triage): match browser to lockfile and fix pr-assets branch conflict (#8016) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the chromium install into system deps (tools step, as root) and browser binary (new post-checkout step, as node using the checkout's own Playwright). This eliminates the version pin that produced an unusable browser tree. Change evidence hosting from a bare pr-assets leaf branch — which cannot coexist with the existing pr-assets/* namespace — to per-PR branches (pr-assets/pr-verify) with orphan-init fallback. Update terminal-capture skill to warn CI agents away from the forbidden playwright install command, and close test blind spots for conditionality (M1b), version matching (M5), and marker ordering (M6). --- .github/workflows/qwen-triage.yml | 81 ++++++++++++---------- .qwen/skills/terminal-capture/SKILL.md | 10 ++- .qwen/skills/verify-pr/SKILL.md | 5 +- scripts/tests/qwen-triage-workflow.test.js | 73 +++++++++++++------ 4 files changed, 110 insertions(+), 59 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index c16c555efb..5bded06305 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2073,27 +2073,13 @@ jobs: (cd "${RUNNER_TEMP:?}" && npm install -g --registry=https://registry.npmjs.org '@qwen-code/qwen-code@latest') qwen --version - # Chromium for evidence screenshots, installed HERE and not by the - # agent, because the agent cannot: it runs as `node` under `env -i` - # with a fresh HOME, so `playwright install --with-deps` would need - # an apt it has no rights to and would re-download ~170 MB into a - # directory thrown away at the end of every run. Installing as root - # into a shared path fixes both. - # - # Best-effort: a failure here must not fail the job. The agent - # checks QWEN_VERIFY_CHROMIUM before attempting a capture and falls - # back to a text-only report, which is what every run has produced - # so far anyway. - export PLAYWRIGHT_BROWSERS_PATH="${RUNNER_TEMP:?}/pw-browsers" - mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" - if (cd "${RUNNER_TEMP:?}" && npx --yes playwright@1.56.1 install --with-deps chromium); then - # World-readable: the agent runs as `node` and this tree is - # created by root. - chmod -R a+rX "$PLAYWRIGHT_BROWSERS_PATH" - printf '%s' "$PLAYWRIGHT_BROWSERS_PATH" > "${RUNNER_TEMP:?}/verify-chromium-path" - echo "Chromium available for evidence screenshots." >> "$GITHUB_STEP_SUMMARY" - else - echo "::warning::Chromium install failed; the verification agent will produce a text-only report." + # Chromium system dependencies (apt packages) for evidence + # screenshots. The browser BINARY is downloaded after checkout + # by the "Install evidence browser" step, using the checkout's + # own Playwright so the binary always matches the lockfile. + # Best-effort: a failure here must not fail the job. + if ! (cd "${RUNNER_TEMP:?}" && npx --yes playwright@1.56.1 install-deps chromium); then + 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 @@ -2343,6 +2329,26 @@ 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 == ''" + run: |- + set -uo pipefail + # Download the browser binary matching the checkout's Playwright + # (node_modules/.bin/playwright resolves the exact lockfile + # version). System deps were installed as root in the tools step; + # this 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" + if runuser -u node -- env PLAYWRIGHT_BROWSERS_PATH="$PW_PATH" \ + npx playwright 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 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' @@ -2777,11 +2783,12 @@ jobs: "QWEN_CI_REAL_GH=${QWEN_CI_REAL_GH:-}" "QWEN_CI_REAL_GIT=${QWEN_CI_REAL_GIT:-}" ) - # Evidence screenshots: root pre-installed chromium into a shared, - # world-readable path (the agent is `node` and could not). 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. + # 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+=( @@ -3047,8 +3054,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/pr-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 @@ -3059,6 +3066,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${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 @@ -3078,9 +3086,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. @@ -3116,11 +3127,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 @@ -3128,7 +3139,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/.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 79bc3d188c..e6d6aa50a1 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -343,8 +343,9 @@ workflow globs). It must contain: not spend budget working around it. Route: `terminal-capture` skill (node-pty → xterm.js → Playwright PNG). - The publish job hosts what you produce on the `pr-assets` branch and - appends it below the report, capped at **8 images, 2 MB each**; anything + The publish job hosts what you produce on a per-PR branch + (`pr-assets/pr-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 diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index ff188782a1..4d1ea58b10 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1004,7 +1004,7 @@ describe('qwen-triage verify hardening', () => { // qwen under `env -i`, which needs no per-variable stripping. const prepare = verifyJob.slice( verifyJob.indexOf('Install and build PR app'), - verifyJob.indexOf('Run verification agent'), + verifyJob.indexOf("name: 'Install evidence browser'"), ); const commands = prepare.match(/runuser -u node -- env[\s\S]*?\n/g) ?? []; expect(commands.length).toBe(2); @@ -1353,7 +1353,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/pr7999-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/pr7999-verify`, ); writeFileSync( join(dir, 'gh'), @@ -1415,7 +1415,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/pr7999-verify | grep verify/ || true`, ) .stdout.trim() .split('\n') @@ -1431,8 +1431,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, @@ -2140,31 +2140,51 @@ describe('qwen-triage verify round-3 hardening', () => { expect(group).toContain("vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true'"); }); - // Evidence images: the hosting machinery has been complete since the lane - // shipped, and across 14 real reports it produced ZERO images. Two - // independent causes, both fixed here — the agent physically could not - // install chromium (runs as `node`, `env -i`, fresh HOME, no apt), and the - // skill framed captures as optional and TUI-only. + // 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'); - // Installed as ROOT here, because the agent cannot. - expect(tools).toContain('playwright'); - expect(tools).toContain('--with-deps chromium'); - // Shared, world-readable, and outside the agent's throwaway HOME. - expect(tools).toContain('PLAYWRIGHT_BROWSERS_PATH'); - expect(tools).toContain('chmod -R a+rX'); - // Best-effort: a failed browser install must not fail a verification. - expect(tools).toContain('::warning::Chromium install failed'); - expect(tools).not.toContain('exit 1'); + expect(tools).toContain('install-deps chromium'); + expect(tools).not.toContain('install --with-deps'); + expect(tools).toContain('::warning::Chromium system deps install failed'); + + // Browser binary: downloaded after npm ci so npx resolves the + // checkout's lockfile version — never a hardcoded pin (M5). + const browser = stepIn('verify', 'Install evidence browser'); + expect(browser).toContain('npx playwright install chromium'); + expect(browser).not.toMatch(/playwright@[\d.]/); + expect(browser).toContain('PLAYWRIGHT_BROWSERS_PATH'); + // 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. + const ifIdx = browser.indexOf('if runuser'); + const markerIdx = browser.indexOf('verify-chromium-path'); + const elseIdx = browser.indexOf( + '::warning::Chromium browser download failed', + ); + 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'); // 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'); - const marker = runStep.indexOf('QWEN_VERIFY_CHROMIUM=1'); const guard = runStep.indexOf('verify-chromium-path'); + const marker = runStep.indexOf('QWEN_VERIFY_CHROMIUM=1'); expect(guard).toBeLessThan(marker); + // The if-fi block must wrap the QWEN_ENV+= assignment. + const ifBlock = runStep.slice(guard - 20, marker + 30); + expect(ifBlock).toContain('if'); + 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( @@ -2180,6 +2200,19 @@ describe('qwen-triage verify round-3 hardening', () => { expect(flat).not.toContain('Optionally `evidence/*.png`'); }); + // 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${PR_NUMBER}-verify'); + expect(publish).toContain('checkout -q --orphan'); + expect(publish).not.toMatch(/--branch pr-assets["\s]/); + }); + // Cleanups must never descend through a PR-writable parent, and an // outward-resolving hooks entry must be removed rather than reported. it('survives symlink escapes in the workspace cleanup', () => {