diff --git a/.changeset/install-local-post-storage-dir.md b/.changeset/install-local-post-storage-dir.md new file mode 100644 index 0000000000..4ae8774139 --- /dev/null +++ b/.changeset/install-local-post-storage-dir.md @@ -0,0 +1,46 @@ +--- +"@objectstack/cloud-connection": minor +"@objectstack/cli": patch +--- + +feat(cloud-connection,cli): the install-local POST reports where it cached the manifest, and `os package install` quotes it (#6721) + +The two endpoints of `MarketplaceInstallLocalPlugin` disagreed about one fact. +`GET /api/v1/marketplace/install-local` — the console's Installed Apps list — +served `storageDir: this.storageDir`, the ledger directory as resolved by +`LocalManifestSource` (`config.storageDir` when the host set one, the +`.objectstack/installed-packages` default when it did not). The `POST` that +performs the install did not, so its `data` block described everything about +the install except **where the install went**. + +That gap was load-bearing for the one consumer of that response. `os package +install` runs on a different machine from the runtime it installs into: it +speaks HTTP and never touches the target's disk. With no directory in the +response it could only describe the cache location by literal, and the literal +it printed was the plugin's *default* — wrong for every host that configures +`storageDir`, and wrong today rather than eventually. No consumer-side fix +existed: a locally-resolved constant names the wrong machine, and importing it +from `@objectstack/cloud-connection` would make a pure-HTTP command fail at +module load wherever that package is absent. The producer is the contract, so +the fix is there. + +**`@objectstack/cloud-connection` (additive, no migration).** The install POST +response's `data` now carries `storageDir`, read from the same +`this.storageDir` field the GET listing already returns — one field, two +endpoints, so they cannot drift apart again. No existing key changed, and +nothing needs to read the new one. + +**`@objectstack/cli`.** The post-install hint now quotes the directory the +runtime reported: + +``` + The manifest is cached on the runtime host and re-registers on every + boot (survives restarts): + /srv/objectstack/state/ledger-packages +``` + +Against a runtime older than this release — one whose response has no +`storageDir` — the CLI prints **no** directory sentence at all. It does not +fall back to the old literal: a consumer stating a value the producer declined +to state is the defect Prime Directive #12 forbids, and saying less is correct +where guessing is not. Everything else the command prints is unchanged. diff --git a/packages/cli/src/commands/package/install.ts b/packages/cli/src/commands/package/install.ts index c770560231..b47f285b4e 100644 --- a/packages/cli/src/commands/package/install.ts +++ b/packages/cli/src/commands/package/install.ts @@ -215,43 +215,32 @@ export default class PackageInstall extends Command { printKV(' Runtime', runtime); if (data.installedAt) printKV(' Installed', String(data.installedAt)); console.log(''); - // ⚠️ This path is a DESCRIPTION OF THE DEFAULT CONVENTION, deliberately - // left as a literal — not a consumer restating a value it could have read - // (#6643, the sibling half of #5996). Do not "fix" it into a reference to - // `DEFAULT_INSTALLED_PACKAGES_DIR`. Four measured reasons, in order: + // #6721: the cache directory is quoted from the RESPONSE, never from a + // literal or a locally-resolved constant. The directory lives on the + // REMOTE host — everything above this line came back over HTTP from + // `runtime`, and this command never touches the target's disk — so the + // only truthful source is the host itself. #6643 documented at length why + // no consumer-side answer works (a local constant describes the machine + // typing the command; it is only the *default*, wrong the moment a host + // configures `storageDir`; a static import of + // `@objectstack/cloud-connection` would make a pure-HTTP command fail at + // module load). The fix was upstream, and it landed: the POST response now + // carries `storageDir: this.storageDir` — the same resolved value + // (`ledger.dir`) its GET listing sibling already served. // - // 1. **The directory is on the REMOTE host.** Everything above this - // line came back over HTTP from `runtime`; this command never - // touches the target's disk. A constant resolved HERE describes the - // machine typing the command, not the one that stored the manifest. - // 2. **The remote's directory is configurable, so the constant is only - // its default.** `MarketplaceInstallLocalPlugin` builds its ledger as - // `new LocalManifestSource(config.storageDir)` — the export is the - // fallback that ctor applies when the host configured nothing. - // Interpolating it would state a default as an observed fact. - // 3. **The response we just read does not carry the real answer.** The - // POST returns `{ manifestId, version, versionId, installedAt, - // hotLoaded, upgradedFrom, translationsLoaded, seeded, note }` — no - // `storageDir`. The GET listing endpoint does carry one; this one - // does not, and asking for it would be an extra round-trip (and a - // new failure mode) bolted onto a success hint. - // 4. **Importing it would cost this command its independence.** Every - // CLI reference to `@objectstack/cloud-connection` is a DYNAMIC load - // behind `loadOptionalPackage()` (doctor.ts) or a guarded `import()` - // (serve.ts), because the CLI must keep working where that package - // is absent or unbuilt. A static import for one hint line would make - // a pure-HTTP command fail at module load; a dynamic one needs a - // literal fallback — the exact `??` Prime Directive #12 forbids and - // #5996 deleted. - // - // So the divergence surface is accepted here, knowingly: if the constant - // ever changes value, this sentence goes stale and no gate will say so. - // The honest fix is upstream — the POST response carrying `storageDir` - // like its GET sibling — which would let this line quote the real remote - // directory. Tracked separately; `@objectstack/cloud-connection` is out - // of scope for #6643. - console.log(' The manifest is cached under .objectstack/installed-packages/ on the'); - console.log(' runtime host and re-registers on every boot (survives restarts).'); + // ⛔ No fallback. When the field is missing — an older host that predates + // the producer half — this block prints NOTHING. A `?? + // '.objectstack/installed-packages/'` would re-introduce exactly the + // defect Prime Directive #12 forbids and #5996 deleted: a consumer + // inventing a value the producer declined to state. Saying less is + // correct; guessing is not. An empty or non-string value is the same + // "host did not state it" case, not a reason to print a blank path. + const storageDir = typeof data.storageDir === 'string' ? data.storageDir.trim() : ''; + if (storageDir) { + console.log(' The manifest is cached on the runtime host and re-registers on every'); + console.log(' boot (survives restarts):'); + console.log(` ${storageDir}`); + } } catch (error) { printError((error as Error).message); this.exit(1); diff --git a/packages/cli/test/package-install-storage-dir.test.ts b/packages/cli/test/package-install-storage-dir.test.ts new file mode 100644 index 0000000000..182a336632 --- /dev/null +++ b/packages/cli/test/package-install-storage-dir.test.ts @@ -0,0 +1,108 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6721 — `os package install`'s post-install directory hint quotes the + * RESPONSE, and prints nothing when the response has no directory to quote. + * + * The directory lives on the runtime host, which is a different machine: this + * command speaks only HTTP and never touches the target's disk. #6643 measured + * that no consumer-side answer exists — a locally-resolved constant describes + * the wrong machine and is in any case only the host's *default*, wrong for + * every host that configures `storageDir` — and left the literal in place with + * a comment saying the honest fix was upstream. It landed (POST now carries + * `storageDir`), so the literal is gone. + * + * The absent case is the one that keeps it gone. A `?? '.objectstack/…'` bolted + * onto the new reference would read as a harmless kindness to older hosts and + * would reinstate precisely the defect Prime Directive #12 forbids and #5996 + * deleted: a consumer stating a value the producer declined to state. Only a + * case that FAILS when the sentence appears anyway can hold that door shut — + * a test covering just the happy path leaves it open. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import PackageInstall from '../src/commands/package/install.js'; + +/** The host configured its ledger somewhere no default would ever guess. */ +const REMOTE_DIR = '/srv/objectstack/state/ledger-packages'; + +/** Stub the runtime's install POST with the given `data` block. */ +function stubRuntime(data: Record): void { + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: true, + status: 200, + statusText: 'OK', + headers: { get: () => null }, + json: async () => ({ success: true, data }), + }) as any)); +} + +/** Run the command in catalog mode and return everything it printed. */ +async function runInstall(): Promise { + const lines: string[] = []; + const capture = (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }; + vi.spyOn(console, 'log').mockImplementation(capture); + vi.spyOn(console, 'error').mockImplementation(capture); + await PackageInstall.run(['com.acme.crm', '--runtime', 'http://runtime.test']); + return lines.join('\n'); +} + +const INSTALLED = { + manifestId: 'com.acme.crm', + version: '1.0.0', + versionId: 'ver_1', + installedAt: '2026-08-10T00:00:00.000Z', + hotLoaded: true, +}; + +describe('os package install — post-install directory hint', () => { + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('quotes the storageDir the runtime reported, verbatim', async () => { + stubRuntime({ ...INSTALLED, storageDir: REMOTE_DIR }); + + const out = await runInstall(); + + expect(out).toContain('Package installed into the running kernel'); + expect(out).toContain(REMOTE_DIR); + expect(out).toContain('re-registers on every'); + // The old literal is not printed alongside the real value — the point is + // that the sentence has ONE source, and it is the response. + expect(out).not.toContain('.objectstack/installed-packages'); + }); + + // The delivery criterion of this card: no fallback. An older host that + // predates the producer half says nothing about its ledger, so neither do we. + it('prints NO directory sentence when the response omits storageDir', async () => { + stubRuntime({ ...INSTALLED }); + + const out = await runInstall(); + + // The install itself is still reported — only the directory claim is gone. + expect(out).toContain('Package installed into the running kernel'); + expect(out).toContain('com.acme.crm'); + expect(out).not.toContain('cached'); + expect(out).not.toContain('survives restarts'); + expect(out).not.toContain('.objectstack/installed-packages'); + expect(out).not.toContain('undefined'); + }); + + // A host that answers with an empty/blank value has told us nothing either; + // that is the same case, not a licence to print a blank path. + it.each([ + ['an empty string', ''], + ['whitespace', ' '], + ['null', null], + ])('prints no directory sentence when storageDir is %s', async (_label, value) => { + stubRuntime({ ...INSTALLED, storageDir: value }); + + const out = await runInstall(); + + expect(out).toContain('Package installed into the running kernel'); + expect(out).not.toContain('cached'); + expect(out).not.toContain('survives restarts'); + }); +}); diff --git a/packages/cloud-connection/src/marketplace-install-local-plugin.ts b/packages/cloud-connection/src/marketplace-install-local-plugin.ts index 0f2347d3d6..917a96da9f 100644 --- a/packages/cloud-connection/src/marketplace-install-local-plugin.ts +++ b/packages/cloud-connection/src/marketplace-install-local-plugin.ts @@ -731,6 +731,15 @@ export class MarketplaceInstallLocalPlugin implements Plugin { upgradedFrom: conflict === 'marketplace' ? 'previous-marketplace-version' : null, translationsLoaded: seededSummary.translationsLoaded, seeded: seededSummary.seeded, + // #6721: the RESOLVED ledger directory on THIS host — the same + // value the GET listing serves (`handleList`), from the same + // field. It is here because the installer is remote: `os package + // install` never touches this machine's disk, so without it the + // CLI can only describe the directory by literal, and that + // literal is wrong the moment a host configures `storageDir`. + // Keep the two endpoints reading `this.storageDir` — a second + // derivation is how they diverged in the first place. + storageDir: this.storageDir, note: 'App is now available in this runtime. Refresh the console to see it in the app switcher.', }, }, 200); diff --git a/packages/cloud-connection/src/marketplace-install-local-storage-dir.test.ts b/packages/cloud-connection/src/marketplace-install-local-storage-dir.test.ts new file mode 100644 index 0000000000..10b7f55fe7 --- /dev/null +++ b/packages/cloud-connection/src/marketplace-install-local-storage-dir.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6721 — the install POST reports WHERE it cached the manifest. + * + * The two endpoints of this plugin had diverged on one fact. `GET + * /api/v1/marketplace/install-local` (the console's Installed Apps list) served + * `storageDir: this.storageDir`; the `POST` that performs the install did not, + * so its only consumer — `os package install`, which runs on a DIFFERENT machine + * and never touches this host's disk — had no resolved value to quote and could + * only describe the ledger directory by literal. That literal is the ctor's + * default, i.e. wrong for every host that configures `storageDir`. + * + * The non-default-`storageDir` case below is therefore the point of this file, + * not a variation on it: it is exactly the configuration the old literal + * misreported, and the only one that can tell "quotes the resolved value" apart + * from "restates the default". + * + * `this.storageDir` is `this.ledger.dir` — already resolved by + * `LocalManifestSource`'s ctor — so both endpoints must keep reading that one + * field. A second derivation is how they diverged in the first place, which is + * why the parity case asserts POST and GET are the same string rather than + * asserting each against a separately-computed expectation. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { tmpdir } from 'node:os'; +import { MarketplaceInstallLocalPlugin } from './marketplace-install-local-plugin.js'; +import { DEFAULT_INSTALLED_PACKAGES_DIR } from './local-manifest-source.js'; + +type Handler = (c: any) => Promise; + +function makeRawApp() { + const routes = new Map(); + return { + routes, + get: (p: string, h: Handler) => routes.set(`GET ${p}`, h), + post: (p: string, h: Handler) => routes.set(`POST ${p}`, h), + delete: (p: string, h: Handler) => routes.set(`DELETE ${p}`, h), + }; +} + +function makeCtx(rawApp: any) { + const hooks = new Map(); + const services: Record = { + manifest: { register: vi.fn() }, + auth: { api: { getSession: async () => ({ user: { id: 'admin' } }) } }, + objectql: { syncSchemas: async () => undefined }, + }; + return { + ctx: { + hook: (e: string, h: any) => hooks.set(e, h), + getService: (name: string) => { + if (name === 'http-server') return { getRawApp: () => rawApp }; + const svc = services[name]; + if (svc === undefined) throw new Error(`no ${name}`); + return svc; + }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + }, + fire: async () => { await hooks.get('kernel:ready')?.(); }, + }; +} + +function makeC(body: any) { + const json = vi.fn((payload: any, status?: number) => ({ payload, status: status ?? 200 })); + return { + req: { + url: 'http://localhost:3000/api/v1/marketplace/install-local', + raw: new Request('http://localhost:3000/x'), + json: async () => body, + param: () => undefined, + }, + json, + }; +} + +/** Boot the plugin far enough that its routes are mounted. */ +async function mount(storageDir?: string) { + const rawApp = makeRawApp(); + const { ctx, fire } = makeCtx(rawApp); + const plugin = new MarketplaceInstallLocalPlugin({ controlPlaneUrl: 'off', storageDir }); + await plugin.start(ctx as any); + await fire(); + return { + install: async (manifest: any) => + rawApp.routes.get('POST /api/v1/marketplace/install-local')!(makeC({ manifest })), + list: async () => + rawApp.routes.get('GET /api/v1/marketplace/install-local')!(makeC({})), + }; +} + +const MANIFEST = { id: 'com.acme.crm', name: 'Acme CRM', version: '1.0.0', objects: [] }; + +let dir: string; +beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'mil-storage-dir-')); }); +afterEach(() => { rmSync(dir, { recursive: true, force: true }); vi.restoreAllMocks(); }); + +describe('install-local POST response — storageDir', () => { + it('carries the RESOLVED ledger directory of a host that configured a non-default storageDir', async () => { + const { install } = await mount(dir); + + const res = await install(MANIFEST); + + expect(res.payload?.success).toBe(true); + // The configured directory, resolved — not the ctor default. Both + // halves matter: the first is the value the CLI must be able to quote, + // the second is the assertion the old literal would have failed. + expect(res.payload.data.storageDir).toBe(resolve(dir)); + expect(res.payload.data.storageDir).not.toContain(DEFAULT_INSTALLED_PACKAGES_DIR); + }); + + it('carries the ctor-default directory when the host configured nothing', async () => { + // chdir into the temp dir first: with no `storageDir` the ledger + // resolves against `process.cwd()`, and the install genuinely writes a + // file there. Without this the case would litter the repo checkout. + const prevCwd = process.cwd(); + process.chdir(dir); + try { + const { install } = await mount(undefined); + + const res = await install(MANIFEST); + + expect(res.payload?.success).toBe(true); + expect(res.payload.data.storageDir).toBe(resolve(process.cwd(), DEFAULT_INSTALLED_PACKAGES_DIR)); + } finally { + process.chdir(prevCwd); + } + }); + + it('reports the same directory as the GET listing — one field, two endpoints', async () => { + const { install, list } = await mount(dir); + + const installed = await install(MANIFEST); + const listed = await list(); + + expect(installed.payload.data.storageDir).toBe(listed.payload.data.storageDir); + expect(typeof installed.payload.data.storageDir).toBe('string'); + expect(installed.payload.data.storageDir.length).toBeGreaterThan(0); + }); +});