Pass sources-sync defaults to git-sync

[PrzemyslawKlys]

Summary

• pass sources-sync step-level git defaults through to the generated git-sync batch
• cover the default passthrough with a pipeline runner regression test

Tests

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj --filter "FullyQualifiedName~WebPipelineRunnerSourcesSyncTests" --framework net10.0 --logger "console;verbosity=minimal"

[claude]

Code Review — PR #336: Pass sources-sync defaults to git-sync

Overview

This PR fills a real gap: step-level git defaults set on a sources-sync pipeline step (e.g. authType, retry, depth) were previously ignored when building the per-repo git-sync batch. The fix forwards them as top-level keys in gitSyncStep, which ParseGitSyncRequest already consumes as the defaults object via GetStringFrom/GetBoolFrom/GetIntFrom. Null-valued keys are harmless thanks to the !string.IsNullOrWhiteSpace / .HasValue guards in those helpers. The approach is correct and consistent with the existing serialise-round-trip pattern.

---

Implementation — WebPipelineRunner.Tasks.SourcesSync.cs

Bug / inconsistency — incomplete alias coverage for repoBaseUrl

ParseGitSyncRequest (GitSync.cs:207–210) resolves repoBaseUrl from six aliases:

"repoBaseUrl", "repo-base-url", "repositoryBaseUrl", "repository-base-url", "repoHost", "repo-host"

The new passthrough only captures two of them:

["repoBaseUrl"] = GetString(step, "repoBaseUrl") ?? GetString(step, "repo-base-url"),

If a pipeline author uses repositoryBaseUrl, repoHost, or repo-host at the sources-sync step level, the default is silently dropped. The fix is to add the missing aliases:

["repoBaseUrl"] = GetString(step, "repoBaseUrl") ?? GetString(step, "repo-base-url")
               ?? GetString(step, "repositoryBaseUrl") ?? GetString(step, "repository-base-url")
               ?? GetString(step, "repoHost") ?? GetString(step, "repo-host"),

Omitted fields — sparseCheckout / sparsePaths

ParseGitSyncRequest supports step-level sparseCheckout and sparsePaths as defaults. These are intentionally omitted here (they're string-array types, and gitSyncStep is built from scalar GetString/GetBool/GetInt calls). That's fine provided it's a deliberate design decision — worth a short comment or a follow-up issue if sparse-checkout defaults should also be supported.

ref/branch/tag/commit not forwarded

These are also resolved from defaults in ParseGitSyncRequest. Omitting them is probably intentional (a global ref default would be unusual), but worth confirming.

Token handling

Passing token as a step-level default is consistent with existing per-repo behaviour and the inline-token warning in ExecuteGitSync will fire correctly. No new concern here.

---

Test — WebPipelineRunnerSourcesSyncTests.cs

The test structure follows established patterns and is readable. A few gaps:

1. Only authType is asserted. The PR adds 13 new forwarded fields; the test validates one. Consider adding at least one more assertion — e.g. verify retry in the manifest, or that retryDelayMs: 25 was applied (the step already sets this for speed, so it costs nothing to assert it if it's exposed in the manifest).

2. Missing step-level assertions — both existing tests assert Assert.Single(result.Steps) and Assert.True(result.Steps[0].Success). The new test only checks result.Success. Suggest adding:

   Assert.Single(result.Steps);
   Assert.True(result.Steps[0].Success);

3. Manifest entry count not validated. The test accesses entries[0] directly; if the manifest were empty or malformed, the exception message would be confusing. A quick Assert.Equal(1, entry.GetProperty("entries").GetArrayLength()) (or similar) would make failures clearer.

4. IsGitAvailable() silent skip. Pre-existing pattern — noted only for completeness. Tests that skip silently can mask gaps on git-less CI agents.

---

Summary

Area	Verdict
Core logic correctness	✅ Correct — null values are safe, alias patterns are mirrored
repoBaseUrl alias coverage	⚠️ Incomplete — 4 aliases missing
sparseCheckout support	ℹ️ Intentional omission — confirm or track
Test coverage breadth	⚠️ Only 1 of 13 forwarded fields is asserted
Test assertions completeness	⚠️ Missing step-level and manifest-count checks
Style / conventions	✅ Consistent with codebase

The incomplete repoBaseUrl alias coverage is the main concrete fix needed before merge. The test gaps are lower priority but worth addressing for regression confidence.