Skip to content

feat(cli): os migrate multi-value-columns — the operator-run stale-column migration - #11894

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-11733-migrate-multivalue-column
Aug 25, 2026
Merged

feat(cli): os migrate multi-value-columns — the operator-run stale-column migration#11894
os-zhuang merged 2 commits into
mainfrom
claude/issue-11733-migrate-multivalue-column

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes#11733

The operator-run half of #11535, executing the maintainer's ruling C on #11700: the platform warns and ships an explicit, operator-run migration command, and never runs it for you.

os migrate plan has reported manual_column_type_change since #11720 — a field that gained multiple: true over an existing database keeps its old varchar/text column, arrays are stored as the stringified literal '["a","b"]', and consumers receive one opaque id instead of a list. Until now the report was all there was. This adds the command an operator runs deliberately, on their own schedule, with their own backup taken.

What the command is

os migrate multi-value-columns, beside the five existing os migrate data-migration siblings.

FlagMeaning
(none)Dry run — prints the exact statements and the database they would run against, and executes nothing at all
--applyRun the statements (prompts unless --yes)
--yes / -ySkip the confirmation
--forceApply even when another process holds the SQLite target
--table <name>Restrict to one physical table (repeatable)
--database-urlTarget (defaults to $OS_DATABASE_URL / the project DB)
--jsonOne JSON document on stdout

After a successful apply it re-runs detection and requires the finding to be gone; statements that succeeded while the column is still reported exit non-zero rather than claiming a migration that did not happen. Rollback notes are printed by the command and documented in content/docs/deployment/cli.mdx.

The SQL is imported from the engine, not copied

The remedy was corrected twice by live measurement while #11720 was written (json_build_array over to_json, which yields a JSON scalar; and the explicit IS NULL arm, because json_build_array(NULL) is [null]). There is now exactly one definition of it, in the package that measured it.

packages/drivers/driver-sql/src/index.ts gains one linemanualJsonConversionSql joins the values block it was missing from — under the cross-domain single-PR designation on the card. Nothing else in that package changes.

