fix(ci): use pull_request.head.sha in validate-lockfile git diff#7716
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
In a pull_request_target workflow, github.sha resolves to the base branch HEAD (develop), not the PR head commit. This causes a 'fatal: bad object' error on fork PRs because the upstream develop HEAD does not exist in the fork's cloned history. The script exits immediately (set -e), the issues array is never populated, and every fork PR touching pnpm-lock.yaml gets a blank 'Lockfile Validation Failed' comment as a false positive. Replace github.sha with github.event.pull_request.head.sha so the diff correctly compares the PR base against the PR head. The actions/checkout step above already uses pull_request.head.sha, so this change also improves consistency.
57f721f to
17abbd0
Compare
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7716 +/- ##
==========================================
- Coverage 3.30% 3.30% -0.01%
==========================================
Files 560 561 +1
Lines 58344 58355 +11
Branches 873 873
==========================================
Hits 1928 1928
- Misses 56416 56427 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
@knsv - please can you review and hopefully approve? This is intended to fix the SHA hard-coded issue affecting the |
knsv
left a comment
There was a problem hiding this comment.
Nice catch and clean fix. Diagnosis is correct: under pull_request_target, ${{ github.sha }} resolves to the base branch HEAD, not the PR head, so the git diff resolved a commit that wasn't in the fork's cloned history and tripped set -e before issues was populated — producing the empty "Lockfile Validation Failed" comment.
Replacing it with ${{ github.event.pull_request.head.sha }} matches what the actions/checkout step on the line above already uses, so the workflow is now internally consistent and the diff actually resolves. CI is green. LGTM.
aloisklink
left a comment
There was a problem hiding this comment.
LGTM to me too.
It looks like this issue has been around since #7116
As a warning to other reviewers (e.g. @knsv), this validate-lockfile.yml uses pull_request_target, so changes to this are extremely dangerous and should be reviewed carefully.
But using github.event.pull_request.head.sha instead of github.sha is safe.
|
@sjackson0109, Thank you for the contribution! |
|
Thanks for the approval and merge. |
Problem
In a
pull_request_targetworkflow,${{ github.sha }}resolves to the base branch HEAD (i.e.develop), not the PR head commit. The checkout step correctly uses${{ github.event.pull_request.head.sha }}to check out the fork's code, but the subsequentgit diffcommand still uses${{ github.sha }}:When a PR comes from a fork,
${{ github.sha }}refers to a commit that does not exist in the fork's cloned history. This causes:Because the script runs under
set -e, it exits immediately — before theissuesarray is ever populated. The workflow then posts a "Lockfile Validation Failed" comment with a blank body, causing confusion and a false-positive failure on every fork PR that touchespnpm-lock.yamlorpackage.json.Fix
Replace
${{ github.sha }}→${{ github.event.pull_request.head.sha }}so the diff compares the correct two commits: the PR base vs the PR head.Verification
This was originally diagnosed while investigating CI failures on PR #7700 (a fork PR touching
package.json). Checked manually: all three lockfile validations pass; only thegit diffstep fails with the bad object error.The
actions/checkoutstep on the line immediately above already usesgithub.event.pull_request.head.sha— consistency is also improved by this change.