fix: Prevent local NuGet modules from running in CI PR builds

[thomhurst]

Summary

Fixes the pipeline failure where CI incorrectly registers local NuGet modules on PR branches.

Root Cause

The GitHub Actions workflow sets DOTNET_ENVIRONMENT=Development for non-main branches. This caused IsDevelopment() to return true, registering local NuGet modules (UploadPackagesToLocalNuGetModule) that depend on PackagePathsParserModule which wasn't registered in that code path.

Fix

Modified Program.cs to use BuildSystemDetector.Instance.IsKnownBuildAgent to distinguish:

Environment	IsDevelopment	IsKnownBuildAgent	Modules Registered
Local dev	true	false	Local NuGet modules
CI main	false	true	NuGet + Release modules
CI PR	true	true	Neither (just test/build)

The BuildSystemDetector already exists and detects CI via environment variables like GITHUB_ACTIONS, TF_BUILD, GITLAB_CI, etc.

Test plan

• PR builds should no longer fail with ModuleNotRegisteredException
• Other PRs (chore: Add ExcludeFromCodeCoverage to generated Docker/Helm services and document CommandExtensions #1672) should pass their pipeline checks

🤖 Generated with Claude Code

[thomhurst]

Summary

Fixes CI PR builds by preventing local NuGet modules from being registered when running in CI environments.

Critical Issues

None found ✅

Suggestions

Logic clarity improvement:
The current code at src/ModularPipelines.Build/Program.cs:66-82 has three branches but the third case (CI PR builds) is implicit in the final else comment. Consider making the second condition more explicit by checking both IsKnownBuildAgent and !IsDevelopment(). However, the current implementation is functionally correct.

Minor: Namespace organization
The new using ModularPipelines; statement could be placed alphabetically before using ModularPipelines.Build; for consistency, though this is a style preference.

Verdict

✅ APPROVE - No critical issues

The fix correctly addresses the root cause by distinguishing between local development and CI environments using BuildSystemDetector. The dependency on PackagePathsParserModule will no longer be missing in CI PR builds since those modules will not be registered at all.

[thomhurst]

Summary

Fixes module registration logic to distinguish between local development and CI PR builds by using BuildSystemDetector.

Critical Issues

None found ✅

Suggestions

Logic clarity in edge case: The current implementation has three paths:

1. Local dev (IsDevelopment + !IsKnownBuildAgent) → registers local NuGet modules
2. Production (!IsDevelopment) → registers NuGet.org upload + release modules
3. PR builds in CI (IsDevelopment + IsKnownBuildAgent) → registers nothing

The third case (PR builds) intentionally falls through both conditionals, which is correct but not immediately obvious. The comment "else: CI Development mode (PR builds) - don't register..." helps, but consider explicitly checking for this case to make the intent clearer:

if (isLocalDevelopment)
{
    // Local NuGet modules
}
else if (!context.HostingEnvironment.IsDevelopment())
{
    // Production modules
}
else
{
    // PR builds in CI - explicitly do nothing
    // BuildSystemDetector.Instance.IsKnownBuildAgent is true here
}

This makes it impossible to accidentally break the PR build case when refactoring.

Minor: The variable name isLocalDevelopment is excellent and self-documenting. The comments are also helpful.

Verdict

✅ APPROVE - No critical issues

The logic is sound and correctly handles the three deployment scenarios. The use of BuildSystemDetector.Instance.IsKnownBuildAgent is the right approach given that DOTNET_ENVIRONMENT=Development is set for PR builds in the GitHub workflow (dotnet.yml:77).

[thomhurst]

Summary

Fixes pipeline failures on PR branches by preventing local NuGet modules from being registered when running in CI.

Critical Issues

1. Implementation doesn't match PR description 🚨

The PR description states:

Modified Program.cs to use BuildSystemDetector.Instance.IsKnownBuildAgent

But the actual implementation manually checks environment variables:

var isRunningInCI = \!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("CI"))
    || \!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("GITHUB_ACTIONS"))
    || \!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("TF_BUILD"))
    || \!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("GITLAB_CI"));

This duplicates logic from BuildSystemDetector (src/ModularPipelines/BuildSystemDetector.cs:38-48) which already checks these exact variables.

Recommendation: Use the existing BuildSystemDetector.Instance.IsKnownBuildAgent as described in the PR:

var isLocalDevelopment = context.HostingEnvironment.IsDevelopment() 
    && \!BuildSystemDetector.Instance.IsKnownBuildAgent;

This eliminates duplicate logic and ensures consistency across the codebase.

2. Checking undefined CI environment variable

The code checks for a CI environment variable which is:

• Not documented in BuildSystemDetector (see src/ModularPipelines/BuildSystemDetector.cs:7-21)
• Not part of the official build system detection logic
• May not be set by all CI systems

If BuildSystemDetector should detect this variable, it should be added there, not duplicated here.

Suggestions

Comment clarity: The comment on line 71 says "Production environment (main branch CI)" but this condition (\!context.HostingEnvironment.IsDevelopment()) would also be true for any Production DOTNET_ENVIRONMENT, not just main branch CI. Consider clarifying.

Verdict

⚠️ REQUEST CHANGES - Implementation should use existing BuildSystemDetector infrastructure instead of duplicating environment variable checks.