Skip to content

Commit 7c76cc5

Browse files
acoliveracoliver
authored
feat: Add CLI configuration for task and shell tool timeout settings (#1021)
* feat: add CLI configuration for task and shell tool timeout settings (closes #920) This commit adds CLI configuration support for tool-level timeouts as requested in issue #920. Changes: - Added 4 new ephemeral settings in ephemeralSettings.ts - Added validation: accepts positive integers OR -1 for unlimited - Added 3 new tests in setCommand.test.ts - Updated setCommand.test.ts invalid-keys test to include new keys - Updated docs/settings-and-profiles.md with documentation Note: The core timeout implementation was already present in task.ts and shell.ts. This commit only adds the CLI settings surface for users to configure these values. closes #920 * feat: implement timeout_ms parameter for shell and task tools Completes issue #920 by adding the missing timeout_ms parameter to run_shell_command and task tools. PR #621 previously added the CLI ephemeral settings but the actual tool parameters were not implemented. Changes: - Added timeout_ms parameter to ShellToolParams and TaskToolParams interfaces - Implemented timeout logic in both ShellToolInvocation and TaskToolInvocation following the GrepTool pattern: - Creates AbortController with setTimeout when timeout is specified - Reads default and max timeouts from ephemeral settings with hardcoded fallbacks - Handles -1 as unlimited (no timeout) - Clamps requested timeout to max setting - Combines user abort signal with timeout signal - Returns ToolResult with TIMEOUT error type on timeout (not EXECUTION_FAILED) - Includes partial output in timeout message - Added comprehensive timeout tests for both tools: - Default timeout when parameter omitted - Max timeout enforcement (clamping) - -1 means unlimited - Returns TIMEOUT error type with partial output - Returns EXECUTION_FAILED for user aborts (distinct from timeout) - Updated tool schemas to document timeout_ms parameter - Integration with existing ephemeral settings (shell_default_timeout_ms, shell_max_timeout_ms, task_default_timeout_ms, task_max_timeout_ms) All tests pass including timeout-specific test suites for both tools. The implementation distinguishes between timeout and user abort scenarios for clear error reporting. closes #920 * refactor: remove unnecessary removeEventListener call before listener is added CodeRabbit nitpick: The removeEventListener call at line 271 (before the listener is added at line 282) is unnecessary. While harmless (removing non-existent listener is no-op), it creates confusion. Removing it improves code clarity.
1 parent ec58191 commit 7c76cc5

File tree

7 files changed

+828
-24
lines changed

7 files changed

+828
-24
lines changed

‎docs/settings-and-profiles.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ Ephemeral settings are runtime configurations that last only for your current se
4545
| `tool-output-item-size-limit` | Maximum size per item/file in bytes | `524288` | `1048576` (1MB) |
4646
| `max-prompt-tokens` | Maximum tokens allowed in any prompt sent to LLM | `200000` | `300000` |
4747
| `shell-replacement` | Allow command substitution ($(), <(), backticks) | `false` | `true` |
48+
| `shell_default_timeout_ms` | Default timeout for shell tool executions in milliseconds | `60000` | `120000` |
49+
| `shell_max_timeout_ms` | Maximum timeout for shell tool executions in milliseconds | `300000` | `600000` |
50+
| `task_default_timeout_ms` | Default timeout for task tool executions in milliseconds | `60000` | `120000` |
51+
| `task_max_timeout_ms` | Maximum timeout for task tool executions in milliseconds | `300000` | `600000` |
4852
| `emojifilter` | Emoji filter mode for LLM responses | `auto` | `allowed`, `auto`, `warn`, `error` |
4953

5054
**Note:** `auth-key` and `auth-keyfile` are no longer supported as ephemeral settings. Use `/key` and `/keyfile` commands instead.
@@ -89,6 +93,13 @@ The CLI parses each `--set key=value` just like `/set`, so CI jobs and scripts c
8993
9094
/set shell-replacement true
9195
96+
# Tool timeout settings
97+
98+
/set shell_default_timeout_ms 120000
99+
/set shell_max_timeout_ms 600000
100+
/set task_default_timeout_ms 120000
101+
/set task_max_timeout_ms 600000
102+
92103
# Tool output control settings
93104
94105
/set tool-output-max-items 100 # Allow up to 100 files/matches

‎packages/cli/src/settings/ephemeralSettings.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,16 @@ export const ephemeralSettingHelp: Record<string, string> = {
9090
'Time window for counting failures in milliseconds (positive integer, default: 60000, load balancer only)',
9191
circuit_breaker_recovery_timeout_ms:
9292
'Cooldown period before retrying after circuit opens in milliseconds (positive integer, default: 30000, load balancer only)',
93+
94+
// Tool timeout settings (Issue #920)
95+
task_default_timeout_ms:
96+
'Default timeout for task tool executions in milliseconds (default: 60000, -1 for unlimited)',
97+
task_max_timeout_ms:
98+
'Maximum timeout allowed for task tool executions in milliseconds (default: 300000, -1 for unlimited)',
99+
shell_default_timeout_ms:
100+
'Default timeout for shell tool executions in milliseconds (default: 60000, -1 for unlimited)',
101+
shell_max_timeout_ms:
102+
'Maximum timeout allowed for shell tool executions in milliseconds (default: 300000, -1 for unlimited)',
93103
};
94104

95105
const validEphemeralKeys = Object.keys(ephemeralSettingHelp);
@@ -464,6 +474,25 @@ export function parseEphemeralSettingValue(
464474
}
465475
}
466476

477+
if (
478+
key === 'task_default_timeout_ms' ||
479+
key === 'task_max_timeout_ms' ||
480+
key === 'shell_default_timeout_ms' ||
481+
key === 'shell_max_timeout_ms'
482+
) {
483+
const numValue = parsedValue as number;
484+
if (
485+
typeof numValue !== 'number' ||
486+
!Number.isInteger(numValue) ||
487+
(numValue !== -1 && numValue <= 0)
488+
) {
489+
return {
490+
success: false,
491+
message: `${key} must be a positive integer in milliseconds or -1 for unlimited`,
492+
};
493+
}
494+
}
495+
467496
return { success: true, value: parsedValue };
468497
}
469498

