diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 14989ff30..55578a183 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -378,6 +378,8 @@ tool call **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 only when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, or when `run_shell` passes a positive timeout (requested plus slack, so this layer cannot beat shell-guard). The `task` tool is always exempt, regardless of Settings — a sub-agent run is bounded by its own limits (maxTurns, no-progress, thrash, opt-in deadlineMs), so the generic per-tool budget never aborts a healthy long-running worker; parent cancel, maxTurns, and eval `--agent-timeout-ms` still bound the run. 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. + Approval scopes offered: Allow Once (persist nothing), Allow Always for a file or its directory (file tools), or a command shape (shell). There is intentionally no "all files" rung. Project-scoped Allow Always grants are confined to the session that minted them: they match the session root and its registered git worktrees (`cwdMatchesGrant` in `src/permission/authz-grants.ts` via `createWorktreeRootsProvider`), not bare process-cwd equality — so a grant at the repo root still covers a sub-agent running in a sibling worktree of the same project. A queued gate's display-dependent timers (auto-deny timeout, tool-budget pause ceiling) arm when the request is actually shown to the operator, not when it is received — a request sitting behind others in the queue does not burn its timeout invisibly. `ask_operator` has the same abort/timeout safety net as the permission gate, so a queued operator question behind a stuck overlay cannot hang a run. Both live in `src/tui/gate-wire.ts`'s `onPermission`/`onOperator`. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index bf6e9e863..660eb4fba 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -233,6 +233,16 @@ 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 watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. The `task` tool is always exempt: a dispatched sub-agent is bounded by its own limits (maxTurns, no-progress, thrash, opt-in `deadlineMs`), not the generic per-tool budget. - `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): + + ```json + "mcp": { + "timeoutMs": 300000 + } + ``` + + 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") that the model can react to; the turn itself is never aborted. `tools.maxTimeoutMs`, if set, still caps `mcp.timeoutMs`. + Optional `subagentMaxTurns` (integer **1–100**, default **30**) sets the default inference-turn budget for dispatched workers (not the parent chat session limit). Per-dispatch `task(maxTurns)` and agent profile `maxTurns` override this default; values above **100** are rejected on `task` and clamped for profiles. Always applies — the primary session is always orchestrator-capable (CL-5814). Optional `sessionMode` is **deprecated**. Legacy values (`single` | `orchestrator`) may still appear on disk and load without error; resolve always returns **orchestrator**. There is no first-run mode picker and no Settings row. Both the interactive TUI (`runTUI`) and the non-TUI product path (`runExec` / `corbits exec`) are orchestrator-only. Exec bootstrap is otherwise a forked copy of the TUI path (shared stack, intentional deltas documented under Architecture → Exec Runner). @@ -253,6 +263,7 @@ All `tools.*` keys live in the global settings file only — there is no per-rep | `tools.timeoutMs` | unset (watchdog unarmed) | Outer wall-clock budget per tool `run()` when set | | `tools.maxTimeoutMs` | unset | Cap on the outer budget when set; does not cap a longer requested `run_shell` | | `tools.waitForApproval` | `true` | Freeze the budget while a permission prompt is open (freeze capped at 30 min); `false` keeps the clock ticking and auto-dismisses the prompt on expiry | +| `mcp.timeoutMs` | **300000** (5 min) — armed even when unset | Outer wall-clock budget for `mcp__*` tool calls specifically; capped by `tools.maxTimeoutMs` when set | The `waitForApproval` default is resolved once at the watchdog boundary (`resolveWaitForApproval`); toggling **Settings → Tools** updates the live config for the next tool call and persists the value here. diff --git a/src/config/settings.ts b/src/config/settings.ts index 1c5c516ba..82e258cb5 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -135,6 +135,11 @@ export type Settings = { // the budget keeps ticking during the prompt; if it expires first the tool is // skipped and the prompt is dismissed. tools?: { timeoutMs?: number; maxTimeoutMs?: number; waitForApproval?: boolean }; + // Wall-clock budget for MCP tool calls specifically (mcp__* names). Unlike + // the generic `tools` budget, this one is armed by default (see + // DEFAULT_MCP_TOOL_TIMEOUT_MS) since a wedged MCP server otherwise hangs a + // tool call forever with nothing to bound it. + mcp?: { timeoutMs?: number }; // Anonymous PostHog telemetry. Global only — never written to per-repo // local settings. `enabled` defaults to true (opt-out); `installationId` // is a random UUID generated once on first use; `noticeShown` stamps that @@ -244,20 +249,23 @@ export function shellTimeoutFromSettings( }; } -// Maps the settings tools block to the shape the tool-execution watchdog expects. -// Returns undefined when nothing is configured so callers can skip the override. +// Maps the settings tools/mcp blocks to the shape the tool-execution watchdog +// expects. Returns undefined only when nothing at all is configured so callers +// can skip the override; mcp.timeoutMs alone (with no tools.* set) still +// produces a config, since MCP timeouts are armed unconditionally. export function toolWatchdogFromSettings( settings?: Settings | null, -): { defaultMs?: number; maxMs?: number; waitForApproval?: boolean } | undefined { +): { defaultMs?: number; maxMs?: number; waitForApproval?: boolean; mcpTimeoutMs?: number } | undefined { const tools = settings?.tools; - if (tools === undefined) return undefined; - const hasTimeout = tools.timeoutMs !== undefined || tools.maxTimeoutMs !== undefined; - const hasWait = tools.waitForApproval !== undefined; - if (!hasTimeout && !hasWait) return undefined; + const mcpTimeoutMs = settings?.mcp?.timeoutMs; + const hasTimeout = tools?.timeoutMs !== undefined || tools?.maxTimeoutMs !== undefined; + const hasWait = tools?.waitForApproval !== undefined; + if (!hasTimeout && !hasWait && mcpTimeoutMs === undefined) return undefined; return { - ...(tools.timeoutMs !== undefined ? { defaultMs: tools.timeoutMs } : {}), - ...(tools.maxTimeoutMs !== undefined ? { maxMs: tools.maxTimeoutMs } : {}), - ...(tools.waitForApproval !== undefined ? { waitForApproval: tools.waitForApproval } : {}), + ...(tools?.timeoutMs !== undefined ? { defaultMs: tools.timeoutMs } : {}), + ...(tools?.maxTimeoutMs !== undefined ? { maxMs: tools.maxTimeoutMs } : {}), + ...(tools?.waitForApproval !== undefined ? { waitForApproval: tools.waitForApproval } : {}), + ...(mcpTimeoutMs !== undefined ? { mcpTimeoutMs } : {}), }; } @@ -464,6 +472,9 @@ const SettingsSchema = type({ "maxTimeoutMs?": "number", "waitForApproval?": "boolean", }), + "mcp?": type({ + "timeoutMs?": "number", + }), "telemetry?": type({ "enabled?": "boolean", "installationId?": "string", @@ -771,6 +782,7 @@ export async function loadSettings(path: string): Promise { : undefined, shell: s.shell as Settings["shell"] | undefined, tools: s.tools as Settings["tools"] | undefined, + mcp: s.mcp as Settings["mcp"] | undefined, telemetry: s.telemetry as Settings["telemetry"] | undefined, otel: s.otel as Settings["otel"] | undefined, recentModels: s.recentModels as Settings["recentModels"] | undefined, diff --git a/src/plugins/tool-time-budget.ts b/src/plugins/tool-time-budget.ts index f6cc15af4..e12b3ce14 100644 --- a/src/plugins/tool-time-budget.ts +++ b/src/plugins/tool-time-budget.ts @@ -39,6 +39,14 @@ export function formatToolExecutionTimeoutMessage( return `${trimmed}\n\n${notice}`; } +export function formatMcpToolTimeoutMessage(toolName: string, timeoutMs: number): string { + const seconds = Math.round(timeoutMs / 1000); + return ( + `MCP tool ${toolName} timed out after ${seconds}s — the server may be wedged; ` + + `retry or continue without it.` + ); +} + export function formatReadFileTimeoutMessage( path: string, partialResult?: string, diff --git a/src/settings.test.ts b/src/settings.test.ts index f3a5e4230..258a26cf8 100644 --- a/src/settings.test.ts +++ b/src/settings.test.ts @@ -738,6 +738,22 @@ describe("loaders", () => { }); expect(toolWatchdogFromSettings({ providers: {} })).toBeUndefined(); }); + + test("toolWatchdogFromSettings maps mcp.timeoutMs alone (no tools.* set)", () => { + expect(toolWatchdogFromSettings({ providers: {}, mcp: { timeoutMs: 45_000 } })).toEqual({ + mcpTimeoutMs: 45_000, + }); + }); + + test("toolWatchdogFromSettings merges mcp.timeoutMs alongside tools.*", () => { + expect( + toolWatchdogFromSettings({ + providers: {}, + tools: { timeoutMs: 120_000, maxTimeoutMs: 600_000 }, + mcp: { timeoutMs: 45_000 }, + }), + ).toEqual({ defaultMs: 120_000, maxMs: 600_000, mcpTimeoutMs: 45_000 }); + }); }); describe("persistSkipPermissionsDefault", () => { diff --git a/src/tui/tool-execution-watchdog.test.ts b/src/tui/tool-execution-watchdog.test.ts index a91fa4d36..6bb822498 100644 --- a/src/tui/tool-execution-watchdog.test.ts +++ b/src/tui/tool-execution-watchdog.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import type { AgentTool } from "@intx/agent"; import { createDynamicToolRunner } from "./dynamic-tool-runner.js"; import { + DEFAULT_MCP_TOOL_TIMEOUT_MS, MAX_TOOL_EXECUTION_TIMEOUT_MS, RUN_SHELL_WATCHDOG_SLACK_MS, getToolApprovalBudget, @@ -122,6 +123,44 @@ describe("tool execution watchdog", () => { expect(resolveToolExecutionTimeoutMs({ defaultMs: 9_999_999, maxMs: 100 }, call)).toBe(100); }); + test("mcp tool calls are bounded by default even with no config (CL-6895)", () => { + const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} }; + expect(resolveToolExecutionTimeoutMs(undefined, call)).toBe(DEFAULT_MCP_TOOL_TIMEOUT_MS); + expect(resolveToolExecutionTimeoutMs({}, call)).toBe(DEFAULT_MCP_TOOL_TIMEOUT_MS); + }); + + test("mcp.timeoutMs overrides the mcp default", () => { + const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} }; + expect(resolveToolExecutionTimeoutMs({ mcpTimeoutMs: 45_000 }, call)).toBe(45_000); + }); + + test("tools.defaultMs alone (no mcpTimeoutMs) does not affect the mcp default", () => { + const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} }; + expect(resolveToolExecutionTimeoutMs({ defaultMs: 5_000 }, call)).toBe( + DEFAULT_MCP_TOOL_TIMEOUT_MS, + ); + }); + + test("tools.maxTimeoutMs still caps a longer mcp.timeoutMs override", () => { + const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} }; + expect( + resolveToolExecutionTimeoutMs({ mcpTimeoutMs: 9_999_999, maxMs: 100 }, call), + ).toBe(100); + }); + + test("non-positive or non-finite mcp.timeoutMs falls back to the default instead of a 1ms timeout", () => { + const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} }; + expect(resolveToolExecutionTimeoutMs({ mcpTimeoutMs: 0 }, call)).toBe( + DEFAULT_MCP_TOOL_TIMEOUT_MS, + ); + expect(resolveToolExecutionTimeoutMs({ mcpTimeoutMs: -5 }, call)).toBe( + DEFAULT_MCP_TOOL_TIMEOUT_MS, + ); + expect(resolveToolExecutionTimeoutMs({ mcpTimeoutMs: NaN }, call)).toBe( + DEFAULT_MCP_TOOL_TIMEOUT_MS, + ); + }); + test("withTimeout dispose clears timer without leaving hung state", async () => { const parent = new AbortController(); const budget = withTimeout(parent.signal, 50); @@ -158,6 +197,72 @@ describe("tool execution watchdog", () => { expect(result.content).toBe(formatToolExecutionTimeoutMessage("slow", 30)); }); + test( + "mcp tool whose promise never resolves times out with a model-reactable error, turn continues", + async () => { + const runner = createDynamicToolRunner( + [ + stringTool( + "mcp__linear__get_issue", + () => new Promise(() => {}), // never resolves — wedged server + ), + ], + { mcpTimeoutMs: 30 }, + ); + const result = await runner.run( + { id: "1", name: "mcp__linear__get_issue", arguments: {} }, + new AbortController().signal, + ); + expect(result.isError).toBe(true); + expect(result.content).toContain("mcp__linear__get_issue timed out after 0s"); + expect(result.content).toContain("the server may be wedged"); + }, + 10_000, + ); + + test( + "concurrent mcp tool calls each time out independently", + async () => { + const runner = createDynamicToolRunner( + [ + stringTool("mcp__linear__get_issue", () => new Promise(() => {})), + stringTool("mcp__linear__list_issues", async () => "ok"), + ], + { mcpTimeoutMs: 30 }, + ); + const signal = new AbortController().signal; + const [hung1, hung2, fast] = await Promise.all([ + runner.run({ id: "1", name: "mcp__linear__get_issue", arguments: {} }, signal), + runner.run({ id: "2", name: "mcp__linear__get_issue", arguments: {} }, signal), + runner.run({ id: "3", name: "mcp__linear__list_issues", arguments: {} }, signal), + ]); + expect(hung1.isError).toBe(true); + expect(hung1.content).toContain("mcp__linear__get_issue timed out"); + expect(hung2.isError).toBe(true); + expect(hung2.content).toContain("mcp__linear__get_issue timed out"); + expect(fast.content).toBe("ok"); + expect(fast.isError).toBeUndefined(); + }, + 10_000, + ); + + test( + "mcp.timeoutMs: 0 does not instantly time out an mcp tool call (falls back to the default)", + async () => { + const runner = createDynamicToolRunner( + [stringTool("mcp__linear__get_issue", async () => "ok")], + { mcpTimeoutMs: 0 }, + ); + const result = await runner.run( + { id: "1", name: "mcp__linear__get_issue", arguments: {} }, + new AbortController().signal, + ); + expect(result.isError).toBeUndefined(); + expect(result.content).toBe("ok"); + }, + 10_000, + ); + test("parent cancel prefers execute salvage body over synthetic aborted", async () => { const parent = new AbortController(); const salvage = { diff --git a/src/tui/tool-execution-watchdog.ts b/src/tui/tool-execution-watchdog.ts index baa736d43..db93eff32 100644 --- a/src/tui/tool-execution-watchdog.ts +++ b/src/tui/tool-execution-watchdog.ts @@ -1,5 +1,9 @@ import { AsyncLocalStorage } from "node:async_hooks"; -import { formatToolExecutionTimeoutMessage } from "../plugins/tool-time-budget.js"; +import { + formatMcpToolTimeoutMessage, + formatToolExecutionTimeoutMessage, +} from "../plugins/tool-time-budget.js"; +import { isMcpToolName } from "../mcp/tool-name.js"; import type { ToolCall, ToolResult } from "@intx/types/runtime"; /** Wall-clock budget for a single tool `run()` invocation (outer guard). */ @@ -13,8 +17,22 @@ export type ToolWatchdogConfig = { * is dismissed via the budget AbortSignal. */ waitForApproval?: boolean; + /** + * Override for mcp__* tool calls (settings.mcp.timeoutMs). Unlike the + * generic defaultMs/maxMs pair, MCP tools are always bounded — a wedged MCP + * server otherwise hangs a tool call forever (CL-6895) — so this only + * changes the bound, it never leaves it unarmed. + */ + mcpTimeoutMs?: number; }; +// Default wall-clock budget for a single MCP tool call when settings.mcp.timeoutMs +// is unset. Live forensics (CL-6895) showed multi-minute MCP calls that were +// merely slow and later completed successfully, not deadlocked — so this stays +// generous (5 minutes) rather than the shorter default used for other tools, +// while still bounding a genuinely wedged server. +export const DEFAULT_MCP_TOOL_TIMEOUT_MS = 300_000; + // Cap applied when Settings set tools.timeoutMs without tools.maxTimeoutMs. // Not an implicit default — omitted settings leave the watchdog unarmed. export const MAX_TOOL_EXECUTION_TIMEOUT_MS = 1_800_000; @@ -54,6 +72,11 @@ const BUDGET_EXPIRED = Symbol("tool-execution-budget-expired"); * The task tool is exempt: it runs an entire sub-agent that carries its own * bounds (maxTurns, no-progress, thrash, opt-in deadline), so the generic * per-tool budget would abort healthy long-running workers mid-run. + * + * mcp__* tool calls are the opposite of exempt: they arm unconditionally (see + * resolveMcpToolTimeoutMs) even when no Settings are configured, because an + * MCP server can wedge a call forever with no other watchdog to bound it + * (CL-6895). */ export function resolveToolExecutionTimeoutMs( config?: ToolWatchdogConfig, @@ -66,9 +89,25 @@ export function resolveToolExecutionTimeoutMs( return requested + RUN_SHELL_WATCHDOG_SLACK_MS; } } + if (call !== undefined && isMcpToolName(call.name)) { + return resolveMcpToolTimeoutMs(config); + } return resolveSettingsWatchdogTimeoutMs(config); } +function resolveMcpToolTimeoutMs(config: ToolWatchdogConfig | undefined): number { + const max = config?.maxMs ?? MAX_TOOL_EXECUTION_TIMEOUT_MS; + const configured = config?.mcpTimeoutMs; + // A non-positive or non-finite configured value is not a valid budget (it + // would floor to a ~0ms timeout and instantly fail every MCP call, which is + // unconditionally armed) — fall back to the default instead of clamping to 1ms. + const raw = + configured !== undefined && Number.isFinite(configured) && configured > 0 + ? configured + : DEFAULT_MCP_TOOL_TIMEOUT_MS; + return Math.min(max, Math.max(1, Math.floor(raw))); +} + function requestedRunShellTimeoutMs(call: ToolCall): number | undefined { const timeout = call.arguments.timeout; if (typeof timeout !== "number" || !Number.isFinite(timeout) || timeout <= 0) { @@ -283,6 +322,12 @@ function isAbortLikeToolError(content: string): boolean { return /abort/i.test(content); } +function formatTimeoutMessage(toolName: string, timeoutMs: number): string { + return isMcpToolName(toolName) + ? formatMcpToolTimeoutMessage(toolName, timeoutMs) + : formatToolExecutionTimeoutMessage(toolName, timeoutMs); +} + /** True when execute produced a body the parent should prefer over synthetic abort/timeout. */ export function isUsableToolExecuteResult(result: ToolResult): boolean { return ( @@ -402,7 +447,7 @@ export async function runWithToolExecutionWatchdog( void executePromise.catch(() => {}); const content = timeoutMs !== undefined && !parentSignal.aborted - ? formatToolExecutionTimeoutMessage(call.name, timeoutMs) + ? formatTimeoutMessage(call.name, timeoutMs) : `${call.name} aborted`; return { callId: call.id, content, isError: true }; } @@ -425,7 +470,7 @@ export async function runWithToolExecutionWatchdog( ) { return { callId: call.id, - content: formatToolExecutionTimeoutMessage(call.name, timeoutMs), + content: formatTimeoutMessage(call.name, timeoutMs), isError: true, }; }