review-code

Review Code

Review a target and report findings. Each finding is rated by severity (how bad if it happens), likelihood (how often the bad path actually runs), and a worth-fixing verdict derived from both — plus a high-level fix when worth fixing. This skill reports only; it never edits code unless the user explicitly asks afterward.

Resolve the target

Pick the target from the argument, ignoring any flags. Default to diff when no target is given.

1. diff (default) — local work on the current branch: uncommitted edits, unpushed commits, and already-pushed commits, reviewed together. Anchor on the base-branch fork point, not the branch's own remote — otherwise commits drop out of the diff once they are pushed. Include untracked files, which a plain diff omits.
2. pr <number> (or a PR URL) — review a GitHub PR: its diff plus enough PR context (title, description, base branch) to judge intent. If the GitHub CLI is unavailable, say so and fall back to diff.
3. all / codebase — review the whole repository. State the scope you can realistically cover and prioritize entry points, core logic, and recently changed files. Note anything skipped.
4. <path> — a file or directory argument scopes the review to that path.

For a diff/PR, review the changed lines plus enough surrounding context to judge them (callers, related functions). A bug is in scope even if the changed line only exposes it.

Where to run the review

Default is inline. With --sub, run the whole review in a subagent and relay its report verbatim — use it for all/codebase or a large diff, where reading the files would bloat the session. Never re-review or paste back what the subagent returns. If the harness has no subagents, say so and review inline.

Review lens (in priority order)

1. Correctness — logic errors, off-by-one, nil/null derefs, wrong conditionals, race conditions, unhandled errors, incorrect API usage, broken edge cases.
2. Security — injection, missing authz/authn, secrets in code, unsafe deserialization, path traversal, unvalidated input.
3. Data & resources — leaks (fd/memory/goroutine), unbounded growth, missing transaction boundaries, N+1 queries.
4. Performance — needless allocations, O(n²) on hot paths, blocking calls in loops.
5. Codebase fit — a second pattern for something the repo already solves (HTTP client, error style, config), logic in the wrong layer, or code size out of proportion to what it delivers. Favor a small surface: few methods, simple params, complexity hidden inside. Deletion test for new abstractions: if deleting one makes complexity vanish, it was a pass-through; if complexity reappears across callers, it earns its keep.
   • No unnecessary Go interfaces. A small surface is a concrete type with few methods, not the interface keyword. Declare one only when two real implementations exist or the real dependency cannot run in a test (third-party API, remote service). Flag single-implementation interfaces as indirection.
6. Maintainability — duplication, dead code, unclear naming, missing-but-needed tests. Report these only when they materially hurt; do not pad the report with style nits.
7. Tests — too many unit tests are a liability, not an asset. Flag tests that mostly exercise mocks/fakes rather than real behavior, tests that pin implementation details so any refactor breaks them, and redundant cases that add maintenance cost without catching new bugs. Prefer fewer tests against real components (integration-style, in-memory DB, real HTTP handler) over many mock-heavy ones. Do not ask for more unit tests by default; ask only when a real bug path is uncovered. After a refactor, tests on pieces that became internal should be deleted, not kept beside the new ones. If the repo already uses testify, flag hand-rolled if got != want { t.Fatal(...) } blocks.

Do not invent problems. If the code is clean, say so. Prefer a few high-confidence findings over many speculative ones.

Rating each finding

Severity — impact if the bad path executes:

• Critical — data loss/corruption, security breach, crash on a common path, wrong results affecting users or money.
• High — wrong results or crash under realistic conditions; security issue needing some precondition.
• Medium — degraded behavior, perf regression, or a correctness bug on a rare path.
• Low — style, readability, minor inefficiency with no functional impact.

Rate a test finding by the risk it hides (the real path left unverified), not by the test's own impact.

Likelihood — how often the triggering condition is actually met:

• High — hit by normal usage or common inputs.
• Medium — uncommon but realistic inputs/timing/config.
• Low — requires rare, adversarial, or near-impossible conditions.

Worth fixing — derived from severity × likelihood, adjusted for fix cost:

                 Likelihood
Severity     High        Medium      Low
Critical     Yes         Yes         Yes
High         Yes         Yes         Judgment call
Medium       Yes         Judgment    No (note only)
Low          Judgment    No          No

• Yes — recommend fixing; include a fix.
• Judgment call — explain the trade-off (fix effort vs. payoff) and give a recommendation; include a fix.
• No — note it for awareness; omit the fix or keep it to one line.

Fix effort — rate whenever a fix is included (Yes / Judgment call); omit for No:

• Trivial — a few lines, one file, no design change.
• Small — localized change, under an hour of work.
• Medium — touches several files or needs new tests.
• Large — refactor, design change, or risky migration.

Bump a Judgment call to Yes when the fix effort is Trivial; drop toward No when it is Large or risky relative to payoff. State the reason when you override the matrix.

Report format

Lead with a one-line summary and a table sorted by worth-fixing (Yes first), then severity. Then one block per finding.

For diff/pr, add a Net effect line: weigh what the change removes or simplifies against what it adds. It improves when callers learn less and changes land in one place; it degrades when it adds shallow layers or spreads one concern across callers. Default to Neutral without concrete evidence either way.

**Reviewed:** <target> — <N files, what was covered>
**Summary:** <one line: overall health + count of must-fix findings>
**Net effect:** Improves | Neutral | Degrades — <one-line reason>

Then for each:

End with: **Not worth fixing right now:** <one-line list> if any No/low items were folded out, so nothing is silently dropped.

Verification procedure

Before sending the report, check:

1. Every finding cites a real location (file:line) you actually inspected — no hypothetical line numbers.
2. The worth-fixing verdict matches the matrix, or you stated why you overrode it.
3. Every "Yes" and "Judgment call" has a concrete fix and a fix-effort rating; every fix is high-level (approach, not a finished diff), and none was applied to the working tree.
4. Clean code is reported as clean. If you found nothing worth fixing, say that plainly instead of manufacturing Low findings.

Common mistakes to watch for

• Conflating severity with likelihood. A SQL injection reachable only by an admin is High severity / Low likelihood — rate the two axes separately.
• Reviewing only changed lines. A diff can introduce a bug whose root cause is in unchanged code a caller away; read enough context to judge it.
• Guessing the target state. If the tools return nothing or error out, report that instead of reviewing an empty target.
• Treating test count as quality. A PR with many mock-based unit tests can still be untested where it matters. Judge tests by what real behavior they pin down, and call out mock/fake-heavy tests as a maintainability cost.