diff --git a/.changeset/object-parse-path-strict.md b/.changeset/object-parse-path-strict.md new file mode 100644 index 0000000000..7d50fcf97c --- /dev/null +++ b/.changeset/object-parse-path-strict.md @@ -0,0 +1,22 @@ +--- +'@objectstack/spec': minor +--- + +`ObjectSchema` rejects unknown top-level keys on the PARSE path, not only in `create()` — closing the founding example of #4001, which had been live for the whole time #1535 was considered fixed. + +**The gap.** #1535 built the unknown-key guard as a hand-rolled check inside the `ObjectSchema.create()` factory, on the reasoning that authored `*.object.ts` modules call `create()`. They do — but they are not the only producer, and not the path most instances travel. `defineStack({ objects })`, `/api/v1/meta/types/object` and the Studio form all reach the schema through `parse()` / `safeParse()`, which kept stripping unknown keys in silence: + +``` +ObjectSchema.parse({ …, workflows: ['x'] }) → key silently discarded +ObjectSchema.create({ …, workflows: ['x'] }) → rejected since #1535 +``` + +Object-level `workflows: [...]` — the example this campaign was filed on, an author believing they had wired up automation and shipping dead metadata — was still reproducible on the main path. + +The base shape is now `.strict()` with the `UNKNOWN_KEY_GUIDANCE` tombstones and the semantic renames the warning layer already knew (`capabilities` / `features` → `enable`), so graduating from warn to reject costs an author no prescription. `create()` is unaffected: its own check runs before parsing and throws a richer located error. Safe on the read path for the same reason the other closed registered types are — the ADR-0010 envelope is declared, and `stripReadDecorations` removes `_diagnostics` / `_draft` before any strict re-parse. Verified rather than assumed: every `ObjectSchema.create()` call across `platform-objects` and the three example apps uses only declared top-level keys. + +**A new tombstone.** `namespace` (retired in ADR-0006 D4) had none, so it was stripped in silence — an object written as `{ namespace: 'sys', name: 'user' }` shipped as plain `user`, under a name its author never intended. The rejection now carries the fix (`name: "sys_user"`). + +**And a coverage regression this change would otherwise have introduced.** The unknown-key warning layer gated each metadata collection on its ROOT schema's posture, so closing `object` at the root switched off the warnings for everything *beneath* it too — its 71 nested strip-mode sites would have stopped reporting in the same change, with nothing to say so. Posture is a per-node property and the walk now treats it as one: a strict root stays silent at its own level (the parse owns that failure) while the descent continues. Nested `object.fields.*` warnings are unaffected by the graduation. + +Three tests that asserted the strip as correct behaviour are now rejection tests — `namespace`, the retired `compactLayout` alias, and the removed `detail` block. The `compactLayout` one had pinned the author-hostile outcome explicitly: "the retired key is STRIPPED, not aliased — an old-key author gets no highlightFields rather than silently working." diff --git a/packages/spec/src/data/object.test.ts b/packages/spec/src/data/object.test.ts index 6d977f5de6..27cd8dc258 100644 --- a/packages/spec/src/data/object.test.ts +++ b/packages/spec/src/data/object.test.ts @@ -997,16 +997,20 @@ describe('ObjectSchema name-as-identity', () => { } }); - it('strips legacy `namespace` keys from input (deprecated, dropped in D4)', () => { + it('REJECTS legacy `namespace` with the prefix-embedding fix (ADR-0006 D4)', () => { + // Until #4001 closed this shape on the parse path, this key was stripped in + // silence — so an object written as `{ namespace: 'sys', name: 'user' }` + // shipped as plain `user`, under a name its author never intended. The test + // that stood here asserted that strip as correct behaviour. const result = ObjectSchema.safeParse({ namespace: 'sys', name: 'user', fields: {}, } as Record); - expect(result.success).toBe(true); - if (result.success) { - expect((result.data as Record).namespace).toBeUndefined(); - } + expect(result.success).toBe(false); + const message = result.success ? '' : result.error.issues[0].message; + expect(message).toContain('`namespace`'); + expect(message).toContain('name: "sys_user"'); }); it('accepts prefix-embedded names without any tableName field', () => { @@ -1087,13 +1091,15 @@ describe('ObjectSchema semantic roles (ADR-0085)', () => { // The transition mirror is gone: output carries the canonical key only. expect((direct as Record).compactLayout).toBeUndefined(); - // Lenient parse: the retired key is STRIPPED, not aliased — an old-key - // author gets no highlightFields rather than silently working. - const legacy = ObjectSchema.parse({ + // The parse path REJECTS the retired key and carries the rename (#4001). + // It used to strip it, so an old-key author got no highlightFields and no + // diagnostic — the outcome this test previously pinned as correct. + const legacy = ObjectSchema.safeParse({ name: 'account', fields: {}, compactLayout: ['name', 'industry'], }); - expect(legacy.highlightFields).toBeUndefined(); - expect((legacy as Record).compactLayout).toBeUndefined(); + expect(legacy.success).toBe(false); + expect(legacy.success ? '' : legacy.error.issues[0].message) + .toContain('`compactLayout` was renamed to `highlightFields`'); // Authoring path: create() REJECTS the retired key like any unknown key. expect(() => @@ -1117,14 +1123,13 @@ describe('ObjectSchema semantic roles (ADR-0085)', () => { ).toThrow(/detail/); }); - it('strips the removed detail block on safeParse (no key on output)', () => { + it('REJECTS the removed detail block, carrying the ADR-0085 re-home map', () => { const result = ObjectSchema.safeParse({ name: 'product', fields: {}, detail: { renderViaSchema: false }, }); - expect(result.success).toBe(true); - if (result.success) { - expect((result.data as Record).detail).toBeUndefined(); - } + expect(result.success).toBe(false); + expect(result.success ? '' : result.error.issues[0].message) + .toContain('`detail` UI-hints block was removed by ADR-0085'); }); }); diff --git a/packages/spec/src/data/object.zod.ts b/packages/spec/src/data/object.zod.ts index f9e39a451d..5b28bb1341 100644 --- a/packages/spec/src/data/object.zod.ts +++ b/packages/spec/src/data/object.zod.ts @@ -12,6 +12,7 @@ import { ObjectListViewSchema } from '../ui/view.zod'; import { ExpressionInputSchema, TemplateExpressionInputSchema, type Expression, type ExpressionInput } from '../shared/expression.zod'; import { lazySchema } from '../shared/lazy-schema'; import { MetadataProtectionFields } from '../kernel/metadata-protection.zod'; +import { strictUnknownKeyError } from '../shared/suggestions.zod'; import { ProtectionSchema } from '../shared/protection.zod'; export const ApiMethod = z.enum([ 'get', 'list', // Read @@ -707,9 +708,60 @@ export const RowCrudActionOverrideSchema = z.object({ export type RowCrudActionOverride = z.infer; export type RowCrudActionOverrideInput = z.input; +/** + * Unknown-key error for {@link ObjectSchemaBase}, built on FIRST USE. + * + * Deferred because the pieces it needs — `UNKNOWN_KEY_GUIDANCE` and the shape's + * own key list — are declared *below* this schema, and `ObjectSchemaBase` is a + * plain `z.object` evaluated at module load. An error map only runs at parse + * time, so resolving them then sidesteps the temporal dead zone without moving + * several hundred lines around. `knownKeys` reads the shape rather than a + * transcription, for the reason `strictObject` exists. + */ +let objectUnknownKeyErrorImpl: z.core.$ZodErrorMap | undefined; +const objectUnknownKeyError: z.core.$ZodErrorMap = (issue) => + (objectUnknownKeyErrorImpl ??= strictUnknownKeyError({ + surface: 'this object', + knownKeys: Object.keys(ObjectSchemaBase.shape), + // The same semantic renames the WARNING layer already knew + // (`OBJECT_KEY_GUIDANCE`). Graduating a surface from warn to reject must + // not cost the author a prescription: `capabilities` → `enable` is a + // different word for the same intent, so edit distance cannot reach it and + // only an explicit entry can. + aliases: { capabilities: 'enable', features: 'enable' }, + guidance: UNKNOWN_KEY_GUIDANCE, + history: + 'Until #4001 closed this shape these were dropped silently on the PARSE path — ' + + '`ObjectSchema.create()` has rejected them since #1535, but `defineStack({ objects })`, ' + + '`/api/v1/meta/types/object` and the Studio form all go through `parse()`, which did not.', + }))(issue); + +/* + * ── Unknown-key strictness (#4001 registered-types line) ──────────────────── + * + * `.strict()` HERE, not only in `create()`. #1535 built the unknown-key guard + * as a hand-rolled check inside the `create()` factory, on the reasoning that + * "authored `*.object.ts` modules call `create()`". They do — but they are not + * the only producer, and they are not the path most instances travel: + * `defineStack({ objects })`, `/api/v1/meta/types/object` and the Studio form + * all reach this schema through `parse()` / `safeParse()`, which stripped + * unknown keys in silence for the whole time #1535 was considered fixed. The + * founding example of #4001 — object-level `workflows: [...]` believed to wire + * up automation, silently discarded — was still reproducible on `parse()`. + * + * `create()` is unaffected: its own check runs BEFORE parsing and throws a + * richer located Error, so its message still wins where it applies. + * + * Safe on the read path for the same reason the other closed registered types + * are: the ADR-0010 envelope is declared below, and `stripReadDecorations` + * removes `_diagnostics` / `_draft` before any strict re-parse (cloud#971). + * Verified empirically rather than assumed — every `ObjectSchema.create()` call + * across `platform-objects` and the three example apps uses only declared + * top-level keys. + */ const ObjectSchemaBase = z.object({ - /** - * Identity & Metadata + /** + * Identity & Metadata */ name: z.string().regex(/^[a-z_][a-z0-9_]*$/).describe('Machine unique key (snake_case). Immutable.'), label: z.string().optional().describe('Human readable singular label (e.g. "Account")'), @@ -1215,7 +1267,7 @@ const ObjectSchemaBase = z.object({ // ADR-0010 — runtime protection envelope (internal — set by loader). ...MetadataProtectionFields, -}); +}, { error: objectUnknownKeyError }).strict(); /** * Converts a snake_case name to a human-readable Title Case label. @@ -1265,6 +1317,13 @@ const UNKNOWN_KEY_GUIDANCE: Record = { // names what replaced the key and the version/decision that removed it. // Tombstones age out too: drop an entry ~two majors after the removal // (by then it's archaeology, not an upgrade; see CHANGELOG.md for history). + namespace: + '`namespace` was retired in ADR-0006 D4 — the object `name` IS the canonical id ' + + 'everywhere (API, ObjectQL, REST, SDK, DB table), so there is no separate namespace ' + + 'to declare. Embed the module prefix in the name instead: `namespace: "sys", ' + + 'name: "user"` becomes `name: "sys_user"`. Until #4001 closed this shape on the ' + + 'parse path it was stripped in silence, so an object declaring one shipped under ' + + 'the unprefixed name its author did not intend.', compactLayout: '`compactLayout` was renamed to `highlightFields` in @objectstack/spec 11.7.0 ' + '(ADR-0085 semantic roles) and the alias was retired in 11.9.1 (#2536). ' + diff --git a/packages/spec/src/kernel/metadata-authoring-lint.test.ts b/packages/spec/src/kernel/metadata-authoring-lint.test.ts index 9a5981b686..e2a4b68519 100644 --- a/packages/spec/src/kernel/metadata-authoring-lint.test.ts +++ b/packages/spec/src/kernel/metadata-authoring-lint.test.ts @@ -20,6 +20,7 @@ import { } from './metadata-authoring-lint'; import { STACK_KEY_GUIDANCE, STACK_RUNTIME_MEMBERS } from '../data/authoring-key-lint'; import { PLURAL_TO_SINGULAR } from '../shared/metadata-collection.zod'; +import { ObjectSchema } from '../data/object.zod'; import { ObjectStackDefinitionSchema } from '../stack.zod'; import { getMetadataTypeSchema } from './metadata-type-schemas'; @@ -36,12 +37,15 @@ describe('coverage derivation (#3786 — no third hand-written list)', () => { // from "lint warns" to "parse rejects", which is the campaign succeeding, not // coverage rotting. Lower it only after confirming the shrink against the // list below; that confirmation is the whole point of pinning a number here. - // 15 → 13 when `seed` + `doc` graduated (#4001 registered-types batch). - expect(lintables.length).toBeGreaterThanOrEqual(13); + // 15 → 13 when `seed` + `doc` graduated (#4001 registered-types batch); + // 13 → 12 when `object` closed on the parse path. Note what did NOT shrink + // with it: `object`'s 71 NESTED strip sites still report, because the walk + // no longer gates a whole collection on its root's posture. + expect(lintables.length).toBeGreaterThanOrEqual(12); // `view` matters doubly: it is a UNION (container | ViewItem | overlay), so // its presence pins the union half of the posture logic — a regression that // silently dropped unions would shrink coverage without failing the count. - for (const expected of ['object', 'page', 'agent', 'dashboard', 'action', 'report', 'view']) { + for (const expected of ['page', 'agent', 'dashboard', 'action', 'report', 'view']) { expect(lintableTypes, `expected '${expected}' to be lint-covered`).toContain(expected); } }); @@ -61,6 +65,10 @@ describe('coverage derivation (#3786 — no third hand-written list)', () => { // helper-built `.strict()` exactly like a hand-wired one. for (const strict of [ 'flow', 'permission', 'position', 'tool', 'app', 'hook', 'datasource', 'seed', 'doc', + // `object` graduated by closing its PARSE path — #1535 had only ever + // guarded `create()`. Its nested strip sites still report; only the root + // moved from warn to reject. + 'object', ]) { expect(lintableTypes, `'${strict}' is .strict(); the lint must not double-report`).not.toContain(strict); } @@ -117,13 +125,21 @@ describe('the #4148 behaviours survive the generalization', () => { expect(finding.suggestion).toBeUndefined(); }); - it('reports the renamed object key with its rename', () => { - const [finding] = lintUnknownAuthoringKeys(stackWith({ capabilities: { trackHistory: true } }, {})); - expect(finding).toMatchObject({ - path: 'objects.crm_case.capabilities', - surface: 'object', - suggestion: 'enable', + it('no longer WARNS on a renamed object key — the parse rejects it now', () => { + // `object` closed on the parse path (#4001), so a top-level unknown key is a + // hard error rather than a warning, and warning too would double-report. + // The rename itself did not get quieter, it got louder: the same suggestion + // now arrives as a rejection. + expect(lintUnknownAuthoringKeys(stackWith({ capabilities: { trackHistory: true } }, {}))) + .toEqual([]); + + const parsed = ObjectSchema.safeParse({ + name: 'crm_case', label: 'Case', fields: {}, capabilities: { trackHistory: true }, }); + expect(parsed.success).toBe(false); + const message = parsed.success ? '' : parsed.error.issues[0].message; + expect(message).toContain('`capabilities`'); + expect(message).toContain('`enable`'); }); it('reports across collections in one walk, with per-type surfaces', () => { diff --git a/packages/spec/src/kernel/metadata-authoring-lint.ts b/packages/spec/src/kernel/metadata-authoring-lint.ts index 2baa360a61..013932dc07 100644 --- a/packages/spec/src/kernel/metadata-authoring-lint.ts +++ b/packages/spec/src/kernel/metadata-authoring-lint.ts @@ -330,7 +330,16 @@ export function lintUnknownAuthoringKeys(rawStack: unknown): UnknownAuthoringKey const schema = getMetadataTypeSchema(type); if (!schema) continue; const posture = keyPosture(schema); - if (!posture || posture.mode !== 'strip' || posture.keys.size === 0) continue; + // NOT gated on the ROOT being `strip`. A strict root reports nothing at its + // own level — the parse rejects unknown keys there, so warning as well + // would double-report — but the shapes BELOW it are a separate question, + // and most are still strip. Gating the whole collection on the root's + // posture meant that closing a root silently switched off the warnings for + // everything under it: when `object` went strict on the parse path, its 71 + // nested strip-mode sites stopped reporting in the same change, with + // nothing anywhere to say so. Posture is a per-node property; this loop + // now treats it as one. + if (!posture || posture.keys.size === 0) continue; const guidance = GUIDANCE_BY_SURFACE[type] ?? EMPTY_GUIDANCE; for (let i = 0; i < items.length; i++) { @@ -338,7 +347,9 @@ export function lintUnknownAuthoringKeys(rawStack: unknown): UnknownAuthoringKey if (!isPlainRecord(item)) continue; const name = typeof item.name === 'string' && item.name ? item.name : String(i); const basePath = `${collection}.${name}`; - lintAuthoredRecordKeys(item, posture.keys, guidance, type, basePath, out); + if (posture.mode === 'strip') { + lintAuthoredRecordKeys(item, posture.keys, guidance, type, basePath, out); + } descend(schema, item, basePath, '', type, guidance, out, 0); } }