address additional review feedback from Copilot

- Fix integration test to capture output and exit code in single execution
  instead of running helmfile twice (more efficient)
- Add detailed documentation explaining why test helper duplication is
  intentional: extracting shared function would require exposing internal
  API and complex refactoring of processChartification dependencies
- Note that integration test exercises actual code path end-to-end

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

pkg/state/issue_2355_test.go

@@ -123,9 +123,21 @@ func TestDryRunServerWithExistingTemplateArgs(t *testing.T) {
 // shouldAddDryRunServer determines whether to add --dry-run=server to template args.
 // This helper function encapsulates the logic from processChartification for testing.
 //
-// IMPORTANT: This function duplicates the command classification logic from
-// processChartification() in state.go (lines 1497-1507). If new commands are added
-// or the classification changes in state.go, this function must be updated to match.
+// NOTE ON DUPLICATION: This function intentionally duplicates the command classification
+// logic from processChartification() in state.go (lines 1497-1523). While extracting this
+// into a shared function would reduce duplication, it would require:
+// 1. Exposing internal implementation details in the public API
+// 2. Complex refactoring of processChartification which has many dependencies (chartify
+//    library, filesystem, HelmState)
+//
+// For this focused bug fix, the duplication is acceptable because:
+// - The integration test (test/integration/test-cases/issue-2355.sh) exercises the actual
+//   processChartification code path end-to-end
+// - This unit test documents the expected behavior and catches regressions quickly
+// - The logic being tested is simple and unlikely to change frequently
+//
+// SYNC WARNING: If the command classification in processChartification() changes
+// (state.go lines 1497-1507), this function must be updated to match.
 //
 // Parameters:
 // - helmfileCommand: the helmfile command being run (diff, apply, template, etc.)
@@ -135,7 +147,7 @@ func TestDryRunServerWithExistingTemplateArgs(t *testing.T) {
 // Returns the updated template args string.
 func shouldAddDryRunServer(helmfileCommand string, validate bool, existingTemplateArgs string) string {
 	// Determine if the command requires cluster access
-	// NOTE: This must be kept in sync with processChartification() in state.go
+	// SYNC: Keep in sync with processChartification() in state.go lines 1497-1507
 	var requiresCluster bool
 	switch helmfileCommand {
 	case "diff", "apply", "sync", "destroy", "delete", "test", "status":

test/integration/test-cases/issue-2355.sh

@@ -14,10 +14,9 @@ test_start "issue-2355: validate flag with kustomize charts (Helm 4 compatibilit
 # Test 1: helmfile diff --validate with kustomize chart should NOT fail due to validate/dry-run mutual exclusion
 # We deliberately use diff here because it is a cluster-requiring command that can trigger --dry-run=server via chartify.
 info "Test 1: Running diff --validate with kustomize chart"
-if ! ${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff --validate > /dev/null 2>&1; then
-    # Capture the error for debugging
-    error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff --validate 2>&1)
-
+error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff --validate 2>&1)
+exit_code=$?
+if [ $exit_code -ne 0 ]; then
     # Check if it's the specific mutual exclusion error we're fixing
     if echo "$error_output" | grep -q "validate.*dry-run.*were all set"; then
         fail "helmfile diff --validate with kustomize failed with mutual exclusion error (issue #2355 not fixed): $error_output"
@@ -29,8 +28,9 @@ fi
 
 # Test 2: Verify that without --validate, the command does not hit the mutual exclusion error
 info "Test 2: Running diff without --validate (baseline test for flag interaction)"
-if ! ${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff > /dev/null 2>&1; then
-    error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff 2>&1)
+error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff 2>&1)
+exit_code=$?
+if [ $exit_code -ne 0 ]; then
     if echo "$error_output" | grep -q "validate.*dry-run.*were all set"; then
         fail "helmfile diff without --validate failed with mutual exclusion error (issue #2355 not fixed): $error_output"
     else