DEV Community

Cover image for I shipped a fix for a breaking change. The code could never select it.
LucioLiu
LucioLiu

Posted on

I shipped a fix for a breaking change. The code could never select it.

#bugsmash #devchallenge #debugging #python

Summer Bug Smash: Smash Stories πŸ›πŸ›Ή

This is a submission for DEV's Summer Bug Smash: Smash Stories, powered by Sentry.

Upstream renamed two files. agent-template.xml became agent-template.md. Anyone who had copied that skeleton and referenced it by name in their own scripts was about to have a bad afternoon, so I added a row to the upgrade doc telling them exactly what to re-copy.

The upgrade tool worked out which row to print by reading the kernel filename. This release didn't touch the kernel. Its filename was the same as the previous version's, byte for byte.

So the row I had just written could never be selected. Not rarely, not on some paths. Never, for as long as one layer of the product could ship without renaming another.

The existing release verification still reported 135 green checks across its smoke suites.

Why one row in a markdown table was worth this much trouble

I maintain agent-modpack, a small open-source packer. It bundles a persona kernel, a template library, team rules and a handful of skills into a folder you unzip and open with whatever AI coding tool you already have. Its Python installer exposes four commands: assemble, upgrade, revert, import.

upgrade is the interesting one. By the time someone runs it, they have a working folder full of their own edits and their own memory files. Upgrading cannot mean "overwrite and hope." So the tool prints a plan, does the mechanical parts, and then prints a section I call β‘€ asset sync to-dos: the things it deliberately will not do for you, lifted verbatim from the upstream repo's docs/UPGRADE.md table.

That printed section is the tool's entire mechanism for saying "this release breaks something you did by hand." If it prints the wrong rows, it does not crash, it does not warn, and it does not fail a test. It just quietly tells the user everything is fine.

The before

Two functions, thirty lines apart.

def _version_label_from_kernel_filename(filename: str) -> str:
    """Derive the UPGRADE.md table label from a kernel filename
    (nuwa-v10.md / nuwa-9.0.xml -> the table's bare 'v10' / '9.0' labels)."""
    token = _extract_version_token(filename)
    return token or filename

# ... and the only caller that mattered:
version_label = _version_label_from_kernel_filename(plan["sourceKernelName"])
asset_row = load_upgrade_asset_row(nuwa_root, version_label)
Enter fullscreen mode Exit fullscreen mode

Read in isolation, that is a perfectly reasonable helper with a docstring that tells the truth about what it does. The bug is not in either function. It is in the sentence nobody had written down: the filename is the version.

That had been true for the entire life of the project. Every previous release changed the kernel, and every kernel change came with a rename. The assumption was load-bearing and invisible, which is the combination that gets you.

Attempt 1: the row that was dead on arrival

Upstream shipped 10.1.0 as a templates-only release:

agent-template.xml        ->  agent-template.md
agent-template.zh-CN.xml  ->  agent-template.zh-CN.md
Enter fullscreen mode Exit fullscreen mode

The kernel, nuwa-v10.md, was byte-for-byte unchanged. Which means its filename was unchanged too, because there was nothing to rename it for.

I wrote a 10.1 row into the upstream asset table describing what to re-copy and what to leave alone, and I was about ten minutes from shipping it.

Then I went to read how the tool actually picks a row. Not because anything looked wrong. Because I had just recommended a fix and had not yet watched the code agree with me.

_version_label_from_kernel_filename("nuwa-v10.md") returns "v10". The lookup asks the table for v10, finds it, prints it, and stops. My new 10.1 row sat one line above it, permanently out of reach.

The reason this is worth writing up is not that the helper was wrong. It is that the doc change and the code were each individually correct and jointly useless, and nothing in the system was capable of noticing that.

Attempt 2: what I shipped that day, and why I refused to call it a fix

A release was going out. The correct repair touched row selection, which has a regression surface I did not want inside a release commit. So I shipped the smallest thing that put the warning in front of a human, with zero code change:

  1. Kept the 10.1 row. That version happened; the table stays honest.
  2. Added an explicit pointer inside the v10 row, the one the tool does print:

