# Code Review

> Canonical code review guidelines. Use when reviewing code, preparing PRs for review, or giving feedback on changesets. Covers tone, procedures, code quality, changeset scope, and review process.

- Skill: `tonyandrewmeyer/code-review` (Agent Skill)
- Install (CLI): `npx skillmds add tonyandrewmeyer/code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/tonyandrewmeyer/code-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- License: default
- Author: tonyandrewmeyer (https://skillmd.com/u/tonyandrewmeyer)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/tonyandrewmeyer/code-review

---


# Canonical Code Review Guidelines

These guidelines define how code reviews should be conducted at Canonical. They focus on broadly applicable review techniques that are relevant to all projects.

Based on spec PE-ALL-005.

---

## Soft Skills

### Tone
- Review the **submission**, not the author. Avoid "you did this wrong."
- Prefer "this could be improved by…" or "this doesn't seem right to me…"
- Be **constructive** — suggest how something may be improved
- For non-critical suggestions, make clear it's informational, not a change request: "I might have done this differently for these reasons…"
- Avoid being overtly negative, even if something is poor quality
- Remember: review is a **learning experience** for both author and reviewers

### Self-review
- Review your own code before submitting
- Catches spurious changes, incomplete work, and things you missed in the editor
- Self-review changes how you think about the code

---

## Procedures

- CI builds must **succeed**
- **Ticket numbers** included in commits where needed
- Pull request descriptions must be **usefully descriptive** — "Fixed Bug 12345" is not sufficient; include a sentence or two about what changed and why
- Two **Signed-off-by** tags from Canonical reviewers (use judgement: trivial patches may need only one)

---

## Code Quality

- All patches must follow the **code style and conventions** of the appropriate project
- Look for cases where end-user function behaviour **diverges from upstream** — ask for clarification and push for upstreamable implementations

---

## Changeset Size and Scope

### Size
- Large changesets are complex and difficult to review
- Large changesets can usually be **split** into smaller commits or separate tickets
- They tend to combine related but not strictly connected changes
- Use pragmatism — sometimes a large changeset is genuinely necessary

### Scope
- Does it address things **not needed** in the ticket? Call them out — good changes can go in a separate commit/ticket
- Does it address things **not mentioned** in the commit message?
- Does it make unintentional changes that might cause bugs?

### Completeness
- Watch for **missing files** (forgot to `git add`)
- Missing files do not always cause CI failures (e.g. a comment referencing a README that was not committed)

---

## Recommended Review Process

1. **Read the ticket(s)** — understand what should be done, verify referenced tickets are correct
2. **Skim** commit messages and code for formatting, size, and scope issues
3. **Dive deeper**:
   - Check **leaf functions** first (no calls to other modified functions)
   - Work upward toward functions with more modified invocations
4. **Walk through** new control flow, computational, or memory mapping behaviour — consider optimisation opportunities, especially on hot paths
5. Use **linters and static analysis** as pre-commit hooks where available

---

## Project-Specific Considerations

For projects applying patches not yet upstreamed:
- Patches should be of **upstream quality** and submitted upstream simultaneously
- Include DEP3-style `Forwarded:` tags with links to upstream submissions
- Ubuntu-originated patches: mark `UBUNTU: SAUCE:`
- Partner-originated patches: mark `<VENDOR>: SAUCE:`