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

.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

@@ -1776,6 +1776,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").
@@ -6307,14 +6315,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) {