fix(mcp-client): the real-child stdio tests spawn on a subprocess-capable loop (#692) - #951

Claude finished @cdeust's task in 2m 35s —— View job

PR #951 review complete

• Read the diff and context
• Checked CI status: the Pipeline run was still in progress when I checked
• Ran the five reviewer agents: code quality, documentation, performance, security and test coverage
• Posted findings and the verdict: review comment

There are no Blockers and three Notes, none needed before merge. I could not run pytest, ruff or the craftsmanship gate here because those commands needed approval, so the review comes from reading the code. Nobody ran the tests on Windows.

Verdict: approve

Review 1 of 3: PR #951

I found no Blockers. Five reviewers looked at the diff: code quality, documentation accuracy, performance, security and test coverage.

What the reviewers confirmed by reading the code:

• The allowlist works on both platforms. It compares the last / component (mcp_client.py:122). On Windows, sys.executable is a backslash path with no /, so the whole path is compared, and the whole path is in the allowed set. On POSIX, Path(sys.executable).name covers names like python3.12. The production allowlist and its callers are unchanged.
• The new test catches a revert. mcp_client.py calls asyncio.create_subprocess_exec through the module, so the patch reaches the real call. The bare NotImplementedError lands in except Exception. With {e} the assertion would fail.
• {e!r} leaks nothing new. For OSError it shows (errno, strerror), which is less than str() showed, since str() also printed the filename. Environment values never enter the message.
• No other test needs the subprocess loop. No other test in tests_py spawns a real child on an asyncio loop; every other create_subprocess_exec is patched or monkeypatched.

Not verified: the reviewers only read the code. I could not run pytest, ruff or the craftsmanship gate in this environment. When I checked, the Pipeline CI run was still in progress. Nobody exercised the Windows Proactor path.

Notes, no action before merge

1. In test_mcp_client.py:1344-1346 and :1323, transport.close() only schedules the pipe close for the next loop pass, and loop.close() runs before that pass happens. The stderr reader task (mcp_client.py:79) is not cancelled either, so ResourceWarnings or "Task was destroyed" can still show up. Using asyncio.Runner(loop_factory=...) (3.11+) would drain both.
2. In test_mcp_client.py:1365 and :1383, await client.connect() is outside the try, so a failed handshake skips _close_and_reap and leaves the child unreaped.
3. _run_with_subprocesses copies _run (test_mcp_client.py:22-27) except for the loop factory.

Verdict: approve

Réponse aux trois notes non bloquantes de la revue 1. Aucune ne justifie à elle seule un push, qui relancerait la CI et la revue.

1. et 3. asyncio.Runner(loop_factory=...) réglerait d'un coup le drainage des pipes et du lecteur stderr, ainsi que la copie de _run. Je l'intègre au prochain push de code sur cette PR, s'il y en a un. Sinon, je le garde pour la mesure Windows qui doit confirmer cette tranche, car c'est elle qui montrera si des ResourceWarnings apparaissent réellement sous Proactor.
2. connect() hors du try existait déjà avant cette PR. Il ne concerne pas la cause Windows corrigée ici, donc je le laisse en l'état.

Generated by Claude Code