Skip to content

fix(driver-sql): honour storage.notNull on the multi-value column, the ADR-0113 site it returned above - #18630

Merged
huangyiirene merged 4 commits into
mainfrom
claude/issue-17231-multivalue-notnull-createcolumn
Sep 17, 2026
Merged

huangyiirene merged 4 commits into
mainfrom
claude/issue-17231-multivalue-notnull-createcolumn

Conversation

@huangyiirene

@huangyiirene huangyiirene commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Fixes #17231

Clause-②: no

Honouring a declared constraint is narrowing back to the contract (ADR-0113), not widening one — 「拉回已声明契约不触条款②」. No accept set grows and no public surface enlarges: a column whose metadata declared it constrained becomes constrained.

The falsification triage asked for, answered FIRST

Triage flagged its own premise as falsify-first: does ADR-0113 intend multi-value / JSON columns to be in scope at all, or did the short-circuit predate and survive P0 deliberately? It does not falsify. Four readings, taken on this branch's base:

  1. ADR-0113 carves out no field type. Its D2 is unconditional — storage.notNull "is what emits col.notNullable() and what schema-drift compares against the physical column" — and the word multiple (and any JSON/array wording) appears nowhere in the record. Its P0 note names the site verbatim: 「the physical constraint now keys off the explicitly-authored storage.notNull at that same #createColumn site」, with @objectstack/driver-sql (sql-driver.ts column DDL, schema-drift.ts) listed as the consumer.
  2. The declaration is authorable, measured. storage.notNull's only declared exclusivity is requiredWhen (FieldSchema.superRefine, ADR-0113 Q1 rider). { type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } } parses green, and the requiredWhen pair is still refused — both asserted in the new suite, the second as the discriminating control.
  3. ADR-0113 is already implemented as in-scope for JSON columns at its OTHER named consumer in this package. diffManagedTable compares storage.notNull against the physical column for every field fieldHasColumn answers true for, and that predicate answers multi-value FIRST. So the platform reported nullability_mismatch / tighten_not_null / destructive against a table it had itself just created, with no rows in it. A reading in which JSON columns are out of ADR-0113's scope has to call the differ wrong too.
  4. The nearest counter-reading is about DRIFT, not about the create path. ADR-0113's Context row — 「imposing NOT NULL over possibly-null data is the classifier's destructive class」 — is the tighten_not_null ceremony over an EXISTING column, untouched here. createColumn runs on CREATE TABLE and on ALTER TABLE ADD COLUMN, so the column it constrains is always EMPTY, which is the same reason the string family's driver-sql: the string family ignores maxLength too — a declared 1024 becomes varchar(255) and refuses legitimate writes #11431 note gives for sizing a varchar at this very site.

⛔ NOT MEASURED, and the conclusion does not rest on it: the historical half of the question ("did the short-circuit survive P0 deliberately?"). This checkout is shallow (500 commits) and both git log -S probes collapse onto the shallow boundary commit, so no date ordering was read. What replaces it is (3): had P0 exempted multi-value columns, the exemption would be in the differ, and it is not — nor is any exemption recorded in createColumn, a method that records every one it does have (virtual formula, the FILE family's deployment fact, unique deferred to the tenancy-aware path, options[].default).

What changed

SqlDriver.createColumn decides the JSON column shape before its per-type switch and returned there — above the ADR-0113 nullability line and above the column DEFAULT. The short-circuit decides the column TYPE; returning made it silently decide the column's CONSTRAINTS as well.

The constraint predicate is now stated once, as declaresColumnNotNull, and asked at BOTH of createColumn's exits. Truthiness, not === true: that is the test the tail has always applied, and this moves WHERE the question is asked, never which values answer it.

⛔ The pin was not touched and the generators were not relaxed: packages/cli has no file in this diff. The multi-value short-circuit still decides multiple before the type switch, so generate-multiple-json-column.pin.test.ts's source-read of this method reads the same two landmarks it always did.

The column DEFAULT is still NOT emitted on this path. os generate migration skips it for the same recorded reason (declaredColumnDefault: the multi-value shape has no scalar DDL form), so the two producers already agreed there and nothing diverges.

Evidence — driven, with the control fired

Server-free, on three of the card's own probe shapes plus two controls, through initObjects into an in-memory better-sqlite3 database read back with knex columnInfo().

Before the repair, on base 21b7c12b4f — 6 passed, 3 failed:

× the multi-value column honours `storage.notNull`     expected true to be false
× an INSERT omitting the field is refused              promise resolved instead of rejecting
× the differ agrees with the writer                    [ { kind: 'nullability_mismatch',
                                                           column: 'multi_nn',
                                                           expected: 'NOT NULL', actual: 'NULL',
                                                           category: 'destructive',
                                                           op: { type: 'tighten_not_null' } } ]
✓ control — this reader can see a NOT NULL, and sees NULL where nothing declared one
✓ …and nothing else acquired the constraint: `multiple` and `required` still do not bind the column

The control is what makes the green afterwards a reading: scalar_nn (a scalar carrying the same storage.notNull) comes back NOT NULL in BOTH runs, so the harness was never blind to the constraint. After the repair: 9 passed, 2 skipped, the two skips named below.

Also asserted, so the repair cannot be bought with the type decision: the constrained multi-value column's type still equals the unconstrained multi-value column's, and still differs from the scalar's.

Verification

Run through the shared verify lock; exit codes captured before any pipe.

what reading
pnpm --filter @objectstack/driver-sql test 178 files passed, 11 skipped · 2627 tests passed, 168 skipped (exit 0), at merged HEAD c20356f0e1
pnpm --filter @objectstack/driver-sql typecheck exit 0. The new suite is really in that program: tsc --noEmit --listFiles names it, one occurrence
gate families, derived not guessed scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands at c20356f0e161 families, each run with its exit code recorded, reconciled with --ran: 61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN
gate results 60 green. 1 red — check:cross-package-test-inputs — is NOT from this diff; the control is below
spec-generated artefacts after merging main pnpm --filter @objectstack/spec build && check:generated ⇒ "All 15 generated artifacts are up to date"

The narrowing declared, with its three pieces of evidence. pnpm lint is a repo-scale scan CI owns; what ran here is eslint --no-inline-config --format json over the two changed source files ⇒ 2 files, 0 errors, 0 warnings, at c20356f0e1. (i) The population comes from eslint's own config — the base block matches **/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}, so the third changed path, a .changeset/*.md, is outside eslint's population entirely; (ii) the file count is the JSON reporter's own array length, 2; (iii) this repo's single eslint.config.mjs never enables type-aware linting for ANY file (no parserOptions.project, no typed @typescript-eslint rules — stated and measured in its own header), so this diff cannot move the verdict on a file it does not contain.

✅ Dialect cells — ALL THREE executed, after the patch round

⚠️ Corrected by the domain:engine seat (session_01CqmCgU5RGDoJYhHUMVp2af) at landing. This section previously read 「Locally only sqlite executed」 and 「⛔ Nothing here claims a Postgres or MySQL reading」. That was true at c20356f0e1 and is false at dd723b6dbc: the patch round provisioned both live servers and ran them. The earlier text was an accurate reading of an earlier head, ⛔ not an error — it is superseded, not retracted. Durable record: report comment 5713337146.

The patch round provisioned CI's own conditions locally — a private PostgreSQL 16.13 (initdb + pg_ctl, timezone='Asia/Shanghai', CI's step-4 setting) and a private MySQL 8.0.46 (default_time_zone='+08:00', database conformance), process TZ=America/New_York, OS_EXPECT_LIVE_DIALECT_MATRIX=1.

  • Targeted, all three cells: 23 passed | 0 skipped — 2 entrance tests plus 7 per cell × 3 cells.
  • The job's own population (the full driver-sql suite against both live servers, under those conditions): 189 files passed | 0 failed, 4017 passed | 1 skipped, exit 0. The single skip is pre-existing and ⛔ not a dialect cell (schema-drift.base-type-mismatch.test.ts, sqlite).

What the CI red actually was, and it was this suite, ⛔ not the product. MySQL refuses the omitting INSERT in a third vocabulary the original assertion did not carry. The driver omits the column on every dialect; SQLite and PostgreSQL then substitute the implicit NULL and report the constraint (NOT NULL constraint failed / null value in column … violates not-null constraint), while MySQL in strict mode refuses the omission before a NULL is ever considered: ER_NO_DEFAULT_FOR_FIELD, 「Field 'multi_nn' doesn't have a default value」. One behaviour, three spellings.

sql-driver.ts is byte-identical to the commit CI graded red (sha256 5de174652a3515e2 at both c20356f0e1 and dd723b6dbc). ⛔ No product change was needed and none was made.

