From 3135ce37ca310118250a502e1c00c877df1e45dd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:10:34 +0000 Subject: [PATCH 1/4] Initial plan From 168136f55e71f8dd8853cc446ab00a6160c78e15 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:21:15 +0000 Subject: [PATCH 2/4] =?UTF-8?q?refactor:=20semantic=20function=20clusterin?= =?UTF-8?q?g=20=E2=80=94=20move=20GenerateRandomAgentID,=20extract=20jsonF?= =?UTF-8?q?ileSink,=20centralize=20ParseServerIDFromToolName?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/auth/header.go | 17 ------ internal/auth/id.go | 27 ++++++++ internal/logger/fileutil.go | 31 ++++++++++ internal/logger/logger_namespace_test.go | 5 +- .../logger/observed_url_domains_logger.go | 19 +++--- .../observed_url_domains_logger_test.go | 10 +-- internal/logger/tools_logger.go | 15 ++--- internal/logger/tools_logger_test.go | 24 +++----- internal/middleware/jqschema.go | 10 +-- internal/middleware/jqschema_coverage_test.go | 9 +-- internal/util/toolname.go | 24 ++++++++ internal/util/toolname_test.go | 61 +++++++++++++++++++ 12 files changed, 177 insertions(+), 75 deletions(-) create mode 100644 internal/auth/id.go create mode 100644 internal/util/toolname.go create mode 100644 internal/util/toolname_test.go diff --git a/internal/auth/header.go b/internal/auth/header.go index 590664412..66cb55c29 100644 --- a/internal/auth/header.go +++ b/internal/auth/header.go @@ -36,16 +36,13 @@ package auth import ( "errors" - "fmt" "strings" "github.com/github/gh-aw-mcpg/internal/logger" "github.com/github/gh-aw-mcpg/internal/sanitize" - "github.com/github/gh-aw-mcpg/internal/util" ) var logAuth = logger.ForFile() -var logAPIKey = logger.New("auth:apikey") var ( // ErrMissingAuthHeader is returned when the Authorization header is missing @@ -200,17 +197,3 @@ func IsMalformedHeader(header string) bool { } return false } - -// GenerateRandomAgentID generates a cryptographically random agent ID. -// Per spec §7.3, the gateway SHOULD generate a random agent ID on startup -// if none is provided. Returns a 32-byte hex-encoded string (64 chars). -func GenerateRandomAgentID() (string, error) { - logAPIKey.Print("Generating random agent ID") - key, err := util.RandomHex(32) - if err != nil { - logAPIKey.Printf("Random agent ID generation failed: %v", err) - return "", fmt.Errorf("failed to generate random agent ID: %w", err) - } - logAPIKey.Print("Random agent ID generated successfully") - return key, nil -} diff --git a/internal/auth/id.go b/internal/auth/id.go new file mode 100644 index 000000000..66f275c9c --- /dev/null +++ b/internal/auth/id.go @@ -0,0 +1,27 @@ +package auth + +import ( + "fmt" + + "github.com/github/gh-aw-mcpg/internal/logger" + "github.com/github/gh-aw-mcpg/internal/util" +) + +// logAPIKey is the debug logger for API-key / agent-ID generation. +// It uses the custom namespace "auth:apikey" so callers can filter these +// debug logs independently with DEBUG=auth:apikey. +var logAPIKey = logger.New("auth:apikey") + +// GenerateRandomAgentID generates a cryptographically random agent ID. +// Per spec §7.3, the gateway SHOULD generate a random agent ID on startup +// if none is provided. Returns a 32-byte hex-encoded string (64 chars). +func GenerateRandomAgentID() (string, error) { + logAPIKey.Print("Generating random agent ID") + key, err := util.RandomHex(32) + if err != nil { + logAPIKey.Printf("Random agent ID generation failed: %v", err) + return "", fmt.Errorf("failed to generate random agent ID: %w", err) + } + logAPIKey.Print("Random agent ID generated successfully") + return key, nil +} diff --git a/internal/logger/fileutil.go b/internal/logger/fileutil.go index 14a4556da..436a60af2 100644 --- a/internal/logger/fileutil.go +++ b/internal/logger/fileutil.go @@ -123,3 +123,34 @@ func writeJSONToFile(logDir, fileName string, data any, perm os.FileMode) error } return atomicWriteFile(filepath.Join(logDir, fileName), jsonData, perm) } + +// jsonFileSink holds the shared state common to stateful JSON-file loggers +// (logDir, fileName, useFallback). Embed this struct in logger types that +// persist an in-memory data structure to a JSON file so the three repeated +// fields and the writeJSON helper do not have to be duplicated. +// +// Usage: +// +// type MyLogger struct { +// lockable +// jsonFileSink +// data MyData +// } +// +// The embedded jsonFileSink.writeJSON method can then be called from +// writeToFile to write data to the configured JSON file: +// +// func (l *MyLogger) writeToFile() error { +// return l.writeJSON(l.data, 0644) +// } +type jsonFileSink struct { + logDir string + fileName string + useFallback bool +} + +// writeJSON marshals data as indented JSON and atomically writes it to the +// file at s.logDir/s.fileName with the given permissions. +func (s *jsonFileSink) writeJSON(data any, perm os.FileMode) error { + return writeJSONToFile(s.logDir, s.fileName, data, perm) +} diff --git a/internal/logger/logger_namespace_test.go b/internal/logger/logger_namespace_test.go index 120d80d06..783321c73 100644 --- a/internal/logger/logger_namespace_test.go +++ b/internal/logger/logger_namespace_test.go @@ -25,10 +25,9 @@ func TestLoggerNamespacesMatchFileConventions(t *testing.T) { internalRoot := filepath.Join(repoRoot, "internal") exceptionNamespaces := map[string][]string{ - // header.go defines two loggers: one for general auth (auto-derived via ForFile as - // "auth:header") and one for API-key auth which uses the custom namespace "auth:apikey" + // id.go defines the logAPIKey logger with the custom namespace "auth:apikey" // so callers can filter API-key debug logs independently with DEBUG=auth:apikey. - "internal/auth/header.go": {"auth:apikey"}, + "internal/auth/id.go": {"auth:apikey"}, // The following files use intentionally shorter or semantically clearer namespaces // instead of the full file-name-derived form. These are preserved for backward diff --git a/internal/logger/observed_url_domains_logger.go b/internal/logger/observed_url_domains_logger.go index 4070f6470..9f440fd51 100644 --- a/internal/logger/observed_url_domains_logger.go +++ b/internal/logger/observed_url_domains_logger.go @@ -27,10 +27,8 @@ func URLDomainAuditEnabled() bool { // ObservedURLDomainsLogger manages unique observed URL domains grouped by server ID. type ObservedURLDomainsLogger struct { lockable - logDir string - fileName string - data map[string]map[string]struct{} - useFallback bool + jsonFileSink + data map[string]map[string]struct{} } var ( @@ -46,9 +44,8 @@ var observedURLDomainsLoggerFactory = newLoggerFactory( } l := &ObservedURLDomainsLogger{ - logDir: logDir, - fileName: fileName, - data: make(map[string]map[string]struct{}), + jsonFileSink: jsonFileSink{logDir: logDir, fileName: fileName}, + data: make(map[string]map[string]struct{}), } if err := l.writeToFile(); err != nil { return nil, err @@ -58,10 +55,8 @@ var observedURLDomainsLoggerFactory = newLoggerFactory( }, func(err error, logDir, fileName string) (*ObservedURLDomainsLogger, error) { return fallbackLoggerOnInitError(err, "Failed to initialize observed URL domains log file", "Observed URL domains logging disabled", &ObservedURLDomainsLogger{ - logDir: logDir, - fileName: fileName, - data: make(map[string]map[string]struct{}), - useFallback: true, + jsonFileSink: jsonFileSink{logDir: logDir, fileName: fileName, useFallback: true}, + data: make(map[string]map[string]struct{}), }) }, ) @@ -112,7 +107,7 @@ func (l *ObservedURLDomainsLogger) writeToFile() error { for serverID, domains := range l.data { serialized[serverID] = util.SortedSetKeys(domains) } - return writeJSONToFile(l.logDir, l.fileName, serialized, 0600) + return l.writeJSON(serialized, 0600) } func (l *ObservedURLDomainsLogger) Close() error { return nil } diff --git a/internal/logger/observed_url_domains_logger_test.go b/internal/logger/observed_url_domains_logger_test.go index 1752bd87b..b2fc20078 100644 --- a/internal/logger/observed_url_domains_logger_test.go +++ b/internal/logger/observed_url_domains_logger_test.go @@ -90,7 +90,7 @@ func TestInitObservedURLDomainsLogger_FallbackOnBadDir(t *testing.T) { // The global logger should be a fallback instance (not nil). globalObservedURLDomainsMu.RLock() assert.NotNil(t, globalObservedURLDomainsLogger, "fallback logger should still be set") - assert.True(t, globalObservedURLDomainsLogger.useFallback, "logger should be in fallback mode") + assert.True(t, globalObservedURLDomainsLogger.jsonFileSink.useFallback, "logger should be in fallback mode") globalObservedURLDomainsMu.RUnlock() } @@ -155,8 +155,8 @@ func TestLogDomains_NilDomains(t *testing.T) { func TestLogDomains_FallbackMode_ReturnsNil(t *testing.T) { l := &ObservedURLDomainsLogger{ - data: make(map[string]map[string]struct{}), - useFallback: true, + data: make(map[string]map[string]struct{}), + jsonFileSink: jsonFileSink{useFallback: true}, } // In fallback mode LogDomains should silently succeed without writing. @@ -320,8 +320,8 @@ func TestLogObservedURLDomains_FallbackMode_NoPanic(t *testing.T) { globalObservedURLDomainsMu.Lock() prev := globalObservedURLDomainsLogger globalObservedURLDomainsLogger = &ObservedURLDomainsLogger{ - data: make(map[string]map[string]struct{}), - useFallback: true, + data: make(map[string]map[string]struct{}), + jsonFileSink: jsonFileSink{useFallback: true}, } globalObservedURLDomainsMu.Unlock() t.Cleanup(func() { diff --git a/internal/logger/tools_logger.go b/internal/logger/tools_logger.go index a07f7f8f4..6f9996719 100644 --- a/internal/logger/tools_logger.go +++ b/internal/logger/tools_logger.go @@ -26,10 +26,8 @@ type ToolsData struct { // ToolsLogger manages logging of MCP server tools to a JSON file type ToolsLogger struct { lockable - logDir string - fileName string - data *ToolsData - useFallback bool + jsonFileSink + data *ToolsData } var ( @@ -47,8 +45,7 @@ var toolsLoggerFactory = newLoggerFactory( } tl := &ToolsLogger{ - logDir: logDir, - fileName: fileName, + jsonFileSink: jsonFileSink{logDir: logDir, fileName: fileName}, data: &ToolsData{ Servers: make(map[string][]ToolInfo), }, @@ -58,9 +55,7 @@ var toolsLoggerFactory = newLoggerFactory( }, func(err error, logDir, fileName string) (*ToolsLogger, error) { return fallbackLoggerOnInitError(err, "Failed to initialize tools log file", "Tools logging disabled", &ToolsLogger{ - logDir: logDir, - fileName: fileName, - useFallback: true, + jsonFileSink: jsonFileSink{logDir: logDir, fileName: fileName, useFallback: true}, data: &ToolsData{ Servers: make(map[string][]ToolInfo), }, @@ -92,7 +87,7 @@ func (tl *ToolsLogger) LogTools(serverID string, tools []ToolInfo) error { // writeToFile writes the current tools data to the JSON file. // Caller must hold tl.mu lock. func (tl *ToolsLogger) writeToFile() error { - return writeJSONToFile(tl.logDir, tl.fileName, tl.data, 0644) + return tl.writeJSON(tl.data, 0644) } // Close is a no-op for ToolsLogger (implements closableLogger interface) diff --git a/internal/logger/tools_logger_test.go b/internal/logger/tools_logger_test.go index a3dae7b58..0a4172934 100644 --- a/internal/logger/tools_logger_test.go +++ b/internal/logger/tools_logger_test.go @@ -221,8 +221,7 @@ func TestWriteToFile_Success(t *testing.T) { tmpDir := t.TempDir() tl := &ToolsLogger{ - logDir: tmpDir, - fileName: "tools.json", + jsonFileSink: jsonFileSink{logDir: tmpDir, fileName: "tools.json"}, data: &ToolsData{ Servers: map[string][]ToolInfo{ "server1": { @@ -252,9 +251,8 @@ func TestWriteToFile_WriteFileFails(t *testing.T) { assert := assert.New(t) tl := &ToolsLogger{ - logDir: "/nonexistent/dir/that/does/not/exist", - fileName: "tools.json", - data: &ToolsData{Servers: make(map[string][]ToolInfo)}, + jsonFileSink: jsonFileSink{logDir: "/nonexistent/dir/that/does/not/exist", fileName: "tools.json"}, + data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } err := tl.writeToFile() @@ -275,9 +273,8 @@ func TestWriteToFile_RenameFails(t *testing.T) { require.NoError(os.MkdirAll(targetPath, 0755)) tl := &ToolsLogger{ - logDir: tmpDir, - fileName: "tools.json", - data: &ToolsData{Servers: make(map[string][]ToolInfo)}, + jsonFileSink: jsonFileSink{logDir: tmpDir, fileName: "tools.json"}, + data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } err := tl.writeToFile() @@ -299,9 +296,8 @@ func TestLogToolsForServer_ErrorIsLogged(t *testing.T) { oldLogger := globalToolsLogger // Point the global logger at a nonexistent directory so writeToFile fails. globalToolsLogger = &ToolsLogger{ - logDir: "/nonexistent/path/for/test", - fileName: "tools.json", - data: &ToolsData{Servers: make(map[string][]ToolInfo)}, + jsonFileSink: jsonFileSink{logDir: "/nonexistent/path/for/test", fileName: "tools.json"}, + data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } globalToolsMu.Unlock() t.Cleanup(func() { @@ -325,10 +321,8 @@ func TestLogToolsForServer_FallbackSkipsErrors(t *testing.T) { globalToolsMu.Lock() oldLogger := globalToolsLogger globalToolsLogger = &ToolsLogger{ - logDir: "/nonexistent/path", - fileName: "tools.json", - useFallback: true, - data: &ToolsData{Servers: make(map[string][]ToolInfo)}, + jsonFileSink: jsonFileSink{logDir: "/nonexistent/path", fileName: "tools.json", useFallback: true}, + data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } globalToolsMu.Unlock() t.Cleanup(func() { diff --git a/internal/middleware/jqschema.go b/internal/middleware/jqschema.go index 0c4d2460b..4c0e03b36 100644 --- a/internal/middleware/jqschema.go +++ b/internal/middleware/jqschema.go @@ -718,7 +718,7 @@ func auditObservedURLDomains(toolName string, data any) { if !logger.URLDomainAuditEnabled() || data == nil { return } - serverID := parseServerIDFromToolName(toolName) + serverID := util.ParseServerIDFromToolName(toolName) domains := urlutil.ExtractURLDomainsFromValue(data) if len(domains) == 0 { return @@ -726,14 +726,6 @@ func auditObservedURLDomains(toolName string, data any) { logger.LogObservedURLDomains(serverID, domains) } -func parseServerIDFromToolName(toolName string) string { - serverID, _, ok := strings.Cut(toolName, "___") - if !ok || serverID == "" { - return toolName - } - return serverID -} - // savePayload saves the payload to disk and returns the file path // The file is saved to {baseDir}/{sessionID}/{queryID}/payload.json // The returned path uses pathPrefix if provided, otherwise returns the actual filesystem path diff --git a/internal/middleware/jqschema_coverage_test.go b/internal/middleware/jqschema_coverage_test.go index 3623e64ba..c2136d7e6 100644 --- a/internal/middleware/jqschema_coverage_test.go +++ b/internal/middleware/jqschema_coverage_test.go @@ -19,6 +19,7 @@ import ( "github.com/github/gh-aw-mcpg/internal/jqutil" "github.com/github/gh-aw-mcpg/internal/logger" + "github.com/github/gh-aw-mcpg/internal/util" sdk "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -595,11 +596,11 @@ func TestCompileOptsWithVariables_DoesNotMutateSharedSecureOpts(t *testing.T) { } // --------------------------------------------------------------------------- -// parseServerIDFromToolName +// util.ParseServerIDFromToolName // --------------------------------------------------------------------------- -// TestParseServerIDFromToolName exercises all three branches of the unexported -// parseServerIDFromToolName helper: +// TestParseServerIDFromToolName exercises all three branches of +// util.ParseServerIDFromToolName (formerly the unexported helper in jqschema.go): // // - No "___" separator present → !ok, returns the full toolName // - "___" separator with empty serverID → serverID=="", returns the full toolName @@ -652,7 +653,7 @@ func TestParseServerIDFromToolName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := parseServerIDFromToolName(tt.toolName) + got := util.ParseServerIDFromToolName(tt.toolName) assert.Equal(t, tt.want, got) }) } diff --git a/internal/util/toolname.go b/internal/util/toolname.go new file mode 100644 index 000000000..7be1d7879 --- /dev/null +++ b/internal/util/toolname.go @@ -0,0 +1,24 @@ +package util + +import "strings" + +// toolNameSeparator is the delimiter used to join a backend server ID with a +// tool name when tools are prefixed with their originating server. For example, +// a tool named "search_code" from server "github" is exposed as +// "github___search_code". +const toolNameSeparator = "___" + +// ParseServerIDFromToolName extracts the server ID prefix from a prefixed tool +// name of the form "___". If the tool name contains no +// separator, or the server ID portion is empty, the full toolName is returned. +// +// This is the canonical parser for the prefixed tool-name format defined in +// the server package. Both middleware and other consumers should use this +// function instead of duplicating the string-splitting logic. +func ParseServerIDFromToolName(toolName string) string { + serverID, _, ok := strings.Cut(toolName, toolNameSeparator) + if !ok || serverID == "" { + return toolName + } + return serverID +} diff --git a/internal/util/toolname_test.go b/internal/util/toolname_test.go new file mode 100644 index 000000000..9e6079069 --- /dev/null +++ b/internal/util/toolname_test.go @@ -0,0 +1,61 @@ +package util + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseServerIDFromToolName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + toolName string + want string + }{ + { + name: "no separator returns full tool name", + toolName: "list_repos", + want: "list_repos", + }, + { + name: "normal prefixed tool name returns server ID", + toolName: "github___list_repos", + want: "github", + }, + { + // strings.Cut("___list_repos", "___") → ("", "list_repos", true) + // serverID=="" so the function falls into the !ok||serverID=="" branch + // and returns the original toolName unchanged. + name: "tool name starting with separator returns full name", + toolName: "___list_repos", + want: "___list_repos", + }, + { + name: "empty tool name returns empty string", + toolName: "", + want: "", + }, + { + // strings.Cut("___", "___") → ("", "", true); serverID=="" → returns "___" + name: "separator only returns full name", + toolName: "___", + want: "___", + }, + { + // strings.Cut splits on the FIRST occurrence only. + name: "multiple separators returns portion before first", + toolName: "github___owner___list_repos", + want: "github", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ParseServerIDFromToolName(tt.toolName) + assert.Equal(t, tt.want, got) + }) + } +} From bf8e4fbc05ed0ed4c37b4a7a56caecb650f03b49 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 19 Jul 2026 16:35:05 -0700 Subject: [PATCH 3/4] Fix golangci-lint QF1008 in observed_url_domains_logger_test Remove the redundant embedded field `jsonFileSink` from the selector; the `useFallback` field is promoted through the embedded struct, so `globalObservedURLDomainsLogger.useFallback` is equivalent and satisfies staticcheck QF1008. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/logger/observed_url_domains_logger_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logger/observed_url_domains_logger_test.go b/internal/logger/observed_url_domains_logger_test.go index b2fc20078..a4d36b7f0 100644 --- a/internal/logger/observed_url_domains_logger_test.go +++ b/internal/logger/observed_url_domains_logger_test.go @@ -90,7 +90,7 @@ func TestInitObservedURLDomainsLogger_FallbackOnBadDir(t *testing.T) { // The global logger should be a fallback instance (not nil). globalObservedURLDomainsMu.RLock() assert.NotNil(t, globalObservedURLDomainsLogger, "fallback logger should still be set") - assert.True(t, globalObservedURLDomainsLogger.jsonFileSink.useFallback, "logger should be in fallback mode") + assert.True(t, globalObservedURLDomainsLogger.useFallback, "logger should be in fallback mode") globalObservedURLDomainsMu.RUnlock() } From 3ffc166b120185d4c7fa312962480c601afec267 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 19 Jul 2026 16:42:47 -0700 Subject: [PATCH 4/4] Address review feedback: relocate ID tests, drop duplicate parser test - Move GenerateRandomAgentID failure/recovery tests and the errorReader helper out of internal/auth/header_test.go into a new id_test.go, so the header-focused test file no longer owns ID-generation test code. Drop the now-unused crypto/rand and errors imports from header_test.go. - Remove the duplicate TestParseServerIDFromToolName block (and the unused util import) from internal/middleware/jqschema_coverage_test.go; the canonical table in internal/util/toolname_test.go already covers these cases, and TestAuditObservedURLDomains covers the middleware call site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/auth/header_test.go | 49 -------------- internal/auth/id_test.go | 57 ++++++++++++++++ internal/middleware/jqschema_coverage_test.go | 65 ------------------- 3 files changed, 57 insertions(+), 114 deletions(-) create mode 100644 internal/auth/id_test.go diff --git a/internal/auth/header_test.go b/internal/auth/header_test.go index 3fb50135a..c56e3ec11 100644 --- a/internal/auth/header_test.go +++ b/internal/auth/header_test.go @@ -1,8 +1,6 @@ package auth import ( - "crypto/rand" - "errors" "testing" "github.com/stretchr/testify/assert" @@ -596,50 +594,3 @@ func TestStripAuthScheme(t *testing.T) { }) } } - -// errorReader is a test helper io.Reader that always returns the configured error. -type errorReader struct { - err error -} - -func (r *errorReader) Read(_ []byte) (int, error) { - return 0, r.err -} - -// TestGenerateRandomAgentID_RandomFailure verifies that GenerateRandomAgentID -// correctly wraps and propagates errors from the underlying random source. -// This test must NOT run in parallel because it temporarily replaces the -// global crypto/rand.Reader. -func TestGenerateRandomAgentID_RandomFailure(t *testing.T) { - syntheticErr := errors.New("synthetic entropy failure") - - origReader := rand.Reader - rand.Reader = &errorReader{err: syntheticErr} - defer func() { rand.Reader = origReader }() - - key, err := GenerateRandomAgentID() - - assert.Empty(t, key, "key should be empty when random generation fails") - require.Error(t, err, "should return an error when the random source fails") - assert.ErrorIs(t, err, syntheticErr, "error should wrap the underlying source error") - assert.Contains(t, err.Error(), "failed to generate random agent ID", - "error message should describe the failure context") -} - -// TestGenerateRandomAgentID_RecoveryAfterFailure verifies that -// GenerateRandomAgentID works correctly after the random source is restored, -// confirming that no state is leaked between calls. -// This test must NOT run in parallel because it temporarily replaces the -// global crypto/rand.Reader. -func TestGenerateRandomAgentID_RecoveryAfterFailure(t *testing.T) { - origReader := rand.Reader - rand.Reader = &errorReader{err: errors.New("transient failure")} - _, err := GenerateRandomAgentID() - require.Error(t, err, "should fail with broken reader") - - // Restore and verify subsequent call succeeds. - rand.Reader = origReader - key, err := GenerateRandomAgentID() - require.NoError(t, err, "should succeed after reader is restored") - assert.Len(t, key, 64, "restored call should return 64-char hex key") -} diff --git a/internal/auth/id_test.go b/internal/auth/id_test.go new file mode 100644 index 000000000..0d2231c81 --- /dev/null +++ b/internal/auth/id_test.go @@ -0,0 +1,57 @@ +package auth + +import ( + "crypto/rand" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// errorReader is a test helper io.Reader that always returns the configured error. +type errorReader struct { + err error +} + +func (r *errorReader) Read(_ []byte) (int, error) { + return 0, r.err +} + +// TestGenerateRandomAgentID_RandomFailure verifies that GenerateRandomAgentID +// correctly wraps and propagates errors from the underlying random source. +// This test must NOT run in parallel because it temporarily replaces the +// global crypto/rand.Reader. +func TestGenerateRandomAgentID_RandomFailure(t *testing.T) { + syntheticErr := errors.New("synthetic entropy failure") + + origReader := rand.Reader + rand.Reader = &errorReader{err: syntheticErr} + defer func() { rand.Reader = origReader }() + + key, err := GenerateRandomAgentID() + + assert.Empty(t, key, "key should be empty when random generation fails") + require.Error(t, err, "should return an error when the random source fails") + assert.ErrorIs(t, err, syntheticErr, "error should wrap the underlying source error") + assert.Contains(t, err.Error(), "failed to generate random agent ID", + "error message should describe the failure context") +} + +// TestGenerateRandomAgentID_RecoveryAfterFailure verifies that +// GenerateRandomAgentID works correctly after the random source is restored, +// confirming that no state is leaked between calls. +// This test must NOT run in parallel because it temporarily replaces the +// global crypto/rand.Reader. +func TestGenerateRandomAgentID_RecoveryAfterFailure(t *testing.T) { + origReader := rand.Reader + rand.Reader = &errorReader{err: errors.New("transient failure")} + _, err := GenerateRandomAgentID() + require.Error(t, err, "should fail with broken reader") + + // Restore and verify subsequent call succeeds. + rand.Reader = origReader + key, err := GenerateRandomAgentID() + require.NoError(t, err, "should succeed after reader is restored") + assert.Len(t, key, 64, "restored call should return 64-char hex key") +} diff --git a/internal/middleware/jqschema_coverage_test.go b/internal/middleware/jqschema_coverage_test.go index c2136d7e6..cee8a4bd3 100644 --- a/internal/middleware/jqschema_coverage_test.go +++ b/internal/middleware/jqschema_coverage_test.go @@ -19,7 +19,6 @@ import ( "github.com/github/gh-aw-mcpg/internal/jqutil" "github.com/github/gh-aw-mcpg/internal/logger" - "github.com/github/gh-aw-mcpg/internal/util" sdk "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -595,70 +594,6 @@ func TestCompileOptsWithVariables_DoesNotMutateSharedSecureOpts(t *testing.T) { assert.Len(t, opts, initialLen+1) } -// --------------------------------------------------------------------------- -// util.ParseServerIDFromToolName -// --------------------------------------------------------------------------- - -// TestParseServerIDFromToolName exercises all three branches of -// util.ParseServerIDFromToolName (formerly the unexported helper in jqschema.go): -// -// - No "___" separator present → !ok, returns the full toolName -// - "___" separator with empty serverID → serverID=="", returns the full toolName -// - "___" separator with non-empty serverID → returns the serverID prefix -func TestParseServerIDFromToolName(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - toolName string - want string - }{ - { - name: "no separator returns full tool name", - toolName: "list_repos", - want: "list_repos", - }, - { - name: "normal prefixed tool name returns server ID", - toolName: "github___list_repos", - want: "github", - }, - { - // strings.Cut("___list_repos", "___") → ("", "list_repos", true) - // serverID=="" so the function falls into the !ok||serverID=="" branch - // and returns the original toolName unchanged. - name: "tool name starting with separator returns full name", - toolName: "___list_repos", - want: "___list_repos", - }, - { - name: "empty tool name returns empty string", - toolName: "", - want: "", - }, - { - // strings.Cut("___", "___") → ("", "", true); serverID=="" → returns "___" - name: "separator only returns full name", - toolName: "___", - want: "___", - }, - { - // strings.Cut splits on the FIRST occurrence only. - name: "multiple separators returns portion before first", - toolName: "github___owner___list_repos", - want: "github", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got := util.ParseServerIDFromToolName(tt.toolName) - assert.Equal(t, tt.want, got) - }) - } -} - // --------------------------------------------------------------------------- // auditObservedURLDomains // ---------------------------------------------------------------------------