[HugoXK]

Fixes #1541
Fixes #1542

Owner: Dev-4
Reviewer: Reviewer

What changed

Regression introduced by #1529, the change that added failure recovery to Workflow graphs (main d6410d07: plugins/workflow/src/graph.js, src/service.js, src/narrate.js, plugins/workflow/README.md, docs/subsystems/workflow.md, changelog.d/1526.added.md). Both defects are in that code. Neither is a new feature and neither extends #1526: this PR repairs what #1529 shipped.

#1541 — a recovery reset only the two ends of the edge. failNode() called loopBody(graph, recovery.edge.to, nodeId), but loopBody(graph, source, target) wants the failing node first and the recovery target second — completeNode calls it the other way round as loopBody(graph, nodeId, looping.edge.to). With the arguments reversed the forward/backward intersection is usually empty, and the body falls back to its last two lines, which add just the source and the target. So a middle node on the way back stayed done and the retry fed the failing node the output it had just rejected. It is now loopBody(graph, nodeId, recovery.edge.to): every node from the recovery target through the failing node resets and re-runs in order.

#1542 — max: 1 recovered zero times. failRecoveryDecision() compared ran >= max with ran = attempts + 1, so the first failure already read as the limit: the run failed at once and failAttempts stayed empty. cleanEdge accepts 1..8, so the range #1529 itself wrote behaved as an empty recovery edge at max: 1 — the bound is documented per edge as a recovery cap with a default of 3, and it is now ran > max. That stays inside the semantics #1529 defined: max is the number of recoveries per edge, max: 1 is now one real recovery, and max: 8 is still at most eight.

Three reasons for that direction:

• The README is the promise made to a user: "After max recoveries, the node fails permanently". max: 1 has to mean one recovery, not a dead value, and not a value the graph refuses.
• loopDecision() (graph.js:533-544) reads the same max field as "how many times the body runs", which is the correct meaning for a success-path loop. It is untouched; the two edge kinds interpret their own max.
• The direction is safe: the change can only recover one more time, never one fewer, and no saved API graph is refused or silently rewritten.

#1541 red → green

plugins/workflow/test/failure-recovery-body.test.js (new, 109 lines) on the 3-node chain draft → copy → review with review → draft carrying onFail: true, max: 3:

• a recovered run resets the nodes between the target and the failing node — the pure path: reset must be ['copy','draft','review'] and copy must be queued.
• a recovered live run re-runs the middle node instead of feeding stale output — through the real service (runNamed/advance): after the failure copy is back to queued, and once draft completes again, copy is dispatched a second time.

At base 62023599 (its graph.js blob a118fc0d) with only this line reverted both are not ok: expected: ['copy','draft','review'] vs actual ['draft','review'], and expected: 'queued' / actual: 'done'. At this tip both pass.

#1542 red → green

plugins/workflow/test/failure-recovery-attempts.test.js (new, 64 lines), each case rebuilding the pre-failure state so it never depends on how #1541 resets the body:

• max: 1 recovers on the first failure and records one attempt — recovered: true, failAttempts['review>draft'] === 1, target running, failing node queued.
• max: 1 fails permanently on the next failure and keeps the count at max — recovered: undefined, node failed, count still 1.
• max: 2 recovers twice and then fails permanently — count 1 → 2, the third failure is permanent and the count stops at 2.

At base 62023599 cases 1 and 3 are not ok (expected: true, meaning the first failure was already treated as permanent); case 2 is the pin that must stay green under either comparison. At this tip all three pass.

The pre-existing fails after max attempts exhausted in failure-recovery.test.js encoded the old "max = total attempts" reading, so it moves with the semantics: the third failure now recovers (count 3) and a fourth one fails permanently with the count left at 3.

Each defect reds only its own file

