In July I was looking at our charging map and saw two pins where there should have been one. Same charging site, a few metres apart, two listings. I zoomed out and found another. Then another.
This should not happen. Our map pulls charging stations from several sources: the German federal register, the roaming networks, Tesla. The same physical site often appears in more than one of them, and a nightly job decides which copy users see. That job had been running for fourteen months. It had been changed twenty times. At the end of May it had been refactored, made faster, made to use less memory, and given its first proper test suite. All tests passed.
The fixture said the operator names match. In 9,269 real pairs, they matched once.
When we measured, a third of the register listings we were showing had a duplicate from another source within a hundred metres.
This is about how that happened, and about what I have changed in how I review code I no longer write myself.
Every change was an improvement
The idea behind the job was simple. Run a series of passes over the data, in order. Each pass asks whether two records share a key, for example the same charger ID, exactly the same coordinates, the same address with the punctuation stripped, the same rounded position and the same operator. If they do, hide the lower-priority copy.
Most of it was written by agents, one pass at a time. Each change was small, readable and reasonable. Each one solved the problem in front of it.
The refactor in May was the most polished of all. The instruction was to consolidate the passes while preserving behaviour, and it did exactly that. It preserved the behaviour. It also wrote fifteen tests, and those tests are where the flaw was hiding.
The tests agreed with the code
In the test fixtures, the register record and the roaming record for the same site carried the same operator name. It is the obvious way to write that test. It is also almost never true.
In the real data, the names matched once in 9,269 pairs of duplicates. The two sources name operators differently: a municipal utility on one side, the company running the charging network on the other. Both are correct. They are never the same string.
One of the tests even asserted the flaw as intended behaviour: records from different companies must never be merged. In the real data, that was nearly every duplicate we had.
Test data that nobody took from reality does not test the concept. It tests whether the code agrees with the assumptions of whoever wrote it. When the same agent writes both, the answer is always yes.
The moment that did not happen
If I had written that test by hand, I would have needed a duplicate pair. I would not have invented one. I would have opened the production mirror and pulled a real one, because that is easier than making one up convincingly. And I would have seen a utility's name on one side, a network operator's on the other, and the pins a few metres apart. The concept would have broken right there, in the fixture, before any code was reviewed.
That is what the slow work used to do for me. Writing the test was not just checking the code. It was the moment the concept first met the data. Nobody scheduled it. It was a side effect of the effort, and when the effort went away, it went with it.
I have to be honest about the rest. The refactor was merged seventy-eight minutes after it was opened, without a review. That is a failure, and it is mine. But I have reread that change since, carefully, and I am not sure I would have caught it. The code is clean. The tests are thorough, well named, and they pass. There is nothing wrong in the diff.
The flaw was never in any diff. It was in an assumption that twenty diffs had inherited. By July, the newest pass carried a comment in capitals: MUST stay last. That is a design telling you something. At the time it read like care.
The fix was also written by an agent
This is not a story about the machine getting it wrong. The replacement was agent-written too. What changed was the question.
"Preserve the behaviour" gets you a better version of whatever you already have, flaw included. "Take a snapshot of production and tell me how many duplicates users can see" gets you a number. The number was a third. From there it took four days to replace the passes with something that matches on distance and street name and does not depend on the order it runs in. Two more corrections followed, and both were found by dry runs against real data.
Same kind of tool, same quality of code. The difference was whether anyone was asking about the outcome or only about the code.
Reviewing the code
I still review every change, and most of how I do it has not changed. I just know now which level it works on.
Small steps. I ask for a plan, then for the first step. A two-thousand-line change cannot be reviewed. It can only be approved. A hundred and fifty lines can be read and argued with in ten minutes. Across the summer, the median change in my repositories stayed at around 35 added lines while the number of changes more than doubled.
Read it where I would have written it. The agent's summary is a claim, not evidence. One of the changes in this story described a functional test suite it did not contain. I open the code in the IDE, follow the calls, run the tests, set a breakpoint.
Attack it. I assume the change is correct and try to prove otherwise: the empty list, the second call, the timeout, the error that is logged and ignored. The attempt is the review.
Rename and delete. If I cannot name something better than the agent did, I do not understand it well enough to approve it. And the generated version of anything is too complete. Something can usually go.
Distrust coverage. For each test I ask whether it would fail if the behaviour I care about broke. If not, it is documentation, not protection.
All of this is worth doing. None of it would have found the duplicates. Small steps kept every change reviewable, and every change was fine. That is the trap: twenty correct steps can carry a wrong idea a long way.
Reviewing the concept
So I have added a second level, and it works on different material.
Where did the test data come from? For anything that models the outside world, I want at least one fixture taken from real data. An invented fixture tests the code. A real one tests the idea.
Measure the outcome, not the algorithm. The refactor was checked against real data, but only through the job's own statistics: how many records each pass touched. They looked fine. They described what the code did, not what users saw. I now ask for one number that describes the result from the outside, and I check it against production.
Read the comments that apologise. "MUST stay last", "workaround", "for now". Each one marks a place where the design pushed back and the code pushed through.
Look at the product. No test found this. I found it by looking at the map. It is the cheapest review there is, and the one I did least.
Where the saved time should go
The agents have given me back a lot of time. So far, most of it has gone into more changes: another step, another feature, another summary accepted because it reads well. That is the easy place for it to go, and I think it is the wrong one.
The time belongs one level up: what the system assumes about the world, where its data comes from, which rule depends on which. That used to be the expensive kind of thinking, and most of us only did it when something was already on fire.
It is also where the tools are strongest, and where I used them least. An agent can read a whole service in minutes and draw it: the data flow, the order of a nightly job, which component trusts which input. Ten minutes with a good diagram tells me more about a design than an hour of reading diffs.
A diagram of the deduplication job would have shown six boxes in a fixed order, each comparing one exact field across sources. Anyone who knows how the register and the roaming networks name their operators would have stopped at the box that compares operator names.
That is the part the agent cannot do. It can draw the picture. It cannot tell that the picture is wrong, because it does not know which names never match, or that a comment in capitals is a warning. That knowledge is what years of building things gives you, and for an experienced developer, having the picture handed over ready to read is a larger gain than the typing we no longer do.
The picture is still a claim, like the summary. It shows what the code does, not whether that fits the world. So I check it the same way: against one real case.
What review is now
I used to think of review as the part after the work. It turns out it was always the part that decided whether the work was any good, and some of it happened while I was writing: in the tests, in the fixtures, in the small resistance of code that did not want to fit.
The machine has taken the writing, and with it that resistance. What it has given back is time, and tools that can show me a whole system in one picture. I can spend that time on more changes. Or I can spend it where the flaw in this story actually lived: in the design, the assumptions, the big picture. That is what experience is for.
So I still try to break the code. But more and more, I spend my time trying to break the idea behind it, before twenty good changes are built on top of it.
Originally published on Medium.
Top comments (1)
The test that asserted the flaw as intended behaviour scares me most. "Records from different companies must never merge" reads like a sensible safety rule, so nobody pushes back on it in review.
The cheapest check I know for generated fixtures is putting one real row from production next to the made-up one before reading the test. If they don't look alike, the test is checking someone's idea of the data.