name: review

description: Scoped, single-category code review with convergence constraints. Use when performing targeted code review for specific issue types (security, bugs, dead-code, quality, simplify, tests, or diff-only reviews).

argument-hint: "[security | bugs | dead-code | quality | simplify | tests | diff]"

disable-model-invocation: true

allowed-tools: Read, Glob, Grep, Bash(git:*)

metadata:

author: cpplain

version: "1.0"

You are performing a **single-category code review** with hard constraints to ensure convergence. This skill exists because iterative whole-codebase reviews create infinite loops: each pass finds issues, fixes add new code, giving the next review fresh surface area.

The user invokes this skill with ONE category argument:

| Category | Scope | Key Constraint |

| ----------- | ----------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- |

| `security` | Exploitable vulnerabilities with concrete attack vectors | No "defense-in-depth" improvements |

| `bugs` | Logic errors causing incorrect behavior now | No "could be cleaner" suggestions |

| `dead-code` | Unreachable code, unused imports/vars/functions | Must verify zero callers via Grep |

| `quality` | Type errors, resource leaks, broken error recovery | No style, naming, or refactoring |

| `simplify` | Overly complex, over-abstracted, or over-engineered code | Prefer deletion/inlining; net line count must decrease |

| `tests` | Test quality and coverage gaps — flaky tests, meaningless assertions, over-mocking, untested critical paths | Improves existing tests; reports gaps but doesn't write new tests |

| `diff` | Only `+` lines from `git diff HEAD~1` | Ignores pre-existing issues entirely |

**If no argument provided**: Print this category table and stop. Do not perform a review.

**Category argument**: `${0}`

These constraints are **non-negotiable** and apply to every review:

1. **Maximum 7 issues per review** — Report only the 7 most severe issues. If you find more, defer them.

2. **Fixes modify existing code; they do not create new constructs** — If a fix requires adding new functions, classes, constants, or abstractions that did not exist before, it is NOT a fix for this review. Flag it as "Requires human review: [one-sentence explanation]". You have discretion on fix size, but not on fix shape.

3. **No surrounding refactoring** — Fix ONLY the specific issue. Do not clean up nearby code, rename variables, or improve formatting.

4. **Only broken or vulnerable code** — Do not report "improvable" code. If it works correctly, leave it alone.

5. **Stay in category** — A security review cannot report quality issues. A bugs review cannot report dead code. Strictly enforce category boundaries.

6. **No test changes** unless a test is currently broken. Do not modify tests to "improve coverage" or "make them clearer."

These are explicitly **not issues** for any category:

- "Extract this into a helper function"

- "Add a constant for this magic number"

- "Add input validation" (unless it's a current vulnerability)

- "Improve this error message"

- "Add type annotations / docstrings"

- "This could be simplified" (unless category is `simplify`)

- "Consider using X instead of Y"

- "Make this more consistent with..."

- "Add logging here"

- "This might be confusing to future developers"

If you find yourself wanting to report any of the above, **stop and reconsider**.

**Goal**: Find exploitable vulnerabilities with concrete attack vectors.

**Step-by-step**:

1. Identify all external input sources (CLI args, file reads, environment variables, API calls)

2. Trace data flow from external inputs to dangerous operations (command execution, file writes, path construction, eval-like operations)

3. For each potential issue, construct a concrete exploit scenario

4. Report ONLY if the exploit is realistic and achievable

**Do NOT report**:

- "Should validate input" without a concrete exploit

- "Defense-in-depth" improvements

- Theoretical vulnerabilities with no practical attack vector

- Missing input sanitization if the input source is already trusted

**Example valid issue**: "User-controlled `--output` flag is passed directly to `open()` without path validation, allowing directory traversal: `--output ../../etc/passwd` writes to arbitrary paths."

**Example invalid issue**: "Should validate that port is in range 1-65535" (not exploitable if the port comes from config file).

**Goal**: Find logic errors causing incorrect behavior under realistic inputs.

**Step-by-step**:

1. Verify return types match expected types

2. Check all code paths reach correct outcomes

3. Verify exception handling doesn't swallow critical errors

4. Check loop termination conditions

5. Verify conditional logic covers all cases

**Do NOT report**:

- "This could be cleaner"

- "Consider refactoring this"

- Edge cases that can't occur in practice

- Style or naming issues

- Performance problems (unless they cause timeouts/hangs)

**Example valid issue**: "Function returns `None` on error but caller expects boolean, causing `TypeError` at line 45."

**Example invalid issue**: "This function is hard to understand" (not a bug).

**Goal**: Find code that is provably unreachable or unused.

**Step-by-step**:

1. Use Glob to find all source files

2. For each candidate (unused import, function, class, variable):

- Use Grep to search the entire codebase for references

- Check if it's part of a public API (exported, documented)

- Check if it's a test utility used by multiple tests

3. Report ONLY if you can prove zero references exist

**Do NOT report**:

- Code that "looks unused" without verification

- Public APIs (even if unused internally)

- Test utilities

- Logging/debugging code

- Code used conditionally (feature flags, platform-specific)

**Example valid issue**: "Function `_legacy_formatter()` has zero callers in codebase (verified via `Grep`). Can be deleted."

**Example invalid issue**: "Function `export_csv()` is not called" (didn't check if it's a public API).

**Goal**: Find type errors, resource leaks, and broken error recovery.

**Step-by-step**:

1. Check error handling blocks for correct behavior

2. Verify resources (files, connections, locks) are properly released

3. Check type mismatches (string used as int, etc.)

4. Verify exceptions are caught at appropriate levels

**Do NOT report**:

- Style issues

- Naming conventions

- "Could be more Pythonic"

- Missing docstrings or comments

- Performance optimizations

**Example valid issue**: "File handle opened at line 23 is never closed if exception occurs at line 27. Use context manager."

**Example invalid issue**: "Variable name `x` is not descriptive" (style, not quality).

**Goal**: Find overly complex, over-abstracted, or over-engineered code. Every fix must result in fewer lines than before.

**Step-by-step**:

1. Find abstractions with only one caller (functions, classes, wrappers)

2. Find constants used exactly once

3. Find defensive validation for impossible cases (e.g., type checking after isinstance)

4. Find wrapper functions that add no value

5. For each issue, verify the simplified version has fewer lines

**Do NOT report**:

- Complexity that serves a purpose (multiple callers, legitimate abstraction)

- "Could be simplified" without proving line count reduction

- Abstractions that improve clarity (even if one caller)

**Fix requirement**: The fix must demonstrate NET line reduction. If removing an abstraction requires inlining more code than the abstraction saved, it's not a valid simplification.

**Example valid issue**: "Helper function `_validate_config()` has one caller and just calls two validation functions. Remove it and inline the calls (-5 lines)."

**Example invalid issue**: "Could extract this into a helper" (increases complexity).

**Goal**: Find test quality issues and coverage gaps. Two distinct parts.

**Part 1: Fix (report as issues with fixes)**

Step-by-step:

1. Find tests that don't assert anything meaningful (only assert function returns, no validation of correctness)

2. Find tests that over-mock dependencies so they don't test real behavior

3. Find tests with timing dependencies (sleeps, brittle timeouts)

4. Find tests that duplicate other tests exactly

5. For each issue, provide a fix that modifies the existing test

**Part 2: Coverage Gaps (report as "Coverage gap:" entries)**

Step-by-step:

1. Identify critical code paths with no test coverage:

- Error handling branches

- Security validators

- Edge cases in parsing/validation

- Conditional logic branches

2. Report each gap as: "Coverage gap: [description of what's untested]"

3. **DO NOT write the new tests** — just flag what's missing

**Do NOT report**:

- "Should add more tests" without specifics

- Missing tests for trivial getters/setters

- Coverage of internal implementation details

- "Could improve test clarity" (not a test quality issue)

**Example valid issue (Part 1)**: "Test `test_parse_config()` only asserts function doesn't raise exception. Add assertions to verify parsed values are correct."

**Example valid coverage gap (Part 2)**: "Coverage gap: `extract_commands()` error handling when shell command has unbalanced quotes is untested."

**Example invalid issue**: "Add more tests for this module" (too vague).

**Goal**: Review only the lines changed in the most recent commit.

**Step-by-step**:

1. Run `git diff HEAD~1` to get the diff

2. Extract ONLY lines starting with `+` (additions)

3. Apply all other review constraints to those lines only

4. Ignore all pre-existing code, even if it has issues

**Do NOT report**:

- Issues in unchanged code

- Issues in lines starting with `-` (deletions)

- Context lines (neither `+` nor `-`)

**Example valid issue**: "Line 45 (added in HEAD): variable `result` is unused after assignment."

**Example invalid issue**: "Line 20 has a security issue" (line 20 wasn't changed in HEAD~1).

Use this exact structure for each issue:

- **CRITICAL**: Security exploits, data loss, crashes

- **HIGH**: Logic errors, resource leaks, broken functionality

- **MEDIUM**: Dead code, quality issues, test gaps

After all issues, include:

If you found more than 7 issues, add:

1. **Validate category argument** — If `${0}` is empty or invalid, print category table and stop.

2. **Scope the review** — If category is `diff`, run `git diff HEAD~1` first to establish scope.

3. **Find issues** — Follow the category-specific process strictly. Use Glob/Grep/Read as needed.

4. **Apply hard constraints** — Filter to top 7, verify fixes don't create new constructs, eliminate anti-patterns.

5. **Format output** — Use structured format for each issue.

6. **Print summary** — Include issue count and deferrals if applicable.

This skill is designed to enforce convergence:

- **Capped issue count** prevents infinite work

- **No new constructs** prevents fixes from adding surface area

- **Category isolation** prevents scope creep

- **Anti-patterns list** prevents enhancement inflation

- **Diff mode** enables steady-state reviews of just new changes

After a fix commit, run `/review diff` to verify the fixes didn't introduce new issues. The review should find fewer issues each time, confirming convergence.