Skip to content

fix: deliver codex MCP server via CODEX_HOME overlay on Windows (#681) - #683

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-10_codex-mcp-windows-file
Sep 10, 2026
Merged

fix: deliver codex MCP server via CODEX_HOME overlay on Windows (#681)#683
ranxianglei merged 1 commit into
masterfrom
2026-09-10_codex-mcp-windows-file

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What & why

Fixes #681.

On Windows every bili codex launch rides an npm .cmd shim through cmd.exe. The default MCP injection passes the bili server definition inline:

-c mcp_servers.bili.command=<JSON.stringify(process.execPath)>

When node lives under a spaced path (the stock location C:\Program Files\nodejs\node.exe), that single argv token contains double quotes and spaces. cmd.exe treats \" purely as a quote-state toggle and has no escape for a literal quote, so it strips the token's surrounding quotes before the shim sees it. Codex then receives an unquoted TOML value with spaces — invalid TOML — so the bili MCP server never registers and plugin-mode compression silently never activates.

This is broader than spaced paths: any Windows absolute path in an inline -c value breaks, because a valid TOML string requires enclosing quotes that cmd.exe always strips. No quoting scheme can fix it — the incompatibility is fundamental.

Fix

Stop encoding the path into argv on Windows. Deliver [mcp_servers.bili] via a file instead: a persistent <CODEX_HOME>-bili overlay (reusing the proven pi/omp/dsh overlay machinery) whose merged config.toml holds the block, pointed at by CODEX_HOME. Every real-home entry except config.toml is shared (auth.json, sessions, model settings survive). POSIX keeps the byte-identical inline -c args (gated on process.platform === "win32"), so macOS/Linux are untouched.

If the overlay cannot be built (e.g. hollow on a locked-down Windows), injection degrades gracefully to wire mode — codex still runs and compression stays active server-side — with a logged warning rather than a broken launch.

Changes

  • src/client-config.ts: resolveCodexHome (CODEX_HOME || ~/.codex), re-exported from launcher.
  • src/launcher.ts: prepareCodexHome / mergeCodexBiliBlock / stripCodexBiliBlock / prepareCodexMcpInjection; the codex launch branch now routes through prepareCodexMcpInjection after env is set (covers both direct-URL and MITM sub-branches).
  • tests/launcher.test.ts: 6 new unit tests (resolve + overlay build/replace/preserve + platform gating).

Pre-flight

  • npm run typecheck — clean
  • npm test — 1240/1241 pass; the single failure is a pre-existing, environment-dependent resolveClientCommand test (this machine has /usr/bin/codex installed) — identical failure on the clean tree, unrelated to this change
  • npm run build — success

Note per the issue: real-Windows end-to-end verification of codex accepting the CODEX_HOME overlay is the remaining manual step; the unit tests lock down the generated config shape and platform gating.

On Windows every `bili codex` launch rides an npm .cmd shim through cmd.exe.
The inline `-c mcp_servers.bili.command=<JSON path>` value embeds an absolute
path carrying both quotes and spaces; cmd.exe strips the TOML-required quotes
(it has no literal-quote escape), leaving malformed TOML — so the bili MCP
server never registers and plugin-mode compression silently never activates
(stock node location C:\Program Files\nodejs\node.exe is the common trigger).

Fix: on win32 deliver [mcp_servers.bili] via a file instead of argv — a
persistent <CODEX_HOME>-bili overlay (reusing the pi/omp/dsh machinery) whose
merged config.toml holds the block, pointed at by CODEX_HOME. POSIX keeps the
byte-identical inline -c args (gated on process.platform). If the overlay
cannot be built, injection degrades to wire mode (compression still active)
with a warning.

- client-config.ts: resolveCodexHome (CODEX_HOME || ~/.codex)
- launcher.ts: prepareCodexHome / mergeCodexBiliBlock / stripCodexBiliBlock /
  prepareCodexMcpInjection; codex launch branch refactored to use them
- tests/launcher.test.ts: 6 new unit tests

Pre-flight: typecheck clean; full suite 1240/1241 (the single failure is a
pre-existing environment-dependent resolveClientCommand test, unrelated);
build success.
@github-actions

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-10_codex-mcp-windows-file (1205d8b)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-683

Each push to this PR publishes a new version under the pr-683 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr683.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 ## Review verdict: ready for human merge

Duplicate screening — searched open/closed issues & PRs for Windows/codex/MCP-injection delivery; no other item covers this. This PR implements #681 (root-cause fix), not a duplicate of anything; #679 (dsh spawn crash) is a different mechanism and stays separate.

Triage / layering — the reported symptom is "MCP server never registers on Windows"; the root cause is structural, not a quoting bug: cmd.exe cannot deliver literal " into an argv token (it only toggles quote state), while valid TOML requires enclosing quotes around path values — so any Windows absolute path in an inline -c value is undeliverable, regardless of quoting scheme. The PR fixes exactly this layer by changing the delivery mechanism (file via CODEX_HOME overlay instead of argv-encoded TOML), matching the direction decided in #681. It does not paper over the symptom.

What I verified (local, node v22.23.2):

  • npm run typecheck — clean on the PR commit (1205d8b) and on the merged tree.
  • npm test on PR base: 1240/1241 — sole failure is the env-dependent resolveClientCommand test (this sandbox has /usr/bin/codex); master already fixed it hermetically in f0a85ec, so it disappears post-merge.
  • Merged onto current origin/master (4f22209): 0 conflicts, then fresh npm ci (master pins acp-kernel 0.0.61 vs the PR base's 0.0.56) → 1301/1301 pass, including all 6 new tests.
  • npm run build — success.
  • Diff hygiene: 3 files (+228/−3), every line serves the stated purpose; no version change; no stray files. CI's windows-latest matrix already runs these exact test patterns — the dsh overlay tests assert isSymbolicLink() on overlay entries and pass there (junctions report as symlinks in Node lstat), so the new symlink assertions are proven-safe on GH runners.

Code-level findings:

  • Block shape parity: mergeCodexBiliBlock emits byte-for-byte the same fields as buildCodexMcpArgs (command/args/env.BILI_MCP_PROXY/env.BILI_CONVERSATION_ID); JSON.stringify output is a valid TOML basic string (quotes/backslashes escaped).
  • POSIX untouched: win32 gate confirmed; unified injection point preserves arg order and env semantics (old direct-URL branch's bare codexConversationId check ≡ new injectMcp && codexConversationId, since the id exists iff injection is armed).
  • Real home never mutated: all writes go through refreshOverlayHome/writeOverlayFileAtomic; tests assert real-home integrity, stale-block replacement (no duplicate [mcp_servers.bili] when the user already ran bili plugin install codex), and other tables preserved.
  • Degradation path: refreshOverlayHome returns false only on mkdir failure or HOLLOW detection (total > 0 && accessible === 0, i.e. junction/hardlink/copy all denied) → logged warning + wire mode, codex still runs. Fresh-machine case (no real home yet) builds a config-only overlay — acceptable.

Minor observations (report-only, not blocking):

  1. writeOverlayFileAtomic swallows write errors: if the config.toml write fails after a successful refresh, CODEX_HOME would point at an overlay lacking config.toml without the advertised warning. Pre-existing pattern shared with dsh; extremely rare (same dir just succeeded mkdir). Worth a follow-up hardening issue at most.
  2. Concurrent launches share the <CODEX_HOME>-bili overlay → last writer wins for BILI_MCP_PROXY/conversation id. Inherited from the shared mechanism (the existing .bili-launch.pid warning already fires for dsh today), not a regression.
  3. Remaining manual step per the PR note: real-Windows end-to-end confirmation that codex accepts the CODEX_HOME overlay (unit tests lock shape + platform gating only).

I made no code changes — nothing needed fixing. Merge is human-only per repo rules: #683


中文摘要:审核了 PR #683(Windows 上改用 CODEX_HOME 文件覆盖层投递 codex MCP server,根治 cmd.exe 无法传递带引号路径导致内联 TOML 失效的问题)——根因判断正确、方案打到核心,typecheck/build 通过、合入当前 master 后全量测试 1301/1301 绿、diff 干净无越界改动,可以合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Windows: codex MCP injection -c value loses its quotes through cmd.exe when node lives under a spaced path

1 participant