From 32522260af2ecaed4ac8e9713113d3dc6e5b5228 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:30:21 +0000 Subject: [PATCH 1/3] fix(cli): doctor ledger failure row quotes the resolved dir; annotate install.ts prose (#6643) --- packages/cli/src/commands/doctor.ts | 25 +++++++++++--- packages/cli/src/commands/package/install.ts | 35 ++++++++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 6b4434eca1..5b6405337b 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -1408,17 +1408,25 @@ const LEDGER_ROW_NAME = 'Installed packages'; * • **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. + * • **`dir` is the directory doctor actually read** (#6643) — resolved from + * `DEFAULT_INSTALLED_PACKAGES_DIR` and carried on the reading, exactly as + * its `skipped` sibling takes it since #5996. It used to open with a + * re-hardcoded ``.objectstack/installed-packages/`` literal "under the + * project root": the consumer restating a value only the producer decides, + * and — since the resolved directory is `cwd`-joined — a vaguer answer than + * the one doctor was holding. A row reporting an unreadable directory owes + * the reader the directory it actually tried. */ -export function installedPackageLedgerFailureCheck(err: unknown): HealthCheckResult { +export function installedPackageLedgerFailureCheck(err: unknown, dir: string): HealthCheckResult { const cause = describeThrown(err); return { name: LEDGER_ROW_NAME, status: 'warning', message: `Could not read the installed-package ledger (installed packages NOT checked) — ${reportRowHeadline(cause)}`, fix: - '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". Every package it lists is one\n' + `The ledger is \`${dir}\`; it exists here, which is why\n` + + ' this is reported rather than treated as "nothing was ever\n' + + ' installed". Every package it lists is one\n' + ' this runtime ALSO cannot rehydrate at boot — not registered with the\n' + ' kernel, absent from the console’s installed-apps list — so an app missing\n' + ' from this environment is very likely in there.\n' @@ -1675,7 +1683,14 @@ export function installedPackageLedgerChecks( return [installedPackageLedgerDirAuthorityMissingCheck(reading.dirAuthorityMissing.received)]; } const out: HealthCheckResult[] = []; - if (reading.failure) out.push(installedPackageLedgerFailureCheck(reading.failure.cause)); + // Same invariant as the `skipped` row below, one boundary out (#6643): a + // `failure` reading always carries the directory the failure was ABOUT. + // `failure` is set only in the `catch` of `readInstalledPackageEntries()`, + // and that `try` opens after `dir` is already resolved — the two are set on + // the same return. The `!` states that where the flat reading shape cannot, + // and the parameter stays required so this row can never quietly fall back + // to a guessed literal. + if (reading.failure) out.push(installedPackageLedgerFailureCheck(reading.failure.cause, reading.dir!)); // Independent of the row above, not an `else`: `failure` means the directory // could not be enumerated at all, `skipped` means it enumerated fine and // named files inside it would not parse. Each names packages the other does diff --git a/packages/cli/src/commands/package/install.ts b/packages/cli/src/commands/package/install.ts index c5f9e4e17c..c770560231 100644 --- a/packages/cli/src/commands/package/install.ts +++ b/packages/cli/src/commands/package/install.ts @@ -215,6 +215,41 @@ export default class PackageInstall extends Command { printKV(' Runtime', runtime); if (data.installedAt) printKV(' Installed', String(data.installedAt)); console.log(''); + // ⚠️ This path is a DESCRIPTION OF THE DEFAULT CONVENTION, deliberately + // left as a literal — not a consumer restating a value it could have read + // (#6643, the sibling half of #5996). Do not "fix" it into a reference to + // `DEFAULT_INSTALLED_PACKAGES_DIR`. Four measured reasons, in order: + // + // 1. **The directory is on the REMOTE host.** Everything above this + // line came back over HTTP from `runtime`; this command never + // touches the target's disk. A constant resolved HERE describes the + // machine typing the command, not the one that stored the manifest. + // 2. **The remote's directory is configurable, so the constant is only + // its default.** `MarketplaceInstallLocalPlugin` builds its ledger as + // `new LocalManifestSource(config.storageDir)` — the export is the + // fallback that ctor applies when the host configured nothing. + // Interpolating it would state a default as an observed fact. + // 3. **The response we just read does not carry the real answer.** The + // POST returns `{ manifestId, version, versionId, installedAt, + // hotLoaded, upgradedFrom, translationsLoaded, seeded, note }` — no + // `storageDir`. The GET listing endpoint does carry one; this one + // does not, and asking for it would be an extra round-trip (and a + // new failure mode) bolted onto a success hint. + // 4. **Importing it would cost this command its independence.** Every + // CLI reference to `@objectstack/cloud-connection` is a DYNAMIC load + // behind `loadOptionalPackage()` (doctor.ts) or a guarded `import()` + // (serve.ts), because the CLI must keep working where that package + // is absent or unbuilt. A static import for one hint line would make + // a pure-HTTP command fail at module load; a dynamic one needs a + // literal fallback — the exact `??` Prime Directive #12 forbids and + // #5996 deleted. + // + // So the divergence surface is accepted here, knowingly: if the constant + // ever changes value, this sentence goes stale and no gate will say so. + // The honest fix is upstream — the POST response carrying `storageDir` + // like its GET sibling — which would let this line quote the real remote + // directory. Tracked separately; `@objectstack/cloud-connection` is out + // of scope for #6643. console.log(' The manifest is cached under .objectstack/installed-packages/ on the'); console.log(' runtime host and re-registers on every boot (survives restarts).'); } catch (error) { From f48db1fa97bb1e0bdeaa7fdbca483d79483bb187 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:32:02 +0000 Subject: [PATCH 2/3] test(cli): pin the failure row's resolved dir against the authority (#6643) --- .../doctor-ledger-read-failure.test.ts | 77 ++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) 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 f7ce207e3b..5a6451e9ca 100644 --- a/packages/cli/src/commands/doctor-ledger-read-failure.test.ts +++ b/packages/cli/src/commands/doctor-ledger-read-failure.test.ts @@ -210,11 +210,20 @@ async function assertLedgerReaderIsBuilt(): Promise { } describe('installedPackageLedgerFailureCheck — the finding the shared catch used to eat', () => { + /** + * The resolved ledger directory the reading carries (#5996), which this row + * has taken as a required parameter since #6643. Deliberately rooted at + * `/srv/app` rather than left relative: the old hard-coded literal was + * `.objectstack/installed-packages/` with no root, so any assertion naming + * this path is one the literal cannot satisfy. + */ + const DIR = '/srv/app/.objectstack/installed-packages'; + 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); + const check = installedPackageLedgerFailureCheck(err, DIR); // Before #5412 this text existed nowhere in any doctor output under any // flag — the error object was discarded at the point it was caught. @@ -223,7 +232,7 @@ describe('installedPackageLedgerFailureCheck — the finding the shared catch us }); it('takes the `Installed packages` name column, so the row is present rather than missing', () => { - const check = installedPackageLedgerFailureCheck(new Error('boom')); + const check = installedPackageLedgerFailureCheck(new Error('boom'), DIR); // Load-bearing, not cosmetic. An operator scans the report by its name // column, and this row has to be somewhere findable rather than absent — @@ -238,22 +247,25 @@ describe('installedPackageLedgerFailureCheck — the finding the shared catch us }); it('stays a warning — the environment runs, doctor’s sight of it is what broke', () => { - expect(installedPackageLedgerFailureCheck(new Error('boom')).status).toBe('warning'); + expect(installedPackageLedgerFailureCheck(new Error('boom'), DIR).status).toBe('warning'); }); it('says WHICH half did not run, rather than that "something" failed', () => { - const check = installedPackageLedgerFailureCheck(new Error('boom')); + const check = installedPackageLedgerFailureCheck(new Error('boom'), DIR); 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/'); + // Re-pointed by #6643. This used to assert the bare + // `.objectstack/installed-packages/` literal the fix restated; the row now + // names the directory doctor actually read, so the assertion follows it. + expect(fix).toContain(DIR); }); 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')); + const check = installedPackageLedgerFailureCheck(new Error('line one\nline two: the reason'), DIR); expect(check.message).toContain('line two: the reason'); // One row is one line. @@ -262,15 +274,37 @@ describe('installedPackageLedgerFailureCheck — the finding the shared catch us }); it('never trails off into nothing for an Error with no message', () => { - const check = installedPackageLedgerFailureCheck(new TypeError()); + const check = installedPackageLedgerFailureCheck(new TypeError(), DIR); 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'); + expect(installedPackageLedgerFailureCheck('boom', DIR).message).toContain('boom'); + expect(installedPackageLedgerFailureCheck(42, DIR).message).toContain('42'); + }); + + it('a NON-default directory flows through — the assertion a re-hardcoded literal cannot pass', () => { + // #6643, the sibling of #5996's identical case on + // `installedPackageLedgerSkippedEntriesCheck`. The parameter exists so this + // row tracks the producer's `DEFAULT_INSTALLED_PACKAGES_DIR` instead of + // restating the consumer's old guess. A directory sharing NO substring with + // that guess is what separates the two: if the literal ever creeps back, + // both assertions below go red at once. + const check = installedPackageLedgerFailureCheck(new Error('boom'), '/var/lib/os-ledger'); + + expect(check.fix).toContain('The ledger is `/var/lib/os-ledger`;'); + expect(check.fix).not.toContain('.objectstack/installed-packages'); + }); + + it('drops the "under the project root" hedge — the resolved dir already says where it is', () => { + // The old literal was relative, so the row had to add a sentence locating + // it. `dir` is `cwd`-joined and absolute, which makes that sentence both + // redundant and (for a non-default `storageDir`) wrong. + const check = installedPackageLedgerFailureCheck(new Error('boom'), DIR); + + expect(check.fix).not.toContain('project root'); }); }); @@ -430,6 +464,31 @@ describe('os doctor, end to end, against an unreadable installed-package ledger' expect(run.exitCode).toBeUndefined(); }, 60_000); + it('the verbose fix names the directory doctor actually read, resolved from the authority (#6643)', async () => { + writeConfig(); + fs.mkdirSync(path.dirname(ledgerPath()), { recursive: true }); + fs.writeFileSync(ledgerPath(), 'not a directory\n'); + + // The expectation is COMPUTED FROM THE AUTHORITY, never hard-coded: the + // whole point of #6643 is that this row stops restating a value only + // `@objectstack/cloud-connection` decides. Writing `.objectstack/ + // installed-packages` here would move the literal into the test and pin + // the row against the guess a second time — the defect, relocated. + // Re-imported rather than read off `LEDGER_REL` for the same reason. + const { DEFAULT_INSTALLED_PACKAGES_DIR } = await import('@objectstack/cloud-connection'); + // `mkdtemp` makes this unique per run, which is what makes "the resolved + // dir, not the literal" assertable at all: no literal can contain it. + const resolved = path.join(tmp, DEFAULT_INSTALLED_PACKAGES_DIR); + + const run = await runDoctor(['--verbose']); + + expect(run.out).toContain(LEDGER_HEADLINE); + expect(run.out).toContain(`The ledger is \`${resolved}\`;`); + // And the relative literal it replaced is gone from the report entirely. + expect(run.out).not.toContain('The ledger is `.objectstack/installed-packages/` directory'); + expect(run.out).not.toContain('under the\n project root'); + }, 60_000); + it('expands the detail under --verbose, and only under --verbose', async () => { writeConfig(); fs.mkdirSync(path.dirname(ledgerPath()), { recursive: true }); From 381ac45998b6adc13693a9a290e283db9c74b9ad Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 13:30:39 +0000 Subject: [PATCH 3/3] chore(cli): changeset for #6643 --- .changeset/doctor-ledger-failure-dir.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .changeset/doctor-ledger-failure-dir.md diff --git a/.changeset/doctor-ledger-failure-dir.md b/.changeset/doctor-ledger-failure-dir.md new file mode 100644 index 0000000000..dc0ece7fc9 --- /dev/null +++ b/.changeset/doctor-ledger-failure-dir.md @@ -0,0 +1,12 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os doctor`'s ledger-failure row names the directory it actually read (#6643) + +Removes divergence surface; not a live defect. `DEFAULT_INSTALLED_PACKAGES_DIR` — `@objectstack/cloud-connection`'s export, the single authority on what the installed-package ledger directory is called — exists in every version ever shipped and has never changed value, so the literal this change deletes currently agrees with it. What it buys is that the agreement stops being a coincidence nobody would notice breaking. + +The residue of #5996, which fixed the same restatement one row over (`installedPackageLedgerSkippedEntriesCheck`) and enumerated the rest rather than widening in place: + +- `installedPackageLedgerFailureCheck` takes the resolved `dir` — already carried on the reading since #5996 — and quotes it. Its `fix` used to open with a re-hardcoded ``.objectstack/installed-packages/`` "under the project root", which was the consumer restating a value only the producer decides, and a vaguer answer than the one doctor was holding: the reading's `dir` is `cwd`-joined and absolute. Under `--verbose` the row now reads ``The ledger is `/srv/app/.objectstack/installed-packages`;`` and drops the now-redundant project-root hedge. The parameter is required, so the row cannot quietly fall back to a guess. +- `os package install`'s post-install hint keeps its literal, now with the reasons written down. That sentence describes the **remote** runtime host's directory: the CLI never touches that disk, the host's directory is configurable (`MarketplaceInstallLocalPlugin` builds `new LocalManifestSource(config.storageDir)`, so the export is only its default), and the install response carries no `storageDir` to quote. Resolving it locally would state this machine's default as an observed fact about another one — so the literal stays, marked as the description of a convention rather than a consumer read.