Skip to content

Commit 0453d4c

Browse files
committed
fix(cli): load driver-sql's schema-work classifier lazily (#5726)
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWUR56YsttL5sTF72Q75TQ
1 parent c001422 commit 0453d4c

6 files changed

Lines changed: 210 additions & 26 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
CLI: load the SQL driver's schema-work classifier lazily, so an unbuilt driver no longer breaks command discovery (#5726)
6+
7+
`packages/cli/src/utils/schema-migrate.ts` statically value-imported
8+
`isInPlaceSchemaWork` from `@objectstack/driver-sql`. oclif's `findCommand`
9+
`import()`s every command module on every CLI invocation, and nine commands
10+
reach that file (`meta:resync`, `migrate`, and seven `migrate:*`), so a
11+
workspace whose `packages/drivers/driver-sql/dist` was not built printed nine
12+
`MODULE_NOT_FOUND` blocks — naming nine commands the operator never invoked —
13+
in front of whatever command they actually ran, and dropped all nine out of the
14+
command table (`os migrate plan` answered `Command migrate:plan not found.`).
15+
16+
The import is now `await import('@objectstack/driver-sql')` at the point of use,
17+
inside the two renderers that need the classifier. The classifier keeps its one
18+
definition in the driver — it is a fact about `PendingSchemaWorkKind` and a copy
19+
in the CLI could disagree, listing a row rewrite under the heading that promises
20+
the work is never data-losing.
21+
22+
No user-visible behaviour change: this is local/worktree developer experience
23+
only, and CI always builds before running the CLI. `renderPendingSchemaWork` and
24+
`summarizePendingSchemaWork` — internal helpers, not part of the package's
25+
public entry — are now `async`.

