Christ is my all
2713 stories
·
3 followers

Attempting to reverse engineer code review

1 Share

I keep hearing complaints about code review. About volume, about quality, about trying to take responsibility for code produced by colleagues who have abdicated their responsibility for thinking and are uncritically hitting enter.

But how much of that is just grumpiness? Frustration? “AI slop” is the new “I didn’t like it.” Code review was always something I paid attention to as a manager, because when there was conflict on a team, it was the place most likely to show up. Code review is subjective work presented with the illusion of objectivity; everything unsaid can play out in it.

In the software factory model, no one writes the code and no one reviews the code. These go together: once the code is generated, human review is the bottleneck, so you validate some other way, through guardrails and automation. Most teams aren’t building from scratch, though. Akin to the challenge of self-driving cars, they are on mixed roads, humans and machines sharing the same lanes, which is the harder case (I loved this podcast on the topic).

So I wanted to see whether I could reverse engineer code review: some kind of machine gate a PR has to pass before it’s allowed to request actual human time and attention. Much of my career has been in open source, the write-code-in-public kind, so I picked a team with an open source repo I knew well enough to have context. I deliberately didn’t pick the highest-performing team I’d worked with: this one had mixed levels, engagement, and AI adoption, and I was curious how that played out in review.

What I did

First, I constrained Claude to be read-only on GitHub. The last thing I wanted was to leave a trail of inadvertent and likely weird AI-generated comments.

Then I had Claude (Opus) find the strongest code reviewers on the team, seeded with three names I asked it to confirm or deny. Its reasoning tracked with what I knew. It also flagged some reviewers who were thorough but inclined to nitpick, which made me add a style guide to the scope.

An aside on style guides: I started my software engineering career at Google, where the style guide was canon. I had readability in two languages, Objective-C and Java (basically, I jumped through a set of hoops to prove I could deliver high-quality reviews according to the style guide). I don’t really subscribe to any religions, in software development or otherwise, but one thing I took from that period of my career is that the true benefit of a style guide is that we do not discuss it, and spend the time on the things that matter instead.

Claude (Sonnet) built a corpus of 4,176 review comments and summaries by twelve reviewers, on over 1,700 PRs going back about 20 months. A Fable agent per reviewer pulled out the patterns in what they flagged, with examples: ~300 of them, which another agent compiled into 67 themes. Then I had it turn the themes into checks. Finally it went through every PR merged since the start of September (~500) to work out which checks the version the first reviewer saw would have failed, and whether review caught it. As on many teams, there were already AI review bots on the repo, so “review” in everything that follows means humans and bots together, unless noted otherwise.

This took real time, most of it reading what came back. It’s the kind of data I would have loved to have when I was running large orgs, and now it’s feasible.

What the machine recommended

What came out was a tidy recommendation: 57 checks:

  • 11 style guide rules
  • 8 test requirements, either tests for classes of issue or required test types for certain kinds of work
  • 26 CI and static analysis checks (lint rules and scripts that run on every push)
  • 12 instructions for a project-specific review skill that an AI reviewer could run, the judgement calls that don’t reduce to a regex.

I’ll call those style, tests, CI and judgement from here on. It also set aside 16 norms that authors had pushed back on at least once, sometimes for good reasons, and said not to enforce those mechanically at all.

Run against the last six weeks, more than half of PRs would have failed at least one check, with ~700 issues between them. 71% of those were never raised in review, by human or bot, and most of those merged without anyone mentioning them. Review raised 75% of the judgement issues, for example around metrics and concurrency, 32% of the style issues, 14% of the missing tests and 10% of the CI-level problems. Count only humans and it’s 43%, 22%, 9% and 5%.

(Some caveats. The CI checks were simulated by agents rather than actually run. The agents could see the review history, which probably flatters the judgement numbers: the checks were mined from these reviewers’ comments, so “review raised it” is partly “the check describes what the reviewer said”. A second pass re-checked every eighth PR and agreed with about nine in ten of the findings it was confident about. And only GitHub is visible, which is obviously incomplete.)

On paper, that’s the case for a gate. Write the bar down, have an AI teammate check every PR against it, and only then ask for human time.

Very nice. But what does it say about code review?

What it taught me about code review

