fix(chroma): spawn uvx directly on Windows instead of via cmd.exe (#2716, #2667)#2816
fix(chroma): spawn uvx directly on Windows instead of via cmd.exe (#2716, #2667)#2816YOMXXX wants to merge 1 commit into
Conversation
Greptile SummaryFixes the Windows chroma-mcp connection failure (#2716 / #2667) by removing the
Confidence Score: 5/5Safe to merge; the change is narrowly scoped, removes known-broken Windows code, and is backed by regression tests. The core change eliminates the The test file mixes an unrelated codex smoke-test with the chroma spawn-contract suite. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as Worker
participant CM as ChromaMcpManager
participant SDK as StdioClientTransport
participant UVX as uvx (direct)
Note over W,UVX: Before this PR (Windows)
W->>CM: connectInternal()
CM->>SDK: "command=cmd.exe, args=[/c, uvx, protobuf<7, ...]"
SDK-->>W: "cmd.exe re-parses < as redirect, connection closed"
Note over W,UVX: After this PR (all platforms)
W->>CM: connectInternal()
CM->>CM: "buildChromaSpawnConfig() returns {command:uvx, args verbatim}"
CM->>CM: getSpawnEnv() calls ensureUvOnPath()
CM->>SDK: "command=uvx, args=[protobuf<7, ...]"
SDK->>UVX: uvx.exe spawned directly
UVX-->>W: chroma-mcp starts successfully
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
tests/services/integrations/spawn-contract-windows.test.ts:48-61
**Unrelated codex test bundled into chroma spawn-contract file**
The second `describe` block for `codexSpawn` / #2695 is unrelated to the chroma-mcp metacharacter fix this file is named and scoped for. Mixing the two makes the file title misleading and means a failure in the codex smoke-test silences the chroma contract suite with no obvious connection. Consider moving it to a dedicated `codex-spawn.test.ts` or `spawn-contract-codex.test.ts`.
### Issue 2 of 2
src/services/sync/ChromaMcpManager.ts:70-75
**`uvx.cmd` shim on Windows re-exposes cmd.exe via cross-spawn**
When `uvx` is installed as `uvx.cmd` (possible on Windows if uv was installed via pip/pipx or certain package managers), cross-spawn detects the `.cmd` extension and wraps the invocation with `cmd.exe /c uvx.cmd …` internally. This reintroduces the exact metacharacter issue this PR removes for users in that scenario.
The standard uv Windows installer places `uvx.exe`, so the common path is safe. Worth a code comment or a CI note documenting that the fix assumes `uvx.exe` (not `.cmd`) is on PATH, so the team knows to keep this assumption in mind if uv's Windows packaging ever changes.
Reviews (2): Last reviewed commit: "fix(chroma): spawn uvx directly instead ..." | Re-trigger Greptile |
The Windows path wrapped the chroma-mcp launch as `cmd.exe /c uvx ...`, which makes cmd.exe re-parse the dep-override version specifiers (onnxruntime>=1.20, protobuf<7) as I/O redirection, so chroma-mcp never started and every tool call closed in ~30ms with MCP error -32000. Hand-quoting the args (quoteForCmdExe, thedotmack#2701) did not survive the spawn: the MCP SDK launches through cross-spawn with shell:false, and because the command is cmd.exe (a .exe, not a .bat/.cmd) cross-spawn leaves windowsVerbatimArguments unset. Node then re-escapes the quotes ("protobuf<7" -> "\"protobuf<7\""), and cmd.exe, which treats \" as a literal backslash plus a quote toggle rather than an escape, closes the quote early and sees < outside quotes again. Spawn uvx directly on every platform (buildChromaSpawnConfig): Node resolves uvx on PATH on Windows the same way as POSIX, so no shell wrap is needed and the dep specs reach uvx intact. Replace the failed quoteForCmdExe helper and its unit test with a spawn-contract test that guards against the wrapper being reintroduced. refs thedotmack#2716, thedotmack#2667, thedotmack#2696
74a01ae to
eef2dd0
Compare
Status for reviewersCI — the one red check ( That file isn't touched by this PR — it's the same Greptile — re-reviewed the latest commit ( On Greptile's one residual note (the Verified locally: |
There was a problem hiding this comment.
Posted from the wrong account by mistake — please disregard this review; see my validation under @tomeyerman instead. As a non-maintainer I can't delete a submitted review — @thedotmack, feel free to dismiss this one.
tomeyerman
left a comment
There was a problem hiding this comment.
Validated on Windows 11 Pro (10.0.26200) — the direct-uvx spawn this PR introduces connects Chroma end to end. 🎉
Disclosure on what I ran: I'm running a self-host fork that carries the functionally identical change, not a checkout of this exact commit. I diffed my ChromaMcpManager.ts against this PR — the only differences are that you extracted the logic into buildChromaSpawnConfig() and added richer comments; the runtime behavior (drop the cmd.exe /c wrapper, spawn uvx directly on every platform) is the same. So this validates the approach, on a real Windows box.
Evidence
1. Process tree — worker spawns uvx directly, no cmd.exe in the chain, dep specs verbatim:
bun.exe (worker, PID 24764) worker-service.cjs --daemon
└─ uvx.exe (PID 36612) uvx --python 3.13 --with onnxruntime>=1.20 --with protobuf<7 chroma-mcp==0.2.6 --client-type persistent --data-dir .../chroma
└─ uv.exe (PID 36700)
└─ chroma-mcp.exe (PID 36344)
└─ python.exe → python.exe
The worker (bun.exe) is the direct parent of uvx.exe — no intermediate cmd.exe. Critically, onnxruntime>=1.20 and protobuf<7 reach uvx intact; these are the exact >/< specifiers the old cmd.exe /c wrapper re-parsed as I/O redirection, which is what killed chroma-mcp in ~30ms with MCP error -32000: Connection closed.
2. Semantic round-trip succeeds (not the FTS5 keyword fallback):
// GET /api/chroma/status?deep=1
{
"status": "healthy",
"connected": true,
"details": "chroma-mcp semantic search round-trip succeeded",
"deep": true,
"probe": { "ok": true, "stage": "done", "queryLatencyMs": 762 }
}// GET /api/health
{ "status": "ok", "platform": "win32", "initialized": true, "mcpReady": true, "version": "13.4.0" }Conclusion
The cmd.exe wrapper was the whole problem, and spawning uvx directly resolves it on Windows 11 with no regression — uvx.exe/uv.exe/chroma-mcp.exe resolve on PATH and the --with pins survive the spawn. The added spawn-contract test is a good guard against the wrapper being reintroduced. LGTM for the Windows path. 👍
|
Closing as duplicate: the Windows Chroma fix landed on main via #2847 — chroma-mcp now spawns a directly-resolved uvx/uvx.exe (no cmd.exe /c), so the |
Problem
On Windows the chroma-mcp subprocess is launched as
cmd.exe /c uvx .... cmd.exe re-parses the dep-override version specifiers (onnxruntime>=1.20,protobuf<7) as I/O redirection (protobuf < 7), sochroma-mcpnever starts — every semantic-search tool call closes in ~30 ms withMCP error -32000: Connection closed.This is the defect tracked in #2716 / #2667 (closed as not-planned, but still reproducing on latest
main, as flagged by @tomeyerman in #2699).Why the existing
quoteForCmdExefix (#2701) doesn't workquoteForCmdExewrapsprotobuf<7→"protobuf<7"as a string, and its unit test is green. But the quotes don't survive the actual spawn:StdioClientTransportlaunches through cross-spawn withshell: false.cmd.exe(a.exe, not a.bat/.cmd), so cross-spawn does not setwindowsVerbatimArguments."protobuf<7"→"\"protobuf<7\"".\"as a literal backslash + a quote toggle (not an escape), so it closes the quote early and sees<outside quotes again → redirection → "The system cannot find the file specified".So the hand-quoting is defeated by the very re-quoting layer it tries to anticipate. The
cmd.exewrapper is the whole problem.Fix
Spawn
uvxdirectly on every platform (buildChromaSpawnConfig). Node'schild_process.spawnresolvesuvx→uvx.exe/uvx.cmdon PATH on Windows exactly as on POSIX, so no shell wrap is needed: the argv array is passed through without any cmd.exe re-parsing and the</>dep specs reach uvx intact. This matches the approach #2502 / #2668 converged on in the #2699 canary.cmd.exe /cWindows branch and the defeatedquoteForCmdExehelper.uvx(nocmd.exe, no/c, dep specs verbatim) — guarding against the wrapper being reintroduced.Testing
bun test tests/services/integrations/spawn-contract-windows.test.ts tests/services/sync/chroma-mcp-manager-singleton.test.ts→ 7 pass / 0 failtsc --noEmit: 0 errors introduced by this changerefs #2716, #2667, #2696, #2699