fix: address PR #1895 review comments

- Fix null reference in PackProjectsModule.cs using null-conditional operator
- Fix documentation: classes are deprecated, not removed
- Fix property pattern syntax in design doc example
- Update implementation plan to reflect custom JsonConverter usage
- Document StackTrace serialization limitation in exception converter

Addresses PR #1895 review feedback

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: thomhurst

docs/plans/2026-01-07-module-result-discriminated-union-implementation.md

@@ -144,9 +144,10 @@ namespace ModularPipelines.Models;
 /// }
 /// </code>
 /// </example>
-[JsonDerivedType(typeof(Success), "Success")]
-[JsonDerivedType(typeof(Failure), "Failure")]
-[JsonDerivedType(typeof(Skipped), "Skipped")]
+// NOTE: Actual implementation uses a custom JsonConverter instead of JsonDerivedType
+// because JsonDerivedType doesn't handle generic types well at runtime.
+// The actual attribute is: [JsonConverter(typeof(ModuleResultJsonConverterFactory))]
+[JsonConverter(typeof(ModuleResultJsonConverterFactory))]
 public abstract record ModuleResult<T> : IModuleResult
 {
     // === Metadata (available on all outcomes) ===

docs/plans/2026-01-07-module-result-discriminated-union.md

@@ -76,8 +76,8 @@ switch (result)
     case ModuleResult<string>.Failure { Exception: var ex }:
         Console.WriteLine($"Failed: {ex.Message}");
         break;
-    case ModuleResult<string>.Skipped { Decision.Reason: var reason }:
-        Console.WriteLine($"Skipped: {reason}");
+    case ModuleResult<string>.Skipped { Decision: var skip }:
+        Console.WriteLine($"Skipped: {skip.Reason}");
         break;
 }
 ```
@@ -172,11 +172,13 @@ internal static Skipped CreateSkipped(SkipDecision decision, ModuleExecutionCont
 
 ### Removed
 - `ModuleResult<T>.Value` property (the throwing one)
-- `ModuleFailedException` class
-- `ModuleSkippedException` class
 - `SkippedModuleResult<T>` subclass
 - `TimedOutModuleResult<T>` subclass
 
+### Deprecated (not removed)
+- `ModuleFailedException` class (marked with [Obsolete])
+- `ModuleSkippedException` class (marked with [Obsolete])
+
 ### Preserved
 - `ModuleResultType` property (now computed)
 - `IModuleResult` interface

src/ModularPipelines.Build/Modules/PackProjectsModule.cs

@@ -38,7 +38,7 @@ public class PackProjectsModule : Module<CommandResult[]>
         var others = await projectFiles.ValueOrDefault!.Others
             .Where(x =>
             {
-                if (changedFiles.SkipDecisionOrDefault.ShouldSkip)
+                if (changedFiles.SkipDecisionOrDefault?.ShouldSkip == true)
                 {
                     return true;
                 }

src/ModularPipelines/Models/ModuleResult.cs

@@ -335,6 +335,10 @@ internal sealed class ExceptionJsonConverter : JsonConverter<Exception>
             }
         }
 
+        // NOTE: StackTrace is intentionally not restored during deserialization.
+        // Setting the StackTrace property via reflection is fragile and can cause issues.
+        // The original stack trace is preserved in the JSON for diagnostic purposes,
+        // but deserialized exceptions will have a new stack trace from deserialization.
         return new Exception(message ?? "Deserialized exception");
     }