Read timeout for command_execute #134
Conversation
WalkthroughThe updates introduce an ASCII art banner to the setup process, clarify the "read_output" tool's description, and enhance terminal output reading with asynchronous waiting and timeout support. Schema changes enforce a required timeout for command execution and allow an optional timeout for reading output. A new method enables direct terminal session access by PID. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant TerminalManager
Client->>Server: Call readOutput with pid and optional timeout_ms
Server->>TerminalManager: getSession(pid)
alt Session exists
loop Poll for new output (every 300ms, up to timeout_ms)
TerminalManager-->>Server: Check for new output
end
alt Output found
Server-->>Client: Return new output
else Timeout reached
Server-->>Client: Return timeout message
end
else Session not found
Server-->>Client: Return error (session not found)
end
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
setup-claude-server.js (1)
521-530: Nice branding addition, though unrelated to the main PR objective.The ASCII art banner adds a professional touch to the setup process. The placement is appropriate and doesn't interfere with any functionality.
src/tools/execute.ts (1)
142-148: Consider adding parentheses for clarity.The string concatenation logic is correct but could be clearer with explicit parentheses.
- text: output || 'No new output available' + (timeoutReached ? ' (timeout reached)' : '') + text: output || ('No new output available' + (timeoutReached ? ' (timeout reached)' : ''))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
setup-claude-server.js(2 hunks)src/server.ts(1 hunks)src/terminal-manager.ts(1 hunks)src/tools/execute.ts(1 hunks)src/tools/schemas.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/terminal-manager.ts (1)
src/types.ts (1)
TerminalSession(10-16)
src/tools/execute.ts (2)
src/tools/schemas.ts (1)
ReadOutputArgsSchema(23-26)src/terminal-manager.ts (1)
terminalManager(174-174)
🔇 Additional comments (6)
src/tools/schemas.ts (2)
25-25: Good addition of optional timeout for read operations.Adding the optional
timeout_msparameter toReadOutputArgsSchemaprovides flexibility for callers to specify how long to wait for output without breaking existing functionality.
19-19:✅ Verification successful
Verify the breaking change impact.
Making
timeout_msrequired inExecuteCommandArgsSchemais a breaking change that could affect existing callers. Ensure all existing code that callsexecute_commandhas been updated to provide the required timeout parameter.
🏁 Script executed:
#!/bin/bash # Description: Verify all execute_command calls include timeout_ms parameter # Expected: All calls should include timeout_ms parameter # Search for execute_command function calls ast-grep --pattern 'execute_command($$$)' # Search for executeCommand function calls (camelCase variant) ast-grep --pattern 'executeCommand($$$)' # Search for handleExecuteCommand calls ast-grep --pattern 'handleExecuteCommand($$$)'Length of output: 302
🏁 Script executed:
#!/bin/bash # Show ExecuteCommandArgsSchema definition rg -n -C3 "ExecuteCommandArgsSchema" -n src/tools/schemas.ts # Locate executeCommand definition with context rg -n -C3 "executeCommand" -n srcLength of output: 2415
🏁 Script executed:
#!/bin/bash # Display the import and call site in the test to verify usage of executeCommand sed -n '1,120p' test/test-blocked-commands.jsLength of output: 3474
All executeCommand call sites supply
timeout_ms– no action needed.Verification discovered only two invoke points:
- src/handlers/terminal-handlers.ts (line 22) calls
executeCommand(parsed), and the schema parse guaranteesparsed.data.timeout_msis present.- test/test-blocked-commands.js’s wrapper always sets
args.timeout_msbefore callingexecuteCommandAPI(args).No other calls to
executeCommandexist in the codebase. This breaking change has been accommodated everywhere and will not introduce missing‐parameter errors.src/server.ts (1)
338-338: Clear and helpful description update.The updated description effectively communicates the new timeout functionality for long-running commands, helping users understand when and how to use the timeout parameter.
src/terminal-manager.ts (1)
129-136: Well-implemented session accessor method.The
getSessionmethod is cleanly implemented with proper documentation and type safety. The method signature correctly returnsTerminalSession | undefined, making it clear when a session might not be found.src/tools/execute.ts (2)
65-72: LGTM! Proper argument validation.The argument parsing follows the established pattern with appropriate error handling.
73-82: LGTM! Good defensive programming.The session existence check prevents errors when trying to read from non-existent PIDs, and the default timeout of 5 seconds is reasonable.
| let output = ""; | ||
| let timeoutReached = false; | ||
| try { | ||
| // Create a promise that resolves when new output is available or when timeout is reached | ||
| const outputPromise: Promise<string> = new Promise<string>((resolve) => { | ||
| // Check for initial output | ||
| const initialOutput = terminalManager.getNewOutput(pid); | ||
| if (initialOutput && initialOutput.length > 0) { | ||
| resolve(initialOutput); | ||
| return; | ||
| } | ||
|
|
||
| let resolved = false; | ||
| let interval: NodeJS.Timeout | null = null; | ||
| let timeout: NodeJS.Timeout | null = null; | ||
|
|
||
| const cleanup = () => { | ||
| if (interval) { | ||
| clearInterval(interval); | ||
| interval = null; | ||
| } | ||
| if (timeout) { | ||
| clearTimeout(timeout); | ||
| timeout = null; | ||
| } | ||
| }; | ||
|
|
||
| const resolveOnce = (value: string, isTimeout = false) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| cleanup(); | ||
| if (isTimeout) timeoutReached = true; | ||
| resolve(value); | ||
| }; | ||
|
|
||
| // Setup an interval to poll for output | ||
| interval = setInterval(() => { | ||
| const newOutput = terminalManager.getNewOutput(pid); | ||
| if (newOutput && newOutput.length > 0) { | ||
| resolveOnce(newOutput); | ||
| } | ||
| }, 300); // Check every 300ms | ||
|
|
||
| // Set a timeout to stop waiting | ||
| timeout = setTimeout(() => { | ||
| const finalOutput = terminalManager.getNewOutput(pid) || ""; | ||
| resolveOnce(finalOutput, true); | ||
| }, timeout_ms); | ||
| }); | ||
|
|
||
| output = await outputPromise; | ||
| } catch (error) { | ||
| return { | ||
| content: [{ type: "text", text: `Error reading output: ${error}` }], | ||
| isError: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider optimizing the polling interval.
The implementation is well-structured with proper cleanup and race condition handling. However, the 300ms polling interval might cause unnecessary CPU usage, especially when multiple sessions are being monitored.
Consider making the polling interval configurable or using an exponential backoff strategy.
Additionally, verify that the code handles the case where a session terminates while waiting for output:
🏁 Script executed:
#!/bin/bash
# Description: Check if terminalManager handles terminated sessions gracefully
# Search for session termination handling in terminal-manager
ast-grep --pattern 'getNewOutput($_, $_) {
$$$
}'
# Look for any session state checks
rg -A 10 "getNewOutput|getSession" src/terminal-manager.tsLength of output: 698
Optimize polling interval in execute.ts
The hard-coded 300 ms polling interval can add up under load. Consider making it configurable (e.g. via an optional parameter or environment setting) or switching to an exponential backoff strategy to reduce CPU usage when output is sparse.
Note: session termination is already handled by terminalManager.getNewOutput(), which checks both active and completed sessions. No additional checks are needed for cleanup here.
• File: src/tools/execute.ts
• Lines: ~84–140
🤖 Prompt for AI Agents
In src/tools/execute.ts around lines 84 to 140, the polling interval for
checking new output is hard-coded to 300ms, which can cause unnecessary CPU
usage under load. Modify the code to make the polling interval configurable by
adding an optional parameter or reading from an environment variable.
Alternatively, implement an exponential backoff strategy that increases the
interval between polls when no new output is detected, resetting it when output
arrives. This will optimize resource usage while maintaining responsiveness.
Summary by CodeRabbit
New Features
Enhancements
Breaking Changes