RFC: Require Pull Requests for all llvm-project commits

LLVM Project
,
1

TL;DR All unreviewed changes should be merged to the main branch via a GitHub Pull Request (PR). If you create PRs for all your changes and submit them (with or without reviewers approval) using the GitHub UI, this proposal does not affect you.

Introduction

The LLVM project’s current contribution model is a hybrid of direct commits to the main branch and PRs. This model is the result of our migrations from SVN to Git and from Phabricator to GitHub. The LLVM Developer Policy still reflects legacy “obvious commit” and "trust but verify” practices, which are at odds with the tooling and scale of the project today.

This RFC proposes that we complete our transition to a modern development workflow by requiring all changes to the llvm-project repository to go through the GitHub Pull Request process and revoking direct commit access to the main branch.

Proposal

This RFC proposes a change to the LLVM development process to standardize on a Pull Request-centric workflow. The goal is to unify the contribution mechanism, with stricter enforcement of checks and approvals to be considered in future proposals.

  1. The main branch of the llvm-project repository will be configured with branch protection rules to disallow direct pushes.
  2. All code contributions, from all developers, must be submitted via a GitHub PR. This creates a single, auditable[1] path for all changes entering the repository.
  3. Direct commit access to main will be revoked for all individual developers. Merging will be handled exclusively through the GitHub PR interface.

Non-proposal

This RFC explicitly does not propose making premerge checks mandatory, nor does it propose requiring reviewer approval[2].

Acceptance of this RFC does allow us to consider workflow changes like these, and they’re certainly worth discussing as a follow-up. That said, the intent of this proposal is to gather feedback on a process change that’s useful in its own right.

Rationale

Adopting a universal PR model, even without immediate enforcement of CI and reviews, provides foundational benefits for the project.

  • Unlocking New Workflows: There are process improvements that would be unlocked by this change such as the ability to revert commits and to merge PRs to the release branch directly from the GitHub UI. The PR also provides a consistent place for post-commit review and allows the use of labels to track and coordinate post-commit reviews.
  • Standardizing the Contributor Experience: This change lowers the barrier to entry for new contributors by standardizing on the “fork-and-PR” model. If contributors in a specific area use direct commits, it is difficult for new contributors to subscribe to changes or share feedback, which would be improved by having a PR for each change.
  • A Foundation for Mainline Stability: While not strictly enforced, requiring PRs provides a clear venue for running pre-merge tests and encourages a “review-then-merge” mindset over the current “commit-then-revert” reality. This makes it easier to trace breakages back to a specific PR that includes any discussion and the evolution of the patch which is helpful in understanding the intent of the change.
  • Enabling Future Process Improvements: This proposal is a prerequisite for any future improvements to our development process. We cannot programmatically enforce quality gates like mandatory reviews or CI until we first require that all changes pass through a PR. This move unlocks the ability to incrementally introduce more robust automation and governance in the future.
  • Improving Security Posture: The current model grants direct write access to the main branch to hundreds of developers, creating an unnecessarily large attack surface. Revoking direct commit access shrinks this surface and enforces the principle of least privilege.

Implementation

We are aware of concerns around the impact on developer velocity, but we believe the benefits outweigh the costs and a small overhead is a worthwhile trade-off. The primary change is the mechanical process of creating a PR versus a direct push.

While most contributors already use GitHub PRs and thus should be unaffected by this change, there are still a number of contributors relying on direct commits and we want to give them sufficient time to adjust to the new workflow.

Our proposal is to enable the “Require a pull request before merging” ruleset for the llvm-project repository upon acceptance of this RFC, but also give everyone the ability to bypass this rule for a trial period. This change would not prevent any existing workflows, you would still be able to directly commit to main, but you would see the following message every time you do so:

Bypassed rule violations for refs/heads/main:

- Changes must be made through a pull request.

This is similar to the “Require status checks to pass” ruleset we recently enabled as a prerequisite for the “auto-merge” feature.

We hope this change would encourage everyone to use GitHub PRs and to bring up any friction points so we can look into streamlining the workflow such as improving llvm/utils/git scripts to automate the creation of PRs for trivial changes.

We propose to remove the bypass option at the end of October. This coincides with LLVM Developers’ Meeting and we plan to host a round table to collect more feedback from the community and discuss potential future improvements.

@petrhosek on behalf of the @infra-area-team.

  1. The Git commit log itself is not auditable since the Git commit author and committer can be changed. GitHub Activity view lets you see who pushed a change regardless of the Git metadata but it is not regularly reviewed. ↩︎

  2. This is already the case today, while the reviewer approval on PR is strongly encouraged, it is not required; authors are also encouraged to use skip-precommit-approval label if they do not seek approval. ↩︎

