code-review

Files (1)

• SKILL.md

SKILL.md (first lines)

Two-axis review of the diff between `HEAD` and a fixed point the user supplies:

- **Conventions** — does the code follow this repo's documented coding standards and Tidy First practices?
- **Intent** — does the change faithfully do what it claims to do?

Both axes run as **parallel sub-agents** so they don't pollute each other's context, then this skill aggregates their findings.

## Where This Fits

This skill owns **micro / local quality** — conventions, clarity, correctness of individual changes. It answers: "Is this change well-crafted and does it do what it says?"

For **macro / structural quality** (architecture, code judo, 1k-line limits, abstraction quality), use `code-quality-review`. That skill asks: "Is there a dramatically simpler structure hiding inside this implementation?"

| Concern | code-review (this skill) | code-quality-review |
|---|---|---|
| Clean code & naming | ✅ Primary owner | — |
| Tidy First practices | ✅ Primary owner | — |
| Behavior matches commits | ✅ Primary owner | — |
| Guard clauses, helper vars | ✅ Primary owner | — |
| File under 1k lines | Flag if crossed | Enforce strictly |
| Structural simplification | Note opportunities | Demand code judo |
| Abstraction quality | Flag thin wrappers | Delete unnecessary layers |

### Companion Skills

A complete quality pipeline, in order:

| Phase | Skills | Purpose |
|-------|--------|---------|
| 1. Discovery | `blindspot-pass`<br>`context-discovery` | Find unknown unknowns and gather project context before starting |
| 2. During implementation | `implementation-logger` | Track deviations from plan as you go |
| 3. Pre-review cleanup | `slop` | Remove AI-generated clutter so the review focuses on substance |
| 4. **Review** | **`code-review`** (this skill) | Conventions + Intent, side by side |
| 5. Structural audit | `code-quality-review` | Code judo, 1k-line limits, abstraction quality |
| 6. Fix & wrap | `pr-review` → `commit-atomic` → `quiz-me` | Apply fixes, group into logical commits, verify understanding |

Phases 1–4 are the core loop. Phase 5 is recommended when the change touches architecture or crosses file-size boundaries. Phase 6 depends on what the review finds.

## Process

### 1. Pin the fixed point

The user supplies a fixed point — a commit SHA, branch name, tag, `main`, `HEAD~5`, etc. If they don't specify one, ask for it.

Capture the diff command once: `git diff <fixed-point>...HEAD` (three-dot, so the comparison is against the merge-base). Also note the list of commits via `git log <fixed-point>..HEAD --oneline`.

Before going further, confirm the fixed point resolves (`git rev-parse <fixed-point>`) and the diff is non-empty. A bad ref or empty diff should fail here — not inside the sub-agents.

### 2. Gather context

**Conventions sources** — discover the repo's coding standards. Look for any of these common patterns:
- `CONVENTIONS.md`, `.planning/codebase/CONVENTIONS.md`, `STYLE_GUIDE.md` — language-specific idioms and patterns
- `CONTRIBUTING.md`, `best-practices.md`, `CODING_STANDARDS.md` — general development philosophy, guard clauses, helper expectations
- `AGENTS.md`, `CLAUDE.md`, `GEMINI.md` — project-specific instructions for AI coding assistants
- Any file the repo advertises as its source of truth for code style — search `README.md` or `docs/` for mentions
- If the repo documents nothing, the clean code smell baseline below still applies

**Intent sources** — understand what the change claims to do:
- Find issue references in commit messages and the PR description (`#123`, `Closes #45`, etc.) and fetch the linked issue when possible
- Read the PR description if one exists (from `gh pr view` or branch context)
- Look for a spec or plan under `docs/`, `specs/`, `.scratch/`, or a path supplied by the user
- Check for an `.implementation-log.md` file that records conscious deviations from the plan — the review should **not** penalize a valid pivot
- If no issue, spec, plan, or PR description exists, say so explicitly and use commit messages as the fallback source of intent

A commit message is evidence of intent, not a substitute for the originating spec. Separate missing requirements from unrequested scope.

### 3. Spawn both sub-agents in parallel

Send a single message with two `Agent` tool calls. Use the `general-purpose` subagent for both.

**Conventions sub-agent prompt** — include:
- The full diff command and commit list.
- The list of conventions-source files you found in step 2, **plus the clean code smell baseline below** pasted in full — the sub-agent has no other access to it.
- The brief: "Report — per file/hunk where relevant — (a) every place the diff violates a documented convention: cite the convention (file + the rule); and (b) any baseline smell you spot: name it and quote the hunk. Distinguish hard violations from judgement calls — documented-convention breaches can be hard, but baseline smells are always judgement calls. A documented repo convention overrides the baseline. Skip anything tooling (linters, formatters, pre-commit hooks) already enforces. Under 400 words."

**Intent sub-agent prompt** — include:
- The diff and commit list.
- The issue, spec, plan, or PR description found in step 2, and identify which source is authoritative.
- The commit messages and any implementation log.
- The brief: "Report: (a) requirements from the authoritative issue/spec that are missing or partial; (b) behavior in the diff that was not requested (scope creep); (c) claimed behavior that looks incorrectly implemented. Quote the source line for each finding. If only commit messages exist, say that confidence is lower. If an implementation log records a conscious deviation, note it but don't flag it as a problem. Under 400 words."

### 4. Aggregate

Present the two reports under `## Conventions` and `## Intent` headings, verbatim or lightly cleaned. Do **not** merge or rerank findings — the two axes are deliberately separate.

End with a one-line summary: total findings per axis, and the worst issue *within each axis* (if any). Don't pick a single winner across axes.

### 5. Suggest next steps

Based on findings, suggest which companion skill to run next:
- Structural concerns → `code-quality-review` (phase 5)
- AI-generated clutter → `slop`, then re-review (phase 3 — clean first, then re-review)
- Fixes needed → `pr-review` (phase 6)
- Commit hygiene → `commit-atomic` (phase 6)

## Clean Code Smell Baseline

These 10 smells apply on top of whatever the repo documents. Two rules bind them:
- **The repo overrides.** A documented convention always wins; where it endorses something the baseline would flag, suppress the smell.
- **Always a judgement call.** Each smell is a labelled heuristic, never a hard violation — and skip anything tooling (linters, formatters, pre-commit hooks) already enforces.

Each smell reads *what it is* → *how to fix*; match it against the diff:

---

**1. Mysterious Name** — a function, variable, class, or type whose name doesn't reveal what it does or holds. If no honest short name comes, the design itself is murky.
→ Rename to something descriptive. Names are the first line of documentation — prefer clarity over brevity.

**2. Duplicated Code** — the same logic shape, conditional chain, or data transformation appears in more than one hunk or file in the change.
→ Extract the shared shape into a function, helper, or shared module. Call it from both places.

**3. Long Function** — a function or method that does too many things. The reader must hold multiple concerns in their head at once.
→ Extract logical sections into well-named helper functions. A function should do one thing and do it at a single level of abstraction.

**4. Deep Nesting** — code indented 3+ levels deep. Arrow code that forces the reader to track multiple branching paths simultaneously. The happy path is buried under validation.
→ Invert conditions and bail out early at the top: `if invalid → return`. The main logic stays at the outermost level.

**5. Magic Values** — unexplained literals, hardcoded numbers, strings, or paths that carry implicit meaning. The reader can't tell if `7` means days, retries, or something else.
→ Extract into a well-named constant or configuration value: `MAX_RETRY_ATTEMPTS = 7`.

From jellydn/my-ai-tools, MIT.

More from jellydn/my-ai-tools