fix: surface bundle subprocess stderr in runtime logs (#116)#132
Merged
mgoldsborough merged 3 commits intomainfrom Apr 29, 2026
Merged
fix: surface bundle subprocess stderr in runtime logs (#116)#132mgoldsborough merged 3 commits intomainfrom
mgoldsborough merged 3 commits intomainfrom
Conversation
When an MCP bundle subprocess crashes, the developer previously saw only a generic source.crashed event with no payload — the actual traceback / import error / config failure was written to stderr but nothing in NB ever read it. Two-hour debug loops were the symptom; the unconsumed pipe was the cause. - Drain transport.stderr into a per-source 50-line ring buffer and a default-on dimmed [bundle:<name>] live print. Attached at construction so initialize-time crashes are captured (the SDK exposes the PassThrough synchronously from the constructor for exactly this reason). - Add the previously missing stdio onclose handler so subprocess exits are detected eagerly instead of on the next tool call. Include stderrTail in the source.crashed event payload across all transport modes for shape consistency. - Render stderrTail in ConsoleEventSink, dimmed and indented under the error line, so the traceback is visible without digging through separate streams. - Suppress source.crashed on graceful stop() via a stopping flag — transport.close() fires onclose synchronously, which would otherwise surface every deliberate teardown as a crash. bundle stderr is intentionally default-on, not gated behind NB_DEBUG. It is the bundle author's deliberate diagnostic output (Python uses stderr for tracebacks, warnings, logs), a different concern than NB's own protocol tracing. Visual prefix + dim formatting makes the channel tunable by eye without a config flag.
Addresses QA review on PR #132. - Consolidate four near-identical onclose handlers + execute()'s catch emit into a single emitSourceCrashed helper. Adds a `dead` guard so a subprocess death observed by both the transport's onclose and the in-flight execute()'s catch fires source.crashed exactly once. Whichever path runs first wins; its payload is canonical. - Set this.stopping = true in cleanupOnStartFailure so a failed start doesn't surface as a crash event for a source the listener never saw alive. The OAuth retry path resets the flag before the second connect attempt so successful retries re-arm crash detection. - Refactor attachStderrReader to take the stream directly instead of a StdioClientTransport, which makes the chunk/CRLF/partial-line/runaway logic testable against a synthetic Readable. - Add 13 unit tests covering: chunk-boundary line splitting, partial- line buffering across chunks, end-of-stream flush, CRLF stripping, ring-buffer FIFO eviction at 50, runaway-line truncation, dead-guard de-dup, payload shape with stderrTail, graceful stop suppression, failed-start suppression, restart re-arms detection, ring-buffer reset across start cycles. - Document the default-on [bundle:<name>] channel in AGENTS.md Debugging section as `log.ts` instructed.
Contributor
Author
|
Addressed QA review (commit 9a2b3be). Fixed:
Skipped, with reasoning:
Verify: |
scripts/repro-issue-116.ts spawns a deliberately-broken Python MCP server, runs the production McpSource against it, and asserts: 1. start() throws (subprocess dies during initialize) 2. Live [bundle:...] drain rendered stderr to the developer 3. Pre-crash output was captured (proves attach-at-construction) 4. Exactly one source.crashed event was emitted (dead-guard works) 5. source.crashed.stderrTail contains the Python traceback Exits non-zero on any contract violation. Runs in ~1s and exercises the whole drain → ring buffer → console renderer → crash event chain against a real subprocess — closer to user-visible behavior than the unit tests, faster than spinning up `bun run dev` and configuring a bundle by hand.
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
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.
Closes #116.
Summary
transport.stderrinto a per-source 50-line ring buffer and a default-on dimmed[bundle:<name>]live print. Attached at construction so initialize-time crashes are captured.onclosehandler so subprocess exits are detected eagerly instead of on the next tool call. IncludestderrTailin thesource.crashedpayload across all transport modes.stderrTailinConsoleEventSink, dimmed and indented under the error line, so the traceback is visible without digging through separate streams.source.crashedon gracefulstop()via astoppingflag —transport.close()firesonclosesynchronously, which would otherwise surface every deliberate teardown as a crash. This was a latent bug across remote/in-process; adding stdio made it loud.Why default-on (not
NB_DEBUG-gated)Bundle stderr is the bundle author's deliberate diagnostic output — Python uses stderr for tracebacks, warnings, deprecation notices, logs. Different concern than NB's own protocol tracing (
NB_DEBUG=mcp). The cost asymmetry is severe: hiding signal causes the multi-hour debug loop the issue was filed about; showing dimmed prefixed lines is recoverable with shell redirection or by quieting the bundle. Visual prefix + dim formatting makes the channel tunable by eye without a config flag — same pattern as Docker /foreman/overmind.Architecture
Three concerns kept separate:
run.error.data.stderrTail(additive field for downstream consumers like a future web UI).Edge cases handled: partial lines (
print(end="")), CRLF endings, runaway-line truncation, lossy UTF-8 decode, restart cycles (ring buffer reset atstart()), graceful stops (suppressed viastoppingflag).Reader is attached at transport construction, not after
connect(), becauseStdioClientTransport.stderris aPassThroughexposed synchronously from the constructor — the SDK's own doc-comment calls this out specifically to avoid losing early child output. No race, no monkey-patching ofstart().Did NOT introduce a new
bundle.logengine event for live lines. Out of scope per the issue, and live console output covers the dev-loop case. Web UI surfacing falls out cheaply later by readingevent.data.stderrTail.Test plan
bun run verifypasses (2141 unit + 213 web + 416 integration + 17 smoke, exit 0)[bundle:echo] ...lines streaming through during real subprocess runsModuleNotFoundErrorin a bundle and confirm the traceback appears under the crash line inbun run devbun run dev, restart a bundle (stop + start) and confirm no spurioussource.crashedevents fire