From 0dffaa972316ad14588b28c8cc1f130cb28a91c5 Mon Sep 17 00:00:00 2001 From: Bernhard Wittmann Date: Tue, 18 Aug 2026 07:12:43 +0000 Subject: [PATCH] ci: Report single-instance dependency findings without failing advisory runs (#36437) --- .../workflows/test-single-instance-npm.yml | 20 ++- .../verify-npm-install.test.ts | 29 +++- .../src/single-instance/verify-npm-install.ts | 154 +++++++++++------- 3 files changed, 141 insertions(+), 62 deletions(-) diff --git a/.github/workflows/test-single-instance-npm.yml b/.github/workflows/test-single-instance-npm.yml index 1a0b66c3063..6d4da8d9aca 100644 --- a/.github/workflows/test-single-instance-npm.yml +++ b/.github/workflows/test-single-instance-npm.yml @@ -34,13 +34,19 @@ on: jobs: verify: runs-on: ubuntu-latest - continue-on-error: ${{ !inputs.blocking }} timeout-minutes: ${{ inputs.timeout-minutes }} permissions: contents: read + env: + # Advisory runs downgrade findings to `::warning::` annotations and exit 0, so the known + # third-party splits don't leave a permanently red job that everyone learns to ignore. + # This deliberately replaces `continue-on-error`, which also swallowed the failures we do + # want — a usage error, a rejected input, or a crash that verified nothing. + REPORT_ONLY: ${{ !inputs.blocking && '--report-only' || '' }} steps: # Every rejected combination below otherwise degrades into a run that verifies nothing and - # still exits 0 — silently, since this job is advisory by default. + # still exits 0. These fail the job even in advisory mode — `--report-only` downgrades + # findings, not misconfiguration. - name: Validate inputs env: SCOPE: ${{ inputs.scope }} @@ -88,8 +94,14 @@ jobs: if: inputs.scope == 'changed' env: BASE_REF: ${{ inputs.base-ref }} - run: pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --changed="$BASE_REF" + run: | + # REPORT_ONLY is a single flag or empty. Quoting it would pass an empty argument, which + # the CLI reads as an (unknown) package name and rejects. + # shellcheck disable=SC2086 + pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --changed="$BASE_REF" $REPORT_ONLY - name: Verify npm-install closure (all packages) if: inputs.scope == 'all' - run: pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --all + run: | + # shellcheck disable=SC2086 # see the scoped step above + pnpm --dir packages/testing/code-health exec tsx src/cli.ts verify-npm-install --all $REPORT_ONLY diff --git a/packages/testing/code-health/src/single-instance/verify-npm-install.test.ts b/packages/testing/code-health/src/single-instance/verify-npm-install.test.ts index 0e4a51852fb..b3e1b520b73 100644 --- a/packages/testing/code-health/src/single-instance/verify-npm-install.test.ts +++ b/packages/testing/code-health/src/single-instance/verify-npm-install.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { closureOf, filesTriggerFullRun, matchChangedFiles } from './verify-npm-install.js'; +import { + closureOf, + filesTriggerFullRun, + matchChangedFiles, + resolveTargets, +} from './verify-npm-install.js'; /** Build a WorkspacePkg fixture from `[depName, section]` pairs (only names + sections matter). */ function ws(name: string, deps: Array<[string, string]>) { @@ -94,3 +99,25 @@ describe('filesTriggerFullRun', () => { expect(filesTriggerFullRun(['packages/core/src/index.ts'])).toBe(false); }); }); + +describe('resolveTargets', () => { + it('does not mistake --report-only for a package name', () => { + expect(resolveTargets(['a', '--report-only'], byName, '/x')).toEqual(['a']); + }); + + // Flag-only args must reach the usage error rather than silently verifying nothing. + it('resolves no targets when only flags are passed', () => { + expect(resolveTargets(['--report-only'], byName, '/x')).toEqual([]); + }); + + it('still expands --all alongside the flag', () => { + expect(resolveTargets(['--all', '--report-only'], byName, '/x')?.sort()).toEqual([ + 'a', + 'b', + 'c', + 'd', + 'dev-only', + 'solo', + ]); + }); +}); diff --git a/packages/testing/code-health/src/single-instance/verify-npm-install.ts b/packages/testing/code-health/src/single-instance/verify-npm-install.ts index d978f61e046..9ae26fb8a96 100644 --- a/packages/testing/code-health/src/single-instance/verify-npm-install.ts +++ b/packages/testing/code-health/src/single-instance/verify-npm-install.ts @@ -2,6 +2,7 @@ import { execFileSync } from 'node:child_process'; import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; +import { setTimeout } from 'node:timers/promises'; import { analyze, collectCopies } from './collect-copies.js'; import type { PackageJsonInfo } from '../utils/package-json-scanner.js'; @@ -23,6 +24,35 @@ export function filesTriggerFullRun(files: string[]): boolean { return files.some((f) => ROOT_TRIGGERS.some((t) => f === t || f.startsWith(t))); } +/** + * Surface a finding in the Actions UI. Report-only runs exit 0, and nobody opens the log of a + * green check — without an annotation the finding is indistinguishable from silence. + */ +function annotate(message: string): void { + if (process.env.GITHUB_ACTIONS) console.log(`::warning::${message}`); +} + +/** + * The scratch install is the one network-bound step, and a registry blip must not read as a + * dependency finding. Retries only the install; a persistent failure still throws. + */ +async function installWithRetry(scratch: string, attempts = 3): Promise { + for (let attempt = 1; ; attempt++) { + try { + execFileSync( + 'npm', + ['install', '--no-audit', '--no-fund', '--ignore-scripts', '--no-package-lock'], + { cwd: scratch, stdio: ['ignore', 'inherit', 'inherit'] }, + ); + return; + } catch (error) { + if (attempt >= attempts) throw error; + console.error(`npm install failed (attempt ${attempt}/${attempts}); retrying...`); + await setTimeout(attempt * 5_000); + } + } +} + interface WorkspacePkg { dir: string; relDir: string; @@ -114,7 +144,8 @@ export function closureOf(targets: string[], byName: Map): return [...seen]; } -function resolveTargets( +/** Targets to verify: `null` means "nothing to do", `[]` means the args were unusable. */ +export function resolveTargets( args: string[], byName: Map, rootDir: string, @@ -141,11 +172,14 @@ function resolveTargets( * `workspace:` like publishing does), installs into a scratch dir with npm, then verifies. */ export async function runVerifyNpmInstall(args: string[], rootDir: string): Promise { + const reportOnly = args.includes('--report-only'); const byName = await loadWorkspace(rootDir); const targets = resolveTargets(args, byName, rootDir); if (targets === null) return 0; if (targets.length === 0) { - console.error('Usage: verify-npm-install (--all | --changed= | ...)'); + console.error( + 'Usage: verify-npm-install (--all | --changed= | ...) [--report-only]', + ); return 2; } const unknown = targets.filter((t) => !byName.has(t)); @@ -163,64 +197,70 @@ export async function runVerifyNpmInstall(args: string[], rootDir: string): Prom mkdirSync(tarballs, { recursive: true }); mkdirSync(scratch, { recursive: true }); - console.log(`Packing ${toPack.length} workspace package(s) (targets: ${targets.length})...`); - const tarballByName: Record = {}; - for (const name of toPack) { - const entry = byName.get(name); - if (!entry) continue; - const before = new Set(readdirSync(tarballs)); - execFileSync('pnpm', ['pack', '--pack-destination', tarballs], { - cwd: entry.dir, - stdio: ['ignore', 'ignore', 'inherit'], - }); - const produced = readdirSync(tarballs).find((f) => !before.has(f) && f.endsWith('.tgz')); - if (!produced) throw new Error(`pnpm pack produced no tarball for ${name}`); - tarballByName[name] = join(tarballs, produced); - } + // An enforcing-mode finding is the only reason to keep the tree. A report-only run exits 0 so + // nothing downstream reads it, and a throw (pack failure, registry outage) leaves nothing worth + // inspecting — either way, keeping it just leaks a full node_modules into tmpdir per run. + let keepScratch = false; + try { + console.log(`Packing ${toPack.length} workspace package(s) (targets: ${targets.length})...`); + const tarballByName: Record = {}; + for (const name of toPack) { + const entry = byName.get(name); + if (!entry) continue; + const before = new Set(readdirSync(tarballs)); + execFileSync('pnpm', ['pack', '--pack-destination', tarballs], { + cwd: entry.dir, + stdio: ['ignore', 'ignore', 'inherit'], + }); + const produced = readdirSync(tarballs).find((f) => !before.has(f) && f.endsWith('.tgz')); + if (!produced) throw new Error(`pnpm pack produced no tarball for ${name}`); + tarballByName[name] = join(tarballs, produced); + } - // Scratch project: install targets as file: deps, force ALL packed workspace deps to their - // local tarballs. Third-party deps resolve from the real npm registry. - const fileDep = (n: string): [string, string] => [n, `file:${tarballByName[n]}`]; - const overrides = Object.fromEntries(toPack.map(fileDep)); - const deps = Object.fromEntries(targets.map(fileDep)); - writeFileSync( - join(scratch, 'package.json'), - JSON.stringify( - { name: 'single-instance-scratch', private: true, dependencies: deps, overrides }, - null, - 2, - ), - ); - - console.log('Installing into scratch dir with npm...'); - execFileSync( - 'npm', - ['install', '--no-audit', '--no-fund', '--ignore-scripts', '--no-package-lock'], - { - cwd: scratch, - stdio: ['ignore', 'inherit', 'inherit'], - }, - ); - - const { duplicates, failures } = analyze(collectCopies(scratch)); - - console.log(`\nnpm-install closure — scratch: ${scratch}\n`); - for (const d of duplicates) { - const tag = d.isCurated ? (d.allowed ? 'ALLOWED DUP' : 'FAIL') : 'dup (report)'; - console.log( - ` ${d.name}: ${tag} — ${d.copies.length} copies (${d.copies.map((c) => `v${c.version}`).join(', ')})`, + // Scratch project: install targets as file: deps, force ALL packed workspace deps to their + // local tarballs. Third-party deps resolve from the real npm registry. + const fileDep = (n: string): [string, string] => [n, `file:${tarballByName[n]}`]; + const overrides = Object.fromEntries(toPack.map(fileDep)); + const deps = Object.fromEntries(targets.map(fileDep)); + writeFileSync( + join(scratch, 'package.json'), + JSON.stringify( + { name: 'single-instance-scratch', private: true, dependencies: deps, overrides }, + null, + 2, + ), ); - } - if (failures.length === 0) rmSync(work, { recursive: true, force: true }); - else console.log(`\nScratch install kept for inspection: ${scratch}`); + console.log('Installing into scratch dir with npm...'); + await installWithRetry(scratch); - if (failures.length > 0) { - console.error( - `\nFAIL: ${failures.length} curated library duplicate(s) in the npm-install graph.`, - ); - return 1; + const { duplicates, failures } = analyze(collectCopies(scratch)); + + console.log(`\nnpm-install closure — scratch: ${scratch}\n`); + const curatedTag = reportOnly ? 'CURATED DUP (report)' : 'FAIL'; + for (const d of duplicates) { + const tag = d.isCurated ? (d.allowed ? 'ALLOWED DUP' : curatedTag) : 'dup (report)'; + console.log( + ` ${d.name}: ${tag} — ${d.copies.length} copies (${d.copies.map((c) => `v${c.version}`).join(', ')})`, + ); + } + + if (failures.length > 0) { + keepScratch = !reportOnly; + for (const f of failures) { + annotate( + `${f.name}: ${f.copies.length} copies in the npm-install graph (${f.copies.map((c) => `v${c.version}`).join(', ')})`, + ); + } + console.error( + `\n${reportOnly ? 'REPORT' : 'FAIL'}: ${failures.length} curated library duplicate(s) in the npm-install graph.`, + ); + return reportOnly ? 0 : 1; + } + console.log('\nOK: no curated duplicates in the npm-install graph.'); + return 0; + } finally { + if (keepScratch) console.log(`\nScratch install kept for inspection: ${scratch}`); + else rmSync(work, { recursive: true, force: true }); } - console.log('\nOK: no curated duplicates in the npm-install graph.'); - return 0; }