It’s inconsistent. 92% of PRs had exactly one human reviewer (6% had none), so whatever that person missed went through. Individual reviewers raised anywhere from none to 38% of the problems in the PRs they reviewed, with a median around 20%. And there were 65 cases where someone flagged a problem on one PR, then let the same problem through on another. Reading the examples, my impression was that the miss was often a side change rather than the point of the PR. That makes sense, and with more and bigger PRs, it’s a prime reason to move this work to automated checks.

Some of that is just asking people to do a machine’s job. PR review was never the way to catch mechanical things. How do you hold test coverage in your head? That’s why teams that cared about these things always instrumented them. Looking back, readability at Google was kind of weird: a linter installed in my head. Test coverage requirements would probably have been more impactful.

The misses don’t obviously cost anything. I had Claude trace the 400+ unflagged issues forward: did a bug fix, hotfix or revert later touch the same lines? Then it compared that against other new code in the same PRs, adjusted for size. About the same. 20 of the 345 it could trace were followed by a fix, against about 17 expected. Reading the 20, only about half fixed anything the check plausibly described. The clearest cases were missing tests, which fits my hunch about coverage, but on small numbers to YMMV.

That’s a lower bound: each PR had between two and 42 days of follow-up. It also only measures bugs, not the slower cost of architectural mismatches that make the next change harder to test, or dead code someone reads around for years. But it was enough to shake my intuition that the misses mattered.

So what is review doing? Enforcing things the team hasn’t necessarily agreed to. Some of what reviewers raised was really shared: everybody flagged leftover dead and debug code. A lot of it wasn’t. Dependency injection, the architecture rule that came up most, was raised by only half the reviewers. Asking for missing tests was something seven of the twelve did over the 20 months, but in the six weeks I audited, review asked for tests only about 14% of the time, mostly humans. And in 16 places a reviewer’s norm got pushed back on, and in a few of them reviewers wanted opposite things: for instance, whether a feature flag should be read at runtime or once at launch.

These are all reasonable stances to take, and it’s possible they were situationally dependent in ways that I don’t have insight into. But from this data, and for an individual onboarding, it would be hard to distinguish a team norm from one person’s preference. It’s muddier still when, as here, some of what read as a reviewer’s judgement was actually AI output transferred via C+P.

That’s where the conflict comes from, and where bias gets in. Google studied pushback in its own code review and found the amount authors get varies with their gender, race and age, at a cost of more than 1,000 engineer hours a day, borne by engineers who aren’t white men. It is very easy to shred a PR if the person who wrote it seems like someone whose competence is in question, whether that’s down to what we politely call “unconscious bias”, because they were annoying in the meeting yesterday, or because the perception is they have handed their judgement to the machine.

So would I actually implement this?

There’s a real cost. An AI review on every PR costs tokens, and new lint rules and required tests add CI minutes to every push. The tokens are largely a more effective use of the AI review that’s already in place, at about a dollar a PR. Implementing everything would add somewhere between 6% and 38% to the CI minutes the repo already uses, depending on how carefully the expensive checks are scoped. Then there’s time. A gate adds 5 to 30 minutes before a PR goes green, a few percent of the time a PR already waits for its first human review. The bigger cost is that 55% of PRs would fail a check on their first push, so more than half would go round a fix-and-rerun cycle before a human was even asked to look. Against a bug rate that, so far, doesn’t move, that’s hard to justify on bug prevention alone. The case for it is consistency, which I think has value of its own, but has to be balanced against the cost.

When individual enforcement becomes team enforcement, the team needs to agree. The cheap checks, which are also the lowest leverage, are the ones nobody disputes. The highest-leverage ones, like dependency injection, or what counts as properly testing a feature flag, are the ones that need an expensive team discussion first. Skipping that discussion would turn one reviewer’s preference into a CI failure. But the value of that discussion could easily lower than what it would cost to have it.

The biggest value of code review isn’t in the repo. The most valuable thing review does is ask whether we’re building the right thing in the right way. Nearly half of what review raised here didn’t map to any check: product and behaviour questions specific to that PR. Ideally, those come up before review, because review is the last gate before user impact. But they’re context-specific, and the data to answer them isn’t in the codebase. What they point to is structure: who gets to decide what. What can an IC decide about their own project? What needs a broader group? Google’s readability and owner split was a version of this: style was settled once, for everyone, and ownership said whose call the rest was. Without that structure, those decisions get made in code review, by whoever happens to be reviewing.

