fix(ci): Use merge-base for correct target validation #2588
Conversation
The validation workflow was comparing changes against the wrong base, causing incorrect target detection when PR branches get out of sync. Now it uses git merge-base to find where the branch actually split off, so it only validates the targets that were actually changed in the PR. This is the same fix as sherlock-project#2588 in the upstream repo.
|
Hey @shreyasNaik0101! I tested this merge-base approach in my fork and can confirm it fixes the incorrect target detection when branches are out of sync. Great solution! The commit reference that appeared was just me testing your fix locally - I'm not submitting a competing PR. Looking forward to this landing! |
|
Thanks for verifying it! Glad to hear it worked as expected. Appreciate you testing it out! |
|
There's one issue with this method. This workflow runs automatically, even for those who haven't contributed to the repository in the past (notice So the correct approach would require corrections to the diff in the |
|
Thanks again for the detailed feedback! I've pushed up a new version that uses the secure method you recommended. The diff logic should be correct now. Let me know if there's anything else. |
| git fetch origin pull/${{ github.event.pull_request.number }}/head:pr | ||
| # The initial checkout may be shallow. To find a merge-base, | ||
| # we need more history. We can 'unshallow' the repository if needed. | ||
| git fetch --unshallow || true |
There was a problem hiding this comment.
Can't this be just a single command, rather than making two net calls?
If there's a good reason not to, all ears ofc
|
@ppfeister I've pushed an update that should address both points. Let me know what you think. |
|
Looks good so far... will revert if there are any issues, so keep this branch open temporarily ---- will provide an update here shortly after a few PRs are successfully screened. |
|
Further validated in #2611, using an old head. Should be good to delete your branch! |
| echo -e ">>> Changed targets: \n$(echo $CHANGED | tr ',' '\n')" | ||
| echo "changed_targets=$CHANGED" >> "$GITHUB_OUTPUT" | ||
| # --- The rest of the steps below are unchanged --- |
There was a problem hiding this comment.
Quick retrospective tip -
These lines are unnecessary as the diff itself shows what was changed and what wasn't --- so really all it does it add clutter to the code.
Not important, just a friendly note as you continue your development work!
Cheers! And thanks again! Your changes saved me quite a bit of time here. And it resolved what was quite an annoying issue on PRs with stale merge bases.
There was a problem hiding this comment.
Thanks for the tip and for all the help, @ppfeister! I'm glad I could contribute.
This pull request addresses an issue related to automatic target validation during chunking. Currently, the system performs a diff between the head and the base for chunking purposes. This approach causes incorrect target selection when the head is out of sync with the base.
The proposed fix changes the diff target to be the head against the latest upstream match (i.e., the default pull request diff) instead of the base. This correction ensures accurate target selection even when the head and base are not synchronized.
This behavior and the issue can be referenced in detail under issue #2568.