fix: address PR review comments

- Fix temp file leak in prepareStdinValues on error paths
- Use %q for quoted chart paths in error messages
- Nil out manifest byte slices after parsing to reduce peak memory
- Refactor tests to use captureStdout helper
- Add tests for prepareStdinValues stdin handling

Signed-off-by: yxxhero <aiopsclub@163.com>

Author: yxxhero

cmd/local.go

@@ -119,12 +119,12 @@ func (l *local) run() error {
 
 	manifest1, err := l.renderChart(l.chart1)
 	if err != nil {
-		return fmt.Errorf("failed to render chart %s: %w", l.chart1, err)
+		return fmt.Errorf("failed to render chart %q: %w", l.chart1, err)
 	}
 
 	manifest2, err := l.renderChart(l.chart2)
 	if err != nil {
-		return fmt.Errorf("failed to render chart %s: %w", l.chart2, err)
+		return fmt.Errorf("failed to render chart %q: %w", l.chart2, err)
 	}
 
 	excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook}
@@ -134,6 +134,8 @@ func (l *local) run() error {
 
 	specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)
 	specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)
+	manifest1 = nil
+	manifest2 = nil
 
 	seenAnyChanges := diff.Manifests(specs1, specs2, &l.Options, os.Stdout)
 
