Skip to content

fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths (#12015) - #12109

Merged
os-warren merged 3 commits into
mainfrom
claude/issue-12015-builtin-column-collision-warning
Aug 25, 2026
Merged

fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths (#12015)#12109
os-warren merged 3 commits into
mainfrom
claude/issue-12015-builtin-column-collision-warning

Conversation

@os-warren

@os-warrenos-warren commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes#12015

initObjects emits id, created_at and updated_at itself and then skipped any declared field colliding with one — if (builtinColumns.has(name)) continue;, with no warning, no throw and no record anywhere that the author's declaration had been discarded. The driver is right to own its primary key and audit stamps; the defect was that it disagreed with the author in silence.

Two maintainer rulings, both 2026-08-25, and the second is why this PR looks the way it does:

  1. A named, loud load-time warning on all three paths (issue comment 5406814345) — route B (refuse boot) and route C (make the declaration meaningful) explicitly not taken.
  2. Narrow the trigger to declarations that actually lose something (issue comment 5408446814), after the corpus measurement below showed the first shape said an untrue thing on the dominant population — and gave advice that would have caused a regression.

Nothing here moves an accept/reject door or widens a public surface. The platform still owns these three columns and the declaration still does not take effect: what changed is what the driver says.

Why the narrowing, in one example

Against sys_presence.id = Field.text({ label: 'Presence ID', required: true, readonly: true }), the first shape said "the declaration is NOT applied … Remove the declaration". Both halves of that are wrong there: the labelis applied (it feeds the generated *.objects.generated.ts translation files in four locales, highlightFields, FLS and sortability), requiredis enforced (ADR-0113 makes it the write contract, not a column constraint), and an author who followed the advice would have deleted the author-facing label for a column every list view shows. Only the storage half is discarded — so only the storage half is reported now.

What changed

  • packages/drivers/driver-sql/src/builtin-column-collision.ts (new) — the split, in one place:
    • FIELD_KEY_STORAGE_CLASS classifies everyFieldSchema key as storage (the DDL layer would have read it: type, maxLength, unique, defaultValue, storage.notNull, multiple, expression, precision…) or presentation (honoured elsewhere: label, readonly, searchable, required, autonumberFormat, the ADR-0010 provenance markers…). The line is drawn at what the DDL reads, and the load-bearing entry is required — ADR-0113 makes it the write contract, so calling it discarded is exactly the false sentence this narrowing removes.
    • BUILTIN_COLUMN_DELIVERY records what the platform's own columns actually provide, read off the emitting lines: id = table.string('id').primary() (varchar(255), NOT NULL, unique, no default); created_at / updated_at = createAuditTimestampColumn (a timestamp defaulted to the DB clock, left nullable).
    • undeliveredStorageAttributes(column, field) returns only what the declaration asks for and the column does not provide. At runtime an unclassified key is silent — a diagnostic must never invent a warning it cannot justify — and the exhaustiveness pin, not a throw, is what makes an unclassified key visible.
  • sql-driver.ts — one warnBuiltinColumnCollisions helper, called from each of the three paths that drop such a declaration:
pathsitephrase
createinitObjects, before the CREATE TABLEwhile creating table "…"
ADD COLUMN diffinitObjects, existing-table branch — the builtin is already there, so the diff never proposes it; the path a stock upgrade takeswhile syncing existing table "…"
rotation shardensureShardTable, ahead of both its create and column-sync brancheswhile syncing shard "…"

The message now names the attributes rather than denying the declaration:

[sql-driver] while creating table "conformance_agg": declared field 'id' asks for storage the platform's own 'id' column does not provide — type: 'text' (the column is 'string'). The platform emits id/created_at/updated_at itself, so THOSE attributes are not applied; the rest of the declaration (label, help text, the ADR-0113 write contract, and everything other layers read) is honoured as written. Drop the storage attribute(s) named above, or rename the field if you meant a column of your own.

id: { type: 'number' } — the author error the card was filed for — still fires, as does the #11456 fixture's id: { type: 'text' }. created_at: { type: 'datetime', defaultValue: 'NOW()' } describes precisely what lands, and says nothing.

Measurements

The defect, re-measured rather than recalled. The card measured PostgreSQL 16.13 (declared id: textvarchar(255)). The accept-set pin measures SQLite: id: { type: 'text', maxLength: 12 } lands as varchar(255) with the bound binding nothing, while a non-colliding region: { type: 'text' } gets its declared text. MySQL was not measured — no live cell in this container — so the card's MySQL gap stands, explicitly.

The corpus, re-measured after the narrowing with the same real boot (all 45 object definitions exported by @objectstack/platform-objects, driven through SqlDriver.initObjects on SQLite, 45/45 booting green):

warningsshape
before narrowing116id 45, created_at 42, updated_at 29
after narrowing45id only — every one id: Field.text(…) against a varchar(255) key; sys_migration.id also declares a maxLength that binds nothing

The 71 that disappeared are the created_at / updated_at declarations that describe exactly what createAuditTimestampColumn emits. ⛔ The 45 that remain were not tuned away: they are the same disagreement the ruling insists must keep firing, on the platform's own metadata. They are named individually and filed as #12131 (unassigned) — a metadata question for platform-objects, not something to fix from inside this card's band.

Ablation, five legs (be08012875, predictions written before each run, direction RED throughout — these are presence/absence pins). Mutation proven on disk by anchored counts of the exact text being changed, before any result was read; packages/drivers/driver-sql/dist was absent and both pin files import from inside the package, so vitest read src/ with no rebuild able to hide a mutation.

legpredictedobserved
remove the 'shard' callonly the SHARD pin red1 failed | 16 passed× SHARD path: …
remove the 'create' callboth CREATE pins + the accept-set pin3 failed | 14 passed — exactly those
remove the 'alter' callonly the ADD COLUMN diff pin1 failed | 16 passed
neutralize the delivered-check (report every declared storage key = the pre-narrowing behaviour)the silence pins and every message-text pin, in both files10 failed | 7 passed — 9 as predicted, plus the unknown-key case, which the prediction missed and which is correct: its fixture declares a type the mutation now reports
reclassify required as storagethe ADR-0113 classification pin + the end-to-end silence pin3 failed | 14 passed — those two plus the module's presentation-silence case

⚠️ The delivered-check leg's first attempt was a no-op — its perl substitution never matched, the on-disk proof read 0 → 0, and no result was read from it; it was redone with a mutation that provably lands (marker 0 → 1) and the table above reports that run. Every leg restored under trap … EXIT INT TERM with git checkout HEAD -- given the absolute path, then verified: git hash-object equalled git rev-parse HEAD: for both files (db4846b…, 3e78a6c…) and git status --porcelain was empty after each leg and at the end.

Verification

Gate union derived at the final commit, not recalled: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack at be08012875 (6 paths vs merge base 8450eeacb) — 14 matched families + 6 convention-triggered by the test files. Every one run in the foreground under scripts/pm/os-verify-lock.sh, exit code captured before any pipe. All 23 green:

  • pnpm --filter @objectstack/driver-sql exec vitest runTests 2028 passed | 121 skipped (2149), GATE_EXIT=0
  • pnpm --filter @objectstack/driver-sql typecheck (tsc --noEmit) — GATE_EXIT=0
  • check:nul-bytesOK (scanned 6711 text file(s) … no raw ASCII control bytes)
  • check:driver-conformanceOK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt
  • check:test-source-aliasOK — 72 packages with tests scanned
  • check:cross-package-test-inputsOK: 16 package(s) read outside themselves, all declared
  • check:where-matcher299 matcher(s) … 0 silently-wrong … none new
  • check:type-check-debt — closure built first exactly as lint.yml does (70 successful, 70 total), because its refusal means NOT MEASURED: --re-measure: OK — 32 ledger entr(ies) re-measured in 234.1s, 1897 raw tsc error(s) total, none above its recorded number
  • plus check:published-files, check:slot-lookup, check:type-source-resolution, check:changeset-gate-self-tests, check:objectui-changeset, check:query-options-erasure, check:type-check-coverage, check:engine-double-contract, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-plugin-teardown-shape, release-rehearsal-clone --self-test, docs-audit/check-affected-docs, docs-audit/check-drift-comment

Changeset: patch on @objectstack/driver-sql, graded in the file itself — the emitted DDL is byte-identical, no public type or metadata key moves, and the only observable difference is a log line for storage that was already being discarded.


Generated by Claude Code

…n all three DDL paths
`initObjects` emits `id`, `created_at` and `updated_at` itself and skipped any
declared field colliding with one in silence — the declared type, length and
constraints were dropped with no diagnostic anywhere. Measured on PostgreSQL
16.13 and again here on SQLite: a declared `id: { type: 'text', maxLength: 12 }`
lands as `varchar(255)`.
Every path that drops such a declaration now warns once per colliding field,
naming the field, the object and the platform's ownership: the CREATE branch,
the ADD COLUMN diff (a stock upgrade's path), and the rotation shard sync.
Each path carries its own call and its own pin so a regression to a silent
`continue` on one path fails by name.
⛔ Not a rejection door: the accept set is untouched and the DDL emitted is
byte-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 25, 2026
@github-actions

github-actionsBot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

74 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 494279cb31f1d92adab959763085e19c923a8652.

8 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: created_at (symbol, 34 pages)
  • 27 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 — 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 494279cb31f1d92adab959763085e19c923a8652packageMentionDocs.

Which tree this was computed on

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

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

…ion discards
The first cut warned on every declared field colliding with `id` /
`created_at` / `updated_at`. Measured against the real corpus that was not
merely noisy but FALSE: 116 warnings on a stock boot of platform-objects
alone, against declarations like `id: Field.text({ label: 'Presence ID',
required: true, readonly: true })` whose label IS applied (four generated
locales, highlightFields, FLS, sortability) and whose `required` IS enforced
(ADR-0113 write contract). "The declaration is NOT applied … remove the
declaration" was untrue there, and following it would have deleted an
author-facing label.
The trigger is now "asks for storage the platform's own column does not
deliver", decided by one classification table pinned against
`FieldSchema.shape`, and the message names the lost attributes and what the
column really is instead of denying the whole declaration. `id: { type:
'number' }` and `id: { type: 'text' }` still fire.
⛔ Not route C and not a rejection door: the platform still owns the column
and the declaration still does not take effect. This changes what we say.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@os-warren
os-warren marked this pull request as ready for review August 25, 2026 14:47
@os-warren
os-warren added this pull request to the merge queueAug 25, 2026
Merged via the queue into main with commit 84de7e3Aug 25, 2026
34 checks passed
@os-warren
os-warren deleted the claude/issue-12015-builtin-column-collision-warning branch August 25, 2026 15:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-warren@claude