Refine GitHub Actions workflow to improve reference commit logic for pull requests. Updated condition to handle merged pull requests more effectively.#3251
Conversation
…pull requests. Updated condition to handle merged pull requests more effectively.
There was a problem hiding this comment.
PR Summary:
- Updates the
refused byactions/checkoutin the CI workflow to better handlepull_requestevents. - For open/reopened PRs: now uses
head.sha(the PR branch tip) instead ofmerge_commit_sha(which can benullwhen there are merge conflicts). - For closed+merged PRs: continues to use
merge_commit_shafor the actual merge commit.
Review Summary:
The fix addresses a real bug — the old expression used merge_commit_sha for all pull_request events (including opened/reopened), which is a GitHub-generated test merge commit that can be null when conflicts exist, causing checkout failures. The new logic correctly branches on merged status.
The main concern is that the deeply nested &&/|| ternary expression carries a silent fallback risk inherent to GitHub Actions' expression evaluation: if merge_commit_sha were ever falsy on a merged PR, the expression silently falls back to head.sha, potentially causing a release build to package the wrong content. A step-based ref resolution approach is safer and considerably more readable.
Suggestions
- Replace the nested && || ternary on the
ref:field with a dedicatedResolve checkout refbash step for safer, more readable ref selection. Apply - Add a
Report Contextecho forgithub.event.pull_request.merge_commit_shaandgithub.event.pull_request.head.shato help debug future checkout issues. Apply
| fetch-depth: 0 | ||
| submodules: recursive | ||
| ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.merge_commit_sha || github.sha }} | ||
| ref: ${{ github.event_name == 'pull_request' && (github.event.pull_request.merged && github.event.pull_request.merge_commit_sha || github.event.pull_request.head.sha) || github.sha }} |
There was a problem hiding this comment.
Potential silent fallback when merge_commit_sha is null during open PRs with conflicts
For opened/reopened events, github.event.pull_request.merged is false, so the inner expression correctly falls through to head.sha. That part is a genuine improvement over the old code. 👍
However, the overall expression relies on GitHub Actions' JavaScript-like &&/|| short-circuit to emulate a ternary — which means if merge_commit_sha is ever falsy (e.g. null in a conflicting-but-somehow-merged state) the fallback to head.sha would silently trigger on a closed+merged event, checking out the PR branch tip instead of the actual merge commit. This could cause a release build to package the wrong content.
A safer and far more readable approach using an explicit step output (insert before the Checkout step):
- name: Resolve checkout ref
id: resolve_ref
shell: bash
run: |
if [ "${{ github.event_name }}" == "pull_request" ]; then
if [ "${{ github.event.pull_request.merged }}" == "true" ]; then
echo "ref=${{ github.event.pull_request.merge_commit_sha }}" >> $GITHUB_OUTPUT
else
echo "ref=${{ github.event.pull_request.head.sha }}" >> $GITHUB_OUTPUT
fi
else
echo "ref=${{ github.sha }}" >> $GITHUB_OUTPUT
fi
- name: Checkout Repository
uses: actions/checkout@v6
with:
fetch-depth: 0
submodules: recursive
ref: ${{ steps.resolve_ref.outputs.ref }}This makes the intent explicit, avoids the &&/|| ternary trap, and is much easier to maintain.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
📦 New public release are available for 6.3.0.26091+2238 |
No description provided.