fix(deploy): gate branch-deploy approval check on exit code (EXSC-687) - #2128
fix(deploy): gate branch-deploy approval check on exit code (EXSC-687)#21280xDEnYO wants to merge 2 commits into
Conversation
verify-approvals.ts printed the "OK" marker unconditionally, so deployUpgradesToSAFE.sh - which compared that marker - continued even after the check had logged failures. The script now exits non-zero on any failure and prints the marker only on the fully successful path, and the shell caller gates on the exit code instead. Also paginates the GitHub list calls (listFiles capped at 30 entries, producing false "facet is not included in this PR" results), turns the swallowed helper errors into throws so a failed lookup can no longer read as an empty result, and requires GH_TOKEN up front (now documented in .env.example). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR refactors GitHub approval verification into exported helpers, adds paginated lookup and validation tests, documents ChangesGitHub approval verification
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
script/deploy/github/verify-approvals.ts (1)
99-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFilter open pull requests with the
headparameterGitHub’s list-pulls endpoint accepts
headinowner:branchform, so this can request only PRs on the target branch instead of paginating every open pull request and filtering client-side. This reduces deploy-list network traffic as the repo accumulates more open PRs.♻️ Proposed fix
const getOpenPRsForBranch = async (octokit: Octokit, branch: string) => { const pullRequests = await octokit.paginate(octokit.rest.pulls.list, { owner: OWNER, repo: REPO, state: 'open', + head: `${OWNER}:${branch}`, per_page: PER_PAGE, }) - return pullRequests.filter((pullRequest) => pullRequest.head.ref === branch) + return pullRequests }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/github/verify-approvals.ts` around lines 99 - 108, Update getOpenPRsForBranch to pass the GitHub pulls.list head parameter as `${OWNER}:${branch}` while retaining the open state and pagination options. Remove the client-side pullRequests.filter by head.ref so the API performs the branch filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/deployUpgradesToSAFE.sh`:
- Line 37: Quote the GIT_BRANCH variable in the condition within
deployUpgradesToSAFE.sh, changing the comparison to use the repository-standard
quoted Bash variable form while preserving the existing main-branch check.
- Around line 49-58: Update the script-selection iteration around SCRIPTS to
preserve each newline-delimited facet name exactly, including whitespace and
glob characters: read the selections into an array or iterate with read -r, and
use an uppercase iteration variable. Use that preserved variable when
constructing UPDATE_SCRIPT and related messages.
In `@script/deploy/github/verify-approvals.ts`:
- Around line 140-155: Update getPRApprovers to deduplicate reviews by
user.login, retaining each user’s last submission in the oldest-first reviews
list before filtering for APPROVED and returning logins. Ensure a later
CHANGES_REQUESTED removes that user from approvers, and add coverage for an
approve-then-request-changes sequence.
---
Nitpick comments:
In `@script/deploy/github/verify-approvals.ts`:
- Around line 99-108: Update getOpenPRsForBranch to pass the GitHub pulls.list
head parameter as `${OWNER}:${branch}` while retaining the open state and
pagination options. Remove the client-side pullRequests.filter by head.ref so
the API performs the branch filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c3a62e4-57f3-40f3-b870-d5668ddbad30
📒 Files selected for processing (4)
.env.examplescript/deploy/deployUpgradesToSAFE.shscript/deploy/github/verify-approvals.test.tsscript/deploy/github/verify-approvals.ts
…n facet loop (EXSC-687) - getPRApprovers now dedupes reviews to each user's latest state-changing submission, so a later CHANGES_REQUESTED or a dismissal supersedes an earlier approval (COMMENTED leaves it standing); covered by three new tests - quote GIT_BRANCH in the main-branch condition - iterate selected facet names with a fd-3 read loop instead of unquoted word-splitting, so names are never glob-expanded and inner commands keep their own stdin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Which Linear task belongs to this PR?
https://linear.app/lifi-linear/issue/EXSC-687
Why did I implement it this way?
Exit-code contract instead of a stdout sentinel.
verify-approvals.tswroteOKto stdout unconditionally at the end of itsrun()— the individual checks only logged to stderr viaconsole.errorand then fell through to the same final write. Its only consumer,deployUpgradesToSAFE.sh, captured that stdout intoVERIFIEDand compared it to"OK", so a run that had just reported "Missing required approvals" still matched and continued into the proposal loop. On top of that, the shellifhad noelsebranch, so the non-matching case returned success and the deploy step ended without any signal at all. Exit codes are the contract the rest of this repo's shell/TS boundary relies on, so the script now exits non-zero on any failure and writesOKonly when the failure list is empty, and the shell gates onif ! bun … ; then error …; return 1; fi. The success path is unchanged: an approved PR still printsOK, still logs "PR has been approved. Continuing...", and still runs the same loop (that block moved out of the oldif, which is why its indentation shows up in the diff).Pagination.
getFilesInPRcalledoctokit.rest.pulls.listFileswithout pagination, which returns at most 30 entries. Any PR touching more than 30 files could omit the verysrc/Facets/<Facet>.solentry being checked and report " is not included in this PR" for a facet that is in fact there. All four list calls (pulls.list,pulls.listFiles,pulls.listReviews,teams.listMembersInOrg) now go throughoctokit.paginatewithper_page: 100, so the reviews and team-membership lookups can't be truncated either — the hand-rolled paging loop ingetOpenPRsForBranchwas replaced by the same mechanism rather than left as a second way of doing the same thing.Explicit token precondition.
GH_TOKENwas read bydeployUpgradesToSAFE.shand passed through as--token, but it existed nowhere else in the repo and was missing from.env.example, so a contributor who never set it got--token "". Because this repo is public the unauthenticated PR and review lookups still succeed and only the team-membership call fails, which is a degraded state that is hard to recognise from the output. The token is now resolved (flag first,GH_TOKENfrom the environment as fallback) before the Octokit client is constructed, and a missing or blank value aborts immediately with a message naming the variable and the scopes it needs..env.exampledocuments it with an empty placeholder.Fail-closed helpers.
getPRApproversandgetFilesInPRcaught their errors, logged them and returnedundefined; every downstream check used?.and read the resulting empty list as "nothing to report", so a rate-limited or unauthorised call produced the same result as a clean PR. Both now let the error propagate, andgetTeamMembersrethrows with the scope hint instead of collapsing into a generic message. The orchestration is split into a purecollectApprovalFailurespredicate plusverifyApprovalsfor the I/O, andrunMainsits behindif (import.meta.main)(matching the other citty scripts here) so the newverify-approvals.test.tscan exercise the policy, the pagination, the missing-token path and the exit-code contract without touching the network. The check's semantics are otherwise unchanged — same conditions, same messages.Governance impact (rule 105): this changes only the pre-proposal approval check for branch-based deploys; it does not alter Safe threshold, timelock delay, roles, or how any transaction is authorized. The check becomes strictly more likely to block, never less.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)