Skip to content

Conversation

@cfsmp3
Copy link
Contributor

@cfsmp3 cfsmp3 commented Dec 23, 2025

Summary

This PR fixes an issue where release webhooks would create test entries with invalid or incorrect commit hashes (such as 0000000000000000000000000000000000000000).

The Problem

The previous code used last_commit from GeneralData to get the commit hash for releases. This is problematic because:

  • last_commit represents HEAD of master at the time of the last push event
  • It may not match the actual commit the release tag points to
  • If webhooks arrive out of order or a release is created for an older tag, the commit would be wrong
  • In some edge cases, it could be the Git null SHA which blocks subsequent tests

The Solution

  1. New is_valid_commit_hash() helper function - Validates that a commit hash is:

    • Not empty/None
    • 7-40 characters (valid Git hash length)
    • Valid hexadecimal
    • Not the null SHA (all zeros)
  2. Release webhook now fetches commit from tag - Uses GitHub API to get the actual commit:

    • Handles lightweight tags (point directly to commit)
    • Handles annotated tags (need to dereference to get commit)
    • Falls back to last_commit only if tag lookup fails
  3. Validation in add_test_entry() - Rejects invalid commit hashes before creating test entries

  4. Null-safe baseline update - Checks if test exists before trying to update baseline (prevents AttributeError crash)

Credit

Based on issue and approach identified by @NexionisJake in PR #937. This PR provides an improved implementation with:

  • A reusable validation helper function
  • Comprehensive unit tests
  • Cleaner error handling

Test Plan

  • New unit tests for is_valid_commit_hash() function
  • New unit tests for release webhook with lightweight tags
  • New unit tests for release webhook with annotated tags
  • New unit tests for fallback behavior when tag lookup fails
  • New unit tests for graceful failure with invalid commits
  • Existing release webhook tests still pass

🤖 Generated with Claude Code

@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.02%. Comparing base (78c3f60) to head (01d8ace).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
mod_ci/controllers.py 93.47% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
+ Coverage   86.98%   87.02%   +0.04%     
==========================================
  Files          35       35              
  Lines        3780     3816      +36     
  Branches      774      783       +9     
==========================================
+ Hits         3288     3321      +33     
- Misses        354      356       +2     
- Partials      138      139       +1     
Flag Coverage Δ
unittests 87.02% <93.47%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cfsmp3 and others added 4 commits December 23, 2025 17:51
This fixes an issue where release webhooks would create test entries with
invalid or incorrect commit hashes. The previous code used `last_commit`
from GeneralData, which represents the HEAD of master at the time of the
last push event - not necessarily the commit the release tag points to.

Changes:
- Add `is_valid_commit_hash()` helper function to validate commit hashes
- Modify release webhook to fetch commit directly from tag via GitHub API
- Handle both lightweight and annotated tags correctly
- Fall back to last_commit only if tag lookup fails
- Add validation in `add_test_entry()` to reject invalid commits
- Handle case where no test exists for the commit (prevents crash)
- Add comprehensive unit tests for all scenarios

This prevents:
- Test entries with null SHA (0000000...)
- Test entries with stale/wrong commit hashes
- Crashes when test is None during baseline update

Credit: Based on issue identified by NexionisJake in PR #937

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Update existing tests to mock get_git_ref for release webhooks
- Replace invalid commit hash strings with valid 40-char hex strings
- 'customizedcommitcheck' -> valid hex (not valid hex chars)
- 'abcdefgh' -> valid 40-char hex (was only 8 chars)
- Mock commit hashes in new tests must be valid hex

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add missing test cases to increase patch coverage:
- Test whitespace stripping in is_valid_commit_hash()
- Test rejection of hashes > 40 characters
- Test release webhook with existing test entry (covers baseline update path)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@sonarqubecloud
Copy link

@canihavesomecoffee canihavesomecoffee merged commit 7d83adf into master Dec 23, 2025
8 checks passed
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