fix(subagents,workflows): add attempt watchdog and request-incompatible fallback classification

[flora131]

Summary

Fixes the #1580 stall end-to-end and hardens model fallback chains in subagents and workflows.

Issue #1580 reported a parallel subagent run stuck at 0/3 done for 2h34m: the first candidate failed fast (missing API key, already retryable), but the second candidate's request hung with 0 turns/0 tokens, and await runPiStreaming(...) had no idle watchdog or per-attempt timeout — so the attempt (and the whole batch) blocked until manual abort. Separately, request/context-incompatibility failures (HTTP 400/413/422, context-length overflow, unsupported tool/parameter) were classified non-retryable, so a chain that did fail with one of those never reached the current user-selected model as the last resort.

Closes #1580

Changes

Per-attempt idle watchdog + wall-clock cap (the #1580 hang fix)

• New packages/subagents/src/runs/shared/attempt-watchdog.ts: every model attempt now runs under an idle/no-progress watchdog (default 5 min, reset on every streamed child event/stdout/stderr activity) and an absolute wall-clock cap (default 60 min). On trip, the child is killed (SIGTERM → SIGKILL after a 3s grace) and the attempt resolves with a synthetic "...timed out..." failure that the existing classifier treats as retryable, so the fallback loop advances to the next candidate instead of blocking forever.
• An in-flight tool execution counts as activity, so a slow, quiet tool call (a long build or test run that streams nothing until it finishes) does not falsely trip the idle window — only the wall-clock cap bounds such attempts. Wired into both the foreground and background spawn paths.
• Wired into both the background path (subagent-runner-step.ts / subagent-runner-streaming.ts) and the foreground path (execution-run-sync.ts / execution-attempt.ts), including chain/parallel executors.
• Overridable via ATOMIC_SUBAGENT_ATTEMPT_IDLE_TIMEOUT_MS, ATOMIC_SUBAGENT_ATTEMPT_TIMEOUT_MS, and ATOMIC_SUBAGENT_ATTEMPT_KILL_GRACE_MS. Setting the idle or wall-clock override to 0 (or a negative value) now disables that timeout entirely; non-numeric values remain ignored and fall back to the default.

Pre-spawn candidate filtering

• New packages/subagents/src/runs/shared/model-candidate-filter.ts: candidates whose provider is known and affirmatively has no configured API key/auth are skipped before spawning a child, recorded as skipped modelAttempts so the decision is visible in artifacts. Unknown/custom providers are still attempted (they already fail fast), and the current user-selected model appended as last resort is never filtered out.
• Extracted collectKnownModelProviders() in packages/subagents/src/shared/model-info.ts to replace four duplicated model-registry provider-derivation expressions.
• The background runner (subagent-runner-step.ts) now mirrors the foreground's empty-candidates handling: an empty modelCandidates array with no pre-spawn skipped attempts means no candidates were ever configured (no primary, no fallbacks, no current model), so it spawns one default-model attempt instead of silently exiting with no attempt and no error. A list that was filtered down to empty (which always carries skipped attempts) is still respected and surfaced as an error.

request_incompatible fallback classification

• Adds a request_incompatible ModelFallbackFailureKind to both packages/workflows/src/runs/shared/model-fallback-failures.ts and packages/subagents/src/runs/shared/model-fallback.ts, included in each package's FALLBACKABLE_FAILURE_KINDS.
• Classifies HTTP 400/413/422, request-too-large, context-window/context-length overflow, unsupported tool/parameter, and invalid_request/bad_request/too_large-style codes and messages as request_incompatible via status codes, error codes, and message patterns.
• Preserves precedence for non-retryable signals — refusals, content-filter/safety blocks, cancellations, and task failures still stop the chain.
• Direct-message classification in both the subagents and workflows classifiers now runs after nested cause/diagnostic traversal (and before outer status/code) so a generic wrapper message (e.g. "400 bad request") can't mask a non-retryable nested signal — the two classifier copies are aligned on this precedence and a new cross-package conformance test (model-fallback-classifier-conformance.test.ts) runs a shared failure corpus through both to guard against future silent drift.

Key files

• packages/subagents/src/runs/shared/attempt-watchdog.ts (new)
• packages/subagents/src/runs/shared/model-candidate-filter.ts (new)
• packages/subagents/src/runs/shared/model-fallback.ts
• packages/workflows/src/runs/shared/model-fallback-failures.ts
• packages/subagents/src/runs/background/{subagent-runner-step,subagent-runner-streaming}.ts
• packages/subagents/src/runs/foreground/{execution-run-sync,execution-attempt,chain-execution*}.ts
• packages/subagents/src/shared/model-info.ts

Tests, docs, changelogs

• test/unit/subagents-attempt-watchdog.test.ts: watchdog trips on a stalled child and the fallback loop advances with a synthetic retryable failure; idle timer resets on child activity and on in-flight tool execution (no false kills, foreground and background); wall cap trips independently of tool activity; SIGTERM→SIGKILL escalation; pre-spawn filter skips known keyless providers but never unknown providers or the current-model last resort; background default-attempt regression when no candidates are configured; idle/wall timeout disabled via 0/negative env override; non-numeric env values ignored.
• test/unit/model-fallback-request-incompatible.test.ts, test/unit/model-fallback-classifier-conformance.test.ts, test/unit/subagents-model-fallback.test.ts, test/unit/stage-runner-fallback-resume.test.ts: classifier behavior, subagents/workflows classifier parity, and an end-to-end workflow fallback loop reaching the current user-selected model.
• Docs: packages/coding-agent/docs/subagents.md (watchdog/timeouts/tool-activity deferral/pre-spawn filtering/env disable escape hatch) and packages/coding-agent/docs/workflows.md (request-incompatible fallback).
• Changelog entries under [Unreleased] in packages/coding-agent, packages/subagents, and packages/workflows.

Validation

• bun test test/unit/subagents-attempt-watchdog.test.ts test/unit/subagents-model-fallback.test.ts test/unit/model-fallback-request-incompatible.test.ts test/unit/model-fallback-classifier-conformance.test.ts test/unit/stage-runner-fallback-resume.test.ts — all pass
• bun run test:unit — green (run in the pre-push hook)
• bun run typecheck / bun run lint — pass
• bun run check:file-length — pass (watchdog and filter extracted into new ≤100-line modules; no file pushed over the 500-line gate)

QA E2E video

No QA E2E video applies; this is backend runner/classifier behavior with no browser/UI scenario. The watchdog and fallback-loop regression tests provide executable proof.

[flora131]

Implementation Notes

Task: Investigate and fix GitHub issue #1580 in the Atomic monorepo. Work in a separate git worktree named ../atomic-issue-1580-model-fallbacks (create it from origin/main if needed). Determine why the rest of the model fallback candidates did not get tried. Implement the required behavior: if none of the configured fallback candidates are applicable/usable for the current request/context, fall back to the currently selected user model rather than stopping without an applicable fallback. Preserve existing fallback behavior when candidates are applicable. Add or update focused tests that reproduce the issue and verify the selected user model fallback path. Update relevant user-facing docs in packages/coding-agent/docs and package changelog(s) under ## [Unreleased] if behavior changes. Use Bun commands only (bun test, bun run typecheck/lint as appropriate). Create a pull request when complete with a clear summary and validation evidence.

Running Notes

• Record implementation decisions, deviations from the research findings, tradeoffs, blockers, validation notes, and anything else the user should know.

Implementation Decisions

• Preflight found the worktree was a Bun workspace checkout with missing dependencies; bun install was run successfully before implementation.
• Root cause confirmed by subagent analysis: the workflow and subagent fallback loops already append/advance through candidate models, including the current selected user model, but the retryability classifiers marked request/context incompatibility failures as unknown/non-retryable. That stopped the chain before later configured candidates or the selected user model.
• Implemented a dedicated request_incompatible fallback kind in both workflow and subagent classifiers instead of overloading provider/transport failures. This keeps audit metadata specific and preserves existing behavior for other fallback causes.
• Classification is intentionally narrow: HTTP 400/422 plus curated request/context-incompatibility codes/messages (invalid/bad request, context length/window overflow, request too large, unsupported/unknown/invalid tool/parameter/function). Other 4xx statuses are not made broadly retryable.
• Refusal, content-filter/safety, cancellation, and task-failure precedence remains non-retryable and continues to win over request-incompatible signals.
• The implementation subagent refactored the subagents kindFromCode switch into a set/table lookup to satisfy the repository's 500-line source-file gate. This was a small behavior-preserving refactor but is a notable deviation from the minimal research guidance.
• The workflows fallback classifier is now close to the file-length gate (validation reported 499 lines); future additions should consider splitting it.

Continuation Update — Reviewer Precedence Fix

• Latest research identified an unresolved reviewer finding: the workflows classifier could classify generic wrapper messages such as invalid request / 400 bad request as retryable before inspecting nested diagnostics or causes for non-retryable content-filter, safety, AbortError/cancellation, or task-failure signals.
• Implemented the minimal precedence fix in packages/workflows/src/runs/shared/model-fallback-failures.ts: direct-message fallback classification now runs after diagnostics and cause traversal, while direct refusal/cancellation checks remain ahead of fallbackable request-incompatible classification.
• Added focused workflow classifier regression tests proving nested non-retryable causes/diagnostics win over retryable wrapper messages and genuine request-incompatible wrappers still classify as fallbackable.
• No subagent classifier change was needed for this continuation because the reviewer finding was specific to the workflows classifier ordering.
• The workflows classifier is now exactly at the 500-line gate; validation passes, but any future additions should split the file rather than adding more lines.

Continuation Update — HTTP 413 Payload Too Large

• Latest research identified another unresolved reviewer finding: both classifiers mapped HTTP 400/422 to request_incompatible but omitted bare HTTP 413 Payload Too Large.
• Implemented HTTP 413 status classification in both packages/workflows/src/runs/shared/model-fallback-failures.ts and packages/subagents/src/runs/shared/model-fallback.ts. This covers { status: 413 }, { statusCode: 413 }, { httpStatus: 413 }, { code: 413 }, and { code: "413" } because status extraction and code-to-status classification already route through kindFromStatus.
• Added focused 413 tests in workflow and subagent fallback classifier test coverage. Docs and changelog wording now mention HTTP 400/413/422 and payload/request-too-large conditions.
• Scope remains narrow: only 400/413/422 are classified as request-incompatible; other 4xx statuses are not broadly retryable, and refusal/cancellation/content-filter/task-failure precedence is unchanged.
• The workflows classifier remains exactly at the 500-line gate, so future changes should split/refactor before adding more cases.

Validation Outcomes

• Focused tests passed after the final HTTP 413 update: bun test test/unit/model-fallback-01.test.ts test/unit/model-fallback-request-incompatible.test.ts test/unit/subagents-model-fallback.test.ts test/unit/stage-runner-fallback-resume.test.ts (55 pass, 0 fail).
• Type checking passed after the final HTTP 413 update: bun run typecheck.
• Lint passed after the final HTTP 413 update: bun run lint.
• File-length gate passed after the final HTTP 413 update: bun run check:file-length.
• Final validation confirmed modified implementation, test, docs, and changelog files plus a new focused workflow classifier test file.

Blockers / Deferred

• No functional blockers remain.
• No pull request was created in this stage because the orchestrator role instruction explicitly said to ignore PR submission requests.
• Untracked orchestration artifacts remain in the worktree (subagent-*.md, progress.md, and existing research artifacts); validation recommended staging only implementation/test/docs/changelog files in a later PR stage.

QA E2E Video

• No QA E2E video applies: the change is backend/classifier model-fallback behavior with no browser/UI scenario. The executable behavior proof is the workflow fallback loop regression in test/unit/stage-runner-fallback-resume.test.ts.

[mintlify]

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

Project	Status	Preview	Updated (UTC)
bastani	🟢 Ready	View Preview	Jul 1, 2026, 7:06 PM

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

[claude]

Review: fix: retry request-incompatible model fallbacks

Thanks for this — the classifier change itself is clean, well-factored, and the test coverage is genuinely strong (status fields, numeric/string codes, message regexes, and non-retryable precedence are all exercised, plus an end-to-end stage-runner fallback loop). A few things worth discussing, one of them important.

🔴 Scope vs. the linked issue (#1580)

The PR says Closes #1580, but #1580's reported root cause is a subagent model attempt that hangs indefinitely (2h34m, 0 turns) because runPiStreaming has no idle watchdog and no per-attempt wall-clock timeout. Its "Expected behavior" / "Suggested fixes" are all about adding timeouts and a no-progress watchdog and treating a stalled attempt as retryable.

This PR does not touch any of that. It only makes HTTP 400/413/422 + request/context-incompatibility failures fallbackable. Notably, neither failure in the issue's evidence is a request-incompatibility — attempt #1 was No API key found for zai (already retryable) and attempt #2 hung with an empty error. So this change would not have prevented the reported stall.

This is a reasonable, useful improvement on its own, but I'd suggest not auto-closing #1580 with it (change Closes -> Refs #1580 / "contributes to"), or the actual hang bug will silently disappear from the tracker while the watchdog/timeout work remains undone.

🟡 Breadth of HTTP 400 -> request_incompatible

400 is a very broad bucket. A genuinely malformed request that Atomic constructs (an actual bug, not a model incompatibility) will now be classified retryable and burn through every configured candidate plus the current user model — all failing identically with 400 — before surfacing the error, instead of failing fast. That's added latency/cost on a real bug. The refusal/cancel/task_failure precedence guards this against the dangerous cases, so I think it's an acceptable trade, but worth a code comment noting the intent, since "400 is always fallbackable" is a strong claim.

🟡 The two classifiers have drifted in shape

packages/subagents/.../model-fallback.ts and packages/workflows/.../model-fallback-failures.ts are near-duplicate classifiers that must stay behaviorally in sync, but this PR diverges their structure:

• subagents refactored kindFromCode into a CODE_KINDS_BY_KIND table; workflows kept the switch.
• The REQUEST_INCOMPATIBLE_FAILURE_PATTERNS regexes are written differently in each file (subagents merges context length|window and max context|tokens into single alternations; workflows keeps four separate patterns). Functionally equivalent today, but easy to let them drift apart tomorrow.
• The masking fix (direct-message classification after nested traversal) was applied to structuredSignal in workflows; subagents didn't need it because it already classifies the direct message only in normalizeModelFailureSignal after structuredSignal returns. Correct outcome, but the asymmetry makes it harder to reason about "are these two still equivalent?"

Not blocking, but since these files are effectively forks of one another, keeping the two edits structurally parallel (or extracting the shared table/patterns) would reduce future drift risk.

🟢 The structuredSignal reorder is correct

Moving fallbackSignalFromDirectMessage below the diagnostic/cause traversal is the right call: nested non-retryable signals (cancelled/task_failure) return early via isRefusalSignal, so a generic "invalid request" wrapper can no longer mask a non-retryable nested cause. Retryable-vs-retryable precedence (direct message over outer status/code) is preserved since direct-message classification already ran before those. The regression tests for this ("non-retryable nested causes win over retryable wrapper messages") are exactly the right shape.

🟢 Minor

• packages/workflows/src/runs/shared/model-fallback-failures.ts is now at exactly 500 lines — right at the check:file-length ceiling. It passes, but there's zero headroom; the next edit to this file will trip the gate.
• max_tokens in REQUEST_INCOMPATIBLE_CODES and the /\bmax[_\s-]?tokens?\b/i message pattern are a bit broad (max_tokens is normally a legit parameter name), but it only matters when it appears as an error code/message, so low risk.
• // @ts-nocheck on the new test file is consistent with repo convention (~1/3 of test/unit files use it), so no concern there.

Verdict

Code quality and tests are solid; the classifier logic is correct as far as I can trace it. My one real ask is to reconcile the PR with #1580 — either implement the timeout/watchdog the issue actually reports, or downgrade Closes to Refs so the hang bug stays open. The rest are nits/notes.

🤖 Automated review — verified the classifier logic and ordering by reading both source files; did not execute the test suite in this environment (the PR reports 55 pass, typecheck/lint/file-length green).

[claude]

Code Review — PR #1581: advance model fallback on request-incompatible errors

Thanks for this — the classifier fix is well-targeted and the regression coverage is genuinely thorough (status fields, numeric/string codes, message regexes, and the crucial "incompatibility must not outrank refusals/cancellations" precedence tests). The reordering of direct-message classification in the workflows structuredSignal so a generic "400 bad request" wrapper can't mask a nested cancel/refusal is a nice, subtle correctness improvement. Below is my feedback, roughly in priority order.

🔴 Scope: two distinct changes bundled under one issue

The PR's stated purpose (#1580) is the request_incompatible classifier fix, but the second commit adds a whole per-attempt watchdog + pre-spawn keyless-provider filter subsystem (attempt-watchdog.ts, model-candidate-filter.ts, threading knownModelProviders through ~10 files). The watchdog kills child processes — a materially riskier change than a classifier tweak, and both changelog entries point at #1580. Consider splitting the watchdog into its own PR/issue so it gets review attention proportional to its risk, and so a revert of one doesn't drag the other.

🟠 Behavioral change: empty candidate list now produces zero spawn attempts

In subagent-runner-step.ts the guard changed from step.modelCandidates && step.modelCandidates.length > 0 to step.modelCandidates !== undefined. Combined with the new pre-spawn filter, if filterSpawnableModelCandidates strips every candidate (all providers known-but-keyless and no current model to preserve), candidates is now [], the for loop never runs, and the step returns exitCode: 1 with error: undefined (only the skip note in the output). Previously an empty array fell through to [step.model] and at least attempted it. This is likely the intended improvement (don't spawn a doomed keyless attempt), but:

• Returning exitCode 1 with error === undefined is a slightly odd terminal state — downstream error handling keys off error in places. Consider synthesizing an error from the skip notes so the failure reason propagates, not just the summary output.
• Worth a one-line comment at that guard explaining that an empty (fully-filtered) array is intentionally respected, since !== undefined vs .length > 0 is exactly the kind of thing a future reader "simplifies" back.

🟠 Broad message/code patterns may over-fallback on genuinely fatal requests

context_length_exceeded and friends are now request_incompatible → fallbackable. But a context-window overflow is fundamentally "your input is too big" — falling back to the current selected model (the last resort), which may have the same or smaller window, won't help. The net effect for a truly oversized turn is: fan out failing attempts across every candidate + the current model, burning latency/tokens, before finally erroring — where before it stopped immediately. That's an accepted tradeoff for #1580's "reach the current model" goal, but it's worth calling out in the issue/docs that context-length failures specifically may just be N wasted retries.

Separately, a couple of message patterns are broad enough to risk false positives:

• `/\bmax[_\s-]?tokens?\b/i` — "max tokens" is normal output-truncation vocabulary, not necessarily a request incompatibility. If any provider surfaces a max-output-tokens stop as an error string, this misclassifies it as retryable. Context-scoped matching would be safer than a bare "max tokens".
• `too_large` / `/\btoo[_\s-]?large\b/` is comparatively specific — lower risk.

These only fire on the error path (after refusal/cancel precedence), so the blast radius is limited, but tightening max[_\s-]?tokens is worth considering.

🟡 Duplicated knownModelProviders derivation (DRY)

This exact expression is copy-pasted into 5 foreground executors:
```ts
const knownModelProviders = [...new Set((typeof ctx.modelRegistry.getAll === "function"
? ctx.modelRegistry.getAll() : ctx.modelRegistry.getAvailable()).map((m) => m.provider))];
```
Extract a knownModelProviders(ctx.modelRegistry) helper — one place to fix if the registry API or the getAll fallback logic changes.

🟡 // @ts-nocheck in a new test file

test/unit/model-fallback-request-incompatible.test.ts starts with // @ts-nocheck. Given CLAUDE.md's strict-typing stance (no any/unknown, noUnusedLocals), blanket-disabling type checking on a brand-new file is a step back — it also means the test won't catch signature drift in normalizeModelFailureSignal/isRetryableModelFailure. The sibling subagents-model-fallback.test.ts additions don't need it; prefer typing the failure fixtures (or a small type Failure = Record<string, unknown>) over suppressing the whole file.

🟢 Minor

• attempt-watchdog.ts: clean design (idle + wall-clock + SIGTERM→SIGKILL grace, all unref'd). On trip() the wallTimer isn't explicitly cleared, but its own callback re-checks tripped/isSettled so it no-ops — fine, just noting it's load-bearing.
• The asymmetry where only the workflows classifier reorders direct-message classification is actually correct: the subagents structuredSignal never classifies the direct message inline (it's deferred to normalizeModelFailureSignal), so it can't mask a nested signal. Good that you didn't blindly mirror the change — a brief comment noting why subagents doesn't need the reorder would help the next reader.
• Docs and per-package changelogs are updated consistently with the code — appreciated.

Test coverage

Strong on the classifier and the workflow end-to-end fallback loop. Two gaps worth a test each:

1. The empty-filtered-candidates path in subagent-runner-step.ts (all providers keyless, no current model) — asserting the resulting exit code/error is what you intend.
2. A subagents-side nested-cause case ({ message: "400 bad request", cause: { message: "request was cancelled" } } → cancelled) to lock in the subagents classifier's precedence, mirroring the workflows test.

Overall: solid fix with good tests. The main asks are splitting the watchdog out (or at least surfacing its risk in review), and deciding intentionally about the empty-candidate error === undefined terminal state.

Reviewed with Claude Opus 4.8.

[claude]

Review: fix(subagents,workflows): harden model fallback resilience

Thanks for this — it's a well-structured, well-tested hardening pass. The precedence design (non-retryable refusals/cancels/task-failures still stop the chain, everything request/context-incompatible advances) is sound, and coverage across classifier precedence, nested-cause masking, watchdog kill+advance, and pre-spawn filtering is genuinely thorough. A few things worth considering before merge.

Potential issues

1. Idle watchdog can kill a healthy attempt during long silent tool calls (medium confidence)
attempt-watchdog.ts resets the idle timer only on child stdout/stderr data (execution-attempt.ts / subagent-runner-streaming.ts wire attemptWatchdog.activity() into the data handlers). A subagent that runs a single long, quiet tool — a >5 min build, bun install, or a large test suite that streams nothing until it finishes — produces no interim child output and will trip the 5-minute DEFAULT_IDLE_MS, get SIGTERM'd, and be recorded as a retryable timeout that advances the chain. Note the foreground activityTimer (execution-attempt.ts:350) is UI-progress only and is not fed to the watchdog, so it won't rescue such an attempt. The default is generous and overridable, but a legitimately busy child classified as "hung" is a real failure mode. Consider treating an in-flight tool call as activity, or at least calling this out prominently in the docs (they currently frame idle purely as "no activity," implying stall).

2. knownModelProviders computation is duplicated verbatim 4x
The exact expression

[...new Set((typeof ctx.modelRegistry.getAll === "function" ? ctx.modelRegistry.getAll() : ctx.modelRegistry.getAvailable()).map((m) => m.provider))]

appears in subagent-executor-single.ts:79, subagent-executor-parallel.ts:111, subagent-executor-async.ts:79, and chain-execution.ts:120. Worth extracting to a small typed helper (e.g. collectKnownModelProviders(registry)). The typeof getAll === "function" guard also suggests the registry type doesn't declare getAll() — adding it to the type (or the helper signature) would be cleaner than a runtime typeof probe repeated four times.

3. Two parallel copies of the classifier can drift
The request-incompatible codes/patterns and the structuredSignal ordering now live in both packages/workflows/.../model-fallback-failures.ts and packages/subagents/.../model-fallback.ts, and they have already diverged slightly (workflows split context-length/context-window into two regexes; subagents kept a combined one; the direct-message reorder was only applied to the workflows copy). The subagents copy is still correct because non-retryable nested signals early-return via isRefusalSignal (lines 414/421) before statusKind/codeKind are returned (424-426), so a wrapper 400 cannot mask a nested refusal there. Flagging only because these are maintained as parallel copies — a shared module or a cross-package conformance test would reduce drift risk.

Minor

• @ts-nocheck in test/unit/model-fallback-request-incompatible.test.ts disables all type-checking for the file, inconsistent with the sibling subagents-model-fallback.test.ts which is fully typed. Given the repo's strict no-any/no-unknown stance, prefer typed fixtures here.
• Broad too_large matching: /\btoo[_\s-]?large\b/i and the too_large code are generic enough to catch unrelated "too large" errors and reclassify them as retryable request_incompatible. Low risk (failure path only), but note the tradeoff: a genuinely fatal bad-request that fails identically on every model now walks the whole fallback chain + the current model before surfacing, spending extra latency/tokens. Intended per Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580, just worth being explicit about the cost.
• Test gaps: the watchdog tests exercise the idle path and kill+advance well, but the wall-clock cap (ATOMIC_SUBAGENT_ATTEMPT_TIMEOUT_MS) and the SIGTERM->SIGKILL escalation (killGraceMs) are not directly exercised — the stalled fake child trips idle first and a plain node process exits on SIGTERM. A focused test for each would lock in that behavior.
• subagent-runner-step.ts candidate guard change: switching from step.modelCandidates && length > 0 to step.modelCandidates !== undefined means an explicitly empty modelCandidates array now means "try nothing" rather than falling back to step.model/default. Correct for the pre-spawn-filter case (guarded by the new candidates.length === 0 && modelAttempts.length > 0 error synth), but if any caller passes [] with no modelAttempts it yields an empty result rather than a default attempt. Worth confirming no caller relied on the old default-on-empty behavior.

Nice work

Precedence handling, the exitCode === null convention to mark pre-spawn-skipped attempts, threading modelAttempts through the attempt-note bookkeeping, .unref?.() on all timers, and the docs/changelog updates across all three packages are all clean and correct.

Automated review — no test run in this environment; relied on the PR's stated bun test/typecheck/lint validation plus source reading.

[flora131]

Re: the review's scope concern — resolved by expanding this branch to actually fix the hang rather than downgrading to Refs. Two follow-up commits (eb91530e0, 21f034ecd) add a per-attempt idle watchdog (default 5 min, reset on any child activity) plus an absolute wall-clock cap (default 60 min) around runPiStreaming in both background and foreground paths; a tripped attempt is killed and recorded as a synthetic retryable failure so the fallback loop advances. Candidates whose provider affirmatively has no configured key are now skipped pre-spawn (recorded as skipped attempts; unknown/custom providers and the current-model last resort are never filtered). Closes #1580 now reflects the actual root-cause fix. The 500-line headroom note was also addressed by extracting the new logic into dedicated modules.

[claude]

Review: fix(subagents,workflows) — attempt watchdog + request-incompatible fallback

Reviewed the watchdog, the pre-spawn candidate filter, the classifier changes, and the wiring across the foreground/background/chain/parallel paths. This is a well-scoped, well-tested fix for #1580. Feedback below, roughly by severity.

Strengths

• The core fix is sound and layered. The idle watchdog (reset on any stream/stdout/stderr activity) catches the exact 0 turns/0 tokens hang from Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580, and the independent wall-clock cap bounds pathological cases the idle timer can't (e.g. a wedged tool call). Resolving a trip into a synthetic "…timed out…" failure that the existing classifier already treats as retryable is a clean way to reuse the fallback loop rather than bolting on new control flow.
• SIGTERM → SIGKILL escalation with isSettled() guards correctly avoids double-signalling a child that exits during the grace window, and every exit path (close/error/finish) calls attemptWatchdog.clear(), so no dangling timers. Timers are unref()'d throughout.
• Excellent test coverage. subagents-attempt-watchdog.test.ts drives real subprocesses through a fake CLI and asserts end-to-end behavior (stall → advance, idle reset on activity, wall cap, tool-active deferral, SIGTERM→SIGKILL). The cross-package model-fallback-classifier-conformance.test.ts is a great call — the two classifier copies (subagents/…/model-fallback.ts and workflows/…/model-fallback-failures.ts) can no longer silently drift.
• Precedence handling is careful: refusals, content-filter/safety blocks, cancellations, and task failures still short-circuit the chain, and moving direct-message classification after nested cause/diagnostic traversal (so a generic "400 bad request" wrapper can't mask a nested AbortError/content-filter) is the right ordering. The conformance fixtures exercise these wrapper permutations directly.
• Docs (subagents.md, workflows.md) and all three changelogs updated per the repo conventions; new modules extracted to stay under the 500-line gate.

Things worth a closer look

1. (medium) 5-minute idle default vs. slow first token. The idle watchdog trips after 5 min of no streamed output and no active tool. A legitimate request that streams nothing before the first token — very large context, a slow/queued provider, or a reasoning model with a long pre-token phase — could be killed spuriously. It degrades gracefully (retryable → next candidate / current-model last resort) rather than hanging, so this is a soft regression at worst, but it's worth confirming 5 min sits comfortably above real p99 first-token latency for the providers you target, or surfacing the ATOMIC_SUBAGENT_ATTEMPT_IDLE_TIMEOUT_MS escape hatch more prominently in the docs.

2. (low) A hung tool is bounded only by the 60-min wall cap. Because an in-flight tool_execution_start with no matching tool_execution_end keeps isToolActive() true, the idle timer re-arms indefinitely and a truly wedged tool call stays alive up to an hour. This is the documented, intentional tradeoff (don't kill a slow-but-healthy build), and Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580's hang was pre-tool so it's still caught at 5 min — just flagging that the worst-case wedged-tool latency is now ~60 min.

3. (low) Fuzzy message-based request_incompatible classification. The too large / invalid request message patterns can catch benign strings — e.g. a tool result mentioning output too large would be reclassified retryable and retried on another model, while conversely a genuine too-large error whose message happens to contain abort/cancel would be classed cancelled (non-retryable) and stop the chain. The status-code and error-code paths are far more robust; the message regexes are inherently best-effort. Not blocking — just be aware the message layer trades precision for recall here.

4. (nit) Both classifier files are at 499/500 lines, i.e. right at the gate — the next change to either forces an extraction. Given they're maintained as parallel copies (mitigated well by the conformance test), consider whether the shared classifier logic could eventually live in one place both packages import, rather than two hand-kept copies.

5. (question) Workflow-level protection. The watchdog is wired into the subagents runner paths; workflow stages get it transitively via the subagents they spawn, which covers Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580. Just confirming there's no workflow code path that issues a model request directly (outside a spawned subagent) that could hang the same way without a watchdog.

Overall this is a solid, defensive fix with strong tests. The items above are mostly tuning/edge-case considerations rather than blockers.

Reviewed by Claude (Opus 4.8).

[claude]

Review: model-attempt watchdog + request-incompatible fallback (#1580)

Overall this is a strong, well-scoped fix. The hang is addressed at the right layer (per-attempt watchdog wired into both spawn paths), the synthetic timeout is genuinely retryable, and the tests/docs/changelogs are thorough. A few notes below, one of which I think is a real behavior regression worth addressing.

🔴 Foreground/background asymmetry on empty candidates (likely regression)

The foreground path deliberately distinguishes "every candidate was filtered out" from "there were no candidates to begin with":

packages/subagents/src/runs/foreground/execution-run-sync.ts:107

const modelsToTry = candidates.length > 0 ? candidates : (rawCandidates.length === 0 ? [undefined] : []);

So a default-model agent (no explicit model, no fallbackModels) still spawns one [undefined] default attempt.

The background path does not make that distinction. With the guard now keyed on !== undefined:

packages/subagents/src/runs/background/subagent-runner-step.ts:68 and the synthetic-error guard at :214

const candidates = step.modelCandidates !== undefined ? step.modelCandidates : step.model ? [step.model] : [undefined];
...
if (!finalResult && candidates.length === 0 && modelAttempts.length > 0) { /* "No spawnable..." */ }

buildModelCandidates() only returns defined model strings (model-fallback.ts:50-51, if (!raw) continue), so an agent with no primary, no fallbacks, and an undefined ctx.currentModel produces empty rawModelCandidates → step.modelCandidates = [] and step.modelAttempts = []. The loop then never runs, and because the synthetic-error guard requires modelAttempts.length > 0, it falls through and returns exitCode: 1 with error: undefined and no attempt.

Before this PR the .length > 0 guard fell through to [undefined] and spawned a default attempt, so this is a behavior change: a default-model background subagent (when no current model is present) now silently exits 1 with no error and no spawn, instead of running. Suggest mirroring the foreground rawCandidates.length === 0 ? [undefined] : [] distinction in the background path (or threading rawModelCandidates.length through so the guard can tell the two empty cases apart).

🟡 Minor

• positiveEnvMs can't disable the watchdog (attempt-watchdog.ts:19-24): value > 0 means ATOMIC_SUBAGENT_ATTEMPT_IDLE_TIMEOUT_MS=0 is treated as unset and reverts to the default rather than disabling the idle window. If an escape hatch to disable is desirable, consider allowing 0/negative to mean "off". Non-numeric values are also silently ignored — fine, but worth a doc note.
• Idle default vs. quiet reasoning turns: the in-flight-tool deferral is a nice touch, but a single long model generation that streams nothing for >5 min with no tool active (unusual, but possible for some providers/reasoning modes) could still be false-killed. It's overridable, so acceptable — just calling it out.
• Duplicated classifier tables: the REQUEST_INCOMPATIBLE_CODES/regex sets and the structuredSignal precedence are now duplicated across the subagents and workflows copies. The new cross-package conformance test is exactly the right mitigation for the mandated package split — good call keeping the two literally in lockstep.

✅ Strengths

• Synthetic timeout message ("...timed out...") is correctly classified retryable via /timed? out/i (model-fallback.ts:92), so the fallback loop actually advances — the core of the fix holds up.
• SIGTERM→SIGKILL escalation with unref()'d timers and an isSettled() re-check avoids signalling an already-exited child and won't keep the event loop alive.
• Precedence fix (nested non-retryable cause/diagnostic wins over a generic 400/"invalid request" wrapper) is well-tested in model-fallback-request-incompatible.test.ts and mirrored in both classifiers.
• The !== undefined empty-candidates guard is clearly commented so it won't get "simplified" back.
• Docs (subagents.md, workflows.md) and per-package changelogs are all updated with specifics.

Note: I was unable to execute the test suite in this environment, so I'm relying on the PR's stated bun test/typecheck/lint results for green status.

[flora131]

Addressed the review feedback in 776c4b0:

🔴 Foreground/background asymmetry on empty candidates — fixed in subagent-runner-step.ts. The runner now distinguishes the two empty cases using an invariant of filterSpawnableModelCandidates: every filtered-out candidate is always recorded as a skipped attempt in step.modelAttempts. So an empty modelCandidates array with skipped attempts means "filtered to empty" (respected; surfaced as the No spawnable... error), while an empty array without skipped attempts means "no candidates were ever configured" and now spawns one default-model attempt, mirroring the foreground modelsToTry distinction. Regression test added (background runner spawns one default attempt when no candidates were ever configured).

🟡 positiveEnvMs can't disable the watchdog — ATOMIC_SUBAGENT_ATTEMPT_IDLE_TIMEOUT_MS/ATOMIC_SUBAGENT_ATTEMPT_TIMEOUT_MS set to 0 (or negative) now disable the corresponding timeout entirely; non-numeric values remain ignored (default applies) and this is now documented in subagents.md. ATOMIC_SUBAGENT_ATTEMPT_KILL_GRACE_MS keeps positive-only semantics. Tests cover idle-disabled-via-0 (silent child survives idle window, bounded only by wall cap) and non-numeric/negative env resolution.

🟡 Quiet reasoning turns / duplicated classifier tables — acknowledged; no code change. The idle window stays overridable (and now fully disableable) for providers with long silent generations, and the cross-package conformance test remains the guard for the classifier copies.

bun run test:unit (2822 pass), typecheck, and check:file-length are green.

[claude]

Review: fix(subagents,workflows): add attempt watchdog and fallback classifier (#1581)

Thorough, well-tested change that fixes a real class of hangs. The three pieces — per-attempt watchdog, pre-spawn candidate filtering, and request_incompatible classification — are cleanly separated, each with its own module and focused tests. The cross-package classifier conformance test is a nice touch, and the "in-flight tool execution counts as activity" refinement shows careful thinking about false positives. Below are observations, ordered roughly by impact.

Correctness / design

1. Breadth of the request_incompatible → retryable classification (biggest thing to weigh). HTTP 400 and message patterns like too large / invalid request are frequently deterministic across providers — a malformed request body, an oversized prompt, or an unsupported parameter that Atomic itself constructs will 400 on every candidate. The chain now walks the entire fallback list (each a full child spawn + prompt) before finally 400-ing on the current model too, where previously it failed fast. This is the intended behavior per the issue (reach the user's current model as last resort), but it converts one fast failure into N slow ones for the common "the request is just bad" case. Worth confirming this cost tradeoff is acceptable, and consider whether a context-window overflow specifically should truncate/compact rather than blindly retry the same oversized payload on another model that may have a smaller window.

2. The too large message pattern is quite broad (/\btoo[_\s-]?large\b/i in workflows; the combined form in subagents). A benign model/task error message that happens to contain "too large" would be reclassified as retryable request_incompatible. The risk is bounded because refusal/cancel/task-failure patterns are checked first, but message-substring classification is inherently fragile — prefer anchoring on structured status/code where available and treating message patterns as the last resort (roughly the current order; just flagging the residual fragility).

3. Two divergent copies of the classifier. packages/subagents/.../model-fallback.ts and packages/workflows/.../model-fallback-failures.ts reimplement the same logic, and the new regex sets are structurally different between them (subagents combined several alternations that workflows kept separate). They appear behaviorally equivalent, and the conformance test guards a shared corpus — but the corpus is finite, so a divergence on an input outside it would pass CI silently. Given both packages ship raw TS, extracting the classifier into one shared module would be more robust than testing two copies for agreement. At minimum, the conformance comment should note that structural (not just behavioral) parity isn't enforced.

Watchdog

4. Foreground isToolActive depends on progress.currentTool being cleared. It's set on tool_execution_start and cleared on tool_execution_end (execution-attempt.ts:261/273). If a tool ends abnormally without emitting its end event, currentTool stays defined and the idle watchdog is deferred indefinitely — the wall-clock cap becomes the sole backstop for that attempt. Acceptable degradation (it still terminates), but worth a callsite comment so nobody later lowers the wall cap assuming idle always fires.

5. error ??= message on timeout. In both spawn paths the synthetic timeout only sets the error if none is already present. Correct for advancing the chain (the "…timed out…" text matches /timed? out/i → network_timeout → retryable, verified), and it gives a pre-existing non-retryable error precedence over the timeout — the desired behavior. Just noting it's load-bearing and the "error already set when the wall cap trips" ordering isn't directly tested.

6. killGraceMs intentionally can't be disabled (positiveEnvMs, falls back to default on 0), while idle/wall can. Sensible asymmetry, but undocumented — a one-liner in docs/comment would prevent confusion.

Minor / nits

• The empty-candidates logic in subagent-runner-step.ts (distinguishing "filtered to empty" from "never configured") is subtle but correct, and the explanatory comment is excellent — exactly the kind of guard that gets "simplified" into a regression otherwise. The parallel modelsToTry guard in execution-run-sync.ts is consistent. 👍
• collectKnownModelProviders() dedups four inline copies and the typeof registry.getAll === "function" probe is appropriately defensive. Good cleanup.
• Changelog/docs conventions followed well: per-package [Unreleased] entries, #1580 for the issue vs #1581 (pull) for review-feedback refinements, user-facing docs updated, and the new modules stay under the 500-line gate.

Tests

Coverage is strong: stall→advance, idle reset on activity, tool-active idle deferral (fg + bg), wall cap independent of tool activity, SIGTERM→SIGKILL, pre-spawn filter semantics, env escape hatches (0/negative/non-numeric), and the default-attempt-when-nothing-configured path. The classifier conformance corpus covers status/code/message and precedence cases. Main untested surfaces: the benign-too large false positive (point 2) and the abnormal-tool-end deferral (point 4).

Overall: solid fix, safe to merge once the point-1 cost tradeoff (deterministic 400s walking the whole chain, and context-overflow retrying an oversized payload) is confirmed as intended. Nice work.

Automated review — verified against the diff and source; tests were not re-run here (sandbox blocked execution), relying on the PR's stated green run.

[flora131]

Addressed the second-pass feedback in 723d21b:

1. Cost tradeoff of request_incompatible walking the chain — confirmed as intended per #1580: the explicit goal is that when no configured candidate can serve the request, the run reaches the user's currently selected model as last resort instead of failing outright. The chain is typically short (primary + a few fallbacks + current model), each incompatible attempt fails fast at request time (no long generation), and refusals/cancellations/task failures still stop immediately. The context-overflow-specific idea (truncate/compact rather than retrying the oversized payload on a possibly smaller-window candidate) is a good follow-up but out of scope for this fix — retrying a larger-window fallback is exactly the recovery #1580 asks for; a smaller-window candidate just fails fast again and the chain proceeds.

2. Breadth of the bare too large pattern — kept as-is (structured status/code and refusal/cancel/task-failure checks already run first, per the current precedence), but the residual fragility is now documented and pinned: added a conformance fixture (benign message containing too large) that documents the known false positive in both classifier copies, with a comment requiring lockstep updates if either copy tightens the pattern.

3. Two classifier copies — extracting a shared module is blocked by the package split (the copies must not import each other), so the conformance suite header now explicitly states its limitation: it enforces behavioral parity over a finite corpus only, notes the copies are structurally different, and instructs that any rule change must update both copies plus add a corpus fixture.

4. isToolActive vs. abnormal tool end — added the requested callsite comments in both execution-attempt.ts and subagent-runner-streaming.ts (idle deferred indefinitely if a tool end event is lost; the wall cap is the sole backstop — do not lower it assuming idle fires), plus a new test proving the wall cap terminates a tool-start-without-end stall and fallback advances.

5. error ??= message ordering — acknowledged as load-bearing; left as-is (pre-existing non-retryable error keeps precedence over the synthetic timeout, which is the desired behavior).

6. killGraceMs not disableable — now documented in three places: a comment at the resolver (attempt-watchdog.ts), docs/subagents.md, and both changelog entries (0/negative/non-numeric fall back to the default so escalation always stays bounded).

Also split the watchdog test suite into shared helpers + a separate pre-spawn candidate-filtering suite to stay under the repo's 500-line file gate. bun run test:unit, typecheck, and check:file-length are green.

[claude]

Review: attempt watchdog + fallback classifier (#1581)

Reviewed the full diff (new attempt-watchdog.ts, model-candidate-filter.ts, both classifier copies, and the foreground/background/chain wiring). This is a well-scoped, high-quality fix that targets the real root cause of the #1580 hang. Wiring is consistent across every spawn path, the invariant comments are genuinely helpful, and the env escape hatches have clean, well-documented semantics. Overall LGTM — comments below are mostly maintainability/cost nits, not blockers.

Note: bun test was not runnable in my sandbox (approval-gated), so I verified by reading + the PR's stated green runs rather than re-executing the suite.

What's solid

• Root-cause fix is correct. For the exact Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580 repro (2nd candidate hangs at 0 turns/0 tokens, no tool active), the 5-min idle watchdog fires, kills the child (SIGTERM→SIGKILL), and the synthetic `"...timed out..."` message classifies as `network_timeout` → retryable, so the loop advances. Confirmed the message matches `/timed? out/i` and dodges the non-retryable patterns.
• Tool-active deferral (`activeToolExecutions > 0` / `progress.currentTool !== undefined`) is the right call to avoid killing a legitimately-slow-but-quiet build/test, and the wall cap still bounds it.
• Empty-candidates handling in `subagent-runner-step.ts` (the `!== undefined` guard) correctly distinguishes "filtered to empty → error" from "nothing configured → default attempt," mirroring `execution-run-sync.ts`. Good that the skipped attempts propagate through `step.modelAttempts`.
• Cross-package conformance test is a pragmatic guard against classifier drift.

Maintainability — the main concern

• Two ~500-line parallel classifier copies, both at the file-length gate. `packages/workflows/src/runs/shared/model-fallback-failures.ts` is exactly 500 lines and `packages/subagents/src/runs/shared/model-fallback.ts` is 499. There is essentially zero headroom: the next classification rule added to the workflows copy will trip `check:file-length` and force a refactor anyway. Combined with the conformance test only enforcing behavioral parity over a finite corpus (a rule divergence on an out-of-corpus input passes CI silently), this duplication is the biggest long-term risk in the PR. The comment says a shared module is "blocked by the mandated package split," but both packages already ship raw TS bundled into `@bastani/atomic` — worth confirming whether a shared internal module (or generated/copied source with a `@generated` marker) is truly impossible, since the current state is right at the wall.

Correctness / cost nits (non-blocking)

• Broad `request_incompatible` message patterns. `/\btoo[\s-]?large\b/i` (and `invalid[\s-]?request` / `bad[_\s-]?request`) match anywhere in a message. Because `request_incompatible` is now retryable, an app/tool-level failure surfaced as e.g. `"output too large to process"` (without a `command failed`/`tests failed` marker to catch it first) would be misclassified as retryable and burn a full extra model attempt re-running the whole task. Documented as a known fixture — just flagging the wasted-attempt cost, not just the classification.
• Guaranteed-futile retries for context overflow. A prompt that exceeds the context window fails identically on every candidate, so classifying `context_length_exceeded` as retryable means N full wasted attempts before the current-model last resort also fails. Inherent to the "always reach the user-selected model" guarantee, but for context-length specifically the retries can't succeed — worth a note in the design, or a short-circuit for that one code.
• Abnormal tool-end → 60-min worst case. As the code comments honestly call out: if a `tool_execution_start` never gets its `tool_execution_end` (child dies mid-tool without emitting end), the idle watchdog defers indefinitely and only the 60-min wall cap catches it. The headline Subagent model attempt hangs indefinitely (2.5h, 0 turns) when a fallback model request stalls — no per-attempt timeout/idle watchdog #1580 case doesn't hit this (no tool active), so the fix holds — just noting the residual worst case is a full hour.

Minor

• `positiveEnvMs` for `KILL_GRACE_MS` correctly can't be disabled (0/negative/non-numeric → default) while idle/wall can be zeroed — the asymmetry is intentional and documented; good.
• Test/docs/changelog coverage across all three packages is thorough.

Nice work — the fix is thoughtful and the invariants are unusually well-commented. The classifier-duplication question is the one thing I'd want a follow-up answer on before it calcifies.