refactor(mcp): replace vendored adapter with published pi-mcp-adapter package - #1141
refactor(mcp): replace vendored adapter with published pi-mcp-adapter package#1141pmateusz wants to merge 1 commit into
Conversation
… package Remove the in-repo mcp-adapter extension (~78 files) and depend on the published pi-mcp-adapter package instead. Kimchi-specific concerns live in a new thin src/extensions/mcp/ wrapper: config loading, OAuth storage migration, server probing, read-only tool registry integration, and tool annotation catalog. Adds --mcp-config CLI flag for pointing at a specific MCP config file, and updates ACP session/new handling, permissions, and the e2e MCP suites accordingly. Co-Authored-By: Kimchi <noreply@kimchi.dev>
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 81/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Existing tests were updated: 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 81/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Existing tests were updated: src/commands/mcp.test.ts was rewritten to cover the new keyring-check subcommand and the upstream probe delegation, and src/extensions/context-budget.test.ts was updated for the new mcp/config.js path. However, the diff shows a large reduction in mcp.test.ts coverage (from 736 to 173 lines), removing many validation, OAuth isolation, timeout, cleanup, and large-payload tests. No new test files for the new facade modules are visible in the diff; the audit document describes manual/CI verification gates instead.
📝 Found 5 issue(s). See inline comments for details.
| @@ -1,736 +1,173 @@ | |||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" | |||
There was a problem hiding this comment.
The diff replaces the previous 736-line probe test suite with a 173-line suite, dropping coverage for input-validation edge cases, OAuth URL-mismatch credential isolation, stdin TTY/timeout/size guards, large-payload flush behavior, and finally-cleanup assertions. Without equivalent replacement tests in the diff, regressions in runProbe error paths are more likely to go undetected.
💡 Suggestion: Migrate the deleted probe tests to exercise UpstreamMcpProbe instead of McpServerManager, or add integration tests for kimchi mcp probe covering OAuth credential isolation, timeout paths, and adapter cleanup before this branch ships.
| @@ -114,76 +124,26 @@ async function runProbe(args: string[]): Promise<number> { | |||
| return await emitError("Server config must have either 'command' or 'url'", null) | |||
| } | |||
There was a problem hiding this comment.
runProbe now sets timeoutMs to 60_000 and passes authenticate: true for every URL-based ServerEntry, not only servers that advertise OAuth support. If UpstreamMcpProbe.probeTools interprets authenticate: true as a request to start an OAuth flow, plain HTTP MCP servers may unnecessarily prompt for authorization and wait a full minute before failing.
💡 Suggestion: Drive the authenticate option and the longer timeout from the server's auth configuration (for example, definition.auth === "oauth" or an upstream supportsOAuth check) rather than from the presence of a URL.
| const usedThrowaway = probeName !== name | ||
|
|
||
| const manager = new McpServerManager() | ||
| const controller = new AbortController() |
There was a problem hiding this comment.
The finally block in runProbe only clears the timeout; it no longer calls manager.closeAll() or explicitly aborts the UpstreamMcpProbe instance. If probeTools throws before returning, the upstream adapter's subprocesses, HTTP transports, or OAuth callback listener could remain alive after the CLI exits.
💡 Suggestion: Call controller.abort() in the finally block and, if UpstreamMcpProbe exposes a close/dispose method, invoke it there so the command cleans up even when probeTools rejects.
| @@ -114,76 +124,26 @@ async function runProbe(args: string[]): Promise<number> { | |||
| return await emitError("Server config must have either 'command' or 'url'", null) | |||
| } | |||
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The timeout message claims the 60-second limit is "including OAuth flow" for any server with a URL, even when the server does not use OAuth. This makes logs and error messages misleading for non-auth HTTP servers.
💡 Suggestion: Choose the timeout and message based on whether the server configuration actually requires OAuth, or rephrase the message to avoid implying OAuth for non-auth HTTP servers.
| { | ||
| get: (_target, prop) => { | ||
| if (prop === "registerTool") return (tool: AnyToolDef) => tools.set(tool.name, tool) | ||
| if (prop === "getFlag") return () => undefined |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The createCaptureApi proxy now intercepts getFlag and always returns undefined. Any extension whose registered tool surface depends on feature flags will register a different set of tools under measurement than in production, weakening the canonical context-budget measurement.
💡 Suggestion: Either mirror the real flag values from the extension API or document that getFlag is intentionally stubbed for the budget measurement and verify the resulting tool list remains representative.
1 participant
Linked issue
Closes #0
What does this PR do?
Remove the in-repo mcp-adapter extension (~78 files) and depend on the published pi-mcp-adapter package instead. Kimchi-specific concerns live in a new thin src/extensions/mcp/ wrapper: config loading, OAuth storage migration, server probing, read-only tool registry integration, and tool annotation catalog. Adds --mcp-config CLI flag for pointing at a specific MCP config file, and updates ACP session/new handling, permissions, and the e2e MCP suites accordingly.
Checklist
pnpm run test)pnpm run check)