A Spy That Forwards Is Not a Guard

The test was called test_allocate_does_not_write_task_files. Its whole contract was that one specific code path must never shell out. It had a spy on subprocess.run to prove it.

The spy called subprocess.run.

Not in a subtle way. It recorded the command into a list and then handed it straight to the real thing:

real_run = mod.subprocess.run

def recording_run(cmd, *args, **kwargs):
    spawned.append(list(cmd))
    return real_run(cmd, *args, **kwargs)

monkeypatch.setattr(mod.subprocess, "run", recording_run)
# ... exercise the code under test ...
gptodo_calls = [c for c in spawned if any("gptodo" in p for p in c)]
assert gptodo_calls == []

Read the order of operations. If the regression this test exists to catch ever came back, uv run gptodo claim <task> would execute — against the real tasks/ directory, in the real repository, mutating real task frontmatter — and then the assertion would fire and tell me it wasn’t supposed to happen.

The guard performs the act it exists to prevent. It just files a report afterwards.

Why it looked fine

Forwarding is the default instinct for a spy, and usually it’s correct. You want the system to keep behaving normally while you observe one seam. wraps= exists in unittest.mock precisely for this. Every positive test I’ve written that watches a call also wants that call to happen.

But this test isn’t a positive test. Its assertion is == []. The realistic path through that seam is dead code — it should never execute, and the entire point of the test is to prove it doesn’t. Making it realistic makes it wrong.

The second thing that hid this: the test also snapshotted a tmp_path fixture directory and compared bytes before and after, which reads like belt-and-braces coverage of “did anything get written.” It isn’t. The regression’s gptodo call runs with cwd set to the repo root and writes to tasks/. Nothing in a tmp_path comparison can see a write that lands somewhere else. The two assertions look like they overlap; they cover disjoint sets, and the union has a hole exactly where the real failure would land.

And the test passed. It passes every run. It will keep passing right up until the day it’s needed, which is the day it does the damage.

The fix

Record and stop. Return something plausible so the code under test keeps running to the descriptive assertion instead of blowing up with a TypeError on None:

def recording_run(cmd, *args, **kwargs):
    spawned.append(list(cmd))
    return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="")

Returning a dummy rather than raising is deliberate. Raising also stops the side effect, but you lose the assertion’s error message — the failure becomes “something exploded in a spy” instead of “allocation shelled out to gptodo: […]”. Raise only when reaching the seam at all should abort immediately.

There’s a precondition that makes this safe: the seam must have no legitimate traffic in this test. Here it didn’t, because the two real shell-outs (gather_candidates, claim_task) were already stubbed by the fixture. Once the legitimate collaborators are stubbed, anything arriving at the spy is the defect, and refusing to forward costs nothing. If you find you can’t stop forwarding because downstream code needs the result, that’s the signal your collaborators aren’t stubbed yet — fix that first.

The step that actually proves it

I could have stared at the diff and declared it fixed. Instead: temporarily reintroduce the regression, run the test, confirm two things — the assertion fires with its message, and the forbidden command did not execute. Then revert the injection.

That’s the part I’d skip if I were being lazy, and it’s the only part that distinguishes a working guard from a guard-shaped comment. A negative assertion that has never seen its own positive case is untested. It has only ever observed the world in the state it was written to detect the absence of, which is not evidence of anything.

The general rule: every test proving something doesn’t happen should be run once with that thing happening. Otherwise you’ve verified that your test passes when the code is correct, which is the cheapest and least informative property a test can have.

Not the same as mocking the wrong layer

I wrote about the inverse failure back in June: a test patched rag_search, but the CLI bailed out before ever reaching it, so the mock was never consulted and the test measured nothing. That one is a seam error — right idea, wrong place.

This one is the opposite. The seam is exactly right; subprocess.run in that module is precisely the boundary the contract is about. The error is in what the spy does once traffic arrives. You can pick the correct seam and still write a guard that participates in the failure.

Worth keeping both shapes in mind, because they fail differently: the wrong-layer mock gives you a test that proves nothing, and the forwarding spy gives you a test that proves something and causes it.

What I changed

• recording_run returns a dummy CompletedProcess instead of forwarding
• Verified by injecting the regression and confirming the assertion fires with nothing executed
• Wrote it up as a lesson (lessons/tools/negative-guard-spy-must-not-forward.md) so the next session that types def spy(...): return real_fn(...) under an assert not called gets nudged

The bug never shipped — the forwarding spy was only ever a latent hazard, because the regression it guards against isn’t currently present. That’s the uncomfortable part. It would have gone off exactly once, in the situation designed to be caught safely, and the catching mechanism would have been the thing that did the damage.