refactor: Extract shared `LogConnectionError` to eliminate duplicate connection error diagnostics by Copilot · Pull Request #2524 · github/gh-aw-mcpg

Copilot · 2026-03-25T17:39:12Z

Connection error diagnostic logging was duplicated across mcp/connection.go and launcher/log_helpers.go — each independently printing command/args, error, stderr, and hint analysis for known error strings (executable file not found, EOF, broken pipe). Adding a new diagnostic hint required updating both sites; divergence meant inconsistent output depending on which code path triggered the failure.

Changes

internal/mcp/errors.go (new): Defines ConnectionErrorContext and LogConnectionError — the single source of truth for connection failure diagnostics. Merges hint logic from both original sites: containerization hints (IsDirectCommand + RunningInContainer) and error-string hints. Fields omit gracefully when zero-valued, so both callers can populate only what they know.
internal/mcp/connection.go: Replaces ~35-line inline error block with a LogConnectionError call passing {ServerID, Command, Args, StderrOutput}.
internal/launcher/log_helpers.go: logLaunchError delegates entirely to mcp.LogConnectionError with full launcher context (SessionID, Env, RunningInContainer, IsDirectCommand, StartupTimeout).
internal/mcp/errors_test.go (new): Tests covering all hint branches, stderr rendering, arg sanitization, and edge cases.

// Before: each site had its own copy of this logic
if strings.Contains(err.Error(), "executable file not found") || ... {
    log.Printf("⚠️  Command '%s' not found in PATH", command)
    ...
}

// After: one shared function with full context
mcp.LogConnectionError(mcp.ConnectionErrorContext{
    ServerID:           serverID,
    SessionID:          sessionID,
    Command:            serverCfg.Command,
    Args:               serverCfg.Args,
    Env:                serverCfg.Env,
    RunningInContainer: l.runningInContainer,
    IsDirectCommand:    isDirectCommand,
    StartupTimeout:     l.startupTimeout,
}, err)

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:

