From 0453d4c19ce3073a2d2a2ee2c7933a6dfd87f233 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 05:28:41 +0000 Subject: [PATCH] fix(cli): load driver-sql's schema-work classifier lazily (#5726) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `schema-migrate.ts` statically value-imported `isInPlaceSchemaWork` from `@objectstack/driver-sql`. That import is not paid for by the command that needs it: oclif's `findCommand` `import()`s every command module on every CLI invocation, and nine commands reach this file (`meta:resync`, `migrate` and seven `migrate:*`). An unbuilt `packages/drivers/driver-sql/dist` therefore printed nine MODULE_NOT_FOUND blocks — naming nine commands the operator never invoked — in front of whatever they actually ran, and dropped all nine out of the command table (`os migrate plan` answered `Command migrate:plan not found.`). Measured on a worktree with driver-sql's dist moved aside: before `os --version` 9 MODULE_NOT_FOUND blocks, exit 0 `os migrate plan -h` 9 blocks + `Command migrate:plan not found.` after `os --version` 0 blocks, clean version line `os migrate plan -h` full help, exit 0 The classifier keeps its ONE definition in the driver — the additive/in-place split is a fact about `PendingSchemaWorkKind`, and a copy in the CLI would be free to disagree the day a kind is added, by listing a row rewrite under the heading that promises the work is never data-losing (#3954). So this is a lazy `await import()` at the point of use, not a re-derivation. `renderPendingSchemaWork` / `summarizePendingSchemaWork` become async; their five call sites in `migrate plan` / `migrate apply` await them. A source-level pin test keeps the shape from growing back: no CLI production module may statically value-import an `@objectstack/driver-*` package, the dynamic import to driver-sql must survive, and every call of the two async renderers must be awaited. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DWUR56YsttL5sTF72Q75TQ --- .changeset/cli-lazy-driver-sql-import.md | 25 ++++ packages/cli/src/commands/migrate/apply.ts | 6 +- packages/cli/src/commands/migrate/plan.ts | 4 +- .../schema-migrate.lazy-driver-import.test.ts | 122 ++++++++++++++++++ .../schema-migrate.pending-render.test.ts | 36 +++--- packages/cli/src/utils/schema-migrate.ts | 43 +++++- 6 files changed, 210 insertions(+), 26 deletions(-) create mode 100644 .changeset/cli-lazy-driver-sql-import.md create mode 100644 packages/cli/src/utils/schema-migrate.lazy-driver-import.test.ts diff --git a/.changeset/cli-lazy-driver-sql-import.md b/.changeset/cli-lazy-driver-sql-import.md new file mode 100644 index 0000000000..d3dc8ab1eb --- /dev/null +++ b/.changeset/cli-lazy-driver-sql-import.md @@ -0,0 +1,25 @@ +--- +'@objectstack/cli': patch +--- + +CLI: load the SQL driver's schema-work classifier lazily, so an unbuilt driver no longer breaks command discovery (#5726) + +`packages/cli/src/utils/schema-migrate.ts` statically value-imported +`isInPlaceSchemaWork` from `@objectstack/driver-sql`. oclif's `findCommand` +`import()`s every command module on every CLI invocation, and nine commands +reach that file (`meta:resync`, `migrate`, and seven `migrate:*`), so a +workspace whose `packages/drivers/driver-sql/dist` was not built printed nine +`MODULE_NOT_FOUND` blocks — naming nine commands the operator never invoked — +in front of whatever command they actually ran, and dropped all nine out of the +command table (`os migrate plan` answered `Command migrate:plan not found.`). + +The import is now `await import('@objectstack/driver-sql')` at the point of use, +inside the two renderers that need the classifier. The classifier keeps its one +definition in the driver — it is a fact about `PendingSchemaWorkKind` and a copy +in the CLI could disagree, listing a row rewrite under the heading that promises +the work is never data-losing. + +No user-visible behaviour change: this is local/worktree developer experience +only, and CI always builds before running the CLI. `renderPendingSchemaWork` and +`summarizePendingSchemaWork` — internal helpers, not part of the package's +public entry — are now `async`. diff --git a/packages/cli/src/commands/migrate/apply.ts b/packages/cli/src/commands/migrate/apply.ts index acb2ee41cd..acbb32caae 100644 --- a/packages/cli/src/commands/migrate/apply.ts +++ b/packages/cli/src/commands/migrate/apply.ts @@ -161,9 +161,9 @@ export default class MigrateApply extends Command { if (!flags.json) { printInfo(`Database: ${chalk.white(stack.dbLabel)}`); console.log(''); - renderPendingSchemaWork(pending); + await renderPendingSchemaWork(pending); renderPlan(drift); - if (pending.length > 0) printInfo(summarizePendingSchemaWork(pending)); + if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending)); printInfo(summarize(drift)); if (deferred.length > 0) { printWarning(`${deferred.length} destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).`); @@ -211,7 +211,7 @@ export default class MigrateApply extends Command { console.log(''); if (created.length > 0) { - printSuccess(`Created/extended ${created.length} table(s): ${summarizePendingSchemaWork(created)}.`); + printSuccess(`Created/extended ${created.length} table(s): ${await summarizePendingSchemaWork(created)}.`); } printSuccess(`Applied ${applied.length} change(s).`); if (skipped.length > 0) { diff --git a/packages/cli/src/commands/migrate/plan.ts b/packages/cli/src/commands/migrate/plan.ts index a92597d0a6..9f9f5ea0ec 100644 --- a/packages/cli/src/commands/migrate/plan.ts +++ b/packages/cli/src/commands/migrate/plan.ts @@ -158,9 +158,9 @@ export default class MigratePlan extends Command { return; } - renderPendingSchemaWork(pending); + await renderPendingSchemaWork(pending); renderPlan(drift); - if (pending.length > 0) printInfo(summarizePendingSchemaWork(pending)); + if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending)); printInfo(summarize(drift)); console.log(chalk.dim(' Apply with: ') + chalk.white('os migrate apply') + chalk.dim(' (add --allow-destructive for drops / tightenings)')); diff --git a/packages/cli/src/utils/schema-migrate.lazy-driver-import.test.ts b/packages/cli/src/utils/schema-migrate.lazy-driver-import.test.ts new file mode 100644 index 0000000000..62794d8f06 --- /dev/null +++ b/packages/cli/src/utils/schema-migrate.lazy-driver-import.test.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5726 — no CLI production module may STATICALLY value-import a driver. + * + * The cost of one such import is not paid by the command that needs the driver. + * oclif's `findCommand` walks the command table and `import()`s every command + * module on **every** CLI invocation, so a broken import chain anywhere in that + * table is charged to whatever command you actually ran. `schema-migrate.ts` is + * shared by nine commands (`meta:resync`, `migrate`, and seven `migrate:*`), and + * its one static `import { isInPlaceSchemaWork } from '@objectstack/driver-sql'` + * meant that an unbuilt `packages/drivers/driver-sql/dist` made `os dev` print + * nine `MODULE_NOT_FOUND` blocks (eighteen — `dev` forks a child) naming nine + * commands the operator never invoked, and made all nine vanish from the command + * table: `os migrate plan` answered `Command migrate:plan not found.` The real + * cause was `pnpm build`, which nothing in that output said. + * + * These are source-level assertions on purpose. The defect lives in the shape of + * the import graph, which is decided at authoring time and is invisible to any + * test that merely calls the functions — every behavioural test in this package + * runs in a workspace where the driver happens to be built. + * + * Scanning `src` rather than `dist` is the same choice: `dist` is what oclif + * loads, but building it inside a unit test would be far slower than the thing + * it guards, and `tsc` does not move imports between the two forms — a static + * value import in `src` is a static import in `dist`, and an `await import()` + * stays dynamic. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** `packages/cli/src` — this file lives in `src/utils/`. */ +const SRC_ROOT = fileURLToPath(new URL('..', import.meta.url)); + +/** Every production `.ts` under `packages/cli/src`, as `[relativePath, source]`. */ +function productionSources(): Array<[string, string]> { + return readdirSync(SRC_ROOT, { recursive: true, encoding: 'utf8' }) + .filter((rel) => rel.endsWith('.ts') && !rel.endsWith('.d.ts')) + .filter((rel) => !/\.(test|spec)\.ts$/.test(rel)) + .map((rel) => [rel, readFileSync(join(SRC_ROOT, rel), 'utf8')] as [string, string]); +} + +/** + * Static `import … from ''` statements, with the leading `type` + * keyword captured when present. + * + * `[^;]*?` cannot cross a statement terminator, so a multi-line import clause is + * matched whole while two adjacent statements can never be spliced together. + */ +const STATIC_IMPORT = /^[ \t]*import[ \t]+(?:(type)[ \t]+)?([^;]*?)[ \t]*from[ \t]*['"]([^'"]+)['"]/gm; + +/** Bare side-effect imports — `import '';` — which also load the module. */ +const SIDE_EFFECT_IMPORT = /^[ \t]*import[ \t]*['"]([^'"]+)['"]/gm; + +const DRIVER_PACKAGE = /^@objectstack\/driver-/; + +describe('#5726 — CLI command modules must not statically value-import a driver', () => { + it('has no static value import of any @objectstack/driver-* package in production sources', () => { + const offenders: string[] = []; + + for (const [rel, src] of productionSources()) { + for (const m of src.matchAll(STATIC_IMPORT)) { + const [, typeKeyword, clause, specifier] = m; + if (!DRIVER_PACKAGE.test(specifier)) continue; + // `import type { … } from` erases entirely — no runtime edge, no cost. + if (typeKeyword) continue; + // A clause of inline `type` specifiers still emits the module under + // `verbatimModuleSyntax`. Flagged deliberately: the fix (hoisting the + // keyword to `import type`) is trivial and always available, so there is + // no reason to let the risky spelling through on a technicality. + offenders.push(`${rel}: import ${clause.trim()} from '${specifier}'`); + } + for (const m of src.matchAll(SIDE_EFFECT_IMPORT)) { + if (DRIVER_PACKAGE.test(m[1])) offenders.push(`${rel}: import '${m[1]}'`); + } + } + + expect( + offenders, + 'A static value import of a driver package makes EVERY command that reaches this module ' + + 'fail oclif command discovery when the driver is not built, and prints one MODULE_NOT_FOUND ' + + 'block per such command in front of whatever the operator actually ran (#5726). ' + + 'Use `await import(\'@objectstack/driver-…\')` at the point of use, or `import type` when ' + + 'only the types are needed.', + ).toEqual([]); + }); + + it('still reaches the driver for the in-place classifier, lazily — one definition, loaded later', () => { + const src = readFileSync(join(SRC_ROOT, 'utils/schema-migrate.ts'), 'utf8'); + + // The point of the fix is NOT "stop depending on driver-sql". The + // additive/in-place split is a fact about `PendingSchemaWorkKind`, declared + // beside that union in the driver; re-deriving it here would let the CLI + // disagree with the driver the day a kind is added — by listing a row + // rewrite under a heading that promises the work is never data-losing + // (#3954). Pin that the dependency survives, in its dynamic form. + expect(src).toMatch(/await import\((['"])@objectstack\/driver-sql\1\)/); + expect(src).toContain('isInPlaceSchemaWork'); + }); + + it('awaits every call of the now-async pending-work renderers', () => { + // `renderPendingSchemaWork` returns `Promise`, so a dropped `await` is + // not a type error — it is output that races the process exit. (The repo + // already carries an eslint rule for exactly this shape on `formatOutput`.) + const CALL = /(\bawait\s+|\bfunction\s+|\.)?\b(renderPendingSchemaWork|summarizePendingSchemaWork)\s*\(/g; + const unawaited: string[] = []; + + for (const [rel, src] of productionSources()) { + for (const m of src.matchAll(CALL)) { + const prefix = m[1] ?? ''; + if (/^function\s+$/.test(prefix)) continue; // the declaration itself + if (/^await\s+$/.test(prefix)) continue; + unawaited.push(`${rel}: ${m[0].trim()}`); + } + } + + expect(unawaited, 'These renderers became async in #5726 — await them.').toEqual([]); + }); +}); diff --git a/packages/cli/src/utils/schema-migrate.pending-render.test.ts b/packages/cli/src/utils/schema-migrate.pending-render.test.ts index 3fd6fe5226..555a13f907 100644 --- a/packages/cli/src/utils/schema-migrate.pending-render.test.ts +++ b/packages/cli/src/utils/schema-migrate.pending-render.test.ts @@ -46,13 +46,13 @@ const IN_PLACE: PendingSchemaWork[] = [ ]; describe('renderPendingSchemaWork (#3954)', () => { - it('renders nothing at all when there is nothing pending', () => { - renderPendingSchemaWork([]); + it('renders nothing at all when there is nothing pending', async () => { + await renderPendingSchemaWork([]); expect(out()).toBe(''); }); - it('keeps the additive section exactly as it was when only additive work is pending', () => { - renderPendingSchemaWork(ADDITIVE); + it('keeps the additive section exactly as it was when only additive work is pending', async () => { + await renderPendingSchemaWork(ADDITIVE); expect(out()).toContain('New (additive — created when you apply)'); expect(out()).toContain('widgets'); expect(out()).toContain('[create_table, 2 column(s)]'); @@ -61,16 +61,16 @@ describe('renderPendingSchemaWork (#3954)', () => { expect(out()).not.toContain('In place'); }); - it('puts the datetime convergence under its OWN heading, not the additive one', () => { - renderPendingSchemaWork(IN_PLACE); + it('puts the datetime convergence under its OWN heading, not the additive one', async () => { + await renderPendingSchemaWork(IN_PLACE); expect(out()).toContain('In place (existing rows converged when you apply)'); // The additive heading claims the work is never data-losing; a row rewrite // must never be listed beneath it. expect(out()).not.toContain('New (additive'); }); - it('names the columns and the size of each in-place step', () => { - renderPendingSchemaWork(IN_PLACE); + it('names the columns and the size of each in-place step', async () => { + await renderPendingSchemaWork(IN_PLACE); expect(out()).toContain('normalize_datetime_storage: at'); expect(out()).toContain('1,234,567 row update(s)'); expect(out()).toContain('widen_datetime_columns: at, created_at'); @@ -83,30 +83,30 @@ describe('renderPendingSchemaWork (#3954)', () => { expect(out()).toContain('9 row table rebuild'); }); - it('shows both sections when both kinds are pending', () => { - renderPendingSchemaWork([...ADDITIVE, ...IN_PLACE]); + it('shows both sections when both kinds are pending', async () => { + await renderPendingSchemaWork([...ADDITIVE, ...IN_PLACE]); expect(out()).toContain('New (additive — created when you apply)'); expect(out()).toContain('In place (existing rows converged when you apply)'); }); - it('reads an unmeasured count as unknown rather than zero', () => { - renderPendingSchemaWork([{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'] }]); + it('reads an unmeasured count as unknown rather than zero', async () => { + await renderPendingSchemaWork([{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'] }]); expect(out()).toContain('? row update(s)'); expect(out()).not.toContain('0 row update(s)'); }); }); describe('summarizePendingSchemaWork (#3954)', () => { - it('is unchanged for purely additive work', () => { - expect(summarizePendingSchemaWork(ADDITIVE)).toBe('1 table(s) to create, 1 column(s) to add'); + it('is unchanged for purely additive work', async () => { + expect(await summarizePendingSchemaWork(ADDITIVE)).toBe('1 table(s) to create, 1 column(s) to add'); }); - it('is unchanged when nothing is pending', () => { - expect(summarizePendingSchemaWork([])).toBe('0 table(s) to create, 0 column(s) to add'); + it('is unchanged when nothing is pending', async () => { + expect(await summarizePendingSchemaWork([])).toBe('0 table(s) to create, 0 column(s) to add'); }); - it('never omits in-place work — this is the line read before confirming', () => { - const summary = summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]); + it('never omits in-place work — this is the line read before confirming', async () => { + const summary = await summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]); expect(summary).toContain('1 table(s) to create'); expect(summary).toContain('1 column(s) to add'); expect(summary).toContain('5 temporal column(s) to converge in place'); diff --git a/packages/cli/src/utils/schema-migrate.ts b/packages/cli/src/utils/schema-migrate.ts index a9fef33cd5..403ed60e00 100644 --- a/packages/cli/src/utils/schema-migrate.ts +++ b/packages/cli/src/utils/schema-migrate.ts @@ -14,7 +14,6 @@ */ import chalk from 'chalk'; import type { ManagedDriftEntry, DriftCategory, PendingSchemaWork } from '@objectstack/driver-sql'; -import { isInPlaceSchemaWork } from '@objectstack/driver-sql'; import type { IObjectQLEngine } from '@objectstack/spec/contracts'; import { describeDriverConnection } from './connection-display.js'; @@ -269,6 +268,42 @@ export async function bootSchemaStack( // ── Rendering ─────────────────────────────────────────────────────── +/** + * Load the driver's additive/in-place classifier at the moment it is used, + * rather than when this module is loaded (#5726). + * + * `isInPlaceSchemaWork` is the ONLY thing this module needs from + * `@objectstack/driver-sql` at runtime — everything else it takes from that + * package is `import type`, which erases. A *static* value import for it was + * not a local cost, because this file is not loaded only when someone migrates: + * oclif's `findCommand` `import()`s every command module on **every** CLI + * invocation, and nine commands reach this file (`meta:resync`, `migrate`, and + * seven `migrate:*`). So an unbuilt `packages/drivers/driver-sql/dist` did not + * merely break those nine — running *any* command, `os dev` included, printed + * one `MODULE_NOT_FOUND` block per command in front of the output you asked for + * (and `os dev` forks a child, so you saw each one twice), while the nine + * dropped out of the command table entirely: `os migrate plan` answered + * `Command migrate:plan not found.` None of that noise named the real cause + * (`pnpm build`) and the one actionable line it ended on pointed elsewhere. + * + * Deliberately a lazy import of the driver's own predicate rather than a copy + * of it here. The additive/in-place split is a fact about + * `PendingSchemaWorkKind`, declared next to that union in the driver; a second + * copy in the CLI would be free to disagree the day a kind is added — and the + * way it would disagree is by listing a row rewrite under a heading that + * promises the work is never data-losing (#3954). One definition, loaded later. + * + * By the time either renderer runs, the caller is holding a live SQL driver + * (the entries it renders came from `previewDeferredSchemaWork()`), so the + * module is already in the loader cache and this costs nothing. It is + * deliberately not wrapped in a `try`: if it ever did fail, rendering must fail + * loudly rather than fall back to a guess about which work rewrites data. + */ +async function loadIsInPlaceSchemaWork(): Promise<(kind: PendingSchemaWork['kind']) => boolean> { + const { isInPlaceSchemaWork } = await import('@objectstack/driver-sql'); + return isInPlaceSchemaWork; +} + const CATEGORY_ORDER: DriftCategory[] = ['safe', 'needs_confirm', 'destructive']; const CATEGORY_META: Record string; icon: string }> = { @@ -328,9 +363,10 @@ export function summarize(drift: ManagedDriftEntry[]): string { * get their own heading, and their row counts, because "how long will this hold * the table" is the question they raise and the additive kinds do not. */ -export function renderPendingSchemaWork(pending: PendingSchemaWork[]): void { +export async function renderPendingSchemaWork(pending: PendingSchemaWork[]): Promise { if (pending.length === 0) return; + const isInPlaceSchemaWork = await loadIsInPlaceSchemaWork(); const additive = pending.filter((p) => !isInPlaceSchemaWork(p.kind)); const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind)); @@ -367,7 +403,7 @@ function formatRows(rows: number | undefined): string { return rows === undefined ? '?' : rows.toLocaleString('en-US'); } -export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string { +export async function summarizePendingSchemaWork(pending: PendingSchemaWork[]): Promise { const creates = pending.filter((p) => p.kind === 'create_table').length; const columns = pending .filter((p) => p.kind === 'add_columns') @@ -376,6 +412,7 @@ export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string // Only mentioned when there is some, so the common in-sync summary is // unchanged — but never omitted when there is, which is the #3954 point. + const isInPlaceSchemaWork = await loadIsInPlaceSchemaWork(); const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind)); if (inPlace.length > 0) { const cols = inPlace.reduce((n, p) => n + p.columns.length, 0);