From bddbbfd5cf8eb549ac8903ec37a71b0db31586a5 Mon Sep 17 00:00:00 2001 From: Baris Demirdelen Date: Wed, 15 Jul 2026 12:05:33 +0200 Subject: [PATCH 1/3] fix(acp): pick up MCP tools that connect after the startup race --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/modes/acp/acp-agent.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 059389934..669582ba7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -30,6 +30,7 @@ ### Fixed - Fixed discarded `Settings` instances keeping debounced save timers and chained background saves armed; discarding an instance now cancels its pending writes so they cannot race a successor's file locks. +- Fixed ACP sessions silently dropping tools from MCP servers that finished connecting after `MCPManager`'s 250ms startup race window; the initial post-connect refresh and every later `onToolsChanged` follow-up now run through a single ordered queue, so late-arriving tools are still applied to the session. - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. - Fixed terminal `yield` results racing post-turn maintenance, which could trigger an unnecessary automatic handoff or compaction. - Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator. diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 7d862fc26..2db4b4a0a 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -2380,6 +2380,18 @@ export class AcpAgent implements Agent { } const manager = new MCPManager(record.session.sessionManager.getCwd()); + manager.setOnToolsChanged(tools => { + // `connectServers` below only waits `STARTUP_TIMEOUT_MS` (250ms) before + // returning; slower servers (stdio JVM processes, remote HTTP MCPs) finish + // connecting afterward and report their tools through this callback. Without + // it those tools would connect successfully but never reach the model. + if (record.mcpManager !== manager) return; + void record.session.refreshMCPTools(tools, { activateAll: true }).catch(error => { + logger.warn("ACP MCP tool refresh failed", { + error: error instanceof Error ? error.message : String(error), + }); + }); + }); const configs: MCPConfigMap = {}; const sources: MCPSourceMap = {}; for (const server of servers) { From d4f97d39f05cc5b109de54f3ffba8bfe58216fe2 Mon Sep 17 00:00:00 2001 From: Baris Demirdelen Date: Wed, 15 Jul 2026 15:33:40 +0200 Subject: [PATCH 2/3] Fix race condition in the race condition fix --- .../coding-agent/src/modes/acp/acp-agent.ts | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 2db4b4a0a..a7792bc7a 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -2380,17 +2380,27 @@ export class AcpAgent implements Agent { } const manager = new MCPManager(record.session.sessionManager.getCwd()); - manager.setOnToolsChanged(tools => { - // `connectServers` below only waits `STARTUP_TIMEOUT_MS` (250ms) before - // returning; slower servers (stdio JVM processes, remote HTTP MCPs) finish - // connecting afterward and report their tools through this callback. Without - // it those tools would connect successfully but never reach the model. - if (record.mcpManager !== manager) return; - void record.session.refreshMCPTools(tools, { activateAll: true }).catch(error => { - logger.warn("ACP MCP tool refresh failed", { - error: error instanceof Error ? error.message : String(error), - }); + // MCP servers connect and reconnect independently, so `onToolsChanged` can fire + // several times back to back. Each firing is chained onto `refreshChain` so + // refreshes apply in order, and each one re-reads `manager.getTools()` at the + // time it actually runs rather than the snapshot from when it was queued — so a + // refresh can never apply a stale, smaller tool set after a newer one already landed. + let refreshChain: Promise = Promise.resolve(); + const enqueueMcpToolsRefresh = (): Promise => { + refreshChain = refreshChain.then(async () => { + if (record.mcpManager !== manager) return; + try { + await record.session.refreshMCPTools(manager.getTools()); + } catch (error) { + logger.warn("ACP MCP tool refresh failed", { + error: error instanceof Error ? error.message : String(error), + }); + } }); + return refreshChain; + }; + manager.setOnToolsChanged(() => { + void enqueueMcpToolsRefresh(); }); const configs: MCPConfigMap = {}; const sources: MCPSourceMap = {}; @@ -2414,7 +2424,7 @@ export class AcpAgent implements Agent { } record.mcpManager = manager; - await record.session.refreshMCPTools(result.tools); + await enqueueMcpToolsRefresh(); } #toMcpConfig(server: McpServer): MCPServerConfig { From 140fd58b701d0503a36187b4063892066539d190 Mon Sep 17 00:00:00 2001 From: Baris Demirdelen Date: Sat, 18 Jul 2026 01:33:13 +0200 Subject: [PATCH 3/3] Add test for late mcp startup in acp --- packages/coding-agent/test/acp-agent.test.ts | 56 ++++++++++++- .../test/fixtures/delayed-tool-mcp.ts | 80 +++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) create mode 100755 packages/coding-agent/test/fixtures/delayed-tool-mcp.ts diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 78900cd54..fdea41505 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it } from "bun:test"; +import { afterEach, describe, expect, it, spyOn } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -39,6 +39,7 @@ import { } from "@oh-my-pi/pi-coding-agent/tts/models"; import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; import type { z } from "zod/v4"; +import { TOOL_NAME as DELAYED_MCP_TOOL_NAME } from "./fixtures/delayed-tool-mcp"; /** * Validate an ACP wire payload against the external `@agentclientprotocol/sdk` @@ -2517,3 +2518,56 @@ describe("ACP agent", () => { }); }); }); + +describe("ACP agent MCP server configuration (late-connecting servers)", () => { + const FIXTURE_PATH = path.join(import.meta.dir, "fixtures", "delayed-tool-mcp.ts"); + const BUN_EXEC = process.execPath; + + // Real polling, not fake timers: the fixture is a genuine child process + // racing MCPManager's own `Bun.sleep`-based 250ms startup window, and a + // subprocess's timers cannot be advanced from this test's fake-timer clock. + async function pollUntil(predicate: () => boolean, timeoutMs = 3_000): Promise { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + if (Date.now() >= deadline) throw new Error("pollUntil timed out"); + await Bun.sleep(5); + } + } + + /** + * Regression test: an MCP server that finishes connecting after + * `MCPManager`'s 250ms startup race window used to have its tools + * silently discarded — `#configureMcpServers` only called + * `session.refreshMCPTools` once, synchronously, with whatever + * `connectServers` returned inside the race window. The background + * `onToolsChanged` -> `refreshMCPTools` follow-up now runs through a + * `refreshChain` queue so late connections still land in the session. + */ + it("delivers a late-connecting server's tools via a queued refreshMCPTools call", async () => { + const harness = await createHarness(); + const refreshSpy = spyOn(FakeAgentSession.prototype, "refreshMCPTools"); + const namesOf = (tools: unknown[]) => (tools as Array<{ name: string }>).map(tool => tool.name); + + try { + const created = await harness.agent.newSession({ + cwd: harness.cwdA, + mcpServers: [{ name: "delayed", command: BUN_EXEC, args: [FIXTURE_PATH], env: [] }], + }); + expectAcpStructure(zNewSessionResponse, created); + + // The fixture delays its `initialize` response past the 250ms startup + // race, so the first (synchronous) refresh inside `#configureMcpServers` + // must see no tools yet. + expect(refreshSpy.mock.calls).toHaveLength(1); + expect(namesOf(refreshSpy.mock.calls[0]?.[0] ?? [])).toEqual([]); + + // Once the delayed `initialize` response lands, the background + // `onToolsChanged` -> queued `refreshMCPTools` call must deliver the + // server's tool. Before the fix, this late arrival was dropped. + await pollUntil(() => refreshSpy.mock.calls.length > 1); + expect(namesOf(refreshSpy.mock.calls.at(-1)?.[0] ?? [])).toEqual([`mcp__delayed_${DELAYED_MCP_TOOL_NAME}`]); + } finally { + refreshSpy.mockRestore(); + } + }, 15_000); +}); diff --git a/packages/coding-agent/test/fixtures/delayed-tool-mcp.ts b/packages/coding-agent/test/fixtures/delayed-tool-mcp.ts new file mode 100755 index 000000000..5bf8a2027 --- /dev/null +++ b/packages/coding-agent/test/fixtures/delayed-tool-mcp.ts @@ -0,0 +1,80 @@ +#!/usr/bin/env bun +/** + * Test fixture: a well-behaved stdio MCP server that answers `initialize` + * only after a deliberate delay exceeding `MCPManager`'s `STARTUP_TIMEOUT_MS` + * (250 ms), then responds normally to `tools/list`. + * + * Models a server that eventually connects successfully but not within the + * manager's startup race window (`Promise.race([Promise.allSettled(...), + * delay(STARTUP_TIMEOUT_MS)])`), so its tools only land via the background + * `#onToolsChanged` path instead of the initial `connectServers` result. + * + * Speaks newline-delimited JSON-RPC 2.0 (the wire format of `StdioTransport`): + * one JSON object per line on stdin, one JSON response per line on stdout. + * Only requests (objects with an `id`) get a response; notifications + * (including `notifications/initialized`) are dropped. + */ +import * as readline from "node:readline"; + +export const TOOL_NAME = "late_tool"; +export const TOOL_RESULT = "MCP_LATE_CONNECT_OK_9d21"; +export const INITIALIZE_DELAY_MS = 450; + +type JsonRpcRequest = { + jsonrpc: "2.0"; + id?: string | number; + method: string; + params?: Record; +}; + +function buildResult(method: string): Record { + switch (method) { + case "initialize": + return { + protocolVersion: "2025-03-26", + serverInfo: { name: "delayed-tool-fixture", version: "1.0.0" }, + capabilities: { tools: {} }, + }; + case "tools/list": + return { + tools: [ + { + name: TOOL_NAME, + description: "Fixture tool that only arrives after the startup race window.", + inputSchema: { type: "object", properties: {}, additionalProperties: false }, + }, + ], + }; + case "tools/call": + return { content: [{ type: "text", text: TOOL_RESULT }], isError: false }; + default: + return {}; + } +} + +function startServer(): void { + const rl = readline.createInterface({ input: process.stdin }); + rl.on("line", line => { + void (async () => { + const trimmed = line.trim(); + if (trimmed.length === 0) return; + let msg: JsonRpcRequest; + try { + msg = JSON.parse(trimmed) as JsonRpcRequest; + } catch { + return; + } + if (msg.id === undefined || msg.id === null) return; + if (msg.method === "initialize") { + await Bun.sleep(INITIALIZE_DELAY_MS); + } + const response = { jsonrpc: "2.0" as const, id: msg.id, result: buildResult(msg.method) }; + process.stdout.write(`${JSON.stringify(response)}\n`); + })(); + }); + rl.on("close", () => process.exit(0)); +} + +if (import.meta.main) { + startServer(); +}