Refactor: Consolidate micro-files and extract Docker helpers#1641
Merged
Refactor: Consolidate micro-files and extract Docker helpers#1641
Conversation
- Merge config_logging.go into config_payload.go (both operational defaults) - Move EnvDebug/EnvDebugColors constants from logger/constants.go into logger/logger.go - Move logRuntimeError from server/errors.go into server/auth.go (only used there) - Extract Docker helpers from validation_env.go into docker_helpers.go - Create guard_policy.go implementation file matching config_guardpolicies_test.go Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Claude
AI
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates three "micro-files" (each containing trivially small amounts of code) into their related files, extracts Docker helper functions from validation_env.go into a dedicated docker_helpers.go, and introduces a new guard_policy.go file with a type-safe wrapper for guard policy configuration.
Changes:
- Merged
config_logging.go,logger/constants.go, andserver/errors.gointo their respective related files (config_payload.go,logger.go,auth.go), eliminating 3 micro-files. - Extracted Docker inspection/validation functions from
validation_env.gointodocker_helpers.goto separate Docker I/O concerns from validation logic. - Created
guard_policy.gowith a newGuardPolicyConfigtype and helper methods for type-safe guard policy access.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/config/config_logging.go |
Deleted; DefaultLogDir constant moved to config_payload.go |
internal/config/config_payload.go |
Added DefaultLogDir constant alongside existing payload defaults |
internal/logger/constants.go |
Deleted; EnvDebug/EnvDebugColors constants moved to logger.go |
internal/logger/logger.go |
Added EnvDebug/EnvDebugColors constants where they are referenced |
internal/server/errors.go |
Deleted; logRuntimeError moved to auth.go |
internal/server/auth.go |
Added logRuntimeError function (its only call site) |
internal/config/validation_env.go |
Removed Docker helper functions and containerIDPattern regex |
internal/config/docker_helpers.go |
New file with extracted Docker inspection functions |
internal/config/guard_policy.go |
New file with GuardPolicyConfig type and helper methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2
to
+51
| // This file defines guard policy configuration and helpers. | ||
| package config | ||
|
|
||
| // GuardPolicyConfig provides a type-safe interface for accessing guard policies | ||
| // from ServerConfig. Guard policies control access to MCP server resources. | ||
| // | ||
| // Structure (GitHub MCP Server): | ||
| // - repos: "all", "public", or array of patterns (e.g., ["owner/repo", "owner/*"]) | ||
| // - min-integrity: "none", "reader", "writer", or "merged" | ||
| // | ||
| // The guard policies are stored as map[string]interface{} to support | ||
| // server-specific schemas without forcing all servers to use the same structure. | ||
| type GuardPolicyConfig struct { | ||
| policies map[string]interface{} | ||
| } | ||
|
|
||
| // NewGuardPolicyConfig creates a GuardPolicyConfig wrapper around the raw policies map. | ||
| func NewGuardPolicyConfig(policies map[string]interface{}) *GuardPolicyConfig { | ||
| if policies == nil { | ||
| return &GuardPolicyConfig{policies: make(map[string]interface{})} | ||
| } | ||
| return &GuardPolicyConfig{policies: policies} | ||
| } | ||
|
|
||
| // GetPolicy returns the policy configuration for a specific service (e.g., "github", "slack"). | ||
| // Returns nil if the service has no policy configured. | ||
| func (gp *GuardPolicyConfig) GetPolicy(service string) map[string]interface{} { | ||
| if gp.policies == nil { | ||
| return nil | ||
| } | ||
| if policy, ok := gp.policies[service].(map[string]interface{}); ok { | ||
| return policy | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // HasPolicy returns true if a policy is configured for the given service. | ||
| func (gp *GuardPolicyConfig) HasPolicy(service string) bool { | ||
| return gp.GetPolicy(service) != nil | ||
| } | ||
|
|
||
| // IsEmpty returns true if no policies are configured. | ||
| func (gp *GuardPolicyConfig) IsEmpty() bool { | ||
| return len(gp.policies) == 0 | ||
| } | ||
|
|
||
| // GetGuardPolicies is a helper method on ServerConfig for type-safe access to guard policies. | ||
| func (sc *ServerConfig) GetGuardPolicies() *GuardPolicyConfig { | ||
| return NewGuardPolicyConfig(sc.GuardPolicies) | ||
| } |
There was a problem hiding this comment.
The GuardPolicyConfig type and all its methods (NewGuardPolicyConfig, GetPolicy, HasPolicy, IsEmpty, GetGuardPolicies) are currently unused anywhere in the codebase — they are dead code with no callers and no tests. The existing config_guardpolicies_test.go tests the raw GuardPolicies map on ServerConfig directly, not through these new helpers.
If these are intended for future use, consider either adding tests now (to match the pattern of the existing config_guardpolicies_test.go) or deferring this code until there are actual consumers. Introducing untested, unused code increases maintenance burden without providing value.
Suggested change
| // This file defines guard policy configuration and helpers. | |
| package config | |
| // GuardPolicyConfig provides a type-safe interface for accessing guard policies | |
| // from ServerConfig. Guard policies control access to MCP server resources. | |
| // | |
| // Structure (GitHub MCP Server): | |
| // - repos: "all", "public", or array of patterns (e.g., ["owner/repo", "owner/*"]) | |
| // - min-integrity: "none", "reader", "writer", or "merged" | |
| // | |
| // The guard policies are stored as map[string]interface{} to support | |
| // server-specific schemas without forcing all servers to use the same structure. | |
| type GuardPolicyConfig struct { | |
| policies map[string]interface{} | |
| } | |
| // NewGuardPolicyConfig creates a GuardPolicyConfig wrapper around the raw policies map. | |
| func NewGuardPolicyConfig(policies map[string]interface{}) *GuardPolicyConfig { | |
| if policies == nil { | |
| return &GuardPolicyConfig{policies: make(map[string]interface{})} | |
| } | |
| return &GuardPolicyConfig{policies: policies} | |
| } | |
| // GetPolicy returns the policy configuration for a specific service (e.g., "github", "slack"). | |
| // Returns nil if the service has no policy configured. | |
| func (gp *GuardPolicyConfig) GetPolicy(service string) map[string]interface{} { | |
| if gp.policies == nil { | |
| return nil | |
| } | |
| if policy, ok := gp.policies[service].(map[string]interface{}); ok { | |
| return policy | |
| } | |
| return nil | |
| } | |
| // HasPolicy returns true if a policy is configured for the given service. | |
| func (gp *GuardPolicyConfig) HasPolicy(service string) bool { | |
| return gp.GetPolicy(service) != nil | |
| } | |
| // IsEmpty returns true if no policies are configured. | |
| func (gp *GuardPolicyConfig) IsEmpty() bool { | |
| return len(gp.policies) == 0 | |
| } | |
| // GetGuardPolicies is a helper method on ServerConfig for type-safe access to guard policies. | |
| func (sc *ServerConfig) GetGuardPolicies() *GuardPolicyConfig { | |
| return NewGuardPolicyConfig(sc.GuardPolicies) | |
| } | |
| // This file previously defined guard policy configuration helpers. | |
| package config |
Comment on lines
12
to
+15
|
|
||
| // logRuntimeError logs runtime errors to stdout per spec section 9.2 | ||
| func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) { | ||
| logAuth.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail) |
There was a problem hiding this comment.
The logRuntimeError function previously used logErrors = logger.New("server:errors") (defined in the deleted errors.go), but now uses logAuth which logs under the "server:auth" namespace. This changes the debug log namespace for runtime error messages. While minor (typical usage is DEBUG=server:* or DEBUG=*), anyone filtering by DEBUG=server:errors would no longer see these messages. Consider whether this namespace change is intentional.
Suggested change
| // logRuntimeError logs runtime errors to stdout per spec section 9.2 | |
| func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) { | |
| logAuth.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail) | |
| var logErrors = logger.New("server:errors") | |
| // logRuntimeError logs runtime errors to stdout per spec section 9.2 | |
| func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) { | |
| logErrors.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail) |
This was referenced Mar 6, 2026
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.
Analysis of 71 Go files identified micro-files fragmenting trivially small code, Docker helpers misplaced in validation logic, and a test file without a matching implementation file.
Changes
Merged operational defaults
DefaultLogDirwithDefaultPayloadDirandDefaultPayloadSizeThresholdConsolidated tightly-coupled code
EnvDebugandEnvDebugColorsto where they're referencedlogRuntimeErrorto its only call site (2 uses in auth middleware)Separated concerns
validation_env.gorunDockerInspect,checkDockerAccessible,checkPortMapping,checkStdinInteractive,checkLogDirMountedEstablished implementation file
config_guardpolicies_test.gopatternGuardPolicyConfigtype with helper methods (GetPolicy,HasPolicy,IsEmpty)Impact
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/tmp/go-build3487354539/b297/launcher.test /tmp/go-build3487354539/b297/launcher.test -test.testlogfile=/tmp/go-build3487354539/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 9526385/b231/_pkg_.a .cfg .12/x64/bin/as -p b/gh-aw-mcpg/int-d -lang=go1.25 ache/go/1.25.7/x64/pkg/tool/linuHEAD(dns block)/tmp/go-build2202525201/b297/launcher.test /tmp/go-build2202525201/b297/launcher.test -test.testlogfile=/tmp/go-build2202525201/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true cal/bin/git x_amd64/vet 7354539/b325/timeutil.test uRC0HPMQT 9526385/b019/vet--version tnet/tools/as 7354539/b325/timeutil.test e=/t�� t0 m0s ker/cli-plugins/docker-compose -s -w -buildmode=exe ker/cli-plugins/docker-compose(dns block)/tmp/go-build2711332198/b297/launcher.test /tmp/go-build2711332198/b297/launcher.test -test.testlogfile=/tmp/go-build2711332198/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref ude-agent-sdk/vendor/ripgrep/x64github.com/github/gh-aw-mcpg/internal/mcp /usr/bin/base64 se 9526385/b220/vet-d k/_temp/ghcca-node/node/bin/git base64 -d --64 as docker-buildx -I /tmp/go-build114/usr/bin/runc x_amd64/vet docker-buildx(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build3487354539/b279/config.test /tmp/go-build3487354539/b279/config.test -test.testlogfile=/tmp/go-build3487354539/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 9526385/b229/_pkg_.a .cfg de/node/bin/as -p Sushi/toml/inter-d -lang=go1.25 /x86_64-linux-gnu/13/cc1 -I 64/src/net -I 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linux_amd64/vet(dns block)/tmp/go-build2202525201/b279/config.test /tmp/go-build2202525201/b279/config.test -test.testlogfile=/tmp/go-build2202525201/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 10 x_amd64/vet ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet 9526385/b038/_pkbase64 .cfg ache/go/1.25.7/x-lang=go1.25 ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet rev-�� --abbrev-ref HEAD docker-buildx 64/src/runtime/cbase64 HEAD 64/pkg/tool/linu-importcfg docker-buildx(dns block)/tmp/go-build2711332198/b279/config.test /tmp/go-build2711332198/b279/config.test -test.testlogfile=/tmp/go-build2711332198/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -o as ache/go/1.25.7/x64/bin/git ache/go/1.25.7/xgit 64/src/crypto/tlrev-parse ache/go/1.25.7/x--abbrev-ref git rev-�� --abbrev-ref HEAD /usr/bin/base64 se 9526385/b160/vet-d tnet/tools/git base64(dns block)nonexistent.local/tmp/go-build3487354539/b297/launcher.test /tmp/go-build3487354539/b297/launcher.test -test.testlogfile=/tmp/go-build3487354539/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 9526385/b231/_pkg_.a .cfg .12/x64/bin/as -p b/gh-aw-mcpg/int-d -lang=go1.25 ache/go/1.25.7/x64/pkg/tool/linuHEAD(dns block)/tmp/go-build2202525201/b297/launcher.test /tmp/go-build2202525201/b297/launcher.test -test.testlogfile=/tmp/go-build2202525201/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true cal/bin/git x_amd64/vet 7354539/b325/timeutil.test uRC0HPMQT 9526385/b019/vet--version tnet/tools/as 7354539/b325/timeutil.test e=/t�� t0 m0s ker/cli-plugins/docker-compose -s -w -buildmode=exe ker/cli-plugins/docker-compose(dns block)/tmp/go-build2711332198/b297/launcher.test /tmp/go-build2711332198/b297/launcher.test -test.testlogfile=/tmp/go-build2711332198/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref ude-agent-sdk/vendor/ripgrep/x64github.com/github/gh-aw-mcpg/internal/mcp /usr/bin/base64 se 9526385/b220/vet-d k/_temp/ghcca-node/node/bin/git base64 -d --64 as docker-buildx -I /tmp/go-build114/usr/bin/runc x_amd64/vet docker-buildx(dns block)slow.example.com/tmp/go-build3487354539/b297/launcher.test /tmp/go-build3487354539/b297/launcher.test -test.testlogfile=/tmp/go-build3487354539/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 9526385/b231/_pkg_.a .cfg .12/x64/bin/as -p b/gh-aw-mcpg/int-d -lang=go1.25 ache/go/1.25.7/x64/pkg/tool/linuHEAD(dns block)/tmp/go-build2202525201/b297/launcher.test /tmp/go-build2202525201/b297/launcher.test -test.testlogfile=/tmp/go-build2202525201/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true cal/bin/git x_amd64/vet 7354539/b325/timeutil.test uRC0HPMQT 9526385/b019/vet--version tnet/tools/as 7354539/b325/timeutil.test e=/t�� t0 m0s ker/cli-plugins/docker-compose -s -w -buildmode=exe ker/cli-plugins/docker-compose(dns block)/tmp/go-build2711332198/b297/launcher.test /tmp/go-build2711332198/b297/launcher.test -test.testlogfile=/tmp/go-build2711332198/b297/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref ude-agent-sdk/vendor/ripgrep/x64github.com/github/gh-aw-mcpg/internal/mcp /usr/bin/base64 se 9526385/b220/vet-d k/_temp/ghcca-node/node/bin/git base64 -d --64 as docker-buildx -I /tmp/go-build114/usr/bin/runc x_amd64/vet docker-buildx(dns block)this-host-does-not-exist-12345.com/tmp/go-build3487354539/b306/mcp.test /tmp/go-build3487354539/b306/mcp.test -test.testlogfile=/tmp/go-build3487354539/b306/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net ache/go/1.25.7/x64/src/net/http/internal/testcert/testcert.go cal/bin/as -I /tmp/go-build114rev-parse -I 02.o -o 64/src/net httptest/httptest.go 64/pkg/tool/linux_amd64/compile -p github.com/githu-d -lang=go1.25 64/pkg/tool/linux_amd64/compile(dns block)/tmp/go-build2202525201/b306/mcp.test /tmp/go-build2202525201/b306/mcp.test -test.testlogfile=/tmp/go-build2202525201/b306/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true bin/git x_amd64/vet /opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/link se 9526385/b030/vet/usr/bin/runc 64/pkg/tool/linu--version /opt/hostedtoolcache/go/1.25.7/xHEAD -o /tmp/go-build3487354539/b224/cmd.test -importcfg ker/cli-plugins/docker-compose -s -w -buildmode=exe ker/cli-plugins/docker-compose(dns block)/tmp/go-build731852698/b310/mcp.test /tmp/go-build731852698/b310/mcp.test -test.testlogfile=/tmp/go-build731852698/b310/testlog.txt -test.paniconexit0 -test.timeout=10m0s dock�� ache/uv/0.10.9/x-stringintconv git ker/docker-init --abbrev-ref HEAD ver.test ker/docker-init exit�� --abbrev-ref HEAD ash -lang=go1.25 git ker/docker-init git(dns block)If you need me to access, download, or install something from one of these locations, you can either: