Code Review

Code Review Skill

Quality and semantic review. Separate from security-gate (which is an OWASP pass) — this reviews logic, correctness, and maintainability.

What Claude Gets Wrong Without This Skill

Without explicit semantic review, Claude's code review focuses on what it can see at a glance — naming, formatting, obvious bugs. It misses logical errors that are invisible unless you trace execution, tests that assert the wrong thing, and anti-patterns that only become problems under load or edge cases.

Semantic Anti-Patterns to Actively Check

These are the patterns that pass linting, pass type checking, and still cause production incidents:

Silent failure

# WRONG — exception swallowed, caller never knows it failed
try:
    result = process(data)
except Exception:
    pass

# RIGHT — at minimum, log and re-raise or return an error

Boolean parameter flags

# WRONG — caller has no idea what True means
render_page(user, True)

# RIGHT — use explicit keyword args or separate functions
render_page(user, include_draft=True)

Mutable default arguments

# WRONG — default list is shared across all calls
def append_item(item, items=[]):
    items.append(item)
    return items

# RIGHT
def append_item(item, items=None):
    if items is None:
        items = []

Late error detection Code that validates input halfway through execution after already modifying state. Validate first, act second.

Asymmetric error handling Some code paths return None on failure, others raise exceptions, others return False. Callers must handle all three.

Test that tests the mock

# WRONG — this test only proves the mock works
mock_service.process.return_value = {"status": "ok"}
result = handler.run()
assert result == {"status": "ok"}  # this just asserts the mock returned what it was told to

Implicit ordering dependency Code that works only if methods are called in a specific order, with no enforcement of that order.

Callback hell / pyramid of doom Deeply nested conditionals or callbacks that make control flow impossible to follow.

Logic Tracing Protocol

For critical paths (auth, payment, data writes), trace the execution manually:

1. Identify entry point
2. Follow the happy path — does it reach the expected outcome?
3. Follow the primary failure path — is the error handled correctly?
4. Follow the edge case path — what happens with empty input, null, zero, max value?

Verdict Format

APPROVE: Ready to merge. State any minor observations that do not require changes.

REQUEST CHANGES:

Issue: [description]
Location: [file:line]
Category: [semantic / logic / error-handling / test / readability]
Fix: [specific recommendation]
Blocking: No

BLOCK:

Issue: [description]
Location: [file:line]
Category: [category]
Impact: [what breaks or could break]
Fix: [specific recommendation]
Blocking: YES

Adversarial Mode (/code-review --adversarial)

Standard mode asks: "What is wrong with this code?" Adversarial mode asks: "Why is this the wrong approach entirely?"

Use adversarial mode when you want the implementation challenged, not just inspected. The output argues against the code. The user decides whether the argument holds.

Trigger: user invokes with --adversarial flag or asks for adversarial review.

Questions to answer:

1. Wrong abstraction? Is the chosen abstraction level correct, or does it leak internals / hide too much?
2. Tests testing the right thing? Are tests asserting behavior or implementation details? Would a rewrite break the tests even if behavior is preserved?
3. Over-engineered? Is this solving a problem that doesn't exist yet? What is the simplest version that satisfies the actual requirement?
4. Hidden assumption? What assumption does this code make that, if wrong, causes it to fail entirely?
5. Complete rewrite trigger? What real-world scenario would force a complete rewrite of this approach?
6. Early-termination risk? Is there a reasoning gap in this code that would cause an agent or process to exit early before the task is complete? (e.g., missing error branches that return prematurely, incomplete state machines, loops that exit on the wrong condition)

Scoring rubric (after answering all questions):

Score 1 = fundamentally broken; 5 = no concerns. Scores of 1-2 on any dimension = ARGUMENT HOLDS.

Nudge step (mandatory):

After scoring, identify the single assumption in the implementation most likely to be wrong. Challenge it explicitly:

Nudge: The implementation assumes [specific assumption]. If this is false, [specific consequence].
Second-pass question: [one question the author must answer to validate or invalidate this assumption]

GAA Loop (Generator-Attacker-Analyzer):

Run adversarial mode as a structured 3-role loop:

1. Generator -- the existing implementation (the code under review)
2. Attacker -- the adversarial pass above; finds counterexamples and failure cases
3. Analyzer -- for each surviving challenge, add a specific constraint that closes the gap

Loop until no new counterexample survives (max 2 iterations). On the second loop, if the same challenge reappears unresolved, escalate to BLOCK regardless of original verdict.

Output format:

ADVERSARIAL VERDICT: [ARGUMENT HOLDS / ARGUMENT WEAK]

Scores: Correctness [N]/5 | Completeness [N]/5 | Feasibility [N]/5

Challenge 1 — [abstraction / tests / complexity / assumption / fragility / early-termination]:
[Specific argument against the implementation]
Evidence: [file:line]
Analyzer constraint: [what must change to close this gap]

Challenge 2 — ...

Nudge: [assumption] — [consequence if false]
Second-pass question: [question]

User decision required: Accept / Reject each challenge.

Do not produce both standard and adversarial output in the same run. They are separate passes.

Anti-Patterns

Do not combine code-review with security-gate. They are separate passes. Flag security concerns and recommend running security-gate — do not attempt both in one pass.

Do not issue REQUEST CHANGES without specific locations and fixes. "This could be cleaner" is not actionable feedback.

Do not approve code with untraced critical paths. If you did not trace auth, payment, or data write paths, say so in the scope limitations.

Mandatory Checklist

1. Verify all semantic anti-patterns were actively checked (not assumed absent)
2. Verify critical paths were traced (auth, payment, data writes — wherever applicable)
3. Verify every REQUEST CHANGES or BLOCK issue has a file:line and specific fix
4. Verify tests were checked for what they actually assert, not just that they exist
5. Verify verdict is one of: APPROVE / REQUEST CHANGES / BLOCK
6. Verify scope limitations were stated (what was not reviewed)