Add Go M0 bash tool#49
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a workspace-scoped Bash execution tool exposing ChangesBash Tool Implementation and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tools/bash_tool_test.go (1)
84-177: ⚡ Quick winAdd a registry “grant allows execution” integration test.
Current tests prove denial without grant and direct
NewBashTool(...).Run(...)success, but they do not verify thatRegistryactually permits bash execution when the required grant is present. Adding this test closes the permission-gating contract end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/bash_tool_test.go` around lines 84 - 177, Add an integration test that verifies Registry grants allow bash execution: create a test (e.g. TestBashToolAllowsExecutionWhenRegistryGrantPresent) that constructs a Registry instance with the specific permission/grant that enables bash execution, register or inject that Registry so NewBashTool(...) uses it, then call NewBashTool(root).Run(ctx, map[string]any{"command": helperCommand("success")}) and assert result.Status == StatusOK and stdout contains the expected output; mirror existing assertions (stdout contains "hello from bash", exit_code meta "0") to prove the grant actually permits execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tools/bash_tool_test.go`:
- Around line 84-177: Add an integration test that verifies Registry grants
allow bash execution: create a test (e.g.
TestBashToolAllowsExecutionWhenRegistryGrantPresent) that constructs a Registry
instance with the specific permission/grant that enables bash execution,
register or inject that Registry so NewBashTool(...) uses it, then call
NewBashTool(root).Run(ctx, map[string]any{"command": helperCommand("success")})
and assert result.Status == StatusOK and stdout contains the expected output;
mirror existing assertions (stdout contains "hello from bash", exit_code meta
"0") to prove the grant actually permits execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 336341d2-b072-48bd-817e-02ea503f6f2f
📒 Files selected for processing (3)
internal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/registry.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- Workspace boundary is enforced through
resolveWorkspacePath(internal/tools/bash.go:54-56), so the sameEvalSymlinks+..rejection that protects the read/write tools applies to the shell'scwd.command.Dir = absoluteCwdtherefore always points at a real, symlink-resolved directory inside the workspace. - Timeout is bounded both at the schema layer (
Minimum: 1, Maximum: 600000) and at the runtime layer (context.WithTimeout+exec.CommandContext,internal/tools/bash.go:58-60, 62-63). When the deadline fires,errors.Is(commandCtx.Err(), context.DeadlineExceeded)is checked before the genericerr != nilbranch, so timeout reports don't get misclassified as a regular failure. - OS-aware shell invocation:
shellExecutable/shellArguments(internal/tools/bash.go:107-116) returncmd.exe /d /s /c <command>on Windows and/bin/sh -c <command>elsewhere. The/d /s /ctriple is the documented safe form for one-shot command execution on Windows; theset -e/set -udefaults from a login shell don't leak in. - Result metadata carries
cwd,exit_code, andtimeout_ms(internal/tools/bash.go:69-72), andformatBashOutput(internal/tools/bash.go:135-150) only emitsexit_code: Nfor non-zero exits — keeps happy-path output clean. Stdout/stderr are trimmed of trailing newlines and only rendered if non-empty. - Registry wiring:
CoreShellToolsexposes justbash, andCoreToolsappends it after read-only and write tools. Permission isSideEffectShell/PermissionPrompt, matchingapply_patch/write_file/edit_file— consistent safety tier. - Test harness is self-contained:
TestMain(internal/tools/bash_tool_test.go:13-22) detects--zero-bash-helper <name>and dispatches to a tiny in-process command library (success,stderr,fail,pwd,sleep). Avoids the cross-platform headache of depending onecho/cat/sleephaving the same flags everywhere, andshellQuotehandles the POSIX quoting. CI green on all four matrices; CodeRabbit reports SUCCESS.
Observations (non-blocking)
-
Unbounded stdout/stderr buffers.
var stdout bytes.Buffer; var stderr bytes.Buffer(internal/tools/bash.go:65-67) have no size cap, and the onlyRuncompletion path that returns early is the timeout. Acat /dev/urandom | head -c 10Gstyle command (or any noisy build) can OOM the agent process before the timeout fires, becausecommand.Runonly returns after the child closes its stdout/stderr — which a 10-minute-long noisemaker could legitimately do. Wrapping the writers inio.LimitReader-style guards (e.g.io.Copy(&limitedBuffer{limit: 64 << 20}, command.Stdout)) or checking the buffer size in aWaitDelaycallback would cap the worst case. Worth adding before this lands in front of users with large repos. -
Stdin is inherited from the parent process.
command.Stdinis not set (internal/tools/bash.go:62-67). When the agent is run from a TUI / interactive terminal, the child shell will share that stdin file descriptor with the UI. A bash command that doesreadorcatwould consume whatever the TUI was about to read, and any input the TUI was reading could be reinterpreted as command input. Settingcommand.Stdin = os.Stdinexplicitly is also wrong here; the safe choice is either to leave it nil and document that the tool assumes a non-interactive parent or to point it at/dev/null(oros.DevNull) so the child is guaranteed EOF. -
Timeout exit code is
-1in metadata. When the deadline fires,command.Run()returns the kill error rather than*exec.ExitError, socommandExitCodefalls through to-1(internal/tools/bash.go:118-126). The output string is correct (timed out after Nms), butmeta.exit_code = -1is opaque. A dedicatedmeta["timed_out"] = "true"(and an explicitmeta["exit_code"] = "-1"only if you want to keep the key stable) would let downstream consumers distinguish "command exited 7" from "killed by deadline" without string-matching the output. -
Env vars are inherited as-is. The child sees the full parent env, including anything the agent stashed in
GITHUB_TOKEN,OPENAI_API_KEY,ANTHROPIC_API_KEY, etc. This is the standard behaviour forexec.Cmdand probably what users want, but it's worth a one-line note in the tool description (Description) so the LLM side doesn't accidentally leak secrets into a sub-process that the user can see inps. Pairing with a--inherit-env=falseopt-out (or aenv: {}arg) is a reasonable follow-up. -
Grant-integration test is missing. CodeRabbit's only nitpick on this PR is a
TestBashToolAllowsExecutionWhenRegistryGrantPresentstyle test that goes throughRegistry.RunWithOptionswithPermissionGranted: trueand asserts the command actually runs.TestBashToolRunsCommandInWorkspaceexercises the tool directly, which proves the tool works but doesn't prove the registry's prompt path is wired up for this specific tool. The test forwrite_file(TestRegistryRunsPromptToolsWithGrantin #48) is a good template; copy that for bash and the gating contract is covered end-to-end. -
Workspace cwd uses the read-side resolver.
resolveWorkspacePathrequires the path to exist (EvalSymlinkswill fail onENOENT). Forbashthat's the right choice — a non-existent cwd would error at exec time anyway — but it meanscwd: "./new-dir"errors with theEvalSymlinkslstatmessage rather than the friendlier "directory does not exist" text.errors.Is(err, fs.ErrNotExist)on the resolver error path would let the tool produce a more user-friendly message. Same pattern as the read_file observation in #47. -
Helper binary path is platform-fragile.
helperCommandusesos.Args[0]to relaunch the test binary (internal/tools/bash_tool_test.go:181-184). On Linux this is fine; on Windows the test relies on the test runner using the actual.exerather than a wrapper — and on cross-compiled runs the path could include forward slashes thatcmd.exe /cdoesn't love.shellQuotealready special-cases Windows to skip quoting, which is the right call for now, but aruntime.GOOS == "windows" && !strings.HasSuffix(executable, ".exe")guard might be worth adding if any of the CI matrices starts being flaky.
No blockers. The output-size cap in #1 and the stdin handling in #2 are the two I'd most want to see tightened in a follow-up; the rest are correctness / documentation polish.
2 participants
Summary
Tests
Local blockers
Summary by CodeRabbit
New Features
Tests