refactor: eliminate 3 duplicate code patterns across launcher and server packages by Copilot · Pull Request #3234 · github/gh-aw-mcpg

Copilot · 2026-04-05T22:16:25Z

Three recurring duplication patterns were identified across internal/launcher and internal/server: redundant dual-sink logging, duplicated HTTP body read-and-restore plumbing, and repeated env-var lookup for WASM guard discovery.

Pattern 1 — Dual stdlib/structured logging (`launcher.go`, `log_helpers.go`)

Every significant launcher event was logged twice — once via logger.LogXxxWithServer (structured file log) and again via log.Printf("[LAUNCHER]...") (stdout). Removed the redundant log.Printf calls where the structured logger already captures the same information. Kept log.Printf lines that carry unique diagnostic context (privilege warnings, command/args detail, startup hints).

Updated log_helpers_test.go expectations to match the reduced stdlib log output.

Pattern 2 — HTTP body read-and-restore (`http_helpers.go`, `sdk_logging.go`)

logHTTPRequestBody and WithSDKLogging each independently implemented:

bodyBytes, err := io.ReadAll(r.Body)
// ...
r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))

Extracted a shared peekRequestBody(r *http.Request) ([]byte, error) helper that reads, restores, and returns the body bytes. Both callers now delegate to it. Removed the now-unused bytes and io imports from sdk_logging.go.

Pattern 3 — WASM guard env-var lookup (`guard_init.go`)

findServerWASMGuardFile and logWASMGuardsDirConfiguration each called strings.TrimSpace(os.Getenv(wasmGuardsDirEnvVar)) independently. Extracted getWASMGuardsRootDir() string and updated both call sites.

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-build3552219979/b510/launcher.test /tmp/go-build3552219979/b510/launcher.test -test.testlogfile=/tmp/go-build3552219979/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
- Triggering command: /tmp/go-build3265671661/b001/launcher.test /tmp/go-build3265671661/b001/launcher.test -test.testlogfile=/tmp/go-build3265671661/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a 0123343/b286/ x_amd64/vet . ernal/mcp --64 x_amd64/vet 0123�� util.test -I 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build1678977432/b001/launcher.test /tmp/go-build1678977432/b001/launcher.test -test.testlogfile=/tmp/go-build1678977432/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W KDQapGuL- .cfg 64/pkg/tool/linux_amd64/vet . --gdwarf2 --64 64/pkg/tool/linu-importcfg 0123�� olang.org/genpro-s .cfg 64/pkg/tool/linu-buildmode=exe --gdwarf-5 .io/otel/exporte--norc -o 64/pkg/tool/linu-extld=gcc (dns block)
invalid-host-that-does-not-exist-12345.com
- Triggering command: /tmp/go-build3552219979/b492/config.test /tmp/go-build3552219979/b492/config.test -test.testlogfile=/tmp/go-build3552219979/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ce/batch_span_processor.go ce/doc.go x_amd64/compile rt-size '1280, 7/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build546762636/b496/config.test /tmp/go-build546762636/b496/config.test -test.testlogfile=/tmp/go-build546762636/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o HerihSWMV aw-mcpg/internal/difc/capabilities.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -p go/scanner -lang=go1.25 ortcfg -uns�� aw-mcpg/internal/middleware/jqschema.go aw-mcpg/internal/middleware/jqschema_bench_test.-ifaceassert docker-buildx go1.25.8 -c=4 -nolocalimports docker-buildx (dns block)
nonexistent.local
- Triggering command: /tmp/go-build3552219979/b510/launcher.test /tmp/go-build3552219979/b510/launcher.test -test.testlogfile=/tmp/go-build3552219979/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
- Triggering command: /tmp/go-build3265671661/b001/launcher.test /tmp/go-build3265671661/b001/launcher.test -test.testlogfile=/tmp/go-build3265671661/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a 0123343/b286/ x_amd64/vet . ernal/mcp --64 x_amd64/vet 0123�� util.test -I 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build1678977432/b001/launcher.test /tmp/go-build1678977432/b001/launcher.test -test.testlogfile=/tmp/go-build1678977432/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W KDQapGuL- .cfg 64/pkg/tool/linux_amd64/vet . --gdwarf2 --64 64/pkg/tool/linu-importcfg 0123�� olang.org/genpro-s .cfg 64/pkg/tool/linu-buildmode=exe --gdwarf-5 .io/otel/exporte--norc -o 64/pkg/tool/linu-extld=gcc (dns block)
slow.example.com
- Triggering command: /tmp/go-build3552219979/b510/launcher.test /tmp/go-build3552219979/b510/launcher.test -test.testlogfile=/tmp/go-build3552219979/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
- Triggering command: /tmp/go-build3265671661/b001/launcher.test /tmp/go-build3265671661/b001/launcher.test -test.testlogfile=/tmp/go-build3265671661/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a 0123343/b286/ x_amd64/vet . ernal/mcp --64 x_amd64/vet 0123�� util.test -I 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build1678977432/b001/launcher.test /tmp/go-build1678977432/b001/launcher.test -test.testlogfile=/tmp/go-build1678977432/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W KDQapGuL- .cfg 64/pkg/tool/linux_amd64/vet . --gdwarf2 --64 64/pkg/tool/linu-importcfg 0123�� olang.org/genpro-s .cfg 64/pkg/tool/linu-buildmode=exe --gdwarf-5 .io/otel/exporte--norc -o 64/pkg/tool/linu-extld=gcc (dns block)
this-host-does-not-exist-12345.com
- Triggering command: /tmp/go-build3552219979/b519/mcp.test /tmp/go-build3552219979/b519/mcp.test -test.testlogfile=/tmp/go-build3552219979/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a 330123343/b151//-ifaceassert x_amd64/vet --gdwarf-5 ternal/engine/wa-atomic 0123343/b151/ x_amd64/vet -I 64/src/os/user late/attr.go x_amd64/vet --gdwarf-5 --64 ut-2245836168.c x_amd64/vet (dns block)
- Triggering command: /tmp/go-build546762636/b523/mcp.test /tmp/go-build546762636/b523/mcp.test -test.testlogfile=/tmp/go-build546762636/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet -ato�� cron.hourly ettings.Ports}} x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet (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)

