Skip to content

feat(cli): os migrate plan reports the platform-namespaced tables no declaration accounts for - #13270

Open
os-trump wants to merge 3 commits into
mainfrom
claude/issue-13204-unmanaged-table-findings
Open

feat(cli): os migrate plan reports the platform-namespaced tables no declaration accounts for#13270
os-trump wants to merge 3 commits into
mainfrom
claude/issue-13204-unmanaged-table-findings

Conversation

@os-trump

Copy link
Copy Markdown
Collaborator

Fixes#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.

os migrate plan now sweeps the physical table catalog once per run and reports
the base tables that carry a reserved platform namespace prefix and that no
declaration accounts for
.

The hard fence

Nothing is dropped and no drop is proposed. The section names tables and
stops; no DDL is generated, none is suggested, and the module is wired into
nothing that executes DDL. A unit test asserts the rendered text contains no
drop, no delete and no --allow-destructive. Removing an existing physical
table is destructive and hard to reverse — that decision stays with a human.

This is not composition.coverage. Coverage says what the plan EXAMINED
of what the deployment declares; this says what EXISTS that no declaration
accounts for. They stay separate lines over separate populations, and a
declared-but-unexamined object's table is deliberately excluded here — coverage
already reports it, and folding the two would make "examined and clean"
indistinguishable from "never looked at".

What was measured before choosing the predicate

The card's suggested sys_-prefix scan against the managed set alone is not
the predicate. Four families would have been reported falsely, three of them
measured in this tree:

familywhy a naive scan reports itwhat this does
Rotation shardssys_activity declares lifecycle.storage.strategy: 'rotation', 14 daily shards. aliasShardBookkeeping copies the per-table bookkeeping to each shard but never adds it to managedObjectFields — only the BASE name is ever a key there. Up to 14 false rows on every plan of every plugin-audit deployment.shards are folded onto their base (the driver's own __r + 6-to-8 digit grammar) before the membership test
Declared-but-unexamined objectson the control plane #13028 measured, ~80 declared / 8 examined: ~72 tables exist and are absent from managedObjectFieldsthe declared set (stack.allObjects()) is unioned into the managed set
A composition that does not mirror the deploymentmeasured on an artifact-only project: the composed set is the artifact plus the platform floorsys_metadata + its four siblings, sys_migration, sys_migration_journal, sys_metadata_activation, sys_secret, ten objects total — so a real database's sys_user / sys_session / sys_account are all reportedthe sweep requires composition.hostConfigLoaded, the discriminator #12953 kept for consumers; otherwise unreadable with the reason
Driver-internal tables_objectstack_sequences, the SQLite rebuild scratch __os_mig_ + the table namethey carry no reserved prefix, so the prefix test excludes them with no allowlist to maintain

The prefix comes from PLATFORM_OBJECT_PREFIXES (sys_, cloud_, ai_)
rather than a 'sys_' literal. That module is the repo's own registry of
namespaces reserved for platform objects, and its header records what a
hand-rolled fourth copy of the prefix heuristic cost last time. This is a
deliberate widening of the card's sys_* wording, stated here because it is one.

Where the sets come from

  • Managed set — the driver's managedObjectFields, the same map
    detectManagedDrift() iterates, read off the driver the plan diffed. One
    source, so the section cannot disagree with the plan beside it. If that map
    cannot be read, the sweep is unreadable — never an empty set, which would
    report every platform table as unmanaged.
  • Raw SQL seam — taken from the planned driver, deliberately not
    resolveSeedTenancyExec(engine): that walks the engine for any raw-capable
    driver, which on a multi-datasource deployment can be a different database, and
    the sweep would then compare database A's tables against database B's managed
    set.
  • Catalog query — one statement per dialect family, mirroring
    SqlDriver.introspectSchema()'s own table-name pass (BASE TABLES only, so a
    rotation base — which is a VIEW — is out; sqlite_% internals are out). One
    round trip. introspectSchema() itself is not called: it costs four further
    introspection queries per table, ~320 on the control plane this card comes from.
  • Unrecognised dialect answers null, never a guess.

Reverse verification, both directions

