--- name: better-coding-review description: "Review code, diffs, and pull requests for bugs, security issues, and maintainability problems before merge — read-only, any language, any repo. MUST be used whenever code is reviewed, a diff or PR is checked, or pre-merge quality is in question — even if the user does not explicitly say review. Two-stage (spec compliance first, then code quality), three-pass, severity-tagged (CRITICAL/MAJOR/MINOR/NIT); every finding reported as file:line + problem + why + fix, verified against source. Reports only, never modifies files. Not for whole-repo health checks — that is the audit skill. Trigger: /better-coding-review" metadata: version: "2.12.0" license: Apache-2.0 --- # Better Coding Review Structured, read-only code review designed to counter the failure modes that make reviews inconsistent: ad-hoc every-time checks, findings that vary session to session, unverified nitpicks, and blocking on style a linter should own. **Primary objective:** catch bugs, security issues, and maintainability problems with a repeatable process and a fixed output format — the *same* checks in the *same* order producing the *same* shape of findings, so nothing important is skipped because attention drifted. Two review stages: spec compliance first (does the code do what was asked?), code quality second (is it well-built?). --- ## Activation Begin the first response after loading with exactly this line: ▸ better-coding-review active — read-only, three-pass, source-verified Emit it once, then begin the review. It signals to the user that a structured review (not an ad-hoc glance) is running and that no files will be touched. Do not repeat it later in the review. --- A repeatable review beats a fresh-every-time one: the *same* checks run in the *same* order and produce the *same* output shape, so findings are comparable across sessions and nothing important is skipped because attention drifted. --- ## Scope **In scope:** reading existing code and reporting bugs, security issues, and maintainability problems. Static, read-only analysis. **Out of scope — say so honestly rather than pretending coverage:** * Writing code or applying fixes. This skill only *reports*. If the user then wants fixes, switch to the `better-coding-workflow` skill — its "Responding to review feedback" section governs how to act on these findings (verify each against source, push back when wrong, no performative agreement). * Style a formatter/linter already owns. Do not block on formatting a linter auto-fixes; instead flag the linter *not being configured*. * Rewriting the author's correct-but-different approach into your preferred one (that is gatekeeping — see Anti-Patterns). **Prime directive: do not modify files.** No formatting, no "fix while I'm here", no commits. Review = read + report. --- ## Before You Start 1. Get the change. Skip if it's already in context (e.g. injected by a review app). ```bash git status git diff --stat HEAD git diff HEAD # or: git diff origin/main..HEAD git log --oneline -10 ``` 2. State the target and baseline in one line, e.g. "Reviewing the diff of `feature/x` vs `main`: 6 files, ~240 lines." For a diff, review the changed lines **plus the code they touch** — a change is only correct in context. 3. If the *intent* of the change is unclear, ask before reviewing. You cannot judge correctness against an unknown goal. If a design document or plan exists (from `better-coding-plan`), read it first — it is the spec to check against. 4. **Never review more than ~400 lines in one sitting.** Comprehension drops sharply past that; split large diffs into sessions and say you're doing so. 5. If automated tooling is available (linter, type checker, security scanner), note whether it was run and its results. Don't duplicate what a linter catches — focus on what requires human judgment. 6. Exclude generated code, lockfiles, and vendored dependencies from findings — note their presence, don't review their content. --- ## Two-Stage Review ### Stage 1: Spec compliance Before evaluating code quality, verify the code does what was asked: - Does the implementation match the spec/plan/PR description? - Is there anything in the spec that is not implemented? - Is there anything implemented that is not in the spec (scope creep)? - Are the acceptance criteria from the plan met? - Are the edge cases identified in the spec handled? If no spec exists, infer the intent from the PR description, commit messages, and the change itself. State the inferred intent explicitly so the author can correct it if wrong. **Spec compliance is a gate.** Code that is beautifully written but doesn't do what was asked is a failed review. Report spec deviations as findings before code quality issues. ### Stage 2: Code quality Only once spec compliance is verified, evaluate the code itself using the three-pass process and checklists below. --- ## Three-Pass Process Do not try to catch everything in one read. |Pass|Focus|Look for| |-|-|-| |**1**|High-level structure|Does the change scope make sense? Right solution to the problem? Architectural drift? API/file organization.| |**2**|Line-by-line detail|Logic errors, security issues, performance problems, edge cases. Read each file top to bottom. Flag anything that makes you pause.| |**3**|Edge cases & hardening|What breaks in production? Concurrency, boundary values, failure modes, rollback safety, missing tests on flagged paths.| Run the checklists below during Pass 2 and Pass 3. --- ## What to Check Priority order when time is limited: **Security → Correctness → Performance → Maintainability → Testing → Docs.** One missed vulnerability outweighs a hundred style nits. Every item is something to *verify against source*, not assume. ### Security (Critical priority) * **SQL / NoSQL injection** — all queries parameterized or via ORM; no string concatenation or f-strings with user input. * **Command injection** — no `os.system` / `subprocess(..., shell=True)` / `eval` / `exec` on untrusted input. * **XSS / template injection** — user content escaped before rendering; `dangerouslySetInnerHTML` (or equivalent) justified and safe. * **Insecure deserialization** — no untrusted input through `pickle`, `yaml.load` without `SafeLoader`, `eval`-based parsers, or native object-deserialization of attacker-controlled data. * **CSRF** — state-changing requests require valid tokens; `SameSite` cookies set. * **Authentication** — every protected endpoint verifies the user is authenticated before processing. * **Authorization** — access scoped to the requester's permissions; no IDOR (acting on an id from the request with no ownership check). * **Path traversal** — file paths built from user input are validated/normalized and confined to an allowed base directory; no `../` escape via params, headers, or upload filenames. * **Token / session handling** — JWTs verify signature and algorithm (reject `alg: none` and algorithm confusion), check expiry and audience; sessions regenerate on privilege change (no fixation); tokens not placed in URLs or logs. * **Input validation** — all external input (params, headers, body, files) validated for type, length, format, range on the server side. * **Secrets** — no keys/passwords/tokens in source; from env or a vault; never logged; not committed. No hardcoded fallback secrets (`getenv("KEY", "dev-secret")` ships the dev secret). * **Sensitive data** — PII/tokens/secrets never logged, put in error messages, or returned in responses. * **Dependencies** — new deps trusted, maintained, free of known CVEs. Verify the package name is real (slopsquatting risk). * **Rate limiting** — public and auth endpoints (and anything hitting an LLM or external API) are rate-limited against brute-force and cost abuse. * **File uploads** — validated for type and size, stored outside the webroot, served with safe `Content-Type`; filesystem path never derived from the client-supplied filename (path traversal). * **HTTP security headers** — CSP, `X-Content-Type-Options`, HSTS set. * **Debug off** — no `debug=True` / verbose stack traces in production paths. * **SSRF** — server-side requests validate and restrict destination URLs; no internal network access from user-controlled URLs. ### Correctness (High priority) * **Edge cases** — empty arrays/strings, zero, negative, max values handled. * **Null / undefined** — nullable values guarded before access. * **Off-by-one** — loop bounds, slicing, pagination offsets, ranges verified. * **Race conditions** — concurrent access to shared state uses locks/transactions/atomics. * **Timezones** — stored in UTC; converted at the presentation layer. * **Unicode / encoding** — multi-byte handled; encoding explicit (UTF-8). * **Integer overflow / precision** — currency and large-number math uses Decimal/BigInt, not floats. * **Error propagation** — errors from async calls and external services caught and handled; promises never silently swallowed; no bare `except:`. * **State consistency** — multi-step mutations are transactional; partial failure leaves a valid state. * **Off-nominal returns** — functions returning `None`/null on failure whose callers assume success. * **Resource cleanup** — file handles, connections, streams closed in all paths (including error paths); `finally` or equivalent used. ### Performance (High priority) * **N+1 queries** — DB access batched or joined; no per-row queries in a loop (check for missing `select_related`/`prefetch_related`/`selectinload`). * **Unbounded queries** — list endpoints paginated; no `SELECT *` without limit. * **Blocking on hot paths** — no blocking I/O or heavy CPU inside async request handlers / the event loop. * **Missing indexing** — queries filter/sort on indexed columns; new queries checked with EXPLAIN where relevant. * **Caching** — expensive computations / API responses cached appropriately. * **Memory leaks** — event listeners, subscriptions, timers, intervals cleaned up on unmount/disposal. * **Frontend** — unnecessary re-renders (missing/incorrect memoization, index-as-key on lists), bundle bloat (full-library import for one function), lazy loading for heavy/below-the-fold content. * **Algorithmic complexity** — nested loops over growing datasets; O(n²) where O(n) or O(n log n) is achievable. ### Maintainability (Medium priority) Reference the standards in the `better-coding-workflow` skill rather than restating them; the review checks that existing code *meets* them. * **Naming** — reveals intent; a newcomer understands it in 12 months. * **Single responsibility** — one reason to change per unit; business logic not leaking into controllers/UI/route handlers. * **Dependency direction** — no circular imports/dependencies between modules; dependencies point inward (infrastructure → domain), not the reverse. * **DRY / reuse** — duplicated logic extracted; new code not re-implementing an existing shared utility; near-identical copy-paste blocks consolidated. * **Over-engineering** — speculative abstractions, unused flexibility/config, reinvented stdlib, a new dependency where the standard library suffices. (A minimal smoke test or assert is never "bloat".) * **Complexity** — low branching; deep nesting flattened with early returns. Flag cyclomatic complexity above ~10, treat above ~20 as a defect. Flag functions with more than ~5 parameters (pass an object/struct instead). * **Dead code** — commented-out blocks, unused imports/vars, unreachable branches, `console.log`/`print` debugging removed. * **Magic values** — literals extracted into named constants. * **Consistency** — follows conventions already in the codebase; uses the project's design-token/theme system rather than hardcoded colors. * **Function length** — long functions decomposed. * **File size** — files approaching ~300 lines flagged for future split. ### Testing (Medium priority) * **Coverage** — new logic paths have tests; critical paths have happy-path *and* failure-case tests. * **Edge-case tests** — boundaries, empty inputs, nulls, error conditions. * **Deterministic** — no reliance on timing, external services, or shared mutable state; no flaky tests. * **Independent** — each test sets up and tears down its own state; order doesn't matter. * **Meaningful assertions** — asserts behavior/outcomes, not implementation details. * **Readable** — Arrange-Act-Assert; names describe scenario + expected outcome. * **Mocking discipline** — only external boundaries (network, DB, filesystem) mocked; prefer spying over wholesale module mocks. * **Regression tests** — a bug fix includes a test reproducing the original bug. * **Test first** — was the test written before the implementation? Tests written after tend to validate the code's bugs, not the spec. ### Documentation (Low priority) * **Comments explain why**, not what; none left lying about the code after a change. No narration ("// loop over users"). * **Public APIs** documented (purpose, params, returns, failure modes). * **README / changelog / migration notes** updated in sync when behavior changes. ### Automated tooling check Verify whether automated tooling was run and note its status: * **Linter / formatter** — run? passing? configured? * **Type checker** — run? passing? any new type errors? * **Security scanner** (Semgrep, CodeQL, Snyk) — run? findings? * **Test suite** — run? passing? coverage maintained or improved? * **Dependency audit** (`npm audit`, `pip-audit`, `cargo audit`) — run? new vulnerabilities? If tooling was not run, note it as a process gap — don't duplicate what a linter catches. Focus on what requires human judgment. ### Project-Specific Additions Extend this section with rules unique to your codebase — this is where a generic review becomes a *team* review. Examples worth encoding: * **Config-pair drift** — "if the diff touches router config A, does it also update its paired config B? Single-file edits cause drift and blank screens." * **Migration idempotency** — "migration scripts must use `IF NOT EXISTS` / `IF EXISTS` guards." * **i18n** — "new user-facing strings use i18n keys, not hardcoded text; keys added to the locale files with the agreed naming scheme." * **Downstream/cloud impact** — "flag changed backend route paths, moved SSR pages, bumped dependency versions, or changed public exports a downstream deployment depends on." * **Boundary hygiene** — "internal/commercial logic must not leak into shared packages via code or comments." --- ## Verify Before You Report The single most important discipline, and the one most often skipped: > For **each** finding, re-read the actual source around it and confirm the > problem is real — not a scanner false positive, not a misread, not already > handled two lines down. A reviewer told only to "find problems" will > manufacture them. After verifying all findings, state explicitly: **"All findings verified against source."** The per-category status line in the output format (below) forces you to give every dimension a verdict — a clean category gets an explicit PASS, so silence never gets mistaken for "not checked". --- ## Severity Levels Classify every finding. Use exactly these four labels — do not invent more. |Label|Meaning|Blocks merge?| |-|-|-| |`[CRITICAL]`|Security vulnerability, data loss, or production crash|**Yes**| |`[MAJOR]`|Bug, logic error, or significant performance regression|**Yes**| |`[MINOR]`|Improvement that reduces future maintenance cost|No| |`[NIT]`|Style/naming preference, trivial cleanup|No| --- ## Output Format Open with a **spec compliance verdict** and a **per-category status line** so it's visible that every dimension was actually checked, not just the ones with findings. Use PASS (no issues) / WARN (minor/major issues) / FAIL (critical issue) for each: ``` Spec compliance: PASS Security: FAIL Correctness: WARN Performance: PASS Maintainability: WARN Testing: PASS Docs: PASS Automated tooling: linter=pass, types=pass, tests=not run, security=not run ``` Then list findings **by severity** (critical first), numbered sequentially, each in this exact shape. **Always include file:line and the enclosing function/method** — a finding the author can't locate is not actionable: ``` N. [SEVERITY] path/to/file.ext:LINE (in ) Problem: Why: Fix: ``` If you cannot cite a specific line for something, it is an observation, not a finding — keep it out of the numbered list or mark it clearly as general. For a diff review, findings outside the changed code go under a separate **"Pre-existing (out of scope)"** heading so the author decides — don't bundle them into the blocking list. Close with a one-line verdict: ``` All findings verified against source. Verdict: . . ``` --- ## Giving Feedback (how to phrase findings) * **Be specific** — point to the exact line; "this is wrong" is not a finding. * **Explain why** — state the risk/consequence, not just the rule. * **Suggest a fix** — a concrete alternative or snippet where possible. * **Ask, don't demand, on subjective points** — "What do you think about…?" for taste; assertions only for real defects. * **Acknowledge good work** briefly when it's genuinely clean — but never pad the report with praise to soften it. * **Critique the code, never the coder.** Assume good intent. Bad: `This is wrong. Fix it.` Good: `[MAJOR] auth.py:42 — the query interpolates user input into the SQL string, allowing injection. Use a parameterized query with a bound :id param.` --- ## Anti-Patterns (do not do these) |Anti-pattern|Why it's harmful| |-|-| |Rubber-stamping|Approving without reading every changed line — worse than no review.| |Bikeshedding|Debating a variable name while ignoring a race condition.| |Blocking on style|Refusing over formatting a linter should enforce.| |Gatekeeping|Demanding your preferred approach when the submitted one is correct.| |Drive-by review|One vague comment, no follow-through.| |Scope-creep review|Requesting unrelated refactors that belong in separate PRs.| |Emotional language|"This is terrible / obviously wrong." Critique code, not person.| |Unverified findings|Flagging without re-reading the source to confirm it's real.| |Skipping spec compliance|Beautiful code that doesn't do what was asked is a failed review.| |Duplicating linter output|Wastes the reviewer's attention on what tooling already catches.| --- ## Relationship to the other skills - `/better-coding-orient` — routes to this skill after `/better-coding-workflow` for non-trivial tasks. - `/better-coding-plan` — produces the spec/plan that this review checks against in Stage 1 (spec compliance). - `/better-coding-workflow` — produces the diff this skill reviews. When the review finds issues, the user switches to `/better-coding-workflow` to apply fixes. - `/better-coding-audit` — the whole-repo version of this review. Use this per PR; use the audit periodically or before a release. - `/better-coding-frontend` — the UX/a11y specialisation of the review's frontend concerns. - `/better-coding-debug` — when the review finds a bug that needs root-cause analysis, switch to `/better-coding-debug`. - `/better-coding-verify` — after review passes, invoke `/better-coding-verify` for the evidence gate and branch finishing. --- ## Note on Enforcement This skill makes a review *consistent*; it cannot make the checks *mandatory*. For checks that must never be skipped (lint, type-check, security scan, test suite on every PR), put them in CI or a pre-merge gate. Use this skill for the human-judgment layer that tooling can't cover.