The Internal Review Said Clean. A Second Pass Found Six Real Bugs.

The internal review of a bulk-edit feature came back clean on the questions it asked. Nothing was injectable, every write was scoped to the right account, and the one flagged risk (a set of database writes running as several separate calls instead of one transaction) was rated correctly and had a fix already proposed. I read it, agreed with all of it, and sent the same feature to a second, independent reviewer anyway, mostly out of habit.

The second review confirmed everything the first one found. Then it kept going, because “does the code do what the internal review checked for” and “does the code do what a user with a mouse can make it do” are different questions, and the first review had only asked the first one.

The transaction problem the internal review flagged was about moving a row between two tables: delete from one, insert into the other, no wrapping transaction, so a failure partway through could leave the data half-moved. Correct, and rated high severity, with a fix already proposed. What the internal review didn’t ask was where else that same move already happens. A single-row edit path elsewhere in the same feature carried a code comment saying the field in question was “just a relabel,” implying the record stays in its current table. It doesn’t. The backend routes that field through the identical two-table move the internal review had just flagged as unsafe in the bulk path. The comment describing the behavior was wrong, and more importantly, the fix proposed for the bulk case never touched this second call site, because nobody had gone looking for a second call site.

The one that worried me more doesn’t fail loudly at all. One of the bulk operations shifts a date field by moving it relative to its current value; math on the existing date and a target date. If the existing date on a given row happens to be empty, that kind of date arithmetic in SQL Server doesn’t error. It silently resolves to nothing, and the update sets the field to nothing. No error page, no rejected write. A real value gets quietly replaced with an empty one on whichever rows happened to have no date already. The internal review’s checklist covered the loud failures: things that would blow up in an admin’s face. Nothing on it was built to notice a write that succeeds and is simply wrong.

A related one was almost funny. When you tell the tool to update the field only on rows matching some value you searched for, then leave that search field blank meaning “match everything,” the backend reads the blank as the number zero and filters for rows whose value literally equals zero. Leave the search box empty expecting “apply to everything I selected” and you instead apply the change only to the small subset that happens to already have a zero in that field. Everyone else in your selection is silently skipped. The success message still says how many rows you touched, except that count was taken before the filter ran, so it reports the full number regardless of how many rows the update actually reached.

None of these six findings needed exotic tooling. They needed someone reading the same code with a different question in mind: not “is this reviewed,” but “what happens on every input a real user, with no malicious intent, could plausibly produce.” The internal review wasn’t wrong about anything it checked. It just checked one shape of failure (loud, structural, the kind that throws an error) and the code’s actual weak points were the quiet shape: a write that succeeds, reports success, and is wrong.

I don’t run every feature through two independent reviewers as a permanent policy; that doesn’t scale and most changes don’t need it. What changed is the trigger for the second pass. It used to be “does this touch security.” Now it’s “does this write to the database on behalf of a selection the user built by hand,” because that’s exactly the shape of input a checklist built from known failure modes won’t anticipate.

dxdev LESSON · paste into your AI coding agent
LESSON: A checklist-driven review can be entirely right about what it checks and still miss the write that succeeds, reports success, and is simply wrong
SOURCE: dxdev.com/blog/2026-06-25_internal-review-said-clean-second-pass-found-six
WHAT HAPPENED: An internal review of a bulk-edit feature correctly identified one real issue, two related database writes not wrapped in a single transaction, with a fix already proposed. An independent second review confirmed that finding, then kept going, because the first review had only asked whether the code passed its own checklist, not what a real user with no malicious intent could plausibly make it do. It found the same unsafe two-table move at a second call site the proposed fix never touched, contradicted by a stale code comment claiming otherwise; a date-shift update that silently resolves to nothing rather than erroring when the existing date is empty, quietly replacing a real value with an empty one; and a bulk filter where leaving a search field blank, intended to mean apply to everything, got read as the literal number zero, silently narrowing the update to a small subset while the success message still reported the count computed before the filter ran.
THE RULE: A checklist-driven review answers whether the code passes the checks it was built to run, not what happens on every plausible input a real user could produce, and the dangerous gap between those two questions is specifically the writes that succeed, report success, and are simply wrong: no exception, no rejected write, just a quietly incorrect result.
CHECK MY CODE, then report PASS or FAIL with file:line for each:
      1. Any code review checklist built primarily around loud, structural failure modes (exceptions, injection, unscoped writes) with no explicit check for a quiet failure where an operation succeeds but silently does the wrong thing.
      2. Any fix proposed for one flagged issue without checking whether the same unsafe pattern exists at other call sites performing conceptually the same operation.
      3. Any bulk operation whose success message reports a count computed before a filter or condition is applied rather than the count of rows actually affected, or any blank user input in a filter that gets coerced to a default value like zero instead of being treated as no filter at all.
THEN PRINT: a table (check, PASS/FAIL, evidence, fix) + a verdict (applies / partially / OUT_OF_SCOPE / no) + the single most important next action.