Refactor logger persistence wrappers and document env expansion semantics

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)

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 {

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.

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

internal/logger/tools_logger_test.go

@@ -214,8 +214,8 @@ func TestToolsLoggerFallback(t *testing.T) {
 	assert.NoError(err, "CloseAllLoggers failed")
 }
 
-// TestWriteToFile_Success verifies writeToFile 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)
 
@@ -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"))
@@ -246,23 +246,23 @@ 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{
 		jsonFileSink: jsonFileSink{logDir: "/nonexistent/dir/that/does/not/exist", fileName: "tools.json"},
 		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")
 }
 
-// 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)
 
@@ -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.