Code Review
Purpose
Evaluate a change for correctness, readability, maintainability, and consistency with the project's conventions and architecture — and report actionable findings. Supports Gate 6.
When to Use
- After implementation of a task/feature/fix, before merge/release.
- For a code review request type.
- Not as a substitute for security review (
security-review) or tests (testing-strategy).
Inputs
- The diff/change under review and its context (work item, acceptance criteria).
- Project conventions and architecture.
Discovery Questions
- What was this change supposed to do (acceptance criteria)?
- Which files/modules changed, and what do they touch?
- Are there tests, and do they cover the change?
Responsibilities
- Verify the change meets its acceptance criteria.
- Check correctness (logic, edge cases, error handling).
- Check clarity/maintainability (naming, structure, duplication).
- Check convention/architecture fit and unintended behavior changes.
- Confirm tests exist and are meaningful (delegate depth to
testing-strategy).
- Report findings with severity and confirmed vs suggestion.
Required Workflow
- Read acceptance criteria + the diff.
- Assess correctness and edge cases.
- Assess clarity, maintainability, conventions.
- Check tests cover the change.
- Flag security/perf concerns to the specialist skills.
- Report findings (severity, confirmed/suggestion) with
file:line.
Decision Rules
- Block on confirmed correctness or security defects; suggestions don't block.
- Prefer the smallest correct change; flag scope creep.
- Hand security-specific and performance-specific concerns to
security-review / performance-review.
Rules
- Be specific: cite
file:line and the failure scenario.
- Separate confirmed defects from style/preference.
- Don't approve unverified correctness; don't rubber-stamp.
Anti-Patterns
- Vague feedback with no location or scenario.
- Bikeshedding style while missing correctness.
- Approving without checking the change meets its criteria.
Validation Checklist
Definition of Done
A review report with located, severity-rated findings (confirmed defects vs suggestions), a clear pass/block verdict against acceptance criteria, and any security/perf items routed to specialists.
Related Skills
security-review, performance-review, testing-strategy, ai-output-review, final-quality-audit, project-orchestrator.
Related Knowledge
../../knowledge/ (conventions, architecture).
Related References
../../references/<topic>/ for the language/framework idioms if needed.
Context Loading Guidance
- Requires: the diff, acceptance criteria, relevant conventions.
- Does not require: the whole repo, unrelated references, planning skills.
- May load:
security-review, performance-review, testing-strategy for depth.
- Stop when: the review report is delivered.
Token Efficiency Guidance
Review the diff plus only the surrounding context needed to judge it. Don't re-read the whole module unless the change's correctness depends on it.
1---2name: code-review3description: Use to review a change or diff for correctness, clarity, maintainability, and convention-fit before it advances. Reports findings with severity, separating confirmed defects from suggestions; does not rubber-stamp.4---56# Code Review78## Purpose910Evaluate a change for correctness, readability, maintainability, and consistency with the project's conventions and architecture — and report actionable findings. Supports Gate 6.1112## When to Use1314- After implementation of a task/feature/fix, before merge/release.15- For a **code review** request type.16- **Not** as a substitute for security review (`security-review`) or tests (`testing-strategy`).1718## Inputs1920- The diff/change under review and its context (work item, acceptance criteria).21- Project conventions and architecture.2223## Discovery Questions2425- What was this change supposed to do (acceptance criteria)?26- Which files/modules changed, and what do they touch?27- Are there tests, and do they cover the change?2829## Responsibilities3031- Verify the change **meets its acceptance criteria**.32- Check **correctness** (logic, edge cases, error handling).33- Check **clarity/maintainability** (naming, structure, duplication).34- Check **convention/architecture fit** and unintended behavior changes.35- Confirm **tests** exist and are meaningful (delegate depth to `testing-strategy`).36- Report findings with **severity** and **confirmed vs suggestion**.3738## Required Workflow39401. Read acceptance criteria + the diff.412. Assess correctness and edge cases.423. Assess clarity, maintainability, conventions.434. Check tests cover the change.445. Flag security/perf concerns to the specialist skills.456. Report findings (severity, confirmed/suggestion) with `file:line`.4647## Decision Rules4849- Block on confirmed correctness or security defects; suggestions don't block.50- Prefer the smallest correct change; flag scope creep.51- Hand security-specific and performance-specific concerns to `security-review` / `performance-review`.5253## Rules5455- Be specific: cite `file:line` and the failure scenario.56- Separate confirmed defects from style/preference.57- Don't approve unverified correctness; don't rubber-stamp.5859## Anti-Patterns6061- Vague feedback with no location or scenario.62- Bikeshedding style while missing correctness.63- Approving without checking the change meets its criteria.6465## Validation Checklist6667- [ ] Change meets acceptance criteria.68- [ ] Correctness/edge cases assessed.69- [ ] Clarity/maintainability assessed.70- [ ] Conventions/architecture respected.71- [ ] Tests cover the change.72- [ ] Findings have severity + confirmed/suggestion + location.7374## Definition of Done7576A review report with located, severity-rated findings (confirmed defects vs suggestions), a clear pass/block verdict against acceptance criteria, and any security/perf items routed to specialists.7778## Related Skills7980`security-review`, `performance-review`, `testing-strategy`, `ai-output-review`, `final-quality-audit`, `project-orchestrator`.8182## Related Knowledge8384`../../knowledge/` (conventions, architecture).8586## Related References8788`../../references/<topic>/` for the language/framework idioms if needed.8990## Context Loading Guidance9192- **Requires:** the diff, acceptance criteria, relevant conventions.93- **Does not require:** the whole repo, unrelated references, planning skills.94- **May load:** `security-review`, `performance-review`, `testing-strategy` for depth.95- **Stop when:** the review report is delivered.9697## Token Efficiency Guidance9899Review the diff plus only the surrounding context needed to judge it. Don't re-read the whole module unless the change's correctness depends on it.
ahtishamshahzad