|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: Review code changes for correctness, security, performance and maintainability issues. Use when the user asks to review code, check a diff, look over recent edits, audit changes before committing, or find bugs in what changed. |
| 4 | +--- |
| 5 | + |
| 6 | +# Code review methodology |
| 7 | + |
| 8 | +This skill is the full review method; the /review, /commit and /pr commands apply shorter versions of it. The reviewer is the agent itself - every conclusion must come from actually reading the code, never from pattern-matching the diff shape. |
| 9 | + |
| 10 | +## 1. Establish the change set |
| 11 | + |
| 12 | +- Staged changes: `git diff --cached`. Unstaged: `git diff`. Branch vs base: `git diff <base>...HEAD -U10`. |
| 13 | +- Outside a git repository, review only the files the user named and say that repo-level checks (callers, history) were skipped. |
| 14 | +- Note the change size up front. Over ~400 changed lines, review in passes: correctness first, then security, then everything else. |
| 15 | + |
| 16 | +## 2. Read for context, not just diff |
| 17 | + |
| 18 | +- Read the full file around each hunk; a hunk that looks fine in isolation is often wrong in context. |
| 19 | +- When a function signature, return shape, or error behavior changes, grep for its callers and check each one. |
| 20 | +- Check adjacent code the diff touches but does not show (initialization, cleanup, config) before declaring a bug or clearing one. |
| 21 | + |
| 22 | +## 3. Checklist, in severity order |
| 23 | + |
| 24 | +**🔴 Correctness (ship-blockers)** |
| 25 | + |
| 26 | +- Logic errors and inverted conditions; edge cases: null/undefined/empty/zero/negative/huge input, unicode, concurrent access. |
| 27 | +- Error handling: caught-and-swallowed errors, wrong error types, promises without rejection handling, missing timeouts. |
| 28 | +- Resource leaks: files, connections, subscribers, locks not released on every path including the error path. |
| 29 | +- Public API or persisted-data changes that break existing consumers. |
| 30 | + |
| 31 | +**🔴 Security (ship-blockers)** |
| 32 | + |
| 33 | +- Hardcoded credentials, tokens, private keys; secrets interpolated into logs or error messages. |
| 34 | +- Injection: SQL, shell, XPath, XSS (unsanitized output), path traversal on user-supplied paths. |
| 35 | +- Missing authorization checks on new endpoints or actions; trusting client-side data. |
| 36 | +- Unsafe deserialization; crypto misuse (weak algorithms, non-random IVs, homegrown schemes). |
| 37 | + |
| 38 | +**🟡 Reliability & performance** |
| 39 | + |
| 40 | +- Unbounded work: N+1 queries, loops growing with request size, unpaginated list endpoints. |
| 41 | +- Hot-path allocations, repeated recomputation of loop-invariant values. |
| 42 | +- Missing retry/backoff on network calls; silent fallbacks that turn failures into wrong results. |
| 43 | + |
| 44 | +**🟡 Tests** |
| 45 | + |
| 46 | +- Behavior changed with no test change; new branches with no coverage. Suggest the specific missing test case, not "add tests". |
| 47 | + |
| 48 | +**🔵 Maintainability** |
| 49 | + |
| 50 | +- Misleading names, dead code, duplicated logic, magic constants. Only flag what a teammate would; skip personal style. |
| 51 | + |
| 52 | +## 4. Report |
| 53 | + |
| 54 | +- One line per finding: `file:line - issue - why it matters - suggested fix`. The fix must be concrete enough to act on. |
| 55 | +- Group by severity; explicitly state when a severity has no findings. |
| 56 | +- End with a verdict: overall risk, the short list that must be fixed before merge, what can wait. |
| 57 | +- Write the report in the user's language, matching the language of the conversation. |
| 58 | +- Review only. Do not edit files unless asked. If a secret appears in the diff, redact it to its first 4 characters, put the warning at the very top of the report, and advise rotation. |
0 commit comments