example.com
- Triggering command: /tmp/go-build3051059008/b329/launcher.test /tmp/go-build3051059008/b329/launcher.test -test.testlogfile=/tmp/go-build3051059008/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go dsa/ecdsa.go p/bin/bash (dns block)
- Triggering command: /tmp/go-build2944181223/b333/launcher.test /tmp/go-build2944181223/b333/launcher.test -test.testlogfile=/tmp/go-build2944181223/b333/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� d -n 10 stmain.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/link (compatibility issue with Go 1.25.0). Continuing with other checks..."; \ elif command -v golan telabs/wazero/in/tmp/go-build4284220926/b202/vet.cfg --64 ache/go/1.25.8/x64/pkg/tool/linux_amd64/link -I 1059008/b224/cmd.test /launcher/log_helpers.go ker/docker-init -pthread -Wl,--no-gc-sectdocker-cli-plugin-metadata -fmessage-length=0 ker/docker-init (dns block)
- Triggering command: /tmp/go-build2960403912/b329/launcher.test /tmp/go-build2960403912/b329/launcher.test -test.testlogfile=/tmp/go-build2960403912/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4181223/b314/_pk/run/containerd/io.containerd.runtime.v2.task/moby/83010862c130074fa53cbdec2f66a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline by/3d68fe704b33a--log-format by/a44f1c6d2c345json by/3d68fe704b33a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline 300bcc2a4ea80026start 64/pkg/tool/linu83010862c130074fa53cbdec2f66a97b6b568134d8551ff471fd0b14e7d12390 /opt/hostedtoolcache/go/1.25.8/x/var/run/docker/runtime-runc/moby d945�� irdJ/-TWaY8JNiNqMr3rjirdJ 64/pkg/tool/linux_amd64/asm /opt/hostedtoolcache/go/1.25.8/xjson /tmp/go-build305/usr/lib/open-iscsi/net-interface-handler 1059008/b165/ x_amd64/compile 4181223/b314/importcfg (dns block)
invalid-host-that-does-not-exist-12345.com
- Triggering command: /tmp/go-build3051059008/b314/config.test /tmp/go-build3051059008/b314/config.test -test.testlogfile=/tmp/go-build3051059008/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go Pe6rubutY x_amd64/vet copilot.original/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build2944181223/b318/config.test /tmp/go-build2944181223/b318/config.test -test.testlogfile=/tmp/go-build2944181223/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s (dns block)
nonexistent.local
- Triggering command: /tmp/go-build3051059008/b329/launcher.test /tmp/go-build3051059008/b329/launcher.test -test.testlogfile=/tmp/go-build3051059008/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go dsa/ecdsa.go p/bin/bash (dns block)
- Triggering command: /tmp/go-build2944181223/b333/launcher.test /tmp/go-build2944181223/b333/launcher.test -test.testlogfile=/tmp/go-build2944181223/b333/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� d -n 10 stmain.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/link (compatibility issue with Go 1.25.0). Continuing with other checks..."; \ elif command -v golan telabs/wazero/in/tmp/go-build4284220926/b202/vet.cfg --64 ache/go/1.25.8/x64/pkg/tool/linux_amd64/link -I 1059008/b224/cmd.test /launcher/log_helpers.go ker/docker-init -pthread -Wl,--no-gc-sectdocker-cli-plugin-metadata -fmessage-length=0 ker/docker-init (dns block)
- Triggering command: /tmp/go-build2960403912/b329/launcher.test /tmp/go-build2960403912/b329/launcher.test -test.testlogfile=/tmp/go-build2960403912/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4181223/b314/_pk/run/containerd/io.containerd.runtime.v2.task/moby/83010862c130074fa53cbdec2f66a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline by/3d68fe704b33a--log-format by/a44f1c6d2c345json by/3d68fe704b33a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline 300bcc2a4ea80026start 64/pkg/tool/linu83010862c130074fa53cbdec2f66a97b6b568134d8551ff471fd0b14e7d12390 /opt/hostedtoolcache/go/1.25.8/x/var/run/docker/runtime-runc/moby d945�� irdJ/-TWaY8JNiNqMr3rjirdJ 64/pkg/tool/linux_amd64/asm /opt/hostedtoolcache/go/1.25.8/xjson /tmp/go-build305/usr/lib/open-iscsi/net-interface-handler 1059008/b165/ x_amd64/compile 4181223/b314/importcfg (dns block)
slow.example.com
- Triggering command: /tmp/go-build3051059008/b329/launcher.test /tmp/go-build3051059008/b329/launcher.test -test.testlogfile=/tmp/go-build3051059008/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go dsa/ecdsa.go p/bin/bash (dns block)
- Triggering command: /tmp/go-build2944181223/b333/launcher.test /tmp/go-build2944181223/b333/launcher.test -test.testlogfile=/tmp/go-build2944181223/b333/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� d -n 10 stmain.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/link (compatibility issue with Go 1.25.0). Continuing with other checks..."; \ elif command -v golan telabs/wazero/in/tmp/go-build4284220926/b202/vet.cfg --64 ache/go/1.25.8/x64/pkg/tool/linux_amd64/link -I 1059008/b224/cmd.test /launcher/log_helpers.go ker/docker-init -pthread -Wl,--no-gc-sectdocker-cli-plugin-metadata -fmessage-length=0 ker/docker-init (dns block)
- Triggering command: /tmp/go-build2960403912/b329/launcher.test /tmp/go-build2960403912/b329/launcher.test -test.testlogfile=/tmp/go-build2960403912/b329/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4181223/b314/_pk/run/containerd/io.containerd.runtime.v2.task/moby/83010862c130074fa53cbdec2f66a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline by/3d68fe704b33a--log-format by/a44f1c6d2c345json by/3d68fe704b33a/usr/lib/networkd-dispatcher/off.d/chrony-onoffline 300bcc2a4ea80026start 64/pkg/tool/linu83010862c130074fa53cbdec2f66a97b6b568134d8551ff471fd0b14e7d12390 /opt/hostedtoolcache/go/1.25.8/x/var/run/docker/runtime-runc/moby d945�� irdJ/-TWaY8JNiNqMr3rjirdJ 64/pkg/tool/linux_amd64/asm /opt/hostedtoolcache/go/1.25.8/xjson /tmp/go-build305/usr/lib/open-iscsi/net-interface-handler 1059008/b165/ x_amd64/compile 4181223/b314/importcfg (dns block)
this-host-does-not-exist-12345.com
- Triggering command: /tmp/go-build3051059008/b338/mcp.test /tmp/go-build3051059008/b338/mcp.test -test.testlogfile=/tmp/go-build3051059008/b338/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-p zGGbXQmyc ache/go/1.25.8/x-lang=go1.24 (dns block)
- Triggering command: /tmp/go-build2944181223/b342/mcp.test /tmp/go-build2944181223/b342/mcp.test -test.testlogfile=/tmp/go-build2944181223/b342/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� pkg/mod/github.com/segmentio/asm@v1.1.3/cpu/arm6+30 /tmp/go-build3051059008/b122/vet.cfg x_amd64/link -b copilot/duplicat/usr/bin/runc 64/bin/bash x_amd64/link -o /tmp/go-build3051059008/b297/_pkg_.a -trimpath x_amd64/vet -p qV/wxj7iTUpwRLRu/usr/bin/runc -lang=go1.17 x_amd64/vet (dns block)
- Triggering command: /tmp/go-build2960403912/b338/mcp.test /tmp/go-build2960403912/b338/mcp.test -test.testlogfile=/tmp/go-build2960403912/b338/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

