refactor: Remove unnecessary type parameter from Failure and Skipped result types

[thomhurst]

Summary

Fixes #1876

• Created a non-generic ModuleResult base class containing common metadata and non-generic Failure and Skipped result types
• ModuleResult<T> now only contains Success<T> as its generic variant
• Added implicit conversions from ModuleResult.Failure and ModuleResult.Skipped to ModuleResult<T> for any T
• Updated pattern matching helpers (Match, Switch) to handle both direct and wrapper result types
• Updated JSON converters to properly serialize/deserialize the new type hierarchy

Rationale

The type parameter T in ModuleResult<T> was only actually used by the Success variant. The Failure and Skipped variants don't carry a typed value, yet they were forced to carry the type parameter. This meant ModuleResult<string>.Failure and ModuleResult<int>.Failure were different types even though they hold the same data (an Exception).

The new design:

• Eliminates unnecessary generic type instantiations for failure/skipped cases
• Provides a cleaner API where only success results need the type parameter
• Allows non-generic result creation in factory methods
• Maintains full backward compatibility through implicit conversions

Breaking Changes

None - this is a backward-compatible refactoring. Existing code patterns like ModuleResult<T>.Failure still work through the implicit conversion.

Test plan

• Solution builds successfully
• All unit tests pass (569/570 - 1 test fails due to worktree directory name, unrelated to changes)
• Pattern matching (Match, Switch) works correctly
• JSON serialization/deserialization works correctly
• ModuleResultType, IsFailure, IsSkipped properties work correctly for wrapper types

Generated with Claude Code

[thomhurst]

Summary

Refactors ModuleResult to use a non-generic base class for Failure and Skipped results, reducing unnecessary generic type instantiations.

Critical Issues

1. JSON Deserialization Truncation (Line 882+)

The diff shows the JSON deserialization code for ModuleResultJsonConverter<T>.Read is truncated with ... [59 lines truncated] .... The visible portion shows:

