feat: Implement discriminated union pattern for ModuleResult<T>

[thomhurst]

Summary

Implements issue #1869 - Discriminated Union Result Pattern for ModuleResult<T>.

BREAKING CHANGE: ModuleResult<T>.Value no longer throws exceptions. Use pattern matching or IsSuccess/ValueOrDefault instead.

Changes

• Core Discriminated Union: Rewrote ModuleResult<T> as an abstract record with three sealed variants:

  • Success(T Value) - successful execution with a value
  • Failure(Exception Exception) - failed execution with an exception
  • Skipped(SkipDecision Decision) - skipped execution with reason

• Safe Accessors (no exceptions):

  • ValueOrDefault - returns value or default(T)
  • ExceptionOrDefault - returns exception or null
  • SkipDecisionOrDefault - returns skip decision or null

• Quick Checks:

  • IsSuccess, IsFailure, IsSkipped boolean properties

• Pattern Matching Helpers:

  • Match<TResult>(onSuccess, onFailure, onSkipped) - functional matching
  • Switch(onSuccess, onFailure, onSkipped) - imperative matching

• JSON Serialization: Full support via custom ModuleResultJsonConverterFactory

• Deprecated: ModuleFailedException and ModuleSkippedException (use pattern matching instead)

• Deleted: SkippedModuleResult.cs and TimedOutModuleResult.cs (replaced by variants)

Migration Example

// Before (throws on failure/skip):
var value = result.Value;

// After (pattern matching - recommended):
switch (result)
{
    case ModuleResult<string>.Success { Value: var value }:
        Console.WriteLine($"Got: {value}");
        break;
    case ModuleResult<string>.Failure { Exception: var ex }:
        Console.WriteLine($"Failed: {ex.Message}");
        break;
    case ModuleResult<string>.Skipped { Decision: var skip }:
        Console.WriteLine($"Skipped: {skip.Reason}");
        break;
}

// Or use safe accessors:
if (result.IsSuccess)
{
    var value = result.ValueOrDefault;
}

Test plan

• All unit tests pass (486 passing, matching baseline)
• Full solution builds with 0 errors
• JSON serialization/deserialization works correctly
• History repository integration works with UsedHistory status
• Pattern matching works for all three variants

Closes #1869

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR implements a discriminated union pattern for ModuleResult, replacing the exception-throwing .Value property with type-safe pattern matching via Success/Failure/Skipped variants.

Critical Issues

1. Missing .Value property - Breaking Change Documentation

The old ModuleResult<T>.Value property is completely removed, but the plan documents state it should be removed. However, I cannot find explicit guidance in the PR description or commit messages about:

• Whether this is intended as a major version bump
• Migration guide for existing consumers
• Compatibility shims if any

Recommendation: Verify this is intentional for a major version release. If consumers exist outside this repo, consider adding a migration guide.

---

2. JSON Converter Implementation - Exception Handling

In the ModuleResultJsonConverter<T> class, I see custom exception serialization via ExceptionJsonConverter. The implementation should be verified for:

• Security: Exception messages may contain sensitive data (file paths, connection strings, etc.)
• Completeness: Stack traces and inner exceptions should be preserved
• Deserialization: Can exceptions be safely deserialized, or should they be converted to a safe DTO?

Location: src/ModularPipelines/Models/ModuleResult.cs (the JSON converter section)

Recommendation: Review the ExceptionJsonConverter implementation to ensure it doesn't leak sensitive information and handles deserialization safely.

---

3. Factory Method Timing Logic - Potential Bug

In the factory methods (CreateSuccess, CreateFailure, CreateSkipped), there's defensive timing logic:

ModuleDuration = (ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime) -
                (ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime)

Issue: If both StartTime and EndTime are MinValue, this calculates duration as 0, which might mask timing issues. If only one is MinValue, the calculation uses DateTimeOffset.Now inconsistently.