@@ -165,10 +167,12 @@ func (l *local) prepareStdinValues() (func(), error) {
 
 				if _, err := tmpfile.Write(data); err != nil {
 					_ = tmpfile.Close()
+					_ = os.Remove(tmpfile.Name())
 					return nil, err
 				}
 
 				if err := tmpfile.Close(); err != nil {
+					_ = os.Remove(tmpfile.Name())
 					return nil, err
 				}
 

cmd/local_test.go

@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"bytes"
 	"errors"
 	"os"
 	"strings"
@@ -86,31 +85,20 @@ data:
 	chart1 := t.TempDir()
 	chart2 := t.TempDir()
 
-	oldStdout := os.Stdout
-	r, w, err := os.Pipe()
-	if err != nil {
-		t.Fatalf("Failed to create pipe: %v", err)
-	}
-	os.Stdout = w
-
-	cmd := localCmd()
-	cmd.SetArgs([]string{chart1, chart2})
+	output, err := captureStdout(func() {
+		cmd := localCmd()
+		cmd.SetArgs([]string{chart1, chart2})
 
-	err = cmd.Execute()
-	w.Close()
-	os.Stdout = oldStdout
+		if execErr := cmd.Execute(); execErr != nil {
+			t.Errorf("Expected no error but got: %v", execErr)
+		}
+	})
 
 	if err != nil {
-		t.Fatalf("Expected no error but got: %v", err)
-	}
-
-	var buf bytes.Buffer
-	if _, err := buf.ReadFrom(r); err != nil {
-		t.Fatalf("Failed to read from pipe: %v", err)
+		t.Fatalf("Failed to capture stdout: %v", err)
 	}
-	r.Close()
-	if buf.String() != "" {
-		t.Errorf("Expected no output when charts are identical, got: %q", buf.String())
+	if output != "" {
+		t.Errorf("Expected no output when charts are identical, got: %q", output)
 	}
 }
 
@@ -138,30 +126,18 @@ data:
 	chart1 := t.TempDir()
 	chart2 := t.TempDir()
 
-	oldStdout := os.Stdout
-	r, w, err := os.Pipe()
-	if err != nil {
-		t.Fatalf("Failed to create pipe: %v", err)
-	}
-	os.Stdout = w
+	output, err := captureStdout(func() {
+		cmd := localCmd()
+		cmd.SetArgs([]string{chart1, chart2})
 
-	cmd := localCmd()
-	cmd.SetArgs([]string{chart1, chart2})
-
-	err = cmd.Execute()
-	w.Close()
-	os.Stdout = oldStdout
+		if execErr := cmd.Execute(); execErr != nil {
+			t.Errorf("Expected no error but got: %v", execErr)
+		}
+	})
 
 	if err != nil {
-		t.Fatalf("Expected no error but got: %v", err)
-	}
-
-	var buf bytes.Buffer
-	if _, err := buf.ReadFrom(r); err != nil {
-		t.Fatalf("Failed to read from pipe: %v", err)
+		t.Fatalf("Failed to capture stdout: %v", err)
 	}
-	r.Close()
-	output := buf.String()
 
 	if !strings.Contains(output, "value1") || !strings.Contains(output, "value2") {
 		t.Errorf("Expected diff output containing value1 and value2, got: %q", output)
@@ -280,6 +256,92 @@ func TestLocalCmdHelmTemplateError(t *testing.T) {
 	}
 }
 
+func TestPrepareStdinValues(t *testing.T) {
+	l := &local{
+		valueFiles: valueFiles{"-", "-", "real-values.yaml"},
+	}
+
+	stdinContent := `key: stdin-value
+`
+	tmpStdin, err := os.CreateTemp("", "fake-stdin")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(tmpStdin.Name())
+	if _, err := tmpStdin.WriteString(stdinContent); err != nil {
+		t.Fatal(err)
+	}
+	if err := tmpStdin.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	oldStdin := os.Stdin
+	f, err := os.Open(tmpStdin.Name())
+	if err != nil {
+		t.Fatal(err)
+	}
+	os.Stdin = f
+	defer func() {
+		os.Stdin = oldStdin
+		f.Close()
+	}()
+
+	cleanup, err := l.prepareStdinValues()
+	if err != nil {
+		t.Fatalf("Expected no error but got: %v", err)
+	}
+	if cleanup == nil {
+		t.Fatal("Expected cleanup function but got nil")
+	}
+
+	if l.valueFiles[0] == "-" {
+		t.Error("Expected first valueFile to be replaced with temp file path")
+	}
+	if l.valueFiles[1] == "-" {
+		t.Error("Expected second valueFile to be replaced with temp file path")
+	}
+	if l.valueFiles[0] != l.valueFiles[1] {
+		t.Errorf("Expected both '-' entries to point to the same temp file, got %q and %q", l.valueFiles[0], l.valueFiles[1])
+	}
+	if _, err := os.Stat(l.valueFiles[0]); os.IsNotExist(err) {
+		t.Errorf("Expected temp file %q to exist", l.valueFiles[0])
+	}
+	if l.valueFiles[2] != "real-values.yaml" {
+		t.Errorf("Expected third valueFile to be unchanged, got %q", l.valueFiles[2])
+	}
+
+	data, err := os.ReadFile(l.valueFiles[0])
+	if err != nil {
+		t.Fatalf("Failed to read temp file: %v", err)
+	}
+	if string(data) != stdinContent {
+		t.Errorf("Expected temp file to contain stdin content %q, got %q", stdinContent, string(data))
+	}
+
+	cleanup()
+
+	if _, err := os.Stat(l.valueFiles[0]); !os.IsNotExist(err) {
+		t.Errorf("Expected temp file %q to be removed after cleanup", l.valueFiles[0])
+	}
+}
+
+func TestPrepareStdinValuesNoStdin(t *testing.T) {
+	l := &local{
+		valueFiles: valueFiles{"values1.yaml", "values2.yaml"},
+	}
+
+	cleanup, err := l.prepareStdinValues()
+	if err != nil {
+		t.Fatalf("Expected no error but got: %v", err)
+	}
+	if cleanup != nil {
+		t.Error("Expected nil cleanup function when no stdin values")
+	}
+	if l.valueFiles[0] != "values1.yaml" || l.valueFiles[1] != "values2.yaml" {
+		t.Errorf("Expected valueFiles to be unchanged, got %v", l.valueFiles)
+	}
+}
+
 func setupFakeHelm(t *testing.T, mode, output, argsFile, countFile string) {
 	t.Helper()
 	t.Setenv("HELM_DIFF_FAKE_HELM", "1")