From 169b769c3254405f7b8e5f2b2545250affde171e Mon Sep 17 00:00:00 2001 From: Avocado Date: Thu, 3 Sep 2026 10:28:32 +0900 Subject: [PATCH] feat: check large pull request approvals in git node land Large pull requests follow the same approval path as semver-major changes: at least two TSC approvals. `git node land` did not check for this, so such a pull request could land with fewer. Whether a pull request is large is not something that can be derived from the diff. The policy counts a new subsystem as large regardless of size, and exempts routine dependency updates but not other dependency changes, neither of which is visible in the pull request data. The project already records the judgement with the `large-pr` label, so check that, the way `semver-major` is checked. Refs: https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md Signed-off-by: Avocado --- lib/pr_checker.js | 29 ++++++++++++++++---- test/fixtures/data.js | 1 + test/fixtures/large_pr.json | 22 ++++++++++++++++ test/unit/pr_checker.test.js | 51 ++++++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/large_pr.json diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 5e41eb8b..ef8fe461 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -26,6 +26,10 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED']; const GITHUB_ACTIONS_APP = 'github-actions'; +// Both semver-major and large pull requests take this approval path. +// https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md +const REQUIRED_TSC_APPROVALS = 2; + const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to approve\.$/; const FAST_TRACK_MIN_APPROVALS = 2; const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork'; @@ -43,6 +47,7 @@ export const PR_CHECK_REASON_CODES = Object.freeze({ MISSING_FULL_JENKINS_CI: 'missing-full-jenkins-ci', MISSING_GITHUB_CI: 'missing-github-ci', MISSING_JENKINS_CI: 'missing-jenkins-ci', + MISSING_LARGE_PR_TSC_APPROVAL: 'missing-large-pr-tsc-approval', MISSING_TSC_APPROVAL: 'missing-tsc-approval', NEW_CONTRIBUTOR: 'new-contributor', NO_COMMITS: 'no-commits', @@ -196,6 +201,10 @@ export default class PRChecker { let isFastTracked = labels.includes('fast-track'); const isSemverMajor = labels.includes('semver-major'); + // Large pull requests follow the same approval path as semver-major + // changes. Whether a pull request is large is a judgement the project + // records with a label, rather than something derived from the diff. + const isLargePR = labels.includes('large-pr'); // NOTE: a semver-major PR with fast-track should have either one of // these labels removed because that doesn't make sense if (isFastTracked) { @@ -206,16 +215,26 @@ export default class PRChecker { return false; } - if (isSemverMajor) { + if (isSemverMajor || isLargePR) { const tscApproved = approved .filter((p) => p.reviewer.isTSC()) .map((p) => p.reviewer.login); - if (tscApproved.length < 2) { - const message = 'semver-major requires at least 2 TSC approvals'; + if (tscApproved.length < REQUIRED_TSC_APPROVALS) { + // A pull request can carry both labels; report it as semver-major, + // since that is the stronger statement about the change. + const code = isSemverMajor + ? PR_CHECK_REASON_CODES.MISSING_TSC_APPROVAL + : PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL; + const subject = isSemverMajor + ? 'semver-major requires' + : 'large pull requests require'; + const message = + `${subject} at least ${REQUIRED_TSC_APPROVALS} ` + + 'TSC approvals'; cli.error(message); - this.addReason(PR_CHECK_REASON_CODES.MISSING_TSC_APPROVAL, message, { + this.addReason(code, message, { approvals: tscApproved.length, - required: 2 + required: REQUIRED_TSC_APPROVALS }); return false; // 7 day rule doesn't matter here } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index c05b5d05..5bbc17cd 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -86,6 +86,7 @@ export const firstTimerPR = readJSON('first_timer_pr.json'); export const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json'); export const semverMajorPR = readJSON('semver_major_pr.json'); +export const largePR = readJSON('large_pr.json'); export const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json'); export const fixCrossPR = readJSON('pr_with_fixes_cross.json'); export const duplicateRefPR = readJSON('pr_with_duplicate_refs.json'); diff --git a/test/fixtures/large_pr.json b/test/fixtures/large_pr.json new file mode 100644 index 00000000..3457d892 --- /dev/null +++ b/test/fixtures/large_pr.json @@ -0,0 +1,22 @@ +{ + "createdAt": "2017-10-24T11:13:43Z", + "authorAssociation": "COLLABORATOR", + "author": { + "login": "pr_author", + "email": "pr_author@example.com", + "name": "Their Github Account email" + }, + "url": "https://github.com/nodejs/node/pull/16438", + "bodyHTML": "

Awesome changes

", + "bodyText": "Awesome changes", + "labels": { + "nodes": [ + { + "name": "large-pr" + } + ] + }, + "title": "lib: awesome changes", + "baseRefName": "main", + "headRefName": "awesome-changes" +} diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a64c4855..e601cbef 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -40,6 +40,7 @@ import { firstTimerPR, firstTimerPrivatePR, semverMajorPR, + largePR, conflictingPR, closedPR, mergedPR, @@ -61,6 +62,7 @@ const { MISSING_APPROVAL, MISSING_GITHUB_CI, MISSING_JENKINS_CI, + MISSING_LARGE_PR_TSC_APPROVAL, MISSING_TSC_APPROVAL, REQUESTED_CHANGES, STALE_REVIEW, @@ -176,6 +178,55 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when large PR has only 1 TSC approval', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['large pull requests require at least 2 TSC approvals'] + ], + ok: [ + ['Approvals: 4'], + ['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], + ['- Quux User (@Quux): LGTM'], + ['- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236'], + ['- Bar User (@bar) (TSC): lgtm'] + ], + info: [ + ['This PR was created on Fri, 23 Nov 2018 17:50:44 GMT'], + ['- Quux User (@Quux) approved in via LGTM in comments'], + ['- Bar User (@bar) approved in via LGTM in comments'] + ] + }; + const pr = Object.assign({}, largePR, { + createdAt: GT_7D + }); + + const data = { + pr, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => false, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, {}, argv); + + const status = checker.checkReviewsAndWait(new Date(NOW), true); + assert(!status); + assert.deepStrictEqual(checker.reasons, [{ + code: MISSING_LARGE_PR_TSC_APPROVAL, + message: 'large pull requests require at least 2 TSC approvals', + approvals: 1, + required: 2 + }]); + cli.assertCalledWith(expectedLogs); + }); + it('should error when PR has change requests', () => { const cli = new TestCLI();