Repository navigation
fix(pr-validation): treat a cancelled approval as superseded - #912
Merged
Merged
Conversation
Caught on #906. Editing the pull request description fires `edited`, which queues a new run on the very same commit. The concurrency group cancelled the previous pending approval, correctly — but the superseded guard only compared head SHAs, and the head had not moved. So the dead run fell through to "decision not recorded", overwrote the live revision's comment and failed its check, while the new run sat waiting with the real request. A cancelled approval now counts as superseded on its own, and is checked first because it is the broader signal: nothing cancels it except the concurrency group replacing the run or a human doing it by hand, and neither is a decision worth reporting. The head comparison stays for whatever slips past that. Test pins both the new branch and its order ahead of the comparison.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe approval outcome step now handles cancellation before comparing the pull request head. It marks the run as superseded and exits without further pull-request actions. The test and documentation cover cancellations when the head remains unchanged. ChangesApproval cancellation handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary fix: treating cancelled approvals as superseded. Description check The description explains the problem, affected behavior, implementation, testing, and lack of breaking changes. The Related Issues section is empty, but the required issue reference is not critical be… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, affected behavior, implementation, testing, and lack of breaking changes. The Related Issues section is empty, but the required issue reference is not critical because the description identifies the reproduced case and objective.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
lerian-studio
added
size/S
Contributor
🔍 Lint Analysis
|
Contributor
🛡️ CodeQL Analysis ResultsLanguages analyzed: Found 1 issue(s): 1 Medium
🔍 View full scan logs | 🛡️ Security tab |
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
This comment has been minimized.
This comment has been minimized.
lerian-studio
added
the
review-ready
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses the CodeRabbit review on #912. The cancellation test checked the branch and its ordering but not its `exit 0`. Without that exit a cancelled run falls through, and a recorded rejection downstream wins: the verdict comes back `rejected` and the pull request is closed — because a concurrency cancellation raced a reviewer. Closing a pull request by accident is the most destructive thing this workflow does, so "trivial" undersells it. Asserting the characters `exit 0` are present would prove nothing about whether the branch stops, and would be the fifth text assertion in this series. `run_verdict()` executes the step with `gh` stubbed instead, and the test hands it a rejection to fall through onto. Removing the exit makes it fail with 'superseded' != 'rejected' — which is how the report was confirmed in the first place. Also covers the whole state table by running it: approved, rejected, superseded from a cancellation, superseded from a moved head, unresolved, none, blocked. Those cases existed only in a scratch harness before.
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
6 of 14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Found by exercising #906, not by reading the code.
Editing a pull request description fires
edited, which queues a new run on the same commit. The concurrency group from #908 cancelled the previous pending approval — correct — but the superseded guard only compared head SHAs, and the head had not moved.So the dead run fell through to
unresolved, overwrote the live comment with "💥 Breaking change — decision not recorded" and failed its check, while the new run was sitting right there holding the real request. The comment told the reader the opposite of what was true.3883332934638833721558ab25957, waiting with the real requestA cancelled approval is now superseded on its own, checked before the head comparison because it is the broader signal: nothing cancels it except the concurrency group replacing the run or a human doing it by hand, and neither is a decision worth reporting. The head comparison stays for whatever slips past that.
Type of Change
feat: New workflow or new input/output/step in an existing workflowfix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)perf: Performance improvement (e.g. caching, parallelism, reduced steps)refactor: Internal restructuring with no behavior changedocs: Documentation only (README, docs/, inline comments)ci: Changes to self-CI (workflows under.github/workflows/that run on this repo)chore: Dependency bumps, config updates, maintenancetest: Adding or updating testsBREAKING CHANGE: Callers must update their configuration after this PRBreaking Changes
None. One more condition in an existing step; no input, output or job name changes.
Testing
@this-branchor the beta tagThe failing case is reproduced on #906 and visible there.
Verdict logic exercised against a stubbed
ghacross seven paths: cancelled with the same head (the case that broke), cancelled with a different head, different head with the approval fine, approved, rejected, failed-without-a-recorded-rejection, and nothing-to-approve. Only the first three resolve tosuperseded; the rest are unchanged.Test pins the new branch and its position ahead of the head comparison, verified by mutation — removing the guard fails it.
Caller repo / workflow run: https://github.com/LerianStudio/github-actions-shared-workflows/actions/runs/38833329346
Related Issues