fix(metadata-protocol): read decorations stop round-tripping into persisted metadata bodies (#4326)

.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.

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';

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<string, unknown>): 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<string, unknown>) {
+    return `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`;
+}
+
+function makeStubEngine() {
+    const rows = new Map<string, Row>();
+    let nextId = 0;
+    const findRow = (w: Record<string, unknown>) => {
+        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<string, unknown> }) {
+            return findRow(opts.where)?.row ?? null;
+        },
+        async find(_t: string, opts: { where: Record<string, unknown> }) {
+            return Array.from(rows.values()).filter((r) => matches(r, opts.where));
+        },
+        async insert(table: string, data: Record<string, unknown>) {
+            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<string, unknown>, opts: { where: Record<string, unknown> }) {
+            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<T>(cb: (ctx: any) => Promise<T>): Promise<T> { 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<string, Row>, 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<string, unknown>;
+        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<string, unknown>;
+        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);
+    });
+});

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<string, unknown>;
+    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

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,