Skip to content

fix(spec): withdraw the ADR-0087 field-required-notnull-explicit conversion — required: true stops stamping storage.notNull - #16890

Merged
os-musk merged 7 commits into
mainfrom
claude/issue-16693-adr87-required-notnull-conversion
Sep 8, 2026
Merged

fix(spec): withdraw the ADR-0087 field-required-notnull-explicit conversion — required: true stops stamping storage.notNull#16890
os-musk merged 7 commits into
mainfrom
claude/issue-16693-adr87-required-notnull-conversion

Conversation

@os-musk

@os-musk os-musk commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16693

Clause-②: no

Executes the maintainer ruling recorded on the card (director seat, decision batch #85, 2026-09-08) — option A: the ADR-0087 field-required-notnull-explicit conversion drops its storage claim. Quoted rather than paraphrased:

the conversion no longer adds storage.notNull; the warning's remedy sentence (duplicated across compile.ts / lint.ts / validate.ts, #13743) states the write-time contract only and never prescribes the column tightening; the falsified comment at sql-driver.ts:16432 is corrected. No migration is owed to anyone … The two wording fixes may land ahead of the conversion change. Governed text (docs/adr/**) is not edited by the executing PR.

ADR-0113 is the protocol: required is the write-time contract and is not a column constraint; storage.notNull alone binds the physical column. A conversion that adds storage.notNull wherever it finds required: true asserts exactly the implication that ADR abolished.


The mandatory first measurement: the artifact door APPLIES the conversion, it does not merely report it

The order required this to be established before any edit, because it decides the shape of the fix. It was taken on this branch, with controls, and it refutes the reading that retiredFromLoadPath: true held the transform to os migrate meta.

Which path emits the boot line. packages/metadata/src/plugin.ts, method _convertArtifactForward — it builds the [MetadataPlugin] artifact … predates this runtime's spec … converted N site(s) forward via ADR-0087 conversion … string, and its only transform is applyArtifactForwardConversions from @objectstack/metadata-core. That function's return value is not discarded: _parseAndRegisterArtifact feeds it to EnvironmentArtifactSchema.parse / ObjectStackDefinitionSchema.parse at all three of its call sites and registers the result.

Whether it applies. applyArtifactForwardConversions calls applyConversions(definition, { includeRetired: true, … }) and returns converted. The includeRetired: true is what overrides the flag — the loop it overrides reads:

// A retired entry is graduated chain history (ADR-0087 D2 window, second
// half): the loader no longer accepts its old shape — only `migrate meta`
// (and the fixture CI) replays it, via `includeRetired`.
if (conversion.retiredFromLoadPath && !includeRetired) continue;

Measured at that seam, by ablation on this branch — the withdrawn entry re-added from the merge base, @objectstack/spec and @objectstack/metadata-core rebuilt, the mutation proved live in dist/ by scripts/ablation-dist-preflight.mjs (marker present in 6 built files) before any verdict was read:

                                      ABLATED (entry re-added)      HEAD (entry withdrawn)
[SUBJECT floor ^17.0.0] verdict       converted-forward             converted-forward
[SUBJECT floor ^17.0.0] notices       ["field-required-notnull-…"]  []
[SUBJECT floor ^17.0.0] storage.notNull  true                       undefined
[SUBJECT floor ^17.0.0] required kept    true                       true

[NEG CTRL floor ^99.0.0] verdict      authored-current              authored-current   (window shut)
[FIRING CTRL same floor] notices      ["action-inert-keys-removed" x2]  (same, both legs)
[FIRING CTRL same floor] REAL rewrite  true                          true   (copy-on-write; the shortcut key is gone)

[applyConversions includeRetired=false]  notices=[]  storage.notNull=undefined   (both legs)
[applyConversions includeRetired=true ]  notices=["field-required-…"] / []

The negative control shows the instrument can say NO; the firing control shows the retired window is open on the same floor for a different retired conversion, so the empty subject line at HEAD is a verdict, not a dead instrument; the includeRetired=false leg shows the flag itself is not broken — it is overridden by the caller.

So the ruling's first clause is live and non-vacuous. Removing the registry entry is what stops the stamping; nothing else on the load path was holding it back. os migrate meta's pre-17 path is untouched, exactly as the order required.

The three clauses, and what each turned out to cost

1 — the conversion no longer adds storage.notNull. The entry leaves CONVERSIONS_BY_MAJOR, protocol 17's ADR-0087 ledger step, and the generated spec-changes.json / protocol-upgrade-guide.md rows. A tombstone docblock stands where the entry was, so re-adding one is a mistake with a comment in its way.

2 — the remedy sentence: measured, and there is no hand-written sentence to rewrite. This is the half the dispatching seat flagged as possibly missing, and it is worth stating what the tree actually holds:

  • git grep -n "notNull" over packages/cli/src/commands/compile.ts, lint.ts and validate.ts returns zero hits.
  • All three print one string, from one function: formatConversionNotice in packages/cli/src/utils/format.ts. The ADR-0087 conversion-notice wording is duplicated verbatim across all three authoring commands, and no gate holds the three equal #13743 already hoisted the three verbatim copies into it, and its docblock says so in the past tense. That function carries no remedy clause at all.
  • The literal remedy sentence lives in exactly two places, both fully parameterised on the conversion's own from / to pair: packages/spec/src/conversions/apply.ts`Update the source to '${detail.to}'.` — which is the one the boot warning echoes through agg.message, and packages/spec/src/stack.zod.ts, whose wording is the constant "Update the source to the canonical shape" and never names a column.

The boot line said Update the source to 'storage.notNull: true'. only because the conversion declared to: 'storage.notNull: true'. Withdrawing the conversion is what removes the prescription; rewording any of the three commands would either be a no-op or would change every other conversion's notice. ⇒ Clause 2 is discharged by clause 1, and the ruling's parenthetical about a three-way duplication is stale against this tree rather than wrong about the intent.

3 — the falsified comment in sql-driver.ts. Anchored on its text, not on a line number (:16432 in the ruling had already drifted to :16591, and drifted again under this branch's merge). The sentence that stood there:

Sources authored before protocol 17 carry storage.notNull explicitly via the field-required-notnull-explicit conversion, so their columns come out exactly as they always did.

is replaced by what the block actually does: a column reaches notNullable() there because its author wrote storage: { notNull: true }, and for no other reason. createColumn has one notNullable() branch and no dialect switch — the only other notNullable() on a managed column in that file is the SQLite table-rebuild path, which reads physical introspection plus the drift entry sets, never required.

One correction beyond the fence, because this change is what makes it false

packages/drivers/driver-sql/src/schema-drift.ts carried the same falsified claim as the sql-driver.ts sentence, in user-facing output. The relax_not_null finding — raised when a column is NOT NULL and the metadata declares no storage constraint — prescribed:

(pre-protocol-17 sources: os migrate meta stamps it for every previously-required field)

and the comment above it closed with "os migrate meta ratifies it whenever the source is next migrated". Withdrawing the conversion in this PR is precisely what makes both untrue: after it, no chain step writes storage.notNull for anybody, at any protocol floor. Both now say the constraint is the author's own declaration.

⚠️ The behaviour is untouched: the deliberate silence for a required: true field whose column is already NOT NULL, and both drift arms, are byte-for-byte the same. This corrects two sentences, never a finding. @objectstack/driver-sql is added to the changeset because one of the two is a message users read.

The two files outside the order's original fence, and why each has to move

Neither is discretionary, and "it pins the conversion" is not by itself a reason — here is the measured one for each.

packages/cli/test/migrate-meta.e2e.test.ts — forced, and invisible to this PR's CI. Reverting only this file to its merge-base bytes against the fixed tree was run as its own leg: the anchor was proved on disk first (0 occurrences at HEAD, 1 after the revert), no rebuild was needed because the subject resolves through packages/spec/dist, which already carries the fix. Result — Tests 2 failed | 16 passed:

AssertionError: expected a rewrite from field-required-notnull-explicit; got action-execute-to-target,
  sharing-rule-access-level-full-to-edit, permission-rls-priority-removed, tool-inert-authoring-keys-removed,
  action-inert-keys-removed, flow-inert-keys-removed, view-inert-keys-removed, dashboard-inert-keys-removed,
  agent-knowledge-removed, skill-trigger-phrases-removed: expected false to be true
AssertionError: expected undefined to deeply equal { notNull: true }

The file states the old contract twice — in EXPECTED_CONVERSIONS, and in a snapshot assertion that title.storage equals { notNull: true } — so leaving it untouched lands a red. It is also a *.e2e.test.ts, which since #16481 runs nightly on main, not on this PR: OS_TEST_TIERS unset selects the 212 non-tier files and this file is in neither project's include list. Left alone it would have gone red on main after the merge, hours later, off the PR that caused it. It was run here under OS_TEST_TIERS=nightly --project integration in both directions.

The edit keeps the conversion's input rather than deleting it: title is still authored required: true in the protocol-16 fixture, and the new assertion is that the chain attributes nothing to the withdrawn id while still attributing rewrites to others — so it cannot pass vacuously on an empty applied list.

packages/metadata-core/src/artifact-forward-conversion.test.ts — the seam is the defect's blast radius, and only this package can state it. The spec-side pin proves the registry holds no such conversion in either posture. It would keep passing if the stamp came back at the door instead — and the door is where this reached users: an artifact declaring ^17.0.0, the range create-objectstack stamps, converted at boot because the key is the declared engines.protocol floor and not the artifact's age. The regression users felt is "a scaffolded app boots with NOT NULL columns nobody asked for", and that sentence is only expressible in this package's terms. The pin carries its own anti-vacuity (the window really is open on the input: verdict is converted-forward and authoredFloor is 17.0.0), a positive case that an author-declared storage.notNull still survives the door, and a firing control asserting a real rewrite by a different retired conversion on the same floor. Under the ablation above, this file goes red — Tests 1 failed | 18 passed — so it discriminates.

Verification

Run on the final head, after git merge origin/main through scripts/pm/os-regen-merge.sh (merge committed first, then regenerated; git status --porcelain empty afterwards).

what result
pnpm --filter @objectstack/spec test Test Files 465 passed (465) · Tests 12972 passed (12972) · exit 0
pnpm --filter @objectstack/spec typecheck exit 0 · test layer holds at 54 files / 261 errors / 145 pinned signatures
pnpm --filter @objectstack/metadata-core test Test Files 15 passed (15) · Tests 264 passed (264) · exit 0
pnpm --filter @objectstack/metadata-core typecheck exit 0
pnpm --filter @objectstack/driver-sql test Test Files 161 passed | 10 skipped (171) · Tests 2445 passed | 145 skipped · exit 0
pnpm --filter @objectstack/driver-sql typecheck exit 0
pnpm --filter @objectstack/cli exec vitest run --project unit Test Files 186 passed (186) · Tests 2555 passed | 6 expected fail · exit 0
OS_TEST_TIERS=nightly … --project integration test/migrate-meta.e2e.test.ts Test Files 1 passed (1) · Tests 19 passed (19) · exit 0
pnpm --filter @objectstack/cli typecheck exit 0
generated artifacts check:spec-changes "spec-changes.json is up to date" · check:upgrade-guide "protocol-upgrade-guide.md is up to date" · check:authorable-surface verified against upstream 5aef6db · check:generated "All 15 generated artifacts are up to date" — all exit 0
eslint . --no-inline-config (the whole repo, not a narrowing) 6369 files · 0 errors · 0 warnings · exit 0

Gates. Derived with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack on this head and reconciled with --ran, verbatim:

✓ dispatch-gates --ran: 82 derived famil(ies) accounted for — 82 run, 0 NOT-MEASURED.

⚠️ That line is coverage, not verdicts. The verdicts, stated separately: all 82 exited 0, each exit code captured by redirecting to a log first and reading $? before any pipe. Nothing in the run returned 3, 99, 254 or a queue timeout.

Heavy runs went through scripts/pm/os-verify-lock.sh; every wall-clock figure above is a shared-box reading, as that script's own verdict lines say.

Ablation hygiene. Both ablations (the registry entry, and the pre-edit e2e file) restored via git checkout HEAD -- PATH under an EXIT INT TERM trap with absolute paths, each restore proved by blob-hash equality and an empty whole-tree git status --porcelain, and the registry one additionally by ablation-dist-preflight … --absent: marker absent from all 218 built files.

Reported, not edited — stale lines on paths this PR may not touch

Each still describes the withdrawn conversion as live. All are out of bounds for a code PR and are listed for whoever owns them:

  • docs/adr/0113-required-write-contract-vs-column-constraint.md lines 3 and 168 — governed (docs/adr/**); the ruling assigns this to a docs-only governed PR for the maintainer.
  • skills/objectstack-upgrade/SKILL.md lines 431 and 461 — governed (skills/**). Line 461 is a worked example whose right-hand column is the stamp this PR removes.
  • .claude/skills/spec-property-retirement/SKILL.md line 212 — governed (.claude/**). It teaches the retiredFromLoadPath reasoning the measurement above refutes.
  • content/docs/releases/v17.mdx lines 892 and 894 — release notes, written centrally at release time, never a rider on a code PR.
  • packages/spec/CHANGELOG.md, packages/objectql/CHANGELOG.md, packages/drivers/driver-sql/CHANGELOG.md — generated, not hand-edited.

验收备注


Generated by Claude Code

claude and others added 7 commits September 8, 2026 10:44
…ta` supplies `storage.notNull`

The `relax_not_null` finding prescribed "(pre-protocol-17 sources: `os migrate
meta` stamps it for every previously-required field)" as the way a NOT NULL
column with no declared storage constraint gets ratified, and the comment above
it closed with "`os migrate meta` ratifies it whenever the source is next
migrated". Withdrawing the `field-required-notnull-explicit` conversion in this
same PR is what makes both false: no chain step writes `storage.notNull` for
anybody, at any protocol floor.

Both now say the constraint is the author's own declaration. The deliberate
SILENCE for a `required: true` field whose column is already NOT NULL is
untouched — this corrects the sentences, never the finding.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/driver-sql, @objectstack/spec, touching 7 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/spec/spec-changes.json), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/ai/agents.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/api/client-sdk.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/api/error-catalog.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/data-modeling/index.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/deployment/validating-metadata.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/permissions/authorization.mdx (via crm_lead (literal, a string literal in fixture))
  • content/docs/ui/index.mdx (via crm_lead (literal, a string literal in fixture))

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

  • content/docs/releases/v13.mdx (via crm_lead (literal, a string literal in fixture))

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
  • 1 changed file(s) yielded no anchor (packages/spec/spec-changes.json) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 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 — 133 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 afa3a2698f5f1e24294f509ab9f0afefbdd424b8packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json afa3a2698f5f1e24294f509ab9f0afefbdd424b8

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

os-musk commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Landing — ready + auto-merge armed. All three pre-checks PASS, and ② is a real pass this time.

① In-seat contract tier — n/a. Clause-②: no; neither PR nor card #16693 carries needs:contract-review. ⛔ Not waived, absent.

② Both carriers, machine-read. Pair document built with files regenerated from git (5aef6db0f3..371d28c357) and the dev's real body (23,439 bytes):

document findings UNJUDGED-finding bullets
the real pair 0 0
control A — declaration line stripped 1 0
control B — whole claim comment removed 1 0

The second column is the one that matters, and it is why this is not the same reading as #16840. There, the script returned 0 findings with 1 pair(s) UNJUDGED — a 0-out-of-0 that had to be substituted for (#16833). Here the summary reads 0 clause-② finding(s), 0 pair(s) UNJUDGED and the UNJUDGED-finding bullet count is 0. ⇒ the limb was actually read.

⚠️ Stated honestly: the negative control on that bullet grep also returns 0, which is expected and does not discriminate — a pattern matching nothing always returns 0. The discriminating evidence is the contrast with #16840, where the same grep returns 1 on the same instrument. ⛔ A control that cannot fail is not a control, and this note does not pretend otherwise.

③ Every check green. Head 371d28c357: 11 workflow runs, CI success, Lint & Type Check success, PR Automation, Spec Liveness, Governed Surface Guard and every other guard success; one skipped (Pack Smoke, opt-in). Zero failures, zero in_progress.

Closing-keyword scan of the whole body: exactly one keyword+issue pair — Fixes #16693.

Landing is verified afterwards by content on origin/main, ⛔ never the API's merged field; pm:dispatched comes off #16693 then.

⚠️ Carried forward so it is not assumed dead with the card: the #16693 card's downstream half is unresolved — this round proved the door does apply, so "converted 40 site(s) / 0 NOT NULL columns" sits downstream of it, and it stays live for the other 73 retired entries. That is a measurement card the seat has taken and will not file blind. Recorded at 5586854846.


Generated by Claude Code

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