feat(workflows): typed failure taxonomy with active-blocked lifecycle

[lavaman131]

Summary

Introduces a typed workflow failure taxonomy and an active-blocked lifecycle that keeps recoverable provider failures (rate limits, quota exhaustion, missing keys, login prompts) alive and resumable in-place, while terminating non-recoverable failures immediately. Hardens the failure classifier so bare string-only provider auth messages are correctly identified as terminal, and adds diagnostic-message and nested-error precedence to prevent weak wrapper status codes from masking decisive signals.

Closes #1269

Key Changes

Typed failure taxonomy (store-types.ts, workflow-failures.ts)

• Adds WorkflowFailureCode (10 codes: login_required, missing_api_key, invalid_api_key, forbidden_config, unknown_model, rate_limited, quota_limited, provider_unavailable, cancelled, unknown), WorkflowFailureRecoverability, and WorkflowFailureDisposition to the shared type surface
• Carries code, recoverability, disposition, and retryAfterMs through classification, stage/run snapshots, and all persistence paths
• Adds isWorkflowFailureCode, isWorkflowFailureRecoverability, isWorkflowFailureDisposition guard helpers for safe deserialization

Failure classifier hardening (workflow-failures.ts)

• Classifies string-only provider auth messages (Unauthorized, authentication required, API error (401)) as terminal invalid_api_key failures
• Preserves local login prompts as recoverable login_required active-blocked failures
• Prioritises structured signals and nested diagnostic messages over weak wrapper status codes; handles AggregateError so terminal failures beat recoverable blocked siblings
• Extracts retryAfterMs from Retry-After headers, retryAfterSeconds, and retryAfterMs fields across error shapes
• Adds internal WorkflowFailureClassification type with source and evidence fields for deterministic precedence resolution

Active-blocked lifecycle (executor.ts, store.ts)

• Recoverable failures (rate_limited, quota_limited, provider_unavailable, missing_api_key, login_required) leave the run running with blockedAt metadata via the new recordRunBlocked store API, rather than terminating
• Non-recoverable failures (invalid_api_key, forbidden_config, unknown_model, cancelled) terminate the run with killed/failed metadata as before
• clearStaleBlockedRunMetadata clears stale blocked state when a run is killed; sanitises terminal run-end persistence to prevent active-blocked metadata from leaking into terminal entries
• Active-blocked runs are reported as running in workflow details output

Persistence & session restore (persistence-session-entries.ts, persistence-restore.ts)

• Writes workflow.run.blocked journal entries with full failure taxonomy metadata
• On session restore, replays blocked runs as running and marks only descendant stages of the failed stage as blocked; unrelated in-flight stages remain running

Resume paths (extension/index.ts, extension/runtime.ts)

• Unifies resumable continuation detection via a shared isResumableContinuation check covering both terminal-failed resumable runs and active-blocked recoverable runs
• finalizeResumedActiveBlockedSourceRun closes out the source run as terminal_killed when a continuation run starts, preventing dangling active-blocked entries
• Updates slash-command and tool execution paths to use the unified check
• Adjusts resume messaging: "Resuming blocked workflow…" vs. "Resuming failed workflow…"

Tests

• Classifier (workflow-failures.test.ts, +30 cases): provider auth fallback, local login fallback, precedence, retry-after extraction, aggregate errors, redaction
• Persistence restore (persistence-restore.test.ts, +137 lines): blocked-run round trips, descendant-only blocked stage restoration
• Executor (executor.test.ts, +693 lines): active-blocked lifecycle, disposition routing, runFailureMetadata propagation
• Background status (background-status.test.ts, +62 lines), session entries (persistence-session-entries.test.ts, +118 lines), slash-command dispatch (slash-dispatch.test.ts, +123 lines), runtime resumability (runtime.test.ts), overlay graph (overlay-graph.test.ts)
• AGENT=1 bun test test/unit/workflow-failures.test.ts test/unit/persistence-restore.test.ts — 66 tests passed
• AGENT=1 bun run typecheck passed

[claude]

Review — fix(workflows): distinguish recoverable model failures

Thanks for this — the failure taxonomy (kind / code / recoverability / disposition) is a clean model, the classification is layered sensibly (structured signal → status → message-token fallback), and the test coverage is genuinely strong (classification matrix, executor lifecycle for all three dispositions, persistence round-trip, and restore). Most of my notes are about the blocked-run modeling and a couple of small cleanups, not the correctness of the classifier.

Design / maintainability

1. Blocked runs are modeled as status: "running", which is ambiguous. recordRunBlocked sets run.status = "running" and relies on the compound predicate endedAt === undefined && resumable === true && failureRecoverability === "recoverable" to mean "blocked." That predicate is now duplicated in four places:

   • extension/runtime.ts:398
   • runs/background/status.ts:271
   • extension/index.ts:1800 and :3142
   • (and a near-variant gates the executor at executor.ts:3580)

   Any consumer that lists/counts "running" workflows will treat a blocked run as actively executing, and the four hand-rolled copies can drift. Two suggestions, either helps: (a) extract a single isActiveBlockedResumable(run) helper and use it everywhere; and/or (b) consider whether a first-class "blocked" RunStatus models this more honestly than overloading "running". At minimum, the dedup is worth doing.

2. A blocked run never reaches a terminal state on its own. run() resolves with status: "running", the background runner unregisters it from the job tracker/cancellation registry on settle, but the store entry lingers as "running" indefinitely until the user resumes (which creates a new run) or kills it. This appears to be the intended "active, attach-only" design, but it's worth confirming there's a UI affordance for these — otherwise an abandoned blocked run is an invisible, permanently-"running" store entry. (killRun still works on it, which is good.)

3. onRunEnd does not fire for the blocked path. recordActiveBlockedFailure returns without calling opts.onRunEnd, whereas every other terminal path calls it. Please confirm no caller depends on onRunEnd as the single completion signal for a settled run() promise — for nested ctx.workflow(...) children especially, a child that resolves as "running"/blocked is an unusual state for a parent to observe.

Possible inconsistency

4. Message-only rate limits get a different disposition than structured ones. In decisionFromMessageTokens, a bare "rate limit" text match returns rateLimitDecision("rate_limited", …, "terminal_failed") (executor ends the run as failed), while a structured 429 / "too many requests" returns the default active_blocked. A rate limit is the same condition regardless of detection channel, so a provider that only emits text (no status code) ends the run terminally instead of blocking-and-resuming. If this asymmetry is deliberate (lower confidence on text-only matches), a one-line comment would help; otherwise consider active_blocked here too.

Minor / cleanup

5. Dead code in structuredDecision (workflow-failures.ts:680-681):

   if (hasStructuredDecisionSignal(signal)) return undefined;
   return undefined;

   Both branches return undefined, so the guard (and the otherwise-unused hasStructuredDecisionSignal helper) does nothing. Drop both lines, or implement whatever distinction was intended.

6. redactSensitiveText leaks the first 8 chars of an sk- secret. The pattern (sk-[A-Za-z0-9_-]{8})[A-Za-z0-9_-]+ preserves sk- + 8 characters in the sanitized userMessage. For terminal credential errors this path isn't hit (they use a static userMessage), but on the generic/unknown path the redacted message is workflow-facing. Consider redacting the entire token (sk-[redacted]) unless the prefix is genuinely needed for support diagnostics.

Nit

• runFailureMetadata's errorMessage: failedStage?.error ?? failure.userMessage reads fine, but note failedStage.error already holds the sanitized userMessage while failureMessage holds the raw text — the naming is easy to misread; a brief comment would help future readers.

Nothing here blocks the core fix, which looks solid and well-tested. Items (1) and (5) are the highest-value follow-ups.

Note: I was unable to run bun test in this environment (command approval), so I reviewed the test diffs statically rather than executing them.

[claude]

Review: failure codes, dispositions, and recoverable blocked runs

Reviewed the full diff (classification module, executor lifecycle, store, persistence/restore, background status, runtime, extension dispatch, and tests). This is a well-structured, carefully-typed change. The classification refactor from broad kind into a decision-based model with code/recoverability/disposition is clean, the snapshot/persistence/restore fields are threaded symmetrically, and test coverage is genuinely strong (redaction, retryAfterMs extraction, terminal-kill vs. active-blocked executor paths, run.blocked persistence + restore, runtime resume, and slash dispatch).

A few things worth a look before merge — mostly questions and polish, nothing blocking:

Correctness / design questions

1. Rate-limit disposition is inconsistent depending on detection path. In decisionFromMessageTokens, a message matching HTTP_RATE_LIMIT_PHRASES (429, "too many requests") returns active_blocked, but a message matching only RATE_LIMIT_PHRASES ("rate limit"/"rate limited") returns rateLimitDecision("rate_limited", ..., "terminal_failed") (workflow-failures.ts:626-628). The same underlying condition lands in different lifecycle states based on which phrase the provider used. This is deliberate and tested (workflow-failures.test.ts:238) — presumably a bare "rate limit" substring is a weaker signal you don't want pinning a run in active_blocked indefinitely. That rationale is non-obvious; a one-line comment at that branch would save the next reader the archaeology.

2. Recoverable aggregate failure with no failed stage degrades to terminal failed. In selectRunFailureDisposition, the recoverable-aggregate branch builds metadata via runFailureMetadataFromFailure(recoverableAggregateFailure, firstFailedStage), which only sets failedStageId when firstFailedStage !== undefined. The active_blocked branch in run() requires metadata.failedStageId !== undefined, so a recoverable aggregate error that produced no failed stage snapshot falls through to recordRunEnd("failed", ...) instead of recordRunBlocked. It stays resumable so it degrades gracefully, but won't be in the intended active-blocked state. Worth confirming this is acceptable (or whether it can even occur).

