Aio Code Review

Code Review

A CL (changelist) is one self-contained change under review — equivalent to a PR / MR. LGTM = "Looks Good to Me" (reviewer's approval). Nit: = optional polish, won't block.

This skill does two things:

• Runs a review — authors and runs an adaptive review workflow over a diff/PR (the default when there's code in front of you). See Run a review below.
• Coaches a role — the reviewer / author playbook. It's both the rubric the workflow's agents apply and your reference for reviewing by hand.

Full principles and rationale: references/01-overview.md.

Pick the lane first

Run a review — the workflow

When there's actual code to review, don't eyeball it — run the adaptive review workflow. It profiles the project, reviews on two independent tracks, then kills its own false positives before giving a verdict:

1. Profile (3 agents in parallel) — understand the project · detect the lint/type-check/test tooling it actually configures · extract the house conventions and design patterns. These are independent discovery tasks, so they fan out, not run in sequence.
2. Mechanical — run the tools the profile found (linters, type-checkers, tests) and parse their output into findings scoped to the diff.
3. Semantic — a multi-lens sweep applying the 8-dimension rubric below plus the discovered house conventions, so the review enforces this codebase's style, not generic advice.
4. Consolidate — dedup both tracks, adversarially verify each finding against the real code (linters and LLMs both emit noise — a false positive erodes trust more than a missed nit), severity-rank, and synthesize an LGTM verdict against The Standard.

To run it:

1. Scope the diff inline first — git diff --name-only <base> for the changed files, note the base ref (working tree vs HEAD, origin/main...HEAD, or a PR ref). For a GitHub PR, gh pr diff <n>.
2. Pick a tier from the user's signal — lean (quick spot-check), standard (default), maximal ("thorough / audit / spare no cost"). Tier scales lenses, semantic rounds, and verify votes.
3. Author + run the Workflow using the script in references/review-workflow.md, passing args = { scope, base, files, tier }. Pass the script inline; adapt the knobs to the detected project.
4. Present the consolidated report in the eng-practices vocabulary — verdict, blocking items vs Nit:s, what's good, each comment kind and with a concrete fix.

Skip the workflow for a trivial one-file diff — a single reviewer agent (or reviewing it yourself against the checklist below) is enough; the multi-agent machinery is dead weight on a change with one obvious answer.

The full harness, its schemas, and the tuning knobs live in references/review-workflow.md. Workflow mechanics (pure-literal meta, barrier vs pipeline, budget guard, cache-safe resume) live in the aio-workflow-creator skill.

The Standard (the senior principle — memorize)

Approve a CL once it definitely improves the overall code health of the system, even if the CL isn't perfect.

There is no "perfect" code, only better code. Seek continuous improvement, not polish. Never approve a CL that worsens code health — only emergencies justify that.

Underlying principles:

• Technical facts and data overrule opinions and personal preferences.
• The style guide is the absolute authority on style.
• Software design is almost never pure preference — weigh on engineering principles, not vibes.
• When no other rule applies, prefer consistency with surrounding code.

Deep dive: references/reviewer-01-standard.md.

Reviewer — every review touches 8 things

These eight dimensions are the rubric the workflow's Semantic phase applies (one lens per concern), and your checklist when reviewing by hand.

1. Design — does the change belong here? Does it fit the system?
2. Functionality — does it do what was intended, and is that good for users? Think about edge cases, concurrency, UI impact.
3. Complexity — could it be simpler? Watch for over-engineering (solving problems the dev speculates might appear later).
4. Tests — present in the same CL (unless emergency), correct, useful, not over-complex. Tests are code; they must be maintainable too.
5. Naming — long enough to communicate, short enough to read.
6. Comments — explain why, not what. Different from documentation (which describes purpose/usage).
7. Style — the style guide is the absolute authority. Personal preferences → prefix Nit:. Style-only changes belong in a separate CL.
8. Documentation — READMEs and generated reference docs updated when behavior changes.

Plus three discipline points: read every line assigned to you (with documented exceptions for multi-reviewer CLs); look at the broader context of the system; call out good things, not only defects — reinforcement teaches as much as critique.

Deep dive: references/reviewer-02-what-to-look-for.md.

Reviewer — sequence (don't waste your own time)

1. Broad view — does the change make sense at all? If not, say so courteously and immediately; suggest what the author should have done instead. Don't waste their next-CL effort by staying silent.
2. Main parts first — find the file(s) carrying the major logic; review those first. Send any major design comments immediately — much of the rest may disappear once the design changes.
3. The rest — walk remaining files in a logical order (often tool order, or tests-then-code).

Deep dive: references/reviewer-03-navigating-cl.md.

Reviewer — speed

• Respond within one business day when not in focused work. Team velocity > individual velocity.
• Don't interrupt your own deep work. Wait for a natural break — getting back into flow costs more than the author's wait.
• LGTM-with-comments to unblock when remaining items are trusted to the author or are clearly optional — especially across time zones.
• Large CL → ask to split before reviewing. If it truly can't be split, at least send broad design comments now so the author isn't blocked.

Deep dive: references/reviewer-04-speed.md.

Reviewer — writing comments

• Be kind. Comment on the code, not the developer.
  • Bad: "Why did you use threads here when there's obviously no benefit…"
  • Good: "The concurrency model adds complexity without a performance benefit that I can see. Best to keep this single-threaded."
• Explain why — your intent, the principle, how the suggestion improves code health. Reasoning helps the author internalize, not just patch.
• Label severity so the author can prioritize:
  • Nit: — minor; technically you should fix it but it won't move the needle.
  • Optional: / Consider: — good idea, not required.
  • FYI: — future thought, not for this CL.
• If the author has to explain code in the review tool, ask them to fix the code or add an in-code comment. Review-tool explanations don't help future readers; the code does.

Deep dive: references/reviewer-05-writing-comments.md.

Reviewer — handling pushback

• First, ask if they're right. They're often closer to the code than you are. If their point holds up on code-health grounds, drop the issue and tell them so.
• If they're not, explain again — acknowledge what they said, restate your reasoning, stay polite. Improving code health happens in small steps; a few rounds is fine.
• Don't accept "I'll clean it up later." It almost never happens — the dev moves on, the issue gets lost. Insist on cleanup now. If it truly must wait, file a bug, assign it to the author, and add a TODO referencing the bug.
• Persistent conflict → escalate (TL, maintainer, eng manager). Don't let the CL rot because two people can't agree.

Deep dive: references/reviewer-06-handling-pushback.md.

Author — writing the description

Every description must answer what changed and why.

• First line: short, focused, imperative — "Delete the FizzBuzz RPC and replace it with the new system", not "Deleting…". This line stands alone in version-control history summaries.
• Body: the problem being solved, the chosen approach, known shortcomings, links to bugs / benchmarks / design docs. Inline enough context that future link-rot doesn't destroy the meaning.
• "Fix bug." / "Phase 1." / "Add patch." are not descriptions.
• Re-read before submitting — the CL may have changed significantly during review.

Deep dive: references/author-01-cl-descriptions.md.

Author — keep CLs small

One self-contained change. ~100 lines is usually fine; ~1000 is usually too large; file count also matters (200 lines spread across 50 files is often too much). Small CLs review faster, review more thoroughly, introduce fewer bugs, are easier to merge and roll back, and waste less work when rejected.

Splitting strategies:

• Stack — write a small CL, send for review, immediately start the next based on it.
• By files — proto change separate from code change; code separate from config.
• Horizontal — split by stack layer (client / API / service / model) using shared stubs.
• Vertical — split by feature (multiplication / division) as parallel tracks.

Rules:

• Refactorings go in separate CLs from feature/bug changes.
• Tests stay in the same CL as the behavior change. Independent test additions/refactors can stand alone.
• Don't break the build between stacked CLs.

Deep dive: references/author-02-small-cls.md.

Author — handling reviewer comments

• Don't take it personally. The goal is the codebase, not you. Never reply in anger — walk away if needed.
• First question to yourself: do I understand what the reviewer is asking? If not, ask for clarification before reacting.
• If the reviewer didn't understand the code → fix the code (or add an in-code comment). Don't explain only in the review tool — future readers won't see that.
• Disagree collaboratively, never combatively:
  • Bad: "No, I'm not going to do that."
  • Good: "I went with X because of [pros/cons] with [these tradeoffs]. My understanding is Y would be worse because [reasons]. Are you suggesting Y better serves the original tradeoffs, that we weigh tradeoffs differently, or something else?"

Deep dive: references/author-03-handling-comments.md.

Emergencies (rare)

A real emergency is small AND one of: rescues a major launch, fixes a prod bug significantly affecting users, handles a pressing legal issue, closes a major security hole.

Not emergencies: ship-this-week-not-next, the dev has worked on this for ages, reviewers in another timezone, end-of-Friday, soft deadline, rolling back a build break.

In a real emergency, the reviewer prioritizes speed and correctness-of-fix above all else, and the CL jumps the queue. After the emergency, give the CL a thorough post-review.

A hard deadline = something disastrous happens if missed (contractual obligation, marketplace failure, once-a-year hardware ship window). Most deadlines are soft and don't justify cutting corners.

Deep dive: references/02-emergencies.md.

Resolving conflicts (either role)

1. Try to reach consensus using the principles above as common ground.
2. Hard? Move to face-to-face or video — record the outcome as a CL comment for future readers.
3. Still stuck → escalate (TL, maintainer, eng manager). Never let a CL sit because two people can't agree.