fix(mcp): isolate stdout to prevent stray output from corrupting JSON-RPC#383
fix(mcp): isolate stdout to prevent stray output from corrupting JSON-RPC#383
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRedirects startup stdout to stderr, duplicates and reopens the original stdout file descriptor for MCP JSON-RPC traffic, runs an explicit MCP stdio server using those streams, forces spawned subprocesses to use DEVNULL for stdin, and adds tests for logging, fd redirection, and subprocess stdin behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CLI as CLI (cli.py)
participant Server as Server (server.py)
participant MCP as MCP Core
participant Subproc as Subprocess
Client->>CLI: start server
CLI->>CLI: sys.stdout = sys.stderr
CLI->>Server: import & run server
Server->>Server: dup fd 1 (save MCP stdout)
Server->>Server: redirect fd 1 & sys.stdout -> stderr
Server->>Server: open UTF-8 streams for stdin.buffer and duped fd
Server->>MCP: start stdio_server(read_stream, write_stream)
MCP->>Server: JSON-RPC loop over provided streams
Server->>Subproc: create_subprocess_exec(..., stdin=DEVNULL)
Subproc->>Subproc: stdin is isolated
Client->>MCP: send JSON-RPC request
MCP->>Client: respond via duped stdout stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-RPC MCP communicates via JSON-RPC over stdin/stdout. Log messages, gRPC C extension output, and print() calls in dependencies were leaking into stdout and crashing the MCP session in Cursor. - Duplicate real stdout fd and redirect fd 1 to stderr so only the MCP transport can write to the original stdout - Redirect sys.stdout to stderr early in cli.py before module imports - Set subprocess stdin to DEVNULL to prevent j from consuming MCP input - Add tests for the fd-redirect pattern, subprocess stdin isolation, and _setup_logging behavior Made-with: Cursor
mangelajo
force-pushed
the
fix/mcp-stdout-isolation
branch
from
358c422 to
680e074
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 475-489: The code duplicates and rebinds stdout using
sys.stdout.fileno(), which after cli.py has reassigned sys.stdout to stderr
yields fd 2 instead of fd 1; replace os.dup(sys.stdout.fileno()) with os.dup(1)
and replace os.dup2(sys.stderr.fileno(), sys.stdout.fileno()) with os.dup2(2, 1)
so you operate on the real fd numbers; update the block around real_stdout_fd,
the dup/dup2 calls, and the wrapping of mcp_stdout (near _setup_logging(),
create_server(), manager.running(), mcp_stdin/mcp_stdout) accordingly, and add a
regression test that runs the cli.py-style rebind (sys.stdout -> sys.stderr)
before invoking run_server() to assert MCP JSON-RPC is written to fd 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58501406-6bee-4e84-b61c-ea387862174f
📒 Files selected for processing (4)
python/packages/jumpstarter-mcp/jumpstarter_mcp/cli.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py (1)
656-660:⚠️ Potential issue | 🟡 MinorMake this regression test reproduce the
cli.pystartup state.Line 658 and Line 659 still duplicate through
sys.stdout.fileno()/sys.stderr.fileno()whilesys.stdoutis bound to fd 1, so this test would also pass ifrun_server()regressed to the old broken implementation. Rebindsys.stdout = sys.stderrbefore the duplication step and exercise the literalos.dup(1)/os.dup2(2, 1)path so the test actually guards the fix.🧪 Proposed fix
- # Apply the same redirect pattern as run_server(): - sys.stdout.flush() - mcp_fd = os.dup(sys.stdout.fileno()) # save "real stdout" (pipe) - os.dup2(sys.stderr.fileno(), sys.stdout.fileno()) # fd 1 -> stderr - sys.stdout = sys.stderr + # Reproduce cli.py's early stdout rebind before run_server(): + sys.stdout = sys.stderr + + # Apply the same redirect pattern as run_server(): + mcp_fd = os.dup(1) + os.dup2(2, 1) + sys.stdout = sys.stderr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py` around lines 656 - 660, The test's stdout/stderr redirection doesn't reproduce cli.py startup because it calls os.dup/sys.stdout.fileno() while sys.stdout is still fd 1; modify the setup in server_test.py so you first rebind sys.stdout = sys.stderr (so sys.stdout no longer points to the original fd) and then perform the literal fd-level operations using os.dup(1) to save fd 1 into mcp_fd and os.dup2(2, 1) to redirect fd 1 to fd 2, mirroring the run_server() behavior and ensuring the test will fail if run_server() regresses to the old broken implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py`:
- Around line 573-626: These tests call _setup_logging() which mutates global
logger and handler state; capture and restore any mutated levels: record
pre-test levels of all root StreamHandler instances and the per-name logger
levels (e.g., loggers "jumpstarter_mcp", "jumpstarter", "mcp") before calling
_setup_logging(), then after the test restore root.handlers, reset each
StreamHandler.level to its saved value, and reset each logger level using the
saved_levels mapping; update test_adds_file_handler_to_root_logger,
test_caps_stream_handlers_to_warning, and test_sets_logger_levels to save and
restore these handler and logger levels (use variables like handlers_before,
stream_handler, saved_levels, and the loggers dict to locate code).
---
Duplicate comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py`:
- Around line 656-660: The test's stdout/stderr redirection doesn't reproduce
cli.py startup because it calls os.dup/sys.stdout.fileno() while sys.stdout is
still fd 1; modify the setup in server_test.py so you first rebind sys.stdout =
sys.stderr (so sys.stdout no longer points to the original fd) and then perform
the literal fd-level operations using os.dup(1) to save fd 1 into mcp_fd and
os.dup2(2, 1) to redirect fd 1 to fd 2, mirroring the run_server() behavior and
ensuring the test will fail if run_server() regresses to the old broken
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 028501fb-26d5-471e-af63-b6660a77ec9c
📒 Files selected for processing (4)
python/packages/jumpstarter-mcp/jumpstarter_mcp/cli.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-mcp/jumpstarter_mcp/cli.py
| def test_adds_file_handler_to_root_logger(self, tmp_path): | ||
| root = logging.getLogger() | ||
| handlers_before = list(root.handlers) | ||
|
|
||
| try: | ||
| with patch("pathlib.Path.home", return_value=tmp_path): | ||
| _setup_logging() | ||
|
|
||
| new_file_handlers = [ | ||
| h | ||
| for h in root.handlers | ||
| if isinstance(h, logging.FileHandler) and h not in handlers_before | ||
| ] | ||
| assert len(new_file_handlers) == 1 | ||
| assert "mcp-server.log" in new_file_handlers[0].baseFilename | ||
| finally: | ||
| root.handlers = handlers_before | ||
|
|
||
| def test_caps_stream_handlers_to_warning(self, tmp_path): | ||
| root = logging.getLogger() | ||
| handlers_before = list(root.handlers) | ||
|
|
||
| stream_handler = logging.StreamHandler() | ||
| stream_handler.setLevel(logging.DEBUG) | ||
| root.addHandler(stream_handler) | ||
|
|
||
| try: | ||
| with patch("pathlib.Path.home", return_value=tmp_path): | ||
| _setup_logging() | ||
| assert stream_handler.level == logging.WARNING | ||
| finally: | ||
| root.handlers = handlers_before | ||
|
|
||
| def test_sets_logger_levels(self, tmp_path): | ||
| root = logging.getLogger() | ||
| handlers_before = list(root.handlers) | ||
|
|
||
| loggers = { | ||
| "jumpstarter_mcp": logging.getLogger("jumpstarter_mcp"), | ||
| "jumpstarter": logging.getLogger("jumpstarter"), | ||
| "mcp": logging.getLogger("mcp"), | ||
| } | ||
| saved_levels = {name: lg.level for name, lg in loggers.items()} | ||
|
|
||
| try: | ||
| with patch("pathlib.Path.home", return_value=tmp_path): | ||
| _setup_logging() | ||
| assert loggers["jumpstarter_mcp"].level == logging.DEBUG | ||
| assert loggers["jumpstarter"].level == logging.DEBUG | ||
| assert loggers["mcp"].level == logging.WARNING | ||
| finally: | ||
| root.handlers = handlers_before | ||
| for name, level in saved_levels.items(): | ||
| loggers[name].setLevel(level) |
There was a problem hiding this comment.
Restore the logging levels these tests mutate.
Line 589, Line 604, and Line 624 only replace root.handlers, but _setup_logging() also changes every existing root StreamHandler level and the jumpstarter_mcp/jumpstarter/mcp logger levels. Those objects survive the list swap, so later tests can inherit altered logging behavior depending on execution order.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py` around lines
573 - 626, These tests call _setup_logging() which mutates global logger and
handler state; capture and restore any mutated levels: record pre-test levels of
all root StreamHandler instances and the per-name logger levels (e.g., loggers
"jumpstarter_mcp", "jumpstarter", "mcp") before calling _setup_logging(), then
after the test restore root.handlers, reset each StreamHandler.level to its
saved value, and reset each logger level using the saved_levels mapping; update
test_adds_file_handler_to_root_logger, test_caps_stream_handlers_to_warning, and
test_sets_logger_levels to save and restore these handler and logger levels (use
variables like handlers_before, stream_handler, saved_levels, and the loggers
dict to locate code).
| read_stream, | ||
| write_stream, | ||
| ): | ||
| await mcp._mcp_server.run( |
There was a problem hiding this comment.
this is meh, but trying to do the right thing here: modelcontextprotocol/python-sdk#2343 for the long term
They don't let you wire stdio/stdout in the async streams
| try: | ||
| async with manager.running(): | ||
| await mcp.run_stdio_async() | ||
| mcp_stdin = anyio.wrap_file( |
There was a problem hiding this comment.
not needed?
https://github.com/modelcontextprotocol/python-sdk/blob/7ba4fb881d85406f44a5af8169fb7200fa7c8e49/src/mcp/server/stdio.py#L41
I see it does the same internally
There was a problem hiding this comment.
Good catch — you're right, stdio_server() already wraps sys.stdin.buffer with a UTF-8 TextIOWrapper when no stdin is passed (stdio.py#L41-L42). Since we haven't redirected stdin (only stdout), the default is fine.
Dropped the custom stdin — now we only pass the custom stdout (which points at the saved fd from before the redirect). Fixed in b5bd109.
There was a problem hiding this comment.
for stdout we absolutely need the custom stream — by the time stdio_server() runs, fd 1 has been redirected to stderr via os.dup2(2, 1). If we let stdio_server() use its default (sys.stdout.buffer), MCP JSON-RPC would go to stderr. The custom mcp_stdout wraps the saved fd (the real original stdout) which is the whole point of the isolation.
There was a problem hiding this comment.
lol, the first answer was claude using the github mcp 🥇
stdio_server() already wraps sys.stdin.buffer with UTF-8 TextIOWrapper when no stdin is passed. Only stdout needs a custom stream (the saved fd from before the redirect). Made-with: Cursor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.8
git worktree add -d .worktree/backport-383-to-release-0.8 origin/release-0.8
cd .worktree/backport-383-to-release-0.8
git switch --create backport-383-to-release-0.8
git cherry-pick -x 5a6c312a8307e0edb8ad02b98fc309ecae28e3af |
2 participants
Summary
"is not valid JSON"errors.os.dup) for exclusive MCP transport use, then redirecting fd 1 to stderr (os.dup2) so all stray output is harmless. Also redirectsys.stdoutearly in the CLI entry point before module imports can trigger output.stdin=DEVNULLto prevent thejchild process from consuming MCP JSON-RPC input.Test plan
test_stray_writes_do_not_reach_saved_stdout— verifies the fd-redirect pattern behaviorally using pipes (stray writes land in stderr, MCP writes land in stdout)test_subprocess_stdin_is_devnull— verifies subprocess is created withstdin=DEVNULL_setup_logging— verifies file handler, stream handler capping, and logger levelsMade with Cursor