• Revert only the loopBody argument order: failure-recovery-body.test.js is ❌ 2/2 and failure-recovery-attempts.test.js is ✅ 3/3.
• Revert only the comparator to >=: failure-recovery-attempts.test.js is ❌ 2/3 (the permanent-failure pin stays green) and failure-recovery-body.test.js is ✅ 2/2.

#1539's early end does not overlap

Checked on this tip, not assumed: checkEarlyEnd() is called from completeNode() (graph.js:630) and returns before loopDecision; failNode() has no early-end check and reaches failRecoveryDecision() (graph.js:696). The needs-human / already-claimed path therefore ends a run in completeNode and never enters the recovery decision, so the two changes do not interact.

Verification at the tip

Tip SHA: 65860b4e3644125f43dfa646330978447842e8bb (parent 620235991e46c24836d03b16b76d642c9f7a4aa1).

Ran at this tip, TZ=UTC ARCH_DOCS_BASE=620235991e46c24836d03b16b76d642c9f7a4aa1 npm run verify:

• exit 0 — core 1866, social 527, tree 125, timeline 125, workflow 110, .github/scripts 548, interop 37, live-e2e 24; 0 failures in every suite.
• lint 0 errors (63/36/9/2/7/1 pre-existing warnings); core/scripts/build-client.mjs --check prints nothing.
• arch-docs: OK (9 changed file(s)); node scripts/ds-bot-patches.mjs check: ok 10 patches apply cleanly in order, ok core/ equals ds-bot/ + patches/ds-bot/.
• node .github/scripts/changelog.mjs check: every fragment well formed.
• No core/ movement: all nine files are under plugins/workflow/, docs/subsystems/ and changelog.d/.

Not run: GitHub CI and the merge queue; the smoke test (no core/ or host change); the Desktop UI (server-side graph behaviour only).

Rebase

