diff --git a/.changeset/view-union-identity-precondition.md b/.changeset/view-union-identity-precondition.md new file mode 100644 index 0000000000..af97797bfc --- /dev/null +++ b/.changeset/view-union-identity-precondition.md @@ -0,0 +1,96 @@ +--- +"@objectstack/spec": major +--- + +feat(spec)!: a `view` body must be a view before the union judges it (#5599) + +`ViewMetadataSchema` — the schema the `view` metadata type registers, and so the +one both `saveMetaItem`'s 422 gate and the read-time `_diagnostics` badge consult +— accepted **any object at all**. Measured on `origin/main`: + +``` +getMetadataTypeSchema('view').safeParse({ nope: 1 }) -> success, data = { type: 'simple' } +getMetadataTypeSchema('view').safeParse({}) -> success, data = { type: 'simple' } + +saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } }) + -> { success: true, state: 'active', seq: 1 } + persisted body = {"nope":1,"name":"garbage_view"} +``` + +The union's fourth member (`FormViewSchema.extend(…).strip()`) both strips +unknown keys **and** declares no required key — `type` even carries a `'simple'` +default — so it matched every object and handed the whole union a wildcard. The +`.strip()` is deliberate and load-bearing (#5074: it is what carries Studio's +round-trip keys); the defect is that an arm which strips *and* requires nothing +is a universal match. So `view` was the one common overlay type whose declared +write-path spec validation (ADR-0005 §Validation) could be bypassed outright — a +`declared ≠ enforced` gap at union **member selection**, one level above the +object schemas #4001 closed. + +Because `saveMetaItem` persists the *original* body rather than the parse output, +a wrong-shaped view — an AI-generated body in the wrong dialect, a hand-written +one with every key misspelled — did not fail loudly. It became an **active** view +overlay that renders nothing, and the read path then re-parsed it through the same +schema and badged it `_diagnostics.valid: true` (#5598), so Studio agreed it was +fine. + +**The fix.** A minimal identity precondition now runs ahead of all four arms: a +`view` body must carry at least one key some member declares, discounting the +keys the write path stamps onto every body itself (`name` always, plus +`viewKind`/`object`/`label` inherited from a shadowed registry entry — #2555). +The bar is *shape*, not completeness: `{ isPinned: true }` is not a renderable +view either, but it is unambiguously a view operation and still saves. No arm's +`.strip()` changed, and `/api/v1/meta/types/view` emits a byte-identical +`anyOf` of four in both the output and input directions, so Studio's SchemaForm +renders exactly as before. + +**Behaviour change** (why this is major — it is an enforcement close, not a new +capability): + +| `view` body | Before | After | +|:--|:--|:--| +| `{ nope: 1 }`, `{ id: 'x' }` — no recognized key | saved, stored **active** | **422** | +| `{}` | saved, stored active | **422** | +| identity only (`{ name }`, `{ name, object, viewKind, label }`) | saved | **422** | +| `{ isPinned: true }`, `{ hidden: true }`, `{ sortOrder: 3 }`, `{ order: 2 }` | saved | unchanged — saved | +| any container / ViewItem record / flattened overlay | as before | unchanged | +| a body mixing garbage **with** a real view key | stripped and saved | unchanged — still stripped and saved | + +That last row is the deliberate residue of the minimal fix: the precondition asks +"is this a view", never "is every key meaningful". Closing it means closing the +arms, which would break the round-trip capability #5074 exists to protect. + +**FROM → TO.** Existing projects whose stored views carry stray-key bodies will +start seeing 422 on the next save of those views. Reads are unaffected — nothing +is deleted or rewritten — but the same documents now badge `valid: false`, which +is how you find them. The platform ships a sweep endpoint for exactly this: + +```bash +curl -s "$OS_URL/api/v1/meta/diagnostics?type=view" -H "Authorization: Bearer $TOKEN" \ + | jq -r '.entries[] | "\(.name)\t\(.diagnostics.errors[0].message)"' +``` + +Each row names the view and why it is rejected. The fix is per row: give the body +a real view shape, or delete the overlay if it was never a view to begin with. + +```diff +- { "nope": 1, "name": "crm_lead.all" } ++ { "name": "crm_lead.all", "object": "crm_lead", "viewKind": "list", ++ "config": { "type": "grid", "columns": ["name"] } } +``` + +The rejection carries its own prescription rather than a rootless +`Invalid input` — it names the key classes a view may open with, separates keys +it does not recognize from identity keys it recognizes but discounts, and it is +one issue, not one plus four `invalid_union` branches. + +**New export.** `VIEW_WRITE_PATH_IDENTITY_KEYS` (`@objectstack/spec/ui`) — the +discounted set, exported so the producer side can be pinned against it. It is: +`normalizeViewMetadata` must never stamp a key absent from that set, or the key +silently becomes evidence again and re-opens this hole; a behavioural test in +`@objectstack/metadata-protocol` fails in the file that would introduce it. + +Direction A from the issue — giving the form arm a required floor — remains +deliberately **not** taken. It needs Studio's flattened round-trip bodies +measured first, or it 422s writes the platform itself makes; the ruling on #5599 +deferred it as a possible second tightening on top of this one. diff --git a/packages/metadata-protocol/src/metadata-diagnostics.union-issues.test.ts b/packages/metadata-protocol/src/metadata-diagnostics.union-issues.test.ts index 27131868d9..ddd48515b6 100644 --- a/packages/metadata-protocol/src/metadata-diagnostics.union-issues.test.ts +++ b/packages/metadata-protocol/src/metadata-diagnostics.union-issues.test.ts @@ -136,3 +136,71 @@ describe('#5598 the entries that never went through a union are unchanged', () = expect(computeMetadataDiagnostics('service', { name: 'whatever' })).toBeUndefined(); }); }); + +/** + * #5599 — the OTHER half of the same badge, closed in `packages/spec`. + * + * #5598 fixed a stored view whose defect collapsed to one rootless line. It could + * not touch the worse case one row over: a stored view that is not a view at all + * got `valid: true`. `ViewMetadataSchema`'s fourth union member both stripped + * unknown keys and required none, so `{ nope: 1 }` MATCHED, and the badge this + * module computes from that same schema said the document was fine. The two bugs + * are one mechanism seen from both ends — a union that explains its rejections + * badly, and a union that does not reject at all — which is why the ruling on + * #5599 asked for the disappearance of this false `valid: true` to be asserted + * from the READ path, not only from the schema's own unit tests. + */ +describe('#5599 a stored `view` that is not a view is no longer badged valid', () => { + it('`{ nope: 1 }` — the issue\'s headline document — is now `valid: false`', () => { + // On `origin/main` this returned exactly `{ valid: true }`. + const diag = computeMetadataDiagnostics('view', { nope: 1 }); + expect(diag?.valid).toBe(false); + expect(diag?.errors?.length).toBeGreaterThan(0); + }); + + it('…and the badge names WHY, so Studio has something to render', () => { + const diag = computeMetadataDiagnostics('view', { nope: 1 }); + expect(diag?.errors?.[0]?.message).toContain('no recognized `view` key'); + expect(diag?.errors?.[0]?.code).toBe('custom'); + }); + + it('an empty stored `view` body is `valid: false` too', () => { + expect(computeMetadataDiagnostics('view', {})?.valid).toBe(false); + }); + + it('reaches Studio through `decorateMetadataItem`, like every other verdict', () => { + const decorated = decorateMetadataItem('view', { nope: 1 }) as { + _diagnostics?: { valid: boolean }; + }; + expect(decorated._diagnostics?.valid).toBe(false); + }); + + it('read and save still agree — one ranking, applied to the new rejection', () => { + // The #5598 invariant, re-proved on the issue class #5599 introduces: + // a document must not be "valid to open, invalid to save" or vice versa. + const schema = getMetadataTypeSchema('view') as z.ZodTypeAny; + const parsed = schema.safeParse({ nope: 1 }); + expect(parsed.success).toBe(false); + const fromSharedRanking = zodIssuesToMetadataIssues( + (parsed as { error: { issues: unknown[] } }).error.issues, + ); + expect(computeMetadataDiagnostics('view', { nope: 1 })?.errors).toEqual(fromSharedRanking); + }); + + it('a legitimately-lean overlay is still valid — no collateral badge', () => { + // The precondition asks "is this a view at all", never "is it complete". + expect(computeMetadataDiagnostics('view', { isPinned: true })).toEqual({ valid: true }); + expect(computeMetadataDiagnostics('view', { hidden: true })).toEqual({ valid: true }); + }); + + it('a stored row of pure identity is no longer valid either', () => { + // The stored twin of the write-path case: `{ nope: 1 }` was persisted as + // `{ nope: 1, name: … }` (plus inherited identity where a registry entry + // existed), so every such row read back `valid: true`. Those rows are + // exactly the ones an operator now has to find — see the changeset. + expect(computeMetadataDiagnostics('view', { nope: 1, name: 'garbage_view' })?.valid).toBe(false); + expect(computeMetadataDiagnostics('view', { + nope: 1, name: 'showcase_task.default', viewKind: 'list', object: 'showcase_task', label: 'All Tasks', + })?.valid).toBe(false); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.lock-gate-fail-closed.test.ts b/packages/metadata-protocol/src/protocol.lock-gate-fail-closed.test.ts index 40c1b18dd2..73d1aa499f 100644 --- a/packages/metadata-protocol/src/protocol.lock-gate-fail-closed.test.ts +++ b/packages/metadata-protocol/src/protocol.lock-gate-fail-closed.test.ts @@ -185,8 +185,17 @@ function protocolFor(h: Harness) { return new ObjectStackProtocolImplementation(h.engine, undefined, 'env_1'); } +// [#5599] The body carries a real view key (`type` / `columns`), not identity +// alone. It used to be `{ name, label }`, which the `view` schema accepted only +// because its union had a member that stripped unknown keys and required none — +// the hole #5599 closed. Nothing here needs a contentless body: this file's +// subject is WHICH writes the lock gate admits, not what a view looks like. const save = (p: ObjectStackProtocolImplementation) => - p.saveMetaItem({ type: 'view', name: 'v1', item: { name: 'v1', label: 'Edited' } } as any); + p.saveMetaItem({ + type: 'view', + name: 'v1', + item: { name: 'v1', label: 'Edited', type: 'grid', columns: ['name'] }, + } as any); const remove = (p: ObjectStackProtocolImplementation) => p.deleteMetaItem({ type: 'view', name: 'v1' } as any); diff --git a/packages/metadata-protocol/src/view-write-path-identity.test.ts b/packages/metadata-protocol/src/view-write-path-identity.test.ts new file mode 100644 index 0000000000..6b1201b579 --- /dev/null +++ b/packages/metadata-protocol/src/view-write-path-identity.test.ts @@ -0,0 +1,81 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5599 — the producer half of the identity precondition, pinned where the + * producer lives. + * + * `ViewMetadataSchema`'s precondition asks "did the AUTHOR send something that + * is a view?". It cannot ask that directly, because `saveMetaItem` normalizes + * before it validates: by the time the schema sees the body, + * {@link normalizeViewMetadata} has already stamped keys onto it. The schema + * therefore discounts a fixed set — `VIEW_WRITE_PATH_IDENTITY_KEYS` — when + * judging evidence. + * + * That makes the two files a matched pair with no compiler link between them: + * the day this function learns to stamp a fifth key, that key silently becomes + * "evidence the author sent a view" over in `packages/spec`, and `{ nope: 1 }` + * starts passing the gate again — the exact defect #5599 closed, reopened by an + * edit that looks entirely reasonable and touches neither the schema nor this + * test's subject. + * + * So the pin is behavioural, not a copy of the list: it feeds the normalizer the + * emptiest possible body together with a maximal baseline, and asserts that + * everything it stamps is discounted. A new stamped key fails here, in the file + * that introduced it, with the remedy named. + */ +import { describe, expect, it } from 'vitest'; +import { VIEW_WRITE_PATH_IDENTITY_KEYS } from '@objectstack/spec/ui'; +import { getMetadataTypeSchema } from '@objectstack/spec/kernel'; +import { normalizeViewMetadata } from './protocol.js'; + +/** A registry entry carrying every identity field `viewIdentityPatch` inherits. */ +const baseline = { + name: 'showcase_task.default', + object: 'showcase_task', + viewKind: 'list', + label: 'All Tasks', + scope: 'package', + config: { type: 'grid', data: { provider: 'object', object: 'showcase_task' }, columns: ['title'] }, +}; + +describe('#5599 the write path stamps only keys the spec discounts as identity', () => { + it('every key stamped onto an empty body is in VIEW_WRITE_PATH_IDENTITY_KEYS', () => { + const stamped = normalizeViewMetadata('view', {}, 'showcase_task.default', baseline) as Record; + const unaccounted = Object.keys(stamped).filter((k) => !VIEW_WRITE_PATH_IDENTITY_KEYS.has(k)); + expect( + unaccounted, + 'normalizeViewMetadata stamped a key the #5599 identity precondition does not discount. ' + + 'That key now counts as evidence that the author sent a view, which re-opens #5599. ' + + 'Add it to VIEW_WRITE_PATH_IDENTITY_KEYS in packages/spec/src/ui/view.zod.ts.', + ).toEqual([]); + }); + + it('…and with no baseline it stamps only `name`', () => { + const stamped = normalizeViewMetadata('view', {}, 'adhoc.view', undefined) as Record; + expect(Object.keys(stamped)).toEqual(['name']); + expect(VIEW_WRITE_PATH_IDENTITY_KEYS.has('name')).toBe(true); + }); + + it('the normalized garbage body is REJECTED — the two halves compose', () => { + // This is the end-to-end statement of the fix, at the seam: the body the + // schema actually receives for the issue's headline input, in both the + // baseline and no-baseline cases. + const schema = getMetadataTypeSchema('view')!; + for (const withBaseline of [undefined, baseline]) { + const normalized = normalizeViewMetadata('view', { nope: 1 }, 'garbage_view', withBaseline); + expect(schema.safeParse(normalized).success).toBe(false); + } + }); + + it('…while a real personalization PUT survives the same seam', () => { + const schema = getMetadataTypeSchema('view')!; + const personalization = { + type: 'grid', + data: { provider: 'object', object: 'showcase_task' }, + columns: ['title'], + sort: [{ field: 'estimate_hours', order: 'desc' }], + }; + const normalized = normalizeViewMetadata('view', personalization, 'showcase_task.default', baseline); + expect(schema.safeParse(normalized).success).toBe(true); + }); +}); diff --git a/packages/objectql/src/protocol-view-identity-overlay.test.ts b/packages/objectql/src/protocol-view-identity-overlay.test.ts index 08d225fc89..1cc66c4793 100644 --- a/packages/objectql/src/protocol-view-identity-overlay.test.ts +++ b/packages/objectql/src/protocol-view-identity-overlay.test.ts @@ -231,4 +231,62 @@ describe('view overlay identity (#2555)', () => { expect(persisted.name).toBe('adhoc.view'); expect('viewKind' in persisted).toBe(false); }); + + // ── #5599 — the write path's spec gate was bypassable by ANY body ──────── + // + // #3095 (above) closed the case where a view's nested `config` was stripped + // to `{}`. #5599 is the case one level further out: the union's fourth + // member both `.strip()`s and requires nothing, so `{ nope: 1 }` MATCHED it, + // the gate reported success, and — because `saveMetaItem` persists the + // ORIGINAL body, not the parse output — `{"nope":1,"name":"garbage_view"}` + // landed in `sys_metadata` as an ACTIVE view. `view` was the one common + // overlay type whose declared spec validation (ADR-0005 §Validation) could + // be bypassed outright: Prime Directive #10's "declared ≠ enforced", at the + // union's member-selection layer rather than inside any member. + // + // Measured on `origin/main` before the fix, this exact call returned + // `{ success: true, state: 'active', seq: 1 }`. + it('#5599 write path REJECTS a body that is not a view at all (was: success + stored active)', async () => { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + await expect( + protocol.saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } }), + ).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 }); + // The half that made this a data bug rather than a validation nit: + // nothing may reach the store. + expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(false); + }); + + it('#5599 write path REJECTS an empty body, and stores nothing', async () => { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + await expect( + protocol.saveMetaItem({ type: 'view', name: 'empty_view', item: {} }), + ).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 }); + expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(false); + }); + + it('#5599 the 422 carries the prescription, not a rootless "Invalid input"', async () => { + const { engine } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + const failure = await protocol + .saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } }) + .then(() => null, (e: unknown) => e); + expect(failure).toBeTruthy(); + expect(JSON.stringify(failure)).toContain('no recognized `view` key'); + }); + + it('#5599 …while the personalization PUT this file exists for still saves', async () => { + // The regression this precondition must never cause: a 422 on a body the + // platform itself writes. `personalization` is the captured console PUT. + const { engine, rows } = makeStubEngine({ 'showcase_task.default': flattened }); + const protocol = new ObjectStackProtocolImplementation(engine); + const result = await protocol.saveMetaItem({ + type: 'view', + name: 'showcase_task.default', + item: { ...personalization }, + }); + expect(result.success).toBe(true); + expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(true); + }); }); diff --git a/packages/spec/api-surface/ui.json b/packages/spec/api-surface/ui.json index d9ec6b7997..4493788c56 100644 --- a/packages/spec/api-surface/ui.json +++ b/packages/spec/api-surface/ui.json @@ -338,6 +338,7 @@ "VIEW_CONSOLE_ROW_DECORATIONS (const)", "VIEW_FILTER_OPERATORS (const)", "VIEW_FILTER_OPERATOR_ALIASES (const)", + "VIEW_WRITE_PATH_IDENTITY_KEYS (const)", "View (type)", "ViewData (type)", "ViewDataParsed (type)", diff --git a/packages/spec/src/ui/view-authoring-wire-split.test.ts b/packages/spec/src/ui/view-authoring-wire-split.test.ts index 39e0fb4ac0..af725548b2 100644 --- a/packages/spec/src/ui/view-authoring-wire-split.test.ts +++ b/packages/spec/src/ui/view-authoring-wire-split.test.ts @@ -263,6 +263,32 @@ describe('#5074 — the wire door accepts what the platform itself writes', () = // nested under a member that is itself strict. accept(ViewMetadataSchema, body); }); + + // ── #5599: the identity precondition sits in FRONT of this whole door ──── + // It runs in the same `z.preprocess` stage as `stripViewConsoleDecorations`, + // so it is the first thing every body above meets. These pin that it is inert + // on the wire — the failure mode of a precondition is 422-ing our own writes. + it('#5599 — the precondition does not disturb the decoration strip it shares a stage with', () => { + // The strip runs only AFTER the identity check passes; a decorated body + // must still arrive at the union stripped, not rejected. + accept(ViewMetadataSchema, { ...OVERLAY_BASE, sort: [CONSOLE_SORT_ROW], filter: [CONSOLE_FILTER_ROW] }); + }); + + it('#5599 — a body carrying ONLY an aux round-trip key still reaches the union', () => { + // `updateView` PUTs `{ ...current, ...partial }`; when there is no stored + // item to merge, `current` is empty and the body is the bare partial. + // `isPinned`/`sortOrder` are declared on member 1, so they are vocabulary. + accept(ViewMetadataSchema, { isPinned: true }); + accept(ViewMetadataSchema, { sortOrder: 3 }); + }); + + it('#5599 — …but a body speaking NO view key is stopped before any member runs', () => { + const r = ViewMetadataSchema.safeParse({ nope: 1 }); + expect(r.success).toBe(false); + if (r.success) return; + expect(r.error.issues).toHaveLength(1); + expect(r.error.issues[0]!.code).toBe('custom'); + }); }); // =========================================================================== @@ -355,6 +381,28 @@ describe('#5074 — landmine 1: `/api/v1/meta/types/view` must still get an `any expect(json).toContain('"operator"'); expect(json).not.toContain('"console row key"'); }); + + it('#5599 — the identity precondition is invisible to the emitted contract', () => { + // This is WHY the check reports through the existing preprocess's `ctx` + // rather than as an extra pipe stage. `z.unknown().superRefine(…).pipe(union)` + // would satisfy the output-direction assertion above and silently degrade + // the INPUT direction to `{}` — Studio's SchemaForm would render nothing, + // and no `anyOf`-length pin would have caught it. Reporting through `ctx` + // changes no types, so both directions are unchanged from before #5599. + // (Verified out-of-band as byte-identical to `origin/main` for both + // directions; asserted here on the properties that make the form work.) + for (const io of ['output', 'input'] as const) { + const json = z.toJSONSchema(getMetadataTypeSchema('view')!, { unrepresentable: 'any', io }) as Record; + const members = json.anyOf as Array>; + expect(members).toHaveLength(4); + // Member 1 is the discriminated ViewItem record (a nested union); members + // 2-4 are plain objects. A degraded emission loses exactly this shape. + expect(Array.isArray(members[0]!.oneOf ?? members[0]!.anyOf)).toBe(true); + for (const member of members.slice(1)) expect(member.type).toBe('object'); + // The form needs real property sets, not an empty permissive schema. + expect(Object.keys((members[3]!.properties ?? {}) as object).length).toBeGreaterThan(10); + } + }); }); describe('#5074 — landmine 2: the lazySchema Proxy / ADR-0089 D3a trap', () => { diff --git a/packages/spec/src/ui/view-metadata-schema.test.ts b/packages/spec/src/ui/view-metadata-schema.test.ts index 3e53f70f35..7a71edca24 100644 --- a/packages/spec/src/ui/view-metadata-schema.test.ts +++ b/packages/spec/src/ui/view-metadata-schema.test.ts @@ -92,11 +92,16 @@ describe('ViewMetadataSchema — genuine validation across the three runtime sha expect(ViewMetadataSchema.safeParse({ listViews: {} }).success).toBe(false); }); - it('accepts a bare `{}` (legacy-compatible — the old ViewSchema also accepted it)', () => { - // Not a regression: a truly empty body carries no viewKind/object, so - // every consumer that filters on identity drops it. Pinned so the lenient - // flattened-overlay branch behaviour is intentional, not accidental. - expect(ViewMetadataSchema.safeParse({}).success).toBe(true); + it('REJECTS a bare `{}` — the pin this line used to make, reversed by #5599', () => { + // This assertion previously read `.toBe(true)`, justified as "legacy- + // compatible … a truly empty body carries no viewKind/object, so every + // consumer that filters on identity drops it". #5599 measured what that + // reasoning missed: `saveMetaItem` does NOT drop it — it persists the + // ORIGINAL body and reports success, so `{}` (and `{ nope: 1 }`, which + // reached the same lenient member) landed as an ACTIVE view overlay that + // renders nothing, badged `valid: true` on read. The lenient branch was + // intentional; accepting bodies that are not views was the accident. + expect(ViewMetadataSchema.safeParse({}).success).toBe(false); }); }); @@ -191,6 +196,135 @@ describe('ViewMetadataSchema — genuine validation across the three runtime sha }); }); + // ── #5599: the identity precondition, ahead of all four arms ────────────── + describe('identity precondition (#5599)', () => { + // The reproduction from the issue, verbatim. On `origin/main` every input + // in this block was ACCEPTED and reduced to `{ type: 'simple' }` — member 4 + // (`FormViewSchema.extend(…).strip()`) both strips unknown keys and requires + // none, so it matched any object at all and handed the union a wildcard. + it('REJECTS `{ nope: 1 }` — the issue\'s headline input', () => { + expect(ViewMetadataSchema.safeParse({ nope: 1 }).success).toBe(false); + }); + + it('REJECTS a body of purely unrecognized keys, however many', () => { + expect(ViewMetadataSchema.safeParse({ nope: 1, alsoNope: 'x', deeply: { wrong: true } }).success).toBe(false); + }); + + it('REJECTS a body whose keys are ALL misspellings of real ones', () => { + // The AI-authored / hand-typo case the issue calls out: a whole body + // written in the wrong dialect used to land silently as an empty view. + expect(ViewMetadataSchema.safeParse({ colums: ['name'], viewType: 'grid' }).success).toBe(false); + }); + + it('REJECTS a top-level `id` — never declared on any member (批 18 Q1)', () => { + expect(ViewMetadataSchema.safeParse({ id: 'abc' }).success).toBe(false); + }); + + it('names the failure instead of reporting a rootless `invalid_union`', () => { + const r = ViewMetadataSchema.safeParse({ nope: 1 }); + expect(r.success).toBe(false); + if (r.success) return; + // ONE issue, not one + four union branches: `z.NEVER` aborts the pipe. + expect(r.error.issues).toHaveLength(1); + expect(r.error.issues[0]!.code).toBe('custom'); + expect(r.error.issues[0]!.message).toContain('no recognized `view` key'); + // The prescription travels with the rejection (AGENTS.md post-task §3). + expect(r.error.issues[0]!.message).toContain('listViews'); + expect(r.error.issues[0]!.message).toContain('`nope`'); + }); + + it('fails CLOSED, not open — it does not reject everything', () => { + // Guards against the mirror-image defect: a precondition that rejects the + // platform's own writes is strictly worse than the hole it closed. + expect(ViewMetadataSchema.safeParse({ type: 'simple' }).success).toBe(true); + }); + + // Every shape the platform itself writes carries a declared key, so the + // precondition is inert on all of them. These are the acceptance inputs the + // #5074 trace established for `updateView`'s `{ ...current, ...partial }`. + it.each([ + ['a pin PUT with no stored item to merge', { isPinned: true }], + ['a switcher-reorder PUT', { sortOrder: 3 }], + ['a column-only overlay', { columns: ['name'] }], + ['a filter-only overlay', { filter: [{ field: 'name', operator: 'contains', value: 'x' }] }], + ['a hide PUT', { hidden: true }], + ['an order-only overlay', { order: 2 }], + ['a renamed view that still carries its config', { label: 'New name', columns: ['name'] }], + ])('leaves %s alone', (_label, body) => { + expect(ViewMetadataSchema.safeParse(body).success).toBe(true); + }); + + // ── identity is not shape ──────────────────────────────────────────────── + // The subtraction that makes the precondition bite on the WRITE path. + // `saveMetaItem` normalizes before it validates: `normalizeViewMetadata` + // stamps `name` onto every view body, and `viewIdentityPatch` inherits + // `viewKind`/`object`/`label` from the shadowed registry entry (#2555). So + // `{ nope: 1 }` reaches this schema as `{ nope: 1, name: 'garbage_view' }` + // — the exact body #5599 reports as PERSISTED. Counting those keys as + // evidence would have made the whole precondition a no-op where it matters. + it.each([ + ['the stamped name alone', { name: 'garbage_view' }], + ['garbage plus the stamped name (the write path\'s real input)', { nope: 1, name: 'garbage_view' }], + ['garbage plus FULL inherited identity (baseline present)', { + nope: 1, name: 'showcase_task.default', viewKind: 'list', object: 'showcase_task', label: 'All Tasks', + }], + ['identity with no content at all', { viewKind: 'list', object: 'crm_lead', label: 'Leads' }], + ])('REJECTS %s', (_label, body) => { + expect(ViewMetadataSchema.safeParse(body).success).toBe(false); + }); + + it('says WHY an identity-only body is rejected, in its own words', () => { + const r = ViewMetadataSchema.safeParse({ name: 'v', object: 'o', label: 'L' }); + expect(r.success).toBe(false); + if (r.success) return; + expect(r.error.issues[0]!.message).toContain('only identity fields'); + expect(r.error.issues[0]!.message).toContain('the write path stamps them itself'); + }); + + it('reports the two halves separately — `name` is discounted, not "unrecognized"', () => { + // The write path's real input. Calling `name` unrecognized would send an + // author to fix a key that is perfectly valid and merely not evidence. + const r = ViewMetadataSchema.safeParse({ nope: 1, name: 'garbage_view' }); + expect(r.success).toBe(false); + if (r.success) return; + const message = r.error.issues[0]!.message; + expect(message).toContain('unrecognized key `nope`'); + expect(message).toContain('only identity fields (`name`)'); + }); + + it('…but identity PLUS any real view key is fine — leanness is not the bar', () => { + expect(ViewMetadataSchema.safeParse({ name: 'v', object: 'o', hidden: true }).success).toBe(true); + expect(ViewMetadataSchema.safeParse({ name: 'v', viewKind: 'list', isPinned: true }).success).toBe(true); + }); + + it('leaves non-objects to the union — it judges objects only', () => { + // Unchanged from origin/main: these were already rejected, as invalid_union. + for (const body of ['a string', 42, null, undefined, []]) { + const r = ViewMetadataSchema.safeParse(body); + expect(r.success).toBe(false); + if (!r.success) expect(r.error.issues[0]!.code).toBe('invalid_union'); + } + }); + + it('derives its vocabulary from the members, so a new arm key is admitted automatically', () => { + // Not a hand-written list: every top-level key any member declares counts. + // `splitSize` is a FormView key nobody would think to allow-list by hand. + expect(ViewMetadataSchema.safeParse({ splitSize: 30 }).success).toBe(true); + // …and a key that exists only NESTED (inside `config`) is not top-level + // vocabulary, so it cannot smuggle a garbage body through. + expect(ViewMetadataSchema.safeParse({ groupByField: 'stage' }).success).toBe(false); + }); + + it('does NOT close the arms — `.strip()` round-tripping is untouched (#5074)', () => { + // The ruling on #5599 kept every arm's `.strip()`: a body that speaks the + // vocabulary still carries undeclared aux keys through without a 422. + // This is the deliberate residue of the minimal fix, pinned so a later + // batch cannot mistake it for an oversight. + const r = ViewMetadataSchema.safeParse({ isPinned: true, someFutureStudioKey: 'x' }); + expect(r.success).toBe(true); + }); + }); + // ── JSON Schema emission (/api/v1/meta/types/view) ──────────────────────── it('converts to a JSON Schema anyOf (union → anyOf) without throwing', () => { const json = z.toJSONSchema(ViewMetadataSchema, { unrepresentable: 'any' }) as Record; diff --git a/packages/spec/src/ui/view.zod.ts b/packages/spec/src/ui/view.zod.ts index 7c625e4865..8e301f5d8a 100644 --- a/packages/spec/src/ui/view.zod.ts +++ b/packages/spec/src/ui/view.zod.ts @@ -2270,6 +2270,21 @@ export function defineViewItem(config: z.input): ViewItem // recursive-effective and what let `ViewFilterRuleSchema` / `ListView.sort` // close for authoring. Anyone closing a member here must re-run that trace, // not re-read this comment. +// +// [#5599] …and the cost of that opening, before the identity precondition +// below existed: member 4 both `.strip()`s AND declares no required key +// (`FormViewSchema.type` even carries a `'simple'` default), so it matched +// ANY object. `{ nope: 1 }` did not merely pass — it passed as a *view*, +// reduced to `{ type: 'simple' }`, and `saveMetaItem` (which persists the +// ORIGINAL body, not the parse output) wrote `{"nope":1,"name":"…"}` into +// `sys_metadata` as an ACTIVE view overlay. The read path then re-parsed the +// same body through the same schema and badged it `_diagnostics.valid: true` +// (#5598), so Studio agreed. `view` was the one common overlay type whose +// declared write-path spec gate (ADR-0005 §Validation) could be bypassed by +// an arbitrary body — Prime Directive #10's "declared ≠ enforced", one layer +// above the object schemas #4001 closed: at union-MEMBER SELECTION, not at +// any single member. The fix is a precondition, NOT a strictness flip on the +// members — see {@link viewIdentityVocabulary}. /** * Optional identity + structural-guard fields layered onto the two "flattened @@ -2318,6 +2333,143 @@ function containerHasAView(v: unknown): boolean { ); } +/** + * [#5599] The keys the WRITE PATH itself puts on a `view` body, which therefore + * cannot be evidence that the author sent a view. + * + * `saveMetaItem` normalizes before it validates. `normalizeViewMetadata` + * (`@objectstack/metadata-protocol`) stamps `name` onto **every** view body at + * the single write chokepoint — it exists precisely to guarantee one — and, when + * the overlay shadows a registry entry, `viewIdentityPatch` inherits `viewKind`, + * `object` and `label` from it (#2555). A key present on 100% of the bodies + * reaching the gate has zero discriminating power: counting `name` as evidence + * would leave this precondition unable to reject anything on the write path at + * all — which is the one path #5599 is about, since `{ nope: 1 }` arrives at the + * schema as `{ nope: 1, name: 'garbage_view' }`, exactly the body the issue + * reports as persisted. + * + * So the bar is *shape*, not identity — which is how the ruling worded it too. + * A body may be very lean, but it must say something about what the view IS; a + * body of these keys alone says only which view it is attached to. + * + * Exported so the producer side can be pinned against it: `normalizeViewMetadata` + * must never stamp a key that is not listed here, or that key silently becomes + * evidence again and re-opens this hole. That pin lives with the producer, in + * `metadata-protocol`'s `view-write-path-identity.test.ts`. + */ +export const VIEW_WRITE_PATH_IDENTITY_KEYS: ReadonlySet = new Set([ + 'name', + 'viewKind', + 'object', + 'label', +]); + +/** + * [#5599] Every top-level key ANY member of {@link ViewMetadataSchema} declares, + * MINUS {@link VIEW_WRITE_PATH_IDENTITY_KEYS} — the vocabulary the identity + * precondition judges "is this a `view` at all?" against. + * + * **Derived from the members, never hand-listed.** A literal array here would be + * a second declaration of the union's own surface, and the two would drift on + * the first member that grows a key — the precondition would then reject a body + * the union accepts, which is a 422 on a shape the platform itself writes. So it + * walks the members' Zod defs instead: objects contribute their `shape` keys, + * unions recurse into their options (member 1 is a discriminated union of two + * arms), and wrappers (`.refine()`, `.strip()`, pipes) are unwrapped. Adding a + * key to any arm widens this set in the same edit, with no bookkeeping. + * + * Computed once, on first parse rather than at schema-construction time: the + * members are {@link lazySchema} proxies whose factories run on first `_zod` + * touch, and deriving eagerly would force every view schema in the file to + * materialise as a side effect of this module loading (ADR-0089 D3a). + */ +function collectDeclaredTopLevelKeys(schema: unknown, into: Set, depth = 0): void { + const def = (schema as { _zod?: { def?: Record } } | undefined)?._zod?.def; + if (!def || depth > 6) return; + if (def.type === 'object') { + for (const key of Object.keys((def.shape ?? {}) as object)) into.add(key); + return; + } + if (def.type === 'union') { + for (const option of (def.options ?? []) as unknown[]) collectDeclaredTopLevelKeys(option, into, depth + 1); + return; + } + // `.refine()` keeps the object def; these cover pipes / wrappers defensively. + for (const hop of ['innerType', 'in', 'out', 'schema'] as const) { + if (def[hop]) collectDeclaredTopLevelKeys(def[hop], into, depth + 1); + } +} + +/** + * [#5599] The minimal identity precondition, run BEFORE the four-arm union. + * + * A `view` body must speak the `view` vocabulary — carry at least one key some + * member declares that the write path did not stamp itself + * ({@link VIEW_WRITE_PATH_IDENTITY_KEYS}). That is the whole bar, deliberately: + * it rejects bodies that are **not a view at all** (`{ nope: 1 }`, `{}`, + * `{ id: 'x' }`, and identity with no content) while making no judgement about + * whether the view is *complete* — which is what keeps it compatible with every + * lean shape the platform round-trips. A pin PUT (`{ isPinned: true }`), a hide + * PUT (`{ hidden: true }`), a reorder (`{ sortOrder: 3 }`) and a column-sort PUT + * all carry declared non-identity keys and are unaffected. + * + * "Complete" is deliberately NOT the bar, and the distinction is the whole + * reason this is safe: `{ isPinned: true }` is not a renderable view either, but + * it is unambiguously a *view operation*, and 422-ing the platform's own writes + * would be a worse defect than the one being fixed. + * + * **Why a precondition and not a required floor on member 4.** Giving the form + * arm a required key (the other candidate fix) would 422 the flattened overlays + * Studio writes, whose required-ness nobody has measured; the ruling on #5599 + * took this route and deferred that one explicitly. This route also leaves every + * arm's `.strip()` untouched — the round-trip capability #5074 traced and + * documented is load-bearing and is not what was broken. What was broken is that + * an arm which strips AND requires nothing is a wildcard for the whole union, so + * the fix belongs one level up, at the union's door. + * + * **Why it lives in the existing `z.preprocess` stage.** `/api/v1/meta/types/view` + * serves `z.toJSONSchema()` of this schema to Studio's SchemaForm, and both pins + * (`view-metadata-schema.test.ts`, `view-authoring-wire-split.test.ts`) require a + * four-member `anyOf` in the OUTPUT *and* INPUT directions. Adding a stage to the + * pipe (`z.unknown().superRefine(…).pipe(union)`) satisfies the output direction + * and silently degrades the input one to `{}` — the form would render nothing. + * Reporting through the preprocess's own `ctx` changes no types, so the emitted + * `anyOf` is byte-identical. It also short-circuits: `z.NEVER` aborts the pipe, + * so a rejected body yields ONE named issue instead of that issue plus four + * `invalid_union` branches for arms that were never the point. + */ +function assertViewIdentity(body: unknown, ctx: z.RefinementCtx, vocabulary: Set): boolean { + // Non-objects and arrays are left to the union, which already rejects them — + // this precondition answers "which object is not a view", nothing else. + if (typeof body !== 'object' || body === null || Array.isArray(body)) return true; + const keys = Object.keys(body as Record); + if (keys.some((key) => vocabulary.has(key))) return true; + // Report the two halves separately. Lumping them together would call `name` + // "unrecognized", which is false and would send an author to fix the wrong + // key: `name` is declared, it is just discounted as evidence. + const identity = keys.filter((key) => VIEW_WRITE_PATH_IDENTITY_KEYS.has(key)); + const unknown = keys.filter((key) => !VIEW_WRITE_PATH_IDENTITY_KEYS.has(key)); + const quote = (list: string[], cap = 5) => + `${list.slice(0, cap).map((k) => `\`${k}\``).join(', ')}${list.length > cap ? ', …' : ''}`; + ctx.addIssue({ + code: 'custom', + path: [], + message: + 'Not a `view` body: no recognized `view` key. Expected at least one of a container slot ' + + '(`list` / `form` / `listViews` / `formViews`), a ViewItem record (`viewKind` + `config`), ' + + 'or an inline view config (`type`, `columns`, `sections`, `filter`, …). ' + + (keys.length === 0 ? 'Received `{}`.' : 'Received ') + + (unknown.length > 0 ? `unrecognized ${unknown.length === 1 ? 'key' : 'keys'} ${quote(unknown)}` : '') + + (unknown.length > 0 && identity.length > 0 ? ', and ' : '') + + (identity.length > 0 + ? `only identity fields (${quote(identity)}) — these say which view this attaches to, ` + + 'not what the view is, and the write path stamps them itself' + : '') + + (keys.length === 0 ? '' : '.'), + }); + return false; +} + /** * Canonical schema for ANY persisted `view` metadata body — the schema the * `view` type registers in `metadata-type-schemas.ts`. A union over the three @@ -2336,9 +2488,14 @@ function containerHasAView(v: unknown): boolean { * {@link stripViewConsoleDecorations}. It runs once, ahead of every member, so * the openness this union needs reaches nested blocks that a member-level * `.strip()` can never reach. + * + * [#5599] That same stage now also carries the identity precondition — see + * {@link assertViewIdentity} for why the check has to live here rather than as + * an extra pipe stage, and why it is a precondition rather than a required floor + * on member 4. */ -export const ViewMetadataSchema = lazySchema(() => - z.preprocess(stripViewConsoleDecorations, z.union([ +export const ViewMetadataSchema = lazySchema(() => { + const members = [ // 1. Standalone ViewItem record — nested config validated genuinely, and // the WIRE variant, so Studio's round-trip keys have a declared home. ViewItemWireSchema, @@ -2361,8 +2518,28 @@ export const ViewMetadataSchema = lazySchema(() => // schema must strip back, or an upstream field addition becomes a crash. ListViewSchema.extend(flattenedViewOverlayFields()).strip(), FormViewSchema.extend(flattenedViewOverlayFields()).strip(), - ])), -); + ] as const; + + // [#5599] Derived once, on first parse — see `collectDeclaredTopLevelKeys`. + let vocabulary: Set | undefined; + const viewVocabulary = (): Set => { + if (!vocabulary) { + vocabulary = new Set(); + for (const member of members) collectDeclaredTopLevelKeys(member, vocabulary); + // Identity the write path supplies is not evidence of shape — see + // `VIEW_WRITE_PATH_IDENTITY_KEYS` for why this subtraction is the + // difference between closing #5599 and only appearing to. + for (const key of VIEW_WRITE_PATH_IDENTITY_KEYS) vocabulary.delete(key); + } + return vocabulary; + }; + + return z.preprocess( + (body, ctx) => + assertViewIdentity(body, ctx, viewVocabulary()) ? stripViewConsoleDecorations(body) : z.NEVER, + z.union(members as unknown as readonly [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]), + ); +}); // ─────────────────────────────────────────────────────────────────────────── // defineView container → ViewItem expansion (shared by every loader)