fix: bind merge approvals to current head#3844
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the post-approval changes action to recognize reviews from trusted project members (MEMBER) and ensures that the minimum reviews validation requires at least one approval on the latest commit. Unit tests have been added to verify this behavior. The feedback points out a potential runtime crash in the validation logic if the commit object is null, suggesting the use of optional chaining (commit?.oid) to safely access the commit ID.
| ({authorAssociation, commit}) => | ||
| authorAssociation === 'MEMBER' && commit.oid === pullRequest.headRefOid, |
There was a problem hiding this comment.
In GitHub's GraphQL API, the commit field on a PullRequestReview is nullable (for example, if the commit has been garbage collected or is no longer available after a force-push). Destructuring commit and directly accessing commit.oid without a null/undefined check can lead to a runtime TypeError: Cannot read properties of null (reading 'oid') and crash the validation process.
Using optional chaining (commit?.oid) safely handles cases where commit might be null.
| ({authorAssociation, commit}) => | |
| authorAssociation === 'MEMBER' && commit.oid === pullRequest.headRefOid, | |
| ({authorAssociation, commit}) => | |
| authorAssociation === 'MEMBER' && commit?.oid === pullRequest.headRefOid, |
Summary
MEMBERreviews in the post-approval freshness checkWhy
The post-approval action and caretaker merge validator used different reviewer trust predicates. A repository member review could satisfy the merge validator while being ignored by the freshness action. The merge validator also counted an approval without checking whether it applied to the current head commit.
This aligns the trust sets and binds the required approval to
headRefOid, so a pull request head update requires a fresh member approval before caretaker can merge it.Validation
bazel test --lockfile_mode=update //ng-dev/pr/common/test:testbazel test --lockfile_mode=update //github-actions/post-approval-changes:main_test //github-actions/post-approval-changes:main_prettierignore_test //github-actions/post-approval-changes:lib_strict_deps_testpnpm exec prettier --check github-actions/post-approval-changes/lib/main.ts ng-dev/pr/common/test/common.spec.ts ng-dev/pr/common/validation/assert-minimum-reviews.tsgit diff --check