feat: Add CLI configuration for task and shell tool timeout settings #1021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughAdds per-call and ephemeral timeouts for shell and task tools: new ephemeral keys, CLI parsing/validation, tool parameter Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant Tool as Shell/Task Tool
participant Exec as ExecutionService/Orchestrator
Client->>Tool: invoke run(..., timeout_ms?)
Tool->>Config: read ephemeral defaults/max (`*_timeout_ms`)
Note right of Tool: resolveTimeoutMs (use default, clamp to max, -1 = unlimited)
alt timeout configured (not unlimited)
Tool->>Tool: create AbortController + timer
Tool->>Exec: launch(..., signal: combinedSignal)
par execution runs
Exec->>Exec: stream partial output
and
alt timer fires
Tool->>Exec: signal.abort (timer)
Exec-->>Tool: partial output + abort/error
Tool-->>Client: result { status: TIMEOUT, partialOutput }
else user aborts
Client->>Tool: user abort
Tool->>Exec: signal.abort
Exec-->>Tool: aborted
Tool-->>Client: result { status: EXECUTION_FAILED, message: aborted }
else completes
Exec-->>Tool: success result
Tool-->>Client: success result
end
end
else unlimited
Tool->>Exec: launch(..., signal: userSignal)
Exec-->>Tool: success/abort result
Tool-->>Client: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
LLxprt PR Review – PR #1021Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/cli/src/ui/commands/setCommand.test.ts:
- Around line 176-213: The failing test leaves mock state from prior tests, so
before asserting mockRuntime.setEphemeralSetting was not called you must clear
the mock(s); update the "rejects invalid timeout settings" test (around the it
block calling setCommand.action) to call a mock reset such as
mockRuntime.setEphemeralSetting.mockClear() or jest.clearAllMocks() before the
for-loop (or at start of each iteration) so each invalid case begins with a
clean mock and the
expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled() assertion is
reliable.
In @packages/core/src/tools/task.ts:
- Around line 221-228: The code treats -1 as a numeric timeout which causes
setTimeout(-1) and incorrect clamping; change the logic around timeoutMs,
this.params.timeout_ms, defaultTimeout and maxTimeout so that -1 means
"unlimited"/undefined: compute requestedTimeout = this.params.timeout_ms ??
defaultTimeout, then if requestedTimeout === -1 treat it as undefined; also
treat maxTimeout === -1 as "no limit" (i.e., skip Math.min). Finally set
timeoutMs to undefined when requestedTimeout is undefined, otherwise if
maxTimeout !== -1 use Math.min(requestedTimeout, maxTimeout) else use
requestedTimeout; remove the redundant DEFAULT_TASK_TIMEOUT_MS fallback since
defaultTimeout already defaults elsewhere to DEFAULT_TASK_TIMEOUT_MS to avoid
ever passing -1 to setTimeout.
🧹 Nitpick comments (2)
packages/core/src/tools/shell.ts (1)
261-276: Simplify redundant nullish coalescing and remove unnecessary type assertions.Line 263-264: The type assertion
as number | undefinedis unnecessary sinceephemeralSettingsis already typed asEphemeralSettingswhich has these fields asnumber | undefined.Line 274: The double nullish coalescing is redundant since
defaultTimeoutalready hasDEFAULT_SHELL_TIMEOUT_MSas its fallback.🔎 Proposed simplification
const ephemeralSettings = this.config.getEphemeralSettings(); const defaultTimeout = - (ephemeralSettings.shell_default_timeout_ms as number | undefined) ?? + ephemeralSettings.shell_default_timeout_ms ?? DEFAULT_SHELL_TIMEOUT_MS; const maxTimeout = - (ephemeralSettings.shell_max_timeout_ms as number | undefined) ?? + ephemeralSettings.shell_max_timeout_ms ?? MAX_SHELL_TIMEOUT_MS; let timeoutMs: number | undefined; if (this.params.timeout_ms === -1) { timeoutMs = undefined; } else { const requestedTimeout = - this.params.timeout_ms ?? defaultTimeout ?? DEFAULT_SHELL_TIMEOUT_MS; + this.params.timeout_ms ?? defaultTimeout; timeoutMs = Math.min(requestedTimeout, maxTimeout); }packages/cli/src/ui/commands/setCommand.test.ts (1)
135-155: Consider clearing mocks between loop iterations.The test iterates over multiple cases but doesn't clear mocks between iterations. While the current assertions use
toHaveBeenCalledWithwhich checks for any matching call, this could lead to false positives if a previous iteration's mock state affects later assertions.🔎 Proposed fix
it('validates task timeout settings', async () => { const testCases = [ { key: 'task_default_timeout_ms', value: '90000' }, { key: 'task_max_timeout_ms', value: '180000' }, { key: 'shell_default_timeout_ms', value: '60000' }, { key: 'shell_max_timeout_ms', value: '300000' }, ]; for (const { key, value } of testCases) { + vi.clearAllMocks(); const result = await setCommand.action!(context, `${key} ${value}`); expect(mockRuntime.setEphemeralSetting).toHaveBeenCalledWith( key, Number(value), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/settings-and-profiles.mdpackages/cli/src/settings/ephemeralSettings.tspackages/cli/src/ui/commands/setCommand.test.tspackages/core/src/tools/shell.test.tspackages/core/src/tools/shell.tspackages/core/src/tools/task.test.tspackages/core/src/tools/task.tspackages/core/src/types/modelParams.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/types/modelParams.tspackages/cli/src/settings/ephemeralSettings.tspackages/cli/src/ui/commands/setCommand.test.tspackages/core/src/tools/shell.test.tspackages/core/src/tools/shell.tspackages/core/src/tools/task.test.tspackages/core/src/tools/task.ts
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation-only changes (*.md files, docs/) do NOT require build/test/lint cycle
Files:
docs/settings-and-profiles.md
🧠 Learnings (6)
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: Shell tool (packages/core/src/tools/shell.ts) always applies tool-output-max-tokens via limitOutputTokens to the model-facing llmContent at the end of execute(), formatting a warning when truncated.
Applied to files:
packages/core/src/types/modelParams.tspackages/core/src/tools/shell.test.tspackages/core/src/tools/shell.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/tools/shell.test.tspackages/core/src/tools/shell.tspackages/core/src/tools/task.test.ts
📚 Learning: 2025-12-18T14:06:22.557Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-18T14:06:22.557Z
Learning: In the llxprt-code codebase, the `tool-output-max-items` ephemeral setting only applies to tools that return lists/collections (glob, grep, read-many-files), not to single-output tools like shell. Not all ephemerals are relevant to every tool.
Applied to files:
docs/settings-and-profiles.md
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
Applied to files:
packages/core/src/tools/shell.ts
📚 Learning: 2025-12-05T14:59:34.368Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T14:59:34.368Z
Learning: Use subagents (general-purpose, typescript-code-reviewer, or typescript-coder) for complex multi-step tasks, research, complex implementations, code review, and extensive file searching
Applied to files:
packages/core/src/tools/task.test.tspackages/core/src/tools/task.ts
📚 Learning: 2025-12-05T14:59:34.368Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T14:59:34.368Z
Learning: Run `npm run typecheck` to ensure type safety
Applied to files:
packages/core/src/tools/task.test.ts
🧬 Code graph analysis (4)
packages/cli/src/ui/commands/setCommand.test.ts (2)
packages/cli/src/ui/commands/setCommand.ts (1)
setCommand(595-806)scripts/oldui-tmux-harness.js (1)
key(274-274)
packages/core/src/tools/shell.test.ts (1)
packages/core/src/services/shellExecutionService.ts (1)
ShellExecutionResult(32-50)
packages/core/src/tools/shell.ts (1)
packages/core/src/utils/toolOutputLimiter.ts (1)
formatLimitedOutput(232-257)
packages/core/src/tools/task.ts (4)
packages/core/src/tools/tools.ts (1)
ToolResult(423-451)packages/core/src/core/subagentOrchestrator.ts (1)
SubagentLaunchRequest(55-61)packages/core/src/core/subagent.ts (1)
OutputObject(89-104)packages/core/src/core/turn.ts (1)
DEFAULT_AGENT_ID(39-39)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
🔇 Additional comments (14)
packages/core/src/types/modelParams.ts (1)
120-127: LGTM!The new timeout fields are correctly typed as optional numbers and follow the naming convention used by other load balancer settings in this interface (underscore_case for timeout-related settings).
packages/core/src/tools/shell.ts (3)
437-463: LGTM!The timeout vs user-cancellation distinction logic is correct. The message construction properly differentiates between the two cases and includes partial output when available.
513-524: LGTM!The error payload correctly distinguishes between
TIMEOUTandEXECUTION_FAILEDerror types, using the same condition as the message construction. This ensures consistent behavior throughout the error handling flow.
733-737: LGTM!The schema correctly documents the
timeout_msparameter with accurate default/max values and the -1 unlimited option.docs/settings-and-profiles.md (2)
96-102: LGTM!The CLI examples are clear and demonstrate practical usage of the new timeout settings.
48-51: LGTM!The documentation clearly describes the new timeout settings with appropriate defaults that match the implementation in
task.ts(task_default_timeout_ms: 60000, task_max_timeout_ms: 300000). The naming convention is consistent across documentation and code.packages/core/src/tools/shell.test.ts (2)
279-331: LGTM!The timeout test suite is comprehensive and well-structured. It correctly uses fake timers, properly sets up ephemeral settings via the mock config, and validates both the error type and message content. The abort signal handling pattern in the mock is correct.
450-489: LGTM!The test correctly verifies that user-initiated aborts produce
EXECUTION_FAILEDerror type (notTIMEOUT) and include "cancelled" in the message, properly distinguishing between the two abort scenarios.packages/cli/src/settings/ephemeralSettings.ts (2)
93-102: LGTM!The help text entries are clear, consistent with the other settings, and correctly document the defaults and the -1 unlimited option.
477-494: LGTM!The validation logic correctly handles all cases:
- Accepts positive integers and -1
- Rejects zero, other negative numbers, decimals, and non-numeric values
The error message clearly explains the expected format.
packages/core/src/tools/task.test.ts (3)
212-298: LGTM!The timeout test is comprehensive and well-structured. It correctly:
- Uses fake timers for deterministic testing
- Mocks the launch to respond to abort signals
- Verifies the
TIMEOUTerror type, message content, and metadata includingterminateReasonandemittedVars
501-551: LGTM!The user abort distinction test correctly verifies that:
- User-initiated aborts produce
EXECUTION_FAILED(notTIMEOUT)- The metadata includes
cancelled: trueThis ensures the TaskTool matches the ShellTool behavior for abort handling.
86-95: Test correctly validates therunConfig.max_time_minutesfield in the launch request.The implementation properly constructs
maxTimeMinutesby converting timeout milliseconds to minutes (60,000 ms ÷ 60,000 = 1 minute), and the test assertion correctly verifies this field is passed through to the orchestrator.packages/core/src/tools/task.ts (1)
119-120: Excellent timeout implementation with sophisticated abort coordination.The timeout functionality is well-architected:
- Configuration: Properly reads ephemeral settings (
task_default_timeout_ms,task_max_timeout_ms) with sensible hardcoded fallbacks (60s default, 5min max).- AbortSignal coordination: Creates a separate
timeoutControllerand elegantly links it with the user'ssignal, distinguishing timeout aborts from user-initiated aborts throughout execution (lines 287-291, 325-327, etc.).- Comprehensive coverage: Handles timeouts at multiple stages—during launch, post-launch, execution, subagent termination, and catch blocks—ensuring consistent behavior.
- Cleanup: Properly clears timeout timers and removes event listeners in all code paths, including teardown and error branches.
- Result construction: The new
createTimeoutResulthelper (lines 516-546) produces standardizedToolResultobjects withTIMEOUTerror type, partial output, and clear metadata.- Schema documentation: The
timeout_msparameter is well-documented for LLM consumption (lines 634-638).The threading of
maxTimeMinutesintorunConfig.max_time_minutes(lines 145-147) correctly propagates the timeout constraint to the orchestrator and subagent layers.Also applies to: 127-127, 139-139, 145-147, 230-250, 287-291, 304-306, 317-320, 325-361, 410-430, 451-465, 516-546, 565-565, 634-638, 666-666
Comment on lines
+176
to
+213
| it('rejects invalid timeout settings', async () => { | ||
| const invalidCases = [ | ||
| { | ||
| key: 'task_default_timeout_ms', | ||
| value: '-5', | ||
| expectedError: | ||
| 'must be a positive integer in milliseconds or -1 for unlimited', | ||
| }, | ||
| { | ||
| key: 'task_max_timeout_ms', | ||
| value: '0', | ||
| expectedError: | ||
| 'must be a positive integer in milliseconds or -1 for unlimited', | ||
| }, | ||
| { | ||
| key: 'shell_default_timeout_ms', | ||
| value: 'not-a-number', | ||
| expectedError: | ||
| 'must be a positive integer in milliseconds or -1 for unlimited', | ||
| }, | ||
| { | ||
| key: 'shell_max_timeout_ms', | ||
| value: '1.5', | ||
| expectedError: | ||
| 'must be a positive integer in milliseconds or -1 for unlimited', | ||
| }, | ||
| ]; | ||
|
|
||
| for (const { key, value, expectedError } of invalidCases) { | ||
| const result = await setCommand.action!(context, `${key} ${value}`); | ||
| expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled(); | ||
| expect(result).toEqual({ | ||
| type: 'message', | ||
| messageType: 'error', | ||
| content: expect.stringContaining(expectedError), | ||
| }); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Reset mocks before invalid case assertions.
Since the previous test (validates task timeout settings) calls setEphemeralSetting successfully, the mock will have recorded those calls. The assertion expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled() on line 206 will fail because the mock retains state from previous tests unless cleared.
This test should clear mocks before running or at the start of each iteration.
🔎 Proposed fix
it('rejects invalid timeout settings', async () => {
const invalidCases = [
{
key: 'task_default_timeout_ms',
value: '-5',
expectedError:
'must be a positive integer in milliseconds or -1 for unlimited',
},
// ... other cases
];
for (const { key, value, expectedError } of invalidCases) {
+ vi.clearAllMocks();
const result = await setCommand.action!(context, `${key} ${value}`);
expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled();🤖 Prompt for AI Agents
In @packages/cli/src/ui/commands/setCommand.test.ts around lines 176-213, The
failing test leaves mock state from prior tests, so before asserting
mockRuntime.setEphemeralSetting was not called you must clear the mock(s);
update the "rejects invalid timeout settings" test (around the it block calling
setCommand.action) to call a mock reset such as
mockRuntime.setEphemeralSetting.mockClear() or jest.clearAllMocks() before the
for-loop (or at start of each iteration) so each invalid case begins with a
clean mock and the
expect(mockRuntime.setEphemeralSetting).not.toHaveBeenCalled() assertion is
reliable.
…loses #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
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@CoreRabbit I don't see how this fully implements #920 as it just added the ephemerals but didn't add the optional parameters to the shell or task tool calls - am I wrong? |
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/tools/shell.ts (2)
269-282: Minor: UnnecessaryremoveEventListenercall before listener is added.At line 271,
signal.removeEventListener('abort', onUserAbort)is called before the listener is actually added at line 282. While this is harmless (removing a non-existent listener is a no-op), it's unnecessary and slightly confusing. Consider removing line 271 or restructuring the flow for clarity.🔎 Suggested simplification
if (signal.aborted) { onUserAbort(); - signal.removeEventListener('abort', onUserAbort); return { llmContent: 'Command was cancelled by user before it could start.', returnDisplay: 'Command cancelled by user.', error: { message: 'Command was cancelled by user before it could start.', type: ToolErrorType.EXECUTION_FAILED, }, }; } signal.addEventListener('abort', onUserAbort, { once: true });
516-530: Consider refactoring the nested ternary for readability.The nested ternary operator spans multiple lines and is difficult to parse. Consider extracting this logic into a helper function or using early returns / if-else for clarity.
🔎 Proposed refactor
function determineExecutionError( result: ShellExecutionResult | null, timeoutController: AbortController, signal: AbortSignal, llmContent: string, ): { error?: { message: string; type: ToolErrorType } } { if (result?.error) { return { error: { message: result.error.message, type: ToolErrorType.SHELL_EXECUTE_ERROR, }, }; } if (result?.aborted && timeoutController.signal.aborted && !signal.aborted) { return { error: { message: llmContent, type: ToolErrorType.TIMEOUT, }, }; } if (result?.aborted) { return { error: { message: llmContent, type: ToolErrorType.EXECUTION_FAILED, }, }; } return {}; }packages/core/src/tools/task.ts (1)
525-531: Minor type safety concern with ephemeral settings access.The code casts ephemeral setting values to
number | undefined:(settings.task_default_timeout_ms as number | undefined)While this works, consider adding a helper function or type guard to safely extract and validate numeric settings, especially since invalid values could come from user configuration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/tools/shell.test.tspackages/core/src/tools/shell.tspackages/core/src/tools/task.test.tspackages/core/src/tools/task.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/tools/task.test.tspackages/core/src/tools/shell.test.tspackages/core/src/tools/task.tspackages/core/src/tools/shell.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/core/src/tools/task.test.tspackages/core/src/tools/task.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/tools/task.test.tspackages/core/src/tools/task.tspackages/core/src/tools/shell.ts
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: Shell tool (packages/core/src/tools/shell.ts) always applies tool-output-max-tokens via limitOutputTokens to the model-facing llmContent at the end of execute(), formatting a warning when truncated.
Applied to files:
packages/core/src/tools/shell.test.tspackages/core/src/tools/shell.ts
📚 Learning: 2025-12-05T14:59:34.368Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T14:59:34.368Z
Learning: Use subagents (general-purpose, typescript-code-reviewer, or typescript-coder) for complex multi-step tasks, research, complex implementations, code review, and extensive file searching
Applied to files:
packages/core/src/tools/task.ts
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
Applied to files:
packages/core/src/tools/shell.ts
🧬 Code graph analysis (1)
packages/core/src/tools/task.ts (4)
packages/core/src/core/subagentOrchestrator.ts (1)
SubagentLaunchRequest(55-61)packages/core/src/core/subagent.ts (1)
OutputObject(89-104)packages/core/src/tools/tools.ts (1)
ToolResult(423-451)packages/core/src/core/turn.ts (1)
DEFAULT_AGENT_ID(39-39)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
🔇 Additional comments (26)
packages/core/src/tools/task.test.ts (6)
7-12: LGTM!The
afterEachandToolErrorTypeimports are correctly added to support the new timeout handling tests.
91-94: LGTM!The updated assertions correctly verify that
runConfigincludes the timeout (converted to minutes) and that anAbortSignalis passed to the orchestrator launch.
356-363: LGTM!The test suite correctly sets up and tears down fake timers for timeout testing.
524-527: Verify the unlimited timeout assertion.The assertion
expect.not.objectContaining({ runConfig: expect.anything() })expects norunConfigto be passed whentimeout_msis -1. However, looking at the implementation intask.ts, whentimeoutMsisundefined(unlimited),createLaunchRequestreturns a request withoutrunConfigonly if no timeout is set.This may cause the test to fail if other parts of the code add
runConfigfor non-timeout purposes. Consider asserting specifically thatrunConfig.max_time_minutesis absent or undefined rather than that the entirerunConfigis missing.
532-608: LGTM!The TIMEOUT error test correctly simulates the timeout scenario by:
- Capturing the reject function from
runInteractive- Advancing timers past the timeout
- Manually triggering an AbortError to simulate the subagent detecting the abort
The test verifies that
ToolErrorType.TIMEOUTis returned when the timeout triggers.
610-658: LGTM!The user abort test correctly verifies that
ToolErrorType.EXECUTION_FAILEDis returned (notTIMEOUT) when the user aborts the signal before execution, ensuring proper distinction between timeout and user cancellation.packages/core/src/tools/shell.ts (5)
47-48: LGTM!The default (2 minutes) and max (10 minutes) timeout constants are reasonable defaults for shell command execution.
131-136: LGTM!The
timeout_msparameter is correctly added as an optional number with appropriate documentation.
441-468: LGTM!The timeout detection logic correctly distinguishes between:
- Timeout:
timeoutController.signal.aborted && !signal.aborted- User abort: The else branch handles user-initiated cancellation
The partial output is included in both cases as per the PR objectives.
599-618: LGTM!The
resolveTimeoutMsmethod correctly handles all timeout scenarios:
- Returns
undefined(unlimited) ifrequestedTimeoutMsordefaultTimeoutMsis -1- Skips clamping if
maxTimeoutMsis -1- Clamps to
maxTimeoutMswhen the effective timeout exceeds itThis properly implements the -1/unlimited semantics per the PR objectives.
761-765: LGTM!The schema correctly documents the
timeout_msparameter with the-1 for unlimitedsemantics.packages/core/src/tools/shell.test.ts (6)
196-200: LGTM!The assertions correctly updated to
expect.any(AbortSignal)since the shell tool now passes a combined timeout/abort signal rather than the original user signal.Also applies to: 231-235
348-368: LGTM!The test correctly verifies that when
timeout_msis omitted, the default timeout from ephemeral settings (1234ms) is used to schedule the timeout.
370-393: LGTM!The test correctly verifies that when
timeout_ms(5000) exceeds the maximum setting (2000), the timeout is clamped to the maximum value.
395-418: LGTM!The test correctly verifies that when
timeout_msis -1 (unlimited), nosetTimeoutis scheduled.
420-445: LGTM!The test correctly verifies that when a timeout occurs:
- The error type is
ToolErrorType.TIMEOUT- The
llmContentcontains "timeout_ms" and partial outputThe test advances time past the timeout, then resolves with
aborted: trueto simulate the shell service detecting the abort.
447-459: LGTM!The test correctly verifies that user-initiated aborts (pre-aborted signal) result in
ToolErrorType.EXECUTION_FAILED, notTIMEOUT, ensuring proper error classification.packages/core/src/tools/task.ts (9)
33-34: LGTM!The default (1 minute) and max (5 minutes) timeout constants are reasonable for task execution.
52-52: LGTM!The
timeout_msparameter is correctly added as an optional number.
147-151: LGTM!The timeout is correctly converted from milliseconds to minutes for the subagent's
runConfig.max_time_minutes.
519-554: LGTM!The
createTimeoutControllersmethod correctly:
- Reads ephemeral settings with fallbacks to defaults
- Resolves the effective timeout using
resolveTimeoutMs- Creates an
AbortControllerfor timeout management- Schedules the timeout (or skips if unlimited)
- Sets up user abort handling to clear timeout and propagate abort
556-575: LGTM!The
resolveTimeoutMsimplementation correctly handles all -1/unlimited scenarios, matching the behavior inshell.ts. This addresses the concern from the past review comment.
577-589: LGTM!The
isTimeoutErrormethod correctly identifies timeout scenarios by checking:
timeoutController.signal.abortedis truesignal.aborted(user signal) is false- Any error is either absent or is an
AbortErrorThis ensures user aborts are not misclassified as timeouts.
315-326: LGTM!The
teardownfunction properly cleans up resources:
- Removes the abort handler listener
- Removes the user abort listener
- Clears the timeout if set
- Disposes the subagent scope
All execution paths that reach this point use
await teardown()before returning.
591-614: LGTM!The
createTimeoutResultmethod correctly creates a timeout error response with:
ToolErrorType.TIMEOUTerror typetimedOut: truein metadata- Preservation of any emitted variables from partial execution
668-672: LGTM!The schema correctly documents the
timeout_msparameter with the-1 for unlimitedsemantics.
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/core/src/tools/shell.ts:
- Around line 597-617: The resolveTimeoutMs helper currently returns any numeric
requestedTimeoutMs (including 0 or negative values other than -1), which allows
immediate timeouts; add validation so requestedTimeoutMs must be either
undefined, -1 (meaning no timeout), or a positive integer > 0. Implement this by
adding a guard either at the start of execute() (near the existing parameter
validation) or inside resolveTimeoutMs: if requestedTimeoutMs is a number and
requestedTimeoutMs !== -1 and requestedTimeoutMs <= 0, throw an error (or
reject) indicating invalid timeout_ms; then proceed to call resolveTimeoutMs as
before so only valid positive values (or -1/undefined) reach setTimeout. Ensure
references to resolveTimeoutMs and the execute method are updated accordingly.
🧹 Nitpick comments (1)
packages/core/src/tools/shell.ts (1)
515-529: Error type assignment is functionally correct.The nested ternary correctly assigns
ToolErrorType.TIMEOUTfor timeout-triggered aborts andToolErrorType.EXECUTION_FAILEDfor user-initiated aborts. Consider extracting this to a helper function for improved readability if the logic grows more complex.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/tools/shell.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/tools/shell.ts
🧠 Learnings (3)
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: Shell tool (packages/core/src/tools/shell.ts) always applies tool-output-max-tokens via limitOutputTokens to the model-facing llmContent at the end of execute(), formatting a warning when truncated.
Applied to files:
packages/core/src/tools/shell.ts
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
Applied to files:
packages/core/src/tools/shell.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/tools/shell.ts
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
🔇 Additional comments (8)
packages/core/src/tools/shell.ts (8)
47-49: LGTM!Sensible default values with clear naming. The numeric separators improve readability.
131-136: LGTM!Interface property correctly defined as optional with appropriate documentation.
245-256: LGTM!The timeout resolution logic correctly retrieves ephemeral settings and resolves the effective timeout. The type assertions are acceptable here given the dynamic nature of ephemeral settings.
257-283: LGTM!The AbortController pattern is well-implemented:
- Timeout is only set when
timeoutMsis defined (finite).- The
onUserAbortcallback correctly clears the timeout to prevent dangling timers.- Using
{ once: true }for the event listener is a good practice.- Early abort handling correctly returns before registering listeners.
379-379: LGTM!Correctly passes the combined signal to the execution service, allowing either timeout or user abort to cancel the operation.
440-467: LGTM!The timeout detection logic (
timeoutController.signal.aborted && !signal.aborted) correctly distinguishes between timeout and user-initiated abort. Partial output is properly included in both cases as required.
588-591: LGTM!Comprehensive cleanup in the
finallyblock ensures no resource leaks:
- Timeout is cleared to prevent dangling timers.
- Event listener is removed to prevent memory leaks.
760-764: LGTM with note on validation.The schema correctly exposes
timeout_msas an optional numeric parameter. While JSON Schema constraints likeminimumcould be added, runtime validation in code (as suggested above) is the reliable approach since LLMs may not always respect schema constraints.
Comment on lines
+597
to
+617
|
|
||
| private resolveTimeoutMs( | ||
| requestedTimeoutMs: number | undefined, | ||
| defaultTimeoutMs: number, | ||
| maxTimeoutMs: number, | ||
| ): number | undefined { | ||
| if (requestedTimeoutMs === -1 || defaultTimeoutMs === -1) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const effectiveTimeout = requestedTimeoutMs ?? defaultTimeoutMs; | ||
| if (maxTimeoutMs === -1) { | ||
| return effectiveTimeout; | ||
| } | ||
|
|
||
| if (effectiveTimeout > maxTimeoutMs) { | ||
| return maxTimeoutMs; | ||
| } | ||
|
|
||
| return effectiveTimeout; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing validation for timeout_ms parameter can cause immediate timeouts.
The resolveTimeoutMs method does not validate the requestedTimeoutMs value. If an LLM passes timeout_ms: 0 or a negative value other than -1, the method will return that value, causing setTimeout to fire immediately (since JavaScript treats zero/negative delays as zero).
For example:
timeout_ms: 0→ returns0→ immediate timeouttimeout_ms: -5→ returns-5→ immediate timeout (negative treated as 0 bysetTimeout)
Add validation in the execute() method or within resolveTimeoutMs to reject invalid values:
🔎 Proposed fix
Add validation at the start of the execute() method alongside other parameter validations (around line 239):
if (this.params.grep_flags) {
validateGrepFlags(this.params.grep_flags);
}
+ if (
+ this.params.timeout_ms !== undefined &&
+ this.params.timeout_ms !== -1 &&
+ (!Number.isInteger(this.params.timeout_ms) || this.params.timeout_ms <= 0)
+ ) {
+ throw new Error(
+ `timeout_ms must be a positive integer or -1 (unlimited), got: ${this.params.timeout_ms}`,
+ );
+ }
const strippedCommand = stripShellWrapper(this.params.command);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/core/src/tools/shell.ts around lines 597 - 617, The
resolveTimeoutMs helper currently returns any numeric requestedTimeoutMs
(including 0 or negative values other than -1), which allows immediate timeouts;
add validation so requestedTimeoutMs must be either undefined, -1 (meaning no
timeout), or a positive integer > 0. Implement this by adding a guard either at
the start of execute() (near the existing parameter validation) or inside
resolveTimeoutMs: if requestedTimeoutMs is a number and requestedTimeoutMs !==
-1 and requestedTimeoutMs <= 0, throw an error (or reject) indicating invalid
timeout_ms; then proceed to call resolveTimeoutMs as before so only valid
positive values (or -1/undefined) reach setTimeout. Ensure references to
resolveTimeoutMs and the execute method are updated accordingly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds CLI configuration support for tool-level timeouts as requested in issue #920. The commit adds the CLI settings surface for users to configure timeout defaults and limits for the task (TaskTool) and run_shell_command (ShellTool) tools.
Background
Issue #920 requested the ability to add tool-level timeouts for the task and shell tools. The core implementation was already complete in packages/core/src/tools/task.ts and packages/core/src/tools/shell.ts, which supports:
What was missing: The CLI did not expose the ephemeral settings that allow users to configure these timeout values.
Changes Made
CLI Settings (packages/cli/src/settings/ephemeralSettings.ts)
Added 4 new ephemeral setting keys with help text:
Added validation logic: Accepts positive integers OR -1 for unlimited, rejects other negatives, zero, non-integers
Tests (packages/cli/src/ui/commands/setCommand.test.ts)
Documentation (docs/settings-and-profiles.md)
closes #920
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.