… peek, WASM guard env-var) - Pattern 1 (#3209): Remove redundant log.Printf calls in launcher.go and log_helpers.go where logger.LogXxxWithServer already covers the same info. Update tests to match new stdlib log output. - Pattern 2 (#3208): Extract peekRequestBody helper in http_helpers.go and use it in both logHTTPRequestBody and WithSDKLogging. Remove unused bytes and io imports from sdk_logging.go. - Pattern 3 (#3210): Extract getWASMGuardsRootDir helper in guard_init.go and use it in findServerWASMGuardFile and logWASMGuardsDirConfiguration. Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/d6f0cf87-de59-4348-912d-98a4373e1b0d Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot

Pull request overview

This PR refactors internal/launcher and internal/server to remove repeated patterns by consolidating shared logic and trimming redundant stdout logging where structured logging already captures the same events.

Changes:

Removed duplicated “dual-sink” launcher log lines (structured + log.Printf) and updated launcher log tests accordingly.
Extracted shared HTTP request-body “read + restore” logic into a new peekRequestBody(*http.Request) helper and reused it across request logging and SDK logging.
Centralized WASM guard root directory env-var lookup into getWASMGuardsRootDir().

Show a summary per file

File	Description
internal/server/sdk_logging.go	Uses the new shared request-body peeking helper instead of duplicating read/restore logic.
internal/server/http_helpers.go	Adds `peekRequestBody` and refactors request body logging to use it.
internal/server/guard_init.go	Extracts env-var lookup into `getWASMGuardsRootDir()` and reuses it in both call sites.
internal/launcher/log_helpers.go	Removes redundant stdout log lines now covered by structured logging.
internal/launcher/log_helpers_test.go	Updates expectations for reduced stdout logs; adjusts launch-success test.
internal/launcher/launcher.go	Removes redundant stdout logging during launcher initialization and HTTP backend setup.

Copilot's findings

Tip

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

Files reviewed: 6/6 changed files
Comments generated: 2

internal/server/http_helpers.go

Copilot · 2026-04-06T00:30:42Z

internal/launcher/log_helpers_test.go

 func TestLauncher_LogLaunchSuccess(t *testing.T) {
 	tests := []struct {
 		name      string
 		serverID  string
 		sessionID string
-		wantInLog []string
 	}{
 		{
 			name:      "success with session",
 			serverID:  "github",
 			sessionID: "session-789",
-			wantInLog: []string{
-				"Successfully launched",
-				"github",
-				"session-789",
-			},
 		},
 		{
 			name:      "success without session",
 			serverID:  "slack",
 			sessionID: "",
-			wantInLog: []string{
-				"Successfully launched",
-				"slack",
-			},
 		},
 	}

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			launcher := &Launcher{}

-			output := captureLogOutput(t, func() {
+			// logLaunchSuccess now uses only structured logging (no log.Printf);
+			// verify it runs without panic.
+			require.NotPanics(t, func() {
 				launcher.logLaunchSuccess(tt.serverID, tt.sessionID)
 			})
-
-			for _, want := range tt.wantInLog {
-				assert.Contains(t, output, want, "Expected log output to contain: %s", want)
-			}
-			assert.Contains(t, output, "[LAUNCHER]", "Expected [LAUNCHER] prefix")
 		})


TestLauncher_LogLaunchSuccess no longer asserts any observable behavior (it only checks that the helper doesn't panic). Since logLaunchSuccess still has user-visible side effects via structured logging, this test won't catch regressions (e.g., log line removed/changed). Consider either (a) removing the test if it's no longer meaningful, or (b) initializing the logger to a temp dir and asserting the expected structured log entry is written.

See below for a potential fix:

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Strengthen TestLauncher_LogLaunchSuccess: init server file logger with a temp dir and assert the structured log file contains the expected server ID and success message, rather than only checking for no panic. - Fix search_repositories URL encoding (url.QueryEscape) for query param to fix pre-existing test failure on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Initial plan

355a12a

Copilot AI assigned Copilot and lpcox Apr 5, 2026

Copilot started work on behalf of lpcox April 5, 2026 23:11 View session

Copilot AI linked an issue Apr 5, 2026 that may be closed by this pull request

[duplicate-code] Duplicate Code Analysis Report #3207

Closed

Copilot AI changed the title ~~[WIP] Analyze and resolve duplicate code patterns in internal packages~~ refactor: eliminate 3 duplicate code patterns across launcher and server packages Apr 5, 2026

Copilot AI requested a review from lpcox April 5, 2026 23:22

Copilot finished work on behalf of lpcox April 5, 2026 23:22

lpcox marked this pull request as ready for review April 6, 2026 00:27

Copilot AI review requested due to automatic review settings April 6, 2026 00:27

Copilot started reviewing on behalf of lpcox April 6, 2026 00:27 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

lpcox and others added 2 commits April 5, 2026 17:31

Update internal/server/http_helpers.go

3308570

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

github-actions bot mentioned this pull request Apr 6, 2026

[smoke-safeoutputs] Smoke Safe-Outputs PRs: 24014527922 #3245

Closed

lpcox merged commit b7479ed into main Apr 6, 2026
17 checks passed

lpcox deleted the copilot/duplicate-code-analysis-report branch April 6, 2026 01:24

This was referenced Apr 6, 2026

Smoke Test: Copilot - 24015012812 #3253

Closed

Smoke Test: Copilot - 24015156796 #3254

Closed

Smoke Test: Copilot - 24015268002 #3255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: eliminate 3 duplicate code patterns across launcher and server packages#3234

refactor: eliminate 3 duplicate code patterns across launcher and server packages#3234
lpcox merged 4 commits intomainfrom
copilot/duplicate-code-analysis-report

Copilot AI commented Apr 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Pattern 1 — Dual stdlib/structured logging (launcher.go, log_helpers.go)

Pattern 2 — HTTP body read-and-restore (http_helpers.go, sdk_logging.go)

Pattern 3 — WASM guard env-var lookup (guard_init.go)

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

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Apr 5, 2026 •

edited

Loading

Pattern 1 — Dual stdlib/structured logging (`launcher.go`, `log_helpers.go`)

Pattern 2 — HTTP body read-and-restore (`http_helpers.go`, `sdk_logging.go`)

Pattern 3 — WASM guard env-var lookup (`guard_init.go`)