Proposal: relax PR merge rules, enforce them via CI by kolyshkin · Pull Request #5512 · opencontainers/runc

kolyshkin · 2026-10-04T23:53:45Z

This is a proposal. Maintainers, please share your opinion (or LGTM, if you agree).

Problem

We have quite a few maintainers, but most of us are busy with other things, so
reviews are scarce. As a result, many pull requests stay open for weeks or
months waiting for two LGTMs, which slows down the development a lot
(or requires a lot of pinging).

Proposal

Relax the merge rules (see the MAINTAINERS_GUIDE.md change for exact wording).
A pull request can be merged once it has:

two LGTMs from maintainers (as now); or
one LGTM, if it is a routine change: opened by Dependabot, or only
changing documentation and/or CI configuration; or
one LGTM from another maintainer, if the author is a maintainer and the
pull request has been open for review (not a draft) for at least 7 days
(14 days if it changes more than 100 lines, not counting vendor/),
giving other maintainers a chance to review and object.

Governance changes (MAINTAINERS, top-level *.md files other than
README.md and CHANGELOG.md, and the review-policy workflows) always
require two LGTMs.

In all cases, a maintainer requesting changes blocks the merge.

The guide also clarifies a few things which are current practice but are not
written down: an LGTM is a PR approval, and the author's own LGTM is not counted.

Enforcement

GitHub can't require a different number of approvals depending on who the
author is, or what files are changed. So the second commit adds a
review-policy check which evaluates the above rules and sets a commit status
on the PR head. It is designed so that a PR can't change the policy it is
checked against: it runs via pull_request_target, workflow_run (for
reviews), and schedule (for the time-based rule), using the workflow and
MAINTAINERS from the default branch, and never checks out the PR code.

A nice side effect is that only approvals from current MAINTAINERS are
counted. Currently GitHub also counts approvals from anyone with write access,
which includes a few emeritus maintainers.

I did a dry run of the policy script against all open PRs (with the API write
call stubbed out) and the results look right. The triggers themselves can only
be tested once this is in the default branch.

After merging

Once the check has set statuses on open PRs (it runs every 4 hours), a repo
admin needs to update the main branch protection:

Add review-policy (from GitHub Actions) as a required status check.
Lower the required number of approvals from 2 to 1.

Note that since enforce_admins is currently disabled, and all maintainers
are repo admins, any of us can still merge bypassing the rules. Whether to
change that is a separate discussion.

cc @mrunalp @cyphar @AkihiroSuda @thaJeztah @lifubang @rata

AkihiroSuda · 2026-10-05T01:16:11Z

+* one LGTM from a maintainer, if it is a routine change; or
+* one LGTM from another maintainer, if the author is a maintainer, the pull
+  request is not a governance change, and it has been open for review (that
+  is, not a draft) for at least 7 days. This gives other maintainers a chance


A large PR (e.g., > 100 lines) should require a longer period?

What do you have in mind? A month?

Also, a maintainer can submit require changes and just ask for more time.

I looked at the sizes of last 50 PRs (both opened and merged), to check lines added + removed, excluding vendor/, here's what I got:

Lines (except vendor/) Last 50 merged PRs Last 50 open PRs

≤ 20 33 7

21–100 12 18

101–300 4 15

> 300 1 10

median 11 ~100

So small PRs get merged, and it's the large ones that get stuck. That's exactly where a longer review period makes sense. So your threshold of 100 lines affects only 10% of what we merge, but half of what's waiting, and it seems right to me.

I've added this: a maintainer's PR with one LGTM can be merged after 14 days instead of 7 if it has more than 100 lines changed (not counting vendor/).

NOTE tests are counted. Let me know if you think they shouldn't be.

rata · 2026-10-05T16:03:25Z

I agree with the idea. It's EOD here now, I'd like my AI friend to take a closer look tomorrow.

A nice side effect is that only approvals from current MAINTAINERS are
counted. Currently GitHub also counts approvals from anyone with write access,
which includes a few emeritus maintainers.

Let's remove their access. Nothing good can come out of that, right? We can always give access to people that need it. But I guess we need to ask some LF people to do that?

kolyshkin · 2026-10-05T19:51:12Z

I agree with the idea. It's EOD here now, I'd like my AI friend to take a closer look tomorrow.

A nice side effect is that only approvals from current MAINTAINERS are
counted. Currently GitHub also counts approvals from anyone with write access,
which includes a few emeritus maintainers.

Let's remove their access. Nothing good can come out of that, right? We can always give access to people that need it. But I guess we need to ask some LF people to do that?

Yes, one needs to be an org admin to do that. I can only file a bug in the org: opencontainers/tob#160

rata

Left a few minor comments. Thanks a lot for proposing this and automating it!

kolyshkin · 2026-10-07T01:53:05Z

@rata thanks for the detailed review -- I think I've addressed all the comments, PTAL.

@opencontainers/runc-maintainers PTAL, this is an important process change.

rata

LGTM, thanks!

I'd add a few more comments to make sure all the precautions to keep this safe (never run from the PR branch, etc.) are not lifted by mistake

rata · 2026-10-08T09:39:27Z

+#    from forks it only gets a read-only token (see review-policy-trigger.yml);
+#  - schedule: re-evaluate all open pull requests. This is mostly for the
+#    time-based rule, but it also catches up with MAINTAINERS changes, missed
+#    events, and pull requests opened before this workflow was added.


I'd add very explictely what to avoid also, as it was not obvious to me:

# When changing this workflow, do NOT: # - add pull_request, pull_request_review or pull_request_review_comment triggers: those run the pull request's copy of this file; # - add actions/checkout, or run anything from a checked out tree; # - read repository files with a ref other than the default branch;

rata · 2026-10-08T09:40:19Z

+            const largeDays = 14;
+            const largeLines = 100;
+
+            // MAINTAINERS lines look like "Name <email> (@login)".


I'd add something like:

// XXX: Always get this from the default branch, even for pull requests against // release-* branches, so that there is a single source of truth.

rata · 2026-10-08T09:42:50Z

+# commit status.
+name: review-policy-trigger
+on:
+  pull_request_review:


I'd add something like:

# XXX: We need to be careful here. This runs on `pull_request_review`, this file is # run from the pull request's merge commit, so a pull request can change it. # That is harmless only as long as it stays a no-op: no permissions, no # secrets, no token use. review-policy.yml does the actual work from the # default branch and only accepts this trigger for pull_request_review runs.

thaJeztah · 2026-10-08T10:44:04Z

In general, I'm in favor of relaxing the approval requirements (as enforced by branch protection rules). I think it's reasonable to enforce a single approval, and leave it to the maintainers' discretion to decide if additional reviews are needed.

Someone who was given maintainer status was given that status based on trust, which (IMO) includes the trust to decide whether a change has risk, and because of that requires more eyes (which can be a review of a specific person / domain expert, or more review than enforced). At times, I wish GitHub reviews were not a "binary" approval; there's been many pull requests (across projects) where I'm not a domain expert, and my review / approval may be "best judgement". GitHub doesn't really have mechanisms to communicate that, beyond writing it in words ("LGTM" with some explanation 😂).

Requiring two (or "majority") approvals for governance or policy changes makes sense.

I'm not sure if the LOC and waiting period complexity is needed; in my experience, a single-line change can be just as risky (or riskier) than a large change; they sometimes are more risky because they may appear innocuous, so get merged without verifying the impact 😂

That said; I wouldn't mind some "nag" automation to ping maintainers for a (second) review. My notification inbox tends to fill up pretty fast, so "review requested" PRs may end up at page 2 or 3 of my notifications within a day.

kolyshkin · 2026-10-09T05:28:39Z

I'm not sure if the LOC and waiting period complexity is needed; in my experience, a single-line change can be just as risky (or riskier) than a large change; they sometimes are more risky because they may appear innocuous, so get merged without verifying the impact 😂

So this is not about the risk but about the complexity of doing a review. We assume that changes with smaller LOC require less maintainer's time to be reviewed, and big PRs with more LOC changed call for more review time. While this is not always the case, usually it is. So we (rather arbitrarily) divide all PRs into two categories, big and small, and set the rule that big PRs need more time to review than the small ones. It's not perfect but there is no way to have it perfect (in a perfect world you wake up and all your PRs from yesterday are reviewed by all other maintainers).

