Code Review And Quality

Default output: return only the result, blockers, and required evidence. Omit preambles, process narration, repeated context, confidence scores, and follow-up offers. Use at most five bullets unless a required artifact or schema needs more.

Code Review and Quality

Adapted from addyosmani/agent-skills (MIT), commit 82ceff41ed4d3c644e3dcca8a0514390b2911223.

Overview

Review changes across five axes: correctness, readability, architecture, security, and performance. Standard mode asks whether the change improves the codebase without avoidable risk. Strict maintainability mode adds structural ambition: hunt for simplifications that delete complexity, not just rearrange it.

When to Use

• Before merging any change
• After a feature, bug fix, or refactor
• When validating whether tests and verification are sufficient
• When the user or parent agent requests a deep maintainability audit (strict mode)

Review modes

Strict mode baseline: Rethink structure without changing behavior. Improve abstractions, modularity, and legibility. If a code-judo move could delete whole branches or layers, push for it.

The Five-Axis Review

1. Correctness

• Does the change meet the task or spec?
• Happy paths, edge cases, and errors handled?
• Tests cover the real behavior change?

2. Readability and Simplicity

• Clear names consistent with the codebase?
• Control flow easy to follow?
• Clever shortcuts that should be straightforward code?

In strict mode, also ask: Is the implementation direct, or special-case spaghetti? Any thin wrappers or pass-through helpers that add indirection without clarity?

3. Architecture

• Follows existing patterns?
• Module boundaries still clean?
• Abstraction level appropriate?

In strict mode, also ask:

• Logic in the canonical layer? Reusing existing helpers vs bespoke one-offs?
• Feature logic leaking into shared paths?
• File crossing 1000 lines without strong justification—prefer extract/split first.
• New ad-hoc conditionals bolted onto busy flows—prefer dedicated abstraction or module.
• Sequential orchestration or non-atomic updates when parallel/atomic structure is obvious?

4. Security

• Untrusted input validated at boundaries?
• Secrets out of code and logs?
• Auth, permissions, and external data handled safely?

5. Performance

• N+1, repeated expensive work, unbounded operations?
• Pagination, batching, caching, async boundaries where needed?

Review Process

Step 1: Understand intent

What changed, why, and what proof should exist.

Step 2: Review tests first

• Regression tests for bug fixes?
• Behavior verified, not private implementation?
• Would tests fail if the bug returned?

Step 3: Review implementation (five axes)

Prefer concrete findings over style. In strict mode, prioritize:

1. Structural regressions and missed dramatic simplifications
2. Spaghetti / branching growth
3. Boundary, abstraction, and type-contract problems
4. File-size and decomposition
5. Modularity and legibility

Strict questions (when applicable):

• Code-judo move that removes concepts/branches/layers?
• Refactor that moves complexity without deleting it?
• Casts, any, unknown, or optionality hiding the real invariant?
• Duplicate of a canonical helper or wrong package/layer?

Step 4: Label findings

• Critical: security, broken behavior, data loss
• Required: must fix before merge
• Optional: worthwhile, not blocking
• Nit: cosmetic
• FYI: context only

Strict mode: do not soften structural issues into nits. Be direct; skip cosmetic floods when structural problems exist.

Step 5: Verify the verification story

Targeted tests, relevant suite, build/typecheck, manual checks for UI/ops.

Findings priority (strict mode)

1. Structural regressions
2. Missed simplification / code-judo opportunities
3. Spaghetti and special-case branching
4. Boundary and type-contract problems
5. File size and decomposition
6. Modularity and legibility

Approval bar

Standard: no critical/required issues unresolved; relevant tests and build pass.

Strict: behavior correct is not enough. Presumptive blockers unless clearly justified:

• Plausible simplification path ignored; complexity preserved
• File pushed from under to over 1000 lines
• Ad-hoc branching tangling shared flows
• Feature checks scattered across general modules
• Unnecessary wrapper, cast-heavy contract, or wrong-layer logic
• Duplicate of existing canonical helper

Output template

## Findings

- Critical: ...
- Required: ...
- Optional: ...

## Verification

- Tests: ...
- Build: ...
- Manual: ...

Red flags

• No regression test for a bug fix
• Large change without verification summary
• Unlabeled severity in feedback
• Security-sensitive changes reviewed only for style
• Diffs that should have been split
• (Strict) File-size jump, scattered conditionals, or refactor that only relocates complexity

Anti-Rationalization

Verification checklist

• Critical and required issues resolved or explicitly deferred with justification
• Relevant tests pass; build succeeds
• Review summary documents what was checked
• (Strict) No presumptive blockers above without justification

Subagent orchestration (strict mode)

When invoked as a Task subagent, the parent supplies ### Git / diff output and ### Changed file contents. Apply this skill to that input only; trace cross-file impact at module boundaries. Do not spawn nested subagents unless asked.

Typical parent flow: parallel shell + explore tasks for diff and file contents, then invoke code-reviewer in strict mode with labeled sections.