31 Likes
Tooling for PRs-without-precommit-review
LLVM Qualification WG sync-ups meeting minutes
Historical Clang Language WG Meeting Minutes (Jun 2025-Sep 2025)
Historical Clang Language WG Meeting Minutes (Sep 2025-Dec 2025)
LLVM Project Council Meeting Notes - February 4, 2026
Require Pull Requests for all CIRCT commits
Attributing a commit to a different author
LLVM Project Council Meeting - October 1, 2025
LLVM Project Council Meeting - November 19, 2025
Operational Maturity Round Table Notes
LLVM Project Council Meeting - December 10, 2025
LLVM Weekly - #610, September 8th 2025
Requests for all llvm-project commits push
[RFC] Enabling Graphite Merge Queue to resolve infinite loops while merging stacked PRs
Announcement: LLVM Dashboard to Monitor Code Review and Contribution Rates Over Time
RFC: Removing the "Require Pull Request" bypass
2

Thanks for all the work bringing this PR to the community, @petrhosek

Both individually, and as part of the @infra-area-team I’m really enthusiastic about this change on its own merits, and for the future opportunities it may enable. It seems well overdue & well worth doing.

3

+1 very excited to see this!

4

I’m not a big fan of this. As a maintainer, we spend a lot of time fixing up the build/etc as well as do stuff that doesn’t require review, and this is going to raise the overhead for these things and make us less responsive. Maintainers are already overworked, so adding additional overhead for things that are common is unfortunate.

As a side note: I remember distinctly that as a part of the ‘github’ transition, and again the transition to start using PRs/ending Phab, that there were quite a few people who were assured that these both didn’t mean that PRs would ever be required/etc. It seems like that was either a mistruth, or a forgotten assurance.

9 Likes
Historical Clang Language WG Meeting Minutes (Jun 2025-Sep 2025)
5

I don’t quite follow what you mean by this, can you elaborate?

If you don’t change the policy on review, how is this proposal actually making it more secure? If I’m malicious I can just open a PR and merge it immediately just as well?

6

The one I’m most looking forward to, would be to track post-commit review (tag changes pushed without approval that haven’t been post-commit reviewed yet) - allowing coordinating post-commit review amongst multiple developers (whereas currently if one attempts to post-commit review there’s no idea what other folks have already reviewed, so we’re all doing it alone/redundantly, if we’re doing it at all)

Per the [1] footnote I believe the notion is that direct pushes can have arbitrary author/committer information included, but pushing via the PR UI doesn’t allow such variation/creativity.

2 Likes
7

For folks who still want a scripted way to push to main from the CLI in a single invocation, this can be scripted: git-push-to-llvm-main · GitHub

I use this as git push-to-llvm-main HEAD <branch_name_in_my_fork>.

Here is an example of a PR pushed and merged with this script: [MLIR] Apply clang-tidy fixes for readability-container-size-empty in XeGPUOps.cpp (NFC) by joker-eph · Pull Request #156752 · llvm/llvm-project · GitHub

git push-to-llvm
git push-to-llvm1337×605 80.1 KB

One extra perk: this works even if your branch isn’t perfectly rebased on main! (where a git push would fail)
GitHub during squash/merge takes care of it.

Re-reading, I think I got it actually: we can’t revert commits pushed to main from the UI, we have to use the CLI. On the other hand PRs have a convenient “revert” button (I’m using it all the time, it’s really nice!).
(Thanks @dblaikie for clarifying the secure aspect of merging vs pushing, I now remember this mentioned in a past discussion!)

6 Likes
Tooling for PRs-without-precommit-review
8

I remain very strongly opposed to this. I’ve been outspoken on this at multiple points in the past, so I won’t bother to repeat. I do want to be very clear that I believe this to be a poor decision for the project as a whole, though I also understand the reasons why it seems appealing to some.

5 Likes
9

I pretty much only use direct pushes in two cases: reverts and NFC changes like removing trailing whitespace from a file. I still don’t think these need a PR, but I’m interested in what process improvements this proposal would suggest for these cases.

10

I’m supportive of this proposal. Since enabling auto-merge, I’ve been creating PRs for changes I would normally push directly to main, so they can go through PR testing in case my local testing missed something. The added confidence has far outweighed the small overhead of clicking two buttons in the GitHub UI.

11

I’m supportive too. I’m often afraid of accidentally pushing something to main.

However I feel uneasy about the last paragraph. How are we going to fix broken CI if the bypass option is removed and direct pushes are prohibited? Even if there is a way, I wouldn’t want my PRs to be blocked pending CI fix. (Well, I could rebase my PR on a stable commit, but this is an ugly workaround and not always possible.)

12

To clarify, our proposal is to only remove the bypass option for the “Require a pull request before merging” ruleset and not the “Require status checks to pass” ruleset. You’ll be still able to merge a PR even without passing CI.

1 Like
13

