Validate command models eagerly

[thomhurst]

Summary

• validate CLI flag, optional-value, grouped-value, and CliValuePair property shapes when command metadata is cached
• reject duplicate positional argument indexes within the same semantic phase
• emit the required type-compatibility facts in generated metadata while preserving legacy generated optional-string support
• remove renderer checks made unreachable by validated command models

Breaking change

Invalid CLI attribute/property combinations now fail when command metadata is first requested, even when the corresponding option value is null or unset.

Validation

• CliAttributeTests: 63 passed
• reflection-fallback validation test: passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• changed-file whitespace verification and git diff --check: passed

The standalone source-generator test project exceeded the required local 2 GB agent guard before execution. The normal unit-test project also has a pre-existing stale RunReportTests initializer on main; it was temporarily adapted only for local execution and fully reverted before this PR.

Closes #3779

[thomhurst]

@claude review

[claude]

Code review

Reviewed the eager command-model validation change (CommandOptionsGenerator.cs, CommandModelProvider.cs, CommandArgumentBuilder.cs, PropertyCommandLinePart.cs, CliAttributeTests.cs).

Design read: This is a solid architectural improvement. Moving the "grouped option must use space separator" / "value-pair option must use space separator" / "optional-value option must be CliOptionValue" checks out of the renderer (CommandArgumentBuilder, where they only fired if a value happened to be set) and into CommandModelProvider.ValidateModel/ValidateProperty (run once per type via the ConditionalWeakTable cache) is a good fail-fast pattern — it catches misconfigured option classes even when the offending property is never populated at runtime, and it consolidates what used to be three separate scattered throw sites into one declarative switch. The nullable IsSupportedPropertyType tri-state (true = validated supported, false = validated unsupported, null = unknown/not validated) is a nice touch: it lets hand-built PropertyCommandLineParts (used in a few existing unit tests and the OtherGeneratorsCanContributeAotSafeRuntimeMetadata third-party-metadata test) bypass the new eager check instead of breaking them, while still failing closed for anything that actually went through CommandModelProvider/the source generator.

I traced the IsSupportedFlagType / IsSupportedOptionalValueType / IsLegacyGeneratedOption logic through both the Roslyn-symbol version (compile-time, CommandOptionsGenerator.cs) and the System.Type version (runtime fallback, CommandModelProvider.cs), and spot-checked the shipped generated option classes that rely on the legacy string/IEnumerable<string> optional-value allowance (Helm/Podman/Pulumi/Jq *.Generated.cs, all tagged [GeneratedCode("ModularPipelines.OptionsGenerator", ...)]) — they all pass the new validation. No correctness issues found.

One maintainability suggestion (non-blocking): the shape rules in CommandOptionsGenerator.IsSupportedOptionalValueType/IsLegacyGeneratedOption and CommandModelProvider.IsSupportedOptionalValueType/IsLegacyGeneratedOption are two independent implementations of the same business rule (one over ITypeSymbol, one over Type), and this PR itself had to update both in lockstep. Test coverage for the new "optional-value type" and "legacy generated option" rules currently only exercises the generated (compile-time) path (Parser_Rejects_Handwritten_Legacy_Optional_String_Value, Parser_Rejects_Unrelated_Generated_Legacy_Optional_String_Value) — unlike the flag-type rule, which has both a generated-path test and a reflection-fallback test (Reflection_CommandModel_Rejects_Unsupported_Flag_Type via ReflectionInvalidFlagOptions<T>, forcing the reflection fallback with a generic type). Adding an equivalent reflection-path test for the optional-value/legacy rules would give the same drift protection the flag-type rule already has, so a future edit to one implementation without the other gets caught by CI instead of by manual review.

Nothing else stood out — the removed inline separator/format checks in CommandArgumentBuilder are fully subsumed by the new eager validation, and the updated tests correctly assert the new "no value needs to be set to trigger the error" behavior described in the PR's breaking-change note.

[thomhurst]

CI diagnosis: Ubuntu/Windows/macOS fail on the same current-main compile break in RunReportTests.cs:1483-1484: removed PipelineOptions.PrintLogo / PrintResults properties. Focused baseline fix #3864 is already running; this PR does not touch run reports. No rerun until #3864 merges and this branch refreshes.

[thomhurst]

The shared RunReportTests regression is fixed on current main by merged PR #3844. Refreshed this branch onto current main for clean CI and review.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3309e6d28

ℹ️ 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]

Preserve existing multi-operand positional options

When a command model has more than one [CliArgument] in the same phase without an explicit position, every attribute defaults to position 0, so this new key makes GetCommandModel() throw before any values are rendered. The repo already has shipped options in that shape, e.g. AptGetInstallOptions.CommandName/Package at src/ModularPipelines/Options/Linux/AptGet/AptGetInstallOptions.cs:28-32 and NpmTeamAddOptions.Scope/User/Otpcode at src/ModularPipelines.Node/Models/NpmTeamAddOptions.cs:9-11; those commands used to render multiple positional operands in declaration order but now fail even when the caller supplies valid arguments. Either assign distinct positions in the existing models/generator or allow the legacy duplicate-position pattern to continue rendering.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in 7835bb9. Parameterless [CliArgument] metadata now records its position as implicit, so shipped multi-operand models retain stable declaration-order rendering; explicitly duplicated positions still fail. Added direct AptGetInstallOptions coverage plus legacy/default and explicit-duplicate regressions. CLI tests 66/66; core Release build 0 warnings/errors.

[claude]

Code review (re-review after rebase onto main)

The head commit is still b3309e6d28 — identical to the commit reviewed in the prior review — so this is a status check rather than a fresh diff review. Ran two independent CLAUDE.md-compliance passes and two independent bug/security passes over the diff; no compliance violations and no confirmed bugs, logic errors, or security issues were found. I also independently verified the two loose ends a reviewer agent raised but couldn't confirm from the diff alone:

• GetSeparator(attribute) != " " (the removed renderer checks) is exactly equivalent to the new Attribute.Format != OptionFormat.SpaceSeparated check — GetSeparator is a pure switch over Format where only SpaceSeparated maps to " " (CommandArgumentBuilder.cs:564-574). No behavior gap from removing the renderer-side throws.
• The hardcoded false, false added to GetSecretProperties (CommandOptionsGenerator.cs:15-23) is harmless: that method builds secret-redaction metadata, a separate path from the FlagPart/OptionPart construction that actually feeds AppendCommandRegistration, so it can't trigger a spurious eager-validation throw.

Previous suggestion — still open. The earlier review flagged that IsSupportedOptionalValueType/IsLegacyGeneratedOption (and IsSupportedFlagType) are two independent implementations of the same business rule — one over ITypeSymbol (CommandOptionsGenerator.cs), one over System.Type (CommandModelProvider.cs) — and this PR had to hand-edit both in lockstep. Since the commit hasn't changed, that's still true, and the suggested drift-protection test (a reflection-path test for the optional-value/legacy rule, mirroring Reflection_CommandModel_Rejects_Unsupported_Flag_Type) still hasn't been added — only the flag-type rule has both a generated-path and reflection-path test.

If you want to actually close that gap rather than just test around it: since the source generator can't reference the runtime assembly (and vice versa), the two implementations can't share code directly today. A common pattern for this is a small linked shared-source file (e.g. src/ModularPipelines.Shared/CliPropertyShape.cs, added via <Compile Include> to both .SourceGenerator.csproj and the core ModularPipelines.csproj) that defines a tiny primitive shape (IsCliOptionValue, IsEnumerableOfCliOptionValue, IsString, IsEnumerableOfString, IsBool, IsInt, IsLegacyGeneratedOption) plus the pure IsSupportedFlagType/IsSupportedOptionalValueType predicates operating on that shape. Each side would only need its own thin glue (ITypeSymbol → shape, Type → shape); the actual "what's a valid flag/optional-value type" business rule would live in exactly one place instead of two, so a future rule change can't update one side and silently miss the other — the compiler enforces it rather than relying on a matching pair of hand-written tests.

Nothing blocking. This is a non-blocking maintainability suggestion, same as last time — happy to see this merge as-is if you'd rather track it separately.

[thomhurst]

Refreshed onto current main after the Ubuntu failure was isolated to the unrelated Node test host (exit 2; core suite completed). Validation on 9514ed4: CliAttributeTests 63/63 and ModularPipelines.slnx Release build 0 warnings/errors. A fresh detached worktree was used because the existing local branch diverged from the force-updated remote; that existing worktree was preserved untouched.

[claude]

Code review

Re-checked this PR: the head is now the merge commit 9514ed4 (merged main in for a clean CI baseline), but that merge only pulled in unrelated main changes (Directory.Packages.props, docs/docs/_sidebar.md, FileSystemModuleCache.cs/tests) — the actual diff for this PR's own files (CommandOptionsGenerator.cs, CommandArgumentBuilder.cs, CommandModelProvider.cs, PropertyCommandLinePart.cs, CliAttributeTests.cs) is byte-for-byte identical to commit b3309e6 that was already fully reviewed.

I independently traced through the eager-validation path again (CommandModelProvider.GetCommandModel → ValidateModel/ValidateProperty, and the generator's CreatePropertyMetadata/IsSupportedFlagType/IsSupportedOptionalValueType) rather than relying on the prior pass, and confirm:

• Validation genuinely runs once per Type (inside the ConditionalWeakTable factory), so this is fail-fast at first metadata request, not per-argument-build — matches the PR description.
• IsSupportedPropertyType is a real tri-state (true/false from validated code paths, null for hand-built PropertyCommandLineParts), and ValidateProperty's switch patterns only match on IsSupportedPropertyType: false, so hand-built parts used in existing tests correctly bypass the new eager check instead of breaking.
• The removed renderer-side GetSeparator(...) != " " throws in CommandArgumentBuilder.cs are subsumed exactly by the new Format != OptionFormat.SpaceSeparated checks — GetSeparator is a pure switch over Format where only SpaceSeparated maps to " ", so there's no behavioral gap.
• ArgumentPart intentionally never sets IsSupportedPropertyType/AllowsLegacyOptionalValues (left false/default) and the ValidateProperty switch only matches FlagPart/OptionPart, so arguments correctly skip the new type checks.

No correctness or security issues found.

Design read: moving the grouped-option/value-pair separator checks and the optional-value type check out of the renderer and into CommandModelProvider.ValidateModel (run once, cached) is a good fail-fast pattern — previously a misconfigured option class only threw when a value happened to be populated at runtime; now it throws unconditionally the first time the model is requested, which is a much better error-surfacing point for library consumers building their own CLI option classes.

Standing suggestion (non-blocking, unaddressed since the last review): IsSupportedOptionalValueType/IsLegacyGeneratedOption/IsSupportedFlagType are implemented twice — once over ITypeSymbol in CommandOptionsGenerator.cs (compile-time), once over System.Type in CommandModelProvider.cs (reflection fallback) — and this PR had to hand-edit both in lockstep to keep them in sync. That's a duplicated-business-rule smell: a future change to "what counts as a supported optional-value type" only needs to touch one of the two and both compile fine, but they'll silently diverge at runtime.

Since the source generator can't reference the runtime assembly (and vice versa), true code sharing isn't possible today, but the duplication surface can still be shrunk: express the rule as data instead of two parallel algorithms — e.g. a shared, linked source file (src/ModularPipelines.Shared/OptionShapeRules.cs, compiled into both ModularPipelines.SourceGenerator and ModularPipelines via <Compile Include>) that operates on a small common vocabulary (fully-qualified type name + "is enumerable-of" flag) which each side computes from its own ITypeSymbol/Type representation before calling into the shared rule. That turns "two implementations of one rule" into "two thin adapters over one rule," so future changes (e.g. adding a new supported optional-value type) touch exactly one file instead of two.

Short of that refactor, the test gap is easy to close now: the flag-type rule has both a generated-path test and a reflection-fallback test (Reflection_CommandModel_Rejects_Unsupported_Flag_Type, forced via the generic ReflectionInvalidFlagOptions<T>>), but the optional-value/legacy-generated-option rule only has generated-path tests (Parser_Rejects_Handwritten_Legacy_Optional_String_Value, Parser_Rejects_Unrelated_Generated_Legacy_Optional_String_Value). Adding a ReflectionInvalidOptionalValueOptions<T>-style reflection-path test mirroring the flag-type one would at least catch future drift between the two implementations via CI, even without eliminating the duplication itself.

CI is still running on the current head (9514ed4) at review time — no failures visible yet, just pending checks.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9514ed4ff7

ℹ️ 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]

Include option scope in positional validation

Separate from the existing same-phase multi-operand case, this also rejects valid global/command combinations: CommandLineBuilder.Build partitions IsGlobalOption parts and command-specific parts and renders them on opposite sides of the subcommand, so their positional indexes are independent. With this key, a [CliGlobalOptions] base that has a position-0 operand and a derived subcommand with its own position-0 operand now fails before rendering even though those arguments would not compete in the same argument list; include the global/command scope in the key or validate after partitioning.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in 7835bb9. Duplicate-position validation is now keyed by (IsGlobalOption, Phase, Position), matching the renderer partition, with a regression proving identical explicit positions in global and command scopes are accepted. CLI tests 66/66; core Release build 0 warnings/errors.

[thomhurst]

Review fixes pushed in 7835bb9278.

@claude review
@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7835bb9278

ℹ️ 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]

Do not split terminal positions by global scope

When a [CliGlobalOptions] base and the derived command both declare an explicit [CliArgument(0, Phase = CommandLinePhase.Terminal)], this key treats them as independent because IsGlobalOption differs, so GetCommandModel() accepts the duplicate. I checked CommandLineBuilder.Build: terminal parts are pulled out before the global/command partition and terminal arguments are rendered together from terminalCommandModel, so these operands compete in the same positional list and can be emitted in declaration order instead of being rejected like other explicit duplicates. Scope the key by global/command only for non-terminal phases, or validate after the same partitioning the renderer uses.

Useful? React with 👍 / 👎.

[claude]

Code review

Re-reviewed after the latest push (7835bb9278, "fix: preserve positional option scopes"). The diff since the last review (9514ed4) only touches CommandOptionsGenerator.cs, CommandModelProvider.cs, PropertyCommandLinePart.cs, and adds three tests to CliAttributeTests.cs — no other files changed.

What this commit does: adds HasExplicitPosition/HasExplicitArgumentPosition tracking (generator + reflection paths) and scopes the duplicate-argument-position check by IsGlobalOption in addition to (Phase, Position). This fixes two real gaps in the eager validation introduced earlier in this PR:

• Multiple [CliArgument] properties with no explicit position (all implicitly Position = 0 via the parameterless constructor) no longer falsely trip the "duplicate position" check — they're excluded from the uniqueness check and continue to rely on declaration order, matching pre-existing legacy behavior (verified via CommandModel_Preserves_Legacy_Default_Argument_Positions and CommandModel_Preserves_Shipped_Multi_Operand_Options).
• A [CliArgument(0)] on a [CliGlobalOptions]-scoped property and a [CliArgument(0)] on a command-specific property no longer falsely collide. I traced this through CommandLineBuilder.Build (src/ModularPipelines/Context/CommandLineBuilder.cs:66-79): global and command-specific parts are split into globalCommandModel/commandSpecificModel and rendered via two independent BuildArguments calls, so they never actually share a position slot at render time — the new IsGlobalOption key on the validation dictionary correctly mirrors that real separation rather than introducing a new rule.

I also double-checked that explicit-position duplicates are still caught: TestCliOptionsWithDuplicateArgumentPosition uses [CliArgument(0)] on both properties (explicit positions), and CommandModel_Rejects_Duplicate_Argument_Positions_In_Phase still passes.

Verified locally:

• CliAttributeTests: 66/66 passed (dotnet run via TUnit --treenode-filter), including the 3 new tests.
• Generator-side PropertyMetadata record gained one field (HasExplicitArgumentPosition); all 4 construction call sites (Secret, Argument, Flag, Option) were updated consistently, and only the Argument case's rendering path consumes it — confirmed no call-site/field-count mismatch.
• attribute.ConstructorArguments.Length > 0 (generator/Roslyn) and attribute.ConstructorArguments.Count > 0 (reflection/CustomAttributeData) compute HasExplicitPosition consistently for both [CliArgument] and [CliArgument(0)] forms.

No bugs or CLAUDE.md violations found in this push.

Still open (non-blocking, carried over from the prior review): IsSupportedOptionalValueType/IsLegacyGeneratedOption/IsSupportedFlagType remain implemented twice — once over ITypeSymbol in CommandOptionsGenerator.cs, once over System.Type in CommandModelProvider.cs — and this push again had to hand-edit both in lockstep (the attribute.ConstructorArguments.Length/Count > 0 check) to stay in sync. Not blocking for this PR, but the suggestion stands: a small linked shared-source file operating on a common primitive vocabulary (fully-qualified type name + "is enumerable-of" flag) would turn this into two thin adapters over one rule instead of two independently-maintained algorithms, so a future rule change can't update one side and silently miss the other.