diff --git a/.changeset/7603-resource-edit-read-decoration-strip.md b/.changeset/7603-resource-edit-read-decoration-strip.md new file mode 100644 index 0000000000..f54483aad9 --- /dev/null +++ b/.changeset/7603-resource-edit-read-decoration-strip.md @@ -0,0 +1,40 @@ +--- +'@object-ui/app-shell': patch +--- + +Strip the framework's read-time decorations off a served draft before the metadata +editor's client Zod gate judges it (objectui#7603). + +**The symptom.** Opening an item that has a **pending draft** in the metadata admin +raised a false "this item is invalid" banner plus inline field errors — on a body the +server accepts, and on exactly the workflow where an author is mid-edit. Every wired +metadata type whose schema is `.strict()` was affected (14 of the 15); `sharing_rule` +was shielded only by accident, because `AUTHOR_SHAPE_ONLY_TYPES` switches its edit gate +off for an unrelated reason. + +**The cause.** `client.getDraft()` serves a **decorated** body: the strict draft branch +returns `item: decorateMetadataItem(type, …)`, which attaches `_diagnostics` whenever +the type has a registered Zod schema, and `_draft` on the preview-draft branch. +`ResourceEditPage` merged that body over the layered baseline and handed the result +straight to `validateMetadataDraft`. The layered half is clean — `getMetaItemLayered` +serves RAW layers — so the misfire required a pending draft to exist, which is why it +stayed invisible: an item with no draft passes today. + +`@objectstack/spec` names both keys a read-time decoration and states that a served body +"is therefore NOT a valid input to the schema that produced it until these are removed". +The server was right and the client was wrong. + +**The fix.** `extractDraftBody` — the one function that turns a served draft envelope +into a body, and the chokepoint all three merge sites go through (the load effect, the +post-save refresh, the post-publish refresh) — now passes the body through the spec's +own exported `stripReadDecorations`, the same helper `MetadataService.saveFields` +already uses on the write side. The strip runs after the presence verdict, so removing +our own decorations can never turn a served draft into "no draft". + +No schema was loosened and no key list is restated in this repo. The list is the spec's, +reached through its helper, because a local copy goes stale the next time the framework +adds a decoration — and a decoration this code does not know to remove is precisely the +defect. The ADR-0010 protection envelope (`_lock`, `_provenance`, …) is deliberately +**not** on that list: those keys are allowlisted by the closed schemas so provenance +survives a re-parse, and this strip leaves them alone. A genuinely undeclared key on the +same draft is still refused, by name. diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.readDecorationStrip.test.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.readDecorationStrip.test.tsx new file mode 100644 index 0000000000..b4c4bb9742 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.readDecorationStrip.test.tsx @@ -0,0 +1,263 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pins that the framework's READ DECORATIONS never reach the client edit gate + * through the pending-draft merge (objectui#7603). + * + * ## The defect this file exists to keep fixed + * + * `client.getDraft()` serves a DECORATED body: the strict draft branch returns + * `item: decorateMetadataItem(type, …)`, which attaches `_diagnostics` whenever + * the type has a registered Zod schema, and `_draft` on the preview-draft + * branch. The spec names both a READ-TIME decoration and says a served body + * "is therefore NOT a valid input to the schema that produced it until these + * are removed" — `METADATA_READ_DECORATIONS` in the spec kernel. + * + * `ResourceEditPage` merges that body over the layered baseline + * (`{ ...baseline, ...draftReal }`) and hands the result to + * `validateMetadataDraft`. The baseline half is clean — `getMetaItemLayered` + * serves RAW layers — so the decoration arrives ONLY when the item has a + * pending draft, which is why this stayed invisible: a no-draft case passes + * today and cannot catch it. Every wired metadata type whose schema is + * `.strict()` then reported a body THE SERVER ACCEPTS as `unrecognized_keys`, + * putting a false "this item is invalid" banner plus inline field errors on an + * author who is mid-edit. + * + * ## Why the fixture must carry a pending draft + * + * ⚠️ Drop the `getDraft` item from these cases and both of them still pass, + * while the defect is fully present. The pending draft IS the trigger. + * + * ## The two directions, and why the second one is not optional + * + * A strip is only correct if it removes the framework's own keys and NOTHING + * else. `unrecognizedGate` below feeds the same pending-draft path a body that + * carries the two decorations AND one genuinely undeclared key, and requires + * the gate to still refuse — naming the real key and neither decoration. That + * case is also this file's CONTROL: it proves the gate actually runs on the + * merged draft in this harness, so the first case's "no issues" is a measured + * verdict rather than a validator that never fired. + * + * ⛔ The cure is never to loosen a schema. The ADR-0010 envelope keys + * (`_lock`, `_provenance`, …) are allowlisted by the closed schemas ON PURPOSE + * so provenance survives a re-parse; the decorations are not, and a gate that + * tolerated them would destroy that distinction (AGENTS.md #0.1). + * + * ## Where the strip lives + * + * In `extractDraftBody` — the one function that turns a served draft envelope + * into a body, and the chokepoint all THREE merge sites go through (the load + * effect, the post-save refresh and the post-publish refresh). It reuses the + * spec's exported `stripReadDecorations`, the same helper + * `MetadataService.saveFields` already reaches for. ⛔ Never a second + * hand-maintained `['_diagnostics', '_draft']` in objectui: a local copy goes + * stale the next time the framework adds a decoration, and a decoration this + * code does not know to remove is precisely the defect above. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, cleanup, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +/** A key the bundled spec does not declare — a GENUINE author-side defect. */ +const UNDECLARED_KEY = 'zzUndeclaredKey'; + +/** Present only in the pending draft, so we can prove the merge ran. */ +const DRAFT_ONLY_LABEL = 'Home (draft in flight)'; + +/** The published body, exactly as the RAW layered read serves it: clean. */ +const PUBLISHED_PAGE = { + name: 'home', + label: 'Home', + type: 'home', + template: 'default', + regions: [{ name: 'main', components: [{ type: 'text', id: 'b1' }] }], +}; + +/** + * The framework's read-time decorations, spelled out because this fixture is + * standing in for a real served body. ⛔ Not imported from the spec: a fixture + * that derives its own input from the list under test would pass even if that + * list were emptied. The PRODUCTION strip is the thing that must read the + * spec, and it does. + */ +const READ_DECORATIONS = { + _diagnostics: { valid: true, errors: [], warnings: [] }, + _draft: true, +}; + +/** A served draft envelope: `{ type, name, item }` with the item decorated. */ +const draftEnvelope = (extra: Record = {}) => ({ + type: 'page', + name: 'home', + item: { ...PUBLISHED_PAGE, label: DRAFT_ONLY_LABEL, ...READ_DECORATIONS, ...extra }, +}); + +/** + * Records what the REAL gate was asked to judge and what it answered. + * + * The assertions read this rather than the banner on purpose: the banner is a + * rendering of the verdict, while this is the verdict — the same measurement + * the card reports (`body + _diagnostics -> REJECT unrecognized_keys`). + */ +const gate = vi.hoisted(() => ({ + calls: [] as Array<{ draft: Record; ok: boolean; issues: Array<{ path: string; message: string }> }>, +})); + +const mockClient = vi.hoisted(() => ({ + list: vi.fn(async () => []), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async () => ({ effective: PUBLISHED_PAGE, code: PUBLISHED_PAGE, editable: true })), + getDraft: vi.fn(async () => draftEnvelope()), + get: vi.fn(async () => null), + saveDraft: vi.fn(async () => ({})), +})); + +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ + entries: [ + { + type: 'page', + name: 'page', + label: 'Page', + allowOrgOverride: true, + // The most lenient live server there can be. It is handed to the + // cross-repo spec-skew cure, which only drops TOP-LEVEL absent + // required fields — it structurally cannot suppress an + // `unrecognized_keys` issue, whose path is empty. So no server hint + // can rescue a decorated body; only the strip can. + schema: { required: [] }, + }, + ], + }), + }; +}); + +/** + * The gate itself is REAL — this wrapper only records. Stubbing the verdict + * would leave the assertions measuring the stub. + */ +vi.mock('./clientValidation.js', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + validateMetadataDraft: async ( + ...args: Parameters + ): Promise>> => { + const res = await mod.validateMetadataDraft(...args); + gate.calls.push({ + draft: (args[1] ?? {}) as Record, + ok: res.ok, + issues: res.issues, + }); + return res; + }, + }; +}); + +import { MetadataResourceEditPage } from './ResourceEditPage'; +import { registerBuiltinInspectors } from './inspectors'; + +registerBuiltinInspectors(); + +beforeEach(() => { + gate.calls.length = 0; + mockClient.getDraft.mockImplementation(async () => draftEnvelope()); +}); + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +function openEditor() { + render( + + + , + ); +} + +/** + * The LAST verdict taken on the merged draft — the one the pending draft won. + * + * Index arithmetic rather than `Array.prototype.findLast`: this package's + * `type-check` compiles against a lib older than es2023, where that method does + * not exist on the type. It runs fine under vitest, so the failure would have + * been `tsc`-only — green tests, red CI. + */ +function lastMergedVerdict() { + const merged = gate.calls.filter((c) => c.draft.label === DRAFT_ONLY_LABEL); + return merged[merged.length - 1]; +} + +/** + * Wait for the gate to judge the MERGED draft. Anchoring on `DRAFT_ONLY_LABEL` + * is what makes every later assertion be about the pending-draft path: a + * verdict taken on the baseline-only draft would be clean for a reason that has + * nothing to do with this fix. + */ +async function awaitMergedVerdict() { + await waitFor( + () => { + expect(lastMergedVerdict(), 'the gate never judged the merged pending draft').toBeDefined(); + }, + { timeout: 4000 }, + ); + return lastMergedVerdict()!; +} + +const keysNamedIn = (issues: Array<{ message: string }>) => issues.map((i) => i.message).join(' | '); + +describe('MetadataResourceEditPage — read decorations never reach the edit gate (#7603)', () => { + it('accepts a served body whose pending draft carries `_diagnostics` and `_draft`', async () => { + openEditor(); + const verdict = await awaitMergedVerdict(); + + // The decorations were on the wire… + expect(await mockClient.getDraft.mock.results[0]!.value).toMatchObject({ + item: { _diagnostics: expect.anything(), _draft: true }, + }); + // …and did NOT survive into what the gate judged. + expect(Object.keys(verdict.draft)).not.toContain('_diagnostics'); + expect(Object.keys(verdict.draft)).not.toContain('_draft'); + + // The verdict itself: the body the server accepts, the client accepts. + expect(verdict.issues, keysNamedIn(verdict.issues)).toEqual([]); + expect(verdict.ok).toBe(true); + + // …and the author sees no "this item is invalid" banner. + expect(screen.queryByTestId('metadata-validation-banner')).not.toBeInTheDocument(); + + // The author's own content survived the strip untouched. + expect(verdict.draft.label).toBe(DRAFT_ONLY_LABEL); + expect(verdict.draft.regions).toEqual(PUBLISHED_PAGE.regions); + }); + + it('still refuses a genuinely undeclared key riding the same decorated draft', async () => { + mockClient.getDraft.mockImplementation(async () => draftEnvelope({ [UNDECLARED_KEY]: 1 })); + openEditor(); + const verdict = await awaitMergedVerdict(); + + // The strip is not a "drop whatever the schema refuses" pass: an author's + // real defect still fails, loudly, and by name. + expect(verdict.ok).toBe(false); + expect(keysNamedIn(verdict.issues)).toContain(UNDECLARED_KEY); + + // …and neither decoration is named alongside it — the strip took exactly + // the framework's own keys and left the author's. + expect(keysNamedIn(verdict.issues)).not.toContain('_diagnostics'); + expect(keysNamedIn(verdict.issues)).not.toContain('_draft'); + + // The banner is the CONTROL for the previous case: it fires here, in this + // same harness, on this same pending-draft path. + const banner = await screen.findByTestId('metadata-validation-banner', undefined, { timeout: 4000 }); + expect(banner).toHaveTextContent(UNDECLARED_KEY); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index 75680b79a9..d5c59d3a78 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -127,6 +127,7 @@ import { validateMetadataDraft, hasClientValidator, type DraftMode } from './cli import { describeIssuePath } from './issuePath.js'; import { buildCreateModeBody } from './createBody.js'; import { errorCodeIs, errorCodeIsAnyOf } from '@object-ui/types'; +import { stripReadDecorations } from '@objectstack/spec/kernel'; /** * ADR-0010 §3.6 lock state -> the lock banner's headline sentence. @@ -215,6 +216,39 @@ const CANVAS_OWNED_KEYS: Record = { * to using the envelope itself as the body — doing so would mis-identify the * "no draft" stub (which still has `type`/`name`/`label` keys) as a real * pending draft and would corrupt the editor baseline. + * + * ## The served body is DECORATED, and this is where that stops (objectui#7603) + * + * The strict draft branch returns `item: decorateMetadataItem(type, …)`, which + * attaches `_diagnostics` whenever the type has a registered Zod schema, and + * `_draft` on the preview-draft branch. The spec calls both a READ-TIME + * decoration and says a served body "is therefore NOT a valid input to the + * schema that produced it until these are removed". Every merge site below + * spreads this body over the layered baseline (`{ ...baseline, ...draftReal }`) + * and the result reaches the client Zod gate, so the decoration made 14 of the + * 15 wired types — every one whose schema is `.strict()` — report a body THE + * SERVER ACCEPTS as `unrecognized_keys`. The layered half is clean + * (`getMetaItemLayered` serves RAW layers), so the misfire needed a PENDING + * DRAFT to exist, which is why it stayed invisible. + * + * This function is the chokepoint: it is the one place a served draft envelope + * becomes a body, and all three merge sites (the load effect, the post-save + * refresh, the post-publish refresh) read it. Stripping here fixes them + * together and leaves no fourth site to forget. + * + * ⛔ Never by loosening a schema, and ⛔ never with a local + * `['_diagnostics', '_draft']`: the list is the SPEC'S, reached through its own + * exported helper — the same one `MetadataService.saveFields` uses on the write + * side — because a second hand-maintained copy goes stale the next time the + * framework adds a decoration, and a decoration this code does not know to + * remove is precisely the defect. The ADR-0010 protection envelope (`_lock`, + * `_provenance`, …) is deliberately NOT on that list: those keys are + * allowlisted by the closed schemas so provenance survives a re-parse, and this + * strip leaves them alone. + * + * The strip runs AFTER the presence verdict, never before it: what counts as a + * pending draft is `getDraft`'s answer, and removing our own decorations must + * not be able to turn a served draft into "no draft". */ function extractDraftBody( draftResp: unknown, @@ -224,9 +258,8 @@ function extractDraftBody( if (!('item' in env)) return null; const body = env.item; if (!body || typeof body !== 'object') return null; - return Object.keys(body as object).length > 0 - ? (body as Record) - : null; + if (Object.keys(body as object).length === 0) return null; + return stripReadDecorations(body) as Record; } /**