Two route handlers called the same upstream service, parsed the same payload, and looked almost identical. The agent read both, concluded the block was duplicated, and lifted it into one shared helper. On the surface the diff was a clean win: less code, one place to fix the parsing, fewer chances to drift. Tests stayed green. The fault behavior did not survive the move.
Duplication hides differences that look cosmetic in the happy path and matter only when something breaks. Extracting the block freezes one chosen exit for both callers, even when the callers legitimately wanted different exits.
Why duplication looks like the obvious target
Refactor prompts reward removal of repetition. When two functions share a chunk of lines, the agent reaches for the textbook move: extract, parameterize the inputs, call the helper from both sites. That instinct is right when the two sites truly do the same thing. It breaks when the repetition is only on success and diverges on failure.
Here the first route, an internal write endpoint, was meant to rethrow the upstream error so the framework returned a 500 and the caller could retry or open the circuit. The second route, a read endpoint, was meant to catch the same failure and serve a cached fallback with a 200 so the page still renders during a blip. The agent copied the read route's catch branch into the helper because it was the more recent of the two.
The two callers were never actually identical
Compare a function by its happy path and the differences vanish: same call, same parse, same return shape. Compare it by every exit branch and the picture changes. One path propagates the error; the other swallows it and substitutes fallback data. One expects the caller above it to handle fault; the other expects the helper to handle fault locally.
A shared helper cannot honor two conflicting error policies at once without an explicit choice passed into it. Left to its own device, it adopts whichever branch appeared in the block the agent copied most closely, and the other caller quietly adopts that policy whether it wanted it or not.
Extraction collapses the divergent exit
After the move, the write endpoint returned the cached fallback instead of a 500 when the upstream failed. The client saw a success-shaped response carrying stale or missing data, retried nothing, and the circuit never opened. Nobody got an error to page on; the system looked healthy while silently serving degraded writes.
The test suite missed it because the tests that covered the helper mocked a successful upstream call. The read endpoint's fault path still had a test for the fallback. The write endpoint's rethrow path, which existed before the extraction, was never re-exercised after the block changed hands. A green suite on a refactor proves the happy callers work, not that every old exit still fires.
Verify the extracted helper against every caller's fault path
Before approving this kind of refactor, trace each caller's original exit and name the expected behavior under fault: rethrow, fallback, null, empty response, retry, or abort. Then force the upstream to fail for every caller after the move and confirm the response still matches that original expectation. If a caller genuinely needs its own error policy, pass it in explicitly rather than letting the helper pick by default.
The rule that catches most of these: duplication of success is worth removing, divergence of failure is not. When the only difference between two blocks is what happens on error, that difference is the contract, and extracting it away quietly changes the contract without any line that looks like a regression.
Top comments (0)