Reply below > On Oct 17, 2025, at 04:20, Piotr P. Karwasz <[email protected]> wrote: > > > Hi Matt, > > Regarding your thoughts on RTC, as you know, I’m personally a strong > supporter of the Review-Then-Commit model, especially in the current > climate of increasing supply chain attacks and phishing campaigns, it > remains one of the most reliable safeguards we have.
The largest threat of supply chain attacks at this point is from third party dependencies, especially given the proliferation of Dependabot updates. Changes made to dependencies are never (or almost never) reviewed with the same level of rigor as changes made to our own code. > Please know that this isn’t about trust in you as a person or as an > engineer, I have complete confidence in both. My concerns are about two > things that go beyond any individual contributor: > > - Account security: Even well-protected accounts can be temporarily > compromised, through tokens, SSH keys, or access from another machine. I > know your setup is exceptionally secure (physical second factors for > both ASF and GitHub, SSH and GPG keys, etc.), but that’s the exception > rather than the rule. For context, even my own authentication tokens are > stored in 1Password, and I have copies of them on internet-connected > machines. These seem like attributes of an account which could allow for direct commits to protected branches rather than a reason to prevent anyone from doing so. > - Human oversight: No matter how careful we are, everyone occasionally > overlooks small details. That’s exactly what peer review helps catch. This is what I used to do when I could review the commits@ mailing list. Ever since that was flooded with Dependabot-related chatter, I have been unable to use the list for post-commit review. It’s not like this project moves at a super fast pace! We tend to make quarterly releases which surely gives enough time to browse through commits and bring up issues to dev@. > >> RTC tends to be used in highly active codebases with dozens or >> hundreds of regular contributors. > > That’s true, but it’s not exclusive to those projects. For example, the > `packageurl-java` repository [1] has only one active maintainer, and yet > RTC is still used successfully there. In larger projects, RTC (and often > a merge queue) becomes essential to keep `main` stable, but it can be > just as valuable in smaller teams. A project with one maintainer using RTC would mean that they can only accept external contributions and are otherwise not allowed to merge their own code. > >> RTC works best when reviewers have sufficient time available every >> week. > > I don’t think time availability necessarily dictates whether RTC or CTR > works better. The key question is simply whether every change receives a > review. For small or “trivial” commits, the review itself is usually > trivial, so I’d still prefer having that trace of peer validation. It is indirectly related. If review time increases, then the average size of code changes increases in order to batch changes. This is pretty standard DevOps practice: minimize work in progress batch size. Take a theoretical example: suppose a project maintains a maximum WIP count of 3 (i.e., only 3 PRs can be open at a time). In such a case, the project members have a couple choices: they can either ensure that reviewing and merging PRs is a top priority (can’t work on new things while the WIP queue is full after all), or they can make those WIP items large due to the long time waiting for feedback (obviously not ideal, but it’s the sort of behavior that is incentivized by this plan). Without a WIP limit, there is very little incentive to prioritize PR and RFC review, especially for anything non-trivial. > Personally, I’m flexible about the *order* of review (before or after > commit), as long as every commit is reviewed by someone other than the > author. If we ever decided to adopt a true CTR model, where every commit > is traceably reviewed, I’d be fine with that too. This is an interesting idea. Making the review process asynchronous and non-blocking is my main desire. I’m ok with making follow-up changes (it’s software; we can change it infinitely). What I’m not fond of is maintaining long-lived branches (an anti-pattern) and applying different review standards to different changes (which can happen incidentally based on who decides to review the change regardless of who made the change). >> RTC can technically be accomplished by having a coworker review the >> code before making a PR... > > That’s a fair point, and it highlights one of the challenges of > commercial open source: employer pressure can sometimes lead to rushed > or superficial reviews. > > Fortunately, that’s not really an issue for us: none of us are full-time > contributors here. In fact, it’s quite the opposite: if your employer > allows you to dedicate even 10% part of your time to Log4j, that’s a > generous contribution. (For reference, the small Tidelift sponsorship > Volkan and I receive is far below that.) > >> RTC seems to think that the ideal time to go over architectural design >> and other planning details is at the time of proposing the code >> change... > > I completely agree with this concern, but I don’t think it’s a > limitation of RTC itself: it’s more about how we apply it. When a PR > introduces an architectural change, we should indeed move that > discussion to the mailing list or GitHub discussions (which are mirrored > there) and hold the PR until consensus is reached. In such a situation, the architecture would have been reviewed on the mailing list first. If someone didn’t bother participating in that discussion but then tries to hold up the code change due to architectural concerns, that’s the sort of problem I’m concerned with. >> RTC is a flow optimized for accepting contributions from third >> parties... > > Outside of very active projects, I think the timing of review (before or > after merging) matters less than ensuring the review happens at all. > That’s where the real quality and trust come from. I’d like this post-merge review sign-off. It would also make release votes a little easier as there would be a sort of audit trail of approvals of everything leading up to that point. > In summary, I don’t think our discussion is really about “RTC vs CTR”, > but about ensuring that every commit to `main` (or `2.x`) is reviewed by > someone other than the author. I believe that’s essential for > maintaining quality and shared ownership. Personally, I prefer to have > reviews happen before merging, but if you prefer to merge first and > review afterward, that’s also fine: as long as every commit is > eventually reviewed before we cut a release. Sounds cool. Let me poke at the edge cases since that’s what I’m used to doing from a security point of view. How would we handle a code change that nobody wants to review or feels comfortable reviewing? Let’s use a Commons project as a theoretical example. Suppose someone wanted to contribute code to commons-crypto in order to support new PQH encryption ciphers recently standardized by NIST and IETF. Suppose that person is a cryptographer or works in that space in general. Now also suppose that the Commons PMC has approximately 0 cryptographic experts. Then nobody feels comfortable reviewing the code. Several years from then, some devastating exploit is found for elliptic curve cryptography that doesn’t affect PQH use of ECC. Thanks to the overly cautious review culture of this project, instead of avoiding new CVEs, they’ve allowed bit-rot to create new CVEs without the benefit of an alternative mode of operation to use (thus requiring an expensive dependency update and code changes to support the PQH mode). Keep in mind that in this example, I haven’t even specified if the contributor added new crypto code or if they’re simply integrating new crypto code added to OpenSSL. > Both approaches have trade-offs: > - With RTC, pull requests can sometimes linger unreviewed for too long. > - With CTR, there’s a risk that older commits slip through without ever > being reviewed. > > We’ve seen how that can happen in practice. For example, last year > Commons Compress gained two new dependencies that went unnoticed by > anyone except the original committer, and even the changelog didn’t > mention them until months later [2]. That was a security release, which > makes the situation even more delicate. When urgency is high, it becomes > harder to balance the need for timely fixes with the discipline of > thorough review [3]. Release early, release often! > > So, whichever workflow we use, the key is to make sure that review > actually happens and that we help each other keep that process moving. > > Best, > Piotr > > References: > [1] https://github.com/package-url/packageurl-java > [2] > https://github.com/apache/commons-compress/commit/804c4fd9bc7dba9826c6b938cff2d6fdfccc5dc4 > > [3] https://lists.apache.org/thread/5vtbbs18d4rntn8tpnpsvo0g9h6svc30 >
