From d7b4e9aa580b2c5b98f42b9a64750b135e592dc5 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:57:01 +0530 Subject: [PATCH 01/18] test: add an offline jspm double the vendor suites can resolve against The vendor suites resolve against the live jspm CDN, so a jspm outage reds a pull request that has nothing to do with vendoring. This is the fixture that replaces the network. It models jspm rather than merely answering it. vendor.js's fallback ladder keys off the exact failure semantics: a 5xx or 429 is transient and retries per package, a 4xx is permanent and probes per install so the resolvable ones survive, and an unresolvable install fails the WHOLE batch. A double that answered every call with a 200 would leave all of that untested while looking green. Refusals are recorded rather than thrown, because every fetch caller in vendor.js swallows a throw and would degrade to "resolved nothing" instead of failing. The install-string parse moves into a shared module so the e2e stub and the double cannot drift apart on the subpath case. --- test/e2e/fixtures/stub-jspm.mjs | 51 ++---- test/fixtures/install-spec.mjs | 78 +++++++++ test/fixtures/jspm-double-preload.mjs | 58 ++++++ test/fixtures/jspm-double.mjs | 243 ++++++++++++++++++++++++++ 4 files changed, 389 insertions(+), 41 deletions(-) create mode 100644 test/fixtures/install-spec.mjs create mode 100644 test/fixtures/jspm-double-preload.mjs create mode 100644 test/fixtures/jspm-double.mjs diff --git a/test/e2e/fixtures/stub-jspm.mjs b/test/e2e/fixtures/stub-jspm.mjs index bc7059853..015d808c0 100644 --- a/test/e2e/fixtures/stub-jspm.mjs +++ b/test/e2e/fixtures/stub-jspm.mjs @@ -36,6 +36,7 @@ import { readFileSync } from 'node:fs'; import { createRequire } from 'node:module'; import { fileURLToPath } from 'node:url'; import { dirname, join, resolve } from 'node:path'; +import { splitInstall, packageName, subpath } from '../../fixtures/install-spec.mjs'; const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..', '..'); // Resolve from the app under test, not from a hardcoded path, so it does not @@ -79,49 +80,17 @@ function localModuleUrl(name) { } /** - * Split an install string into its package name and its subpath. + * The install-string parse lives in `test/fixtures/install-spec.mjs` so this + * fixture and the offline jspm double (#1150) share one implementation rather + * than each carrying its own. Re-exported here because this fixture's own test, + * `test/repo-health/e2e-vendor-stub.test.mjs`, imports them from this path. * - * The four shapes, all of which jspm accepts: `dayjs`, `dayjs@1.11.21`, - * `dayjs/plugin/utc`, `dayjs@1.11.21/plugin/utc`, each also in scoped form - * (`@scope/pkg...`). So the version is OPTIONAL and the subpath does not always - * ride behind one, which rules out cutting at the version separator alone: on - * `dayjs/plugin/utc` there is no `@` to cut at, and taking the whole string as - * the name would report no subpath for an install that plainly has one. - * - * Cut on the first `/` that is not part of a scope instead, then strip any - * version off the name. A scoped name's leading `@` is not a version separator - * and its first `/` is not a subpath, hence the offsets. - * - * @param {string} install - * @returns {{ name: string, subpath: string }} - */ -export function splitInstall(install) { - const scoped = install.startsWith('@'); - // For a scoped install the subpath starts at the SECOND slash, since the - // first one separates the scope from the package. - const scopeSlash = scoped ? install.indexOf('/') : -1; - const slash = install.indexOf('/', scoped ? scopeSlash + 1 : 0); - const head = slash === -1 ? install : install.slice(0, slash); - const at = head.indexOf('@', scoped ? 1 : 0); - return { - name: at === -1 ? head : head.slice(0, at), - subpath: slash === -1 ? '' : install.slice(slash), - }; -} - -/** @param {string} install @returns {string} */ -export function packageName(install) { return splitInstall(install).name; } - -/** - * The part of an install after its package name AND version, if any, so - * `/plugin/utc` for both `dayjs/plugin/utc` and `dayjs@1.11.21/plugin/utc`. A subpath - * needs its own importmap key pointing at its own file, which this fixture does - * not build, so a subpath install counts as unserviceable rather than being - * answered with the bare package's entry. - * @param {string} install - * @returns {string} + * A subpath install matters to this fixture in one specific way: it needs its + * own importmap key pointing at its own file, which this fixture does not + * build, so `localImportsFor` treats it as unserviceable rather than answering + * it with the bare package's entry. */ -export function subpath(install) { return splitInstall(install).subpath; } +export { splitInstall, packageName, subpath }; /** * Build the importmap this fixture would answer a `/generate` call with, or diff --git a/test/fixtures/install-spec.mjs b/test/fixtures/install-spec.mjs new file mode 100644 index 000000000..f03be82c7 --- /dev/null +++ b/test/fixtures/install-spec.mjs @@ -0,0 +1,78 @@ +/** + * Parse a jspm install string into its package name, version, and subpath. + * + * Shared by the two vendor fixtures, which need the same parse for different + * reasons. `test/e2e/fixtures/stub-jspm.mjs` (#1228) uses it to decide whether + * it can serve an install from this repo, and `test/fixtures/jspm-double.mjs` + * (#1150) uses it to mint a jspm-shaped URL. Both would otherwise reach for the + * `install.replace(/@[^@]*$/, '')` shortcut that several inline mocks in + * `packages/server/test/vendor/vendor.test.js` use, which is wrong on any + * install carrying a subpath: on `dayjs@1.11.13/plugin/utc` the trailing + * `@1.11.13/plugin/utc` is one match, so the whole subpath disappears with the + * version and the caller believes the install was a bare `dayjs`. + * + * This module has NO side effects, so importing it never patches anything. + */ + +/** + * Split an install string into its package name, version, and subpath. + * + * The four shapes, all of which jspm accepts: `dayjs`, `dayjs@1.11.21`, + * `dayjs/plugin/utc`, `dayjs@1.11.21/plugin/utc`, each also in scoped form + * (`@scope/pkg...`). So the version is OPTIONAL and the subpath does not always + * ride behind one, which rules out cutting at the version separator alone: on + * `dayjs/plugin/utc` there is no `@` to cut at, and taking the whole string as + * the name would report no subpath for an install that plainly has one. + * + * Cut on the first `/` that is not part of a scope instead, then strip any + * version off the name. A scoped name's leading `@` is not a version separator + * and its first `/` is not a subpath, hence the offsets. + * + * @param {string} install + * @returns {{ name: string, version: string, subpath: string }} + */ +export function splitInstall(install) { + const scoped = install.startsWith('@'); + // For a scoped install the subpath starts at the SECOND slash, since the + // first one separates the scope from the package. + const scopeSlash = scoped ? install.indexOf('/') : -1; + const slash = install.indexOf('/', scoped ? scopeSlash + 1 : 0); + const head = slash === -1 ? install : install.slice(0, slash); + const at = head.indexOf('@', scoped ? 1 : 0); + return { + name: at === -1 ? head : head.slice(0, at), + version: at === -1 ? '' : head.slice(at + 1), + subpath: slash === -1 ? '' : install.slice(slash), + }; +} + +/** @param {string} install @returns {string} */ +export function packageName(install) { return splitInstall(install).name; } + +/** + * The version an install pins, or the empty string when it names none. + * @param {string} install + * @returns {string} + */ +export function packageVersion(install) { return splitInstall(install).version; } + +/** + * The part of an install after its package name AND version, if any, so + * `/plugin/utc` for both `dayjs/plugin/utc` and `dayjs@1.11.21/plugin/utc`. A + * subpath needs its own importmap key pointing at its own file. + * @param {string} install + * @returns {string} + */ +export function subpath(install) { return splitInstall(install).subpath; } + +/** + * The importmap KEY an install resolves under, which is the package name plus + * the subpath and never the version. `dayjs@1.11.21/plugin/utc` is imported in + * source as `dayjs/plugin/utc`, so that is what the browser looks up. + * @param {string} install + * @returns {string} + */ +export function importKey(install) { + const { name, subpath: sub } = splitInstall(install); + return `${name}${sub}`; +} diff --git a/test/fixtures/jspm-double-preload.mjs b/test/fixtures/jspm-double-preload.mjs new file mode 100644 index 000000000..9d2e474b1 --- /dev/null +++ b/test/fixtures/jspm-double-preload.mjs @@ -0,0 +1,58 @@ +/** + * Install the jspm double into a SPAWNED process (#1150). + * + * `test/vendor-cli/vendor-cli.test.mjs` runs the real CLI binary in a child + * process, so the in-process `withJspmDouble` cannot reach it and the child + * would resolve vendors against the live CDN. This module is what closes that + * gap: the test passes it as `--import` (Node) or `--preload` (Bun) ahead of + * the CLI path, and it patches `globalThis.fetch` before any application code + * runs. It is loaded as a runtime flag rather than through `NODE_OPTIONS` + * because Bun ignores `NODE_OPTIONS` and neither runtime honours the other's + * flag, the lesson `test/e2e/e2e.test.mjs` already carries for #1229's stub. + * + * Two signals go to stderr, and both are load-bearing. + * + * `[jspm-double] armed` proves the preload actually took effect. Without it, + * dropping the flag from `runCli` would leave every test green while silently + * restoring the network dependency, because the CLI's observable output looks + * the same either way. The CLI test asserts this marker on EVERY spawn rather + * than on one, so no call site can lose the wiring unnoticed. + * + * A refusal line plus a non-zero `process.exitCode` is how an unserved request + * fails the test. Every fetch caller in `packages/server/src/vendor.js` + * swallows a throw, so a request this double does not serve would otherwise + * degrade to "resolved nothing" and the CLI could still exit 0. Forcing the + * exit code makes the existing `assert.equal(code, 0)` catch it. + * + * Configure it with a `WEBJS_JSPM_DOUBLE` env var holding the JSON options + * `jspmDouble()` takes. Absent means resolve everything. + */ +import { jspmDouble } from './jspm-double.mjs'; + +/** @type {import('./jspm-double.mjs').JspmDoubleOptions} */ +let opts = {}; +const raw = process.env.WEBJS_JSPM_DOUBLE; +if (raw) { + try { + opts = JSON.parse(raw); + } catch (err) { + // A config this process cannot read must not silently become "serve + // everything", since that is a different test than the one asked for. + process.stderr.write(`[jspm-double] unreadable WEBJS_JSPM_DOUBLE: ${String(err)}\n`); + process.exitCode = 1; + } +} + +const double = jspmDouble(opts); +let reported = 0; + +globalThis.fetch = /** @type {any} */ (async function doubledFetch(input, init) { + const response = await double(input, init); + while (reported < double.unexpected.length) { + process.stderr.write(`[jspm-double] refused ${double.unexpected[reported++]}\n`); + process.exitCode = 1; + } + return response; +}); + +process.stderr.write('[jspm-double] armed\n'); diff --git a/test/fixtures/jspm-double.mjs b/test/fixtures/jspm-double.mjs new file mode 100644 index 000000000..9d33f20b5 --- /dev/null +++ b/test/fixtures/jspm-double.mjs @@ -0,0 +1,243 @@ +/** + * An offline stand-in for api.jspm.io and ga.jspm.io (#1150). + * + * The required `Unit + integration` CI job used to resolve vendors against the + * live jspm CDN, so a jspm outage redded pull requests that had nothing to do + * with vendoring (#1149 was a five-file documentation change). This double is + * what the vendor tests resolve against instead. Exactly one file in the tree, + * `packages/server/test/vendor/jspm-cdn.live.test.js`, still talks to the real + * CDN, and both test runners keep `*.live.test.*` out of a normal run. + * + * It models jspm rather than merely answering, because `packages/server/src/ + * vendor.js` is built on jspm's exact failure semantics. `jspmGenerate` sends + * one unified call for a multi-install set, and its whole fallback ladder keys + * off what comes back: a 5xx or a 429 is transient and retries per package, a + * 4xx is permanent and triggers per-install probes so the resolvable ones + * survive. A double that answered every request with a 200 would leave that + * ladder untested while looking green. + * + * The `/double.js` tail on every minted URL is load-bearing. Real jspm never + * emits it, so a test can assert on it to prove it is talking to this double + * and not to the network. `test/vendor-cli/vendor-cli.test.mjs` does exactly + * that, because its own `ga.jspm.io/npm:picocolors@` prefix check is equally + * true of the real CDN and so cannot notice the double being unplugged. + * + * REFUSAL IS RECORDED, NOT THROWN. Every fetch caller in vendor.js swallows a + * throw (`jspmCall`, `downloadBundle`, `fetchIntegrity`, `fetchLiveIntegrity` + * all catch and degrade), so a double that threw on an unexpected request + * would silently turn into "resolved nothing" and a weak assertion would still + * pass. Unexpected requests land on `double.unexpected` instead, which + * `withJspmDouble` asserts is empty and the preload turns into a non-zero exit. + * + * This is deliberately NOT the same fixture as `test/e2e/fixtures/ + * stub-jspm.mjs`. That one must emit a real executable module for a browser to + * run, and it passes anything it cannot serve through to the real network. This + * one only needs jspm-SHAPED urls and some bytes, and it must never pass + * anything through. Keep them separate. + * + * This module has NO side effects. Importing it patches nothing; call + * `jspmDouble()` or `withJspmDouble()` to use it. + */ +import { importKey, splitInstall } from './install-spec.mjs'; + +/** Hosts this double owns. A request to any of them must never reach the network. */ +const OWNED_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; + +const GENERATE_ENDPOINT = 'https://api.jspm.io/generate'; + +/** The body a minted bundle url serves, when a caller does not supply one. */ +const DEFAULT_BUNDLE = 'export default "offline jspm double bundle";\n'; + +/** + * @typedef {object} JspmDoubleOptions + * @property {string[]} [unresolvable] + * Installs jspm cannot resolve. Real jspm fails the WHOLE batch with a 401 + * when any single install is unresolvable (`vendor.js` documents this as the + * reason `jspmGenerate` probes per package on a permanent failure), so + * listing one install here fails every call that carries it. + * @property {Record} [transitives] + * Extra `{ importKey: url }` entries folded into an answer alongside the + * requested installs, standing in for the flattened transitives a real + * unified resolve returns (#446). Only added when the call resolved, since + * jspm cannot hoist a transitive out of nothing. + * @property {number} [status] + * Force every `/generate` call to this HTTP status. Use it for the transient + * paths (503, 429), which `vendor.js` retries per package rather than + * probing. + * @property {string} [bundle] + * The body a minted bundle URL serves. Defaults to a tiny ES module. + */ + +/** + * Build an offline `fetch` that answers jspm. + * + * @param {JspmDoubleOptions} [opts] + */ +export function jspmDouble(opts = {}) { + const unresolvable = new Set(opts.unresolvable || []); + const transitives = opts.transitives || {}; + const bundle = opts.bundle ?? DEFAULT_BUNDLE; + + /** @type {Array<{ url: string, method: string, installs: string[] }>} */ + const calls = []; + /** @type {string[]} */ + const unexpected = []; + /** Every bundle url this double has handed out, so a GET can be recognised. */ + const minted = new Set(); + + /** + * The url a resolved install is served from. Keeping `@` in + * the path verbatim matters: `derivePinParts` in vendor.js recovers a + * flattened transitive's version by locating exactly that substring in the + * resolved url, and `pinAll` cannot derive a `--download` filename without + * it. + * @param {string} install + */ + const mint = (install) => { + const { name, version, subpath } = splitInstall(install); + // An install with no pinned version still has to produce a parseable url, + // and jspm would have chosen a concrete version here. + const url = `https://ga.jspm.io/npm:${name}@${version || '0.0.0'}${subpath}/double.js`; + minted.add(url); + return url; + }; + + /** @param {any} input */ + const urlOf = (input) => (typeof input === 'string' ? input + : input instanceof URL ? input.href + : (input && input.url) || ''); + + /** @param {any} init */ + const installsOf = (init) => { + try { + const body = init && typeof init.body === 'string' ? JSON.parse(init.body) : null; + if (body && Array.isArray(body.install)) { + return body.install.filter((/** @type {unknown} */ i) => typeof i === 'string'); + } + } catch { /* an unreadable body names no installs, handled by the caller */ } + return []; + }; + + /** @param {number} status @param {unknown} body */ + const json = (status, body) => new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); + + /** + * @param {any} input + * @param {any} [init] + * @returns {Promise} + */ + async function doubledFetch(input, init) { + const url = urlOf(input); + const method = (init && init.method) || 'GET'; + + if (url === GENERATE_ENDPOINT || url.startsWith(`${GENERATE_ENDPOINT}?`)) { + const installs = installsOf(init); + calls.push({ url, method, installs }); + + // A caller that named no installs sent something this double cannot + // read. Answering `{}` would be the silent failure the whole fixture + // exists to remove, since an absent importmap entry is an unresolved + // bare specifier that kills a page's entire module graph. + if (!installs.length) { + unexpected.push(`${method} ${url} (no readable install list)`); + return json(400, { error: 'Error: no install list' }); + } + + if (opts.status && opts.status !== 200) { + return json(opts.status, { error: `Error: forced ${opts.status}` }); + } + + // Real jspm fails the WHOLE batch, not the individual entry. That is the + // premise `jspmGenerate`'s per-package probing is built on, and + // `packages/server/test/vendor/jspm-cdn.live.test.js` re-checks it + // against the real API nightly. + if (installs.some((/** @type {string} */ i) => unresolvable.has(i))) { + return json(401, { error: 'Error: Not Found' }); + } + + /** @type {Record} */ + const imports = {}; + for (const install of installs) imports[importKey(install)] = mint(install); + // Transitives are hoisted by the unified resolve, so they ride along + // with a successful answer rather than appearing on their own. + for (const [key, target] of Object.entries(transitives)) { + imports[key] = target; + minted.add(target); + } + return json(200, { map: { imports } }); + } + + if (minted.has(url)) { + calls.push({ url, method, installs: [] }); + return new Response(bundle, { + status: 200, + headers: { 'content-type': 'text/javascript' }, + }); + } + + if (OWNED_HOSTS.some((h) => url.includes(h))) { + // Recorded rather than thrown: vendor.js catches every fetch rejection, + // so a throw here would be indistinguishable from "the CDN was down" and + // would quietly weaken whatever test hit it. + unexpected.push(`${method} ${url}`); + return json(599, { error: 'Error: the jspm double was not asked to serve this' }); + } + + unexpected.push(`${method} ${url} (not a jspm double host)`); + return json(599, { error: 'Error: the jspm double does not proxy to the network' }); + } + + return Object.assign(doubledFetch, { + calls, + /** Just the `/generate` calls, which is what a round-trip count means. */ + get generateCalls() { return calls.filter((c) => c.url.startsWith(GENERATE_ENDPOINT)); }, + unexpected, + minted, + }); +} + +/** + * Run `body` with the double installed on `globalThis.fetch`, then restore. + * + * The vendor caches are cleared on both sides, because they are keyed on the + * install set and would otherwise carry one test's answer into the next. Any + * request the double refused throws at the end, which is what makes an + * unplugged or mis-shaped double loud instead of silent. + * + * `vendor.js` is imported lazily, and by relative path rather than as + * `@webjsdev/server`, for two reasons. Lazily, so the preload arm can load + * `jspmDouble` into a spawned CLI without dragging server source in behind it. + * By relative path, so this clears the caches of the same module instance + * `packages/server/test/vendor/vendor.test.js` imports; a bare specifier + * resolves through `node_modules`, which in a linked worktree is a different + * checkout and therefore a different set of caches. + * + * @template T + * @param {JspmDoubleOptions} opts + * @param {(double: ReturnType) => Promise} body + * @returns {Promise} + */ +export async function withJspmDouble(opts, body) { + const { clearVendorCache } = await import( + new URL('../../packages/server/src/vendor.js', import.meta.url).href + ); + const double = jspmDouble(opts); + const original = globalThis.fetch; + globalThis.fetch = /** @type {any} */ (double); + clearVendorCache(); + try { + return await body(double); + } finally { + globalThis.fetch = original; + clearVendorCache(); + if (double.unexpected.length) { + throw new Error( + `the jspm double was asked for ${double.unexpected.length} request(s) it does not serve:\n ` + + `${double.unexpected.join('\n ')}`, + ); + } + } +} From e0b0a65655a69d3dfd37cadaf382cfc91d81d629 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:59:37 +0530 Subject: [PATCH 02/18] test: resolve the CLI vendor tests through the double, not the CDN Every runCli spawn now carries the preload, so the child answers its own jspm calls. Verified with the network namespace cut: all ten pass under unshare -rn, where six of them hard-failed before. The flag is picked from the runtime rather than hardcoded, because the spawn uses process.execPath and under bun test that is the bun binary, which ignores --import and NODE_OPTIONS both. Node gets a file:// URL because the spawn sets cwd to the temp app, so a relative path would resolve against the wrong place. The armed marker is asserted inside runCli rather than in one test, since the CLI's own output is identical whether the preload is there or not. Unplugging the flag reds all ten. The /double.js tail assertion is the second half of that proof, because the existing ga.jspm.io prefix check is equally true of the real CDN. --- test/vendor-cli/vendor-cli.test.mjs | 63 +++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/test/vendor-cli/vendor-cli.test.mjs b/test/vendor-cli/vendor-cli.test.mjs index 291a0f6ba..ccf70282e 100644 --- a/test/vendor-cli/vendor-cli.test.mjs +++ b/test/vendor-cli/vendor-cli.test.mjs @@ -1,29 +1,52 @@ /** * CLI integration tests for `webjs vendor pin` / `unpin` / `list`. * - * Spawns the actual webjs CLI binary against a temp app directory and - * asserts the file-system + stdout contracts. + * Spawns the actual CLI binary against a temp app directory and asserts the + * file-system + stdout contracts. * - * Network-gated: pin without --download calls api.jspm.io. Skip via - * WEBJS_SKIP_NETWORK_TESTS=1 in air-gapped CI environments. + * OFFLINE (#1150). `webjs vendor pin` resolves through api.jspm.io, and this + * file used to let the spawned CLI reach it, which is how a jspm outage redded + * the required CI job on PR #1149, a documentation-only change. Every spawn now + * carries `test/fixtures/jspm-double-preload.mjs`, so the child answers itself. + * The live half of the same contract lives in `vendor-pin.live.test.mjs`, which + * both test runners skip unless `WEBJS_REQUIRE_NETWORK=1`. */ import { test, before, after, describe } from 'node:test'; import assert from 'node:assert/strict'; import { spawn } from 'node:child_process'; +import { statSync } from 'node:fs'; import { mkdtemp, writeFile, mkdir, readFile, rm, symlink } from 'node:fs/promises'; import { join, resolve, dirname } from 'node:path'; import { tmpdir } from 'node:os'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; const __dirname = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = resolve(__dirname, '..', '..'); const CLI = resolve(REPO_ROOT, 'packages', 'cli', 'bin', 'webjs.js'); -const NETWORK_OK = !process.env.WEBJS_SKIP_NETWORK_TESTS; +const PRELOAD = resolve(__dirname, '..', 'fixtures', 'jspm-double-preload.mjs'); +// A moved or renamed preload must fail here rather than in the child, where a +// module-not-found would surface as an opaque non-zero exit code and the +// obvious reading would be that the CLI itself broke. +statSync(PRELOAD); + +/** + * The flag that loads the preload into the child, chosen by runtime. + * + * Node and Bun each ignore the other's spelling, and Bun ignores NODE_OPTIONS + * entirely, so this cannot be one hardcoded flag or an env var. The parent + * runtime IS the child runtime here, because the spawn below uses + * `process.execPath`, which under `bun test` is the bun binary. Node wants a + * URL rather than a path, since the spawn sets `cwd` to the temp app directory + * and a relative `--import` would resolve against that instead of the repo. + */ +const PRELOAD_ARGS = process.versions.bun + ? ['--preload', PRELOAD] + : ['--import', pathToFileURL(PRELOAD).href]; function runCli(args, cwd) { return new Promise((res, rej) => { - const child = spawn(process.execPath, [CLI, ...args], { + const child = spawn(process.execPath, [...PRELOAD_ARGS, CLI, ...args], { cwd, env: { ...process.env, FORCE_COLOR: '0' }, }); @@ -31,7 +54,16 @@ function runCli(args, cwd) { let stderr = ''; child.stdout.on('data', (d) => { stdout += d.toString(); }); child.stderr.on('data', (d) => { stderr += d.toString(); }); - child.on('exit', (code) => res({ code, stdout, stderr })); + child.on('exit', (code) => { + // Assert the wiring on EVERY spawn, not in one test. Dropping the flag + // would otherwise leave this file green while silently restoring the + // live-CDN dependency, since the CLI's own output is identical either + // way. The preload also forces a non-zero exit on any request the double + // does not serve, which the per-test `code` assertions then catch. + assert.match(stderr, /\[jspm-double\] armed/, + 'the jspm double must be preloaded into every CLI child'); + res({ code, stdout, stderr }); + }); child.on('error', rej); }); } @@ -62,7 +94,7 @@ describe('webjs vendor CLI', () => { assert.match(stdout, /No pin file/); }); - test('pin writes .webjs/vendor/importmap.json with picocolors entry', { skip: !NETWORK_OK }, async () => { + test('pin writes .webjs/vendor/importmap.json with picocolors entry', async () => { const { code, stdout, stderr } = await runCli(['vendor', 'pin'], appDir); assert.equal(code, 0, `pin failed: ${stderr}`); assert.match(stdout, /Pinning vendor packages/); @@ -73,16 +105,21 @@ describe('webjs vendor CLI', () => { const parsed = JSON.parse(file); assert.ok(parsed.imports.picocolors, 'picocolors should be in the pinned importmap'); assert.match(parsed.imports.picocolors, /^https:\/\/ga\.jspm\.io\/npm:picocolors@/); + // The prefix above is equally true of the real CDN, so on its own it cannot + // tell a doubled resolve from a live one. This tail can: jspm never emits + // it, so it only appears when the preload is actually in the child. + assert.match(parsed.imports.picocolors, /\/double\.js$/, + 'the url must come from the jspm double, not from the network'); }); - test('list with pin file reports the pinned package + URL', { skip: !NETWORK_OK }, async () => { + test('list with pin file reports the pinned package + URL', async () => { const { code, stdout } = await runCli(['vendor', 'list'], appDir); assert.equal(code, 0); assert.match(stdout, /picocolors@/); assert.match(stdout, /https:\/\/ga\.jspm\.io\/npm:picocolors@/); }); - test('unpin removes a package entry from importmap.json', { skip: !NETWORK_OK }, async () => { + test('unpin removes a package entry from importmap.json', async () => { const { code, stdout } = await runCli(['vendor', 'unpin', 'picocolors'], appDir); assert.equal(code, 0); assert.match(stdout, /picocolors\s+unpinned/); @@ -107,7 +144,7 @@ describe('webjs vendor CLI', () => { assert.match(stderr, /not in pin file/); }); - test('pin --download writes bundle files alongside importmap.json', { skip: !NETWORK_OK }, async () => { + test('pin --download writes bundle files alongside importmap.json', async () => { const { code, stdout, stderr } = await runCli(['vendor', 'pin', '--download'], appDir); assert.equal(code, 0, `pin --download failed: ${stderr}`); assert.match(stdout, /downloading bundles/); @@ -162,7 +199,7 @@ describe('webjs vendor CLI', () => { // #448: the opt-in pins `webjs vendor pin` writes must be committable. A // `.gitignore` that excludes `.webjs/` silently swallows them; pinning must // self-heal that so a user can commit what they deliberately created. -describe('webjs vendor pin makes pins committable (#448)', { skip: !NETWORK_OK }, () => { +describe('webjs vendor pin makes pins committable (#448)', () => { function git(args, cwd) { return new Promise((res) => { const { GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_PREFIX, ...env } = process.env; From e8eba94efb175133ab05a2f448209dfd2a049f76 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:03:59 +0530 Subject: [PATCH 03/18] test: resolve the in-process vendor tests through the double Thirteen tests move off the live CDN, two lose a gate they never needed (an empty install list short-circuits before any call, and the audit test installs its own throwing fetch), and two that were never gated at all stop reaching registry.npmjs.org. The whole vendor surface now runs with the network namespace cut: 170 tests, 0 failures. Three become stronger rather than merely offline. The cache test could only compare two results against a live CDN, which stays true even if a second round trip fired; it counts calls now. The partial-success test pins the exact permanent-failure trace (unified 401, two probes, the survivor served from the probe cache). And pinAll's flattened-transitive path (#446) gains its first coverage at any layer, because every existing pinAll test resolves picocolors, which has no dependencies for a live CDN to hoist. The live parity test moves to jspm-cdn.live.test.js, where it keeps its transport-level skip and gains a companion that re-anchors the premise the fallback ladder rests on: jspm fails the WHOLE batch, permanently, when one install is unresolvable. A double cannot vouch for that, since the double is built from the same belief. --- .../server/test/vendor/jspm-cdn.live.test.js | 196 ++++++++ packages/server/test/vendor/vendor.test.js | 418 ++++++++---------- test/fixtures/jspm-double.mjs | 12 +- 3 files changed, 396 insertions(+), 230 deletions(-) create mode 100644 packages/server/test/vendor/jspm-cdn.live.test.js diff --git a/packages/server/test/vendor/jspm-cdn.live.test.js b/packages/server/test/vendor/jspm-cdn.live.test.js new file mode 100644 index 000000000..4d4775741 --- /dev/null +++ b/packages/server/test/vendor/jspm-cdn.live.test.js @@ -0,0 +1,196 @@ +/** + * The ONLY tests in this repo that talk to the real jspm CDN (#1150). + * + * `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` both skip any + * `*.live.test.*` file unless `WEBJS_REQUIRE_NETWORK=1`, so nothing here runs + * in the required `Unit + integration` CI job. That is the point: a jspm + * outage used to red pull requests that had nothing to do with vendoring, and + * PR #1149, a five-file documentation change, is what finally made the case. + * Everything else in the vendor suites resolves through + * `test/fixtures/jspm-double.mjs`. + * + * Deleting the live coverage instead was never the goal. The vendor resolver's + * whole job is to talk to jspm, and a double can only ever return what this + * repo already believes about the API. So these two assertions stay real, and + * `.github/workflows/vendor-cdn.yml` runs them nightly with + * `WEBJS_REQUIRE_NETWORK=1`, which ALSO turns a skip into a failure. Without + * that, a permanently skipping test is indistinguishable from a passing one. + * + * Upstream trouble skips rather than reds, judged at the transport: a throw, a + * 5xx, or a 429 is jspm having a bad moment. A 4xx does not skip, because by + * then a ground-truth call has just succeeded against the same fixture, so + * upstream is demonstrably healthy and a 4xx means OUR request is malformed. + * That distinction is #1219's, and it is the reason this file can be run + * nightly without becoming a source of false alarms. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { jspmGenerate, clearVendorCache } from '../../src/vendor.js'; + +/** The body vendor.js posts, so a ground-truth call is comparable to ours. */ +const GENERATE_BODY = (install) => JSON.stringify({ + install, flattenScope: true, env: ['browser', 'production', 'module'], provider: 'jspm.io', +}); + +/** + * Build a loud skip for one fixture. + * + * Loud on purpose: a silent skip is how a real regression hides, so the reason + * and the fixture are always named. Under `WEBJS_REQUIRE_NETWORK` the skip + * becomes a FAILURE instead, which is what makes the nightly job able to tell + * "jspm changed under us" from "everything is fine". A normal run is + * unaffected, since the runners exclude this file entirely. + * + * @param {import('node:test').TestContext} t + * @param {string} fixture + */ +function skipper(t, fixture) { + return (reason) => { + const first = String(reason).split('\n')[0]; + if (process.env.WEBJS_REQUIRE_NETWORK) { + assert.fail(`live jspm check could not run (${fixture}): ${first}`); + } + console.warn(`[jspm-cdn.live] SKIP ${fixture} (${first})`); + t.skip('jspm.io was not in a state that can answer this comparison'); + }; +} + +test('jspm fails the WHOLE batch, permanently, when one install is unresolvable', async (t) => { + // The premise the entire fallback ladder in jspmGenerate rests on, and the + // one thing a double cannot vouch for, since the double is built from this + // very belief. Two properties, both load-bearing: + // + // 1. WHOLE batch. A resolvable install alongside an unresolvable one still + // fails, which is why jspmGenerate probes each install alone instead of + // trusting a partial map. If jspm ever switched to partial-success 200s, + // the probing would become dead code and nothing else would notice. + // 2. PERMANENT, not transient. vendor.js classifies >= 500 and 429 as + // transient and retries per package; anything else drops the failing + // install. An unknown package landing on the transient side would turn + // a pin failure into a retry storm. + const skip = skipper(t, 'whole-batch 401 premise'); + const installs = ['picocolors@1.1.1', 'this-package-truly-does-not-exist-xyz-789@99.0.0']; + + let res; + try { + res = await fetch('https://api.jspm.io/generate', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: GENERATE_BODY(installs), + signal: AbortSignal.timeout(15_000), + }); + } catch (err) { + skip(`${err.name}: ${err.message}`); + return; + } + // A 5xx or a 429 is upstream having a bad moment rather than an answer about + // the premise, so it skips like any other transport trouble. + if (res.status >= 500 || res.status === 429) { skip(`HTTP ${res.status}`); return; } + + assert.ok(!res.ok, + `jspm answered ${res.status} for a batch containing an unresolvable install; ` + + 'jspmGenerate\'s per-install probing assumes the whole batch fails'); + assert.ok(res.status < 500 && res.status !== 429, + `an unresolvable install must be a PERMANENT failure, got HTTP ${res.status}`); +}); + +test('jspmGenerate #446: matches jspm\'s own unified graph (real CDN)', async (t) => { + // The integration half: our merged output must equal what jspm itself + // computes for the same install set. The mock above cannot prove this, + // because a mock only ever returns what this file already believes. + // + // The fixture is chosen so the comparison can actually FAIL two distinct + // ways, since a parity assertion over a set with nothing to disagree about + // is decoration: + // + // 1. Per-package skew. Resolved alone, @codemirror/lint drags in + // view@6.41.x; in the unified graph the pinned view@6.39.0 wins. So a + // revert of jspmGenerate to the pre-#446 per-package loop makes lint's + // isolated call supply the newer view, which wins last-write and diverges + // from the ground truth here. Two packages with no shared transitive + // (say picocolors + clsx) cannot catch that: their unified graph is + // byte-identical to the union of their single-install graphs. + // 2. A dropped flattenScope. This pair hoists five transitives to top level + // (@codemirror/state, crelt, style-mod, w3c-keyname, + // @marijn/find-cluster-break). vendor.js sends flattenScope: true so the + // browser gets no unresolved bare specifier, and this ground truth sends + // it too, so removing it from vendor.js drops those entries from our + // imports and reds the deepEqual. Nothing else in the suite covers that + // flag: every mock here answers only on `install`, so a mocked assertion + // on a transitive key reads a value the mock itself fabricated. + // + // lint is pinned at 6.9.5 rather than a version whose view range EXCLUDES + // 6.39.0 (only 6.9.6 and 6.9.7 do that, and neither resolves on jspm.io, see + // the mock test above). The incompatible-range case is the mock's job; this + // one only needs a shared transitive whose resolution differs per strategy. + const installs = ['@codemirror/view@6.39.0', '@codemirror/lint@6.9.5']; + + const skip = skipper(t, `unified-graph parity: ${installs.join(' + ')}`); + + // Half one, the ground truth. Every failure mode routes to the skip, not + // just an `error` in a well-formed JSON body: a DNS failure or reset throws + // out of fetch, a proxy's HTML 502 throws out of .json(), and a hang is cut + // by the timeout. Without that timeout a wedged api.jspm.io would hold the + // unit job open until the CI job limit, since node --test applies no + // per-test deadline of its own. The shipped code guards its own call the + // same way (JSPM_GENERATE_TIMEOUT_MS in packages/server/src/vendor.js). + let gt; + let why = ''; + try { + const gtResp = await fetch('https://api.jspm.io/generate', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + install: installs, flattenScope: true, + env: ['browser', 'production', 'module'], provider: 'jspm.io', + }), + signal: AbortSignal.timeout(15_000), + }); + gt = await gtResp.json(); + if (!gtResp.ok) why = `HTTP ${gtResp.status}`; + else if (gt.error) why = String(gt.error); + else if (!gt.map?.imports) why = 'response carried no map.imports'; + } catch (err) { + why = `${err.name}: ${err.message}`; + } + if (why) { skip(why); return; } + + // Half two, our own call, watched at the TRANSPORT rather than judged by its + // return value. jspmGenerate fail-opens, so its output cannot tell the two + // failure kinds apart: a transient on the unified call returns a NON-empty + // merge of per-install fragments (skewed to view@6.41.x for this fixture), + // and an unresolvable set returns {}. Reading the map alone therefore either + // reds on an upstream blip or skips on a real bug, depending on which shape + // you test for. Both are wrong. + // + // So record what the network actually did. A throw, a 5xx, or a 429 is + // upstream having a bad moment, and skips. A 4xx does NOT skip: the ground + // truth just succeeded for this same fixture moments ago, so upstream is + // demonstrably healthy, and a 4xx now means OUR request is malformed, which + // is precisely the regression this test exists to catch. + const realFetch = globalThis.fetch; + /** @type {string[]} */ + const transient = []; + globalThis.fetch = async (url, opts) => { + try { + const r = await realFetch(url, opts); + if (r.status >= 500 || r.status === 429) transient.push(`HTTP ${r.status}`); + return r; + } catch (err) { + transient.push(`${err.name}: ${err.message}`); + throw err; + } + }; + let map; + try { + clearVendorCache(); + map = await jspmGenerate(installs); + } finally { + globalThis.fetch = realFetch; + } + if (transient.length) { skip(`jspm.io flaked on our own call (${transient[0]})`); return; } + + assert.deepEqual(map, gt.map.imports, + 'jspmGenerate must equal the single unified graph, not a per-package merge'); +}); diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index 2144f0211..20d7ed901 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -23,6 +23,7 @@ import { findOutdated, updatePinned, } from '../../src/vendor.js'; +import { withJspmDouble } from '../../../../test/fixtures/jspm-double.mjs'; // --- extractPackageName --- @@ -396,35 +397,43 @@ test('getPackageVersion: returns null for unresolvable package', () => { assert.equal(v, null); }); -// --- jspmGenerate (network-gated) --- +// --- jspmGenerate --- // -// These tests hit api.jspm.io. Skip via WEBJS_SKIP_NETWORK_TESTS=1 in -// air-gapped CI. +// These used to hit api.jspm.io, which is how a jspm outage redded the required +// CI job on a documentation-only PR (#1149, #1150). They resolve through +// `test/fixtures/jspm-double.mjs` now. The one test that genuinely needs the +// real API lives in `jspm-cdn.live.test.js`, which both runners skip unless +// `WEBJS_REQUIRE_NETWORK=1`. -const NETWORK_OK = !process.env.WEBJS_SKIP_NETWORK_TESTS; - -test('jspmGenerate: empty install list returns empty map', { skip: !NETWORK_OK }, async () => { +test('jspmGenerate: empty install list returns empty map', async () => { clearVendorCache(); + // No double needed: an empty list short-circuits before any call. const result = await jspmGenerate([]); assert.deepEqual(result, {}); }); -test('jspmGenerate: resolves a real package to a CDN URL', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); - const result = await jspmGenerate(['picocolors@1.1.1']); - const url = result['picocolors']; - assert.ok(url, 'expected picocolors entry in result'); - assert.match(url, /^https:\/\/ga\.jspm\.io\/npm:picocolors@1\.1\.1/); +test('jspmGenerate: resolves a package to a CDN URL', async () => { + await withJspmDouble({}, async () => { + const result = await jspmGenerate(['picocolors@1.1.1']); + const url = result['picocolors']; + assert.ok(url, 'expected picocolors entry in result'); + assert.match(url, /^https:\/\/ga\.jspm\.io\/npm:picocolors@1\.1\.1/); + }); }); -test('jspmGenerate: second call with same installs hits in-process cache', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); - const first = await jspmGenerate(['picocolors@1.1.1']); - // Per-install cache: each call rebuilds a merged container but the - // underlying URL is the cached Promise's resolved value, so the URL - // is identical and no second HTTP round-trip fires. - const second = await jspmGenerate(['picocolors@1.1.1']); - assert.deepEqual(first, second, 'cached call returns the same URLs'); +test('jspmGenerate: second call with same installs hits in-process cache', async () => { + await withJspmDouble({}, async (double) => { + const first = await jspmGenerate(['picocolors@1.1.1']); + // Per-install cache: each call rebuilds a merged container but the + // underlying URL is the cached Promise's resolved value, so the URL + // is identical and no second HTTP round-trip fires. + const second = await jspmGenerate(['picocolors@1.1.1']); + assert.deepEqual(first, second, 'cached call returns the same URLs'); + // Against the live CDN this test could only compare the two results, which + // stays true even if a second round trip fired. The double can count, so + // the cache claim in the comment above is now actually asserted. + assert.equal(double.generateCalls.length, 1, 'the second call must not reach the API'); + }); }); test('jspmGenerate: install order does not affect OUR merged output (deterministic mock, no live CDN)', async () => { @@ -553,18 +562,24 @@ test('jspmGenerate: 200 with malformed JSON does not crash', async () => { }); }); -test('jspmGenerate: per-package isolation - one bad install does not poison good ones', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); - // Mix a known-good package with a known-bad one. jspm.io 401s the - // bad one alone, but the good one MUST still resolve. This is the - // regression test for the batched-call bug where one unresolvable - // dep collapsed the entire importmap. - const result = await jspmGenerate([ - 'picocolors@1.1.1', - 'this-package-truly-does-not-exist-xyz-789@99.0.0', - ]); - assert.ok(result['picocolors'], 'good package must resolve despite bad neighbor'); - assert.match(result['picocolors'], /^https:\/\/ga\.jspm\.io\//); +test('jspmGenerate: per-package isolation, one bad install does not poison good ones', async () => { + // Mix a known-good package with a known-bad one. jspm 401s the WHOLE batch + // when any single install is unresolvable, but the good one MUST still + // resolve through the per-install probes. This is the regression test for + // the batched-call bug where one unresolvable dep collapsed the entire + // importmap. + // + // The double models the whole-batch 401 rather than answering a partial map, + // because that premise is what the probe path exists for. It is re-checked + // against the real API by `jspm-cdn.live.test.js`. + const bad = 'this-package-truly-does-not-exist-xyz-789@99.0.0'; + await withJspmDouble({ unresolvable: [bad] }, async () => { + const result = await jspmGenerate(['picocolors@1.1.1', bad]); + assert.ok(result['picocolors'], 'good package must resolve despite bad neighbor'); + assert.match(result['picocolors'], /^https:\/\/ga\.jspm\.io\//); + assert.equal(result['this-package-truly-does-not-exist-xyz-789'], undefined, + 'the unresolvable install must be dropped, not faked'); + }); }); /* ---------- #446: unified whole-set resolution + 401 fallback + parity ---------- */ @@ -659,111 +674,6 @@ test('jspmGenerate #446: a conflicting graph cannot skew a version (deterministi }); }); -test('jspmGenerate #446: matches jspm\'s own unified graph (real CDN)', { skip: !NETWORK_OK }, async (t) => { - // The integration half: our merged output must equal what jspm itself - // computes for the same install set. The mock above cannot prove this, - // because a mock only ever returns what this file already believes. - // - // The fixture is chosen so the comparison can actually FAIL two distinct - // ways, since a parity assertion over a set with nothing to disagree about - // is decoration: - // - // 1. Per-package skew. Resolved alone, @codemirror/lint drags in - // view@6.41.x; in the unified graph the pinned view@6.39.0 wins. So a - // revert of jspmGenerate to the pre-#446 per-package loop makes lint's - // isolated call supply the newer view, which wins last-write and diverges - // from the ground truth here. Two packages with no shared transitive - // (say picocolors + clsx) cannot catch that: their unified graph is - // byte-identical to the union of their single-install graphs. - // 2. A dropped flattenScope. This pair hoists five transitives to top level - // (@codemirror/state, crelt, style-mod, w3c-keyname, - // @marijn/find-cluster-break). vendor.js sends flattenScope: true so the - // browser gets no unresolved bare specifier, and this ground truth sends - // it too, so removing it from vendor.js drops those entries from our - // imports and reds the deepEqual. Nothing else in the suite covers that - // flag: every mock here answers only on `install`, so a mocked assertion - // on a transitive key reads a value the mock itself fabricated. - // - // lint is pinned at 6.9.5 rather than a version whose view range EXCLUDES - // 6.39.0 (only 6.9.6 and 6.9.7 do that, and neither resolves on jspm.io, see - // the mock test above). The incompatible-range case is the mock's job; this - // one only needs a shared transitive whose resolution differs per strategy. - const installs = ['@codemirror/view@6.39.0', '@codemirror/lint@6.9.5']; - - const skip = (reason) => { - // Loud on purpose. A silent skip is how a real regression hides, so name - // the fixture and the reason. - console.warn(`[vendor.test] SKIP unified-graph parity: ${installs.join(' + ')} (${String(reason).split('\n')[0]})`); - t.skip('jspm.io was not in a state that can answer this comparison'); - }; - - // Half one, the ground truth. Every failure mode routes to the skip, not - // just an `error` in a well-formed JSON body: a DNS failure or reset throws - // out of fetch, a proxy's HTML 502 throws out of .json(), and a hang is cut - // by the timeout. Without that timeout a wedged api.jspm.io would hold the - // unit job open until the CI job limit, since node --test applies no - // per-test deadline of its own. The shipped code guards its own call the - // same way (JSPM_GENERATE_TIMEOUT_MS in packages/server/src/vendor.js). - let gt; - let why = ''; - try { - const gtResp = await fetch('https://api.jspm.io/generate', { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ - install: installs, flattenScope: true, - env: ['browser', 'production', 'module'], provider: 'jspm.io', - }), - signal: AbortSignal.timeout(15_000), - }); - gt = await gtResp.json(); - if (!gtResp.ok) why = `HTTP ${gtResp.status}`; - else if (gt.error) why = String(gt.error); - else if (!gt.map?.imports) why = 'response carried no map.imports'; - } catch (err) { - why = `${err.name}: ${err.message}`; - } - if (why) { skip(why); return; } - - // Half two, our own call, watched at the TRANSPORT rather than judged by its - // return value. jspmGenerate fail-opens, so its output cannot tell the two - // failure kinds apart: a transient on the unified call returns a NON-empty - // merge of per-install fragments (skewed to view@6.41.x for this fixture), - // and an unresolvable set returns {}. Reading the map alone therefore either - // reds on an upstream blip or skips on a real bug, depending on which shape - // you test for. Both are wrong. - // - // So record what the network actually did. A throw, a 5xx, or a 429 is - // upstream having a bad moment, and skips. A 4xx does NOT skip: the ground - // truth just succeeded for this same fixture moments ago, so upstream is - // demonstrably healthy, and a 4xx now means OUR request is malformed, which - // is precisely the regression this test exists to catch. - const realFetch = globalThis.fetch; - /** @type {string[]} */ - const transient = []; - globalThis.fetch = async (url, opts) => { - try { - const r = await realFetch(url, opts); - if (r.status >= 500 || r.status === 429) transient.push(`HTTP ${r.status}`); - return r; - } catch (err) { - transient.push(`${err.name}: ${err.message}`); - throw err; - } - }; - let map; - try { - clearVendorCache(); - map = await jspmGenerate(installs); - } finally { - globalThis.fetch = realFetch; - } - if (transient.length) { skip(`jspm.io flaked on our own call (${transient[0]})`); return; } - - assert.deepEqual(map, gt.map.imports, - 'jspmGenerate must equal the single unified graph, not a per-package merge'); -}); - test('jspmGenerate #446 fallback: an unresolvable install does not collapse the map', async () => { // Preserve the per-package-isolation safety property. The unified call // 401s because one install (a private/server-only dep) is unresolvable. @@ -1001,12 +911,13 @@ test('vendorImportMapEntries: skips packages with no installed version', async ( assert.equal(entries['this-package-does-not-exist-xyz-456'], undefined); }); -test('vendorImportMapEntries: resolves installed packages to jspm.io URLs', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); - const entries = await vendorImportMapEntries(new Set(['picocolors']), process.cwd()); - const url = entries['picocolors']; - assert.ok(url, 'expected picocolors entry'); - assert.match(url, /^https:\/\/ga\.jspm\.io\/npm:picocolors@/); +test('vendorImportMapEntries: resolves installed packages to jspm.io URLs', async () => { + await withJspmDouble({}, async () => { + const entries = await vendorImportMapEntries(new Set(['picocolors']), process.cwd()); + const url = entries['picocolors']; + assert.ok(url, 'expected picocolors entry'); + assert.match(url, /^https:\/\/ga\.jspm\.io\/npm:picocolors@/); + }); }); // --- file-based pin (Rails-style committed importmap.json) --- @@ -1036,20 +947,51 @@ async function makeTempAppWithSource(sourceFiles) { return dir; } -test('pinAll default: writes importmap.json with jspm.io URLs', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll default: writes importmap.json with jspm.io URLs', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - const result = await pinAll(dir); - assert.ok(!result.failed, 'pin should not be flagged failed'); - assert.ok(result.pins.length >= 1, 'should pin picocolors'); - assert.equal(result.pruned.length, 0, 'no orphans on fresh pin'); - assert.equal(result.downloaded, 0, 'default mode does not download'); - const file = await readPinFile(dir); - assert.ok(file, 'pin file should exist'); - assert.match(file.imports['picocolors'], /^https:\/\/ga\.jspm\.io\/npm:picocolors@/); + await withJspmDouble({}, async () => { + const result = await pinAll(dir); + assert.ok(!result.failed, 'pin should not be flagged failed'); + assert.ok(result.pins.length >= 1, 'should pin picocolors'); + assert.equal(result.pruned.length, 0, 'no orphans on fresh pin'); + assert.equal(result.downloaded, 0, 'default mode does not download'); + const file = await readPinFile(dir); + assert.ok(file, 'pin file should exist'); + assert.match(file.imports['picocolors'], /^https:\/\/ga\.jspm\.io\/npm:picocolors@/); + }); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test('pinAll: a flattened transitive is pinned with a derivable bundle name (#446)', async () => { + // The unified resolve hoists a transitive to top level, and `pinAll` has to + // pin it even though it was never a directly scanned install: without it a + // pinned app's importmap is missing an entry the live path serves, and the + // browser hits an unresolved bare specifier. `partsByInstall` has no entry + // for such a spec, so `derivePinParts` recovers the version by locating + // `@` inside the resolved url. + // + // Nothing covered this before. Every pinAll test here resolves picocolors, + // which has no dependencies, so the live CDN never returned a transitive to + // exercise the path with. A double can just hand one over. + const dir = await makeTempAppWithSource({ + 'app/page.ts': `import pico from 'picocolors';`, + }); + try { + const transitive = 'https://ga.jspm.io/npm:tiny-dep@2.3.4/double.js'; + await withJspmDouble({ transitives: { 'tiny-dep': transitive } }, async () => { + const result = await pinAll(dir, { download: true }); + const file = await readPinFile(dir); + assert.ok(file.imports['picocolors'], 'the direct install still pins'); + assert.match(file.imports['tiny-dep'], /^\/__webjs\/vendor\/tiny-dep@2\.3\.4/, + 'the transitive pins under a filename derived from its resolved url'); + const pinned = result.pins.find((p) => p.pkg === 'tiny-dep'); + assert.equal(pinned.version, '2.3.4', 'version recovered from the url, not from the scan'); + }); } finally { await rm(dir, { recursive: true, force: true }); } @@ -1112,7 +1054,7 @@ test('pinAll: reports found-but-uninstalled specifiers instead of noBareImports } }); -test('pinAll: refuses to write empty pin file when every install fails', { skip: !NETWORK_OK }, async () => { +test('pinAll: refuses to write empty pin file when every install fails', async () => { // Regression: previously pinAll wrote `{ imports: {} }` when every // jspm.io call failed (e.g. brand-new package version not yet on // CDN, or unrelated transient errors). The empty pin file would @@ -1133,18 +1075,23 @@ test('pinAll: refuses to write empty pin file when every install fails', { skip: await writeFile(join(dir, 'app', 'page.ts'), `import x from 'fake-pkg-xyz-no-such-version';`); try { - const result = await pinAll(dir); - assert.ok(result.failed, 'pin must be flagged failed'); - assert.deepEqual(result.pins, [], 'no pins recorded'); - // Pin file MUST NOT have been written (so live API fallback runs next boot). - const file = await readPinFile(dir); - assert.equal(file, null, 'pin file must not exist after total failure'); + // Drive the failure through the double's unresolvable list rather than by + // relaxing anything: a refused pin is the CORRECT outcome here, and the + // assertions below are the contract, not an artifact of the CDN being down. + await withJspmDouble({ unresolvable: ['fake-pkg-xyz-no-such-version@99.99.99'] }, async () => { + const result = await pinAll(dir); + assert.ok(result.failed, 'pin must be flagged failed'); + assert.deepEqual(result.pins, [], 'no pins recorded'); + // Pin file MUST NOT have been written (so live API fallback runs next boot). + const file = await readPinFile(dir); + assert.equal(file, null, 'pin file must not exist after total failure'); + }); } finally { await rm(dir, { recursive: true, force: true }); } }); -test('pinAll: warns by name when some installs fail (partial success)', { skip: !NETWORK_OK }, async () => { +test('pinAll: warns by name when some installs fail (partial success)', async () => { // Regression for the partial-warning bug: the missing-installs // list was derived by filtering installs[] (versioned strings) // against pinnedSpecs (bare specs), which never matched. The warn @@ -1171,82 +1118,93 @@ test('pinAll: warns by name when some installs fail (partial success)', { skip: const origWarn = console.warn; console.warn = (...args) => { warns.push(args.join(' ')); }; try { - const result = await pinAll(dir); - // picocolors succeeded so pinAll proceeded; partial-warn must fire. - assert.equal(result.failed, undefined, 'partial success is not total failure'); - assert.ok(result.pins.length >= 1, 'at least picocolors made it into pins'); - const partial = warns.find(w => w.includes('partial success')); - assert.ok(partial, `expected partial-success warn; got warns:\n${warns.join('\n')}`); - const missingLines = warns.filter(w => w.includes('fake-pkg-xyz-no-such-version')); - assert.ok(missingLines.length > 0, 'fake-pkg-xyz must appear in the missing list'); - // The successful package must NOT appear in the missing list. - const wronglyListed = warns.find(w => - /^\s+picocolors@/.test(w) && !w.includes('partial success') - ); - assert.equal(wronglyListed, undefined, - 'successful packages must NOT appear in the missing list'); + await withJspmDouble({ unresolvable: ['fake-pkg-xyz-no-such-version@99.99.99'] }, async (double) => { + const result = await pinAll(dir); + // picocolors succeeded so pinAll proceeded; partial-warn must fire. + assert.equal(result.failed, undefined, 'partial success is not total failure'); + assert.ok(result.pins.length >= 1, 'at least picocolors made it into pins'); + const partial = warns.find(w => w.includes('partial success')); + assert.ok(partial, `expected partial-success warn; got warns:\n${warns.join('\n')}`); + const missingLines = warns.filter(w => w.includes('fake-pkg-xyz-no-such-version')); + assert.ok(missingLines.length > 0, 'fake-pkg-xyz must appear in the missing list'); + // The successful package must NOT appear in the missing list. + const wronglyListed = warns.find(w => + /^\s+picocolors@/.test(w) && !w.includes('partial success') + ); + assert.equal(wronglyListed, undefined, + 'successful packages must NOT appear in the missing list'); + // The exact trace the permanent-failure ladder takes, which only a + // counting double can see: the unified call 401s, both installs are + // probed alone, and the single survivor is then served from the probe's + // cache rather than re-resolved. Four calls would mean the survivor was + // fetched twice; two would mean the probes never ran. + assert.equal(double.generateCalls.length, 3, + `expected unified + two probes; got ${JSON.stringify(double.generateCalls.map(c => c.installs))}`); + }); } finally { console.warn = origWarn; await rm(dir, { recursive: true, force: true }); } }); -test('pinAll --download: writes importmap.json with local URLs + bundle files', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll --download: writes importmap.json with local URLs + bundle files', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - const { pins, downloaded } = await pinAll(dir, { download: true }); - assert.ok(pins.length >= 1); - assert.ok(downloaded >= 1, 'should download at least one bundle'); - const file = await readPinFile(dir); - assert.match(file.imports['picocolors'], /^\/__webjs\/vendor\/picocolors@.*\.js$/); - const bundleFilename = file.imports['picocolors'].slice('/__webjs/vendor/'.length); - const bytes = await readFileFs(join(dir, '.webjs', 'vendor', bundleFilename), 'utf8'); - assert.ok(bytes.length > 0, 'bundle file must contain bytes'); + await withJspmDouble({}, async () => { + const { pins, downloaded } = await pinAll(dir, { download: true }); + assert.ok(pins.length >= 1); + assert.ok(downloaded >= 1, 'should download at least one bundle'); + const file = await readPinFile(dir); + assert.match(file.imports['picocolors'], /^\/__webjs\/vendor\/picocolors@.*\.js$/); + const bundleFilename = file.imports['picocolors'].slice('/__webjs/vendor/'.length); + const bytes = await readFileFs(join(dir, '.webjs', 'vendor', bundleFilename), 'utf8'); + assert.ok(bytes.length > 0, 'bundle file must contain bytes'); + }); } finally { await rm(dir, { recursive: true, force: true }); } }); -test('pinAll: prune removes orphan bundle files from prior pins', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll: prune removes orphan bundle files from prior pins', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { await mkdir(join(dir, '.webjs', 'vendor'), { recursive: true }); await writeFile(join(dir, '.webjs', 'vendor', 'orphan-package@1.0.0.js'), 'export default {}'); - const { pruned } = await pinAll(dir); - assert.ok(pruned.includes('orphan-package@1.0.0.js'), `expected orphan in pruned list, got: ${pruned.join(', ')}`); + await withJspmDouble({}, async () => { + const { pruned } = await pinAll(dir); + assert.ok(pruned.includes('orphan-package@1.0.0.js'), `expected orphan in pruned list, got: ${pruned.join(', ')}`); + }); } finally { await rm(dir, { recursive: true, force: true }); } }); -test('pinAll: mode switch from --download to default removes bundles', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll: mode switch from --download to default removes bundles', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - const first = await pinAll(dir, { download: true }); - assert.ok(first.downloaded >= 1); - const second = await pinAll(dir); - assert.ok(second.pruned.length >= 1, 'switching to default mode should prune leftover bundle files'); + await withJspmDouble({}, async () => { + const first = await pinAll(dir, { download: true }); + assert.ok(first.downloaded >= 1); + const second = await pinAll(dir); + assert.ok(second.pruned.length >= 1, 'switching to default mode should prune leftover bundle files'); + }); } finally { await rm(dir, { recursive: true, force: true }); } }); -test('unpinPackage: removes entry from importmap.json (deletes file when last pin removed)', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('unpinPackage: removes entry from importmap.json (deletes file when last pin removed)', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - await pinAll(dir); + await withJspmDouble({}, () => pinAll(dir)); const r = await unpinPackage(dir, 'picocolors'); assert.equal(r.removed, true); // After the last pin is removed the pin file is deleted so the @@ -1698,40 +1656,42 @@ test('importMapTag: integrity field omitted when empty, present when populated', await setVendorEntries({}, {}); }); -test('pinAll default mode: writes integrity field alongside imports', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll default mode: writes integrity field alongside imports', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - await pinAll(dir); - const file = await readPinFile(dir); - assert.ok(file.integrity, 'integrity field should be written'); - const url = file.imports['picocolors']; - assert.ok(url, 'picocolors should pin'); - assert.match(file.integrity[url], /^sha384-/, 'integrity must be sha384 hash of fetched bundle'); + await withJspmDouble({}, async () => { + await pinAll(dir); + const file = await readPinFile(dir); + assert.ok(file.integrity, 'integrity field should be written'); + const url = file.imports['picocolors']; + assert.ok(url, 'picocolors should pin'); + assert.match(file.integrity[url], /^sha384-/, 'integrity must be sha384 hash of fetched bundle'); + }); } finally { await rm(dir, { recursive: true, force: true }); } }); -test('pinAll --download: writes integrity matching the on-disk bytes', { skip: !NETWORK_OK }, async () => { - clearVendorCache(); +test('pinAll --download: writes integrity matching the on-disk bytes', async () => { const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';`, }); try { - await pinAll(dir, { download: true }); - const file = await readPinFile(dir); - assert.ok(file.integrity, 'integrity field should be written'); - const localUrl = file.imports['picocolors']; - assert.match(localUrl, /^\/__webjs\/vendor\//); - assert.match(file.integrity[localUrl], /^sha384-/, 'integrity must match downloaded bytes'); - // Recompute hash from the on-disk file to prove it matches. - const { sha384Integrity } = await import('../../src/vendor.js'); - const filename = localUrl.slice('/__webjs/vendor/'.length); - const onDisk = await readFileFs(join(dir, '.webjs', 'vendor', filename), 'utf8'); - assert.equal(file.integrity[localUrl], await sha384Integrity(onDisk)); + await withJspmDouble({}, async () => { + await pinAll(dir, { download: true }); + const file = await readPinFile(dir); + assert.ok(file.integrity, 'integrity field should be written'); + const localUrl = file.imports['picocolors']; + assert.match(localUrl, /^\/__webjs\/vendor\//); + assert.match(file.integrity[localUrl], /^sha384-/, 'integrity must match downloaded bytes'); + // Recompute hash from the on-disk file to prove it matches. + const { sha384Integrity } = await import('../../src/vendor.js'); + const filename = localUrl.slice('/__webjs/vendor/'.length); + const onDisk = await readFileFs(join(dir, '.webjs', 'vendor', filename), 'utf8'); + assert.equal(file.integrity[localUrl], await sha384Integrity(onDisk)); + }); } finally { await rm(dir, { recursive: true, force: true }); } @@ -1920,7 +1880,13 @@ test('updatePinned: respects pin file provider when --from is not passed', async }), ); try { - const result = await updatePinned(dir); + // registry.npmjs.org is not part of what this asserts, and reaching it made + // an unrelated outage able to stall the required job for ten seconds here + // (#1150). A 404 exercises the same read path. + const result = await withMockedFetch( + async () => /** @type any */ ({ ok: false, status: 404, json: async () => ({}) }), + () => updatePinned(dir), + ); assert.equal(result.provider, 'jsdelivr', 'updatePinned must use the pin file provider when no --from passed'); } finally { @@ -1939,7 +1905,10 @@ test('updatePinned: explicit --from overrides pin file provider', async () => { }), ); try { - const result = await updatePinned(dir, { from: 'unpkg' }); + const result = await withMockedFetch( + async () => /** @type any */ ({ ok: false, status: 404, json: async () => ({}) }), + () => updatePinned(dir, { from: 'unpkg' }), + ); assert.equal(result.provider, 'unpkg', 'explicit opts.from must override pin file provider'); } finally { @@ -1947,11 +1916,10 @@ test('updatePinned: explicit --from overrides pin file provider', async () => { } }); -test('auditPinned: surfaces network failure as errored:true', { skip: !NETWORK_OK }, async () => { +test('auditPinned: surfaces network failure as errored:true', async () => { // The audit command must NOT silently report "no vulnerabilities" - // when the registry call failed. Use an obviously-unresolvable - // hostname by stubbing the global fetch for the duration of the - // test. Fail-closed contract: errored:true means the user must + // when the registry call failed. Never network-bound despite the gate it + // used to carry: it stubs the global fetch for its own duration. Fail-closed contract: errored:true means the user must // retry. const dir = join(tmpdir(), `webjs-audit-err-${Date.now()}`); await mkdir(join(dir, '.webjs', 'vendor'), { recursive: true }); diff --git a/test/fixtures/jspm-double.mjs b/test/fixtures/jspm-double.mjs index 9d33f20b5..3321aab29 100644 --- a/test/fixtures/jspm-double.mjs +++ b/test/fixtures/jspm-double.mjs @@ -190,13 +190,15 @@ export function jspmDouble(opts = {}) { return json(599, { error: 'Error: the jspm double does not proxy to the network' }); } - return Object.assign(doubledFetch, { - calls, + // A getter has to be DEFINED rather than assigned. `Object.assign` reads a + // source accessor and copies its VALUE, so a `get generateCalls()` in an + // object literal here would freeze to the empty array it returns at + // construction, and every call count would silently read zero. + Object.defineProperty(doubledFetch, 'generateCalls', { /** Just the `/generate` calls, which is what a round-trip count means. */ - get generateCalls() { return calls.filter((c) => c.url.startsWith(GENERATE_ENDPOINT)); }, - unexpected, - minted, + get() { return calls.filter((c) => c.url.startsWith(GENERATE_ENDPOINT)); }, }); + return Object.assign(doubledFetch, { calls, unexpected, minted }); } /** From 34f84acfc2f04ab05b27a94ddd70d829f66a214a Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:07:41 +0530 Subject: [PATCH 04/18] test: gate live-CDN tests behind a filename both runners honour A *.live.test.* file is the only place allowed to reach a third party, and run-node-tests.js and run-bun-tests.js both drop those unless WEBJS_REQUIRE_NETWORK is set. Measured: the node runner selects 364 files normally and 366 with the variable, the bun matrix 0 live and 2. This is what the NETWORK_OK gate could never be. That gate was opt-OUT, so CI, which never set it, always ran live; it was convention rather than a rule the runner could enforce; and it missed two registry.npmjs.org callers entirely. Leaving the parity test gated in place would not have fixed anything either, since after #1219 it still reds on a 4xx, and a WAF 403 or a moved route is exactly the shape #1149 hit. ci.yml is untouched on purpose. Eleven jobs share its trigger block and #1135 and #1257 are already editing it, so the filter belongs in the runners. The vendor suite rejoins the Bun matrix, which the exclusion had blocked as network-bound. All five files pass, and that is worth having: the double is a globalThis.fetch swap, which is where the two runtimes are most likely to diverge. A live pin test keeps one real run of the CLI command, so what a user actually types is still exercised against the real CDN nightly. The preload comment is corrected while here. Node rejects --preload outright, but Bun does accept --import as an alias, so the two are not symmetric the way the first draft claimed. Selecting per runtime is still right, since it keeps this from depending on Bun continuing to accept a Node spelling, and Bun does ignore NODE_OPTIONS entirely, which is the part that rules out an env var. --- scripts/run-bun-tests.js | 28 ++++++-- scripts/run-node-tests.js | 10 ++- test/fixtures/jspm-double-preload.mjs | 4 +- test/vendor-cli/vendor-cli.test.mjs | 12 +++- test/vendor-cli/vendor-pin.live.test.mjs | 82 ++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 test/vendor-cli/vendor-pin.live.test.mjs diff --git a/scripts/run-bun-tests.js b/scripts/run-bun-tests.js index c69ec8e3f..d3af2ceef 100644 --- a/scripts/run-bun-tests.js +++ b/scripts/run-bun-tests.js @@ -4,7 +4,7 @@ * * Runs the runtime-sensitive `node:test` files (under `test/`, `packages/core/test/`, * and `packages/server/test/`, excluding `browser/`, the `e2e/` gate, and the - * network-bound `vendor/` suite) under Bun, file by file via `bun test `. + * live-CDN `*.live.test.*` files) under Bun, file by file via `bun test `. * * SOUNDNESS: the runner does NOT classify failures into skips (a self-classifying * runner can silently hide a real bug behind a "skip", which defeats the purpose). @@ -80,12 +80,25 @@ walk(join(ROOT, 'packages', 'core', 'test'), all); walk(join(ROOT, 'packages', 'server', 'test'), all); const SEP = sep; -// Exclude browser (needs wtr), e2e (gated), the network-bound vendor suite, and -// the example-app smoke/probe tests (test/examples/**), which boot a real app -// that needs a migrated Drizzle DB + jspm vendor resolution the matrix job does -// not provision (the dedicated e2e / in-repo-app CI jobs do; on Bun a real app -// boot is covered deterministically by the test/bun/*.mjs scripts). -const excludeSegs = [`${SEP}browser${SEP}`, `${SEP}e2e${SEP}`, `${SEP}vendor${SEP}`, `${SEP}examples${SEP}`]; +// Exclude browser (needs wtr), e2e (gated), and the example-app smoke/probe +// tests (test/examples/**), which boot a real app that needs a migrated Drizzle +// DB + jspm vendor resolution the matrix job does not provision (the dedicated +// e2e / in-repo-app CI jobs do; on Bun a real app boot is covered +// deterministically by the test/bun/*.mjs scripts). +// +// `packages/server/test/vendor/` used to be excluded here as network-bound. +// That stopped being true in #1150: the suite resolves through an offline +// double now, and it is worth running on Bun precisely BECAUSE that double is a +// `globalThis.fetch` swap, which is the kind of thing the two runtimes are most +// likely to disagree about. +const excludeSegs = [`${SEP}browser${SEP}`, `${SEP}e2e${SEP}`, `${SEP}examples${SEP}`]; + +// Live third-party calls live only in `*.live.test.*` files, and those are +// opt-in (#1150). A jspm outage must never be able to red a required check, so +// the matrix skips them unless a caller explicitly asks for the network. The +// nightly `vendor-cdn` workflow is what asks. +const LIVE_MARKER = '.live.test.'; +const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK); const filter = (process.env.WEBJS_BUN_TESTS || '').split(',').map((s) => s.trim()).filter(Boolean); // Repo-relative path, always forward-slashed so DENYLIST matching is OS-stable. const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); @@ -95,6 +108,7 @@ const denyOf = (f) => DENYLIST.find((d) => (d.match.endsWith('/') ? rel(f).start const files = all .filter((f) => !excludeSegs.some((s) => f.includes(s))) + .filter((f) => wantsNetwork || !f.includes(LIVE_MARKER)) .filter((f) => filter.length === 0 || filter.some((q) => f.includes(q))) .sort(); diff --git a/scripts/run-node-tests.js b/scripts/run-node-tests.js index 03e5d8fc3..bc544ce06 100644 --- a/scripts/run-node-tests.js +++ b/scripts/run-node-tests.js @@ -57,10 +57,18 @@ for (const pkg of readdirSync(packagesDir, { withFileTypes: true })) { const SEP = sep; const browserSeg = `${SEP}browser${SEP}`; const e2eSeg = `${SEP}e2e${SEP}`; +// Live third-party calls live only in `*.live.test.*` files, and those are +// opt-in (#1150). This job is REQUIRED, so a jspm or npm-registry outage must +// not be able to red it; a documentation-only PR was blocked that way on +// #1149. The nightly `vendor-cdn` workflow sets WEBJS_REQUIRE_NETWORK to run +// them for real, where a skip is promoted to a failure. +const LIVE_MARKER = '.live.test.'; +const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK); const files = all .filter((f) => !f.includes(browserSeg)) - .filter((f) => !f.includes(e2eSeg)); + .filter((f) => !f.includes(e2eSeg)) + .filter((f) => wantsNetwork || !f.includes(LIVE_MARKER)); if (!files.length) { console.log('[run-node-tests] no test files matched.'); diff --git a/test/fixtures/jspm-double-preload.mjs b/test/fixtures/jspm-double-preload.mjs index 9d2e474b1..38fc37666 100644 --- a/test/fixtures/jspm-double-preload.mjs +++ b/test/fixtures/jspm-double-preload.mjs @@ -7,8 +7,8 @@ * gap: the test passes it as `--import` (Node) or `--preload` (Bun) ahead of * the CLI path, and it patches `globalThis.fetch` before any application code * runs. It is loaded as a runtime flag rather than through `NODE_OPTIONS` - * because Bun ignores `NODE_OPTIONS` and neither runtime honours the other's - * flag, the lesson `test/e2e/e2e.test.mjs` already carries for #1229's stub. + * because Bun ignores that variable outright, the lesson + * `test/e2e/e2e.test.mjs` already carries for #1229's stub. * * Two signals go to stderr, and both are load-bearing. * diff --git a/test/vendor-cli/vendor-cli.test.mjs b/test/vendor-cli/vendor-cli.test.mjs index ccf70282e..70760bad7 100644 --- a/test/vendor-cli/vendor-cli.test.mjs +++ b/test/vendor-cli/vendor-cli.test.mjs @@ -33,10 +33,16 @@ statSync(PRELOAD); /** * The flag that loads the preload into the child, chosen by runtime. * - * Node and Bun each ignore the other's spelling, and Bun ignores NODE_OPTIONS - * entirely, so this cannot be one hardcoded flag or an env var. The parent + * It cannot be NODE_OPTIONS, because Bun ignores that variable outright + * (measured: `NODE_OPTIONS=--import ... bun -e 0` loads nothing). The parent * runtime IS the child runtime here, because the spawn below uses - * `process.execPath`, which under `bun test` is the bun binary. Node wants a + * `process.execPath`, which under `bun test` is the bun binary. + * + * The flags are not symmetric. `node --preload` is a hard `bad option` error, + * while `bun --import` currently works as an alias, so a bare `--import` would + * in fact run on both today. Selecting per runtime anyway is the same choice + * `test/e2e/e2e.test.mjs` made for #1229's stub, and it means this file does + * not silently depend on Bun continuing to accept a Node spelling. Node wants a * URL rather than a path, since the spawn sets `cwd` to the temp app directory * and a relative `--import` would resolve against that instead of the repo. */ diff --git a/test/vendor-cli/vendor-pin.live.test.mjs b/test/vendor-cli/vendor-pin.live.test.mjs new file mode 100644 index 000000000..b5411cdc4 --- /dev/null +++ b/test/vendor-cli/vendor-pin.live.test.mjs @@ -0,0 +1,82 @@ +/** + * `webjs vendor pin` against the real jspm CDN (#1150). + * + * `vendor-cli.test.mjs` preloads an offline double into every CLI child, which + * is what keeps a jspm outage from redding the required CI job. The cost is + * that the command a user actually runs would otherwise stop being exercised + * end to end anywhere, against anything real. This file is that one real run. + * + * Both test runners skip `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, so + * this never runs in a required check. `.github/workflows/vendor-cdn.yml` runs + * it nightly with that variable set, where a skip is promoted to a failure. + * + * It asserts only what a live resolve is uniquely able to prove: that jspm + * answers with a url of the shape the pin file expects, and that the bytes + * behind that url hash into an SRI value. Everything about pin file structure, + * pruning, gitignore healing, and the failure paths belongs in the offline + * file, where it is deterministic. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { mkdtemp, writeFile, mkdir, readFile, rm, symlink } from 'node:fs/promises'; +import { join, resolve, dirname } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = resolve(__dirname, '..', '..'); +const CLI = resolve(REPO_ROOT, 'packages', 'cli', 'bin', 'webjs.js'); + +/** Deliberately NO preload here. This one is supposed to reach the network. */ +function runCli(args, cwd) { + return new Promise((res, rej) => { + const child = spawn(process.execPath, [CLI, ...args], { + cwd, + env: { ...process.env, FORCE_COLOR: '0' }, + }); + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (d) => { stdout += d.toString(); }); + child.stderr.on('data', (d) => { stderr += d.toString(); }); + child.on('exit', (code) => res({ code, stdout, stderr })); + child.on('error', rej); + }); +} + +test('pin resolves picocolors against the real CDN and hashes the bytes', async (t) => { + const dir = await mkdtemp(join(tmpdir(), 'webjs-vendor-live-')); + try { + await symlink(join(REPO_ROOT, 'node_modules'), join(dir, 'node_modules')); + await writeFile(join(dir, 'package.json'), '{"name":"tmp","version":"0.0.0"}'); + await mkdir(join(dir, 'app'), { recursive: true }); + await writeFile(join(dir, 'app', 'page.ts'), + `import pico from 'picocolors';\nexport default () => pico.green('ok');`); + + const { code, stdout, stderr } = await runCli(['vendor', 'pin'], dir); + if (code !== 0) { + // A failed pin here is upstream trouble far more often than a regression, + // and the CLI already names the reason. Under WEBJS_REQUIRE_NETWORK the + // nightly wants to know, so fail loudly there instead of skipping. + const why = `exit ${code}: ${(stderr || stdout).split('\n').filter(Boolean).slice(-1)[0] || 'no output'}`; + if (process.env.WEBJS_REQUIRE_NETWORK) { + assert.fail(`live \`webjs vendor pin\` could not run (${why})`); + } + console.warn(`[vendor-pin.live] SKIP live pin (${why})`); + t.skip('jspm.io was not in a state that can answer a pin'); + return; + } + + const parsed = JSON.parse(await readFile(join(dir, '.webjs', 'vendor', 'importmap.json'), 'utf8')); + const url = parsed.imports.picocolors; + assert.match(url, /^https:\/\/ga\.jspm\.io\/npm:picocolors@\d+\.\d+\.\d+\//, + 'a real resolve must carry a concrete version in a jspm CDN url'); + // The offline double mints this tail, so its absence is what proves this + // run really went to the network rather than picking up a stray preload. + assert.doesNotMatch(url, /\/double\.js$/, 'this test must NOT be running against the double'); + assert.match(parsed.integrity[url], /^sha384-/, + 'the bundle behind the resolved url must have been fetched and hashed'); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); From c2ad8831fda0f6912e68b72f11aed9d3229e80e1 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:12:02 +0530 Subject: [PATCH 05/18] test: add the nightly live-CDN job and the two guards that keep it honest The nightly runs the live files with WEBJS_REQUIRE_NETWORK, which both selects them and turns their upstream-trouble skip into a failure. That second half is the point: a permanently skipping test is indistinguishable from a passing one, which is how live coverage rots into decoration. It has no pull_request trigger, so it can never become a required check. A failure opens or comments on one fixed-title tracking issue rather than creating a new one per night, because GitHub notifies only the workflow file's last committer about a failed scheduled run, and a nightly nobody watches is a placebo. The double gets its own contract test, in the spirit of the one #1229's fixture carries. A double that answers the wrong shape makes the vendor tests pass for the wrong reason, which is strictly worse than the live dependency it replaced. It pins the url shape derivePinParts has to be able to read, the whole-batch 401, the recorded refusal, and the live generateCalls getter (which caught a real bug in the double while this was being written: Object.assign copies a getter's value, so every round-trip count read zero). The policy guard asserts the PROPERTY rather than a spelling. Counting occurrences of some marker constant would certify nothing, since a new test can call fetch without it, and would red on a rename. So it looks for a third-party host inside a fetch call and for the vendor entry points that reach one internally, exempting any file that installs a fetch it owns. Verified: dropping a live fetch into an ordinary test file reds it. --- .github/workflows/vendor-cdn.yml | 96 +++++++++++ test/repo-health/jspm-double.test.mjs | 191 +++++++++++++++++++++ test/repo-health/live-cdn-callers.test.mjs | 185 ++++++++++++++++++++ 3 files changed, 472 insertions(+) create mode 100644 .github/workflows/vendor-cdn.yml create mode 100644 test/repo-health/jspm-double.test.mjs create mode 100644 test/repo-health/live-cdn-callers.test.mjs diff --git a/.github/workflows/vendor-cdn.yml b/.github/workflows/vendor-cdn.yml new file mode 100644 index 000000000..1096ca7bf --- /dev/null +++ b/.github/workflows/vendor-cdn.yml @@ -0,0 +1,96 @@ +name: Vendor CDN contract (nightly) + +# Runs the only tests that talk to the real jspm CDN (#1150). +# +# Why they are not in CI: the required `Unit + integration` job used to resolve +# vendors live, so a jspm outage redded pull requests that had nothing to do +# with vendoring. PR #1149, a five-file documentation change, was blocked that +# way and passed on a re-run of the identical commit. Both test runners now +# skip `*.live.test.*` unless WEBJS_REQUIRE_NETWORK is set. +# +# Why they still exist somewhere: deleting the live coverage was never the +# goal. The vendor resolver's whole job is to talk to jspm, and the offline +# double can only ever return what this repo already believes about the API. +# Two things it cannot vouch for are checked here for real: that our merged +# output equals jspm's own unified graph (#446), and that jspm still fails a +# WHOLE batch permanently when one install is unresolvable, which is the +# premise the entire per-package fallback ladder in vendor.js rests on. +# +# Why nightly rather than on a pull request: a live check on a PR is a live +# check, whatever job it sits in. Moving it to a non-required job on the +# `pull_request` trigger would still queue on every PR and still go red on an +# outage; it would just be a red somebody is told to ignore, which is how a +# real failure gets ignored too. +# +# WEBJS_REQUIRE_NETWORK does double duty. It selects the live files, and it +# turns their upstream-trouble SKIP into a FAILURE. Without that second half a +# permanently skipping test is indistinguishable from a passing one, which is +# the exact way live coverage rots into decoration. +# +# There is deliberately no `pull_request` trigger, so this can never become a +# required check and can never block a merge. + +on: + schedule: + # 04:20 UTC daily. Off the hour on purpose: GitHub queues scheduled jobs + # from every repository at :00, so an on-the-hour cron is the one most + # likely to be delayed or dropped. + - cron: '20 4 * * *' + workflow_dispatch: + +permissions: + contents: read + # Needed by the failure step below, which is what stops this from being a + # job nobody watches. GitHub notifies only the workflow file's last + # committer about a failed scheduled run. + issues: write + +concurrency: + group: vendor-cdn + cancel-in-progress: false + +jobs: + live: + name: Live jspm contract + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-node@v6 + with: + node-version: '24' + cache: npm + - run: npm ci + - name: Run the live CDN tests + env: + WEBJS_REQUIRE_NETWORK: '1' + run: | + node --test \ + packages/server/test/vendor/jspm-cdn.live.test.js \ + test/vendor-cli/vendor-pin.live.test.mjs + + - name: Report a failure on the tracking issue + if: failure() + env: + GH_TOKEN: ${{ github.token }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + TITLE='Nightly live jspm contract check is failing' + # One issue, reopened and commented rather than duplicated, so a week + # of failures is one thread instead of seven issues. + NUM=$(gh issue list --state all --search "$TITLE in:title" \ + --json number,title \ + --jq "[.[] | select(.title == \"$TITLE\")] | first | .number // empty") + BODY="The nightly live jspm contract check failed: $RUN_URL + + Either jspm changed something the resolver depends on, or the fixture + the parity test pins has stopped resolving. Neither blocks a merge: + nothing here runs in a required check. See the header of + \`packages/server/test/vendor/jspm-cdn.live.test.js\` for what the two + tests assert and why they are live." + if [ -n "$NUM" ]; then + gh issue reopen "$NUM" || true + gh issue comment "$NUM" --body "$BODY" + else + gh issue create --title "$TITLE" --label bug --assignee vivek7405 --body "$BODY" + fi diff --git a/test/repo-health/jspm-double.test.mjs b/test/repo-health/jspm-double.test.mjs new file mode 100644 index 000000000..452388005 --- /dev/null +++ b/test/repo-health/jspm-double.test.mjs @@ -0,0 +1,191 @@ +/** + * The offline jspm double (#1150), tested on its own. + * + * `test/fixtures/jspm-double.mjs` is what keeps a jspm outage from redding the + * required CI job, and it is the kind of thing that can rot silently: a double + * that answers the wrong shape makes the vendor tests pass for the wrong + * reason, which is strictly worse than the live dependency it replaced. So the + * contract it owes `packages/server/src/vendor.js` is pinned here, in the same + * spirit as `e2e-vendor-stub.test.mjs` pins #1229's fixture. + * + * Three properties carry most of the weight. + * + * The URL SHAPE. `pinAll` recovers a flattened transitive's version by locating + * `@` inside the resolved url (`derivePinParts`), and derives a + * `--download` filename from it. A double that dropped the version, or that + * collapsed a subpath into the package name, would make `pinAll` report a + * failure that looks like a product bug. + * + * The WHOLE-BATCH 401. Real jspm fails the entire call when any one install is + * unresolvable, and `jspmGenerate`'s per-install probing exists only because of + * that. A double that answered a partial map would leave the probing untested. + * The premise itself is re-checked against the real API by + * `packages/server/test/vendor/jspm-cdn.live.test.js`. + * + * The REFUSAL. Every fetch caller in vendor.js catches, so an unserved request + * cannot be signalled by throwing: it would be indistinguishable from the CDN + * being down, and would quietly weaken whatever test hit it. It is recorded + * instead, and `withJspmDouble` fails the test on any recorded entry. + * + * This file installs no global fetch of its own and calls the double directly, + * so it is network-free by construction rather than by discipline. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { jspmDouble } from '../fixtures/jspm-double.mjs'; +import { packageName, packageVersion, subpath, importKey } from '../fixtures/install-spec.mjs'; + +const GENERATE = 'https://api.jspm.io/generate'; + +/** The body `vendor.js` posts, so the double is exercised through its real shape. */ +const generate = (double, install) => double(GENERATE, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + install, flattenScope: true, env: ['browser', 'production', 'module'], provider: 'jspm.io', + }), +}); + +test('an install string yields its package name, version, and subpath', () => { + // All four shapes jspm accepts, each in bare and scoped form. The version is + // OPTIONAL, so the unversioned-with-subpath rows are the ones that catch a + // parser assuming a subpath always rides behind a version. They are also the + // rows the `install.replace(/@[^@]*$/, '')` shortcut gets wrong, which is + // why this parse is shared rather than rewritten per fixture. + const cases = [ + ['dayjs', 'dayjs', '', ''], + ['dayjs@1.11.21', 'dayjs', '1.11.21', ''], + ['dayjs/plugin/utc', 'dayjs', '', '/plugin/utc'], + ['dayjs@1.11.21/plugin/utc', 'dayjs', '1.11.21', '/plugin/utc'], + ['@scope/pkg', '@scope/pkg', '', ''], + ['@scope/pkg@1.0.0', '@scope/pkg', '1.0.0', ''], + ['@scope/pkg/sub', '@scope/pkg', '', '/sub'], + ['@scope/pkg@1.0.0/sub', '@scope/pkg', '1.0.0', '/sub'], + ]; + for (const [install, name, version, sub] of cases) { + assert.equal(packageName(install), name, `name of ${install}`); + assert.equal(packageVersion(install), version, `version of ${install}`); + assert.equal(subpath(install), sub, `subpath of ${install}`); + assert.equal(importKey(install), `${name}${sub}`, `import key of ${install}`); + } +}); + +test('a generate call answers a map keyed the way the browser looks entries up', async () => { + const double = jspmDouble(); + const res = await generate(double, ['picocolors@1.1.1', '@scope/pkg@2.0.0/sub']); + assert.equal(res.status, 200); + const { map } = await res.json(); + + // Keyed on name + subpath and never on the version, because that is what + // appears in source: `import x from '@scope/pkg/sub'`. + assert.deepEqual(Object.keys(map.imports).sort(), ['@scope/pkg/sub', 'picocolors']); + assert.equal(map.imports['picocolors'], 'https://ga.jspm.io/npm:picocolors@1.1.1/double.js'); + assert.equal(map.imports['@scope/pkg/sub'], 'https://ga.jspm.io/npm:@scope/pkg@2.0.0/sub/double.js'); + assert.equal(double.unexpected.length, 0); +}); + +test('the minted url keeps name@version parseable, which pinAll depends on', async () => { + // `derivePinParts` locates `@` in the resolved url to recover + // a transitive's version. Assert that literally, since a url shape that only + // LOOKS jspm-ish would pass every other test here and fail inside pinAll. + const double = jspmDouble(); + const { map } = await (await generate(double, ['@codemirror/view@6.39.0/dist/index.js'])).json(); + const url = map.imports['@codemirror/view/dist/index.js']; + const match = new RegExp('(?:^|[^a-zA-Z0-9_.-])@codemirror/view@([^/]+)').exec(url); + assert.ok(match, `derivePinParts must be able to read a version out of ${url}`); + assert.equal(match[1], '6.39.0'); +}); + +test('the /double.js tail is what proves a resolve did not go to the network', async () => { + // Real jspm never emits this, so it is the only part of the url a wiring + // assertion can key on. `vendor-cli.test.mjs` asserts it for exactly that. + const double = jspmDouble(); + const { map } = await (await generate(double, ['picocolors@1.1.1'])).json(); + assert.match(map.imports.picocolors, /\/double\.js$/); +}); + +test('one unresolvable install fails the WHOLE batch, not just its own entry', async () => { + const bad = 'nope-xyz@9.9.9'; + const double = jspmDouble({ unresolvable: [bad] }); + + const mixed = await generate(double, ['picocolors@1.1.1', bad]); + assert.equal(mixed.status, 401, 'a batch carrying an unresolvable install must fail entirely'); + assert.equal((await mixed.json()).error, 'Error: Not Found'); + + // And the resolvable one still succeeds when probed alone, which is the half + // that makes jspmGenerate's per-install fallback able to recover anything. + const alone = await generate(double, ['picocolors@1.1.1']); + assert.equal(alone.status, 200); + assert.equal(double.unexpected.length, 0); +}); + +test('a forced transient status is distinguishable from a permanent one', async () => { + // vendor.js treats >= 500 and 429 as transient and retries per package; + // everything else drops the install. The double has to be able to produce + // both sides or the transient branch cannot be tested at all. + for (const status of [503, 429]) { + const double = jspmDouble({ status }); + const res = await generate(double, ['picocolors@1.1.1']); + assert.equal(res.status, status); + } +}); + +test('a minted bundle url serves bytes with a JavaScript content type', async () => { + // downloadBundle and fetchIntegrity both GET the resolved url, one to write + // it to disk and one to hash it, so an answer with no body would make every + // integrity assertion vacuous. + const double = jspmDouble({ bundle: 'export default 1;\n' }); + const { map } = await (await generate(double, ['picocolors@1.1.1'])).json(); + const res = await double(map.imports.picocolors); + assert.equal(res.status, 200); + assert.equal(res.headers.get('content-type'), 'text/javascript'); + assert.equal(await res.text(), 'export default 1;\n'); + assert.equal(double.unexpected.length, 0); +}); + +test('a jspm url the double never minted is RECORDED, not passed through', async () => { + const double = jspmDouble(); + const res = await double('https://ga.jspm.io/npm:never-minted@1.0.0/index.js'); + assert.equal(res.status, 599, 'the double must answer rather than reach the network'); + assert.equal(double.unexpected.length, 1); + assert.match(double.unexpected[0], /never-minted/); +}); + +test('registry.npmjs.org is owned too, so an audit or update call cannot slip out', async () => { + const double = jspmDouble(); + await double('https://registry.npmjs.org/-/npm/v1/security/advisories/bulk', { method: 'POST' }); + assert.equal(double.unexpected.length, 1); + assert.match(double.unexpected[0], /registry\.npmjs\.org/); +}); + +test('a body the double cannot read is refused, never answered with an empty map', async () => { + // The silent failure this fixture exists to remove: an absent importmap + // entry is an unresolved bare specifier that kills a page's whole module + // graph, so answering `{}` would be worse than answering nothing. + for (const init of [ + { method: 'POST' }, + { method: 'POST', body: new Uint8Array([1, 2, 3]) }, + { method: 'POST', body: '{ not json' }, + { method: 'POST', body: JSON.stringify({ install: [] }) }, + ]) { + const double = jspmDouble(); + const res = await double(GENERATE, init); + assert.equal(res.status, 400, `expected a refusal for ${JSON.stringify(init.body ?? null)}`); + assert.equal(double.unexpected.length, 1); + } +}); + +test('generateCalls counts only generate calls, and counts them live', async () => { + // It is a getter over a growing array. Assigning it with Object.assign would + // snapshot the empty value at construction, which silently turns every + // round-trip assertion in the vendor suite into `0 === 0`. That is not + // hypothetical; it happened while building this. + const double = jspmDouble(); + assert.equal(double.generateCalls.length, 0); + const { map } = await (await generate(double, ['picocolors@1.1.1'])).json(); + assert.equal(double.generateCalls.length, 1); + await double(map.imports.picocolors); + assert.equal(double.generateCalls.length, 1, 'a bundle GET is not a generate call'); + assert.equal(double.calls.length, 2, 'but it is still a call'); +}); diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs new file mode 100644 index 000000000..bcd1b6b0c --- /dev/null +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -0,0 +1,185 @@ +/** + * Live third-party calls belong only in `*.live.test.*` files (#1150). + * + * That is the whole policy, and it is worth enforcing mechanically because the + * failure it prevents is invisible: a test that quietly reaches api.jspm.io or + * registry.npmjs.org passes every day until the third party has a bad hour, + * and then reds a pull request that never touched vendoring. PR #1149, a + * five-file documentation change, is what finally made the case. The old + * `WEBJS_SKIP_NETWORK_TESTS` convention could not prevent it: it was opt-OUT, + * so CI always ran live, and two `registry.npmjs.org` callers were never + * covered by it at all. + * + * The rule is enforced on the PROPERTY, not on a spelling. Asserting that some + * marker constant appears exactly once would certify nothing (a new test can + * always call fetch without it) and would red on a rename or a reformat. So + * this looks for the actual live surface: a third-party host inside a `fetch(` + * call, and the vendor entry points that reach one internally. + * + * Both test runners drop `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, so + * a file on the allowlist below genuinely cannot run in a required check. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { readdirSync, readFileSync } from 'node:fs'; +import { join, resolve, dirname, sep } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); + +/** + * Files allowed to reach a third party, each with what it asserts and why it + * has to be live. Shaped like `scripts/run-bun-tests.js`'s DENYLIST on + * purpose: a reason per entry, so adding one is a decision somebody wrote down + * rather than a guard somebody silenced. + * + * Every entry MUST be a `*.live.test.*` path, which is what the runners key on. + */ +const LIVE_CALLERS = [ + { + file: 'packages/server/test/vendor/jspm-cdn.live.test.js', + why: 'the two things an offline double cannot vouch for: that our merged output equals ' + + "jspm's own unified graph (#446), and that jspm still fails a WHOLE batch permanently " + + 'when one install is unresolvable, which the entire fallback ladder in vendor.js assumes.', + }, + { + file: 'test/vendor-cli/vendor-pin.live.test.mjs', + why: 'one real run of the command a user actually types, so `webjs vendor pin` does not ' + + 'become a thing that is only ever exercised against a fixture.', + }, +]; + +/** Third-party hosts no required check may depend on. */ +const LIVE_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; + +/** + * Vendor entry points that reach a live host internally, so naming one is as + * live as calling fetch. Each is allowed inside a `withMockedFetch` or + * `withJspmDouble` body, which is how the offline suites use them. + */ +const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'findOutdated']; + +const LIVE_MARKER = '.live.test.'; + +/** @param {string} dir @param {string[]} out */ +function walk(dir, out) { + let entries; + try { entries = readdirSync(dir, { withFileTypes: true }); } + catch { return; } + for (const e of entries) { + if (e.name === 'node_modules' || e.name === '.git') continue; + const full = join(dir, e.name); + if (e.isDirectory()) walk(full, out); + else if (e.isFile() && (e.name.endsWith('.test.js') || e.name.endsWith('.test.mjs'))) out.push(full); + } +} + +const files = []; +walk(join(ROOT, 'test'), files); +for (const pkg of readdirSync(join(ROOT, 'packages'), { withFileTypes: true })) { + if (!pkg.isDirectory()) continue; + walk(join(ROOT, 'packages', pkg.name, 'test'), files); + // packages/editors/* and packages/wrappers/* nest one level deeper. + for (const sub of readdirSync(join(ROOT, 'packages', pkg.name), { withFileTypes: true })) { + if (sub.isDirectory()) walk(join(ROOT, 'packages', pkg.name, sub.name, 'test'), files); + } +} + +const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); +// This file names every host and entry point it polices, so it would match its +// own rule. Excluded by exact path rather than by a heuristic, since a fuzzy +// self-exclusion is the kind of hole that lets a real caller through too. +const SELF = 'test/repo-health/live-cdn-callers.test.mjs'; + +/** + * Strip block and line comments, so a host named in a rationale is not read as + * a call. Crude on purpose: it only has to be good enough to avoid false + * positives on prose, and a missed strip fails LOUD rather than silent. + * @param {string} src + */ +function withoutComments(src) { + return src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/(^|[^:])\/\/.*$/gm, '$1'); +} + +/** + * Whether a file installs a `fetch` it owns, which is what makes naming a live + * host in it harmless. `e2e-vendor-stub.test.mjs` is the shape this exists for: + * it POSTs to api.jspm.io on purpose, having replaced `globalThis.fetch` with a + * sentinel first, so it exercises the real request shape without a packet + * leaving the machine. + * + * Deliberately file-level rather than per-call. A per-call scope check needs a + * parser, and this version cannot be quietly defeated by moving a call one + * block outward. The tradeoff is real and worth stating: a file that mocks + * fetch in one test and reaches the network in another passes. Accepted, + * because the failure this guard exists to prevent is a whole file nobody + * realised was live, not one call inside a file that is otherwise careful. + * + * @param {string} src + */ +function controlsFetch(src) { + return /withJspmDouble|withMockedFetch|globalThis\.fetch\s*=/.test(src); +} + +test('every allowlisted live caller is a *.live.test.* file that exists', () => { + for (const entry of LIVE_CALLERS) { + assert.ok(entry.file.includes(LIVE_MARKER), + `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); + assert.ok(files.some((f) => rel(f) === entry.file), + `${entry.file} is allowlisted but no such test file exists`); + assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`); + } +}); + +test('a live third-party host is only fetched from an allowlisted file', () => { + const allowed = new Set(LIVE_CALLERS.map((e) => e.file)); + /** @type {string[]} */ + const offenders = []; + for (const file of files) { + const path = rel(file); + if (path === SELF || allowed.has(path)) continue; + const raw = readFileSync(file, 'utf8'); + if (controlsFetch(raw)) continue; + const src = withoutComments(raw); + // A host inside a fetch call. Anything else (a string fed to a mock, an + // expected url in an assertion, an importmap fixture) is inert, and the + // suite is full of those on purpose. + for (const m of src.matchAll(/\bfetch\s*\(([^)]*)/g)) { + const host = LIVE_HOSTS.find((h) => m[1].includes(h)); + if (host) offenders.push(`${path}: fetch(... ${host} ...)`); + } + } + assert.deepEqual(offenders, [], + 'these reach a third party from a file a required check runs; move them into a ' + + '*.live.test.* file and allowlist it, or resolve through test/fixtures/jspm-double.mjs'); +}); + +test('a vendor entry point that reaches the network is called inside a double or a mock', () => { + const allowed = new Set(LIVE_CALLERS.map((e) => e.file)); + /** @type {string[]} */ + const offenders = []; + for (const file of files) { + const path = rel(file); + if (path === SELF || allowed.has(path)) continue; + const src = readFileSync(file, 'utf8'); + const uses = LIVE_ENTRY_POINTS.filter((fn) => new RegExp(`\\b${fn}\\s*\\(`).test(withoutComments(src))); + if (!uses.length) continue; + if (!controlsFetch(src)) { + offenders.push(`${path}: calls ${uses.join(', ')} with no double or mock in the file`); + } + } + assert.deepEqual(offenders, [], + 'these call a vendor entry point that resolves through a third party, without controlling ' + + 'fetch; wrap them in withJspmDouble from test/fixtures/jspm-double.mjs'); +}); + +test('both runners drop live files unless the network is explicitly required', () => { + // The policy above is only worth anything because the runners enforce it, so + // assert the enforcement rather than trusting it. A refactor that renames + // the marker or drops the filter reds here. + for (const runner of ['scripts/run-node-tests.js', 'scripts/run-bun-tests.js']) { + const src = readFileSync(join(ROOT, runner), 'utf8'); + assert.match(src, /WEBJS_REQUIRE_NETWORK/, `${runner} must honour the opt-in`); + assert.match(src, /\.live\.test\./, `${runner} must filter on the live marker`); + } +}); From 60815627db90401579d7281650041f6c531803fa Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:13:46 +0530 Subject: [PATCH 06/18] fix: bound the vendor pin bundle hash with the timeout its siblings have fetchIntegrity was the only outbound call in vendor.js with no AbortSignal, while jspmCall, fetchNpmJson, and fetchLiveIntegrity all carry one. Default mode pinAll runs it once per resolved URL, and a CLI run has no ambient deadline, so a CDN that accepted the connection and then stalled held the pin open indefinitely with nothing to interrupt it. It now uses the same INTEGRITY_FETCH_TIMEOUT_MS its sibling already had, which moves up beside the other outbound timeouts rather than sitting a thousand lines below its caller. The test asserts the SIGNAL rather than a wall clock: waiting out ten real seconds would make the test the slow thing it is complaining about, and shortening the timeout would mean growing a test-only knob in shipped code. Counterfactual run: dropping the signal argument reds it. A second candidate was investigated and deliberately NOT changed. clearVendorCache does not reset lastLiveResolveFailed, which reads like an omission in the documented start-clean primitive. It is not observable: resolveVendorImports is the flag's only reader and resets it on entry, and the pinned short-circuit ahead of that returns ok:true outright, so a value left behind by an earlier pinAll can never reach anything. The first version of this commit changed it anyway, with a test that passed just as happily once the fix was removed. A comment now says why the line is absent, so the next reader does not re-derive the same wrong conclusion. --- packages/server/src/vendor.js | 30 ++++++++++++++++-- packages/server/test/vendor/vendor.test.js | 37 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/packages/server/src/vendor.js b/packages/server/src/vendor.js index c53136209..9cc374d6b 100644 --- a/packages/server/src/vendor.js +++ b/packages/server/src/vendor.js @@ -338,6 +338,11 @@ let lastLiveResolveFailed = false; const JSPM_GENERATE_ENDPOINT = 'https://api.jspm.io/generate'; const JSPM_GENERATE_TIMEOUT_MS = 10_000; +// Bounds a single bundle GET, whether it is being hashed for a pin +// (`fetchIntegrity`) or during the warmup live-integrity pass +// (`fetchLiveIntegrity`). Declared here with the other outbound timeouts +// rather than beside one of its two callers. +const INTEGRITY_FETCH_TIMEOUT_MS = 10_000; /** * Provider names accepted by `webjs vendor pin --from `. @@ -679,6 +684,12 @@ export async function vendorImportMapEntries(bareImports, appDir) { export function clearVendorCache() { jspmCache.clear(); liveIntegrityCache.clear(); + // Deliberately does NOT touch `lastLiveResolveFailed`, which looks like an + // omission and is not. `resolveVendorImports` is the flag's only reader and + // it resets the flag on entry, while the pinned short-circuit above that + // returns `ok: true` outright, so a value left behind by an earlier + // `pinAll` or `vendorImportMapEntries` can never be observed. Clearing it + // here would be a change nothing could write a failing test for (#1150). } /** @@ -1105,12 +1116,21 @@ async function downloadBundle(url, appDir, filename) { * so the importmap can carry SRI hashes even when bundles aren't * locally vendored. * + * Bounded by the same timeout every other outbound call here carries. + * `pinAll` runs this once per resolved URL, so a CDN that accepts the + * connection and then stalls would otherwise hang the pin forever with + * nothing to interrupt it: there is no ambient deadline on a CLI run, + * and `node --test` imposes none either, so this was the one call in + * the file that could hold a process open indefinitely (#1150). + * * @param {string} url * @returns {Promise} the integrity string, or null on failure */ async function fetchIntegrity(url) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), INTEGRITY_FETCH_TIMEOUT_MS); try { - const response = await fetch(url); + const response = await fetch(url, { signal: controller.signal }); if (!response.ok) { console.error(`[webjs] hash ${url} returned ${response.status}`); return null; @@ -1121,8 +1141,13 @@ async function fetchIntegrity(url) { const buf = new Uint8Array(await response.arrayBuffer()); return await sha384Integrity(buf); } catch (e) { - console.error(`[webjs] hash ${url} failed: ${e && e.message}`); + const why = e && e.name === 'AbortError' + ? `timed out after ${INTEGRITY_FETCH_TIMEOUT_MS}ms` + : e && e.message; + console.error(`[webjs] hash ${url} failed: ${why}`); return null; + } finally { + clearTimeout(timer); } } @@ -2169,7 +2194,6 @@ export async function checkImportmapCoherence(imports, opts) { */ const liveIntegrityCache = new Map(); -const INTEGRITY_FETCH_TIMEOUT_MS = 10_000; // Cap concurrent bundle fetches so a large dep set does not open dozens of // sockets at once during warmup. Matches the bounded posture of the rest of // vendor.js (the jspm resolve is per-package but the network is the shared diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index 20d7ed901..2c9c15318 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -2317,6 +2317,43 @@ test('resolveVendorImports live: in-process cache avoids re-fetching an already- } }); +test('pinAll: a bundle hash that hangs is abandoned, not waited on forever', async () => { + // fetchIntegrity was the one outbound call in vendor.js with no AbortSignal, + // while jspmCall, fetchNpmJson, and fetchLiveIntegrity all carry one. pinAll + // runs it once per resolved URL and there is no ambient deadline on a CLI + // run, so a CDN that accepts the connection and then stalls held the pin + // open indefinitely (#1150). + // + // Assert the SIGNAL rather than the wall clock: a real 10s wait would make + // this test the slow thing it is complaining about, and a shortened timeout + // would need production code to grow a test-only knob. + const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';` }); + try { + /** @type {Array} */ + const signals = []; + await withMockedFetch(async (url, opts) => { + const s = String(url); + if (s.includes('api.jspm.io')) { + return jspmResponse({ picocolors: 'https://ga.jspm.io/npm:picocolors@1.1.1/index.js' }); + } + signals.push(opts && opts.signal); + // Abort exactly the way a timeout would, so the catch path is exercised. + const err = new Error('The operation was aborted'); + err.name = 'AbortError'; + throw err; + }, async () => { + const result = await pinAll(dir); + // Fail-open is unchanged: an unhashable bundle still pins, without SRI. + assert.ok(!result.failed, 'a failed hash must not fail the whole pin'); + }); + assert.equal(signals.length, 1, 'the bundle GET fired exactly once'); + assert.ok(signals[0] instanceof AbortSignal, + 'the bundle hash fetch must carry an AbortSignal so a stalled CDN cannot hang the pin'); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test('resolveVendorImports: PINNED path is unchanged (live-hash path not taken)', async () => { // Counterfactual that the pin path did not regress: a pin file with its own // integrity returns verbatim, and NO bundle fetch fires for it. From d6e5a55bb4259d790908c8a6300037fc9a8145eb Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:19:14 +0530 Subject: [PATCH 07/18] docs: state the live-CDN test policy once, in framework-dev.md The rule is now carried by a filename that both test runners enforce, so it needs one place that says what the filename means, why a nightly job exists to stop a permanent skip from hiding, and why the workflow deliberately has no pull_request trigger. Written down beside the #1228 note, which is the other half of the same problem. Also corrects the preload claim in that #1228 note while here. It says Node and Bun each ignore the other's flag spelling, which is only half true: node --preload is a hard bad-option error, but bun --import works today as an alias. The part that actually rules out an env var is that Bun ignores NODE_OPTIONS, so that is what the note says now. The no-build docs page said the LIVE hashing is bounded and fail-open, which was true and incomplete: pin-time hashing had no timeout at all until this branch. It now describes both. --- framework-dev.md | 22 +++++++++++++++++++++- website/app/docs/no-build/page.ts | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/framework-dev.md b/framework-dev.md index cd96a10d3..0589aa1f7 100644 --- a/framework-dev.md +++ b/framework-dev.md @@ -107,7 +107,27 @@ An ES module graph instantiates as a unit, so a failure at either point means `a So the OFF server boots with `test/e2e/fixtures/stub-jspm.mjs` preloaded, which answers the `api.jspm.io/generate` call from this repo's `node_modules` and points `dayjs` at a `data:` URL carrying those bytes. Stubbing the API call closes both holes at once, because the URL the browser fetches is whatever that map says. The ON server is left alone, since it resolves nothing. -Two things to keep in mind when touching this. The stub serves only the packages listed in its `LOCAL_VENDORS` map and passes everything else through to the real API, so **a vendor added to the blog later needs an entry there.** That failure is not silent: one unserviceable install makes the stub refuse the whole batch, the real API answers, and the block's first test fails naming the CDN url it got instead of a `data:` one. The same test is what catches the wiring itself going away, so do not delete it to make a new vendor pass. And the preload flag is runtime-specific (`--import` on Node, `--preload` on Bun, neither honouring the other, and Bun ignoring `NODE_OPTIONS`), which is why it is passed as argv through `preloadArgs` rather than an env var; the `E2E (blog served on Bun)` CI job is what a Node-only spelling would silently skip. +Two things to keep in mind when touching this. The stub serves only the packages listed in its `LOCAL_VENDORS` map and passes everything else through to the real API, so **a vendor added to the blog later needs an entry there.** That failure is not silent: one unserviceable install makes the stub refuse the whole batch, the real API answers, and the block's first test fails naming the CDN url it got instead of a `data:` one. The same test is what catches the wiring itself going away, so do not delete it to make a new vendor pass. And the preload flag is passed as argv through `preloadArgs` rather than an env var, because Bun ignores `NODE_OPTIONS` outright (measured: `NODE_OPTIONS=--import ... bun -e 0` loads nothing). The two flags are not symmetric, so do not reason from the Node side: `node --preload` is a hard `bad option` error, while `bun --import` currently works as an alias. Selecting per runtime anyway is what keeps this from depending on Bun continuing to accept a Node spelling. + +--- + +### Live third-party calls live only in `*.live.test.*` files (#1150) + +No required check may depend on a third party being up. The required `Unit + integration` job used to resolve vendors against the live jspm CDN, so a jspm outage redded pull requests that had nothing to do with vendoring; PR #1149, a five-file documentation change, is the one that finally made the case (it failed on the `#448` gitignore-healing test and passed on a re-run of the identical commit). + +The rule is carried by the FILENAME, so the test runners can enforce it rather than leaving it to discipline. `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` both drop any `*.live.test.*` file unless `WEBJS_REQUIRE_NETWORK=1` is set. Everything else resolves against `test/fixtures/jspm-double.mjs`, an offline double that models jspm rather than merely answering it (a 5xx or 429 is transient and retries per package, a 4xx probes per install, and an unresolvable install fails the WHOLE batch, which is the premise `jspmGenerate`'s fallback ladder is built on). + +This replaced a `WEBJS_SKIP_NETWORK_TESTS` gate that could not work: it was opt-OUT, so CI, which never set it, always ran live; it was convention rather than something the runner could check; and two `registry.npmjs.org` callers were never covered by it at all. Leaving the one live parity test gated in place would not have been enough either, since after #1219 it still reds on a 4xx, and a WAF 403 or a moved route is exactly the shape #1149 hit. + +Four things to keep in mind when touching this. + +**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. `test/repo-health/live-cdn-callers.test.mjs` reds if a live host reaches a `fetch(` outside an allowlisted file. + +**The nightly is what stops a permanent skip from hiding.** `.github/workflows/vendor-cdn.yml` runs the live files with `WEBJS_REQUIRE_NETWORK=1`, which both selects them AND promotes their upstream-trouble skip into a failure. Without that second half a permanently skipping test is indistinguishable from a passing one. A failure opens or comments on one fixed-title tracking issue, since GitHub notifies only the workflow file's last committer about a failed scheduled run. + +**Do not add a `pull_request` trigger to that workflow.** A live check on a PR is a live check whatever job it sits in; making it non-required would just produce a red somebody is told to ignore, which is how a real failure gets ignored too. + +**`.github/workflows/ci.yml` is deliberately not involved.** Eleven jobs share its `on:` block, so the filter belongs in the runners, where it also covers a local `npm test`. --- diff --git a/website/app/docs/no-build/page.ts b/website/app/docs/no-build/page.ts index 6e417401a..7073116c8 100644 --- a/website/app/docs/no-build/page.ts +++ b/website/app/docs/no-build/page.ts @@ -124,7 +124,7 @@ Pinning vendor packages from /home/me/my-app (downloading bundles)... Pinned 2 packages, wrote .webjs/vendor/importmap.json + 2 bundles.

