diff --git a/.changeset/cli-serve-host-importer-caller-base.md b/.changeset/cli-serve-host-importer-caller-base.md new file mode 100644 index 0000000000..bba1a4bb07 --- /dev/null +++ b/.changeset/cli-serve-host-importer-caller-base.md @@ -0,0 +1,13 @@ +--- +"@objectstack/cli": patch +--- + +**Fix:** `os serve`'s host importer now states its own resolution base, so a package the served app does not declare resolves from `packages/cli` instead of from `@objectstack/types` (#11157). + +`createHostImporter` has two legs. The **declared** leg resolves out of the served app's `node_modules` (#4719; #11185 fixed *which* app that is). The **undeclared** leg falls back to "the importing package's own resolution" — and which package that is depends entirely on where the `import()` is physically written, because Node ESM resolves a bare specifier against the module containing the call. #10943 turned that into an explicit parameter, `options.fallbackImport`, so a caller can hand in its own `import()`. `@objectstack/verify` (`bootStack`) and the `packages/qa/dogfood` enterprise probe both pass theirs; `serve`'s `importFromHost` did not, so it advertised the CLI's resolution and actually used `@objectstack/types`', which under a pnpm-isolated layout sees only `@objectstack/spec`. + +**Measured accept-set delta**: the undeclared fallback now reaches exactly what `packages/cli` itself declares, and nothing else. Re-measured with `import.meta.resolve` from a probe in each package — `chalk`, `@objectstack/plugin-auth` and `@objectstack/plugin-audit` resolve from `packages/cli` and not from `@objectstack/types`; every specifier `serve` itself routes through the helper (`@objectstack/service-cluster` and its drivers, `@objectstack/service-i18n`, `@objectstack/organizations`, `@objectstack/service-ai`, `@objectstack/service-ai-studio`) resolves from **neither**, which is why this was harmless in every shape that ships today. The #4719 declaration gate on the declared leg is untouched: a package that is merely reachable is still refused, and no app gains a way to load something it has not declared. + +**Two user-visible consequences.** A `plugins: [...]` entry naming a package the app does not declare but the CLI ships now resolves through the host importer rather than a separate local `import()` — same module, one attempt instead of two. And the undeclared-package diagnostic drops its "the caller did not pass `fallbackImport`" note, which `@objectstack/types` emits only for callers that withhold their base; the note existed so this gap would report itself, and it has now been closed rather than silenced. + +`Serve.importConfigPlugin`'s three-branch shape collapses to two in the same change. The undeclared branch kept a local `import()` *because* the helper's fallback resolved from the wrong package; with the base threaded, that branch and the re-entry branch are the same call, so the declaration is read once — by `readHostDeclaration` inside the helper — instead of being asked there and again here. Behaviour was measured case by case first: the app's declared copy still wins, a declared-but-uninstalled package still reports the install remedy, a package present-but-throwing still propagates as a crash (both paths gate on the one shared `isModuleNotFoundError`), and a package resolvable nowhere still produces the #4719 "declare it in that app's package.json" text. diff --git a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts index 6abed90d0a..97569d212a 100644 --- a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts @@ -278,18 +278,31 @@ const UNRESOLVABLE_BARE_IMPORTS: Record = { // Serve.CAPABILITY_PROVIDERS — every `pkg` in that table is CLI-declared. 'spec.pkg': 'Serve.CAPABILITY_PROVIDERS entries are all CLI-declared', 'ex.pkg': 'CAPABILITY_PROVIDERS `extras` entries are all CLI-declared', - // The app's own `plugins: [...]` config entries, now routed through - // `Serve.importConfigPlugin` (#10908). Two bare `import()` sites remain there, - // both reached only AFTER the declaration has been consulted, and both are the - // reason this list exists rather than a hole in it: - // • the specifier is not a package name at all (path, `file://`, `node:`) — - // nothing a package.json can declare; - // • the served app does NOT declare it, so it must resolve from this CLI, - // which is exactly the pre-existing behaviour #10908 promised to keep. - // The DECLARED case — the only one this card moves — goes to `importFromHost`. - // Pinned behaviourally, not by this comment, in - // `serve-config-plugin-host-resolution.test.ts`. - pluginSpecifier: 'post-declaration branches: a path/URL, or a package the app does not declare (#10908)', + // The app's own `plugins: [...]` config entries, routed through + // `Serve.importConfigPlugin` (#10908). ONE bare `import()` site remains there, + // and it is the reason this list exists rather than a hole in it: the + // specifier is not a package name at all (an absolute path, a `file://` URL, a + // `node:` builtin), so nothing a package.json can declare, and every one of + // those spellings means the same module from every base. + // + // It used to be TWO. The second was the UNDECLARED branch, which kept a local + // `import()` because the host importer's fallback resolved from + // `@objectstack/types` rather than from this CLI. #11157 threaded the base + // (`fallbackImport`), which made that branch identical to the helper's own + // fallback, and it was collapsed into `importFromHost`. Pinned behaviourally, + // not by this comment, in `serve-config-plugin-host-resolution.test.ts` and + // `serve-host-fallback-base.test.ts`. + pluginSpecifier: 'the non-package branch: an absolute path, a file:// URL or a node: builtin (#10908)', + // `importFromHost`'s own `fallbackImport` (#11157) — the caller base + // `createHostImporter` resolves everything the served app does NOT declare + // from. It is a bare `import()` on purpose and it MUST be written in this + // file: ESM resolves a bare specifier against the module containing the call, + // so moving it anywhere else moves the base, which is the whole defect. Its + // parameter is the helper's argument, so no scan can know the specifier — + // and no scan needs to: this site is not a load of any particular package, + // it is the resolution base every other undeclared load is handed. + fallbackSpecifier: + "importFromHost's caller base — the CLI's own resolver, handed to createHostImporter (#11157)", }; /** diff --git a/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts index 37d44d6b7c..3072f65b85 100644 --- a/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts @@ -15,10 +15,18 @@ import Serve from './serve.js'; * CLI could see. Green in a dev checkout, absent on a real distribution layout * (#10908; the same mechanism as cloud#1013 and #10645). * - * The repair moves ONLY the declared case. These tests pin all three branches, - * because two of them exist to keep behaviour that a naive + * The repair moves ONLY the declared case. These tests pin every branch the + * method has, including the ones that exist to keep behaviour a naive * `await importFromHost(specifier)` would have taken away — see * `Serve.importConfigPlugin` for the measurements. + * + * ⚠️ #11157 collapsed the shape from three branches to two: once `importFromHost` + * hands `createHostImporter` this file's own resolver (`fallbackImport`), the + * helper's undeclared leg IS the local `import()` the undeclared branch used to + * make, so that branch and the re-entry branch became one call. Every assertion + * below is unchanged and still describes real behaviour — that is what made the + * collapse safe to take. The one that had to move is the structural one at the + * bottom: the declaration read now has a single owner inside the helper. */ const roots: string[] = []; @@ -143,11 +151,16 @@ describe('os serve → the missing-plugin diagnostic is a chosen text (#10908 / */ describe('os serve → the branches that must NOT move (#10908 supersedes nothing)', () => { it('keeps this CLI as the resolver for a package the app does not declare', async () => { - // `chalk` is declared by packages/cli and by no fixture app. Today's bare - // `import()` finds it; through the host importer's fallback — which resolves - // from `@objectstack/types` — it does not. An app that writes - // `plugins: ['@objectstack/plugin-auth']` without declaring it boots today, - // and this is the assertion that says it still does. + // `chalk` is declared by packages/cli and by no fixture app. An app that + // writes `plugins: ['@objectstack/plugin-auth']` without declaring it boots + // today, and this is the assertion that says it still does. + // + // ⚠️ This assertion is why #11157 had to land BEFORE the branch collapse and + // not after. It used to be kept true by a local `import()` here; it is now + // kept true by `importFromHost` carrying this file's base. Take the base + // away and this line goes red — measured, and pinned again from the other + // side (with the no-base control beside it) in + // `serve-host-fallback-base.test.ts`. const root = makeApp(APP_ONLY, { declare: false, install: false }); const mod = await Serve.importConfigPlugin('chalk', root); @@ -204,10 +217,24 @@ describe('os serve → the config-plugin load stays wired to the helper', () => }); it('the declaration decides the resolver, so the gate keeps its say (#4719)', () => { - // A helper that stopped consulting the declaration would still pass every - // behavioural test above that uses a DECLARED fixture, so pin the wiring. + // A helper that reached the app's copy by some route OTHER than the host + // importer would still pass the behavioural tests above, so pin the wiring. + // + // ⚠️ This used to also require `isDeclaredByHost(pluginSpecifier, root)` in + // this method. #11157 removed that call — not the check. `importFromHost` + // now carries this file's resolution base, which made the local undeclared + // branch identical to the helper's own fallback, so the declaration is read + // exactly once, by `readHostDeclaration` inside `createHostImporter`. Asking + // the same question twice in two places is the fork Prime Directive #12 + // exists to prevent; requiring the second copy HERE would have pinned it. + // The single owner is pinned in `packages/types/src/node.test.ts`. const helper = SERVE_SOURCE.slice(SERVE_SOURCE.indexOf('static async importConfigPlugin')); - expect(helper).toContain('isDeclaredByHost(pluginSpecifier, root)'); - expect(helper).toContain('importFromHost(pluginSpecifier, root)'); + const body = helper.slice(0, helper.indexOf('\n }\n')); + expect(body).toContain('importFromHost(pluginSpecifier, root)'); + // The resolver is never chosen by a second, local reading of the manifest. + expect(body).not.toContain('isDeclaredByHost'); + // …and the entry is never handed to a bare `import()` once it names a + // package: that is the #10908 defect itself. + expect(body).not.toMatch(/if \(isDeclaredByHost/); }); }); diff --git a/packages/cli/src/commands/serve-host-fallback-base.test.ts b/packages/cli/src/commands/serve-host-fallback-base.test.ts new file mode 100644 index 0000000000..3ec3d88348 --- /dev/null +++ b/packages/cli/src/commands/serve-host-fallback-base.test.ts @@ -0,0 +1,136 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `serve` hands `createHostImporter` its OWN resolution base (#11157) — the + * half of that card an in-process test can honestly measure. + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `createHostImporter`'s UNDECLARED leg falls back to "the importing package's + * own resolution", and which package that is depends on where the `import()` is + * physically WRITTEN: Node ESM resolves a bare specifier against the module + * containing the call. #10943 made it an explicit parameter, + * `options.fallbackImport`. `@objectstack/verify` and the `packages/qa/dogfood` + * probe pass theirs; `serve`'s `importFromHost` did not, so its fallback + * resolved from `@objectstack/types` — which under a pnpm-isolated layout sees + * only `@objectstack/spec`. + * + * ── ⛔ DO NOT ASSERT RESOLUTION IN THIS FILE — it cannot fail here ────────── + * + * `@objectstack/types` is a LINKED workspace package, so Vite processes it as + * source instead of externalising it and rewrites the `import()` inside + * `packages/types/dist/node.mjs` to its own resolver — which resolves from the + * vitest root, `packages/cli`. MEASURED in this checkout: an in-process + * `createHostImporter(appRoot)('chalk')`, with NO caller base at all, RESOLVES + * under vitest and THROWS `Cannot find package 'chalk'` under Node. + * + * Under vitest the two bases ARE the same base. A "before/after" written here + * is green both ways, and — worse — so is the anti-vacuity control beside it, + * so nothing reports that the pin stopped measuring anything. The resolution + * pins therefore live in `test/serve-host-fallback-base.e2e.test.ts`, which + * spawns a real Node process. This is also why the `chalk` assertion in + * `serve-config-plugin-host-resolution.test.ts` is a behaviour statement and + * not the measurement of this card. + * + * ── What DOES fail here, and why it is not a proxy ───────────────────────── + * + * `undeclaredMessage` (`@objectstack/types/node`) composes two different texts + * depending on `fallbackImport !== undefined`. That branch is pure logic: no + * resolver touches it, so vitest cannot flatten it. Before this card `serve` got + * the text that tells the reader the caller withheld its base — a sentence this + * card makes false. Moving the branch is part of the fix, not evidence about it. + * + * This file reads only `serve.ts` and `package.json` from its own package. + */ + +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterAll, describe, expect, it } from 'vitest'; +import Serve from './serve.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** `packages/cli/package.json` — this package's OWN declared surface. */ +const CLI_MANIFEST = JSON.parse( + readFileSync(resolve(HERE, '..', '..', 'package.json'), 'utf8'), +) as { dependencies?: Record }; + +/** Declared by `packages/cli`, resolvable from it, NOT from `@objectstack/types`. */ +const CLI_DECLARED = 'chalk'; + +/** A name no package anywhere can satisfy, so no result can be an accident. */ +const NOWHERE = '@os-fixture/host-fallback-base-probe'; + +const roots: string[] = []; +afterAll(() => { + for (const r of roots) rmSync(r, { recursive: true, force: true }); +}); + +/** A served app that declares nothing. */ +function makeApp(): string { + const root = mkdtempSync(join(tmpdir(), 'os-fallback-base-')); + roots.push(root); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'fixture-app', version: '1.0.0', type: 'module' }), + ); + return root; +} + +describe('os serve → the undeclared diagnostic takes the caller-supplied-base branch', () => { + it('names the APP (#11185) and no longer says the caller withheld its base (#11157)', async () => { + const root = makeApp(); + + const err = (await Serve.importConfigPlugin(NOWHERE, root).catch((e: unknown) => e)) as Error; + + expect(err).toBeInstanceOf(Error); + expect(err.message).toContain(`Failed to import plugin '${NOWHERE}':`); + expect(err.message).toContain(`Cannot find package '${NOWHERE}'`); + // #11185's text: the app being served, never the process CWD. + expect(err.message).toContain(`host app: ${root}`); + expect(err.message).not.toContain(`host app: ${process.cwd()}`); + // #11157: the other branch of the same message. `serve` supplies its base + // now, so the note that exists to report the gap must not be printed. + expect(err.message).not.toContain('the caller did not pass `fallbackImport`'); + // The #4719 remedy the helper owns is unchanged — this card moved a base, + // not the declaration contract. + expect(err.message).toMatch(/Declare it in that app's package\.json/); + expect(err.message).toMatch(/merely REACHABLE is not enough/); + }); +}); + +describe('os serve → the base is wired at the single importer construction', () => { + const SERVE_SOURCE = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); + + it('the specifier the e2e pin uses is one packages/cli DECLARES', () => { + // Guards the e2e against the manifest changing under it: if `chalk` stopped + // being a declared dependency, that pin could still pass by workspace + // hoisting and would no longer measure the accept-set this card widens. + expect(Object.keys(CLI_MANIFEST.dependencies ?? {})).toContain(CLI_DECLARED); + }); + + it('passes fallbackImport where the importer is built', () => { + // `serve-cluster-host-resolution.test.ts` pins that there is exactly ONE + // `createHostImporter(` in this file. This pins that the one carries a base. + expect(SERVE_SOURCE).toMatch(/createHostImporter\(hostRoot,\s*\{/); + expect(SERVE_SOURCE).toMatch( + /fallbackImport: \(fallbackSpecifier\) => import\(\/\* webpackIgnore: true \*\/ fallbackSpecifier\)/, + ); + // A URL/string base would compile and silently ignore the parent argument — + // measured on Node v22 and recorded in `@objectstack/types/node`. It is not + // a spelling variant of the line above; it is the phantom fix of this card. + expect(SERVE_SOURCE).not.toMatch(/fallbackImport:\s*(?:import\.meta\.url|['"`])/); + }); + + it('the config-plugin path no longer re-implements the declaration read', () => { + // Collapsed in #11157: the undeclared branch's local `import()` and the + // re-entry branch became the same call once the base was threaded, so the + // declaration is read once, by `readHostDeclaration` inside the helper. + const helper = SERVE_SOURCE.slice(SERVE_SOURCE.indexOf('static async importConfigPlugin')); + const body = helper.slice(0, helper.indexOf('\n }\n')); + expect(body).toContain('importFromHost(pluginSpecifier, root)'); + expect(body).not.toContain('isDeclaredByHost'); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 50c5ee3cf7..0ea76ca0eb 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -295,10 +295,11 @@ function servedAppRootOrCwd(): string { * MEASURED. Before: hostRoot was the CWD, so `declared` was true and * `hostRequire.resolve` found the package under the CWD's `node_modules`. * After: hostRoot is the served app, `declared` is false, and the load goes to - * `createHostImporter`'s fallback — which is a bare `import()` physically - * inside `@objectstack/types`, because `importFromHost` passes no - * `fallbackImport` (#11157's residue). Node ESM walks `node_modules` UPWARD - * from there, so the common shape survives: in a hoisted monorepo whose ROOT + * `createHostImporter`'s fallback — which is a bare `import()` written in THIS + * file, because `importFromHost` now hands the helper its own resolution base + * (`fallbackImport`, #11157; it used to resolve from `@objectstack/types` + * instead). Node ESM walks `node_modules` UPWARD from there, so the common + * shape survives: in a hoisted monorepo whose ROOT * manifest declares the package while the served `apps/foo/package.json` does * not, that walk reaches the same hoisted store and boot is what it always * was — only the leg changed. It genuinely refuses only where the walk cannot @@ -374,6 +375,41 @@ function anchorServedApp(configArg: string): { configPath: string; configExists: * `serve-cluster-host-resolution.test.ts` is the detection backstop: it scans * this file for every app-declarable optional load and fails on a bare one. * + * ── What the UNDECLARED leg resolves from, and why it is stated here (#11157) ─ + * + * `createHostImporter` has two legs. The DECLARED one resolves out of the served + * app's own `node_modules`, anchored by `hostRoot`. The UNDECLARED one falls back + * to "the importing package's own resolution" — and which package that IS depends + * entirely on where the `import()` is physically WRITTEN, because Node ESM + * resolves a bare specifier against the module containing the call. + * + * Omit `fallbackImport` and that call is the default one inside + * `@objectstack/types`, so the fallback sees only what THAT package declares. + * MEASURED in this checkout, `import.meta.resolve` from each base: + * + * specifier from packages/cli from packages/types + * @objectstack/plugin-auth OK MISS + * @objectstack/plugin-audit OK MISS + * chalk OK MISS + * @objectstack/spec OK OK ← types' one dep + * + * #10943 made the base an explicit parameter for exactly that reason, and its + * other two callers (`@objectstack/verify`'s `bootStack`, the `packages/qa/ + * dogfood` enterprise probe) pass theirs. This file was the one that did not, so + * its undeclared leg resolved from a package it has nothing to do with. That was + * harmless only by accident — every specifier reaching it today is app-supplied + * and resolves from NEITHER base — and the next CLI-declared package loaded here + * would have silently missed the CLI's own dependencies. Passing the base widens + * the undeclared leg's reach to exactly what `packages/cli` itself declares, and + * to nothing else: the #4719 declaration gate on the DECLARED leg is untouched, + * so no app gains a way to load something it has not declared. + * + * The base is a FUNCTION and not a `parentURL` string because both string + * spellings were measured wrong — `import.meta.resolve`'s parent argument is + * silently ignored without a flag, and `createRequire().resolve` re-opens the + * `NODE_PATH` hole #4719 closed. `@objectstack/types/node` carries both + * measurements; do not re-derive them here. + * * @param hostRoot Directory holding the served app's `package.json`. Defaults to * {@link servedAppRootOrCwd} — the directory `serve` read the app's * `objectstack.config.ts` from, which is the app's own root and NOT necessarily @@ -386,7 +422,12 @@ function importFromHost(specifier: string, hostRoot: string = servedAppRootOrCwd // one mid-function `const` did before it was hoisted out here. let importer = hostImporters.get(hostRoot); if (!importer) { - importer = createHostImporter(hostRoot); + importer = createHostImporter(hostRoot, { + // THIS module's own resolver, written HERE (#10943/#11157) — see the + // "what the undeclared leg resolves from" note above for why it has to be + // a function in the calling module and not a URL string. + fallbackImport: (fallbackSpecifier) => import(/* webpackIgnore: true */ fallbackSpecifier), + }); hostImporters.set(hostRoot, importer); } return importer(specifier); @@ -580,49 +621,67 @@ export default class Serve extends Command { * distribution layout. Same mechanism as cloud#1013 and #10645, but on the * surface users are explicitly told to use. * - * ── Why this is three branches and not `await importFromHost(specifier)` ──── + * ── Why this WAS three branches, and why it is now two (#10908 → #11157) ──── * - * The obvious repair is to hand every specifier to `importFromHost`. MEASURED, - * that is NOT a superset of what this line does today — in two ways, both of - * which would take working deployments away: + * The three-branch shape existed because handing every specifier to + * `importFromHost` was MEASURED not to be a superset of a bare `import()` — + * in two ways, both of which would have taken working deployments away: * * 1. A RELATIVE specifier changes base. `createHostImporter` passes a * non-package specifier through to an `import()` that physically lives in * `@objectstack/types`, and ESM resolves a relative specifier against the * module CONTAINING the call — so `'./local-plugin.js'` would resolve * against `@objectstack/types/dist/` instead of this file's directory. - * Neither base is the served app's root, so no relative spelling works - * the way an author would expect either way; #10944 carries that - * question, and this branch is why the answer stays open rather than - * being decided by a silent re-base here. - * 2. An UNDECLARED bare name changes base the same way, and this one bites. + * STILL TRUE, and still why the non-package branch below stays here + * rather than being folded into the helper. (#10944 has since RULED on + * the relative spelling itself: it is refused above, before any base is + * chosen. The remaining non-package spellings — an absolute path, a + * `file://` URL, a `node:` builtin — mean the same module from every + * base, so keeping them local costs nothing and removes the one way a + * future relative spelling this file's refusal does not recognise could + * be silently re-based inside `@objectstack/types`.) + * 2. An UNDECLARED bare name changed base the same way, and that one bit. * `createHostImporter`'s fallback is documented as "the importing - * package's own resolution", but the import it falls back to also lives - * in `@objectstack/types`, which under a pnpm-isolated layout can see - * only `@objectstack/types`'s own dependencies. Measured from an app that - * declares nothing: `@objectstack/plugin-auth` and `@objectstack/plugin- - * audit` resolve from THIS package and fail through the host importer. - * An app that writes `plugins: ['@objectstack/plugin-auth']` without - * declaring it — a spelling this repo's own fixtures use — boots today - * and would stop booting. The helper's own docblock claims the opposite - * ("falls back to the importing package's own resolution"); that text is - * wrong, and #10943 carries the fix. Until it lands, a caller that needs - * its own resolution has to ask the declaration itself, as below. + * package's own resolution", but the import it fell back to also lived + * in `@objectstack/types`, which under a pnpm-isolated layout sees only + * `@objectstack/types`'s own dependencies. Measured from an app that + * declares nothing: `@objectstack/plugin-auth`, `@objectstack/plugin- + * audit` and `chalk` resolve from THIS package and failed through the + * host importer. An app that writes `plugins: ['@objectstack/plugin- + * auth']` without declaring it — a spelling this repo's own fixtures use + * — booted, and would have stopped booting. So this method asked the + * declaration itself and kept a local `import()` for the undeclared leg. + * + * ⇒ NO LONGER TRUE. #10943 made the base a parameter and #11157 made + * `importFromHost` pass it, so the helper's undeclared leg now runs THIS + * file's own `import()` — the identical call this method used to make + * inline. The workaround's reason is gone, so the workaround is gone with + * it, and the declaration has ONE owner again (`readHostDeclaration`, + * inside the helper) instead of being asked here and again there. * - * So the declaration is what selects the resolver, exactly as #4719 says it - * should, and each branch keeps the resolution it already had: + * The collapse was measured case by case before it was written, and every case + * lands on the same module and the same error as the three-branch shape did: * - * • not a package name (path, `file://` URL, `node:` builtin) → unchanged; - * nothing a package.json can declare, so the gate has no opinion. - * • DECLARED by the served app → `importFromHost`: the app's own copy wins. - * This is the repair — the whole card is this branch. - * • UNDECLARED → this CLI's own resolution, byte-identical to the bare - * `import()` that has always been here. No app loses a plugin it does not - * declare but the CLI ships. + * • app DECLARES it, resolvable there → the app's copy (unchanged leg). + * • app DECLARES it, not installed → `declared-unresolvable`, the + * "repair the INSTALL" remedy — never a fallback (unchanged leg). + * • app does NOT declare it, resolvable from this CLI → the CLI's copy, via + * `fallbackImport`, which is `import()` written in this module. Byte-for- + * byte the resolution the deleted branch performed. + * • app does NOT declare it, present but throwing while it evaluates → the + * crash propagates untouched. Both the deleted branch and the helper gate + * on the SAME predicate (`isModuleNotFoundError`, one owner in + * `@objectstack/types`), so "present but broken" is still never + * reinterpreted as an absence. + * • app does NOT declare it, resolvable nowhere → the #4719 "declare it in + * that app's package.json" text. The deleted shape reached that by failing + * a local `import()` and then RE-ENTERING the helper purely for the + * wording; one call now does both, so the specifier is attempted once + * instead of twice. * - * Nothing about WHICH plugins are accepted changes: this only moves where a - * declared one resolves FROM. The #4719 declaration gate is untouched, and no - * undeclared package gains a way in that it did not already have. + * Nothing about WHICH plugins are accepted changes, in either edit: this only + * moves where one resolves FROM. The #4719 declaration gate is untouched, and + * no undeclared package gains a way in that it did not already have. * * ── The relative branch is REFUSED, not resolved (#10944) ────────────────── * @@ -665,20 +724,7 @@ export default class Serve extends Command { if (packageNameFromSpecifier(pluginSpecifier) === undefined) { return await import(/* webpackIgnore: true */ pluginSpecifier); } - if (isDeclaredByHost(pluginSpecifier, root)) { - return await importFromHost(pluginSpecifier, root); - } - try { - return await import(/* webpackIgnore: true */ pluginSpecifier); - } catch (cliError: unknown) { - // Present but broken is a crash, not an absence — never reinterpret it. - if (!Serve.isModuleNotFoundError(cliError)) throw cliError; - // Undeclared AND unresolvable anywhere. Re-enter the host importer for - // the failure alone: it owns the #4719 "declare it in that app's - // package.json" remedy, and having one owner of that wording is why - // this does not compose the message itself. - return await importFromHost(pluginSpecifier, root); - } + return await importFromHost(pluginSpecifier, root); } catch (importError: any) { // The wrapper lives with the load it describes, so the composed // user-facing string is testable rather than assembled at the call site diff --git a/packages/cli/test/serve-host-fallback-base.e2e.test.ts b/packages/cli/test/serve-host-fallback-base.e2e.test.ts new file mode 100644 index 0000000000..8fd7c8871e --- /dev/null +++ b/packages/cli/test/serve-host-fallback-base.e2e.test.ts @@ -0,0 +1,260 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #11157 — `serve`'s host importer resolves the UNDECLARED leg from + * `packages/cli`, because it hands `createHostImporter` its own base. + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `createHostImporter` has two legs. The DECLARED leg resolves out of the served + * app's `node_modules` (#4719; #11185 fixed WHICH app that is). The UNDECLARED + * leg falls back to "the importing package's own resolution" — and which package + * that is depends entirely on where the `import()` is physically WRITTEN, + * because Node ESM resolves a bare specifier against the module containing the + * call. #10943 made that an explicit parameter, `options.fallbackImport`, so a + * caller can hand in its own `import()`. `@objectstack/verify` (`bootStack`) and + * the `packages/qa/dogfood` enterprise probe both pass theirs; `serve`'s + * `importFromHost` did not, so the CLI advertised its own resolution and + * actually used `@objectstack/types`', which under a pnpm-isolated layout sees + * only `@objectstack/spec`. + * + * ── WHY THIS FILE SPAWNS A REAL NODE PROCESS — measured, not stylistic ────── + * + * This is the whole reason the card's pin does not live in + * `src/commands/serve-host-fallback-base.test.ts` beside the other unit pins. + * + * `@objectstack/types` is a LINKED workspace package, so Vite processes it as + * source rather than externalising it, and the `import()` inside + * `packages/types/dist/node.mjs` is rewritten to Vite's own resolver — which + * resolves from the vitest root, `packages/cli`. MEASURED in this checkout: an + * in-process `createHostImporter(appRoot)('chalk')`, with NO caller base at all, + * RESOLVES under vitest and THROWS `Cannot find package 'chalk'` under Node. + * + * So under vitest the two bases are the same base, and every in-process + * assertion about which one is in use is green either way. That is not a + * weakness of one test — it silently makes the anti-vacuity control itself + * vacuous, which is the failure mode the card was filed to avoid. Only a real + * process measures it. + * + * ── Why a probe script and not a full `serve` boot ───────────────────────── + * + * The base is a property of `serve.ts`'s module identity, which a spawned + * `import()` of that file reproduces exactly — the same file, the same realpath, + * the same `node_modules` walk as the shipped `dist/commands/serve.js`. Booting + * a whole server to observe it would add a database, a port and ~40s per case + * and measure nothing extra. `test/serve-app-anchored-optional-import.e2e.test.ts` + * spawns the real CLI because #11185's base is computed inside `run()` and is + * unobservable from outside it; this card's base is not. + * + * One spawn covers every case, because the expensive part is loading + * `serve.ts`'s module graph once. + * + * ── The anti-vacuity floor ─────────────────────────────────────────────── + * + * `chalk` is DECLARED by `packages/cli` and resolvable from it, and NOT + * resolvable from `@objectstack/types`. Re-measured with `import.meta.resolve` + * from a probe inside each package: + * + * specifier from packages/cli from packages/types + * chalk OK MISS + * @objectstack/plugin-auth OK MISS + * @objectstack/plugin-audit OK MISS + * @objectstack/spec OK OK ← types' one dep + * @objectstack/service-cluster MISS MISS ← app-supplied + * + * The `control` case below builds the importer the way `importFromHost` used to + * — `createHostImporter(root)`, no base — and shows it failing on the very + * specifier the pin loads. Delete that and the pin degrades to "chalk is + * installed somewhere". + * + * `@objectstack/plugin-auth` would have worked equally well and is deliberately + * NOT used: this package's `vitest.config.ts` aliases it to source. + */ + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { execFile } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** The module under test, loaded by the child exactly as the CLI's own dist is. */ +const SERVE_TS = resolve(HERE, '../src/commands/serve.ts'); +/** The workspace `tsx` binary (an installed dependency, not a repo source input). */ +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +/** + * ⚠️ `@objectstack/types` is reached by SPECIFIER, resolved inside the child from + * `serve.ts`'s own location — never as `resolve(HERE, '../../types/dist/…')`. + * Both spellings land on the same file; only one of them is honest about what it + * is naming. `check:cross-package-test-inputs` states the rule and the reason: + * a filesystem climb names a repo SOURCE input that no turbo glob covers, so a + * change to it would not re-run this package's tests, while a bare specifier is + * an installed dependency — and `@objectstack/cli` already declares + * `@objectstack/types`, so turbo's task graph carries that edge already. + * + * The child gets the CJS twin (`dist/node.js`) because that is what + * `createRequire().resolve()` selects, and for THIS measurement the twins are + * interchangeable: both sit in `packages/types/dist/`, so the `node_modules` + * walk their fallback `import()` performs starts from the same directory. The + * control below asserts the reported origin, so a future layout change that made + * them differ would show up as a failure rather than as a quiet pass. + */ + +/** Declared by `packages/cli`; not resolvable from `@objectstack/types`. */ +const CLI_DECLARED = 'chalk'; +/** App-supplied: resolvable from NEITHER base. The class `serve` actually loads. */ +const APP_SUPPLIED = '@objectstack/service-cluster'; +/** Satisfiable by nothing anywhere, so no result can be an accident. */ +const NOWHERE = '@os-fixture/host-fallback-base-probe'; +/** Written into the app's `node_modules` and never declared (#4719). */ +const REACHABLE_UNDECLARED = '@os-fixture/reachable-but-undeclared'; + +const SENTINEL = '===os-11157-probe==='; + +/** + * Everything measured, in one child. Each entry is either `RESOLVED` or the + * first line / full text of what was thrown, so an unexpected outcome shows up + * as itself rather than as a bare boolean. + */ +const PROBE = ` +const [servePath, appRoot] = process.argv.slice(2); +const { pathToFileURL } = await import('node:url'); +const { createRequire } = await import('node:module'); +const { default: Serve } = await import(pathToFileURL(servePath).href); +// The helper as SERVE.TS itself reaches it — by specifier, from serve.ts's own +// location — so the control measures the real dependency edge and this file +// never names a path outside its own package. +const typesPath = createRequire(servePath).resolve('@objectstack/types/node'); +const { createHostImporter } = await import(pathToFileURL(typesPath).href); + +const attempt = async (fn) => { + try { await fn(); return 'RESOLVED'; } + catch (e) { return e && e.message ? e.message : String(e); } +}; + +const out = { + controlNoBase: await attempt(() => createHostImporter(appRoot)(${JSON.stringify(CLI_DECLARED)})), + cliDeclared: await attempt(() => Serve.importConfigPlugin(${JSON.stringify(CLI_DECLARED)}, appRoot)), + appSupplied: await attempt(() => Serve.importConfigPlugin(${JSON.stringify(APP_SUPPLIED)}, appRoot)), + nowhere: await attempt(() => Serve.importConfigPlugin(${JSON.stringify(NOWHERE)}, appRoot)), + reachableUndeclared: await attempt(() => + Serve.importConfigPlugin(${JSON.stringify(REACHABLE_UNDECLARED)}, appRoot), + ), +}; +console.log(${JSON.stringify(SENTINEL)}); +console.log(JSON.stringify(out)); +`; + +/** A served app that declares nothing, and a CWD that is not it. */ +let appRoot: string; +let neutralCwd: string; +let probeFile: string; +let probe: Record; + +beforeAll(async () => { + appRoot = mkdtempSync(join(tmpdir(), 'os-11157-app-')); + neutralCwd = mkdtempSync(join(tmpdir(), 'os-11157-cwd-')); + writeFileSync( + join(appRoot, 'package.json'), + JSON.stringify({ name: 'fixture-app', version: '1.0.0', type: 'module' }), + ); + // Present in the app's node_modules, absent from its package.json: the #4719 + // shape that must stay refused however the fallback base moves. + const reachable = join(appRoot, 'node_modules', ...REACHABLE_UNDECLARED.split('/')); + mkdirSync(reachable, { recursive: true }); + writeFileSync( + join(reachable, 'package.json'), + JSON.stringify({ + name: REACHABLE_UNDECLARED, + version: '1.0.0', + type: 'module', + main: 'index.js', + }), + ); + writeFileSync(join(reachable, 'index.js'), 'export const loadedFrom = "app-node_modules";\n'); + + probeFile = join(neutralCwd, 'probe.mjs'); + writeFileSync(probeFile, PROBE, 'utf8'); + + const { stdout } = await execFileAsync(TSX, [probeFile, SERVE_TS, appRoot], { + cwd: neutralCwd, + env: { ...process.env, NO_COLOR: '1' }, + maxBuffer: 16 * 1024 * 1024, + }); + const payload = stdout.slice(stdout.indexOf(SENTINEL) + SENTINEL.length); + probe = JSON.parse(payload.trim()) as Record; +}, 180_000); + +afterAll(() => { + for (const dir of [appRoot, neutralCwd]) { + if (dir) rmSync(dir, { recursive: true, force: true }); + } +}); + +describe('os serve → the undeclared fallback resolves from packages/cli (#11157)', () => { + it('CONTROL: the same importer with NO caller base cannot reach a CLI-declared package', () => { + // The floor. This is `createHostImporter(root)` exactly as `importFromHost` + // built it before this card, failing on the specifier the next case loads. + expect(probe.controlNoBase).not.toBe('RESOLVED'); + expect(probe.controlNoBase).toContain(`Cannot find package '${CLI_DECLARED}'`); + expect(probe.controlNoBase).toContain('does not declare it'); + // Named, not just failed: the no-base leg really does resolve from + // `packages/types`, which is the sentence this whole card is about. + expect(probe.controlNoBase).toMatch(/imported from .*[/\\]packages[/\\]types[/\\]/); + // …and it says exactly why, which is the branch #11157 moves `serve` off. + expect(probe.controlNoBase).toContain('the caller did not pass `fallbackImport`'); + }); + + it('loads a package the served app does NOT declare but packages/cli DOES', () => { + // The load-bearing pin: the undeclared leg now runs serve.ts's own + // `import()`. Remove `fallbackImport` from `importFromHost` and this reads + // like the control above — measured. + expect( + probe.cliDeclared, + `the undeclared fallback did not reach packages/cli:\n${probe.cliDeclared}`, + ).toBe('RESOLVED'); + }); +}); + +describe('os serve → the accept-set delta is exactly "what packages/cli declares"', () => { + it('still refuses a package NEITHER the app nor packages/cli declares', () => { + // The bound the card requires stated. The fallback moved from what + // `@objectstack/types` declares to what `packages/cli` declares — it did not + // become "anything reachable". Every specifier `serve` itself routes through + // this helper is in THIS class, which is why the card is "harmless today". + expect(probe.appSupplied).not.toBe('RESOLVED'); + expect(probe.appSupplied).toContain(`Failed to import plugin '${APP_SUPPLIED}':`); + expect(probe.appSupplied).toContain('does not declare it'); + }); + + it('leaves the #4719 gate alone: reachable-but-undeclared is still refused', () => { + // The package IS in the app's node_modules. Moving a resolution base must + // never turn "declared" into "resolvable from somewhere". + expect(probe.reachableUndeclared).not.toBe('RESOLVED'); + expect(probe.reachableUndeclared).toContain('does not declare it'); + expect(probe.reachableUndeclared).toMatch(/merely REACHABLE is not enough/); + }); +}); + +describe('os serve → the undeclared diagnostic reports the base actually used', () => { + it('names the APP (#11185) and the CLI as the fallback origin (#11157)', () => { + expect(probe.nowhere).toContain(`Cannot find package '${NOWHERE}'`); + // #11185: the app being served, never the process CWD. + expect(probe.nowhere).toContain(`host app: ${appRoot}`); + expect(probe.nowhere).not.toContain(`host app: ${neutralCwd}`); + // #11157: the fallback that failed is now THIS package's, so the path Node + // reports is inside packages/cli and not inside packages/types. + expect(probe.nowhere).toMatch(/fallback resolution also failed: .*imported from /); + expect(probe.nowhere).toMatch(/imported from .*[/\\]packages[/\\]cli[/\\]/); + expect(probe.nowhere).not.toMatch(/imported from .*[/\\]packages[/\\]types[/\\]/); + // `undeclaredMessage` composes two different texts depending on + // `fallbackImport !== undefined`; `serve` is now on the other branch. + expect(probe.nowhere).not.toContain('the caller did not pass `fallbackImport`'); + }); +});