Improve module build versioning and owner summaries

[PrzemyslawKlys]

Summary

• fix the direct module build path so it consistently loads the fresh binary output instead of stale repo payloads
• sync the project PSD1 on normal successful builds, not only refresh-only flows
• add owner-facing build notes and overhaul binary conflict reporting with deduped module versions and full mismatch reports

Details

• keep the final interactive summary actionable by splitting warning notes from FYI notes
• simplify the module entry script note and remove the old bootstrapper-heavy wording
• write a full desktop binary conflict report with exact ours/theirs assembly versions while keeping the summary concise
• keep only the newest installed version per module name in conflict summaries and reports

Verification

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~ModuleBuilderBinaryConflictAdvisoryTests|FullyQualifiedName~ModulePipelineRefreshManifestOnlyTests|FullyQualifiedName~ModuleBuildWorkflowServiceTests"
• & .\Module\Build\Build-Module.ps1 -NoSign

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

PR Review: Improve module build versioning and owner summaries

Overall this is a well-structured refactor. The changes are cohesive and the intent is clear. A few observations below.

---

Build-Module.ps1 — Load order fix ✅

Moving Remove-Item for Lib to after Remove-Module is the correct fix. The comment explains the rationale clearly. Dropping the $preferBinaryImport flag in favour of always preferring the binary output simplifies the logic noticeably and the fallback Write-Warning makes the degraded path visible.

---

Potential issues

1. BuildBinaryConflictScopeText is a one-liner alias for BuildDeclaredDependencyModulesText

// ModuleBuilder.cs
private static string BuildBinaryConflictScopeText(BinaryConflictAdvisorySummary advisory)
    => BuildDeclaredDependencyModulesText(advisory);

This wrapper adds indirection without adding value. Either inline the call at the single call site or remove it.

---

2. BuildBinaryConflictModuleOwnerDetail appears unused

private static string BuildBinaryConflictModuleOwnerDetail(BinaryConflictModuleSummary module, bool includeModuleLabel)

This method is defined but not called anywhere in the diff. If it's dead code it should be removed to keep the class lean.

---

3. SummarizeItems appears unused

// ModulePipelineRunner.Run.Helpers.cs
private static string SummarizeItems(IEnumerable<string>? items, int maxItems)

Same situation — defined but never called. Remove it or wire it up.

---

4. BinaryConflictModuleSummary.Examples appears to be dead

examples is computed in the LINQ projection (top 2 per module) and stored on BinaryConflictModuleSummary.Examples, but the only consumer in the diff is WriteBinaryConflictReport, which uses module.Mismatches exclusively. If Examples isn't read anywhere, it's computing and allocating data that is never used.

---

5. Exception return in NormalizeMixedPowerShellLineEndings is approximated

catch (Exception ex)
{
    _logger.Warn($"Mixed line-ending normalization in staging failed: {ex.Message}");
    return (0, 1);  // ← hard-coded to "1 error"
}

If the outer try throws before any per-file processing, returning (0, 1) is fine. But if it throws midway, the actual converted/error counts are lost. Consider tracking counts in a local variable before the loop so you can return the real counts even on a partial failure — or at least document the approximation in a comment.

---

Minor observations

priorityModuleNames normalization is done twice

In WarnOnInstalledBinaryConflicts the caller normalises opts.BinaryConflictPriorityModuleNames into priorityModuleNames, then passes it to BuildBinaryConflictAdvisorySummary, which normalises it again. The second normalisation is harmless but redundant; consider accepting an already-normalised IReadOnlySet<string> or documenting that the method does its own normalisation and removing the outer copy.

VersionComparer could use Comparer<Version?>.Create

The private VersionComparer class exists solely to handle null gracefully when calling OrderByDescending. A one-liner static comparer or Comparer<Version?>.Create((x, y) => ...) would remove the extra class.

---

Test coverage

• WriteBinaryConflictReport (disk I/O, directory creation, file content) has no test. A simple temp-directory test would catch any path-construction bugs.
• BuildOwnerNotes is private but exercised indirectly through BuildPipelineWorkflowServiceTests. Confirming that the existing integration tests cover the new dependency-install, staging, and merge note branches would be reassuring.
• The two new tests in ModuleBuilderBinaryConflictAdvisoryTests look solid and cover the core deduplication logic well.

---

Manifest sync timing change

SyncRefreshManifestToProjectRoot and SyncPublishedManifestToProjectRoot are removed from their original mid-pipeline positions and replaced by a single SyncBuildManifestToProjectRoot call at the very end. The updated test (Run_NormalBuild_UpdatesProjectRootManifestOnly) validates the observable result, but worth double-checking that the moved sync can't be skipped by an early-exit path that didn't previously skip it.