So, would I implement it? The uncontested checks, yes: done efficiently, they add only a percent or two to CI. The rest depends on decisions the team hasn’t made yet, and depending on the team those discussions could be expensive. Right now code review is where those decisions get made, one PR at a time, by whoever is reviewing. Even if that worked, more or less, when humans wrote the PRs, I don’t think it works in an agentic world.

Read the whole story
rtreborb
8 minutes ago
reply
San Antonio, TX
Share this story
Delete

Quoting The New York Times

1 Share

Anthropic detailed the activity of its A.I. agents in a blog post on Friday, without naming the targeted websites. But two sources with knowledge of the incidents said Anthropic’s A.I. agents had submitted 20 visa applications through a form available on the State Department’s website. All the applications were incomplete and were not processed, they said.

— The New York Times, Anthropic Agents Tried to Fill Out Visa Forms on State Dept. Website

Tags: accidental-cyberattacks, anthropic, generative-ai, ai, llms

Read the whole story
rtreborb
29 minutes ago
reply
San Antonio, TX
Share this story
Delete

'Tis the Season

1 Share


Read the whole story
rtreborb
3 hours ago
reply
San Antonio, TX
Share this story
Delete

Don't You Dare

1 Share


I see his point. (via Tastefully Offensive) 
Read the whole story
rtreborb
1 day ago
reply
San Antonio, TX
Share this story
Delete

GitHub Copilot app for Beginners: How to teach Copilot about your codebase

2 Shares

Context matters when you’re working with AI. Every codebase has its own house rules: how components are structured, which scripts to run, and which conventions apply to different frameworks and file types.

Copilot can help you make changes across a project, but it works best when it knows those rules, too. Much of that knowledge can live inside the repository, so Copilot can pick it up automatically instead of requiring you to explain the same standards, scripts, and workflows every time you start a new task.

In this installment of our GitHub Copilot app for Beginners series, we’ll look at four ways to tailor how Copilot works with your project:

  • Custom instructions communicate the conventions Copilot should follow.
  • Agent skills package repeatable procedures.
  • Custom agents bring specialized guidance to larger tasks.
  • Model Context Protocol (MCP) servers connect Copilot to external tools and data sources, expanding what it can do.

Let’s look at how each one works and how they can work together.

Start with custom instructions

Suppose you ask Copilot to add a custom page size to an application. Making the change is only part of the job. The new code should also follow the patterns your team already uses.

Repository custom instructions give Copilot that baseline context. Add a .github/copilot-instructions.md file to define guidance that applies across the repository, such as your preferred architecture, naming conventions, or expectations for testing and validation.

You can also create path-specific instruction files inside .github/instructions. These files use an applyTo field to tell Copilot when the guidance is relevant.

When Copilot works on a matching file, it can use both the repository-wide instructions and the more specific guidance. That lets you keep the broad rules in one place without forcing every file type into the same mold.

Package repeatable procedures as agent skills

Custom instructions are useful for standards that should shape many tasks. But some guidance only matters when Copilot is performing a particular procedure, such as running quality checks, preparing a release, or debugging a failed workflow.

That’s where agent skills come in. A skill is a folder containing instructions and, optionally, scripts and other resources that Copilot can load when they’re relevant. Project-level skills can live in .github/skills, with each skill in its own subfolder.

For example, a .github/skills/quality-checks folder could contain a SKILL.md file that explains:

  • When Copilot should use the skill.
  • Which scripts run the project’s tests and linting.
  • What each script checks.
  • How to troubleshoot common failures.

The description in SKILL.md helps Copilot determine when the skill applies. After updating the interface, Copilot can load the quality-checks skill and follow the project’s established process for testing and linting instead of guessing which commands to run.

Use a custom agent for broader, specialized work

Some tasks are larger than a single procedure. An accessibility review, for example, may require Copilot to inspect components, templates, styles, and interactions across multiple files.

Custom agents let you create specialized versions of Copilot for work like this. A repository-level agent profile lives in .github/agents and can define the agent’s purpose, instructions, tools, and relevant project context.

Use a custom agent when a recurring type of work benefits from a dedicated role or perspective, especially when the task may span multiple files and draw on several tools or sources of context.

Connect Copilot to external tools with MCP servers

Instructions, skills, and agents help Copilot understand how to work within your project. MCP servers expand what it can access and do beyond the repository.

MCP servers connect Copilot to external tools, data sources, and services. A documentation server can give it access to framework guidance, while a server such as Playwright can let it open and interact with your application in a browser.

