Skip to content

fix: bind merge approvals to current head#3844

Open
s2ongmo wants to merge 2 commits into
angular:mainfrom
s2ongmo:agent/bind-approvals-to-current-head
Open

fix: bind merge approvals to current head#3844
s2ongmo wants to merge 2 commits into
angular:mainfrom
s2ongmo:agent/bind-approvals-to-current-head

Conversation

@s2ongmo

@s2ongmo s2ongmo commented Jul 14, 2026

Copy link
Copy Markdown

Summary

  • include repository MEMBER reviews in the post-approval freshness check
  • require the accepted member approval to match the current pull request head
  • add current-head and stale-head regression coverage
  • regenerate the checked-in action bundle

Why

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:test
  • bazel 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_test
  • pnpm 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.ts
  • git diff --check

@s2ongmo
s2ongmo marked this pull request as ready for review July 14, 2026 21:43

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +21 to +22
({authorAssociation, commit}) =>
authorAssociation === 'MEMBER' && commit.oid === pullRequest.headRefOid,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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.

Suggested change
({authorAssociation, commit}) =>
authorAssociation === 'MEMBER' && commit.oid === pullRequest.headRefOid,
({authorAssociation, commit}) =>
authorAssociation === 'MEMBER' && commit?.oid === pullRequest.headRefOid,

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.

1 participant