From dfb3fda64c65b16345752556ee574c480cd0fdbd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 02:25:52 +0000 Subject: [PATCH] fix(metadata-protocol): saveMetaItem canonicalizes flow bodies on write (#4542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Studio edit healed every legacy row except a flow's: reads serve stored flows verbatim (the ADR-0078 conflict guard needs the live executor registry), and FlowNodeSchema.config is an open z.record, so the served legacy dialect sailed back through the schema gate and re-persisted verbatim — the row stayed `pending` in `os migrate meta --stored` no matter how many times an author edited it. saveMetaItem now runs resolveFlowCanonicalizer (#4498) on flow bodies before the schema gate and persists `storable` (never the parsed shape — schema defaults stay excluded, ADR-0087). A refused node-type rename fails the save with 409 FLOW_CONVERSION_CONFLICT naming the token; a body the stricter canonicalizer cannot parse (cycles, regions) falls back to the raw save in draft and publish mode alike, so WIP drafts stay saveable; with no automation service reachable the save behaves exactly as before. Copy-on-write keeps migrateStoredMetadata / duplicatePackage re-entry free. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NDmJ5ASMSzcw1q17vh32MG --- .../save-meta-item-flow-canonicalization.md | 41 +++ ...0087-metadata-protocol-upgrade-contract.md | 26 ++ ...stored-flow-resolution.integration.test.ts | 64 ++++ ...rotocol.save-flow-canonicalization.test.ts | 294 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 57 ++++ 5 files changed, 482 insertions(+) create mode 100644 .changeset/save-meta-item-flow-canonicalization.md create mode 100644 packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts diff --git a/.changeset/save-meta-item-flow-canonicalization.md b/.changeset/save-meta-item-flow-canonicalization.md new file mode 100644 index 0000000000..184d510aa5 --- /dev/null +++ b/.changeset/save-meta-item-flow-canonicalization.md @@ -0,0 +1,41 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): `saveMetaItem` canonicalizes flow bodies on write — a Studio edit now heals a legacy flow row like every other type's (#4542) + +The once-per-boot stored-conversion warning promises that re-saving a row +("Studio edit → save") persists the canonical shape. That held for every type +except `flow`: the read path serves stored flows verbatim (the ADR-0078 +open-namespace conflict guard needs the engine's live executor registry, so +`convertStoredItem` skips them), and `FlowNodeSchema.config` is an open +`z.record`, so the legacy dialect an author was served (`config.filters`, pre-17 +node aliases) sailed back through `saveMetaItem`'s schema gate and re-persisted +verbatim. A flow row stayed `pending` in `os migrate meta --stored` no matter +how many times an author edited it — only the migration itself could retire it. + +`saveMetaItem` now runs the #4498 resolver (`resolveFlowCanonicalizer`) on flow +bodies **before** the schema gate and persists `storable` — conversions plus the +derived condition envelopes, deliberately not the schema's defaults (ADR-0087). +The pass is copy-on-write, so already-canonical bodies (including the ones +`migrateStoredMetadata` and `duplicatePackage` hand in) are untouched. + +Failure postures, same as the duplication seam: + +- **A refused node-type rename** (the old token is a live name owned by a custom + executor here) refuses the save with `409 FLOW_CONVERSION_CONFLICT`, naming + the token and path — never a silent legacy persist. 409 rather than 422 + because the body may be perfectly valid: the refusal comes from environment + state, so resubmitting the same body cannot help. +- **A body the canonicalizer cannot parse** falls back to the raw save and + today's schema gate — in draft AND publish mode. `canonicalizeStoredFlow` is + stricter than the gate (cycle detection, control-flow regions), and a + work-in-progress draft with a temporary cycle must not become unsaveable; + `registerFlow` still refuses to arm a malformed flow either way. +- **No automation service reachable** (a control-plane or metadata-only host): + the save behaves exactly as before — a host must not start refusing flow + writes it accepted yesterday. `os migrate meta --stored` reports what it + could not canonicalize. + +Reads are still unchanged — served bodies keep the stored dialect ("reads +diagnose, never drop"); the heal happens on the way back in. diff --git a/docs/adr/0087-metadata-protocol-upgrade-contract.md b/docs/adr/0087-metadata-protocol-upgrade-contract.md index ad1bbf46ab..39d7eea314 100644 --- a/docs/adr/0087-metadata-protocol-upgrade-contract.md +++ b/docs/adr/0087-metadata-protocol-upgrade-contract.md @@ -532,3 +532,29 @@ report still saying protocol N until the next run. The premise is restored rather than restated: the stored pass shrinks because every write path now canonicalizes, not because the sentence says so. + +## Addendum (2026-08-02) — the save seam itself (#4542) + +"Every write path now canonicalizes" above was still one short. `duplicatePackage` +was the *platform* producer; the ordinary Studio/REST save was a producer by +round-trip: reads serve stored flows verbatim (deliberately — see 2026-07-31), +`FlowNodeSchema.config` is an open `z.record`, so an author served the legacy +dialect who edited a label and saved re-persisted that dialect — and the row +stayed `pending` in the stored report no matter how many times it was edited. +That contradicted the boot warning's own remediation text ("re-save it (Studio +edit → save …) to persist the canonical shape"), which held for every type +except the one it never fires for. + +`saveMetaItem` now runs `resolveFlowCanonicalizer` on flow bodies before its +schema gate and persists `storable`, with the same postures as the duplication +seam: a refused rename fails the save loudly (`409 FLOW_CONVERSION_CONFLICT`, +naming the token — the refusal comes from environment state, so it is not a 422 +the author can fix by editing the body); a body the stricter canonicalizer +cannot parse (cycles, malformed regions) falls back to the raw save so a +work-in-progress draft stays saveable, in draft and publish mode alike — +`registerFlow` still refuses to arm it; no engine reachable saves as before. +The pass is copy-on-write, so `migrateStoredMetadata` and `duplicatePackage` +re-entering `saveMetaItem` with already-canonical bodies pay nothing. + +Reads still skip flows, and now the loop is closed from the other side: a +served legacy body is healed the moment it is saved back. diff --git a/packages/cli/src/commands/migrate/meta.stored-flow-resolution.integration.test.ts b/packages/cli/src/commands/migrate/meta.stored-flow-resolution.integration.test.ts index 249b10568c..43d8e02dee 100644 --- a/packages/cli/src/commands/migrate/meta.stored-flow-resolution.integration.test.ts +++ b/packages/cli/src/commands/migrate/meta.stored-flow-resolution.integration.test.ts @@ -155,6 +155,70 @@ describe('os migrate meta --stored — the protocol resolves the engine itself ( } }, 120_000); + it('a Studio edit heals the row — save persists the canonical dialect (#4542)', async () => { + // The other half of the acceptance: the migration is no longer the ONLY + // path that retires a legacy flow row. An author's ordinary round-trip — + // GET (served the legacy dialect, per the ADR-0078 read skip) → edit a + // label → PUT the body back — used to re-persist `config.filters` + // verbatim and leave the row `pending` forever; `saveMetaItem` now + // canonicalizes flow bodies before its schema gate. + const stack = await bootSchemaStack({ + databaseUrl: `file:${dbFile}`, + projectRoot: dir, + extraPlugins: await buildDataMigrationPlugins({ automation: true }), + }); + try { + const ql = engineOf(stack); + await ql.insert('sys_metadata', { + type: 'flow', + name: 'sfs_purge', + state: 'active', + metadata: JSON.stringify(LEGACY_FLOW), + }, SYSTEM); + + const protocol: any = stack.kernel.getService('protocol'); + + // The read serves the stored (legacy) dialect — that skip is deliberate + // and unchanged; the heal happens on the way back in. + const served = await protocol.getMetaItem({ type: 'flow', name: 'sfs_purge' }); + const item = served?.item ?? served; + expect(item.nodes.find((n: any) => n.id === 'n1').config).toHaveProperty('filters'); + + // Edit only the label — exactly the probe from #4542. Explicit + // `parentVersion: null`: a raw-seeded row has `checksum: null`, so the + // derived parent would disagree with the column and 409 (probe-only + // artifact; governed rows always carry a checksum). + await protocol.saveMetaItem({ + type: 'flow', + name: 'sfs_purge', + item: { ...item, label: 'Purge Stale Leads (edited)' }, + parentVersion: null, + actor: 'studio-roundtrip-probe', + }); + + const [row] = await ql.find('sys_metadata', { + where: { type: 'flow', name: 'sfs_purge', state: 'active' }, + }, SYSTEM); + const stored = typeof row.metadata === 'string' ? JSON.parse(row.metadata) : row.metadata; + expect(stored.label).toBe('Purge Stale Leads (edited)'); + const node = stored.nodes.find((n: any) => n.id === 'n1'); + expect(node.config).toEqual({ objectName: 'sfs_lead', filter: { title: 'stale' } }); + expect(node.config).not.toHaveProperty('filters'); + // Still no schema defaults — the save persists `storable`, not `parsed`. + expect(stored).not.toHaveProperty('runAs'); + + // The row the edit healed is retired from the stored report: the + // `--stored` preview that stayed `pending` "no matter how many times an + // author edits it" now comes back canonical. + const preview = await protocol.migrateStoredMetadata({ types: ['flow'] }); + expect(preview.scanned).toBe(1); + expect(preview.canonical).toBe(1); + expect(preview.pending).toBe(0); + } finally { + await stack.shutdown(); + } + }, 120_000); + it('without the automation plugin the row is skipped with the reason, never counted done', async () => { // The honest negative: the coverage comes from the engine being present, // not from the report defaulting to optimistic. diff --git a/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts b/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts new file mode 100644 index 0000000000..84c1d024aa --- /dev/null +++ b/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts @@ -0,0 +1,294 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4542 — `saveMetaItem` canonicalizes flow bodies on write. + * + * The read path serves stored flows verbatim (the ADR-0078 conflict guard + * needs the engine's executor registry), and `FlowNodeSchema.config` is an + * open record — so before this fix a Studio round-trip (GET legacy dialect → + * edit label → PUT it back) re-persisted the pre-protocol shape and the row + * stayed `pending` in `os migrate meta --stored` forever. Every other type is + * healed by an author's save; these tests pin that flows now are too. + * + * Harness: the real repository write path (stub engine with findOne / find / + * insert / update, as in objectql's protocol-save-meta-repo-path tests) plus + * the services-table canonicalizer stub from protocol.flow-canonicalizer.test + * — the existing flow-canonicalizer harness mocks `saveMetaItem` itself, which + * a fix INSIDE `saveMetaItem` cannot use. + */ +import { describe, expect, it, vi } from 'vitest'; +import { hashSpec } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +/** A flow body that passes `saveMetaItem`'s schema gate. */ +const flowBody = (config: Record) => ({ + name: 'purge_flow', + label: 'Purge Stale Leads', + type: 'autolaunched', + status: 'active', + nodes: [{ id: 'n1', type: 'delete_record', label: 'Purge', config }], + edges: [], +}); + +/** + * Stands in for `AutomationEngine.canonicalizeStoredFlow` — same contract, + * including the copy-on-write identity an unchanged body comes back with. + */ +const canonicalizeStoredFlow = (_name: string, body: any) => { + const node = body?.nodes?.[0]; + if (!node || !('filters' in (node.config ?? {}))) { + return { storable: body, notices: [], conflicts: [] }; + } + const { filters, ...rest } = node.config; + return { + storable: { ...body, nodes: [{ ...node, config: { ...rest, filter: filters } }] }, + notices: [{ + conversionId: 'flow-node-crud-filter-alias', + surface: 'flow.node.config.filter', + from: 'filters', + to: 'filter', + path: 'flows[0].nodes[0].config', + message: 'filters → filter', + }], + conflicts: [], + }; +}; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + state: string; + metadata: string; + checksum?: string; +} + +function keyOf(w: Record) { + return `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}`; +} + +/** The engine surface the repository write path touches. */ +function makeStubEngine() { + const rows = new Map(); + let nextId = 0; + const findRow = (w: Record): { key: string; row: Row } | null => { + if (w.id !== undefined) { + for (const [k, r] of rows) if (r.id === w.id) return { key: k, row: r }; + return null; + } + for (const [k, r] of rows) { + if (w.type !== undefined && r.type !== w.type) continue; + if (w.name !== undefined && r.name !== w.name) continue; + if (w.organization_id !== undefined && r.organization_id !== w.organization_id) continue; + if (w.state !== undefined && r.state !== w.state) continue; + 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) => { + if (opts.where.type && r.type !== opts.where.type) return false; + if (opts.where.organization_id !== undefined + && r.organization_id !== opts.where.organization_id) return false; + if (opts.where.state && r.state !== opts.where.state) return false; + return true; + }); + }, + async insert(_t: string, data: Record) { + if (_t === 'sys_metadata_audit') return { id: 'audit_skip' }; + nextId += 1; + const row = { id: `r_${nextId}`, ...(data as any) } as Row; + rows.set(keyOf(data), row); + return { id: row.id }; + }, + async update(_t: string, data: Record, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) return { id: null }; + rows.set(found.key, { ...found.row, ...(data as any) }); + return { id: found.row.id }; + }, + async delete(_t: string, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) return { deleted: 0 }; + rows.delete(found.key); + return { deleted: 1 }; + }, + registry: { + registerItem: () => {}, + registerObject: () => {}, + }, + }; + return { engine, rows }; +} + +function makeProtocol(services: Map = new Map()) { + const { engine, rows } = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(engine, () => services); + return { protocol, rows, services }; +} + +const storedFlow = (rows: Map) => { + const row = Array.from(rows.values()).find((r) => r.type === 'flow'); + return row ? { row, body: JSON.parse(row.metadata) } : undefined; +}; + +const save = (protocol: any, item: unknown, extra: Record = {}) => + protocol.saveMetaItem({ type: 'flow', name: 'purge_flow', item, ...extra }); + +describe('saveMetaItem canonicalizes flow bodies (#4542)', () => { + const legacyBody = () => flowBody({ objectName: 'lead', filters: { status: 'stale' } }); + + it('a legacy dialect is healed by the save — the promise every other type already keeps', async () => { + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow }]]), + ); + + const result = await save(protocol, legacyBody()); + + expect(result.success).toBe(true); + const stored = storedFlow(rows)!; + expect(stored.body.nodes[0].config).toEqual({ objectName: 'lead', filter: { status: 'stale' } }); + expect(stored.body.nodes[0].config).not.toHaveProperty('filters'); + // The checksum pairs with the CANONICAL body — what the row holds is + // what was hashed, so history/OCC stay coherent. + expect(stored.row.checksum).toBe(hashSpec(stored.body)); + }); + + it('a refused rename fails the save loudly and persists NOTHING', async () => { + const conflicting = () => ({ + storable: {}, + notices: [], + conflicts: [{ + conversionId: 'flow-node-type-open-namespace', + token: 'http_request', + path: 'flows[0].nodes[0].type', + message: 'a custom executor owns this node type here', + }], + }); + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: conflicting }]]), + ); + + await expect(save(protocol, legacyBody())).rejects.toMatchObject({ + code: 'FLOW_CONVERSION_CONFLICT', + status: 409, + }); + await expect(save(protocol, legacyBody())).rejects.toThrow(/'http_request' at flows\[0\]\.nodes\[0\]\.type is a live name in this environment/); + expect(rows.size).toBe(0); + }); + + it('a canonicalizer throw falls back to the raw save — today\'s gate stays the arbiter', async () => { + // `canonicalizeStoredFlow` is stricter than the schema gate (cycle + // detection, control-flow regions). A WIP draft that saves fine today + // must not become unsaveable. + const throwing = () => { throw new Error('cycle detected: n1 → n1'); }; + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: throwing }]]), + ); + + const result = await save(protocol, legacyBody()); + + expect(result.success).toBe(true); + expect(storedFlow(rows)!.body.nodes[0].config).toHaveProperty('filters'); + }); + + it('a canonicalizer throw does NOT rescue a body the schema gate rejects', async () => { + const throwing = () => { throw new Error("Unrecognized key: '_uiPosition'"); }; + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: throwing }]]), + ); + + await expect( + save(protocol, { ...legacyBody(), _uiPosition: { x: 1, y: 2 } }), + ).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 }); + expect(rows.size).toBe(0); + }); + + it('no automation service reachable → saved exactly as today', async () => { + // A control-plane / metadata-only host must not start refusing flow + // writes it accepted yesterday. The row is then no better than before — + // `os migrate meta --stored` reports it. + const { protocol, rows } = makeProtocol(new Map()); + + const result = await save(protocol, legacyBody()); + + expect(result.success).toBe(true); + expect(storedFlow(rows)!.body.nodes[0].config).toHaveProperty('filters'); + }); + + it('an already-canonical body persists byte-identical (copy-on-write identity)', async () => { + const spy = vi.fn(canonicalizeStoredFlow); + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: spy }]]), + ); + const body = flowBody({ objectName: 'lead', filter: { status: 'stale' } }); + + await save(protocol, body); + + expect(spy).toHaveBeenCalledTimes(1); + expect(storedFlow(rows)!.body).toEqual(body); + }); + + it('draft mode canonicalizes the same way', async () => { + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow }]]), + ); + + const result = await save(protocol, legacyBody(), { mode: 'draft' }); + + expect(result.success).toBe(true); + const stored = storedFlow(rows)!; + expect(stored.row.state).toBe('draft'); + expect(stored.body.nodes[0].config).toEqual({ objectName: 'lead', filter: { status: 'stale' } }); + }); + + it('draft mode keeps the throw-fallback too — a WIP cycle stays saveable', async () => { + const throwing = () => { throw new Error('cycle detected: n1 → n1'); }; + const { protocol, rows } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: throwing }]]), + ); + + const result = await save(protocol, legacyBody(), { mode: 'draft' }); + + expect(result.success).toBe(true); + expect(storedFlow(rows)!.body.nodes[0].config).toHaveProperty('filters'); + }); + + it('resolution is LAZY — a service registered after construction is still found', async () => { + const services = new Map(); + const { protocol, rows } = makeProtocol(services); + services.set('automation', { canonicalizeStoredFlow }); + + await save(protocol, legacyBody()); + + expect(storedFlow(rows)!.body.nodes[0].config).not.toHaveProperty('filters'); + }); + + it('the canonicalizer is called with the request NAME, not the body', async () => { + const spy = vi.fn(canonicalizeStoredFlow); + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: spy }]]), + ); + await save(protocol, legacyBody()); + expect(spy.mock.calls[0][0]).toBe('purge_flow'); + }); + + it('non-flow saves never consult the canonicalizer', async () => { + const spy = vi.fn(canonicalizeStoredFlow); + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: spy }]]), + ); + await protocol.saveMetaItem({ + type: 'view', + name: 'case_grid', + organizationId: 'org_alpha', + item: { name: 'case_grid', type: 'grid', label: 'Cases', columns: ['id', 'title'] }, + }); + expect(spy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index a3fb503d90..3acab0dd60 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -6286,6 +6286,63 @@ export class ObjectStackProtocolImplementation implements request.item = normalizeViewMetadata(request.type, request.item, request.name, baseline); } + // Canonicalize `flow` bodies BEFORE the schema gate, so an author's + // save heals a pre-protocol row the way it heals every other type + // (#4542). The read path serves stored flows verbatim (the ADR-0078 + // conflict guard needs the engine's executor registry — see + // {@link resolveFlowCanonicalizer}), and `FlowNodeSchema.config` is an + // open record, so without this pass the gate below accepts the legacy + // dialect back and the row stays `pending` in `os migrate meta + // --stored` forever. Persists `storable`, never the parsed shape — + // schema defaults are deliberately excluded (ADR-0087). Copy-on-write: + // an already-canonical body comes back reference-identical, so + // `migrateStoredMetadata` and `duplicatePackage` re-entering here pay + // nothing. + if (singularType === 'flow' && request.item) { + // No automation service reachable (control-plane / metadata-only + // host): save exactly as today — a host must not start refusing + // flow writes it accepted yesterday. + const canonicalizeFlow = this.resolveFlowCanonicalizer(); + if (canonicalizeFlow) { + let result: StoredFlowCanonicalization | undefined; + try { + result = canonicalizeFlow(request.name, request.item); + } catch { + // `canonicalizeStoredFlow` is STRICTER than the gate below + // (strict parse + cycle detection + control-flow region + // validation). A work-in-progress draft with a temporary + // cycle must stay saveable, so fall back to the raw body + // and let today's gate stay the arbiter — in draft AND + // publish mode; `registerFlow` refuses to arm a malformed + // flow either way. + result = undefined; + } + if (result) { + if (result.conflicts.length > 0) { + // ADR-0078's guard refused a node-type rename because + // the old token is a LIVE name owned by a custom + // executor here. Persisting the un-renamed body would + // mint exactly the row this pass exists to prevent + // (same posture as `duplicatePackage` / #4454). 409, + // not 422: the body may be perfectly valid — the + // refusal comes from environment state, so + // resubmitting the same body cannot help. + const first = result.conflicts[0]!; + const err = new Error( + `[flow_conversion_conflict] ${request.type}/${request.name}: conversion refused — ` + + `'${first.token}' at ${first.path} is a live name in this environment ` + + `(${result.conflicts.length} conflict(s)). ${first.message}` + ); + (err as any).code = 'FLOW_CONVERSION_CONFLICT'; + (err as any).status = 409; + (err as any).conflicts = result.conflicts; + throw err; + } + request.item = result.storable; + } + } + } + // Spec-conformance check: if a Zod schema is registered for this // overlay type (see OVERLAY_VALIDATION_SCHEMAS), validate the payload // before persisting. We surface invalid payloads as `422