| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
I checked 52ed34a locally on Windows/Python 3.13.13:
uv run --frozen pytest tests/server/test_stdio.py -q
# 2 passed
uv run --frozen ruff check src/mcp/server/stdio.py tests/server/test_stdio.py
uv run --frozen pyright src/mcp/server/stdio.py tests/server/test_stdio.py
# 0 errors
git diff --check origin/main...HEADThe behavior-level assertion that the process wrappers stay open after stdio_server() exits is useful for #1933.
One edge to decide on: the default path now requires sys.stdin / sys.stdout to expose fileno(). A default-path probe with in-memory TextIOWrapper(io.BytesIO(...)) streams now fails immediately with UnsupportedOperation: fileno; the previous test_stdio_server_invalid_utf8 shape used that kind of wrapper before this branch switched it to TemporaryFile. If the project only wants the default path to support real process stdio, this is fine. If preserving fileno-less fake/default streams matters for tests or embedders, this probably needs a fallback path.
Sorry, something went wrong.
|
Thanks for checking this on Windows. I kept the real-stdio path on duplicated file descriptors, but added a fallback for fileno-less in-memory stdin/stdout replacements so the old test/embedding shape continues to work. Added a regression test for that path too.\n\nValidated locally:\n\nconsole\nuv run --frozen pytest tests/server/test_stdio.py -q\n# 3 passed\nuv run --frozen ruff check src/mcp/server/stdio.py tests/server/test_stdio.py\n# All checks passed\nuv run --frozen pyright src/mcp/server/stdio.py tests/server/test_stdio.py\n# 0 errors\ngit diff --check origin/main...HEAD\n |
Sorry, something went wrong.
|
One more CI follow-up: the Python 3.14 lowest-direct jobs exposed an AnyIO subprocess startup SyntaxWarning being forwarded through stderr before the expected clean-exit marker. I adjusted the stdio interaction test to keep proving the child exited cleanly while tolerating dependency startup warnings before that marker.\n\nAdditional local validation:\n\nconsole\nuv run --frozen pytest tests/server/test_stdio.py tests/interaction/transports/test_stdio.py -q\n# 5 passed\nuv run --frozen ruff check src/mcp/server/stdio.py tests/server/test_stdio.py tests/interaction/transports/test_stdio.py\n# All checks passed\nuv run --frozen pyright src/mcp/server/stdio.py tests/server/test_stdio.py tests/interaction/transports/test_stdio.py\n# 0 errors\ngit diff --check origin/main...HEAD\n |
Sorry, something went wrong.
There was a problem hiding this comment.
Rechecked current head bf3fabf on Windows/Python 3.13.13.
The original fileno-less default-path crash I reported is fixed: an in-memory TextIOWrapper(io.BytesIO(...)) stdin/stdout replacement now lets stdio_server() read the JSON-RPC request successfully. The real-file-descriptor path also preserves the original wrappers after the context; my temporary-file probe could still write to stdout after stdio_server() exited.
One remaining edge to decide on: the fileno-less fallback reads successfully, but it still closes the replacement sys.stdin / sys.stdout wrappers after the context exits. My probe result was:
{
"message_id": 123,
"stdin_closed": True,
"stdout_closed": True,
"stdout_write_error": "ValueError: I/O operation on closed file.",
"stdin_read_error": "ValueError: I/O operation on closed file.",
}So if the intended compatibility guarantee is only "fileno-less default streams do not crash", this head covers it. If the intent is also "preserve replacement stdio wrappers" for embedders/tests that provide in-memory streams, the fallback probably still needs to avoid wrapping/closing sys.stdin.buffer and sys.stdout.buffer directly.
Checks I ran:
I also tried the broader command from the author note, uv run --frozen pyright src\mcp\server\stdio.py tests\server\test_stdio.py tests\interaction\transports\test_stdio.py; that still reports the existing errlog=errlog type issue in tests/interaction/transports/test_stdio.py, on a line this branch does not change.
Sorry, something went wrong.
|
Thanks for rechecking this on Windows. That remaining edge is real: the current fallback fixes the fileno-less crash, but it does not fully preserve replacement in-memory wrappers after the context exits. I think the stronger guarantee is worth keeping, especially for embedders/tests that temporarily replace stdio. I’ll update the fallback so it avoids closing the caller-provided fileno-less wrappers, then rerun the focused stdio tests plus ruff/pyright. Also noted on the existing errlog=errlog pyright issue in the broader interaction test file. That looks pre-existing and outside this branch, so I’ll keep the validation scope explicit. |
Sorry, something went wrong.
|
Updated this edge case in e626a5e: the fileno-less fallback now reuses the caller-provided sys.stdin / sys.stdout wrappers instead of creating TextIOWrapper objects around their buffers, so exiting stdio_server() no longer closes those replacement streams. I also extended test_stdio_server_supports_fileno_less_standard_streams to assert the replacement wrappers remain open and writable/readable after the context exits. Validated locally: uv run --frozen pytest tests/server/test_stdio.py -q
# 3 passed
uv run --frozen pytest tests/server/test_stdio.py tests/interaction/transports/test_stdio.py -q
# 5 passed
uv run --frozen ruff check src/mcp/server/stdio.py tests/server/test_stdio.py tests/interaction/transports/test_stdio.py
# All checks passed
uv run --frozen pyright src/mcp/server/stdio.py tests/server/test_stdio.py
# 0 errors
git diff --check origin/main...HEAD |
Sorry, something went wrong.
|
Cross-reference / consolidation note (verified independently from CI, not a fix request) The tests/interaction/transports/test_stdio.py::test_tool_call_and_notification_round_trip_over_a_stdio_subprocess change in this PR is fixing a pre-existing, main-wide failure on the test (3.14, lowest-direct, windows-latest) matrix cell — not anything specific to this PR's feature work. I hit the identical red cell on two of my own unrelated PRs (auth/OAuth client changes that don't touch stdio at all), and reproduced the root cause:
This is currently being patched, in parallel, inside 6 open PRs that each bundle it as collateral with unrelated work:
All six are functionally equivalent for the test's stated intent (the docstring/comment says the assertion's purpose is to prove the child wrote clean exit on its own + that the transport passes stderr through — not that stderr is byte-pristine). I verified str.endswith(snapshot(...)) works (inline-snapshot's proxy forwards to the underlying str), so #2734's form is valid. Recommendation: land one focused fix for this (the .endswith(...) last-line check reads cleanest and keeps the snapshot semantics), and drop the test edit from the other PRs to keep each PR scoped to its actual feature. I did not open a 7th duplicate PR for this — flagging instead so the parallel edits can be consolidated. |
Sorry, something went wrong.
|
Rebased this on latest main to clear the dirty state. While resolving the rebase I dropped the collateral tests/interaction/transports/test_stdio.py stderr assertion tweak, since main now handles the Python 3.14/AnyIO startup warning via PYTHONWARNINGS and the useful part of this PR is the stdio preservation fix.\n\nCurrent branch keeps the focused changes to:\n- duplicate real stdin/stdout fds before wrapping them so process stdio wrappers stay open\n- support fileno-less replacement streams without closing caller-provided wrappers\n- cover the real-fd, fileno-less, invalid UTF-8, and MCPServer.run stdio paths\n\nValidated after rebase:\n\nconsole\nuv run --frozen pytest tests/server/test_stdio.py -q\n# 5 passed\nuv run --frozen ruff check src/mcp/server/stdio.py tests/server/test_stdio.py\n# All checks passed\nuv run --frozen pyright src/mcp/server/stdio.py tests/server/test_stdio.py\n# 0 errors\ngit diff --check origin/main...HEAD\n |
Sorry, something went wrong.
|
Thanks for the PR. Tracking this in #2040 instead. I'm closing this out as part of a backlog cleanup now that v2 is out. Feel free to reopen if this is still relevant. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Fixes #1933
Test plan