Restore conditional command exports

[PrzemyslawKlys]

Summary

• Preserve module-only New-ConfigurationCommand -ModuleName ... entries in the module build plan.
• Analyze exported functions for optional command-module dependencies and generate conditional export filtering in merged/bootstrapper PSM1 output.
• Add regression coverage for module-only command dependencies, inferred optional module usage, helper propagation, and generated export blocks.

Root Cause

New-ConfigurationCommand entries without explicit command names were discarded during planning, and the generated C# export block always exported every manifest function. That bypassed the older PSPublishModule behavior that filtered exports when optional modules such as ActiveDirectory, DHCPServer, or DNSServer were unavailable.

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --no-restore --filter "FullyQualifiedName~CommandModuleExportDependencyAnalyzerTests|FullyQualifiedName~ModuleMergeComposerTests|FullyQualifiedName~Plan_KeepsModuleOnlyCommandDependencies|FullyQualifiedName~Run_KeepsCommandModuleDependenciesInPlanWithoutPersistingManifestKey"
• dotnet build .\PSPublishModule.sln -c Release --no-restore

Note: a broader PowerForge.Tests run previously hit an unrelated timeout in PowerForgeCliProjectReleaseTests.ProjectRelease_CliPreservesProjectDefaultsFromConfig; the focused tests above and full solution build passed.

[claude]

Code Review — PR #329: Restore conditional command exports

Overview

This PR restores the conditional module-export filtering behavior from the old PSPublishModule. Two root causes are fixed:

1. New-ConfigurationCommand entries that specify only a ModuleName (no explicit command names) were being silently discarded during planning.
2. The generated export block always exported every manifest function, bypassing the legacy behavior that suppressed exports when optional modules (ActiveDirectory, DHCPServer, DNSServer) were unavailable at runtime.

The fix introduces two new services (CommandModuleExportDependencyAnalyzer, ModuleConditionalExportBlockBuilder), threads conditional-export data through the merge and bootstrapper pipelines, and adds regression coverage. The approach is well-structured and the test signal is clear.

---

Potential Issues

Medium — BuildSources called twice on the hot path

In ModulePipelineRunner.MergeAndTests.cs, ApplyMerge calls ModuleMergeComposer.BuildSources once to get mergeInfo.ScriptFiles, then immediately calls it again if conditional exports are found:

var mergeInfo = ModuleMergeComposer.BuildSources(...);                     // full merge pass 1
var conditionalExportDependencies = ResolveConditionalExportDependencies(
    plan, mergeInfo.ScriptFiles, buildResult.Exports);
if (conditionalExportDependencies.Count > 0 && mergeInfo.HasScripts)
{
    mergeInfo = ModuleMergeComposer.BuildSources(...,                       // full merge pass 2
        conditionalFunctionDependencies: conditionalExportDependencies);
}

This doubles the I/O and AST-parse cost for any module that uses conditional exports. Exposing ScriptFiles from BuildSources as a static helper (or computing the conditional map before the first BuildSources call) would avoid the redundant pass.

Medium — ResolveCommandSource spins up a new PowerShell runspace per command

using var ps = PowerShell.Create();
ps.AddCommand("Get-Command")...
var result = ps.Invoke();

Each call that misses the cache creates a new PowerShell instance (and implicitly a new runspace). On a project with many external commands this can be slow, and—more importantly—the result is environment-dependent: the build agent's session determines which modules are visible, which may differ from the target deployment. The cache mitigates repeated calls within a run but not across different environments. Consider documenting this behavior or preferring the explicit-command-list path when reliability matters.

Medium — TextLooksLikeModuleDependency only handles ActiveDirectory

CommandLooksLikeModuleCommand has heuristics for ActiveDirectory, DnsServer, and DhcpServer, but TextLooksLikeModuleDependency only checks for ActiveDirectory patterns:

if (string.Equals(moduleName, "ActiveDirectory", StringComparison.OrdinalIgnoreCase) && ...)
    return true;

return false;   // DnsServer, DhcpServer — nothing

Either extend the text-match heuristic to cover the other modules, or remove it and rely solely on AST command-name analysis. As-is, the two heuristic paths are inconsistent.

Low — NormalizeDependencies silently drops module-only entries in the export block builder

ModuleConditionalExportBlockBuilder.NormalizeDependencies skips any dependency whose command array is empty:

if (commands.Length == 0)
    continue;

This is the right behavior when the analyzer fails to infer any commands, but it means a module-only dependency that the analyzer couldn't resolve will produce no warning at all in the generated output. A Write-Warning or at least a build-time log message would help diagnose missing conditional exports.

Low — FunctionDependencyInfo.LocalCalls mutable after construction

internal string[] LocalCalls { get; set; }

The two-phase initialization (construct with Array.Empty, then backfill LocalCalls in the second foreach) works, but the mutable setter is surprising for a type that otherwise looks immutable. A static factory method that takes the full graph as input and computes local calls internally would be cleaner and eliminate the unexpected mutation window.

---

Generated PowerShell — Design Note

The generated bootstrapper now runs Import-Module for each optional dependency at module-load time, not lazily at function-call time:

foreach ($PowerForgeDependencyModule in $PowerForgeCommandModuleDependencies.Keys) {
    try {
        Import-Module -Name $PowerForgeDependencyModule -ErrorAction Stop -Verbose:$false
    } catch { ... }
}

This matches the legacy behavior and the escaping (EscapePsSingleQuoted, EscapePsDoubleQuoted) is correct. One thing to be aware of: if an optional module is present but takes a long time to load, this loop adds latency to every module import. That is a pre-existing trade-off from the original design rather than a regression introduced here.

---

Test Coverage

The tests are well-scoped and use isolated temp directories cleanly. Three things that lack coverage:

• CommandLooksLikeModuleCommand with an unknown module (the generic noun-prefix fallback path)
• TextLooksLikeModuleDependency for any of the ActiveDirectory text patterns
• The ResolveCommandSource path (the PowerShell invocation branch) — even a negative/cache-hit test would be useful

---

Minor / Nits

• The whitespace-only diff in ModulePipelineRegressionParityTests.cs (trailing brace reformatting) adds noise to the diff; no functional impact.
• The static lambda annotations (static function => ..., static command => ...) are consistently applied — good for avoiding unintended captures.
• Centralizing FormatPsStringList and BuildExportBlock into ModuleConditionalExportBlockBuilder and removing the duplicates in ModuleMergeComposer and ModuleBootstrapperGenerator is a clean improvement.

---

Summary

The fix is correct and the regression tests validate the core scenarios. The main items worth addressing before merge are the double BuildSources call (wasteful but not a correctness issue) and the asymmetry in TextLooksLikeModuleDependency. The ResolveCommandSource environment-dependency is worth a comment at minimum so future maintainers understand why results can differ between machines.

[claude]

Code Review — PR #329: Restore conditional command exports

Overview

This PR restores the behaviour where exported functions are conditionally filtered at module import time when an optional dependency module (e.g. ActiveDirectory, DhcpServer, DnsServer) is unavailable. The fix has two main parts:

1. ModulePipelineRunner.Plan.cs — Preserves module-only New-ConfigurationCommand entries (those with no explicit command names) that were previously discarded.
2. New CommandModuleExportDependencyAnalyzer — Walks parsed PowerShell ASTs to determine which exported functions transitively depend on a given optional module.
3. New ModuleConditionalExportBlockBuilder — Emits the conditional Export-ModuleMember block into merged/bootstrapper PSM1 output.

The test additions are well-placed and the overall architecture is clean.

---

Bugs / Correctness

Medium — ResolveCommandSource is environment-dependent

CommandModuleExportDependencyAnalyzer.ResolveCommandSource (lines ~255–277) spawns a live PowerShell instance and calls Get-Command at build time. This makes the build non-deterministic: a command that happens to be loaded on the developer's machine will be attributed to an optional module; the same command on a clean CI agent will not. The method is documented as a best-effort fallback and the cache mitigates repeated calls, but it is easy to miss that results can vary by environment. At minimum a _logger.Verbose trace when the PowerShell path is taken would make this easier to diagnose.

Low — commandSourceCache is correctly shared across modules

Initially looks like it could be per-module (declared just before foreach (var module in configured)) but it is correctly declared outside the loop, so resolved command sources are reused across modules. No issue — just worth confirming.

Low — DependsOnModule uses unbounded recursion

The visited-set guard (visited.Add) prevents infinite cycles, but there is no depth limit. For a pathologically deep call graph this could overflow the stack. In practice PowerShell module function graphs are shallow, so this is theoretical, but worth documenting.

---

Performance

Double file-scan when both phases run

ResolveConditionalExportDependencies is called from both ApplyMerge and TryRegenerateBootstrapperFromManifest. Each call invokes ModuleMergeComposer.ResolveScriptFiles and then re-parses every file's AST in ParseFunctions. For large modules this doubles the I/O and parse work. Caching the IReadOnlyDictionary<string, string[]> result on the pipeline run — or passing it between phases — would eliminate the duplication.

Get-Alias inside the generated foreach loop

The emitted PowerShell (line ~961) calls Get-Alias -ErrorAction SilentlyContinue inside the per-function loop. For a module with many gated commands this runs one Get-Alias per command at import time. Hoisting the Get-Alias call outside the loop and filtering in memory would be faster:

$PowerForgeAllAliases = @(Get-Alias -ErrorAction SilentlyContinue)
foreach ($PowerForgeFunction in $PowerForgeDependencyCommands) {
    ...
    $PowerForgeAllAliases | Where-Object { $_.Definition -eq $PowerForgeFunction } | ForEach-Object { ... }
}

---

Code Quality

Heuristic module detection is fragile for non-standard modules

CommandLooksLikeModuleCommand and TextLooksLikeModuleDependency hard-code three module names. CommandLooksLikeModuleCommand falls back to noun.StartsWith(moduleName) for unknown modules, which will misfire on any module whose name prefix matches a noun used by unrelated commands. This is acceptable for the documented use-cases but could silently produce incorrect gating for other optional modules — worth a code comment.

LooksLikeCommandName redundant trim

At line ~180 the caller already trims name before passing it to LooksLikeCommandName, yet the helper trims again (line ~331). Harmless but noisy.

FormatPsStringList is internal but only used by tests

ModuleConditionalExportBlockBuilder.FormatPsStringList is marked internal rather than private. If the only reason it is internal is test accessibility, consider leaving it private and testing it indirectly through BuildExportBlock.

Whitespace-only change in ModulePipelineRegressionParityTests.cs

Lines 693–708 are a whitespace-only reformat of an existing test. This is harmless but adds noise to the diff.

---

Security

The generated PowerShell correctly escapes module names with EscapePsSingleQuoted in the hashtable key and EscapePsDoubleQuoted in the Write-Warning string. No injection concerns.

---

Test Coverage

Good coverage of the new analyzer:

