Prevent MCP stdio startup diagnostics from polluting JSON-RPC stdout#26921
Merged
Prevent MCP stdio startup diagnostics from polluting JSON-RPC stdout#26921
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c706a8e3-1a8b-422e-90e9-b564187e75fe Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix MCP server writing diagnostics to stdout instead of stderr
Add MCP stdio integration test to enforce stdout/stderr stream separation
Apr 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an integration test to prevent MCP stdio-mode regressions where diagnostic/banner output leaks to stdout and corrupts JSON-RPC communication.
Changes:
- Introduces a new
//go:build integrationtest that launchesgh-aw mcp-serverwith stdio and capturesstdout/stderr. - Asserts that
stdoutremains free of non-protocol output and that diagnostics are emitted onstderr.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_server_stdio_integration_test.go | New integration test to enforce stdout/stderr separation for MCP stdio transport. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/cli/mcp_server_stdio_integration_test.go:60
- The test discards the error from cmd.Run(). If the process fails to start (bad path, permission, missing deps), the test outcome is driven indirectly by stream assertions and can be misleading. Check the error: fail fast on start failures, and if you intentionally expect a non-zero exit (e.g., due to EOF/timeout), assert that explicitly (e.g., allow *exec.ExitError or context deadline exceeded).
_ = cmd.Run()
pkg/cli/mcp_server_stdio_integration_test.go:69
- Requiring stderr to be non-empty makes this test brittle: if MCP server startup becomes silent (or diagnostics are suppressed), the test will fail even though the key contract (no non-protocol output on stdout) still holds. Consider making stderr assertions conditional (e.g., only assert that any startup text goes to stderr and stdout stays clean) or verify the process started via the cmd.Run() error handling instead of depending on stderr content.
stderrText := strings.TrimSpace(stderr.String())
if stderrText == "" {
t.Fatal("Expected MCP diagnostics on stderr, got empty output")
}
- Files reviewed: 1/1 changed files
- Comments generated: 2
Collaborator
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ced7bfd5-256b-47a5-b224-452c6cf309eb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ced7bfd5-256b-47a5-b224-452c6cf309eb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Reviewed and addressed the actionable review comments. I updated the integration test to use |
Collaborator
|
@copilot focus on this message and fix it These ⚠ ℹ ✓ ✗ diagnostic lines are written to stdout, not stderr. VS Code reads them as the first bytes of the JSON-RPC response, fails to parse them as JSON, and never receives the actual initialize response. |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Fixed in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Add MCP stdio integration test to enforce stdout/stderr stream separation
Prevent MCP stdio startup diagnostics from polluting JSON-RPC stdout
Apr 17, 2026
pelikhan
reviewed
Apr 17, 2026
| } | ||
|
|
||
| // checkAndLogGHVersion checks if gh CLI is available and logs its version | ||
| func checkAndLogGHVersion() { |
Collaborator
There was a problem hiding this comment.
@copilot use logger instead of diagnostic output
Contributor
Author
There was a problem hiding this comment.
Done in a8345cc (with follow-up docs in be474a2): I removed the diagnostic output writer plumbing and switched startup/validation diagnostics to logger-only (mcpLog/mcpValidationLog). Screenshot: N/A (no UI changes).
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/699fb236-6542-4357-9393-fd8bf353a63e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/699fb236-6542-4357-9393-fd8bf353a63e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
gh aw mcp-servermust keepstdoutreserved for JSON-RPC frames in stdio mode. Any diagnostic/banner text on the stdio channel can corrupt MCP initialize handshakes (e.g., VS Code hanging on server startup).What changed
pkg/cli/mcp_server_stdio_integration_test.go.gh aw mcp-serverin stdio mode with timeout/error handling and deterministic env setup, then asserts the JSON-RPC channel contract:stdoutis empty (no diagnostic pollution)mcpLog/mcpValidationLog.stdout, preserving JSON-RPC framing.Why this matters
> [!WARNING]
>
>