-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
fix(mcp): fix .mcp.json, timeouts, txt outline, subprocess leaks, version pin, proxy stripping #2066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(mcp): fix .mcp.json, timeouts, txt outline, subprocess leaks, version pin, proxy stripping #2066
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,9 @@ | ||
| { | ||
| "mcpServers": {} | ||
| "mcpServers": { | ||
| "mcp-search": { | ||
| "type": "stdio", | ||
| "command": "bun", | ||
| "args": ["${CLAUDE_PLUGIN_ROOT}/scripts/mcp-server.cjs"] | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -849,8 +849,57 @@ export function parseFile(content: string, filePath: string, projectRoot?: strin | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| const grammarPath = resolveGrammarPathWithFallback(language, projectRoot); | ||||||||||||||||||||||
| if (!grammarPath) { | ||||||||||||||||||||||
| // No tree-sitter grammar available — produce a basic fallback outline | ||||||||||||||||||||||
| // by scanning for lines that look like headers (e.g., Markdown-style #, | ||||||||||||||||||||||
| // ALL-CAPS lines, or lines ending with a colon that resemble section titles). | ||||||||||||||||||||||
| const fallbackSymbols: CodeSymbol[] = []; | ||||||||||||||||||||||
| for (let i = 0; i < lines.length; i++) { | ||||||||||||||||||||||
| const line = lines[i]; | ||||||||||||||||||||||
| const trimmed = line.trim(); | ||||||||||||||||||||||
| if (!trimmed) continue; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Markdown-style headers | ||||||||||||||||||||||
| const headerMatch = trimmed.match(/^(#{1,6})\s+(.+)/); | ||||||||||||||||||||||
| if (headerMatch) { | ||||||||||||||||||||||
| fallbackSymbols.push({ | ||||||||||||||||||||||
| name: headerMatch[2], | ||||||||||||||||||||||
| kind: "section", | ||||||||||||||||||||||
| signature: trimmed, | ||||||||||||||||||||||
| lineStart: i + 1, | ||||||||||||||||||||||
| lineEnd: i + 1, | ||||||||||||||||||||||
| exported: false, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // ALL-CAPS lines (likely section headers in plain text) | ||||||||||||||||||||||
| if (trimmed.length >= 3 && trimmed.length <= 80 && /^[A-Z][A-Z0-9 _\-]+$/.test(trimmed)) { | ||||||||||||||||||||||
| fallbackSymbols.push({ | ||||||||||||||||||||||
| name: trimmed, | ||||||||||||||||||||||
| kind: "section", | ||||||||||||||||||||||
| signature: trimmed, | ||||||||||||||||||||||
| lineStart: i + 1, | ||||||||||||||||||||||
| lineEnd: i + 1, | ||||||||||||||||||||||
| exported: false, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Lines ending with colon (section-title style) | ||||||||||||||||||||||
| if (trimmed.endsWith(":") && trimmed.length >= 3 && trimmed.length <= 80 && !trimmed.includes(" ")) { | ||||||||||||||||||||||
| fallbackSymbols.push({ | ||||||||||||||||||||||
| name: trimmed.slice(0, -1), | ||||||||||||||||||||||
| kind: "section", | ||||||||||||||||||||||
| signature: trimmed, | ||||||||||||||||||||||
| lineStart: i + 1, | ||||||||||||||||||||||
| lineEnd: i + 1, | ||||||||||||||||||||||
| exported: false, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return { | ||||||||||||||||||||||
| filePath, language, symbols: [], imports: [], | ||||||||||||||||||||||
| filePath, language, symbols: fallbackSymbols, imports: [], | ||||||||||||||||||||||
| totalLines: lines.length, foldedTokenEstimate: 50, | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
Comment on lines
901
to
904
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fallback path returns a hardcoded
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -907,13 +956,10 @@ export function parseFilesBatch( | |||||||||||||||||||||
| for (const [language, groupFiles] of languageGroups) { | ||||||||||||||||||||||
| const grammarPath = resolveGrammarPathWithFallback(language, projectRoot); | ||||||||||||||||||||||
| if (!grammarPath) { | ||||||||||||||||||||||
| // No grammar — return empty results for these files | ||||||||||||||||||||||
| // No tree-sitter grammar — use fallback parser for these files | ||||||||||||||||||||||
| for (const file of groupFiles) { | ||||||||||||||||||||||
| const lines = file.content.split("\n"); | ||||||||||||||||||||||
| results.set(file.relativePath, { | ||||||||||||||||||||||
| filePath: file.relativePath, language, symbols: [], imports: [], | ||||||||||||||||||||||
| totalLines: lines.length, foldedTokenEstimate: 50, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| const fallbackResult = parseFile(file.content, file.relativePath, projectRoot); | ||||||||||||||||||||||
| results.set(file.relativePath, fallbackResult); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,7 +207,7 @@ export class ChromaMcpManager { | |
|
|
||
| const args = [ | ||
| '--python', pythonVersion, | ||
| 'chroma-mcp', | ||
| 'chroma-mcp==0.2.3', | ||
| '--client-type', 'http', | ||
| '--host', chromaHost, | ||
| '--port', chromaPort | ||
|
|
@@ -233,7 +233,7 @@ export class ChromaMcpManager { | |
| // Local mode: persistent client with data directory | ||
| return [ | ||
| '--python', pythonVersion, | ||
| 'chroma-mcp', | ||
| 'chroma-mcp==0.2.3', | ||
| '--client-type', 'persistent', | ||
| '--data-dir', DEFAULT_CHROMA_DATA_DIR.replace(/\\/g, '/') | ||
| ]; | ||
|
|
@@ -264,9 +264,14 @@ export class ChromaMcpManager { | |
| // Transport error: chroma-mcp subprocess likely died (e.g., killed by orphan reaper, | ||
| // HNSW index corruption). Mark connection dead and retry once after reconnect (#1131). | ||
| // Without this retry, callers see a one-shot error even though reconnect would succeed. | ||
| // IMPORTANT: Close transport BEFORE nulling to prevent subprocess leaks (#1925). | ||
| this.connected = false; | ||
| const staleTransport = this.transport; | ||
| const staleClient = this.client; | ||
| this.client = null; | ||
| this.transport = null; | ||
| try { if (staleTransport) await staleTransport.close(); } catch { /* already dead */ } | ||
| try { if (staleClient) await staleClient.close(); } catch { /* already dead */ } | ||
|
|
||
| logger.warn('CHROMA_MCP', `Transport error during "${toolName}", reconnecting and retrying once`, { | ||
| error: transportError instanceof Error ? transportError.message : String(transportError) | ||
|
|
@@ -354,6 +359,26 @@ export class ChromaMcpManager { | |
| logger.info('CHROMA_MCP', 'chroma-mcp MCP connection stopped'); | ||
| } | ||
|
|
||
| /** | ||
| * Synchronous shutdown — kills the chroma-mcp subprocess immediately. | ||
| * Called from process 'exit' handler where async operations are not possible. | ||
| * Ensures no orphaned chroma-mcp processes survive the worker. | ||
| */ | ||
| shutdownSync(): void { | ||
| const transportRef = this.transport as unknown as { _process?: import('child_process').ChildProcess } | null; | ||
| const childProcess = transportRef?._process; | ||
|
Comment on lines
+368
to
+369
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if (!childProcess) {
logger.warn('CHROMA_MCP', 'shutdownSync: _process not available on transport (MCP SDK may have changed)');
} |
||
| if (childProcess?.pid && !childProcess.killed) { | ||
| try { | ||
| childProcess.kill('SIGKILL'); | ||
| logger.info('CHROMA_MCP', 'Killed chroma-mcp subprocess on shutdown', { pid: childProcess.pid }); | ||
| } catch { /* best effort */ } | ||
| } | ||
| this.client = null; | ||
| this.transport = null; | ||
| this.connected = false; | ||
| this.connecting = null; | ||
| } | ||
|
|
||
| /** | ||
| * Reset the singleton instance (for testing). | ||
| * Awaits stop() to prevent dual subprocesses. | ||
|
|
@@ -434,15 +459,26 @@ export class ChromaMcpManager { | |
| } | ||
| } | ||
|
|
||
| /** Proxy env var names to strip -- chroma-mcp uses local persistent storage | ||
| * and doesn't need network proxies. SOCKS5 proxies in particular break the | ||
| * stdio handshake by interfering with Python's HTTP client initialization. */ | ||
| private static readonly PROXY_ENV_VARS_TO_STRIP = new Set([ | ||
| 'ALL_PROXY', 'all_proxy', | ||
| 'HTTP_PROXY', 'http_proxy', | ||
| 'HTTPS_PROXY', 'https_proxy', | ||
| 'NO_PROXY', 'no_proxy', | ||
| ]); | ||
|
|
||
| /** | ||
| * Build subprocess environment with SSL certificate overrides for enterprise proxy compatibility. | ||
| * Strips proxy env vars that break chroma-mcp's stdio handshake (#1927). | ||
| * If a combined cert bundle exists (Zscaler), injects SSL_CERT_FILE, REQUESTS_CA_BUNDLE, etc. | ||
| * Otherwise returns a plain string-keyed copy of process.env. | ||
| */ | ||
| private getSpawnEnv(): Record<string, string> { | ||
| const baseEnv: Record<string, string> = {}; | ||
| for (const [key, value] of Object.entries(sanitizeEnv(process.env))) { | ||
| if (value !== undefined) { | ||
| if (value !== undefined && !ChromaMcpManager.PROXY_ENV_VARS_TO_STRIP.has(key)) { | ||
| baseEnv[key] = value; | ||
| } | ||
| } | ||
|
|
@@ -482,3 +518,12 @@ export class ChromaMcpManager { | |
| }); | ||
| } | ||
| } | ||
|
|
||
| // Safety net: kill chroma-mcp subprocess on process exit to prevent orphans. | ||
| // The 'exit' event is the last chance to clean up — only synchronous code runs here. | ||
| process.on('exit', () => { | ||
| const instance = ChromaMcpManager['instance']; | ||
| if (instance) { | ||
| instance.shutdownSync(); | ||
| } | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLAUDE_PLUGIN_ROOTis unset"${CLAUDE_PLUGIN_ROOT}/scripts/mcp-server.cjs"relies on Claude Code expanding the env var at load time. Every shell command inhooks.jsonhas an explicit fallback chain ([ -z "$_R" ] && _R=$(ls -dt ...)) for the case whereCLAUDE_PLUGIN_ROOTisn't populated. This MCP entry has no equivalent fallback, so if the variable is absent the literal string is passed as the executable path and the MCP server silently fails to start. Documenting the requirement or adding a wrapper script with fallback logic (matching the hooks pattern) would make the registration more robust.