Reviewing Code
Structured review methodology producing consistent, severity-based, actionable feedback. This skill defines the review process — how to conduct a review, format findings, and decide; the what to look for lives in perspective reference files the orchestrator loads selectively. Non-code artefacts use assessing-quality (rageatc-core-oss); architecture design review is the critic's job at architecture approval.
Perspective Model
Perspectives are reference files under references/ defining what to examine. The orchestrator tells the reviewer-agent which to load.
| Reference |
Focus |
Typical use |
references/spec-compliance.md |
Does code match the brief and acceptance criteria? |
All tiers — always loaded |
references/code-quality.md |
Correctness, simplicity, readability, test quality |
Standard + Thorough |
references/security.md |
Input validation, injection, auth, data exposure |
Thorough, or when chunk handles user input |
references/whole-project.md |
Full codebase against PRD success criteria |
End of Standard/Thorough build only |
references/design-compliance.md |
UI matches system.md tokens, depth, patterns |
Standard + Thorough, when project has system.md and chunk touches UI |
Tier defaults (orchestrator may override — e.g., security for a Quick auth fix):
| Tier |
Perspectives loaded |
| Quick |
spec-compliance |
| Standard |
spec-compliance → code-quality → design-compliance (if system.md exists and chunk touches UI) |
| Thorough |
spec-compliance → code-quality → design-compliance (if applicable) → security (as needed) |
| End-of-build |
whole-project (standalone, against PRD not brief) |
Inputs Required
- Enriched roadmap chunk — acceptance criteria, file set, purpose (the enriched roadmap IS the brief)
- ARCHITECTURE.md — patterns, conventions, component boundaries
- Codebase access and changed files list
- Test results — or the ability to run the suite
- Perspective references — loaded by orchestrator
- For re-reviews: previous review and the developer's changes
Output
# Code Review: [chunk name/ID]
**Brief:** [reference to development brief or roadmap chunk]
**Perspectives:** [which perspectives were applied]
**Decision:** Accept / Revise
**Findings:** [count by severity]
## Findings
[Findings grouped by severity: critical first, then major, minor, notes]
## What Works Well
[Brief acknowledgement of what meets standards]
## Decision
[Accept or Revise with rationale]
## Observations
[Anything relevant to the broader project but outside this chunk's scope]
Review Process
- Understand intent before reading code. Read the enriched roadmap chunk completely: acceptance criteria (your primary checklist), which files were supposed to change, what the chunk contributes to the system. Do not open code files first.
- Run tests. Record pass/fail as factual input — no conclusions yet.
- Review against loaded perspectives, spec-compliance first. Not every concern applies to every chunk — spend time proportional to risk. Traps: reviewing beyond scope (review this chunk; broader issues go in Observations) and style preferences over standards (review against ARCHITECTURE.md patterns, not taste).
- Structure findings per the Finding Format, with honest severity. Trap: nitpicking on accept — if heading towards acceptance, limit minor findings to the 3-5 most valuable.
- Step back. Does this code, as a whole, solve the problem the brief describes? A review that catches every style issue but misses that the core logic is wrong has failed.
- Decide per the criteria below, stating the rationale.
Re-review Protocol
- Read your previous review first
- Verify each previously raised finding is addressed
- Do not re-raise findings that were adequately resolved
- Check that fixes have not introduced new issues
- Carry forward unresolved minor findings only if still relevant
- Apply the same decision criteria — a re-review is not a lower bar
Finding Format
**[SEVERITY]** file:line (or component)
_Finding:_ What the issue is.
_Impact:_ Why it matters.
_Suggestion:_ How to fix it.
Example:
**[MAJOR]** src/validators/email-validator.ts:1-45
_Finding:_ Email validation uses an abstract factory pattern with three
interfaces and a plugin system. The actual validation is 4 lines of code
wrapped in 40 lines of abstraction.
_Impact:_ Maintenance burden disproportionate to the problem. Future
developers must understand the factory pattern to modify a simple check.
Violates simplicity principle.
_Suggestion:_ Replace with a direct validation function. If additional
validators are needed later, extract a pattern then — not before.
Severity Levels
| Level |
Meaning |
Examples |
| Critical |
Blocks acceptance. Must fix. |
Acceptance criterion not met, security vulnerability, data loss risk, tests broken |
| Major |
Degrades quality significantly. Should fix. |
Unhandled error causing user-facing failure, missing test for core logic, architectural pattern violation |
| Minor |
Improves quality. Desirable but does not block. |
Naming could be clearer, minor duplication, missing edge case test for non-critical path |
| Note |
Observation only. No action required. |
Worth knowing for future work, pattern that could become a problem at scale |
Severity honesty: do not inflate. A naming improvement is not critical; inflation erodes trust and wastes developer cycles. Security findings are never below Major — exploitable vulnerabilities are Critical — even when spec-compliance passes (calibration table in references/security.md).
Decision Criteria
Accept — all of: zero critical findings, zero major findings, all acceptance criteria satisfied, tests pass. Minor findings and notes are included but do not block.
Revise — any of: a critical or major finding, an unsatisfied acceptance criterion, failing tests (unless explained by an intentional, brief-sanctioned change). List the specific issues requiring revision.
1---2name: reviewing-code3description: Reviews code. Use when reviewing code implementations, assessing code quality, running a security review, checking spec or design system compliance, or conducting an end-of-build whole-project review against PRD success criteria.4---56# Reviewing Code78Structured review methodology producing consistent, severity-based, actionable feedback. This skill defines the **review process** — how to conduct a review, format findings, and decide; the **what to look for** lives in perspective reference files the orchestrator loads selectively. Non-code artefacts use assessing-quality (rageatc-core-oss); architecture design review is the critic's job at architecture approval.910## Perspective Model1112Perspectives are reference files under `references/` defining what to examine. The orchestrator tells the reviewer-agent which to load.1314| Reference | Focus | Typical use |15|-----------|-------|-------------|16| `references/spec-compliance.md` | Does code match the brief and acceptance criteria? | All tiers — always loaded |17| `references/code-quality.md` | Correctness, simplicity, readability, test quality | Standard + Thorough |18| `references/security.md` | Input validation, injection, auth, data exposure | Thorough, or when chunk handles user input |19| `references/whole-project.md` | Full codebase against PRD success criteria | End of Standard/Thorough build only |20| `references/design-compliance.md` | UI matches system.md tokens, depth, patterns | Standard + Thorough, when project has system.md and chunk touches UI |2122**Tier defaults** (orchestrator may override — e.g., security for a Quick auth fix):2324| Tier | Perspectives loaded |25|------|--------------------|26| Quick | spec-compliance |27| Standard | spec-compliance → code-quality → design-compliance (if system.md exists and chunk touches UI) |28| Thorough | spec-compliance → code-quality → design-compliance (if applicable) → security (as needed) |29| End-of-build | whole-project (standalone, against PRD not brief) |3031## Inputs Required3233- **Enriched roadmap chunk** — acceptance criteria, file set, purpose (the enriched roadmap IS the brief)34- **ARCHITECTURE.md** — patterns, conventions, component boundaries35- **Codebase access** and **changed files list**36- **Test results** — or the ability to run the suite37- **Perspective references** — loaded by orchestrator38- For re-reviews: **previous review** and the developer's changes3940## Output4142```43# Code Review: [chunk name/ID]4445**Brief:** [reference to development brief or roadmap chunk]46**Perspectives:** [which perspectives were applied]47**Decision:** Accept / Revise48**Findings:** [count by severity]4950## Findings5152[Findings grouped by severity: critical first, then major, minor, notes]5354## What Works Well5556[Brief acknowledgement of what meets standards]5758## Decision5960[Accept or Revise with rationale]6162## Observations6364[Anything relevant to the broader project but outside this chunk's scope]65```6667## Review Process68691. **Understand intent before reading code.** Read the enriched roadmap chunk completely: acceptance criteria (your primary checklist), which files were supposed to change, what the chunk contributes to the system. Do not open code files first.702. **Run tests.** Record pass/fail as factual input — no conclusions yet.713. **Review against loaded perspectives**, spec-compliance first. Not every concern applies to every chunk — spend time proportional to risk. Traps: reviewing beyond scope (review this chunk; broader issues go in Observations) and style preferences over standards (review against ARCHITECTURE.md patterns, not taste).724. **Structure findings** per the Finding Format, with honest severity. Trap: nitpicking on accept — if heading towards acceptance, limit minor findings to the 3-5 most valuable.735. **Step back.** Does this code, as a whole, solve the problem the brief describes? A review that catches every style issue but misses that the core logic is wrong has failed.746. **Decide** per the criteria below, stating the rationale.7576## Re-review Protocol77781. Read your previous review first792. Verify each previously raised finding is addressed803. Do not re-raise findings that were adequately resolved814. Check that fixes have not introduced new issues825. Carry forward unresolved minor findings only if still relevant836. Apply the same decision criteria — a re-review is not a lower bar8485## Finding Format8687```88**[SEVERITY]** file:line (or component)8990_Finding:_ What the issue is.9192_Impact:_ Why it matters.9394_Suggestion:_ How to fix it.95```9697Example:9899```100**[MAJOR]** src/validators/email-validator.ts:1-45101102_Finding:_ Email validation uses an abstract factory pattern with three103interfaces and a plugin system. The actual validation is 4 lines of code104wrapped in 40 lines of abstraction.105106_Impact:_ Maintenance burden disproportionate to the problem. Future107developers must understand the factory pattern to modify a simple check.108Violates simplicity principle.109110_Suggestion:_ Replace with a direct validation function. If additional111validators are needed later, extract a pattern then — not before.112```113114### Severity Levels115116| Level | Meaning | Examples |117|-------|---------|----------|118| **Critical** | Blocks acceptance. Must fix. | Acceptance criterion not met, security vulnerability, data loss risk, tests broken |119| **Major** | Degrades quality significantly. Should fix. | Unhandled error causing user-facing failure, missing test for core logic, architectural pattern violation |120| **Minor** | Improves quality. Desirable but does not block. | Naming could be clearer, minor duplication, missing edge case test for non-critical path |121| **Note** | Observation only. No action required. | Worth knowing for future work, pattern that could become a problem at scale |122123**Severity honesty:** do not inflate. A naming improvement is not critical; inflation erodes trust and wastes developer cycles. Security findings are never below Major — exploitable vulnerabilities are Critical — even when spec-compliance passes (calibration table in `references/security.md`).124125## Decision Criteria126127**Accept** — all of: zero critical findings, zero major findings, all acceptance criteria satisfied, tests pass. Minor findings and notes are included but do not block.128129**Revise** — any of: a critical or major finding, an unsatisfied acceptance criterion, failing tests (unless explained by an intentional, brief-sanctioned change). List the specific issues requiring revision.
isvlasov