‎packages/cli/src/ui/commands/setCommand.test.ts‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ describe('setCommand runtime integration', () => {
114114
type: 'message',
115115
messageType: 'error',
116116
content:
117-
'Invalid setting key: invalid-key. Valid keys are: context-limit, compression-threshold, base-url, tool-format, api-version, custom-headers, user-agent, stream-options, streaming, shell-replacement, socket-timeout, socket-keepalive, socket-nodelay, tool-output-max-items, tool-output-max-tokens, tool-output-truncate-mode, tool-output-item-size-limit, max-prompt-tokens, emojifilter, retries, retrywait, maxTurnsPerPrompt, authOnly, dumponerror, dumpcontext, prompt-caching, include-folder-structure, rate-limit-throttle, rate-limit-throttle-threshold, rate-limit-max-wait, reasoning.enabled, reasoning.includeInContext, reasoning.includeInResponse, reasoning.format, reasoning.stripFromContext, reasoning.effort, reasoning.maxTokens, enable-tool-prompts, tpm_threshold, timeout_ms, circuit_breaker_enabled, circuit_breaker_failure_threshold, circuit_breaker_failure_window_ms, circuit_breaker_recovery_timeout_ms',
117+
'Invalid setting key: invalid-key. Valid keys are: context-limit, compression-threshold, base-url, tool-format, api-version, custom-headers, user-agent, stream-options, streaming, shell-replacement, socket-timeout, socket-keepalive, socket-nodelay, tool-output-max-items, tool-output-max-tokens, tool-output-truncate-mode, tool-output-item-size-limit, max-prompt-tokens, emojifilter, retries, retrywait, maxTurnsPerPrompt, authOnly, dumponerror, dumpcontext, prompt-caching, include-folder-structure, rate-limit-throttle, rate-limit-throttle-threshold, rate-limit-max-wait, reasoning.enabled, reasoning.includeInContext, reasoning.includeInResponse, reasoning.format, reasoning.stripFromContext, reasoning.effort, reasoning.maxTokens, enable-tool-prompts, tpm_threshold, timeout_ms, circuit_breaker_enabled, circuit_breaker_failure_threshold, circuit_breaker_failure_window_ms, circuit_breaker_recovery_timeout_ms, task_default_timeout_ms, task_max_timeout_ms, shell_default_timeout_ms, shell_max_timeout_ms',
118118
});
119119
});
120120

