fix: restore net472 compatibility in ModulePipelineRunner

[PrzemyslawKlys]

Summary\n- fix nullable-flow-safe normalization in NormalizeDraftValue to avoid CS8602 on

et472\n- replace string.IndexOf(char, StringComparison) with string.IndexOf(string, StringComparison) for
et472 API compatibility\n\n## Why\nBuilding PSPublishModule failed for
et472 with:\n- CS8602: Dereference of a possibly null reference\n- CS1503: cannot convert from 'System.StringComparison' to 'int'\n\n## Validation\n- dotnet publish PowerForge/PowerForge.csproj -c Release -f net472\n- dotnet build PSPublishModule.sln -c Release\n- dotnet test PowerForge.Tests/PowerForge.Tests.csproj --filter ModulePipeline --nologo\n\nAll commands passed locally.

[claude]

PR Review: fix: restore net472 compatibility in ModulePipelineRunner

Summary

This PR fixes two .NET Framework 4.7.2 compatibility issues in ModulePipelineRunner.cs. The changes are minimal, targeted, and address real build failures without altering functionality.

---

Code Quality ✅

Overall Assessment: Good

Both changes follow clean coding practices:

1. NormalizeDraftValue refactoring (lines 1731-1740):

   • ✅ Properly handles null flow analysis for .NET 4.7.2
   • ✅ Explicit null check resolves CS8602 warning
   • ✅ Intermediate trimmed variable improves readability
   • ✅ Logic remains functionally equivalent to the original

2. IndexOf API compatibility fix (line 3268):

   • ✅ Correct fix: string.IndexOf(char, StringComparison) doesn't exist in net472
   • ✅ Using IndexOf("-", StringComparison.Ordinal) is the proper alternative
   • ✅ Minimal performance impact (single character string)

---

Potential Issues ⚠️

Minor concern: Missing test coverage

• NormalizeDraftValue appears to be a private helper with no direct unit tests
• Consider adding tests to verify:

  NormalizeDraftValue(null) == string.Empty
  NormalizeDraftValue("  ") == string.Empty  
  NormalizeDraftValue(" value ") == "VALUE"

Suggestion: If you're refactoring for null safety, this would be a good opportunity to add regression tests, though not required for this specific fix.

---

Performance Considerations ✅

No meaningful performance impact:

1. NormalizeDraftValue: The refactored version:

   • Adds one null check (negligible)
   • Adds one variable assignment (optimized away by JIT)
   • Functionally identical in the non-null path

2. IndexOf change:

   • IndexOf(string) vs IndexOf(char) has minimal difference for a single character
   • The ordinal comparison is identical in both cases
   • This is not a hot path (module inference happens during build setup)

---

Security Concerns ✅

No security issues identified:

• Changes are purely syntactic/compatibility fixes
• No changes to validation, input handling, or security-sensitive logic
• ToUpperInvariant() correctly used for case-insensitive normalization (not culture-dependent ToUpper())

---

Test Coverage 📋

From PR description:

✅ dotnet publish PowerForge/PowerForge.csproj -c Release -f net472
✅ dotnet build PSPublishModule.sln -c Release  
✅ dotnet test PowerForge.Tests/PowerForge.Tests.csproj --filter ModulePipeline

Validation appears thorough, but consider:

• Ensure CI/CD runs net472 builds to catch these issues automatically
• The --filter ModulePipeline test subset should ideally cover AreRequiredModuleDraftListsEquivalent which calls NormalizeDraftValue

---

Best Practices Alignment ✅

Per CONTRIBUTING.md guidelines:

• ✅ Changes are in PowerForge (core C# logic)
• ✅ No unsafe code introduced
• ✅ Maintains typed approach (no hashtable/dictionary changes)
• ✅ Build and test commands executed as documented

---

Recommendations

1. Merge: This PR is ready to merge as-is
2. Future enhancement: Consider adding CI matrix builds for all target frameworks (net472, net6.0, net7.0, etc.) to catch API compatibility issues earlier
3. Optional: Add unit tests for NormalizeDraftValue edge cases in a follow-up PR

---

Conclusion

LGTM ✅ - This is a solid compatibility fix that resolves real build errors without introducing risk. The changes are minimal, well-tested locally, and maintain code clarity.

Great work ensuring multi-framework support! 🚀

[Copilot]

Pull request overview

Restores .NET Framework 4.7.2 (net472) build compatibility for ModulePipelineRunner by avoiding nullable flow-analysis pitfalls in older reference assemblies and removing usage of APIs not available on net472.

Changes:

• Reworked NormalizeDraftValue to use explicit null + trim handling (avoids CS8602 under net472 nullable annotations).
• Replaced string.IndexOf(char, StringComparison) with string.IndexOf(string, StringComparison) to match net472 available overloads.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.