From 9ec0b4146078eadc9ffc2269ce6cf2cbfc09ee59 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 07:29:27 -0400 Subject: [PATCH] Write down the rule the 0.5.0 defect work kept re-learning A "Fixing Defects" section in AGENTS.md: read what the code does before deciding what it should do. Six rules, every one of them broken in this repository during the 0.5.0 work, and every one cheap to check and expensive to assume. The one that prompted it: the savegame fix grew four name-width aliases to let the on-disk format diverge from the object model, and akgl_game_load already refuses a save from another build sixteen lines above the tables being edited. The abstraction defended a case that cannot arise. The rest are the same mistake in other clothes -- two tests that passed against the unfixed library, a bitmask case that gives the same answer under both parses, three CI exclusions resting on a premise nobody had re-checked, and eleven TODO entries describing code that had already changed. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index a9cb1f7..88884f5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -344,6 +344,63 @@ its dependencies in. Declare no-argument functions as `(void)`, not `()`. +## Fixing Defects + +**Read what the code does before deciding what it should do.** Every rule below +is here because it was broken in this repository, and every one of them was +cheap to check and expensive to assume. + +- **Check the surrounding code before designing around a hazard.** The savegame + name-width fix first grew four `AKGL_GAME_SAVE_*_NAME_WIDTH` aliases, so the + on-disk format's field widths could diverge from the object model's. They were + deleted: `akgl_game_load` compares the save's `libversion` against + `AKGL_VERSION` and refuses a mismatch **in the same function, sixteen lines + above the tables being edited**, so the divergence they defended against + cannot happen quietly. An + abstraction defending an impossible case is worse than none, because the next + reader has to work out what it is for. + +- **Abstract when the second consumer is real, not before.** A `#define` with + one use, a wrapper with one caller, a parameter only ever passed one value: + delete it and inline the thing. Those four aliases had exactly one use per + side and each expanded to exactly one existing constant. + +- **A test that has not failed has not been tested.** Break the fix, watch the + new test go red, put the fix back. Two tests in the 0.5.0 defect work passed + against the unfixed library on the first attempt: + + - the savegame roundtrip asserted the load *succeeded*, and a reader stepping + the wrong field width does not fail — it finds a run of zeros mid-entry, + stops early, and reports success with silently wrong maps; + - the `akgl_get_json_with_default` test used `TEST_EXPECT_OK`, which releases + whatever context the statement returns — and that function hands *back* the + context it was given when it does not handle the status, so the `CLEANUP` + block released it a second time and the double release corrupted the + failure instead of reporting it. + + Neither was discovered by reasoning. Both were discovered by reverting the fix + and being surprised. + +- **Pick the test case that distinguishes the two behaviours, not the obvious + one.** `!AKGL_BITMASK_HAS(mask, bit)` misparses to `!(mask & bit) == bit` + without the parentheses — but for a bit that *is* set that is `0 == bit`, + false, which is the same answer the correct parse gives. It only diverges for + a bit that is **not** set whose value is **not** 1. The obvious test passes + either way. + +- **Do not trust a comment, a TODO entry, or a CI exclusion that states a + premise.** Verify it. `tests/character.c` was excluded from two CI jobs and + from the mutation harness because it "fails deliberately". It did not: it was + exiting 0 while running one of its four tests. `TODO.md` carried eleven + entries describing code that had already changed. A premise nobody has + re-checked is where the next defect is hiding. + +- **When a measurement moves, say what you measured.** Absolute benchmark + numbers drift with the machine. The `akgl_game_update` fix is recorded as the + *gap* between two rows of the same run — 92 µs before, noise after — because + a later run read every row 15% high, including rows nothing had touched. Do + not re-baseline a whole table to record one change. + ## Rules - Add yourself (agent program name, model name and version) as a co-author on every commit message