From 39fec76cb1de47035569ff9aefa92586fe8fc5ae Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 12:38:25 +0800 Subject: [PATCH] feat(core): stop a hung tool from hanging the whole turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only Bash bounded itself. A Grep against a stalled network mount, or a fetch wedged before its own timer arms, hangs the agent turn indefinitely — and what the user sees is a blinking cursor with no way to tell a slow command from a dead one. This adds a backstop deadline around every tool call. Backstop is the operative word: the default is deliberately generous, because a deadline that raced each tool's own limit would replace a precise error ("killed by timeout after 120000ms") with a vague one. It fires only when the inner limit does not. A caller-requested `timeout` in the tool's own arguments always wins when it is longer. Bash({ timeout: 900000 }) is an explicit request for a fifteen-minute command; a ten-minute backstop killing it would make that parameter a lie. When it fires on a side-effecting tool, the message says the effect is unknown rather than implying nothing happened. Abandoning the wait does not abort the work — the process may still be running, and "it did not happen" would be a guess presented as a fact. The tool's signal is aborted so a well-behaved tool can stop, but the race is settled first: a tool that resolves synchronously from its abort listener would otherwise win the race and return its own cancellation result, losing both the deadline message and its warning about a half-applied side effect. Co-Authored-By: Claude Opus 5 --- packages/core/src/agent.test.ts | 94 +++++++++++++++++++ packages/core/src/agent.ts | 56 ++++++++++- packages/core/src/guard/index.ts | 15 ++- packages/core/src/guard/tool-deadline.test.ts | 66 +++++++++++++ packages/core/src/guard/tool-deadline.ts | 76 +++++++++++++++ packages/core/src/index.ts | 5 + 6 files changed, 306 insertions(+), 6 deletions(-) create mode 100644 packages/core/src/guard/tool-deadline.test.ts create mode 100644 packages/core/src/guard/tool-deadline.ts diff --git a/packages/core/src/agent.test.ts b/packages/core/src/agent.test.ts index 14fcc0e..59a4195 100644 --- a/packages/core/src/agent.test.ts +++ b/packages/core/src/agent.test.ts @@ -1405,4 +1405,98 @@ describe('runAgent', () => { expect(fired[0]).toContain('called Spin 3 times in a row'); }); }); + + describe('tool deadline', () => { + const hangTool: ToolHandler = { + name: 'Hang', + definition: { + name: 'Hang', + description: 'never returns on its own', + inputSchema: { type: 'object', properties: {} }, + }, + execute: (_input, ctx) => + new Promise((resolve) => { + // Well-behaved: stops when asked. The point of the test is that the + // asking happens at all. + ctx.signal?.addEventListener('abort', () => resolve({ content: 'stopped' }), { + once: true, + }); + }), + }; + const hangCall = (): ToolUseBlock => ({ + type: 'tool_use', + id: 'call_hang', + name: 'Hang', + input: {}, + }); + + function resultText(history: StoredMessage[]): string { + for (const msg of history) { + for (const block of msg.content) { + if (typeof block !== 'string' && block.type === 'tool_result') return block.content; + } + } + expect.fail('no tool result in history'); + } + + it('abandons a tool that never returns, instead of hanging the turn', async () => { + const result = await runAgent({ + provider: new MockProvider([toolUse('hanging', hangCall()), endTurn('done')]), + tools: new ToolRegistry([hangTool]), + systemPrompt: '', + userMessage: 'go', + model: 'deepseek-chat', + cwd, + toolDeadlines: { defaultMs: 50 }, + }); + + expect(result.stopReason).toBe('end_turn'); + expect(resultText(result.history)).toContain('did not return within'); + }); + + it('signals the tool so a well-behaved one can stop', async () => { + let aborted = false; + const watcher: ToolHandler = { + ...hangTool, + execute: (_input, ctx) => + new Promise((resolve) => { + ctx.signal?.addEventListener( + 'abort', + () => { + aborted = true; + resolve({ content: 'stopped' }); + }, + { once: true }, + ); + }), + }; + await runAgent({ + provider: new MockProvider([toolUse('hanging', hangCall()), endTurn('done')]), + tools: new ToolRegistry([watcher]), + systemPrompt: '', + userMessage: 'go', + model: 'deepseek-chat', + cwd, + toolDeadlines: { defaultMs: 50 }, + }); + expect(aborted).toBe(true); + }); + + it('leaves a tool that returns in time completely alone', async () => { + const quick: ToolHandler = { + ...hangTool, + execute: () => Promise.resolve({ content: 'fast enough' }), + }; + const result = await runAgent({ + provider: new MockProvider([toolUse('quick', hangCall()), endTurn('done')]), + tools: new ToolRegistry([quick]), + systemPrompt: '', + userMessage: 'go', + model: 'deepseek-chat', + cwd, + toolDeadlines: { defaultMs: 10_000 }, + }); + expect(resultText(result.history)).toBe('fast enough'); + }); + }); }); diff --git a/packages/core/src/agent.ts b/packages/core/src/agent.ts index 8cfb80a..1178f21 100644 --- a/packages/core/src/agent.ts +++ b/packages/core/src/agent.ts @@ -7,7 +7,13 @@ import type { FileContract } from './config/file-contract.js'; import type { UnattendedApprovalPolicy } from './cron/index.js'; import type { LedgerSink } from './ledger/index.js'; import { buildToolCallRecord, ledgerKindForTool, readPathFor } from './ledger/record-tool-call.js'; -import { RepeatToolGuard, type RepeatGuardOptions } from './guard/index.js'; +import { + deadlineMessage, + RepeatToolGuard, + resolveToolDeadlineMs, + type RepeatGuardOptions, + type ToolDeadlineConfig, +} from './guard/index.js'; import { dispatchToolCall, type DispatchVerdict } from './harness/tool-dispatcher.js'; import { TaskManager, type TaskRunner } from './tasks/manager.js'; import type { HookDispatcher } from './hooks/index.js'; @@ -121,6 +127,8 @@ export interface RunAgentOptions { spillStore?: SpillStore; /** Nudge the model when it repeats an identical tool call. `false` disables it. */ repeatGuard?: false | RepeatGuardOptions; + /** Backstop deadline for a tool call that never returns. See `guard/tool-deadline.ts`. */ + toolDeadlines?: ToolDeadlineConfig; /** Host callback for AskUserQuestion tool. Optional — when absent the tool * errors. */ askUser?: NonNullable; @@ -801,6 +809,50 @@ export async function runAgent(opts: RunAgentOptions): Promise { ready.push({ toolUse, handler }); } + /** + * Run a tool, abandoning the wait if it blows past its backstop deadline. + * + * Abandoning the wait is not the same as stopping the work: the signal asks + * the tool to stop, but a tool that ignores it keeps running. That is why + * the message for a side-effecting tool says the effect is unknown rather + * than claiming nothing happened. + */ + const withDeadline = async ( + toolUse: ToolUseBlock, + handler: NonNullable>, + ): Promise>> => { + const ms = resolveToolDeadlineMs(toolUse.name, toolUse.input, opts.toolDeadlines); + if (ms === undefined) return handler.execute(toolUse.input, toolCtx); + + const deadline = new AbortController(); + const signal = opts.signal + ? AbortSignal.any([opts.signal, deadline.signal]) + : deadline.signal; + let timer: ReturnType | undefined; + const expired = new Promise<'deadline'>((resolve) => { + timer = setTimeout(() => { + // Settle the race BEFORE aborting. A tool that honors its signal + // resolves synchronously from the abort listener, and if that landed + // first the race would return the tool's own "I was cancelled" + // result — losing the deadline message, including its warning that a + // side effect may be half-applied. + resolve('deadline'); + deadline.abort(); + }, ms); + }); + try { + const outcome = await Promise.race([ + handler.execute(toolUse.input, { ...toolCtx, signal }), + expired, + ]); + return outcome === 'deadline' + ? { content: deadlineMessage(toolUse.name, ms), isError: true } + : outcome; + } finally { + clearTimeout(timer); + } + }; + // Runs one approved tool end-to-end: pre-snapshot, execute, PostToolUse // hook, post-snapshot, event + result. Side-effect-free tools call this // concurrently; mutating tools call it one at a time (see partition below). @@ -841,7 +893,7 @@ export async function runAgent(opts: RunAgentOptions): Promise { let tr; try { - tr = await handler.execute(toolUse.input, toolCtx); + tr = await withDeadline(toolUse, handler); } catch (err) { tr = { content: `Error: ${(err as Error).message}`, isError: true }; } diff --git a/packages/core/src/guard/index.ts b/packages/core/src/guard/index.ts index fb278d3..d557951 100644 --- a/packages/core/src/guard/index.ts +++ b/packages/core/src/guard/index.ts @@ -1,7 +1,7 @@ -// Loop-hygiene guards — advisory plugins that watch the agent loop for -// unproductive patterns. A guard never vetoes; it only tells the model what it -// is doing. -// Spec: docs/DSH_ADOPTION_PLAN.md §1.2 +// Loop-hygiene guards — plugins that watch the agent loop for unproductive +// patterns and enforce per-call budgets. A guard never vetoes a call on +// behavioral grounds; it advises, or it enforces a deployment budget. +// Spec: docs/DSH_ADOPTION_PLAN.md §1.2, §1.3 export { RepeatToolGuard, @@ -10,3 +10,10 @@ export { type RepeatReminder, type RepeatReminderKind, } from './repeat-tool.js'; +export { + resolveToolDeadlineMs, + deadlineMessage, + DEFAULT_TOOL_DEADLINE_MS, + SIDE_EFFECTING_TOOLS, + type ToolDeadlineConfig, +} from './tool-deadline.js'; diff --git a/packages/core/src/guard/tool-deadline.test.ts b/packages/core/src/guard/tool-deadline.test.ts new file mode 100644 index 0000000..0a90c60 --- /dev/null +++ b/packages/core/src/guard/tool-deadline.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from 'vitest'; +import { + DEFAULT_TOOL_DEADLINE_MS, + deadlineMessage, + resolveToolDeadlineMs, +} from './tool-deadline.js'; + +describe('resolveToolDeadlineMs', () => { + it('applies the default when nothing is configured', () => { + expect(resolveToolDeadlineMs('Grep', {})).toBe(DEFAULT_TOOL_DEADLINE_MS); + }); + + it('honors a per-tool override', () => { + expect(resolveToolDeadlineMs('Grep', {}, { perTool: { Grep: 5_000 } })).toBe(5_000); + expect(resolveToolDeadlineMs('Read', {}, { perTool: { Grep: 5_000 } })).toBe( + DEFAULT_TOOL_DEADLINE_MS, + ); + }); + + it('returns nothing when disabled', () => { + expect(resolveToolDeadlineMs('Bash', {}, { disabled: true })).toBeUndefined(); + }); + + it('never cuts a caller-requested timeout short', () => { + // Bash({ timeout: 900000 }) is an explicit request for a 15-minute command. + // A backstop that killed it at 10 would make the parameter a lie. + const ms = resolveToolDeadlineMs('Bash', { timeout: 900_000 }); + expect(ms).toBeGreaterThan(900_000); + }); + + it('leaves room for the tool to time out first', () => { + // The inner limit firing produces the better error, so it must win the race. + const ms = resolveToolDeadlineMs('Bash', { timeout: 900_000 }) as number; + expect(ms - 900_000).toBeGreaterThanOrEqual(30_000); + }); + + it('keeps the backstop when the requested timeout is shorter', () => { + expect(resolveToolDeadlineMs('Bash', { timeout: 1_000 })).toBe(DEFAULT_TOOL_DEADLINE_MS); + }); + + it('ignores an unusable timeout argument', () => { + for (const timeout of ['soon', -5, 0, Number.NaN, Number.POSITIVE_INFINITY, null]) { + expect(resolveToolDeadlineMs('Bash', { timeout })).toBe(DEFAULT_TOOL_DEADLINE_MS); + } + }); +}); + +describe('deadlineMessage', () => { + it('does not claim a side-effecting tool changed nothing', () => { + // Abandoning the wait does not abort the work. Saying otherwise would be a + // guess presented as a fact. + const msg = deadlineMessage('Bash', 600_000); + expect(msg).toContain('may still be running'); + expect(msg).toContain('unknown'); + }); + + it('suggests narrowing for a read-only tool', () => { + const msg = deadlineMessage('Grep', 600_000); + expect(msg).toContain('Narrow the request'); + expect(msg).not.toContain('unknown'); + }); + + it('states the elapsed budget in seconds', () => { + expect(deadlineMessage('Grep', 5_000)).toContain('5s'); + }); +}); diff --git a/packages/core/src/guard/tool-deadline.ts b/packages/core/src/guard/tool-deadline.ts new file mode 100644 index 0000000..11b8e6b --- /dev/null +++ b/packages/core/src/guard/tool-deadline.ts @@ -0,0 +1,76 @@ +// Tool deadline — a backstop for a tool call that never returns at all. +// Spec: docs/DSH_ADOPTION_PLAN.md §1.3 +// +// This is explicitly NOT the primary timeout. Most tools already bound +// themselves: Bash takes a `timeout`, WebFetch has a fetch deadline, ripgrep +// exits. Those inner limits fire first and produce a specific error, which is +// the better outcome. This layer exists for the case where the inner limit does +// not fire — a hung network mount, a fetch stuck before its own timer arms, a +// tool that simply forgot — where the alternative today is an agent turn that +// hangs forever with nothing on screen but a blinking cursor. +// +// Because it is a backstop, the default is generous. A deadline that races the +// inner timeout would replace a precise error with a vague one. + +/** Tools whose interruption can leave the workspace in an unknown state. */ +export const SIDE_EFFECTING_TOOLS = new Set(['Bash', 'Edit', 'Write', 'NotebookEdit']); + +/** Grace added to a caller-requested timeout so the tool's own limit fires first. */ +const REQUESTED_TIMEOUT_GRACE_MS = 30_000; + +/** Backstop for a tool that neither declares nor honors a limit of its own. */ +export const DEFAULT_TOOL_DEADLINE_MS = 600_000; + +export interface ToolDeadlineConfig { + /** Applied to any tool without a `perTool` entry. */ + defaultMs?: number; + /** Per-tool overrides, by tool name. */ + perTool?: Record; + /** Turn the backstop off entirely. */ + disabled?: boolean; +} + +/** + * Resolve the backstop deadline for one call. + * + * A caller-requested `timeout` in the tool's own arguments always wins when it + * is longer than the configured backstop: `Bash({ timeout: 900000 })` is an + * explicit request for a 15-minute command, and a 10-minute backstop killing it + * would make the tool's own parameter a lie. + * + * @param tool Tool name. + * @param input The call's arguments. + * @param config Deployment configuration, if any. + * @returns Milliseconds to allow, or undefined when no deadline applies. + */ +export function resolveToolDeadlineMs( + tool: string, + input: Record, + config?: ToolDeadlineConfig, +): number | undefined { + if (config?.disabled) return undefined; + const base = config?.perTool?.[tool] ?? config?.defaultMs ?? DEFAULT_TOOL_DEADLINE_MS; + const requested = input['timeout']; + if (typeof requested === 'number' && Number.isFinite(requested) && requested > 0) { + return Math.max(base, requested + REQUESTED_TIMEOUT_GRACE_MS); + } + return base; +} + +/** + * The message shown when the backstop fires. + * + * For a side-effecting tool this states plainly that the effect is unknown. + * Aborting the wait does not abort the work: the process may still be running, + * and reporting "it did not happen" would be a guess presented as a fact. + * + * @param tool Tool name. + * @param ms The deadline that elapsed. + * @returns Text for the tool result. + */ +export function deadlineMessage(tool: string, ms: number): string { + const base = `Error: ${tool} did not return within ${Math.round(ms / 1000)}s and was abandoned.`; + return SIDE_EFFECTING_TOOLS.has(tool) + ? `${base} It may still be running, and whether it changed anything is unknown — check the current state before retrying.` + : `${base} Narrow the request (a smaller scope, a filter, a more specific path) and try again.`; +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b281f1e..57c2385 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -234,9 +234,14 @@ export { export { RepeatToolGuard, DEFAULT_REPEAT_EXCLUDE, + resolveToolDeadlineMs, + deadlineMessage, + DEFAULT_TOOL_DEADLINE_MS, + SIDE_EFFECTING_TOOLS, type RepeatGuardOptions, type RepeatReminder, type RepeatReminderKind, + type ToolDeadlineConfig, } from './guard/index.js'; // Agent loop's approval callback type (M3b)