• Inferred AD dependency from Get-ADUser-style calls ✅
• Propagation through private helpers ✅
• Explicit command list as direct conditional exports ✅
• Text-pattern fallback ([Microsoft.ActiveDirectory.Management.*], Import-Module DnsServer, etc.) ✅
• Plan_KeepsModuleOnlyCommandDependencies covering the planner fix ✅

Gaps worth considering:

• No test for the bootstrapper path (TryRegenerateBootstrapperFromManifest) emitting the conditional block — only the merge path is covered.
• No test asserting that functions with no module dependency are excluded from the conditional list (a negative case).
• No test for the DependsOnModule visited-set cycle-guard.
• The ResolveCommandSource → Get-Command path is untested (difficult to test in isolation, but a note in the code would help).

---

Summary

The design is sound, the refactoring is clean, and the test additions are meaningful. The main actionable items are:

Severity	Item
Medium	ResolveCommandSource is environment-dependent — add verbose logging when the PowerShell path is taken
Low	Double AST parse when both merge and bootstrapper phases run — cache or thread the result
Low	Get-Alias inside the generated per-function loop — hoist outside for performance
Low	Add a negative test (no dependency) and a bootstrapper-path test

Good work on restoring this functionality. The conditional export block builder in particular is a nice, reusable abstraction.

[claude]

Code Review — PR #329: Restore conditional command exports

Overview

This PR fixes a regression where New-ConfigurationCommand -ModuleName ... entries without explicit command names were silently discarded, causing the generated PSM1 to always export every manifest function (bypassing the conditional-export behavior for optional modules like ActiveDirectory, DHCPServer, and DNSServer).

The fix introduces:

• CommandModuleExportDependencyAnalyzer — static analysis of PS function ASTs to infer which exported functions depend on an optional module (direct commands + transitive through private helpers)
• ModuleConditionalExportBlockBuilder — generates the PowerShell Import-Module ... -ErrorAction Stop try/catch guard block
• A one-line fix in the planner to preserve module-only entries
• Solid regression test coverage

The approach is sound. Most of the issues below are minor or medium-impact; nothing is a blocker.

---

Issues

Medium: Double filesystem scan per build

In ModulePipelineRunner.MergeAndTests.cs, ResolveScriptFiles is called before BuildSources:

var scriptFiles = ModuleMergeComposer.ResolveScriptFiles(buildResult.StagingPath, plan.Information);
var conditionalExportDependencies = ResolveConditionalExportDependencies(plan, scriptFiles, buildResult.Exports);
var mergeInfo = ModuleMergeComposer.BuildSources(...);  // calls ResolveScriptFiles again internally

BuildSources always calls ResolveScriptFiles again inside itself, so every merge operation walks the directory tree twice. The same pattern occurs in TryRegenerateBootstrapperFromManifest. For large modules this doubles the I/O at merge time.

Suggested fix: add an optional IReadOnlyList<string>? scriptFiles = null overload path to BuildSources, or compute the file list once and pass it through.

Medium: Get-Command reflection is build-host dependent

ResolveCommandSource spawns a new PowerShell.Create() and calls Get-Command to resolve a command's source module at build time. The comment correctly calls this a "best-effort fallback," but it introduces a subtle footgun: if the build machine happens to have, say, the ActiveDirectory RSAT tools installed, the fallback will correctly detect AD commands. On a clean CI agent without them it returns null — and the heuristics kick in instead. This means the same source code could produce different conditional-export decisions depending on the build environment.

Since the explicit-command-name path and the heuristic paths already handle the common cases well, it may be safer to make this fallback opt-in or at least emit a Verbose log line when the fallback is the only reason a function was included in the conditional set.

Low: Hardcoded module heuristics with no extension point

