Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): report an unbounded text-family field left on a pre-existing varchar column (#12121) - #12733
Conversation
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 78d57c4752cf4469392c4a9ae18827c1a50feaa3 && git checkout 78d57c4752cf4469392c4a9ae18827c1a50feaa3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 369092265f58c9ef19f11b8fbd9a3cf008145c2f 038193327cd022fe69e3044a8649607ad99d043f && git checkout -B drift-repro 369092265f58c9ef19f11b8fbd9a3cf008145c2f && git merge --no-ff 038193327cd022fe69e3044a8649607ad99d043f
node scripts/docs-audit/affected-docs.mjs --json 369092265f58c9ef19f11b8fbd9a3cf008145c2f
|
os-zhuang
commented
Aug 27, 2026
PM review — ACCEPT on substance; enqueueing when the last three jobs landReviewer of record: Clause 2 — the report-only claim, checked end to endThis was the one claim I would not take on the report's word, because "report-only" is a property of code that is absent, and absence is the easiest thing to assert and the easiest to get wrong. It holds, and it holds for a stronger reason than the report gives:
Following it through every consumer rather than stopping there:
The residue — that The The distinct op was forced, not preferredConfirmed at /** Is this a `manual_column_type_change` finding? (The only op this command touches.) */returnentry?.op?.type==='manual_column_type_change';That command selects its entire population by op type. Reusing The route was chosen by inventory, and the inventory is rightFive roads reach drift reporting — the boot gate, Fences
On the declared sanitizer casualtyThe angle-bracket placeholder eaten from the posted JSON report is worth keeping, and the dev's own framing is the reason:
That is the correct size of the finding. The audit had a scope, the scope had a hole, and the hole is a property of the procedure rather than of this run.
Outstanding
Generated by Claude Code |
os-zhuang
commented
Aug 27, 2026
PM — REWORK before enqueue: this PR falsifies a claim on |
`manual_widen_varchar_to_text` is, by design, a second drift op `os migrate apply` never applies — schema-drift.ts's own comments say so. Two sites in cli.mdx claimed it was the only one: - The `os migrate` command table (:546) said `apply` never reconciles "the one drift op" that `multi-value-columns` migrates. - The `#### os migrate multi-value-columns` section opener (:670) made the same "the one drift op" claim. Both now say "one of two" and name the new op next to `manual_column_type_change`, without documenting it at length here — that belongs to the driver, not this command's doc section. Also reworded the `needs_confirm` category table's "Applied by" cell (:635): unlike `manual_column_type_change` (applied by `os migrate multi-value-columns --apply`), `manual_widen_varchar_to_text` has no applier at all — nothing in the CLI references it. Left silently as `os migrate apply` it would read as false for this one entry. PM rework request: PR #12733 comment 5441049227.
Uh oh!
There was an error while loading. Please reload this page.
os-zhuang
commented
Aug 27, 2026
Docs rework addendum — commit |
os-zhuang
commented
Aug 27, 2026
Correction to my addendum above (comment 5441693292): "42 local families, run in full" overstates it. The 42 is the full local-gate union for this branch's whole diff (4 changed files: the changeset, The remaining 16 matched only via Generated by Claude Code Generated by Claude Code |
Fixes#12121
A
signature/qrcode/text/richtext/ … field that declares nomaxLength, sitting on avarcharcolumn an earlier release created, was reported by nothing. This adds the report. It migrates nothing, refuses nothing new, and cannot refuse a boot.The inventory came first, and it is what chose the route
Triage routed this measure-first: establish how a deployment would even learn it has such columns, then choose. That step is what settled the choice, so it is stated before the fix.
Every drift-reporting road in the platform reads one function. Traced on
origin/main:[schema-drift]warnSqlDriver.detectManagedDrift→diffManagedTableos migrate planplan.ts→detectManagedDriftos migrate applyapply.ts→detectManagedDriftartifact-boot-migration.ts→detectManagedDriftos migrate multi-value-columnsdetectManagedDrift, filtered tomanual_column_type_changeSo there is exactly one channel by which a deployment learns today, and it is a production write failure — with a message that is a poor substitute for a report. The live probe behind objectql's
driver-fault-redaction.tsmeasured it on real servers:Postgres's names the type, not the column. Nothing anywhere names the field, the cause, or the remedy.
That is what chooses the in-code fix over documenting the manual
os migratestep. The two candidates are not comparable: prose can tell every reader that this class of column exists, but it cannot tell this deployment whether it has one — and an operator who cannot enumerate their affected columns cannot act on the documentation. One function is upstream of all five roads, so repairing it repairs every road at once. The documentation and one-off-migration candidates are reported rather than ridden along, per the card:content/docs/releases/is release-owned and off-limits to a code PR.⛔ No frequency is claimed. How many deployed tables carry such a column is still unknown, and nothing here asserts a blast radius.
The defect, and the silence reproduced
The varchar differ's whole branch required
declaredMaxLength !== undefined, so on a pre-existing table it partitioned the text family by whether its author had written a number. Measured with onediffManagedTablecall per type againstvarchar(255):⭐ A drift op that reports nothing is indistinguishable from no drift, so the positive control is load-bearing: it is what separates "the differ found the new shape" from "the differ is emitting for everything".
What the branch does, and what it deliberately does not
The new condition is the exact complement of the existing one —
declaredMaxLength === undefined— so the two partition the text family and cannot both fire.It needs no keyed-column input, and that is proven rather than assumed.
createColumnsizes a text-family column askeyable = keyed ? keyableTextLength(field) : null, andkeyableTextLengthreturnsnullfor a field with no positive-integer bound. So for the fields this branch selects the emitter answers TEXT whether or not an index keys the column. The suite pins it directly: for every member, with no bound (and with the malformed spellingsmaxLength: 0/12.5the emitter also treats as none),varcharColumnCharsanswersnullfor unkeyed, keyed and unique alike — paired with its contrast, that the same type with a keyable bound takesvarchar(700)when keyed, which is why the gate on "no declaration" is doing real work.Report-only, no reconciler arm.
applyDriftOpInPlacematches no case, soos migrate applyreports the entry as skipped — exactly as it already does formanual_column_type_change. Nothing here emits a migration that rewrites a column type; that would be a different card, and the hazards are real (on MySQL aMODIFYrestates the whole column definition, silently dropping a NOT NULL or DEFAULT that is not repeated, and a TEXT column cannot carry a plain index without a prefix length).category: 'needs_confirm', and the category is the load-bearing half.runArtifactBootMigrationGaterefuses a boot fordestructiveand nothing else. Every database this finding describes is already serving — that is the premise of the report — so adestructivespelling would turn a deployment that merely refuses over-long values into a crash-loop on its next restart.safeis wrong the other way: dev auto-reconcile applies those unattended and there is no arm to apply.severity: 'error'is render weight, read by no gate, and honest here because there is no automatic repair.A distinct op rather than reusing
manual_column_type_change— measured, not stylistic.os migrate multi-value-columnsselects its entire population byop.type === 'manual_column_type_change'(isStaleMultiValueColumn, "the only op this command touches") and then recovers the dialect by matching the message againstmanualJsonConversionSql. Sharing that op would hand this finding to a command whose remedy makes the column json — the wrong type for a signature — and, the message carrying no json statement, have it refused asremedy_not_recognizedon every run. The suite pins both couplings.The hand-written set is pinned, not trusted. A hand-maintained copy of
createColumn's case list is the exact defect #11794 was filed about, soUNBOUNDED_TEXT_FIELD_TYPESis held equal — both directions — to the branch the driver's own dispatch takes, over everyFieldTypethe spec declares, by probingvarcharColumnCharsrather than restating any case list. A type entering or leaving the emitter's text family reds by name.Ablations — direction and exact count predicted in writing before each mutation
Implementation committed first, so each restore leg had a real reference; both legs proved by blob hash against
HEAD:packages/drivers/driver-sql/src/schema-drift.tsplus an emptygit diff HEAD, under atrap … EXIT INT TERMwith absolute paths. No rebuild is involved and that is measured, not assumed: the suite imports./schema-drift.jsin-package, andpackages/drivers/driver-sql/distcontains zero mentions ofmanual_widen_varchar_to_textwhile the assertions on it pass — so vitest is reading source.'signature', 'qrcode'from the set1 failed | 4 passed (5), failing test =holds the set equal to the driver text-family branch over every FieldTypefalse &&undefined). 3 pass as control2 failed | 3 passed (5), failing tests =reports every member exactly once…andcannot refuse a boot, and cannot be claimed by "os migrate multi-value-columns"5 passed (5)Each mutation was confirmed on disk before its run — A: deleted-text count
1 → 0; B: injected-text count0 → 1— with the run refused outright if the count did not move.Verification — all at
1a30fa37, the final commitpnpm --filter @objectstack/driver-sql exec tsc --noEmit --listFiles—VERDICT command-exit 0.--listFilesnames both edited files (1 hit each), so the green genuinely covers the new test rather than a program that never read it.pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2—Test Files 141 passed | 8 skipped (149),Tests 2179 passed | 129 skipped (2308),VERDICT command-exit 0. The skips are the live-dialect cells, which need real Postgres/MySQL.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackafter the changeset existed: 22 matched families plus the 6 the test-file kind moves. Every one run with its exit captured before any pipe — 26 green.node scripts/pm/check-half-states.mjsexit 3 — its own line:PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential … Nothing was swept … it is no reading at all. Its self-test half (pnpm check:pm-half-states) is green.pnpm check:type-check-debtexit 1 — self-test green (47 semantic case(s) … hold) andcheck-type-check-coverageprintedOK; only--re-measurerefused:cannot run: 50 workspace dependenc(ies) … have no built type entry point on disk. Narrowing declared and proven:@objectstack/driver-sqlappears in none of the gate's ownDEBT/TEST_DEBT/EXEMPTledgers,--re-measurere-measures only ledgered packages, and driver-sql'stsconfig.jsonincludessrc/**/*with no test exclusion — so its tests are inside the very program whose green run above named them. This diff touches no other package. CI runs the built-closure measurement regardless.Scope
packages/drivers/driver-sql/src/schema-drift.tsand its new suite, plus a changeset. Untouched by design:packages/spec(read-only here),content/docs/releases/**(release-owned), andsql-driver.ts(queued card #12593 owns that file next — the branch above was designed to need nothing from it). The neighbouringapplyMigrationEntriesreporting gap in #11722 is out of scope here and remains open; nothing in this diff touches it.Adjacent finding, filed rather than ridden along: #12732. The declared half of the same seam diverges from the emitter in two measured directions — an unkeyed bounded text field reports
narrow_varcharatdestructive(which refuses a boot) for a divergence the write seam already forecloses, and a bound pastMAX_VARCHAR_CHARSplansvarchar(100000), DDL MySQL refuses. Both need the keyed-column map threaded in fromsql-driver.ts, which this card deliberately does not take.Changeset grade
minoron@objectstack/driver-sql, argued rather than defaulted, and matching the sibling drift-op addition that shipped for #11535:detectManagedDriftemits a finding on existing deployments where it emitted none before, and the exportedDriftOpunion gains a member — additive for producers, but a widening any consumer switching exhaustively over it must account for. Notmajor: nothing is removed, renamed, or newly rejected.Generated by Claude Code