feat(harness): add in-house MCP extension#3270
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
Reviews (1): Last reviewed commit: "feat(harness): add MCP extension" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| const error = url.searchParams.get("error"); | ||
| if (error) { | ||
| const description = url.searchParams.get("error_description"); | ||
| const message = description ? `${error}: ${description}` : error; | ||
| res.writeHead(200, { "Content-Type": "text/html" }); | ||
| res.end(page("Authorization failed", "Authorization failed", message)); | ||
| this.pending.delete(state); | ||
| clearTimeout(pending.timer); | ||
| pending.reject(new McpError(message, "<oauth>", "connection")); | ||
| return; | ||
| } | ||
|
|
||
| const code = url.searchParams.get("code"); | ||
| if (!code) { | ||
| res.writeHead(400, { "Content-Type": "text/html" }); | ||
| res.end( | ||
| page( | ||
| "Authorization failed", |
There was a problem hiding this comment.
When the OAuth authorization server redirects with a known state but neither a code nor an error parameter, the response is a 400 HTML page but the pending callback is silently dropped — it is neither resolved nor rejected. The waiter then hangs until the 5-minute timeout fires, leaving the user with a browser error page and an unresponsive terminal for up to 5 minutes. Adding this.pending.delete(state), clearTimeout(pending.timer), and pending.reject(new McpError(...)) before the early return (mirroring the pattern used in the error branch above) would fix this.
| ); | ||
| if (current.has(piName)) { | ||
| // Two MCP tools sanitize to the same pi name (e.g. "a-b" vs "a_b"). | ||
| // The later definition wins; recorded for /mcp <name> diagnostics. | ||
| collisions.push({ | ||
| serverName, | ||
| mcpToolName: tool.name, | ||
| piToolName: piName, | ||
| }); | ||
| } | ||
| current.add(piName); | ||
| this.registerTool(piName, serverName, tool, client, timeoutMs); |
There was a problem hiding this comment.
When two MCP tools sanitize to the same pi name (e.g. "a-b" and "a_b" both become "a_b"), only the LATER tool is appended to collisions. The first tool — the one whose name is taken — is registered and then immediately overwritten by the later registerTool call, but its name never appears in the collision list shown by /mcp <name>. A user debugging a "missing tool" situation would see only one entry in the collisions report, not both sides of the conflict.
| if (manager === null) { | ||
| if (Object.keys(nextConfig.mcpServers).length === 0) return; | ||
| buildRuntime(nextConfig); | ||
| } else if (JSON.stringify(nextConfig) !== JSON.stringify(config)) { | ||
| // Config changed (e.g. resumed into a different project): tear down | ||
| // the old runtime and rebuild from the new config. | ||
| await teardown(); | ||
| buildRuntime(nextConfig); | ||
| } | ||
|
|
||
| await startEagerServers(ctx); | ||
| }); |
There was a problem hiding this comment.
JSON.stringify config comparison is sensitive to key-insertion order
JSON.stringify(nextConfig) !== JSON.stringify(config) compares the two parsed configs. Zod's output preserves the key ordering from the source JSON, so if the user edits mcp.json and reorders fields without changing values, the comparison produces a false positive, triggering a full teardown and rebuild of the runtime on every subsequent session resume. A deep-equality check (or normalizing to a canonical form) would be more robust.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- callback-server: reject the pending waiter on a malformed redirect (known state, no code/error) instead of leaving it to hang until the 5-minute timeout. - tool-bridge: record both sides of a tool-name collision, not just the tool that wins the shadowing. - extension: replace JSON.stringify config-change comparison with a deep-equal check, so reordering (but not changing) mcp.json fields no longer triggers a needless runtime teardown/rebuild on resume.
|
LFG |
2 participants
Replace https://github.com/nicobailon/pi-mcp-adapter with our own extension which comes with no unnecessary bells and whistles