Code Review Expert
Senior engineer code review skill — delivers structured reviews covering architecture, SOLID principles, security, performance, error handling, and code quality.
This skill is the single source of truth for the review workflow. The code-review-expert agent's system prompt intentionally defers to it.
Workflow
Follow these steps sequentially for every review:
Step 1: Scope Changes via Git Diff
- Identify all modified, added, and deleted files
- Understand the intent of the changes from PR title/description
- Map out which components/modules are affected
Step 2: Understand the Whole Picture and the Architecture
Build global context BEFORE judging any line of code:
- Project context: browse the local clone (README, docs, directory layout,
entry points, build config) to understand what the project does, its module
boundaries, and its layering.
- Read the diff end-to-end, then trace relationships FROM the diff:
who calls the changed code, what the changed code calls, which data flows
through it. Reconstruct the call chain around every changed public symbol.
- State the goal: summarize in one or two sentences what this change is
trying to achieve and how it intends to achieve it. Every later finding is
judged against this goal.
Mandatory Verification (before forming any verdict)
For EVERY non-trivial changed file:
- Read the FULL file — not just the diff hunks. Use the local clone when it
is checked out at the PR head; if the clone is unavailable or is not at
the PR head, fetch post-change file content via the
get-pr-diff skill's
full-file strategy (contents?ref=<head SHA>).
- Grep for all call sites of every changed/added/removed public symbol;
read the enclosing function of each call site.
- For changed types/interfaces/schemas: locate every usage and confirm
it still holds with the new shape.
- For changed behavior: find the tests covering it; if none exist, say so.
A diff-only review is invalid. You may not assert correctness of code you
did not open.
Large-PR escape hatch: for very large PRs (hundreds of files, or a diff
beyond GitHub's limit), apply full verification to the prioritized subset of
files per the get-pr-diff skill's large-PR guidance, and explicitly state
in the review summary which files were NOT fully verified.
Step 3: Architecture-Level Review
Refer to references/architecture-review.md for the detailed checklist.
- Direction: is the change architecturally correct? Is there a better
approach — where "better" means globally optimal for the codebase, not
locally clean within the diff? If a materially better design exists at
comparable cost, propose it concretely.
- Entities: review every added/changed API, field, type, enum, config
key, and DB column against software design philosophy — SSOT (no two
fields/entities carrying the same meaning, one authoritative writer per
fact) and Occam's razor (no entity multiplied beyond necessity, no
speculative generality).
- Scope: architecture findings must be introduced or materially worsened
by THIS change. Pre-existing problems are out of scope — do not report
them as findings.
Step 4: Evaluate Design Principles (SOLID)
Refer to references/solid-checklist.md for detailed smell prompts.
- SRP: Does any file own unrelated concerns?
- OCP: Are there rigid switch/if blocks that should be extension points?
- LSP: Any subclass/implementation violations?
- ISP: Fat interfaces with unused methods?
- DIP: High-level modules depending on concrete implementations?
Step 5: Identify Unused Code Candidates
- Dead code: unreachable, never-called, or commented-out code
- Deprecated APIs still in use
- Feature flags that are always on/off
- Refer to
references/removal-plan.md for safe deletion process
Step 6: Security Scanning
Refer to references/security-checklist.md for the full checklist.
- XSS/Injection: Unsafe HTML, SQL/NoSQL/command injection
- Auth gaps: Missing access checks, authorization gaps, IDOR
- Secrets: Hardcoded keys, tokens in code/logs
- CSRF: State-changing operations without CSRF protection when cookie/session auth is used
- Race conditions: TOCTOU, concurrent state, check-then-act
- Input validation: Missing sanitization, path traversal
Step 7: Code Quality Assessment
Refer to references/code-quality-checklist.md for detailed patterns.
- Error handling: Swallowed exceptions, missing catch, async gaps
- Performance: N+1 queries, blocking I/O, missing cache, O(n^2) hot paths, unnecessary React/Vue re-renders, memory leaks
- Boundary conditions: Null/undefined, empty collections, off-by-one
- Complexity & maintainability: Long functions, deep nesting, duplicated logic, tight coupling, poor testability
- Naming & clarity: Magic numbers, unclear names, documentation gaps, commented-out code, sensitive logs
Step 8: Report Findings
Classify each finding by severity:
- P0 (Critical): Must block merge — security vulnerabilities, data loss risks
- P1 (High): Should fix — bugs, significant design issues
- P2 (Medium): Consider fixing — maintainability, code quality
- P3 (Low): Optional — style, minor improvements
Step 9: Deliver the Review
- Non-interactive (summoned
code-review-expert agent, e.g. automated PR
review): the output contract in the agent's system prompt takes precedence
— submit the structured result via submit_output. Skip the markdown
format below.
- Interactive (chat): present findings using the Output Format below and
wait for explicit approval before suggesting fixes. Do NOT auto-apply
changes.
Output Format (interactive mode only)
## Review Summary
[One paragraph overview]
### Findings
- **[P0/P1/P2/P3]** `category` — **Title**
- File: `path/to/file`
- Description
- Suggested fix
### Verdict
APPROVE | REQUEST_CHANGES | COMMENT
1---2name: code-review-expert3description: Senior engineer code review workflow — whole-picture understanding, mandatory beyond-the-diff verification, architecture-level review (SSOT, Occam's razor), SOLID, security, performance, and code quality, with P0-P3 severity classification.4---56# Code Review Expert78Senior engineer code review skill — delivers structured reviews covering architecture, SOLID principles, security, performance, error handling, and code quality.910This skill is the **single source of truth for the review workflow**. The `code-review-expert` agent's system prompt intentionally defers to it.1112## Workflow1314Follow these steps sequentially for every review:1516### Step 1: Scope Changes via Git Diff17- Identify all modified, added, and deleted files18- Understand the intent of the changes from PR title/description19- Map out which components/modules are affected2021### Step 2: Understand the Whole Picture and the Architecture22Build global context BEFORE judging any line of code:23241. **Project context**: browse the local clone (README, docs, directory layout,25 entry points, build config) to understand what the project does, its module26 boundaries, and its layering.272. **Read the diff end-to-end**, then trace relationships FROM the diff:28 who calls the changed code, what the changed code calls, which data flows29 through it. Reconstruct the call chain around every changed public symbol.303. **State the goal**: summarize in one or two sentences what this change is31 trying to achieve and how it intends to achieve it. Every later finding is32 judged against this goal.3334#### Mandatory Verification (before forming any verdict)35For EVERY non-trivial changed file:361. Read the FULL file — not just the diff hunks. Use the local clone when it37 is checked out at the PR head; if the clone is unavailable or is not at38 the PR head, fetch post-change file content via the `get-pr-diff` skill's39 full-file strategy (`contents?ref=<head SHA>`).402. Grep for all call sites of every changed/added/removed public symbol;41 read the enclosing function of each call site.423. For changed types/interfaces/schemas: locate every usage and confirm43 it still holds with the new shape.444. For changed behavior: find the tests covering it; if none exist, say so.4546A diff-only review is invalid. You may not assert correctness of code you47did not open.4849**Large-PR escape hatch:** for very large PRs (hundreds of files, or a diff50beyond GitHub's limit), apply full verification to the prioritized subset of51files per the `get-pr-diff` skill's large-PR guidance, and explicitly state52in the review summary which files were NOT fully verified.5354### Step 3: Architecture-Level Review55Refer to `references/architecture-review.md` for the detailed checklist.56571. **Direction**: is the change architecturally correct? Is there a better58 approach — where "better" means globally optimal for the codebase, not59 locally clean within the diff? If a materially better design exists at60 comparable cost, propose it concretely.612. **Entities**: review every added/changed API, field, type, enum, config62 key, and DB column against software design philosophy — SSOT (no two63 fields/entities carrying the same meaning, one authoritative writer per64 fact) and Occam's razor (no entity multiplied beyond necessity, no65 speculative generality).663. **Scope**: architecture findings must be introduced or materially worsened67 by THIS change. Pre-existing problems are out of scope — do not report68 them as findings.6970### Step 4: Evaluate Design Principles (SOLID)71Refer to `references/solid-checklist.md` for detailed smell prompts.72- **SRP**: Does any file own unrelated concerns?73- **OCP**: Are there rigid switch/if blocks that should be extension points?74- **LSP**: Any subclass/implementation violations?75- **ISP**: Fat interfaces with unused methods?76- **DIP**: High-level modules depending on concrete implementations?7778### Step 5: Identify Unused Code Candidates79- Dead code: unreachable, never-called, or commented-out code80- Deprecated APIs still in use81- Feature flags that are always on/off82- Refer to `references/removal-plan.md` for safe deletion process8384### Step 6: Security Scanning85Refer to `references/security-checklist.md` for the full checklist.86- **XSS/Injection**: Unsafe HTML, SQL/NoSQL/command injection87- **Auth gaps**: Missing access checks, authorization gaps, IDOR88- **Secrets**: Hardcoded keys, tokens in code/logs89- **CSRF**: State-changing operations without CSRF protection when cookie/session auth is used90- **Race conditions**: TOCTOU, concurrent state, check-then-act91- **Input validation**: Missing sanitization, path traversal9293### Step 7: Code Quality Assessment94Refer to `references/code-quality-checklist.md` for detailed patterns.95- **Error handling**: Swallowed exceptions, missing catch, async gaps96- **Performance**: N+1 queries, blocking I/O, missing cache, O(n^2) hot paths, unnecessary React/Vue re-renders, memory leaks97- **Boundary conditions**: Null/undefined, empty collections, off-by-one98- **Complexity & maintainability**: Long functions, deep nesting, duplicated logic, tight coupling, poor testability99- **Naming & clarity**: Magic numbers, unclear names, documentation gaps, commented-out code, sensitive logs100101### Step 8: Report Findings102Classify each finding by severity:103- **P0 (Critical)**: Must block merge — security vulnerabilities, data loss risks104- **P1 (High)**: Should fix — bugs, significant design issues105- **P2 (Medium)**: Consider fixing — maintainability, code quality106- **P3 (Low)**: Optional — style, minor improvements107108### Step 9: Deliver the Review109- **Non-interactive (summoned `code-review-expert` agent, e.g. automated PR110 review):** the output contract in the agent's system prompt takes precedence111 — submit the structured result via `submit_output`. Skip the markdown112 format below.113- **Interactive (chat):** present findings using the Output Format below and114 wait for explicit approval before suggesting fixes. Do NOT auto-apply115 changes.116117## Output Format (interactive mode only)118119```markdown120## Review Summary121[One paragraph overview]122123### Findings124- **[P0/P1/P2/P3]** `category` — **Title**125 - File: `path/to/file`126 - Description127 - Suggested fix128129### Verdict130APPROVE | REQUEST_CHANGES | COMMENT131```
rome-os