refactor: Extract NuGet upload logic to shared helper

[thomhurst]

Summary

• Extracted duplicated NuGet package upload logic into a new NugetUploadHelper class
• Updated UploadPackagesToNugetModule and UploadPackagesToLocalNuGetModule to use the shared helper
• Reduced code duplication by ~12 lines across both modules

Changes

• Created src/ModularPipelines.Build/Helpers/NugetUploadHelper.cs with a configurable UploadPackagesAsync method that accepts source and optional apiKey parameters
• Refactored both upload modules to delegate to the helper

Fixes #1459

Test plan

• Build succeeds locally
• CI pipeline passes
• NuGet upload functionality works as expected (verified via existing tests)

🤖 Generated with Claude Code

[github-actions]

Summary

Extracts duplicated NuGet package upload logic into a shared helper class to reduce code duplication.

Critical Issues

1. Compilation Error - Type Mismatch in UploadPackagesToLocalNuGetModule.cs

Location: src/ModularPipelines.Build/Modules/LocalMachine/UploadPackagesToLocalNuGetModule.cs:26

The code passes localRepoLocation.Value.AssertExists()! as the source parameter, but AssertExists() returns a Folder object, not a string. The helper's source parameter expects string.

Current code:

source: localRepoLocation.Value.AssertExists()!,

Should be:

source: localRepoLocation.Value.AssertExists().Path,

Or convert the Folder to string appropriately based on how the original code worked.

2. Missing Null Check for API Key

Location: src/ModularPipelines.Build/Modules/UploadPackagesToNugetModule.cs:40-48

The original code had ArgumentNullException.ThrowIfNull(_nugetSettings.Value.ApiKey) before uploading to NuGet.org. This validation was removed in the refactor, but the helper accepts nullable apiKey.

The null check should be preserved before calling the helper, since uploading to nuget.org requires an API key:

ArgumentNullException.ThrowIfNull(_nugetSettings.Value.ApiKey);

return await NugetUploadHelper.UploadPackagesAsync(
    context,
    packagePaths.Value!,
    source: "https://api.nuget.org/v3/index.json",
    apiKey: _nugetSettings.Value.ApiKey,
    cancellationToken);

Suggestions

None - the refactoring approach is sound once the critical issues are fixed.

Verdict

⚠️ REQUEST CHANGES - Critical issues found (compilation error + missing validation)

[github-actions]

Summary

Extracts duplicated NuGet package upload logic into a shared helper class to reduce code duplication.

Critical Issues

Missing Null Check for API Key

Location: src/ModularPipelines.Build/Modules/UploadPackagesToNugetModule.cs:40-48

The original code had ArgumentNullException.ThrowIfNull(_nugetSettings.Value.ApiKey) before uploading to NuGet.org (line 42). This validation was removed in the refactor, but the helper accepts nullable apiKey.

The null check should be preserved before calling the helper, since uploading to nuget.org requires an API key. The validation ensures fast-fail with a clear error message rather than failing later during the push operation.

Required fix: Keep the ArgumentNullException.ThrowIfNull(_nugetSettings.Value.ApiKey); line before calling NugetUploadHelper.UploadPackagesAsync.

Previous Review Status

The previous automated review incorrectly identified a compilation error in UploadPackagesToLocalNuGetModule.cs. Upon inspection, the code correctly includes .Path (line 26 of the diff: source: localRepoLocation.Value.AssertExists()!.Path). The implicit conversion from Folder to string would have worked anyway (see Folder.cs:260-263), but explicitly using .Path is clearer.

The null check issue identified in the previous review is valid and must be addressed.

Verdict

⚠️ REQUEST CHANGES - Critical validation removed