Enforce generated command option validation

[thomhurst]

Summary

• enforce DataAnnotations before rendering command options
• cache validation eligibility per option type without pinning plugin assemblies
• report all invalid Type.Property values and cover an actual generated Ansible [Range]

This is intentionally breaking: invalid generated option values now throw ValidationException before command rendering.

Validation

• CommandLineBuilderTests: 57/57
• Ansible.UnitTests: 4/4
• core Release build: 0 warnings, 0 errors
• Ansible Release build: 0 warnings, 0 errors
• targeted whitespace validation and git diff --check: clean

Closes #3778

[thomhurst]

@claude review

[claude]

Review

This directly implements the fix proposed in #3778 and does it cleanly.

What's good:

• ConditionalWeakTable<Type, ValidationMetadata> for the per-type "does this type need validation" cache is the right call — it avoids pinning dynamically-loaded plugin assemblies (a regular Dictionary<Type, bool> would leak them), and the RequiresValidation short-circuit keeps the hot path (options with no ValidationAttribute/IValidatableObject) essentially free of reflection cost after the first call per type.
• Aggregating all ValidationResults into one ValidationException (rather than throwing on the first failure) matches the PR's stated goal and is genuinely more useful to a pipeline author who fixes one bad property, reruns, and hits the next one.
• Deterministic ordering (.Order(StringComparer.Ordinal)) on the formatted errors is a nice touch for stable test assertions and reproducible CI logs.
• Test coverage is well-targeted: multiple simultaneous violations, the CliOptionValueRangeAttribute wrapper path (via CliOptionValue), a happy-path smoke test, and a regression test against the actual generated AnsibleExecuteOptions.Verbose [Range(0,6)] that #3778 called out as unenforced.
• I checked the blast radius of the "breaking" change: grepping the whole src/ tree, AnsibleExecuteOptions.Verbose is currently the only generated option with a real validation attribute, and its default (0) is within range — so this won't break existing callers who aren't already passing invalid values. Good, low-risk rollout for something flagged as breaking.

One architectural note (non-blocking): Validator.TryValidateObject only validates top-level properties on the options object — it won't recurse into a complex property's own ValidationAttributes or IValidatableObject implementation. Today that's a non-issue: I traced CliTypeMapper/CliOptionDefinition in the options generator and confirmed generated properties are restricted to primitives, enums, and collections thereof, plus KeyValue/CliOptionValue, neither of which carries any validation logic. But since this PR is explicitly building the enforcement contract other generated options will rely on going forward, it might be worth either a one-line remark in CommandLineOptionsValidator noting the "no recursive validation" limitation, or a guard (e.g. a unit test asserting no nested complex option-value type gets silently skipped) so a future generator change that adds a validated nested type doesn't quietly slip past this validator the same way the original issue's attributes slipped past having no validator at all.

No other correctness, design, or test-gap issues found. Approving from a code standpoint.

[thomhurst]

Required pipeline attempt 1 ended in the known core-suite hang/exit 2 with no deterministic assertion or compile error. Requested the single failed-job rerun.

[thomhurst]

The single failed-job rerun also ended in unrelated runner/MSBuild process failure: multiple MSBuild child nodes exited prematurely (MSB4166) with no branch-specific compile error or test assertion. Windows, macOS, analyzers, AOT, and focused local validation are green. Per retry policy, no second rerun triggered.

[thomhurst]

Refreshed onto current main at 79b8c1d. Current-head validation: CommandLineBuilderTests 57/57; Ansible.UnitTests 4/4; core and Ansible Release builds both 0 warnings / 0 errors. Please re-review current head.

[claude]

Re-review (current head 79b8c1d)

Verified this head against the previously reviewed commit (170103a): the diff between them touches only unrelated files pulled in from main via rebase (docs, Azure provisioners, engine/module-executor internals, etc.) — the four files that make up this PR's actual change (CommandLineBuilder.cs, CommandLineOptionsValidator.cs, and the two test files) are byte-for-byte identical to what was already reviewed and approved.

Restating that review's conclusion, which still holds:

What's good:

• ConditionalWeakTable<Type, ValidationMetadata> for the per-type "requires validation" cache is the right structure — it avoids pinning dynamically-loaded plugin assemblies (a plain Dictionary<Type, bool> would leak them), and the RequiresValidation short-circuit keeps options with no ValidationAttribute/IValidatableObject effectively reflection-free after the first call per type.
• Aggregating all ValidationResults into a single ValidationException instead of throwing on the first failure is more useful in practice — a pipeline author fixing one bad property immediately sees the next one instead of re-running iteratively.
• Deterministic ordering (.Order(StringComparer.Ordinal)) on formatted errors keeps assertions and CI logs stable.
• Test coverage is well-targeted: multiple simultaneous violations, the CliOptionValueRangeAttribute/CliOptionValue wrapper path, a happy-path smoke test, and a regression test against the actual generated AnsibleExecuteOptions.Verbose [Range(0,6)] that #3778 flagged as unenforced.
• Blast radius of the breaking change is low: AnsibleExecuteOptions.Verbose is the only generated option today with a real validation attribute, and its default (0) is within range, so existing callers not already passing invalid values are unaffected.

Non-blocking architectural note (carried over, still unaddressed by design — and that's fine): Validator.TryValidateObject only validates top-level properties; it won't recurse into a complex property's own attributes or IValidatableObject. That's a non-issue today (generated option properties are restricted to primitives, enums, and KeyValue/CliOptionValue, none of which carry validation logic), but since this establishes the enforcement contract other generated options will build on, consider either a one-line remark on CommandLineOptionsValidator documenting the "no recursive validation" limitation, or a test asserting a nested complex option type isn't silently skipped — so a future generator change doesn't quietly slip past this validator the same way the original gap in #3778 went unnoticed.

Also noting for context (not a concern): src/ModularPipelines/Validation/OptionsValidator.cs is a separate, pre-existing validator for pipeline-level configuration (PipelineOptions) using a different pattern (IOptionsValidator + collected ValidationResult/ValidationErrorCategory). It's unrelated to this PR's per-command-options CommandLineOptionsValidator and there's no overlap in what each validates — just flagging so the similar naming doesn't cause confusion later.

No correctness or test-gap issues found. Approving.

[thomhurst]

Refreshed again onto current main after Ubuntu's core test host ran 16m8s and exited without a failing-test summary. Validation on f13c011: CommandLineBuilderTests 57/57, Ansible tests 4/4, ModularPipelines.slnx and Ansible solution Release builds both 0 warnings/errors.