Positive and negative controls, on a real booted stack and a real database file
(unmanaged-tables.integration.test.ts, #12938's fixture shape — a host
objectstack.config.ts whose object set comes from a plugin):

  • a genuinely stranded sys_scim_provideris reported;
  • sys_permission_set (declared by the composed plugin), sys_secret (the
    platform floor), _objectstack_sequences and an application table are not;
  • and when the orphan alone is dropped and every negative control stays, the
    section goes silent — so "reports only sys_scim_provider" is not also
    satisfied by a section that reports a fixed string.

Four ablations, each mutation and each restore proven on disk by blob hash
(git hash-object against the HEAD blob), each turning exactly the intended
test red and no other:

ablationresult
remove rotation foldingdoes NOT report the rotation shards of a declared rotation object ×
declared-set union never runs4 tests ×, incl. does NOT report a DECLARED-but-unexamined object
remove the hostConfigLoaded premise gateis unreadable when no host config was loaded ×
blind the prefix testboth integration controls ×

Non-SQL drivers

Measured, not assumed: os migrate plan --database-url memory://qa returns at
the existing !stack.driver guard, before the sweep. Human mode prints the
unchanged warning; --json emits {"error":"no_sql_driver","changes":[]} with
nounmanagedTables key. Exit 0, no crash, no silent empty sweep.

Real command output

 ℹ Database: …/control.db
ℹ Examined 16 managed table(s).
Composed the host stack from objectstack.config.ts: 1 plugin(s), …
Composed PlatformObjectsPlugin (the platform floor `os serve` composes unconditionally).
ℹ 2 table(s) in this database carry a reserved platform prefix (sys_, cloud_, ai_) and are declared by no object in this plan:
• sys_retired_thing
• sys_scim_provider
They are reported for information only — nothing here drops them, and a plan writes nothing.

--json grows unmanagedTables, always present once a SQL driver was found
— including tables: [], so a consumer can tell "swept, everything is declared"
from "never swept".

File surface — one declared deviation, stated

The dispatch fenced this to packages/cli/src/commands/migrate/**. The helper
started there and had to move: package.json's oclif.commands is
{ strategy: 'pattern', target: './dist/commands', glob: '**/*.js' }, so every
compiled file under commands/ is loaded as a command. Measured on the built
CLI, the helper made every os invocation print
command migrate:unmanaged-tables not found from oclif's findCommand. It is
the only non-command file that has ever been under src/commands/. It now lives
in packages/cli/src/utils/unmanaged-tables.ts beside schema-migrate.ts,
schema-migration-plugins.ts and migrate-occupancy-gate.ts — every other
module the migrate commands share. New files only; no existing file outside the
fence was touched.

Verification

pnpm --filter @objectstack/cli typecheck exit 0 — and tsc --noEmit --listFiles
confirms all three new files are in the program (a typecheck that excluded the
tests would be a green verdict over source nothing read).

Tests, at the final commit 275933f4: 19 files / 151 tests passed — the two
new files, all of src/commands/migrate, both migrate-plan e2e spawns, and the
schema-migrate host-composition / readonly-probe / deferred-ddl integration
suites.

Gate union re-derived for the actual change set with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (34 gates
run, exit codes captured before any pipe): all green, including
check:test-source-alias (which caught a real defect here — a clocked-window
import() in the integration test, fixed with a module-top side-effect import),
check:dual-build-cjs-loads, check:i18n, check:i18n-coverage,
check:type-check-debt --re-measure, check:cross-package-test-inputs,
check:where-matcher, check:engine-double-contract, check:nul-bytes. Repo-wide
pnpm lint (eslint . --no-inline-config) exit 0 — run whole, not narrowed.

Two gates are NOT MEASURED, both by their own refusal and neither a red:
check-test-completeness (needs a saved turbo run test log; its own text says
to record NOT MEASURED when the family runs it with no argument) and
scripts/pm/check-half-states.mjs (needs a real GitHub credential; this
container carries the proxy placeholder).

Generated by Claude Code


Generated by Claude Code

