SANDBOX-1813: add openspec for bash MCP tool handler - #17
Conversation
- Define bash tool registration, X-Session-ID routing, and timeout clamping - Lock error model to tool IsError (option A), not JSON-RPC protocol errors - Include CommandExecutor interface and unit-test task plan Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
WalkthroughThe OpenSpec change documents a typed MCP ChangesBash MCP tool specification
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md (1)
148-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin an exact SDK version.
Resolving an unspecified patch at implementation time makes this OpenSpec non-reproducible and can change typed-handler behavior between implementations. Record the exact module version in
go.mod/go.sumand validate the selected version’s schema and error-mapping behavior. The SDK documents these transformations as responsibilities of typedAddTool. (pkg.go.dev)As per path instructions, this is a material maintainability and integration concern, not a version-style nitpick.
🤖 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 `@openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md` around lines 148 - 155, Update Decision 8 to specify the exact github.com/modelcontextprotocol/go-sdk patch version rather than the v1.4.x range, and require recording that version in go.mod and go.sum. Validate the selected SDK version’s schema and error-mapping behavior through the typed AddTool integration before finalizing the change.Source: Path instructions
openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md (1)
33-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an end-to-end typed-SDK registration test.
Listing the tool does not verify generated input/output schemas,
ReadOnlyHint,StructuredContent, or typed error packing. Add an in-memory MCP server/client call covering exit 0, exit 1, and executor errors; otherwise direct handler tests can pass while the actualtools/callwire behavior is broken. The SDK documents theseAddTooltransformations explicitly. (pkg.go.dev)As per path instructions, this targets a high-value integration gap rather than test-style cleanup.
🤖 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 `@openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md` around lines 33 - 45, Add an end-to-end typed-SDK MCP registration test alongside the existing bash tests, using an in-memory server/client and an injected mock CommandExecutor to invoke the registered bash tool through tools/call. Cover exit 0, exit 1, and executor-error cases, asserting generated input/output schemas, ReadOnlyHint, StructuredContent, and typed error packing on the wire rather than only testing the handler or tool listing.Source: Path instructions
🤖 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 `@openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md`:
- Around line 148-155: Update Decision 8 to specify the exact
github.com/modelcontextprotocol/go-sdk patch version rather than the v1.4.x
range, and require recording that version in go.mod and go.sum. Validate the
selected SDK version’s schema and error-mapping behavior through the typed
AddTool integration before finalizing the change.
In `@openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md`:
- Around line 33-45: Add an end-to-end typed-SDK MCP registration test alongside
the existing bash tests, using an in-memory server/client and an injected mock
CommandExecutor to invoke the registered bash tool through tools/call. Cover
exit 0, exit 1, and executor-error cases, asserting generated input/output
schemas, ReadOnlyHint, StructuredContent, and typed error packing on the wire
rather than only testing the handler or tool listing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7cabe1fc-8958-49d0-b124-4e4fd80f7bac
📒 Files selected for processing (5)
openspec/changes/2026-07-13-bash-mcp-tool-handler/.openspec.yamlopenspec/changes/2026-07-13-bash-mcp-tool-handler/design.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md
🔗 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. (2)
- GitHub Check: build-test-coverage
- 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:
openspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/design.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.mdopenspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md
🔇 Additional comments (8)
openspec/changes/2026-07-13-bash-mcp-tool-handler/.openspec.yaml (1)
1-2: LGTM!openspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.md (1)
1-20: LGTM!Also applies to: 23-35, 39-41
openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md (2)
1-20: LGTM!Also applies to: 28-147, 156-193
25-25: 🚀 Performance & ScalabilityRemove the output-limit concern. The
/execclient already caps successful responses at 10 MB before the tool handles them, so this handler is not exposed to unbounded stdout/stderr.> Likely an incorrect or invalid review comment.openspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.md (3)
1-6: LGTM!Also applies to: 19-26, 45-120
7-18: 🩺 Stability & AvailabilityNo change needed here. Persistence and same-session serialization are defined in the sandbox agent/session-manager specs, so this tool spec doesn’t need to restate them.
> Likely an incorrect or invalid review comment.
27-44: 🔒 Security & PrivacyNo server-side session binding needed here.
X-Session-IDis trusted routing metadata injected by TARSy; access to the MCP endpoint is already gated by kube-rbac-proxy/bearer auth, and this handler does not own a separate session-authorization check.> Likely an incorrect or invalid review comment.openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md (1)
1-13: LGTM!Also applies to: 16-32, 46-54
1 participant
bashMCP tool handler): proposal, design, spec, and tasksX-Session-IDrouting, timeout clamping (default 60 / max 300),BashInput/BashOutput, andCommandExecutorfor testabilityIsError+ structuredBashOutput; infra/validation → toolIsErrorwithoutBashOutput(not JSON-RPC protocol errors)Assisted By: Cursor
Summary by CodeRabbit
bashtool for executing commands within active sessions.