fix(workflow): an onFail max of 1 never recovers, and recoveries come one short of max #1542

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment on this issue.

Written-by: FlyBot Lead
Claimed-by: FlyBot Lead
Claim-Id: 8ac9af64cd044b0f8c53a701873727e5
Branch: dev4/fix/p78-onfail-recovery-body
Base: a406fee
At: 2026-10-28T03:58:37Z

What happens

plugins/workflow/src/graph.js, failRecoveryDecision():

const ran = (Number(failAttempts?.[edgeKey(edge)]) || 0) + 1
const max = Number.isInteger(edge.max) ? edge.max : 3
if (ran >= max) continue

max is documented (README, "Failure recovery") as the number of recovery attempts: "with max from 1 to 8 (default 3). When the node fails and recovery attempts remain, the workflow resets to the recovery target and runs through the failing node again. After max recovery attempts, the node fails permanently and the run stops." cleanEdge() also accepts max: 1 for an onFail edge.

With ran >= max the number of recoveries is max - 1, and max: 1 is a dead value: the first failure is already 1 >= 1, the recovery edge is skipped, the node fails permanently on its first attempt, and failAttempts stays empty — indistinguishable, field by field, from a node that has no recovery edge at all.

Reproduction (measured on d6410d07)

Two-node graph draft → review, plus review → draft with { onFail: true, max }, failNode(review, { failAttempts: {} }):

• max: 1 → { recovered: false, status: { draft: "done", review: "failed" }, failAttempts: {} }
• max: 2 → { recovered: true, reset: ["draft","review"], failAttempts: { "review>draft": 1 } }

So max: 1 behaves like "no edge", while max: 2 gives one recovery — one fewer than the README promises.

Expected

max is the number of recoveries, so max: 1 recovers exactly once, max: 2 twice, and after the last recovery the next failure is permanent with failAttempts recorded at max.

Scope

• failRecoveryDecision() only: compare ran > max instead of ran >= max. One line.
• loopDecision() keeps ran >= max: for a loop edge max means total executions of the loop body (max: 1 runs it once), which is the correct semantics and stays untouched. A short comment records that the same field means different things on the two edge kinds.
• cleanEdge() keeps accepting 1–8 — max: 1 becomes meaningful instead of being rejected.
• Wording: the README sentence and changelog.d/1526.added.md ("up to 3 times total" → "up to three recoveries") are aligned with the code in the same change.
• Tests: max: 1 recovers once and records failAttempts = 1; after max recoveries the next failure is permanent and failAttempts stops at max; a max: 2 counting case. Each must fail on the base tree.

Out of scope

The recovery body itself (failNode's loopBody argument order — a separate defect), the Settings authoring surface for onFail edges (none exists), and showing n/max in the run log / publicRun() (observability, separate request).