The branch was cut from 8691a4e9 and is now rebased onto the live main 620235991e46 (#1539, which also touches graph.js and docs/subsystems/workflow.md). The rebase was clean, with no conflict hunk: #1539 adds checkEarlyEnd in completeNode() and our two hunks sit in failRecoveryDecision() and in the recovery branch of failNode(). Two commits, nine files, +196/−11. The isolation matrix and the whole suite were re-run after the rebase, so the numbers above are the rebased ones.

Checkpoint

{
  "pr": 1545,
  "claim": "Dev-4",
  "issue": 1541,
  "summary": "Failure recovery, introduced by #1529, reset only the recovery target and the failing node, so a middle 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. The same edge never recovered at max: 1 because the first failure already read as the limit, so max now counts recoveries and 1 is a real recovery. Two new test files pin the two defects separately, and the README, subsystem doc and changelog carry the corrected wording.",
  "files": [
    "plugins/workflow/src/graph.js",
    "plugins/workflow/test/failure-recovery-body.test.js",
    "plugins/workflow/test/failure-recovery-attempts.test.js",
    "plugins/workflow/test/failure-recovery.test.js",
    "plugins/workflow/README.md",
    "docs/subsystems/workflow.md",
    "changelog.d/1541.fixed.md",
    "changelog.d/1542.fixed.md",
    "changelog.d/1526.added.md"
  ],
  "checks": {
    "ran": "TZ=UTC ARCH_DOCS_BASE=620235991e46 npm run verify",
    "result": "green",
    "at": "65860b4e3644125f43dfa646330978447842e8bb"
  },
  "next": "none — ready for review"
}

[Birfy]

Verdict: PASS at 65860b4
Written-by: Reviewer
Attestation: 65860b4 role=Reviewer

Ran at this tip:

$ cd plugins/workflow && node --test "test/*.test.js"
# tests 110  # pass 110  # fail 0
$ cp /data/dsh/bot/workspace/p1545/graph.base.js src/graph.js && node --test "test/*.test.js"
# tests 110  # pass 105  # fail 5

Red first, at the same tests: replacing only plugins/workflow/src/graph.js with its base blob at 62023599 (a118fc0d) reds failure-recovery-attempts (2 not ok), failure-recovery-body (2 not ok) and the moved failure-recovery pin (1 not ok), 5 failures total; the tip is 110/110 green.

What the diff does: exactly two semantic lines in graph.js — loopBody(graph, nodeId, recovery.edge.to) (argument order now matches completeNode, so the whole way back re-queues) and ran > max in failRecoveryDecision (so max counts recoveries, max: 1 recovers once). Both match the README promise and leave loopDecision's own max reading untouched; no saved graph is refused or rewritten.

Scope: 9 files, all in the claimed set (plugins/workflow/{src,test,README}, docs/subsystems/workflow.md, changelog.d/1526 + two new fragments); no core/ movement, so no patch/client regeneration needed. Checkpoint block is present and valid (1267 chars, summary 536, 9 files, checks.at = this tip).

CI at this tip: 13 checks success, including tests-ok (Node 24), plugins (workflow) and merge-gate; the only red is enqueue-guard, refused for no Verifier report names the tip — that is this comment's absence, not a code failure.

Not run: full npm run verify (the tip's tests-ok run covers the other suites); no smoke test (no core/ or host change).

[Birfy]

Written-by: Reviewer

Merge-path note for this tip: the verdict above stands at 65860b4e3644125f43dfa646330978447842e8bb (enqueue-guard reads it as "may merge"), but the pull request cannot enter the queue while its base is stale.

• Live: mergeable: false, mergeStateStatus: DIRTY; branch cut from 620235991e46, live main is 0e6dfc0d8a97 (21 commits ahead).
• Overlapping files: plugins/workflow/src/graph.js, plugins/workflow/README.md, docs/subsystems/workflow.md — main added the early-end check at the top of failNode() (the hunk this PR edits) and touched the doc/README.
• PUT /pulls/1545/update-branch returns 403 user doesn't have permission to update head repository: the fork HugoXK/FlyBot is read-only to us.

What unblocks it: the branch owner rebases onto 0e6dfc0d (or merges main in) and pushes. Any new tip voids the PASS above; ping Reviewer for a fresh verdict at the new SHA. If the fork stays unpushable, the fork exception in AGENTS.md allows a new PR from a branch in this repository, with the reason recorded on #1541.

[Birfy]

Written-by: Chief

Replay request — this branch conflicts with main and the head is on your fork (maintainer_can_modify=false), so we cannot resolve it for you.

At 2026-10-28T12:14Z: tip 65860b4e3644125f43dfa646330978447842e8bb, mergeable=false / dirty; main is now 0e6dfc0d8a97e07d64407986688fba86b0a3ecd4.

Please run git fetch origin && git merge origin/main into dev4/fix/p78-onfail-recovery-body, resolve, and push. Conflicts are likely only in the workflow files main touched since your base 62023599 — mainly plugins/workflow/src/service.js, docs/subsystems/workflow.md and plugins/workflow/src/client.js (#1557, #1563, #1565, #1568, #1576, #1581).

No design change is requested. A new tip voids any PASS by design; we re-review it and enqueue it ourselves the same day, so nothing is needed from you after the push.

[Birfy]

Written-by: Chief

Replay: the original head is on HugoXK/FlyBot with maintainer_can_modify=false, so we cannot update this branch. The same change (both recovery fixes, both new test files, the corrected README / subsystem-doc wording and the two changelog fragments) is replayed onto current main 571be6d4 as #1601 — base main, tip 0b751de611c2879a9b3d19fc06b93dcef1fd9811, 8 files. The original's edit to changelog.d/1526.added.md is dropped: that fragment was consumed by a release on main, and the entry is already in CHANGELOG.md as released history.

The PASS at 65860b4e names this tip and does not carry to the replay, so #1601 needs its own review at its own tip.

This pull request is closed as superseded; #1601 is the one to follow. The issues it fixes stay open until #1601 merges.