---

Summary

Area	Status
Build-Module.ps1 load order fix	✅ Clean
Binary conflict dedup/reporting	✅ Solid logic, well tested
ModuleOwnerNote model	✅ Clean API
Spectre summary writer	✅ Good separation of warning/FYI
Dead code (BuildBinaryConflictModuleOwnerDetail, SummarizeItems, Examples)	⚠️ Should be removed
BuildBinaryConflictScopeText wrapper	⚠️ Simplify or inline
Exception error count in NormalizeMixedPowerShellLineEndings	⚠️ Minor accuracy concern
Test coverage for WriteBinaryConflictReport	⚠️ Missing

[claude]

PR Review: Improve module build versioning and owner summaries

Overall this is a solid, well-structured PR. The three core fixes (binary import path, manifest sync on normal builds, configured-version precedence) are clearly motivated and the new owner-notes system is a nice UX improvement. A few things worth looking at before merging:

Bugs / Correctness

1. WriteBinaryConflictReport - direct result.Issues.Length access (ModuleBuilder.cs)

The report header uses result.Issues.Length directly, but the rest of the method uses the local issues variable (which guards against null via ?? Array.Empty<>()). If result.Issues is ever null this line will throw while the rest of the method handles it safely. Recommend replacing with issues.Length for consistency.

2. SyncPublishedManifestToProjectRoot appears to be dead code (ModulePipelineRunner.RefreshOnlySync.cs)

Both SyncRefreshManifestToProjectRoot and SyncPublishedManifestToProjectRoot are removed from Run.cs and replaced by the single SyncBuildManifestToProjectRoot call at the end. SyncPublishedManifestToProjectRoot is still defined in RefreshOnlySync.cs but no longer called anywhere. If publish sync is now unified, the old method should be removed to avoid confusion.

Performance

3. EnsureResolvedVersionIsAvailable - worst-case 8 minutes of blocking I/O (ModuleVersionStepper.cs)

maxProbeCount = 24 probes x 20-second timeout = up to 480 seconds before the exception is thrown. In typical cases this is one fast call, but hitting a long chain of already-published versions produces a very confusing, silent delay. Consider adding a _logger.Info inside the loop, or documenting the ceiling.

Code quality

4. Redundant null-check in ResolveConfiguredVersion (ModuleBuildPreparationService.cs)

moduleVersion was already confirmed non-null/non-whitespace by the outer IsNullOrWhiteSpace guard, so the inner (moduleVersion ?? string.Empty) and second IsNullOrWhiteSpace check are dead code. Simplify to return moduleVersion.Trim();.

5. Unused fields in MergeExecutionResult (ModulePipelineRunner.MissingAnalysis.cs)

RequiredModules, ApprovedModules, DependentModules, and TopLevelInlinedFunctions are stored on MergeExecutionResult but BuildOwnerNotes (the only consumer) never reads any of them. Either use them in the notes or remove them to reduce constructor noise.

6. TopModules is redundant in BinaryConflictAdvisorySummary (ModuleBuilder.cs)

TopModules is always allModules.Take(3) and nothing in this diff reads it - both SpectrePipelineSummaryWriter and the owner-note builder iterate AllModules. The property can be dropped; callers can .Take(3) themselves.

7. Unused using directives in test file (ModuleBuildPreparationServiceTests.cs)

System.Text.Json and System.Text.Json.Serialization are added but neither is used in the new test code.

8. Build notes silently discarded in ModuleBuilder.Build (ModuleBuilder.cs)

_ = BuildInPlace(opts); discards the returned notes. Probably intentional for the lower-level installer path, but a brief comment would help a future reader understand why.

Minor

• EnsureBuildDependenciesInstalledIfNeeded catch block swallows exceptions and returns Array.Empty - consistent with original behaviour, but a _logger.Warn would make debugging easier when dependency install silently fails.
• BuildPipelineResult now takes 4 more parameters on an already long signature. A dedicated OwnerNotesContext record might clean this up in a follow-up.

What is well done

• Moving Remove-Item after Remove-Module is the correct fix for Windows file-lock issues; the comment explains why clearly.
• Unifying manifest sync to a single call at the end of a successful run is much easier to reason about than two separate conditional calls.
• The ModuleOwnerNote model is clean and the Spectre rendering (action-needed panels vs FYI table) is a clear UX win.
• Test coverage for configured-version precedence and binary-conflict report writing is solid.
• Module PSD1 version bump is correctly paired with the help version in Readme.md.