fix: Propagating flag parse errors instead of swallowing them (#6732)

Author: yhakbar

internal/cli/flags/flag.go

@@ -3,6 +3,7 @@ package flags
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"io"
 	"strconv"
@@ -164,16 +165,20 @@ func (newFlag *Flag) Parse(args clihelper.Args, env map[string]string) error {
 		return err
 	}
 
-	const maxFlagsParse = 1000 // Maximum flags parse
+	const maxFlagsParse = 1000
 
 	for range maxFlagsParse {
 		err := flagSet.Parse(args)
 		if err == nil {
-			break
+			return nil
 		}
 
-		if errStr := err.Error(); !strings.HasPrefix(errStr, clihelper.ErrMsgFlagUndefined) {
-			break
+		// The set holds only this flag, so the loop skips flags owned by other
+		// parsers instead of failing on them. That includes -h and --help, which
+		// arrive as [flag.ErrHelp] rather than as an undefined-flag error.
+		if !errors.Is(err, flag.ErrHelp) &&
+			!strings.HasPrefix(err.Error(), clihelper.ErrMsgFlagUndefined) {
+			return err
 		}
 
 		args = flagSet.Args()

internal/cli/flags/flag_test.go

@@ -2,6 +2,7 @@ package flags_test
 
 import (
 	"bytes"
+	"errors"
 	"flag"
 	"fmt"
 	"strings"
@@ -167,3 +168,84 @@ func TestFlag_Evaluate(t *testing.T) {
 		})
 	}
 }
+
+func TestFlag_Parse(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name        string
+		expected    string
+		args        clihelper.Args
+		expectedErr bool
+	}{
+		{
+			name:     "accepted value",
+			args:     clihelper.Args{"--level", "debug"},
+			expected: "debug",
+		},
+		{
+			name:        "value rejected by the flag setter",
+			args:        clihelper.Args{"--level", "bogus"},
+			expectedErr: true,
+		},
+		{
+			name:        "value missing",
+			args:        clihelper.Args{"--level"},
+			expectedErr: true,
+		},
+		{
+			name: "flag belonging to another parser",
+			args: clihelper.Args{"--some-other-flag"},
+		},
+		{
+			name:     "value after a flag belonging to another parser",
+			args:     clihelper.Args{"--some-other-flag", "--level", "debug"},
+			expected: "debug",
+		},
+		{
+			name:        "rejected value after a flag belonging to another parser",
+			args:        clihelper.Args{"--some-other-flag", "--level", "bogus"},
+			expectedErr: true,
+		},
+		{
+			name:     "value after --help",
+			args:     clihelper.Args{"--help", "--level", "debug"},
+			expected: "debug",
+		},
+		{
+			name: "-h alone",
+			args: clihelper.Args{"-h"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			var got string
+
+			testFlag := flags.NewFlag(&clihelper.GenericFlag[string]{
+				Name: "level",
+				Setter: func(val string) error {
+					if val == "bogus" {
+						return errors.New("unsupported level")
+					}
+
+					got = val
+
+					return nil
+				},
+			})
+
+			err := testFlag.Parse(tc.args, map[string]string{})
+
+			if tc.expectedErr {
+				require.Error(t, err)
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tc.expected, got)
+		})
+	}
+}