fix: restore branch protections in cleanup trap on push failure - #303
Open
rafistrauss wants to merge 2 commits into
Open
fix: restore branch protections in cleanup trap on push failure#303rafistrauss wants to merge 2 commits into
rafistrauss wants to merge 2 commits into
Conversation
When unprotect_reviews=true and the push step fails (due to permissions, race conditions, etc.), branch protections were left disabled because the protect() function was only called in the happy path after a successful push. This fix: - Introduces PUSH_PROTECTED_REVIEWS_REMOVED flag set after unprotect() succeeds and cleared after protect() succeeds. - Extends the cleanup() EXIT trap to call protect_reviews when the flag is still set, ensuring protections are always restored regardless of whether the push succeeded or failed. - Logs a warning when restoring in the error path and an explicit error (with manual remediation guidance) if restoration itself fails. Fixes CasperWA#302 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
In the existing force-pushing CI job, after the expected push failure (non-fast-forwardable without --force, with unprotect_reviews=true), add a step that queries the GitHub API to assert the pull-request review protection is still present on the 'protected' branch. This exercises the cleanup() trap fix from the previous commit: if protections were not restored by the trap, the curl check would return a response without 'required_approving_review_count' and the step fails. Also add a sourcing guard to entrypoint.sh so the file can be sourced in isolation (e.g. future shell-level tests) without executing the main body or registering the EXIT trap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a reliability bug in the action’s unprotect_reviews flow where required review protections could remain disabled if the push step failed, by ensuring protections are restored from the EXIT cleanup trap. It also adds CI verification that branch protections are restored after a simulated push failure.
Changes:
- Track whether review protections have been removed and restore them in
cleanup()when exiting early due to push failure. - Clear the “protections removed” flag on successful re-protection.
- Add a CI workflow step to verify required review protections are present after a failed push attempt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
entrypoint.sh |
Adds a flag and extends the EXIT trap cleanup to restore review protections on failure paths; adds a sourcing guard for tests. |
.github/workflows/ci_tests.yml |
Adds a CI assertion that required review protections are restored after a push failure scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+154
to
+156
| # Allow this file to be sourced (e.g. in unit tests) to load function | ||
| # definitions without registering the trap or running the main body. | ||
| [[ "${BASH_SOURCE[0]}" != "${0}" ]] && return 0 |
Comment on lines
+156
to
+162
| response=$(curl --silent \ | ||
| --header "Authorization: Bearer ${{ secrets.CI_PUSH_TO_PROTECTED_BRANCH }}" \ | ||
| --header "Accept: application/vnd.github.v3+json" \ | ||
| "https://api.github.com/repos/${GITHUB_REPOSITORY}/branches/protected/protection/required_pull_request_reviews") | ||
| echo "Protection response: ${response}" | ||
| # If required_pull_request_reviews is present, protections were restored | ||
| echo "${response}" | python3 -c "import sys, json; d = json.load(sys.stdin); sys.exit(0 if 'required_approving_review_count' in d else 1)" |
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
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.
Summary
Fixes #302
When
unprotect_reviews: trueis set and the push step fails (e.g. due to permissions issues, a branch update race condition, or a rejected push), branch protections were left disabled becauseprotect()was only ever called in the happy-path flow after a successful push.Root cause
entrypoint.shcallsunprotect→push_to_target→protectin sequence. Thecleanup()EXIT trap only removed the temporary branch. Ifpush_to_targetfailed (causing bashset -eto trigger EXIT), the trap ran but skippedprotect().Changes
PUSH_PROTECTED_REVIEWS_REMOVEDflag that is set toyesafterunprotect_reviewssucceeds and cleared afterprotect_reviewssucceeds.cleanup()EXIT trap to callprotect_reviewswhen the flag is still set, ensuring protections are always restored regardless of whether the push succeeded or failed.::warning::when restoring in the error path and a loud::error::with manual remediation guidance if restoration itself fails.Acceptance criteria (from issue)
unprotect_reviews=true, protections are restored after a successful push (unchanged behaviour; flag is cleared before exit).unprotect_reviews=true, protections are also restored after any push failure (new behaviour; cleanup trap handles it).