Systematic Code Review Methodology

Multi-dimensional review framework for auditing complex software modules. Covers 18 quality dimensions across three review passes, plus multi-angle iterative review for cross-cutting features.

Review Passes

Pass 1 — Execution & Integration (7 dimensions)

Focus on what's broken right now — bugs, gaps, missing wiring.

Pass 2 — Architecture & Systems (6 dimensions)

Focus on how the system runs, scales, and breaks.

Pass 3 — Reliability & Correctness (5 dimensions)

Focus on what happens when things go wrong.

Priority Tiers

Review Workflow

1. SCOPE         → List all files in the module boundary (list dir, grep)
2. READ          → Read every file in the critical path (don't skip)
3. TRACE         → Follow the data flow: API → Service → Engine → Worker
4. SCAN-PUBLIC   → For every new/changed public symbol (enum value, class, function),
                    grep the whole repo for all callers and verify each one with
                    at least 8 lines of context. Include:
                    - Enum additions: check all `if x == Enum` comparisons and
                      `x in {...}` set membership checks for breakage
                    - Function signature changes: verify every call site still
                      works with new params
                    - Type additions: verify serialization/deserialization paths
5. FIND          → For each dimension, ask the standard questions
6. DOCUMENT      → Write findings with: severity, exact location (file:line), impact description
7. CROSS-REFERENCE → Note findings that appear in multiple dimensions (highest priority)
8. PRIORITIZE    → Assign P0-P3 with estimated effort
9. PUBLISH       → Write the review to your repo's review log with date + module

Output Format

Each finding:

| Severity | Issue | `file:line` | Impact |

Master aggregation should include:

• Deduplication across review passes
• Cross-reference tracking (which passes found the same issue)
• Heatmap of dimensions vs priority tiers
• Fixed/unfixed status tracking

Cross-Dimension Correlation

Findings that appear in multiple dimensions signal architectural flaws, not isolated bugs:

Library / Utility Module Review

When reviewing a non-runtime library module, the dimensions shift from runtime concerns to correctness, integration, and hygiene.

Pass 1 — Code Quality: data model correctness (field validation, enum→string coercion, lossless serialization roundtrips), edge cases (empty inputs, None, missing files, corrupted data), accurate type hints, docstrings matching behavior, dead code.

Pass 2 — Integration & Consistency: cross-module wiring (does data flow correctly between modules?), naming consistency, paired-dictionary key alignment (e.g. LANGUAGE_MAP vs FILE_TYPE_MAP keys must match), __init__.py exports, design-doc alignment.

Pass 3 — Safety & Performance: SQL injection (parameterized queries), path traversal (resolve() + containment check, symlinks off by default), memory (large files read in chunks), error handling (OSError/JSONDecodeError caught, actionable messages).

Tool-assisted review notes:

1. ruff check <path> (project rules) — import hygiene, trailing commas
2. ruff check --select=ALL <path> — broader issues; respect project config (many violations may be intentional)
3. git diff --stat HEAD after auto-fix — verify the fixer only touched what you expected
4. Run the full test suite after any fix — no matter how trivial
5. python -c "import module" — verify no import errors after __init__.py changes
6. Don't trust project-rules-only lint config: if pyproject.toml selects only a few rules, running all rules reveals far more issues (many auto-fixable). Check the config before deciding what to fix.

Multi-Angle Iterative Review

When reviewing a cross-cutting feature that touches multiple layers (API, UI, persistence, tests), a single-pass review finds at most ~30% of real bugs. The remaining 70% surface only when you deliberately switch angles.

The pattern: Review → Fix → Repeat × N

Each round picks ONE family of angles. Don't mix families — the brain optimizes for the first pattern it recognizes and becomes blind to others.

Core insight: Early rounds find bugs in what was built. Later rounds find bugs in how it connects to everything else. Cross-module issues are invisible until the core implementation stabilizes — that's why you need multiple passes.

8 angle families

Rules

1. One family per pass. Checking security and data flow in the same pass guarantees you'll miss subtle issues in both.
2. Grep test selectors against source before writing tests. For every selector or identifier used in a test file, verify the corresponding attribute/element exists in the production code. A 10-second grep prevents a wasted review round.
3. Assume every async/sync boundary is wrong until proven correct. Static tools rarely catch sync functions calling async methods without await, or blocking I/O inside an event loop.
4. Trace every state mutation to its persistence call. "In-memory only" is the most common data-loss bug in stateful backends.
5. Cross-module issues are invisible in single-file review. When two files implement parallel versions of the same logic, review them side-by-side in the same pass.
6. Commit per fix, not per review round. Keeps git bisect functional.
7. Verify after every edit. A 3-second syntax check catches broken commits before they land.
8. New enum variant → full-codebase grep. Adding a value to an enum/union/dict key-set without grepping all consumers is the most common silent-gap bug. Grep the type name AND every existing variant value (raw strings) — mapping dicts, switch cases, match arms, if-elif chains, serialization round-trips. Pay special attention to .get(x, default) — it masks the gap.
9. Dual-module consistency check. When the same event/concept is projected by two modules, verify they agree on output shape. Neither module is individually wrong, but their outputs diverge silently.
10. Snapshot payload ≠ state. When a system writes snapshots, verify the snapshot content is the accumulated state, not the triggering event's payload (the implementation defaults to the data at hand).
11. Real integration smoke test before sign-off. Start the actual server and hit the changed endpoint. Mock-only testing cannot catch import errors, route registration failures, middleware misconfiguration, or key-resolution bugs.
12. Warning/error pattern precision. Test that warnings fire for the RIGHT imports/paths and not unrelated ones — a DeprecationWarning in a package __init__ can fire on every import of any symbol from that package.

When to use which strategy

Universal verification checklist

Run after every review round, regardless of language or stack:

• Compile/parse check for every changed file
• Type-check if the language has a type checker
• Grep all test selectors/identifiers against source code
• git diff --stat — verify the file list matches intent
• Manual trace: pick one user action, follow it from entry point to persistence and back
• Audit every I/O call for async/sync correctness
• For every state change in memory, verify a corresponding persistence call exists
• If enum/type-union changed: grep all consumers of that type name + every existing variant value

E2E + LLM-Driven Test Review Angles

When reviewing persona-driven, LLM-as-Judge, or multi-layer E2E tests, the standard angles miss test-infrastructure bugs that only surface at runtime:

Rules for E2E test review:

1. Grep every test selector against source before trusting it — a selector that silently matches nothing wastes a whole round.
2. Every behavioral toggle in the source has a default — find it. If the test needs the toggle ON but never clicks it, the test is exercising a different path.
3. Response-text extraction must be page-aware, not hard-coded to one selector.
4. LLM-as-Judge must degrade gracefully in 3 failure modes: backend unreachable → skip; API timeout → heuristic fallback; malformed response → text-based heuristic. All three paths must be exercised in review.

Data Migration Review

When reviewing a data migration (format upgrade, schema change, file conversion), the standard angles miss data-integrity bugs that only surface in the target dataset:

Migration review workflow:

1. COUNT          → Compare counts: source vs target (mismatch = bug)
2. RE-RUN         → Execute migration again; verify ALL skip (idempotency)
3. SAMPLE         → Random-sample 3-5 target files; verify new + old fields
4. DIFF-BODY      → For one file, verify body unchanged (only metadata modified)
5. CROSS-PATH     → For every output constructor, verify fields appear in ALL paths
6. VALIDATE       → Run the target validator on the full directory
7. SECURITY-SCAN  → Audit every Path(user_input) for traversal

Derived rules:

1. Count before trusting. len(source) == len(target) catches 100% of stale-target bugs.
2. Re-run proves idempotency. Second run with all skips = correct.
3. Cross-path field audit: a new field in constructor A but not B is a bug, not a TODO.
4. Every user-input path needs a fence: resolve() + relative_to containment check.

Pre-deletion safety (refactor reviews)

Before deleting any file: grep the full project for its path, imports, and exported symbols. Archive references are acceptable — active code references are NOT. Auto-generated reference docs (route indexes, README tables, endpoint counts) drift from actual code — re-check the doc's claim against the code before trusting a "0 endpoints" report.

Also check for unreachable code after return/raise/sys.exit: read the 5 lines after each at function scope; code after a return statement is dead code — the most common round-2 discovery.