There is one more breaking change after v10 (skeleton xml to md): look one row up, at 10.1. 10.1 changed nothing in the kernel and the file is still nuwa-v10.md, so any tool or process that infers a version from the kernel filename will stop at the v10 row and miss the rename.

  1. Ran upgrade preview against a real base folder and read the output with my own eyes.

Step 3 is the only one that proved anything. Attempt 1 also looked right in the diff.

I wrote in the commit message that this was navigation for a human, not a repair: ship 10.2 and I would be hand-writing the same paragraph again. Naming your own workaround as a workaround costs one sentence and buys you the right to come back to it.

Attempt 3: the actual root cause

Two days later, no release pending, I went back for it.

Fix one: the version label gets a single source, and the output says which source fired.

The manifest already carried the real product version. The packer had been reading it correctly for its own component table for a while. The upgrade path was the only place still guessing from a filename.

def resolve_target_version_label(manifest, source_kernel_name) -> tuple[str, str]:
    """Decide which row to look up. Returns (label, where the label came from).
    Priority: productVersion > kernelVersion > kernel filename (fallback)."""
    nuwa = manifest.get("sources", {}).get("nuwa", {})
    product_version = str(nuwa.get("productVersion") or "").strip()
    if product_version:
        return product_version, f"productVersion {product_version} (manifest)"
    kernel_version = str(nuwa.get("kernelVersion") or "").strip()
    if kernel_version:
        return kernel_version, f"kernelVersion {kernel_version} (no productVersion declared)"
    fallback = _version_label_from_kernel_filename(source_kernel_name)
    return fallback, f"derived from kernel filename {source_kernel_name} (fallback)"
Enter fullscreen mode Exit fullscreen mode

The return type is the point. It hands back where the number came from next to the number, and the tool prints that line on every run. The failure mode I had just lived through was silent; the next one announces which rule fired.

The old helper survives as the last fallback, demoted in its own docstring so the next reader inherits the assumption instead of rediscovering it:

    """LAST-RESORT FALLBACK, not the source of truth. A filename only equals a
    version under the assumption that the kernel is renamed whenever the version
    moves, and the template layer can evolve independently of the kernel."""
Enter fullscreen mode Exit fullscreen mode

Fix two: a range, not a point.

The old code looked up exactly one row, which is wrong even when the label is right. Someone upgrading a 9.0 base to 10.1 needs every version they are jumping over, not just the destination. Selection became every row where current < x <= target, with comparison normalized to zero-padded tuples so the table's historical mix of v10 / 10.1 / 9.6 sorts correctly without a data migration.

For that 9.0 base, this is the difference between printing 1 row and printing 8.

And a third face I only found because I went looking. preview returned early on ALREADY_UP_TO_DATE, before it reached β‘€. A byte-identical kernel is not the same claim as "your companion assets are in sync," and a templates-only release is exactly the case where the kernel is identical and there is still work to do. That early return would have swallowed the entire section for precisely the users who needed it. Same root cause, different symptom, and it would have survived the first fix untouched.

Proving the regression lock isn't decorative

I added a case to tests/upgrade_smoke.py. It parses the manifest and UPGRADE.md itself and computes the expected row set independently. It does not import a single function from the code under test, because a test that imports the buggy helper inherits the bug and passes.

Then I checked that the lock actually locks:

with the fix:                                          35/35 pass
upgrade.py reverted to the pre-fix version, same test:  31/35   (4b-4e fail)
Enter fullscreen mode Exit fullscreen mode

Five new assertions, four of which go red against the old code. Before that experiment I had a test I hoped about. After it I had a test I believe.

Before vs after

Before After
Version label source kernel filename (nuwa-v10.md -> v10) productVersion, then kernelVersion, filename last
Output says which rule fired no yes, every run
Rows printed, 9.0 base to 10.1 1 8
Templates-only release invisible to the user printed
ALREADY_UP_TO_DATE returns before printing β‘€ prints β‘€ first
Assertions guarding this path 0 5, four verified red against pre-fix code

The same shape, three more times

Once you have a name for it, you start seeing it. All three of these are the same mistake: trusting a proxy for the fact instead of the fact.

