🐛 Fix GitHub Actions PR commit SHA detection

[Robdel12]

Summary

• Fix incorrect commit SHA detection for GitHub Actions pull_request events
• The CLI now reads GITHUB_EVENT_PATH to extract the actual PR head commit SHA instead of using GITHUB_SHA (which is a merge commit)
• This fixes check runs appearing on wrong commits and baseline detection issues

Problem

For pull_request events in GitHub Actions, GITHUB_SHA points to a temporary merge commit, not the actual head commit of the PR. This caused:

• Check runs appearing on non-existent or wrong commits
• "Check run not found" errors in GitHub
• Incorrect baseline detection (comparing against wrong ancestor)

From GitHub's official docs:

"GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead."

Solution

The CLI now reads the GitHub Actions event payload from GITHUB_EVENT_PATH (a JSON file containing the full webhook payload) and extracts pull_request.head.sha for PR events.

Scenario	Before	After
push to main	✅ Correct SHA	✅ Correct SHA
pull_request event	❌ Merge commit SHA	✅ Head commit SHA
VIZZLY_COMMIT_SHA set	✅ Uses override	✅ Uses override

Changes

• src/utils/ci-env.js:
  • Add getGitHubEvent() to read and cache event payload
  • Update getCommit() to extract PR head SHA
  • Update getPullRequestHeadSha() with same fix
  • Update getPullRequestBaseSha() to extract base SHA
• Tests updated with comprehensive coverage

Test plan

• All existing tests pass
• New tests cover PR event file reading
• New tests cover fallback to GITHUB_SHA for push events
• New tests cover error handling (missing file, invalid JSON)

[claude]

Code Review

This is a solid fix for a common GitHub Actions gotcha. The implementation is clean, well-tested, and properly documented.

✅ Strengths

1. Problem Identification & Documentation

• Correctly identifies the GitHub Actions pull_request event merge commit issue
• Excellent inline documentation with GitHub docs reference
• PR description clearly explains the problem with helpful table

2. Implementation Quality

• Clean caching pattern in getGitHubEvent() prevents redundant file reads
• Proper fallback chain: override → event payload → GITHUB_SHA → other CIs
• Silent failure on missing/invalid event file (returns empty object) - appropriate for CI environments
• Exports resetGitHubEventCache() for testing - good practice

3. Test Coverage

• Comprehensive test coverage for all scenarios:
  • PR events (reads from event file)
  • Push events (falls back to GITHUB_SHA)
  • Missing event file
  • Invalid JSON
  • Cache behavior
  • Override priority
• Tests properly clean up temp files
• Cache reset in beforeEach prevents test pollution

4. Consistency

• Same pattern applied to getCommit(), getPullRequestHeadSha(), and getPullRequestBaseSha()
• Maintains backward compatibility with other CI providers

🔍 Observations & Suggestions

1. Minor: Temporary directory cleanup could be more robust

In tests, if an assertion fails before unlinkSync(), temp files may leak. Consider using a cleanup array:

let tempFiles = [];

afterEach(() => {
  // Cleanup any remaining temp files
  tempFiles.forEach(file => {
    try { unlinkSync(file); } catch {}
  });
  tempFiles = [];
  Object.keys(process.env).forEach(key => {
    if (!(key in originalEnv)) delete process.env[key];
  });
  Object.assign(process.env, originalEnv);
});

// Then in tests:
tempFiles.push(eventPath);

But this is very minor - current approach is fine for tests that will likely never fail.

2. Optional: Consider logging when event file parsing fails

The silent failure in getGitHubEvent() is appropriate, but if debugging CI issues, it might be helpful to know when event file parsing fails. This could be a future enhancement:

try {
  let content = readFileSync(eventPath, 'utf8');
  _githubEventCache = JSON.parse(content);
} catch (error) {
  // Optional: Add debug logging here
  // logger.debug(`Failed to parse GitHub event file: ${error.message}`);
  _githubEventCache = {};
}

Not required - just a thought for future debugging scenarios.

3. Edge case: What if GITHUB_EVENT_PATH points to directory?

Currently would fail silently (good), but readFileSync would throw. The catch block handles it. All good.

4. Question: Should this handle pull_request_target events?

pull_request_target events also use merge commits. The current code handles them automatically since it checks for event.pull_request?.head?.sha regardless of event type. Might be worth mentioning in comments or tests.

🔐 Security

• ✅ No user input in file paths (GITHUB_EVENT_PATH is set by GitHub Actions)
• ✅ No command injection risks
• ✅ Proper error handling prevents crashes
• ✅ Caching doesn't expose sensitive data

🚀 Performance

• ✅ Caching prevents repeated file reads
• ✅ Synchronous file read is fine (happens once per process)
• ✅ Early returns in fallback chains

📊 Test Coverage

The test coverage is excellent. All new code paths are tested:

• ✅ Event file parsing (happy path)
• ✅ Missing file fallback
• ✅ Invalid JSON fallback
• ✅ No pull_request object fallback
• ✅ Cache behavior
• ✅ Override precedence
• ✅ Integration with existing tests updated

🎯 Verdict

This PR is ready to merge. The implementation correctly solves a real problem, is well-tested, and maintains backward compatibility. The minor suggestions above are optional enhancements, not blockers.

Nice work! 🚀