fix(mcp): preserve bridged input schemas#167
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe STDIO bridge was refactored to use an MCP SDK Server that proxies ListTools and CallTool to a connected remote MCP Client; integration tests add a typed ChangesRemote MCP bridge request-proxy + remember tests
Sequence Diagram(s)sequenceDiagram
participant Client
participant BridgeServer
participant RemoteMcpClient
participant RemoteServer
Client->>BridgeServer: ListToolsRequest
BridgeServer->>RemoteMcpClient: forward listTools()
RemoteMcpClient->>RemoteServer: transport -> remote MCP server
RemoteServer-->>RemoteMcpClient: tools list
RemoteMcpClient-->>BridgeServer: tools list
BridgeServer-->>Client: tools list
Client->>BridgeServer: CallToolRequest(name,args,_meta)
BridgeServer->>RemoteMcpClient: callTool(name,args,_meta) via callMcpToolWithResolvedTimeout
RemoteMcpClient->>RemoteServer: transport -> remote callTool
RemoteServer-->>RemoteMcpClient: tool response (content / progress)
RemoteMcpClient-->>BridgeServer: response / progress notifications
BridgeServer-->>Client: tool response (content) / notifications/progress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR refactors
Confidence Score: 5/5This PR is safe to merge; the transparent-proxy approach eliminates the Zod conversion layer entirely and the integration tests confirm schema fidelity over both transports. The change replaces a lossy schema conversion with a direct passthrough — removing the main source of information loss. The callTool path correctly delegates argument normalisation and timeout resolution to callMcpToolWithResolvedTimeout. Integration tests exercise both HTTP and SSE paths and assert the exact schema shape round-trips, including required fields, nullable anyOf, and nested objects. No behavioural regressions were identified. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/cli/src/mcp/bridges/remoteMcpStdioBridge.ts | Replaces per-tool McpServer registration (with z.any() schemas) with raw Server request handlers that transparently proxy listTools and callTool to the remote client; logic is simpler and more correct. |
| apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts | Upgrades test harness to low-level Server, adds the 'remember' tool with nullable anyOf fields and nested object schema, asserts schema round-trip fidelity over HTTP and SSE, and fixes client cleanup in finally blocks. |
Sequence Diagram
sequenceDiagram
participant STDIO Client
participant Bridge (Server / StdioServerTransport)
participant Remote MCP Server (HTTP or SSE)
STDIO Client->>Bridge (Server / StdioServerTransport): listTools (cursor?)
Bridge (Server / StdioServerTransport)->>Remote MCP Server (HTTP or SSE): listTools(request.params)
Remote MCP Server (HTTP or SSE)-->>Bridge (Server / StdioServerTransport): tools[] with raw JSON schemas
Bridge (Server / StdioServerTransport)-->>STDIO Client: tools[] (verbatim, no Zod conversion)
STDIO Client->>Bridge (Server / StdioServerTransport): callTool(name, arguments)
Bridge (Server / StdioServerTransport)->>Remote MCP Server (HTTP or SSE): callMcpToolWithResolvedTimeout(name, arguments)
Remote MCP Server (HTTP or SSE)-->>Bridge (Server / StdioServerTransport): tool result
Bridge (Server / StdioServerTransport)-->>STDIO Client: tool result (verbatim)
Reviews (2): Last reviewed commit: "test(mcp): address remote bridge review ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const anyOf = Array.isArray(record.anyOf) ? record.anyOf.map((entry) => jsonSchemaToZod(entry)) : null; | ||
| if (anyOf && anyOf.length > 0) { | ||
| const [first, ...rest] = anyOf; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } | ||
|
|
||
| const oneOf = Array.isArray(record.oneOf) ? record.oneOf.map((entry) => jsonSchemaToZod(entry)) : null; | ||
| if (oneOf && oneOf.length > 0) { | ||
| const [first, ...rest] = oneOf; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } |
There was a problem hiding this comment.
anyOf/oneOf alternatives
The pairwise reduce builds z.union([z.union([A, B]), C]) instead of a flat z.union([A, B, C]). When the bridge's McpServer re-serializes this Zod schema for the STDIO client, some Zod-to-JSON-Schema adapters emit a nested {"anyOf": [{"anyOf": [A_schema, B_schema]}, C_schema]} rather than the flat {"anyOf": [A, B, C]} the remote originally advertised. The PR's goal is to faithfully preserve the remote schema; a 3-alternative union (e.g., string | number | null) would arrive at the downstream client in a non-standard nested form. A flat approach — collecting all entries and passing them directly to z.union(entries as [ZodTypeAny, ZodTypeAny, ...ZodTypeAny[]]) — avoids the nesting entirely.
| function jsonSchemaToZod(schema: unknown): z.ZodTypeAny { | ||
| const record = asRecord(schema); | ||
| if (!record) return z.any(); | ||
|
|
||
| const anyOf = Array.isArray(record.anyOf) ? record.anyOf.map((entry) => jsonSchemaToZod(entry)) : null; | ||
| if (anyOf && anyOf.length > 0) { | ||
| const [first, ...rest] = anyOf; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } | ||
|
|
||
| const oneOf = Array.isArray(record.oneOf) ? record.oneOf.map((entry) => jsonSchemaToZod(entry)) : null; | ||
| if (oneOf && oneOf.length > 0) { | ||
| const [first, ...rest] = oneOf; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } | ||
|
|
||
| if (Array.isArray(record.enum) && record.enum.length > 0) { | ||
| const literals = record.enum.map((entry) => z.literal(entry)); | ||
| const [first, ...rest] = literals; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } | ||
|
|
||
| const typeValue = record.type; | ||
| if (Array.isArray(typeValue) && typeValue.length > 0) { | ||
| const entries = typeValue.map((entry) => jsonSchemaToZod({ ...record, type: entry })); | ||
| const [first, ...rest] = entries; | ||
| return rest.reduce<z.ZodTypeAny>((current, entry) => z.union([current, entry]), first); | ||
| } | ||
|
|
||
| switch (typeValue) { | ||
| case 'string': | ||
| return z.string(); | ||
| case 'number': | ||
| return z.number(); | ||
| case 'integer': | ||
| return z.number().int(); | ||
| case 'boolean': | ||
| return z.boolean(); | ||
| case 'null': | ||
| return z.null(); | ||
| case 'array': { | ||
| return z.array(jsonSchemaToZod(record.items)); | ||
| } | ||
| case 'object': { | ||
| const properties = asRecord(record.properties) ?? {}; | ||
| const required = new Set(Array.isArray(record.required) ? record.required.filter((entry): entry is string => typeof entry === 'string') : []); | ||
| const shape: Record<string, z.ZodTypeAny> = {}; | ||
| for (const [key, value] of Object.entries(properties)) { | ||
| const propertySchema = jsonSchemaToZod(value); | ||
| shape[key] = required.has(key) ? propertySchema : propertySchema.optional(); | ||
| } | ||
|
|
||
| let objectSchema = z.object(shape); | ||
| if (record.additionalProperties === false) return objectSchema.strict(); | ||
|
|
||
| const additionalProperties = asRecord(record.additionalProperties); | ||
| if (additionalProperties) return objectSchema.catchall(jsonSchemaToZod(additionalProperties)); | ||
|
|
||
| return objectSchema.passthrough(); | ||
| } | ||
| default: | ||
| return z.any(); | ||
| } | ||
| } |
There was a problem hiding this comment.
allOf and $ref silently degrade to z.any()
jsonSchemaToZod handles anyOf, oneOf, enum, and scalar primitive types but has no branch for allOf or $ref. If a remote MCP server returns a schema that uses either construct (e.g., an intersection type or a shared definition), the resulting Zod schema is z.any(), stripping all field-level constraints. This is the same loss of information the PR is meant to fix. A comment or // TODO in the fallback default case would at least surface the gap; adding a best-effort allOf → z.intersection path (or z.and) would close it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts (1)
52-68: ⚡ Quick winOptional: consider strengthening the assertion to also verify
context/tagsare surfaced as nullable.The current assertion only pins
required: ['content']. Since the translator now also has to round-trip nullable+optional fields (z.union([z.string(), z.null()]).optional()), asserting thatcontextandtagsappear inpropertieswith the expected nullable shape would lock in the broader translator behavior, not just the required-field fix. Skip if you'd rather keep the test minimal.🧪 Example assertion extension
expect(rememberTool?.inputSchema).toMatchObject({ type: 'object', required: ['content'], properties: { content: { type: 'string' }, + context: expect.any(Object), + tags: expect.any(Object), }, }); + expect((rememberTool?.inputSchema as any)?.required).not.toContain('context'); + expect((rememberTool?.inputSchema as any)?.required).not.toContain('tags');Also applies to: 110-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts` around lines 52 - 68, Extend the test assertion for the 'remember' tool registration (mcp.registerTool with inputSchema) to also verify that the generated JSON Schema includes nullable+optional shapes for context and tags: assert that properties.context is a schema allowing string or null and optional, and properties.tags is a schema allowing array-of-strings or null and optional, in addition to the existing required:['content'] check; update the parallel assertion in the second block (the other registration around lines 110-126) similarly so both round-trip nullable+optional fields are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts`:
- Around line 52-68: Extend the test assertion for the 'remember' tool
registration (mcp.registerTool with inputSchema) to also verify that the
generated JSON Schema includes nullable+optional shapes for context and tags:
assert that properties.context is a schema allowing string or null and optional,
and properties.tags is a schema allowing array-of-strings or null and optional,
in addition to the existing required:['content'] check; update the parallel
assertion in the second block (the other registration around lines 110-126)
similarly so both round-trip nullable+optional fields are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f6bcad3-b6c0-49b4-bd63-3a6d8eb20c94
📒 Files selected for processing (2)
apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.tsapps/cli/src/mcp/bridges/remoteMcpStdioBridge.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts (1)
236-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the stdio client in
finally, not only on success.If any assertion or tool call fails before Lines 255/304, the bridge subprocess launched by
StdioClientTransportnever gets closed. That leaks processes across tests and can introduce flake. Hoistclientout of thetryblock and close it in eachfinally.🔧 Apply the same cleanup pattern to both tests
- try { + let client: Client | null = null; + try { ... - const client = new Client({ name: 'bridge-test', version: '1.0.0' }, { capabilities: {} }); + client = new Client({ name: 'bridge-test', version: '1.0.0' }, { capabilities: {} }); await client.connect(transport); ... - await client.close(); } finally { + await client?.close().catch(() => {}); httpServer.stop(); await rm(tmp, { recursive: true, force: true }); }As per coding guidelines, "Tests should be deterministic and isolated (no shared global state, no ordering reliance)".
Also applies to: 289-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts` around lines 236 - 255, The test currently constructs and connects a Client inside a try block but only calls client.close() on the success path, leaking the StdioClientTransport subprocess on failures; hoist the Client variable (new Client(...)) and its connect call out so the client reference is available in the outer scope, then ensure you call client.close() in a finally block (or use a try/finally) after the test's tool calls and assertions; apply this cleanup pattern to both tests that create a Client (the blocks invoking client.connect, listTools(), callTool() and readFirstTextContent) so the subprocess is always closed even on assertion/tool-call failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts`:
- Around line 27-37: The metadata schema in the test uses allOf with the source
property nested inside it while additionalProperties: false sits at the root,
which invalidates fixtures like { source: 'test' }; update the schema in
remoteMcpStdioBridge.integration.test.ts by removing the allOf wrapper and
declaring properties: { source: { type: 'string' } }, required: ['source'], and
additionalProperties: false directly on the metadata object so the validator
recognizes the source property (i.e., flatten the schema used in the test).
In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.ts`:
- Around line 86-89: The Server is advertising capabilities.tools.listChanged =
true but the bridge (remoteMcpStdioBridge) does not forward remote
"notifications/tools/list_changed" events, violating MCP; either remove the
false advertisement or implement forwarding: update the Server constructor to
use { capabilities: { tools: {} } } to stop advertising listChanged, or
subscribe the remote client for "notifications/tools/list_changed" and forward
those events over the stdio transport by emitting the same notification from the
Server when the remote event arrives (ensure forwarding logic ties into the
existing ListTools/CallTool proxy paths and the stdio client used by this
bridge).
---
Outside diff comments:
In `@apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.ts`:
- Around line 236-255: The test currently constructs and connects a Client
inside a try block but only calls client.close() on the success path, leaking
the StdioClientTransport subprocess on failures; hoist the Client variable (new
Client(...)) and its connect call out so the client reference is available in
the outer scope, then ensure you call client.close() in a finally block (or use
a try/finally) after the test's tool calls and assertions; apply this cleanup
pattern to both tests that create a Client (the blocks invoking client.connect,
listTools(), callTool() and readFirstTextContent) so the subprocess is always
closed even on assertion/tool-call failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0839d9e1-f6c6-4c63-b67d-c2835ae47388
📒 Files selected for processing (2)
apps/cli/src/mcp/bridges/remoteMcpStdioBridge.integration.test.tsapps/cli/src/mcp/bridges/remoteMcpStdioBridge.ts
|
Thank you @jr200 ! I have actually just changed the approach slightly: instead of converting the remote JSON Schema into Zod and registering each remote tool locally, the bridge now uses the lower-level MCP
That way, the remote @CodeRabbit @greptile please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the current state of the files before reviewing. 🏁 Script executed: Length of output: 22833
A couple of things worth considering: 1. Remote client not closed on stdio disconnect (potential process hang) When the stdio transport closes (e.g., the parent process exits or drops the connection), // After server.connect(stdio), add:
stdio.onclose = async () => {
await remoteClient.close().catch(() => {});
};Or equivalently via 2. Progress token (
3. Pagination cursor is correctly forwarded for Passing 4. Integration test structure ✅ The per-request Overall the implementation is solid. The remote client lifecycle ( |
2 participants
Summary
z.any()contentrequired: [\"content\"]over both HTTP and SSE transportsProblem
When Happier bridged remote MCP tools through
remoteMcpStdioBridge, it registered every tool withinputSchema: z.any(). Tool calls still worked, butlistTools()lost required-field information. In practice this broke Hindsight-backed tools in Happier-managed OpenCode sessions because required arguments such ascontentdisappeared from the surfaced schema.Verification
yarn --cwd "/Users/primary/code/jr200/happier/apps/cli" buildyarn --cwd "/Users/primary/code/jr200/happier/apps/cli" vitest run --config vitest.integration.config.ts src/mcp/bridges/remoteMcpStdioBridge.integration.test.tsNeed help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Improvements
Tests