Harden git-sync verify commit resolution

[PrzemyslawKlys]

Summary

• fall back to the verified 40-character commit when git-sync has already checked it out successfully
• keep verify-mode commit comparison deterministic when final HEAD resolution is unavailable

Tests

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

[claude]

Code Review — PR #337: Harden git-sync verify commit resolution

Overview

This PR adds a fallback mechanism for ExecuteGitSyncRequest: when the checked-out Reference is already a full 40-char SHA and ResolveHeadCommit (i.e. git rev-parse HEAD) returns null, the known SHA is used as the Commit result instead of propagating null. This prevents a false "resolved commit 'unknown' does not match lock commit" error in verify mode when git's HEAD resolution temporarily fails despite a successful checkout.

---

Correctness ✅

The logic is sound. In verify mode, lockedCommit (a full SHA from the lock file) is injected as request.Reference via WithReference. If checkout succeeds but rev-parse HEAD fails, falling back to the reference SHA itself is accurate — the SHA was explicitly checked out and is the ground truth.

The null-coalescing pattern ResolveHeadCommit(...) ?? checkedOutCommit is appropriately conservative: the fallback only activates when ResolveHeadCommit returns null, never overriding a successfully resolved commit.

---

IsFullGitCommitSha implementation ✅

Clean, readable, and avoids a regex dependency. One minor observation:

// In IsFullGitCommitSha:
var text = value.Trim();   // trims here...

// At call site:
checkedOutCommit = request.Reference!.Trim();  // ...and trims again

The double-trim is harmless but redundant. Consider either removing the .Trim() at the call site (since the helper already normalises whitespace) or removing it from inside the helper and handling it only at the call site. Neither causes a bug, but it is slightly inconsistent.

---

Edge cases

• CheckoutReference takes the FETCH_HEAD path: if git checkout <sha> fails and the code falls through to git fetch origin <sha> && git checkout FETCH_HEAD, checkedOutCommit is still set to request.Reference. This is correct — on success, FETCH_HEAD and the requested SHA are the same object.
• CheckoutReference throws: if checkout throws entirely, checkedOutCommit is set but the exception propagates before GitSyncExecutionResult is constructed, so the fallback is never reached. No issue.
• DisplayReference has no equivalent fallback: ResolveHeadReference (used for the summary message) still returns null if git fails, but that only affects the human-readable summary string, not correctness or the verify assertion. Acceptable.

---

Test coverage ⚠️

The existing RunPipeline_GitSync_LockModeVerify_UsesLockedCommit test exercises the verify-mode happy path end-to-end, but it does not exercise the specific fallback introduced by this PR — the branch where ResolveHeadCommit returns null and checkedOutCommit is substituted instead.

Without a test covering that path, a future refactor of ResolveHeadCommit (e.g. changing its return contract) could silently break the fix. Consider adding a test — even a simple unit test for IsFullGitCommitSha and an integration test that simulates or stubs a failing rev-parse HEAD — to lock in the behaviour.

---

Minor suggestions

1. IsFullGitCommitSha — consider ReadOnlySpan<char> if this is ever called in a hot path: the current foreach over a string is fine for this use case, but worth noting if performance ever becomes a concern.
2. Comment on the fallback: the intent ("git rev-parse HEAD can fail even after a successful checkout") is non-obvious. A one-line comment on the ?? expression would help future readers understand why the fallback exists.

---

Summary

The fix is targeted, correct, and well-structured. The IsFullGitCommitSha helper is clean. The main gap is the absence of a test that specifically validates the ResolveHeadCommit → null → checkedOutCommit fallback path, which is the core behaviour being hardened. Everything else looks good.