fix(mcp): fix .mcp.json, timeouts, txt outline, subprocess leaks, version pin, proxy stripping

[thedotmack]

Summary

• Fix [BUG] Root .mcp.json is empty, mcp-search never registers #1921: Populate root .mcp.json with mcp-search server declaration
• Fix [BUG] MCP server uses 3s health-check timeout for corpus prime/query calls #1922: Use appropriate timeouts for corpus operations instead of 3s
• Fix [BUG] smart_outline returns empty output for .txt files with no tree-sitter grammar #1923: Add fallback outline for files without tree-sitter grammar
• Fix [BUG] chroma-mcp child process not terminated when Claude Code exits #1924: Kill chroma-mcp on worker shutdown
• Fix [BUG] chroma-mcp subprocess leak via null-before-close on transport error #1925: Fix transport null-before-close causing subprocess leaks
• Fix [BUG] chroma-mcp stdio handshake broken across all available versions #1926: Pin chroma-mcp to compatible version
• Fix [BUG] chroma-mcp fails on WSL/Linux with ALL_PROXY=socks5 #1927: Strip proxy env vars from chroma-mcp env

Test plan

• Verify mcp-search registers
• Verify corpus operations don't timeout at 3s
• Verify .txt files get basic outline
• Verify chroma-mcp killed on shutdown
• Verify no orphan processes after reconnect
• Verify proxy vars stripped

Closes #1921, closes #1922, closes #1923, closes #1924, closes #1925, closes #1926, closes #1927

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@thedotmack has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 54 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 54 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3e4b861-93ac-436e-8d79-80863f6ddf08

📥 Commits

Reviewing files that changed from the base of the PR and between beea789 and 22cd5cc.

📒 Files selected for processing (4)

• .mcp.json
• src/servers/mcp-server.ts
• src/services/smart-file-read/parser.ts
• src/services/sync/ChromaMcpManager.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix/mcp-bugs

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review

Good set of targeted bug fixes addressing real operational issues. Overall the changes are well-scoped. A few things worth discussing:

---

.mcp.json — ${CLAUDE_PLUGIN_ROOT} expansion

The new entry relies on ${CLAUDE_PLUGIN_ROOT} being interpolated at runtime by Claude Code. Worth confirming this variable is reliably set in all environments (installed via marketplace vs. dev clone). If it's unset the path becomes a literal string and the server silently fails to register.

---

ChromaMcpManager.ts

Hard-coded version in two places

'chroma-mcp==0.2.3',  // line 210 and 236

These two sites will drift. A named constant (e.g. private static readonly CHROMA_MCP_VERSION = '0.2.3') would prevent that.

Fragile internal cast in shutdownSync

const transportRef = this.transport as unknown as { _process?: import('child_process').ChildProcess } | null;

This depends on a private _process field in the MCP SDK's transport implementation. If the SDK renames or removes it, childProcess is silently undefined and the orphan kill is skipped with no indication. Consider logging a warning when childProcess is not found, so the failure surface is visible. Alternatively, a comment noting the SDK version this was verified against would help future maintainers.

Double-close ordering

try { if (staleTransport) await staleTransport.close(); } catch { /* already dead */ }
try { if (staleClient) await staleClient.close(); } catch { /* already dead */ }

Most MCP client implementations close the transport internally when the client is closed. Calling transport.close() first and then client.close() is safe given the try/catch wrapping, but may produce unnecessary noise in error logs. Consider closing only the client and letting it own the transport lifecycle — or document why both are needed.

process.on('exit') registered at module import time

process.on('exit', () => {
  const instance = ChromaMcpManager['instance'];
  ...
});

This runs as a module-level side effect, so importing the module in a test registers the handler. If the module is imported more than once (e.g. via require cache busting in tests), multiple handlers register and shutdownSync is called multiple times. Since shutdownSync nulls this.client and this.transport on first call it's idempotent, but worth noting. Also: ChromaMcpManager['instance'] bypasses TypeScript's private visibility — a static getter or a module-level exported helper would be cleaner.

connecting nulled in shutdownSync

this.connecting = null;

Assuming connecting is Promise<void> | null, this is fine. Just confirming there's no race where the in-flight connecting promise resolves after shutdownSync and re-sets connected = true.

---

parser.ts — Fallback outline heuristics

The ALL-CAPS regex:

/^[A-Z][A-Z0-9 _\-]+$/

…matches things like "THE QUICK BROWN FOX JUMPED" in prose .txt files. For files that are genuinely plain text (README-style), this may over-report. Worth capping the match count or only applying these heuristics to known plain-text extensions (.txt, .rst, .md) rather than all files without a grammar.

The colon-ending heuristic:

if (trimmed.endsWith(":") && trimmed.length >= 3 && trimmed.length <= 80 && !trimmed.includes("  "))

!trimmed.includes(" ") (two spaces) is a fragile signal — common in formatted text. Using !trimmed.includes(" ") (any space) or checking word count might be more robust for distinguishing section titles from prose.

foldedTokenEstimate: 50 remains hardcoded even when symbols are found. If the fallback outline has, say, 30 symbols, the estimate should reflect that.

---

mcp-server.ts — Magic timeout numbers

callWorkerAPIPost('/api/corpus', args, 120000);
callWorkerAPIPost(`/api/corpus/${...}/prime`, rest, 60000);

Inline millisecond values make the intent opaque at a glance. Named constants at the top of the file (CORPUS_CREATE_TIMEOUT_MS, CORPUS_QUERY_TIMEOUT_MS, etc.) would make the choices self-documenting and easier to tune.

