From 47ac0c9fb51ce9b3714990fdb81ee8d8818bee8f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 15 Sep 2026 22:25:40 -0700 Subject: [PATCH 1/3] fix(tui): abort stall after post-tool and post-compact silence Awaiting-response with a null stream is no longer a permanent stall exemption. Execution-watchdog-exempt tools stay bounded by stall timeout instead of pinning forever. Verification: bun run check (7664 pass). --- docs/ARCHITECTURE.md | 6 +- docs/IMPLEMENTATION.md | 4 +- docs/TUI.md | 24 ++++--- src/tui/runtime-bridge.ts | 1 + src/tui/stall-watchdog.test.ts | 125 ++++++++++++++++++++++++--------- src/tui/stall-watchdog.ts | 45 ++++++------ src/tui/turn-monitor.test.ts | 81 ++++++++++++++------- 7 files changed, 192 insertions(+), 94 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b9c48c811..c436f8225 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -335,11 +335,11 @@ Data-only agent plugins (`src/plugins/data-only-agent.ts`) synthesize `agentPlug ### System Prompt (`src/agent/prompts.ts`) -The primary session identity is **Skywalker** (`buildChatRole` → `createSkywalkerSystemPrompt`). Product name remains Corbits Code; when asked its name, the primary answers Skywalker. Role: orchestrate — classify, DIY tiny/single-file/one-route product edits, dispatch closed directors via `spawn_agent`/`wait_agents` for substantial work, track the fleet, synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary session (CORE and `SKYWALKER_TOOLS`) so Skywalker can DIY bounded edits; spawn remains the default for substantial, multi-file, parallel, or specialist work. Shell file-writes stay denied by auto-shell policy. MCP tools are not re-filtered by a product-write deny list (that list is gone). There is no static per-worker write-path lock; concurrent lanes sharing a cwd are instead flagged (not blocked) as a `conflict` intervention. A frontier model already knows how to code; the static prompt carries harness-specific facts and the closed-fleet orchestration policy. The base is three individually-exported sections: +The primary session identity is **Skywalker** (`buildChatRole` → `createSkywalkerSystemPrompt`). Product name remains Corbits Code; when asked its name, the primary answers Skywalker. Role: orchestrate — classify, DIY tiny/single-file/one-route product edits, dispatch closed directors via `spawn_agent` (mailbox mail on TUI; `wait_agents` on exec primary) for substantial work, track the fleet, synthesize. Product mutation tools (`write_file` / `edit_file` / `delete_file`) are mounted on the primary session (CORE and `SKYWALKER_TOOLS`) so Skywalker can DIY bounded edits; spawn remains the default for substantial, multi-file, parallel, or specialist work. Shell file-writes stay denied by auto-shell policy. MCP tools are not re-filtered by a product-write deny list (that list is gone). There is no static per-worker write-path lock; concurrent lanes sharing a cwd are instead flagged (not blocked) as a `conflict` intervention. A frontier model already knows how to code; the static prompt carries harness-specific facts and the closed-fleet orchestration policy. The base is three individually-exported sections: - `buildChatRole` — Skywalker primary identity (orchestrate; DIY tiny/bounded product edits; spawn for substantial work). - `buildHarnessFacts` — the non-derivable rules: shell file-writes are blocked, path tools are the DIY surface on primary (spawn builder/docs directors for substantial work), dependency installs and off-limits paths need approval, images are native multimodal input, core tools plus the advertised catalog (including `skill_search`) are resident (MCP and other unadvertised tools load via `tool_search`; use `search_agents` before dispatching specialists), workflows run only from slash-command steps, and session memory lives at `.corbits/MEMORY.md`. -- `buildGuidelines` — be concise, prefer `spawn_agent`/`wait_agents` for substantial product work, DIY tiny/bounded edits on the parent, answer questions and diagnose visual/product feedback before editing, work autonomously for explicit coding tasks, use `lsp` for symbol work, and require implementation agents to run the repository-defined typecheck, relevant tests, and every defined full verification command. Agents report exact commands, outcomes, and exit statuses; a repository with no typecheck command produces an explicit Blocker backed by project-configuration evidence rather than an invented command or silent skip. +- `buildGuidelines` — be concise, prefer `spawn_agent` (mailbox mail on TUI; exec-primary `wait_agents`) for substantial product work, DIY tiny/bounded edits on the parent, answer questions and diagnose visual/product feedback before editing, work autonomously for explicit coding tasks, use `lsp` for symbol work, and require implementation agents to run the repository-defined typecheck, relevant tests, and every defined full verification command. Agents report exact commands, outcomes, and exit statuses; a repository with no typecheck command produces an explicit Blocker backed by project-configuration evidence rather than an invented command or silent skip. - `buildPromptDisciplineBlock` — a shared, prohibition-form section appended exactly once to every built prompt (chat and sub-agent, every provider family). Primary vs worker wording differs for product writes: workers are told to use `read_file`/`edit_file`/`write_file`; Skywalker is told to DIY tiny/bounded edits with those path tools and spawn directors for substantial work. Shared rules: never `cat`/`sed`/heredoc/`echo` for file work, no setting or exporting environment variables (recurring needs belong in project settings), `web_fetch`/`web_search` instead of `curl`/`wget`/hand-rolled queries, one operation per `run_shell` call, turn semantics (a tool-less reply is the final answer, no repeat searches, stop and change approach after three failed attempts, batch independent reads in parallel), and TTY output rules (short bold headers, one-line bullets, backticks for paths/commands, no wide tables). **Provider-conditional residuals.** Per-family additions layer on top of the shared block via the same `ModelFamilyPolicy` mechanism the directors use (`src/subagent/provider-family.ts`, `src/agent/model-family-policy.ts`) — additive lines, never prompt forks. **Grok** leaves get `buildGrokLeafAntiThrashNote` (gated by `shouldApplyGrokAntiThrash` / `applyGrokFinishBias`, withheld from orchestrators): a compact finish-bias reinforcement plus a one-line reminder to route file/web work through the dedicated tools rather than `run_shell`, motivated by observed tool-routing thrash on the same harness. **Kimi** intentionally has no residual yet — `detectModelFamily` already resolves the family so callers can branch on it, but the prompt seam is left unfilled pending eval characterization of Kimi's behavior, mirroring the provisional (permissive-default) policy in `model-family-policy.ts`. @@ -419,7 +419,7 @@ tool call **Approval log** (`src/permission/approval-log.ts`, CL-5666): every consequential decision the gate makes — auto-mode allow/deny or an interactive prompt's allow-once/allow-with-scope/deny/timeout/abort — is appended as one JSONL record to `approvals.jsonl` in the session dir, carrying the classifier/auto-shell rule name that fired (the existing `auto-shell-policy.ts`/`classify.ts` rule names, plus a small closed set of additional fixed literals the log itself defines for decisions those modules don't otherwise name — `auto-allowed-tool`, `non-interactive`, `mega-chain` — never model- or user-authored text), whether the decision was `auto` or `interactive`, a shell chain's segment count, and queued/displayed/settled timestamps. `displayedAt` is set by `PermissionRequest.markDisplayed`, called from `gate-wire.ts`'s `open()` the moment a request actually reaches the overlay host — distinct from when it was raised, so the gap it exposes is the CL-5664 signal (a queued gate arming its timeout before the operator could see it). No command text, file content, path, credential, or other free text is ever recorded — only tool name, rule, mode, segment count, and timing; a sub-agent's free-text dispatch label is deliberately left out, even though it would enable a per-agent breakdown, because nothing constrains what a model puts in it. A hard size cap on the serialized line is defense in depth against a future field reintroducing free text. Writes are fire-and-forget and swallow their own errors; the log defaults to a no-op so nothing depends on it being wired. `scripts/approval-forensics.ts` aggregates across local sessions the same way `intervention-forensics.ts` does for stop/nudge events: per-tool counts by outcome and mode, duration/display-delay percentiles, mega-chain counts, and a duplicate-rate proxy (sessions that hit the same rule more than once). -**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline runs until the parent cancels it or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool. +**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog. On TUI, in-flight `shell_collect` and `ask_director` are still bounded by the stall watchdog so those polls cannot pin the turn forever. `wait_agents` is exec-primary only and is not mounted on TUI. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool. `mcp__*` tool calls are the exception to "arms only when Settings set it": they arm unconditionally with a 5-minute default (`DEFAULT_MCP_TOOL_TIMEOUT_MS`), overridable via `mcp.timeoutMs` and still capped by `tools.maxTimeoutMs` (CL-6895). Nothing else bounds an MCP call — the stall watchdog treats an in-flight tool as activity by design, so a wedged MCP server previously hung a tool call, and the turn, forever. On expiry the call returns a normal tool-error result ("MCP tool `` timed out after ``s — the server may be wedged; retry or continue without it"); the turn is never aborted. The MCP client itself (`src/mcp/client.ts`, wrapping `@modelcontextprotocol/sdk`) multiplexes concurrent requests over one connection by JSON-RPC message id with no serial queue or mutex in our code or in the vendored SDK's `Protocol.request()` — so concurrent calls to the same server are not expected to deadlock each other. Live forensics for CL-6895 showed multi-minute MCP calls that eventually completed successfully, consistent with a slow server response rather than a client-side deadlock. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index d32229c37..a0e54f5e0 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -202,7 +202,7 @@ Listings are list-free, dumps are dump-locked: a bounded `ls`/`tree` prints name `ChatInputProps` carries `isProcessing?: boolean` and `onInterrupt?: (message: string) => void`. When `isProcessing` is true, drain timing is **parent-idle** vs **session-idle**: -- **Enter** soft-steers while the parent is busy — enqueues kind `"steer"` and delivers at the next **parent** `tool.boundary` (the parent tool finishing, not a child). Does not interrupt. **Parent-idle** is when the primary Skywalker turn is not inside an in-flight parent tool; a long parent **foreground** `run_shell` or awaiting `wait_agents` is parent-busy and holds steers. A `run_shell` started with `background: true` returns at once and releases the boundary; its completion is delivered as a system message (`buildShellBackgroundMessage`, mailbox `system`, no operator-originated flag — it re-enters the reactor without counting as operator input) on a later turn. +- **Enter** soft-steers while the parent is busy — enqueues kind `"steer"` and delivers at the next **parent** `tool.boundary` (the parent tool finishing, not a child). Does not interrupt. **Parent-idle** is when the primary Skywalker turn is not inside an in-flight parent tool; a long parent **foreground** `run_shell` is parent-busy and holds steers. A `run_shell` started with `background: true` returns at once and releases the boundary; its completion is delivered as a system message (`buildShellBackgroundMessage`, mailbox `system`, no operator-originated flag — it re-enters the reactor without counting as operator input) on a later turn. #### Background shell mode @@ -286,7 +286,7 @@ Provider and model configuration lives in JSON settings files. The global file h } ``` - - `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms (a bounded poll over a process that outlives the turn). + - `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms the per-tool watchdog (a bounded poll over a process that outlives the turn). An in-flight TUI `shell_collect` or `ask_director` is still bounded by the TUI stall watchdog so it cannot pin the turn forever. `wait_agents` is exec-primary only and is not mounted on TUI. - `waitForApproval` (default **true** when unset) — freeze that budget while a permission prompt is open so a late approve still runs the tool. **Settings → Tools** toggles this live for the next tool call and persists it here. When **false**, the budget keeps ticking during the prompt; on expiry the tool is skipped and the modal is auto-dismissed. The freeze is bounded: after **30 minutes** with the prompt still unanswered the budget resumes ticking on its own, so a prompt that never becomes visible (overlay open, UI gone) cannot hang a tool run indefinitely. Optional `mcp` block bounds MCP tool calls (`mcp__*` names) specifically — unlike `tools.*`, this arms **unconditionally** even with no settings at all, defaulting to **5 minutes**, since a wedged MCP server otherwise hangs a call forever with nothing to bound it (CL-6895): diff --git a/docs/TUI.md b/docs/TUI.md index 95fcf2ff1..48af57c7b 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -201,13 +201,17 @@ threshold, so a resumed session with already-stale activity shows the settled glyph immediately rather than alarming about silence the operator missed, and a stall that breaks and re-arms bursts again. -Auto-abort (`shouldAbortForStall`) is reserved for a stream that had already -started producing tokens and then went dead mid-flight — not for a run that -is merely _awaiting_ the model's next response (right after submit, or the -instant a tool batch resolves and `awaitingResponse` flips back to true). -That wait has no signal to tell "still coming" from "never coming" apart, so -it is never auto-aborted no matter how long it runs; it still surfaces via -the notice, keeping the operator in control of whether to give up on it. +Auto-abort (`shouldAbortForStall`) fires when a live turn has been silent +past `STALL_TIMEOUT_MS`. That includes a stream that started producing +tokens and then went dead, a wait for the model's next response that never +arrives (right after submit, after a tool batch resolves, or after compact +continuation re-entry), and an in-flight poll the per-tool execution +watchdog leaves unarmed (`shell_collect`, `ask_director`). TUI primary does +not mount `wait_agents` — mailbox mail is the collect path. +Ordinary in-flight tools stay exempt here — they have their own execution +budget — and an open operator gate still blocks the clock. The notice +still arms first at `STALL_NOTICE_MS` and hands over to the abort at the +budget so the two never speak at once. The notice is a live diagnosis, not a sticky banner: it comes down on the same paint as the activity that ends the silence, including when the turn settles before the next monitor tick. @@ -638,9 +642,9 @@ Two mid-run gestures, two delivery times (CL-6290): - **Enter, mid-run** — soft steer: enqueues kind `"steer"` and delivers at the next **parent** `tool.boundary` (the parent tool finishing, not a child) via `Agent.deliver` into the live reactor, not a new `send`. A - long parent `run_shell` or an awaiting `wait_agents` is parent-busy and holds - steers. An in-flight TUI-primary `wait_agents` yields as a timeout when a - steer is queued so occupancy can pick it up. A queued steer delivers at the + long parent `run_shell` is parent-busy and holds + steers. TUI primary does not mount `wait_agents`; occupancy delivers + mailbox mail instead. A queued steer delivers at the next parent `tool.boundary` so occupancy can pick it up. Pending items list in the pending column above the prompt, not the transcript; the transcript only ever sees the item that actually delivers, as an ordinary user row diff --git a/src/tui/runtime-bridge.ts b/src/tui/runtime-bridge.ts index d5ad1922f..a92abadff 100644 --- a/src/tui/runtime-bridge.ts +++ b/src/tui/runtime-bridge.ts @@ -1461,6 +1461,7 @@ export function attachSessionBridge( stallTimeoutMs, isProcessing: bag.turn.isProcessing, streamingType: bag.turn.streamingType, + currentToolName: bag.turn.currentToolName, activeToolCalls: bag.turn.activeToolCalls, stallNoticeMs, repeating: bag.turn.repeating, diff --git a/src/tui/stall-watchdog.test.ts b/src/tui/stall-watchdog.test.ts index 0fb2d8fea..075712dd1 100644 --- a/src/tui/stall-watchdog.test.ts +++ b/src/tui/stall-watchdog.test.ts @@ -13,9 +13,7 @@ import { } from "./stall-watchdog.js"; describe("shouldAbortForStall", () => { - // Mid-stream hang: tokens already flowed, then everything went silent — - // the one shape auto-abort is willing to act on. The generic guards - // (status, threshold, exemptions) are exercised against this base. + // Mid-stream hang: tokens already flowed, then everything went silent. const base = { status: "running" as const, awaitingResponse: false, @@ -24,6 +22,7 @@ describe("shouldAbortForStall", () => { stallTimeoutMs: STALL_TIMEOUT_MS, isProcessing: true, streamingType: "text" as const, + currentToolName: null, activeToolCalls: [], }; @@ -70,17 +69,23 @@ describe("shouldAbortForStall", () => { }); test("long tool runs are not stalls", () => { - expect(shouldAbortForStall({ ...base, streamingType: "tool" })).toBe(false); + expect( + shouldAbortForStall({ + ...base, + streamingType: "tool", + currentToolName: "bash", + }), + ).toBe(false); }); }); -// The other shape silence can take: awaiting the model's next response, with -// no tokens yet — set right after submit and again the instant the last -// outstanding tool call resolves (`turnStateOnSubmit`, the `tool.done` -// handler). A slow model produces exactly this state for as long as it takes -// to reply, so it is never auto-aborted, however long the silence — only the -// notice may surface. This is the regression coverage for CL-5640. -describe("shouldAbortForStall — awaiting the model's next token is never auto-aborted", () => { +// Awaiting the model's next response with no tokens yet — set right after +// submit, after the last tool call resolves, and after compact continuation +// re-entry (`turnStateOnSubmit`, `tool.done`, `beginSystemContinuation`). +// Cadence still ticks and the notice still arms at STALL_NOTICE_MS; past +// STALL_TIMEOUT_MS this shape auto-aborts so a continuation that never lands +// cannot freeze the turn. +describe("shouldAbortForStall — awaiting-response with a null stream eventually aborts", () => { const awaiting = { status: "running" as const, awaitingResponse: true, @@ -89,25 +94,35 @@ describe("shouldAbortForStall — awaiting the model's next token is never auto- stallTimeoutMs: STALL_TIMEOUT_MS, isProcessing: true, streamingType: null, + currentToolName: null, activeToolCalls: [], }; - test("does not abort a run merely awaiting a response, however long", () => { - expect(shouldAbortForStall(awaiting)).toBe(false); + test("aborts a run awaiting a response once the stall budget elapses", () => { expect( - shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS * 10 }), + shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS - 1 }), ).toBe(false); + expect(shouldAbortForStall(awaiting)).toBe(true); + expect( + shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS * 10 }), + ).toBe(true); }); // Mirrors the tool.done handler: the last outstanding call just resolved, - // awaitingResponse flips true and streamingType resets to null, then the - // model itself takes a long-but-healthy while to start its next reply. - test("healthy post-tool-batch wait never auto-aborts", () => { + // awaitingResponse flips true and streamingType resets to null, then nothing + // else arrives. + test("post-tool-batch silence auto-aborts after the stall budget", () => { expect(shouldAbortForStall({ ...awaiting, activeToolCalls: [] })).toBe( - false, + true, ); }); + // Same turn shape as compact continuation: beginSystemContinuation calls + // turnStateOnSubmit, which is awaitingResponse + null streamingType. + test("post-compact continuation silence auto-aborts after the stall budget", () => { + expect(shouldAbortForStall(awaiting)).toBe(true); + }); + test("a parallel fan-out with sibling tools still running is not a stall", () => { expect( shouldAbortForStall({ ...awaiting, activeToolCalls: ["call-2"] }), @@ -132,6 +147,37 @@ describe("shouldAbortForStall — awaiting the model's next token is never auto- }); }); +describe("shouldAbortForStall — execution-watchdog-exempt tools do not pin forever", () => { + const collect = { + status: "running" as const, + awaitingResponse: false, + lastActivityAt: 0, + nowMs: STALL_TIMEOUT_MS, + stallTimeoutMs: STALL_TIMEOUT_MS, + isProcessing: true, + streamingType: "tool" as const, + currentToolName: "shell_collect", + activeToolCalls: ["collect-1"], + }; + + test("in-flight collect auto-aborts after the stall budget", () => { + expect( + shouldAbortForStall({ ...collect, nowMs: STALL_TIMEOUT_MS - 1 }), + ).toBe(false); + expect(shouldAbortForStall(collect)).toBe(true); + }); + + test("wait_agents is bounded by the same stall budget", () => { + expect( + shouldAbortForStall({ + ...collect, + currentToolName: "wait_agents", + activeToolCalls: ["wait-1"], + }), + ).toBe(true); + }); +}); + describe("applyStallRecovery", () => { test("aborts then notifies with the default message", () => { const calls: string[] = []; @@ -170,6 +216,7 @@ describe("shouldNoticeStall", () => { stallNoticeMs: STALL_NOTICE_MS, isProcessing: true, streamingType: null, + currentToolName: null, repeating: false, activeToolCalls: [], }; @@ -207,15 +254,12 @@ describe("shouldNoticeStall", () => { ); }); - test("a healthy wait for the model's next token keeps noticing rather than handing over to an abort", () => { - // Unlike the mid-stream case above, this shape never reaches "abort" — - // see the shouldAbortForStall describe block above — so the notice keeps - // surfacing indefinitely instead of going silent once the old timeout - // would have fired. - expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(true); - expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe( + test("an awaiting-response wait hands over to abort at the stall budget", () => { + expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(false); + expect(shouldAbortForStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe( true, ); + expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("abort"); }); test("a long tool run is not stuck", () => { @@ -224,9 +268,27 @@ describe("shouldNoticeStall", () => { ...base, awaitingResponse: false, streamingType: "tool", + currentToolName: "bash", }), ).toBe(false); }); + + test("in-flight collect notices, then aborts at the stall budget", () => { + const collect = { + ...base, + awaitingResponse: false, + streamingType: "tool" as const, + currentToolName: "shell_collect", + activeToolCalls: ["collect-1"], + }; + expect(shouldNoticeStall(collect)).toBe(true); + expect(shouldNoticeStall({ ...collect, nowMs: STALL_TIMEOUT_MS })).toBe( + false, + ); + expect(shouldAbortForStall({ ...collect, nowMs: STALL_TIMEOUT_MS })).toBe( + true, + ); + }); }); describe("the stall level the indicator reads", () => { @@ -239,6 +301,7 @@ describe("the stall level the indicator reads", () => { stallNoticeMs: STALL_NOTICE_MS, isProcessing: true, streamingType: null, + currentToolName: null, activeToolCalls: [], repeating: false, }; @@ -256,16 +319,10 @@ describe("the stall level the indicator reads", () => { expect(stallLevel({ ...midStream, nowMs: STALL_TIMEOUT_MS })).toBe("abort"); }); - // Awaiting the model's next token (right after submit, or right after a - // tool batch resolves) never escalates to "abort" — see - // shouldAbortForStall's dedicated describe block — so this shape stays at - // "notice" indefinitely instead of handing over. - test("a healthy wait for the model's next token stays at notice, never abort", () => { + test("an awaiting-response wait notices, then aborts at the stall budget", () => { expect(stallLevel({ ...base, nowMs: STALL_NOTICE_MS })).toBe("notice"); - expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("notice"); - expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe( - "notice", - ); + expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("abort"); + expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe("abort"); }); test("the indicator keeps reading stalled across the abort threshold", () => { diff --git a/src/tui/stall-watchdog.ts b/src/tui/stall-watchdog.ts index 6370592b5..2cb3e76ec 100644 --- a/src/tui/stall-watchdog.ts +++ b/src/tui/stall-watchdog.ts @@ -22,6 +22,7 @@ export interface ShouldAbortForStallArgs { readonly stallTimeoutMs: number; readonly isProcessing: boolean; readonly streamingType: "text" | "thinking" | "tool" | null; + readonly currentToolName: string | null; readonly activeToolCalls: readonly string[]; } @@ -40,8 +41,14 @@ function silentPastThreshold( // sub-agent's tool call finishes, even while siblings are still running. // Outstanding calls mean the run is not silent, regardless of that flag. if (args.awaitingResponse && args.activeToolCalls.length === 0) return true; - // Mid-stream hang: model stream stalled after first token. Long in-flight - // tool runs do not emit parent stream events; do not abort those. + // Execution-watchdog-exempt polls (collect, wait_agents, ask_director) emit + // no parent stream events. That must not pin the stall clock forever: they + // have no other wall-clock bound, so the stall budget is the backstop. + // TUI primary does not mount wait_agents; the name is kept so a stray mount cannot pin the clock. + if (isStallBoundedInFlightTool(args) && args.isProcessing) return true; + // Mid-stream hang: model stream stalled after first token. Ordinary in-flight + // tool runs still do not emit parent stream events; leave those to the + // per-tool execution watchdog rather than this silence clock. return ( args.isProcessing && args.streamingType !== null && @@ -50,32 +57,28 @@ function silentPastThreshold( } /** - * Whether the run has gone silent while merely *awaiting* the model's next - * response — right after submit or the instant a tool batch resolves, before - * any token of the reply has arrived. `turnStateOnSubmit` and the `tool.done` - * handler both reset `streamingType` to null exactly when they flip - * `awaitingResponse` true, so this state can persist for as long as the model - * takes to start replying: a slow model or a long thinking pass, not - * necessarily a dead one. There is no signal available here to tell "still - * coming" from "never coming" apart, so this case is deliberately excluded - * from auto-abort and left to the notice instead — see `shouldAbortForStall`. + * Polls the per-tool execution watchdog leaves unarmed. Without a stall + * bound they hold `streamingType === "tool"` with no other wall-clock limit. */ -function awaitingFirstToken(args: ShouldAbortForStallArgs): boolean { - return args.awaitingResponse && args.streamingType === null; +function isStallBoundedInFlightTool(args: ShouldAbortForStallArgs): boolean { + if (args.streamingType !== "tool") return false; + const name = args.currentToolName; + return ( + name === "shell_collect" || + name === "wait_agents" || + name === "ask_director" + ); } // Pure decision helper: returns true when the run is genuinely stuck and should // be aborted. Extracted so the timeout logic is unit-testable without timers. // -// Auto-abort is reserved for a stream that had already started producing -// tokens and then went dead mid-flight — the one case silence cannot be -// explained by "still waiting on the model." A long-but-healthy wait for the -// model to start (right after submit, or right after a tool batch resolves) -// is exempted here even past the timeout: it still surfaces via the notice -// (`stallLevel` / `shouldNoticeStall`), but the operator stays in control of -// whether to give up on it rather than having the turn discarded for them. +// Silence past `stallTimeoutMs` aborts a live turn, including a wait for the +// model's next token (right after submit, after a tool batch resolves, or after +// compact continuation re-entry) and an in-flight poll the per-tool watchdog +// does not bound. Ordinary tool runs stay exempt here because they have their +// own execution budget. export function shouldAbortForStall(args: ShouldAbortForStallArgs): boolean { - if (awaitingFirstToken(args)) return false; return silentPastThreshold(args, args.stallTimeoutMs); } diff --git a/src/tui/turn-monitor.test.ts b/src/tui/turn-monitor.test.ts index 63049d2ab..a9adf90d1 100644 --- a/src/tui/turn-monitor.test.ts +++ b/src/tui/turn-monitor.test.ts @@ -476,31 +476,33 @@ describe("stall watchdog", () => { }); }); - // CL-5640: a healthy wait for the model to start its next reply — right - // after submit, or right after the last outstanding tool call resolves — - // must never be auto-aborted just because the parent stream is quiet. Only - // a stream that had already started producing tokens and then went dead - // (covered above) earns the abort; this shape gets the notice at most. - test("a long-but-healthy wait right after submit is never auto-aborted, only noticed", async () => { + // Awaiting the model's next token — right after submit, after the last + // outstanding tool call resolves, or after compact continuation re-entry — + // still notices at the notice threshold, then auto-aborts at the stall budget + // so a reply or continuation that never lands cannot freeze the turn. + test("a wait right after submit auto-aborts once the stall budget elapses", async () => { await withTestRenderer(async (h) => { const t: Harness = await setup(h); try { t.bridge.submit("build it", "immediate"); t.port.clear(); - // No delta ever arrives — the model is just slow to start — held - // far past the stall timeout. - t.advance(20 * 60_000); + t.advance(500); t.tick(); expect(t.port.calls).toEqual([]); expect(t.shell.statusFlash).toBe(STALL_NOTICE_MESSAGE); + + t.advance(1_000); + t.tick(); + expect(t.port.calls).toEqual([{ op: "interrupt" }]); + expect(t.shell.statusFlash).toBe(STALL_RECOVERY_MESSAGE); } finally { t.bridge.dispose(); } }); }); - test("a long-but-healthy wait right after a tool batch resolves is never auto-aborted", async () => { + test("post-tool-batch silence auto-aborts once the stall budget elapses", async () => { await withTestRenderer(async (h) => { const t: Harness = await setup(h); try { @@ -515,23 +517,55 @@ describe("stall watchdog", () => { }); t.port.clear(); - // The last outstanding call resolved; the model just takes a long - // while to start its next reply. Held far past the stall timeout. - t.advance(20 * 60_000); + t.advance(1_500); t.tick(); - expect(t.port.calls).toEqual([]); - expect(t.shell.statusFlash).toBe(STALL_NOTICE_MESSAGE); + expect(t.port.calls).toEqual([{ op: "interrupt" }]); + expect(t.shell.statusFlash).toBe(STALL_RECOVERY_MESSAGE); + } finally { + t.bridge.dispose(); + } + }); + }); + + test("post-compact continuation silence auto-aborts once the stall budget elapses", async () => { + await withTestRenderer(async (h) => { + const t: Harness = await setup(h); + try { + t.bridge.beginSystemContinuation("continue after compact"); + t.port.clear(); + + t.advance(1_500); + t.tick(); + expect(t.port.calls).toEqual([{ op: "interrupt" }]); + expect(t.shell.statusFlash).toBe(STALL_RECOVERY_MESSAGE); + } finally { + t.bridge.dispose(); + } + }); + }); + + test("in-flight collect auto-aborts once the stall budget elapses", async () => { + await withTestRenderer(async (h) => { + const t: Harness = await setup(h); + try { + t.bridge.submit("build it", "immediate"); + t.bridge.handle({ + type: "inference.tool_call.end", + data: { name: "shell_collect", callId: "c1" }, + }); + t.port.clear(); + + t.advance(1_500); + t.tick(); + expect(t.port.calls).toEqual([{ op: "interrupt" }]); + expect(t.shell.statusFlash).toBe(STALL_RECOVERY_MESSAGE); } finally { t.bridge.dispose(); } }); }); - // CL-5640: live sub-agent progress must keep the parent stream's silence - // exempt from abort even though the parent's own `wait_agents` call is the only - // thing in `activeToolCalls` — a future change to fleet-lifecycle handling - // must not silently drop this exemption. - test("live sub-agent progress under an outstanding wait_agents call is never auto-aborted", async () => { + test("an outstanding wait_agents call auto-aborts once the stall budget elapses", async () => { await withTestRenderer(async (h) => { const t: Harness = await setup(h); try { @@ -542,11 +576,10 @@ describe("stall watchdog", () => { }); t.port.clear(); - // The parent stream stays quiet while the sub-agent works — far past - // the stall timeout. - t.advance(20 * 60_000); + t.advance(1_500); t.tick(); - expect(t.port.calls).toEqual([]); + expect(t.port.calls).toEqual([{ op: "interrupt" }]); + expect(t.shell.statusFlash).toBe(STALL_RECOVERY_MESSAGE); } finally { t.bridge.dispose(); } From 8288e8684b93cf2aa1e117d3beb06d93057db55c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 16 Sep 2026 08:53:00 -0700 Subject: [PATCH 2/3] fix(tui): abort stall while a sibling collect is still in flight A sibling tool.done clears currentToolName while shell_collect stays in activeToolCalls, so keying only the last name left that poll unbounded. --- docs/PRODUCT.md | 2 +- src/tui/runtime-bridge.ts | 1 + src/tui/stall-watchdog.test.ts | 37 ++++++++++++++++++++++++++++++++++ src/tui/stall-watchdog.ts | 22 ++++++++++++++++---- 4 files changed, 57 insertions(+), 5 deletions(-) diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index b561269f7..2f730bee8 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -41,7 +41,7 @@ The evidence is in how the product fails today: the personas already produce exc 5. **Resume capability** — Runs persist to a git-backed store and resume from the last point after interruption. 6. **Legible loop** — A live event log, working-tree diff panel, plan tracker, and real-time cost meter show what happened, when, and why. 7. **Operator-in-the-loop** — The agent can call `ask_operator` to pause and ask a clarifying question; the operator answers from a modal (TUI). Headless `corbits exec` unmounts `ask_operator` when stdin/stdout are not TTYs. TTY exec still reads a single line from stdin. -8. **Mid-run steering** — Two modes while the agent is running, keyed to **whose** idle. **Parent-idle** is when the primary Skywalker turn is not inside an in-flight parent tool; **session-idle** is parent-idle **and** no live fleet lanes. **Enter** soft-steers while the parent is busy — delivers at the next **parent** `tool.boundary` without stopping the current run; a long parent `run_shell` or an awaiting `wait_agents` is parent-busy, so Enter is a queued steer, not a new turn. An in-flight TUI-primary `wait_agents` yields as a timeout when that steer is queued so occupancy can deliver it. A queued steer delivers at the next parent `tool.boundary` so occupancy can pick it up. Idle-with-fleet is shipped: after a non-blocking `spawn_agent` dispatch the parent goes idle while workers keep running, mailbox mail arrives as inbound when a worker finishes or fails, and mid-hold Enter starts a new primary turn instead of queueing a steer. **Alt+Enter** queues a follow-up delivered only on session-idle (`run` goes idle; does not interrupt). Session-idle Alt+Enter is a no-op. **Ctrl+C** stops the run outright. The notice row shows distinct `steer N` / `follow-up M` badges, and held messages list in a pending column stacked on the prompt box — `↑`/`↓` select, `Enter` force-pushes one now, `Ctrl+X` drops it — instead of echoing labelled transcript rows; when steers are pending and a parent tool has been in flight a few seconds, the notice names that command. Shortcuts are listed in `/help` (`Enter` soft-steer · `Alt+Enter` follow-up · `Ctrl+C` stop). +8. **Mid-run steering** — Two modes while the agent is running, keyed to **whose** idle. **Parent-idle** is when the primary Skywalker turn is not inside an in-flight parent tool; **session-idle** is parent-idle **and** no live fleet lanes. **Enter** soft-steers while the parent is busy — delivers at the next **parent** `tool.boundary` without stopping the current run; a long parent `run_shell` is parent-busy, so Enter is a queued steer, not a new turn. A queued steer delivers at the next parent `tool.boundary` so occupancy can pick it up. Idle-with-fleet is shipped: after a non-blocking `spawn_agent` dispatch the parent goes idle while workers keep running, mailbox mail arrives as inbound when a worker finishes or fails, and mid-hold Enter starts a new primary turn instead of queueing a steer. **Alt+Enter** queues a follow-up delivered only on session-idle (`run` goes idle; does not interrupt). Session-idle Alt+Enter is a no-op. **Ctrl+C** stops the run outright. The notice row shows distinct `steer N` / `follow-up M` badges, and held messages list in a pending column stacked on the prompt box — `↑`/`↓` select, `Enter` force-pushes one now, `Ctrl+X` drops it — instead of echoing labelled transcript rows; when steers are pending and a parent tool has been in flight a few seconds, the notice names that command. Shortcuts are listed in `/help` (`Enter` soft-steer · `Alt+Enter` follow-up · `Ctrl+C` stop). 9. **Orchestrator-only (TUI + exec)** — The primary session is always the orchestrator: it can act directly and delegates via `spawn_agent` (then idle; mailbox mail inbound) / `search_agents`. Nested orchestrators collect through mailbox mail the same way. Long jobs belong on workers — a parent that runs them itself stays parent-busy and holds Enter steers. Single-agent session mode, the first-run mode picker, and Settings → Session are gone (CL-5814). Legacy `sessionMode` values on disk are ignored. ## User Experience diff --git a/src/tui/runtime-bridge.ts b/src/tui/runtime-bridge.ts index a92abadff..c978bd3c1 100644 --- a/src/tui/runtime-bridge.ts +++ b/src/tui/runtime-bridge.ts @@ -1463,6 +1463,7 @@ export function attachSessionBridge( streamingType: bag.turn.streamingType, currentToolName: bag.turn.currentToolName, activeToolCalls: bag.turn.activeToolCalls, + callIdByName: bag.turn.callIdByName, stallNoticeMs, repeating: bag.turn.repeating, }); diff --git a/src/tui/stall-watchdog.test.ts b/src/tui/stall-watchdog.test.ts index 075712dd1..930ac8839 100644 --- a/src/tui/stall-watchdog.test.ts +++ b/src/tui/stall-watchdog.test.ts @@ -24,6 +24,7 @@ describe("shouldAbortForStall", () => { streamingType: "text" as const, currentToolName: null, activeToolCalls: [], + callIdByName: {}, }; test("aborts a mid-stream hang past the timeout", () => { @@ -96,6 +97,7 @@ describe("shouldAbortForStall — awaiting-response with a null stream eventuall streamingType: null, currentToolName: null, activeToolCalls: [], + callIdByName: {}, }; test("aborts a run awaiting a response once the stall budget elapses", () => { @@ -158,6 +160,7 @@ describe("shouldAbortForStall — execution-watchdog-exempt tools do not pin for streamingType: "tool" as const, currentToolName: "shell_collect", activeToolCalls: ["collect-1"], + callIdByName: { shell_collect: "collect-1" }, }; test("in-flight collect auto-aborts after the stall budget", () => { @@ -173,9 +176,41 @@ describe("shouldAbortForStall — execution-watchdog-exempt tools do not pin for ...collect, currentToolName: "wait_agents", activeToolCalls: ["wait-1"], + callIdByName: { wait_agents: "wait-1" }, }), ).toBe(true); }); + + // tool.done of a sibling bash clears currentToolName and streamingType + // while shell_collect is still in activeToolCalls. Keying only the last + // name would leave that poll unbounded forever. + test("sibling tool.done while collect is in-flight still aborts at the stall budget", () => { + const afterSiblingDone = { + ...collect, + currentToolName: null, + streamingType: null, + awaitingResponse: false, + activeToolCalls: ["collect-1"], + callIdByName: { shell_collect: "collect-1" }, + }; + expect( + shouldAbortForStall({ ...afterSiblingDone, nowMs: STALL_TIMEOUT_MS - 1 }), + ).toBe(false); + expect(shouldAbortForStall(afterSiblingDone)).toBe(true); + }); + + test("a remaining ordinary tool after a sibling done is not a stall", () => { + expect( + shouldAbortForStall({ + ...collect, + currentToolName: null, + streamingType: null, + awaitingResponse: false, + activeToolCalls: ["bash-1"], + callIdByName: { bash: "bash-1" }, + }), + ).toBe(false); + }); }); describe("applyStallRecovery", () => { @@ -219,6 +254,7 @@ describe("shouldNoticeStall", () => { currentToolName: null, repeating: false, activeToolCalls: [], + callIdByName: {}, }; test("a parallel fan-out with sibling tools still running does not notice", () => { @@ -303,6 +339,7 @@ describe("the stall level the indicator reads", () => { streamingType: null, currentToolName: null, activeToolCalls: [], + callIdByName: {}, repeating: false, }; diff --git a/src/tui/stall-watchdog.ts b/src/tui/stall-watchdog.ts index 2cb3e76ec..98473712d 100644 --- a/src/tui/stall-watchdog.ts +++ b/src/tui/stall-watchdog.ts @@ -24,6 +24,7 @@ export interface ShouldAbortForStallArgs { readonly streamingType: "text" | "thinking" | "tool" | null; readonly currentToolName: string | null; readonly activeToolCalls: readonly string[]; + readonly callIdByName: Readonly>; } /** @@ -58,11 +59,12 @@ function silentPastThreshold( /** * Polls the per-tool execution watchdog leaves unarmed. Without a stall - * bound they hold `streamingType === "tool"` with no other wall-clock limit. + * bound they hold the turn with no other wall-clock limit. A sibling + * tool.done clears `currentToolName` / `streamingType` while the poll is + * still in `activeToolCalls`, so the bound keys any in-flight stall-bounded + * name, not only the last announced tool. */ -function isStallBoundedInFlightTool(args: ShouldAbortForStallArgs): boolean { - if (args.streamingType !== "tool") return false; - const name = args.currentToolName; +function isStallBoundedToolName(name: string | null | undefined): boolean { return ( name === "shell_collect" || name === "wait_agents" || @@ -70,6 +72,18 @@ function isStallBoundedInFlightTool(args: ShouldAbortForStallArgs): boolean { ); } +function isStallBoundedInFlightTool(args: ShouldAbortForStallArgs): boolean { + if (isStallBoundedToolName(args.currentToolName)) return true; + for (const [name, id] of Object.entries(args.callIdByName)) { + if (isStallBoundedToolName(name) && args.activeToolCalls.includes(id)) { + return true; + } + } + // Name-only announcements track the call under its name until a real id + // arrives, so the placeholder itself is the stall-bounded name. + return args.activeToolCalls.some(isStallBoundedToolName); +} + // Pure decision helper: returns true when the run is genuinely stuck and should // be aborted. Extracted so the timeout logic is unit-testable without timers. // From beaa11b2b060f023b0bc65bdd01b26965f51fd55 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 16 Sep 2026 12:10:29 -0700 Subject: [PATCH 3/3] docs(tui): qualify stall abort cannot-pin-forever claim Post-tool and post-compact silence still abort. Concurrent same-name shell_collect can still lose the stall bound via callIdByName collision, so docs must not promise an absolute cannot-pin-forever. --- docs/ARCHITECTURE.md | 2 +- docs/IMPLEMENTATION.md | 2 +- docs/TUI.md | 6 ++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c436f8225..03d14a13a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -419,7 +419,7 @@ tool call **Approval log** (`src/permission/approval-log.ts`, CL-5666): every consequential decision the gate makes — auto-mode allow/deny or an interactive prompt's allow-once/allow-with-scope/deny/timeout/abort — is appended as one JSONL record to `approvals.jsonl` in the session dir, carrying the classifier/auto-shell rule name that fired (the existing `auto-shell-policy.ts`/`classify.ts` rule names, plus a small closed set of additional fixed literals the log itself defines for decisions those modules don't otherwise name — `auto-allowed-tool`, `non-interactive`, `mega-chain` — never model- or user-authored text), whether the decision was `auto` or `interactive`, a shell chain's segment count, and queued/displayed/settled timestamps. `displayedAt` is set by `PermissionRequest.markDisplayed`, called from `gate-wire.ts`'s `open()` the moment a request actually reaches the overlay host — distinct from when it was raised, so the gap it exposes is the CL-5664 signal (a queued gate arming its timeout before the operator could see it). No command text, file content, path, credential, or other free text is ever recorded — only tool name, rule, mode, segment count, and timing; a sub-agent's free-text dispatch label is deliberately left out, even though it would enable a per-agent breakdown, because nothing constrains what a model puts in it. A hard size cap on the serialized line is defense in depth against a future field reintroducing free text. Writes are fire-and-forget and swallow their own errors; the log defaults to a no-op so nothing depends on it being wired. `scripts/approval-forensics.ts` aggregates across local sessions the same way `intervention-forensics.ts` does for stop/nudge events: per-tool counts by outcome and mode, duration/display-delay percentiles, mega-chain counts, and a duplicate-rate proxy (sessions that hit the same rule more than once). -**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog. On TUI, in-flight `shell_collect` and `ask_director` are still bounded by the stall watchdog so those polls cannot pin the turn forever. `wait_agents` is exec-primary only and is not mounted on TUI. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool. +**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog. On TUI, an in-flight `shell_collect` or `ask_director` is still bounded by the stall watchdog — including after a tool batch resolves and after compact continuation re-entry — so a typical poll does not pin the turn. Concurrent same-name `shell_collect` calls still collide in `callIdByName` (one slot per name); a sibling collect finishing can drop the mapping and the remaining poll can lose that stall bound. That residual lives in the name-keyed tracker (`src/tui/turn-state.ts`), not in the post-tool / post-compact abort itself. `wait_agents` is exec-primary only and is not mounted on TUI. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool. `mcp__*` tool calls are the exception to "arms only when Settings set it": they arm unconditionally with a 5-minute default (`DEFAULT_MCP_TOOL_TIMEOUT_MS`), overridable via `mcp.timeoutMs` and still capped by `tools.maxTimeoutMs` (CL-6895). Nothing else bounds an MCP call — the stall watchdog treats an in-flight tool as activity by design, so a wedged MCP server previously hung a tool call, and the turn, forever. On expiry the call returns a normal tool-error result ("MCP tool `` timed out after ``s — the server may be wedged; retry or continue without it"); the turn is never aborted. The MCP client itself (`src/mcp/client.ts`, wrapping `@modelcontextprotocol/sdk`) multiplexes concurrent requests over one connection by JSON-RPC message id with no serial queue or mutex in our code or in the vendored SDK's `Protocol.request()` — so concurrent calls to the same server are not expected to deadlock each other. Live forensics for CL-6895 showed multi-minute MCP calls that eventually completed successfully, consistent with a slow server response rather than a client-side deadlock. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index a0e54f5e0..b0a1f54df 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -286,7 +286,7 @@ Provider and model configuration lives in JSON settings files. The global file h } ``` - - `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms the per-tool watchdog (a bounded poll over a process that outlives the turn). An in-flight TUI `shell_collect` or `ask_director` is still bounded by the TUI stall watchdog so it cannot pin the turn forever. `wait_agents` is exec-primary only and is not mounted on TUI. + - `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms the per-tool watchdog (a bounded poll over a process that outlives the turn). An in-flight TUI `shell_collect` or `ask_director` is still bounded by the TUI stall watchdog, including after a tool batch resolves and after compact continuation re-entry, so a typical poll does not pin the turn. Concurrent same-name `shell_collect` calls still collide in `callIdByName` (one slot per name); a sibling collect finishing can drop the mapping and the remaining poll can lose that stall bound — a residual of the name-keyed tracker, not a hole in the post-tool / post-compact abort. `wait_agents` is exec-primary only and is not mounted on TUI. - `waitForApproval` (default **true** when unset) — freeze that budget while a permission prompt is open so a late approve still runs the tool. **Settings → Tools** toggles this live for the next tool call and persists it here. When **false**, the budget keeps ticking during the prompt; on expiry the tool is skipped and the modal is auto-dismissed. The freeze is bounded: after **30 minutes** with the prompt still unanswered the budget resumes ticking on its own, so a prompt that never becomes visible (overlay open, UI gone) cannot hang a tool run indefinitely. Optional `mcp` block bounds MCP tool calls (`mcp__*` names) specifically — unlike `tools.*`, this arms **unconditionally** even with no settings at all, defaulting to **5 minutes**, since a wedged MCP server otherwise hangs a call forever with nothing to bound it (CL-6895): diff --git a/docs/TUI.md b/docs/TUI.md index 48af57c7b..dedb3d2e1 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -208,6 +208,12 @@ arrives (right after submit, after a tool batch resolves, or after compact continuation re-entry), and an in-flight poll the per-tool execution watchdog leaves unarmed (`shell_collect`, `ask_director`). TUI primary does not mount `wait_agents` — mailbox mail is the collect path. +Concurrent same-name `shell_collect` polls still collide in `callIdByName` +(one slot per name), so a sibling collect finishing can drop the mapping and +the remaining poll can lose that stall bound. That residual is in the +name-keyed tracker, not a hole in the post-tool / post-compact abort; the +usual single-collect shape stays bounded, including after a sibling +`tool.done` that clears `currentToolName`. Ordinary in-flight tools stay exempt here — they have their own execution budget — and an open operator gate still blocks the clock. The notice still arms first at `STALL_NOTICE_MS` and hands over to the abort at the