From ef8ed9e6bc4aeacfc298ed5dfee1837a1b1c2226 Mon Sep 17 00:00:00 2001
From: Dominik Schmidt <dev@dominik-schmidt.de>
Date: Fri, 22 May 2026 00:27:26 +0200
Subject: [PATCH] fix: make Color/NoColor/env interaction consistent

Two issues with the env-aware NoColor() introduced together with
HELMFILE_NO_COLOR / NO_COLOR support:

1. Color() consulted the raw GlobalOptions.NoColor field instead of
   NoColor(), so in a TTY with only the env set, Color() fell through
   to terminal autodetect and ValidateConfig() spuriously errored with
   "--color and --no-color cannot be specified at the same time".

2. NoColor() returned true via env even when --color was explicitly
   passed, so `helmfile --color` with NO_COLOR (or HELMFILE_NO_COLOR=true)
   in the environment hit the same ValidateConfig() error. A flag should
   always win over an env var.

Fix both by routing Color() through NoColor() and giving NoColor() an
explicit --color short-circuit. Regression tests added for both paths.

Signed-off-by: Dominik Schmidt <dev@dominik-schmidt.de>
---
 pkg/config/global.go      |  6 ++++-
 pkg/config/global_test.go | 50 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/pkg/config/global.go b/pkg/config/global.go
index 7c42a37e..b7378f10 100644
--- a/pkg/config/global.go
+++ b/pkg/config/global.go
@@ -293,7 +293,7 @@ func (g *GlobalImpl) Color() bool {
 		return c
 	}
 
-	if g.GlobalOptions.NoColor {
+	if g.NoColor() {
 		return false
 	}
 
@@ -313,6 +313,10 @@ func (g *GlobalImpl) NoColor() bool {
 	if g.GlobalOptions.NoColor {
 		return true
 	}
+	// Explicit --color short-circuits env-derived no-color: a flag must win over an env var.
+	if g.GlobalOptions.Color {
+		return false
+	}
 	if os.Getenv(envvar.NoColor) == "true" {
 		return true
 	}
diff --git a/pkg/config/global_test.go b/pkg/config/global_test.go
index a1aeb923..15547309 100644
--- a/pkg/config/global_test.go
+++ b/pkg/config/global_test.go
@@ -370,3 +370,53 @@ func TestNoColor(t *testing.T) {
 	os.Unsetenv(envvar.NoColor)
 	os.Unsetenv("NO_COLOR")
 }
+
+// TestColorRespectsNoColorEnv guards against ValidateConfig() firing when
+// HELMFILE_NO_COLOR / NO_COLOR is set without an explicit --color/--no-color flag.
+// Color() must consult NoColor() (which is env-aware) before falling back to TTY autodetect.
+func TestColorRespectsNoColorEnv(t *testing.T) {
+	tests := []struct {
+		name        string
+		helmfileEnv string
+		standardEnv string
+	}{
+		{name: "HELMFILE_NO_COLOR=true", helmfileEnv: "true"},
+		{name: "NO_COLOR set", standardEnv: "1"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Setenv(envvar.NoColor, test.helmfileEnv)
+			t.Setenv("NO_COLOR", test.standardEnv)
+			g := NewGlobalImpl(&GlobalOptions{})
+			require.True(t, g.NoColor(), "NoColor() should be true when env is set")
+			require.False(t, g.Color(), "Color() should be false when NoColor() is true via env")
+			require.NoError(t, g.ValidateConfig(), "ValidateConfig() should not error from env-only no-color")
+		})
+	}
+}
+
+// TestColorFlagOverridesNoColorEnv guards against ValidateConfig() firing when
+// --color is explicitly passed but HELMFILE_NO_COLOR / NO_COLOR is set in the
+// environment. The flag must win over the env var.
+func TestColorFlagOverridesNoColorEnv(t *testing.T) {
+	tests := []struct {
+		name        string
+		helmfileEnv string
+		standardEnv string
+	}{
+		{name: "HELMFILE_NO_COLOR=true", helmfileEnv: "true"},
+		{name: "NO_COLOR set", standardEnv: "1"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Setenv(envvar.NoColor, test.helmfileEnv)
+			t.Setenv("NO_COLOR", test.standardEnv)
+			g := NewGlobalImpl(&GlobalOptions{Color: true})
+			require.True(t, g.Color(), "Color() should be true when --color is set")
+			require.False(t, g.NoColor(), "NoColor() should be false when --color is set, even if env says otherwise")
+			require.NoError(t, g.ValidateConfig(), "ValidateConfig() should not error when --color overrides env no-color")
+		})
+	}
+}