A Latent Upstream Bug, Found and Fixed in About 30 Minutes

Page content

I keep seeing arguments about AI coding assistants conducted entirely in the abstract, so here is a concrete one: a real change I made recently, start to finish, including the parts that went wrong. Everything below is a real MCUboot change; the PR numbers are live and you can check my work.

Summary

An automated review of an external contributor’s pull request flagged that the author had quietly disabled a test on one simulated device. Rather than stopping at “you disabled a test”, the model worked out why the test failed — a long-standing bug in MCUboot’s own test simulator that had been invisible for as long as it had existed, and only became observable because this PR raised a configuration limit. The fix was implemented, reviewed, pushed as PR #2808, and then empirically demonstrated to resolve the original author’s problem.

Cost: roughly 30 minutes of my attention, some of it spent doing other things while test suites ran. My honest estimate for doing this by hand is one to two days — most of it spent staring at trailer arithmetic trying to work out why an “oversized” image wasn’t oversized.

The tool was Claude Code running Opus 5.

Background, briefly

MCUboot is a secure bootloader for microcontrollers. It reserves space at the end of each flash slot for a “trailer” holding upgrade state. How much space depends on the flash write alignment and on the device’s sector layout — arithmetic that exists in two places: the firmware itself (boot/bootutil/src/swap_scratch.c) and, independently reimplemented, in the Rust test simulator (sim/src/image.rs) that generates test images.

Two implementations of the same formula. They had drifted.

PR #2797 (“boot: imgtool: Extend align to 128B”) raises the maximum supported flash write alignment from 32 to 128 bytes. That is a perfectly reasonable feature. It also happened to push the trailer arithmetic into the region where the two implementations disagree.

What happened, stage by stage

1. Automated review found the suppression and the root cause

I ran an automated review over #2797. It reported that the author had added this above one of the simulator’s tests:

#[cfg(all(not(feature = "max-align-64"), not(feature = "max-align-128")))]
sim_test!(oversized_secondary_slot, ...);

Spotting a disabled test is easy, and any reviewer would have caught it. What was not easy — and what I would have had to do myself — was answering the next question: why did it fail?

The model worked it through. The simulator’s estimate_swap_scratch_trailer_size() computed the trailer padding by looking at one flash slot: whichever one it happened to be installing an image into. The firmware’s app_max_size_adjust_to_trailer() deliberately looks at both slots and takes the larger, because the swap algorithm copies data on the common sector boundary of the two slots, so the larger sector is the unit that matters.

On a device whose two slots have different sector sizes, the simulator therefore computed the padding from the small sector while the bootloader computed it from the large one. The simulator believed less space was available than the bootloader did. The test that builds a deliberately-oversized image was building one that fit comfortably inside the real limit; the bootloader correctly accepted it; the test reported failure.

This bug had been in the tree for as long as that code had existed. At alignments of 32 bytes and below the two models happen to agree, so nothing ever showed it. Raising the alignment limit is what made it visible — which is exactly why it landed in the lap of a contributor who had nothing to do with causing it, and who reasonably reached for a #[cfg] to get his own feature moving.

This is the part that saved the day or two. The symptom (“an oversized image was accepted”) points nowhere useful. The path from there to “two divergent implementations of a sector-boundary calculation, one of which only consults one slot” is a genuine debugging slog.

2. A written plan as the handoff between contexts

I asked the reviewing agent to write a detailed description of the change MCUboot needed — not to make the change, just to describe it. That produced a markdown file: the diagnosis, a worked numerical example, a sketch of the new function, the caller changes, and an explicit verification protocol.

I then started a fresh agent in the MCUboot project itself and handed it that file.

This turned out to matter more than I expected. The review agent had the PR loaded and the diagnosis fresh; the implementing agent had the actual repository, the build, and the test suite. Passing a written artifact between them meant the second agent inherited the reasoning without inheriting a context window full of review chatter. It also left me with a reviewable document before any code was written — I could disagree with the plan cheaply.

3. Implementation, review, and a PR description better than I would have written

The implementing agent made the change, split it into two commits (a small prerequisite exposing an existing firmware function to the simulator, then the fix itself), and ran the full simulator suite across twelve feature configurations. I reviewed the commits, then asked it to push and open a PR explaining the relationship to #2797.

The resulting PR #2808 description explains the divergence, gives a before/after table of the actual computed values, explains why the bug is latent on main, and states which #[cfg] in #2797 can now be removed and why leaving it would be harmful.

