Skill: Code Review

Use this skill when reviewing a PR, auditing recent changes, or checking code quality before merge. The review covers 8 dimensions and produces a structured report.

Workflow

1. Gather the Diff

Determine what changed. Use one of:

git diff main..HEAD --stat           # file list
git diff main..HEAD                   # full diff
git log --oneline main..HEAD          # commit messages

For ad-hoc reviews, the user may point to specific files instead.

2. Run Automated Checks

Before any manual review, run the project's automated gatekeepers:

npm run lint          # ESLint
npm run typecheck     # tsc --noEmit
npm run test          # Vitest unit tests
npm run build         # Next.js production build

If any fail, note them as P0 findings — blocking issues that must be fixed.

3. Manual Review — 8 Dimensions

Walk through every changed file and evaluate against the checklist in resources/review-checklist.md. The 8 dimensions are:

4. Produce the Report

Structure findings using the template in resources/report-template.md. Classify each finding:

5. Run the Diagnostic Script (Optional)

For a quick automated scan of common anti-patterns:

# View usage
bash .agent/skills/code-review/scripts/scan-antipatterns.sh --help

# Scan all changed files vs main
bash .agent/skills/code-review/scripts/scan-antipatterns.sh

The script checks for inline financial math, raw fetch() in components, missing withBudgetAccess(), toast() in hooks, and more.

Dimension Details

D1. Correctness

• •Does the code actually implement the stated intent?
• •Are there off-by-one errors, null/undefined derefs, or type coercion traps?
• •Are async operations properly awaited? (Common: await params in Next.js 15)
• •Are early returns correct? (if (!access.ok) return access.response)
• •Edge cases: empty arrays, zero values, negative amounts, null category IDs (transfers)

D2. Architecture Compliance (Rules 05, 11)

• •Engine boundary: No financial math outside lib/engine/. See rule 05.
• •3-layer pattern: Engine (pure) → Repo (orchestration) → Hook/Route (consumer)
• •No inline SQL in routes: All DB access via lib/repos/
• •No raw DB rows in responses: Always use DTO mappers from lib/dtos/
• •Barrel exports: New modules exported from their index.ts

D3. Security (Rule 12)

• •Every budget-scoped route uses withBudgetAccess(budgetId, handler)
• •Every non-budget route calls requireAuth()
• •Input validation: All write endpoints use validateBody() with Zod schemas
• •No credential leaks: No passwords, tokens, or internal IDs in responses
• •await params in Next.js route handlers (not direct access)
• •RLS session vars set via withBudgetAccess() (transaction-scoped)

D4. Performance

• •N+1 queries: Is data fetched in a loop instead of a batch query?
• •Missing cancelQueries in onMutate can cause stale refetch races
• •Unnecessary re-renders: Large objects in context, unstable references
• •Future date filter: Balance queries MUST use AND date <= date('now') (rule 02 §4D)
• •React Query keys: Are invalidation scopes too broad or too narrow?

D5. Financial Logic (Rules 02, 03, 04, 05, 07)

• •Milliunit types: All monetary values use Milliunit branded type, never plain number
• •Test helpers: Tests use mu() and ZERO, never as Milliunit
• •Engine purity: Engine functions have ZERO dependencies on DB/HTTP/React
• •RTA formula: Cash − Available − FutureAssigned − CreditOverspending
• •CC Payment: Funded spending calculation correct? Payments subtracted?
• •Carryforward: Regular categories max(0, prev), CC payment categories carry debt
• •Ghost entries: Assignment of 0 deletes the budget_months row, not creates one
• •Overspending: Credit (yellow) never reduces RTA; cash (red) deducts next month

D6. Testing (Rule 08)

• •New logic has unit tests in lib/__tests__/
• •Bug fixes include regression test that reproduces the bug
• •Engine coverage: 100% branch coverage target
• •E2E tests updated if UI flows changed
• •No skipped, disabled, or commented-out tests
• •Test helpers: mu(), ZERO from ./test-helpers (not manual casts)

D7. Style & Conventions (Rules 00, 06, 09)

• •API payloads: camelCase (Zod schemas), DB columns: snake_case
• •Mutation hooks: Have mutationKey, meta, optimistic updates with snapshot/rollback
• •No direct toast() in mutation hooks — use meta.errorMessage
• •Debounced inputs: Text/numeric fields use useDebouncedMutation
• •UI language: Spanish for user-facing strings
• •File naming: use*.ts for hooks, *.dto.ts for DTOs, lib/repos/*.ts for repos
• •Schema changes: schema.ts → db:generate → db:migrate (rule 09)

D8. UX & Accessibility

• •Optimistic updates for every user-visible mutation
• •Loading states: Skeleton/spinner while data fetches
• •Error states: Graceful degradation, not blank screens
• •data-testid on interactive elements (for E2E)
• •Keyboard navigation: Focus management, Enter to submit, Escape to cancel

Key Rules Reference

Resources & Scripts