feat(mcp): add @openrouter/mcp package#56
Conversation
Expose remote MCP server tools (Streamable HTTP / SSE) as callModel tools, with pluggable auth, faithful JSON-Schema->Zod conversion, and a serializable, rehydratable cache.
![perry-the-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/3222961?s=60&v=4){kind=small}
There was a problem hiding this comment.
Perry's Review
New @openrouter/mcp package that exposes a remote (Streamable HTTP / SSE) MCP server's tools as @openrouter/agent callModel tools, with JSON-Schema→Zod conversion, a serializable/rehydratable cache, and pluggable auth. The core wrapping, schema conversion, and result-mapping are solid and well-tested — but the cache rehydration path has three correctness blockers that defeat the headline caching feature.
Verdict: 🔁 Needs changes
Details
Risk: 🔴 High — human approval required (touches credential handling/persistence — Step 6c categories 1 & 7 — and the confirmed bugs are squarely in that auth/cache path)
CI:
Findings (see inline comments for full context):
- 🔴
rehydrate.ts:108— expiry/missing-cred fallback re-enters the same cache → unbounded recursion (fresh-by-maxAge + token-expired snapshot) - 🔴
rehydrate.ts:112— snapshot auth never replayed into the reconnect; cacheCredentials snapshots reconnect unauthenticated (contradicts README) - 🔴
create-mcp-tools.ts:113— cache-hit rehydration drops toolNamePrefix/includeTools/excludeTools/resources/emitProgress/staleness/… → cached handle ≠ fresh handle - 🟡
create-mcp-tools.ts:28— listTools not paginated; paginated servers expose only page 1 - 🟡
resource-tools.ts:35— list_resources not paginated; resources and templates silently truncated - 🟡
create-mcp-tools.ts:84— connection leaked if initial listTools/writeCache throws - 🟡
create-mcp-tools.ts:221— list_changed refresh is fire-and-forget; rejection is unhandled - nit:
isOAuthAuth(auth-types.ts:26) is exported but unused anywhere in the package — dead code.
Codex (HEAVY_SECONDARY_MODEL): independently surfaced all three blockers (recursion, dropped cached creds, dropped caller options) plus the two pagination gaps, the connection leak, and the unhandled refresh rejection. All verified against source before posting.
Research: MCP TypeScript SDK — confirmed the tools-list, resources-list, and resource-templates-list endpoints are cursor-paginated (nextCursor, loop until absent); confirmed StreamableHTTPClientTransport replays static creds via requestInit headers and OAuth via authProvider, so the snapshot→reconnect credential gap at rehydrate.ts:112 is a real omission, not an SDK limitation.
Security: Credential handling reviewed (cat. 1) — cacheCredentials defaults to false, the cache-store JSDoc + README correctly warn to treat the store as a secret store and namespace by principal, and no secrets are committed. No exploitable issue found, but the auth-replay bug (rehydrate.ts:112) means the secure feature is currently non-functional rather than unsafe.
Test coverage: Strong unit coverage for schema conversion, result mapping, build-tools, elicitation, and serialize. Gap: the rehydration paths (rehydrate.ts, tryCacheHit) and the mcp-connection fallback have no unit tests — exactly the code where all three blockers live. The e2e tests are skipped without a live MCP_TEST_URL. Adding rehydrate unit tests with a seeded InMemoryMCPCacheStore (expired-token + cacheCredentials cases) would have caught all three.
Unresolved threads: none (first review)
Scope: first review (full)
Review: tier=large · model=claude-opus-latest · score=?
…pagination - freshConnect breaks the recursive cache-fallback loop (no-auth/expired snapshots no longer re-read their own cache and re-enter rehydrate) - reconstruct MCPAuth from a credential-bearing snapshot so reconnects authenticate when no explicit auth is passed - thread toolNamePrefix/include/exclude/resources/emitProgress/ autoRefresh/cacheCredentials through cache hits so filters and prefixes survive rehydration - follow nextCursor in tools/list, resources/list, and templates/list
… thread clientInfo Addresses remaining Perry review threads: - freshConnect tears down the connection if listToolDefs or the initial writeCache throws, so a failed setup never leaks the open transport - the tools/list_changed auto-refresh now .catch()es, so a failed refresh cannot escape as an unhandled rejection; listeners keep the last good set - thread clientInfo through the cache-hit/rehydrate path so a warm handle initializes with the same client identity as a cold one
The sentrux structural gate failed on cycle + quality + complexity regressions. Resolved without weakening the baseline: - extract makeHandle/freshConnect/listToolDefs into a leaf handle.ts so create-mcp-tools and rehydrate no longer form a cycle - move MCPTransportKind to a leaf transport-types.ts, breaking the types <-> cache-types cycle - import Tool/tool from @openrouter/agent's deep subpaths (/tool, /tool-types) instead of the barrel, so mcp doesn't absorb the agent lib SCC - drop the unused ExecuteToolsResult interface in agent's tool-types.ts, which removes the tool-types -> model-result edge and breaks agent's pre-existing lib cycle (this is what mcp was being dragged into) - collapse tryCacheHit's option forwarding into a data-driven helper to drop it back under the max-cyclomatic-complexity threshold Gate now: cycles 1->1, quality 0.51->0.52, no degradation.
Extract the buildTools-args and serializeServer-input assembly out of makeHandle into buildToolsArgs/serializeArgs helpers, dropping makeHandle from cc=17 to under the gate's threshold of 15. No behavior change.
perry-the-pr-reviewer
Bot
dismissed
their stale review
Superseded by updated Perry review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A poisoned snapshot with NaN/Infinity cachedAt passed isSerializedMCPServer (typeof NaN === 'number'), making tryCacheHit's staleness comparison always false so the entry never expired. Mirror serialize.ts's isExpiry guard with a Number.isFinite check. Also document that includeTools/excludeTools apply to discovered MCP tools only; resource tools are gated by the resources option.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ializer Extract the 'valid cachedAt' predicate into a single exported isFiniteEpoch in cache-types.ts, consumed by both the read-side isSerializedMCPServer validator and the write-side serializer, so the two can't drift if the rule is later tightened. No behavior change.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
An epoch (ms) is non-negative by definition; rejecting negatives validates the invariant at the snapshot boundary instead of relying on downstream maxAge arithmetic to fail-safe on a clock-skewed/garbage value.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
isFiniteEpoch's value>=0 tightening means serializeServer now substitutes Date.now() for a negative cachedAt; assert that branch produces a valid, non-negative epoch that passes the read validator.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…'t untype client tools Adds a `source: 'client' | 'mcp'` discriminant to ToolExecutionResult and the streaming ToolResultEvent. An MCP tool's `unknown` output no longer collapses the whole result union — narrowing on source keeps client-tool results precise and isolates MCP results as unknown. - agent: McpBranded type, isMcpTool guard, markMcp helper; source threaded through ToolExecutionResult(Union), the broadcaster channels, and tool.result events; source stamped at every result-construction site (executor + orchestrator). - mcp: wrapMcpTool and buildResourceTools brand their tools via markMcp. - ToolExecutionResult.toolName now infers the literal name where available. - New mcp-result-discrimination.test-d.ts locks the guarantee. Breaking: the tool.result streaming payload now includes `source`. Verified: full monorepo build/typecheck/lint/test green (295 agent + 39 mcp tests, incl. type-level tests); sentrux structural gate passes (cycles 1->1, quality up, no complex-function regression).
Tighten the fallback test to pin Date.now() substitution (before <= cachedAt <= now) rather than mere non-negativity, catching a regression to a hard-coded sentinel.
There was a problem hiding this comment.
Perry's Review
Incremental re-review: this push adds a source discriminant (client vs mcp) to tool results — plus markMcp(), isMcpTool() and the McpBranded type — so an untyped MCP tool no longer collapses the precise result types of fully-typed client tools.
Verdict: 💬 Comments / questions
Details
Risk: 🟢 Low
CI: all passing ✅ (typecheck, lint, unit-tests, e2e-tests, structural-gate)
Findings (see inline comment):
- 🟡 tool-types.ts:844 — the streaming tool-result event gains the source field but its result stays flat, so the streaming surface does not get the type-level discrimination the non-streaming result path got.
What I verified (incremental scope vs prior reviewed SHA 38bf22d):
- The brand-first ordering in the non-streaming result type is correct — the MCP brand is tested before the execute/generator branch, isolating MCP unknown under source mcp while typed tools keep precise results under source client. Confirmed by tsc --noEmit (exit 0) and the new type-level test mcp-result-discrimination (vitest --typecheck: 0 type errors).
- Blast radius: every tool-execution-result construction site got the new required source field — all 7 executor returns (regular/generator/HITL, success + catch), both orchestrator sites, and the model-result toolResults mapper + broadcastToolResult. A missing site would fail typecheck; CI typecheck is green.
- Runtime / type-level consistency: markMcp shallow-copies preserving the client-tool structure, and isClientTool (not server-tool) still matches MCP-branded tools, so the find lookups in model-result resolve MCP tools. The orchestrator hardcoded client at line 95 is the tool-not-found path (no tool object to inspect — correct default); the rejected path at line 153 correctly resolves via isMcpTool(rejectedTool).
- The cache test negative-cachedAt assertion tightening verified (7/7 pass) — it now asserts the fallback is a fresh Date.now(), catching a regression to a hard-coded sentinel.
Codex (gpt-5.5): 1 in-delta finding surfaced (the streaming-event parity gap, posted inline). Its other findings target rehydrate / create-mcp-tools / mcp-connection — outside this incremental delta (prior-reviewed PR content), several already resolved in earlier rounds.
Research: Confirmed against TypeScript narrowing semantics (microsoft/TypeScript issue 55425; the TS narrowing handbook) — an unknown or imprecise union member poisons sibling members, and the tool() factory widens the name literal to string, so a literal source discriminant (not toolName) is the correct design. The approach is sound.
Security: no concerns — type-system and tool-result plumbing only; no secrets, auth, SSE, logging, or rendering touched in the delta.
Test coverage: the new type-level test covers the non-streaming result discrimination well. The streaming-event discrimination is not type-tested (see inline).
Unresolved threads: none — all 13 prior threads (Perry's own, from earlier rounds) are resolved and fixed by the author.
Scope: incremental — 353 new lines since prior review (38bf22d)
Review: tier=large · model=claude-opus-latest · score=?
| * consumers discriminate so an MCP result's `unknown` doesn't force callers to | ||
| * treat every tool result as untyped. | ||
| */ | ||
| source: 'client' | 'mcp'; |
There was a problem hiding this comment.
[suggestion][codex] streaming ToolResultEvent gains source but its result: TResult is not discriminated by it — narrowing a streamed tool.result on source === 'client' does NOT recover precise client result types the way the non-streaming ToolExecutionResult does.
Details
Why: ToolExecutionResult['result'] was upgraded to a conditional that is brand-first (MCP → unknown, else zodInfer<O>), so a mixed tuple narrowed on source === 'client' yields the precise union of typed results. ToolResultEvent (line 835) instead keeps a single flat result: TResult independent of source. On the streaming surface TResult is instantiated as InferToolOutputsUnion<TTools> (see getFullResponsesStream / ResponseStreamEvent), which already collapses to unknown the moment any MCP tool is present in the array — exactly the poisoning this PR set out to prevent for the non-streaming path. So a consumer of getFullResponsesStream() / getToolStream() that narrows a tool.result event on source === 'client' still sees result: unknown.
This is a parity gap, not a correctness bug: source is genuinely useful for runtime discrimination on the stream (and the changeset documents it as such). But the changeset's framing — that ToolResultEvent "gains the same source field" — implies callers get the same type-level recovery, which they do not.
Fix: make the event's result conditional on source, mirroring ToolExecutionResult — e.g. split into a { source: 'client'; result: <typed union> } | { source: 'mcp'; result: unknown } discriminated shape, and thread the per-tool result type through ResponseStreamEvent so the client arm carries InferToolOutput of the non-MCP tools rather than the whole collapsed union. If type-level recovery on the streaming path is explicitly out of scope, consider a one-line note in the changeset clarifying that source on the event is runtime-only.
Prompt for agents
In packages/agent/src/lib/tool-types.ts, ToolResultEvent (around line 835) added a `source: 'client' | 'mcp'` field but keeps a single flat `result: TResult`, so streaming consumers that narrow on source === 'client' still get `result: unknown` (TResult is instantiated as InferToolOutputsUnion<TTools>, which collapses to unknown when any MCP tool is present). Mirror the ToolExecutionResult fix: change ToolResultEvent into a discriminated union on `source` — `{ type:'tool.result'; toolCallId; source:'client'; result:<precise client result union>; timestamp; preliminaryResults? } | { ...; source:'mcp'; result: unknown }` — and thread the per-tool result type through ResponseStreamEvent / getFullResponsesStream / getToolStream so the client arm carries the typed union and the mcp arm carries unknown. Add a type-level test in tests/unit asserting that for a mixed [typedTool, mcpTool] array, a tool.result event narrowed on source==='client' has a precise result type, not unknown. If type-level recovery on the streaming path is intentionally out of scope, instead add a sentence to .changeset/mcp-result-discrimination.md clarifying that source on ToolResultEvent is for runtime discrimination only.
Reviewed at d3539ee
2 participants
Summary
Adds a new
@openrouter/mcppackage that turns a remote MCP server (non-stdio: Streamable HTTP or SSE) into tools consumable by@openrouter/agent'scallModel({ tools }).createMCPTools({ url, auth })connects, authenticates once, discovers tools, and returns a handle whose.toolsdrop straight intocallModel. The same auth is reused for discovery and every tool call. Supports static bearer token, custom headers, or a pluggableOAuthClientProvider.convertMcpInputSchema) via the built-inz.fromJSONSchema, with a per-propertylooseLeaffallback for unrepresentable keywords. This is required, not optional: the agent SDK derives the model-facing parameters solely from the tool's ZodinputSchema, so a passthrough schema would make the model call tools blind.serialize()/rehydrateMCPTools()/ pluggableMCPCacheStore+InMemoryMCPCacheStore. The happy path skipslistTools()and (opt-in) re-auth; falls back to a full reconnect on token expiry / missing creds / connection failure. Credential caching is off by default.tools/list_changedauto-refresh, cancellation via an abort signal, resources exposed as syntheticlist_resources/read_resourcetools, and elicitation with an optional handler (auto-declines when none provided).Built on the official
@modelcontextprotocol/sdk(v1). Mirrors@openrouter/agentconventions (ESM, tsc→esm, biome, vitest unit+e2e, strict types — noany/as, every MCP-JSON boundary guarded with a typeguard).Note
The plan assumed the abort signal would arrive via
callModel's tool context, but the agent SDK'sToolExecuteContextdoesn't expose one — sosignalis acreateMCPToolsoption (passed once, like auth) threaded into everycallTool.Test plan
pnpm turbo run build typecheck lint testgreen across the monorepoany/asinsrc/tests/e2e/, gated onMCP_TEST_URL) run against a real remote MCP server