Skip to content

fix(spec): notNull / not_null prescribe storage.notNull, not required (#16867) - #17477

Merged
os-bill merged 1 commit into
mainfrom
claude/issue-16867-notnull-prescribes-storage
Sep 10, 2026
Merged

os-bill merged 1 commit into
mainfrom
claude/issue-16867-notnull-prescribes-storage

Conversation

@os-bill

@os-bill os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16867

Clause-②: no — no accepted key moves. notNull, not_null and storageNotNull are refused before this change and refused after it; only the sentence attached to the refusal moves. This was measured, not assumed: strict-object.ts records that an aliases / guidance table "runs only from the unrecognized_keys path", so it decorates a rejection and never admits a key, and the five-probe run below shows all three spellings still failing parse afterwards. A pin (clause ② — both spellings are REFUSED, before and after) holds that property.

Authored by the domain:spec execution seat in session session_01MkQhmuuJAVDjmeWNixwDDH (https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH).

The fence came first, and it is what determined the wording

The card declined to propose a target, calling it a contract question. Triage answered it from the card's characterisation of ADR-0113 and explicitly required this seat to verify against the ADR itself. Read on origin/main (docs/adr/0113-required-write-contract-vs-column-constraint.md), it determines the target cleanly, and by its own adjudication rather than by inference:

Q1 — spelling: storage: { notNull: true }.

storage.notNull (exact spelling open, see Q1) is what emits col.notNullable() and what schema-drift compares against the physical column.

New fields: required: true alone creates a nullable column with the write-contract gate — uniformly, on new and old objects alike.

One correction to triage's ruling, in the direction of the ADR. Triage wrote that "the message should name storageNotNull". storageNotNull is not a correct key — it is itself a rejected flat spelling, which the positive control below proves (it fails parse). The ADR's Q1 adjudicated the spelling as the nested storage: { notNull: true }, so that is what all three flat spellings now name. The reasoning triage gave is untouched and holds exactly: ADR-0113 excludes required from column constraints, therefore required cannot be the target.

Reading 2 in the card — notNull as a legacy synonym of required from before the split — does not survive contact with the record. The pre-split conflation lived in required (the knob ADR-0113's TL;DR describes as binding three meanings, with the driver's if (field.required) col.notNullable()); there was and is no authorable notNull field key for it to be a legacy synonym of. ADR-0113 then assigned the word notNull to the column constraint deliberately, on the argument that it is "vocabulary a model already knows from the SQL corpus". The same reading was ruled on twice more in this tree: conversions/registry.ts (maintainer ruling 2026-09-08) — "A conversion cannot be what decides a column constraint — that is the author's explicit act" — and migrations/registry.ts"Post-17 a column is NOT NULL because its author wrote storage: { notNull: true }, and for no other reason."

So no fork, and no guessed wording: the prescription is the ADR's own sentence.

Premises re-measured on the current tree

The card measured against built dist/ at c5ea982d97 plus an unmerged branch. Re-run through ObjectStackDefinitionSchema.safeParse on origin/main 3644fadc8e, keeping the card's control structure. Every premise reproduced; nothing was already fixed.

Probe Before After
subject notNull: true Did you mean notNullrequired? ADR-0113 sentence naming storage: { notNull: true }
subject not_null: true Did you mean not_nullrequired? same sentence
positive control storageNotNull: true ADR-0113 sentence (correct, reachable) same sentence
negative control bare field parses; required=false, storage=undefined unchanged
negative control storage: { notNull: true } parses; storage={"notNull":true} unchanged

The two negative controls are what prove the instrument can say "fine" — and they caught an error in the first run of the probe itself, where a stray name key in the envelope made every case fail for an unrelated reason.

What changed

packages/spec/src/data/field.zod.ts only, plus its test and a changeset.

  • notNull: 'required' leaves the alias table. isRequired and mandatory stay: both are genuine spellings of the write contract, and ADR-0113 moved neither.
  • All three flattened spellings are answered by one guidanceSets entry, COLUMN_CONSTRAINT_FLAT_KEYS, whose anchored pattern /^(?:storage[_-]?)?not[_-]?null$/i cannot reach required, requiredWhen or storage itself.
  • The prescription names both axes — the column constraint and the write contract. The defect being repaired is precisely that the author cannot tell which of the two they are getting, so answering only the column half would have fixed the measured direction and opened the mirror-image one.
  • The stale comment at the old :911-913 is gone. It claimed notNull "gets its own sentence rather than a rename", describing an arrangement that never landed on the spellings it was written for. It is replaced by a docblock on the new constant that records what the arrangement actually is, why the rename was wrong, and the mechanical trap below.

The mechanical trap, for whoever repairs the next table like this

The entry moved from aliases to guidanceSets, not to exact guidance. aliases is indexed by aliasProbe (case- and separator-folded, so the single row also covered not_null), while exact guidance is matched case-sensitively on the authored spelling. A lone guidance.notNull row would have looked like a complete repair and quietly dropped not_null onto the edit-distance fallback. The two spellings are pinned separately for exactly that reason, and a single-spelling pin would not have seen it.

Evidence

  • Pins: 6 new cases in field.test.ts. Each absence claim is a pair — the ADR-0113 sentence is present and the rename template Did you mean is absent. Asserting only the first would pass on a message carrying both.
  • Ablation (from the committed state, field.zod.ts reverted to origin/main with the new pins kept): 3 pins go red, restored by git checkout HEAD -- ... and verified by an empty git diff HEAD plus a blob-hash match against HEAD (977b998b…). On-disk mutation was proven by anchor count 1 → 0 and a changed hash, not by the editor's exit code. The storageNotNull pin stays green under ablation, honestly: it already carried the correct sentence and is a regression guard, not a defect detector.
  • Gates: packages/spec build, typecheck, and the full suite (470 files / 13259 tests) green; check:generated reports all 15 generated artifacts up to date with zero working-tree drift; alias-integrity.test.ts (whose [finding] alias-integrity cannot see an alias row made unreachable by a guidanceSet — a vis-shaped alias on VISIBILITY_STRICT_OPTIONS surfaces would be dead code with no gate saying so #7889 check fails exactly the dead-alias shape this repair could have left behind) green; repo-wide pnpm lint green. check:skill-examples is NOT MEASURED — it refuses on an unbuilt packages/client-react/dist in a fresh worktree rather than return a false green; this diff adds zero prose example blocks, so it has no input to that gate.

One observation, not a card

Triage flagged that a correct key nobody writes is the next card in this family. The measurement narrows it: the undiscoverable spelling is storageNotNull, which is not a correct key at all but a flat catcher, while the genuinely correct storage: { notNull: true } is declared, described, and parses — so the "correct key nobody can discover" premise is measurably false here, and the refusal now hands the author that exact nested spelling. Recorded for the next reader; ⛔ not fixed here and ⛔ not filed without a named carrier.

⛔ Not audited and out of scope, per the card: whether other rename tables point at keys an ADR has since split off, and whether isRequired / mandatory are affected.


Generated by Claude Code

…equired`

`FieldSchema` refused the flattened column-constraint spellings and then
prescribed `required` — the one key ADR-0113 exists to say is NOT the column
constraint. `required`'s own `.describe()` in the same file says the opposite
of what the rename prescribed: "NOT a column constraint — the physical NOT
NULL is a separate explicit opt-in (`storage.notNull`)".

The refusal was never the problem; the REMEDY was. An author reaching for a
NOT NULL column complied, wrote `required: true`, and got a nullable column
plus a write gate, with nothing downstream to refuse it — the loud failure
resolved into a silent wrong end state.

`notNull: 'required'` leaves the alias table. All three flattened spellings —
`notNull`, `not_null`, `storageNotNull` — are answered by one
`guidanceSets` entry naming `storage: { notNull: true }`, and naming
`required` as the write contract too, since the defect is precisely that the
author cannot tell the two axes apart.

The entry moved to `guidanceSets` rather than exact `guidance` because the
two channels fold differently: `aliases` is indexed by `aliasProbe` (case and
separator folded, so one row also covered `not_null`) while exact `guidance`
is matched case-sensitively. A lone `guidance.notNull` row would have dropped
`not_null` onto the edit-distance fallback.

No accepted key moves: a guidance table decorates a rejection and never
admits a key, so both spellings are refused before and after. `isRequired`
and `mandatory` are genuine write-contract synonyms and still rename.

The stale comment claiming `notNull` "gets its own sentence rather than a
rename" is replaced — it described an arrangement that had not landed.

Fixes #16867

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/spec, touching 6 documentable anchor(s).

12 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-driven.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/external-datasources.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/field-types.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/validation-rules.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/deployment/troubleshooting.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/deployment/validating-metadata.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/getting-started/quick-reference.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/kernel/contracts/data-engine.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/backward-compatibility.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/objectql/types.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/objectui/concept.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/ui/forms.mdx (via FieldSchema (symbol, a top-level const))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17/17-0.mdx (via FieldSchema (symbol, a top-level const), notNull (literal, a string literal in examples))
  • content/docs/releases/v17/17-1.mdx (via FieldSchema (symbol, a top-level const))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • 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 — 135 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 65ad77d5aea371d67cee8cac2aa4634457fc409bpackageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 65ad77d5aea371d67cee8cac2aa4634457fc409b

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

os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

ACCEPT — the fence passed and CORRECTED triage. ⚠️ And a red check the seat is landing over, deliberately and on the record

domain:spec execution seat, session_01MkQhmuuJAVDjmeWNixwDDH, 2026-09-10T16:46Z.

⭐ The fence did what a fence is for — it caught triage's target, not just the dev's

Triage ruled "the message should name storageNotNull". The round read ADR-0113 itself, as instructed, and found storageNotNull is not a correct key at all — it is another rejected flat spelling, which its own positive control demonstrates (it fails parse). ADR-0113 Q1 adjudicated the nested form:

Q1 — spelling: storage: { notNull: true }.

⇒ the new prescription names that. ⭐ Triage's reasoning was right and its target was wrong, and the only reason that surfaced is that the order said read the ADR yourself and quote the sentence you rely on rather than triage says X, do X. Had the round obeyed the grading literally, it would have shipped a refusal pointing at a second key that also refuses — the exact defect this card is about, moved one key over.

Card reading 2 (notNull as a legacy synonym of required) was closed on the record, not on judgement: the pre-split conflation lived in required, there is no authorable notNull field key for it to be a synonym of, and two further in-tree rulings say the same — "A conversion cannot be what decides a column constraint — that is the author's explicit act" and "Post-17 a column is NOT NULL because its author wrote storage: { notNull: true }, and for no other reason." docs/adr/** read only, zero edits. ✓

⭐ The channel choice is the second thing worth naming

The entry went to guidanceSets, not exact guidance — because aliases is aliasProbe-indexed (case and separator folded, so one row also covers not_null) while exact guidance is case-sensitive. A lone guidance.notNull row would have looked complete and silently dropped not_null onto the edit-distance fallback. ⇒ that is this card's own defect class, avoided one layer down, by reading how the machinery indexes rather than assuming.

And the pins are pairs: ADR-0113 sentence PRESENT and the rename template Did you mean ABSENT. ⭐ Asserting only the first would pass on a message carrying both.

⚠️ The red check — my call, made explicitly

Part-of PR must not also close its card is failing, because commit d060473c4 carries Fixes #16867 in its message. The gate's RULE 2 refuses a card relation in a commit message; RULE 1 (the body) is clean.

The round was right not to "fix" it. Once a branch is pushed, no author action clears this red — a new commit joins the list and leaves the offending message in it, and the only thing that removes it is the amend/rebase/force-push this repo forbids absolutely. The gate says so in its own words, and the round reproduced the gate locally to confirm the verdict rather than infer it from the check name.

I verified the gate's advisory claim myself rather than taking it on report: scripts/check-required-contexts.mjs names this check 0 times, while the lit control Test Core reads 14 — so the registry is populated, readable, and genuinely does not carry it. The gate is explicit that the rest is not its call:

⛔ Whether a pull request LANDS carrying this red is not this gate's call and its output must not make it … that decision belongs to whoever lands the PR, under the rules that bind them. This gate reports.

Decision: land it. The reasoning, so it can be argued with:

  • The discharge route the gate names — the lander replacing the assembled commit list with the PR body by hand at the merge button — is unavailable to me. ⛔ I do not hand-merge and ⛔ do not bypass the merge queue, and the gate itself records that a queue merge never does this.
  • The cost is bounded and I measured its direction: squash_merge_commit_message = COMMIT_MESSAGES, so Fixes #16867 reaches permanent history. ⭐ It names the same card the body closes and the card this PR is for — so no other card can be wrongly closed. The residue is a duplicate declaration, not a wrong one.
  • The repo has landed this exact shape before, and the gate cites it as a measurement: PR fix(tooling): isolate git children from ambient GIT_*, and make a shared core.bare flip loud #16646 at 0a61db1f5 landed carrying its Refs #16624.
  • The alternative available to me — recreating the branch with reworded commits under a new PR — is not a history rewrite and would leave clean history, but costs a full CI cycle and a duplicate PR record to remove a duplicate-but-correct trailer. ⚠️ I judge that not worth it here. If the maintainer disagrees, the remedy is cheap to state and I will take it next time rather than argue.

⇒ My landing pre-check ④ says every check green. This is a declared exception, not an oversight: the red is measured, permanent, advisory, understood, and cannot be cleared by any action open to this seat. ⭐ I am recording it rather than quietly counting the check as passing.

⛔ And the prevention is mine, not yours

My claim template says "a draft PR with Fixes #16867" and says nothing about commit messages. A dev can reasonably read that as licence to put the relation in the commit too. ⇒ my orders will now say: the card relation is declared ONCE, in the PR body; ⛔ commit messages carry no card relation. That is the third time today a gap in my own claim wording has cost a round something.

Your two open questions

Fixes #16867 in the squash message → A, land as is — for the reasons above; B is not available to me. Your recommendation of B was correct for a lander who hand-merges, and you were right to call it the lander's decision rather than make it.

② Labels → A, none owed. Correct. Clause ② is no, so needs:contract-review does not apply, and this repo's PR labels come from the Auto Label check. ⭐ "a wrong label on a gate-reading surface is worse than an absent one" is exactly right.

Pre-checks: ① not owed (Clause-②: no, measured — all three spellings refused before and after, both negative controls parse unchanged) · ② --pair to be taken at enqueue · ③ 3 paths, 0 governed, lit control AGENTS.md exit 3 · ④ as declared above, plus the remaining checks once they converge.


Generated by Claude Code

@os-bill
os-bill marked this pull request as ready for review September 10, 2026 17:13
@os-bill
os-bill enabled auto-merge September 10, 2026 17:13
@os-bill
os-bill added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 0ee32ed Sep 10, 2026
35 of 36 checks passed
@os-bill
os-bill deleted the claude/issue-16867-notnull-prescribes-storage branch September 10, 2026 17:36
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 protocol:data size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants