Skip to content

fix: expand commit hash regex to support SHA-256 (64-char) hashes#4347

Draft
yaananth wants to merge 2 commits intoactions:mainfrom
yaananth:sha256/fix-wellknown-regex
Draft

fix: expand commit hash regex to support SHA-256 (64-char) hashes#4347
yaananth wants to merge 2 commits intoactions:mainfrom
yaananth:sha256/fix-wellknown-regex

Conversation

@yaananth
Copy link
Copy Markdown
Contributor

Summary

Expand the core commit-hash regex in WellKnownRegularExpressions to support both SHA-1 (40-char) and SHA-256 (64-char) commit hashes, preparing the runner for Git's SHA-256 transition.

Changes

  • WellKnownRegularExpressions.cs: Update regex from \b[0-9a-f]{40}\b to \b(?:[0-9a-f]{40}|[0-9a-f]{64})\b; add CommitHash constant as canonical name while keeping SHA1 for backward compatibility; rename internal field from s_validSha1 to s_validCommitHash
  • RepositoryPlugin.cs (v1.0): Remove dead _validSha1 field that was declared but never used

Validation

  • Build passes (dev.sh build Debug)
  • 983 tests pass, 0 failures (dev.sh test Debug)
  • Backward compatibility preserved: existing SHA1 constant and all callsites unchanged

Context


Automated by project-board-agents-plugin via /project-board-agents:lead
Board: https://github.com/orgs/github/projects/24272/views/1
Item: [P0] Fix core SHA-1 regex in actions/runner WellKnownRegularExpressions.cs
Run: sha256-p0-runner-00050

- update WellKnownRegularExpressions to match both 40-char (SHA-1) and 64-char (SHA-256) hex strings
- add CommitHash constant as canonical name while keeping SHA1 for backward compatibility
- remove dead _validSha1 field in RepositoryPlugin v1.0

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
private static readonly Lazy<Regex> s_validSha1 = new Lazy<Regex>(() => new Regex(
@"\b[0-9a-f]{40}\b",
// 40 or 64 hex characters (SHA-1 or SHA-256 commit hash)
private static readonly Lazy<Regex> s_validCommitHash = new Lazy<Regex>(() => new Regex(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex change looks correct, but WellKnownRegularExpressions currently has no dedicated unit tests.

Would be good if we could add a test class (e.g. src/Test/L0/Sdk/WellKnownRegularExpressionsL0.cs alongside the existing ExpressionParserL0.cs) with these minimum:

  • 40-char hex → matches (regression guard)
  • 64-char hex → matches (the new case this PR adds)
  • 63-char and 65-char hex → do NOT match (boundary)
  • Mixed-case 64-char → matches (IgnoreCase still works)

Would be good to avoid a regression in the future, in-case a future change breaks 64-char support to avoid it going undetected since nothing in CI directly tests it.

@@ -24,7 +25,8 @@ public static Lazy<Regex> GetRegex(String regexType)
case IPv4Address:
return s_validIPv4Address;
case SHA1:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to remove the SHA1 and update all the caller to use the CommitHash?

Add dedicated test class covering:
- 40-char SHA-1 match (regression guard)
- 64-char SHA-256 match (new behavior)
- 63/65-char boundary rejection
- Mixed-case matching (IgnoreCase)
- SHA1 and CommitHash keys return same regex instance
- Unknown key returns null

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants