Guide · Python

How to review Python pull requests: the 9 defect classes with examples

Python pull requests read easily, which is the problem: a short, tidy function can hide a default argument shared across calls, a naive datetime or a parser that executes its input. This guide walks the nine defect classes DiffDojo trains, in the order a reviewer should weigh them, with what each one looks like in Python code.

1. Security

Look for untrusted input reaching an interpreter: an f-string inside a SQL call, yaml.load with Loader=yaml.Loader or UnsafeLoader instead of yaml.safe_load, pickle.loads, subprocess with shell=True, or eval on request data. Then check object-level authorization: a handler that fetches a row by id must also check that the row belongs to the caller.

Worked example:

• yaml.Loader on untrusted input can construct arbitrary Python objects

2. Requirement fidelity

Read the ticket before the code. Sort direction, the exact filter, the field a total is computed from and which records count are where Python PRs most often do something reasonable that is not what was asked.

Worked example:

• 'Newest first' but sorted ascending: the order is reversed

3. Edge cases & error handling

Ask what happens on empty, missing and huge input. Typical misses: requests calls without a timeout, mutating a dict or list while iterating it, int() on user text without handling ValueError, and an off-by-one on a slice or range bound.

Worked examples:

• Deleting from a dict while iterating it raises RuntimeError
• requests.get with no timeout can hang the handler forever

4. Hallucinated APIs & dependencies

Models invent plausible methods and keyword arguments: a pandas or SQLAlchemy method that does not exist, a retries= argument a client never had. If you do not recognise a call, look it up; unless a type checker covers that library, the code will only fail when that line runs.

5. State & data flow

Watch for state that outlives one call: a mutable default argument (def f(items=[])), a module-level cache that is never invalidated, or a class attribute shared by every instance when each needed its own. A default list or dict is created once, when the function is defined, so every call that relies on it shares the same object; default to None and create it inside.

6. Performance

Look for work that grows with the input: one query per row (N+1) in an ORM loop, x in some_list inside a loop over a large input (use a set), or reading a whole file into memory to look at its first lines. With ten rows of test data none of them may be noticeable unless a test counts queries, so ask what the code does at the production size the ticket implies.

7. Data correctness

Money and time are the usual suspects. Floats for currency, rounding at the wrong step, and a local naive datetime.now() compared with naive UTC timestamps from the database all produce code that runs and returns the wrong number; ordering or subtracting naive and aware values at least raises a TypeError.

Worked example:

• datetime.now() is naive: comparing it to an aware datetime raises, and it's local time anyway

8. Deceptive tests

A green test proves only what it asserts. Check for tests that mock away the logic under test, assert the implementation's own formula back at it, or never await the coroutine they claim to check.

9. Over-engineering

Not a bug, but a review comment: a strategy class hierarchy for two branches, a plugin registry for one implementation, or a new dependency for ten lines of standard library. Ask for the simple version.

The checklist

1. Does any request value reach SQL, YAML, pickle, a shell or eval?
2. Does the change do exactly what the ticket says, field by field?
3. Empty, missing, huge, malformed input: what happens, and is there a timeout?
4. Does every unfamiliar method or argument actually exist?
5. Is any state shared between calls or instances that should not be?
6. Is there I/O or a linear search inside a loop?
7. Are money and time handled with Decimal and aware datetimes?
8. Would the tests fail if the code were wrong?
9. Is there a simpler way to write this?

As of October 2026, the DiffDojo library has 98 live Python PRs to practise on, each graded on whether you catch the planted bug or approve the clean one.

All review guides · All code review examples