code-review-standards

Code Review Standards

Purpose

This skill defines the structured checklist that the code-critic agent applies during Stage 4 of the code production pipeline. The checklist is severity-tagged so that PM and engineer both know exactly which findings block delivery and which are advisory. Engineers may load this skill for self-review before requesting a critic pass.

The checklist exists because unstructured code review in multi-agent systems produces inconsistent signal: one critic dispatch flags naming; another flags security; neither flags the same things. Severity tagging makes the review deterministic across dispatches.

The Severity-Tagged Checklist

CRITICAL (must fix, blocks delivery)

• No secrets, API keys, or credentials hardcoded
• No SQL injection vectors (parameterized queries only)
• No arbitrary code execution paths (no eval, exec, unrestricted pickle.loads)
• Authentication/authorization not bypassable
• No infinite loops without escape conditions

HIGH (must fix, blocks delivery)

• Type hints on all public functions and classes
• mypy --strict passes with zero errors
• pytest passes with zero failures
• Test coverage >= 90% on new code
• No bare except clauses
• No mutable default arguments
• No global mutable state
• No synchronous I/O inside async functions
• No N+1 query patterns
• Error cases handled explicitly (not silently swallowed)

MEDIUM (flag, note in report, proceed)

• Functions <= 20 lines (prefer <= 10)
• No nested loops where hash map would reduce complexity
• list.pop(0) replaced with deque.popleft() where relevant
• asyncio.gather uses return_exceptions=True where appropriate
• Async operations have explicit timeouts
• Docstrings on public methods (Google or NumPy style)
• No Any types in production code paths

Efficiency (see criteria-efficiency.md):

• No nested loops over two collections that should be a hash-map lookup (O(n*m) → O(n+m))
• No per-iteration I/O (queries/RPCs/fetches inside a loop) — batch outside the loop (HIGH on hot paths; see "No N+1 query patterns")
• Repeated deep property/selector resolution cached in a local (no greedy data access)
• String accumulation in loops uses list+join / StringBuilder, not +=
• No SELECT * / over-fetching in production query paths