@@ -132,6 +132,85 @@ describe('setCommand runtime integration', () => {
132132
'compression-threshold must be a decimal between 0 and 1 (e.g., 0.7 for 70%)',
133133
});
134134
});
135+
it('validates task timeout settings', async () => {
136+
const testCases = [
137+
{ key: 'task_default_timeout_ms', value: '90000' },
138+
{ key: 'task_max_timeout_ms', value: '180000' },
139+
{ key: 'shell_default_timeout_ms', value: '60000' },
140+
{ key: 'shell_max_timeout_ms', value: '300000' },
141+
];
142+
143+
for (const { key, value } of testCases) {
144+
const result = await setCommand.action!(context, `${key} ${value}`);
145+
expect(mockRuntime.setEphemeralSetting).toHaveBeenCalledWith(
146+
key,
147+
Number(value),
148+
);
149+
expect(result).toEqual({
150+
type: 'message',
151+
messageType: 'info',
152+
content: expect.stringContaining(`'${key}'`),
153+
});
154+
}
155+
});
156+
157+
it('validates task timeout settings with -1 for unlimited', async () => {
158+
const testCases = [
159+
{ key: 'task_default_timeout_ms', value: '-1' },
160+
{ key: 'task_max_timeout_ms', value: '-1' },
161+
{ key: 'shell_default_timeout_ms', value: '-1' },
162+
{ key: 'shell_max_timeout_ms', value: '-1' },
163+
];
164+
165+
for (const { key, value } of testCases) {
166+
const result = await setCommand.action!(context, `${key} ${value}`);
167+
expect(mockRuntime.setEphemeralSetting).toHaveBeenCalledWith(key, -1);
168+
expect(result).toEqual({
169+
type: 'message',
170+
messageType: 'info',
171+
content: expect.stringContaining(`'${key}'`),
172+
});
173+
}
174+
});
175+
176+
it('rejects invalid timeout settings', async () => {
177+
const invalidCases = [
178+
{
179+
key: 'task_default_timeout_ms',
180+
value: '-5',
181+
expectedError:
182+
'must be a positive integer in milliseconds or -1 for unlimited',
183+
},
184+
{
185+
key: 'task_max_timeout_ms',
186+
value: '0',
187+
expectedError:
188+
'must be a positive integer in milliseconds or -1 for unlimited',
189+
},
190+
{
191+
key: 'shell_default_timeout_ms',
192+
value: 'not-a-number',
193+
expectedError:
194+
'must be a positive integer in milliseconds or -1 for unlimited',
195+
},
196+
{
197+
key: 'shell_max_timeout_ms',
198+
value: '1.5',
199+
expectedError:
200+
'must be a positive integer in milliseconds or -1 for unlimited',
201+
},
202+
];
203+
204+
for (const { key, value, expectedError } of invalidCases) {
205+
const result = await setCommand.action!(context, `${key} ${value}`);
206+
expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled();
207+
expect(result).toEqual({
208+
type: 'message',
209+
messageType: 'error',
210+
content: expect.stringContaining(expectedError),
211+
});
212+
}
213+
});
135214

