Stop generating MCP gateway stderr logs - #50765
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot also remove the writing of /tmp/gh-aw/mcp-logs/start-gateway.log , echo the values directly so that they are in the action log |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR #50765 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Stops MCP gateway launchers from generating redundant stderr/startup log files while retaining console diagnostics.
Changes:
- Discards gateway process stderr in shell and JavaScript launchers.
- Removes obsolete log guidance and documentation.
- Adds launcher regression tests.
Show a summary per file
| File | Description |
|---|---|
scratchpad/layout.md |
Removes the obsolete stderr log path. |
actions/setup/sh/start_mcp_gateway.sh |
Discards stderr and removes startup log capture. |
actions/setup/sh/start_mcp_gateway_test.sh |
Adds shell launcher regression coverage. |
actions/setup/sh/check_mcp_servers.sh |
Updates failure guidance. |
actions/setup/js/start_mcp_gateway.test.cjs |
Adds JavaScript launcher regression coverage. |
actions/setup/js/start_mcp_gateway.cjs |
Ignores process stderr and removes file diagnostics. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
actions/setup/sh/start_mcp_gateway_test.sh:312
- This check only rejects the two legacy filenames, so it still passes if gateway stderr is later inherited or redirected elsewhere. Assert the new
2> /dev/nullbehavior as well so the regression test covers the stated requirement to discard process stderr.
if grep -Eq "/tmp/gh-aw/mcp-logs/(stderr|start-gateway)\.log" "$SCRIPT_PATH"; then
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| describe("start_mcp_gateway logging", () => { | ||
| it("does not create the legacy MCP gateway stderr log", () => { | ||
| const source = fs.readFileSync(new URL("./start_mcp_gateway.cjs", import.meta.url), "utf8"); | ||
| expect(source).not.toContain("/tmp/gh-aw/mcp-logs/stderr.log"); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The changes cleanly remove the stderr log file (/tmp/gh-aw/mcp-logs/stderr.log) and the startup log (start-gateway.log) from the MCP gateway across both the JS and shell implementations. Tests are added to guard against regressions. No security, correctness, or reliability issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.5 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel Report 🧪PR: #50765 — "Stop generating MCP gateway stderr logs" SummaryThis PR adds a single, high-value behavioral test that directly verifies the PR's intent: preventing unwanted log file generation. The test enforces a design invariant by statically verifying that the source code does not reference the removed log path. Result: ✅ APPROVE — All tests pass quality checks. No violations. Strong design invariant enforcement. Test Classification
Test Details:
MetricsTest Inflation: 13 test lines added ÷ 40 production lines removed = 0.33:1 ✅ Scoring BreakdownConclusion✅ Quality passed. The test enforces a meaningful design invariant with no violations. Implementation ratio: 100% design tests (threshold: ≤30% implementation tests). No further action required.
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues; two minor observations worth addressing.
📋 Key Themes & Highlights
Key Themes
- Silent stderr: Both launchers now discard gateway process stderr to
/dev/null/"ignore". This is consistent and correct if the MCP Gateway Specification guarantees all errors go to stdout — but that contract is nowhere documented in the changed code, making the intent easy to misread during a future debug session. - Source-scanning regression tests: The JS and shell regression tests scan source text for the old log paths. This approach is simple and catches the most likely reintroduction vector, but it would not catch a renamed constant or a
path.join-based construction. The tests are a net positive; the suggestion is to strengthen the pattern slightly.
Positive Highlights
- ✅ Clean, consistent removal across both JS and shell launchers
- ✅ Correct removal of
set -o pipefailfrom the shell launcher (it was only meaningful whenteewas in the pipeline) - ✅ Regression tests added in both test suites — good coverage discipline
- ✅ Documentation (
scratchpad/layout.md) updated atomically with the code change
|
|
||
| const outputFd = fs.openSync(outputPath, "w", 0o600); | ||
| const stderrFd = fs.openSync(stderrLogPath, "w", 0o600); | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] Silencing stderr with "ignore" permanently discards any output the gateway process writes to stderr (Go runtime panics, library errors, etc.) with no fallback capture mechanism. If the MCP Gateway Specification guarantees all meaningful errors go to stdout, add a comment explaining that contract so the next developer does not restore stderr logging in a future debug session.
💡 Suggested comment
// MCP Gateway Specification routes all errors to stdout (gateway-output.json).
// stderr is intentionally discarded to avoid redundant log artifacts.
stdio: ["pipe", outputFd, "ignore"],@copilot please address this.
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { applyOTLPIgnoreIfMissing, detectEngineType, getJSONParseErrorContext, getOTLPIfMissingMode, hasNonEmptyOTLPHeaders, normalizeSinkVisibilityEncoding, resolveCopilotConfigPaths } from "./start_mcp_gateway.cjs"; | ||
|
|
||
| describe("start_mcp_gateway logging", () => { |
There was a problem hiding this comment.
[/tdd] Source-scanning tests (reading the .cjs file and checking it does not contain(...) a string) are fragile: a comment or a string in a different context that references the old path would produce a false positive, and the test passes even if the path is moved to a different variable name.
💡 Suggestion
Consider complementing with a behavioural assertion — e.g. verify no file is created at that path after spawning the gateway in a test fixture, or at minimum strengthen the regex to catch all forms:
it("does not reference the legacy stderr log path in any form", () => {
const source = fs.readFileSync(new URL("./start_mcp_gateway.cjs", import.meta.url), "utf8");
// Catches both string literals and path.join constructions
expect(source).not.toMatch(/mcp-logs[\/]stderr\.log/);
});@copilot please address this.
| # Pass MCP_GATEWAY_LOG_DIR to the container via -e flag | ||
| echo "$MCP_CONFIG" | MCP_GATEWAY_LOG_DIR="$MCP_GATEWAY_LOG_DIR" $MCP_GATEWAY_DOCKER_COMMAND \ | ||
| > /tmp/gh-aw/mcp-config/gateway-output.json 2> /tmp/gh-aw/mcp-logs/stderr.log & | ||
| > /tmp/gh-aw/mcp-config/gateway-output.json 2> /dev/null & |
There was a problem hiding this comment.
[/diagnosing-bugs] Same diagnostics concern as the JS launcher: redirecting to /dev/null silences all gateway process stderr permanently. If the gateway process crashes due to a missing shared library or OOM, that output is gone with no way to diagnose it post-hoc.
💡 Suggestion
Add a brief inline comment documenting why stderr is discarded:
# MCP Gateway writes all errors to stdout (gateway-output.json) per spec.
# stderr is discarded to prevent redundant log artifacts.
> /tmp/gh-aw/mcp-config/gateway-output.json 2> /dev/null &@copilot please address this.
Smoke Test: Copilot - PR SummaryPR: Stop generating MCP gateway stderr logs (#50765)
Overall: PASS Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test automated review pass.> [!WARNING]
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:network: allowed: - defaults - "accounts.google.com" - "android.clients.google.com" - "clients2.google.com" - "contentautofill.googleapis.com" - "safebrowsingohttpgateway.googleapis.com" - "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 59.5 AIC · ⌖ 2.44 AIC · ⊞ 8.6K
Comment /smoke-copilot to run again
Add label smoke to run again
|
@copilot run pr-finisher skill and fix https://github.com/github/gh-aw/actions/runs/31074728619/job/92530916599?pr=50765 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass in
Two notes: CI on this branch is stale after my push and needs a maintainer re-trigger before merge. |
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
Smoke Test SummaryPR: Stop generating MCP gateway stderr logs All 16 checks: PASS ✅ Overall status: PASS Author: app/copilot-swe-agent · Assignees: pelikhan, Copilot Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test caveman review. All look good, ugh.> [!WARNING]
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:network: allowed: - defaults - "accounts.google.com" - "android.clients.google.com" - "clients2.google.com" - "contentautofill.googleapis.com" - "safebrowsingohttpgateway.googleapis.com" - "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 48.9 AIC · ⌖ 3.66 AIC · ⊞ 8.7K
Comment /smoke-copilot to run again
Add label smoke to run again
|
🎉 This pull request is included in a new release. Release: |
The MCP gateway launchers generated
/tmp/gh-aw/mcp-logs/stderr.log, causing redundant stderr output to be included in MCP log artifacts.Gateway startup
stderr.log.Cleanup