Add delivery-level signing for bundled internals

[PrzemyslawKlys]

Summary

• add New-ConfigurationDelivery -Sign so delivery bundles can opt their internals into module signing
• merge delivery signing into the existing signing pipeline and honor custom InternalsPath
• add focused tests and regenerate delivery docs/help

Stack

• stacked on top of Refactor module pipeline and slim PSPublishModule payload #178
• review Refactor module pipeline and slim PSPublishModule payload #178 first, then this PR

Details

• DeliveryOptionsConfiguration now carries a Sign flag and uses schema 1.4
• delivery signing now enables signing for the run and forces the configured delivery path to be included in signing
• signing exclusion logic now respects custom delivery internals paths instead of hardcoding only Internals
• explicit internals exclusions are removed when delivery signing opts the bundle back in
• builds now fail with a clear error if delivery/module signing is enabled without any certificate configured

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "ModulePipelineDeliverySigningTests|SigningIncludePatternsTests|DeliveryCommandGeneratorTests|ModulePipelineDeliveryMergedTests|ModulePipelineRefreshManifestOnlyTests"
• pwsh .\Module\Build\Build-Module.ps1 -NoSign

[claude]

PR 179 Code Review - see inline for details

[claude]

PR 179 Full Code Review

Overall this is a well-structured feature addition with good test coverage. A few issues worth addressing before merging.

---

1. Bug: ShouldRemoveInternalsExcludePath has a problematic hardcoded fallback

File: PowerForge/Services/ModulePipelineRunner.FormattingSigningDelivery.cs (lines 293-294)

The clause normalized.Equals("Internals", StringComparison.OrdinalIgnoreCase) will silently remove "Internals" from ExcludePaths even when InternalsPath is something entirely different. Example scenario:

• InternalsPath = "Assets" (delivery internals live in Assets/)
• ExcludePaths = ["Internals"] (user excluded an unrelated folder for a different reason)
• Enabling Sign incorrectly strips that exclusion

ResolveDeliveryInternalsPath already returns "Internals" as the default when InternalsPath is unset, so the fallback is redundant in the safe case and incorrect in the custom-path case. It should be removed or gated behind trimmedInternals.Equals("Internals", ...).

No test covers this edge case. A test with InternalsPath = "Assets" and ExcludePaths = ["Internals", "Assets"] would expose the bug: "Internals" should survive the filter but currently would not.

[claude]

2. ApplyDeliverySigningPreference is called twice

Plan already calls ApplyDeliverySigningPreference and stores the effective result in plan.Signing. SignBuiltModuleOutput then calls it again on the already-modified signing object (via the delivery parameter passed from Run.cs).

The second call is a no-op in practice — ExcludePaths are already filtered and IncludeInternals is already true — but it creates two unnecessary clones of SigningOptionsConfiguration and is non-obvious to future readers. Since Plan is where decisions are made and Run executes them, the preference should ideally be applied only once. The delivery parameter on SignBuiltModuleOutput could then be removed, simplifying the signature.

If the second call is intentional as a defensive measure for callers that invoke Run without calling Plan first, a comment explaining that reasoning would help.

---

3. Missing test for the no-certificate error path

There is no test for delivery.Sign = true with no certificate configured. The PR description calls this out as a key guarantee: "builds now fail with a clear error if delivery/module signing is enabled without any certificate configured."

A test asserting InvalidOperationException is thrown (with the expected message fragment) when delivery.Sign = true and all of CertificateThumbprint, CertificatePFXPath, and CertificatePFXBase64 are empty would validate this promise.

[claude]

4. Schema default change is a silent bump

File: PowerForge/Models/Configuration/ConfigurationOptionsSegment.cs (line 157)

The default value changed from "1.3" to "1.4". Any code that round-trips a DeliveryOptionsConfiguration through a default constructor will now silently emit schema 1.4. If downstream consumers (e.g. the installer/updater script) gate on Schema == "1.3" or perform strict version checks, this could break them. Worth confirming all consumers handle 1.4 gracefully before this lands.

---

5. Minor: primary example now always shows -Sign

File: Module/Docs/New-ConfigurationDelivery.md

Example 1 now includes -Sign unconditionally. Since signing requires a certificate to be configured and is not universally applicable, this may give the impression that signing is the expected default. A basic example without -Sign as Example 1 and a signing-enabled variant as an additional example would better communicate that it is opt-in.

---

Nits

• In ShouldRemoveInternalsExcludePath, the guard (internalsPath ?? "Internals") is redundant: the parameter is non-nullable and ResolveDeliveryInternalsPath never returns null.
• value! on line 282 is unnecessary because the preceding IsNullOrWhiteSpace guard already guarantees non-null at that point.

---

Summary: The core logic is sound and the feature adds real value. The two main asks before merging are: (1) fix the ShouldRemoveInternalsExcludePath fallback bug, and (2) add a test for the no-certificate error path. The double ApplyDeliverySigningPreference call is worth addressing even if not blocking.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85fbac5fba

ℹ️ 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]

Pass delivery context into signing exclusion builder

SignBuiltModuleOutput still calls BuildSigningExcludeSubstrings(signing) without the delivery config, so the runtime exclusion list always falls back to Internals instead of the configured Delivery.InternalsPath. This means builds that enable module signing with IncludeInternals=false and a custom internals folder (for example Assets) will sign files that should remain excluded, despite the new custom-path logic added in this commit.

Useful? React with 👍 / 👎.

[claude]

PR Review for delivery-level signing feature. Key findings: (1) Bug - duplicate null check dead code in SignBuiltModuleOutput in FormattingSigningDelivery.cs; the second if(signing is null) block is unreachable and should be removed. (2) Concern - fragile exception message string matching in RegisterCertificateCommand to distinguish missing signing commands; a dedicated exception type would be safer. (3) Minor - gitHubToken null-forgiving operator in InvokeProjectBuildCommand suppresses compiler warning without runtime guard. (4) Minor - IsInternalsPath hardcodes Internals regardless of configured delivery InternalsPath. (5) Minor - per-file PowerShell instance creation is not a regression but a future optimisation opportunity. Overall positives: clean removal of PS-script-backed signing in favor of typed C# services; good test coverage for delivery signing; correct fix for Microsoft.PowerShell.* inbox module resolution failures; schema bump to 1.4 paired with new Sign flag.