Code Review for AI Agents v4
codecode-reviewbest-practicesbooks-for-agents
Add to your library first to use in Claude Code
Pages680
Formatmarkdown
ListedMarch 31, 2026
UpdatedJuly 21, 2026
Subscribers145
About
Complete reference for AI coding agents performing code reviews. 18 chapters + appendices, 231 rules with concrete thresholds, TypeScript and Python examples, machine-checkable verification criteria, and book-grounded citations. Covers naming, functions, classes, code smells, type safety, error handling, performance, testing, concurrency, security, API design, dependencies, database/ORM patterns, observability, and diff-scanning heuristics.
Part of the Pro Library
Get full access to this book and 6 others with CandleKeep Pro. Preview the first 2 pages below.
680Chapters
1100Topics
680Pages
Preview
Code Review for AI Agents
A complete reference for AI coding agents performing code reviews. 18 chapters + appendices covering naming, functions, classes, code smells, type safety, error handling, performance, testing, concurrency, security, API design, dependencies, and review comment writing. ~231 rules with concrete thresholds, TypeScript and Python examples, and machine-checkable verification criteria. Language-agnostic.
Add to library to read more
Table of Contents
Code Review for AI Agents
BAD -- generic name in a 30-line function
GOOD -- says what the data actually is
BAD -- function name is vague
BAD -- "flag" says nothing
BAD -- magic string repeated in multiple places
GOOD -- single source of truth
BAD -- status codes scattered as literals
BAD -- three lines of commented-out code nobody understands
conn = db.connect(host="old-server", port=5432)
cursor = conn.cursor()
cursor.execute("SELECT * FROM legacy_table")
BAD -- crutch comment for a bad name
GOOD -- self-documenting name, no comment needed
GOOD -- comment explains the non-obvious
GOOD -- comment explains why, not what
We sort by created_at descending because the UI shows newest first,
and the database index on created_at makes this O(log n) not O(n log n).
GOOD -- comment documents a workaround
Workaround for Chrome bug #123456: requestAnimationFrame fires twice
BAD -- vowel-dropped abbreviation
BAD -- single-letter name for module-level variable used across 200 lines
GOOD -- descriptive for its scope
BAD -- class name is one character
BAD -- mixed conventions
BAD -- unused import and unused variable
GOOD -- only what's needed
BAD -- feature-flagged dead code without ticket reference
BAD -- "process" says nothing about what happens
GOOD -- specific verb
BAD -- negated boolean name + negated check
BAD -- `result` reused for three different things
BAD -- does clip() modify the text or return a new string?
BAD -- bare FIXME, impossible to prioritize or assign
FIXME: this is slow
GOOD -- owner attached, can be searched and assigned
BAD -- hardcodes a value that lives in settings.py
Max retries is 5 (configured in settings.py)
BAD -- mixed casing in the same module
BAD -- orphaned TODOs that will never be addressed
TODO fix this later
FIXME not sure why this works
HACK temporary
GOOD -- every TODO has accountability
TODO(CND-423): handle edge case where user has no email
FIXME(@bob, 2025-06-01): replace with batch API once available
HACK(GH-112): workaround for sqlalchemy bug #4521, remove after upgrade
BAD -- long function doing validation + transformation + persistence
BAD -- 5 positional args
BAD -- flag toggles behavior
BAD -- "validate" suggests pure, but it modifies the order
BAD -- deeply nested
BAD -- fetch + parse + filter in one function
BAD -- deep nesting from lack of early returns
BAD -- business logic mixed with HTTP details
BAD -- repeated date formatting in two functions
BAD -- name says "calculate" but it also modifies the order
BAD -- set_and_return pattern violates CQS
Later:
GOOD -- separated
BAD -- mutates the input list (output argument)
Caller:
GOOD -- returns a new dict with defaults applied
BAD -- complex boolean expression repeated in two places
BAD -- temporal coupling with no enforcement
Caller must "just know" the order:
GOOD -- each step returns what the next step needs
BAD -- enum selects completely different notification strategies
GOOD -- separate functions, dispatchable if needed
BAD -- loop variable modified inside body
BAD — user cancel checks ownership, admin cancel skips all checks
BAD -- caller gets None, forgets to check, AttributeError in production
GOOD -- use Optional with explicit type hint
BAD -- business logic buried inside try/except
BAD -- nested callbacks via callback-passing style
BAD -- mutable default is shared across all calls
GOOD -- use None sentinel and create inside the function
BAD — handles both user authentication AND email sending
BAD — 8 fields, low cohesion
BAD — function reaches into another object's internals
BAD — refused bequest: overrides parent to disable
BAD — 4 levels of navigation
BAD — shallow: wrapper adds no value, just passes through
BAD — circular dependency
file: order.py
file: customer.py
GOOD — break cycle with interface or restructure
file: types.py
BAD — middle man adds nothing
BAD — constructor does I/O and can fail
BAD — temporal decomposition leaks protocol details
BAD — data class with no behavior
Logic lives elsewhere
BAD — abstract class with one subclass, unused parameter
BAD — no-arg constructor, required setters to reach valid state
GOOD — valid at construction
BAD — returning internal list reference
Caller bypasses validation:
BAD — protocol forces implementors to stub unused methods
BAD -- shallow: 6 public methods, each 2-3 lines
BAD -- both the sender and receiver know the message envelope format
sender.py
receiver.py
GOOD -- single module owns the envelope format
BAD -- pass-through that adds nothing
BAD -- hybrid: public attributes + business methods
BAD — deeply nested
BAD — near-duplicate: same structure, different field names
BAD — money as float, unit confusion
BAD — date range as separate params everywhere
BAD — repeated if-else on shape type
Shotgun Surgery — one change, many files
BAD: adding a new "premium" user tier requires changes in:
- models/user.py (add tier enum value)
- services/billing.py (add pricing logic)
- templates/dashboard.html (add premium badge)
- api/serializers.py (add tier field)
- tests/test_billing.py (add premium test)
GOOD: consolidate tier logic into one module
models/user_tier.py — single source of truth for tier behavior
BAD — module-level global modified from multiple functions
Bug: in tests, state leaks between test cases; in production, thread safety issues
BAD — field set only during parsing, None otherwise
BAD — imperative accumulation
BAD — trivial wrappers that add navigation cost, not clarity
BAD -- money as float; mixing currencies is invisible
Bug: charge_customer(price_in_yen, "USD") -- no type error
GOOD -- value object with validation
BAD -- function in shipping.py but mostly touches order fields
shipping.py
BAD -- bidirectional: auth imports user, user imports auth
auth.py
user.py
GOOD -- extract interface or pass data
BAD — no type annotation, anything goes
BAD
BAD — returns mutable internal list
BAD — trusts external JSON
BAD
BAD — cast without validation
BAD — equality for None check
BAD — raw strings
BAD — raw types, duplicated validation
BAD — list with repeated membership checks
BAD — list of tuples used as key-value store
GOOD — set for deduplication and membership
BAD — raw primitives, no protection against invalid values
BAD — stored derived field
line.length is still 10 — stale!
BAD -- missing variant, no exhaustiveness check
BAD -- unchecked cast silences the type checker
GOOD -- validate with Pydantic at the boundary
BAD — swallows all errors
BAD — bare except catches SystemExit, KeyboardInterrupt
BAD — fails with AttributeError deep in processing
BAD — file handle leaks on exception
BAD — EAFP overused for expected case
BAD
BAD — fixed delay, no limit
BAD — cleanup exception replaces original
BAD — implicit assumption with no assertion
GOOD — assertion for internal invariant (NOT user input)
BAD — constraints in docstring only, no enforcement
BAD — user input in error response
BAD — raw input in raised exception
GOOD — generic client error, details server-side
BAD — raises when the desired state already holds
Caller must:
if is_subscribed(user_id, topic): # race-prone!
unsubscribe(user_id, topic)
BAD — duplicated error handling in every endpoint
BAD — abstraction leak
BAD — returns default on error, caller can't tell the difference
BAD — no default, new role silently ignored
BAD -- FastAPI route with no error handling; unhandled exception returns 500 with stack trace
BAD -- bare Exception with no structure
GOOD -- typed error hierarchy with context
Caller can handle each case distinctly
BAD — N+1: one query per post
BAD — loads all rows into memory
BAD — returns all records
BAD — blocking HTTP call in async handler
BAD — overlapping alternation with quantifier
BAD — O(n^2) string concatenation
BAD — recomputes on every call
BAD — set grows unbounded
BAD — no timeout, blocks indefinitely
BAD — debug log in tight loop, string formatting always runs
BAD — new date object created on every iteration
BAD — JSON encode/decode in high-throughput message handler
BAD — premature optimization sacrificing readability
"Using bitwise shift for division by 2 — it's faster"
BAD — nested conditionals for multi-dimensional lookup
BAD — parallelizes (level 5) without considering "do less" (level 3)
BAD -- imports entire module for one function
BAD -- wildcard import pollutes namespace and obscures dependencies
GOOD -- import only what you need
BAD -- observer started without cleanup
GOOD -- cleanup via context manager or explicit stop
Usage: cleanup guaranteed
BAD — no assertion
BAD — asserts on internal call order
BAD — shared list mutated across tests
BAD — over-mocked, more mocks than assertions
BAD — depends on network and external service
BAD — tests only the normal case
BAD — method-name tests
BAD — mirrors the implementation
BAD — asserts on internal call order and arguments
BAD — tests a dataclass constructor
BAD — class-level mutable fixture
BAD — mock hides real query behavior
BAD — only 2 hardcoded cases for a serializer
BAD — tests internal call sequence
BAD — mocking the repository (managed dependency)
BAD — SQLite for tests, PostgreSQL in production
BAD — datetime.now() buried in domain logic
GOOD — time as explicit parameter
BAD — conditional assertion
BAD -- snapshot of entire serialized response (100+ lines)
BAD -- only happy path
BAD -- interleaved actions and assertions
GOOD -- one behavior per test, clear AAA structure
BAD — shared dict modified from multiple threads
BAD — check-then-act race on file existence
BAD — coroutine created but never awaited
BAD — lock ordering violation
BAD — CPU-bound work in async handler
BAD — unbounded concurrency
BAD — fire-and-forget coroutine with no error handling
BAD — background task with no cancellation
BAD — sequential writes without transaction
BAD — read-modify-write without protection
BAD — write skew on booking overlap check
BAD — phantom: both transactions see zero conflicts, both insert
GOOD — unique constraint + handle violation
BAD — counter increment is not idempotent
BAD — threading interleaved with computation
BAD — f-string SQL injection
BAD — password in source
BAD — no authorization check (any authenticated user can access any user's data)
BAD — rendering user input without escaping in Jinja2
BAD — directory traversal
BAD — user input flows to eval (code injection)
BAD — config.yaml committed to git with real secrets
config.yaml
aws:
access_key_id: "AKIAIOSFODNN7EXAMPLE"
secret_access_key: "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"
GOOD — reference environment variables in config
config.yaml
aws:
access_key_id: ${AWS_ACCESS_KEY_ID}
BAD — no timeout on external call
BAD — no ownership check
BAD — mass assignment via **kwargs
BAD — merging user input into config dict
BAD — decorator-based auth (opt-in; forget decorator = public endpoint)
BAD — trusts Content-Type, uses user filename
BAD — startsWith bypass with //evil.com
BAD — no signature verification on webhook
BAD — introspection enabled, no limits
GOOD — disable introspection, add depth validation
BAD — no security headers in Flask
BAD — token as plain string
BAD — changing a required field to optional breaks consumers expecting it
v1: {"status": "active", "email": "[email protected]"}
v1 modified: {"status": "active"} # email removed — breaks consumers
BAD — no validation
BAD — POST creates duplicate on retry
BAD — no pagination
BAD — adding a required field to a Pydantic model used in API serialization
BAD — deprecated without alternative
BAD — removing a field from API response breaks old clients
v1 response: {"name": "Alice", "email": "[email protected]", "legacy_id": 42}
v2 response: {"name": "Alice", "email": "[email protected]"}
Old mobile app crashes: KeyError: 'legacy_id'
GOOD — deprecate, retain, then remove after transition
v2 response: {"name": "Alice", "email": "[email protected]", "legacy_id": 42}
^^ still included but documented as deprecated
BAD — DELETE with body
GOOD — query params for filtered delete
BAD -- rate limit exists but 429 response has no retry information
GOOD -- rate limit headers on every response, clear 429 with Retry-After
BAD — circular import
models/user.py
models/order.py
BAD — model layer imports from Flask
models/user.py
GOOD — model is framework-agnostic
BAD — adding requests just to make one HTTP call when httpx is already in the project
requirements.txt
GOOD — use what's already available
BAD — importing private module
GOOD — use public API
BAD — importing from vendored sub-package
BAD — hardcoded database URL
GOOD — from environment with validation
BAD — using a sub-dependency of requests
GOOD — add to requirements.txt
requirements.txt:
BAD — god module
utils/__init__.py that re-exports everything
Every import of `from utils import format_date` triggers loading ALL of utils
BAD — config threaded through functions that don't use it
BAD — business logic directly imports concrete email library
services/notification.py
GOOD — depend on a protocol
ports/email.py
services/notification.py
BAD -- boto3 used directly in 6 files
upload.py
download.py
GOOD -- wrapped in a storage adapter
BAD -- factory pattern with one product
Only EmailNotification exists. The factory is dead weight.
GOOD -- direct instantiation until a second type appears
BAD -- adding a new user field requires changes in 4 files
models.py: add field
serializer.py: add field to serializer
form.py: add field to form
template.html: add field to template
All four files "know" about user fields -- not orthogonal
GOOD -- auto-generated forms and serializers from model
models.py: add field (single source of truth)
serializer.py: fields = "__all__" (auto-includes new fields)
BAD -- 6 required config parameters for a connection pool
GOOD -- sensible defaults; user only overrides what they understand
BAD — vague comment
BAD — no actionable fix
BAD -- N+1: one query per order to get the customer
BAD -- deep eager-load chain
BAD -- loads entire Post objects
BAD
BAD -- HTTP call inside transaction
BAD -- no statement timeout, long-running transaction can block others
BAD -- scalar_one_or_none() returns None, next line crashes
BAD -- partial write if second insert fails
BAD -- no migration history, may drop data
GOOD -- versioned, reviewable migration
Review the SQL in prisma/migrations/
BAD -- Django syncdb / Prisma db push equivalent
python manage.py syncdb # deprecated and dangerous
GOOD -- Django migrations
python manage.py makemigrations # creates migration file
python manage.py migrate # apply to staging first
BAD -- apply untested DDL directly to production
GOOD -- test on staging, then apply to production
BAD -- direct connection from Lambda
BAD -- default pool size
BAD -- new engine on every module reload
GOOD -- module-level singleton (Python doesn't have hot reload issues as often,
BAD -- separate STRING keys for each field
Reading all fields: 3 round trips
GOOD -- single HASH key
BAD -- no expiration
GOOD -- with TTL
BAD -- large list cached as single value
GOOD -- paginated cache or store in DB, cache IDs only
BAD -- f-string SQL injection
BAD -- string format
GOOD -- parameterized query with psycopg
BAD -- relative import reaching outside package boundary
In services/api/handlers/user.py:
fragile: depends on directory depth and internal structure
GOOD -- import via installed package name
In services/api/handlers/user.py:
myorg_shared is declared in pyproject.toml as a workspace dependency
BAD — unstructured print, no level, no timestamp
BAD — basic logging with string formatting, not machine-parseable
GOOD — structlog with bound context
BAD — validation failure logged as ERROR
BAD — database connection failure logged as WARNING
BAD — no correlation, logs from concurrent requests are interleaved and inseparable
GOOD — correlation ID middleware with contextvars
BAD — logs entire request payload including password
BAD — logs the auth token
GOOD — log only safe identifiers
BAD — per-row logging in data pipeline
BAD — bare error message
BAD — just the exception, no business context
BAD — inconsistent names, no units
BAD — database query with no timing
BAD — alert with no remediation guidance
BAD — f-string message, every entry is unique
"User u_123 signed up via google" — unique, ungrepable without regex
BAD — %-format in the message field
Produces unique strings; cannot group or count in log aggregator
GOOD — static event with structured context (structlog)
Event is always "user_signed_up" — searchable, countable, alertable
GOOD — static message with extra dict (stdlib logging + JSON formatter)
BAD — no timeout; hangs indefinitely if server is slow
BAD — only connect timeout, no read timeout
GOOD — explicit (connect_timeout, read_timeout) tuple
BAD — retry loop without idempotency key
BAD — fixed 1-second interval; all instances retry in sync
BAD — retries all HTTP errors including permanent ones
BAD — direct call; 1000 concurrent requests pile up when inventory is down
BAD — dual write with no atomicity guarantee
BAD — ReportingService reads InventoryService's private tables
In reporting-service:
GOOD — ReportingService queries a local read model updated via events
inventory-service publishes StockUpdated events
BAD — property access transparently calls remote service
BAD — synchronous-looking name and return type for a network call
BAD — removes a required field from Pydantic model; breaks old message readers
BAD — changes field type; breaks all existing data
BAD — Queue() with no maxsize = unbounded
BAD — discovers missing S3 credentials during processing, after acquiring DB lock
BAD — trivial health check; load balancer can't tell if we're really healthy
BAD — pricing service outage kills the product page
BAD — shallow module; caller must understand internals
BAD — UserUpdatedEvent duplicated across services, drifted between them
In user-service/events.py:
In analytics-service/events.py: (stale copy — missing `tier`)
GOOD — shared schema package published to internal PyPI
Package: mycompany-events (pip install mycompany-events==1.3.0)
mycompany_events/user.py:
In user-service:
19.1 — Find HTTP calls without timeout
19.2 — Find retry loops without idempotency key
Manually verify each file contains "idempotency" or "Idempotency-Key"
19.3 — Find sleep/delay in retry logic without exponential/jitter
19.5 — Find integration points without circuit breaker
19.6 — Find dual writes (DB write followed immediately by event/message send)
19.10 — Find unbounded queues
19.12 — Find health check endpoints that only return 200