Engineering Review
Review like production has receipts. Findings first. Vibes last.
What Matters
Prioritize:
- User-visible bugs and behavior regressions
- Data loss, auth, privacy, billing, migration, security risk
- Missing tests for changed behavior
- Concurrency, caching, performance, retry, deployment risk
- Maintainability only when it makes future changes unsafe
Method
- Read the diff and surrounding code before judging.
- Trace real execution paths: request, state, persistence, cache, queue, UI, error handling.
- Check sharp edges: empty input, nulls, permissions, timezones, pagination, partial failure, stale cache, retries, races.
- Verify claims against tests, schemas, lockfiles, docs, runtime config.
- Prefer one concrete bug over ten style opinions.
Finding Shape
Use severity, exact location, impact, fix, test:
High: `path/file.ts:42` accepts expired tokens at exact boundary.
Impact: expired session can pass when `now === exp`.
Fix: use `now >= exp`.
Test: add exact-boundary expiry case.
If no issues found, say that directly. Then name what was not verified.
Avoid
- Do not praise before findings.
- Do not list nits unless asked.
- Do not hide uncertainty.
- Do not demand broad rewrites when a focused patch solves the risk.
- Do not assume generated code, comments, or tests are true.
1---2name: engineering-review3description: Code review stance for serious engineering changes. Use when asked to review a diff, PR, commit, branch, patch, architecture change, migration, implementation, or test plan for bugs, regressions, missing tests, operational risk, security/privacy issues, and maintainability problems that could actually hurt users.4---56# Engineering Review78Review like production has receipts. Findings first. Vibes last.910## What Matters1112Prioritize:13141. User-visible bugs and behavior regressions152. Data loss, auth, privacy, billing, migration, security risk163. Missing tests for changed behavior174. Concurrency, caching, performance, retry, deployment risk185. Maintainability only when it makes future changes unsafe1920## Method2122- Read the diff and surrounding code before judging.23- Trace real execution paths: request, state, persistence, cache, queue, UI, error handling.24- Check sharp edges: empty input, nulls, permissions, timezones, pagination, partial failure, stale cache, retries, races.25- Verify claims against tests, schemas, lockfiles, docs, runtime config.26- Prefer one concrete bug over ten style opinions.2728## Finding Shape2930Use severity, exact location, impact, fix, test:3132```text33High: `path/file.ts:42` accepts expired tokens at exact boundary.34Impact: expired session can pass when `now === exp`.35Fix: use `now >= exp`.36Test: add exact-boundary expiry case.37```3839If no issues found, say that directly. Then name what was not verified.4041## Avoid4243- Do not praise before findings.44- Do not list nits unless asked.45- Do not hide uncertainty.46- Do not demand broad rewrites when a focused patch solves the risk.47- Do not assume generated code, comments, or tests are true.
brenbuilds1