From 235d252227db2e9a5bdd9b57cdfbe275258edcec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 20:00:08 +0000 Subject: [PATCH 1/2] feat(types): verify a link:/file: install by the location the host declared WIP checkpoint: second verification axis + tests, before the closure build. Claude-Session: https://claude.ai/code/session_01DapQyvYrFb1MxSYe7BL2nt Co-authored-by: Claude --- packages/types/src/node.test.ts | 455 ++++++++++++++++++++++++++++---- packages/types/src/node.ts | 328 +++++++++++++++++++---- 2 files changed, 678 insertions(+), 105 deletions(-) diff --git a/packages/types/src/node.test.ts b/packages/types/src/node.test.ts index 0d0db68aa2..419d673b42 100644 --- a/packages/types/src/node.test.ts +++ b/packages/types/src/node.test.ts @@ -1299,8 +1299,10 @@ describe('the declared leg loads an ESM-only package via a hostRoot node_modules * stays green). * * `link:` / `file:` name a LOCATION rather than a package, so no name can be - * derived from them at all; they keep the key expectation, and with it today's - * conservative refusal. + * derived from them at all; they keep the key expectation on THIS axis. What + * #17046 later added is a second axis rather than a looser first one — the + * declared PATH — so the boundary pin below is now a two-axis boundary, and + * says so. */ describe('an aliased install is verified against the name its DECLARATION names (#14278)', () => { /** The card's exact shape: `import` condition only, no `require`, no `main`. */ @@ -1465,18 +1467,21 @@ describe('an aliased install is verified against the name its DECLARATION names expect((await createHostImporter(root)('linked')).BUILD).toBe('linked'); }); - it('BOUNDARY: a `link:` target whose manifest names something else keeps the refusal', async () => { + it('BOUNDARY: a `link:` key holding a DIFFERENT directory, named otherwise, keeps the refusal', async () => { // Deliberate, and the reason `link:` is not "parsed" into a name: a path // specifier carries no package name for the finder to expect, so there is - // nothing to verify a differing manifest against. The conservative - // direction (refuse, never load the wrong thing) is kept rather than - // guessed at — widening it here would make the finder looser than the - // manifest-name check exists to be. + // nothing on the NAME axis to verify a differing manifest against. // - // ⚠️ This pin asserted the INSTALL wording until #15045. The REFUSAL is - // what #14278 declared and it is unchanged — same kind, same throw; only - // the words changed, because the install this message described was - // already correct. The wording itself is pinned in the #15045 suite below. + // ⚠️ This pin has been re-read twice and its reason has moved twice, so it + // is spelled out rather than inherited. It asserted the INSTALL wording + // until #15045 (which re-worded, and kept the refusal). #17046 then gave + // the fallback a SECOND axis — the declared PATH — under which a `link:` + // whose key IS the declared directory now loads. This fixture is not that: + // `installAs` writes a plain directory at `node_modules/`, while the + // declaration names `../elsewhere`, so BOTH axes are measured and BOTH + // fail. The assertion below reads the location axis's own line out of the + // message, so a future change that silently stopped running it would redden + // here instead of passing for the wrong reason. const root = app('link-mismatch', 'linked-other', 'link:../elsewhere'); installAs(root, 'linked-other', '@fixture/some-other-name', { exports: ESM_ONLY_EXPORTS }, { 'dist/index.js': "export const BUILD = 'other';\n", @@ -1484,6 +1489,9 @@ describe('an aliased install is verified against the name its DECLARATION names const err = await createHostImporter(root)('linked-other').catch((e: unknown) => e); expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); expect((err as Error).message).not.toMatch(/INSTALL problem/); + // The second axis ran, and said what it compared. + expect((err as Error).message).toMatch(/checks\n {2}the LOCATION too — and that did not match/); + expect((err as Error).message).toMatch(/the key resolves to: {4}.*node_modules\/linked-other/); }); it('TIGHTNESS: an alias naming one package does not license a directory holding another', async () => { @@ -1892,13 +1900,15 @@ exports.BUILD = 'cjs'; * pruned, and its `import` target exists. The operator runs `pnpm install`, * nothing changes, and they go hunting for a build that is not broken. * - * ⛔ What this suite does NOT pin, because it was NOT built: the second - * verification axis the card also proposes (comparing - * `realpath(node_modules/)` against the declared location), which would - * make these installs LOAD. That relaxes the finder's accept set and is a - * contract decision; the card stays open for it. The tests below assert the - * opposite — that the refusal still fires on exactly the inputs it fired on - * before. + * ⚠️ #17046 BUILT the second axis this suite was written without, so the + * suite's own fixtures had to move — read this before assuming a `link:` + * fixture here means what it did. `linkedApp` symlinks `node_modules/` AT + * the declared directory, which is now the shape that LOADS; the wording below + * is therefore driven on the residue that still cannot be verified either way, + * and each fixture says which residue it is. ⛔ Nothing here was deleted to + * make room: every #15045 assertion survives, on an input that still reaches + * the text it pins. The load itself is pinned in the #17046 suite below, next + * to the negative controls that keep the refusal. */ describe('a location install whose manifest differs states the LIMIT, not a false remedy (#15045)', () => { const bases: string[] = []; @@ -1911,13 +1921,24 @@ describe('a location install whose manifest differs states the LIMIT, not a fals const ESM_ONLY_EXPORTS = { '.': { import: './dist/index.js' } }; /** - * A REAL location install: `node_modules/` is a SYMLINK to a sibling - * directory, which is what `link:` and a directory `file:` actually produce. + * A REAL location install. Two on-disk shapes, both of which a package + * manager really produces for a path specifier — and after #17046 they are + * NOT the same input, which is why the shape is now a parameter rather than + * an assumption: * - * #14278's fixtures above install a plain directory instead — correct there, - * since the finder reads `node_modules/` and realpaths only afterwards, - * so both shapes exercise one code path. This card is ABOUT the location - * shape, so it drives the on-disk shape an operator would really have. + * - `'symlink'` — `node_modules/` IS the declared directory, reached + * through a symlink. `link:` under pnpm and npm, and `file:` under npm. + * This is what the second axis verifies, so it now LOADS. + * - `'copy'` — `node_modules/` holds an independent COPY, and the + * declared directory sits untouched beside it. MEASURED on pnpm 10.33: + * `file:../x` on a directory routes through the virtual store + * (`.pnpm/@file+..+x/node_modules/`), which realpaths inside + * the host's own `node_modules` and is not the declared path. The location + * axis measures a DIFFERENT directory, so the refusal — and this suite's + * wording — stands. + * + * #14278's fixtures above install a plain directory too; that suite predates + * the distinction and its one location pin now names it explicitly. */ function linkedApp( tag: string, @@ -1925,6 +1946,7 @@ describe('a location install whose manifest differs states the LIMIT, not a fals specifier: string, manifestName: string, exportsField: unknown = ESM_ONLY_EXPORTS, + shape: 'symlink' | 'copy' = 'symlink', ): string { const base = mkdtempSync(join(tmpdir(), `os-loc-${tag}-`)); bases.push(base); @@ -1937,26 +1959,43 @@ describe('a location install whose manifest differs states the LIMIT, not a fals JSON.stringify({ name: 'location-host-fixture', type: 'module', dependencies: { [key]: specifier } }), 'utf8', ); - writeFileSync( - join(linked, 'package.json'), - JSON.stringify({ name: manifestName, version: '0.0.0-fixture', type: 'module', exports: exportsField }), - 'utf8', - ); - writeFileSync(join(linked, 'dist', 'index.js'), `export const BUILD = ${JSON.stringify(manifestName)};\n`, 'utf8'); + const manifest = JSON.stringify({ + name: manifestName, + version: '0.0.0-fixture', + type: 'module', + exports: exportsField, + }); + const entry = `export const BUILD = ${JSON.stringify(manifestName)};\n`; + writeFileSync(join(linked, 'package.json'), manifest, 'utf8'); + writeFileSync(join(linked, 'dist', 'index.js'), entry, 'utf8'); const at = join(root, 'node_modules', ...key.split('/')); mkdirSync(dirname(at), { recursive: true }); - symlinkSync(linked, at, 'dir'); + if (shape === 'symlink') { + symlinkSync(linked, at, 'dir'); + } else { + mkdirSync(join(at, 'dist'), { recursive: true }); + writeFileSync(join(at, 'package.json'), manifest, 'utf8'); + writeFileSync(join(at, 'dist', 'index.js'), entry, 'utf8'); + } return root; } + /** + * The residue this suite's wording still governs after #17046: a location + * declaration whose key is a COPY rather than the declared directory. Both + * axes are measured, both fail, and the message is the one #15045 wrote. + */ + const copiedApp = (tag: string, key: string, specifier: string, manifestName: string): string => + linkedApp(tag, key, specifier, manifestName, ESM_ONLY_EXPORTS, 'copy'); + const refusalFor = async (root: string, spec: string): Promise => (await createHostImporter(root)(spec).catch((e: unknown) => e)) as Error; - it('PRECONDITION: the symlinked ESM-only install reaches the fallback at all', () => { + it('PRECONDITION: the copied ESM-only install reaches the fallback at all', () => { // Without this, everything below could be passing for the wrong reason: - // the CJS resolver must FIND the symlink and refuse on the CONDITION, so - // the fallback inside that catch is what decides. - const root = linkedApp('precondition', 'linked-other', 'link:../elsewhere', '@fixture/some-other-name'); + // the CJS resolver must FIND the directory at the key and refuse on the + // CONDITION, so the fallback inside that catch is what decides. + const root = copiedApp('precondition', 'linked-other', 'file:../elsewhere', '@fixture/some-other-name'); let code: string | undefined; try { createHostRequire(root).resolve('linked-other'); @@ -1969,7 +2008,7 @@ describe('a location install whose manifest differs states the LIMIT, not a fals it('THE CARD: the refusal is UNCHANGED — same kind, same code, still not loaded', async () => { // The half that must not move. A diff that turned this into a load would // have left the card's scope whatever its message said. - const root = linkedApp('refusal', 'linked-other', 'link:../elsewhere', '@fixture/some-other-name'); + const root = copiedApp('refusal', 'linked-other', 'file:../elsewhere', '@fixture/some-other-name'); const err = await refusalFor(root, 'linked-other'); expect(err).toBeInstanceOf(Error); expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); @@ -1979,7 +2018,7 @@ describe('a location install whose manifest differs states the LIMIT, not a fals it('THE CARD: the three false remedies are gone', async () => { // Asserted as ABSENCES of the old message's load-bearing claims, not as // punctuation. Each was measurably false for this shape. - const root = linkedApp('remedies-gone', 'linked-other', 'link:../elsewhere', '@fixture/some-other-name'); + const root = copiedApp('remedies-gone', 'linked-other', 'file:../elsewhere', '@fixture/some-other-name'); const { message } = await refusalFor(root, 'linked-other'); expect(message).not.toMatch(/INSTALL problem, not a declaration problem/); expect(message).not.toMatch(/dependencies never installed/); @@ -1988,7 +2027,7 @@ describe('a location install whose manifest differs states the LIMIT, not a fals }); it('THE CARD: it states the MEASUREMENT — what is installed, and what was expected', async () => { - const root = linkedApp('measurement', 'linked-other', 'link:../elsewhere', '@fixture/some-other-name'); + const root = copiedApp('measurement', 'linked-other', 'file:../elsewhere', '@fixture/some-other-name'); const { message } = await refusalFor(root, 'linked-other'); // The two names it compared, both present, so the operator can see the // mismatch rather than infer it. @@ -1999,49 +2038,61 @@ describe('a location install whose manifest differs states the LIMIT, not a fals }); it('THE CARD: it names the LIMIT — a location specifier carries no name to expect', async () => { - const root = linkedApp('limit', 'linked-other', 'link:../elsewhere', '@fixture/some-other-name'); + const root = copiedApp('limit', 'linked-other', 'file:../elsewhere', '@fixture/some-other-name'); const { message } = await refusalFor(root, 'linked-other'); expect(message).toMatch(/NOT an install problem/); expect(message).toMatch(/names a LOCATION, not a package/); - // The card asked for the git / tarball sentence: they carry no on-disk - // location either and land in exactly this sub-case. - expect(message).toMatch(/tarball URL/); // ⚠️ `pnpm install` IS mentioned, inside the sentence that says it changes // nothing. Pinned as a PRESENCE so nobody later "fixes" the mention by // deleting the one line that stops the operator's reflex. expect(message).toMatch(/re-running `pnpm install`.*change nothing/s); }); + it('THE CARD: the git / tarball sentence is on the spelling that HAS no location', async () => { + // #15045 asked for this sentence and pinned it on a `link:` fixture, + // because at the time every spelling on the list shared one paragraph. + // #17046 split the paragraph — a path specifier now gets the location + // axis's own reading instead — so the sentence is pinned where it is true. + const root = linkedApp('limit-remote', 'located-other', 'https://example.invalid/pkg.tgz', '@fixture/some-other-name'); + const { message } = await refusalFor(root, 'located-other'); + expect(message).toMatch(/NOT an install problem/); + expect(message).toMatch(/names no on-disk location at all/); + expect(message).toMatch(/tarball URL/); + expect(message).toMatch(/re-running `pnpm install`.*change nothing/s); + }); + it('THE CARD: the remedy it prints WORKS — renaming either end makes it load', async () => { // The message tells the operator to make the two names agree. Both ends // are pinned, because a printed remedy nobody measured is the class of // defect this card is about. - const byKey = linkedApp('remedy-key', '@fixture/some-other-name', 'link:../elsewhere', '@fixture/some-other-name'); + const byKey = copiedApp('remedy-key', '@fixture/some-other-name', 'file:../elsewhere', '@fixture/some-other-name'); expect((await createHostImporter(byKey)('@fixture/some-other-name')).BUILD).toBe('@fixture/some-other-name'); - const byManifest = linkedApp('remedy-manifest', 'linked-other', 'link:../elsewhere', 'linked-other'); + const byManifest = copiedApp('remedy-manifest', 'linked-other', 'file:../elsewhere', 'linked-other'); expect((await createHostImporter(byManifest)('linked-other')).BUILD).toBe('linked-other'); }); it('NEGATIVE CONTROL: a `link:` install whose manifest MATCHES the key still loads, silently', async () => { - // The load path is untouched. If this ever reddens, the change stopped - // being a wording change. + // The load path #15045 must not have moved, and #17046 must not have moved + // either: this one is carried by the NAME axis, which is untouched. const root = linkedApp('control', 'linked', 'link:../elsewhere', 'linked'); expect((await createHostImporter(root)('linked')).BUILD).toBe('linked'); }); it('the same wording covers `file:`, a tarball URL and the bare `owner/repo` shorthand', async () => { - // One fact, four spellings: none of them names a package. `file:` and - // `link:` are the two location protocols; a git or tarball URL names no - // on-disk location at all and installs under the key with whatever the - // published manifest carries. - for (const [tag, specifier] of [ - ['file', 'file:../elsewhere'], - ['tarball', 'https://example.invalid/pkg.tgz'], - ['github', 'github:acme/bar'], - ['shorthand', 'acme/bar'], + // One fact, four spellings: none of them names a package, so none of them + // is an install fault. ⚠️ The SHAPE now matters and the table says which, + // because #17046 verifies a path specifier against the directory it names: + // `file:` is driven on the copy shape (the pnpm virtual-store install), + // while the three remote artefacts name no directory at all and are the + // same whatever sits at the key. + for (const [tag, specifier, shape] of [ + ['file', 'file:../elsewhere', 'copy'], + ['tarball', 'https://example.invalid/pkg.tgz', 'symlink'], + ['github', 'github:acme/bar', 'symlink'], + ['shorthand', 'acme/bar', 'symlink'], ] as const) { - const root = linkedApp(tag, 'located-other', specifier, '@fixture/some-other-name'); + const root = linkedApp(tag, 'located-other', specifier, '@fixture/some-other-name', ESM_ONLY_EXPORTS, shape); const { message } = await refusalFor(root, 'located-other'); expect(message, `${specifier} should get the location wording`).toMatch(/NOT an install problem/); expect(message, `${specifier} should not keep the install remedy`).not.toMatch(/INSTALL problem/); @@ -2069,6 +2120,11 @@ describe('a location install whose manifest differs states the LIMIT, not a fals // symlinked mismatch under `^1.0.0` is a registry install that landed // wrong, and `pnpm install` is exactly the remedy for it. A red here would // mean the re-wording leaked out of the location sub-case. + // + // ⭐ After #17046 this is also the tightest control in the suite: the key + // IS a symlink pointing at a real sibling directory, exactly the on-disk + // shape the location axis verifies — and it is still refused, because the + // DECLARATION named no location. A range licenses no path. const root = linkedApp('range-mismatch', '@fixture/plain-range', '^1.0.0', '@fixture/somebody-else'); const { message } = await refusalFor(root, '@fixture/plain-range'); expect(message).toMatch(/INSTALL problem, not a declaration problem/); @@ -2082,3 +2138,290 @@ describe('a location install whose manifest differs states the LIMIT, not a fals expect(message).toMatch(/INSTALL problem, not a declaration problem/); }); }); + +/** + * ── #17046: the SECOND verification axis — a correctly LINKED package loads ─── + * + * #14041's fallback verifies the one directory it consults by NAME: does the + * manifest at `/node_modules/` carry the name the declaration + * promises? A `link:` / `file:` value promises no name, so the KEY stands in + * for one — and a package that is linked exactly as its host asked, whose own + * manifest happens to be named something else, is REFUSED. That is a false + * refusal on a valid setup: nothing is broken, and the operator's only + * recourse is to stop using a supported linking mode. + * + * The fix moves the expectation the same way #14278 moved it — from the KEY to + * what the host ACTUALLY declared — only along the other axis: a PATH instead + * of a NAME. `realpath(node_modules/)` is compared with + * `realpath(resolve(hostRoot, ))`, exactly, both sides + * canonicalised, no basename matching and no case folding. + * + * ⛔ What it is NOT, quoted from #15045's triage because it predicted a dev + * would reach for it: *"skip the check when the specifier is a location. That + * accepts any directory sitting at the key — the looser finder #4719 and + * #14041 exist to prevent — and trades a confidently-wrong remedy for a wrong + * LOAD."* Every NEGATIVE below is that sentence: a directory the host did not + * declare — by name or by path — is still refused. A suite proving only the + * new load would be proving the wrong half. + * + * Three boundaries, deliberate and pinned rather than assumed: + * + * - **remote artefacts are untouched.** `github:acme/bar`, a tarball URL and + * the bare `owner/repo` shorthand name no on-disk location, so there is + * nothing to compare and the refusal stands. + * - **the #13330 leg is untouched.** A dual-published `link:` target still + * resolves through CommonJS and still keeps its `require` entry — pinned by + * the #15044 BOUNDARY above. Extending there would change a load that + * already succeeds; this axis only ever fires inside `hostRequire.resolve`'s + * catch, so nothing that loads today changes. + * - **case-insensitive filesystems are NOT covered by a test**, and the + * comparison deliberately does not fold case. This suite runs on a + * case-sensitive filesystem, so a fixture could only assert the same thing + * the other cases already do. The direction is the safe one: folding would + * ACCEPT a path the host did not write wherever case matters, while + * declining to fold can at worst fail to verify — which is today's refusal, + * not a regression. + */ +describe('a correctly LINKED package is verified by the LOCATION its host declared (#17046)', () => { + const bases: string[] = []; + + afterAll(() => { + for (const dir of bases) rmSync(dir, { recursive: true, force: true }); + }); + + /** The card's exact shape: `import` condition only, no `require`, no `main`. */ + const ESM_ONLY_EXPORTS = { '.': { import: './dist/index.js' } }; + + interface LinkFixture { + /** How the declaration spells the location, resolved against the app root. */ + specifier: string; + /** Manifest `name` of the package at the other end — deliberately NOT the key. */ + manifestName?: string; + /** + * Where `node_modules/` actually points, relative to the base dir. + * Defaults to the real package directory (`'linked'`), i.e. a correct link. + */ + installTarget?: string; + /** Install a COPY at the key instead of a symlink (pnpm's `file:` shape). */ + copy?: boolean; + /** Omit the `node_modules/` entry entirely. */ + absent?: boolean; + exportsField?: unknown; + } + + /** + * A host app that DECLARES `key` with a location specifier, plus a real + * package directory `linked/` beside it and an `alias/` symlink pointing at + * that same directory (so a fixture can declare one spelling and install the + * other, which is what proves the comparison is canonical rather than + * textual). + */ + function linkFixture(tag: string, key: string, fixture: LinkFixture): string { + const { + specifier, + manifestName = '@fixture/named-something-else', + installTarget = 'linked', + copy = false, + absent = false, + exportsField = ESM_ONLY_EXPORTS, + } = fixture; + const base = mkdtempSync(join(tmpdir(), `os-17046-${tag}-`)); + bases.push(base); + const root = join(base, 'app'); + const linked = join(base, 'linked'); + const decoy = join(base, 'decoy'); + mkdirSync(join(root, 'node_modules'), { recursive: true }); + mkdirSync(join(linked, 'dist'), { recursive: true }); + mkdirSync(join(decoy, 'dist'), { recursive: true }); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'linked-host-fixture', type: 'module', dependencies: { [key]: specifier } }), + 'utf8', + ); + const manifest = (name: string): string => + JSON.stringify({ name, version: '0.0.0-fixture', type: 'module', exports: exportsField }); + writeFileSync(join(linked, 'package.json'), manifest(manifestName), 'utf8'); + writeFileSync(join(linked, 'dist', 'index.js'), "export const BUILD = 'the-linked-directory';\n", 'utf8'); + writeFileSync(join(decoy, 'package.json'), manifest(manifestName), 'utf8'); + writeFileSync(join(decoy, 'dist', 'index.js'), "export const BUILD = 'the-decoy';\n", 'utf8'); + // A second NAME for the same directory, so `link:../alias` and a key + // symlinked at `../../linked` still have to compare equal. + symlinkSync(linked, join(base, 'alias'), 'dir'); + if (absent) return root; + const at = join(root, 'node_modules', ...key.split('/')); + mkdirSync(dirname(at), { recursive: true }); + if (copy) { + mkdirSync(join(at, 'dist'), { recursive: true }); + writeFileSync(join(at, 'package.json'), manifest(manifestName), 'utf8'); + writeFileSync(join(at, 'dist', 'index.js'), "export const BUILD = 'the-copy';\n", 'utf8'); + } else { + symlinkSync(join(base, installTarget), at, 'dir'); + } + return root; + } + + const refusalFor = async (root: string, spec: string): Promise => + (await createHostImporter(root)(spec).catch((e: unknown) => e)) as Error; + + it('PRECONDITION: the linked ESM-only install reaches the fallback at all', () => { + // Without this, the load below could be happening for the wrong reason. + // The CJS resolver must FIND the symlink and refuse on the CONDITION, so + // the fallback inside that catch is the thing that decides. + const root = linkFixture('precondition', 'linked-key', { specifier: 'link:../linked' }); + let code: string | undefined; + try { + createHostRequire(root).resolve('linked-key'); + } catch (e) { + code = (e as { code?: string }).code; + } + expect(code).toBe('ERR_PACKAGE_PATH_NOT_EXPORTED'); + }); + + it('THE CARD: a `link:` install whose manifest names something else now LOADS', async () => { + // The false refusal, gone. On `origin/main` this rejects with + // `declared-unresolvable` / MODULE_NOT_FOUND; the directory at the key is + // the one the host's own package.json named, so the host declared it. + const root = linkFixture('loads', 'linked-key', { specifier: 'link:../linked' }); + expect((await createHostImporter(root)('linked-key')).BUILD).toBe('the-linked-directory'); + }); + + it('THE CARD: it loads the DECLARED directory — not merely something', async () => { + // A load is not the assertion; loading the right module is. The fixture + // keeps a `decoy/` beside `linked/` carrying the SAME manifest name and a + // different build marker, so an implementation that resolved by name, or + // by "whatever is nearby", is visible here rather than green. + const root = linkFixture('right-one', 'linked-key', { specifier: 'link:../linked' }); + const loaded = await createHostImporter(root)('linked-key'); + expect(loaded.BUILD).toBe('the-linked-directory'); + expect(loaded.BUILD).not.toBe('the-decoy'); + }); + + it('THE CARD: the comparison is CANONICAL — a symlinked spelling on either side still matches', async () => { + // ⭐ The measurement triage asked for. The declaration names `../alias` + // (itself a symlink to `linked/`), the key points at `../../linked`, and + // the declared path is spelled with a traversal span and a trailing + // separator. Every one of those is a DIFFERENT string for the same + // directory: a raw string comparison fails all four, `resolve()` alone + // fails the symlink, and a basename match would pass things it must not. + const root = linkFixture('canonical', 'linked-key', { + specifier: 'link:../linked/../alias/', + installTarget: 'linked', + }); + expect((await createHostImporter(root)('linked-key')).BUILD).toBe('the-linked-directory'); + }); + + it('`file:` and `portal:` are location protocols too, and verify the same way', async () => { + // npm installs a directory `file:` dependency AS a symlink, which is this + // shape. (pnpm routes `file:` through its virtual store instead — a copy, + // covered by the negative below and by the #15045 suite's wording pins.) + for (const specifier of ['file:../linked', 'portal:../linked']) { + const root = linkFixture(`proto-${specifier.slice(0, 4)}`, 'linked-key', { specifier }); + expect((await createHostImporter(root)('linked-key')).BUILD, specifier).toBe('the-linked-directory'); + } + }); + + it('NEGATIVE: a directory at the key that the host did NOT declare is still refused', async () => { + // ⛔ The thing this must not become. The key holds a COPY — a real package, + // named the same as the declared one's manifest, sitting exactly where CJS + // resolution would happily take it — and the declared directory is + // elsewhere. Neither axis ties it to the declaration, so it is refused. + // This is also pnpm's measured `file:` virtual-store shape. + const root = linkFixture('unnamed-copy', 'linked-key', { specifier: 'file:../linked', copy: true }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect((err as unknown as { code?: string }).code).toBe('MODULE_NOT_FOUND'); + expect(err.message).toMatch(/the LOCATION too — and that did not match/); + }); + + it('NEGATIVE: a link pointing SOMEWHERE ELSE than the declaration names is refused', async () => { + // The stale / redirected link. `decoy/` is a real package directory with + // the same manifest name, so only the PATH distinguishes it — which is the + // whole point of the axis. + const root = linkFixture('wrong-target', 'linked-key', { + specifier: 'link:../linked', + installTarget: 'decoy', + }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/the LOCATION too — and that did not match/); + }); + + it('NEGATIVE: a remote artefact declaration gains nothing — there is no location to compare', async () => { + // The declared boundary. Even with the key symlinked at a perfectly good + // package directory, `github:` / a tarball URL / the `owner/repo` + // shorthand name no on-disk location, so the finder has only the KEY and + // the refusal stands — unchanged by this card. + for (const specifier of ['github:acme/bar', 'https://example.invalid/pkg.tgz', 'acme/bar']) { + const root = linkFixture(`remote-${specifier.slice(0, 4)}`, 'linked-key', { specifier }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err), specifier).toBe('declared-unresolvable'); + expect(err.message, specifier).toMatch(/names no on-disk location at all/); + } + }); + + it('NEGATIVE: a plain RANGE licenses no path, however correct the link looks', async () => { + // The tightness control #14278 wrote for the name axis, re-run for this + // one: the on-disk shape is IDENTICAL to the loading case above — a + // symlink at the key pointing at a real package directory — and it is + // refused, because the declaration named no location. The predicate is on + // the DECLARATION, never on what is found. + const root = linkFixture('plain-range', 'linked-key', { specifier: '^1.0.0' }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/INSTALL problem, not a declaration problem/); + }); + + it('NEGATIVE: an `npm:` alias still names a PACKAGE, and a differing directory stays refused', async () => { + // The two axes do not blur into one another: an alias declaration names a + // name, so the name is what is checked, and the location axis never runs. + const root = linkFixture('alias', 'linked-key', { specifier: 'npm:@fixture/declared@1' }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/INSTALL problem, not a declaration problem/); + }); + + it('NEGATIVE: a declared location with NOTHING at the key does not verify itself', async () => { + // ⛔ The `undefined === undefined` trap, pinned. The declared directory + // exists and is a perfectly good package; `node_modules/` does not + // exist at all. Two unresolvable sides must not compare equal. + const root = linkFixture('nothing-installed', 'linked-key', { specifier: 'link:../linked', absent: true }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/INSTALL problem, not a declaration problem/); + }); + + it('NEGATIVE: a `link:` whose declared directory does not exist is refused', async () => { + // The other half of the same trap: the key IS a real package directory and + // the declared path resolves to nothing. + const root = linkFixture('declared-gone', 'linked-key', { + specifier: 'link:../no-such-directory', + copy: true, + }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/does not exist/); + }); + + it('NEGATIVE: a `file://` URL spelling is DECLINED, not guessed at', async () => { + // Documented decline: the remainder after `file:` is not a path, and a + // second grammar (authority, percent-decoding) is not parsed on the way to + // licensing a directory. Declining costs a load that was already refused. + const root = linkFixture('file-url', 'linked-key', { specifier: 'file:///nowhere/linked' }); + const err = await refusalFor(root, 'linked-key'); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect(err.message).toMatch(/names no on-disk location at all/); + }); + + it('the load stays inside the FALLBACK — a package with a `require` entry never reaches it', async () => { + // The property that makes this strictly additive: the axis fires only in + // `hostRequire.resolve`'s catch. A dual-published linked package resolves + // through CommonJS and keeps today's behaviour, which the #15044 BOUNDARY + // above pins from the other side (#13330's leg is deliberately untouched). + const root = linkFixture('dual', 'linked-key', { + specifier: 'link:../linked', + exportsField: { '.': { require: './dist/index.cjs', import: './dist/index.js' } }, + }); + writeFileSync(join(root, 'node_modules', 'linked-key', 'dist', 'index.cjs'), 'module.exports={BUILD:"cjs"};\n', 'utf8'); + expect(createHostRequire(root).resolve('linked-key')).toMatch(/index\.cjs$/); + }); +}); diff --git a/packages/types/src/node.ts b/packages/types/src/node.ts index e9c6c4cde9..67bebb3229 100644 --- a/packages/types/src/node.ts +++ b/packages/types/src/node.ts @@ -294,9 +294,11 @@ export function isDeclaredByHost(specifier: string, hostRoot?: string): boolean * resolve. Remedy: fix the INSTALL. Re-reading the manifest is wasted effort. * ⚠️ One sub-case under this kind is NOT an install problem and does not say * it is (#15045): a `link:` / `file:` (or git / tarball) declaration names a - * LOCATION, so the fallback has only the KEY to expect, and a linked manifest - * naming something else is refused with - * {@link unverifiableLocationMessage}'s wording instead. The KIND is shared + * LOCATION rather than a package, so the fallback cannot verify the directory + * by NAME. #17046 gave it the second axis — the declared path — so a + * correctly linked package now LOADS; what still lands here is the residue + * where neither axis ties the directory to the declaration, refused with + * {@link unverifiableLocationMessage}'s wording. The KIND is shared * deliberately — the refusal, the `MODULE_NOT_FOUND` code and every consumer * branch are unchanged; minting a fourth kind would widen a published union * for a wording fix. ⛔ A consumer that re-words this kind LOCALLY instead of @@ -644,14 +646,18 @@ const ALIAS_DECLARATION_PROTOCOLS = [ * refuses a directory holding another. * * Anything that does not parse as a bare package name yields no expectation to - * move to, so the key stays and the pre-#14278 refusal is kept: a `workspace:` - * range, a `link:` / `file:` location, an alias value carrying a subpath, a - * malformed value. Deliberate — {@link packageNameFromSpecifier} is the one - * authority on what a package name is here, and its own documentation blesses - * the aliased declaration shape. On the #13330 leg that residue is a load - * rather than a refusal: a `link:` target whose manifest names something else - * keeps today's `require`-condition entry, unchanged by #15044 and pinned as - * such. + * move to, so the key stays: a `workspace:` range, a `link:` / `file:` + * location, an alias value carrying a subpath, a malformed value. Deliberate — + * {@link packageNameFromSpecifier} is the one authority on what a package name + * is here, and its own documentation blesses the aliased declaration shape. + * + * ⚠️ "The key stays" is a statement about THIS axis only. On the #14041 + * fallback leg a location specifier now gets a SECOND one + * ({@link declaredLocationAxis}, #17046): the host named a directory, so the + * directory is what gets verified when the name cannot be. On the #13330 leg + * the residue is still a load rather than a refusal: a `link:` target whose + * manifest names something else keeps today's `require`-condition entry, + * unchanged by #15044 and by #17046, and pinned as such. */ function declaredManifestName(declaration: HostDeclaration): string { const { packageName, specifier } = declaration; @@ -682,11 +688,12 @@ function declaredManifestName(declaration: HostDeclaration): string { * made, so a mismatch is the finder's declared limit and NOT an install fault * — which is the whole difference between the two messages below. * - * ⚠️ Read for WORDING only. It moves no expectation and licenses no directory: - * {@link hostInstalledPackageDir} refuses exactly what it refused before, and - * the second verification axis the card names (comparing - * `realpath(node_modules/)` against the declared location, which WOULD - * make these load) is deliberately not built here. + * ⚠️ Read for WORDING only. By itself it moves no expectation and licenses no + * directory: it is what separates {@link unresolvableMessage}'s INSTALL + * remedies from {@link unverifiableLocationMessage}'s statement of the limit. + * The SECOND verification axis (#17046) reads a different, strictly narrower + * list — {@link LOCATION_DECLARATION_PREFIXES} — because only some of these + * name a directory there is anything to compare against. * * An unrecognised spelling falls out as "the key is a promise" and keeps * today's INSTALL wording — the conservative direction, matching @@ -721,6 +728,155 @@ function declarationNamesNoPackage(declaration: HostDeclaration): boolean { return specifier.indexOf('/') > 0 && specifier.indexOf(':') === -1; } +/** + * ── #17046: the SECOND verification axis — the declaration names a DIRECTORY ── + * + * The strict subset of {@link NAMELESS_DECLARATION_PREFIXES} whose value is a + * FILESYSTEM PATH the host itself wrote. Everything else on that list — a git + * or tarball URL, the bare `owner/repo` shorthand — names a remote artefact + * and no on-disk location at all, so there is nothing here for it: those keep + * the refusal, unchanged, and {@link unverifiableLocationMessage} keeps saying + * so. + * + * ⚠️ This list exists because the two questions are NOT the same question. + * `NAMELESS_…` asks *"does the declaration name a package?"* (a WORDING + * question, #15045); this one asks *"does the declaration name a directory I + * can compare against?"* — the question that decides whether a load happens. + * Merging them would license `github:acme/bar` to be verified against a path + * nobody wrote. + */ +const LOCATION_DECLARATION_PREFIXES = ['link:', 'file:', 'portal:'] as const; + +/** + * What {@link declaredLocationAxis} measured — kept as a record rather than a + * boolean because the REFUSAL has to be able to say what it compared, exactly + * as #15045 made the name axis say what it read. + */ +interface DeclaredLocationAxis { + /** The declared path, resolved against `hostRoot` — as written, not canonicalised. */ + declaredPath: string; + /** `realpath(declaredPath)`, or `undefined` when it does not resolve. */ + declaredReal: string | undefined; + /** `realpath(/node_modules/)`, or `undefined` when it does not resolve. */ + installedReal: string | undefined; + /** Both sides canonicalised, and the SAME directory. */ + verified: boolean; +} + +/** + * `realpath(path)`, or `undefined` when it cannot be canonicalised at all. + * + * ⚠️ Deliberately NOT the tolerant catch {@link hostInstalledPackageDir} uses, + * which falls back to the uncanonicalised path so an exotic `realpath` failure + * cannot un-install a package it already read a manifest out of. Here the + * canonical form IS the evidence: falling back to the raw string would make + * the comparison below a raw-string comparison in disguise, which is the one + * thing #17046's triage ruled out by name. + */ +function canonicalPath(path: string): string | undefined { + try { + return realpathSync(path); + } catch { + return undefined; + } +} + +/** + * The `link:` / `file:` / `portal:` path the declaration names, resolved + * against the host root — or `undefined` when the declaration names no + * directory. + * + * `file://…` URL spellings are declined on purpose: the remaining `//…` is not + * a path, percent-decoding and the optional authority make it a second + * grammar, and every unparsed spelling simply keeps today's refusal. Declining + * costs a load that was already refused; guessing would license a directory + * nobody named. + */ +function declaredLocationPath(declaration: HostDeclaration): string | undefined { + const { specifier, hostRoot } = declaration; + if (specifier === undefined) return undefined; + const prefix = LOCATION_DECLARATION_PREFIXES.find((p) => specifier.indexOf(p) === 0); + if (prefix === undefined) return undefined; + const value = specifier.slice(prefix.length); + if (value === '' || value.indexOf('//') === 0) return undefined; + return resolve(hostRoot, value); +} + +/** + * Is `/node_modules/` the very directory the host's declaration + * NAMED (#17046)? + * + * ## Why this is a second axis and not a hole in the first + * + * The #14041 fallback verifies one directory by asking whether its manifest + * carries the name the declaration promises ({@link declaredManifestName}). + * A `link:` / `file:` value promises no name, so that axis has nothing to + * check and the KEY stands in for it — which refuses a *correctly linked* + * package whose manifest happens to be named something else. That refusal is a + * false one: the host DID name this directory, in its own manifest, in the + * same authoring act the declaration gate reads. + * + * So the expectation moves the same way #14278 moved it — from the KEY to + * *what the host actually declared* — only along the other axis: a PATH + * instead of a NAME. ⛔ It is emphatically NOT "skip the check when the + * specifier is a location", which would accept any directory sitting at the + * key and trade a confidently-wrong remedy for a wrong LOAD. Both axes still + * end at the host's own `package.json`, and a directory the host did not + * declare is refused by both. + * + * ## The comparison, and why each part of it is what it is + * + * ⛔ NOT a raw string comparison, and ⛔ not a basename match — the two shapes + * `packages/cli`'s own `isProcessEntry` (`src/utils/invocation.ts`) ruled out + * for the same reason, where #10086 found basename matching in the wild and + * PR #10084 pinned the symlink leg with a real symlink fixture. The DISCIPLINE + * is reused here; nothing is imported, since `packages/types` sits below + * `packages/cli`. Both sides are canonicalised with `realpathSync` and + * compared EXACTLY: + * + * - **symlinks** — the whole point. `link:` installs `node_modules/` AS a + * symlink, so the left side is a link and the right side is its target; + * without `realpath` they never compare equal. Chains, `..` spans and + * trailing separators all collapse here too, which is why `resolve()` alone + * is not enough on either side. + * - **pnpm's store layout** — MEASURED, pnpm 10.33: `link:../x` symlinks the + * key straight at `../x`, so it verifies; `file:../x` on a DIRECTORY does + * not — pnpm routes it through the virtual store + * (`.pnpm/@file+..+x/node_modules/`), a hard-linked COPY whose + * realpath is inside the host's own `node_modules` and is not the declared + * path. That shape keeps today's refusal, deliberately: reading the store's + * encoded directory name to recover the origin would be parsing a package + * manager's private layout, and accepting "anything under `node_modules`" + * is the forbidden relaxation above. npm's `file:` (a symlink) verifies. + * - **case-insensitive filesystems** — NOT case-folded, and that is the safe + * direction rather than an oversight. Folding would accept a path the host + * did not write wherever the filesystem is case-SENSITIVE; declining to fold + * can only fail to verify a link whose declared spelling differs in case + * from the on-disk entry, and failing to verify is exactly today's + * behaviour. `realpathSync` (not `.native`) is used because the sibling read + * in {@link hostInstalledPackageDir} uses it: comparing two different + * canonicalisers is its own defect class. + * + * `undefined === undefined` is not a match: an unresolvable side answers + * `verified: false`, so a declaration pointing at nothing and a key holding + * nothing do not verify each other. + */ +function declaredLocationAxis( + declaration: HostDeclaration, + installedAt: string, +): DeclaredLocationAxis | undefined { + const declaredPath = declaredLocationPath(declaration); + if (declaredPath === undefined) return undefined; + const declaredReal = canonicalPath(declaredPath); + const installedReal = canonicalPath(installedAt); + return { + declaredPath, + declaredReal, + installedReal, + verified: installedReal !== undefined && installedReal === declaredReal, + }; +} + /** * The directory of the package named `manifestName` that owns `resolvedFile`. * @@ -839,7 +995,11 @@ function esmEntryForDeclared( * ({@link hasInvalidExportsSubpathSegments}): a subpath carrying `''`, * `.`, `..` or `node_modules` segments is refused exactly as both of * Node's resolvers refuse it — the one validation the specifier has NOT - * already passed by the time it reaches this catch (#14271 review). + * already passed by the time it reaches this catch (#14271 review); + * - and that directory is VERIFIED against the host's own declaration + * before anything is loaded out of it — by the manifest NAME the + * declaration promises, or by the PATH it names (#17046). CJS resolution + * asks neither question, which is what "strictly tighter" means here. * * `import.meta.resolve` with a parent URL is NOT the mechanism, on the same * measurement the #10943 note below records: without @@ -869,11 +1029,23 @@ type DeclaredCjsResolveFallback = | { outcome: 'absent' } /** * Present at the key, holding a package named something ELSE, under a - * declaration that names no package to expect (#15045). Refused exactly as - * `absent` is — same kind, same throw — but it is a different measurement and - * gets its own wording: nothing about the install is broken. + * declaration that names no package to expect (#15045) — and, when that + * declaration DID name a directory, not that directory either (#17046). + * Refused exactly as `absent` is — same kind, same throw — but it is a + * different measurement and gets its own wording: nothing about the install + * is broken. + * + * `location` is the second axis's own reading, `undefined` when the + * declaration named no directory to read (a git or tarball URL). It is + * carried so the refusal can state what it compared instead of asserting a + * limit it no longer has. */ - | { outcome: 'unverifiable-location'; packageDir: string; installedName: string } + | { + outcome: 'unverifiable-location'; + packageDir: string; + installedName: string; + location: DeclaredLocationAxis | undefined; + } /** Rescued: the `import`-condition entry to load. */ | { outcome: 'entry'; entry: string } /** Present, and its manifest names a runtime target — the FILES are the problem. */ @@ -941,27 +1113,54 @@ function manifestNameAt(dir: string): string | undefined { } } +/** Where the fallback looks, and the only place it looks: `node_modules/`. */ +function hostNodeModulesEntry(declaration: HostDeclaration): string { + const { packageName, hostRoot } = declaration; + return join(hostRoot, 'node_modules', ...packageName.split('/')); +} + /** - * The one directory the fallback finder consults, verified to hold the - * declared package (a `package.json` whose `name` is the one - * {@link declaredManifestName} reads out of the host's declaration) and then - * realpath'd — under pnpm the link target is + * The one directory the fallback finder consults, verified to be the declared + * package's install and then realpath'd — under pnpm the link target is * `.pnpm/@/node_modules/`, the directory the package's own * transitive imports resolve against, exactly as the CJS resolver's realpath * answer behaves on the succeeding path. + * + * ⚠️ TWO verification axes, either of which is sufficient, and BOTH of which + * are read out of the host's own `package.json` (#17046): + * + * 1. **the NAME** (#14041, moved by #14278) — the manifest at the key carries + * the name {@link declaredManifestName} reads out of the declaration; + * 2. **the LOCATION** ({@link declaredLocationAxis}) — the declaration names a + * directory and the key IS that directory, canonically. + * + * The second exists because a `link:` / `file:` declaration promises no name, + * so axis 1 falls back to the KEY and refuses a correctly linked package whose + * manifest is named something else — a false refusal on a valid setup, where + * the operator's only recourse is to stop using a supported linking mode. + * + * ⛔ The axes are alternatives, never a weakening: a directory the host + * declared NEITHER by name NOR by path is refused exactly as before, and no + * specifier that names no directory gains anything. The finder therefore stays + * strictly tighter than the CommonJS resolution it backs up, which accepts + * whatever sits at the key without asking either question. */ function hostInstalledPackageDir(declaration: HostDeclaration): string | undefined { - const { packageName, hostRoot } = declaration; - const linked = join(hostRoot, 'node_modules', ...packageName.split('/')); + const linked = hostNodeModulesEntry(declaration); // Unreadable, unparseable, or named something else — all `undefined`, exactly // as before #15045; the CALLER is what now distinguishes them, and only to // pick the wording. - if (manifestNameAt(linked) !== declaredManifestName(declaration)) return undefined; + const namedAsDeclared = manifestNameAt(linked) === declaredManifestName(declaration); + if (!namedAsDeclared && declaredLocationAxis(declaration, linked)?.verified !== true) { + return undefined; + } try { return realpathSync(linked); } catch { // The manifest read above already succeeded through this path; an exotic - // realpath failure does not un-install the package. + // realpath failure does not un-install the package. (Unreachable via the + // location axis, which is `verified` only when this same realpath just + // succeeded.) return linked; } } @@ -971,21 +1170,28 @@ function declaredCjsResolveFallback( specifier: string, declaration: HostDeclaration, ): DeclaredCjsResolveFallback { - const { packageName, hostRoot } = declaration; + const { packageName } = declaration; const packageDir = hostInstalledPackageDir(declaration); if (packageDir === undefined) { // #15045: the finder has REFUSED. Re-read the one directory it consulted so // the failure can say which of the two absences it measured. A cold error // path that was already about to build a multi-line message, so the second // read costs nothing anyone can observe. - const linked = join(hostRoot, 'node_modules', ...packageName.split('/')); + const linked = hostNodeModulesEntry(declaration); const installedName = manifestNameAt(linked); if ( installedName !== undefined && installedName !== packageName && declarationNamesNoPackage(declaration) ) { - return { outcome: 'unverifiable-location', packageDir: linked, installedName }; + // #17046: BOTH axes have now failed, so the message says so — carrying + // the location axis's own measurement when there was one to make. + return { + outcome: 'unverifiable-location', + packageDir: linked, + installedName, + location: declaredLocationAxis(declaration, linked), + }; } return { outcome: 'absent' }; } @@ -1034,14 +1240,25 @@ function declaredCjsResolveFallback( /** * The wording for {@link DeclaredCjsResolveFallback} `unverifiable-location` * (#15045) — a `link:` / `file:` (or git / tarball) install whose linked - * manifest names something other than the key. - * - * The refusal it explains is unchanged and deliberate; what changed is that it - * no longer prescribes {@link unresolvableMessage}'s remedies, every one of - * which is measurably false here: the package IS on disk, so it was neither - * "never installed" nor pruned away, and its `import` target exists. An - * operator handed those runs `pnpm install`, watches nothing change, and then - * goes looking for a build that is not broken. + * manifest names something other than the key, and which #17046's location + * axis could not tie to the declaration either. + * + * The refusal it explains is deliberate; what #15045 changed is that it no + * longer prescribes {@link unresolvableMessage}'s remedies, every one of which + * is measurably false here: the package IS on disk, so it was neither "never + * installed" nor pruned away, and its `import` target exists. An operator + * handed those runs `pnpm install`, watches nothing change, and then goes + * looking for a build that is not broken. + * + * ⚠️ #17046 NARROWED what reaches this text, so the text had to move with it. + * It used to be able to say the KEY was all the finder had; that is no longer + * true whenever the declaration names a directory, because + * {@link declaredLocationAxis} then compares one. What survives here is the + * residue: a remote artefact (no location to compare), or a location that was + * compared and came back DIFFERENT — pnpm's `file:` virtual-store copy being + * the measured example. Both facts are now stated rather than assumed, because + * a message asserting a limit the finder no longer has is the same defect + * #15045 removed. * * The closing remedy is one fact stated from both ends, and it was MEASURED, * not reasoned: make the key and the linked manifest's `name` agree — rename @@ -1059,12 +1276,29 @@ function declaredCjsResolveFallback( */ function unverifiableLocationMessage( declaration: HostDeclaration, - found: { packageDir: string; installedName: string }, + found: { packageDir: string; installedName: string; location: DeclaredLocationAxis | undefined }, cause: unknown, ): string { const { packageName, hostRoot, field, specifier } = declaration; - const { packageDir, installedName } = found; + const { packageDir, installedName, location } = found; const detail = cause instanceof Error ? cause.message : String(cause); + // The second axis, reported only when there WAS one to run. Its two lines are + // the two paths that were compared, canonically — the same "say what you + // measured" the name lines above owe (#17046). + const locationLines = + location === undefined + ? ' What it IS: this declaration names no on-disk location at all — a git or a\n' + + ' tarball URL (github:owner/repo, https://.../pkg.tgz) installs under the key with\n' + + ' whatever the published manifest carries, so the KEY is the only thing this finder\n' + + ' has to check against, and the manifest above is not it.\n' + : ' What it IS: a "link:" / "file:" declaration names a LOCATION, not a package, so\n' + + ' the manifest at the other end may carry any name. This finder therefore checks\n' + + ' the LOCATION too — and that did not match either:\n' + + ` the declaration names: ${location.declaredReal ?? `${location.declaredPath} (does not exist)`}\n` + + ` the key resolves to: ${location.installedReal ?? `${packageDir} (does not resolve)`}\n` + + ' A `file:` directory install under pnpm lands in the virtual store rather than at\n' + + ' the declared path, and reaches this text for exactly that reason; `link:` points\n' + + ' at the declared directory and is verified by it.\n'; return ( `Cannot load module '${packageName}': the host app DECLARES it ` + `(${field}: ${JSON.stringify(specifier)}), a package IS installed at that key, and ` + @@ -1076,15 +1310,11 @@ function unverifiableLocationMessage( '\n This is NOT an install problem, and NOT a declaration problem — the package is\n' + ' on disk and the declaration is right, so re-running `pnpm install`, un-pruning a\n' + ' deploy and rebuilding a dist all change nothing here.\n' + - ' What it IS: a "link:" / "file:" declaration names a LOCATION, not a package. The\n' + - ' manifest at the other end may carry any name, and the specifier holds none for\n' + - ' this finder to expect, so the KEY is all it has to check against. A git or\n' + - ' tarball URL (github:owner/repo, https://.../pkg.tgz) names no on-disk location\n' + - ' either and lands here the same way.\n' + + locationLines + ' The refusal is deliberate: this fallback stays strictly tighter than the\n' + ' CommonJS resolution it backs up, and will not load a directory it cannot tie to\n' + - ' the declaration. Only a package publishing no `require` condition reaches it at\n' + - ' all, so nothing that loads today is affected either way.\n' + + ' the declaration — by NAME or by LOCATION. Only a package publishing no `require`\n' + + ' condition reaches it at all, so nothing that loads today is affected either way.\n' + ' What DOES change it — make the two names AGREE, from whichever end you own:\n' + ` • declare the linked package under its own name: key ${JSON.stringify(installedName)},\n` + ' pointing at the same location, and import it under that name\n' + From 3711838529f80aba629b3cd25d72ce0e2c39d38d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 20:16:31 +0000 Subject: [PATCH 2/2] fix(cli): defer the multi-org declared-unresolvable remedy to the cause Also updates content/docs/deployment/tenancy-modes.mdx for the two shapes the declared-unresolvable kind now covers, and adds both changesets. Claude-Session: https://claude.ai/code/session_01DapQyvYrFb1MxSYe7BL2nt Co-authored-by: Claude --- .../link-finder-declared-location-axis.md | 13 ++++++ .changeset/serve-org-remedy-defers.md | 9 ++++ content/docs/deployment/tenancy-modes.mdx | 22 +++++++++- ...rve-organizations-message-spelling.test.ts | 43 ++++++++++++++++--- packages/cli/src/commands/serve.ts | 28 +++++++++--- 5 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 .changeset/link-finder-declared-location-axis.md create mode 100644 .changeset/serve-org-remedy-defers.md diff --git a/.changeset/link-finder-declared-location-axis.md b/.changeset/link-finder-declared-location-axis.md new file mode 100644 index 0000000000..adf64377a0 --- /dev/null +++ b/.changeset/link-finder-declared-location-axis.md @@ -0,0 +1,13 @@ +--- +'@objectstack/types': minor +--- + +Host importer: a `link:` / `file:` install is now verified by the LOCATION the app declared, so a correctly linked package loads instead of being refused. + +The ESM fallback finder (`createHostImporter`) verifies the one directory it consults — `/node_modules/` — against what the host's own `package.json` declares. Until now it could only do that by NAME, and a `link:` / `file:` value promises no name, so the KEY stood in for one: a package linked exactly as the app asked, whose own manifest happens to be named something else, was refused with `declared-unresolvable` / `MODULE_NOT_FOUND`. Nothing was broken, and the only way out was to stop using a supported linking mode. + +Such a declaration does name something checkable — a directory — so the finder now checks that too: `realpath(node_modules/)` against `realpath(resolve(hostRoot, ))`, both sides canonicalised, compared exactly (no basename matching, no case folding). If they are the same directory, the host declared it and it loads. + +This is a second verification axis, not a looser first one. A directory the app declared neither by name nor by path is refused exactly as before, and the finder stays strictly tighter than the CommonJS resolution it backs up, which asks neither question. Unchanged: a plain version range licenses no path; an `npm:` alias is still checked by name; `github:` / tarball URLs and the bare `owner/repo` shorthand name no on-disk location, so they gain nothing; a package that publishes a `require` condition never reaches this fallback at all, so no load that succeeds today changes. + +Measured on pnpm 10.33: `link:` symlinks the key at the declared directory and verifies; a `file:` directory install routes through pnpm's virtual store (a copy), so it does not, and keeps today's refusal. The refusal's text now states what the location check compared instead of asserting a limit the finder no longer has. diff --git a/.changeset/serve-org-remedy-defers.md b/.changeset/serve-org-remedy-defers.md new file mode 100644 index 0000000000..c9ddb300ab --- /dev/null +++ b/.changeset/serve-org-remedy-defers.md @@ -0,0 +1,9 @@ +--- +'@objectstack/cli': patch +--- + +`serve`: the multi-org runtime's stage-1 refusal no longer prints its own install remedy for a `declared-unresolvable` failure — it defers to the importer's message, which the same refusal already prints as its `cause:` line. + +Driven on both shapes that kind covers, the minted bullet ("Repair the INSTALL … run `pnpm install`, check that a production prune did not drop it, and that its dist is actually built") was wrong twice over. For a genuinely broken install it repeated, word for word, the three remedies the cause line four lines below already carried. For a location install the finder cannot tie to the declaration, the cause says outright that re-running `pnpm install`, un-pruning a deploy and rebuilding a dist all change nothing — so one screen contradicted itself. + +The arm now says only what it uniquely knows (the app DOES declare the package, so re-reading `package.json` will not help) and names the cause as the authority on the remedy — the same deferral the `declared-no-loadable-entry` arm has had since it landed. diff --git a/content/docs/deployment/tenancy-modes.mdx b/content/docs/deployment/tenancy-modes.mdx index 03296762ab..45d2360350 100644 --- a/content/docs/deployment/tenancy-modes.mdx +++ b/content/docs/deployment/tenancy-modes.mdx @@ -257,8 +257,26 @@ So, three ways out: - **set `OS_ALLOW_DEGRADED_TENANCY=1`** to boot anyway in an explicitly degraded single-org state. -If the package is already declared but the install is broken, the message says so -and tells you to repair the install rather than re-read `package.json`. +If the package is already declared, the remedy bullet says only that — the +declaration is not the problem — and hands the remedy to the `cause:` line +below it, which is the one thing that measured what actually went wrong. There +are two shapes of that, and they want opposite actions: + +- **the install is broken** — never installed, pruned away by a filtered deploy, + or pointing at a dist that was never built. The cause names all three and + tells you to repair the install rather than re-read `package.json`. +- **the install is fine and the runtime still could not be verified** — the app + declares the package under a `link:` / `file:` path, or a git or tarball URL, + and the package sitting at that key carries a different `name` in its own + `package.json`. This is neither an install nor a declaration fault, and the + cause says so outright: re-running `pnpm install` changes nothing. A `link:` + install pointing at exactly the directory the declaration names is verified by + that path and loads normally; what reaches this message is the residue where + neither the name nor the path ties the directory to the declaration — a + `file:` directory install under pnpm (which lands in the virtual store rather + than at the declared path), or a URL that names no on-disk location at all. + The remedy the cause prints is to make the two names agree, from whichever end + you own. ### A present-but-refusing runtime is a different failure diff --git a/packages/cli/src/commands/serve-organizations-message-spelling.test.ts b/packages/cli/src/commands/serve-organizations-message-spelling.test.ts index f374c71acf..0266964844 100644 --- a/packages/cli/src/commands/serve-organizations-message-spelling.test.ts +++ b/packages/cli/src/commands/serve-organizations-message-spelling.test.ts @@ -157,20 +157,49 @@ const DECLARED: HostDeclaration = { }; const UNDECLARED: HostDeclaration = { packageName: PKG, hostRoot: '/srv/app', declared: false }; -const remedyDeclared = () => formatOrganizationsInstallRemedy('declared-unresolvable', DECLARED, '/srv/app'); -const remedyUndeclared = () => formatOrganizationsInstallRemedy('undeclared', UNDECLARED, '/srv/app'); -const remedyNoLoadableEntry = () => - formatOrganizationsInstallRemedy('declared-no-loadable-entry', DECLARED, '/srv/app'); +const remedyDeclared = () => formatOrganizationsInstallRemedy('declared-unresolvable', DECLARED); +const remedyUndeclared = () => formatOrganizationsInstallRemedy('undeclared', UNDECLARED); +const remedyNoLoadableEntry = () => formatOrganizationsInstallRemedy('declared-no-loadable-entry', DECLARED); describe('serve — the multi-org runtime name an operator READS comes from the declaration (#12151)', () => { - it('site 1 — the "install is broken" remedy names it, with the spacing intact', () => { - // The `declared-unresolvable` branch: the app's package.json IS correct and - // the install is what broke (#4719). One of the two an operator ACTS on. + it('site 1 — the "the declaration is not the problem" remedy names it, with the spacing intact', () => { + // The `declared-unresolvable` branch: the app's package.json IS correct + // (#4719). One of the two an operator ACTS on. expect(lines(remedyDeclared())[0]).toBe( ` • this app DECLARES ${PKG} (dependencies: "^1.2.3") — the`, ); }); + it('site 1b — the `declared-unresolvable` remedy DEFERS too; it mints no install advice (#17046)', () => { + // ── The arm that used to fire, quoted so the flip is legible ────────── + // + // Repair the INSTALL in /srv/app: run `pnpm install`, check that a + // production prune did not drop it, and that its dist is actually built — or + // + // DRIVEN on both shapes this one kind covers, that text was wrong twice: + // for a genuinely broken install it repeats, word for word, the three + // remedies `unresolvableMessage` already prints in the `cause:` line four + // lines below; and for #15045's location sub-case — narrowed by #17046 but + // NOT removed, since pnpm's `file:` virtual-store copy and every git / + // tarball declaration still reach it — the same `cause:` says outright + // that re-running `pnpm install`, un-pruning and rebuilding change + // nothing. One screen contradicting itself, the #14270 class. + const rendered = plain(remedyDeclared()); + expect(rendered).not.toContain('Repair the INSTALL'); + expect(rendered).not.toContain('pnpm install'); + expect(rendered).not.toContain('production prune'); + expect(rendered).not.toContain('dist is actually built'); + // ⛔ Nor may it drift into the OTHER arm's instruction. + expect(rendered).not.toContain("declare it in the app's package.json"); + // What it says instead: the declaration is fine, and the cause below owns + // the remedy — the same DEFERRAL site 2b pins for the sibling kind. + expect(rendered).toContain('declaration is NOT the problem'); + expect(rendered).toContain('the cause below names the'); + expect(rendered).toContain('authority on'); + // It still chains into the `Fix one of:` list the fatal assembles. + expect(rendered.endsWith(' — or\n')).toBe(true); + }); + it('site 2 — the "add it to THIS APP" remedy names it, with the spacing intact', () => { // The other instruction an operator acts on: the app never declared it. expect(lines(remedyUndeclared())[0]).toBe( diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index b3aaf19515..f48928e204 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -3770,7 +3770,6 @@ export default class Serve extends Command { const remedy = formatOrganizationsInstallRemedy( hostImportFailureKind(orgErr), declaration, - hostRoot, ); console.error( chalk.red(formatOrganizationsAbsentFatal(tenancyPosture, remedy, cause)), @@ -5404,9 +5403,26 @@ export function formatI18nLoadDiagnostic(pkg: string, err: unknown): string { * * The question each arm answers is **"is the declaration the problem?"**: * - * - `declared-unresolvable` — the app's `package.json` DOES name the package - * and the INSTALL is what is broken. Telling that operator to re-read a + * - `declared-unresolvable` — the app's `package.json` DOES name the package, + * so the DECLARATION is not the problem. Telling that operator to re-read a * file that is already correct is the defect this branch exists to avoid. + * ⚠️ It used to mint an install remedy here too — "Repair the INSTALL: run + * `pnpm install`, check that a production prune did not drop it, and that + * its dist is actually built". DRIVEN, on both shapes this kind covers, it + * was wrong twice over (#17046): + * · a genuinely broken install already gets those three remedies, word + * for word, from `unresolvableMessage` in the `cause:` line printed + * four lines below — so the bullet was a second copy to drift; + * · the #15045 sub-case — a location install this finder cannot tie to + * the declaration — prints a `cause:` that says outright "This is NOT + * an install problem … re-running `pnpm install`, un-pruning a deploy + * and rebuilding a dist all change nothing here", so the two halves of + * one screen contradicted each other. #17046 narrowed that sub-case + * (a correctly linked package now LOADS) but did NOT remove it: pnpm's + * `file:` virtual-store copy and every git / tarball declaration still + * land here. + * So this arm now says only what it uniquely knows — the declaration — and + * defers the remedy, exactly as the next one does. * - `declared-no-loadable-entry` (#14041) — the app declares it, the install * DELIVERED it, and the package's own `exports` names no runtime entry * Node can load. Neither absence applies: there is nothing to declare and @@ -5434,15 +5450,15 @@ export function formatI18nLoadDiagnostic(pkg: string, err: unknown): string { export function formatOrganizationsInstallRemedy( kind: HostImportFailureKind | undefined, declaration: HostDeclaration, - hostRoot: string, ): string { const pkg = Serve.ORGANIZATIONS_RUNTIME_PKG; if (kind === 'declared-unresolvable') { return ` • this app DECLARES ${pkg} ` + `(${declaration.field}: ${JSON.stringify(declaration.specifier)}) — the\n` + ' declaration is NOT the problem and re-reading package.json will not help.\n' + - ` Repair the INSTALL in ${hostRoot}: run \`pnpm install\`, check that a\n` + - ' production prune did not drop it, and that its dist is actually built — or\n'; + ' What IS wrong was measured by the importer: the cause below names the\n' + + ' directory it consulted and what it found there, and is the authority on\n' + + ' whether any install action can change it — or\n'; } if (kind === 'declared-no-loadable-entry') { return ` • this app DECLARES ${pkg} ` +