We arrived at a similar concept, though ours it the negation of this: we have a series of deterministic checks that can potentially add the `needs-human-review` tag to a PR. If that tag isn't present, you can merge your own PR; otherwise you need at least one human.
I think we're probably around 50/50 needing human review vs not.
Our rules for whether something needs a human review are currently mostly filepath-based: are you touching the api, or an important internal contract, or adding db migrations? Though I'd like to make it a bit smarter as we're definitely getting false positives.
I thought for sure you might be one of my coworkers based on this comment, we do pretty much the same thing with our “LLM Review” tool. It tries its best to determine what needs human eyes (mostly through file paths and the model’s own “discretion” for changes outside the “important” paths).
It can be even more lines as long as the hunks are continuous. Once you lose the context of change and have to “zoom out” to see where it fits in, the review time grows significantly.
This struck me too, but I think it is intentionally a high limit to prevent problems like relatively low impact/high LoC tests from tipping a small change into a large one. That would encourage gamification like skipping useful scenarios to keep a change "small".
I disagree with their criteria that they couple with it, but I could come up with some of my own that I'd like better.
"This has 900 lines but 850 of them are in codegen boilerplate that changed", for instance.
"This has 900 lines but they're mostly updating a bunch of test fixtures along common patterns."
"This is 800 lines because we bumped ten version dependencies and that bumped a lot of other things + they changed the package naming for some so we had to rewrite a bunch of imports."
"I added two panels to a Grafana dashboard." (ok slight exaggeration)
Of course it's relevant: your statement is a strawman ... they didn't claim that 1000 lines is a small change, that was part of a conjunction. Basic logic here.
I did bother to read it (and that ad hominem charge is an explicit violation of the HN guidelines) and you're mischaracterizing it. In any case they did not say that 1000 lines is a small change.
> Only my personal view but even 1000 LoC is not a "small change".
To make matters worse, they effectively implemented a YOLO allowlist to allow a specific developer to batch 1k loc changes to the set of services they are working on without requiring any scrutiny.
Crazy stuff.
They could achieve this in a much simpler way by just removing PR requirements from those users in those projects.
Despite the name, the label isn’t is only added if it meets about 5 other criteria, files and folders can be excluded meaning any PR with a change in them isn’t eligible for the label, and the PRs still need to pass all the non-AI quality checks.
Even if it’s not perfect, I think it’s the right idea. There’s some category of changes that are low risk and simple enough that a human reviewer isn’t needed and if you can identify those then you’re going to be able to save human review for the things that really need it.
Every bad change here, that has been merged in the last 6 months, correlated with a lack of human review. Sure, someone hit the approve button, but it was done within 30 seconds of the PR creation time, clearly no review was performed. This is in repos that have deterministic as well as ai driven review CI checks.
And that doesn't even get into the "features" that were implemented without reviewing the design or requirements were actually hit (they were not).
I would love to replace all of these useless meat bags with more ai. But I would absolutely still want to be in the loop to review things.
The lesson I've learned about AI is that lazy/bad engineers without AI are still lazy/bad engineers with AI.
Okay, sorry, I don't know what I'm arguing anymore, this turned into a rant.
We have something similar. I made a service that you install in your GitHub repo. It has configuration where you can set the review bar for specific paths. That review bar could be an AI agent with a specific prompt has to bless the change, so you can do stuff like "any change except to billing is okay.” If that passes you can merge your own PRs, no review needed.
Something like 30% of PRs in our company are merged like this. It's a huge velocity win when you don't have to wait around for a review.
IIRC this is also somewhat verified empirically. I think it was in Steve McConnell's I saw a plot of fault injection rate as a function of change size based on real data (probably collected in the '80s, but still!), and the number of bugs per line of code were by far highest in the 1–5 line change range.
This is the sort of thing that's difficult to argue economically for studying with proper randomisation and everything, but an observational study can be made on open source repositories (even in projects where the main contributors are on a corporate payroll) fairly easily with LLMs these days. I wish I had more time to pursue things like this...
> In my experience, the “small change” merge requests tend to be the most consequential.
I think you're confusing things. The "small change" refers to the diff size. It's far easier to review a one-liner because the impact analysis is limited to that local change.
Of course if your change is something like "mutex.lock()" or "rm -rf *" then these changes will be more consequential than 500 files worth of whitespace changes.
Oh no! This is going in the wrong direction, isn't it? From the headline, I'd assumed the takeaway was encouraging small changes (and 1000 LoC is not "small" by any definition) and maybe encouraging the author to refactor at larger sizes to make human review easier. Instead it's just bypassing human review entirely?
Code review is such a key step to the feedback loop and helps keep people current on the changes as they come out. Skipping that entirely is just inviting disaster.
"It’s just a small change; it doesn't affect anything, it won't break anything", any development manager would tell you that.
While small, frequent changes generally allow the system to remain stable, assuming the change is truly minor and doesn't shake the underlying infrastructure, it can be quite frustrating for the QA team to receive a new build every few hours. And also as the project progresses, the regression scope becomes extremely massive.
The solution that has worked for me in my projects is to combine daily nightly runs, performed prior to approving a major release, with local smoke tests and automation. Specifically, I focus on API test automation, which is both fast and efficient and cheap to develop and execute.
34 comments
[ 0.32 ms ] story [ 14.1 ms ] threadI think we're probably around 50/50 needing human review vs not.
Our rules for whether something needs a human review are currently mostly filepath-based: are you touching the api, or an important internal contract, or adding db migrations? Though I'd like to make it a bit smarter as we're definitely getting false positives.
Only my personal view but even 1000 LoC is not a "small change".
The other criteria are more important.
"This has 900 lines but 850 of them are in codegen boilerplate that changed", for instance.
"This has 900 lines but they're mostly updating a bunch of test fixtures along common patterns."
"This is 800 lines because we bumped ten version dependencies and that bumped a lot of other things + they changed the package naming for some so we had to rewrite a bunch of imports."
"I added two panels to a Grafana dashboard." (ok slight exaggeration)
I won't respond further.
> I won't respond further.
That's okay, I don't care.
If you bother to read the blog post, the other conditions in the "AND" are who posted the developer and which project the PR is updating.
This does not make it better. It's a YOLO bro allowlist.
I won't respond further.
To make matters worse, they effectively implemented a YOLO allowlist to allow a specific developer to batch 1k loc changes to the set of services they are working on without requiring any scrutiny.
Crazy stuff.
They could achieve this in a much simpler way by just removing PR requirements from those users in those projects.
But this has "agents".
I do not think this is a good metric
Even if it’s not perfect, I think it’s the right idea. There’s some category of changes that are low risk and simple enough that a human reviewer isn’t needed and if you can identify those then you’re going to be able to save human review for the things that really need it.
If something is low risk and simple enough then it can be quickly judged in 1 to 5 minutes. Not sure why someone can’t review it.
And that doesn't even get into the "features" that were implemented without reviewing the design or requirements were actually hit (they were not).
I would love to replace all of these useless meat bags with more ai. But I would absolutely still want to be in the loop to review things.
The lesson I've learned about AI is that lazy/bad engineers without AI are still lazy/bad engineers with AI.
Okay, sorry, I don't know what I'm arguing anymore, this turned into a rant.
Something like 30% of PRs in our company are merged like this. It's a huge velocity win when you don't have to wait around for a review.
Sure, they look harmless, but it’s all the external pieces (not shown in the diff) impacted by the “small change” tend to make such a disaster.
This is the sort of thing that's difficult to argue economically for studying with proper randomisation and everything, but an observational study can be made on open source repositories (even in projects where the main contributors are on a corporate payroll) fairly easily with LLMs these days. I wish I had more time to pursue things like this...
I think you're confusing things. The "small change" refers to the diff size. It's far easier to review a one-liner because the impact analysis is limited to that local change.
Of course if your change is something like "mutex.lock()" or "rm -rf *" then these changes will be more consequential than 500 files worth of whitespace changes.
The tiny 2 or 3 line change that fixes a problem doesn’t always get the same critical thought process.
When it crosses subsystems rarely crossed, there are often nasty surprises.
Code review is such a key step to the feedback loop and helps keep people current on the changes as they come out. Skipping that entirely is just inviting disaster.
What kind of change impacts fewer than 1 services or applications? Is this meant to say “at most N”?
While small, frequent changes generally allow the system to remain stable, assuming the change is truly minor and doesn't shake the underlying infrastructure, it can be quite frustrating for the QA team to receive a new build every few hours. And also as the project progresses, the regression scope becomes extremely massive.
The solution that has worked for me in my projects is to combine daily nightly runs, performed prior to approving a major release, with local smoke tests and automation. Specifically, I focus on API test automation, which is both fast and efficient and cheap to develop and execute.