fix: bound wizard-created Which against its item table (closes #10)

createObj stored the raw 0-f nibble as Object.Which with no bounds
check, so wizard mode -> C -> / -> f made a wand numbered 15 against a
14-entry table and panicked in fixStick; input below 'a' or '0' went
negative and panicked the same way. C's create_obj() was equally
unchecked, but its consumers were either switches (defined for any
value) or static-array reads past the end (undefined, and survivable in
practice). Since one game is now one process, the Go panic kills the
game outright and leaves the terminal in raw mode.

Reject at the two boundaries a bad Which can enter through. createObj
now refuses an out-of-range choice with a message drawn from C's own
type_name() vocabulary and adds nothing to the pack, a deliberate
divergence recorded in a comment because C had no defined behavior here
to be faithful to. Restore refuses a snapshot describing such an object
(ErrSaveCorrupt) rather than loading a game that would explode later.

Behind those, whichLimit/hasValidWhich back defensive guards at every
dispatch the issue names: the quaffHandler/readHandler/zapHandler
accessors return no handler instead of indexing (for wands that is
exactly what non-MASTER C did, matching no case and still running
o_charges--), the callIt lore lookups, identifyType, armorClass for the
a_class[] reads, initWeapon against the missing init_dam[] row for
WeaponFlame, fixStick's ws_type[] read, and inventoryName and
objectWorth, hoisted so one check each covers the whole family of
per-kind name and appraisal tables.

No in-range input changes behavior and no guard consumes a random
number: the rejection precedes every rnd() call. TestSeedCompatItemTables
stays green untouched.

New game/wizard_test.go covers the exact reproducer, a rejection sweep
over every indexed kind including both negative-input forms, an
acceptance sweep proving valid choices still build the right item, one
no-panic test per guarded family, the fixStick crash site, the
corrupt-save rejection, and a check that whichLimit still agrees with
the table sizes. Each guard was confirmed load-bearing by reverting it
and watching the test panic.

TODO.md records the step; Next Step is deliberately left alone, since
this arrived out of band via an issue.
This commit is contained in:
2026-08-09 05:09:03 +00:00
parent eb31473ef0
commit 9dbd9d11cb
12 changed files with 660 additions and 13 deletions

38
TODO.md
View File

@@ -34,6 +34,44 @@ wizard commands).
# Completed Steps
- 2026-08-09 Wizard-create bounds fix (`fix/wizard-which-bounds`, closes #10):
`createObj` stored the raw `0-f` nibble as `Object.Which` with no bounds
check, so wizard mode -> `C` -> `/` -> `f` produced a wand numbered 15 against
a 14-entry table and panicked in `fixStick`; input below `'a'` or `'0'` went
negative (`'A'` gives -22, `'!'` gives -54) and panicked the same way. C's
`create_obj()` was equally unchecked, but every C consumer was either a
`switch` (defined for any value) or a static-array read past the end
(undefined, and survivable in practice), whereas since refactor step 8 one
game is one process, so the Go panic kills the game with the terminal still in
raw mode. Fixed at the two boundaries a bad `Which` can enter through:
`createObj` now rejects an out-of-range choice with a message built from C's
own `type_name()` vocabulary and adds nothing to the pack (a deliberate,
commented divergence, since C had no defined behavior here to be faithful to),
and `Restore` refuses a snapshot describing such an object (`ErrSaveCorrupt`)
instead of loading a game that would explode later. Behind those,
`whichLimit`/`hasValidWhich` back defensive guards at every dispatch named in
the issue: the three effect tables (the new `quaffHandler`, `readHandler`, and
`zapHandler` accessors return no handler rather than indexing — for wands that
is exactly non-`MASTER` C, which matched no case and still ran `o_charges--`),
the `callIt` lore lookups, `identifyType` (whose table is shorter than the
scroll table keying it), `armorClass` for all four `a_class[]` reads,
`initWeapon` against the missing `init_dam[]` row for `WeaponFlame`,
`fixStick`'s `ws_type[]` read, and `inventoryName`, hoisted so one check
covers the scroll-title read the issue listed plus its potion-color,
ring-stone, wand-material, weapon and armor siblings. `objectWorth` got the
same hoisted guard, since the death-screen appraisal reads the identical
per-kind tables. No in-range input changes behavior and no guard consumes a
random number — the rejection precedes every `rnd()` call, verified both by an
explicit seed-unchanged test and by `TestSeedCompatItemTables` staying green
untouched. New `game/wizard_test.go`: the exact reproducer, a rejection sweep
over every indexed kind including both negative-input forms, an acceptance
sweep proving valid choices still build the right item, one no-panic test per
guarded family (wand/potion/scroll/armor/weapon), the `fixStick` crash site,
the corrupt-save rejection, and a check that `whichLimit` still agrees with
the table sizes. Each guard was confirmed load-bearing by reverting it and
watching the test panic. `Next Step` deliberately not rotated: this was
out-of-band issue work.
- 2026-08-09 Stale-docs correction (`docs-staleness`, closes #3): four claims in
`MEMORY.md`/`TODO.md`/`README.md` had gone false and were misdirecting agents
— the reviewer on PR #9 repeated one of them verbatim. Each was re-verified