Skip to content

fix(driver-sql): the stale multi-value column warning names os migrate multi-value-columns - #12012

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-11535-multi-value-column-signal
Aug 25, 2026
Merged

fix(driver-sql): the stale multi-value column warning names os migrate multi-value-columns#12012
os-warren merged 1 commit into
mainfrom
claude/issue-11535-multi-value-column-signal

Conversation

@os-warren

@os-warrenos-warren commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Part of #11535

The remaining half of the card, unblocked by os migrate multi-value-columns (#11733, 0e5bea6). The detection half landed in #11720; this makes the finding it emits point at the command that now exists.

The defect, measured on origin/main before touching anything

This is the line an operator meets on every restart of an affected database — captured through the real boot path (reconcileAndWarnDrift → logger), on live Postgres 16.13, at d63b014360:

[schema-drift] zztmp11535_task.tags: metadata declares a multi-value field (stored as `json`)
but the column is `character varying` — … ObjectStack will NOT change this column for you.
Migrate it by hand, in a transaction, with a backup taken first — dropping any index on the
column first, since a json column cannot carry a plain btree: ALTER TABLE …

Two things are wrong with it, and neither is cosmetic:

  1. "ObjectStack will NOT change this column for you" is false. It was true when it was written; os migrate multi-value-columns falsified it. The message tells the operator the command two lines below it does not exist.
  2. It sends them to hand-write DDL on a production table. The command they are not being told about dry-runs by default, prompts before applying, runs the statements in the order MySQL requires, and re-runs detection afterwards and exits non-zero if the finding has not cleared. Hand-running gets none of that.

The change

The message leads with the command and keeps the hand-run statement after it, for an operator without the CLI:

… receives one opaque id instead of a list (#11535). REMEDY: run "os migrate multi-value-columns"
— it is a DRY RUN by default that executes nothing and prints the statements; take a backup, then
re-run it with --apply. ObjectStack never migrates this column on its own: the boot path only
reports it and "os migrate apply" skips it, so nothing changes until you run that command. To do
it by hand instead, … ALTER TABLE "crm_case" ALTER COLUMN "assignee" TYPE json USING (CASE …);
Rows written while the column was stale may already hold a stringified array in a RELATED
single-value column; neither route repairs those.

Both operator-facing surfaces pick this up without touching either: renderPlan (packages/cli/src/utils/schema-migrate.ts:442) prints d.message verbatim, so os migrate plan shows it, and so does the boot warning.

⛔ What was NOT changed, deliberately

  • severity: 'error', category: 'needs_confirm' — untouched. Boot-gating is decided by category: runArtifactBootMigrationGate refuses a boot on category === 'destructive' and nothing else, at kernel:ready before the socket opens. Every database this finding describes is already serving — that is the premise of the user's report — so making the report louder must never become the outage. Pinned by the existing category case, which the diff leaves alone.
  • No load-time or write-time refusal was added. The platform still never migrates the column on its own, per the ruling on [Decision] 老客户把「选一个人」的字段改成「选多个人」时,平台该自己动客户的库,还是只报警要人来动?(#11535 的自动迁移半边) #11700 (route C: warn, and ship an explicit operator-run migration).
  • The dialect statement stays embedded VERBATIM. This is a contract, not formatting — see below.

⚠️ The cross-package coupling this change had to respect

planStaleColumnTargets (packages/cli/src/commands/migrate/multi-value-columns.ts:154) recovers the dialect by testing which dialect's statement the message contains:

constdialect=CORRUPTING_DIALECTS.find((d)=>message.includes(opts.sql(d,op.table,op.column)));

A ManagedDriftEntry carries no dialect. So a reword that paraphrased, wrapped, or line-broke the SQL would make every finding remedy_not_recognizedthe command this message now recommends would refuse to run, and nothing in driver-sql's own suite would have noticed. That is now pinned from the emitting side (keeps the statement VERBATIM, because the CLI recovers the dialect by containment, which reproduces the probe rather than describing it), and the risk is written into the emission site's comment.

Verified rather than reasoned: driver-sql rebuilt, then the CLI's three multi-value-columns suites run against that dist21 passed.

P0 confirmation (Zone 1b) — it still reproduces

Triage flagged this P0-suspect and left the engine seat to confirm. Re-measured end-to-end on live Postgres 16.13 at d63b014360, through the operator path (single-value table → metadata gains multiple: trueinitObjects → write):

  • the column stays character varying(255) while a fresh DB of the same metadata gets json;
  • the array reads back as the string["x","y"] (typeof === 'string', Array.isArray === false).

The corruption is still reachable. What has changed since the report is that it is no longer silent (#11720) and is now repairable by a command (#11733) — the "silent" in "silent data corruption" is gone; the reachability is not, and removing it would mean refusing the write or the boot, which this dispatch fences off and the #11700 ruling settles. Reported plainly rather than as a footnote.

Non-vacuity, both directions

A signal that fires on everything and one that never fires read equally green, so both are pinned:

Fires — unit (both dialects), and on a live Postgres boot: the line names the table, the physical type, the dry run, --apply, and carries the statement intact through the logger. Also pinned: the command appears before the raw SQL, so an operator who stops reading at the first ALTER TABLE has already passed it.

Does not fire — six shapes enumerated (migrated json column · single-value field · single-value narrow and widen width drift · SQLite · stale integer), asserting no message names the command and no manual_column_type_change op appears; two of those rows do produce a finding, asserted, so the loop reads real messages rather than passing over empty arrays. On a live database: after the remedy runs, a restart must stop recommending it.

⚠️ That last one caught a false green while being written. driftWarned is a per-instance throttle keyed by driftKey(d), so re-initObjects-ing the same driver stays silent whether or not the drift cleared — the assertion would have passed against nothing. It now boots a second driver, which is what a restart actually is, and asserts the pre-migration boot did name it so the silence belongs to the repair.

Ablation

Direction predicted in advance, mutation proven on disk before any result was read, restore under trap … EXIT INT TERM.

  • Mutation: the ${MULTI_VALUE_COLUMN_REMEDY_COMMAND} interpolation removed from the message (anchor verified unique: 1 occurrence). ⚠️ Mutating the constant would have been void — the suite imports it, so a mutated constant would be compared against a mutated message and pass.
  • On disk, before reading anything: removed-text 1 → 0, injected marker 0 → 1. The script aborts with exit 90 if either count is wrong.
  • Predicted: 3 red — the two "names the command" cases (unit + live boot) and the live remedy case via its non-vacuity line; the negative-direction case stays green, because with nothing naming the command it still passes.
  • Observed: Tests 3 failed | 21 passed | 2 skipped — exactly those three.
  • No rebuild leg, and the ablation itself proves why: the suite imports the subject by a relative specifier (./schema-drift.js) from inside its own package, so vitest resolves src/. The source was mutated with no rebuild and the suite went red — had it stayed green, that would have been the stale-dist signature. Not a dist-leg ablation, so the strippable-marker caveat does not apply.
  • Restore: marker 0, anchor 1, diff against the pre-mutation copy byte-identical, git status clean.

Verification

Gate union derived, not recalled: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (asserted against this checkout's origin; 3 paths vs merge base). 14 path-matched + 6 convention-triggered families, all run, exits captured before any pipe. All green, plus:

  • pnpm --filter @objectstack/driver-sql typecheck (tsc --noEmit) — 0
  • pnpm lint — whole repo, eslint . --no-inline-config, 0. Not narrowed.
  • drift family, 6 files, live Postgres — 99 passed | 2 skipped
  • this suite — 24 passed | 2 skipped (was 18 passed before this change)
  • CLI consumers against the rebuilt dist21 passed

Declared narrowings, both with their measurement:

  • check:type-check-debt --re-measure — not run; needs the whole workspace closure built, and it re-runs tsc per ledger entry. driver-sql is in neither the EXEMPT nor the TEST_DEBT block of scripts/check-type-check-coverage.mjs (measured here, not inherited), so this change cannot move a ledgered count. The structural half passed, and tsc --listFiles shows both changed files inside the package's own program, which exits 0.
  • live MySQLmysqld is not installed in this container, so that matrix cell is a named skip. Postgres carries the live legs; CI runs both.

All 20 were read at the final commit 765f07814e in one run, with a clean working tree asserted in the same run (ALL GATES ABOVE READ AT: 765f07814e · working tree 0 change(s)). An earlier attempt was cut off mid-run by the container's 10-minute foreground cap under lock contention; it is not the run reported here.

Generated by Claude Code

…y command
The finding that reports a multi-value field left on a stale varchar/text
column opened its remedy with "ObjectStack will NOT change this column for
you. Migrate it by hand" and then printed raw SQL. That became false when
`os migrate multi-value-columns` shipped: there is now an operator-run
command that does exactly this, with a dry run as the default, a prompt,
and a post-run re-detection that exits non-zero if the finding has not
cleared. Operators were being sent to hand-write DDL on a production table
while the safer route sat one command away, unnamed.
The message now leads with the command and keeps the hand-run statement
after it. Both surfaces print `message` verbatim, so the boot warning and
`os migrate plan` both pick it up.
Unchanged, deliberately: severity `error` + category `needs_confirm`. The
artifact boot gate refuses a boot on category === 'destructive' and nothing
else, and every database this finding describes is already serving. No
load-time or write-time refusal was added.
The dialect statement stays embedded VERBATIM — a contract, not formatting:
a ManagedDriftEntry carries no dialect, so the CLI recovers one by testing
which dialect's statement the message contains. Now pinned from the
emitting side too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 3 documentable anchor(s).

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

  • content/docs/deployment/cli.mdx(via needs_confirm (literal))

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

  • content/docs/releases/v17.mdx(via needs_confirm (literal))

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
  • 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 — 9 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 2cc71222459e91964e883419611a820c28302429packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 2cc71222459e91964e883419611a820c28302429 → 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/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-warren@claude