This downloads each bundle from jspm.io to .webjs/vendor/<pkg>@<version>.js. The importmap then points at local /__webjs/vendor/<file>.js URLs; the server serves the committed bundle files. Browser never touches jspm.io at runtime; works fully offline.

Pin is intentionally manual (no predev/prestart auto-run). Auto-pin would cause silent churn in the committed importmap.json as jspm.io resolves URLs or transitive deps drift. Rails takes the same posture: bin/importmap pin is always developer-invoked.

-

sha384 SRI integrity is on by default, with OR without a pin file. An unpinned (live-resolved) app hashes each cross-origin bundle at warmup and the SSR pipeline stamps the matching hash on each <link rel="modulepreload"> and on the importmap entry itself, so the browser refuses to execute a bundle whose bytes don't match (CDN compromise defense), even before you pin. The live hashing is bounded and fail-open: if a bundle fetch fails (a CDN hiccup), that one URL simply loads without integrity (logged once) and the app still boots. Running webjs vendor pin makes the hashes reproducible and removes the warmup fetch: it writes them alongside the imports in importmap.json under an integrity key (both webjs vendor pin and --download populate it), so the hashes only update when the command is rerun and routine cache-busting cannot drop them.

+

sha384 SRI integrity is on by default, with OR without a pin file. An unpinned (live-resolved) app hashes each cross-origin bundle at warmup and the SSR pipeline stamps the matching hash on each <link rel="modulepreload"> and on the importmap entry itself, so the browser refuses to execute a bundle whose bytes don't match (CDN compromise defense), even before you pin. The live hashing is bounded and fail-open: if a bundle fetch fails (a CDN hiccup), that one URL simply loads without integrity (logged once) and the app still boots. Pin-time hashing is bounded the same way, so a CDN that accepts the connection and then stalls cannot hang webjs vendor pin; that bundle is pinned without a hash and the command tells you to rerun once jspm.io is healthy. Running webjs vendor pin makes the hashes reproducible and removes the warmup fetch: it writes them alongside the imports in importmap.json under an integrity key (both webjs vendor pin and --download populate it), so the hashes only update when the command is rerun and routine cache-busting cannot drop them.