---

Summary

Area	Status
.mcp.json registration	✅ Correct, needs env-var resilience check
Timeout plumbing	✅ Clean, suggest named constants
Fallback outline	✅ Useful, heuristics could be tightened
Transport leak fix	✅ Correct ordering
shutdownSync / exit handler	✅ Works, fragile SDK cast worth documenting
Version pinning	✅ Correct, extract to constant
Proxy stripping	✅ Well-justified

The core logic is sound — these are mostly polish/robustness suggestions. The subprocess leak fix and proxy stripping in particular look correct and well-reasoned.

[greptile-apps]

Greptile Summary

This PR addresses seven bug fixes: registering the mcp-search server in .mcp.json, extending corpus operation timeouts, adding a heuristic fallback outline for grammar-less files, killing the chroma-mcp subprocess on worker shutdown, closing the transport before nulling to prevent subprocess leaks, pinning chroma-mcp to 0.2.3, and stripping proxy env vars that break the stdio handshake. All fixes are targeted and consistent with existing patterns in the codebase.

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 style/robustness suggestions that do not block correctness.

No P0 or P1 issues found. The three P2 comments cover: a fragile private-API cast consistent with existing code, a fixed token estimate that matches prior behavior, and a missing env-var fallback in the MCP config. None cause incorrect behavior today.

.mcp.json warrants a quick sanity-check that CLAUDE_PLUGIN_ROOT is reliably set in all deployment scenarios before this ships.

Important Files Changed

Filename	Overview
.mcp.json	Populates root .mcp.json with mcp-search server declaration using bun; relies on ${CLAUDE_PLUGIN_ROOT} env var with no fallback path unlike hooks.json commands.
src/servers/mcp-server.ts	Adds optional timeoutMs parameter to callWorkerAPI/callWorkerAPIPost and applies appropriate per-operation timeouts (120s for create/rebuild, 60s for prime/query/reprime); straightforward and correct.
src/services/smart-file-read/parser.ts	Adds heuristic fallback outline (Markdown headers, ALL-CAPS lines, colon-terminated lines) for files without tree-sitter grammar; foldedTokenEstimate is hardcoded at 50 regardless of symbol count.
src/services/sync/ChromaMcpManager.ts	Four fixes: correct transport-close ordering before null, shutdownSync() via private _process cast for exit handler, proxy env var stripping, and chroma-mcp version pinned to 0.2.3.

Sequence Diagram

sequenceDiagram
    participant Worker as Worker Process
    participant MCP as mcp-server.ts
    participant CM as ChromaMcpManager
    participant Chroma as chroma-mcp subprocess

    MCP->>CM: callTool(toolName, args)
    CM->>Chroma: stdio request
    alt Transport error (#1925 fix)
        Chroma-->>CM: error
        CM->>CM: save staleTransport / staleClient
        CM->>CM: null this.transport / this.client
        CM->>CM: staleTransport.close()
        CM->>CM: staleClient.close()
        CM->>Chroma: reconnect + retry
    end

    Note over Worker,Chroma: On process 'exit' (#1924 fix)
    Worker->>CM: shutdownSync()
    CM->>Chroma: SIGKILL(_process)

    Note over MCP: Corpus ops use per-op timeouts (#1922 fix)
    MCP->>MCP: corpus/create → 120s
    MCP->>MCP: corpus/prime → 60s
    MCP->>MCP: corpus/query → 60s
    MCP->>MCP: corpus/rebuild → 120s

Loading

_{Reviews (1): Last reviewed commit: "fix(mcp): fix .mcp.json, timeouts, txt o..." | Re-trigger Greptile}

[greptile-apps]

Fragile private-API cast for _process

_process is not part of StdioClientTransport's public contract — it's an internal property accessed via as unknown as { _process? }. If a future version of @modelcontextprotocol/sdk renames or removes this field, shutdownSync will silently become a no-op (no error, no kill, orphaned subprocess). The same cast already exists at line 505 for registerManagedProcess, so this is consistent with the existing pattern, but consider adding a fallback sentinel so any silent failures surface in logs:

if (!childProcess) {
  logger.warn('CHROMA_MCP', 'shutdownSync: _process not available on transport (MCP SDK may have changed)');
}

[greptile-apps]

foldedTokenEstimate ignores actual symbol count

The fallback path returns a hardcoded foldedTokenEstimate: 50 regardless of how many sections were detected. For a large .txt or configuration file with many headers, this underestimates the folded view and can throw off token-budget calculations downstream. The old parseFilesBatch code also used 50, so this is not a regression — but now that parseFile is called directly, computing an estimate from fallbackSymbols.length would be more accurate at almost no extra cost.

Suggested change

	return {
	filePath, language, symbols: [], imports: [],
	filePath, language, symbols: fallbackSymbols, imports: [],
	totalLines: lines.length, foldedTokenEstimate: 50,
	};
	return {
	filePath, language, symbols: fallbackSymbols, imports: [],
	totalLines: lines.length,
	foldedTokenEstimate: Math.max(50, fallbackSymbols.length * 10),
	};

[greptile-apps]

No fallback if CLAUDE_PLUGIN_ROOT is unset

"${CLAUDE_PLUGIN_ROOT}/scripts/mcp-server.cjs" relies on Claude Code expanding the env var at load time. Every shell command in hooks.json has an explicit fallback chain ([ -z "$_R" ] && _R=$(ls -dt ...)) for the case where CLAUDE_PLUGIN_ROOT isn'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.

[thedotmack]

Closing to start fresh from main — will redo fixes isolated in Docker container.