Repository navigation
Connect external MCP servers over HTTP/SSE, with per-tool policy (#17) - #100
Merged
Merged
Conversation
…olicy (#17) The engine can now be handed the operator's connectors (HTTP/SSE MCP servers) and decides each of their calls against the connector's own configuration row, never against a name the server chose. - mcp-connectors.ts (pure): the connector name rule (lower-case, digits, single hyphens, max 32, agentstudio and the CLI's own server names reserved), the run's connector map with per-tool allow/ask/block keyed by the CLI's spelling of the tool name, connectorCallVerdict, and composeMcpServers (connectors first, ours last so no key can shadow it; none for an agent with a fixed tool list). - A call is refused unless its <server> is one of the run's rows and the SDK's provenance agrees (source 'dynamic', same key); with no provenance reported, allow degrades to ask. Our own server name from any source but 'sdk' is refused. - resolveToolGate takes the policy: block denies in every mode, allow runs unasked in default and acceptEdits, plan still refuses, and the per-tool settings list is not consulted for external tools. - canUseTool and the PreToolUse hook pass the SDK's mcpServer / mcp_server provenance through decideToolCall. - Options: strictMcpConfig so only servers AgentStudio passes load (no .mcp.json, user, plugin or claude.ai connectors); blocked tools go in disallowedTools; the CLI's generic MCP resource readers are refused. - sdk-notices: system/init reporting a configured server failed or needs-auth becomes one mcp_unavailable notice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ection test (#17) Operators can add remote MCP servers over Streamable HTTP or SSE and chat runs consume them. - mcp_servers (migration 0079): one row per connector per user with an immutable validated name, label, transport, URL, header names, a has-bearer-token flag, the bearer token and header values as one document encrypted with APP_ENCRYPTION_KEY, per-tool policies, the last test's tool list and outcome, enabled, timeout. Audit actions mcp_server.created/updated/deleted record names, never values. - mcp-config.ts (pure): URL rule (http/https, no credentials in the URL, public hosts only by the egress policy unless listed in MCP_ALLOWED_PRIVATE_HOSTS), header and token rules, the blank-keeps / null-removes secrets patch, and the SDK config. - mcp.server.ts: CRUD scoped to the caller, a list that never selects the secrets, and loadRunMcpServers, which gives only interactive chat runs whose agent has no fixed tool list the enabled rows, re-checks each URL and resolves its host under the egress guard, and names any connector it leaves out in a notice. - mcp-probe.server.ts: the Test button's own MCP client (the SDK's Query handle lives for one turn), connect and list tools with a 10 s deadline, every request through a new guarded fetch (egress-fetch.server.ts: spelling check, per-socket address check, no redirects, capped body, streamed for SSE). - /settings/connectors: add, edit, test, enable/disable, remove, and Allow/Ask/Block per tool; linked from Settings. - The chat stream route loads the connectors per run and passes them to the options and the gate. Stdio servers are not a transport: they would run outside the Bash sandbox with the CLI's environment (see docs/mcp/mcp.md). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onnector (#17) getFriendlyToolLabel names an mcp__<server>__<tool> call by the tool's own name plus " · <server>" in every status, and never borrows the copy written for one of our tools. The per-call ledger entry carries provider 'mcp:<server>' so usage can be grouped by connector; ours and the built-ins carry none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
system/init naming a configured server as failed or needs-auth gives one kept warning; pending, all-connected and our own sdk server say nothing; a server name is reduced to safe characters and clipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tests/mcp-fixture.ts is a hand-written MCP server speaking Streamable
HTTP and the older SSE transport, plus a redirect, a server that never
answers and one that echoes credentials in a 500. mcp.probe.spec.ts
drives the Test button's client against it through the guarded fetch:
both transports list every page of tools and send the saved headers;
401s say whether credentials were missing or refused; loopback, a name
resolving to a private address and redirects are refused before the
server sees a request unless that exact host is allowed; a stuck server
costs the deadline; the body cap holds.
The probe now reads its own deadline and the MCP client's per-request
timeout the same way ("No answer within 1 second."), and redacts a
bearer token on its own as well as the whole header value.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…last test (#17) The edit form sends a blank token and blank header values for everything it did not touch. updateMcpServer treated any bearerToken key as touching the secrets, so relabelling a connector re-encrypted them and wiped its test result. Now only a patch that asks for something (null, or a non-blank value) opens the stored secrets, and only a real difference re-seals them and clears the result. Specs, against the live database and through the page: - mcp.server.spec.ts: secrets encrypted and absent from the list and the audit trail; name, URL and uniqueness rules on the server; no APP_ENCRYPTION_KEY refuses a token but not an open server; blank keeps, null removes, a value replaces; enable/disable/policy/delete audited; another user's id reaches nothing; the Test button records its outcome and keeps the last tool list on failure; run start gives only interactive unscoped chat runs the enabled rows, decrypted, with blocked tools, and leaves out one whose host resolves privately. - crud/mcp.crud.spec.ts (desktop and mobile): add with a token and a header, switch off, a failing test, edit keeping secrets, Allow and Block per tool, remove; Settings links to the page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tor's allowed tool (#17) The switch promises that every tool call pauses. A connector's Allow used to override it because the gate ignored the settings for external tools. The settings still never loosen a connector tool, but for an external name the settings callback can only be true under the wildcard (nobody can tick a connector's tool on the per-tool list), so Allow now asks when it is on, with a reason naming the switch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…shipped - docs/mcp/mcp.md (new): what a connector is, adding/testing/policies, what happens in a chat turn, the approval table by mode, integrations, business rules, security notes, what is not supported yet (stdio, OAuth) and why, and what was checked in the Agent SDK 0.3.278 typings, sdk.mjs and the bundled CLI. - docs/mcp/spec.md: rewritten from the unbuilt pre-SDK design to the table, modules, run-start and gate contracts, and the specs. - README: Connectors feature, APP_ENCRYPTION_KEY's second use, MCP_ALLOWED_PRIVATE_HOSTS, the docs link and /settings/connectors. - .env.example: MCP_ALLOWED_PRIVATE_HOSTS. - tools.md and settings spec: connectors' tools and the new page. - claude-desktop-parity.md: the MCP row is "behind" with v1 described; parity-reaudit-2026-09-22.md: #17 shipped (v1), with what is left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… turn (#17) The "connector unavailable" notice read only system/init. The bundled CLI (0.3.278) starts MCP servers without blocking the turn (MCP_CONNECTION_NONBLOCKING !== false) and alwaysLoad is never set, so a remote connector is almost always still `pending` there; one that then failed (401, refused connection, wrong path) was never reported. mcp-status-watch.ts (new, pure apart from the injected session call and clock) asks Query.mcpServerStatus() again once the turn is under way: on assistant, user, stream_event and tool_progress messages, never on init or on the result (the SDK closes the CLI's stdin when the first result arrives). It asks only while one of the run's connectors is unsettled, at least 2 s apart, at most 15 times, each ask abandoned after 1 s. A connector newly failed or needing a sign-in gets one kept mcp_unavailable warning; each is reported once per turn, init's report included, and only the run's own connector keys count. sdk-notices.ts exports the shared pieces (mcpServersDown, mcpUnavailableNotice); init's notice is unchanged. stream.server.ts gains the optional mcpServerStatus on EngineQuerySource and one observe() call in the loop; runs with no connectors, or a test double without the method, never ask. Specs: engine.mcp-status-watch.spec.ts pins when it asks, spacing, the cap, report-once, only-our-connectors, and a failing or silent ask; engine.connectors-stream.spec.ts drives it through runEngineStream (pending in init, failed on the ask -> one kept notice; no connectors -> no ask). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…17) The bundled CLI special-cases two more server names by exact match: computer-use (its first-party and telemetry checks) and remote-devices (beside ide in its set of built-in servers, with a tool list of its own). Both passed connectorNameProblem, so a connector could be registered under a name the CLI treats as its own. Specs: mcp.config.spec.ts refuses both and keeps longer names that only start the same way; engine.connectors.spec.ts shows a row under either name never reaches the SDK options or the run's connector map. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… fails (#17) The switch was one-way (checked={connector.enabled} with onchange). When setMcpServerEnabledCommand failed, `connectors` did not change, so Svelte never reset the checkbox: it stayed flipped while the On/Off text beside it and the database said the opposite. toggle() now puts the input back to connector.enabled before the error is shown. Spec: mcp.crud.spec.ts routes the command to a scripted 400, clicks the switch, and checks the alert, an unchecked switch, "Off", and an unchanged row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) Two edge cases from review: - toolSnapshot clipped a tool name over 128 characters to 128 plus an ellipsis. The clipped name matched no call, and it failed the policy command's max(128) key check, so "Every tool" (and any change to that tool) could not be saved for the connector. A tool with no name or a name over 128 characters is now left out of the list; it asks, like any tool without a policy. - The edit form sends new headers before the stored rows' removals, so removing "X-Team" and adding "x-team" deleted the header just added. applySecretsPatch now applies removals first, then values. Specs: mcp.probe.spec.ts (128 kept, 129 and blank dropped) and mcp.config.spec.ts (the replacement holds in either order). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ew role (#17) - docs/mcp/mcp.md: a chat turn's step 4 and "Live status" now describe what happens: connectors connect in the background, are usually still connecting at the start, and are re-checked while the turn runs; one that fails after the last step goes unreported. Reserved names gain computer-use and remote-devices. New security note: the engine's own connection may follow redirects. The long-tool-name rule. What was checked in the SDK: MCP_CONNECTION_NONBLOCKING, mcpServerStatus() and when stdin closes, the two exact-name servers. - docs/mcp/spec.md: the two new modules, the watch's contract under Notices, the snapshot rule, and the specs table. - docs/runtime/spec.md: the protected-files row for .mcp.json said it decides which tool servers connect; since strictMcpConfig the engine ignores it (still protected). The unbuilt MCP lifecycle design now points at the Connectors doc. - parity-reaudit-2026-09-22.md: #17's notice line and what is left open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings the wave-3 chain (#14, #9, #22, #24, #4, #35, #32, #18 and main) under #17's connectors so the PRs merge in order. Migrations: - drizzle/: took #18's migrations, snapshots and _journal.json verbatim (0079_conversation_lifecycle), deleted this branch's 0079_mcp_servers.sql and its 0079 snapshot, kept the mcp_servers schema .ts, and let `bun run db:generate --name mcp_servers` emit 0080_mcp_servers.sql with 0080_snapshot.json chained on #18's 0079 (SQL identical to the old 0079). `bun run db:check` passes. - docs/mcp/spec.md, docs/structure/parity-reaudit-2026-09-22.md: the connectors table is migration 0080. Engine: - tool-decision.ts: both gates kept. Order is scope, then #17's own-server provenance check and connector verdict (unknown server / mismatched provenance refused), then #4's AskUserQuestion exemption, then containment and resolveToolGate with the connector's per-tool policy. The provenance checks sit before the exemption, which is keyed on a bare name a spoofed own-server call would share. - stream.server.ts: imports from both sides (watchConnectorStatus beside background shells, AskUserQuestion, UsageCoverage, turn-input). The PreToolUse hook is #32's (delegation admission, updatedInput) with #17's `mcp_server` provenance passed to decide(); canUseTool destructures both #4's `agentID` and #17's `mcpServer`. - options.server.ts: #9's imports (no `$env` import, gateway env) with #17's tools.server import (no ENGINE_MCP_SERVER); composeMcpServers, strictMcpConfig and the MCP resource-tool disallow list sit beside #4's toolConfig/settings and #32's disallowed Workflow/SendMessage. - builtin-tools.ts, permission-mode.ts, sdk-notices.ts, chat.ts and the stream route merged cleanly; the route loads connectors once per turn and hands them to every #24 resume-fallback attempt. Docs: - tools.md: #4's MCP endpoint wording (no ask_user) plus #17's Connectors bullet. - claude-desktop-parity.md: wave-4 line keeps #17 struck through (v1) and #18/#22's landed notes. - parity-reaudit-2026-09-22.md: #17 shipped (v1) and #32 shipped rows; the order list keeps #17 done and #4/#18/#14 shipped. - runtime/spec.md: the chat tool check lists connectors as its fourth check and says AskUserQuestion skips all but the tool-list check. - mcp/spec.md: notes the provenance checks run before the AskUserQuestion exemption. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
derekhearst
changed the base branch from
feat/18-conversation-lifecycle
to
main
September 24, 2026 07:03
The spec clicked "Add connector" as soon as the server-rendered heading showed, before the page hydrated, so the click had no handler and the form never opened — every time on CI's slower runners. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n specs The stub records every OpenRouter request in the Playwright worker, and the in-process job worker can run another spec's job meanwhile — CI caught an evaluator call landing in "nothing reached the extractor or the embeddings". The assertion now filters to calls carrying the turn under test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Summary
Picked up the interrupted attempt and finished #17 with triage options A and B. Chat runs can now use remote MCP servers over HTTP or SSE. The new mcp_servers table (migration 0079) stores the bearer token and header values as one document encrypted with APP_ENCRYPTION_KEY. A new Settings → Connectors page (/settings/connectors) adds, edits, tests, enables, disables and removes servers, and sets each tool to Allow, Ask or Block. Enabled rows are loaded only for interactive chat runs whose agent has no fixed tool list. They are merged into mcpServers beside our own server, which is placed last so nothing can shadow it. strictMcpConfig is on for every run, blocked tools go into disallowedTools, and the CLI's generic MCP resource readers are disallowed. The tool gate trusts the configuration row, not a name the server chooses. A call is refused unless its server is one of the run's rows and the SDK's provenance agrees (source 'dynamic', same key). A call using our own server name from any source other than 'sdk' is refused, and Allow becomes Ask when there is no provenance. Block refuses in every mode; Allow runs in Ask and Accept-edits unless "Require approval for all tools" is on; Plan refuses every connector tool. The Test button uses its own MCP client and sends every request through a new guarded fetch (egress spelling check, per-socket address check, no redirects, capped body); run start also resolves each connector's host under the egress rule. Also added: tool cards labelled "Tool · connector", ledger rows with provider 'mcp:', and an mcp_unavailable notice when system/init reports a connector failed or needs-auth. Two bugs fixed along the way: a label-only edit re-sealed the secrets and wiped the test result, and a connector's Allow overrode "Require approval for all tools".
After review: Picked up the review on top of the 8 existing #17 commits. There were no must-fix items; all 4 should_fix items are fixed, plus 2 of the minor edge cases from the notes and one of the residual-risk notes.
system/init, where remote connectors are almost always stillpending. A new pure module,src/lib/engine/mcp-status-watch.ts, asks the session again (Query.mcpServerStatus()) once the turn is under way. It asks on the model's messages, stream events and tool results, never on init or on the result, because the CLI's stdin is closed by the time the result arrives. It keeps asking only while a connector is unsettled: at least 2 s apart, at most 15 times, each ask abandoned after 1 s. It reports each connector once and only this run's own connectors. The hot filestream.server.tsgets just an import, an optional type member and oneobserve()call.computer-useandremote-devicesare now refused as connector names. I checked both in the bundled CLI..mcp.jsonrow in docs/runtime/spec.md now says the engine ignores the file sincestrictMcpConfig(it is still protected). docs/mcp/mcp.md step 4 and "Live status" now describe the re-check honestly, including that a failure after the turn's last step goes unreported.Also fixed from the notes:
bun run checkshows 0 errors,bun run buildpasses andbun run db:checkis fine. The worktree is clean.Closes #17
What was addressed
Audit findings (from the 2026-09-22 subsystem audit) were re-checked against the code before any change; "not a bug" means the claim did not hold up.
Verification
bun run check: 0 errorsbun run build: passesbun run db:checkpasses. Instead, the pure specs were run underbun testwith a scratchpad preload that maps @playwright/test to bun:test (no DB, no server): - tests/engine.mcp-status-watch.spec.ts, tests/engine.sdk-notices.spec.ts, tests/mcp.config.spec.ts, tests/mcp.probe.spec.ts and tests/engine.connectors.spec.ts: 110 passed, 0 failed. - tests/engine.connectors-stream.spec.ts, with a stub for the registry module's bareToolName, copied from the real one: 9 passed, 0 failed. With the stream.server.ts change stashed, the new 'kept warning' test fails, as expected. - tests/crud/mcp.crud.spec.ts (the switch spring-back step) has not been run; CI will run it on both projects.Docs updated
Follow-ups / residual risk
alwaysLoadon connectors, which blocks the turn start until they connect; it was deliberately not chosen.🤖 Generated with Claude Code