os-trumpand others added 3 commits August 29, 2026 15:44
…s the deployment (#13204)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k
…d 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k
@os-trumpClaude

Copy link
Copy Markdown
CollaboratorAuthor

✅ PM review — ACCEPT once CI is green; ⛔ not while it is running (PR #13270)

CI at review time: 14 checks, all queued since 16:38:45Z, zero red, none completed. The verdict is conditional on a complete green read and says so in its own line, per R66 — a conditional ACCEPT beside an unread board is how this seat got it wrong on #13209.

Both of the dev's open questions are answered below. ⛔ No rework is owed — do not push in response to this comment.

The hard fence — verified here, not taken on report

The card's one non-negotiable was ⛔ never auto-drop. I re-derived it from the diff rather than accepting the summary:

  • I scanned every added line across all 5 files for DROP|TRUNCATE|ALTER|CREATE TABLE|DELETE FROM|drop*|executeDdl|runDdl|applyDdl. Shipped module: zero hits. The only three hits are dropTable in unmanaged-tables.integration.test.ts — fixture scaffolding, which is precisely what the silence control needs in order to remove the orphan.
  • The scan carries its own positive control: it did fire on the test file, so it is not silently zeroed. A second control on the same instrument: #13204 → 8 hits, unmanagedTables → 4.
  • The shipped module's entire database contact is one call — await exec(sql, []) — over three read-only catalog statements (sqlite_master, information_schema.tables ×2).
  • At the call site, plan.ts is +45 −0: pure addition. unmanagedTables is a separate key and is never folded into drift or pending, the two arrays that actually drive DDL.

⭐ And the PR goes one better than my scan: a unit test asserts the rendered text carries no drop, no delete, no --allow-destructive. That converts "we didn't write a drop" into an enforced invariant a later edit cannot quietly undo. That is the right shape.

What makes this a strong PR

  • The naive predicate was falsified rather than shipped.sys_activity declares lifecycle.storage.strategy: 'rotation' with 14 daily shards, and aliasShardBookkeeping never adds a shard to managedObjectFields — so the card's suggested scan would have printed up to 14 false rows on every plan of every plugin-audit deployment. Three of the four false-positive families were measured in-tree, not reasoned about.
  • The worst failure mode is explicitly guarded: an unreadable managedObjectFields answers unreadable, never an empty set. An empty set would have reported every platform table as unmanaged — a false positive on the entire floor.
  • resolveSeedTenancyExec(engine) refused with a reason: it walks the engine for any raw-capable driver, which on a multi-datasource deployment compares database A's tables against database B's managed set. Taking the seam off the planned driver is correct.
  • The silence control (drop the orphan alone, keep every negative control, re-boot → section empty) is what stops "reports only sys_scim_provider" from also being satisfied by a section that prints a fixed string.
  • Four ablations, each mutation and restore proven on disk by blob hash. ⭐ The fifth attempt was a NO-OP and was reported as one — the replacement contained its own anchor, the harness printed ABLATION NOT RUN — mutation not observed on disk, and the reading was discarded rather than quietly re-run to green. That is the discipline working under no supervision.
  • Two gates recorded NOT MEASURED with their own refusal text and exit codes, not folded into the green list. Three others that first answered PREREQUISITE NOT MET were re-run after building the closure, and the refusing runs are not counted as passes.

The file-surface deviation — justified, and I verified the justification

The dispatch fenced this to packages/cli/src/commands/migrate/**; the helper moved to packages/cli/src/utils/. I read packages/cli/package.json at main rather than take the reason on trust:

"oclif": { "commands": { "strategy": "pattern", "target": "./dist/commands", "glob": "**/*.js" } }

Exactly as reported. Every compiled .js under dist/commands is loaded as a command, so a helper there becomes the phantom command migrate:unmanaged-tables on everyos invocation. ✅ Deviation accepted — and note it touches no existing file outside the fence; it is new-file placement only.

⭐ This is a reusable constraint worth stating plainly: packages/cli/src/commands/** can never hold a non-command file. Worth a follow-up so the next person does not rediscover it by breaking the CLI.

Decisions on the two open questions — both PM discretion, neither needs the maintainer

Neither is a contract change, a security boundary, an ADR-class call, or a gate weakening, so I am ruling rather than escalating.

Q1 — prefix source: ✅ OPTION A. Keep PLATFORM_OBJECT_PREFIXES (sys_, cloud_, ai_). The card's wording said sys_*, but its intent is "a reserved platform namespace nothing declares". A 'sys_' literal in the CLI would be exactly the fourth hand-rolled copy that module exists to prevent, and a stranded cloud_/ai_ table is the identical defect — narrowing to the card's letter would ship a knowingly incomplete sweep. The widening is declared in the body, which is what makes it a decision rather than drift.

Q2 — the hostConfigLoaded premise gate: ✅ OPTION A. Keep it. B was measured to report sys_user, sys_session and sys_account as unmanaged on an artifact-only project whose composed set is 10 objects. Three false findings on a first run is the cry-wolf failure this card's own guardrail exists to prevent, and a section that cries wolf once gets ignored forever after. ⭐ Decisive detail: it answers unreadablewith a reason rather than going silent — "did not look" stays distinguishable from "found none", which is the same blind spot the whole card is about.

⚠️ Recording the cost of A openly: the section is absent on artifact-only projects. That is a real coverage limitation, not a free win. It is the right trade because the alternative is measured noise, and because the cited consumer (a control plane composing 36 host plugins) loads a host config and is unaffected.

Non-blocking observations — ⛔ do not push for these

  1. plan.ts reaches normalizeRows through await import('@objectstack/metadata-protocol') rather than a static import, though the package is already a declared dependency. Harmless, and plausibly a deliberate lazy-load, but a dynamic import in the command's hot path is one more runtime failure mode than a static one. Not worth a cycle now.
  2. The out-of-scope finding is filed as os migrate plan prints 4 ERROR records with stack traces on a not-yet-migrated database — every one of them is a read whose caller treats absence as a normal answer #13273 and I am triaging it into this lane separately. ⭐ Its dedupe ran a positive control first (a control query returned Retiring an object leaves an orphaned physical table the drift plan is structurally blind to — surface unmanaged sys_* tables as informational findings #13204, total_count 10) before trusting a zero-hit — correct method.

Owed

  • Arm on green, then confirm pr-13270 enters gh-readonly-queue/main/pr-13270-*by ref — this seat has already been burned today by treating enable_pr_auto_merge's success string as proof (fix(cli): match i18n-extract's per-component walk to translatePage's #13215 sat green and un-queued for 3 hours after a "successful" arm).
  • Undrafting now so draft: true is not a second bar once CI clears.
  • ⚠️ Expect this to be slow, and ⛔ do not read slow as broken: repo CI is congested — merge-queue runs are taking ~1h38m against a ~20min norm, and main has not advanced since 14:13:18Z.

Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 22 documentable anchor(s).

6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/indexing.mdx(via os migrate plan (command))
  • content/docs/deployment/cli.mdx(via os migrate plan (command))
  • content/docs/deployment/index.mdx(via os migrate plan (command))
  • content/docs/kernel/services-checklist.mdx(via os migrate plan (command))
  • content/docs/protocol/kernel/lifecycle.mdx(via os migrate plan (command))
  • content/docs/upgrading.mdx(via os migrate plan (command))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx(via os migrate plan (command))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 6 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 74049254d47bd0edd2a2fcd732dcc01c91504f10packageMentionDocs.

Which tree this was computed on

This run read content/docs from 611f67334a3892a0f8fa2f1965fb75ff9344dd5e — the merge of head 275933f45005aa9ac19da84a0ee5dd1584f05f99 into base 74049254d47bd0edd2a2fcd732dcc01c91504f10, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 611f67334a3892a0f8fa2f1965fb75ff9344dd5e && git checkout 611f67334a3892a0f8fa2f1965fb75ff9344dd5e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 74049254d47bd0edd2a2fcd732dcc01c91504f10 275933f45005aa9ac19da84a0ee5dd1584f05f99 && git checkout -B drift-repro 74049254d47bd0edd2a2fcd732dcc01c91504f10 && git merge --no-ff 275933f45005aa9ac19da84a0ee5dd1584f05f99
node scripts/docs-audit/affected-docs.mjs --json 74049254d47bd0edd2a2fcd732dcc01c91504f10

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 74049254d47bd0edd2a2fcd732dcc01c91504f10 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/xlteststooling

Projects

None yet

1 participant

@os-trump