refactor: thin Invoke-DotNetRepositoryRelease

[PrzemyslawKlys]

Summary

• move Invoke-DotNetRepositoryRelease request parsing and spec construction into a dedicated preparation service
• move the plan-vs-execute orchestration into a small workflow service so the cmdlet mostly handles PowerShell entrypoint behavior and summary output
• add focused tests for request preparation and workflow execution behavior

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "DotNetRepositoryReleasePreparationServiceTests|DotNetRepositoryReleaseWorkflowServiceTests|DotNetRepositoryReleaseServiceTests"
• pwsh .\Module\Build\Build-Module.ps1 -NoSign

[claude]

PR Review: refactor: thin Invoke-DotNetRepositoryRelease

Good overall direction — extracting preparation and workflow concerns out of the cmdlet makes the logic testable and the cmdlet easier to reason about. The tests are a welcome addition. A few issues worth addressing before merge:

---

Bugs / Behavioral Changes

1. ParseExpectedVersionMap error type changed — breaks PS error handling

The original code called ThrowTerminatingError(new ErrorRecord(...)), which produces a properly formatted PowerShell terminating error with a category and target object. The new PreparationService throws a plain ArgumentException instead:

// PreparationService.cs
throw new ArgumentException("ExpectedVersionMap entries must include both project name and version.", nameof(entries));

This will surface as an unhandled .NET exception in the PowerShell host rather than a structured ErrorRecord. The cmdlet should catch this and convert it via ThrowTerminatingError, or the service should return a validation result that the cmdlet can act on.

---

2. DotNetRepositoryReleasePreparedContext.Spec is mutated as a side effect

WorkflowService.Execute mutates context.Spec.WhatIf:

context.Spec.WhatIf = true;
var plan = _executeRelease(context.Spec);
if (!executeBuild) return plan;

context.Spec.WhatIf = false;  // side-effect on caller's object
return _executeRelease(context.Spec);

After Execute returns, context.Spec.WhatIf is always false. Any caller that inspects the context after the call will see unexpected state. Consider working with a clone/copy of the spec, or making WhatIf a parameter to a private helper rather than mutating shared state.

---

3. RootPath is stored in two places within DotNetRepositoryReleasePreparedContext

Both context.RootPath and context.Spec.RootPath are set to the same resolved path in PreparationService. The cmdlet uses context.RootPath for ShouldProcess while the service uses context.Spec.RootPath for execution. If they ever diverge (e.g., someone updates the spec but not the context), ShouldProcess will show the wrong path. Consider removing context.RootPath and reading from context.Spec.RootPath directly.

---

4. CertificateStore mapping is duplicated across cmdlet and service

The cmdlet maps the store enum before creating the request:

CertificateStore = CertificateStore == CertificateStoreLocation.LocalMachine
    ? PowerForge.CertificateStoreLocation.LocalMachine
    : PowerForge.CertificateStoreLocation.CurrentUser,

Then PreparationService maps it again:

var mappedStore = request.CertificateStore == CertificateStoreLocation.LocalMachine
    ? PowerForge.CertificateStoreLocation.LocalMachine
    : PowerForge.CertificateStoreLocation.CurrentUser;

Since DotNetRepositoryReleasePreparationRequest.CertificateStore is already typed as PowerForge.CertificateStoreLocation, the second mapping is a no-op. The service copy should be removed.

---

Test Gaps

5. DotNetRepositoryReleaseWorkflowServiceTests — ErrorMessage misused as a test carrier

ErrorMessage = spec.WhatIf ? "plan" : "release"

Using ErrorMessage as a label to distinguish plan vs. release calls is fragile and misleading — it implies an error occurred. Consider using a separate counter or a list of recorded WhatIf values instead:

var whatIfValues = new List<bool>();
executeRelease: spec => { whatIfValues.Add(spec.WhatIf); return new DotNetRepositoryReleaseResult { Success = true }; }
// assert: whatIfValues == [true, false]

6. No test for inline NugetCredentialSecret resolution

Prepare_resolves_root_credentials_and_spec covers file-based and env-based secret resolution but skips the inline path. A simple case with NugetCredentialSecret = "inline-token" (and no file/env set) would complete the coverage.

7. Pack field not asserted in preparation test

The test sets SkipPack implicitly to false (default), so context.Spec.Pack should be true, but this isn't asserted. Worth adding given that Pack = !request.SkipPack is a non-obvious inversion.

---

Minor

8. OutputPath resolution is a behavior change

Previously OutputPath was passed as-is to the spec. Now it is resolved relative to rootPath via ProjectBuildSupportService.ResolveOptionalPath. This is arguably an improvement, but it is a breaking change for callers who pass absolute paths (those still work) or repo-relative paths (now resolved differently). The PR description doesn't call this out — worth documenting.

9. Trace.TraceWarning removed from secret resolution

The old ResolveSecret logged diagnostics via Trace.TraceWarning when file/env reads failed. The new shared ProjectBuildSupportService.ResolveSecret silently swallows exceptions (// best effort). Losing the diagnostic trace makes it harder to debug misconfigured secret paths in production scenarios.

---

Summary

#	Severity	Area
1	Medium	Error handling — ArgumentException vs. ThrowTerminatingError
2	Medium	Design — spec mutation side-effect
3	Low	Design — RootPath duplication
4	Low	Code — redundant CertificateStore mapping
5	Low	Tests — misleading use of ErrorMessage
6	Low	Tests — missing inline secret test
7	Low	Tests — Pack field not asserted
8	Info	Behavior — OutputPath resolution change
9	Info	Diagnostics — trace logging removed

The structure is clean and the direction is right. Items 1 and 2 are the most worth addressing before this lands.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f3caa7373

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve PowerShell path expansion for release root

This refactor changed root resolution from PowerShell's GetUnresolvedProviderPathFromPSPath to ResolveOptionalPath (System.IO.Path semantics), so valid cmdlet inputs like -Path ~ or other PSDrive-style paths are now interpreted as literal/relative filesystem text instead of expanded PowerShell paths. In those scenarios the cmdlet can target the wrong directory (or fail with missing path) even though the same input worked before, which is a behavior regression for interactive users and scripts.

Useful? React with 👍 / 👎.