You can browse available MCP servers in the Copilot app or add a custom one. Once Playwright is configured, you can ask: Use the Playwright MCP server to test the functionality manually.

Copilot can then use Playwright to open the application in a browser and interact with it much as you would. That gives it a way to verify not only that the code passes automated checks, but also that the updated interface behaves as expected.

Choose the right type of customizations

These customizations solve different problems, but they become more useful when you combine them:

  • Custom instructions define the conventions Copilot should follow across the repository or for particular files.
  • Agent skills package repeatable procedures that Copilot can load when they’re relevant.
  • Custom agents provide focused expertise and guidance for broader tasks.
  • MCP servers connect Copilot to external tools, services, and data sources, expanding what Copilot can do.

Take this with you

Giving Copilot useful project context starts with four questions:

  • What project convention do I keep explaining?
  • What multi-step procedure do I repeat?
  • What complex task could benefit from a dedicated custom agent?
  • What outside tool or source does Copilot need?

Not sure where to begin? The community has shared ready-made instructions, skills, agents, and more through Awesome Copilot. Find one that addresses a workflow you already repeat, review what it contains, and adapt it to your repository’s tools and standards.

Start small: choose one convention you keep explaining, add it to .github/copilot-instructions.md, and see how Copilot applies it to your next task.

Try using the GitHub Copilot app >

The post GitHub Copilot app for Beginners: How to teach Copilot about your codebase appeared first on The GitHub Blog.

Read the whole story
rtreborb
2 days ago
reply
San Antonio, TX
Share this story
Delete

Speeding up code review with `small-change`

1 Share

Imprint’s average engineer is merging 4x more PRs than 2 years ago. Yay. Aggregated behind the average is something even more interesting: we have two teams that are merging 10x their PRs over that period. One engineer is merging 25x the PRs per week of our average engineer two years ago, and a number of others are closing in on that number. That’s pretty neat, and the slope of the line suggests our average in a year is going to look much closer to today’s p90 than today’s p50. Even more importantly, we aren’t just shipping more, our quality has improved: incidents are down 42% half-over-half, and critical incidents are down 78% half-over-half. I think we’ve always known the quality/velocity tradeoff wasn’t a particularly useful construct, but that’s undeniable today.

As we’ve walked this path, we–like everyone else progressing down it–have run into the problem of pull request review. The p50 pull request review took about 33 minutes to complete 2 years ago, but now takes us about 77 minutes. The slope here correlates tightly with the number of pull requests getting merged. Tightly enough that addressing it became one of my priorities for the week.

We solved it! Just kidding, we’ve absolutely not solved it, but I think our current approach is an interesting one, which we call the small-change.


The work discussed here was shaped and done by many folks at Imprint including Dan, Joy, Daniel, Lenny, Taylor, Ted, and Annie, among many others.

Why review pull requests?

There are four reasons we review pull requests:

  1. Knowledge sharing between author and reviewer – software engineering is generally learned via apprenticeship, and reviews are a primary mechanism of experienced engineers teaching others what good software looks like. We believe the current volume means that this knowledge sharing will have to move towards operating on clusters of pull requests, rather than individual pull requests
  2. Preventing errors – review has always been an expensive time to catch errors, and we believe that effective CICD along with harness verification techniques like poteto’s pstack are more effective mechanisms for preventing errors than review
  3. Preventing malicious behavior – my experience is that even careful code review will generally miss competent malicious behavior, although fortunately most malicious behavior is incompetent. We believe both sorts of behavior are best identified using a combination of fixed rules (modifying these specific objects/classes/files requires review), anti-abuse mechanisms that we’ve refined in our LLM-based member support experiences, auditing, and audit log inspection
  4. Compliance obligations in our documented controls – these can be changed, but today they require that an independent human reviews changes in a subset of our repositories

In the modern development paradigm, we think there is a better solution for each of these problems. But, we also want to make these changes very cautiously.

Small steps with small-change

