fix: Remove null-forgiving operator misuse in model classes

[thomhurst]

Summary

• Added parameterless constructor to AptGetInstallOptions for commands like apt-get install --fix-broken that don't require a package name
• Made AptGetInstallOptions.Package nullable to properly represent the optional state
• Replaced null! parameter in LinuxInstaller with parameterless constructor usage
• Added proper null validation in ModuleResultJsonConverter.Read() for moduleName
• Replaced null-forgiving operators with null-coalescing operators that provide sensible defaults for exception and skipDecision during JSON deserialization

Fixes #1921

Test plan

• Build succeeds with dotnet build ModularPipelines.sln -c Release
• Verify AptGetInstallOptions works correctly with and without package parameter
• Verify ModuleResultJsonConverter properly handles malformed JSON with missing properties

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR removes misuse of null-forgiving operators by adding a parameterless constructor to AptGetInstallOptions, making the Package property nullable, and improving null handling in ModuleResultJsonConverter.

Critical Issues

1. Breaking Change Risk: Making Package nullable may break existing callers

The change makes AptGetInstallOptions.Package nullable (line 18 in the new version), but the calling code in AptGet.cs:51-54 still requires a non-nullable AptGetInstallOptions options parameter. This creates an inconsistency:

• If someone creates new AptGetInstallOptions() with no package, then sets other properties like FixBroken, the Package property will be null
• When this gets serialized to a CLI command via [CliArgument(Placement = ArgumentPlacement.AfterOptions)], it may produce unexpected command-line output

Location: src/ModularPipelines/Options/Linux/AptGet/AptGetInstallOptions.cs:32

Recommendation: Verify that the CLI argument serialization properly handles null values. The [CliArgument] attribute should skip null values, otherwise apt-get install --fix-broken <null> could be generated. Please check the CliArgument implementation to confirm null values are properly excluded from command generation.

2. Potential JSON deserialization issue with default values

In ModuleResultJsonConverter.Read() at lines 461 and 468 of the modified file:

"Failure" => new ModuleResult<T>.Failure(exception ?? new Exception("Deserialized failure with no exception details"))
"Skipped" => new ModuleResult<T>.Skipped(skipDecision ?? SkipDecision.Skip("Deserialized skip with no decision details"))

Issue: Creating fallback objects when deserializing malformed JSON could mask data corruption issues. The original code would fail fast with a NullReferenceException, which is arguably better for detecting corrupted serialization data.

Recommendation: Consider throwing a JsonException instead of creating fallback objects:

"Failure" => exception != null 
    ? new ModuleResult<T>.Failure(exception) { ... }
    : throw new JsonException("Failure result requires an Exception property"),
"Skipped" => skipDecision != null
    ? new ModuleResult<T>.Skipped(skipDecision) { ... }
    : throw new JsonException("Skipped result requires a Decision property"),

This maintains fail-fast behavior while being explicit about what went wrong.

Suggestions

1. Add validation for Package property

Since Package can now be null, consider adding validation to ensure that either:

• A package name is provided, OR
• A flag like FixBroken is set that makes the package optional

This could be done with a custom validation method or in the command builder.

2. Consider adding XML documentation example for parameterless constructor usage

The new parameterless constructor has excellent documentation, but it would be helpful to show a concrete example of apt-get install --fix-broken usage in the XML docs.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 must be verified (CLI argument serialization handling nulls correctly), and critical issue #2 should be reconsidered for fail-fast behavior.

[thomhurst]

Review feedback addressed

Critical Issue #1 - CLI argument serialization handling nulls

✅ Already handled correctly - Verified that CommandArgumentBuilder.AddArguments() (lines 55-59) already skips null values:

var rawValue = argumentPart.Property.GetValue(optionsObject);
if (rawValue is null)
{
    continue;
}

So apt-get install --fix-broken with a null Package will correctly omit the package argument.

Critical Issue #2 - Fail-fast JSON deserialization

✅ Fixed in commit f2065a6 - Changed from fallback objects to throwing JsonException:

// Before (masks data corruption)
"Failure" => new ModuleResult<T>.Failure(exception ?? new Exception("Deserialized failure with no exception details"))

// After (fail-fast)
"Failure" => exception is not null
    ? new ModuleResult<T>.Failure(exception) { ... }
    : throw new JsonException("Failure result requires an Exception property in the JSON."),

The same pattern applied for Skipped results.

