fix(build): stabilize net8 packaging resolution and module build pipeline

[PrzemyslawKlys]

Summary

• fix System.IO.Packaging version selection per target framework (net8 vs net10) in both PSPublishModule and PowerForge
• update lock files for locked restore compatibility
• harden staging/build pipeline behavior (line ending normalization and refresh-manifest flow)
• add regression tests for line endings, dependency copy behavior, and refresh-manifest-only pipeline path
• regenerate module docs from current cmdlet metadata

Validation

• dotnet test PowerForge.Tests/PowerForge.Tests.csproj -c Release --no-build
• Build/Build-Module.ps1 in PSPublishModule completes successfully
• Build/Build-Module.ps1 in PSEventViewer completes successfully
• import checks pass in both Windows PowerShell 5.1 and PowerShell 7.x

Notes

• this removes the net8 docs/import failure caused by loading a net10 System.IO.Packaging assembly during module import

[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: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-structured PR with clear, targeted fixes and solid test coverage. Here are my observations by area:

---

System.IO.Packaging version selection (PSPublishModule.csproj / PowerForge.csproj)

Positive. Splitting the combined net8.0 Or net10.0 condition into two separate ItemGroup blocks so each TFM gets the correct package version is the right fix. Loading a net10 assembly under a net8 host causes binding failures at import time, so this directly resolves the reported failure. Lock files are updated consistently.

Minor note: once a net9 TFM is added in future, a third block will be needed; a comment or TODO referencing this maintenance concern would help future contributors.

---

LineEndingConverter – bare \r handling

Correct fix. The old normalization path:

text.Replace("\r\n", "\n").Replace("\n", "\r\n")

left classic Mac (\r-only) line endings untouched; the new path explicitly normalises bare \r to \n before the final conversion. The test in LineEndingConverterTests.cs validates exactly this case. Good.

---

NormalizeMixedPowerShellLineEndings (ModuleBuildPipeline.cs)

Minor naming issue. The method is called NormalizeMixed… but uses force: true, onlyMixed: false, meaning it normalises every PowerShell file regardless of whether it has mixed endings. The name implies selective behavior. Consider renaming to NormalizeStagedPowerShellLineEndings (or similar) to avoid confusion for future maintainers.

The hardcoded LineEnding.CRLF target is fine for PowerShell compatibility on Windows-hosted consumers, but worth a comment explaining the rationale (e.g. "Windows PowerShell 5.1 and the Gallery expect CRLF").

---

ComputeExcludedLibraries – BFS change (ModuleBuilder.cs)

This is the most behaviorally significant change in the PR and warrants a closer look.

Before: the BFS cascaded unconditionally – if a PowerShell runtime library X had a dep Y, Y was excluded too (and then Y's deps, etc.).

After: only deps whose own IDs are PowerShell runtime libraries are added to the exclusion set; non-PS transitive deps are kept.

The included test validates the reference-assemblies case (Microsoft.PowerShell.5.ReferenceAssemblies → System.Text.Json should not cause System.Text.Json to be excluded). That case is clearly correct.

Question/concern: For runtime SDK packages like Microsoft.PowerShell.SDK, the old cascading would have excluded their transitive deps (e.g. Newtonsoft.Json if PS SDK brought it). With the new code, those deps get included in the module output. Is that intentional? A test covering a real-SDK-style dependency chain (not just reference assemblies) would increase confidence that this is the desired behavior and not an accidental inclusion.

---

Reflection-based private-method tests (ModuleBuilderDependencyCopyTests.cs)

var method = typeof(ModuleBuilder).GetMethod("ComputeExcludedLibraries", BindingFlags.Static | BindingFlags.NonPublic);

Testing private methods via reflection is fragile: the test breaks silently if the method is renamed, extracted, or inlined, and it does not appear in normal public-API coverage. Consider either:

• Making the method internal and using [assembly: InternalsVisibleTo("PowerForge.Tests")] in the project, or
• Testing through CopyPublishOutputBinaries or higher-level public surface.

The same applies to CopyPublishOutputBinaries via reflection in the same file.

---

RefreshManifestOnly / RefreshPSD1Only feature

Well-implemented. The flag consistently gates all non-manifest phases (binary publish, bootstrapper generation, merge, signing, artefacts, publishes, documentation, tests, installs) in a single Plan-time block, which is the right place for it. The ModulePipelineRefreshManifestOnlyTests verify the key assertions.

One small thing: CsprojPath is cleared in two places – once in ModulePipelineRunner.Plan.cs (CsprojPath = refreshPsd1Only ? string.Empty : csproj) and again defensively in ModuleBuildPipeline.cs. The defensive guard in ModuleBuildPipeline is good practice since ModuleBuildSpec can be constructed directly; a brief comment there (// also guarded at plan time) would clarify intent.

---

UseCiStrictDefaults extraction (WebPipelineRunner.cs)

Good refactor. Extracting the repeated ConsoleEnvironment.IsCI && \!isDev pattern into a single method and adding explicit mode: ci recognition is cleaner and makes the semantics testable. WebPipelineRunnerModeStrictDefaultsTests.cs covers the relevant cases.

---

WebVerifyPolicy warning code classification

Backward compatible. The OR-with-fallback pattern (code-prefix first, then legacy message text) is the right approach. Tests cover both paths. Minor style note: in IsNavigationLintWarning, the HasWarningCodePrefix helper already handles the null/empty check internally, so the outer \!string.IsNullOrWhiteSpace(warning) guard in the legacy fallback arm is slightly redundant but harmless.

---

Sitemap canonicalization (WebSitemapGenerator.cs)

The new IsApiLegacyHtmlAliasRoute guard is appropriately narrow (requires /api/ in path, excludes /types/, excludes index.html and 404.html). The logic of promoting a legacy .html route to a canonical slash route in the sitemap when the slash route doesn't already exist is sound. Tests in WebSitemapGeneratorCanonicalizationTests.cs cover this path.

---

Module documentation (Module/Docs/*.md)

The addition of Possible values: lines to parameter docs is a useful improvement to the generated docs. The new Docs/PowerForge.Web.WarningCodes.md catalog and the cross-references added to Pipeline.md, QualityGates.md, and ContentSpec.md are valuable for discoverability.

---

Summary

Area	Assessment
net8 packaging fix	✅ Correct, targeted
LineEndingConverter CR fix	✅ Correct, tested
RefreshManifestOnly feature	✅ Well-implemented, tested
NormalizeMixedPSLineEndings	⚠️ Misleading name; consider rename
ComputeExcludedLibraries BFS	⚠️ Behavioral change; SDK case needs test
Reflection-based tests	⚠️ Fragile; prefer InternalsVisibleTo or public API
UseCiStrictDefaults refactor	✅ Clean, tested
Warning code classification	✅ Backward compatible, tested
Sitemap canonicalization	✅ Appropriately narrow guard
Docs / schema updates	✅ Comprehensive

The critical items to address before merge are the BFS change coverage question and the reflection-based tests. The naming issue is minor but worth a note.

[PrzemyslawKlys]

Added a follow-up fix for PSGallery publishing with PSResourceGet:\n\n- normalize ExternalModuleDependencies during repository publish\n- for PSResourceGet + PSGallery, automatically remove inbox PowerShell modules (for example Microsoft.PowerShell.Utility, Microsoft.PowerShell.Management, Microsoft.PowerShell.Diagnostics) before publish\n- keep normal dependency checks for real required dependencies\n- added tests: ModulePublisherExternalDependenciesTests\n\nThis addresses the Publish-PSResource dependency-not-found failure for inbox modules.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a solid, well-scoped PR. The core fixes are correct and the new RefreshPSD1Only feature is cleanly integrated throughout the pipeline. Tests cover the key scenarios. Below are observations ranging from actionable issues to minor suggestions.

---

Core packaging fix (PSPublishModule.csproj / PowerForge.csproj)

The split of the System.IO.Packaging condition into per-TFM ItemGroup blocks is the right fix. Pinning 8.0.1 for net8.0 and 10.0.1 for net10.0 prevents the net10 assembly from being loaded into a net8 host — the root cause of the import failure. Clean and correct.

---

LineEndingConverter.cs

The CR-only fix is minimal and correct:

// Before
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
// After
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");

The two-step normalisation (collapse everything to LF, then expand to target) is idiomatic and handles mixed \r\n/\r content correctly.

---

ModuleBuildPipeline.cs — NormalizeMixedPowerShellLineEndings

Potential performance concern: The method uses force: true and onlyMixed: false, which means every PowerShell file in staging is unconditionally read and rewritten on every build, even if already CRLF. For large modules this adds measurable I/O on every run. Consider using onlyMixed: true (or at minimum force: false) to skip files that are already correct:

var result = converter.Convert(new LineEndingConversionOptions(
    enumeration: enumeration,
    target: LineEnding.CRLF,
    createBackups: false,
    backupDirectory: null,
    force: false,      // skip files already CRLF
    onlyMixed: true,   // only fix files with mixed/non-CRLF endings
    ...));

If force: true is intentional (e.g., to also normalise encoding/BOM), a comment explaining why would help.

Platform consideration: Hard-coding CRLF in the staging pass is appropriate for a PowerShell module targeting Windows PowerShell 5.1 compatibility, but it's worth documenting this assumption so future maintainers understand it is not a bug on Linux build agents.

---

ModuleBuilder.cs — CreateCopyPlan / IncludeTopLevelBinaryFiles

Behaviour change: Before this PR, top-level binary fallback only ran when TryCreateCopyPlanFromDeps returned null. Now it also runs on a successful deps.json plan, meaning any DLL present in the publish dir but absent from the deps graph is always included. The test validates the intended case (System.IO.Pipelines.dll present but not in deps).

This is generally safe because AlwaysExcludedRootFiles and IsBinaryFileName filter out PS host assemblies, but it is worth noting that this could pull in platform-specific or test assemblies if they land in the publish directory for any reason. A comment documenting this intentional double-inclusion would help reviewers in future.

ComputeExcludedLibraries BFS fix:

// Added guard — only cascade exclusion for PS runtime libs
var depId = GetLibraryIdFromTargetKey(depKey);
if (\!IsPowerShellRuntimeLibraryId(depId)) continue;

This correctly prevents a PS SDK dependency on (e.g.) System.Text.Json from causing System.Text.Json to be excluded from the module output. The test in ModuleBuilderDependencyCopyTests covers this case well.

---

ModulePipelineRunner.Run.cs — style nit

var mergedScripts = plan.BuildSpec.RefreshManifestOnly ? false : ApplyMerge(plan, buildResult);

This silently skips the call to ApplyMerge (which presumably has side effects) when RefreshManifestOnly is true — which is the intent. However, the ternary form obscures that. A short-circuit form is more idiomatic:

var mergedScripts = \!plan.BuildSpec.RefreshManifestOnly && ApplyMerge(plan, buildResult);

Minor, but improves readability.

---

ModulePublisher.cs — in-place manifest patch before publish

NormalizeExternalDependenciesInManifestForPublish modifies the staging manifest on disk before calling _psResourceGet.Publish. If the publish step throws after the manifest has been written, the staging manifest is left in the modified state. Since this is in the staging directory (not the source), it's acceptable — but a comment would help:

// Modify staging manifest only; source is untouched.
ManifestEditor.TrySetPsDataStringArray(buildResult.ManifestPath, "ExternalModuleDependencies", filtered);

Completeness of InBoxExternalModuleDependenciesForPsGallery: The 8 entries cover the main offenders. PackageManagement and CimCmdlets are also inbox in PS 5.1+ but may not be commonly listed in ExternalModuleDependencies. If you see publish failures related to those in practice, extending the set would be straightforward.

---

Schema / Documentation mismatch

Docs/PowerForge.Web.ContentSpec.md documents two new EditorialCards fields:

• ShowCategories: default show_categories helper behavior when argument is omitted
• LinkTaxonomy: default link_taxonomy helper behavior when argument is omitted

However, Schemas/powerforge.web.sitespec.schema.json does not include ShowCategories/showCategories or LinkTaxonomy/linkTaxonomy in the EditorialCardsSpec definition. Since the schema uses "additionalProperties": false, users who add those properties to their site spec will get a schema validation error even though the documentation says they are supported. Either add them to the schema or note they are not yet schema-validated.

---

Tests

Reflection on private methods (ModuleBuilderDependencyCopyTests): Both tests call private methods via BindingFlags.NonPublic. This works but is brittle — renames or signature changes break the test without a compile error. Consider annotating the methods with internal and adding [assembly: InternalsVisibleTo("PowerForge.Tests")] to make them directly accessible in tests with compile-time safety.

WebVerifyPolicy.cs — null-safety in HasWarningCodePrefix: The new helper does not null-check warning before passing it to TryGetCode. The original IsNavigationLintWarning had an explicit \!string.IsNullOrWhiteSpace(warning) guard that is no longer in the primary path. If TryGetCode handles null gracefully, this is fine — but it is worth verifying (or adding a null-check defensively).

Test coverage for NormalizeExternalModuleDependenciesForRepositoryPublish: ModulePublisherExternalDependenciesTests covers the three main cases (PSGallery+PSResourceGet, PSGallery+PowerShellGet, custom repo). A case for null/empty externalModuleDependencies input would complete the boundary coverage.

---

Lock files

The packages.lock.json updates are expected and correct as a consequence of the version pin changes. Locked restore compatibility is maintained.

---

Summary

Area	Verdict
Core net8/net10 packaging fix	Correct
CR-only line ending fix	Correct
RefreshManifestOnly feature	Well-implemented
Dependency exclusion cascade fix	Correct, well-tested
PSGallery inbox module filtering	Practical and scoped correctly
Schema/doc mismatch (ShowCategories/LinkTaxonomy)	Should be resolved before merge
Staging CRLF normalization perf	Consider onlyMixed: true
Reflection-based private tests	Low priority; consider InternalsVisibleTo

The schema mismatch is the only item I'd flag as worth addressing before merge, since it will produce misleading validation errors for users following the new documentation.

[PrzemyslawKlys]

Updated approach to a global fix:\n\n- added custom dependency precheck for RequiredModules before publish (repository-aware)\n- switched PSResourceGet publish call to -SkipDependenciesCheck\n- this avoids false failures from ExternalModuleDependencies (for example inbox/RSAT modules such as ActiveDirectory)\n- preserved manifest metadata (no longer strips external dependencies before publish)\n\nNew tests cover required-module version matching rules used by the precheck.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-structured PR with clear intent and solid test coverage for the key fixes. Below are observations by area.

---

System.IO.Packaging version split (*.csproj, packages.lock.json)

The root cause fix is correct and straightforward. Splitting the single Or-condition ItemGroup into two TFM-specific groups ensures each target framework gets its matching package version. Previously, both net8.0 and net10.0 resolved to 10.0.1, loading a net10 assembly in a net8 host — the documented import failure. No concerns here.

---

LineEndingConverter (PowerForge/Services/LineEndingConverter.cs)

// Before
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n");

// After
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n").Replace("\r", "\n");

The fix is correct. The three-step chain for CRLF (\r\n → \n, then \r → \n, then \n → \r\n) handles legacy Mac CR-only files without risk of double-expansion since \r\n pairs are collapsed first. The new test covers this case.

---

Line ending normalization in staging (ModuleBuildPipeline.cs)

Hardcoded exclude directories — NormalizeMixedPowerShellLineEndings appends "Artefacts", "Modules", "Ignore" to the exclusion set regardless of the module being built. These are project-specific names baked into library code. If a module legitimately contains a folder called Modules (common for nested module layouts), its PowerShell files would silently be skipped. Consider sourcing these from spec.ExcludeDirectories or from a constant/config rather than hardcoding.

force: true + onlyMixed: false — This normalizes all staged PowerShell files unconditionally, not just those with mixed endings. For large modules this adds I/O proportional to every .ps1/.psm1/.psd1 file. Functionally correct, but worth noting as a performance consideration for heavy modules. onlyMixed: true would limit work to files that actually need it.

---

ModuleBuilder.ComputeExcludedLibraries cascade fix

Before: all transitive dependencies of any PowerShell runtime library were excluded.
After: only dependencies that are themselves PS runtime libraries are excluded.

The added guard:

var depId = GetLibraryIdFromTargetKey(depKey);
if (!IsPowerShellRuntimeLibraryId(depId)) continue;

stops the BFS from cascading into non-PS dependencies (e.g., System.Text.Json depended on by Microsoft.PowerShell.SDK). This is the right fix for over-exclusion. The test in ModuleBuilderDependencyCopyTests validates the intent clearly.

IncludeTopLevelBinaryFiles is now called on both paths — When a deps.json plan is found, top-level binaries are now also included. Previously the fallback was only used when deps.json was absent. Make sure this doesn't regress by including files that were previously deliberately excluded when a proper deps.json exists (e.g., transitively excluded PS host assemblies that happen to sit at the top level). A test exercising the "deps.json and extra top-level dll" scenario would add confidence.

---

RefreshManifestOnly / RefreshPSD1Only pipeline path

The plan-time disabling of merge, signing, install, publish, artefacts, docs, compat, and tests is comprehensive. The double-clearing of CsprojPath (once in Plan() and again in BuildInStaging()) is defensive and does not cause harm, but is redundant since the spec is set in Plan() before it reaches BuildInStaging(). A comment explaining why BuildInStaging also guards would help future readers.

The Run.cs change:

var mergedScripts = plan.BuildSpec.RefreshManifestOnly ? false : ApplyMerge(plan, buildResult);

is slightly awkward. !plan.BuildSpec.RefreshManifestOnly && ApplyMerge(...) is equivalent and more idiomatic for a boolean result.

---

ModulePublisher — custom dependency pre-check + skipDependenciesCheck: true

The approach of disabling the built-in check and substituting a custom one with proper version-range semantics is reasonable. A few observations:

Network failure blocks publishing with no escape hatch. If the target repository is slow, air-gapped, or temporarily unavailable, ValidateRequiredModulesForRepositoryPublish throws and publishing is aborted. There is no option to skip this check (similar to how the built-in skipDependenciesCheck could be set to true). Consider exposing a SkipDependenciesCheck or ValidateDependencies flag on the publish configuration so callers can opt out when needed.

Hardcoded 2-minute timeout per dependency. For a module with many required modules, the timeout multiplies. This is fine for typical use but could be a CI concern for repositories with many dependencies. Exposing the timeout or using a shared budget across all checks would be more robust.

hasConstraints variable is slightly misleading. It is computed but only consulted in the !TryParseSemVer(candidateVersion) branch — not in the main parsed-version logic. Once inside the parsed path, the code naturally returns true at the end if no constraint is set, so the behavior is correct. But the variable name implies broader use; an inline comment would clarify.

GetRequiredModulesForPublish fallback order — The method prefers the manifest on disk over plan.RequiredModules. If staging modifies the manifest (e.g., version bump, adds entries), the on-disk version is used, which is correct. The fallback to plan data is fine for cases where the manifest path is unavailable, though in practice ManifestPath should always be set after a successful build.

---

Tests

• LineEndingConverterTests: clear, exercises the exact regression case.
• ModuleBuilderDependencyCopyTests: uses BindingFlags.NonPublic reflection to access ComputeExcludedLibraries and CopyPublishOutputBinaries. This works but is fragile — a rename or signature change silently breaks the test at runtime rather than compile time. Consider marking these methods internal and adding [assembly: InternalsVisibleTo("PowerForge.Tests")] to avoid reflection.
• ModulePipelineRefreshManifestOnlyTests: good coverage of both plan-disablement and run-path skipping.
• ModulePublisherRequiredModulesTests: covers exact, minimum, and range version semantics well.

---

Minor / Nits

• Whitespace-only changes in ModulePipelineRunner.Plan.cs (trailing space alignment) add diff noise without functional change — worth excluding from the PR or committing separately.
• Trailing newline removed from LineEndingConverter.cs — fine, but ironic in a PR about line endings.
• Module/Docs/ changes (Possible values: lines added to every parameter) appear to be generated output from a doc regeneration pass. These are mechanical and correct.

---

Summary

Area	Status
System.IO.Packaging TFM split	✅ Correct fix
CR-only line ending handling	✅ Correct, tested
Staging CRLF normalization	✅ Works; hardcoded excludes worth revisiting
Dependency exclusion BFS fix	✅ Correct fix, well-tested
Top-level binary inclusion on deps path	⚠️ Additional test recommended
RefreshManifestOnly pipeline	✅ Comprehensive, minor style nits
Custom dependency pre-check	⚠️ No escape hatch for network failures / air-gapped repos
Test approach (reflection)	⚠️ InternalsVisibleTo preferred over NonPublic reflection

Good work overall — the core packaging fix and line-ending hardening are solid. The main actionable items are the network-failure escape hatch for the dependency pre-check and the hardcoded exclude directories in staging normalization.

[PrzemyslawKlys]

Follow-up pushed in 0c7eaeb: clear stale RequiredModules in staged manifest when MergeMissing filters configured required modules to empty. Added regression test in ModulePipelineApprovedModulesTests.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-structured PR with a clear primary bug fix and solid supporting improvements. The test coverage is notably improved. A few items worth discussing below.

---

The core fix

System.IO.Packaging per-framework version split (both .csproj files, lock files) — clean and correct. Splitting the combined net8.0 Or net10.0 condition into two separate ItemGroup conditions is the right approach. The lock files are updated consistently. This directly resolves the net8 import failure.

---

Correctness concerns

Schema gap: ShowCategories and LinkTaxonomy missing from JSON schema (Schemas/powerforge.web.sitespec.schema.json)

The C# EditorialCardsSpec model gains ShowCategories and LinkTaxonomy properties, but the schema definition only includes Image, ImageFit, ImagePosition, ImageAspect, Variant, GridClass, and CardClass. The schema uses "additionalProperties": false, which means any site.json that sets these two fields will fail schema validation. They should be added:

"ShowCategories": { "type": "boolean" },
"showCategories": { "type": "boolean" },
"LinkTaxonomy": { "type": "boolean" },
"linkTaxonomy": { "type": "boolean" }

---

CreateCopyPlan behavioral change in ModuleBuilder.cs

Old behavior: IncludeTopLevelBinaryFiles was only called on the fallback path (no deps file).
New behavior: it is also called when TryCreateCopyPlanFromDeps succeeds, augmenting the deps-derived plan with additional top-level DLLs.

This is presumably intentional (catches binaries the deps graph misses), and ModuleBuilderDependencyCopyTests.CopyPublishOutputBinaries_IncludesTopLevelFallbackFiles_WhenDepsMissesThem validates the intent. However it could also include platform-specific or test DLLs not referenced by the project's deps tree. Worth a comment clarifying the intent, especially since it changes the set of files copied for any project that has a valid deps.json.

---

NormalizeMixedPowerShellLineEndings name vs. behavior

The method is named "NormalizeMixed..." but uses force: true and onlyMixed: false, which normalizes all PowerShell files in staging unconditionally, not only mixed ones. This is likely intentional — the staging step should produce a clean, deterministic output — but the name implies it only touches mixed files. Either rename to NormalizePowerShellLineEndings or add a brief comment clarifying the intent.

---

EditorialPostNav: currentIndex == -1 edge case

When the current page cannot be matched in the sorted collection (currentIndex == -1), newer/older navigation is correctly skipped. However ResolveRelatedPosts is still called with currentIndex = -1. Inside ResolveRelatedPosts, the filter is index != currentIndex, and since index is 0-based, -1 never matches — meaning all items become candidates for the related section. An unindexed post may still show a "Related posts" block using all collection items. This is probably harmless, but if the intent is to suppress the related section for an unindexed post, a currentIndex >= 0 guard before calling ResolveRelatedPosts would be cleaner.

---

Design observations

shouldPatchRequiredModules semantics change in ModulePipelineRunner.Run.cs

The change from manifestRequiredModules is { Length: > 0 } to shouldPatchRequiredModules (based on plan.RequiredModules.Length > 0) is subtle but intentional and correct — it ensures stale required-module entries are cleared from the manifest even when MergeMissing/ApprovedModules filtering reduces the resolved list to empty. The new test Run_ClearsStaleRequiredModules_WhenAllConfiguredRequiredAreFilteredByMergeMissing explicitly covers this. Good catch.

skipDependenciesCheck: true + custom validation

Switching to skipDependenciesCheck: true and doing the check manually gives you control over error messages and lets you check against credentials-protected registries. The 2-minute timeout per dependency lookup is not user-configurable. For modules with many required dependencies or a slow registry this could block the pipeline for a long time. Consider exposing a timeout parameter or at least making it a named constant.

UseCiStrictDefaults extraction in WebPipelineRunner.cs

Good refactoring — removing the repeated ConsoleEnvironment.IsCI && !isDev pattern and making mode: ci an explicit opt-in is a clear improvement.

---

Minor items

• DoesVersionMatchRequiredModule: when a candidate version cannot be parsed by TryParseSemVer and the requirement specifies only ModuleVersion/MaximumVersion (not RequiredVersion), the method returns false. This is conservative but could reject pre-release or non-semver tags from private registries. Worth documenting the expected version format.

• The NormalizeObjectPosition regex allows bare numbers without units (e.g., 20). That is valid CSS in some contexts for object-position, so this is fine — just worth confirming it is intentional.

• EditorialPostNav does not have a test covering rendered HTML output (unlike EditorialCards and EditorialPager). Given the new AllItems context property and the collection-lookup behavior, an integration test similar to Build_RendersPfEditorialCards_InScribanTheme would help prevent regressions.

---

Summary

The primary fix (per-framework System.IO.Packaging selection) is correct, and the supporting changes (line-ending handling, RefreshManifestOnly, dependency cascade fix, custom pre-publish validation, warning-code classification) are well-reasoned. The schema gap for ShowCategories/LinkTaxonomy should be addressed before merge to avoid breaking schema-validated site.json files.

[PrzemyslawKlys]

Added another follow-up in 885c55d:

• publish precheck now skips repository lookup for RequiredModules entries that are also declared in PSData.ExternalModuleDependencies
• added ManifestEditor.TryGetPsDataStringArray for robust manifest reads
• added tests in ModulePublisherRequiredModulesTests and ManifestEditorStringArrayTests

This supports runtime-required inbox modules while avoiding false PSGallery dependency failures under PSResourceGet.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-scoped PR with clear intent, solid test coverage for the core fixes, and good documentation hygiene. Here are my findings grouped by severity.

---

Bug / Correctness

ComputeExcludedLibraries BFS — cascade direction may be inverted

The fix adds a guard before cascading into transitive excludes:

foreach (var depKey in deps)
{
    var depId = GetLibraryIdFromTargetKey(depKey);
    if (!IsPowerShellRuntimeLibraryId(depId)) continue;  // only cascade if dep is itself a PS runtime lib
    if (excluded.Add(depKey)) queue.Enqueue(depKey);
}

This means transitive deps of a PS runtime lib are only excluded when they themselves are PS runtime libs. The test validates the intended behaviour (non-PS transitive deps like System.Text.Json are no longer cascaded into the exclude set). That is almost certainly the right semantic — but it's worth double-checking that genuinely shared PS runtime transitive libs (e.g. System.Management.Automation → Newtonsoft.Json on older targets) are still correctly excluded in cases where both the root and the dep are PS runtime libs. The test only covers the case where they are not.

IncludeTopLevelBinaryFiles now runs after a deps-based plan

Previously IncludeTopLevelBinaryFiles was the fallback path (no deps file). Now it runs after a deps-based plan too. If the deps.json intentionally omits a top-level binary (e.g. a test helper DLL that was compiled into the publish dir but should not be shipped), it will now be included. This is presented as a fix for missed files, but it widens the blast radius. A comment explaining why the deps-based plan can legitimately miss files that the top-level scan would find (e.g. native runtime assets, resource satellites) would help reviewers verify correctness.

---

Potential Issues

NormalizeMixedPowerShellLineEndings — exception swallowing and opt-out

The method wraps the entire normalization in a broad catch (Exception ex) that emits only a warning and continues. A staging failure that silently produces files with wrong line endings is worse than a hard failure. Consider whether this should be configurable (e.g. a FailOnLineEndingNormalizationError flag) or at least log the full exception including the stack trace for diagnostics.

Additionally, onlyMixed: false + force: true rewrites every staged PowerShell file to CRLF regardless of its current state. Developers on Linux/macOS using intentional LF in their modules will have staging silently rewrite their files. This may be intentional policy, but it is not mentioned in the PR description and is a behavioural change for cross-platform users.

ValidateRequiredModulesForRepositoryPublish — per-dependency network calls with 2-minute timeouts

Each required module triggers a separate Find call to the repository with a 2-minute timeout. For a module with 10 dependencies against a slow internal registry this becomes a 20-minute blocking pre-publish step. Consider:

• Batching the Find calls where the underlying API supports it
• Making the timeout configurable via ModulePipelinePlan or the publish segment config

skipDependenciesCheck: true — silent bypass of PSResourceGet validation

Switching from false to true removes the PSResourceGet built-in dependency check in favour of the new custom validation. If ValidateRequiredModulesForRepositoryPublish throws (e.g. network error), the catch re-throws an InvalidOperationException which will surface to the user. But if it returns silently (e.g. requiredModules.Length == 0 from a manifest read failure), the publish proceeds without any dependency check at all. Validate that GetRequiredModulesForPublish can never silently return empty when the manifest actually contains required modules.

---

Minor / Style

Schema missing ShowCategories / LinkTaxonomy in EditorialCardsSpec

PowerForge.Web.ContentSpec.md documents ShowCategories and LinkTaxonomy as supported EditorialCards fields, but Schemas/powerforge.web.sitespec.schema.json does not include them in EditorialCardsSpec. Users relying on schema validation / IDE autocompletion will not see these properties. They should be added (both PascalCase and camelCase variants, consistent with the other fields).

Naming inconsistency: RefreshPSD1Only vs RefreshManifestOnly

The config key read from the build segment is RefreshPSD1Only and the model property is RefreshManifestOnly. This dual naming is mildly confusing when reading the pipeline code. A comment on the model property noting the public config key name would help.

mergedScripts ternary style

var mergedScripts = plan.BuildSpec.RefreshManifestOnly ? false : ApplyMerge(plan, buildResult);

C# idiomatic form for this pattern is !plan.BuildSpec.RefreshManifestOnly && ApplyMerge(plan, buildResult). Not a bug, just readability.

Module docs: empty Possible values: entries

Parameters without enum values now emit Possible values: (trailing space, no values). This is cosmetically noisy in generated documentation. It would be cleaner to suppress the line entirely when there are no possible values to enumerate.

---

Positives

• The System.IO.Packaging per-TFM version split is the right fix; the lock file update correctly reflects this.
• LineEndingConverter.NormalizeEndings correctly orders the replacements to avoid double-converting CRLF when handling legacy \r-only files.
• The RefreshPSD1Only feature properly zeroes out all non-manifest pipeline phases (merge, sign, install, tests, artefacts, publishes) in the Plan phase — no partial-skip risks.
• shouldPatchRequiredModules = plan.RequiredModules is { Length: > 0 } correctly handles the case where all required modules are filtered out by MergeMissing, preventing stale entries from being written.
• Test coverage for LineEndingConverter, ModuleBuilder dependency copy, RefreshManifestOnly plan/run paths, and ModulePublisher version matching is appreciated and directly validates the fixes.
• WarningCodes.md is a valuable addition for operations teams managing suppress lists.

[PrzemyslawKlys]

Implemented legacy behavior restore in 9d26d7c:

• ConfigurationModule -Type ExternalModule now also mirrors entries into manifest RequiredModules
• ExternalModuleDependencies is still written as before
• RequiredModulesForPackaging remains unaffected so external/inbox deps are not bundled into artefact module packs

Added regression tests in ModulePipelineRegressionParityTests to verify both planning and manifest output behavior for:

• Microsoft.PowerShell.Utility
• Microsoft.PowerShell.Management
• Microsoft.PowerShell.Diagnostics

This keeps your existing build script unchanged while restoring import/runtime semantics.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a solid PR with good test coverage for the new functionality. The core net8 fix is clean and targeted. A few observations below, ranging from potential issues to minor notes.

---

Core Bug Fix: net8/net10 System.IO.Packaging Split

PSPublishModule/PSPublishModule.csproj — Clean, correct fix. Splitting the ItemGroup condition into two separate ones ensures net8 gets 8.0.1 and net10 gets 10.0.1 rather than both getting 10.0.1. The lock file update follows naturally.

---

LineEndingConverter.NormalizeEndings (LineEndingConverter.cs)

The old code silently left bare \r (classic Mac line endings) untouched; the new normalization collapses them first before re-expanding to the target ending. The new regression test in LineEndingConverterTests covers this case well.

---

Method Name Mismatch: NormalizeMixedPowerShellLineEndings (ModuleBuildPipeline.cs)

The method operates on all staged PowerShell files (force: true, onlyMixed: false), not only files with mixed endings. The name is misleading. Consider renaming to NormalizeStagedPowerShellLineEndings or changing the options to match the name.

Additionally, the method hard-codes CRLF as the target. For maintainers working cross-platform, a comment explaining why CRLF is the right default (Windows PowerShell 5.1 compatibility) would help future readers.

---

skipDependenciesCheck: true Behavioral Change (ModulePublisher.cs)

This swaps PSResourceGet's built-in dependency check for the new ValidateRequiredModulesForRepositoryPublish custom check. The custom check is more nuanced (correctly skips ExternalModuleDependencies), but the trade-offs are:

• Adds a live network call per required module (2-minute timeout each), which can slow publish significantly for modules with many dependencies.
• If all required modules are external (all skipped), the custom validation passes with no network calls, but PSGallery's server-side validation will still check RequiredModules in the manifest. See the ExternalModule mirroring note below for a related concern.

The version matching logic in DoesVersionMatchRequiredModule is correct: exact, minimum, range, and unparseable versions are all handled. Using prerelease: true in Find is the right call to avoid missing prerelease-only versions in the repository.

---

ExternalModule -> RequiredModules Mirroring (ModulePipelineRunner.Plan.cs)

This is a meaningful behavioral change: external modules now appear in RequiredModules in the published manifest. The pre-publish validation correctly skips them. However, PSGallery's server-side validation may reject a module whose manifest lists a RequiredModule entry that does not exist in PSGallery (e.g., inbox PowerShell modules like Microsoft.PowerShell.Utility). It would be worth documenting which scenario prompted this change and confirming PSGallery server behavior with these entries before merging.

---

ComputeExcludedLibraries Cascade Fix (ModuleBuilder.cs)

Good bug fix. The previous BFS propagated exclusions through any dependency of a PowerShell runtime library, regardless of whether the dependency itself was a PS library. For example, System.Text.Json pulled in by a PS SDK would have been incorrectly excluded. Now only deps that are themselves PS runtime libraries continue the BFS. The test ComputeExcludedLibraries_DoesNotCascadeIntoNonPowerShellDependencies covers this well.

---

IncludeTopLevelBinaryFiles Applied to deps.json Success Path (ModuleBuilder.cs)

Previously, if TryCreateCopyPlanFromDeps succeeded, top-level binary files were not added on top of it. Now they always are. The test CopyPublishOutputBinaries_IncludesTopLevelFallbackFiles_WhenDepsMissesThem validates the intent. This is a behavior change worth calling out explicitly in the PR description since it affects which files end up in the module output for binary modules.

---

shouldPatchRequiredModules Semantics (ModulePipelineRunner.Run.cs)

The change from checking manifestRequiredModules is { Length: > 0 } to plan.RequiredModules is { Length: > 0 } intentionally allows writing an empty RequiredModules array to the manifest when all entries are filtered out by ApprovedModules. The commit message ("clear stale RequiredModules after merge filtering") confirms the intent. Looks correct.

---

RefreshPSD1Only / RefreshManifestOnly

Well-scoped feature. When enabled, the plan phase explicitly zeros out all non-manifest activities (sign, install, merge, docs, artefacts, publishes). The test suite covers both the plan and run phases. One minor note: the Run path guards are scattered as plan.BuildSpec.RefreshManifestOnly conditionals across multiple spots — if this flag gains more callers, consolidating to an early-return pattern at the top of Run might be cleaner, though the current approach is functional.

---

Test Coverage

Good additions:

• LineEndingConverterTests — covers the \r-only normalization
• ModuleBuilderDependencyCopyTests — covers the cascade fix and fallback copy behavior
• ModulePipelineRefreshManifestOnlyTests — covers plan and run for RefreshPSD1Only
• ModulePublisherRequiredModulesTests — covers version matching edge cases
• ModulePipelineApprovedModulesTests, ModulePipelineRegressionParityTests — regression coverage

The use of reflection to test ComputeExcludedLibraries (private static) is acceptable given it is a critical internal algorithm. If this pattern becomes common, making the method internal with InternalsVisibleTo would be a cleaner alternative.

---

Scope Note

The PR title focuses on the net8 packaging fix, but the actual scope is considerably broader: new features (pre-publish validation, RefreshPSD1Only, ExternalModule mirroring, editorial card defaults, post-nav helper, taxonomy links) plus several bug fixes and docs regeneration. For future reviews, splitting feature additions from bug fixes would make bisecting regressions easier.

---

Summary: The net8 fix and dependency cascade fix are clearly correct. The pre-publish validation is a solid addition with good unit tests. The main areas worth a follow-up discussion before merging are:

1. ExternalModules mirrored into RequiredModules — confirm PSGallery server-side acceptance for inbox modules listed as RequiredModules
2. Per-module Find network calls in ValidateRequiredModulesForRepositoryPublish — consider whether parallelism or a configurable timeout cap is needed for modules with many dependencies

[PrzemyslawKlys]

Added fix for repository publish packaging in 4beb3b0.

Issue:

• publish used raw staging root, which could include repo content (Sources, .github, tests, etc.)

Fix:

• repository publish now stages a module-only temporary package via ArtefactBuilder.CopyModulePackageForInstall(...)
• publishes that curated folder instead of full staging root
• temp publish folder is cleaned up after publish
• pass includeScriptFolders: !mergedScripts from pipeline runner for parity with artefact/install behavior

Tests:

• new ModulePublisherPackagingTests verifies module-only content and script-folder behavior.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-structured PR with clear intent, good test coverage for the key changes, and properly updated lock files. The fixes are targeted and the new RefreshManifestOnly feature has a solid test harness. A few observations below.

---

Core bug fix — System.IO.Packaging per-TFM split

PSPublishModule/PSPublishModule.csproj and PowerForge/PowerForge.csproj

Splitting the combined ItemGroup into two TFM-specific conditions is the right approach. Using System.IO.Packaging 10.0.1 under a net8.0 host would load an assembly compiled for a newer runtime, causing the import failure described in the summary. The fix is correct.

One thing to be aware of: when net9.0 support is added later, a third ItemGroup will need to be added. A comment on these blocks (e.g. "version must match the target framework major") would help future maintainers avoid re-introducing the same issue.

---

LineEndingConverter.cs — legacy \r-only handling

// before
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n");

// after
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n").Replace("\r", "\n");

The fix is correct. The old code silently left standalone \r bytes in place for both CRLF and LF targets. The new three-pass approach (normalize \r\n → \n, normalize \r → \n, then optionally convert \n → \r\n) correctly handles classic Mac CR-only files. The new LineEndingConverterTests validates this path.

---

ModuleBuilder.cs — dependency exclusion cascade

ComputeExcludedLibraries BFS early-exit:

foreach (var depKey in deps)
{
+    var depId = GetLibraryIdFromTargetKey(depKey);
+    if (!IsPowerShellRuntimeLibraryId(depId)) continue;
+
    if (excluded.Add(depKey)) queue.Enqueue(depKey);
}

This prevents non-PS dependencies from being dragged into the exclusion set, which caused the System.IO.Packaging assembly (and potentially other packages) to be omitted from module output directories. The ModuleBuilderDependencyCopyTests clearly demonstrates the regression.

CreateCopyPlan — IncludeTopLevelBinaryFiles in both paths:

if (plan is not null)
{
    IncludeTopLevelBinaryFiles(plan.RootFileNames, publishDir);
    return plan;
}

The intent is to catch binaries that exist in the publish directory but are absent from the deps.json manifest (common for projects that don't fully populate the deps file). Since RootFileNames is a HashSet<string>, duplicates are automatically deduplicated.

Potential concern: this will now include every top-level binary file that passes IsBinaryFileName and isn't in AlwaysExcludedRootFiles, even if the deps.json author intentionally omitted it (e.g. a build artifact that shouldn't ship). The fallback behavior was already doing this, but now it applies on top of a successful deps.json parse too. Is there a scenario where a binary should be intentionally excluded? If so, an opt-out would be useful. Otherwise, the current approach looks correct for the described problem.

---

RefreshManifestOnly / RefreshPSD1Only

The plumbing looks correct: the flag propagates from BuildModuleConfiguration.RefreshPSD1Only → refreshPsd1Only in Plan() → ModuleBuildSpec.RefreshManifestOnly → guarded in BuildInStaging and Run. The Plan-side disabling of merge, sign, install, artefacts, publishes, docs, and tests is comprehensive.

One gap: testsBeforeMerge is not cleared when refreshPsd1Only is true. The current code clears testsAfterMerge but pre-merge tests remain scheduled. If this is intentional (e.g. smoke-test the module even during a manifest-only refresh) it should be documented. If not intentional, testsBeforeMerge.Clear() is missing.

shouldPatchRequiredModules change in Run.cs:

// before: only wrote RequiredModules if the resolved output was non-empty
if (manifestRequiredModules is { Length: > 0 })

// after: writes (potentially empty) RequiredModules if the plan *configured* any
var shouldPatchRequiredModules = plan.RequiredModules is { Length: > 0 };
if (shouldPatchRequiredModules)

This is intentional: when all configured required modules are filtered out by ResolveOutputRequiredModules (e.g. all are approved-only modules that got rejected), the empty array must still be written to clear any stale entries from the manifest. The new test Run_ClearsStaleRequiredModules_WhenAllConfiguredRequiredAreFilteredByMergeMissing covers this case. Good.

---

NormalizeMixedPowerShellLineEndings in staging

The method always targets LineEnding.CRLF and preferUtf8BomForPowerShell: true. The CRLF default is sensible for PowerShell module distribution but could be surprising for cross-platform consumers. If LF normalization is ever needed (e.g. Linux-hosted build agents distributing to Linux targets), this would need to be configurable. Low priority given the current target audience, but worth noting.

---

ManifestEditor.PsData.cs — TryGetPsDataStringArray

Clean addition. The unused tokens variable from Parser.ParseFile is the standard pattern for AST-only analysis and is not a bug. The fallback handling of single-string values (StringConstantExpressionAst, ConstantExpressionAst) is consistent with how other manifest readers handle optional array-or-string polymorphism.

---

External module mirroring into RequiredModules

// External dependencies are mirrored into RequiredModules so Import-Module honors
// runtime prerequisites, but not into RequiredModulesForPackaging (so artefacts
// do not bundle inbox/platform modules).

The comment clearly states the intent. The implementation (upsert into requiredModulesDraft) correctly handles duplicates.

---

WebVerifyPolicy.cs — code-prefix based warning classification

Switching from text-prefix matching to structured warning codes is the right direction. The backward-compatible fallback to legacy text patterns ensures old-format warnings still classify correctly during any transition period.

---

Miscellaneous

• The whitespace-only changes (trailing space removal, alignment) in ModulePipelineRunner.Plan.cs are mixed with functional changes, which makes reviewing the functional diff harder. Consider separating cosmetic and functional changes in future PRs.
• GetLibraryIdFromTargetKey returns key (possibly null) when IsNullOrWhiteSpace(key) is true. In practice the callers always pass lib.Name from a JsonElement.EnumerateObject() which is never null, so this is a non-issue at runtime. Returning string.Empty in that branch would be more explicit.
• Lock file updates look consistent with the package version changes.
• Module docs regeneration is a clean generated-output update.

---

Summary: The core bug fixes (TFM-specific packaging version, CR-only line ending handling, dependency exclusion cascade) are well-reasoned and tested. The RefreshManifestOnly feature is comprehensive. The main items worth discussing before merge are: (1) whether testsBeforeMerge should also be cleared in refresh-only mode, and (2) whether the always-include-top-level-binaries behavior on top of a successful deps.json parse could over-include in any realistic scenario.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a solid, well-motivated PR with good test coverage. The System.IO.Packaging version split is the correct root-cause fix, and the dependency cascade fix in ComputeExcludedLibraries addresses a subtle but real over-exclusion bug. A few items worth discussing:

---

Bugs / Correctness

CreateCopyPlan — top-level binary inclusion is now unconditional

// PowerForge/Services/ModuleBuilder.cs
if (plan is not null)
{
    IncludeTopLevelBinaryFiles(plan.RootFileNames, publishDir);  // <-- always runs now
    return plan;
}

Previously, IncludeTopLevelBinaryFiles only ran in the fallback path (when no deps.json plan could be built). Now it runs even when a valid deps plan exists. This means every top-level binary that passes IsBinaryFileName and is not in AlwaysExcludedRootFiles gets added — even if the deps-resolved plan already accounts for the module layout. This could silently package extra assemblies (e.g., test helpers or tooling binaries that end up in the publish output) into modules that have a correct deps.json. The test CopyPublishOutputBinaries_IncludesTopLevelFallbackFiles_WhenDepsMissesThem validates the new behavior, but it does not verify that files already covered by the deps plan are not double-counted or over-included. Is this unconditional inclusion intentional for all cases?

---

ComputeExcludedLibraries cascade — correct fix, but verify the intent

foreach (var depKey in deps)
{
    var depId = GetLibraryIdFromTargetKey(depKey);
    if (!IsPowerShellRuntimeLibraryId(depId)) continue;  // new guard
    if (excluded.Add(depKey)) queue.Enqueue(depKey);
}

The guard prevents cascade into non-PowerShell libraries, which is the right fix. The test ComputeExcludedLibraries_DoesNotCascadeIntoNonPowerShellDependencies covers the primary regression. One edge case not tested: a transitive dependency that is a PowerShell runtime ID but is only pulled in through a non-PowerShell package (e.g., a custom package that re-exports Microsoft.PowerShell.SDK). In practice this is unlikely, but worth noting.

---

Code Quality

NormalizeMixedPowerShellLineEndings — name does not match behavior

The method is called NormalizeMixed... but it uses onlyMixed: false and force: true, meaning it normalizes all PowerShell files, not only those with mixed endings. This will convert intentionally LF-only source files (common in cross-platform/Linux PowerShell development) to CRLF on every staging run. Consider either:

• Renaming to NormalizeStagedPowerShellLineEndings to reflect actual behavior, or
• Using onlyMixed: true if the intent is only to fix files that have inconsistent endings.

TryGetPsDataStringArray — unused variable

Token[] tokens;  // declared but never read
ParseError[] errors;
var ast = Parser.ParseFile(filePath, out tokens, out errors);

tokens is declared as an out parameter but never used after parsing. Use a discard (out _) to be explicit:

var ast = Parser.ParseFile(filePath, out _, out errors);

GetLibraryIdFromTargetKey — minor: prefer AsSpan / IndexOf with StringComparison

var slash = key.IndexOf('/');
return slash > 0 ? key.Substring(0, slash) : key;

String.IndexOf(char) without StringComparison is fine for / (ordinal), but if this were a string literal it would be worth noting. As-is this is acceptable.

---

EditorialPostNav — AllItems fallback

var sourceItems = _context.AllItems.Count > 0
    ? _context.AllItems
    : _context.Items;

If AllItems is not populated by the caller and Items is a paginated subset, prev/next navigation will be computed against only the current page's items, producing incorrect or missing links for posts on page 2+. The reliability of this helper depends entirely on the rendering context populating AllItems. It may be worth asserting or logging a warning when AllItems is empty but pagination is active.

---

Minor / Nits

• Whitespace-only changes in ModulePipelineRunner.Plan.cs (trailing spaces removed from several bool/var declarations) add noise to the diff with no functional effect.
• Possible values: (empty) entries added to all Module/Docs/*.md parameter tables — these are fine for parameters with no enum constraint, but the trailing space after the colon is cosmetically inconsistent. Worth normalizing the doc generator to omit the line entirely when there are no values, rather than emitting an empty entry.
• The refreshPsd1Only local variable in Plan() is set correctly but there is no corresponding reset of mergeModule/signModule/installEnabled in the existing segment loop; those overrides happen in a post-processing block. Make sure there is no ordering dependency where a later segment could re-enable merge/sign after RefreshPSD1Only has been set.

---

Test Coverage

The new test files provide good coverage of the happy paths:

• LineEndingConverterTests — covers \r-only (classic Mac) normalization. A test for a file that is already CRLF with force: true would confirm no unintended re-writes occur.
• ModuleBuilderDependencyCopyTests — covers both the cascade fix and the new unconditional top-level inclusion behavior.
• ModulePipelineRefreshManifestOnlyTests — Plan_RefreshPSD1Only_DisablesNonManifestPhases is a solid behavioural assertion.

---

Summary

Area	Verdict
System.IO.Packaging version split	Correct and necessary fix
ComputeExcludedLibraries cascade guard	Correct fix; minor edge case not tested
CreateCopyPlan unconditional top-level inclusion	Needs confirmation of intent
NormalizeMixedPowerShellLineEndings naming/behavior	Minor naming inconsistency
TryGetPsDataStringArray unused tokens	Minor — use discard
EditorialPostNav AllItems fallback	Correctness risk on paginated collections
Test coverage	Good; a few gap cases noted above

Overall the PR is ready to merge pending confirmation on the CreateCopyPlan behavior change intent and the line-ending normalization scope. The core packaging fix is clean and well-validated.

[claude]

PR Review — fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a solid PR with a clear root-cause fix, good test coverage, and several useful refactors. Below are my observations, split by category.

---

Core Bug Fix — System.IO.Packaging TFM Split ✅

PSPublishModule/PSPublishModule.csproj and PowerForge/PowerForge.csproj

The fix is correct. Splitting the single Or-conditioned <ItemGroup> into two separate TFM-specific groups ensures net8.0 gets System.IO.Packaging 8.0.1 and net10.0 gets 10.0.1. Loading a net10 assembly into a net8 runtime is a clear assembly-binding failure, and this is the right resolution.

The lock files are updated accordingly — good; locked restore would otherwise reject the version change.

---

Line Ending Normalization ✅

PowerForge/Services/LineEndingConverter.cs

// Before
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n");

// After
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
return text.Replace("\r\n", "\n").Replace("\r", "\n");

Adding the .Replace("\r", "\n") middle step is correct — without it, lone \r (classic Mac line endings) pass through unchanged, which then get converted to \r\r\n by the final step (a bug). The fix handles all three cases (CRLF, LF, CR) before emitting the target.

The new LineEndingConverterTests.cs directly validates this fix with a CR-only file. Good regression anchor.

---

ModuleBuilder.cs — Dependency Copy Refactor ✅

Extracting IncludeTopLevelBinaryFiles and GetLibraryIdFromTargetKey as static helpers is a clean improvement. The important behavioral fix here is:

if (plan is not null)
{
    IncludeTopLevelBinaryFiles(plan.RootFileNames, publishDir); // <-- now added for deps path too
    return plan;
}

Previously the IncludeTopLevelBinaryFiles walk only ran for the fallback path (no deps.json). Files present in publishDir but not in the deps.json graph would be silently dropped. The fix applies the same top-level binary scan regardless of which path is taken — ModuleBuilderDependencyCopyTests covers this regression.

The IsPowerShellRuntimeLibraryId guard in the BFS walk (preventing over-exclusion of non-PS transitive deps) also looks correct, though I'd like to see a comment explaining why non-PS deps shouldn't be excluded transitively — it's a subtle invariant.

---

Concerns / Issues

1. Exception swallowing in NormalizeMixedPowerShellLineEndings

The staging normalization wraps the LineEndingConverter call in a catch-all with a warning log. While a best-effort approach makes sense to avoid blocking the entire build over a normalization hiccup, silently continuing with un-normalized files could mask real issues (encoding errors, file locks, permission problems). Consider logging the exception type and message at minimum, and whether warnings are surfaced visibly enough in CI output.

2. skipDependenciesCheck flipped to true in ModulePublisher.cs

// Before: false
// After:  true
skipDependenciesCheck: true

This removes the native PSResourceGet dependency pre-check in favour of the new ValidateRequiredModulesForRepositoryPublish. The new method looks thorough, but this is a non-trivial behavior change: if ValidateRequiredModulesForRepositoryPublish has a bug or an edge case (e.g. a Find() call throws, or a version comparison is off), the fallback native check no longer exists. Worth a comment in the code explaining the rationale, and ensuring ValidateRequiredModulesForRepositoryPublish is covered by tests.

3. Temp directory hygiene in PrepareModulePackageForRepositoryPublish

The temp path is Path.GetTempPath()/PowerForge/publish/<some-subfolder>. If a prior run crashed before the finally block (e.g. process kill, OOM), leftover directories accumulate. Consider either:

• Using a GUID in the subdirectory name (so each run gets a unique path), or
• Deleting any pre-existing directory of the same name before copying into it.

The CleanupTemporaryPublishPath method is best-effort, which is fine, but stale temp dirs from failed runs could build up on long-lived agents.

4. ManifestEditor.Helpers.cs — RemoveKeyValue silent no-op

RemoveKeyValue returns false when the content would be unchanged. Callers in ModulePipelineRunner.ManifestRefresh.cs using SetOrRemoveTopLevelString / SetOrRemovePsDataString silently succeed even if the removal did not actually happen (key not found). If a manifest field is meant to be cleared and the key is present but the removal silently no-ops (e.g. due to AST parse issues), the field remains set. Consider logging at Debug level when a removal is attempted but skipped.

5. SyncRefreshOnlyOutputsToProjectRoot — path identity check

The method copies staging files back to the project root. If staging == projectRoot (unusual, but possible in certain monorepo or flat-layout configurations), this becomes a self-copy. File.Copy(src, src, overwrite: true) on the same path is either a no-op or an error depending on platform. A guard like if (string.Equals(src, dst, StringComparison.OrdinalIgnoreCase)) continue; would be defensive.

---

Minor Observations

• ManifestEditor.Helpers.cs — InsertKeyValue newline guard: The needsLeadingNewLine check (content[closeIndex - 1] is not \r or \n) is correct for the bug described. No issue.
• ModulePipelineRunner.ManifestRefresh.cs: Extracting manifest update logic into its own partial class file is a clear improvement for maintainability. The NormalizeStringArray helper is a good touch.
• ModulePipelineRunner.Plan.cs: The ExternalModuleDependencies → RequiredModules mirroring for runtime import-module correctness is noted; ensure this doesn't produce duplicates if the user also specified those modules explicitly in RequiredModules.
• Module docs (Module/Docs/*.md): The regenerated docs add Possible values: lines — many are blank (for SwitchParameter, free-form String, etc.). This is fine for generated output; just confirm the generator correctly populates enum/ValidateSet values where applicable (e.g. ProjectType, TargetEncoding).
• Docs/PowerForge.Web.WarningCodes.md: Good addition as a canonical reference. The PFWEB.GITSYNC.SECURITY entry noting inline token risk is a nice security-practice nudge.

---

Test Coverage Summary

New Test File	What It Covers
LineEndingConverterTests.cs	CR-only → CRLF normalization regression
ModuleBuilderDependencyCopyTests.cs	Excluded library BFS, top-level fallback copy
ModulePipelineRefreshManifestOnlyTests.cs	Plan phase gating, install/publish skip, project root sync
ModulePipelineRegressionParityTests.cs	Extended regression suite (509 → 648 lines)

Coverage looks appropriate for the changes introduced. The ModulePublisher.cs validation additions (ValidateRequiredModulesForRepositoryPublish and friends) are internal static for testability — worth adding unit tests for DoesVersionMatchRequiredModule edge cases (exact match, wildcard, range).

---

Summary

The root cause fix is correct and clearly targeted. The refactors are well-scoped and improve maintainability. The main items worth addressing before merge:

1. Log exception details in the NormalizeMixedPowerShellLineEndings catch block.
2. Document (in code) the rationale for skipDependenciesCheck: true.
3. Use unique temp paths in PrepareModulePackageForRepositoryPublish to avoid stale-dir accumulation.
4. Add unit tests for ValidateRequiredModulesForRepositoryPublish / DoesVersionMatchRequiredModule edge cases.

[claude]

PR Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a solid, well-scoped PR with real bug fixes, meaningful refactoring, and good test coverage. Below are observations grouped by area.

---

System.IO.Packaging version split (core fix)

The split of the shared ItemGroup condition into two separate, framework-specific groups is the right approach:

<!-- net8.0 -->
<PackageReference Include="System.IO.Packaging" Version="8.0.1" />
<!-- net10.0 -->
<PackageReference Include="System.IO.Packaging" Version="10.0.1" />

The original single condition net8.0 Or net10.0 resolving to 10.0.1 was the root cause of the import failure described in the PR notes. This fix is correct and the lock files updated accordingly.

---

LineEndingConverter.NormalizeEndings — bug fix

The fix to handle lone \r (old Mac CR-only line endings) is correct:

// Before: missed lone \r
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
// After: normalizes \r\n → \n, then \r → \n, then \n → \r\n
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");

The three-step chain is the idiomatic safe approach. Good fix.

---

NormalizeMixedPowerShellLineEndings — design note

The staging normalization always forces CRLF (target: LineEnding.CRLF, force: true). A few observations:

1. force: true converts all PS files, even those already in CRLF — using onlyMixed: true instead would be more targeted and avoid unnecessary writes. If the intent is truly to enforce CRLF unconditionally, a comment explaining why (e.g., "Windows PowerShell compatibility") would help future readers.

2. The broad catch (Exception ex) that logs a warning and continues is reasonable for a non-critical normalization step, but a file that fails silently here could ship with inconsistent line endings. Consider logging the failing file path (available from the result object) for easier debugging.

3. The method hardcodes an extra excludes set that duplicates some of the caller-supplied excludedDirectoryNames. This is defensively fine, but the overlap (e.g., .git, bin, obj) could be confusing — consider a comment noting that these defaults augment, not replace, the caller set.

---

CreateCopyPlan / IncludeTopLevelBinaryFiles refactor

The extraction of IncludeTopLevelBinaryFiles into a shared helper is clean. The behavioral change — calling it for both the deps plan and the fallback — is the important correctness fix here: previously, when a deps.json plan existed, top-level binaries not listed in the JSON were silently skipped.

Worth double-checking: are there scenarios where a top-level binary correctly excluded by the deps JSON (e.g., a PS host assembly) might now be inadvertently re-included by IncludeTopLevelBinaryFiles? The AlwaysExcludedRootFiles guard and IsBinaryFileName filter should prevent this, but an explicit test covering that case would add confidence.

---

ComputeExcludedLibraries — transitive dependency traversal fix

The added early-continue guard:

var depId = GetLibraryIdFromTargetKey(depKey);
if (!IsPowerShellRuntimeLibraryId(depId)) continue;
if (excluded.Add(depKey)) queue.Enqueue(depKey);

Correctly restricts BFS traversal to PS-runtime library subtrees. Previously, the BFS would enqueue and traverse all transitive dependencies of any PS-runtime library, potentially marking unrelated assemblies as excluded. This is a meaningful correctness fix for modules with complex dependency graphs.

---

ManifestEditor.InsertKeyValue — leading newline fix

The needsLeadingNewLine check prevents spurious blank lines when the closing } is already on a new line:

var needsLeadingNewLine = closingPos > 0 && content[closingPos-1] != '\r' && content[closingPos-1] != '\n';
var insertText = (needsLeadingNewLine ? NewLine : string.Empty) + indent + key + " = " + valueExpression + NewLine;

Correct and safe. The check at closingPos-1 won't underflow because closingPos > 0 is guarded first.

---

RefreshManifestOnly / RefreshPSD1Only flow

The new pipeline mode is a good feature addition and the refactoring is well-structured:

• ModulePipelineRunner.ManifestBaseline.cs — reads existing .psd1 as baseline (seeds defaults)
• ModulePipelineRunner.ManifestRefresh.cs — centralises all manifest writes
• ModulePipelineRunner.RefreshOnlySync.cs — copies outputs back to project root

One concern: SyncRefreshOnlyOutputsToProjectRoot copies .psd1, .psm1, and formatting-changed .ps1 files from staging back to the project root. If the staging directory contains files that were modified by an earlier pipeline step (e.g., line-ending normalization), these normalized versions will overwrite the project source files. This is probably intentional, but worth a comment in the method to clarify the expected side effects.

---

Test coverage

The new test files are well-structured and cover the main scenarios:

• LineEndingConverterTests.cs — CR/LF normalization (new)
• ModuleBuilderDependencyCopyTests.cs — dependency copy behaviour (new)
• ModulePipelineRefreshManifestOnlyTests.cs — RefreshManifestOnly plan + run (new)
• ModulePipelineManifestRefreshTests.cs — manifest refresh (new)
• ModulePublisherPackagingTests.cs / ModulePublisherRequiredModulesTests.cs — packaging (new)

Temp directory cleanup via finally { root.Delete(recursive: true) } is consistent throughout.

One note: WriteMinimalModule in ModulePipelineRefreshManifestOnlyTests.cs uses Environment.NewLine for the .psd1 content, which will produce \r\n on Windows and \n on Linux. If any test subsequently checks for specific line endings in the output manifest, results may differ across CI environments. If that is not a concern currently, it's worth being aware of as the test suite runs cross-platform.

---

Docs

The new Docs/PowerForge.Web.WarningCodes.md catalog is a useful reference and the cross-references added to the existing pipeline/quality-gates docs are consistent. The Module/Docs/ regenerated files have Possible values: lines added for enum-typed parameters (correctly populated) and left blank for string/switch parameters (appropriate).

---

Summary

Area	Verdict
Core packaging fix (System.IO.Packaging versioning)	Correct
LineEndingConverter lone-\r fix	Correct
ManifestEditor leading-newline fix	Correct
ComputeExcludedLibraries BFS scope fix	Correct
CreateCopyPlan / top-level binary inclusion	Correct, but a test for PS-host exclusion would help
RefreshManifestOnly pipeline mode	Well-structured; minor sync side-effect worth documenting
NormalizeMixedPowerShellLineEndings	Works, but onlyMixed and per-file error reporting worth considering
Test coverage	Good — new files cover the key new paths
Security	No concerns

No blocking issues. The suggestions above are either minor polish or documentation improvements.

[claude]

PR #155 Review: fix(build): stabilize net8 packaging resolution and module build pipeline

Good work on this PR. The scope is broad (7,000+ net lines) but the changes are logically cohesive. Below are my observations, grouped by severity.

---

Correctness / Bugs

1. CreateCopyPlan behavior change — top-level binaries now always included when deps plan succeeds

In ModuleBuilder.cs, the old code used top-level file enumeration only as a fallback when TryCreateCopyPlanFromDeps returned null. The new code always calls IncludeTopLevelBinaryFiles even when the deps plan succeeded. This means any top-level binary not listed in the .deps.json will now always be included. The test CopyPublishOutputBinaries_IncludesTopLevelFallbackFiles_WhenDepsMissesThem explicitly validates this intent, but the change could silently include unwanted assemblies for projects that relied on the deps-only filter being strict. A comment explaining why both paths include top-level files would help future readers.

2. NormalizeMixedPowerShellLineEndings — unconditional CRLF conversion on every staging

NormalizeMixedPowerShellLineEndings is called unconditionally in StageToStaging with force: true and onlyMixed: false. This converts all PowerShell files to CRLF on every build, including files that were originally LF-only (authored on Linux/macOS). There is no way to opt out without modifying source. For cross-platform projects this could produce unexpected diffs and break git hygiene checks. Consider making this conditional on a FileConsistency configuration segment or a NormalizeLineEndingsInStaging flag on BuildModuleConfiguration. The onlyMixed: true mode already exists and would be a safer default.

3. ModulePipelineRunner.RefreshOnlySync.cs — path prefix comparison case sensitivity

if (!sourcePath.StartsWith(sourcePrefix, StringComparison.OrdinalIgnoreCase))
    continue;

On Linux (case-sensitive) filesystems this could silently skip valid files when casing doesn't match exactly. Since both paths derive from Path.GetFullPath, they should match in practice, but this is masking a latent issue. Prefer StringComparison.Ordinal or use Path.GetRelativePath (available since .NET 5) which handles OS differences correctly.

---

Design Observations

4. Plan() now reads from disk — impact on reproducibility

TryReadProjectManifestBaseline is called during Plan(), meaning the plan is no longer a pure function of its ModulePipelineSpec. In CI systems with a fresh checkout (no pre-existing PSD1), the baseline returns null and the pipeline behaves differently than a local re-run with a pre-existing manifest. This is a reasonable trade-off for RefreshManifestOnly, but deserves a comment in Plan() noting the intentional stateful read.

5. skipDependenciesCheck: true in PSResourceGet

Changed from false to true. This disables PSResourceGet's built-in dependency verification in favor of the custom ValidateRequiredModulesForRepositoryPublish. The custom check is more capable (handles RequiredVersion/ModuleVersion/MaximumVersion ranges), but it permanently bypasses any future PSResourceGet validation improvements. Documenting the rationale in a code comment would help future maintainers.

6. ValidateRequiredModulesForRepositoryPublish — sequential network calls per dependency

Each required module triggers a separate _psResourceGet.Find() call with a 2-minute timeout. For a module with 5+ required modules against a slow or rate-limited feed this validation could add significant time to the publish step. Consider batching multiple module names into a single Find call if the PSResourceGet API supports it, or reducing the per-call timeout.

7. Duplicate normalization helpers across partial classes

NormalizeArray (in ManifestBaseline.cs) and NormalizeStringArray (in ManifestRefresh.cs) are functionally identical. Since both are private static in the same partial class, one should be removed and callers updated. The ManifestRefresh.cs version with IEnumerable<string>? input is more general and should be kept.

8. External modules mirrored into RequiredModules — undocumented behavior change

The new code in Plan.cs now mirrors external module dependencies into requiredModulesDraft (comment: "Legacy behavior compatibility: external dependencies are mirrored into RequiredModules so Import-Module honors runtime prerequisites"). This will change the generated RequiredModules array in manifests for any module that previously listed external dependencies without also listing them as required modules. This warrants a CHANGELOG note so consumers know to expect a PSD1 diff after upgrading.

---

Minor / Nits

9. TryRemovePsDataKey / TryRemovePsDataSubKey — double file read

Both methods call File.ReadAllText(filePath) and then Parser.ParseFile(filePath, ...), which internally re-reads the file. This is consistent with the existing pattern in other ManifestEditor methods, but consider passing the content string to Parser.ParseInput instead of ParseFile to halve the I/O.

---

Test Coverage

The new tests are well-structured and cover the key scenarios:

• LineEndingConverterTests — CR-only file normalization ✓
• ModuleBuilderDependencyCopyTests — BFS cascade fix + fallback inclusion ✓
• ModulePipelineRefreshManifestOnlyTests — plan/run/sync round-trip for refresh-only path ✓
• ModulePublisherPackagingTests — clean publish layout ✓
• ModulePublisherRequiredModulesTests — version constraint matching ✓

One gap: no test for the unconditional CRLF normalization in StageToStaging against a LF-only source file (to document or challenge the behavior noted in item 2).

---

Summary

The core fixes (System.IO.Packaging per-TFM versioning, CR-only line ending handling, dependency cascade BFS fix) are correct and well-tested. The main risks worth addressing before merging are the unconditional CRLF normalization (item 2) and the path comparison case sensitivity in RefreshOnlySync (item 3). Items 4–8 are design observations that are acceptable as-is if documented.

---

Review generated by Claude Code

[claude]

Code Review — PR #155: fix(build): stabilize net8 packaging resolution and module build pipeline

Overall this is a well-structured PR with a clear primary fix, a meaningful new feature, and solid test coverage. Below is a detailed breakdown.

---

✅ Strengths

Core packaging fix is correct and clean.
Splitting the combined net8.0 Or net10.0 condition into two separate ItemGroup blocks in both .csproj files is the right approach. Previously, the net8.0 target was pulling System.IO.Packaging 10.0.1 (a net10.0 build), which caused the import failure. The fix is minimal and targeted.

RefreshPSD1Only feature is well-factored.
Breaking the manifest refresh logic into ManifestBaseline.cs, ManifestRefresh.cs, and RefreshOnlySync.cs partial classes follows the existing partial-class convention and keeps the pipeline readable. The RefreshManifestOnly flag is correctly threaded through the plan, pipeline, and runner.

Line ending fix is correct.
The fix in LineEndingConverter.NormalizeEndings now handles bare \r (CR-only) line endings:

// Before:
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\n", "\r\n");
// After:
if (target == "\r\n") return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");

The intermediate \r → \n pass is the correct approach. The corresponding LineEndingConverterTests validates exactly this scenario.

Bug fix: duplicate ExternalModuleDependencies write removed.
The old Run.cs had:

if (plan.ExternalModuleDependencies is { Length: > 0 })
    ManifestEditor.TrySetPsDataStringArray(..., "ExternalModuleDependencies", ...);
if (plan.ExternalModuleDependencies is { Length: > 0 })   // ← duplicate
    ManifestEditor.TrySetPsDataStringArray(..., "ExternalModuleDependencies", ...);

This is now correctly consolidated into RefreshManifestFromPlan.

ModuleBuilder BFS fix is important.
The old ComputeExcludedLibraries propagated exclusions transitively through all deps — including deps of PowerShell assemblies that are not themselves PowerShell host assemblies (e.g., System.Text.Json depended on by Microsoft.PowerShell.5.ReferenceAssemblies). The new guard:

var depId = GetLibraryIdFromTargetKey(depKey);
if (!IsPowerShellRuntimeLibraryId(depId)) continue;

correctly stops propagation at the boundary. The ModuleBuilderDependencyCopyTests covers this regression.

Test coverage is thorough.
New test files cover every significant behavioral change: CR line ending normalization, RefreshPSD1Only mode (plan-level and run-level), manifest refresh/cleanup semantics, packaging layout, required-modules version matching, and the BFS dep exclusion fix.

---

🔍 Issues and Observations

1. Duplicate NormalizeArray/NormalizeStringArray — DRY violation

ManifestBaseline.cs defines:

private static string[] NormalizeArray(string[] values)
{
    return (values ?? Array.Empty<string>())
        .Where(v => !string.IsNullOrWhiteSpace(v))
        .Select(v => v.Trim())
        .Distinct(StringComparer.OrdinalIgnoreCase)
        .ToArray();
}

ManifestRefresh.cs defines an identical method as NormalizeStringArray(IEnumerable<string>? values). The only difference is the parameter type (string[] vs IEnumerable<string>?). Since both live in the same partial class ModulePipelineRunner, these should be unified into a single private static helper in Utils.cs.

2. RefreshManifestFromPlan is called twice in Run.cs

The method is called once during the manifest step, and a second time after the formatting pipeline completes (the "post-format manifest patch"). For the ScriptsToProcess field specifically, this means: read → remove → re-add (call 1), then read → remove → re-add again (call 2). While functionally idempotent and tested, calling the same full manifest refresh twice is redundant. The original post-format patch only re-applied RequiredModules and ScriptsToProcess. Consider a targeted ReApplyPostFormatManifestFields that only re-writes the fields that formatting could have disturbed, rather than re-running the entire refresh.

3. Asymmetry between manifest-segment and non-manifest-segment paths

In RefreshManifestFromPlan, the hasManifestSegment branch uses removeWhenEmpty: true (nulls remove the key from the manifest), while the else branch uses removeWhenEmpty: false (empty values are silently skipped). This is intentional backward-compatibility behaviour but worth a comment, since the current code only documents what it does, not why the two paths behave differently.

4. InsertKeyValue trailing-blank-line regex scope

The refactored InsertKeyValue in ManifestEditor.Helpers.cs uses a regex to strip trailing blank lines before the closing }. If a PSD1 string value contains a literal \n\n followed by } (e.g., a multi-line description ending with a blank line), the regex could incorrectly modify the value content. This is unlikely in practice for manifest files, but the regex scope should be limited to the structural region after the last key-value pair and before the closing brace.

5. IncludeTopLevelBinaryFiles now called for deps-based plans too

Old behaviour: top-level binary inclusion was the fallback when no deps plan existed.
New behaviour: top-level binaries are always included via IncludeTopLevelBinaryFiles, even when a deps plan is present.

if (plan is not null)
{
    IncludeTopLevelBinaryFiles(plan.RootFileNames, publishDir);  // ← new
    return plan;
}
var fallback = new PublishCopyPlan();
IncludeTopLevelBinaryFiles(fallback.RootFileNames, publishDir);
return fallback;

This is covered by CopyPublishOutputBinaries_IncludesTopLevelFallbackFiles_WhenDepsMissesThem, but there is no negative test verifying that PowerShell host assemblies are not included when a deps plan exists. AlwaysExcludedRootFiles should guard this, but it's worth confirming the set contains all relevant assemblies (e.g., System.Management.Automation.dll).

6. ReadPsDataSingleString silently drops extra values

private static string? ReadPsDataSingleString(string manifestPath, string key)
{
    var values = ReadPsDataStringArray(manifestPath, key);
    if (values.Length == 0) return null;
    return values[0];   // remaining values silently dropped
}

Used for IconUri, ProjectUri, LicenseUri, Prerelease — all of which are semantically single-valued, so this is fine. A defensive log warning for values.Length > 1 would be a minor improvement.

7. Indentation inconsistency in Run.cs (pre-existing, not introduced)

The installResult block in Run.cs still has inconsistent indentation (8-space outer try, 12-space inner). This is carried over from the original code. Not a blocker, but it makes the install section slightly harder to scan.

8. No test for NormalizeMixedPowerShellLineEndings staging step

ModuleBuildPipeline.StageToStaging now calls NormalizeMixedPowerShellLineEndings after copying. This step swallows all exceptions as warnings. There are no tests that stage a module with a mix of CRLF/LF/CR files and assert the staging output is normalized. The LineEndingConverterTests covers the underlying converter, but not the integration path through StageToStaging.

9. Minor: mergedScripts ternary could be clearer

var mergedScripts = plan.BuildSpec.RefreshManifestOnly ? false : ApplyMerge(plan, buildResult);

ApplyMerge has side effects. The ternary makes it easy to miss that ApplyMerge is conditionally not called (not just returning false). A clearer form:

var mergedScripts = false;
if (!plan.BuildSpec.RefreshManifestOnly)
    mergedScripts = ApplyMerge(plan, buildResult);

---

📋 Documentation

• Docs/PowerForge.Web.WarningCodes.md is a welcome addition — having a single reference catalog for warning codes improves operator experience.
• The cross-references added to Pipeline.md, QualityGates.md, and AgentHandoff.md are consistent and well-placed.
• Module/Docs/*.md Possible values: additions are clearly auto-generated from cmdlet metadata. These are noise-free improvements.
• The EditorialCards collection-level defaults and editorial_post_nav helper documentation in ContentSpec.md are detailed and include good examples.

---

Summary

Area	Assessment
Core packaging fix	✅ Correct, minimal
LineEndingConverter fix	✅ Correct, tested
RefreshPSD1Only feature	✅ Well-structured, tested end-to-end
ManifestEditor additions	✅ Solid, minor edge-case note on regex scope
ModuleBuilder BFS fix	✅ Correct, regression test added
ModulePublisher changes	✅ Good, consider negative test for AlwaysExcludedRootFiles
Duplicate NormalizeArray	⚠️ Should be unified into one helper
Double RefreshManifestFromPlan call	⚠️ Consider narrowing the post-format re-apply
Test coverage	✅ Comprehensive for changed behaviour
Documentation	✅ Clear and well-referenced

The packaging fix is the critical path item here and it's solid. The feature additions are well-tested. The two items worth addressing before merge are the duplicate NormalizeArray/NormalizeStringArray and considering whether the double RefreshManifestFromPlan call is the right long-term shape for the post-format patch step.