…to deduplicate connection error diagnostics Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/f1398685-5ca9-46a3-9c73-fc8c4e04814b

Copilot

Pull request overview

This PR refactors MCP backend connection failure diagnostics by extracting duplicated logging/hint logic into a shared helper in internal/mcp, and updating both the launcher and MCP connection path to use it.

Changes:

Added ConnectionErrorContext + LogConnectionError as the centralized connection-failure diagnostic logger.
Replaced inline connection-failure diagnostics in internal/mcp/connection.go with a LogConnectionError call.
Replaced launcher’s logLaunchError implementation with a delegated mcp.LogConnectionError call, and added tests for the shared helper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`internal/mcp/errors.go`	New shared connection error diagnostic logger and context struct.
`internal/mcp/errors_test.go`	New tests covering hint branches, stderr rendering, sanitization, and edge cases.
`internal/mcp/connection.go`	Uses `LogConnectionError` instead of inline diagnostics on connect failure.
`internal/launcher/log_helpers.go`	Delegates `logLaunchError` to `mcp.LogConnectionError` with richer launcher context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T19:24:33Z

internal/mcp/connection.go

+		LogConnectionError(ConnectionErrorContext{
+			ServerID:     serverID,
+			Command:      command,
+			Args:         expandedArgs,
+			StderrOutput: stderrOutput,
+		}, err)


NewConnection now calls LogConnectionError on connect failure, but the launcher also calls logLaunchError (which delegates to mcp.LogConnectionError) when NewConnection returns an error. This results in duplicate, near-identical diagnostic blocks being printed for the same failure. Consider making only the launcher responsible for human-readable diagnostics (remove this call), or add an option/context flag so NewConnection can emit only structured logs and let callers decide whether to print the console diagnostics.

Suggested change

LogConnectionError(ConnectionErrorContext{

ServerID: serverID,

Command: command,

Args: expandedArgs,

StderrOutput: stderrOutput,

}, err)

if stderrOutput != "" {

logConn.Printf("MCP server stderr before connection failure (serverID=%s, command=%s): %s", serverID, command, sanitize.SanitizeString(stderrOutput))

}

Copilot · 2026-03-25T19:24:33Z

internal/mcp/errors.go

