Make net472 support real

[PrzemyslawKlys]

Summary

• make the shared PowerForge compatibility-sensitive services compile and run on net472 instead of fencing newer code away from the legacy target
• add a dedicated PowerForge.Net472SmokeTests project so the shared C# layer gets real .NET Framework runtime coverage in addition to PS 5.1 module coverage
• tighten the module test harness and CI so Windows PowerShell 5.1 and explicit net472 validation are both part of the normal build workflow

Validation

• dotnet build .\PowerForge\PowerForge.csproj -c Release -f net472 --nologo
• dotnet build .\PowerForge.PowerShell\PowerForge.PowerShell.csproj -c Release -f net472 --nologo
• dotnet build .\PSPublishModule\PSPublishModule.csproj -c Release -f net472 --nologo
• dotnet test .\PowerForge.Net472SmokeTests\PowerForge.Net472SmokeTests.csproj -c Release -f net472 --nologo
• powershell.exe -NoProfile -ExecutionPolicy Bypass -File .\PSPublishModule.Tests.ps1
• pwsh -NoProfile -ExecutionPolicy Bypass -File .\PSPublishModule.Tests.ps1

Notes

• a local Build-ModuleSelf.ps1 rerun still hit a machine-specific installed-module lock on PowerForge.dll from active VS Code / pwsh sessions, but the direct net472 builds and both PowerShell test hosts passed cleanly

