# Emrecdr Devt Code Review Guide

> Code Review Guide

- Skill: `tomevault-io/emrecdr-devt-code-review-guide` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add tomevault-io/emrecdr-devt-code-review-guide`
- Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/emrecdr-devt-code-review-guide/raw
- Safety review: pending (external: skill-scanner PASS, skillspector PASS)
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: tomevault-io (https://skillmd.com/u/tomevault-io)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/tomevault-io/emrecdr-devt-code-review-guide

---


# Code Review Guide

## Overview

A code review is a structured inspection that produces a scored assessment with specific, actionable findings. Every finding must reference a concrete location, describe the problem, explain the impact, and suggest a fix.

Reviews are objective. They evaluate code against documented project standards, not personal preferences. If a standard does not exist for something, it is not a finding.

**Precision mandate**: Every finding must name a specific file, line number, and exact violation. If a finding could apply to any codebase, it is too vague to report.

## When NOT to Use

Skip for self-review during implementation — this is for the formal review phase.

## Time Budget

- **Quick review** (1-3 files): 2-3 minutes
- **Full review** (5+ files): 5-10 minutes

## The Iron Law

```
EVERY VALID FINDING MUST BE REPORTED — NO FILTERING, NO MERCY
```

An unreported finding is a silently disabled quality gate. If the code violates a documented standard, report it — regardless of origin, perceived severity, or whether the developer "probably knows."

The tendency to filter findings by perceived importance, origin ("pre-existing"), or social comfort ("it's minor") directly undermines the review's value. Users rely on the review to catch what they missed. A finding omitted because it seemed small is a finding the developer never gets to evaluate. The reviewer's job is detection, not triage — the developer decides what to fix.

## The Process

### Step 1: Load Standards

Before reviewing any code, read the project's rules:

1. `.devt/rules/coding-standards.md` — code conventions
2. `.devt/rules/architecture.md` — structural boundaries
3. `.devt/rules/quality-gates.md` — pass/fail criteria
4. `CLAUDE.md` — project-specific rules

A review without loaded standards is a review against personal opinion. Do not start reviewing until all applicable standards are loaded.

### Step 2: Understand Scope

Read the implementation summary or PR description:

- What files were changed and why
- What the acceptance criteria are
- What the intended behavior is

This sets context but does not limit findings. Issues found outside the stated scope are still valid.

### Step 3: Review by Category

Evaluate each changed file against these categories:

#### Architecture (weight: high)

- Layer boundaries respected (domain has no infrastructure imports)
- Dependency direction correct (inner layers do not depend on outer)
- Separation of concerns (routes are thin, services hold logic)
- No circular dependencies
- Repository pattern followed (services use interfaces, not sessions)

#### Security (weight: critical)

- Input validation present (schema validation, type safety)
- Authentication/authorization checks in place
- No secrets in code or logs
- SQL injection prevention (parameterized queries)
- No data exposure in error responses

#### Performance (weight: medium)

- No N+1 query patterns
- No unnecessary database round-trips
- Appropriate use of async/sync
- No unbounded queries (missing pagination/limits)

#### Error Handling (weight: high)

- Custom errors inherit from base error classes (not plain Exception)
- No swallowed exceptions (bare except or except Exception: pass)
- Errors carry enough context for debugging
- Graceful degradation where appropriate

#### Test Coverage (weight: high)

- New code has corresponding tests
- Edge cases covered (empty input, null, boundary values)
- Tests are independent and deterministic
- Test names describe behavior, not implementation

#### UI/UX Quality (weight: medium, frontend projects only)

For Vue/frontend projects, also check:

- Focus states present and visible on interactive elements
- Touch targets meet minimum size (24x24px min)
- Text contrast ratio meets WCAG AA (4.5:1 for normal text)
- Semantic HTML used (`<nav>`, `<main>`, `<button>` vs generic `<div>`)
- `prefers-reduced-motion` respected for animations

#### Code Quality (weight: medium)

- Clear naming (variables, functions, classes)
- No deep nesting (3+ levels)
- Early returns used for guard clauses
- No duplication (DRY)
- No dead code, commented-out code, or TODOs
- Type hints present on all function signatures

### Step 4: Score Each Finding

Every finding gets a severity:

| Severity      | Point Deduction | Criteria                                                                              |
| ------------- | --------------- | ------------------------------------------------------------------------------------- |
| **Critical**  | -15             | Security vulnerability, data loss risk, architectural violation, broken functionality |
| **Important** | -7              | Missing error handling, missing tests, performance issue, inconsistent pattern        |
| **Minor**     | -3              | Naming issue, style inconsistency, missing type hint, documentation gap               |

### Step 5: Calculate Score

```
Starting score: 100
Final score: 100 - sum(deductions)
Minimum: 0
```

### Step 6: Determine Verdict

| Score | Verdict                 | Meaning                                                 |
| ----- | ----------------------- | ------------------------------------------------------- |
| >= 90 | **APPROVED**            | Ship it. Minor issues can be addressed later.           |
| 80-89 | **APPROVED_WITH_NOTES** | Acceptable but has important findings to address.       |
| < 80  | **NEEDS_WORK**          | Must fix critical/important findings before proceeding. |

### Step 7: Write Report

Structure the report as:

```
## Review Summary
Score: XX/100 — VERDICT