+	if errCtx.RunningInContainer || errCtx.IsDirectCommand {
+		log.Printf("[LAUNCHER]   - Running in container: %v", errCtx.RunningInContainer)


The doc comment says “Fields left at their zero values are omitted from the output,” but the implementation prints both boolean fields whenever either is true (e.g., it will print Is direct command: false when only RunningInContainer is true). Either adjust the behavior to truly omit zero-value fields, or update the comment to describe the actual output rules so callers/tests don’t rely on an inaccurate contract.

Suggested change

if errCtx.RunningInContainer || errCtx.IsDirectCommand {

log.Printf("[LAUNCHER] - Running in container: %v", errCtx.RunningInContainer)

if errCtx.RunningInContainer {

log.Printf("[LAUNCHER] - Running in container: %v", errCtx.RunningInContainer)

}

if errCtx.IsDirectCommand {

Copilot · 2026-03-25T19:24:33Z

internal/mcp/errors.go

+	// Log captured stderr output from the container/process.
+	if errCtx.StderrOutput != "" {
+		sanitizedStderr := sanitize.SanitizeString(errCtx.StderrOutput)
+		logger.LogErrorMd("backend", "MCP backend stderr output:\n%s", sanitizedStderr)
+		log.Printf("[LAUNCHER]   📋 Process stderr output:")
+		for _, line := range strings.Split(sanitizedStderr, "\n") {
+			log.Printf("[LAUNCHER]      %s", line)
+		}
+	}


StderrOutput is treated as “present” whenever it’s a non-empty string. If a caller passes whitespace-only stderr (or stderr with trailing newlines), this will still emit a stderr section with blank lines. Since the goal is to omit fields when effectively empty, consider trimming (strings.TrimSpace) inside LogConnectionError before the emptiness check so callers don’t need to remember to pre-trim.

Copilot · 2026-03-25T19:24:34Z

internal/mcp/errors_test.go

+			// TrimSpace is done by the caller before passing to LogConnectionError
+			// so we test that empty string (after trim) produces no stderr block.


The “whitespace-only stderr is empty” case doesn’t assert anything, and the comment claims trimming is done by the caller (which isn’t enforced by LogConnectionError). This test won’t catch regressions and may encode the wrong contract. Add an assertion that whitespace-only stderr does not emit the stderr block (or, if the function is intended to log it, assert the opposite) and align the comment with the actual behavior/contract.

Suggested change

// TrimSpace is done by the caller before passing to LogConnectionError

// so we test that empty string (after trim) produces no stderr block.

// Whitespace-only stderr should be treated as empty and produce no stderr block.

notInLog: []string{

"📋 Process stderr output:",

},

Initial plan

18ad57a

Copilot AI assigned Copilot and lpcox Mar 25, 2026

Copilot AI linked an issue Mar 25, 2026 that may be closed by this pull request

[duplicate-code] Duplicate Code Pattern: Repeated Connection Error Diagnostics Across Launcher and MCP Packages #2496

Closed

7 tasks

Copilot started work on behalf of lpcox March 25, 2026 17:39 View session

refactor: extract shared ConnectionErrorContext + LogConnectionError …

1a97a0e

…to deduplicate connection error diagnostics Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/f1398685-5ca9-46a3-9c73-fc8c4e04814b

Copilot AI changed the title ~~[WIP] Refactor duplicate connection error logging across files~~ refactor: Extract shared LogConnectionError to eliminate duplicate connection error diagnostics Mar 25, 2026

Copilot AI requested a review from lpcox March 25, 2026 17:55

Copilot finished work on behalf of lpcox March 25, 2026 17:55

lpcox marked this pull request as ready for review March 25, 2026 19:20

Copilot AI review requested due to automatic review settings March 25, 2026 19:20

lpcox merged commit 6c41ec9 into main Mar 25, 2026
28 checks passed

lpcox deleted the copilot/duplicate-code-connection-error-logging branch March 25, 2026 19:20

Copilot started reviewing on behalf of lpcox March 25, 2026 19:21 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

This was referenced Mar 25, 2026

Smoke Test: Copilot - 23559737552 #2535

Closed

Smoke Test: Copilot - 23559972825 #2536

Closed

Smoke Test: Copilot - 23562176347 #2538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Extract shared `LogConnectionError` to eliminate duplicate connection error diagnostics#2524

refactor: Extract shared `LogConnectionError` to eliminate duplicate connection error diagnostics#2524
lpcox merged 2 commits intomainfrom
copilot/duplicate-code-connection-error-logging

Copilot AI commented Mar 25, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if errCtx.RunningInContainer \|\| errCtx.IsDirectCommand {
		log.Printf("[LAUNCHER] - Running in container: %v", errCtx.RunningInContainer)

		// TrimSpace is done by the caller before passing to LogConnectionError
		// so we test that empty string (after trim) produces no stderr block.

-			// TrimSpace is done by the caller before passing to LogConnectionError
-			// so we test that empty string (after trim) produces no stderr block.
+			// Whitespace-only stderr should be treated as empty and produce no stderr block.
+			notInLog: []string{
+				"📋 Process stderr output:",
+			},

Conversation

Copilot AI commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Mar 25, 2026 •

edited

Loading