Limit portable publish signing scope

[PrzemyslawKlys]

Summary

• change dotnet publish signing to sign executables by default instead of every dll in the output tree
• add explicit IncludeDlls opt-in on publish signing profiles and overrides
• cover the new default and opt-in behavior with hardening tests

Why

During the signed IntelligenceX release rehearsal on April 6, 2026, a portable release spent an unreasonable amount of time inside token-backed signing because PowerForge was recursively signing the full portable payload. That is too expensive for normal desktop-app portable releases and does not match the common expectation of signing the app binary layer by default.

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter DotNetPublishPipelineRunnerHardeningTests
• dotnet build .\PowerForge.Cli\PowerForge.Cli.csproj -c Release -f net10.0
• dotnet build .\PSPublishModule\PSPublishModule.csproj -c Release -f net8.0
• dotnet build .\PSPublishModule\PSPublishModule.csproj -c Release -f net472

[claude]

Code Review

Overall this is a clean, well-motivated change. The diff is focused, the model/resolver/schema are kept in sync, and the new tests cover both the default and opt-in paths. A few things worth flagging:

---

Breaking change — silent for existing users

The default behavior flips from signing *.exe + *.dll to signing *.exe only. Any existing profile that relied on DLLs being signed will silently stop signing them after upgrading. Consider:

• A one-time warning log when DLL files are detected in the output but IncludeDlls is not set (i.e., "Found N .dll file(s); set IncludeDlls: true to include them in signing"), or
• A changelog / migration note so callers know to audit their configurations.

---

Reflection on a private method in tests

Both new tests reach into TrySignOutput via BindingFlags.NonPublic:

var method = typeof(DotNetPublishPipelineRunner)
    .GetMethod("TrySignOutput", BindingFlags.Instance | BindingFlags.NonPublic);

This is already the pattern in the surrounding test class, so it is at least consistent. However, it silently breaks (returns null) if the method is renamed — the Assert.NotNull(method) guard only prevents a NullReferenceException, it doesn't produce a clear "method not found" failure message. Consider using Assert.NotNull(method, \"TrySignOutput private method not found — was it renamed?\") to make failures diagnosable.

A longer-term alternative would be marking TrySignOutput as internal and adding [assembly: InternalsVisibleTo(\"PowerForge.Tests\")], which keeps it package-private while making the test call site type-safe.

---

ComSpec may be null in some CI environments

ToolPath = Environment.GetEnvironmentVariable("ComSpec"),

ComSpec is typically set on Windows but is not guaranteed in all CI runners. If it resolves to null, ToolPath is null, and the test's behavior depends on how the runner handles a null tool path (which currently defaults to \"signtool.exe\"). Hardcoding @\"C:\Windows\System32\cmd.exe\" (or using Environment.SystemDirectory) would be more robust, though the platform-skip guard does reduce the risk.

---

Minor: IncludeDlls = false on a patch is a no-op

In DotNetPublishSignPatch, IncludeDlls is bool?. Setting it to false in a patch would override a profile that had IncludeDlls = true, but it also means "explicitly disable DLLs" — which is already the default. This is fine semantically, but the XML doc comment ("Optional include-DLLs override") could note that false resets to the default rather than being a no-op, to avoid confusion.

---

What looks good

• The model, resolver (ApplySignPatch, profile copy, override copy), and JSON schema are all updated consistently — no drift.
• The CollectingLogger helper is clean and minimal.
• TryDelete / CreateTempRoot reuse from existing infrastructure is correct.
• The schema correctly makes IncludeDlls non-nullable in the base options object and nullable in the patch object, matching the pattern for Enabled.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a62b4c2840

ℹ️ 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".

[claude]

Code Review

The change is well-scoped and addresses a real performance problem (over-signing during portable releases). The model, resolver, schema, and tests are updated consistently. Here is my assessment:

---

What looks good

• Consistent propagation: IncludeDlls is correctly threaded through DotNetPublishSignOptions, DotNetPublishSignPatch, DotNetPublishSigningProfileResolver (profile copy, override copy, and ApplySignPatch), and the JSON schema — no drift between layers.
• Schema correctness: IncludeDlls is non-nullable in the base options object and nullable in the patch object, matching the established pattern for Enabled.
• Test coverage: Both the new default (executables only) and the opt-in path (IncludeDlls: true) are covered with dedicated tests.
• CollectingLogger is minimal and clean.
• The whitespace-only reformat of the catch block in SigningAndProcess.cs is harmless.

---

Concerns

1. Breaking change is silent for existing callers

The default flips from *.exe + *.dll to *.exe only. Any existing profile that relied on DLLs being signed will silently drop them after upgrading. Two mitigations worth considering:

• A one-time info/warn log when .dll files are present in the output but IncludeDlls is false: "Found N .dll file(s) in output; set IncludeDlls: true to include them in signing." This surfaces the gap without requiring callers to read a changelog.
• A migration note in the changelog so callers know to audit configurations.

2. Assert.NotNull on the reflected method lacks a diagnostic message

Assert.NotNull(method);

If TrySignOutput is renamed, this assertion fails with a generic "Expected: not null, Actual: null" rather than indicating the cause. Prefer:

Assert.NotNull(method, "TrySignOutput private method not found — was it renamed or made public?");

The longer-term fix would be internal + [assembly: InternalsVisibleTo("PowerForge.Tests")], which makes the call site type-safe and removes the reflection entirely.

3. ComSpec is not guaranteed in all CI runners

ToolPath = Environment.GetEnvironmentVariable("ComSpec"),

ComSpec is typically set on Windows, but some CI images (e.g., containerized runners or stripped Windows Server Core images) may not have it. If it resolves to null, the test behaves differently than intended. Using Environment.SystemDirectory + @"\cmd.exe" or Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), "cmd.exe") would be more robust. The existing IsWindows() guard reduces the risk, but it is worth hardening.

