From bb518cee863ce66cbf602c3b13a786e080ca9c11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Sat, 29 Aug 2026 04:36:57 +0000 Subject: [PATCH] ci: host PR evidence outside Git (#9985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: host PR evidence outside Git Co-authored-by: Qwen-Coder * ci: pin node and ossutil paths in privileged OSS uploads The publish-verify evidence upload resolved `node` through a PATH prefixed with $RUNNER_TEMP — the one directory PR code can write to via the verify container's bind mount — so a planted fake `node` there won interpreter resolution inside the step that holds CI_BOT_PAT and the OSS config (PATH hijack). Resolve node under the inherited clean PATH and run ossutil from a fresh job-private copy of the sha256-verified binary installed above instead of resolving either through $RUNNER_TEMP. Apply the same shape to web-shell-visuals-publish.yml so the pattern cannot regress if that job ever moves to a persistent pool. Co-authored-by: Qwen-Coder * ci: make OSS setup best-effort and pair bucket/base-URL defaults Mirror qwen-triage.yml in web-shell-visuals-publish.yml: mark the ossutil install and credential-config steps continue-on-error so a setup failure cannot abort the no-image marker-comment path (the upload itself stays loud when images are present). In both publishers, derive the default ALIYUN_OSS_PUBLIC_BASE_URL from ALIYUN_OSS_BUCKET so overriding only one of the two vars cannot post comment links that 404 against (or show stale objects from) the other bucket. Re-record both workflow sizes in the baseline. Co-authored-by: Qwen-Coder * ci: enrich evidence-upload warning with local preconditions The single "Failed to upload evidence images" warning could not distinguish its causes: the ossutil install and credential-config steps run with continue-on-error and render green when they fail, so a missing binary or config looks identical to a transient upload blip or rotated keys. Probe both preconditions into the warning (ossutil=ok/MISSING, config=ok/MISSING) so the investigator sees the gap without trawling three steps of logs. Co-authored-by: Qwen-Coder * test: enforce the uploader flag contract and pin the production arm The evidence-hosting stub shifted --config away unvalidated, and every harness run sets VERIFY_ASSETS_UPLOADER, so the production `node "$uploader"` arm and its --config flag were never exercised: dropping --config from the workflow kept every test green while the real uploader exits 1 on it, silently degrading every /verify report to text-only. Make the stub reject a missing/empty --bucket, --config or --prefix, and pin the production invocation shape (inherited-PATH node resolution plus all three ossutil flags). Mutation-probed: with --config removed from the workflow both tests now fail. Co-authored-by: Qwen-Coder * test: pin the ossutil credential lifecycle in both publishers Nothing pinned the credential lifecycle this PR adds — the sha256-checked ossutil install, the credential-config step, and the if:always() cleanup — so a future edit dropping any of them (or the always() condition) would leave every test green while the OSS key pair persists in $RUNNER_TEMP on the persistent ecs-qwen pool. Add shape assertions for all three steps in qwen-triage.yml and web-shell-visuals-publish.yml, mirroring the release/desktop wiring pins in install-script.test.js and desktop-oss-workflow.test.js. Co-authored-by: Qwen-Coder * docs: align assets-repo guidance with the self-targeting guard The PR-review workflow now blanks a QWEN_REVIEW_ASSETS_REPO that equals the repository under review, but the user doc, the bundled review skill, and the parseAssetsRepo unset-error still recommended pointing it at the repo under review — so a maintainer following the published guidance landed in the "not set" refusal path, and the refusal message re-recommended the exact value the guard had just rejected. Update all three surfaces to the new contract: a dedicated external image-host repository (or a fork/scratch repo), with unset or self-targeting values deliberately degrading to prose and local artifact paths. Co-authored-by: Qwen-Coder * test: execute the visuals hosting block against a stub uploader The workflow-level assertions covered the OSS upload path with shape regexes only, so a --prefix that drifts away from RAW_BASE (every preview URL 404s) or a dropped --config line (the uploader hard-fails before any comment posts) both left the suite green. Extract the hosting block and run it against a stub uploader that mirrors the real flag contract and records the upload destination, in the same pattern as the verify path in scripts/tests/qwen-triage-workflow.test.js: the images arm must land the staged files under $STUB_ROOT/$ALIYUN_OSS_BUCKET/pr-assets/web-shell-visuals// with the exact --bucket/--config/--prefix wiring, and RAW_BASE must equal that layout; the no-change arm must never invoke the uploader. Co-authored-by: Qwen-Coder * fix(ci): gate ossutil consumers on verified install Prevent the persistent publish runner from executing a stale binary under RUNNER_TEMP when the best-effort install step fails. Pin both the install outcome gate and the hardened PATH invocation in focused workflow tests. Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder * fix(ci): grant publisher checkout access Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder * test(ci): pin OSS publisher failure paths Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder * test(ci): exercise production OSS dispatch Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder * fix(ci): isolate OSS publisher on hosted runner * fix(ci): sync size baseline and stale runner comments Co-authored-by: Qwen-Coder * fix(ci): give each visuals publish a fresh OSS prefix The web-shell prefix was pr-assets/web-shell-visuals//, so a re-run for the same head wrote back over the object keys the already-posted comment referenced. GitHub serves comment images through a caching proxy, so reviewers would keep seeing the previous run's screenshots at a URL whose bytes had changed. The Git-backed design got cache-busting free from the per-run commit SHA; add the run id back to restore it (the verify lane already carries -). Also: - Resolve the destination as ALIYUN_OSS_PR_ASSETS_BUCKET -> ALIYUN_OSS_BUCKET -> qwen-code-assets in both publishers, with the public base URL derived from whichever wins. Unset, this is byte-for-byte today's behaviour; set, it moves untrusted PR-derived evidence off the bucket that also serves release, desktop, and live-host downloads without a workflow change. - Sparse-check out /package.json alongside the uploader: it is ESM in a .js file, and package.json declares engines >= 22.0 while neither publisher pins a Node, so without the "type": "module" marker the uploader only parses on the Node versions that infer module syntax. - Require the credential file, not just a green install outcome, before dispatching the /verify upload. The configure step is continue-on-error, so a missing or rotated secret otherwise cost three retry backoffs per image before degrading to the same text-only report. - Drop the stale "runs on the persistent ECS pool" premise from the clear-stale-downloads step; publish-verify is on ubuntu-latest now. * fix(ci): retry the ossutil download in both PR publishers The release syncs download ossutil with no retry, which is fine there: a red run is rerun by hand. Here the consequence differs. A CDN blip in the web-shell publisher leaves no ossutil for the upload, and since the upload aborts the step under set -e, the PR loses its preview comment entirely rather than degrading; in the verify publisher it silently costs the report its evidence images. Three retries with --retry-all-errors covers the transient class without changing any success path. Also reword the unset-QWEN_REVIEW_ASSETS_REPO refusal. It ended with a flat claim about the PR-review workflow blanking a self-targeting designation, which reads as a non sequitur to the far more common reader: someone running the CLI locally who simply never set the variable. Keep the diagnostic — a maintainer whose repository variable IS set needs it — but scope it to CI and say why self-targeting is discouraged in the first place. * fix(ci): include run attempt in the web-shell OSS asset prefix The workflow_run id is stable across re-run attempts, so a maintainer re-run of the visuals publish overwrote the exact object keys an already-posted comment references (camo keeps rendering attempt-1 shots). Append run_attempt to the prefix, mirroring the verify lane, and cover same-runId attempt-1-vs-attempt-2 in the hosting harness. Co-authored-by: Qwen-Coder * fix(ci): close out PR evidence hosting review findings - The web-shell visuals prefix gains the run attempt: a re-run keeps the run id and only increments the attempt, so the old prefix let a same-head re-run write back over the object keys an already-posted comment references (camo keeps serving the stale screenshots). Mirrors the verify lane -; the re-run test now models same-run attempt 1 vs 2, not just distinct run ids. - The QWEN_REVIEW_ASSETS_REPO self-targeting guard now trims and compares case-insensitively before the CLI reads the value, so padded or case-shifted self-references degrade like an unset one. - Both publisher checkouts pin ref: github.sha (the default-branch head on their events) instead of relying on default resolution. - The ossutil download retry budget fits the 10-minute job cap in both publishers (~6.1 min worst case; was ~20 min). - Upload attempts are bounded via OSS_UPLOAD_ATTEMPT_TIMEOUT_MS so a stalled ossutil degrades to the text-only report / re-triggerable publish instead of burning the job cap; release syncs stay unbounded (the knob is opt-in). - Fix the serving-model comment (the extension allowlist, not the magic bytes, is what pins the served Content-Type) and pin what earlier suites left unpinned: sparse-checkout lists, bucket-to-URL derivation, curl retry flags, persist-credentials, and the ossutil isolation lines. Both uploader stubs now require the --config file to exist, so config drift turns the harnesses red. - runHostingBlock tears down its mkdtemp fixture instead of leaking it on every call. # Conflicts: # .github/scripts/web-shell-visuals-publish.test.mjs # .github/workflows/web-shell-visuals-publish.yml # docs/design/2026-08-25-pr-evidence-oss-hosting.md * fix(ci): gate visuals-publish Configure step on ossutil install outcome The Install ossutil step keeps continue-on-error: true so a failed install must not block the no-image marker-comment path, but the unguarded Configure Aliyun OSS credentials step still ran and failed confusingly on the missing binary. Give the install step the install-ossutil id and gate Configure on its success outcome, matching the qwen-triage.yml twins. Co-authored-by: Qwen-Coder --------- Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder Co-authored-by: Shaojin Wen --- .../web-shell-visuals-publish.test.mjs | 417 +++++++++++++++++ .github/workflows/.size-baseline | 6 +- .github/workflows/qwen-code-pr-review.yml | 33 +- .github/workflows/qwen-triage.yml | 220 ++++++--- .../workflows/web-shell-visuals-cleanup.yml | 13 +- .../workflows/web-shell-visuals-publish.yml | 184 +++++--- .qwen/skills/verify-pr/SKILL.md | 5 +- .../2026-08-25-pr-evidence-oss-hosting.md | 99 ++++ docs/users/features/code-review.md | 4 +- .../cli/src/commands/review/lib/assets.ts | 8 +- .../bundled/review/references/posting.md | 2 +- scripts/tests/qwen-pr-review-workflow.test.js | 59 ++- scripts/tests/qwen-triage-workflow.test.js | 441 +++++++++++++++--- .../tests/upload-aliyun-oss-assets.test.js | 104 ++++- scripts/upload-aliyun-oss-assets.js | 53 ++- 15 files changed, 1417 insertions(+), 231 deletions(-) create mode 100644 docs/design/2026-08-25-pr-evidence-oss-hosting.md diff --git a/.github/scripts/web-shell-visuals-publish.test.mjs b/.github/scripts/web-shell-visuals-publish.test.mjs index 993bf2f3e6c..1964526c4fa 100644 --- a/.github/scripts/web-shell-visuals-publish.test.mjs +++ b/.github/scripts/web-shell-visuals-publish.test.mjs @@ -5,6 +5,18 @@ */ import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import test from 'node:test'; import { @@ -22,6 +34,411 @@ import { const PNG = '89504e470d0a1a0a'; const GIF89 = '474946383961'; const GIF87 = '474946383761'; +const workflow = readFileSync( + '.github/workflows/web-shell-visuals-publish.yml', + 'utf8', +); + +test('workflow hosts visuals on OSS without writing Git refs', () => { + assert.match(workflow, /scripts\/upload-aliyun-oss-assets\.js/); + // Head SHA, run id AND run attempt: GitHub's camo proxy caches comment + // images by URL, so a re-run for the same head must not write back over + // the object the previous comment already published — and the run id is + // stable across re-run attempts, so the attempt must be in the key too. + assert.match( + workflow, + /pr-assets\/web-shell-visuals\/\$\{PR\}\/\$\{RUN_HEAD_SHA\}\/\$\{RUN_ID\}\/\$\{RUN_ATTEMPT\}/, + ); + assert.match(workflow, /ALIYUN_OSS_PUBLIC_BASE_URL/); + assert.match( + workflow, + /RUN_ATTEMPT: '\$\{\{ github\.event\.workflow_run\.run_attempt \}\}'/, + ); + assert.match(workflow, /PATH="\$trusted_bin" "\$node_bin"/); + // ossutil must run from a fresh job-private dir, never $RUNNER_TEMP: a + // trusted_bin=$RUNNER_TEMP mutant would re-resolve the binary through the + // shared dir on every call and survive the PATH assertion above. + assert.match(workflow, /trusted_bin="\$\(mktemp -d\)"/); + assert.match( + workflow, + /install -m 0755 "\$\{RUNNER_TEMP\}\/ossutil" "\$trusted_bin\/ossutil"/, + ); + assert.doesNotMatch(workflow, /PATH="\$RUNNER_TEMP:\$PATH"/); + assert.doesNotMatch(workflow, /git push/); + assert.doesNotMatch(workflow, /checkout -q --orphan/); +}); + +test('workflow checks out only trusted base-repo scripts', () => { + // The job carries OSS credentials. Its checkout must pin the trusted ref + // (on workflow_run events github.sha is the default-branch head, never + // the PR head) and take only the publisher files — dropping /package.json + // makes the uploader's `.js` stop parsing as ESM on Node versions that + // don't infer module syntax, and this job pins no Node. + const checkout = + workflow.split("'Checkout the publish scripts'")[1]?.split('- name:')[0] ?? + ''; + assert.ok(checkout, 'checkout step not found'); + assert.match(checkout, /ref: '\$\{\{ github\.sha \}\}'/); + assert.match(checkout, /persist-credentials: false/); + assert.match(checkout, /sparse-checkout-cone-mode: false/); + for (const entry of [ + '/package.json', + '.github/scripts/web-shell-visuals-publish.mjs', + 'scripts/upload-aliyun-oss-assets.js', + 'scripts/release-script-utils.js', + ]) { + assert.ok( + checkout.includes(`\n ${entry}\n`), + `sparse-checkout entry missing: ${entry}`, + ); + } +}); + +test('workflow derives the public URL from the resolved bucket', () => { + // Overriding only one of the bucket/base-URL vars must not post comment + // links that 404 against (or show stale objects from) the other bucket: + // the default base URL is derived from whichever bucket wins. + const publish = + workflow.split("'Publish visuals to the PR'")[1]?.split('run: |-')[0] ?? ''; + assert.ok(publish, 'publish step env not found'); + assert.match( + publish, + /ALIYUN_OSS_BUCKET: "\$\{\{ vars\.ALIYUN_OSS_PR_ASSETS_BUCKET \|\| vars\.ALIYUN_OSS_BUCKET \|\| 'qwen-code-assets' \}\}"/, + ); + assert.match( + publish, + /ALIYUN_OSS_PUBLIC_BASE_URL: "\$\{\{ vars\.ALIYUN_OSS_PR_ASSETS_PUBLIC_BASE_URL \|\| \(vars\.ALIYUN_OSS_PR_ASSETS_BUCKET == '' && vars\.ALIYUN_OSS_PUBLIC_BASE_URL\) \|\| format\('https:\/\/\{0\}\.oss-cn-hangzhou\.aliyuncs\.com', vars\.ALIYUN_OSS_PR_ASSETS_BUCKET \|\| vars\.ALIYUN_OSS_BUCKET \|\| 'qwen-code-assets'\) \}\}"/, + ); + // A stalled upload is bounded per attempt so one black-hole socket cannot + // burn the whole 10-minute job cap and strand the preview comment. + assert.match(publish, /OSS_UPLOAD_ATTEMPT_TIMEOUT_MS: '120000'/); +}); + +test('workflow keeps the ossutil download retry budget inside the job cap', () => { + // A CDN retry loop whose worst case exceeds the 10-minute job cap turns + // the intended "degrade to the no-image path" into "job killed mid-install". + const install = + workflow.split("'Install ossutil'")[1]?.split('- name:')[0] ?? ''; + const m = install.match( + /curl -fsSL --retry (\d+) --retry-delay (\d+) --retry-all-errors \\\n\s+--connect-timeout (\d+) --max-time (\d+)/, + ); + assert.ok(m, 'curl retry flags not found in the install step'); + const worstSeconds = + (Number(m[1]) + 1) * Number(m[4]) + Number(m[1]) * Number(m[2]); + assert.ok( + worstSeconds < 10 * 60, + `worst-case download budget ${worstSeconds}s exceeds the 10-minute job cap`, + ); +}); + +test('workflow pins the ossutil credential lifecycle (sha256 install, config, always() cleanup)', () => { + // The sha256-verified install and the credential config it feeds are + // best-effort here (a setup failure must not block the no-image path)... + const install = + workflow.split("'Install ossutil'")[1]?.split('- name:')[0] ?? ''; + assert.match(install, /continue-on-error: true/); + assert.match(install, /sha256sum -c/); + const configure = + workflow + .split("'Configure Aliyun OSS credentials'")[1] + ?.split('- name:')[0] ?? ''; + assert.match(configure, /continue-on-error: true/); + assert.match(configure, /-c "\$RUNNER_TEMP\/\.ossutilconfig"/); + // ...but the cleanup must run even on failure, or the OSS key pair + // lingers in $RUNNER_TEMP after the job. + const cleanup = workflow.split("'Cleanup Aliyun OSS credentials'")[1] ?? ''; + assert.match(cleanup, /if: '\$\{\{ always\(\) \}\}'/); + assert.match(cleanup, /rm -f "\$RUNNER_TEMP\/\.ossutilconfig"/); +}); + +// --- Behavioural coverage for the OSS hosting block ----------------------- +// The shape regexes above pin the hosting contract as text; this harness +// EXECUTES the extracted hosting block against a stub uploader that mirrors +// scripts/upload-aliyun-oss-assets.js's flag contract (readOptionValue +// rejects a valueless --bucket/--config/--prefix; at least one asset is +// required) and records where the objects land — the same stub-uploader +// pattern as the verify path in scripts/tests/qwen-triage-workflow.test.js. +// A --prefix that drifts away from RAW_BASE (every preview URL 404s) or a +// dropped --config (the real uploader dies before any comment posts) fails +// here, where the shape regexes stay green. + +function hostingBlockSource() { + const raw = workflow + .split('# --- Host on Aliyun OSS')[1] + ?.split('\n') + .slice(1) // drop the marker line's trailing dashes — not a comment. + .join('\n') + .split('# --- Changed paths')[0] + .replace(/^ {10}/gm, ''); // the step run block's indentation. + assert.ok(raw, 'hosting block markers not found in the publish workflow'); + return raw; +} + +function listFiles(root) { + if (!existsSync(root)) { + return []; + } + const out = []; + const walk = (dir, prefix) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const rel = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + walk(join(dir, entry.name), rel); + } else { + out.push(rel); + } + } + }; + walk(root, ''); + return out; +} + +function runHostingBlock( + hasImages, + { + publicBaseUrl = 'https://assets.example.test', + uploadFails = false, + runId = '900001', + runAttempt = '1', + } = {}, +) { + const dir = mkdtempSync(join(tmpdir(), 'visuals-hosting-')); + try { + return runHostingBlockIn(dir, hasImages, { + publicBaseUrl, + uploadFails, + runId, + runAttempt, + }); + } finally { + // The fixture used to leak a mkdtemp dir per call; capture everything + // the assertions need inside, then tear it down. + rmSync(dir, { recursive: true, force: true }); + } +} + +function runHostingBlockIn( + dir, + hasImages, + { publicBaseUrl, uploadFails, runId, runAttempt }, +) { + const runnerTemp = join(dir, 'runner-temp'); + const stage = join(dir, 'stage'); + const work = join(dir, 'work'); + const stubRoot = join(dir, 'oss'); + const recordPath = join(dir, 'upload-record.json'); + mkdirSync(runnerTemp, { recursive: true }); + mkdirSync(stage, { recursive: true }); + mkdirSync(join(work, 'scripts'), { recursive: true }); + // The block installs its job-private ossutil copy from here; the stub + // uploader never executes it, but the install must succeed. + writeFileSync(join(runnerTemp, 'ossutil'), 'fake ossutil\n'); + writeFileSync(join(stage, 'home-light.png'), 'png-bytes'); + writeFileSync(join(stage, 'model-switch.gif'), 'gif-bytes'); + // Stub uploader: enforces the real flag contract, then copies the objects + // under $OSS_STUB_ROOT// and records the flags it saw. + writeFileSync( + join(work, 'scripts', 'upload-aliyun-oss-assets.js'), + [ + "'use strict';", + "const { copyFileSync, mkdirSync, writeFileSync } = require('node:fs');", + "const { basename, join } = require('node:path');", + "if (process.env.OSS_STUB_FAIL === '1') process.exit(1);", + "const opts = { bucket: '', config: '', prefix: '' };", + 'const assets = [];', + 'const argv = process.argv.slice(2);', + 'for (let i = 0; i < argv.length; i += 1) {', + ' const arg = argv[i];', + " if (arg === '--bucket' || arg === '--config' || arg === '--prefix') {", + ' opts[arg.slice(2)] = argv[i + 1] ?? "";', + ' i += 1;', + ' } else {', + ' assets.push(arg);', + ' }', + '}', + 'for (const [name, value] of Object.entries(opts)) {', + ' if (!value || value.startsWith("-")) {', + ' console.error(`upload-assets stub: --${name} requires a value`);', + ' process.exit(1);', + ' }', + '}', + 'if (assets.length === 0) {', + ' console.error("upload-assets stub: at least one ASSET path is required");', + ' process.exit(1);', + '}', + 'const target = join(process.env.OSS_STUB_ROOT, opts.bucket, opts.prefix);', + 'mkdirSync(target, { recursive: true });', + 'for (const file of assets) {', + ' copyFileSync(file, join(target, basename(file)));', + '}', + 'writeFileSync(process.env.OSS_STUB_RECORD, JSON.stringify(opts));', + ].join('\n'), + ); + const pr = '4242'; + const headSha = 'cafe4242cafe4242cafe4242cafe4242cafe4242'; + const script = [ + 'set -euo pipefail', + `HAS_IMAGES=${hasImages ? 1 : 0}`, + `PR=${pr}`, + `RUN_HEAD_SHA=${headSha}`, + `RUN_ID=${runId}`, + `RUN_ATTEMPT=${runAttempt}`, + `STAGE=${JSON.stringify(stage)}`, + 'ALIYUN_OSS_BUCKET=assets-bucket', + `ALIYUN_OSS_PUBLIC_BASE_URL=${JSON.stringify(publicBaseUrl)}`, + `RUNNER_TEMP=${JSON.stringify(runnerTemp)}`, + hostingBlockSource(), + 'printf \'RAW_BASE=%s\\n\' "$RAW_BASE"', + ].join('\n'); + const driverPath = join(dir, 'driver.sh'); + writeFileSync(driverPath, script); + const res = spawnSync('bash', [driverPath], { + encoding: 'utf8', + cwd: work, + env: { + ...process.env, + OSS_STUB_ROOT: stubRoot, + OSS_STUB_RECORD: recordPath, + OSS_STUB_FAIL: uploadFails ? '1' : '0', + }, + }); + // Capture the outcomes the caller asserts on before the fixture dir is + // torn down: the parsed upload record (null when the uploader never ran + // or wrote nothing) and the flat listing of everything hosted. + const record = existsSync(recordPath) + ? JSON.parse(readFileSync(recordPath, 'utf8')) + : null; + const hostedFiles = listFiles(stubRoot).sort(); + return { + res, + record, + hostedFiles, + pr, + headSha, + runId, + runAttempt, + runnerTemp, + }; +} + +test('hosting block uploads staged images to the exact prefix RAW_BASE promises', () => { + const { + res, + record, + hostedFiles, + pr, + headSha, + runId, + runAttempt, + runnerTemp, + } = runHostingBlock(true); + assert.equal(res.status, 0, res.stderr); + // Flag wiring: the uploader gets the bucket, the credential file the + // configure step writes, and the prefix the comment URLs are built from. + const prefix = `pr-assets/web-shell-visuals/${pr}/${headSha}/${runId}/${runAttempt}`; + assert.deepEqual(record, { + bucket: 'assets-bucket', + config: `${runnerTemp}/.ossutilconfig`, + prefix, + }); + // Upload-prefix ↔ URL agreement: the objects land exactly where RAW_BASE + // (the comment's image base URL) points. + assert.deepEqual(hostedFiles, [ + `assets-bucket/${prefix}/home-light.png`, + `assets-bucket/${prefix}/model-switch.gif`, + ]); + assert.match( + res.stdout, + new RegExp( + `^RAW_BASE=https://assets\\.example\\.test/pr-assets/web-shell-visuals/${pr}/${headSha}/${runId}/${runAttempt}$`, + 'm', + ), + ); + assert.match( + res.stdout, + new RegExp( + `Web-shell visuals hosted at https://assets\\.example\\.test/pr-assets/web-shell-visuals/${pr}/${headSha}/${runId}/${runAttempt}\\.`, + ), + ); +}); + +// The regression this guards: publishing back onto the previous run's object +// keys. GitHub serves comment images through a caching proxy, so a re-run for +// the same head would keep showing the stale screenshots. A re-run comes in +// TWO shapes: a brand-new workflow run (fresh run id) and a re-run of the +// SAME run — which keeps the run id and only increments the attempt, and +// still re-fires workflow_run `completed`. Both must get fresh prefixes; +// dropping the attempt component from ASSET_PREFIX turns this red. +test('hosting block gives a re-run of the same head a fresh, non-colliding prefix', () => { + const first = runHostingBlock(true, { runId: '900001', runAttempt: '1' }); + assert.equal(first.res.status, 0, first.res.stderr); + const rerunSameRun = runHostingBlock(true, { + runId: '900001', + runAttempt: '2', + }); + assert.equal(rerunSameRun.res.status, 0, rerunSameRun.res.stderr); + const freshRun = runHostingBlock(true, { runId: '900002', runAttempt: '1' }); + assert.equal(freshRun.res.status, 0, freshRun.res.stderr); + const prefixes = [first, rerunSameRun, freshRun].map((r) => r.record.prefix); + assert.equal(new Set(prefixes).size, 3); + // All still hang off the same immutable per-head path, so the head SHA + // stays the thing that binds a URL to the code it depicts. + const head = `pr-assets/web-shell-visuals/${first.pr}/${first.headSha}/`; + for (const prefix of prefixes) { + assert.ok(prefix.startsWith(head), `unexpected prefix: ${prefix}`); + } +}); + +// A maintainer re-run of the SAME workflow run keeps its run id — only +// run_attempt increments. Without the attempt segment the re-run would +// overwrite the exact object keys the attempt-1 comment already references +// (and camo-cached), so attempts of one run must land on distinct prefixes. +test('hosting block gives re-run attempts of the SAME run distinct prefixes', () => { + const first = runHostingBlock(true, { runId: '900001', runAttempt: '1' }); + assert.equal(first.res.status, 0, first.res.stderr); + const second = runHostingBlock(true, { runId: '900001', runAttempt: '2' }); + assert.equal(second.res.status, 0, second.res.stderr); + const prefixOf = (r) => r.record.prefix; + assert.equal( + prefixOf(first), + `pr-assets/web-shell-visuals/${first.pr}/${first.headSha}/900001/1`, + ); + assert.equal( + prefixOf(second), + `pr-assets/web-shell-visuals/${first.pr}/${first.headSha}/900001/2`, + ); + assert.notEqual(prefixOf(first), prefixOf(second)); +}); + +test('hosting block skips the uploader entirely on the no-change arm', () => { + const { res, record } = runHostingBlock(false); + assert.equal(res.status, 0, res.stderr); + assert.match(res.stdout, /^RAW_BASE=$/m); // empty -> image-less comment + assert.doesNotMatch(res.stdout, /hosted at/); + assert.equal(record, null); // the uploader never ran +}); + +test('hosting block aborts without publishing a URL when upload fails', () => { + const { res, record } = runHostingBlock(true, { uploadFails: true }); + assert.notEqual(res.status, 0); + assert.doesNotMatch(res.stdout, /^RAW_BASE=/m); + assert.equal(record, null); +}); + +test('hosting block strips a trailing slash from the public base URL', () => { + const { res, pr, headSha, runId, runAttempt } = runHostingBlock(true, { + publicBaseUrl: 'https://assets.example.test/', + }); + assert.equal(res.status, 0, res.stderr); + assert.match( + res.stdout, + new RegExp( + `^RAW_BASE=https://assets\\.example\\.test/pr-assets/web-shell-visuals/${pr}/${headSha}/${runId}/${runAttempt}$`, + 'm', + ), + ); + assert.doesNotMatch(res.stdout, /assets\.example\.test\/\/pr-assets/); +}); test('sanitizeName preserves the extension (regression: a trailing char broke the .png filter)', () => { assert.equal( diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 515a080ccf6..33d481a6b99 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,12 +36,12 @@ 5942 qwen-autofix-fork-signal.yml 426924 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -165876 qwen-code-pr-review.yml +168310 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml 27648 qwen-triage-finalize.yml -344548 qwen-triage.yml +350381 qwen-triage.yml 9657 release-sdk-java.yml 22037 release-sdk-python.yml 19094 release-sdk.yml @@ -60,6 +60,6 @@ 10138 sync-release-to-oss.yml 3303 update-ecs-runner-qwen.yml 2307 web-shell-visuals-cleanup.yml -15380 web-shell-visuals-publish.yml +20047 web-shell-visuals-publish.yml 16384 web-shell-visuals.yml 4712 windows-runner-smoke.yml diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 0c5e046b9a0..f5adb2eee5d 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -924,17 +924,12 @@ jobs: QWEN_CODE_API_TIMEOUT_MS: '600000' QWEN_STREAM_IDLE_TIMEOUT_MS: '600000' QWEN_STREAM_MAX_LIFETIME_MS: '1800000' - # Evidence-image destination for `qwen review publish-assets` — - # OPT-IN by design: the command refuses to push images anywhere the - # user did not designate, so with the repository variable unset the - # review simply keeps its evidence as prose and local paths. Set the - # variable (e.g. to this repository) to let reviews embed capture - # PNGs; images land on commit-pinned pr-assets/-review branches, - # pushed with the same CI_BOT_PAT this step already uses for gh. - # Retention: the visuals cleanup workflow deletes those branches - # only when the variable points at THIS repository — a fork or - # scratch destination manages its own retention. - QWEN_REVIEW_ASSETS_REPO: '${{ vars.QWEN_REVIEW_ASSETS_REPO }}' + # Evidence-image destination for `qwen review publish-assets`. + # It must be a dedicated external host repository: project-local + # image branches are fetched by ordinary clones and permanently + # inflate this repository. Unset or self-targeting configurations + # deliberately degrade to prose and local artifact paths. + QWEN_REVIEW_ASSETS_REPO: "${{ vars.QWEN_REVIEW_ASSETS_REPO != github.repository && vars.QWEN_REVIEW_ASSETS_REPO || '' }}" # Per-run agent home so this review's session/memory cannot leak into # the next on the reused self-hosted workspace (reset in "Clean stale # agent state"). Must match the QWEN_HOME computed there. @@ -965,6 +960,22 @@ jobs: } REPO="${GITHUB_REPOSITORY}" + + # Normalize the assets-repo designation before the CLI reads it. + # The env-level guard compares the RAW variable, so whitespace + # variants (" QwenLM/qwen-code ") and case variants slip past it; + # trim both ends and re-check self-targeting case-insensitively + # (repository names are case-insensitive). A padded or case-shifted + # self-reference must degrade to prose exactly like an unset one. + QWEN_REVIEW_ASSETS_REPO="${QWEN_REVIEW_ASSETS_REPO#"${QWEN_REVIEW_ASSETS_REPO%%[![:space:]]*}"}" + QWEN_REVIEW_ASSETS_REPO="${QWEN_REVIEW_ASSETS_REPO%"${QWEN_REVIEW_ASSETS_REPO##*[![:space:]]}"}" + if [ -n "${QWEN_REVIEW_ASSETS_REPO}" ] && + [ "$(printf '%s' "${QWEN_REVIEW_ASSETS_REPO}" | tr '[:upper:]' '[:lower:]')" = \ + "$(printf '%s' "${REPO}" | tr '[:upper:]' '[:lower:]')" ]; then + QWEN_REVIEW_ASSETS_REPO='' + fi + export QWEN_REVIEW_ASSETS_REPO + REVIEW_URL="${GITHUB_SERVER_URL}/${REPO}/pull/${PR_NUMBER}" LOG_PATH="${RUNNER_TEMP:-/tmp}/qwen-review-pr-${PR_NUMBER}.jsonl" # Set by configure_qwen_network once the wrapper dir exists. diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index a06868752a4..73a7114a4fc 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -4980,8 +4980,8 @@ jobs: echo "Workspace wiped after external code (deny-by-default cleanup for the next pool job)." >> "$GITHUB_STEP_SUMMARY" # Post the verification report back to the PR. Runs on a clean GitHub-hosted - # runner with the write PAT and never checks out PR code, so the write - # credential is isolated from the untrusted-code execution in verify above — + # runner with the write PAT and only checks out trusted base-repo publisher + # scripts, so credentials stay isolated from untrusted execution above — # the same split as publish-tmux. Unlike publish-tmux it also reports the # skip/na cases: /verify is always an explicit request, so silence would # read as a lost run. @@ -5003,31 +5003,82 @@ jobs: # inherit the 360-minute default and a hung gh call could hold a hosted # runner for six hours. timeout-minutes: 10 - # Checks out nothing and runs no repository code (base-repo YAML events), - # so the persistent ECS pool is safe. - # Kill-switch: MAINTAINER_ECS_RUNNER_DISABLED. - runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' + # This job carries OSS credentials, so it must stay isolated from the + # persistent runner pool that executes untrusted PR code. + runs-on: 'ubuntu-latest' permissions: + contents: 'read' pull-requests: 'write' steps: - - name: 'Restore workspace ownership' - if: "${{ runner.environment == 'self-hosted' }}" - run: |- - set -uo pipefail - RUNNER_UID="$(id -u)" - RUNNER_GID="$(id -g)" - if [ "$RUNNER_UID" != "0" ]; then - chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace ownership; artifact download may fail on leftover root-owned files" - fi - chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; artifact download may fail on leftover read-only files" + # Sparse — the uploader, its helper dependency, and the root + # package.json that marks the uploader's `.js` as ESM (without it the + # uploader only parses on the Node versions that infer module syntax, + # and this job pins no Node). + - name: 'Checkout the OSS publisher scripts' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + # Pin the trusted ref explicitly: publish-verify only runs under + # issue_comment events (needs: verify), where github.sha is the + # default-branch head — never a PR merge ref. The pin makes + # "never PR code" a property of this step instead of an inference + # from the event's default resolution. + ref: '${{ github.sha }}' + sparse-checkout: |- + /package.json + scripts/upload-aliyun-oss-assets.js + scripts/release-script-utils.js + sparse-checkout-cone-mode: false + persist-credentials: false - # The publish job runs on the persistent ECS pool and downloads into - # a WORKSPACE-relative directory, which the runner does not clean - # between jobs: without this, a previous run's (possibly a different - # PR's) flake-gate.log survives and the publisher — which treats the - # file's presence as proof THIS run staged it — embeds it as this - # run's evidence. The verify side already applies the same rm-first - # rule to its own $RUNNER_TEMP tree. + - name: 'Install ossutil' + id: 'install-ossutil' + # Evidence hosting is best-effort; the report still posts as text if + # the client cannot be installed. + continue-on-error: true + env: + OSSUTIL_URL: "${{ vars.OSSUTIL_URL || 'https://gosspublic.alicdn.com/ossutil/1.7.19/ossutil-v1.7.19-linux-amd64.zip' }}" + OSSUTIL_SHA256: "${{ vars.OSSUTIL_SHA256 || 'dcc512e4a893e16bbee63bc769339d8e56b21744fd83c8212a9d8baf28767343' }}" + run: |- + set -euo pipefail + tmp_dir="$(mktemp -d)" + trap 'rm -rf "$tmp_dir"' EXIT + # Retry, unlike the release syncs: a red run there is simply rerun, + # but a CDN blip here costs this PR its evidence images. The + # worst-case budget stays inside the 10-minute job cap: 3 attempts + # x 120s max-time + 2 x 2s delays ≈ 6.1 minutes (an unreachable + # mirror fails the connect-timeout long before that). + curl -fsSL --retry 2 --retry-delay 2 --retry-all-errors \ + --connect-timeout 10 --max-time 120 "$OSSUTIL_URL" -o "$tmp_dir/ossutil.zip" + echo "$OSSUTIL_SHA256 $tmp_dir/ossutil.zip" | sha256sum -c - + unzip -q "$tmp_dir/ossutil.zip" -d "$tmp_dir" + ossutil_path="$(find "$tmp_dir" -type f \( -name 'ossutil' -o -name 'ossutil64' \) -print -quit)" + [ -n "$ossutil_path" ] || { echo '::error::ossutil binary not found'; exit 1; } + chmod +x "$ossutil_path" + install -m 0755 "$ossutil_path" "$RUNNER_TEMP/ossutil" + "$RUNNER_TEMP/ossutil" >/dev/null + + - name: 'Configure Aliyun OSS credentials' + if: "${{ steps.install-ossutil.outcome == 'success' }}" + continue-on-error: true + env: + ALIYUN_OSS_ACCESS_KEY_ID: '${{ secrets.ALIYUN_OSS_ACCESS_KEY_ID }}' + ALIYUN_OSS_ACCESS_KEY_SECRET: '${{ secrets.ALIYUN_OSS_ACCESS_KEY_SECRET }}' + ALIYUN_OSS_ENDPOINT: "${{ vars.ALIYUN_OSS_ENDPOINT || 'https://oss-cn-hangzhou.aliyuncs.com' }}" + run: |- + set -euo pipefail + if [ -z "$ALIYUN_OSS_ACCESS_KEY_ID" ] || [ -z "$ALIYUN_OSS_ACCESS_KEY_SECRET" ]; then + echo '::error::Missing Aliyun OSS credentials.' + exit 1 + fi + "$RUNNER_TEMP/ossutil" config -e "$ALIYUN_OSS_ENDPOINT" -i "$ALIYUN_OSS_ACCESS_KEY_ID" -k "$ALIYUN_OSS_ACCESS_KEY_SECRET" -L EN -c "$RUNNER_TEMP/.ossutilconfig" + + # Belt-and-braces against a stale download directory. The job moved to + # an ephemeral hosted runner (and checkout cleans the workspace first), + # so nothing should survive between runs — but the publisher treats the + # presence of a file like flake-gate.log as proof THIS run staged it, + # and that inference must not silently start embedding another PR's + # evidence if this job is ever moved back onto the persistent pool. + # The verify side applies the same rm-first rule to its $RUNNER_TEMP. - name: 'Clear stale downloaded results' run: 'rm -rf verify-results' @@ -5058,6 +5109,25 @@ jobs: VERIFY_RESULT: '${{ needs.verify.result }}' DOWNLOAD_OUTCOME: '${{ steps.download.outcome }}' RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' + OSSUTIL_INSTALL_OUTCOME: '${{ steps.install-ossutil.outcome }}' + # PR evidence is untrusted, PR-derived content, so it gets its own + # bucket knob: set ALIYUN_OSS_PR_ASSETS_BUCKET to move it off the + # bucket that also serves release, desktop, and live-host downloads. + # Unset, it falls back to the shared bucket — today's behaviour. + # The default public URL is derived from the resolved bucket so that + # overriding only one of the two vars cannot post comment links + # that 404 against (or show stale objects from) the other bucket. + # A cross-region ALIYUN_OSS_ENDPOINT override must also override + # ALIYUN_OSS_PUBLIC_BASE_URL because the default host pins Hangzhou. + ALIYUN_OSS_BUCKET: "${{ vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || 'qwen-code-assets' }}" + ALIYUN_OSS_PUBLIC_BASE_URL: "${{ vars.ALIYUN_OSS_PR_ASSETS_PUBLIC_BASE_URL || (vars.ALIYUN_OSS_PR_ASSETS_BUCKET == '' && vars.ALIYUN_OSS_PUBLIC_BASE_URL) || format('https://{0}.oss-cn-hangzhou.aliyuncs.com', vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || 'qwen-code-assets') }}" + # A stalled upload must not burn this job's 10-minute cap and take + # the whole report with it: bound every ossutil attempt so the + # uploader exhausts its retries and degrades to a text-only report + # instead of hanging one black-hole socket until the job dies. + # Worst case is one image's 3 attempts x 120s + backoffs ≈ 6.1 min + # (the uploader stops at the first image that fails all attempts). + OSS_UPLOAD_ATTEMPT_TIMEOUT_MS: '120000' # shellcheck disable=SC2016 run: |- set -euo pipefail @@ -5362,21 +5432,19 @@ jobs: printf '\n' } - # 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 + # Host the agent's evidence images (if any) on Aliyun OSS 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 # find predicates enforce both), and any failure degrades to a # text-only comment rather than blocking the report. The - # VERIFY_ASSETS_REMOTE override exists as a test seam only. + # VERIFY_ASSETS_UPLOADER override exists as a test seam only. EVIDENCE_SECTION='' 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}" + local prefix="pr-assets/verify/pr${PR_NUMBER}-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT:-1}" + local stage_dir="${RUNNER_TEMP:-/tmp}/verify-assets-stage" + local uploader="${VERIFY_ASSETS_UPLOADER:-scripts/upload-aliyun-oss-assets.js}" # `|| true` on every find: the artifact download is # continue-on-error, so verify-results may not exist at all, and # a bare non-zero find under `pipefail` would abort the step @@ -5393,19 +5461,8 @@ jobs: EVIDENCE_SECTION="_${total} evidence image(s) were produced but none passed the hosting caps (8 images, ≤2 MB each); see the run artifacts._"$'\n\n' return 0 fi - rm -rf "$clone_dir" - 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. - git -C "$clone_dir" config user.name 'qwen-code-ci-bot' - git -C "$clone_dir" config user.email 'qwen-code-ci-bot@users.noreply.github.com' - mkdir -p "$clone_dir/$dest_dir" + rm -rf "$stage_dir" + mkdir -p "$stage_dir" for f in "${imgs[@]}"; do base="$(basename "$f")" safe="$(printf '%s' "$base" | tr -cd 'a-zA-Z0-9._-' | head -c 80)" @@ -5418,10 +5475,14 @@ jobs: # Two artifact dirs can sanitize to the same name; the second # copy would silently overwrite the first and render twice. case "$seen" in *" $safe "*) continue ;; esac - # Extension is attacker-choosable; the magic bytes are what - # raw.githubusercontent.com will actually serve. PNG only. + # Extension is attacker-choosable, and it is the enforcement + # point for what OSS serves: ossutil derives the object's + # Content-Type from the key's extension, so the .png allowlist + # above is what keeps these hosted as an image type. This + # magic-byte check then verifies the bytes actually match the + # claimed type. PNG only. [ "$(head -c 8 "$f" | od -An -tx1 | tr -d ' \n')" = '89504e470d0a1a0a' ] || continue - cp "$f" "$clone_dir/$dest_dir/$safe" || continue + cp "$f" "$stage_dir/$safe" || continue seen="${seen}${safe} " hosted=$((hosted + 1)) done @@ -5430,26 +5491,55 @@ jobs: EVIDENCE_SECTION="_${total} evidence image(s) were produced but none passed the hosting checks (PNG magic, unique sanitized name, ≤2 MB, max 8); see the run artifacts._"$'\n\n' return 0 fi - if ! ( - cd "$clone_dir" && - git add "$dest_dir" && - git commit -q -m "verify evidence for PR #${PR_NUMBER} (run ${GITHUB_RUN_ID})" && - { - 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 "$assets_branch" 2>/dev/null && - git push -q origin "HEAD:$assets_branch" 2>/dev/null - } - } - ); then - echo "::warning::Failed to push evidence images; posting a text-only report." >&2 + local upload_cmd=() + local upload_path="${PATH}" + local trusted_bin='' + if [ -n "${VERIFY_ASSETS_UPLOADER:-}" ]; then + upload_cmd=("$uploader") + elif [ "${OSSUTIL_INSTALL_OUTCOME:-}" = 'success' ] && + [ -f "${RUNNER_TEMP:-/tmp}/.ossutilconfig" ]; then + # Resolve node under the inherited PATH, never through the + # shared temp dir, and run ossutil from a fresh job-private + # copy of the sha256-verified binary installed above instead + # of resolving it through $RUNNER_TEMP (PATH hijack shape). + # Not exploitable on today's ephemeral ubuntu-latest runner, + # but the shape must not regress if this job ever moves to a + # persistent pool. + local node_bin + node_bin="$(command -v node || true)" + upload_cmd=("$node_bin" "$uploader") + trusted_bin="$(mktemp -d || true)" + if [ -n "$trusted_bin" ]; then + install -m 0755 "${RUNNER_TEMP:-/tmp}/ossutil" "$trusted_bin/ossutil" 2>/dev/null || true + upload_path="$trusted_bin" + fi + else + # The install and configure steps are continue-on-error, so a + # failure there renders green; say which precondition is missing + # instead of burning three retry backoffs per image on an + # uploader that cannot possibly authenticate. + echo "::warning::OSS publishing unavailable (ossutil=$([ -x "${RUNNER_TEMP:-/tmp}/ossutil" ] && echo ok || echo MISSING), config=$([ -f "${RUNNER_TEMP:-/tmp}/.ossutilconfig" ] && echo ok || echo MISSING)); posting a text-only report." >&2 EVIDENCE_SECTION='' return 0 fi - local raw_base="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${assets_branch}/${dest_dir}" + if ! PATH="$upload_path" "${upload_cmd[@]}" \ + --bucket "$ALIYUN_OSS_BUCKET" \ + --config "${RUNNER_TEMP:-/tmp}/.ossutilconfig" \ + --prefix "$prefix" \ + "$stage_dir"/*.png; then + if [ -n "$trusted_bin" ]; then rm -rf "$trusted_bin"; fi + # Report the local preconditions so a tooling/credential gap + # (the install/config steps are continue-on-error and render + # green when they fail) is distinguishable from a transient + # upload blip without trawling three steps of logs. + echo "::warning::Failed to upload evidence images (ossutil=$([ -x "${RUNNER_TEMP:-/tmp}/ossutil" ] && echo ok || echo MISSING), config=$([ -f "${RUNNER_TEMP:-/tmp}/.ossutilconfig" ] && echo ok || echo MISSING)); posting a text-only report." >&2 + EVIDENCE_SECTION='' + return 0 + fi + if [ -n "$trusted_bin" ]; then rm -rf "$trusted_bin"; fi + local raw_base="${ALIYUN_OSS_PUBLIC_BASE_URL%/}/${prefix}" EVIDENCE_SECTION=$'### Evidence images\n\n' - for f in "$clone_dir/$dest_dir"/*.png; do + for f in "$stage_dir"/*.png; do [ -f "$f" ] || continue safe="$(basename "$f")" EVIDENCE_SECTION+="![${safe%.png}](${raw_base}/${safe})"$'\n\n' @@ -5894,3 +5984,7 @@ jobs: gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments" -F body=@"$BODY_FILE" >/dev/null fi echo "Posted verification result to PR #${PR_NUMBER} (verdict=${VERDICT}, agent=${AGENT_VERDICT:-none})." >> "$GITHUB_STEP_SUMMARY" + + - name: 'Cleanup Aliyun OSS credentials' + if: '${{ always() }}' + run: 'rm -f "$RUNNER_TEMP/.ossutilconfig"' diff --git a/.github/workflows/web-shell-visuals-cleanup.yml b/.github/workflows/web-shell-visuals-cleanup.yml index b3f83a8305f..8fe5a57ecc8 100644 --- a/.github/workflows/web-shell-visuals-cleanup.yml +++ b/.github/workflows/web-shell-visuals-cleanup.yml @@ -1,13 +1,10 @@ name: 'PR Asset Branch Cleanup' -# 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. -# -# All producers are covered, and every new `pr-assets/*` producer must be -# added here: a branch nothing deletes is permanent. +# When a PR closes, delete legacy per-PR asset branches left by the former Git +# publishers. New previews and verification reports use OSS, and automated +# reviews cannot target this repository for assets, but historical refs still +# need to drain. Runs in the base context (pull_request_target) and only deletes +# refs by name; it never checks out or runs PR code. on: pull_request_target: types: diff --git a/.github/workflows/web-shell-visuals-publish.yml b/.github/workflows/web-shell-visuals-publish.yml index a50ca832bd5..f6125990105 100644 --- a/.github/workflows/web-shell-visuals-publish.yml +++ b/.github/workflows/web-shell-visuals-publish.yml @@ -3,7 +3,7 @@ name: 'Web-shell Visuals Publish' # Privileged companion to `web-shell-visuals.yml`. It runs AFTER that workflow # via workflow_run, so it executes in the base-repo context with a write token # but NEVER checks out or runs PR code — it only downloads the image artifact -# (opaque bytes), hosts it on the `pr-assets` branch, and posts an inline +# (opaque bytes), hosts it on Aliyun OSS, and posts an inline # comment. This is the GitHub-recommended split for commenting on fork PRs with # the results of untrusted-code execution. on: @@ -13,15 +13,13 @@ on: types: - 'completed' -# GITHUB_TOKEN only needs to read the triggering run's artifact; the branch push -# and PR comment are done with CI_BOT_PAT, which carries its own scope. +# GITHUB_TOKEN only needs to read the triggering run's artifact. The PR comment +# is posted with CI_BOT_PAT; OSS uses its own repository secrets. permissions: actions: 'read' -# Serialize publishes for the SAME PR (identified by its source repo + branch), -# since they force-push to that PR's own `pr-assets/web-shell-visuals-` -# branch; serializing means the force-push never has to reconcile a concurrent -# same-PR snapshot (a bounded retry below covers transient failures). Different +# Serialize publishes for the SAME PR (identified by its source repo + branch) +# so an older run cannot replace a newer run's marker comment. Different # PRs — including forks that happen to share a branch name like `main` — get # distinct groups and publish in parallel. Never cancel an in-flight publish. concurrency: @@ -43,15 +41,74 @@ jobs: runs-on: 'ubuntu-latest' timeout-minutes: 10 steps: - # Trusted base-repo script (staging + comment builder). workflow_run - # checks out the default branch, never PR code. Sparse — just the script. - - name: 'Checkout the publish script' + # Trusted base-repo scripts. workflow_run checks out the default branch, + # never PR code (the PR head is github.event.workflow_run.head_sha, + # consumed below as RUN_HEAD_SHA — never as a checkout ref). Sparse — + # the publisher, its helper dependency, and the root package.json that + # marks the uploader's `.js` as ESM (without it the uploader only + # parses on the Node versions that infer module syntax, and this job + # pins no Node). + - name: 'Checkout the publish scripts' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: - sparse-checkout: '.github/scripts/web-shell-visuals-publish.mjs' + # Pin the trusted ref explicitly: on workflow_run events + # github.sha is the default-branch head this workflow runs from, + # so the pin makes "never PR code" a property of this step + # instead of an inference from the event's default resolution. + ref: '${{ github.sha }}' + sparse-checkout: |- + /package.json + .github/scripts/web-shell-visuals-publish.mjs + scripts/upload-aliyun-oss-assets.js + scripts/release-script-utils.js sparse-checkout-cone-mode: false persist-credentials: false + - name: 'Install ossutil' + id: 'install-ossutil' + # Evidence hosting is best-effort: a setup failure must not block the + # no-image marker-comment path, which has no external dependency + # (mirrors the qwen-triage.yml twins). The upload below stays loud: + # with images present it still aborts the publish step. + continue-on-error: true + env: + OSSUTIL_URL: "${{ vars.OSSUTIL_URL || 'https://gosspublic.alicdn.com/ossutil/1.7.19/ossutil-v1.7.19-linux-amd64.zip' }}" + OSSUTIL_SHA256: "${{ vars.OSSUTIL_SHA256 || 'dcc512e4a893e16bbee63bc769339d8e56b21744fd83c8212a9d8baf28767343' }}" + run: |- + set -euo pipefail + tmp_dir="$(mktemp -d)" + trap 'rm -rf "$tmp_dir"' EXIT + # Retry, unlike the release syncs: a red run there is simply rerun, + # but a CDN blip here costs this PR its preview comment (the + # upload aborts the step) or its evidence images. The worst-case + # budget stays inside the 10-minute job cap: 3 attempts x 120s + # max-time + 2 x 2s delays ≈ 6.1 minutes (an unreachable mirror + # fails the connect-timeout long before that). + curl -fsSL --retry 2 --retry-delay 2 --retry-all-errors \ + --connect-timeout 10 --max-time 120 "$OSSUTIL_URL" -o "$tmp_dir/ossutil.zip" + echo "$OSSUTIL_SHA256 $tmp_dir/ossutil.zip" | sha256sum -c - + unzip -q "$tmp_dir/ossutil.zip" -d "$tmp_dir" + ossutil_path="$(find "$tmp_dir" -type f \( -name 'ossutil' -o -name 'ossutil64' \) -print -quit)" + [ -n "$ossutil_path" ] || { echo '::error::ossutil binary not found'; exit 1; } + chmod +x "$ossutil_path" + install -m 0755 "$ossutil_path" "$RUNNER_TEMP/ossutil" + "$RUNNER_TEMP/ossutil" >/dev/null + + - name: 'Configure Aliyun OSS credentials' + if: "${{ steps.install-ossutil.outcome == 'success' }}" + continue-on-error: true + env: + ALIYUN_OSS_ACCESS_KEY_ID: '${{ secrets.ALIYUN_OSS_ACCESS_KEY_ID }}' + ALIYUN_OSS_ACCESS_KEY_SECRET: '${{ secrets.ALIYUN_OSS_ACCESS_KEY_SECRET }}' + ALIYUN_OSS_ENDPOINT: "${{ vars.ALIYUN_OSS_ENDPOINT || 'https://oss-cn-hangzhou.aliyuncs.com' }}" + run: |- + set -euo pipefail + if [ -z "$ALIYUN_OSS_ACCESS_KEY_ID" ] || [ -z "$ALIYUN_OSS_ACCESS_KEY_SECRET" ]; then + echo '::error::Missing Aliyun OSS credentials.' + exit 1 + fi + "$RUNNER_TEMP/ossutil" config -e "$ALIYUN_OSS_ENDPOINT" -i "$ALIYUN_OSS_ACCESS_KEY_ID" -k "$ALIYUN_OSS_ACCESS_KEY_SECRET" -L EN -c "$RUNNER_TEMP/.ossutilconfig" + - name: 'Download visuals artifact' id: 'download' continue-on-error: true @@ -66,6 +123,10 @@ jobs: env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' RUN_ID: '${{ github.event.workflow_run.id }}' + # The run id is stable across re-run attempts — only the attempt + # number increments — so it alone cannot keep a re-run off the + # previous attempt's object keys in the OSS prefix below. + RUN_ATTEMPT: '${{ github.event.workflow_run.run_attempt }}' RUN_URL: '${{ github.event.workflow_run.html_url }}' # Authenticated (NOT artifact-sourced) head SHA of the run that # triggered this publish — used to bind the artifact to its real PR. @@ -75,6 +136,23 @@ jobs: # shares the same head commit SHA. RUN_HEAD_REPO: '${{ github.event.workflow_run.head_repository.full_name }}' RUN_HEAD_BRANCH: '${{ github.event.workflow_run.head_branch }}' + # PR evidence is untrusted, PR-derived content, so it gets its own + # bucket knob: set ALIYUN_OSS_PR_ASSETS_BUCKET to move it off the + # bucket that also serves release, desktop, and live-host downloads. + # Unset, it falls back to the shared bucket — today's behaviour. + # The default public URL is derived from the resolved bucket so that + # overriding only one of the two vars cannot post comment links + # that 404 against (or show stale objects from) the other bucket. + # A cross-region ALIYUN_OSS_ENDPOINT override must also override + # ALIYUN_OSS_PUBLIC_BASE_URL because the default host pins Hangzhou. + ALIYUN_OSS_BUCKET: "${{ vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || 'qwen-code-assets' }}" + ALIYUN_OSS_PUBLIC_BASE_URL: "${{ vars.ALIYUN_OSS_PR_ASSETS_PUBLIC_BASE_URL || (vars.ALIYUN_OSS_PR_ASSETS_BUCKET == '' && vars.ALIYUN_OSS_PUBLIC_BASE_URL) || format('https://{0}.oss-cn-hangzhou.aliyuncs.com', vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || 'qwen-code-assets') }}" + # A stalled upload must not burn this job's 10-minute cap: bound + # every ossutil attempt so the uploader exhausts its retries (and + # aborts this publish, which stays re-triggerable) instead of + # hanging one black-hole socket until the job dies mid-upload. + # Worst case is one image's 3 attempts x 120s + backoffs ≈ 6.1 min. + OSS_UPLOAD_ATTEMPT_TIMEOUT_MS: '120000' run: |- set -euo pipefail @@ -174,58 +252,46 @@ jobs: HAS_IMAGES=1 fi - # Re-validate right before the force-push too: a close/new-head during - # download+staging must not force-push a stale snapshot (which would - # orphan the commit the existing comment still points at). + # Re-validate right before the upload too: a close/new-head during + # download+staging must not publish a stale snapshot. gate - # --- Host on the PR's own pr-assets branch (bounded) -------------- - # Replace the branch with a SINGLE orphan snapshot each run and - # force-push, so untrusted PR content can't accumulate unbounded - # history in the base repo. The comment always points at the new - # immutable commit SHA; the previous snapshot becomes unreachable and - # is GC'd. Per-PR branch under the existing pr-assets/ convention - # (a bare `pr-assets` branch would D/F-conflict with pr-assets/*), and - # the per-PR concurrency group serializes same-PR runs so the - # force-push never drops a concurrent snapshot. + # --- Host on Aliyun OSS ------------------------------------------- + # Head SHA, run id *and* run attempt, so every publish writes a + # prefix nothing has ever served before. The run id is not + # decoration: GitHub renders comment images through its camo proxy, + # which caches by URL, so re-running a preview for the SAME head + # onto the same object keys would leave reviewers looking at the + # previous run's screenshots. The run id alone is not enough either: + # it is stable across re-run attempts (only run_attempt increments), + # so the attempt segment keeps a maintainer re-run off the exact + # keys the previous attempt's comment already references. The old + # design got this free from the per-run commit SHA in the raw URL; + # the verify lane keeps it via -. # Host the images ONLY when there are changed views. A no-change run - # skips the asset commit entirely; RAW_BASE stays empty and the + # skips the upload entirely; RAW_BASE stays empty and the # comment below is image-less ("no screenshot changes"). RAW_BASE='' if [ "${HAS_IMAGES}" = "1" ]; then - BRANCH="pr-assets/web-shell-visuals-${PR}" - AUTH_URL="https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" - WORK="${RUNNER_TEMP}/pr-assets" - rm -rf "${WORK}" - mkdir -p "${WORK}/imgs" - cp "${STAGE}"/* "${WORK}/imgs/" - cd "${WORK}" - git init -q - git config user.name 'qwen-code-bot' - git config user.email 'qwen-code-bot@users.noreply.github.com' - git checkout -q --orphan snapshot - git add imgs - git commit -q -m "web-shell visuals: PR #${PR} (run ${RUN_ID})" - # Bounded retry for transient push failures (network / brief lock). - # Same-PR runs are serialized by concurrency, so this never needs to - # reconcile a concurrent snapshot — a plain retry suffices. - pushed=0 - for attempt in 1 2 3; do - if git push -q --force "${AUTH_URL}" "HEAD:${BRANCH}"; then - pushed=1 - break - fi - echo "::notice::force-push attempt ${attempt} failed; retrying." - sleep 2 - done - if [ "${pushed}" -ne 1 ]; then - echo "::error::Failed to push web-shell visuals to ${BRANCH} after retries." - exit 1 - fi - ASSET_SHA="$(git rev-parse HEAD)" - cd "${GITHUB_WORKSPACE}" - echo "web-shell visuals hosted on ${BRANCH} at ${ASSET_SHA}." - RAW_BASE="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${ASSET_SHA}/imgs" + ASSET_PREFIX="pr-assets/web-shell-visuals/${PR}/${RUN_HEAD_SHA}/${RUN_ID}/${RUN_ATTEMPT}" + # Same PATH discipline as qwen-triage.yml's publish-verify: never + # resolve node or ossutil through the shared $RUNNER_TEMP. node + # is resolved under the inherited PATH and ossutil runs from a + # fresh job-private copy of the sha256-verified binary installed + # above. Not exploitable on today's ephemeral runners, but the + # shape must not regress if this job ever moves to a persistent + # pool. + node_bin="$(command -v node)" + trusted_bin="$(mktemp -d)" + install -m 0755 "${RUNNER_TEMP}/ossutil" "$trusted_bin/ossutil" + PATH="$trusted_bin" "$node_bin" scripts/upload-aliyun-oss-assets.js \ + --bucket "${ALIYUN_OSS_BUCKET}" \ + --config "${RUNNER_TEMP}/.ossutilconfig" \ + --prefix "${ASSET_PREFIX}" \ + "${STAGE}"/* + rm -rf "$trusted_bin" + RAW_BASE="${ALIYUN_OSS_PUBLIC_BASE_URL%/}/${ASSET_PREFIX}" + echo "Web-shell visuals hosted at ${RAW_BASE}." fi # --- Changed paths, for triaging an EMPTY preview ------------------ @@ -295,3 +361,7 @@ jobs: gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/comments" -F body=@"${BODY_FILE}" >/dev/null echo "Posted web-shell visuals comment on PR #${PR}." fi + + - name: 'Cleanup Aliyun OSS credentials' + if: '${{ always() }}' + run: 'rm -f "$RUNNER_TEMP/.ossutilconfig"' diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 7cc72352a45..8438825776d 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -828,9 +828,8 @@ workflow globs). It must contain: fails, say so under _Not covered_ in one line and ship the text-only report; do not reconstruct the pipeline by hand. - 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 + The publish job hosts what you produce on Aliyun OSS 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/docs/design/2026-08-25-pr-evidence-oss-hosting.md b/docs/design/2026-08-25-pr-evidence-oss-hosting.md new file mode 100644 index 00000000000..023f0a3c3c7 --- /dev/null +++ b/docs/design/2026-08-25-pr-evidence-oss-hosting.md @@ -0,0 +1,99 @@ +# PR Evidence Hosting on Aliyun OSS + +## Problem + +Automated PR verification currently publishes screenshots by committing them to +`pr-assets/*` branches in the main repository. A normal clone fetches objects +reachable from every advertised branch, so long-lived PNG-only branches add +hundreds of megabytes to clone traffic and repository storage even though the +images are unrelated to the product source tree. + +## Goals + +- Keep inline screenshots in Web Shell visual previews and `/verify` reports. +- Stop automated workflows from creating or updating image branches in this + repository. +- Reuse the existing, tested Aliyun OSS uploader and repository credentials. +- Preserve the current validation, size limits, retries, and fail-safe behavior. +- Keep PR-derived bytes and OSS credentials separated by a trusted publisher + job. + +## Design + +Both trusted publisher jobs configure the existing `ossutil` client and invoke +`scripts/upload-aliyun-oss-assets.js`. The uploader keeps its existing +three-attempt retry policy and publishes objects with `public-read` ACL. + +Web Shell previews use this immutable prefix: + +```text +pr-assets/web-shell-visuals///// +``` + +An object key is never written twice. The head SHA binds a URL to the code it +depicts; the run id keeps a re-run of that same head from writing back over the +objects an already-posted comment references. The run id alone would not be +enough: it is stable across re-run attempts (only the attempt number +increments), so the run attempt segment keeps a maintainer re-run of the same +workflow run off the previous attempt's keys too. That matters because GitHub +serves comment images through a caching image proxy: reusing a key would leave +reviewers looking at the previous run's screenshots at a URL whose bytes had +changed. The Git-backed design got this property free from the per-run commit +SHA in the raw URL. + +Sandboxed `/verify` reports use this immutable prefix: + +```text +pr-assets/verify/pr--/ +``` + +The workflow continues to accept at most eight PNGs of at most 2 MiB each, +checks PNG magic bytes, sanitizes names, and degrades to a text-only report if +hosting fails. + +The publisher jobs run in the base-repository context and never expose OSS +credentials to the jobs that execute PR code. They consume only bounded, +validated image artifacts. + +The automated review workflow has a third, separate image path through +`qwen review publish-assets`. That CLI feature remains available for an +explicitly designated assets repository, but the workflow now rejects a +designation that points back to the repository under review. An unset or +self-targeting destination degrades to prose and downloadable run artifacts; +an external image-host repository remains supported. + +## Bucket selection + +Both publishers resolve the destination as +`ALIYUN_OSS_PR_ASSETS_BUCKET` → `ALIYUN_OSS_BUCKET` → `qwen-code-assets`, with +the public base URL derived from whichever bucket wins (or set explicitly via +`ALIYUN_OSS_PR_ASSETS_PUBLIC_BASE_URL`). Unset, this is exactly today's shared +bucket. The knob exists because PR evidence is untrusted, PR-derived content +while the shared bucket also serves release, desktop, and live-host downloads; +setting one variable separates them without a workflow change. + +It does not narrow the credential: these jobs hold the same OSS key the release +sync uses. Scoping a dedicated RAM user to the `pr-assets/` prefix is ops work +outside this PR, and is the reason the destination is a variable rather than a +constant. + +## Compatibility and cleanup + +Existing PR comments keep their Git-backed URLs. The PR-close cleanup workflow +continues deleting historical `pr-assets/*` refs, but no new refs are produced +by the migrated workflows. + +OSS object retention is intentionally external to the workflow. The +`pr-assets/` prefix can use a bucket lifecycle rule without changing repository +history or clone behavior. + +## Verification + +- Execute the workflow-level `/verify` image-hosting harness against a fake OSS + uploader, including valid images, rejected images, duplicate names, the exact + size boundary, first publication, and upload failure. +- Assert both publisher workflows call the shared uploader and contain no Git + image push path. +- Assert the automated review workflow cannot target this repository for image + branches. +- Run the shared uploader unit tests and workflow parser tests. diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index c28a6859334..33de06032d0 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -261,9 +261,9 @@ export QWEN_REVIEW_ASSETS_REPO=your-org/your-repo # a repo you can push to /review 123 --comment ``` -Maintainers typically point it at the repo under review; anyone else can use a fork or a scratch repo. Images land on the `pr-assets/-review` branch with content-hashed names, and comments reference them by **commit-pinned** URL — immutable even if the branch later moves, and working unchanged on GitHub Enterprise. +Point it at a repository you can push to — a dedicated image-host repository is the recommended destination; a fork or scratch repo works too. Avoid the repository under review itself: image branches pushed there become reachable objects that every clone fetches. Images land on the `pr-assets/-review` branch with content-hashed names, and comments reference them by **commit-pinned** URL — immutable even if the branch later moves, and working unchanged on GitHub Enterprise. -For GitHub-triggered reviews (the PR-review workflow), the same variable is wired from a **repository variable** of the same name: with the variable unset the workflow passes an empty value and publishing refuses — nothing changes. A maintainer who sets `QWEN_REVIEW_ASSETS_REPO` in the repository's Actions variables (typically to the repository itself) enables review comments to embed capture PNGs; the branches it writes are cleaned up by the visuals cleanup workflow when the variable points at the same repository, while a fork or scratch destination manages its own retention. +For GitHub-triggered reviews (the PR-review workflow), the same variable is wired from a **repository variable** of the same name, and the workflow passes through only a dedicated external host: with the variable unset, or set to the repository under review (compared after trimming and case-normalization, so padded or case-shifted spellings of the same repository count as self-targeting), the workflow passes an empty value and publishing refuses — nothing changes, and the review keeps its evidence as prose and local artifact paths. A maintainer who sets `QWEN_REVIEW_ASSETS_REPO` in the repository's Actions variables to a separate image-host repository enables review comments to embed capture PNGs. An external destination manages its own retention; the visuals cleanup workflow only drains the historical `pr-assets/*` branches that former Git-backed publishers left in this repository. The publishing is gated exactly like posting: no designated repo means no publish, and an unauthorized run (no effective `--comment`) is refused the same way `submit` refuses. Only image types are accepted (SVG is excluded deliberately), with size caps, and each file's bytes must match the format its extension claims — mislabeled or unrecognized content is refused. A manifest records every file pushed. Without a designation, findings keep their evidence as local file paths in the terminal and saved report — nothing breaks, comments just stay text-only. diff --git a/packages/cli/src/commands/review/lib/assets.ts b/packages/cli/src/commands/review/lib/assets.ts index 21e7d3724c3..5bd623dfb96 100644 --- a/packages/cli/src/commands/review/lib/assets.ts +++ b/packages/cli/src/commands/review/lib/assets.ts @@ -222,8 +222,12 @@ export function parseAssetsRepo( return { error: 'QWEN_REVIEW_ASSETS_REPO is not set. Designate an assets repository ' + - '(owner/repo you can push to — the repo under review for maintainers, ' + - 'a fork or scratch repo otherwise) to publish review evidence images.', + '(owner/repo you can push to — a dedicated image-host repository, or ' + + 'a fork or scratch repo rather than the repository under review, ' + + 'whose clones would fetch the image branches) to publish review ' + + 'evidence images. In CI, note that the PR-review workflow blanks a ' + + 'designation pointing at the repository under review, so a set ' + + 'repository variable can still arrive here as unset.', }; } const parts = v.split('/'); diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index 8108a264ff7..9942760bcc1 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -182,7 +182,7 @@ Rationale: an inline comment is the only place GitHub renders a ` ```suggestion ### Evidence images (`publish-assets`) — only for an authorised, posting run -**When a finding's evidence is an image** — a TUI screenshot, a rendered-output comparison, a browser capture produced during verification — a comment that embeds it is worth more than one that describes it. GitHub's API cannot attach images to review comments (the web UI's drag-and-drop upload has no API equivalent), so image evidence is hosted in a **user-designated assets repository** and referenced by URL. The designation is the `QWEN_REVIEW_ASSETS_REPO` environment variable (`owner/repo` the user can push to — the repo under review for maintainers, a fork or scratch repo otherwise). It is deliberately a **different** variable from `QWEN_REVIEW_SCRATCH_REPO`: the scratch repo's contract forbids PR-derived content, and an evidence screenshot is exactly that. +**When a finding's evidence is an image** — a TUI screenshot, a rendered-output comparison, a browser capture produced during verification — a comment that embeds it is worth more than one that describes it. GitHub's API cannot attach images to review comments (the web UI's drag-and-drop upload has no API equivalent), so image evidence is hosted in a **user-designated assets repository** and referenced by URL. The designation is the `QWEN_REVIEW_ASSETS_REPO` environment variable (`owner/repo` the user can push to — a dedicated image-host repository, or a fork or scratch repo rather than the repository under review, whose clones would fetch the image branches; the PR-review workflow refuses a designation that points back at the repository under review). It is deliberately a **different** variable from `QWEN_REVIEW_SCRATCH_REPO`: the scratch repo's contract forbids PR-derived content, and an evidence screenshot is exactly that. Findings carry their evidence as local paths in the artifact's `assetFiles` field (Step 6's `qwen review findings` accepts it per finding). Publish them in one call, which weaves the resulting URLs back into the artifact as `assets`: diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index ce90b3227e4..59e2dc622d1 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1395,15 +1395,64 @@ describe('capture-tools step wiring', () => { ); }); - it('passes the assets-repo variable into the review step', () => { - // The CLI reads QWEN_REVIEW_ASSETS_REPO from the environment; the run: - // script never names it, so only this assertion sees a dropped or - // misspelled wiring line. + it('keeps review evidence branches out of the project repository', () => { + // The CLI reads QWEN_REVIEW_ASSETS_REPO from the environment. The workflow + // passes through only a dedicated external host, never this project repo. const doc = parse(workflow); expect( doc.jobs['review-pr'].steps.find((s) => s.name === 'Run review').env .QWEN_REVIEW_ASSETS_REPO, - ).toBe('${{ vars.QWEN_REVIEW_ASSETS_REPO }}'); + ).toBe( + "${{ vars.QWEN_REVIEW_ASSETS_REPO != github.repository && vars.QWEN_REVIEW_ASSETS_REPO || '' }}", + ); + }); + + it('normalizes whitespace and case variants of the assets-repo designation', () => { + // The env-level guard compares the RAW variable, so padded or + // case-shifted self-references slip past it; the run body trims and + // re-checks before the CLI reads the value. Executed, not asserted as + // text: a dropped trim or a case-sensitive compare re-enables a + // self-targeting designation while every shape assertion stays green. + const run = runReviewStep(); + const marker = '# Normalize the assets-repo designation'; + const start = run.indexOf(marker); + expect(start).toBeGreaterThan(-1); + const endMarker = 'export QWEN_REVIEW_ASSETS_REPO'; + const end = run.indexOf(endMarker, start); + expect(end).toBeGreaterThan(-1); + const fragment = run.slice(start, end + endMarker.length); + + function normalize(value) { + return execFileSync( + 'bash', + [ + '-c', + [ + 'set -euo pipefail', + 'REPO="QwenLM/qwen-code"', + 'QWEN_REVIEW_ASSETS_REPO="$DESIGNATION"', + fragment, + 'printf "%s" "$QWEN_REVIEW_ASSETS_REPO"', + ].join('\n'), + ], + { + encoding: 'utf8', + env: { ...process.env, DESIGNATION: value }, + }, + ); + } + + // Self-targeting in every disguise degrades to the empty designation. + expect(normalize('QwenLM/qwen-code')).toBe(''); + expect(normalize(' QwenLM/qwen-code ')).toBe(''); + expect(normalize('qwenlm/QWEN-CODE')).toBe(''); + expect(normalize('\tQwenLM/qwen-code\n')).toBe(''); + // An external host survives, trimmed. + expect(normalize('other-org/assets')).toBe('other-org/assets'); + expect(normalize(' other-org/assets ')).toBe('other-org/assets'); + // Unset-ish values stay empty. + expect(normalize('')).toBe(''); + expect(normalize(' ')).toBe(''); }); }); diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 09e32d17d82..77e6978f42f 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -3910,7 +3910,7 @@ describe('qwen-triage verify hardening round 2', () => { }); // The evidence-hosting path carries the untrusted-image checks; exercise - // it end to end against a bare local remote. + // it end to end against a local OSS uploader stub. it('hosts only valid, unique, in-limit PNGs and degrades to text', () => { const publishStep = step('Post verification report comment'); const script = publishStep @@ -3920,10 +3920,43 @@ describe('qwen-triage verify hardening round 2', () => { const sh = (cmd, opts = {}) => spawnSync('bash', ['-c', cmd], { encoding: 'utf8', ...opts }); try { - // 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/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`, + // The uploader stub preserves the object layout while avoiding network + // access, and enforces the real uploader's flag contract: the workflow + // must pass --bucket, --config and --prefix with values, or the + // harness fails exactly where production would silently degrade every + // report to text-only. The gh stub captures the rendered report body. + writeFileSync( + join(dir, 'upload-assets'), + [ + '#!/usr/bin/env bash', + 'set -euo pipefail', + 'if [ "${OSS_STUB_FAIL:-}" = 1 ]; then exit 1; fi', + 'bucket=""; config=""; prefix=""', + 'while [ "$#" -gt 0 ]; do', + ' case "$1" in', + ' --bucket) bucket="$2"; shift 2 ;;', + ' --config) config="$2"; shift 2 ;;', + ' --prefix) prefix="$2"; shift 2 ;;', + ' *) break ;;', + ' esac', + 'done', + 'if [ -z "$bucket" ] || [ -z "$config" ] || [ -z "$prefix" ]; then', + ' echo "upload-assets stub: --bucket, --config and --prefix are all required" >&2', + ' exit 1', + 'fi', + // The real uploader hands --config to ossutil, which dies on a + // missing file: checking it here means a workflow mutation that + // drifts the flag away from the credential file the configure + // step writes turns the harness red instead of staying green. + 'if [ ! -f "$config" ]; then', + ' echo "upload-assets stub: --config file does not exist: $config" >&2', + ' exit 1', + 'fi', + 'target="$OSS_STUB_ROOT/$bucket/$prefix"', + 'mkdir -p "$target"', + 'for file in "$@"; do cp "$file" "$target/$(basename "$file")"; done', + ].join('\n'), + { mode: 0o755 }, ); writeFileSync( join(dir, 'gh'), @@ -3960,6 +3993,12 @@ describe('qwen-triage verify hardening round 2', () => { png(join(art, 'evidence', '04-edge.png'), 2 * 1024 * 1024 - 9); writeFileSync(join(art, 'report.md'), '## r\n'); + // The stub arm receives --config "${RUNNER_TEMP}/.ossutilconfig" and + // (like the real uploader's ossutil) refuses to run without that + // file, so seed the configure step's product for the stub runs. The + // production arms below manage the file explicitly instead. + writeFileSync(join(dir, '.ossutilconfig'), '[Credentials]\n'); + const out = join(dir, 'comment.md'); const res = sh(script, { cwd: work, @@ -3980,29 +4019,27 @@ describe('qwen-triage verify hardening round 2', () => { AGENT_VERDICT: 'findings', SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', - VERIFY_ASSETS_REMOTE: `${dir}/assets.git`, + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + VERIFY_ASSETS_UPLOADER: join(dir, 'upload-assets'), + OSS_STUB_ROOT: join(dir, 'oss'), }, }); expect(res.status).toBe(0); - const hosted = sh( - `git -C "${dir}/assets.git" ls-tree -r --name-only pr-assets/7999-verify | grep verify/ || true`, - ) - .stdout.trim() - .split('\n') - .filter(Boolean); + const hosted = readdirSync( + join(dir, 'oss', 'assets-bucket', 'pr-assets', 'verify', 'pr7999-77-1'), + ); // Valid + at the exact 2 MiB boundary are hosted; the text file, the // oversize file and the duplicate name are not. - expect(hosted.map((p) => p.split('/').pop()).sort()).toEqual([ - '01-ab.png', - '04-edge.png', - ]); + expect(hosted.sort()).toEqual(['01-ab.png', '04-edge.png']); const comment = readFileSync(out, 'utf8'); - expect(comment).toContain('![01-ab]('); + expect(comment).toContain( + '![01-ab](https://assets.example.test/pr-assets/verify/pr7999-77-1/01-ab.png)', + ); expect(comment).not.toContain('02-fake'); expect(comment).toContain('did not pass the hosting checks'); - // Unreachable remote -> text-only, never an aborted report. - sh(`rm -rf "${dir}/empty.git" && mkdir -p "${dir}/empty.git"`); + // Upload failure -> text-only, never an aborted report. const out2 = join(dir, 'comment2.md'); const res2 = sh(script, { cwd: work, @@ -4023,7 +4060,11 @@ describe('qwen-triage verify hardening round 2', () => { AGENT_VERDICT: 'findings', SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', - VERIFY_ASSETS_REMOTE: `${dir}/empty.git`, + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + VERIFY_ASSETS_UPLOADER: join(dir, 'upload-assets'), + OSS_STUB_ROOT: join(dir, 'oss'), + OSS_STUB_FAIL: '1', }, }); expect(res2.status).toBe(0); @@ -4031,19 +4072,8 @@ describe('qwen-triage verify hardening round 2', () => { 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(''); + // A later run gets an immutable prefix instead of overwriting or + // accumulating commits in the same Git ref. const out3 = join(dir, 'comment3.md'); const res3 = sh(script, { cwd: work, @@ -4064,33 +4094,311 @@ describe('qwen-triage verify hardening round 2', () => { AGENT_VERDICT: 'findings', SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', - VERIFY_ASSETS_REMOTE: `${dir}/assets.git`, + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + VERIFY_ASSETS_UPLOADER: join(dir, 'upload-assets'), + OSS_STUB_ROOT: join(dir, 'oss'), }, }); 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]('); + readdirSync( + join( + dir, + 'oss', + 'assets-bucket', + 'pr-assets', + 'verify', + 'pr7999-79-1', + ), + ).sort(), + ).toEqual(['01-ab.png', '04-edge.png']); + expect(readFileSync(out3, 'utf8')).toContain( + 'https://assets.example.test/pr-assets/verify/pr7999-79-1/01-ab.png', + ); + + // The production arms below run WITHOUT the seeded credential file + // until the success arm writes it back — the missing-file case is + // exactly what res4b asserts on. + rmSync(join(dir, '.ossutilconfig')); + + // An ossutil install failure is expected to degrade to a text-only + // report. This executes the production dispatch arm without touching + // the network and pins its successful return under set -e. + const out4 = join(dir, 'comment4.md'); + const res4 = sh(script, { + cwd: work, + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_STUB_OUT: out4, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + RUNNER_TEMP: dir, + GITHUB_STEP_SUMMARY: '/dev/null', + GITHUB_RUN_ID: '80', + GITHUB_RUN_ATTEMPT: '1', + PR_NUMBER: '7999', + RUN_URL: 'u', + VERIFY_RESULT: 'success', + VERDICT: 'pass', + AGENT_VERDICT: 'findings', + SKIP_REASON: '', + PREPARE_FAILURE_PHASE: '', + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + OSSUTIL_INSTALL_OUTCOME: 'failure', + OSS_STUB_ROOT: join(dir, 'oss'), + }, + }); + expect(res4.status).toBe(0); + const comment4 = readFileSync(out4, 'utf8'); + expect(comment4).toContain('Sandboxed verification'); + expect(comment4).not.toContain('Evidence images'); + + // Install green but NO credential file: the configure step is + // continue-on-error, so this is what a missing/rotated OSS secret + // looks like from here. The publisher must degrade to text rather + // than hand the uploader a config it cannot read and pay three + // retry backoffs per image to learn that. + const out4b = join(dir, 'comment4b.md'); + const res4b = sh(script, { + cwd: work, + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_STUB_OUT: out4b, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + RUNNER_TEMP: dir, + GITHUB_STEP_SUMMARY: '/dev/null', + GITHUB_RUN_ID: '80', + GITHUB_RUN_ATTEMPT: '2', + PR_NUMBER: '7999', + RUN_URL: 'u', + VERIFY_RESULT: 'success', + VERDICT: 'pass', + AGENT_VERDICT: 'findings', + SKIP_REASON: '', + PREPARE_FAILURE_PHASE: '', + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + OSSUTIL_INSTALL_OUTCOME: 'success', + OSS_STUB_ROOT: join(dir, 'oss'), + }, + }); + expect(res4b.status).toBe(0); + expect(existsSync(join(dir, '.ossutilconfig'))).toBe(false); + const comment4b = readFileSync(out4b, 'utf8'); + expect(comment4b).toContain('Sandboxed verification'); + expect(comment4b).not.toContain('Evidence images'); + + // Exercise the production success dispatch too: copy the verified + // binary into a job-private PATH and run the real uploader through it. + const realUploader = readFileSync( + 'scripts/upload-aliyun-oss-assets.js', + 'utf8', + ); + const realUtils = readFileSync('scripts/release-script-utils.js', 'utf8'); + mkdirSync(join(work, 'scripts'), { recursive: true }); + writeFileSync( + join(work, 'scripts', 'upload-aliyun-oss-assets.js'), + realUploader, + ); + writeFileSync( + join(work, 'scripts', 'release-script-utils.js'), + realUtils, + ); + writeFileSync( + join(dir, 'ossutil'), + [ + '#!/bin/bash', + 'set -euo pipefail', + '[ "$1" = cp ]', + 'src="$2"; dest="$3"', + 'target="$OSS_STUB_ROOT/${dest#oss://}"', + '/bin/mkdir -p "$(/usr/bin/dirname "$target")"', + '/bin/cp "$src" "$target"', + ].join('\n'), + { mode: 0o755 }, + ); + // The configure step's product. The stub ossutil ignores -c, but the + // publisher now refuses to dispatch without it — as production does. + writeFileSync(join(dir, '.ossutilconfig'), '[Credentials]\n'); + const out5 = join(dir, 'comment5.md'); + const res5 = sh(script, { + cwd: work, + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_STUB_OUT: out5, + GH_TOKEN: 'x', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + RUNNER_TEMP: dir, + GITHUB_STEP_SUMMARY: '/dev/null', + GITHUB_RUN_ID: '81', + GITHUB_RUN_ATTEMPT: '1', + PR_NUMBER: '7999', + RUN_URL: 'u', + VERIFY_RESULT: 'success', + VERDICT: 'pass', + AGENT_VERDICT: 'findings', + SKIP_REASON: '', + PREPARE_FAILURE_PHASE: '', + ALIYUN_OSS_BUCKET: 'assets-bucket', + ALIYUN_OSS_PUBLIC_BASE_URL: 'https://assets.example.test', + OSSUTIL_INSTALL_OUTCOME: 'success', + OSS_STUB_ROOT: join(dir, 'oss-production'), + }, + }); + if (res5.status !== 0) { + throw new Error( + `production dispatch failed:\n${res5.stdout}\n${res5.stderr}`, + ); + } + expect( + readdirSync( + join( + dir, + 'oss-production', + 'assets-bucket', + 'pr-assets', + 'verify', + 'pr7999-81-1', + ), + ).sort(), + ).toEqual(['01-ab.png', '04-edge.png']); + expect(readFileSync(out5, 'utf8')).toContain( + 'https://assets.example.test/pr-assets/verify/pr7999-81-1/01-ab.png', + ); } finally { rmSync(dir, { recursive: true, force: true }); } }); + // The behavioural runs above either stub the uploader via + // VERIFY_ASSETS_UPLOADER or (res5) already exercise the production + // ossutil arm end-to-end through the real uploader. Pin the full + // invocation shape as well: node must resolve under the inherited PATH + // (never $RUNNER_TEMP — PATH hijack) and all three ossutil flags must be + // present — the real uploader exits 1 on a valueless --config, which in + // production silently degrades every /verify report to text-only. + it('pins the production uploader invocation and its ossutil flags', () => { + const script = + stepIn('publish-verify', 'Post verification report comment').match( + /run: \|-\n([\s\S]*)$/, + )?.[1] ?? ''; + expect(script).toContain('node_bin="$(command -v node || true)"'); + expect(script).toContain('upload_cmd=("$node_bin" "$uploader")'); + expect(script).toContain('upload_path="$trusted_bin"'); + expect(script).toContain('if ! PATH="$upload_path" "${upload_cmd[@]}"'); + expect(script).not.toContain('PATH="$RUNNER_TEMP:$PATH"'); + expect(script).toContain('--bucket "$ALIYUN_OSS_BUCKET"'); + expect(script).toContain('--config "${RUNNER_TEMP:-/tmp}/.ossutilconfig"'); + expect(script).toContain('--prefix "$prefix"'); + }); + + // The credential lifecycle is security-relevant: dropping the cleanup + // (or its always() condition) leaves the OSS key pair in $RUNNER_TEMP — + // not exploitable on today's ephemeral ubuntu-latest runner, but the + // shape must not regress if the job ever returns to a persistent pool. + // A dropped sha256 check installs whatever the mirror serves. Consumers + // must also trust the Actions-computed outcome, never a marker in the + // PR-writable temp dir. + it('pins the ossutil install/configure/cleanup credential lifecycle', () => { + const install = stepIn('publish-verify', 'Install ossutil'); + expect(install).toContain("id: 'install-ossutil'"); + expect(install).toContain('continue-on-error: true'); + expect(install).toContain('sha256sum -c'); + const configure = stepIn( + 'publish-verify', + 'Configure Aliyun OSS credentials', + ); + expect(configure).toContain( + 'if: "${{ steps.install-ossutil.outcome == \'success\' }}"', + ); + expect(configure).toContain('continue-on-error: true'); + expect(configure).toContain('-c "$RUNNER_TEMP/.ossutilconfig"'); + const publish = stepIn( + 'publish-verify', + 'Post verification report comment', + ); + expect(publish).toContain( + "OSSUTIL_INSTALL_OUTCOME: '${{ steps.install-ossutil.outcome }}'", + ); + // Both preconditions: the Actions-computed install outcome (never a + // marker in the PR-writable temp dir) AND the credential file the + // continue-on-error configure step is supposed to have written. + expect(publish).toContain( + 'elif [ "${OSSUTIL_INSTALL_OUTCOME:-}" = \'success\' ] &&', + ); + expect(publish).toContain( + '[ -f "${RUNNER_TEMP:-/tmp}/.ossutilconfig" ]; then', + ); + const cleanup = stepIn('publish-verify', 'Cleanup Aliyun OSS credentials'); + expect(cleanup).toContain("if: '${{ always() }}'"); + expect(cleanup).toContain('rm -f "$RUNNER_TEMP/.ossutilconfig"'); + }); + + // The publisher's checkout is the trust boundary for this job's OSS + // credentials: it must take only the trusted base-repo scripts, from the + // default-branch head (publish-verify only runs under issue_comment + // events, where github.sha is exactly that — never a PR merge ref). + // Dropping /package.json stops the uploader's `.js` parsing as ESM on + // Node versions that don't infer module syntax, and this job pins no Node. + it('checks out only trusted base-repo scripts at a pinned ref', () => { + const checkout = stepIn( + 'publish-verify', + 'Checkout the OSS publisher scripts', + ); + expect(checkout).toContain("ref: '${{ github.sha }}'"); + expect(checkout).toContain('persist-credentials: false'); + expect(checkout).toContain('sparse-checkout-cone-mode: false'); + for (const entry of [ + '/package.json', + 'scripts/upload-aliyun-oss-assets.js', + 'scripts/release-script-utils.js', + ]) { + expect(checkout).toContain(`\n ${entry}\n`); + } + }); + + // A CDN retry loop whose worst case exceeds the 10-minute job cap turns + // the intended "degrade to a text-only report" into "job killed + // mid-install"; pin the retry flags and compute the bound. + it('keeps the ossutil download retry budget inside the job cap', () => { + const install = stepIn('publish-verify', 'Install ossutil'); + expect(install).toContain('--retry-all-errors'); + const m = install.match( + /curl -fsSL --retry (\d+) --retry-delay (\d+) --retry-all-errors \\\n\s+--connect-timeout (\d+) --max-time (\d+)/, + ); + expect(m).not.toBeNull(); + const worstSeconds = + (Number(m[1]) + 1) * Number(m[4]) + Number(m[1]) * Number(m[2]); + const cap = job('publish-verify').match(/timeout-minutes: (\d+)/); + expect(cap).not.toBeNull(); + expect(worstSeconds).toBeLessThan(Number(cap[1]) * 60); + }); + + // Overriding only one of the bucket/base-URL vars must not post comment + // links that 404 against (or show stale objects from) the other bucket: + // the default base URL is derived from whichever bucket wins. A stalled + // upload is additionally bounded per attempt so one black-hole socket + // cannot burn the job cap and lose the whole report. + it('derives the public URL from the resolved bucket and bounds uploads', () => { + const publish = stepIn( + 'publish-verify', + 'Post verification report comment', + ); + expect(publish).toContain( + 'ALIYUN_OSS_BUCKET: "${{ vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || \'qwen-code-assets\' }}"', + ); + expect(publish).toContain( + "ALIYUN_OSS_PUBLIC_BASE_URL: \"${{ vars.ALIYUN_OSS_PR_ASSETS_PUBLIC_BASE_URL || (vars.ALIYUN_OSS_PR_ASSETS_BUCKET == '' && vars.ALIYUN_OSS_PUBLIC_BASE_URL) || format('https://{0}.oss-cn-hangzhou.aliyuncs.com', vars.ALIYUN_OSS_PR_ASSETS_BUCKET || vars.ALIYUN_OSS_BUCKET || 'qwen-code-assets') }}\"", + ); + expect(publish).toContain("OSS_UPLOAD_ATTEMPT_TIMEOUT_MS: '120000'"); + }); + // Every publish fixture returned [] for the comments listing, so the // PATCH arm — the one that must reuse a live status comment instead of // stranding it — was never executed by any test. @@ -4158,7 +4466,6 @@ describe('qwen-triage verify hardening round 2', () => { AGENT_VERDICT: 'findings', SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', - VERIFY_ASSETS_REMOTE: join(dir, 'none.git'), ...env, }, }); @@ -4518,7 +4825,6 @@ describe('qwen-triage verify publish fidelity', () => { AGENT_VERDICT: '', SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', - VERIFY_ASSETS_REMOTE: join(dir, 'nonexistent.git'), ...env, }, }); @@ -5393,24 +5699,22 @@ describe('qwen-triage verify round-3 hardening', () => { }, ); - // 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', () => { + it('hosts verification evidence on OSS without writing Git refs', () => { 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]/); + expect(publish).toContain( + 'pr-assets/verify/pr${PR_NUMBER}-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT:-1}', + ); + expect(publish).toContain('scripts/upload-aliyun-oss-assets.js'); + expect(publish).not.toContain('checkout -q --orphan'); + expect(publish).not.toContain('git push'); }); - // 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', () => { + // Keep deleting legacy refs until the historical branch population has + // drained, even though new evidence is published to OSS. + it('deletes both legacy pr-assets refs when a PR closes', () => { const cleanup = readFileSync( '.github/workflows/web-shell-visuals-cleanup.yml', 'utf8', @@ -6136,7 +6440,6 @@ describe('qwen-triage verify maintainer-review round', () => { SKIP_REASON: '', PREPARE_FAILURE_PHASE: '', AGENT_VERDICT: '', - VERIFY_ASSETS_REMOTE: join(dir, 'none.git'), }, }); return { @@ -6211,6 +6514,12 @@ describe('qwen-triage verify maintainer-review round', () => { expect(minutes).toBeLessThanOrEqual(30); }); + it('keeps the credential-bearing publisher on a hosted runner', () => { + const publish = job('publish-verify'); + expect(publish).toContain("runs-on: 'ubuntu-latest'"); + expect(publish).not.toContain('ecs-qwen'); + }); + // Upstream failure text can name resolved hosts and TLS detail; the agent // only needs to know the call failed. it('does not forward upstream error text to the agent', () => { diff --git a/scripts/tests/upload-aliyun-oss-assets.test.js b/scripts/tests/upload-aliyun-oss-assets.test.js index 0705b421ef5..90a021110ee 100644 --- a/scripts/tests/upload-aliyun-oss-assets.test.js +++ b/scripts/tests/upload-aliyun-oss-assets.test.js @@ -8,7 +8,11 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { describe, expect, it } from 'vitest'; -import { parseUploadArgs, uploadAssets } from '../upload-aliyun-oss-assets.js'; +import { + parseUploadArgs, + resolveAttemptTimeoutMs, + uploadAssets, +} from '../upload-aliyun-oss-assets.js'; describe('parseUploadArgs', () => { it('returns help=true and skips later validation when --help is passed', () => { @@ -87,12 +91,16 @@ describe('uploadAssets (integration)', () => { fs.mkdirSync(workDir, { recursive: true }); const ossutilPath = path.join(workDir, 'ossutil-shim.cjs'); const logPath = path.join(workDir, 'ossutil.log'); + const exitLine = + behavior === 'hang' + ? 'setTimeout(() => {}, 60000);' // never exits on its own + : `process.exit(${behavior === 'fail' ? 1 : 0});`; fs.writeFileSync( ossutilPath, [ "const fs = require('node:fs');", `fs.appendFileSync(${JSON.stringify(logPath)}, process.argv.slice(2).join('\\n') + '\\n');`, - `process.exit(${behavior === 'fail' ? 1 : 0});`, + exitLine, '', ].join('\n'), ); @@ -172,4 +180,96 @@ describe('uploadAssets (integration)', () => { fs.rmSync(tmp, { recursive: true, force: true }); } }, 30_000); + + it('kills a stalled attempt and retries it when attemptTimeoutMs is set', async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-upload-hang-')); + try { + const { logPath, ossutilCommand, ossutilCommandArgs } = makeOssutilShim( + tmp, + 'hang', + ); + const assetPath = path.join(tmp, 'asset.tar.gz'); + fs.writeFileSync(assetPath, 'asset'); + const configPath = path.join(tmp, '.ossutilconfig'); + fs.writeFileSync(configPath, '[Credentials]\n'); + + // Without the bound this test would sit in the shim's 60s hang; the + // kill turns the stall into an ordinary failed attempt, so all three + // attempts happen and the uploader fails loudly instead of hanging. + expect(() => + uploadAssets( + { + assets: [assetPath], + bucket: 'qwen-test-bucket', + config: configPath, + prefix: 'releases/qwen-code/v0.0.0', + }, + { ossutilCommand, ossutilCommandArgs, attemptTimeoutMs: 400 }, + ), + ).toThrow(/ossutil failed after 3 attempts/); + const uploadAttempts = fs + .readFileSync(logPath, 'utf8') + .split(/\r?\n/) + .filter((line) => line === assetPath); + expect(uploadAttempts).toHaveLength(3); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, 30_000); + + it('succeeds within the attempt bound when the upload is fast', async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-upload-fast-')); + try { + const { ossutilCommand, ossutilCommandArgs } = makeOssutilShim(tmp); + const assetPath = path.join(tmp, 'asset.tar.gz'); + fs.writeFileSync(assetPath, 'asset'); + const configPath = path.join(tmp, '.ossutilconfig'); + fs.writeFileSync(configPath, '[Credentials]\n'); + + expect(() => + uploadAssets( + { + assets: [assetPath], + bucket: 'qwen-test-bucket', + config: configPath, + prefix: 'releases/qwen-code/v0.0.0', + }, + { ossutilCommand, ossutilCommandArgs, attemptTimeoutMs: 5000 }, + ), + ).not.toThrow(); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, 30_000); +}); + +describe('resolveAttemptTimeoutMs', () => { + const KEY = 'OSS_UPLOAD_ATTEMPT_TIMEOUT_MS'; + + it('defaults to unbounded when the env var is unset', () => { + delete process.env[KEY]; + expect(resolveAttemptTimeoutMs()).toBe(0); + }); + + it('reads a millisecond bound from the environment', () => { + process.env[KEY] = '120000'; + try { + expect(resolveAttemptTimeoutMs()).toBe(120000); + } finally { + delete process.env[KEY]; + } + }); + + it('rejects a non-integer or negative bound', () => { + for (const bad of ['soon', '-5', '1.5']) { + process.env[KEY] = bad; + try { + expect(() => resolveAttemptTimeoutMs()).toThrow( + /non-negative integer of milliseconds/, + ); + } finally { + delete process.env[KEY]; + } + } + }); }); diff --git a/scripts/upload-aliyun-oss-assets.js b/scripts/upload-aliyun-oss-assets.js index 8449b178816..145673ae725 100644 --- a/scripts/upload-aliyun-oss-assets.js +++ b/scripts/upload-aliyun-oss-assets.js @@ -12,6 +12,7 @@ import { fail, isMainModule, readOptionValue } from './release-script-utils.js'; const MAX_UPLOAD_ATTEMPTS = 3; const INITIAL_BACKOFF_MS = 2000; +const ATTEMPT_TIMEOUT_ENV = 'OSS_UPLOAD_ATTEMPT_TIMEOUT_MS'; if (isMainModule(import.meta.url)) { try { @@ -28,13 +29,32 @@ function main(argv) { printUsage(); return; } - uploadAssets(args); + uploadAssets(args, { attemptTimeoutMs: resolveAttemptTimeoutMs() }); +} + +// A stalled ossutil (a black-hole socket that accepts but never progresses) +// must not hang the caller forever: the PR publishers run inside a +// 10-minute job cap, and one stuck upload would burn the cap and lose the +// whole report/comment. The bound is opt-in via env so the release syncs — +// which move much larger files — keep today's unbounded behaviour exactly. +function resolveAttemptTimeoutMs() { + const raw = process.env[ATTEMPT_TIMEOUT_ENV]; + if (!raw) { + return 0; + } + const value = Number(raw); + if (!Number.isInteger(value) || value < 0) { + fail( + `${ATTEMPT_TIMEOUT_ENV} must be a non-negative integer of milliseconds, got ${JSON.stringify(raw)}.`, + ); + } + return value; } function printUsage() { console.log(`Usage: node scripts/upload-aliyun-oss-assets.js [options] ASSET... -Uploads local release assets to a public Aliyun OSS prefix via ossutil. +Uploads local assets to a public Aliyun OSS prefix via ossutil. Options: --bucket NAME OSS bucket name. @@ -101,13 +121,18 @@ function parseUploadArgs(argv) { function uploadAssets( { assets, bucket, config, prefix }, - { ossutilCommand = 'ossutil', ossutilCommandArgs = [] } = {}, + { + ossutilCommand = 'ossutil', + ossutilCommandArgs = [], + attemptTimeoutMs = 0, + } = {}, ) { for (const asset of assets) { const key = `${prefix}/${path.basename(asset)}`; uploadWithRetry(asset, bucket, key, config, { ossutilCommand, ossutilCommandArgs, + attemptTimeoutMs, }); } } @@ -117,7 +142,7 @@ function uploadWithRetry( bucket, key, config, - { ossutilCommand, ossutilCommandArgs }, + { ossutilCommand, ossutilCommandArgs, attemptTimeoutMs }, ) { for (let attempt = 1; attempt <= MAX_UPLOAD_ATTEMPTS; attempt += 1) { const result = spawnSync( @@ -133,10 +158,20 @@ function uploadWithRetry( '--acl', 'public-read', ], - { stdio: 'inherit' }, + { + stdio: 'inherit', + // 0 disables the bound (the release syncs' default); a positive + // value SIGKILLs an attempt that stalls past it instead of letting + // it hang the caller out to the job cap. + ...(attemptTimeoutMs > 0 + ? { timeout: attemptTimeoutMs, killSignal: 'SIGKILL' } + : {}), + }, ); - if (result.error) { + // A timeout surfaces as error.code ETIMEDOUT and a killed child — that + // is a retryable attempt failure, not a spawn error to rethrow. + if (result.error && result.error.code !== 'ETIMEDOUT') { throw result.error; } if (result.status === 0) { @@ -145,7 +180,9 @@ function uploadWithRetry( if (attempt < MAX_UPLOAD_ATTEMPTS) { const delayMs = INITIAL_BACKOFF_MS * 2 ** (attempt - 1); console.warn( - `Upload attempt ${attempt}/${MAX_UPLOAD_ATTEMPTS} failed for ${path.basename(asset)}, retrying in ${delayMs / 1000}s...`, + `Upload attempt ${attempt}/${MAX_UPLOAD_ATTEMPTS} failed for ${path.basename(asset)}${ + result.error ? ' (timed out)' : '' + }, retrying in ${delayMs / 1000}s...`, ); sleepSync(delayMs); } @@ -161,4 +198,4 @@ function sleepSync(ms) { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); } -export { parseUploadArgs, uploadAssets }; +export { parseUploadArgs, resolveAttemptTimeoutMs, uploadAssets };