feat(ai): MCP client — in-app agent consumes external MCP servers (AGENT-3)#145
Merged
Conversation
… (AGENT-3) Add a server-side McpClientManager that pools connections to admin-registered external MCP servers and hands the agent route pre-built, namespaced `mcp__<server>__<tool>` tools it merges like plugin tools (no run-loop changes). Security controls (per resolved decisions): - Q1 trust-level stdio gate: stdio transport allowed only on desktop/unclaimed instances (ownerSubject unset); rejected at registration AND at run time on a claimed multi-user instance; hidden in the settings UI there. HTTP-only there. - Q2 taint: once a run uses any external tool, later document writes are forced back through the review layer even with direct-edit access. - Q3 writer-gated: external tools are offered only to a principal passing the create gate; a guest run gets none. - Q4 per-conversation consent: the first external call pauses via a permission_request (kind:external_tools), sticky once granted. - Q5 off/empty by default; global + per-server enable + OPENBOOK_MCP_CLIENTS=0. Admin-only GET/PUT/test /api/ai/mcp (requireInstanceAdmin), write-only auth tokens (redacted, preserve/replace/clear), untrusted-output label-wrap + clip, per-call timeout riding AGENT-4's recoverable-error path, 3s discovery deadline with per-server cache + failure backoff. Settings section + "external" badge on mcp__* tool events. Unit tests (merge/namespacing/sanitize/redaction/backoff + stdio gate via InMemoryTransport) and agentLoop tests (success, throw/timeout recoverable, consent pause, taint forces review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFk2T9k3p7ghCjdzMfkA5w
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…taint, add-flow (AGENT-3) Address both AGENT-3 review REQUEST-CHANGES on the same branch: HIGH — consent gate was bypassable by a parallel native tool batch: the guard only fired when `interactive` was unset, so a second `mcp__*` call in one batch (after the first set `interactive`) ran and shipped args to the untrusted server before consent. The gate is now INDEPENDENT of `interactive` — no external tool runs until `externalConsent`, period — plus a batch-level `break` once the run pauses. Regression tests cover two parallel externals and request_edit_access + external in one batch. HIGH — "Add server" was broken against a real (HTTP) server: it PUT a blank row (empty id) which `setConfig` rejected (SLUG_RE → 400) so the row never rendered. Add now updates LOCAL state; the per-field onBlur persists once id + endpoint are valid. MAJOR — taint is now conversation-sticky (was per-run): the client re-sends `externalToolsUsed` once it has seen any `mcp__*` tool event, OR'd into the per-run taint at run start, so a later turn that edits without itself calling an external tool still routes through review. Threaded sdk→client→route→runner; test added. MEDIUM — connection pool leak under concurrent runs: `connection()` now shares an in-flight connect promise per server id (the second run no longer opens a duplicate socket/child and orphans the first). MINOR (security) — an `isError` result now gets the same untrusted-output envelope before it throws (its text is attacker-controlled); `errorClass()` returns the error class (+ numeric code) only, never the free-text message (§S6). Local-mode `putMcpConfig` stub now redacts the echoed tokens. Adds route-level tests for GET/PUT/test `/api/ai/mcp` (admin gating, redaction, slug + stdio-on-claimed 400s). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFk2T9k3p7ghCjdzMfkA5w
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
OpenBook's in-app AI agent can now use tools from external, user-registered MCP servers. Board: AGENT-3 (Epic: Agent-first-class), Wave-1 in-repo. Built to the reviewed design (
scratchpad/agent3-design.md) with owner-resolved security decisions.Pieces
packages/server/src/ai/mcpClients.ts—McpClientManager:ai.mcpsettings key with secret-preserving merge + redaction; pooled lazy@modelcontextprotocol/sdkconnections;toolsForRun(deadlineMs)(one 3s deadline viaPromise.allSettled, 5-min tool cache, 60s failure backoff, 40-tool cap, 300-char desc clip);callTool(per-call timeout, untrusted-output envelope + 4000-char clip,isError→throw into AGENT-4's recoverable path);test;dispose.OPENBOOK_MCP_CLIENTS=0hard-disables the subsystem.McpTransport/McpServerConfig/McpClientConfigtypes,aiMcp/aiMcpTestroutes,getMcpConfig/putMcpConfig/testMcpServerclient methods,LocalDataClientstubs.test/api/ai/mcp(requireInstanceAdmin); external tools gathered besidecollectPluginTools, only for writer principals.agent.ts):externalToolsoption,externalToolDefs()(write:false,mcp__<server>__<tool>namespaced, schema pass-through), untrusted-output system-prompt rule, consent pause, taint.settings/McpSettings.tsx(admin-self-gated, off-by-default, stdio hidden when disallowed, token set/replace/clear, per-row Test, warnings) + "external" badge and consent card inAgentPanel.tsx.Security decisions (owner + manager resolved)
store.getInstanceConfig().ownerSubjectis set (same signalrequireInstanceAdminuses).stdioAllowed = ownerSubject === undefined. Enforced in three places —setConfigrejects stdio on a claimed instance,test()refuses stdio dry-run, andtoolsForRunfilters stale stdio at run time (covers a desktop instance later becoming claimed) — plus UI hiding. HTTP-only on claimed multi-user.tainted;propose()forces writes back through review even ifallowDirectEditswas granted.mcp__*call setspermission_request{kind:'external_tools'}and returns before executing (nothing leaves the box pre-consent); sticky per conversation.decideCreateAccess(principal).canWrite— guests get none.enabled:false.resolveKeycontract), redacted in GET/PUT, HTTP→Bearer header / stdio→env var never argv; stdio childstderr:'ignore'.Scope
Wave-1 in-repo only. EXCLUDES the outward/remote surface (AGENT-5/6, owner-gated), OAuth flows, MCP resources/prompts/sampling/roots, stdio sandboxing.
Tests / verify
mcpClients.test.ts(10: secret merge, redaction, slug/dup rejection, stdio trust-level gate incl. runtime skip, discovery+dispatch over an in-processInMemoryTransportserver, untrusted-wrap, isError→throw, backoff) +agentLoop.test.ts(+7: external success, throw/timeout→recoverable, consent pause never-runs, taint→review, control).pnpm verifyexit 0: sdk 188, ui 981, server 681, server e2e 251, mcp e2e 43.