4. IncludeDlls = false on a patch resets rather than no-ops

This is minor, but DotNetPublishSignPatch.IncludeDlls = false will override a profile's IncludeDlls = true back to false. The XML doc comment says "Optional include-DLLs override," which is correct, but it is easy to misread as a no-op when false. A brief clarifying note — "set to false to explicitly disable DLL signing even if the base profile enables it" — would prevent confusion.

---

Summary

The core logic change is correct and the scope is appropriate. The main action items are: (1) add a log hint to avoid silent breakage for existing users, and (2) improve the reflected-method assertion message. The rest are minor polish items.

[claude]

Code Review

The change is well-scoped and addresses a real performance problem (over-signing during portable releases). The model, resolver, schema, and tests are updated consistently.

What looks good

• Consistent propagation: IncludeDlls is added in all three required places in DotNetPublishSigningProfileResolver - the profile copy, the override copy, and ApplySignPatch. No drift.
• Schema correctness: In powerforge.dotnetpublish.schema.json, IncludeDlls is typed as boolean (non-nullable) on the base options object and ["boolean", "null"] on the patch object - correctly mirrors the Enabled precedent.
• Test helper refactor: Extracting GetTrySignOutputMethod() with Assert.True(method is not null, "...") is an improvement over the previous inline Assert.NotNull - the failure message makes renames diagnosable.
• CollectingLogger: Minimal, sealed, correct - exactly what a test double should be.
• Environment.SystemDirectory is used in both new tests (rather than %ComSpec%), which is more robust across CI environments.

Concerns

1. Breaking change - silent for existing consumers

The default behavior flips from signing *.exe + *.dll to signing *.exe only. Any caller that relied on DLLs being signed will silently stop getting them signed after upgrading, with no runtime warning.

Suggestion: emit a one-time info/warn log when DLL files are present in the output directory but IncludeDlls is false. This makes the new default visible to existing users without blocking anything.

2. Tests skip silently on non-Windows

Both new tests use an early-return guard. On Linux/macOS CI the tests pass vacuously - there is no signal that coverage was skipped. Consider using xUnit's Assert.Skip so the test run reports them as skipped rather than passed. This is a minor nit and is consistent with the existing test class pattern.

3. IncludeDlls = false in a patch is a reset, not a no-op

The doc comment says "Set false to explicitly disable DLL signing even if the base profile enables it" - this is accurate, but worth noting that false is also the default, so omitting the field entirely has the same effect unless a base profile already set it true. Helps avoid confusion for future readers.

Summary

No blocking issues. The main item worth acting on before merge is the silent breaking change - a warn log when DLLs are detected but not opted into would be cheap to add and would protect existing users.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44453b4f12

ℹ️ 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".