Repository navigation
Conversation
b6f08d6 to
191dc02
Compare
There was a problem hiding this comment.
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
letstatements (lines 133, 134, 136, 137, 147, 157, 193) - The guidelines recommend avoidingletstatements where possible. Consider usingconstwith objects to track mutable state. -
Use of
else ifstatements (lines 195-202) - The guidelines recommend avoidingelsestatements 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 + truncatedMessageThese 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 |
f16de3d to
cc0d460
Compare
|
closing this one as it looks like develop has improved now (see #2986) |
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