fix(code): capture stdio MCP server stderr into the logger - #5610
Merged
Mason Daugherty (mdrxy) merged 21 commits intoAug 19, 2026
Conversation
Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
github-actions
Bot
added
dcode
Mason Daugherty (mdrxy)
marked this pull request as ready for review
August 19, 2026 01:58
![open-swe[bot]](https://avatars.githubusercontent.com/in/1395372?s=60&v=4){kind=small}
A stdio MCP server can spawn a longer-lived descendant that inherits the stderr pipe and then exit when its stdin closes. The surviving descendant keeps the pipe's write end open, so the drain thread blocks in os.read past process exit and the unbounded join in wait_closed hung session cleanup (discovery failure, reload, shutdown). Bound the join and, when the thread is still alive, close the pipe read end to force os.read to fail so the daemon thread exits.
os.open mode bits and chmod are no-ops on Windows DACLs, so the debug log (which captures MCP stderr that may contain credentials) stayed readable by other local users despite the documented user-only guarantee. On Windows, replace the file DACL via ctypes/advapi32 with one granting full control to the current user only; POSIX keeps the 0o600 path. Failures downgrade to a warning so logging still attaches.
`TrusteeForm=2` is `TRUSTEE_BAD_FORM`, not `TRUSTEE_IS_SID` (0), and `TrusteeType=0` is `TRUSTEE_IS_UNKNOWN`, not `TRUSTEE_IS_USER` (1). With the bad form, `SetEntriesInAclW` fails with `ERROR_INVALID_PARAMETER`, so `_apply_windows_owner_only_dacl` always raised and `configure_debug_logging` always fell through to its warning path. The debug log kept its inherited DACL on every Windows run — the one platform where the POSIX `chmod` cannot help. Promote the three `accctrl.h` enums to named constants. All of them start at 0 with unrelated meanings, so a transposed literal compiles fine and only fails at runtime; naming them keeps the mistake from recurring silently.
The Windows DACL hardening had no coverage anywhere: its only test is `skipif(sys.platform != "win32")` and `test-code` ran on ubuntu-latest only. That is why the `TRUSTEE_BAD_FORM` bug shipped unnoticed. Add a windows-latest leg, matching `test-deepagents`. Verify the DACL through `icacls` instead of pywin32, which was imported at module scope but declared in no dependency group — so on Windows the whole module failed at collection rather than skipping one test. Asserting a single entry naming the current user is what distinguishes an applied, protected DACL from an inherited one. Drop `test_prepare_debug_file_routes_to_windows_acl`: it patched a private helper and asserted the call rather than any behavior, and the real DACL test covers the same path.
`_prepare_debug_file` opens with `O_NOFOLLOW` so a symlink planted at the debug path is refused. That refusal was downgraded to a warning, and `logging.FileHandler` then reopened the same path without `O_NOFOLLOW` — following the symlink and appending through it. The default path is /tmp/deepagents_debug.log, so a blocked redirect became a successful one. The same fall-through applied to any other hardening failure, leaving captured MCP server stderr in a file of unknown permissions. Fail closed instead: warn on stderr and via the logger, then skip the file handler. The in-memory buffer still backs the Debug Console.
`_debug` is imported from `deepagents_code/__init__.py`, so it runs on every command including `dcode -v`. `ctypes` costs a few milliseconds and pulls in `struct`, and every consumer of it sits inside `if os.name == "nt"`. Move it under the existing guard, per the startup-performance rule in AGENTS.md.
`_close_read_fd` did an unlocked check-then-set on `_read_fd_closed` while being called from two threads: the drain thread's `finally` and `wait_closed`. `Thread.is_alive()` stays true while the thread runs its `finally`, so both callers could pass the check and close the same fd twice. Between the two closes the fd number is free, so the second close could reap a descriptor another thread had since opened — and `suppress(OSError)` guaranteed it went unreported. Hold `_fd_lock` across the check and the close, and warn instead of suppressing. Add a `_stopping` event set before the force-close. Closing a descriptor does not reliably interrupt a blocked `os.read`: on darwin the read returns EOF, on Linux it can stay parked. Either way the fd number is already released, so the drain loop must not issue another read against it.
Capture requires DEBUG, so the default runtime configuration had `_capture` false — and there `except OSError` discarded every drain failure with no record at any level. Draining is the half that must always work: once the pipe buffer fills, the server blocks forever on its next stderr write and nothing explains why. Log at WARNING regardless of capture, since a stopped drain is a server-liveness problem rather than a logging one. Add an `except Exception` backstop. A `MemoryError` or a future `TypeError` in the decode path escaped into `threading.excepthook`, which is invisible in the TUI, and left the child with nobody reading its stderr.
The capture decoder took the server's `encoding_error_handler`, which defaults to `strict`. One stray non-UTF-8 byte on stderr therefore raised `UnicodeDecodeError` and discarded the whole 8 KiB read chunk — losing exactly the diagnostic this capture exists to provide. Nothing parses captured stderr, so a mangled character is strictly better than a dropped chunk. Keep the server's handler for the protocol stream only. Flush the buffered line before resetting the decoder on the remaining defensive path, so a reader sees a truncated line rather than text spliced across the discarded bytes. Add the missing behavioral tests: a byte invalid for the declared encoding, a line of exactly `_MCP_STDERR_LINE_LIMIT`, and a below-DEBUG drain of far more than a pipe buffer holds. All three fail against a deliberately reintroduced regression.
`mcp` hands `errlog` to `anyio.open_process(stderr=...)` on POSIX and to `create_windows_process` on Windows; both consume only `fileno()`. So `write`, `flush`, `writable`, `encoding` and `errors` were never called — about thirty untested lines, including a `written is None` busy-spin that would have burned a core had anything reached it. Remove them, along with the now-unused `errors` constructor argument; the server's handler still governs the protocol stream via `StdioServerParameters`. `io.TextIOBase` stays for its `closed` bookkeeping and the `TextIO` cast the transport signature needs. Say so in the class docstring, together with the reader thread's two jobs and the fact that capture latches at construction.
`_build_connection` already runs `resolve_mcp_server_env` over `env` before
the connection is built, and `_interpolate_env` there handles both `${VAR}`
and `${VAR:-default}` and raises on an unset reference. Every connection
reaching `_create_mcp_session` — discovery and `MCPSessionManager` alike —
comes from that path, so `_resolve_stdio_env` could not substitute anything.
Its "unexpanded variable reference" warning was reachable only as a false
positive on a value that legitimately resolved to text containing `${`, and
its only other effect was a second regex pass over resolved secrets.
The stderr capture test hand-built a connection with `${MCP_TEST_ENV}`, which
was the sole caller relying on the second pass. Pass the resolved value the
way production does; the test's subject is stderr decoding either way.
Several docstrings described behavior the code does not have: - "full control" in four places. The ACE grants `FILE_GENERIC_READ | FILE_GENERIC_WRITE`; `DELETE` and `WRITE_DAC` are not granted. - `_set_windows_owner_only_dacl` credited `_prepare_debug_file` with catching the error. It has no handler; `configure_debug_logging` does. - `Raises: WinError` names a factory function, not a type. The object raised is an `OSError`, which is why the caller's `except OSError` works. - `_get_current_user_sid` explained the SID lifetime via a "referrer", which is not a ctypes concept, and presented the `_buffer` attribute as the sole mechanism when `.contents` already retains the array. - The drain-join comments asserted that closing the read end makes `os.read` fail. On darwin it returns EOF, and on Linux it may not return at all. - `_prepare_debug_file` documented neither its `OSError` contract nor why `O_NOFOLLOW` is there. Also record that the stdio branch of `_create_mcp_session` mirrors the adapter's `_create_stdio_session`, and why the parent's write end is closed inside the `stdio_client` context — an uncommented duplicate-looking close is an easy target for a cleanup that would reintroduce the teardown stall. DEVELOPMENT.md overstated the file permissions as a guarantee and implied the log is terminal-safe: only the ESC byte is stripped, so `[31m` survives as literal text. Rewrite the section in shorter sentences.
Two small lifecycle holes in `_MCPStderrSink.__init__`: If `self._writer.close()` raised in the thread-start failure path, the read end leaked. Close it in a `finally`, and route it through `_close_read_fd` so the single-close bookkeeping is not bypassed. If `os.fdopen` raised, the partially built object was still finalized, and `io.IOBase`'s finalizer called `close()`, which dereferenced `_writer` before it existed. The resulting `AttributeError` reached the unraisable hook next to the real error — invisible in a TUI, or screen-corrupting. Verified with a patched `os.fdopen` that the hook now stays clean. Also make the stderr capture test's timing invariant explicit: the count of two holds because `final` has no trailing newline and the child blocks on stdin, so the EOF flush cannot happen until the session context exits.
github-actions
Bot
added
github_actions
`test_footers_render_for_hydrated_messages_above` passed only when earlier
tests left residual state that kept the visible window tight. Run alone, the
`set_timer` patch suppresses the deferred transcript prune that
`_load_thread_history`'s "Resumed thread" mount schedules, so the window
never shrinks back and `hist-0`'s footer is already mounted -- the
`pytest.raises(NoMatches)` guard then never fires.
Build the archived-head state the same way the transcript virtualization
tests do: mount rows via `_mount_message`, shrink `WINDOW_SIZE`, and
`_prune_messages("above")` synchronously, instead of driving it through
history loading's deferred timers.
On Linux, closing a pipe's read end does not wake a thread blocked in `os.read`, so the drain thread legitimately outlives the bounded teardown (it is a daemon by design and dies when the leaked descendant exits). The test's real contract is that session close stays bounded; wrap it in `asyncio.timeout` so a regression to an unbounded join still fails.
The libs/code unit test suite is POSIX-only (os.killpg/getpgid, bash, /tmp paths), so the new windows-latest matrix entry failed ~280 tests en masse. Remove it until the suite is made Windows-compatible.
Mason Daugherty (mdrxy)
deleted the
mdrxy/code/mcp-stdio-stderr-logging
branch
August 19, 2026 19:19
Mason Daugherty (mdrxy)
pushed a commit
that referenced
this pull request
Aug 19, 2026
> [!CAUTION] > Merging this PR will automatically publish to **PyPI** and create a **GitHub release**. For the full release process, see [`.github/RELEASING.md`](https://github.com/langchain-ai/deepagents/blob/main/.github/RELEASING.md). --- _Release notes preview: keep this section in sync with the package `CHANGELOG.md`. Publish reads the merged CHANGELOG via `release.yml`, not this PR description — keep them aligned anyway so the PR stays an accurate historical record for reviewers and anyone returning later._ --- ## [0.1.58](deepagents-code==0.1.57...deepagents-code==0.1.58) (2026-08-19) ### Breaking Changes - `deepagents-code` now requires Python 3.12 or newer. ([#5603](#5603)) ### Features - Added OpenRouter `z-ai/glm-5.3` to the model switcher. ([#5641](#5641)) - Added support for re-authenticating MCP servers from the viewer. ([#5637](#5637)) - Footer pickers can now be opened with `ctrl+click`. ([#5611](#5611)) - Resume hints now account for `TERM_PROGRAM` support before showing terminal-specific guidance. ([#5580](#5580)) ### Fixes - Completed the `dcode config` command surface. ([#5581](#5581)) - Made `/offload` interruptible. ([#5590](#5590)) - Improved chat and footer UI behavior: rapid typing stays visible, double-click collapses a resized chat input, and the MCP footer wraps on narrow windows. ([#5424](#5424), [#5578](#5578), [#5651](#5651)) - Captured stdio MCP server stderr in the logger. ([#5610](#5610)) - Drained hook pipes after timeout. ([#5606](#5606)) - Grouped resume trace rounds. ([#5593](#5593)) - Omitted web-search prompt guidance when web search is unavailable. ([#5602](#5602)) - Resolved message pointer shapes per cell. ([#5592](#5592)) _End release notes preview._ --- > [!NOTE] > A **community contributors** list and a **Special thanks** section (crediting the users who filed the issues this release's PRs closed) are appended to the GitHub release notes automatically at publish time (see [Release Pipeline](https://github.com/langchain-ai/deepagents/blob/main/.github/RELEASING.md#release-pipeline), step 3). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: langchain-oss-automated-triage[bot] <248757908+langchain-oss-automated-triage[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stdio MCP server diagnostics are now captured in DEBUG logs during tool discovery and runtime sessions. Debug log files are restricted to user-only permissions; captured output is sanitized and bounded, but can still contain sensitive server-provided values.
The MCP adapter discarded stdio server stderr, leaving subprocess failures invisible. This routes the SDK stderr pipe through a dedicated drain thread without blocking the event loop while preserving remote transports and stdio environment behavior.
Made by Open SWE
References