The other thing is, small PRs gets merged and larger ones gets stuck (see some statistics at #5512 (comment)). The two-week "deadline" is here to help with that.

I wouldn't mind some "nag" automation to ping maintainers for a (second) review.

I use @thaJeztah PTAL for that, occasionally adding a 🙏🏻, so I don't need any automation here. In fact I'd rather be against it, as I have too much emails in my inbox already. But I might be wrong and we can implement some nagging and pinging later as a followup.

kolyshkin · 2026-10-09T05:29:45Z

@lifubang @cyphar WDYT? This is an important change, I don't want to push it without you having a look.

Many pull requests stay open for weeks waiting for a second review, which slows down development considerably. Spell out the merge rules, and relax them for maintainers: a pull request authored by a maintainer can be merged with a single LGTM from another maintainer, provided it has been open for review for at least 7 days (14 days for large changes, i.e. more than 100 lines not counting vendor/), so that other maintainers had a chance to object. Also, allow routine pull requests (opened by Dependabot, or only changing documentation and/or CI configuration) to be merged with a single LGTM. Neither of the above applies to governance changes, i.e. changes to the project rules (such as this document) or their enforcement, which always require two LGTMs. While at it, clarify that: - an LGTM is a pull request approval; - the author's own LGTM does not count; - a maintainer requesting changes blocks the merge. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Add a check enforcing the pull request merge rules from MAINTAINERS_GUIDE.md. The result is set as a "review-policy" commit status on the pull request head, to be used as a required status check (instead of the GitHub's required number of approvals, which can not depend on who the author is, and also counts approvals from anyone having write access, not just from current maintainers). The policy is evaluated when a pull request is opened, updated, or its draft status changes (pull_request_target), when a review is submitted or dismissed (pull_request_review, chained via workflow_run so that it works for pull requests from forks), and every 4 hours (schedule), which is needed for the time-based rule. Runs triggered by Dependabot are skipped, as they only get a read-only token; such pull requests are handled once reviewed, or by the scheduled run. The pull_request and pull_request_review triggers are not used directly as those run the workflow from the pull request itself, which means a pull request could change the policy it is checked against. The workflow never checks out the pull request code, so add a zizmor exception for the dangerous-triggers audit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2026-10-10T03:10:22Z

@thaJeztah also, if you agree with this in principle, please LGTM (we can address pinging and nagging in a followup)

kolyshkin requested review from AkihiroSuda, cyphar, lifubang, mrunalp, rata and thaJeztah October 5, 2026 00:41

AkihiroSuda reviewed Oct 5, 2026

View reviewed changes

kolyshkin force-pushed the review-policy branch from 3d3832f to f6467f7 Compare October 5, 2026 02:47

AkihiroSuda approved these changes Oct 5, 2026

View reviewed changes

rata reviewed Oct 6, 2026

View reviewed changes

kolyshkin force-pushed the review-policy branch from f6467f7 to 87a92c5 Compare October 7, 2026 01:42

kolyshkin requested a review from rata October 7, 2026 01:51

kolyshkin force-pushed the review-policy branch from 87a92c5 to 4b28eac Compare October 7, 2026 01:51

rata approved these changes Oct 8, 2026

View reviewed changes

kolyshkin force-pushed the review-policy branch from 4b28eac to 85b39ca Compare October 9, 2026 05:36

kolyshkin mentioned this pull request Oct 9, 2026

[org] RFC: ways to move PRs forward #3052

Closed

kolyshkin added 2 commits October 9, 2026 13:36

kolyshkin force-pushed the review-policy branch from 85b39ca to b83e951 Compare October 9, 2026 20:37

Lines (except `vendor/`)	Last 50 merged PRs	Last 50 open PRs
≤ 20	33	7
21–100	12	18
101–300	4	15
> 300	1	10
median	11	~100

Conversation

kolyshkin commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Proposal

Enforcement

After merging

Uh oh!

AkihiroSuda Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

rata commented Oct 5, 2026

Uh oh!

kolyshkin commented Oct 5, 2026

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Oct 7, 2026

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rata Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

rata Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

rata Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 8, 2026

Uh oh!

kolyshkin commented Oct 9, 2026

Uh oh!

kolyshkin commented Oct 9, 2026

Uh oh!

kolyshkin commented Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kolyshkin commented Oct 4, 2026 •

edited

Loading