fix(metadata-protocol): a flow save that skipped canonicalization says so (#4580) (#4582)

`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 WIP draft with a temporary cycle stays saveable. That fallback is correct
and unchanged here. It was also completely silent.

Three of the four postures at this seam announce themselves: a clean pass 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 re-persisted verbatim. That is
the #4542 symptom arriving silently, against a boot warning that tells the
author re-saving is the remedy.

The fallback now warns, 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.

No behavior change: the body still saves, the 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 can only be
enforced where an automation service exists, making the same body saveable on a
control-plane host and a 422 on an automation host.

Tests: 5 new cases in protocol.save-flow-canonicalization.test.ts — warns with
the flow name and reason; deduped across repeat saves; silent on the clean,
conflict, and no-service paths. Full metadata-protocol suite green (206).


Claude-Session: https://claude.ai/code/session_01NDmJ5ASMSzcw1q17vh32MG

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

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

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<string, unknown> = {})
 describe('saveMetaItem canonicalizes flow bodies (#4542)', () => {
     const legacyBody = () => flowBody({ objectName: 'lead', filters: { status: 'stale' } });
 
+    let warn: ReturnType<typeof vi.spyOn>;
+    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(

packages/metadata-protocol/src/protocol.ts

@@ -1719,6 +1719,14 @@ export class ObjectStackProtocolImplementation implements
      */
     private storedConversionWarned = new Set<string>();
 
+    /**
+     * 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<string>();
+
     /**
      * Canonicalize a stored `sys_metadata` body on rehydration (#3903;
      * ADR-0087 addendum "stored metadata replays the chain").
@@ -6250,14 +6258,39 @@ 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
                     // 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.
+                    //
+                    // 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) {