fix(cli): generated SQL migrations give timestamp columns their time zone - #16070
Conversation
…zone (#15521) `os generate migration --format sql` spelled its two audit-stamp columns, and every declared `datetime` field, as bare `TIMESTAMP`. On PostgreSQL that is `timestamp WITHOUT time zone`, while both other producers of the same columns yield `timestamptz`: driver-sql's `createAuditTimestampColumn` and this CLI's own TypeScript migration format both build them with knex's `table.timestamp`, and `createColumn`'s `datetime` arm states the zone-aware column as a decision — "Postgres deliberately keeps `table.timestamp` -> `timestamptz`". Driven rather than compiled: all three producers were run against a live PostgreSQL 16.13 and their columns read back out of `information_schema.columns`. Only the SQL format came back zone-naive. The consequence is a data defect, not a cosmetic type difference: a zone-naive column stores the wall clock of whatever session wrote the row, and `DEFAULT now()` is folded into that session's TimeZone on the way in. Two defaulted rows inserted six milliseconds apart, one under `TimeZone='UTC'` and one under `Asia/Tokyo`, were recorded nine hours apart in the generated table and 3 ms apart in the driver's own. The whole temporal class was enumerated in the same run and `datetime` is its only divergent member; `date` and `time` already agreed on all three producers, so neither moves. The nullability half of #15521 is deliberately untouched — the driver leaves both audit columns nullable, both generators say NOT NULL, nothing fails either way, and the driver's audit DDL is dialect-branched in a way a Postgres- flavoured generated migration does not reproduce. It stays recorded, with the `now()` / `CURRENT_TIMESTAMP` default spelling beside it, in generate-builtin-id-column.pin.test.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
📓 Docs Drift CheckThis PR changes 1 package(s): 13 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 22 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 1e0b430635b2c441e951ea96e4bd6c95c32c4424 && git checkout 1e0b430635b2c441e951ea96e4bd6c95c32c4424
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2648774b967870b33a87b8c2c9dce26a0973cd5d c9304f98ef9555ac75caea030a277a6131836308 && git checkout -B drift-repro 2648774b967870b33a87b8c2c9dce26a0973cd5d && git merge --no-ff c9304f98ef9555ac75caea030a277a6131836308
node scripts/docs-audit/affected-docs.mjs --json 2648774b967870b33a87b8c2c9dce26a0973cd5d
|
os-litant
left a comment
There was a problem hiding this comment.
Contract review — clause ② verdict, judged from the delivered diff at 76a26a6
Mechanical floor limb: NO. The delivered diff touches nothing under packages/spec/src/**, adds no key to any published payload and exports no new symbol: it is two string literals and one map entry in packages/cli/src/commands/generate.ts, two pin tests in the same directory, and one changeset.
Conformance limb: YES. The diff re-selects an input class — the authored field type datetime, plus the builtin created_at / updated_at every generated table gets — between two verdicts that were both already published in this tree and disagreeing: TIMESTAMP (FIELD_TYPE_SQL_MAP.datetime and the two hardcoded audit lines of generateMigrationSql) against timestamptz (driver-sql's createColumn datetime arm and createAuditTimestampColumn, and this same CLI's TypeScript format). The shipped face is os generate migration, whose emitted migration is its product; that product now says a different column for an existing input.
The seat's conformance-limb YES survives, and it survives a harder test than the one it was graded on. The strongest counter-argument — that a strict accept-vs-reject reading flips nothing because both column types admit the same literals — was driven, not argued: one explicit literal, 2026-09-05T22:31:28+09:00, inserted into the datetime column of all four tables and read back under UTC, is stored at epoch 1788647488 in the base generator's table and at epoch 1788615088 in the head generator's, the driver's and the TypeScript format's — nine hours apart for one input, with no DEFAULT in play at all. The verdict on an admitted input is not value-neutral, so "re-selection between two published verdicts" is the correct reading, and the rule's own tie-break (拿不准 ⇒ yes) points the same way. One thing the grading did not name and should: the authority behind the second verdict is not only the driver's comment but ADR-0053 (accepted, addendum D-B1..D-B4), whose D-B4 says in one sentence that Field.datetime keeps timestamptz on Postgres and "the builtin created_at/updated_at take the same type". The PR implements that decision; it reverses nothing.
VERDICT: CLEARED
Submitted as a COMMENT because GitHub refuses an APPROVE on a same-account PR. No blocking defect found; the non-blocking items are at the end.
1. Driven independently — three producers, one live PostgreSQL 16.13, catalog read back
A private cluster (port 54329, server TimeZone=UTC) was initialised for this review. BEFORE is the base commit's own generator (53cbad9f755, extracted with git show and imported as a module), not a string reconstruction; AFTER is the head's. The driver went through initObjects, the SQL format by executing its emitted DDL verbatim, the TypeScript format by importing the generated module and calling its own up() against a live knex.
table column data_type null default
m_driver created_at timestamp with time zone YES CURRENT_TIMESTAMP
m_driver f_datetime timestamp with time zone YES
m_sqlgen_before created_at timestamp without time zone NO now()
m_sqlgen_before f_datetime timestamp without time zone YES
m_sqlgen_after created_at timestamp with time zone NO now()
m_sqlgen_after f_datetime timestamp with time zone YES
m_tsgen created_at timestamp with time zone NO CURRENT_TIMESTAMP
m_tsgen f_datetime timestamp with time zone YES
(f_date = date and f_time = time without time zone on all four tables; updated_at mirrors created_at everywhere)
Two defaulted rows inserted back to back under SET TIME ZONE 'UTC' and 'Asia/Tokyo': skew 09:00:00.00097 in m_sqlgen_before, 00:00:00.001239 in m_sqlgen_after, 0.001944 in the driver's table, 0.000866 in the TypeScript format's. The dev's numbers reproduce. What broken looks like: with the fix absent, m_sqlgen_after reads timestamp without time zone and its skew is nine hours; with the driver or TypeScript format not on timestamptz, the four tables would agree and there would be no divergence to re-select.
2. The ride-along — enumeration verified, it holds
- Spec vocabulary: the
FieldType"Date & Time" group is exactlydate,datetime,time(packages/spec/src/data/field.zod.ts:60); a grep of the vocabulary fortimestamp/duration/interval/year/month/week/daterangefinds no fourth temporal name. - Driver:
createColumncarries exactly three temporal arms (sql-driver.ts15973table.date, 15988table.timestampon non-MySQL, 15996table.time). - TypeScript format: exactly three (
generate.ts1353–1360); SQL map: exactly three. - Driven:
f_dateandf_timecame back identical on all four tables; onlyf_datetimediverged, and only in the base SQL format. - Repo-wide sweep for the old literal outside the PR's files (excluding
node_modules/dist): the only carriers left are a JSDoc@exampleon spec'sDataTypeMappingSchemaand that schema's own test fixtures — a schema with no consumer outsidepackages/spec, i.e. an inert example, not a producer. No CLI template directory exists. No published doc or skill states the generator's column types (content/docs/protocol/kernel/lifecycle.mdx:504names the command only), so none is falsified.
ADR-0053 D-B4 binds the audit columns to the same physical type as a declared Field.datetime, which is the strongest form of the "identical reason" argument: the two sites are one decision in the ADR's own wording, so repairing one without the other would have left the generator contradicting the ADR in the same file. The PR does not under-repair.
3. What was deliberately not changed — the tree is in a defensible state, not a worse one
Divergence rows against the driver, per producer: SQL format 3 → 2 (nullability, default spelling), TypeScript format 1 → 1 (nullability). Nothing new was introduced and nothing moved without being pinned. Confirmed live: now() = current_timestamp and now() = transaction_timestamp() both t, while pg_get_expr on the two defaults reads now() for the generated table and CURRENT_TIMESTAMP for the driver's — one instant, two catalog spellings, exactly the second row the dev added to the maintainer's ruling. Both remaining rows are asserted in generate-builtin-id-column.pin.test.ts in both directions with a message naming the card, so neither can move silently while the ruling is open. Shipping the type half alone is coherent: it is the one row that was decidable without a ruling, and it is the one that ADR-0053 had already decided.
4. Changeset level — patch fits; no **BREAKING** banner is owed, so no ADR-0087 marker is either
Nothing an author can write is removed or renamed, no export changes, and the accept set of os generate migration is unchanged; what changes is the content of newly generated output, corrected to the platform's own column type, with already-generated files untouched (stated in the changeset). That is a bug fix in a released package, which AGENTS.md grades patch. Same-surface precedent agrees: #15040's uuid → VARCHAR(255) on this generator's id column — a hard-failure-class DDL change — is pending in .changeset/generated-migration-id-column-shape.md as patch with no banner, and #14828's two DDL corrections shipped under Patch Changes in packages/cli/CHANGELOG.md. Read off the CATEGORIES const in scripts/check-adr-0087-registration.mjs (unpublished, already-registered, no-migration-prescription, runtime-interface-only, type-surface-only): were a future reader to declare this breaking anyway, the fitting disposition would be not-required (no-migration-prescription); as delivered none is required. check-adr-0087-registration --base origin/main: "adds no declared-breaking changeset (1 non-breaking changeset(s) seen)"; check-changeset-no-major: green; the body carries no token the gate's BREAKING detector reads.
5. Reverse verification reproduced
Prediction stated first: exactly three red — the two new cases and the recorded-divergence case — 35 green. git restore --source=53cbad9f755 -- generate.ts (tree only; on disk TIMESTAMPTZ count 1 → 0, old literal 0 → 1), run: 3 failed | 35 passed (38), the three named cases across both pin files. Restored with git checkout HEAD --, blob b02a8510… equal to HEAD:'s, git status --porcelain empty. At head: 38/38.
6. Gates, all at 76a26a6 in a dedicated worktree, every exit code captured before any pipe
Gate union: 56 families, derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands on the PR's four paths and asserted against the tool's own Reconciliation — 56 famil(ies) line. All 56 ran. 54 green. Two are NOT MEASURED, both exit 3 and neither a pass: check:dual-build-cjs-loads (PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/) and check:i18n-coverage (Nothing was compared: 12 config(s) did lint, but a partial round cannot judge the ratchet). check:type-check-debt is green under a real budget (12 ledger entr(ies) re-measured in 119.5s, 140 raw tsc error(s) total, none above its recorded number); check:query-options-erasure (145 s) and check:slot-lookup (76 s) are green after first tripping my own 75 s harness cap — that 124 was mine, not a verdict.
Artifact rosters block: 37 families, run separately, outside that total by design. All 37 ran. 33 green. node scripts/check-partof-closing-keyword.mjs and node scripts/check-single-claim-paths.mjs exit 2 NOT WIRED (no PR context locally). The partof one was then run wired with PR_BODY and PR_NUMBER=16070: ✓ check:partof-closing-keyword: PR #16070 carries no Part-of/closing-keyword contradiction. The single-claim one, wired with PR_NUMBER/GITHUB_REPOSITORY, died on an uncaught GitHub API 401 (no usable token here) — NOT MEASURED. check:react-declaration-parity exit 1: MANIFEST is not set — there is no registry side to compare against (objectui's sdui.manifest.json, which this repo does not contain; surface untouched by this diff). check:published-readme-exports exit 3: PREREQUISITE NOT MET — 5 package(s) whose built type entry this run would read are not built (my build was the cli/driver-sql closure only). Per #16030 the pnpm-spelled check:partof-closing-keyword and check:single-claim-paths rows are green but resolve to --self-test: they grade the checker's fixtures, not this PR, and are not counted as evidence here.
Package runs: pnpm --filter @objectstack/cli test through scripts/pm/os-verify-lock.sh: 261 of 264 files passed, 3058 passed | 6 expected fail (the dev reported 264 / 3145). The three non-passing files failed with [vitest-pool]: Worker forks emitted error at the exact timestamps of SIGTERMs I sent to vitest workers under my own worktree while clearing a killed background runner — a self-inflicted measurement artefact, not a PR failure — and a bounded re-run to name and clear them did not fit this turn, so those three files are NOT MEASURED (listed below). The two pin files, which are the load-bearing tests for this diff, are measured three ways: 38/38 at head, 3 red / 35 green under ablation, 38/38 on the merged tree. pnpm --filter @objectstack/cli typecheck: exit 0 (36 s), including check:test-typecheck: OK — @objectstack/cli's test layer compiles. tsc -p tsconfig.json --noEmit --listFiles (exit 0) lists both edited pin files, so the typecheck's coverage of the edit is measured rather than assumed.
7. Merged with today's origin/main — the census re-run, not trusted from the merge's silence
origin/main (1c00b0152, 20 commits past the PR's base) merged locally into a second worktree → 8d15f6455e2, no conflict, never pushed. Per the line-anchor caution measured on #16071, the census was re-run on the merged tree rather than read off the clean merge: check-system-context-census OK (105 elevation read sites in 19 packages across 44 files, all anchored). Also on the merged tree: check-adr-0087-registration, check-changeset-no-major, check-empty-changeset all green; both pin files 38/38.
NOT MEASURED — each by name, none read as a pass or a red
check:dual-build-cjs-loads— exit 3,PREREQUISITE NOT MET(unbuiltdist/). NOT MEASURED.check:i18n-coverage— exit 3, a partial round judges nothing. NOT MEASURED.check:published-readme-exports(rosters block) — exit 3, five packages' built type entries absent. NOT MEASURED.check:react-declaration-parity(rosters block) — exit 1,MANIFEST is not set; an objectui artifact this repo lacks, on a surface this diff does not touch. NOT MEASURED.check-single-claim-paths— exit 2 NOT WIRED in the block; wired, an uncaughtGitHub API 401. NOT MEASURED.node scripts/pm/check-clause2-carriers.mjs --pair 16070— exit 3,PREREQUISITE NOT MET(the token was refused, HTTP 403). NOT MEASURED by the tool. Read by hand from both carriers instead:needs:contract-reviewis on PR #16070's labels and on card #15521's labels, and the card's claim comment carriesClause-②: yesin the machine spelling — both limbs readable and consistent.pnpm --filter @objectstack/cli test, 3 of 264 files — worker crash from my own SIGTERMs, not re-run inside this turn. NOT MEASURED for those three files; 261 files and the two pins are measured.- pnpm-spelled
check:partof-closing-keyword/check:single-claim-paths—--self-testonly (#16030); green, not evidence about this PR.
Non-blocking observations
- ADR-0053 is the governing decision and is uncited. The
generate.tscomment and the changeset quote the driver's prose ("Postgres deliberately keeps…"); that sentence is ADR-0053 D-B4's, and Prime Directive #13 asks that an implemented ADR's id be left in the code. Worth naming in the comment (a follow-up is fine; it implements the ADR rather than reversing it). - The type-half pin case is coupled to the open ruling.
'#15521 — the audit columns take the driver's zone-AWARE type'asserts"created_at" TIMESTAMPTZ NOT NULL DEFAULT now()bytoContain, so the nullability ruling will red the type case as well as the record case; asserting the type token alone (/"created_at" TIMESTAMPTZ\b/) would keep the two halves separable. TIMESTAMPTZis Postgres spelling. The SQL format was already Postgres-only (JSONB, double-quoted identifiers), so no portability is newly lost, but it folds into the maintainer's "which dialect does the generator claim" question already on the card.- Spec's
DataTypeMappingSchemaJSDoc example (packages/spec/src/data/driver-sql.zod.ts:32,96) still illustratesdatetime: 'TIMESTAMP'for PostgreSQL. Inert (no consumer outside spec) and correctly left out of this diff — touching it would put a spec file in the PR for a comment. - A correction to the report's measurement note, not a defect: "no dist sits in the ablation loop" is half right.
generate.tscompiles from source, but its transitive@objectstack/specimport resolves todist— the cli'svitest.config.tscarries no spec alias (driver-sql's does) — measured: on a tree without spec'sdist, both pin suites fail to load. The ablation stays valid because the PR does not touch spec.
Implemented-by: claude/issue-15521-audit-stamp-column-divergence (a mode:subagent dev — its branch is its identity)
Reviewed-by: session_01D47qPfEWVPmhguWgBZCi5N (context-isolated contract-review subagent of the same parent session; dedicated worktree at 76a26a6)
Generated by Claude Code
Contract review corrected the AUTHORITY behind this change, not its answer. The changeset cited driver-sql's own comment; the decision it implements is ADR-0053 D-B4 (accepted), whose resolution states both sites in one sentence: `Field.datetime` maps to DATETIME(3) on MySQL while "Postgres deliberately keeps timestamptz", and "the builtin created_at/updated_at take the same type -- the registry declares them Field.datetime". So the declared-field row and the audit-stamp rows belong in one diff by decision rather than by this seat's judgement, and a reader of the release notes does not have to reconstruct that from a driver source comment. Changeset prose only. No code, no pin, no test and no FIELD_TYPE_SQL_MAP entry moves; the nullability and now()/CURRENT_TIMESTAMP rows stay untouched and open with the maintainer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
Part of #15521 — deliberately
Part of, not a closing keyword: the card carries two divergences and this change rules only one of them. The nullability half stays open and is recorded, not decided.What was ruled, and what was not
TIMESTAMPwhere both knex paths yieldtimestamptzNOT NULLwhere the driver emits nullableThe dispatching seat's working expectation was that the driver is the reference. That held for the type half and is now driven rather than assumed; it is not asserted for the nullability half, where the driver being the reference is the question rather than the answer.
The producer set, enumerated before anything was repaired
Three producers emit these columns, and all three were driven:
driver-sqlcreateAuditTimestampColumn— what actually creates the table at runtimeos generate migration --format sqlgenerateMigrationSql, hardcoded literalsos generate migration(TypeScript, the default)generateMigrationTs,table.timestamps(true, true)A fourth site emits the same column type for a different column and is repaired in the same breath — see "The second site" below.
dateandtimewere enumerated with them and diverge nowhere.Driven, not compiled: real PostgreSQL 16.13
The earlier measurement on this card was an offline knex compile, because that container had no Postgres. This one has: a private cluster was initialised and every producer was run against it for real — the driver through
initObjects, the SQL format by executing its emitted DDL, the TypeScript format by importing the generated module and calling its ownup()against a live knex. The columns are then read out ofinformation_schema.columns, so the numbers below are the catalog's, not a prediction.Before (
created_at, and a declareddatetimefield, per producer):After:
All three producers now agree on the type. What remains between them is exactly the two things this change declines to touch:
null=YESversusnull=NO, andCURRENT_TIMESTAMPversusnow().Why the type half is a data defect and not a cosmetic type nit
A zone-naive column stores the wall clock of whatever session wrote the row and keeps nothing to recover the offset from, and
DEFAULT now()is folded into that session'sTimeZoneon the way in. Two defaulted rows were inserted six milliseconds apart, one underTimeZone='UTC'and one underAsia/Tokyo:The same two inserts into the driver's own table were 3 ms apart throughout. A table generated by the SQL format was recording an
updated_atordering that depends on which client wrote the row.Contract review then removed
DEFAULTfrom the picture entirely and drove the sharper version: the same explicit literal2026-09-05T22:31:28+09:00, admitted by both column shapes, lands at epoch1788647488in the base generator's column and1788615088in the head, driver and TypeScript columns — nine hours apart, one admitted input, no default involved.The second site, and why it is in this diff rather than a follow-up card
The governing decision is ADR-0053 D-B4 (accepted) (
docs/adr/0053-date-and-datetime-semantics.md), and it covers both sites in a single sentence, so they belong in one diff by decision rather than by this seat's judgement. Its resolution reads:Field.datetimemaps toDATETIME(3)on MySQL, "Postgres deliberately keepstimestamptz: asking for precision 3 there would reduce it from microseconds", and "the builtincreated_at/updated_attake the same type — the registry declares themField.datetime, and they are what most list views sort by".FIELD_TYPE_SQL_MAP.datetimecarried the identical literal, for the identical reason, with the identical measured consequence — the same producer, the same file, the same decision behind it.driver-sql'screateAuditTimestampColumn, itscreateColumndatetimearm and this CLI's TypeScript format were already implementing that ADR; the SQL format was the one producer that was not, on both of its temporal rows at once.Repairing only the audit columns would have manufactured a within-file contradiction of exactly the kind
generate.tshas been closing card by card: one generated migration in whichcreated_atisTIMESTAMPTZand a declareddatetimefield two lines above it isTIMESTAMP.The class was enumerated before it was repaired, and it has exactly one divergent member:
Clause ②, per limb, judged from this diff
packages/spec/src/**is touched.datetime, and the builtin audit columns, move from the SQL format'sTIMESTAMPto theTIMESTAMPTZthatdriver-sqland the TypeScript format already emit. The emitted DDL of a shipped command changes, so this is graded yes rather than argued down.The declaration is carried in the machine spelling on the card's claim comment. Contract review has since cleared this PR with both limbs standing as declared; the label carriers are the seat's to manage and are not touched here.
The pins
generate-builtin-id-column.pin.test.ts— the case the card names as "theitto edit" is edited, and split: the type half is now asserted as agreement with the driver, read off the driver's own builder rather than transcribed, so the day that builder stops emitting a knextable.timestampthis fails here instead of leaving the generators quietly wrong again. The nullability half keeps its recorded-divergence case, with thenow()versusCURRENT_TIMESTAMPdefault spelling recorded beside it.generate-field-type-vocabulary.pin.test.ts— one new case covering the whole temporal class against the driver's three arms, so the guard closes the class rather than the one line.Verification — code at
76a26a6, headc9304f9(changeset prose only)c9304f9adds the ADR-0053 D-B4 citation to the changeset and to the section above, and moves nothing else:git diff --name-only 76a26a6..c9304f9is exactly.changeset/generated-migration-audit-stamp-timestamptz.md, one line changed. Every measurement below therefore still describes the delivered code byte for byte. Re-run on the new head:check-empty-changeset,check-changeset-no-major(both spellings of each),check-changeset-fixed,check:changeset-gate-self-tests,check:adr-0087-registrationandcheck:nul-bytes— all exit 0.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commandsand asserted against that tool's ownReconciliation — 56 famil(ies)line. 53 green. Three are NOT MEASURED and none of them is a pass:check:dual-build-cjs-loads(exit 3, PREREQUISITE NOT MET — reads built output, 11 packages have nodist/),check:i18n-coverage(exit 3, partial round, judges nothing), andcheck:type-check-debt, which first tripped my own harness timeout and then ran clean under a real budget (green, 12 ledger entries re-measured, none above its recorded number).check-partof-closing-keywordandcheck-single-claim-pathsexit 2 (NOT WIRED — no PR context locally), andcheck:react-declaration-parityexits 1 for a missing objectui manifest this repo does not contain, on a surface this diff does not touch. Their pnpm-spelled siblings are green but grade only their own fixtures.pnpm --filter @objectstack/cli test— 264 files, 3145 passed, 6 expected-fail.pnpm --filter @objectstack/cli typecheck— green, and its coverage of the edited files is measured rather than assumed: both pin files appear in thetsc --noEmitprogram's--listFilesoutput.main'sgenerate.tsover the committed fix turns exactly three cases red — the two new ones and the recorded-divergence case — across both pin files, with the other 35 staying green. The mutation was confirmed on disk by grep count in both directions before the run, and the restore by an emptygit diff HEADplus a blob-hash comparison againstHEAD.Scope for an existing deployment
Generated migration files already checked in are not rewritten, and no deployed column is altered: a table created from an older generated migration keeps
timestamp without time zoneuntil its owner migrates it. What changes is what the next generated migration says. This is worth knowing precisely because it is the schema-diff argument the card makes — a generated table and a platform-created table now agree on the column type, and disagree only on the two things left open.