I’d like to share my thoughts as a Clang contributor. In short, I’m against this RFC. It’s at the very least premature, not motivated enough in terms of tangible benefits it brings to the community that outweigh the churn it introduces.

Unlocking New Workflows

I agree that having more options is useful, but I’m not sure this will make any practical difference. It is definitely not improving anything for reverts that do not apply cleantly. I invite other maintainers to speak whether this is helpful.

Additionally, more reverts via UI means we’re going to have even more revert-* branches upstream. So far we’ve accumulated 93 of those, but with this push towards using UI, I expect this number to explode. I haven’t heard of any plan to deal with them.

This sounds useful, but I’d like to hear more from release managers on what will change in the backporting process.

I agree that this is useful. Technically you can comment on individual commits, which I did myself on several occasions and received comments this way several times, but overall the feature seems rather obscure and suffers from underdevelopment (I didn’t receive notifications unless I was explicitly tagged).

For the record, use of labels for that was a suggestion made on the thread, but it didn’t get much discussion, so it’s not like community is asking for this in that thread. I do agree that this sounds useful, but for bookkeeping and coordination between reviewers. To me it feels we’re way too short on them to take real advantage of this, but happy to be told I’m wrong.

Standardizing the Contributor Experience

As much as I’d like to see this model to be standardized, I don’t see this happening as long as we have user branches enabled. Today I counted 1610 branches created by 155 contributors, which is close double of the numbers reported a year ago.

At the moment PR merge options are limited to squash-and-merge, so revoking direct push access to main can disable users of stacked patches to merge their work. I’m not a user of stacked patches, so I invite other contributors to speak on this matter.

Sounds good, but do we have any evidence of this being a problem that new contributors have? From my experience on both sides, monitoring for changes requires familiarity with the code in question, which new contributors lack. This is what maintainers typically do, so it would make sense to advertise this feature for them.

A Foundation for Mainline Stability

I definitely agree that we’d benefit from more pre-merge testing and less broken main branch, but do you have an estimation how much additional CI load we’re looking at vs how much spare capacity we have? I see that for the first time in years we enjoy a very responsive CI, and it would be a shame to sacrifice this to check (mostly) NFC commits and reverts.

I think intent of the change is something commit description should have anyway. Having a PR for change is helpful for context in simpler cases, but in more complex cases we rely on commit authors to link relevant PRs and issues. This proposal doesn’t help with that.

Enabling Future Process Improvements

I strongly disagree with this assertion. I think there’s ample room to improve our processes without accepting this proposal. I do see that a certain direction of changes to the process would greatly benefit from this proposal, but I don’t remember an accepted RFC on this matter to say that something is blocked.

I don’t think we can do that anyway, so this proposal enabling those is no more than a theoretical benefit to me.

Reviews don’t help much with quality unless done by the right (experienced) reviewers, and we don’t have nearly enough of them. Ever growing graveyard of open PRs is one evidence of that.

Mandatory CI is helpful in most cases, but actively harmful when you need to do a revert because post-commit CI failed. It requires you to wait for pre-commit CI to finish (hopefully you don’t have to wait for hours), knowing that it’s very unlikely to fail, because it didn’t catch the failure in the first place. While all this is happening, main is broken, and every PR opened against such broken main would also be broken for reasons unrelated to changes in the PR.

Improving Security Posture

I tend to agree with this, but not a security expert myself, so open to feedback from our Security Response Group. However, I’m doubtful that replacing direct pushes with unreviewed PRs makes any difference. Combined with third-party review — yes, but as outlined above, at least some of subprojects don’t have enough reviewer time to require mandatory reviews. This is one of the reasons I found this proposal premature.

Historical Clang Language WG Meeting Minutes (Jun 2025-Sep 2025)
14

Our goal is to try and minimize the negative impact of this change. Would a script that allows the creation and merging of PRs from the CLI in a single invocation such as the one shared in RFC: Require Pull Requests for all llvm-project commits - #7 by mehdi_amini alleviate your concerns?

I don’t recall any such discussion but it’s possible I wasn’t involved. I’m not sure if anyone can give such a strong assurance given the ever evolving nature of LLVM (I can’t even guarantee that we’re going to be using Git a decade from now…), but regardless of what was promised, this discussion exists and should be made on the merits.

15

This RFC alone does not prevent malicious changes. To do so, we would need to require PRs for all changes (this RFC), plus we would need to require approval for all PRs, and we would need to discard approvals on push to avoid the situation where the author submits a legitimate PR but modifies it after approval prior to merge. However, I still think this RFC is a step in the right direction that improves auditability.

1 Like
16