Switch CDN with --from

If jspm.io has an incident, or you want jsdelivr-served packages, pass a different resolver:

From 82218e774649813c1af882abca8181a4b33b4e04 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:38:18 +0530 Subject: [PATCH 08/18] fix: bound downloadBundle too, and make the live-caller guard discriminating Four things this branch claimed about itself were not true of what shipped. The guard was the serious one. It was file-level, so a single withMockedFetch anywhere in a file exempted every live call in it. Measured against the pre-PR tree it reported ZERO offenders for vendor.test.js, the file carrying a live fetch(api.jspm.io) and four unwrapped vendor entry points, which is precisely the regression it exists to prevent. Its counterfactual passed only because the throwaway offender I tested with had no mock at all, a shape nobody writes. The scan is per call now, and lives in test/fixtures/live-caller-scan.mjs so it can be run against inline fixtures rather than only against today's tree. A call is exempt inside a withJspmDouble or withMockedFetch body, or behind an explicit `// live-cdn-ok: ` marker at the site. Eleven such markers go in, each naming why that call provably returns before dialling; wrapping those in a double to satisfy a checker would have been churn that teaches the wrong thing. Against the pre-PR tree the rewritten scan now reports 24 offenders, starting with the live fetch. It has one blind spot and the header says so: it cannot see a spawned child, so vendor-cli.test.mjs reports zero both before and after. What covers that file is the armed-marker assertion inside its own runCli. downloadBundle was the other real one. It sits twenty lines above the call this branch bounded and is the same unguarded shape on the same CLI run, so `webjs vendor pin --download` kept exactly the behaviour the comment said had been removed. Both are bounded now and the test covers both modes. The two doc claims are corrected. The no-build page described only default-mode pinning, and framework-dev said no required check may depend on a third party, which is not true of the tree: the app-boot tests still resolve vendors live on cold boot. They fail OPEN, which is the property that actually matters, so that is what it says now. Measured with jspm and registry.npmjs.org forced to 503: the only failures across npm test are three that fail identically on a clean origin/main baseline. --- framework-dev.md | 8 +- packages/server/src/vendor.js | 34 ++-- packages/server/test/vendor/vendor.test.js | 81 ++++++---- test/fixtures/live-caller-scan.mjs | 160 +++++++++++++++++++ test/repo-health/live-cdn-callers.test.mjs | 173 +++++++++++---------- website/app/docs/no-build/page.ts | 2 +- 6 files changed, 332 insertions(+), 126 deletions(-) create mode 100644 test/fixtures/live-caller-scan.mjs diff --git a/framework-dev.md b/framework-dev.md index 0589aa1f7..28bed07cd 100644 --- a/framework-dev.md +++ b/framework-dev.md @@ -113,7 +113,9 @@ Two things to keep in mind when touching this. The stub serves only the packages ### Live third-party calls live only in `*.live.test.*` files (#1150) -No required check may depend on a third party being up. The required `Unit + integration` job used to resolve vendors against the live jspm CDN, so a jspm outage redded pull requests that had nothing to do with vendoring; PR #1149, a five-file documentation change, is the one that finally made the case (it failed on the `#448` gitignore-healing test and passed on a re-run of the identical commit). +No required check may FAIL because a third party is down. The required `Unit + integration` job used to resolve vendors against the live jspm CDN, so a jspm outage redded pull requests that had nothing to do with vendoring; PR #1149, a five-file documentation change, is the one that finally made the case (it failed on the `#448` gitignore-healing test and passed on a re-run of the identical commit). + +Be precise about what that does and does not say, because the weaker-sounding version is the true one. Required checks still REACH jspm: no in-repo app carries a pin file, so every test that cold-boots one (`test/preload-subset.test.mjs`, the `test/docs/*` boot tests, `test/integration/blog-http.test.mjs`, `packages/server/test/elision/differential-elision.test.js`) resolves its vendors live on the first request, transitively, through `resolveVendorImports`. What makes that acceptable is that the resolve fails OPEN: an unreachable CDN yields a partial importmap and a warning, never a throw, and none of those tests assert on a vendor entry. Measured with jspm forced to fail, each of them still passes in a few seconds. The rule is about what can turn a check red, not about counting packets. The rule is carried by the FILENAME, so the test runners can enforce it rather than leaving it to discipline. `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` both drop any `*.live.test.*` file unless `WEBJS_REQUIRE_NETWORK=1` is set. Everything else resolves against `test/fixtures/jspm-double.mjs`, an offline double that models jspm rather than merely answering it (a 5xx or 429 is transient and retries per package, a 4xx probes per install, and an unresolvable install fails the WHOLE batch, which is the premise `jspmGenerate`'s fallback ladder is built on). @@ -121,7 +123,9 @@ This replaced a `WEBJS_SKIP_NETWORK_TESTS` gate that could not work: it was opt- Four things to keep in mind when touching this. -**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. `test/repo-health/live-cdn-callers.test.mjs` reds if a live host reaches a `fetch(` outside an allowlisted file. +**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. + +**The guard is per call, and it has one blind spot.** `test/repo-health/live-cdn-callers.test.mjs` reds when a live host reaches a `fetch(`, or a vendor entry point (`pinAll` / `updatePinned` / `auditPinned` / `findOutdated`) is called, outside an allowlisted file, a `withJspmDouble` / `withMockedFetch` body, or a `// live-cdn-ok: ` marker. Per call matters: the first version was file-level, and a single `withMockedFetch` anywhere in `vendor.test.js` exempted its live `fetch(api.jspm.io)` and all four unwrapped entry points, so the guard reported zero offenders for the exact file this change had to fix. The blind spot is a SPAWNED child: `test/vendor-cli/vendor-cli.test.mjs` reaches jspm by running the CLI in another process, so a static scan of its source sees nothing. What covers that file is the `[jspm-double] armed` stderr assertion inside its own `runCli`, which fires on every spawn. A new spawning test needs the same treatment. **The nightly is what stops a permanent skip from hiding.** `.github/workflows/vendor-cdn.yml` runs the live files with `WEBJS_REQUIRE_NETWORK=1`, which both selects them AND promotes their upstream-trouble skip into a failure. Without that second half a permanently skipping test is indistinguishable from a passing one. A failure opens or comments on one fixed-title tracking issue, since GitHub notifies only the workflow file's last committer about a failed scheduled run. diff --git a/packages/server/src/vendor.js b/packages/server/src/vendor.js index 9cc374d6b..15532c2e1 100644 --- a/packages/server/src/vendor.js +++ b/packages/server/src/vendor.js @@ -338,10 +338,11 @@ let lastLiveResolveFailed = false; const JSPM_GENERATE_ENDPOINT = 'https://api.jspm.io/generate'; const JSPM_GENERATE_TIMEOUT_MS = 10_000; -// Bounds a single bundle GET, whether it is being hashed for a pin -// (`fetchIntegrity`) or during the warmup live-integrity pass +// Bounds a single bundle GET: written to disk by `webjs vendor pin +// --download` (`downloadBundle`), hashed in place by default-mode pin +// (`fetchIntegrity`), or hashed during the warmup live-integrity pass // (`fetchLiveIntegrity`). Declared here with the other outbound timeouts -// rather than beside one of its two callers. +// rather than beside one of its three callers. const INTEGRITY_FETCH_TIMEOUT_MS = 10_000; /** @@ -1082,14 +1083,22 @@ async function writePinFile(appDir, imports, integrity, provider) { * success or null on failure. The integrity hash is computed from the * downloaded bytes so it's always consistent with what's on disk. * + * Bounded by the same timeout as every other outbound call here. + * `pinAll(dir, { download: true })` runs this once per resolved URL on + * a CLI run with no ambient deadline, so a CDN that accepts the + * connection and then stalls would otherwise hang the pin with nothing + * to interrupt it (#1150). + * * @param {string} url * @param {string} appDir * @param {string} filename * @returns {Promise<{ bytes: number, integrity: string } | null>} */ async function downloadBundle(url, appDir, filename) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), INTEGRITY_FETCH_TIMEOUT_MS); try { - const response = await fetch(url); + const response = await fetch(url, { signal: controller.signal }); if (!response.ok) { console.error(`[webjs] download ${url} returned ${response.status}`); return null; @@ -1105,8 +1114,13 @@ async function downloadBundle(url, appDir, filename) { await writeFile(join(pinDir(appDir), filename), buf); return { bytes: buf.byteLength, integrity: await sha384Integrity(buf) }; } catch (e) { - console.error(`[webjs] download ${url} failed: ${e && e.message}`); + const why = e && e.name === 'AbortError' + ? `timed out after ${INTEGRITY_FETCH_TIMEOUT_MS}ms` + : e && e.message; + console.error(`[webjs] download ${url} failed: ${why}`); return null; + } finally { + clearTimeout(timer); } } @@ -1117,11 +1131,11 @@ async function downloadBundle(url, appDir, filename) { * locally vendored. * * Bounded by the same timeout every other outbound call here carries. - * `pinAll` runs this once per resolved URL, so a CDN that accepts the - * connection and then stalls would otherwise hang the pin forever with - * nothing to interrupt it: there is no ambient deadline on a CLI run, - * and `node --test` imposes none either, so this was the one call in - * the file that could hold a process open indefinitely (#1150). + * Default-mode `pinAll` runs this once per resolved URL, so a CDN that + * accepts the connection and then stalls would otherwise hang the pin + * with nothing to interrupt it: there is no ambient deadline on a CLI + * run, and `node --test` imposes none either. `downloadBundle` is the + * `--download` half of the same gap and is bounded the same way (#1150). * * @param {string} url * @returns {Promise} the integrity string, or null on failure diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index 2c9c15318..334c1dd55 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -1010,6 +1010,7 @@ test('pinAll: returns noBareImports without writing pin file when no bare import await writeFile(join(dir, 'package.json'), '{"name":"tmp","version":"0.0.0"}'); await writeFile(join(dir, 'app', 'page.ts'), `export default () => 'no bare imports here';`); try { + // live-cdn-ok: no bare imports, so pinAll returns before the resolve. const result = await pinAll(dir); assert.ok(result.noBareImports, 'noBareImports must be true'); assert.equal(result.failed, undefined, 'failed must be absent (not a failure, just nothing to do)'); @@ -1033,6 +1034,7 @@ test('pinAll: reports found-but-uninstalled specifiers instead of noBareImports 'app/page.ts': `import * as THREE from 'three';\nimport { OrbitControls } from 'three/addons/controls/OrbitControls.js';`, }); try { + // live-cdn-ok: every specifier is dropped by the version gate, so installs is empty. const result = await pinAll(dir); assert.equal(result.noBareImports, undefined, 'must NOT claim there were no bare imports'); assert.ok(Array.isArray(result.droppedUnresolvable), 'droppedUnresolvable must be an array'); @@ -1758,6 +1760,7 @@ test('pinAll: rejects unknown provider with a clear error', async () => { await writeFile(join(dir, 'package.json'), '{"name":"tmp"}'); try { await assert.rejects( + // live-cdn-ok: the provider is rejected before any call is dialled. () => pinAll(dir, { from: 'not-a-real-cdn' }), /unknown provider 'not-a-real-cdn'/, ); @@ -1815,6 +1818,7 @@ test('auditPinned: no pin file returns zero-checked', async () => { const dir = join(tmpdir(), `webjs-audit-empty-${Date.now()}`); await mkdir(dir, { recursive: true }); try { + // live-cdn-ok: no pin file, so it short-circuits before the registry call. const { vulnerable, totalChecked } = await auditPinned(dir); assert.equal(totalChecked, 0); assert.deepEqual(vulnerable, []); @@ -1827,6 +1831,7 @@ test('findOutdated: no pin file returns []', async () => { const dir = join(tmpdir(), `webjs-outdated-empty-${Date.now()}`); await mkdir(dir, { recursive: true }); try { + // live-cdn-ok: no pin file, so it short-circuits before the registry call. assert.deepEqual(await findOutdated(dir), []); } finally { await rm(dir, { recursive: true, force: true }); @@ -1838,6 +1843,7 @@ test('updatePinned: rejects unknown provider', async () => { await mkdir(dir, { recursive: true }); try { await assert.rejects( + // live-cdn-ok: the provider is rejected before any call is dialled. () => updatePinned(dir, { from: 'not-real' }), /unknown provider/, ); @@ -1853,6 +1859,7 @@ test('updatePinned: no outdated returns noOutdated:true without writing', async const dir = join(tmpdir(), `webjs-update-clean-${Date.now()}`); await mkdir(dir, { recursive: true }); try { + // live-cdn-ok: an empty pin file short-circuits before the registry call. const result = await updatePinned(dir); assert.ok(result.noOutdated); assert.deepEqual(result.updated, []); @@ -1932,6 +1939,7 @@ test('auditPinned: surfaces network failure as errored:true', async () => { const origFetch = globalThis.fetch; globalThis.fetch = async () => { throw new Error('simulated network failure'); }; try { + // live-cdn-ok: the test installs its own throwing fetch for its whole duration. const result = await auditPinned(dir); assert.equal(result.errored, true); assert.deepEqual(result.vulnerable, []); @@ -1964,6 +1972,7 @@ test('pinAll: respects existing pin file provider when --from is not passed', as // without writing. The interesting assertion: it didn't throw // and pinAll read the provider for whatever it would have done. // Verify by checking pin file's provider field unchanged. + // live-cdn-ok: the app has no bare imports, so pinAll returns before the resolve. const result = await pinAll(dir); assert.ok(result.noBareImports); const file = await readPinFile(dir); @@ -2060,6 +2069,7 @@ test('updatePinned: only counts a package as updated when at least one spec reso return /** @type any */ ({ ok: false, status: 404, json: async () => ({}) }); }; try { + // live-cdn-ok: the test installs its own fetch for its whole duration. const result = await updatePinned(dir); assert.deepEqual(result.updated, [], 'no spec resolved, so updated[] must be empty even though findOutdated saw dayjs as outdated'); @@ -2109,6 +2119,7 @@ test('findOutdated: returns an Array, not undefined (ASI regression guard)', asy // No pin file → grouped is empty → no fetches → return empty array. // The interesting assertion is that the return value is an // ARRAY (.length accessible), not undefined. + // live-cdn-ok: the test installs its own fetch for its whole duration. const result = await findOutdated(dir); assert.ok(Array.isArray(result), 'findOutdated must always return an Array'); assert.equal(result.length, 0); @@ -2317,40 +2328,50 @@ test('resolveVendorImports live: in-process cache avoids re-fetching an already- } }); -test('pinAll: a bundle hash that hangs is abandoned, not waited on forever', async () => { - // fetchIntegrity was the one outbound call in vendor.js with no AbortSignal, - // while jspmCall, fetchNpmJson, and fetchLiveIntegrity all carry one. pinAll - // runs it once per resolved URL and there is no ambient deadline on a CLI - // run, so a CDN that accepts the connection and then stalls held the pin - // open indefinitely (#1150). +test('pinAll: a bundle fetch that hangs is abandoned, not waited on forever', async () => { + // fetchIntegrity (default mode) and downloadBundle (--download) were the two + // outbound calls in vendor.js with no AbortSignal, while jspmCall, + // fetchNpmJson, and fetchLiveIntegrity all carried one. pinAll runs one of + // them once per resolved URL and there is no ambient deadline on a CLI run, + // so a CDN that accepted the connection and then stalled held the pin open + // indefinitely (#1150). + // + // BOTH modes are covered, because they take different calls. The first + // version of this test only exercised the default path, which left + // `webjs vendor pin --download` with exactly the behaviour the fix claimed + // to have removed. // // Assert the SIGNAL rather than the wall clock: a real 10s wait would make // this test the slow thing it is complaining about, and a shortened timeout // would need production code to grow a test-only knob. - const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';` }); - try { - /** @type {Array} */ - const signals = []; - await withMockedFetch(async (url, opts) => { - const s = String(url); - if (s.includes('api.jspm.io')) { - return jspmResponse({ picocolors: 'https://ga.jspm.io/npm:picocolors@1.1.1/index.js' }); - } - signals.push(opts && opts.signal); - // Abort exactly the way a timeout would, so the catch path is exercised. - const err = new Error('The operation was aborted'); - err.name = 'AbortError'; - throw err; - }, async () => { - const result = await pinAll(dir); - // Fail-open is unchanged: an unhashable bundle still pins, without SRI. - assert.ok(!result.failed, 'a failed hash must not fail the whole pin'); - }); - assert.equal(signals.length, 1, 'the bundle GET fired exactly once'); - assert.ok(signals[0] instanceof AbortSignal, - 'the bundle hash fetch must carry an AbortSignal so a stalled CDN cannot hang the pin'); - } finally { - await rm(dir, { recursive: true, force: true }); + for (const opts of [{}, { download: true }]) { + const mode = opts.download ? '--download' : 'default'; + const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';` }); + try { + /** @type {Array} */ + const signals = []; + await withMockedFetch(async (url, init) => { + const s = String(url); + if (s.includes('api.jspm.io')) { + return jspmResponse({ picocolors: 'https://ga.jspm.io/npm:picocolors@1.1.1/index.js' }); + } + signals.push(init && init.signal); + // Abort exactly the way a timeout would, so the catch path is exercised. + const err = new Error('The operation was aborted'); + err.name = 'AbortError'; + throw err; + }, async () => { + const result = await pinAll(dir, opts); + // Fail-open is unchanged: an unreachable bundle must not fail the pin + // outright in default mode, where the entry still pins without a hash. + if (!opts.download) assert.ok(!result.failed, 'a failed hash must not fail the whole pin'); + }); + assert.equal(signals.length, 1, `${mode}: the bundle GET fired exactly once`); + assert.ok(signals[0] instanceof AbortSignal, + `${mode}: the bundle fetch must carry an AbortSignal so a stalled CDN cannot hang the pin`); + } finally { + await rm(dir, { recursive: true, force: true }); + } } }); diff --git a/test/fixtures/live-caller-scan.mjs b/test/fixtures/live-caller-scan.mjs new file mode 100644 index 000000000..952c03266 --- /dev/null +++ b/test/fixtures/live-caller-scan.mjs @@ -0,0 +1,160 @@ +/** + * Find test code that reaches a third-party host (#1150). + * + * Split out from `test/repo-health/live-cdn-callers.test.mjs` so the analysis + * can be exercised against inline fixtures rather than only against whatever + * the tree happens to contain today. That is not a style preference: the first + * version of the guard was file-level, and measured against the pre-PR tree it + * flagged NEITHER of the two files the change converts. `vendor.test.js` + * carried a live `fetch('https://api.jspm.io/generate')` and four unwrapped + * vendor entry points, and a single `withMockedFetch` elsewhere in the same + * file exempted all of it. A guard with an empty counterfactual is worse than + * no guard, because it reads as protection. + * + * So the exemption is PER CALL. A live host or a vendor entry point is fine + * only where it sits lexically inside a `withMockedFetch(...)` or + * `withJspmDouble(...)` argument list, which is the shape that actually + * controls `globalThis.fetch` for the duration of that call. + * + * The second exemption is an explicit per-site marker, `// live-cdn-ok: why`, + * on or just above the call. Several entry-point calls genuinely cannot reach + * the network (a `pinAll` on an app with no resolvable bare imports returns + * before the resolve, an `auditPinned` with no pin file short-circuits, a + * provider-validation test rejects before dialling), and wrapping those in a + * double to satisfy a checker would be churn that teaches the wrong thing. The + * marker records the reason at the site instead. It is deliberately noisy to + * add, so it is a decision rather than a default, and unlike the file-level + * flag it replaced it exempts exactly one call. + * + * This module has NO side effects. + */ + +/** Third-party hosts no required check may depend on. */ +export const LIVE_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; + +/** + * Vendor entry points that reach a live host internally, so naming one is as + * live as calling fetch yourself. + */ +export const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'findOutdated']; + +/** Helpers that install a `fetch` the test controls for the duration of a call. */ +const GUARDS = ['withMockedFetch', 'withJspmDouble']; + +/** + * Blank out comments, strings, and template literals, replacing each with + * same-length filler so every index still lines up with the original source. + * + * Preserving offsets is what lets the guarded-range scan below run on the + * masked text and still report positions in the real file. Blanking strings + * matters for two different reasons: a host named inside a mock's expected-url + * string is not a call, and an unbalanced parenthesis inside a string would + * otherwise wreck the brace matching. + * + * @param {string} src + * @returns {string} + */ +export function maskLiterals(src) { + const out = src.split(''); + let i = 0; + const blank = (from, to) => { + for (let k = from; k < to && k < out.length; k++) if (out[k] !== '\n') out[k] = ' '; + }; + while (i < src.length) { + const c = src[i]; + const next = src[i + 1]; + if (c === '/' && next === '*') { + const end = src.indexOf('*/', i + 2); + const stop = end === -1 ? src.length : end + 2; + blank(i, stop); i = stop; continue; + } + if (c === '/' && next === '/') { + const end = src.indexOf('\n', i); + const stop = end === -1 ? src.length : end; + blank(i, stop); i = stop; continue; + } + if (c === '"' || c === "'" || c === '`') { + let j = i + 1; + while (j < src.length) { + if (src[j] === '\\') { j += 2; continue; } + if (src[j] === c) break; + j++; + } + // Blank the CONTENTS but keep the quotes, so a masked string is still + // recognisably a string and cannot merge with the token beside it. + blank(i + 1, j); i = j + 1; continue; + } + i++; + } + return out.join(''); +} + +/** + * Character ranges covered by a `withMockedFetch(` / `withJspmDouble(` call, + * from its opening parenthesis to its match. + * + * @param {string} masked output of {@link maskLiterals} + * @returns {Array<[number, number]>} + */ +export function guardedRanges(masked) { + /** @type {Array<[number, number]>} */ + const ranges = []; + for (const guard of GUARDS) { + const re = new RegExp(`\\b${guard}\\s*\\(`, 'g'); + for (const m of masked.matchAll(re)) { + const open = m.index + m[0].length - 1; + let depth = 0; + for (let k = open; k < masked.length; k++) { + if (masked[k] === '(') depth++; + else if (masked[k] === ')') { + depth--; + if (depth === 0) { ranges.push([open, k]); break; } + } + } + } + } + return ranges; +} + +/** + * Live callers in one file, each with the 1-indexed line it sits on. + * + * @param {string} src + * @returns {Array<{ kind: 'host' | 'entry', what: string, line: number }>} + */ +export function findLiveCallers(src) { + const masked = maskLiterals(src); + const ranges = guardedRanges(masked); + const lines = src.split('\n'); + const lineAt = (idx) => src.slice(0, idx).split('\n').length; + const inGuard = (idx) => ranges.some(([a, b]) => idx > a && idx < b); + // The marker is read from the ORIGINAL source, since masking blanks + // comments. Three lines of lookback, so it can sit above a call that wraps. + const marked = (line) => lines + .slice(Math.max(0, line - 4), line) + .some((l) => l.includes('live-cdn-ok:')); + const guarded = (idx) => inGuard(idx) || marked(lineAt(idx)); + + /** @type {Array<{ kind: 'host' | 'entry', what: string, line: number }>} */ + const found = []; + + // A host literal has to be read from the ORIGINAL source, since masking + // blanks string contents. Only its position is taken from the mask, and a + // `fetch(` whose argument names a live host is what counts; the same host in + // an assertion or an importmap fixture is inert, and the suite is full of + // those on purpose. + for (const m of masked.matchAll(/\bfetch\s*\(/g)) { + const argStart = m.index + m[0].length; + const arg = src.slice(argStart, argStart + 200); + const host = LIVE_HOSTS.find((h) => arg.includes(h)); + if (host && !guarded(m.index)) found.push({ kind: 'host', what: host, line: lineAt(m.index) }); + } + + for (const fn of LIVE_ENTRY_POINTS) { + for (const m of masked.matchAll(new RegExp(`\\b${fn}\\s*\\(`, 'g'))) { + if (!guarded(m.index)) found.push({ kind: 'entry', what: fn, line: lineAt(m.index) }); + } + } + + return found.sort((a, b) => a.line - b.line); +} diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index bcd1b6b0c..5877e5921 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -10,11 +10,26 @@ * so CI always ran live, and two `registry.npmjs.org` callers were never * covered by it at all. * - * The rule is enforced on the PROPERTY, not on a spelling. Asserting that some - * marker constant appears exactly once would certify nothing (a new test can - * always call fetch without it) and would red on a rename or a reformat. So - * this looks for the actual live surface: a third-party host inside a `fetch(` - * call, and the vendor entry points that reach one internally. + * The analysis lives in `test/fixtures/live-caller-scan.mjs` so it can be run + * against inline fixtures rather than only against whatever the tree happens + * to contain, which is what the `counterfactual` tests below do. The first + * version of this guard was file-level and, measured against the pre-PR tree, + * flagged NEITHER of the two files this change converts: a single + * `withMockedFetch` anywhere in `vendor.test.js` exempted its live + * `fetch(api.jspm.io)` and all four of its unwrapped entry points. So the + * exemption is per call now, and the counterfactual is a real one. + * + * Measured against the pre-PR tree, the rewritten scan reports 24 offenders in + * `vendor.test.js`, starting with the live `fetch('https://api.jspm.io/generate')`. + * + * ONE BLIND SPOT, stated rather than papered over: it cannot see a SPAWNED + * child. `test/vendor-cli/vendor-cli.test.mjs` reaches jspm by running the CLI + * in another process, so it contains no `fetch(` and no entry-point call, and + * this scan reports zero for it both before and after the change. What covers + * that file is the `[jspm-double] armed` assertion inside its own `runCli`, + * which fires on every spawn and reds all ten of its tests if the preload flag + * is dropped. A future spawning test needs the same treatment; a static scan + * of the parent's source cannot give it. * * Both test runners drop `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, so * a file on the allowlist below genuinely cannot run in a required check. @@ -25,6 +40,8 @@ import { readdirSync, readFileSync } from 'node:fs'; import { join, resolve, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { findLiveCallers } from '../fixtures/live-caller-scan.mjs'; + const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); /** @@ -33,32 +50,33 @@ const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); * purpose: a reason per entry, so adding one is a decision somebody wrote down * rather than a guard somebody silenced. * - * Every entry MUST be a `*.live.test.*` path, which is what the runners key on. + * Every entry MUST be a `*.live.test.*` path, which is what the runners key + * on, with ONE exception recorded below. */ const LIVE_CALLERS = [ { file: 'packages/server/test/vendor/jspm-cdn.live.test.js', + live: true, why: 'the two things an offline double cannot vouch for: that our merged output equals ' + "jspm's own unified graph (#446), and that jspm still fails a WHOLE batch permanently " + 'when one install is unresolvable, which the entire fallback ladder in vendor.js assumes.', }, { file: 'test/vendor-cli/vendor-pin.live.test.mjs', + live: true, why: 'one real run of the command a user actually types, so `webjs vendor pin` does not ' + 'become a thing that is only ever exercised against a fixture.', }, + { + file: 'test/repo-health/e2e-vendor-stub.test.mjs', + live: false, + why: 'NOT a live caller. It POSTs to api.jspm.io on purpose to exercise the real request ' + + 'shape, having installed a sentinel fetch at module scope BEFORE importing the fixture ' + + 'under test, which is the whole point of that ordering (#1228). The sentinel is a bare ' + + 'assignment outside any call, so a per-call scan cannot see it covering anything.', + }, ]; -/** Third-party hosts no required check may depend on. */ -const LIVE_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; - -/** - * Vendor entry points that reach a live host internally, so naming one is as - * live as calling fetch. Each is allowed inside a `withMockedFetch` or - * `withJspmDouble` body, which is how the offline suites use them. - */ -const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'findOutdated']; - const LIVE_MARKER = '.live.test.'; /** @param {string} dir @param {string[]} out */ @@ -86,91 +104,80 @@ for (const pkg of readdirSync(join(ROOT, 'packages'), { withFileTypes: true })) } const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); -// This file names every host and entry point it polices, so it would match its -// own rule. Excluded by exact path rather than by a heuristic, since a fuzzy -// self-exclusion is the kind of hole that lets a real caller through too. -const SELF = 'test/repo-health/live-cdn-callers.test.mjs'; - -/** - * Strip block and line comments, so a host named in a rationale is not read as - * a call. Crude on purpose: it only has to be good enough to avoid false - * positives on prose, and a missed strip fails LOUD rather than silent. - * @param {string} src - */ -function withoutComments(src) { - return src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/(^|[^:])\/\/.*$/gm, '$1'); -} - -/** - * Whether a file installs a `fetch` it owns, which is what makes naming a live - * host in it harmless. `e2e-vendor-stub.test.mjs` is the shape this exists for: - * it POSTs to api.jspm.io on purpose, having replaced `globalThis.fetch` with a - * sentinel first, so it exercises the real request shape without a packet - * leaving the machine. - * - * Deliberately file-level rather than per-call. A per-call scope check needs a - * parser, and this version cannot be quietly defeated by moving a call one - * block outward. The tradeoff is real and worth stating: a file that mocks - * fetch in one test and reaches the network in another passes. Accepted, - * because the failure this guard exists to prevent is a whole file nobody - * realised was live, not one call inside a file that is otherwise careful. - * - * @param {string} src - */ -function controlsFetch(src) { - return /withJspmDouble|withMockedFetch|globalThis\.fetch\s*=/.test(src); -} -test('every allowlisted live caller is a *.live.test.* file that exists', () => { +test('every allowlisted LIVE caller is a *.live.test.* file that exists', () => { for (const entry of LIVE_CALLERS) { - assert.ok(entry.file.includes(LIVE_MARKER), - `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); assert.ok(files.some((f) => rel(f) === entry.file), `${entry.file} is allowlisted but no such test file exists`); assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`); + if (entry.live) { + assert.ok(entry.file.includes(LIVE_MARKER), + `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); + } } }); -test('a live third-party host is only fetched from an allowlisted file', () => { +test('no test outside the allowlist reaches a third party', () => { const allowed = new Set(LIVE_CALLERS.map((e) => e.file)); /** @type {string[]} */ const offenders = []; for (const file of files) { const path = rel(file); - if (path === SELF || allowed.has(path)) continue; - const raw = readFileSync(file, 'utf8'); - if (controlsFetch(raw)) continue; - const src = withoutComments(raw); - // A host inside a fetch call. Anything else (a string fed to a mock, an - // expected url in an assertion, an importmap fixture) is inert, and the - // suite is full of those on purpose. - for (const m of src.matchAll(/\bfetch\s*\(([^)]*)/g)) { - const host = LIVE_HOSTS.find((h) => m[1].includes(h)); - if (host) offenders.push(`${path}: fetch(... ${host} ...)`); + if (allowed.has(path)) continue; + for (const hit of findLiveCallers(readFileSync(file, 'utf8'))) { + offenders.push(`${path}:${hit.line} ${hit.kind === 'host' ? `fetch(${hit.what})` : `${hit.what}()`}`); } } assert.deepEqual(offenders, [], - 'these reach a third party from a file a required check runs; move them into a ' - + '*.live.test.* file and allowlist it, or resolve through test/fixtures/jspm-double.mjs'); + 'these reach a third party from a file a required check runs. Wrap the call in ' + + 'withJspmDouble (test/fixtures/jspm-double.mjs), move it into a *.live.test.* file and ' + + 'allowlist that, or if the call provably returns before dialling, mark the site with a ' + + '`// live-cdn-ok: ` comment.'); }); -test('a vendor entry point that reaches the network is called inside a double or a mock', () => { - const allowed = new Set(LIVE_CALLERS.map((e) => e.file)); - /** @type {string[]} */ - const offenders = []; - for (const file of files) { - const path = rel(file); - if (path === SELF || allowed.has(path)) continue; - const src = readFileSync(file, 'utf8'); - const uses = LIVE_ENTRY_POINTS.filter((fn) => new RegExp(`\\b${fn}\\s*\\(`).test(withoutComments(src))); - if (!uses.length) continue; - if (!controlsFetch(src)) { - offenders.push(`${path}: calls ${uses.join(', ')} with no double or mock in the file`); - } - } - assert.deepEqual(offenders, [], - 'these call a vendor entry point that resolves through a third party, without controlling ' - + 'fetch; wrap them in withJspmDouble from test/fixtures/jspm-double.mjs'); +test('counterfactual: the scan catches the shapes this change converted', () => { + // The two real regressions, reduced to their essentials. Before this guard + // was rewritten it reported ZERO offenders for both, because a single + // `withMockedFetch` elsewhere in the file exempted the whole file. + const liveFetchBesideAMock = ` + function withMockedFetch(fn, body) { return body(); } + test('mocked', async () => { + await withMockedFetch(async () => ({ ok: true }), async () => { await jspmGenerate([]); }); + }); + test('live', async () => { + const res = await fetch('https://api.jspm.io/generate', { method: 'POST' }); + }); + `; + const hits = findLiveCallers(liveFetchBesideAMock); + assert.equal(hits.length, 1, `expected exactly the live call, got ${JSON.stringify(hits)}`); + assert.equal(hits[0].kind, 'host'); + assert.equal(hits[0].what, 'api.jspm.io'); + + const unwrappedEntryBesideAMock = ` + function withMockedFetch(fn, body) { return body(); } + test('mocked', async () => { await withMockedFetch(m, () => pinAll(dir)); }); + test('live', async () => { const r = await pinAll(dir, { download: true }); }); + `; + const entryHits = findLiveCallers(unwrappedEntryBesideAMock); + assert.equal(entryHits.length, 1, `expected exactly the unwrapped call, got ${JSON.stringify(entryHits)}`); + assert.equal(entryHits[0].what, 'pinAll'); +}); + +test('counterfactual: the scan does not fire on the shapes that are genuinely safe', () => { + // A host inside an assertion, an expected-url string, or an importmap + // fixture is inert, and the suite is full of those on purpose. + assert.deepEqual(findLiveCallers(` + assert.match(url, /^https:\\/\\/ga\\.jspm\\.io\\/npm:picocolors@/); + const imports = { dayjs: 'https://ga.jspm.io/npm:dayjs@1.11.20/index.js' }; + // A comment mentioning api.jspm.io and calling fetch('https://api.jspm.io/generate'). + `), []); + + // A call inside a double, and one carrying the explicit marker. + assert.deepEqual(findLiveCallers(` + await withJspmDouble({}, async () => { await pinAll(dir); }); + // live-cdn-ok: no bare imports, so it returns before the resolve. + const r = await pinAll(emptyDir); + `), []); }); test('both runners drop live files unless the network is explicitly required', () => { diff --git a/website/app/docs/no-build/page.ts b/website/app/docs/no-build/page.ts index 7073116c8..acc3b4fb9 100644 --- a/website/app/docs/no-build/page.ts +++ b/website/app/docs/no-build/page.ts @@ -124,7 +124,7 @@ Pinning vendor packages from /home/me/my-app (downloading bundles)... Pinned 2 packages, wrote .webjs/vendor/importmap.json + 2 bundles.

