[frogscooper]

What type of PR is this?

• bug

What this PR does / why we need it:

FlagBase.Set runs the flag's Validator every time the flag is set. For a slice or map flag given more than once, that means the validator sees the value before the later occurrences are added. With a StringSliceFlag whose validator needs at least two values, app -l a,b passes but app -l a -l b fails with invalid value "a" for flag -l: need at least 2 values, got ["a"]. So a rule about the whole value, like a minimum count or a set of required map keys, can't be written for a repeated flag, and when it fails the error points at the wrong value.

Set now does the same thing as before by calling two new unexported methods, setValue and validate. When the command line parser sets a multi-value flag, Command.set calls only setValue and records the name and values it was given. After parseFlags returns without an error, validateMultiValueFlags runs each of those validators once on the full value. The error goes through the same usage error path as before, and it has the same format. For a flag given once, the message is unchanged. For a flag given more than once, it lists every value, like invalid value ["a" "b,c"] for flag -list: ....

Everything else still validates in Set: single-value flags, values from Sources (each source is applied with one Set call, so it already sees the whole value), and calls to Command.Set or Flag.Set from code. ValidateDefaults is unchanged, and a validator still doesn't run for a flag that wasn't set.

Which issue(s) this PR fixes:

None, I didn't find an existing issue for this.

Special notes for your reviewer:

A persistent slice flag given on both a parent command and its subcommand, like app -l a sub -l b, still has its validator run on the parent's part first, because each command validates after its own parse. That fails the same way today, so this PR doesn't make it worse, but fixing it would mean moving validation to the command that runs the action, which also moves the error out of the usage error path. I left it out to keep this small.

Testing

Added TestMultiValueFlagValidation for a slice flag given once, given twice, with too few values, with too many values through an alias, and from an env var, and for a map flag given twice and missing a key. TestMultiValueFlagValidatorRunsOnce checks that the validator is called once with every value. The three cases with repeated flags and the run once test fail on main and pass with the change, and the existing validator tests pass unchanged. make passes, and make v3diff shows no public API change.

Release Notes

The Validator of a slice or map flag now runs once on the full value after the command line is parsed, so a flag given more than once, like `-l a -l b`, is validated with all of its values instead of failing on the first one.

[kimjooyoon]

The intended aggregate-validation change works in the native reproducer, but it also bypasses external Flag.Set overrides on embedded slice/map flags. The inline finding includes an applicable test patch and original base/head outputs.

Four compatibility cases pass at the base and regress here; three controls pass on both. The aggregate-validation case motivating the PR changes from failure to success. Both eight-case runs intentionally exit1 for those different reasons.

Verified with cached Go 1.27.3 on macOS arm64, offline dependencies, two workers and bounded execution. No production implementation, full make, race-suite or hosted-CI validation is claimed. Review and execution were performed autonomously by Codex, an AI coding agent.

[kimjooyoon]

[P2] Preserve custom Set dispatch for embedded multi-value flags

An external flag such as type customFlag struct { cli.StringSliceFlag } with its own public Set(name, value) also satisfies this private interface through its promoted methods. This call then goes directly to the embedded setValue, skipping the caller's override. A Set that rejects an input is never called and the command action runs with no error; a Set that normalizes inputs silently stops doing so. The same occurs with an embedded StringMapFlag.

Eight-case external-package test and original outputs: all four custom rejection/normalization cases pass on 6898360 and fail on this exact head. Scalar overrides and programmatic Command.Set still dispatch correctly. Please preserve external flags' public Set dispatch while deferring built-in validation, and retain an external-package embedding regression. This was executed through the actual cli.Command.Run API, not a copied parser model.

[frogscooper]

thanks for catching this, the test cases are super useful. the new commit makes the deferred validation path only kick in when the flag is one of the built-in flag types. anything that wraps one, with or without its own Set, goes through the public Set like it did before.

[kimjooyoon]

Rechecked e3f5de6e144a9bc76a2c637817f99ff4aff62970: the earlier public Set dispatch finding is addressed. All eight original review cases now pass unchanged, including slice/map rejection and normalization, programmatic dispatch, and the built-in aggregate-validation case. Your eight embedding cases and existing whole-value validation/run-once tests also pass.

The full native go test -json -count=1 -p=2 -mod=readonly -timeout=120s ./... run exited 0 on cached Go 1.27.3/macOS arm64: 1,484 passing leaf tests, 5 skipped, no failures. I have no remaining concern on this dispatch issue. This does not cover race, full make, lint or hosted CI.

Exact command, complete original output, unchanged fixture and before/after comparison. Execution was performed by an AI agent; production source was not modified during this follow-up.