-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix: Improve bash tool timeout and output handling #1811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
b6f08d6
to
191dc02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AGENTS.md Guidelines Review
I've reviewed this PR against the guidelines in AGENTS.md and found several suggestions for improvement. These are suggestions to help maintain code consistency - you can decide whether to address them.
Main Violations Found:
-
Multiple
let
statements (lines 133, 134, 136, 137, 147, 157, 193) - The guidelines recommend avoidinglet
statements where possible. Consider usingconst
with objects to track mutable state. -
Use of
else if
statements (lines 195-202) - The guidelines recommend avoidingelse
statements unless necessary.
Suggested Improvements:
For the mutable variables:
const terminatedState = { terminated: false, byTimeout: true }
const outputState = { stderr: "", stdout: "" }
const sizeState = { stdout: 0, stderr: 0 }
For the conditional logic:
const baseOutput = `<stdout>${stdout}</stdout>\n<stderr>${stderr}</stderr>\n<exitCode>${process.exitCode}</exitCode>`
const timeoutMessage = terminated && terminatedByTimeout
? `\n<timeout>Process was terminated by timeout after ${timeout}ms...</timeout>`
: ''
const truncatedMessage = terminated && (stdoutSize >= MAX_OUTPUT_LENGTH || stderrSize >= MAX_OUTPUT_LENGTH) && !terminatedByTimeout
? `\n<outputTruncated>Output was truncated to ${MAX_OUTPUT_LENGTH} characters...</outputTruncated>`
: ''
const forLLM = baseOutput + timeoutMessage + truncatedMessage
These are suggestions - feel free to keep the current approach if it works better for your use case!
e9efcbe
to
0a52eb8
Compare
In response to the bot above:
|
b80f207
to
eae2424
Compare
eae2424
to
4ffd9ed
Compare
1c05edf
to
02cd7ad
Compare
02cd7ad
to
d106ad1
Compare
d106ad1
to
21aa7c8
Compare
My own two centsBackgroundI am working on an Ansible repo right now and some of my playbook runs can easily exceed 30 minutes for the test runs when spinning up a cluster and provisioning it. ObservationsLatest version of Currently what happens when I deploy a playbook is that after a while (I have not measured the duration), the bash tool returns. But the process is still running in the background, it is not being timed out correctly. Only the tool call times out. The output is being cut off. My current workaround is to tell the agent to stream the output into a log file and to periodically check whether the process is still running ( Claude Code will kill the process after the bash tool times out. It also allows the LLM to set a parameter to override the default timeout, which I find to be a good solution, since I can just tell the agent which duration to expect with which type of command.
The projects everyone is working on are so diverse that I do not think a hardcoded maximum is valuable at all, maybe as a config option to set:
A tool needs to not be in the way to be useful. Maybe a bit off topic but you be the judgeFurthermore, for long outputs, other tools display the first and the last e.g. 50 lines and display how many lines have been truncated from the output. As the bash tool is now, it will sometimes just return due to the output having become too long. It is not very useful that way, since Lmk if you wish me to make my own PR or whether you'd like to collab. |
Hey @lukidoescode. I have since made this MCP which I use in preference to the bash tool (I disable the bash tool globally) https://github.com/xhuw/async-bash-mcp you might be interested. It runs commands asynchronously and lets the llm poll the output. I am surprised you see that the process continues running as the "timeout" parameter is passed directly to
so timeout should kill the process. The issues this PR is trying to solve is the fact that with the current bash tool, the LLM wont even know that the script has timed out. I saw very odd behaviour after timeouts where it would say things like "Lets wait for it to finish runs |
My attempt to resolve #1721
2 key changes:
10 minute max timeout is still too short. I am not sure why its there though so not changing it