This downloads each bundle from jspm.io to .webjs/vendor/<pkg>@<version>.js. The importmap then points at local /__webjs/vendor/<file>.js URLs; the server serves the committed bundle files. Browser never touches jspm.io at runtime; works fully offline.

Pin is intentionally manual (no predev/prestart auto-run). Auto-pin would cause silent churn in the committed importmap.json as jspm.io resolves URLs or transitive deps drift. Rails takes the same posture: bin/importmap pin is always developer-invoked.

-

sha384 SRI integrity is on by default, with OR without a pin file. An unpinned (live-resolved) app hashes each cross-origin bundle at warmup and the SSR pipeline stamps the matching hash on each <link rel="modulepreload"> and on the importmap entry itself, so the browser refuses to execute a bundle whose bytes don't match (CDN compromise defense), even before you pin. The live hashing is bounded and fail-open: if a bundle fetch fails (a CDN hiccup), that one URL simply loads without integrity (logged once) and the app still boots. Pin-time hashing is bounded the same way, so a CDN that accepts the connection and then stalls cannot hang webjs vendor pin; that bundle is pinned without a hash and the command tells you to rerun once jspm.io is healthy. Running webjs vendor pin makes the hashes reproducible and removes the warmup fetch: it writes them alongside the imports in importmap.json under an integrity key (both webjs vendor pin and --download populate it), so the hashes only update when the command is rerun and routine cache-busting cannot drop them.

