fix(workflows): stop masking blocked returned statuses as completed

[lavaman131]

Summary

Workflow-returned terminal statuses beyond failed/blocked (e.g. needs_human, incomplete, auth_blocked) were being rendered as successful completed runs. This let exhausted model/provider fallbacks in reviewer-gated workflows (like Goal) silently disappear behind a green completion notice instead of surfacing as a resumable, human-actionable blocked state. This PR normalizes returned-status handling across the run lifecycle and makes Goal's reviewer batch execution fail fast instead of burning another worker turn when reviewers can't run.

Changes

• src/shared/returned-run-status.ts (new): shared helpers — normalizeReturnedWorkflowStatus, isReturnedBlockedWorkflowStatus, isReturnedResumableBlockedWorkflowStatus, actionableReturnedStatusText (falls back through summary → remaining_work → result), and effectiveRunStatus (recomputes a run's true terminal status from its returned result.status, even when the stored run is completed).
• src/engine/run-returned-status.ts: classifyReturnedRunStatus now recognizes the full set of returned blocked statuses (blocked, needs_human, incomplete, active, auth_blocked), not just failed/blocked. Recoverable auth/provider/rate-limit blocked statuses are classified via classifyWorkflowFailure and marked resumable: true with preserved failure metadata (failureKind, failureCode, retryAfterMs, etc.).
• src/engine/run-durable-finalize.ts: durable status persistence no longer force-overrides resumable to false for blocked runs, so recoverable blocked metadata survives into durable storage.
• src/engine/run.ts: wires the updated returned-status classification into the run lifecycle.
• src/extension/lifecycle-notifications.ts: terminal notices use effectiveRunStatus and surface the actionable returned-status text as the notice error for blocked/failed returned states.
• src/runs/background/status.ts: statusRuns/inspectRun report effectiveRunStatus instead of the raw stored status, and backfill error from the actionable returned-status text when the raw status doesn't match the effective one.
• src/tui/status-list.ts: /workflow status rendering (accent colors, trailing labels, card meta, stage cells, counts, badges, icons) is driven by effectiveRunStatus; adds a dedicated blocked bucket/badge distinct from completed.
• builtin/goal-runner.ts: reviewer batch execution now uses failFast: true; when the batch throws, the workflow no longer retries with another worker turn — it records a needs_human reducer decision with the collected remaining work, appends a status_decided lifecycle event, persists the ledger, and stops.
• Updated packages/coding-agent/docs/workflows.md's reserved status output convention and added a workflows changelog entry.

Tests

• test/unit/workflow-returned-status.test.ts — returned-status normalization/classification, including recoverable auth/provider/rate-limit blocked metadata.
• test/unit/background-status-01.test.ts — legacy completed snapshots with an incomplete returned status now report as blocked via statusRuns/inspectRun.
• test/unit/workflow-lifecycle-notifications-01.test.ts — lifecycle notices for returned incomplete/blocked statuses.
• test/unit/status-list-render.test.ts — /workflow status rendering of the new blocked bucket/badge.
• test/unit/builtin-workflows-goal-reviewer-failfast.test.ts — Goal reviewer-batch fail-fast behavior on fallback exhaustion.

Validation

• Pre-commit hooks passed during commit and push: bun run lint, bun run check:file-length, bun run test:unit.
• Also verified: targeted Bun tests, bun run typecheck, bun run lint, bun run check:file-length, git diff --check, and a terminal smoke test of /workflow status rendering.

Notes

• Provider-auth credential loading is intentionally out of scope; this fixes the workflow status/fail-fast contract when configured model/provider fallbacks are exhausted.
• Docs and tests use generic provider/auth failure shapes, not live credentials.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bastani	🟢 Ready	View Preview	Jul 9, 2026, 12:36 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Review: fix(workflows): block exhausted auth fallbacks

Reviewed the full diff. This is a well-scoped, well-tested fix. The core problem — workflow-defined incomplete reducer outputs (needs_human / incomplete / auth_blocked) being surfaced as green completed runs — is addressed cleanly by centralizing the classification in the new returned-run-status.ts helper and threading effectiveRunStatus through every read surface (status list, background status, lifecycle notices, TUI). Docs and changelog are updated per CLAUDE.md conventions. 👍

What I verified

• failFast: false -> true in goal-runner.ts is safe. I initially flagged this as a potential resilience regression (one reviewer failing aborting a reachable quorum), but the old failFast: false path already threw an AggregateError whenever any step failed (executor-direct-helpers.ts:153-158) and discarded surviving results — so there was never a "quorum from survivors" path. The change just fails fast instead of waiting for the other reviewers before throwing the same way. The real fix is the new reviewerBatchFailed branch that breaks to needs_human instead of looping into another worker turn. Correct.
• run-durable-finalize.ts resumable change is correct. Dropping the hard-coded status === "blocked" ? false and reading runSnapshot.resumable is consistent with the snapshot value set at run.ts:428. For ctx.exit blocked runs the snapshot resumable is false (executor-run-finalizers.ts:78), so no regression there; for returned-blocked auth exhaustion it now correctly preserves resumable: true. This actually fixes the finally-block overwriting a correct resumable value.
• Type soundness: effectiveRunStatus only ever returns "failed" | "blocked" | run.status, all valid RunStatus members, and only newly diverges from run.status when the stored status was completed. "complete"/"completed" short-circuit to completed correctly.

Discussion points (non-blocking)

1. Read-time derivation must be remembered by every new surface. The design derives the effective status at read time (intentionally, to normalize legacy completed snapshots that predate this fix). The trade-off is that any future read surface reading run.status directly will silently show a stale completed for a needs_human run. render-result.ts and the inspectRun paths look consistent today, but consider a short comment on RunSnapshot.status (or a lint note) pointing readers to effectiveRunStatus so this contract does not erode. Normalizing once at recordRunEnd write time is the alternative — but that loses the legacy-snapshot backfill, so read-time is the right call; just worth guarding.

2. "active" in RETURNED_BLOCKED_STATUSES. Treating a returned top-level status: "active" as a terminal blocked run is a little surprising for a word that reads like "in progress." It is fine for Goal reducer vocabulary and the docs warn against overloading top-level status, but a one-line comment on the set explaining why active counts as an incomplete terminal state would help the next reader.

3. Minor: double classification in returnedBlockedMetadata. For a plain blocked status it calls returnedRecoverableBlockedMetadata (which runs classifyWorkflowFailure) and then inspects failureKind; negligible cost, but the two-step "classify, then decide whether to keep it" reads slightly indirectly. Not worth changing unless you are already touching it.

Test coverage

Strong — regression tests cover the Goal-like needs_human auth-exhaustion path (resumable + auth metadata preserved), legacy completed snapshots with incomplete returned status rendering as blocked, blocked lifecycle notices, status-list badges/counts, and the reviewer fail-fast ledger sequence. builtin-workflows-goal-reviewer-failfast.test.ts asserting ctx.calls.task === ["work-turn-1"] nicely pins the "no extra worker turn" guarantee.

Note: I could not execute bun test in this sandbox (command blocked), so I am relying on the PR-reported passing runs plus static verification of the logic and assertions.

Overall: looks good to merge. The discussion points are all optional polish.

[claude]

Review: fix(workflows): block generic auth fallback exhaustion

Thanks for this — the core idea (don't let a run that stopped on recoverable auth/rate-limit/provider-exhaustion masquerade as completed) is a real correctness fix, and the change is well-tested across finalization, status rendering, lifecycle notices, and the Goal fail-fast path. Below is constructive feedback, roughly ordered by impact.

1. Text-mining generic output fields risks false positives (correctness)

classifyReturnedRecoverableFailure / effectiveRunStatus mine summary (unconditionally) and short result strings (<=500 chars, unconditionally) and feed them to classifyWorkflowFailure(new Error(text)). The underlying matcher (decisionFromMessageTokens) matches on fairly broad phrasing — e.g. RATE_LIMIT_PHRASES, tokenNearAny(tokens, "provider", ..., 8), tokenNearAny(tokens, "model", ..., 8).

That means a successful run whose summary/result legitimately describes provider/rate-limit work could be reclassified as blocked. Concrete example: a completed run returning { summary: "Implemented rate limiting for the model provider integration" }, or a research/report workflow returning a short result mentioning "the provider was unavailable during our test", would now render as blocked in /workflow status, the TUI, and lifecycle notices.

Given the PR explicitly targets runs that did not return a status field, consider tightening the heuristic to reduce collateral damage: only mine free-text fields when there is also a corroborating blocked/failed signal, gate summary mining the same way remaining_work/result are gated, or require a stronger match than the broad provider/model proximity rules for text pulled from success-shaped output. At minimum there is no negative test asserting a benign completed run stays completed — worth adding so future matcher tweaks don't silently widen the blast radius.

2. failFast: true on the reviewer batch is strictly less tolerant (behavior change)

In goal-runner.ts the reviewer ctx.parallel flips from failFast: false to failFast: true. Previously an individual reviewer failure came back as a per-reviewer error result and the other reviewers could still form quorum and complete the goal. Now the first reviewer to throw aborts the whole round and forces needs_human.

This is fine for the intended "all fallbacks exhausted / total auth outage" case, but it also converts a single transient reviewer failure (e.g. one reviewer's model hits a 429 after exhausting its own fallbacks, while the other two would have succeeded) into a hard needs_human stop. If the intent is specifically auth/provider exhaustion, consider gating the fail-fast on the classified failure kind rather than treating every reviewer exception as terminal — otherwise the Goal loop gets noticeably more fragile. Worth confirming this trade-off is deliberate.

3. Performance: effectiveRunStatus re-classifies on every call in the render hot path

effectiveRunStatus calls classifyReturnedRecoverableFailure, which constructs up to ~6 Error objects and runs full tokenization/regex classification. In status-list.ts it is invoked repeatedly per run per frame — runCardMeta alone calls it ~5x, plus runAccent, runTrailing, statusIconForRun, stageStatusFromRun, and countBuckets. For a TUI that re-renders frequently with many runs, that is a lot of redundant regex work each frame.

Recommend computing effectiveRunStatus(run) once per run (hoist to the top of renderRunEntry/countBuckets and thread the value down) so classification runs O(runs) per frame rather than O(runs x ~10).

4. Two parallel reimplementations of the same status derivation (maintainability)

classifyReturnedRunStatus (engine/run-returned-status.ts, finalization) and effectiveRunStatus (shared/returned-run-status.ts, display) independently re-derive the effective status with subtly different fallbacks (the former returns "completed" for unknown non-blocked statuses; the latter returns run.status). They agree today, but can drift. Consider having the finalizer build on the shared helper so there is a single source of truth.

5. Minor

• Precedence: in classifyReturnedRunStatus, recoverableFailure is checked before returnedStatus === "failed", so { status: "failed", error: "rate limited" } is downgraded from failed to blocked. Probably desirable (resumable), but a non-obvious precedence worth a comment.
• Naming: returnedRecoverableBlockedMetadata stamps failureRecoverability: "recoverable" / failureDisposition: "active_blocked" even when the text is not recognizably recoverable (the needs_human fallback path), making the label semantically inaccurate; a name/comment clarifying "always-resumable blocked metadata" would help.
• "active" in RETURNED_BLOCKED_STATUSES: a workflow returning a domain-ish status: "active" is now forced to blocked. The docs warn against domain status values, so likely acceptable, but it is the most surprising member of the set.
• goal-runner.ts is now 479 lines — close to the 500-line check:file-length gate. Not a blocker, just flagging headroom.

Nice touches

• Preserving resumable metadata for blocked auth runs (dropping the status === "blocked" ? false override in run-durable-finalize.ts) is the right call.
• Good regression coverage on the Goal fail-fast ledger/lifecycle sequence and the legacy-completed-snapshot normalization.
• Docs + changelog updates are thorough.

Note: I could not run bun test/bun run typecheck in this environment (command approval unavailable), so the above is static analysis — please rely on CI for the green checks listed in the PR description.

[claude]

PR Review: fix(workflows): block structured auth fallback exhaustion

Thanks for this — the intent (stop recoverable auth / rate-limit / provider-exhaustion runs from masquerading as completed, and keep them resumable) is clearly valuable, and the change is well-tested across finalization, status rendering, lifecycle notices, and Goal fail-fast. The extraction into shared/returned-run-status.ts is clean and reused consistently. A few things worth a look before merge.

1. structuredRecoverableWorkflowFailure can mark a genuinely completed run as blocked (potential false positive)

structuredRecoverableWorkflowFailure() scans every stage in the run (packages/workflows/src/shared/returned-run-status.ts:51) and, if any stage carries failureDisposition: "active_blocked" + failureRecoverability: "recoverable" + an auth/rate_limit/provider kind, reclassifies the whole run as blocked — regardless of whether the run actually completed successfully by tolerating that stage failure.

applyFailureToStage() (executor-lifecycle.ts:103) stamps that disposition on any failed stage independent of failFast. So a workflow that runs ctx.parallel([...], { failFast: false }) and completes fine on the surviving branches will still have the failed branch's snapshot present — and now be reported as blocked.

This is concretely reachable in-repo: packages/workflows/builtin/ralph-runner.ts:268 still uses failFast: false for its three reviewers. If one reviewer hits recoverable auth/provider exhaustion in a round but Ralph later reaches approval and creates the PR, the run legitimately completed — yet structuredRecoverableWorkflowFailure will find the failed reviewer stage and surface the run as blocked. Interestingly this PR fixed Goal by switching it to failFast: true (goal-runner.ts:284), but Ralph was left on the old semantics, so the two builtins now behave inconsistently under the new classifier.

Worth considering: scope the scan to the run's actual terminal/blocking stage (e.g. run.failedStageId) rather than "any failed stage anywhere", or gate the reclassification so a run that produced a valid declared output isn't overridden — and either align Ralph to failFast: true or add coverage for the tolerated-partial-failure case. There's currently no test asserting that a completed run containing a tolerated recoverable stage failure stays completed, which is exactly the boundary that could regress.

2. Goal failFast: false → true trades resilience for fast-fail

Flipping the reviewer batch to failFast: true (goal-runner.ts:284) means any single reviewer execution error — including a transient, non-auth blip — now aborts the whole batch and forces needs_human, where previously quorum could still be reached from the other two reviewers. That's a reasonable and more consistent choice given the rest of the change, but it does make the reviewer step more brittle to one-off flakes. Worth confirming this is the intended tradeoff (the fallback models are already exhausted internally before a throw, so in practice most throws are genuine exhaustion — a sentence to that effect in the changelog/docs would help future readers).

3. Minor / nits

• "active" as a blocked terminal status (returned-run-status.ts:9): treating a returned status: "active" as blocked+resumable reads oddly, since "active" semantically implies still-running. If a workflow ever returns it as a domain value it'd be misclassified. Low risk given the docs discourage domain use of status, but the naming is surprising.
• Broadened matching: normalizeReturnedWorkflowStatus now lowercases + collapses punctuation, so "Blocked", "needs-human", etc. now trigger the reserved-status path where the old exact === "failed"/"blocked" check did not. This is a behavior change (generally an improvement) but worth a docs note that the match is now fuzzy.
• Repeated recomputation: effectiveRunStatus() (and via it structuredRecoverableWorkflowFailure, which allocates [...run.stages].reverse() each call) is invoked several times per run inside runCardMeta/countBuckets/inspectRun. Fine for TUI scale, but computing it once per run and threading the value would avoid the repeated reversed-array allocation on every render.

What looks good

• The run-durable-finalize.ts change (dropping the forced resumable = false for blocked) is the right fix and is covered by the resumable/non-resumable blocked tests.
• Clear separation of "reserved status convention" vs "structured stage failure", and the explicit choice not to scan arbitrary output text is well-documented in workflows.md.
• Good regression coverage for legacy completed snapshots, lifecycle notices, and the Goal fail-fast ledger sequence.

Overall solid; item #1 (the Ralph / tolerated-partial-failure false positive) is the one I'd most want addressed or explicitly ruled out before merge.

Reviewed by Claude Code (Opus 4.8).

[lavaman131]

@/tmp/tmp.OeJE807R7p

[claude]

Review: fix(workflows): stop masking blocked returned statuses as completed

Solid, well-tested fix. Centralizing returned-status/structured-failure classification into returned-run-status.ts and routing every surface (finalization, /workflow status, lifecycle notices, durable state, background status, TUI) through effectiveRunStatus/structuredRecoverableWorkflowFailure is the right shape and removes the previous ad-hoc run.result?.["status"] checks scattered across call sites. Test coverage is genuinely good: legacy snapshots, tolerated-vs-blocking branch failures, resumable metadata, and lifecycle rendering are all exercised. A few things worth a second look before merge.

1. failFast: true on reviewers is a broader change than "auth fallback exhaustion" (confirm intent)

packages/workflows/builtin/goal-runner.ts:284 flips the reviewer batch from failFast: false -> failFast: true. With 3 reviewers and DEFAULT_REVIEW_QUORUM = 2, the previous behavior tolerated a single reviewer failure — the other two could still form quorum and complete the goal. Now any single reviewer that throws aborts the whole batch and stops as needs_human.

Two consequences:

• Resilience regression for partial failures. A single provider hiccup / rate-limit-after-fallback / auth gap on one of three reviewers now halts the entire goal, even when the other two reached consensus. Defensible ("can't trust quorum if a reviewer is missing"), but it's a real fault-tolerance change that goes beyond the stated "fallback exhaustion" scope, since failFast fires on any thrown reviewer error, not only recoverable-provider exhaustion.
• Successful reviewer results are discarded. The catch at goal-runner.ts:286-295 replaces reviewResults entirely with a single reviewer-error record, so reviewers that did complete before the abort are thrown away (the new test asserts ledger.reviews.length === 1). That loses diagnostic signal — the ledger can no longer show quorum was nearly reached, and the surviving reviewers' findings/remaining-work are lost.

If the intent is specifically to short-circuit on recoverable provider/auth exhaustion, consider either (a) keeping failFast: false and detecting the recoverable-exhaustion condition from the collected results, or (b) preserving the completed reviewers' records in the needs_human ledger even on abort. If the intent really is "any reviewer failure => human", that's fine — just worth making explicit, since it's the most consequential behavior change here and isn't called out as such in the summary.

2. Structured recoverable failure takes precedence over an explicit status: "failed"

In classifyReturnedRunStatus and effectiveRunStatus, the structured-failure check runs before the returned status check. So a workflow that returns { status: "failed" } while the run/blocking-stage also carries recoverable auth/rate-limit metadata is downgraded to blocked + resumable: true rather than failed. Probably the desired outcome (recoverable => resumable beats a hard fail), but it means an author's explicit failed intent can be silently overridden. Worth confirming the ordering is deliberate.

3. "active" in RETURNED_BLOCKED_STATUSES is semantically surprising

returned-run-status.ts treats a top-level status: "active" as a blocked terminal state. "active" reads as in-progress, not blocked, so a workflow returning status: "active" for unrelated domain state would be misclassified as blocked/resumable. The docs warn against overloading top-level status and this is an intentional Goal-reducer convention — but of the five recognized values, active is the one most likely to collide with a legitimate domain meaning. Consider a less ambiguous token (e.g. in_progress).

Minor

• returned-run-status.ts: double blank line before stringResultField — trivial style nit.
• status.ts:482: the inline error: ternary (copy.error ?? (effectiveRunStatus(copy) === copy.status ? undefined : (... ?? ...))) is dense; a small named helper would read better. Not blocking.
• @ts-nocheck in the new builtin-workflows-goal-reviewer-failfast.test.ts is consistent with sibling builtin-workflow tests — no concern.

Overall a good fix for the "blocked masked as completed" problem. The main thing I'd want resolved before merge is confirming the failFast: true / discarded-reviewer-results tradeoff in #1 is intended.