From 6901c2bd682e0570c3e58aa1474d9653910e0436 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 15:44:18 +0000 Subject: [PATCH 1/3] feat(cli): os migrate plan reports unmanaged platform-namespaced tables (#13204) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k --- .../migrate-plan-unmanaged-table-findings.md | 48 +++ packages/cli/src/commands/migrate/plan.ts | 44 ++ .../unmanaged-tables.integration.test.ts | 189 +++++++++ .../commands/migrate/unmanaged-tables.test.ts | 361 ++++++++++++++++ .../src/commands/migrate/unmanaged-tables.ts | 394 ++++++++++++++++++ 5 files changed, 1036 insertions(+) create mode 100644 .changeset/migrate-plan-unmanaged-table-findings.md create mode 100644 packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts create mode 100644 packages/cli/src/commands/migrate/unmanaged-tables.test.ts create mode 100644 packages/cli/src/commands/migrate/unmanaged-tables.ts diff --git a/.changeset/migrate-plan-unmanaged-table-findings.md b/.changeset/migrate-plan-unmanaged-table-findings.md new file mode 100644 index 0000000000..7a9055d24d --- /dev/null +++ b/.changeset/migrate-plan-unmanaged-table-findings.md @@ -0,0 +1,48 @@ +--- +"@objectstack/cli": minor +--- + +feat(cli): `os migrate plan` reports the platform-namespaced tables no declaration accounts for (#13204) + +`detectManagedDrift()` diffs the tables metadata DECLARES against the physical +database, so a table nothing declares is not in its input and no plan can ever +mention it. Every object retirement therefore strands its table forever, and +the plan reads clean while it sits there — measured on ObjectStack Cloud's +control DB, where a retired `sys_scim_provider` is still present (0 rows) and +has never appeared in a plan. + +`os migrate plan` now sweeps the physical table catalog once per run (one +`sqlite_master` / `information_schema.tables` SELECT against the same driver it +diffed) and reports the BASE TABLES that carry a reserved platform namespace +prefix — `PLATFORM_OBJECT_PREFIXES`, i.e. `sys_` / `cloud_` / `ai_` — and that +no declaration accounts for. Human mode prints a block only when there is +something to report; `--json` grows an `unmanagedTables` key that is **always +present** once a SQL driver was found, so a consumer can tell "swept, everything +is declared" from "never swept". + +⛔ **Nothing is dropped, and no drop is proposed.** The section names tables and +stops. Removing an existing physical table is destructive and hard to reverse; +that decision stays with a human and this change does not introduce it. + +⛔ **It is not `composition.coverage`.** Coverage says what the plan EXAMINED of +what the deployment declares; this says what EXISTS that no declaration +accounts for. A declared-but-unexamined object's table is deliberately excluded +here — coverage already reports it, and folding the two together would make +"examined and clean" indistinguishable from "never looked at". + +**Why the predicate is not a bare `sys_` prefix scan.** Three physical-table +families carry a platform prefix while being legitimate, and a difference +against the managed set alone reports all three: rotation shards +(`sys_activity` declares `strategy: 'rotation'` with 14 daily shards, and only +the BASE name is ever a `managedObjectFields` key — up to 14 false rows on +every plan of every `plugin-audit` deployment); declared-but-unexamined objects +(~72 of them on the control plane #13028 measured); and driver-internal tables. +Shards are folded onto their base before the membership test, the declared set +is unioned into the managed set, and the driver's own scratch tables +(`_objectstack_sequences`, `__os_mig_*`) carry no reserved prefix. + +Every path that fails to obtain an answer reports `status: 'unreadable'` with a +reason — a missing seam, an unrecognised dialect, a catalog read that threw, a +seam that returns no result set — and never an empty list. Non-SQL drivers are +unaffected: `plan` already returns before this point when no SQL driver is +active. diff --git a/packages/cli/src/commands/migrate/plan.ts b/packages/cli/src/commands/migrate/plan.ts index 2c19a88b87..59af2be708 100644 --- a/packages/cli/src/commands/migrate/plan.ts +++ b/packages/cli/src/commands/migrate/plan.ts @@ -25,6 +25,11 @@ import { type SchemaMigrationComposition, } from '../../utils/schema-migration-plugins.js'; import { probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js'; +import { + collectUnmanagedTables, + renderUnmanagedTables, + type UnmanagedTablesReport, +} from './unmanaged-tables.js'; import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; import { resolveTenancyPosture, @@ -165,6 +170,26 @@ export default class MigratePlan extends Command { const drift = await stack.driver.detectManagedDrift(); const pending = stack.pendingSchemaWork; + // [#13204] What EXISTS that nothing declares. Deliberately computed + // BEFORE the "in sync" early return below: a stranded table is exactly + // the finding a drift-free plan is otherwise structurally blind to, and + // retiring an object is how one gets there. + // + // ⛔ Informational only. It proposes no drop and reaches no DDL path — + // dropping an existing physical table is destructive and hard to + // reverse, so that decision stays with a human. + // + // ⛔ And it is NOT `composition.coverage`: coverage says what this plan + // EXAMINED of what the deployment declares, this says what exists that + // no declaration accounts for. Merged, "examined and clean" would be + // indistinguishable from "never looked at". See `./unmanaged-tables.ts`. + const { normalizeRows } = await import('@objectstack/metadata-protocol'); + const unmanagedTables: UnmanagedTablesReport = await collectUnmanagedTables({ + driver: stack.driver, + declaredObjects: stack.allObjects(), + normalize: normalizeRows, + }); + // [ADR-0120 D5e, advisory form] Installation-wide uniques on app objects // are a decision point under the `isolated` posture — organizations there // are separate CUSTOMERS. The HARD gate runs at app install; this covers @@ -182,6 +207,13 @@ export default class MigratePlan extends Command { total: drift.length, changes: drift, pending, + // [#13204] Always present once a SQL driver was found, including + // when the sweep found nothing (`tables: []`) and when it could not + // run (`status: 'unreadable'`). A consumer must be able to tell + // "swept, everything is declared" from "never swept" — omitting the + // key on the clean case would make those two byte-identical, which + // is the blind spot this section exists to remove. + unmanagedTables, ...(uniqueScopeAdvisory.length > 0 ? { uniqueScopeAdvisory: { @@ -247,6 +279,18 @@ export default class MigratePlan extends Command { for (const note of stack.composition.notes) console.log(chalk.dim(` ${note}`)); console.log(''); + // [#13204] Its own block, above the drift verdict and never folded into + // the "Examined N managed table(s)" line — it is a statement about a + // DIFFERENT population (what exists) than every other line here (what is + // declared). Silent when the sweep ran and found nothing; loud when it + // could not run, because "did not look" must never read as "found none". + const unmanagedLines = renderUnmanagedTables(unmanagedTables); + if (unmanagedLines.length > 0) { + printInfo(unmanagedLines[0]!); + for (const line of unmanagedLines.slice(1)) console.log(chalk.dim(` ${line}`)); + console.log(''); + } + if (drift.length === 0 && pending.length === 0) { // [#13028] "In sync" is a claim about the objects this plan EXAMINED. // On a composed host that examined a strict subset — a control plane diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts b/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts new file mode 100644 index 0000000000..dd14246193 --- /dev/null +++ b/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts @@ -0,0 +1,189 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13204 — the unmanaged-table sweep against a REAL booted stack and a REAL + * database file, in both directions. + * + * `unmanaged-tables.test.ts` pins the predicate over hand-built sets. What it + * cannot pin is the two joins to the rest of the system, and both are exactly + * where a section like this goes wrong: + * + * 1. **The managed set is the plan's own.** The sweep reads + * `managedObjectFields` off the driver `os migrate plan` diffed. That + * member is `protected` and reached structurally, so only a real boot can + * say whether it is populated at the moment the sweep runs — and a + * mistimed read (before `measureComposedCoverage` binds the composed + * host's objects) would report every platform table as unmanaged. + * 2. **The catalog query runs.** The three statements are written for three + * dialects; this exercises the sqlite one through the driver's own raw + * seam, against tables that really exist. + * + * ⛔ The NEGATIVE control is the point. `sys_user` is put in the database + * BEFORE the boot and must NOT be reported, alongside `_objectstack_sequences` + * and an application table. A sweep observed only in the presence of an orphan + * would be indistinguishable from one that reports everything. + * + * The positive control is the card's own measured case: a `sys_`-prefixed + * table left behind by a retired object, which no plan can mention today. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { bootSchemaStack, type SchemaStack } from '../../utils/schema-migrate.js'; +import { collectUnmanagedTables, type UnmanagedTablesReport } from './unmanaged-tables.js'; + +const ARTIFACT = { + // #8687: manifest fields under `manifest:` — the flat spelling is refused. + manifest: { id: 'orphan_smoke', name: 'Orphan Smoke', version: '0.0.0', type: 'app' }, + objects: [{ name: 'orphan_widget', fields: { name: { type: 'text' } } }], +}; + +/** + * The env vars that outrank the unified project default (#6469). Every one of + * them must be absent or the boot resolves somewhere else entirely and this + * test silently stops testing anything. + */ +const OVERRIDING_ENV = [ + 'OS_DATABASE_URL', + 'DATABASE_URL', + 'TURSO_DATABASE_URL', + 'OS_DATABASE_DRIVER', + 'OS_HOME', +] as const; + +describe('os migrate plan — unmanaged tables, against a real database (#13204)', () => { + let dir: string; + let dbFile: string; + let stack: SchemaStack | null = null; + const savedEnv: Record = {}; + + beforeEach(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-orphan-')); + mkdirSync(join(dir, 'dist'), { recursive: true }); + mkdirSync(join(dir, 'data'), { recursive: true }); + dbFile = join(dir, 'data', 'app.db'); + writeFileSync(join(dir, 'dist', 'objectstack.json'), JSON.stringify(ARTIFACT)); + + // The physical database, assembled OUTSIDE the booted stack so every table + // below is a fact about the file rather than about the boot. + const seed = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: dbFile }, + useNullAsDefault: true, + }); + const k = (seed as unknown as { knex: any }).knex; + const plain = async (name: string): Promise => { + await k.schema.createTable(name, (t: any) => { t.string('id').primary(); }); + }; + // NEGATIVE controls — every one of these is legitimate and must stay unreported. + await plain('sys_user'); // declared AND managed by the platform floor + await plain('_objectstack_sequences'); // the driver's own autonumber ledger + await plain('orphan_widget'); // this deployment's own object + await plain('app_leftovers'); // no reserved prefix at all + // POSITIVE control — the card's measured shape: a retired object's table. + await plain('sys_scim_provider'); + await k.destroy(); + + for (const key of OVERRIDING_ENV) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } + savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH; + savedEnv.NODE_ENV = process.env.NODE_ENV; + process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json'); + process.env.NODE_ENV = 'production'; // no dev auto-reconcile + + // The same boot `os migrate plan` performs: deferred DDL, read-only probe, + // and the deployment's own composed object set. + stack = await bootSchemaStack({ + jsonOutput: false, + projectRoot: dir, + databaseUrl: dbFile, + deferSchemaDdl: true, + readOnlyProbe: true, + composeHostStack: true, + }); + }, 120_000); + + afterEach(async () => { + try { await stack?.shutdown(); } catch { /* torn down either way */ } + stack = null; + for (const key of OVERRIDING_ENV) { + if (savedEnv[key] === undefined) delete process.env[key]; + else process.env[key] = savedEnv[key]; + } + process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH; + process.env.NODE_ENV = savedEnv.NODE_ENV; + try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + async function sweep(): Promise { + const { normalizeRows } = await import('@objectstack/metadata-protocol'); + return collectUnmanagedTables({ + driver: stack!.driver, + declaredObjects: stack!.allObjects(), + normalize: normalizeRows, + }); + } + + it('the boot this sweep reads from really did populate the managed set', () => { + // The premise the whole section rests on. A zero here would make every + // assertion below pass for the wrong reason — the sweep would be + // differencing against an empty set. + expect(stack!.driver).not.toBeNull(); + expect(stack!.managedTableCount).toBeGreaterThan(0); + }); + + it('REPORTS the stranded table, and reports ONLY it', async () => { + const report = await sweep(); + // ⛔ `unreadable` is not a pass. It is the third state, and it means the + // measurement below never happened. + expect(report.status).toBe('read'); + const read = report as Extract; + expect(read.tables.map((f) => f.table)).toEqual(['sys_scim_provider']); + // The negative controls, named individually so a failure says which one moved. + for (const legitimate of ['sys_user', '_objectstack_sequences', 'orphan_widget', 'app_leftovers']) { + expect(read.tables.map((f) => f.table)).not.toContain(legitimate); + } + expect(read.physicalTables).toBeGreaterThanOrEqual(5); + }, 120_000); + + it('goes SILENT once the stranded table is the only thing that changes', async () => { + // The other direction of the same measurement: remove the orphan, leave + // every negative control in place, and the section must disappear. Without + // this, "reports only sys_scim_provider" is also satisfied by a section + // that reports a fixed string. + const before = await sweep(); + expect((before as { tables: unknown[] }).tables).toHaveLength(1); + + await stack!.shutdown(); + stack = null; + const surgeon = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: dbFile }, + useNullAsDefault: true, + }); + const k = (surgeon as unknown as { knex: any }).knex; + await k.schema.dropTable('sys_scim_provider'); + await k.destroy(); + + stack = await bootSchemaStack({ + jsonOutput: false, + projectRoot: dir, + databaseUrl: dbFile, + deferSchemaDdl: true, + readOnlyProbe: true, + composeHostStack: true, + }); + + const after = await sweep(); + expect(after.status).toBe('read'); + expect((after as { tables: unknown[] }).tables).toEqual([]); + // Still a real sweep, not an early return: the negative controls are all + // still in the database. + expect((after as { physicalTables: number }).physicalTables).toBeGreaterThanOrEqual(4); + }, 180_000); +}); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.test.ts b/packages/cli/src/commands/migrate/unmanaged-tables.test.ts new file mode 100644 index 0000000000..f013e3788b --- /dev/null +++ b/packages/cli/src/commands/migrate/unmanaged-tables.test.ts @@ -0,0 +1,361 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13204 — the unmanaged-table sweep, in BOTH directions. + * + * The section this covers is informational, which is exactly why its negative + * control is the load-bearing half. A section only ever observed when an orphan + * exists has no evidence that it stays quiet when nothing is stranded, and a + * section that cries wolf on every run gets trained into noise and then + * ignored — at which point the real orphan it was built for scrolls past + * unread. So every legitimate platform-prefixed table family measured in this + * repo gets a test that asserts it is NOT reported: + * + * - a managed table (in the planned driver's `managedObjectFields`); + * - a DECLARED-but-unexamined object's table — `composition.coverage`'s + * population, deliberately not this one; + * - the rotation shards of a declared rotation object (`sys_activity` ships + * with `strategy: 'rotation'`, 14 daily shards, and only the BASE name is + * ever a `managedObjectFields` key); + * - driver-internal tables (`_objectstack_sequences`, `__os_mig_*`); + * - an application table with no reserved prefix. + * + * And the third direction, which is neither: every way the sweep can fail to + * OBTAIN an answer must report `unreadable`, never an empty `tables` list. + * "Did not look" and "looked, found nothing" are byte-identical to a reader + * otherwise, and they have opposite consequences. + */ + +import { describe, it, expect } from 'vitest'; +import { + buildKnownTableNames, + collectUnmanagedTables, + describeUnmanagedTable, + physicalTableListSql, + readManagedTableNames, + renderUnmanagedTables, + resolvePlannedDriverExec, + rotationBaseOf, + selectUnmanagedTables, + type UnmanagedTablesReport, +} from './unmanaged-tables.js'; + +/** `normalizeRows`' sqlite limb — a bare row array. Kept local so the unit tests hold no driver. */ +const normalize = (result: unknown): Record[] => + Array.isArray(result) ? (result as Record[]) : []; + +/** + * A stand-in for the driver `os migrate plan` diffed: the same two members the + * sweep reads off it — `managedObjectFields` (the map `detectManagedDrift` + * iterates) and a raw-SQL `execute`. + */ +function fakeDriver(opts: { + managed?: string[] | null; + client?: string | null; + answer?: unknown | (() => unknown); +}): unknown { + const driver: Record = {}; + if (opts.managed !== null) { + driver.managedObjectFields = new Map((opts.managed ?? []).map((t) => [t, {}])); + } + if (opts.client !== null) driver.config = { client: opts.client ?? 'better-sqlite3' }; + if (opts.answer !== undefined) { + driver.execute = async () => + typeof opts.answer === 'function' ? (opts.answer as () => unknown)() : opts.answer; + } + return driver; +} + +/** Catalog rows in the sqlite shape the sweep's own SELECT produces. */ +const rows = (...names: string[]) => names.map((name) => ({ table_name: name })); + +async function sweep(opts: { + managed?: string[] | null; + declared?: Array<{ name: string }>; + client?: string | null; + answer?: unknown | (() => unknown); +}): Promise { + return collectUnmanagedTables({ + driver: fakeDriver(opts), + declaredObjects: opts.declared ?? [], + normalize, + }); +} + +describe('physicalTableListSql — one statement per dialect family, null for the rest', () => { + it('enumerates BASE TABLES on each supported family and excludes views', () => { + for (const client of ['sqlite3', 'sqlite', 'better-sqlite3']) { + const sql = physicalTableListSql(client)!; + expect(sql).toContain('sqlite_master'); + expect(sql).toContain("type = 'table'"); + expect(sql).toContain("name NOT LIKE 'sqlite_%'"); + } + for (const client of ['postgres', 'pg', 'postgresql', 'pgnative']) { + const sql = physicalTableListSql(client)!; + expect(sql).toContain('information_schema.tables'); + expect(sql).toContain('current_schemas(false)'); + expect(sql).toContain("table_type = 'BASE TABLE'"); + } + for (const client of ['mysql', 'mysql2']) { + const sql = physicalTableListSql(client)!; + expect(sql).toContain('information_schema.tables'); + expect(sql).toContain('DATABASE()'); + expect(sql).toContain("table_type = 'BASE TABLE'"); + } + }); + + it('is case-insensitive on the client spelling', () => { + expect(physicalTableListSql('Better-SQLite3')).toBe(physicalTableListSql('better-sqlite3')); + }); + + it('returns null — never a guess — for a dialect it cannot enumerate', () => { + // ⛔ Not "assume sqlite": a wrong catalog query either throws or answers for + // the wrong population, and the second is silent. + for (const client of ['mssql', 'oracledb', 'cockroachdb', 'redshift', '', undefined]) { + expect(physicalTableListSql(client)).toBeNull(); + } + }); +}); + +describe('rotationBaseOf — SqlDriver.ensureRotation’s own shard grammar', () => { + it('folds a shard onto its base', () => { + expect(rotationBaseOf('sys_activity__r20260829')).toBe('sys_activity'); + expect(rotationBaseOf('sys_activity__r202608')).toBe('sys_activity'); + }); + + it('leaves a name that merely looks similar alone', () => { + expect(rotationBaseOf('sys_activity')).toBeNull(); + expect(rotationBaseOf('sys_activity__rollup')).toBeNull(); + expect(rotationBaseOf('sys_activity__r12345')).toBeNull(); // 5 digits — below the grammar + expect(rotationBaseOf('__r20260829')).toBeNull(); // no base to fold onto + }); +}); + +describe('readManagedTableNames — an unreadable map is null, never an empty set', () => { + it('reads the map keys', () => { + expect([...readManagedTableNames(fakeDriver({ managed: ['sys_user', 'sys_secret'] }))!].sort()) + .toEqual(['sys_secret', 'sys_user']); + }); + + it('answers null when the member is missing or is not a Map', () => { + // The field is `protected` on SqlDriver and reached structurally: a rename + // hands this `undefined`, and reading that as "nothing is managed" would + // report every platform table in the database as unmanaged. + expect(readManagedTableNames(fakeDriver({ managed: null }))).toBeNull(); + expect(readManagedTableNames({ managedObjectFields: {} })).toBeNull(); + expect(readManagedTableNames(null)).toBeNull(); + expect(readManagedTableNames(undefined)).toBeNull(); + }); +}); + +describe('buildKnownTableNames — the managed set UNION every declared object', () => { + it('carries both, so a declared-but-unexamined object is accounted for', () => { + const known = buildKnownTableNames(new Set(['sys_user']), [{ name: 'sys_secret' }]); + expect(known.has('sys_user')).toBe(true); + expect(known.has('sys_secret')).toBe(true); + }); + + it('adds a legacy double-underscore name under both spellings', () => { + const known = buildKnownTableNames(new Set(), [{ name: 'crm__account' }]); + expect(known.has('crm__account')).toBe(true); + expect(known.has('account')).toBe(true); + }); + + it('ignores objects with no usable name', () => { + const known = buildKnownTableNames(new Set(), [{}, { name: '' }, null, undefined] as never[]); + expect(known.size).toBe(0); + }); +}); + +describe('selectUnmanagedTables — the predicate', () => { + it('REPORTS a platform-prefixed table nothing declares', () => { + // The card's measured case: `sys_scim_provider` retired, table still there. + expect(selectUnmanagedTables(['sys_scim_provider'], new Set(['sys_user']))) + .toEqual([{ table: 'sys_scim_provider' }]); + }); + + it('does NOT report a managed table (negative control)', () => { + expect(selectUnmanagedTables(['sys_user'], new Set(['sys_user']))).toEqual([]); + }); + + it('does NOT report a DECLARED-but-unexamined object — that is composition.coverage', () => { + // ~80 declared / 8 examined is the measured control-plane shape. Reporting + // the other ~72 here would be false (they ARE declared) and would bury the + // one real orphan. + const declared = Array.from({ length: 72 }, (_, i) => ({ name: `sys_declared_${i}` })); + const physical = [...declared.map((o) => o.name), 'sys_scim_provider']; + const known = buildKnownTableNames(new Set(['sys_user']), declared); + expect(selectUnmanagedTables(physical, known)).toEqual([{ table: 'sys_scim_provider' }]); + }); + + it('does NOT report the rotation shards of a declared rotation object', () => { + // `sys_activity` ships `lifecycle.storage.strategy: 'rotation'` with 14 + // daily shards; `aliasShardBookkeeping` never adds a shard to + // `managedObjectFields`, so only the base name is ever a key there. + const shards = Array.from({ length: 14 }, (_, i) => `sys_activity__r202608${String(i + 10)}`); + expect(selectUnmanagedTables(shards, new Set(['sys_activity']))).toEqual([]); + }); + + it('collapses the shards of an UNDECLARED rotation base into one row', () => { + const shards = ['sys_activity__r20260810', 'sys_activity__r20260811']; + expect(selectUnmanagedTables(shards, new Set())).toEqual([ + { table: 'sys_activity', rotationShards: shards }, + ]); + }); + + it('merges an undeclared base with its own orphaned shards into a single row', () => { + expect(selectUnmanagedTables(['sys_activity', 'sys_activity__r20260810'], new Set())).toEqual([ + { table: 'sys_activity', rotationShards: ['sys_activity__r20260810'] }, + ]); + }); + + it('does NOT report driver-internal or application tables', () => { + expect( + selectUnmanagedTables( + ['_objectstack_sequences', '__os_mig_sys_user', 'contacts', 'crm_account', 'knex_migrations'], + new Set(), + ), + ).toEqual([]); + }); + + it('covers every reserved platform prefix, not just sys_', () => { + const found = selectUnmanagedTables(['sys_a', 'cloud_b', 'ai_c', 'app_d'], new Set()) + .map((f) => f.table); + expect(found).toEqual(['ai_c', 'cloud_b', 'sys_a']); + }); + + it('is sorted and de-duplicated', () => { + expect(selectUnmanagedTables(['sys_b', 'sys_a', 'sys_b'], new Set()).map((f) => f.table)) + .toEqual(['sys_a', 'sys_b']); + }); +}); + +describe('collectUnmanagedTables — every no-answer path is unreadable, not empty', () => { + it('reads a clean database and says so with an empty list', async () => { + const report = await sweep({ + managed: ['sys_user'], + answer: rows('sys_user', '_objectstack_sequences', 'contacts'), + }); + expect(report).toMatchObject({ status: 'read', physicalTables: 3, tables: [] }); + }); + + it('reports the orphan and nothing else', async () => { + const report = await sweep({ + managed: ['sys_user'], + declared: [{ name: 'sys_secret' }], + answer: rows('sys_user', 'sys_secret', 'sys_scim_provider', 'contacts'), + }); + expect(report).toMatchObject({ status: 'read', tables: [{ table: 'sys_scim_provider' }] }); + }); + + it('reads MySQL’s uppercase TABLE_NAME column', async () => { + const report = await sweep({ + managed: [], + client: 'mysql2', + answer: [{ TABLE_NAME: 'sys_scim_provider' }], + }); + expect(report).toMatchObject({ status: 'read', tables: [{ table: 'sys_scim_provider' }] }); + }); + + it('is unreadable when the managed map cannot be read', async () => { + const report = await sweep({ managed: null, answer: rows('sys_scim_provider') }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('managed-table map'); + }); + + it('is unreadable when the planned driver exposes no raw SQL seam', async () => { + const report = await sweep({ managed: [] }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('raw SQL seam'); + }); + + it('is unreadable on a dialect it cannot enumerate', async () => { + const report = await sweep({ managed: [], client: 'mssql', answer: rows('sys_x') }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('mssql'); + }); + + it('is unreadable when the seam returns no result set (#10677’s shape)', async () => { + // `InMemoryDriver.execute()` returns `null` — it neither throws nor is + // absent, and `normalizeRows(null)` is `[]`, which is also what a real + // driver returns for a catalog with nothing in it. + const report = await sweep({ managed: [], answer: null }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('no result set'); + }); + + it('is unreadable when the catalog read throws', async () => { + const report = await sweep({ + managed: [], + answer: () => { throw new Error('permission denied for schema information_schema'); }, + }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('permission denied'); + }); + + it('is unreadable when rows carry no recognised table-name column', async () => { + const report = await sweep({ managed: [], answer: [{ relname: 'sys_x' }] }); + expect(report.status).toBe('unreadable'); + expect((report as { detail: string }).detail).toContain('no recognised table-name column'); + }); + + it('reads an EMPTY catalog as a real answer, not as unreadable', async () => { + const report = await sweep({ managed: [], answer: [] }); + expect(report).toMatchObject({ status: 'read', physicalTables: 0, tables: [] }); + }); +}); + +describe('resolvePlannedDriverExec — bound to the planned driver, never to some other one', () => { + it('prefers execute, falls back to raw, and answers null for neither', async () => { + const seen: string[] = []; + const viaExecute = resolvePlannedDriverExec({ + execute: async (sql: string) => { seen.push(`execute:${sql}`); return []; }, + raw: async () => { seen.push('raw'); return []; }, + })!; + await viaExecute('select 1'); + expect(seen).toEqual(['execute:select 1']); + + const viaRaw = resolvePlannedDriverExec({ raw: async (sql: string) => { seen.push(`raw:${sql}`); return []; } })!; + await viaRaw('select 2'); + expect(seen).toEqual(['execute:select 1', 'raw:select 2']); + + expect(resolvePlannedDriverExec({})).toBeNull(); + expect(resolvePlannedDriverExec(null)).toBeNull(); + }); +}); + +describe('rendering — informational, and silent when there is nothing to say', () => { + it('prints nothing when the sweep ran and found nothing', () => { + expect(renderUnmanagedTables({ status: 'read', prefixes: ['sys_'], physicalTables: 9, tables: [] })) + .toEqual([]); + }); + + it('is LOUD when the sweep could not run', () => { + const lines = renderUnmanagedTables({ status: 'unreadable', prefixes: ['sys_'], detail: 'no seam' }); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('did not run'); + expect(lines[0]).toContain('no seam'); + }); + + it('names the tables and proposes NOTHING — no drop, no remedy', () => { + const lines = renderUnmanagedTables({ + status: 'read', + prefixes: ['sys_', 'cloud_', 'ai_'], + physicalTables: 40, + tables: [{ table: 'sys_scim_provider' }], + }); + const text = lines.join('\n'); + expect(text).toContain('sys_scim_provider'); + expect(text).toContain('information only'); + // ⛔ The hard fence: this section never proposes a drop. + expect(text.toLowerCase()).not.toContain('drop '); + expect(text.toLowerCase()).not.toContain('--allow-destructive'); + expect(text.toLowerCase()).not.toContain('delete'); + }); + + it('describes a collapsed rotation family by its shard count', () => { + expect(describeUnmanagedTable({ table: 'sys_activity', rotationShards: ['sys_activity__r20260810'] })) + .toBe('sys_activity (1 rotation shard(s): sys_activity__r20260810)'); + expect(describeUnmanagedTable({ table: 'sys_scim_provider' })).toBe('sys_scim_provider'); + }); +}); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.ts b/packages/cli/src/commands/migrate/unmanaged-tables.ts new file mode 100644 index 0000000000..b430cd31e6 --- /dev/null +++ b/packages/cli/src/commands/migrate/unmanaged-tables.ts @@ -0,0 +1,394 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The physical tables **nothing declares** — `os migrate plan`'s informational + * sweep (#13204). + * + * ## The blind spot this closes + * + * `detectManagedDrift()` iterates the driver's `managedObjectFields`: it diffs + * the tables metadata DECLARES against the physical database, and a table no + * declaration names is not in that map, so no plan can ever mention it. + * Retiring an object therefore strands its table forever — measured on + * ObjectStack Cloud's control DB, where a retired `sys_scim_provider` is still + * present (0 rows) and every plan since has read as clean. + * + * ## ⛔ It never drops anything, and never proposes a drop + * + * The output is a list of names and a count. No DDL is generated here, none is + * suggested, and this module is wired into nothing that executes DDL. Dropping + * an existing physical table is destructive and hard to reverse: that decision + * stays with a human, and this card deliberately does not introduce it. + * + * ## ⛔ It is NOT `composition.coverage` (#13057), and must not blur into it + * + * Two different predicates over two different populations: + * + * - **coverage** — of the objects this deployment DECLARES, how many did the + * plan examine? Its unexamined remainder is declared metadata that the plan + * could not reach. + * - **this sweep** — of the tables that physically EXIST, which ones does no + * declaration account for at all? + * + * Folding them together would make "examined and clean" indistinguishable from + * "never looked at", which is the blind spot both exist to remove. So a + * declared-but-unexamined object's table is deliberately **excluded** here (see + * {@link buildKnownTableNames}): coverage already reports it, and reporting it + * a second time as "unmanaged" would be false — it is declared. + * + * ## Why a naive `sys_` prefix scan is NOT the predicate (measured) + * + * Three physical-table families carry a platform prefix while being legitimate, + * and a difference against `managedObjectFields` alone reports all three: + * + * 1. **Rotation shards.** `sys_activity` declares + * `lifecycle.storage.strategy: 'rotation'` with 14 daily shards, so the + * physical database carries up to 14 `sys_activity__r` tables and + * the base name is a VIEW. `aliasShardBookkeeping` copies the per-table + * bookkeeping to each shard but deliberately does NOT add it to + * `managedObjectFields` — only the base name is ever a key there. A naive + * sweep prints 14 false rows on every plan of every deployment running + * `plugin-audit`. {@link rotationBaseOf} folds a shard back onto its base + * before the membership test. + * 2. **Declared-but-unexamined objects.** On the control plane #13028 + * measured, ~80 objects were declared and **8** reached the driver: the + * other ~72 tables exist and are absent from `managedObjectFields`. A + * naive sweep would print ~72 false rows and bury the one real orphan. + * 3. **Driver-internal tables.** `_objectstack_sequences` and the SQLite + * rebuild scratch table `__os_mig_` carry no platform prefix, so the + * prefix test excludes them without an allowlist to maintain. + * + * The prefix itself is read from `PLATFORM_OBJECT_PREFIXES` rather than + * spelled `'sys_'` here. That constant is the repo's own registry of namespaces + * reserved for platform-provided objects, and its module header records what a + * fourth hand-rolled copy of the prefix heuristic cost the last time. + * + * ## ⛔ "Could not look" is never reported as "nothing found" + * + * Every path that fails to obtain an answer returns {@link UnmanagedTablesUnreadable} + * — the same discipline `os migrate duplicates` carries for its own probe. An + * empty list here means the sweep RAN and found nothing; a sweep that could not + * run says so, because those two have opposite consequences and are otherwise + * byte-identical to a reader. + */ + +import { PLATFORM_OBJECT_PREFIXES, StorageNameMapping, hasPlatformObjectPrefix } from '@objectstack/spec/system'; +// The result-set/no-answer predicate `os migrate duplicates` already publishes +// (#10677). Imported rather than re-spelled: a second copy would drift, and the +// two commands must agree on what "the seam did not answer" looks like. Both +// modules are siblings under `commands/migrate/`, and the import costs nothing +// `plan.ts` does not already load. +import { isResultSet } from './duplicates.js'; + +/** How a raw SELECT is issued against the driver the plan diffed. */ +export type PlannedDriverExec = (sql: string, params?: unknown[]) => Promise; + +/** + * One row of the informational section. + * + * `table` is the physical table name — except when `rotationShards` is present, + * where it is the rotation BASE the shards belong to. A retired rotation object + * leaves N shard tables and no base table, and listing all N separately would + * be N rows of noise for one stranded object. + */ +export interface UnmanagedTableFinding { + /** The unmanaged table, or the rotation base when this row collapses a shard family. */ + table: string; + /** Present only on a collapsed rotation family: the shard tables it stands for. */ + rotationShards?: string[]; +} + +/** The sweep ran. `tables: []` is a real answer — nothing physical is unaccounted for. */ +export interface UnmanagedTablesRead { + status: 'read'; + /** Namespace prefixes the sweep considered. */ + prefixes: readonly string[]; + /** How many physical base tables the sweep enumerated, of any prefix. */ + physicalTables: number; + /** Sorted, and empty when everything physical is declared. */ + tables: UnmanagedTableFinding[]; +} + +/** The sweep could not obtain an answer. ⛔ Never to be rendered as "none found". */ +export interface UnmanagedTablesUnreadable { + status: 'unreadable'; + prefixes: readonly string[]; + /** Why, in the operator's terms. */ + detail: string; +} + +export type UnmanagedTablesReport = UnmanagedTablesRead | UnmanagedTablesUnreadable; + +/** knex client spellings, per family — the same three families `SqlDriver` emits for. */ +const SQLITE_CLIENTS: ReadonlySet = new Set(['sqlite3', 'sqlite', 'better-sqlite3']); +const POSTGRES_CLIENTS: ReadonlySet = new Set(['postgres', 'pg', 'postgresql', 'pgnative']); +const MYSQL_CLIENTS: ReadonlySet = new Set(['mysql', 'mysql2']); + +/** + * The statement that lists physical BASE TABLES, for the dialect actually + * connected — or `null` when the client spelling names no family this sweep can + * enumerate. + * + * `null` is deliberately not "assume sqlite": a wrong catalog query either + * throws (loud, fine) or returns rows for the wrong population (silent, not + * fine), and an unrecognised dialect must reach {@link UnmanagedTablesUnreadable} + * instead of either. + * + * The three statements mirror `SqlDriver.introspectSchema()`'s own table-name + * pass, including its two exclusions that matter here: VIEWS are out (a + * rotation base is a view, and a view is not stranded storage), and SQLite's + * internal `sqlite_%` tables are out. This sweep does not CALL that method + * because it needs table names alone — `introspectSchema` costs four further + * introspection round-trips per table, which on the ~80-table control plane + * this card comes from would be ~320 queries added to a dry run that the card + * asks to stay cheap. One query, one round trip. + */ +export function physicalTableListSql(client?: string): string | null { + const c = String(client ?? '').toLowerCase(); + if (SQLITE_CLIENTS.has(c)) { + return "SELECT name AS table_name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'"; + } + if (POSTGRES_CLIENTS.has(c)) { + return 'SELECT table_name FROM information_schema.tables ' + + 'WHERE table_schema = ANY (current_schemas(false)) AND table_type = \'BASE TABLE\''; + } + if (MYSQL_CLIENTS.has(c)) { + return 'SELECT table_name FROM information_schema.tables ' + + "WHERE table_schema = DATABASE() AND table_type = 'BASE TABLE'"; + } + return null; +} + +/** + * The rotation BASE a shard table belongs to, or `null` when the name is not a + * shard. + * + * `__r` + a 6-to-8 digit period key is `SqlDriver.ensureRotation`'s own shard + * grammar (`/__r\d{6,8}$/`), restated here because this is the consumer side of + * the same naming decision. + */ +export function rotationBaseOf(table: string): string | null { + const match = /^(.+)__r\d{6,8}$/.exec(table); + return match ? match[1]! : null; +} + +/** + * Read the planned driver's managed-table map — the SAME `managedObjectFields` + * `detectManagedDrift()` iterates — as a name set. + * + * ⚠️ Returns `null`, never an empty set, when the map cannot be read. The field + * is `protected` on `SqlDriver` and reached structurally, so a rename would + * hand this an `undefined`; treating that as "nothing is managed" would report + * every platform-namespaced table in the database as unmanaged — the loudest + * possible false alarm, from the quietest possible cause. + */ +export function readManagedTableNames(driver: unknown): ReadonlySet | null { + const map = (driver as { managedObjectFields?: unknown } | null | undefined)?.managedObjectFields; + if (!(map instanceof Map)) return null; + const names = new Set(); + for (const key of map.keys()) if (typeof key === 'string' && key) names.add(key); + return names; +} + +/** + * Every table name a declaration accounts for: the planned driver's managed set + * UNION the table names of every object the booted stack registered. + * + * The managed set is the plan's own (premise: one source, so the section cannot + * disagree with the plan beside it). The declared set is the wider one, and it + * is what keeps a declared-but-unexamined object out of this report — see this + * module's header on why that belongs to `composition.coverage` instead. + * + * Both an object's `name` and its resolved table name are added: legacy + * double-underscore names resolve to a different table, and over-excluding is + * the safe direction for a section whose failure mode is crying wolf. + */ +export function buildKnownTableNames( + managed: ReadonlySet, + declaredObjects: readonly unknown[], +): ReadonlySet { + const known = new Set(managed); + for (const object of declaredObjects) { + const name = (object as { name?: unknown } | null | undefined)?.name; + if (typeof name !== 'string' || !name) continue; + known.add(name); + try { + known.add(StorageNameMapping.resolveTableName({ name })); + } catch { + /* a name the mapping rejects is already added verbatim above */ + } + } + return known; +} + +/** + * The predicate itself: platform-namespaced physical tables that `known` + * accounts for neither directly nor as a rotation shard. + */ +export function selectUnmanagedTables( + physical: readonly string[], + known: ReadonlySet, +): UnmanagedTableFinding[] { + const findings = new Map(); + for (const table of physical) { + if (!hasPlatformObjectPrefix(table)) continue; + if (known.has(table)) continue; + const base = rotationBaseOf(table); + if (base !== null) { + // A declared rotation object's shards ARE its managed storage; only the + // base name is ever a `managedObjectFields` key. + if (known.has(base)) continue; + const existing = findings.get(base) ?? { table: base }; + (existing.rotationShards ??= []).push(table); + findings.set(base, existing); + continue; + } + findings.set(table, findings.get(table) ?? { table }); + } + const out = [...findings.values()]; + for (const finding of out) finding.rotationShards?.sort(); + return out.sort((a, b) => a.table.localeCompare(b.table)); +} + +/** Pull a table name out of one catalog row — MySQL answers `TABLE_NAME`. */ +function tableNameOf(row: Record): string | null { + for (const key of ['table_name', 'TABLE_NAME', 'name'] as const) { + const value = row[key]; + if (typeof value === 'string' && value) return value; + } + return null; +} + +/** + * Resolve the raw-SQL seam **of the driver the plan diffed**. + * + * ⛔ Deliberately NOT `resolveSeedTenancyExec(engine)`, which walks the engine + * for ANY raw-capable driver: on a deployment with more than one datasource + * that can be a different database entirely, and this sweep would then compare + * database A's tables against database B's managed set — every row a false + * positive. The identity that matters is the same one `measureComposedCoverage` + * uses (`driver !== plannedDriver`), so the seam is taken from that driver and + * nowhere else. + */ +export function resolvePlannedDriverExec(driver: unknown): PlannedDriverExec | null { + const d = driver as { + execute?: (sql: string, params?: unknown[]) => Promise; + raw?: (sql: string) => Promise; + } | null | undefined; + if (typeof d?.execute === 'function') return (sql, params) => d.execute!(sql, params ?? []); + if (typeof d?.raw === 'function') return (sql) => d.raw!(sql); + return null; +} + +const unreadable = (detail: string): UnmanagedTablesUnreadable => ({ + status: 'unreadable', + prefixes: PLATFORM_OBJECT_PREFIXES, + detail, +}); + +/** + * Sweep the physical database for platform-namespaced tables no declaration + * accounts for. + * + * Reads only — one SELECT against a catalog. Every failure lands as + * {@link UnmanagedTablesUnreadable}; none of them throws, because this section + * is informational and must never turn a working `os migrate plan` into a + * failing one. + * + * @param driver the driver whose managed set the plan diffed. + * @param declaredObjects everything the booted stack registered (`stack.allObjects()`). + * @param normalize `normalizeRows` — flattens the three dialect result shapes. + */ +export async function collectUnmanagedTables(opts: { + driver: unknown; + declaredObjects: readonly unknown[]; + normalize: (result: unknown) => Record[]; +}): Promise { + const managed = readManagedTableNames(opts.driver); + if (managed === null) { + return unreadable( + 'the planned driver exposes no readable managed-table map, so "declared by nothing" could not be ' + + 'decided — reporting nothing rather than reporting every platform table as unmanaged', + ); + } + + const exec = resolvePlannedDriverExec(opts.driver); + if (exec === null) { + return unreadable('the planned driver exposes no raw SQL seam, so its table catalog could not be read'); + } + + const client = (opts.driver as { config?: { client?: unknown } } | null | undefined)?.config?.client; + const clientName = typeof client === 'string' ? client : ''; + const sql = physicalTableListSql(clientName); + if (sql === null) { + return unreadable( + `the connected dialect (${clientName || 'unnamed client'}) is not one this sweep enumerates tables on ` + + '(sqlite / postgres / mysql)', + ); + } + + let result: unknown; + try { + result = await exec(sql, []); + } catch (error: unknown) { + return unreadable( + `the table-catalog read failed: ${error instanceof Error ? error.message : String(error)}`, + ); + } + + // #10677's distinction, restated for this probe: a seam that returns no + // RESULT SET did not answer "no tables" — it did not answer. + if (!isResultSet(result)) { + return unreadable( + 'the raw SQL seam returned no result set — a seam that cannot answer is not a seam that answered ' + + '"no unmanaged tables"', + ); + } + + const rows = opts.normalize(result); + const names: string[] = []; + for (const row of rows) { + const name = tableNameOf(row); + if (name !== null) names.push(name); + } + if (rows.length > 0 && names.length === 0) { + return unreadable( + `the table catalog answered ${rows.length} row(s) carrying no recognised table-name column`, + ); + } + + return { + status: 'read', + prefixes: PLATFORM_OBJECT_PREFIXES, + physicalTables: names.length, + tables: selectUnmanagedTables(names, buildKnownTableNames(managed, opts.declaredObjects)), + }; +} + +/** One finding as an operator-facing line (no leading bullet, no colour). */ +export function describeUnmanagedTable(finding: UnmanagedTableFinding): string { + if (!finding.rotationShards || finding.rotationShards.length === 0) return finding.table; + return `${finding.table} (${finding.rotationShards.length} rotation shard(s): ` + + `${finding.rotationShards.join(', ')})`; +} + +/** + * The informational section, as the lines `os migrate plan` prints — or `[]` + * when there is nothing to say. + * + * ⛔ The wording states what was found and stops. It proposes no remedy, + * because the only remedy is a destructive DDL statement this card does not + * introduce. + */ +export function renderUnmanagedTables(report: UnmanagedTablesReport): string[] { + if (report.status === 'unreadable') { + return [`Unmanaged-table sweep did not run: ${report.detail}.`]; + } + if (report.tables.length === 0) return []; + return [ + `${report.tables.length} table(s) in this database carry a reserved platform prefix ` + + `(${report.prefixes.join(', ')}) and are declared by no object in this plan:`, + ...report.tables.map((finding) => ` • ${describeUnmanagedTable(finding)}`), + 'They are reported for information only — nothing here drops them, and a plan writes nothing.', + ]; +} From 65b5d998536cb00ba4b7d0a77fe9e415633ffd30 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 16:00:08 +0000 Subject: [PATCH 2/3] fix(cli): gate the unmanaged-table sweep on a composition that mirrors the deployment (#13204) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k --- packages/cli/src/commands/migrate/plan.ts | 1 + .../unmanaged-tables.integration.test.ts | 214 +++++++++++------- .../commands/migrate/unmanaged-tables.test.ts | 30 +++ .../src/commands/migrate/unmanaged-tables.ts | 30 +++ 4 files changed, 191 insertions(+), 84 deletions(-) diff --git a/packages/cli/src/commands/migrate/plan.ts b/packages/cli/src/commands/migrate/plan.ts index 59af2be708..ffe1bbad03 100644 --- a/packages/cli/src/commands/migrate/plan.ts +++ b/packages/cli/src/commands/migrate/plan.ts @@ -187,6 +187,7 @@ export default class MigratePlan extends Command { const unmanagedTables: UnmanagedTablesReport = await collectUnmanagedTables({ driver: stack.driver, declaredObjects: stack.allObjects(), + composition: stack.composition, normalize: normalizeRows, }); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts b/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts index dd14246193..af2e54b9d2 100644 --- a/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts +++ b/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts @@ -5,8 +5,8 @@ * database file, in both directions. * * `unmanaged-tables.test.ts` pins the predicate over hand-built sets. What it - * cannot pin is the two joins to the rest of the system, and both are exactly - * where a section like this goes wrong: + * cannot pin is the three joins to the rest of the system, and each is a place + * a section like this goes wrong: * * 1. **The managed set is the plan's own.** The sweep reads * `managedObjectFields` off the driver `os migrate plan` diffed. That @@ -14,32 +14,45 @@ * say whether it is populated at the moment the sweep runs — and a * mistimed read (before `measureComposedCoverage` binds the composed * host's objects) would report every platform table as unmanaged. - * 2. **The catalog query runs.** The three statements are written for three + * 2. **The declared set is the deployment's own.** The fixture is #12938's + * shape — a host `objectstack.config.ts` whose whole object set comes from + * a plugin, which is what ObjectStack Cloud's control plane has — so the + * composition this sweep requires is the one it actually gets in the field. + * 3. **The catalog query runs.** The three statements are written for three * dialects; this exercises the sqlite one through the driver's own raw * seam, against tables that really exist. * - * ⛔ The NEGATIVE control is the point. `sys_user` is put in the database + * ⛔ The NEGATIVE control is the point. `sys_permission_set` (from the composed + * plugin) and `sys_secret` (from the platform floor) are put in the database * BEFORE the boot and must NOT be reported, alongside `_objectstack_sequences` * and an application table. A sweep observed only in the presence of an orphan * would be indistinguishable from one that reports everything. * - * The positive control is the card's own measured case: a `sys_`-prefixed - * table left behind by a retired object, which no plan can mention today. + * The positive control is the card's own measured case: a `sys_`-prefixed table + * left behind by a retired object, which no plan can mention today. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync, symlinkSync } from 'node:fs'; +import { createRequire } from 'node:module'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; import { SqlDriver } from '@objectstack/driver-sql'; import { bootSchemaStack, type SchemaStack } from '../../utils/schema-migrate.js'; import { collectUnmanagedTables, type UnmanagedTablesReport } from './unmanaged-tables.js'; -const ARTIFACT = { - // #8687: manifest fields under `manifest:` — the flat spelling is refused. - manifest: { id: 'orphan_smoke', name: 'Orphan Smoke', version: '0.0.0', type: 'app' }, - objects: [{ name: 'orphan_widget', fields: { name: { type: 'text' } } }], -}; +const require_ = createRequire(import.meta.url); + +/** + * The installed `@objectstack/plugin-security` package root — resolved through + * this package's own dependency graph rather than written as a path climbing + * into a sibling package, so this test's inputs stay equal to its declared ones + * (the `check:cross-package-test-inputs` reasoning; a `node_modules` read is + * what a dependency IS). + */ +function securityPackageRoot(): string { + return resolve(dirname(require_.resolve('@objectstack/plugin-security')), '..'); +} /** * The env vars that outrank the unified project default (#6469). Every one of @@ -54,38 +67,97 @@ const OVERRIDING_ENV = [ 'OS_HOME', ] as const; +/** Legitimate, and every one of them must stay OUT of the report. */ +const NEGATIVE_CONTROLS = [ + 'sys_permission_set', // declared by the composed host plugin + 'sys_secret', // declared by the platform floor + '_objectstack_sequences', // the driver's own autonumber ledger + 'app_leftovers', // no reserved prefix at all +]; + +/** The card's measured shape: a retired object's table, declared by nothing. */ +const STRANDED = 'sys_scim_provider'; + describe('os migrate plan — unmanaged tables, against a real database (#13204)', () => { let dir: string; let dbFile: string; let stack: SchemaStack | null = null; const savedEnv: Record = {}; - beforeEach(async () => { - dir = mkdtempSync(join(tmpdir(), 'os-orphan-')); - mkdirSync(join(dir, 'dist'), { recursive: true }); - mkdirSync(join(dir, 'data'), { recursive: true }); - dbFile = join(dir, 'data', 'app.db'); - writeFileSync(join(dir, 'dist', 'objectstack.json'), JSON.stringify(ARTIFACT)); - - // The physical database, assembled OUTSIDE the booted stack so every table - // below is a fact about the file rather than about the boot. + /** Assemble physical tables OUTSIDE the booted stack, so each is a fact about the file. */ + async function createTables(names: readonly string[]): Promise { const seed = new SqlDriver({ client: 'better-sqlite3', connection: { filename: dbFile }, useNullAsDefault: true, }); const k = (seed as unknown as { knex: any }).knex; - const plain = async (name: string): Promise => { - await k.schema.createTable(name, (t: any) => { t.string('id').primary(); }); - }; - // NEGATIVE controls — every one of these is legitimate and must stay unreported. - await plain('sys_user'); // declared AND managed by the platform floor - await plain('_objectstack_sequences'); // the driver's own autonumber ledger - await plain('orphan_widget'); // this deployment's own object - await plain('app_leftovers'); // no reserved prefix at all - // POSITIVE control — the card's measured shape: a retired object's table. - await plain('sys_scim_provider'); - await k.destroy(); + try { + for (const name of names) { + await k.schema.createTable(name, (t: any) => { t.string('id').primary(); }); + } + } finally { + await k.destroy(); + } + } + + async function dropTable(name: string): Promise { + const surgeon = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: dbFile }, + useNullAsDefault: true, + }); + const k = (surgeon as unknown as { knex: any }).knex; + try { + await k.schema.dropTable(name); + } finally { + await k.destroy(); + } + } + + /** The same boot `os migrate plan` performs. */ + const bootLikeMigrate = (): Promise => bootSchemaStack({ + jsonOutput: false, + projectRoot: dir, + databaseUrl: `file:${dbFile}`, + deferSchemaDdl: true, + readOnlyProbe: true, + composeHostStack: true, + }); + + async function sweep(): Promise { + const { normalizeRows } = await import('@objectstack/metadata-protocol'); + return collectUnmanagedTables({ + driver: stack!.driver, + declaredObjects: stack!.allObjects(), + composition: stack!.composition, + normalize: normalizeRows, + }); + } + + beforeEach(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-orphan-')); + dbFile = join(dir, 'control.db'); + + // #12938's fixture shape: a host config whose whole object set comes from a + // plugin, and no compiled artifact at all. + mkdirSync(join(dir, 'node_modules', '@objectstack'), { recursive: true }); + symlinkSync( + securityPackageRoot(), + join(dir, 'node_modules', '@objectstack', 'plugin-security'), + 'dir', + ); + writeFileSync( + join(dir, 'objectstack.config.ts'), + [ + "import { SecurityPlugin } from '@objectstack/plugin-security';", + '', + 'export default { plugins: [new SecurityPlugin()] };', + '', + ].join('\n'), + ); + + await createTables([...NEGATIVE_CONTROLS, STRANDED]); for (const key of OVERRIDING_ENV) { savedEnv[key] = process.env[key]; @@ -93,20 +165,11 @@ describe('os migrate plan — unmanaged tables, against a real database (#13204) } savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH; savedEnv.NODE_ENV = process.env.NODE_ENV; - process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json'); + process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json'); // deliberately absent process.env.NODE_ENV = 'production'; // no dev auto-reconcile - // The same boot `os migrate plan` performs: deferred DDL, read-only probe, - // and the deployment's own composed object set. - stack = await bootSchemaStack({ - jsonOutput: false, - projectRoot: dir, - databaseUrl: dbFile, - deferSchemaDdl: true, - readOnlyProbe: true, - composeHostStack: true, - }); - }, 120_000); + stack = await bootLikeMigrate(); + }, 180_000); afterEach(async () => { try { await stack?.shutdown(); } catch { /* torn down either way */ } @@ -115,26 +178,23 @@ describe('os migrate plan — unmanaged tables, against a real database (#13204) if (savedEnv[key] === undefined) delete process.env[key]; else process.env[key] = savedEnv[key]; } - process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH; - process.env.NODE_ENV = savedEnv.NODE_ENV; + if (savedEnv.OS_ARTIFACT_PATH === undefined) delete process.env.OS_ARTIFACT_PATH; + else process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH; + if (savedEnv.NODE_ENV === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = savedEnv.NODE_ENV; try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } }); - async function sweep(): Promise { - const { normalizeRows } = await import('@objectstack/metadata-protocol'); - return collectUnmanagedTables({ - driver: stack!.driver, - declaredObjects: stack!.allObjects(), - normalize: normalizeRows, - }); - } - - it('the boot this sweep reads from really did populate the managed set', () => { - // The premise the whole section rests on. A zero here would make every - // assertion below pass for the wrong reason — the sweep would be - // differencing against an empty set. + it('the boot this sweep reads from really did compose the deployment', () => { + // The premises the whole section rests on. A zero managed set, or a + // composition that did not load the host config, would make every + // assertion below pass for the wrong reason. expect(stack!.driver).not.toBeNull(); + expect(stack!.composition.hostConfigLoaded).toBe(true); expect(stack!.managedTableCount).toBeGreaterThan(0); + const declared = (stack!.allObjects() as Array<{ name?: string }>).map((o) => o?.name); + expect(declared).toContain('sys_permission_set'); + expect(declared).toContain('sys_secret'); }); it('REPORTS the stranded table, and reports ONLY it', async () => { @@ -143,13 +203,13 @@ describe('os migrate plan — unmanaged tables, against a real database (#13204) // measurement below never happened. expect(report.status).toBe('read'); const read = report as Extract; - expect(read.tables.map((f) => f.table)).toEqual(['sys_scim_provider']); - // The negative controls, named individually so a failure says which one moved. - for (const legitimate of ['sys_user', '_objectstack_sequences', 'orphan_widget', 'app_leftovers']) { + expect(read.tables.map((f) => f.table)).toEqual([STRANDED]); + // The negative controls, named individually so a failure says which moved. + for (const legitimate of NEGATIVE_CONTROLS) { expect(read.tables.map((f) => f.table)).not.toContain(legitimate); } - expect(read.physicalTables).toBeGreaterThanOrEqual(5); - }, 120_000); + expect(read.physicalTables).toBeGreaterThanOrEqual(NEGATIVE_CONTROLS.length + 1); + }, 180_000); it('goes SILENT once the stranded table is the only thing that changes', async () => { // The other direction of the same measurement: remove the orphan, leave @@ -161,29 +221,15 @@ describe('os migrate plan — unmanaged tables, against a real database (#13204) await stack!.shutdown(); stack = null; - const surgeon = new SqlDriver({ - client: 'better-sqlite3', - connection: { filename: dbFile }, - useNullAsDefault: true, - }); - const k = (surgeon as unknown as { knex: any }).knex; - await k.schema.dropTable('sys_scim_provider'); - await k.destroy(); - - stack = await bootSchemaStack({ - jsonOutput: false, - projectRoot: dir, - databaseUrl: dbFile, - deferSchemaDdl: true, - readOnlyProbe: true, - composeHostStack: true, - }); + await dropTable(STRANDED); + stack = await bootLikeMigrate(); const after = await sweep(); expect(after.status).toBe('read'); expect((after as { tables: unknown[] }).tables).toEqual([]); // Still a real sweep, not an early return: the negative controls are all // still in the database. - expect((after as { physicalTables: number }).physicalTables).toBeGreaterThanOrEqual(4); - }, 180_000); + expect((after as { physicalTables: number }).physicalTables) + .toBeGreaterThanOrEqual(NEGATIVE_CONTROLS.length); + }, 240_000); }); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.test.ts b/packages/cli/src/commands/migrate/unmanaged-tables.test.ts index f013e3788b..f2a3539511 100644 --- a/packages/cli/src/commands/migrate/unmanaged-tables.test.ts +++ b/packages/cli/src/commands/migrate/unmanaged-tables.test.ts @@ -69,15 +69,20 @@ function fakeDriver(opts: { /** Catalog rows in the sqlite shape the sweep's own SELECT produces. */ const rows = (...names: string[]) => names.map((name) => ({ table_name: name })); +/** A composition that mirrors the deployment — the premise the sweep requires. */ +const MIRRORED = { hostConfigLoaded: true, hostConfigPath: '/app/objectstack.config.ts' }; + async function sweep(opts: { managed?: string[] | null; declared?: Array<{ name: string }>; client?: string | null; answer?: unknown | (() => unknown); + composition?: { hostConfigLoaded: boolean; hostConfigPath: string | null }; }): Promise { return collectUnmanagedTables({ driver: fakeDriver(opts), declaredObjects: opts.declared ?? [], + composition: opts.composition ?? MIRRORED, normalize, }); } @@ -257,6 +262,31 @@ describe('collectUnmanagedTables — every no-answer path is unreadable, not emp expect(report).toMatchObject({ status: 'read', tables: [{ table: 'sys_scim_provider' }] }); }); + it('is unreadable when no host config was loaded — the declaration set is knowingly partial', async () => { + // Measured: with a compiled artifact and NO config, the composed set is the + // artifact plus the platform FLOOR — ten objects, of which the `sys_*` half + // is `sys_metadata` + its four siblings, `sys_migration`, + // `sys_migration_journal`, `sys_metadata_activation`, `sys_secret`. A + // database carrying the other ~40 platform tables would have every one of + // them reported. That is the cry-wolf shape, and it is UNMEASURED, not + // false. + const noConfig = await sweep({ + managed: ['sys_metadata'], + answer: rows('sys_user', 'sys_session', 'sys_account'), + composition: { hostConfigLoaded: false, hostConfigPath: null }, + }); + expect(noConfig.status).toBe('unreadable'); + expect((noConfig as { detail: string }).detail).toContain('no host config'); + + const brokenConfig = await sweep({ + managed: ['sys_metadata'], + answer: rows('sys_user'), + composition: { hostConfigLoaded: false, hostConfigPath: '/app/objectstack.config.ts' }, + }); + expect(brokenConfig.status).toBe('unreadable'); + expect((brokenConfig as { detail: string }).detail).toContain('/app/objectstack.config.ts'); + }); + it('is unreadable when the managed map cannot be read', async () => { const report = await sweep({ managed: null, answer: rows('sys_scim_provider') }); expect(report.status).toBe('unreadable'); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.ts b/packages/cli/src/commands/migrate/unmanaged-tables.ts index b430cd31e6..9ccb600de1 100644 --- a/packages/cli/src/commands/migrate/unmanaged-tables.ts +++ b/packages/cli/src/commands/migrate/unmanaged-tables.ts @@ -63,6 +63,22 @@ * reserved for platform-provided objects, and its module header records what a * fourth hand-rolled copy of the prefix heuristic cost the last time. * + * ## The declaration set has to be the deployment's own, or the question is unaskable + * + * "No declaration accounts for this table" is only answerable when the composed + * object set actually MIRRORS what this deployment's `os serve` boot registers. + * On a project with a compiled artifact and no host config, the composed set is + * the artifact plus the platform FLOOR — measured: `sys_metadata` and its four + * siblings, `sys_migration`, `sys_migration_journal`, `sys_metadata_activation`, + * `sys_secret`, and nothing else — so a database carrying the other ~40 + * platform tables would have every one of them reported. That is the cry-wolf + * failure, from a knowingly partial premise rather than from a wrong predicate. + * + * So the sweep runs only when `composition.hostConfigLoaded` is true — the same + * discriminator #12953's ruling kept for consumers, and for the same reason: a + * composition that does not mirror the deployment produces an UNMEASURED + * result, and an unmeasured result must say so rather than render as findings. + * * ## ⛔ "Could not look" is never reported as "nothing found" * * Every path that fails to obtain an answer returns {@link UnmanagedTablesUnreadable} @@ -297,13 +313,27 @@ const unreadable = (detail: string): UnmanagedTablesUnreadable => ({ * * @param driver the driver whose managed set the plan diffed. * @param declaredObjects everything the booted stack registered (`stack.allObjects()`). + * @param composition what the boot composed — read for `hostConfigLoaded`, the + * premise above. * @param normalize `normalizeRows` — flattens the three dialect result shapes. */ export async function collectUnmanagedTables(opts: { driver: unknown; declaredObjects: readonly unknown[]; + composition: { hostConfigLoaded: boolean; hostConfigPath: string | null }; normalize: (result: unknown) => Record[]; }): Promise { + if (!opts.composition.hostConfigLoaded) { + return unreadable( + opts.composition.hostConfigPath === null + ? 'this project has no host config, so the composed object set is the compiled artifact plus the ' + + 'platform floor rather than what `os serve` registers — against a knowingly partial declaration ' + + 'set, "declared by nothing" is UNMEASURED rather than false' + : `the host config ${opts.composition.hostConfigPath} did not load, so the composed object set covers ` + + 'only a fraction of this deployment — "declared by nothing" is UNMEASURED against it', + ); + } + const managed = readManagedTableNames(opts.driver); if (managed === null) { return unreadable( From 275933f45005aa9ac19da84a0ee5dd1584f05f99 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 16:26:31 +0000 Subject: [PATCH 3/3] refactor(cli): move the unmanaged-table sweep out of the oclif command directory (#13204) Every compiled file under dist/commands is loaded as a command (oclif.commands.glob), so a helper module there made every 'os' invocation print 'command migrate:unmanaged-tables not found'. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k --- .../migrate-plan-unmanaged-table-findings.md | 11 +++++++++++ packages/cli/src/commands/migrate/plan.ts | 4 ++-- .../unmanaged-tables.integration.test.ts | 9 ++++++++- .../migrate => utils}/unmanaged-tables.test.ts | 0 .../migrate => utils}/unmanaged-tables.ts | 18 ++++++++++++++---- 5 files changed, 35 insertions(+), 7 deletions(-) rename packages/cli/src/{commands/migrate => utils}/unmanaged-tables.integration.test.ts (94%) rename packages/cli/src/{commands/migrate => utils}/unmanaged-tables.test.ts (100%) rename packages/cli/src/{commands/migrate => utils}/unmanaged-tables.ts (95%) diff --git a/.changeset/migrate-plan-unmanaged-table-findings.md b/.changeset/migrate-plan-unmanaged-table-findings.md index 7a9055d24d..994e52e834 100644 --- a/.changeset/migrate-plan-unmanaged-table-findings.md +++ b/.changeset/migrate-plan-unmanaged-table-findings.md @@ -41,6 +41,17 @@ Shards are folded onto their base before the membership test, the declared set is unioned into the managed set, and the driver's own scratch tables (`_objectstack_sequences`, `__os_mig_*`) carry no reserved prefix. +**The sweep runs only where the question is answerable.** "No declaration +accounts for this table" needs a composed object set that MIRRORS what this +deployment's `os serve` registers, so the sweep requires +`composition.hostConfigLoaded` — the same discriminator #12953 kept for +consumers. On a project with a compiled artifact and no host config the composed +set is the artifact plus the platform floor (measured: `sys_metadata` and its +four siblings, `sys_migration`, `sys_migration_journal`, +`sys_metadata_activation`, `sys_secret`), against which a real database's other +platform tables would every one be reported — so that shape reports +`unreadable` with the reason instead of findings. + Every path that fails to obtain an answer reports `status: 'unreadable'` with a reason — a missing seam, an unrecognised dialect, a catalog read that threw, a seam that returns no result set — and never an empty list. Non-SQL drivers are diff --git a/packages/cli/src/commands/migrate/plan.ts b/packages/cli/src/commands/migrate/plan.ts index ffe1bbad03..0c29d0926e 100644 --- a/packages/cli/src/commands/migrate/plan.ts +++ b/packages/cli/src/commands/migrate/plan.ts @@ -29,7 +29,7 @@ import { collectUnmanagedTables, renderUnmanagedTables, type UnmanagedTablesReport, -} from './unmanaged-tables.js'; +} from '../../utils/unmanaged-tables.js'; import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; import { resolveTenancyPosture, @@ -182,7 +182,7 @@ export default class MigratePlan extends Command { // ⛔ And it is NOT `composition.coverage`: coverage says what this plan // EXAMINED of what the deployment declares, this says what exists that // no declaration accounts for. Merged, "examined and clean" would be - // indistinguishable from "never looked at". See `./unmanaged-tables.ts`. + // indistinguishable from "never looked at". See `../../utils/unmanaged-tables.ts`. const { normalizeRows } = await import('@objectstack/metadata-protocol'); const unmanagedTables: UnmanagedTablesReport = await collectUnmanagedTables({ driver: stack.driver, diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts b/packages/cli/src/utils/unmanaged-tables.integration.test.ts similarity index 94% rename from packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts rename to packages/cli/src/utils/unmanaged-tables.integration.test.ts index af2e54b9d2..a5e710588b 100644 --- a/packages/cli/src/commands/migrate/unmanaged-tables.integration.test.ts +++ b/packages/cli/src/utils/unmanaged-tables.integration.test.ts @@ -38,7 +38,14 @@ import { createRequire } from 'node:module'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { SqlDriver } from '@objectstack/driver-sql'; -import { bootSchemaStack, type SchemaStack } from '../../utils/schema-migrate.js'; +// Module-top side-effect load, paid during COLLECTION rather than inside a +// clocked test body: `sweep()` below reaches `normalizeRows` through a dynamic +// `import()`, and this package resolves that specifier through `dist/`, so the +// first call would transform that dependency's whole module graph while a +// `testTimeout` is running (`check:test-source-alias`; measured 3.1-3.6s idle, +// 20.26s on a starved core). This decides only WHERE the load is paid. +import '@objectstack/metadata-protocol'; +import { bootSchemaStack, type SchemaStack } from './schema-migrate.js'; import { collectUnmanagedTables, type UnmanagedTablesReport } from './unmanaged-tables.js'; const require_ = createRequire(import.meta.url); diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.test.ts b/packages/cli/src/utils/unmanaged-tables.test.ts similarity index 100% rename from packages/cli/src/commands/migrate/unmanaged-tables.test.ts rename to packages/cli/src/utils/unmanaged-tables.test.ts diff --git a/packages/cli/src/commands/migrate/unmanaged-tables.ts b/packages/cli/src/utils/unmanaged-tables.ts similarity index 95% rename from packages/cli/src/commands/migrate/unmanaged-tables.ts rename to packages/cli/src/utils/unmanaged-tables.ts index 9ccb600de1..47b40f229e 100644 --- a/packages/cli/src/commands/migrate/unmanaged-tables.ts +++ b/packages/cli/src/utils/unmanaged-tables.ts @@ -79,6 +79,16 @@ * composition that does not mirror the deployment produces an UNMEASURED * result, and an unmeasured result must say so rather than render as findings. * + * ## Why this lives in `utils/` and not beside `plan.ts` + * + * `package.json`'s `oclif.commands` declares `{ strategy: 'pattern', target: + * './dist/commands', glob: '**' + '/*.js' }`, so EVERY compiled file under + * `commands/` is loaded as a command — and before this module moved here, every + * `os` invocation printed `command migrate:unmanaged-tables not found` from + * oclif's `findCommand`. Measured on the built CLI. Every other module the + * migrate commands share (`schema-migrate`, `schema-migration-plugins`, + * `migrate-occupancy-gate`) already lives here for the same reason. + * * ## ⛔ "Could not look" is never reported as "nothing found" * * Every path that fails to obtain an answer returns {@link UnmanagedTablesUnreadable} @@ -91,10 +101,10 @@ import { PLATFORM_OBJECT_PREFIXES, StorageNameMapping, hasPlatformObjectPrefix } from '@objectstack/spec/system'; // The result-set/no-answer predicate `os migrate duplicates` already publishes // (#10677). Imported rather than re-spelled: a second copy would drift, and the -// two commands must agree on what "the seam did not answer" looks like. Both -// modules are siblings under `commands/migrate/`, and the import costs nothing -// `plan.ts` does not already load. -import { isResultSet } from './duplicates.js'; +// two commands must agree on what "the seam did not answer" looks like. It +// costs nothing `plan.ts` does not already load — that module's own heavy value +// imports are deliberately lazy, for the reason its header gives. +import { isResultSet } from '../commands/migrate/duplicates.js'; /** How a raw SELECT is issued against the driver the plan diffed. */ export type PlannedDriverExec = (sql: string, params?: unknown[]) => Promise;