136215
it('sets model parameters through runtime helper', async () => {
137216
const result = await setCommand.action!(

‎packages/core/src/tools/shell.test.ts‎

Lines changed: 121 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ describe('ShellTool', () => {
193193
wrappedCommand,
194194
expect.any(String),
195195
expect.any(Function),
196-
mockAbortSignal,
196+
expect.any(AbortSignal),
197197
false,
198198
undefined,
199199
undefined,
@@ -228,7 +228,7 @@ describe('ShellTool', () => {
228228
'dir',
229229
expect.any(String),
230230
expect.any(Function),
231-
mockAbortSignal,
231+
expect.any(AbortSignal),
232232
false,
233233
undefined,
234234
undefined,
@@ -340,6 +340,125 @@ describe('ShellTool', () => {
340340
expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile);
341341
});
342342

343+
describe('timeout_ms handling', () => {
344+
afterEach(() => {
345+
vi.useRealTimers();
346+
});
347+
348+
it('uses default timeout when timeout_ms is omitted', async () => {
349+
vi.useFakeTimers();
350+
const setTimeoutSpy = vi.spyOn(global, 'setTimeout');
351+
mockConfig.getEphemeralSettings.mockReturnValue({
352+
shell_default_timeout_ms: 1234,
353+
shell_max_timeout_ms: 5000,
354+
});
355+
356+
const invocation = shellTool.build({ command: 'ls' });
357+
const promise = invocation.execute(mockAbortSignal);
358+
resolveShellExecution({
359+
output: '',
360+
stdout: '',
361+
stderr: '',
362+
rawOutput: Buffer.from(''),
363+
});
364+
await promise;
365+
366+
expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 1234);
367+
setTimeoutSpy.mockRestore();
368+
});
369+
370+
it('clamps timeout_ms to the maximum setting', async () => {
371+
vi.useFakeTimers();
372+
const setTimeoutSpy = vi.spyOn(global, 'setTimeout');
373+
mockConfig.getEphemeralSettings.mockReturnValue({
374+
shell_default_timeout_ms: 1000,
375+
shell_max_timeout_ms: 2000,
376+
});
377+
378+
const invocation = shellTool.build({
379+
command: 'ls',
380+
timeout_ms: 5000,
381+
});
382+
const promise = invocation.execute(mockAbortSignal);
383+
resolveShellExecution({
384+
output: '',
385+
stdout: '',
386+
stderr: '',
387+
rawOutput: Buffer.from(''),
388+
});
389+
await promise;
390+
391+
expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 2000);
392+
setTimeoutSpy.mockRestore();
393+
});
394+
395+
it('skips the timeout when timeout_ms is -1', async () => {
396+
vi.useFakeTimers();
397+
const setTimeoutSpy = vi.spyOn(global, 'setTimeout');
398+
mockConfig.getEphemeralSettings.mockReturnValue({
399+
shell_default_timeout_ms: 1000,
400+
shell_max_timeout_ms: 2000,
401+
});
402+
403+
const invocation = shellTool.build({
404+
command: 'ls',
405+
timeout_ms: -1,
406+
});
407+
const promise = invocation.execute(mockAbortSignal);
408+
resolveShellExecution({
409+
output: '',
410+
stdout: '',
411+
stderr: '',
412+
rawOutput: Buffer.from(''),
413+
});
414+
await promise;
415+
416+
expect(setTimeoutSpy).not.toHaveBeenCalled();
417+
setTimeoutSpy.mockRestore();
418+
});
419+
420+
it('returns a TIMEOUT error with partial output', async () => {
421+
vi.useFakeTimers();
422+
const invocation = shellTool.build({
423+
command: 'long-running',
424+
timeout_ms: 50,
425+
});
426+
const promise = invocation.execute(mockAbortSignal);
427+
428+
await vi.advanceTimersByTimeAsync(60);
429+
resolveShellExecution({
430+
aborted: true,
431+
output: 'partial output',
432+
stdout: 'partial output',
433+
stderr: '',
434+
rawOutput: Buffer.from('partial output'),
435+
exitCode: null,
436+
signal: null,
437+
error: null,
438+
});
439+
440+
const result = await promise;
441+
442+
expect(result.error?.type).toBe(ToolErrorType.TIMEOUT);
443+
expect(result.llmContent).toContain('timeout_ms');
444+
expect(result.llmContent).toContain('partial output');
445+
});
446+
447+
it('returns EXECUTION_FAILED for user aborts', async () => {
448+
const abortController = new AbortController();
449+
abortController.abort();
450+
const invocation = shellTool.build({
451+
command: 'ls',
452+
timeout_ms: 1000,
453+
});
454+
455+
const result = await invocation.execute(abortController.signal);
456+
457+
expect(result.error?.type).toBe(ToolErrorType.EXECUTION_FAILED);
458+
expect(result.error?.type).not.toBe(ToolErrorType.TIMEOUT);
459+
});
460+
});
461+
343462
describe('Streaming to `updateOutput`', () => {
344463
let updateOutputMock: Mock;
345464
beforeEach(() => {

0 commit comments

Comments
 (0)