Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions packages/core/src/agent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});
56 changes: 54 additions & 2 deletions packages/core/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<ToolContext['askUser']>;
Expand Down Expand Up @@ -801,6 +809,50 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
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<ReturnType<typeof opts.tools.get>>,
): Promise<Awaited<ReturnType<typeof handler.execute>>> => {
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<typeof setTimeout> | 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).
Expand Down Expand Up @@ -841,7 +893,7 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {

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 };
}
Expand Down
15 changes: 11 additions & 4 deletions packages/core/src/guard/index.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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';
66 changes: 66 additions & 0 deletions packages/core/src/guard/tool-deadline.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
76 changes: 76 additions & 0 deletions packages/core/src/guard/tool-deadline.ts
Original file line number Diff line number Diff line change
@@ -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<string, number>;
/** 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<string, unknown>,
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.`;
}
5 changes: 5 additions & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading