From 185615497ca02bdfaa4c3eb2d708a7dd2e47fae9 Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 20 May 2026 21:40:43 +0000 Subject: [PATCH] fix(acp): disable host MCP discovery in ACP session factory ACP clients own MCP server configuration via session/new.mcpServers and AcpAgent#configureMcpServers. The ACP session factory previously left enableMCP at its default (true), so createAgentSession ran discoverAndLoadMCPTools on every session/new and the resulting host MCP tools landed in the session tool registry alongside the client-supplied ones. search_tool_bm25 then surfaced only the host tools. Force enableMCP: false on every session created through createAcpSessionFactory so on-disk discovery is bypassed in ACP mode. Non-ACP modes (omp interactive, print, RPC) keep auto-discovery. Fixes #1234 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/main.ts | 15 +++- .../test/acp-mcp-isolation.test.ts | 76 +++++++++++++++++++ 3 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/acp-mcp-isolation.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6526c6481..ebd49b15a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `omp acp` auto-discovering host `.mcp.json` servers in parallel with the ACP client's `session/new.mcpServers`, which shadowed client-supplied MCP tools in `search_tool_bm25` and the session tool registry. The ACP session factory now forces `enableMCP: false`, so MCP ownership stays with `AcpAgent#configureMcpServers`. Non-ACP modes keep on-disk discovery. ([#1234](https://github.com/can1357/oh-my-pi/issues/1234)) + ## [15.1.8] - 2026-05-20 ### Fixed diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index 14b4f137f..43c33cbaa 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -199,7 +199,7 @@ function applyExtensionFlagValues(session: AgentSession, rawArgs: string[]): Map type AcpSessionFactory = (cwd: string) => Promise; -interface AcpSessionFactoryOptions { +export interface AcpSessionFactoryOptions { baseOptions: CreateAgentSessionOptions; settings: Settings; sessionDir?: string; @@ -210,7 +210,17 @@ interface AcpSessionFactoryOptions { createSession: (options: CreateAgentSessionOptions) => Promise; } -function createAcpSessionFactory(args: AcpSessionFactoryOptions): AcpSessionFactory { +/** + * Build the per-`session/new` factory used by ACP mode. + * + * MCP servers in ACP sessions are owned exclusively by the ACP client, which + * supplies them through `session/new.mcpServers` and re-applies them via + * {@link AcpAgent#configureMcpServers}. We therefore force `enableMCP: false` + * on every session created here so {@link createAgentSession} skips the on-disk + * `.mcp.json` discovery path — otherwise host MCP tools land in the session's + * tool registry and shadow the client-supplied servers (issue #1234). + */ +export function createAcpSessionFactory(args: AcpSessionFactoryOptions): AcpSessionFactory { return async cwd => { const nextSettings = await args.settings.cloneForCwd(cwd); const nextSessionManager = SessionManager.create(cwd, args.sessionDir); @@ -224,6 +234,7 @@ function createAcpSessionFactory(args: AcpSessionFactoryOptions): AcpSessionFact modelRegistry: args.modelRegistry, agentId, hasUI: false, + enableMCP: false, }); if (args.parsedArgs.apiKey && !args.baseOptions.model && nextSession.model) { args.authStorage.setRuntimeApiKey(nextSession.model.provider, args.parsedArgs.apiKey); diff --git a/packages/coding-agent/test/acp-mcp-isolation.test.ts b/packages/coding-agent/test/acp-mcp-isolation.test.ts new file mode 100644 index 000000000..926bf53c3 --- /dev/null +++ b/packages/coding-agent/test/acp-mcp-isolation.test.ts @@ -0,0 +1,76 @@ +/** + * Regression test for issue #1234. + * + * `omp acp` must not auto-discover host `.mcp.json` servers when creating a + * session for an ACP client. MCP server ownership belongs entirely to the ACP + * client (`session/new.mcpServers` → `AcpAgent#configureMcpServers`); letting + * `createAgentSession` run on-disk discovery in parallel registers host MCP + * tools that shadow the client-supplied ones in the session tool registry. + * + * The contract enforced here is narrow on purpose: every call routed through + * the ACP session factory must reach `createAgentSession` with + * `enableMCP: false`, regardless of what `baseOptions` carries. + */ + +import { describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Snowflake } from "@oh-my-pi/pi-utils"; +import { ModelRegistry } from "../src/config/model-registry"; +import { Settings } from "../src/config/settings"; +import { createAcpSessionFactory } from "../src/main"; +import type { CreateAgentSessionOptions, CreateAgentSessionResult } from "../src/sdk"; +import type { AgentSession } from "../src/session/agent-session"; +import { AuthStorage } from "../src/session/auth-storage"; + +describe("createAcpSessionFactory MCP isolation (issue #1234)", () => { + it("forces enableMCP=false even when baseOptions opts in", async () => { + const tempDir = path.join(os.tmpdir(), `pi-acp-mcp-isolation-${Snowflake.next()}`); + fs.mkdirSync(tempDir, { recursive: true }); + const authStorage = await AuthStorage.create(path.join(tempDir, "auth.db")); + try { + const modelRegistry = new ModelRegistry(authStorage); + const settings = Settings.isolated({}); + const fakeSession = {} as AgentSession; + const captured: CreateAgentSessionOptions[] = []; + const createSession = async (options: CreateAgentSessionOptions): Promise => { + captured.push(options); + return { + session: fakeSession, + extensionsResult: { + extensions: [], + errors: [], + runner: undefined, + } as unknown as CreateAgentSessionResult["extensionsResult"], + setToolUIContext: () => {}, + eventBus: { + emit: () => {}, + on: () => () => {}, + off: () => {}, + } as unknown as CreateAgentSessionResult["eventBus"], + }; + }; + + // baseOptions deliberately sets enableMCP=true to prove the factory ignores it. + const factory = createAcpSessionFactory({ + baseOptions: { enableMCP: true } as CreateAgentSessionOptions, + settings, + sessionDir: path.join(tempDir, "sessions"), + authStorage, + modelRegistry, + parsedArgs: {}, + rawArgs: [], + createSession, + }); + + const result = await factory(tempDir); + expect(result).toBe(fakeSession); + expect(captured).toHaveLength(1); + expect(captured[0].enableMCP).toBe(false); + } finally { + authStorage.close(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); +});