From 6c5e0316d0ade6c3d15a1b0f9ff159a1dae2151b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 18 Aug 2026 19:05:47 -0400 Subject: [PATCH 1/2] test: hold pack-manifest and check-conformance in agreement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two files encode the same rule about which pnpm specifier shapes survive `npm publish`: pack-manifest.mjs decides what it rewrites at pack time, and check-conformance.mjs's publishable-manifests rule decides what it lets past CI on the strength of those hooks being wired. A shape the script REFUSES but the check EXCUSES is a green CI with a red release — the check reports all clear and the release is what breaks, which is the exact inversion the check exists to prevent. That inversion shipped twice during review of #417, both times in code that read as obviously correct: catalog: (4127ead) and pnpm's alias form workspace:@ (de6a900). Nothing prevented a third. The deep review there named the absence of a test as the top remaining risk. The test drives BOTH real implementations rather than restating their logic. A third copy of the rule in a test would be one more thing to drift, which is the bug class rather than a fix for it. It asserts the invariant as a biconditional — the script refuses a shape if and only if the check flags it — across all twelve documented shapes, plus the negative cases: workspace:^5.0.0 and friends must NOT trip the alias predicate, or the fix for #412 starts rejecting the shape it was written to support. Both files needed a seam. pack-manifest.mjs derived its package root from import.meta.url and process.exit'd inside the decision logic, so the per-spec decision is now one exported function taking its version lookup as an argument and throwing rather than exiting; main() is exported and only runs when the file is executed directly. The catalog refusal moved out of the prepack loop into that same function, so "refused or resolved" lives in one place instead of two. check-conformance.mjs exports the two rules and guards main(), because importing a module should not run a repo-wide conformance sweep. Verified by mutation rather than by a green run. Reintroducing each inversion that actually shipped turns the suite red: check excuses catalog: (2 failures), check excuses the alias form (3), script false-positives on ^ ranges (3), script stops refusing catalog: (3), bare workspace: stops resolving (1), a refusal returns 0 instead of 1 (3), postpack stops restoring (1). One mutation is not caught and is equivalent rather than a gap: widening lastIndexOf('@') > 0 to includes('@') cannot change any real shape's verdict, because an explicit range holds no @ for the widened predicate to see. The publish path is exercised for real, since this script runs during npm publish: npm pack produces a tarball whose manifest carries ^5.11.1 with no workspace: or catalog: anywhere, the repo manifest comes back byte for byte, and no stale .pack-backup is left to wedge the next pack. The exit codes matter as much as the rewrite — npm reads them to abort a publish — so the refusal paths are frozen too: a refused specifier exits 1, leaves the manifest untouched, and writes no backup. Closes #435 --- bestax-migrate/scripts/pack-manifest.mjs | 234 +++++++++++------ scripts/check-conformance.mjs | 28 ++- scripts/pack-manifest.test.mjs | 305 +++++++++++++++++++++++ 3 files changed, 480 insertions(+), 87 deletions(-) create mode 100644 scripts/pack-manifest.test.mjs diff --git a/bestax-migrate/scripts/pack-manifest.mjs b/bestax-migrate/scripts/pack-manifest.mjs index cef7831f9..645f021ac 100644 --- a/bestax-migrate/scripts/pack-manifest.mjs +++ b/bestax-migrate/scripts/pack-manifest.mjs @@ -29,14 +29,25 @@ * risk npm not running `postpack` and leaving the repo manifest rewritten for * @semantic-release/git to commit — the exact failure the backup below exists * to prevent. + * + * The per-specifier decision is exported and takes its version lookup as an + * argument, and `main` only runs when this file is executed directly. That + * seam exists for one reason (#435): this script and + * scripts/check-conformance.mjs encode the same rule about which pnpm shapes + * are resolvable, and a shape this script REFUSES but the check EXCUSES is a + * green CI with a red release. That inversion happened twice during review of + * #417 — once for `catalog:`, once for the alias form — so a test now drives + * both real implementations and asserts they agree shape for shape. It can + * only do that if the decision is callable without packing anything. */ import fs from 'node:fs'; import path from 'node:path'; -import { fileURLToPath } from 'node:url'; +import process from 'node:process'; +import { fileURLToPath, pathToFileURL } from 'node:url'; -const pkgRoot = path.dirname(path.dirname(fileURLToPath(import.meta.url))); -const manifest = path.join(pkgRoot, 'package.json'); -const backup = path.join(pkgRoot, 'package.json.pack-backup'); +const defaultPkgRoot = path.dirname( + path.dirname(fileURLToPath(import.meta.url)) +); const DEP_SECTIONS = [ 'dependencies', @@ -45,23 +56,41 @@ const DEP_SECTIONS = [ 'optionalDependencies', ]; -/** Version of a workspace package, read through the linked node_modules entry. */ -function resolveWorkspaceVersion(name) { - const linked = path.join(pkgRoot, 'node_modules', name, 'package.json'); - if (!fs.existsSync(linked)) { - console.error( - `pack-manifest: cannot resolve the workspace dependency "${name}" — ` + - `${path.relative(pkgRoot, linked)} does not exist.\n` + - 'Run `pnpm install` from the repo root before packing.' - ); - process.exit(1); - } - const { version } = JSON.parse(fs.readFileSync(linked, 'utf8')); - if (!version) { - console.error(`pack-manifest: "${name}" has no version in its manifest`); - process.exit(1); +/** + * Raised for a specifier this script will not resolve. Thrown rather than + * exited so the decision can be exercised by a test; `main` turns it back into + * the same message-and-exit-1 the CLI has always produced. + */ +export class UnsupportedSpecifierError extends Error { + constructor(message) { + super(message); + this.name = 'UnsupportedSpecifierError'; } - return version; +} + +/** + * Version of a workspace package, read through the linked node_modules entry. + * Curried over the package root so a test can supply its own lookup instead of + * needing a real pnpm-linked tree. + */ +export function makeWorkspaceVersionResolver(pkgRoot = defaultPkgRoot) { + return name => { + const linked = path.join(pkgRoot, 'node_modules', name, 'package.json'); + if (!fs.existsSync(linked)) { + throw new UnsupportedSpecifierError( + `pack-manifest: cannot resolve the workspace dependency "${name}" — ` + + `${path.relative(pkgRoot, linked)} does not exist.\n` + + 'Run `pnpm install` from the repo root before packing.' + ); + } + const { version } = JSON.parse(fs.readFileSync(linked, 'utf8')); + if (!version) { + throw new UnsupportedSpecifierError( + `pack-manifest: "${name}" has no version in its manifest` + ); + } + return version; + }; } /** @@ -71,89 +100,136 @@ function resolveWorkspaceVersion(name) { * A bare `workspace:` is pnpm's shorthand for `workspace:*` — leaving it to the * unwrap branch would emit an empty specifier. */ -function resolveSpecifier(name, spec) { +export function resolveSpecifier(name, spec, resolveVersion, label = name) { const rest = spec.slice('workspace:'.length); - if (rest === '*' || rest === '') return resolveWorkspaceVersion(name); - if (rest === '^' || rest === '~') - return `${rest}${resolveWorkspaceVersion(name)}`; + if (rest === '*' || rest === '') return resolveVersion(name); + if (rest === '^' || rest === '~') return `${rest}${resolveVersion(name)}`; // `workspace:@` is pnpm's alias form, and it does NOT publish as // a bare range: pnpm emits `npm:@`. Unwrapping it would write // "@scope/pkg@^5", which no package manager can install — #412 again, wearing // a different hat. A semver range never contains "/" or a non-leading "@", // so this only catches the alias form. if (rest.includes('/') || rest.lastIndexOf('@') > 0) { - console.error( - `pack-manifest: "${name}": "${spec}" is pnpm's alias form, which ` + + throw new UnsupportedSpecifierError( + `pack-manifest: ${label} is "${spec}", pnpm's alias form, which ` + `publishes as \`npm:@\`.\n` + 'This script does not synthesize that. Depend on the package under its ' + 'real name, or teach pack-manifest.mjs the npm: alias rewrite.' ); - process.exit(1); } return rest; } -const mode = process.argv[2]; - -if (mode === 'prepack') { - if (fs.existsSync(backup)) { - console.error( - 'pack-manifest: package.json.pack-backup already exists — a previous ' + - 'pack did not finish.\n' + - 'Restore the workspace manifest first: mv package.json.pack-backup package.json' +/** + * The whole per-specifier decision, in one place: what this script does with + * any one dependency entry. + * + * Returns the rewritten specifier, or `null` when the entry is none of this + * script's business and should be left exactly as written. Throws + * UnsupportedSpecifierError for the shapes it refuses. + * + * This is the function scripts/pack-manifest.test.mjs drives, and it is the + * single source of "refused or resolved" that check-conformance.mjs's + * UNRESOLVABLE_AT_PACK has to stay in step with (#435). + */ +export function rewriteSpecifier(name, spec, resolveVersion, label = name) { + if (typeof spec !== 'string') return null; + // `catalog:` is the other protocol `pnpm publish` resolves and `npm publish` + // ships verbatim — the exact shape of #412. This script cannot resolve it + // (the range lives in pnpm-workspace.yaml, not in the linked package), so + // fail the release rather than pack a broken tarball. + if (spec.startsWith('catalog:')) { + throw new UnsupportedSpecifierError( + `pack-manifest: ${label} is "${spec}", and this script cannot resolve ` + + `the catalog: protocol.\n` + + 'Give it a plain semver range, or teach pack-manifest.mjs to read ' + + '`catalog`/`catalogs` from pnpm-workspace.yaml.' ); - process.exit(1); } + if (!spec.startsWith('workspace:')) return null; + return resolveSpecifier(name, spec, resolveVersion, label); +} + +/** + * The CLI. Wrapped in a function and guarded below so importing this module + * for the agreement test does not pack anything, and so the refusals above can + * throw (testable) while the command still exits 1 with the same message. + */ +export function main( + argv = process.argv.slice(2), + { pkgRoot = defaultPkgRoot } = {} +) { + const manifest = path.join(pkgRoot, 'package.json'); + const backup = path.join(pkgRoot, 'package.json.pack-backup'); + const resolveVersion = makeWorkspaceVersionResolver(pkgRoot); + const mode = argv[0]; + + if (mode === 'prepack') { + if (fs.existsSync(backup)) { + console.error( + 'pack-manifest: package.json.pack-backup already exists — a previous ' + + 'pack did not finish.\n' + + 'Restore the workspace manifest first: mv package.json.pack-backup package.json' + ); + return 1; + } - const pkg = JSON.parse(fs.readFileSync(manifest, 'utf8')); - const rewritten = []; + const pkg = JSON.parse(fs.readFileSync(manifest, 'utf8')); + const rewritten = []; - for (const section of DEP_SECTIONS) { - for (const [name, spec] of Object.entries(pkg[section] ?? {})) { - if (typeof spec !== 'string') continue; - // `catalog:` is the other protocol `pnpm publish` resolves and - // `npm publish` ships verbatim — the exact shape of #412. This script - // cannot resolve it (the range lives in pnpm-workspace.yaml, not in the - // linked package), so fail the release rather than pack a broken tarball. - if (spec.startsWith('catalog:')) { - console.error( - `pack-manifest: ${section}.${name} is "${spec}", and this script ` + - `cannot resolve the catalog: protocol.\n` + - 'Give it a plain semver range, or teach pack-manifest.mjs to read ' + - '`catalog`/`catalogs` from pnpm-workspace.yaml.' - ); - process.exit(1); + for (const section of DEP_SECTIONS) { + for (const [name, spec] of Object.entries(pkg[section] ?? {})) { + let resolved; + try { + resolved = rewriteSpecifier( + name, + spec, + resolveVersion, + `${section}.${name}` + ); + } catch (err) { + if (!(err instanceof UnsupportedSpecifierError)) throw err; + console.error(err.message); + return 1; + } + if (resolved === null) continue; + pkg[section][name] = resolved; + rewritten.push(`${section}.${name}: ${spec} -> ${resolved}`); } - if (!spec.startsWith('workspace:')) continue; - const resolved = resolveSpecifier(name, spec); - pkg[section][name] = resolved; - rewritten.push(`${section}.${name}: ${spec} -> ${resolved}`); } - } - if (!rewritten.length) { - console.log('pack-manifest: no workspace: specifiers to resolve'); - process.exit(0); + if (!rewritten.length) { + console.log('pack-manifest: no workspace: specifiers to resolve'); + return 0; + } + + fs.copyFileSync(manifest, backup); + // Keep npm's own formatting (2-space + trailing newline) so the restored + // file and the packed one differ only in the specifiers. + fs.writeFileSync(manifest, `${JSON.stringify(pkg, null, 2)}\n`); + console.log( + `pack-manifest: resolved ${rewritten.length} workspace specifier(s)` + ); + for (const line of rewritten) console.log(` ${line}`); + return 0; } - fs.copyFileSync(manifest, backup); - // Keep npm's own formatting (2-space + trailing newline) so the restored - // file and the packed one differ only in the specifiers. - fs.writeFileSync(manifest, `${JSON.stringify(pkg, null, 2)}\n`); - console.log( - `pack-manifest: resolved ${rewritten.length} workspace specifier(s)` - ); - for (const line of rewritten) console.log(` ${line}`); -} else if (mode === 'postpack') { - if (!fs.existsSync(backup)) { - // prepack exits early when there is nothing to rewrite; that is not an error. - console.log('pack-manifest: no backup to restore'); - process.exit(0); + if (mode === 'postpack') { + if (!fs.existsSync(backup)) { + // prepack exits early when there is nothing to rewrite; not an error. + console.log('pack-manifest: no backup to restore'); + return 0; + } + fs.copyFileSync(backup, manifest); + fs.rmSync(backup); + console.log('pack-manifest: workspace manifest restored'); + return 0; } - fs.copyFileSync(backup, manifest); - fs.rmSync(backup); - console.log('pack-manifest: workspace manifest restored'); -} else { + console.error('Usage: node scripts/pack-manifest.mjs '); - process.exit(1); + return 1; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + process.exitCode = main(); } diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index 2c19db5be..f12cc2bd8 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -42,7 +42,7 @@ */ import { readFile, readdir, writeFile, access } from 'node:fs/promises'; import { join, relative, dirname } from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; // `registerVarsKeys` lives in lib/ so the API-docs generator can share the same // parser — it additionally exposes values and selector nesting, which the CSS @@ -1025,8 +1025,15 @@ const hasPackTimeProtocol = spec => // The two shapes the pack hooks refuse rather than guess at, so devDependencies // carrying them are violations no matter how the hooks are wired. Kept in step -// with bestax-migrate/scripts/pack-manifest.mjs, which exits 1 on both. -const UNRESOLVABLE_AT_PACK = [ +// with bestax-migrate/scripts/pack-manifest.mjs, which refuses both. +// +// "Kept in step" used to mean "by reading both files carefully", and that +// failed twice during review of #417 — once for `catalog:`, once for the alias +// form. A shape the script REFUSES but this check EXCUSES is a green CI with a +// red release, which is the exact inversion this check exists to prevent. So +// these are exported and scripts/pack-manifest.test.mjs now drives them against +// the real resolver, asserting the two agree shape for shape (#435). +export const UNRESOLVABLE_AT_PACK = [ { matches: spec => spec.startsWith('catalog:'), why: @@ -1047,7 +1054,7 @@ const UNRESOLVABLE_AT_PACK = [ }, ]; -const unresolvableAtPack = spec => +export const unresolvableAtPack = spec => typeof spec === 'string' && UNRESOLVABLE_AT_PACK.find(rule => rule.matches(spec)); @@ -1307,7 +1314,12 @@ async function main() { } } -main().catch(err => { - console.error(err); - process.exit(1); -}); +// Only run the suite when invoked as a command. The rules above are imported +// by scripts/pack-manifest.test.mjs, and importing a module should not run a +// repo-wide conformance sweep as a side effect. +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + main().catch(err => { + console.error(err); + process.exit(1); + }); +} diff --git a/scripts/pack-manifest.test.mjs b/scripts/pack-manifest.test.mjs new file mode 100644 index 000000000..affc0a898 --- /dev/null +++ b/scripts/pack-manifest.test.mjs @@ -0,0 +1,305 @@ +/** + * Holds bestax-migrate/scripts/pack-manifest.mjs and the + * `publishable-manifests` rule in scripts/check-conformance.mjs in agreement + * (#435). + * + * Both files encode the same rule about which pnpm specifier shapes survive + * `npm publish`. The pack script decides what it will rewrite at pack time; the + * conformance check decides what it will let past CI on the strength of the + * pack hooks being wired. **A shape the script refuses but the check excuses is + * a green CI with a red release** — the check reports all clear and the release + * is what breaks, which is the exact inversion the check exists to prevent. + * + * That inversion shipped twice during review of #417, both times in code that + * read as obviously correct: `catalog:` (fixed in 4127ead) and pnpm's alias + * form `workspace:@` (fixed in de6a900, after it was found + * unwrapping to a bare `@scope/pkg@^5` that no package manager can install — + * #412 wearing a different hat). + * + * So this drives BOTH real implementations rather than restating their logic. A + * third copy of the rule in a test would be one more thing to drift, which is + * the bug class rather than a fix for it. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { + main, + rewriteSpecifier, + resolveSpecifier, + UnsupportedSpecifierError, +} from '../bestax-migrate/scripts/pack-manifest.mjs'; +import { unresolvableAtPack } from './check-conformance.mjs'; + +/** The linked-package lookup, stubbed so no pnpm tree is required. */ +const VERSION = '5.11.1'; +const resolveVersion = () => VERSION; + +const NAME = '@allxsmith/bestax-bulma'; + +/** Does the pack script refuse this specifier? */ +function packRefuses(spec) { + try { + rewriteSpecifier(NAME, spec, resolveVersion); + return false; + } catch (err) { + if (err instanceof UnsupportedSpecifierError) return true; + throw err; + } +} + +/** Does the conformance check flag this specifier as unresolvable at pack time? */ +const checkFlags = spec => Boolean(unresolvableAtPack(spec)); + +/** + * Every shape pnpm documents, and what each side is supposed to do with it. + * `refused: true` means BOTH the script refuses it and the check flags it — + * that pairing is the invariant, and it is asserted as a biconditional below + * rather than as two independent expectations. + */ +const SHAPES = [ + { spec: 'workspace:*', refused: false, resolves: VERSION }, + { spec: 'workspace:', refused: false, resolves: VERSION }, + { spec: 'workspace:^', refused: false, resolves: `^${VERSION}` }, + { spec: 'workspace:~', refused: false, resolves: `~${VERSION}` }, + { spec: 'workspace:^5.0.0', refused: false, resolves: '^5.0.0' }, + { spec: 'workspace:~5.0.0', refused: false, resolves: '~5.0.0' }, + { spec: 'workspace:5.0.0', refused: false, resolves: '5.0.0' }, + { spec: 'workspace:>=5 <6', refused: false, resolves: '>=5 <6' }, + { spec: 'workspace:@allxsmith/bestax-bulma@^5', refused: true }, + { spec: 'workspace:bestax-bulma@^5', refused: true }, + { spec: 'catalog:', refused: true }, + { spec: 'catalog:default', refused: true }, +]; + +// --- the invariant ---------------------------------------------------------- + +for (const { spec, refused } of SHAPES) { + test(`agreement: "${spec}" is ${refused ? 'refused by both' : 'accepted by both'}`, () => { + assert.equal( + packRefuses(spec), + checkFlags(spec), + `pack-manifest.mjs and check-conformance.mjs disagree about "${spec}". ` + + `That is a green-CI/red-release inversion: whichever one is wrong, the ` + + `check will pass a manifest the pack hooks then refuse to publish.` + ); + assert.equal(packRefuses(spec), refused, `expected refusal=${refused}`); + }); +} + +// --- what the accepted shapes actually resolve to --------------------------- + +for (const { spec, refused, resolves } of SHAPES) { + if (refused) continue; + test(`resolve: "${spec}" -> "${resolves}"`, () => { + assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), resolves); + }); +} + +// --- the false-positive guard the #412 fix depends on ----------------------- + +test('an explicit range is not mistaken for the alias form', () => { + // `rest.includes('/') || rest.lastIndexOf('@') > 0` is the alias predicate on + // both sides. If it ever caught `workspace:^5.0.0`, the fix for #412 would + // start rejecting the very shape it was written to support. + for (const spec of [ + 'workspace:^5.0.0', + 'workspace:>=5 <6', + 'workspace:5.0.0', + ]) { + assert.equal(checkFlags(spec), false, `${spec} must not be flagged`); + assert.equal(packRefuses(spec), false, `${spec} must not be refused`); + } +}); + +test('a scoped name in the alias form is caught by the "/" arm', () => { + // Two arms, two shapes: scoped names trip `includes('/')`, unscoped ones trip + // `lastIndexOf('@') > 0`. Both are exercised so neither can be dropped. + assert.ok(packRefuses('workspace:@scope/pkg@^1')); + assert.ok(checkFlags('workspace:@scope/pkg@^1')); + assert.ok(packRefuses('workspace:pkg@^1')); + assert.ok(checkFlags('workspace:pkg@^1')); +}); + +// --- shapes neither side owns ---------------------------------------------- + +test('a plain semver range is nobody’s business', () => { + for (const spec of ['^5.0.0', '5.0.0', '>=5 <6', 'npm:other@^1']) { + assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), null); + assert.equal(checkFlags(spec), false); + } +}); + +test('a non-string specifier is left alone rather than crashing', () => { + // package.json can carry odd values; neither side should throw on them. + for (const spec of [undefined, null, 42, {}]) { + assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), null); + assert.equal(checkFlags(spec), false); + } +}); + +// --- messages name the dependency, since that is what a maintainer greps ---- + +test('a refusal names the offending entry and the specifier', () => { + assert.throws( + () => + rewriteSpecifier(NAME, 'catalog:', resolveVersion, 'devDependencies.x'), + err => + err instanceof UnsupportedSpecifierError && + err.message.includes('devDependencies.x') && + err.message.includes('catalog:') + ); + assert.throws( + () => + rewriteSpecifier( + NAME, + 'workspace:pkg@^1', + resolveVersion, + 'devDependencies.y' + ), + err => + err instanceof UnsupportedSpecifierError && + err.message.includes('devDependencies.y') && + err.message.includes('alias form') + ); +}); + +test('resolveSpecifier defers the version lookup to its caller', () => { + // The seam that makes this testable at all: no pnpm-linked tree required. + const calls = []; + const spy = name => { + calls.push(name); + return '1.2.3'; + }; + assert.equal(resolveSpecifier(NAME, 'workspace:^', spy), '^1.2.3'); + assert.deepEqual(calls, [NAME]); +}); + +// --- what the CLI does with a refusal --------------------------------------- +// +// The specifier decisions above are the invariant this file exists for, but the +// exit CODE is what npm keys off to abort a publish. These refusals used to be +// `process.exit(1)` and are now thrown and translated by `main`, so the +// translation is worth freezing: if a refusal ever returned 0, npm would +// publish the broken tarball this whole guard exists to prevent — the bug +// inverted. + +/** A throwaway package root, so nothing in the repo is touched. */ +function fixtureRoot(pkg) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pack-manifest-')); + fs.writeFileSync( + path.join(root, 'package.json'), + `${JSON.stringify(pkg, null, 2)}\n` + ); + return root; +} + +const readManifest = root => + fs.readFileSync(path.join(root, 'package.json'), 'utf8'); +const backupPath = root => path.join(root, 'package.json.pack-backup'); + +test('a refused specifier aborts the pack without touching the manifest', () => { + const root = fixtureRoot({ + name: 'fixture', + version: '0.0.0', + devDependencies: { '@allxsmith/bestax-bulma': 'catalog:' }, + }); + const before = readManifest(root); + + assert.equal(main(['prepack'], { pkgRoot: root }), 1, 'must exit non-zero'); + assert.equal(readManifest(root), before, 'manifest must be left intact'); + // A stale backup makes the NEXT prepack refuse to run, so a half-done + // refusal would wedge the following release too. + assert.equal(fs.existsSync(backupPath(root)), false, 'no backup left behind'); +}); + +test('the alias form aborts the pack the same way', () => { + const root = fixtureRoot({ + name: 'fixture', + version: '0.0.0', + devDependencies: { 'bestax-bulma': 'workspace:@allxsmith/bestax-bulma@^5' }, + }); + const before = readManifest(root); + assert.equal(main(['prepack'], { pkgRoot: root }), 1); + assert.equal(readManifest(root), before); + assert.equal(fs.existsSync(backupPath(root)), false); +}); + +test('prepack refuses to clobber a backup a previous pack left behind', () => { + const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); + fs.writeFileSync(backupPath(root), '{}'); + assert.equal(main(['prepack'], { pkgRoot: root }), 1); +}); + +test('a manifest with nothing to rewrite is a no-op, not a failure', () => { + const root = fixtureRoot({ + name: 'fixture', + version: '0.0.0', + dependencies: { bulma: '^1.0.4' }, + }); + const before = readManifest(root); + assert.equal(main(['prepack'], { pkgRoot: root }), 0); + assert.equal(readManifest(root), before); + assert.equal(fs.existsSync(backupPath(root)), false); +}); + +test('postpack with no backup is a no-op, not a failure', () => { + const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); + assert.equal(main(['postpack'], { pkgRoot: root }), 0); +}); + +test('an unknown mode fails rather than silently doing nothing', () => { + const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); + assert.equal(main(['bogus'], { pkgRoot: root }), 1); + assert.equal(main([], { pkgRoot: root }), 1); +}); + +test('prepack rewrites and postpack restores the repo manifest exactly', () => { + // The round trip is the whole safety property: the release commits + // package.json, so a postpack that did not restore byte-for-byte would + // commit resolved specifiers back into the workspace. + const root = fixtureRoot({ + name: 'fixture', + version: '0.0.0', + devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, + }); + const linked = path.join(root, 'node_modules', '@allxsmith', 'bestax-bulma'); + fs.mkdirSync(linked, { recursive: true }); + fs.writeFileSync( + path.join(linked, 'package.json'), + JSON.stringify({ name: '@allxsmith/bestax-bulma', version: '5.11.1' }) + ); + const before = readManifest(root); + + assert.equal(main(['prepack'], { pkgRoot: root }), 0); + const packed = JSON.parse(readManifest(root)); + assert.equal( + packed.devDependencies['@allxsmith/bestax-bulma'], + '^5.11.1', + 'the packed manifest carries a real range' + ); + assert.ok(fs.existsSync(backupPath(root)), 'the original is backed up'); + + assert.equal(main(['postpack'], { pkgRoot: root }), 0); + assert.equal(readManifest(root), before, 'restored byte for byte'); + assert.equal(fs.existsSync(backupPath(root)), false, 'backup cleaned up'); +}); + +test('an unresolvable workspace dep fails instead of packing a broken range', () => { + // No linked package in node_modules — `pnpm install` was never run. Packing + // anyway would emit an empty or bogus specifier. This exits 1 rather than + // propagating, which is what the CLI did before the refactor and what npm + // reads to abort the publish. + const root = fixtureRoot({ + name: 'fixture', + version: '0.0.0', + devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, + }); + const before = readManifest(root); + assert.equal(main(['prepack'], { pkgRoot: root }), 1); + assert.equal(readManifest(root), before); + assert.equal(fs.existsSync(backupPath(root)), false); +}); From cf95eb781550ef75ecdd23d779822f4304c4ac6b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 18 Aug 2026 19:27:32 -0400 Subject: [PATCH 2/2] test: hold the agreement for shapes nobody enumerated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep review on #531 named the gap the twelve-shape table leaves: the two predicates agree on those twelve by assertion, and on every other string only because they are currently textually identical. An edit to one of them touching a shape nobody listed would slip through — which is the drift class this file exists to catch, just outside the part it was watching. So the biconditional now runs over a spread of odd, adversarial and not-yet-invented specifiers as well: plausible future pnpm shapes, adjacent protocols neither side owns, and degenerate strings. It deliberately does not assert what the verdict should be. Deciding the right answer for a hypothetical future pnpm syntax is not this test's job; noticing that the two files stopped answering it the same way is. Verified the same way as the rest: adding `|| rest === 'latest'` to one side only — a shape absent from the table — turns the suite red, where before it passed. --- scripts/pack-manifest.test.mjs | 59 ++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/scripts/pack-manifest.test.mjs b/scripts/pack-manifest.test.mjs index affc0a898..e88bfcddc 100644 --- a/scripts/pack-manifest.test.mjs +++ b/scripts/pack-manifest.test.mjs @@ -303,3 +303,62 @@ test('an unresolvable workspace dep fails instead of packing a broken range', () assert.equal(readManifest(root), before); assert.equal(fs.existsSync(backupPath(root)), false); }); + +// --- agreement beyond the enumerated shapes --------------------------------- +// +// The table above pins the twelve shapes pnpm documents. Deep review on #531 +// noted the gap that leaves: the two predicates agree on those twelve by +// assertion, and on everything else only because they are currently textually +// identical. An edit to one of them that happens to change a shape nobody +// listed would slip through. +// +// So assert the biconditional over a spread of odd, adversarial and +// not-yet-invented specifiers. This deliberately does NOT assert what the +// verdict should be — only that both sides reach the same one. Deciding the +// right answer for a hypothetical future pnpm syntax is not this test's job; +// noticing that the two files stopped answering it the same way is. +const ODD_SPECIFIERS = [ + // plausible future or undocumented pnpm shapes + 'workspace:^1.0.0-beta.1', + 'workspace:*-next', + 'workspace:latest', + 'workspace:1.x', + 'workspace:>=1', + 'workspace:@scope/name', + 'workspace:@scope/name@', + 'workspace:name@', + 'workspace:@', + 'workspace:@@', + 'workspace://', + 'workspace:a/b/c@1', + 'catalog:with-a-name', + 'catalog:@scope/thing', + // adjacent protocols neither side owns + 'npm:pkg@^1', + 'file:../pkg', + 'link:../pkg', + 'git+https://example.test/x.git', + 'jsr:@scope/pkg', + // degenerate strings + '', + ' ', + 'workspace', + 'workspaces:*', + 'catalog', + 'CATALOG:', + 'WORKSPACE:^', + 'x'.repeat(200), + 'workspace:' + 'a'.repeat(200), +]; + +for (const spec of ODD_SPECIFIERS) { + test(`agreement holds for an unlisted shape: ${JSON.stringify(spec).slice(0, 40)}`, () => { + assert.equal( + packRefuses(spec), + checkFlags(spec), + `pack-manifest.mjs and check-conformance.mjs disagree about ` + + `${JSON.stringify(spec)}. Whichever is right, one of them was edited ` + + `without the other — the drift this file exists to catch.` + ); + }); +}