Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions github-actions/post-approval-changes/lib/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,11 @@ async function runPostApprovalChangesAction(
if (knownReviewers.has(user)) {
continue;
}
// Only consider reviews by Googlers for this check.
if (!(await isGooglerOrgMember(membershipCheckClient, user))) {
// Only consider reviews by trusted project members for this check.
if (
review.author_association !== 'MEMBER' &&
!(await isGooglerOrgMember(membershipCheckClient, user))
) {
continue;
}
knownReviewers.add(user);
Expand Down
4 changes: 2 additions & 2 deletions github-actions/post-approval-changes/main.js

Large diffs are not rendered by default.

39 changes: 39 additions & 0 deletions ng-dev/pr/common/test/common.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,45 @@ describe('pull request validation', () => {
});
});

describe('assert-minimum-reviews', () => {
it('should pass when a team member approved the latest commit', async () => {
const config = createIsolatedValidationConfig({assertMinimumReviews: true});
const pr = createTestPullRequest();
pr.reviews.nodes.push({
authorAssociation: 'MEMBER' as CommentAuthorAssociation,
author: {
login: 'fakelogin',
},
bodyText: '',
commit: {
oid: pr.headRefOid,
},
});

const results = await assertValidPullRequest(pr, config, ngDevConfig, null, prTarget, git);
expect(results.length).toBe(0);
});

it('should reject when the only team member approval is for an older commit', async () => {
const config = createIsolatedValidationConfig({assertMinimumReviews: true});
const pr = createTestPullRequest();
pr.reviews.nodes.push({
authorAssociation: 'MEMBER' as CommentAuthorAssociation,
author: {
login: 'fakelogin',
},
bodyText: '',
commit: {
oid: '1234',
},
});

const results = await assertValidPullRequest(pr, config, ngDevConfig, null, prTarget, git);
expect(results.length).toBe(1);
expect(results[0].message).toContain('for the latest commit');
});
});

describe('assert-isolate-primitives', () => {
it('should pass when no google sync config present', async () => {
const config = createIsolatedValidationConfig({assertIsolatedSeparateFiles: true});
Expand Down
5 changes: 3 additions & 2 deletions ng-dev/pr/common/validation/assert-minimum-reviews.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ export const minimumReviewsValidation = createPullRequestValidation(
class Validation extends PullRequestValidation {
assert(pullRequest: PullRequestFromGithub) {
const totalCount = pullRequest.reviews.nodes.filter(
({authorAssociation}) => authorAssociation === 'MEMBER',
({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,

).length;
if (totalCount === 0) {
throw this._createError(
`Pull request cannot be merged without at least one review from a team member`,
`Pull request cannot be merged without at least one review from a team member for the latest commit`,
);
}
}
Expand Down