Skip to content

Commit e8f435c

Browse files
qq9340100claude
andauthored
feat(spec)!: reject view bodies that are not views, before the union (#5599) (#6280)
`ViewMetadataSchema` accepted any object at all. Its fourth union 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. Measured on origin/main: getMetadataTypeSchema('view').safeParse({ nope: 1 }) -> 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"} `saveMetaItem` persists the ORIGINAL body, so a wrong-shaped view became an active overlay that renders nothing, and the read path re-parsed it through the same schema and badged it `_diagnostics.valid: true` (#5598). `view` was the one common overlay type whose declared write-path validation (ADR-0005 §Validation) could be bypassed outright — declared != enforced at union MEMBER SELECTION, one level above the object schemas #4001 closed. Direction B of the ruling: a minimal identity precondition ahead of all four arms. A body must carry at least one key some member declares, discounting the keys the write path stamps itself (`name` always; `viewKind`/`object`/ `label` inherited from a shadowed registry entry, #2555) — those are present on every body reaching the gate and so carry no evidence. The bar is shape, not completeness: `{ isPinned: true }` still saves. No arm's `.strip()` changed — the #5074 round-trip capability is untouched, and direction A (a required floor on member 4) stays deferred. The check reports through the existing `z.preprocess` stage's `ctx` rather than as an extra pipe stage. An added stage satisfies the output-direction `anyOf` pin and silently degrades the INPUT direction to `{}`, which would leave Studio's SchemaForm rendering nothing. Verified byte-identical `/api/v1/meta/types/view` emission in both directions. - vocabulary derived from the members, never hand-listed, so a new arm key widens it in the same edit - `VIEW_WRITE_PATH_IDENTITY_KEYS` exported so the producer side is pinned: `normalizeViewMetadata` may not stamp a key outside it without going red - `{}` pin in view-metadata-schema.test.ts reversed, with the reasoning it used to carry and why the measurement contradicted it Claude-Session: https://claude.ai/code/session_01Gn96jhbN7TGSPzjBDsZfXx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 59b794f commit e8f435c

9 files changed

Lines changed: 682 additions & 10 deletions
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
---
2+
"@objectstack/spec": major
3+
---
4+
5+
feat(spec)!: a `view` body must be a view before the union judges it (#5599)
6+
7+
`ViewMetadataSchema` — the schema the `view` metadata type registers, and so the
8+
one both `saveMetaItem`'s 422 gate and the read-time `_diagnostics` badge consult
9+
— accepted **any object at all**. Measured on `origin/main`:
10+
11+
```
12+
getMetadataTypeSchema('view').safeParse({ nope: 1 }) -> success, data = { type: 'simple' }
13+
getMetadataTypeSchema('view').safeParse({}) -> success, data = { type: 'simple' }
14+
15+
saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } })
16+
-> { success: true, state: 'active', seq: 1 }
17+
persisted body = {"nope":1,"name":"garbage_view"}
18+
```
19+
20+
The union's fourth member (`FormViewSchema.extend(…).strip()`) both strips
21+
unknown keys **and** declares no required key — `type` even carries a `'simple'`
22+
default — so it matched every object and handed the whole union a wildcard. The
23+
`.strip()` is deliberate and load-bearing (#5074: it is what carries Studio's
24+
round-trip keys); the defect is that an arm which strips *and* requires nothing
25+
is a universal match. So `view` was the one common overlay type whose declared
26+
write-path spec validation (ADR-0005 §Validation) could be bypassed outright — a
27+
`declared ≠ enforced` gap at union **member selection**, one level above the
28+
object schemas #4001 closed.
29+
30+
Because `saveMetaItem` persists the *original* body rather than the parse output,
31+
a wrong-shaped view — an AI-generated body in the wrong dialect, a hand-written
32+
one with every key misspelled — did not fail loudly. It became an **active** view
33+
overlay that renders nothing, and the read path then re-parsed it through the same
34+
schema and badged it `_diagnostics.valid: true` (#5598), so Studio agreed it was
35+
fine.
36+
37+
**The fix.** A minimal identity precondition now runs ahead of all four arms: a
38+
`view` body must carry at least one key some member declares, discounting the
39+
keys the write path stamps onto every body itself (`name` always, plus
40+
`viewKind`/`object`/`label` inherited from a shadowed registry entry — #2555).
41+
The bar is *shape*, not completeness: `{ isPinned: true }` is not a renderable
42+
view either, but it is unambiguously a view operation and still saves. No arm's
43+
`.strip()` changed, and `/api/v1/meta/types/view` emits a byte-identical
44+
`anyOf` of four in both the output and input directions, so Studio's SchemaForm
45+
renders exactly as before.
46+
47+
**Behaviour change** (why this is major — it is an enforcement close, not a new
48+
capability):
49+
50+
| `view` body | Before | After |
51+
|:--|:--|:--|
52+
| `{ nope: 1 }`, `{ id: 'x' }` — no recognized key | saved, stored **active** | **422** |
53+
| `{}` | saved, stored active | **422** |
54+
| identity only (`{ name }`, `{ name, object, viewKind, label }`) | saved | **422** |
55+
| `{ isPinned: true }`, `{ hidden: true }`, `{ sortOrder: 3 }`, `{ order: 2 }` | saved | unchanged — saved |
56+
| any container / ViewItem record / flattened overlay | as before | unchanged |
57+
| a body mixing garbage **with** a real view key | stripped and saved | unchanged — still stripped and saved |
58+
59+
That last row is the deliberate residue of the minimal fix: the precondition asks
60+
"is this a view", never "is every key meaningful". Closing it means closing the
61+
arms, which would break the round-trip capability #5074 exists to protect.
62+
63+
**FROM → TO.** Existing projects whose stored views carry stray-key bodies will
64+
start seeing 422 on the next save of those views. Reads are unaffected — nothing
65+
is deleted or rewritten — but the same documents now badge `valid: false`, which
66+
is how you find them. The platform ships a sweep endpoint for exactly this:
67+
68+
```bash
69+
curl -s "$OS_URL/api/v1/meta/diagnostics?type=view" -H "Authorization: Bearer $TOKEN" \
70+
| jq -r '.entries[] | "\(.name)\t\(.diagnostics.errors[0].message)"'
71+
```
72+
73+
Each row names the view and why it is rejected. The fix is per row: give the body
74+
a real view shape, or delete the overlay if it was never a view to begin with.
75+
76+
```diff
77+
- { "nope": 1, "name": "crm_lead.all" }
78+
+ { "name": "crm_lead.all", "object": "crm_lead", "viewKind": "list",
79+
+ "config": { "type": "grid", "columns": ["name"] } }
80+
```
81+
82+
The rejection carries its own prescription rather than a rootless
83+
`Invalid input` — it names the key classes a view may open with, separates keys
84+
it does not recognize from identity keys it recognizes but discounts, and it is
85+
one issue, not one plus four `invalid_union` branches.
86+
87+
**New export.** `VIEW_WRITE_PATH_IDENTITY_KEYS` (`@objectstack/spec/ui`) — the
88+
discounted set, exported so the producer side can be pinned against it. It is:
89+
`normalizeViewMetadata` must never stamp a key absent from that set, or the key
90+
silently becomes evidence again and re-opens this hole; a behavioural test in
91+
`@objectstack/metadata-protocol` fails in the file that would introduce it.
92+
93+
Direction A from the issue — giving the form arm a required floor — remains
94+
deliberately **not** taken. It needs Studio's flattened round-trip bodies
95+
measured first, or it 422s writes the platform itself makes; the ruling on #5599
96+
deferred it as a possible second tightening on top of this one.

packages/metadata-protocol/src/metadata-diagnostics.union-issues.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,3 +136,71 @@ describe('#5598 the entries that never went through a union are unchanged', () =
136136
expect(computeMetadataDiagnostics('service', { name: 'whatever' })).toBeUndefined();
137137
});
138138
});
139+
140+
/**
141+
* #5599 — the OTHER half of the same badge, closed in `packages/spec`.
142+
*
143+
* #5598 fixed a stored view whose defect collapsed to one rootless line. It could
144+
* not touch the worse case one row over: a stored view that is not a view at all
145+
* got `valid: true`. `ViewMetadataSchema`'s fourth union member both stripped
146+
* unknown keys and required none, so `{ nope: 1 }` MATCHED, and the badge this
147+
* module computes from that same schema said the document was fine. The two bugs
148+
* are one mechanism seen from both ends — a union that explains its rejections
149+
* badly, and a union that does not reject at all — which is why the ruling on
150+
* #5599 asked for the disappearance of this false `valid: true` to be asserted
151+
* from the READ path, not only from the schema's own unit tests.
152+
*/
153+
describe('#5599 a stored `view` that is not a view is no longer badged valid', () => {
154+
it('`{ nope: 1 }` — the issue\'s headline document — is now `valid: false`', () => {
155+
// On `origin/main` this returned exactly `{ valid: true }`.
156+
const diag = computeMetadataDiagnostics('view', { nope: 1 });
157+
expect(diag?.valid).toBe(false);
158+
expect(diag?.errors?.length).toBeGreaterThan(0);
159+
});
160+
161+
it('…and the badge names WHY, so Studio has something to render', () => {
162+
const diag = computeMetadataDiagnostics('view', { nope: 1 });
163+
expect(diag?.errors?.[0]?.message).toContain('no recognized `view` key');
164+
expect(diag?.errors?.[0]?.code).toBe('custom');
165+
});
166+
167+
it('an empty stored `view` body is `valid: false` too', () => {
168+
expect(computeMetadataDiagnostics('view', {})?.valid).toBe(false);
169+
});
170+
171+
it('reaches Studio through `decorateMetadataItem`, like every other verdict', () => {
172+
const decorated = decorateMetadataItem('view', { nope: 1 }) as {
173+
_diagnostics?: { valid: boolean };
174+
};
175+
expect(decorated._diagnostics?.valid).toBe(false);
176+
});
177+
178+
it('read and save still agree — one ranking, applied to the new rejection', () => {
179+
// The #5598 invariant, re-proved on the issue class #5599 introduces:
180+
// a document must not be "valid to open, invalid to save" or vice versa.
181+
const schema = getMetadataTypeSchema('view') as z.ZodTypeAny;
182+
const parsed = schema.safeParse({ nope: 1 });
183+
expect(parsed.success).toBe(false);
184+
const fromSharedRanking = zodIssuesToMetadataIssues(
185+
(parsed as { error: { issues: unknown[] } }).error.issues,
186+
);
187+
expect(computeMetadataDiagnostics('view', { nope: 1 })?.errors).toEqual(fromSharedRanking);
188+
});
189+
190+
it('a legitimately-lean overlay is still valid — no collateral badge', () => {
191+
// The precondition asks "is this a view at all", never "is it complete".
192+
expect(computeMetadataDiagnostics('view', { isPinned: true })).toEqual({ valid: true });
193+
expect(computeMetadataDiagnostics('view', { hidden: true })).toEqual({ valid: true });
194+
});
195+
196+
it('a stored row of pure identity is no longer valid either', () => {
197+
// The stored twin of the write-path case: `{ nope: 1 }` was persisted as
198+
// `{ nope: 1, name: … }` (plus inherited identity where a registry entry
199+
// existed), so every such row read back `valid: true`. Those rows are
200+
// exactly the ones an operator now has to find — see the changeset.
201+
expect(computeMetadataDiagnostics('view', { nope: 1, name: 'garbage_view' })?.valid).toBe(false);
202+
expect(computeMetadataDiagnostics('view', {
203+
nope: 1, name: 'showcase_task.default', viewKind: 'list', object: 'showcase_task', label: 'All Tasks',
204+
})?.valid).toBe(false);
205+
});
206+
});

packages/metadata-protocol/src/protocol.lock-gate-fail-closed.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,17 @@ function protocolFor(h: Harness) {
185185
return new ObjectStackProtocolImplementation(h.engine, undefined, 'env_1');
186186
}
187187

188+
// [#5599] The body carries a real view key (`type` / `columns`), not identity
189+
// alone. It used to be `{ name, label }`, which the `view` schema accepted only
190+
// because its union had a member that stripped unknown keys and required none —
191+
// the hole #5599 closed. Nothing here needs a contentless body: this file's
192+
// subject is WHICH writes the lock gate admits, not what a view looks like.
188193
const save = (p: ObjectStackProtocolImplementation) =>
189-
p.saveMetaItem({ type: 'view', name: 'v1', item: { name: 'v1', label: 'Edited' } } as any);
194+
p.saveMetaItem({
195+
type: 'view',
196+
name: 'v1',
197+
item: { name: 'v1', label: 'Edited', type: 'grid', columns: ['name'] },
198+
} as any);
190199

191200
const remove = (p: ObjectStackProtocolImplementation) =>
192201
p.deleteMetaItem({ type: 'view', name: 'v1' } as any);
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #5599 — the producer half of the identity precondition, pinned where the
5+
* producer lives.
6+
*
7+
* `ViewMetadataSchema`'s precondition asks "did the AUTHOR send something that
8+
* is a view?". It cannot ask that directly, because `saveMetaItem` normalizes
9+
* before it validates: by the time the schema sees the body,
10+
* {@link normalizeViewMetadata} has already stamped keys onto it. The schema
11+
* therefore discounts a fixed set — `VIEW_WRITE_PATH_IDENTITY_KEYS` — when
12+
* judging evidence.
13+
*
14+
* That makes the two files a matched pair with no compiler link between them:
15+
* the day this function learns to stamp a fifth key, that key silently becomes
16+
* "evidence the author sent a view" over in `packages/spec`, and `{ nope: 1 }`
17+
* starts passing the gate again — the exact defect #5599 closed, reopened by an
18+
* edit that looks entirely reasonable and touches neither the schema nor this
19+
* test's subject.
20+
*
21+
* So the pin is behavioural, not a copy of the list: it feeds the normalizer the
22+
* emptiest possible body together with a maximal baseline, and asserts that
23+
* everything it stamps is discounted. A new stamped key fails here, in the file
24+
* that introduced it, with the remedy named.
25+
*/
26+
import { describe, expect, it } from 'vitest';
27+
import { VIEW_WRITE_PATH_IDENTITY_KEYS } from '@objectstack/spec/ui';
28+
import { getMetadataTypeSchema } from '@objectstack/spec/kernel';
29+
import { normalizeViewMetadata } from './protocol.js';
30+
31+
/** A registry entry carrying every identity field `viewIdentityPatch` inherits. */
32+
const baseline = {
33+
name: 'showcase_task.default',
34+
object: 'showcase_task',
35+
viewKind: 'list',
36+
label: 'All Tasks',
37+
scope: 'package',
38+
config: { type: 'grid', data: { provider: 'object', object: 'showcase_task' }, columns: ['title'] },
39+
};
40+
41+
describe('#5599 the write path stamps only keys the spec discounts as identity', () => {
42+
it('every key stamped onto an empty body is in VIEW_WRITE_PATH_IDENTITY_KEYS', () => {
43+
const stamped = normalizeViewMetadata('view', {}, 'showcase_task.default', baseline) as Record<string, unknown>;
44+
const unaccounted = Object.keys(stamped).filter((k) => !VIEW_WRITE_PATH_IDENTITY_KEYS.has(k));
45+
expect(
46+
unaccounted,
47+
'normalizeViewMetadata stamped a key the #5599 identity precondition does not discount. '
48+
+ 'That key now counts as evidence that the author sent a view, which re-opens #5599. '
49+
+ 'Add it to VIEW_WRITE_PATH_IDENTITY_KEYS in packages/spec/src/ui/view.zod.ts.',
50+
).toEqual([]);
51+
});
52+
53+
it('…and with no baseline it stamps only `name`', () => {
54+
const stamped = normalizeViewMetadata('view', {}, 'adhoc.view', undefined) as Record<string, unknown>;
55+
expect(Object.keys(stamped)).toEqual(['name']);
56+
expect(VIEW_WRITE_PATH_IDENTITY_KEYS.has('name')).toBe(true);
57+
});
58+
59+
it('the normalized garbage body is REJECTED — the two halves compose', () => {
60+
// This is the end-to-end statement of the fix, at the seam: the body the
61+
// schema actually receives for the issue's headline input, in both the
62+
// baseline and no-baseline cases.
63+
const schema = getMetadataTypeSchema('view')!;
64+
for (const withBaseline of [undefined, baseline]) {
65+
const normalized = normalizeViewMetadata('view', { nope: 1 }, 'garbage_view', withBaseline);
66+
expect(schema.safeParse(normalized).success).toBe(false);
67+
}
68+
});
69+
70+
it('…while a real personalization PUT survives the same seam', () => {
71+
const schema = getMetadataTypeSchema('view')!;
72+
const personalization = {
73+
type: 'grid',
74+
data: { provider: 'object', object: 'showcase_task' },
75+
columns: ['title'],
76+
sort: [{ field: 'estimate_hours', order: 'desc' }],
77+
};
78+
const normalized = normalizeViewMetadata('view', personalization, 'showcase_task.default', baseline);
79+
expect(schema.safeParse(normalized).success).toBe(true);
80+
});
81+
});

packages/objectql/src/protocol-view-identity-overlay.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,4 +231,62 @@ describe('view overlay identity (#2555)', () => {
231231
expect(persisted.name).toBe('adhoc.view');
232232
expect('viewKind' in persisted).toBe(false);
233233
});
234+
235+
// ── #5599 — the write path's spec gate was bypassable by ANY body ────────
236+
//
237+
// #3095 (above) closed the case where a view's nested `config` was stripped
238+
// to `{}`. #5599 is the case one level further out: the union's fourth
239+
// member both `.strip()`s and requires nothing, so `{ nope: 1 }` MATCHED it,
240+
// the gate reported success, and — because `saveMetaItem` persists the
241+
// ORIGINAL body, not the parse output — `{"nope":1,"name":"garbage_view"}`
242+
// landed in `sys_metadata` as an ACTIVE view. `view` was the one common
243+
// overlay type whose declared spec validation (ADR-0005 §Validation) could
244+
// be bypassed outright: Prime Directive #10's "declared ≠ enforced", at the
245+
// union's member-selection layer rather than inside any member.
246+
//
247+
// Measured on `origin/main` before the fix, this exact call returned
248+
// `{ success: true, state: 'active', seq: 1 }`.
249+
it('#5599 write path REJECTS a body that is not a view at all (was: success + stored active)', async () => {
250+
const { engine, rows } = makeStubEngine();
251+
const protocol = new ObjectStackProtocolImplementation(engine);
252+
await expect(
253+
protocol.saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } }),
254+
).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 });
255+
// The half that made this a data bug rather than a validation nit:
256+
// nothing may reach the store.
257+
expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(false);
258+
});
259+
260+
it('#5599 write path REJECTS an empty body, and stores nothing', async () => {
261+
const { engine, rows } = makeStubEngine();
262+
const protocol = new ObjectStackProtocolImplementation(engine);
263+
await expect(
264+
protocol.saveMetaItem({ type: 'view', name: 'empty_view', item: {} }),
265+
).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 });
266+
expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(false);
267+
});
268+
269+
it('#5599 the 422 carries the prescription, not a rootless "Invalid input"', async () => {
270+
const { engine } = makeStubEngine();
271+
const protocol = new ObjectStackProtocolImplementation(engine);
272+
const failure = await protocol
273+
.saveMetaItem({ type: 'view', name: 'garbage_view', item: { nope: 1 } })
274+
.then(() => null, (e: unknown) => e);
275+
expect(failure).toBeTruthy();
276+
expect(JSON.stringify(failure)).toContain('no recognized `view` key');
277+
});
278+
279+
it('#5599 …while the personalization PUT this file exists for still saves', async () => {
280+
// The regression this precondition must never cause: a 422 on a body the
281+
// platform itself writes. `personalization` is the captured console PUT.
282+
const { engine, rows } = makeStubEngine({ 'showcase_task.default': flattened });
283+
const protocol = new ObjectStackProtocolImplementation(engine);
284+
const result = await protocol.saveMetaItem({
285+
type: 'view',
286+
name: 'showcase_task.default',
287+
item: { ...personalization },
288+
});
289+
expect(result.success).toBe(true);
290+
expect(Array.from(rows.values()).some((r) => r.type === 'view')).toBe(true);
291+
});
234292
});

packages/spec/api-surface/ui.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@
338338
"VIEW_CONSOLE_ROW_DECORATIONS (const)",
339339
"VIEW_FILTER_OPERATORS (const)",
340340
"VIEW_FILTER_OPERATOR_ALIASES (const)",
341+
"VIEW_WRITE_PATH_IDENTITY_KEYS (const)",
341342
"View (type)",
342343
"ViewData (type)",
343344
"ViewDataParsed (type)",

0 commit comments

Comments
 (0)