Use when reviewing changes since a fixed point, or when a non-trivial in-flight decision needs adversarial scrutiny — keep Standards and Spec separate, measure relevant complexity, and use the bounded doubt cycle (claim → extract → fresh-context adversarial review → reconcile → stop).
Use when reviewing changes since a fixed point, or when a non-trivial in-flight decision needs adversarial scrutiny — keep Standards and Spec separate, measure relevant complexity, and use the bounded doubt cycle (claim → extract → fresh-context adversarial review → reconcile → stop).
Code Review
What a review looks for and how findings are handled — for human and agent reviewers
alike. Adapted from Matt Pocock's code-review and Addy Osmani's
doubt-driven-development, with the complexity pass informed by Saurabh Kumar's
cyclomatic-complexity skill. The local entrypoint keeps the two-axis review model and
adds complexity as a measured maintainability signal.
Areas under consideration
Review structure — the two-axis model
Baseline smell taxonomy
Review scope — diff against a pinned fixed point
In-flight adversarial review — the doubt cycle
Complexity measurement and behavior-preserving refactoring
Priority order — correctness > security > simplicity > style? The user's ranking
Severity taxonomy — what blocks merge vs what's a suggestion
Self-review checklist — what's checked before requesting review
How /code-review passes fit in, at what effort
Skill
Review along two independent axes
A change can pass one axis and fail the other — code that follows every standard but
implements the wrong thing, or does exactly what the issue asked while breaking
conventions. Review (does the code conform to the repo's documented
standards?) and (does the diff faithfully implement the originating issue/PRD/spec?)
separately — as parallel sub-agents when available so they do not pollute each other's
context — and report them under separate headings. Never merge or rerank findings across
axes. End with findings-per-axis counts and the worst issue within each axis.
Standards
Spec
Pin the fixed point first
The review target is git diff <fixed-point>...HEAD (three-dot — against the merge-base),
plus the commit list from git log <fixed-point>..HEAD --oneline. If the user did not name
a fixed point, ask. Confirm the ref resolves and the diff is non-empty before doing any
review work.
Find the spec source
In order: issue references in commit messages (#123, Closes #45) → a path the user
passed → a PRD/spec file under docs/, specs/, or scratch dirs matching the branch →
ask the user. If there is no spec, skip the Spec axis and say so in the report. The Spec
axis reports: (a) requirements missing or partial, (b) behavior not asked for (scope creep),
(c) requirements implemented but implemented wrong — quote the spec line for each finding.
The Standards axis: repo docs plus the smell baseline
Use whatever the repo documents (CODING_STANDARDS.md, CONTRIBUTING.md, CLAUDE.md) — plus
this fixed baseline of Fowler smells (Refactoring, ch. 3), which applies even when the
repo documents nothing. The repo overrides the baseline. Every baseline smell is a
judgment call, never a hard violation. Skip anything tooling already enforces. Each smell
reads what it is → how to fix:
Mysterious Name — a name does not reveal what it does or holds → rename it; if no
honest name comes, the design is unclear.
Duplicated Code — the same logic shape appears in multiple hunks or files → extract
the shared shape and call it from both.
Feature Envy — a method reaches into another object's data more than its own → move
it onto the data it envies.
Data Clumps — the same few fields or parameters travel together → bundle them into
one type.
Primitive Obsession — a primitive stands in for a domain concept → give the concept
a type.
Repeated Switches — the same switch or conditional cascade on the same type recurs
→ replace it with polymorphism or one shared map.
Shotgun Surgery — one logical change forces scattered edits → gather the change into
one module.
Divergent Change — one module is edited for unrelated reasons → split it so each part
changes for one reason.
Speculative Generality — hooks or abstractions serve no current requirement → inline
them until a real need appears.
Message Chains — long navigation such as a.b().c().d() leaks an object graph → hide
the walk behind a method.
Middle Man — a thing mostly delegates onward → cut it and call the target directly.
Refused Bequest — an implementer ignores most inherited behavior → use composition.
Distinguish hard violations (documented-standard breaches can be) from judgment calls
(baseline smells always are), citing the standard or naming the smell and quoting the hunk.
Measure complexity before judging it
Cyclomatic complexity is a count of independent decision paths: decision points + 1.
Count the constructs recognized by the project's analyzer, commonly conditionals, case
arms, loops, exception handlers, conditional expressions, and logical operators in
conditions. Counting rules differ by tool, so report the tool and rule when one exists.
Project configuration wins. Read the repository's linter, analyzer, or quality gate first.
Use the analyzer already present when possible; common choices include:
Python: radon cc -s -a <path>
JavaScript or TypeScript: the ESLint complexity rule
Go: gocyclo
Polyglot repositories: lizard <path>
If no analyzer is available, count manually per touched function and show the count. When
the project has no threshold, use these as triage bands rather than universal merge laws:
1–5: usually leave it alone.
6–10: inspect it, especially if the function is already being changed.
11–15: refactor when the branches obscure the behavior or the change adds more paths.
Above 15: split or document the reason to keep it before accepting more behavior.
A number alone is not a finding. Report complexity when it makes behavior harder to read,
test, review, or change, or when it violates a project threshold. Do not demand unrelated
cleanup in untouched code.
Refactor complexity without hiding it
When the diff changes a complex function, measure all touched functions, rank the hotspots,
and review the worst one first. Prefer these tactics in order:
Guard clauses — return early to remove unnecessary nesting.
Extract functions — give each extracted behavior a name that says what it does.
Lookup tables or maps — replace repeated selection branches when the variation is data.
Named predicates — replace a dense boolean expression with a domain-level question.
Polymorphism or a strategy — use it for a recurring type-based variation, not a single
switch that is still clear.
Flatten loops — extract the loop body or continue early instead of nesting conditions.
Preserve behavior and public interfaces. Run the relevant tests before and after when they
exist; when they do not, say so and refactor conservatively. Move complexity into small,
well-named units rather than clever expressions, compressed one-liners, or opaque helpers.
If a function's name needs “and,” examine whether it has more than one responsibility.
Every complexity refactor ends with a compact before/after report:
## Complexity report
| Function | Before | After |
|----------|--------|-------|
| parseOrder | 14 | 4 |
Extracted: validateHeader, resolveDiscount
Behavior verified: <tests or other evidence>
Doubt-driven review: in-flight, not just post-hoc
The two-axis review above is a verdict on a finished diff; doubt-driven review applies the
same adversarial energy while course-correction is still cheap. A confident answer is not a
correct one.
Apply it to non-trivial decisions only: new or changed branching logic, a module or
service boundary, properties the compiler cannot verify (thread safety, idempotence,
ordering), correctness that depends on invisible context, or an irreversible blast radius.
Do not use it for renames, formatting, or clear instructions.
The cycle is bounded to three rounds:
Claim — name the decision in two or three lines and say why it matters. If it cannot
be stated that compactly, the decision is not clear yet.
Extract — isolate the smallest reviewable artifact and its contract. Strip the journey
and conclusions from what the reviewer receives.
Doubt — use a fresh-context reviewer with an explicitly adversarial prompt: find what
is wrong, assume the author is overconfident, and look for unstated assumptions, edge
cases, hidden coupling, and contract violations. Pass the artifact and contract, not the
claim.
Reconcile — classify each finding as a contract misread, valid and actionable,
valid trade-off, or noise. Fix actionable findings and repeat.
Stop — stop when findings become trivial, after three rounds, or when the user says
to ship. Three unresolved rounds mean the artifact should be decomposed or escalated.
Watch for doubt theater: repeated rounds with substantive findings and no actionable change
mean the review is validating instead of doubting.
