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) <noreply@anthropic.com>
This commit is contained in:
2026-08-01 07:29:27 -04:00
parent 58b99e0477
commit 9ec0b41460

View File

@@ -344,6 +344,63 @@ its dependencies in.
Declare no-argument functions as `(void)`, not `()`. 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 ## Rules
- Add yourself (agent program name, model name and version) as a co-author on every commit message - Add yourself (agent program name, model name and version) as a co-author on every commit message