fix(workflow): reset the whole recovery body and count onFail max as recoveries (#1541, #1542)

changelog.d/1526.added.md

@@ -1 +1 @@
-**Workflow failure recovery:** A node can now recover from failure by returning to an upstream node and retrying, instead of stopping the run. Define a failure recovery edge with `onFail: true` pointing from the failing node back to the recovery target, with a configurable `max` attempts bound (1–8, default 3). Example: `{ from: 'review', to: 'draft', onFail: true, max: 3 }` — when Review fails, the workflow resets to Draft and runs through Review again, up to 3 times total. After attempts are exhausted, the run fails cleanly. Does not interfere with existing `failOnce` retry or success-path `loop` edges.
+**Workflow failure recovery:** A node can now recover from failure by returning to an upstream node and retrying, instead of stopping the run. Define a failure recovery edge with `onFail: true` pointing from the failing node back to the recovery target, with a configurable `max` bound on recoveries (1–8, default 3). Example: `{ from: 'review', to: 'draft', onFail: true, max: 3 }` — when Review fails, the workflow resets the body from Draft through Review and runs it again, up to three recoveries. After the recoveries are used, the run fails cleanly. Does not interfere with existing `failOnce` retry or success-path `loop` edges.

changelog.d/1541.fixed.md

@@ -0,0 +1 @@
+The failure recovery edge added by #1529 reset only the recovery target and the failing node, so a node on the way back stayed `done` and the retry was fed the output it had just rejected. Recovery now resets every node from the target through the failing node and re-runs them in order. ([#1541](https://github.com/ThinkFlowLab/FlyBot/issues/1541))

changelog.d/1542.fixed.md

@@ -0,0 +1 @@
+The failure recovery added by #1529 accepted a `max` from 1 to 8 but never recovered on `max: 1`: the first failure already read as the limit, so the run failed at once and the attempt counter stayed empty. `max` now counts recoveries, so `max: 1` recovers once and `max: 8` still means at most eight. ([#1542](https://github.com/ThinkFlowLab/FlyBot/issues/1542))

docs/subsystems/workflow.md

@@ -27,7 +27,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher,
 | File | Role |
 | --- | --- |
 | `plugins/workflow/index.js` | Plugin entry (`flybot-workflow`). Endpoints, `run_workflow`, `define_workflow`, the board listener, and the reply middleware that ingests a step result and denies delivery to persistent Bot chats. |
-| `plugins/workflow/src/graph.js` | Nodes, edges, positions, and named inputs and outputs. A node starts when its forward predecessors are settled and a taken edge reaches it. Same-rank nodes run together. A forward cycle is refused. A loop edge is the only cycle for success paths. A `when` edge chooses a branch; the other is skipped. A required input with no upstream provider is refused before a run starts. An `onFail` edge provides failure recovery: when a node fails with recovery attempts remaining, the workflow resets to the upstream target and re-runs through the failing node (tracked by `failAttempts` per edge, with a configurable `max` bound, 1–8, default 3). After attempts are exhausted, the node fails permanently. |
+| `plugins/workflow/src/graph.js` | Nodes, edges, positions, and named inputs and outputs. A node starts when its forward predecessors are settled and a taken edge reaches it. Same-rank nodes run together. A forward cycle is refused. A loop edge is the only cycle for success paths. A `when` edge chooses a branch; the other is skipped. A required input with no upstream provider is refused before a run starts. An `onFail` edge provides failure recovery: when a node fails with recoveries remaining, the workflow resets the body from the upstream target through the failing node and re-runs it in order (tracked by `failAttempts` per edge, with `max` counting recoveries, 1–8, default 3). After the recoveries are used, the node fails permanently. |
 | `plugins/workflow/src/service.js` | The store, lanes (active runs, FIFO queue, terminal history), `maxConcurrent`, `rename`, `delete`, `saveGraph`, `define`, `runNamed`, `ingest`, `stepResult`. `rename` updates a workflow's name and optional description; `delete` removes the workflow, releases ephemeral Bots, and cleans logs/runs/routes. Factory template ids (e.g. `delivery`) are normal saved rows and may be deleted. A spawn hook makes one run Bot per node. Disposing the plugin ends every live run still marked `running` (nodes in flight are failed, the run is archived) and then releases every run Bot and drops those ids from the saved record only once the release has finished, handing the release back as a promise the host awaits, so a shutdown cannot persist a run as `running` with no ephemeral map and cannot leave hidden Bots that no state names (#1387, #1414). A released run Bot also gives its runner name back to the allocator instead of keeping it for the lifetime of the process. A dispatch that does not land is not marked handed. |
 | `plugins/workflow/src/ephemeral.js` | Builds each node's run Bot from its prompt, role, model and tools, and releases the ids the host returns. The `tools` field is **always omitted** from `fb.bot.create` so every node Bot inherits all registered tools (GitHub, Composio, file/shell tools, and team tools limited by `EPHEMERAL_DENIED_TOOLS` at the guard layer). `node.tools` is stored as documentation only in `fields.tools` but does not restrict the grant. All node Bots get `danger-full-access` permission. |
 | `plugins/workflow/src/match.js` | Which open card matches a Workflow or a local Bot. |
@@ -50,7 +50,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher,
 4. Each workflow keeps a FIFO queue and up to `maxConcurrent` live runs (default 2, an integer from 1 to 8 on the record, via `workflow/concurrency`, or in Settings → Workflow graphs). A task that does not fit is enqueued. When a run finishes, every workflow with a free slot is drained, oldest enqueue first. A new assign on a workflow that already has a queue waits behind it. A task does not replace a running run or release that run's Bots. Migrating an old store keeps only runs that were still `running` with ephemeral Bot ids; other legacy scripted rows are dropped from active state (finished tasks may remain in lane history). History records a finished or failed live run. Lowering the cap does not stop a run that has already started. Assigning `{ botId }` claims the card for that local Bot and does not enqueue. A remote Bot is refused. `activeCount` is `runningOf(lane)` — live runs with `live === true` and `status === 'running'`. The stage aggregate chip and the Runs tab (运行记录) must agree: every such run appears as its own running row and progress card, and the chip count does not exceed that list.
 5. A node declares up to eight inputs and eight outputs. A required input is an output of the same name on an ancestor, or `task`, `title`, or `brief` on a root. A start with no provider is refused, and Bots spawned for that attempt are released. On a Bot-backed run a successor stays queued until its required inputs are present. If every forward predecessor is done, failed or skipped and an input is still missing, that node fails and the run stops. The dispatch text carries `Input <name>: <value>`. The Bot reports `OUTPUT <name>: <value>`; a single declared output may be the reply body. Nodes start in edge order.
 6. Each node is a real ephemeral Bot for that run, then it is removed. A permanent sidebar Bot (`botId`) is optional and is not required for the step to run. **Every node Bot gets the same full permissions a normal sidebar Bot gets:** the `tools` field is always omitted from `fb.bot.create`, letting the Bot inherit all registered tools — file/shell tools, GitHub (`mcp__github__*`: pull requests, reviews, comments), Composio, and team tools — limited only by `EPHEMERAL_DENIED_TOOLS` at the guard/approval layer (#1466, 29 team-admin tools). `node.tools` is stored as documentation in `fields.tools` but does not restrict the grant. All node Bots get `danger-full-access` permission. A hidden run Bot cannot show an approval card, so a granted tool is allowed and a connector it did not name is refused. The host stores this on the run Bot as `stepTools` and `permission`.
-7. A loop edge (`loop: true`, `max` from 1 to 8, default 3, optional `until: { output, eq }`) is the only cycle for success paths. The body runs again until `until` matches or `max` runs have finished, and the forward edges wait until then. Restarting the body drops the branch choice made inside it and queues a sibling that the previous pass skipped, so the next pass can take the other side. A `when: { output, eq }` edge is taken only when that output equals `eq`. `when: { else: true }` is taken only when no other `when` matched. A node whose forward edges are all closed is `skipped` and does not block a join. A run cannot spin: every loop has a max. An `onFail` edge (`onFail: true`, `max` from 1 to 8, default 3) provides failure recovery: when a node fails, if an `onFail` edge points from that node back to an upstream target and recovery attempts remain (tracked in `failAttempts` per edge), the workflow resets both nodes to their pre-failure states (the target to `running`, the failing node to `queued`) and re-runs from the target through the failing node. After `max` attempts are exhausted, the node fails permanently and the run stops. An `onFail` edge does not interfere with `failOnce` retry (which restarts the same node once) or success-path `loop` edges. Settings → Workflow graphs can add a link, a condition, a loop, or a failure recovery edge.
+7. A loop edge (`loop: true`, `max` from 1 to 8, default 3, optional `until: { output, eq }`) is the only cycle for success paths. The body runs again until `until` matches or `max` runs have finished, and the forward edges wait until then. Restarting the body drops the branch choice made inside it and queues a sibling that the previous pass skipped, so the next pass can take the other side. A `when: { output, eq }` edge is taken only when that output equals `eq`. `when: { else: true }` is taken only when no other `when` matched. A node whose forward edges are all closed is `skipped` and does not block a join. A run cannot spin: every loop has a max. An `onFail` edge (`onFail: true`, `max` recoveries from 1 to 8, default 3) provides failure recovery: when a node fails, if an `onFail` edge points from that node back to an upstream target and recoveries remain (tracked in `failAttempts` per edge), the workflow resets every node from the recovery target through the failing node (the target to `running`, the rest to `queued`) and re-runs them in order. After `max` recoveries are used, the node fails permanently and the run stops. An `onFail` edge does not interfere with `failOnce` retry (which restarts the same node once) or success-path `loop` edges. Settings → Workflow graphs can add a link, a condition, or a loop; a failure recovery edge is added through `define_workflow` (`onFail: true`) and has no Settings control yet.
 8. Auto-assign considers an `open` card only. A saved Bot claim beats a workflow trigger. Otherwise first hit wins: a `workflow:` token, then `workflowId` / `workflow`, then an assignee that names a workflow, then a `bot:` token, then an assignee that names a local Bot, then the longest trigger contained in the title, note or criteria. A miss returns no route. A second ingest of the same task and target is a no-op, and a live run for that task id is not restarted.
 9. The browser draws text only. Names and messages are React text children. The unit renderer asserts a hostile name is not markup.
 10. `enabled: false` registers no endpoint, tool or listener and writes nothing.

plugins/workflow/README.md

@@ -42,9 +42,9 @@ An edge may be a condition, a loop, or a failure recovery. A condition is `when:
 
 A loop is `loop: true` on the edge that returns to the body, with `max` from 1 to 8 (default 3) and an optional `until: { output, eq }`. The body runs again while `until` does not match and the run count is still under `max`. A branch the previous pass skipped can be chosen the next time. The forward edges wait until the loop stops. A loop is the only cycle the graph allows for success paths.
 
-A failure recovery is `onFail: true` on an edge pointing from the failing node back to an upstream node, 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. Failure recovery edges do not interfere with success-path loops or `failOnce` retry behavior.
+A failure recovery is `onFail: true` on an edge pointing from the failing node back to an upstream node, with `max` from 1 to 8 recoveries (default 3). When the node fails and fewer than `max` recoveries have run, the workflow resets the body from the recovery target through the failing node and runs it again. After `max` recoveries, the node fails permanently and the run stops. Failure recovery edges do not interfere with success-path loops or `failOnce` retry behavior.
 
-Settings → Workflow graphs can add a link, a condition, a loop, or a failure recovery. After a live run finishes, the stage keeps showing that run until a new one starts.
+Settings → Workflow graphs can add a link, a condition, or a loop. A failure recovery edge is added through `define_workflow` (`onFail: true`); the Settings canvas has no control for it yet. After a live run finishes, the stage keeps showing that run until a new one starts.
 
 ## When a step does not land
 
@@ -57,7 +57,7 @@ A reply that says the model could not answer (`could not answer your message` or
 A local Bot uses the same store:
 
 - `run_workflow` — `workflow` (id or exact name) and `brief` (the task). Optional `task` id. This joins the same queue a manual assign uses.
-- `define_workflow` — `name`, `nodes` (`{ id, name, prompt?, role?, model?, tools?, botId?, inputs?, outputs?, x?, y? }`), `edges` (`{ from, to, when?, loop?, max?, until? }`). `prompt` is that node's Bot system prompt. `tools` equips the ephemeral Bot (`github`, `bash`, `read`, `edit`). `botId` is optional. `inputs` and `outputs` are `{ name, type?, required? }`. `when` chooses an edge; `loop: true` with `max` is the only cycle. Pass `workflow` to replace one. A friend's Bot is refused. This writes the same graph Settings and the flowchart edit.
+- `define_workflow` — `name`, `nodes` (`{ id, name, prompt?, role?, model?, tools?, botId?, inputs?, outputs?, x?, y? }`), `edges` (`{ from, to, when?, loop?, max?, until?, onFail? }`). `prompt` is that node's Bot system prompt. `tools` equips the ephemeral Bot (`github`, `bash`, `read`, `edit`). `botId` is optional. `inputs` and `outputs` are `{ name, type?, required? }`. `when` chooses an edge; `loop: true` with `max` is the only cycle. Pass `workflow` to replace one. A friend's Bot is refused. This writes the same graph Settings and the flowchart edit.
 
 ## Install
 

plugins/workflow/src/graph.js

@@ -542,13 +542,14 @@
   return null
 }
 
-// Check if a failed node has an onFail edge and attempts remaining.
+// Check if a failed node has an onFail edge and recoveries left. `max` counts
+// recoveries, unlike a loop edge's max which counts how many times its body runs.
 function failRecoveryDecision(graph, nodeId, failAttempts) {
   const edges = graph.edges.filter(edge => edge.onFail === true && edge.from === nodeId)
   for (const edge of edges) {
     const ran = (Number(failAttempts?.[edgeKey(edge)]) || 0) + 1
     const max = Number.isInteger(edge.max) ? edge.max : 3
-    if (ran >= max) continue
+    if (ran > max) continue
     return { edge, ran }
   }
   return null
@@ -622,8 +623,8 @@
   const index = layerIndex(graph, nodeId)
   const layer = graph.layers[index]
   const outputs = ctx.outputs && typeof ctx.outputs === 'object' ? ctx.outputs : {}
   const loops = { ...(ctx.loops ?? {}) }
   const fired = { ...(ctx.fired ?? {}) }
 
   // Check if this completion should end the run early (#1537, #1538)
   const earlyEnd = checkEarlyEnd(outputs, ctx.text)
@@ -691,13 +692,13 @@
   if (!nodeById(graph, nodeId)) throw new Error(`Unknown Bot ${nodeId}`)
   if (status[nodeId] !== 'running') throw new Error(`${nodeId} is not running`)
 
   const failAttempts = { ...(ctx.failAttempts ?? {}) }
   const recovery = failRecoveryDecision(graph, nodeId, failAttempts)
 
   if (recovery) {
     // Failure recovery: reset to the upstream target node
     failAttempts[edgeKey(recovery.edge)] = recovery.ran
-    const body = loopBody(graph, recovery.edge.to, nodeId)
+    const body = loopBody(graph, nodeId, recovery.edge.to)
     const next = copyStatus(status)
     const reset = []
 

plugins/workflow/test/failure-recovery-attempts.test.js

@@ -0,0 +1,64 @@
+// #1542: an onFail edge's `max` counts recoveries. max: 1 must recover once;
+// the run fails permanently only after those recoveries are used.
+
+import { describe, it } from 'node:test'
+import assert from 'node:assert/strict'
+import { failNode, normalizeGraph } from '../src/graph.js'
+
+function chain(max) {
+  return normalizeGraph({
+    nodes: [
+      { id: 'draft', name: 'Draft' },
+      { id: 'copy', name: 'Copy' },
+      { id: 'review', name: 'Review' },
+    ],
+    edges: [
+      { from: 'draft', to: 'copy' },
+      { from: 'copy', to: 'review' },
+      { from: 'review', to: 'draft', onFail: true, max },
+    ],
+  })
+}
+
+// The state just before Review fails on a three-node chain. It is rebuilt for
+// each attempt so this test does not depend on how the recovered body is reset.
+function reviewRunning() {
+  return { draft: 'done', copy: 'done', review: 'running' }
+}
+
+describe('Failure recovery attempts', () => {
+  it('max: 1 recovers on the first failure and records one attempt', () => {
+    const graph = chain(1)
+    const result = failNode(graph, reviewRunning(), 'review', { failAttempts: {} })
+
+    assert.equal(result.recovered, true)
+    assert.equal(result.failAttempts['review>draft'], 1)
+    assert.equal(result.status.draft, 'running')
+    assert.equal(result.status.review, 'queued')
+  })
+
+  it('max: 1 fails permanently on the next failure and keeps the count at max', () => {
+    const graph = chain(1)
+    const result = failNode(graph, reviewRunning(), 'review', { failAttempts: { 'review>draft': 1 } })
+
+    assert.equal(result.recovered, undefined)
+    assert.equal(result.status.review, 'failed')
+    assert.equal(result.failAttempts['review>draft'], 1)
+  })
+
+  it('max: 2 recovers twice and then fails permanently', () => {
+    const graph = chain(2)
+    const first = failNode(graph, reviewRunning(), 'review', { failAttempts: {} })
+    assert.equal(first.recovered, true)
+    assert.equal(first.failAttempts['review>draft'], 1)
+
+    const second = failNode(graph, reviewRunning(), 'review', { failAttempts: first.failAttempts })
+    assert.equal(second.recovered, true)
+    assert.equal(second.failAttempts['review>draft'], 2)
+
+    const third = failNode(graph, reviewRunning(), 'review', { failAttempts: second.failAttempts })
+    assert.equal(third.recovered, undefined)
+    assert.equal(third.status.review, 'failed')
+    assert.equal(third.failAttempts['review>draft'], 2)
+  })
+})