"Skipped" => skipDecision is not null
    ? new ModuleResult<T>.SkippedWrapper(new ModuleRe

The code is incomplete and won't compile. This needs to be verified in the actual PR.

2. Breaking Change in Pattern Matching

The PR changes the examples to show:

case ModuleResult.Failure { Exception: var ex }:  // NEW
case ModuleResult.Skipped { Decision: var skip }: // NEW

But existing code uses:

case ModuleResult<string>.Failure { Exception: var ex }:  // OLD
case ModuleResult<string>.Skipped { Decision: var skip }: // OLD

Impact: This is a breaking API change. All existing pattern matching code will need to be updated. The PR should:

• Document this as a breaking change
• Provide migration guidance
• Verify all internal usage is updated
• Consider if this warrants a major version bump

3. Required Members and Constructor Complexity

The new FailureWrapper and SkippedWrapper types use [SetsRequiredMembers] and manually copy all properties from the inner type:

internal FailureWrapper(Failure inner)
{
    _inner = inner;
    ModuleName = inner.ModuleName;
    ModuleDuration = inner.ModuleDuration;
    // ... etc
}

Issues:

• Fragile: Adding new required properties to ModuleResult requires updating both wrapper constructors
• Easy to miss properties during maintenance
• No compile-time guarantee that all properties are copied

Suggestion: Consider using the with expression pattern or reflection to copy properties automatically.

4. WithStatus Method Has Edge Case Bug

The WithStatus method in ModuleResultFactory.cs:

public static IModuleResult WithStatus(IModuleResult result, Status status)
{
    // Handle non-generic Failure/Skipped types directly
    if (result is ModuleResult.Failure failure)
    {
        return failure with { ModuleStatus = status };
    }

    if (result is ModuleResult.Skipped skipped)
    {
        return skipped with { ModuleStatus = status };
    }

    // Falls through to reflection-based code for generic types
}

Issue: If a ModuleResult<T>.FailureWrapper or ModuleResult<T>.SkippedWrapper is passed, the early returns won't trigger (they check for non-generic types), but the wrapper's inner value won't be updated properly. The reflection code operates on the wrapper, not the inner Failure/Skipped.

Result: The method will create a new wrapper with updated status, but the _inner field will still reference the old status. When accessing Exception or Decision, the inconsistent state could cause issues.

5. Performance Concern in IsFailure/IsSkipped

The properties now use pattern matching with multiple cases:

public bool IsFailure => this is Failure or IFailureResult;
public bool IsSkipped => this is Skipped or ISkippedResult;

This checks against 2 types per call. In hot paths (execution engine, result processing), this adds overhead compared to the single type check in the original code.

Suggestion: Profile to ensure this doesn't impact performance in result processing loops.

Suggestions

1. Consider Public API Stability

This refactoring changes public API surface area significantly. Consider:

• Mark old patterns as [Obsolete] first in one version
• Provide shim types for backward compatibility
• Only remove in a major version

2. Add XML Documentation

The new non-generic ModuleResult base class and wrapper types should have clear documentation explaining:

• When to use non-generic vs generic types
• How implicit conversions work
• Migration path from old patterns

3. Unit Test Coverage

Ensure tests cover:

• Implicit conversion from ModuleResult.Failure to ModuleResult<T>
• Pattern matching with both old and new syntaxes
• JSON serialization/deserialization of wrapper types
• WithStatus method with wrapper types

Verdict

⚠️ REQUEST CHANGES - Critical issues found:

1. Truncated/incomplete code in diff
2. Breaking API changes need documentation and migration guidance
3. Bug in WithStatus method with wrapper types
4. Fragile property copying in wrapper constructors

[thomhurst]

Summary

Refactors ModuleResult to use a non-generic base class for Failure and Skipped results, reducing unnecessary generic type instantiations.

Critical Issues

1. JSON Serialization Bug (ModuleResult.cs:880)

In the Write method of ModuleResultJsonConverter<T>, there's a copy-paste error:

var discriminator = value switch
{
    ModuleResult<T>.Success => "Success",
    ModuleResult<T>.FailureWrapper => "Failure",
    ModuleResult<T>.SkippedWrapper => "Failure",  // BUG: Should be "Skipped"
    ModuleResult.Failure => "Failure",
    ModuleResult.Skipped => "Skipped",
    _ => throw new JsonException("Unknown ModuleResult type")
};

The SkippedWrapper case returns "Failure" instead of "Skipped". This will cause incorrect JSON serialization where skipped results are serialized as failures.

Impact: This breaks JSON serialization/deserialization round-trips for skipped module results when using the generic ModuleResult<T> type.

Fix: Change line to ModuleResult<T>.SkippedWrapper => "Skipped",

Previous Review Status

From the previous review, the following issues have been addressed:

✅ Issue 1 (Truncated code): Resolved - code is complete and compiles
✅ Issue 4 (WithStatus bug): Partially addressed - non-generic types now handled correctly
⚠️ Issue 3 (Property copying fragility): Still present but acceptable for this refactor
⚠️ Issue 2 (Breaking changes): Documentation claims backward compatibility, appears correct due to implicit conversions, but should be verified with tests
⚠️ Issue 5 (Performance): Pattern matching changed, but impact likely negligible

However, a new critical bug (#1 above) was introduced in the JSON serialization code.

Verdict

⚠️ REQUEST CHANGES - Critical bug in JSON serialization that will cause skipped results to serialize incorrectly.

[thomhurst]

Fixed the JSON serialization bug - changed SkippedWrapper => "Failure" to SkippedWrapper => "Skipped" on line 881.

[thomhurst]

Summary

Refactors ModuleResult<T> to extract non-generic Failure and Skipped types into a base class, eliminating unnecessary generic type instantiations since these variants don't carry typed values.

Critical Issues

1. Incomplete Pattern Matching in WithStatus Method (ModuleResultFactory.cs:95)

The WithStatus method adds handling for non-generic Failure and Skipped types at the beginning, but the wrapper types (FailureWrapper and SkippedWrapper) won't match these direct type checks and will fall through to the reflection-based code path.

Looking at the added code:

// Handle non-generic Failure/Skipped types directly
if (result is ModuleResult.Failure failure)
{
    return failure with { ModuleStatus = status };
}

if (result is ModuleResult.Skipped skipped)
{
    return skipped with { ModuleStatus = status };
}
// ... reflection code continues

When a ModuleResult<T>.FailureWrapper or ModuleResult<T>.SkippedWrapper is passed in, it won't match these checks since the wrappers are distinct types that inherit from ModuleResult<T>, not from the non-generic ModuleResult.Failure/Skipped. This inconsistency could lead to performance issues (unnecessary reflection) or unexpected behavior.

Recommendation: Add explicit checks for wrapper types before falling back to reflection, or document why the reflection path is acceptable for wrappers.

2. Missing using Directive

The diff adds System.Diagnostics.CodeAnalysis usage (for [SetsRequiredMembers] attribute on line 1), but I don't see this import in the current file. Please verify this compiles correctly.

Suggestions

1. JSON Converter Factory Could Return Null

In ModuleResultJsonConverterFactory.CreateConverter, if the value type can't be determined, the method returns null which could silently fail. Consider throwing a more descriptive exception.

2. Documentation Example Needs Update

The XML documentation example (lines 8-27 in the new code) still shows:

case ModuleResult<string>.Failure { Exception: var ex }:

With the new design, this should be updated to:

case ModuleResult.Failure { Exception: var ex }:

This would better demonstrate the benefit of the refactoring and guide developers to use the non-generic types.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 (incomplete pattern matching in WithStatus) should be addressed to ensure consistent behavior across all result types.

[thomhurst]

Addressed the remaining review feedback:

Issue #1 (Incomplete Pattern Matching in WithStatus):
Added documentation explaining the design decision. The wrapper types (FailureWrapper and SkippedWrapper) intentionally fall through to the reflection path because:

1. They require the generic type parameter T to maintain type safety
2. The reflection path correctly handles with expressions on all ModuleResult<T> variants
3. The wrappers copy all metadata from _inner to their own properties on construction, so with { ModuleStatus = status } correctly updates the observable ModuleStatus
4. The _inner field is private and its internal state is not observable externally

The performance impact is minimal because:

• Non-generic Failure and Skipped types get the efficient direct path
• Wrappers are only created via implicit conversions and are less common in typical usage

Issue #2 (Missing using directive): Already addressed - System.Diagnostics.CodeAnalysis is imported at line 1.

Issue #3 (Documentation example): Already addressed - the example in ModuleResult<T> XML docs shows the non-generic pattern matching syntax (ModuleResult.Failure and ModuleResult.Skipped).

Commit: 9d513da

[thomhurst]

Summary

This PR refactors ModuleResult<T> to split it into a non-generic base class (ModuleResult) and a generic derived class, eliminating unnecessary type parameters from Failure and Skipped variants which don't carry typed values.

Critical Issues

None found ✅

Suggestions

1. Missing null-forgiving operator in JSON converter (line 846+)

The diff shows the JSON converter at line 846+ appears to be creating wrapper instances but the code is truncated. If this follows the pattern elsewhere, ensure proper null handling when deserializing the inner Failure/Skipped objects.

2. Performance consideration: Wrapper overhead

The new FailureWrapper and SkippedWrapper types copy all metadata properties from the inner object:

ModuleName = inner.ModuleName;
ModuleDuration = inner.ModuleDuration;
// ... etc

While this maintains immutability and makes the with expression work correctly (as noted in your comment at line 95+), it does add memory overhead. Since the comment explicitly addresses this design choice, this appears intentional. However, consider if the wrapper could just expose properties that delegate to _inner instead of copying them all.

3. Pattern exhaustiveness in ModuleResultType property

In ModuleResult.cs (new base class), the ModuleResultType switch expression at line ~106:

public ModuleResultType ModuleResultType => this switch
{
    ISuccessResult => ModuleResultType.Success,
    Failure => ModuleResultType.Failure,
    Skipped => ModuleResultType.Skipped,
    IFailureResult => ModuleResultType.Failure,
    ISkippedResult => ModuleResultType.Skipped,
    _ => throw new InvalidOperationException("Unknown result type")
};

The marker interfaces (IFailureResult, ISkippedResult) come after the concrete types. Consider reordering to check concrete types first for a minor performance benefit, though this is negligible.

4. Documentation: Example code needs updating

The XML documentation example at the top of ModuleResult<T> still shows:

case ModuleResult<string>.Failure { Exception: var ex }:

With the refactoring, users can now also write:

case ModuleResult.Failure { Exception: var ex }:

Consider adding a note about this in the example or documentation to highlight the cleaner non-generic pattern matching option.

5. [SetsRequiredMembers] attribute

The wrapper constructors use [SetsRequiredMembers] which is correct. Verify this is available in all target frameworks (it was added in .NET 7). If targeting earlier frameworks, you may need to polyfill this attribute.

Verdict

✅ APPROVE - No critical issues

This is a well-thought-out refactoring that improves the type system design without breaking changes. The implicit conversions maintain backward compatibility while providing a cleaner API for new code. The comments explain the design decisions clearly.