Repository navigation
update action with MATCH state - #17
Conversation
PR SummaryMedium Risk Overview Moves SHA persistence (write + Written by Cursor Bugbot for commit cec52a9. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: MATCH=1 set too late in else branch
- Moved MATCH=1 assignment to the beginning of the else branch before debug output to ensure it's always persisted even if subsequent echo commands fail.
Or push these changes by commenting:
@cursor push 7cc73d5477
Preview (7cc73d5477)
diff --git a/action.yml b/action.yml
--- a/action.yml
+++ b/action.yml
@@ -49,28 +49,14 @@
echo "HEAD_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV
echo "BASE_SHA=${{ github.event.pull_request.base.sha }}" >> $GITHUB_ENV
- - name: Write SHAs to file
- shell: bash
- run: |
- echo "${{ env.HEAD_SHA }}" > shas.txt
- echo "${{ env.BASE_SHA }}" >> shas.txt
-
- - name: Upload SHAs
- uses: actions/upload-artifact@v4
- continue-on-error: true
- id: artifact-upload-step
- with:
- name: dismiss-stale-approvals-shas
- path: shas.txt
-
- name: Check if diff has changed
continue-on-error: true
id: check
shell: bash
run: |
- if [ -z ${{ env.PREV_HEAD_SHA }} ] || [ -z ${{ env.PREV_BASE_SHA }} ]; then
+ if [ -z "${{ env.PREV_HEAD_SHA }}" ] || [ -z "${{ env.PREV_BASE_SHA }}" ]; then
echo ::notice:: "No previous SHAs found; behaving as if diff has changed"
- echo MATCH="0" >> $GITHUB_ENV
+ echo "MATCH=0" >> $GITHUB_ENV
exit 0
fi
@@ -84,34 +70,49 @@
echo "Merge bases: $PREV_MERGE_BASE $MERGE_BASE"
RANGE_DIFF=$(git range-diff "$PREV_MERGE_BASE".."${{ env.PREV_HEAD_SHA }}" "$MERGE_BASE".."${{ env.HEAD_SHA }}")
- MATCH=$(echo "$RANGE_DIFF" | awk '{print $3}' | grep -vq '^=$'; echo $?)
- echo MATCH="$MATCH" >> $GITHUB_ENV
- if [ "$MATCH" == "0" ]; then
- # Run git range-diff again with colors to make it easier to read
+ HAS_CHANGES=$(echo "$RANGE_DIFF" | awk '{print $3}' | grep -vq '^=$'; echo $?)
+
+ if [ "$HAS_CHANGES" == "0" ]; then
+ echo "MATCH=0" >> $GITHUB_ENV
echo ::notice:: "PR was modified:
$(git range-diff --color=always "$PREV_MERGE_BASE".."${{ env.PREV_HEAD_SHA }}" "$MERGE_BASE".."${{ env.HEAD_SHA }}")"
else
+ echo "MATCH=1" >> $GITHUB_ENV
echo "PR was not modified, range diff for debugging:"
echo "$RANGE_DIFF"
fi
+ - name: Write SHAs to file
+ if: env.MATCH != ''
+ shell: bash
+ run: |
+ echo "${{ env.HEAD_SHA }}" > shas.txt
+ echo "${{ env.BASE_SHA }}" >> shas.txt
+
+ - name: Upload SHAs
+ if: env.MATCH != ''
+ uses: actions/upload-artifact@v4
+ continue-on-error: true
+ id: artifact-upload-step
+ with:
+ name: dismiss-stale-approvals-shas
+ path: shas.txt
+
- name: Construct dismissal reason
+ if: env.MATCH != '1'
shell: bash
run: |
- if [ "${{ env.NO_PREV_SHAS }}" == "1" ]; then
- DETAILS="Could not find data on the previous version of this PR; see action logs at "
- elif [ "${{ env.MATCH }}" == "0" ]; then
+ if [ "${{ env.MATCH }}" == "0" ]; then
DETAILS='See the output of `git range-diff` at '
else
DETAILS="Failed to check if diff has changed; see action logs at "
fi
echo "REASON=Your organization requires reapproval when changes are made, so Graphite has dismissed approvals. ${DETAILS}${{ env.WORKFLOW_RUN_URL }}" >> $GITHUB_ENV
- - name: Dismiss approvals if diff has changed or if any prior step failed
- if: env.MATCH == '0' || env.NO_PREV_SHAS == '1'
+ - name: Dismiss approvals unless diff is proven unchanged
+ if: env.MATCH != '1'
uses: withgraphite/dismiss-all-approvals-js@main
with:
github-token: ${{ inputs.github-token }}
dry-run: ${{ inputs.dry-run }}
reason: ${{ env.REASON }}
-You can send follow-ups to this agent here.
5b6e965 to
75bc379
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Misleading message when previous SHAs missing
- Added explicit elif branch to distinguish MATCH=-1 (no previous SHAs) from actual failures, providing accurate 'No previous SHAs found' message instead of misleading 'Failed to check' message.
Or push these changes by commenting:
@cursor push fa12c3b6e3
Preview (fa12c3b6e3)
diff --git a/action.yml b/action.yml
--- a/action.yml
+++ b/action.yml
@@ -104,6 +104,8 @@
run: |
if [ "${{ env.MATCH }}" == "0" ]; then
DETAILS='See the output of `git range-diff` at '
+ elif [ "${{ env.MATCH }}" == "-1" ]; then
+ DETAILS="No previous SHAs found; see action logs at "
else
DETAILS="Failed to check if diff has changed; see action logs at "
fiYou can send follow-ups to this agent here.
75bc379 to
13d49d6
Compare
nathantypanski
left a comment
nathantypanski
left a comment
There was a problem hiding this comment.
The changes I've proposed make the gating more explicit and correct: gating on the step check outcome (Bash exit status) and variable initialization.
| echo "${{ env.BASE_SHA }}" >> shas.txt | ||
|
|
||
| - name: Upload SHAs | ||
| if: env.MATCH != '' |
There was a problem hiding this comment.
Better to gate SHA upload on steps.check.outcome == 'success' rather than env.MATCH != ''.
If the check script writes MATCH to $GITHUB_ENV and then fails on a subsequent line (e.g. the git range-diff --color=always re-run in the logging branch), env.MATCH is set but the step outcome is failure. With the current guard, SHAs get uploaded despite the failed validation pass.
steps.check.outcome reflects the actual exit code (outcome is the exit status of the prior bash script). This is the principled guard.
| if: env.MATCH != '' | |
| if: steps.check.outcome == 'success' |
| shell: bash | ||
| run: | | ||
| if [ -z ${{ env.PREV_HEAD_SHA }} ] || [ -z ${{ env.PREV_BASE_SHA }} ]; then | ||
| if [ -z "${{ env.PREV_HEAD_SHA }}" ] || [ -z "${{ env.PREV_BASE_SHA }}" ]; then |
There was a problem hiding this comment.
| if [ -z "${{ env.PREV_HEAD_SHA }}" ] || [ -z "${{ env.PREV_BASE_SHA }}" ]; then | |
| echo "MATCH=0" >> $GITHUB_ENV | |
| if [ -z "${{ env.PREV_HEAD_SHA }}" ] || [ -z "${{ env.PREV_BASE_SHA }}" ]; then |
You may want to explicitly set MATCH=0 here, since explicit assignment helps survive future changes and avoids crufty $VAR != '' checks.
Note: with this change the upload guard must be
steps.check.outcome == 'success'(above) rather thanenv.MATCH != '', sinceMATCHwill always be non-empty.
patwolfe-gt
left a comment
patwolfe-gt
left a comment
There was a problem hiding this comment.
Let's create a ticket to respond to some of these comments and go ahead and merge!
13d49d6 to
ab6001d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Default MATCH masks check-step failures
- Removed the initial MATCH=0 assignment so early failures are properly distinguished from diff changes and route to the correct dismissal reason branch.
- ✅ Fixed: No separator before URL in -1 reason
- Added trailing separator text 'See ' to the MATCH=-1 branch to properly format the URL in the dismissal reason.
Or push these changes by commenting:
@cursor push 3c4d0f92d6
Preview (3c4d0f92d6)
diff --git a/action.yml b/action.yml
--- a/action.yml
+++ b/action.yml
@@ -54,8 +54,6 @@
id: check
shell: bash
run: |
- echo "MATCH=0" >> $GITHUB_ENV
-
if [ -z "${{ env.PREV_HEAD_SHA }}" ] || [ -z "${{ env.PREV_BASE_SHA }}" ]; then
echo ::notice:: "No previous SHAs found; behaving as if diff has changed"
echo "MATCH=-1" >> $GITHUB_ENV
@@ -107,7 +105,7 @@
if [ "${{ env.MATCH }}" == "0" ]; then
DETAILS='See the output of `git range-diff` at '
elif [ "${{ env.MATCH }}" == "-1" ]; then
- DETAILS="No previous SHAs found; treated as if diff has changed"
+ DETAILS="No previous SHAs found; treated as if diff has changed. See "
else
DETAILS="Failed to check if diff has changed; see action logs at "
fiYou can send follow-ups to this agent here.
| shell: bash | ||
| run: | | ||
| if [ -z ${{ env.PREV_HEAD_SHA }} ] || [ -z ${{ env.PREV_BASE_SHA }} ]; then | ||
| echo "MATCH=0" >> $GITHUB_ENV |
There was a problem hiding this comment.
Default MATCH masks check-step failures
Medium Severity
Setting MATCH=0 by default makes check-step failures indistinguishable from real diff changes, so dismissal reasons incorrectly reference git range-diff even when no diff was produced.
ab6001d to
cec52a9
Compare
Merge activity
|
3 participants
TL;DR
Improved the logic flow by moving SHA artifact upload after diff checking and adding proper conditionals to prevent unnecessary operations.
MATCH = 1: Diff unchanged
MATCH = 0: Diff changed, dismiss
MATCH = -1: We don't have previous upload, upload current and dismiss
MATCH = unset: some form of failure, dismiss
What changed?
if: env.MATCH != ''andif: env.MATCH != '1') to prevent these steps from running when unnecessary${{ env.PREV_HEAD_SHA }}and${{ env.PREV_BASE_SHA }}MATCHvariable logic to useHAS_CHANGESfor clarity and explicitly setMATCHenvironment variable in both branchesNO_PREV_SHAScondition checkenv.MATCH != '1'How to test?
Test with pull requests in different scenarios:
Why make this change?
This change optimizes the workflow by avoiding unnecessary file operations and artifact uploads when the diff check fails or when we already know the diff hasn't changed. It also simplifies the conditional logic by consolidating the decision-making into a single
MATCHvariable that clearly indicates whether approvals should be dismissed.