Location: src/ModularPipelines/Models/ModuleResult.cs:287-289, :299-301, :311-313

Recommendation: Either throw an exception if timing data is invalid, or use a consistent fallback (e.g., always use Now for both if either is MinValue).

---

Suggestions

1. Pattern Matching Exhaustiveness

The Match and Switch methods use _ => throw new InvalidOperationException("Unknown result type") as a default case. Since the type is sealed and only three variants exist, this should never execute.

Suggestion: Consider removing the default case to get compiler exhaustiveness checking, or add a comment explaining why it's needed (e.g., for future-proofing).

---

2. Nullability on Success.Value

The Success record allows Success(T Value) where T can be nullable. However, ValueOrDefault returns T?, which would be T?? for nullable T.

Example:

ModuleResult<string?>.Success(null) // Valid, but confusing

Suggestion: Consider documenting expected behavior when T itself is nullable, or constrain T to be non-nullable in Success if that's the intent.

---

3. Obsolete Attribute Message

The obsolete messages on ModuleFailedException and ModuleSkippedException say "no longer thrown by ModuleResult.Value" but the .Value property itself is removed.

Suggestion: Update the message to something like: "Use pattern matching on ModuleResult instead. The .Value property has been removed in favor of .ValueOrDefault or pattern matching."

---

Previous Review Status

Cannot access previous comments due to GitHub token scope limitations.

---

Verdict

⚠️ REQUEST CHANGES

Critical issues #1, #2, and #3 should be addressed:

1. Confirm this is a major version bump and add migration guidance
2. Verify exception JSON serialization doesn't leak sensitive data
3. Fix or document the timing fallback logic in factory methods

The overall design is solid and follows C# discriminated union best practices. The changes are well-structured and comprehensive.

[Copilot]

Pull request overview

This PR implements a discriminated union pattern for ModuleResult<T>, replacing the exception-throwing Value property with three explicit sealed variants: Success, Failure, and Skipped.

Key changes:

