refactor: extract project build support service

[PrzemyslawKlys]

Summary

• move Invoke-ProjectBuild config loading, path resolution, secret resolution, staging prep, plan writing, and publish preflight checks into a shared support service
• replace the nested project build config helper with a typed PowerForge configuration model that is friend-visible to PSPublishModule and tests
• keep the cmdlet helper partial focused on PowerShell-specific path resolution and output formatting

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "ProjectBuildSupportServiceTests|ProjectBuildGitHubPreflightTests|ProjectBuildGitHubPublisherTests"
• pwsh .\Module\Build\Build-Module.ps1 -NoSign

[claude]

PR Review: refactor: extract project build support service

This is a well-motivated refactoring that correctly identifies testability as the core benefit of moving helpers out of the private cmdlet scope. Here are my observations:

---

Critical Issues

1. Type Safety Regression: ParseGitHubTagConflictPolicy returns string instead of GitHubTagConflictPolicy enum

Before:

private static GitHubTagConflictPolicy ParseGitHubTagConflictPolicy(string? value)
{
    if (Enum.TryParse<GitHubTagConflictPolicy>(text, ignoreCase: true, out var parsed))
        return parsed;
    return GitHubTagConflictPolicy.Reuse;
}

After (ProjectBuildSupportService.cs):

public static string ParseGitHubTagConflictPolicy(string? value) { ... }

This loses compiler exhaustiveness checking at call sites. The call site in GitHubPreflight.cs changed from:

if (conflictPolicy != GitHubTagConflictPolicy.Reuse)

to:

if (!string.Equals(conflictPolicy, "Reuse", StringComparison.OrdinalIgnoreCase))

The new implementation also hard-codes only "AppendUtcTimestamp" and "Fail" as recognized values. The old Enum.TryParse handled all enum members automatically — any new member added to the enum would need a corresponding manual update to the service. This is a hidden maintenance hazard.

Recommendation: Either keep the GitHubTagConflictPolicy enum or define an equivalent internal enum in PowerForge. The string-based approach has no tangible benefit.

2. Silent Bug: ApplyTagConflictPolicy doesn't handle "Fail" policy

ParseGitHubTagConflictPolicy recognizes and returns "Fail" as a valid value. However, ApplyTagConflictPolicy only handles "AppendUtcTimestamp" and falls through to returning the unchanged tag for all other values — including "Fail". If "Fail" is intended to signal a conflict error, this is a silent no-op that produces "Reuse" behavior instead. The old enum-based switch had the same _ => tag fallthrough, but "Fail" was never surfaced as a recognized policy name before.

---

Potential Bugs

3. IsTrue drops the SwitchParameter branch

Before:

if (value is SwitchParameter sp) return sp.IsPresent;

After (removed — falls through to value.ToString()):
The new code relies on SwitchParameter.ToString() returning "True" or "False", which works incidentally but depends on an undocumented behavior rather than the .IsPresent property. This is fragile. The SwitchParameter branch was dropped without adding using System.Management.Automation to the new file — add it back explicitly.

---

Test Coverage Gaps

The 3 new tests are a good start, but they miss the highest-risk methods changed in this PR:

Method	Tested?
ParseGitHubTagConflictPolicy	❌ No — the most changed method has zero coverage
ApplyTagConflictPolicy	❌ No — especially the "Fail" path
IsTrue	❌ No — especially for SwitchParameter input
ResolveGitHubBaseVersion	❌ No
ApplyTemplate	❌ No
PrepareStaging	❌ No
LoadConfig	✅ Yes
ResolveSecret	✅ Yes
ValidatePreflight	✅ Partial

ParseGitHubTagConflictPolicy is the whole point of making this logic testable — please add tests before merging.

---

Design Observations

4. Mixed static/instance design is inconsistent

ProjectBuildSupportService has an ILogger _logger field (instance), but about half its methods are public static. This makes the class hard to mock in tests (instance methods call static helpers like ResolveSecret directly). Consider either:

• All-static pure utility class, or
• All-instance with an interface (IProjectBuildSupportService) for injection

The current hybrid forces callers to juggle both support.InstanceMethod(...) and ProjectBuildSupportService.StaticMethod(...).

5. InternalsVisibleTo grants broad access

Adding [assembly: InternalsVisibleTo("PSPublishModule")] gives PSPublishModule access to all future internal types in PowerForge, not just the two new ones. This is architecturally intentional but worth noting — any new internal type added to PowerForge is automatically accessible from PSPublishModule without further review.

---

What's Good

• Correct motivation: moving logic into PowerForge makes it testable without the PowerShell runtime
• Clean separation: cmdlet partial is now focused on PS-specific path resolution and output
• Good XML doc comments on all new members
• The InternalsVisibleTo pattern is used appropriately
• ProjectBuildConfig → ProjectBuildConfiguration rename is more idiomatic for .NET
• No public API breaking changes

---

Summary of Recommended Fixes Before Merge

1. Restore GitHubTagConflictPolicy enum as return type of ParseGitHubTagConflictPolicy (or define an equivalent in PowerForge)
2. Handle "Fail" policy in ApplyTagConflictPolicy with defined behavior (throw? return error indicator?)
3. Add SwitchParameter branch back to IsTrue (or add test proving ToString() path works correctly)
4. Add tests for ParseGitHubTagConflictPolicy, ApplyTagConflictPolicy, and IsTrue

Reviewed by Claude (claude-sonnet-4-6)