[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: Make net472 support real

This is a well-structured PR that cleanly introduces real .NET Framework 4.7.2 compatibility across the shared C# layer. The central FrameworkCompatibility shim approach is the right call — it keeps #if NET472 guards centralised rather than scattered throughout service implementations.

---

What works well

• Central shim pattern (FrameworkCompatibility.cs): Consolidating all framework-conditional logic into one static class is the right architecture. It keeps service files readable and makes the compatibility surface easy to audit.
• IsWindows() semantics: Returning true unconditionally for NET472 is correct — .NET Framework only runs on Windows.
• GetSha256Hex: The manual SHA256.Create() fallback is a clean, correct replacement for GetCertHashString(HashAlgorithmName.SHA256).
• Git parsing fixes (GitClient.cs): Replacing StartsWith(char), range indexers ([n..]), and collection-expression Split overloads with net472-compatible equivalents is correct and thorough.
• Test harness refactoring (PSPublishModule.Tests.ps1): The Test-ModulePayloadUsableForCurrentHost function is a clear improvement over the previous fragile Lib-copy-and-cleanup approach. Making the installed-module fallback opt-in via PSPUBLISHMODULE_TEST_ALLOW_INSTALLED_FALLBACK is a good fail-fast default.
• CI additions: Placing the net472 build/test steps before the module build so failures are caught early is sensible ordering.

---

Issues and suggestions

1. FrameworkCompatibility.GetRelativePath — URI edge cases (minor)

// NET472 path
return Uri.UnescapeDataString(baseUri.MakeRelativeUri(targetUri).ToString())
    .Replace('/', Path.DirectorySeparatorChar);

Uri percent-encodes characters like #, ?, %, and spaces. Uri.UnescapeDataString will decode most of these back, but a path that literally contains a % (e.g. C:\My%20Stuff\file.cs) would be double-decoded: the %20 becomes a space, silently corrupting the path.

For a build/tooling context this is unlikely to cause real-world issues, but worth documenting with a comment:

// Note: this URI-based fallback does not round-trip correctly for paths that
// contain literal '%' characters on .NET Framework 4.7.2.

2. FrameworkCompatibility.ReadAsStreamAsync — cancellation gap (minor)

#if NET472
    cancellationToken.ThrowIfCancellationRequested();
    return content.ReadAsStreamAsync();
#else

Cancellation is only honoured at the call site, not once the HTTP read begins. This is a known limitation of the net472 HttpContent API, but it's worth a comment so future reviewers don't assume the cancellation token is fully respected:

// NET472: HttpContent.ReadAsStreamAsync does not accept a CancellationToken.
// Cancellation is only checked eagerly at the point of call.

3. NuGetPackageIdentity record → class — unnecessary conversion

// Before
private sealed record NuGetPackageIdentity(string Id, string Version);
// After
private sealed class NuGetPackageIdentity { ... }

record types are fully supported on net472 with LangVersion latest — the compiler synthesises the required System.Runtime.CompilerServices.IsExternalInit class automatically. This conversion is not required, and converts a succinct positional record into ~10 lines of boilerplate. Consider reverting if you want to keep the diff minimal.

Same applies to ProbeResponse: converting from readonly record struct to a plain mutable struct also removes the readonly invariant, which is a minor but real regression in safety.

4. ReleaseSigningHostSettingsResolver — semantic change in ModulePath

// Before
ModulePath = modulePath,          // could be null
// After
ModulePath = modulePath ?? string.Empty,  // never null

If ModulePath on ReleaseSigningHostSettings is declared as string?, callers may rely on the null-check pattern (if (settings.ModulePath is not null)). Changing it to an empty-string default is a semantic change. Ensure all consumers of ModulePath are prepared for an empty string where they previously expected null, especially if there is any path-joining logic that would produce a rooted path from an empty string.

5. Step-Version.Tests.ps1 — module loaded twice per test run

It 'Testing version 0.1.X' {
    $ModuleToLoad = if ($env:...) { ... } else { ... }
    Import-Module $ModuleToLoad -Force | Out-Null
    ...
}
It "Testing version 0.2.X" {
    $ModuleToLoad = if ($env:...) { ... } else { ... }
    Import-Module $ModuleToLoad -Force | Out-Null
    ...
}

Each It block re-imports the module independently. This is pre-existing, but since the test file was touched it would be worth moving the import into a BeforeAll block for consistency with the other test files (and to avoid repeated module loading overhead).

6. Build-ModuleSelf.ps1 — Sync-LocalModuleLib removed without replacement

Sync-LocalModuleLib was responsible for populating Module\Lib\Core and Module\Lib\Default from the build output. With it removed, $sourceLibRoot will never exist after a self-build, so Build-Module.ps1 will always fall through to the binary import path. This is intentional per the PR summary, but it means the source-manifest bootstrap path in Build-Module.ps1 is now essentially dead code for self-builds. A comment or, eventually, cleanup of that branch would help future readers understand why sourceLibRoot can still be referenced.

---

Summary

The approach is sound and the implementation is careful. The main actionable items are:

Priority	Item
Low	Document the %-in-path limitation in GetRelativePath
Low	Document the single-check cancellation limitation in ReadAsStreamAsync
Low	Consider reverting NuGetPackageIdentity and ProbeResponse to record types
Low	Verify ModulePath = "" vs ModulePath = null doesn't break consumers
Nit	Move module import to BeforeAll in Step-Version.Tests.ps1

Overall this is ready to merge once the author confirms the ModulePath semantic change is intentional.

Review generated with Claude Code

[claude]

PR Review: Make net472 support real

This is a well-structured PR that takes a methodical approach to adding real .NET Framework 4.7.2 support. The centralized FrameworkCompatibility class is a clean design decision. Here is detailed feedback across the key areas.

---

Code Quality and Best Practices

Strengths:

• Centralized compatibility layer (FrameworkCompatibility.cs) is the right pattern. Putting all #if NET472 guards in one place keeps service classes clean and makes the compatibility surface easy to audit and update.
• Step-Version.Tests.ps1 refactor correctly moves module import to BeforeAll instead of per-It block — this is a proper Pester pattern improvement.
• Environment variable injection (PSPUBLISHMODULE_TEST_MANIFEST_PATH) for pointing test files at the right module location is cleaner than the previous copy-then-cleanup approach.
• The Test-ModulePayloadUsableForCurrentHost helper in PSPublishModule.Tests.ps1 is a good encapsulation of the PSEdition + Lib subfolder detection logic.

Concerns:

• Record-to-class/struct conversions in PublishVerificationHostService.cs: sealed record has value-based equality and ToString() generated automatically. Converting to plain class/struct changes those semantics silently. If ProbeResponse or similar types are ever used in dictionaries, sets, or compared with ==, this is a latent bug. Consider adding explicit Equals/GetHashCode overrides or an XML comment noting the change.

• Nullable ! assertion density: The PR adds many ! null-forgiving operators after IsNullOrWhiteSpace checks. This is correct when the guard is always present, but fragile — a future refactor that moves or removes the guard turns the assertion into a hidden null dereference. Where feasible, prefer the FrameworkCompatibility.NotNull/NotNullOrWhiteSpace helpers (already introduced here) for consistency and safety.

---

Potential Bugs

1. URI-based GetRelativePath fallback edge cases

The implementation comment already acknowledges it does not round-trip literal % path segments on .NET Framework. Consider adding a test that covers a path containing a space (URI-encoded as %20) to confirm the behaviour is intentional and will not regress.

2. HttpContent.ReadAsStreamAsync cancellation is not mid-stream

The net472 shim calls cancellationToken.ThrowIfCancellationRequested() only once at entry, then delegates to the no-token overload. Cancellation is not honoured mid-stream. For small NuGet payloads this is probably fine, but a comment explaining this limitation would help future maintainers.

3. Response file cleanup on failure is untested

DotNetNuGetClientNet472SmokeTests asserts the response file is deleted after a successful push, but there is no test for the failure path. If production code skips cleanup on failure, an API key in a temp file could persist after a failed push. Verify the cleanup path covers failure and consider adding a test.

4. Get-ModulePayloadRootForTests throw on missing payload

The new throw is better than the old silent fallback to source root. The CI step ordering (build before test) is correct per the workflow YAML, so this should surface as a clear build failure rather than a cryptic test error.

---

Performance

No performance concerns. The compatibility shims are thin wrappers with no repeated allocations or loops. Removing Sync-LocalModuleLib from Build-ModuleSelf.ps1 eliminates a potentially large recursive file copy on every self-build, which is a net improvement.

---

Security

• API key in response file (see Bug 3): DotNetNuGetClient writes credentials to a temp file. On Windows, %TEMP% is user-scoped so this is generally acceptable, but worth confirming the file is deleted even on failure so credentials cannot linger.
• PSPUBLISHMODULE_TEST_ALLOW_INSTALLED_FALLBACK=1: Gating the installed-module fallback behind an opt-in env var is a good security/reproducibility practice.
• FrameworkCompatibility.IsWindows() always returns true: Correct since net472 is Windows-only, but worth a short XML doc comment so readers do not mistake it for a bug.

---

Test Coverage

What is covered well:

• Path normalisation (ArtefactConfigurationFactoryNet472SmokeTests)
• Response-file lifecycle for NuGet push (DotNetNuGetClientNet472SmokeTests)
• Git status output parsing with CRLF and branch counts (GitClientNet472SmokeTests)
• PowerShell command generation (ProjectBuildCommandHostServiceNet472SmokeTests)

Gaps to consider:

• No smoke test for the URI-based GetRelativePath fallback, particularly for paths with spaces.
• No smoke test for AuthenticodeSigningHostService or ModuleBuildHostService net472 paths (though these are simpler wrappers).
• Response-file cleanup on failure is untested (see Bug 3).
• PublishVerificationHostService record-to-class conversions are not smoke-tested; a basic instantiation test would catch regressions in equality semantics.

---

Workflow / CI

The new CI steps are well-ordered: build net472, run net472 smoke tests, build module, test PS5.1, test PS7. Minor note: the Verify net472 compatibility step uses backslash paths (.\PowerForge\...) with shell: pwsh — these work on Windows runners but would fail on Linux. If the runner is always Windows, adding an explicit runs-on: windows-* label or a comment would make this self-documenting.

---

Summary

Area	Assessment
Overall design	Solid — centralized shim class is the right approach
Record-to-class conversions	Equality semantics silently changed — document or fix
Cancellation in ReadAsStreamAsync	Mid-stream cancellation not honoured — document the limitation
Response file cleanup on failure	Untested path; verify credentials cannot linger
GetRelativePath edge cases	Known limitation; consider adding a space-in-path test
Test coverage	Good smoke test suite; minor gaps noted above
CI workflow	Well-structured build and test ordering

This is a high-quality contribution. Addressing the record equality concern and the response-file cleanup-on-failure case would make it production-safe. The remaining notes are improvements rather than blockers.