Refactor: Fix null-forgiving operator misuse in model classes

[thomhurst]

Problem

Several model classes misuse the null-forgiving operator (!) in ways that can hide bugs or cause runtime exceptions.

Affected Files:

• src/ModularPipelines/Models/ModuleResult.cs
• src/ModularPipelines/Context/LinuxInstaller.cs

Issue 1: Unnecessary null-forgiving on value types

Location: ModuleResult.cs (lines 128-130)

protected ModuleResult()
{
    ModuleName = null!;
    ModuleDuration = TimeSpan.Zero!;  // WRONG: TimeSpan is a struct, can't be null
    ModuleStart = DateTimeOffset.MinValue!;  // WRONG: DateTimeOffset is a struct
    ModuleEnd = DateTimeOffset.MinValue;  // Correct (no !)
    SkipDecision = null!;
    TypeDiscriminator = GetType().FullName!;
}

Problem: TimeSpan.Zero and DateTimeOffset.MinValue are value types - they can never be null. The ! operator is meaningless here and suggests misunderstanding of nullable reference types.

Issue 2: Passing null! to required parameter

Location: LinuxInstaller.cs (line 23)

public virtual async Task<CommandResult> InstallFromDpkg(DpkgInstallOptions options)
{
    await _aptGet.Install(new AptGetInstallOptions(null!)  // Passing null! as required param
    {
        FixBroken = true,
    }).ConfigureAwait(false);
}

Problem: The first parameter appears to be required but receives null!. This masks a potential NullReferenceException.

Issue 3: Null-forgiving in exception throwing

Location: ModuleResult.cs (line 52)

public T? Value
{
    get
    {
        if (ModuleResultType == ModuleResultType.Failure)
        {
            throw new ModuleFailedException(ModuleType!, Exception!);  // Both null!
        }
    }
}

Problem: If ModuleType or Exception are null when in Failure state, the exception is thrown with null values, losing context.

Why This Is Problematic

1. Hides bugs: null! tells compiler "trust me" but may crash at runtime
2. Confusing semantics: TimeSpan.Zero! suggests TimeSpan can be null (it can't)
3. Missing validation: Required parameters receiving null bypass compile-time checks
4. Poor error messages: Exceptions thrown with null values lose diagnostic info

Proposed Solutions

Fix 1: Remove unnecessary null-forgiving on structs

protected ModuleResult()
{
    ModuleName = null!;  // Keep - string is reference type
    ModuleDuration = TimeSpan.Zero;  // Remove ! - struct can't be null
    ModuleStart = DateTimeOffset.MinValue;  // Remove !
    ModuleEnd = DateTimeOffset.MinValue;
    SkipDecision = null!;  // Keep - reference type
    TypeDiscriminator = GetType().FullName!;  // Keep - could be null for anonymous types
}

Fix 2: Pass proper value or make parameter optional

// Option A: Pass empty array if that's the intent
await _aptGet.Install(new AptGetInstallOptions(Array.Empty<string>())
{
    FixBroken = true,
});

// Option B: Make parameter optional in AptGetInstallOptions
public record AptGetInstallOptions(string[]? Packages = null);

Fix 3: Add null checks before throwing

public T? Value
{
    get
    {
        if (ModuleResultType == ModuleResultType.Failure)
        {
            var moduleType = ModuleType ?? throw new InvalidOperationException("ModuleType not set for failed module");
            var exception = Exception ?? new InvalidOperationException("Exception not set for failed module");
            throw new ModuleFailedException(moduleType, exception);
        }
    }
}

Fix 4: Use [MemberNotNull] attribute

[MemberNotNull(nameof(ModuleType))]
[MemberNotNull(nameof(Exception))]
private void EnsureFailureState()
{
    if (ModuleResultType != ModuleResultType.Failure)
        throw new InvalidOperationException("Not in failure state");
    
    Debug.Assert(ModuleType != null);
    Debug.Assert(Exception != null);
}

Impact

• Medium severity correctness issue
• Could cause NullReferenceExceptions at runtime
• Improves code clarity and compiler trust

Files to Update

• src/ModularPipelines/Models/ModuleResult.cs
• src/ModularPipelines/Context/LinuxInstaller.cs
• Audit all uses of null! in the codebase