From 203c9b944085614d68ff670f8c9e8da85e4a73ed Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:13:23 +0000 Subject: [PATCH 1/3] Initial plan From 339923fd3041111fd542355d0a2e6681630d1b67 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:15:43 +0000 Subject: [PATCH 2/3] refactor(logger): inline thin file-write wrappers --- internal/envutil/expand_env_args.go | 2 ++ internal/logger/observed_url_domains_logger.go | 16 ++++++---------- internal/logger/tools_logger.go | 8 +------- internal/logger/tools_logger_test.go | 14 +++++++------- 4 files changed, 16 insertions(+), 24 deletions(-) diff --git a/internal/envutil/expand_env_args.go b/internal/envutil/expand_env_args.go index 0502922e9..bf0c18ff4 100644 --- a/internal/envutil/expand_env_args.go +++ b/internal/envutil/expand_env_args.go @@ -30,6 +30,8 @@ func WalkDockerEnvArgs(args []string, fn func(index int, varName, value string, // ExpandEnvArgs expands Docker -e flags that reference environment variables. // Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment. // If the variable is not set, the flag is passed through unchanged. +// This behavior is intentionally different from config expansion in internal/config/expand.go, +// which fails hard when a referenced variable is undefined. func ExpandEnvArgs(args []string) []string { logExpand.Printf("Expanding env args: input_count=%d", len(args)) expandedValues := make(map[int]string) diff --git a/internal/logger/observed_url_domains_logger.go b/internal/logger/observed_url_domains_logger.go index 9f440fd51..d5e6eae55 100644 --- a/internal/logger/observed_url_domains_logger.go +++ b/internal/logger/observed_url_domains_logger.go @@ -47,7 +47,7 @@ var observedURLDomainsLoggerFactory = newLoggerFactory( jsonFileSink: jsonFileSink{logDir: logDir, fileName: fileName}, data: make(map[string]map[string]struct{}), } - if err := l.writeToFile(); err != nil { + if err := l.writeJSON(make(map[string][]string), 0600); err != nil { return nil, err } log.Printf("Observed URL domains logging to file: %s", filepath.Join(logDir, fileName)) @@ -98,18 +98,14 @@ func (l *ObservedURLDomainsLogger) LogDomains(serverID string, domains []string) if !changed { return nil } - return l.writeToFile() + serialized := make(map[string][]string, len(l.data)) + for serverID, serverDomains := range l.data { + serialized[serverID] = util.SortedSetKeys(serverDomains) + } + return l.writeJSON(serialized, 0600) }) } -func (l *ObservedURLDomainsLogger) writeToFile() error { - serialized := make(map[string][]string, len(l.data)) - for serverID, domains := range l.data { - serialized[serverID] = util.SortedSetKeys(domains) - } - return l.writeJSON(serialized, 0600) -} - func (l *ObservedURLDomainsLogger) Close() error { return nil } // LogObservedURLDomains appends newly observed domains for a server. diff --git a/internal/logger/tools_logger.go b/internal/logger/tools_logger.go index 6f9996719..3fbe7bd3b 100644 --- a/internal/logger/tools_logger.go +++ b/internal/logger/tools_logger.go @@ -80,16 +80,10 @@ func (tl *ToolsLogger) LogTools(serverID string, tools []ToolInfo) error { tl.data.Servers[serverID] = tools // Write the updated data to file - return tl.writeToFile() + return tl.writeJSON(tl.data, 0644) }) } -// writeToFile writes the current tools data to the JSON file. -// Caller must hold tl.mu lock. -func (tl *ToolsLogger) writeToFile() error { - return tl.writeJSON(tl.data, 0644) -} - // Close is a no-op for ToolsLogger (implements closableLogger interface) func (tl *ToolsLogger) Close() error { // No file handle to close since we write directly each time diff --git a/internal/logger/tools_logger_test.go b/internal/logger/tools_logger_test.go index 0a4172934..bd3af5367 100644 --- a/internal/logger/tools_logger_test.go +++ b/internal/logger/tools_logger_test.go @@ -214,7 +214,7 @@ func TestToolsLoggerFallback(t *testing.T) { assert.NoError(err, "CloseAllLoggers failed") } -// TestWriteToFile_Success verifies writeToFile writes valid JSON atomically. +// TestWriteToFile_Success verifies writeJSON writes valid JSON atomically. func TestWriteToFile_Success(t *testing.T) { assert := assert.New(t) require := require.New(t) @@ -231,8 +231,8 @@ func TestWriteToFile_Success(t *testing.T) { }, } - err := tl.writeToFile() - require.NoError(err, "writeToFile should succeed") + err := tl.writeJSON(tl.data, 0644) + require.NoError(err, "writeJSON should succeed") // Verify file was written data, err := os.ReadFile(filepath.Join(tmpDir, "tools.json")) @@ -255,8 +255,8 @@ func TestWriteToFile_WriteFileFails(t *testing.T) { data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } - err := tl.writeToFile() - assert.Error(err, "writeToFile should fail when logDir does not exist") + err := tl.writeJSON(tl.data, 0644) + assert.Error(err, "writeJSON should fail when logDir does not exist") assert.ErrorContains(err, "failed to write temp file") } @@ -277,8 +277,8 @@ func TestWriteToFile_RenameFails(t *testing.T) { data: &ToolsData{Servers: make(map[string][]ToolInfo)}, } - err := tl.writeToFile() - assert.Error(err, "writeToFile should fail when rename target is a directory") + err := tl.writeJSON(tl.data, 0644) + assert.Error(err, "writeJSON should fail when rename target is a directory") assert.ErrorContains(err, "failed to rename temp file") // Verify that the cleanup removed the temp file. From 30d3e489ca050b4b08af06e93787eef73d709bd0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:36:17 +0000 Subject: [PATCH 3/3] Align logger docs and test names with writeJSON refactor --- internal/logger/fileutil.go | 6 +++--- internal/logger/tools_logger_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/logger/fileutil.go b/internal/logger/fileutil.go index 436a60af2..72622f5af 100644 --- a/internal/logger/fileutil.go +++ b/internal/logger/fileutil.go @@ -137,10 +137,10 @@ func writeJSONToFile(logDir, fileName string, data any, perm os.FileMode) error // data MyData // } // -// The embedded jsonFileSink.writeJSON method can then be called from -// writeToFile to write data to the configured JSON file: +// The embedded jsonFileSink.writeJSON method can then be called directly +// at the persistence site to write data to the configured JSON file: // -// func (l *MyLogger) writeToFile() error { +// func (l *MyLogger) LogData() error { // return l.writeJSON(l.data, 0644) // } type jsonFileSink struct { diff --git a/internal/logger/tools_logger_test.go b/internal/logger/tools_logger_test.go index bd3af5367..b799cd757 100644 --- a/internal/logger/tools_logger_test.go +++ b/internal/logger/tools_logger_test.go @@ -214,8 +214,8 @@ func TestToolsLoggerFallback(t *testing.T) { assert.NoError(err, "CloseAllLoggers failed") } -// TestWriteToFile_Success verifies writeJSON writes valid JSON atomically. -func TestWriteToFile_Success(t *testing.T) { +// TestWriteJSON_Success verifies writeJSON writes valid JSON atomically. +func TestWriteJSON_Success(t *testing.T) { assert := assert.New(t) require := require.New(t) @@ -246,8 +246,8 @@ func TestWriteToFile_Success(t *testing.T) { assert.True(os.IsNotExist(err), "temp file should be removed after rename") } -// TestWriteToFile_WriteFileFails verifies the error path when os.WriteFile fails. -func TestWriteToFile_WriteFileFails(t *testing.T) { +// TestWriteJSON_WriteFileFails verifies the error path when os.WriteFile fails. +func TestWriteJSON_WriteFileFails(t *testing.T) { assert := assert.New(t) tl := &ToolsLogger{ @@ -260,9 +260,9 @@ func TestWriteToFile_WriteFileFails(t *testing.T) { assert.ErrorContains(err, "failed to write temp file") } -// TestWriteToFile_RenameFails verifies the error and cleanup path when os.Rename fails. +// TestWriteJSON_RenameFails verifies the error and cleanup path when os.Rename fails. // On Linux, renaming a regular file to a path occupied by a directory returns EISDIR. -func TestWriteToFile_RenameFails(t *testing.T) { +func TestWriteJSON_RenameFails(t *testing.T) { assert := assert.New(t) require := require.New(t)