fix(spec,service-automation)!: errorHandling.maxRetries has one default, and strategy: 'retry' states its count (#4247)

.changeset/flow-max-retries-single-default.md

@@ -0,0 +1,56 @@
+---
+"@objectstack/spec": major
+"@objectstack/service-automation": major
+---
+
+fix(spec,service-automation)!: `errorHandling.maxRetries` has one default, and `strategy: 'retry'` states its count (#4247)
+
+`flow.errorHandling.maxRetries` was declared twice, with different values:
+
+- **spec** — `FlowSchema` (`automation/flow.zod.ts`): `.default(0)`
+- **engine** — `retryExecution` (`service-automation/src/engine.ts`):
+  `errorHandling.maxRetries ?? 3`
+
+`??` fires only on `undefined`, so the winner was decided by the ROUTE a flow
+took into the engine, not by what its author wrote:
+
+| Path | `errorHandling.maxRetries` | Retries |
+|:---|:---|---:|
+| parsed by `FlowSchema` (`.default(0)` fills it) | `0` | **0** |
+| object built by hand and fed to the engine | `undefined` | **3** |
+
+One authored intent — "I didn't write a count" — two behaviors. The neighbouring
+`retryDelayMs ?? 1000` / `backoffMultiplier ?? 1` agreed with their `.default()`s;
+only `maxRetries` disagreed, which reads as a schema default changed from 3 to 0
+without the engine following, not as a deliberate two-track design.
+
+**The engine keeps no defaults of its own.** `retryExecution` now takes the
+parsed `NonNullable<FlowParsed['errorHandling']>` and destructures all five
+knobs — no `??`. This is safe because `AutomationEngine.flows` only ever holds
+`FlowSchema.parse` output (`registerFlow` parses; the version-history rollback
+re-seats an already-parsed snapshot), and it is what keeps a second set of
+defaults from growing back: a knob the spec stops defaulting becomes a compile
+error rather than a silent engine-side guess. Per Prime Directive #12 the spec
+is the one contract; a consumer-side fallback is a second de-facto one.
+
+**BREAKING — `strategy: 'retry'` now requires `maxRetries` >= 1.** With the
+engine's copy gone, an unstated count is unambiguously `0`, and `'retry'` with 0
+attempts runs the flow once and stops — i.e. `strategy: 'fail'` wearing another
+label, a declared capability the runtime does not deliver (Prime Directive #10
+corollary). Rather than pick 0 or 3 on the author's behalf, `FlowSchema` refuses
+the combination in both spellings (omitted → defaulted 0, and an explicit 0),
+with the prescription in the message. A retry re-runs the **whole flow from the
+start** — records created again, callouts fired again — which is not a number to
+guess for someone.
+
+FROM → TO:
+
+- `errorHandling: { strategy: 'retry' }` → `errorHandling: { strategy: 'retry', maxRetries: 3 }`
+  (or `strategy: 'fail'` if no retry was intended — that is what it did).
+- `errorHandling: { strategy: 'retry', maxRetries: 0 }` → same choice, spelled out.
+
+Unaffected: `maxRetries: 0` under `strategy: 'fail'` / `'continue'` (neither
+reads it, and a fully spelled-out block stays legal), flows with no
+`errorHandling` at all, and every flow that already states a count — including
+the `try_catch` node's own `config.retry`, which is a separate per-region policy
+(`control-flow.zod.ts`) and is unchanged.

content/docs/automation/flows.mdx

@@ -665,7 +665,7 @@ The two recovery mechanisms operate at different scopes and do not compound:
 | | Scope | On failure |
 | :--- | :--- | :--- |
 | `fault` edge | one node | traversal continues from the handler; the run completes |
-| `errorHandling: { strategy: 'retry' }` | the whole flow | the flow re-runs **from the start** |
+| `errorHandling: { strategy: 'retry', maxRetries: n }` | the whole flow | the flow re-runs **from the start**, up to `n` more times |
 
 A failure a fault edge handled is not a flow failure, so it does **not** consume
 a retry. That matters because flow-level retry replays every node that already
@@ -707,13 +707,46 @@ errorHandling: {
 
 | Property | Type | Description |
 | :--- | :--- | :--- |
-| `strategy` | `enum` | `'fail'` (stop) or `'retry'` (re-run the whole flow). `'continue'` parses but the engine branches only on `'retry'`, so it behaves exactly like `'fail'` — use a `fault` edge to keep going past a failed node |
-| `maxRetries` | `number` | Maximum retry attempts (0-10) |
-| `retryDelayMs` | `number` | Delay between retries (ms) |
+| `strategy` | `enum` | `'fail'` (stop) or `'retry'` (re-run the whole flow). `'continue'` parses but the engine branches only on `'retry'`, so it behaves exactly like `'fail'` — use a `fault` edge to keep going past a failed node (default `'fail'`) |
+| `maxRetries` | `number` | Retry attempts **after** the initial one, `0`–`10`. Under `strategy: 'retry'` it must be at least `1` and there is no default — see below (default `0`, i.e. no retries, for the strategies that never retry) |
+| `retryDelayMs` | `number` | Delay between retries (ms) (default `1000`) |
 | `backoffMultiplier` | `number` | Exponential backoff multiplier (default `1`) |
 | `maxRetryDelayMs` | `number` | Ceiling on the backed-off delay (default `30000`) |
 | `jitter` | `boolean` | Randomize the delay to avoid a thundering herd (default `false`) |
 
+`maxRetries` counts the **re-runs**, not the total attempts: `maxRetries: 2`
+runs the flow up to three times.
+
+### `strategy: 'retry'` has to say how many times
+
+There is no default retry count. `strategy: 'retry'` without `maxRetries` — or
+with `maxRetries: 0` — is refused when the flow is registered:
+
+```typescript
+// ❌ rejected: "retry" that retries zero times is just "fail"
+errorHandling: { strategy: 'retry' }
+
+// ✅ state the attempts
+errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 5000 }
+```
+
+A retry re-runs the **whole flow from the start**, so every node that already
+succeeded runs again — records get created again, callouts fire again. That is
+too consequential a number to pick on the author's behalf, and picking `0`
+would make opting into `'retry'` do nothing at all. The knobs above are read
+only under `'retry'`; a fully spelled-out block under `'fail'` or `'continue'`
+is fine and simply ignored.
+
+<Callout type="info">
+  Before ObjectStack 17 the count depended on how the flow reached the engine:
+  a flow parsed by `FlowSchema` retried `0` times when the count was unstated,
+  while a hand-built definition passed straight to the engine retried `3` — the
+  schema and the engine each carried a default and they disagreed
+  ([#4247](https://github.com/objectstack-ai/objectstack/issues/4247)). The
+  engine's copy is gone; the schema is the only source, and the case that was
+  ambiguous is now rejected instead of guessed.
+</Callout>
+
 ## Discovery & Registration
 
 You almost never call `engine.registerFlow()` directly. The

content/docs/releases/v17.mdx

@@ -930,6 +930,17 @@ from `@objectstack/spec` directly.
 - **`sys_view_definition`'s all-six `apiMethods` whitelist is dropped** (#3026).
 - **`os migrate plan` shows index drift** — index DDL is no longer applied
   silently at boot (#3728).
+- **A flow's `errorHandling.strategy: 'retry'` must state `maxRetries` (>= 1)**
+  (#4247). `maxRetries` had two defaults — `.default(0)` in `FlowSchema` and
+  `?? 3` in the engine's `retryExecution` — so an unstated count retried 0 times
+  for a flow that had been through the schema and 3 times for a definition
+  handed to the engine directly. The engine's copy is gone (it reads the parsed
+  block with no fallback), and the case that was ambiguous is rejected rather
+  than guessed: retrying zero times is `strategy: 'fail'` under another name,
+  and a retry re-runs the *whole* flow, so the count is the author's to state.
+  Fix: write `{ strategy: 'retry', maxRetries: 3 }`, or `strategy: 'fail'` if no
+  retry was intended. `maxRetries: 0` stays legal under `'fail'` / `'continue'`,
+  which never read it.
 
 ## New capabilities in 17.0.0
 

docs/protocol-upgrade-guide.md

@@ -142,6 +142,8 @@ On the wire contract it also retires the `/analytics/query` request ENVELOPE (#3
 
 The close-out sweep finishes the enforce-or-remove worklist across the remaining types: action `shortcut`/`bulkEnabled` (no keydown path; the multi-select toolbar reads the view's bulkActions), flow `active`/`template`/node `outputSchema`/errorHandling `fallbackNodeId` (`active: false` never stopped a flow — `status` is the enforced lifecycle; faults route via per-node fault edges), the inert view keys (list `responsive`/`performance`, form `data`/`defaultSort`/`aria` — list aria/data stay live), dashboard and widget `aria`/`performance`, `agent.knowledge` (declaring sources never scoped retrieval — absorbs the former topics→sources rename), and `skill.triggerPhrases` (phrases were never matched; routing is triggerConditions + the agent allowlist). All pure lossless deletes, each tombstoned at its schema with the prescription.
 
+One flow key changes WITHOUT a lossless target: `errorHandling.maxRetries` (#4247). It carried two defaults — `.default(0)` in FlowSchema and `maxRetries ?? 3` in the engine's retryExecution — and because `??` fires only on `undefined`, an unstated count meant 0 retries for a flow parsed by the schema and 3 for a definition handed to the engine directly: the retry count was a function of the route in, not of the authored flow. The engine's copy is deleted (it reads the parsed block, no fallback), which makes an unstated count unambiguously 0 — and `strategy: 'retry'` that retries zero times is `strategy: 'fail'` under another name, the declared-not-delivered shape ADR-0049 exists to close. The schema therefore requires `maxRetries` at least 1 under `'retry'`, in both spellings (omitted, and an explicit 0). This is the one v17 flow change the chain cannot apply for you: choosing the count is a judgment about re-running the WHOLE flow with its side effects, so it is a semantic TODO rather than a rewrite.
+
 ### Mechanical (applied for you)
 
 | Conversion | Surface | Change | Load window |
@@ -169,6 +171,9 @@ The close-out sweep finishes the enforce-or-remove worklist across the remaining
 
 ### Semantic (delegated to you, with acceptance criteria)
 
+- **`flow-retry-max-retries-required`** — `flow.errorHandling.maxRetries (under strategy: 'retry')` → an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'
+  - Why not automatic: maxRetries had two defaults — FlowSchema `.default(0)` and the engine's `maxRetries ?? 3` — so an unstated count retried 0 times through the schema and 3 times through a hand-built definition (#4247). With the engine's copy removed the unstated count is unambiguously 0, and retrying zero times is exactly `strategy: 'fail'`, so the schema now refuses the combination instead of it silently doing nothing. There is no lossless rewrite: 0 preserves the behaviour a parsed flow got but contradicts what its author wrote, and any positive count is a NEW decision about re-running the whole flow with its side effects. That choice is the author's.
+  - Done when: Every flow declaring `errorHandling.strategy: 'retry'` also declares `maxRetries` >= 1, and each count was chosen knowing a retry replays the flow FROM THE START (records re-created, callouts re-fired); flows that never actually wanted retries say `strategy: 'fail'`. No flow fails to register with the maxRetries prescription.
 - **`analytics-query-request-envelope-retired`** — `api.analyticsQueryRequest.query` → bare AnalyticsQuery body (top-level cube/measures/dimensions/where/...)
   - Why not automatic: The { cube, query: {...} } envelope was an HTTP-wire dialect of the retired degraded analytics shim (#3891), never stored in stack metadata — there is no source for the chain to rewrite. Callers of POST /analytics/query and /analytics/sql must move the query.* fields to the body top level themselves.
   - Done when: Every /analytics/query and /analytics/sql call sends the bare AnalyticsQuery shape and succeeds; no request answers 400 VALIDATION_FAILED with the envelope prescription.

packages/cli/test/migrate-meta.e2e.test.ts

@@ -62,7 +62,10 @@ export default {
     type: 'autolaunched',
     active: false,                  // 16: removed (status is the lifecycle)
     template: true,                 // 16: removed (no reader)
-    errorHandling: { strategy: 'retry', fallbackNodeId: 'n9' },  // 16: fault edges own this
+    // 16: fallbackNodeId removed (fault edges own this). maxRetries is stated
+    // because #4247 refuses a zero-attempt 'retry' — the count is the one v17
+    // flow change the chain surfaces as a semantic TODO instead of rewriting.
+    errorHandling: { strategy: 'retry', maxRetries: 2, fallbackNodeId: 'n9' },
     nodes: [
       { id: 'n1', type: 'start', label: 'Start', outputSchema: { ok: { type: 'boolean' } } },  // 16: removed
       { id: 'n2', type: 'delete_record', label: 'Purge', config: { objectName: 'e2e_ticket', filter: { done: true } } },  // canonical since protocol 11 — must pass through untouched

packages/services/service-automation/src/engine.ts

@@ -3485,24 +3485,35 @@ export class AutomationEngine implements IAutomationService {
     /**
      * Retry execution with exponential backoff, jitter, and recursive protection.
      * Uses an iterative loop with an internal retry flag to prevent recursive call stacking.
+     *
+     * Reads the PARSED `errorHandling` block straight — no `??` fallbacks
+     * (#4247). It used to declare every knob optional and re-state a default
+     * for each, and one of those copies disagreed with the schema
+     * (`maxRetries ?? 3` against `.default(0)`), which made the retry count a
+     * function of how the flow reached the engine rather than of what its
+     * author wrote. `this.flows` only ever holds `FlowSchema.parse` output —
+     * `registerFlow` parses, and the version-history rollback re-seats a
+     * previously parsed snapshot — so every field below is present, and typing
+     * the parameter as the parsed shape is what keeps a second set of defaults
+     * from growing back here: a knob the spec stops defaulting becomes a
+     * compile error, not a silent engine-side guess.
      */
     private async retryExecution(
         flowName: string,
         context: AutomationContext | undefined,
         startTime: number,
-        errorHandling: {
-            maxRetries?: number;
-            retryDelayMs?: number;
-            backoffMultiplier?: number;
-            maxRetryDelayMs?: number;
-            jitter?: boolean;
-        },
+        errorHandling: NonNullable<FlowParsed['errorHandling']>,
     ): Promise<AutomationResult> {
-        const maxRetries = errorHandling.maxRetries ?? 3;
-        const baseDelay = errorHandling.retryDelayMs ?? 1000;
-        const multiplier = errorHandling.backoffMultiplier ?? 1;
-        const maxDelay = errorHandling.maxRetryDelayMs ?? 30000;
-        const useJitter = errorHandling.jitter ?? false;
+        // `maxRetries >= 1` is guaranteed under `strategy: 'retry'` — the schema
+        // refuses the zero-attempt spelling of "retry" (#4247), so reaching this
+        // method always means at least one re-run.
+        const {
+            maxRetries,
+            retryDelayMs: baseDelay,
+            backoffMultiplier: multiplier,
+            maxRetryDelayMs: maxDelay,
+            jitter: useJitter,
+        } = errorHandling;
 
         let lastError = 'Max retries exceeded';
         for (let i = 0; i < maxRetries; i++) {