From a3ecc7cec74a1e8215dfa3035bd1c42f3947cdf9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 19:28:32 +0000 Subject: [PATCH] feat(types,cli,verify)!: gate host-app package resolution on the host's declaration, not on NODE_PATH reachability (#4719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `createHostRequire` returns a CJS `createRequire`, and CJS resolution honours `NODE_PATH` (`Module.globalPaths`). Every pnpm bin shim exports NODE_PATH at the hoisted workspace store before it execs, so any package transitively reachable from anywhere in the workspace resolved "from the host app" — regardless of what that app declared. ADR-0093 D5's wall therefore fired or not according to HOW the process was launched, and its own message told operators to "declare it in the app's package.json", the one thing the CLI never checked. The host lookup is now gated on the host's declaration: a name is looked up in the host's node_modules only when it is a key of `dependencies`, `devDependencies`, `optionalDependencies` or `peerDependencies`. Undeclared names fall back to the importing package's own (ESM, NODE_PATH-blind) resolution, so every framework-owned load is unchanged. The two absences are now reported apart, with their opposite remedies: undeclared points at declaring + installing; declared-but-unresolvable points at the install and says the declaration is not the problem. `hostImportFailureKind()` exposes the classification; both errors keep `code: 'MODULE_NOT_FOUND'`. `serve`'s private `hostDeclaresDependency` (#1597) now delegates to the shared owner, so "declared" cannot mean two things in one file. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- .../host-declared-package-resolution.md | 64 ++++ packages/cli/src/commands/serve.ts | 65 ++-- ...-organizations-host-resolution.e2e.test.ts | 83 ++++- .../test/enterprise-organizations.test.ts | 39 ++- .../dogfood/test/enterprise-organizations.ts | 43 ++- packages/types/src/node-isolation.test.ts | 4 + packages/types/src/node.test.ts | 301 +++++++++++++++-- packages/types/src/node.ts | 319 +++++++++++++++++- .../src/harness.host-resolution.test.ts | 50 ++- packages/verify/src/harness.ts | 23 +- 10 files changed, 898 insertions(+), 93 deletions(-) create mode 100644 .changeset/host-declared-package-resolution.md 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());