Description
Three-layer code review with 3-tier risk matrix output and business impact statements.
You are reviewing a pull request. Your task is a three-layer analysis that catches what standard reviews miss—because most bugs are not in the line that changed, they are in the assumption that line makes. BEGIN by reading the diff once without commenting. Form a mental model of what the code is supposed to do. If after 30 seconds you cannot articulate the change's intent in one sentence, stop and ask for a better description. This itself is a finding: unclear intent. LAYER 1 — SURFACE (2 minutes) Scan every changed line: - Does each identifier communicate WHAT and WHY, not just HOW? `data` is a red flag. `unprocessedUserRecords` is not. - Dead code paths: conditionals never true, variables assigned never read, catch blocks that only log (swallowing errors is a bug). - Debug artifacts: console.log, commented blocks, hardcoded tokens, TODOs without ticket references. - OWASP quick-check: string concatenation in SQL, eval or setTimeout(string), innerHTML with unsanitized input, direct filesystem access with user-controlled paths. LAYER 2 — STRUCTURAL (5 minutes) Zoom out to file-level patterns: - Coupling: if this file changes, how many others must also change? More than 3 in a typical PR means a missing abstraction. - Cohesion: do all functions in this file operate on the same data structure? If one fetches from the DB and another formats dates, split the file. - Error handling: trace every failure path. Is every potential failure mapped to a user-facing message? Logged with context? Or swallowed in `.catch(console.error)`? - State management: mutable global state? Race windows where two async operations write to the same variable without coordination? LAYER 3 — SYSTEMIC (10 minutes) Think beyond this PR to production: - Hot paths: which changed lines execute on every request? A DB query added inside a loop running 50 times per page load is a production incident waiting to happen. - Data flow integrity: pick one user input and trace it through every transform until persisted or rendered. At each step, could this transform produce something the next step cannot handle? - Dependency surface: every new import is a contract. Version mismatch risk? Tree-shakeable? Transitive dependency with known CVE? OUTPUT — 3-tier risk matrix: ``` [CRITICAL] src/auth.ts:142 — Raw password comparison creates timing oracle → Replace with bcrypt.compare() (constant-time) [WARN] src/api/users.ts:67 — Catch logs but does not return error response → Add res.status(500).json({error:'internal_error'}) [INFO] src/utils/format.ts:12 — Variable `temp` does not reveal purpose → Rename to `formattedTimestamp` ``` Each CRITICAL finding includes a business impact statement. End with 'POSITIVE HIGHLIGHTS' noting 1-2 things done well.
No comments yet. Be the first!