fix(cloud-connection): LocalManifestSource.read() 说清它的 null 指的是两件事里的哪一件 (#5426)

.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.

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';

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,19 +31,25 @@ 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);
     });
 
     it('write → has/read/list round-trips and upserts by manifestId', () => {
         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 ──────────

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 } };
         }
     }