From 4dd0bb86d041be0abaf36dfc84b70e29277a52c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:37:50 +0000 Subject: [PATCH 1/3] fix(cli): make serve's host importer reachable from anywhere, and sweep for the class (#10769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `importFromHost` was a `const` bound partway down one very long boot method, so it existed only below its own binding. A load written above it was not a compile error — the author wrote a bare `import()`, which resolves from the CLI and is green in any dev checkout where everything is hoisted into one `node_modules`, and dead at boot on a real distribution layout. That shipped twice (cloud#1013, #10645); hoisting the binding fixed each instance and left the class open. It is now a module-scope function declaration, hoisted over the entire module, so "above the definition" is not a state this file can be in. Sweeping for the class found one live instance: `@objectstack/service-i18n` was loaded bare although `packages/cli` does not declare it. Now host-anchored; the undeclared fallback keeps its quiet-skip path unchanged. The source scan is widened from the cluster pair to every app-declarable optional load, classified mechanically against the CLI's own manifest, with a vacuity guard so it cannot pass by matching nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- .../serve-host-importer-module-scope.md | 51 +++ .../serve-cluster-host-resolution.test.ts | 405 ++++++++++++++++-- packages/cli/src/commands/serve.ts | 129 ++++-- 3 files changed, 522 insertions(+), 63 deletions(-) create mode 100644 .changeset/serve-host-importer-module-scope.md diff --git a/.changeset/serve-host-importer-module-scope.md b/.changeset/serve-host-importer-module-scope.md new file mode 100644 index 0000000000..ee5ba5ea2f --- /dev/null +++ b/.changeset/serve-host-importer-module-scope.md @@ -0,0 +1,51 @@ +--- +"@objectstack/cli": patch +--- + +`os serve` now resolves **every** app-declarable optional package from the app +being served, not from the CLI, and the ordering hazard that broke it twice is +gone by construction (#10769). + +`serve.ts` reaches optional and enterprise packages through `createHostImporter`, +which anchors resolution at the host app. The helper was bound as a `const` +partway down one very long boot method, so it existed only *below* its own +binding — and a load written above that point was **not** a compile error. The +author simply wrote a bare `import()`, which resolves against the CLI's own +realpath and works fine in a dev checkout where everything is hoisted into one +`node_modules`. It breaks only in a real distribution layout, at boot, in +production. That shipped twice: + +- **cloud#1013** — the binding sat below the auth block, so the enterprise + `@objectstack/organizations` load resolved in the framework workspace, never + found the cloud-private package, and every walled-posture deployment hit the + ADR-0093 D5 fail-fast and exited 1. +- **#10645** — the binding sat below the cluster block, so on the published EE + image `OS_CLUSTER_DRIVER=redis` died at boot with `Cannot find package + '@objectstack/service-cluster'`, and compose's `service_completed_successfully` + took the whole stack down with it. + +Each was fixed by hoisting the binding, which left the class open: the next load +added above the new line reproduces it exactly, and no author has any reason to +know where that line is. `importFromHost` is now a **module-scope function +declaration**, hoisted over the whole module, so "above the definition" is no +longer a state the file can be in — every line of `serve.ts` reaches the same +host-anchored importer, in any order. + +Sweeping the file for the class then turned up one live instance: +`@objectstack/service-i18n` was loaded with a bare `import()`. `packages/cli` +does not declare it, so an app that declares its own copy could only be found by +accident of workspace hoisting — green in a dev checkout, absent on a real +install layout. It is now host-anchored like the rest. An app that does not +declare the package still falls back to the CLI's own resolution, so the quiet +"i18n not installed, use the kernel fallback" path is unchanged. + +Nothing about what `serve` binds, listens on, advertises, or *accepts* moves: +this changes only where a module resolves **from**. The `#4719` declaration gate +is untouched — a package the app has not declared is still refused rather than +picked up from a hoisted store. + +`serve-cluster-host-resolution.test.ts` is widened from the cluster pair to every +app-declarable optional load, classifying mechanically (a package is +app-declarable exactly when `packages/cli`'s own manifest does not declare it) so +a newly added optional package is covered without anyone remembering the test +exists. 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 de6d58a883..578cdd7067 100644 --- a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts @@ -40,15 +40,31 @@ * "any app-declared optional package", not "these two cluster packages", and * a synthetic one needs nothing built. * - * 2. The ORDERING, by source scan: `importFromHost` must be defined ABOVE the - * cluster block. This is the half that actually regressed, twice — the - * helper is a `const` in one long boot function, so a load placed above it - * is not a compile error, it is a silent fall-back to bare resolution. The - * first time it cost the enterprise organizations load (cloud#1013); the - * second time it cost EE multi-node boot outright. + * 2. The REACHABILITY of the helper, by source scan. This is the half that + * actually regressed, twice: `importFromHost` used to be a `const` bound + * partway down one very long boot function, so it existed only BELOW its own + * binding. A load placed above it is not a compile error — the author writes + * a bare `import()`, which resolves from the CLI and is green in any dev + * checkout where everything is hoisted into one `node_modules`. The first + * time it cost the enterprise organizations load (cloud#1013); the second + * time it cost EE multi-node boot outright (#10645). * - * The source scan reads `serve.ts` from THIS package, so no cross-package test - * input is declared or needed. + * #10769 closed the class rather than hoisting a third time: the helper is + * now a module-scope FUNCTION DECLARATION, hoisted over the entire module, so + * "above the definition" is not a state this file can be in. The scan below + * pins that shape — a `const`, or a declaration nested inside a function, + * fails — which is strictly stronger than the ordering check it replaced. + * + * 3. EVERY app-declarable optional load, by source scan (#10769). The cluster + * pair was only the instance that happened to ship. A package is treated as + * app-declarable exactly when `packages/cli`'s own manifest does not declare + * it — mechanically, so a newly added optional package is covered without + * anyone remembering this file. Bare `import()` of such a package fails, and + * a bare `import()` whose specifier the scan cannot resolve must be + * enumerated with its reason. + * + * The source scan reads `serve.ts` and `package.json` from THIS package, so no + * cross-package test input is declared or needed. */ import { describe, it, expect } from 'vitest'; @@ -63,6 +79,212 @@ const HERE = dirname(fileURLToPath(import.meta.url)); /** `packages/cli/src/commands/serve.ts` — same package, no escaping read. */ const SERVE_SOURCE = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); +/** `packages/cli/package.json` — the CLI's OWN declared dependency surface. */ +const CLI_MANIFEST = JSON.parse( + readFileSync(resolve(HERE, '..', '..', 'package.json'), 'utf8'), +) as { + dependencies?: Record; + peerDependencies?: Record; + optionalDependencies?: Record; +}; + +/** + * What the CLI itself declares, and therefore what a bare `import()` from + * `dist/commands/serve.js` can actually resolve. `devDependencies` are + * deliberately excluded: they are not installed beside a published CLI. + */ +const CLI_DECLARES = new Set([ + ...Object.keys(CLI_MANIFEST.dependencies ?? {}), + ...Object.keys(CLI_MANIFEST.peerDependencies ?? {}), + ...Object.keys(CLI_MANIFEST.optionalDependencies ?? {}), +]); + +/** + * Blank out comments, preserving every byte offset and every newline, so the + * sweep below reads CODE only. + * + * This matters more than it looks: `serve.ts` discusses `import()` in prose all + * over its comments (including the note that describes this very defect), and a + * naive scan matches those and reports hazards that do not exist. Strings and + * template literals are tracked so a `'http://…'` literal is not mistaken for a + * line comment. + */ +function stripComments(src: string): string { + const out = src.split(''); + const n = src.length; + let i = 0; + let prevCode = ''; + while (i < n) { + const c = src[i]; + const d = src[i + 1]; + if (c === '/' && d === '/') { + while (i < n && src[i] !== '\n') { out[i] = ' '; i++; } + continue; + } + if (c === '/' && d === '*') { + while (i < n && !(src[i] === '*' && src[i + 1] === '/')) { + if (src[i] !== '\n') out[i] = ' '; + i++; + } + if (i < n) { out[i] = ' '; out[i + 1] = ' '; i += 2; } + continue; + } + if (c === '"' || c === "'") { + i++; + while (i < n && src[i] !== c) { if (src[i] === '\\') i++; i++; } + i++; prevCode = c; continue; + } + if (c === '`') { + i++; + while (i < n && src[i] !== '`') { + if (src[i] === '\\') { i += 2; continue; } + if (src[i] === '$' && src[i + 1] === '{') { + let depth = 1; i += 2; + while (i < n && depth > 0) { + if (src[i] === '{') depth++; + else if (src[i] === '}') depth--; + i++; + } + continue; + } + i++; + } + i++; prevCode = '`'; continue; + } + if (c === '/' && /[=(,:[!&|?+\-*%^~{;]/.test(prevCode)) { // regex literal + i++; + while (i < n && src[i] !== '/') { + if (src[i] === '\\') i++; + else if (src[i] === '[') { while (i < n && src[i] !== ']') { if (src[i] === '\\') i++; i++; } } + i++; + } + i++; prevCode = '/'; continue; + } + if (!/\s/.test(c)) prevCode = c; + i++; + } + return out.join(''); +} + +/** `serve.ts` with comments blanked — offsets and line numbers preserved. */ +const SERVE_CODE = stripComments(SERVE_SOURCE); + +type LoadSite = { + /** 1-based line in `serve.ts`. */ + line: number; + callee: 'import' | 'importFromHost'; + /** The argument source text, whitespace-collapsed. */ + argument: string; + /** The literal specifier, when the scan can determine one statically. */ + specifier?: string; + /** Bare package name of `specifier` (`@scope/name`), when it names a package. */ + packageName?: string; +}; + +/** Read the balanced argument text of the call whose `(` is at `open`. */ +function argumentAt(code: string, open: number): string { + let depth = 0; + let out = ''; + for (let i = open; i < code.length; i++) { + const c = code[i]; + if (c === '(') { depth++; if (depth === 1) continue; } + if (c === ')') { depth--; if (depth === 0) break; } + out += c; + } + return out.replace(/\s+/g, ' ').trim(); +} + +/** `@scope/name/sub` → `@scope/name`. Paths, URLs and `node:` builtins → undefined. */ +function packageNameOf(specifier: string): string | undefined { + if (specifier.startsWith('.') || specifier.startsWith('/') || specifier.includes(':')) { + return undefined; + } + const parts = specifier.split('/'); + return specifier.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]; +} + +/** + * Resolve one level of `const X = ''` — the idiom `serve.ts` uses + * everywhere to keep `tsc` from statically resolving an optional package + * (`const i18nPkg = '@objectstack/service-i18n'`). Without this the sweep would + * see only an identifier and classify every optional load as unknowable. + */ +function resolveIdentifier(code: string, name: string): string | undefined { + const m = code.match( + new RegExp(`\\bconst\\s+${name}\\s*(?::\\s*string\\s*)?=\\s*(['"\`])([^'"\`]*)\\1`), + ); + return m?.[2]; +} + +/** Every dynamic load in `serve.ts`, bare or host-anchored. */ +function collectLoadSites(code: string): LoadSite[] { + const sites: LoadSite[] = []; + const re = /\b(?:await\s+)?(importFromHost|import)\s*\(/g; + let m: RegExpExecArray | null; + while ((m = re.exec(code))) { + const callee = m[1] as LoadSite['callee']; + const open = m.index + m[0].length - 1; + const argument = argumentAt(code, open); + const line = code.slice(0, m.index).split('\n').length; + + let specifier: string | undefined; + const literal = argument.match(/^(['"])([^'"]*)\1$/); + const plainTemplate = argument.match(/^`([^`$]*)`$/); + const prefixTemplate = argument.match(/^`([^`$]*)\$\{/); + const identifier = argument.match(/^([A-Za-z_$][\w$]*)$/); + if (literal) specifier = literal[2]; + else if (plainTemplate) specifier = plainTemplate[1]; + else if (prefixTemplate) specifier = prefixTemplate[1]; // `@objectstack/service-cluster-${driver}` + else if (identifier) specifier = resolveIdentifier(code, identifier[1]); + + sites.push({ + line, + callee, + argument, + specifier, + packageName: specifier ? packageNameOf(specifier) : undefined, + }); + } + return sites; +} + +const LOAD_SITES = collectLoadSites(SERVE_CODE); + +/** + * The class this file exists for: a package `serve` loads that the CLI does NOT + * declare. A bare `import()` from the CLI cannot resolve it except by accident + * of workspace hoisting — which is precisely why the two shipped instances + * passed every dev checkout and died on a distribution image. + */ +const APP_DECLARABLE_LOADS = LOAD_SITES.filter( + (site) => site.packageName?.startsWith('@objectstack/') && !CLI_DECLARES.has(site.packageName), +); + +/** + * Bare `import()` calls whose specifier no source scan can resolve — a member + * expression or a loop/parameter variable. Each is allowlisted BY ITS ARGUMENT + * TEXT (stable across line moves) with the reason it is not the class above. + * A new one fails the test, which is the point: an unknowable specifier is + * exactly where a bare `import()` hides. + */ +const UNRESOLVABLE_BARE_IMPORTS: Record = { + // A filesystem path to the app's own compiled config/artifact, never a package. + // `createHostImporter` passes non-package specifiers through untouched anyway. + "absolutePath.startsWith('/') ? `file://${absolutePath}` : absolutePath": + 'a path to the served artifact, not a package name', + // Loop over a literal pair: '@objectstack/setup', '@objectstack/account'. + // Both are declared by packages/cli, so bare resolution finds them. + appPkg: 'iterates @objectstack/setup + @objectstack/account, both CLI-declared', + // 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 — an app-supplied specifier, + // so this IS the class, but it cannot be classified from source and changing + // it moves app-config plugin loading. Filed separately rather than widened + // here; see the PR for #10769. + plugin: 'app-supplied plugin name from objectstack.config.ts — filed separately', +}; + /** * A host app that DECLARES an optional package and carries it in its own * `node_modules` — the shape of every EE app that declares @@ -137,26 +359,161 @@ describe('os serve → cluster block source shape', () => { expect(SERVE_SOURCE).not.toMatch(/await import\(`@objectstack\/service-cluster-/); }); - it('defines importFromHost ABOVE the cluster block that consumes it', () => { - const definition = SERVE_SOURCE.indexOf('const importFromHost = createHostImporter('); - const clusterUse = SERVE_SOURCE.indexOf('await importFromHost(__clusterPkg)'); + // ── Replaces the former "definition is ABOVE the cluster block" assertion ── + // + // That assertion pinned an ORDERING inside one long boot method, which is the + // shape #10769 removed: `importFromHost` is now a module-scope FUNCTION + // DECLARATION, hoisted over the entire module. The ordering it used to check + // is not merely satisfied, it is unrepresentable — so the check below is the + // strictly stronger one it must be read as. Ordering can only regress again if + // the helper is moved back INSIDE a function, which is exactly what fails here. + it('defines importFromHost at MODULE scope, so no load can sit above it', () => { + const moduleScopeDefinitions = [...SERVE_CODE.matchAll(/^function importFromHost\s*\(/gm)]; + + expect( + moduleScopeDefinitions.length, + 'No module-scope `function importFromHost(...)` in serve.ts. A function ' + + 'DECLARATION at column 0 is hoisted over the whole module, which is what ' + + 'makes "a load written above the helper" impossible. If this was moved back ' + + 'inside the boot method — or turned into a `const`/arrow — the ordering ' + + 'hazard is back: a load placed above it is not a compile error, the author ' + + 'writes a bare `import()`, and it resolves from the CLI. That shipped twice ' + + '(cloud#1013, #10645).', + ).toBe(1); + + // A nested (indented) declaration is scoped to its enclosing function again. + expect( + SERVE_CODE, + 'importFromHost is declared INSIDE a function — module scope is the point.', + ).not.toMatch(/^[ \t]+function importFromHost\s*\(/m); + + // No binding form can re-introduce a temporal dead zone. + expect( + SERVE_CODE, + 'importFromHost is bound with const/let. A binding only exists BELOW itself; ' + + 'that is the defect. Keep it a hoisted function declaration.', + ).not.toMatch(/\b(?:const|let|var)\s+importFromHost\b/); + }); + + it('keeps exactly one host importer, so the helper cannot fork', () => { + // One definition, and one place that builds the underlying importer. + expect([...SERVE_CODE.matchAll(/^function importFromHost\s*\(/gm)]).toHaveLength(1); + expect([...SERVE_CODE.matchAll(/createHostImporter\s*\(/g)]).toHaveLength(1); + }); +}); + +/** + * The detection backstop, widened from the cluster pair to EVERY app-declarable + * optional load in `serve.ts` (#10769). + * + * The structural half of that card makes the ordering hazard unrepresentable + * (`importFromHost` is a hoisted module-scope declaration). This sweep is what + * catches the remaining way in: a load written as a bare `import()` even though + * the helper was reachable. It classifies mechanically rather than from a + * hand-kept list — a package is app-declarable exactly when `packages/cli`'s own + * manifest does not declare it — so a NEW optional package is covered the moment + * it is added, with nobody having to remember this file exists. + */ +describe('os serve → every app-declarable optional load is host-anchored', () => { + it('the sweep actually reads serve.ts (vacuity guard)', () => { + // A sweep that asserts "nothing is wrong" passes trivially when it matches + // nothing. These floors fail loudly instead, so a broken scanner can never + // read as a clean bill of health. + expect(LOAD_SITES.length, 'no dynamic loads found in serve.ts at all').toBeGreaterThan(25); + + const resolvedPackages = LOAD_SITES.filter((s) => s.packageName?.startsWith('@objectstack/')); + expect( + resolvedPackages.length, + 'the specifier resolver stopped resolving — every load now looks unknowable, ' + + 'which would empty the sweep below without failing it', + ).toBeGreaterThan(20); + + expect( + CLI_DECLARES.size, + "packages/cli's manifest read as empty — every package would look app-declarable", + ).toBeGreaterThan(20); + + // Named, not just counted: this proves the resolver still handles all three + // spellings serve.ts uses — a `const` binding, a template prefix, and the + // manifest cross-check that decides app-declarable at all. + const found = new Set(APP_DECLARABLE_LOADS.map((s) => s.packageName)); + for (const pkg of [ + '@objectstack/service-cluster', // const binding (#10645) + '@objectstack/service-cluster-', // template prefix (#10645, the driver) + '@objectstack/organizations', // const binding (cloud#1013) + '@objectstack/service-i18n', // const binding (#10769) + ]) { + expect(found, `the sweep no longer sees the ${pkg} load`).toContain(pkg); + } + expect(APP_DECLARABLE_LOADS.length).toBeGreaterThanOrEqual(4); + }); + + it('reads code, not the prose that discusses `import()` (stripper guard)', () => { + // serve.ts explains this very defect in its comments. If the stripper broke + // OPEN, prose matches would be scanned as loads; if it broke CLOSED it could + // blank real code and empty the sweep. Pin both directions. + expect(SERVE_CODE.length).toBe(SERVE_SOURCE.length); + expect(SERVE_CODE.split('\n').length).toBe(SERVE_SOURCE.split('\n').length); + // A phrase that exists ONLY inside a comment in serve.ts. + expect(SERVE_SOURCE).toContain('Node ESM resolves a bare'); + expect(SERVE_CODE).not.toContain('Node ESM resolves a bare'); + // …and real code either side of the comments survives untouched. + expect(SERVE_CODE).toContain("const __clusterPkg: string = '@objectstack/service-cluster'"); + expect(SERVE_CODE).toContain('function importFromHost('); + }); + + it('never loads an app-declarable optional package through a bare import()', () => { + const bare = APP_DECLARABLE_LOADS.filter((site) => site.callee === 'import'); + + expect( + bare.map((site) => `serve.ts:${site.line} import(${site.argument}) → ${site.packageName}`), + 'These packages are NOT declared by packages/cli, so a bare `import()` resolves ' + + "against the CLI's own realpath and can only find them by accident of workspace " + + 'hoisting — green in a dev checkout, dead at boot on a real distribution layout. ' + + 'That is the exact failure that shipped as cloud#1013 and #10645. Load them with ' + + '`importFromHost(...)`, which is a module-scope declaration reachable from every ' + + 'line of serve.ts. An app that does not declare the package still falls back to ' + + "the CLI's own resolution, so no quiet-skip path changes.", + ).toEqual([]); + }); + + it('enumerates every bare import() whose specifier a scan cannot resolve', () => { + const unresolvable = LOAD_SITES.filter( + (site) => site.callee === 'import' && site.specifier === undefined, + ); + + // Non-vacuity: these sites exist, so an empty list means the scan broke. + expect(unresolvable.length).toBeGreaterThan(0); - expect(definition, 'importFromHost definition not found — was it renamed?').toBeGreaterThan(-1); - expect(clusterUse, 'cluster gate no longer loads via importFromHost').toBeGreaterThan(-1); + const unjustified = unresolvable + .filter((site) => !(site.argument in UNRESOLVABLE_BARE_IMPORTS)) + .map((site) => `serve.ts:${site.line} import(${site.argument})`); - // `const` in one long boot function: a use above the definition is a - // temporal-dead-zone throw at boot, and the load it guards is exactly the - // one that must not fall back to bare resolution. expect( - definition, - 'importFromHost is defined AFTER the cluster block. That is the defect this file ' - + 'pins: every optional load placed above the helper silently resolves from the ' - + "CLI's own node_modules instead of the host app's. Hoist the helper.", - ).toBeLessThan(clusterUse); + unjustified, + 'A new bare `import()` whose specifier this scan cannot resolve. An unknowable ' + + 'specifier is exactly where an app-declared package hides from the sweep above, ' + + 'so it cannot pass silently. Either load it through `importFromHost(...)` — the ' + + 'right answer whenever the specifier can come from the served app — or add it to ' + + 'UNRESOLVABLE_BARE_IMPORTS with the reason it can only ever name a CLI-declared ' + + 'package or a filesystem path.', + ).toEqual([]); }); - it('keeps exactly one host-importer definition, so hoisting cannot fork it', () => { - const definitions = [...SERVE_SOURCE.matchAll(/const importFromHost\s*=/g)]; - expect(definitions).toHaveLength(1); + it('keeps the cluster and organizations loads host-anchored (the shipped instances)', () => { + // The two regressions, pinned by package rather than by line number. + const byPackage = (pkg: string) => APP_DECLARABLE_LOADS.filter((s) => s.packageName === pkg); + for (const pkg of [ + '@objectstack/service-cluster', + '@objectstack/service-cluster-', + '@objectstack/organizations', + '@objectstack/service-i18n', + ]) { + const sites = byPackage(pkg); + expect(sites.length, `no load site found for ${pkg}`).toBeGreaterThan(0); + for (const site of sites) { + expect(site.callee, `serve.ts:${site.line} loads ${pkg} bare`).toBe('importFromHost'); + } + } }); }); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 5dece607de..95a1437c03 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -55,6 +55,7 @@ import { hostImportFailureKind, isDeclaredByHost, readHostDeclaration, + type HostImporter, } from '@objectstack/types/node'; import { printHeader, @@ -209,6 +210,74 @@ type CapabilitySpec = { extras?: Array<{ pkg: string; export: string; identities: CapabilityIdentities }>; }; +const hostImporters = new Map(); + +/** + * Host-anchored dynamic import: load a package **as the app being served + * declares it**, falling back to the CLI's own resolution when the app does not + * declare it (`createHostImporter`, `@objectstack/types/node`). + * + * Node ESM resolves a bare `import(pkg)` against the IMPORTER's own realpath. + * The CLI is reached through a workspace/`link:` dependency, so that realpath is + * inside the FRAMEWORK workspace: a bare import can only see what the framework + * itself installed. A package supplied by the app being served — a cloud-private + * one such as `@objectstack/organizations`, a distribution one such as + * `@objectstack/service-cluster`, or anything a customer installs into their own + * project — is invisible to it no matter what the host app declares. + * + * #4719: "resolve from the host root" means "resolve what the host root + * DECLARES". The host lookup was a CJS require, CJS honours NODE_PATH, and the + * pnpm bin shim exports NODE_PATH pointing at the hoisted workspace store — so + * anything transitively reachable from anywhere in the workspace resolved as if + * the app had declared it, and whether the ADR-0093 D5 wall fired came down to + * how the process was launched. The declaration is the contract; reachability is + * not. This helper only moves where a module resolves FROM; it does not widen + * what `serve` will accept. + * + * ── Why this is a MODULE-SCOPE FUNCTION DECLARATION, not a `const` ─────────── + * + * It used to be `const importFromHost = createHostImporter(hostRoot)` bound + * partway down the boot method, so it existed only BELOW its own binding — and a + * load written above that line was NOT a compile error. The author simply wrote + * a bare `import()`, which resolves from the CLI and works fine in a dev checkout + * where everything is hoisted into one `node_modules`. It breaks only in a real + * distribution layout, at boot, in production. That shipped TWICE: + * + * • cloud#1013 — the binding sat below the AUTH block, so the enterprise + * organizations load resolved in the framework workspace, never found the + * cloud-private package, and every walled-posture deployment hit the + * ADR-0093 D5 fail-fast and exited 1. + * • #10645 — the binding sat below the CLUSTER block, so `serve` could not load + * an app-declared `@objectstack/service-cluster*` at all: on the published EE + * image `OS_CLUSTER_DRIVER=redis` died at boot with `Cannot find package + * '@objectstack/service-cluster'`, and compose's + * `service_completed_successfully` took the whole stack down with it. + * + * Hoisting the binding fixed each instance and left the CLASS open: the next load + * added above the new line reproduces it exactly, and no author has any reason to + * know where that line is. A function declaration at module scope is hoisted over + * the ENTIRE module, so "above the definition" is no longer a state this file can + * be in — every line of `serve.ts`, in any order, reaches the same host-anchored + * importer (#10769). + * + * `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. + * + * @param hostRoot Directory holding the served app's `package.json`. Defaults to + * the process CWD — the same root `serve` reads `objectstack.config.ts` from, and + * the value its boot path computes as `hostRoot`. + */ +function importFromHost(specifier: string, hostRoot: string = process.cwd()): Promise { + // Memoised per root so one boot shares a single host `require`, exactly as the + // one mid-function `const` did before it was hoisted out here. + let importer = hostImporters.get(hostRoot); + if (!importer) { + importer = createHostImporter(hostRoot); + hostImporters.set(hostRoot, importer); + } + return importer(specifier); +} + export default class Serve extends Command { static override description = 'Start ObjectStack server. Reads `objectstack.config.ts` if present; otherwise falls back to `dist/objectstack.json` (or OS_ARTIFACT_PATH, including http(s):// URLs) as a portable artifact.'; @@ -1409,42 +1478,17 @@ export default class Serve extends Command { // keys off it too (#4012). const loggerConfig = { level: bootLogLevel }; - // Host-app package resolution — shared by every optional / enterprise - // package loaded from here down. - // - // Node ESM resolves a bare `import(pkg)` against the IMPORTER's own - // realpath. The CLI is reached through a workspace/`link:` dependency, so - // that realpath is inside the FRAMEWORK workspace: a bare import can only - // see what the framework itself installed. A package supplied by the app - // being served — a cloud-private one such as `@objectstack/organizations`, - // or anything a customer installs into their own project — is invisible - // to it no matter what the host app declares. Resolve from the host root - // instead; the CLI's own resolution stays as the fallback for the - // framework-owned packages the CLI depends on. - // - // #4719: "resolve from the host root" now means "resolve what the host - // root DECLARES". The host lookup was a CJS require, CJS honours - // NODE_PATH, and the pnpm bin shim exports NODE_PATH pointing at the - // hoisted workspace store — so anything transitively reachable from - // anywhere in the workspace resolved as if the app had declared it, and - // whether the D5 wall below fired came down to whether `serve` was reached - // through that shim. The declaration is the contract; reachability is not. + // The root of the app being served: where its `package.json` and + // `objectstack.config.ts` live. `importFromHost` (module scope, top of this + // file) anchors every app-declarable optional load here, and defaults to + // this same `process.cwd()`, so a load written ANYWHERE in this method — + // above this line included — resolves from the app rather than the CLI. + // That reachability is the point: see the helper's own note for the two + // shipped instances (cloud#1013, #10645) that a mid-function binding cost. // - // Defined HERE, at the TOP of the boot sequence, because the very first - // optional package `serve` loads is the cluster gate a few lines below. - // This helper has now been hoisted twice for the same reason, which is the - // point worth keeping: every load placed ABOVE it silently falls back to a - // bare import and can only see the framework's own node_modules. It first - // sat below the auth block, so the enterprise organizations load resolved - // in the framework workspace, never found the cloud-private package, and - // every walled-posture deployment hit the ADR-0093 D5 fail-fast and exited - // 1 (cloud#1013). It then sat below the cluster block, so `serve` could not - // load an app-declared `@objectstack/service-cluster*` at all and EE - // multi-node boot died outright on `OS_CLUSTER_DRIVER=redis`. A new - // optional load added above this line reintroduces the same defect a third - // time — put it below, or hoist this further and say why here. + // #4719: what the host root DECLARES is the contract; being merely + // reachable through a hoisted workspace store is not, and is refused. const hostRoot = process.cwd(); - const importFromHost = createHostImporter(hostRoot); // Cluster wiring: env-driven driver selection (mirrors OS_DATABASE_URL). // The remote driver self-registers on import; import it dynamically so it @@ -1778,9 +1822,15 @@ export default class Serve extends Command { ); if (!hasI18nPlugin && configHasTranslations && tierEnabled('i18n')) { try { - // Dynamic import with variable to prevent tsc from resolving the optional package + // Dynamic import with variable to prevent tsc from resolving the optional package. + // Host-anchored: `packages/cli` does NOT declare @objectstack/service-i18n, + // so a bare import here resolves against the CLI's own realpath and can + // only ever find the package by workspace hoisting — the same defect + // class that cost cloud#1013 and #10645 (#10769). An app that does not + // declare it still falls back to the CLI's resolution, so the quiet-skip + // path below is unchanged. const i18nPkg = '@objectstack/service-i18n'; - const { I18nServicePlugin } = await import(/* webpackIgnore: true */ i18nPkg); + const { I18nServicePlugin } = await importFromHost(i18nPkg); const i18nCfg = config.i18n || config.manifest?.i18n || {}; await kernel.use(new I18nServicePlugin({ defaultLocale: i18nCfg.defaultLocale, @@ -2756,9 +2806,10 @@ export default class Serve extends Command { (p: any) => p.name === 'com.objectstack.service-ai' || p.constructor?.name === 'AIServicePlugin' ); - // `importFromHost` (declared above, before the auth block) resolves - // optional plugin packages from the HOST APP's context — the app being - // served declares them as deps, including private packages like + // `importFromHost` (module scope, hoisted over this whole file — it is no + // longer "declared above" anything, which is the point) resolves optional + // plugin packages from the HOST APP's context — the app being served + // declares them as deps, including private packages like // @objectstack/service-ai-studio that the framework CLI itself does not // depend on. // [CE AI opt-in] Auto-register the headless AI service ONLY when the host From c3723c88041354419cd3ba688616ea654274732c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:41:20 +0000 Subject: [PATCH 2/3] test(cli): decouple the stripper guard from the shape the sibling tests pin Reverse-verifying the structural change turned the stripper guard red as a third failure: it used `function importFromHost(` as its "real code survived comment stripping" marker, so mutating that declaration reddened it for a reason that has nothing to do with the stripper. Its markers are now unrelated to the shape under test, so it reports on the stripper alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- .../cli/src/commands/serve-cluster-host-resolution.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 578cdd7067..b36f168365 100644 --- a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts @@ -458,8 +458,10 @@ describe('os serve → every app-declarable optional load is host-anchored', () expect(SERVE_SOURCE).toContain('Node ESM resolves a bare'); expect(SERVE_CODE).not.toContain('Node ESM resolves a bare'); // …and real code either side of the comments survives untouched. + // Markers deliberately unrelated to the shape the tests above pin, so this + // guard reports on the STRIPPER and never doubles as a second shape check. expect(SERVE_CODE).toContain("const __clusterPkg: string = '@objectstack/service-cluster'"); - expect(SERVE_CODE).toContain('function importFromHost('); + expect(SERVE_CODE).toContain('export default class Serve extends Command {'); }); it('never loads an app-declarable optional package through a bare import()', () => { From bb14311793891c201111620f45e8a910e39effbc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 17:03:13 +0000 Subject: [PATCH 3/3] test(cli): name the filed issue in the unresolvable-import allowlist The `plugin` entry pointed at "filed separately"; it now names #10908 so the next reader can find the decision instead of re-deriving it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- .../src/commands/serve-cluster-host-resolution.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 b36f168365..471797d4b0 100644 --- a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts @@ -278,11 +278,11 @@ 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 — an app-supplied specifier, - // so this IS the class, but it cannot be classified from source and changing - // it moves app-config plugin loading. Filed separately rather than widened - // here; see the PR for #10769. - plugin: 'app-supplied plugin name from objectstack.config.ts — filed separately', + // The app's own `plugins: [...]` config entries — an app-supplied specifier, so + // this IS the class, but no source scan can classify it and host-anchoring it + // changes a user-facing error message plus which copy of a CLI-declared plugin + // wins. Filed as #10908 rather than widened here. + plugin: 'app-supplied plugin name from objectstack.config.ts — see #10908', }; /**