[thomhurst]

Summary

Removes null-forgiving operator misuse by adding a parameterless constructor to AptGetInstallOptions and improving null handling in ModuleResultJsonConverter.

Critical Issues

1. Incomplete validation in ModuleResultJsonConverter (src/ModularPipelines/Models/ModuleResult.cs:450-485)

The PR adds validation for moduleName (line 450-453) and for exception/skipDecision (lines 459-477), but it's inconsistent:

• ✅ Good: moduleName is validated and throws if null
• ✅ Good: exception and skipDecision are validated within their respective branches
• ❌ Problem: No validation for value in the Success case (line 452)

If malformed JSON has "": "Success" but missing or null Value, line 452 will use value! where value is still default(T), which could be null for reference types. This silently creates invalid Success results.

Fix needed: Add validation similar to the Failure/Skipped branches:

"Success" => value is not null
    ? new ModuleResult<T>.Success(value)
    {
        ModuleName = moduleName,
        // ...
    }
    : throw new JsonException("Success result requires a Value property in the JSON."),

Note: This may need special handling for T being a nullable reference type or value type, but at minimum should validate when T is a non-nullable reference type.

Suggestions

1. Consider making Install method signature consistent (src/ModularPipelines/Context/Linux/AptGet.cs:51)

The Install method takes non-nullable AptGetInstallOptions options (line 51), but now that the options class supports parameterless construction with null Package, callers might expect to pass null like other methods (e.g., Autoclean on line 18).

Consider either:

• Change to AptGetInstallOptions? options = default for consistency with other methods
• Or document why Install requires explicit options object

2. Documentation clarity for Package property (src/ModularPipelines/Options/Linux/AptGet/AptGetInstallOptions.cs:32)

The new string? Package property is well-documented with XML comments explaining when it's null. However, the runtime behavior when Package is null depends on how the CLI argument serialization handles it. Consider adding a code example or note about the --fix-broken use case to the property's XML comment itself.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 must be fixed (incomplete validation for Success.Value)

[thomhurst]

Critical Issue #1 Addressed - Success.Value validation

✅ Fixed in commit 49f710b - Added validation for the Success case:

// Before (could silently create invalid Success with null value)
"Success" => new ModuleResult<T>.Success(value!)

// After (fail-fast validation)
"Success" => value is not null
    ? new ModuleResult<T>.Success(value)
    { ... }
    : throw new JsonException("Success result requires a Value property in the JSON."),

This removes the last null-forgiving operator (value!) and adds consistent fail-fast validation across all three result types (Success, Failure, Skipped).

Note on value types: For value types like int, the JSON deserializer will produce default(T) (e.g., 0) rather than null, so the validation will pass. This is the expected behavior since we can't distinguish "missing" from "zero" for value types in JSON.

[thomhurst]

Summary

This PR removes null-forgiving operator misuse in model classes, adding proper null validation and parameterless constructor support for AptGetInstallOptions.

Critical Issues

None found ✅

The PR correctly addresses all the issues mentioned in #1921:

1. AptGetInstallOptions - Properly fixed with:

   • Added parameterless constructor for commands like apt-get install --fix-broken
   • Made Package property nullable (string?)
   • LinuxInstaller.cs now uses parameterless constructor instead of null!
   • Good documentation explaining when to use each constructor

2. ModuleResultJsonConverter - Improved validation:

   • Added null check for moduleName with clear exception message
   • Replaced null! with proper null validation for Success/Failure/Skipped cases
   • Uses ternary expressions with throw new JsonException for better error messages
   • Each result type now validates its required property exists before construction

Suggestions

1. Consider extracting magic strings (minor): The discriminator strings ("Success", "Failure", "Skipped") appear in both Read and Write methods. Consider using constants to avoid typos:

private const string SuccessDiscriminator = "Success";
private const string FailureDiscriminator = "Failure";
private const string SkippedDiscriminator = "Skipped";

2. JSON error messages could be more specific: The exception messages are good, but could mention the discriminator type for easier debugging:

"Success result requires a Value property in the JSON."
// Could be:
"Success result (discriminator='Success') requires a Value property in the JSON."

These are very minor and don't block approval.

Verdict

✅ APPROVE - No critical issues

The PR successfully addresses all the null-forgiving operator misuses identified in #1921. The changes improve type safety and provide better error messages for JSON deserialization failures. The code is well-documented and follows good practices.