Add reusable PowerForge publish bundles and hooks by PrzemyslawKlys · Pull Request #331 · EvotecIT/PSPublishModule

PrzemyslawKlys · 2026-04-28T18:13:50Z

Summary

Add reusable PowerForge dotnet publish command hooks for restore/build/publish/bundle phases.
Add bundle composition primitives for primary subdirectories, target includes, copy items, module includes, generated scripts, and post-processing.
Add bundle post-process signing so composed package payloads can sign scripts, module manifests, DLLs, and EXEs before zip/MSI preparation.
Add MSI prepare support from composed bundles, harvest exclude patterns, schemas, docs, examples, and export coverage for existing PSPublishModule helpers.
Harden module formatting so mixed line endings are normalized before PSScriptAnalyzer formatting and self-build wrapper failures propagate to CI.

Why

TierBridge and TestimoX both had large bespoke PowerShell package/MSI flows. This moves the reusable pieces into PowerForge so downstream repos can use thin wrappers and shared JSON configs instead of reimplementing staging, bundle composition, signing, and MSI preparation.

Validation

dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~DotNetPublish" --nologo passed: 101 tests.
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~DotNetPublishPipelineRunnerBundleTests|FullyQualifiedName~PowerForgeBundlePostProcessServiceTests" --nologo passed: 12 tests.
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~LineEndingsNormalizerTests|FullyQualifiedName~FormattingSummaryTests" --nologo passed: 40 tests.
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~DotNetPublishPipelineRunnerBundleTests|FullyQualifiedName~PowerForgeBundlePostProcessServiceTests|FullyQualifiedName~LineEndingsNormalizerTests" --nologo passed: 14 tests.
.\Build\Build-Module.ps1 passed locally.
.\Build\Build-Module.ps1 -NoBuild passed locally after restoring the mixed source files, proving the formatter pre-normalization path.
git diff --check passed.
Earlier full validation on this branch: dotnet test .\PSPublishModule.sln -c Release --nologo, PowerForge CLI net10 build, PowerForge net472 build.
TierBridge package/MSI branch was validated against this PowerForge branch and now consumes bundle post-process signing.

claude · 2026-04-28T18:46:21Z

Code Review: PR #331 — PowerForge Reusable Publish Bundles and Hooks

Overview

This PR is a well-scoped, meaningful addition to the PowerForge engine. It introduces four bundle composition primitives (PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[]), a full command-hook system with six lifecycle phases, WiX harvest exclude patterns, bundle topology sorting, a plugin catalog schema/example, new module exports, and tests for all of the above. The scope is well-contained, and the validation reported in the PR body (1564 tests, both TierBridge and TestimoX branch validation) is solid.

Bugs / Correctness Issues

1. Dead code in NormalizeArguments
After .Where(v => v is not null) the subsequent .Select(v => v ?? string.Empty) can never branch — v is already guaranteed non-null by the filter. Minor, but dead code creates confusion:

// PowerForge/Services/DotNetPublishPipelineRunner.Plan.cs
private static string[] NormalizeArguments(IEnumerable<string>? values)
{
    return (values ?? Array.Empty<string>())
        .Where(v => v is not null)
        .Select(v => v ?? string.Empty)   // <- dead branch
        .ToArray();
}

2. Potential glob-matching gap in ShouldExcludeHarvestFile

The exclude pattern logic:

var rootlessGlobstar = pattern.StartsWith("**/") ? pattern.Substring(3) : null;
if (WildcardMatch(relative, pattern)
    || WildcardMatch(fileName, pattern)
    || (rootlessGlobstar is not null && WildcardMatch(relative, rootlessGlobstar)))

For a pattern like **/*.pdb applied to sub/dir/myfile.pdb:

WildcardMatch("sub/dir/myfile.pdb", "**/*.pdb") — depends on whether WildcardMatch treats ** as a multi-segment wildcard.
WildcardMatch("myfile.pdb", "**/*.pdb") — likely false because of the **/ prefix.
WildcardMatch("sub/dir/myfile.pdb", "*.pdb") — likely false if * doesn't cross /.

If WildcardMatch is a simple single-segment wildcard matcher, **/*.pdb won't reliably exclude files in subdirectories. Worth verifying the implementation handles this correctly or adding a dedicated test for a 2-level-deep path. The quickstart docs advertise **/*.pdb as a working pattern, so this would be a user-visible bug if WildcardMatch can't handle it.

3. PostProcess cloning is shallow in CloneBundles

The new PostProcess = b.PostProcess assignment is a reference copy, not a deep clone. PostProcess contains array fields (DeletePatterns, ArchiveDirectories). This is fine as long as PostProcess is immutable after planning (which appears to be the case), but if anything mutates it downstream the shared reference would be a problem. The other collections in this file get defensive copies — PostProcess should too, or a comment should explain why a shallow copy is intentional here.

Code Quality

4. Silent kill in timeout path

// DotNetPublishPipelineRunner.Hooks.cs
try { process.Kill(); } catch { /* best effort */ }

Swallowing the kill exception entirely means kill failures are invisible even in verbose/diagnostic runs. A _logger.Verbose(...) on the catch block would make timeouts easier to debug.

5. ResolveHookCommandPath strips leading/trailing quotes from the command

var raw = (command ?? string.Empty).Trim().Trim('"');

