fix(acp): make configured MCP tools callable - #1003
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughACP now creates a scoped workspace for each turn. The workspace carries the registry, sandbox, and cleanup callback. MCP tools register for non-plan modes, while failed initialization and turn completion close resources correctly. ChangesACP workspace lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR makes configured MCP tools callable in ACP and includes the stated validation checks; no actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant Client
participant ACP as runTurn
participant Workspace as buildACPWorkspace
participant MCP as MCP runtime
participant Agent
Client->>ACP: start turn
ACP->>Workspace: create with context and permission mode
Workspace->>MCP: register tools when mode is not plan
Workspace-->>ACP: return scoped Workspace
ACP->>Agent: execute with registry and sandbox
Agent-->>ACP: return result
ACP->>Workspace: close resources
ACP-->>Client: return turn result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make newly configured MCP tools visible in the default ACP mode
internal/cli/acp.go:97
This registers real MCP tools into the per-turn workspace, but a fresh ACP session starts inPermissionModeAuto, whilemcp.newRegistryToolgives an unapproved toolPermissionPromptwithoutAdvertiseInAuto.agent.ToolAdvertisedtherefore filters the tool out before the provider sees it. The same visibility result is used byregisterToolSearchIfEligible, so a fresh set of MCP tools also contributes zero eligible tools and does not produce a discovery loader. The new test misses the production path becausecliFakeDeferredToolreportsPermissionAllow; registry membership consequently passes even though the provider-visible set for a real fresh tool is empty.The root cause is the mismatch between ACP's default mode, the safety metadata assigned to MCP tools, and the visibility-aware loader count—not MCP connection or registration itself. Please align those seams so a newly configured MCP tool is discoverable and callable through the ordinary ACP workflow while its execution still requires the existing low-autonomy permission prompt. Avoid fixing this by converting MCP actions to pre-approved actions, weakening plan mode, or globally changing unrelated surfaces. Add an end-to-end regression using production-equivalent
PermissionPromptMCP safety and assert what the ACP provider actually receives in the default mode. -
[P2] Pass one effective defer threshold through both ACP consumers
internal/acp/agent.go:296
buildACPWorkspacereadsresolved.Tools.DeferThresholdand may registertool_search, but thisagent.Optionsliteral does not pass the same value to the agent. Its zero value reachespartitionToolsCached, where deferral is active only whenoptions.DeferThreshold > 0; the inactive path removestool_searchand eagerly advertises every visible MCP schema. As a result, the loader asserted byTestBuildACPWorkspaceRegistersSandboxedMCPToolsAndClosesThemis never provider-visible, the default threshold of 3 is ignored, and larger MCP installations pay the full schema/context cost on every turn.The root cause is split ownership of a single configuration contract: workspace construction consumes the threshold, while the downstream partition independently receives an unset value. Please derive one effective ACP threshold and carry it through loader registration and
agent.Options, as the sibling exec/TUI paths do. Cover the complete behavior—not only registry membership—by asserting that at/above threshold the provider receivestool_searchwhile eligible schemas remain deferred. Preserve explicit zero-disable, below-threshold eager exposure, and plan-mode suppression; this does not require redesigning generic deferral. -
[P2] Preserve and surface MCP setup diagnostics across the workspace boundary
internal/cli/acp.go:97
The helper returns two non-fatal diagnostic channels:trustSkipexplains when project MCP configuration was excluded by the fail-closed trust gate, andruntime.Skipped()records configured servers that timed out, failed to connect, returned invalid tools, or conflicted by name. This new caller assigns the trust result to_and retains onlyruntime.Close, so both outcomes are discarded. The turn then proceeds with a reduced registry and no explanation; to an ACP user, an untrusted project or failed configured server is indistinguishable from the original “MCP tools are not callable” defect. Other consumers explicitly emit thezero trustguidance and redacted configured-server warnings.The root cause is an incomplete workspace result/ownership contract:
Workspacecarries the registry, sandbox, and cleanup callback, but no setup notices survive long enough for the ACP boundary to report them. Please retain the trust and configured-server skip results and surface them through an ACP-safe, redacted diagnostic path. The implementation can choose the appropriate ACP notification or stderr route, but it must not corrupt stdout JSON-RPC framing. Keep trust fail-closed, keep multi-server startup best-effort, keep hard setup errors as errors, and keep unconfigured default-server failures silent; making optional server failure fatal or bypassing trust would introduce scope and behavioral drift.
2 participants
Fixes #998.
ACP now builds the same scoped MCP-enabled registry as
zero execfor each turn. Configured MCP servers run through the sandbox execution runner, respect workspace trust, use low autonomy, are omitted in plan mode, and are closed after success, failure, or cancellation.tool_searchis registered after the full ACP-visible registry is built.Verification:
make fmt-check,go vet ./...,go test ./...-count=20)go run ./cmd/zero-release buildandsmokemake lint-static,make vulncheck, diff checkThe storage-heavy mutation sweep was intentionally not run under the active storage-safety restriction.
Summary by CodeRabbit
Bug Fixes
Behavior Updates