fix(mcp): honor subprocess PATHEXT for stdio resolution - #491
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
Found 1 medium-severity issue: misleading comments in Files Reviewed (2 files) |
There was a problem hiding this comment.
Risk: 🟡 Medium (48/100) — 1 medium finding · 167 LOC across 2 files
Review Summary
1 file flagged from 2 changed files in a focused PR targeting MCP stdio process management.
Findings
- Medium (confidence 85): Misleading comments + incomplete PID filter markers in
tools/mcp_tool.py_filter_mcp_childrenadded by this PR has three issues:- Line 1895 references a "complementary
start_new_sessionfix" inagent/lsp/client.py:266that does not exist —asyncio.create_subprocess_execis called withoutstart_new_session=True. - Lines 3037-3038 claim "LSP servers and slash_worker now use
start_new_session=Truetoo" — buttui_gateway/server.py:287does not pass it tosubprocess.Popen. - Lines 1891-1892 state the filter covers "jdtls/pyright/yaml-ls" — but
_NON_MCP_CHILD_CMDLINE_MARKERSonly coversjdtlsand TUI components, missing markers forpyright-langserverandyaml-language-server.
- Line 1895 references a "complementary
- While the
_send_signalpgid guard prevents catastrophic self-kill, LSP servers that leak through the filter would be force-killed during MCP shutdown, disrupting IDE features.
Assessment
The core logic of the PR is sound, but the defense-in-depth documentation is misleading and the marker list is incomplete. These are documentation and coverage gaps rather than active bugs in the current code path, but they create false confidence for future maintainers.
| # directly by the gateway without start_new_session, so their pgid | ||
| # equals the TUI parent PID. If they leak into _stdio_pgids, the | ||
| # shutdown sweep's killpg() kills the TUI parent itself. | ||
| # See agent/lsp/client.py for the complementary start_new_session fix. |
There was a problem hiding this comment.
🟡 Misleading comments claim non-existent start_new_session fix; PID filter markers incomplete for LSP servers (bug)
Three issues in the _filter_mcp_children defense added by this PR in tools/mcp_tool.py:
-
Line 1895 says "See agent/lsp/client.py for the complementary start_new_session fix" — but
agent/lsp/client.py:266does NOT passstart_new_session=Truetoasyncio.create_subprocess_exec. -
Lines 3037-3038 say "LSP servers and slash_worker now use start_new_session=True too" — but
tui_gateway/server.py:287does NOT passstart_new_session=Truetosubprocess.Popen, and neither does the LSP client. -
Lines 1891-1892 say the filter covers "jdtls/pyright/yaml-ls" — but
_NON_MCP_CHILD_CMDLINE_MARKERS(lines 3042-3048) only contains markers forjdtlsand TUI components (tui_gateway.slash_worker,tui_gateway.entry, Eclipse launcher markers). There are no markers forpyright-langserveroryaml-language-server.
The pgid guard in _send_signal (line 4933: pgid == _my_pgid check) prevents the gateway from self-killing when an LSP child leaks into _stdio_pgids. However, LSP servers that race into the PID snapshot delta and are not caught by the filter would still be incorrectly tracked and force-killed during MCP shutdown (lines 4965, 4979), disrupting language server features for the user.
💡 Suggestion: The primary fix is to add start_new_session=True to agent/lsp/client.py:266 (asyncio.create_subprocess_exec) and tui_gateway/server.py:287 (subprocess.Popen). This makes the PID filter true defense-in-depth. If that fix is deferred, at minimum: (a) add pyright-langserver and yaml-language-server to _NON_MCP_CHILD_CMDLINE_MARKERS, and (b) update the misleading comments at lines 1895 and 3037-3038 to reflect the actual state.
📋 Prompt for AI Agents
In agent/lsp/client.py, line 266, add start_new_session=True to the asyncio.create_subprocess_exec() call. In tui_gateway/server.py, line 287, add start_new_session=True to the subprocess.Popen() call. Then update the comment at tools/mcp_tool.py line 1895 to remove the reference to the complementary fix (since it will now be present), and update the comment at line 3037 to say the fix has been applied rather than claiming it already exists. Consider adding 'pyright-langserver' and 'yaml-language-server' to _NON_MCP_CHILD_CMDLINE_MARKERS at line 3042 as defense-in-depth.
Fixes NousResearch#56536.\n\n## Summary\n- resolve stdio MCP commands using PATHEXT from the MCP subprocess environment on Windows\n- restore the parent process PATHEXT after lookup\n- add regression coverage for PATH/PATHEXT resolution plumbing\n\n## Tests\n- scripts/run_tests.sh tests/tools/test_mcp_stdio_resolution.py
Mirror-of: NousResearch#56594
NousResearch#56594