The repair keys the refusal PER DIALECT (REFUSAL_BY_DIALECT) rather than widening one alternation until it matches. A widened regex would also swallow a refusal arriving for some other reason — a connection fault, a tenancy error — and the test would stay green through it. A dialect-independent leg was added beside it: the table's row count is unchanged across the rejection, which is the fact the card is actually about. ⛔ Nothing was skipped, disabled or quarantined; the assertion that ran and failed now runs and passes on the server that failed it.

Acceptance notes

pnpm check:cross-package-test-inputs is red on this branch and the red is not this diff's — it is build-state dependent and reproduces on pristine origin/main. Two legs, same gate, same message, on a detached worktree at origin/main (d5e64d8d9d) carrying none of this branch's changes:

  • as checked out, with no packages/spec/dist/ on disk ⇒ exit 0, "OK: 29 package(s) read outside themselves, all declared";
  • after a bare mkdir -p packages/spec/dist/chunks and nothing else ⇒ exit 1, @objectstack/cli descends a directory tree from packages/spec/dist/ … rooted in packages/cli/test/init-created-files-summary.e2e.test.ts.

The gate's walked-root limb reads the filesystem (uncoveredWalkRadiusreaddirSync) and drops a folded walk root that names no real directory, so the same tree passes before a build and fails after one. This diff touches neither that test, nor the glob table, nor any build output. Filed as a finding for the seat rather than repaired here: it is outside this card's file surface and outside its subject. Dedupe words: cross-package-test-inputs, uncoveredWalkRadius, walked root, packages/spec/dist, build-state dependent.

Noted, not filed: nothing else. createColumn's other early exits were read while establishing the two-exit shape and none of them owes the nullability line — formula emits no column at all, and the string / text / file arms all fall through to the shared tail.
Body section corrected at landing by the domain:engine seat, session_01CqmCgU5RGDoJYhHUMVp2af. The rest of the body is the delivering dev's and is unedited.


Generated by Claude Code

`SqlDriver.createColumn` decides the JSON column shape before its per-type
switch and returned there, above both the nullability line and the column
DEFAULT. So `storage: { notNull: true }` on a multi-valued field was inert on
the platform's own table while both `os generate migration` formats emitted
the constraint: an INSERT omitting the field was accepted by the platform's
table and refused by every generated one.

ADR-0113 P0 names `#createColumn` verbatim as the site the physical constraint
keys off `storage.notNull`, and carves out no field type — the key's only
declared exclusivity is `requiredWhen`. The differ, the ADR's other named
consumer in this package, never had the gap, so the platform reported
DESTRUCTIVE `tighten_not_null` drift against tables it had just created itself.

The constraint predicate is now stated once for both of `createColumn`'s exits.
The column DEFAULT stays unemitted on the multi-value path, matching the
generators.

Claude-Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 17, 2026
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: 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 — 11 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 00115a8442da240e3d6589cf73a6131b90b4185fpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 43ef7321de3d7ca8ff244b5ac9818182f3783bd3 — the merge of head dd723b6dbcf198ff630b102ac2b5f96d5bb59c15 into base 00115a8442da240e3d6589cf73a6131b90b4185f, 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 43ef7321de3d7ca8ff244b5ac9818182f3783bd3 && git checkout 43ef7321de3d7ca8ff244b5ac9818182f3783bd3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 00115a8442da240e3d6589cf73a6131b90b4185f dd723b6dbcf198ff630b102ac2b5f96d5bb59c15 && git checkout -B drift-repro 00115a8442da240e3d6589cf73a6131b90b4185f && git merge --no-ff dd723b6dbcf198ff630b102ac2b5f96d5bb59c15

node scripts/docs-audit/affected-docs.mjs --json 00115a8442da240e3d6589cf73a6131b90b4185f

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

…t's own words

The live MySQL cell was the one nobody had run. It refuses the omitting INSERT
exactly as SQLite and PostgreSQL do, but in a third vocabulary: the write omits
the column, so SQLite and PostgreSQL substitute the implicit NULL and report the
constraint, while MySQL in strict mode refuses the omission itself —
ER_NO_DEFAULT_FOR_FIELD, "Field 'multi_nn' doesn't have a default value".

The assertion now carries one regex per cell instead of one widened alternation,
so a refusal arriving for some other reason still reddens the test, and a
dialect-independent row-count leg states the fact the card is actually about:
the write did not land.

Claude-Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants