Render non-null CLI strings literally

[thomhurst]

Summary

• define null as the only absent CLI string value
• render empty and whitespace values literally for options, grouped values, value pairs, and positionals
• keep CliOptionValue.Bare distinct from an explicit empty optional value
• update public API documentation and regression coverage

Validation

• CliAttributeTests: 68/68 passed
• core Release build: 0 warnings, 0 errors
• targeted dotnet format passed
• current main has two stale RunReportTests initializers for removed PipelineOptions properties; they were excluded locally only to execute the focused tests and no workaround is committed

Closes #3780

[claude]

Code review

Reviewed the diff, the linked issue (#3780), and traced the full value-flow through CommandArgumentBuilder (including the legacy vs. new CliOptionValue paths for optional-arity options).

Summary: This is a clean, well-scoped fix. It does exactly what #3780 asked for — one consistent rule ("non-null renders literally, only null is rejected/omitted") applied uniformly across required options, grouped options, value pairs, and positionals, replacing the previous inconsistent IsNullOrWhiteSpace checks scattered across AddOption, AddGroupedOption, AddOptionValuePairs, and the CliOptionValue implicit conversion. Removing IsEmpty/the whitespace throw in favor of trusting the null check is also a nice simplification — less defensive code guarding against a case that's no longer a case.

Things I specifically checked and are handled correctly:

• AddArguments now only rejects a Required positional when values.Count == 0 (i.e. the backing value was null), not when it renders to ""/" " — matches the new doc comment on CliArgumentAttribute.Required.
• AddGroupedOption / AddOptionValuePairs correctly distinguish "null operand → throw CreateNullRequiredValueException" from "empty/whitespace operand → render literally," and the grouped-option null check is still reachable via the CliValuePair-sourced path even though plain string collections never surface nulls (those get filtered earlier in GetCollectionValues).
• The legacy-generated-option compatibility shim (ToLegacyOptionalValue, gated on GeneratedCodeAttribute.Tool == "ModularPipelines.OptionsGenerator") deliberately keeps mapping empty/whitespace strings to CliOptionValue.Bare for old generated packages, while the new CliOptionValue-typed path renders them literally. That split is intentional and is exercised by Parser_Preserves_Legacy_Multiple_Optional_String_Values vs. Parser_Groups_Literal_Optional_Values, so it's not a regression — just flagging it because the new XML doc on CliOptionAttribute.ValueArity ("Non-null string values are rendered literally...") doesn't call out that this doesn't apply to the legacy shim. Worth a one-line doc note if a future reader hits it, but not blocking.
• Test coverage is thorough: empty, whitespace, null, grouped, pairs, and the legacy-compat path are all covered, including the negative cases (Parser_Rejects_Null_Value_Pair_Operands).

No bugs or CLAUDE.md violations found. No architectural concerns beyond the minor doc-completeness note above — the change actually reduces an anti-pattern (ad hoc validation duplicated across four call sites) rather than introducing one.

[thomhurst]

CI diagnosis: Ubuntu, Windows, and macOS all fail only on the current-main RunReportTests initializer referencing removed PipelineOptions.PrintLogo / PrintResults properties. This PR does not touch that file. #3844 contains the PipelineOptions.Console correction and now has a fresh full workflow running, so rerunning #3861 before that merges would deterministically reproduce the same baseline failure. No rerun triggered.

[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: Didn't find any major issues. Keep it up!

Reviewed commit: 4432e13c23

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

Re-reviewed the diff plus the linked issue (#3780) and traced the value-flow through CommandArgumentBuilder. A prior review from claude[bot] already covered the core design (one consistent "null is the only absent value" rule applied uniformly across options/grouped options/pairs/positionals) and flagged the legacy-shim doc gap — that analysis holds up and I won't repeat it. One additional, previously-unflagged issue and one architectural suggestion:

Misleading exception message for empty (non-null) required argument collections

src/ModularPipelines/Helpers/Internal/CommandArgumentBuilder.cs:162-167

if (argumentPart.Attribute.Required && values.Count == 0)
{
    throw new ArgumentException(
        $"Required CLI argument '{optionsType.Name}.{argumentPart.PropertyName}' cannot be null.",
        argumentPart.PropertyName);
}

Before this PR the message was "cannot be null or empty", which was accurate for both causes of values.Count == 0. Now it unconditionally says "cannot be null", but that branch is also reached when the backing property is a non-null, empty collection — confirmed by the existing test Parser_Rejects_Empty_Required_CliArgument_Collection (TestCliOptionsWithRequiredArgumentCollection { Files = [] }, line ~690), which still passes because it only asserts the message contains the property name, not the specific wording.

For a Required scalar argument, values.Count == 0 really does mean the value was null, so the message is fine there. But for a Required collection argument (IEnumerable<string>), Files = [] is a perfectly non-null value that still hits this branch — telling the caller "cannot be null" when they passed a non-null empty array will send them looking in the wrong place. Suggest restoring the null-or-empty framing for this specific message (e.g. "cannot be null or empty"), since the single check now covers two distinct causes with one message that's only accurate for one of them.

Suggestion: validate CliValuePair nullness at construction, not at render time

src/ModularPipelines/Models/CliValuePair.cs:10 / CommandArgumentBuilder.cs:544-547

public record CliValuePair(string First, string Second);
...
if (pair.First is null || pair.Second is null)
{
    throw CreateNullRequiredValueException(optionsType, optionPart);
}

First/Second are declared as non-nullable string, but the only way the runtime null-check ever fires is because callers can still pass null! (as the new Parser_Rejects_Null_Value_Pair_Operands test does) or construct one from a nullable-oblivious context. Right now that contract violation isn't caught until the object is deep inside CommandArgumentBuilder's rendering pipeline — possibly far away in both code and time from where the bad CliValuePair was actually built (e.g. constructed in a module, stored, and only rendered when the pipeline later executes that command). Adding ArgumentNullException.ThrowIfNull(First)/(Second) in the record's primary constructor would fail fast at the actual mistake site, produce a more specific exception (ArgumentNullException vs. a generic InvalidOperationException keyed off optionPart), and let the non-nullable annotation on First/Second actually be a real, enforced guarantee rather than just a compile-time hint — which in turn would let AddGroupedOption/AddOptionValuePairs drop their defensive null checks entirely, since a validly-constructed CliValuePair could never contain one. This is a small change but it fits the PR's own goal of moving validation to one clear, consistent place instead of scattering it through the builder.

Neither point blocks this PR — the collection-message issue is a wording regression worth a quick follow-up, and the CliValuePair point is a structural nice-to-have for a future pass.

[thomhurst]

Refreshed onto current main after Ubuntu's core test host ran 27m18s and exited without a failing-test summary; all other jobs passed. Validation on 55353a7: CliAttributeTests 68/68 and ModularPipelines.slnx Release build 0 warnings/errors. A detached worktree preserved the existing divergent local branch untouched.

[claude]

Code Review

I reviewed the diff, rebuilt the core solution (ModularPipelines.slnx, 0 warnings/0 errors), and re-ran CliAttributeTests (68/68 passed), confirming the PR's stated validation.

Summary

Good, well-scoped fix: it makes null the single "absent" sentinel for CLI string values and renders every other string (including ""/whitespace) literally, matching the doc-comment updates on CliArgumentAttribute.Required / CliOptionAttribute.ValueArity. Test coverage for the new literal-rendering paths (grouped options, value pairs, optional values, required arguments) looks thorough and each new/renamed test matches the behavior it exercises.

Findings

1. CliValuePair's non-nullable properties don't match the runtime contract (src/ModularPipelines/Models/CliValuePair.cs:10, used in CommandArgumentBuilder.cs:544 and :509)

The record is declared public record CliValuePair(string First, string Second) — both properties are non-nullable per NRT annotations. Yet AddOptionValuePairs/AddGroupedOption explicitly check pair.First is null || pair.Second is null and throw a dedicated InvalidOperationException ("cannot contain null values"), and the new test Parser_Rejects_Null_Value_Pair_Operands only reaches that branch via null! forgiveness. That's a real mismatch: the type tells the compiler (and IDE nullable warnings) that null can never happen, while the implementation defends against exactly that at runtime. Anyone constructing a CliValuePair from a genuinely nullable source (e.g. a lookup, an external API result) gets no compile-time signal and only finds out via a pipeline failure at build-argument time.

Making these string?, string? would let the compiler/analyzer flag the misuse at the call site (consistent with how CliOptionValue.Value is already modeled as string?), and the existing runtime check would then simply be defense-in-depth against a state the type system also acknowledges is possible — rather than the type asserting a guarantee the code doesn't actually trust.

2. ValidateOptionalValue's null check is now unreachable dead code (CommandArgumentBuilder.cs:462-473)

CliOptionValue is a sealed record with a private constructor; the only ways to obtain a non-null instance are CliOptionValue.Bare (Value == null, IsBare == true) or the implicit string? conversion, which returns null itself for a null input and otherwise always sets a non-null Value with IsBare == false. So the invariant "!IsBare implies Value != null" holds for every instance reachable through the public API, including through the legacy-compat path (ToLegacyOptionalValue, which also only returns Bare or a wrapped non-null value).

Both call sites of ValidateOptionalValue (:382, :458) only invoke it on values where !IsBare already holds, so optionValue.Value is null can never be true there anymore — confirmed by there being no test left that exercises this exception message post-refactor (there was one pre-PR, testing the old IsNullOrWhiteSpace check, which this replaced). This isn't harmful, but it's misleading: a reader sees a guarded exception path and reasonably assumes it's reachable, when actually the type system already rules it out. Either delete the check (and let the Value! null-forgiving operator carry the real guarantee), or turn it into a Debug.Assert/comment documenting the invariant instead of a public-facing InvalidOperationException that can't fire.

3. Minor: Required-argument exception message narrower than its trigger condition (CommandArgumentBuilder.cs:162-167)

argumentPart.Attribute.Required && values.Count == 0 now throws "... cannot be null.", but values.Count == 0 is also true for a non-null, genuinely empty collection-valued argument (unchanged from the prior behavior, which had the same trigger condition via IsEmpty). The message is slightly misleading for that case since the property isn't actually null. Low priority, but worth a wording tweak (e.g. "cannot be null or empty") if you want the error to stay accurate for both scalar and collection arguments.

Nice-to-have

None of the above block merging — findings 1 and 2 are about keeping the type system honest with the implementation rather than functional bugs, and 3 is a message-wording nit. Everything I could exercise locally (core build, focused test run) passed cleanly.