Skip to content

feat(harness): add MCP client - #79

Open
sanchitrk wants to merge 8 commits into
canaryfrom
claude/sleepy-gates-ce83d0
Open

sanchitrk wants to merge 8 commits into
canaryfrom
claude/sleepy-gates-ce83d0

Conversation

@sanchitrk

@sanchitrk sanchitrk commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds the low-level MCP client the MCP service will build on: one scoped connection to one server, over stdio or Streamable HTTP, through the official SDK (@modelcontextprotocol/client 2.3.1).

  • src/mcp/client.ts: McpClient.connect(...) returns a Connection with tools(), resources(cursor?), resourceTemplates(cursor?), readResource(uri), callTool(...) and onToolsChanged / onResourcesChanged / onSessionExpired / onClose callbacks.

    • Timeouts: 30 s to start, 60 s per request, restarted by progress.
    • Retries: HTTP connect and list/read retry transient failures; tool calls never retry.
    • Errors: HTTP session expiry becomes SessionExpiredError, auth failures become NeedsAuthError.
    • Closing the scope ends the HTTP session and never fires onClose.
  • src/mcp/stdio.ts: stdio transport. Servers run on the host in their own process group.

    • A crash is reported with the exit code and the stderr tail.
    • Closing ends stdin, waits 2 s, then SIGTERM and SIGKILL to the group.
    • An exit hook stops leftover servers if the host exits without closing them.
  • src/mcp/content.ts: tool results as model text and image content. src/mcp/error.ts: tagged errors.

  • SDK validation: list pages and tool results (against a tool's outputSchema) are validated by the SDK.

  • Process.terminate: the process-group stop sequence moves out of the host shell into sandbox/utils/process.ts, shared by the host shell and the stdio transport.

  • Server logging: onLog receives the server's notifications/message entries, and src/mcp/log.ts (McpLog) appends them one line per entry, rotating the file past 5 MB.

  • Readable validation errors: the SDK's multi-line issue list becomes one line, Invalid result for <method>: <path>: <message>; the raw list is logged at debug level.

  • Tracing: entry points use Effect.fn (McpClient.connect, McpStdio.make, McpLog.make), and each request gets a span named McpClient.<method>.

No user-facing surface yet: config, the MCP service, scoping and tool wiring come in follow-up PRs stacked on this one.

Review fixes (2026-10-24)

Rebased onto canary (effect 4.0.5, harness 0.5.0); the lockfile now only adds the MCP SDK.

  • Retries: connects retry on HTTP 408/429/5xx and on fetch failures with a system code only. Any other TypeError, such as an invalid header value, fails once instead of being retried and logged. Lists and reads retry once, 250 ms later, after a transient HTTP error.
  • Startup timeout: bounded by the request timeout (60 s by default) unless set on its own, instead of a fixed 30 s.
  • Progress: only requests given a progress handler (tool calls) ask for progress.
  • Shutdown: a stdio server's process group gets SIGTERM even after the server exits on its own, and SIGTERM before SIGKILL when the leader is gone but a pipe is still open. The host shell keeps its immediate kill.
  • Server logs from the start: the log handler is a connect option, so messages sent during the handshake, or by a server that then fails, reach mcp.log.
  • List changes before the handler: a tools/list_changed sent while the first listing is answered is reported once the handler is set, instead of being lost.

Test plan

  • test/mcp.client.test.ts: 29 scenarios against real stdio and Streamable HTTP fixture servers.
    • Process behaviour: crash mid-call, process-group cleanup, a server that ignores SIGTERM.
    • Protocol: message framing and the 16 MiB limit, progress keeping a call alive, cancellation.
    • HTTP: transient retry, connection refused, session expiry, 401 with and without an auth provider.
    • Data: roots, resources and paging, output-schema validation, malformed list pages, server logging and log rotation.
    • Error messages: a missing command or working directory.
    • Ends with __artifacts__/mcp.client.json, stable across runs.
  • Sandbox tests still pass after the Process.terminate extraction
  • vp check and vp run typecheck
  • Node SEA bundle with the SDK (checked once the follow-up PRs land)

🤖 Generated with Claude Code

@sanchitrk

sanchitrk commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Review (2026-10-24)

Stack status

should-fix: a resolved header value can leak a secret

  • Where: mcp/value.ts resolves ${VAR} / !command header values without checking them.
  • What happens: if a value contains CR, LF or NUL, new Headers(...) throws a TypeError whose message is the whole value. I reproduced it on Node: Headers.append: "Bearer sk-secret\nx" is an invalid header value.
  • Why it spreads: isTransient treats any TypeError as transient, so the message is:
  • Realistic trigger: a !token-cmd that prints a notice line after the token.
  • Fix:
    • In resolve, reject header values matching /[\r\n\0]/ with a message that names the header but not the value.
    • Narrow isTransient to fetch network failures (a TypeError with cause.code).

nit: read retries

  • Reads and lists retry twice (250 ms, then 1 s), on any TypeError.
  • The reference retries reads once, after 250 ms, and only on transient HTTP and network errors. Connect retries already match it.
  • Match it, or note why we differ.

sanchitrk and others added 6 commits October 24, 2026 19:09
Connect to one MCP server over stdio or Streamable HTTP through the
official SDK. Stdio servers run on the host in their own process group
and are stopped with the group; HTTP connects retry transient failures.
List pages and tool results are validated by the SDK.

The process-group stop sequence moves out of the host shell into a
shared Process.terminate helper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Connections pass the server's notifications/message entries to an
onLog handler, and McpLog appends them to a file with one line per
entry, rotating it past 5 MB.

A result that fails the SDK's schema is reported on one line as
`Invalid result for <method>: <path>: <message>` instead of the
validator's multi-line issue list, which is still logged at debug level.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Name the MCP entry points with Effect.fn and give each request its own
span. A server without resource templates is now recovered from inside
the Effect rather than on the SDK promise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Count stderr as an open pipe, so a descendant holding only stderr is
  still killed with the server's process group.
- Bound the whole connect attempt by the startup timeout; the SDK only
  times the initialize request, not the initialized notification.
- End the HTTP session a failed connect attempt was given, through a
  fresh transport, since the SDK aborts the failed transport's signal.

Tests now cover these, retries of reads but not tool calls, session
termination and 400 classification, and register their own teardown
for spawned processes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The process spawner kills the group when the connection's scope closes,
which hid a missing kill. Assert instead that a grandchild holding only
stderr is gone once the connection reports the server's exit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…shutdown

- Retry connects only on HTTP 408/429/5xx and fetch failures with a system code;
  any other TypeError, such as an invalid header value, fails once.
- Retry list and read requests once, 250 ms later, after a transient HTTP error.
- Bound startup by the request timeout unless set on its own.
- Ask for progress only on requests given a progress handler.
- SIGTERM a stdio server's process group even after the server exits on its own,
  and before SIGKILL when the leader is gone but a pipe is still open.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sanchitrk
sanchitrk force-pushed the claude/sleepy-gates-ce83d0 branch from 84e21d0 to f64f0c1 Compare October 24, 2026 14:19
sanchitrk and others added 2 commits October 24, 2026 21:02
The log handler is now a connect option instead of a setter called after the first tool
listing, so messages a server sends while starting, or before failing to, are kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A tools/list_changed sent while the first listing is answered used to reach no handler,
leaving the old list in place; it is now reported once the handler is set.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant