Ruby Code Review Levels

W5H (required for non-trivial changes):

• What: what does this code do?
• Why: why is this approach needed here?
• Where: where does this belong (controller/model/service/pattern)?
• When: when does it run (sync vs async, request vs background)?
• How it fails: what are the failure modes and what happens next?

Level 2 — Correctness & Edge Case Review (Logic)

Objective: ensure correct behavior for realistic inputs and boundary conditions.

Mandatory checks:

• Nil handling is explicit and intentional (no accidental nil propagation).
• Boundaries: empty collections, zero values, missing keys, unexpected formats.
• Determinism: same inputs should produce the same outputs (especially for decision logic).
• Numeric safety: for floats/rounding, ensure the behavior is stable and documented.
• Time/zone correctness: use the correct time source (avoid mixing app time and system time incorrectly).
• Idempotency guards (where relevant): avoid double-processing, double-exits, duplicate updates.
• Validate invariants before doing work (e.g., existence checks, “state is allowed” checks).

Questions:

• What if upstream data is stale/incomplete?
• What if state already changed between decision and action?
• Are there hidden assumptions about ordering or concurrency?

Level 3 — Design & Abstraction Review (Structure)

Objective: ensure boundaries are real and abstractions are earned.

Rules:

• SRP: each unit has one reason to change.
• Don’t add useless service objects that just proxy model calls.
• Put domain logic with models (or dedicated POROs) and keep controllers thin.
• Extract shared logic across strategies instead of duplicating it with small variations.
• Avoid “fake polymorphism”: only abstract when it reduces duplication or clarifies invariants.

Review heuristics:

• Is the abstraction describing a real concept (e.g., Position, Policy, Validator)?
• Where should this logic live: model, service (orchestrator), or PORO (calculation/logic)?
• Are naming and ownership clear enough that a future developer can extend it safely?

Level 4 — System & Architectural Review (Integrity)

Objective: ensure correctness under load, failures, and concurrency.

Focus:

• Deterministic flows: event-driven or async sequences should be explicit.
• Idempotency at system boundaries: prevent duplicate side effects (DB writes, external calls).
• State management: single source of truth; avoid mixing “cache truth” and “DB truth” without reconciliation.
• Invariants:
  • Never place/trigger the same action twice for the same logical entity.
  • Never transition state in an invalid order.
  • Always re-check critical preconditions close to execution if the world can change.

Failure modes:

• What happens if external dependencies fail or time out?
• Are retries safe (idempotent) and bounded (no infinite loops)?
• Are partial failures handled with clear compensation or persistence rules?

Level 5 — Production Readiness Review (Safety)

Objective: make the change safe to ship.

Required checks:

• Observability: structured logs (or consistent log lines), including correlation/request IDs when available.
• External/API safety: timeouts, bounded retries with backoff, and verification of post-conditions.
• Graceful degradation: clear error propagation; avoid silent failures.
• Performance sanity: avoid accidental N+1, and keep work bounded (no unbounded loops).
• Testing coverage for the critical behavior:
  • at least one spec for the new behavior
  • edge cases for nil/boundaries
  • idempotency/duplicate prevention when side effects exist

Exit criteria:

• Level 2 + Level 4 must be green.
• Level 5 requires explicit safety and observability for any side-effecting changes.

Output format (what to write back to the user)

For each finding, include:

• Level (1–5)
• File and symbol/method name (where possible)
• Why it fails or risks failure (one concise sentence)
• Concrete fix suggestion (smallest change that addresses it)