One Util, Used Everywhere: Reviewing a Go Backend Cleanup with Two AI Models

It started with one paginated list that was slower than the other eight. It ended, a day later, as eleven merge requests stacked on top of each other, touching most of a Go backend, with every phase reviewed against the same rule: one util, used everywhere.

This post is about why that was the right time to do it, how the review actually worked, and what two AI models did in the loop. Nothing here is specific to where I work. Every backend built quickly by a small team ends up in the same shape, so the shape is the story.

The shape a codebase drifts into#

Nobody writes four ways to say "not found" on purpose. It happens like this.

A service needs to convert an optional integer into the database driver's nullable type. Someone writes a five-line helper in that package. Three months later a different feature needs the same conversion in a different package. The author does not know the first helper exists, or knows and decides a copy is faster than a shared package. Now there are two. Repeat for a year and there are eleven, and two of them disagree about what to do with a malformed input, and nobody chose either behaviour.

The same drift shows up at every layer:

• Nine paginated lists, one of which scrolled from the top instead of seeking to the cursor.
• Six lists that answered 500 to a garbage cursor and three that answered 400.
• Thirteen routes that said 404 for a malformed id and seven that said 400.
• Seventy-three places that built an error by hand instead of through the named constructor next to it.
• Thirty nil-checks on services that were never nil, defending against a wiring mistake nobody could make.
• Two loggers when tracing was on, and no request id on any line a service wrote.

None of these is a bug you can file. Each is a small tax on every reader and a small trap for every new feature. Together they are the reason "add a field to this list" takes a day instead of an hour.

Why now, and not later#

Refactors like this fail when they are attempted too early or too late. Three things lined up.

There was a net to catch regressions. An integration suite against a real database had landed a couple of weeks earlier, covering roughly two-thirds of the internal packages with real routes, real middleware and real SQL. Before that, a change to error mapping would have been a change to strings nobody tested.

The tooling had just started enforcing things. Git hooks with vet and a full linter set had been wired into the repository the day before. A convention that lives in a document is a suggestion. One that fails the pre-commit hook is a rule. Several phases below end by adding a lint rule that makes the retired pattern impossible to reintroduce.

There was a quiet window. Several feature branches were open, and each one would conflict with a cross-cutting cleanup at rebase time. Doing the cleanup as one serial stack, merged before the next round of features, costs one set of conflicts instead of one per feature.

The alternative was to keep paying the tax and fix things opportunistically. I have tried that on other codebases. Opportunistic cleanup produces a twelfth helper.

The stack#

Each phase is its own branch, based on the one before it, with its own merge request targeting the branch below. Reviewers read one concern at a time. When main moves, one rebase from the tip with --update-refs moves all eleven pointers, and eleven force pushes with lease put them back.

The order was not arbitrary. Pagination came first because it was the original bug and the smallest surface. Deduplication came before error mapping because the error helpers needed a home the dedupe phase created. Wiring came before the query helpers because the query helpers assume a composition root that builds adapters once. Logging came last because every earlier phase changed which struct held the logger.

The bug that started it#

The slow list used a keyset cursor, as the others did, but wrote its seek predicate in the form most people reach for first:

WHERE org_id = $1
  AND (created_at < $2 OR (created_at = $2 AND id < $3))
ORDER BY created_at DESC, id DESC
LIMIT 51

Logically that is a seek. To the planner it is a filter over a range scan on the tenant. Every index entry above the cursor is read and discarded, so the cost grows with how deep the page is.

The fix is a row comparison, which the planner turns into a single index condition, plus an index whose columns and directions match the sort:

WHERE org_id = $1
  AND (created_at, id) < ($2, $3)
ORDER BY created_at DESC, id DESC
LIMIT 51

One migration and one predicate. That would have been a fine one-line fix. The reason it became a phase is that the other eight lists already used a shared helper that wrote the row comparison, and this list had its own private cursor type with its own encoder. The wire token was byte-identical. There was no reason for the second copy to exist, and once it was gone, the question was how many other second copies there were.

The review loop#

The rule for the whole day was that nothing gets committed until it has been read against a strict rubric. Not "does it work" but "does it make the codebase simpler than it was, and is there a restructuring that would delete more".

The rubric is a prompt I keep as a reusable skill. It pushes hard on structural questions: is there a code-judo move that removes a layer instead of polishing it, did a file cross a size threshold, did a special case land in a busy flow, is a new helper a pass-through wrapper, is logic in the layer that owns the concept. It is deliberately demanding. The bar for approval is not correctness.

The review earned its keep more than once. The pass over the composition-root phase found that four handlers were still reading repositories directly, bypassing the service layer. That became its own commit and its own lint rule: services and handlers may not import adapters, enforced by the linter, shown red with twenty-four hits before it went green.

"Done" is a claim#

The most important habit from the day was refusing to accept "the phase is done" from anyone, including myself and including the model.

A refactor phase has a natural end: the obvious copies are folded, the tests pass, the diff looks complete. That end is a feeling. The alternative is a census: a query over the whole tree that returns zero when the phase is actually closed.

func optionalInt4(v *int) pgtype.Int4 {...}
// identical body in a second adapter

func toInt4(v *int) pgtype.Int4 {...}
// body hashes equal once the name is stripped

func fromProto(t *timestamppb.Timestamp) time.Time {
  // scanner sends UTC
  ...
}

conn := repotesting.Connect(t)
tx, _ := conn.Begin(ctx)
t.Cleanup(func() { tx.Rollback(ctx) })

