Brian Muenzenmeyer

Review Scrutiny

We use a pull request workflow at work to review one another’s changesets. Our monorepo contains 24 packages of varying scopes, from our user-facing command-line tools, componentry, and configuration to our documentation site, internal modules, and tumbleweeds of dependencies. This central hub of code deploys with automated precision after exhaustive CI assertions. We move a lot of code through it, cleanly and with confidence. Yet as the team grows, cracks have formed not in the code, but in the process.

Requesting Clearance to Land 🛬

Being little-A agile practitioners, we try to meet as a team to reduce written documentation. Our manifestation of this principle within our code is the pull request. We structure our pull requests as the public forum where change is discussed and agreed upon. The burden is on the author to communicate the scope, the trade-offs, how to validate the changes, and what areas may require additional eyes. We structure our release notes off of blocks of text introduced in the pull request. We semver our packages based on labels. We put a lot of time into pull requests because we often reference them months or years later to justify or address some system decision.

The upfront accounting of the change is good, but making everyone look at everything creates bottlenecks. It’s as inefficient as having everyone attend every meeting. It may have made sense with a team of two (an actual circumstance we found ourselves in), but not four, or eight. We kept finding ourselves, sprint after sprint, retrospecting that there was always more code to review than time to write our own. It consistently led to pull requests being open a long time, verbal pleas to have a pull request looked at right now, and carry over. What began as an earnest attempt to keep everyone in the loop was creating too much friction.

And the tools were arrayed against us too. GitHub’s CODEOWNERS feature ensured everyone on the core team we alerted to new pull requests. A sprint-based project board automatically tracked new things to review. Branch protection ensured that a defined number of approvals accompanied every change. What we found, however, was that review thresholds often didn’t match the scope. A single number couldn’t apply to the spectrum of work that 24 packages presented us with during the sprint. Our pull request process was low-trust. This sentiment is not new. Patricia puts it well:

Other teams opt for more extreme measures.

What we needed was a more flexible set of tools, or rather, a more flexible process. Enter “Review Scrutiny.”

Review Scrutiny

Review scrutiny is another upfront consideration we make when writing a pull request. It’s a risk mitigation calculation that we force ourselves to think through before asking for others to review. Each pull request now comes prefilled with this new section:

## Review Scrutiny

Because this change `has this impact`, this pull request requires `N` approval(s). 

Some examples:

  • Because this change is limited to internal comments and tests, this pull request requires 1 approval.
  • Because this change affects unreleased code, this pull request requires 1 approval.
  • Because this change affects the public API, this pull request requires 2 approvals.
  • Because this change impacts team process, this pull request requires 3 approvals.

To encourage use, we added a new checklist item to our pull request template. This brings our template’s boilerplate to a healthy-but-almost-heavy 5 things. An accompanying HTML comment visible only while editing markdown reminds the team and contributors how to determine review scrutiny. When a pull request reaches the number of approvals review scrutiny defines it may be merged. Sometimes this means it exceeds the branch protection minimum. Often times the review scrutiny threshold is lower, leading to an admin merge. People over process.

👋 GitHub, if you are listening, please allow defining the number of approvers per pull request.

Impact and Results

We introduced review scrutiny at the end of May. Some in-flight pull requests were augmented to define their requirements. New ones now for over a month have 100% adherence to this new process. From my perspective, it’s felt like a win and an intuitive part of our workflow. As a writer, I now find myself wondering where the boilerplate is on our repositories outside of the monorepo. As a reviewer, I expect the same. But what struck me was when I glanced at our GitHub metrics.

We have a dashboard that reports simple metrics like pull request throughput or time to merge and the like. No one looks at it most of the time, as programming efficacy is hard to distill into lines of code changed or how many review comments a pull request generates. But there is something here to think about.

A graph depicting PR merge time, the time it takes for a pull request to go from open to merged.

PR (pull request) Merge Time is the time it take for a pull request to go from open to merged.

This looks like a healthy trend to me. And yes, there are many many factors that can add up to different throughputs, like smaller stories that are easier to review. But I posit that we are always trying to do this, so it’s a close enough for me measure, even if not controlled scientifically.

The same dashboard gave me an average merge time for each week too. As far as I know, these numbers include nights and weekends within both ranges:

Weeks Average Merge Time
4 Weeks Prior to Review Scrutiny 208 hours
4 Weeks After Review Scrutiny 129 hours

This is a 38% reduction in time it takes our code to be merged. All from a simple process change added to our pull request template.

The pull request author knows best what sort of change they are introducing. We streamlined our process by altering a rigid tool and trusting one another. We’ve had one instance of a single-reviewer pull request being approved and merged, only for a bug to be found that required a follow up. We’ll monitor whether or not that was acceptable risk—but so far its smoother skies ahead and our review queue is a bit more manageable.