From 10b830d6768b55a0ee9779fab986dfb12d1ef118 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 16:25:09 +0800 Subject: [PATCH 1/8] =?UTF-8?q?=F0=9F=90=9B=20fix(sdk):=20MCP=20manager=20?= =?UTF-8?q?=E9=80=80=E9=81=BF=E9=87=8D=E8=BF=9E=E3=80=81=E5=85=A8=E5=B1=80?= =?UTF-8?q?=E5=B7=A5=E5=85=B7=E5=90=8D=E5=8E=BB=E9=87=8D=E4=B8=8E=E8=A7=84?= =?UTF-8?q?=E8=8C=83=E5=8C=96=E6=8B=BC=E8=A3=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ensureConnected 失败负缓存：指数退避（5s 起步倍增封顶 60s）， 挂死 server 不再每个 run 阻塞整个连接超时；connect/testServer 支持 force 绕过，sidecar 手动探测走 force。 - wrapper 工具名去重提升为 manager 实例级集合：server id 大小写 折叠后跨 server 碰撞不再静默互相覆盖，断开/重建时回收旧名。 - callTool 结果为 undefined 时抛 protocol_error：在途 disconnect 不再把空结果伪装成成功喂给模型（合法空结果是对象）。 - 默认 client factory 订阅 tools/list_changed：带 generation 校验 地重拉 listTools 原地刷新工具表，动态增删工具不再陈旧到重连。 - 新增 mcp/naming 共享 util：normalizeMcpServerId/ToolName + buildMcpToolName（64 字符截断 + hash 消歧），client.ts 与 sdk-mcp-server 两处拼装点统一收口，engine 用量报告前缀同步。 Co-Authored-By: Claude Fable 5 --- .../src/services/mcp/workspace-mcp-manager.ts | 7 +- packages/sdk/src/mcp/default-factory.test.ts | 96 +++++++++++ packages/sdk/src/mcp/manager.test.ts | 128 +++++++++++++++ packages/sdk/src/mcp/manager.ts | 150 +++++++++++++----- packages/sdk/src/mcp/naming.test.ts | 27 ++++ packages/sdk/src/mcp/naming.ts | 53 +++++++ 6 files changed, 414 insertions(+), 47 deletions(-) create mode 100644 packages/sdk/src/mcp/default-factory.test.ts create mode 100644 packages/sdk/src/mcp/naming.test.ts create mode 100644 packages/sdk/src/mcp/naming.ts diff --git a/apps/sidecar/src/services/mcp/workspace-mcp-manager.ts b/apps/sidecar/src/services/mcp/workspace-mcp-manager.ts index 3d0059aac..390274afd 100644 --- a/apps/sidecar/src/services/mcp/workspace-mcp-manager.ts +++ b/apps/sidecar/src/services/mcp/workspace-mcp-manager.ts @@ -31,8 +31,8 @@ import { createDiagnosticLogSummary, createLogger, type Logger } from "../infra/ export interface WorkspaceSdkMcpManager { sync(configs: Record): void; - connect?(serverId: string): Promise; - ensureConnected?(serverId: string): Promise; + connect?(serverId: string, options?: { force?: boolean }): Promise; + ensureConnected?(serverId: string, options?: { force?: boolean }): Promise; disconnect(serverId: string): Promise; dispose(): Promise; getStatus(): Record; @@ -377,7 +377,8 @@ export class WorkspaceMcpManager { const state = this.ensureWorkspaceState(workspaceSlug); state.sdk.sync(this.normalizedConfigs(config)); try { - await (state.sdk.connect ?? state.sdk.ensureConnected)?.call(state.sdk, serverId); + // Manual probes bypass the manager's reconnect backoff (#312). + await (state.sdk.connect ?? state.sdk.ensureConnected)?.call(state.sdk, serverId, { force: true }); } catch (error) { // 连接失败的底层错误常内嵌 URL/凭据片段；与其余 MCP 路径一致经脱敏再下发， // 且错误码归一，不把原文直抛 renderer（#403） diff --git a/packages/sdk/src/mcp/default-factory.test.ts b/packages/sdk/src/mcp/default-factory.test.ts new file mode 100644 index 000000000..03d5d9b8c --- /dev/null +++ b/packages/sdk/src/mcp/default-factory.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, test, mock } from "bun:test"; +import { McpClientManager } from "./manager.js"; + +// Intercept the lazily-imported official Client so the test can observe the +// constructor options the default factory passes (the listChanged wiring is +// exactly what issue #384 requires) and drive its onChanged callback. +class FakeMcpClient { + static instances: FakeMcpClient[] = []; + tools: Array<{ name: string; inputSchema?: unknown }> = []; + closed = 0; + + constructor(_info: unknown, options: any) { + this.options = options; + FakeMcpClient.instances.push(this); + } + + options: any; + + async connect() {} + + async listTools() { + return { tools: this.tools }; + } + + async close() { + this.closed += 1; + } + + emitToolsChanged() { + const handler = this.options?.listChanged?.tools?.onChanged; + if (!handler) throw new Error("default client factory did not subscribe to tools/list_changed"); + void handler(undefined, undefined); + } +} + +mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ Client: FakeMcpClient })); + +const config = { enabled: true, transport: "streamable_http", url: "http://127.0.0.1:8787/mcp" } as const; + +describe("default client factory (#384)", () => { + test("subscribes to tools/list_changed and refreshes the tool list in place", async () => { + FakeMcpClient.instances = []; + const manager = new McpClientManager({ transportFactory: () => ({}) }); + manager.sync({ dynamic: config }); + await manager.ensureConnected("dynamic"); + + expect(FakeMcpClient.instances).toHaveLength(1); + expect(manager.getTools("dynamic")).toEqual([]); + + const client = FakeMcpClient.instances[0]!; + client.tools = [{ name: "fresh_tool", inputSchema: { type: "object" } }]; + client.emitToolsChanged(); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(manager.getTools("dynamic").map((tool) => tool.originalName)).toEqual(["fresh_tool"]); + expect(manager.getTools("dynamic")[0]?.wrapperName).toBe("mcp__dynamic__fresh_tool"); + }); + + test("ignores stale notifications after disconnect instead of resurrecting tools", async () => { + FakeMcpClient.instances = []; + const manager = new McpClientManager({ transportFactory: () => ({}) }); + manager.sync({ dynamic: config }); + await manager.ensureConnected("dynamic"); + + const client = FakeMcpClient.instances[0]!; + client.tools = [{ name: "late_tool" }]; + await manager.disconnect("dynamic"); + client.emitToolsChanged(); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(manager.getTools("dynamic")).toEqual([]); + // The refresh failure path must not leave an unhandled rejection either. + client.options.listChanged.tools.onChanged(new Error("boom")); + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + + test("keeps the last good tool list when the refresh request fails", async () => { + FakeMcpClient.instances = []; + const manager = new McpClientManager({ transportFactory: () => ({}) }); + manager.sync({ dynamic: config }); + await manager.ensureConnected("dynamic"); + + const client = FakeMcpClient.instances[0]!; + client.tools = [{ name: "stable" }]; + client.emitToolsChanged(); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(manager.getTools("dynamic").map((tool) => tool.originalName)).toEqual(["stable"]); + + client.listTools = async () => { + throw new Error("connection reset"); + }; + client.emitToolsChanged(); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(manager.getTools("dynamic").map((tool) => tool.originalName)).toEqual(["stable"]); + }); +}); diff --git a/packages/sdk/src/mcp/manager.test.ts b/packages/sdk/src/mcp/manager.test.ts index 77c43def4..57626d78f 100644 --- a/packages/sdk/src/mcp/manager.test.ts +++ b/packages/sdk/src/mcp/manager.test.ts @@ -402,6 +402,134 @@ describe("McpClientManager", () => { manager.sync({ local: { enabled: true, transport: "stdio", command: "node" } }); await expect(manager.callTool("local", "boom", {})).rejects.toMatchObject({ code: "protocol_error" }); }); + + test("throws protocol_error when a racing disconnect yields no result (#375)", async () => { + const manager = new McpClientManager({ + clientFactory: () => ({ + async connect() {}, + async listTools() { return { tools: [] }; }, + async callTool() { return undefined; }, + async close() {}, + }), + transportFactory: fakeTransportFactory, + }); + + manager.sync({ ghost: { enabled: true, transport: "stdio", command: "node" } }); + await expect(manager.callTool("ghost", "vanish", {})).rejects.toMatchObject({ code: "protocol_error" }); + }); + + test("accepts an empty-object MCP result as a legal successful call (#375)", async () => { + const manager = new McpClientManager({ + clientFactory: () => ({ + async connect() {}, + async listTools() { return { tools: [] }; }, + async callTool() { return {}; }, + async close() {}, + }), + transportFactory: fakeTransportFactory, + }); + + manager.sync({ quiet: { enabled: true, transport: "stdio", command: "node" } }); + const result = await manager.callTool("quiet", "noop", {}); + expect(result.text).toBe("{}"); + expect(result.isError).toBeUndefined(); + }); + + test("dedupes wrapper names across servers that normalize identically (#325)", async () => { + const factory = createFakeMcpFactory({ tools: [{ name: "search" }] }); + const manager = new McpClientManager({ + clientFactory: factory.clientFactory, + transportFactory: factory.transportFactory + }); + + manager.sync({ + GitHub: { enabled: true, transport: "stdio", command: "node" }, + github: { enabled: true, transport: "stdio", command: "node" } + }); + await manager.ensureConnected("GitHub"); + await manager.ensureConnected("github"); + + const names = manager.getTools().map((tool) => tool.wrapperName); + expect(new Set(names).size).toBe(names.length); + expect(names).toContain("mcp__github__search"); + expect(names.some((name) => /^mcp__github__search_[a-z0-9]{6}$/.test(name))).toBe(true); + }); + + test("reclaims released wrapper names after disconnect (#325)", async () => { + const factory = createFakeMcpFactory({ tools: [{ name: "search" }] }); + const manager = new McpClientManager({ + clientFactory: factory.clientFactory, + transportFactory: factory.transportFactory + }); + + manager.sync({ + GitHub: { enabled: true, transport: "stdio", command: "node" }, + github: { enabled: true, transport: "stdio", command: "node" } + }); + await manager.ensureConnected("GitHub"); + await manager.ensureConnected("github"); + await manager.disconnect("GitHub"); + await manager.connect("GitHub"); + + const githubNames = manager.getTools("github").map((tool) => tool.wrapperName); + const reconnectNames = manager.getTools("GitHub").map((tool) => tool.wrapperName); + // The surviving server keeps its suffixed name; the reconnected one may + // take the freed base name again. + expect(githubNames[0]).toMatch(/^mcp__github__search_[a-z0-9]{6}$/); + expect(reconnectNames).toContain("mcp__github__search"); + expect(new Set([...githubNames, ...reconnectNames]).size).toBe(2); + }); + + test("fast-fails during reconnect backoff and honors force (#312)", async () => { + let factoryCalls = 0; + const manager = new McpClientManager({ + clientFactory: () => { + factoryCalls += 1; + throw new Error("Connection refused"); + }, + transportFactory: fakeTransportFactory, + defaultReconnectBackoffBaseMs: 50, + defaultReconnectBackoffMaxMs: 10_000, + }); + + manager.sync({ dead: { enabled: true, transport: "stdio", command: "node" } }); + await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); + await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); + expect(factoryCalls).toBe(1); + + // Manual probes bypass the negative cache. + await expect(manager.ensureConnected("dead", { force: true })).rejects.toMatchObject({ code: "transport_error" }); + expect(factoryCalls).toBe(2); + + // Second consecutive failure doubles the window: past the base window but + // still inside the doubled one. + await delay(70); + await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); + expect(factoryCalls).toBe(2); + + // Past the doubled window the gate opens again. + await delay(120); + await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); + expect(factoryCalls).toBe(3); + }); + + test("explicit connect accepts the same force option (#312)", async () => { + let factoryCalls = 0; + const manager = new McpClientManager({ + clientFactory: () => { + factoryCalls += 1; + throw new Error("Connection refused"); + }, + transportFactory: fakeTransportFactory, + defaultReconnectBackoffBaseMs: 60_000, + defaultReconnectBackoffMaxMs: 60_000, + }); + + manager.sync({ dead: { enabled: true, transport: "stdio", command: "node" } }); + await expect(manager.connect("dead")).rejects.toThrow(); + await expect(manager.connect("dead", { force: true })).rejects.toThrow(); + expect(factoryCalls).toBe(2); + }); }); describe("McpClientManager #312 failed 负缓存", () => { diff --git a/packages/sdk/src/mcp/manager.ts b/packages/sdk/src/mcp/manager.ts index 550c94f92..00911f5b3 100644 --- a/packages/sdk/src/mcp/manager.ts +++ b/packages/sdk/src/mcp/manager.ts @@ -1,5 +1,6 @@ import type { SandboxSettings } from '../types.js'; import { SandboxedStdioClientTransport } from './sandboxed-stdio-transport.js'; +import { buildMcpToolName, normalizeMcpServerId, shortHash } from './naming.js'; export type McpTransportKind = 'stdio' | 'sse' | 'streamable_http'; export type McpClientStatus = 'idle' | 'connecting' | 'connected' | 'failed'; @@ -227,43 +228,16 @@ function cloneConfig(config: NormalizedMcpServerConfig): NormalizedMcpServerConf }; } -function normalizeServerId(value: string): string { - return value - .trim() - .toLowerCase() - .replace(/[^a-z0-9_-]+/g, '-') - .replace(/-+/g, '-') - .replace(/^-+|-+$/g, '') || 'server'; -} - -function normalizeToolName(value: string): string { - return value - .trim() - .toLowerCase() - .replace(/[^a-z0-9_-]+/g, '_') - .replace(/_+/g, '_') - .replace(/^_+|_+$/g, '') || 'tool'; -} - -function shortHash(value: string): string { - let hash = 2166136261; - for (let i = 0; i < value.length; i += 1) { - hash ^= value.charCodeAt(i); - hash = Math.imul(hash, 16777619); - } - return (hash >>> 0).toString(36).padStart(6, '0').slice(0, 6); -} - function buildWrapperName(serverId: string, originalToolName: string, takenNames: Set): string { - const serverNamespace = normalizeServerId(serverId); - const toolNamespace = normalizeToolName(originalToolName); - const base = `mcp__${serverNamespace}__${toolNamespace}`; + // buildMcpToolName applies the shared normalization + length clamp so the + // wrapper name is always a legal provider tool name (#326). + const base = buildMcpToolName(serverId, originalToolName); if (!takenNames.has(base)) { takenNames.add(base); return base; } - const suffix = shortHash(`${serverNamespace}\0${originalToolName}`); + const suffix = shortHash(`${normalizeMcpServerId(serverId)}\0${originalToolName}`); let candidate = `${base}_${suffix}`; let counter = 2; while (takenNames.has(candidate)) { @@ -277,9 +251,12 @@ function buildWrapperName(serverId: string, originalToolName: string, takenNames function buildToolDetails( serverId: string, config: NormalizedMcpServerConfig, - tools: Array<{ name: string; description?: string; inputSchema?: unknown }> + tools: Array<{ name: string; description?: string; inputSchema?: unknown }>, + // Manager-wide, not per-server: server ids are case-folded by + // normalizeServerId, so two servers can normalize identically and must not + // mint colliding wrapper names that silently shadow each other (#325). + takenNames: Set ): McpToolDetail[] { - const takenNames = new Set(); return tools.map((tool) => { const wrapperName = buildWrapperName(serverId, tool.name, takenNames); return { @@ -438,12 +415,37 @@ function normalizeCallResult(result: unknown): McpCallResult { }; } -async function defaultClientFactory(serverId: string): Promise { - const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); +/** + * Test seam for the lazily-imported official MCP client constructor. Mocking + * the whole '@modelcontextprotocol/sdk/client/index.js' module would leak into + * every other test file sharing bun's single test process. + */ +type McpSdkClientCtor = new ( + info: { name: string; version: string }, + options: unknown +) => unknown; +let sdkClientCtor: McpSdkClientCtor | undefined; + +export function setDefaultMcpSdkClientConstructor(ctor: McpSdkClientCtor | undefined): void { + sdkClientCtor = ctor; +} + +async function defaultClientFactory( + serverId: string, + options: { onToolsListChanged?: () => Promise | void } = {} +): Promise { + const Client: McpSdkClientCtor = sdkClientCtor + ?? ((await import('@modelcontextprotocol/sdk/client/index.js')).Client as unknown as McpSdkClientCtor); return new Client( { name: `lume-agent-sdk-${serverId}`, version: '1.0.0' }, - {} - ) as McpClientLike; + { + // Subscribe to tools/list_changed so dynamically added/removed tools do + // not leave a permanently stale tool list until the next reconnect (#384). + listChanged: options.onToolsListChanged + ? { tools: { onChanged: async () => { await options.onToolsListChanged?.(); } } } + : {}, + } + ) as unknown as McpClientLike; } async function defaultTransportFactory( @@ -492,9 +494,14 @@ export class McpClientManager { private readonly failureRetryBaseMs: number; private readonly failureRetryMaxMs: number; private readonly servers = new Map(); + // Instance-wide registry of already-issued wrapper tool names so servers + // whose ids normalize identically cannot mint colliding tools (#325). + private readonly takenWrapperNames = new Set(); constructor(options: McpClientManagerOptions = {}) { - this.clientFactory = options.clientFactory ?? defaultClientFactory; + this.clientFactory = options.clientFactory ?? ((serverId) => defaultClientFactory(serverId, { + onToolsListChanged: () => this.refreshToolsAfterListChanged(serverId), + })); this.transportFactory = options.transportFactory ?? defaultTransportFactory; this.defaultConnectTimeoutMs = options.defaultConnectTimeoutMs ?? DEFAULT_CONNECT_TIMEOUT_MS; this.defaultCallTimeoutMs = options.defaultCallTimeoutMs ?? DEFAULT_CALL_TIMEOUT_MS; @@ -507,7 +514,8 @@ export class McpClientManager { if (existing && configsEqual(existing.config, config)) { return; } - if (existing?.client) { + if (existing) { + this.releaseToolNames(existing); void this.closeState(existing); } this.servers.set(serverId, { @@ -531,19 +539,21 @@ export class McpClientManager { } } - async connect(serverId: string): Promise { - await this.ensureConnected(serverId); + async connect(serverId: string, options: { force?: boolean } = {}): Promise { + await this.ensureConnected(serverId, options); } - async ensureConnected(serverId: string): Promise { + async ensureConnected(serverId: string, options: { force?: boolean } = {}): Promise { const state = this.getStateOrThrow(serverId); if (state.status === 'connected' && state.client) { return; } // #312:failed 负缓存——退避窗口内不发起连接,快速抛缓存错误 //(waitForConnections 的调用方 catch 吞掉,run 启动不再被挂死服务器卡满 timeout) + // Manual probes pass force to bypass the gate. if ( - state.status === 'failed' + !options.force + && state.status === 'failed' && state.nextRetryAt !== undefined && Date.now() < state.nextRetryAt ) { @@ -573,6 +583,7 @@ export class McpClientManager { return; } state.generation += 1; + this.releaseToolNames(state); await this.closeState(state); state.status = 'idle'; state.tools = []; @@ -667,6 +678,12 @@ export class McpClientManager { options.timeoutMs ?? this.defaultCallTimeoutMs, options.signal ); + // A disconnect racing an in-flight call nulls state.client and the + // optional chain yields undefined; a legal empty MCP result is always + // an object, so undefined can only mean "no result" (#375). + if (result === undefined) { + throw createMcpError('protocol_error', `MCP tool call returned no result: ${serverId}/${originalToolName}`); + } return normalizeCallResult(result); } catch (error) { const classified = classifyError(error); @@ -724,7 +741,7 @@ export class McpClientManager { ); if (!isCurrent()) throw createMcpError('aborted', `MCP connection was superseded: ${serverId}`); - state.tools = buildToolDetails(serverId, state.config, toolList.tools ?? []); + this.replaceStateTools(state, serverId, toolList.tools ?? []); state.status = 'connected'; state.error = undefined; state.failureCount = 0; @@ -767,6 +784,51 @@ export class McpClientManager { } } + /** + * Swap a server's tool list while keeping the manager-wide wrapper-name + * registry consistent: the previous names are released before new ones are + * claimed (#325). + */ + private replaceStateTools( + state: ServerState, + serverId: string, + tools: Array<{ name: string; description?: string; inputSchema?: unknown }> + ): void { + this.releaseToolNames(state); + state.tools = buildToolDetails(serverId, state.config, tools, this.takenWrapperNames); + } + + /** + * Re-pull listTools after a tools/list_changed notification. Generation and + * client identity checks discard stale notifications that raced a + * disconnect/reconnect (#384). + */ + private async refreshToolsAfterListChanged(serverId: string): Promise { + const state = this.servers.get(serverId); + const client = state?.client; + if (!state || !client) return; + const generation = state.generation; + try { + const toolList = await withTimeout( + Promise.resolve(client.listTools?.() ?? { tools: [] }), + this.defaultConnectTimeoutMs + ); + if (this.servers.get(serverId) !== state || state.generation !== generation || state.client !== client) { + return; + } + this.replaceStateTools(state, serverId, toolList.tools ?? []); + } catch { + // Keep the last good tool list; connection errors surface through the + // next callTool/listResources path. + } + } + + private releaseToolNames(state: ServerState): void { + for (const tool of state.tools) { + this.takenWrapperNames.delete(tool.wrapperName); + } + } + private async closeState(state: ServerState): Promise { const client = state.client; state.client = undefined; diff --git a/packages/sdk/src/mcp/naming.test.ts b/packages/sdk/src/mcp/naming.test.ts new file mode 100644 index 000000000..8369e488f --- /dev/null +++ b/packages/sdk/src/mcp/naming.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, test } from "bun:test"; +import { buildMcpToolName, MAX_MCP_TOOL_NAME_LENGTH } from "./naming.js"; + +describe("MCP tool-name assembly (#326)", () => { + test("normalizes illegal characters and case in both namespaces", () => { + expect(buildMcpToolName("My Server", "Search.Issues!")).toBe("mcp__my-server__search_issues"); + expect(buildMcpToolName("", "")).toBe("mcp__server__tool"); + expect(buildMcpToolName("--weird--", "__tool__")).toBe("mcp__weird__tool"); + }); + + test("clamps oversized identities to the provider-safe length with a hash suffix", () => { + const longServer = "s".repeat(120); + const longTool = "t".repeat(120); + const name = buildMcpToolName(longServer, longTool); + expect(name.length).toBeLessThanOrEqual(MAX_MCP_TOOL_NAME_LENGTH); + // The clamp eats into the joined name and appends a hash suffix. + expect(name).toMatch(/^mcp__[a-z0-9_-]+_[a-z0-9]{6}$/); + + // Deterministic, and distinct identities that truncate to the same prefix + // must not collide. + const repeat = buildMcpToolName(longServer, longTool); + expect(repeat).toBe(name); + const other = buildMcpToolName(longServer, "u".repeat(120)); + expect(other).not.toBe(name); + expect(other.length).toBeLessThanOrEqual(MAX_MCP_TOOL_NAME_LENGTH); + }); +}); diff --git a/packages/sdk/src/mcp/naming.ts b/packages/sdk/src/mcp/naming.ts new file mode 100644 index 000000000..6ab975a3f --- /dev/null +++ b/packages/sdk/src/mcp/naming.ts @@ -0,0 +1,53 @@ +/** + * Shared MCP tool-name assembly. + * + * Every `mcp____` name that reaches a provider API is built + * through these helpers: identifiers are normalized to legal characters and + * clamped to a provider-safe length, so a hostile or sloppy server name can + * never produce a 400 for the whole request (#326). + */ + +export const MAX_MCP_TOOL_NAME_LENGTH = 64; + +export function normalizeMcpServerId(value: string): string { + return value + .trim() + .toLowerCase() + .replace(/[^a-z0-9_-]+/g, '-') + .replace(/-+/g, '-') + .replace(/^-+|-+$/g, '') || 'server'; +} + +export function normalizeMcpToolName(value: string): string { + return value + .trim() + .toLowerCase() + .replace(/[^a-z0-9_-]+/g, '_') + .replace(/_+/g, '_') + .replace(/^_+|_+$/g, '') || 'tool'; +} + +export function shortHash(value: string): string { + let hash = 2166136261; + for (let i = 0; i < value.length; i += 1) { + hash ^= value.charCodeAt(i); + hash = Math.imul(hash, 16777619); + } + return (hash >>> 0).toString(36).padStart(6, '0').slice(0, 6); +} + +/** + * Build the wrapper tool name for an MCP tool. Deterministic; names beyond + * the length cap are disambiguated by a short hash of the full identity. + */ +export function buildMcpToolName(serverName: string, originalToolName: string): string { + const server = normalizeMcpServerId(serverName); + const tool = normalizeMcpToolName(originalToolName); + const joined = `mcp__${server}__${tool}`; + if (joined.length <= MAX_MCP_TOOL_NAME_LENGTH) { + return joined; + } + const suffix = shortHash(`${server}\0${tool}`); + const keep = MAX_MCP_TOOL_NAME_LENGTH - suffix.length - 1; + return `${joined.slice(0, keep)}_${suffix}`; +} From 27528b29dcc62bfec657df8d87e2232d10fa1d34 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 17:48:17 +0800 Subject: [PATCH 2/8] =?UTF-8?q?=F0=9F=90=9B=20fix(sdk):=20SwiftLint=20?= =?UTF-8?q?=E9=80=82=E9=85=8D=E5=99=A8=E6=8C=89=E5=AE=9E=E9=99=85=E8=A7=A3?= =?UTF-8?q?=E6=9E=90=20shell=20=E9=80=89=E6=8B=A9=E8=B0=83=E7=94=A8?= =?UTF-8?q?=E7=AC=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit win32 无条件加 PowerShell "& " 调用符，但 Bash 工具在 win32 优先 走 Git Bash，行首 & 是语法错误，SwiftLint 诊断全量静默丢失。 shellKind 判定从 bash.ts 提升到 utils/shell-invocation 共享导出， 适配器改用与 Bash 工具一致的 resolveShellInvocation+shellKind 判定：仅 PowerShell 方言加 "&"，Git Bash 不加。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/adapters.test.ts | 53 +++++++++++++++++++++- packages/sdk/src/lsp/adapters.ts | 8 +++- packages/sdk/src/tools/bash.ts | 6 +-- packages/sdk/src/utils/shell-invocation.ts | 8 ++++ 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/sdk/src/lsp/adapters.test.ts b/packages/sdk/src/lsp/adapters.test.ts index 3562ef72a..e6c32a980 100644 --- a/packages/sdk/src/lsp/adapters.test.ts +++ b/packages/sdk/src/lsp/adapters.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from 'bun:test' -import { parseSwiftLintDiagnostics } from './adapters.js' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { collectLspAdapterDiagnostics, parseSwiftLintDiagnostics } from './adapters.js' +import { resolveShellInvocation, shellKind } from '../utils/shell-invocation.js' describe('SwiftLint LSP adapter', () => { test('normalizes JSON diagnostics to LSP positions and severities', () => { @@ -41,4 +45,51 @@ describe('SwiftLint LSP adapter', () => { expect(parseSwiftLintDiagnostics('swiftlint: unknown output')).toBeUndefined() expect(parseSwiftLintDiagnostics('{}')).toBeUndefined() }) + + test('shellKind classifies resolved shell executables (#328)', () => { + expect(shellKind('powershell.exe')).toBe('powershell') + expect(shellKind('C:\\Windows\\System32\\WindowsPowerShell\\1.0\\powershell.exe')).toBe('powershell') + expect(shellKind('/usr/bin/pwsh')).toBe('powershell') + expect(shellKind('bash.exe')).toBe('bash') + expect(shellKind('/usr/bin/bash')).toBe('bash') + expect(shellKind('C:\\Program Files\\Git\\bin\\bash.exe')).toBe('bash') + expect(shellKind('sh')).toBe('bash') + }) + + test('picks the PowerShell call operator from the resolved shell, not the platform (#328)', async () => { + const root = await mkdtemp(join(tmpdir(), 'lume-lsp-adapter-')) + try { + await writeFile(join(root, '.swiftlint.yml'), '') + const swiftlint = join(root, process.platform === 'win32' ? 'swiftlint.cmd' : 'swiftlint') + await writeFile(swiftlint, '') + + let captured: string | undefined + const context = { + cwd: root, + toolConfig: {}, + executeNestedTool: async (invocation: { params: { command: string } }) => { + captured = invocation.params.command + return { content: '[]' } + }, + } + const filePath = join(root, 'Sources', 'Demo.swift') + const result = await collectLspAdapterDiagnostics(filePath, context as any) + + expect(result).toBeDefined() + expect(captured).toBeDefined() + const command = captured! + const withoutOperator = command.replace(/^& /, '') + expect(withoutOperator).toBe(`${swiftlint} lint --path ${resolve(filePath)} --quiet --reporter json`) + // The operator decision must match exactly what the Bash tool will + // resolve for this command line: PowerShell dialect gets '& ', Git Bash + // must not (a leading '&' is a syntax error there). + const expectedPowerShell = shellKind(resolveShellInvocation(withoutOperator).command) === 'powershell' + expect(command.startsWith('& ')).toBe(expectedPowerShell) + if (process.platform !== 'win32') { + expect(command.startsWith('& ')).toBe(false) + } + } finally { + await rm(root, { recursive: true, force: true }) + } + }) }) diff --git a/packages/sdk/src/lsp/adapters.ts b/packages/sdk/src/lsp/adapters.ts index a7f1ce973..2948c4a77 100644 --- a/packages/sdk/src/lsp/adapters.ts +++ b/packages/sdk/src/lsp/adapters.ts @@ -1,6 +1,7 @@ import { resolve } from 'node:path' import type { LspDiagnosticBatch, ToolContext } from '../types.js' import { findLspWorkspaceRoot, resolveLspExecutable } from './registry.js' +import { resolveShellInvocation, shellKind } from '../utils/shell-invocation.js' export async function collectLspAdapterDiagnostics( filePath: string, @@ -19,10 +20,15 @@ export async function collectLspAdapterDiagnostics( if (!root) return undefined const command = await resolveLspExecutable(configured.command ?? 'swiftlint', root, configured.cwd) if (!command) return undefined + const cliCommand = `${quote(command)} lint --path ${quote(resolve(filePath))} --quiet --reporter json` + // PowerShell needs an explicit call operator to invoke a quoted path, but a + // leading '&' is a syntax error in Git Bash — pick the operator from the + // shell the Bash tool will actually resolve, not from the platform (#328). + const callOperator = shellKind(resolveShellInvocation(cliCommand).command) === 'powershell' ? '& ' : '' const result = await context.executeNestedTool({ toolName: 'Bash', params: { - command: `${process.platform === 'win32' ? '& ' : ''}${quote(command)} lint --path ${quote(resolve(filePath))} --quiet --reporter json`, + command: `${callOperator}${cliCommand}`, purpose: 'lsp-diagnostics', description: 'SwiftLint diagnostics', }, diff --git a/packages/sdk/src/tools/bash.ts b/packages/sdk/src/tools/bash.ts index 6c9a09835..4daa15b8b 100644 --- a/packages/sdk/src/tools/bash.ts +++ b/packages/sdk/src/tools/bash.ts @@ -21,7 +21,7 @@ import { defineTool } from './types.js' import type { ToolContext, ToolExecutionMetadata, ToolResult } from '../types.js' import { bundledRipgrepDirectory } from '../utils/ripgrep.js' import { analyzeBashCommand } from '../utils/bash-command-analysis.js' -import { resolveShellInvocation } from '../utils/shell-invocation.js' +import { resolveShellInvocation, shellKind } from '../utils/shell-invocation.js' import { spawnWithProcessSandbox, terminateProcessTree } from '../utils/process-sandbox.js' const MAX_OUTPUT_BYTES = 50 * 1024 * 1024 @@ -1092,10 +1092,6 @@ async function readIncrementalFile(path: string, offset: number): Promise<{ chun } } -function shellKind(command: string): 'bash' | 'powershell' { - return /(?:^|[\\/])(?:pwsh|powershell)(?:\.exe)?$/i.test(command) ? 'powershell' : 'bash' -} - function getShellDialectError(command: string, shellCommand: string): string | undefined { if (shellKind(shellCommand) !== 'powershell') return undefined if (/\bcd\s+\/d\b/i.test(command) || /\bfindstr\b[\s\S]*\|\s*head\b/i.test(command)) { diff --git a/packages/sdk/src/utils/shell-invocation.ts b/packages/sdk/src/utils/shell-invocation.ts index 396badbd3..9478c4fc8 100644 --- a/packages/sdk/src/utils/shell-invocation.ts +++ b/packages/sdk/src/utils/shell-invocation.ts @@ -2,6 +2,14 @@ import { spawnSync } from 'node:child_process' let discoveredWindowsBashPath: string | null | undefined +/** + * Classify a resolved shell executable. Shared so every caller that builds + * shell command lines agrees on which dialect will actually run them (#328). + */ +export function shellKind(shellCommand: string): 'bash' | 'powershell' { + return /(?:^|[\\/])(?:pwsh|powershell)(?:\.exe)?$/i.test(shellCommand) ? 'powershell' : 'bash' +} + export function resolveShellInvocation( command: string, platform: NodeJS.Platform = process.platform, From 8617548ec63f858ac8562e4b94c964b95772d781 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 17:50:37 +0800 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=90=9B=20fix(sdk):=20lspmux=20?= =?UTF-8?q?=E6=8E=A2=E6=B5=8B=E7=BC=93=E5=AD=98=E6=8C=89=20cwd=20=E9=94=AE?= =?UTF-8?q?=E6=8E=A7=E5=B9=B6=E7=BC=A9=E7=9F=AD=E6=AD=A3=E5=90=91=20TTL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 探测缓存原为模块级单例：不分 cwd 导致跨工作区误判，且 5 分钟 TTL 内 running 不复检，守护进程启/停双向滞后。改为 Map 按 resolve(cwd) 键控，正向 TTL 压到 30s、负向保 5min；新增 invalidateLspmuxCache，LspClient 启动 mux'd server 失败时主动 失效缓存，下次解析回退直连。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/client.ts | 5 +- packages/sdk/src/lsp/lspmux.test.ts | 166 ++++++++++++++++++++++++++++ packages/sdk/src/lsp/lspmux.ts | 52 +++++++-- 3 files changed, 211 insertions(+), 12 deletions(-) create mode 100644 packages/sdk/src/lsp/lspmux.test.ts diff --git a/packages/sdk/src/lsp/client.ts b/packages/sdk/src/lsp/client.ts index e7f631587..3db2ec8c2 100644 --- a/packages/sdk/src/lsp/client.ts +++ b/packages/sdk/src/lsp/client.ts @@ -10,7 +10,7 @@ import { supportsLspFile, type LspServerRole, } from './registry.js' -import { wrapRustAnalyzerWithLspmux } from './lspmux.js' +import { invalidateLspmuxCache, wrapRustAnalyzerWithLspmux } from './lspmux.js' export interface LspPosition { line: number @@ -682,6 +682,9 @@ export class LspClient { await client.initialize() } catch (error) { killLspProcessTree(child) + // A failed mux'd start must not keep serving a stale "running" probe; + // invalidate so the next resolution falls back to a direct connection (#374). + if (server.lspmux) invalidateLspmuxCache(resolve(server.cwd ?? cwd)) const detail = error instanceof Error ? error.message : String(error) throw new Error(`Unable to start LSP server "${server.command}": ${detail}`) } diff --git a/packages/sdk/src/lsp/lspmux.test.ts b/packages/sdk/src/lsp/lspmux.test.ts new file mode 100644 index 000000000..fa1cc1006 --- /dev/null +++ b/packages/sdk/src/lsp/lspmux.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, test, mock } from 'bun:test' +import { EventEmitter } from 'node:events' +import { chmod, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +const spawnCalls: Array<{ command: string; args: string[] }> = [] +let nextExitCode: number | null = 0 +let nextSpawnError: Error | undefined + +class FakeChild extends EventEmitter { + kill = () => true +} + +function fakeSpawn(command: string, args: string[]): FakeChild { + spawnCalls.push({ command, args }) + const child = new FakeChild() as any + queueMicrotask(() => { + if (nextSpawnError) child.emit('error', nextSpawnError) + else child.emit('exit', nextExitCode) + }) + return child +} + +// Intercept the probe spawn before importing the module under test. +mock.module('node:child_process', () => ({ spawn: fakeSpawn })) + +const { invalidateLspmuxCache, setLspmuxCacheTtls, wrapRustAnalyzerWithLspmux } = await import('./lspmux.js') + +async function makeWorkspace(): Promise { + const root = await mkdtemp(join(tmpdir(), 'lume-lspmux-')) + // A plain file named `lspmux` resolves as the daemon executable; spawn is + // mocked, so no real binary is needed. + const shim = join(root, 'lspmux') + await writeFile(shim, '') + if (process.platform !== 'win32') await chmod(shim, 0o755) + return root +} + +function wrap(root: string) { + return wrapRustAnalyzerWithLspmux({ + command: join(root, 'rust-analyzer'), + args: [], + cwd: root, + enabled: true, + }) +} + +describe('lspmux detection cache (#374)', () => { + test('wraps rust-analyzer when the daemon answers and caches positive probes per cwd', async () => { + const root = await makeWorkspace() + try { + setLspmuxCacheTtls({ positiveMs: 60, negativeMs: 120 }) + nextExitCode = 0 + spawnCalls.length = 0 + invalidateLspmuxCache() + + const wrapped = await wrap(root) + expect(wrapped.lspmux).toBe(true) + expect(wrapped.args).toEqual(['client']) + expect(wrapped.env?.LSPMUX_SERVER).toBe(join(root, 'rust-analyzer')) + expect(spawnCalls).toHaveLength(1) + + // Inside the positive TTL the cached "running" answer is reused. + await wrap(root) + expect(spawnCalls).toHaveLength(1) + } finally { + setLspmuxCacheTtls({ positiveMs: 30_000, negativeMs: 300_000 }) + invalidateLspmuxCache() + await rm(root, { recursive: true, force: true }) + } + }, 10_000) + + test('re-probes once the positive TTL lapses', async () => { + const root = await makeWorkspace() + try { + setLspmuxCacheTtls({ positiveMs: 40, negativeMs: 120_000 }) + nextExitCode = 0 + spawnCalls.length = 0 + invalidateLspmuxCache() + + await wrap(root) + expect(spawnCalls).toHaveLength(1) + await new Promise((resolve) => setTimeout(resolve, 70)) + await wrap(root) + expect(spawnCalls).toHaveLength(2) + } finally { + setLspmuxCacheTtls({ positiveMs: 30_000, negativeMs: 300_000 }) + invalidateLspmuxCache() + await rm(root, { recursive: true, force: true }) + } + }, 10_000) + + test('caches negative results for the longer window and falls back to a direct connection', async () => { + const root = await makeWorkspace() + try { + setLspmuxCacheTtls({ positiveMs: 40, negativeMs: 150 }) + nextExitCode = 1 + spawnCalls.length = 0 + invalidateLspmuxCache() + + const direct = await wrap(root) + expect(direct.lspmux).toBe(false) + expect(direct.command).toBe(join(root, 'rust-analyzer')) + expect(spawnCalls).toHaveLength(1) + + // Still inside the negative TTL: no new probe. + await wrap(root) + expect(spawnCalls).toHaveLength(1) + + await new Promise((resolve) => setTimeout(resolve, 180)) + await wrap(root) + expect(spawnCalls).toHaveLength(2) + } finally { + setLspmuxCacheTtls({ positiveMs: 30_000, negativeMs: 300_000 }) + invalidateLspmuxCache() + await rm(root, { recursive: true, force: true }) + } + }, 10_000) + + test('invalidation forces an immediate re-probe and cwd keys stay independent', async () => { + const rootA = await makeWorkspace() + const rootB = await makeWorkspace() + try { + setLspmuxCacheTtls({ positiveMs: 60_000, negativeMs: 300_000 }) + nextExitCode = 0 + spawnCalls.length = 0 + invalidateLspmuxCache() + + await wrap(rootA) + await wrap(rootB) + expect(spawnCalls).toHaveLength(2) + + // Only A's entry is dropped. + invalidateLspmuxCache(rootA) + await wrap(rootB) + expect(spawnCalls).toHaveLength(2) + await wrap(rootA) + expect(spawnCalls).toHaveLength(3) + } finally { + setLspmuxCacheTtls({ positiveMs: 30_000, negativeMs: 300_000 }) + invalidateLspmuxCache() + await rm(rootA, { recursive: true, force: true }) + await rm(rootB, { recursive: true, force: true }) + } + }, 10_000) + + test('a failing probe counts as not running', async () => { + const root = await makeWorkspace() + try { + setLspmuxCacheTtls({ positiveMs: 60_000, negativeMs: 300_000 }) + nextSpawnError = new Error('spawn failed') + spawnCalls.length = 0 + invalidateLspmuxCache() + + const wrapped = await wrap(root) + expect(wrapped.lspmux).toBe(false) + expect(spawnCalls).toHaveLength(1) + } finally { + nextSpawnError = undefined + setLspmuxCacheTtls({ positiveMs: 30_000, negativeMs: 300_000 }) + invalidateLspmuxCache() + await rm(root, { recursive: true, force: true }) + } + }, 10_000) +}) diff --git a/packages/sdk/src/lsp/lspmux.ts b/packages/sdk/src/lsp/lspmux.ts index 59495ccf5..9bcbf4ba5 100644 --- a/packages/sdk/src/lsp/lspmux.ts +++ b/packages/sdk/src/lsp/lspmux.ts @@ -1,8 +1,34 @@ import { spawn } from 'node:child_process' -import { basename } from 'node:path' +import { basename, resolve } from 'node:path' import { resolveLspExecutable } from './registry.js' -let cached: { checkedAt: number; command?: string; running: boolean } | undefined +interface LspmuxProbe { + checkedAt: number + command?: string + running: boolean +} + +// Positive results expire quickly so a stopped daemon is picked up within +// seconds; negative results stick longer because every probe spawns a +// process. The cache is keyed by cwd — probes are per-workspace, so one +// module-wide entry made daemons in other workspaces appear (or vanish) +// wrongly (#374). +let positiveTtlMs = 30_000 +let negativeTtlMs = 5 * 60_000 +const probes = new Map() + +export function setLspmuxCacheTtls(ttls: { positiveMs?: number; negativeMs?: number }): void { + if (ttls.positiveMs !== undefined) positiveTtlMs = ttls.positiveMs + if (ttls.negativeMs !== undefined) negativeTtlMs = ttls.negativeMs +} + +export function invalidateLspmuxCache(cwd?: string): void { + if (cwd === undefined) { + probes.clear() + } else { + probes.delete(resolve(cwd)) + } +} export async function wrapRustAnalyzerWithLspmux(input: { command: string @@ -24,27 +50,31 @@ export async function wrapRustAnalyzerWithLspmux(input: { } async function detectLspmux(cwd: string): Promise<{ command?: string; running: boolean }> { - if (cached && Date.now() - cached.checkedAt < 5 * 60_000) return cached + const key = resolve(cwd) + const cached = probes.get(key) + if (cached && Date.now() - cached.checkedAt < (cached.running ? positiveTtlMs : negativeTtlMs)) return cached const command = await resolveLspExecutable('lspmux', cwd) if (!command) { - cached = { checkedAt: Date.now(), running: false } - return cached + const next: LspmuxProbe = { checkedAt: Date.now(), running: false } + probes.set(key, next) + return next } - const running = await new Promise((resolve) => { + const running = await new Promise((resolvePromise) => { const child = spawn(command, ['status'], { cwd, windowsHide: true, stdio: 'ignore' }) const timer = setTimeout(() => { child.kill() - resolve(false) + resolvePromise(false) }, 1_000) child.once('error', () => { clearTimeout(timer) - resolve(false) + resolvePromise(false) }) child.once('exit', (code) => { clearTimeout(timer) - resolve(code === 0) + resolvePromise(code === 0) }) }) - cached = { checkedAt: Date.now(), command, running } - return cached + const next: LspmuxProbe = { checkedAt: Date.now(), command, running } + probes.set(key, next) + return next } From 84af941835005294b7276b8121b11225bf6ff540 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 17:51:49 +0800 Subject: [PATCH 4/8] =?UTF-8?q?=F0=9F=90=9B=20fix(sdk):=20LSP=20stdin=20?= =?UTF-8?q?=E5=86=99=E5=85=A5=E5=8A=A0=E8=B6=85=E6=97=B6=E5=B9=B6=E8=A7=A6?= =?UTF-8?q?=E5=8F=91=E6=97=A2=E6=9C=89=E9=87=8D=E5=90=AF=E9=93=BE=E8=B7=AF?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit send() 对 stdin.write 回调无限期等待且全通知链路无超时，LSP 僵死 即 Write/Edit 永久挂起。所有通知路径都汇聚于 send()，在此处统一 加 Promise.race 写超时（默认 10s）并在超时时 fail() 标记客户端 死亡，一处改动覆盖全部通知路径，调用方经 onDead 进入既有重启链。 新增 setLspWriteTimeout 供运行时与测试调整。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/client.test.ts | 49 +++++++++++++++++++++++++++++ packages/sdk/src/lsp/client.ts | 30 +++++++++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/packages/sdk/src/lsp/client.test.ts b/packages/sdk/src/lsp/client.test.ts index a579b99f5..8aa2f9d33 100644 --- a/packages/sdk/src/lsp/client.test.ts +++ b/packages/sdk/src/lsp/client.test.ts @@ -11,6 +11,7 @@ import { LspClient, parseLspMessages, resolveLspServerConfigsForFile, + setLspWriteTimeout, shutdownLspClients, warmupLspClients, } from './client.js' @@ -163,6 +164,54 @@ describe('LSP protocol helpers', () => { ]) }) + test('fails the client when the server never drains stdin writes (#327)', async () => { + setLspWriteTimeout(30) + try { + const client = Object.create(LspClient.prototype) as any + client.writeQueue = Promise.resolve() + client.lastActivity = 0 + client.dead = false + client.initialized = true + client.disposed = false + client.pending = new Map() + let dead = false + client.onDead = () => { dead = true } + // Accepts the write but never invokes the callback: a wedged pipe. + client.process = { stdin: { writable: true, write: () => undefined } } + + await expect(client['send']({ jsonrpc: '2.0', method: 'test/hang' })).rejects.toThrow(/timed out/i) + expect(dead).toBe(true) + expect(client.dead).toBe(true) + } finally { + setLspWriteTimeout(10_000) + } + }) + + test('resolves send once the write callback fires and leaves the client alive (#327)', async () => { + setLspWriteTimeout(30) + try { + const client = Object.create(LspClient.prototype) as any + client.writeQueue = Promise.resolve() + client.lastActivity = 0 + client.dead = false + client.initialized = true + client.disposed = false + client.pending = new Map() + let dead = false + client.onDead = () => { dead = true } + client.process = { stdin: { writable: true, write: (_body: Buffer, callback: () => void) => callback() } } + + await expect(client['send']({ jsonrpc: '2.0', method: 'test/ok' })).resolves.toBeUndefined() + expect(dead).toBe(false) + + // The timer must be released after success rather than firing later. + await new Promise((resolve) => setTimeout(resolve, 60)) + expect(dead).toBe(false) + } finally { + setLspWriteTimeout(10_000) + } + }) + test('spawns .cmd server shims through cmd.exe on Windows', async () => { if (process.platform !== 'win32') return const root = await mkdtemp(join(tmpdir(), 'lume-lsp-cmd-shim-')) diff --git a/packages/sdk/src/lsp/client.ts b/packages/sdk/src/lsp/client.ts index 3db2ec8c2..43a0f596c 100644 --- a/packages/sdk/src/lsp/client.ts +++ b/packages/sdk/src/lsp/client.ts @@ -121,6 +121,13 @@ const failedStarts = new Map() const resolutionCache = new Map }>() let idleTimeoutMs: number | null = 10 * 60_000 let idleChecker: ReturnType | undefined +// Upper bound for one stdin write; a wedged language-server pipe must fail +// the client instead of hanging Write/Edit forever (#327). +let writeTimeoutMs = 10_000 + +export function setLspWriteTimeout(timeoutMs: number): void { + writeTimeoutMs = Math.max(1, timeoutMs) +} export type LspClientState = 'initializing' | 'ready' | 'failed' | 'restarting' | 'disposed' @@ -945,7 +952,28 @@ export class LspClient { }) }) this.writeQueue = write.catch(() => undefined) - return write + // Every notification path funnels through here, so one timeout + fail() + // covers them all: a wedged pipe marks the client dead and routes every + // caller into the existing restart chain instead of hanging forever (#327). + let timer: ReturnType | undefined + return Promise.race([ + write, + new Promise((_, reject) => { + // No unref here: bun's test runner never fires unref'd timers, and + // this one is always released by the settle handlers below anyway. + timer = setTimeout(() => reject(new Error(`LSP server write timed out after ${writeTimeoutMs}ms`)), writeTimeoutMs) + }), + ]).then( + (value) => { + clearTimeout(timer) + return value + }, + (error) => { + clearTimeout(timer) + this.fail(error instanceof Error ? error : new Error(String(error))) + throw error + }, + ) } private onOutput(chunk: Buffer): void { From 03a6792e416a29d866d284a9f190c9538790f8e6 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 17:55:35 +0800 Subject: [PATCH 5/8] =?UTF-8?q?=F0=9F=94=92=20fix(sdk):=20LSP=20server=20s?= =?UTF-8?q?pawn=20=E7=8E=AF=E5=A2=83=E6=94=B9=E6=9C=80=E5=B0=8F=E7=99=BD?= =?UTF-8?q?=E5=90=8D=E5=8D=95=EF=BC=8C=E4=B8=8D=E5=86=8D=E7=BB=A7=E6=89=BF?= =?UTF-8?q?=E5=AE=BF=E4=B8=BB=E5=85=A8=E9=87=8F=E7=8E=AF=E5=A2=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LspClient.start 原样 {...process.env} 全量继承宿主环境，而 lsp.json 由项目侧可控，任意 command 可读取宿主 API key/token（MCP 两路径 早已收口 getDefaultEnvironment，LSP 未同步）。spawn env 改为官方 SDK 最小默认环境 + server.env 显式白名单合并；normalizeServerConfig 补 record.env 透传入口（过滤非字符串值），resolveAvailableServer 对 lspmux 包装 env 改覆盖为合并避免丢失用户配置。测试以注入宿主变量 钉死不泄漏、白名单变量透传、PATH 可用。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/client.test.ts | 65 +++++++++++++++++++++++++++++ packages/sdk/src/lsp/client.ts | 12 +++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/packages/sdk/src/lsp/client.test.ts b/packages/sdk/src/lsp/client.test.ts index 8aa2f9d33..79112cad9 100644 --- a/packages/sdk/src/lsp/client.test.ts +++ b/packages/sdk/src/lsp/client.test.ts @@ -326,6 +326,71 @@ process.stdin.on('data', (chunk) => { } }) + test('spawns servers with the minimal default environment plus explicit env, not the host environment (#380)', async () => { + const root = await mkdtemp(join(tmpdir(), 'lume-lsp-env-')) + const secretName = 'LUME_LSP_ENV_PROBE_SECRET' + const previousSecret = process.env[secretName] + process.env[secretName] = 'top-secret-value' + try { + const script = join(root, 'server.mjs') + const source = join(root, 'index.ts') + await writeFile(join(root, 'package.json'), '{}') + await writeFile(source, '') + await writeFile(script, ` +let buffer = Buffer.alloc(0) +const send = (value) => { + const body = Buffer.from(JSON.stringify(value)) + process.stdout.write(Buffer.concat([Buffer.from('Content-Length: ' + body.length + '\\r\\n\\r\\n'), body])) +} +process.stdin.on('data', (chunk) => { + buffer = Buffer.concat([buffer, chunk]) + while (true) { + const end = buffer.indexOf('\\r\\n\\r\\n') + if (end < 0) return + const match = buffer.subarray(0, end).toString().match(/Content-Length:\\s*(\\d+)/i) + if (!match) process.exit(2) + const message = JSON.parse(buffer.subarray(end + 4, end + 4 + Number(match[1]))) + buffer = buffer.subarray(end + 4 + Number(match[1])) + if (message.method === 'initialize') send({ jsonrpc: '2.0', id: message.id, result: { capabilities: {} } }) + else if (message.method === 'shutdown' || (message.id !== undefined && !message.method)) send({ jsonrpc: '2.0', id: message.id, result: null }) + else if (message.method === 'test/env') { + send({ jsonrpc: '2.0', id: message.id, result: { + secret: process.env.LUME_LSP_ENV_PROBE_SECRET ?? null, + marker: process.env.LUME_LSP_ENV_PROBE_MARKER ?? null, + hasPath: typeof process.env.PATH === 'string' && process.env.PATH.length > 0, + } }) + } + } +}) +`) + const config = { + lsp: { + servers: { + test: { + command: process.execPath, + args: [script], + fileTypes: ['.ts'], + rootMarkers: ['package.json'], + env: { LUME_LSP_ENV_PROBE_MARKER: 'passed-through' }, + }, + }, + }, + } + const client = await getLspClient(root, config, source) + const env = await client.request<{ secret: string | null; marker: string | null; hasPath: boolean }>('test/env', {}) + // Host-only variables must not leak into the project-configured server; + // the explicit per-server env is the opt-in passthrough. + expect(env.secret).toBeNull() + expect(env.marker).toBe('passed-through') + expect(env.hasPath).toBe(true) + } finally { + if (previousSecret === undefined) delete process.env[secretName] + else process.env[secretName] = previousSecret + await shutdownLspClients(root) + await rm(root, { recursive: true, force: true }) + } + }, 20_000) + test('warms up a configured server and never rejects when none matches', async () => { const root = await mkdtemp(join(tmpdir(), 'lume-lsp-warmup-')) const empty = await mkdtemp(join(tmpdir(), 'lume-lsp-warmup-empty-')) diff --git a/packages/sdk/src/lsp/client.ts b/packages/sdk/src/lsp/client.ts index 43a0f596c..a4443fce5 100644 --- a/packages/sdk/src/lsp/client.ts +++ b/packages/sdk/src/lsp/client.ts @@ -410,6 +410,15 @@ function normalizeServerConfig(name: string, value: unknown, workspaceRoot: stri priority: typeof record.priority === 'number' ? record.priority : 0, role: record.role === 'linter' ? 'linter' : record.role === 'primary' ? 'primary' : undefined, adapter: record.adapter === 'swiftlint' ? 'swiftlint' : undefined, + // Explicit per-server environment passthrough; merged over the minimal + // default spawn environment, never the host environment (#380). + ...(record.env && typeof record.env === 'object' && !Array.isArray(record.env) + ? { + env: Object.fromEntries( + Object.entries(record.env).filter((entry): entry is [string, string] => typeof entry[1] === 'string') + ), + } + : {}), ...(typeof record.cwd === 'string' && record.cwd.trim() ? { cwd: resolve(workspaceRoot, record.cwd) } : {}), } } @@ -439,7 +448,8 @@ async function resolveAvailableServer( command: shim?.command ?? wrapped.command, args: shim?.args ?? wrapped.args, cwd: server.cwd ?? root, - ...(wrapped.env ? { env: wrapped.env } : {}), + // Merge instead of replace so configured env survives mux wrapping (#380). + ...(server.env || wrapped.env ? { env: { ...server.env, ...wrapped.env } } : {}), lspmux: wrapped.lspmux, } } From d046e77272c8420cd2c7b565179f99655e0d2a5f Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 18:53:28 +0800 Subject: [PATCH 6/8] =?UTF-8?q?=F0=9F=A7=AA=20fix(sdk):=20=E6=B5=8B?= =?UTF-8?q?=E8=AF=95=E6=94=B9=E6=B3=A8=E5=85=A5=20seam=EF=BC=8C=E7=A7=BB?= =?UTF-8?q?=E9=99=A4=20mock.module=20=E5=85=A8=E8=BF=9B=E7=A8=8B=E6=B1=A1?= =?UTF-8?q?=E6=9F=93?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lspmux 探测 spawn 与 MCP 默认 client 构造器原经 mock.module 整模块 替换，bun test 单进程套件内全局生效且 factory 缺失具名导出，组合跑 时污染后续文件（CI bun 1.3.13 更严必炸）。生产代码零行为变化： lspmux 新增 setLspmuxProbeSpawn、manager 新增 setDefaultMcpSdkClientConstructor 两个模块级可覆盖 seam，测试改注 入 fake。reviewer 五文件组合复跑零串扰。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/lspmux.test.ts | 13 +++++++++---- packages/sdk/src/lsp/lspmux.ts | 20 ++++++++++++++++++-- packages/sdk/src/mcp/default-factory.test.ts | 12 ++++++------ 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/packages/sdk/src/lsp/lspmux.test.ts b/packages/sdk/src/lsp/lspmux.test.ts index fa1cc1006..42f3ddc24 100644 --- a/packages/sdk/src/lsp/lspmux.test.ts +++ b/packages/sdk/src/lsp/lspmux.test.ts @@ -1,8 +1,9 @@ -import { describe, expect, test, mock } from 'bun:test' +import { afterEach, describe, expect, test } from 'bun:test' import { EventEmitter } from 'node:events' import { chmod, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { invalidateLspmuxCache, setLspmuxCacheTtls, setLspmuxProbeSpawn, wrapRustAnalyzerWithLspmux } from './lspmux.js' const spawnCalls: Array<{ command: string; args: string[] }> = [] let nextExitCode: number | null = 0 @@ -22,10 +23,14 @@ function fakeSpawn(command: string, args: string[]): FakeChild { return child } -// Intercept the probe spawn before importing the module under test. -mock.module('node:child_process', () => ({ spawn: fakeSpawn })) +// Injected through the module seam: mocking node:child_process itself would +// pollute every other suite sharing bun's test process. +setLspmuxProbeSpawn(fakeSpawn as any) -const { invalidateLspmuxCache, setLspmuxCacheTtls, wrapRustAnalyzerWithLspmux } = await import('./lspmux.js') +afterEach(() => { + nextExitCode = 0 + nextSpawnError = undefined +}) async function makeWorkspace(): Promise { const root = await mkdtemp(join(tmpdir(), 'lume-lspmux-')) diff --git a/packages/sdk/src/lsp/lspmux.ts b/packages/sdk/src/lsp/lspmux.ts index 9bcbf4ba5..46eb8d514 100644 --- a/packages/sdk/src/lsp/lspmux.ts +++ b/packages/sdk/src/lsp/lspmux.ts @@ -1,4 +1,4 @@ -import { spawn } from 'node:child_process' +import { type ChildProcess, spawn } from 'node:child_process' import { basename, resolve } from 'node:path' import { resolveLspExecutable } from './registry.js' @@ -8,6 +8,22 @@ interface LspmuxProbe { running: boolean } +// Test seam so suites can inject a fake probe process instead of mocking the +// whole node:child_process module (which pollutes every other test in the +// shared bun test process). +type ProbeSpawn = ( + command: string, + args: string[], + options: { cwd: string } +) => Pick +const defaultProbeSpawn: ProbeSpawn = (command, args, options) => + spawn(command, args, { cwd: options.cwd, windowsHide: true, stdio: 'ignore' }) +let probeSpawn: ProbeSpawn = defaultProbeSpawn + +export function setLspmuxProbeSpawn(impl: ProbeSpawn | undefined): void { + probeSpawn = impl ?? defaultProbeSpawn +} + // Positive results expire quickly so a stopped daemon is picked up within // seconds; negative results stick longer because every probe spawns a // process. The cache is keyed by cwd — probes are per-workspace, so one @@ -60,7 +76,7 @@ async function detectLspmux(cwd: string): Promise<{ command?: string; running: b return next } const running = await new Promise((resolvePromise) => { - const child = spawn(command, ['status'], { cwd, windowsHide: true, stdio: 'ignore' }) + const child = probeSpawn(command, ['status'], { cwd }) const timer = setTimeout(() => { child.kill() resolvePromise(false) diff --git a/packages/sdk/src/mcp/default-factory.test.ts b/packages/sdk/src/mcp/default-factory.test.ts index 03d5d9b8c..1be68f2d6 100644 --- a/packages/sdk/src/mcp/default-factory.test.ts +++ b/packages/sdk/src/mcp/default-factory.test.ts @@ -1,9 +1,9 @@ -import { describe, expect, test, mock } from "bun:test"; -import { McpClientManager } from "./manager.js"; +import { describe, expect, test } from "bun:test"; +import { McpClientManager, setDefaultMcpSdkClientConstructor } from "./manager.js"; -// Intercept the lazily-imported official Client so the test can observe the -// constructor options the default factory passes (the listChanged wiring is -// exactly what issue #384 requires) and drive its onChanged callback. +// Injected through the module seam instead of mock.module: replacing the +// whole SDK client module would leak into every other suite sharing bun's +// single test process. class FakeMcpClient { static instances: FakeMcpClient[] = []; tools: Array<{ name: string; inputSchema?: unknown }> = []; @@ -33,7 +33,7 @@ class FakeMcpClient { } } -mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({ Client: FakeMcpClient })); +setDefaultMcpSdkClientConstructor(FakeMcpClient as any); const config = { enabled: true, transport: "streamable_http", url: "http://127.0.0.1:8787/mcp" } as const; From ad859c8ad96775aaec6d5acfa058a43988e581dc Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 20:53:54 +0800 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=A7=AA=20test(sdk):=20force=20?= =?UTF-8?q?=E6=97=81=E8=B7=AF=E6=B5=8B=E8=AF=95=E9=80=82=E9=85=8D=20#437?= =?UTF-8?q?=20=E8=90=BD=E5=9C=B0=E7=9A=84=20failureRetry*=20=E9=80=89?= =?UTF-8?q?=E9=A1=B9=E5=90=8D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #312 退避已由 main 以 failureRetryBaseMs/MaxMs + nextRetryAt 实现， 本分支原退避时序断言与之重复，收敛为单个 force 旁路测试并换用 main 的选项名。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/mcp/manager.test.ts | 40 +++++----------------------- 1 file changed, 6 insertions(+), 34 deletions(-) diff --git a/packages/sdk/src/mcp/manager.test.ts b/packages/sdk/src/mcp/manager.test.ts index 57626d78f..8d74d4416 100644 --- a/packages/sdk/src/mcp/manager.test.ts +++ b/packages/sdk/src/mcp/manager.test.ts @@ -480,7 +480,7 @@ describe("McpClientManager", () => { expect(new Set([...githubNames, ...reconnectNames]).size).toBe(2); }); - test("fast-fails during reconnect backoff and honors force (#312)", async () => { + test("manual probes bypass the reconnect backoff gate via force (#312)", async () => { let factoryCalls = 0; const manager = new McpClientManager({ clientFactory: () => { @@ -488,8 +488,8 @@ describe("McpClientManager", () => { throw new Error("Connection refused"); }, transportFactory: fakeTransportFactory, - defaultReconnectBackoffBaseMs: 50, - defaultReconnectBackoffMaxMs: 10_000, + failureRetryBaseMs: 60_000, + failureRetryMaxMs: 60_000, }); manager.sync({ dead: { enabled: true, transport: "stdio", command: "node" } }); @@ -497,38 +497,10 @@ describe("McpClientManager", () => { await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); expect(factoryCalls).toBe(1); - // Manual probes bypass the negative cache. - await expect(manager.ensureConnected("dead", { force: true })).rejects.toMatchObject({ code: "transport_error" }); - expect(factoryCalls).toBe(2); - - // Second consecutive failure doubles the window: past the base window but - // still inside the doubled one. - await delay(70); - await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); - expect(factoryCalls).toBe(2); - - // Past the doubled window the gate opens again. - await delay(120); - await expect(manager.ensureConnected("dead")).rejects.toMatchObject({ code: "transport_error" }); - expect(factoryCalls).toBe(3); - }); - - test("explicit connect accepts the same force option (#312)", async () => { - let factoryCalls = 0; - const manager = new McpClientManager({ - clientFactory: () => { - factoryCalls += 1; - throw new Error("Connection refused"); - }, - transportFactory: fakeTransportFactory, - defaultReconnectBackoffBaseMs: 60_000, - defaultReconnectBackoffMaxMs: 60_000, - }); - - manager.sync({ dead: { enabled: true, transport: "stdio", command: "node" } }); - await expect(manager.connect("dead")).rejects.toThrow(); + // Both entry points accept the same force option. await expect(manager.connect("dead", { force: true })).rejects.toThrow(); - expect(factoryCalls).toBe(2); + await expect(manager.ensureConnected("dead", { force: true })).rejects.toThrow(); + expect(factoryCalls).toBe(3); }); }); From cde1210b07c665e90888d30e619921abd8d32d9e Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 22 Aug 2026 22:00:18 +0800 Subject: [PATCH 8/8] =?UTF-8?q?=F0=9F=A7=AA=20test(sdk):=20SwiftLint=20?= =?UTF-8?q?=E8=B0=83=E7=94=A8=E7=AC=A6=E6=B5=8B=E8=AF=95=E9=92=89=E6=AD=BB?= =?UTF-8?q?=E5=8F=AF=E6=89=A7=E8=A1=8C=E6=96=87=E4=BB=B6=E5=89=8D=E6=8F=90?= =?UTF-8?q?=EF=BC=8C=E4=BF=AE=E5=A4=8D=20Linux=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit collectLspAdapterDiagnostics 依赖 resolveLspExecutable 找到 swiftlint：POSIX 下探针要求 X_OK，测试只 writeFile 未 chmod，CI Linux 上解析失败静默返回 undefined；且裸名解析会兜底扫宿主 PATH， 结果不确定。改为显式 toolConfig 注入绝对路径 + POSIX 补 chmod。 Co-Authored-By: Claude Fable 5 --- packages/sdk/src/lsp/adapters.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/sdk/src/lsp/adapters.test.ts b/packages/sdk/src/lsp/adapters.test.ts index e6c32a980..e92594140 100644 --- a/packages/sdk/src/lsp/adapters.test.ts +++ b/packages/sdk/src/lsp/adapters.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { chmod, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' import { collectLspAdapterDiagnostics, parseSwiftLintDiagnostics } from './adapters.js' @@ -62,11 +62,16 @@ describe('SwiftLint LSP adapter', () => { await writeFile(join(root, '.swiftlint.yml'), '') const swiftlint = join(root, process.platform === 'win32' ? 'swiftlint.cmd' : 'swiftlint') await writeFile(swiftlint, '') + // resolveLspExecutable requires the exec bit on POSIX; without it the + // adapter silently finds no swiftlint and returns undefined. + if (process.platform !== 'win32') await chmod(swiftlint, 0o755) let captured: string | undefined const context = { cwd: root, - toolConfig: {}, + // Pin the executable explicitly so resolution never falls through to + // a real swiftlint installed on the machine's PATH. + toolConfig: { lsp: { servers: { swiftlint: { command: swiftlint } } } }, executeNestedTool: async (invocation: { params: { command: string } }) => { captured = invocation.params.command return { content: '[]' }