models.Member{ID: r.ID, Email: r.Email, ...}
// and again, inline, in the list query

rows.Scan(&id, &capability, &exposure, &governance)
// swap two: tests stay green

Each time I said "check again", the model had to invent a new angle it had not used before and run every angle over the current tree. The dedupe phase closed after six checks. The query-helper phase closed after six. The error phase after ten. Every one of those extra checks found something the previous angles had missed:

That last row is the honest one. The model called the same three spies "local types, leave them" on three consecutive checks. They were fakes with a different receiver. The sixth angle looked at the body instead of the receiver and found them immediately. Nothing about the model got smarter between check five and check six. The question got better.

The logging phase#

The last phase deserves its own section because it is the one most teams recognise.

The backend had structured logging with a well-known library, and tracing through OpenTelemetry. Both worked. Neither knew about the other. A service line said "audit event not recorded" and nothing else: no request id, no tenant, no actor, no trace id. Finding the request that produced it meant grepping timestamps.

The fix has three parts and none of them is clever.

One logger, built once. Two were being built when tracing was on: one plain, one bridged to the collector. Now there is one, built in main before configuration is even validated, and the tracing bridge tees onto it. Ten call sites that reached for a global now take the logger as a parameter.

Fields on the context, stamped at the edge. Middleware stamps the request id, the tenant, the role and the actor onto the request context once. A worker acting for one tenant stamps that itself. Everything downstream calls a one-line method:

func (s *Service) log(ctx context.Context) *zap.Logger {
    return logging.From(ctx, s.logger)
}

s.log(ctx).Warn("audit event not recorded",
    zap.String("event", ev), zap.Error(err))

Records linked to spans. The context rides along as a field the JSON encoder skips and the tracing bridge reads, so the collector nests each log record under the span that produced it, without any parsing of fields. Seventeen structs gained the method and fifty-nine call sites use it. An AST rewriter did the mechanical part.

The rewriter also produced the day's worst moment. Its first version computed byte offsets from the wrong base and corrupted eight files, every one of which started with a comment. The files were restored from git and the offset came from the token file's method instead. The lesson is old: a tool that rewrites source needs a diff review like any other change.

Smaller decisions from the same phase, each a one-line reversal if they turn out wrong: health probes no longer produce an access-log line, 4xx summary lines log at info instead of warn, handler panics go through the logger with a stack instead of the framework's coloured stderr block, the HTTP server's own errors go through the logger too, and the log level can be switched at runtime on the debug listener without a restart.

The last two globals#

With the logger done, two globals remained: the config accessor and the process-wide connection pool. Same shape, same technique.

The pool was opened lazily on first use, which meant a database that was down at boot surfaced as a panic from whichever goroutine happened to touch it first. Now it is opened once in main, with a clear fatal message if it fails, and every adapter takes it as a parameter. A test that needs a repository hands in a mock pool. A test that does not never opens a connection.

What the two models did#

I used two models across the day and they were not interchangeable.

Fable 5.1 did the long-horizon work. Eleven phases, around a hundred commits, and a standing set of rules that had to hold across all of it: no commit until I had reviewed the diff, one logical change per commit, no attribution trailers, comments of one line, never push to main, show a test red before making it green. It held those rules for the whole session without being reminded. It wrote the AST census tools, ran them, and rewrote them when an angle came back empty. When I said "check again" it did not re-run the previous census and report zero. It invented a new one.

Where it fell short was exactly where you would expect a fast worker to fall short. It called things "done" that were not. It dismissed the same three test doubles three times. It condensed two comments and carried a claim into the shorter version that was no longer true. It introduced a flaky test by reusing a fixed id across packages that ran in parallel. Every one of those was caught by the loop, not by the model.

Opus 5.5 was the second reader. I handed it each merge request cold, with none of the reasoning that produced it, and asked the questions a teammate would ask: does the description match the diff, is anything in here a wire change that is not called out, would a reviewer who has not seen the previous ten branches understand why this one exists. It was also the faster model for the small, bounded jobs in between: reading one file, explaining one plan, drafting one MR description from a diff. A reader with no memory of the journey is a good proxy for the reviewer who will actually see the MR, and a cheaper model for the bounded jobs keeps the expensive one on the long thread.

Neither model reviewed itself well. The useful configuration was a worker with the full context and a reader with none of it, and a human between them who refused to accept "done".

What I would tell myself at the start#

Measure the bug before touching the code. The EXPLAIN output for the slow list was the whole argument for phase one. Without it, the index migration is a guess.

A wire contract is a contract even when it is ugly. Several folds changed which helper produced a message but kept the bytes identical. The handful of reachable wording changes were listed in the merge request, one line each, as decisions for the reviewer.

Uniform beats correct-in-isolation. A 400 for a malformed id everywhere is better than the best-argued mix of 400s and 404s. Every product call of that shape was made once, then applied by census.

Delete the guard nobody can trigger. A nil-check on a dependency the composition root always provides is a comment pretending to be code. The tests that pinned those guards were calling a literal nil.

Make the boundary a lint failure. Every phase that established a convention ended by adding the rule that makes the old pattern fail the build. Documents drift. Linters do not.

"Check again" is a verb. The difference between a phase that is done and one that feels done is a new query over the tree. If you cannot name the query, you do not know it is done.

None of this needed a large language model. All of it went faster with one, on the condition that the human in the loop treated every "done" as a claim to be tested rather than a result to be accepted.