AI is increasing the pace of development, especially as more and more code is written with Spec Driven Development like spec-kit, OpenSpec or the AI-DLC. I'm not going to talk about these techniques, there is so much written on these already, what i do want to look at what happens downstream from AI writing your code, especially code review, otherwise known as PR review.
Why Code review?
First, I want to look at why we do code reviews in the first place, I'm looking at this list from a pre-AI era before we dig into consequences and differences that AI introduces.
You can think of this in four areas:
Shared ownership
Rarely is code owned by one person, especially in an enterprise, the PR process promotes a shared code ownership model, many people can pick up and work on your applications. If there is a bug there is a pool of people who can pitch in and help debug or analyse the logs.
Feedback to the author, training and learning, mentoring
Most teams consist of a mix of junior and senior engineers, it's imperative that the seniors help to impart their knowledge around the team, this can be slow at times but in the medium to long term yields great benefit as juniors become more proficient, they are your future senior engineers.
Is it understandable
This is very closely linked to shared ownership, we need to make sure new code is easily understood by the entire team, if complicated esoteric logic and patterns are used the more junior engineers will struggle to contribute, it's important to remember code is traditionally written once and read many times by many different people with different skills and experience.
- Ensure the code is understandable
- Ensure low complexity
- Remove un-necessary complexity
- Check for over engineering
- No code duplication
- Check against common code smells
- Ensure the code is required
Alignment to standards
Often an overlooked aspect to a code review, it's easy to spot if code conventions are violated (spaces vs tabs, naming conventions) but it's harder to spot if the code is drifting away from the original design, (who has not inserted a function when an object method would have better matched the intended design).
- Does the new code conform to our coding standards and conventions
- Does it align with our patterns
- Does the solution still align with the original/existing/intended design and architecture
Is it correct
Sometimes quoted as the main reason for doing a code review, but with automated tools has become less the focus. In my opinion it’s the job of the author to ensure they have tested their code locally, run the linters and tests, read and re-read the stories/specs and thought about edge cases, in short it’s the authors job to deliver correct working code for review, it's not the reviewers job to validate the code works.
Having said that a senior engineer will be better at validating certain things than juniors and so the question of "is it correct" is still valid and should form part of the review. Specific things to look for include:
- Does it meet the spec
- Are there any unintended bugs and defects
- Is there any inconsistent or missing error handling
- Are the usages of APIs and packages correct
- Edge cases
- Security
- Are all packages secure
- Is there sufficient validation checks
- Anything daft like SQL injection or methods with missing auth
- Tests
- Test coverage
- Correct assert statements
Add to this axis is the benefit that different engineers have different ideas, different perspectives about what the code 'should' do, everyone has different understanding of the users, what they are wanting to achieve and the use cases, rarely is the spec as given by the user or product owner fully fleshed out and containing all the assumptions that are in their heads, history of past decisions often lives in the heads of the engineers who have been on the project for the longest time.
What makes for a good PR
Having looked at why we traditionally perform a code review let's have a look at what makes for a good PR. These properties help to achieve the goals listed above.
- Focused on one small story or requirement
- Atomic, all relevant changes including tests are included in one PR
- Limited refactoring, if you are refactoring and adding new features at the same time it is hard to understand which parts of the change set is focused on refactoring and which is focused on new functionality
- Small change set
- Lines of code changed (<200, 400 max), we will get to this in a moment
- Number of files changed
- Good name and a detailed description
- Links back to a story tracking the issue
- The branch has a good relevant name
The small change set is quite key, everyone who has been involved in code review before knows that the larger the review, the longer it will wait in a queue to be reviewed, and if it is a large PR there is a tendency to miss the wood for the trees, comments about spelling and style can be easier to spot than flaws in logic or missing edge cases. This leads to a LGTM comment.
There was a case study of code reviews at Cisco in 2006 which found that the longer the code review the less effective it was, especially when the size of the review passed 200 to 400 lines of code the number of defects found per line of code reduced dramatically. A similar study by Microsoft in 2015 found similar conclusions, the larger the change set the less effective the review.
Basically, if the reviewer is overwhelmed by a large change set, they cannot give each line of code the same attention to detail as if it was a small change set, so you are bound to miss things the larger it is.
Review overload
Just as a large change set is overwhelming, so is a large number of PRs. AI is making the size AND volume of PRs a problem, ultimately this causes what is commonly referred to as review theatre, and the liberal use of LGTM (Looks Good To Me) often signifying that the PR was too big, too complex or the reviewer didn’t have time to properly look into it.
AI makes it possible to generate multiple PRs per developer per day, especially when the devs run parallel agents on different stories. So, on a team of 4 (two juniors, a mid-level and one senior) the three engineers will easily produce enough work to keep the senior 100% full time reviewing PRs, if they can keep up at all, no one wants that.
SDD
Spec-driven development with agentic coding agents makes this problem worse, in my experience work performed with an agent using this technique results in larger PRs which are also more complex due to the fact that the code was not written for human readability or with any complexity guardrails. That’s not to say SDD is bad, it is just making the review process harder than it already was.
Why code review in an age of AI?
Look at the original question, "why code review", If AI is now writing the code, what are we trying to achieve.
Shared ownership
When people don’t write the code, is there any point in trying to keep track of the code, to understand what areas of the code do what and how, we are just asking agents to look at the current code base and generate the next iteration of the code, often re-writing swathes of the code base at the same time, I would argue that engineers don’t own the code anymore, they own the application and the spec.
Mentoring and feedback, is also debateable, agents don’t learn from your feedback, you are not mentoring anyone when you give comments on a PR that an agent will then address, there is no learning for engineers from a human review.
I believe a better approach is to use the AI to help you understand areas of a codebase when it’s needed. Ask the agent for an overview, for how the architecture works. The codebases are getting too big to keep in your head anyway, and review is not the best place to do this. I think a continuous code review style achieves the goal of shared ownership better, on a cadence the team does a mob review of significant changes that have occurred since the last review, it’s more of a show and tell, the author can decide what constitutes a significant change.
Is it understandable
Does it matter 🤷
If engineers are not making the edits, are not reading the code, does it matter if its overly complex, with duplication, full of code smells 🤷
OK, I'm being slightly facetious, but you get the point, I would argue that even agents would benefit from simple over complex code, that code duplication would be better off not being present, but this can all be achieved via agent instruction and tooling, measure cyclomatic complexity and code duplication via tooling, you can automate this away, at the very least tools that flag issues and allow the author to make informed decisions regarding do they fix the issues or not.
So, yes, the code should be understandable, but this is not “enforced” by code review, we use CI, linters, static analysis, and review agents to get this job done. Ultimately, it’s the authors responsibility to ensure the code produced is good quality, if they are junior or unsure of a certain refactor ask the AI, failing that ask for a desk review before pushing the changes.
Alignment to standards
This forms a very similar argument to "is it understandable", and again I would lean on tooling and agentic review.
It’s also worth saying that if humans are not writing code, we need to ensure that the agents know what our standards are, you can’t just trust them to follow the patterns in the codebase. Historically the senior developers would know all this in their heads, institutional knowledge that is passed from dev to dev often via code review and design sessions, in my general experience this was never written down, and kept up to date if it was. The agents will need proper architecture and design guides, spend time upfront and ensure that these guides are available to the agents when they need to pull it into context.
Is it correct
Does the change achieve the goal as laid out in the spec or story? As stated before, I don’t believe this was really the purpose for a code review. This is why we have a comprehensive test suite and QA engineers who perform exploratory testing on the new feature.
The author is the human responsible for the code they ship, that is not to say they need to have read it all, but they need to have confidence that it does what it is supposed to, it has good test coverage, the tests are valid and have coverage of edge cases and assert statements, yes code coverage reports don’t indicate adequate testing its just another indicator/metric. As part of asking an agent to create code is to ensure it is following a good spec or story, if anything it is this that needs a human to look over, is the spec good, is it complete, are edge cases covered. But again, you can use the AI to review your spec before any work is carried out, get the agent to grill you on design choices and missing assumptions.
Why code review at all?
I feel that the time for human code reviews is coming to an end, a combination of modern tooling plus agentic review can cover all the reasons that we once performed code reviews. But you can’t just drop them if you have not properly thought about it and have something in place that covers all the reasons that code review exists pre AI.
- A shared definition of what good looks like
- A team process to review specs and stories before implementation
- Code author willing and empowered to take responsibility for the full lifecycle
- It’s the code authors job to ensure the code created meets the spec, and technical requirements defined by the team
- Good tooling to check for regression, security, coding standards, and practices
- Linting
- Static analysis
- Dependency checking
- All tooling can run on a dev machine before pushing
- Tooling automation is fast, so devs are willing and able to run often, especially before commit
- Full test coverage
- Good code coverage > 95 %
- Edge cases are covered
- Failure cases are covered
- All tests have assert statements that are valid and make the test meaningful
- Tests run quickly < 1-2 minutes
- Specialist agents which can validate:
- The code meets the spec or story
- The code is secure
- CICD system
- All changes trigger a build test cycle (same checks as in 4.)
- Engineers stop work when the build is red, this is always the priority
- Continuous delivery allows the QA to deploy the latest release candidate to pre-prod environments for testing
- Promotion to live can be either a manual gate or automated if integration tests pass in a preprod environment
- Deployments need to be fully automated, no manual steps other than a button to deploy
- Have quick and easy rollback mechanisms
- Have quick and easy fix forward workflows
- Desk review process for tricky situations, and a team who is responsive to lend time to help at a short notice
- Rolling code/design/architecture reviews by team members so we all keep abreast of changes
Rolling code/design/architecture reviews
If code reviews are no longer the gate to pushing to trunk/main line, we do need some other way to align the team. In a rolling review time is set aside weekly for the team to get together and review code and design changes, this can be daily, weekly or some time in between, remember, everyone is getting time back since there are no code reviews now so this time for the rolling review is not extra time that engineers need to spend on yet another meeting, this is core to maintaining an understanding of the project.
The purpose of this meeting is not to go through every change but just the critical ones. Things that change core concepts in the code, new data structures, changes to automation. It’s up to the engineers to define what to bring to this meeting, additionally you can ask an agent to look at all the changes done over the past x days and recommend areas for the team to be notified of.
This is a good time to review the direction of the code and to look at if any refactoring is needed to keep the code healthy, this is something that again an agent can help with.
Trunk based development
No human code review does not automatically mean no PR, although I would argue its heading that way. In a nutshell trunk-based development is the idea that everyone pushes to main multiple times per day, that every commit is a potential release candidate, that we are baking in all our checks in to the developers workflow and the CI system runs against every commit.
We don’t have to jump straight to trunk-based development, besides, not every codebase is suitable for that technique (discussed in the next section). It’s possible to keep the feature branch approach and progressively apply the principles of the 10 steps, when the PR happens and results in a green build the PR is approved with a LGTM comment, the code has already been reviewed by the author. If the author thinks a human review would be useful for this change, they can request it in the description/comments along with the area that they want someone to focus on, but I would argue that a desk review would be better, performed at the time of development rather than as a gate at the end.
So, why code review again?
There are times when a code review gate is still a good idea, often based on risk; is the code particularly important or critical, do you trust the code authors, does the code have inherent security related concerns (secrets and certificate infrastructure), IaC that changes infrastructure directly, IAM, automation code that is shared across many teams or codebases, shared libraries (code with a large blast radius if there are issues).
Who is committing, is the project or code base managed like open-source software with a core group of trusted owners and many engineers outside that team submitting changes.
My honest take
I still have cognitive dissonance (holding multiple conflicting views or beliefs at the same time).
- I believe that to have a good quality codebase you need humans to understand it, to help grow it, to guide its direction
- I believe that AI agents are now good enough to write good quality code and can create much more code in an hour than a human could review in a day, or two!
I've been in this business for over 25 years, I’ve seen many trends in my time, I started out in a classic waterfall SDLC environment, worked with many flavors of agile from scrum to XP, I’ve worked at large enterprises on critical systems, at small startups where every line goes straight to prod and many places in between. I’ve done years of dedicated pair programming working with trunk-based development, and with teams where individuals spent weeks/months! on a feature branch that results in a PR for 2 engineers to review as a gate to main line. I’ve worked on large mono repo codebases and microservices where each tiny service is a separate repo. I've seen codebases with full automated unit/system/integration tests, where load and performance tests run on every commit, and code bases that have none.
I find the current push to Spec Driven Development (SDD) quite interesting it feels like I’m being taken back in time whist using the most cutting-edge tools we have. I started my career with waterfall development, and I’ve had plenty of experience with cucumber and Behaviour Driven Development (BDD) from 15 years ago, it feels like nothing we are discussing here is new other than the speed of creating/changing code. There is no silver bullet, there was not when Fred Brooks wrote about this in the 70’s and there is still not today, let’s not pretend there is. Every enterprise/team/project/codebase/engineer is different, and we need to take this in context when thinking about how to work in this new world.
Links:
https://martinfowler.com/articles/exploring-gen-ai/sdd-3-tools.html
https://github.github.com/spec-kit/
https://awslabs.github.io/aidlc-workflows/
https://static0.smartbear.co/support/media/resources/cc/book/code-review-cisco-case-study.pdf
https://martinfowler.com/articles/branching-patterns.html#mainline-integration
https://en.wikipedia.org/wiki/No_Silver_Bullet
Disclaimer: AI was not used in the generation of this post, all words, expressions and ideas came from a human brain. Does that matter? Maybe not, it’s a personal matter, given the prevalence of AI usage I wanted to make sure I'm engaging my brain and making the effort to keep my ability to think and reason.
No comments:
Post a Comment