Skip to content

Avoid dismissing approvals on restacks alone#58

Open
lunelson wants to merge 1 commit into
hashintel:mainfrom
lunelson:fix/graphite-restack-stale-approvals
Open

Avoid dismissing approvals on restacks alone#58
lunelson wants to merge 1 commit into
hashintel:mainfrom
lunelson:fix/graphite-restack-stale-approvals

Conversation

@lunelson
Copy link
Copy Markdown

Summary

  • Avoid dismissing approvals solely because restacking/rebasing rewrote commit IDs
  • Defer rewritten-history cases to the existing range-diff stale-check logic
  • Add self-test coverage for safe restacks and changed patch series

Tests

  • npm ci
  • npx renovate-config-validator
  • .github/actions/dismiss-stale-approvals/self-test.sh

@cursor
Copy link
Copy Markdown

cursor Bot commented May 12, 2026

PR Summary

Medium Risk
Changes the stale-approval decision logic in the GitHub Action, which can affect whether PR approvals are dismissed; mistakes could leave stale approvals in place or dismiss valid ones. Scope is small and covered by expanded self-tests.

Overview
Stops treating “approval commit is not an ancestor of HEAD” as automatically stale in check-manual-merge-resolutions.sh, so simple rebases/restacks no longer dismiss approvals and the decision is deferred to the existing git range-diff check.

Updates self-test.sh to reflect the new behavior and adds coverage to ensure rewritten history only becomes stale when range-diff detects a changed patch series (and that the final decision output points reviewers to the range-diff output).

Reviewed by Cursor Bugbot for commit befef1c. Bugbot is set up for automated code reviews on this repo. Configure here.

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented May 12, 2026

🤖 Augment PR Summary

Summary: This PR refines the “dismiss stale approvals” composite action to avoid invalidating reviews due to restacks/rebases that rewrite commit SHAs.

Changes:

  • Updates check-manual-merge-resolutions.sh so “approval commit is not an ancestor of HEAD” no longer marks approvals stale by itself.
  • Defers rewritten-history situations to the existing git range-diff-based staleness check.
  • Expands self-test.sh coverage to validate that safe rewrites stay non-stale, while changed patch series are still detected as stale via range-diff.

Technical Notes: The action now treats “non-ancestor” as informational (common after rebases) and relies on range-diff output to decide whether the reviewed patch series materially changed.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@lunelson
Copy link
Copy Markdown
Author

augment review

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

printf 'reason=Latest approval commit %s is not an ancestor of %s, indicating rewritten history after approval\n' "$approval_sha" "$head_sha"
# Rebases and restacks rewrite commit IDs. Non-ancestor alone does not prove
# that the reviewed patch series changed, so let range-diff decide that.
printf 'stale=false\nreason=\n'
Copy link
Copy Markdown

@augmentcode augmentcode Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git merge-base --is-ancestor can exit with an error status (e.g., missing history in a shallow fetch), and this block currently treats any nonzero exit as a harmless “non-ancestor” case. Consider distinguishing a real command failure from a true non-ancestor result so the action doesn’t silently skip the manual-merge-resolution check when it can’t be evaluated.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants