fix(cli): a generated migration carries the column DEFAULT driver-sql puts on the same field - #17230
Conversation
…nerator-notnull-and-default
📓 Docs Drift CheckThis PR changes 1 package(s): 4 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 — 23 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 e97271494f8c0417d8be50f622c3feae005def0b && git checkout e97271494f8c0417d8be50f622c3feae005def0b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bccf311100cad7affccd6cbfcddbd81fe734d97d 2134a6ebedbbbb0e77a9109b5780aafec4da718e && git checkout -B drift-repro bccf311100cad7affccd6cbfcddbd81fe734d97d && git merge --no-ff 2134a6ebedbbbb0e77a9109b5780aafec4da718e
node scripts/docs-audit/affected-docs.mjs --json bccf311100cad7affccd6cbfcddbd81fe734d97d
|
Fixes #16294
Clause-②: no
Cause 3 of the card: neither
os generate migrationformat read a field'sdefaultValue, so a table built from a generated migration carried no column DEFAULT where the platform's own table has one. A row inserted out of band — by a database client, a seed script, anything that does not go through the engine — got NULL where the declared value belonged.origin/mainI re-ran the card's six-column probe against
generate.tsas #16887 and #17208 leave it, before touching anything. The baseline isdiverged: 2 of 6, not the card's4 of 6:#16887 already landed the repair for cause 2, and for cause 1 with it — as one substitution, not two. Its diff introduced
declaredNotNull()and moved both generators offrequired:Reading
storage.notNullis cause 2. So:f_storage_notnullagrees. Coverage already exists atgenerate-numeric-column-representation.pin.test.tsandgenerate-multiple-json-column.pin.test.ts. Nothing in this diff repairs it; the new pin re-asserts it from the catalog as part of the six-column baseline.f_requiredno longer diverges: arequired: truefield with nostorage.notNullis nullable on all three producers.f_required"stays diverged pending #16887 already moved both migration generators offrequiredontostorage.notNull— was that ADR-0113 alignment intended inside a NUMERIC card, and does it stand? #17218" — that expectation was formed before feat(spec,driver-sql,cli): one physical representation for the NUMERIC column family, read by all three producers #16887 landed, and #16887 already moved both migration generators offrequiredontostorage.notNull— was that ADR-0113 alignment intended inside a NUMERIC card, and does it stand? #17218's own divergence table and quoted source lines are false atorigin/maintoday. ⛔ I have not ruled cause 1 and this PR does not touch it; #16887 already moved both migration generators offrequiredontostorage.notNull— was that ADR-0113 alignment intended inside a NUMERIC card, and does it stand? #17218 remains open and needs a re-read by whoever holds it.f_requiredreadsagreein every table below, and that is a measurement, not a repair by this diff.Re-derived cause 2's justification from the withdrawal rather than from triage, as instructed:
packages/spec/src/conversions/registry.tscarries⛔ WITHDRAWN — there is deliberately NO 'field-required-notnull-explicit', anddocs/protocol-upgrade-guide.mdtells upgraders to addstorage: { notNull: true }"yourself — deliberately". That reading is what made cause 2 worth re-asserting in the new pin even though the emitter already satisfied it.The fix
Both formats now render one shared verdict,
declaredColumnDefault(), taken fromSqlDriver.applyDeclaredColumnDefault— the single place adefaultValuebecomes DDL on the platform side. Its four cases, and the two shapes the driver skips before reaching it:'hello',42,0,true,'')defaultTo(value)'NOW()'ondatetimeCURRENT_TIMESTAMPCURRENT_TIMESTAMP/db.fn.now()'NOW()'ondate/timedb.raw'current_user'{ dialect, source }default: truemultiple: truecreateColumnreturns first)The two token predicates are imported from
@objectstack/spec/data— the sameisNowDefaultToken/isRuntimeDefaultTokenthe driver itself imports — so the token vocabulary is not transcribed and a token added later degrades to "no column default" on both sides at once.⭐ One row is a trap a "just emit the literal" repair gets wrong and never notices.
DEFAULT 42andDEFAULT '42'are the same default, and PostgreSQL keeps them textually apart forever ininformation_schema.column_default(42vs'42'::numeric). knex quotes every bound default, so the driver's column carries the quoted form. Measured, mid-change:That is exactly the cost #15521 already paid once for the audit pair, where a schema differ comparing default text reported them forever. Every literal is therefore emitted quoted, booleans as knex's
'1'/'0'(PostgreSQL normalises both spellings totrue/false, and a SQLite table built by the driver carries the quoted form verbatim, so one rule agrees with the driver on both dialects where two rules agree on one).The knex DEFAULT / NOT NULL coupling — read, and it does not constrain this
The comment already in
generate.tsrecords thattable.timestamps(true, true)compiles its second argument to.notNullable().defaultTo(...)on both columns with no helper for a DEFAULT without the NOT NULL. That coupling belongs to thetimestampsHELPER, not to knex columns in general — an ordinaryColumnBuildertakes.nullable()/.notNullable()and.defaultTo()as independent modifiers. The audit-column block already pays for the helper's coupling by spelling those two columns out longhand; nothing about this change needed to work around it, and the new.defaultTo(...)link is simply appended after the nullability call, increateColumn's own order. Stated here because the card asked for it explicitly rather than left silent.Acceptance — the card's own probe, re-run
Three producers driven into three PostgreSQL 16.13 schemas —
driver-sqlthroughinitObjects,--format sqlthroughdb.rawof the emitted DDL,--format tsby importing the emitted module and callingup(db)— withinformation_schema.columnsread back per schema.A wider 23-column probe on the same cluster covers every
defaultValueshape (NOW()on all three temporal types and its lowercase spelling,current_user, an Expression envelope, an option-level default, a quote, a newline, a backslash, zero, false, an empty string, a negative, an integer-representation type,multiplewith a default, andstorage.notNullbeside a default):diverged: 1 of 23before and after. The one row is not this card's — see Acceptance notes.Tests
New:
packages/cli/src/commands/generate-declared-column-default.pin.test.ts, 10 tests, modelled ongenerate-declared-unique-index.pin.test.ts.PRAGMA table_info) — the live-PostgreSQL acceptance above transplanted into a tier that runs everywhere. Plus the card's consequence as behaviour: an out-of-bandINSERTthat omits the column reads back the declared value from all three tables.NOW()spelling recomputed fromSqlDriver.nowColumnDefaultitself, through aprotected-widening subclass on a PostgreSQL-configured driver that never connects. A pin that transcribedCURRENT_TIMESTAMPwould re-create this very defect one layer up.Commands and verdicts, all captured redirect-then-
$?:pnpm --filter @objectstack/cli exec vitest run --project unitpnpm --filter @objectstack/cli exec vitest run --project integrationpnpm --filter @objectstack/cli typecheckpnpm --filter '@objectstack/cli^...' build+pnpm buildpnpm lint(repo-wide,eslint . --no-inline-config)2134a6ebed— the whole tree, so no narrowing to justifydispatch-gates.mjs --commands --repo objectstack-ai/objectstack‡ two of those unit files first came back as a PREREQUISITE NOT MET (
packages/cli is not built), not a failure; they pass afterpnpm --filter @objectstack/cli build. Three gates first exited 3 for the same class of reason —check:dual-build-cjs-loadsandcheck:i18n-coverageneeded a fullpnpm build, andcheck:type-check-debtOOMed under my own tighterNODE_OPTIONSheap cap; all three are exit 0 on the final head once given what they asked for. ⛔ None of the three was read as a pass.Ablation — directions predicted in writing before any leg ran
Four legs, one script, whole thing under
trap ... EXIT INT TERMwith absolute paths; every mutation proved on disk by occurrence count and a blob hash differing from the HEAD blob, every restore bygit checkout HEAD -- path(never bare) with the blob back to HEAD's,git diff HEADempty andgit status --porcelainempty at the end.columnDefaultSql's literal arm returns''e1fa07b0= HEAD,git diff HEADemptynow-datebranch collapses intonowgit status --porcelainemptythe token match is the spec's, because its closing assertion checks that a near-miss'NOW'is emitted as an ordinary string literal, which routes through the literal arm the leg ablated. The direction was as predicted; the containment claim was not, for a legible reason.Leg 3 is the one that matters for trust: it fails only where the pin recomputes from
nowColumnDefault, which is the assertion a transcribing pin would have left permanently green.Docs drift, on the final head
scripts/docs-audit/affected-docs.mjsreported 34 pages across 8 anchors. Judged:content/docs/references/**is auto-generated andcontent/docs/releases/**is release-owned (neither touched); the permissions / ui / concepts pages match only through the over-broadsys_userliteral anchor and say nothing about DDL emission;content/docs/deployment/cli.mdxdocumentsos generate TYPE NAMEscaffolds and nevergenerate migration.One page is a real row, and it is a row the change makes TRUE rather than false:
content/docs/protocol/objectql/types.mdx'sdefaultValue: 'NOW()'section attributed the translation to the driver alone. It now says both generator formats reproduce the PostgreSQL arm, and names the three shapes that deliberately get no column default — the same shape of sentence #16887 added to this page one property over.content/docsonly, sodocs/can never appear in any run. Checked by hand:docs/protocol-upgrade-guide.mdspeaks only torequired/storage.notNulland is untouched by this change;docs/DX_ROADMAP.mdlistsobjectstack generate migrationas unimplemented, which was already stale before this diff and is not this card's row.Single-writer
Measured from the open PR list (21 PRs, 316 files, each against its own merge base via
GET /pulls/N/files), never from remote branches: no open PR touchespackages/cli/src/commands/generate.ts. Positive control on the same exact-match predicate fires —scripts/pm/check-widening-tells.mjsis named by PR #17216. #16319, the next card queued against this file, has no open PR.Acceptance notes
multiple: true+storage.notNulldiverges — filed as Both migration generators emit NOT NULL on amultiple: truecolumn, whichSqlDriver.createColumnreturns before ever constraining — and the pin asserts the generator side #17231, not repaired here. The one remaining row of the 23-column probe.SqlDriver.createColumnshort-circuits on themultipleflag and returns before the nullability line, so the driver leaves such a column nullable; both generators emitNOT NULLfor it. Measured on live PostgreSQL 16.13:driver null=YESvssqlgen/tsgen null=NO. It is pinned as current behaviour bygenerate-multiple-json-column.pin.test.ts, so moving it contradicts a landed pin and is a separate card rather than a rider here; Both migration generators emit NOT NULL on amultiple: truecolumn, whichSqlDriver.createColumnreturns before ever constraining — and the pin asserts the generator side #17231 carries it, unassigned and ungraded, with both sides written out and neither picked. What this PR does assert about that field is the half it owns: the driver emits no DEFAULT for it and neither format does.requiredontostorage.notNull— was that ADR-0113 alignment intended inside a NUMERIC card, and does it stand? #17218 needs a re-read. Its "The divergence" table and the twogenerate.tssource lines it quotes are false atorigin/maintoday — feat(spec,driver-sql,cli): one physical representation for the NUMERIC column family, read by all three producers #16887 already moved both generators tostorage.notNull, i.e. option A of the two sides that card lays out. Whether the decision it carries is still open, or was answered de facto by a PR that named it as a side effect, is not mine to rule. ⛔ Not addressed here, and #16887 already moved both migration generators offrequiredontostorage.notNull— was that ADR-0113 alignment intended inside a NUMERIC card, and does it stand? #17218 remains open.Clause-②: noholds against the delivered diff: nothing was added. No new export, no new authorable key, no compatibility alias. The two spec symbols newly imported were already on@objectstack/spec/data's exported surface and are already imported bydriver-sqlfrom there; every new function ingenerate.tsis module-local.Authored by Claude Code in session
session_015QE8qk46e5CHJxyQEUjbf8.Generated by Claude Code