Code Review Standards

Intent

This skill makes every code block produced in this session pass an internal principal-engineer code review before it is shown to the user. It is a meta-skill: it does not govern one domain but governs the quality of all output across every domain. The self-review checklist below must be applied mentally before outputting any code block. Code that would fail this checklist must be corrected before output — not flagged as "a potential improvement" but fixed. This skill also defines the naming, commenting, and security standards that apply universally to all code in this session.

This skill is never overridden by domain-specific skills. It applies universally.

1. Self-Review Checklist

Apply this checklist internally to every code block before outputting it. If any item fails, fix it first.

Correctness

• Does this function do exactly one thing? If it does two things, split it.
• Are all edge cases handled: empty input, null/None, zero, negative numbers, empty collections, single-element collections, maximum input size?
• Is the logic correct for the boundary conditions? (Off-by-one errors, inclusive/exclusive ranges, ≤ vs <?)
• If there is concurrency, are race conditions possible?
• If there is recursion, is the base case correct and is stack overflow possible?

Complexity and Performance

• Is the time complexity acceptable for the expected input size? (Document it if not obvious)
• Is there a hidden O(n²) or worse operation? (Nested loops, in on a list, repeated sort)
• Is there an N+1 query pattern? (Database query or API call inside a loop)
• Are large objects unnecessarily held in memory when streaming/chunking would suffice?
• For ML code: are there Python loops over arrays that should be vectorized?

Robustness and Error Handling

• Is the error handling correct and informative? (Specific exceptions, context in the message)
• Are all external inputs validated before use?
• Are all system boundaries (API handlers, CLI entry points) catching exceptions properly?
• Is there any silent failure? (Empty except, swallowed exception, None returned on error without documentation)

Readability and Maintainability

• Would a new team member understand this code in 60 seconds without asking questions?
• Are all magic numbers replaced with named constants?
• Does the naming convey intent? (No data, result, temp, x, thing in non-trivial code)
• Are comments explaining WHY, not WHAT? (The code explains what)
• Is the function/method length reasonable? (40 lines max for Python; flag anything longer)

Security

• Are there any SQL injection risks? (String interpolation in queries → must use parameterized queries)
• Are there any path traversal risks? (User input in file paths → must sanitize)
• Are there any XSS risks? (User input rendered as HTML → must escape)
• Are there any secrets in the code output? (API keys, passwords, tokens → must not appear)
• Are any user inputs deserialized without validation? (Pickle deserialization → refuse; JSON → validate schema)
• Are external commands constructed from user input? (Shell injection → must use argument arrays, not string interpolation)

Testability

• Is the code testable? (No hidden global state, dependencies are injectable)
• Does the function have a single, testable return path?
• Are all dependencies mockable via interface or injection?

2. Naming Conventions — Universal Rules

Functions and Methods

• Always use verb phrases: calculate_revenue, fetch_user_by_id, validate_payment, parse_config
• Avoid: process, handle, do_thing, run — these are not descriptive enough
• Async functions: same rules; do not prefix with async_ — the async def keyword is sufficient

Boolean Variables and Functions

• Always use is_, has_, can_, should_, was_, needs_ prefixes:
  • is_active, has_premium_subscription, can_withdraw, should_retry
  • Predicate functions: is_valid_email(email), has_sufficient_balance(account)
• Never: flag, check, status as boolean names

Classes

• Always nouns or noun phrases, PascalCase: UserRepository, ChurnPredictor, TransactionService
• Avoid: Manager, Helper, Util, Handler as class name suffixes — these indicate unclear responsibility

Constants

• SCREAMING_SNAKE_CASE: MAX_RETRY_ATTEMPTS, DEFAULT_BATCH_SIZE, CHURN_PROBABILITY_THRESHOLD
• Never: magic numbers inline in code

Variables — Forbidden Names in Non-Trivial Code

The following are forbidden as variable names outside of well-understood mathematical or loop contexts: data, result, temp, tmp, x, y, z, thing, obj, item (unless in a comprehension over a clearly named collection), val, value (use the semantic name instead), info, stuff

Acceptable Single-Letter Variables

Single-letter variables are only acceptable in:

• Loop indices over integer ranges: i, j, k
• Mathematical formulas where the letter has a standard meaning: n for count, m for rows, k for clusters
• Lambda expressions with trivially obvious meaning: sorted(items, key=lambda x: x.timestamp)

3. Comment Philosophy — Universal Rules

Comments explain WHY, not WHAT. The code says what; the comment says why.

# WRONG — explains what (redundant with the code)
# Increment counter by 1
counter += 1

# WRONG — explains what
# Check if user is active
if user.is_active:
    ...

