From e49e03f772ecf3734caafe616ddc84de0f6296e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:35:37 +0000 Subject: [PATCH 1/2] fix(cli): `os doctor` tells a broken cloud-connection install from an absent one (#5644) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readInstalledPackageEntries()` reached the installed-package ledger through a dynamic `import('@objectstack/cloud-connection')` whose `catch` meant "the optional package is not installed". That covered two states with opposite remedies: a specifier that does not resolve (genuinely absent — silence is correct and stays), and a package that IS installed and will not load (unbuilt or pruned `dist/`, interrupted install, an artefact that throws while it evaluates). The second was answered with the first one's silence, so the ADR-0120 D5e advisory saw "no installed packages" and printed `✓ Unique scope` over a ledger nobody read — the false PASS #5412 removed at the `readdir` boundary and #5413 at the entry boundary, one boundary up. The two are separated by resolution, not by the `import()` having thrown: `isModuleNotFoundError()` first (an error that is not module-not-found came from the package itself), then `import.meta.resolve()`, which answers "is the package there" without stating its entry file. New `utils/optional-package.ts` owns the classification and the measurements behind it; doctor renders the present-but-broken state as a `Unique scope` warning row through the same renderer its two siblings use, and withholds the success line. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- .../doctor-optional-package-load-boundary.md | 59 ++++ .../doctor-ledger-read-failure.test.ts | 333 ++++++++++++++++-- packages/cli/src/commands/doctor.ts | 132 ++++++- .../cli/src/utils/optional-package.test.ts | 116 ++++++ packages/cli/src/utils/optional-package.ts | 123 +++++++ 5 files changed, 723 insertions(+), 40 deletions(-) create mode 100644 .changeset/doctor-optional-package-load-boundary.md create mode 100644 packages/cli/src/utils/optional-package.test.ts create mode 100644 packages/cli/src/utils/optional-package.ts diff --git a/.changeset/doctor-optional-package-load-boundary.md b/.changeset/doctor-optional-package-load-boundary.md new file mode 100644 index 0000000000..748a260776 --- /dev/null +++ b/.changeset/doctor-optional-package-load-boundary.md @@ -0,0 +1,59 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os doctor` no longer treats a broken `@objectstack/cloud-connection` install as "not installed" (#5644) + +`readInstalledPackageEntries()` reached the installed-package ledger through a +dynamic `import('@objectstack/cloud-connection')` whose `catch` meant "the +optional package is not installed". That is right for one of the two things it +caught: + +- **The specifier does not resolve** — the optional package really is not + there. Silence is correct and unchanged: `os doctor` must run to completion in + a checkout that never had it. +- **The package is installed and will not load** — a pruned or unbuilt `dist/`, + an interrupted install, an artefact that throws while it evaluates, a + transitive dependency missing under it. It threw too, so it was answered with + the same silence. + +The ADR-0120 D5e unique-scope advisory then saw "no installed packages", found +nothing to report, and the run printed: + +``` + ✓ Unique scope No unconfirmed installation-wide uniques for this 'isolated' environment +``` + +Measured: with the package present-but-unloadable and a ledger declaring an +installation-wide `unique`, that line was printed and the finding appeared +nowhere, `--verbose` included. It is the same false PASS #5412 removed at the +`readdir` boundary and #5413 at the entry boundary, one boundary further up — +and `os serve`, loading the same package in the same directory, has always named +the failure out loud. + +The two states are now separated by **resolution**, not by the `import()` having +thrown (`isModuleNotFoundError()` first — an error that is not a module-not-found +error came from the package itself, so it is present by definition; then +`import.meta.resolve()`, which answers "is the package there" without stating its +entry file, unlike `createRequire().resolve()`). Only the genuinely-absent half +is silent. The other prints an ordinary `HealthCheckResult` through the same +renderer every other check uses: + +``` + ⚠ Unique scope Could not load the installed-package ledger reader (installed packages + NOT checked for installation-wide uniques) — Cannot find module … +``` + +**Warning, not error**, and the exit code is unchanged, matching its two +siblings: the environment still runs; what broke is doctor's ability to see part +of it. The cause is quoted from the thrower, and `--verbose` expands it together +with the remedy — reinstall, or build the package in a monorepo checkout. + +The row is **not** conditional on `.objectstack/installed-packages/` existing. +Doctor cannot honestly say a ledger is absent when the constant naming the +ledger's location is an export of the package that would not load. + +One consequence worth stating: in a monorepo checkout where +`packages/cloud-connection` has not been built, `os doctor` under the `isolated` +posture now prints this warning instead of a clean bill. That state is exactly +what sent #5612 chasing a report face that had never regressed. 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 d65513f97d..83ad7cc49c 100644 --- a/packages/cli/src/commands/doctor-ledger-read-failure.test.ts +++ b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts @@ -44,6 +44,28 @@ * 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). + * + * ── The third half, one boundary UP (#5644) ────────────────────────────── + * + * The `import('@objectstack/cloud-connection')` that reaches all of the above + * had its own un-bound `catch`, and it merged the same two kinds of thing one + * level higher: a specifier that does not resolve (the optional package is not + * installed — silence is correct and stays) and a package that IS installed and + * will not load (unbuilt or pruned `dist/`, interrupted install, an artefact + * that throws). The second was answered with the first one's silence, so a + * ledger declaring an installation-wide `unique` produced `✓ Unique scope` and + * the finding appeared nowhere — measured, under `--verbose` included. + * + * The two are now separated by resolution rather than by the `import()` having + * thrown (`utils/optional-package.ts`, pinned against the real runtime in + * `utils/optional-package.test.ts`), and only the absent half is silent. + * + * That changes what the last describe of this file can simulate. Absence used + * to be simulated by making the module's evaluation throw; under the new + * contract that is precisely the OTHER state, so the case now pins the report + * it produces, and the absent contract is pinned separately through the loader + * seam. Both facts are still here — one of them changed how it is spelled, + * because the fact it used to spell was the defect. */ import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest'; @@ -52,7 +74,10 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import Doctor, { installedPackageLedgerFailureCheck } from './doctor.js'; +import Doctor, { + installedPackageLedgerFailureCheck, + installedPackageLedgerReaderFailureCheck, +} from './doctor.js'; const HERE = path.dirname(fileURLToPath(import.meta.url)); /** `packages/cli` — the oclif root the real command is loaded against below. */ @@ -75,6 +100,15 @@ 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'; +/** + * The head of the READER-level row (#5644) — the boundary above both. + * + * Deliberately not a superstring of `LEDGER_HEADLINE` ("read" vs "load"), so + * the `not.toContain(LEDGER_HEADLINE)` assertions below keep meaning what they + * say when this row is the one on screen. + */ +const READER_HEADLINE = 'Could not load the installed-package ledger reader'; + /** * ── Why this file needs a preflight (#5612) ────────────────────────────── * @@ -209,6 +243,58 @@ describe('installedPackageLedgerFailureCheck — the finding the shared catch us }); }); +describe('installedPackageLedgerReaderFailureCheck — the finding one boundary up (#5644)', () => { + it('quotes what was thrown, in the row AND in the verbose detail', () => { + const check = installedPackageLedgerReaderFailureCheck( + new Error("Cannot find module '/app/node_modules/@objectstack/cloud-connection/dist/index.js'"), + ); + + expect(check.message).toContain('dist/index.js'); + expect(check.fix).toContain('dist/index.js'); + }); + + it('takes the `Unique scope` name column and stays a warning, like its two siblings', () => { + const check = installedPackageLedgerReaderFailureCheck(new Error('boom')); + + // Same reasoning as #5412: an operator scans the report by its name column, + // and a row under a different name leaves `Unique scope` simply missing — + // the silence this family of issues is about, wearing a different hat. + expect(check.name).toBe('Unique scope'); + // The environment still runs; what broke is doctor's sight of part of it. + expect(check.status).toBe('warning'); + }); + + it('says the package is PRESENT — the one thing that separates it from silence', () => { + const check = installedPackageLedgerReaderFailureCheck(new Error('boom')); + const fix = check.fix ?? ''; + + // A checkout that never installed the package prints nothing at all, so + // the row's whole meaning is "it is here and it is broken". If the text did + // not say so, the reader's first move would be to install what is already + // installed. + expect(fix).toContain('IS installed here'); + expect(check.message).toContain('installed packages NOT checked'); + // And it names the remedy for the state repo developers hit daily. + expect(fix).toContain('@objectstack/cloud-connection build'); + }); + + it('does not claim to know whether a ledger exists', () => { + // It cannot: `DEFAULT_INSTALLED_PACKAGES_DIR` is an export of the package + // that would not load. Saying "the ledger is there" (the #5412 row's + // wording, correct for #5412) would be doctor asserting what it just lost + // the ability to check. + const fix = installedPackageLedgerReaderFailureCheck(new Error('boom')).fix ?? ''; + + expect(fix).not.toContain('it exists here'); + expect(fix).toContain('cannot even tell you'); + }); + + it('never trails off into nothing, and reports a thrown non-Error', () => { + expect(installedPackageLedgerReaderFailureCheck(new TypeError()).message).toContain('TypeError'); + expect(installedPackageLedgerReaderFailureCheck('boom').message).toContain('boom'); + }); +}); + 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 @@ -494,23 +580,29 @@ describe('os doctor, end to end, against an unreadable installed-package ledger' }, 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. - */ +/** + * ── The two states the `import()` boundary used to merge (#5644) ───────── + * + * These two describes were ONE before #5644, and it asserted silence for a + * simulation that produced the wrong state. It made the module's evaluation + * throw — "the way an unresolvable specifier does", its comment said — and + * pinned that doctor said nothing. That is exactly the false PASS #5644 is + * about: an evaluation that throws means the package is HERE and unusable, and + * silence over it is a clean bill of health for a ledger nobody read. + * + * So the simulation keeps its mechanism and swaps its verdict (the "replace the + * assertion, not the repro" disposition #5413 used one layer down), and the + * contract it used to stand for — a package that is genuinely NOT INSTALLED is + * silent — moves to its own describe, spelled the only way that is now honest. + */ +describe('the optional package INSTALLED BUT UNLOADABLE is reported (#5644)', () => { /** - * #5612 — this case needs the guard MORE than the e2e block does, not less. - * It simulates the package being unloadable and asserts doctor stays silent; - * in a worktree where the package really is unloadable it passes for that - * reason instead of for the mock's, i.e. green because nothing was proven - * (the empty-verdict trap PR #5046 wrote down). The preflight is what keeps - * the simulation distinguishable from the accident it simulates. + * #5612's preflight still earns its place here, and for its original reason. + * This case makes the real module's evaluation throw and asserts the row that + * follows; in a worktree where `cloud-connection` is genuinely unbuilt, the + * SAME row appears without the mock having done anything — green because the + * accident reproduced the simulation (PR #5046's empty-verdict trap in + * reverse). The guard is what keeps the two distinguishable. */ beforeAll(assertLedgerReaderIsBuilt); @@ -519,7 +611,192 @@ describe('the optional package being absent stays completely silent (#5412 does vi.resetModules(); }); - it('prints no ledger row when the optional package cannot be loaded', async () => { + it('withholds the clean bill and names the reader, with a ledger it never read', async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5644-broken-')); + fs.mkdirSync(path.join(tmp, 'node_modules')); + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + [ + 'export default {', + " manifest: { name: 'os5644', label: 'Broken Reader', version: '1.0.0' },", + " objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],", + '};', + '', + ].join('\n'), + ); + // A ledger that DOES declare an installation-wide unique. Before #5644 this + // exact tree printed `✓ Unique scope` and `invoice.code` appeared nowhere, + // under `--verbose` included — the false PASS, measured. + const ledger = path.join(tmp, '.objectstack/installed-packages'); + fs.mkdirSync(ledger, { recursive: true }); + fs.writeFileSync( + path.join(ledger, 'billing.json'), + JSON.stringify({ + manifestId: 'billing', + manifest: { + objects: [ + { + name: 'invoice', + label: 'Invoice', + fields: [{ name: 'code', type: 'text', label: 'Code', unique: 'global' }], + }, + ], + }, + }), + ); + + const savedPosture = process.env.OS_TENANCY_POSTURE; + process.env.OS_TENANCY_POSTURE = 'isolated'; + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + + vi.resetModules(); + // The package RESOLVES — it is a workspace dependency of this very package + // — and its evaluation throws. That is the "installed and broken" state, + // and the classifier's real `import.meta.resolve()` is what recognises it. + vi.doMock('@objectstack/cloud-connection', () => { + throw new Error('simulated corrupt build artefact'); + }); + 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(['--verbose'], { 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')); + + // ① THE assertion of #5644: no clean bill over a ledger nobody read. + expect(out).not.toContain(CLEAN_BILL); + // ② …replaced by a row that names what could not be loaded. + expect(out).toContain(READER_HEADLINE); + expect(out).toContain('Unique scope'); + // ③ NOT the directory-level row: the directory was never reached, and its + // text asserts a ledger exists — which doctor cannot know from here. + expect(out).not.toContain(LEDGER_HEADLINE); + expect(out).not.toContain(SKIPPED_HEADLINE); + // ④ The verbose channel carries the cause, like every other warning row. + expect(out).toContain('cause:'); + // Gauge: warning, the report finishes, exit stays 0. + expect(out).toContain('Environment is functional but has some warnings'); + }, 60_000); + + it('quotes the load failure verbatim and keeps the row to one line', async () => { + // Driven through the loader seam so the cause is a fixed string rather than + // whatever the mocking machinery wraps a thrown error in. What the previous + // case proves is that the REAL mechanism reaches this row; what this one + // proves is what the row says once it does. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5644-cause-')); + fs.mkdirSync(path.join(tmp, 'node_modules')); + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + [ + 'export default {', + " manifest: { name: 'os5644c', label: 'Cause', version: '1.0.0' },", + " objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],", + '};', + '', + ].join('\n'), + ); + + 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('../utils/optional-package.js', () => ({ + loadOptionalPackage: async () => ({ + state: 'broken', + cause: new Error( + "Cannot find module '/app/node_modules/@objectstack/cloud-connection/dist/index.js'", + ), + }), + })); + 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); + + let exitCode: number | undefined; + try { + await FreshDoctor.run(['--verbose'], { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + exitCode = Number(err.message.split(':')[1]); + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + cwdSpy.mockRestore(); + vi.doUnmock('../utils/optional-package.js'); + vi.resetModules(); + 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')); + const row = out.split('\n').find((l) => l.includes(READER_HEADLINE)) ?? ''; + + // The producer's own words, on the row and in the detail. + expect(row).toContain('dist/index.js'); + expect(out).toContain('IS installed here'); + // One row is one line — the cause is folded, never wrapped into the report. + expect(row.includes('\n')).toBe(false); + expect(out).not.toContain(CLEAN_BILL); + expect(exitCode).toBeUndefined(); + }, 60_000); +}); + +describe('the optional package being genuinely ABSENT stays completely silent (#5412, unchanged)', () => { + /** + * ③, second half — the one branch that is SUPPOSED to swallow, and the + * constraint #5644 was not allowed to break: `os doctor` must run to + * completion, and print its clean bill, in a checkout that never had the + * optional package. + * + * Simulated through the loader seam, and it has to be. Absence is now defined + * by RESOLUTION — `@objectstack/cloud-connection` is a declared dependency of + * this package, so it resolves here no matter what a module mock does to its + * evaluation, and a mock that throws now means "installed and broken" (the + * describe above). The seam is where the two states are decided, so it is the + * seam this case has to speak through. + * + * Which layer proves what, deliberately split: + * - that a real unresolvable specifier IS classified absent, against the + * real runtime: `utils/optional-package.test.ts`. + * - that doctor stays silent when told so: here. + * + * No `assertLedgerReaderIsBuilt` preflight, and that is not a rollback of + * #5612: the preflight exists to stop a case passing because the real package + * happened to be unloadable. Nothing here reads the real package at all — the + * seam is mocked — so there is no accident left for it to catch. + */ + afterEach(() => { + vi.doUnmock('../utils/optional-package.js'); + vi.resetModules(); + }); + + it('prints no ledger row, and still prints the clean bill', async () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5412-nopkg-')); fs.mkdirSync(path.join(tmp, 'node_modules')); fs.writeFileSync( @@ -532,9 +809,9 @@ describe('the optional package being absent stays completely silent (#5412 does '', ].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. + // A ledger directory EXISTS — so if "not installed" were confused with + // "installed and broken" in the other direction, this is where the + // confusion would surface as a spurious warning. fs.mkdirSync(path.join(tmp, '.objectstack/installed-packages'), { recursive: true }); const savedPosture = process.env.OS_TENANCY_POSTURE; @@ -542,9 +819,9 @@ describe('the optional package being absent stays completely silent (#5412 does const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); vi.resetModules(); - vi.doMock('@objectstack/cloud-connection', () => { - throw new Error("Cannot find package '@objectstack/cloud-connection'"); - }); + vi.doMock('../utils/optional-package.js', () => ({ + loadOptionalPackage: async () => ({ state: 'absent' }), + })); const { default: FreshDoctor } = await import('./doctor.js'); const logs: string[] = []; @@ -556,7 +833,7 @@ describe('the optional package being absent stays completely silent (#5412 does }) as never); try { - await FreshDoctor.run([], { root: CLI_ROOT }); + await FreshDoctor.run(['--verbose'], { root: CLI_ROOT }); } catch (err) { if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; } finally { @@ -569,9 +846,13 @@ describe('the optional package being absent stays completely silent (#5412 does } const out = plain(logs.join('\n')); - // Silence about the ledger, and the advisory's own half still reports. + // Silence about the ledger, in all three of its shapes… expect(out).not.toContain(LEDGER_HEADLINE); + expect(out).not.toContain(SKIPPED_HEADLINE); + expect(out).not.toContain(READER_HEADLINE); + // …not even the package's name, under `--verbose`. expect(out).not.toContain('cloud-connection'); + // …and the advisory's own half still reports. expect(out).toContain(CLEAN_BILL); }, 60_000); }); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 96a237d77f..4ed26843c2 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -10,6 +10,11 @@ import { normalizeStackInput } from '@objectstack/spec'; import { printHeader, printSuccess, printWarning, printError, printStep, printInfo } from '../utils/format.js'; import { loadConfig, configExists } from '../utils/config.js'; import { checkSpecVersionGap } from '../utils/spec-version.js'; +// #5644 — "the optional package is not installed" and "it is installed and +// will not load" are two facts, and one `catch` around `import()` cannot tell +// them apart. That classification lives in one place, with the measurements +// behind it written down there. +import { loadOptionalPackage } from '../utils/optional-package.js'; import { validateWidgetBindings } from '@objectstack/lint'; import { resolveTenancyPosture, @@ -819,6 +824,17 @@ interface InstalledPackageLedgerReading { skipped: SkippedLedgerEntry[]; /** Present ONLY when the ledger EXISTS and could not be read. */ failure?: { cause: unknown }; + /** + * Present ONLY when the package that READS the ledger is installed and could + * not be loaded (#5644). + * + * A THIRD fact, one boundary above `failure`: that one is "the ledger is + * there and I could not read it", this one is "the thing I read ledgers with + * is there and I could not load it". Both leave the ledger unexamined; only + * this one leaves doctor unable to say where the ledger even is, because the + * directory name is the missing package's own constant. + */ + readerFailure?: { cause: unknown }; } /** @@ -865,17 +881,34 @@ interface SkippedLedgerEntry { * 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`. + * + * And a FOURTH, one boundary ABOVE case 1 (#5644). Case 1 says "the specifier + * does not resolve"; the `catch` that implemented it said "the `import()` + * threw", which is not the same sentence. A package that IS installed and will + * not load — a pruned or unbuilt `dist/`, an interrupted install, an artefact + * that throws while it evaluates — threw too, and was answered with the silence + * meant for a package that was never there. The ledger went unread with no + * trace, and the report printed `✓ Unique scope` for the third time in this + * function's history: now over a reader it could not even start. The two are + * separated by `loadOptionalPackage()` (`utils/optional-package.ts` carries how, + * and the measurements behind it); only the genuinely-absent half stays silent. */ 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. THIS - // catch, and only this one, is allowed to be silent. - mod = await import('@objectstack/cloud-connection'); - } catch { - return { entries: [], skipped: [] }; + // Dynamic, like serve.ts's cloud-connection load: `os doctor` must still run + // in a checkout where the optional package is not resolvable. + const load = await loadOptionalPackage('@objectstack/cloud-connection'); + // Case 1, and ONLY case 1, is allowed to be silent: no package is here, so + // nothing was installed through it and nothing went unchecked. + if (load.state === 'absent') return { entries: [], skipped: [] }; + // Case 4. Reported whether or not a ledger directory exists: doctor cannot + // honestly claim there is no ledger when the constant naming the ledger's + // location is an export of the package that would not load. Gating this row + // on `fs.existsSync()` is the rejected option B of #5644 — it reads "has + // anything ever been installed" as a proxy for "should this package be here". + if (load.state === 'broken') { + return { entries: [], skipped: [], readerFailure: { cause: load.cause } }; } + const mod: any = load.module; const dir = path.join(cwd, mod.DEFAULT_INSTALLED_PACKAGES_DIR ?? '.objectstack/installed-packages'); try { @@ -908,6 +941,14 @@ interface UniqueScopeReading { * say it does not. */ skippedLedgerEntries: SkippedLedgerEntry[]; + /** + * Present when the ledger half could not even START — the package doctor + * reads ledgers through is installed and would not load (#5644). Suppresses + * the success line for the same reason `ledgerFailure` does, and is reported + * separately because it is a different fact with a different remedy: repair + * the INSTALL of `@objectstack/cloud-connection`, not the ledger. + */ + ledgerReaderFailure?: { cause: unknown }; } /** @@ -952,6 +993,7 @@ async function findUnscopedGlobalUniques( advisories: out, skippedLedgerEntries: ledger.skipped, ...(ledger.failure ? { ledgerFailure: ledger.failure } : {}), + ...(ledger.readerFailure ? { ledgerReaderFailure: ledger.readerFailure } : {}), }; } @@ -1301,6 +1343,56 @@ export function installedPackageLedgerSkippedEntriesCheck( }; } +/** + * What doctor reports when the package it reads ledgers THROUGH is installed + * and will not load (#5644). + * + * The third sibling of `installedPackageLedgerFailureCheck`, one boundary + * above it. That one fires when the ledger directory could not be enumerated; + * `installedPackageLedgerSkippedEntriesCheck` fires when individual files in it + * would not parse; this one fires when the reader itself never started. All + * three produce the identical false PASS if unreported — `✓ Unique scope` over + * installed packages nobody looked at — so all three take the `Unique scope` + * name column, hold back the success line, and stay warnings. + * + * What is deliberately NOT a condition here: whether + * `.objectstack/installed-packages/` exists. Doctor does not know that it does + * not — the directory's name is `DEFAULT_INSTALLED_PACKAGES_DIR`, an export of + * the very package that would not load, and answering from the hard-coded + * fallback would be doctor claiming knowledge it just lost. Making the row + * conditional on the directory was option B of #5644 and was rejected on + * exactly that ground: "has anything ever been installed" is not a proxy for + * "should this package be here". + * + * The row exists because the ALTERNATIVE is provably worse, and was measured: + * with the package present-but-unloadable, a ledger declaring an + * installation-wide `unique` produced `✓ Unique scope` and the finding + * appeared nowhere, under `--verbose` included. In-repo this state is reached + * daily — any worktree where `packages/cloud-connection` is unbuilt — and its + * silence is what sent #5612 chasing a report face that had never regressed. + */ +export function installedPackageLedgerReaderFailureCheck(err: unknown): HealthCheckResult { + const cause = describeThrown(err); + return { + name: 'Unique scope', + status: 'warning', + message: + 'Could not load the installed-package ledger reader (installed packages NOT checked ' + + `for installation-wide uniques) — ${reportRowHeadline(cause)}`, + fix: + '`@objectstack/cloud-connection` IS installed here — its specifier resolves — and loading\n' + + ' it threw. That package is how `os doctor` reads `.objectstack/installed-packages/`,\n' + + ' so this half of the check never started: an installed app declaring an\n' + + ' installation-wide `unique` would not have appeared. Doctor cannot even tell you\n' + + ' whether a ledger is present — the directory’s name is one of that package’s\n' + + ' exports.\n' + + ' A checkout that never installed the package says nothing at all, so this row means\n' + + ' the install itself is broken: reinstall it, or — in a monorepo checkout — build it\n' + + ' (`pnpm --filter @objectstack/cloud-connection build`).\n' + + ` cause: ${indentUnderGutter(cause)}`, + }; +} + // ─── Command ──────────────────────────────────────────────────────── export default class Doctor extends Command { @@ -1599,11 +1691,12 @@ 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, skippedLedgerEntries } = await findUnscopedGlobalUniques( - cwd, - config, - postureReading.posture, - ); + const { + advisories, + ledgerFailure, + ledgerReaderFailure, + skippedLedgerEntries, + } = await findUnscopedGlobalUniques(cwd, config, postureReading.posture); if (advisories.length > 0) { hasWarnings = true; for (const { source, finding } of advisories) { @@ -1629,7 +1722,18 @@ export default class Doctor extends Command { flags.verbose, ); } - if (ledgerFailure) { + // #5644 — the same claim failing one boundary UP: the reader + // package is installed and would not load, so neither the directory + // nor the entries were ever reached. Mutually exclusive with the two + // above (nothing downstream of a reader that never loaded can also + // fail), so it is an `else if` rather than a fourth independent row. + if (ledgerReaderFailure) { + hasWarnings = true; + renderHealthCheckResult( + installedPackageLedgerReaderFailureCheck(ledgerReaderFailure.cause), + flags.verbose, + ); + } else if (ledgerFailure) { hasWarnings = true; renderHealthCheckResult(installedPackageLedgerFailureCheck(ledgerFailure.cause), flags.verbose); } else if (advisories.length === 0 && skippedLedgerEntries.length === 0) { diff --git a/packages/cli/src/utils/optional-package.test.ts b/packages/cli/src/utils/optional-package.test.ts new file mode 100644 index 0000000000..ec0fd5a3b2 --- /dev/null +++ b/packages/cli/src/utils/optional-package.test.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `loadOptionalPackage()` tells "not installed" from "installed and broken" + * (#5644). + * + * Every case below drives the REAL mechanism — a real `import()` and a real + * `import.meta.resolve()` against real files on disk. Nothing is mocked, + * because the whole subject of this module is what the runtime actually + * answers, and a mocked resolver would pin the assumption rather than the + * behaviour. `os doctor`'s three-state report is built on top of this, and its + * own file mocks THIS module in turn; the split is deliberate — the + * classification is proven here against the runtime, the rendering is proven + * there against doctor. + * + * The state each case stands for, in the world the caller cares about: + * + * - absent → the optional package was never installed. + * - broken / throwing → an artefact that blows up while it evaluates. + * - broken / entry gone → an unbuilt or pruned `dist/`. THE case a + * `catch`-only classifier gets wrong: it fails with + * `ERR_MODULE_NOT_FOUND`, exactly like a package + * that is not installed at all. + * - broken / dep missing → the package is fine, something under it is not. + * - loaded → nothing to report. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; + +import { loadOptionalPackage } from './optional-package.js'; + +/** A specifier nothing in this workspace resolves — genuinely not installed. */ +const NEVER_INSTALLED = '@objectstack/not-a-real-package-5644'; + +let tmp: string; +/** Absolute `file:` URL, so resolution is anchored on the file, not on a base. */ +const fixtureUrl = (name: string) => pathToFileURL(path.join(tmp, name)).href; + +beforeAll(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-5644-optional-pkg-')); + fs.writeFileSync(path.join(tmp, 'healthy.mjs'), 'export const marker = "loaded";\n'); + fs.writeFileSync(path.join(tmp, 'throws.mjs'), 'throw new Error("boom while evaluating");\n'); + fs.writeFileSync( + path.join(tmp, 'missing-dep.mjs'), + 'import "@objectstack/not-a-real-dependency-5644";\nexport const marker = "unreachable";\n', + ); +}); + +afterAll(() => { + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +describe('loadOptionalPackage — the two facts one `catch` used to merge', () => { + it('reports a specifier that does not resolve as absent, and carries no cause', async () => { + const load = await loadOptionalPackage(NEVER_INSTALLED); + + // Silence downstream depends on this: `os doctor` must run to completion + // in a checkout that never had the optional package. + expect(load.state).toBe('absent'); + // An absent package has nothing to say, so there is nothing to report. + expect(load).not.toHaveProperty('cause'); + }); + + it('reports a module that throws while evaluating as broken, with what it threw', async () => { + const load = await loadOptionalPackage(fixtureUrl('throws.mjs')); + + expect(load.state).toBe('broken'); + expect((load as { cause: Error }).cause).toBeInstanceOf(Error); + // Quoted, not paraphrased — the row doctor prints repeats this verbatim. + expect((load as { cause: Error }).cause.message).toContain('boom while evaluating'); + }); + + it('reports a RESOLVABLE module whose file is not there as broken, not absent', async () => { + // The unbuilt / pruned `dist/` state, and the reason this module exists. + // Node answers it with `ERR_MODULE_NOT_FOUND` — the same code a package + // that was never installed produces — so classifying by the error alone + // returns "absent" here and the caller goes silent over a package that IS + // installed. Resolution is what separates them: it succeeds, because the + // specifier is resolvable even though the file it names is missing. + const load = await loadOptionalPackage(fixtureUrl('not-built-yet.mjs')); + + expect(load.state).toBe('broken'); + expect(String((load as { cause: Error }).cause.message)).toContain('not-built-yet.mjs'); + }); + + it('reports a package whose own dependency is missing as broken, not absent', async () => { + // The same trap one level down: the failure is `ERR_MODULE_NOT_FOUND` and + // it is NOT about this package, which is present and resolvable. + const load = await loadOptionalPackage(fixtureUrl('missing-dep.mjs')); + + expect(load.state).toBe('broken'); + expect(String((load as { cause: Error }).cause.message)).toContain( + '@objectstack/not-a-real-dependency-5644', + ); + }); + + it('hands back the namespace when the package loads', async () => { + const load = await loadOptionalPackage(fixtureUrl('healthy.mjs')); + + expect(load.state).toBe('loaded'); + expect((load as { module: { marker: string } }).module.marker).toBe('loaded'); + }); + + it('resolves a real workspace package rather than mistaking it for absent', async () => { + // The end-to-end control: the package `os doctor` actually loads. If this + // ever came back `absent` in a built worktree, every ledger check in this + // repo would be silently skipped and nothing else would notice. + const load = await loadOptionalPackage('@objectstack/cloud-connection'); + + expect(load.state).toBe('loaded'); + }); +}); diff --git a/packages/cli/src/utils/optional-package.ts b/packages/cli/src/utils/optional-package.ts new file mode 100644 index 0000000000..b4a8a98321 --- /dev/null +++ b/packages/cli/src/utils/optional-package.ts @@ -0,0 +1,123 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Loading a package the caller can live without — with "it is not here" and + * "it is here and broken" kept apart (#5644). + * + * ── Why this exists ────────────────────────────────────────────────────── + * + * The shape it replaces is one `try` around a dynamic `import()` whose `catch` + * means "not installed": + * + * try { mod = await import('@objectstack/cloud-connection'); } + * catch { return nothing; } + * + * That is right for exactly one of the two things it catches. A specifier that + * does not resolve IS the optional package being absent, and silence is the + * correct, deliberate answer — `os doctor` must run to completion in a checkout + * that never had it. But a package that IS there and fails to load — an + * interrupted install, a pruned or unbuilt `dist/`, an artefact that throws + * while it evaluates, a transitive dependency missing under it — arrives in the + * same `catch` and is answered with the same silence. The caller then reports + * on a world it never looked at. In `os doctor` that produced a clean bill of + * health over an installed-package ledger it had not read (#5644, the `import` + * boundary of the same false PASS #5412 fixed at the `readdir` boundary). + * + * ── How the two are told apart ─────────────────────────────────────────── + * + * Two mechanisms, in this order, both owned elsewhere in this repo: + * + * 1. **The error is not a module-not-found error at all** — the module was + * found and threw while evaluating. Nothing more needs asking: that is a + * broken package under every resolver. `isModuleNotFoundError()` is the + * repo's single shared owner of this classification (`@objectstack/types`, + * framework#3265); `serve.ts` reads it for the same purpose. + * 2. **It IS a module-not-found error** — which still covers two worlds, and + * the error alone cannot separate them. `import.meta.resolve()` can, and + * is the documented, stable API for asking it (Node >= 20.6; this package + * requires >= 22). Measured on Node 22.22 against the real package: + * + * | state | resolve() | import() | + * |-----------------------------------------|-------------|-------------| + * | package absent | throws | throws | + * | present, `dist/` missing (unbuilt/prun.) | RESOLVES | throws | + * | present, entry throws while evaluating | resolves | throws | + * | healthy | resolves | loads | + * + * The second row is the one that matters and the reason `resolve()` is + * used rather than `createRequire().resolve()`: CJS resolution stats the + * entry file, so it fails there too and collapses the row back into + * "absent" — and it answers for the `require` condition, i.e. a DIFFERENT + * artefact than the one `import()` loads. `import.meta.resolve()` answers + * for the same artefact and does not stat it, so "the package is there" + * and "its entry is loadable" stay two questions. + * + * The resolve call is made only after `import()` has already failed: it costs + * nothing on the happy path, and the loaded module — never the resolver — stays + * the thing that decides whether loading worked. + * + * ── The one thing this must never do ───────────────────────────────────── + * + * Report a package that is genuinely absent. Every `absent` answer here is a + * caller's silence; every `broken` answer is a caller's finding. When the + * classification cannot be made — a runtime with no `import.meta.resolve` — + * only step 1's certainty is kept and the ambiguous case stays `absent`, i.e. + * the pre-#5644 behaviour, never a false alarm in a checkout that never + * installed anything. + */ + +import { isModuleNotFoundError } from '@objectstack/types'; + +/** + * What became of an optional package's load. Three states, because there are + * three facts — the two-state version of this type is the defect. + */ +export type OptionalPackageLoad = + /** The specifier does not resolve. The package is not installed. */ + | { state: 'absent' } + /** + * It loaded. `module` is its namespace — `any`, like every other read of an + * optional package in this package: nothing here compiles against it. + */ + | { state: 'loaded'; module: any } + /** It is installed and could not be loaded. `cause` is what was thrown. */ + | { state: 'broken'; cause: unknown }; + +/** + * True when `specifier` resolves from THIS module — the same base the + * `import()` below uses, so the two answer about the same package. + * + * A runtime without `import.meta.resolve` cannot answer, and says so by + * returning `false`: the caller then keeps the ambiguous case silent (see the + * header). Node 22+, this package's floor, always has it; so does vitest. + */ +function specifierResolves(specifier: string): boolean { + const resolve = (import.meta as ImportMeta & { resolve?: (s: string) => string }).resolve; + if (typeof resolve !== 'function') return false; + try { + resolve(specifier); + return true; + } catch { + return false; + } +} + +/** + * Load an optional package, saying which of the three things happened. + * + * @param specifier Bare package name, resolved from this file exactly as a + * static import in it would be. + */ +export async function loadOptionalPackage(specifier: string): Promise { + try { + const module = await import(specifier); + return { state: 'loaded', module }; + } catch (cause) { + // Anything that is not a module-not-found error was thrown BY the package: + // it is present by definition, whatever any resolver would say. + if (!isModuleNotFoundError(cause)) return { state: 'broken', cause }; + // A module-not-found error is either the package being absent or something + // missing UNDER a package that is present. Only the resolver knows. + return specifierResolves(specifier) ? { state: 'broken', cause } : { state: 'absent' }; + } +} From 45277acb04b987732c04d5e4a8cf11c92a0a4d06 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:37:32 +0000 Subject: [PATCH 2/2] docs(cli): state what loadOptionalPackage's specifier accepts (#5644) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- packages/cli/src/utils/optional-package.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/utils/optional-package.ts b/packages/cli/src/utils/optional-package.ts index b4a8a98321..87036475d9 100644 --- a/packages/cli/src/utils/optional-package.ts +++ b/packages/cli/src/utils/optional-package.ts @@ -105,8 +105,10 @@ function specifierResolves(specifier: string): boolean { /** * Load an optional package, saying which of the three things happened. * - * @param specifier Bare package name, resolved from this file exactly as a - * static import in it would be. + * @param specifier What a static import in THIS file would be given — normally + * a bare package name, resolved against this file's own `node_modules` chain. + * An absolute `file:` URL resolves as itself and is what the tests use to stand + * a real broken artefact up on disk. */ export async function loadOptionalPackage(specifier: string): Promise { try {