Skip to content

feat: check large pull request approvals in git node land - #1173

Open
zeexzeex wants to merge 1 commit into
nodejs:mainfrom
zeexzeex:feat/large-pr-checks
Open

feat: check large pull request approvals in git node land#1173
zeexzeex wants to merge 1 commit into
nodejs:mainfrom
zeexzeex:feat/large-pr-checks

Conversation

@zeexzeex

@zeexzeex zeexzeex commented Aug 31, 2026

Copy link
Copy Markdown

Node.js requires two TSC approvals for large pull requests, but nothing enforces it at land time. This adds that check to git node land.

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 is visible in the pull request data. An earlier revision of this PR tried a 5000 line threshold, and checking it against the 6 pull requests currently carrying the large-pr label, it missed 2 of them: nodejs/node#64429 is well under the threshold, and nodejs/node#62217 has a net negative diff.

The project already records this judgement with the large-pr label, so this checks that instead, the way semver-major is checked. The approval requirement is the same for both, so the existing branch is widened rather than duplicated, with a distinct message and reason code so a failure tells the author which rule applies. A pull request carrying both labels is reported as semver-major.

Refs: https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md
Fixes: #1063

@zeexzeex
zeexzeex force-pushed the feat/large-pr-checks branch from 2149ab7 to f8afeef Compare August 31, 2026 07:00
@Renegade334

Copy link
Copy Markdown
Member

I don't think that trying to automatically detect whether a PR falls into the policy definition of a large PR is ever going to be feasible. This can't detect whether a PR adds a new subsystem, or detect whether a non-automated dependency change PR meets the large PR threshold (it's only automated commits that are exempt), and indeed a PR only needs to touch a single gypfile in /deps for this logic to exempt it.

I think we would be much better off making this a label-based check, à la fast track or semver-major. Large PRs are not getting landed quickly, it's highly likely that a clued-in reviewer will check in to add the label long before it's in a position to land, and we can always couple it with an action to conditionally comment a reminder when the PR is opened.

@zeexzeex

zeexzeex commented Sep 3, 2026

Copy link
Copy Markdown
Author

You're right. I checked against the large-pr label: of the 6 PRs carrying it, my logic misses 2. #64429 is well under the threshold, and #62217 has a net negative diff plus the dependencies label, so it gets exempted twice over despite not being a routine update.

I'll rewrite this as a large-pr label check alongside the existing semver-major branch, and drop the line counting, the exemption list, and the additions/deletions query fields.

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 <ujubongbong@gmail.com>
@zeexzeex
zeexzeex force-pushed the feat/large-pr-checks branch from f8afeef to 169b769 Compare September 3, 2026 01:28
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.

Add large pull request checks in git node land

2 participants