+

sha384 SRI integrity is on by default, with OR without a pin file. An unpinned (live-resolved) app hashes each cross-origin bundle at warmup and the SSR pipeline stamps the matching hash on each <link rel="modulepreload"> and on the importmap entry itself, so the browser refuses to execute a bundle whose bytes don't match (CDN compromise defense), even before you pin. The live hashing is bounded and fail-open: if a bundle fetch fails (a CDN hiccup), that one URL simply loads without integrity (logged once) and the app still boots. Pin-time bundle fetches are bounded the same way, so a CDN that accepts the connection and then stalls cannot hang webjs vendor pin in either mode. What happens next differs by mode: the default pins that entry without a hash and tells you to rerun once jspm.io is healthy, while --download has no bytes to write, so it leaves the package out of the pin and reports it in the partial-success list. Running webjs vendor pin makes the hashes reproducible and removes the warmup fetch: it writes them alongside the imports in importmap.json under an integrity key (both webjs vendor pin and --download populate it), so the hashes only update when the command is rerun and routine cache-busting cannot drop them.

Switch CDN with --from

If jspm.io has an incident, or you want jsdelivr-served packages, pass a different resolver:

From e5f9f9eeff4d33ef44f22a33a59852bc149155df Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:50:12 +0530 Subject: [PATCH 09/18] fix: stop a regex literal from blinding the live-caller scan Three holes in the guard the previous commit rewrote, all found by reviewing that commit rather than the branch. The masker had no regex-literal awareness, and this suite is full of patterns like /rel=["']modulepreload["']/ whose quote characters it read as string delimiters. That desyncs the mask for the REST OF THE FILE, so every live call below such a line vanished from the scan. Eighteen test files carry the shape, including the app-boot tests. Demonstrated end to end: injecting a live fetch after the regex on blog-smoke.test.js:115 left the guard fully green, while the identical line above it redded. So the scan could report clean because it had gone blind, which is the same class of hole the previous commit was written to close. Telling a regex from a division needs the preceding token, so the usual heuristic goes in, with character classes and escapes handled and division still reading as division. The host lookup used a 200-character raw window, which crosses statements. A `fetch(localUrl)` followed two lines later by an assertion naming a jspm url read as a live call, and so did a comment mentioning one. It reads the call's actual argument list now, via the same paren matcher the guard ranges use. And the allowlist's `live` flag was read as merely falsy, so an entry added without the key skipped the *.live.test.* requirement while still collecting a whole-file exemption. It must be an explicit boolean now. Each of the three ships with the counterfactual that reproduces it, since the previous round's lesson was that this guard's own tests were the thing not being checked. --- test/fixtures/live-caller-scan.mjs | 111 +++++++++++++++++---- test/repo-health/live-cdn-callers.test.mjs | 48 ++++++++- 2 files changed, 140 insertions(+), 19 deletions(-) diff --git a/test/fixtures/live-caller-scan.mjs b/test/fixtures/live-caller-scan.mjs index 952c03266..0e2406817 100644 --- a/test/fixtures/live-caller-scan.mjs +++ b/test/fixtures/live-caller-scan.mjs @@ -42,24 +42,58 @@ export const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'find const GUARDS = ['withMockedFetch', 'withJspmDouble']; /** - * Blank out comments, strings, and template literals, replacing each with - * same-length filler so every index still lines up with the original source. + * Blank out comments, strings, template literals, and REGEX literals, replacing + * each with same-length filler so every index still lines up with the original + * source. * * Preserving offsets is what lets the guarded-range scan below run on the * masked text and still report positions in the real file. Blanking strings * matters for two different reasons: a host named inside a mock's expected-url * string is not a call, and an unbalanced parenthesis inside a string would - * otherwise wreck the brace matching. + * otherwise wreck the paren matching. + * + * Regex literals are the subtle one, and skipping them is not a rounding + * error: this suite is full of patterns like + * `/rel=["']modulepreload["']/`, whose quote characters would otherwise be + * read as string delimiters. That desyncs the mask for the REST OF THE FILE, + * so every live call below such a line silently disappears from the scan. + * Eighteen test files here carry that shape, including the app-boot tests, so + * a masker without this is a guard that reports clean because it went blind. + * + * Telling a regex from a division needs the preceding token, since `/` is + * both. The usual heuristic applies: a regex may start where a VALUE may not + * have just ended, so after an operator, an opening bracket, a comma, a + * semicolon, or a keyword like `return`, but not after an identifier, a + * number, or a closing bracket. * * @param {string} src * @returns {string} */ +/** Keywords after which a `/` opens a regex rather than dividing. */ +const REGEX_PRECEDING_WORDS = new Set([ + 'return', 'typeof', 'instanceof', 'in', 'of', 'new', 'delete', 'void', + 'throw', 'case', 'do', 'else', 'yield', 'await', +]); + export function maskLiterals(src) { const out = src.split(''); let i = 0; const blank = (from, to) => { for (let k = from; k < to && k < out.length; k++) if (out[k] !== '\n') out[k] = ' '; }; + + /** The last token character that was not whitespace and not masked away. */ + let prev = ''; + /** The identifier immediately before `prev`, when `prev` ends a word. */ + let prevWord = ''; + + const regexCanStartHere = () => { + if (!prev) return true; + if (/[)\]}]/.test(prev)) return false; + if (/[A-Za-z0-9_$]/.test(prev)) return REGEX_PRECEDING_WORDS.has(prevWord); + return true; + }; + while (i < src.length) { const c = src[i]; const next = src[i + 1]; @@ -73,6 +107,25 @@ export function maskLiterals(src) { const stop = end === -1 ? src.length : end; blank(i, stop); i = stop; continue; } + if (c === '/' && regexCanStartHere()) { + // Scan to the closing delimiter, honouring escapes and character + // classes (a `/` inside `[...]` does not end the literal). + let j = i + 1; + let inClass = false; + let closed = false; + while (j < src.length && src[j] !== '\n') { + const d = src[j]; + if (d === '\\') { j += 2; continue; } + if (d === '[') inClass = true; + else if (d === ']') inClass = false; + else if (d === '/' && !inClass) { closed = true; break; } + j++; + } + if (closed) { + blank(i + 1, j); prev = '/'; prevWord = ''; i = j + 1; continue; + } + // An unterminated `/` was a division after all. Fall through. + } if (c === '"' || c === "'" || c === '`') { let j = i + 1; while (j < src.length) { @@ -82,13 +135,37 @@ export function maskLiterals(src) { } // Blank the CONTENTS but keep the quotes, so a masked string is still // recognisably a string and cannot merge with the token beside it. - blank(i + 1, j); i = j + 1; continue; + blank(i + 1, j); prev = c; prevWord = ''; i = j + 1; continue; + } + if (!/\s/.test(c)) { + if (/[A-Za-z0-9_$]/.test(c)) { + prevWord = /[A-Za-z0-9_$]/.test(prev) ? prevWord + c : c; + } else { + prevWord = ''; + } + prev = c; } i++; } return out.join(''); } +/** + * Index of the `)` matching the `(` at `openIdx`, or -1. + * @param {string} masked @param {number} openIdx + */ +function matchParen(masked, openIdx) { + let depth = 0; + for (let k = openIdx; k < masked.length; k++) { + if (masked[k] === '(') depth++; + else if (masked[k] === ')') { + depth--; + if (depth === 0) return k; + } + } + return -1; +} + /** * Character ranges covered by a `withMockedFetch(` / `withJspmDouble(` call, * from its opening parenthesis to its match. @@ -103,14 +180,8 @@ export function guardedRanges(masked) { const re = new RegExp(`\\b${guard}\\s*\\(`, 'g'); for (const m of masked.matchAll(re)) { const open = m.index + m[0].length - 1; - let depth = 0; - for (let k = open; k < masked.length; k++) { - if (masked[k] === '(') depth++; - else if (masked[k] === ')') { - depth--; - if (depth === 0) { ranges.push([open, k]); break; } - } - } + const close = matchParen(masked, open); + if (close !== -1) ranges.push([open, close]); } } return ranges; @@ -139,13 +210,17 @@ export function findLiveCallers(src) { const found = []; // A host literal has to be read from the ORIGINAL source, since masking - // blanks string contents. Only its position is taken from the mask, and a - // `fetch(` whose argument names a live host is what counts; the same host in - // an assertion or an importmap fixture is inert, and the suite is full of - // those on purpose. + // blanks string contents. Only the BOUNDS come from the mask, and they are + // the call's actual argument list rather than a fixed character window: a + // window crosses statement boundaries, so `fetch(localUrl)` followed two + // lines later by an assertion naming a jspm url read as a live call. The + // same host in an assertion, a comment, or an importmap fixture is inert, + // and this suite is full of those on purpose. for (const m of masked.matchAll(/\bfetch\s*\(/g)) { - const argStart = m.index + m[0].length; - const arg = src.slice(argStart, argStart + 200); + const open = m.index + m[0].length - 1; + const close = matchParen(masked, open); + if (close === -1) continue; + const arg = src.slice(open + 1, close); const host = LIVE_HOSTS.find((h) => arg.includes(h)); if (host && !guarded(m.index)) found.push({ kind: 'host', what: host, line: lineAt(m.index) }); } diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index 5877e5921..66ea68313 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -105,11 +105,17 @@ for (const pkg of readdirSync(join(ROOT, 'packages'), { withFileTypes: true })) const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); -test('every allowlisted LIVE caller is a *.live.test.* file that exists', () => { +test('every allowlist entry declares itself and, if live, is a *.live.test.* file', () => { for (const entry of LIVE_CALLERS) { assert.ok(files.some((f) => rel(f) === entry.file), `${entry.file} is allowlisted but no such test file exists`); assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`); + // `live` is REQUIRED and must be boolean. Reading it as merely falsy would + // let an entry added without the key skip the marker check below while + // still collecting a whole-file scan exemption, which is the quietest + // possible way to reopen the hole this guard exists to close. + assert.equal(typeof entry.live, 'boolean', + `${entry.file} must state \`live: true\` or \`live: false\` explicitly`); if (entry.live) { assert.ok(entry.file.includes(LIVE_MARKER), `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); @@ -180,6 +186,46 @@ test('counterfactual: the scan does not fire on the shapes that are genuinely sa `), []); }); +test('counterfactual: a regex literal carrying a quote does not blind the scan', () => { + // The subtlest failure this guard can have, and it went undetected until a + // review reproduced it. A pattern like /rel=["']modulepreload["']/ has quote + // characters in it; a masker without regex awareness reads the first one as + // a string opener and desyncs for the REST OF THE FILE, so every live call + // below that line silently vanishes from the scan. Eighteen test files here + // carry that shape, including the app-boot tests. + const afterARegex = [ + 'const re = /]+rel=["\']modulepreload["\']/g;', + 'const leak = await fetch("https://api.jspm.io/generate", { method: "POST" });', + ].join('\n'); + assert.deepEqual(findLiveCallers(afterARegex).map((h) => h.what), ['api.jspm.io'], + 'a live call after a quote-bearing regex must still be seen'); + + // The other half: a `/` that is division must NOT be read as a regex, or the + // mask desyncs the other way and starts swallowing real code. + const withDivision = [ + 'const half = total / 2;', + 'const other = count / 4;', + 'await fetch("https://api.jspm.io/generate");', + ].join('\n'); + assert.deepEqual(findLiveCallers(withDivision).map((h) => h.what), ['api.jspm.io']); +}); + +test('counterfactual: a host named near, but not inside, a fetch call is inert', () => { + // The scan reads the call's actual argument list, not a character window. A + // window crossed statement boundaries, so a local fetch followed by an + // assertion naming a jspm url read as a live call. + assert.deepEqual(findLiveCallers([ + 'const r = await fetch(localUrl);', + 'assert.equal(r.status, 200);', + 'assert.equal(map.dayjs, "https://ga.jspm.io/npm:dayjs@1.11.20/index.js");', + ].join('\n')), []); + + assert.deepEqual(findLiveCallers([ + 'await fetch(baseUrl);', + '// TODO: point this at the double instead of ga.jspm.io one day.', + ].join('\n')), []); +}); + test('both runners drop live files unless the network is explicitly required', () => { // The policy above is only worth anything because the runners enforce it, so // assert the enforcement rather than trusting it. A refactor that renames From d4fcc5a7b7640b19aa3c4913df4378c4a57177eb Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 00:06:51 +0530 Subject: [PATCH 10/18] fix: deny third-party hosts at runtime instead of scanning for them Three review rounds found three different ways the static scan went blind, and each fix opened a new hole. A file-level exemption excused every live call in any file containing one withMockedFetch. Adding regex-literal awareness then desynced the mask on nested html templates, because the slash in a closing tag read as a regex opener and swallowed the template's backtick, blinding thirteen more files. Measured: 30 files blind before that fix, 15 after, 13 of them new. The fourth heuristic would not have been right either. Deciding whether a slash opens a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup is going to keep being wrong. A scan also cannot see the callers that matter most: the app-boot tests reach jspm through resolveVendorImports with no fetch and no vendor entry point in their source. So the runners preload a deny instead. It answers 503 for jspm.io and registry.npmjs.org unless WEBJS_REQUIRE_NETWORK is set, needs no parsing, has no blind spots, and covers the transitive callers. A test that depends on a third party now fails on every run rather than only during an outage, so it surfaces the day it is written. 503 rather than a throw because every fetch caller in vendor.js catches: a rejection would be swallowed, while 503 is the shape those sites already classify as transient. Verified: npm test with the deny installed is 3915 tests and the same five failures that fail identically on a clean origin/main baseline. The deny prints what it refuses, which is how the app-boot tests were confirmed to reach jspm. The eleven live-cdn-ok markers stay. They are accurate notes about why those calls return before dialling, and they cost nothing now that no scanner reads them. --- framework-dev.md | 8 +- scripts/run-bun-tests.js | 8 +- scripts/run-node-tests.js | 14 +- test/fixtures/deny-live-hosts.mjs | 89 ++++++++ test/fixtures/live-caller-scan.mjs | 235 -------------------- test/repo-health/live-cdn-callers.test.mjs | 239 +++++++-------------- 6 files changed, 189 insertions(+), 404 deletions(-) create mode 100644 test/fixtures/deny-live-hosts.mjs delete mode 100644 test/fixtures/live-caller-scan.mjs diff --git a/framework-dev.md b/framework-dev.md index 28bed07cd..70e9fb03f 100644 --- a/framework-dev.md +++ b/framework-dev.md @@ -123,9 +123,13 @@ This replaced a `WEBJS_SKIP_NETWORK_TESTS` gate that could not work: it was opt- Four things to keep in mind when touching this. -**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. +**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. The runtime deny answers 503 for the same reason, since that is the shape those call sites classify as transient. -**The guard is per call, and it has one blind spot.** `test/repo-health/live-cdn-callers.test.mjs` reds when a live host reaches a `fetch(`, or a vendor entry point (`pinAll` / `updatePinned` / `auditPinned` / `findOutdated`) is called, outside an allowlisted file, a `withJspmDouble` / `withMockedFetch` body, or a `// live-cdn-ok: ` marker. Per call matters: the first version was file-level, and a single `withMockedFetch` anywhere in `vendor.test.js` exempted its live `fetch(api.jspm.io)` and all four unwrapped entry points, so the guard reported zero offenders for the exact file this change had to fix. The blind spot is a SPAWNED child: `test/vendor-cli/vendor-cli.test.mjs` reaches jspm by running the CLI in another process, so a static scan of its source sees nothing. What covers that file is the `[jspm-double] armed` stderr assertion inside its own `runCli`, which fires on every spawn. A new spawning test needs the same treatment. +**The deny is at RUNTIME, and that was learned the hard way.** Both runners preload `test/fixtures/deny-live-hosts.mjs`, which answers 503 for jspm.io and registry.npmjs.org unless `WEBJS_REQUIRE_NETWORK` is set. It needs no parsing and has no blind spots, and it covers the transitive callers a source scan structurally cannot see: the app-boot tests reach jspm through `resolveVendorImports` with no `fetch(` anywhere in their own source. A test that depends on a third party now fails on EVERY run rather than only during an outage, which arrives the day it is written instead of months later. + +The first three attempts were a STATIC scan over test sources, and each went blind a different way: a file-level exemption, so one `withMockedFetch` anywhere excused every live call in the file; then no regex-literal awareness, so `/rel=["']modulepreload["']/` desynced the mask to end of file; then regex awareness that read the `/` in `` inside a nested `` html`...` `` template as a regex opener, swallowing the closing backtick. Each fix opened a new hole, because deciding whether a `/` starts a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup will keep being wrong. **Do not reintroduce it.** If the deny needs to be tighter, tighten the deny. + +**A spawned child does not inherit the deny.** `test/vendor-cli/vendor-cli.test.mjs` runs the CLI in another process, so it passes its own preload and asserts a `[jspm-double] armed` marker on every spawn, which reds all ten of its tests if the flag is dropped. A new test that spawns a process and vendors needs the same treatment. **The nightly is what stops a permanent skip from hiding.** `.github/workflows/vendor-cdn.yml` runs the live files with `WEBJS_REQUIRE_NETWORK=1`, which both selects them AND promotes their upstream-trouble skip into a failure. Without that second half a permanently skipping test is indistinguishable from a passing one. A failure opens or comments on one fixed-title tracking issue, since GitHub notifies only the workflow file's last committer about a failed scheduled run. diff --git a/scripts/run-bun-tests.js b/scripts/run-bun-tests.js index d3af2ceef..f19adf81d 100644 --- a/scripts/run-bun-tests.js +++ b/scripts/run-bun-tests.js @@ -99,6 +99,12 @@ const excludeSegs = [`${SEP}browser${SEP}`, `${SEP}e2e${SEP}`, `${SEP}examples${ // nightly `vendor-cdn` workflow is what asks. const LIVE_MARKER = '.live.test.'; const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK); +// Same third-party deny the node runner installs, so a jspm outage cannot red +// this job either (#1150). Bun ignores NODE_OPTIONS, hence the explicit flag. +const denyArgs = wantsNetwork + ? [] + : ['--preload', resolve(ROOT, 'test', 'fixtures', 'deny-live-hosts.mjs')]; + const filter = (process.env.WEBJS_BUN_TESTS || '').split(',').map((s) => s.trim()).filter(Boolean); // Repo-relative path, always forward-slashed so DENYLIST matching is OS-stable. const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); @@ -131,7 +137,7 @@ for (const f of files) { console.log(`SKIP(node-only) ${rel(f)}`); continue; } - const r = spawnSync(BUN, ['test', f], { + const r = spawnSync(BUN, [...denyArgs, 'test', f], { cwd: ROOT, encoding: 'utf8', timeout: PER_FILE_TIMEOUT_MS, env: { ...process.env, FORCE_COLOR: '0' }, }); diff --git a/scripts/run-node-tests.js b/scripts/run-node-tests.js index bc544ce06..394446430 100644 --- a/scripts/run-node-tests.js +++ b/scripts/run-node-tests.js @@ -15,7 +15,7 @@ import { spawn } from 'node:child_process'; import { readdirSync, statSync } from 'node:fs'; import { join, sep } from 'node:path'; import { resolve, dirname } from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; const __dirname = dirname(fileURLToPath(import.meta.url)); const ROOT = resolve(__dirname, '..'); @@ -89,6 +89,16 @@ const coverageArgs = process.env.WEBJS_COVERAGE ] : []; -const args = ['--test', ...coverageArgs, ...files]; +// Deny outbound calls to jspm.io / registry.npmjs.org for the whole run, so +// this REQUIRED job cannot be redded by a third-party outage (#1150). Off when +// the caller explicitly asked for the network, which is the same switch that +// selects the *.live.test.* files above. Passed as argv rather than +// NODE_OPTIONS because Bun ignores that variable and the sibling bun runner +// uses the same fixture. +const denyArgs = wantsNetwork + ? [] + : ['--import', pathToFileURL(resolve(ROOT, 'test', 'fixtures', 'deny-live-hosts.mjs')).href]; + +const args = ['--test', ...denyArgs, ...coverageArgs, ...files]; const child = spawn(process.execPath, args, { stdio: 'inherit' }); child.on('exit', (code) => process.exit(code ?? 1)); diff --git a/test/fixtures/deny-live-hosts.mjs b/test/fixtures/deny-live-hosts.mjs new file mode 100644 index 000000000..610409e83 --- /dev/null +++ b/test/fixtures/deny-live-hosts.mjs @@ -0,0 +1,89 @@ +/** + * Refuse outbound calls to a third-party host for the whole test run (#1150). + * + * Loaded by `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` into + * the test process, so no required check can depend on jspm.io or + * registry.npmjs.org being up. `WEBJS_REQUIRE_NETWORK=1` turns it off, which + * is the same switch that selects the `*.live.test.*` files. + * + * WHY THIS SHAPE, after three attempts at the other one. The first version of + * this guard was a STATIC scan: mask a test file's strings and comments, then + * look for a live host inside a `fetch(`. Three review rounds found three + * different ways it went blind, each one hiding every call below it in the + * file, and each fix opened a new hole: + * + * 1. A file-level exemption, so one `withMockedFetch` anywhere excused every + * live call in the file. It reported ZERO offenders for the very file + * this change had to convert. + * 2. No regex-literal awareness, so `/rel=["']modulepreload["']/` desynced + * the mask from that line to EOF. Eighteen files carry that shape. + * 3. Regex awareness that then read the `/` in `` inside a nested + * ``html`...` `` template as a regex opener, swallowing the closing + * backtick and blinding thirteen more files. + * + * The lesson is not that the fourth heuristic would have been right. Deciding + * whether a `/` opens a regex requires lexing JavaScript, and a hand-rolled + * lexer facing nested template literals holding markup is going to keep being + * wrong. A static scan is also structurally unable to see the callers that + * matter most here: the app-boot tests reach jspm transitively through + * `resolveVendorImports`, with no `fetch(` and no vendor entry point anywhere + * in their source. + * + * Denying at runtime needs no parsing and has no blind spots. A test that + * depends on a third party now fails on EVERY run rather than only during an + * outage, which is a better signal than any scan could give, and it arrives + * the day the test is written instead of months later. + * + * WHY A 503 RATHER THAN A THROW. Every fetch caller in + * `packages/server/src/vendor.js` catches, so a throw is indistinguishable + * from a network error and would be swallowed. A 503 is the shape those call + * sites already classify as transient, so vendor resolution degrades exactly + * as it does during a real outage, which is the behaviour under test. It also + * keeps the app-boot tests passing: they fail open and assert nothing about a + * vendor entry, verified by running the whole suite this way. + */ + +/** Hosts no required check may depend on. */ +export const DENIED_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; + +/** + * Install the deny on a fetch-like function. + * + * Exported separately from the self-install below so the guard test can + * exercise it without patching its own process. + * + * @param {(input: any, init?: any) => Promise} realFetch + * @param {(url: string) => void} [onDenied] + * @returns {(input: any, init?: any) => Promise} + */ +export function denyLiveHosts(realFetch, onDenied) { + return async function deniedFetch(input, init) { + const url = typeof input === 'string' ? input + : input instanceof URL ? input.href + : (input && input.url) || ''; + const host = DENIED_HOSTS.find((h) => url.includes(h)); + if (!host) return realFetch(input, init); + if (onDenied) onDenied(url); + return new Response( + JSON.stringify({ error: `Error: ${host} is denied during the test run` }), + { status: 503, headers: { 'content-type': 'application/json' } }, + ); + }; +} + +if (!process.env.WEBJS_REQUIRE_NETWORK) { + /** @type {Set} */ + const seen = new Set(); + const real = globalThis.fetch; + globalThis.fetch = /** @type {any} */ (denyLiveHosts(real, (url) => { + // One line per distinct url, not per call, so a warmup that resolves + // twenty packages does not bury the run. Visible on purpose: a required + // test reaching a third party is worth knowing about even when it degrades + // cleanly, and this is the list to work through if that ever stops being + // acceptable. + const key = url.split('?')[0]; + if (seen.has(key)) return; + seen.add(key); + process.stderr.write(`[deny-live-hosts] refused ${key}\n`); + })); +} diff --git a/test/fixtures/live-caller-scan.mjs b/test/fixtures/live-caller-scan.mjs deleted file mode 100644 index 0e2406817..000000000 --- a/test/fixtures/live-caller-scan.mjs +++ /dev/null @@ -1,235 +0,0 @@ -/** - * Find test code that reaches a third-party host (#1150). - * - * Split out from `test/repo-health/live-cdn-callers.test.mjs` so the analysis - * can be exercised against inline fixtures rather than only against whatever - * the tree happens to contain today. That is not a style preference: the first - * version of the guard was file-level, and measured against the pre-PR tree it - * flagged NEITHER of the two files the change converts. `vendor.test.js` - * carried a live `fetch('https://api.jspm.io/generate')` and four unwrapped - * vendor entry points, and a single `withMockedFetch` elsewhere in the same - * file exempted all of it. A guard with an empty counterfactual is worse than - * no guard, because it reads as protection. - * - * So the exemption is PER CALL. A live host or a vendor entry point is fine - * only where it sits lexically inside a `withMockedFetch(...)` or - * `withJspmDouble(...)` argument list, which is the shape that actually - * controls `globalThis.fetch` for the duration of that call. - * - * The second exemption is an explicit per-site marker, `// live-cdn-ok: why`, - * on or just above the call. Several entry-point calls genuinely cannot reach - * the network (a `pinAll` on an app with no resolvable bare imports returns - * before the resolve, an `auditPinned` with no pin file short-circuits, a - * provider-validation test rejects before dialling), and wrapping those in a - * double to satisfy a checker would be churn that teaches the wrong thing. The - * marker records the reason at the site instead. It is deliberately noisy to - * add, so it is a decision rather than a default, and unlike the file-level - * flag it replaced it exempts exactly one call. - * - * This module has NO side effects. - */ - -/** Third-party hosts no required check may depend on. */ -export const LIVE_HOSTS = ['api.jspm.io', 'ga.jspm.io', 'registry.npmjs.org']; - -/** - * Vendor entry points that reach a live host internally, so naming one is as - * live as calling fetch yourself. - */ -export const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'findOutdated']; - -/** Helpers that install a `fetch` the test controls for the duration of a call. */ -const GUARDS = ['withMockedFetch', 'withJspmDouble']; - -/** - * Blank out comments, strings, template literals, and REGEX literals, replacing - * each with same-length filler so every index still lines up with the original - * source. - * - * Preserving offsets is what lets the guarded-range scan below run on the - * masked text and still report positions in the real file. Blanking strings - * matters for two different reasons: a host named inside a mock's expected-url - * string is not a call, and an unbalanced parenthesis inside a string would - * otherwise wreck the paren matching. - * - * Regex literals are the subtle one, and skipping them is not a rounding - * error: this suite is full of patterns like - * `/rel=["']modulepreload["']/`, whose quote characters would otherwise be - * read as string delimiters. That desyncs the mask for the REST OF THE FILE, - * so every live call below such a line silently disappears from the scan. - * Eighteen test files here carry that shape, including the app-boot tests, so - * a masker without this is a guard that reports clean because it went blind. - * - * Telling a regex from a division needs the preceding token, since `/` is - * both. The usual heuristic applies: a regex may start where a VALUE may not - * have just ended, so after an operator, an opening bracket, a comma, a - * semicolon, or a keyword like `return`, but not after an identifier, a - * number, or a closing bracket. - * - * @param {string} src - * @returns {string} - */ -/** Keywords after which a `/` opens a regex rather than dividing. */ -const REGEX_PRECEDING_WORDS = new Set([ - 'return', 'typeof', 'instanceof', 'in', 'of', 'new', 'delete', 'void', - 'throw', 'case', 'do', 'else', 'yield', 'await', -]); - -export function maskLiterals(src) { - const out = src.split(''); - let i = 0; - const blank = (from, to) => { - for (let k = from; k < to && k < out.length; k++) if (out[k] !== '\n') out[k] = ' '; - }; - - /** The last token character that was not whitespace and not masked away. */ - let prev = ''; - /** The identifier immediately before `prev`, when `prev` ends a word. */ - let prevWord = ''; - - const regexCanStartHere = () => { - if (!prev) return true; - if (/[)\]}]/.test(prev)) return false; - if (/[A-Za-z0-9_$]/.test(prev)) return REGEX_PRECEDING_WORDS.has(prevWord); - return true; - }; - - while (i < src.length) { - const c = src[i]; - const next = src[i + 1]; - if (c === '/' && next === '*') { - const end = src.indexOf('*/', i + 2); - const stop = end === -1 ? src.length : end + 2; - blank(i, stop); i = stop; continue; - } - if (c === '/' && next === '/') { - const end = src.indexOf('\n', i); - const stop = end === -1 ? src.length : end; - blank(i, stop); i = stop; continue; - } - if (c === '/' && regexCanStartHere()) { - // Scan to the closing delimiter, honouring escapes and character - // classes (a `/` inside `[...]` does not end the literal). - let j = i + 1; - let inClass = false; - let closed = false; - while (j < src.length && src[j] !== '\n') { - const d = src[j]; - if (d === '\\') { j += 2; continue; } - if (d === '[') inClass = true; - else if (d === ']') inClass = false; - else if (d === '/' && !inClass) { closed = true; break; } - j++; - } - if (closed) { - blank(i + 1, j); prev = '/'; prevWord = ''; i = j + 1; continue; - } - // An unterminated `/` was a division after all. Fall through. - } - if (c === '"' || c === "'" || c === '`') { - let j = i + 1; - while (j < src.length) { - if (src[j] === '\\') { j += 2; continue; } - if (src[j] === c) break; - j++; - } - // Blank the CONTENTS but keep the quotes, so a masked string is still - // recognisably a string and cannot merge with the token beside it. - blank(i + 1, j); prev = c; prevWord = ''; i = j + 1; continue; - } - if (!/\s/.test(c)) { - if (/[A-Za-z0-9_$]/.test(c)) { - prevWord = /[A-Za-z0-9_$]/.test(prev) ? prevWord + c : c; - } else { - prevWord = ''; - } - prev = c; - } - i++; - } - return out.join(''); -} - -/** - * Index of the `)` matching the `(` at `openIdx`, or -1. - * @param {string} masked @param {number} openIdx - */ -function matchParen(masked, openIdx) { - let depth = 0; - for (let k = openIdx; k < masked.length; k++) { - if (masked[k] === '(') depth++; - else if (masked[k] === ')') { - depth--; - if (depth === 0) return k; - } - } - return -1; -} - -/** - * Character ranges covered by a `withMockedFetch(` / `withJspmDouble(` call, - * from its opening parenthesis to its match. - * - * @param {string} masked output of {@link maskLiterals} - * @returns {Array<[number, number]>} - */ -export function guardedRanges(masked) { - /** @type {Array<[number, number]>} */ - const ranges = []; - for (const guard of GUARDS) { - const re = new RegExp(`\\b${guard}\\s*\\(`, 'g'); - for (const m of masked.matchAll(re)) { - const open = m.index + m[0].length - 1; - const close = matchParen(masked, open); - if (close !== -1) ranges.push([open, close]); - } - } - return ranges; -} - -/** - * Live callers in one file, each with the 1-indexed line it sits on. - * - * @param {string} src - * @returns {Array<{ kind: 'host' | 'entry', what: string, line: number }>} - */ -export function findLiveCallers(src) { - const masked = maskLiterals(src); - const ranges = guardedRanges(masked); - const lines = src.split('\n'); - const lineAt = (idx) => src.slice(0, idx).split('\n').length; - const inGuard = (idx) => ranges.some(([a, b]) => idx > a && idx < b); - // The marker is read from the ORIGINAL source, since masking blanks - // comments. Three lines of lookback, so it can sit above a call that wraps. - const marked = (line) => lines - .slice(Math.max(0, line - 4), line) - .some((l) => l.includes('live-cdn-ok:')); - const guarded = (idx) => inGuard(idx) || marked(lineAt(idx)); - - /** @type {Array<{ kind: 'host' | 'entry', what: string, line: number }>} */ - const found = []; - - // A host literal has to be read from the ORIGINAL source, since masking - // blanks string contents. Only the BOUNDS come from the mask, and they are - // the call's actual argument list rather than a fixed character window: a - // window crosses statement boundaries, so `fetch(localUrl)` followed two - // lines later by an assertion naming a jspm url read as a live call. The - // same host in an assertion, a comment, or an importmap fixture is inert, - // and this suite is full of those on purpose. - for (const m of masked.matchAll(/\bfetch\s*\(/g)) { - const open = m.index + m[0].length - 1; - const close = matchParen(masked, open); - if (close === -1) continue; - const arg = src.slice(open + 1, close); - const host = LIVE_HOSTS.find((h) => arg.includes(h)); - if (host && !guarded(m.index)) found.push({ kind: 'host', what: host, line: lineAt(m.index) }); - } - - for (const fn of LIVE_ENTRY_POINTS) { - for (const m of masked.matchAll(new RegExp(`\\b${fn}\\s*\\(`, 'g'))) { - if (!guarded(m.index)) found.push({ kind: 'entry', what: fn, line: lineAt(m.index) }); - } - } - - return found.sort((a, b) => a.line - b.line); -} diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index 66ea68313..e59fa74c0 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -1,38 +1,26 @@ /** - * Live third-party calls belong only in `*.live.test.*` files (#1150). + * No required check may FAIL because a third party is down (#1150). * - * That is the whole policy, and it is worth enforcing mechanically because the - * failure it prevents is invisible: a test that quietly reaches api.jspm.io or - * registry.npmjs.org passes every day until the third party has a bad hour, - * and then reds a pull request that never touched vendoring. PR #1149, a - * five-file documentation change, is what finally made the case. The old - * `WEBJS_SKIP_NETWORK_TESTS` convention could not prevent it: it was opt-OUT, - * so CI always ran live, and two `registry.npmjs.org` callers were never - * covered by it at all. + * The required `Unit + integration` job used to resolve vendors against the + * live jspm CDN, so a jspm outage redded pull requests that had nothing to do + * with vendoring. PR #1149, a five-file documentation change, is what finally + * made the case: it failed on the `#448` gitignore-healing test and passed on a + * re-run of the identical commit. * - * The analysis lives in `test/fixtures/live-caller-scan.mjs` so it can be run - * against inline fixtures rather than only against whatever the tree happens - * to contain, which is what the `counterfactual` tests below do. The first - * version of this guard was file-level and, measured against the pre-PR tree, - * flagged NEITHER of the two files this change converts: a single - * `withMockedFetch` anywhere in `vendor.test.js` exempted its live - * `fetch(api.jspm.io)` and all four of its unwrapped entry points. So the - * exemption is per call now, and the counterfactual is a real one. + * Two mechanisms enforce that, and this file asserts both. * - * Measured against the pre-PR tree, the rewritten scan reports 24 offenders in - * `vendor.test.js`, starting with the live `fetch('https://api.jspm.io/generate')`. + * The RUNTIME DENY (`test/fixtures/deny-live-hosts.mjs`) is loaded by both test + * runners and answers 503 for jspm.io and registry.npmjs.org. That covers every + * caller, including the app-boot tests that reach jspm transitively through + * `resolveVendorImports` with no `fetch(` anywhere in their own source. * - * ONE BLIND SPOT, stated rather than papered over: it cannot see a SPAWNED - * child. `test/vendor-cli/vendor-cli.test.mjs` reaches jspm by running the CLI - * in another process, so it contains no `fetch(` and no entry-point call, and - * this scan reports zero for it both before and after the change. What covers - * that file is the `[jspm-double] armed` assertion inside its own `runCli`, - * which fires on every spawn and reds all ten of its tests if the preload flag - * is dropped. A future spawning test needs the same treatment; a static scan - * of the parent's source cannot give it. + * The FILENAME RULE keeps the genuinely-live tests out of a normal run: both + * runners skip `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, which is the + * same switch that lifts the deny. * - * Both test runners drop `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, so - * a file on the allowlist below genuinely cannot run in a required check. + * This file used to hold a static scan instead, and three review rounds found + * three different ways it went blind. `deny-live-hosts.mjs` carries that + * history and the reason a fourth heuristic was not the answer. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -40,41 +28,28 @@ import { readdirSync, readFileSync } from 'node:fs'; import { join, resolve, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { findLiveCallers } from '../fixtures/live-caller-scan.mjs'; +import { denyLiveHosts, DENIED_HOSTS } from '../fixtures/deny-live-hosts.mjs'; const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); /** - * Files allowed to reach a third party, each with what it asserts and why it - * has to be live. Shaped like `scripts/run-bun-tests.js`'s DENYLIST on + * The files allowed to reach a third party, each with what it asserts and why + * it has to be live. Shaped like `scripts/run-bun-tests.js`'s DENYLIST on * purpose: a reason per entry, so adding one is a decision somebody wrote down * rather than a guard somebody silenced. - * - * Every entry MUST be a `*.live.test.*` path, which is what the runners key - * on, with ONE exception recorded below. */ const LIVE_CALLERS = [ { file: 'packages/server/test/vendor/jspm-cdn.live.test.js', - live: true, why: 'the two things an offline double cannot vouch for: that our merged output equals ' + "jspm's own unified graph (#446), and that jspm still fails a WHOLE batch permanently " + 'when one install is unresolvable, which the entire fallback ladder in vendor.js assumes.', }, { file: 'test/vendor-cli/vendor-pin.live.test.mjs', - live: true, why: 'one real run of the command a user actually types, so `webjs vendor pin` does not ' + 'become a thing that is only ever exercised against a fixture.', }, - { - file: 'test/repo-health/e2e-vendor-stub.test.mjs', - live: false, - why: 'NOT a live caller. It POSTs to api.jspm.io on purpose to exercise the real request ' - + 'shape, having installed a sentinel fetch at module scope BEFORE importing the fixture ' - + 'under test, which is the whole point of that ordering (#1228). The sentinel is a bare ' - + 'assignment outside any call, so a per-call scan cannot see it covering anything.', - }, ]; const LIVE_MARKER = '.live.test.'; @@ -105,134 +80,70 @@ for (const pkg of readdirSync(join(ROOT, 'packages'), { withFileTypes: true })) const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/'); -test('every allowlist entry declares itself and, if live, is a *.live.test.* file', () => { - for (const entry of LIVE_CALLERS) { - assert.ok(files.some((f) => rel(f) === entry.file), - `${entry.file} is allowlisted but no such test file exists`); - assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`); - // `live` is REQUIRED and must be boolean. Reading it as merely falsy would - // let an entry added without the key skip the marker check below while - // still collecting a whole-file scan exemption, which is the quietest - // possible way to reopen the hole this guard exists to close. - assert.equal(typeof entry.live, 'boolean', - `${entry.file} must state \`live: true\` or \`live: false\` explicitly`); - if (entry.live) { - assert.ok(entry.file.includes(LIVE_MARKER), - `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); - } - } -}); - -test('no test outside the allowlist reaches a third party', () => { - const allowed = new Set(LIVE_CALLERS.map((e) => e.file)); +test('the deny answers every third-party host and passes everything else through', async () => { /** @type {string[]} */ - const offenders = []; - for (const file of files) { - const path = rel(file); - if (allowed.has(path)) continue; - for (const hit of findLiveCallers(readFileSync(file, 'utf8'))) { - offenders.push(`${path}:${hit.line} ${hit.kind === 'host' ? `fetch(${hit.what})` : `${hit.what}()`}`); - } + const passedThrough = []; + /** @type {string[]} */ + const denied = []; + const fetch = denyLiveHosts( + async (input) => { passedThrough.push(String(input)); return new Response('real', { status: 200 }); }, + (url) => denied.push(url), + ); + + for (const host of DENIED_HOSTS) { + const res = await fetch(`https://${host}/whatever`, { method: 'POST' }); + assert.equal(res.status, 503, `${host} must be denied`); + // 503 rather than a throw, because every fetch caller in vendor.js catches: + // a rejection would be swallowed, while a 503 is the shape those call sites + // already classify as transient, so resolution degrades exactly as it does + // during a real outage. + assert.match((await res.json()).error, new RegExp(host)); } - assert.deepEqual(offenders, [], - 'these reach a third party from a file a required check runs. Wrap the call in ' - + 'withJspmDouble (test/fixtures/jspm-double.mjs), move it into a *.live.test.* file and ' - + 'allowlist that, or if the call provably returns before dialling, mark the site with a ' - + '`// live-cdn-ok: ` comment.'); -}); - -test('counterfactual: the scan catches the shapes this change converted', () => { - // The two real regressions, reduced to their essentials. Before this guard - // was rewritten it reported ZERO offenders for both, because a single - // `withMockedFetch` elsewhere in the file exempted the whole file. - const liveFetchBesideAMock = ` - function withMockedFetch(fn, body) { return body(); } - test('mocked', async () => { - await withMockedFetch(async () => ({ ok: true }), async () => { await jspmGenerate([]); }); - }); - test('live', async () => { - const res = await fetch('https://api.jspm.io/generate', { method: 'POST' }); - }); - `; - const hits = findLiveCallers(liveFetchBesideAMock); - assert.equal(hits.length, 1, `expected exactly the live call, got ${JSON.stringify(hits)}`); - assert.equal(hits[0].kind, 'host'); - assert.equal(hits[0].what, 'api.jspm.io'); - - const unwrappedEntryBesideAMock = ` - function withMockedFetch(fn, body) { return body(); } - test('mocked', async () => { await withMockedFetch(m, () => pinAll(dir)); }); - test('live', async () => { const r = await pinAll(dir, { download: true }); }); - `; - const entryHits = findLiveCallers(unwrappedEntryBesideAMock); - assert.equal(entryHits.length, 1, `expected exactly the unwrapped call, got ${JSON.stringify(entryHits)}`); - assert.equal(entryHits[0].what, 'pinAll'); -}); - -test('counterfactual: the scan does not fire on the shapes that are genuinely safe', () => { - // A host inside an assertion, an expected-url string, or an importmap - // fixture is inert, and the suite is full of those on purpose. - assert.deepEqual(findLiveCallers(` - assert.match(url, /^https:\\/\\/ga\\.jspm\\.io\\/npm:picocolors@/); - const imports = { dayjs: 'https://ga.jspm.io/npm:dayjs@1.11.20/index.js' }; - // A comment mentioning api.jspm.io and calling fetch('https://api.jspm.io/generate'). - `), []); - - // A call inside a double, and one carrying the explicit marker. - assert.deepEqual(findLiveCallers(` - await withJspmDouble({}, async () => { await pinAll(dir); }); - // live-cdn-ok: no bare imports, so it returns before the resolve. - const r = await pinAll(emptyDir); - `), []); -}); - -test('counterfactual: a regex literal carrying a quote does not blind the scan', () => { - // The subtlest failure this guard can have, and it went undetected until a - // review reproduced it. A pattern like /rel=["']modulepreload["']/ has quote - // characters in it; a masker without regex awareness reads the first one as - // a string opener and desyncs for the REST OF THE FILE, so every live call - // below that line silently vanishes from the scan. Eighteen test files here - // carry that shape, including the app-boot tests. - const afterARegex = [ - 'const re = /]+rel=["\']modulepreload["\']/g;', - 'const leak = await fetch("https://api.jspm.io/generate", { method: "POST" });', - ].join('\n'); - assert.deepEqual(findLiveCallers(afterARegex).map((h) => h.what), ['api.jspm.io'], - 'a live call after a quote-bearing regex must still be seen'); - - // The other half: a `/` that is division must NOT be read as a regex, or the - // mask desyncs the other way and starts swallowing real code. - const withDivision = [ - 'const half = total / 2;', - 'const other = count / 4;', - 'await fetch("https://api.jspm.io/generate");', - ].join('\n'); - assert.deepEqual(findLiveCallers(withDivision).map((h) => h.what), ['api.jspm.io']); + assert.equal(denied.length, DENIED_HOSTS.length, 'each denial is reported'); + assert.deepEqual(passedThrough, [], 'nothing reached the real fetch'); + + // Anything else is untouched, including a same-origin app request, which is + // what the app-boot tests spend their time doing. + const ok = await fetch('http://localhost:3000/'); + assert.equal(ok.status, 200); + assert.deepEqual(passedThrough, ['http://localhost:3000/']); }); -test('counterfactual: a host named near, but not inside, a fetch call is inert', () => { - // The scan reads the call's actual argument list, not a character window. A - // window crossed statement boundaries, so a local fetch followed by an - // assertion naming a jspm url read as a live call. - assert.deepEqual(findLiveCallers([ - 'const r = await fetch(localUrl);', - 'assert.equal(r.status, 200);', - 'assert.equal(map.dayjs, "https://ga.jspm.io/npm:dayjs@1.11.20/index.js");', - ].join('\n')), []); - - assert.deepEqual(findLiveCallers([ - 'await fetch(baseUrl);', - '// TODO: point this at the double instead of ga.jspm.io one day.', - ].join('\n')), []); +test('the deny recognises a URL object and a Request, not only a string', async () => { + // vendor.js passes strings, but a caller elsewhere may not, and a deny that + // only matched strings would be silently partial. + const fetch = denyLiveHosts(async () => new Response('real', { status: 200 })); + assert.equal((await fetch(new URL('https://api.jspm.io/generate'))).status, 503); + assert.equal((await fetch(new Request('https://ga.jspm.io/npm:x@1/i.js'))).status, 503); + assert.equal((await fetch(new URL('http://localhost:3000/'))).status, 200); }); -test('both runners drop live files unless the network is explicitly required', () => { - // The policy above is only worth anything because the runners enforce it, so - // assert the enforcement rather than trusting it. A refactor that renames - // the marker or drops the filter reds here. +test('both runners install the deny and skip live files, unless the network is required', () => { + // The policy is only worth anything because the runners enforce it, so assert + // the wiring rather than trusting it. A refactor that drops either half reds + // here. for (const runner of ['scripts/run-node-tests.js', 'scripts/run-bun-tests.js']) { const src = readFileSync(join(ROOT, runner), 'utf8'); assert.match(src, /WEBJS_REQUIRE_NETWORK/, `${runner} must honour the opt-in`); assert.match(src, /\.live\.test\./, `${runner} must filter on the live marker`); + assert.match(src, /deny-live-hosts/, `${runner} must install the third-party deny`); + assert.match(src, /const denyArgs = wantsNetwork/, `${runner} must lift the deny when the network is required`); } }); + +test('every allowlisted live caller is a *.live.test.* file that exists', () => { + for (const entry of LIVE_CALLERS) { + assert.ok(entry.file.includes(LIVE_MARKER), + `${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`); + assert.ok(files.some((f) => rel(f) === entry.file), + `${entry.file} is allowlisted but no such test file exists`); + assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`); + } +}); + +test('a *.live.test.* file exists for every allowlisted caller and no others', () => { + // The reverse direction. A live file added without an allowlist entry is a + // test that reaches a third party with nobody having written down why. + const onDisk = files.filter((f) => rel(f).includes(LIVE_MARKER)).map(rel).sort(); + assert.deepEqual(onDisk, LIVE_CALLERS.map((e) => e.file).sort()); +}); From 3312a36805fc4468b944b9f90cfada980ae82637 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 00:17:24 +0530 Subject: [PATCH 11/18] fix: put bun's preload after its test subcommand, and correct two doc claims The deny flag went before the subcommand, and `bun --preload X test ` stops treating `test` as bun's subcommand: it resolves it as the package.json script of that name, which here is the whole Node suite. Every matrix file then spawned that, hit the 120s per-file timeout, and failed. Measured before the fix: 0 pass, 23 genuine fail. After: 23 pass, 0 fail. The wiring guard did not catch it, because it only asserted the runner source mentions the fixture, which is true of the broken argv too. It asserts the order now, with the failure mode written down, since a flag-order mistake here surfaces as a timeout that looks nothing like its cause. Two doc claims are corrected. The framework-dev paragraph describing which required tests reach jspm predates the deny and said they still do; under the deny those calls get a 503 without leaving the process, and the point worth making is that they pass anyway because the resolve fails open. And both the fixture header and framework-dev claimed the deny has no blind spots while the next bullet described one: a spawned child starts with its own globalThis, so the claim is scoped to the test process now. --- framework-dev.md | 4 ++-- scripts/run-bun-tests.js | 5 ++++- test/fixtures/deny-live-hosts.mjs | 14 ++++++++++---- test/repo-health/live-cdn-callers.test.mjs | 10 ++++++++++ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/framework-dev.md b/framework-dev.md index 70e9fb03f..3f501b55c 100644 --- a/framework-dev.md +++ b/framework-dev.md @@ -115,7 +115,7 @@ Two things to keep in mind when touching this. The stub serves only the packages No required check may FAIL because a third party is down. The required `Unit + integration` job used to resolve vendors against the live jspm CDN, so a jspm outage redded pull requests that had nothing to do with vendoring; PR #1149, a five-file documentation change, is the one that finally made the case (it failed on the `#448` gitignore-healing test and passed on a re-run of the identical commit). -Be precise about what that does and does not say, because the weaker-sounding version is the true one. Required checks still REACH jspm: no in-repo app carries a pin file, so every test that cold-boots one (`test/preload-subset.test.mjs`, the `test/docs/*` boot tests, `test/integration/blog-http.test.mjs`, `packages/server/test/elision/differential-elision.test.js`) resolves its vendors live on the first request, transitively, through `resolveVendorImports`. What makes that acceptable is that the resolve fails OPEN: an unreachable CDN yields a partial importmap and a warning, never a throw, and none of those tests assert on a vendor entry. Measured with jspm forced to fail, each of them still passes in a few seconds. The rule is about what can turn a check red, not about counting packets. +Plenty of required tests still TRY. No in-repo app carries a pin file, so every test that cold-boots one (`test/preload-subset.test.mjs`, the `test/docs/*` boot tests, `test/integration/blog-http.test.mjs`, `packages/server/test/elision/differential-elision.test.js`) asks `resolveVendorImports` to resolve its vendors on the first request. Under the deny those calls get a 503 without leaving the process, and each test still passes in a few seconds, because the resolve fails OPEN: an unreachable CDN yields a partial importmap and a warning, never a throw, and none of them assert on a vendor entry. That is what makes denying safe rather than disruptive, and it is why the deny prints one line per refused url: the list is there if that ever stops being true. The rule is carried by the FILENAME, so the test runners can enforce it rather than leaving it to discipline. `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` both drop any `*.live.test.*` file unless `WEBJS_REQUIRE_NETWORK=1` is set. Everything else resolves against `test/fixtures/jspm-double.mjs`, an offline double that models jspm rather than merely answering it (a 5xx or 429 is transient and retries per package, a 4xx probes per install, and an unresolvable install fails the WHOLE batch, which is the premise `jspmGenerate`'s fallback ladder is built on). @@ -125,7 +125,7 @@ Four things to keep in mind when touching this. **A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. The runtime deny answers 503 for the same reason, since that is the shape those call sites classify as transient. -**The deny is at RUNTIME, and that was learned the hard way.** Both runners preload `test/fixtures/deny-live-hosts.mjs`, which answers 503 for jspm.io and registry.npmjs.org unless `WEBJS_REQUIRE_NETWORK` is set. It needs no parsing and has no blind spots, and it covers the transitive callers a source scan structurally cannot see: the app-boot tests reach jspm through `resolveVendorImports` with no `fetch(` anywhere in their own source. A test that depends on a third party now fails on EVERY run rather than only during an outage, which arrives the day it is written instead of months later. +**The deny is at RUNTIME, and that was learned the hard way.** Both runners preload `test/fixtures/deny-live-hosts.mjs`, which answers 503 for jspm.io and registry.npmjs.org unless `WEBJS_REQUIRE_NETWORK` is set. It needs no parsing, and within the test process it has no blind spots (a spawned child is the exception, below). It covers the transitive callers a source scan structurally cannot see: the app-boot tests reach jspm through `resolveVendorImports` with no `fetch(` anywhere in their own source. A test that depends on a third party now fails on EVERY run rather than only during an outage, which arrives the day it is written instead of months later. The first three attempts were a STATIC scan over test sources, and each went blind a different way: a file-level exemption, so one `withMockedFetch` anywhere excused every live call in the file; then no regex-literal awareness, so `/rel=["']modulepreload["']/` desynced the mask to end of file; then regex awareness that read the `/` in `` inside a nested `` html`...` `` template as a regex opener, swallowing the closing backtick. Each fix opened a new hole, because deciding whether a `/` starts a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup will keep being wrong. **Do not reintroduce it.** If the deny needs to be tighter, tighten the deny. diff --git a/scripts/run-bun-tests.js b/scripts/run-bun-tests.js index f19adf81d..7baa0b574 100644 --- a/scripts/run-bun-tests.js +++ b/scripts/run-bun-tests.js @@ -101,6 +101,9 @@ const LIVE_MARKER = '.live.test.'; const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK); // Same third-party deny the node runner installs, so a jspm outage cannot red // this job either (#1150). Bun ignores NODE_OPTIONS, hence the explicit flag. +// It goes AFTER the `test` subcommand: `bun --preload X test ` treats +// `test` as the package.json SCRIPT and runs the whole Node suite instead, +// which fails in a way that looks nothing like a flag-order mistake. const denyArgs = wantsNetwork ? [] : ['--preload', resolve(ROOT, 'test', 'fixtures', 'deny-live-hosts.mjs')]; @@ -137,7 +140,7 @@ for (const f of files) { console.log(`SKIP(node-only) ${rel(f)}`); continue; } - const r = spawnSync(BUN, [...denyArgs, 'test', f], { + const r = spawnSync(BUN, ['test', ...denyArgs, f], { cwd: ROOT, encoding: 'utf8', timeout: PER_FILE_TIMEOUT_MS, env: { ...process.env, FORCE_COLOR: '0' }, }); diff --git a/test/fixtures/deny-live-hosts.mjs b/test/fixtures/deny-live-hosts.mjs index 610409e83..5ac896030 100644 --- a/test/fixtures/deny-live-hosts.mjs +++ b/test/fixtures/deny-live-hosts.mjs @@ -29,10 +29,16 @@ * `resolveVendorImports`, with no `fetch(` and no vendor entry point anywhere * in their source. * - * Denying at runtime needs no parsing and has no blind spots. A test that - * depends on a third party now fails on EVERY run rather than only during an - * outage, which is a better signal than any scan could give, and it arrives - * the day the test is written instead of months later. + * Denying at runtime needs no parsing, and inside the test process it has no + * blind spots. A test that depends on a third party now fails on EVERY run + * rather than only during an outage, which is a better signal than any scan + * could give, and it arrives the day the test is written instead of months + * later. + * + * The one thing it does NOT cover is a SPAWNED child, which starts with its + * own `globalThis`. `test/vendor-cli/vendor-cli.test.mjs` runs the CLI in + * another process, so it passes its own preload and asserts a marker on every + * spawn. A new test that spawns a process and vendors needs the same. * * WHY A 503 RATHER THAN A THROW. Every fetch caller in * `packages/server/src/vendor.js` catches, so a throw is indistinguishable diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index e59fa74c0..adc78fab8 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -129,6 +129,16 @@ test('both runners install the deny and skip live files, unless the network is r assert.match(src, /deny-live-hosts/, `${runner} must install the third-party deny`); assert.match(src, /const denyArgs = wantsNetwork/, `${runner} must lift the deny when the network is required`); } + + // Flag ORDER, not just presence. `bun --preload X test ` stops + // treating `test` as the subcommand and runs the package.json script of that + // name instead, which here is the whole Node suite: every matrix file then + // spawns it, times out at 120s, and the job goes red having run zero Bun + // tests. A guard that only greps for the fixture path passes on exactly that + // argv, which is how it shipped once. + const bun = readFileSync(join(ROOT, 'scripts/run-bun-tests.js'), 'utf8'); + assert.match(bun, /spawnSync\(BUN, \['test', \.\.\.denyArgs/, + "the preload must come AFTER bun's `test` subcommand"); }); test('every allowlisted live caller is a *.live.test.* file that exists', () => { From 917970f64c539468fc2b05bf7dda6de0abfbeaad Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 10:30:52 +0530 Subject: [PATCH 12/18] docs: scope the last two claims the redesign left behind Two stale copies of claims corrected elsewhere in this branch. The guard test's header still said the deny "covers every caller", the same absolute the previous commit rewrote in the fixture and in framework-dev, and in the very file that commit edited. It is contradicted by the spawned-child bullet a few lines below in framework-dev and by the guard's own allowlist, whose second entry is a spawn-based caller. And the e2e preload comment still said Node and Bun each ignore the other's flag. Measured on bun 1.3.14 and node v26.1.0: node --preload is a hard bad option error, but bun --import loads the module as an alias. framework-dev's #1228 paragraph documents that comment and already carried the correction, so the two disagreed. Independent confirmation of the narrowed claim, from the review: a recording preload chained ahead of deny-live-hosts over the whole node suite, 3907 tests, logged zero external hosts outside the denied set. --- test/e2e/e2e.test.mjs | 12 ++++++++---- test/repo-health/live-cdn-callers.test.mjs | 9 ++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/test/e2e/e2e.test.mjs b/test/e2e/e2e.test.mjs index cb86e42ff..1ba141386 100644 --- a/test/e2e/e2e.test.mjs +++ b/test/e2e/e2e.test.mjs @@ -75,10 +75,14 @@ function blogRuntimeExec() { /** * Runtime flags that load `file` into the SERVER process before it boots, in - * whichever runtime `blogRuntimeExec` picked. Node spells this `--import`, Bun - * spells it `--preload`, and neither honours the other's flag, so a fixture - * wired through only one of them would silently do nothing on the Bun e2e job. - * Passed as argv rather than NODE_OPTIONS for the same reason (Bun ignores it). + * whichever runtime `blogRuntimeExec` picked. Passed as argv rather than + * NODE_OPTIONS because Bun ignores that variable outright, so a fixture wired + * through it would silently do nothing on the Bun e2e job. + * + * The two flags are not symmetric, so do not reason from the Node side: + * `node --preload` is a hard `bad option` error, while `bun --import` + * currently works as an alias. Selecting per runtime anyway is what keeps this + * from depending on Bun continuing to accept a Node spelling. * @param {string} file absolute path to an ES module * @returns {string[]} */ diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index adc78fab8..7eb9dee3e 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -10,9 +10,12 @@ * Two mechanisms enforce that, and this file asserts both. * * The RUNTIME DENY (`test/fixtures/deny-live-hosts.mjs`) is loaded by both test - * runners and answers 503 for jspm.io and registry.npmjs.org. That covers every - * caller, including the app-boot tests that reach jspm transitively through - * `resolveVendorImports` with no `fetch(` anywhere in their own source. + * runners and answers 503 for jspm.io and registry.npmjs.org. Inside the test + * process that covers every caller, including the app-boot tests that reach + * jspm transitively through `resolveVendorImports` with no `fetch(` anywhere in + * their own source. It does NOT reach a spawned child, which starts with its + * own `globalThis`; `test/vendor-cli/vendor-cli.test.mjs` passes its own + * preload and asserts a marker on every spawn for that reason. * * The FILENAME RULE keeps the genuinely-live tests out of a normal run: both * runners skip `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, which is the From fb26cac9e74f38e9d526fcd7026ae195e9c024e0 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 10:59:31 +0530 Subject: [PATCH 13/18] fix: stop the nightly crying wolf, and prove the deny is actually armed Three from the final whole-diff review. The worst was self-inflicted and would have shipped as a recurring 4am alarm. WEBJS_REQUIRE_NETWORK did double duty: it selected the live files AND promoted every upstream-trouble skip into a failure. The nightly always sets it, and the runners only select those files under it, so the transport-level skip that #1219 built was unreachable everywhere automated. A single jspm 503 or DNS blip would have redded the job and filed a bug issue, which is exactly the cry-wolf outcome the workflow's own header argues against. The two concerns are separate variables now: WEBJS_REQUIRE_NETWORK selects and lifts the deny, WEBJS_FAIL_ON_SKIP promotes, and the nightly sets only the first. A skip becomes a warning annotation instead, so a permanently skipping test is still visible without waking anyone for an outage. The live pin test had a wider version of the same trap. Its skip fired only on a non-zero exit, but a hiccup on the bundle GET leaves pin exiting 0 with the entry pinned and no hash, so the integrity assertion hard-failed on an outage the exit code had already forgiven. It skips there too now. withJspmDouble threw from a finally, which REPLACES an in-flight error. A refused request usually travels with the assertion it broke, so that discarded the message explaining what went wrong, in the case where it matters most. The restore stays in the finally; the report moved to the success path. And the guard file claimed to assert both enforcement mechanisms while never checking the deny was armed in a running process. Its other tests exercise a pure function and grep runner sources, so an inverted self-install would leave them all green while the required job went back to reaching jspm. It now spawns a preloaded child and asserts both branches of the switch. --- .github/workflows/vendor-cdn.yml | 36 ++++++++++++++++--- .../server/test/vendor/jspm-cdn.live.test.js | 29 +++++++++++---- test/fixtures/deny-live-hosts.mjs | 10 ++++++ test/fixtures/jspm-double.mjs | 21 +++++++---- test/repo-health/live-cdn-callers.test.mjs | 36 ++++++++++++++++++- test/vendor-cli/vendor-pin.live.test.mjs | 24 +++++++++---- 6 files changed, 130 insertions(+), 26 deletions(-) diff --git a/.github/workflows/vendor-cdn.yml b/.github/workflows/vendor-cdn.yml index 1096ca7bf..29cea6365 100644 --- a/.github/workflows/vendor-cdn.yml +++ b/.github/workflows/vendor-cdn.yml @@ -22,10 +22,18 @@ name: Vendor CDN contract (nightly) # outage; it would just be a red somebody is told to ignore, which is how a # real failure gets ignored too. # -# WEBJS_REQUIRE_NETWORK does double duty. It selects the live files, and it -# turns their upstream-trouble SKIP into a FAILURE. Without that second half a -# permanently skipping test is indistinguishable from a passing one, which is -# the exact way live coverage rots into decoration. +# WEBJS_REQUIRE_NETWORK selects the live files and lifts the test-run deny. It +# does NOT promote their upstream-trouble skip into a failure, and that +# separation is deliberate: it briefly did both, and since this job always sets +# it, the transport-level skip was unreachable wherever the tests actually run. +# A single jspm 503 or DNS blip at 04:20 UTC would then have redded the job and +# filed the issue below, which is precisely the cry-wolf failure the paragraph +# above argues against. +# +# A permanently skipping test still must not pass for a healthy one, so the run +# is scanned for skips and annotates a warning instead. That is visible in the +# run summary without waking anyone for an outage. `WEBJS_FAIL_ON_SKIP=1` +# promotes a skip to a failure when you want to force the question by hand. # # There is deliberately no `pull_request` trigger, so this can never become a # required check and can never block a merge. @@ -61,12 +69,30 @@ jobs: cache: npm - run: npm ci - name: Run the live CDN tests + id: live env: WEBJS_REQUIRE_NETWORK: '1' run: | + set -o pipefail node --test \ packages/server/test/vendor/jspm-cdn.live.test.js \ - test/vendor-cli/vendor-pin.live.test.mjs + test/vendor-cli/vendor-pin.live.test.mjs 2>&1 | tee live.log + + - name: Warn if a live check only skipped + # Runs even when the step above failed, so a partial skip is still + # reported. A skip means jspm could not answer, which is upstream's + # problem, not a regression; it is surfaced rather than escalated. + if: always() + run: | + set -euo pipefail + skipped=$(grep -c '^# SKIP\|^ℹ skipped' live.log 2>/dev/null || true) + if grep -q 'SKIP ' live.log 2>/dev/null; then + echo "::warning title=Live jspm check skipped::jspm.io could not answer at least one check. \ + Not a regression, but if this repeats for days the live coverage has stopped running. \ + Re-run with WEBJS_FAIL_ON_SKIP=1 to force it to fail instead." + grep 'SKIP ' live.log || true + fi + echo "skip markers: ${skipped}" - name: Report a failure on the tracking issue if: failure() diff --git a/packages/server/test/vendor/jspm-cdn.live.test.js b/packages/server/test/vendor/jspm-cdn.live.test.js index 4d4775741..cb98f44a7 100644 --- a/packages/server/test/vendor/jspm-cdn.live.test.js +++ b/packages/server/test/vendor/jspm-cdn.live.test.js @@ -13,8 +13,9 @@ * whole job is to talk to jspm, and a double can only ever return what this * repo already believes about the API. So these two assertions stay real, and * `.github/workflows/vendor-cdn.yml` runs them nightly with - * `WEBJS_REQUIRE_NETWORK=1`, which ALSO turns a skip into a failure. Without - * that, a permanently skipping test is indistinguishable from a passing one. + * `WEBJS_REQUIRE_NETWORK=1`, and surfaces any skip as a warning annotation, so + * a permanently skipping test is visible rather than indistinguishable from a + * passing one. * * Upstream trouble skips rather than reds, judged at the transport: a throw, a * 5xx, or a 429 is jspm having a bad moment. A 4xx does not skip, because by @@ -22,6 +23,16 @@ * upstream is demonstrably healthy and a 4xx means OUR request is malformed. * That distinction is #1219's, and it is the reason this file can be run * nightly without becoming a source of false alarms. + * + * That only holds if the skip is REACHABLE where the file runs. It briefly was + * not: `WEBJS_REQUIRE_NETWORK` both selected these files and promoted every + * skip to a failure, and the nightly always sets it, so the transport + * distinction had no effect anywhere automated and a single 503 at 04:20 UTC + * would have filed a bug issue. The two concerns are separate variables now. + * `WEBJS_REQUIRE_NETWORK` selects the files and lifts the deny; + * `WEBJS_FAIL_ON_SKIP` promotes a skip, and the nightly does NOT set it. The + * nightly instead reports skips as a warning annotation, so a permanently + * skipping test is visible without waking anyone for an outage. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -37,10 +48,14 @@ const GENERATE_BODY = (install) => JSON.stringify({ * Build a loud skip for one fixture. * * Loud on purpose: a silent skip is how a real regression hides, so the reason - * and the fixture are always named. Under `WEBJS_REQUIRE_NETWORK` the skip - * becomes a FAILURE instead, which is what makes the nightly job able to tell - * "jspm changed under us" from "everything is fine". A normal run is - * unaffected, since the runners exclude this file entirely. + * and the fixture are always named, and the nightly turns any skip into a + * warning annotation. + * + * `WEBJS_FAIL_ON_SKIP` promotes it to a failure. Deliberately NOT the same + * variable that selects this file, and deliberately not set by the nightly: + * upstream being down is not a regression, and a job that reds on it is a job + * whose reds get ignored. Set it by hand when you want to know that the check + * genuinely ran. * * @param {import('node:test').TestContext} t * @param {string} fixture @@ -48,7 +63,7 @@ const GENERATE_BODY = (install) => JSON.stringify({ function skipper(t, fixture) { return (reason) => { const first = String(reason).split('\n')[0]; - if (process.env.WEBJS_REQUIRE_NETWORK) { + if (process.env.WEBJS_FAIL_ON_SKIP) { assert.fail(`live jspm check could not run (${fixture}): ${first}`); } console.warn(`[jspm-cdn.live] SKIP ${fixture} (${first})`); diff --git a/test/fixtures/deny-live-hosts.mjs b/test/fixtures/deny-live-hosts.mjs index 5ac896030..71564833a 100644 --- a/test/fixtures/deny-live-hosts.mjs +++ b/test/fixtures/deny-live-hosts.mjs @@ -77,6 +77,15 @@ export function denyLiveHosts(realFetch, onDenied) { }; } +/** + * Set on `globalThis` when the self-install below has run, so a guard can + * prove the preload actually took effect in a process rather than only that a + * runner's source mentions it. An inverted or dropped self-install is + * otherwise invisible: every unit test of `denyLiveHosts` keeps passing while + * the required job goes back to reaching jspm. + */ +export const DENY_INSTALLED_FLAG = '__webjsDenyLiveHostsInstalled'; + if (!process.env.WEBJS_REQUIRE_NETWORK) { /** @type {Set} */ const seen = new Set(); @@ -92,4 +101,5 @@ if (!process.env.WEBJS_REQUIRE_NETWORK) { seen.add(key); process.stderr.write(`[deny-live-hosts] refused ${key}\n`); })); + /** @type {any} */ (globalThis)[DENY_INSTALLED_FLAG] = true; } diff --git a/test/fixtures/jspm-double.mjs b/test/fixtures/jspm-double.mjs index 3321aab29..7315ac69a 100644 --- a/test/fixtures/jspm-double.mjs +++ b/test/fixtures/jspm-double.mjs @@ -230,16 +230,23 @@ export async function withJspmDouble(opts, body) { const original = globalThis.fetch; globalThis.fetch = /** @type {any} */ (double); clearVendorCache(); + let result; try { - return await body(double); + result = await body(double); } finally { globalThis.fetch = original; clearVendorCache(); - if (double.unexpected.length) { - throw new Error( - `the jspm double was asked for ${double.unexpected.length} request(s) it does not serve:\n ` + - `${double.unexpected.join('\n ')}`, - ); - } } + // Deliberately OUTSIDE the finally. Throwing from a finally REPLACES an + // in-flight error, and a refused request usually travels with the assertion + // it broke, so raising it there would discard the message that explains + // what actually went wrong. Restoring is what the finally is for; reporting + // happens only on the success path, where nothing is being displaced. + if (double.unexpected.length) { + throw new Error( + `the jspm double was asked for ${double.unexpected.length} request(s) it does not serve:\n ` + + `${double.unexpected.join('\n ')}`, + ); + } + return result; } diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index 7eb9dee3e..64bab2d98 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -31,7 +31,10 @@ import { readdirSync, readFileSync } from 'node:fs'; import { join, resolve, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { denyLiveHosts, DENIED_HOSTS } from '../fixtures/deny-live-hosts.mjs'; +import { spawnSync } from 'node:child_process'; +import { pathToFileURL } from 'node:url'; + +import { denyLiveHosts, DENIED_HOSTS, DENY_INSTALLED_FLAG } from '../fixtures/deny-live-hosts.mjs'; const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); @@ -144,6 +147,37 @@ test('both runners install the deny and skip live files, unless the network is r "the preload must come AFTER bun's `test` subcommand"); }); +test('the preload actually arms the deny in a real process', () => { + // Everything else here checks `denyLiveHosts` as a pure function or greps a + // runner's source, and neither notices if the self-install at the bottom of + // the fixture is inverted or deleted: the unit tests stay green while the + // required job goes back to reaching jspm. That is the same class of defect + // three earlier review rounds found in this guard, so prove the install by + // running it. + // + // Spawned rather than asserted on this process, so the check does not depend + // on how THIS file was launched, and so both branches of the env switch can + // be exercised in one test. + const fixture = pathToFileURL(join(ROOT, 'test/fixtures/deny-live-hosts.mjs')).href; + const probe = 'const r = await fetch("https://api.jspm.io/generate", { method: "POST" });' + + `console.log(JSON.stringify({ status: r.status, armed: Boolean(globalThis[${JSON.stringify(DENY_INSTALLED_FLAG)}]) }));`; + + const run = (env) => { + const r = spawnSync(process.execPath, ['--import', fixture, '--input-type=module', '-e', probe], + { encoding: 'utf8', env: { ...process.env, ...env }, timeout: 30_000 }); + return JSON.parse((r.stdout || '{}').trim() || '{}'); + }; + + const denied = run({ WEBJS_REQUIRE_NETWORK: '' }); + assert.equal(denied.armed, true, 'the preload must install itself by default'); + assert.equal(denied.status, 503, 'a jspm call in a preloaded process must be denied, not sent'); + + // The opt-out has to actually opt out, or the nightly could never reach the + // real CDN. Asserting the flag rather than a live status keeps this offline. + const allowed = run({ WEBJS_REQUIRE_NETWORK: '1' }); + assert.notEqual(allowed.armed, true, 'WEBJS_REQUIRE_NETWORK must lift the deny'); +}); + test('every allowlisted live caller is a *.live.test.* file that exists', () => { for (const entry of LIVE_CALLERS) { assert.ok(entry.file.includes(LIVE_MARKER), diff --git a/test/vendor-cli/vendor-pin.live.test.mjs b/test/vendor-cli/vendor-pin.live.test.mjs index b5411cdc4..54b8e57fe 100644 --- a/test/vendor-cli/vendor-pin.live.test.mjs +++ b/test/vendor-cli/vendor-pin.live.test.mjs @@ -54,16 +54,19 @@ test('pin resolves picocolors against the real CDN and hashes the bytes', async `import pico from 'picocolors';\nexport default () => pico.green('ok');`); const { code, stdout, stderr } = await runCli(['vendor', 'pin'], dir); - if (code !== 0) { - // A failed pin here is upstream trouble far more often than a regression, - // and the CLI already names the reason. Under WEBJS_REQUIRE_NETWORK the - // nightly wants to know, so fail loudly there instead of skipping. - const why = `exit ${code}: ${(stderr || stdout).split('\n').filter(Boolean).slice(-1)[0] || 'no output'}`; - if (process.env.WEBJS_REQUIRE_NETWORK) { + // Upstream trouble is not a regression. `WEBJS_FAIL_ON_SKIP` promotes it, + // and is deliberately NOT the variable that selects this file nor one the + // nightly sets, so a jspm outage does not red a scheduled run. + const skip = (why) => { + if (process.env.WEBJS_FAIL_ON_SKIP) { assert.fail(`live \`webjs vendor pin\` could not run (${why})`); } console.warn(`[vendor-pin.live] SKIP live pin (${why})`); t.skip('jspm.io was not in a state that can answer a pin'); + }; + + if (code !== 0) { + skip(`exit ${code}: ${(stderr || stdout).split('\n').filter(Boolean).slice(-1)[0] || 'no output'}`); return; } @@ -74,6 +77,15 @@ test('pin resolves picocolors against the real CDN and hashes the bytes', async // The offline double mints this tail, so its absence is what proves this // run really went to the network rather than picking up a stray preload. assert.doesNotMatch(url, /\/double\.js$/, 'this test must NOT be running against the double'); + + // A hiccup on the BUNDLE GET is the wider version of the same trap: pin + // exits 0 with the entry pinned and no hash, and the CLI says so, so + // asserting the hash outright would hard-fail on an outage the exit code + // already forgave. + if (!parsed.integrity || !parsed.integrity[url]) { + skip('jspm.io resolved the package but would not serve its bundle to hash'); + return; + } assert.match(parsed.integrity[url], /^sha384-/, 'the bundle behind the resolved url must have been fetched and hashed'); } finally { From af9df78c23223c02362221061e6fad14cf402e44 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 11:07:10 +0530 Subject: [PATCH 14/18] docs: drop a marker prefix that outlived its checker The eleven live-cdn-ok comments were written for the static scanner, which reads them as a per-site exemption. That scanner is gone, so nothing reads them and the prefix implies a mechanism that no longer exists. The reasons are accurate and worth keeping, so only the prefix goes. --- packages/server/test/vendor/vendor.test.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index 334c1dd55..86d5d763d 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -1010,7 +1010,7 @@ test('pinAll: returns noBareImports without writing pin file when no bare import await writeFile(join(dir, 'package.json'), '{"name":"tmp","version":"0.0.0"}'); await writeFile(join(dir, 'app', 'page.ts'), `export default () => 'no bare imports here';`); try { - // live-cdn-ok: no bare imports, so pinAll returns before the resolve. + // Offline: no bare imports, so pinAll returns before the resolve. const result = await pinAll(dir); assert.ok(result.noBareImports, 'noBareImports must be true'); assert.equal(result.failed, undefined, 'failed must be absent (not a failure, just nothing to do)'); @@ -1034,7 +1034,7 @@ test('pinAll: reports found-but-uninstalled specifiers instead of noBareImports 'app/page.ts': `import * as THREE from 'three';\nimport { OrbitControls } from 'three/addons/controls/OrbitControls.js';`, }); try { - // live-cdn-ok: every specifier is dropped by the version gate, so installs is empty. + // Offline: every specifier is dropped by the version gate, so installs is empty. const result = await pinAll(dir); assert.equal(result.noBareImports, undefined, 'must NOT claim there were no bare imports'); assert.ok(Array.isArray(result.droppedUnresolvable), 'droppedUnresolvable must be an array'); @@ -1760,7 +1760,7 @@ test('pinAll: rejects unknown provider with a clear error', async () => { await writeFile(join(dir, 'package.json'), '{"name":"tmp"}'); try { await assert.rejects( - // live-cdn-ok: the provider is rejected before any call is dialled. + // Offline: the provider is rejected before any call is dialled. () => pinAll(dir, { from: 'not-a-real-cdn' }), /unknown provider 'not-a-real-cdn'/, ); @@ -1818,7 +1818,7 @@ test('auditPinned: no pin file returns zero-checked', async () => { const dir = join(tmpdir(), `webjs-audit-empty-${Date.now()}`); await mkdir(dir, { recursive: true }); try { - // live-cdn-ok: no pin file, so it short-circuits before the registry call. + // Offline: no pin file, so it short-circuits before the registry call. const { vulnerable, totalChecked } = await auditPinned(dir); assert.equal(totalChecked, 0); assert.deepEqual(vulnerable, []); @@ -1831,7 +1831,7 @@ test('findOutdated: no pin file returns []', async () => { const dir = join(tmpdir(), `webjs-outdated-empty-${Date.now()}`); await mkdir(dir, { recursive: true }); try { - // live-cdn-ok: no pin file, so it short-circuits before the registry call. + // Offline: no pin file, so it short-circuits before the registry call. assert.deepEqual(await findOutdated(dir), []); } finally { await rm(dir, { recursive: true, force: true }); @@ -1843,7 +1843,7 @@ test('updatePinned: rejects unknown provider', async () => { await mkdir(dir, { recursive: true }); try { await assert.rejects( - // live-cdn-ok: the provider is rejected before any call is dialled. + // Offline: the provider is rejected before any call is dialled. () => updatePinned(dir, { from: 'not-real' }), /unknown provider/, ); @@ -1859,7 +1859,7 @@ test('updatePinned: no outdated returns noOutdated:true without writing', async const dir = join(tmpdir(), `webjs-update-clean-${Date.now()}`); await mkdir(dir, { recursive: true }); try { - // live-cdn-ok: an empty pin file short-circuits before the registry call. + // Offline: an empty pin file short-circuits before the registry call. const result = await updatePinned(dir); assert.ok(result.noOutdated); assert.deepEqual(result.updated, []); @@ -1939,7 +1939,7 @@ test('auditPinned: surfaces network failure as errored:true', async () => { const origFetch = globalThis.fetch; globalThis.fetch = async () => { throw new Error('simulated network failure'); }; try { - // live-cdn-ok: the test installs its own throwing fetch for its whole duration. + // Offline: the test installs its own throwing fetch for its whole duration. const result = await auditPinned(dir); assert.equal(result.errored, true); assert.deepEqual(result.vulnerable, []); @@ -1972,7 +1972,7 @@ test('pinAll: respects existing pin file provider when --from is not passed', as // without writing. The interesting assertion: it didn't throw // and pinAll read the provider for whatever it would have done. // Verify by checking pin file's provider field unchanged. - // live-cdn-ok: the app has no bare imports, so pinAll returns before the resolve. + // Offline: the app has no bare imports, so pinAll returns before the resolve. const result = await pinAll(dir); assert.ok(result.noBareImports); const file = await readPinFile(dir); @@ -2069,7 +2069,7 @@ test('updatePinned: only counts a package as updated when at least one spec reso return /** @type any */ ({ ok: false, status: 404, json: async () => ({}) }); }; try { - // live-cdn-ok: the test installs its own fetch for its whole duration. + // Offline: the test installs its own fetch for its whole duration. const result = await updatePinned(dir); assert.deepEqual(result.updated, [], 'no spec resolved, so updated[] must be empty even though findOutdated saw dayjs as outdated'); @@ -2119,7 +2119,7 @@ test('findOutdated: returns an Array, not undefined (ASI regression guard)', asy // No pin file → grouped is empty → no fetches → return empty array. // The interesting assertion is that the return value is an // ARRAY (.length accessible), not undefined. - // live-cdn-ok: the test installs its own fetch for its whole duration. + // Offline: the test installs its own fetch for its whole duration. const result = await findOutdated(dir); assert.ok(Array.isArray(result), 'findOutdated must always return an Array'); assert.equal(result.length, 0); From 3ef9e59ac6d897ef635f10aa4550a40fb7d6becc Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 11:10:18 +0530 Subject: [PATCH 15/18] fix: stop the guard itself reaching jspm, and count skips from our own markers Six from the fix-check, one of them badly wrong. The armed-in-process guard I added last commit ran its probe twice, and the second run lifted the deny. The probe fetches, so that was a real POST to api.jspm.io on every npm test, from the one file whose entire job is to stop a required check reaching a third party, and invisible to everything else because the static scan that would have caught it was deleted earlier in this branch. The two branches run different probes now: the first fetches, because the point is that the deny answers it in-process; the second only reads the install flag. Verified with the network namespace cut. The same probe swallowed every child failure into `{}`, which satisfies the `notEqual(armed, true)` assertion, so that half passed unconditionally whenever the probe had not run at all. It asserts the exit status and an explicit ok sentinel now. It also used `--input-type`, which is Node-only, while spawning process.execPath, which is bun under the matrix; the probe is a temp file and the preload flag is runtime-selected, so it runs on both. The nightly's skip counter matched neither thing the run emits. Rather than guess again at which reporter node --test picks, which depends on the version and on whether stdout is a TTY, it counts the `] SKIP ` markers the two live files print themselves. Measured: 3 on a skipping run, 0 on a passing one. And three more copies of the promotion claim the previous commit invalidated, in framework-dev, the node runner, and the live pin test's own header. --- .github/workflows/vendor-cdn.yml | 14 ++++-- framework-dev.md | 2 +- scripts/run-node-tests.js | 3 +- test/repo-health/live-cdn-callers.test.mjs | 55 +++++++++++++++++----- test/vendor-cli/vendor-pin.live.test.mjs | 4 +- 5 files changed, 59 insertions(+), 19 deletions(-) diff --git a/.github/workflows/vendor-cdn.yml b/.github/workflows/vendor-cdn.yml index 29cea6365..22b390076 100644 --- a/.github/workflows/vendor-cdn.yml +++ b/.github/workflows/vendor-cdn.yml @@ -85,12 +85,18 @@ jobs: if: always() run: | set -euo pipefail - skipped=$(grep -c '^# SKIP\|^ℹ skipped' live.log 2>/dev/null || true) - if grep -q 'SKIP ' live.log 2>/dev/null; then - echo "::warning title=Live jspm check skipped::jspm.io could not answer at least one check. \ + # Counted from the markers the tests print THEMSELVES, not from the + # reporter. Which reporter `node --test` picks depends on the Node + # version and on whether stdout is a TTY, and guessing wrong here is + # silent: the count reads 0 in exactly the runs that skipped. Both + # live files print `[] SKIP ` from their own skip + # helper, which is ours and does not move. + skipped=$(grep -c '] SKIP ' live.log || true) + if [ "${skipped}" != "0" ]; then + echo "::warning title=Live jspm check skipped::jspm.io could not answer ${skipped} check(s). \ Not a regression, but if this repeats for days the live coverage has stopped running. \ Re-run with WEBJS_FAIL_ON_SKIP=1 to force it to fail instead." - grep 'SKIP ' live.log || true + grep '] SKIP ' live.log || true fi echo "skip markers: ${skipped}" diff --git a/framework-dev.md b/framework-dev.md index 3f501b55c..8dac55520 100644 --- a/framework-dev.md +++ b/framework-dev.md @@ -131,7 +131,7 @@ The first three attempts were a STATIC scan over test sources, and each went bli **A spawned child does not inherit the deny.** `test/vendor-cli/vendor-cli.test.mjs` runs the CLI in another process, so it passes its own preload and asserts a `[jspm-double] armed` marker on every spawn, which reds all ten of its tests if the flag is dropped. A new test that spawns a process and vendors needs the same treatment. -**The nightly is what stops a permanent skip from hiding.** `.github/workflows/vendor-cdn.yml` runs the live files with `WEBJS_REQUIRE_NETWORK=1`, which both selects them AND promotes their upstream-trouble skip into a failure. Without that second half a permanently skipping test is indistinguishable from a passing one. A failure opens or comments on one fixed-title tracking issue, since GitHub notifies only the workflow file's last committer about a failed scheduled run. +**The nightly is what stops a permanent skip from hiding.** `.github/workflows/vendor-cdn.yml` runs the live files with `WEBJS_REQUIRE_NETWORK=1`, which selects them and lifts the deny. It does NOT promote their upstream-trouble skip into a failure: a jspm outage is not a regression, and a job that reds on one is a job whose reds get ignored. `WEBJS_FAIL_ON_SKIP=1` promotes, by hand, and the nightly does not set it; a skip surfaces as a warning annotation instead. A genuine failure opens or comments on one fixed-title tracking issue, since GitHub notifies only the workflow file's last committer about a failed scheduled run. **Do not add a `pull_request` trigger to that workflow.** A live check on a PR is a live check whatever job it sits in; making it non-required would just produce a red somebody is told to ignore, which is how a real failure gets ignored too. diff --git a/scripts/run-node-tests.js b/scripts/run-node-tests.js index 394446430..fa3add95b 100644 --- a/scripts/run-node-tests.js +++ b/scripts/run-node-tests.js @@ -61,7 +61,8 @@ const e2eSeg = `${SEP}e2e${SEP}`; // opt-in (#1150). This job is REQUIRED, so a jspm or npm-registry outage must // not be able to red it; a documentation-only PR was blocked that way on // #1149. The nightly `vendor-cdn` workflow sets WEBJS_REQUIRE_NETWORK to run -// them for real, where a skip is promoted to a failure. +// them for real; a skip there is a warning, not a failure, since an outage is +// not a regression (WEBJS_FAIL_ON_SKIP promotes it when you want that). const LIVE_MARKER = '.live.test.'; const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK); diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index 64bab2d98..dd2202ce3 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -32,6 +32,8 @@ import { join, resolve, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; import { spawnSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { pathToFileURL } from 'node:url'; import { denyLiveHosts, DENIED_HOSTS, DENY_INSTALLED_FLAG } from '../fixtures/deny-live-hosts.mjs'; @@ -158,24 +160,53 @@ test('the preload actually arms the deny in a real process', () => { // Spawned rather than asserted on this process, so the check does not depend // on how THIS file was launched, and so both branches of the env switch can // be exercised in one test. - const fixture = pathToFileURL(join(ROOT, 'test/fixtures/deny-live-hosts.mjs')).href; - const probe = 'const r = await fetch("https://api.jspm.io/generate", { method: "POST" });' - + `console.log(JSON.stringify({ status: r.status, armed: Boolean(globalThis[${JSON.stringify(DENY_INSTALLED_FLAG)}]) }));`; - - const run = (env) => { - const r = spawnSync(process.execPath, ['--import', fixture, '--input-type=module', '-e', probe], + // + // The probe goes in a temp FILE rather than `-e`, because `--input-type` is + // Node-only and `process.execPath` is the Bun binary when this file runs + // under `bun test`. Same reason the preload flag is runtime-selected. + // + // CRUCIALLY the two branches run DIFFERENT probes. The first fetches, + // because the whole point is that the deny answers it without a packet + // leaving the process. The second must NOT fetch: with the deny lifted the + // call would go to the real CDN, which would make this file, whose entire + // job is to stop a required check reaching a third party, itself a live + // caller on every `npm test`. It reads the install flag instead. + const fixture = join(ROOT, 'test/fixtures/deny-live-hosts.mjs'); + const flag = JSON.stringify(DENY_INSTALLED_FLAG); + const armed = `armed: Boolean(globalThis[${flag}])`; + + const run = (probe, env) => { + const file = join(mkdtempSync(join(tmpdir(), 'webjs-deny-probe-')), 'probe.mjs'); + writeFileSync(file, probe); + const preload = process.versions.bun + ? ['--preload', fixture] + : ['--import', pathToFileURL(fixture).href]; + const r = spawnSync(process.execPath, [...preload, file], { encoding: 'utf8', env: { ...process.env, ...env }, timeout: 30_000 }); - return JSON.parse((r.stdout || '{}').trim() || '{}'); + // A spawn that failed must NOT collapse into `{}`. An empty object + // satisfies a `notEqual(..., true)` assertion, so swallowing the error + // would make half this test pass unconditionally, including on a machine + // where the probe could never run at all. + assert.equal(r.status, 0, `probe exited ${r.status}: ${r.stderr || r.error || 'no output'}`); + const out = JSON.parse((r.stdout || '').trim()); + assert.equal(out.ok, true, 'the probe must report that it ran to completion'); + return out; }; - const denied = run({ WEBJS_REQUIRE_NETWORK: '' }); + const denied = run( + 'const r = await fetch("https://api.jspm.io/generate", { method: "POST" });' + + `console.log(JSON.stringify({ ok: true, status: r.status, ${armed} }));`, + { WEBJS_REQUIRE_NETWORK: '' }, + ); assert.equal(denied.armed, true, 'the preload must install itself by default'); assert.equal(denied.status, 503, 'a jspm call in a preloaded process must be denied, not sent'); - // The opt-out has to actually opt out, or the nightly could never reach the - // real CDN. Asserting the flag rather than a live status keeps this offline. - const allowed = run({ WEBJS_REQUIRE_NETWORK: '1' }); - assert.notEqual(allowed.armed, true, 'WEBJS_REQUIRE_NETWORK must lift the deny'); + // No fetch here, deliberately. See above. + const allowed = run( + `console.log(JSON.stringify({ ok: true, ${armed} }));`, + { WEBJS_REQUIRE_NETWORK: '1' }, + ); + assert.equal(allowed.armed, false, 'WEBJS_REQUIRE_NETWORK must lift the deny'); }); test('every allowlisted live caller is a *.live.test.* file that exists', () => { diff --git a/test/vendor-cli/vendor-pin.live.test.mjs b/test/vendor-cli/vendor-pin.live.test.mjs index 54b8e57fe..b57e9aa90 100644 --- a/test/vendor-cli/vendor-pin.live.test.mjs +++ b/test/vendor-cli/vendor-pin.live.test.mjs @@ -8,7 +8,9 @@ * * Both test runners skip `*.live.test.*` unless `WEBJS_REQUIRE_NETWORK=1`, so * this never runs in a required check. `.github/workflows/vendor-cdn.yml` runs - * it nightly with that variable set, where a skip is promoted to a failure. + * it nightly with that variable set; a skip there is a warning rather than a + * failure, since an outage is not a regression. `WEBJS_FAIL_ON_SKIP=1` + * promotes it when you want to force the question. * * It asserts only what a live resolve is uniquely able to prove: that jspm * answers with a url of the shape the pin file expects, and that the bytes From 27724372de27655b38bcf187af9aeaba2ba788b3 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 11:20:50 +0530 Subject: [PATCH 16/18] fix: do not report a jspm skip for runs where the tests never ran Two from the second fix-check. Dropping `2>/dev/null` from the skip counter turned a missing live.log into an empty string rather than 0, and an inequality against "0" is then true. The step is `if: always()`, so it announced a jspm skip precisely in the runs that never produced a log: a failed checkout, a failed npm ci, a cancellation. It now returns early when the file is absent and defaults the count otherwise. Verified across all three cases: missing file gives no warning, a skipping run gives 3, a passing run gives 0. And the reason committed with the temp-file probe was wrong. Bun ignores flags it does not recognise, so the Node-only --input-type was silently dropped and the old -e form ran fine there; nothing was broken. Moving to a file is still right, because an argv that means the same thing on both runtimes beats one that happens to be inert on one of them, and that is the honest reason. It is the same reason-from-the-Node-side mistake framework-dev warns about, made in the commit that cites it. --- .github/workflows/vendor-cdn.yml | 10 ++++++++++ test/repo-health/live-cdn-callers.test.mjs | 11 ++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/.github/workflows/vendor-cdn.yml b/.github/workflows/vendor-cdn.yml index 22b390076..a27ff0dfb 100644 --- a/.github/workflows/vendor-cdn.yml +++ b/.github/workflows/vendor-cdn.yml @@ -91,7 +91,17 @@ jobs: # silent: the count reads 0 in exactly the runs that skipped. Both # live files print `[] SKIP ` from their own skip # helper, which is ours and does not move. + # `grep -c` on a MISSING file prints nothing, so a bare command + # substitution yields the empty string, not 0, and an inequality + # against "0" is then true. The step is `if: always()`, so that + # reported a jspm skip for runs where the live tests never ran at all + # (a failed checkout, npm ci, or a cancellation). Default explicitly. + if [ ! -f live.log ]; then + echo 'no live.log; the test step did not produce output' + exit 0 + fi skipped=$(grep -c '] SKIP ' live.log || true) + skipped=${skipped:-0} if [ "${skipped}" != "0" ]; then echo "::warning title=Live jspm check skipped::jspm.io could not answer ${skipped} check(s). \ Not a regression, but if this repeats for days the live coverage has stopped running. \ diff --git a/test/repo-health/live-cdn-callers.test.mjs b/test/repo-health/live-cdn-callers.test.mjs index dd2202ce3..4e9274e7f 100644 --- a/test/repo-health/live-cdn-callers.test.mjs +++ b/test/repo-health/live-cdn-callers.test.mjs @@ -161,9 +161,14 @@ test('the preload actually arms the deny in a real process', () => { // on how THIS file was launched, and so both branches of the env switch can // be exercised in one test. // - // The probe goes in a temp FILE rather than `-e`, because `--input-type` is - // Node-only and `process.execPath` is the Bun binary when this file runs - // under `bun test`. Same reason the preload flag is runtime-selected. + // The probe goes in a temp FILE rather than `-e` so the argv means the same + // thing on both runtimes, since `process.execPath` is the Bun binary when + // this file runs under `bun test`. Not because the `-e` form was broken + // there: Bun ignores flags it does not recognise, so the Node-only + // `--input-type=module` was silently dropped and the old probe ran fine. A + // flag that is inert today is a poor thing to depend on, which is the same + // reason the preload flag below is chosen per runtime rather than relying on + // `bun --import` continuing to alias the Node spelling. // // CRUCIALLY the two branches run DIFFERENT probes. The first fetches, // because the whole point is that the deny answers it without a packet From 58f918cdb9e2066b756b6d0fc50ea829029e9152 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 11:38:08 +0530 Subject: [PATCH 17/18] fix: do not report a jspm skip for runs where the tests never ran Two from the second fix-check. Dropping `2>/dev/null` from the skip counter turned a missing live.log into an empty string rather than 0, and an inequality against "0" is then true. The step is `if: always()`, so it announced a jspm skip precisely in the runs that never produced a log: a failed checkout, a failed npm ci, a cancellation. It now returns early when the file is absent and defaults the count otherwise. Verified across all three cases: missing file gives no warning, a skipping run gives 3, a passing run gives 0. And the reason committed with the temp-file probe was wrong. Bun ignores flags it does not recognise, so the Node-only --input-type was silently dropped and the old -e form ran fine there; nothing was broken. Moving to a file is still right, because an argv that means the same thing on both runtimes beats one that happens to be inert on one of them, and that is the honest reason. It is the same reason-from-the-Node-side mistake framework-dev warns about, made in the commit that cites it. --- packages/server/src/vendor.js | 54 ++++++++++++++-------- packages/server/test/vendor/vendor.test.js | 20 ++++++++ 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/packages/server/src/vendor.js b/packages/server/src/vendor.js index 15532c2e1..b33928c5b 100644 --- a/packages/server/src/vendor.js +++ b/packages/server/src/vendor.js @@ -338,13 +338,28 @@ let lastLiveResolveFailed = false; const JSPM_GENERATE_ENDPOINT = 'https://api.jspm.io/generate'; const JSPM_GENERATE_TIMEOUT_MS = 10_000; -// Bounds a single bundle GET: written to disk by `webjs vendor pin -// --download` (`downloadBundle`), hashed in place by default-mode pin -// (`fetchIntegrity`), or hashed during the warmup live-integrity pass -// (`fetchLiveIntegrity`). Declared here with the other outbound timeouts -// rather than beside one of its three callers. +// Bounds a single bundle GET during the SERVER's warmup live-integrity pass +// (`fetchLiveIntegrity`). Short on purpose: that pass gates readiness, so a +// stalled CDN must not hold the first request, and it is additionally capped +// by INTEGRITY_TOTAL_BUDGET_MS across all URLs. const INTEGRITY_FETCH_TIMEOUT_MS = 10_000; +// Bounds a single bundle GET made by the pin command, which either writes the +// bytes to disk (`downloadBundle`) or fetches them to hash +// (`fetchIntegrity`). Deliberately six times the warmup budget, because the +// two are not the same situation: a pin is a one-shot command a person ran and +// is waiting on, with a whole multi-megabyte package to transfer, while the +// warmup is a server holding a request. Ten seconds is generous for the +// latter and tight for the former on a slow link. +// +// 60s matches what importmap-rails effectively allows. It sets no timeout at +// all, but Ruby's Net::HTTP defaults open_timeout and read_timeout to 60s, so +// a Rails pin is bounded at a minute without asking. JavaScript's fetch() has +// no default whatsoever, which is why this has to be explicit: without it a +// CDN that accepts the connection and then stalls hangs the pin forever, with +// no ambient deadline on a CLI run to cut it short. +const PIN_BUNDLE_TIMEOUT_MS = 60_000; + /** * Provider names accepted by `webjs vendor pin --from `. * Default `jspm` resolves to jspm.io. Same set Rails's importmap-rails @@ -1083,11 +1098,10 @@ async function writePinFile(appDir, imports, integrity, provider) { * success or null on failure. The integrity hash is computed from the * downloaded bytes so it's always consistent with what's on disk. * - * Bounded by the same timeout as every other outbound call here. - * `pinAll(dir, { download: true })` runs this once per resolved URL on - * a CLI run with no ambient deadline, so a CDN that accepts the - * connection and then stalls would otherwise hang the pin with nothing - * to interrupt it (#1150). + * Bounded by PIN_BUNDLE_TIMEOUT_MS. `pinAll(dir, { download: true })` + * runs this once per resolved URL on a CLI run with no ambient + * deadline, so a CDN that accepts the connection and then stalls would + * otherwise hang the pin with nothing to interrupt it (#1150). * * @param {string} url * @param {string} appDir @@ -1096,7 +1110,7 @@ async function writePinFile(appDir, imports, integrity, provider) { */ async function downloadBundle(url, appDir, filename) { const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), INTEGRITY_FETCH_TIMEOUT_MS); + const timer = setTimeout(() => controller.abort(), PIN_BUNDLE_TIMEOUT_MS); try { const response = await fetch(url, { signal: controller.signal }); if (!response.ok) { @@ -1115,7 +1129,7 @@ async function downloadBundle(url, appDir, filename) { return { bytes: buf.byteLength, integrity: await sha384Integrity(buf) }; } catch (e) { const why = e && e.name === 'AbortError' - ? `timed out after ${INTEGRITY_FETCH_TIMEOUT_MS}ms` + ? `timed out after ${PIN_BUNDLE_TIMEOUT_MS}ms` : e && e.message; console.error(`[webjs] download ${url} failed: ${why}`); return null; @@ -1130,19 +1144,19 @@ async function downloadBundle(url, appDir, filename) { * so the importmap can carry SRI hashes even when bundles aren't * locally vendored. * - * Bounded by the same timeout every other outbound call here carries. - * Default-mode `pinAll` runs this once per resolved URL, so a CDN that - * accepts the connection and then stalls would otherwise hang the pin - * with nothing to interrupt it: there is no ambient deadline on a CLI - * run, and `node --test` imposes none either. `downloadBundle` is the - * `--download` half of the same gap and is bounded the same way (#1150). + * Bounded by PIN_BUNDLE_TIMEOUT_MS, the same budget `downloadBundle` + * gets, since it transfers the same bytes and differs only in whether + * they are written to disk. Default-mode `pinAll` runs this once per + * resolved URL, so a CDN that accepts the connection and then stalls + * would otherwise hang the pin with nothing to interrupt it: there is + * no ambient deadline on a CLI run (#1150). * * @param {string} url * @returns {Promise} the integrity string, or null on failure */ async function fetchIntegrity(url) { const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), INTEGRITY_FETCH_TIMEOUT_MS); + const timer = setTimeout(() => controller.abort(), PIN_BUNDLE_TIMEOUT_MS); try { const response = await fetch(url, { signal: controller.signal }); if (!response.ok) { @@ -1156,7 +1170,7 @@ async function fetchIntegrity(url) { return await sha384Integrity(buf); } catch (e) { const why = e && e.name === 'AbortError' - ? `timed out after ${INTEGRITY_FETCH_TIMEOUT_MS}ms` + ? `timed out after ${PIN_BUNDLE_TIMEOUT_MS}ms` : e && e.message; console.error(`[webjs] hash ${url} failed: ${why}`); return null; diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index 86d5d763d..ae20a4555 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -2347,12 +2347,23 @@ test('pinAll: a bundle fetch that hangs is abandoned, not waited on forever', as for (const opts of [{}, { download: true }]) { const mode = opts.download ? '--download' : 'default'; const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';` }); + const realSetTimeout = globalThis.setTimeout; try { /** @type {Array} */ const signals = []; + /** Milliseconds each bundle fetch was given, read off the real timer. */ + /** @type {number[]} */ + const timeouts = []; + globalThis.setTimeout = /** @type {any} */ ((fn, ms, ...rest) => { + if (typeof ms === 'number' && ms >= 1000) timeouts.push(ms); + return realSetTimeout(fn, ms, ...rest); + }); await withMockedFetch(async (url, init) => { const s = String(url); if (s.includes('api.jspm.io')) { + // Forget the generate call's own 10s timer, so the only budget left + // in `timeouts` is the one the BUNDLE fetch sets a moment from now. + timeouts.length = 0; return jspmResponse({ picocolors: 'https://ga.jspm.io/npm:picocolors@1.1.1/index.js' }); } signals.push(init && init.signal); @@ -2369,7 +2380,16 @@ test('pinAll: a bundle fetch that hangs is abandoned, not waited on forever', as assert.equal(signals.length, 1, `${mode}: the bundle GET fired exactly once`); assert.ok(signals[0] instanceof AbortSignal, `${mode}: the bundle fetch must carry an AbortSignal so a stalled CDN cannot hang the pin`); + // The BUDGET, not just its existence. A pin transfers a whole package on + // a link the user may not control, so it gets 60s rather than the 10s + // the server's readiness-gating warmup pass uses; asserting only that a + // signal exists could not tell the two apart. 60s is what + // importmap-rails effectively allows, since Ruby's Net::HTTP defaults + // read_timeout to 60 even though the gem sets none. + assert.equal(timeouts[0], 60_000, + `${mode}: a pin bundle fetch must get the 60s budget, not the warmup pass's 10s`); } finally { + globalThis.setTimeout = realSetTimeout; await rm(dir, { recursive: true, force: true }); } } From 1c54ec8bc310e3c6305d27b2e9dd1abe27b73308 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 6 Aug 2026 11:42:22 +0530 Subject: [PATCH 18/18] fix: bring every CLI-path vendor timeout to the 60s Rails effectively allows A full comparison against importmap-rails, which sets no timeout anywhere and inherits Ruby's 60s Net::HTTP open and read defaults on every call it makes. The npm registry calls now take 60s. They are reached only by audit, outdated, and update, all CLI commands, and importmap-rails makes these same two requests with the same 60s effective bound, so 10s was a plain divergence. The generate call takes a per-caller budget instead of a constant, because it is the one call reached from BOTH sides. importmap-rails only ever resolves from the CLI, since its importmap is a static config file, so its flat 60s has no request path to slow down. Ours does: an unpinned app resolves on its cold first request. A pin passes 60s; a live resolve keeps 10s, so one stalled CDN cannot hold a request open for a minute. Two internal fallback call sites inside jspmGenerate were nearly missed, which would have dropped a pin back to 10s the moment the unified call failed and the per-install ladder took over. All five now thread the budget, and a grep for a call that omits it returns zero. The remaining 10s, on the warmup live-integrity pass, is deliberate and has no Rails counterpart: it gates readiness and is separately capped by a 15s total budget across every URL. --- packages/server/src/vendor.js | 45 ++++++++++++++-------- packages/server/test/vendor/vendor.test.js | 41 ++++++++++++++++++++ 2 files changed, 69 insertions(+), 17 deletions(-) diff --git a/packages/server/src/vendor.js b/packages/server/src/vendor.js index b33928c5b..26ea514eb 100644 --- a/packages/server/src/vendor.js +++ b/packages/server/src/vendor.js @@ -408,12 +408,15 @@ export function normalizeProvider(name) { * * @param {Array} installs e.g. ['dayjs@1.11.13', '@codemirror/lint@6.9.6'] * @param {string} provider one of SUPPORTED_PROVIDERS + * @param {number} [timeoutMs] defaults to the SERVER budget; a CLI caller + * passes the longer one, since it is a command someone is waiting on rather + * than a request being held open. * @returns {Promise} */ -async function jspmCall(installs, provider) { +async function jspmCall(installs, provider, timeoutMs = JSPM_GENERATE_TIMEOUT_MS) { const label = installs.length === 1 ? `'${installs[0]}'` : `${installs.length} packages`; const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), JSPM_GENERATE_TIMEOUT_MS); + const timer = setTimeout(() => controller.abort(), timeoutMs); try { const response = await fetch(JSPM_GENERATE_ENDPOINT, { method: 'POST', @@ -461,7 +464,7 @@ async function jspmCall(installs, provider) { return { ok: true, imports, transient: false }; } catch (e) { const msg = e && e.name === 'AbortError' - ? `timed out after ${JSPM_GENERATE_TIMEOUT_MS}ms` + ? `timed out after ${timeoutMs}ms` : `${e && e.message}`; console.error(`[webjs] could not vendor ${label} via ${provider}: ${msg}`); return { ok: false, imports: {}, transient: true }; @@ -487,8 +490,8 @@ async function jspmCall(installs, provider) { * @param {string} [provider] one of SUPPORTED_PROVIDERS; defaults to 'jspm' * @returns {Promise>} */ -async function jspmResolveOne(install, provider = 'jspm') { - const { ok, imports, transient } = await jspmProbeOne(install, provider); +async function jspmResolveOne(install, provider = 'jspm', timeoutMs) { + const { ok, imports, transient } = await jspmProbeOne(install, provider, timeoutMs); // Preserve the public contract: an empty map on any failure, and the // module-global retry flag set ONLY on a transient one (a permanent 401 // for an unresolvable private/server-only dep is tolerated). @@ -513,13 +516,13 @@ async function jspmResolveOne(install, provider = 'jspm') { * @param {string} provider * @returns {Promise} */ -function jspmProbeOne(install, provider) { +function jspmProbeOne(install, provider, timeoutMs) { const cacheKey = `${provider}::probe::${install}`; const existing = jspmCache.get(cacheKey); if (existing) return existing; const promise = (async () => { - const result = await jspmCall([install], provider); + const result = await jspmCall([install], provider, timeoutMs); // Do not cache a failure: a transient one must be re-attempted on the // next resolve, and a permanent one is cheap to re-confirm and must not // pin a stale "unresolvable" verdict across a dependency change. @@ -564,14 +567,18 @@ function jspmProbeOne(install, provider) { * * @param {Array} installs e.g. ['dayjs@1.11.13', 'clsx@2.1.1'] * @param {string} [provider] one of SUPPORTED_PROVIDERS; defaults to 'jspm' + * @param {number} [timeoutMs] per-call budget. Defaults to the SERVER one, + * because this runs on a cold first request as well as from the CLI; the two + * pin commands pass PIN_BUNDLE_TIMEOUT_MS instead. importmap-rails only ever + * resolves from the CLI, so its flat 60s has no request path to slow down. * @returns {Promise>} */ -export async function jspmGenerate(installs, provider = 'jspm') { +export async function jspmGenerate(installs, provider = 'jspm', timeoutMs) { if (installs.length === 0) return {}; // A single install has no cross-package graph to reconcile, so the // isolated path IS the coherent path; reuse the per-install cache. - if (installs.length === 1) return jspmResolveOne(installs[0], provider); + if (installs.length === 1) return jspmResolveOne(installs[0], provider, timeoutMs); // Stable key regardless of scan order so the same dep set hits cache. const unifiedKey = `${provider}::unified::${[...installs].sort().join('\n')}`; @@ -579,7 +586,7 @@ export async function jspmGenerate(installs, provider = 'jspm') { if (cached) return cached; const promise = (async () => { - const unified = await jspmCall(installs, provider); + const unified = await jspmCall(installs, provider, timeoutMs); if (unified.ok) return unified.imports; // The unified call failed. Drop the cached failure so a later retry @@ -591,14 +598,14 @@ export async function jspmGenerate(installs, provider = 'jspm') { // per-install fragments (each may still be cached / reachable) so we // serve whatever we can, and flag the transient failure for retry. lastLiveResolveFailed = true; - return mergePerInstall(await Promise.all(installs.map(i => jspmResolveOne(i, provider)))); + return mergePerInstall(await Promise.all(installs.map(i => jspmResolveOne(i, provider, timeoutMs)))); } // Permanent failure: at least one install is unresolvable. Probe each // in isolation to learn which ones jspm can resolve, then re-run the // unified call over only those so the survivors form one consistent // graph (restores #446 coherence for the resolvable subset). - const probes = await Promise.all(installs.map(i => jspmProbeOne(i, provider))); + const probes = await Promise.all(installs.map(i => jspmProbeOne(i, provider, timeoutMs))); // A GOOD package whose isolated probe failed TRANSIENTLY (a network blip // mid-probe) must NOT be classified as unresolvable and dropped. Only a @@ -633,11 +640,11 @@ export async function jspmGenerate(installs, provider = 'jspm') { return mergePerInstall(probes.map(p => p.imports)); } if (resolvable.length === 0) return {}; - if (resolvable.length === 1) return jspmResolveOne(resolvable[0], provider); + if (resolvable.length === 1) return jspmResolveOne(resolvable[0], provider, timeoutMs); // Re-run unified over the resolvable subset. If even that fails (a // conflict among the survivors), fall back to their merged fragments. - const retry = await jspmCall(resolvable, provider); + const retry = await jspmCall(resolvable, provider, timeoutMs); if (retry.ok) return retry.imports; return mergePerInstall(resolvable.map(i => probes[installs.indexOf(i)].imports)); })(); @@ -1307,7 +1314,7 @@ export async function pinAll(appDir, opts = {}) { installs.push(install); partsByInstall.set(spec, { pkg, version, subpath }); } - const resolved = await jspmGenerate(installs, from); + const resolved = await jspmGenerate(installs, from, PIN_BUNDLE_TIMEOUT_MS); /** @type {Record} */ const importmap = {}; @@ -1549,7 +1556,11 @@ export async function listPinned(appDir) { // --------------------------------------------------------------------------- const NPM_REGISTRY = 'https://registry.npmjs.org'; -const NPM_TIMEOUT_MS = 10_000; +// The npm registry is reached only by `audit`, `outdated`, and `update`, +// all CLI commands, so it takes the same generous budget the pin bundle +// fetch does rather than the server's. importmap-rails makes these same two +// calls with Ruby's 60s Net::HTTP default. +const NPM_TIMEOUT_MS = 60_000; /** * Fetch one URL from registry.npmjs.org with a small timeout. Returns @@ -1749,7 +1760,7 @@ export async function updatePinned(appDir, opts = {}) { if (specPkg !== pkg) continue; const subpath = spec.slice(specPkg.length); const install = `${pkg}@${latest}${subpath}`; - const resolved = await jspmGenerate([install], from); + const resolved = await jspmGenerate([install], from, PIN_BUNDLE_TIMEOUT_MS); const newUrl = resolved[spec]; if (!newUrl) continue; newImports[spec] = newUrl; diff --git a/packages/server/test/vendor/vendor.test.js b/packages/server/test/vendor/vendor.test.js index ae20a4555..6a1318c80 100644 --- a/packages/server/test/vendor/vendor.test.js +++ b/packages/server/test/vendor/vendor.test.js @@ -2395,6 +2395,47 @@ test('pinAll: a bundle fetch that hangs is abandoned, not waited on forever', as } }); +test('the generate call gets the CLI budget from a pin and the server budget from a resolve', async () => { + // importmap-rails sets no timeout and inherits Ruby's 60s Net::HTTP default + // on every jspm call. Matching that flatly would be wrong here, because + // importmap-rails only ever resolves from the CLI (its importmap is a static + // config file), while WebJs also resolves on a cold first request. A 60s + // budget on the request path would let one stalled CDN hold a request for a + // minute, so the budget is per-caller: 60s from `pinAll`, 10s from a live + // resolve. + // + // Counterfactual: drop the `timeoutMs` argument at either call site and the + // matching half of this fails, since both numbers are asserted. + const budgets = []; + const realSetTimeout = globalThis.setTimeout; + globalThis.setTimeout = /** @type any */ ((fn, ms, ...rest) => { + if (typeof ms === 'number' && ms >= 1000) budgets.push(ms); + return realSetTimeout(fn, ms, ...rest); + }); + const dir = await makeTempAppWithSource({ 'app/page.ts': `import pico from 'picocolors';` }); + try { + const mock = async (url) => (String(url).includes('api.jspm.io') + ? jspmResponse({ picocolors: 'https://ga.jspm.io/npm:picocolors@1.1.1/index.js' }) + : bundleResponse(new TextEncoder().encode('export default 1;'))); + + budgets.length = 0; + await withMockedFetch(mock, async () => { clearVendorCache(); await pinAll(dir); }); + assert.ok(budgets.includes(60_000), `a pin's generate call must get 60s, saw ${budgets}`); + assert.ok(!budgets.includes(10_000), `a pin must not fall back to the server budget, saw ${budgets}`); + + budgets.length = 0; + await withMockedFetch(mock, async () => { + clearVendorCache(); + await vendorImportMapEntries(new Set(['picocolors']), process.cwd()); + }); + assert.ok(budgets.includes(10_000), `a live resolve must keep the 10s server budget, saw ${budgets}`); + assert.ok(!budgets.includes(60_000), `a live resolve must not take the CLI budget, saw ${budgets}`); + } finally { + globalThis.setTimeout = realSetTimeout; + await rm(dir, { recursive: true, force: true }); + } +}); + test('resolveVendorImports: PINNED path is unchanged (live-hash path not taken)', async () => { // Counterfactual that the pin path did not regress: a pin file with its own // integrity returns verbatim, and NO bundle fetch fires for it.