From my own experience as a maintainer of the LLVM build system and a vendor Clang toolchain that closely follows HEAD and is thus exposed to a lot of the churn, not every NFC is in fact an NFC. I did a quick search through the Git log and found that over the last year, 7% of all reverted changes were NFC ones; that number remains remarkably consistent even if I go further into the past. This doesn’t include broken NFC commits that were fixed forward, NFC commits that changed the ABI, file formats, etc. (I’ve seen all of those) so by my estimates roughly 2% of changes labeled as NFC introduce functional changes and would benefit from more scrutiny. I don’t think that every one of them necessarily has to go through a code review, but even just running presubmit CI checks would be often beneficial.

Regarding reverts, this was already mentioned by others in this topic, but I also find the “Revert” button on PRs very convenient compared to:

git stash
git fetch origin
git checkout -b revert origin/main
git revert <commit>
git push origin HEAD:main
git checkout -
git stash pop

I stopped pushing changes directly to main for the “Revert” button alone, in case I need to revert my own changes. The fact that I can do it from my phone is an extra benefit.

3 Likes
17

My biggest concern is that this is yet another change that makes it harder for hobbyists and academics to be involved with LLVM. All of these are little changes; combined they mean each contribution takes a long time. And maybe it’s time I don’t have to spare. And this change is mostly because github doesn’t offer the same capabilities as phabricator. In the past we could use it to setup alerts; there was no need for PRs.

LLVM conferences are now extremely expensive. Compiling LLVM & running tests is impossible unless you have a nearby expensive server. I personally struggle a lot to keep my students productive and involved in LLVM these days. We just don’t have resources nor energy to spend on years-long PR reviews.

I understand that some of you want to professionalize LLVM, but I think academics/hobbyists played their role in making LLVM what it is today. Alienating them means less research done on LLVM and less people trained on LLVM. Which turn means you’ll have a harder time hiring in the future. And less people trying out crazy things that may turn out to be useful.

7 Likes
18

FWIW, I’m +1 to this change. I’m not necessarily a security person or anything like that, but I find it a bit bonkers that a project so fundamental to several major companies’ toolchains has such a simple way to get something into LLVM without an easy way of monitoring what is going on. Even if reviews aren’t required, the creation of a PR triggers notifications for me, if it touches code in areas I care about, so as long as I’m looking at my emails (which I do normally), with this change in policy, nobody could make a change to the core llvm-objcopy code (for example) without me noticing. Admittedly changes could still be made to some underlying libraries, but hopefully you get my point.

As stated in the original post, this isn’t about requiring reviews on PRs. Contributors, would still be able to merge without review as things stand, so it shouldn’t have any noticeable impact on your ability to make changes.

Since this is the place for that discussion, it may be worth you reiterating your concerns here. Otherwise, those with shorter memories/the lack of desire to go digging/weren’t involved with the original discussion, won’t understand your concerns.

5 Likes
Pull request merge queue from Github
19

I’m somewhat split on this. On the one hand I think it’s kinda nuts that everybody can push commits without any sort of notification for maintainers, on the other hand this can be incredibly valuable for maintainers themselves I think.
Since the main goal of this seems to be more strict merge requirements, I’m going to comment on that mostly. I understand that that is not proposed here, but if that won’t happen I would be very hesitant to make the lives of other maintainers more difficult, even if it’s not significant. I almost never revert stuff, so can’t really say how much more difficult/easy it would be after the change.

In libc++ I don’t think we have a significant amount of commits pushed directly, since we have incredibly good pre-commit CI (I break post-commit bots maybe once a month or so on average, probably less), meaning that almost all patches get PRs already to get some CI coverage. I would very much welcome if people couldn’t just push patches to libc++, or merge them without review. As a libc++ maintainer myself I do want to merge stuff without review that are obvious though. Reviewer time is incredibly limited, so I want to spend it on PRs that are non-trivial, not me doing some simple refactorings. The problem here is that GitHub is just not flexible enough to allow different actions for different sets of people as far as I’m aware. There is no way to require review from a group for changes to a subdirectory AFAIK. This means there is no way to enforce that a group has actually reviewed a patch. While rare nowadays, it happens from time to time that PRs are merged without any of the libc++ maintainers having looked at a PR. There is also no way to not approve as a group when you are a member of that group.

20

It is trivial to script…

Here is a script that finds all user branches (starting with /users/ or revert-) without an associated open pull-request: Prune LLVM user branches · GitHub

Right now, the script prints:

Found 1709 user branches
Found 549 branches with a PR

It also optionally offers to delete the branches older than X days. Here is a full trace:

Happy to proceed with deleting old branch if we agree on a policy (number of days old? or something?).
Instead of just deleting them, we can also move them to an “archive” repo of some sort (but we had to handle naming collisions in case a branch is reintroduced later and has to be archived again).

I don’t quite see the issue actually? Stacked patches are implemented with one separate PR per commit in the stack, and they don’t require push-to-main to merge nicely.

1 Like