## Findings

### [Critical] File:Line — Short description
**Problem**: What is wrong
**Impact**: Why it matters
**Fix**: What to do

### [Important] File:Line — Short description
...
```

### Scoring Examples

**Critical finding (-15)**:

- PASS: "SQL injection in `users.ext:47` -- user input concatenated into query string without parameterization"
- FAIL: "Possible security issue" (too vague -- name the file, line, exact problem)

**Important finding (-7)**:

- PASS: "Missing error handling in `payment_service.ext:89` -- API call to Stripe has no try/catch, will crash on network timeout"
- FAIL: "Error handling could be better" (unactionable)

**Minor finding (-3)**:

- PASS: "Inconsistent naming: `getUserData()` at `api.ext:23` but `fetch_user_info()` at `api.ext:67` -- pick one convention"
- FAIL: "Naming is inconsistent" (where? what? be specific)

## Gate: Honest Scoring

- [ ] No findings dismissed as "acceptable" or "pre-existing"
- [ ] Deductions applied consistently across findings
- [ ] Verdict matches the score (no manual override)

## Anti-patterns

| Don't                                          | Why It Fails                                                                 | Do Instead                                                |
| ---------------------------------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------- |
| Skip review because "it's simple"              | Simple code has simple bugs that reach production                            | Review everything, even one-liners                        |
| Accept "it works" as sufficient                | Working code can be insecure, unmaintainable, or wrong                       | Check quality dimensions, not just functionality          |
| Label findings as "pre-existing"               | Origin is irrelevant -- if it's visible, it's your responsibility            | Report every finding. No origin column.                   |
| Rate "close enough" as APPROVED                | Partial compliance becomes full non-compliance over time                     | Score honestly. 78 is NEEDS_WORK, not 80.                 |
| Skip security checks for internal code         | Internal code gets promoted to external. Supply chain attacks hit internals. | Full security checklist every time                        |
| Say "the tests pass so it's fine"              | Passing tests prove tests pass, not that code is correct                     | Evaluate all 6 categories, not just functionality         |
| Dismiss "it follows the existing pattern"      | If the pattern is wrong, it is still a finding                               | Report it. Consistency with bad patterns is not a defense |
| Soften scores because "flagging this is harsh" | Accuracy is not harshness                                                    | Apply deductions by severity criteria, not by feelings    |

## Integration

- **Prerequisites**: Standards files must exist in `.devt/rules/`
- **Used by agents**: code-reviewer (primary consumer)
- **Related skills**: codebase-scan (to verify no duplication introduced), architecture-health-scanner (for systemic issues)

## Memory + Graphify integration

Reviews must include an "ADR Compliance" section. For each diff hunk:
1. `node bin/devt-tools.cjs memory affects <changed-file>` — enumerate governing docs
2. Verify diff respects active ADRs (treat violations as Critical findings)
3. `node bin/devt-tools.cjs memory rejected-keywords` — flag any diff text matching a REJ tombstone
4. When Graphify enabled, enumerate affected callers via `graphify neighbors <symbol> --direction=in`
   (uses `skills/graphify-helpers/SKILL.md` protocol — falls back to grep when disabled).

---
> Source: [emrecdr/devt](https://github.com/emrecdr/devt) — distributed by [TomeVault](https://tomevault.io).
<!-- tomevault:4.0:skill_md:2026-06-08 -->