fix(agent): add Windows fallback for bash tool#1999
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe bash command execution in the agent now supports dual backends: executing via system bash when available, or falling back to an in-process POSIX interpreter. Error handling and output formatting are centralized, with instructions updated to reflect this flexibility and avoid assuming Unix-only tools. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Finder as bashPathFinder
participant BashExec as executeWithBash
participant ProcMgr as Process Manager
participant BashBin as System bash Binary
participant Fallback as executeWithInterpreter
participant Parser as syntax.NewParser
participant Runner as Interpreter Runner
Client->>Finder: Get bash path?
alt Bash Available
Finder-->>Client: bash path found
Client->>BashExec: executeWithBash(ctx, cmd)
BashExec->>ProcMgr: SetupCommand(process)
BashExec->>BashBin: exec.Command(...bash...)<br/>Start process
BashBin-->>BashExec: stdout/stderr/err
Note over BashExec: On ctx cancel:<br/>KillProcessGroup
BashExec-->>Client: commandRunResult<br/>(stdout, stderr, err)
else Bash Unavailable
Finder-->>Client: no bash path
Client->>Fallback: executeWithInterpreter(ctx, cmd)
Fallback->>Parser: Parse command<br/>(LangBash)
Parser-->>Fallback: AST
Fallback->>Runner: Build runner with<br/>stdout/stderr buffers
Runner->>Runner: Run(ctx, AST)
Runner-->>Fallback: buffered output/err
Fallback-->>Client: commandRunResult<br/>(stdout, stderr, err)
end
Client->>Client: buildCommandError(...)<br/>or format output
Client-->>Client: Return ToolOut or<br/>"(no output)"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/agent/bash_test.go (1)
215-311: Add one PATH-based executable case to lock in the fallback contract.These suites cover builtins and shell syntax well, but the new contract also includes “installed CLIs”. A helper-process test here would catch regressions in interpreter PATH/env wiring without depending on platform-specific binaries.
Also applies to: 345-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/bash_test.go` around lines 215 - 311, Add a test case in TestExecuteCommand_InterpreterFallback that exercises a PATH-installed CLI to lock the fallback contract: create a temp dir, write a small executable helper script that prints a distinct marker, chmod it executable, prepend that dir to PATH in the environment used by runCommandWithFinder, then call runCommandWithFinder (same signature used elsewhere in this file) with the helper script name as the command and noBashPath so the interpreter fallback must invoke the PATH binary; assert the result is not an error and contains the marker. Ensure you reference TestExecuteCommand_InterpreterFallback, runCommandWithFinder and noBashPath when locating where to add the case.internal/agent/system_prompt.txt (1)
97-97: Prefer wording that matches the actual fallback syntax mode.
internal/agent/bash.goparses the fallback withsyntax.LangBash, andinternal/agent/bash_test.gonow validates[[ ... ]]in that path. Calling it a “built-in POSIX shell interpreter” will push the agent away from syntax the fallback intentionally supports. “Built-in shell interpreter” or “bash-compatible shell interpreter” would fit the implementation better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/system_prompt.txt` at line 97, The system prompt wording incorrectly calls the fallback a “built-in POSIX shell interpreter”; update the sentence to reflect the actual fallback syntax by using phrasing like “built-in shell interpreter” or “bash-compatible shell interpreter” to match how internal/agent/bash.go parses the fallback with syntax.LangBash and how internal/agent/bash_test.go validates [[ ... ]]; replace the phrase and adjust the example list if needed to avoid implying strict POSIX-only behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/agent/bash.go`:
- Around line 138-189: The current executeWithBash (and the similar command
execution block later in this file) uses bytes.Buffer for cmd.Stdout/cmd.Stderr
which can grow unbounded; replace those buffers with a capped writer/ring buffer
that implements io.Writer and enforces a configurable maxOutputLength (use the
existing maxOutputLength constant), stop accepting bytes once the cap is reached
and record an overflow marker (e.g., "...[truncated]" or similar) so callers
know output was truncated; wire cmd.Stdout and cmd.Stderr to this capped writer,
and when returning commandRunResult ensure you return the capped writer's
contents via its String() method (preserving existing stdout/stderr semantics)
and preserve error handling behavior.
In `@internal/agent/system_prompt.txt`:
- Around line 101-105: The example uses $TMPFILE but never initializes it;
update the snippet or surrounding text so TMPFILE is created before use (e.g.,
via a host-appropriate temp creation like mktemp) or explicitly label the block
as pseudocode that assumes a temp path exists; reference the symbols $TMPFILE
and ${CONTENT} in the guidance so the reader knows to create/obtain TMPFILE
before running the shown printf and some-cli-tool < "$TMPFILE" sequence.
---
Nitpick comments:
In `@internal/agent/bash_test.go`:
- Around line 215-311: Add a test case in TestExecuteCommand_InterpreterFallback
that exercises a PATH-installed CLI to lock the fallback contract: create a temp
dir, write a small executable helper script that prints a distinct marker, chmod
it executable, prepend that dir to PATH in the environment used by
runCommandWithFinder, then call runCommandWithFinder (same signature used
elsewhere in this file) with the helper script name as the command and
noBashPath so the interpreter fallback must invoke the PATH binary; assert the
result is not an error and contains the marker. Ensure you reference
TestExecuteCommand_InterpreterFallback, runCommandWithFinder and noBashPath when
locating where to add the case.
In `@internal/agent/system_prompt.txt`:
- Line 97: The system prompt wording incorrectly calls the fallback a “built-in
POSIX shell interpreter”; update the sentence to reflect the actual fallback
syntax by using phrasing like “built-in shell interpreter” or “bash-compatible
shell interpreter” to match how internal/agent/bash.go parses the fallback with
syntax.LangBash and how internal/agent/bash_test.go validates [[ ... ]]; replace
the phrase and adjust the example list if needed to avoid implying strict
POSIX-only behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 683b2760-fe5d-4338-87e3-860f6478babb
📒 Files selected for processing (5)
internal/agent/bash.gointernal/agent/bash_test.gointernal/agent/inputspill.gointernal/agent/inputspill_test.gointernal/agent/system_prompt.txt
Summary
bashfor the agent bash tool and fall back to the built-inmvdan/shinterpreter whenbashis unavailableWhy
The agent bash tool was hardcoded to
bash -c, which made it unusable on Windows hosts without Git Bash. The surrounding prompt guidance also nudged the agent toward Unix-only helper commands, so even a shell fallback needed clearer host-agnostic instructions.Test plan
go test ./internal/agent -count=1Summary by CodeRabbit
Release Notes
New Features
Documentation