SANDBOX-1813: Implement bash MCP tool handler - #18
Conversation
- Add pkg/tools with BashTool handler, BashInput/BashOutput types, and CommandExecutor interface (satisfied by *session.SessionManager) - Extract X-Session-ID from request headers; reject if missing - Clamp timeout: default 60s, max 300s - Non-zero exit codes return IsError with structured output retained - Infrastructure failures return tool-level errors (option A) - Unit tests with mock executor covering all AC scenarios - Add github.com/modelcontextprotocol/go-sdk v1.4.0 dependency Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughAdds an MCP bash tool with structured input/output, session validation, bounded timeouts, injectable command execution, non-zero exit handling, MCP registration, and unit tests. Updates Go module requirements for the MCP SDK and related indirect dependencies. ChangesBash MCP tool
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant BashTool
participant CommandExecutor
MCPClient->>BashTool: Call bash with command and X-Session-ID
BashTool->>BashTool: Validate request and clamp timeout
BashTool->>CommandExecutor: ExecuteCommand
CommandExecutor-->>BashTool: Return execution response
BashTool-->>MCPClient: Return BashOutput and call result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tools/bash_test.go (1)
207-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the server actually exposes the registered tool.
This test passes even if
RegisterWithis a no-op because Line 219 only inspectstool.Tool(). Use the SDK’s tool-listing or invocation path to verify thatserverexposesbash, as required by the PR objective.🤖 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 `@pkg/tools/bash_test.go` around lines 207 - 220, Update TestRegisterWith to verify the server exposes the registered bash tool through the MCP server’s SDK tool-listing or invocation API, rather than only asserting tool.Tool().Name. Keep the existing registration setup and assert that the exposed tool is named “bash,” ensuring the test fails when RegisterWith is a no-op.
🤖 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.
Inline comments:
In `@go.mod`:
- Line 9: Update the github.com/modelcontextprotocol/go-sdk dependency in go.mod
to version v1.4.1 or newer, then regenerate or update go.sum so it contains the
matching checksum entries.
---
Nitpick comments:
In `@pkg/tools/bash_test.go`:
- Around line 207-220: Update TestRegisterWith to verify the server exposes the
registered bash tool through the MCP server’s SDK tool-listing or invocation
API, rather than only asserting tool.Tool().Name. Keep the existing registration
setup and assert that the exposed tool is named “bash,” ensuring the test fails
when RegisterWith is a no-op.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6513cfa4-d698-4d3b-b697-e3d556175237
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/tools/bash.gopkg/tools/bash_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/tools/bash.gopkg/tools/bash_test.gogo.mod
🪛 OSV Scanner (2.4.0)
go.mod
[HIGH] 9-9: github.com/modelcontextprotocol/go-sdk 1.4.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
(GO-2026-4770)
[HIGH] 9-9: github.com/modelcontextprotocol/go-sdk 1.4.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
(GO-2026-4773)
[HIGH] 9-9: github.com/modelcontextprotocol/go-sdk 1.4.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
[HIGH] 9-9: github.com/modelcontextprotocol/go-sdk 1.4.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
🔇 Additional comments (3)
go.mod (1)
27-38: LGTM!pkg/tools/bash.go (1)
1-112: LGTM!pkg/tools/bash_test.go (1)
1-205: LGTM!
- Address CodeRabbit review: TestRegisterWith now connects a client via in-memory transport and asserts the server exposes "bash" tool - Test fails if RegisterWith is a no-op Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/tools/bash_test.go`:
- Line 226: Update the test setup around server.Connect to retain the returned
*mcp.ServerSession, then close that session during t.Cleanup alongside
cs.Close(). Handle and report any session-close error so teardown failures are
not silently ignored.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ea719c62-8d9f-43d7-8046-9e3b38f4d402
📒 Files selected for processing (1)
pkg/tools/bash_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: govulncheck
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/tools/bash_test.go
🪛 golangci-lint (2.12.2)
pkg/tools/bash_test.go
[error] 226-226: Error return value of cs.Close is not checked
(errcheck)
🔇 Additional comments (1)
pkg/tools/bash_test.go (1)
207-225: LGTM!Also applies to: 227-232
- Capture *ServerSession from server.Connect and close in t.Cleanup - Check and report Close() errors for both server and client sessions - Addresses CodeRabbit review: errcheck lint and resource leak Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
1 participant
pkg/tools/bash.go—bashMCP tool handler withBashInput/BashOutputtypes,CommandExecutorinterface, session ID extraction fromX-Session-IDheader, and timeout clamping (default 60s, max 300s)IsError: truewith structured output retained (stdout/stderr/exit_code/duration_ms available to the LLM)IsErrorwithoutBashOutput(option A per design doc)Assisted By: Cursor
Summary by CodeRabbit