This is probably fine for the common case of "pwsh" or "dotnet", but it silently modifies "C:\Program Files\my app\run.exe" to C:\Program Files\my app\run.exe, which then goes through the rooted-path branch correctly. If someone passes a command with an asymmetric quote (e.g. a template substitution result that starts with " but doesn't end with it) Trim('"') will strip only the leading quote and pass a malformed path. A TrimStart/TrimEnd pair with a comment about the intent would be clearer.

6. Two-tier token syntax without a prominent doc callout

Bundle path tokens use single-brace {output}, {rid} etc. (via ApplyTemplate), while script template bodies use double-brace {{TokenName}} (via ScriptTemplateRenderer.Render). Token values in GeneratedScripts[].Tokens are first resolved through ApplyTemplate (single-brace) and then available as {{…}} placeholders inside the template. This is correct and documented in the quickstart, but the two-brace convention is easy to misread as a C# verbatim string escape rather than the actual delimiter. The quickstart sentence is a bit buried — it might help to add a small token-expansion table or a worked example showing the layering.

Design / Documentation

7. Docs/PowerForge.BuildReuse.TestimoXTierBridge.md contains internal artifacts

The file opens with:

Date: 2026-04-28
Branch/worktree: `codex/catalog-reuse` in `C:\Support\GitHub\PSPublishModule-codex-catalog-reuse`

Hardcoded local paths and a date-stamped analysis artifact make this look like a planning scratchpad rather than long-lived documentation. The content is genuinely useful context, but the machine path (C:\Support\GitHub\...) will immediately confuse anyone reading the repo later. Consider either removing the branch/worktree line, abstracting to a more generic framing, or moving it to an ADR or wiki entry rather than Docs/.

What's Working Well

OrderTargetsForBundleIncludes — the topological sort with cycle detection is correct and handles the case where there are no dependencies without unnecessary re-ordering (early return when all dependency sets are empty).
EnsurePathWithinRoot guards on every new CopyBundlePath call — good path-traversal hygiene.
Token scoping in CopyBundleModules — building a new moduleTokens dictionary per module rather than mutating the shared tokens is the right pattern.
CloneBundles now includes PostProcess — this fixes a pre-existing bug where PostProcess was silently dropped during profile resolution. Good catch.
ExportDetectorTests — adding parity checks between binary cmdlets and module-manifest exports is exactly the right test to prevent the gap described in the planning doc from recurring.
Schema completeness — powerforge.plugins.schema.json has additionalProperties: false and explicit required arrays throughout; DotNetPublishCommandHook and the new bundle primitives follow the same pattern.
Test coverage for the new features is solid; bundle, hook, MSI prepare, and preparation service tests all include plan-mode and execution-mode variants.

Minor Nits

HarvestExcludePatterns is added to DotNetPublishInstaller (spec class) but there's no corresponding New-ConfigurationDotNetInstaller parameter update shown in the diff. If this is set via JSON/config only, that's fine — just worth confirming the PowerShell surface intentionally omits it.
TimeoutSeconds defaults to 600 in the step model (DotNetPublishPlan.cs) but to 0 via default initialization in the spec model (DotNetPublishCommandHook). If 0 is treated as "use default" by NormalizeCommandHooks's Math.Max(1, hook.TimeoutSeconds), the effective timeout for an unset hook is 1 second, not 600. Check that the default propagation is intentional.

Summary

The implementation is solid and the new features are well-tested. The two items most worth verifying before merge are the glob-matching depth for HarvestExcludePatterns (item 2) and the TimeoutSeconds default propagation (last nit). The internal-path artifact in the new doc (item 7) is a low-risk cosmetic issue but worth cleaning up since it will confuse future readers.

claude · 2026-04-28T19:31:14Z

Code Review: Add Reusable PowerForge Publish Bundles and Hooks

Overview: This PR extracts common TierBridge/TestimoX build/package/MSI flows into reusable PowerForge primitives — command hooks (6 phases), bundle composition (CopyItems, ModuleIncludes, GeneratedScripts, PrimarySubdirectory), post-process signing, and MSI harvest exclude patterns. The architecture is sound and the motivation is solid. Below is my analysis.

✅ Strengths

Good validation at plan-normalization time — duplicate hook IDs, blank commands, and blank IDs all throw early with clear messages, not at execution time.
Path safety — EnsurePathWithinRoot is applied consistently to all new copy/generate destinations (CopyItems, ModuleIncludes, GeneratedScripts output), which prevents path traversal in bundle outputs.
Cross-platform argument handling — the #if NET472 / modern ArgumentList split in RunHookProcess is the correct approach and avoids shell injection on modern .NET.
Timeout floor — Math.Max(1, step.HookTimeoutSeconds) is a simple but correct guard against zero-second timeouts.
Test coverage — 16 new tests across 7 test files, covering the plan-phase (hook ordering, spec deserialization, normalization) and the run-phase (token expansion, bundle composition).

⚠️ Issues & Suggestions

1. Potential deadlock in hook timeout path on .NET 4.7.2 (`RunHookProcess`)

if (!process.WaitForExit(timeoutMs))
{
    try { process.Kill(); } catch { /* best effort */ }
    var stdout = stdoutTask.GetAwaiter().GetResult();  // ← can block indefinitely
    var stderr = stderrTask.GetAwaiter().GetResult();  // ← on .NET 472
    return (-1, stdout, stderr, fileName);
}

On .NET Framework, Process.WaitForExit(int milliseconds) does not guarantee that the redirected StandardOutput/StandardError streams have been fully read after it returns (only the zero-argument overload provides that guarantee — see the MSDN note). After Kill(), the process is gone but the async ReadToEndAsync() tasks may not complete promptly if the internal pipe hasn't flushed. This is a potential deadlock on the net472 target.

Suggested fix: After Kill(), give the tasks a bounded wait (e.g., Task.WhenAll(stdoutTask, stderrTask).Wait(TimeSpan.FromSeconds(5))), then read whatever arrived:

if (!process.WaitForExit(timeoutMs))
{
    try { process.Kill(); } catch { }
    Task.WhenAll(stdoutTask, stderrTask).Wait(TimeSpan.FromSeconds(5));
    return (-1, stdoutTask.IsCompleted ? stdoutTask.Result : string.Empty,
                stderrTask.IsCompleted ? stderrTask.Result : string.Empty, fileName);
}

Also consider using process.Kill(entireProcessTree: true) (net5+) to avoid orphaned child processes spawned by the hook.

2. `TemplatePath` is not bounds-checked against `ProjectRoot`

In GenerateBundleScripts:

var templatePath = ResolvePath(plan.ProjectRoot, ApplyTemplate(script.TemplatePath!, tokens));
// No EnsurePathWithinRoot check here ↑
template = File.ReadAllText(templatePath, Encoding.UTF8);

The output path is checked (EnsurePathWithinRoot(outputDir, outputPath, ...)), but the read path (template source) is not. A config with "TemplatePath": "../../secrets/.env" would read a file outside the project tree. This is config-controlled, not user-input-controlled, but it's still worth adding a similar root-check against plan.ProjectRoot for defense in depth.

3. High default timeout (`HookTimeoutSeconds = 600`)

public int HookTimeoutSeconds { get; set; } = 600;

Ten minutes is a long default for what is likely a pre/post-step command. If a hook silently hangs (e.g., a network call, an interactive prompt), the entire pipeline will block for 10 minutes before reporting failure. Consider a lower default (e.g., 120s or 300s) with documentation encouraging users to increase it for genuinely long-running hooks. At minimum the quickstart doc or schema description should call this out.

4. `BundleSignPatternMatches` — redundant `**` stripping may indicate `WildcardMatch` doesn't handle globstar

private static bool BundleSignPatternMatches(string relativePath, string pattern)
{
    if (WildcardMatch(relativePath, pattern))
        return true;
    if (pattern.StartsWith("**/", StringComparison.Ordinal))
        return WildcardMatch(relativePath, pattern.Substring("**/".Length));
    return false;
}

If WildcardMatch already handles ** (matching across directory separators), the second branch is redundant. If it doesn't handle **, then the first call with a **/ pattern won't match nested files anyway, and only the stripped version will. The behavior of WildcardMatch("foo/bar.dll", "**/bar.dll") should be tested explicitly — the existing signing tests may not cover this edge case (a file more than one level deep).

5. `ShouldExcludeHarvestFile` — filename-only match may be surprising for path-qualified patterns

if (WildcardMatch(relative, pattern)
    || WildcardMatch(fileName, pattern)   // ← filename-only fallback
    || (rootlessGlobstar is not null && WildcardMatch(relative, rootlessGlobstar)))

The WildcardMatch(fileName, pattern) branch means "subdir/*.pdb" would try to match just "foo.pdb" against "subdir/*.pdb" — that won't match, so the fallback is harmless in that case. But a bare pattern like "createdump.exe" will silently match "createdump.exe" anywhere in the tree due to the filename branch, which may or may not be the intended semantics. This should be documented in the schema/quickstart so users know bare patterns act as basename globs.

6. Hook step key uniqueness when the same hook fires at multiple targets

AddCommandHookSteps generates step keys like "hook-{id}-{phase}". If a BeforeTargetPublish hook matches multiple targets (e.g., App and Service), will two steps with the same key be added? This could cause conflicts if step keys are used as identifiers downstream. The plan-phase tests cover hook ordering but it's not clear if key collision is tested.

7. CLI phase shift for spec overrides (`ApplyDotNetPublishSpecOverrides`)

// Moved spec override application from plan phase to spec phase (order matters for profile handling)

This is a behavior change for existing CLI users who use --target, --runtime, --framework, or --style flags. The PR description mentions it but the change is not called out in CHANGELOG.MD or docs. If any downstream consumer relies on the previous plan-phase override behavior, this is a silent breaking change. A note in the changelog or migration guide would be appropriate.

8. Two template syntaxes for different contexts

Bundle token substitution uses {token} (single braces, e.g. {output}, {rid}), but GeneratedScripts[].Tokens values can themselves use {bundle-token} references, while the script template body uses {{TokenName}} (double braces). The quickstart explains this, but the asymmetry adds cognitive load. At minimum, a note in the schema description for GeneratedScripts clarifying which syntax applies where would reduce confusion.

📋 Minor / Nits

CopyBundleModules builds a new moduleTokens dictionary via manual copy instead of just adding to a derived dictionary. A small helper or new Dictionary<>(existingTokens) { ["moduleName"] = ... } would be more concise.
Example.PackageBundleMsi.json deserialization is not covered by an automated test. The bundle tests use inline specs. Consider adding a test analogous to DotNetPublishPipelineRunnerBundleTests.CanDeserializeExamplePackageBundleMsiSpec() to catch schema drift in the example file.
Line count discipline (node .\Build\linecount.js . 800): DotNetPublishPipelineRunner.Plan.cs is likely approaching or over 800 lines now given the additions. Worth verifying locally.

🧪 Test Coverage Assessment

Area	Covered	Notes
Hook plan ordering (all 6 phases)	✅	`Plan_AddsCommandHooksAroundPublishAndBundleSteps`
Hook token expansion	✅	`RunCommandHook_ExpandsTokensInArgumentsAndEnvVars`
Bundle CopyItems / ModuleIncludes / GeneratedScripts	✅	`BuildBundle_ComposesPackageWithAllBundlePrimitives`
Signing profile resolution	✅	New bundle signing tests
MSI harvest excludes	✅	`MsiPrepare_AppliesHarvestExcludePatterns`
Hook timeout / deadlock path	❌	Not tested
Template path outside project root	❌	Not tested
`WildcardMatch` with deep `**/` paths	❌	Pattern matching edge cases
`Example.PackageBundleMsi.json` round-trip	❌	Example drift

Summary

The PR is well-structured, architecturally sound, and meaningfully tested. The three issues worth addressing before merge are: (1) the potential deadlock in the net472 timeout path, (2) the missing EnsurePathWithinRoot on TemplatePath, and (3) the CLI behavior change deserving a changelog entry. The remaining items are improvements to consider but not blockers.

claude · 2026-04-28T19:45:17Z

Code Review — PR #331: Add reusable PowerForge publish bundles and hooks

Overview

This is a large, well-structured addition (2,844 lines) that moves bespoke build/package/MSI logic from downstream repos (TierBridge, TestimoX) into PowerForge as reusable engine primitives. The new surfaces — CopyItems[], ModuleIncludes[], GeneratedScripts[], Hooks[], PrimarySubdirectory, HarvestExcludePatterns, bundle post-process signing — are cleanly modelled with spec → plan separation, JSON schema coverage, and solid test coverage. The module export parity test is a good enforcement mechanism.

Specific Issues / Suggestions

1. Local filesystem path left in shipped documentation

Docs/PowerForge.BuildReuse.TestimoXTierBridge.md (line 65) contains:

Branch/worktree: `codex/catalog-reuse` in `C:\Support\GitHub\PSPublishModule-codex-catalog-reuse`

This is a local developer path that will be misleading for anyone reading the published docs. Consider removing or replacing with just the branch name.

2. `FindRepoRoot()` duplicated across three test files

DotNetPublishPipelineRunnerBundleTests.cs, ExportDetectorTests.cs, and PowerForgePluginCatalogServiceTests.cs each define an identical private FindRepoRoot() method. Extract this to a shared TestHelpers or RepoRootLocator utility in the test project to avoid the drift risk.

3. `NormalizeArguments` has dead code after the null filter

private static string[] NormalizeArguments(IEnumerable<string>? values)
{
    return (values ?? Array.Empty<string>())
        .Where(v => v is not null)
        .Select(v => v ?? string.Empty)  // unreachable: v is already non-null here
        .ToArray();
}

The .Select(v => v ?? string.Empty) is unreachable. Either drop the null-coalesce or remove the Where and let the select handle it, matching the style used in NormalizeStrings.

4. Manual dictionary copying could use the copy constructor

In CopyBundleModules and GenerateBundleScripts, tokens are merged manually:

var moduleTokens = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
foreach (var token in tokens)
    moduleTokens[token.Key] = token.Value ?? string.Empty;

This can be written more clearly as:

var moduleTokens = new Dictionary<string, string>(tokens, StringComparer.OrdinalIgnoreCase);

(The copy constructor preserves entries. The ?? string.Empty guard is unnecessary since tokens values are already non-null strings from BuildBundleTokens.)

5. `ConfigLabel` field is semantically overloaded

ModuleBuildPreparedContext.ConfigLabel is documented as a label ("cmdlet", "dsl") but in the new PrepareFromConfig path it carries the full config file path:

ConfigLabel = configFullPath  // e.g. C:\repo\Build\module.build.json

This is then passed to .Execute(preparation, interactive, preparation.ConfigLabel). If downstream consumers ever display or compare this label, the full path will be unexpected. Consider a dedicated ConfigFilePath property for the path and keep ConfigLabel as a short display string, or document the overloaded behaviour explicitly.

6. `GenerateBundleScripts`: `Overwrite=false` throws instead of skipping

if (File.Exists(outputPath) && !script.Overwrite)
    throw new IOException($"Generated script already exists and Overwrite=false: {outputPath}");

The error message (IOException) is appropriate for consumers who treat this as an error, but a user who sets Overwrite=false to mean "skip if present" will be surprised. Clarify in docs/schema whether false means "error on conflict" or "skip silently", and align the behaviour accordingly.

7. `BundleSignPatternMatches` applies globstar stripping on top of wildcard matching

private static bool BundleSignPatternMatches(string relativePath, string pattern)
{
    if (WildcardMatch(relativePath, pattern))
        return true;

    if (pattern.StartsWith("**/", StringComparison.Ordinal))
        return WildcardMatch(relativePath, pattern.Substring("**/".Length));

    return false;
}

If WildcardMatch already handles **/ correctly, the second branch is redundant and could cause unexpected matches (e.g., **/foo.ps1 matching foo.ps1 at the root even if WildcardMatch would not). The Distinct at call-site avoids double-adds, but it's worth confirming whether WildcardMatch needs this fallback or not, and adding a comment either way.

8. `ApplyDotNetPublishSpecOverrides` applies runtimes/frameworks/styles to both profile and target levels

if (activeProfile is not null)
    activeProfile.Runtimes = runtimes;

foreach (var target in targets)
{
    if (target.Publish is null) continue;
    target.Publish.Runtimes = runtimes;
}

When a profile is active, the override is written to both the profile and each target's Publish. If the planner gives precedence to target-level Runtimes over profile-level Runtimes, the profile write is silently ignored (or vice versa). A comment explaining which wins — or applying the override to only one level — would prevent future confusion.

9. `CopyBundlePath` when file exists and `ClearDestination=false`

File.Copy(source, destinationFile, overwrite: clearDestination);

When clearDestination=false and the destination file already exists, File.Copy throws IOException. It's not clear from the API contract whether this is intentional (treat as error) or whether the caller expects a silent skip. A test case covering this path would be valuable.

10. Hook test `RunCommandHook_ExpandsArgumentsWorkingDirectoryAndEnvironment` requires `pwsh`

The test shells out to pwsh and will fail on environments where PowerShell Core is not installed. This is fine for the project's stated CI environment, but consider adding [SkippableFact] (or [ConditionalFact]) guarded by a which pwsh / Get-Command pwsh check if cross-platform CI is ever added.

Positives

Spec → plan separation is clean. New primitives follow the established pattern exactly.
Security: EnsurePathWithinRoot used consistently. All new copy/sign destinations are checked against the bundle root — path traversal is not introduced.
LineEndingsNormalizer fix is correct. The old code left bare CR untouched; the new algorithm (→\n→\r\n) handles all three cases and the new tests verify both directions.
Module export parity test (GeneratedModuleFiles_export_PSPublishModule_plugin_and_bundle_cmdlets) is a great regression guard for the recurring "cmdlet exists in source but not in shipped manifest" class of bugs.
OrderTargetsForBundleIncludes topological sort is correct for the publish-before-bundle ordering requirement.
Schema coverage for all new primitives is complete and uses additionalProperties: false throughout, which prevents silent config typos.
The Build-Module.ps1 exit-code fix is small and correct — CI will now surface self-build failures properly.

Summary

The implementation is solid and the approach is well-reasoned. The main actionable items before merge are: (1) remove the local filesystem path from the docs, (2) de-duplicate FindRepoRoot in tests, and (3) clarify the Overwrite=false semantics. The other points are minor and can be addressed as follow-ups.

claude · 2026-04-28T20:25:15Z

Code Review: PR #331 — Add reusable PowerForge publish bundles and hooks

This is a well-structured, high-quality PR that significantly expands the PowerForge publish pipeline. The additions are coherent, consistently styled, and backed by good test coverage. Below is my detailed feedback.

Overview

The PR delivers:

Bundle composition primitives — PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[]
Command hooks — Hooks[] with six phases (BeforeRestore → AfterBundle), target/runtime/framework/style filters
Bundle post-process signing — PostProcess.Sign / PostProcess.SignProfile / PostProcess.SignPatterns[]
MSI harvest exclusions — HarvestExcludePatterns[]
Invoke-ModuleBuild -ConfigPath — JSON-driven module build parameter set
CLI spec overrides before profile resolution — fix for plan/export divergence vs. executed run
Pre-normalization of line endings before PSScriptAnalyzer — fixes mixed-ending parse failures
Export parity fixes and tests — plugin/bundle cmdlets now exported and verified by test

Code Quality & Style ✅

Consistent naming, null-checking, and StringComparison.OrdinalIgnoreCase usage throughout.
Clone methods (CloneBundleCopyItems, CloneBundleModuleIncludes, CloneBundleGeneratedScripts, CloneCommandHooks) follow the established pattern perfectly.
NormalizeArguments correctly preserves empty strings in argument lists (unlike NormalizeStrings which trims/dedupes), matching .NET ArgumentList semantics.
Public internal surface methods (FindBundleSignTargets, NormalizeBundleSignPatterns, RunCommandHook) are appropriately internal and directly tested.

Specific Observations

`LineEndingsNormalizer.NormalizeEndings` (Bug Fix)

// Old: only handled bare LF, missed bare CR
return text.Replace("\r\n", "\n").Replace("\n", "\r\n");

// New: collapses bare CRs then re-expands
return text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");

This is correct. The old code would convert \r into \r\r\n (replace-chain artifact). New tests NormalizeFile_ToCrLf_ConvertsBareCrAndLf and NormalizeFile_ToLf_ConvertsBareCrAndCrLf validate both directions.

`ApplyDotNetPublishSpecOverrides` (CLI) — minor concern

When a profile is active and overrideRids/overrideFrameworks/overrideStyles are provided, the code updates both the profile and each target's Publish options:

if (activeProfile is not null)
    activeProfile.Runtimes = runtimes;
foreach (var target in targets)
{
    if (target.Publish is null) continue;
    target.Publish.Runtimes = runtimes;  // also mutates each target
}

This double-write is intentional (profile for filtering, target for plan expansion), but it's not documented and could surprise a future reader. A short comment explaining "profile constrains plan filtering; target overrides drive publish expansion" would help.

`OrderTargetsForBundleIncludes` — topological sort

The DFS-based topological sort is correct and handles cycles via the visiting set. One edge case: the sort only models dependencies from PrepareFromTarget → Includes[].Target, not cross-bundle dependencies (Bundle A depends on Bundle B's primary target). That's fine for the current feature scope, but worth noting if bundles depending on bundles are ever needed.

`GenerateBundleScripts` — template security

The EnsurePathWithinRoot(plan.ProjectRoot, templatePath, ...) guard correctly blocks templates outside ProjectRoot. The test BuildBundle_RejectsGeneratedScriptTemplateOutsideProjectRoot covers this. ✅

`RunHookProcess` — timeout kill sequence

if (!process.WaitForExit(timeoutMs))
{
    process.Kill(entireProcessTree: true);  // NET5+
    // …
    var stdout = TryGetCompletedOutput(stdoutTask);
    return (-1, stdout, stderr, fileName);
}

The function returns -1 on timeout (not a real exit code). The caller checks result.ExitCode == 0 and throws if HookRequired. This means a timed-out required hook fails as expected. The -1 sentinel is fine but is not documented. If callers ever need to distinguish "timeout" from "exit code 1," a separate field or enum would be needed. Not a problem now.

`ShouldExcludeHarvestFile` — pattern matching logic

var rootlessGlobstar = pattern.StartsWith("**/", StringComparison.Ordinal) ? pattern.Substring(3) : null;
if (WildcardMatch(relative, pattern)
    || WildcardMatch(fileName, pattern)
    || (rootlessGlobstar is not null && WildcardMatch(relative, rootlessGlobstar)))

The second branch (WildcardMatch(fileName, pattern)) means a bare pattern like createdump.exe (no path separator) matches the filename anywhere under the tree — which is the documented intent. This is consistent with BundleSignPatternMatches. ✅

`CopyBundlePath` — file-into-directory behavior

When the source is a file and the destination path is an existing directory, the code copies the file into that directory using Path.GetFileName(source) as the file name. This is convenient but slightly surprising if DestinationPath was intended as a file path that happens to collide with a leftover directory. The ClearDestination=true default removes the directory first, so this is only reachable when ClearDestination=false. Given the narrow use case, this is acceptable.

Potential Issues

PrepareFromConfig reads spec.Build.Version from the PSD1 but falls back to ResolveBaseVersion(SourcePath, Name) when Version is omitted. The private overload ResolveBaseVersion(string, string) (no segments) is a new private method added in this PR. Make sure it doesn't silently return "0.0.1" or similar default when the PSD1 is missing a ModuleVersion. (This is internal behaviour but worth a quick check if the JSON config path is meant to be used without a DSL version segment.)
Hook BeforeBundle / AfterBundle are added once per target combination, not once per bundle. If a spec has 2 targets that both include the same bundle, and there's an AfterBundle hook with no target filter, the hook fires twice. This is arguably correct (it fires once per bundle-step, which appears once per triggering target/combo), but documentation should call this out for hook authors who assume bundle-scoped hooks run exactly once.
RepoRootLocator.Find() traverses up to 12 levels looking for PowerForge/PowerForge.csproj. This is fine for CI but could be slow or misdirected on deeply nested build trees. Hard-limit + clear error message is already in place — no action needed.
ModuleBuildPreparationService.ReadPipelineSpecJson catches all exceptions except FileNotFoundException. If the JSON contains an unknown property and PropertyNameCaseInsensitive is true but UnknownPropertyHandling throws, the error message wraps it correctly. One scenario that isn't caught: JsonException from a truncated file produces a reasonable wrapped error. ✅

Test Coverage ✅

New tests cover:

DotNetPublishPipelineRunnerBundleHardeningTests — path-escape and conflict-detection for copy items
DotNetPublishPipelineRunnerHookTests — phase ordering, key uniqueness, argument/env token expansion, pwsh integration test
DotNetPublishPipelineRunnerMsiPrepareTests.PrepareMsiPackage_ExcludesConfiguredHarvestPatterns — harvest filter
DotNetPublishPipelineRunnerBundleTests — deserialization of example, primitive normalization, signing profile resolution, step ordering, copy/module/script execution
LineEndingsNormalizerTests — both CRLF and LF normalization
ModuleBuildPreparationServiceTests.Prepare_from_config_loads_pipeline_json_and_resolves_paths
ExportDetectorTests.GeneratedModuleFiles_export_PSPublishModule_plugin_and_bundle_cmdlets — regression guard for export parity

The integration hook test (RunCommandHook_ExpandsArgumentsWorkingDirectoryAndEnvironment) invokes pwsh and is OS-sensitive; it will be skipped or fail on agents without pwsh. Consider adding a [Trait("Category","Integration")] or [SkippableFact] guard if this runs in a Linux CI environment without PowerShell Core.

Minor Suggestions

Schema: DotNetPublishCommandHook.TimeoutSeconds minimum is 1, but NormalizeCommandHooks defaults TimeoutSeconds <= 0 to 600. The schema could set minimum: 0 and document that 0 means "use default," or the code could reject 0 at validation. Currently a user who writes "TimeoutSeconds": 0 gets a schema error but not a clear "use default" message.
FormattingPipeline.cs: The preNormalize list is built by iterating list, and later looked up via FirstOrDefault by path (O(n²)). For the typical number of files this is fine, but a Dictionary<string, NormalizationResult> would be cleaner.
Build/Build-Module.ps1: The exit $LASTEXITCODE propagation fix is correct and important for CI. ✅

Summary

This is a high-quality, production-ready contribution. The architecture is sound, the feature surface is well-bounded, and the test coverage is thorough. The two items most worth a follow-up are:

Document (or test) the "hook fires once per publish step, not once per bundle" semantic for BeforeBundle/AfterBundle.
Consider a guard or [SkippableFact] on the pwsh integration test for CI agents that don't have PowerShell Core.

Everything else is either correct as-is or nits. Happy to approve after the above are acknowledged/addressed.

claude · 2026-04-28T20:35:06Z

Code Review — PR #331: PowerForge publish bundles and hooks

Overview

Substantial, well-structured addition that moves reusable package/MSI build mechanics out of downstream repos and into the PowerForge engine. The change surface is large but logically cohesive: six new bundle composition primitives, a command-hook system with six pipeline phases, harvest exclusion patterns, bundle post-process signing, target-ordering for bundle includes, and a pre-normalization fix for mixed line endings before PSScriptAnalyzer.

Code Quality and Style

Strengths

Partial-class split (Bundle.cs, Hooks.cs, MsiPrepare.cs, Plan.cs, Run.cs) keeps each concern readable.
Consistent null-guarding on array inputs throughout (e.g., ?? Array.Empty<T>()).
Path-traversal protection via EnsurePathWithinRoot is applied to every new output path (copy destinations, generated script output, generated script template source). The security test in DotNetPublishPipelineRunnerBundleHardeningTests confirms this.
NormalizeCommandHooks validates duplicate IDs and missing Command eagerly at plan time rather than at run time.
Schema is fully updated to match all new model classes, with additionalProperties: false keeping configs strict.

Minor style inconsistencies

NormalizeCommandHooks throws ArgumentException("Hooks[].Id is required.") (JSON-path style) while the companion check throws ArgumentException($"Hook '{id}' requires Command.") (name style). Small but worth aligning.
AddCommandHookSteps recomputes the timeout guard (hook.TimeoutSeconds <= 0 ? DefaultCommandHookTimeoutSeconds : Math.Max(1, hook.TimeoutSeconds)) even though NormalizeCommandHooks already guarantees the value is ≥ 1 when hooks reach this point. Not a bug, but the redundant check can be simplified to hook.TimeoutSeconds.

Potential Bugs / Issues

1. Inconsistent basename matching between harvest exclude and bundle signing

ShouldExcludeHarvestFile matches a pattern against both the relative path and the bare filename component:

|| WildcardMatch(relative, pattern)
|| WildcardMatch(fileName, pattern)          // bare filename match anywhere
|| (rootlessGlobstar is not null && ...)

BundleSignPatternMatches only matches the relative path (with optional **/ prefix stripping):

if (WildcardMatch(relativePath, pattern)) return true;
if (pattern.StartsWith("**/")) return WildcardMatch(relativePath, pattern.Substring(3));

A pattern like createdump.exe (no path separators, no wildcards) will exclude the file from WiX harvest at any depth, but will not match it for bundle signing unless it happens to be at the root. The test PrepareMsiPackage_ExcludesConfiguredHarvestPatterns covers createdump.exe for harvest but there is no equivalent test showing the same bare name works as a sign pattern in FindBundleSignTargets. The docs say both surfaces accept "basename globs", so the signing path should match ShouldExcludeHarvestFile's logic, or the difference should be explicitly documented.

2. ClearDestination=false error message quality

When clearDestination=false and the destination file exists, File.Copy(source, dest, overwrite: false) throws a generic IOException with a framework message like "The file '...' already exists." The PR description says ClearDestination=false means "fail if destination already exists, not skip". A custom InvalidOperationException with the bundle ID and paths included would make CI logs far easier to read.

3. OrderTargetsForBundleIncludes: cycle detection throws but is untested

The local Visit function throws ArgumentException("Bundle include target dependency cycle detected at '…'") on a cycle. There is no test for this path. A cycle can be authored in config (target A's bundle includes target B, target B's bundle includes target A), and a missing test means the error message and throw-site haven't been validated.

Performance Considerations

FindBundleSignTargets is O(files × patterns)

For each pattern, the method does a full Directory.EnumerateFiles(root, "*", SearchOption.AllDirectories) walk. A single directory walk that tests all patterns per file would be O(files × patterns) without the repeated I/O overhead. For typical bundle sizes this is unlikely to matter, but it is worth noting if bundles ever contain tens of thousands of files.

Security

The new surfaces were reviewed and look sound:

Template file paths are validated against ProjectRoot before reading — path traversal is blocked and tested.
Copy destinations are validated against the bundle output root.
Hook Command and Arguments are expanded from user-controlled config but that is the intended contract for a build tool; users who control the config control the build machine.
RunHookProcess uses ArgumentList on modern runtimes (no shell injection) and falls back to BuildWindowsArgumentString on NET472.

Test Coverage

Good coverage overall. The following gaps are worth considering before shipping:

Gap	Suggested test
`BundleSignPatternMatches` with a bare basename pattern (no slashes, no globs)	`FindBundleSignTargets` with `createdump.exe` pattern against a nested file
`NormalizeCommandHooks` validation — duplicate IDs and missing `Command`	Two `ArgumentException` tests in a new hooks-normalizer test class
`OrderTargetsForBundleIncludes` cycle	A spec with two bundles whose includes form a cycle, assert `ArgumentException`
`ClearDestination=false` on a module include	Parallel to the existing `CopyItem` hardenening test

Specific Suggestions

// AddCommandHookSteps — simplify redundant guard (hook is already normalized)
HookTimeoutSeconds = hook.TimeoutSeconds,   // was: hook.TimeoutSeconds <= 0 ? Default : Math.Max(1, ...)

// Align error message style in NormalizeCommandHooks
if (string.IsNullOrWhiteSpace(id))
    throw new ArgumentException("Hooks[].Id is required.");
if (string.IsNullOrWhiteSpace(hook.Command))
    throw new ArgumentException($"Hooks['{id}'].Command is required.");  // consistent JSON-path style

// CopyBundlePath — surface a more actionable error when ClearDestination=false conflicts
if (!clearDestination && File.Exists(destinationFile))
    throw new IOException($"{description}: destination already exists and ClearDestination=false: {destinationFile}");
File.Copy(source, destinationFile, overwrite: clearDestination);

Documentation

Docs are thorough and accurate. The double-brace / single-brace distinction for template tokens is clearly explained in the quickstart. The BuildReuse.TestimoXTierBridge.md migration guide is unusually helpful context for future maintainers.

Summary

This is well-engineered work. The three items that are worth addressing before merge are:

Verify or align BundleSignPatternMatches vs ShouldExcludeHarvestFile basename semantics (likely a real behavioural inconsistency).
Add a cycle-detection test for OrderTargetsForBundleIncludes.
Optionally improve ClearDestination=false error message quality for CI debugging.

Everything else is either correct, cosmetic, or a future-nice-to-have. The hook system, bundle primitives, and signing integration are clean additions that follow the existing patterns of the codebase.

claude · 2026-04-28T20:47:33Z

PR Review: Add reusable PowerForge publish bundles and hooks

Overall Assessment: This is a well-structured, substantial PR that extracts reusable build/publish patterns from TierBridge and TestimoX into a shared PowerForge foundation. The design is sound, security guards are present, and test coverage is reasonable. I have a few specific findings worth addressing before merge.

Strengths

Path traversal protections are solid. EnsurePathWithinRoot() is enforced on bundle output paths, template source paths, and generated script outputs, with dedicated tests validating the rejections (BuildBundle_RejectsGeneratedScriptTemplateOutsideProjectRoot, etc.).
Command hook process handling is safe. ProcessStartInfo.ArgumentList is used for .NET Core (no shell interpretation), with graceful stderr/stdout tail on failure and a configurable process timeout.
Bundle composition order is deterministic. Primary → Includes → CopyItems → ModuleIncludes → GeneratedScripts → Scripts → PostProcess — unambiguous and easy to reason about.
Cycle detection in target ordering. Topological sort with a visiting set in OrderTargetsForBundleIncludes() prevents infinite loops when bundles include each other.
Export parity tests. ExportDetectorTests checking that binary-detected cmdlets match the .psd1 manifest is a great guard against silent export drift.
CLI override timing fix. Moving ApplyDotNetPublishSpecOverrides() to before profile resolution (pre-planning) is correct; the changelog entry and docs update are consistent with the code change.

Specific Concerns

1. Token expansion — recursive substitution risk

In GenerateBundleScripts, tokens are resolved with single-brace expansion first, then passed to the {{...}} template renderer. If a token value itself contains {{...}}, it will be re-expanded in the second pass. This could produce unexpected output or, with user-controlled values, a handy way to inject template tokens that were not intended. Worth either:

Documenting explicitly that token values are not re-expanded (and verifying that in code), or
Adding a test with a token value containing {{nested}} to confirm it is treated as a literal.

2. `FindBundleSignTargets` — unbounded recursive enumeration

Directory.EnumerateFiles(bundleRoot, "*", SearchOption.AllDirectories)

For large bundles the full recursive scan runs on every post-process signing call. Consider filtering by extension early (e.g. *.ps1, *.dll, *.exe, *.psd1) before pattern matching, rather than enumerating everything and discarding. Not a blocker for current use, but worth noting if bundle sizes grow.

3. Process stream cleanup — hardcoded 5-second `Task.WaitAll` after kill

In RunHookProcess (the timeout path), after killing the process the code does:

Task.WaitAll([stdoutTask, stderrTask], TimeSpan.FromSeconds(5));

The 5-second drain is reasonable but undocumented and not configurable. If the hook emits huge stderr/stdout just before being killed, 5 seconds may be insufficient to capture the tail that gets logged. A comment explaining the intent would help future readers.

4. `Process.Start()` return value — already handled, but could be clearer

The null-coalescing throw after Process.Start() is correct but the null path is unreachable when UseShellExecute = false. A brief comment or removing the throw in favour of ! (since it can't be null there) would reduce reader confusion.

5. Module export: three newly exported cmdlets

Invoke-PowerForgePluginExport, Invoke-PowerForgePluginPack, and Invoke-PowerForgeBundlePostProcess are now in the manifest. The parity test is great. Confirm the AliasesToExport list in PSPublishModule.psd1 is intentional (some alias additions visible in the diff) — aliases create sticky API surface.

6. Hook environment variable values — secret leakage surface

Environment variables for hooks are set from the spec JSON (hook.Environment). If a token substitution ({version}, {outputDir}, etc.) ends up in a value that is later logged in verbose/debug output, secrets embedded in the pipeline config could be echoed. The current logging redacts args but environment values may not be redacted. Worth confirming that hook environment values are not logged verbatim in any diagnostic output.

Minor / Nits

ClearDestination=false exception message says "destination" twice in some paths — consider normalising the wording.
The new RepoRootLocator.cs in tests is a nice utility; if it's only used in one place, a static helper method in the test class would also work, but no strong opinion.
Docs/PowerForge.BuildReuse.TestimoXTierBridge.md references "Phase 2 / Phase 3" future work. Worth marking these sections with a note that they are not implemented in this PR so readers don't assume the doc is already implemented.

Test Coverage Assessment

Coverage is solid for the new paths:

✅ Hook planning and validation (7 tests)
✅ Bundle hardening — path traversal and conflict (4 tests)
✅ Bundle composition — copy, modules, generated scripts (4 tests)
✅ Sign target matching, harvest exclusion
✅ Export parity, config file loading, example deserialization

Gaps worth adding (not blocking):

Token value containing {{...}} passed through the two-pass expansion (see concern Release version number does not automatically update #1)
Hook with an environment variable that contains a token — verifies substitution and confirms no logging leakage
FindBundleSignTargets called on a directory with 0 matching files (edge case: nothing to sign)

Summary

The overall design follows the repo's working agreement well: thin cmdlets, reusable logic in PowerForge/PowerForge.PowerShell, no business logic pushed into PSPublishModule\Cmdlets\. The token expansion concern (#1) is the one I'd want a clear answer on before shipping, since it's a subtle correctness/security edge. The rest are improvements or documentation clarifications.

claude · 2026-04-28T20:57:51Z

Code Review: PR 331 - Add reusable PowerForge publish bundles and hooks

Large, well-structured PR (+3423/-64) adding new engine primitives: Hooks[] (declarative command hooks at six publish phases), Bundles[].PrimarySubdirectory, Bundles[].CopyItems[], Bundles[].ModuleIncludes[], Bundles[].GeneratedScripts[], PostProcess.Sign/SignProfile/SignPatterns, Installers[].HarvestExcludePatterns[], Invoke-ModuleBuild -ConfigPath (new JSON-driven parameter set), and a Build-Module exit-code propagation fix. The design document is unusually thorough.

Positives

Strong test coverage across happy paths, error paths, and security boundaries. The 101+ tests cited hold up.
Path traversal hardened. EnsurePathWithinRoot guards template paths, copy destinations, and module include destinations. BuildBundle_RejectsGeneratedScriptTemplateOutsideProjectRoot is a good security regression anchor.
Token rendering is deliberately non-recursive. BuildBundle_GeneratedScriptTokenValuesAreNotRenderedRecursively locks in the behaviour - preventing injection via token values.
RepoRootLocator is a clean, reliable test helper.
Module manifest and bootstrapper parity test (GeneratedModuleFiles_export_PSPublishModule_plugin_and_bundle_cmdlets) will catch cmdlets that exist in source but disappear from the shipped module.

Issues

Medium: ApplyDotNetPublishSpecOverrides mutates shared spec on CLI override path

In Program.Helpers.RunAndParse.cs the new function edits spec.Targets and activeProfile in-place before runner.Plan(spec, ...). The old ApplyDotNetPublishPlanOverrides operated on the immutable plan output. Mutating the spec means a second call with the same spec object (e.g. in plan-then-run flow) sees the overridden values, and profile filters are permanently changed even when -Plan + -ValidateOnly are combined. Consider cloning the relevant parts before mutation, or documenting that the function is destructive.

Low: Multi-style CLI override silently drops profile style filter

When the user passes --style AotSpeed --style PortableCompat, styles.Length == 1 ? styles[0] : null sets the profile Style to null (no filter) while targets get both styles expanded. This may be intentional but no warning is emitted and the documentation does not describe this edge case. A comment or log warning would prevent future confusion.

Low: GenerateBundleScripts reads template with BOM-aware Encoding.UTF8

If a template file has a UTF-8 BOM, the BOM bytes appear at the start of the rendered script, producing an invisible character that can break PowerShell #Requires detection. Prefer new UTF8Encoding(encoderShouldEmitUTF8Identifier: false) for the read as well to strip any BOM from input.

Low: RepoRootLocator depth limit is a magic constant

The loop caps at 12 directory levels without explanation. A brief inline comment or named constant would clarify intent.

Nit: CopyBundleModules allocates a full token dictionary copy per module include - fine at typical bundle sizes.

Security

Path traversal for template paths and copy destinations is blocked by EnsurePathWithinRoot and tested.
Hook Command and Arguments are passed directly to Process.Start without shell expansion by PowerForge. Worth noting in the docs that Command should be a trusted path, not user-supplied data, for consumers building wrapper tooling.
Token values are inserted literally into rendered scripts with no escaping of PowerShell-special characters. Appropriate for a build tool where config is developer-controlled, but the quickstart could note this for consumers who surface token values from user-editable config files.

Summary

Production-quality work. The implementation is clean, security boundaries are tested, and the design doc makes the migration path clear. The medium issue around in-place spec mutation is worth addressing before merge to avoid subtle bugs in multi-phase CLI flows; the rest are low-priority or stylistic. No blocking issues found.

claude · 2026-04-28T21:10:03Z

Code Review — PR #331: Add reusable PowerForge publish bundles and hooks

This is a well-structured PR that adds significant new infrastructure to PowerForge. The implementation is generally solid, with good test coverage, proper security hardening on path operations, and clean cross-platform compatibility handling. Notes below are organized by area.

Overview

The PR delivers five related features:

Command hooks (Hooks[]) injected at six publish phases.
Bundle composition primitives: PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[].
Bundle post-process signing via PostProcess.Sign / PostProcess.SignPatterns[].
WiX harvest exclusion via HarvestExcludePatterns[].
Formatting pre-normalization — bare \r (classic Mac) and mixed line endings now normalized before PSScriptAnalyzer.

All five features are documented in the quickstart and schemas, and Build/Build-Module.ps1 now propagates its exit code to CI.

Strengths

Security: path traversal is defended. Every user-controlled path (copy items, module includes, generated script output, template paths) goes through EnsurePathWithinRoot before any filesystem write. This is the right pattern for a pipeline that accepts config from JSON files.
Command injection risk is low. Hook commands are executed via ProcessStartInfo.FileName + ArgumentList (not shell string expansion) on modern .NET. On .NET 472 the BuildWindowsArgumentString helper is used. Environment values are set directly in psi.Environment, not interpolated into a command string.
Clone coverage restored. CloneBundlePostProcessOptions was not called in the old CloneBundles, meaning profile resolution could share a live PostProcess object across profile copies. Adding it here is a correctness fix, not just feature support.
LineEndingsNormalizer fix is correct. The old implementation skipped bare \r (CR-only, old Mac line endings). The new path:
```
text.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n")
```
handles all three variants correctly before PSScriptAnalyzer sees the file.
ScriptTemplateRenderer is improved. Switching from iterative string.Replace (O(n × tokens)) to Regex.Replace with a match evaluator (O(n)) is a clean improvement. The old "unresolved tokens after replacement" second pass is intentionally dropped, and the behavior is now correctly documented: token values are inserted literally and are not re-rendered.
Test coverage is solid. New test classes cover hook ordering, hook step deduplication, timeouts, bundle hardening (path escapes, missing sources, overwrite conflicts), line endings (CRLF/LF/CR, BOM variants), MsiPrepare exclusion patterns, and the module export parity check.

Issues and Suggestions

Medium

1. DotNetPublishStep is growing wide (DotNetPublishPipelineRunner.Plan.cs)

The step now carries 8 hook-specific properties (HookId, HookPhase, HookCommand, HookArguments, HookWorkingDirectory, HookEnvironment, HookTimeoutSeconds, HookRequired) alongside properties for every other step kind. If a third or fourth step variant lands, this flat bag will become hard to navigate. Consider a sealed discriminated structure:

public DotNetPublishCommandHookPayload? Hook { get; set; }

…containing the hook fields, populated only when Kind == CommandHook. Non-breaking now; harder to retrofit later.

2. GenerateBundleScripts — silent null from Path.GetDirectoryName (DotNetPublishPipelineRunner.Bundle.cs)

Directory.CreateDirectory(Path.GetDirectoryName(outputPath)!);

GetDirectoryName returns null for root paths (e.g. C:\) and Directory.CreateDirectory(null!) will throw a NullReferenceException rather than an ArgumentNullException, hiding the cause. A guard here keeps the error message actionable:

var dir = Path.GetDirectoryName(outputPath)
    ?? throw new InvalidOperationException($"Cannot determine parent directory of '{outputPath}'.");
Directory.CreateDirectory(dir);

3. DefaultCommandHookTimeoutSeconds constant vs model default are decoupled (DotNetPublishPipelineRunner.Plan.cs, DotNetPublishSpec.cs)

DotNetPublishCommandHook.TimeoutSeconds defaults to 600 in the model, and DefaultCommandHookTimeoutSeconds = 600 is a separate constant in Plan.cs. If one is changed without the other they diverge silently. The plan code should reference the model default, or the constant should live next to the model and be referenced from both.

Minor

4. Redundant branch in BundleSignPatternMatches (DotNetPublishPipelineRunner.Bundle.cs)

if (WildcardMatch(relativePath, pattern)) return true;
var fileName = Path.GetFileName(relativePath);
if (WildcardMatch(fileName, pattern)) return true;
if (pattern.StartsWith("**/", StringComparison.Ordinal))
    return WildcardMatch(relativePath, pattern.Substring("**/".Length));
return false;

For a pattern like **/*.dll, the third branch tries WildcardMatch(relativePath, "*.dll"), which matches only files in the root of the bundle. The first branch already handles the **/ glob if WildcardMatch implements globstar. If WildcardMatch does not implement globstar natively, the third branch is the only thing that makes **/ patterns work for subdirectories — which would make the first branch redundant for globstar patterns. The behavior should be documented or a comment added to explain which branch handles which case, since the interaction between them is non-obvious.

5. CopyBundleModules creates a new dictionary per module (DotNetPublishPipelineRunner.Bundle.cs)

var moduleTokens = tokens.ToDictionary(t => t.Key, t => t.Value, StringComparer.OrdinalIgnoreCase);
moduleTokens["moduleName"] = module.ModuleName;

This is fine for typical module-include counts, but worth noting if the token dictionary grows large. An alternative is a lightweight overlay (a two-level lookup) that avoids copying. Not a blocking issue.

6. TrimMatchingQuotes strips only double quotes (DotNetPublishPipelineRunner.Hooks.cs)

Single-quoted paths (less common on Windows but valid in PowerShell config) would not be stripped. Documenting this limitation in the method's context (or in the schema description for Command) prevents user surprise when a single-quoted path fails to resolve.

7. Whitespace churn in DotNetPublishPipelineRunner.Run.cs

The diff reformats the Restore and Clean case arms (indentation change) alongside the functional CommandHook addition. This mixes cosmetic and functional diffs, which makes the functional change slightly harder to isolate in git blame.

No Issues Found

OrderTargetsForBundleIncludes (referenced but not in diff) — the name and position in the call sequence (after BuildBundlePlans, before BuildInstallerPlans) make the intent clear. Presumed correct based on the hook-ordering tests passing.
Hook filtering — empty Targets[], Runtimes[], Frameworks[], Styles[] arrays correctly match everything; non-empty arrays filter correctly. Verified in the hook ordering tests.
Exit-code propagation in Build/Build-Module.ps1 — simple and correct.
Module export additions (PSPublishModule.psd1 / .psm1) — the new ExportDetectorTests.GeneratedModuleFiles_export_PSPublishModule_plugin_and_bundle_cmdlets test creates a regression guard so these can't silently fall out of the manifest again.

Summary

The implementation is ready to merge with the medium issues addressed or tracked. Issue #2 (GetDirectoryName null guard) is the only one that could surface as a confusing runtime error in a real workflow; the others are maintainability concerns. Excellent test coverage and documentation for a large feature addition.

claude · 2026-04-28T21:15:40Z

Code Review: PR #331 — Add reusable PowerForge publish bundles and hooks

Overview

This is a large, well-scoped PR (+3440/-65) that adds four major capabilities to the PowerForge publish pipeline:

Command hooks — declarative Hooks[] executed at fixed pipeline phases (BeforeRestore, BeforeBuild, BeforeTargetPublish, AfterTargetPublish, BeforeBundle, AfterBundle)
Bundle composition primitives — PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[]
Bundle post-process signing — PostProcess.SignProfile / PostProcess.Sign / PostProcess.SignPatterns[]
MSI harvest exclude patterns — HarvestExcludePatterns[] on installers
Line endings hardening — pre-normalise mixed line endings before PSScriptAnalyzer, propagate Build-Module.ps1 exit codes to CI

The design is clean and follows established patterns in the codebase well. Most of the feedback below is minor.

Positives

Test coverage is excellent. Every new primitive has unit tests covering happy path, error cases, boundary cases (ClearDestination=false, path-traversal guard, cycle detection, token non-recursion), and integration-style round-trip tests against actual example JSON files.
Security boundaries are enforced. EnsurePathWithinRoot guards template file paths, copy-item destinations, module-include destinations, and generated-script output paths.
Token non-recursion is intentional and tested. The PR doc and test BuildBundle_GeneratedScriptTokenValuesAreNotRenderedRecursively both confirm that token values containing {{...}} are not expanded a second time. Good.
CloneDotNetPublishSpec + ApplyDotNetPublishSpecOverrides (CLI) correctly moves overrides before Plan() so plan/export output matches what the runner executes — a real correctness fix.
Cycle detection in OrderTargetsForBundleIncludes is a nice proactive guard.
Graceful timeout handling in RunHookProcess: kills the process tree on net6+, drains streams with a bounded 5-second wait after kill, and returns whatever partial output completed.

Issues / Suggestions

1. `ScriptTemplateRenderer.Render` — error handling regression on unknown tokens

Before this PR, calling Render with a template that references a token not present in the map threw InvalidOperationException("Template '...' contains unresolved token(s): ...").

After this PR, the replacement is:

text = TokenRegex.Replace(text, match => map[match.Groups[1].Value]);

If a token appears in the template but is not in map, this throws a KeyNotFoundException from the dictionary indexer — losing the context-rich error message. The pre-check that warns about missing tokens was removed. Consider restoring the missing-token validation before the Replace call, or at minimum switching to map.TryGetValue with a meaningful fallback/exception.

2. `FindBundleSignTargets` — duplicate pattern matching

private static bool BundleSignPatternMatches(string relativePath, string pattern)
{
    if (WildcardMatch(relativePath, pattern)) return true;
    var fileName = Path.GetFileName(relativePath);
    if (WildcardMatch(fileName, pattern)) return true;          // ← may double-match basename patterns
    if (pattern.StartsWith("**/", ...))
        return WildcardMatch(relativePath, pattern.Substring(3));
}

A bare basename pattern like createdump.exe will match via WildcardMatch(fileName, pattern) for every file named createdump.exe, which is correct. But it also tries WildcardMatch(relativePath, "createdump.exe") first (which fails for nested files), so the logic is safe. However, the **/* glob path also goes through WildcardMatch(fileName, "*.ps1") after already matching WildcardMatch(relativePath, "**/*.ps1"). The deduplication at the end (Distinct) handles the double-match, so this is not a bug — but the matching logic is harder to reason about than it needs to be. A comment explaining the three-path matching strategy would help future maintainers.

3. `RunHookProcess` — timeout returns exit code -1 silently

When a hook times out, the function returns exit code -1. The caller in RunCommandHook then falls into the error-reporting branch, which emits the tail of stderr/stdout as the failure message. This means a timeout looks identical to a process that exited with code -1. A dedicated timeout exception (or at minimum a distinct message like "Hook '{id}' timed out after {timeout}s.") would improve CI diagnostics.

4. `ApplyDotNetPublishSpecOverrides` — multiple framework override sets both `.Framework` and `.Frameworks`

target.Publish.Framework = frameworks[0];
target.Publish.Frameworks = frameworks;

When --framework net10.0 net8.0 is passed, Framework (singular) is set to only net10.0. If the planner expands from Frameworks (plural), this is fine; but if code paths elsewhere read Framework expecting the full set, the first-element truncation could surprise users. Worth a brief comment documenting that Framework is set for back-compat and Frameworks drives expansion.

5. `ModuleBuildPreparationService.PrepareFromConfig` — string-literal parameter set name

if (string.Equals(request.ParameterSetName, "Config", StringComparison.Ordinal))
    return PrepareFromConfig(request);

The constant ParameterSetConfig = "Config" is defined in InvokeModuleBuildCommand.cs, but the service uses a string literal. A mismatch would silently fall through to the wrong code path. Consider moving the constant to a shared location, or passing the resolved parameter set as an enum/flag.

6. `FormattingPipeline` — pre-normalisation result lookup is O(n²)

var preNormalizeResult = preNormalize.FirstOrDefault(x =>
    string.Equals(x.Path, f, StringComparison.OrdinalIgnoreCase));

preNormalize is a List<NormalizationResult> and the lookup happens inside foreach (var f in list). For large module files this is quadratic. The existing preResult and pssaResult lookups have the same pattern — so this isn't new — but since you're touching the method, consider a Dictionary<string, NormalizationResult> keyed by path for all three lookups.

7. `DotNetPublishCommandHook.Validate` — `Hooks[].Id` error message omits array index

throw new ArgumentException("Hooks[].Id is required.");

When an unnamed hook fails, the error gives no way to identify which entry failed. Emitting the array index (e.g. Hooks[2].Id is required) would make the error actionable.

8. `Example.PackageBundleMsi.json` — `GeneratedScripts[].Template` uses `\r\n` literals

"Template": "Import-Module \"$PSScriptRoot\\Modules\\{{ModuleName}}\\{{ModuleName}}.psd1\" -Force\r\n{{InstallCommand}}\r\n"

The \r\n sequences are JSON escape sequences, so they will literally be CR LF in the rendered script. This is intentional for Windows packaging, but the template string already has \\ (literal backslash) and embedded quotes, making it fragile and hard to read. For a published example, a TemplatePath pointing to a .ps1.template file would be clearer and easier to maintain.

Minor / Nit

RepoRootLocator.Find() walks up to MaxSearchDepth = 12 directories looking for PowerForge/PowerForge.csproj. This is fine for tests, but the magic number and sentinel path could move to a constant with a comment.
NormalizeArguments preserves null-argument entries as empty strings rather than filtering them. This seems intentional (hooks need positional args), but a comment would help.
The CHANGELOG.MD "Unreleased" section describes CLI override ordering, but does not mention the new Hooks[], CopyItems[], ModuleIncludes[], or GeneratedScripts[] features. Worth updating before merge.
Several new test classes (DotNetPublishPipelineRunnerBundleHardeningTests, DotNetPublishPipelineRunnerHookTests) duplicate the CreateTempRoot / TryDelete / CreateProject helpers. These could be extracted into a shared TestFixtureHelpers class, but this is consistent with the existing test style so it is not a blocker.

Summary

This is a well-implemented, thoroughly tested feature addition. The main actionable items are:

Fix the ScriptTemplateRenderer missing-token error message regression — the current KeyNotFoundException from the dict indexer is a step back in debuggability.
Distinguish hook timeout from non-zero exit in the error message for CI.
Update CHANGELOG.MD to mention all new features, not just the CLI override fix.

The rest of the feedback is low-priority polish. Ship when items 1 and 2 are addressed.

claude · 2026-04-28T21:23:25Z

Code Review: PR #331 — Add reusable PowerForge publish bundles and hooks

Overview

This is a large, well-scoped PR that extracts shared build/package/MSI mechanics from TierBridge and TestimoX into PowerForge reusable primitives. The additions are:

Hooks[] — declarative command hooks at six publish phases (BeforeRestore, BeforeBuild, BeforeTargetPublish, AfterTargetPublish, BeforeBundle, AfterBundle)
Bundles[].CopyItems[] / ModuleIncludes[] / GeneratedScripts[] / PrimarySubdirectory — richer bundle composition
PostProcess.Sign* — bundle post-process signing
Installers[].HarvestExcludePatterns[] — WiX harvest filtering
Invoke-ModuleBuild -ConfigPath — JSON-driven module build parameter set
Line-endings pre-normalization before PSScriptAnalyzer and CI exit-code propagation

The design rationale document (PowerForge.BuildReuse.TestimoXTierBridge.md) is clear and the phased migration plan is sensible.

Code Quality and Style

Positive patterns throughout:

Every public-facing addition has a corresponding JSON schema entry with additionalProperties: false, which is exactly right for catching typos early.
Null/empty guards are consistently applied in NormalizeCommandHooks, BuildBundlePlans, etc.
The CloneDotNetPublishSpec round-trip-via-JSON approach before applying CLI overrides is clean and avoids aliasing bugs.
NormalizeArguments correctly preserves empty strings (unlike NormalizeStrings which filters them), which matters for CLI argument lists.

One nit on BundleSignPatternMatches:

private static bool BundleSignPatternMatches(string relativePath, string pattern)
{
    if (WildcardMatch(relativePath, pattern)) return true;

    var fileName = Path.GetFileName(relativePath);
    if (WildcardMatch(fileName, pattern)) return true;

    if (pattern.StartsWith("**/", StringComparison.Ordinal))
        return WildcardMatch(relativePath, pattern.Substring("**/".Length));

    return false;
}

The third branch ("**/".Length strip) is a dead path in practice because the first WildcardMatch(relativePath, pattern) already handles **/*.dll → lib/foo.dll correctly if WildcardMatch honours globstar. The test passes without it. Not wrong, just redundant. Worth a comment or removal to avoid confusion.

Correctness

Line-endings normalizer fix is correct. The old code text.Replace("\r\n", "\n").Replace("\n", "\r\n") missed bare \r (old Mac CR). The new three-step Replace("\r\n","\n").Replace("\r","\n").Replace("\n","\r\n") handles all three variants. The new LineEndingsNormalizerTests validate this.

CLI override ordering is a real correctness fix. Moving from ApplyDotNetPublishPlanOverrides (post-plan) to ApplyDotNetPublishSpecOverrides (pre-plan) ensures that profile-filtered plan and export output is consistent with what the runner actually executes. The motivation is clearly documented and the approach is correct.

ScriptTemplateRenderer refactor from sequential string.Replace in a loop to a single-pass Regex.Replace with a callback is strictly better: it is O(n) in template length rather than O(n·tokens), it correctly prevents recursive substitution (a token value containing {{OtherToken}} is not re-expanded), and the test BuildBundle_GeneratedScriptTokenValuesAreNotRenderedRecursively explicitly verifies this.

ApplyDotNetPublishSpecOverrides style handling for multiple styles:

if (activeProfile is not null)
    activeProfile.Style = styles.Length == 1 ? styles[0] : null;

When the caller passes multiple --style values with an active profile, activeProfile.Style is set to null, which is silently permissive. This matches the doc comment ("Profile style is single-valued; multiple CLI styles are represented on targets for planner expansion"), but if a user passes --style AotSpeed AotSize with a profile, the profile constraint is effectively dropped for style. Consider a warning log in this case so the behaviour is visible.

Potential Issues

1. CopyBundlePath: directory destination type mismatch (minor)

if (Directory.Exists(source))
{
    if (clearDestination && Directory.Exists(destination))
        Directory.Delete(destination, recursive: true);
    else if (clearDestination && File.Exists(destination))
        File.Delete(destination);
    else if (!clearDestination && (Directory.Exists(destination) || File.Exists(destination)))
        throw new IOException(...);

    DirectoryCopy(source, destination);
    return;
}

If clearDestination=true and destination exists as a file (rare but possible if a prior run wrote a file where the module dir should be), the code deletes the file but then calls DirectoryCopy(source, destination) which will try to create destination as a directory. Directory.CreateDirectory handles this, so it works — but the path through Directory.Delete + DirectoryCopy without an intervening Directory.CreateDirectory for the file-deletion branch relies on DirectoryCopy to create the root. Worth verifying DirectoryCopy calls Directory.CreateDirectory at its start (it appears to from context).

2. RunCommandHook: non-verbose output lost on success

if (result.ExitCode == 0)
{
    if (_logger.IsVerbose)
    {
        if (!string.IsNullOrWhiteSpace(result.StdOut)) _logger.Verbose(result.StdOut.TrimEnd());
        if (!string.IsNullOrWhiteSpace(result.StdErr)) _logger.Verbose(result.StdErr.TrimEnd());
    }
    return;
}

Hook stdout/stderr are only emitted at verbose level on success. If a hook writes meaningful progress to stdout, users running without -Verbose will see nothing between "Hook BeforeBuild: sync-catalog" and the next pipeline step. This is consistent with how RunBundleScripts works in the bundle runner, so it's a deliberate choice — just flagging it since hooks are a new user-visible surface.

3. ModuleBuildPreparationService.PrepareFromConfig: missing version segment handling

When loading from JSON config, version resolution falls back to ResolveBaseVersion(spec.Build.SourcePath, spec.Build.Name) (2-argument overload), which reads the .psd1. The "4.0.X" placeholder in spec.Build.Version is passed through directly — this is fine as long as downstream version handling understands the X placeholder. The test (Prepare_from_config_loads_pipeline_json_and_resolves_paths) doesn't assert on the resolved version because Version = "4.0.X" is already non-empty and the test JSON sets it explicitly. No bug — just confirm that the version placeholder semantics are handled downstream.

4. Timeout test reliability

[Fact]
public void RunCommandHook_ReportsTimeoutExplicitly()
{
    if (!CommandExists("pwsh")) return;
    // ...
    HookArguments = new[] { "-NoLogo", "-NoProfile", "-Command", "Start-Sleep -Seconds 10" },
    HookTimeoutSeconds = 1,

Start-Sleep -Seconds 10 with a 1-second timeout can be flaky on slow CI agents. Consider Start-Sleep -Seconds 60 with a 2-second timeout for more margin, or use [System.Threading.Thread]::Sleep([int]::MaxValue) which doesn't tick in 1-second increments.

Security

Hook command execution is inherently privileged, since hooks run arbitrary commands from the JSON config. The token expansion (ApplyTemplate) is not used for shell interpolation — args are passed as a list via ArgumentList on modern .NET or BuildWindowsArgumentString on net472, so there is no shell injection risk from token expansion. The net472 path (BuildWindowsArgumentString) bears watching if untrusted config values can reach it, but this is an existing pattern in the codebase.

EnsurePathWithinRoot guards on TemplatePath, OutputPath, and all copy destinations are present and correct. The test BuildBundle_RejectsGeneratedScriptTemplateOutsideProjectRoot explicitly validates the traversal check.

Test Coverage

Coverage is strong:

DotNetPublishPipelineRunnerBundleHardeningTests — edge cases (outside-root template, ClearDestination=false conflicts, non-recursive token rendering, module destination conflicts, cycle detection)
DotNetPublishPipelineRunnerHookTests — plan ordering, default timeout, duplicate ID rejection, missing command rejection, argument/env expansion, timeout reporting
DotNetPublishPipelineRunnerMsiPrepareTests extension — harvest exclude patterns
LineEndingsNormalizerTests — bare CR and CRLF normalization
ExportDetectorTests extension — binary cmdlet/manifest export parity

Gaps worth noting:

No test for ApplyDotNetPublishSpecOverrides with a multi-style --style + active profile (the silent null profile style case above).
No integration-level test for RunCommandHook on non-Windows (test skips when pwsh is absent, which is correct, but a Linux-compatible sh -c "exit 0" test would improve Linux CI coverage).
No test for the BundleSignPatternMatches with nested wildcards hitting the third branch specifically.

Summary

Area	Assessment
Correctness	✅ — CLI override fix, line-endings fix, and template renderer refactor are all correct.
Design	✅ — Phased, well-bounded, consistent with existing patterns.
Security	✅ — Path traversal guards present; hook argument expansion avoids shell injection.
Tests	✅ with minor gaps noted above.
Documentation	✅ — Schema, quickstart, examples, and rationale doc are all updated.

The two actionable items I'd flag before merge:

Consider a warning log when multiple --style values are passed with an active profile (silent profile style drop).
Make the timeout test less flaky (longer sleep, slightly higher timeout).

Everything else is nits or informational. The PR is in good shape.

claude · 2026-04-28T21:30:08Z

Code Review — PR #331: Add reusable PowerForge publish bundles and hooks

This is a substantial, well-structured PR. The overall design is sound, validation is thorough, and test coverage is solid. The notes below are ordered by severity.

Potential bugs

1. CopyBundlePath — file→directory edge case deletes dir then writes file to the same path

When source is a file, destination is an existing directory, and clearDestination=true:

if (clearDestination && Directory.Exists(destination))
    Directory.Delete(destination, recursive: true);   // directory removed

var destinationFile = destination;
if (Directory.Exists(destinationFile))                // always false now
    destinationFile = Path.Combine(destinationFile, Path.GetFileName(source));
// → copies source to `destination` path (no filename appended)

If a user passes DestinationPath = "Lib" expecting bundle-root/Lib/file.dll, they get bundle-root/Lib (a file with no extension). The directory is also irrecoverably deleted before the copy, so a failure mid-way leaves no original and no copy. Whether this is spec-correct or an oversight is worth clarifying with a comment or a test covering this exact case.

2. BundleSignPatternMatches — third branch can produce false positives

if (pattern.StartsWith("**/", StringComparison.Ordinal))
    return WildcardMatch(relativePath, pattern.Substring("**/".Length));

**/*.exe → strips prefix → *.exe → tested against the full relative path Subdir/App.exe. Depending on the WildcardMatch implementation, a pattern like **/Modules/*.dll could match OtherDir/Modules/X.dll via the third path but also a root-level Modules/X.dll via the first path. A focused test covering **/<folder>/<ext> patterns against both matching and non-matching paths would confirm the intended behaviour.

3. RunHookProcess — .NET Framework stream drain race on happy path

The timeout path correctly drains streams with a bounded Task.WhenAll(...).Wait(5s). The happy path calls stdoutTask.GetAwaiter().GetResult() after process.WaitForExit(timeoutMs). On .NET Framework, WaitForExit(int) can return true before async stream buffers are fully flushed (per the known Process + redirected-streams contract). A follow-up call to the no-argument process.WaitForExit() after WaitForExit(ms) returns true is the standard guard on net472. This is a low-frequency issue but worth addressing before it surfaces in production CI.

Code quality & design

Positive highlights

EnsurePathWithinRoot guards applied consistently on all new paths (template paths, destinations, sign targets). Good defensive posture.
NormalizeCommandHooks validates duplicate IDs and empty commands at plan-time rather than runtime — correct approach.
OrderTargetsForBundleIncludes DFS cycle detection is correctly implemented (visiting set cleaned before marking visited).
ScriptTemplateRenderer regex-callback rewrite eliminates the double-pass and is strictly better.
LineEndingsNormalizer fix correctly handles bare \r (old Mac CR-only line endings). The two-step Replace("\r\n","\n").Replace("\r","\n") is the canonical safe pattern.
Pre-normalizing before PSScriptAnalyzer in FormattingPipeline is the right call; mixed endings were the actual root cause.

Minor issues

ResolveHookCommandPath does not call EnsurePathWithinRoot for relative paths that contain separators. Since hook config is team-authored this is low risk, but a note in the method doc explaining the trust boundary would help future readers.
Plan_NormalizesBundlePackageLayoutPrimitives test asserts that " Service " (with spaces) is trimmed to "Service" — great. There is no equivalent test that " Lib " in CopyItem.DestinationPath is trimmed before the EnsurePathWithinRoot check; the trim happens earlier in BuildBundlePlans so it is safe, but it is not directly exercised.
The FormattingPipeline step-3 loop uses preNormalize.FirstOrDefault(...) (linear scan) for each file — O(N²) in the file count. For the typical dozens-of-files case this is fine, but converting preNormalize to a Dictionary<string, NormalizationResult> would make it O(N) with negligible extra code.
RunCommandHook_ExpandsArgumentsWorkingDirectoryAndEnvironment and RunCommandHook_ReportsTimeoutExplicitly are guarded by CommandExists("pwsh") and silently pass if pwsh is absent. Consider Assert.Skip (xUnit v2.5+) or Skip.If(...) so CI reports them as skipped rather than passing.
CloneDotNetPublishSpec round-trips through JSON (serialize then deserialize). If any future spec property is marked [JsonIgnore] or uses a custom converter that is absent from CliJson.Context, data will silently disappear. A comment flagging this invariant would help.

Test coverage

Coverage is genuinely good — path-traversal rejection, cycle detection, token non-recursive expansion, and timeout reporting all have dedicated tests. A few gaps worth filling:

The file→directory copy edge case noted in issue Release version number does not automatically update #1 above.
A BundleSignPatternMatches unit test covering **/<folder>/<ext> against both a matching and a non-matching path.
Happy-path RunCommandHook when the hook exits non-zero with HookRequired=false (warn-only path).

Security

No new attack surfaces beyond the existing model. The EnsurePathWithinRoot guards and the PATH-only resolution for unqualified hook commands (dotnet, pwsh) are both correct. The one thing worth documenting is that hook Command values with path separators resolve against the project root without a root-containment check — this is intentional (hooks can call scripts anywhere on the machine), but it diverges from the containment model applied to template paths and destinations.

Summary

The feature design is solid and the implementation is careful. The three items worth addressing before merge are: the CopyBundlePath file→directory semantics (clarify or fix), the BundleSignPatternMatches third-branch false-positive risk (add a targeted test), and the net472 stream drain on the happy path of RunHookProcess. Everything else is refinement.

claude · 2026-04-28T21:37:10Z

Code Review: PR #331 — Add reusable PowerForge publish bundles and hooks

This is a well-designed, substantial addition (+3572/−63 lines) that generalises bespoke TierBridge/TestimoX build flows into reusable PowerForge primitives. Overall quality is high: security invariants are respected, the design is internally consistent, and test coverage is good. Below are the findings, grouped by severity.

Overview

The PR ships six cohesive capabilities:

Hooks[] — declarative command steps at six publish phases (BeforeRestore, BeforeBuild, BeforeTargetPublish, AfterTargetPublish, BeforeBundle, AfterBundle)
Bundle composition primitives — PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[]
Bundle post-process signing — PostProcess.SignProfile / SignPatterns[]
MSI harvest exclude patterns — HarvestExcludePatterns[]
Mixed line-endings fix — pre-normalise before PSScriptAnalyzer to prevent parse failures
Module export parity — missing cmdlets added to psd1/psm1, with a new test enforcing alignment

Potential Bugs

1. CopyBundlePath — directory-to-directory with ClearDestination=true does unnecessary extra work

if (clearDestination && destinationWasDirectory)
{
    Directory.Delete(destination, recursive: true);
    Directory.CreateDirectory(destination);  // recreate parent
}
if (clearDestination && File.Exists(destinationFile))
    File.Delete(destinationFile);             // then delete again?
File.Copy(source, destinationFile, overwrite: clearDestination);

When source is a file and the destination resolves to Path.Combine(existingDir, fileName), the Directory.Delete + Directory.CreateDirectory path clears the whole directory (good), but then File.Delete(destinationFile) fires again (harmless since it no longer exists). The File.Copy(..., overwrite: clearDestination) is also redundant in this branch since the file was just deleted. Not a correctness bug, but logic is harder to follow than it needs to be.

2. FormattingPipeline.cs — O(n²) linear scan for pre-normalise results

var preNormalizeResult = preNormalize.FirstOrDefault(x => string.Equals(x.Path, f, StringComparison.OrdinalIgnoreCase));

preNormalize is a List<NormalizationResult> iterated once per file. For small list.Length this is fine, but since list is already deduplicated and ordered, a Dictionary<string, NormalizationResult> lookup would be cleaner and matches what the existing preResult / pssaResult lookups also do (same O(n²) pattern already present pre-PR). Worth noting but not a blocker.

3. RunHookProcess — second process.WaitForExit() after timeout branch

if (!process.WaitForExit(timeoutMs))
{
    process.Kill(entireProcessTree: true);
    Task.WhenAll(stdoutTask, stderrTask).Wait(TimeSpan.FromSeconds(5));
    ...
    return (-1, stdout, stderr, fileName, TimedOut: true);
}

process.WaitForExit();   // ← this is the happy-path

The second process.WaitForExit() (no timeout) is correct on .NET Core/5+ to flush async stream buffers, but on NET472 it can block indefinitely if the redirected output pipe stalls. The #if NET472 guard exists for Kill() and argument building but not for this call. Consider adding a bounded wait:

#if NET472
process.WaitForExit(30_000);
#else
process.WaitForExit();
#endif

Security Observations

Good: EnsurePathWithinRoot is applied consistently to all new paths — generated script template paths, copy item destinations, module include destinations, and post-process sign targets. The template token non-recursive rendering test (BuildBundle_GeneratedScriptTokenValuesAreNotRenderedRecursively) correctly documents and enforces that token values containing {{...}} are not re-rendered.

Worth noting: Hook Command values are arbitrary strings from user-controlled JSON config and are executed with the full permissions of the calling process. This is intentional and appropriate for a build tool, but the docs could call it out explicitly (similar to how Scripts[] is treated today).

NET472 argument escaping: BuildWindowsArgumentString (not visible in this diff) is used on NET472 instead of psi.ArgumentList. Ensure it correctly escapes arguments containing spaces, quotes, and special shell characters — particularly since hook arguments may contain token-expanded values that include paths.

Design Observations

Spec cloning before overrides (CLI path):

var spec = CloneDotNetPublishSpec(loaded.Value);
ApplyDotNetPublishSpecOverrides(spec, overrideTargets, effectiveRids, effectiveFrameworks, effectiveStyles);
var plan = runner.Plan(spec, specPath);
// previously: ApplyDotNetPublishPlanOverrides(plan, ...)

This is the correct fix — overrides now influence planning rather than only execution. The JSON round-trip clone is a pragmatic approach; the dependency on CliJson.Context.DotNetPublishSpec being round-trip stable is implicit — if new fields are added without updating the serialization context, CLI overrides will silently drop them. A comment documenting this contract would help future maintainers.

BundleSignPatternMatches logic:
The three-branch pattern matching (full relative path → basename → strip **/ prefix) is documented and tested, but the layering can produce surprising results: *.ps1 matches any .ps1 file at any depth via the basename branch, while *.ps1 without **/ in a standard glob would normally match only at the root. The docs in the quickstart correctly describe this behavior; consider adding a brief comment in the code too.

OrderTargetsForBundleIncludes — topological sort only uses Includes[], not CopyItems[]/ModuleIncludes[]:
This is correct — CopyItems and ModuleIncludes source from project root paths, not from target publish outputs, so they don't create target-ordering dependencies. Worth a brief comment since the function name implies it handles all bundle-level ordering.

DotNetPublishCommandHook.DefaultTimeoutSeconds = 600 referenced via constant alias:

private const int DefaultCommandHookTimeoutSeconds = DotNetPublishCommandHook.DefaultTimeoutSeconds;

This alias adds no value — callers can reference DotNetPublishCommandHook.DefaultTimeoutSeconds directly.

Test Coverage

Coverage is strong. Specific highlights:

✅ Cycle detection in bundle includes
✅ Path traversal rejection for template paths outside ProjectRoot
✅ ClearDestination=false conflict detection for CopyItems and ModuleIncludes
✅ Non-recursive token rendering
✅ Hook ordering (before/after publish/bundle)
✅ Duplicate hook ID detection
✅ Timeout reporting
✅ Line-endings normaliser for bare CR and mixed endings
✅ Export parity between binary assembly, psd1, and psm1

One gap: there is no test for PostProcess.Sign integration in BuildBundle (signing is only plan-tested via Plan_ResolvesBundlePostProcessSigningProfile). A hardening test that calls BuildBundle with a mock sign operation and verifies SignBundlePostProcessFiles was invoked would close this.

Minor Style Notes

NormalizeArguments preserves empty strings (unlike NormalizeStrings which filters them). This is intentional — empty-string CLI arguments are meaningful — but a comment would clarify intent.
The DotNetPublishBundlePostProcessOptions.SignProfile and SignOverrides are nulled out after plan-time resolution, so the plan representation differs from the spec. This is consistent with how signing profiles are handled elsewhere in the pipeline, but callers reading the plan should be aware the properties are always null post-planning.
RepoRootLocator.MaxSearchDepth = 12 is a magic number; a brief comment (e.g., // generous limit for deep test output trees) would help.

Summary

Area	Status
Correctness	✅ No blocking bugs found
Security	✅ Path containment enforced; template injection prevented
Design	✅ Consistent with existing patterns
Test coverage	✅ Comprehensive; one minor gap noted
Docs/schema	✅ Updated and accurate
CI propagation fix	✅ `Build-Module.ps1` exit code hardening correct

The PR is in good shape. The items above are improvement suggestions rather than blockers — the most actionable ones are the NET472 bounded-wait for the hook process and the signing integration test in BuildBundle.

claude · 2026-04-28T21:42:46Z

Code Review: PR #331 — PowerForge Publish Bundles and Hooks

Overview

This PR adds a substantial set of reusable build primitives to PowerForge: declarative command hooks at fixed publish phases, new bundle composition fields (PrimarySubdirectory, CopyItems[], ModuleIncludes[], GeneratedScripts[]), bundle post-process signing, MSI harvest exclusion patterns, a module build JSON config path, and a mixed line-endings fix that pre-normalizes files before PSScriptAnalyzer runs. The scope is large but well-scoped to real upstream consumers (TierBridge, TestimoX).

Code Quality and Best Practices ✅

Strengths:

Consistent API pattern: spec → plan (validated/normalized) → run. All new features follow this layered model cleanly.
Validation is front-loaded at Plan() time — duplicate hook IDs, missing commands, missing source/destination paths, and bundle include dependency cycles all throw ArgumentException with clear messages before any I/O happens.
CloneCommandHooks, CloneBundleCopyItems, CloneBundleModuleIncludes, CloneBundleGeneratedScripts follow the existing clone pattern consistently.
ResolveProfile correctly propagates Hooks through profile filtering (the CloneCommandHooks(spec.Hooks) call in the profile clone path).
NormalizeArguments correctly preserves empty string arguments (unlike NormalizeStrings which strips them), which matters for hook args like "".
ScriptTemplateRenderer refactor from string-replace loop → Regex.Replace callback is cleaner and removes the two-pass approach.
LineEndingsNormalizer.NormalizeEndings fix correctly handles bare \r that the old code missed for both CRLF and LF targets.
Build-Module.ps1 exit code propagation is a clean one-liner that directly solves the CI failure swallowing problem.

Minor style issue:

DotNetPublishPipelineRunner.Hooks.cs:RunCommandHook — step.HookEnvironment ?? new Dictionary<...>() is passed to foreach but step.HookEnvironment is initialized to new Dictionary<...>() in the plan type, so the null-coalesce is dead code. Harmless but slightly misleading.

Potential Bugs / Issues ⚠️

1. CopyBundlePath — redundant file-existence check when clearing a file-into-directory copy

if (clearDestination && destinationWasDirectory) {
    Directory.Delete(destination, recursive: true);
    Directory.CreateDirectory(destination); // empty directory recreated
}
if (clearDestination && File.Exists(destinationFile)) // always false here
    File.Delete(destinationFile);
File.Copy(source, destinationFile, overwrite: clearDestination); // overwrite=true anyway

After deleting and recreating the destination directory, File.Exists(destinationFile) is always false. The File.Delete and the File.Copy(overwrite: true) are both redundant in that branch. Not a bug, but dead code that could confuse future readers.

2. Missing test for GeneratedScripts[].Overwrite=false throwing

The hardening tests cover CopyItems.ClearDestination=false and ModuleIncludes.ClearDestination=false explicitly, but there is no test for the analogous GeneratedScripts[].Overwrite=false case. The implementation (if (File.Exists(outputPath) && !script.Overwrite) throw new IOException(...)) looks correct, but the gap means this path is not exercised in the test suite.

3. FormattingPipeline pre-normalization — O(n²) FirstOrDefault

var preNormalizeResult = preNormalize.FirstOrDefault(x =>
    string.Equals(x.Path, f, StringComparison.OrdinalIgnoreCase));

For each file in the outer loop, this does a linear scan over preNormalize. For modules with hundreds of source files this is quadratic. Consider building a Dictionary<string, NormalizationResult> keyed on path after the pre-normalize step. Low priority given typical module sizes, but worth fixing before the list grows.

4. RepoRootLocator — fragile in unusual CI layouts

RepoRootLocator.Find() walks up to 12 parent directories looking for PowerForge/PowerForge.csproj. If tests are ever run from a staging artifact directory where that marker doesn't exist in a parent path, the tests that depend on it (ExamplePackageBundleMsi_DeserializesPackageLayoutPrimitives, GeneratedModuleFiles_export_PSPublishModule_plugin_and_bundle_cmdlets, etc.) will throw DirectoryNotFoundException rather than a meaningful test failure. Consider providing a POWERFORGE_ROOT environment variable fallback, matching the convention already documented in AGENTS.md.

Performance Considerations ✅

FindBundleSignTargets enumerates all files under bundleRoot once per pattern. For bundles with many patterns and large output trees this could be slow. The current approach is fine for the expected use case, but worth noting as a future optimization point if patterns grow.
OrderTargetsForBundleIncludes runs DFS topological sort — O(V+E) — only when at least two targets exist and bundles are present. The early-exit when all dependency sets are empty avoids the overhead in the common no-bundle case. Good.

Security Considerations ✅

Path traversal — EnsurePathWithinRoot is correctly applied on generated script template paths, copy destinations, and module include destinations. The test BuildBundle_RejectsGeneratedScriptTemplateOutsideProjectRoot explicitly covers the traversal rejection.
Template injection — Token values are inserted literally into the double-brace template without a recursive rendering pass. The test BuildBundle_GeneratedScriptTokenValuesAreNotRenderedRecursively verifies a value of {{NestedCommand}} does not expand. The inline documentation (GeneratedScripts[].Tokens in the schema and quickstart) correctly explains this is intentional.
Hook command execution — Hooks run arbitrary commands from JSON config. This is appropriate for a CI build tool that operates on checked-in configuration. ResolveHookCommandPath correctly handles rooted paths, relative paths with separators (resolved from projectRoot), and bare executable names (left to PATH resolution). No unexpected shell expansion is performed — args are passed as a list on .NET 5+ (ArgumentList.Add) and escape-encoded on 472 (BuildWindowsArgumentString).
MSI harvest exclude patterns — ShouldExcludeHarvestFile performs pattern matching on relative paths using WildcardMatch. The patterns are read from config and not user-runtime input, which limits the attack surface.

Test Coverage ✅

Test additions are comprehensive and well-targeted:

DotNetPublishPipelineRunnerBundleHardeningTests — 7 new tests covering path traversal, ClearDestination=false semantics for both CopyItems and ModuleIncludes, non-recursive token rendering, glob matching, and cycle detection.
DotNetPublishPipelineRunnerHookTests — 8 new tests covering hook ordering, per-context step keys, default timeout, duplicate ID validation, missing command validation, token expansion in arguments/env, and timeout reporting.
DotNetPublishPipelineRunnerMsiPrepareTests — 1 new test confirming harvest exclusion patterns work end-to-end (basename globs, **/ globs, exact names).
LineEndingsNormalizerTests — 2 new tests for bare \r handling in both CRLF and LF normalization modes.
ExportDetectorTests — 2 new tests confirming binary cmdlet detection and manifest/bootstrapper export parity for the three newly-exported cmdlets.
ModuleBuildPreparationServiceTests — 1 new test for JSON config path loading and relative path resolution.
PowerForgePluginCatalogServiceTests — 1 new test deserializing the example catalog file.

One gap to close: add a test for GeneratedScripts[].Overwrite=false throwing IOException.

Summary

This is a high-quality, well-tested PR. The new features are additive with no breaking changes, the security-sensitive paths (traversal, template injection) are carefully guarded and tested, and the API design is consistent with the existing codebase conventions. The four items above (CopyBundlePath dead code, missing Overwrite=false test, FormattingPipeline O(n²) scan, RepoRootLocator fragility) are all minor and none block merging. Recommend addressing the missing Overwrite=false test and the RepoRootLocator environment-variable fallback before or shortly after merge.

PrzemyslawKlys added 4 commits April 28, 2026 16:28

Add reusable dotnet publish bundle hooks

80c0b24

Add MSI harvest exclude patterns

c01aa16

Add dotnet publish project root override

bed6ce9

Allow module builds from JSON config

f63c973

Add bundle post-process signing

5d9139c

Fix module formatting line ending normalization

b0867a2

Address publish bundle review feedback

6bc01d4

Clarify bundle hook review followups

a3f29a1

Tighten bundle review edge cases

8b36eec

Render bundle script tokens literally

8dfc754

Clone dotnet publish specs before CLI overrides

5072554

Tighten bundle script and hook defaults

e046ae4

Polish hook diagnostics and changelog

3530c8b

Warn on multi-style profile overrides

1ff34bc

Harden bundle copy and hook stream handling

3d71825

Bound hook stream drain on net472

57d57f5

Uh oh!

Conversation

PrzemyslawKlys commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Validation

Uh oh!

claude Bot commented Apr 28, 2026

Code Review: PR #331 — PowerForge Reusable Publish Bundles and Hooks

Overview

Bugs / Correctness Issues

Code Quality

Design / Documentation

What's Working Well

Minor Nits

Summary

Uh oh!

claude Bot commented Apr 28, 2026

Code Review: Add Reusable PowerForge Publish Bundles and Hooks

✅ Strengths

⚠️ Issues & Suggestions

1. Potential deadlock in hook timeout path on .NET 4.7.2 (RunHookProcess)

2. TemplatePath is not bounds-checked against ProjectRoot

3. High default timeout (HookTimeoutSeconds = 600)

4. BundleSignPatternMatches — redundant ** stripping may indicate WildcardMatch doesn't handle globstar

5. ShouldExcludeHarvestFile — filename-only match may be surprising for path-qualified patterns

6. Hook step key uniqueness when the same hook fires at multiple targets

7. CLI phase shift for spec overrides (ApplyDotNetPublishSpecOverrides)

8. Two template syntaxes for different contexts

📋 Minor / Nits

🧪 Test Coverage Assessment

Summary

Uh oh!

claude Bot commented Apr 28, 2026

Code Review — PR #331: Add reusable PowerForge publish bundles and hooks

Overview

Specific Issues / Suggestions

1. Local filesystem path left in shipped documentation

2. FindRepoRoot() duplicated across three test files

3. NormalizeArguments has dead code after the null filter

4. Manual dictionary copying could use the copy constructor

5. ConfigLabel field is semantically overloaded

6. GenerateBundleScripts: Overwrite=false throws instead of skipping

7. BundleSignPatternMatches applies globstar stripping on top of wildcard matching

8. ApplyDotNetPublishSpecOverrides applies runtimes/frameworks/styles to both profile and target levels

9. CopyBundlePath when file exists and ClearDestination=false

10. Hook test RunCommandHook_ExpandsArgumentsWorkingDirectoryAndEnvironment requires pwsh

Positives

Summary

Uh oh!

claude Bot commented Apr 28, 2026

Code Review: PR #331 — Add reusable PowerForge publish bundles and hooks

Overview

Code Quality & Style ✅

Specific Observations

LineEndingsNormalizer.NormalizeEndings (Bug Fix)

ApplyDotNetPublishSpecOverrides (CLI) — minor concern

OrderTargetsForBundleIncludes — topological sort

GenerateBundleScripts — template security

RunHookProcess — timeout kill sequence

ShouldExcludeHarvestFile — pattern matching logic

CopyBundlePath — file-into-directory behavior

Potential Issues

Test Coverage ✅

Minor Suggestions

Summary

Uh oh!

claude Bot commented Apr 28, 2026

Code Review — PR #331: PowerForge publish bundles and hooks

Overview

Code Quality and Style

Potential Bugs / Issues

Performance Considerations

Security

Test Coverage

Specific Suggestions

Documentation

Summary

Uh oh!

PrzemyslawKlys commented Apr 28, 2026 •

edited

Loading

1. Potential deadlock in hook timeout path on .NET 4.7.2 (`RunHookProcess`)

2. `TemplatePath` is not bounds-checked against `ProjectRoot`

3. High default timeout (`HookTimeoutSeconds = 600`)

4. `BundleSignPatternMatches` — redundant `**` stripping may indicate `WildcardMatch` doesn't handle globstar

5. `ShouldExcludeHarvestFile` — filename-only match may be surprising for path-qualified patterns

7. CLI phase shift for spec overrides (`ApplyDotNetPublishSpecOverrides`)

2. `FindRepoRoot()` duplicated across three test files

3. `NormalizeArguments` has dead code after the null filter

5. `ConfigLabel` field is semantically overloaded

6. `GenerateBundleScripts`: `Overwrite=false` throws instead of skipping

7. `BundleSignPatternMatches` applies globstar stripping on top of wildcard matching

8. `ApplyDotNetPublishSpecOverrides` applies runtimes/frameworks/styles to both profile and target levels

9. `CopyBundlePath` when file exists and `ClearDestination=false`

10. Hook test `RunCommandHook_ExpandsArgumentsWorkingDirectoryAndEnvironment` requires `pwsh`

`LineEndingsNormalizer.NormalizeEndings` (Bug Fix)

`ApplyDotNetPublishSpecOverrides` (CLI) — minor concern

`OrderTargetsForBundleIncludes` — topological sort

`GenerateBundleScripts` — template security

`RunHookProcess` — timeout kill sequence

`ShouldExcludeHarvestFile` — pattern matching logic

`CopyBundlePath` — file-into-directory behavior

2. `FindBundleSignTargets` — unbounded recursive enumeration

3. Process stream cleanup — hardcoded 5-second `Task.WaitAll` after kill

4. `Process.Start()` return value — already handled, but could be clearer

1. `ScriptTemplateRenderer.Render` — error handling regression on unknown tokens

2. `FindBundleSignTargets` — duplicate pattern matching

3. `RunHookProcess` — timeout returns exit code -1 silently

4. `ApplyDotNetPublishSpecOverrides` — multiple framework override sets both `.Framework` and `.Frameworks`

5. `ModuleBuildPreparationService.PrepareFromConfig` — string-literal parameter set name

6. `FormattingPipeline` — pre-normalisation result lookup is O(n²)

7. `DotNetPublishCommandHook.Validate` — `Hooks[].Id` error message omits array index

8. `Example.PackageBundleMsi.json` — `GeneratedScripts[].Template` uses `\r\n` literals