We believe we move fast by finding narrow, contained ways to validate new practices, so to iterate our way into an effective solution, we’ve developed the concept of a small-change. Here’s how it works:

  1. When a pull request is marked ready, CICD determines whether it is eligible to be a small-change. This is represented as a passing Github check. There is also a label for visibility, but it is the check itself, managed by the build and not human modifiable, that determines eligibility. (The label is used for initial filtering, but the check is still independently verified.)
  2. Something is a small-change if it meets all of these criteria:
    1. It is less than 1,000 lines of code
    2. There are no p1 or p2 findings from our ai-code-review automated review bot
    3. The author has “experience” in the service or application, defined as 10+ prior merged pull requests into that service or application’s directories. For changes working across multiple services or applications, the author must have experience in all of them to qualify
    4. Impacts fewer than N distinct services or applications. N is 1 on our frontend monorepo, and N is 3 in our backend monorepo, where typical changes will modify both a gRPC service and a common HTTP gateway
    5. Shared libraries and utilities files are not eligible for small-change
    6. Each repository has a .small_change file which allows per-path customization while we continue to tune the behaviors. For example, a service dealing with money movement can be excluded
  3. Our Slack automation tags every reference to a pull request with reacji labeling the state of that pull request, for example if it has failing tests, if it has been approved but tests are still running, or if it is ready to merge. We added a small-change reacji that indicates which of those pull requests meet the small-change criteria
  4. In our internal skills repository, we created two skills that have been distributed across harnesses:
    1. review-small-change which takes a list of repositories and reviews all open, non-draft, small-changes, including providing approval if they meet the review’s criteria (all checks pass including small-change check, and not touching on sensitive topics such as payments and security).

      Importantly, this skill has only two options: approve or ignore. It never comments on the PR to avoid the proliferation of “meat proxy review” which has become a bottleneck-pretending-to-be-a-solution on some teams

    2. review-pr-cluster which reviews batches of pull requests to identify common themes that came up, and suggest opportunities for discussion and adding verifications

  5. We set the social expectation that any error caused by a small-change is a failure in our system, and has to be solved at the system level: improvements to ai-code-review, to our verification mechanisms, to our CICD, to our harness, etc.

This took about two days of infrequent attention to implement and rollout, although I imagine we will be tuning the knobs here for a long time. Maybe forever.

Meat proxy review

I mentioned this in 4.1 above, but meat proxy review has become an interesting bottleneck for many teams. It’s a particularly difficult bottleneck because it initially looks like a solution, but it makes things worse. The pattern is roughly:

  1. Team is overwhelmed with reviews
  2. Team decides to review pull requests with their local harness
  3. Pull requests get a large number of comments, some of which make sense, and some of which don’t. It’s unclear which is which without study, and you don’t know which comments reflect the reviewer’s point of view. So you end up having to do substantial work to disprove comments that the reviewer may not even agree with
  4. Those teams then start using harnesses to respond to the harness-generated comments, making the entire review process an illusion that doesn’t facilitate learning, and most importantly could be done without the involvement of the other team members

In every case that meat proxy review is performed, the team would get better outcomes by investing into the centralized review mechanism, the harness’ loop for addressing the centralized review’s feedback, and simply stamping approval.

fleet: centrally orchestrated harness

I’ll briefly mention our other approach to reducing code review volume, which anchors on our centrally-orchestrated harness, fleet. (Which is similar to Stripe’s Minions although not following the one-shot approach in particular). We rely on fleet to implement and verify software in a highly controlled harness running in the cloud, with independent evaluation mechanisms to identify the correct number of independent reviewers for any given change. That number can be as low as one (e.g. fleet writes, one human reviews) for changes evaluated as safe.

Results / next steps / reception

I’ll be honest, it’s too early to give a useful result here. From early results, about 10% of changes qualify as a small-change. Those are now eligible for very fast approval, and generally get it unless they trip a flag, generally due to touching a security or money adjacent topic.

While 10% is a small percentage, the core concept here is working, and ultimately will succeed or fail based on our ability to expand that percentage over the next few months in tandem with expanding our verification mechanisms to be increasingly comprehensive. This is ultimately going to fail or succeed depending on our ability to improve our verifications, which does require more difficult sorts of techniques to accomplish in low scale-complexity, high domain-complexity software than in the high scale-complexity, low domain-complexity products where many techniques like incremental rollouts are exceptionally effective. (Don’t get me wrong, they still work, they just provide less coverage when most users have different configuration and behavior.)

Folks have not been uniformly excited about this, it’s a substantial change from how we’ve learned to be good engineers, and it’s uncomfortable to reevaluate whether a long-held best practice has become merely dogma. The most energizing part of the current cycle is that we get to reevaluate these practices we grew up on every week; the most painful part of the current cycle is that we’re likely to fall behind if we ever stop reevaluating them.

Read the whole story
rtreborb
6 days ago
reply
San Antonio, TX
Share this story
Delete
Next Page of Stories