Harden git-sync abbreviated commit verification

[PrzemyslawKlys]

Summary

• Accept unambiguous abbreviated resolved HEAD commits when verifying against a full lock SHA
• Prefer the exact checked-out lock SHA when HEAD resolution returns an abbreviated value
• Add focused git-sync tests for abbreviated commit handling

Tests

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

[claude]

Code review posted below - please see the review details.

[claude]

Code Review: Harden git-sync abbreviated commit verification

Overview

This PR fixes a real-world robustness issue: git rev-parse HEAD can return an abbreviated commit hash on some systems/configurations, causing the verify lock mode to throw even though the correct commit was checked out. The fix introduces two private helpers (CommitMatchesLockedCommit, SelectResolvedCommit) and extracts the existing hex-character loop into a reusable IsHexString.

---

Code Quality and Correctness

CommitMatchesLockedCommit (lines 380-394)

The logic is sound: exact match first, then prefix-match with safeguards (minimum 12 chars, resolved.Length < locked.Length, both must be hex).

• The minimum length of 12 is conservative relative to git default abbreviation (7-12 chars). A brief inline comment would help future readers understand why 12 was chosen.
• IsHexString returns true for an empty string (the foreach exits immediately). Not reachable via the guards in CommitMatchesLockedCommit, but the function has an implicit non-empty precondition. Not a bug, just worth noting.

SelectResolvedCommit (lines 367-378)

Priority order (full resolved > full checked-out > non-empty resolved > checked-out) is well-reasoned. No issues.

Potential inconsistency on line 103

The reference validation on line 103 still uses exact string.Equals while the resolved-commit check now uses the lenient CommitMatchesLockedCommit. Likely intentional (user-supplied references should be explicit), but the asymmetry is worth a comment so it is not silently changed later.

---

Security Considerations

The prefix-match approach is standard git behavior and the 12-char minimum significantly limits the collision window. The condition resolved.Length < locked.Length correctly prevents a resolved full SHA from being accepted as a prefix match of a different locked SHA of the same length. No concerns.

---

Test Coverage

RunPipeline_GitSync_LockModeVerify_AcceptsAbbreviatedResolvedCommit

• Tests 16-char match (pass) and non-matching 16-char value (fail) - good
• Missing: 11-char abbreviated commit should fail (below the 12-char minimum) - worth adding to confirm the boundary is enforced
• Missing: resolved longer than locked case

RunPipeline_GitSync_PrefersCheckedOutFullCommitWhenHeadIsAbbreviated

• Tests the main branch (abbreviated head, full checked-out) - good
• Missing: both null, abbreviated head with null checked-out (should return abbreviated), full head (should return head ignoring checked-out)

Reflection-based testing: Using BindingFlags.NonPublic is pragmatic; Assert.NotNull(method) catches renames but signature changes throw at Invoke with a less clear error. Pre-existing pattern, no change needed now.

---

Minor Suggestions

1. Add the 11-char boundary test: var tooShort = method.Invoke(null, new object[] { lockedCommit.Substring(0, 11), lockedCommit }); Assert.False(tooShort);

2. Add a comment next to the 12 constant in CommitMatchesLockedCommit explaining the threshold choice.

3. Add a comment on line 103 noting the intentional exact-match-only policy for user-supplied references vs. the lenient prefix policy for git-resolved commits.

---

Summary

The core change is correct and well-scoped. The IsHexString extraction is a nice cleanup. Main feedback: a few missing boundary-case tests (particularly the 11-char boundary) and clarity comments on the minimum-length threshold and the asymmetry between reference validation and resolved-commit validation. Logic is solid overall.