From f8afeef974c642e0d4a83d47f1ca6e9ab79cce42 Mon Sep 17 00:00:00 2001 From: Avocado Date: Mon, 31 Aug 2026 13:18:51 +0900 Subject: [PATCH] feat: check large pull request approvals in git node land Node.js requires two TSC approvals for pull requests over 5000 lines of net change, but nothing enforces it at land time. Report a pull request that exceeds the threshold and does not carry two TSC approvals. semver-major pull requests are already held to the same requirement, so they are left to the existing check. The policy also exempts routine dependency updates, WPT imports, bot-issued pull requests and test-only refactors. Whether a change is routine or functional is not something this can decide from the pull request data, so it only skips the categories a label identifies on its own, meaning `dependencies` and `release`. A release proposal is the case that matters most here, since those run to hundreds of thousands of lines and blocking one would stall a release. Missing line counts leave the check inactive rather than treating the pull request as small, so a query that predates the new fields does not silently pass a large pull request through. Refs: https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md Refs: https://github.com/nodejs/node-core-utils/issues/1063 Signed-off-by: Avocado --- lib/pr_checker.js | 66 +++++++++++++++ lib/queries/PR.gql | 2 + test/fixtures/data.js | 2 + test/fixtures/large_pr.json | 20 +++++ test/fixtures/large_pr_exempt.json | 24 ++++++ test/unit/pr_checker.test.js | 129 +++++++++++++++++++++++++++++ 6 files changed, 243 insertions(+) create mode 100644 test/fixtures/large_pr.json create mode 100644 test/fixtures/large_pr_exempt.json diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 5e41eb8b..c743ddfe 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -26,6 +26,14 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED']; const GITHUB_ACTIONS_APP = 'github-actions'; +// https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md +const LARGE_PR_LINE_THRESHOLD = 5000; +const LARGE_PR_REQUIRED_TSC_APPROVALS = 2; +// Labels marking pull requests the large pull request policy does not apply +// to. Routine dependency updates are excluded by the policy, and release +// proposals are machine-generated and follow the release process instead. +const LARGE_PR_EXEMPT_LABELS = ['dependencies', 'release']; + 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 +51,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', @@ -139,6 +148,31 @@ export default class PRChecker { .map((p) => p.reviewer.login); } + // Net change in lines, as the large pull request policy counts it. Returns + // undefined when the data is not available, so callers can tell "not large" + // apart from "cannot tell". + getNetLineChange() { + const { additions, deletions } = this.pr; + + if (typeof additions !== 'number' || typeof deletions !== 'number') { + return undefined; + } + + return additions - deletions; + } + + // Whether the pull request falls into one of the categories the large pull + // request policy exempts. Only the categories that can be told apart from + // the pull request data alone are covered here: the remaining ones (routine + // dependency updates, WPT imports, bot-issued pull requests, test-only + // refactors) need a judgement this cannot make, which is why a suspected + // large pull request is reported rather than blocked outright. + isExemptFromLargePRPolicy() { + const labels = this.pr.labels?.nodes?.map((l) => l.name) ?? []; + + return LARGE_PR_EXEMPT_LABELS.some((label) => labels.includes(label)); + } + formatReview(reviewer, review) { let hint = ''; if (reviewer.isTSC()) { @@ -221,6 +255,38 @@ export default class PRChecker { } } + const netLineChange = this.getNetLineChange(); + const isLargePR = netLineChange > LARGE_PR_LINE_THRESHOLD; + + // A semver-major pull request has already been held to the same approval + // requirement above, so only check the ones that have not been. + if (isLargePR && !isSemverMajor) { + const tscApproved = this.getTSC(approved); + + if (this.isExemptFromLargePRPolicy()) { + cli.info( + `This PR changes ${netLineChange} lines, but is exempt from the ` + + 'large pull request policy' + ); + } else if (tscApproved.length < LARGE_PR_REQUIRED_TSC_APPROVALS) { + const message = 'large pull requests require at least ' + + `${LARGE_PR_REQUIRED_TSC_APPROVALS} TSC approvals`; + cli.error(message); + cli.info( + `This PR changes ${netLineChange} lines, over the ` + + `${LARGE_PR_LINE_THRESHOLD} line threshold. See ` + + 'https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md' + ); + this.addReason( + PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL, message, { + approvals: tscApproved.length, + required: LARGE_PR_REQUIRED_TSC_APPROVALS, + lines: netLineChange + }); + return false; + } + } + let fastTrackAppendix = ''; if (isFastTracked) { const comment = [...this.comments].reverse().find((c) => diff --git a/lib/queries/PR.gql b/lib/queries/PR.gql index 10400af4..ee52f593 100644 --- a/lib/queries/PR.gql +++ b/lib/queries/PR.gql @@ -57,6 +57,8 @@ query PR($prid: Int!, $owner: String!, $repo: String!) { baseRefName, headRefName, changedFiles, + additions, + deletions, mergeable, closed, closedAt, diff --git a/test/fixtures/data.js b/test/fixtures/data.js index c05b5d05..62bf5766 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -86,6 +86,8 @@ 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 largeExemptPR = readJSON('large_pr_exempt.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..4205a842 --- /dev/null +++ b/test/fixtures/large_pr.json @@ -0,0 +1,20 @@ +{ + "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": [] + }, + "title": "lib: awesome changes", + "baseRefName": "main", + "headRefName": "awesome-changes", + "additions": 6000, + "deletions": 500 +} diff --git a/test/fixtures/large_pr_exempt.json b/test/fixtures/large_pr_exempt.json new file mode 100644 index 00000000..8e78d0d5 --- /dev/null +++ b/test/fixtures/large_pr_exempt.json @@ -0,0 +1,24 @@ +{ + "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": "dependencies" + } + ] + }, + "title": "deps: update a dependency", + "baseRefName": "main", + "headRefName": "awesome-changes", + "additions": 6000, + "deletions": 500 +} diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a64c4855..87753ec6 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -40,6 +40,8 @@ import { firstTimerPR, firstTimerPrivatePR, semverMajorPR, + largePR, + largeExemptPR, conflictingPR, closedPR, mergedPR, @@ -61,6 +63,7 @@ const { MISSING_APPROVAL, MISSING_GITHUB_CI, MISSING_JENKINS_CI, + MISSING_LARGE_PR_TSC_APPROVAL, MISSING_TSC_APPROVAL, REQUESTED_CHANGES, STALE_REVIEW, @@ -176,6 +179,132 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when a large PR has fewer than 2 TSC approvals', () => { + 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'], + ['This PR changes 5500 lines, over the 5000 line threshold. See ' + + 'https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md'] + ] + }; + 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, + lines: 5500 + }]); + cli.assertCalledWith(expectedLogs); + }); + + it('should not require TSC approvals for an exempt large PR', () => { + const cli = new TestCLI(); + + const expectedLogs = { + 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'], + ['This PR changes 5500 lines, but is exempt from the large pull ' + + 'request policy'] + ] + }; + const pr = Object.assign({}, largeExemptPR, { + 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, []); + cli.assertCalledWith(expectedLogs); + }); + + it('should not apply the large PR check when the line counts are absent', + () => { + const cli = new TestCLI(); + + // Older fixtures, and any PR queried before `additions`/`deletions` + // were added to the query, carry no line counts. + const pr = Object.assign({}, largePR, { + createdAt: GT_7D, + additions: undefined, + deletions: undefined + }); + + 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, []); + }); + it('should error when PR has change requests', () => { const cli = new TestCLI();