diff --git a/.changeset/manifest-read-says-which-null.md b/.changeset/manifest-read-says-which-null.md new file mode 100644 index 0000000000..ea60a06d96 --- /dev/null +++ b/.changeset/manifest-read-says-which-null.md @@ -0,0 +1,44 @@ +--- +'@objectstack/cloud-connection': minor +--- + +`LocalManifestSource.read()` now says WHICH of the two things its `null` meant + +`read()` answered `null` to two different questions at once — "this manifest was +never installed" and "it was installed, but its ledger file cannot be read" — +and dropped the reason for the second in an un-bound `catch`. Two admin +endpoints check `has()` first, so absence was already ruled out by the time they +called it, and both could only answer +`500 { code: 'MARKETPLACE_STORAGE_FAILED', message: 'Failed to read manifest cache.' }`: +a sentence whose only content is that the thing it just did failed, one line +after `has()` said the file is there. The `Unexpected end of JSON input` / +`EACCES` / `EISDIR` that names the repair had already been thrown away, and +nothing was written to the server log either. + +**Breaking (`@objectstack/cloud-connection`):** `read()` returns an +`InstalledManifestLookup` instead of `InstalledManifestEntry | null`. + +- FROM: `const entry = source.read(id);` +- TO: `const { entry, failure } = source.read(id);` + +`entry` is the old return value unchanged, so a caller that legitimately treats +both nulls alike migrates by reading `.entry`. `failure` is a +`SkippedManifestEntry` — `{ file, cause }`, the same shape `list()` already +reports, with the thrown object carried unwrapped — and is present ONLY when a +ledger file exists and could not be read. `failure === undefined` with +`entry === null` therefore means "not installed", which is the distinction the +merged `null` erased. One new exported type, `InstalledManifestLookup`. + +`read()` still does not validate the parsed value's SHAPE, and enumerating the +ledger directory still throws out of `list()` — both unchanged. + +Consumer-visible behaviour: + +- `POST /api/v1/marketplace/install-local/:manifestId/reseed-sample-data` and + `…/purge-sample-data` keep returning `500 MARKETPLACE_STORAGE_FAILED` — the + same failure, so a client branching on the code is unaffected — but the + message now names the ledger file to repair or remove and quotes the cause + verbatim, and a matching `warn` line goes to the server log. +- The install path's ADR-0120 D5e posture gate is unchanged on purpose: a + corrupt entry still counts as "no attestation on record", so the one-time + installation-wide-unique ceremony is asked again rather than skipped. diff --git a/packages/cloud-connection/src/index.ts b/packages/cloud-connection/src/index.ts index b717a7253d..ba90220c7c 100644 --- a/packages/cloud-connection/src/index.ts +++ b/packages/cloud-connection/src/index.ts @@ -39,12 +39,16 @@ export type { MarketplaceInstallLocalPluginConfig } from './marketplace-install- // ADR-0007 step ⑤ — the local desired-state ledger, exported as a first-class // seam so hosts/reconcilers can read the same ledger without going through HTTP. export { LocalManifestSource, DEFAULT_INSTALLED_PACKAGES_DIR } from './local-manifest-source.js'; -// `list()`'s return contract is part of that seam: it reports what it could NOT -// read alongside what it could (#5413), so a consumer cannot mistake half a -// ledger for a whole one. +// The RETURN CONTRACTS of both read paths are part of that seam. `list()` +// reports what it could NOT read alongside what it could (#5413), so a consumer +// cannot mistake half a ledger for a whole one; `read()` separates "never +// installed" from "installed but unreadable" (#5426), so a consumer that meant +// the first cannot silently answer for the second. Both carry the thrower's own +// object in a `SkippedManifestEntry`. export type { InstalledManifestEntry, InstalledManifestListing, + InstalledManifestLookup, SkippedManifestEntry, } from './local-manifest-source.js'; export { CloudConnectionPlugin, createCloudConnectionPlugin } from './cloud-connection-plugin.js'; diff --git a/packages/cloud-connection/src/local-manifest-source.test.ts b/packages/cloud-connection/src/local-manifest-source.test.ts index 13b169345e..14daed1069 100644 --- a/packages/cloud-connection/src/local-manifest-source.test.ts +++ b/packages/cloud-connection/src/local-manifest-source.test.ts @@ -3,11 +3,12 @@ /** * LocalManifestSource — the local desired-state ledger (cloud ADR-0007 ⑤). * Pure local file operations: list/read/has/write/remove, corrupt-file - * tolerance AND corrupt-file REPORTING (#5413), and manifest-id sanitisation. + * tolerance AND corrupt-file REPORTING on BOTH read paths — `list()` (#5413) + * and `read()` (#5426) — and manifest-id sanitisation. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readdirSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readdirSync, existsSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { LocalManifestSource, type InstalledManifestEntry } from './local-manifest-source.js'; @@ -30,7 +31,10 @@ describe('LocalManifestSource', () => { it('starts empty and lists nothing for a missing directory', () => { const src = new LocalManifestSource(join(dir, 'does-not-exist-yet')); expect(src.list()).toEqual({ entries: [], skipped: [] }); - expect(src.read('com.acme.crm')).toBeNull(); + // #5426 — absence carries NO failure. That is the whole distinction: + // `{ entry: null }` means "never installed", and a consumer can tell it + // apart from "installed, unreadable" without guessing. + expect(src.read('com.acme.crm')).toEqual({ entry: null }); expect(src.has('com.acme.crm')).toBe(false); }); @@ -38,11 +42,14 @@ describe('LocalManifestSource', () => { const src = new LocalManifestSource(dir); src.write(entry('com.acme.crm')); expect(src.has('com.acme.crm')).toBe(true); - expect(src.read('com.acme.crm')?.version).toBe('1.0.0'); + expect(src.read('com.acme.crm').entry?.version).toBe('1.0.0'); + // A clean read reports no failure either — `failure` is a finding, not + // a status field that is always present. + expect(src.read('com.acme.crm').failure).toBeUndefined(); src.write(entry('com.acme.crm', '1.1.0')); // upsert, same file expect(src.list().entries).toHaveLength(1); - expect(src.read('com.acme.crm')?.version).toBe('1.1.0'); + expect(src.read('com.acme.crm').entry?.version).toBe('1.1.0'); }); it('remove deletes the entry and reports absence', () => { @@ -53,13 +60,75 @@ describe('LocalManifestSource', () => { expect(src.list()).toEqual({ entries: [], skipped: [] }); }); - it('skips corrupt ledger files in list() and nulls them in read()', () => { + it('tolerates a corrupt ledger file in both read paths — one bad file costs only itself', () => { const src = new LocalManifestSource(dir); src.write(entry('com.acme.good')); writeFileSync(join(dir, 'com.acme.bad.json'), '{not json', 'utf8'); - // Still skipped — that half was never the bug. + + // Tolerance was never the bug and has not changed: `list()` still hands + // back the good entry, and `read()` still refuses to throw at a caller + // that asked for the bad one. expect(src.list().entries.map((e) => e.manifestId)).toEqual(['com.acme.good']); - expect(src.read('com.acme.bad')).toBeNull(); + expect(src.read('com.acme.bad').entry).toBeNull(); + }); + + // ── #5426 — read()'s null said WHICH of the two things it meant ───── + // + // ⚠️ FIXTURE NOTE: the assertion above used to be this file's whole + // statement about a corrupt single read — `expect(src.read('com.acme.bad')) + // .toBeNull()`, which is the exact limb this issue removed and would have + // stayed green for the wrong reason (a merged null is null either way). + // Tolerance keeps its assertion above; the tests below pin the fact the old + // one could not see. + + it('separates "never installed" from "installed but unreadable"', () => { + const src = new LocalManifestSource(dir); + // The issue's repro: a truncated ledger file for an installed package. + writeFileSync(join(dir, 'com.acme.crm.json'), '{"manifestId":"com.acme.crm","manifest":{', 'utf8'); + + const absent = src.read('com.acme.never-installed'); + const corrupt = src.read('com.acme.crm'); + + // Both still hand back no entry — the tolerant half is unchanged. + expect(absent.entry).toBeNull(); + expect(corrupt.entry).toBeNull(); + // …and they are now TELLABLE APART, which is the entire issue. Two + // handlers check `has()` first, so for them only the second can happen, + // and they had nothing to say about it but "Failed to read manifest + // cache." + expect(absent.failure).toBeUndefined(); + expect(corrupt.failure).toBeDefined(); + expect(corrupt.failure!.file).toBe('com.acme.crm.json'); + // The THROWN object, not a sentence this class invented. + expect(corrupt.failure!.cause).toBeInstanceOf(Error); + expect(String((corrupt.failure!.cause as Error).message)).toMatch(/JSON/i); + }); + + it('read(): reports an UNREADABLE file, not only an unparseable one', () => { + const src = new LocalManifestSource(dir); + // A directory where the ledger file should be: `readFileSync` throws + // EISDIR. Same swallowed null before #5426, entirely different repair — + // which is why the cause travels instead of a summary. + mkdirSync(join(dir, 'com.acme.crm.json')); + + const { entry, failure } = src.read('com.acme.crm'); + + expect(entry).toBeNull(); + expect((failure!.cause as NodeJS.ErrnoException).code).toBe('EISDIR'); + }); + + it('names the SANITISED filename, so the reported path is the real one', () => { + // `read()` reports the file it actually opened, not the manifest id it + // was handed — a consumer joins it onto the ledger dir and tells the + // operator what to repair. A hostile/odd id must not produce a path + // that points at nothing. + const src = new LocalManifestSource(dir); + writeFileSync(join(dir, 'com_acme_crm@bad.json'.replace('@', '_')), 'nope', 'utf8'); + + const { failure } = src.read('com_acme_crm@bad'); + + expect(failure!.file).toBe('com_acme_crm_bad.json'); + expect(existsSync(join(dir, failure!.file))).toBe(true); }); // ── #5413 — a skipped file is REPORTED, not merely skipped ────────── diff --git a/packages/cloud-connection/src/local-manifest-source.ts b/packages/cloud-connection/src/local-manifest-source.ts index f5bd4303a0..b162786396 100644 --- a/packages/cloud-connection/src/local-manifest-source.ts +++ b/packages/cloud-connection/src/local-manifest-source.ts @@ -87,6 +87,50 @@ export interface SkippedManifestEntry { cause: unknown; } +/** + * What {@link LocalManifestSource.read} hands back for ONE manifest id — the + * entry if it parsed, and the reason if it did not (#5426). + * + * `read()` used to answer `null` to two different questions at once: "this + * manifest was never installed" and "it was installed, but its ledger file + * cannot be read". The comment on it (`null when absent or unreadable`) says + * the merge was deliberate; the consequence was not. Two HTTP handlers check + * `has()` first — so absence is already ruled out — and then had nothing left + * to say but `500 Failed to read manifest cache.`: a sentence that points at + * itself, while `Unexpected end of JSON input` / `EACCES` / `EISDIR` — the one + * fact that names the fix — was discarded in an un-bound `catch`. + * + * The two facts are now distinguishable **structurally**, not by convention: + * + * | On disk | `entry` | `failure` | + * |---------------------------------|---------|-----------------------| + * | no file for this manifest id | `null` | absent | + * | a file that will not parse/read | `null` | `{ file, cause }` | + * | a file that parsed | entry | absent | + * + * A caller that legitimately treats both nulls alike keeps doing so by reading + * `.entry` — the install path's ADR-0120 D5e gate does exactly that on purpose + * (a corrupt entry means "no attestation on record", so the one-time ceremony + * is asked again rather than skipped: the fail-safe direction). What changed is + * that conflating them is now a decision a caller makes in the open, instead of + * the only thing this method let it do. + */ +export interface InstalledManifestLookup { + /** The parsed entry, or `null` when there is none to hand back. */ + entry: InstalledManifestEntry | null; + /** + * Present ONLY when a ledger file exists for this manifest id and could not + * be turned into an entry. Absent for a clean read AND for a genuine + * absence — `failure === undefined` with `entry === null` means "not + * installed", which is the distinction the old `null` erased. + * + * Shares {@link SkippedManifestEntry} with `list()` deliberately: it is the + * same fact about the same file, and a consumer that reports both (`os + * doctor`-style) should not need two shapes to say one thing. + */ + failure?: SkippedManifestEntry; +} + /** * What {@link LocalManifestSource.list} hands back — what it READ, and what it * could NOT (#5413). @@ -164,14 +208,29 @@ export class LocalManifestSource { return { entries, skipped }; } - /** Read one entry; null when absent or unreadable. */ - read(manifestId: string): InstalledManifestEntry | null { - const file = this.fileFor(manifestId); - if (!existsSync(file)) return null; + /** + * Read one entry — and, when there is none, say WHICH of the two reasons + * applies (#5426). + * + * See {@link InstalledManifestLookup} for the table. In short: absent → + * `{ entry: null }`; unreadable → `{ entry: null, failure: { file, cause } }` + * with the thrown object carried as-is, never re-wrapped and never + * stringified into a sentence this class invented. + * + * ⚠️ Deliberately NOT total in the other direction: this method does not + * validate the parsed value's SHAPE. A file holding well-formed JSON that + * is not an installed-package entry parses, and is handed back — same as + * before. Schema-checking the ledger is a separate contract decision + * (which schema, what a rejection means at boot) and is not made here. + */ + read(manifestId: string): InstalledManifestLookup { + const name = safeFilename(manifestId); + const file = join(this.dir, name); + if (!existsSync(file)) return { entry: null }; try { - return JSON.parse(readFileSync(file, 'utf8')); - } catch { - return null; + return { entry: JSON.parse(readFileSync(file, 'utf8')) }; + } catch (cause) { + return { entry: null, failure: { file: name, cause } }; } } diff --git a/packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts b/packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts index fadb47b030..430c250a61 100644 --- a/packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts +++ b/packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts @@ -70,6 +70,9 @@ function makeCtx(rawApp: any) { manifest: { register: vi.fn() }, objectql: { syncSchemas: async () => undefined, find: vi.fn(async () => [{ id: 'x' }]) }, metadata: {}, + // #5426's two handlers authenticate first; without this they answer 401 + // and never reach the ledger read under test. + auth: { api: { getSession: async () => ({ user: { id: 'admin' } }) } }, }; return { ctx: { @@ -86,15 +89,15 @@ function makeCtx(rawApp: any) { }; } -/** A Hono-ish context, enough for the GET handler. */ -function makeC() { +/** A Hono-ish context. `manifestId` carries the `:manifestId` route value. */ +function makeC(manifestId?: string) { 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 () => ({}), - param: () => undefined, + param: (k: string) => (k === 'manifestId' ? manifestId : undefined), header: () => undefined, }, json, @@ -131,6 +134,15 @@ function writeBrokenEntry(file = 'broken.json') { writeFileSync(join(dir, file), '{"manifestId":"broken","manifest":{"objects":[{"name":"acct"', 'utf8'); } +/** + * #5426's repro: a truncated file at the ledger path a given manifest id maps + * to — so `has()` answers TRUE and `read()` cannot parse it, which is exactly + * the state reseed and purge walked into. + */ +function writeCorruptEntryFor(manifestId: string) { + writeFileSync(join(dir, `${manifestId}.json`), `{"manifestId":"${manifestId}","manifest":{"data":[`, 'utf8'); +} + /** Boot a fresh plugin so `kernel:ready` runs rehydrate + mounts the routes. */ async function boot() { const rawApp = makeRawApp(); @@ -229,7 +241,82 @@ describe('handleList() — the console list that looked complete', () => { // Named for what the CONSUMER lost, not for what the producer did. expect(row).toContain('MISSING from the installed-apps list'); }); +}); + +/** + * #5426 — the OTHER return contract in the same file: `read()`. + * + * These two handlers already branch on the null (unlike `list()`'s short array, + * which nobody could see), so the defect here is lighter — but the answer they + * could give was `500 Failed to read manifest cache.` and nothing else, after + * `has()` had just confirmed the file exists. + */ +describe('handleReseed() / handlePurge() — the 500 that pointed at itself', () => { + it.each([ + ['reseed', 'reseed-sample-data', 'reseed sample data'], + ['purge', 'purge-sample-data', 'purge sample data'], + ])('%s: the 500 names the file and quotes the cause (#5426)', async (_label, route, attempted) => { + // ── The defect ─────────────────────────────────────────────────── + // Both handlers call `has()` (true — the file IS there) and then + // `read()`, which answered `null` for "absent OR unreadable" and threw + // the reason away in an un-bound `catch`. The operator got + // `500 Failed to read manifest cache.` — a sentence whose only content + // is that the thing it just did failed, with `has()` having just said + // the opposite one line earlier, and nothing in the server log. + writeCorruptEntryFor('app.test.crm'); + const { rawApp, ctx } = await boot(); + ctx.logger.warn.mockClear(); // isolate from rehydrate's own warns + + const c = makeC('app.test.crm'); + const res = await rawApp.routes.get(`POST /api/v1/marketplace/install-local/:manifestId/${route}`)!(c); + + // ① The wire CODE is unchanged — the same failure, newly explained. A + // client branching on the code must not have to change. + expect(res.status).toBe(500); + expect(res.payload.error.code).toBe('MARKETPLACE_STORAGE_FAILED'); + // ② THE assertion of this issue: the thrower's own words reach the + // operator holding the failed response. + expect(res.payload.error.message).toMatch(/JSON/i); + // ③ …together with the file to repair. Self-reference → an address. + expect(res.payload.error.message).toContain(join(dir, 'app.test.crm.json')); + expect(res.payload.error.message).toContain('app.test.crm'); + // ④ And a durable copy in the server log, for whoever reads it later + // instead of the HTTP response. + const row = warnings(ctx).find((s) => s.includes('app.test.crm.json')); + expect(row).toBeDefined(); + expect(row).toContain(`cannot ${attempted}`); + expect(row).toMatch(/JSON/i); + }, 60_000); + + it('reseed: an unreadable file, not only an unparseable one, is explained', async () => { + // EISDIR, not a parse error — a different repair, which is exactly why + // the cause travels unwrapped instead of being summarised at the catch. + mkdirSync(join(dir, 'app.test.crm.json')); + const { rawApp } = await boot(); + + const c = makeC('app.test.crm'); + const res = await rawApp.routes.get('POST /api/v1/marketplace/install-local/:manifestId/reseed-sample-data')!(c); + + expect(res.status).toBe(500); + expect(res.payload.error.code).toBe('MARKETPLACE_STORAGE_FAILED'); + expect(res.payload.error.message).toContain('EISDIR'); + }, 60_000); + + it('reseed: a package that was never installed still gets 404, not the storage 500', async () => { + // The other half of the split. `read()` distinguishing the two nulls + // must not make "never installed" start reading as "corrupt": that + // answer belongs to `has()` and stays a 404. + const { rawApp } = await boot(); + + const c = makeC('app.test.nope'); + const res = await rawApp.routes.get('POST /api/v1/marketplace/install-local/:manifestId/reseed-sample-data')!(c); + + expect(res.status).toBe(404); + expect(res.payload.error.code).toBe('RESOURCE_NOT_FOUND'); + }, 60_000); +}); +describe('handleList() — the wire shape it kept', () => { it('leaves the wire shape untouched — the readable entries, as before', async () => { // ⛔ Deliberate: the skip is NOT added to the response body. Changing // this endpoint's schema is a separate decision (see the handler's diff --git a/packages/cloud-connection/src/marketplace-install-local-heal.test.ts b/packages/cloud-connection/src/marketplace-install-local-heal.test.ts index f41c1f7cf5..91f95fe3d3 100644 --- a/packages/cloud-connection/src/marketplace-install-local-heal.test.ts +++ b/packages/cloud-connection/src/marketplace-install-local-heal.test.ts @@ -158,7 +158,7 @@ describe('rehydrate sample-data healing', () => { expect(loadCalls[0].config.organizationId).toBeUndefined(); // The ledger now records that sample data is present again. - const entry = new LocalManifestSource(dir).read(MANIFEST.id)!; + const entry = new LocalManifestSource(dir).read(MANIFEST.id).entry!; expect(entry.withSampleData).toBe(true); expect(entry.sampleDataPurged).toBe(false); @@ -190,7 +190,7 @@ describe('rehydrate sample-data healing', () => { seedResult = { summary: { totalInserted: 0, totalUpdated: 0, totalSkipped: 0 }, errors: [{ message: 'database is locked' }] }; const { ctx } = await rehydrateWith({}, { crm_x: [], crm_y: [] }); expect(loadCalls).toHaveLength(1); - const entry = new LocalManifestSource(dir).read(MANIFEST.id)!; + const entry = new LocalManifestSource(dir).read(MANIFEST.id).entry!; expect(entry.withSampleData).toBe(false); // The failure is loud, with the underlying reason. expect((ctx.logger.warn as any).mock.calls.some((c: any[]) => String(c[0]).includes('database is locked'))).toBe(true); @@ -221,7 +221,7 @@ describe('rehydrate sample-data healing', () => { makeC({}, MANIFEST.id), ); expect(purgeRes.payload?.success).toBe(true); - expect(new LocalManifestSource(dir).read(MANIFEST.id)?.sampleDataPurged).toBe(true); + expect(new LocalManifestSource(dir).read(MANIFEST.id).entry?.sampleDataPurged).toBe(true); // Restart (fresh plugin over the same ledger, DB now empty): no reseed. loadCalls = []; @@ -248,6 +248,6 @@ describe('rehydrate sample-data healing', () => { makeC({ manifest: MANIFEST }), ); expect(installRes.payload?.success).toBe(true); - expect(new LocalManifestSource(dir).read(MANIFEST.id)?.withSampleData).toBe(true); + expect(new LocalManifestSource(dir).read(MANIFEST.id).entry?.withSampleData).toBe(true); }); }); diff --git a/packages/cloud-connection/src/marketplace-install-local-plugin.ts b/packages/cloud-connection/src/marketplace-install-local-plugin.ts index 87caba8ea7..0f2347d3d6 100644 --- a/packages/cloud-connection/src/marketplace-install-local-plugin.ts +++ b/packages/cloud-connection/src/marketplace-install-local-plugin.ts @@ -70,6 +70,19 @@ import type { IHttpServer } from '@objectstack/spec/contracts'; const ROUTE_BASE = '/api/v1/marketplace/install-local'; +/** + * A ledger read failure in the thrower's own words (#5413 / #5426). + * + * The `cause` travels from `LocalManifestSource` unwrapped precisely so it can + * be quoted here rather than replaced by a summary. `.message` first because + * that is the operational sentence (`EACCES: permission denied, open '…'`); + * `.name` only when a thrower left the message empty, which is still better + * than the empty string. + */ +function describeLedgerCause(cause: unknown): string { + return cause instanceof Error ? (cause.message || cause.name) : String(cause); +} + /** Best-effort manifest id from a registry package entry (shape varies). */ function manifestIdOf(p: any): string | undefined { return p?.manifest?.id ?? p?.id ?? p?.manifest?.name ?? undefined; @@ -583,7 +596,17 @@ export class MarketplaceInstallLocalPlugin implements Plugin { // to the ledger: a stopped install must leave the runtime exactly as // it found it, so the installer can rewrite the metadata and retry // without an uninstall in between. - const previousEntry = this.ledger.read(manifestId); + // + // #5426 — `.entry` alone, on purpose. A ledger file that will not + // parse is treated here exactly as "no previous attestation", so a + // corrupt entry makes the gate ASK AGAIN rather than skip: the + // fail-safe direction (worst case, a one-time ceremony is repeated; + // the case that must never happen — an installation-wide unique + // going unconfirmed because its record was unreadable — cannot). + // Conflating the two nulls is the right call at THIS call site; it + // is now made here, in the open, instead of by the ledger for + // everyone. + const previousEntry = this.ledger.read(manifestId).entry; const gate = this.evaluateGlobalUniqueGate(manifest, previousEntry, body, userId); if (gate.blocked) { ctx.logger?.warn?.( @@ -926,9 +949,12 @@ export class MarketplaceInstallLocalPlugin implements Plugin { if (!this.ledger.has(manifestId)) { return c.json({ success: false, error: { code: 'RESOURCE_NOT_FOUND', message: `No marketplace install for ${manifestId}.` } }, 404); } - const entry: InstalledEntry | null = this.ledger.read(manifestId); + // #5426 — `has()` above already answered "is it installed", so reaching + // this with no entry means the file is THERE and unreadable. The reason + // travels to the operator instead of dying in a `catch`. + const { entry, failure } = this.ledger.read(manifestId); if (!entry) { - return c.json({ success: false, error: { code: 'MARKETPLACE_STORAGE_FAILED', message: 'Failed to read manifest cache.' } }, 500); + return this.unreadableLedgerEntry(c, ctx, manifestId, failure, 'reseed sample data'); } const summary = await this.applySideEffects(ctx, entry.manifest, { seedNow: true, c }); @@ -1006,9 +1032,12 @@ export class MarketplaceInstallLocalPlugin implements Plugin { if (!this.ledger.has(manifestId)) { return c.json({ success: false, error: { code: 'RESOURCE_NOT_FOUND', message: `No marketplace install for ${manifestId}.` } }, 404); } - const entry: InstalledEntry | null = this.ledger.read(manifestId); + // #5426 — same shape as reseed: `has()` said the file is there, so a + // missing entry here is an unreadable one, and the operator gets the + // reason rather than a sentence that points at itself. + const { entry, failure } = this.ledger.read(manifestId); if (!entry) { - return c.json({ success: false, error: { code: 'MARKETPLACE_STORAGE_FAILED', message: 'Failed to read manifest cache.' } }, 500); + return this.unreadableLedgerEntry(c, ctx, manifestId, failure, 'purge sample data'); } const datasets = Array.isArray(entry.manifest?.data) @@ -1330,11 +1359,64 @@ export class MarketplaceInstallLocalPlugin implements Plugin { */ private warnSkippedLedgerEntries = (ctx: PluginContext, skipped: SkippedManifestEntry[], what: string): void => { for (const { file, cause } of skipped) { - const reason = cause instanceof Error ? (cause.message || cause.name) : String(cause); ctx.logger?.warn?.( `[MarketplaceInstallLocal] unreadable ledger entry ${file} — ${what} ` - + `(repair or remove ${join(this.storageDir, file)}): ${reason}`, + + `(repair or remove ${join(this.storageDir, file)}): ${describeLedgerCause(cause)}`, ); } }; + + /** + * The 500 for "`has()` said the entry is there, `read()` could not turn it + * into one" — with the reason attached (#5426). + * + * Shared by reseed and purge because they hit the identical wall, and both + * used to answer `Failed to read manifest cache.`: a sentence whose only + * content is that the thing it just did failed. The operator was left with + * nothing to act on — not the file, not whether it was truncated, locked or + * a directory — while the object that said all three had been dropped in an + * un-bound `catch` one layer down. + * + * Deliberate choices here: + * - **`code` is unchanged** (`MARKETPLACE_STORAGE_FAILED`). This is the same + * failure it always was; only its explanation is new. A client branching + * on the code keeps working — the fix must not cost a wire break. + * - **The cause is quoted, not paraphrased** (#5390 house style). `EACCES`, + * `EISDIR` and `Unexpected end of JSON input` are three different repairs, + * and the thrower words each better than any sentence here could. + * - **In the response body, not only the log.** These are operator-facing + * admin endpoints — the person who can fix the file is the person holding + * the failed response — and the server line is the durable copy for + * whoever reads the log later instead. + */ + private unreadableLedgerEntry = ( + c: any, + ctx: PluginContext, + manifestId: string, + failure: SkippedManifestEntry | undefined, + attempted: string, + ): Response => { + // `failure === undefined` here means the file vanished (or was emptied) + // between `has()` and `read()` — a real race, rare, and worth wording + // honestly rather than blaming a cause we were never handed. Note it + // names the DIRECTORY: with no failure there is no file name to quote, + // and inventing one would be the same self-referential mistake at a + // different address. + const detail = failure + ? `repair or remove ${join(this.storageDir, failure.file)}: ${describeLedgerCause(failure.cause)}` + : `its ledger file under ${this.storageDir} no longer yields an entry ` + + `(removed or emptied between the existence check and the read)`; + + ctx.logger?.warn?.( + `[MarketplaceInstallLocal] cannot ${attempted} for ${manifestId} — its ledger entry is unreadable (${detail})`, + ); + + return c.json({ + success: false, + error: { + code: 'MARKETPLACE_STORAGE_FAILED', + message: `Failed to read the manifest cache entry for ${manifestId} (${detail})`, + }, + }, 500); + }; } diff --git a/packages/cloud-connection/src/marketplace-install-local-posture-gate.test.ts b/packages/cloud-connection/src/marketplace-install-local-posture-gate.test.ts index 2a78f46314..c529006210 100644 --- a/packages/cloud-connection/src/marketplace-install-local-posture-gate.test.ts +++ b/packages/cloud-connection/src/marketplace-install-local-posture-gate.test.ts @@ -24,7 +24,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { MarketplaceInstallLocalPlugin } from './marketplace-install-local-plugin.js'; @@ -167,7 +167,7 @@ describe("ADR-0120 D5e — install into an 'isolated' environment", () => { expect(register).not.toHaveBeenCalled(); expect(syncSchemas).not.toHaveBeenCalled(); - expect(new LocalManifestSource(dir).read('com.acme.mrp')).toBeNull(); + expect(new LocalManifestSource(dir).read('com.acme.mrp').entry).toBeNull(); }); it('confirming records the attestation in the install manifest and installs', async () => { @@ -178,7 +178,7 @@ describe("ADR-0120 D5e — install into an 'isolated' environment", () => { expect(res.payload.success).toBe(true); expect(register).toHaveBeenCalled(); - const entry = new LocalManifestSource(dir).read('com.acme.mrp')!; + const entry = new LocalManifestSource(dir).read('com.acme.mrp').entry!; expect(entry.globalUniqueAttestation).toMatchObject({ posture: 'isolated', confirmed: EXPECTED_IDS.slice().sort(), @@ -199,10 +199,39 @@ describe("ADR-0120 D5e — install into an 'isolated' environment", () => { const res = await second.install(makeC({ manifest: APP_WITH_GLOBAL_UNIQUES })); expect(res.payload.success).toBe(true); - expect(new LocalManifestSource(dir).read('com.acme.mrp')!.globalUniqueAttestation!.confirmed) + expect(new LocalManifestSource(dir).read('com.acme.mrp').entry!.globalUniqueAttestation!.confirmed) .toEqual(EXPECTED_IDS.slice().sort()); }); + it('ASKS AGAIN when the attestation record is corrupt — fail-safe, not fail-open', async () => { + // #5426 — the direction pin. `read()` now separates "never installed" + // from "installed but unreadable"; this call site deliberately keeps + // treating them alike (`.entry`), and this test says WHICH way that + // conflation must fall. + // + // ⚠️ REVERSE-VERIFICATION, stated honestly: reverting #5426 does NOT + // turn this red. The old `read()` returned a bare `null` for a corrupt + // file, so the gate already re-asked — the behaviour is unchanged and + // that is the point (the wiring is semantically equivalent). What this + // pins is the FUTURE: now that a corrupt entry is distinguishable, the + // tempting "we know it was installed, treat it as attested" reading is + // one line away, and it would skip a one-time ceremony over a file + // nobody could read. Repeating the ceremony costs a prompt; skipping it + // can expose one customer's value to another (ADR-0120 S10/S14). + process.env.OS_TENANCY_POSTURE = 'isolated'; + const first = await mountInstall(dir); + await first.install(makeC({ manifest: APP_WITH_GLOBAL_UNIQUES, confirmGlobalUniques: true })); + // Truncate the attested entry in place — the issue's repro. + writeFileSync(join(dir, 'com.acme.mrp.json'), '{"manifestId":"com.acme.mrp","globalUniqu', 'utf8'); + + const second = await mountInstall(dir); + const res = await second.install(makeC({ manifest: APP_WITH_GLOBAL_UNIQUES })); + + expect(res.status).toBe(409); + expect(res.payload.error.code).toBe('UNIQUE_SCOPE_CONFIRMATION_REQUIRED'); + expect(res.payload.error.details.findings.map((f: any) => f.id)).toEqual(EXPECTED_IDS); + }); + it('asks about a NEW constraint an upgrade introduces, and only that one', async () => { process.env.OS_TENANCY_POSTURE = 'isolated'; const first = await mountInstall(dir); @@ -236,7 +265,7 @@ describe("ADR-0120 D5e — install into an 'isolated' environment", () => { expect(res.status).toBe(409); expect(res.payload.error.details.findings.map((f: any) => f.id)).toEqual(['material:index:external_id']); - expect(new LocalManifestSource(dir).read('com.acme.mrp')).toBeNull(); + expect(new LocalManifestSource(dir).read('com.acme.mrp').entry).toBeNull(); }); it('an app whose uniques are all per-organization installs without ceremony', async () => { @@ -256,7 +285,7 @@ describe("ADR-0120 D5e — install into an 'isolated' environment", () => { })); expect(res.payload.success).toBe(true); - expect(new LocalManifestSource(dir).read('com.acme.clean')!.globalUniqueAttestation).toBeUndefined(); + expect(new LocalManifestSource(dir).read('com.acme.clean').entry!.globalUniqueAttestation).toBeUndefined(); }); }); @@ -272,7 +301,7 @@ describe('ADR-0120 D5e — the gate is posture-scoped', () => { const res = await install(makeC({ manifest: APP_WITH_GLOBAL_UNIQUES })); expect(res.payload.success).toBe(true); - expect(new LocalManifestSource(dir).read('com.acme.mrp')!.globalUniqueAttestation).toBeUndefined(); + expect(new LocalManifestSource(dir).read('com.acme.mrp').entry!.globalUniqueAttestation).toBeUndefined(); }); } @@ -304,7 +333,7 @@ describe('ADR-0120 D5e — the gate is posture-scoped', () => { const second = await mountInstall(dir); await second.install(makeC({ manifest: APP_WITH_GLOBAL_UNIQUES })); - expect(new LocalManifestSource(dir).read('com.acme.mrp')!.globalUniqueAttestation) + expect(new LocalManifestSource(dir).read('com.acme.mrp').entry!.globalUniqueAttestation) .toMatchObject({ posture: 'isolated', confirmed: EXPECTED_IDS.slice().sort() }); }); });