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

Summary by CodeRabbit

• New Features

  • Command-line options are now validated automatically before commands are built.
  • Supports range, pattern, custom value, and custom validation rules.
  • Validation errors identify affected options with consistent formatting.
  • Sensitive values are redacted from validation and telemetry errors.

• Bug Fixes

  • Invalid options now fail early instead of producing invalid commands.
  • Failed command creation no longer increments execution counts.
  • Validation failures are recorded correctly in command telemetry.

[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.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The command-line build path now validates options before tool resolution and command construction. Validation errors are deterministic and obfuscated. Command creation failures are traced, and execution counting starts only after successful creation. Source-generator metadata now preserves option properties and handles schema compatibility.

Changes

Command option validation

Layer / File(s)	Summary
Validation engine src/ModularPipelines/Context/CommandLineOptionsValidator.cs, src/ModularPipelines/Context/CommandModelProvider.cs, src/ModularPipelines/Exceptions/*	Adds cached validation metadata, public and non-public property validation, service-aware callbacks, deterministic error formatting, secret obfuscation, and CommandOptionsValidationException.
Build integration and command tracing src/ModularPipelines/Context/CommandLineBuilder.cs, src/ModularPipelines/Context/Command.cs, test/ModularPipelines.UnitTests/Context/CommandLineBuilderTests.cs, test/ModularPipelines.UnitTests/Helpers/CommandTests.cs, test/ModularPipelines.UnitTests/Tracing/TelemetryIntegrationTests.cs, test/ModularPipelines.Ansible.UnitTests/Attributes/AnsibleOptionsTests.cs	Build validates options before command construction. Command creation failures are traced with obfuscated messages. Tests cover invalid values, callback failures, secret redaction, telemetry, and execution counts.
Metadata generation and compatibility coverage src/ModularPipelines.SourceGenerator/CommandOptionsGenerator.cs, test/ModularPipelines.SourceGenerator.UnitTests/IncompleteMetadataDiagnosticTests.cs	Generated registration adds dependency annotations and preserves command-option properties. Metadata handling distinguishes complete and incomplete command or secret coverage. Tests cover trimmed internal and external options.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to ed75a

Conflicting command attributes on inherited options can be silently resolved to the derived definition instead of rejected, which may generate incorrect CLI behavior. Merge should wait for this bounded generator correctness issue to be fixed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant CommandLineBuilder
  participant CommandLineOptionsValidator
  participant Command
  participant Telemetry
  Test->>CommandLineBuilder: Build options
  CommandLineBuilder->>CommandLineOptionsValidator: Validate options
  CommandLineOptionsValidator-->>CommandLineBuilder: Return valid options or exception
  CommandLineBuilder->>Command: Create command
  Command->>Telemetry: Record success or obfuscated failure
  Command-->>Test: Return command or exception

Loading

Possibly related PRs

• thomhurst/ModularPipelines#3860: Both changes modify CommandOptionsGenerator and command metadata handling.

Poem

A rabbit checks each option field,
And keeps secret values concealed.
Invalid flags stop before they run,
Valid commands proceed as one.
Metadata guides trimmed code,
Tests confirm the updated load.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.87% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies enforcement of validation for generated command options, which is the primary change.
Linked Issues check	✅ Passed	The changes enforce generated option validation during Build, support required validation attributes, cache eligibility, and report offending types and properties for issue #3778.
Out of Scope Changes check	✅ Passed	The changes remain within validation, error handling, metadata generation, command counting, trimming support, and related test coverage for issue #3778.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-3778-option-validation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thomhurst]

Rebased onto current main and resolved the CommandLineBuilder documentation conflict by preserving both validation-first and phase-aware argument ordering. Current-head validation: CommandLineBuilderTests 62/62; AnsibleOptionsTests 3/3; ModularPipelines.slnx and Ansible solution Release builds both 0 warnings/errors. Core and Ansible changed-file whitespace checks pass; the unit-test project reports one pre-existing whitespace diagnostic at CommandLineBuilderTests.cs:1194, outside this PR's changed lines.

@codex review
@claude review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aef09feacc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review — PR #3857

Reviewed the DataAnnotations validation enforcement in CommandLineBuilder.Build. The core approach (cache validation-eligibility per type via ConditionalWeakTable, validate before rendering, aggregate all invalid properties into one message) is sound and well-tested (the new CommandLineBuilderTests and AnsibleOptionsTests cases look thorough). A few issues worth addressing before merge:

1. ValidationException breaks the framework's documented exception contract (medium)

src/ModularPipelines/Context/CommandLineOptionsValidator.cs:42 throws a raw System.ComponentModel.DataAnnotations.ValidationException instead of a ModularPipelines.Exceptions.PipelineException-derived type.

PipelineException.cs explicitly documents itself as "the root exception class from which all ModularPipelines-specific exceptions derive" and shows catch (PipelineException ex) as the way to "handle any pipeline-related error." CommandException and PipelineValidationException already exist in that hierarchy specifically for command/validation failures. Any consumer (or the framework's own module-failure handling) that follows the documented pattern of catching PipelineException will not catch this new failure mode — invalid options (e.g. AnsibleExecuteOptions.Verbose = 7) now surface as an unrecognized BCL exception type instead of the framework's own vocabulary.

Suggestion: wrap/throw a PipelineValidationException (or a new CommandOptionsValidationException : PipelineException) that carries the DataAnnotations ValidationException as InnerException, so callers relying on the documented hierarchy keep working, while the original validation detail is still available.

2. Inline step-numbering comments now disagree with the updated <remarks> doc (low)

src/ModularPipelines/Context/CommandLineBuilder.cs:14 — the class-level XML doc's numbered step list was updated to insert "1. Validate DataAnnotations..." and renumber the rest (2-8), but the inline // 1. Resolve tool name... through // 7. A terminal option must not follow... comments in the method body (lines 50-161) were left as-is. The two numbering schemes are now off by one, so a reader cross-referencing the doc comment against the method body will be looking at the wrong step.

Suggestion: renumber the inline comments to match (2-8), or better, drop numbers from one of the two and let the other be the single source of truth to avoid this drifting again.

3. Validation failures bypass command-level tracing/secret obfuscation (medium)

In src/ModularPipelines/Context/Command.cs, CreateCommand(options, execOpts) (line 69, which triggers CommandLineBuilder.Build → CommandLineOptionsValidator.Validate) runs before ModuleActivityTracing.StartCommandActivity (line 90) and before the try/catch block that applies _secretObfuscator.Obfuscate(exception.Message, execOpts) (line 130). So when validation throws, that specific command gets no command-level Activity/telemetry recorded, and the exception message isn't run through secret obfuscation at this layer (it's only caught and obfuscated later by the outer module-level handler in ModuleRunner, if at all).

Given secrets can appear in option values, and the module's own docs emphasize automatic secret obfuscation in logs as a core guarantee, this is worth closing rather than relying on an outer catch-all.

Suggestion: move the Validate call (or at least wrap it) inside the try block / after the Activity is started, so validation failures get the same tracing and obfuscation treatment as execution failures.

---

None of these are blockers to the core design — the validation mechanism itself (per-type eligibility caching, aggregating multiple invalid properties into one message, IValidatableObject + ValidationAttribute support) is a good fit for this codebase's generated-options pattern. #1 and #3 are the ones I'd want resolved before merge since they affect error-handling consumers and secret-safety guarantees; #2 is a docs nit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfd75c7be5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@claude reviewnn@codex reviewnnAddressed all review areas on exact head a9bdfb3383:n- trim/AOT validation metadata is preserved by generated DynamicDependency attributes; schema-1 packages are rescanned under schema 2n- validation now throws CommandOptionsValidationException : PipelineException with the DataAnnotations exception retained as innern- command creation/validation failures create error activities with obfuscated messagesn- inline step numbering matches XML documentationnnValidation: generator regressions 3/3; command/telemetry regressions 3/3; Ansible regression 1/1; core Release build 0 warnings/0 errors. Full generator diagnostic class hit the fixed 2 GB local agent guard (3.1 GB) and was not retried; CI will cover it.

[claude]

Code Review — PR #3857 (head a9bdfb3383)

Re-reviewed after the fixes in bfd75c7be5 and a9bdfb3383. Good progress on the issues raised in earlier rounds:

Resolved since last round:

• ✅ ValidationException → CommandOptionsValidationException : PipelineException (with the DataAnnotations exception preserved as InnerException). Consumers following the documented "catch PipelineException" pattern now work correctly.
• ✅ Validation failures now get command-level Activity tracing and secret obfuscation — CreateCommand is wrapped in its own try/catch in Command.cs that starts an activity and obfuscates the failure message even when validation throws before a CliWrap.Command exists. Well covered by the new Command_Validation_Failure_Is_Recorded_And_Obfuscated test in TelemetryIntegrationTests.cs, which asserts the secret is obfuscated in the recorded exception message.
• ✅ Inline step-numbering comments in CommandLineBuilder.Build now match the <remarks> doc (1–8 in both places).
• ✅ Trim/AOT metadata gap (Codex P1): generated types now carry [DynamicDependency(PublicProperties)], with schema versioning so old (schema-1) packages get rescanned. Covered by new/expanded IncompleteMetadataDiagnosticTests.

Still open — two Codex P2 findings from this round don't look addressed yet:

1. _commandExecutionCounter.Record(...) runs before validation, so rejected commands still get counted (src/ModularPipelines/Context/Command.cs:66). ExecuteCommandLineToolAsync increments the counter unconditionally as its first line, then calls CreateCommand (which now runs CommandLineOptionsValidator.Validate) inside a try/catch that only handles tracing. When validation throws, no command is ever built or run, yet CommandCount/ModuleCommandCounts in the persisted run report already reflect an "executed" command that never happened. I checked RunReportTests.cs — it exercises the counter extensively but has no case for a validation failure, so this regression path is untested and unfixed.

   Rather than just reordering two lines (which would still couple metrics to argument construction), consider making this hard to regress again: increment the counter from the same success path that produces a CommandResult/CommandLine (i.e., after CreateCommand returns), not as a side effect of entering the method. That keeps "a command was counted" and "a command was actually going to run" as the same invariant instead of two things that happen to be ordered correctly today.

2. Reflection-based validation only sees public properties, but the generator legitimately emits internal/protected internal CLI properties (src/ModularPipelines/Context/CommandLineOptionsValidator.cs:58, RequiresValidation, and the Validator.TryValidateObject(... validateAllProperties: true) call above it). Type.GetProperties() with no BindingFlags only returns public members, and Validator.TryValidateObject has the same restriction. I confirmed in CommandOptionsGenerator.IsPropertyAccessible (line ~1834) that Accessibility.Internal and Accessibility.ProtectedOrInternal are explicitly treated as valid, generator-emitted CLI properties — so a [Range]/[Required] on such a property is silently never checked, while the property itself still renders to the command line.

   This is the more interesting architectural gap: the validator is reflecting over the CLR type from scratch, duplicating work the source generator/ICommandModelProvider already did to figure out exactly which properties are real CLI options (including accessibility rules). Rather than a second, independently-scoped reflection pass that can drift from what actually gets rendered, it'd be more robust to validate the same property set the CommandModelProvider/ICommandModelProvider already resolved for rendering (its PropertyCommandLineParts carry the PropertyInfo/accessor needed to build a ValidationContext per member). That guarantees "what gets validated" and "what gets rendered" can never diverge, and sidesteps needing BindingFlags.NonPublic (which would itself need care under trimming — right now the IL2070/IL2026 suppressions are scoped to public-only reflection).

Neither of these blocks the core design — the validation mechanism, exception hierarchy, and trim-safety story are all solid now — but both are real correctness gaps in the enforcement contract this PR is establishing, and neither has a regression test guarding it. I'd want at least #1 addressed before merge since it silently corrupts persisted run-report metrics; #2 is lower risk today (no internal CLI properties currently carry validation attributes) but will bite silently the same way the original #3778 gap did once someone adds one.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9bdfb3383

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@claude review

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea9fddc55d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Code review (re-verification at cf753854)

Head status: current head cf753854 is a byte-identical rebase of 0cd82acba (same tree diff, same two files — CommandOptionsGenerator.cs and IncompleteMetadataDiagnosticTests.cs), which I already reviewed and approved in my last pass. No new logical changes since then, and everything flagged in earlier rounds has been addressed.

Since the incremental commits have each been reviewed narrowly, I also did a holistic architectural pass over the full PR diff (validation-before-render enforcement for generated command options). Two points worth raising, both verified against the current source:

1. Two structurally unrelated "validation" concepts now coexist, with a same-name type collision. The codebase already has a pre-flight validator for PipelineOptions: ModularPipelines.Validation.IOptionsValidator / Validation.ValidationResult (src/ModularPipelines/Validation/ValidationResult.cs). This PR adds a second, unrelated one — CommandLineOptionsValidator (src/ModularPipelines/Context/CommandLineOptionsValidator.cs) — built around System.ComponentModel.DataAnnotations.ValidationResult. Nothing links the two (no shared interface, no shared naming convention), and having two differently-namespaced ValidationResult types in the same codebase is a standing trap for using-directive ambiguity and IDE autocomplete mistakes for future contributors. Not a blocker, but worth a short doc comment or a shared marker interface so the next person extending "validation" picks the right pattern on the first try.

2. No opt-out for a hard-throw enforced against auto-generated, help-text-scraped constraints. The [Range]/[Required]/etc. attributes now enforced are generated by scraping each tool's --help output (tools/ModularPipelines.OptionsGenerator), not hand-verified per integration across ~15+ tools (Docker, DotNet, Git, Helm, Terraform, Azure, AWS, Ansible, …). I confirmed there's no SkipValidation/bypass mechanism anywhere in src/ModularPipelines/. Since this PR intentionally turns "silently build a possibly-wrong command" into "hard ValidationException before execution," a single stale or mis-scraped constraint in any tool package (e.g. a CLI version bump that changes an accepted range) now hard-fails pipelines with no mitigation short of avoiding the property entirely. Given the description explicitly flags this as "intentionally breaking," it'd be worth adding a narrow, explicit escape hatch (e.g. a per-call or per-property suppression) so a bad scrape doesn't become an unpatchable regression for downstream consumers until the next release.

Everything else — the validation-eligibility cache (ConditionalWeakTable<Type, ValidationMetadata>, appropriate given the plugin-assembly-unloading concern), the secret redaction path (correctly centralized through the existing ISecretObfuscator, not duplicated), and the exception design (CommandOptionsValidationException : PipelineException, fitting the existing taxonomy) — looks solid and consistent with the rest of the codebase.

No correctness bugs found beyond what prior review rounds already covered.