Merge pull request #1699 from dgageot/fix-issues

Fix issues found by the review agent

Author: dgageot

cmd/root/api.go

@@ -56,18 +56,26 @@ func newAPICmd() *cobra.Command {
 
 // monitorStdin monitors stdin for EOF, which indicates the parent process has died.
 // When spawned with piped stdio, stdin closes when the parent process dies.
+// The caller is responsible for cancelling the context (e.g. via defer cancel()).
 func monitorStdin(ctx context.Context, cancel context.CancelFunc, stdin *os.File) {
-	// Close stdin when context is cancelled to unblock the read
+	done := make(chan struct{})
+
+	// Close stdin when context is cancelled to unblock the read.
+	// Also exits cleanly when monitorStdin returns.
 	go func() {
-		<-ctx.Done()
+		select {
+		case <-ctx.Done():
+		case <-done:
+		}
 		stdin.Close()
 	}()
 
+	defer close(done)
+
 	buf := make([]byte, 1)
 	for {
 		n, err := stdin.Read(buf)
 		if err != nil || n == 0 {
-			// Only log and cancel if context isn't already done (parent died)
 			if ctx.Err() == nil {
 				slog.Info("stdin closed, parent process likely died, shutting down")
 				cancel()
@@ -113,10 +121,14 @@ func (f *apiFlags) runAPICommand(cmd *cobra.Command, args []string) error {
 	}()
 
 	// Start recording proxy if --record is specified
-	if _, cleanup, err := setupRecordingProxy(f.recordPath, &f.runConfig); err != nil {
+	if _, recordCleanup, err := setupRecordingProxy(f.recordPath, &f.runConfig); err != nil {
 		return err
-	} else if cleanup != nil {
-		defer cleanup()
+	} else if recordCleanup != nil {
+		defer func() {
+			if err := recordCleanup(); err != nil {
+				slog.Error("Failed to cleanup recording proxy", "error", err)
+			}
+		}()
 	}
 
 	if f.pullIntervalMins > 0 && !config.IsOCIReference(agentsPath) {

cmd/root/record.go

@@ -45,9 +45,9 @@ func setupFakeProxy(fakeResponses string, streamDelayMs int, runConfig *config.R
 // and normalizes the path by stripping any .yaml suffix.
 // Returns the cassette path (with .yaml extension) and a cleanup function.
 // The cleanup function must be called when done (typically via defer).
-func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (cassettePath string, cleanup func(), err error) {
+func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (cassettePath string, cleanup func() error, err error) {
 	if recordPath == "" {
-		return "", func() {}, nil
+		return "", func() error { return nil }, nil
 	}
 
 	// Handle auto-generated filename (from NoOptDefVal)
@@ -67,9 +67,5 @@ func setupRecordingProxy(recordPath string, runConfig *config.RuntimeConfig) (ca
 
 	slog.Info("Recording mode enabled", "cassette", cassettePath, "proxy", proxyURL)
 
-	return cassettePath, func() {
-		if err := cleanupFn(); err != nil {
-			slog.Error("Failed to cleanup recording proxy", "error", err)
-		}
-	}, nil
+	return cassettePath, cleanupFn, nil
 }

cmd/root/record_test.go

@@ -21,7 +21,7 @@ func TestSetupRecordingProxy_EmptyPath(t *testing.T) {
 	assert.NotNil(t, cleanup)
 	assert.Empty(t, runConfig.ModelsGateway, "ModelsGateway should not be set")
 
-	cleanup()
+	require.NoError(t, cleanup())
 }
 
 func TestSetupRecordingProxy_AutoGeneratesFilename(t *testing.T) {
@@ -31,7 +31,7 @@ func TestSetupRecordingProxy_AutoGeneratesFilename(t *testing.T) {
 
 	cassettePath, cleanup, err := setupRecordingProxy("true", &runConfig)
 	require.NoError(t, err)
-	defer cleanup()
+	defer func() { require.NoError(t, cleanup()) }()
 
 	assert.True(t, strings.HasPrefix(cassettePath, "cagent-recording-"), "should have auto-generated prefix")
 	assert.True(t, strings.HasSuffix(cassettePath, ".yaml"), "should have .yaml suffix")
@@ -46,7 +46,7 @@ func TestSetupRecordingProxy_CreatesProxy(t *testing.T) {
 
 	resultPath, cleanup, err := setupRecordingProxy(cassettePath, &runConfig)
 	require.NoError(t, err)
-	defer cleanup()
+	defer func() { require.NoError(t, cleanup()) }()
 
 	assert.Equal(t, cassettePath+".yaml", resultPath)
 	assert.True(t, strings.HasPrefix(runConfig.ModelsGateway, "http://"), "ModelsGateway should be HTTP URL")

cmd/root/root.go

@@ -65,7 +65,9 @@ func NewRootCmd() *cobra.Command {
 		},
 		PersistentPostRunE: func(cmd *cobra.Command, args []string) error {
 			if flags.logFile != nil {
-				_ = flags.logFile.Close()
+				if err := flags.logFile.Close(); err != nil {
+					slog.Error("Failed to close log file", "error", err)
+				}
 			}
 			return nil
 		},

cmd/root/run.go

@@ -119,7 +119,7 @@ func (f *runExecFlags) runRunCommand(cmd *cobra.Command, args []string) error {
 	return f.runOrExec(ctx, out, args, tui)
 }
 
-func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []string, tui bool) error {
+func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []string, tui bool) (retErr error) {
 	slog.Debug("Starting agent", "agent", f.agentName)
 
 	// Start CPU profiling if requested
@@ -193,6 +193,9 @@ func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []s
 	defer func() {
 		if err := fakeCleanup(); err != nil {
 			slog.Error("Failed to cleanup fake proxy", "error", err)
+			if retErr == nil {
+				retErr = fmt.Errorf("failed to cleanup fake proxy: %w", err)
+			}
 		}
 	}()
 
@@ -202,7 +205,14 @@ func (f *runExecFlags) runOrExec(ctx context.Context, out *cli.Printer, args []s
 		return err
 	}
 	if cassettePath != "" {
-		defer recordCleanup()
+		defer func() {
+			if err := recordCleanup(); err != nil {
+				slog.Error("Failed to cleanup recording proxy", "error", err)
+				if retErr == nil {
+					retErr = fmt.Errorf("failed to cleanup recording proxy: %w", err)
+				}
+			}
+		}()
 		out.Println("Recording mode enabled, cassette: " + cassettePath)
 	}