fix(spec,service-automation): the wait executor reads its declared contract only (#4045)

.changeset/wait-loose-config-graduation.md

@@ -0,0 +1,46 @@
+---
+'@objectstack/spec': patch
+'@objectstack/service-automation': patch
+---
+
+The `wait` executor reads its declared contract only; the loose `config` back door graduates into the conversion layer (#4045).
+
+`wait` keeps its contract in `waitEventConfig` — a declared, `.describe()`-annotated
+block on `FlowNodeSchema` that is in the authorable-field list, reaches the generated
+reference, and is what the showcase actually authors. Its descriptor publishes no
+`configSchema`, which is by design rather than the gap it first looks like.
+
+The executor nevertheless also read six loose `config` keys behind `wec.X ?? loose.X`,
+two of them (`duration`, `signal`) spellings the spec never declared anywhere. That is
+the `notify.source` shape #4050 retired: a second de-facto contract announced only by a
+code comment, so an author who wrote it got a flow that worked forever and was never
+steered to the declared spelling (PD #12). Not hypothetical: the showcase's own
+`wait_revision` node authored it (`config: { eventType: 'signal', signalName: … }`) and
+moves to the declared block here.
+
+- New ADR-0087 D2 conversion `flow-node-wait-event-config-lift` lifts
+  `config.{eventType,timerDuration,duration,timeoutMs,signalName,signal}` onto the
+  declared `waitEventConfig` block, in the executor's own `??` precedence — a declared
+  value wins and its loose counterpart is left shadowed, exactly as `renameConfigKey`
+  treats a shadowed alias.
+- `eventType` is stamped `'timer'` whenever the lift would otherwise leave the block
+  without one. This is load-bearing: the loader parses the **converted** flow
+  (`applyConversionsToFlow` → `FlowSchema.parse`) and `waitEventConfig.eventType` is
+  required once the block exists — so a stored flow carrying only
+  `config: { duration: 'PT1M' }` would have gone from working to failing to load.
+  `'timer'` is the exact default the executor applied to that shape.
+- The executor's six `?? loose.*` fallbacks are deleted. The surviving `?? 'timer'` is
+  not one: `waitEventConfig` is itself optional, and a wait node without one is a valid
+  timer wait.
+
+Verified at the real seam: the new executor tests author the legacy shape and go through
+`registerFlow`, which is what applies the conversion, so they prove the graduation
+end-to-end on a legacy source rather than only that the executor stopped looking. A
+negative control pins the `eventType` default — deleting it from the converted output
+makes `FlowSchema.parse` throw.
+
+Two things this deliberately does **not** change, filed as #4158 rather than fixed in
+passing: `waitEventConfig.timeoutMs` is declared as a timeout guard but read as a timer
+duration, and `waitEventConfig.onTimeout` has zero readers anywhere — so `wait` has no
+timeout implementation at all, while the showcase authors `onTimeout: 'continue'`.
+Implementing or retracting that is a behaviour change, not a contract cleanup.

docs/protocol-upgrade-guide.md

@@ -124,6 +124,8 @@ Finally it removes agent `tools` (#3894): the legacy inline `{type,name,descript
 
 Beyond those spec-surface removals, it graduates the seven flow-node config key aliases the executors still tolerated (#3796): the CRUD nodes' `object` (use `objectName`) — the last tenant of the `readAliasedConfig` executor shim, which is deleted with it — plus the six open-coded fallbacks that never went through that shim: notify `to`/`subject`/`body`/`url` (use `recipients`/`title`/`message`/`actionUrl`) and script `functionName`/`input` (use `function`/`inputs`). All are pure key renames with unchanged values and replay losslessly. Like the sharing-rule access level above they keep a load-path acceptance window: none carried a prior deprecation warning, and `FlowNodeSchema.config` is an unconstrained record, so no schema tombstone can reject them — the conversion layer is the only seam that can declare, convert, and retire them.
 
+The same graduation covers `wait`, whose fallback was not a config-to-config rename (#4045). `wait` keeps its contract in the declared `waitEventConfig` block, not in `config` at all — yet the executor also read six loose `config` keys, two of them (`duration`, `signal`) spellings the spec never declared. The conversion lifts them onto the declared block in the executor's own `??` precedence, so a value already declared wins and its loose counterpart is left shadowed. One wrinkle makes this a rewrite rather than a delete: `waitEventConfig.eventType` is required once the block exists, and the loader parses the CONVERTED flow — so a source carrying only `config: { duration }` is stamped with `eventType: 'timer'`, the exact default the executor applied to that shape. Behaviour-preserving in both directions.
+
 And it removes the RLS-policy key `priority` (#3896 security audit): promised "conflict resolution" that cannot exist, because applicable policies OR-combine (most permissive wins) — there is never a conflict to order, and nothing ever read the key (call graph closed across the collection site, the projection round-trip and the compiler). A pure lossless delete: outcomes are identical with or without it; the schema tombstones the key with the same prescription.
 
 The same close-out retires the four inert tool authoring keys (`category`, `permissions`, `active`, `builtIn`): none is part of AIToolDefinition and no execution path read them. Two were misleading in the dangerous direction — `permissions` promised an invocation gate nothing enforced, and `active: false` read as "withdrawn" while the tool kept reaching the LLM tool set. Lossless deletes; the strict ToolSchema rejects each with its prescription.
@@ -146,6 +148,7 @@ The close-out sweep finishes the enforce-or-remove worklist across the remaining
 | `sharing-rule-access-level-full-to-edit` | `sharingRule.accessLevel` | sharing-rule accessLevel 'full' → 'edit' (#3865 — `full` never granted more than `edit`) | live — protocol 17 loader accepts the old shape |
 | `flow-node-crud-object-alias` | `flow.node.config.objectName` | CRUD flow-node config key 'object' → 'objectName' (#3796 — `readAliasedConfig` shim graduation) | live — protocol 17 loader accepts the old shape |
 | `flow-node-notify-config-aliases` | `flow.node.notify.config` | notify flow-node config keys 'to' → 'recipients', 'subject' → 'title', 'body' → 'message', 'url' → 'actionUrl' (#3796), and nested 'source: {object, id}' → 'sourceObject' / 'sourceId' (#4045) | live — protocol 17 loader accepts the old shape |
+| `flow-node-wait-event-config-lift` | `flow.node.wait.waitEventConfig` | wait flow-node loose config keys → the declared `waitEventConfig` block: 'eventType', 'timerDuration'/'duration' → 'timerDuration', 'signalName'/'signal' → 'signalName', 'timeoutMs' (#4045) | live — protocol 17 loader accepts the old shape |
 | `flow-node-script-config-aliases` | `flow.node.script.config` | script flow-node config keys 'functionName' → 'function', 'input' → 'inputs' (#3796) | live — protocol 17 loader accepts the old shape |
 | `permission-rls-priority-removed` | `permission.rowLevelSecurity.priority` | RLS-policy key 'priority' removed (#3896 audit — policies OR-combine, so the promised conflict-resolution semantics cannot exist; dropping it changes no outcome) | retired — `migrate meta` only |
 | `tool-inert-authoring-keys-removed` | `tool.category / tool.permissions / tool.active / tool.builtIn` | tool keys 'category'/'permissions'/'active'/'builtIn' removed (#3896 close-out — authorable and inert; permissions gated nothing, active:false withdrew nothing) | retired — `migrate meta` only |

examples/app-showcase/src/automation/flows/index.ts

@@ -206,7 +206,10 @@ export const BudgetApprovalFlow = defineFlow({
       id: 'wait_revision',
       type: 'wait',
       label: 'Awaiting Revision',
-      config: { eventType: 'signal', signalName: 'budget_revision' },
+      // `waitEventConfig`, not a loose `config` — the latter is the undeclared
+      // back door retired in #4045. The conversion layer still rewrites it at
+      // load, but the showcase should demonstrate the declared spelling.
+      waitEventConfig: { eventType: 'signal', signalName: 'budget_revision' },
     },
     {
       id: 'needs_exec',

packages/services/service-automation/src/builtin/wait-node.test.ts

@@ -140,6 +140,101 @@ describe('wait node executor', () => {
   });
 });
 
+/**
+ * The loose `config.*` back door the executor used to read alongside
+ * `waitEventConfig` graduated into the ADR-0087 D2 conversion layer
+ * (`flow-node-wait-event-config-lift`, #4045), so the executor now reads the
+ * declared block only (PD #12).
+ *
+ * These go through `registerFlow`, which is the seam that applies the conversion
+ * — so they prove the graduation end-to-end on a legacy source, not merely that
+ * the executor stopped looking. Both spellings under test (`duration`, `signal`)
+ * are ones the spec never declared; they existed only as the tail of the `??`
+ * chains this change deleted.
+ */
+describe('wait config graduation — legacy loose `config` still works via the conversion (#4045)', () => {
+  let engine: AutomationEngine;
+  let ran: string[];
+
+  /** The same flow, but authored the legacy way: event keys loose under `config`. */
+  const looseWaitFlow = (config: Record<string, unknown>) => ({
+    ...waitFlow({ eventType: 'timer' }),
+    nodes: [
+      { id: 'start', type: 'start', label: 'Start' },
+      { id: 'pause', type: 'wait', label: 'Wait', config },
+      { id: 'after', type: 'mark', label: 'After' },
+      { id: 'end', type: 'end', label: 'End' },
+    ],
+  });
+
+  beforeEach(() => {
+    engine = new AutomationEngine(silentLogger());
+    ran = [];
+    engine.registerNodeExecutor(markerExecutor(ran));
+  });
+
+  it('schedules the same timer from a loose `config.duration` (undeclared spelling, no eventType)', async () => {
+    const { ctx, scheduled } = fakeJobCtx();
+    registerWaitNode(engine, ctx);
+    // No eventType anywhere: the conversion stamps the executor's own 'timer'
+    // default, without which the converted flow would not even parse.
+    engine.registerFlow('wait_flow', looseWaitFlow({ duration: 'PT2H' }));
+
+    const before = Date.now();
+    const paused = await engine.execute('wait_flow');
+    expect(paused.status).toBe('paused');
+    expect(ran).toEqual([]);
+
+    expect(scheduled).toHaveLength(1);
+    const at = new Date(scheduled[0].schedule.at!).getTime();
+    expect(at).toBeGreaterThanOrEqual(before + 7_200_000 - 1000);
+    expect(at).toBeLessThanOrEqual(Date.now() + 7_200_000 + 1000);
+  });
+
+  it('suspends on a loose `config.signal` (undeclared spelling) as its declared counterpart', async () => {
+    registerWaitNode(engine, ctxNoJob());
+    engine.registerFlow('wait_flow', looseWaitFlow({ eventType: 'signal', signal: 'contract.renewed' }));
+
+    const paused = await engine.execute('wait_flow');
+    expect(paused.status).toBe('paused');
+    expect(engine.listSuspendedRuns()[0]).toMatchObject({ nodeId: 'pause', correlation: 'contract.renewed' });
+
+    const resumed = await engine.resume(paused.runId!);
+    expect(resumed.success).toBe(true);
+    expect(ran).toEqual(['after']);
+  });
+
+  // Unlike the two above, this one passes with the conversion unregistered too —
+  // it pins the EXECUTOR's side of the precedence (a declared value is what gets
+  // read), not the conversion's. Kept because that is the half a future
+  // "simplification" of the lift could silently invert.
+  it('reads the declared value, not its loose counterpart, when both are present', async () => {
+    const { ctx, scheduled } = fakeJobCtx();
+    registerWaitNode(engine, ctx);
+    engine.registerFlow('wait_flow', {
+      ...waitFlow({ eventType: 'timer' }),
+      nodes: [
+        { id: 'start', type: 'start', label: 'Start' },
+        {
+          id: 'pause',
+          type: 'wait',
+          label: 'Wait',
+          waitEventConfig: { eventType: 'timer', timerDuration: 'PT1H' },
+          config: { duration: 'PT9H' },
+        },
+        { id: 'after', type: 'mark', label: 'After' },
+        { id: 'end', type: 'end', label: 'End' },
+      ],
+    });
+
+    const before = Date.now();
+    await engine.execute('wait_flow');
+    // 1h (declared) — not 9h (loose). Same precedence the deleted `??` had.
+    const at = new Date(scheduled[0].schedule.at!).getTime();
+    expect(at).toBeLessThanOrEqual(before + 3_600_000 + 1000);
+  });
+});
+
 describe('rearmSuspendedWaitTimers (cold-boot timer re-arm)', () => {
   /** Boot a fresh engine wired to `store` with the wait flow registered — one "process". */
   function bootEngine(store: InMemorySuspendedRunStore, ctx: any, waitConfig: Record<string, unknown>) {

packages/services/service-automation/src/builtin/wait-node.ts

@@ -49,18 +49,29 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext):
       isAsync: true,
     }),
     async execute(node, variables, _context) {
-      // Prefer the spec-structured `waitEventConfig` block; fall back to a loose
-      // `config` for hand-authored flows that put the same keys under config.
-      const loose = (node.config ?? {}) as Record<string, unknown>;
+      // `waitEventConfig` is the whole contract (`FlowNodeSchema`, flow.zod.ts).
+      // The loose `config.*` back door this used to also read — six keys, two of
+      // them (`duration`, `signal`) spellings the spec never declared — graduated
+      // into the ADR-0087 D2 conversion layer as
+      // `flow-node-wait-event-config-lift` (#4045). It is rewritten at load,
+      // including the `registerFlow` rehydration seam, so there is nothing left
+      // to fall back to here (PD #12: no consumer-side fallbacks).
+      //
+      // The `?? 'timer'` below is NOT such a fallback: `waitEventConfig` is
+      // itself optional, and a wait node without one is a valid timer wait.
       const wec = (node.waitEventConfig ?? {}) as Record<string, unknown>;
-      const eventType = String(wec.eventType ?? loose.eventType ?? 'timer');
+      const eventType = String(wec.eventType ?? 'timer');
       const runId = variables.get('$runId');
 
       if (eventType === 'timer') {
+        // `timeoutMs` doubling as a duration is pre-existing behaviour, kept
+        // deliberately — the declared meaning is a timeout guard, and `wait` has
+        // no timeout implementation at all (`onTimeout` has zero readers). That
+        // gap is #4158; changing it here would be a behaviour change riding on a
+        // contract cleanup.
         const durationMs =
-          parseIsoDuration(wec.timerDuration ?? loose.timerDuration ?? loose.duration) ??
-          (typeof wec.timeoutMs === 'number' ? wec.timeoutMs : undefined) ??
-          (typeof loose.timeoutMs === 'number' ? (loose.timeoutMs as number) : undefined);
+          parseIsoDuration(wec.timerDuration) ??
+          (typeof wec.timeoutMs === 'number' ? wec.timeoutMs : undefined);
 
         // Persist the wake deadline as node output: the engine writes output
         // to variables (`<nodeId>.waitUntil`) *before* snapshotting the
@@ -105,7 +116,7 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext):
 
       // signal / webhook / manual / condition — suspend; an external producer
       // resumes the run when the named event arrives.
-      const signal = String(wec.signalName ?? loose.signalName ?? loose.signal ?? `wait:${node.id}`);
+      const signal = String(wec.signalName ?? `wait:${node.id}`);
       return { success: true, suspend: true, correlation: signal };
     },
   });

packages/spec/spec-changes.json

@@ -104,6 +104,12 @@
         "conversionId": "flow-node-notify-config-aliases",
         "toMajor": 17
       },
+      {
+        "surface": "flow.node.wait.waitEventConfig",
+        "to": "wait flow-node loose config keys → the declared `waitEventConfig` block: 'eventType', 'timerDuration'/'duration' → 'timerDuration', 'signalName'/'signal' → 'signalName', 'timeoutMs' (#4045)",
+        "conversionId": "flow-node-wait-event-config-lift",
+        "toMajor": 17
+      },
       {
         "surface": "flow.node.script.config",
         "to": "script flow-node config keys 'functionName' → 'function', 'input' → 'inputs' (#3796)",
@@ -495,6 +501,12 @@
           "conversionId": "flow-node-notify-config-aliases",
           "toMajor": 17
         },
+        {
+          "surface": "flow.node.wait.waitEventConfig",
+          "to": "wait flow-node loose config keys → the declared `waitEventConfig` block: 'eventType', 'timerDuration'/'duration' → 'timerDuration', 'signalName'/'signal' → 'signalName', 'timeoutMs' (#4045)",
+          "conversionId": "flow-node-wait-event-config-lift",
+          "toMajor": 17
+        },
         {
           "surface": "flow.node.script.config",
           "to": "script flow-node config keys 'functionName' → 'function', 'input' → 'inputs' (#3796)",