loom-code-review

# Code Review ## Overview Review a change for correctness, security, performance, and maintainability, then produce prioritized, actionable feedback. Optimize signal: gate on what breaks users; comment (don't gate) on the rest. ## Review vs. adversarial security audit Different jobs — don't conflate: - **Code review (this skill):** holistic, author-empathetic. Reviews the *diff and its blast radius* against stated intent across four lenses. Assumes good faith; catches the bugs a careful peer catches. - **Security audit (`Skill(skill="loom-skills", args="loom-security-audit")`, `Skill(skill="loom-skills", args="loom-threat-model")`):** attacker mindset, threat model, whole-attack-surface. Assumes hostile input everywhere. For auth, crypto, payments, deserialization, or anything touching a trust boundary: do the code review AND trigger a security audit. A passing code review is not a security sign-off. ## Severity taxonomy Label every comment. BLOCKER, SILENT_FAILURE, and CRITICAL gate the merge. | Severity | Gate? | Meaning | | ------------------ | ----- | --------------------------------------------------------------------------------- | | **BLOCKER** | ✅ | Security hole, data loss, crash, corruption. Merge is unsafe. | | **SILENT_FAILURE** | ✅ | Exit 0 but the operation actually failed (sandbox-blocked download, partial fetch, stale cache, swallowed error). Always investigate before merge. | | **CRITICAL** | ✅ | Logic error / real bug that will bite in normal use. | | **MAJOR** | ❌ | Maintainability, tech debt, missing test for a real path. Fix soon. | | **MINOR / nit** | ❌ | Style, naming, micro-optimization. Prefix with `nit:` so the author can skip it. | **Approve-with-comments discipline:** if nothing is BLOCKER/SILENT_FAILURE/CRITICAL, approve and leave the MAJOR/MINOR notes as non-blocking. Don't hold a PR hostage over nits or personal style. Blocking on taste is the top reviewer anti-pattern — it trains authors to ignore you. ## Method 1. **Read intent first.** PR description, linked issue, commit messages. Review against what the change *claims* to do; flag scope creep separately from bugs. 2. **Review the diff AND its blast radius.** A hunk is not self-contained. For every changed symbol, `rg` its callers and callees — a signature change, a new early-return, a changed default, or a widened type ripples outward. Bugs hide at the seams the diff doesn't show. 3. **Four lenses per hunk:** correctness → security → performance → maintainability (below). 4. **Missing-tests / missing-error-path pass** (separate sweep — easy to skip). 5. **Verdict:** approve / approve-with-comments / request-changes, each comment severity-tagged. ### Blast-radius checklist - [ ] Callers of every changed function/signature updated (search, don't assume) - [ ] Callees: are new preconditions actually guaranteed by callers? - [ ] Changed default value / enum variant / error type — who relied on the old one? - [ ] Concurrency: new shared state, lock ordering, `await` points holding a guard? - [ ] Public API / serialized format / DB schema change — back-compat and consumers? - [ ] Tests, docs, and types updated alongside behavior? ### Four lenses - **Correctness:** edge cases (empty, null, zero, negative, overflow, unicode, TZ/DST), off-by-one, error paths, resource cleanup (files/locks/connections on *every* return incl. early ones), idempotency/retry safety, race conditions. Trace the unhappy path, not just the happy one. - **Security:** input validation at the boundary, injection (SQL/command/path/SSRF/XSS), authz on every sensitive op, secrets not logged, crypto-grade randomness. (Deep dive → security audit.) - **Performance:** algorithmic complexity, N+1 queries, unnecessary allocation/clone in hot paths, missing pagination/streaming for unbounded data, `O(n)`-in-a-loop membership checks (want a set/map). Don't nitpick micro-perf off the hot path. - **Maintainability:** does the abstraction fit the problem? single responsibility, honest naming, no copy-paste of logic that will drift, no dead/speculative code, comments explain *why* not *what*. ### Missing tests & error paths The most common real defect in a passing PR: an untested error path. Ask: - [ ] New branch / early-return / `catch` with no test exercising it? - [ ] External call (network/db/fs) — what happens on timeout, 500, empty result? - [ ] Does a returned `Result`/`Err`/rejected promise get handled, or silently dropped? - [ ] New public function without a test for its failure mode, not just success? ## Loom orchestration review For code produced by loom stages, add these (they catch the "compiles + tests pass but doesn't work" class): - **Silent failure (BLOCKER):** exit 0 with error/warning on stderr; sandbox blocked a download but stage completed; external dep referenced but not installed/reachable. - **Wiring (CRITICAL):** feature compiles and tests pass but is never imported / registered / mounted / reachable by a real user. Verify: is the module imported at the entry point? command/route registered? event handler connected? DI binding present? Can a user actually invoke it? (See `/loom-wiring-test`.) - **Dependency reality:** package actually installed (not just in manifest); data file actually downloaded (not just referenced); endpoint actually reachable (not just configured). ## Domain quick-checklists Compressed — expand the relevant one only when the diff touches that domain. - **Infra/IaC:** no hardcoded secrets; least-privilege IAM; state backend secured; resource limits/requests (k8s); non-root, minimal base image, no secrets in layers (Docker); rollback path (CI/CD). - **Data pipeline:** schema validation; null/dup handling; idempotent + exactly-once where claimed; dead-letter queue; partitioning/batching; storage lifecycle for cost. - **ML:** seed set for reproducibility; train/val/test split has no leakage; data + model versioned; drift monitoring and rollback in prod; bias/fairness on training data. ## Reviewer output format Group by severity, cite `file:line`, state issue → impact → fix. Keep it scannable. ```markdown # Review: auth/login.py ## BLOCKER - **L45 SQL injection** — `f"... WHERE email = '{email}'"` interpolates user input. Impact: arbitrary SQL. Fix: `cursor.execute("... WHERE email = %s", (email,))`. ## MAJOR - **L112 unchecked None** — `send_email(user.email, ...)` after `.first()` can `None`-deref when the user doesn't exist. Add a guard returning 404. ## nit - **L23** `GetUser` → `get_user` (snake_case). ## Good - Clean validation split (L30-40); solid test coverage on the happy path. ``` Two tiny wrong→right patterns worth internalizing: ```python # perf: O(n·m) — membership check rebuilds nothing but scans a list each pass if user.id in active_ids: # list → O(n) per lookup # → hoist once: active = set(active_ids) # O(1) per lookup if user.id in active: ``` ## Author-empathy phrasing Same fact, better delivery — critique the code, ask don't command, give the reason: - ❌ "This is wrong." → ✅ "This deref crashes when `user` is None (L112) — guard it?" - ❌ "Why didn't you use a set?" → ✅ "A `set` here makes this O(1) per lookup; worth it on the hot path." - ❌ "Bad naming." → ✅ "nit: `data` is vague — `active_orders`?" Acknowledge good work explicitly; it makes the blocking comments land. ## Verify before done - [ ] Read the PR intent; scope creep flagged separately from bugs - [ ] Every changed symbol's callers/callees checked (blast radius, not just the hunk) - [ ] All four lenses applied; unhappy paths traced - [ ] Missing-test / error-path sweep done - [ ] Loom: wiring + silent-failure + dependency-reality checks (if orchestrated code) - [ ] Every comment severity-tagged; nits marked `nit:`; verdict matches (only BLOCKER/CRITICAL gate) - [ ] Security-sensitive surface → security audit triggered, not just reviewed