3. recordRunBlocked overloads status: "running". A blocked run is indistinguishable from an actively-executing run by status alone — consumers must additionally check endedAt === undefined && resumable === true && failureRecoverability === "recoverable" (this triple appears in extension/index.ts, runtime.ts, and background/status.ts). blockedAt already exists as a cleaner discriminator. Consider documenting this invariant in one place (the Store.recordRunBlocked doc comment is good but doesn't state "status stays running") so the call sites don't drift.

4. Blocked source runs appear to persist as running indefinitely. Resuming a blocked run creates a new run; the source's workflow.run.blocked entry has no terminal run.end, so on the next session it's re-restored as blocked. Over repeated resume cycles this could accumulate perpetually-"running" source runs in history. Is there a cleanup/supersede path, or is the source intended to be re-blocked each session?

Security

5. redactSensitiveText retains sk- + 8 characters (/(sk-[A-Za-z0-9_-]{8})[A-Za-z0-9_-]+/). That's a partial secret prefix in user-facing text. Probably an intentional tradeoff (identify which key), but 8 chars is more than needed for that. Also failure.message (the raw, unredacted text) is persisted as failureMessage / "preserved for diagnostics" — please double-check no UI path renders failureMessage to users, since only userMessage/error go through redaction.

Maintainability

6. Three near-identical metadata builders. runFailureMetadata, runFailureMetadataFromStage, and runFailureMetadataFromFailure share most of their field-merging logic with subtle differences (resumable source, stage-vs-failure precedence). Consolidating into one helper parameterized on the precedence source would reduce drift risk.

7. Duplicated aggregate-extraction logic. executorAggregateErrorItems (executor.ts) reimplements aggregateErrorItems (workflow-failures.ts) almost verbatim. Consider exporting the shared one.

Style / conventions

• Per CLAUDE.md the package ships raw TS with no build step — confirmed, no dist/build artifacts introduced.
• bun run typecheck and the targeted tests couldn't be executed in this review sandbox (commands needed approval); the PR notes report both passing — please ensure CI is green before merge.

Overall: solid, defensive work with thoughtful classification and good tests. The items above are clarifications and polish rather than defects.

[claude]

Review: structured failure codes, dispositions & blocked runs

Reviewed the full diff (classifier, executor lifecycle, store, persistence, status/runtime/dispatch, and tests). Overall this is a well-structured, carefully-typed change with genuinely strong test coverage (23 classifier cases plus dedicated executor tests for killed vs. active-blocked, mixed non-fail-fast parallel adjudication, persistence/restore round-trips, and dispatch). The optional-property spreading and is* type guards follow the repo conventions cleanly. A few things worth considering before merge.

1. Blocked runs render as plain "running" — observability gap (main concern)

recordRunBlocked() sets run.status = "running" with blockedAt/failureRecoverability/failureDisposition metadata, but execution has actually halted and the background JobTracker unregisters the run on settle (runner.ts swallows the resolved RunResult). So in the store a blocked run is indistinguishable from a live, executing one unless a consumer also inspects blockedAt.

• tui/status-list.ts renders it as ● running (warning color) via the run.status === "running" branch (line ~199). runningStageLabels() finds no running stage and returns undefined, so the user sees a "running" workflow with no active stage and no hint it's actually blocked awaiting manual resume.
• The new metadata is surfaced through inspectRun detail, but not in the at-a-glance list.

Suggestion: add a blocked indicator in the status list when run.blockedAt !== undefined / failureDisposition === "active_blocked" (e.g. a blocked glyph), so a silently-halted run isn't mistaken for one making progress. Also worth confirming nothing counts "running runs" as live work and diverges from JobTracker's live-job set.

2. Duplicated aggregate-extraction logic

executorAggregateErrorItems() in executor.ts is a near-verbatim copy of aggregateErrorItems() in workflow-failures.ts (both handle native AggregateError.errors plus a .errors record fallback). Consider exporting the shared helper and reusing it to keep the two in sync.

3. redactSensitiveText coverage gaps (minor, security)

• Bearer tokens are not covered: Authorization: Bearer eyJ... matches neither the sk- pattern nor the (api_key|token|credential|secret)[:=] pattern (bearer/authorization aren't in the keyword set), so a surfaced provider error containing one would leak. Worth adding bearer/authorization to the keyword alternation.
• The sk-…{8} rule keeps sk- + 8 chars and requires + (>=1) more, so a key with exactly 8 trailing chars isn't redacted at all. Likely fine for real key lengths, but an exact-length edge.

These only matter for the unsanitized message paths; userMessage for the credential codes is a fixed constant, so the common case is safe.

4. structuredClassification precedence is hard to follow

The ~860-line classifier is well-tested but the precedence chain (strong code -> strong name -> weak-auth + message refinement -> related[diagnostic/nested/cause/aggregate] vs. status -> message -> weak fallback) is intricate. A short decision-table comment at the top of structuredClassification/classifyWorkflowFailure documenting the intended ordering would meaningfully help future maintainers, since the correctness of #1269's "401 wrapper must not hide a missing_api_key diagnostic" fix lives entirely in this ordering.

5. Mixed candidate set in selectRunFailureDisposition (low risk, worth a comment)

For non-fail-fast parallel failures thrown as an AggregateError, candidates contains both stage candidates and re-classified aggregate candidates representing the same underlying failures. The adjudication is robust to this (find + every(isRecoverableActiveBlockedCandidate)), but if a stage's stored failureDisposition ever disagrees with the re-classification of its inner error, the "all recoverable" gate silently falls through to terminal-failed. A brief comment noting the dual-source candidate list (and that the every gate intentionally requires unanimity across both sources) would prevent a future reader from "fixing" the apparent duplication.

Behavior change to confirm

status.ts resumeRun() broadened the not-resumable branch from failed && endedAt && resumable === false to killed || resumable === false. This looks correct (killed runs shouldn't resume), just flagging it as an intentional semantic change for reviewers.

None of these are blocking. #1 is the one I'd most want addressed (or explicitly deferred) since it affects what users see for every blocked run.

[claude]

Review: structured failure codes, dispositions, and blocked runs

Thorough, well-factored change. The classifier in workflow-failures.ts is readable and the priority-based disposition selection in the executor is carefully layered (terminal_killed → terminal_failed → recoverable-only-blocked). The new types, store method, persistence entries, and restore path all hang together cleanly. A few things worth a look before merge.

🔴 Lifecycle: an active_blocked source run is never finalized on resume

This is my main concern. recordActiveBlockedFailure is reached from the executor's catch block (executor.ts:3773), i.e. after def.run(ctx) has fully unwound — the workflow body is no longer executing. The run is left in status: "running" with endedAt undefined (store.ts:546; recordActiveBlockedFailure returns status: "running").

resumeFailedRun then starts a brand-new continuation run via runDetached (runtime.ts:416) and never transitions the source out of its blocked state. Consequences:

• inFlightRunCount() counts endedAt === undefined (extension/index.ts:1053-1055), so the orphaned blocked run permanently blocks /workflow reload ("still in flight") and shows up in interrupt lists.
• isActiveBlockedResumable stays true (runtime.ts:398), so the same blocked run can be resumed repeatedly, each spawning another continuation.
• On the next session, scanInFlightRuns re-hydrates it and recordRunBlocked re-blocks it (persistence-restore.ts:202-228) — it never clears.

For terminal-failed sources this is fine (they're already terminal and serve as history). For blocked sources, resume should probably finalize/supersede the source (e.g. removeRun, or transition to a terminal state once the continuation is accepted). Is the lingering "running" intentional? If so, how is it ever cleaned up?

🟠 Unsanitized failureMessage is persisted to disk

Redaction (redactSensitiveText) is correctly applied to userMessage, but the raw failure.message flows into failureMessage, which is written to session entries by appendRunBlocked/appendRunEnd (executor.ts:1949,1962; persistence-session-entries.ts). If a provider error embeds an API key or token, it lands unredacted on disk. Given the effort spent on redaction for the user-facing surface, consider redacting (or gating) the persisted diagnostic text too.

Relatedly, the sk- rule keeps the first 8 chars: (sk-[A-Za-z0-9_-]{8})[A-Za-z0-9_-]+ (workflow-failures.ts:521). For sk-ant-... keys that leaks a recognizable prefix; redacting from sk- onward is safer.

🟠 Test coverage gap on the blocked-resume path

The executor blocked path (22 refs in executor.test.ts), persistence entries, and restore are all covered — nice. But the runtime resume of an active-blocked run is not: runtime.test.ts has zero references to blocked/recoverable/resumeFailedRun, and the "blocked" hits in slash-dispatch.test.ts are just a fixture variable name (reload-slash-blocked-…). Since isActiveBlockedResumable (runtime.ts:398) is new branching logic with the lifecycle subtlety above, a direct test would be valuable (blocked run is resumable; resuming it; and the source's post-resume state).

🟡 recordRunBlocked overwrites fields unconditionally

Unlike recordRunEnd, which guards each assignment with !== undefined (store.ts:519-526), recordRunBlocked assigns failureKind/failureCode/failureDisposition/failureMessage directly (store.ts:548-552). The RunBlockedMetadata type makes some required, but the optional ones will null out any existing snapshot values when absent. Current callers always pass them so it's latent, but matching recordRunEnd's guarded style would be more robust and consistent.

🟡 Minor: numeric entries in the semantic code sets are now dead

After the wrapper-status change, codeEvidenceFrom routes any 3-digit code to wrapper_http_status (workflow-failures.ts:272-280), so strongDecisionFromNormalizedCode never sees "401"/"403"/"429"/"500"/"503"…. Those numeric entries in INVALID_API_KEY_CODES, RATE_LIMIT_CODES, FORBIDDEN_CONFIG_CODES, PROVIDER_UNAVAILABLE_CODES are effectively unreachable via the code field (still handled via status + message phrases). Harmless, but a short comment or trimming them would avoid future confusion.

🟡 Question: blocked runs restored regardless of resumeInFlight

In restoreInFlightRuns, the blockedMeta branch (persistence-restore.ts:202) runs before the config.resumeInFlight === "auto" / else check, so a blocked run is re-hydrated as running even when the user configured resumeInFlight: "never". Defensible (blocked ≠ crashed, and it's explicitly recoverable), but worth confirming the "never" preference is meant not to apply here.

---

Nothing above blocks the conceptual approach — the classification/disposition model is solid. The lifecycle item (orphaned blocked run) is the one I'd most want resolved or explained before merge. Nice work on the breadth of the failure taxonomy and the persistence round-trip.

_{🤖 Automated review — verify findings before acting.}

[claude]

PR Review — feat(workflows): add active-blocked failure lifecycle

Thorough, well-factored change. The failure classifier refactor (broad kind to structured decision with code/recoverability/disposition) is a clear improvement over the old kindFromStatus/kindFromCode switch, and the executor/store/persistence/restore plumbing is consistent end-to-end. Test coverage is genuinely strong — active-blocked, terminal-killed, parallel-precedence, persistence (run.blocked without run.end), and restore paths are all exercised, and the secret-redaction and false-positive guards ("failed to log input", "domain model validation failed") are nice. Nicely done overall.

A few things worth a look before merge:

(1) Behavior — any bare HTTP 401 is terminalized as invalid_api_key / non-recoverable (highest priority).
decisionFromStatus(401) returns authDecision("invalid_api_key") => terminal_killed, resumable:false. Message refinement cannot rescue it: canRefineStatusDecisionWithMessage only allows STATUS_MESSAGE_REFINEMENT_CODES, which excludes login_required. So even { status: 401, message: "session expired, please log in" } is classified invalid_api_key and the run is permanently killed, not left active-blocked/resumable.
For pure API-key providers a 401 is genuinely a bad key (terminal is correct). But for the OAuth/subscription login path Atomic itself uses, a 401 is typically an expired session that /login fixes — exactly the recoverable case this PR aims to preserve. As written, a token expiry mid-run becomes an unrecoverable killed run. Deliberate? If a bare 401 cannot be distinguished, treating it as recoverable login_required (active-blocked) seems safer than a terminal kill: a wrong "blocked" costs a manual resume, a wrong "killed" costs the whole run. At minimum, consider letting a login/unauthorized message on a 401 refine down to login_required.

(2) redactSensitiveText leaks an 8-char prefix of sk- keys.
The pattern keeps "sk-" + 8 chars before [redacted] — 8 characters of a live secret retained in userMessage and then persisted to disk in workflow.run.blocked/run.end entries. Prefix-preview redaction is common, but since this is stored, consider redacting the whole token after sk-. The key/value regex also stops at non-space/comma/semicolon, so "Authorization: Bearer " is covered by neither pattern — a likely shape in provider errors.

(3) Active-blocked runs never self-terminalize.
recordActiveBlockedFailure returns status:"running" and the run stays live indefinitely until the user resumes/kills; on restore a workflow.run.blocked entry with no matching run.end re-hydrates as running each session. Appears intentional (the point of active-blocked), but blocked runs accumulate with no TTL/auto-expiry. Worth confirming a UX surface makes these visible/actionable — retryAfterMs is captured but nothing seems to consume it to auto-resume, so the user must know to return.

Minor / nits:

• classifyWorkflowFailure calls errorMessage(error) unconditionally even when structuredClassification supplies its own message — harmless but redundant on the hot path.
• store.ts recordRunEnd grew an intricate wasBlocked branch (clear stale, apply, re-clear disposition, killed override). Correct per the tests, but a one-line comment on why clearStaleBlockedRunMetadata must precede applyRunEndMetadata would help future readers.
• The two identical isResumableContinuation predicates in extension/index.ts (~L1795 and ~L3137) are duplicated; a shared helper would keep them from drifting.

Could not verify locally: test execution and typecheck were blocked in this environment, so this is a static review against the diff. The PR description reports bun test/typecheck/lint green — assuming CI confirms, the logic and coverage look sound.

Nothing here is a hard blocker except possibly item 1, which is a behavior question worth an explicit decision.

[claude]

PR Review: structured failure codes, dispositions, and active-blocked run lifecycle

Thanks for this — the failure-classification model is a real improvement over the coarse WorkflowFailureKind, and the structured recoverability/disposition split maps cleanly onto the kill-vs-block lifecycle. The persistence round-trip (appendRunBlocked → findRunBlockedMetadata → recordRunBlocked) is well thought through, and test coverage across classification, executor lifecycle, persistence, status, runtime, and slash dispatch is genuinely thorough. A few things worth a look before merge.

1. Recoverable session-expiry 401s are classified as terminal/non-recoverable (behavior question)

A bare 401 resolves to invalid_api_key → terminal_killed / non_recoverable / resumable: false (asserted in workflow-failures.test.ts:107). The message-refinement path deliberately excludes login_required — it's absent from STATUS_MESSAGE_REFINEMENT_CODES, so a { status: 401, message: "Please log in / session expired" } is not refined down to the recoverable login_required decision; it stays invalid_api_key and the run is killed as non-resumable.

That's a behavior change from the prior model where auth was resumable. Expired-session 401s (recoverable via /login) would now force a brand-new workflow run instead of a resumable block. If treating bare 401 as invalid-credentials is the intended default, fine — but consider letting a strong login_required message signal refine a 401 wrapper (i.e. add login_required to the status-message refinement set), since that case is genuinely recoverable. At minimum worth a test pinning the intended behavior of { status: 401, message: "please log in" }.

2. failureMessage is persisted unredacted (security)

redactSensitiveText is applied only to userMessage (workflow-failures.ts:95), and at source === "top_level" the raw message is passed through verbatim (workflow-failures.ts:883). That raw message becomes stage.failureMessage / run.failureMessage and is written to session entries via appendStageEnd/appendRunEnd/appendRunBlocked. Raw provider errors can embed API keys/tokens, so secrets can land on disk in the session log.

This is partly pre-existing, but the PR is the right moment to close it: consider running redactSensitiveText over failureMessage before persistence (or before it leaves classifyWorkflowFailure). Note sanitizeTerminalRunEndPayload does not help here — despite the name it only normalizes disposition/recoverability invariants, it doesn't scrub text.

3. Naming: sanitizeTerminalRunEndPayload (minor)

The name implies content sanitization, but it normalizes failureDisposition/failureRecoverability/resumable invariants for killed/blocked entries. normalizeTerminalRunEndPayload would read more accurately and avoid a false sense of redaction (see #2).

4. Classification complexity / maintainability (minor)

structuredClassification plus the family of ad-hoc predicate sets (STATUS_MESSAGE_REFINEMENT_CODES, BROAD_AUTH_MESSAGE_REFINEMENT_CODES, STATUS_RELATED_MESSAGE_REFINEMENT_CODES, canUseRelatedClassificationBeforeStatus, ...) encode a precedence policy that's hard to reconstruct by reading. The tests are effectively the only spec for the ordering. A short doc comment laying out the precedence ladder (strong code → name → weak-auth+message refine → related/diagnostic/nested/cause/aggregate → status → message → weak) would pay off for the next person touching this.

5. Blocked runs restored as running regardless of resumeInFlight (minor — confirm intent)

In restoreOnSessionStart, the blockedMeta branch restores the run as running + blocked and continues before the config.resumeInFlight === "auto" check. So even with resumeInFlight: "off", blocked runs reappear as active. If that's intentional (blocked runs are a distinct category), great — just flagging that it bypasses the in-flight gate.

6. retryAfterMs from a date-form Retry-After is a duration, not a deadline (minor)

retryAfterHeaderMs converts an HTTP-date Retry-After to dateMs - Date.now() (a relative ms duration). When persisted on a blocked run and restored in a later session, that stored duration no longer corresponds to a wall-clock window — a consumer using it post-restore gets a stale value. Storing an absolute resume-after timestamp (or recomputing on restore) would be more robust.

Nits

• recordRunEnd for killed already forces non_recoverable/terminal_killed/resumable:false, so the explicit metadata block now passed from killRun/finalizeKilled is partly redundant — harmless, just noting the duplication.

Overall a solid, well-tested change. #1 (recoverable 401 → terminal) and #2 (secret persistence) are the two I'd want resolved or explicitly confirmed-as-intended before merge; the rest are minor.

Note: typecheck/test runs are gated in this review environment, so I relied on static review plus the PR's stated bun test / bun run typecheck results.

[claude]

Review — feat(workflows): classify recoverable provider failures (#1272)

Thorough, well-tested change. The new two-tier model (terminal-killed/attach-only vs. active-blocked/resumable) maps cleanly onto the acceptance criteria in #1269, and the lifecycle wiring is consistent across the snapshot types, store, executor, persistence, restore, background status, and both resume gates (runtime.ts:397-398, index.ts:1799-1800, index.ts:3141-3142). The disposition + recoverability dimensions are a much better fit than the old binary resumable. Nice work, and the test surface is broad.

Feedback below, roughly by severity.

🔴 Security — raw provider error (with secrets) is persisted unredacted

redactSensitiveText() is applied only to userMessage. The raw failure.message is copied verbatim to stage.failureMessage (executor.ts:1462), into the run metadata (failureMessage), and then written to the session JSONL via appendRunEnd/appendRunBlocked (persistence-session-entries.ts:243,260) and onto the in-memory RunSnapshot.failureMessage.

The motivating example in #1269 is literally 401 Incorrect API key provided: sk-proj-****…a_WA. With this change the redacted form is shown to the user, but the unredacted key is written to disk in the session file (and survives restore). Given the whole feature exists to handle credential errors, persisting the secret at rest undermines the redaction. Recommend redacting failureMessage before it leaves the process (at minimum on the persisted payload), or dropping it from persistence entirely if it isn't needed after restore.

Secondary: redactSensitiveText only catches sk-… keys and key|token|credential|secret = … patterns. Common formats (Google AIza…, AWS AKIA…, bare bearer tokens) pass through even on the display userMessage. Worth widening the patterns, or relying on a canned userMessage (as the auth/forbidden/unknown-model decisions already do) rather than echoing the provider string.

🟠 Resumed active-blocked source run appears to linger as running

resumeFailedRun() starts a new continuation run via runDetached(...) for an active_blocked source (runtime.ts:416-419), but I don't see anything that transitions the original blocked run out of status: running (it stays running with blockedAt set). For the terminal-failed path the source is already terminal, so it's fine — but an active-blocked source looks like it remains a perpetual running entry in the store/status list alongside the new continuation.

Could you confirm the intended behavior? If the source is meant to be superseded on resume, it should be transitioned (e.g. to killed/terminal or otherwise hidden) when the continuation starts; otherwise workflow status will show a ghost running run that never ends. A test asserting the source run's state after resuming a blocked run would lock this down — the existing runtime/slash tests assert the accepted continuation but not the source's post-resume state.

🟡 Aggregate/precedence semantics — a couple of edge cases worth a test

• selectRunFailureDisposition only takes the recoverable-blocked branch when every candidate is recoverable-active-blocked (executor.ts:1667-1671). candidates mixes stage candidates and aggregate-inner candidates, and stageFailureCandidate defaults a stage with no failureDisposition to terminal_failed (executor.ts:1581). So a single recoverable rate-limit alongside any stage that failed through a path that didn't call applyFailureToStage will demote the whole run to terminal-failed. That's defensible (terminal beats recoverable), but the missing-disposition ⇒ terminal_failed default is an easy footgun — a quick comment on why the default is terminal would help future readers.

• decisionFromStatus maps a bare 401 → invalid_api_key (terminal, non-resumable) (workflow-failures.ts:674). A 401 from an expired/missing session token is recoverable in spirit, and is only rescued if message/related refinement kicks in. This matches the issue's guidance to treat the 401 example as terminal, so it's likely intentional — just calling out that a transient/login 401 with no refining message will be classified attach-only.

🟡 Maintainability — classifier complexity

workflow-failures.ts is ~900 lines of layered heuristics with subtle precedence (strong_signal → weak_signal → status → message → related → aggregate, plus the canRefine* / canUseRelatedClassificationBeforeStatus gates). It's the kind of code where a one-line reordering silently changes behavior. The extensive unit tests are the right mitigation, but a short doc-comment at the top of the file laying out the precedence order (and why each refinement gate exists) would make this far more approachable. This is also exactly the heuristic debt #1269 calls out — worth a tracking note to push a canonical taxonomy down into pi-ai so this layer can shrink.

Minor: in relatedStructuredClassification, the seen set is threaded sequentially through diagnostics → nested → cause, so a nested error that is reference-equal to a diagnostic already visited gets skipped. Almost certainly harmless given how these objects are constructed, but worth a glance.

✅ Things done well

• Consistent wasBlocked handling + clearStaleBlockedRunMetadata so a blocked→killed transition doesn't leave stale active_blocked metadata (store.ts:547-566).
• sanitizeTerminalRunEndPayload guarantees terminal run.end entries never persist failureDisposition: active_blocked and force killed→non-resumable (persistence-session-entries.ts:214-228) — good defense in depth.
• Restore correctly routes blocked-then-killed runs through the terminal path (scanInFlightRuns excludes ended runs) and rebuilds blocked siblings as blocked with blockedByStageId (persistence-restore.ts:344-360).
• Strict typing throughout, no any/unknown leakage, exhaustive type guards (isWorkflowFailure*).

Performance is a non-issue — classification only runs on failure paths.

Overall: solid, mergeable after the secret-at-rest concern is addressed and the lingering active-blocked source-run behavior is confirmed (or fixed).

_{🤖 Generated with Claude Code}

[claude]

Review: structured failure classification with active-blocked lifecycle

Nice work — this is a large but well-organized change. The classifier refactor (kind → WorkflowFailureDecision with code/recoverability/disposition) is much more expressive than the old WorkflowFailureKind-only model, the new types are purely additive on the snapshots/payloads, and the precedence handling (strong code → name → message-refined → status → related → weak) is carefully thought through. Test coverage is genuinely strong (classifier precedence, executor lifecycle, persistence round-trips, resume flows, iteration-6/7 regressions).

A few things worth a closer look before merge.

1. Persisted failureMessage can leak provider secrets (medium, mostly pre-existing)

The new redactSensitiveText is applied to userMessage and to nested/diagnostic source messages, but not to the top_level message:

// workflow-failures.ts — classifyWorkflowFailure
const structuredMessage = structured.message !== undefined
  ? (structured.source === "top_level" ? structured.message : redactSensitiveText(structured.message))
  : message;

That raw message then flows into failure.message → applyFailureToStage sets stage.failureMessage = failure.message → it is persisted via appendStageEnd/appendRunEnd and restored. The common case where a provider SDK error (e.g. a 401 echoing the key/credential) propagates as the thrown error is exactly the top_level path — so the most secret-prone message is the one that escapes redaction, while the nested ones get scrubbed. userMessage is safe, but the on-disk failureMessage is not. Consider redacting the persisted diagnostic message too (or running redactSensitiveText on the top-level path). The raw-diagnostics behavior likely predates this PR, but this PR is the natural place to close it since it introduces the redactor.

Minor, related: sk-[A-Za-z0-9_-]{8} retains 8 chars after the sk- prefix. If that is a deliberate "show a prefix" choice it is fine; otherwise consider trimming further.

2. unknown to terminal_failed can mask a recoverable block (design)

In selectRunFailureDisposition, candidate selection is: terminal_killed then terminal_failed then all-recoverable-active_blocked. Because unknownDecision() maps to terminal_failed, an unclassifiable outer wrapper error will be selected ahead of a genuinely recoverable active_blocked stage, terminalizing a run that could have stayed blocked/resumable. The blast radius is limited (unknownDecision.resumable === true, so it still resumes via the terminal-failed path), but it means "we could not classify the outer error" is treated identically to "the outer error is decisively terminal." If that is intended, a one-line comment would help; otherwise consider distinguishing decisively terminal from merely unclassified so the latter does not outrank a recoverable block. This is the one spot where iteration 7's "outer failures participate" goal has a sharp edge.

3. resumeRun early-return was broadened (minor)

The guard went from run.status === "failed" && run.endedAt !== undefined && run.resumable === false to run.status === "killed" || run.resumable === false, and it runs after the paused-handle handle.resume() calls fire. Any run with resumable === false — regardless of status — now short-circuits to not_resumable. In practice paused/HIL runs should not carry resumable === false, so this is likely safe, but the broadening is worth a quick sanity check that no live state sets resumable: false on a still-resumable run.

4. Duplicated aggregate-error extraction (nit)

aggregateErrorItems (workflow-failures.ts) and executorAggregateErrorItems (executor.ts) implement the same AggregateError/.errors unwrap. Worth exporting one and reusing to keep the instanceof AggregateError handling in a single place.

Things I checked that look correct

• workflowDetailsFromRun status mapping now routes non-terminal results to running instead of silently collapsing blocked/paused into completed — good catch.
• Store metadata clearing on terminalization (clearStaleBlockedRunMetadata / wasBlocked handling) and killRun forcing non_recoverable/terminal_killed/resumable: false are consistent across store + persistence (sanitizeTerminalRunEndPayload) + restore.
• Restore validation in findRunBlockedMetadata is appropriately strict (failureRecoverability === "recoverable", resumable === true, numeric ts) and dependents are restored as blocked while the blocking stage is failed.
• classifyExecutorFailure memoization avoids re-classifying the same error object across the executor's multiple call sites.

Overall this looks solid and the lifecycle modeling is a clear improvement. Item 1 (secret redaction on persisted failureMessage) is the only one I would consider blocking; the rest are confirm-intent / cleanup.

🤖 Generated with Claude Code

[claude]

Review: structured failure classification with active-blocked lifecycle

Thanks for this — the design is solid and the test coverage is genuinely impressive (classifier precedence, executor lifecycle, persistence round-trips, status, and slash-dispatch resume are all exercised). The additive, all-optional metadata keeps the snapshot types backward compatible, the secret redaction is a nice touch, and the per-run classifiedFailures cache avoids re-classifying the same error object. Comments below, roughly by severity.

🔴 Resumed active_blocked source run is never terminalized → zombie + re-restore on every session start

This is my main concern. When a terminal failed run is resumed, the source already has a workflow.run.end entry, so it's outside the in-flight set and restoreTerminalRuns rebuilds it as clean terminal history. A blocked source is different: it has start + workflow.run.blocked but no end entry.

• resumeFailedRun (runtime.ts) spawns a new continuation run but never calls recordRunEnd on the source. The slash-dispatch test even asserts this is intentional:

  assert.equal(store.runs().find((run) => run.id === sourceRunId).status, "running");

  So the live store keeps a perpetually-running run that has actually been superseded, and nothing reaps it.
• Worse, on the next restoreOnSessionStart, the in-flight scan (scanInFlight = started-without-end) still sees the source as in-flight, and findRunBlockedMetadata re-hydrates it as a blocked running run again — alongside the continuation run. Each resume → restart cycle resurrects a stale blocked run, so they accumulate.

Suggestion: on a successful resume of an active_blocked source, terminalize it (append a workflow.run.end marking it superseded/resumed, e.g. terminal failed non-resumable) so it leaves the in-flight set and won't be re-restored. If keeping it running in the live store is deliberate for UX, the persistence side still needs an end marker to avoid duplicate restoration.

🟡 Bare 401/403 now classify as terminal, non-recoverable, killed

decisionFromStatus maps an unrefined 401 → invalid_api_key (terminal_killed / non_recoverable) and 403 → forbidden_config (terminal_killed). Previously these were recoverable auth. The refinement chain does a good job catching local-login wrappers and message/code corroboration, but the fallback for a status-only signal is now aggressive: a transient or ambiguous 401 (expired short-lived token) or a 403 that some providers use for region/quota issues will permanently kill an otherwise-resumable run. Worth confirming this trade-off is intended; consider defaulting status-only auth failures to recoverable unless corroborated by a code/message signal, since a false "terminal" is harder to recover from than a false "blocked".

🟢 WorkflowFailure.message is now redacted but still documented as raw

classifyWorkflowFailure passes redactSensitiveText(...) into failureForDecision, so both message and userMessage end up redacted. The JSDoc on WorkflowFailure.message ("Original error text, preserved for diagnostics") and on StageSnapshot.failureMessage ("Original unsanitized error text") are now misleading — they're sanitized. Good for security, but please update the comments so future readers don't assume raw text is available for debugging.

🟢 Unreachable entries in the code Sets

INVALID_API_KEY_CODES ("401"), FORBIDDEN_CONFIG_CODES ("403"), and RATE_LIMIT_CODES ("429") contain 3-digit numeric strings, but codeEvidenceFrom routes any 3-digit value to wrapper_http_status before the semantic-code path, so those entries are unreachable via code (only possibly via name). Harmless, but confusing — consider dropping them for clarity.

🟢 Changelog entry missing

packages/workflows/CHANGELOG.md ## [Unreleased] is empty. Per CLAUDE.md, a feature this size should land an entry under ### Added / ### Changed referencing #1269.

🟢 Classifier precedence is hard to follow

structuredClassification plus the STATUS_*_REFINEMENT_CODES sets and the canRefine… / canUse… predicates encode a lot of edge-case precedence accreted over the iteration history. It's well tested, but the ordering (status vs semantic code vs message vs diagnostic/nested/cause/aggregate, and the 401 login-vs-invalid tie-break) is non-obvious and risks overfitting to the test cases. A short doc comment or decision-table at the top of the function summarizing the precedence would pay off for the next maintainer.

---

Nothing here blocks the core approach — the 🔴 item is the one I'd want resolved (or explicitly confirmed as acceptable) before merge, since it affects persisted state across restarts. Nice work overall.

Note: I couldn't run bun test / bun run typecheck in this environment (sandbox restrictions), so I'm relying on the PR's stated validation and CI for green status.

[claude]

PR Review — structured failure classification with active-blocked lifecycle

Thanks for this — it's a substantial, well-tested piece of work. The taxonomy is coherent, the persistence round-trips are carefully handled, and the ~3k lines of new tests are reassuring. Notes below, ordered by impact.

Potential bugs / behavioral concerns

1. Blocked source run is never retired after resumeFailedRun (highest priority). resumeFailedRun (extension/runtime.ts) spawns a NEW detached run via runDetached(... continuation ...) but never ends or removes the source run. For the pre-existing terminal-failed path this was fine because the source already had endedAt set. For the new active_blocked path the source run has endedAt === undefined, status: running, resumable: true, failureRecoverability: recoverable. After a resume that means: (a) the old blocked run lingers as in-flight indefinitely — inFlightRunCount() (extension/index.ts) counts endedAt === undefined, so it keeps counting the abandoned run and can skew any in-flight gating/limits; (b) it still satisfies isResumableContinuationRun / isActiveBlockedResumable, so the same blocked run can be resumed repeatedly, each time spawning another new run. Is the intent that resume retires the source (e.g. recordRunEnd/removeRun once the successor is accepted), or that a blocked run lives until manually killed? Either way an explicit test asserting source-run state post-resume would help.

2. A bare HTTP 401 is now terminal / non-recoverable. In decisionFromStatus, 401 -> authDecision(invalid_api_key) -> non_recoverable / terminal_killed, and 403 -> forbidden_config -> terminal. Previously kindFromStatus mapped 401/403 -> auth with resumable: true. The classifier does try login-context refinement first (canUseLoginClassificationBeforeWrapper401, isClearLocalLoginMessage), but a 401 with no login hints and no recognizable code now kills the run as non-recoverable, requiring a brand-new run. Mid-run token expiry is often recoverable by re-login, so treating every bare 401 as terminal may be too aggressive — worth confirming this is the desired default.

3. recordRunBlocked writes undefined fields directly. In store.ts, recordRunBlocked does run.failureCode = metadata.failureCode (and similar) unconditionally, whereas the rest of the store (applyRunEndMetadata, etc.) guards with !== undefined. Since failureCode/failureDisposition are optional on RunBlockedMetadata, this can set the property to an explicit undefined. Harmless for reads but diverges from the surrounding convention and can surprise serialization/structuredClone callers — suggest mirroring the guarded-assignment pattern.

Maintainability

4. Classifier complexity. workflow-failures.ts grew a large web of refinement predicates (canUseRelatedClassificationBeforeStatus, canRefineStatusDecisionWithMessage, canRefineWeakAuthDecisionWithMessage, canUseLoginClassificationBeforeWrapper401) plus the strong/weak/status evidence ladder. It is well-tested, but the precedence rules are hard to hold in one's head and easy to regress. A short doc comment at the top of structuredClassification describing the precedence order (strong code -> name -> weak-auth-refined-by-message -> related [diagnostic/nested/cause/aggregate] -> status -> message -> weak) would pay for itself. No logic change required.

Security

5. redactSensitiveText leaks a key prefix and is provider-specific. The sk- pattern preserves sk- plus the first 8 chars (e.g. sk-proj-a...[redacted]), exposing ~11 leading characters of the credential in persisted/diagnostic text. The high-entropy tail is redacted so this is low-risk, but consider redacting the whole token after sk-. Also, the first regex only covers sk--style keys; Google (AIza...), AWS, and bearer-style provider keys rely solely on the generic key=|token=|secret= pattern, which will not catch a raw key without that prefix. Since this text is persisted to session entries, tightening redaction is worth a look.

Minor

• retryAfterHeaderMs calls Date.now() to convert an HTTP-date Retry-After. Fine in package source (not a workflow script), but it makes classification mildly time-dependent — just flagging.
• resumeRun (background/status.ts) broadened the not-resumable guard from failed && endedAt && resumable === false to status === killed || resumable === false. Looks intentional and consistent with killed runs now always carrying resumable: false, but it is a semantics change worth a line in the PR description.
• Nice touch caching classifications via the classifiedFailures Map in the executor to avoid re-classifying the same error object across stage/aggregate/outer paths.

Tests

Coverage is strong (classifier precedence, retry-after extraction, lifecycle transitions, persistence round-trips). Main gaps I would add: an assertion on the source blocked-run state after a successful resume (point 1), and an explicit case for bare 401/403 with no login/code hints documenting the terminal disposition (point 2).

Overall this is solid and close. Items 1 and 2 are the ones I would resolve (or consciously sign off on) before merge; the rest are polish.

Automated review — I could not run bun run typecheck / bun test in this sandbox, so the above is from static reading; trusting the validation results in the PR description.

[claude]

Code Review — typed failure taxonomy with active-blocked lifecycle

Thorough, well-structured PR. The typed taxonomy is clean (exhaustive switch-based guards, decision factories), the secret-redaction addition is a genuine security improvement, and the test coverage (~3k lines across classifier precedence, executor lifecycle, persistence round-trips, and resume flows) is excellent. Defensive fallbacks are thoughtful — e.g. an active_blocked disposition with no failedStageId correctly degrades to a terminal failure rather than stranding the run. Below are findings ordered by significance; most are discussion points rather than blockers.

1. Behavior change: a bare 401 now terminates the run as non-recoverable invalid_api_key

decisionFromStatus(401) -> authDecision("invalid_api_key") -> terminal_killed / non_recoverable, which kills the run immediately. Previously 401 mapped to auth with resumable: true.

The classifier does refine toward login_required when the message clearly indicates login (/login, "not logged in", etc.), but a generic 401 does not refine — e.g. { status: 401, message: "Unauthorized" }:

• "unauthorized" is in LOGIN_REQUIRED_PHRASES, but login_required is not in STATUS_MESSAGE_REFINEMENT_CODES, so no refinement,
• isClearLocalLoginMessage("Unauthorized") is false (not in LOCAL_LOGIN_REQUIRED_PHRASES and no /login),
• falls through to terminal invalid_api_key.

So expired-OAuth-token / transient-session 401s (which would succeed after /login) are now non-recoverable and lose the run. Since the cost asymmetry favors blocking over terminating (a wrongly-"blocked" run is just resumed; a wrongly-"terminal" run is gone), consider defaulting an ambiguous bare 401 to recoverable login_required (active_blocked) and reserving terminal invalid_api_key for cases with a clear invalid-key signal. At minimum, worth confirming this is the intended trade-off.

2. Active-blocked runs stay running with no endedAt indefinitely

A recoverable failure parks the run in running forever until a manual resume/kill. retryAfterMs is captured and persisted but nothing acts on it (no scheduled auto-retry). Two things to confirm:

• Background concurrency/scheduling does not count a blocked run as a live executor slot indefinitely (could starve other runs).
• Whether auto-resume after retryAfterMs is a planned follow-up; if so, a code comment noting the intended operator/resume flow would help, since "running but actually blocked" is a non-obvious state for anything inspecting status.

3. Redaction is best-effort and misses common secret shapes

redactSensitiveText covers sk-... keys and api_key|token|credential|secret : value / = pairs, but the redacted message is now persisted to session entries, so gaps matter. Not caught: bearer tokens (Authorization: Bearer ya29... / JWTs), Google AIza... keys, and Bearer <token> without a :/= separator. Consider broadening the pattern set, or add a comment that redaction is best-effort defense-in-depth.

4. Dead branch in selectDiagnosticFailureClassification

Both the allRecoverableBlocked branch and the final fallback return classifications[0]!, so that allRecoverableBlocked check is a no-op here (unlike the analogous block in aggregateClassification, where the fallback differs by returning unknownDecision()). Either drop the dead check or make the fallback meaningfully different.

5. Stale doc comment on failureMessage

store-types.ts still documents failureMessage as "Original unsanitized error text...", but applyFailureToStage now assigns the redacted failure.message. The safer behavior is good — just update the comment so it does not imply raw secrets may be stored there.

6. CHANGELOG not updated

Per CLAUDE.md, feature work gets an entry under ## [Unreleased] in packages/workflows/CHANGELOG.md, which is currently empty. Add an ### Added entry referencing #1269.

---

Nice work overall — the classifier precedence logic is intricate but the test suite gives good confidence. Items 1-2 are the ones I would most want a maintainer decision on before merge; 4-6 are quick cleanups.

_{Automated review via Claude Code}

[claude]

Code Review — typed failure taxonomy + active-blocked lifecycle

Reviewed the full diff (9 source files, ~600 LOC of logic + ~2.6k LOC of tests). This is a well-structured change: strong typing throughout, exhaustive guard helpers (isWorkflowFailure*), memoized classification (classifyExecutorFailure), and genuinely careful metadata clearing in Store.recordRunEnd. The classifier's evidence-priority model (strong signal → message refinement → status → weak) is thoughtful. Nice work.

A few things worth a closer look before merge.

1. Resumed blocked run lingers as "running" forever and stays independently resumable (design question)

resumeFailedRun (runtime.ts:392) spawns a new continuation run but never transitions the source blocked run out of "running"/endedAt === undefined. The test at slash-dispatch.test.ts:3853 pins this as intended (source ... status === "running" after resume), so I'm raising it as a question rather than a bug:

• The source blocked run stays resumable === true + recoverability === "recoverable", so isActiveBlockedResumable keeps matching it — the same source can be resumed repeatedly, each spawning another continuation run.
• The background promise resolves with status: "running", so runner.ts:137 unregisters the job from the tracker. The store then shows a "running" run with no live controller behind it.
• On every session restore, findRunBlockedMetadata re-hydrates it as "running" blocked (no end entry exists), so these phantom runs accumulate across sessions.

For the pre-existing terminal_failed path this is benign (source is already ended). For active_blocked it means a perpetually-"running" entry that can fan out duplicate continuations. Should accepting a continuation mark the source as superseded/ended (or at least flip resumable off)? Worth a deliberate decision.

2. aggregate and outer failure candidates are an effectively dead distinction

In executor.ts, runFailureMetadataFromCandidate handles case "aggregate" and case "outer" identically (runFailureMetadataFromFailure(candidate.failure, undefined)), and both candidate constructors set the same fields. The two source variants never diverge in behavior — only failedStageIdsForCandidate differentiates them. Consider collapsing them, or documenting why the distinction exists, so a future reader doesn't assume the branches do different things.

3. Duplicated aggregate-extraction logic

executorAggregateErrorItems (executor.ts) and aggregateErrorItems (workflow-failures.ts:779) are near-identical (error instanceof AggregateError ? error.errors : error.errors field). Since executor.ts already imports from workflow-failures.ts, exporting one shared helper would prevent the two from drifting.

4. resumeRun not-resumable predicate was broadened

background/status.ts:273 changed the guard from status === "failed" && endedAt !== undefined && resumable === false to run.status === "killed" || run.resumable === false. The new run.resumable === false clause now matches any run (paused, etc.) with resumable === false, not just terminal failed ones. Please confirm no completed/paused run with resumable === false reaches this path and gets the new "not resumable" message unexpectedly.

5. Bare HTTP 401 → invalid_api_key → terminal_killed (non-recoverable)

decisionFromStatus (workflow-failures.ts:686) maps a top-level status 401 to invalid_api_key, which is terminal_killed / non-recoverable — the run dies immediately with no resume. A transient/misrouted upstream 401 (no refining body) would therefore be unrecoverable. The message-refinement paths (STATUS_MESSAGE_REFINEMENT_CODES, canUseLoginClassificationBeforeWrapper401) mitigate the common cases, but it's worth confirming that's the intended default for an unannotated 401, given how aggressively terminal_killed ends runs.

Minor

• selectRunFailureDisposition: a failed stage with no recorded failureDisposition defaults to terminal_failed, which outranks a recoverable-blocked sibling. Fine for fresh runs, but restored/older stages without that field could tip an otherwise-recoverable run to terminal. Likely an edge case given the persistence round-trip now carries the field.
• recordRunBlocked / classifier rely on Date.now(); expected here, just noting it keeps these functions non-pure for test determinism.

Tests

Coverage is strong — classifier precedence/retry-after extraction, executor lifecycle transitions, persistence round-trips, restore, and resume flows are all exercised. The one scenario I didn't see covered is the post-resume lifecycle of the source blocked run across a second resume / session restore (item 1) — worth a test that asserts the intended behavior once that decision is made.

Did not execute the suite here (bun run needs approval in this environment); relying on the PR's stated green validation.

[claude]

Review: typed failure taxonomy with active-blocked lifecycle

Thorough, well-structured PR. The taxonomy is coherent, persistence/restore round-trips are handled symmetrically, and the resume-path unification (isResumableContinuation) removes previously-duplicated inline checks. Guard helpers + isWorkflow* validators on the restore boundary are exactly right. Test coverage looks solid (active_blocked/recordRunBlocked/terminal_killed exercised across 8 unit files). Comments below are mostly suggestions — I found no blocking correctness bug.

Note: I couldn't run the test suite in this review environment (sandbox blocked bun test), so the findings below are static review only and rely on the author's reported green runs.

Correctness / potential issues

1. recordRunBlocked writes undefined values as present keys (store.ts:589-591). Unlike applyRunEndMetadata, which guards every field (if (metadata.x !== undefined) run.x = ...), recordRunBlocked does unconditional assignment:

   run.failureCode = metadata.failureCode;               // may be undefined
   run.failureDisposition = metadata.failureDisposition; // may be undefined

   RunBlockedMetadata makes failureCode/failureDisposition optional, so a blocked snapshot can end up with explicit failureCode: undefined keys. These survive structuredClone into inspectRun detail and any serialized output, which is inconsistent with the guarded style used everywhere else. Suggest guarding these the same way (or documenting that present-with-undefined is intentional).

2. selectRunFailureDisposition precedence — terminal_failed outranks recoverable-blocked. The candidate scan is terminal_killed → terminal_failed → all-recoverable-blocked → fallback, and the outer-error candidate is always appended. If the outer error ever classifies as unknown (→ terminal_failed) while the real failed stage is recoverable (e.g. rate-limited), the outer wins and the run terminates instead of going active_blocked. In practice the stage does throw err (re-throws the original object) and classifyExecutorFailure is memoized, so outer == stage classification and this stays consistent — but that invariant is load-bearing and implicit. A one-line comment ("outer candidate must be the same error object the stage threw, else recoverable stages can be prematurely terminated") would protect future refactors. Worth confirming a test pins the terminal_failed-outer + recoverable-stage case.

3. 401 → invalid_api_key (terminal, non-recoverable) as the default. For OAuth/session-based providers a bare 401 commonly means an expired login (recoverable) rather than a bad key. The login-message refinement (canUseLoginClassificationBeforeWrapper401, isClearLocalLoginMessage) mitigates this, but only when the message clearly says so; an opaque 401 with no recognizable phrase terminates the run as non-recoverable and forces a brand-new run. Given the cost of getting this wrong (kills an otherwise-resumable run), please double-check this matches the providers in play — defaulting a bare 401 to login_required/recoverable may be the safer bias.

Security

4. redactSensitiveText leaks an 8-char secret prefix. (sk-[A-Za-z0-9_-]{8})[A-Za-z0-9_-]+ deliberately preserves the first 8 chars of the key in the redacted output (which lands in user-facing userMessage and persisted failureMessage). Likely fine for debuggability, but it is a partial-secret disclosure into persisted session entries — worth a conscious decision. The generic branch only matches :/=-separated key=value forms; secrets in other shapes won't be caught. Reasonable scope, just flagging the residual.

Maintainability

5. Classifier complexity. workflow-failures.ts grew ~+860 lines and the precedence (structured signal → semantic code → name → message refinement → nested/cause/aggregate → status, with several canUse*BeforeStatus / BeforeWrapper401 predicates) is genuinely hard to follow end-to-end. The logic appears correct and is well-tested, but a short decision-table doc comment at the top of the classification section (what beats what, and why status is deprioritized vs. decisive signals) would substantially lower future maintenance cost.

Minor / notes

6. retryAfterMs is captured and surfaced in metadata/status but isn't yet used to gate or auto-resume anything — informational only. Fine, just noting the taxonomy is richer than current behavior consumes.
7. workflowDetailsFromRun now returns "running" for the non-completed/failed/killed branch (previously fell through to "completed"). WorkflowDetailsStatus includes "running", so this is type-safe and more correct — just confirm no downstream consumer relied on the old "completed" fallback.

Nice work overall — the in-place blocked lifecycle is a clean way to keep recoverable runs resumable without spawning a new run.

🤖 Generated with Claude Code

[lavaman131]

Follow-up fix for the non-convergent Ralph loop.

What changed:

• Diagnostic message-only provider auth failures like 401 Unauthorized / OpenAI API error (401): Unauthorized now classify as invalid_api_key, terminal_killed, resumable: false.
• Non-contiguous invalid-key fallback messages like The API key provided is invalid now classify as non-recoverable provider credentials.
• Added regression coverage in test/unit/workflow-failures.test.ts.

Validation passed locally and in hooks:

• AGENT=1 bun test test/unit/workflow-failures.test.ts
• AGENT=1 bun test test/unit/persistence-restore.test.ts test/unit/executor.test.ts test/unit/background-status.test.ts test/unit/background-status-kill.test.ts test/unit/persistence-session-entries.test.ts
• AGENT=1 bun test test/integration/runtime-tunables.test.ts --timeout 30000
• AGENT=1 bun run typecheck
• AGENT=1 bun run lint
• pre-commit/pre-push hooks including bun run test:unit

Commit pushed: 7f389871

[claude]

Review — typed failure taxonomy + active-blocked lifecycle

Nice, carefully-structured change. The failure taxonomy is clean, the classifier precedence (strong code → name → message → status, terminal-beats-blocked for aggregates) is principled, redaction is applied consistently, and test coverage across classifier/restore/executor/store/runtime is strong. A few things worth a second look before merge.

Potential issues

1. (Most important) A resumed active-blocked run is never transitioned out of "running".
resumeFailedRun spawns a new detached run (runDetached, new runId) and tags it with resumedFromRunId: source.id, but nothing transitions the source run out of its running/blocked state — not in runtime.resumeFailedRun, runs/background/runner.ts, nor the executor continuation path (packages/workflows/src/extension/runtime.ts:416, executor.ts:2276).

For the pre-existing terminal-failed resume path this was fine — the source was already terminal (failed). But for the new active-blocked path the source is running, so after a successful resume you end up with a stale run stuck at running indefinitely:

• It shows as a perpetually-running workflow in status/history output.
• On the next session restore, findRunBlockedMetadata finds its workflow.run.blocked entry again and re-blocks it as running (persistence-restore.ts:199), so it survives restarts.
• Anything that counts "running" runs (concurrency, awaiters, UI "active" badges) will treat it as live.

Is the intent that resume should recordRunEnd/removeRun the source (mark it superseded), or that resume happens in place on the same run id? Right now it's neither, and the stale entry looks unintended.

2. retryAfterMs is captured/persisted/restored but never consumed. It flows through classification → snapshot → journal → restore, but no code schedules an auto-resume after it elapses (grep shows only storage sites). So rate-limited/quota runs sit blocked until a manual resume regardless of the hint. If that's intended (purely informational for display), worth a comment saying so; otherwise the auto-resume wiring looks missing.

3. Parallel multi-stage block records only the first blocked stage. In selectRunFailureDisposition, the recoverable-blocked branch picks candidates.find(isRecoverableActiveBlockedCandidate) and records a single failedStageId in the workflow.run.blocked entry. On restore, restoreBlockedStageState only marks that one stage + its descendants; a second independently-blocked parallel stage (and its descendants) restores as running rather than blocked/failed (executor.ts:468, persistence-restore.ts:401). Probably an acceptable edge case, but the restored graph won't match the pre-crash state for fan-out blocks.

4. workflowDetailsFromRun now maps any non-terminal status (incl. paused) to "running" (executor.ts:1141). Previously paused→completed. This is likely more correct, but it's a behavior change for paused runs in details output — worth confirming no downstream consumer relied on the old mapping.

Minor

• Redaction gaps: redactSensitiveText catches sk-… and api_key|token|credential|secret: …, but not Authorization: Bearer … / bare bearer tokens. Since failureMessage (raw provider text) is persisted, consider broadening the pattern.
• retryAfterHeaderMs treats a field literally named retryAfter as seconds (workflow-failures.ts:202). If any provider/SDK surfaces retryAfter already in ms, that's a 1000× error. retryAfterMs/retryAfterSeconds are unambiguous; the bare retryAfter is the risky one.
• recordRunBlocked sets run.error = error while keeping status = "running". Double-check no status/error renderer shows an error banner on a run it also labels "running" (could read as a contradictory state to users).

Couldn't verify locally

bun test / bun run typecheck were blocked by the sandbox in this environment, so I'm relying on the PR's stated results (66 tests, typecheck clean) plus static review. Worth confirming CI is green on the full test:unit suite, not just the two named files.

Overall this is solid work — item #1 is the one I'd want resolved (or explicitly confirmed as intended) before merge.

🤖 Generated with Claude Code

[lavaman131]

Addressed review feedback in e9d2cbf0.

Changes:

• Fixed the critical active-blocked resume lifecycle issue: after a continuation is accepted, the source active-blocked run is now finalized/persisted as terminal killed with terminal_killed, non_recoverable, and resumable: false, preventing stale running restores/status entries.
• Updated resume coverage to assert the source run is no longer running and that a workflow.run.end entry is written.
• Clarified retryAfterMs as an informational manual-resume hint, treated bare retryAfter as milliseconds while preserving retry-after header seconds semantics, and added tests.
• Broadened redaction for Authorization: Bearer ... / bare bearer tokens with tests.

Validation passed:

• AGENT=1 bun test test/unit/workflow-failures.test.ts test/unit/slash-dispatch.test.ts --timeout 30000
• AGENT=1 bun test test/unit/persistence-restore.test.ts test/unit/executor.test.ts test/unit/background-status.test.ts test/unit/background-status-kill.test.ts test/unit/persistence-session-entries.test.ts test/unit/slash-dispatch.test.ts test/unit/workflow-failures.test.ts --timeout 30000
• AGENT=1 bun test test/integration/runtime-tunables.test.ts --timeout 30000
• AGENT=1 bun run typecheck
• AGENT=1 bun run lint
• git diff --check
• pre-commit/pre-push hooks, including bun run test:unit

[claude]

Review: typed failure taxonomy with active-blocked lifecycle

Thorough, well-tested change. The taxonomy (code / recoverability / disposition / retryAfterMs) is threaded consistently through classification, stage/run snapshots, persistence, restore, and resume; the guard helpers make deserialization safe; and the new redaction on both message and userMessage is a real improvement. The classifier test suite (+30 cases covering wrapper-401 vs. local-login precedence, aggregate errors, retry-after extraction, redaction) is excellent. Nice work.

Findings ordered by significance. Nothing here is a hard blocker, but a few deserve a second look.

Behavior change worth flagging (intentional, but high-impact)

• Bare 401 / Unauthorized is now a terminal invalid_api_key (terminal_killed, non-recoverable). Previously auth failures were resumable. decisionFromStatus maps 401 to invalid_api_key and 403 to forbidden_config, both terminating the run. This is the stated intent of Workflow SDK: distinguish non-recoverable model errors (terminal/killed, attach-only) from recoverable ones (active, resumable) #1269 and is guarded by isClearLocalLoginMessage / canUseLoginClassificationBeforeWrapper401, but it hinges entirely on the local not-logged-in path emitting a recognizable signal (a structured login_required/not_logged_in code, a /login hint, or a local-login phrase). If the host gateway can ever surface a not-logged-in condition as a bare 401 with only an "Unauthorized" message, the workflow will now be killed instead of blocked for re-login. Worth verifying against what pi actually throws on an expired/absent local session.

Possible bug / correctness

• retryAfter unit ambiguity (retryAfterFieldMs, workflow-failures.ts). retryAfterSeconds is correctly x1000 and retryAfterMs passes through, but a bare retryAfter numeric field is treated as milliseconds. Several provider SDKs express retryAfter in seconds, so retryAfter: 30 would be read as 30 ms. Impact is low since retryAfterMs is documented as informational only (blocked runs resume on explicit user action), but the value will be wrong where it surfaces. Consider treating bare retryAfter numerics as seconds, or documenting the expected unit at the call site.

Active-blocked lifecycle implications (design, confirm intent)

• A blocked run is reported as running indefinitely. recordRunBlocked sets status to running and the run never self-resumes. inspectRun now exposes blockedAt/failureDisposition/retryAfterMs so a UI can distinguish it, but any list/status surface that only reads status will show a stalled run as actively running. Confirm the status/list views consume the new fields so users are not misled into thinking work is in flight.
• Restored blocked runs leave unrelated in-flight stages as running (restoreBlockedStageState). Only the failed stage (to failed) and its descendants (to blocked) are touched; sibling non-ended stages stay running even though no executor is driving them post-restart. Documented trade-off, but it means a restored snapshot can display phantom running stages. Calling it out in case it is not intended.

Code quality / maintainability

• selectRunFailureDisposition and the FailureCandidate machinery in executor.ts (~150 new lines of candidate types/builders/selectors) are well-structured but dense. In runFailureMetadataFromCandidate the aggregate and outer switch arms are identical (runFailureMetadataFromFailure(candidate.failure, undefined)) and could collapse. A short comment on the candidate precedence (terminal_killed, then terminal_failed, then all-recoverable-blocked, then fallback) would help future readers.
• aggregateClassification returns classifications[0]! for the all-recoverable-blocked case, discarding sibling data such as a later inner error's retryAfterMs. Minor, but if one sibling carries a Retry-After and the first does not, the hint is lost.
• redactSensitiveText keeps the first 8 chars after the sk- prefix by design (conventional prefix reveal) and fully redacts bearer / api-key / token assignment forms; good. It will not catch provider keys in other shapes (e.g. a bare Google AIza token) if they ever appear inline in an error message. Probably out of scope; just noting the coverage boundary.

Tests

• Classifier, restore, executor, slash-dispatch, and status coverage all look strong and target the genuinely tricky precedence cases. One gap to consider: an end-to-end restore then resume of a blocked run asserting the source run transitions to killed/terminal_killed via finalizeResumedActiveBlockedSourceRun and that exactly one continuation run is live afterward (guarding against a double-run window if dispatchWorkflowRun and the source-run finalize are ever reordered).

Overall a solid, carefully-staged change with persistence/restore parity and broad test coverage. The main thing I would confirm before merge is the bare-401-to-terminal path against the real local-login failure shape, since that is the one place a misclassification has run-killing consequences.

Reviewed against CLAUDE.md conventions: raw-TS layout preserved, no build-step additions, Bun-only. Did not run bun test/typecheck locally (sandbox restrictions); relying on the PR reported green run.

[lavaman131]

Addressed the latest review feedback in b19da4bb.

Changes:

• Verified local auth failures in this repo include /login guidance and added a wrapper-401 regression for the actual Authentication failed...Run '/login <provider>' shape, so local login remains recoverable while bare provider 401 stays terminal.\n- Changed ambiguous bare retryAfter to follow Retry-After semantics (seconds/date); explicit retryAfterMs remains milliseconds and retryAfterSeconds remains seconds.\n- Preserved retry hints from later all-recoverable diagnostics/aggregate branches instead of always returning the first recoverable classification.\n- Added restore-then-resume coverage for active-blocked runs: restored source run is terminalized as killed/terminal_killed/non-resumable, and only the continuation remains live during resume.\n- Added a short candidate precedence comment and collapsed duplicate aggregate/outer metadata branches in the executor.\n\nValidation passed:\n- AGENT=1 bun test test/unit/workflow-failures.test.ts test/unit/slash-dispatch.test.ts --timeout 30000\n- AGENT=1 bun test test/unit/persistence-restore.test.ts test/unit/executor.test.ts test/unit/background-status.test.ts test/unit/background-status-kill.test.ts test/unit/persistence-session-entries.test.ts test/unit/slash-dispatch.test.ts test/unit/workflow-failures.test.ts --timeout 30000\n- AGENT=1 bun test test/integration/runtime-tunables.test.ts --timeout 30000\n- AGENT=1 bun run typecheck\n- AGENT=1 bun run lint\n- git diff --check\n- pre-commit/pre-push hooks including bun run test:unit

[claude]

Code Review — typed failure taxonomy with active-blocked lifecycle

Nicely scoped and impressively thorough work. The taxonomy is coherent, the persistence/restore round-trips are carefully wired, and the test coverage is genuinely strong (classifier precedence, aggregate handling, redaction, restore, disposition routing). Type-additive surface changes are all optional/guarded for safe deserialization. Feedback grouped by theme below — mostly observations plus a few things worth a closer look.

Potential bugs / correctness

1. Blocked runs are reported as running indefinitely with no auto-resume path. recordRunBlocked keeps the run running and workflowDetailsFromRun reports running. retryAfterMs is explicitly "informational" (resume only via explicit user action). Net effect: a workflow that trips a transient rate_limited/provider_unavailable failure sits as running forever until a human manually resumes. Defensible, but two concerns:

   • A user scanning status sees running for a workflow that is actually stalled — easy to misread as healthy/in-progress. Consider surfacing blockedAt/failureCode so running (blocked: rate_limited) is distinguishable from a healthy running.
   • retryAfterMs is captured but never acted on. If there's no follow-up to auto-resume after the hint elapses, it's purely diagnostic — worth a comment/issue noting the intent.

2. All HTTP 401s classify to terminal, non-recoverable invalid_api_key (terminal_killed). decisionFromStatus(401) → authDecision("invalid_api_key"), which kills the run and forces a brand-new run. A provider 401 from an expired-but-refreshable OAuth token (vs. a genuinely wrong key) is arguably recoverable via re-auth + resume, but here it's terminal. The canUseLoginClassificationBeforeWrapper401 guard only rescues cases with an accompanying local-login signal. If any provider in this stack emits a bare 401 for expired-but-refreshable creds, those users lose their in-flight run. Worth confirming against real provider behavior.

3. restoreBlockedStageState leaves unrelated in-flight stages as running after restore (by design), creating apparent zombies. Unlike the crash path (which marks unended stages failed), the blocked path leaves non-descendant unended stages running with no live executor. On resume these presumably get replayed, but if the user never resumes, status shows perpetually-running stages under a perpetually-running run. Please confirm the continuation/replay path actually re-drives or reconciles these stages; otherwise they're orphaned.

Code quality / maintainability

4. workflow-failures.ts is now ~1000 lines of dense, mutually-recursive classification logic. Well-organized and well-tested, but the precedence rules are spread across structuredClassification, relatedStructuredClassification, selectDiagnosticFailureClassification, aggregateClassification, the STATUS_*_REFINEMENT_CODES sets, and several canUse* predicates. The ordering invariants (strong code → name → broad-auth message refinement → related → status → message) are subtle and only documented implicitly. A short doc comment at the top of structuredClassification spelling out the precedence ladder would pay for itself the next time someone debugs a misclassification.

5. Disposition/recoverability precedence is duplicated between classifier and executor. aggregateClassification / selectDiagnosticFailureClassification (workflow-failures.ts) and selectRunFailureDisposition (executor.ts) both implement the same severity ladder: "terminal_killed beats terminal_failed beats recoverable-blocked, and blocked only survives if all candidates are blocked." Two copies will drift. Consider a single shared comparator (e.g. dispositionSeverity(disposition, recoverability)) used by both.

6. executorAggregateErrorItems duplicates aggregateErrorItems from workflow-failures.ts. Same logic in two files — export and reuse one.

7. runFailureMetadata, runFailureMetadataFromStage, runFailureMetadataFromFailure, runFailureMetadataFromCandidate are four very similar builders. Some consolidation (or a comment on why each variant exists) would reduce the surface area.

Performance

8. classifiedFailures cache is a good call — classification is expensive and previously ran multiple times per error. Keyed on error identity, lifetime = run; fine.

9. aggregateClassification / selectDiagnosticFailureClassification copy the seen set per branch (new Set(seen)). O(n²) set copies for deeply nested aggregate/diagnostic trees. Realistically these trees are tiny, so a non-issue — just flagging.

Security

10. Redaction looks solid and applied at the right boundary. message/userMessage are redacted before reaching snapshots/persistence, and cause (the raw error) is intentionally not persisted onto snapshots — good, that's where raw secrets would otherwise leak. The sk-[A-Za-z0-9_-]{8} rule preserves an 8-char prefix; for sk-ant-... keys that exposes sk-ant-a…, low-risk but worth a conscious decision. Consider also covering Authorization: Basic <base64> and query-param ?key= / access_token= patterns if those ever appear in provider error text.

Tests

11. Coverage is strong across classifier, restore, executor lifecycle, and dispatch. Two gaps worth considering:
    • An end-to-end "block → restore → resume → source run finalized as terminal_killed" round trip exercising finalizeResumedActiveBlockedSourceRun, locking in that resuming a blocked run closes out the source with no dangling active-blocked entry.
    • A test for the "blocked run later killed" path in store.recordRunEnd (the wasBlocked + clearStaleBlockedRunMetadata branch) asserting stale blockedAt / failureDisposition: active_blocked don't leak into the terminal entry.

Nits

• resumeRun not-resumable guard broadened from failed && endedAt && resumable===false to killed || resumable===false — looks intentional/correct (completed runs fall through since resumable is undefined), but it's a behavior change worth a changelog note.
• CHANGELOG.md ## [Unreleased] — didn't see an entry in the diff for this user-facing lifecycle change; per CLAUDE.md it should be recorded.

Overall: high-quality, defensive implementation. The main things I'd want answered before merge are #1 (stalled-running UX), #2 (401 recoverability), and #3 (orphaned running stages on restore). The maintainability items (#4–#7) are good follow-ups, not blockers.

_{🤖 Generated with Claude Code — static review; bun run typecheck/bun test not re-run in this environment (author reports both passing).}