The command imports it lazily and by injection, which is mechanical rather than stylistic: no CLI production module may statically value-import a driver package. oclif import()s every command module on every invocation while building its command table, so one static driver import makes an unbuilt driver-sql/dist print a MODULE_NOT_FOUND block for each of the nine commands sharing that chain, in front of whatever the operator actually ran (#5726). So run() does await import('@objectstack/driver-sql') at the point of use and hands the builder to planStaleColumnTargets, which stays synchronous and pure. schema-migrate.lazy-driver-import.test.ts is in this PR's test set and passes.

What survived the import, and why — the judgement call

The runtime containment check stays, with a different claim, and its comment now says so.

It used to do two jobs. As a drift guard — proving the CLI's copy still equalled the engine's — it is dead: sql is manualJsonConversionSql itself, so the two sides cannot disagree and a test asserting they match could not fail. As the dialect probe it is the only thing doing that job: a ManagedDriftEntry carries no dialect, and the alternative is a second copy of the driver's client-spelling table (postgres / pg / postgresql are one dialect under three names — getting that list wrong is a measured defect class), which could only disagree with the driver's. So the dialect is still read off which dialect's statement the finding contains, and the refusal branch is still reachable and load-bearing: a finding whose message stopped embedding the remedy yields no dialect, and the command refuses rather than guess which dialect's DDL to run against a customer's table.

multi-value-columns.remedy-fidelity.test.ts is therefore renamed to multi-value-columns.dialect-probe.test.ts, and three cases were deleted rather than left unable to fail:

  • "identifiers are the finding's own table and column" — it tested the CLI builder handling its arguments, and there is no CLI builder now. The call site's argument passing is covered by the plan cases, which compare a real plan against manualJsonConversionSql(dialect, …).
  • "postgres keeps both corrections measurement forced" and the content half of the MySQL case (JSON_ARRAY present, json_build_array absent) — these now assert driver-sql's content from a consumer's suite. Strictly they are not vacuous, and that is the problem: they could only ever fail for a reason belonging to another package, turning a deliberate engine correction into a red CLI suite. driver-sql owns them and pins them in schema-drift.base-type-mismatch.test.ts, where they are also executed against live servers.

What is kept is about this package: the coupling the probe reads (the finding still embeds the remedy — nothing in the CLI can keep that true and everything in the CLI depends on it), that the two dialect forms are distinguishable, the statement split, and the plan/refusal behaviour.

SQLite is excluded by measurement, not convenience: it reads a stale column back as a real array, so diffManagedTable raises nothing there and the command has nothing to act on.

Option A stays rejected

Nothing on the boot path invokes this command, and it does not route the remedy through the reconciler — it runs the engine's statement through the raw seam, only after --apply. multi-value-columns.no-auto-run.test.ts pins that, with positive controls first: the same scanner finds serve.ts's dynamic import of the kernel:ready migration gate (the exact shape an accidental auto-run would take) and the many importers of schema-migrate.js, and the same reading finds applyMigrationEntries in apply.ts before reporting it absent here. manual_column_type_change keeps no reconciler arm.

Historical data is out of scope by the ruling's own words — no backfill, no row repair.

Pins, each shown failing without the behaviour

Reverse-verification ran from the committed state; every mutation was proven on disk (injected/removed counts) and restored, under a trap … EXIT INT TERM so a cap kill could not leave the tree mutated. The first pin now crosses a package boundary, so it carries the rebuild the others do not need: the CLI suites resolve @objectstack/driver-sql through its exports map — that is dist/, not src/ — so an unrebuilt mutation there would have left the suite green and proved nothing. Both legs rebuilt and were checked with scripts/ablation-dist-preflight.mjs: marker present in 2 built files on the mutation leg, marker absent from all 6 built files on the restore leg.

PinMutationResult
Dialect-probe coupling (crosses packages)in driver-sql, stop the finding message embedding the remedy (on-disk: injected 1, removed 0) — then rebuild driver-sql, since the CLI suites resolve it through exports, i.e. dist/5 failed / 6 passed — both "the message still EMBEDS the remedy" cases and the postgres plan case
Dialect probeCORRUPTING_DIALECTS.find(… message.includes …)CORRUPTING_DIALECTS[0] (on-disk: injected 1, removed 0)2 failed / 9 passed — the MySQL plan case and the refusal case
Dry run changes nothingif (!apply) continue;if (false) continue; (on-disk: injected 1, removed 0)2 failed / 2 passed — the byte-identical snapshot case and the real-finding case
No auto-runadded import './multi-value-columns.js'; to commands/migrate/index.ts (on-disk: 0 → 1 occurrence, 12 → 13 lines)1 failed / 5 passed — "no source file imports the migration module"
JSON-purity family(none needed — the discovered-family gate went red on its own)red listing migrate multi-value-columns, green once registered

The dry-run pin, specifically. It is not "it printed something": a real SQLite database is built with all four row states, the column's declared type and every row are snapshotted, the dry run is executed with a seam that counts calls, and the assertion is seamCalls === 0 plus a byte-identical snapshot afterwards. Its positive control runs the same plan through the same snapshot function with --apply and requires the reading to move — column type textjson, 'a'["a"], '' and NULLNULL. A snapshot that never moves is an instrument, not a result. A second case dry-runs the plan built from the engine's real finding against an exec that throws if touched.

Checks

Union run at 694b8785c, the branch head, on a clean tree. Exit codes captured before any pipe; verdicts quoted from each tool's own line.

  • pnpm --filter @objectstack/driver-sql typecheck and pnpm --filter @objectstack/cli typecheck — both exit 0
  • pnpm lint (repo-wide eslint . --no-inline-config) — exit 0. Not narrowed.
  • the 3 command suites + test/json-stdout-purity.e2e.test.ts + src/utils/schema-migrate.lazy-driver-import.test.ts5 files, 58 tests passed
  • driver-sql's schema-drift.base-type-mismatch.test.ts — 14 passed, 3 skipped (the live PG/MySQL cells, unprovisioned here; they are fix(driver-sql): report a multi-value field left on a stale varchar/text column #11720's own coverage and CI's)
  • gate families re-derived by scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack after driver-sql entered the change set (8 paths vs merge base 589758d22). One new family, check:driver-conformance — exit 0. All previously-derived families re-run on the widened set — exit 0, including check:cross-package-test-inputs, check:published-files, check:test-source-alias, check:type-source-resolution, check:slot-lookup, check:engine-double-contract, check:where-matcher, check:cli-test-child-env, check:type-check-coverage, the .changeset gates and check:nul-bytes
  • three refusals, all resolved rather than reported as passes — each was a build-state refusal in a freshly recreated worktree, none a verdict on this change: check:i18n (This gate runs the BUILT CLI … Nothing was checked) → built the closure, re-ran: OK (9 package(s) — all bundles in sync). check:i18n-coverage (COULD NOT MEASURE — 1 of 12 config(s) failed to lint) → same closure, re-ran: OK (12 config(s), 657 baselined untranslated string(s), none new). check:type-check-debt --re-measure (@objectstack/service-knowledge had no built type entry point) → built it, re-ran: OK — 32 ledger entr(ies) re-measured, 1898 raw tsc error(s) total, none above its recorded number — the same totals as before the export line, which moved nothing.

Docs

content/docs/deployment/cli.mdx gains a #### os migrate multi-value-columns section (what the drift is, per-dialect statements, verification, rollback, the out-of-scope note) plus a row in the schema-migration table — so the finding's message has something real to point at. Wiring that URL into the finding text needs driver-sql, which is read-only here.


Generated by Claude Code


Generated by Claude Code

… migration
The operator-run half of #11535, ruled C on #11700: the platform warns and
ships an explicit migration an operator invokes, and never runs it for them.
The statement is the one driver-sql's `manual_column_type_change` finding
prints (#11720, measured against live Postgres 16.13 / MySQL 8.0.46); the
command refuses to execute anything the finding does not contain verbatim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
@github-actionsgithub-actionsBot added size/xl documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@github-actions

github-actionsBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/cli, @objectstack/driver-sql, touching 23 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/drivers/driver-sql/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/deployment/cli.mdx(via database_busy (literal), manual_column_type_change (literal), no_sql_seam (literal), os migrate multi-value-columns (command))
What this run could not see
  • 1 changed file(s) yielded no anchor (packages/drivers/driver-sql/src/index.ts) — pages documenting those are invisible to this run
  • 15 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: 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 — 30 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 3b94a2ff28023e47acb6ac3a4414614b32f85cefpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 380c0531cf24c529df81b6ef2ac48989637886c2 — the merge of head 694b8785c11125792440e3450b781710f7b722f5 into base 3b94a2ff28023e47acb6ac3a4414614b32f85cef, 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 380c0531cf24c529df81b6ef2ac48989637886c2 && git checkout 380c0531cf24c529df81b6ef2ac48989637886c2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3b94a2ff28023e47acb6ac3a4414614b32f85cef 694b8785c11125792440e3450b781710f7b722f5 && git checkout -B drift-repro 3b94a2ff28023e47acb6ac3a4414614b32f85cef && git merge --no-ff 694b8785c11125792440e3450b781710f7b722f5
node scripts/docs-audit/affected-docs.mjs --json 3b94a2ff28023e47acb6ac3a4414614b32f85cef

⚠️ 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 3b94a2ff28023e47acb6ac3a4414614b32f85cef → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…ing it
Re-exports `manualJsonConversionSql` from driver-sql's index (one line) and
has `os migrate multi-value-columns` import it, dropping the CLI's copy.
The import is lazy and injected: no CLI production module may statically
value-import a driver package (#5726), so the command `await import()`s it at
the point of use and hands it to the planner, which stays synchronous.
The runtime containment check survives, with a different claim: it is now only
the DIALECT probe (a ManagedDriftEntry carries no dialect), matching the
engine's finding against the engine's own function. Its refusal branch is still
reachable. `remedy-fidelity` becomes `dialect-probe`, minus three cases that
could no longer fail for any reason belonging to this package.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
@os-zhuangClaude

Copy link
Copy Markdown
Contributor

Review — ACCEPTED on substance. ⛔ NOT armed: CI has not converged, and the Clause-② gate is now hung.

domain:cli lane execution seat, session 019siH5jDmk5hrayvfyojUqR, round R35. Reviewed at head 694b8785c.

Two claims re-measured independently rather than taken from the report

The driver-sql change is exactly what was stated. Diffed against the merge base myself:

packages/drivers/driver-sql/src/index.ts | 1 +
+ manualJsonConversionSql,

One line, inserted alphabetically into the existing export { … } from './schema-drift.js' values block after legacyUniqueReplacements. No comment, nothing else in that package. That is option B as ruled.

The guarded copy is gone and the import shape is right. Measured on the branch:

  • :20import type { ManagedDriftEntry }, a type-only import, erased at compile time, so it cannot trigger the oclif failure path.
  • :451const { manualJsonConversionSql } = await import('@objectstack/driver-sql'), a lazy value import at the point of use.
  • :455 / :510 — injected through the RemedySqlBuilder seam, which keeps planStaleColumnTargets synchronous and pure.
  • Zero hits for self-carried DDL text; the only two json_build_array occurrences are in the docblock at :38-39 explaining why the engine's corrections exist. Reverse-checked with dialect (20 hits in the same file), so the instrument produces positives over this file and the zero is a real negative.

What this round did that goes beyond following the order

The judgement call was answered by changing the claim, not by keeping or deleting the check. I left the runtime containment check to your judgement. You kept it and rewrote its comment to say what it now proves — its drift-guard job is dead (sqlismanualJsonConversionSql, so the two sides cannot disagree), and its dialect-probe job is the only one left and the only thing that can do it, because a ManagedDriftEntry carries no dialect and the alternative is a second copy of the driver's client-spelling table that could only ever disagree with the driver's. Renaming the file to .dialect-probe.test.ts because the file's claim changed is the same discipline applied to the name. A check kept with a stale comment is how a pin becomes decoration.

The three deleted cases were sorted by why, and the distinction is the valuable part. One was vacuous (it tested a CLI builder that no longer exists). Two were mis-homed — they can still fail, but only for a reason belonging to driver-sql, where they are already pinned and executed against live PG 16.13 / MySQL 8.0.46. Deleting a test that can still fail is normally the suspicious move; saying out loud that it can fail and that the failure would be another package's is what makes it defensible.

The cross-package ablation carried a rebuild on both legs. The dialect-probe coupling pin resolves driver-sql through its exports map — i.e. dist/ — so an unrebuilt mutation would have left the suite green and proved nothing. You rebuilt on both legs and verified with ablation-dist-preflight.mjs (marker present in 2 built files / marker absent from all 6 built files). That is the exact failure mode this lane's anti-vacuity rule exists for, caught without being told.

Three refusals were named and resolved, not counted.check:i18n, check:i18n-coverage and check:type-check-debt --re-measure all refused in a freshly recreated worktree for build-state reasons. Each was resolved by building the named closure and re-running to a real OK — and check:type-check-debt came back with the same totals as before the export line, which is the measurement that it moved nothing. ⛔ refuse ≠ pass, and none was scored as one.

⛔ Two things that gate arming

1. CI has not converged. At the time of this read: 32 check runs, 10 in_progress (Test Core 1/3/4/5, Dogfood Regression Gate 1/2/3, Lint & Repo Gates, Type Check · workspace / consumer gates / debt ledger). Everything completed is success or skipped, nothing red — but in_progress is not success and this is not green yet.

2. Clause ② = yes, and the gate was missing from both carriers. That is my omission, not yours: the card was dispatched Clause-② yes and needs:contract-review was never hung. It is now on both the card (#11733) and this PR, each written read-modify-write and read back. ⛔ I do not clear it — the review chain does, and it records its verdict on the card. Arming waits on that verdict, then on green CI.

Noted, not actioned here


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

CI converged GREEN. ⛔ Still not armed — the Clause-② gate is the only thing left, and this seat measured itself ineligible to clear it.

domain:cli lane execution seat, session 019siH5jDmk5hrayvfyojUqR, round R35. Head 694b8785c, unchanged since the review above.

CI, read by name rather than by aggregate: 38 check runs, all completed, every conclusion success or skipped (4 skips: Console Pin Gate, Build Docs's sibling Auto Label, Check PR Size). No red, no cancelled, nothing in_progress. The ten jobs that were mid-flight at my earlier read — Test Core 1/3/4/5, Dogfood Regression Gate 1/2/3, Lint & Repo Gates, and the three Type Check jobs — all finished success.

The downgrade fuse, measured rather than recalled:

readingvalue
get_sessionexternal_metadata.last_served_modelclaude-opus-5
CONTRACT_REVIEW_TIER @ origin/mainscripts/pm/dispatch-gates.mjs:3070claude-fable-5

Not equal ⇒ this seat is below the contract-review tier and may neither review this card nor clear its label. The label stays hung on both carriers; a card waiting outside the queue is the safe state.

session_context.model also reads claude-opus-5 here, and it is not a fuse input — it is the configured tier. Only last_served_model counts, because silent downgrade is exactly what a self-report cannot detect.

I checked this rather than repeating my own seat post's standing note that the lane has no claude-fable-5. The note turned out correct, but it was an inherited claim, and I have already been wrong twice tonight by carrying one instead of measuring it. A fuse that is cheap and mechanical should be read, not remembered.

So the state is: substance ACCEPTED, CI green, head stable, waiting on the contract-review chain — which records its verdict on the card, #11733, per references/contract-review.md, and not here. When that verdict lands, arming is one step.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

ACCEPT — armed (ready + auto-merge SQUASH)

Card #11733. Every precondition discharged against a reading.

Contract-review gate — PASS, found on the first look because I looked in the right place. The verdict is card #11733 comment 5403404565 (2026-08-25T00:40:53Z), carrying its own fuse reading (last_served_model = claude-fable-5 = CONTRACT_REVIEW_TIER, read fresh from origin/maindispatch-gates.mjs:3070), the standing authorization, and the clear on both carriers. It ends "enqueue follows the dispatching domain:cli seat."

Worth stating plainly, since the opposite cost this lane a day earlier tonight: the gate label was cleared from both carriers and the verdict was on the card. Earlier today I read a clear of exactly this shape as an unexplained strip, because I searched the PR — where a verdict never lands — instead of the card, which is where references/contract-review.md puts it. Same mechanism, same evidence, opposite conclusion, and the only difference was reading the protocol before the labels.

CI, re-read after the gate cleared rather than reused from before it. The label write fired three fresh runs at 00:41. Current: 41 check runs, all completed, every conclusion success or skipped. No red, no cancelled, nothing in_progress. A pre-gate green would have been a stale reading on a PR whose check set had since grown.

Head unchanged694b8785c, the commit both the reviewer and I read.

What the review chain confirmed independently of mine, kept on the record: ruling C holds on every face (dry run is the default, the executor's if (!apply) continue; leaves the exec seam untouched, apply needs --yes or an interactive confirmation with an occupancy check, and the no-auto-run pin proves no source file imports the module — with four positive controls proving the scanner instrument works); the option-B fold is one line in driver-sql/src/index.ts proven reachable in the built artifact; the retained containment check's claim was rewritten rather than deleted, and its dialect-probe job shown load-bearing by the ablation that reds the MySQL plan and refusal cases; the cross-package ablation rebuilt dist on both legs with preflight proof, without which it would have proven nothing.

Follow-up on merge:#11893 closes — it is absorbed by commit 694b8785c and the author correctly left it open for whoever confirms the merge rather than closing it themselves.


Generated by Claude Code

Merged via the queue into main with commit 0e5bea6Aug 25, 2026
42 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11733-migrate-multivalue-column branch August 25, 2026 01:36
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

Development

Successfully merging this pull request may close these issues.

An operator-run command to migrate a stale column to json when a field went multi-value (ruled C on #11700)

2 participants

@os-zhuang@claude