I will be honest about this: that is more justification than I would have written for myself. Not because it isn’t worth writing — it plainly is, for a change to shared test infrastructure that another contributor is blocked on — but because writing it is tedious, and when the fix is already in front of you the temptation to post “fixes trailer size calc for asymmetric slots” and move on is considerable. Marginal effort that I would have skipped got done because it cost me nothing.

4. Empirical proof, in five minutes

The remaining question was the one that actually mattered to the other contributor: does this fix your problem?

I asked for a temporary merged tree combining #2797 and #2808, with the author’s #[cfg] suppression removed, and a test run to find out.

In about five minutes it built the merged tree, deleted the guard, and ran a clean A/B:

max-align-64 max-align-128
#2797 alone, guard removed FAILED FAILED
#2797 + #2808, guard removed passed passed

It then instrumented the device loop to confirm the failing device was the one the diagnosis had predicted (stm32f4SpiFlash, at alignment 64) rather than something incidental, and ran the whole suite at the new alignments across the other upgrade modes to check that dropping the guard exposed nothing else.

I posted the result on #2797. The author now has evidence, not an assertion, that he can delete his workaround.

The A/B is the step I want to draw attention to. It is not hard, but it is fiddly — fetch a PR, construct a throwaway merge, patch out a #[cfg], run two builds, keep the arms straight in your head. It is exactly the sort of task I would have talked myself out of and replaced with “I’m fairly confident this fixes it.” Making it cheap made it happen.

Where it needed watching

I would be misrepresenting this if I described it as four clean handoffs. Two things went wrong, and both are instructive.

The diagnosis was right but incomplete. The review named Stm32f4SpiFlash as the only affected device. During verification the implementing agent diffed the computed trailer sizes before and after the change and found a second device — Nrf52840SpiFlash, 4K sectors in one slot and 8K in the other — whose numbers also moved, on main, today. The review had asserted the fix would be a strict no-op on main; it isn’t quite. The change is benign (it only ever increases the computed budget, so it cannot turn a rejection into an acceptance), but it needed to be described accurately in the commit message rather than as the pure cleanup the plan predicted.

What caught this was that the plan itself demanded a before/after comparison, and that comparison was actually run rather than assumed. An AI-written verification protocol caught an AI-written analysis error. That only works if you actually execute the protocol.

One suggestion in the plan was simply wrong. It proposed widening a test’s safety margin to 4 * align bytes. At the new maximum alignment that value exceeds the space reserved in one of the upgrade modes, so the “test” image would have been written past the end of its flash slot. It was flagged and dropped rather than implemented.

Neither of these was catastrophic, and both were caught by the process. But they are the reason “review the commits” was a real step and not a formality. The failure mode to guard against is not gibberish — it is plausible, well-argued, and slightly wrong.

What actually made this work

  • Root-causing, not just flagging. The value was not “you disabled a test.” It was the chain from that symptom to a specific divergence between two implementations of one formula. That is the expensive human activity, and it is where the day-or-two went.
  • A written plan as the interface between contexts. A markdown file handed from a review context to an implementation context beat trying to do everything in one long session. It also gave me something to disagree with before code existed.
  • Cheap experiments change which experiments you run. The merged-tree A/B, the twelve-configuration regression sweep, the before/after instrumentation — I would have done perhaps one of those three by hand, and I would have guessed at the other two.
  • Marginal-effort work gets done. Thorough PR descriptions, proof instead of assertion, a note back to the blocked contributor. Each is individually skippable, and each usually gets skipped.
  • Version control that makes throwaway trees free. All of this was done with jj, where creating a temporary merge, testing it, and abandoning it costs nothing. Anything that raises the cost of a throwaway experiment eats directly into the benefit above.
  • The verification step is not optional. Both defects above were caught by verification, not by review of the reasoning. The reasoning read fine.

The arithmetic
My time ~30 minutes, some of it spent elsewhere while suites ran
Estimated time by hand 1–2 days
Output Root cause, 2-commit fix, upstream PR with full justification, empirical proof for the blocked contributor, regression sweep across 12+ configurations

The distribution matters as much as the total. The saving was not evenly spread — it was concentrated almost entirely in stage 1, the diagnosis. Stages 3 and 4 were things I could have done quickly; the point is that at their true cost I probably would not have done them at all.

Artifacts