CommandLooksLikeModuleCommand and TextLooksLikeModuleDependency have hardcoded branches for ActiveDirectory, DnsServer, and DhcpServer. The generic noun-prefix fallback (noun.StartsWith(moduleName)) works for modules whose noun pattern matches their name, but will silently miss any module where the convention differs (like AD's ADxxx pattern is already hardcoded for that reason).

This is fine for now, but as more modules are added the list will grow. A data-driven approach (a small lookup table or configuration key) would be easier to maintain than another if (string.Equals(...)) block.

Low: Silent parse failures lose dependency information

In ParseFunctions, a bare catch swallows all exceptions with only a comment:

catch
{
    // Best effort; parse errors should not make packaging fail.
}

If a file fails to parse (even for a transient reason), that file's functions are silently excluded from dependency analysis. A function in that file will look like it has no module dependencies, so it won't appear in the conditional-export set even if it should. A logger?.Verbose(...) call here (similar to ResolveCommandSource) would make this much easier to diagnose.

---

Minor observations

• ExtractCommandNames has a redundant check: string.IsNullOrWhiteSpace(name) is checked at line 172, then name = name.Trim() + if (name.Length == 0) continue at lines 182–184. IsNullOrWhiteSpace already covers the empty-after-trim case; the second guard is dead for any name that passed the first.

• EscapePsSingleQuoted null-safety inconsistency: value?.Replace("'", "''") ?? string.Empty works but differs from EscapePsDoubleQuoted's (value ?? string.Empty).Replace(...). Pick one style and stick to it.

• FunctionDependencyInfo.LocalCalls mutability: The property is internal string[] LocalCalls { get; set; } while all other properties are init-only. The two-phase initialization (set in constructor, then overwritten in ParseFunctions) is correct but easy to misread. A builder or init-only setter would make the intended lifecycle clearer.

• NormalizeDependencies skips empty-command entries: In ModuleConditionalExportBlockBuilder.NormalizeDependencies, entries with an empty commands array are skipped (if (commands.Length == 0) continue). This is fine because CommandModuleExportDependencyAnalyzer.Analyze only returns modules with at least one function in the conditional set — but the two filters are logically coupled and it's not obvious at the call site.

---

Positive observations

• The DependsOnModule transitive walk correctly uses a visited HashSet to prevent infinite recursion on cyclic helper calls.
• StringComparer.OrdinalIgnoreCase is applied consistently throughout — no raw string comparisons for function/module names.
• The generated PowerShell is clean: Import-Module -ErrorAction Stop, -Verbose:$false suppression, [System.Collections.Generic.List[string]]::new(), and the Write-Warning message all look correct.
• Extracting ResolveScriptFiles as an internal static method is a good refactor that enables reuse and testing.
• Test coverage is well-scoped: AST-inferred dependency, transitive helper propagation, explicit command names, and text-pattern matching are all covered.
• Module layering is respected: CommandModuleExportDependencyAnalyzer (SMA-dependent) is in PowerForge.PowerShell; ModuleConditionalExportBlockBuilder (host-agnostic) is in PowerForge.

---

Summary

The core fix (preserving module-only entries in the plan, then using AST analysis to generate conditional export blocks) is correct and well-tested. The double filesystem scan and the build-host-dependent Get-Command fallback are the most worth addressing before merge. Everything else is a minor polish item.

[claude]

Code Review — PR #329: Restore conditional command exports

Overview

This PR fixes two related issues:

1. Bug fix (planner): New-ConfigurationCommand entries with only a ModuleName and no explicit command names were silently discarded during planning. The guard commandNames.Length == 0 is now removed so module-only entries survive into CommandModuleDependencies.
2. New capability: A new CommandModuleExportDependencyAnalyzer service inspects merged script files at build time, infers which exported functions depend on optional modules (ActiveDirectory, DhcpServer, DnsServer, etc.), and wires that information into a generated conditional export block that suppresses exports when optional modules are unavailable at runtime.

Overall the implementation is solid — good layering, defensive error handling, and reasonable test coverage. The notes below are a mix of actionable concerns and smaller observations.

---

Correctness

DependsOnModule traversal creates a fresh visited set per exported function

In CommandModuleExportDependencyAnalyzer.Analyze:

if (DependsOnModule(function, functions, directModuleHits, new HashSet<string>(StringComparer.OrdinalIgnoreCase)))

A new HashSet is allocated for each exported function. For large modules with many exports sharing a deep call graph, the same subtrees are traversed repeatedly — O(exports × graph depth). Caching the per-function result in a Dictionary<string, bool> (memoization) would make it O(total functions) instead.

NormalizeDependencies in ModuleConditionalExportBlockBuilder skips empty-command entries

if (commands.Length == 0)
    continue;

The commands here are the functions to conditionally export (the analyzer's output, not configured module commands). Entries with an empty array mean the analyzer found no functions to gate — so skipping them is correct. But worth a comment since the parameter name conditionalFunctionDependencies doesn't make this obvious at the call site.

FunctionDirectlyDependsOnModule heuristic gap

When configuredCommands.Count > 0 the naming-convention heuristic (CommandLooksLikeModuleCommand) is skipped entirely. So if a user configures ActiveDirectory = ["Get-ADUser"] but their code also calls Get-ADGroup (which was not listed), Get-ADGroup won't be detected via heuristic. That may be intentional by design (explicit list wins), but it could surprise users who expect the heuristic to fill gaps. A comment explaining the intentional asymmetry would help future readers.

---

Performance

ResolveCommandSource spawns PowerShell.Create() per uncached command

This is correctly guarded with a cache, but on a first run against a large module with many unknown external commands, it will spin up a PowerShell instance for each. The calls are sequential and each blocks on ps.Invoke(). For the common case (small number of AD/DNS/DHCP commands), this is fine. But if a module references many third-party commands, build time could increase noticeably. Consider logging a single summary line with the count of Get-Command fallback lookups rather than one Verbose line per lookup.

Additionally, in a CI environment where optional modules (ActiveDirectory, DhcpServer) are not installed, every Get-Command call for those commands will return nothing — the fallback becomes a no-op and the build still relies entirely on the naming heuristic. This is by design ("best effort"), but worth documenting in a comment so a future maintainer does not expect CI parity with a dev machine that has those modules installed.

---

Fragility / False-negative risk

TextLooksLikeModuleDependency misses common import forms

The current patterns checked:

text.IndexOf("Import-Module ActiveDirectory", StringComparison.OrdinalIgnoreCase) >= 0

This will not match:

• Import-Module -Name ActiveDirectory
• Import-Module 'ActiveDirectory'
• #Requires -Modules ActiveDirectory
• using module ActiveDirectory

Since these are heuristics and the PR description calls them "best effort", this is acceptable — but a brief comment listing what is and isn't covered would save a future bug report.

CommandLooksLikeModuleCommand hardcodes three modules

The generic fallback noun.StartsWith(moduleName, ...) covers unknown modules, but the three hardcoded cases (AD prefix for ActiveDirectory, etc.) diverge from the generic rule. If the module name is something like "Pester" and all its nouns start with "Pester", the generic rule works correctly. The hardcoded cases are needed for the irregular AD prefix, which is reasonable. A comment noting why the override exists for ActiveDirectory specifically would clarify intent.

---

Design / Architecture

FunctionDependencyInfo.LocalCalls is set in a second pass after construction

internal string[] LocalCalls { get; set; }

The two-phase construction (construct → parse all files → backfill LocalCalls) is necessary because LocalCalls resolves cross-file references. The mutable setter on an otherwise record-like class is a smell, though. Consider a factory method or a builder that returns fully initialized instances, or at minimum a comment on the setter explaining the intent.

ResolveScriptFiles is called twice in the bootstrapper regeneration path

In TryRegenerateBootstrapperFromManifest the script files are resolved only to feed ResolveConditionalExportDependencies and then discarded. This is one extra filesystem scan that the merge path avoids by caching scriptFiles in a local variable. Acceptable at current scale, but worth noting if build performance becomes a concern.

---

Generated PowerShell

The generated conditional block is syntactically correct. A few observations:

• Using $PowerForge-prefixed variable names avoids collision with user-defined variables. Good.
• Import-Module -Name $PowerForgeDependencyModule -ErrorAction Stop at module-load time is the right idiom for an import that must succeed or trigger cleanup.
• The alias removal via Get-Alias | Where-Object { $_.Definition -eq $PowerForgeFunction } correctly handles alias forwarding to conditionally removed functions.
• If a module is partially broken (installed but throws on load), the catch block correctly suppresses its functions. This is the desired behavior.

One edge case: if the same function is listed under two optional modules (e.g., a helper that calls both AD and DNS commands), and only one module is missing, the function is removed entirely because it appears in the first module's $PowerForgeDependencyCommands. This seems like acceptable behavior, but noting it explicitly in the Write-Warning message (or the PR description) would help operators diagnose unexpected export removals.

---

Test Coverage

Good baseline coverage:

• Plan_KeepsModuleOnlyCommandDependencies — direct regression for the planner bug.
• Analyze_InferModuleOnlyDependency_FromKnownCommandPattern — naming heuristic path.
• Analyze_PropagatesModuleDependency_ThroughPrivateHelper — transitive call graph.
• Analyze_TreatsExplicitExportedCommandNames_AsConditionalExports — explicit command list.
• Analyze_InferModuleOnlyDependency_FromTextPattern — TextLooksLikeModuleDependency path.
• BuildSources_EmitsConditionalExportFilter_WhenCommandDependenciesAreProvided — generated code shape.

Gaps worth considering:

• No test for a function that calls both a known external command and a local helper, to verify the graph traversal doesn't short-circuit.
• No test for ResolveCommandSource returning a non-null source that matches the module name (the Get-Command fallback path). This path is hard to test without the target module installed, so a skip/conditional test or a note that it is intentionally untested would be helpful.
• The whitespace-only changes in ModulePipelineRegressionParityTests.cs (lines around the Run_KeepsCommandModuleDependenciesInPlanWithoutPersistingManifestKey closing braces) should be reverted to keep the diff clean.

---

Minor

• FormatPsStringList is now internal in ModuleConditionalExportBlockBuilder. The visibility change from private (in ModuleMergeComposer and ModuleBootstrapperGenerator) to internal is needed for the tests. Fine, but consider adding a test that exercises it directly rather than relying on the integration-level tests to cover edge cases (empty list, list with duplicates, single-quoted values containing apostrophes).
• EscapePsDoubleQuoted is a good addition; the backtick-escaping for the module label in Write-Warning is correct.

---

Summary

The core fix (preserving module-only entries in the plan) and the new conditional export generation are sound. The main actionable items are:

1. Memoize DependsOnModule results to avoid redundant traversals for large modules.
2. Add a comment (or extend heuristics) for TextLooksLikeModuleDependency to document what import forms are and aren't matched.
3. Clean up the whitespace-only diff in ModulePipelineRegressionParityTests.cs.
4. Consider documenting the CI vs. dev-machine behavior gap for ResolveCommandSource so operators aren't surprised when optional-module-based detection works locally but not in CI.

Everything else is low-risk polish.

[claude]

Code Review: Restore conditional command exports

Overview

This PR restores a feature where New-ConfigurationCommand -ModuleName ... entries without explicit command names are preserved in the build plan, and the generated PSM1 export block conditionally filters exported functions/aliases when optional dependency modules (ActiveDirectory, DnsServer, DhcpServer, etc.) are unavailable at runtime. The implementation is well-structured and the root cause is clearly described.

---

Code Quality & Architecture

Positives:

• Clean separation of concerns: analysis (CommandModuleExportDependencyAnalyzer), code generation (ModuleConditionalExportBlockBuilder), and orchestration are all separate.
• Good use of case-insensitive Dictionary/HashSet throughout.
• ResolveScriptFiles is correctly extracted into a reusable internal method so the analysis and merge paths can share it.
• The generated PowerShell is safe and idiomatic — try/catch around Import-Module -ErrorAction Stop is the right pattern.
• The DependsOnModule DFS with visited for cycle detection and memo for memoization is correct.

---

Potential Issues

1. PowerShell.Create() inside ResolveCommandSource — performance and environment risk

using var ps = PowerShell.Create();
ps.AddCommand("Get-Command").AddParameter("Name", commandName)...
var result = ps.Invoke();

This creates a full PowerShell runspace for every uncached command that isn't covered by naming heuristics or explicit configuration. On modules with many external dependencies that aren't in the well-known list, this could spawn dozens of runspaces during build — each potentially blocking while PowerShell loads. The cache prevents duplicate calls, but the first pass through a large module could be slow.

More importantly, this reflects the build host's environment. On a clean CI machine, optional modules won't be present, so Get-Command returns nothing, and this path silently contributes nothing. The comment acknowledges this, but consider whether it's worth keeping at all given it can never work in typical CI scenarios and degrades build performance. If it's kept, wrapping the ps.Invoke() with a timeout would be defensive.

2. Double computation of ResolveConditionalExportDependencies

In TryRegenerateBootstrapperFromManifest, the script files are re-enumerated and the AST analysis re-runs:

var conditionalExportDependencies = plan is null
    ? new Dictionary<string, string[]>(StringComparer.OrdinalIgnoreCase)
    : ResolveConditionalExportDependencies(
        plan,
        ModuleMergeComposer.ResolveScriptFiles(buildResult.StagingPath, plan.Information),
        exports);

ApplyMerge also calls ResolveConditionalExportDependencies. These are separate code paths so some duplication is unavoidable, but if both paths execute in the same pipeline run (merge + bootstrapper regeneration), the AST parse happens twice on the same files. Consider caching the result in ModulePipelineRunner state or passing it through.

3. $PowerForgeAllAliases fetched once per failed module import

In the generated PowerShell:

} catch {
    $PowerForgeDependencyCommands = @($PowerForgeCommandModuleDependencies[$PowerForgeDependencyModule])
    $PowerForgeAllAliases = @(Get-Alias -ErrorAction SilentlyContinue)

Get-Alias is called inside each catch block, so if two optional modules fail to load, aliases are fetched twice. Moving this above the loop (unconditionally, or lazily before the foreach) would be slightly cleaner and cheaper.

4. Hardcoded module noun-prefix heuristics in CommandLooksLikeModuleCommand

if (string.Equals(moduleName, "ActiveDirectory", StringComparison.OrdinalIgnoreCase))
    return noun.StartsWith("AD", StringComparison.OrdinalIgnoreCase);
if (string.Equals(moduleName, "DnsServer", StringComparison.OrdinalIgnoreCase))
    return noun.StartsWith("DnsServer", StringComparison.OrdinalIgnoreCase);
if (string.Equals(moduleName, "DhcpServer", StringComparison.OrdinalIgnoreCase))
    return noun.StartsWith("DhcpServer", StringComparison.OrdinalIgnoreCase);

This only applies when configuredCommands.Count == 0 (module-only declaration), so it's a fallback. But new modules (e.g., ExchangeOnlineManagement, Az.*) won't be inferred this way. The generic fallback noun.StartsWith(moduleName, ...) at the end is helpful, but module names rarely match their command noun prefix exactly (e.g., FailoverClusters vs Get-ClusterNode). This is inherent to the heuristic approach — just worth documenting the limitation clearly so users understand they need explicit command names for non-standard modules.

5. NormalizeDependencies in ModuleConditionalExportBlockBuilder silently drops empty-command entries

if (commands.Length == 0)
    continue;

After CommandModuleExportDependencyAnalyzer.Analyze runs, if a module-only declaration produces no inferred function dependencies (e.g., none of the heuristics fire), the module is absent from the analyzer's result. NormalizeDependencies then also drops it. This is correct behavior but means the user gets no warning that a module-only declaration had no effect. A logger.Verbose() or logger.Warn() at the ResolveConditionalExportDependencies call site when a configured module dependency ends up gating zero functions would help users debug misconfigured setups.

---

Minor / Style

• LooksLikeCommandName allocates a new[] array of char literals on every call. Consider a static readonly field.
• TextLooksLikeModuleDependency scans raw function text, so a comment like # See Import-Module DnsServer would trigger a false positive. Low practical risk but worth noting.
• The extra blank line at line 790 of ModulePipelineRegressionParityTests.cs is trivial but visible in the diff.

---

Test Coverage

Tests are well-structured and cover the key scenarios:

• Module-only dependency inference via naming heuristics (Analyze_InferModuleOnlyDependency_FromKnownCommandPattern)
• Transitive dependency propagation through private helpers (Analyze_PropagatesModuleDependency_ThroughPrivateHelper)
• Explicit command name path (Analyze_TreatsExplicitExportedCommandNames_AsConditionalExports)
• Text pattern fallback (Analyze_InferModuleOnlyDependency_FromTextPattern) as a [Theory]
• PSM1 output shape (BuildSources_EmitsConditionalExportFilter_WhenCommandDependenciesAreProvided)
• Regression test for module-only plan preservation (Plan_KeepsModuleOnlyCommandDependencies)

Missing coverage to consider:

• What happens when Get-Command fallback is the only signal (may be hard to test without mocking)
• Cycle detection in DependsOnModule (A→B→A where C has the module dependency)
• A module with no inferrable functions to gate (to verify the silent-drop behavior)
• Generated PowerShell with a module name containing a single quote (tests EscapePsSingleQuoted indirectly, but an explicit test would catch regressions)

---

Summary

The implementation is solid. The core analyzer logic, generated PowerShell, and test coverage are all good quality. The main things worth addressing before merge:

1. Consider removing or disabling ResolveCommandSource/PowerShell.Create() — it can't work in CI, may slow builds, and the explicit-command + heuristic paths are authoritative.
2. Add a warning/verbose log when a configured module dependency gates zero functions after analysis.
3. Minor: move $PowerForgeAllAliases fetch outside/above the per-module catch block.

[claude]

Code Review: PR #329 — Restore Conditional Command Exports

Overview

This PR fixes two related issues:

1. New-ConfigurationCommand -ModuleName ... entries without explicit command names were discarded during planning.
2. The generated C# export block always exported every manifest function, bypassing the older filtering behavior for optional modules (ActiveDirectory, DHCPServer, DNSServer).

The solution is well-structured: a new static analyzer (CommandModuleExportDependencyAnalyzer) inspects PS ASTs to discover which exported functions depend on optional modules, a new builder (ModuleConditionalExportBlockBuilder) generates conditional PowerShell export logic, and the plan pipeline is wired together correctly.

---

Code Quality & Style

Strengths

• Consistent StringComparer.OrdinalIgnoreCase usage throughout — good defensive practice.
• The DFS with shared dependencyMemo + per-call visited set is a correct cycle-safe traversal. Memoization across the exported loop avoids redundant traversals.
• ResolveScriptFiles was cleanly extracted as an internal static, and NormalizeScriptFiles removes duplication. Good refactor.
• Defensive null handling and best-effort I/O error handling throughout (catch { /* best effort */ }).
• EmbeddedScripts template pattern is preserved correctly by reducing the bootstrapper template to {{ExportBlock}}.

Minor nits

• Whitespace-only changes on lines 742–756 of ModulePipelineRegressionParityTests.cs (trailing spaces removed from braces) add noise to the diff.
• FormatPsStringList is now internal on ModuleConditionalExportBlockBuilder — this is fine since tests use it indirectly, but if it's only needed internally it could be private.

---

Potential Bugs / Issues

1. DependsOnModule memos false for cycle-interrupted paths (subtle, but currently correct)

When a cycle is detected mid-traversal, !visited.Add(name) returns false which short-circuits to return false. The caller then backtracks and memos the correct result. This is correct because directModuleHits is pre-computed before the DFS, so the memoized false is never for a node that is truly a direct hit. Worth a comment though — this is non-obvious.

2. TextLooksLikeModuleDependency matches raw function text, including comments

text.IndexOf("Microsoft.ActiveDirectory.Management", StringComparison.OrdinalIgnoreCase) >= 0

A comment like # This does NOT use Microsoft.ActiveDirectory.Management would produce a false positive and cause the function to be gated on ActiveDirectory availability. This is documented as intentional ("historical simple forms"), but worth noting as a known limitation.

3. CommandLooksLikeModuleCommand generic fallback may over-match short module names

return noun.StartsWith(moduleName, StringComparison.OrdinalIgnoreCase);

For a module named "IT" or "Net", any command whose noun starts with those letters would be flagged. The three hard-coded modules (ActiveDirectory, DnsServer, DhcpServer) are safe, but future entries relying on the generic fallback could produce false positives.

4. Double filesystem enumeration when bootstrapper is regenerated

In TryRegenerateBootstrapperFromManifest, ResolveScriptFiles is called fresh:

ModuleMergeComposer.ResolveScriptFiles(buildResult.StagingPath, plan.Information)

But ApplyMerge (which runs first) already resolved script files. For large modules this means two full directory enumerations. Low severity — not a correctness issue — but could be avoided by passing the already-resolved list downstream.

---

Security Considerations

ResolveCommandSource spawns a live PowerShell instance at build time

using var ps = PowerShell.Create();
ps.AddCommand("Get-Command").AddParameter("Name", commandName)...
var result = ps.Invoke();

• This reflects the build host's installed modules, making results environment-sensitive. Two developers building the same source could get different outputs if one has ActiveDirectory installed and one does not.
• The code comments acknowledge this explicitly ("Best-effort fallback … reflects the build host"), and naming heuristics are preferred when available, so this is an intentional design trade-off rather than a bug.
• No injection risk since commandName values come from AST extraction (not user input) and are passed as a typed parameter, not a string that is shell-expanded.

Generated PowerShell variables are injected into module scope

The generated block uses $PowerForgeCommandModuleDependencies, $PowerForgeFunctionsToRemove, etc. as psm1-scope variables. If any user module already defines variables with these names, behavior would be undefined. The prefix is distinctive enough that this is a very low-probability conflict.

Import-Module side effects at module load time

Import-Module -Name $PowerForgeDependencyModule -ErrorAction Stop -Verbose:$false

This is called in the generated psm1 on every module import. Modules like ActiveDirectory may require domain connectivity at import time. A catch correctly handles the failure case (and removes the functions), but end users may see unexpected latency or error messages during module load. Worth documenting in user-facing guidance.

EscapePsDoubleQuoted does not escape $

private static string EscapePsDoubleQuoted(string value)
    => (value ?? string.Empty).Replace("`", "``").Replace("\"", "`\"");

Used to produce the Write-Warning string. If a module name contained $ (e.g., "My$Module"), the generated warning string would attempt PowerShell variable expansion. Module names with $ are invalid in practice, but a defensive .Replace("$", "$")` would make the escaping complete.

---

Test Coverage

• CommandModuleExportDependencyAnalyzerTests covers: command-name heuristic, helper propagation, explicit command list, and text pattern inference. Good breadth.
• ModuleMergeComposerTests.BuildSources_EmitsConditionalExportFilter_WhenCommandDependenciesAreProvided verifies generated content — good.
• Plan_KeepsModuleOnlyCommandDependencies directly tests the planning-layer fix.

Gaps worth considering:

• No test for a module name that matches the generic CommandLooksLikeModuleCommand fallback (non-hardcoded module).
• No test for the ResolveCommandSource / Get-Command fallback path (this is hard to unit-test and may be intentionally skipped).
• No negative test: a function that is similar to an AD function (e.g., Get-ADSomethingInternal defined locally) should NOT be gated — verify the localFunctionNames exclusion in FunctionDirectlyDependsOnModule.

---

Summary

Area	Rating
Correctness of the core fix	✅ Solid
Analyzer logic (DFS + memoization)	✅ Correct
Generated PowerShell quality	✅ Good
Test coverage	✅ Good, minor gaps
Environment sensitivity of Get-Command fallback	⚠️ Known trade-off, documented
Text-pattern false positives	⚠️ Known limitation
Double file enumeration	ℹ️ Low severity
$ not escaped in double-quoted PS strings	ℹ️ Edge case, low risk

Overall this is a well-reasoned, well-tested fix. The main trade-offs (live Get-Command, text-pattern matching) are documented and acceptable. The $ escaping omission and generic module-name heuristic are worth hardening before this goes to a broad audience.

[claude]

Code Review — PR #329: Restore conditional command exports

Overview

This PR restores the historical behaviour of filtering module exports based on optional module availability (ActiveDirectory, DnsServer, DhcpServer). The fix has two parts:

1. Plan layer (ModulePipelineRunner.Plan.cs): preserve module-only New-ConfigurationCommand entries that have no explicit command names.
2. Merge/bootstrapper layer: analyse script ASTs to infer which exported functions depend on those optional modules and emit a PowerShell conditional-export block.

The layering follows AGENTS.md conventions (analyzer in PowerForge.PowerShell; export-block builder in PowerForge), test coverage is solid, and graceful degradation is well handled throughout. Below are the issues worth addressing before merge.

---

Issues

1. ResolveCommandSource invokes PowerShell.Create() at build time — non-deterministic and potentially slow (Risk: Medium)

// CommandModuleExportDependencyAnalyzer.cs ~L284
using var ps = PowerShell.Create();
ps.AddCommand("Get-Command")
    .AddParameter("Name", commandName)
    ...
var result = ps.Invoke();

This runs Get-Command against the build host's loaded modules. Consequences:

• Non-determinism: a developer with ActiveDirectory RSAT installed gets different analysis output than CI, which typically doesn't. The generated PSM1 will differ between machines.
• Performance: a novel (uncached) command name triggers a full in-process PowerShell invocation. Modules with many external calls can accumulate significant overhead here.
• Auto-loading side effects: PowerShell's command resolution can auto-load modules from $env:PSModulePath, causing unexpected imports during packaging.

The cache (commandSourceCache) prevents duplicate calls, but the first hit per command name still pays the full cost. Since the comment already labels this "best-effort fallback", consider disabling it by default and only enabling it via an explicit opt-in flag, or remove it entirely in favour of the naming heuristics + explicit command lists.

2. False positive when a local function name matches a configured command (Minor, but correctness risk)

In FunctionDirectlyDependsOnModule:

if (configuredCommands.Count > 0 && configuredCommands.Contains(command))
    return true;          // ← checked BEFORE local-function skip

if (localFunctionNames.Contains(command))
    continue;

If a developer defines a local helper named Get-ADUser (unlikely but valid) and that name also appears in configuredCommands, the function will be incorrectly flagged as depending on ActiveDirectory. The local-function check should come first:

if (localFunctionNames.Contains(command))
    continue;
if (configuredCommands.Count > 0 && configuredCommands.Contains(command))
    return true;

3. ResolveConditionalExportDependencies may find no script files when called from TryRegenerateBootstrapperFromManifest (Potential correctness bug)

TryRegenerateBootstrapperFromManifest now calls:

ModuleMergeComposer.ResolveScriptFiles(buildResult.StagingPath, plan.Information)

ResolveScriptFiles looks for Public/, Private/, Classes/, Enums/ directories inside StagingPath. If this method runs after ApplyMerge has already collapsed those directories into a single <Module>.psm1, the script file list will be empty, the analyzer will return no dependencies, and the bootstrapper will be generated without any conditional export filtering — silently.

Please verify the relative execution order of these two code paths and add a log warning (or assertion) if scriptFiles is unexpectedly empty while plan.CommandModuleDependencies is non-empty.

4. TextLooksLikeModuleDependency is hardcoded to three modules (Design note)

if (string.Equals(moduleName, "ActiveDirectory", ...) && ...)
if (string.Equals(moduleName, "DnsServer", ...) && ...)
if (string.Equals(moduleName, "DhcpServer", ...) && ...)

Any module not in this list can only match via AST command names or naming heuristics. This is fine for the current scope (restoring parity), but the method name implies generic behaviour. Either rename it to reflect the hardcoded scope (TextLooksLikeKnownWindowsModuleDependency) or add a comment clarifying that new modules require a code change here.

5. Minor: missing blank line before test method

// ModulePipelineRegressionParityTests.cs
        }
    }
    [Fact]   // ← no blank line
    public void ValidateMissingFunctions_InfersLikelyModule_FromCommandModuleDependencies()

---

Positives

• Graceful degradation: parse failures, I/O errors, and analyzer exceptions all fall back cleanly without breaking the build.
• Memoised DFS in DependsOnModule with a per-module dependencyMemo correctly avoids quadratic cost on shared helpers.
• $PowerForgeAllAliases lazy init: Get-Alias only runs if at least one optional module import fails — good optimisation.
• NormalizeConfiguredDependencies correctly preserves module-only entries (empty command arrays), which is the root-cause fix.
• ResolveScriptFiles extraction: moving file discovery into a shared static method is a clean improvement that enables the pre-analysis without duplicating directory-enumeration logic.
• Test coverage covers all four analysis paths (configured commands, naming heuristics, text patterns, helper propagation).

---

Summary

Issues 1–3 are the ones most worth addressing before merge: the Get-Command fallback introduces non-determinism that could produce different PSM1 outputs across environments, the local-vs-configured ordering is a latent correctness bug, and the double-analysis concern could silently produce a bootstrapper without the conditional export block. Issues 4–5 are minor.

🤖 Generated with Claude Code