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