Software engineering · Reference design
Code review matched to change risk
Two comments about naming. None about the default that flipped to allow.
Decide review depth from the change's risk before any model looks at it, make some floors impossible to lower, and never post a finding on a commit that has moved.
Company Quillstack is a hypothetical company. Every figure is illustrative.
About Company Quillstack
Company Quillstack is a hypothetical company, written for this reference design; the details below are its constraints. Every figure is illustrative, computed from stated assumptions. No client work or measured result is claimed.
- Industry
- API platform for generating and signing business documents
- Size
- About 90 engineers; a Python and TypeScript monorepo plus two Go services
- Systems
- A hosted code platform with pull requests, CI and CODEOWNERS; a trialled AI review bot
- Volume
- About 60 pull requests a day, a third of them dependency bumps, generated code or docs
- Constraints
- Zero-retention model provider; signing-key and billing code never goes to an external model; merge stays with code owners
In brief
How the work ran before
Company Quillstack has 90 engineers and sells an API for generating and signing business documents. Most code lives in one repository: Python services, a TypeScript front end, and two Go services. About 60 pull requests land each day. Roughly a third are dependency bumps, regenerated API clients or documentation.
For three months Quillstack trialled an off-the-shelf AI review bot. It reviewed every pull request with the same prompt and left comments on style, naming, possible null values and missing docstrings. Early on people read them. By the second month, most engineers resolved the bot's threads without opening them.
Where it broke
One pull request is titled "Simplify permission helper". It refactors the function that decides whether a user may access a document. The old version looked up a grant and denied access when none existed. The new version looks up a grant, denies access only if the grant exists and is marked denied, and otherwise allows.
- if not policy.lookup(user, resource):
- return DENY
- return ALLOW
+ grant = policy_store.find(user, resource)
+ if grant and grant.denied:
+ return DENY
+ return ALLOW
Constructed example.
For a user and document with no grant at all, the old code denied and the new code allows. The bot left two comments about variable naming. A human reviewer, seeing a small refactor, approved it. It reached production and was caught a week later in a routine access review.
The bot didn't fail because it was a bad model. It failed because it spent the same attention on every change. When everything gets a review, nothing gets a careful one, and people stop reading.
What I would build
Decide how much review a change needs before any model looks at it, and make some of those decisions impossible to lower.
- Route by risk. Code reads the diff and classifies it: which paths changed, which owners those paths have, whether it touches authorization, migrations, dependencies, secrets handling or restricted directories.
- Apply floors. Some routes are fixed by policy. Anything touching authorization or migrations gets a deep review, required owners and a required negative test. A model can raise a route; it can never lower a floor.
- Review at the right depth. A README typo gets no model review at all. Ordinary application code gets a standard pass with the changed functions and their callers. A deep review gets more context: callers, tests, the policy docs, and a checklist specific to the risk.
- Tie findings to a commit. Every finding records the exact commit it was made on. If the branch moves before the finding is posted, it is withheld and the review reruns.
- Leave merging to people. Findings are advice. CODEOWNERS and branch protection decide what merges.
Restricted code, like the signing-key handling, never goes to an external model. It routes to human-only review.
Following one change through
The permission refactor, with routing. Constructed example.
| Signal | Source | Effect |
|---|---|---|
Path auth/permissions.py |
Path rules | Authorization floor: deep route |
Function can_access changed |
Symbol index | Callers collected: 14 call sites |
| No test file changed | Diff | Required negative test missing |
Owner group @platform-security |
CODEOWNERS | Required reviewer added |
The deep review receives the old and new function, the callers, and a checklist item that the standard prompt never had: for every input where the old code denied, does the new code still deny? It posts one finding: "For a user/resource pair with no grant, the old code returned DENY; the new code returns ALLOW. Add a test for an unknown pair." The pull request can't merge without the security owner's approval, and the route requires a test like this:
def test_unknown_pair_is_denied():
assert can_access(unknown_user, unknown_resource) == DENY
Constructed example.
Meanwhile, the 20 dependency bumps and README fixes that day got either no model review or a lightweight check. The deep review had room to be deep.
auth/permissions.py
deep route
high
can_access
authorization floor
For a user/resource pair with no grant, the old code returned DENY; the new code returns ALLOW.
Add a test for an unknown pair before merging.
- evidence changed lines 12 to 16 · 14 callers collected
- required negative test · owner review by @platform-security
- commit analyzed head equals current head · posted
If the branch moves before posting, this finding is withheld and the review reruns.
The finding is advice. Merging stays with the code owners.
When things go wrong
The branch moves during review. The finding was made on commit a1b2c3; the head is now d4e5f6. The finding is withheld, not posted on lines that no longer mean the same thing. The review reruns on the new head.
The router misses something. Path rules are simple and can be fooled: authorization logic in an unexpected file. That's why the model is allowed to raise a route when it sees authorization-like code in a standard review. It can't lower one.
A dependency bump is not trivial. Lockfile changes can pull in a new transitive dependency or a major version. They route to a dependency check (license, known vulnerabilities, major-version changes), not to "trivial".
The model is down or slow. The pull request isn't blocked by the bot. Required human reviews still apply. The missing model review is shown as missing.
What changes for the team
| Change type | Before | After |
|---|---|---|
| Docs, typos | Bot comments on style | No model review |
| Dependency bumps | Bot comments on lockfile lines | Dependency policy check |
| Ordinary code | Generic review | Standard review with callers |
| Authorization, migrations | Same generic review | Deep review, required owners, required negative test |
| Restricted directories | Sent to the bot | Human-only, never sent out |
Illustrative figures from assumptions in the technical design. Not measured.
| Illustrative day (60 pull requests) | Same review for all | Risk-routed |
|---|---|---|
| Pull requests reviewed by a model | 60 | about 38 |
| Deep reviews | 0 | about 6 |
| Model comments posted | about 240 | about 70 |
| Comments on stale commits | some, not tracked | 0 by construction |
Fewer comments is the goal, not a side effect. A comment that people read beats four they resolve unread.
What this does not solve
It doesn't replace the human reviewer's judgement on design. Path-based floors are only as good as the path map, which needs an owner. And deep reviews cost more per pull request, so a team that routes too conservatively will pay for it in money and waiting time.
How I would prove it
Collect 200 merged pull requests from the last quarter, including every one that later caused an incident or a revert. Run the router and check two things. First, did every high-impact change get a deep route? That's the number that matters, and it must be all of them. Second, how many low-impact changes were routed deep anyway? That's the cost. Then run the deep review on the high-impact set and have the owning engineers label findings as useful, wrong or noise.
The technical design
The same system for engineers: architecture, records, failure handling, evaluation, the options I rejected, and the stack. About 4 minutes.
AI review nobody reads?
Bring a month of pull requests.
Titles, paths and outcomes for a month of pull requests are enough to test whether risk routing would have caught what mattered.