Track G — The Diff Bank
Thirty agent-produced diffs. Accept, reject, or revise — in ninety seconds each.
The agentic round gives you an oversized task and an AI agent. The agent will produce more code than you can read carefully, and the round is scored on which of it you let through. That is a trainable skill with a specific technique, and this is the drill.
Companion to
WARMUP.md(the method, the worked hour, the takeover boundary). Every Python behaviour asserted here was measured on CPython 3.13.
Table of Contents
- How to Drill This
- The Six-Pass Review, in Ninety Seconds
- A. Concurrency and Async (D1–D5)
- B. Error Handling (D6–D9)
- C. Correctness and Boundaries (D10–D13)
- D. Security (D14–D17)
- E. Resource Management (D18–D20)
- F. Performance (D21–D23)
- G. Test Quality (D24–D27)
- H. Looks Wrong, Is Right (D28–D30)
- What Agents Get Wrong, Ranked
- The Things You Cannot See in a Diff
- References
How to Drill This
Ninety seconds per diff, timer visible. That is roughly the real budget: an hour-long round with an agent producing five to ten diffs leaves you a couple of minutes each, and the ones you linger on are the ones you should have rejected in ten seconds.
For each: say the verdict first, then the reason. In the round you will be thinking out loud,
and "Reject — time.sleep in an async function blocks the whole loop" is a complete answer.
"Hmm, let me look at this…" followed by forty seconds of silence is not, even if you arrive at
the same place.
The three verdicts, and they are not equally weighted:
| Verdict | Means | The trap |
|---|---|---|
| ACCEPT | Ship it | Accepting because it looks like the code around it |
| REVISE | The approach is right, one thing is wrong | Rewriting the whole thing when one line was wrong |
| REJECT | The approach is wrong; re-prompt or take over | Rejecting everything — see section H |
Section H exists because reflexive rejection is a real failure mode, and interviewers test for it. An engineer who rejects all thirty scores worse than one who accepts the three that are correct, because the round is measuring judgement, not suspicion.
The Six-Pass Review, in Ninety Seconds
Read the diff six times, each pass looking for one thing. In this order — the passes are sorted by how cheaply they find a fatal problem, so you stop early on most diffs.
| # | Pass | Time | Looking for |
|---|---|---|---|
| 1 | Does it do what I asked? | 10 s | Scope drift, a different problem solved, silent extra changes |
| 2 | The error path | 15 s | What happens when this fails? Is the exception caught, swallowed, or wrong? |
| 3 | The boundary | 15 s | Empty, one, exactly-at-the-limit, negative, None |
| 4 | The resource | 15 s | What is opened, locked, or allocated — and is it released on every path? |
| 5 | The concurrency | 15 s | Is this async? Shared? Is there a check-then-act? |
| 6 | The test | 20 s | Does it assert the behaviour, and could it fail? |
Pass 6 is where most defects are found, because an agent's test almost always passes and often asserts nothing that would break if the code were wrong. Ask of every test: what single-line change to the implementation would make this fail? If you cannot name one, the test is decorative.
And one meta-rule that outranks all six: if a diff touches something you did not ask about, that alone is grounds to reject and re-prompt. Scope drift in an agent's output is the leading indicator that it misunderstood the task, and everything downstream inherits the misunderstanding.
A. Concurrency and Async (D1–D5)
D1
async def fetch_all(self, urls):
results = []
for url in urls:
- results.append(await self._get(url))
+ tasks = [self._get(u) for u in urls]
+ results = await asyncio.gather(*tasks)
return results
REJECT. The intent — parallelize — is right; the implementation is unbounded. Ten thousand URLs means ten thousand coroutines scheduled at once: file-descriptor exhaustion locally, and a DoS against the target.
And a second defect that is easy to miss: gather without return_exceptions=True propagates
the first exception while the other tasks keep running, orphaned. Their eventual failures
surface as Task exception was never retrieved or vanish.
Re-prompt with the constraint: "bound concurrency with a semaphore of N, and use
asyncio.TaskGroup so a failure cancels the siblings."
D2
async def _retry(self, request):
for attempt in range(self.max_attempts):
try:
return await self._send(request)
except TransportError:
+ time.sleep(2 ** attempt)
continue
REJECT — the highest-severity class in this section. time.sleep in a coroutine blocks the
entire event loop, not just this task. Every other request in the process stalls for the backoff.
At max_attempts=5 that is 1+2+4+8 = 15 seconds of total service freeze, from one slow
dependency. And it will not show up in a unit test, because a test with one request in flight
cannot observe the other requests that were not there.
Fix: await asyncio.sleep(...). One word, and the diff is otherwise fine — which makes it
REVISE if you are being generous, but say the severity out loud either way.
D3
class RateLimiter:
def allow(self, key):
bucket = self._buckets[key]
bucket.refill()
- with self._lock:
- if bucket.tokens >= 1:
- bucket.tokens -= 1
- return True
- return False
+ if bucket.tokens >= 1:
+ bucket.tokens -= 1
+ return True
+ return False
REJECT. The agent removed a lock, presumably as an optimization, and reintroduced a
check-then-act race: two threads read tokens == 1, both decrement, both return True, and the
bucket goes negative. The limit is exceeded, silently, only under load.
Do not accept "the GIL makes it safe." Measured on 3.13, counter += 1 across four threads
loses zero updates — but that is the eval breaker's current scheduling, not a guarantee, and it
disappears under free-threading (PEP 703). A correctness argument that depends on an interpreter
implementation detail is not a correctness argument.
The critical section is three arithmetic operations; the lock costs nothing. There was no optimization here to make.
D4
+ class Cache:
+ def __init__(self, entries={}):
+ self._entries = entries
REJECT. A mutable default is evaluated once, at function definition, so every Cache()
created without an explicit argument shares the same dict. Two caches, one backing store.
Measured, the canonical demonstration:
def f(x, acc=[]): acc.append(x); return acc
f(1) # [1]
f(2) # [1, 2] <-- the same list
Fix: def __init__(self, entries=None): self._entries = entries if entries is not None else {}.
This is a well-known Python trap and agents still produce it, usually when refactoring a signature. Ten seconds to spot; a shared-state bug that takes hours to find in production.
D5
async def process(self, items):
async with self._lock:
for item in items:
- await self._handle(item)
+ asyncio.create_task(self._handle(item))
REJECT, and it is worse than it looks. Two defects compound:
- The lock is released immediately —
create_taskreturns instantly, so the loop finishes and theasync withexits while every handler is still running. The lock now protects nothing. - The tasks are never awaited and no reference is kept. Python's event loop holds only a weak reference to a running task, so a task with no strong reference can be garbage-collected mid-execution. The work silently does not happen, non-deterministically.
The second is the subtle one and it is documented behaviour: the asyncio docs explicitly say to
keep a reference to the returned task.
Re-prompt: "gather the tasks inside the lock, or use a TaskGroup and keep the lock only around the state it protects."
B. Error Handling (D6–D9)
D6
try:
return self._transport.send(request)
- except (ConnectionError, TimeoutError):
+ except Exception:
if attempt < self.max_attempts:
continue
raise
REJECT. except Exception catches programming errors — a TypeError from a bug in request
construction, an AttributeError from a typo — and retries them three times before surfacing.
An instant, clear failure becomes a slow, confusing one, and the retry metric now counts bugs as
transient failures.
The narrower point worth making: it does not catch asyncio.CancelledError, which is a
BaseException — verified: issubclass(asyncio.CancelledError, Exception) is False. But that
is luck, not intent, and the diff would be equally wrong if it said except BaseException.
Retry only what is retryable, and be able to name the list.
D7
def load_config(path):
try:
return json.loads(Path(path).read_text())
- except json.JSONDecodeError as e:
- raise ConfigError(f"invalid config at {path}") from e
+ except json.JSONDecodeError:
+ return {}
REJECT. A malformed config now starts the service with every setting at its default — silently. The failure mode is a production incident where the service is running, healthy by every health check, and behaving as if nobody configured it.
The general rule and it is worth stating as one: a config error must be loud and must happen at startup. Configuration is the one place where crashing is unambiguously the right behaviour, because the alternative is running with the wrong behaviour.
Note also what the agent deleted: the from e chain, which is what makes the traceback point at
the actual JSON error. Even in an accepted version, raise ... from e should stay.
D8
def transfer(self, src, dst, amount):
self._debit(src, amount)
+ try:
+ self._credit(dst, amount)
+ except Exception:
+ logger.exception("credit failed")
REJECT — this one loses money. If _credit fails, the debit has already happened and is not
rolled back. The log line is the only record, and the account is short by amount.
"Logged" is not "handled." A logger.exception in an except block with no re-raise, no
compensation, and no return-value change is a swallowed error wearing a costume.
The correct shapes, in order of preference: one transaction covering both; or debit → outbox
record → async credit with retry (the outbox
pattern); or an explicit
compensating credit back to src plus an alert, because compensation can itself fail.
D9
def get_user(self, uid):
row = self._db.fetchone("SELECT * FROM users WHERE id = ?", (uid,))
- if row is None:
- raise UserNotFound(uid)
- return User.from_row(row)
+ return User.from_row(row) if row else None
REVISE — the change is defensible, but it is incomplete. Returning None versus raising is a
real API design choice: None is fine for a lookup that is expected to miss, an exception is right
when a miss is an error.
What makes it a REVISE rather than an ACCEPT: the diff changes a public contract and updates
no callers. Every existing get_user(x).name now raises AttributeError: 'NoneType' at a place
far from the cause.
The question to ask the agent — and asking it out loud is the scored behaviour — "how many callers assume this raises?" If the answer is "I didn't check", that is the finding.
C. Correctness and Boundaries (D10–D13)
D10
def value_at(self, key, version):
versions = self._versions[key]
- i = bisect_right(versions, version) - 1
- if i < 0:
- return None
- return self._values[key][i]
+ i = bisect_left(versions, version)
+ return self._values[key][i - 1]
REJECT — two bugs in two lines.
bisect_leftis the wrong predecessor.bisect_left(v, 57) - 1gives the last element strictly less than 57; the correct predecessor query isbisect_right(v, 57) - 1, the last element ≤ 57. A read at exactly the version a write happened now returns the previous value.- The
i < 0guard is gone. For a version before the key existed,i - 1 == -1, and Python's negative indexing returns the newest value instead of raising. A query for the distant past returns data from the future, with no error.
The second is the more dangerous because it is silent, and it is the single most common bug in this problem.
D11
def expire(self, now):
- for key in list(self._entries):
+ for key in self._entries:
if self._entries[key].expiry <= now:
del self._entries[key]
REJECT. Mutating a dict while iterating it raises RuntimeError: dictionary changed size during iteration — verified. The list(...) the agent removed was taking a snapshot of the keys, and
it was there on purpose.
Why the agent did it: it looks like a wasteful allocation. It is a correctness requirement.
Watch for the same removal on sets and on dict.items(), and note that the failure is at least
loud — unlike the equivalent on a list, where deleting during iteration silently skips
elements because the indices shift under you. That silent version is the worse one, and this diff
is the shape it takes.
D12
def is_ready(self, progress):
- return abs(progress - 1.0) < 1e-9
+ return progress == 1.0
REJECT. progress is accumulated in floating point, and floating-point sums do not land on
exact values: 0.1 + 0.2 == 0.3 is False (it is 0.30000000000000004). A progress counter
summed from tenths reaches 0.9999999999999999 and is_ready is never true — a hang with no
error and no log line.
The one case where the agent's version would be right: if progress is assigned 1.0 rather
than accumulated. So the review question is where the value comes from, which the diff does not
show — and "I need to see the caller" is the correct thing to say rather than guessing.
D13
def window(self, items, size):
- for i in range(len(items) - size + 1):
+ for i in range(len(items) - size):
yield items[i:i + size]
REJECT. Classic off-by-one: the last full window starts at len - size, and range is
exclusive, so the bound must be len - size + 1. The agent's version silently drops the final
window.
The tell that this class of bug is present: the diff changes a range bound with no
accompanying test change. Any diff that adjusts an index expression and does not touch a test is
suspicious by construction — either the tests do not cover the boundary, or the change is wrong.
Both are findings, and saying that is better than working the arithmetic in your head.
D. Security (D14–D17)
D14
- rows = db.execute("SELECT * FROM events WHERE tenant = ? AND type = ?", (tenant, typ))
+ rows = db.execute(f"SELECT * FROM events WHERE tenant = '{tenant}' AND type = '{typ}'")
REJECT, immediately, and stop reading the rest of the diff. SQL injection. The agent replaced a parameterized query with string interpolation, probably while adding a clause it found awkward to parameterize.
Any diff that turns a parameterized query into an f-string is an automatic reject regardless of how well-validated the inputs look, because the validation is somewhere else and can change.
And say the second-order thing: if the agent did this once it may have done it elsewhere in the
same change. Grep the whole diff for f"SELECT, f"INSERT, .format( near SQL before
reviewing anything else.
D15
def serve_file(self, name):
- path = (self.root / name).resolve()
- if not path.is_relative_to(self.root):
- raise Forbidden(name)
+ path = self.root / name
return path.read_bytes()
REJECT. Path traversal. name = "../../etc/passwd" now escapes the root, and pathlib's /
operator does no containment checking at all.
The removed check was doing two necessary things, and both matter: resolve() collapses ..
and follows symlinks, and is_relative_to confirms containment. Checking containment without
resolving is also broken — a symlink inside the root pointing outside it passes the string check.
Order matters: resolve, then check. Getting that order right is the actual knowledge being tested here.
D16
def verify_token(self, provided, expected):
- return hmac.compare_digest(provided, expected)
+ return provided == expected
REJECT. == on bytes short-circuits at the first differing byte, so comparison time leaks how
many leading bytes are correct. An attacker measures and recovers the token byte by byte — 256
guesses per byte instead of 256^n total.
The counter-argument you will hear and how to answer it: "network jitter drowns the signal." It does not — statistical averaging over many samples recovers timing differences well below the jitter, and this has been demonstrated repeatedly against real services.
hmac.compare_digest is constant-time and is a drop-in. There is no cost to being right here,
which is what makes accepting this diff indefensible.
D17
def load_manifest(self, text):
- return yaml.safe_load(text)
+ return yaml.load(text, Loader=yaml.Loader)
REJECT. yaml.Loader constructs arbitrary Python objects from tags like
!!python/object/apply:os.system. Loading an untrusted manifest is remote code execution.
The agent's likely motivation is real — safe_load cannot construct custom types, so a manifest
using them fails. The fix is a narrow custom loader, not the full one:
class ManifestLoader(yaml.SafeLoader): pass
ManifestLoader.add_constructor("!duration", _parse_duration)
REVISE with that direction, rather than a flat reject — the agent identified a genuine limitation and reached for the wrong tool, and pointing at the right tool is a better use of the turn.
E. Resource Management (D18–D20)
D18
def process(self, path):
- with open(path) as f:
- data = json.load(f)
+ f = open(path)
+ data = json.load(f)
+ f.close()
return transform(data)
REJECT. If json.load raises — a malformed file, which is the expected failure — close()
never runs and the descriptor leaks. A service processing many malformed files exhausts its
descriptor limit and then fails on everything, including healthy requests.
The failure is remote from the cause, which is what makes leaks expensive: the error surfaces as "cannot open socket" in an unrelated code path, hours later.
CPython's refcounting usually closes it at collection, which is what makes this survive testing and fail in production — under load, or on PyPy, or when an exception traceback holds a reference to the frame, the object outlives its scope.
D19
class Client:
def __init__(self):
- self._cache = LRUCache(maxsize=10_000)
+ self._cache = {}
REJECT. An unbounded cache is a memory leak with a delay. It grows until the process is OOM- killed, and the time-to-failure depends on traffic, so it passes every test and every staging soak.
The question to ask, and it is the one that decides the fix: what is the key space? If keys are a bounded enum, a dict is fine and the agent may be right. If keys are user-supplied — request IDs, URLs, tenant strings — it is unbounded and an attacker can fill it deliberately.
Any dict keyed on user input and never deleted from is a memory leak, and phrasing the review that way finds these faster than looking for the word "cache".
D20
async def query(self, sql):
conn = await self._pool.acquire()
- try:
- return await conn.execute(sql)
- finally:
- await self._pool.release(conn)
+ result = await conn.execute(sql)
+ await self._pool.release(conn)
+ return result
REJECT. Connection leak on the exception path. A failing query never releases, the pool drains
one connection per failure, and eventually every request blocks forever waiting for acquire.
This is a total outage triggered by a partial failure — the worst shape, because the trigger (some queries failing) is survivable and the consequence (all queries hanging) is not.
Prefer the construct that cannot be got wrong:
async with self._pool.acquire() as conn:
return await conn.execute(sql)
A try/finally an agent can delete is worse than a context manager it cannot, and saying that
— preferring the un-deletable construct — is the more valuable observation than spotting the leak.
F. Performance (D21–D23)
D21
def drain(self):
out = []
- while self._queue:
- out.append(self._queue.popleft())
+ while self._items:
+ out.append(self._items.pop(0))
return out
REJECT. list.pop(0) shifts every remaining element, so draining n items is O(n²).
Measured at n = 100,000: deque.popleft 40.7 ns, list.pop(0) 15,895 ns — 390×, and
the gap grows linearly.
This is the most common accidental O(n²) in Python. The agent probably changed the type from
deque to list for an unrelated reason and this followed.
And note the cheaper fix if a list is required: out = self._items[:]; self._items.clear() —
one copy, O(n) total, no per-item shifting.
D22
def enrich(self, orders):
for o in orders:
- o.user = self._users_by_id[o.user_id]
+ o.user = self._db.fetchone("SELECT * FROM users WHERE id = ?", (o.user_id,))
REJECT. The N+1 query. One thousand orders is one thousand round trips; at 1 ms each that is a one-second request that was microseconds.
The fix is a batch fetch, and the version to ask for is explicit about the failure mode:
ids = {o.user_id for o in orders}
users = self._db.fetch_all("SELECT * FROM users WHERE id = ANY(?)", (list(ids),))
by_id = {u.id: u for u in users}
for o in orders:
o.user = by_id.get(o.user_id) # .get, not [] -- a missing user must not KeyError here
N+1 is invisible in a unit test with three rows and obvious in production. Say the scaling, not the principle: "this is one query per order — at our order volume that's a thousand round trips per request."
D23
def parse_lines(self, lines):
+ pattern = re.compile(r"^(\w+)\s+(\d+)$")
for line in lines:
- m = re.match(r"^(\w+)\s+(\d+)$", line)
+ m = pattern.match(line)
ACCEPT — and understand why, because the common explanation is wrong.
The usual claim is that re.match(pattern, s) recompiles each time. It does not — re caches
compiled patterns. Measured, per call:
| ns | |
|---|---|
re.compile(pat).match(s) | 248 |
re.match(pat, s) (module cache) | 420 |
precompiled c.match(s) | 104 |
So the real gain is 4× over re.match, and it comes from skipping the cache lookup, not a
recompile. Note also that re.compile inside the loop would be faster than re.match — which is
the opposite of the folk explanation.
Better still: hoist it to a module-level constant so it compiles once per process rather than
once per call to parse_lines. Accept the diff and suggest that.
G. Test Quality (D24–D27)
D24
+ def test_retry_on_503(self):
+ client = Client(retry=Retry(max_attempts=3))
+ with mock.patch.object(client._transport, "send") as m:
+ m.return_value = Response(503)
+ client.get("https://example.com")
+ assert m.call_count == 3
REVISE. The assertion is real — it pins the retry count, which is the behaviour under test.
What is missing is what the user experiences: the test never asserts what client.get finally
returns or raises after exhausting the attempts. As written it passes even if the client
swallows the failure and returns None.
Add with pytest.raises(RetriesExhausted): or assert the returned response is the final 503.
The general question that finds this class: what single-line change to the implementation would make this test fail? Here, quite a few would not.
D25
+ def test_process_handles_empty(self):
+ result = process([])
+ assert result is not None
REJECT. assert result is not None is satisfied by [], {}, 0, "", False, and a
MagicMock. It asserts almost nothing, and it will keep passing through any refactor.
A test that cannot fail is worse than no test: it occupies a line in the coverage report and creates false confidence that the empty case is handled.
Ask for the specific expectation: assert process([]) == []. And if the correct behaviour for
empty input is genuinely unclear, that is the finding — the agent has written a test to cover a
case whose semantics nobody has decided, and deciding it is the work.
D26
+ async def test_worker_processes_queue(self):
+ q = asyncio.Queue()
+ worker = asyncio.create_task(run_worker(q))
+ await q.put(Item(1))
+ await asyncio.sleep(0.1)
+ assert processed == [Item(1)]
REJECT. sleep(0.1) is a race, not a synchronization. It is slow (0.1 s × hundreds of tests),
flaky on a loaded CI machine, and it does not actually establish that the work finished — it
establishes that 100 ms passed.
Synchronize on the event, not the clock:
await q.put(Item(1))
await q.join() # returns when task_done() has been called for every item
assert processed == [Item(1)]
worker.cancel() # <-- and clean up, or the task leaks into the next test
The cancel matters too: an un-cancelled worker task survives the test and can interfere with later ones, which is a leading cause of "passes alone, fails in the suite".
A sleep in a test is always a bug, and it is worth stating as an absolute — there is always a
condition to wait on instead.
D27
+ def test_report_output(self):
+ out = render_report(SAMPLE)
+ assert out == Path("tests/golden/report.txt").read_text()
REVISE — it depends entirely on how the golden file is maintained, and you must ask.
Legitimate when the output is complex structured text that would be impractical to assert by hand, the golden file is reviewed in the diff when it changes, and regeneration is deliberate.
Worthless when the workflow is "test fails → run with --update → commit". Then the test
asserts only that the code ran without raising, and a genuine regression is committed as an
expected change.
What to ask the agent: "is report.txt reviewed when it changes, and is regeneration a
separate deliberate step?" Plus: the golden file must be in the diff. A snapshot test whose
snapshot is generated rather than written is a test nobody has read.
H. Looks Wrong, Is Right (D28–D30)
These three exist because reflexive rejection is a scored failure. An engineer who rejects everything is not exercising judgement, and a round with an agent is about judgement. Each of these violates a rule you have been taught; each is correct here, and being able to say why the rule does not apply is worth more than either accepting or rejecting.
D28
def build_response(self, chunks):
- return "".join(chunks)
+ out = ""
+ for c in chunks:
+ out += c
+ return out
ACCEPT — with a caveat you must be able to state precisely.
The rule says += on strings in a loop is O(n²). On CPython it is not, because
unicode_concatenate resizes the string in place when its refcount is 1. Measured:
| n | join | += | ratio |
|---|---|---|---|
| 10,000 | 0.28 ms | 0.40 ms | 1.47× |
| 100,000 | 2.89 ms | 4.19 ms | 1.45× |
| 500,000 | 14.68 ms | 22.06 ms | 1.50× |
Constant ratio — linear, not quadratic.
But hold one extra reference and the optimization is defeated entirely:
| n | join | += with an alias | ratio |
|---|---|---|---|
| 10,000 | 0.28 ms | 32.3 ms | 114× |
| 50,000 | 1.39 ms | 415.8 ms | 300× |
So the honest verdict is: accept, note that join is still better, and note that the diff is one
prev = out away from being 300× slower. That is a much stronger answer than either "reject,
that's O(n²)" (wrong on CPython) or "accept, it's fine" (fragile, and wrong on PyPy).
The transferable point: a performance rule that depends on an interpreter optimization should be stated with its precondition.
D29
def handle(self, event):
+ # NOTE: deliberately not deduplicating here -- the sink is idempotent
+ # on (event_id, version) and dedup would need unbounded state.
self._sink.write(event)
ACCEPT. This looks like an agent adding a comment instead of doing the work, and it is the opposite: it is a correctly reasoned decision not to build something.
Deduplicating at this layer would require either unbounded state or a window with a stated reordering bound, and the sink already provides the guarantee. Adding a dedupe here would be redundant work and a new memory leak.
Accepting this is the harder call and it is the right one. Reflexively rejecting a diff for "not doing enough" pushes the agent toward building things that should not exist, and an interviewer watching you demand redundant dedup has learned something about your judgement.
One legitimate follow-up: "is the sink's idempotency tested?" If the comment's premise is unverified, that is the work — not the dedup.
D30
def get_config(self):
- return self._config.copy()
+ return self._config
ACCEPT — conditionally, and the condition is the whole answer.
Returning an internal mutable directly looks like an encapsulation violation, and normally is. It
is correct here if self._config is immutable — a frozen dataclass, a MappingProxyType, or a
namedtuple. Then the copy() was pure overhead on a hot path.
So the verdict depends on a line the diff does not show, and the right move is to say so:
"Accept if _config is immutable — let me check the declaration. If it's a plain dict, reject,
because a caller can now mutate our state."
Naming what you need to see, rather than guessing, is the scored behaviour. In an agentic round
you can actually go and look, and doing so out loud — "let me check how _config is declared" —
is exactly the verification discipline being measured.
What Agents Get Wrong, Ranked
Across these thirty and the material in WARMUP.md, the defects cluster. Knowing
the ranking is what lets you review fast — check the top of this list first and you find most
problems in the first two passes.
| Rank | Failure | Why agents do it | Diffs |
|---|---|---|---|
| 1 | Removing a guard that looks redundant | The guard's reason is not in the local context | D3, D10, D11, D15, D16, D18, D20 |
| 2 | Tests that cannot fail | Optimizing for "the test passes" | D24, D25, D27 |
| 3 | Unbounded anything | The bound is a non-functional requirement, rarely stated | D1, D19 |
| 4 | Sync primitives in async code | Trained on far more sync Python than async | D2, D5 |
| 5 | Broadening an exception clause | It makes the immediate failure go away | D6, D7, D8 |
| 6 | String-building a query or a path | It is the most natural way to express it | D14, D15 |
| 7 | Changing a contract without updating callers | The callers are not in the context window | D9 |
| 8 | Boundary arithmetic | Genuinely easy to get wrong | D12, D13 |
Rank 1 is the dominant class and it has a single root cause: a guard's justification lives in the incident that caused it, not in the code. So the review question that finds these fastest is not "is this correct?" but "why was the deleted line there?" — and if you cannot answer, that is sufficient grounds to reject.
Say that rule out loud in the round. It is the most transferable thing in this document.
The Things You Cannot See in a Diff
And this is the section that separates a good agentic round from an excellent one. A diff shows you what changed. It does not show you:
| Invisible | How to check it in the round |
|---|---|
| Callers of a changed signature | grep -rn "def get_user|get_user(" — 5 seconds, and D9 depended on it |
| Whether the deleted guard had a test | git log -S "is_relative_to" --oneline — find the commit that added it |
| What else the agent touched | git diff --stat before reading any file. Scope drift is the leading indicator |
| Whether the new test actually runs | Run it. Then break the implementation and confirm it fails |
| Whether it works on the real data | The agent's fixture is not your production shape |
| Whether the change is in the hot path | The diff has no profile attached |
The single highest-value habit: run git diff --stat first, every time. A diff touching four
files when you asked for one is a misunderstanding, and you learn that in two seconds instead of
after reading three hundred lines.
And the strongest single move in the whole round: after the agent's tests pass, break the implementation deliberately and confirm the test fails.
# revert one line of the fix, run the test
if it still passes -> the test asserts nothing. This is the finding.
That takes thirty seconds, it directly tests the thing agents are worst at, and almost nobody does it. Doing it out loud, once, is worth more than reviewing five more diffs.
References
WARMUP.md— the eight-step method, the worked 60 minutes, the takeover boundary, promptingREADME.md— Track G drills, the scoring rubric, the repo setup../coding/QUIZBANK.md— the underlying mechanisms: D10 is Q18, D21 is Q7, D3 is Q57, D26 is Q134../python-internals/QUIZBANK.md— the runtime behaviour behind D4, D5, D28../take-home/WARMUP.md— the deep-dive round, where you defend code you wrote../../CHEATSHEET.md#8-agentic-coding— the dense version for the morning of a round- CPython
Objects/unicodeobject.c,unicode_concatenate— the in-place resize behind D28 - Python docs, asyncio — Task object: "save a reference to the result of this function" — the D5 GC hazard
- OWASP Top 10 — the categories behind D14–D17
- Google. Testing on the Toilet: Change-Detector Tests Considered Harmful — the D27 argument