diff --git a/.changeset/save-flow-canonicalization-fallback-warning.md b/.changeset/save-flow-canonicalization-fallback-warning.md new file mode 100644 index 0000000000..cf8094cc9f --- /dev/null +++ b/.changeset/save-flow-canonicalization-fallback-warning.md @@ -0,0 +1,35 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): a flow save that skipped canonicalization says so (#4580) + +`saveMetaItem` canonicalizes flow bodies before the schema gate (#4542). When the +canonicalizer throws — it is stricter than the gate: strict parse, cycle +detection, control-flow region validation — the save falls back to the raw body +so a work-in-progress draft with a temporary cycle stays saveable. That fallback +is correct and unchanged. It was also completely silent. + +Of the four postures at this seam, three announce themselves: a clean +canonicalization heals the row, a refused rename fails with `409 +FLOW_CONVERSION_CONFLICT` naming the token, and a host with no automation service +is reported by `os migrate meta --stored`. The throw-fallback said nothing, so a +save that skipped canonicalization was indistinguishable from one that healed the +row — and a body that is *both* a legacy dialect and unparseable by the strict +canonicalizer re-persisted verbatim. That is the exact #4542 symptom, arriving +silently, while the boot warning for legacy stored rows tells the author that +re-saving is the remedy. + +The fallback now emits a `console.warn` naming the flow and the canonicalizer's +own error, deduped once per flow per process (the `convertStoredItem` pattern — +Studio autosaves the same draft repeatedly, and a WIP cycle throws on every +write). This aligns the write seam with ADR-0087 D2's "loud" posture, where +conversions emit notices, reads warn once per row, and `migrateStoredMetadata` +reports `failed` with the message. + +No behavior change: the body still saves, the schema gate stays the arbiter, and +`registerFlow` still refuses to arm a malformed flow. Refusing the save in +publish mode was considered and rejected — publish is the default mode, so it +would silently tighten validation for every existing caller, and it could only be +enforced on hosts that have an automation service, making the same body saveable +on a control-plane host and a 422 on an automation host. diff --git a/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts b/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts index 84c1d024aa..dd80a814f7 100644 --- a/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts +++ b/packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts @@ -16,7 +16,7 @@ * — the existing flow-canonicalizer harness mocks `saveMetaItem` itself, which * a fix INSIDE `saveMetaItem` cannot use. */ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { hashSpec } from '@objectstack/metadata-core'; import { ObjectStackProtocolImplementation } from './protocol.js'; @@ -143,6 +143,20 @@ const save = (protocol: any, item: unknown, extra: Record = {}) describe('saveMetaItem canonicalizes flow bodies (#4542)', () => { const legacyBody = () => flowBody({ objectName: 'lead', filters: { status: 'stale' } }); + let warn: ReturnType; + beforeEach(() => { warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); }); + afterEach(() => { warn.mockRestore(); }); + + /** + * Only the throw-fallback's own warnings. The protocol emits unrelated + * one-shot warnings (e.g. #3770's "engine has no schema registry"), and + * whether one has already fired depends on test order — matching on the + * message keeps these assertions immune to that. + */ + const fallbackWarnings = (): string[] => (warn.mock.calls as unknown[][]) + .map((c: unknown[]) => String(c[0])) + .filter((m: string) => m.includes('WITHOUT canonicalization')); + 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 }]]), @@ -278,6 +292,81 @@ describe('saveMetaItem canonicalizes flow bodies (#4542)', () => { expect(spy.mock.calls[0][0]).toBe('purge_flow'); }); + // ── #4580: the throw-fallback is correct, but it must not be silent ── + + it('a throw-fallback SAYS SO — naming the flow and the canonicalizer\'s own reason', async () => { + // Before #4580 this posture was the only one with no signal at all: a + // save that skipped canonicalization looked exactly like one that + // healed the row, and a body that is BOTH legacy and unparseable + // re-persisted verbatim while the boot warning told the author that + // re-saving would fix it. + const throwing = () => { throw new Error('cycle detected: n1 → n1'); }; + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: throwing }]]), + ); + + await save(protocol, legacyBody()); + + expect(fallbackWarnings()).toHaveLength(1); + const msg = fallbackWarnings()[0]; + expect(msg).toContain('flow/purge_flow'); + expect(msg).toContain('cycle detected: n1 → n1'); + expect(msg).toContain('os migrate meta --stored'); + }); + + it('the fallback warning is deduped per flow — Studio autosave must not spam', async () => { + const throwing = () => { throw new Error('cycle detected: n1 → n1'); }; + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: throwing }]]), + ); + + await save(protocol, legacyBody()); + await save(protocol, legacyBody()); + await save(protocol, legacyBody()); + + expect(fallbackWarnings()).toHaveLength(1); + }); + + it('the clean path stays silent — a healed row needs no warning', async () => { + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow }]]), + ); + + await save(protocol, legacyBody()); + + expect(fallbackWarnings()).toHaveLength(0); + }); + + it('the conflict path stays silent — the 409 IS the signal', 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 name here.', + }], + }); + const { protocol } = makeProtocol( + new Map([['automation', { canonicalizeStoredFlow: conflicting }]]), + ); + + await expect(save(protocol, legacyBody())).rejects.toMatchObject({ status: 409 }); + + expect(fallbackWarnings()).toHaveLength(0); + }); + + it('a host with no automation service stays silent — nothing was skipped', async () => { + // There is no canonicalizer to fall back FROM; `os migrate meta + // --stored` is what reports these rows. + const { protocol } = makeProtocol(new Map()); + + await save(protocol, legacyBody()); + + expect(fallbackWarnings()).toHaveLength(0); + }); + it('non-flow saves never consult the canonicalizer', async () => { const spy = vi.fn(canonicalizeStoredFlow); const { protocol } = makeProtocol( diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 3acab0dd60..2d4abf81f7 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1776,6 +1776,14 @@ export class ObjectStackProtocolImplementation implements */ private storedConversionWarned = new Set(); + /** + * Once-per-process dedupe (`type|name`) for the warning `saveMetaItem` + * emits when the flow canonicalizer throws and the save falls back to the + * raw body (#4580). Studio autosaves the same draft over and over, and a + * WIP cycle throws on every one of them. + */ + private flowCanonicalizeFallbackWarned = new Set(); + /** * Canonicalize a stored `sys_metadata` body on rehydration (#3903; * ADR-0087 addendum "stored metadata replays the chain"). @@ -6307,7 +6315,7 @@ export class ObjectStackProtocolImplementation implements let result: StoredFlowCanonicalization | undefined; try { result = canonicalizeFlow(request.name, request.item); - } catch { + } catch (e: any) { // `canonicalizeStoredFlow` is STRICTER than the gate below // (strict parse + cycle detection + control-flow region // validation). A work-in-progress draft with a temporary @@ -6315,6 +6323,31 @@ export class ObjectStackProtocolImplementation implements // and let today's gate stay the arbiter — in draft AND // publish mode; `registerFlow` refuses to arm a malformed // flow either way. + // + // Say so (#4580). The fallback is correct but it is the one + // posture here with no signal of its own: a save that + // skipped canonicalization is otherwise indistinguishable + // from one that healed the row, and a body that is BOTH a + // legacy dialect and unparseable re-persists verbatim — + // the #4542 symptom, silently, against a boot warning that + // told the author re-saving would fix it. Every other link + // in the chain is loud (ADR-0087 D2): conversions emit + // notices, `convertStoredItem` warns on read, + // `migrateStoredMetadata` reports `failed`. + // + // Deduped per flow per process, like {@link + // storedConversionWarned} — Studio autosave writes the same + // draft repeatedly and this must not become a spam loop. + const key = `${singularType}|${request.name}`; + if (!this.flowCanonicalizeFallbackWarned.has(key)) { + this.flowCanonicalizeFallbackWarned.add(key); + console.warn( + `[Protocol] flow/${request.name} was saved WITHOUT canonicalization: ` + + `${e?.message ?? String(e)} The body was persisted as submitted, so a ` + + `pre-protocol shape in it stays legacy on disk. Run ` + + `"os migrate meta --stored" to see the row's status.`, + ); + } result = undefined; } if (result) {