From e172ab465cb1b370c80f5d2596b764399e8c2fdb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 10:15:05 +0000 Subject: [PATCH] =?UTF-8?q?fix(cli):=20`os=20doctor`=20=E4=B8=8D=E5=86=8D?= =?UTF-8?q?=E5=9C=A8=20ledger=20=E8=AF=BB=E4=B8=8D=E5=88=B0=E6=97=B6?= =?UTF-8?q?=E6=89=93=E5=87=BA=20`=E2=9C=93=20Unique=20scope`=20(#5412)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readInstalledPackageEntries()` 用一个不带绑定的 `catch` 罩住了两件性质完全 不同的事,并对两者都返回空数组: 1. `@objectstack/cloud-connection` 解析不到 —— 可选包没装。这一条静默是 对的,保持不变:`os doctor` 必须能在没有该包的 checkout 里跑完。 2. ledger 目录**存在**(`fs.existsSync` 已经通过)但读取抛错。 第 2 种被当成第 1 种,于是它以「没有已装包」的身份抵达 ADR-0120 D5e 的 unique-scope 建议,建议无话可说,报告打出 `✓ Unique scope`。一个装了包、 ledger 却读不到的 isolated 环境拿到的是一张干净体检单 —— false PASS 比漏报 更糟,它让操作者停止查看。 两者现已拆开:`import()` 失败保留自己的静默 catch;读取失败带着 cause 回到 调用方,成功行被扣住(它是对建议**两半**的断言,只有两半都跑过才能打), 改为经 `renderHealthCheckResult()` 输出一条 warning 档 `HealthCheckResult`, `--verbose` 展开随之免费获得。已跑过的那一半的 findings 仍照常上报。 不在本单范围:ledger 内**单个条目**损坏 —— `LocalManifestSource.list()` 在 自己的 per-file catch 里跳过它,调用成功且返回短列表,消费者无从分辨。 已另立 #5413,并由本 PR 的 SCOPE BOUNDARY 测试钉住。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- ...r-installed-package-ledger-read-failure.md | 57 +++ .../doctor-ledger-read-failure.test.ts | 424 ++++++++++++++++++ packages/cli/src/commands/doctor.ts | 180 +++++++- 3 files changed, 639 insertions(+), 22 deletions(-) create mode 100644 .changeset/doctor-installed-package-ledger-read-failure.md create mode 100644 packages/cli/src/commands/doctor-ledger-read-failure.test.ts diff --git a/.changeset/doctor-installed-package-ledger-read-failure.md b/.changeset/doctor-installed-package-ledger-read-failure.md new file mode 100644 index 0000000000..9c670181ce --- /dev/null +++ b/.changeset/doctor-installed-package-ledger-read-failure.md @@ -0,0 +1,57 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os doctor` no longer prints `✓ Unique scope` when it could not read the installed-package ledger (#5412) + +`readInstalledPackageEntries()` wrapped two unrelated facts in one un-bound +`catch` and returned an empty entry list for both: + +- **`@objectstack/cloud-connection` does not resolve** — the optional package is + not installed. Silence is correct here and stays: `os doctor` must run to + completion in a checkout that never had it. +- **The ledger directory exists and could not be read** — `fs.existsSync()` had + already confirmed the directory is there, and producing its entry list threw + (the path is occupied by a file, the filesystem refused, the read failed). + +The second was handled as the first. It reached the ADR-0120 D5e unique-scope +advisory as "no installed packages", the advisory found nothing to report, and +the run printed: + +``` + ✓ Unique scope No unconfirmed installation-wide uniques for this 'isolated' environment +``` + +So an environment **with** installed packages, whose ledger doctor could not +read, got a clean bill of health for the one constraint the `isolated` posture +makes dangerous. That is worse than a missing check: a false PASS is what stops +an operator looking further. + +The two are now separate. The `import()` failure keeps its own silent `catch`; +a ledger read failure comes back as a cause the caller can report, and the +success line is withheld — it is a claim about **both** halves of the advisory +(this project's metadata, and the manifests of installed packages), so it may +only be printed when both halves ran. In its place doctor prints an ordinary +`HealthCheckResult` through the same renderer every other check uses: + +``` + ⚠ Unique scope Could not read the installed-package ledger (installed packages NOT + checked for installation-wide uniques) — ENOTDIR: not a directory, … +``` + +**Warning, not error**, and the exit code is unchanged: the environment still +runs; what broke is doctor's ability to see part of it. The cause is quoted from +the thrower rather than paraphrased, and `--verbose` expands the untruncated +original plus which half of the check did not run — both free from reusing +`renderHealthCheckResult()`. + +Findings from the half that **did** run are still reported: a ledger failure +does not swallow the uniques this project's own metadata declares. + +**Not covered by this change**: a single *corrupt entry* inside an otherwise +readable ledger. `LocalManifestSource.list()` skips unparseable files in its own +per-file `catch`, so a truncated manifest is dropped inside the producer and the +call succeeds with a short list that no consumer can distinguish from a complete +one. Fixing that is a change to `@objectstack/cloud-connection`'s own contract +and is tracked separately; the boundary is pinned by a test so it is not +mistaken for covered. diff --git a/packages/cli/src/commands/doctor-ledger-read-failure.test.ts b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts new file mode 100644 index 0000000000..8a26b975b0 --- /dev/null +++ b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts @@ -0,0 +1,424 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os doctor` does not print `✓ Unique scope` when it could not read the + * installed-package ledger (#5412). + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `readInstalledPackageEntries()` wrapped two unrelated facts in one un-bound + * `catch` and returned `[]` for both: + * + * try { + * const mod = await import('@objectstack/cloud-connection'); // (1) + * const dir = path.join(cwd, mod.DEFAULT_INSTALLED_PACKAGES_DIR ?? …); + * if (!fs.existsSync(dir)) return []; + * return new mod.LocalManifestSource(dir).list(); // (2) + * } catch { return []; } + * + * (1) is the optional package being absent — silence is correct, `os doctor` + * must run in a checkout that never had it. (2) is a ledger that EXISTS + * (`existsSync` already said so) and could not be read. Treated as (1), it + * reached the ADR-0120 D5e advisory as "no installed packages", the advisory + * found nothing to report, and the run printed: + * + * ✓ Unique scope No unconfirmed installation-wide uniques for + * this 'isolated' environment + * + * A false PASS, on the one constraint the `isolated` posture makes dangerous. + * + * ── Scope boundary this file also pins ─────────────────────────────────── + * + * 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. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import Doctor, { installedPackageLedgerFailureCheck } from './doctor.js'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +/** `packages/cli` — the oclif root the real command is loaded against below. */ +const CLI_ROOT = path.resolve(HERE, '..', '..'); + +/** + * The escape is written as `\x1b`, never as the byte itself: one raw control + * character makes `grep` treat the whole file as binary, and a test file no + * `git grep` can find stops being maintained (#4890 / #5157). + */ +const SGR = /\x1b\[[0-9;]*m/g; +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. */ +const LEDGER_HEADLINE = 'Could not read the installed-package ledger'; + +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'"), { + code: 'ENOTDIR', + }); + const check = installedPackageLedgerFailureCheck(err); + + // Before #5412 this text existed nowhere in any doctor output under any + // flag — the error object was discarded at the point it was caught. + expect(check.message).toContain('ENOTDIR'); + expect(check.fix).toContain("scandir '/p/.objectstack'"); + }); + + it('takes the `Unique scope` name column, so the row is present rather than missing', () => { + const check = installedPackageLedgerFailureCheck(new Error('boom')); + + // Load-bearing, not cosmetic. An operator scans the report by its name + // column; a separately-named row would leave `Unique scope` simply absent, + // which is the same silence this issue is about wearing a different hat. + expect(check.name).toBe('Unique scope'); + }); + + it('stays a warning — the environment runs, doctor’s sight of it is what broke', () => { + expect(installedPackageLedgerFailureCheck(new Error('boom')).status).toBe('warning'); + }); + + it('says WHICH half did not run, rather than that "something" failed', () => { + const check = installedPackageLedgerFailureCheck(new Error('boom')); + const fix = check.fix ?? ''; + + // The harm the issue names is a reader treating a partial check as a whole + // one. The row has to state its own incompleteness in both channels. + expect(check.message).toContain('installed packages NOT checked'); + expect(fix).toContain('two halves and only one of them ran'); + expect(fix).toContain('.objectstack/installed-packages/'); + }); + + it('folds a multi-line cause onto the row and keeps it whole in the detail', () => { + const check = installedPackageLedgerFailureCheck(new Error('line one\nline two: the reason')); + + expect(check.message).toContain('line two: the reason'); + // One row is one line. + expect(check.message).not.toContain('\n'); + expect(check.fix).toContain('line two: the reason'); + }); + + it('never trails off into nothing for an Error with no message', () => { + const check = installedPackageLedgerFailureCheck(new TypeError()); + + expect(check.message.endsWith('— ')).toBe(false); + expect(check.message).toContain('TypeError'); + }); + + it('reports a thrown non-Error rather than swallowing it', () => { + expect(installedPackageLedgerFailureCheck('boom').message).toContain('boom'); + expect(installedPackageLedgerFailureCheck(42).message).toContain('42'); + }); +}); + +describe('os doctor, end to end, against an unreadable installed-package ledger', () => { + /** + * `node_modules/` exists in the temp cwd on purpose — without it doctor's + * `Dependencies` check is itself an `error` and exits 1 on its own, which + * would make an assertion pass for a reason having nothing to do with this + * change (the trap PR #5390 wrote down, inherited via #5402 / #5410). + */ + let tmp: string; + let cwdSpy: ReturnType; + const savedPosture = process.env.OS_TENANCY_POSTURE; + + beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5412-e2e-')); + fs.mkdirSync(path.join(tmp, 'node_modules')); + // D5e's advisory only runs under `isolated` — the posture under which + // `'global'` stops being unambiguous. Every case below needs it. + process.env.OS_TENANCY_POSTURE = 'isolated'; + cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + }); + + afterEach(() => { + cwdSpy.mockRestore(); + if (savedPosture === undefined) delete process.env.OS_TENANCY_POSTURE; + else process.env.OS_TENANCY_POSTURE = savedPosture; + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + const LEDGER_REL = '.objectstack/installed-packages'; + const ledgerPath = () => path.join(tmp, LEDGER_REL); + + const writeConfig = () => + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + [ + 'export default {', + " manifest: { name: 'os5412', label: 'Ledger Read', version: '1.0.0' },", + " objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],", + '};', + '', + ].join('\n'), + ); + + /** A ledger entry the D5e advisory would have something to say about. */ + const globalUniqueEntry = (manifestId: string) => ({ + manifestId, + manifest: { + objects: [ + { + name: 'invoice', + label: 'Invoice', + fields: [{ name: 'code', type: 'text', label: 'Code', unique: 'global' }], + }, + ], + }, + }); + + async function runDoctor(argv: string[] = []): Promise<{ out: string; exitCode: number | undefined }> { + const logs: string[] = []; + const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')); + }); + let exitCode: number | undefined; + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`__PROCESS_EXIT__:${code}`); + }) as never); + + try { + await Doctor.run(argv, { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + } + return { out: plain(logs.join('\n')), exitCode }; + } + + it('withholds the clean bill of health when the ledger cannot be read', async () => { + writeConfig(); + // The ledger PATH exists but is not a directory — `fs.existsSync(dir)` + // passes, `readdirSync` throws ENOTDIR. This is the shape that used to + // land in the shared `catch` and come back as "nothing installed". + fs.mkdirSync(path.dirname(ledgerPath()), { recursive: true }); + fs.writeFileSync(ledgerPath(), 'this is a file, not the ledger directory\n'); + + const run = await runDoctor(); + + // ① THE assertion of this issue: the false PASS is gone. + expect(run.out).not.toContain(CLEAN_BILL); + // ② …replaced by a row that says what was not read, with the cause. + expect(run.out).toContain(LEDGER_HEADLINE); + expect(run.out).toContain('ENOTDIR'); + // Rendered through the ONE renderer, so it carries a name column. + expect(run.out).toContain('Unique scope'); + // Gauge: warning, the report finishes, exit stays 0. + expect(run.out).toContain('Environment is functional but has some warnings'); + expect(run.exitCode).toBeUndefined(); + }, 60_000); + + it('expands the detail under --verbose, and only under --verbose', async () => { + writeConfig(); + fs.mkdirSync(path.dirname(ledgerPath()), { recursive: true }); + fs.writeFileSync(ledgerPath(), 'not a directory\n'); + + const plainRun = await runDoctor(); + const verboseRun = await runDoctor(['--verbose']); + + // Warning-tier `fix` follows the same rule every other warning follows — + // this is what reusing `renderHealthCheckResult()` buys for free. + expect(plainRun.out).not.toContain('cause:'); + expect(verboseRun.out).toContain('cause:'); + expect(verboseRun.out).toContain('two halves and only one of them ran'); + expect(verboseRun.out).not.toContain(CLEAN_BILL); + expect(verboseRun.exitCode).toBeUndefined(); + }, 60_000); + + it('still reports this project’s own findings when only the ledger half failed', async () => { + // The two halves are independent. A ledger failure must not swallow the + // findings the config half already produced — that would trade one silent + // omission for another. + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + [ + 'export default {', + " manifest: { name: 'os5412u', label: 'Ledger Read', version: '1.0.0' },", + ' objects: [{', + " name: 'account', label: 'Account',", + " fields: [{ name: 'taxId', type: 'text', label: 'Tax ID', unique: 'global' }],", + ' }],', + '};', + '', + ].join('\n'), + ); + fs.mkdirSync(path.dirname(ledgerPath()), { recursive: true }); + fs.writeFileSync(ledgerPath(), 'not a directory\n'); + + const run = await runDoctor(); + + expect(run.out).toContain('account.taxId'); + expect(run.out).toContain(LEDGER_HEADLINE); + expect(run.out).not.toContain(CLEAN_BILL); + }, 60_000); + + it('an intact ledger with nothing to report still prints the clean bill — no regression', async () => { + // ④ The normal D5e path. The new row is a FINDING, not a status line: a + // readable ledger must leave the report byte-for-byte as it was. + writeConfig(); + fs.mkdirSync(ledgerPath(), { recursive: true }); + fs.writeFileSync( + path.join(ledgerPath(), 'clean.json'), + JSON.stringify({ manifestId: 'clean', manifest: { objects: [] } }), + ); + + const run = await runDoctor(['--verbose']); + + expect(run.out).toContain(CLEAN_BILL); + expect(run.out).not.toContain(LEDGER_HEADLINE); + }, 60_000); + + it('an intact ledger WITH a global unique is still reported the way it always was', async () => { + writeConfig(); + fs.mkdirSync(ledgerPath(), { recursive: true }); + fs.writeFileSync( + path.join(ledgerPath(), 'billing.json'), + JSON.stringify(globalUniqueEntry('billing')), + ); + + const run = await runDoctor(); + + expect(run.out).toContain('invoice.code'); + expect(run.out).toContain("installed package 'billing'"); + expect(run.out).not.toContain(CLEAN_BILL); + expect(run.out).not.toContain(LEDGER_HEADLINE); + }, 60_000); + + it('says nothing about the ledger when there is no ledger at all', async () => { + // ③, first half. A runtime that never installed anything has no directory, + // and that is genuinely not a finding. The fix must not turn "never + // installed" into a warning — that would be the opposite over-correction. + writeConfig(); + expect(fs.existsSync(ledgerPath())).toBe(false); + + const run = await runDoctor(['--verbose']); + + expect(run.out).toContain(CLEAN_BILL); + expect(run.out).not.toContain(LEDGER_HEADLINE); + 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. + writeConfig(); + fs.mkdirSync(ledgerPath(), { recursive: true }); + fs.writeFileSync(path.join(ledgerPath(), 'good.json'), JSON.stringify(globalUniqueEntry('good'))); + // Truncated mid-object — the issue's repro verbatim. + fs.writeFileSync( + path.join(ledgerPath(), 'broken.json'), + '{"manifestId":"broken","manifest":{"objects":[{"name":"acct"', + ); + + const run = await runDoctor(); + + // The readable entry is reported… + 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'); + expect(run.out).not.toContain(LEDGER_HEADLINE); + }, 60_000); +}); + +describe('the optional package being absent stays completely silent (#5412 does not regress it)', () => { + /** + * ③, second half — the one branch that is SUPPOSED to swallow. + * + * `@objectstack/cloud-connection` resolves inside this monorepo, so absence + * is simulated by making its module evaluation throw the way an unresolvable + * specifier does. `vi.doMock` (not the hoisted `vi.mock`) plus a fresh + * module registry is what keeps this scoped to this one test — every case + * above needs the real module. + */ + afterEach(() => { + vi.doUnmock('@objectstack/cloud-connection'); + vi.resetModules(); + }); + + it('prints no ledger row when the optional package cannot be loaded', async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5412-nopkg-')); + fs.mkdirSync(path.join(tmp, 'node_modules')); + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + [ + 'export default {', + " manifest: { name: 'os5412np', label: 'No Package', version: '1.0.0' },", + " objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],", + '};', + '', + ].join('\n'), + ); + // A ledger directory EXISTS — so if the `import()` failure were still + // sharing a `catch` with the read failure, this is where the two would be + // confused in the other direction and produce a spurious warning. + fs.mkdirSync(path.join(tmp, '.objectstack/installed-packages'), { recursive: true }); + + const savedPosture = process.env.OS_TENANCY_POSTURE; + process.env.OS_TENANCY_POSTURE = 'isolated'; + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + + vi.resetModules(); + vi.doMock('@objectstack/cloud-connection', () => { + throw new Error("Cannot find package '@objectstack/cloud-connection'"); + }); + const { default: FreshDoctor } = await import('./doctor.js'); + + const logs: string[] = []; + const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')); + }); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new Error(`__PROCESS_EXIT__:${code}`); + }) as never); + + try { + await FreshDoctor.run([], { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + cwdSpy.mockRestore(); + if (savedPosture === undefined) delete process.env.OS_TENANCY_POSTURE; + else process.env.OS_TENANCY_POSTURE = savedPosture; + fs.rmSync(tmp, { recursive: true, force: true }); + } + + const out = plain(logs.join('\n')); + // Silence about the ledger, and the advisory's own half still reports. + expect(out).not.toContain(LEDGER_HEADLINE); + expect(out).not.toContain('cloud-connection'); + expect(out).toContain(CLEAN_BILL); + }, 60_000); +}); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index bb82817df4..8c36f0b17e 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -797,26 +797,92 @@ interface UniqueScopeAdvisory { finding: GlobalUniqueFinding; } -/** Read the installed-package ledger without going through HTTP. Best-effort: - * a runtime that never installed anything simply has no directory. */ -async function readInstalledPackageEntries(cwd: string): Promise { +/** + * The outcome of reading the installed-package ledger (#5412). + * + * An empty `entries` is ambiguous on its own — it is what "nothing installed" + * and what "the ledger could not be read" both produce — so the second case + * carries a `failure` alongside it. The caller distinguishes them; before + * #5412 nobody could, because one `catch` returned `[]` for both. + */ +interface InstalledPackageLedgerReading { + entries: any[]; + /** Present ONLY when the ledger EXISTS and could not be read. */ + failure?: { cause: unknown }; +} + +/** + * Read the installed-package ledger without going through HTTP. + * + * Two failures live here, and they are NOT the same fact (#5412): + * + * 1. `@objectstack/cloud-connection` does not resolve — the optional package + * is not installed. Silence is correct and deliberate: `os doctor` must + * run to completion in a checkout that never had it. + * 2. The ledger directory EXISTS (`fs.existsSync` already said so) and + * reading it threw — the path is not a directory, the filesystem refused, + * the entry list could not be produced. Something IS there and doctor did + * not read it. + * + * One un-bound `catch` covered both and returned `[]` for both, so case 2 + * reached the D5e advisory as "no installed packages" and the report printed + * `✓ Unique scope`. A false PASS is worse than a missing check, because it + * 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. + */ +async function readInstalledPackageEntries(cwd: string): Promise { + let mod: any; try { // Dynamic, like serve.ts's cloud-connection load: `os doctor` must still - // run in a checkout where the optional package is not resolvable. - const mod: any = await import('@objectstack/cloud-connection'); - const dir = path.join(cwd, mod.DEFAULT_INSTALLED_PACKAGES_DIR ?? '.objectstack/installed-packages'); - if (!fs.existsSync(dir)) return []; - return new mod.LocalManifestSource(dir).list(); + // run in a checkout where the optional package is not resolvable. THIS + // catch, and only this one, is allowed to be silent. + mod = await import('@objectstack/cloud-connection'); } catch { - return []; + return { entries: [] }; + } + + const dir = path.join(cwd, mod.DEFAULT_INSTALLED_PACKAGES_DIR ?? '.objectstack/installed-packages'); + try { + // No directory = nothing was ever installed. Genuinely not a finding. + if (!fs.existsSync(dir)) return { entries: [] }; + return { entries: new mod.LocalManifestSource(dir).list() }; + } catch (err) { + return { entries: [], failure: { cause: err } }; } } +/** What `findUnscopedGlobalUniques()` hands back — findings AND completeness. */ +interface UniqueScopeReading { + advisories: UniqueScopeAdvisory[]; + /** + * Present when the ledger half did not run. The advisory is then INCOMPLETE, + * not clean, and the caller must not print its success line (#5412). + */ + ledgerFailure?: { cause: unknown }; +} + /** * Collect every unanswered installation-wide unique this environment would run * under `isolated`. Returns an empty list under every other posture: there * `'global'` is the correct, unambiguous meaning (`single` = one customer; * `group` = the installation IS the customer company). + * + * The advisory has TWO halves — this project's own metadata, and the installed + * packages in the ledger — and #5412 is about the second half being able to + * fail alone. When it does, the findings collected from the first half are + * still real and are still returned; what the caller must not do is read the + * resulting emptiness as a clean bill of health. */ async function findUnscopedGlobalUniques( cwd: string, @@ -826,14 +892,16 @@ async function findUnscopedGlobalUniques( // `resolveTenancyPosture()` here is a throw the wrong `catch` reports as // "Could not load config for analysis". posture: TenancyPosture, -): Promise { - if (!postureGatesGlobalUniques(posture)) return []; +): Promise { + if (!postureGatesGlobalUniques(posture)) return { advisories: [] }; const out: UniqueScopeAdvisory[] = []; for (const finding of collectGlobalUniques(config?.objects)) { out.push({ source: 'this project’s metadata', finding }); } - for (const entry of await readInstalledPackageEntries(cwd)) { + + const ledger = await readInstalledPackageEntries(cwd); + for (const entry of ledger.entries) { const findings = collectGlobalUniques(entry?.manifest?.objects); // Subtract what the install ceremony already answered for — an attested // install must not be re-reported, or the advisory becomes the recurring @@ -842,7 +910,7 @@ async function findUnscopedGlobalUniques( out.push({ source: `installed package '${entry?.manifestId ?? entry?.packageId}'`, finding }); } } - return out; + return { advisories: out, ...(ledger.failure ? { ledgerFailure: ledger.failure } : {}) }; } // ─── Filesystem Checks ────────────────────────────────────────────── @@ -1015,22 +1083,27 @@ function describeThrown(err: unknown): string { * The full text always survives in `fix`; this bound only keeps one aligned * line aligned. */ -const CONFIG_LOAD_HEADLINE_MAX = 160; +const REPORT_ROW_HEADLINE_MAX = 160; /** * Fold a possibly multi-line cause onto the one line a report row is. * + * Named for the ROW rather than for the config load since #5412 — the + * installed-package ledger's failure quotes its cause through the same fold, + * and a helper called `configLoadHeadline` reading a filesystem error would be + * the first line of drift between the two. + * * Whitespace-collapsing, not rewriting: esbuild's failures open with * `Build failed with 1 error:` and put the file, line and reason on the NEXT * line, so a naive "first line" would quote the least informative sentence it * has. Every word is upstream's, in upstream's order; only the line breaks and * an overlong tail are ours. */ -function configLoadHeadline(cause: string): string { +function reportRowHeadline(cause: string): string { const collapsed = cause.replace(/\s+/g, ' ').trim(); - return collapsed.length <= CONFIG_LOAD_HEADLINE_MAX + return collapsed.length <= REPORT_ROW_HEADLINE_MAX ? collapsed - : `${collapsed.slice(0, CONFIG_LOAD_HEADLINE_MAX - 1)}…`; + : `${collapsed.slice(0, REPORT_ROW_HEADLINE_MAX - 1)}…`; } /** Keep a multi-line quote under the report's ` → ` gutter. */ @@ -1070,7 +1143,7 @@ export function configLoadFailureCheck(err: unknown): HealthCheckResult { return { name: 'Config load', status: 'warning', - message: `Could not load config for analysis (config checks skipped) — ${configLoadHeadline(cause)}`, + message: `Could not load config for analysis (config checks skipped) — ${reportRowHeadline(cause)}`, fix: '`os serve` loads this same file the same way — bundle-require, under the `.env*`\n' + ' cascade named above (#5397) — and prints this error in full, so a config that\n' @@ -1081,6 +1154,56 @@ export function configLoadFailureCheck(err: unknown): HealthCheckResult { }; } +/** + * What doctor reports when the installed-package ledger cannot be read (#5412). + * + * The ADR-0120 D5e advisory is the sum of two halves — this project's declared + * metadata, and the manifests of packages installed into this runtime. The + * ledger half used to fail into the same un-bound `catch` that absorbs "the + * optional package is not installed", so a directory that existed and could + * not be read produced an empty entry list, the advisory found nothing to say, + * and the report printed: + * + * ✓ Unique scope No unconfirmed installation-wide uniques for + * this 'isolated' environment + * + * An environment WITH installed packages got a clean bill of health for the + * exact constraint the posture makes dangerous. Per this repo's "absence must + * be loud" rule the success line is now withheld and this row takes its place. + * + * Three deliberate choices, mirroring `configLoadFailureCheck`: + * + * • **Warning, not error.** The environment still runs; what is broken is + * doctor's ability to see part of it. Doctor keeps going and keeps exiting + * 0, exactly as it does for a config it cannot load. + * • **It takes the `Unique scope` name column.** Not a new label: the point + * is that the row an operator scans for is PRESENT and not a `✓`. A + * separately-named row would leave `Unique scope` simply missing, which is + * the silence this issue is about wearing a different hat. + * • **The cause is quoted, not paraphrased** (#5390 / #5403). `ENOTDIR: not + * a directory, scandir '…'` names the file that is in the way; no sentence + * doctor could invent would beat it. + */ +export function installedPackageLedgerFailureCheck(err: unknown): HealthCheckResult { + const cause = describeThrown(err); + return { + name: 'Unique scope', + status: 'warning', + message: + 'Could not read the installed-package ledger (installed packages NOT checked for ' + + `installation-wide uniques) — ${reportRowHeadline(cause)}`, + fix: + 'This check has two halves and only one of them ran. Uniques declared by THIS\n' + + ' project’s metadata were checked and are reported above; uniques declared by\n' + + ' INSTALLED PACKAGES were not looked at, so an installed app carrying an\n' + + ' installation-wide `unique` would not have appeared.\n' + + ' The ledger is the `.objectstack/installed-packages/` directory under the\n' + + ' project root; it exists here, which is why this is reported rather than\n' + + ' treated as "nothing was ever installed".\n' + + ` cause: ${indentUnderGutter(cause)}`, + }; +} + // ─── Command ──────────────────────────────────────────────────────── export default class Doctor extends Command { @@ -1379,14 +1502,27 @@ 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 scopeFindings = await findUnscopedGlobalUniques(cwd, config, postureReading.posture); - if (scopeFindings.length > 0) { + const { advisories, ledgerFailure } = await findUnscopedGlobalUniques( + cwd, + config, + postureReading.posture, + ); + if (advisories.length > 0) { hasWarnings = true; - for (const { source, finding } of scopeFindings) { + for (const { source, finding } of advisories) { printWarning(`Unique scope ${describeGlobalUniqueFinding(finding)} (${source})`); } console.log(chalk.dim(` → ${GLOBAL_UNIQUE_ISOLATED_PRESCRIPTION}`)); - } else { + } + // #5412 — the success line is a claim about BOTH halves of the + // advisory, so it may only be printed when both halves ran. A ledger + // 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. + if (ledgerFailure) { + hasWarnings = true; + renderHealthCheckResult(installedPackageLedgerFailureCheck(ledgerFailure.cause), flags.verbose); + } else if (advisories.length === 0) { printSuccess("Unique scope No unconfirmed installation-wide uniques for this 'isolated' environment"); } }