diff --git a/.changeset/adr-0113-withdraw-required-notnull-conversion.md b/.changeset/adr-0113-withdraw-required-notnull-conversion.md new file mode 100644 index 0000000000..91886fdc06 --- /dev/null +++ b/.changeset/adr-0113-withdraw-required-notnull-conversion.md @@ -0,0 +1,23 @@ +--- +"@objectstack/spec": patch +"@objectstack/driver-sql": patch +--- + +fix(spec): withdraw the `field-required-notnull-explicit` ADR-0087 conversion — `required: true` no longer stamps `storage.notNull: true` on anybody's fields (#16693) + +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. + +**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. + +**What moves for consumers.** + +- `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. +- `os migrate meta --from 16` no longer lists it, and a `required: true` field crosses 16 → 17 carrying its write contract and nothing else. +- Boot no longer warns about it, so an artifact whose only conversion was this one now boots with that warning gone. +- 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. + +**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. + +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. + +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. diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 60f894af29..075e5691de 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -136,7 +136,7 @@ The same close-out retires the four inert tool authoring keys (`category`, `perm 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. -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. +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. 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. @@ -261,7 +261,6 @@ Finally it removes the 'pdf' member of `view.exportOptions` formats (#8010, main | `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 | | `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 | | `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 | -| `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 | | `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 | | `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 | | `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 | diff --git a/packages/cli/test/migrate-meta.e2e.test.ts b/packages/cli/test/migrate-meta.e2e.test.ts index d667bc5e18..e1bd444973 100644 --- a/packages/cli/test/migrate-meta.e2e.test.ts +++ b/packages/cli/test/migrate-meta.e2e.test.ts @@ -34,8 +34,7 @@ const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); /** * A stack authored against protocol 16: every line marked `// 16:` is a shape * the v17 chain must rewrite, spanning each conversion family — renames - * (action execute→target, sharing full→edit), the required→storage.notNull - * explicitization, and the #3896 close-out removals (rls.priority, the four + * (action execute→target, sharing full→edit) and the #3896 close-out removals (rls.priority, the four * tool keys, flow active/template/outputSchema/fallbackNodeId, view/dashboard * inert keys, agent.knowledge, skill.triggerPhrases). */ @@ -48,7 +47,7 @@ export default { name: 'e2e_ticket', label: 'Ticket', fields: { - title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL + title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL — v17 does NOT write that down (#16693) notes: { type: 'textarea', label: 'Notes' }, }, }], @@ -147,7 +146,6 @@ const EXPECTED_CONVERSIONS = [ 'skill-trigger-phrases-removed', 'tool-inert-authoring-keys-removed', 'permission-rls-priority-removed', - 'field-required-notnull-explicit', 'sharing-rule-access-level-full-to-edit', ]; @@ -187,6 +185,21 @@ describe('os migrate meta --from 16 (e2e over the real CLI)', () => { } }); + /** + * ⛔ The negative half of the assertion above (#16693). A conversion that + * stamps `storage: { notNull: true }` onto every `required: true` field + * asserts the implication ADR-0113 abolished, so `migrate meta` must + * attribute NOTHING to it — on a source (`title`) that would have triggered + * it, which is what keeps this from passing vacuously. + */ + it('attributes nothing to the withdrawn required→storage.notNull conversion', () => { + const ids = new Set(out.parsed.applied.map((a: any) => a.conversionId)); + expect(ids.has('field-required-notnull-explicit')).toBe(false); + // Anti-vacuity: the same run DID attribute rewrites, so an empty `applied` + // cannot be what makes the line above green. + expect(ids.size).toBeGreaterThan(0); + }); + it('surfaces the semantic TODOs instead of auto-applying them', () => { expect(Array.isArray(out.parsed.todos)).toBe(true); expect(out.parsed.todos.length).toBeGreaterThan(0); @@ -217,9 +230,16 @@ describe('os migrate meta --from 16 (e2e over the real CLI)', () => { expect(snap.tools[0].category).toBeUndefined(); expect(snap.permissions[0].rowLevelSecurity[0].priority).toBeUndefined(); expect(snap.sharingRules[0].accessLevel).toBe('edit'); - // ADR-0113 explicitization: the pre-17 required field carries its column - // constraint in writing; the optional field gains nothing. - expect(snap.objects[0].fields.title.storage).toEqual({ notNull: true }); + // ⛔ ADR-0113, #16693: the chain does NOT write a column constraint for the + // author. `required: true` crosses 16→17 as the write-time contract and + // nothing else, so `title` comes out with NO `storage` block — exactly like + // the optional field beside it. The withdrawn `field-required-notnull- + // explicit` conversion used to make this line read `{ notNull: true }`, and + // an app that followed the boot warning it raised was performing a + // destructive `tighten_not_null` migration on a populated database while + // believing it was clearing a deprecation notice. + expect(snap.objects[0].fields.title.required, 'the write contract survives the chain').toBe(true); + expect(snap.objects[0].fields.title.storage, 'no column constraint is invented').toBeUndefined(); expect(snap.objects[0].fields.notes.storage).toBeUndefined(); }); diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index aa617f4032..4e4eb867db 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -858,8 +858,18 @@ export function diffManagedTable(args: { // deliberately SILENT: that is every pre-protocol-17 source after a // runtime upgrade, the write gate makes the column constraint // unreachable (harmless belt-and-suspenders), and nagging every legacy - // required field would bury real drift. `os migrate meta` ratifies it - // whenever the source is next migrated. + // required field would bury real drift. + // + // ⚠️ Nothing ratifies it later either, and this block used to say the + // opposite — "`os migrate meta` ratifies it whenever the source is next + // migrated". That died with the ADR-0087 `field-required-notnull-explicit` + // conversion, WITHDRAWN in #16693 (maintainer ruling 2026-09-08): no + // chain step writes `storage.notNull` for anybody, at any protocol floor. + // So the silence above is permanent until the author declares the + // constraint themselves — which is the ADR-0113 posture (`required` is + // the write contract; `storage.notNull` alone binds the column), not a + // gap. The SILENCE itself is unchanged: this PR corrects the sentence, + // never the behaviour. out.push({ kind: 'nullability_mismatch', remoteName: table, @@ -873,8 +883,8 @@ export function diffManagedTable(args: { message: `${table}.${fieldName}: the column is NOT NULL but the metadata declares no ` + `storage constraint. Ratify it by declaring \`storage: { notNull: true }\` ` + - `(pre-protocol-17 sources: \`os migrate meta\` stamps it for every ` + - `previously-required field), or deliberately relax the column via "os migrate".`, + `yourself — nothing supplies it for you, and \`required: true\` does not imply ` + + `it (ADR-0113) — or deliberately relax the column via "os migrate".`, }); } else if (!expectNullable && col.nullable) { out.push({ diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 37c3af1f3c..0e9b25dd2e 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -16587,9 +16587,21 @@ export class SqlDriver implements IDataDriver { // constraint, not from `required` — `required` is the write-time // contract enforced by the record validator at the engine seam, and // binding the DDL to it made every post-deploy tightening a - // destructive migration. 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. + // destructive migration. + // + // ⚠️ NOTHING supplies `storage.notNull` on an author's behalf. The + // sentence that used to close this block said the opposite — that + // pre-17 sources "carry `storage.notNull` explicitly via the + // `field-required-notnull-explicit` conversion, so their columns come + // out exactly as they always did" — and it was measured false from both + // ends (#16693): a real `^17.0.0` app logged that conversion and got + // NULLABLE columns anyway, and the conversion itself has since been + // WITHDRAWN (maintainer ruling 2026-09-08), because stamping the + // constraint wherever `required: true` appears is the implication + // ADR-0113 abolished. So a column reaches `notNullable()` here because + // its author wrote `storage: { notNull: true }`, and for no other + // reason; a `required: true` field with no `storage` block gets a + // nullable column, at every protocol floor, on every dialect. if ((field as { storage?: { notNull?: boolean } }).storage?.notNull) col.notNullable(); this.applyDeclaredColumnDefault(col, field, type); } diff --git a/packages/metadata-core/src/artifact-forward-conversion.test.ts b/packages/metadata-core/src/artifact-forward-conversion.test.ts index 5f6742ee59..8005bbf1c9 100644 --- a/packages/metadata-core/src/artifact-forward-conversion.test.ts +++ b/packages/metadata-core/src/artifact-forward-conversion.test.ts @@ -174,6 +174,95 @@ describe('applyArtifactForwardConversions — the versioned window (#12772)', () }); }); +/** + * ⛔ The artifact door must NOT invent a column constraint (#16693, maintainer + * ruling 2026-09-08, option A). + * + * This is the seam the card measured. `retiredFromLoadPath` does NOT hold a + * conversion back here — this module replays the chain with `includeRetired: + * true` on purpose — so a conversion that stamped `storage: { notNull: true }` + * onto every `required: true` field reached every artifact whose declared + * `engines.protocol` FLOOR sat below the running spec. `^17.0.0` is the range + * `create-objectstack` stamps, so that was every scaffolded app, from its first + * boot: NOT NULL columns nobody asked for, plus a warning instructing the + * author to write the same tightening into the source — a `destructive` + * `tighten_not_null` migration on any populated database, prescribed as the + * remedy for a deprecation notice. + * + * ADR-0113 is the protocol: `required` is the write-time contract and NOT a + * column constraint; `storage.notNull` alone binds the column, and only an + * author writes it. + */ +describe('the artifact door never stamps a column constraint (ADR-0113, #16693)', () => { + const requiredFieldDefinition = (protocolRange: string) => ({ + manifest: { + id: 'app.example.clm', name: 'clm', version: '1.0.0', type: 'app', + engines: { protocol: protocolRange }, + }, + objects: [{ + name: 'clm_party', + label: 'Party', + fields: { + name: { type: 'text', label: 'Name', required: true }, + notes: { type: 'textarea', label: 'Notes' }, + }, + }], + }); + + it('leaves `required: true` alone on an artifact the retired window IS open for', () => { + const def = requiredFieldDefinition('^17.0.0'); + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.3.0' }); + + // ⭐ ANTI-VACUITY, and the whole point of the pin: the window really is + // open on this input. A green line below because the door skipped this + // artifact entirely would prove nothing. + expect(result.verdict).toBe('converted-forward'); + expect(result.authoredFloor).toBe('17.0.0'); + + const fields = (result.definition as { objects: { fields: Record }[] }) + .objects[0]!.fields; + expect(fields.name!.required, 'the write contract is untouched').toBe(true); + expect(fields.name!.storage, 'no NOT NULL is invented for the author').toBeUndefined(); + expect(fields.notes!.storage).toBeUndefined(); + expect(result.notices.map((n) => n.conversionId)).not.toContain('field-required-notnull-explicit'); + // Copy-on-write: nothing was recognized, so the door hands back the same + // reference it was given. + expect(result.definition).toBe(def); + }); + + it('keeps an explicitly declared `storage.notNull` — the author\'s own act still binds the column', () => { + const def = requiredFieldDefinition('^17.0.0') as unknown as { + objects: { fields: Record> }[]; + }; + def.objects[0]!.fields.name!.storage = { notNull: true }; + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.3.0' }); + expect(result.verdict).toBe('converted-forward'); + expect((result.definition as typeof def).objects[0]!.fields.name!.storage).toEqual({ notNull: true }); + }); + + /** + * ⭐ FIRING CONTROL for the two assertions above. They are negatives, so they + * are worthless unless this instrument can still be made to say YES in the + * same window — a door that had stopped converting anything at all would make + * them green for the wrong reason. + */ + it('still replays other retired conversions in that same window (the instrument fires)', () => { + const def = legacyPermissionDefinition('^17.0.0'); + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.3.0' }); + expect(result.verdict).toBe('converted-forward'); + expect(result.notices.length).toBeGreaterThan(0); + // A rewrite actually landed on the same floor the assertions above use: + // the retired permission bits are gone, and copy-on-write proves it by + // handing back a DIFFERENT object than it was given. + const objects = (result.definition as { permissions: { objects: Record> }[] }) + .permissions[0]!.objects; + expect(objects.crm_ticket!.allowRestore).toBeUndefined(); + expect(objects.crm_ticket!.allowPurge).toBeUndefined(); + expect(objects.crm_ticket!.allowRead, 'only the retired bits move').toBe(true); + expect(result.definition).not.toBe(def); + }); +}); + describe('parseRangeFloor — the range spellings artifacts actually carry', () => { it.each([ ['^17.1.0', [17, 1, 0]], diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index dc80ec20ce..d2f7cc209a 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -158,12 +158,6 @@ "conversionId": "app-area-fail-open-gates-removed", "toMajor": 17 }, - { - "surface": "object.fields.*.required / object.fields.*.storage.notNull", - "to": "required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract)", - "conversionId": "field-required-notnull-explicit", - "toMajor": 17 - }, { "surface": "action.shortcut / action.bulkEnabled", "to": "action keys 'shortcut'/'bulkEnabled' removed (#3896 close-out — no keydown path dispatches shortcuts; the multi-select toolbar reads the view's bulkActions)", @@ -1313,12 +1307,6 @@ "conversionId": "app-area-fail-open-gates-removed", "toMajor": 17 }, - { - "surface": "object.fields.*.required / object.fields.*.storage.notNull", - "to": "required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract)", - "conversionId": "field-required-notnull-explicit", - "toMajor": 17 - }, { "surface": "action.shortcut / action.bulkEnabled", "to": "action keys 'shortcut'/'bulkEnabled' removed (#3896 close-out — no keydown path dispatches shortcuts; the multi-select toolbar reads the view's bulkActions)", diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index c18d5cf7f9..6155da6f37 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -1496,3 +1496,64 @@ describe('conversion layer (ADR-0087 D2)', () => { }); }); }); + +/** + * ⛔ The registry must carry NO conversion that turns `required: true` into a + * column constraint (#16693, maintainer ruling, decision batch #85, + * 2026-09-08, option A). + * + * ADR-0113 split the pre-17 tri-binding: `required` is the write-time contract + * and NOT a column constraint, `storage.notNull` alone binds the column. A + * conversion that adds `storage.notNull` wherever it finds `required: true` + * asserts exactly the implication the ADR abolished — and it did not stay in + * `os migrate meta` where its docblock claimed: the artifact-ingestion door + * replays retired conversions (`includeRetired: true`), so every artifact + * declaring a floor below the running spec got the stamp, plus a boot warning + * telling its author to write the same tightening into the source. That + * instruction is a `destructive` `tighten_not_null` migration on a populated + * database, issued as the remedy for a deprecation notice. + * + * Written against the SEAM rather than the id, so re-adding the behaviour + * under a different id fails here too. + */ +describe('no conversion invents a NOT NULL column (ADR-0113, #16693)', () => { + const requiredOnly = () => ({ + objects: [{ + name: 'clm_party', + label: 'Party', + fields: { + name: { type: 'text', label: 'Name', required: true }, + notes: { type: 'textarea', label: 'Notes' }, + }, + }], + }); + + it('leaves `required: true` alone in the load posture AND in the full replay', () => { + for (const includeRetired of [false, true]) { + const { stack, notices } = collectConversionNotices(requiredOnly(), { includeRetired }); + const fields = (stack as ReturnType).objects[0]!.fields as + Record; + expect(fields.name!.required, `required survives (includeRetired=${includeRetired})`).toBe(true); + expect(fields.name!.storage, `no NOT NULL invented (includeRetired=${includeRetired})`).toBeUndefined(); + expect(notices, `nothing to report (includeRetired=${includeRetired})`).toHaveLength(0); + } + }); + + it('no registered conversion declares the storage.notNull surface at all', () => { + const offenders = ALL_CONVERSIONS.filter((c) => c.surface.includes('storage.notNull')); + expect(offenders.map((c) => c.id)).toEqual([]); + // ⭐ ANTI-VACUITY: `surface` is a populated field on a populated registry, + // so the empty list above is a verdict and not an empty scan. + expect(ALL_CONVERSIONS.length).toBeGreaterThan(0); + expect(ALL_CONVERSIONS.every((c) => c.surface.length > 0)).toBe(true); + }); + + it("`storage.notNull` is still the author's own way to bind the column", async () => { + const { FieldSchema } = await import('../data/field.zod.js'); + const parsed = FieldSchema.safeParse({ + name: 'name', type: 'text', label: 'Name', required: true, storage: { notNull: true }, + }); + expect(parsed.success, JSON.stringify(parsed.success ? '' : parsed.error.issues.slice(0, 3))).toBe(true); + expect(parsed.data!.storage).toEqual({ notNull: true }); + }); +}); diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 3a8d38d387..df7ab777f7 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -2018,75 +2018,44 @@ const toolInertAuthoringKeysRemoved: MetadataConversion = { }; /** - * `required: true` gains its explicit `storage.notNull` (protocol 17, - * ADR-0113). - * - * Before protocol 17, `field.required` bound THREE meanings to one knob: the - * write-time contract, the physical NOT NULL DDL, and the drift expectation. - * ADR-0113 splits them: `required` keeps the write contract, and the column - * constraint becomes the explicit `storage: { notNull: true }`. Under the OLD - * semantics every required field's column was created NOT NULL, so this - * conversion preserves each old source's full meaning by WRITING IT DOWN — - * a pure semantic explicitization, lossless by construction. - * - * `retiredFromLoadPath` is load-bearing here in a way it is not for renames: - * a rename is idempotent on canonical input, but this is a DEFAULT FLIP — a - * protocol-17-authored `required: true` deliberately means "nullable column, - * write-gated", and a loader that auto-applied this transform would stamp - * NOT NULL onto it, silently restoring the tri-binding the ADR removed. Only - * `os migrate meta --from <16 or lower>` may apply it, where "this source - * predates the split" is a fact, not a guess. + * ⛔ WITHDRAWN — there is deliberately NO `field-required-notnull-explicit` + * conversion in this registry, and re-adding one is the mistake this comment + * exists to stop (#16693; maintainer ruling, decision batch #85, 2026-09-08, + * option A). + * + * It read `required: true` and wrote `storage: { notNull: true }` beside it, + * on the argument that pre-17 `field.required` bound three meanings to one + * knob — the write contract, the physical NOT NULL, and the drift + * expectation — so writing the constraint down preserved an old source's + * full meaning. ADR-0113 split those axes, and two facts retired the argument: + * + * 1. Adding `storage.notNull` wherever `required: true` appears IS the + * implication ADR-0113 abolished. `required` is the write-time contract; + * `storage.notNull` alone binds the column. A conversion cannot be what + * decides a column constraint — that is the author's explicit act. + * 2. `retiredFromLoadPath: true` did NOT hold the transform to + * `os migrate meta`, whatever this entry's docblock used to claim. The + * artifact-ingestion door replays the whole chain with `includeRetired: + * true` (`applyArtifactForwardConversions`, `@objectstack/metadata-core`), + * keyed off the artifact's declared `engines.protocol` FLOOR rather than + * its age — and `^17.0.0` is the range `create-objectstack` stamps. So + * every scaffolded app was handed NOT NULL columns it never asked for, plus + * a boot warning telling its 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. + * + * No migration is owed to anyone: genuinely pre-ADR-0113 artifacts are not + * measured to exist, and an app that wants NOT NULL columns declares + * `storage.notNull` deliberately. Existing columns are left exactly as they + * are. + * + * ⚠️ Fact 2 is about the MECHANISM, not about this entry, and it outlived the + * entry: `retiredFromLoadPath` still holds nothing back at three runtime seams + * (#16864). Before setting that flag on a DEFAULT FLIP — as opposed to a + * lossless delete or a rename — read that card, because the flag does not mean + * what its name and every docblock around it say it means. */ -const fieldRequiredNotNullExplicit: MetadataConversion = { - id: 'field-required-notnull-explicit', - toMajor: 17, - retiredFromLoadPath: true, - surface: 'object.fields.*.required / object.fields.*.storage.notNull', - summary: "required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract)", - apply(stack, emit) { - return mapCollection(stack, 'objects', (obj, path) => { - const fields = (obj as { fields?: Record> }).fields; - if (!fields || typeof fields !== 'object') return obj; - let touched = false; - const nextFields: Record = { ...fields }; - for (const [fieldName, def] of Object.entries(fields)) { - if (!def || typeof def !== 'object') continue; - if (def.required !== true) continue; - if (def.storage !== undefined) continue; // an explicit storage block wins - nextFields[fieldName] = { ...def, storage: { notNull: true } }; - emit({ from: 'required: true (implied NOT NULL)', to: 'storage.notNull: true', path: `${path}.fields.${fieldName}.storage.notNull` }); - touched = true; - } - return touched ? { ...obj, fields: nextFields } : obj; - }); - }, - fixture: { - before: { - objects: [{ - name: 'crm_lead', - label: 'Lead', - fields: { - name: { type: 'text', required: true }, - status: { type: 'select', required: true }, - notes: { type: 'textarea' }, - }, - }], - }, - after: { - objects: [{ - name: 'crm_lead', - label: 'Lead', - fields: { - name: { type: 'text', required: true, storage: { notNull: true } }, - status: { type: 'select', required: true, storage: { notNull: true } }, - notes: { type: 'textarea' }, - }, - }], - }, - expectedNotices: 2, - }, -}; - /** * The #3896 close-out sweep, part 2 (protocol 17): the remaining inert @@ -9020,7 +8989,6 @@ export const CONVERSIONS_BY_MAJOR: Readonly