fix(ci.yml): handle force push gracefully in script validation #156
Conversation
📝 WalkthroughWalkthroughReplaces prior BASE_SHA derivation with a merge-base against Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the validate-scripts job that was failing when commit history changed due to force pushes. The fix differentiates between pull request and push events, implementing appropriate strategies for each to avoid git merge-base failures.
Key changes:
- For PRs: Changed from using the base commit SHA to comparing directly against the base branch reference (
origin/main) - For push events: Added error handling around
git merge-basewith a fallback toHEAD^when the common ancestor cannot be found
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
39-49: Good fallback logic; consider using GitHub Actions warning annotations.The error handling and fallback to
HEAD^is a reasonable approach. However, the warning messages on lines 44-45 use plainechowhich only appears in the step log. For better visibility in the GitHub Actions UI (surfaced in the summary), use the annotation format:Suggested improvement for warning visibility
if ! COMMON_ANCESTOR=$(git merge-base "$BASE_SHA" "$HEAD_SHA" 2>/dev/null); then - echo "Could not find common ancestor (likely due to force push)" - echo "Comparing with HEAD^ instead" + echo "::warning::Could not find common ancestor (likely due to force push). Comparing with HEAD^ instead." BASE_SHA="HEAD^"Minor edge case: If
HEAD^doesn't exist (e.g., initial commit), this would fail. While unlikely for pushes tomain, you could add a safeguard:if ! git rev-parse HEAD^ >/dev/null 2>&1; then echo "::warning::No parent commit available, skipping diff" echo "scripts_changed=false" >> "$GITHUB_OUTPUT" exit 0 fi
41-41: Minor note: redundantHEAD_SHAassignment.
HEAD_SHAis assigned here (line 41) for use in themerge-basecall, but it's also assigned unconditionally on line 52. This works correctly but is slightly redundant for push events. Not blocking—just noting for awareness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
36-37: LGTM! Robust solution for PR force-push scenarios.Using
origin/${{ github.event.pull_request.base.ref }}directly references the base branch tip, which always exists and sidesteps the merge-base issue entirely. This is a clean fix for the PR case.
There was a problem hiding this comment.
I think that this can be simplified to the below, but I'm not sure since I haven't had a chance to test it. Please will you test it in various ways on your fork and share screenshots with me of the commands you ran in your shell and the outputs in the GitHub Actions tab.
- name: Determine if scripts changed
id: check_changes
shell: bash
run: |
# Find merge base with main (safe for force pushes)
BASE=$(git merge-base HEAD origin/main)
# List added/renamed scripts
matches=$(git diff --name-status "$BASE" HEAD | grep -E '^[AR]\s+(scripts/|docs/scripts/).+\.js$')
if [ -n "$matches" ]; then
echo "Changed scripts: $matches"
echo "scripts_changed=true" >> "$GITHUB_OUTPUT"
else
echo "scripts_changed=false" >> "$GITHUB_OUTPUT"
fi
I'm sorry for not immediately replying like I do with most pull requests - I had to wait until I was available to review this one properly because my Bash skills are a bit rusty and this runs in a completely different environment.
Code Breakdown
You can use this breakdown when writing docs about the changes.
# Find merge base with main (safe for force pushes)
BASE=$(git merge-base HEAD origin/main)git merge-base HEAD origin/mainfinds the most recent common commit between the branch you’re on (HEAD) and themainbranch on the remote.- This gives us a stable reference point for comparing changes, even if
mainwas force-pushed. - The result is stored in the variable
BASE.
# List added/renamed scripts
matches=$(git diff --name-status "$BASE" HEAD | grep -E '^[AR]\s+(scripts/|docs/scripts/).+\.js$')-
git diff --name-status "$BASE" HEADlists all files changed between the base commit and the current commit.-
Output looks like:
A scripts/build.js M scripts/validate.js R docs/scripts/old.js -> docs/scripts/new.jsA→ addedR→ renamedM→ modified
-
-
grep -E '^[AR]\s+(scripts/|docs/scripts/).+\.js$'filters the output so we only get files we care about:^[AR]→ the line must start withA(added) orR(renamed).\s+→ one or more spaces separating the status letter and filename.(scripts/|docs/scripts/)→ the file must be in thescripts/ordocs/scripts/folder..+\.js$→ the filename must end with.js.
-
Combining steps 1 and 2 with a pipe (
|) sends the output of step 1 into the input of step 2, outputting the results of step 2. -
The filtered results are stored in
matches- meaning it filtered out all files that were neither added nor modified.
if [ -n "$matches" ]; then
echo "Changed scripts: $matches"
echo "scripts_changed=true" >> "$GITHUB_OUTPUT"
else
echo "scripts_changed=false" >> "$GITHUB_OUTPUT"
fi-
-n "$matches"checks ifmatchesis not empty. -
If there are changed scripts:
- Print the list for debugging:
Changed scripts: $matches - Set the GitHub Actions output variable
scripts_changedtotrue.
- Print the list for debugging:
-
If no scripts were added or renamed, set
scripts_changedtofalse.
$GITHUB_OUTPUTis how GitHub Actions communicates variables between steps.
Key points
- Only added or renamed scripts trigger validation; modified scripts are ignored.
- The code doesn’t rely on GitHub event variables (like
github.event.before), making it safe for force-pushedmain. - The regex is just a precise filter to detect the exact files we care about.
Comparison of the changes
The new Determine if scripts changed step simplifies the previous implementation in a few key ways:
-
Simplification and maintainability:
- The logic now directly computes the merge base with
origin/mainusinggit merge-base HEAD origin/main. - It eliminates the branching and looping over changed files with multiple
ifstatements. - The code is shorter, easier to read, and easier to maintain.
- The logic now directly computes the merge base with
-
Removal of GitHub environment-specific variables:
- Previously,
${{ github.event.before }}and${{ github.event.pull_request.base.sha }}were used to find the base commit. - These variables can be unreliable, especially with force pushes, as the referenced commits may no longer exist.
- The new approach relies entirely on local Git history, which is robust across force pushes and PR merges.
- Previously,
-
Focus on relevant changes:
- The simplified code detects added or renamed scripts in
scripts/anddocs/scripts/, which is exactly what the validation step cares about. - It no longer attempts to detect changes in
package.jsonscripts, which simplifies logic while still correctly triggering validation for script files.
- The simplified code detects added or renamed scripts in
Summary:
The update improves robustness, reduces complexity, and avoids relying on GitHub-specific event data that can fail in edge cases like force pushes. It also makes the CI step much easier to maintain going forward.
|
Hi, @Bhav-ikkk. Just checking in - do you still want to continue working on this pull request? There's no pressure, I'd just like an update from you since it's been open for a while and I don't want it to sit around for too long. Unfortunately, I can't offer to work on it if you are unable to, so this PR will be closed if you can't. |
|
Hi @Ryan-Millard , thanks for the follow-up and sorry for the delay. |
Thank you for the update! There's no rush - I just wanted to check up on this to see if you were still interested. Have a good day! |
Replace complex event-based logic with simpler merge-base approach: - Use git merge-base HEAD origin/main for stable comparison - Filter only added (A) or renamed (R) scripts with grep - Remove dependency on GitHub event variables - Works reliably with force pushes Addresses owner feedback in PR review.
Bhav-ikkk
force-pushed
the
fix/ci-force-push-validation
branch
from
36da0cb to
a9c14a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 38: The current git diff name-status filter uses the regex
'^[AR]\s+(scripts/|docs/scripts/).+\.js$' so modified files (status 'M') are
skipped; update that regex in the matches assignment (the git diff --name-status
command) to include 'M' (e.g., '^[ARM]\s+(scripts/|docs/scripts/).+\.js$') so
Added, Renamed and Modified script files are detected by the CI check.
- Around line 33-36: The workflow currently always runs BASE=$(git merge-base
HEAD origin/main) with no event detection or error handling; update the script
to detect GITHUB_EVENT_NAME (PR vs push) and branch: for pull_request use git
merge-base HEAD origin/main as before, but for push events wrap the git
merge-base call with error handling and if it fails or when on main fall back to
using HEAD^ (or HEAD~1) as the BASE; ensure the variable name BASE and any
downstream uses remain unchanged and log a clear message when falling back so
failures are visible.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
40-47: LGTM with dependency on line 38 fix.The conditional logic and output handling are correct. Once line 38 is fixed to include Modified files, this segment will work as intended.
|
Hi @Ryan-Millard! Sorry for the delayed response - I was a bit busy this past week but I'm back on track now. I've implemented the simplified script detection logic you suggested! The changes look great and the code is much cleaner now. What I changed: Replaced the complex event-based logic with git merge-base HEAD origin/main TEST 1: Verified git merge-base HEAD origin/main finds common ancestor correctly Key benefits confirmed: Works reliably with force pushes (no dependency on github.event.before) Img2num.ci.yml.mp4 |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is great. Thank you so much!
Please will you write some documentation for this workflow so others can understand how it works in the future. You can find the current documentation here.
To make it easier to write that documentation, you can use some of the contents of my previous review.
Have a great day!
|
Hi @Bhav-ikkk, are you still working on this PR? |
2 participants
Problem
Fixes #153
The
validate-scriptsjob was failing when commit history changed (e.g. via force push) becausegit merge-basecouldn't find a common ancestor with replaced commits.Solution
origin/main) to completely avoid merge-base issuesHEAD^comparisonTesting
The fix ensures that script validation only fails when scripts actually change, not when history is rewritten through force pushes or amended commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.