Skip to content

Commit cb39742

Browse files
authored
Refactor: Code organization improvements from semantic clustering analysis (#1068)
Semantic function clustering analysis identified 4 high-priority refactoring opportunities: duplicate auth middleware application, misplaced generic utilities, schema transformation mixed with type definitions, and unnecessary wrapper indirection. ## Changes ### Extract auth middleware helper - Replaced duplicate conditional auth application with `applyAuthIfConfigured()` helper - Eliminates 8 lines of duplication in `internal/server/transport.go` **Before:** ```go finalHandler := shutdownHandler if apiKey != "" { finalHandler = authMiddleware(apiKey, shutdownHandler.ServeHTTP) } // ... repeated for closeHandler ``` **After:** ```go finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP) finalCloseHandler := applyAuthIfConfigured(apiKey, closeHandler.ServeHTTP) ``` ### Move runtime error logging to dedicated file - Created `internal/server/errors.go` for error handling utilities - Moved `logRuntimeError()` from `auth.go` (was incorrectly placed in auth-specific file) ### Separate schema transformation from type definitions - Created `internal/mcp/schema.go` for 86-line `NormalizeInputSchema()` function - `internal/mcp/types.go` now contains only type definitions per Go conventions ### Remove validation wrapper - Removed thin `validateServerConfig()` wrapper that only passed `nil` - Tests call `validateServerConfigWithCustomSchemas()` directly ## Files Changed - Modified: `internal/server/transport.go`, `internal/server/auth.go`, `internal/mcp/types.go`, `internal/config/validation.go`, `internal/config/validation_test.go` - Added: `internal/server/errors.go`, `internal/mcp/schema.go` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1870635187/b275/launcher.test /tmp/go-build1870635187/b275/launcher.test -test.testlogfile=/tmp/go-build1870635187/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ain cfg s.test --gdwarf-5 --64 -o s.test 6525�� ache/go/1.25.7/x-s -I 86_64/git --gdwarf-5 --64 -o 256506/b165/_x00-extld=gcc` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1870635187/b260/config.test /tmp/go-build1870635187/b260/config.test -test.testlogfile=/tmp/go-build1870635187/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true d -n 10 5877973&#43;lpcox@users.noreply.github.com&gt; 64/pkg/tool/linux_amd64/vet -p net/netip -lang=go1.25 64/pkg/tool/linux_amd64/vet -I /opt/hostedtoolcache/go/1.25.7/x64/src/runtime/cgo 256506/b165/ x_amd64/link --gdwarf-5 --64` (dns block) > - Triggering command: `/tmp/go-build3740779677/b001/config.test /tmp/go-build3740779677/b001/config.test -test.testlogfile=/tmp/go-build3740779677/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� on-clustering-again /tmp/go-build165256506/b208/vet.cfg .12/x64/bin/bash` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1870635187/b275/launcher.test /tmp/go-build1870635187/b275/launcher.test -test.testlogfile=/tmp/go-build1870635187/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ain cfg s.test --gdwarf-5 --64 -o s.test 6525�� ache/go/1.25.7/x-s -I 86_64/git --gdwarf-5 --64 -o 256506/b165/_x00-extld=gcc` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1870635187/b275/launcher.test /tmp/go-build1870635187/b275/launcher.test -test.testlogfile=/tmp/go-build1870635187/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ain cfg s.test --gdwarf-5 --64 -o s.test 6525�� ache/go/1.25.7/x-s -I 86_64/git --gdwarf-5 --64 -o 256506/b165/_x00-extld=gcc` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1870635187/b284/mcp.test /tmp/go-build1870635187/b284/mcp.test -test.testlogfile=/tmp/go-build1870635187/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rverConfigWithCustomSchemas 256506/b082/vet.cfg cfg --gdwarf-5 ut-2576538208.c -o ache/go/1.25.7/x-importcfg` (dns block) > - Triggering command: `/tmp/go-build3437160695/b284/mcp.test /tmp/go-build3437160695/b284/mcp.test -test.testlogfile=/tmp/go-build3437160695/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1870635187/b305/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/version/version.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/version/version_test.go -qui�� ain /opt/hostedtoolcache/go/1.25.7/x64/src/net x_amd64/compile /tmp/go-build165/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/compile -imultiarch x86_64-linux-gnu/tmp/go-build1870635187/b306/_pkg_.a x_amd64/compile` (dns block) > - Triggering command: `/tmp/go-build880157664/b284/mcp.test /tmp/go-build880157664/b284/mcp.test -test.testlogfile=/tmp/go-build880157664/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true C77u/Ma68ZNCRMnumNFLzC77u /tmp/go-build165256506/b065/vet.cfg k/_temp/ghcca-node/node/bin/sh go1.25.7 -c=4 -nolocalimports 256506/b258/importcfg -plu�� celain --ignore-submodules | head -n 10 nction Co-authored-by: lpcox &lt;15877973&#43;lpcox@users.noreply.github.com&gt; 64/bin/git -plugin-opt=-pas/usr/libexec/docker/cli-plugins/docker-buildx -plugin-opt=-pasdocker-cli-plugin-metadata -plugin-opt=-pass-through=-lpthr(create|run) /opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_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](https://gh.io/copilot/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](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>[refactor] Semantic Function Clustering Analysis - Code Organization & Refactoring Opportunities</issue_title> <issue_description>## Executive Summary Analyzed **63 Go source files** (excluding tests) across **16 packages** containing **~450 functions** to identify refactoring opportunities through semantic function clustering and duplicate detection. **Key Findings:** - ✅ **Overall code quality is excellent** - well-organized with strong use of Go patterns - 🔧 **8 high-priority refactoring opportunities** identified (duplicate logic, misplaced functions) - 📊 **Function clusters are generally well-organized** by file purpose - ⚠️ **Minor inconsistencies** in naming patterns and file organization **Analysis covered:** - 16 packages in `internal/` directory - Pattern analysis using semantic clustering - Duplicate detection across files - File organization validation --- ## Package Overview ### Files by Package | Package | Files | Primary Purpose | |---------|-------|----------------| | **logger** | 14 | Debug logging framework, file/markdown/JSONL logging, RPC logging | | **server** | 10 | HTTP server (routed/unified modes), handlers, middleware | | **config** | 8 | Configuration parsing (TOML/JSON), validation, variable expansion | | **cmd** | 7 | CLI (Cobra), flag management, completions | | **difc** | 5 | Data Information Flow Control labels and evaluation | | **guard** | 4 | Security guard framework and registry | | **testutil/mcptest** | 4 | Test utilities for MCP integration | | **launcher** | 3 | Backend process management and connection pooling | | **mcp** | 2 | MCP protocol types and connections | | **tty** | 2 | Terminal detection utilities | | **auth** | 1 | Authentication header parsing | | **envutil** | 1 | Environment variable utilities | | **middleware** | 1 | HTTP middleware (jq schema processing) | | **sys** | 1 | System utilities | | **timeutil** | 1 | Time formatting utilities | | **version** | 1 | Version management | --- ## Identified Issues ### Priority 1: High-Impact Refactoring #### 1. **Duplicate Auth Middleware Application** (server package) **Issue**: Auth middleware is applied with identical pattern in two places. **Location**: `internal/server/transport.go:120-122` and `137-139` **Current Code Pattern**: ````go // Pattern repeated twice finalHandler := shutdownHandler if apiKey != "" { finalHandler = authMiddleware(apiKey, shutdownHandler.ServeHTTP) } finalCloseHandler := closeHandler if apiKey != "" { finalCloseHandler = authMiddleware(apiKey, closeHandler.ServeHTTP) } ```` **Recommendation**: Extract to helper function ````go func applyAuthIfConfigured(apiKey string, handler http.HandlerFunc) http.HandlerFunc { if apiKey != "" { return authMiddleware(apiKey, handler) } return handler } // Usage finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler) finalCloseHandler := applyAuthIfConfigured(apiKey, closeHandler) ```` **Estimated Impact**: Reduced duplication, easier maintenance **Effort**: 30 minutes --- #### 2. **Misplaced Generic Logging Function** (server package) **Issue**: Generic runtime error logging function is in auth-specific file. **Function**: `logRuntimeError()` in `internal/server/auth.go` **Problem**: This function logs generic runtime errors and panics, not authentication-specific errors. **Current Location**: `internal/server/auth.go` (lines ~50-60) **Recommendation**: Move to `internal/server/server.go` or create `internal/server/errors.go` for error handling utilities. **Estimated Impact**: Improved code organization, clearer file purpose **Effort**: 15 minutes --- #### 3. **Large Schema Normalization Function in Types File** (mcp package) **Issue**: 86-line schema normalization logic mixed with type definitions. **Function**: `NormalizeInputSchema()` in `internal/mcp/types.go` **Problem**: `types.go` should contain simple type definitions, not complex transformation logic. **Current**: 86 lines of schema transformation logic in types.go **Better**: Extract to `internal/mcp/schema.go` or `internal/mcp/normalize.go` **Recommendation**: 1. Create `internal/mcp/schema.go` 2. Move `NormalizeInputSchema()` to new file 3. Keep only type definitions in `types.go` **Estimated Impact**: Clearer separation of concerns, easier to test **Effort**: 20 minutes --- #### 4. **Unnecessary Validation Wrapper Function** (config package) **Issue**: Thin wrapper function that adds no value. **Function**: `validateServerConfig()` in `internal/config/validation.go:104-106` **Current Code**: ````go func validateServerConfig(name string, srv ServerConfig, cfg *Config) error { return validateServerConfigWithCustomSchemas(name, srv, cfg, nil) } ```` **Problem**: This wrapper just calls the main function with `nil` as the last parameter. **Recommendation**: Remove wrapper, call `validateServerConfigWithCustomSchem... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #1064 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/github/gh-aw-mcpg/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.
2 parents 18117eb + 7436e68 commit cb39742

7 files changed

Lines changed: 132 additions & 127 deletions

File tree

internal/config/validation.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,6 @@ func validateMounts(mounts []string, jsonPath string) error {
100100
return nil
101101
}
102102

103-
// validateServerConfig validates a server configuration (stdio or HTTP)
104-
func validateServerConfig(name string, server *StdinServerConfig) error {
105-
return validateServerConfigWithCustomSchemas(name, server, nil)
106-
}
107-
108103
// validateServerConfigWithCustomSchemas validates a server configuration with custom schema support
109104
func validateServerConfigWithCustomSchemas(name string, server *StdinServerConfig, customSchemas map[string]interface{}) error {
110105
logValidation.Printf("Validating server config: name=%s, type=%s", name, server.Type)

internal/config/validation_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ func TestValidateStdioServer(t *testing.T) {
352352

353353
for _, tt := range tests {
354354
t.Run(tt.name, func(t *testing.T) {
355-
err := validateServerConfig("test-server", tt.server)
355+
err := validateServerConfigWithCustomSchemas("test-server", tt.server, nil)
356356

357357
if tt.shouldErr {
358358
require.Error(t, err)

internal/mcp/schema.go

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package mcp
2+
3+
import (
4+
"github.com/github/gh-aw-mcpg/internal/logger"
5+
)
6+
7+
var logSchema = logger.New("mcp:schema")
8+
9+
// NormalizeInputSchema ensures tool input schemas are valid for the MCP SDK
10+
// The MCP SDK requires that object type schemas have a "properties" field,
11+
// even if it's empty. This function normalizes schemas to meet that requirement.
12+
//
13+
// Returns a normalized copy of the schema, never modifies the original.
14+
func NormalizeInputSchema(schema map[string]interface{}, toolName string) map[string]interface{} {
15+
logSchema.Printf("Normalizing input schema for tool: %s", toolName)
16+
17+
// If backend didn't provide a schema, use a default empty object schema
18+
// This allows the tool to be registered and clients will see it accepts any parameters
19+
if schema == nil {
20+
logSchema.Printf("Tool %s has nil schema, applying default empty object schema", toolName)
21+
logger.LogWarn("backend", "Tool schema normalized: %s - backend provided no inputSchema, using default empty object schema", toolName)
22+
return map[string]interface{}{
23+
"type": "object",
24+
"properties": map[string]interface{}{},
25+
}
26+
}
27+
28+
// Check if this is an object type schema
29+
typeVal, hasType := schema["type"]
30+
31+
logSchema.Printf("Tool %s schema analysis: hasType=%v", toolName, hasType)
32+
33+
// If schema has no type but has properties, it's implicitly an object type
34+
// The MCP SDK requires "type": "object" to be present, so add it
35+
if !hasType {
36+
_, hasProperties := schema["properties"]
37+
logSchema.Printf("Tool %s has no type field, hasProperties=%v", toolName, hasProperties)
38+
if hasProperties {
39+
logger.LogWarn("backend", "Tool schema normalized: %s - added 'type': 'object' to schema with properties", toolName)
40+
// Create a copy of the schema to avoid modifying the original
41+
normalized := make(map[string]interface{})
42+
for k, v := range schema {
43+
normalized[k] = v
44+
}
45+
normalized["type"] = "object"
46+
logSchema.Printf("Tool %s schema normalized: added object type", toolName)
47+
return normalized
48+
}
49+
// Schema without type and without properties - assume it's an empty object schema
50+
logSchema.Printf("Tool %s has no type and no properties, using empty object schema", toolName)
51+
logger.LogWarn("backend", "Tool schema normalized: %s - schema missing type, assuming empty object schema", toolName)
52+
return map[string]interface{}{
53+
"type": "object",
54+
"properties": map[string]interface{}{},
55+
}
56+
}
57+
58+
typeStr, isString := typeVal.(string)
59+
if !isString || typeStr != "object" {
60+
logSchema.Printf("Tool %s has non-object type or invalid type value, returning schema as-is", toolName)
61+
return schema
62+
}
63+
64+
// Check if properties field exists
65+
_, hasProperties := schema["properties"]
66+
_, hasAdditionalProperties := schema["additionalProperties"]
67+
68+
logSchema.Printf("Tool %s object type schema: hasProperties=%v, hasAdditionalProperties=%v",
69+
toolName, hasProperties, hasAdditionalProperties)
70+
71+
// If it's an object type but missing both properties and additionalProperties,
72+
// add an empty properties object to make it valid
73+
if !hasProperties && !hasAdditionalProperties {
74+
logger.LogWarn("backend", "Tool schema normalized: %s - added empty properties to object type schema", toolName)
75+
76+
// Create a copy of the schema to avoid modifying the original
77+
normalized := make(map[string]interface{})
78+
for k, v := range schema {
79+
normalized[k] = v
80+
}
81+
normalized["properties"] = map[string]interface{}{}
82+
83+
logSchema.Printf("Tool %s schema normalized: added empty properties field", toolName)
84+
return normalized
85+
}
86+
87+
logSchema.Printf("Tool %s schema is valid, no normalization needed", toolName)
88+
return schema
89+
}

internal/mcp/types.go

Lines changed: 0 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,8 @@ package mcp
22

33
import (
44
"encoding/json"
5-
6-
"github.com/github/gh-aw-mcpg/internal/logger"
75
)
86

9-
var logTypes = logger.New("mcp:types")
10-
117
// Request represents a JSON-RPC 2.0 request
128
type Request struct {
139
JSONRPC string `json:"jsonrpc"`
@@ -49,90 +45,3 @@ type ContentItem struct {
4945
Type string `json:"type"`
5046
Text string `json:"text,omitempty"`
5147
}
52-
53-
// NormalizeInputSchema fixes common schema validation issues in tool definitions
54-
// that can cause downstream validation errors.
55-
//
56-
// Known issues fixed:
57-
// 1. Missing schema: When a backend returns no inputSchema (nil), we provide
58-
// a default empty object schema that accepts any properties. This is required
59-
// by the MCP SDK's Server.AddTool method.
60-
// 2. Object schemas without properties: When a schema declares "type": "object"
61-
// but is missing the required "properties" field, we add an empty properties
62-
// object to make it valid per JSON Schema standards.
63-
func NormalizeInputSchema(schema map[string]interface{}, toolName string) map[string]interface{} {
64-
logTypes.Printf("Normalizing input schema for tool: %s", toolName)
65-
66-
// If backend didn't provide a schema, use a default empty object schema
67-
// This allows the tool to be registered and clients will see it accepts any parameters
68-
if schema == nil {
69-
logTypes.Printf("Tool %s has nil schema, applying default empty object schema", toolName)
70-
logger.LogWarn("backend", "Tool schema normalized: %s - backend provided no inputSchema, using default empty object schema", toolName)
71-
return map[string]interface{}{
72-
"type": "object",
73-
"properties": map[string]interface{}{},
74-
}
75-
}
76-
77-
// Check if this is an object type schema
78-
typeVal, hasType := schema["type"]
79-
80-
logTypes.Printf("Tool %s schema analysis: hasType=%v", toolName, hasType)
81-
82-
// If schema has no type but has properties, it's implicitly an object type
83-
// The MCP SDK requires "type": "object" to be present, so add it
84-
if !hasType {
85-
_, hasProperties := schema["properties"]
86-
logTypes.Printf("Tool %s has no type field, hasProperties=%v", toolName, hasProperties)
87-
if hasProperties {
88-
logger.LogWarn("backend", "Tool schema normalized: %s - added 'type': 'object' to schema with properties", toolName)
89-
// Create a copy of the schema to avoid modifying the original
90-
normalized := make(map[string]interface{})
91-
for k, v := range schema {
92-
normalized[k] = v
93-
}
94-
normalized["type"] = "object"
95-
logTypes.Printf("Tool %s schema normalized: added object type", toolName)
96-
return normalized
97-
}
98-
// Schema without type and without properties - assume it's an empty object schema
99-
logTypes.Printf("Tool %s has no type and no properties, using empty object schema", toolName)
100-
logger.LogWarn("backend", "Tool schema normalized: %s - schema missing type, assuming empty object schema", toolName)
101-
return map[string]interface{}{
102-
"type": "object",
103-
"properties": map[string]interface{}{},
104-
}
105-
}
106-
107-
typeStr, isString := typeVal.(string)
108-
if !isString || typeStr != "object" {
109-
logTypes.Printf("Tool %s has non-object type or invalid type value, returning schema as-is", toolName)
110-
return schema
111-
}
112-
113-
// Check if properties field exists
114-
_, hasProperties := schema["properties"]
115-
_, hasAdditionalProperties := schema["additionalProperties"]
116-
117-
logTypes.Printf("Tool %s object type schema: hasProperties=%v, hasAdditionalProperties=%v",
118-
toolName, hasProperties, hasAdditionalProperties)
119-
120-
// If it's an object type but missing both properties and additionalProperties,
121-
// add an empty properties object to make it valid
122-
if !hasProperties && !hasAdditionalProperties {
123-
logger.LogWarn("backend", "Tool schema normalized: %s - added empty properties to object type schema", toolName)
124-
125-
// Create a copy of the schema to avoid modifying the original
126-
normalized := make(map[string]interface{})
127-
for k, v := range schema {
128-
normalized[k] = v
129-
}
130-
normalized["properties"] = map[string]interface{}{}
131-
132-
logTypes.Printf("Tool %s schema normalized: added empty properties field", toolName)
133-
return normalized
134-
}
135-
136-
logTypes.Printf("Tool %s schema is valid, no normalization needed", toolName)
137-
return schema
138-
}

internal/server/auth.go

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
package server
22

33
import (
4-
"log"
54
"net/http"
6-
"time"
75

86
"github.com/github/gh-aw-mcpg/internal/logger"
97
)
@@ -48,23 +46,3 @@ func authMiddleware(apiKey string, next http.HandlerFunc) http.HandlerFunc {
4846
next(w, r)
4947
}
5048
}
51-
52-
// logRuntimeError logs runtime errors to stdout per spec section 9.2
53-
func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) {
54-
logAuth.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail)
55-
56-
timestamp := time.Now().UTC().Format(time.RFC3339)
57-
requestID := r.Header.Get("X-Request-ID")
58-
if requestID == "" {
59-
requestID = "unknown"
60-
}
61-
62-
server := "gateway"
63-
if serverName != nil {
64-
server = *serverName
65-
}
66-
67-
// Spec 9.2: Log to stdout with timestamp, server name, request ID, error details
68-
log.Printf("[ERROR] timestamp=%s server=%s request_id=%s error_type=%s detail=%s path=%s method=%s",
69-
timestamp, server, requestID, errorType, detail, r.URL.Path, r.Method)
70-
}

internal/server/errors.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package server
2+
3+
import (
4+
"log"
5+
"net/http"
6+
"time"
7+
8+
"github.com/github/gh-aw-mcpg/internal/logger"
9+
)
10+
11+
var logErrors = logger.New("server:errors")
12+
13+
// logRuntimeError logs runtime errors to stdout per spec section 9.2
14+
func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) {
15+
logErrors.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail)
16+
17+
timestamp := time.Now().UTC().Format(time.RFC3339)
18+
requestID := r.Header.Get("X-Request-ID")
19+
if requestID == "" {
20+
requestID = "unknown"
21+
}
22+
23+
server := "gateway"
24+
if serverName != nil {
25+
server = *serverName
26+
}
27+
28+
// Spec 9.2: Log to stdout with timestamp, server name, request ID, error details
29+
log.Printf("[ERROR] timestamp=%s server=%s request_id=%s error_type=%s detail=%s path=%s method=%s",
30+
timestamp, server, requestID, errorType, detail, r.URL.Path, r.Method)
31+
}

internal/server/transport.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,15 @@ func withResponseLogging(handler http.Handler) http.Handler {
5656
})
5757
}
5858

59+
// applyAuthIfConfigured applies authentication middleware if an API key is provided
60+
// Returns the handler unchanged if apiKey is empty
61+
func applyAuthIfConfigured(apiKey string, handler http.HandlerFunc) http.HandlerFunc {
62+
if apiKey != "" {
63+
return authMiddleware(apiKey, handler)
64+
}
65+
return handler
66+
}
67+
5968
// CreateHTTPServerForMCP creates an HTTP server that handles MCP over streamable HTTP transport
6069
// If apiKey is provided, all requests except /health require authentication (spec 7.1)
6170
func CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey string) *http.Server {
@@ -116,10 +125,7 @@ func CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey st
116125
shutdownHandler := rejectIfShutdown(unifiedServer, loggedHandler, "server:transport")
117126

118127
// Apply auth middleware if API key is configured (spec 7.1)
119-
finalHandler := shutdownHandler
120-
if apiKey != "" {
121-
finalHandler = authMiddleware(apiKey, shutdownHandler.ServeHTTP)
122-
}
128+
finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)
123129

124130
// Mount handler at /mcp endpoint (logging is done in the callback above)
125131
mux.Handle("/mcp/", finalHandler)
@@ -133,10 +139,7 @@ func CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey st
133139
closeHandler := handleClose(unifiedServer)
134140

135141
// Apply auth middleware if API key is configured (spec 7.1)
136-
finalCloseHandler := closeHandler
137-
if apiKey != "" {
138-
finalCloseHandler = authMiddleware(apiKey, closeHandler.ServeHTTP)
139-
}
142+
finalCloseHandler := applyAuthIfConfigured(apiKey, closeHandler.ServeHTTP)
140143
mux.Handle("/close", withResponseLogging(finalCloseHandler))
141144

142145
return &http.Server{

0 commit comments

Comments
 (0)