From c38a329d5ea577012e987016353fbd02e724adb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 11:04:36 +0000 Subject: [PATCH] fix(cloud-connection,cli): `LocalManifestSource.list()` reports the ledger entries it could not read (#5413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A truncated / unreadable / unparseable file under `.objectstack/installed-packages/` was dropped in an un-bound per-file `catch` and `list()` returned a bare array, so a short list was indistinguishable from a complete one: no difference in the return value, no log, no count. All three consumers gave a confidently wrong answer — `rehydrate()` left the installed app unregistered (gone from the app switcher, its objects nonexistent) with nothing in the log, `handleList()` served the console a list that looked whole with `success: true`, and `os doctor` printed a clean `✓ Unique scope` over manifests it had never parsed. Skipping a corrupt file stays correct — one bad manifest must not stop a runtime booting the packages that are fine. Skipping it SILENTLY was the defect. `list()` now returns `{ entries, skipped }` (option A of the issue's decision point): reporting is the caller's job, and "I read only half the ledger" becomes a fact in the type rather than an absence. Enumerating the DIRECTORY still throws — a different fact from "some files in it would not parse", and #5412 already reports the two as separate rows. Wiring, per triage: - `rehydrate()` warns per skipped file, before the empty-entries early return (an all-corrupt ledger is the worst case, not the exempt one), naming the file, the consequence and the thrower's own words. - `handleList()` logs the same; the WIRE SHAPE is deliberately unchanged — putting the skip in the response body is a separate schema decision. - `os doctor` turns `skipped` into a `Unique scope` warning row and withholds the `✓` success line, alongside the directory-level row from #5412. #5414's `⚠ SCOPE BOUNDARY` test went red exactly as its own comment predicted and is rewritten as the positive assertion. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- .changeset/quiet-ledgers-speak-up.md | 38 +++ .../doctor-ledger-read-failure.test.ts | 126 ++++++--- packages/cli/src/commands/doctor.ts | 146 ++++++++-- packages/cloud-connection/src/index.ts | 9 +- .../src/local-manifest-source.test.ts | 89 +++++- .../src/local-manifest-source.ts | 78 +++++- ...place-install-local-corrupt-ledger.test.ts | 253 ++++++++++++++++++ .../src/marketplace-install-local-plugin.ts | 72 ++++- 8 files changed, 742 insertions(+), 69 deletions(-) create mode 100644 .changeset/quiet-ledgers-speak-up.md create mode 100644 packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts diff --git a/.changeset/quiet-ledgers-speak-up.md b/.changeset/quiet-ledgers-speak-up.md new file mode 100644 index 0000000000..1cf00cc46b --- /dev/null +++ b/.changeset/quiet-ledgers-speak-up.md @@ -0,0 +1,38 @@ +--- +'@objectstack/cloud-connection': minor +'@objectstack/cli': patch +--- + +`LocalManifestSource.list()` now reports the ledger entries it could NOT read + +A truncated, unreadable or unparseable file under +`.objectstack/installed-packages/` was skipped in an un-bound per-file `catch` +and `list()` returned a bare array, so a short list was indistinguishable from a +complete one — no difference in the return value, no log, no count. Three +consumers gave a confidently wrong answer: the installed app was never +registered at boot (gone from the app switcher, its objects nonexistent) with +nothing in the log, the console's installed-apps list came back short with +`success: true`, and `os doctor` printed `✓ Unique scope` over manifests it had +never parsed. + +Skipping a corrupt file stays correct — one bad manifest must not stop a runtime +booting the packages that are fine. Skipping it *silently* was the defect. + +**Breaking (`@objectstack/cloud-connection`):** `LocalManifestSource.list()` +returns `{ entries, skipped }` instead of `InstalledManifestEntry[]`. + +- FROM: `const entries = source.list();` +- TO: `const { entries, skipped } = source.list();` + +`skipped` is `Array< { file: string; cause: unknown } >` — the file's basename +and the object reading or parsing it threw, unwrapped. Callers that only want +the old behaviour read `.entries`; the point of the shape is that dropping +`skipped` is now something a caller has to do on purpose. Two new exported +types, `InstalledManifestListing` and `SkippedManifestEntry`. + +Enumerating the ledger DIRECTORY still throws out of `list()` — unchanged, and a +different fact from "some files in it would not parse". + +`os doctor` reports unparseable entries as a `Unique scope` warning row naming +each file with its cause, and withholds the `✓` success line, alongside the +directory-level row it already had. diff --git a/packages/cli/src/commands/doctor-ledger-read-failure.test.ts b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts index 8a26b975b0..b306b81c2f 100644 --- a/packages/cli/src/commands/doctor-ledger-read-failure.test.ts +++ b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts @@ -27,18 +27,23 @@ * * A false PASS, on the one constraint the `isolated` posture makes dangerous. * - * ── Scope boundary this file also pins ─────────────────────────────────── + * ── The second half, one layer down (#5413) ────────────────────────────── * - * The issue's stated repro — a truncated JSON entry inside the ledger — does - * NOT reach that `catch`, and #5412's fix does not change it. - * `LocalManifestSource.list()` skips unparseable files in its own per-file - * `catch` (`packages/cloud-connection/src/local-manifest-source.ts`), so a - * corrupt entry is dropped inside the PRODUCER and `list()` returns a short - * list indistinguishable from a complete one. Doctor sees a successful call. - * That is a real defect of the same family one layer down, it is a - * cross-package contract change to fix (filed as #5413), and it is pinned here - * as a boundary (`the corrupt-entry case is NOT covered`) rather than left for - * the next reader to re-derive — see that test's comment. + * This file used to pin a SCOPE BOUNDARY: the issue's stated repro — a + * truncated JSON entry inside the ledger — did NOT reach that `catch`, because + * `LocalManifestSource.list()` skipped unparseable files in its own per-file + * `catch` (`packages/cloud-connection/src/local-manifest-source.ts`) and + * returned a short list indistinguishable from a complete one. Doctor saw a + * successful call and printed the same false `✓ Unique scope`, over manifests + * it had never parsed. + * + * #5413 fixed that at the PRODUCER, where it belonged — `list()` now returns + * `{ entries, skipped }`, so "I read only half the ledger" is a fact in the + * type rather than an absence — and doctor turns `skipped` into its own row. + * The boundary case went red exactly as its comment predicted and is now the + * positive assertion below. The two facts stay separately reported: the + * directory could not be enumerated at all (#5412) versus it enumerated fine + * and some files in it would not parse (#5413). */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -64,9 +69,12 @@ const plain = (s: string) => s.replace(SGR, ''); /** The success line that must NOT appear when only half the check ran. */ const CLEAN_BILL = 'No unconfirmed installation-wide uniques'; -/** The head of the row that replaces it. */ +/** The head of the row that replaces it — the DIRECTORY-level failure (#5412). */ const LEDGER_HEADLINE = 'Could not read the installed-package ledger'; +/** The head of its ENTRY-level sibling (#5413). Deliberately distinct text. */ +const SKIPPED_HEADLINE = 'installed-package ledger entr'; + describe('installedPackageLedgerFailureCheck — the finding the shared catch used to eat', () => { it('quotes what was thrown, in the row AND in the verbose detail', () => { const err = Object.assign(new Error("ENOTDIR: not a directory, scandir '/p/.objectstack'"), { @@ -316,21 +324,23 @@ describe('os doctor, end to end, against an unreadable installed-package ledger' expect(run.exitCode).toBeUndefined(); }, 60_000); - it('⚠ SCOPE BOUNDARY: a CORRUPT ENTRY is still absorbed by the producer', async () => { - // This pins the issue's own stated repro as NOT FIXED, deliberately, and - // records why — so the next reader does not re-derive it from scratch or - // assume #5412 covered it. - // - // `LocalManifestSource.list()` skips unparseable files in its own per-file - // `catch`, so a truncated manifest never reaches doctor's `catch`: the - // call SUCCEEDS and returns a short list that is indistinguishable from a - // complete one. Doctor cannot tell the difference without re-implementing - // the producer's parsing rules in the consumer, which is precisely the - // lenient-consumer workaround this repo forbids. The fix belongs in - // `packages/cloud-connection` (a cross-package contract change to - // `list()`) and is filed as #5413. - // - // When #5413 lands, THIS TEST GOES RED — which is the point. + /** + * ── Was the ⚠ SCOPE BOUNDARY case, now flipped positive (#5413) ──────── + * + * This slot used to pin the issue's own stated repro as deliberately NOT + * FIXED: a truncated entry never reached doctor's `catch` because + * `LocalManifestSource.list()` skipped unparseable files in its own per-file + * `catch` and returned a short list indistinguishable from a complete one. + * Doctor could not have told the difference without re-implementing the + * producer's parsing rules in the consumer — the lenient-consumer workaround + * this repo forbids — so the fix went to the producer instead: `list()` now + * returns `{ entries, skipped }` and doctor reports the second half. + * + * The old case asserted `not.toContain('broken')` and went red exactly as its + * comment predicted. Rewritten as the positive assertion rather than deleted: + * the repro is the same, only the expected verdict inverted. + */ + it('reports a CORRUPT ENTRY by name instead of skipping it in silence', async () => { writeConfig(); fs.mkdirSync(ledgerPath(), { recursive: true }); fs.writeFileSync(path.join(ledgerPath(), 'good.json'), JSON.stringify(globalUniqueEntry('good'))); @@ -342,12 +352,66 @@ describe('os doctor, end to end, against an unreadable installed-package ledger' const run = await runDoctor(); - // The readable entry is reported… + // The readable entry is still reported, unchanged. expect(run.out).toContain("installed package 'good'"); - // …and the corrupt one is silently absent, with no row of any kind naming - // it. Not a passing behaviour — a pinned boundary. - expect(run.out).not.toContain('broken'); + // ① The corrupt one is named — the row that did not exist before #5413. + expect(run.out).toContain(SKIPPED_HEADLINE); + expect(run.out).toContain('broken.json'); + // ② With the parser's own words, not a summary doctor invented. + expect(run.out).toMatch(/JSON/i); + // ③ Under the `Unique scope` name column, like its directory-level sibling, + // so the row an operator scans for is present rather than missing. + expect(run.out).toContain('Unique scope'); + // ④ NOT the directory-level row: the directory read fine. Two distinct + // facts, two distinct headlines (#5412 vs #5413). expect(run.out).not.toContain(LEDGER_HEADLINE); + // Gauge: still a warning, report finishes, exit stays 0. + expect(run.out).toContain('Environment is functional but has some warnings'); + expect(run.exitCode).toBeUndefined(); + }, 60_000); + + it('withholds the clean bill when the ONLY finding is an unparseable entry', async () => { + // The false-PASS shape this issue is really about. The good entry declares + // no global unique, so before #5413 the advisory found nothing to say and + // printed `✓ Unique scope` — over a manifest it had never parsed. An + // unreadable manifest may declare an installation-wide unique; nobody can + // say it does not. + writeConfig(); + fs.mkdirSync(ledgerPath(), { recursive: true }); + fs.writeFileSync( + path.join(ledgerPath(), 'clean.json'), + JSON.stringify({ manifestId: 'clean', manifest: { objects: [] } }), + ); + fs.writeFileSync(path.join(ledgerPath(), 'broken.json'), '{"manifestId":"broken"'); + + const run = await runDoctor(); + + expect(run.out).not.toContain(CLEAN_BILL); + expect(run.out).toContain(SKIPPED_HEADLINE); + expect(run.out).toContain('broken.json'); + }, 60_000); + + it('names EVERY unparseable entry, and expands the causes under --verbose', async () => { + writeConfig(); + fs.mkdirSync(ledgerPath(), { recursive: true }); + fs.writeFileSync(path.join(ledgerPath(), 'one.json'), '{oops'); + fs.writeFileSync(path.join(ledgerPath(), 'two.json'), 'not json at all'); + + const plainRun = await runDoctor(); + const verboseRun = await runDoctor(['--verbose']); + + // One row is one line, so the row quotes the first cause and counts the + // rest; `fix` carries every file with its own cause. + expect(plainRun.out).toContain('2 installed-package ledger entries could not be read'); + expect(plainRun.out).toContain('(+1 more)'); + expect(plainRun.out).not.toContain('cause:'); + + expect(verboseRun.out).toContain('one.json'); + expect(verboseRun.out).toContain('two.json'); + expect(verboseRun.out).toContain('cause:'); + // The fix is per-file, so it has to say what to do with each one. + expect(verboseRun.out).toContain('Repair the JSON, or delete the file'); + expect(verboseRun.exitCode).toBeUndefined(); }, 60_000); }); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 8c36f0b17e..96a237d77f 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -807,10 +807,33 @@ interface UniqueScopeAdvisory { */ interface InstalledPackageLedgerReading { entries: any[]; + /** + * Ledger files that exist but could not be turned into entries (#5413). + * + * A DIFFERENT fact from `failure` below, and both can be empty while the + * other is not: `failure` means nothing at all was read, `skipped` means + * some of it was. Before #5413 this list could not exist — the producer + * dropped corrupt files inside its own un-bound `catch` and handed back a + * short array indistinguishable from a complete one. + */ + skipped: SkippedLedgerEntry[]; /** Present ONLY when the ledger EXISTS and could not be read. */ failure?: { cause: unknown }; } +/** + * One unreadable ledger file, as `@objectstack/cloud-connection` reports it. + * + * Structurally identical to that package's `SkippedManifestEntry`, declared + * here rather than imported because the package is loaded through a dynamic + * `import()` that must be allowed to fail (`os doctor` runs in checkouts that + * never had it), so there is no static type import to take. + */ +interface SkippedLedgerEntry { + file: string; + cause: unknown; +} + /** * Read the installed-package ledger without going through HTTP. * @@ -830,16 +853,18 @@ interface InstalledPackageLedgerReading { * tells the operator to stop looking. Case 2 now comes back as a `failure` the * caller turns into a warning row. * - * ⚠️ SCOPE BOUNDARY — this covers DIRECTORY-level read failures only. A single - * CORRUPT ENTRY never reaches this `catch`: `LocalManifestSource.list()` skips - * unparseable files in its own per-file `catch` - * (`packages/cloud-connection/src/local-manifest-source.ts`), so a truncated - * manifest is dropped inside the producer and `list()` returns a short list - * indistinguishable from a complete one. That is a producer-side defect with - * the same false-PASS shape one layer down, and it cannot be fixed from here - * without the consumer re-implementing the producer's parsing rules. Filed as - * #5413; pinned by the SCOPE BOUNDARY case in - * `doctor-ledger-read-failure.test.ts`, which goes red when #5413 lands. + * There is a THIRD fact, one layer down, and it is now reported too (#5413). + * A single CORRUPT ENTRY never reaches this `catch` and never will: + * `LocalManifestSource.list()` skips unparseable files in its own per-file + * `catch` (`packages/cloud-connection/src/local-manifest-source.ts`), which is + * the right behaviour — one truncated manifest must not stop a runtime from + * booting the packages that are fine. It used to skip them silently, returning + * a short list indistinguishable from a complete one, so doctor printed + * `✓ Unique scope` over packages it had never parsed: the same false PASS as + * case 2, one layer down. Fixing it from here would have meant re-implementing + * the producer's parsing rules in the consumer — the lenient-consumer + * workaround this repo forbids — so `list()` was changed to REPORT what it + * skipped, and this function passes that through as `skipped`. */ async function readInstalledPackageEntries(cwd: string): Promise { let mod: any; @@ -849,16 +874,21 @@ async function readInstalledPackageEntries(cwd: string): Promise { - if (!postureGatesGlobalUniques(posture)) return { advisories: [] }; + if (!postureGatesGlobalUniques(posture)) return { advisories: [], skippedLedgerEntries: [] }; const out: UniqueScopeAdvisory[] = []; for (const finding of collectGlobalUniques(config?.objects)) { @@ -910,7 +948,11 @@ async function findUnscopedGlobalUniques( out.push({ source: `installed package '${entry?.manifestId ?? entry?.packageId}'`, finding }); } } - return { advisories: out, ...(ledger.failure ? { ledgerFailure: ledger.failure } : {}) }; + return { + advisories: out, + skippedLedgerEntries: ledger.skipped, + ...(ledger.failure ? { ledgerFailure: ledger.failure } : {}), + }; } // ─── Filesystem Checks ────────────────────────────────────────────── @@ -1204,6 +1246,61 @@ export function installedPackageLedgerFailureCheck(err: unknown): HealthCheckRes }; } +/** + * What doctor reports when INDIVIDUAL ledger entries could not be parsed + * (#5413). + * + * The sibling of `installedPackageLedgerFailureCheck` one layer down. That one + * fires when the ledger DIRECTORY could not be read at all; this one fires when + * the directory read fine and some of the files in it did not. Both produce the + * same false PASS if unreported — `✓ Unique scope` over manifests doctor never + * parsed — and both therefore take the `Unique scope` name column and withhold + * the success line, for the reasons written out above. + * + * Why entry-level corruption is a finding at all, rather than something the + * producer just handles: skipping a corrupt file IS correct — one truncated + * manifest must not stop a runtime booting. But an unparsed manifest is a + * manifest nobody can vouch for, and this advisory's whole subject is + * installation-wide `unique` constraints that are dangerous under `isolated`. + * "It probably didn't declare one" is not an answer doctor is entitled to give. + * + * Two shape choices worth the words: + * + * • **Every file is named, with its own cause.** A single count ("2 entries + * skipped") would send the operator to `ls` the directory and guess. The + * fix for this finding is per-file — repair it or delete it — so the row + * has to carry which file, and `EACCES` vs `Unexpected end of JSON input` + * are different repairs. + * • **The row quotes the FIRST cause; `fix` carries them all** (#5390 body). + * One row is one line, the same bound every other finding here respects. + */ +export function installedPackageLedgerSkippedEntriesCheck( + skipped: SkippedLedgerEntry[], +): HealthCheckResult { + const described = skipped.map((s) => ({ file: s.file, cause: describeThrown(s.cause) })); + const n = described.length; + const noun = n === 1 ? 'entry' : 'entries'; + const head = described[0]!; + const more = n > 1 ? ` (+${n - 1} more)` : ''; + return { + name: 'Unique scope', + status: 'warning', + message: + `${n} installed-package ledger ${noun} could not be read (those packages NOT checked ` + + `for installation-wide uniques) — ${reportRowHeadline(`${head.file}: ${head.cause}`)}${more}`, + fix: + 'The ledger directory was read fine; these files inside it were not. Each one is an\n' + + ' installed package this runtime ALSO drops at boot — it is not registered with\n' + + ' the kernel and does not appear in the console\'s installed-apps list — so an\n' + + ' app missing from this environment is very likely one of the files below.\n' + + ' Repair the JSON, or delete the file to uninstall the package for real.\n' + + ' Under `.objectstack/installed-packages/`:\n' + + described + .map((s) => ` ${s.file}\n cause: ${indentUnderGutter(s.cause).replace(/\n/g, '\n ')}`) + .join('\n'), + }; +} + // ─── Command ──────────────────────────────────────────────────────── export default class Doctor extends Command { @@ -1502,7 +1599,7 @@ export default class Doctor extends Command { // so nothing is silently lost. if (postureReading.ok && postureGatesGlobalUniques(postureReading.posture)) { printStep("Checking unique scopes against the 'isolated' tenancy posture..."); - const { advisories, ledgerFailure } = await findUnscopedGlobalUniques( + const { advisories, ledgerFailure, skippedLedgerEntries } = await findUnscopedGlobalUniques( cwd, config, postureReading.posture, @@ -1519,10 +1616,23 @@ export default class Doctor extends Command { // that exists and could not be read is reported in its place; a // false `✓` here is worse than a missing check, because it is the // one thing that stops the operator looking further. + // + // #5413 — entry-level corruption is the same claim failing one layer + // down, so it gates the `✓` in exactly the same way. The two are + // reported independently rather than as an either/or: a directory + // that read fine can still hold three unparseable files, and each + // names a different package the advisory could not look at. + if (skippedLedgerEntries.length > 0) { + hasWarnings = true; + renderHealthCheckResult( + installedPackageLedgerSkippedEntriesCheck(skippedLedgerEntries), + flags.verbose, + ); + } if (ledgerFailure) { hasWarnings = true; renderHealthCheckResult(installedPackageLedgerFailureCheck(ledgerFailure.cause), flags.verbose); - } else if (advisories.length === 0) { + } else if (advisories.length === 0 && skippedLedgerEntries.length === 0) { printSuccess("Unique scope No unconfirmed installation-wide uniques for this 'isolated' environment"); } } diff --git a/packages/cloud-connection/src/index.ts b/packages/cloud-connection/src/index.ts index 1f23734dbe..b717a7253d 100644 --- a/packages/cloud-connection/src/index.ts +++ b/packages/cloud-connection/src/index.ts @@ -39,7 +39,14 @@ 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'; -export type { InstalledManifestEntry } 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. +export type { + InstalledManifestEntry, + InstalledManifestListing, + SkippedManifestEntry, +} from './local-manifest-source.js'; export { CloudConnectionPlugin, createCloudConnectionPlugin } from './cloud-connection-plugin.js'; export type { CloudConnectionPluginConfig } from './cloud-connection-plugin.js'; export { RuntimeConfigPlugin } from './runtime-config-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 61917c877f..13b169345e 100644 --- a/packages/cloud-connection/src/local-manifest-source.test.ts +++ b/packages/cloud-connection/src/local-manifest-source.test.ts @@ -3,11 +3,11 @@ /** * LocalManifestSource — the local desired-state ledger (cloud ADR-0007 ⑤). * Pure local file operations: list/read/has/write/remove, corrupt-file - * tolerance, and manifest-id sanitisation. + * tolerance AND corrupt-file REPORTING (#5413), and manifest-id sanitisation. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync, readdirSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { LocalManifestSource, type InstalledManifestEntry } from './local-manifest-source.js'; @@ -29,7 +29,7 @@ const entry = (manifestId: string, version = '1.0.0'): InstalledManifestEntry => 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([]); + expect(src.list()).toEqual({ entries: [], skipped: [] }); expect(src.read('com.acme.crm')).toBeNull(); expect(src.has('com.acme.crm')).toBe(false); }); @@ -41,7 +41,7 @@ describe('LocalManifestSource', () => { expect(src.read('com.acme.crm')?.version).toBe('1.0.0'); src.write(entry('com.acme.crm', '1.1.0')); // upsert, same file - expect(src.list()).toHaveLength(1); + expect(src.list().entries).toHaveLength(1); expect(src.read('com.acme.crm')?.version).toBe('1.1.0'); }); @@ -50,17 +50,94 @@ describe('LocalManifestSource', () => { src.write(entry('com.acme.crm')); expect(src.remove('com.acme.crm')).toBe(true); expect(src.remove('com.acme.crm')).toBe(false); - expect(src.list()).toEqual([]); + expect(src.list()).toEqual({ entries: [], skipped: [] }); }); it('skips corrupt ledger files in list() and nulls them in read()', () => { const src = new LocalManifestSource(dir); src.write(entry('com.acme.good')); writeFileSync(join(dir, 'com.acme.bad.json'), '{not json', 'utf8'); - expect(src.list().map((e) => e.manifestId)).toEqual(['com.acme.good']); + // Still skipped — that half was never the bug. + expect(src.list().entries.map((e) => e.manifestId)).toEqual(['com.acme.good']); expect(src.read('com.acme.bad')).toBeNull(); }); + // ── #5413 — a skipped file is REPORTED, not merely skipped ────────── + // + // Before this, `list()` returned a bare array: a short list carried no + // difference in the return value, no log and no count, so `rehydrate()` + // dropped an installed app out of a runtime in silence, `handleList()` + // served the console a list that looked whole, and `os doctor` printed + // `✓ Unique scope` over manifests it had never parsed. + + it('names every file it could not parse, and what parsing threw', () => { + const src = new LocalManifestSource(dir); + src.write(entry('com.acme.good')); + // The issue's repro verbatim: truncated mid-object. + writeFileSync(join(dir, 'broken.json'), '{"manifestId":"broken","manifest":{"objects":[{"name":"acct"', 'utf8'); + + const { entries, skipped } = src.list(); + + expect(entries.map((e) => e.manifestId)).toEqual(['com.acme.good']); + expect(skipped).toHaveLength(1); + expect(skipped[0]!.file).toBe('broken.json'); + // The THROWN object, not a sentence this class invented — the consumer + // quotes it (`os doctor` folds it onto a report row). + expect(skipped[0]!.cause).toBeInstanceOf(Error); + expect(String((skipped[0]!.cause as Error).message)).toMatch(/JSON/i); + }); + + it('reports an unreadable file, not only an unparseable one', () => { + const src = new LocalManifestSource(dir); + // A directory named `*.json` inside the ledger: `readFileSync` throws + // EISDIR. Same silent drop before #5413, different operational fix — + // which is exactly why the cause is carried rather than summarised. + mkdirSync(join(dir, 'notafile.json')); + + const { entries, skipped } = src.list(); + + expect(entries).toEqual([]); + expect(skipped.map((s) => s.file)).toEqual(['notafile.json']); + expect((skipped[0]!.cause as NodeJS.ErrnoException).code).toBe('EISDIR'); + }); + + it('reports EVERY corrupt file, not just the first', () => { + const src = new LocalManifestSource(dir); + writeFileSync(join(dir, 'a.json'), '{oops', 'utf8'); + writeFileSync(join(dir, 'b.json'), 'also not json', 'utf8'); + + const { entries, skipped } = src.list(); + + expect(entries).toEqual([]); + // An all-corrupt ledger is the worst case of this bug — every installed + // app missing — and it must not be the quiet one. + expect(skipped.map((s) => s.file).sort()).toEqual(['a.json', 'b.json']); + }); + + it('reports nothing skipped for a wholly intact ledger', () => { + const src = new LocalManifestSource(dir); + src.write(entry('com.acme.crm')); + src.write(entry('com.acme.hr')); + // Non-`.json` files are ignored, not "skipped" — nothing was lost. + writeFileSync(join(dir, 'README.txt'), 'not a ledger file', 'utf8'); + + const { entries, skipped } = src.list(); + + expect(entries).toHaveLength(2); + expect(skipped).toEqual([]); + }); + + it('still THROWS when the directory itself cannot be enumerated', () => { + // The #5412 boundary, pinned from the producer's side: "nothing at all + // was read" is a different fact from "some of it was", and `os doctor` + // reports them as different rows. Making `list()` total by catching + // here would collapse them back into one. + const notADir = join(dir, 'ledger'); + writeFileSync(notADir, 'this is a file, not the ledger directory\n', 'utf8'); + + expect(() => new LocalManifestSource(notADir).list()).toThrow(/ENOTDIR|ENOENT/); + }); + it('sanitises hostile manifest ids into safe filenames', () => { const src = new LocalManifestSource(dir); src.write(entry('../../etc/passwd')); diff --git a/packages/cloud-connection/src/local-manifest-source.ts b/packages/cloud-connection/src/local-manifest-source.ts index f509a42443..f5bd4303a0 100644 --- a/packages/cloud-connection/src/local-manifest-source.ts +++ b/packages/cloud-connection/src/local-manifest-source.ts @@ -69,6 +69,52 @@ export interface InstalledManifestEntry { globalUniqueAttestation?: GlobalUniqueAttestation; } +/** + * One ledger file {@link LocalManifestSource.list} could not turn into an entry + * (#5413). + * + * The `cause` is the thrown object itself, never a string this class invented: + * `ENOENT`, `EACCES` and `Unexpected end of JSON input` are three different + * operational facts with three different fixes, and the thrower words each of + * them better than any sentence here could. Consumers quote it (`os doctor` + * folds it onto a report row) or log it — that decision is theirs, which is the + * whole reason this is returned rather than logged in place. + */ +export interface SkippedManifestEntry { + /** The ledger file's basename, as it sits on disk (e.g. `com.acme.crm.json`). */ + file: string; + /** What reading or parsing that file threw. Never re-wrapped, never stringified. */ + cause: unknown; +} + +/** + * What {@link LocalManifestSource.list} hands back — what it READ, and what it + * could NOT (#5413). + * + * Skipping a corrupt file is deliberate and stays that way: one truncated + * manifest must not stop a runtime from booting the packages that are fine. + * The defect was skipping it **silently**. `list()` used to return a bare + * array, so a short list was indistinguishable from a complete one — no + * difference in the return value, no log, no count — and all three consumers + * gave a confidently wrong answer: `rehydrate()` dropped an installed app out + * of the runtime with no line in the log, `handleList()` served the console a + * list that looked whole, and `os doctor` printed `✓ Unique scope` over + * packages it had never seen. + * + * Reporting is the CALLER's job, not this class's: a boot wants a `warn`, an + * HTTP handler wants a log line without changing its wire shape, and `os + * doctor` wants a `HealthCheckResult` row — not stderr. Returning the fact + * (rather than taking a logger, or an optional `onSkip` callback that defaults + * to silence) is what makes "I read only half the ledger" impossible to ignore + * by accident: it is in the type, so a consumer that drops it has to say so. + */ +export interface InstalledManifestListing { + /** Every file that parsed into an entry. */ + entries: InstalledManifestEntry[]; + /** Every `.json` file in the ledger that did not, and why. */ + skipped: SkippedManifestEntry[]; +} + /** Default ledger location, relative to the runtime's working directory. */ export const DEFAULT_INSTALLED_PACKAGES_DIR = '.objectstack/installed-packages'; @@ -86,18 +132,36 @@ export class LocalManifestSource { : resolve(process.cwd(), DEFAULT_INSTALLED_PACKAGES_DIR); } - /** Every valid entry in the ledger (corrupt files are skipped). */ - list(): InstalledManifestEntry[] { - if (!existsSync(this.dir)) return []; - const out: InstalledManifestEntry[] = []; + /** + * Read the ledger: every entry that parsed, AND every file that did not + * (#5413). + * + * Corrupt files are still skipped — deliberately, and that has not changed. + * What changed is that they are now **reported** in the return value + * instead of vanishing into an un-bound `catch`. See + * {@link InstalledManifestListing} for why this is the caller's fact to + * report rather than something logged here. + * + * ⚠️ Note what is NOT in `skipped`: a failure to enumerate the DIRECTORY + * still throws out of this method. That is a different fact — nothing at + * all was read, not "some of it" — and `os doctor` already distinguishes + * the two (#5412). Do not wrap `readdirSync` in a `try` here to make this + * method total; the throw is the signal. + */ + list(): InstalledManifestListing { + if (!existsSync(this.dir)) return { entries: [], skipped: [] }; + const entries: InstalledManifestEntry[] = []; + const skipped: SkippedManifestEntry[] = []; for (const name of readdirSync(this.dir)) { if (!name.endsWith('.json')) continue; try { const raw = readFileSync(join(this.dir, name), 'utf8'); - out.push(JSON.parse(raw)); - } catch { /* skip corrupt files */ } + entries.push(JSON.parse(raw)); + } catch (cause) { + skipped.push({ file: name, cause }); + } } - return out; + return { entries, skipped }; } /** Read one entry; null when absent or unreadable. */ 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 new file mode 100644 index 0000000000..fadb47b030 --- /dev/null +++ b/packages/cloud-connection/src/marketplace-install-local-corrupt-ledger.test.ts @@ -0,0 +1,253 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A corrupt ledger entry is REPORTED by both runtime consumers (#5413). + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `LocalManifestSource.list()` dropped unparseable files in an un-bound + * per-file `catch` and returned a bare array, so a short list was + * indistinguishable from a complete one — no difference in the return value, + * no log, no count. The two consumers in this file both gave a confidently + * wrong answer: + * + * • `rehydrate()` — the installed app was never registered with the kernel. + * It vanished from the runtime (absent from the app switcher, its objects + * nonexistent) and not one line was written about it. The function warns + * when it cannot get the `manifest` service, but said nothing about + * reading fewer entries than the ledger holds. + * • `handleList()` — the console's "Installed Apps" list came back short, + * with `success: true`. + * + * Skipping the file was always right — one truncated manifest must not stop a + * runtime booting the packages that are fine. Skipping it SILENTLY was the + * defect. + * + * ── What is pinned here ────────────────────────────────────────────────── + * + * That each consumer names the file and quotes the cause, that the wire shape + * of `handleList()` is UNCHANGED (reporting the skip in the response body is a + * separate decision about that endpoint's schema, deliberately not made), and + * that an all-corrupt ledger — the worst case, every app missing — is not the + * one case that stays quiet. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +// The heal path pulls in the seed loader; stub it exactly as the heal suite +// does so these tests exercise rehydrate's reporting, not seeding. +vi.mock('@objectstack/runtime', () => ({ + SeedLoaderService: class { + async load() { return { summary: { totalInserted: 0, totalUpdated: 0, totalSkipped: 0 }, errors: [] }; } + }, + recordSeedOutcome: vi.fn(), +})); +vi.mock('@objectstack/spec/data', () => ({ + SeedLoaderRequestSchema: { parse: (x: any) => x }, +})); + +import { MarketplaceInstallLocalPlugin } from './marketplace-install-local-plugin.js'; +import { LocalManifestSource } 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() }, + objectql: { syncSchemas: async () => undefined, find: vi.fn(async () => [{ id: 'x' }]) }, + metadata: {}, + }; + 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')?.(); }, + }; +} + +/** A Hono-ish context, enough for the GET handler. */ +function makeC() { + 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, + header: () => undefined, + }, + json, + }; +} + +const GOOD_MANIFEST = { + id: 'app.test.crm', + version: '1.0.0', + objects: [{ name: 'crm_x', fields: { name: { type: 'text' } } }], +}; + +let dir: string; +beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'mil-corrupt-')); }); +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + vi.restoreAllMocks(); +}); + +function writeGoodEntry(manifestId = GOOD_MANIFEST.id) { + new LocalManifestSource(dir).write({ + packageId: 'pkg_1', + versionId: 'pkgv_1', + manifestId, + version: GOOD_MANIFEST.version, + manifest: { ...GOOD_MANIFEST, id: manifestId }, + installedAt: '2026-01-01T00:00:00.000Z', + installedBy: 'admin', + }); +} + +/** The issue's repro verbatim — truncated mid-object. */ +function writeBrokenEntry(file = 'broken.json') { + writeFileSync(join(dir, file), '{"manifestId":"broken","manifest":{"objects":[{"name":"acct"', 'utf8'); +} + +/** Boot a fresh plugin so `kernel:ready` runs rehydrate + mounts the routes. */ +async function boot() { + const rawApp = makeRawApp(); + const { ctx, fire } = makeCtx(rawApp); + const plugin = new MarketplaceInstallLocalPlugin({ controlPlaneUrl: 'off', storageDir: dir }); + await plugin.start(ctx as any); + await fire(); + return { rawApp, ctx }; +} + +const warnings = (ctx: any): string[] => ctx.logger.warn.mock.calls.map((c: any[]) => String(c[0])); + +describe('rehydrate() — an installed app that disappears now says so', () => { + it('warns per corrupt entry, naming the file and quoting the cause', async () => { + writeGoodEntry(); + writeBrokenEntry(); + + const { ctx } = await boot(); + const said = warnings(ctx); + + // ① THE assertion of this issue. Before #5413 no line existed under any + // log level: the thrown object was discarded where it was caught. + const row = said.find((s) => s.includes('broken.json')); + expect(row).toBeDefined(); + // ② The consequence, stated — an app is missing from this runtime. + expect(row).toContain('NOT registered in this runtime'); + // ③ The cause, quoted from the thrower rather than paraphrased. + expect(row).toMatch(/JSON/i); + // ④ The fix: the absolute path of the file to repair or delete. + expect(row).toContain(join(dir, 'broken.json')); + }); + + it('still registers the entries that ARE readable', async () => { + // Skipping stays deliberate: one bad file must not cost the good apps. + writeGoodEntry(); + writeBrokenEntry(); + + const { ctx } = await boot(); + + // Counted over LEDGER manifests only: this plugin also registers its + // own "Installed Apps" Setup nav bundle at `kernel:ready`, which is not + // a rehydration and would make a bare call count read 2. + const rehydrated = ctx.getService('manifest').register.mock.calls + .map((c: any[]) => c[0]?.id) + .filter((id: unknown) => id === GOOD_MANIFEST.id); + expect(rehydrated).toEqual([GOOD_MANIFEST.id]); + }); + + it('warns even when EVERY entry is corrupt — the worst case, not the quiet one', async () => { + // `rehydrate()` returns early on an empty entry list. Reporting after + // that return would leave the total-loss case as the single path that + // still said nothing at all. + writeBrokenEntry('a.json'); + writeBrokenEntry('b.json'); + + const { ctx } = await boot(); + const said = warnings(ctx); + + expect(said.some((s) => s.includes('a.json'))).toBe(true); + expect(said.some((s) => s.includes('b.json'))).toBe(true); + }); + + it('says nothing about the ledger when every entry is intact', async () => { + // The row is a FINDING, not a status line — a clean ledger must leave + // the boot log as it was. + writeGoodEntry(); + + const { ctx } = await boot(); + + expect(warnings(ctx).some((s) => s.includes('unreadable ledger entry'))).toBe(false); + }); + + it('reports an unreadable file, not only an unparseable one', async () => { + // A directory named `*.json`: `readFileSync` throws EISDIR. Same silent + // drop before #5413, different repair — which is why the cause travels. + mkdirSync(join(dir, 'notafile.json')); + + const { ctx } = await boot(); + + expect(warnings(ctx).some((s) => s.includes('notafile.json') && s.includes('EISDIR'))).toBe(true); + }); +}); + +describe('handleList() — the console list that looked complete', () => { + it('reports the skipped entry in the log', async () => { + writeGoodEntry(); + writeBrokenEntry(); + + const { rawApp, ctx } = await boot(); + ctx.logger.warn.mockClear(); // isolate the GET from rehydrate's own warns + const c = makeC(); + await rawApp.routes.get('GET /api/v1/marketplace/install-local')!(c); + + const row = warnings(ctx).find((s) => s.includes('broken.json')); + expect(row).toBeDefined(); + // Named for what the CONSUMER lost, not for what the producer did. + expect(row).toContain('MISSING from the installed-apps list'); + }); + + 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 + // comment); #5413 only removes the case where nobody could have known. + writeGoodEntry(); + writeBrokenEntry(); + + const { rawApp } = await boot(); + const c = makeC(); + await rawApp.routes.get('GET /api/v1/marketplace/install-local')!(c); + + const [payload, status] = c.json.mock.calls[0]!; + expect(status).toBe(200); + expect(payload.success).toBe(true); + expect(payload.data.items).toHaveLength(1); + expect(payload.data.items[0].manifestId).toBe(GOOD_MANIFEST.id); + expect(payload.data.total).toBe(1); + // No new key smuggled into the response under cover of the fix. + expect(Object.keys(payload.data).sort()).toEqual(['items', 'storageDir', 'total']); + }); +}); diff --git a/packages/cloud-connection/src/marketplace-install-local-plugin.ts b/packages/cloud-connection/src/marketplace-install-local-plugin.ts index f88b1bcf72..87caba8ea7 100644 --- a/packages/cloud-connection/src/marketplace-install-local-plugin.ts +++ b/packages/cloud-connection/src/marketplace-install-local-plugin.ts @@ -56,7 +56,14 @@ import { import { postureEnforcesWall, type TenancyPosture } from '@objectstack/spec/security'; import { resolveCloudUrl } from './cloud-url.js'; import { resolveMarketplacePublicBaseUrl } from './marketplace-public-url.js'; -import { LocalManifestSource, type InstalledManifestEntry } from './local-manifest-source.js'; +import { join } from 'node:path'; + +import { + LocalManifestSource, + type InstalledManifestEntry, + type InstalledManifestListing, + type SkippedManifestEntry, +} from './local-manifest-source.js'; import { ConnectionCredentialStore } from './connection-credential-store.js'; import { MARKETPLACE_INSTALLED_UI_BUNDLE } from './marketplace-ui.js'; import type { IHttpServer } from '@objectstack/spec/contracts'; @@ -183,7 +190,7 @@ export class MarketplaceInstallLocalPlugin implements Plugin { const rawApp = httpServer.getRawApp(); const postHandler = async (c: any) => this.handleInstall(c, ctx); - const getHandler = async (c: any) => this.handleList(c); + const getHandler = async (c: any) => this.handleList(c, ctx); const deleteHandler = async (c: any) => this.handleUninstall(c, ctx); const reseedHandler = async (c: any) => this.handleReseed(c, ctx); @@ -209,7 +216,15 @@ export class MarketplaceInstallLocalPlugin implements Plugin { * a marketplace package). */ private rehydrate = async (ctx: PluginContext): Promise => { - const entries = this.readAll(); + const { entries, skipped } = this.readAll(); + + // #5413 — BEFORE the early return, not after. A ledger whose entries + // are ALL corrupt is the worst case of this bug, not an exempt one: + // every installed app silently missing from the runtime, and an + // `entries.length === 0` return above this loop would be the one path + // that still said nothing at all. + this.warnSkippedLedgerEntries(ctx, skipped, 'that installed app is NOT registered in this runtime'); + if (entries.length === 0) return; let manifestService: { register(m: any): void | Promise } | null = null; @@ -698,8 +713,19 @@ export class MarketplaceInstallLocalPlugin implements Plugin { }, 200); }; - private handleList = async (c: any): Promise => { - const entries = this.readAll(); + /** + * `GET /…/installed` — the console's "Installed Apps" list. + * + * #5413: the WIRE SHAPE is deliberately unchanged. A corrupt ledger entry + * is reported to the operator's log, not to the HTTP client — putting it in + * the response body is a separate decision about this endpoint's schema and + * is explicitly NOT made here. What the fix removes is the case where a + * short list was served with `success: true` and nobody, anywhere, could + * have known. + */ + private handleList = async (c: any, ctx: PluginContext): Promise => { + const { entries, skipped } = this.readAll(); + this.warnSkippedLedgerEntries(ctx, skipped, 'it is MISSING from the installed-apps list served to the console'); return c.json({ success: true, data: { @@ -1276,5 +1302,39 @@ export class MarketplaceInstallLocalPlugin implements Plugin { return null; }; - private readAll = (): InstalledEntry[] => this.ledger.list(); + /** + * Read the whole ledger — the entries it parsed AND the files it could not + * (#5413). + * + * Returns the listing rather than unwrapping `.entries` here on purpose: an + * unwrap at this seam would put the silence back one layer up, where it is + * even harder to find. Both call sites below report `skipped` before they + * do anything with `entries`. + */ + private readAll = (): InstalledManifestListing => this.ledger.list(); + + /** + * One line per ledger file that could not be read (#5413). + * + * `warn`, deliberately, and the same tier as this plugin's existing + * "no `manifest` service — rehydrate skipped": this is a FUNCTIONAL + * degradation, not a durability one. Nothing that claimed to persist failed + * to land — the ledger file is still on disk, exactly as written — the + * runtime is simply, visibly smaller than the ledger says it should be, and + * the next person to look for the missing app finds out. (See AGENTS.md, + * "Degradation log levels".) + * + * The file name and the thrower's own words are both in the line, because + * they are the two things that turn "an app is missing" into a fix: + * `.objectstack/installed-packages/` is the thing to repair or delete. + */ + 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}`, + ); + } + }; }