A constant that outranked the tag. Cutting v0.5.1, installer/lib.py still said:

MODPACK_VERSION = "0.5.0"
Enter fullscreen mode Exit fullscreen mode

That constant is the single source the packer reads to stamp team-config.json and to generate the upgrade docs that ship inside the zip. The tag would have said 0.5.1 while every file inside said 0.5.0.

Then the same class again, one release later. Cutting v0.6.0, the release-notes component table printed the upstream version by reading kernelVersion, which said 10.0. Upstream had published 10.1.0. Both numbers were correct. They had genuinely decoupled, because 10.1.0 was templates-only. My table simply had no way to express that.

  "kernelFile": "nuwa-v10.md",
  "kernelVersion": "10.0",
+ "productVersion": "10.1.0",
Enter fullscreen mode Exit fullscreen mode 
"version": f"v{nuwa.get('productVersion') or nuwa['kernelVersion']}",
Enter fullscreen mode Exit fullscreen mode

Prefer the new field, fall back to the old one, so older manifests keep working.

Two releases in a row is not carelessness, it is structure. Anything that ships an artifact has multiple copies of its own version by construction: a constant, a tag, a manifest, generated docs, the config inside the zip. The rule I wrote down: when you bump a version, grep for who else in this artifact declares one.

A commit that did not exist. One step of this work ran against a sandboxed checkout. It reported success and handed back a hash.

$ git cat-file -t d1a4c92
fatal: Not a valid object name d1a4c92
Enter fullscreen mode Exit fullscreen mode

Grep for the change: nothing. The write had landed in an overlay that never reached the real disk, and the layer above reported success anyway. "Committed" is a claim; git cat-file is a fact. The check also has to be per repository, because the same operation landed correctly in one repo and vanished in another, and one green check would have read as an all-clear.

What I did not fix

The filename fallback is still there, and it still runs. An existing base folder has no other version marker in it, so when the tool needs the lower bound of the range it has nothing to read but the kernel filename. If someone assembled their folder by hand and never ran this tool, that lower bound is still a guess. It is now a labelled guess that prints its own provenance, which is an improvement and not a fix.

The asset table is also still a markdown table parsed out of an upstream doc. Change the column layout upstream and parsing degrades. The code has an explicit branch for that ("could not parse the table, check the original by hand"), so it degrades loudly instead of silently. A loud failure is still a failure.

And there is still no test for the thing that actually failed here, which is a human reading the warning. I do not think one exists. Running it and reading the output is the whole method.

What I'd tell my team tomorrow

  • Anything derived from a name is a fallback, and it should say so in its own docstring. Filenames, branch names, directory names. They agree with reality until the day one layer moves without the other.
  • Make the code report which rule fired. Returning (value, provenance) cost one tuple and turned a class of silent misfires into a visible line of output.
  • Point lookups are a bug when users can skip versions. Ask for the range.
  • Early returns are suspects. ALREADY_UP_TO_DATE was a true statement about the kernel and a false conclusion about the release.
  • A regression test is only real once you have watched it go red. Revert the fix, run the test, count the failures. If the count is zero you wrote a decoration.
  • Read "done" back off the disk, per repository.
  • If a feature's only job is to print a warning, run it and read the warning. Every test can pass while the thing you shipped it for never happens.

That last gap is where all of this lived. Green tests measure whether the code is correct. Nothing in the suite measures whether the message arrived.

Links

agent-modpack is open source. The code discussed above:

AI assistance disclosure

This post is written in the first person for readability, but the engineering work it describes was carried out by an AI agent working on agent-modpack, a project I own and direct. Spotting that the row could never be selected, rejecting the first attempt as navigation rather than a repair, choosing to hold the real fix until no release was pending, and building the regression lock: those were the agent's calls, and the reasoning narrated above is its own working log rather than a story reconstructed afterwards.

I mention this because the post argues for not shipping claims you have not watched hold up. The same standard applies to the byline.

AI tools also assisted with research, drafting, structuring, and editing this write-up. Before publication, the technical claims, repository links, and test counts were checked against the public project files and commit records.

Top comments (0)