fix(mcp): handle server names with spaces in mcp_discover (#3037)
getServerConfig now trims whitespace and performs case-insensitive matching so that names like "langgraph Code" resolve correctly. getOrConnect uses config.name as the canonical cache key to prevent duplicate connections from variant casing. Closes #3029 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
74c585074f
commit
47c1b9cd7f
3 changed files with 69 additions and 8 deletions
|
|
@ -54,7 +54,7 @@
|
|||
"copy-themes": "node scripts/copy-themes.cjs",
|
||||
"copy-export-html": "node scripts/copy-export-html.cjs",
|
||||
"test:compile": "node scripts/compile-tests.mjs",
|
||||
"test:unit": "npm run test:compile && node --import ./scripts/dist-test-resolve.mjs --experimental-test-isolation=process --test-reporter=./scripts/test-reporter-compact.mjs --test 'dist-test/src/tests/*.test.js' 'dist-test/src/resources/extensions/gsd/tests/*.test.js' 'dist-test/src/resources/extensions/gsd/tests/*.test.mjs' 'dist-test/src/resources/extensions/shared/tests/*.test.js' 'dist-test/src/resources/extensions/claude-code-cli/tests/*.test.js' 'dist-test/src/resources/extensions/github-sync/tests/*.test.js' 'dist-test/src/resources/extensions/universal-config/tests/*.test.js' 'dist-test/src/resources/extensions/voice/tests/*.test.js'",
|
||||
"test:unit": "npm run test:compile && node --import ./scripts/dist-test-resolve.mjs --experimental-test-isolation=process --test-reporter=./scripts/test-reporter-compact.mjs --test 'dist-test/src/tests/*.test.js' 'dist-test/src/resources/extensions/gsd/tests/*.test.js' 'dist-test/src/resources/extensions/gsd/tests/*.test.mjs' 'dist-test/src/resources/extensions/shared/tests/*.test.js' 'dist-test/src/resources/extensions/claude-code-cli/tests/*.test.js' 'dist-test/src/resources/extensions/github-sync/tests/*.test.js' 'dist-test/src/resources/extensions/universal-config/tests/*.test.js' 'dist-test/src/resources/extensions/voice/tests/*.test.js' 'dist-test/src/resources/extensions/mcp-client/tests/*.test.js'",
|
||||
"test:packages": "node --test packages/pi-coding-agent/dist/core/*.test.js",
|
||||
"test:marketplace": "GSD_TEST_CLONE_MARKETPLACES=1 node --import ./src/resources/extensions/gsd/tests/resolve-ts.mjs --experimental-strip-types --test src/resources/extensions/gsd/tests/claude-import-tui.test.ts src/resources/extensions/gsd/tests/plugin-importer-live.test.ts src/tests/marketplace-discovery.test.ts",
|
||||
"test:coverage": "c8 --reporter=text --reporter=lcov --exclude='src/resources/extensions/gsd/tests/**' --exclude='src/tests/**' --exclude='scripts/**' --exclude='native/**' --exclude='node_modules/**' --check-coverage --statements=40 --lines=40 --branches=20 --functions=20 node --import ./src/resources/extensions/gsd/tests/resolve-ts.mjs --experimental-strip-types --experimental-test-isolation=process --test src/resources/extensions/gsd/tests/*.test.ts src/resources/extensions/gsd/tests/*.test.mjs src/tests/*.test.ts src/resources/extensions/shared/tests/*.test.ts",
|
||||
|
|
|
|||
|
|
@ -111,7 +111,11 @@ function readConfigs(): McpServerConfig[] {
|
|||
}
|
||||
|
||||
function getServerConfig(name: string): McpServerConfig | undefined {
|
||||
return readConfigs().find((s) => s.name === name);
|
||||
const trimmed = name.trim();
|
||||
return readConfigs().find((s) =>
|
||||
s.name === trimmed ||
|
||||
s.name.toLowerCase() === trimmed.toLowerCase(),
|
||||
);
|
||||
}
|
||||
|
||||
/** Resolve ${VAR} references in env values against process.env. */
|
||||
|
|
@ -131,12 +135,14 @@ function resolveEnv(env: Record<string, string>): Record<string, string> {
|
|||
}
|
||||
|
||||
async function getOrConnect(name: string, signal?: AbortSignal): Promise<Client> {
|
||||
const existing = connections.get(name);
|
||||
if (existing) return existing.client;
|
||||
|
||||
const config = getServerConfig(name);
|
||||
if (!config) throw new Error(`Unknown MCP server: "${name}". Use mcp_servers to list available servers.`);
|
||||
|
||||
// Always use config.name as the canonical cache key so that variant
|
||||
// casing / whitespace still hits the same connection.
|
||||
const existing = connections.get(config.name);
|
||||
if (existing) return existing.client;
|
||||
|
||||
const client = new Client({ name: "gsd", version: "1.0.0" });
|
||||
let transport: StdioClientTransport | StreamableHTTPClientTransport;
|
||||
|
||||
|
|
@ -151,15 +157,15 @@ async function getOrConnect(name: string, signal?: AbortSignal): Promise<Client>
|
|||
} else if (config.transport === "http" && config.url) {
|
||||
const resolvedUrl = config.url.replace(
|
||||
/\$\{([^}]+)\}/g,
|
||||
(_, name) => process.env[name] ?? "",
|
||||
(_, varName) => process.env[varName] ?? "",
|
||||
);
|
||||
transport = new StreamableHTTPClientTransport(new URL(resolvedUrl));
|
||||
} else {
|
||||
throw new Error(`Server "${name}" has unsupported transport: ${config.transport}`);
|
||||
throw new Error(`Server "${config.name}" has unsupported transport: ${config.transport}`);
|
||||
}
|
||||
|
||||
await client.connect(transport, { signal, timeout: 30000 });
|
||||
connections.set(name, { client, transport });
|
||||
connections.set(config.name, { client, transport });
|
||||
return client;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,55 @@
|
|||
/**
|
||||
* Regression test for #3029 — mcp_discover fails for server names with spaces.
|
||||
*
|
||||
* The getServerConfig lookup must handle:
|
||||
* 1. Exact match (already works)
|
||||
* 2. Names with leading/trailing whitespace (trimming)
|
||||
* 3. Case-insensitive matching (e.g. "Langgraph code" vs "langgraph Code")
|
||||
*
|
||||
* We test at the source level since getServerConfig is not exported.
|
||||
*/
|
||||
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { dirname, join } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const __filename = fileURLToPath(import.meta.url);
|
||||
const __dirname = dirname(__filename);
|
||||
|
||||
const source = readFileSync(join(__dirname, "..", "index.ts"), "utf-8");
|
||||
|
||||
test("#3029: getServerConfig trims whitespace from input name", () => {
|
||||
assert.ok(
|
||||
source.includes(".trim()"),
|
||||
"getServerConfig should trim the input name before comparison",
|
||||
);
|
||||
});
|
||||
|
||||
test("#3029: getServerConfig performs case-insensitive matching", () => {
|
||||
assert.ok(
|
||||
source.includes(".toLowerCase()"),
|
||||
"getServerConfig should compare names case-insensitively",
|
||||
);
|
||||
});
|
||||
|
||||
test("#3029: getOrConnect normalizes name for connection cache lookup", () => {
|
||||
// The connections Map key must use the canonical (config) name, not the
|
||||
// raw user input, so that subsequent lookups hit the cache even when the
|
||||
// user's casing differs.
|
||||
const getOrConnectMatch = source.match(
|
||||
/async function getOrConnect\(name: string[\s\S]*?const existing = connections\.get\(/,
|
||||
);
|
||||
assert.ok(
|
||||
getOrConnectMatch,
|
||||
"getOrConnect function should exist",
|
||||
);
|
||||
// After the fix, getOrConnect should normalize the name via getServerConfig
|
||||
// or use config.name as the canonical cache key.
|
||||
assert.ok(
|
||||
source.includes("connections.get(config.name") ||
|
||||
source.includes("connections.set(config.name"),
|
||||
"getOrConnect should use config.name (canonical) as the connections cache key",
|
||||
);
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue