From 768efb8f86713f0b20f2babd3314291e8830c4e6 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 23 Aug 2026 15:37:16 +0000 Subject: [PATCH 1/2] fix(cli): serve's host importer states its own resolution base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. #10943 made it an explicit parameter; `@objectstack/verify` and the 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`. Thread the base, and collapse `Serve.importConfigPlugin`'s three-branch shape to two: the local `import()` its undeclared branch kept, and the re-entry it made for the failure text, are now the same call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../cli-serve-host-importer-caller-base.md | 13 + .../serve-cluster-host-resolution.test.ts | 37 ++- ...erve-config-plugin-host-resolution.test.ts | 49 +++- .../commands/serve-host-fallback-base.test.ts | 136 ++++++++++ packages/cli/src/commands/serve.ts | 148 +++++++---- .../test/serve-host-fallback-base.e2e.test.ts | 236 ++++++++++++++++++ 6 files changed, 545 insertions(+), 74 deletions(-) create mode 100644 .changeset/cli-serve-host-importer-caller-base.md create mode 100644 packages/cli/src/commands/serve-host-fallback-base.test.ts create mode 100644 packages/cli/test/serve-host-fallback-base.e2e.test.ts 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..6a658b2fa0 --- /dev/null +++ b/packages/cli/test/serve-host-fallback-base.e2e.test.ts @@ -0,0 +1,236 @@ +// 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 helper, so the control can build an importer with NO caller base. */ +const TYPES_NODE = resolve(HERE, '../../types/dist/node.mjs'); +/** The workspace `tsx` binary (an installed dependency, not a repo source input). */ +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +/** 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, typesPath, appRoot] = process.argv.slice(2); +const { pathToFileURL } = await import('node:url'); +const { default: Serve } = await import(pathToFileURL(servePath).href); +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, TYPES_NODE, 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'); + // …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`'); + }); +}); From 2656cafb295773734c49b26a1549c2052c08ec9b Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 23 Aug 2026 15:59:59 +0000 Subject: [PATCH 2/2] test(cli): name @objectstack/types by specifier, not by a filesystem climb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:cross-package-test-inputs` flagged `resolve(HERE, '../../types/dist/ node.mjs')` in the new e2e: a filesystem climb names a repo SOURCE input no turbo glob covers. The child now resolves the helper from serve.ts's own location by specifier, which is what serve.ts itself does and what the gate asks for — a bare specifier is an installed dependency, and @objectstack/cli already declares @objectstack/types, so turbo's task graph carries that edge. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../test/serve-host-fallback-base.e2e.test.ts | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/cli/test/serve-host-fallback-base.e2e.test.ts b/packages/cli/test/serve-host-fallback-base.e2e.test.ts index 6a658b2fa0..8fd7c8871e 100644 --- a/packages/cli/test/serve-host-fallback-base.e2e.test.ts +++ b/packages/cli/test/serve-host-fallback-base.e2e.test.ts @@ -85,11 +85,27 @@ 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 helper, so the control can build an importer with NO caller base. */ -const TYPES_NODE = resolve(HERE, '../../types/dist/node.mjs'); /** 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. */ @@ -107,9 +123,14 @@ const SENTINEL = '===os-11157-probe==='; * as itself rather than as a bare boolean. */ const PROBE = ` -const [servePath, typesPath, appRoot] = process.argv.slice(2); +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) => { @@ -161,7 +182,7 @@ beforeAll(async () => { probeFile = join(neutralCwd, 'probe.mjs'); writeFileSync(probeFile, PROBE, 'utf8'); - const { stdout } = await execFileAsync(TSX, [probeFile, SERVE_TS, TYPES_NODE, appRoot], { + const { stdout } = await execFileAsync(TSX, [probeFile, SERVE_TS, appRoot], { cwd: neutralCwd, env: { ...process.env, NO_COLOR: '1' }, maxBuffer: 16 * 1024 * 1024, @@ -183,6 +204,9 @@ describe('os serve → the undeclared fallback resolves from packages/cli (#1115 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`'); });