Skip to content

Commit f2b5e46

Browse files
os-muskclaudeos-musk
authored
fix(spec): withdraw the ADR-0087 field-required-notnull-explicit conversion — required: true stops stamping storage.notNull (#16890)
* wip: withdraw field-required-notnull-explicit conversion * fix(spec): withdraw the field-required-notnull-explicit conversion (ADR-0113) * chore: changeset for the withdrawn ADR-0087 conversion * docs(spec): point the withdrawn-conversion tombstone at the mechanism card * test(metadata-core): make the artifact-door firing control assert a real rewrite * fix(driver-sql): the drift notice stops promising that `os migrate meta` 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 --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: os-musk <elon@objectstack.ai>
1 parent 68aee4c commit f2b5e46

10 files changed

Lines changed: 283 additions & 106 deletions

File tree

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/driver-sql": patch
4+
---
5+
6+
fix(spec): withdraw the `field-required-notnull-explicit` ADR-0087 conversion — `required: true` no longer stamps `storage.notNull: true` on anybody's fields (#16693)
7+
8+
ADR-0113 split the pre-17 `required` tri-binding on purpose: `required` is the **write-time contract** and is NOT a column constraint, and `storage.notNull` alone binds the physical column (`sql-driver.ts#createColumn` has keyed off it alone since that ADR's P0). The `field-required-notnull-explicit` conversion asserted exactly the implication the ADR abolished — it added `storage: { notNull: true }` to every field it found `required: true` on — so it is removed from the conversion registry and from protocol 17's ADR-0087 ledger entry.
9+
10+
**Who this was reaching, and why it was not confined to old artifacts.** The entry carried `retiredFromLoadPath: true` and a docblock stating that "only `os migrate meta --from <16 or lower>` may apply it". That was not true of this tree. The artifact-ingestion door replays the whole chain with `includeRetired: true` (`applyArtifactForwardConversions`, `@objectstack/metadata-core`) and keys the replay off the artifact's declared `engines.protocol` **floor**, not its age — so any artifact declaring `^17.0.0`, which is the range `create-objectstack` stamps, was converted at boot. Measured on this tree at that seam: an artifact declaring `^17.0.0` on a 17.3.0 runtime came back from the door with `storage.notNull: true` written onto a field its author wrote as nullable-and-write-gated, and the boot logged `converted N site(s) forward` with a remedy sentence telling the author to write the same tightening into the source. On a populated database that instruction is a `tighten_not_null` / `severity: error` / `category: destructive` migration — prescribed as the remedy for a deprecation notice.
11+
12+
**What moves for consumers.**
13+
14+
- `applyConversions(stack, { includeRetired: true })` — the artifact-ingestion door and `os migrate meta` — no longer emits or applies this rewrite. The default load posture (`includeRetired: false`) is unchanged: the conversion was already skipped there.
15+
- `os migrate meta --from 16` no longer lists it, and a `required: true` field crosses 16 → 17 carrying its write contract and nothing else.
16+
- Boot no longer warns about it, so an artifact whose only conversion was this one now boots with that warning gone.
17+
- Nothing is authored differently and nothing is refused that was accepted before. `required` and `storage.notNull` both remain authorable and both keep their ADR-0113 meanings. A column is NOT NULL because its author wrote `storage: { notNull: true }`, and for no other reason.
18+
19+
**No migration is owed to anyone** (maintainer ruling, 2026-09-08, decision batch #85, option A). Genuinely pre-ADR-0113 artifacts are not measured to exist, existing columns are left exactly as they are, and an app that wants NOT NULL columns declares `storage.notNull` deliberately — which is what the app that reported this had already done.
20+
21+
The protocol-17 ledger entry and the generated upgrade guide now say this in the other direction too, and the falsified sentence in `sql-driver.ts` — "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 corrected where it stood.
22+
23+
Two sentences in `@objectstack/driver-sql` that this withdrawal falsifies are corrected with it, and no drift behaviour changes. The `relax_not_null` finding — raised when a column is NOT NULL and the metadata declares no `storage` constraint — used to prescribe "(pre-protocol-17 sources: `os migrate meta` stamps it for every previously-required field)"; it now says the constraint has to be declared by its author, because nothing supplies it any more. The comment beside it, which closed with "`os migrate meta` ratifies it whenever the source is next migrated", says so too. The deliberate SILENCE for a `required: true` field whose column is already NOT NULL is unchanged — this corrects the sentences, never the finding.

docs/protocol-upgrade-guide.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ The same close-out retires the four inert tool authoring keys (`category`, `perm
136136

137137
The AppSchema sheds its seven dead authoring keys (2026-06 liveness audit, #4001 app step): `version` (apps are versioned by manifest.version), `aria`, `objects`/`apis` (the self-described "config file convenience" — nothing read them; the chatbot derives an app's objects from its nav items), `sharing`/`embed` (a declared-but-unenforced public surface — the only live path is FormView.sharing; ADR-0049), and `mobileNavigation` (fully unimplemented). Pure lossless deletes — none ever had a runtime effect; each key is tombstoned with its prescription.
138138

139-
ADR-0113 splits the `required` tri-binding: post-17, `required` is ONLY the write-time contract (insert must provide; update may not null out; legacy null rows rest), and the physical NOT NULL is the explicit `storage.notNull`. The `field-required-notnull-explicit` conversion preserves every pre-17 source verbatim-in-meaning by stamping `storage.notNull: true` onto each required field — under the old semantics that column WAS created NOT NULL, so the rewrite writes down what the text already meant. Migration-chain-only (retired from the load path): this is a default flip, not a rename, and a loader that auto-applied it would stamp the constraint onto 17-authored sources that deliberately omit it.
139+
ADR-0113 splits the `required` tri-binding: post-17, `required` is ONLY the write-time contract (insert must provide; update may not null out; legacy null rows rest), and the physical NOT NULL is the explicit `storage.notNull`. ⚠️ NOTHING converts the column half for you, and nothing tightens a column you already have. A `field-required-notnull-explicit` conversion did stamp `storage.notNull: true` onto every `required: true` field; it was WITHDRAWN (maintainer ruling 2026-09-08), because stamping the constraint wherever `required: true` appears is exactly the implication the ADR abolished — and because the artifact-ingestion door replays retired conversions, so the LOADER applied it to 17-authored sources that deliberately omit it and then told their authors to write the same tightening into the source, which on a populated database is a destructive `tighten_not_null` migration prescribed as the remedy for a deprecation notice. Post-17 a column is NOT NULL because its author wrote `storage: { notNull: true }`, and for no other reason. If you are upgrading a pre-17 source whose columns ARE NOT NULL and you want them to stay that way, add `storage: { notNull: true }` to those fields yourself — deliberately, and knowing that doing it to a field whose column is currently nullable is a destructive migration with a backfill ceremony.
140140

141141
On the wire contract it also retires the `/analytics/query` request ENVELOPE (#3878): `AnalyticsQueryRequestSchema` used to describe `{ cube, query: {...}, format }` — the dialect of the retired degraded analytics shim (#3891) that the real engine never understood (an envelope body inferred a column-less cube and died as an SQL syntax error). The canonical request body is now the BARE AnalyticsQuery — `cube` + `measures` at the top level — which is what every real caller already sends; the schema tombstones `query`/`format`, and the dispatcher entry validates bodies and answers 400 with the prescription. No stored metadata carries this shape (it was HTTP-only), so the change is two semantic TODOs for API callers rather than a stack conversion.
142142

@@ -261,7 +261,6 @@ Finally it removes the 'pdf' member of `view.exportOptions` formats (#8010, main
261261
| `tool-inert-authoring-keys-removed` | `tool.category / tool.permissions / tool.active / tool.builtIn` | tool keys 'category'/'permissions'/'active'/'builtIn' removed (#3896 close-out — authorable and inert; permissions gated nothing, active:false withdrew nothing) | retired — `migrate meta` only |
262262
| `app-dead-authoring-keys-removed` | `app.version / app.aria / app.objects / app.apis / app.sharing / app.embed / app.mobileNavigation / app.contextSelectors.includeAll / app.contextSelectors.placement / app.homePageId / app.areas.order` | app keys 'version'/'aria'/'objects'/'apis'/'sharing'/'embed'/'mobileNavigation'/'homePageId' plus contextSelectors 'includeAll'/'placement' and areas 'order' removed (liveness audits #4001, #4509, #4667 — unread or wrongly encoded; sharing/embed declared a public surface no route enforced, mobileNavigation was fully unimplemented, includeAll was deliberately disobeyed because an 'All' row would clear a mandatory scope, homePageId WAS read by objectui's console before v17 but encoded the landing page as an ID cross-reference that silently fell back when it dangled — the landing page is the first nav item (premise corrected in #4709; the retirement stands), and no renderer ever sorted areas) | retired — `migrate meta` only |
263263
| `app-area-fail-open-gates-removed` | `app.areas.visible / app.areas.requiredPermissions` | navigation-area keys 'visible'/'requiredPermissions' removed (#4651, ADR-0049 — FAIL-OPEN access gates: no layer ever read them, so a 'hidden' or permission-gated area was served and rendered to every user, while the identically named keys on a navigation ITEM and on the APP are enforced; gate the items inside the area, or gate the app) | retired — `migrate meta` only |
264-
| `field-required-notnull-explicit` | `object.fields.*.required / object.fields.*.storage.notNull` | required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract) | retired — `migrate meta` only |
265264
| `action-inert-keys-removed` | `action.shortcut / action.bulkEnabled` | action keys 'shortcut'/'bulkEnabled' removed (#3896 close-out — no keydown path dispatches shortcuts; the multi-select toolbar reads the view's bulkActions) | retired — `migrate meta` only |
266265
| `flow-inert-keys-removed` | `flow.active / flow.template / flow.nodes[].outputSchema / flow.errorHandling.fallbackNodeId` | flow keys 'active'/'template', node 'outputSchema' and errorHandling 'fallbackNodeId' removed (#3896 close-out — active:false never stopped a flow; status is the enforced lifecycle) | retired — `migrate meta` only |
267266
| `view-inert-keys-removed` | `view.list.responsive / view.list.performance / view.form.defaultSort / view.form.aria` | view keys removed (#3896 close-out): list 'responsive'/'performance', form 'defaultSort'/'aria' — no renderer read them (list aria/data and form data stay live) | retired — `migrate meta` only |

packages/cli/test/migrate-meta.e2e.test.ts

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ const TSX = resolve(HERE, '../../../node_modules/.bin/tsx');
3434
/**
3535
* A stack authored against protocol 16: every line marked `// 16:` is a shape
3636
* the v17 chain must rewrite, spanning each conversion family — renames
37-
* (action execute→target, sharing full→edit), the required→storage.notNull
38-
* explicitization, and the #3896 close-out removals (rls.priority, the four
37+
* (action execute→target, sharing full→edit) and the #3896 close-out removals (rls.priority, the four
3938
* tool keys, flow active/template/outputSchema/fallbackNodeId, view/dashboard
4039
* inert keys, agent.knowledge, skill.triggerPhrases).
4140
*/
@@ -48,7 +47,7 @@ export default {
4847
name: 'e2e_ticket',
4948
label: 'Ticket',
5049
fields: {
51-
title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL
50+
title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL — v17 does NOT write that down (#16693)
5251
notes: { type: 'textarea', label: 'Notes' },
5352
},
5453
}],
@@ -147,7 +146,6 @@ const EXPECTED_CONVERSIONS = [
147146
'skill-trigger-phrases-removed',
148147
'tool-inert-authoring-keys-removed',
149148
'permission-rls-priority-removed',
150-
'field-required-notnull-explicit',
151149
'sharing-rule-access-level-full-to-edit',
152150
];
153151

@@ -187,6 +185,21 @@ describe('os migrate meta --from 16 (e2e over the real CLI)', () => {
187185
}
188186
});
189187

188+
/**
189+
* ⛔ The negative half of the assertion above (#16693). A conversion that
190+
* stamps `storage: { notNull: true }` onto every `required: true` field
191+
* asserts the implication ADR-0113 abolished, so `migrate meta` must
192+
* attribute NOTHING to it — on a source (`title`) that would have triggered
193+
* it, which is what keeps this from passing vacuously.
194+
*/
195+
it('attributes nothing to the withdrawn required→storage.notNull conversion', () => {
196+
const ids = new Set(out.parsed.applied.map((a: any) => a.conversionId));
197+
expect(ids.has('field-required-notnull-explicit')).toBe(false);
198+
// Anti-vacuity: the same run DID attribute rewrites, so an empty `applied`
199+
// cannot be what makes the line above green.
200+
expect(ids.size).toBeGreaterThan(0);
201+
});
202+
190203
it('surfaces the semantic TODOs instead of auto-applying them', () => {
191204
expect(Array.isArray(out.parsed.todos)).toBe(true);
192205
expect(out.parsed.todos.length).toBeGreaterThan(0);
@@ -217,9 +230,16 @@ describe('os migrate meta --from 16 (e2e over the real CLI)', () => {
217230
expect(snap.tools[0].category).toBeUndefined();
218231
expect(snap.permissions[0].rowLevelSecurity[0].priority).toBeUndefined();
219232
expect(snap.sharingRules[0].accessLevel).toBe('edit');
220-
// ADR-0113 explicitization: the pre-17 required field carries its column
221-
// constraint in writing; the optional field gains nothing.
222-
expect(snap.objects[0].fields.title.storage).toEqual({ notNull: true });
233+
// ⛔ ADR-0113, #16693: the chain does NOT write a column constraint for the
234+
// author. `required: true` crosses 16→17 as the write-time contract and
235+
// nothing else, so `title` comes out with NO `storage` block — exactly like
236+
// the optional field beside it. The withdrawn `field-required-notnull-
237+
// explicit` conversion used to make this line read `{ notNull: true }`, and
238+
// an app that followed the boot warning it raised was performing a
239+
// destructive `tighten_not_null` migration on a populated database while
240+
// believing it was clearing a deprecation notice.
241+
expect(snap.objects[0].fields.title.required, 'the write contract survives the chain').toBe(true);
242+
expect(snap.objects[0].fields.title.storage, 'no column constraint is invented').toBeUndefined();
223243
expect(snap.objects[0].fields.notes.storage).toBeUndefined();
224244
});
225245

packages/drivers/driver-sql/src/schema-drift.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -858,8 +858,18 @@ export function diffManagedTable(args: {
858858
// deliberately SILENT: that is every pre-protocol-17 source after a
859859
// runtime upgrade, the write gate makes the column constraint
860860
// unreachable (harmless belt-and-suspenders), and nagging every legacy
861-
// required field would bury real drift. `os migrate meta` ratifies it
862-
// whenever the source is next migrated.
861+
// required field would bury real drift.
862+
//
863+
// ⚠️ Nothing ratifies it later either, and this block used to say the
864+
// opposite — "`os migrate meta` ratifies it whenever the source is next
865+
// migrated". That died with the ADR-0087 `field-required-notnull-explicit`
866+
// conversion, WITHDRAWN in #16693 (maintainer ruling 2026-09-08): no
867+
// chain step writes `storage.notNull` for anybody, at any protocol floor.
868+
// So the silence above is permanent until the author declares the
869+
// constraint themselves — which is the ADR-0113 posture (`required` is
870+
// the write contract; `storage.notNull` alone binds the column), not a
871+
// gap. The SILENCE itself is unchanged: this PR corrects the sentence,
872+
// never the behaviour.
863873
out.push({
864874
kind: 'nullability_mismatch',
865875
remoteName: table,
@@ -873,8 +883,8 @@ export function diffManagedTable(args: {
873883
message:
874884
`${table}.${fieldName}: the column is NOT NULL but the metadata declares no ` +
875885
`storage constraint. Ratify it by declaring \`storage: { notNull: true }\` ` +
876-
`(pre-protocol-17 sources: \`os migrate meta\` stamps it for every ` +
877-
`previously-required field), or deliberately relax the column via "os migrate".`,
886+
`yourself — nothing supplies it for you, and \`required: true\` does not imply ` +
887+
`it (ADR-0113) — or deliberately relax the column via "os migrate".`,
878888
});
879889
} else if (!expectNullable && col.nullable) {
880890
out.push({

packages/drivers/driver-sql/src/sql-driver.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16587,9 +16587,21 @@ export class SqlDriver implements IDataDriver {
1658716587
// constraint, not from `required` — `required` is the write-time
1658816588
// contract enforced by the record validator at the engine seam, and
1658916589
// binding the DDL to it made every post-deploy tightening a
16590-
// destructive migration. Sources authored before protocol 17 carry
16591-
// `storage.notNull` explicitly via the `field-required-notnull-explicit`
16592-
// conversion, so their columns come out exactly as they always did.
16590+
// destructive migration.
16591+
//
16592+
// ⚠️ NOTHING supplies `storage.notNull` on an author's behalf. The
16593+
// sentence that used to close this block said the opposite — that
16594+
// pre-17 sources "carry `storage.notNull` explicitly via the
16595+
// `field-required-notnull-explicit` conversion, so their columns come
16596+
// out exactly as they always did" — and it was measured false from both
16597+
// ends (#16693): a real `^17.0.0` app logged that conversion and got
16598+
// NULLABLE columns anyway, and the conversion itself has since been
16599+
// WITHDRAWN (maintainer ruling 2026-09-08), because stamping the
16600+
// constraint wherever `required: true` appears is the implication
16601+
// ADR-0113 abolished. So a column reaches `notNullable()` here because
16602+
// its author wrote `storage: { notNull: true }`, and for no other
16603+
// reason; a `required: true` field with no `storage` block gets a
16604+
// nullable column, at every protocol floor, on every dialect.
1659316605
if ((field as { storage?: { notNull?: boolean } }).storage?.notNull) col.notNullable();
1659416606
this.applyDeclaredColumnDefault(col, field, type);
1659516607
}

0 commit comments

Comments
 (0)