• BREAKING: ModuleResult<T> is now an abstract record with three sealed record variants instead of a class with subclasses
• Replaced throwing Value property with safe ValueOrDefault, ExceptionOrDefault, and SkipDecisionOrDefault accessors
• Added pattern matching helpers (Match and Switch methods) and boolean checks (IsSuccess, IsFailure, IsSkipped)
• Implemented custom JSON serialization via ModuleResultJsonConverterFactory instead of System.Text.Json polymorphic attributes
• Deprecated (not removed) ModuleFailedException and ModuleSkippedException with [Obsolete] attributes
• Updated all consuming code to use the new safe accessors

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/ModularPipelines/Models/ModuleResult.cs	Complete rewrite as discriminated union with custom JSON converters
src/ModularPipelines/Models/IModuleResult.cs	Added safe accessor properties for non-throwing access
src/ModularPipelines/Models/SkippedModuleResult.cs	Deleted - replaced by Skipped variant
src/ModularPipelines/Models/TimedOutModuleResult.cs	Deleted - timeouts now use Failure variant
src/ModularPipelines/Models/PipelineSummary.cs	Updated to use CreateFailure factory method
src/ModularPipelines/Engine/ModuleExecutionPipeline.cs	Updated to use factory methods and record with expressions
src/ModularPipelines/Engine/Execution/ModuleResultFactory.cs	Simplified to delegate to ModuleResult<T> factory methods, added WithStatus helper
src/ModularPipelines/Engine/Executors/IgnoredModuleResultRegistrar.cs	Updated to use WithStatus for immutable status updates
src/ModularPipelines/Engine/Execution/ModuleLifecycleEventInvoker.cs	Updated to use IModuleResult interface
src/ModularPipelines/Engine/Attributes/IAttributeEventInvoker.cs	Changed parameter from ModuleResult to IModuleResult
src/ModularPipelines/Engine/Attributes/AttributeEventInvoker.cs	Updated parameter type to IModuleResult
src/ModularPipelines/Attributes/Events/IModuleEndEventReceiver.cs	Changed parameter from ModuleResult to IModuleResult
src/ModularPipelines/Exceptions/ModuleFailedException.cs	Added [Obsolete] attribute
src/ModularPipelines/Exceptions/ModuleSkippedException.cs	Added [Obsolete] attribute
src/ModularPipelines.GitHub/GitHubMarkdownSummaryGenerator.cs	Updated to use ExceptionOrDefault
src/ModularPipelines.Build/Modules/*.cs	Updated all build modules to use ValueOrDefault
test/ModularPipelines.UnitTests/*.cs	Updated tests to use safe accessors
test/ModularPipelines.TestHelpers/Assertions/ModuleResultAssertions.cs	Updated assertions to use safe accessors
test/ModularPipelines.Azure.UnitTests/AzureCommandTests.cs	Updated to use ValueOrDefault
docs/plans/2026-01-07-module-result-discriminated-union.md	Design document for the discriminated union pattern
docs/plans/2026-01-07-module-result-discriminated-union-implementation.md	Implementation plan document

[Copilot]

The StackTrace property is serialized but never restored when deserializing exceptions. The deserialized exception will not have the original stack trace, which could make debugging difficult. Consider using reflection to set the stack trace field if preserving it is important, or document this limitation.

[Copilot]

Documentation inaccuracy. The breaking changes section states that ModuleFailedException and ModuleSkippedException classes are "Removed", but they are actually deprecated with [Obsolete] attributes, not deleted. Update the documentation to reflect that these classes are deprecated rather than removed to provide a more accurate migration path.

[Copilot]

Potential null reference exception. SkipDecisionOrDefault returns SkipDecision? (nullable), so accessing .ShouldSkip directly will throw a NullReferenceException if the result is not skipped. Use null-conditional operator or check for null first.

Suggested change

	if (changedFiles.SkipDecisionOrDefault.ShouldSkip)
	if (changedFiles.SkipDecisionOrDefault?.ShouldSkip == true)

[Copilot]

The duration calculation logic is duplicated across all three factory methods (CreateSuccess, CreateFailure, CreateSkipped). Consider extracting this into a private helper method to reduce code duplication and make maintenance easier.

Suggested change

	internal static Success CreateSuccess(T value, ModuleExecutionContext ctx) => new(value)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = (ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime) -
	(ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime),
	ModuleStart = ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime,
	ModuleEnd = ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	internal static Failure CreateFailure(Exception exception, ModuleExecutionContext ctx) => new(exception)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = (ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime) -
	(ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime),
	ModuleStart = ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime,
	ModuleEnd = ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	internal static Skipped CreateSkipped(SkipDecision decision, ModuleExecutionContext ctx) => new(decision)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = (ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime) -
	(ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime),
	ModuleStart = ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime,
	ModuleEnd = ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	private static (DateTimeOffset ModuleStart, DateTimeOffset ModuleEnd, TimeSpan ModuleDuration) GetModuleTiming(ModuleExecutionContext ctx)
	{
	var moduleStart = ctx.StartTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.StartTime;
	var moduleEnd = ctx.EndTime == DateTimeOffset.MinValue ? DateTimeOffset.Now : ctx.EndTime;
	var moduleDuration = moduleEnd - moduleStart;
	return (moduleStart, moduleEnd, moduleDuration);
	}
	internal static Success CreateSuccess(T value, ModuleExecutionContext ctx)
	{
	var timing = GetModuleTiming(ctx);
	return new(value)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = timing.ModuleDuration,
	ModuleStart = timing.ModuleStart,
	ModuleEnd = timing.ModuleEnd,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	}
	internal static Failure CreateFailure(Exception exception, ModuleExecutionContext ctx)
	{
	var timing = GetModuleTiming(ctx);
	return new(exception)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = timing.ModuleDuration,
	ModuleStart = timing.ModuleStart,
	ModuleEnd = timing.ModuleEnd,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	}
	internal static Skipped CreateSkipped(SkipDecision decision, ModuleExecutionContext ctx)
	{
	var timing = GetModuleTiming(ctx);
	return new(decision)
	{
	ModuleName = ctx.ModuleType.Name,
	ModuleDuration = timing.ModuleDuration,
	ModuleStart = timing.ModuleStart,
	ModuleEnd = timing.ModuleEnd,
	ModuleStatus = ctx.Status,
	ModuleType = ctx.ModuleType
	};
	}

[Copilot]

The JSON deserialization uses null-forgiving operators (!) on values that might actually be null if the JSON is malformed (e.g., missing required fields like "ModuleName", "Value" for Success, "Exception" for Failure, or "Decision" for Skipped). This could lead to NullReferenceExceptions at runtime. Consider validating that required fields are present before creating the result instances.

[Copilot]

Invalid property pattern syntax. The pattern { Decision.Reason: var reason } is not valid C# syntax. To access nested properties in pattern matching, use { Decision: { Reason: var reason } } or { Decision.Reason: var reason } only works if Decision is the declaring type. The correct syntax should be case ModuleResult<string>.Skipped { Decision: var skip }: followed by accessing skip.Reason.

Suggested change

	case ModuleResult<string>.Skipped { Decision.Reason: var reason }:
	case ModuleResult<string>.Skipped { Decision: { Reason: var reason } }:

[Copilot]

Documentation-code discrepancy. The design document states that JSON serialization uses .NET 7+ polymorphic serialization with [JsonDerivedType] attributes, but the actual implementation uses a custom ModuleResultJsonConverterFactory and manual converter. Update the documentation to reflect the actual implementation approach, or explain why the custom converter was chosen instead.

[Copilot]

Documentation-code discrepancy. The implementation plan shows using [JsonDerivedType] attributes for polymorphic serialization, but the actual implementation uses a custom ModuleResultJsonConverterFactory. This inconsistency between the plan and implementation should be addressed - either update the plan to reflect the custom converter approach, or explain why it was necessary to deviate from the planned approach.

Suggested change

	[JsonDerivedType(typeof(Success), "Success")]
	[JsonDerivedType(typeof(Failure), "Failure")]
	[JsonDerivedType(typeof(Skipped), "Skipped")]
	/// NOTE: In the concrete implementation, polymorphic serialization for
	/// ModuleResult<T> is handled by a custom ModuleResultJsonConverterFactory
	/// registered in JsonSerializerOptions, rather than JsonDerivedType attributes.

[thomhurst]

Summary

This PR adds design documentation for a discriminated union refactoring and updates all code usages from .Value to .ValueOrDefault.

Critical Issues

BLOCKING: This PR will not compile ❌

The PR changes all usages of ModuleResult<T>.Value to ModuleResult<T>.ValueOrDefault throughout the codebase (e.g., in src/ModularPipelines.Build/Modules/CreateReleaseModule.cs:60,62), but the ValueOrDefault property does not exist in the current ModuleResult implementation.

The current ModuleResult<T> class (src/ModularPipelines/Models/ModuleResult.cs) only has:

• Value property (lines 46-67) - throws exceptions on failure/skipped
• HasValue property (line 72)

The PR adds design documents (docs/plans/2026-01-07-module-result-discriminated-union.md and docs/plans/2026-01-07-module-result-discriminated-union-implementation.md) that describe a ValueOrDefault property, but the actual implementation is missing.

Required fix: Either:

1. Only merge the design documents in this PR, and defer the code changes to the implementation PR
2. Include the complete discriminated union implementation from Task 1-2 of the implementation plan (updating IModuleResult and ModuleResult classes to add the ValueOrDefault property)

The implementation plan itself is comprehensive and well-structured, but this PR appears to be incomplete - it has the documentation and the usage updates, but not the core model changes.

Suggestions

None - fix the critical issue first.

Previous Review Status

Cannot access previous comments due to token scope limitations.

Verdict

⚠️ REQUEST CHANGES - Critical compilation issue: ValueOrDefault property does not exist

[thomhurst]

Addressed Review Comments

Pushed commit 21bb7c3 to address the review feedback:

Fixed Issues:

1. Null reference in PackProjectsModule.cs (line 41)

   • Changed changedFiles.SkipDecisionOrDefault.ShouldSkip to use null-conditional: changedFiles.SkipDecisionOrDefault?.ShouldSkip == true

2. Documentation says 'Removed' not 'Deprecated' (line 175-178)

   • Updated to clarify ModuleFailedException and ModuleSkippedException are deprecated with [Obsolete], not removed

3. Invalid property pattern syntax (line 79)

   • Fixed { Decision.Reason: var reason } to { Decision: var skip } with skip.Reason

4. Documentation-code discrepancy about JSON serialization

   • Added note explaining why custom JsonConverter is used instead of [JsonDerivedType] (generic types)

5. StackTrace serialization limitation

   • Added comment documenting that StackTrace is intentionally not restored during deserialization (for stability)

Not Changed (By Design):

• Duration calculation duplication: While the suggestion to extract a helper is valid, the current code is clear and the slight duplication is acceptable for readability
• JSON null-forgiving operators: The deserialization already has try/catch handling and returns a default exception if anything fails

[thomhurst]

Summary

This PR adds planning documentation for a discriminated union refactoring of ModuleResult and updates existing code to use .ValueOrDefault instead of .Value.

Critical Issues

❌ BLOCKING: Breaking changes without implementation

The PR changes dozens of call sites from .Value to .ValueOrDefault but does not implement the ValueOrDefault property in the actual ModuleResult<T> class.

Evidence from the diff:

• Line 60 in src/ModularPipelines.Build/Modules/CreateReleaseModule.cs: Uses versionInfoResult.ValueOrDefault
• Current src/ModularPipelines/Models/ModuleResult.cs: Only has .Value property (lines 46-67), no ValueOrDefault property exists

This will cause compilation failures. The implementation changes described in the planning documents must be completed before changing call sites.

Implementation order should be:

1. Add ValueOrDefault property to ModuleResult<T> (can coexist with .Value initially)
2. Update call sites to use ValueOrDefault
3. Complete discriminated union refactoring (if desired)

OR keep this as documentation-only (don't change call sites).

Suggestions

Documentation Quality:
The planning documents in docs/plans/ are well-structured and thorough. The discriminated union design looks solid with good use of C# records and pattern matching.

Minor: Task 5 Step 1 command
In the implementation plan (line 388), the grep command for finding usages could be more specific:

grep -rn "result\.Value" src/ModularPipelines/Engine/ --include="*.cs"

Verdict

⚠️ REQUEST CHANGES - Critical compilation issue: code uses ValueOrDefault property that doesn't exist yet in ModuleResult.

[thomhurst]

Summary

This PR implements a discriminated union pattern for ModuleResult<T>, replacing exception-throwing behavior with type-safe Success/Failure/Skipped variants and pattern matching support.

Critical Issues

None found ✅

The PR addresses all previous review feedback comprehensively:

• Null safety: Fixed potential null reference in PackProjectsModule.cs:41 using null-conditional operator
• Timing bug: Implemented GetTimingInfo helper to avoid inconsistent DateTimeOffset.Now calls
• Security: Added namespace restrictions (System.*) for exception deserialization, switched from AssemblyQualifiedName to FullName
• Documentation: Corrected statements about deprecated vs removed classes

Suggestions

1. IModuleResult interface compatibility (minor)

The IModuleResult interface removes the Exception and SkipDecision properties in favor of ExceptionOrDefault and SkipDecisionOrDefault.

Current interface (lines removed):

• Exception? Exception { get; }
• SkipDecision SkipDecision { get; }

Impact: This is a breaking change for anyone using IModuleResult directly. Consider if backwards compatibility matters - you could keep the old properties as explicit interface implementations that throw NotSupportedException with migration guidance.

Status: Likely acceptable as a major version change, but worth confirming.

---

2. Exception Message Sanitization (security hardening)

In ModuleResult.cs:284-341, the ExceptionJsonConverter serializes exception messages directly without sanitization. Exception messages can contain sensitive information (file paths, connection strings, user input).

Suggestion: Add a comment/documentation warning that consumers should sanitize exception messages before serialization if they plan to export results to untrusted locations.

---

3. StackTrace preservation comment (clarification)

The JSON documentation mentions that StackTrace is serialized but notes it won't be "restored" during deserialization. This is correct behavior (StackTrace is a read-only property), but could be clearer.

Current: Serialized but not restored
Suggested: Add comment: // Note: StackTrace is serialized for debugging but cannot be restored (read-only property)

---

Previous Review Status

All critical issues from previous reviews have been addressed:

1. ✅ Null reference bug fixed with ?.ShouldSkip == true
2. ✅ Timing inconsistency fixed with helper method
3. ✅ Security hardened with namespace checks
4. ✅ Documentation corrected (deprecated vs removed)
5. ✅ Pattern matching syntax fixed

Verdict

✅ APPROVE

This is a well-designed discriminated union implementation following C# best practices. The security improvements and bug fixes demonstrate careful attention to review feedback. The breaking changes are clearly documented and appropriate for a major version bump.

[thomhurst]

Summary

This PR implements a discriminated union pattern for ModuleResult, replacing exception-throwing .Value access with type-safe pattern matching using Success/Failure/Skipped variants.

Critical Issues

None found ✅

Suggestions

1. Exception Type Security Restriction May Be Too Strict

Location: src/ModularPipelines/Models/ModuleResult.cs:319

The ExceptionJsonConverter only allows deserializing exceptions from the System namespace:

if (exceptionType \!= null &&
    typeof(Exception).IsAssignableFrom(exceptionType) &&
    (exceptionType.Namespace?.StartsWith("System", StringComparison.Ordinal) == true))

Issue: This prevents deserializing common exceptions from other namespaces like:

• Microsoft.Data.SqlClient.SqlException
• Npgsql.NpgsqlException
• Custom application exceptions

Suggestion: Consider using a more permissive approach:

• Allow exceptions from well-known safe assemblies (System., Microsoft.)
• OR maintain a configurable allowlist of exception types
• OR document this limitation clearly for users

If the strict restriction is intentional for security, add a comment explaining the threat model.

2. Timing Information Loss Not Well Documented

Location: src/ModularPipelines/Models/ModuleResult.cs:198-206

The GetTimingInfo method silently returns TimeSpan.Zero when timing info is invalid:

var duration = (ctx.StartTime == DateTimeOffset.MinValue || ctx.EndTime == DateTimeOffset.MinValue)
    ? TimeSpan.Zero
    : end - start;

Issue: This makes it impossible to distinguish between:

• A module that ran instantly (0ms)
• A module with missing timing information

Suggestion: Consider:

• Using TimeSpan.MinValue or nullable TimeSpan? to indicate invalid timing
• OR documenting this behavior prominently in the property documentation

3. Pattern Matching Example Could Show More Realistic Usage

Location: Documentation and code examples throughout

The examples show basic pattern matching, but don't demonstrate common real-world scenarios like:

• Chaining multiple module results together
• Using the Match helper for functional composition
• Handling partial failures in parallel module execution

Suggestion: Add more comprehensive examples in the documentation showing these patterns, especially since this is a breaking change and users need clear migration guidance.

Observations (Non-Blocking)

✅ Good: Comprehensive Security Documentation

The ExceptionJsonConverter has excellent security documentation explaining the rationale for limiting serialized exception information. This is a model for other converters.

✅ Good: Consistent Refactoring

All 60+ usages of .Value were updated to .ValueOrDefault, showing thorough attention to detail across the entire codebase.

✅ Good: Backward Compatibility Approach

Marking ModuleFailedException and ModuleSkippedException as [Obsolete] instead of deleting them gives consumers a clear upgrade path.

✅ Good: Null Safety

The use of null-conditional operators (e.g., changedFiles.SkipDecisionOrDefault?.ShouldSkip == true) throughout shows careful attention to null safety.

Verdict

✅ APPROVE - No critical issues

This is a well-executed refactoring that meaningfully improves type safety. The suggestions above are minor enhancements that could be addressed in follow-up PRs if desired.

[thomhurst]

Summary

This PR implements a discriminated union pattern for ModuleResult<T>, replacing the exception-throwing Value property with type-safe pattern matching using three sealed record variants (Success, Failure, Skipped).

Critical Issues

1. Breaking Change in JSON Serialization - Missing JsonConverter Implementation

The PR claims "JSON Serialization: Full support via custom ModuleResultJsonConverterFactory" and the plan shows this attribute:

[JsonConverter(typeof(ModuleResultJsonConverterFactory))]
public abstract record ModuleResult<T> : IModuleResult

However, I don't see ModuleResultJsonConverterFactory anywhere in the diff. The file src/ModularPipelines/Serialization/ModuleResultJsonConverterFactory.cs is not created or modified. This will cause runtime failures when trying to serialize/deserialize ModuleResult instances.

2. Obsolete Attribute Guidance Unclear

From the diff:

[Obsolete("Use pattern matching on ModuleResult<T>.Failure instead. This exception is no longer thrown.")]
public class ModuleFailedException : PipelineException

Issue: The message says "This exception is no longer thrown" but this could confuse users on older versions. Consider: "Use pattern matching on ModuleResult instead. Starting in version X.X, accessing .Value on failed results will no longer throw this exception."

3. Null Reference Risk in Factory Methods

In the new ModuleResult.cs:

internal static Success CreateSuccess(T value, ModuleExecutionContext ctx) => new(value)
{
    ModuleName = ctx.ModuleType.Name,  // ctx.ModuleType could be null
    // ...
    ModuleType = ctx.ModuleType
};

The ModuleExecutionContext.ModuleType property is typed as Type?. If null, this will throw NullReferenceException. Need null checks or validation.

Suggestions

1. ValueOrDefault Naming Clarity

ValueOrDefault returns default(T) which is null for reference types. Consider ValueOrNull for clearer intent, or ensure the documentation explicitly states this behavior.

2. Plan Documentation Cleanup

The file docs/plans/2026-01-07-module-result-discriminated-union-implementation.md (656 lines) contains Claude Code implementation instructions. Consider moving to .claude/ or deleting post-implementation to avoid confusion with user-facing documentation.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 (missing JSON converter implementation) will cause runtime failures and must be fixed before merge.

[thomhurst]

Response to JSON Converter Comment

The ModuleResultJsonConverterFactory implementation is present in the PR. It's defined as nested classes within src/ModularPipelines/Models/ModuleResult.cs:

• ModuleResultJsonConverterFactory (lines 360-374) - The factory that creates converters for ModuleResult<T>
• ModuleResultJsonConverter<T> (lines 379-477) - The actual converter implementation
• ExceptionJsonConverter (lines 284-357) - Handles exception serialization with security restrictions

The [JsonConverter(typeof(ModuleResultJsonConverterFactory))] attribute is applied to the ModuleResult<T> class at line 30.

The converter includes:

• Polymorphic serialization via $type discriminator ("Success", "Failure", "Skipped")
• Secure exception deserialization (only allows System.* namespace types)
• Full round-trip support for all three variants

This was implemented as nested classes rather than separate files to keep the serialization logic co-located with the model definition.