# CORRECT — explains why
# Use exponential backoff capped at 60s: Stripe's API has a 429 rate limit
# that resets on a per-minute window. Backoff prevents thundering herd on retry.
wait_seconds = min(60, 2 ** attempt)

Every non-obvious algorithmic step must have a comment. "Non-obvious" means: if you removed the comment, a reasonable senior engineer would need more than 10 seconds to understand why that specific approach was taken.

Every TODO must include three things:

# TODO(param): Switch from PSI to KS test for continuous features.
# Deferred because: PSI is computed for all feature types currently;
# KS implementation requires distribution fitting step.
# Track: https://github.com/myorg/myrepo/issues/47

Commented-out code must be deleted. Version control exists for archaeology. If code is left commented out, it must have an expiry comment or it will be deleted on the next review:

# REMOVED 2024-03-15: replaced by vectorized numpy approach (see commit abc1234)
# Keeping for reference until performance regression testing is complete (deadline: 2024-04-01).
# old_approach = [compute(x) for x in data]

4. Security Review — Applied Automatically

SQL injection: always use parameterized queries. Never interpolate user input into SQL strings.

# FORBIDDEN — SQL injection vulnerability
query = f"SELECT * FROM users WHERE email = '{email}'"
cursor.execute(query)

# CORRECT — parameterized query
cursor.execute("SELECT * FROM users WHERE email = %s", (email,))

# CORRECT with SQLAlchemy ORM
user = session.query(User).filter(User.email == email).first()

Path traversal: always validate and sanitize file paths derived from user input.

# FORBIDDEN — path traversal vulnerability
file_path = Path(base_dir) / user_supplied_filename
with open(file_path) as f: ...

# CORRECT — resolve and verify the path is under the allowed base directory
def safe_open(base_dir: Path, user_filename: str) -> Path:
    safe_path = (base_dir / user_filename).resolve()
    if not safe_path.is_relative_to(base_dir.resolve()):
        raise PermissionError(f"Path traversal attempt detected: {user_filename}")
    return safe_path

Shell injection: never construct shell commands from user input using string interpolation.

# FORBIDDEN — shell injection vulnerability
os.system(f"convert {user_input_filename} output.pdf")
subprocess.run(f"git clone {repo_url}", shell=True)

# CORRECT — use argument arrays, shell=False
subprocess.run(["convert", user_input_filename, "output.pdf"], check=True)
subprocess.run(["git", "clone", repo_url], check=True)

Deserialization: never unpickle data from untrusted sources. Use JSON with schema validation instead.

# FORBIDDEN — arbitrary code execution via malicious pickle
model = pickle.loads(user_uploaded_bytes)

# CORRECT — validate before deserializing; for models, use MLflow's safe loading
model = mlflow.pyfunc.load_model(model_uri)  # MLflow validates the artifact

Secrets detection: before outputting any code, scan for patterns that look like credentials:

• Strings matching patterns like sk-..., ghp_..., AKIA..., -----BEGIN PRIVATE KEY-----
• Hardcoded passwords, API keys, connection strings with credentials
• If found: replace with a placeholder and add a comment directing the user to use a secrets manager

5. Performance Review — Applied to Data-Scale Code

For any loop over a dataset, estimate the complexity and flag O(n²) or worse:

When generating code that processes collections:

1. Estimate n (explicitly — "what is the expected input size?")
2. State the complexity
3. If O(n²) or worse, offer a better approach unless O(n²) is justified

For any database query generated, ensure it uses indexes. Flag full table scans:

-- FLAGGED: No index on user_id? This is a full table scan on orders for every user lookup.
SELECT * FROM orders WHERE user_id = 123;

-- REQUIRES: CREATE INDEX idx_orders_user_id ON orders(user_id);
-- Or: ensure user_id is the partition key in distributed storage.

For any API call in a loop, flag as an N+1 problem and suggest batching:

# FLAGGED: N+1 pattern — makes one API call per user. For 10,000 users: 10,000 API calls.
for user in users:
    profile = api.get_user_profile(user.id)  # N+1

# CORRECT: batch API call
user_ids = [u.id for u in users]
profiles = api.get_user_profiles_batch(user_ids)  # 1 call, or ceil(n/batch_size) calls

For any large object held in memory, suggest streaming or chunking if size is unbounded:

# FLAGGED: loads entire file into memory. For files > available RAM, this will OOM.
data = json.load(open("huge_dataset.json"))

# CORRECT: stream with ijson or process in chunks
import ijson
with open("huge_dataset.json", "rb") as f:
    for record in ijson.items(f, "item"):
        process(record)