Skip to content

fix(agent): add Windows fallback for bash tool#1999

Merged
yottahmd merged 3 commits intomainfrom
fix/agent-bash-windows-fallback
Apr 12, 2026
Merged

fix(agent): add Windows fallback for bash tool#1999
yottahmd merged 3 commits intomainfrom
fix/agent-bash-windows-fallback

Conversation

@yottahmd
Copy link
Copy Markdown
Collaborator

@yottahmd yottahmd commented Apr 12, 2026

Summary

  • prefer system bash for the agent bash tool and fall back to the built-in mvdan/sh interpreter when bash is unavailable
  • preserve timeout, cancellation, working-directory, and output handling across both execution paths
  • update agent prompt guidance and tests so fallback hosts are no longer assumed to have Unix-only helper binaries installed

Why

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=1

Summary by CodeRabbit

Release Notes

  • New Features

    • Bash tool now automatically uses a built-in interpreter when system bash is unavailable, improving compatibility across environments.
  • Documentation

    • Updated guidance to avoid assuming Unix-only tools are installed, with clearer instructions for handling large inputs and inter-step data passing.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2337c098-0ebe-4c8c-9ece-f35508b46882

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The 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

Cohort / File(s) Summary
Bash Execution Backends
internal/agent/bash.go
Introduced executeCommandWithFinder(...) to abstract command execution, routing to executeWithBash(...) (system binary) or executeWithInterpreter(...) (in-process parser/runner). Added bashPathFinder, commandRunResult, buildCommandError(...), and toolErrorWithOutput(...) for unified error handling and timeout/cancellation reporting. Changed successful output behavior to return "(no output)" when no combined stdout/stderr is present.
Bash Execution Tests
internal/agent/bash_test.go
Updated timeout test to use infinite busy-loop instead of sleep. Added test helpers (noBashPath, bashPathForTests, runCommandWithFinder). Introduced comprehensive test suites for both execution backends, covering fallback interpreter mode (output handling, stderr labeling, non-zero exit, pipelines, substitution, bash syntax, parse errors, timeout/cancellation) and explicit bash path mode.
Agent Instructions & Guidance
internal/agent/inputspill.go, internal/agent/inputspill_test.go, internal/agent/system_prompt.txt
Updated instructions to avoid assuming Unix-only helper binaries and to prefer the read tool over cat. Modified system prompt to clarify bash may use a built-in interpreter when unavailable, removed mktemp/trap example, and changed DAG discovery wording from explicit file listing to "inspecting the DAGs directory."

Sequence Diagram

sequenceDiagram
    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)"
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: adding a Windows fallback for the bash tool by implementing a fallback to a built-in interpreter when system bash is unavailable.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/agent-bash-windows-fallback

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go parses the fallback with syntax.LangBash, and internal/agent/bash_test.go now 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

📥 Commits

Reviewing files that changed from the base of the PR and between 84f7492 and 4eb8646.

📒 Files selected for processing (5)
  • internal/agent/bash.go
  • internal/agent/bash_test.go
  • internal/agent/inputspill.go
  • internal/agent/inputspill_test.go
  • internal/agent/system_prompt.txt

Comment thread internal/agent/bash.go
Comment thread internal/agent/system_prompt.txt Outdated
@yottahmd yottahmd merged commit 64b39d6 into main Apr 12, 2026
3 checks passed
@yottahmd yottahmd deleted the fix/agent-bash-windows-fallback branch April 12, 2026 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant