diff --git a/.changeset/strip-read-decorations-on-save.md b/.changeset/strip-read-decorations-on-save.md new file mode 100644 index 0000000000..f4cd9c6012 --- /dev/null +++ b/.changeset/strip-read-decorations-on-save.md @@ -0,0 +1,33 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): read decorations stop round-tripping into persisted metadata bodies (#4326) + +`getMetaItem` / `getMetaItems` decorate every served document with +`_diagnostics` (and `_draft` on preview reads), while the write path persists +the request body **verbatim** by design (ADR-0005 §Validation — `parsed.data` +would strip Studio-only auxiliary fields). Nothing stripped the decorations in +between, so the standard designer round-trip — GET the served document, edit a +field, PUT the whole body back — baked a stale read-time verdict into +`sys_metadata.metadata`, into its checksum, and into every history diff. + +It was never user-visible: reads recompute `_diagnostics` and the fresh verdict +shadows the persisted one. What it corrupted was the stored bytes — a +decoration-only re-save moved the content checksum, and history diffs carried +diagnostic noise no author wrote. + +`saveMetaItem` now strips `_diagnostics` and `_draft` from the body before the +destructive-change diff, the schema gate, the authoring gate, and persistence +(new `stripReadDecorations`, exported for tests). A **silent** strip, unlike the +neighbouring layered-envelope rejection: those keys are our own decoration +riding on a document that is otherwise exactly what the author edited, so +rejecting the round-trip would be hostile. The ADR-0010 protection envelope +(`_lock`, `_lockReason`, `_provenance`) and `_packageId` are deliberately left +alone — envelope state the write path legitimately carries, not read decoration. + +Also documents the #3903 conversion boundary on `SysMetadataRepository.get`: +its body stays verbatim because every caller wants the bytes a hash was +computed over (parent-version lineage, existence probes) or is diffing against +equally-verbatim history rows — conversion belongs one layer up, at the +protocol's serving seams. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 4d4c1b5e4c..e7e28d01bb 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -1,6 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeViewMetadata, graftNormalizedOperators } from './protocol.js'; +export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeViewMetadata, graftNormalizedOperators, stripReadDecorations } from './protocol.js'; export { createMetadataProtocolPlugin, assembleMetadataProtocol } from './plugin.js'; export type { MetadataProtocolPluginOptions } from './plugin.js'; export type { UninstallCleanup, UninstallCleanupOutcome } from './protocol.js'; diff --git a/packages/metadata-protocol/src/protocol.read-decorations.test.ts b/packages/metadata-protocol/src/protocol.read-decorations.test.ts new file mode 100644 index 0000000000..31150aafe2 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.read-decorations.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4326 — read decorations must not round-trip into the persisted body. + * + * `getMetaItem`/`getMetaItems` stamp `_diagnostics` on every served document + * (and `_draft` on draft reads), while the write path persists the request body + * VERBATIM by design (ADR-0005 §Validation — `parsed.data` would strip + * Studio-only auxiliary fields). The standard Studio round-trip therefore used + * to bake a read-time verdict into `sys_metadata.metadata`, into its checksum, + * and into every history diff. + * + * Nothing user-visible was ever wrong — reads recompute and the fresh verdict + * shadows the stale one — which is exactly why this needs a pin: the invariant + * being protected is "a GET → PUT round-trip persists a byte-identical body", + * and only the stored bytes can show it. + */ +import { describe, expect, it } from 'vitest'; +import { ObjectStackProtocolImplementation, stripReadDecorations } from './index.js'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; + state: string; + metadata: string; + checksum?: string; + version?: number; +} + +function matches(r: Row, where: Record): boolean { + for (const [k, v] of Object.entries(where)) { + if (v === undefined) continue; + if ((r as any)[k] !== v) return false; + } + return true; +} + +function keyOf(w: Record) { + return `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`; +} + +function makeStubEngine() { + const rows = new Map(); + let nextId = 0; + const findRow = (w: Record) => { + for (const [k, r] of rows) if (matches(r, w)) return { key: k, row: r }; + return null; + }; + const engine: any = { + async findOne(_t: string, opts: { where: Record }) { + return findRow(opts.where)?.row ?? null; + }, + async find(_t: string, opts: { where: Record }) { + return Array.from(rows.values()).filter((r) => matches(r, opts.where)); + }, + async insert(table: string, data: Record) { + if (table !== 'sys_metadata') return { id: 'side_table' }; + const row = { id: `r_${++nextId}`, ...(data as any) } as Row; + rows.set(keyOf(data), row); + return { id: row.id }; + }, + async update(table: string, data: Record, opts: { where: Record }) { + if (table !== 'sys_metadata') return { id: null }; + const found = findRow(opts.where); + if (!found) return { id: null }; + const merged = { ...found.row, ...(data as any) }; + rows.delete(found.key); + rows.set(keyOf(merged), merged); + return { id: found.row.id }; + }, + async delete() { return { deleted: 0 }; }, + async transaction(cb: (ctx: any) => Promise): Promise { return cb(undefined); }, + async syncObjectSchema() { /* no DDL in this stub */ }, + registry: { + listItems: () => [], + isPackageDisabled: () => false, + getItem: () => undefined, + registerItem: () => {}, + registerObject: () => {}, + getPackage: () => undefined, + }, + }; + return { engine, rows }; +} + +const storedBody = (rows: Map, name: string) => { + const row = Array.from(rows.values()).find((r) => r.name === name)!; + return JSON.parse(row.metadata); +}; + +const objectBody = (name: string) => ({ + name, + label: 'Invoice', + fields: { amount: { type: 'currency', label: 'Amount' } }, +}); + +describe('stripReadDecorations (#4326)', () => { + it('removes the read-only decorations', () => { + const out = stripReadDecorations({ + name: 'x', _diagnostics: { valid: true }, _draft: true, + }) as Record; + expect(out).toEqual({ name: 'x' }); + }); + + it('returns the SAME reference when there is nothing to strip', () => { + const body = { name: 'x', label: 'X' }; + expect(stripReadDecorations(body)).toBe(body); + }); + + it('leaves the ADR-0010 protection envelope and package provenance alone', () => { + // These share the underscore spelling but are envelope state the write + // path legitimately carries — stripping them would loosen a packaged lock. + const body = { + name: 'x', + _lock: 'full', + _lockReason: 'shipped by package', + _provenance: 'package', + _packageId: 'app.crm', + _diagnostics: { valid: false }, + }; + const out = stripReadDecorations(body) as Record; + expect(out).toEqual({ + name: 'x', + _lock: 'full', + _lockReason: 'shipped by package', + _provenance: 'package', + _packageId: 'app.crm', + }); + }); + + it('passes non-object input through untouched', () => { + expect(stripReadDecorations(null)).toBe(null); + expect(stripReadDecorations('str')).toBe('str'); + const arr = [{ _diagnostics: {} }]; + expect(stripReadDecorations(arr)).toBe(arr); + }); + + it('does not mutate the caller input', () => { + const body = { name: 'x', _diagnostics: { valid: true } }; + stripReadDecorations(body); + expect(body._diagnostics).toEqual({ valid: true }); + }); +}); + +describe('saveMetaItem — the Studio round-trip persists a byte-identical body (#4326)', () => { + it('GET → PUT the whole served document stores no _diagnostics', async () => { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + + const authored = objectBody('crm_invoice'); + await protocol.saveMetaItem({ type: 'object', name: 'crm_invoice', item: authored }); + const firstStored = storedBody(rows, 'crm_invoice'); + + // What Studio actually holds: the SERVED document, decorations included. + const served: any = (await protocol.getMetaItem({ type: 'object', name: 'crm_invoice' })).item; + expect(served._diagnostics).toBeDefined(); // precondition — the read does decorate + + // Edit one label and PUT the whole thing back, as the designer does. + await protocol.saveMetaItem({ + type: 'object', + name: 'crm_invoice', + item: { ...served, label: 'Invoice (edited)' }, + }); + + const stored = storedBody(rows, 'crm_invoice'); + expect('_diagnostics' in stored).toBe(false); + expect(stored.label).toBe('Invoice (edited)'); + // Everything except the edited key is byte-identical to the first save. + expect({ ...stored, label: firstStored.label }).toEqual(firstStored); + }); + + it('a draft read PUT back does not persist the _draft preview badge', async () => { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + + await protocol.saveMetaItem({ + type: 'object', name: 'crm_quote', item: objectBody('crm_quote'), mode: 'draft', + }); + // The badge comes from the PREVIEW read (`previewDrafts`), which is what + // the designer's "preview pending changes" surface calls — a plain + // `state: 'draft'` read returns the body unbadged. + const draft: any = (await protocol.getMetaItem({ + type: 'object', name: 'crm_quote', previewDrafts: true, + })).item; + expect(draft._draft).toBe(true); // precondition — the preview read badges + + await protocol.saveMetaItem({ + type: 'object', name: 'crm_quote', item: draft, mode: 'draft', + }); + + const stored = storedBody(rows, 'crm_quote'); + expect('_draft' in stored).toBe(false); + expect('_diagnostics' in stored).toBe(false); + // Draft-ness lives in the row's state column, not the body. + expect(Array.from(rows.values()).find((r) => r.name === 'crm_quote')!.state).toBe('draft'); + }); + + it('keeps the checksum stable across a decoration-only round-trip', async () => { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine); + + await protocol.saveMetaItem({ type: 'object', name: 'crm_lead', item: objectBody('crm_lead') }); + const before = Array.from(rows.values()).find((r) => r.name === 'crm_lead')!.checksum; + + // Re-save the served document unchanged — the only difference is our own + // decoration, so the content hash must not move. + const served: any = (await protocol.getMetaItem({ type: 'object', name: 'crm_lead' })).item; + await protocol.saveMetaItem({ type: 'object', name: 'crm_lead', item: served }); + + const after = Array.from(rows.values()).find((r) => r.name === 'crm_lead')!.checksum; + expect(after).toBe(before); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 74a36aa611..a368fcee6e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -356,6 +356,50 @@ function describeMalformedFilter(filter: unknown[]): string { return `${JSON.stringify(filter)} is not a recognised filter shape.`; } +/** + * Keys the READ path stamps onto a served metadata document, which therefore + * must never survive back into a persisted body (#4326). + * + * Both are recomputed on every read, so persisting them stores a stale copy of + * something the reader already replaces: + * - `_diagnostics` — the spec-validation verdict `decorateMetadataItem` + * spreads onto every `getMetaItem`/`getMetaItems` result. A second producer + * stamps the same key: view-container expansion records a name-collision + * rename warning (`stampRenameWarning`, spec/ui/view.zod.ts). Both are + * derived from the document, so neither belongs inside it; + * - `_draft` — the preview badge added by draft reads. Draft-ness lives in + * the row's `state` column and the `mode` parameter, never in the body. + * + * Deliberately NOT stripped, though they share the underscore spelling: the + * ADR-0010 protection envelope (`_lock`, `_lockReason`, `_provenance`) and + * `_packageId`. Those are envelope state the write path legitimately carries + * and merges (see `mergeArtifactProtection`) — not read-time decoration. + */ +const READ_ONLY_DECORATIONS = ['_diagnostics', '_draft'] as const; + +/** + * Remove {@link READ_ONLY_DECORATIONS} from an about-to-persist body. + * + * A **silent** strip, unlike the layered-envelope rejection in `saveMetaItem`: + * that envelope is a wrong document the caller must fix, whereas these keys are + * our own decoration riding along on a document that is otherwise exactly what + * the author edited. Rejecting the standard GET → edit → PUT round-trip would + * be hostile; stripping restores the invariant that a round-trip persists the + * body byte-identical. + * + * Returns the SAME reference when there is nothing to strip, so the common path + * allocates nothing (the discipline {@link graftNormalizedOperators} follows). + * Non-object inputs pass through — the caller's own validation owns those. + */ +export function stripReadDecorations(item: unknown): unknown { + if (!item || typeof item !== 'object' || Array.isArray(item)) return item; + const dict = item as Record; + if (!READ_ONLY_DECORATIONS.some((k) => k in dict)) return item; + const next = { ...dict }; + for (const k of READ_ONLY_DECORATIONS) delete next[k]; + return next; +} + /** * Guarantee a `view` body carries a top-level `name`. * @@ -5312,6 +5356,15 @@ export class ObjectStackProtocolImplementation implements if (!request.item) { throw new Error('Item data is required'); } + // Drop OUR OWN read decorations before anything reads the body (#4326). + // The write path persists verbatim by design (ADR-0005 §Validation), so + // the standard Studio round-trip — GET (decorated) → edit → PUT the whole + // body — would otherwise bake a read-time verdict into the row, its + // checksum, and every history diff. See {@link stripReadDecorations} for + // why this is a silent strip and which underscore keys are NOT touched. + // Placed first so the destructive-change diff, the schema gate, the + // authoring gate and the persisted body all see the same document. + request.item = stripReadDecorations(request.item); // Per-item lifecycle (ADR-0005 §"Drafts"). Default is `'publish'` // (legacy semantics — save goes straight live) to keep callers // that predate the draft/publish split working. Studio's diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index 73594e7da3..2c1ab47210 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -249,6 +249,22 @@ export class SysMetadataRepository implements MetadataRepository { * `opts.state` selects which lifecycle row to read: defaults to the * live published row (`'active'`). Pass `'draft'` to read the pending * unpublished revision (if any). + * + * **The body is VERBATIM — no ADR-0087 conversion (#3903 boundary).** This + * repository is the version layer, and every caller wants the bytes that + * were written, not today's canonical rendering of them: + * - `saveMetaItem` / `revertCommit` / `deleteMetaItem` read only `hash` + * (parent-version lineage and existence probes) — converting would + * change the body a hash was computed over and break the pairing; + * - `diffMeta` compares this body against `sys_metadata_history` bodies, + * which are verbatim by design. Converting one side only would render + * the conversion itself as a user-authored change in the diff. + * Metadata that is *served to a consumer* is converted one layer up, at the + * `ObjectStackProtocolImplementation` read seams. The lone exception worth + * knowing: the seed body captured for `publishPackageDrafts` flows from here + * into `applySeedBodies`, which IS a serving path — vacuous today because no + * conversion targets the seed/dataset surface, but the seam to wire if one + * ever lands. */ async get( ref: MetaRef,