Lessons learned

False green: when a passing test vouches for a dead feature

A feature passed six-reviewer diff review, two rounds of execution and bug fixes, and full CI, then shipped broken. The only test covering it mocked a state its own state machine can never produce. Every offline gate proved the change was well-formed, not that it ran. Here's the anatomy of that false green, and the fix.

A feature of mine went through a six-reviewer diff review with a dedicated TypeScript reviewer, two rounds of execution and bug fixes until the agent was unable to find any more bugs, and full CI, and came out declared merge-ready. It had never worked. Not flaky, not broken on some edge: the effect at the end of its chain never fired once, from the first commit. The only test covering it passed because it mocked a state the real state machine cannot produce. Every gate in that list is offline, and an offline gate can prove a change is well-formed; none of them can prove it runs. Here is the anatomy of that false green, and the checks that keep a mock from vouching for an impossible state again.

The feature hinged on one effect firing at completion

The feature is a live resolved-model display. An Auto run resolves to a concrete model, and after the run completes, the resolved name (say, “Gemini 3.5 Flash”) should appear on the assistant message without a reload.

The mechanism is a chain of four hops. The stream reaches a terminal status. The refetch effect invalidates the messages query, so the persisted assistant message refetches, now carrying modelDisplayName. Then the swap effect in AgentRunProvider calls loadHistory, which swaps the live SSE content item for the DB assistant-content item that has the name on it. The swap effect is guarded: it only runs when !stream.runId, which was meant to say “the stream is done.”

Open diagram in a new tab ↗

The first three hops worked; the refetched messages carried the name. The feature died at the guard, and seeing why takes one look at the state machine.

complete is not idle: the guard was never true when it mattered

useAgentStream nulls runId in exactly one variant:

// useSSE.ts
const activeRunId = state.kind === "idle" ? null : state.runId;

and the terminal transition carries it forward:

// sseStateTransitions.ts — applyComplete()
return {
  ...carryActive(prev), // keeps runId: prev.runId
  kind: "complete",
};

So complete is a non-idle state with runId still set, and !stream.runId is not a “stream finished” signal. It is true only while idle. At completion, the refetch effect did its job and the messages refetched with the name, but the swap effect looked at a still-set runId and skipped it. The name surfaced only when something else finally nulled runId: a reload, a reset, or switching conversations.

That is why the bug was quiet. Nothing errored; the model name just didn’t show up until you did something unrelated.

The test passed by mocking a state the machine can’t reach

The unit test covering the swap asserted exactly the right thing: the model name renders before any reload. It was green from day one. Its setup line:

setupSSE({ runId: null, status: "complete" });

That combination is impossible. complete implies runId is set; no transition in useAgentStream produces the pair the test mocked. Under the fiction, !stream.runId is true, loadHistory fires, and the assertion passes. The test validated its own fixture, not the product.

Open diagram in a new tab ↗

The fixture wasn’t a random typo, either. The plan for the feature asserted that the swap effect fires “after stream.reset() clears stream.runId (a render later)”. Nothing calls stream.reset() at completion; the reset effect runs only on conversation switch. The plan’s premise was wrong, the test encoded the premise, and every reviewer after that anchored on the plan’s reasoning instead of reading useSSE.ts. A plan’s claim about a lifecycle is a hypothesis until the state machine’s code confirms it, and nobody, me included, ran that check.

Green gates prove a change was accepted, not that it ran

Walk the gates and ask what each one actually proved. The TypeScript reviewer ran the four changed suites, saw every test green, and cleared the live-swap as safe to ship. Running the suite is real work and necessary, but it only proves the tests pass; it says nothing about whether a test’s mocked preconditions are reachable in production, and this one’s weren’t.

The first review round surfaced three unrelated minor findings. The second round fixed those three, another bug-finding pass turned up nothing, and the automated review loop reported nothing left to fix.

But the second round was scoped to the fix commits, so “nothing found” meant “nothing wrong with these three fixes”. It could not mean “the live-swap was re-verified”, because nobody re-examined it. The earlier clearance stood on evidence that was already wrong.

The deeper limit is that this feature’s correctness lives in runtime ordering: SSE complete, then the invalidation, then the refetch landing, then the effect. A test with mocked hooks can assert structure; it cannot reproduce that race. The one verifier whose answer binds is a browser, and a single Auto run would have caught this immediately, because the model name simply doesn’t appear. That run happened only after an outside reviewer’s comment laid out all three pieces at once: applyComplete() carries the run id into complete, the guard needs !stream.runId, and the test mocks an impossible state. Caught before merge, no data loss, about a day latent from the bug landing to the fix.

The fix gates on the concept, not a value that correlates with it

!stream.runId was meant to mean “not streaming”, and it matched that concept only by coincidence, only at idle. The fix names the concept. Streaming means status running or waiting_for_input; gate on that, read through a ref:

// AgentRunProvider.tsx — fixed
const isStreamingRef = useRef(false);
isStreamingRef.current = stream.status === "running" || stream.status === "waiting_for_input";

useEffect(() => {
  if (messages.length > 0 && !isStreamingRef.current) {
    conv.loadHistory(dbMessagesToTimelineItems(messages));
  }
}, [messages, conv.loadHistory]);

The ref is deliberate, not incidental. If stream.status were a dependency, the effect would fire on the running → complete flip, before the refetch lands. It would load stale messages, momentarily dropping the just-completed output. Read from a ref, the effect fires when messages changes, which is the refetch landing, and by then the status is terminal and the data is fresh. The change is also conservative: !stream.runId was equivalent to idle, while !isStreaming is idle or terminal, so the only added firing is the intended swap.

The test work targets the class of bug, not the instance. The rewritten live-swap test models the real lifecycle, with runId still set at complete, and was confirmed to fail against the old guard before passing with the fix. That failure is what makes it a regression guard instead of a ritual. A sibling AgentPanel test leaning on the same impossible running + runId: null combo moved to a consistent idle state. And the swap was confirmed where it counts: a real browser run rendered “Gemini 3.5 Flash” on the assistant message with no reload.

Two follow-ups came out of the investigation, and only one has landed. The fixture audit is done: every SSE-mock fixture now pairs a live status with a set runId, and runId: null shows up only next to idle, so no test still asserts against a state the hook can’t produce. The second is still open. The setupSSE helper takes raw overrides, so nothing stops a future test from writing complete + runId: null again; auditing the fixtures cleaned up today’s instances without closing the door that let one in. Deriving runId from the status in the helper, or asserting the invariant there, is what would make the impossible pair unwritable instead of merely absent.

The fix regressed the same way, one layer up

The first fix dropped stream.runId from the dependency array, since the guard no longer read it. A reviewer caught that this opened a conversation-switch hole. Switch away from a conversation with an active run to one whose messages are already cached, and the first render still sees the old stream as running, so the guard skips the load. Then the reset effect runs, but nothing re-runs the swap effect, because neither messages nor loadHistory changed.

It turned out stream.runId had been doing two jobs in the original deps: a value the guard read, and a re-run trigger whose set-to-null edge re-fired the effect after a reset. The fix replaced the first job and silently deleted the second. The corrected fix keeps stream.runId in the deps purely as a trigger while the guard reads the ref, and since runId doesn’t change on the running → complete flip, the trigger doesn’t reintroduce the stale load the ref was added to prevent.

Both bugs are the same failure at different layers: something looked like it encoded a concept and only correlated with it. !runId correlated with “not streaming” and broke at complete. The dependency array correlated with “what the body reads” and broke where a dep was secretly a trigger.

So, the invariants I wrote down. When you mock a state machine, every mocked field combination must be one the real machine can produce, or the test asserts fiction. A green gate answers “was this accepted”, never “did this run”. And for behavior that lives in runtime ordering, the browser is the only gate whose opinion binds, so ask it first, not after an outside reviewer does.

Found a mistake, or want to argue about an invariant? eng@shannon.dev or send a PR on the blog repo.