packages/cli/src/commands/migrate/apply.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,9 +161,9 @@ export default class MigrateApply extends Command {
161161
if (!flags.json) {
162162
printInfo(`Database: ${chalk.white(stack.dbLabel)}`);
163163
console.log('');
164-
renderPendingSchemaWork(pending);
164+
await renderPendingSchemaWork(pending);
165165
renderPlan(drift);
166-
if (pending.length > 0) printInfo(summarizePendingSchemaWork(pending));
166+
if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending));
167167
printInfo(summarize(drift));
168168
if (deferred.length > 0) {
169169
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 {
211211

212212
console.log('');
213213
if (created.length > 0) {
214-
printSuccess(`Created/extended ${created.length} table(s): ${summarizePendingSchemaWork(created)}.`);
214+
printSuccess(`Created/extended ${created.length} table(s): ${await summarizePendingSchemaWork(created)}.`);
215215
}
216216
printSuccess(`Applied ${applied.length} change(s).`);
217217
if (skipped.length > 0) {

packages/cli/src/commands/migrate/plan.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,9 +158,9 @@ export default class MigratePlan extends Command {
158158
return;
159159
}
160160

161-
renderPendingSchemaWork(pending);
161+
await renderPendingSchemaWork(pending);
162162
renderPlan(drift);
163-
if (pending.length > 0) printInfo(summarizePendingSchemaWork(pending));
163+
if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending));
164164
printInfo(summarize(drift));
165165
console.log(chalk.dim(' Apply with: ') + chalk.white('os migrate apply') +
166166
chalk.dim(' (add --allow-destructive for drops / tightenings)'));
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #5726 — no CLI production module may STATICALLY value-import a driver.
5+
*
6+
* The cost of one such import is not paid by the command that needs the driver.
7+
* oclif's `findCommand` walks the command table and `import()`s every command
8+
* module on **every** CLI invocation, so a broken import chain anywhere in that
9+
* table is charged to whatever command you actually ran. `schema-migrate.ts` is
10+
* shared by nine commands (`meta:resync`, `migrate`, and seven `migrate:*`), and
11+
* its one static `import { isInPlaceSchemaWork } from '@objectstack/driver-sql'`
12+
* meant that an unbuilt `packages/drivers/driver-sql/dist` made `os dev` print
13+
* nine `MODULE_NOT_FOUND` blocks (eighteen — `dev` forks a child) naming nine
14+
* commands the operator never invoked, and made all nine vanish from the command
15+
* table: `os migrate plan` answered `Command migrate:plan not found.` The real
16+
* cause was `pnpm build`, which nothing in that output said.
17+
*
18+
* These are source-level assertions on purpose. The defect lives in the shape of
19+
* the import graph, which is decided at authoring time and is invisible to any
20+
* test that merely calls the functions — every behavioural test in this package
21+
* runs in a workspace where the driver happens to be built.
22+
*
23+
* Scanning `src` rather than `dist` is the same choice: `dist` is what oclif
24+
* loads, but building it inside a unit test would be far slower than the thing
25+
* it guards, and `tsc` does not move imports between the two forms — a static
26+
* value import in `src` is a static import in `dist`, and an `await import()`
27+
* stays dynamic.
28+
*/
29+
30+
import { describe, it, expect } from 'vitest';
31+
import { readdirSync, readFileSync } from 'node:fs';
32+
import { join } from 'node:path';
33+
import { fileURLToPath } from 'node:url';
34+
35+
/** `packages/cli/src` — this file lives in `src/utils/`. */
36+
const SRC_ROOT = fileURLToPath(new URL('..', import.meta.url));
37+
38+
/** Every production `.ts` under `packages/cli/src`, as `[relativePath, source]`. */
39+
function productionSources(): Array<[string, string]> {
40+
return readdirSync(SRC_ROOT, { recursive: true, encoding: 'utf8' })
41+
.filter((rel) => rel.endsWith('.ts') && !rel.endsWith('.d.ts'))
42+
.filter((rel) => !/\.(test|spec)\.ts$/.test(rel))
43+
.map((rel) => [rel, readFileSync(join(SRC_ROOT, rel), 'utf8')] as [string, string]);
44+
}
45+
46+
/**
47+
* Static `import … from '<specifier>'` statements, with the leading `type`
48+
* keyword captured when present.
49+
*
50+
* `[^;]*?` cannot cross a statement terminator, so a multi-line import clause is
51+
* matched whole while two adjacent statements can never be spliced together.
52+
*/
53+
const STATIC_IMPORT = /^[ \t]*import[ \t]+(?:(type)[ \t]+)?([^;]*?)[ \t]*from[ \t]*['"]([^'"]+)['"]/gm;
54+
55+
/** Bare side-effect imports — `import '<specifier>';` — which also load the module. */
56+
const SIDE_EFFECT_IMPORT = /^[ \t]*import[ \t]*['"]([^'"]+)['"]/gm;
57+
58+
const DRIVER_PACKAGE = /^@objectstack\/driver-/;
59+
60+
describe('#5726 — CLI command modules must not statically value-import a driver', () => {
61+
it('has no static value import of any @objectstack/driver-* package in production sources', () => {
62+
const offenders: string[] = [];
63+
64+
for (const [rel, src] of productionSources()) {
65+
for (const m of src.matchAll(STATIC_IMPORT)) {
66+
const [, typeKeyword, clause, specifier] = m;
67+
if (!DRIVER_PACKAGE.test(specifier)) continue;
68+
// `import type { … } from` erases entirely — no runtime edge, no cost.
69+
if (typeKeyword) continue;
70+
// A clause of inline `type` specifiers still emits the module under
71+
// `verbatimModuleSyntax`. Flagged deliberately: the fix (hoisting the
72+
// keyword to `import type`) is trivial and always available, so there is
73+
// no reason to let the risky spelling through on a technicality.
74+
offenders.push(`${rel}: import ${clause.trim()} from '${specifier}'`);
75+
}
76+
for (const m of src.matchAll(SIDE_EFFECT_IMPORT)) {
77+
if (DRIVER_PACKAGE.test(m[1])) offenders.push(`${rel}: import '${m[1]}'`);
78+
}
79+
}
80+
81+
expect(
82+
offenders,
83+
'A static value import of a driver package makes EVERY command that reaches this module ' +
84+
'fail oclif command discovery when the driver is not built, and prints one MODULE_NOT_FOUND ' +
85+
'block per such command in front of whatever the operator actually ran (#5726). ' +
86+
'Use `await import(\'@objectstack/driver-…\')` at the point of use, or `import type` when ' +
87+
'only the types are needed.',
88+
).toEqual([]);
89+
});
90+
91+
it('still reaches the driver for the in-place classifier, lazily — one definition, loaded later', () => {
92+
const src = readFileSync(join(SRC_ROOT, 'utils/schema-migrate.ts'), 'utf8');
93+
94+
// The point of the fix is NOT "stop depending on driver-sql". The
95+
// additive/in-place split is a fact about `PendingSchemaWorkKind`, declared
96+
// beside that union in the driver; re-deriving it here would let the CLI
97+
// disagree with the driver the day a kind is added — by listing a row
98+
// rewrite under a heading that promises the work is never data-losing
99+
// (#3954). Pin that the dependency survives, in its dynamic form.
100+
expect(src).toMatch(/await import\((['"])@objectstack\/driver-sql\1\)/);
101+
expect(src).toContain('isInPlaceSchemaWork');
102+
});
103+
104+
it('awaits every call of the now-async pending-work renderers', () => {
105+
// `renderPendingSchemaWork` returns `Promise<void>`, so a dropped `await` is
106+
// not a type error — it is output that races the process exit. (The repo
107+
// already carries an eslint rule for exactly this shape on `formatOutput`.)
108+
const CALL = /(\bawait\s+|\bfunction\s+|\.)?\b(renderPendingSchemaWork|summarizePendingSchemaWork)\s*\(/g;
109+
const unawaited: string[] = [];
110+
111+
for (const [rel, src] of productionSources()) {
112+
for (const m of src.matchAll(CALL)) {
113+
const prefix = m[1] ?? '';
114+
if (/^function\s+$/.test(prefix)) continue; // the declaration itself
115+
if (/^await\s+$/.test(prefix)) continue;
116+
unawaited.push(`${rel}: ${m[0].trim()}`);
117+
}
118+
}
119+
120+
expect(unawaited, 'These renderers became async in #5726 — await them.').toEqual([]);
121+
});
122+
});

packages/cli/src/utils/schema-migrate.pending-render.test.ts

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,13 @@ const IN_PLACE: PendingSchemaWork[] = [
4646
];
4747

4848
describe('renderPendingSchemaWork (#3954)', () => {
49-
it('renders nothing at all when there is nothing pending', () => {
50-
renderPendingSchemaWork([]);
49+
it('renders nothing at all when there is nothing pending', async () => {
50+
await renderPendingSchemaWork([]);
5151
expect(out()).toBe('');
5252
});
5353

54-
it('keeps the additive section exactly as it was when only additive work is pending', () => {
55-
renderPendingSchemaWork(ADDITIVE);
54+
it('keeps the additive section exactly as it was when only additive work is pending', async () => {
55+
await renderPendingSchemaWork(ADDITIVE);
5656
expect(out()).toContain('New (additive — created when you apply)');
5757
expect(out()).toContain('widgets');
5858
expect(out()).toContain('[create_table, 2 column(s)]');
@@ -61,16 +61,16 @@ describe('renderPendingSchemaWork (#3954)', () => {
6161
expect(out()).not.toContain('In place');
6262
});
6363

64-
it('puts the datetime convergence under its OWN heading, not the additive one', () => {
65-
renderPendingSchemaWork(IN_PLACE);
64+
it('puts the datetime convergence under its OWN heading, not the additive one', async () => {
65+
await renderPendingSchemaWork(IN_PLACE);
6666
expect(out()).toContain('In place (existing rows converged when you apply)');
6767
// The additive heading claims the work is never data-losing; a row rewrite
6868
// must never be listed beneath it.
6969
expect(out()).not.toContain('New (additive');
7070
});
7171

72-
it('names the columns and the size of each in-place step', () => {
73-
renderPendingSchemaWork(IN_PLACE);
72+
it('names the columns and the size of each in-place step', async () => {
73+
await renderPendingSchemaWork(IN_PLACE);
7474
expect(out()).toContain('normalize_datetime_storage: at');
7575
expect(out()).toContain('1,234,567 row update(s)');
7676
expect(out()).toContain('widen_datetime_columns: at, created_at');
@@ -83,30 +83,30 @@ describe('renderPendingSchemaWork (#3954)', () => {
8383
expect(out()).toContain('9 row table rebuild');
8484
});
8585

86-
it('shows both sections when both kinds are pending', () => {
87-
renderPendingSchemaWork([...ADDITIVE, ...IN_PLACE]);
86+
it('shows both sections when both kinds are pending', async () => {
87+
await renderPendingSchemaWork([...ADDITIVE, ...IN_PLACE]);
8888
expect(out()).toContain('New (additive — created when you apply)');
8989
expect(out()).toContain('In place (existing rows converged when you apply)');
9090
});
9191

92-
it('reads an unmeasured count as unknown rather than zero', () => {
93-
renderPendingSchemaWork([{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'] }]);
92+
it('reads an unmeasured count as unknown rather than zero', async () => {
93+
await renderPendingSchemaWork([{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'] }]);
9494
expect(out()).toContain('? row update(s)');
9595
expect(out()).not.toContain('0 row update(s)');
9696
});
9797
});
9898

9999
describe('summarizePendingSchemaWork (#3954)', () => {
100-
it('is unchanged for purely additive work', () => {
101-
expect(summarizePendingSchemaWork(ADDITIVE)).toBe('1 table(s) to create, 1 column(s) to add');
100+
it('is unchanged for purely additive work', async () => {
101+
expect(await summarizePendingSchemaWork(ADDITIVE)).toBe('1 table(s) to create, 1 column(s) to add');
102102
});
103103

104-
it('is unchanged when nothing is pending', () => {
105-
expect(summarizePendingSchemaWork([])).toBe('0 table(s) to create, 0 column(s) to add');
104+
it('is unchanged when nothing is pending', async () => {
105+
expect(await summarizePendingSchemaWork([])).toBe('0 table(s) to create, 0 column(s) to add');
106106
});
107107

108-
it('never omits in-place work — this is the line read before confirming', () => {
109-
const summary = summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]);
108+
it('never omits in-place work — this is the line read before confirming', async () => {
109+
const summary = await summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]);
110110
expect(summary).toContain('1 table(s) to create');
111111
expect(summary).toContain('1 column(s) to add');
112112
expect(summary).toContain('5 temporal column(s) to converge in place');

packages/cli/src/utils/schema-migrate.ts

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
*/
1515
import chalk from 'chalk';
1616
import type { ManagedDriftEntry, DriftCategory, PendingSchemaWork } from '@objectstack/driver-sql';
17-
import { isInPlaceSchemaWork } from '@objectstack/driver-sql';
1817
import type { IObjectQLEngine } from '@objectstack/spec/contracts';
1918
import { describeDriverConnection } from './connection-display.js';
2019

@@ -269,6 +268,42 @@ export async function bootSchemaStack(
269268

270269
// ── Rendering ───────────────────────────────────────────────────────
271270

271+
/**
272+
* Load the driver's additive/in-place classifier at the moment it is used,
273+
* rather than when this module is loaded (#5726).
274+
*
275+
* `isInPlaceSchemaWork` is the ONLY thing this module needs from
276+
* `@objectstack/driver-sql` at runtime — everything else it takes from that
277+
* package is `import type`, which erases. A *static* value import for it was
278+
* not a local cost, because this file is not loaded only when someone migrates:
279+
* oclif's `findCommand` `import()`s every command module on **every** CLI
280+
* invocation, and nine commands reach this file (`meta:resync`, `migrate`, and
281+
* seven `migrate:*`). So an unbuilt `packages/drivers/driver-sql/dist` did not
282+
* merely break those nine — running *any* command, `os dev` included, printed
283+
* one `MODULE_NOT_FOUND` block per command in front of the output you asked for
284+
* (and `os dev` forks a child, so you saw each one twice), while the nine
285+
* dropped out of the command table entirely: `os migrate plan` answered
286+
* `Command migrate:plan not found.` None of that noise named the real cause
287+
* (`pnpm build`) and the one actionable line it ended on pointed elsewhere.
288+
*
289+
* Deliberately a lazy import of the driver's own predicate rather than a copy
290+
* of it here. The additive/in-place split is a fact about
291+
* `PendingSchemaWorkKind`, declared next to that union in the driver; a second
292+
* copy in the CLI would be free to disagree the day a kind is added — and the
293+
* way it would disagree is by listing a row rewrite under a heading that
294+
* promises the work is never data-losing (#3954). One definition, loaded later.
295+
*
296+
* By the time either renderer runs, the caller is holding a live SQL driver
297+
* (the entries it renders came from `previewDeferredSchemaWork()`), so the
298+
* module is already in the loader cache and this costs nothing. It is
299+
* deliberately not wrapped in a `try`: if it ever did fail, rendering must fail
300+
* loudly rather than fall back to a guess about which work rewrites data.
301+
*/
302+
async function loadIsInPlaceSchemaWork(): Promise<(kind: PendingSchemaWork['kind']) => boolean> {
303+
const { isInPlaceSchemaWork } = await import('@objectstack/driver-sql');
304+
return isInPlaceSchemaWork;
305+
}
306+
272307
const CATEGORY_ORDER: DriftCategory[] = ['safe', 'needs_confirm', 'destructive'];
273308

274309
const CATEGORY_META: Record<DriftCategory, { label: string; color: (s: string) => string; icon: string }> = {
@@ -328,9 +363,10 @@ export function summarize(drift: ManagedDriftEntry[]): string {
328363
* get their own heading, and their row counts, because "how long will this hold
329364
* the table" is the question they raise and the additive kinds do not.
330365
*/
331-
export function renderPendingSchemaWork(pending: PendingSchemaWork[]): void {
366+
export async function renderPendingSchemaWork(pending: PendingSchemaWork[]): Promise<void> {
332367
if (pending.length === 0) return;
333368

369+
const isInPlaceSchemaWork = await loadIsInPlaceSchemaWork();
334370
const additive = pending.filter((p) => !isInPlaceSchemaWork(p.kind));
335371
const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind));
336372

@@ -367,7 +403,7 @@ function formatRows(rows: number | undefined): string {
367403
return rows === undefined ? '?' : rows.toLocaleString('en-US');
368404
}
369405

370-
export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string {
406+
export async function summarizePendingSchemaWork(pending: PendingSchemaWork[]): Promise<string> {
371407
const creates = pending.filter((p) => p.kind === 'create_table').length;
372408
const columns = pending
373409
.filter((p) => p.kind === 'add_columns')
@@ -376,6 +412,7 @@ export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string
376412

377413
// Only mentioned when there is some, so the common in-sync summary is
378414
// unchanged — but never omitted when there is, which is the #3954 point.
415+
const isInPlaceSchemaWork = await loadIsInPlaceSchemaWork();
379416
const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind));
380417
if (inPlace.length > 0) {
381418
const cols = inPlace.reduce((n, p) => n + p.columns.length, 0);

0 commit comments

Comments
 (0)