From 401a8e3b2c2ede12a72a23e2be1451e869b40537 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 11:03:19 +0200 Subject: [PATCH 1/6] Prove a bundled guard still blocks, and settle sideEffects with a measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most consumers of the runtime guard are bundled — a Worker through wrangler, a Next edge middleware, a SvelteKit adapter build — and all of them tree-shake. Nothing covered that path. A guard that has lost the part which screens requests still starts, still logs and still looks installed, and the failure would arrive in the consumer's build rather than ours. `npm run test:bundled` bundles a real edge guard with esbuild through the `workerd` export condition, minifies it, and puts the published CVE-2017-5941 exploit through the bundled output in a separate process: exploit blocked, benign control allowed, route scope honoured. Per-shape size ceilings catch the other regression that matters, a top-level side effect or heavy static import added to the root entry. That test answers the tree-shaking question with numbers instead of a guess. `sideEffects: false` changes both consumer shapes by zero bytes, so it is NOT declared: it would be a permanent promise to every bundler that no module needs evaluating, bought for nothing. `npm run audit:side-effects` parses the published bundles for top-level statements that are not pure declarations, so the decision stays reviewable rather than becoming folklore. `module` is kept. Removing it also changes nothing, but only because esbuild resolves through `exports` — that is not evidence about the older bundlers the field exists for. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 32 ++++++ CONTRIBUTING.md | 22 ++++ package.json | 2 + scripts/bundled-consumer.mjs | 209 ++++++++++++++++++++++++++++++++++ scripts/side-effect-audit.mjs | 98 ++++++++++++++++ 5 files changed, 363 insertions(+) create mode 100644 scripts/bundled-consumer.mjs create mode 100644 scripts/side-effect-audit.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 41dee9cc..2fb4ce45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,6 +102,38 @@ jobs: - name: Consumer shapes against a packed tarball run: node scripts/compat-matrix.mjs --manager ${{ matrix.manager }} + # Most consumers of the runtime guard are BUNDLED — a Worker through wrangler, a Next edge middleware, + # a SvelteKit adapter build — and all of them tree-shake. A guard that has lost the part which screens + # requests still starts, still logs, and still looks installed, so the failure arrives through the + # consumer's build rather than ours and is invisible from here. This bundles a real edge guard and puts + # the published CVE-2017-5941 exploit through the bundled output, which is the only place a + # shaken-away branch can be observed. + bundled-consumer: + name: 🧩 Bundled edge guard blocks the exploit + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 22.x + cache: npm + + - name: Install dependencies + run: npm ci + + # `sideEffects: false` is deliberately NOT declared in `package.json`; it was measured to buy zero + # bytes on both consumer shapes, so declaring it would be a standing promise to bundlers with + # nothing bought for it. This audit is what keeps that decision reviewable: if the field is ever + # added, the evidence has to be here first. + - name: Audit the published bundles for import-time side effects + run: npm run audit:side-effects + + - name: Bundle an edge guard and attack it + run: npm run test:bundled + # The generated `.cmd` launcher and path handling are Windows-only code paths in npm's shim, not ours, # and they are exactly what breaks a bin that works everywhere else. One smoke test rather than the whole # matrix: the question is whether the launcher runs and resolves, not whether four managers agree. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4d1ef69f..653897d1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,6 +55,11 @@ reads exactly like passing. is reachable. Nothing else can see those: they are decided by the tarball's metadata rather than by the code, so the source suite passes while a consumer cannot import the package at all. CI runs it across npm, pnpm, Yarn and Bun, plus a Windows smoke test. +- `npm run test:bundled` if you touched the runtime guard, the build config or `exports`. It bundles an + edge guard with esbuild through the `workerd` condition and then puts the real CVE-2017-5941 exploit + through the bundled output. Most consumers of the guard are bundled and every bundler tree-shakes, and a + guard that has lost the part which screens requests still starts, still logs and still looks installed — + so this is the only check that can see it. It also enforces a size ceiling on each shape. - Read your own diff, commit messages and pull-request description against the boundary above. The pull-request template asks you to confirm this; it is the only control that covers prose, because secret scanning finds credentials and nothing finds a paragraph of architecture. @@ -63,6 +68,23 @@ Changing onboarding, the install prompt or the setup guide? Read `MAINTAINING.md files are load-bearing — and note that the install prompt is a tested artifact with its own gate in `field-test/`. +## Two packaging decisions, and why they are what they are + +**`sideEffects: false` is not declared, deliberately.** It was measured, not assumed: `npm run +test:bundled` builds both consumer shapes with and without the field, and it changes the bundle by zero +bytes in each. The root entry already shakes down to 0.8 kB for a single-symbol import, because the +modules have no import-time side effects — `npm run audit:side-effects` parses the published bundles and +proves that rather than asserting it. So the field would buy nothing while standing as a permanent promise +to every bundler that no module in the package ever needs evaluating; the day someone adds a top-level +registration, consumers lose it silently in their own build. If you want to add the field, add the +evidence first: a shape where it demonstrably saves bytes. + +**`module` is kept even though nothing this repository tests reads it.** The bundled test shows removing +it changes nothing, but that only means a bundler which understands `exports` ignores it — which is what +`exports` is for. Webpack 4 and older rollup setups predate `exports` and fall back to `module`. The test +cannot see those, so a zero delta is not evidence that dropping it is safe, and one line is a cheap thing +to be wrong about in the safe direction. + ## Generated artifacts that must stay in step Each of these is generated from a source of truth and committed. Changing the source without regenerating diff --git a/package.json b/package.json index 6686cfb0..5b3fb7e4 100644 --- a/package.json +++ b/package.json @@ -69,6 +69,8 @@ "dev": "tsup --watch", "test": "vitest run", "test:consumers": "node scripts/compat-matrix.mjs", + "test:bundled": "node scripts/bundled-consumer.mjs", + "audit:side-effects": "npm run build && node scripts/side-effect-audit.mjs dist/index.js dist/protect.js dist/protect.edge.js", "test:manifest": "bun scripts/test-manifest.ts", "test:watch": "vitest", "typecheck": "tsc --noEmit && node scripts/typecheck-templates.mjs", diff --git a/scripts/bundled-consumer.mjs b/scripts/bundled-consumer.mjs new file mode 100644 index 00000000..bd6fd1ac --- /dev/null +++ b/scripts/bundled-consumer.mjs @@ -0,0 +1,209 @@ +// Can a BUNDLER take this package apart without breaking it? +// +// Most consumers of `@patchstack/connect/protect` are bundled: a Cloudflare Worker through wrangler, a +// Next edge middleware, a Vite or SvelteKit adapter build. All of them tree-shake, and a tree-shaken +// guard that has lost the part which screens requests still starts, still logs, still looks installed. +// That is the defect class this whole package exists to avoid, arriving through the consumer's build +// rather than ours. +// +// So this does not measure bundle size and stop. It bundles a real edge guard, minifies it, and then puts +// the published CVE-2017-5941 exploit through the bundled output — the same artifact and the same request +// as `tests/protect/canary-engine-proof.test.ts`, so a shaken-out branch shows up as an exploit that is +// no longer blocked instead of as a smaller file. +// +// It runs twice, with and without `sideEffects: false` in the installed manifest, because that field is a +// promise to the bundler that it may drop unevaluated modules. The two runs are what turns declaring it +// from an assumption into a measurement: the sizes say whether the field buys anything, and the canary +// says whether it costs correctness. +// +// `npm run test:bundled`. +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, statSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const artifact = JSON.parse( + readFileSync(path.join(root, 'tests', 'protect', 'fixtures', 'canary', 'cve-2017-5941.rule.json'), 'utf8'), +); + +// Same shapes as the canary test, deliberately duplicated rather than imported: this script runs against +// an INSTALLED tarball in a temporary directory, and reaching back into the repository's test helpers is +// how a packaging test ends up proving something about the repository instead of about the package. +const EXPLOIT = JSON.stringify({ + state: '_$$ND_FUNC$$_function (){ require("child_process").exec("id > /tmp/pwned", function(){}); }()', +}); +const BENIGN = JSON.stringify({ state: '{"cart":[{"sku":"AB-1","qty":2}],"currency":"EUR"}' }); + +const run = (cmd, args, cwd) => + execFileSync(cmd, args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + +function pack() { + const out = mkdtempSync(path.join(tmpdir(), 'ps-pack-')); + // `npm pack` runs `prepare`, so the tarball is built from the current sources rather than a stale dist. + run('npm', ['pack', '--pack-destination', out], root); + const name = `patchstack-connect-${JSON.parse(readFileSync(path.join(root, 'package.json'), 'utf8')).version}.tgz`; + return path.join(out, name); +} + +// A second shape: one small symbol from the ROOT entry. The guard shape above pulls in the whole runtime +// by design, so it cannot show tree-shaking working or failing — this can. It is also the question of +// entry hygiene: whether importing one helper from the root drags the CLI, the scaffolder and the +// TypeScript-dependent map analysis in behind it. +// `compareVersions` is a small pure function with no dependencies of its own, chosen so that whatever +// survives in the bundle is what CANNOT be shaken out rather than what this import needed. +const ROOT_IMPORT = ` +import { compareVersions } from '@patchstack/connect'; + +export const compare = compareVersions; +console.log(compare('1.0.0', '1.0.1')); +`; + +const GUARD = ` +import { createProtection } from '@patchstack/connect/protect'; +import RULE from './rule.json' with { type: 'json' }; + +const ready = createProtection({ rules: { firewall: [RULE], whitelists: [], whitelist_keys: {} }, mode: 'block' }); + +export async function screen(request) { + const protection = await ready; + return protection.fetchGuard()(request); +} +`; + +// The bundled guard is exercised in a SEPARATE process from a file on disk, so what is tested is the +// artifact the bundler emitted and not a module graph this script still has warm in memory. +const HARNESS = ` +import { screen } from './bundle.js'; + +const post = (body, url) => new Request(url, { method: 'POST', headers: { 'content-type': 'application/json' }, body }); +const exploit = await screen(post(${JSON.stringify(EXPLOIT)}, 'https://app.test/api/restore')); +const benign = await screen(post(${JSON.stringify(BENIGN)}, 'https://app.test/api/restore')); +const other = await screen(post(${JSON.stringify(EXPLOIT)}, 'https://app.test/api/profile')); + +console.log(JSON.stringify({ + exploit: exploit ? exploit.status : null, + benign: benign ? benign.status : null, + scoped: other ? other.status : null, +})); +`; + +function measure(tarball, { shape, sideEffectsFalse = false, dropModule = false }) { + const dir = mkdtempSync(path.join(tmpdir(), 'ps-bundle-')); + writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: 'c', private: true, type: 'module' }, null, 2)); + run('npm', ['install', '--no-audit', '--no-fund', tarball], dir); + + const installed = path.join(dir, 'node_modules', '@patchstack', 'connect', 'package.json'); + const manifest = JSON.parse(readFileSync(installed, 'utf8')); + if (sideEffectsFalse) manifest.sideEffects = false; + else delete manifest.sideEffects; + // `module` predates `exports` and is only consulted by bundlers that ignore `exports`. Removing it and + // comparing byte counts is how we learn whether it is still doing anything, rather than keeping it + // because removing it feels risky. + if (dropModule) delete manifest.module; + writeFileSync(installed, JSON.stringify(manifest, null, 2)); + + const edge = shape === 'edge-guard'; + writeFileSync(path.join(dir, 'guard.js'), edge ? GUARD : ROOT_IMPORT); + writeFileSync(path.join(dir, 'rule.json'), JSON.stringify(artifact.rule)); + + // The edge shape resolves `protect.edge.js` through the `workerd` export condition — the build most + // likely to differ from what the tests cover, and the one with a hard size limit. The root shape is + // bundled for Node, which is where a root import actually happens. + run(path.join(root, 'node_modules', '.bin', 'esbuild'), [ + 'guard.js', '--bundle', '--minify', '--format=esm', '--outfile=bundle.js', '--log-level=error', + '--loader:.json=json', + ...(edge ? ['--platform=browser', '--conditions=workerd'] : ['--platform=node']), + ], dir); + + const bytes = statSync(path.join(dir, 'bundle.js')).size; + let verdict = null; + if (edge) { + writeFileSync(path.join(dir, 'harness.mjs'), HARNESS); + verdict = JSON.parse(run(process.execPath, ['harness.mjs'], dir).trim()); + } + + rmSync(dir, { recursive: true, force: true }); + return { bytes, verdict }; +} + +const tarball = pack(); +console.log(`bundling an edge guard from ${path.basename(tarball)} with esbuild ${run(path.join(root, 'node_modules', '.bin', 'esbuild'), ['--version'], root).trim()}\n`); + +// Ceilings, not pins. Generous over what is measured today (0.8 kB root, 97 kB edge guard) so ordinary +// growth does not trip them, and low enough to catch the two regressions that matter: a top-level side +// effect or a heavy static import added to the ROOT entry, which would put the whole scanner into every +// consumer's bundle; and the edge guard doubling, which is the one with a hard platform limit. +const CEILING = { 'root-import': 8 * 1024, 'edge-guard': 200 * 1024 }; + +const CONFIGS = [ + { label: 'as published', opts: {} }, + { label: 'sideEffects: false', opts: { sideEffectsFalse: true } }, + { label: 'no `module` field', opts: { dropModule: true } }, +]; + +let failed = 0; +const sizes = {}; + +for (const shape of ['edge-guard', 'root-import']) { + console.log(` ${shape === 'edge-guard' ? 'edge guard (workerd condition, runs the canary)' : 'one symbol from the root entry'}`); + sizes[shape] = {}; + + for (const { label, opts } of CONFIGS) { + const { bytes, verdict } = measure(tarball, { shape, ...opts }); + sizes[shape][label] = bytes; + + const overCeiling = bytes > CEILING[shape]; + if (overCeiling) { + failed++; + console.log( + ` FAIL ${label.padEnd(20)} ${(bytes / 1024).toFixed(1).padStart(7)} kB ` + + `over the ${(CEILING[shape] / 1024).toFixed(0)} kB ceiling for this shape`, + ); + continue; + } + + if (verdict === null) { + console.log(` ok ${label.padEnd(20)} ${(bytes / 1024).toFixed(1).padStart(7)} kB`); + continue; + } + // Blocked, benign allowed, scope honoured — asserted on the bundled output, which is the only place a + // shaken-away branch can be observed. + const ok = verdict.exploit === 403 && verdict.benign === null && verdict.scoped === null; + if (!ok) failed++; + console.log( + ` ${ok ? 'ok ' : 'FAIL'} ${label.padEnd(20)} ${(bytes / 1024).toFixed(1).padStart(7)} kB ` + + `exploit=${verdict.exploit} benign=${verdict.benign ?? 'allowed'} other-route=${verdict.scoped ?? 'allowed'}`, + ); + } + console.log(''); +} + +const describe = (shape, label) => { + const delta = sizes[shape]['as published'] - sizes[shape][label]; + if (delta === 0) return 'no change'; + return `${delta > 0 ? '-' : '+'}${(Math.abs(delta) / 1024).toFixed(1)} kB`; +}; + +for (const label of ['sideEffects: false', 'no `module` field']) { + console.log(` \`${label}\`: edge guard ${describe('edge-guard', label)}, root import ${describe('root-import', label)}`); +} + +// What this measurement can and cannot settle, stated here so a future reader does not take the second +// line above for more than it is. esbuild resolves through `exports`, so it never consults `module`; a +// zero delta means this bundler does not need the field, NOT that no bundler does. Webpack 4 and older +// rollup setups predate `exports` and fall back to `module`. Keeping it costs one line. +if (sizes['root-import']['no `module` field'] === sizes['root-import']['as published']) { + console.log( + '\n note: the `module` result only shows that a bundler which reads `exports` ignores the field.\n' + + ' It is not evidence that dropping it is safe for bundlers that predate `exports`.', + ); +} + +if (failed > 0) { + console.error('\nA bundled guard did not behave like the engine it was built from. Do not ship this.'); + process.exit(1); +} +rmSync(path.dirname(tarball), { recursive: true, force: true }); +console.log('\n the bundled guard blocks the exploit and allows the control in every configuration.'); diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs new file mode 100644 index 00000000..ad2a48f2 --- /dev/null +++ b/scripts/side-effect-audit.mjs @@ -0,0 +1,98 @@ +// Does `sideEffects: false` tell a bundler the truth? +// +// That field is a promise: if a consumer imports nothing from a module, the bundler may drop the module +// without evaluating it. Declaring it when it is false does not produce a build error — it produces an +// application missing a side effect it depended on, at run time, in the consumer's build and not ours. +// For a package whose job is to be present and screening requests, "silently not there" is the worst +// available failure, so the field is only defensible with an audit behind it. +// +// So this parses the PUBLISHED bundles (what a bundler actually sees, not the sources) and lists every +// top-level statement that is not a pure declaration. It is intentionally conservative: anything it +// cannot prove pure is reported for a human to read, and reporting something harmless costs a minute +// while missing something does not announce itself. +// +// Run as `npm run audit:side-effects` after a build. +import ts from 'typescript'; +import { readFileSync, existsSync } from 'node:fs'; + +// What a bundler asks when it sees `sideEffects: false`: if nothing is imported FROM this module, is it +// safe to drop the module entirely? That is only true if evaluating it top to bottom does nothing +// observable. So: parse the published bundle and list every top-level statement that is not a pure +// declaration. +const DECL = new Set([ + ts.SyntaxKind.FunctionDeclaration, ts.SyntaxKind.ClassDeclaration, ts.SyntaxKind.InterfaceDeclaration, + ts.SyntaxKind.TypeAliasDeclaration, ts.SyntaxKind.EnumDeclaration, ts.SyntaxKind.ImportDeclaration, + ts.SyntaxKind.ExportDeclaration, ts.SyntaxKind.ExportAssignment, ts.SyntaxKind.EmptyStatement, +]); + +// A `const x = ` is a declaration whose initialiser must also be pure. +function initialiserIsPure(node) { + if (!node) return true; + switch (node.kind) { + case ts.SyntaxKind.StringLiteral: case ts.SyntaxKind.NumericLiteral: + case ts.SyntaxKind.TrueKeyword: case ts.SyntaxKind.FalseKeyword: case ts.SyntaxKind.NullKeyword: + case ts.SyntaxKind.ArrowFunction: case ts.SyntaxKind.FunctionExpression: + case ts.SyntaxKind.ClassExpression: case ts.SyntaxKind.Identifier: + case ts.SyntaxKind.RegularExpressionLiteral: case ts.SyntaxKind.TemplateExpression: + case ts.SyntaxKind.NoSubstitutionTemplateLiteral: + return true; + case ts.SyntaxKind.ObjectLiteralExpression: + return node.properties.every((p) => !p.initializer || initialiserIsPure(p.initializer)); + case ts.SyntaxKind.ArrayLiteralExpression: + return node.elements.every((e) => initialiserIsPure(e)); + case ts.SyntaxKind.NewExpression: { + // `new Set([...])`, `new Map([...])`, `new RegExp(...)` construct a value and touch nothing else. + const n = node.expression.getText(); + return ['Set', 'Map', 'WeakSet', 'WeakMap', 'RegExp', 'Error'].includes(n) + && (node.arguments ?? []).every((a) => initialiserIsPure(a)); + } + case ts.SyntaxKind.PropertyAccessExpression: case ts.SyntaxKind.ElementAccessExpression: + return initialiserIsPure(node.expression); + case ts.SyntaxKind.BinaryExpression: + return initialiserIsPure(node.left) && initialiserIsPure(node.right); + case ts.SyntaxKind.PrefixUnaryExpression: case ts.SyntaxKind.ParenthesizedExpression: + case ts.SyntaxKind.AsExpression: case ts.SyntaxKind.NonNullExpression: + return initialiserIsPure(node.expression ?? node.operand); + case ts.SyntaxKind.ConditionalExpression: + return initialiserIsPure(node.condition) && initialiserIsPure(node.whenTrue) && initialiserIsPure(node.whenFalse); + case ts.SyntaxKind.CallExpression: { + // A call at module scope is the interesting case. Object.freeze/keys/entries and Array/String + // statics over pure arguments are value construction; anything else is reported for a human. + const t = node.expression.getText(); + // esbuild's lazy CommonJS wrapper. `__commonJS({ 'x'() { … } })` stores the module body in a + // memoised loader and returns it WITHOUT running it; the body executes on the first call, which + // for these bundles is behind a dynamic import on the Node path only. Deliberately narrow: the + // sibling helper `__toESM(require_x())` *does* invoke the body at module scope, and must keep + // being reported — so this matches the wrapper alone and never a call to what it returns. + if (t === '__commonJS' || t === '__esm') return true; + const PURE = /^(Object\.(freeze|keys|values|entries|assign|create|fromEntries)|Array\.(from|isArray)|String|Number|Boolean|Symbol(\.for)?|JSON\.parse|Math\.\w+|\[[^\]]*\]\.(map|filter|flatMap|concat|join|slice))$/; + return PURE.test(t) && (node.arguments ?? []).every((a) => initialiserIsPure(a)); + } + default: return false; + } +} + +let bad = 0; +for (const file of process.argv.slice(2)) { + if (!existsSync(file)) { console.log(` ${file}: MISSING`); bad++; continue; } + const src = readFileSync(file, 'utf8'); + const sf = ts.createSourceFile(file, src, ts.ScriptTarget.ESNext, true); + const effects = []; + for (const st of sf.statements) { + if (DECL.has(st.kind)) continue; + if (ts.isVariableStatement(st)) { + for (const d of st.declarationList.declarations) { + if (!initialiserIsPure(d.initializer)) { + effects.push({ line: sf.getLineAndCharacterOfPosition(d.getStart()).line + 1, text: d.getText().slice(0, 110) }); + } + } + continue; + } + effects.push({ line: sf.getLineAndCharacterOfPosition(st.getStart()).line + 1, text: st.getText().slice(0, 110).replace(/\n/g, ' ') }); + } + const kb = (src.length / 1024).toFixed(0); + console.log(`\n ${file} (${kb} kB, ${sf.statements.length} top-level statements)`); + if (effects.length === 0) console.log(' no import-time side effects'); + else { bad += effects.length; for (const e of effects) console.log(` L${String(e.line).padEnd(6)} ${e.text}`); } +} +process.exit(bad > 0 ? 1 : 0); From 25431f69e48bf61e511f839cfc95e5ed86171797 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 12:16:56 +0200 Subject: [PATCH 2/6] Make the side-effect audit conservative, and stop embedding source text in maps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit recognised shapes it believed were safe, which is the wrong direction to be wrong in: `import './register.js'`, `export default init()` and `class C extends init()` all execute at module evaluation and all three were reported clean, as were object spreads, template interpolations and computed member names. It now walks the region of the AST that runs at import time — pruning function and method bodies and instance field initialisers — and reports anything that can execute, rather than matching a list of permitted forms. A class is the case worth naming: method bodies do not run, but the heritage clause, computed member names, static field initialisers and static blocks do. One exemption is provable rather than assumed: `Object.freeze()` over an argument constructed in place cannot be observed anywhere else. `Object.freeze(somethingImported)` is still reported. `--selftest` pins 23 cases, and CI runs it before the audit, because a tripwire that cannot fire is the same defect as no tripwire. The prose no longer calls it proof; the bundled-consumer test is the evidence. Source maps drop `sourcesContent`: 975 kB packed becomes 434 kB, and 3.55 MB unpacked becomes 1.77 MB. A frame still resolves to file and line, and the source text it duplicated is public in this repository at the tag matching the version the CLI now reports. Two build steps produce maps independently, so `tests/source-maps.test.ts` asserts over the artifacts rather than the config: every artifact has a map, every artifact points at it, every map has real mappings and sources, no map embeds source text, and a real thrown frame resolves to the statement that threw. Adds a single `Required CI gate` job for branch protection to require, so the gate does not name matrix labels that change with every version bump. It uses `if: always()` — a skipped required check reads as success — and refuses to pass while standing on fewer jobs than it is meant to require. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 73 +++++++- CONTRIBUTING.md | 19 +- scripts/build-edge.mjs | 5 + scripts/side-effect-audit.mjs | 331 ++++++++++++++++++++++++++-------- tests/source-maps.test.ts | 133 ++++++++++++++ tsup.config.ts | 18 ++ 6 files changed, 495 insertions(+), 84 deletions(-) create mode 100644 tests/source-maps.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fb4ce45..b30775a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,9 +126,13 @@ jobs: # `sideEffects: false` is deliberately NOT declared in `package.json`; it was measured to buy zero # bytes on both consumer shapes, so declaring it would be a standing promise to bundlers with - # nothing bought for it. This audit is what keeps that decision reviewable: if the field is ever - # added, the evidence has to be here first. - - name: Audit the published bundles for import-time side effects + # nothing bought for it. The audit is a tripwire that keeps that decision reviewable — it reports + # what executes at import time and cannot prove purity. Its own behaviour is checked first, because + # a tripwire that cannot fire is the same defect as no tripwire. + - name: Check the side-effect audit against its own cases + run: node scripts/side-effect-audit.mjs --selftest + + - name: Report anything the published bundles execute at import time run: npm run audit:side-effects - name: Bundle an edge guard and attack it @@ -235,3 +239,66 @@ jobs: - name: Audit published dependency tree run: npm audit --omit=dev --audit-level=moderate + + # One status for branch protection to require. + # + # Requiring the jobs above directly means branch protection names a matrix label — `Consumers on npm + # 11.6.0`, `Validate on Node 24.x`. Those names change whenever a version is bumped, and a required + # check whose name no longer exists is not reported as missing: it is simply never satisfied, or worse, + # silently dropped from the set that gates the merge. Requiring this one job instead means the matrix can + # change freely and the gate keeps meaning "everything passed". + # + # `if: always()` is what makes it work. Without it this job would be SKIPPED when a dependency fails, + # and a skipped required check reads as success to branch protection — the gate would pass precisely + # when something was broken. + required: + name: Required CI gate + if: always() + needs: + - capability-contract + - consumers + - bundled-consumer + - windows-smoke + - validate + - production-audit + runs-on: ubuntu-latest + + steps: + - name: Every required job must have succeeded + env: + # The whole `needs` context, so this cannot drift out of step with the list above: a job added + # to `needs` is covered without a second edit here. + RESULTS: ${{ toJSON(needs) }} + run: | + set -euo pipefail + + # A matrix job contributes ONE result under its job id, so `consumers` covers every manager and + # `validate` covers every Node version, including 24. + # A floor on how many jobs this gate is standing on. Emptying the `needs` list above would + # otherwise leave a green required check that verifies nothing at all — the one failure mode a + # gate must not have, since it is indistinguishable from a working one. + count=$(printf '%s' "$RESULTS" | python3 -c 'import json, sys; print(len(json.load(sys.stdin)))') + if [ "$count" -lt 6 ]; then + echo "::error::This gate is standing on ${count} job(s); it is meant to require 6. A required check that verifies nothing passes exactly when something is broken." + exit 1 + fi + + failed=$(printf '%s' "$RESULTS" | python3 -c " + import json, sys + needs = json.load(sys.stdin) + bad = [name for name, job in needs.items() if job.get('result') != 'success'] + print(' '.join(sorted(bad))) + ") + + printf '%s' "$RESULTS" | python3 -c " + import json, sys + for name, job in sorted(json.load(sys.stdin).items()): + print(f\"{job.get('result', 'unknown'):>9} {name}\") + " + + if [ -n "$failed" ]; then + echo "::error::Not every required job succeeded: ${failed}" + exit 1 + fi + + echo 'All required jobs succeeded.' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 653897d1..2adfda01 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,12 +72,19 @@ files are load-bearing — and note that the install prompt is a tested artifact **`sideEffects: false` is not declared, deliberately.** It was measured, not assumed: `npm run test:bundled` builds both consumer shapes with and without the field, and it changes the bundle by zero -bytes in each. The root entry already shakes down to 0.8 kB for a single-symbol import, because the -modules have no import-time side effects — `npm run audit:side-effects` parses the published bundles and -proves that rather than asserting it. So the field would buy nothing while standing as a permanent promise -to every bundler that no module in the package ever needs evaluating; the day someone adds a top-level -registration, consumers lose it silently in their own build. If you want to add the field, add the -evidence first: a shape where it demonstrably saves bytes. +bytes in each. The root entry already shakes down to 0.8 kB for a single-symbol import. So the field would +buy nothing while standing as a permanent promise to every bundler that no module in the package ever +needs evaluating; the day someone adds a top-level registration, consumers lose it silently in their own +build. If you want to add the field, add the evidence first: a shape where it demonstrably saves bytes. + +`npm run audit:side-effects` supports that decision but does not settle it. It is a **tripwire**, not a +proof: it walks the part of each published bundle that runs at import time and reports anything that can +execute, deliberately over-reporting rather than recognising shapes it believes are safe. What it cannot +see is what a called function does, so a clean report means "nothing here obviously executes" and never +"safe to drop". `--selftest` pins its behaviour on 23 cases, several of which an earlier version of the +script reported clean — `export default init()`, `class C extends init()` and a bare `import './x.js'` all +look like plain declarations to an allowlist, and all three run code. The real evidence that the package +survives a bundler is `npm run test:bundled`, which attacks the bundled guard. **`module` is kept even though nothing this repository tests reads it.** The bundled test shows removing it changes nothing, but that only means a bundler which understands `exports` ignores it — which is what diff --git a/scripts/build-edge.mjs b/scripts/build-edge.mjs index 1802ccaa..ef6fe0d3 100644 --- a/scripts/build-edge.mjs +++ b/scripts/build-edge.mjs @@ -41,6 +41,11 @@ const result = await esbuild.build({ platform: 'browser', // WinterCG: no Node globals assumed target: 'es2022', sourcemap: true, + // The map resolves a stack frame to file and line without carrying the source text, which is published + // in this repository anyway. `tsup.config.ts` sets the same thing for the other three artifacts, and + // `tests/source-maps.test.ts` checks every one of them — the setting living in two places is the reason + // that test exists. + sourcesContent: false, // Keep the stub inline so the artifact is a single self-contained file (an edge bundler should not // have to chase a chunk that only ever throws). splitting: false, diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs index ad2a48f2..1d0349e4 100644 --- a/scripts/side-effect-audit.mjs +++ b/scripts/side-effect-audit.mjs @@ -4,95 +4,276 @@ // without evaluating it. Declaring it when it is false does not produce a build error — it produces an // application missing a side effect it depended on, at run time, in the consumer's build and not ours. // For a package whose job is to be present and screening requests, "silently not there" is the worst -// available failure, so the field is only defensible with an audit behind it. +// available failure, so the field is only defensible with evidence behind it. // -// So this parses the PUBLISHED bundles (what a bundler actually sees, not the sources) and lists every -// top-level statement that is not a pure declaration. It is intentionally conservative: anything it -// cannot prove pure is reported for a human to read, and reporting something harmless costs a minute -// while missing something does not announce itself. +// This is a TRIPWIRE, not a proof. It reports constructs that execute when a module is evaluated, and it +// is deliberately built to over-report: it walks the region of the AST that runs at import time and flags +// anything that can execute, rather than recognising a list of shapes it believes are safe. An allowlist +// gets this wrong in the dangerous direction — `export default init()`, `class C extends init()` and +// `import './register.js'` all look like plain declarations to one, and all three run code. +// +// What it cannot do is prove purity. It sees one bundle's syntax, not what a called function does, and a +// clean report means "nothing here obviously executes", never "this module is safe to drop". The evidence +// that the package survives a bundler is `npm run test:bundled`, which attacks the bundled guard. // // Run as `npm run audit:side-effects` after a build. import ts from 'typescript'; import { readFileSync, existsSync } from 'node:fs'; -// What a bundler asks when it sees `sideEffects: false`: if nothing is imported FROM this module, is it -// safe to drop the module entirely? That is only true if evaluating it top to bottom does nothing -// observable. So: parse the published bundle and list every top-level statement that is not a pure -// declaration. -const DECL = new Set([ - ts.SyntaxKind.FunctionDeclaration, ts.SyntaxKind.ClassDeclaration, ts.SyntaxKind.InterfaceDeclaration, - ts.SyntaxKind.TypeAliasDeclaration, ts.SyntaxKind.EnumDeclaration, ts.SyntaxKind.ImportDeclaration, - ts.SyntaxKind.ExportDeclaration, ts.SyntaxKind.ExportAssignment, ts.SyntaxKind.EmptyStatement, -]); - -// A `const x = ` is a declaration whose initialiser must also be pure. -function initialiserIsPure(node) { - if (!node) return true; - switch (node.kind) { - case ts.SyntaxKind.StringLiteral: case ts.SyntaxKind.NumericLiteral: - case ts.SyntaxKind.TrueKeyword: case ts.SyntaxKind.FalseKeyword: case ts.SyntaxKind.NullKeyword: - case ts.SyntaxKind.ArrowFunction: case ts.SyntaxKind.FunctionExpression: - case ts.SyntaxKind.ClassExpression: case ts.SyntaxKind.Identifier: - case ts.SyntaxKind.RegularExpressionLiteral: case ts.SyntaxKind.TemplateExpression: - case ts.SyntaxKind.NoSubstitutionTemplateLiteral: - return true; - case ts.SyntaxKind.ObjectLiteralExpression: - return node.properties.every((p) => !p.initializer || initialiserIsPure(p.initializer)); - case ts.SyntaxKind.ArrayLiteralExpression: - return node.elements.every((e) => initialiserIsPure(e)); - case ts.SyntaxKind.NewExpression: { - // `new Set([...])`, `new Map([...])`, `new RegExp(...)` construct a value and touch nothing else. - const n = node.expression.getText(); - return ['Set', 'Map', 'WeakSet', 'WeakMap', 'RegExp', 'Error'].includes(n) - && (node.arguments ?? []).every((a) => initialiserIsPure(a)); +/** + * Constructors that build a value and touch nothing else. Everything else, including any call, is + * reported — the audit does not try to decide whether a function is pure. + */ +const PURE_CONSTRUCTORS = new Set(['Set', 'Map', 'WeakSet', 'WeakMap', 'RegExp', 'Error', 'Date', 'URL']); + +/** + * esbuild's lazy CommonJS wrapper. `__commonJS({ 'x'() { … } })` stores a module body in a memoised + * loader and returns it WITHOUT running it; the body executes on the first call. + * + * Narrow on purpose, and the narrowness is the point: the sibling helper `__toESM(require_x())` DOES + * invoke the body at module scope, so only the wrapper itself is exempt and never a call to what it + * returns. + */ +const LAZY_WRAPPERS = new Set(['__commonJS', '__esm']); + +/** + * `Object.freeze()` — the one call shape this audit accepts, and only over an argument that is + * constructed in place. + * + * The exemption is provable rather than assumed: freezing an object or array literal that no other code + * has a reference to yet cannot be observed from anywhere else, so evaluating it has no effect a bundler + * could drop. Nesting is handled by the ordinary recursion, so `Object.freeze(Object.freeze([…]))` is + * accepted for the same reason at each level. + * + * `Object.freeze(someImportedThing)` is NOT accepted: that mutates an object someone else can see, which + * is exactly the kind of import-time effect this exists to find. + */ +function isFreezeOfLiteral(node) { + if (node.expression.getText() !== 'Object.freeze') return false; + const [arg, ...rest] = node.arguments ?? []; + + return rest.length === 0 && arg !== undefined + && (ts.isObjectLiteralExpression(arg) || ts.isArrayLiteralExpression(arg)); +} + +/** + * Find everything that executes when this source file is evaluated. + * + * Two halves. `skip` prunes the subtrees that do NOT run at import time — function and method bodies, + * instance field initialisers — so a module full of functions reports nothing. Everything still in scope + * after that pruning is checked for constructs that execute. + * + * A class is the case worth naming: its method bodies do not run, but its `extends` clause, its computed + * member names, its static field initialisers and its static blocks all do. + */ +function findExecutable(sf) { + const found = []; + const at = (node) => sf.getLineAndCharacterOfPosition(node.getStart()).line + 1; + const text = (node) => node.getText().slice(0, 100).replace(/\s+/g, ' '); + + const visit = (node) => { + // --- Regions that are NOT evaluated when the module is. + if ( + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isArrowFunction(node) || + ts.isMethodDeclaration(node) || + ts.isGetAccessor(node) || + ts.isSetAccessor(node) || + ts.isConstructorDeclaration(node) + ) { + // The body does not run at evaluation time, and neither do default parameter values — those are + // evaluated per call. A COMPUTED NAME does run, because the key has to exist before the object or + // class does. + if (node.name && ts.isComputedPropertyName(node.name)) visit(node.name.expression); + + return; } - case ts.SyntaxKind.PropertyAccessExpression: case ts.SyntaxKind.ElementAccessExpression: - return initialiserIsPure(node.expression); - case ts.SyntaxKind.BinaryExpression: - return initialiserIsPure(node.left) && initialiserIsPure(node.right); - case ts.SyntaxKind.PrefixUnaryExpression: case ts.SyntaxKind.ParenthesizedExpression: - case ts.SyntaxKind.AsExpression: case ts.SyntaxKind.NonNullExpression: - return initialiserIsPure(node.expression ?? node.operand); - case ts.SyntaxKind.ConditionalExpression: - return initialiserIsPure(node.condition) && initialiserIsPure(node.whenTrue) && initialiserIsPure(node.whenFalse); - case ts.SyntaxKind.CallExpression: { - // A call at module scope is the interesting case. Object.freeze/keys/entries and Array/String - // statics over pure arguments are value construction; anything else is reported for a human. - const t = node.expression.getText(); - // esbuild's lazy CommonJS wrapper. `__commonJS({ 'x'() { … } })` stores the module body in a - // memoised loader and returns it WITHOUT running it; the body executes on the first call, which - // for these bundles is behind a dynamic import on the Node path only. Deliberately narrow: the - // sibling helper `__toESM(require_x())` *does* invoke the body at module scope, and must keep - // being reported — so this matches the wrapper alone and never a call to what it returns. - if (t === '__commonJS' || t === '__esm') return true; - const PURE = /^(Object\.(freeze|keys|values|entries|assign|create|fromEntries)|Array\.(from|isArray)|String|Number|Boolean|Symbol(\.for)?|JSON\.parse|Math\.\w+|\[[^\]]*\]\.(map|filter|flatMap|concat|join|slice))$/; - return PURE.test(t) && (node.arguments ?? []).every((a) => initialiserIsPure(a)); + + // A non-static property initialiser runs on construction, not on evaluation. + if (ts.isPropertyDeclaration(node)) { + const isStatic = (ts.getCombinedModifierFlags(node) & ts.ModifierFlags.Static) !== 0; + if (node.name && ts.isComputedPropertyName(node.name)) visit(node.name.expression); + if (isStatic && node.initializer) visit(node.initializer); + + return; } - default: return false; + + // --- Constructs that DO execute at evaluation time. + switch (node.kind) { + case ts.SyntaxKind.ImportDeclaration: { + // A side-effect import — `import './register.js'` with no bindings — exists only to run another + // module. An import WITH bindings is a dependency edge rather than a statement of its own, and is + // reported separately as context. + if (node.importClause === undefined) { + found.push({ line: at(node), text: text(node), why: 'side-effect import: runs another module' }); + } + + return; + } + case ts.SyntaxKind.CallExpression: { + const callee = node.expression.getText(); + if (isFreezeOfLiteral(node)) { + // Keep walking the literal: the freeze is accepted, what is inside it may not be. + for (const a of node.arguments ?? []) visit(a); + + return; + } + if (LAZY_WRAPPERS.has(callee)) { + // The wrapper does not run its argument, but the argument may still contain code that runs for + // other reasons, so keep walking it. + for (const a of node.arguments ?? []) visit(a); + + return; + } + found.push({ line: at(node), text: text(node), why: `call at module scope: ${callee.slice(0, 40)}` }); + + return; + } + case ts.SyntaxKind.NewExpression: { + const ctor = node.expression.getText(); + if (!PURE_CONSTRUCTORS.has(ctor)) { + found.push({ line: at(node), text: text(node), why: `construction at module scope: new ${ctor.slice(0, 40)}` }); + + return; + } + break; + } + case ts.SyntaxKind.TaggedTemplateExpression: + found.push({ line: at(node), text: text(node), why: 'tagged template: invokes the tag function' }); + + return; + case ts.SyntaxKind.AwaitExpression: + found.push({ line: at(node), text: text(node), why: 'top-level await' }); + + return; + case ts.SyntaxKind.ClassStaticBlockDeclaration: + found.push({ line: at(node), text: text(node), why: 'static initialisation block' }); + + return; + case ts.SyntaxKind.DeleteExpression: + found.push({ line: at(node), text: text(node), why: 'delete: mutates an object' }); + + return; + case ts.SyntaxKind.BinaryExpression: + // Assignment to anything reachable from outside — `module.exports = …`, `globalThis.x = …`. + if ( + ts.isPropertyAccessExpression(node.left) || + ts.isElementAccessExpression(node.left) + ) { + const op = node.operatorToken.kind; + const assigns = + op === ts.SyntaxKind.EqualsToken || + (op >= ts.SyntaxKind.FirstCompoundAssignment && op <= ts.SyntaxKind.LastCompoundAssignment); + if (assigns) { + found.push({ line: at(node), text: text(node), why: 'assignment to a property at module scope' }); + } + } + break; + case ts.SyntaxKind.PrefixUnaryExpression: + case ts.SyntaxKind.PostfixUnaryExpression: + if ( + node.operator === ts.SyntaxKind.PlusPlusToken || + node.operator === ts.SyntaxKind.MinusMinusToken + ) { + found.push({ line: at(node), text: text(node), why: 'increment or decrement at module scope' }); + + return; + } + break; + default: + break; + } + + ts.forEachChild(node, visit); + }; + + ts.forEachChild(sf, visit); + + return found; +} + +/** + * The audit's own tests. + * + * A tripwire that cannot fire is the same defect as no tripwire, and this one is easy to break in the + * silent direction: every one of the CASES below was reported clean by an earlier version of this script + * that recognised "safe" shapes instead of walking for executable ones. They are pinned here so that + * cannot come back unnoticed. + */ +const CASES = [ + { src: "import './register.js';", flagged: true, what: 'side-effect import' }, + { src: 'export default initialize();', flagged: true, what: 'call in a default export' }, + { src: 'class C extends initialize() {}', flagged: true, what: 'call in a heritage clause' }, + { src: 'const x = { ...initialize() };', flagged: true, what: 'call in an object spread' }, + { src: 'const x = `${initialize()}`;', flagged: true, what: 'call in a template interpolation' }, + { src: 'class D { [initialize()] = 1; }', flagged: true, what: 'call in a computed member name' }, + { src: 'class E { static p = initialize(); }', flagged: true, what: 'call in a static field initialiser' }, + { src: 'class F { static { initialize(); } }', flagged: true, what: 'static initialisation block' }, + { src: 'globalThis.patched = true;', flagged: true, what: 'assignment to a global' }, + { src: 'const x = tag`text`;', flagged: true, what: 'tagged template' }, + { src: 'const x = await ready();', flagged: true, what: 'top-level await' }, + { src: 'const x = new Thing();', flagged: true, what: 'construction of an unknown class' }, + { src: 'delete globalThis.x;', flagged: true, what: 'delete' }, + { src: 'counter++;', flagged: true, what: 'increment' }, + + { src: 'export function a(x = compute()) { return initialize(); }', flagged: false, what: 'a function body and its parameter defaults' }, + { src: 'export const b = () => initialize();', flagged: false, what: 'an arrow body' }, + { src: 'class G { p = initialize(); m() { return initialize(); } get g() { return initialize(); } }', flagged: false, what: 'instance fields and method bodies' }, + { src: "import { readFileSync } from 'node:fs';", flagged: false, what: 'an import with bindings' }, + { src: "const s = new Set(['a']); const r = new RegExp('x');", flagged: false, what: 'value-constructing built-ins' }, + { src: 'const o = Object.freeze({ a: Object.freeze([1, 2]) });', flagged: false, what: 'freezing a fresh literal' }, + { src: 'const o = { k: 1, n: { d: [1, 2] } };', flagged: false, what: 'plain data' }, + { src: 'const t = `plain ${name} text`;', flagged: false, what: 'a template over an identifier' }, + + { src: 'Object.freeze(someImportedThing);', flagged: true, what: 'freezing something others can see' }, +]; + +if (process.argv.includes('--selftest')) { + let failures = 0; + for (const { src, flagged, what } of CASES) { + const sf = ts.createSourceFile('case.js', src, ts.ScriptTarget.ESNext, true); + const got = findExecutable(sf).length > 0; + const ok = got === flagged; + if (!ok) failures++; + console.log(` ${ok ? 'ok ' : 'FAIL'} ${flagged ? 'flags ' : 'allows '} ${what}`); } + console.log(failures === 0 ? `\n all ${CASES.length} audit cases behave as specified.` : `\n ${failures} case(s) wrong.`); + process.exit(failures === 0 ? 0 : 1); } let bad = 0; -for (const file of process.argv.slice(2)) { - if (!existsSync(file)) { console.log(` ${file}: MISSING`); bad++; continue; } +const files = process.argv.slice(2); + +if (files.length === 0) { + console.error('usage: node scripts/side-effect-audit.mjs [file ...]'); + process.exit(2); +} + +for (const file of files) { + if (!existsSync(file)) { + console.log(` ${file}: MISSING`); + bad++; + + continue; + } const src = readFileSync(file, 'utf8'); const sf = ts.createSourceFile(file, src, ts.ScriptTarget.ESNext, true); - const effects = []; - for (const st of sf.statements) { - if (DECL.has(st.kind)) continue; - if (ts.isVariableStatement(st)) { - for (const d of st.declarationList.declarations) { - if (!initialiserIsPure(d.initializer)) { - effects.push({ line: sf.getLineAndCharacterOfPosition(d.getStart()).line + 1, text: d.getText().slice(0, 110) }); - } - } - continue; - } - effects.push({ line: sf.getLineAndCharacterOfPosition(st.getStart()).line + 1, text: st.getText().slice(0, 110).replace(/\n/g, ' ') }); + const effects = findExecutable(sf); + console.log(`\n ${file} (${(src.length / 1024).toFixed(0)} kB, ${sf.statements.length} top-level statements)`); + + if (effects.length === 0) { + console.log(' nothing that executes at module scope'); + + continue; + } + bad += effects.length; + // Capped, because a CommonJS bundle produces a long and uninformative list and the first few say it. + for (const e of effects.slice(0, 12)) { + console.log(` L${String(e.line).padEnd(6)} ${e.why}`); + console.log(` ${e.text}`); } - const kb = (src.length / 1024).toFixed(0); - console.log(`\n ${file} (${kb} kB, ${sf.statements.length} top-level statements)`); - if (effects.length === 0) console.log(' no import-time side effects'); - else { bad += effects.length; for (const e of effects) console.log(` L${String(e.line).padEnd(6)} ${e.text}`); } + if (effects.length > 12) console.log(` … and ${effects.length - 12} more`); } + process.exit(bad > 0 ? 1 : 0); diff --git a/tests/source-maps.test.ts b/tests/source-maps.test.ts new file mode 100644 index 00000000..eb041585 --- /dev/null +++ b/tests/source-maps.test.ts @@ -0,0 +1,133 @@ +import { describe, it, expect } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * The published source maps: present, usable, and without the source text embedded. + * + * Maps are what turn a customer's stack trace into a file and a line. Dropping the embedded + * `sourcesContent` keeps that and roughly quarters the published package, because the source text it + * duplicates is already public in this repository at the tag matching the version the CLI reports. + * + * Two separate build steps produce maps — `tsup.config.ts` for the three main artifacts and + * `scripts/build-edge.mjs` for the edge bundle — so the setting exists in two places and can be added to + * one. These assertions are over the ARTIFACTS rather than the configuration for that reason: a new entry + * point added to either builder is covered without anyone remembering to update a list. + */ +const root = fileURLToPath(new URL('..', import.meta.url)); +const dist = join(root, 'dist'); + +// `dist/` is gitignored and built on publish, so a plain checkout has nothing to assert against. CI builds +// before it tests, which is what makes these run there rather than returning green having checked nothing. +const built = existsSync(dist); + +const manifest = JSON.parse(readFileSync(join(root, 'package.json'), 'utf8')); + +describe.skipIf(!built)('published source maps', () => { + // Everything in `dist/` ships, so enumerating it is the same set as enumerating the tarball — without + // running `npm pack`, which would rebuild. Pinned so the equivalence stops holding loudly. + it('ships the whole of dist, which is what makes the checks below equivalent to the tarball', () => { + expect(manifest.files).toContain('dist'); + }); + + const artifacts = readdirSync(dist).filter((f) => /\.(js|cjs)$/.test(f)); + const maps = readdirSync(dist).filter((f) => f.endsWith('.map')); + + it('has artifacts and maps to check', () => { + // A guard against the whole suite passing vacuously if the build layout changes. + expect(artifacts.length).toBeGreaterThan(3); + expect(maps.length).toBeGreaterThan(3); + }); + + it('gives every published JavaScript artifact a map', () => { + // Including the chunks. A stack frame landing in an unmapped chunk is the case where a report says + // "somewhere in the bundle", which is the state maps exist to avoid. + const unmapped = artifacts.filter((f) => !existsSync(join(dist, `${f}.map`))); + + expect(unmapped).toEqual([]); + }); + + it('points each artifact at its map', () => { + // A map on disk that nothing references is not a map anybody will use. + const missingComment = artifacts.filter( + (f) => !/# sourceMappingURL=.+\.map\s*$/.test(readFileSync(join(dist, f), 'utf8').trimEnd()), + ); + + expect(missingComment).toEqual([]); + }); + + it('gives every map real mappings and real sources', () => { + // The failure this catches is a map that exists and resolves nothing — indistinguishable from a + // working map until someone needs it. + const broken = maps.filter((f) => { + const map = JSON.parse(readFileSync(join(dist, f), 'utf8')); + + return ( + typeof map.mappings !== 'string' || + map.mappings.length === 0 || + !Array.isArray(map.sources) || + map.sources.length === 0 + ); + }); + + expect(broken).toEqual([]); + }); + + it('embeds no source text in any map', () => { + // The decision being enforced. Asserted over every map because the setting lives in two build files: + // adding an entry point to one builder and not the other would otherwise reintroduce it quietly. + const withSources = maps.filter((f) => { + const map = JSON.parse(readFileSync(join(dist, f), 'utf8')); + + return map.sourcesContent !== undefined; + }); + + expect(withSources).toEqual([]); + }); + + it('resolves a real stack frame to the source file and line that threw', () => { + // The check that matters, and the only one that would notice a map whose mappings are present but + // wrong. `resolveConfig` rejects a malformed site UUID by throwing from `src/config.ts` through a + // public export, so this is a frame a support conversation could actually produce. + // + // The assertion is not on a line NUMBER — that would break on any edit above the throw, and pinning it + // would say nothing about accuracy. Instead the resolved line is read back out of the source, so the + // map has to land on the statement that actually threw. + const dir = mkdtempSync(join(tmpdir(), 'ps-maps-')); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ type: 'module' })); + writeFileSync( + join(dir, 'probe.mjs'), + `import { resolveConfig } from ${JSON.stringify(join(dist, 'index.js'))};\n` + + `try {\n` + + ` await resolveConfig({ cwd: ${JSON.stringify(dir)}, cliSiteUuid: 'not-a-uuid' });\n` + + ` console.log('NO_THROW');\n` + + `} catch (error) {\n` + + ` console.log(error.stack);\n` + + `}\n`, + ); + + // Node's own source-map consumer, so what is tested is the map working for the tool a reporter uses. + const run = spawnSync(process.execPath, ['--enable-source-maps', join(dir, 'probe.mjs')], { + encoding: 'utf8', + }); + rmSync(dir, { recursive: true, force: true }); + + expect(run.stdout).not.toContain('NO_THROW'); + + const frame = run.stdout.split('\n').find((line) => line.includes('src/config.ts')); + expect(frame, `no src/config.ts frame in:\n${run.stdout}\n${run.stderr}`).toBeDefined(); + + const [, lineNumber] = /src\/config\.ts:(\d+):/.exec(frame ?? '') ?? []; + expect(lineNumber).toBeDefined(); + + const source = readFileSync(join(root, 'src', 'config.ts'), 'utf8').split('\n'); + // The resolved line, plus the one after it: the throw spans several lines and the frame may point at + // either the `throw` or the constructor call on it. + const resolved = `${source[Number(lineNumber) - 1]}${source[Number(lineNumber)] ?? ''}`; + + expect(resolved).toContain('PatchstackError'); + }); +}); diff --git a/tsup.config.ts b/tsup.config.ts index b50bdfdc..8f13a714 100644 --- a/tsup.config.ts +++ b/tsup.config.ts @@ -1,5 +1,20 @@ import { defineConfig } from 'tsup'; +/** + * Maps are published; the source text inside them is not. + * + * A `.map` without `sourcesContent` still resolves a stack frame to the original file and line, which is + * what a support conversation needs. Embedding the source text as well roughly doubles the published + * package for something already public: this repository is open, and the version a consumer is running is + * recorded accurately, so the file and line resolve against the matching tag. + * + * `scripts/build-edge.mjs` builds the edge bundle with its own esbuild call and has to set this too. + * `tests/source-maps.test.ts` asserts it over every published artifact rather than trusting either. + */ +const noEmbeddedSources = (options: { sourcesContent?: boolean }): void => { + options.sourcesContent = false; +}; + export default defineConfig([ { entry: { index: 'src/index.ts' }, @@ -7,6 +22,7 @@ export default defineConfig([ dts: true, clean: true, sourcemap: true, + esbuildOptions: noEmbeddedSources, target: 'node18', }, { @@ -14,6 +30,7 @@ export default defineConfig([ format: ['esm'], clean: false, sourcemap: true, + esbuildOptions: noEmbeddedSources, target: 'node18', banner: { js: '#!/usr/bin/env node' }, // `map` parses the target app's source with a TypeScript compiler resolved at RUNTIME (the app's @@ -28,6 +45,7 @@ export default defineConfig([ format: ['esm', 'cjs'], clean: false, sourcemap: true, + esbuildOptions: noEmbeddedSources, target: 'node18', }, ]); From 720e54757257a940b26fdbed26d5d5b9b1dee7aa Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 13:19:34 +0200 Subject: [PATCH 3/6] Report what every emitted artifact executes, and stop calling it a check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit missed four kinds of import-time execution: a top-level `throw`, a built-in consuming a value from elsewhere (`new Set(imported)` walks its iterator), a spread of such a value (which runs its getters), and imports with bindings — which evaluate the target module whatever this script can see. It also audited three named entries out of eight emitted files, leaving the chunks, the bin and both CommonJS bundles unexamined. All four are now recognised and every emitted artifact is covered. Findings carry a severity so a bound import is neither treated as a finding nor declared safe: it evaluates another module, and what that does is invisible here. Those are always printed, never hidden behind louder findings, because they are the part that is unknown. Local constants are resolved, one pass in source order, so `new Set(PHASES)` over a frozen array declared above it is not reported. Unresolved names stay foreign. Without this the report carried 52 findings, almost all false, and a report nobody reads is not a tripwire either. The exit code no longer speaks for what is found. Every stricter version was wrong: a CommonJS bundle executes at module scope by construction, `dist/cli.js` ends in `main().then(…)` because a bin is supposed to run, and a benign lazy-init call in a library entry is indistinguishable here from a harmful one. It fails only on a missing file or a failing self-test. The self-test is the gating half and now pins 31 cases across three outcomes. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 12 +- CONTRIBUTING.md | 27 ++- package.json | 2 +- scripts/side-effect-audit.mjs | 370 ++++++++++++++++++++++++---------- 4 files changed, 297 insertions(+), 114 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b30775a7..30992b6b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,13 +126,17 @@ jobs: # `sideEffects: false` is deliberately NOT declared in `package.json`; it was measured to buy zero # bytes on both consumer shapes, so declaring it would be a standing promise to bundlers with - # nothing bought for it. The audit is a tripwire that keeps that decision reviewable — it reports - # what executes at import time and cannot prove purity. Its own behaviour is checked first, because - # a tripwire that cannot fire is the same defect as no tripwire. + # nothing bought for it. + # + # The self-test is the gating half: it checks the audit still recognises the constructs it is meant + # to, because a tripwire that cannot fire is the same defect as no tripwire. - name: Check the side-effect audit against its own cases run: node scripts/side-effect-audit.mjs --selftest - - name: Report anything the published bundles execute at import time + # The audit itself is a REPORT and exits zero on what it finds — a CommonJS bundle executes at module + # scope by construction and a bin is supposed to run, so there is no honest threshold to fail on. It + # is here to be read when someone considers declaring the field. + - name: Report what the emitted artifacts execute at import time run: npm run audit:side-effects - name: Bundle an edge guard and attack it diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2adfda01..a6f3b29e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -77,14 +77,25 @@ buy nothing while standing as a permanent promise to every bundler that no modul needs evaluating; the day someone adds a top-level registration, consumers lose it silently in their own build. If you want to add the field, add the evidence first: a shape where it demonstrably saves bytes. -`npm run audit:side-effects` supports that decision but does not settle it. It is a **tripwire**, not a -proof: it walks the part of each published bundle that runs at import time and reports anything that can -execute, deliberately over-reporting rather than recognising shapes it believes are safe. What it cannot -see is what a called function does, so a clean report means "nothing here obviously executes" and never -"safe to drop". `--selftest` pins its behaviour on 23 cases, several of which an earlier version of the -script reported clean — `export default init()`, `class C extends init()` and a bare `import './x.js'` all -look like plain declarations to an allowlist, and all three run code. The real evidence that the package -survives a bundler is `npm run test:bundled`, which attacks the bundled guard. +`npm run audit:side-effects` supports that decision but does not settle it, and it is a **report rather +than a check** — it exits zero on what it finds. It walks the part of every emitted artifact that runs at +import time and names anything that can execute, deliberately over-reporting rather than recognising +shapes it believes are safe. + +The exit code covers only what is unambiguously wrong — a missing file, or `--selftest` failing — because +every version of a stricter gate was wrong: a CommonJS bundle executes at module scope by construction, +`dist/cli.js` ends in `main().then(…)` because a bin is supposed to run, and a benign lazy-init call in a +library entry is indistinguishable here from a harmful one. + +`--selftest` is the part with teeth, and it is what CI gates on: 31 cases, many of which an earlier version +of the script reported clean. `export default init()`, `class C extends init()`, a bare `import './x.js'`, +a top-level `throw`, `new Set(imported)` and `{ ...imported }` all look like plain declarations to an +allowlist, and every one of them runs code. The list has three outcomes rather than two, because an import +with bindings is neither a finding nor safe: it evaluates another module, and this script cannot see what +that does. + +The real evidence that the package survives a bundler is `npm run test:bundled`, which attacks the bundled +guard rather than reading it. **`module` is kept even though nothing this repository tests reads it.** The bundled test shows removing it changes nothing, but that only means a bundler which understands `exports` ignores it — which is what diff --git a/package.json b/package.json index 5b3fb7e4..acdfa96b 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "test": "vitest run", "test:consumers": "node scripts/compat-matrix.mjs", "test:bundled": "node scripts/bundled-consumer.mjs", - "audit:side-effects": "npm run build && node scripts/side-effect-audit.mjs dist/index.js dist/protect.js dist/protect.edge.js", + "audit:side-effects": "npm run build && node scripts/side-effect-audit.mjs --selftest && node -e \"const{readdirSync}=require('fs');console.log(readdirSync('dist').filter(f=>/\\\\.(js|cjs)$/.test(f)).map(f=>'dist/'+f).join(' '))\" | xargs node scripts/side-effect-audit.mjs", "test:manifest": "bun scripts/test-manifest.ts", "test:watch": "vitest", "typecheck": "tsc --noEmit && node scripts/typecheck-templates.mjs", diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs index 1d0349e4..47a28957 100644 --- a/scripts/side-effect-audit.mjs +++ b/scripts/side-effect-audit.mjs @@ -1,28 +1,30 @@ -// Does `sideEffects: false` tell a bundler the truth? +// What do the published bundles execute when they are merely imported? // -// That field is a promise: if a consumer imports nothing from a module, the bundler may drop the module -// without evaluating it. Declaring it when it is false does not produce a build error — it produces an -// application missing a side effect it depended on, at run time, in the consumer's build and not ours. -// For a package whose job is to be present and screening requests, "silently not there" is the worst -// available failure, so the field is only defensible with evidence behind it. +// The question exists because of `sideEffects: false`. That field is a promise: if a consumer imports +// nothing from a module, the bundler may drop the module without evaluating it. Declaring it when it is +// false does not produce a build error — it produces an application missing a side effect it depended on, +// at run time, in the consumer's build and not ours. For a package whose job is to be present and +// screening requests, "silently not there" is the worst available failure. // -// This is a TRIPWIRE, not a proof. It reports constructs that execute when a module is evaluated, and it -// is deliberately built to over-report: it walks the region of the AST that runs at import time and flags -// anything that can execute, rather than recognising a list of shapes it believes are safe. An allowlist -// gets this wrong in the dangerous direction — `export default init()`, `class C extends init()` and -// `import './register.js'` all look like plain declarations to one, and all three run code. +// This is a REPORT, not a verdict, and the distinction is load-bearing rather than modest. It walks the +// region of the AST that runs at import time and names anything that can execute, deliberately +// over-reporting rather than recognising shapes it believes are safe: an allowlist gets this wrong in the +// dangerous direction, and an earlier version of this script called `export default init()`, +// `class C extends init()` and a bare `import './register.js'` all clean. // -// What it cannot do is prove purity. It sees one bundle's syntax, not what a called function does, and a -// clean report means "nothing here obviously executes", never "this module is safe to drop". The evidence -// that the package survives a bundler is `npm run test:bundled`, which attacks the bundled guard. +// What it cannot do is prove purity. It sees syntax, not behaviour: a call it reports may be harmless and +// a call it accepts may not be. Read it before declaring `sideEffects: false`. Do not rely on it instead. +// The evidence that this package survives a bundler is `npm run test:bundled`, which attacks the bundled +// guard rather than reading it. // -// Run as `npm run audit:side-effects` after a build. +// `npm run audit:side-effects` runs the self-test and then reports on every emitted artifact. import ts from 'typescript'; import { readFileSync, existsSync } from 'node:fs'; /** - * Constructors that build a value and touch nothing else. Everything else, including any call, is - * reported — the audit does not try to decide whether a function is pure. + * Constructors that build a value and touch nothing else — PROVIDED their arguments are local literals. + * `new Set([1, 2])` builds a set; `new Set(somethingFromElsewhere)` walks an iterator that may be + * somebody else's code. */ const PURE_CONSTRUCTORS = new Set(['Set', 'Map', 'WeakSet', 'WeakMap', 'RegExp', 'Error', 'Date', 'URL']); @@ -36,24 +38,105 @@ const PURE_CONSTRUCTORS = new Set(['Set', 'Map', 'WeakSet', 'WeakMap', 'RegExp', */ const LAZY_WRAPPERS = new Set(['__commonJS', '__esm']); +/** + * Whether an expression is built here and now, out of things this file can see. + * + * The distinction matters wherever a value is CONSUMED rather than merely referenced: a spread runs an + * iterator or a getter, and so does a built-in over a non-literal. Both are code, and if the value came + * from an import then it is code this file cannot see. + * + * `locals` carries the module-scope names already proven to hold local literals — see + * `localLiteralBindings`. Anything unresolved is treated as foreign, which is the conservative direction. + */ +function isLocalLiteral(node, locals = new Map()) { + if (node === undefined) return true; + switch (node.kind) { + case ts.SyntaxKind.StringLiteral: + case ts.SyntaxKind.NumericLiteral: + case ts.SyntaxKind.BigIntLiteral: + case ts.SyntaxKind.TrueKeyword: + case ts.SyntaxKind.FalseKeyword: + case ts.SyntaxKind.NullKeyword: + case ts.SyntaxKind.RegularExpressionLiteral: + case ts.SyntaxKind.NoSubstitutionTemplateLiteral: + return true; + case ts.SyntaxKind.Identifier: + // A module-scope name in THIS file bound to a local literal is itself a local literal. An import, a + // parameter, or a name bound to a call never enters the map and so stays foreign. Bundles are full of + // `new Set(PHASES)` over a frozen array declared above it, and reporting those makes the report + // unreadable — which is its own way of not being a tripwire. + return locals.get(node.text) === true; + case ts.SyntaxKind.ArrayLiteralExpression: + return node.elements.every((e) => isLocalLiteral(e, locals)); + case ts.SyntaxKind.ObjectLiteralExpression: + return node.properties.every( + (prop) => + !ts.isSpreadAssignment(prop) && + (!('initializer' in prop) || prop.initializer === undefined || isLocalLiteral(prop.initializer, locals)), + ); + case ts.SyntaxKind.BinaryExpression: + // String building out of local pieces, which is how a bundler emits a composed pattern. + return ( + node.operatorToken.kind === ts.SyntaxKind.PlusToken && + isLocalLiteral(node.left, locals) && + isLocalLiteral(node.right, locals) + ); + case ts.SyntaxKind.ParenthesizedExpression: + return isLocalLiteral(node.expression, locals); + case ts.SyntaxKind.CallExpression: + // The one exempt call form, so the exemption composes with itself: the engine's constant tables are + // written as `Object.freeze({ … Object.freeze([…]) … })` several levels deep. + return isFreezeOfLiteral(node, locals); + default: + return false; + } +} + /** * `Object.freeze()` — the one call shape this audit accepts, and only over an argument that is * constructed in place. * * The exemption is provable rather than assumed: freezing an object or array literal that no other code * has a reference to yet cannot be observed from anywhere else, so evaluating it has no effect a bundler - * could drop. Nesting is handled by the ordinary recursion, so `Object.freeze(Object.freeze([…]))` is - * accepted for the same reason at each level. + * could drop. + * + * `Object.freeze(someImportedThing)` is NOT accepted: that mutates an object someone else can see. + * Neither is `Object.freeze({ ...imported })`, which runs somebody else's getters before it freezes + * anything — the freeze being harmless says nothing about the spread. * - * `Object.freeze(someImportedThing)` is NOT accepted: that mutates an object someone else can see, which - * is exactly the kind of import-time effect this exists to find. + * Mutually recursive with `isLocalLiteral`; both are hoisted declarations, so the order here is free. */ -function isFreezeOfLiteral(node) { +function isFreezeOfLiteral(node, locals = new Map()) { if (node.expression.getText() !== 'Object.freeze') return false; const [arg, ...rest] = node.arguments ?? []; - return rest.length === 0 && arg !== undefined - && (ts.isObjectLiteralExpression(arg) || ts.isArrayLiteralExpression(arg)); + return ( + rest.length === 0 && + arg !== undefined && + (ts.isObjectLiteralExpression(arg) || ts.isArrayLiteralExpression(arg)) && + isLocalLiteral(arg, locals) + ); +} + +/** + * Module-scope names in this file that are provably bound to a local literal. + * + * One pass, in source order, so a constant built from constants above it resolves while anything else is + * simply left out. No ordering or reassignment analysis: a name is either provably a local literal or it + * is not, and everything unproven is treated as foreign by the caller. + */ +function localLiteralBindings(sf) { + const locals = new Map(); + + for (const st of sf.statements) { + if (!ts.isVariableStatement(st)) continue; + for (const d of st.declarationList.declarations) { + if (!ts.isIdentifier(d.name)) continue; + locals.set(d.name.text, isLocalLiteral(d.initializer, locals)); + } + } + + return locals; } /** @@ -65,11 +148,18 @@ function isFreezeOfLiteral(node) { * * A class is the case worth naming: its method bodies do not run, but its `extends` clause, its computed * member names, its static field initialisers and its static blocks all do. + * + * Findings carry a severity. `executes` is code in this file. `elsewhere` is an import: it evaluates + * another module, and what that evaluation does is not visible from here — reported rather than accepted, + * because in a bundle the remaining imports are externals and treating them as findings would drown the + * report, while calling them safe would be a claim this cannot support. */ function findExecutable(sf) { const found = []; + const locals = localLiteralBindings(sf); const at = (node) => sf.getLineAndCharacterOfPosition(node.getStart()).line + 1; const text = (node) => node.getText().slice(0, 100).replace(/\s+/g, ' '); + const flag = (node, why, severity = 'executes') => found.push({ line: at(node), text: text(node), why, severity }); const visit = (node) => { // --- Regions that are NOT evaluated when the module is. @@ -83,8 +173,7 @@ function findExecutable(sf) { ts.isConstructorDeclaration(node) ) { // The body does not run at evaluation time, and neither do default parameter values — those are - // evaluated per call. A COMPUTED NAME does run, because the key has to exist before the object or - // class does. + // evaluated per call. A COMPUTED NAME does run, because the key has to exist before the class does. if (node.name && ts.isComputedPropertyName(node.name)) visit(node.name.expression); return; @@ -101,82 +190,96 @@ function findExecutable(sf) { // --- Constructs that DO execute at evaluation time. switch (node.kind) { - case ts.SyntaxKind.ImportDeclaration: { - // A side-effect import — `import './register.js'` with no bindings — exists only to run another - // module. An import WITH bindings is a dependency edge rather than a statement of its own, and is - // reported separately as context. - if (node.importClause === undefined) { - found.push({ line: at(node), text: text(node), why: 'side-effect import: runs another module' }); - } + case ts.SyntaxKind.ImportDeclaration: + // Both forms evaluate the target module. A side-effect import — no bindings — exists ONLY to do + // that, so it is a finding outright; one with bindings is reported at `elsewhere`. + flag( + node, + node.importClause === undefined + ? 'side-effect import: exists only to run another module' + : 'imports and therefore evaluates another module, whose contents are not visible here', + node.importClause === undefined ? 'executes' : 'elsewhere', + ); + + return; + case ts.SyntaxKind.ThrowStatement: + flag(node, 'throw at module scope: evaluating this module fails'); return; - } case ts.SyntaxKind.CallExpression: { const callee = node.expression.getText(); - if (isFreezeOfLiteral(node)) { - // Keep walking the literal: the freeze is accepted, what is inside it may not be. + if (isFreezeOfLiteral(node, locals)) { + // Accepted, but keep walking the literal: what is inside it may not be. for (const a of node.arguments ?? []) visit(a); return; } if (LAZY_WRAPPERS.has(callee)) { - // The wrapper does not run its argument, but the argument may still contain code that runs for - // other reasons, so keep walking it. + // The wrapper does not run its argument, but the argument may contain code that runs for other + // reasons, so keep walking it. for (const a of node.arguments ?? []) visit(a); return; } - found.push({ line: at(node), text: text(node), why: `call at module scope: ${callee.slice(0, 40)}` }); + flag(node, `call at module scope: ${callee.slice(0, 40)}`); return; } case ts.SyntaxKind.NewExpression: { const ctor = node.expression.getText(); if (!PURE_CONSTRUCTORS.has(ctor)) { - found.push({ line: at(node), text: text(node), why: `construction at module scope: new ${ctor.slice(0, 40)}` }); + flag(node, `construction at module scope: new ${ctor.slice(0, 40)}`); + + return; + } + if ((node.arguments ?? []).some((a) => !isLocalLiteral(a, locals))) { + flag(node, `new ${ctor} over a value from elsewhere: may run its iterator or getters`); return; } break; } + case ts.SyntaxKind.SpreadElement: + case ts.SyntaxKind.SpreadAssignment: + // `[...x]` walks x's iterator and `{ ...x }` runs x's getters. Both are code, and if x came from + // elsewhere it is not code this file can see. + if (!isLocalLiteral(node.expression, locals)) { + flag(node, 'spread of a value from elsewhere: runs its iterator or getters'); + + return; + } + break; case ts.SyntaxKind.TaggedTemplateExpression: - found.push({ line: at(node), text: text(node), why: 'tagged template: invokes the tag function' }); + flag(node, 'tagged template: invokes the tag function'); return; case ts.SyntaxKind.AwaitExpression: - found.push({ line: at(node), text: text(node), why: 'top-level await' }); + flag(node, 'top-level await'); return; case ts.SyntaxKind.ClassStaticBlockDeclaration: - found.push({ line: at(node), text: text(node), why: 'static initialisation block' }); + flag(node, 'static initialisation block'); return; case ts.SyntaxKind.DeleteExpression: - found.push({ line: at(node), text: text(node), why: 'delete: mutates an object' }); + flag(node, 'delete: mutates an object'); return; - case ts.SyntaxKind.BinaryExpression: + case ts.SyntaxKind.BinaryExpression: { // Assignment to anything reachable from outside — `module.exports = …`, `globalThis.x = …`. - if ( - ts.isPropertyAccessExpression(node.left) || - ts.isElementAccessExpression(node.left) - ) { + if (ts.isPropertyAccessExpression(node.left) || ts.isElementAccessExpression(node.left)) { const op = node.operatorToken.kind; const assigns = op === ts.SyntaxKind.EqualsToken || (op >= ts.SyntaxKind.FirstCompoundAssignment && op <= ts.SyntaxKind.LastCompoundAssignment); - if (assigns) { - found.push({ line: at(node), text: text(node), why: 'assignment to a property at module scope' }); - } + if (assigns) flag(node, 'assignment to a property at module scope'); } break; + } case ts.SyntaxKind.PrefixUnaryExpression: case ts.SyntaxKind.PostfixUnaryExpression: - if ( - node.operator === ts.SyntaxKind.PlusPlusToken || - node.operator === ts.SyntaxKind.MinusMinusToken - ) { - found.push({ line: at(node), text: text(node), why: 'increment or decrement at module scope' }); + if (node.operator === ts.SyntaxKind.PlusPlusToken || node.operator === ts.SyntaxKind.MinusMinusToken) { + flag(node, 'increment or decrement at module scope'); return; } @@ -194,86 +297,151 @@ function findExecutable(sf) { } /** - * The audit's own tests. + * The audit's own tests, and the only part of this script with teeth. * * A tripwire that cannot fire is the same defect as no tripwire, and this one is easy to break in the - * silent direction: every one of the CASES below was reported clean by an earlier version of this script - * that recognised "safe" shapes instead of walking for executable ones. They are pinned here so that - * cannot come back unnoticed. + * silent direction: many of the cases below were reported clean by an earlier version that recognised + * "safe" shapes instead of walking for executable ones. They are pinned so that cannot come back + * unnoticed. + * + * Three outcomes, not two. `elsewhere` exists so that a bound import is neither treated as a finding nor + * declared safe — importing evaluates the target, and this script cannot see what that does. */ const CASES = [ - { src: "import './register.js';", flagged: true, what: 'side-effect import' }, - { src: 'export default initialize();', flagged: true, what: 'call in a default export' }, - { src: 'class C extends initialize() {}', flagged: true, what: 'call in a heritage clause' }, - { src: 'const x = { ...initialize() };', flagged: true, what: 'call in an object spread' }, - { src: 'const x = `${initialize()}`;', flagged: true, what: 'call in a template interpolation' }, - { src: 'class D { [initialize()] = 1; }', flagged: true, what: 'call in a computed member name' }, - { src: 'class E { static p = initialize(); }', flagged: true, what: 'call in a static field initialiser' }, - { src: 'class F { static { initialize(); } }', flagged: true, what: 'static initialisation block' }, - { src: 'globalThis.patched = true;', flagged: true, what: 'assignment to a global' }, - { src: 'const x = tag`text`;', flagged: true, what: 'tagged template' }, - { src: 'const x = await ready();', flagged: true, what: 'top-level await' }, - { src: 'const x = new Thing();', flagged: true, what: 'construction of an unknown class' }, - { src: 'delete globalThis.x;', flagged: true, what: 'delete' }, - { src: 'counter++;', flagged: true, what: 'increment' }, - - { src: 'export function a(x = compute()) { return initialize(); }', flagged: false, what: 'a function body and its parameter defaults' }, - { src: 'export const b = () => initialize();', flagged: false, what: 'an arrow body' }, - { src: 'class G { p = initialize(); m() { return initialize(); } get g() { return initialize(); } }', flagged: false, what: 'instance fields and method bodies' }, - { src: "import { readFileSync } from 'node:fs';", flagged: false, what: 'an import with bindings' }, - { src: "const s = new Set(['a']); const r = new RegExp('x');", flagged: false, what: 'value-constructing built-ins' }, - { src: 'const o = Object.freeze({ a: Object.freeze([1, 2]) });', flagged: false, what: 'freezing a fresh literal' }, - { src: 'const o = { k: 1, n: { d: [1, 2] } };', flagged: false, what: 'plain data' }, - { src: 'const t = `plain ${name} text`;', flagged: false, what: 'a template over an identifier' }, - - { src: 'Object.freeze(someImportedThing);', flagged: true, what: 'freezing something others can see' }, + { src: "import './register.js';", expect: 'executes', what: 'side-effect import' }, + { src: 'export default initialize();', expect: 'executes', what: 'call in a default export' }, + { src: 'class C extends initialize() {}', expect: 'executes', what: 'call in a heritage clause' }, + { src: 'const x = { ...initialize() };', expect: 'executes', what: 'call in an object spread' }, + { src: 'const x = `${initialize()}`;', expect: 'executes', what: 'call in a template interpolation' }, + { src: 'class D { [initialize()] = 1; }', expect: 'executes', what: 'call in a computed member name' }, + { src: 'class E { static p = initialize(); }', expect: 'executes', what: 'call in a static field initialiser' }, + { src: 'class F { static { initialize(); } }', expect: 'executes', what: 'static initialisation block' }, + { src: 'globalThis.patched = true;', expect: 'executes', what: 'assignment to a global' }, + { src: 'const x = tag`text`;', expect: 'executes', what: 'tagged template' }, + { src: 'const x = await ready();', expect: 'executes', what: 'top-level await' }, + { src: 'const x = new Thing();', expect: 'executes', what: 'construction of an unknown class' }, + { src: 'delete globalThis.x;', expect: 'executes', what: 'delete' }, + { src: 'counter++;', expect: 'executes', what: 'increment' }, + { src: "throw new Error('boom');", expect: 'executes', what: 'throw at module scope' }, + { src: 'const s = new Set(importedIterable);', expect: 'executes', what: 'a built-in consuming a value from elsewhere' }, + { src: 'const a = [...importedIterable];', expect: 'executes', what: 'array spread of a value from elsewhere' }, + { src: 'const o = { ...importedObject };', expect: 'executes', what: 'object spread of a value from elsewhere' }, + { src: 'const o = Object.freeze({ ...importedObject });', expect: 'executes', what: 'freezing a spread of a value from elsewhere' }, + { src: 'Object.freeze(someImportedThing);', expect: 'executes', what: 'freezing something others can see' }, + + // NOT 'clean'. Importing evaluates the target, and an earlier version of this list called that safe. + { src: "import { readFileSync } from 'node:fs';", expect: 'elsewhere', what: 'an import with bindings' }, + + { src: 'export function a(x = compute()) { return initialize(); }', expect: 'clean', what: 'a function body and its parameter defaults' }, + { src: 'export const b = () => initialize();', expect: 'clean', what: 'an arrow body' }, + { src: 'class G { p = initialize(); m() { return initialize(); } get g() { return initialize(); } }', expect: 'clean', what: 'instance fields and method bodies' }, + { src: "const s = new Set(['a', 'b']);", expect: 'clean', what: 'a built-in over its own literal' }, + { src: 'const a = [...[1, 2]];', expect: 'clean', what: 'spread of a literal' }, + { src: 'const o = Object.freeze({ a: Object.freeze([1, 2]) });', expect: 'clean', what: 'freezing a fresh literal, nested' }, + { src: "const P = ['a', 'b']; const s = new Set(P);", expect: 'clean', what: 'a built-in over a local constant' }, + { src: "const G = 'x'; const r = new RegExp('(' + G + ')');", expect: 'clean', what: 'a pattern composed from local constants' }, + { src: 'const o = { k: 1, n: { d: [1, 2] } };', expect: 'clean', what: 'plain data' }, + { src: 'const t = `plain ${name} text`;', expect: 'clean', what: 'a template over an identifier' }, ]; if (process.argv.includes('--selftest')) { let failures = 0; - for (const { src, flagged, what } of CASES) { + const label = { executes: 'flags ', elsewhere: 'notes ', clean: 'allows ' }; + + for (const { src, expect: wanted, what } of CASES) { const sf = ts.createSourceFile('case.js', src, ts.ScriptTarget.ESNext, true); - const got = findExecutable(sf).length > 0; - const ok = got === flagged; + const found = findExecutable(sf); + const got = found.some((e) => e.severity === 'executes') + ? 'executes' + : found.length > 0 + ? 'elsewhere' + : 'clean'; + const ok = got === wanted; if (!ok) failures++; - console.log(` ${ok ? 'ok ' : 'FAIL'} ${flagged ? 'flags ' : 'allows '} ${what}`); + console.log(` ${ok ? 'ok ' : 'FAIL'} ${label[wanted]} ${what}${ok ? '' : ` — got ${got}`}`); } - console.log(failures === 0 ? `\n all ${CASES.length} audit cases behave as specified.` : `\n ${failures} case(s) wrong.`); + console.log( + failures === 0 + ? `\n all ${CASES.length} audit cases behave as specified.` + : `\n ${failures} case(s) wrong.`, + ); process.exit(failures === 0 ? 0 : 1); } -let bad = 0; const files = process.argv.slice(2); if (files.length === 0) { - console.error('usage: node scripts/side-effect-audit.mjs [file ...]'); + console.error('usage: node scripts/side-effect-audit.mjs [--selftest] [file ...]'); process.exit(2); } +/** + * This REPORTS. It does not pass or fail on what it finds. + * + * Which was tempting to do differently, and every version of that gate was wrong: + * + * - A CommonJS bundle executes at module scope by construction — `module.exports = …`, `require(…)`, + * definitions on the exports object. No version of it does not. + * - `dist/cli.js` ends in `main().then(…)`. It is a bin; executing on evaluation is its entire job. + * - The library entries are clean today, but a benign lazy-init call is indistinguishable here from a + * harmful one, so gating on them would mean either an allowlist — which is what made an earlier + * version of this script report `export default init()` as pure — or a red build for a call nobody + * needs to act on. + * + * So the exit code speaks only for what is unambiguously wrong: a file that does not exist, or the + * self-test failing. + */ +const isCommonJs = (file) => file.endsWith('.cjs'); + +let missing = 0; +let reported = 0; + for (const file of files) { if (!existsSync(file)) { console.log(` ${file}: MISSING`); - bad++; + missing++; continue; } const src = readFileSync(file, 'utf8'); const sf = ts.createSourceFile(file, src, ts.ScriptTarget.ESNext, true); const effects = findExecutable(sf); - console.log(`\n ${file} (${(src.length / 1024).toFixed(0)} kB, ${sf.statements.length} top-level statements)`); + const executes = effects.filter((e) => e.severity === 'executes'); + const elsewhere = effects.filter((e) => e.severity === 'elsewhere'); + + console.log( + `\n ${file} (${(src.length / 1024).toFixed(0)} kB, ${sf.statements.length} top-level statements)` + + `${isCommonJs(file) ? ' [CommonJS: executes by construction]' : ''}`, + ); - if (effects.length === 0) { - console.log(' nothing that executes at module scope'); + if (executes.length === 0) { + console.log( + elsewhere.length === 0 + ? ' nothing that executes at module scope' + : ` nothing that executes here; ${elsewhere.length} import(s) evaluate modules this cannot see`, + ); continue; } - bad += effects.length; + + reported += executes.length; // Capped, because a CommonJS bundle produces a long and uninformative list and the first few say it. - for (const e of effects.slice(0, 12)) { + for (const e of executes.slice(0, 10)) { console.log(` L${String(e.line).padEnd(6)} ${e.why}`); console.log(` ${e.text}`); } - if (effects.length > 12) console.log(` … and ${effects.length - 12} more`); + if (executes.length > 10) console.log(` … and ${executes.length - 10} more`); + // Never hidden behind the findings above. Imports are the part this script explicitly cannot see into, + // so leaving them out whenever there is something louder to print would understate what is unknown. + if (elsewhere.length > 0) { + console.log(` plus ${elsewhere.length} import(s) that evaluate modules this cannot see`); + } } -process.exit(bad > 0 ? 1 : 0); +console.log(`\n ${reported} construct(s) that execute at module scope, across ${files.length} artifact(s).`); +console.log(' This is a report, not a verdict. A clean line means nothing there obviously executes — not'); +console.log(' that a module is safe to drop, because what a called function does is invisible from one'); +console.log(" bundle's syntax. Read it before declaring `sideEffects: false`; do not rely on it instead."); + +// Only the unambiguous failure. See the note above `isCommonJs`. +process.exit(missing > 0 ? 1 : 0); From eeb2ccb19c8cffcbaf67bf0f7871a06c7bbfa674 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 13:29:21 +0200 Subject: [PATCH 4/6] Discover the audited artifacts in Node, not through xargs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The npm script piped a directory listing through `xargs`, which an ordinary Windows shell does not have — so a documented contributor command worked on Linux CI and failed on a Windows machine. The script now finds every emitted `.js` and `.cjs` under `dist/` itself when given no file arguments, and still accepts explicit paths to narrow it. A missing `dist/` says to build rather than reporting on nothing. Co-Authored-By: Claude Opus 5 (1M context) --- CONTRIBUTING.md | 6 +++--- package.json | 2 +- scripts/side-effect-audit.mjs | 31 ++++++++++++++++++++++++++----- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a6f3b29e..c7016d6a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -78,9 +78,9 @@ needs evaluating; the day someone adds a top-level registration, consumers lose build. If you want to add the field, add the evidence first: a shape where it demonstrably saves bytes. `npm run audit:side-effects` supports that decision but does not settle it, and it is a **report rather -than a check** — it exits zero on what it finds. It walks the part of every emitted artifact that runs at -import time and names anything that can execute, deliberately over-reporting rather than recognising -shapes it believes are safe. +than a check** — it exits zero on what it finds. It discovers every emitted `.js` and `.cjs` under `dist/` +itself, walks the part of each that runs at import time, and names anything that can execute, deliberately +over-reporting rather than recognising shapes it believes are safe. Pass file paths to narrow it. The exit code covers only what is unambiguously wrong — a missing file, or `--selftest` failing — because every version of a stricter gate was wrong: a CommonJS bundle executes at module scope by construction, diff --git a/package.json b/package.json index acdfa96b..77d177a1 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "test": "vitest run", "test:consumers": "node scripts/compat-matrix.mjs", "test:bundled": "node scripts/bundled-consumer.mjs", - "audit:side-effects": "npm run build && node scripts/side-effect-audit.mjs --selftest && node -e \"const{readdirSync}=require('fs');console.log(readdirSync('dist').filter(f=>/\\\\.(js|cjs)$/.test(f)).map(f=>'dist/'+f).join(' '))\" | xargs node scripts/side-effect-audit.mjs", + "audit:side-effects": "npm run build && node scripts/side-effect-audit.mjs --selftest && node scripts/side-effect-audit.mjs", "test:manifest": "bun scripts/test-manifest.ts", "test:watch": "vitest", "typecheck": "tsc --noEmit && node scripts/typecheck-templates.mjs", diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs index 47a28957..06ecf554 100644 --- a/scripts/side-effect-audit.mjs +++ b/scripts/side-effect-audit.mjs @@ -19,7 +19,8 @@ // // `npm run audit:side-effects` runs the self-test and then reports on every emitted artifact. import ts from 'typescript'; -import { readFileSync, existsSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; /** * Constructors that build a value and touch nothing else — PROVIDED their arguments are local literals. @@ -368,13 +369,33 @@ if (process.argv.includes('--selftest')) { process.exit(failures === 0 ? 0 : 1); } -const files = process.argv.slice(2); +/** + * Discover the emitted artifacts here rather than in the npm script. + * + * The alternative was a shell pipeline through `xargs`, which an ordinary Windows shell does not have — so + * a documented contributor command would have worked on Linux CI and failed on a Windows machine, which is + * the kind of thing nobody discovers until they are already stuck. + * + * Every `.js` and `.cjs` under `dist/`, chunks included: a stack frame landing in an unaudited chunk is the + * case where the report says nothing about the code that ran. + */ +function emittedArtifacts(dir = 'dist') { + if (!existsSync(dir)) { + console.error(` ${dir}/ does not exist — run \`npm run build\` first.`); + process.exit(2); + } -if (files.length === 0) { - console.error('usage: node scripts/side-effect-audit.mjs [--selftest] [file ...]'); - process.exit(2); + return readdirSync(dir) + .filter((f) => /\.(js|cjs)$/.test(f)) + .sort() + .map((f) => path.posix.join(dir, f)); } +// No file arguments means "everything that was emitted" — what the npm script wants, and what someone +// typing the command by hand almost certainly means too. +const named = process.argv.slice(2).filter((a) => !a.startsWith('--')); +const files = named.length > 0 ? named : emittedArtifacts(); + /** * This REPORTS. It does not pass or fail on what it finds. * From 34b4870ee41b2031ac01102fd70b1988e810ef96 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 13:39:16 +0200 Subject: [PATCH 5/6] Audit every emitted artifact, including the ones in subdirectories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discovery listed `dist/` without descending, so it found eight of fourteen files and reported on them as though that were the set. The six it missed are the scaffolder's guard templates in `dist/protect/templates/`, which are the files copied into a consumer's application and executed there. An artifact absent from the report is indistinguishable from one that reported clean, so a partial listing produces a report that looks complete. That is the defect this script exists to find, one level up. Discovery is now recursive, and zero artifacts is an error rather than a clean report: an empty or half-written build directory would otherwise print a summary saying nothing executes anywhere, which reads exactly like a pass. `tests/side-effect-audit-discovery.test.ts` drives the script against temporary directories it constructs — a nested artifact, no artifacts, a directory that does not exist — and checks the real report against an independent walk of `dist/`, so a new entry point cannot leave the audit behind. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/side-effect-audit.mjs | 47 ++++++++---- tests/side-effect-audit-discovery.test.ts | 93 +++++++++++++++++++++++ 2 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 tests/side-effect-audit-discovery.test.ts diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs index 06ecf554..16c35dc6 100644 --- a/scripts/side-effect-audit.mjs +++ b/scripts/side-effect-audit.mjs @@ -370,31 +370,50 @@ if (process.argv.includes('--selftest')) { } /** - * Discover the emitted artifacts here rather than in the npm script. + * Every `.js` and `.cjs` anywhere under `dir`, recursively. * - * The alternative was a shell pipeline through `xargs`, which an ordinary Windows shell does not have — so - * a documented contributor command would have worked on Linux CI and failed on a Windows machine, which is - * the kind of thing nobody discovers until they are already stuck. + * Recursive because the build emits into subdirectories: the scaffolder's guard templates live in + * `dist/protect/templates/`, and those are the files copied into a consumer's application and executed + * there. A non-recursive listing found eight of fourteen and reported on them as though that were the set, + * which is the failure this whole script is supposed to be looking for, one level up. * - * Every `.js` and `.cjs` under `dist/`, chunks included: a stack frame landing in an unaudited chunk is the - * case where the report says nothing about the code that ran. + * Done in Node rather than as a shell pipeline through `xargs`, which an ordinary Windows shell does not + * have — a documented contributor command that works on Linux CI and fails on Windows is the kind of thing + * nobody discovers until they are already stuck. */ -function emittedArtifacts(dir = 'dist') { +function emittedArtifacts(dir) { if (!existsSync(dir)) { console.error(` ${dir}/ does not exist — run \`npm run build\` first.`); process.exit(2); } - return readdirSync(dir) - .filter((f) => /\.(js|cjs)$/.test(f)) - .sort() - .map((f) => path.posix.join(dir, f)); + const out = []; + const walk = (current) => { + for (const entry of readdirSync(current, { withFileTypes: true })) { + const full = path.join(current, entry.name); + if (entry.isDirectory()) walk(full); + else if (/\.(js|cjs)$/.test(entry.name)) out.push(full.split(path.sep).join('/')); + } + }; + walk(dir); + + // Zero artifacts is not a clean report, it is the absence of one. An empty or half-written `dist/` would + // otherwise print a summary saying nothing executes anywhere, which reads exactly like a pass. + if (out.length === 0) { + console.error(` ${dir}/ contains no .js or .cjs files — nothing was audited. Run \`npm run build\`.`); + process.exit(2); + } + + return out.sort(); } // No file arguments means "everything that was emitted" — what the npm script wants, and what someone -// typing the command by hand almost certainly means too. -const named = process.argv.slice(2).filter((a) => !a.startsWith('--')); -const files = named.length > 0 ? named : emittedArtifacts(); +// typing the command by hand almost certainly means too. `--dir` exists so the tests can point this at a +// directory they control instead of the real build. +const args = process.argv.slice(2); +const named = args.filter((a) => !a.startsWith('--')); +const dirFlag = args.find((a) => a.startsWith('--dir=')); +const files = named.length > 0 ? named : emittedArtifacts(dirFlag?.slice('--dir='.length) ?? 'dist'); /** * This REPORTS. It does not pass or fail on what it finds. diff --git a/tests/side-effect-audit-discovery.test.ts b/tests/side-effect-audit-discovery.test.ts new file mode 100644 index 00000000..46e66b94 --- /dev/null +++ b/tests/side-effect-audit-discovery.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * What the side-effect audit looks at, and what it does when there is nothing to look at. + * + * Both matter more than they appear to. The audit's output is a list of files followed by a summary, and + * an artifact absent from that list is indistinguishable from one that reported clean — so a discovery + * step which quietly finds a subset produces a report that looks complete and is not. That is the same + * defect the audit itself exists to find, one level up. + * + * Driven against temporary directories rather than the real build, so the cases can be constructed rather + * than waited for: a nested artifact, and no artifacts at all. + */ +const script = fileURLToPath(new URL('../scripts/side-effect-audit.mjs', import.meta.url)); +const root = fileURLToPath(new URL('..', import.meta.url)); + +const run = (args: string[]) => + spawnSync(process.execPath, [script, ...args], { encoding: 'utf8', cwd: root }); + +/** The `dist/…` paths the report listed, which is exactly the set it spoke for. */ +const audited = (stdout: string): string[] => + stdout + .split('\n') + .map((line) => /^\s{2}(\S+)\s+\(\d+ kB/.exec(line)?.[1]) + .filter((v): v is string => v !== undefined); + +describe('what the audit discovers', () => { + it('finds artifacts in subdirectories, not just the top level', () => { + // The scaffolder's guard templates are emitted into `dist/protect/templates/` and are the files copied + // into a consumer's application. A non-recursive listing left every one of them out. + const dir = mkdtempSync(join(tmpdir(), 'ps-audit-')); + mkdirSync(join(dir, 'nested', 'deeper'), { recursive: true }); + writeFileSync(join(dir, 'top.js'), 'export const a = 1;\n'); + writeFileSync(join(dir, 'nested', 'middle.cjs'), 'module.exports = 1;\n'); + writeFileSync(join(dir, 'nested', 'deeper', 'bottom.js'), 'export const b = 2;\n'); + // Not JavaScript, and not to be reported as if it were. + writeFileSync(join(dir, 'nested', 'notes.md'), '# not code\n'); + writeFileSync(join(dir, 'nested', 'types.d.ts'), 'export declare const c: number;\n'); + + const result = run([`--dir=${dir}`]); + const found = audited(result.stdout).map((p) => p.slice(dir.length + 1)); + rmSync(dir, { recursive: true, force: true }); + + expect(found.sort()).toEqual(['nested/deeper/bottom.js', 'nested/middle.cjs', 'top.js']); + expect(result.status).toBe(0); + }); + + it('refuses to report on an empty directory instead of reporting nothing', () => { + // A summary saying nothing executes anywhere reads exactly like a pass. An empty or half-written build + // directory has to be an error, because the honest answer is that no artifact was examined. + const dir = mkdtempSync(join(tmpdir(), 'ps-audit-')); + + const result = run([`--dir=${dir}`]); + rmSync(dir, { recursive: true, force: true }); + + expect(result.status).not.toBe(0); + expect(`${result.stdout}${result.stderr}`).toContain('no .js or .cjs'); + }); + + it('refuses a directory that does not exist', () => { + const result = run(['--dir=/definitely/not/here']); + + expect(result.status).not.toBe(0); + expect(`${result.stdout}${result.stderr}`).toContain('does not exist'); + }); + + it('speaks for every emitted artifact of the real build', () => { + // The claim the report makes about this repository. Compared against an independent walk of `dist/` + // rather than against a remembered number, so adding an entry point cannot leave the audit behind. + const dist = join(root, 'dist'); + if (!existsSync(dist)) return; + + const expected: string[] = []; + const walk = (dir: string): void => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (/\.(js|cjs)$/.test(entry.name)) expected.push(full); + } + }; + walk(dist); + + const result = run([]); + const found = audited(result.stdout).map((p) => join(root, p)); + + expect(found.sort()).toEqual(expected.sort()); + }); +}); From f2e179676d13847ae41be05e8f986a94bf539b1f Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 13:53:05 +0200 Subject: [PATCH 6/6] State the invariants without narrating how they were arrived at Comments in the audit, its discovery tests and the contributor notes explained several decisions by describing what other arrangements did. The reasoning a reader needs is the invariant and why it holds; the rest belongs in private review notes under the public-repository policy. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- CONTRIBUTING.md | 21 ++++++++-------- scripts/side-effect-audit.mjs | 30 +++++++++++------------ tests/side-effect-audit-discovery.test.ts | 2 +- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c7016d6a..918a3dcb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,17 +82,16 @@ than a check** — it exits zero on what it finds. It discovers every emitted `. itself, walks the part of each that runs at import time, and names anything that can execute, deliberately over-reporting rather than recognising shapes it believes are safe. Pass file paths to narrow it. -The exit code covers only what is unambiguously wrong — a missing file, or `--selftest` failing — because -every version of a stricter gate was wrong: a CommonJS bundle executes at module scope by construction, -`dist/cli.js` ends in `main().then(…)` because a bin is supposed to run, and a benign lazy-init call in a -library entry is indistinguishable here from a harmful one. - -`--selftest` is the part with teeth, and it is what CI gates on: 31 cases, many of which an earlier version -of the script reported clean. `export default init()`, `class C extends init()`, a bare `import './x.js'`, -a top-level `throw`, `new Set(imported)` and `{ ...imported }` all look like plain declarations to an -allowlist, and every one of them runs code. The list has three outcomes rather than two, because an import -with bindings is neither a finding nor safe: it evaluates another module, and this script cannot see what -that does. +The exit code covers only what is unambiguously wrong — a missing file, an empty build directory, or +`--selftest` failing — because there is no honest threshold to fail on: a CommonJS bundle executes at module +scope by construction, `dist/cli.js` ends in `main().then(…)` because a bin is supposed to run, and a benign +lazy-init call in a library entry is indistinguishable here from a harmful one. + +`--selftest` is the part with teeth, and it is what CI gates on: 31 cases. `export default init()`, +`class C extends init()`, a bare `import './x.js'`, a top-level `throw`, `new Set(imported)` and +`{ ...imported }` all look like plain declarations to an allowlist, and every one of them runs code. The +list has three outcomes rather than two, because an import with bindings is neither a finding nor safe: it +evaluates another module, and this script cannot see what that does. The real evidence that the package survives a bundler is `npm run test:bundled`, which attacks the bundled guard rather than reading it. diff --git a/scripts/side-effect-audit.mjs b/scripts/side-effect-audit.mjs index 16c35dc6..a97094c1 100644 --- a/scripts/side-effect-audit.mjs +++ b/scripts/side-effect-audit.mjs @@ -8,9 +8,9 @@ // // This is a REPORT, not a verdict, and the distinction is load-bearing rather than modest. It walks the // region of the AST that runs at import time and names anything that can execute, deliberately -// over-reporting rather than recognising shapes it believes are safe: an allowlist gets this wrong in the -// dangerous direction, and an earlier version of this script called `export default init()`, -// `class C extends init()` and a bare `import './register.js'` all clean. +// over-reporting rather than recognising shapes it believes are safe. An allowlist gets this wrong in the +// dangerous direction: `export default init()`, `class C extends init()` and a bare +// `import './register.js'` all read as plain declarations to one, and all three run code. // // What it cannot do is prove purity. It sees syntax, not behaviour: a call it reports may be harmless and // a call it accepts may not be. Read it before declaring `sideEffects: false`. Do not rely on it instead. @@ -301,9 +301,8 @@ function findExecutable(sf) { * The audit's own tests, and the only part of this script with teeth. * * A tripwire that cannot fire is the same defect as no tripwire, and this one is easy to break in the - * silent direction: many of the cases below were reported clean by an earlier version that recognised - * "safe" shapes instead of walking for executable ones. They are pinned so that cannot come back - * unnoticed. + * silent direction — recognising "safe" shapes instead of walking for executable ones reports most of the + * cases below as clean. They are pinned so that cannot happen unnoticed. * * Three outcomes, not two. `elsewhere` exists so that a bound import is neither treated as a finding nor * declared safe — importing evaluates the target, and this script cannot see what that does. @@ -330,7 +329,7 @@ const CASES = [ { src: 'const o = Object.freeze({ ...importedObject });', expect: 'executes', what: 'freezing a spread of a value from elsewhere' }, { src: 'Object.freeze(someImportedThing);', expect: 'executes', what: 'freezing something others can see' }, - // NOT 'clean'. Importing evaluates the target, and an earlier version of this list called that safe. + // NOT 'clean'. Importing evaluates the target, and this script cannot see what that evaluation does. { src: "import { readFileSync } from 'node:fs';", expect: 'elsewhere', what: 'an import with bindings' }, { src: 'export function a(x = compute()) { return initialize(); }', expect: 'clean', what: 'a function body and its parameter defaults' }, @@ -374,12 +373,12 @@ if (process.argv.includes('--selftest')) { * * Recursive because the build emits into subdirectories: the scaffolder's guard templates live in * `dist/protect/templates/`, and those are the files copied into a consumer's application and executed - * there. A non-recursive listing found eight of fourteen and reported on them as though that were the set, - * which is the failure this whole script is supposed to be looking for, one level up. + * there. An artifact absent from the report is indistinguishable from one that reported clean, so a + * partial listing produces a report that looks complete — the failure this script is looking for, one + * level up. * * Done in Node rather than as a shell pipeline through `xargs`, which an ordinary Windows shell does not - * have — a documented contributor command that works on Linux CI and fails on Windows is the kind of thing - * nobody discovers until they are already stuck. + * have: a documented contributor command must not work on Linux CI and fail on Windows. */ function emittedArtifacts(dir) { if (!existsSync(dir)) { @@ -418,15 +417,14 @@ const files = named.length > 0 ? named : emittedArtifacts(dirFlag?.slice('--dir= /** * This REPORTS. It does not pass or fail on what it finds. * - * Which was tempting to do differently, and every version of that gate was wrong: + * There is no honest threshold to fail on: * * - A CommonJS bundle executes at module scope by construction — `module.exports = …`, `require(…)`, * definitions on the exports object. No version of it does not. * - `dist/cli.js` ends in `main().then(…)`. It is a bin; executing on evaluation is its entire job. - * - The library entries are clean today, but a benign lazy-init call is indistinguishable here from a - * harmful one, so gating on them would mean either an allowlist — which is what made an earlier - * version of this script report `export default init()` as pure — or a red build for a call nobody - * needs to act on. + * - The library entries are clean, but a benign lazy-init call is indistinguishable here from a harmful + * one. Gating on them would mean either an allowlist — the thing that reports `export default init()` + * as pure — or a red build for a call nobody needs to act on. * * So the exit code speaks only for what is unambiguously wrong: a file that does not exist, or the * self-test failing. diff --git a/tests/side-effect-audit-discovery.test.ts b/tests/side-effect-audit-discovery.test.ts index 46e66b94..822c8708 100644 --- a/tests/side-effect-audit-discovery.test.ts +++ b/tests/side-effect-audit-discovery.test.ts @@ -32,7 +32,7 @@ const audited = (stdout: string): string[] => describe('what the audit discovers', () => { it('finds artifacts in subdirectories, not just the top level', () => { // The scaffolder's guard templates are emitted into `dist/protect/templates/` and are the files copied - // into a consumer's application. A non-recursive listing left every one of them out. + // into a consumer's application, so a listing that does not descend speaks for none of them. const dir = mkdtempSync(join(tmpdir(), 'ps-audit-')); mkdirSync(join(dir, 'nested', 'deeper'), { recursive: true }); writeFileSync(join(dir, 'top.js'), 'export const a = 1;\n');