diff --git a/.changeset/host-declared-package-resolution.md b/.changeset/host-declared-package-resolution.md new file mode 100644 index 0000000000..7f51e6710a --- /dev/null +++ b/.changeset/host-declared-package-resolution.md @@ -0,0 +1,64 @@ +--- +"@objectstack/types": minor +"@objectstack/cli": minor +"@objectstack/verify": minor +--- + +feat(types,cli,verify)!: 只解析 host app 声明过的包 —— `NODE_PATH` 不再算数,ADR-0093 D5 那道墙从此与启动方式无关 (#4719) + +**问题:契约写下了,但从没被检查过。** `@objectstack/types/node` 的 +`createHostRequire` 返回一个 CJS `createRequire`,而 CJS 解析认 `NODE_PATH` +(`Module.globalPaths`)。pnpm 生成的 bin shim 第一件事就是 +`export NODE_PATH=/node_modules/.pnpm/node_modules`,于是任何被工作区里 +**任意一个包**传递依赖到的包都能"从 host app 解析成功" —— 跟这个 app 声明了什么毫无关系。 + +实测(cloud `apps/objectos-ee`,当时未声明 `@objectstack/organizations`): +`pnpm start`(经 shim)boot 成功、插件表里有 `Organizations`、ADR-0093 D5 一声不吭; +`node node_modules/@objectstack/cli/bin/run.js serve`(不经 shim)则 +`✖ FATAL: tenancy posture 'isolated' was requested…` 并 exit 1。同一个 app、同一份 +`package.json`、同一个 posture,**只因为进程是怎么被拉起来的**,走出两种结果。 +而 D5 的报错一直在教 operator "declare it in the app's package.json" —— 那正是 +CLI 从来没检查过的那件事。 + +**改法:声明即执行。** 解析前先读 `/package.json`;只有包名出现在 +`dependencies` / `devDependencies` / `optionalDependencies` / `peerDependencies` +的 **键**里,才去 host 的 `node_modules` 里查它。仅仅"能被解析到"不再算数 —— +那正是让契约失效的那个偶然。未声明的包退回到 importing package 自身的解析 +(ESM,不认 `NODE_PATH`),框架自有的包加载路径不受影响。 + +**两种失败从此分开报。** 今天它们都塌成同一条 `MODULE_NOT_FOUND`,补救办法却相反: + +- **未声明** —— 指向"在 app 的 `package.json` 里声明并安装",并说明为什么 + hoisting / `NODE_PATH` 不被接受; +- **声明了但解析不到** —— 明确说这是**安装**问题(`pnpm install`、生产 prune + 砍掉了它、dist 没构建),别再让人回去重看那份已经写对的 `package.json`。 + +分类经新导出的 `hostImportFailureKind(err)` 暴露给调用方;两种错误都仍带 +`code: 'MODULE_NOT_FOUND'`,`isModuleNotFoundError` 的既有判定不变。 + +**BREAKING — 哪类部署会从假绿变红,以及怎么修。** + +1. **靠 hoisting 苟着的部署。** 一个 app 请求了 walled tenancy posture + (`OS_TENANCY_POSTURE=group` / `isolated` 或 `OS_MULTI_ORG_ENABLED=1`)、 + 却没在自己的 `package.json` 里声明 `@objectstack/organizations`,过去经 pnpm + shim 启动能正常 boot —— 现在会命中 ADR-0093 D5 并 exit 1。 + **修法:在那个 app 的 `package.json` 里声明该依赖并安装。** + 这些部署本来就在未声明状态下运行,红的是一直存在的事实,不是新引入的故障: + 同一个 app 不经 shim 启动今天就已经是 exit 1。 + (同样适用于 `@objectstack/service-ai` / `@objectstack/service-ai-studio`,以及 + `bootStack({ multiTenant: true })`、dogfood 的 enterprise 门。) + +2. **`createHostImporter` 的签名变了**,因为它现在需要 host 的**根目录**才能读到 + 那份 manifest,而一个 `NodeRequire` 无法被问出它锚在哪里: + + ```diff + - createHostImporter(createHostRequire(hostRoot)) + + createHostImporter(hostRoot) // 省略参数 = process.cwd(),同旧默认 + ``` + + `createHostRequire` 本身保持不变,仍然导出。 + +新增导出(`@objectstack/types/node`):`HOST_DECLARATION_FIELDS`、 +`HostDeclarationField`、`HostDeclaration`、`readHostDeclaration`、 +`isDeclaredByHost`、`packageNameFromSpecifier`、`HostImportFailureKind`、 +`HOST_IMPORT_FAILURE_KIND`、`hostImportFailureKind`。 diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 7a2ae9148a..ba7a61f040 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -20,7 +20,12 @@ import { graftAuthoredRuntimeMembers, isAppPluginLike } from '../utils/graft-run import { redactConnectionUrl, describeDriverConnection } from '../utils/connection-display.js'; // Shared with @objectstack/verify and the dogfood multi-org probes (#4700) — // node-only, hence the `/node` subpath rather than the edge-safe root export. -import { createHostRequire, createHostImporter } from '@objectstack/types/node'; +import { + createHostImporter, + hostImportFailureKind, + isDeclaredByHost, + readHostDeclaration, +} from '@objectstack/types/node'; import { printHeader, printKV, @@ -1544,14 +1549,22 @@ export default class Serve extends Command { // 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. + // // Defined HERE, above the auth block, because the enterprise organizations // load inside it needs it: this helper used to be declared *after* that // block, so the organizations load fell back to a bare import, 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). - const hostRequire = createHostRequire(); - const importFromHost = createHostImporter(hostRequire); + const hostRoot = process.cwd(); + const importFromHost = createHostImporter(hostRoot); // 5d. Auto-register AuthPlugin (and paired Security/Audit) when the // 'auth' tier is enabled and no auth plugin is already configured. @@ -1795,6 +1808,26 @@ export default class Serve extends Command { // exact footgun this guard closes. const cause = orgErr instanceof Error ? orgErr.message : String(orgErr); if (!resolveAllowDegradedTenancy()) { + // #4719 — TWO ABSENCES, TWO REMEDIES. Until the host lookup was + // gated on the host's declaration, both arrived here as one + // MODULE_NOT_FOUND and got one piece of advice: "declare it in + // the app's package.json". For an operator who HAD declared it + // and whose install was pruned, that sent them to re-read a + // file that was already correct. The importer now says which + // one it is, so this text can too. + const declaration = readHostDeclaration('@objectstack/organizations', hostRoot); + const remedy = + hostImportFailureKind(orgErr) === 'declared-unresolvable' + ? ' • this app DECLARES @objectstack/organizations ' + + `(${declaration.field}: ${JSON.stringify(declaration.specifier)}) — the\n` + + ' declaration is NOT the problem and re-reading package.json will not help.\n' + + ` Repair the INSTALL in ${hostRoot}: run \`pnpm install\`, check that a\n` + + ' production prune did not drop it, and that its dist is actually built — or\n' + : ' • add @objectstack/organizations (the enterprise multi-org runtime) to THIS APP\n' + + " — declare it in the app's package.json and install; the CLI resolves it from the\n" + + ' app, not from the framework it is linked out of. Being merely reachable\n' + + ' through NODE_PATH / a hoisted workspace store is deliberately not enough\n' + + ' (#4719) — that made this wall depend on how the process was launched — or\n'; console.error( chalk.red( `\n ✖ FATAL: tenancy posture '${tenancyPosture}' was requested but ` + @@ -1802,9 +1835,7 @@ export default class Serve extends Command { ' so the organization wall is INACTIVE. Refusing to boot — a deployment that requested\n' + ' multi-organization isolation must not serve traffic without it (ADR-0093 D5).\n\n' + ' Fix one of:\n' + - ' • add @objectstack/organizations (the enterprise multi-org runtime) to THIS APP\n' + - " — declare it in the app's package.json and install; the CLI resolves it from the\n" + - ' app, not from the framework it is linked out of — or\n' + + remedy + " • set OS_TENANCY_POSTURE=single (or unset OS_MULTI_ORG_ENABLED) to run single-org, or\n" + ' • set OS_ALLOW_DEGRADED_TENANCY=1 to boot in an explicitly degraded single-org state.\n\n' + ` cause: ${cause}\n`, @@ -2031,20 +2062,14 @@ export default class Serve extends Command { // surface), while MCP and every other capability are unaffected. Gating on // a *declared* dep — not mere resolvability — makes this reliable in a // workspace/monorepo, where the package stays hoist-resolvable when undeclared. - const _fs = await import('node:fs'); - const hostDeclaresDependency = (pkg: string): boolean => { - try { - const hostPkg = JSON.parse( - _fs.readFileSync(hostRequire.resolve('./package.json'), 'utf8'), - ) as Record | undefined>; - return Boolean( - hostPkg.dependencies?.[pkg] ?? hostPkg.devDependencies?.[pkg] - ?? hostPkg.optionalDependencies?.[pkg] ?? hostPkg.peerDependencies?.[pkg], - ); - } catch { - return false; - } - }; + // + // #4719 — this used to be a local re-implementation of that read. It was + // right, and it was the ONLY place in the boot path that asked the question + // the right way: the enterprise organizations load two blocks up asked + // "does it resolve", which a hoisted store answered yes to regardless. Both + // now go through the one owner in `@objectstack/types/node`, so "declared" + // cannot mean two different things in one file (Prime Directive #12). + const hostDeclaresDependency = (pkg: string): boolean => isDeclaredByHost(pkg, hostRoot); // `wantsAiService` is the AUTO (opt-in) signal: the host app listed the base // AI service — or the Studio that builds on it — in its OWN package.json. This // is a package.json READ (a deliberate authoring act), not a speculative diff --git a/packages/cli/test/serve-organizations-host-resolution.e2e.test.ts b/packages/cli/test/serve-organizations-host-resolution.e2e.test.ts index d4c6a2eb6e..782fda1c31 100644 --- a/packages/cli/test/serve-organizations-host-resolution.e2e.test.ts +++ b/packages/cli/test/serve-organizations-host-resolution.e2e.test.ts @@ -76,6 +76,35 @@ export class OrganizationsPlugin { let appWithPackage: string; /** The same app WITHOUT it — the fail-fast must still fire. */ let appWithoutPackage: string; +/** + * #4719 — an app that declares NOTHING, run with `NODE_PATH` pointing at a + * store that carries the enterprise package. This is not a contrivance: it is + * verbatim what a pnpm bin shim does before it execs the CLI — + * + * export NODE_PATH="/node_modules/.pnpm/node_modules" + * + * — and CJS resolution honours it, so `serve` used to boot a walled posture for + * an app that had never asked for the multi-org runtime. Measured on cloud's + * `apps/objectos-ee`: `pnpm start` (through the shim) booted silently, while + * `node …/@objectstack/cli/bin/run.js serve` on the same app hit D5 and exited 1. + */ +let hoistedStore: string; + +function writeOrganizationsPackage(root: string): void { + const pkgDir = join(root, '@objectstack', 'organizations'); + mkdirSync(pkgDir, { recursive: true }); + writeFileSync( + join(pkgDir, 'package.json'), + JSON.stringify({ + name: '@objectstack/organizations', + version: '0.0.0-fixture', + type: 'module', + main: 'index.js', + }), + 'utf8', + ); + writeFileSync(join(pkgDir, 'index.js'), FAKE_ORGANIZATIONS, 'utf8'); +} function writeApp(prefix: string, opts: { withOrganizations: boolean }): string { const dir = mkdtempSync(join(tmpdir(), prefix)); @@ -96,31 +125,19 @@ function writeApp(prefix: string, opts: { withOrganizations: boolean }): string ), 'utf8', ); - if (opts.withOrganizations) { - const pkgDir = join(dir, 'node_modules', '@objectstack', 'organizations'); - mkdirSync(pkgDir, { recursive: true }); - writeFileSync( - join(pkgDir, 'package.json'), - JSON.stringify({ - name: '@objectstack/organizations', - version: '0.0.0-fixture', - type: 'module', - main: 'index.js', - }), - 'utf8', - ); - writeFileSync(join(pkgDir, 'index.js'), FAKE_ORGANIZATIONS, 'utf8'); - } + if (opts.withOrganizations) writeOrganizationsPackage(join(dir, 'node_modules')); return dir; } beforeAll(() => { appWithPackage = writeApp('os-org-host-ok-', { withOrganizations: true }); appWithoutPackage = writeApp('os-org-host-missing-', { withOrganizations: false }); + hoistedStore = mkdtempSync(join(tmpdir(), 'os-org-hoisted-store-')); + writeOrganizationsPackage(hoistedStore); }); afterAll(() => { - for (const dir of [appWithPackage, appWithoutPackage]) { + for (const dir of [appWithPackage, appWithoutPackage, hoistedStore]) { if (dir) rmSync(dir, { recursive: true, force: true }); } }); @@ -184,4 +201,38 @@ describe('os serve — enterprise organizations resolution (cloud#1013)', () => }, 300_000, ); + + it( + 'refuses when the package is reachable only through NODE_PATH — the pnpm shim shape (#4719)', + async () => { + // The #4719 defect, over a real process, with the launcher reproduced + // exactly. Same app as the case above (declares nothing), same posture — + // the only difference is the NODE_PATH every pnpm bin shim exports. Before + // this change that single environment variable was enough to boot the + // organization wall off a package the app had never declared, so whether + // ADR-0093 D5 fired came down to HOW the process was started. + const port = randomPort(); + const { stdout, stderr } = await runServe(appWithoutPackage, ['--port', port], { + waitFor: /Press Ctrl\+C to stop/, + env: { ...SERVE_ENV, NODE_PATH: hoistedStore }, + timeoutMs: 240_000, + }); + + const seen = `\n--- stdout ---\n${stdout.slice(-4000)}\n--- stderr ---\n${stderr.slice(-4000)}`; + expect( + stderr, + `NODE_PATH got an undeclared app past the D5 wall — the #4719 defect${seen}`, + ).toMatch(/FATAL: tenancy posture 'isolated' was requested/); + // …and the remedy is the declaration one, naming why reachability lost. + expect(stderr).toMatch(/to THIS APP/); + expect(stderr).toMatch(/NODE_PATH/); + expect(stdout, `serve served traffic without the wall${seen}`).not.toContain( + 'Press Ctrl+C to stop', + ); + expect(stdout, `the hoisted package was mounted anyway${seen}`).not.toContain( + 'Organizations', + ); + }, + 300_000, + ); }); diff --git a/packages/qa/dogfood/test/enterprise-organizations.test.ts b/packages/qa/dogfood/test/enterprise-organizations.test.ts index f533bc5ba0..584149605a 100644 --- a/packages/qa/dogfood/test/enterprise-organizations.test.ts +++ b/packages/qa/dogfood/test/enterprise-organizations.test.ts @@ -22,8 +22,20 @@ import { probeOrganizations, MULTI_ORG_ENV, ORGANIZATIONS_PKG } from './enterpri let hostWithPkg: string; let hostWithoutPkg: string; +/** + * #4719 — the package is physically INSTALLED in the app's own `node_modules` + * and the app's `package.json` never mentions it. That is what a hoisted + * workspace store (or a `NODE_PATH` a pnpm bin shim exported) looks like to the + * resolver, and it used to read as AVAILABLE. + */ +let hostInstalledButUndeclared: string; -function writeHost(prefix: string, withPkg: boolean): string { +function writeHost( + prefix: string, + withPkg: boolean, + opts: { declare?: boolean } = {}, +): string { + const declare = opts.declare ?? withPkg; const dir = mkdtempSync(join(tmpdir(), prefix)); writeFileSync( join(dir, 'package.json'), @@ -31,7 +43,7 @@ function writeHost(prefix: string, withPkg: boolean): string { name: 'dogfood-host-fixture', private: true, type: 'module', - ...(withPkg ? { dependencies: { [ORGANIZATIONS_PKG]: '*' } } : {}), + ...(declare ? { dependencies: { [ORGANIZATIONS_PKG]: '*' } } : {}), }), 'utf8', ); @@ -60,10 +72,11 @@ function writeHost(prefix: string, withPkg: boolean): string { beforeAll(() => { hostWithPkg = writeHost('os-dogfood-org-ok-', true); hostWithoutPkg = writeHost('os-dogfood-org-missing-', false); + hostInstalledButUndeclared = writeHost('os-dogfood-org-undeclared-', true, { declare: false }); }); afterAll(() => { - for (const dir of [hostWithPkg, hostWithoutPkg]) { + for (const dir of [hostWithPkg, hostWithoutPkg, hostInstalledButUndeclared]) { if (dir) rmSync(dir, { recursive: true, force: true }); } }); @@ -99,4 +112,24 @@ describe('enterprise multi-org probe (#4700)', () => { it('does not throw when the run declares the package AND it is there', async () => { await expect(probeOrganizations(hostWithPkg, true)).resolves.toEqual({ available: true }); }); + + it('reports UNAVAILABLE when the package is merely PRESENT but not declared (#4719)', async () => { + // The gate is the app's declaration, not what happens to be reachable. Under + // the old resolver this host answered AVAILABLE — same bytes on disk, same + // package.json, and the multi-org gates would run for an app that never + // asked for the enterprise runtime. Worse, in a real pnpm workspace the + // "present" half arrives via the bin shim's NODE_PATH, so the verdict moved + // with the launcher. + const probe = await probeOrganizations(hostInstalledButUndeclared, false); + expect(probe.available).toBe(false); + expect(probe.reason).toContain("package.json"); + }); + + it('THROWS with a DECLARE-it remedy when the run declares it but the app does not (#4719)', async () => { + // The remedy has to be the one that works. "Install it" is unfollowable + // advice here — it is already installed; the missing act is declaring it. + await expect(probeOrganizations(hostInstalledButUndeclared, true)).rejects.toThrow( + new RegExp(`declare ${ORGANIZATIONS_PKG.replace('/', '\\/')} in .* package\\.json`), + ); + }); }); diff --git a/packages/qa/dogfood/test/enterprise-organizations.ts b/packages/qa/dogfood/test/enterprise-organizations.ts index 7b95b5764c..1c2d868dc2 100644 --- a/packages/qa/dogfood/test/enterprise-organizations.ts +++ b/packages/qa/dogfood/test/enterprise-organizations.ts @@ -45,9 +45,28 @@ * resolver, and a cloud/enterprise run that ships the package will actually * execute the blocks (and will fail loudly if it thinks it ships the package but * does not). + * + * ── #4719 ──────────────────────────────────────────────────────────────────── + * + * The shared resolver now looks a package up in the host app only when that app + * DECLARES it (`dependencies` / `devDependencies` / `optionalDependencies` / + * `peerDependencies`). It used to honour `NODE_PATH`, which every pnpm bin shim + * exports at the hoisted workspace store — so in a workspace that happened to + * carry the enterprise package anywhere, this probe could report AVAILABLE for + * an app that never declared it, and the multi-org gates would run (or not) + * according to how the runner was launched. A capability probe whose answer + * moves with the launcher is the same class of lie as the constant-false one + * this file replaced, so the probe now reads the declaration too. + * + * Consequence for an enterprise/cloud run: `OS_TEST_MULTI_ORG_ENABLED=1` + * requires the app under test — the `hostRoot` passed here, defaulting to the + * CWD — to DECLARE `@objectstack/organizations` in its own `package.json`. + * Shipping it only as a hoisted transitive dependency of something else is no + * longer enough. The failure text below says so explicitly rather than leaving + * it to be rediscovered. */ -import { createHostImporter, createHostRequire } from '@objectstack/types/node'; +import { createHostImporter, hostImportFailureKind } from '@objectstack/types/node'; /** The cloud-private enterprise package (ADR-0081 D2). */ export const ORGANIZATIONS_PKG = '@objectstack/organizations'; @@ -77,28 +96,38 @@ export async function probeOrganizations( hostRoot?: string, declared: boolean = process.env[MULTI_ORG_ENV] === '1', ): Promise { - const importFromHost = createHostImporter(createHostRequire(hostRoot)); + const root = hostRoot ?? process.cwd(); + const importFromHost = createHostImporter(root); try { await importFromHost(ORGANIZATIONS_PKG); return { available: true }; } catch (e) { const detail = (e as Error).message; + // #4719 — name the remedy that actually applies. "Install it" is useless + // advice for an app that installed it and never declared it, which is the + // shape a workspace makes easy and a hoisted store used to hide. + const remedy = + hostImportFailureKind(e) === 'declared-unresolvable' + ? `${root} DECLARES ${ORGANIZATIONS_PKG}, so repair its INSTALL there (\`pnpm install\`, ` + + 'un-prune, rebuild its dist)' + : `declare ${ORGANIZATIONS_PKG} in ${root}'s own package.json and install it — being ` + + 'reachable as somebody else\'s transitive dependency is not enough (#4719)'; if (declared) { throw new Error( `${MULTI_ORG_ENV}=1 declares that ${ORGANIZATIONS_PKG} (enterprise, ADR-0081 D2) is ` + - `installed for this run, but it could not be resolved from ${hostRoot ?? process.cwd()}. ` + + `installed for this run, but it could not be resolved from ${root}. ` + 'Refusing to skip the multi-org dogfood gates silently: a run that believes it is ' + 'exercising cross-tenant isolation and is not would report green over gates that ' + - `never executed. Install/link ${ORGANIZATIONS_PKG} into the app under test, or unset ` + + `never executed. To fix: ${remedy}; or unset ` + `${MULTI_ORG_ENV} to accept the skip. (${detail})`, ); } return { available: false, reason: - `${ORGANIZATIONS_PKG} (enterprise) is not resolvable from ${hostRoot ?? process.cwd()} — ` + - `skipping the multi-org gate. Set ${MULTI_ORG_ENV}=1 in a run that ships the package to ` + - 'turn this skip into a failure. ' + + `${ORGANIZATIONS_PKG} (enterprise) is not resolvable from ${root} — ` + + `skipping the multi-org gate. To enable it, ${remedy}. Set ${MULTI_ORG_ENV}=1 in a run ` + + 'that ships the package to turn this skip into a failure. ' + `(${detail})`, }; } diff --git a/packages/types/src/node-isolation.test.ts b/packages/types/src/node-isolation.test.ts index 523969f990..2f1f1ae920 100644 --- a/packages/types/src/node-isolation.test.ts +++ b/packages/types/src/node-isolation.test.ts @@ -118,6 +118,10 @@ describe('@objectstack/types — node-only code stays behind the ./node subpath // subpath would look justified when it no longer was. const specs = specifiersOf(join(SRC, 'node.ts')); expect(specs.filter((s) => s.startsWith('node:')).sort()).toEqual([ + // #4719 added `node:fs`: the host lookup is gated on what the host app's + // package.json DECLARES, which means reading that file. One more reason + // this slice can never move back behind the root export. + 'node:fs', 'node:module', 'node:path', 'node:url', diff --git a/packages/types/src/node.test.ts b/packages/types/src/node.test.ts index 6f25b23c83..b1be2435ed 100644 --- a/packages/types/src/node.test.ts +++ b/packages/types/src/node.test.ts @@ -2,8 +2,9 @@ /** * cloud#1013 / #4700 — resolving a host-app package from a framework package. + * #4719 — and only when the host app DECLARES it. * - * The defect: `serve` loaded `@objectstack/organizations` with a BARE + * The first defect: `serve` loaded `@objectstack/organizations` with a BARE * `import()`. Node ESM resolves that against the importer's own realpath — the * framework package's, inside the framework workspace — while the package is * cloud-private and only ever exists in the host app's `node_modules`. It could @@ -13,22 +14,43 @@ * why the resolver moved here from `packages/cli/src/utils/import-from-host.ts`: * one behaviour, one source. * - * These cases run against a REAL fixture app on disk (a real `node_modules`, - * real resolution, nothing mocked): the first two are the issue's own repro, - * one half per case — the framework package's resolution cannot see the package, - * the host app's can. + * The second defect (#4719) is the fix's own: "resolve from the host app" was a + * CJS `createRequire`, and CJS resolution honours `NODE_PATH` — which every pnpm + * bin shim exports, pointing at the hoisted workspace store. So ANY package + * transitively reachable from anywhere in the workspace resolved "from the host + * app", whatever that app declared, and D5's "declare it in the app's + * package.json" was advice about a thing nothing checked. The gate is now the + * DECLARATION; reachability is refused. + * + * These cases run against REAL fixture apps on disk (a real `node_modules`, real + * resolution, a real `NODE_PATH`, nothing mocked). */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import * as NodeModule from 'node:module'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { createHostImporter, createHostRequire } from './node.js'; +import { + createHostImporter, + createHostRequire, + hostImportFailureKind, + isDeclaredByHost, + packageNameFromSpecifier, + readHostDeclaration, +} from './node.js'; /** The cloud-private package at the heart of cloud#1013. */ const ORGANIZATIONS = '@objectstack/organizations'; /** A package that fails while it EVALUATES — not while it resolves. */ const BROKEN = '@fixture/throws-on-load'; +/** + * Installed ONLY in the `NODE_PATH` store — i.e. exactly the shape a pnpm bin + * shim puts every transitively-reachable workspace package into (#4719). + */ +const HOISTED_ONLY = '@fixture/hoisted-only'; +/** Declared by the host app and installed nowhere at all. */ +const DECLARED_MISSING = '@fixture/declared-but-missing'; /** * A directory inside the framework workspace — what a bare `import()` from a @@ -39,7 +61,21 @@ const BROKEN = '@fixture/throws-on-load'; */ const PACKAGE_ROOT = process.cwd(); +/** Host app that declares (and installs) what it uses — the supported shape. */ let hostRoot: string; +/** Host app that declares NOTHING, used against the NODE_PATH store below. */ +let undeclaringRoot: string; +/** A directory with no `package.json` at all. */ +let manifestlessRoot: string; +/** Stands in for `/node_modules/.pnpm/node_modules`. */ +let nodePathStore: string; + +const originalNodePath = process.env.NODE_PATH; + +/** `Module.globalPaths` is derived from NODE_PATH once, at startup. */ +function reloadNodePath(): void { + (NodeModule as unknown as { _initPaths: () => void })._initPaths(); +} function writeFixturePackage(root: string, name: string, indexJs: string): void { const dir = join(root, 'node_modules', ...name.split('/')); @@ -53,9 +89,9 @@ function writeFixturePackage(root: string, name: string, indexJs: string): void } beforeAll(() => { - // A host app exactly as the fix expects one: it DECLARES the enterprise - // package and has it installed in its own node_modules. The framework - // workspace this package lives in has neither. + // A host app exactly as the fix expects one: it DECLARES the packages it uses + // and has them installed in its own node_modules. The framework workspace this + // package lives in has neither. hostRoot = mkdtempSync(join(tmpdir(), 'os-import-from-host-')); writeFileSync( join(hostRoot, 'package.json'), @@ -63,6 +99,14 @@ beforeAll(() => { name: 'host-app-fixture', type: 'module', dependencies: { [ORGANIZATIONS]: '*' }, + // #4719 fixture amendment: the evaluation-crash case below imports this + // package, and an undeclared name is no longer looked up in the host's + // node_modules at all — so the crash it exists to prove would be masked by + // an "undeclared" verdict. Declaring it is the correct fix (the fixture app + // really does depend on it); loosening the gate would not be. + devDependencies: { [BROKEN]: '*' }, + // Declared, deliberately never installed anywhere. + optionalDependencies: { [DECLARED_MISSING]: '*' }, }), 'utf8', ); @@ -72,10 +116,44 @@ beforeAll(() => { 'export class OrganizationsPlugin { name = "com.objectstack.organizations"; }\n', ); writeFixturePackage(hostRoot, BROKEN, 'throw new Error("fixture package exploded on import");\n'); + + undeclaringRoot = mkdtempSync(join(tmpdir(), 'os-import-undeclared-')); + writeFileSync( + join(undeclaringRoot, 'package.json'), + JSON.stringify({ name: 'undeclaring-app-fixture', type: 'module', dependencies: {} }), + 'utf8', + ); + + manifestlessRoot = mkdtempSync(join(tmpdir(), 'os-import-no-manifest-')); + + // The hoisted store, and NODE_PATH pointed at it — the pnpm bin shim's first + // act, reproduced. `Module.globalPaths` is rebuilt so an ALREADY-CREATED + // `require` sees it, which is what makes this the issue's exact repro. + nodePathStore = mkdtempSync(join(tmpdir(), 'os-node-path-store-')); + const pkgDir = join(nodePathStore, ...HOISTED_ONLY.split('/')); + mkdirSync(pkgDir, { recursive: true }); + writeFileSync( + join(pkgDir, 'package.json'), + JSON.stringify({ + name: HOISTED_ONLY, + version: '0.0.0-fixture', + type: 'module', + main: 'index.js', + }), + 'utf8', + ); + writeFileSync(join(pkgDir, 'index.js'), 'export const hoisted = true;\n', 'utf8'); + process.env.NODE_PATH = originalNodePath ? `${nodePathStore}:${originalNodePath}` : nodePathStore; + reloadNodePath(); }); afterAll(() => { - if (hostRoot) rmSync(hostRoot, { recursive: true, force: true }); + if (originalNodePath === undefined) delete process.env.NODE_PATH; + else process.env.NODE_PATH = originalNodePath; + reloadNodePath(); + for (const dir of [hostRoot, undeclaringRoot, manifestlessRoot, nodePathStore]) { + if (dir) rmSync(dir, { recursive: true, force: true }); + } }); describe('host-app package resolution (cloud#1013, #4700)', () => { @@ -90,32 +168,24 @@ describe('host-app package resolution (cloud#1013, #4700)', () => { }); it('resolves a package that exists ONLY in the host app', async () => { - const importFromHost = createHostImporter(createHostRequire(hostRoot)); + const importFromHost = createHostImporter(hostRoot); const mod = await importFromHost(ORGANIZATIONS); // The export `serve` and `bootStack` construct: `new mod.OrganizationsPlugin()`. expect(typeof mod.OrganizationsPlugin).toBe('function'); expect(new mod.OrganizationsPlugin().name).toBe('com.objectstack.organizations'); }); - it("falls back to the importing package's own resolution when the host cannot resolve", async () => { - // Whatever the host app cannot see must still load from the framework + it("falls back to the importing package's own resolution when the host does not declare", async () => { + // Whatever the host app does not declare must still load from the framework // package's own dependencies — that fallback is what keeps every // framework-owned load in `serve` (plugin-auth, plugin-security, - // service-i18n, …) and in `bootStack` working exactly as before. Modelled - // with a host `require` that resolves nothing, because a real one cannot: - // vitest exports NODE_PATH into the test process, so every package in the - // workspace store resolves from any directory. - const blindHostRequire = { - resolve(pkg: string): string { - throw Object.assign(new Error(`Cannot find module '${pkg}'`), { code: 'MODULE_NOT_FOUND' }); - }, - } as unknown as NodeRequire; - const mod = await createHostImporter(blindHostRequire)('@objectstack/spec'); + // service-i18n, …) and in `bootStack` working exactly as before. + const mod = await createHostImporter(undeclaringRoot)('@objectstack/spec'); expect(mod).toBeTypeOf('object'); }); it('reports a package that neither can resolve as module-not-found', async () => { - const importFromHost = createHostImporter(createHostRequire(hostRoot)); + const importFromHost = createHostImporter(hostRoot); // Callers classify "missing vs crashed" off this error (Serve. // isModuleNotFoundError), so the absent case must stay recognisable. await expect(importFromHost('@fixture/nowhere-at-all')).rejects.toThrow( @@ -129,7 +199,188 @@ describe('host-app package resolution (cloud#1013, #4700)', () => { // would swap the real cause for a MODULE_NOT_FOUND, which every caller // reads as "not installed" — a crash silently downgraded to a skip, or a // fatal telling the operator to install what is already installed. - const importFromHost = createHostImporter(createHostRequire(hostRoot)); + const importFromHost = createHostImporter(hostRoot); await expect(importFromHost(BROKEN)).rejects.toThrow(/fixture package exploded on import/); + const err = await importFromHost(BROKEN).catch((e: unknown) => e); + expect(hostImportFailureKind(err)).toBeUndefined(); + }); +}); + +describe('declaration gates the host lookup (#4719)', () => { + it('PRECONDITION: NODE_PATH really does make an undeclared package resolvable', () => { + // The whole defect in one assertion. This is the resolution the previous + // implementation performed and trusted: a CJS `require` anchored at an app + // that declares NOTHING, finding a package because the launcher exported + // NODE_PATH. It succeeds — which is why the guard below has to exist. + const resolved = createHostRequire(undeclaringRoot).resolve(HOISTED_ONLY); + expect(resolved).toContain(nodePathStore); + expect(isDeclaredByHost(HOISTED_ONLY, undeclaringRoot)).toBe(false); + }); + + it('REFUSES an undeclared package even though NODE_PATH resolves it', async () => { + // THE case. Before #4719 this imported the hoisted copy and reported + // success, so `objectstack serve` booted a walled posture for an app that + // had never declared the enterprise runtime — but only when launched + // through a pnpm shim. Now the manifest is the answer, in every launcher. + const importFromHost = createHostImporter(undeclaringRoot); + const err = await importFromHost(HOISTED_ONLY).catch((e: unknown) => e); + expect(hostImportFailureKind(err)).toBe('undeclared'); + expect((err as Error).message).toMatch(/does not declare it/); + expect((err as Error).message).toMatch(/Declare it in that app's package\.json/); + }); + + it('ACCEPTS the same package once the app declares it', async () => { + // Same package, same NODE_PATH store, same process — only the manifest + // differs. That is the contract: the declaration decides, not the layout. + const declaringRoot = mkdtempSync(join(tmpdir(), 'os-import-declared-')); + try { + writeFileSync( + join(declaringRoot, 'package.json'), + JSON.stringify({ name: 'declaring-app', dependencies: { [HOISTED_ONLY]: '*' } }), + 'utf8', + ); + const mod = await createHostImporter(declaringRoot)(HOISTED_ONLY); + expect(mod.hoisted).toBe(true); + } finally { + rmSync(declaringRoot, { recursive: true, force: true }); + } + }); + + it('DECLARED but unresolvable is a broken install, worded as one', async () => { + // The other half of the fail-fast contract: the two failures used to + // collapse into one MODULE_NOT_FOUND with opposite remedies. An operator + // who has already declared the package must not be sent back to the + // package.json they just edited. + const err = await createHostImporter(hostRoot)(DECLARED_MISSING).catch((e: unknown) => e); + expect(hostImportFailureKind(err)).toBe('declared-unresolvable'); + expect((err as Error).message).toMatch(/DECLARES it \(optionalDependencies: "\*"\)/); + expect((err as Error).message).toMatch(/INSTALL problem, not a declaration problem/); + expect((err as Error).message).not.toMatch(/does not declare it/); + }); + + it('both failures stay classifiable as module-not-found for existing callers', async () => { + // `serve`'s optional-plugin guards and the `requires` resolver branch on + // `isModuleNotFoundError`; neither new error may fall out of that class. + for (const [root, pkg] of [ + [undeclaringRoot, HOISTED_ONLY], + [hostRoot, DECLARED_MISSING], + ] as const) { + const err = await createHostImporter(root)(pkg).catch((e: { code?: string }) => e); + expect(err.code).toBe('MODULE_NOT_FOUND'); + } + }); + + it('a directory with no package.json declares nothing, and says so', async () => { + const decl = readHostDeclaration(HOISTED_ONLY, manifestlessRoot); + expect(decl).toMatchObject({ declared: false, manifestMissing: true }); + await expect(createHostImporter(manifestlessRoot)(HOISTED_ONLY)).rejects.toThrow( + /no readable package\.json was found there/, + ); + }); +}); + +describe('what counts as a declaration (#4719)', () => { + it('reads all four declaration fields, and names the one it found', () => { + const root = mkdtempSync(join(tmpdir(), 'os-decl-fields-')); + try { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'fields-fixture', + dependencies: { a: '^1.0.0' }, + devDependencies: { b: '^2.0.0' }, + optionalDependencies: { c: '^3.0.0' }, + peerDependencies: { d: '^4.0.0' }, + }), + 'utf8', + ); + expect(readHostDeclaration('a', root)).toMatchObject({ + declared: true, + field: 'dependencies', + }); + expect(readHostDeclaration('b', root)).toMatchObject({ + declared: true, + field: 'devDependencies', + }); + expect(readHostDeclaration('c', root)).toMatchObject({ + declared: true, + field: 'optionalDependencies', + }); + expect(readHostDeclaration('d', root)).toMatchObject({ + declared: true, + field: 'peerDependencies', + }); + expect(readHostDeclaration('e', root).declared).toBe(false); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('accepts workspace / link / alias specifiers — the KEY is the declaration', () => { + const root = mkdtempSync(join(tmpdir(), 'os-decl-specifiers-')); + try { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'specifier-fixture', + dependencies: { + '@objectstack/organizations': 'workspace:*', + local: 'link:../local', + aliased: 'npm:@acme/real@1.2.3', + }, + }), + 'utf8', + ); + // A `workspace:` / `link:` value is the package manager's business; the + // authoring act this gate reads is the KEY. + expect(readHostDeclaration('@objectstack/organizations', root)).toMatchObject({ + declared: true, + specifier: 'workspace:*', + }); + expect(isDeclaredByHost('local', root)).toBe(true); + // Alias deps need no special case: `import('aliased')` is what the app can + // write, and `aliased` is the key. The aliased TARGET is not importable by + // that name, and correctly reads as undeclared. + expect(isDeclaredByHost('aliased', root)).toBe(true); + expect(isDeclaredByHost('@acme/real', root)).toBe(false); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('strips subpaths and keeps scopes when finding the declared name', () => { + expect(packageNameFromSpecifier('@objectstack/platform-objects/plugin')).toBe( + '@objectstack/platform-objects', + ); + expect(packageNameFromSpecifier('@objectstack/organizations')).toBe( + '@objectstack/organizations', + ); + expect(packageNameFromSpecifier('chalk/dist/x.js')).toBe('chalk'); + // Not bare package names — nothing a package.json can declare, so they + // bypass the gate entirely rather than being refused. + expect(packageNameFromSpecifier('./local.js')).toBeUndefined(); + expect(packageNameFromSpecifier('/abs/path.js')).toBeUndefined(); + expect(packageNameFromSpecifier('node:fs')).toBeUndefined(); + expect(packageNameFromSpecifier('file:///tmp/x.mjs')).toBeUndefined(); + }); + + it('a declared subpath import is gated by its package NAME, not the subpath', async () => { + const root = mkdtempSync(join(tmpdir(), 'os-decl-subpath-')); + try { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'subpath-fixture', dependencies: { [HOISTED_ONLY]: '*' } }), + 'utf8', + ); + // Declared by package name ⇒ the subpath IS looked up in the host. The + // fixture has no such file, so this fails on the subpath — as a broken + // install, never as an undeclared package. + const err = await createHostImporter(root)(`${HOISTED_ONLY}/missing-subpath`).catch( + (e: unknown) => e, + ); + expect(hostImportFailureKind(err)).not.toBe('undeclared'); + } finally { + rmSync(root, { recursive: true, force: true }); + } }); }); diff --git a/packages/types/src/node.ts b/packages/types/src/node.ts index 4f68d88f50..4e02a575c8 100644 --- a/packages/types/src/node.ts +++ b/packages/types/src/node.ts @@ -50,11 +50,55 @@ * `MODULE_NOT_FOUND`, which every caller here classifies as "not installed" — * turning a broken package into a silent skip (or, on the organizations path, * into a fatal message telling the operator to install what is already there). + * + * ── #4719: the host's DECLARATION gates the lookup, not its resolvability ──── + * + * "Resolve from the host app" was implemented as a CJS `createRequire` anchored + * at the host's `package.json`, and **CJS resolution honours `NODE_PATH`** + * (`Module.globalPaths`). The first thing a pnpm-generated bin shim does is + * + * export NODE_PATH="/node_modules/.pnpm/node_modules" + * + * and every `serve` / `dev` child process inherits it. Everything any package in + * the workspace transitively depends on lives in that hoisted store, so + * `hostRequire.resolve(pkg)` succeeded for packages the host app had never + * declared — the answer depended on HOW THE PROCESS WAS LAUNCHED, not on the + * app. Measured on cloud's `apps/objectos-ee`, which did not declare + * `@objectstack/organizations`: `pnpm start` (through the shim) booted with the + * organizations plugin mounted and ADR-0093 D5 silent, while + * `node node_modules/@objectstack/cli/bin/run.js serve` (no shim, no NODE_PATH) + * hit the D5 fail-fast and exited 1. Same app, same `package.json`, same + * posture. D5's own message told operators to "declare it in the app's + * package.json" — the one thing the CLI never checked. + * + * So the host lookup is now gated on the host's **declaration**: a package name + * is looked up in the host's `node_modules` only when it appears in the host + * `package.json` (see {@link HOST_DECLARATION_FIELDS}). Reachability through a + * hoisted store or `NODE_PATH` is deliberately not accepted — it is precisely + * the accident that made the contract unenforced. This is the "declared = + * enforced" shape the rest of the repo uses (Prime Directive #10): the + * declaration is a deliberate authoring act, machine-checkable at the moment of + * boot, and independent of launcher, package manager and hoist layout. + * + * The two failures it separates were, until now, one indistinguishable + * `MODULE_NOT_FOUND`, with opposite remedies: + * + * - **undeclared** — the app never asked for this package. Remedy: declare it + * in the app's `package.json` and install. + * - **declared but unresolvable** — the app asked for it and the install is + * broken/pruned/unbuilt. Remedy: fix the install. Re-reading the + * `package.json` is wasted effort; the declaration is right there. + * + * {@link hostImportFailureKind} exposes that classification to callers so their + * fail-fast text can say which one it is (`packages/cli` ADR-0093 D5, + * `packages/verify` `bootStack`, `packages/qa/dogfood`'s enterprise probe). */ +import { readFileSync } from 'node:fs'; import { createRequire } from 'node:module'; import { join } from 'node:path'; import { pathToFileURL } from 'node:url'; +import { isModuleNotFoundError } from './module-not-found.js'; /** * Imports a package as the host app would see it. @@ -80,26 +124,269 @@ export function createHostRequire(hostRoot: string = process.cwd()): NodeRequire } /** - * Build an importer that resolves from the host app first, then falls back to - * the importing package's own resolution. + * The `package.json` fields whose KEYS count as a host-app declaration (#4719). + * + * All four are deliberate authoring acts in the app's own manifest that name the + * package, which is the signal this gate is built on — not "is it reachable". + * Why each is in: + * + * - `dependencies` — the obvious one: the app runs with it. + * - `devDependencies` — the app being served / verified / dogfooded IS the + * project, not a library someone else consumes, so its dev deps are installed + * in exactly the environment this resolver runs in. + * - `optionalDependencies` — npm/pnpm install them and tolerate an install + * failure. "Installed ⇒ declared" holds; and if it did NOT install, the + * declared-but-unresolvable branch says so precisely instead of pretending the + * app never asked. + * - `peerDependencies` — an app is nobody's peer, so this is an unusual place to + * put an enterprise add-on; but it still NAMES the package on purpose, and + * `packages/cli`'s own edition gate (`serve`'s AI-service opt-in, #1597) has + * read all four since it was written. Accepting three here and four there + * would fork "declared" into two dialects for one question — the shape Prime + * Directive #12 exists to prevent. That gate now delegates to this list, so + * there is one owner and one answer. + * + * `bundleDependencies` is absent on purpose: it is an array of names that must + * ALSO appear in `dependencies`, so it can never be the only declaration. + */ +export const HOST_DECLARATION_FIELDS = [ + 'dependencies', + 'devDependencies', + 'optionalDependencies', + 'peerDependencies', +] as const; + +export type HostDeclarationField = (typeof HOST_DECLARATION_FIELDS)[number]; + +/** What the host app's `package.json` says about one package name. */ +export interface HostDeclaration { + /** Bare package name the specifier belongs to (subpath stripped). */ + packageName: string; + /** Directory whose `package.json` was consulted. */ + hostRoot: string; + /** True when {@link packageName} is a key of one of {@link HOST_DECLARATION_FIELDS}. */ + declared: boolean; + /** Which field carried it (first match, in {@link HOST_DECLARATION_FIELDS} order). */ + field?: HostDeclarationField; + /** The version range AS WRITTEN — `^1.2.3`, `workspace:*`, `npm:@acme/x@1`, `link:../x`. */ + specifier?: string; + /** True when `hostRoot` has no readable / parseable `package.json` at all. */ + manifestMissing?: boolean; +} + +/** + * The package a bare specifier belongs to, or `undefined` when the specifier is + * not a bare package name at all (a relative/absolute path, a `file:`/`data:` + * URL, or a `node:`-prefixed builtin). Those bypass the declaration gate: they + * are not things a `package.json` can declare. + * + * Subpaths are stripped, so `@objectstack/platform-objects/plugin` is declared + * by `"@objectstack/platform-objects"`, which is the only key that can exist. + * Scoped names keep both segments. + * + * Alias dependencies need no special case, and that is the point: with + * `"foo": "npm:bar@1"` the importable specifier is `foo` and the manifest key is + * `foo`, so keying on the KEY (never the value) is exactly right — `import('bar')` + * correctly reads as undeclared unless `bar` is itself a key. Same for + * `workspace:` / `link:` / `file:` specifiers: the key is the name, the value is + * the package manager's business. + */ +export function packageNameFromSpecifier(specifier: string): string | undefined { + if (!specifier || specifier.startsWith('.') || specifier.startsWith('/')) return undefined; + // A URL-ish or protocol-prefixed specifier (`node:fs`, `file:///…`, `data:…`). + if (/^[a-z][a-z0-9+.-]*:/i.test(specifier)) return undefined; + const segments = specifier.split('/'); + if (specifier.startsWith('@')) { + if (segments.length < 2 || !segments[0] || !segments[1]) return undefined; + return `${segments[0]}/${segments[1]}`; + } + return segments[0] || undefined; +} + +/** + * Read what the host app's `package.json` declares about `specifier`. + * + * Deliberately a plain manifest READ, never a resolution attempt: resolvability + * is the property #4719 proved unreliable (it moved with `NODE_PATH` and the + * hoist layout), while the manifest is the same fact in every launcher. + */ +export function readHostDeclaration( + specifier: string, + hostRoot: string = process.cwd(), +): HostDeclaration { + const packageName = packageNameFromSpecifier(specifier) ?? specifier; + const base: HostDeclaration = { packageName, hostRoot, declared: false }; + + let manifest: Record; + try { + manifest = JSON.parse(readFileSync(join(hostRoot, 'package.json'), 'utf8')) as Record< + string, + unknown + >; + } catch { + // No manifest ⇒ nothing is declared. Recorded rather than swallowed so the + // failure text can say "there is no package.json here" instead of the + // misleading "you did not declare it". + return { ...base, manifestMissing: true }; + } + + for (const field of HOST_DECLARATION_FIELDS) { + const entries = manifest[field]; + if (!entries || typeof entries !== 'object') continue; + const specifierValue = (entries as Record)[packageName]; + if (specifierValue === undefined) continue; + return { ...base, declared: true, field, specifier: String(specifierValue) }; + } + return base; +} + +/** Convenience predicate over {@link readHostDeclaration}. */ +export function isDeclaredByHost(specifier: string, hostRoot?: string): boolean { + return readHostDeclaration(specifier, hostRoot).declared; +} + +/** + * Why a {@link HostImporter} could not produce a module. + * + * - `undeclared` — the host app's `package.json` never names the package, and + * the importing framework package cannot supply it either. Remedy: DECLARE it + * in the app and install. + * - `declared-unresolvable` — the app declares it and it still would not + * resolve. Remedy: fix the INSTALL. Re-reading the manifest is wasted effort. + * + * An evaluation crash is neither: it propagates untouched and carries no kind. + */ +export type HostImportFailureKind = 'undeclared' | 'declared-unresolvable'; + +/** + * Property carrying {@link HostImportFailureKind} on a thrown error. + * + * A string property, read by {@link hostImportFailureKind} — never `instanceof`. + * `serve` loads plugins through this importer, so CLI and package can hold + * different module instances of anything class-shaped; the #4818 comment in + * `serve.ts` names that trap explicitly. + */ +export const HOST_IMPORT_FAILURE_KIND = 'objectstackHostImportFailureKind'; + +/** The classification on an error thrown by a {@link HostImporter}, if any. */ +export function hostImportFailureKind(err: unknown): HostImportFailureKind | undefined { + const kind = (err as Record | null | undefined)?.[HOST_IMPORT_FAILURE_KIND]; + return kind === 'undeclared' || kind === 'declared-unresolvable' ? kind : undefined; +} + +function hostImportError( + kind: HostImportFailureKind, + message: string, + cause: unknown, +): Error { + // `cause` is assigned rather than passed to the constructor: this package + // compiles against a lib without the ES2022 `ErrorOptions` overload. + const err = new Error(message); + // Every caller classifies "missing vs crashed" through + // `isModuleNotFoundError`; both of these ARE the missing case, just with + // different remedies, so they must keep answering true to it. + return Object.assign(err, { + cause, + code: 'MODULE_NOT_FOUND', + [HOST_IMPORT_FAILURE_KIND]: kind, + }); +} + +function undeclaredMessage(declaration: HostDeclaration, cause: unknown): string { + const { packageName, hostRoot, manifestMissing } = declaration; + const detail = cause instanceof Error ? cause.message : String(cause); + return ( + `Cannot find package '${packageName}': the host app does not declare it.\n` + + ` host app: ${hostRoot}\n` + + (manifestMissing + ? ' no readable package.json was found there — nothing can be declared\n' + : ` checked: ${HOST_DECLARATION_FIELDS.join(', ')}\n`) + + `\n Declare it in that app's package.json and install it, e.g.\n` + + ` cd ${hostRoot} && pnpm add ${packageName}\n` + + '\n Being merely REACHABLE is not enough and is rejected on purpose (#4719):\n' + + ' a package hoisted into a workspace store — which is what NODE_PATH points\n' + + " at in every pnpm bin shim — used to resolve here regardless of the app's\n" + + ' package.json, so the same app booted or refused depending on how the\n' + + ' process was launched. The declaration is the contract.\n' + + ` (fallback resolution also failed: ${detail})` + ); +} + +function unresolvableMessage(declaration: HostDeclaration, cause: unknown): string { + const { packageName, hostRoot, field, specifier } = declaration; + const detail = cause instanceof Error ? cause.message : String(cause); + return ( + `Cannot find module '${packageName}': the host app DECLARES it ` + + `(${field}: ${JSON.stringify(specifier)}) but it could not be resolved.\n` + + ` host app: ${hostRoot}\n` + + '\n This is an INSTALL problem, not a declaration problem — the declaration is\n' + + ' already there, so re-reading the package.json will not help. Check:\n' + + ` • dependencies never installed, or installed before the declaration was added → run \`pnpm install\` in ${hostRoot}\n` + + ' • a production prune / filtered deploy dropped it (devDependencies and\n' + + ' optionalDependencies go first)\n' + + ' • it IS installed but its "main"/"exports" points at a dist that was never built\n' + + ` (resolver: ${detail})` + ); +} + +/** + * Build an importer that loads a package **as the host app declares it**, and + * otherwise falls back to the importing package's own resolution. * - * @param hostRequire Reuse an existing host `require` (callers usually also need - * it to read the host `package.json`); defaults to one anchored at the CWD. + * Order of operations, and why (#4719): + * + * 1. The host `package.json` is READ. Only a declared name is looked up in the + * host's `node_modules`. An undeclared name never reaches the host resolver, + * so no amount of `NODE_PATH` / hoisting can make it appear to be the app's. + * 2. Declared but unresolvable is reported AS SUCH — the app asked for it and + * the install is broken. It is not retried bare: falling back there would + * reintroduce exactly the "some other package happens to supply it" accident + * this gate closes, and would report an install problem as an absence. + * 3. Undeclared falls back to the importing package's own resolution, which is + * what keeps every framework-owned load working (`serve`'s plugin-auth / + * service-i18n path, `bootStack`'s service plugins). Bare `import()` is ESM, + * and ESM does not honour `NODE_PATH`, so the fallback cannot re-open the + * hole either. Only when that fails as module-not-found does the undeclared + * error surface; a package that RESOLVES and then throws while evaluating is + * a genuine crash and propagates untouched, as before. + * + * @param hostRoot Directory holding the host app's `package.json` (default: the + * process CWD, which is where the CLI reads `objectstack.config.ts` from too). + * Note this used to take a pre-built `NodeRequire`; it needs the ROOT now, + * because a `NodeRequire` cannot be asked where it was anchored and the manifest + * has to be read from there. */ -export function createHostImporter( - hostRequire: NodeRequire = createHostRequire(), -): HostImporter { +export function createHostImporter(hostRoot: string = process.cwd()): HostImporter { + const hostRequire = createHostRequire(hostRoot); return async (pkg: string): Promise => { - let resolved: string; - try { - resolved = hostRequire.resolve(pkg); - } catch { - // Invisible to the host app — try the importing package's own - // dependencies. A package neither can see throws MODULE_NOT_FOUND from - // here, which is what the callers' "missing vs crashed" classification - // expects. + // Not a bare package name (a path, a URL, a `node:` builtin) — nothing a + // manifest could declare. Hand it to the normal resolver untouched. + if (packageNameFromSpecifier(pkg) === undefined) { return import(/* webpackIgnore: true */ pkg); } - return import(pathToFileURL(resolved).href); + + const declaration = readHostDeclaration(pkg, hostRoot); + + if (declaration.declared) { + let resolved: string; + try { + resolved = hostRequire.resolve(pkg); + } catch (cause) { + throw hostImportError( + 'declared-unresolvable', + unresolvableMessage(declaration, cause), + cause, + ); + } + return import(pathToFileURL(resolved).href); + } + + try { + return await import(/* webpackIgnore: true */ pkg); + } catch (cause) { + // A package that resolved and then exploded is a crash, not an absence. + if (!isModuleNotFoundError(cause)) throw cause; + throw hostImportError('undeclared', undeclaredMessage(declaration, cause), cause); + } }; } diff --git a/packages/verify/src/harness.host-resolution.test.ts b/packages/verify/src/harness.host-resolution.test.ts index 8a38492a7d..5f13e4495d 100644 --- a/packages/verify/src/harness.host-resolution.test.ts +++ b/packages/verify/src/harness.host-resolution.test.ts @@ -72,8 +72,18 @@ interface TenancyShape { let appWithPackage: string; /** The same app WITHOUT it — the hard error must still fire. */ let appWithoutPackage: string; +/** + * #4719 — the package is INSTALLED in the app's own `node_modules` and the app + * never declares it. That is what a hoisted workspace store looks like to the + * resolver, and `bootStack` used to mount multi-tenant off it. + */ +let appInstalledButUndeclared: string; -function writeApp(prefix: string, opts: { withOrganizations: boolean }): string { +function writeApp( + prefix: string, + opts: { withOrganizations: boolean; declare?: boolean }, +): string { + const declare = opts.declare ?? opts.withOrganizations; const dir = mkdtempSync(join(tmpdir(), prefix)); writeFileSync( join(dir, 'package.json'), @@ -82,7 +92,7 @@ function writeApp(prefix: string, opts: { withOrganizations: boolean }): string name: 'hostres-fixture', private: true, type: 'module', - ...(opts.withOrganizations ? { dependencies: { '@objectstack/organizations': '*' } } : {}), + ...(declare ? { dependencies: { '@objectstack/organizations': '*' } } : {}), }, null, 2, @@ -110,10 +120,14 @@ function writeApp(prefix: string, opts: { withOrganizations: boolean }): string beforeAll(() => { appWithPackage = writeApp('os-verify-org-host-ok-', { withOrganizations: true }); appWithoutPackage = writeApp('os-verify-org-host-missing-', { withOrganizations: false }); + appInstalledButUndeclared = writeApp('os-verify-org-host-undeclared-', { + withOrganizations: true, + declare: false, + }); }); afterAll(() => { - for (const dir of [appWithPackage, appWithoutPackage]) { + for (const dir of [appWithPackage, appWithoutPackage, appInstalledButUndeclared]) { if (dir) rmSync(dir, { recursive: true, force: true }); } }); @@ -179,4 +193,34 @@ describe('bootStack multiTenant — host-app package resolution (#4700)', () => }, BOOT_TIMEOUT, ); + + it( + 'refuses an UNDECLARED package even though it sits in the app\'s node_modules (#4719)', + async () => { + // Same fixture package, same directory layout as the passing case above — + // only the `package.json` differs. Before #4719 the host lookup was a CJS + // `require`, which finds anything reachable (the app's own node_modules + // here; the pnpm shim's NODE_PATH store in the field), so `bootStack` + // mounted the enterprise plugin for an app that had never asked for it and + // the fixture's RLS posture silently depended on the workspace layout. + await expect( + bootStack(app as never, { multiTenant: true, hostRoot: appInstalledButUndeclared }), + ).rejects.toThrow(/requires the enterprise @objectstack\/organizations/); + expect(process.env.OS_TENANCY_POSTURE).toBeUndefined(); + }, + BOOT_TIMEOUT, + ); + + it( + 'says DECLARE it, not just install it, when the app has it but never declared it (#4719)', + async () => { + // The remedy must be the one that works. Telling an operator to install a + // package that is demonstrably installed is the same unfollowable advice + // #4700 removed from this message, one layer along. + await expect( + bootStack(app as never, { multiTenant: true, hostRoot: appInstalledButUndeclared }), + ).rejects.toThrow(/DECLARE it in that app's package\.json/); + }, + BOOT_TIMEOUT, + ); }); diff --git a/packages/verify/src/harness.ts b/packages/verify/src/harness.ts index efe9b98fb4..9b02153511 100644 --- a/packages/verify/src/harness.ts +++ b/packages/verify/src/harness.ts @@ -33,7 +33,7 @@ import { PlatformObjectsPlugin } from '@objectstack/platform-objects/plugin'; // Node-only subpath (#4700). Optional packages supplied by the app under // verification — `@objectstack/organizations` above all — must be resolved from // THAT app, not from `packages/verify`'s own realpath inside this workspace. -import { createHostImporter, createHostRequire } from '@objectstack/types/node'; +import { createHostImporter, hostImportFailureKind } from '@objectstack/types/node'; /** A Hono app exposes `.request(path, init)` returning a standard `Response`. */ interface InjectableApp { @@ -295,15 +295,32 @@ export async function bootStack( // telling them to install it again. Resolve from the host app (the project // `objectstack verify` runs in) and fall back to this package's own // resolution — the same helper `objectstack serve` uses (cloud#1013). + // + // #4719: that host resolution is now gated on the host app DECLARING the + // package. It previously honoured NODE_PATH (a CJS require), so under a pnpm + // bin shim a fixture app that never declared the enterprise runtime still + // booted multi-tenant off a hoisted copy — and the RLS posture a fixture + // then asserted against depended on the launcher. const organizationsPkg = '@objectstack/organizations'; + const hostRoot = opts.hostRoot ?? process.cwd(); let mod: any; try { - mod = await createHostImporter(createHostRequire(opts.hostRoot))(organizationsPkg); + mod = await createHostImporter(hostRoot)(organizationsPkg); } catch (e) { restoreTenancyPosture(); + // Two absences, two remedies (#4719). "Install/link it in THIS APP" is + // exactly wrong for an app that already declared it and has a pruned or + // unbuilt install — it sends the operator back to a correct package.json. + const remedy = + hostImportFailureKind(e) === 'declared-unresolvable' + ? `It IS declared in ${hostRoot}'s package.json, so the declaration is not the problem — ` + + `repair the install there (\`pnpm install\`, un-prune, rebuild its dist).` + : `Install/link it in THIS APP (${hostRoot}) — and DECLARE it in that app's ` + + 'package.json, which is what is actually checked: a package merely reachable through ' + + 'NODE_PATH or a hoisted workspace store is not accepted (#4719) — to run multi-org fixtures.'; throw new Error( 'verify: multiTenant=true requires the enterprise @objectstack/organizations package (migrated from plugin-org-scoping, ADR-0081 D2). ' + - `Install/link it in THIS APP (${opts.hostRoot ?? process.cwd()}) to run multi-org fixtures. (${(e as Error).message})`, + `${remedy} (${(e as Error).message})`, ); } await kernel.use(new mod.OrganizationsPlugin());