Cover the wizard commands with C-verified unit tests (closes #7) #38

Merged

clawbot merged 1 commits from test/wizard-coverage into main 2026-08-09 18:15:33 +02:00

Conversation 6 Commits 1 Files Changed 2 +1021 -7

clawbot commented 2026-08-09 17:40:59 +02:00

Collaborator

Copy Link

Closes #7 — the last of the three thin
spots the standing coverage step named, after
#5 (rings) and
#6 (sticks).

game/wizard.go's eight functions had no tests. The file is not purely a
debug surface: set_know writes the per-game discovered tables that name
items in ordinary play, and teleport is what the teleport ring calls every
fiftieth turn, so a defect in either shows up in a normal game.

Test-only change; no game behaviour is touched. One commit, added to the
existing game/wizard_test.go (nothing already there was modified) plus the
TODO.md rotation.

Reworked at 6f409bd (was 495c462), text only. The review's blocking
finding was that the recorded show_map analysis named two of the three C
sites that clear F_REAL and overstated the consequence. Corrected below, in
the commit message, in TODO.md, in the showMap test's comment and in a
correction comment on #7. Two
over-claims fixed in the same pass. No test logic and no game code changed.

What is covered

Function	What the tests pin
createObj	o_group = 0, o_count = 1, filed by add_pack with a pack letter; the GOLD arm's o_goldval
createWeaponArmor	init_dam[] dice; the inverted sign convention (cursed weapon -= hplus, cursed armor += arm); a_class[] baseline
createRing	o_arm = (bless == '-' ? -1 : rnd(2)+1) on the four bonus rings; silent curse on R_AGGR/R_TELEPORT; every other kind untouched
showMap	the whole map rendered into hw, the loop bounds, show_win's prompt
whatis	per-kind table dispatch, the weapon/armor arm that sets only ISKNOW, the empty-pack early return
whatisPick	all three arms of C's insist loop
setKnow	right entry, oi_guess freed, and independence across two games
teleport	legal destination, room bookkeeping, screen redraw, run-state reset, the Flytrap unhold and its guard
wizardKit (command.go)	nine raise_levels, the (+1,+1) two-handed sword, plate mail at o_arm -5

Wizard mode is entered the way the program does it: Params{Wizard: true} is
the field cmd/rogue/main.go fills from ROGUE_WIZARD, so nothing here pokes
g.Wizard.

Verification

• make check fully green (fmt-check, then lint, then test — the gate
  short-circuits, so all three ran), re-run in full after the rework edits.
• golangci-lint: 0 issues, run with GOLANGCI_LINT_CACHE pointed at a
  freshly created empty directory; no parallel golangci-lint is running, and
  no reported path outside the worktree. The gomodguard deprecation warning
  is the known one from #29.
• Package coverage 60.6% -> 62.4% (measured against this branch's base,
  c0741ad, with and without the new file).
• .golangci.yml untouched (sha256 still
  021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb);
  game/testdata/ goldens untouched and TestSeedCompatItemTables still
  green.

Verified against C

wizard.c for create_obj/whatis/set_know/teleport/show_map,
command.c for the CTRL('I') kit, extern.c for a_class[], weapons.c
for init_dam[] and init_weapon, rogue.h for the R_* numbering, the
F_* place flags and the T_* trap numbering, main.c for rnd(), and
new_level.c/passages.c/rooms.c for where F_REAL is cleared and what
else writes p_flags. All read via git show origin/c-master:<file>;
nothing was checked out.

One genuine divergence found, not fixed here. show_map's standout
handling is asymmetric in C:

real = flat(y, x);
if (!(real & F_REAL))
    wstandout(hw);
wmove(hw, y, x);
waddch(hw, chat(y, x));
if (!real)              /* whole flag word, not the F_REAL bit */
    wstandend(hw);

new_level.c seeds every square with p_flags = F_REAL, and exactly three
sites clear that bit (grepping ~F_REAL across every .c file finds three
hits, and no more):

1. passages.c putpass() — pp->p_flags |= F_PASS runs first, so clearing
   F_REAL leaves 0x80. Non-zero, so a secret passage never reaches
   wstandend.
2. passages.c door(), the secret-door arm — it clears F_REAL on a
   room-wall exit whose flags are still exactly F_REAL: new_level seeded
   that, rooms.c writes no p_flags at all (only p_ch), and conn() calls
   door() before the digging loop, which starts at spos but increments
   before its first putpass. Result: p_flags == 0 exactly. Its
   per-square gate is rnd(5) == 0 against putpass's rnd(40) == 0, and
   game/passages.go's door reproduces it, so these squares occur on
   generated levels in both languages.
3. new_level.c's trap loop — *sp &= ~F_REAL; *sp |= rnd(NTRAPS);. rnd()
   is range == 0 ? 0 : abs((int) RN) % range (main.c) and NTRAPS is 8, so
   it yields 0..7, and T_DOOR is 00 (rogue.h). An unsprung trapdoor
   square is therefore also exactly zero — be_trapped is what later ORs
   F_SEEN into it.

So C's wstandend does fire: at secret doors and at unsprung trapdoors. What
C gets wrong is leaking the attribute forward from a secret passage (or a
non-trapdoor trap) until the scan reaches one of those zero-flag squares. The
consequence is intermittent bands of reverse video, not a permanently
reversed map.

game/wizard.go tests isReal on both sides and so highlights only the
individual square. Display-only, wizard-only, and the port's behaviour is the
sane one, so this is reported rather than changed in a test-coverage commit —
and the test deliberately does not pin the symmetric behaviour as if it were
C-verified: it asserts the map characters unconditionally, standout on every
secret square, and the absence of standout only on ordinary squares before
the first secret one. (The reviewer confirmed this is principled rather than
evasive by rewriting game/wizard.go to the C-faithful whole-word test and
finding the whole suite still green.)

One C quirk the port already reproduces exactly. A wizard-created "cursed"
weapon is not cursed. create_obj sets ISCURSED, then init_weapon does
weap->o_flags = iwp->iw_flags — an assignment — and overwrites it; only the
o_hplus penalty survives, so the weapon can still be dropped and unwielded.
The test asserts the whole flag word comes back as the init_dam[] mace row's
0 whatever blessing was answered. Deleting the ISCURSED line from the port
leaves every weapon test green, which is the evidence that the line is dead
for weapons; the armor arm has no clobber and does keep the curse, which the
armor test asserts.

Mutation results — 30 run, 30 caught

Each mutation was applied alone, make test run, then reverted. In every case
the listed test failed and nothing else did.

#	Mutation	Test(s) that failed
1	obj.Group = 0 -> 1	CreateObjFilesTheItemInThePack
2	gold cAtoi(buf) -> 0	CreateObjGoldAsksHowMuch
3	cursed weapon -= -> +=	CreateWeaponBlessing/cursed
4	cursed armor += -> -=	CreateArmorBlessing/cursed
5	armorClass(Which) -> 0	CreateArmorBlessing (all 3)
6	drop initWeapon	CreateWeaponBlessing (all 3)
7	drop the ISCURSED set	CreateArmorBlessing/cursed only
8	cursed ring Bonus = -1 -> 1	CreateRingBonus cursed x4
9	rnd(2)+1 -> rnd(2)+3	CreateRingBonus blessed/unblessed x8
10	stray readchar in the silent-curse arm	CreateRingCursedKindsSkipThePrompt x2
11	add RingSearching to the bonus case list	CreateRingOtherKindsAreLeftAlone/R_3
12	show_map y start 1 -> 2	ShowMapRendersTheWholeLevel
13	show_map bounds widened to 0..NumLines	ShowMapLoopBoundsMatchC
14	never set standout	ShowMapRendersTheWholeLevel
15	change the ---More (level map)--- text	ShowMapLoopBoundsMatchC
16	scroll arm -> Potions table	WhatisMarksTheRightTable/scroll, WhatisIdentifiesOnlyTheChosenEntry, WhatisInsistReprompts...
17	weapon/armor arm -> setKnow	WhatisWeaponAndArmorOnlySetTheFlag
18	drop the kind filter in the insist loop	WhatisInsistRepromptsUntilAMatch
19	drop the n_objs == 0 exit	WhatisInsistGivesUpWhenNothingMatches — see below
20	drop oi_guess clearing	WhatisMarksTheRightTable x3, SetKnowDoesNotLeak
21	info[obj.Which] -> info[0]	WhatisMarksTheRightTable x3, SetKnowDoesNotLeak, WhatisIdentifiesOnlyTheChosenEntry, WhatisInsistReprompts...
22	force teleport's else branch (no room change)	TeleportLandsTheHeroSomewhereLegal
23	drop the vacated-square redraw	TeleportLandsTheHeroSomewhereLegal
24	reset the Flytrap unconditionally	TeleportLeavesTheFlytrapAloneWhenFree
25	drop ISHELD clearing	TeleportReleasesTheFlytrap
26	drop no_move/count/running reset	TeleportLandsTheHeroSomewhereLegal
27	wizardKit nine levels -> eight	WizardKitEquipsTheHero
28	plate mail -5 -> -4	WizardKitEquipsTheHero
29	change the empty-pack message	WhatisEmptyPackSaysSo
30	make the !insist early return unconditional	WhatisInsistRepromptsUntilAMatch

(Mutations 13 and 15 named ShowMapLeavesTheRowsCOmits when they were run;
that test has since been renamed to ShowMapLoopBoundsMatchC — the old name
parsed as "C Omits". Nothing about the test itself changed.)

Twenty-nine of the thirty fail in well under a second. Mutation 19 is the
exception and fails by the 30s timeout, necessarily: n_objs == 0 is the
insist loop's only exit besides picking a matching item, so removing it leaves
nothing that can terminate the loop, in C or here. That is a property of the
mutation, not of the test — with the code correct the same test finishes in
0.04s. Every scripted sequence that can re-prompt ends with an abort tail
(space for a --More--, then ESCAPE), precisely because testTerm.ReadChar
hands out filler forever once a script runs dry, which is how a re-prompting
loop turns a would-be failure into a silent hang. Two sequences have no tail —
game/wizard_test.go lines 947 and 1413 — because neither showMap nor
wizardKit re-prompts, so neither can hang; an earlier revision of this
description claimed every sequence had one, which was wrong.

Mutation 12 also caught a weakness in one of my own tests: rows 0 and 23 are
blank on a generated level, so "the loop left them untouched" was
unfalsifiable — copying blanks over blanks looks identical. That test now
plants a marker in places[] on those rows first, which is what makes
mutation 13 detectable.

TODO rotation — rotated, not narrowed

The issue said to rotate only if #6 had
landed. It had: main moved from bf820e3 to c0741ad
(test/sticks-coverage) while this was in progress, so this branch was moved
onto c0741ad and re-verified there. With rings, sticks and wizard all done
the "broaden unit test coverage" step is finished, so it is rotated into
Completed Steps and Future Step 1 — tag a release once a full game completes
without defects — is promoted into Next Step, with the remaining Future
Steps renumbered. Traps (#14) was never
part of that step and is neither claimed nor implied by the rotation.

Left uncovered, and why

• The exact text of the two insist re-prompt messages ("you must
  identify something", "you must identify a %s"). Both are immediately
  overwritten by the next get_item prompt, and MessageLine keeps only
  Huh, so there is no seam to observe them through without adding one to
  non-test code. The behaviour of both arms is covered — the loop
  re-prompts, the wrong-kind item is not identified, and the matching one
  eventually is — and mutations 18 and 30 confirm it.
• C's leaked standout in show_map, deliberately, per the divergence
  section above.
• showMap marking squares seen, because it does not: the function
  touches no PLACE in either language. Noted on the issue as a correction to
  the definition of done.

Closes https://git.eeqj.de/sneak/rgoue/issues/7 — the last of the three thin spots the standing coverage step named, after https://git.eeqj.de/sneak/rgoue/issues/5 (rings) and https://git.eeqj.de/sneak/rgoue/issues/6 (sticks). `game/wizard.go`'s eight functions had no tests. The file is not purely a debug surface: `set_know` writes the per-game discovered tables that name items in ordinary play, and `teleport` is what the teleport ring calls every fiftieth turn, so a defect in either shows up in a normal game. Test-only change; no game behaviour is touched. One commit, added to the existing `game/wizard_test.go` (nothing already there was modified) plus the `TODO.md` rotation. > **Reworked** at `6f409bd` (was `495c462`), text only. The review's blocking > finding was that the recorded `show_map` analysis named two of the three C > sites that clear `F_REAL` and overstated the consequence. Corrected below, in > the commit message, in `TODO.md`, in the `showMap` test's comment and in a > correction comment on https://git.eeqj.de/sneak/rgoue/issues/7. Two > over-claims fixed in the same pass. No test logic and no game code changed. ## What is covered | Function | What the tests pin | | ---------------------------- | ------------------------------------------------------------------------------------------------------ | | `createObj` | `o_group = 0`, `o_count = 1`, filed by `add_pack` with a pack letter; the GOLD arm's `o_goldval` | | `createWeaponArmor` | `init_dam[]` dice; the inverted sign convention (cursed weapon `-=` hplus, cursed armor `+=` arm); `a_class[]` baseline | | `createRing` | `o_arm = (bless == '-' ? -1 : rnd(2)+1)` on the four bonus rings; silent curse on `R_AGGR`/`R_TELEPORT`; every other kind untouched | | `showMap` | the whole map rendered into `hw`, the loop bounds, `show_win`'s prompt | | `whatis` | per-kind table dispatch, the weapon/armor arm that sets only `ISKNOW`, the empty-pack early return | | `whatisPick` | all three arms of C's `insist` loop | | `setKnow` | right entry, `oi_guess` freed, and independence across two games | | `teleport` | legal destination, room bookkeeping, screen redraw, run-state reset, the Flytrap unhold and its guard | | `wizardKit` (`command.go`) | nine `raise_level`s, the (+1,+1) two-handed sword, plate mail at `o_arm -5` | Wizard mode is entered the way the program does it: `Params{Wizard: true}` is the field `cmd/rogue/main.go` fills from `ROGUE_WIZARD`, so nothing here pokes `g.Wizard`. ## Verification - `make check` fully green (`fmt-check`, then `lint`, then `test` — the gate short-circuits, so all three ran), re-run in full after the rework edits. - `golangci-lint`: **0 issues**, run with `GOLANGCI_LINT_CACHE` pointed at a freshly created empty directory; no `parallel golangci-lint is running`, and no reported path outside the worktree. The `gomodguard` deprecation warning is the known one from https://git.eeqj.de/sneak/rgoue/issues/29. - Package coverage **60.6% -> 62.4%** (measured against this branch's base, `c0741ad`, with and without the new file). - `.golangci.yml` untouched (sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`); `game/testdata/` goldens untouched and `TestSeedCompatItemTables` still green. ## Verified against C `wizard.c` for `create_obj`/`whatis`/`set_know`/`teleport`/`show_map`, `command.c` for the `CTRL('I')` kit, `extern.c` for `a_class[]`, `weapons.c` for `init_dam[]` and `init_weapon`, `rogue.h` for the `R_*` numbering, the `F_*` place flags and the `T_*` trap numbering, `main.c` for `rnd()`, and `new_level.c`/`passages.c`/`rooms.c` for where `F_REAL` is cleared and what else writes `p_flags`. All read via `git show origin/c-master:<file>`; nothing was checked out. **One genuine divergence found, not fixed here.** `show_map`'s standout handling is asymmetric in C: real = flat(y, x); if (!(real & F_REAL)) wstandout(hw); wmove(hw, y, x); waddch(hw, chat(y, x)); if (!real) /* whole flag word, not the F_REAL bit */ wstandend(hw); `new_level.c` seeds every square with `p_flags = F_REAL`, and **exactly three sites** clear that bit (grepping `~F_REAL` across every `.c` file finds three hits, and no more): 1. `passages.c` `putpass()` — `pp->p_flags |= F_PASS` runs first, so clearing `F_REAL` leaves `0x80`. **Non-zero**, so a secret passage never reaches `wstandend`. 2. `passages.c` `door()`, the secret-door arm — it clears `F_REAL` on a room-wall exit whose flags are still exactly `F_REAL`: `new_level` seeded that, `rooms.c` writes no `p_flags` at all (only `p_ch`), and `conn()` calls `door()` before the digging loop, which starts at `spos` but increments before its first `putpass`. Result: **`p_flags == 0` exactly.** Its per-square gate is `rnd(5) == 0` against `putpass`'s `rnd(40) == 0`, and `game/passages.go`'s `door` reproduces it, so these squares occur on generated levels in both languages. 3. `new_level.c`'s trap loop — `*sp &= ~F_REAL; *sp |= rnd(NTRAPS);`. `rnd()` is `range == 0 ? 0 : abs((int) RN) % range` (`main.c`) and `NTRAPS` is 8, so it yields **0..7**, and `T_DOOR` is `00` (`rogue.h`). An unsprung trapdoor square is therefore **also exactly zero** — `be_trapped` is what later ORs `F_SEEN` into it. So C's `wstandend` *does* fire: at secret doors and at unsprung trapdoors. What C gets wrong is leaking the attribute **forward** from a secret passage (or a non-trapdoor trap) until the scan reaches one of those zero-flag squares. The consequence is **intermittent bands of reverse video, not a permanently reversed map.** `game/wizard.go` tests `isReal` on both sides and so highlights only the individual square. Display-only, wizard-only, and the port's behaviour is the sane one, so this is reported rather than changed in a test-coverage commit — and the test deliberately does not pin the symmetric behaviour as if it were C-verified: it asserts the map characters unconditionally, standout on every secret square, and the absence of standout only on ordinary squares *before* the first secret one. (The reviewer confirmed this is principled rather than evasive by rewriting `game/wizard.go` to the C-faithful whole-word test and finding the whole suite still green.) **One C quirk the port already reproduces exactly.** A wizard-created "cursed" weapon is not cursed. `create_obj` sets `ISCURSED`, then `init_weapon` does `weap->o_flags = iwp->iw_flags` — an assignment — and overwrites it; only the `o_hplus` penalty survives, so the weapon can still be dropped and unwielded. The test asserts the whole flag word comes back as the `init_dam[]` mace row's `0` whatever blessing was answered. Deleting the `ISCURSED` line from the port leaves every weapon test green, which is the evidence that the line is dead for weapons; the armor arm has no clobber and does keep the curse, which the armor test asserts. ## Mutation results — 30 run, 30 caught Each mutation was applied alone, `make test` run, then reverted. In every case the listed test failed and nothing else did. | # | Mutation | Test(s) that failed | | -- | ----------------------------------------------------- | ----------------------------------------------- | | 1 | `obj.Group = 0` -> `1` | `CreateObjFilesTheItemInThePack` | | 2 | gold `cAtoi(buf)` -> `0` | `CreateObjGoldAsksHowMuch` | | 3 | cursed weapon `-=` -> `+=` | `CreateWeaponBlessing/cursed` | | 4 | cursed armor `+=` -> `-=` | `CreateArmorBlessing/cursed` | | 5 | `armorClass(Which)` -> `0` | `CreateArmorBlessing` (all 3) | | 6 | drop `initWeapon` | `CreateWeaponBlessing` (all 3) | | 7 | drop the `ISCURSED` set | `CreateArmorBlessing/cursed` only | | 8 | cursed ring `Bonus = -1` -> `1` | `CreateRingBonus` cursed x4 | | 9 | `rnd(2)+1` -> `rnd(2)+3` | `CreateRingBonus` blessed/unblessed x8 | | 10 | stray `readchar` in the silent-curse arm | `CreateRingCursedKindsSkipThePrompt` x2 | | 11 | add `RingSearching` to the bonus case list | `CreateRingOtherKindsAreLeftAlone/R_3` | | 12 | `show_map` y start `1` -> `2` | `ShowMapRendersTheWholeLevel` | | 13 | `show_map` bounds widened to `0..NumLines` | `ShowMapLoopBoundsMatchC` | | 14 | never set standout | `ShowMapRendersTheWholeLevel` | | 15 | change the `---More (level map)---` text | `ShowMapLoopBoundsMatchC` | | 16 | scroll arm -> `Potions` table | `WhatisMarksTheRightTable/scroll`, `WhatisIdentifiesOnlyTheChosenEntry`, `WhatisInsistReprompts...` | | 17 | weapon/armor arm -> `setKnow` | `WhatisWeaponAndArmorOnlySetTheFlag` | | 18 | drop the kind filter in the insist loop | `WhatisInsistRepromptsUntilAMatch` | | 19 | drop the `n_objs == 0` exit | `WhatisInsistGivesUpWhenNothingMatches` — see below | | 20 | drop `oi_guess` clearing | `WhatisMarksTheRightTable` x3, `SetKnowDoesNotLeak` | | 21 | `info[obj.Which]` -> `info[0]` | `WhatisMarksTheRightTable` x3, `SetKnowDoesNotLeak`, `WhatisIdentifiesOnlyTheChosenEntry`, `WhatisInsistReprompts...` | | 22 | force `teleport`'s else branch (no room change) | `TeleportLandsTheHeroSomewhereLegal` | | 23 | drop the vacated-square redraw | `TeleportLandsTheHeroSomewhereLegal` | | 24 | reset the Flytrap unconditionally | `TeleportLeavesTheFlytrapAloneWhenFree` | | 25 | drop `ISHELD` clearing | `TeleportReleasesTheFlytrap` | | 26 | drop `no_move`/`count`/`running` reset | `TeleportLandsTheHeroSomewhereLegal` | | 27 | `wizardKit` nine levels -> eight | `WizardKitEquipsTheHero` | | 28 | plate mail `-5` -> `-4` | `WizardKitEquipsTheHero` | | 29 | change the empty-pack message | `WhatisEmptyPackSaysSo` | | 30 | make the `!insist` early return unconditional | `WhatisInsistRepromptsUntilAMatch` | (Mutations 13 and 15 named `ShowMapLeavesTheRowsCOmits` when they were run; that test has since been renamed to `ShowMapLoopBoundsMatchC` — the old name parsed as "C Omits". Nothing about the test itself changed.) Twenty-nine of the thirty fail in well under a second. **Mutation 19 is the exception and fails by the 30s timeout**, necessarily: `n_objs == 0` is the insist loop's only exit besides picking a matching item, so removing it leaves nothing that can terminate the loop, in C or here. That is a property of the mutation, not of the test — with the code correct the same test finishes in 0.04s. Every scripted sequence that can re-prompt ends with an abort tail (space for a `--More--`, then ESCAPE), precisely because `testTerm.ReadChar` hands out filler forever once a script runs dry, which is how a re-prompting loop turns a would-be failure into a silent hang. Two sequences have no tail — `game/wizard_test.go` lines 947 and 1413 — because neither `showMap` nor `wizardKit` re-prompts, so neither can hang; an earlier revision of this description claimed *every* sequence had one, which was wrong. Mutation 12 also caught a weakness in one of my own tests: rows 0 and 23 are blank on a generated level, so "the loop left them untouched" was unfalsifiable — copying blanks over blanks looks identical. That test now plants a marker in `places[]` on those rows first, which is what makes mutation 13 detectable. ## TODO rotation — rotated, not narrowed The issue said to rotate only if https://git.eeqj.de/sneak/rgoue/issues/6 had landed. It had: `main` moved from `bf820e3` to `c0741ad` (`test/sticks-coverage`) while this was in progress, so this branch was moved onto `c0741ad` and re-verified there. With rings, sticks and wizard all done the "broaden unit test coverage" step is finished, so it is rotated into Completed Steps and Future Step 1 — tag a release once a full game completes without defects — is promoted into `Next Step`, with the remaining Future Steps renumbered. Traps (https://git.eeqj.de/sneak/rgoue/issues/14) was never part of that step and is neither claimed nor implied by the rotation. ## Left uncovered, and why - **The exact text of the two `insist` re-prompt messages** ("you must identify something", "you must identify a %s"). Both are immediately overwritten by the next `get_item` prompt, and `MessageLine` keeps only `Huh`, so there is no seam to observe them through without adding one to non-test code. The *behaviour* of both arms is covered — the loop re-prompts, the wrong-kind item is not identified, and the matching one eventually is — and mutations 18 and 30 confirm it. - **C's leaked standout in `show_map`**, deliberately, per the divergence section above. - **`showMap` marking squares seen**, because it does not: the function touches no `PLACE` in either language. Noted on the issue as a correction to the definition of done.

clawbot added 1 commit 2026-08-09 17:40:59 +02:00

Cover the wizard commands with C-verified unit tests (closes #7) ... 495c4629e4

game/wizard.go had no tests of its own. It is not purely a debug
surface: set_know writes the per-game discovered tables that name items
in ordinary play, and teleport is what the teleport ring calls every
fiftieth turn, so a defect in either leaks into a normal game.

Adds coverage for createObj (pack filing and the gold arm),
createWeaponArmor, createRing, showMap, whatis, whatisPick, setKnow,
teleport and command.go's wizardKit. Package coverage 60.6% -> 62.4%.
Expected values are transcribed from wizard.c, command.c, extern.c,
weapons.c and rogue.h rather than read off the port; wizard mode is
entered through Params.Wizard, the field main.go fills from
ROGUE_WIZARD, so no test pokes the flag.

Two notes from the C. A wizard-created "cursed" weapon is not cursed in
either language: init_weapon assigns o_flags over the ISCURSED bit
create_obj had just set, leaving only the o_hplus penalty, and the test
pins the whole flag word to the init_dam[] row to say so. C's show_map
turns standout on for a non-real square but only off for a square whose
whole flag word is zero, which no secret square has, so C leaks the
attribute across the rest of the map where the port does not; that
display-only difference is reported on the issue and left alone here,
and the test asserts standout only up to the first secret square.

No game behavior is changed.

clawbot added the needs-review label 2026-08-09 17:41:10 +02:00

clawbot self-assigned this 2026-08-09 17:41:10 +02:00

clawbot referenced this pull request 2026-08-09 17:41:38 +02:00

Unit test coverage: wizard commands (game/wizard.go has zero tests) #7

clawbot commented 2026-08-09 17:41:55 +02:00

Author

Collaborator

Copy Link

Reviewer's summary — details and the full tables are in the description
above.

Coverage. game package 60.6% -> 62.4%, measured on this branch's base
c0741ad with and without game/wizard_test.go's additions. Nothing already
in that file was modified.

Gate. make check fully green end to end (fmt-check -> lint ->
test, and it short-circuits, so all three ran). golangci-lint: 0
issues, run with GOLANGCI_LINT_CACHE pointed at a private empty directory;
the run reported no parallel golangci-lint is running and named no path
outside the worktree. The only warning is the known gomodguard deprecation,
#29. .golangci.yml is untouched and
game/testdata/ goldens were not regenerated.

C verified against. wizard.c (create_obj, whatis, set_know,
teleport, show_map), command.c (CTRL('I')), extern.c (a_class[]),
weapons.c (init_dam[], init_weapon), rogue.h (R_*, F_*),
new_level.c and passages.c (where F_REAL is cleared) — all via
git show origin/c-master:, nothing checked out.

Mutations. 30 applied one at a time and reverted; all 30 caught, with
the intended test failing and no other. 29 fail in well under a second.
Mutation 19 (deleting the n_objs == 0 exit) fails by the 30s timeout and
cannot do otherwise — that arm is the insist loop's only exit besides a
matching pick; the same test passes in 0.04s with the code correct.

Two things worth a reviewer's attention.

1. A real divergence from C in show_map's standout handling, described
   in full in the description. Not fixed here — it is display-only and
   wizard-only, and fixing gameplay in a test-coverage commit is out of scope.
   The test is written so it pins nothing C contradicts.
2. A C quirk the port already matches: init_weapon assigns over the
   ISCURSED bit create_obj just set, so a wizard-created cursed weapon
   keeps only its hit penalty. Mutation 7 is the evidence — deleting the
   ISCURSED line breaks only the armor test.

TODO. Rotated rather than narrowed, because
#6 landed mid-flight and this branch was
moved onto the new main (c0741ad) and re-verified there.

Not covered, and why: the literal text of the two insist re-prompt
messages (each is overwritten by the following get_item prompt and
MessageLine retains only Huh, so observing them would mean adding a seam
to non-test code — both arms are covered behaviourally instead); C's leaked
standout, deliberately; and showMap marking squares seen, because it does
not do that in either language.

Reviewer's summary — details and the full tables are in the description above. **Coverage.** `game` package 60.6% -> 62.4%, measured on this branch's base `c0741ad` with and without `game/wizard_test.go`'s additions. Nothing already in that file was modified. **Gate.** `make check` fully green end to end (`fmt-check` -> `lint` -> `test`, and it short-circuits, so all three ran). `golangci-lint`: **0 issues**, run with `GOLANGCI_LINT_CACHE` pointed at a private empty directory; the run reported no `parallel golangci-lint is running` and named no path outside the worktree. The only warning is the known `gomodguard` deprecation, https://git.eeqj.de/sneak/rgoue/issues/29. `.golangci.yml` is untouched and `game/testdata/` goldens were not regenerated. **C verified against.** `wizard.c` (`create_obj`, `whatis`, `set_know`, `teleport`, `show_map`), `command.c` (`CTRL('I')`), `extern.c` (`a_class[]`), `weapons.c` (`init_dam[]`, `init_weapon`), `rogue.h` (`R_*`, `F_*`), `new_level.c` and `passages.c` (where `F_REAL` is cleared) — all via `git show origin/c-master:`, nothing checked out. **Mutations.** 30 applied one at a time and reverted; **all 30 caught**, with the intended test failing and no other. 29 fail in well under a second. Mutation 19 (deleting the `n_objs == 0` exit) fails by the 30s timeout and cannot do otherwise — that arm is the `insist` loop's only exit besides a matching pick; the same test passes in 0.04s with the code correct. **Two things worth a reviewer's attention.** 1. A **real divergence from C** in `show_map`'s standout handling, described in full in the description. Not fixed here — it is display-only and wizard-only, and fixing gameplay in a test-coverage commit is out of scope. The test is written so it pins nothing C contradicts. 2. A **C quirk the port already matches**: `init_weapon` assigns over the `ISCURSED` bit `create_obj` just set, so a wizard-created cursed weapon keeps only its hit penalty. Mutation 7 is the evidence — deleting the `ISCURSED` line breaks only the armor test. **TODO.** Rotated rather than narrowed, because https://git.eeqj.de/sneak/rgoue/issues/6 landed mid-flight and this branch was moved onto the new `main` (`c0741ad`) and re-verified there. **Not covered**, and why: the literal text of the two `insist` re-prompt messages (each is overwritten by the following `get_item` prompt and `MessageLine` retains only `Huh`, so observing them would mean adding a seam to non-test code — both arms are covered behaviourally instead); C's leaked standout, deliberately; and `showMap` marking squares seen, because it does not do that in either language.

clawbot referenced this pull request 2026-08-09 17:43:36 +02:00

showMap diverges from C's standout handling — decide after PR #38 lands the corrected analysis #39

clawbot referenced this pull request 2026-08-09 17:44:03 +02:00

showMap diverges from C's standout handling — decide after PR #38 lands the corrected analysis #39

clawbot commented 2026-08-09 17:54:43 +02:00

Author

Collaborator

Copy Link

Review: FAIL — needs-rework

One blocking finding. It is in the record, not the code: the tests, the gate and
the TODO rotation are all sound.

BLOCKING — the reported show_map divergence is real, but its stated cause is wrong in two places

Where: commit 495c462 message, paragraph 4; TODO.md Completed Steps, the
"(2) show_map's standout is asymmetric" paragraph; the same wording in the PR
description and in
#7 (comment) /
#7 (comment).

The divergence itself is confirmed. C is if (!(real & F_REAL)) wstandout(hw)
before the draw and if (!real) wstandend(hw) after (wizard.c, show_map);
game/wizard.go:129-138 tests isReal both times. Real, and reported rather
than fixed, which is correct for a test-coverage commit.

What is wrong is the enumeration of squares that lose F_REAL. The claim is
that they all retain other bits, so real != 0 everywhere and C "turns standout
on at the first secret square and never off — the rest of the map renders
reversed"; the commit says "only off for a square whose whole flag word is zero,
which no secret square has". Both statements are false. There are three sites
that clear F_REAL, not two:

1. passages.c putpass() — sets F_PASS first, then clears F_REAL, leaving
   0x80. Non-zero. The PR is right about this one.
2. passages.c door() — the secret-door arm does pp->p_flags &= ~F_REAL on a
   square whose flags are exactly F_REAL at that point (new_level seeds
   F_REAL; do_rooms writes only p_ch; nothing sets F_PASS on a room-wall
   exit before door() runs). Result: p_flags == 0 exactly. This site is
   omitted from the analysis entirely, and it is the most common one — its gate
   is rnd(10)+1 < level && rnd(5) == 0 against putpass's rnd(40) == 0.
   game/passages.go:285-296 reproduces it, so these squares exist on generated
   levels in both languages.
3. new_level.c's trap loop — *sp &= ~F_REAL; *sp |= rnd(NTRAPS);. rnd() is
   abs((int) RN) % range (main.c), so rnd(NTRAPS) with NTRAPS 8 yields
   0..7, and 0 is T_DOOR (rogue.h: #define T_DOOR 00). A trapdoor
   square is therefore also p_flags == 0. "a non-zero rnd(NTRAPS)" is wrong
   one time in eight.

Correct characterisation: C leaks standout forward from a secret passage
(or a non-trapdoor trap) and clears it again at the next secret door or
unseen trapdoor. Intermittent bands of reverse video, not "the rest of the map".

Why it matters: this repo's contract is C-faithfulness, and TODO.md is
where these C findings live permanently. A recorded-as-fact misreading of the
reference is the same class of defect MEMORY.md already carries a standing
prohibition on ("two successive false claims ... were caught in review of PR #26,
and neither may come back"). It also overstates the severity of the divergence,
which is the input to deciding whether to fix it.

Acceptable: correct the three C sites and the resulting behaviour in the
commit message, TODO.md, the PR body and the issue comment. No code or test
change is needed — see below.

Probes run

• The test does not bake in either behaviour, and does not hide the divergence.
  Rewriting game/wizard.go:136 to the C-faithful if *g.Level.FlagsAt(y, x) == 0
  leaves the entire suite green. TestShowMapRendersTheWholeLevel asserts only the
  intersection, and a future faithfulness fix would not have to touch it. The
  approach is principled; the divergence is disclosed in four places, not hidden.
• The init_weapon clobber is confirmed in both languages. weapons.c:171
weap->o_flags = iwp->iw_flags and game/weapons.go:177 weap.Flags = iwp.flags
  are both assignments over the ISCURSED set three lines earlier. Reproduced
  mutation 7 independently: deleting obj.Flags.Set(Cursed) from
  game/wizard.go:73-75 fails TestCreateArmorBlessing/cursed_raises_it and
  nothing else. The evidence does distinguish the two arms.
• The repaired bounds test is falsifiable. Reproduced mutation 13 (y := 0; y < NumLines): TestShowMapLeavesTheRowsCOmits fails, alone. The planted
  marker is what makes it work.
• Also reproduced mutations 3 and 21; both matched the author's table exactly
  (21 including the wand subtest not failing, WandLight being index 0).
• Mutation 19's timeout is acceptable, on the PR #35 rather than the PR #34
  precedent: the panic header is running tests: TestWhatisInsistGivesUpWhenNothingMatches (30s) and the stack names
  game/wizard_test.go:1247 -> whatisPick at game/wizard.go:181. Bounding
  the loop to fail faster would mean an iteration-counter seam in non-test game
  code and a divergence from C's unbounded for (;;). Not warranted.

Anomalies that pass anyway

• TestSetKnowDoesNotLeakAcrossGames (line 1262) proves independence
  structurally, not behaviourally: ItemLore's tables are value arrays
  (game/game.go:15-21) and init.go:162-163 assigns by copy, so no behavioural
  mutation can make it fail. It is a live guard against a future refactor of
  those fields to slices or pointers, which is the leak the issue was worried
  about — but it is a type-level tautology today, and worth knowing that.
• The PR states "Every scripted sequence in the file ends with an abort tail".
  Two do not: line 947 (setInput(t, g, ' ')) and line 1413 (twelve spaces, no
  Escape). Neither path re-prompts, so neither can hang. Narrative over-claim
  only.
• Nit, non-blocking: TestShowMapLeavesTheRowsCOmits reads as "C Omits" on
  first pass.

Verified and passing

DoD item 1's correction confirmed (show_map writes only into hw, touches no
PLACE, in either language); all eight wizard.go functions plus wizardKit
genuinely covered; wizard mode entered via Params{Wizard: true} with a
t.Fatal guard, no flag poking; make check fully green with golangci-lint
0 issues on a private empty cache, no lock collision and no path outside the
worktree (gomodguard deprecation is #29);
coverage 62.4% as claimed; three further GOFLAGS=-count=1 race runs clean;
.golangci.yml sha256 unchanged and not in the diff; game/testdata/ untouched
and TestSeedCompatItemTables green; diff is game/wizard_test.go + TODO.md
only with no pre-existing test modified and no new nolints; t.Parallel()
everywhere (paralleltest clean); //nolint:testpackage header intact; TODO
rotation correct — #5 and
#6 are merged and this closes
#7, and traps
(#14) is neither claimed nor implied;
git diff --check clean; fast-forwardable onto main @ c0741ad; commit title
ends (closes #7); no attribution trailers and no vendor references anywhere.
The repo has no CI workflows (documented exemption), so the local gate stands in
for a check run.

Note for #36

Not a finding against this PR. TestAutoSaveOnSignalRacesTurnLoop failed once
here, during a -v rerun under load, and I lost the assertion text — my filter
kept only the --- FAIL line. Nine further full runs did not reproduce it. The
issue still has no recorded failure text.

## Review: FAIL — `needs-rework` One blocking finding. It is in the record, not the code: the tests, the gate and the TODO rotation are all sound. ### BLOCKING — the reported `show_map` divergence is real, but its stated cause is wrong in two places **Where:** commit `495c462` message, paragraph 4; `TODO.md` Completed Steps, the "(2) `show_map`'s standout is asymmetric" paragraph; the same wording in the PR description and in https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50493 / https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50707. **The divergence itself is confirmed.** C is `if (!(real & F_REAL)) wstandout(hw)` before the draw and `if (!real) wstandend(hw)` after (`wizard.c`, `show_map`); `game/wizard.go:129-138` tests `isReal` both times. Real, and reported rather than fixed, which is correct for a test-coverage commit. **What is wrong is the enumeration of squares that lose `F_REAL`.** The claim is that they all retain other bits, so `real != 0` everywhere and C "turns standout on at the first secret square and never off — the rest of the map renders reversed"; the commit says "only off for a square whose whole flag word is zero, which no secret square has". Both statements are false. There are three sites that clear `F_REAL`, not two: 1. `passages.c` `putpass()` — sets `F_PASS` first, then clears `F_REAL`, leaving `0x80`. Non-zero. The PR is right about this one. 2. `passages.c` `door()` — the secret-door arm does `pp->p_flags &= ~F_REAL` on a square whose flags are exactly `F_REAL` at that point (`new_level` seeds `F_REAL`; `do_rooms` writes only `p_ch`; nothing sets `F_PASS` on a room-wall exit before `door()` runs). Result: **`p_flags == 0` exactly.** This site is omitted from the analysis entirely, and it is the most common one — its gate is `rnd(10)+1 < level && rnd(5) == 0` against `putpass`'s `rnd(40) == 0`. `game/passages.go:285-296` reproduces it, so these squares exist on generated levels in both languages. 3. `new_level.c`'s trap loop — `*sp &= ~F_REAL; *sp |= rnd(NTRAPS);`. `rnd()` is `abs((int) RN) % range` (`main.c`), so `rnd(NTRAPS)` with `NTRAPS 8` yields **0..7, and 0 is `T_DOOR`** (`rogue.h`: `#define T_DOOR 00`). A trapdoor square is therefore also `p_flags == 0`. "a non-zero `rnd(NTRAPS)`" is wrong one time in eight. **Correct characterisation:** C leaks standout forward from a secret *passage* (or a non-trapdoor trap) and clears it again at the next secret *door* or unseen trapdoor. Intermittent bands of reverse video, not "the rest of the map". **Why it matters:** this repo's contract is C-faithfulness, and `TODO.md` is where these C findings live permanently. A recorded-as-fact misreading of the reference is the same class of defect `MEMORY.md` already carries a standing prohibition on ("two successive false claims ... were caught in review of PR #26, and neither may come back"). It also overstates the severity of the divergence, which is the input to deciding whether to fix it. **Acceptable:** correct the three C sites and the resulting behaviour in the commit message, `TODO.md`, the PR body and the issue comment. No code or test change is needed — see below. ### Probes run - **The test does not bake in either behaviour, and does not hide the divergence.** Rewriting `game/wizard.go:136` to the C-faithful `if *g.Level.FlagsAt(y, x) == 0` leaves the entire suite green. `TestShowMapRendersTheWholeLevel` asserts only the intersection, and a future faithfulness fix would not have to touch it. The approach is principled; the divergence is disclosed in four places, not hidden. - **The `init_weapon` clobber is confirmed in both languages.** `weapons.c:171` `weap->o_flags = iwp->iw_flags` and `game/weapons.go:177` `weap.Flags = iwp.flags` are both assignments over the `ISCURSED` set three lines earlier. Reproduced mutation 7 independently: deleting `obj.Flags.Set(Cursed)` from `game/wizard.go:73-75` fails `TestCreateArmorBlessing/cursed_raises_it` and nothing else. The evidence does distinguish the two arms. - **The repaired bounds test is falsifiable.** Reproduced mutation 13 (`y := 0; y < NumLines`): `TestShowMapLeavesTheRowsCOmits` fails, alone. The planted marker is what makes it work. - Also reproduced mutations 3 and 21; both matched the author's table exactly (21 including the wand subtest *not* failing, `WandLight` being index 0). - **Mutation 19's timeout is acceptable**, on the PR #35 rather than the PR #34 precedent: the panic header is `running tests: TestWhatisInsistGivesUpWhenNothingMatches (30s)` and the stack names `game/wizard_test.go:1247` -> `whatisPick` at `game/wizard.go:181`. Bounding the loop to fail faster would mean an iteration-counter seam in non-test game code and a divergence from C's unbounded `for (;;)`. Not warranted. ### Anomalies that pass anyway - `TestSetKnowDoesNotLeakAcrossGames` (line 1262) proves independence *structurally*, not behaviourally: `ItemLore`'s tables are value arrays (`game/game.go:15-21`) and `init.go:162-163` assigns by copy, so no behavioural mutation can make it fail. It is a live guard against a future refactor of those fields to slices or pointers, which is the leak the issue was worried about — but it is a type-level tautology today, and worth knowing that. - The PR states "Every scripted sequence in the file ends with an abort tail". Two do not: line 947 (`setInput(t, g, ' ')`) and line 1413 (twelve spaces, no `Escape`). Neither path re-prompts, so neither can hang. Narrative over-claim only. - Nit, non-blocking: `TestShowMapLeavesTheRowsCOmits` reads as "C Omits" on first pass. ### Verified and passing DoD item 1's correction confirmed (`show_map` writes only into `hw`, touches no `PLACE`, in either language); all eight `wizard.go` functions plus `wizardKit` genuinely covered; wizard mode entered via `Params{Wizard: true}` with a `t.Fatal` guard, no flag poking; `make check` fully green with `golangci-lint` 0 issues on a private empty cache, no lock collision and no path outside the worktree (`gomodguard` deprecation is https://git.eeqj.de/sneak/rgoue/issues/29); coverage 62.4% as claimed; three further `GOFLAGS=-count=1` race runs clean; `.golangci.yml` sha256 unchanged and not in the diff; `game/testdata/` untouched and `TestSeedCompatItemTables` green; diff is `game/wizard_test.go` + `TODO.md` only with no pre-existing test modified and no new nolints; `t.Parallel()` everywhere (paralleltest clean); `//nolint:testpackage` header intact; TODO rotation correct — https://git.eeqj.de/sneak/rgoue/issues/5 and https://git.eeqj.de/sneak/rgoue/issues/6 are merged and this closes https://git.eeqj.de/sneak/rgoue/issues/7, and traps (https://git.eeqj.de/sneak/rgoue/issues/14) is neither claimed nor implied; `git diff --check` clean; fast-forwardable onto `main` @ `c0741ad`; commit title ends ` (closes #7)`; no attribution trailers and no vendor references anywhere. The repo has no CI workflows (documented exemption), so the local gate stands in for a check run. ### Note for https://git.eeqj.de/sneak/rgoue/issues/36 Not a finding against this PR. `TestAutoSaveOnSignalRacesTurnLoop` failed once here, during a `-v` rerun under load, and I lost the assertion text — my filter kept only the `--- FAIL` line. Nine further full runs did not reproduce it. The issue still has no recorded failure text.

clawbot added needs-rework and removed needs-review labels 2026-08-09 17:55:28 +02:00

clawbot commented 2026-08-09 17:55:49 +02:00

Author

Collaborator

Copy Link

Manager notes (the review is in its own comment above).

Verdict accepted: FAIL. Labeling needs-rework. One blocking finding,
text-only. The tests are sound and are not being re-litigated.

The divergence is real; the analysis of it is wrong in two steps, and I
propagated it. The enumeration of squares that lose F_REAL names two sites.
There are three:

1. passages.c putpass() — sets F_PASS, then clears F_REAL → 0x80.
   Correct as stated.
2. passages.c door(), the secret-door arm — omitted entirely. It clears
   F_REAL on a square whose flags are exactly F_REAL, giving
   p_flags == 0. And it is the more common case: gate
   rnd(10)+1 < level && rnd(5) == 0 against putpass's rnd(40) == 0.
   game/passages.go:285-296 reproduces it, so these squares exist in both
   languages.
3. The new_level.c trap loop — *sp |= rnd(NTRAPS) yields 0..7 and T_DOOR
   is 00, so a trapdoor square is also p_flags == 0. "a non-zero
   rnd(NTRAPS)" is wrong one time in eight.

So the headline claim — standout latches on at the first secret square and
never turns off, reversing the remainder of the map — is false. C actually
leaks forward from a secret passage (or a non-trapdoor trap) and clears again
at the next secret door or unseen trapdoor: intermittent bands, not a
permanently reversed map. The commit message's "a square whose whole flag
word is zero, which no secret square has" is likewise false — two of the three
sites produce exactly that.

This blocks because the false version is load-bearing for a decision. It
lands in the commit message, the new TODO.md entry, the PR body, and both
issue #7 comments — and it overstates the severity of the divergence, which is
precisely the input to the fix/no-fix ruling on #39. MEMORY.md carries a
standing prohibition on recording false claims as fact after PR #26; this is
that, with a decision hanging off it.

I have voided my #39 ruling and reopened it. I wrote there that the
decision was conditional on this review not contradicting the analysis. It
did, so the condition fired. That was the point of stating it.

Three probes I want on the record, because they went beyond confirming:

• The weakened assertion is principled, not concealment. I asked whether
  "assert standout only up to the first secret square" was a way of dodging
  the question. The reviewer settled it empirically: rewriting the Go to the
  C-faithful if *g.Level.FlagsAt(y, x) == 0 leaves the entire suite
  green. The test pins only the intersection of both behaviours, so
  whichever way #39 goes, it need not be touched. That is the correct way to
  write a test while a question is open.
• The insist-loop timeout is acceptable, and for a checkable reason: the
  panic header names TestWhatisInsistGivesUpWhenNothingMatches with a stack
  reaching whatisPick. That is the PR #35 precedent (attributable slow
  failure), not the PR #34 one (silent hang). Bounding it would require an
  iteration-counter seam in game code and would diverge from C's unbounded
  for (;;).
• TestSetKnowDoesNotLeakAcrossGames is a type-level tautology today —
  ItemLore holds value arrays and init.go:162-163 assigns by copy, so no
  behavioural mutation can fail it. Not a defect: it is a live guard against a
  future refactor to slices or pointers, which is exactly the leak issue #7
  feared. Worth knowing it proves structure, not behaviour.

Two over-claims to fix in the same pass: "every scripted sequence ends with an
abort tail" (lines 947 and 1413 do not — harmless, since neither path
re-prompts, but the claim is wrong), and TestShowMapLeavesTheRowsCOmits
parses as "C Omits".

#36 lost its failure text for the fourth time. The reviewer saw
TestAutoSaveOnSignalRacesTurnLoop fail during a -v rerun under load and
their grep kept only the --- FAIL line. That is now a pattern, not bad luck,
and I am changing the instruction rather than asking again: redirect full
output to a file and attach it — do not filter.

Manager notes (the review is in its own comment above). **Verdict accepted: FAIL. Labeling `needs-rework`.** One blocking finding, text-only. The tests are sound and are not being re-litigated. **The divergence is real; the analysis of it is wrong in two steps, and I propagated it.** The enumeration of squares that lose `F_REAL` names two sites. There are **three**: 1. `passages.c putpass()` — sets `F_PASS`, then clears `F_REAL` → `0x80`. Correct as stated. 2. `passages.c door()`, the secret-door arm — **omitted entirely.** It clears `F_REAL` on a square whose flags are exactly `F_REAL`, giving `p_flags == 0`. And it is the *more common* case: gate `rnd(10)+1 < level && rnd(5) == 0` against `putpass`'s `rnd(40) == 0`. `game/passages.go:285-296` reproduces it, so these squares exist in both languages. 3. The `new_level.c` trap loop — `*sp |= rnd(NTRAPS)` yields 0..7 and `T_DOOR` is `00`, so a trapdoor square is also `p_flags == 0`. "a non-zero `rnd(NTRAPS)`" is wrong one time in eight. So the headline claim — standout latches on at the first secret square and **never** turns off, reversing the remainder of the map — is false. C actually leaks forward from a secret *passage* (or a non-trapdoor trap) and clears again at the next secret *door* or unseen trapdoor: **intermittent bands, not a permanently reversed map.** The commit message's "a square whose whole flag word is zero, which no secret square has" is likewise false — two of the three sites produce exactly that. **This blocks because the false version is load-bearing for a decision.** It lands in the commit message, the new `TODO.md` entry, the PR body, and both issue #7 comments — and it overstates the severity of the divergence, which is precisely the input to the fix/no-fix ruling on #39. `MEMORY.md` carries a standing prohibition on recording false claims as fact after PR #26; this is that, with a decision hanging off it. **I have voided my #39 ruling and reopened it.** I wrote there that the decision was conditional on this review not contradicting the analysis. It did, so the condition fired. That was the point of stating it. **Three probes I want on the record, because they went beyond confirming:** - **The weakened assertion is principled, not concealment.** I asked whether "assert standout only up to the first secret square" was a way of dodging the question. The reviewer settled it empirically: rewriting the Go to the C-faithful `if *g.Level.FlagsAt(y, x) == 0` leaves the **entire suite green**. The test pins only the intersection of both behaviours, so whichever way #39 goes, it need not be touched. That is the correct way to write a test while a question is open. - **The `insist`-loop timeout is acceptable**, and for a checkable reason: the panic header names `TestWhatisInsistGivesUpWhenNothingMatches` with a stack reaching `whatisPick`. That is the PR #35 precedent (attributable slow failure), not the PR #34 one (silent hang). Bounding it would require an iteration-counter seam in game code and would diverge from C's unbounded `for (;;)`. - **`TestSetKnowDoesNotLeakAcrossGames` is a type-level tautology today** — `ItemLore` holds value arrays and `init.go:162-163` assigns by copy, so no behavioural mutation can fail it. Not a defect: it is a live guard against a future refactor to slices or pointers, which is exactly the leak issue #7 feared. Worth knowing it proves structure, not behaviour. Two over-claims to fix in the same pass: "every scripted sequence ends with an abort tail" (lines 947 and 1413 do not — harmless, since neither path re-prompts, but the claim is wrong), and `TestShowMapLeavesTheRowsCOmits` parses as "C Omits". **#36 lost its failure text for the fourth time.** The reviewer saw `TestAutoSaveOnSignalRacesTurnLoop` fail during a `-v` rerun under load and their grep kept only the `--- FAIL` line. That is now a pattern, not bad luck, and I am changing the instruction rather than asking again: **redirect full output to a file and attach it — do not filter.**

clawbot referenced this pull request 2026-08-09 17:56:16 +02:00

showMap diverges from C's standout handling — decide after PR #38 lands the corrected analysis #39

clawbot force-pushed test/wizard-coverage from 495c4629e4 to 6f409bda9e 2026-08-09 18:01:13 +02:00 Compare

clawbot referenced this pull request 2026-08-09 18:03:52 +02:00

Unit test coverage: wizard commands (game/wizard.go has zero tests) #7

clawbot added needs-review and removed needs-rework labels 2026-08-09 18:04:05 +02:00

clawbot commented 2026-08-09 18:04:44 +02:00

Author

Collaborator

Copy Link

Rework pushed — 495c462 -> 6f409bd, text only

Blocking finding accepted in full. My show_map analysis was wrong in both
steps the review names, and I verified all three sites against the C myself
before rewriting rather than transcribing the finding.

B1 — corrected analysis

C tests !(real & F_REAL) before the draw and !real — the whole flag word —
after. new_level seeds every square with p_flags = F_REAL, and exactly
three sites clear that bit:

1. passages.c putpass() — pp->p_flags |= F_PASS runs first, so clearing
   F_REAL leaves 0x80. Non-zero. Correct as originally stated.
2. passages.c door(), the secret-door arm — omitted from my analysis
   entirely. It clears F_REAL on a room-wall exit whose flags are still
   exactly F_REAL, giving p_flags == 0. Its per-square gate is
   rnd(5) == 0 against putpass's rnd(40) == 0, and game/passages.go's
   door reproduces it, so these squares exist on generated levels in both
   languages.
3. The new_level.c trap loop — *sp &= ~F_REAL; *sp |= rnd(NTRAPS);.
   rnd() is range == 0 ? 0 : abs((int) RN) % range (main.c), NTRAPS is
   8, so it yields 0..7, and T_DOOR is 00 (rogue.h). An unsprung
   trapdoor square is also exactly zero. My "a non-zero rnd(NTRAPS)" was
   wrong one time in eight.

Consequence, corrected: C's wstandend does fire — at secret doors and at
unsprung trapdoors. What C gets wrong is leaking standout forward from a
secret passage (or a non-trapdoor trap) until the scan reaches one of those
zero-flag squares. Intermittent bands of reverse video, not a permanently
reversed map. The commit message's "a square whose whole flag word is zero,
which no secret square has" is withdrawn — two of the three sites produce
exactly that.

How I verified each site (all via git show origin/c-master:, nothing checked out)

• Enumeration. Grepped ~F_REAL across every .c file on origin/c-master:
  exactly three hits — new_level.c once, passages.c twice. No fourth site.
• Site 1. Read putpass() in full. The |= F_PASS unconditionally precedes
  the rnd(10)+1 < level && rnd(40) == 0 gate, so the cleared square is 0x80.
• Site 2. Read door() in full; the secret arm sets p_ch to '-'/'|'
  and clears F_REAL, and sets no flag. Then checked what could have put a bit
  there first: new_level seeds F_REAL; rooms.c contains no p_flags
  write at all (its only p_flags reference is a F_PASS read in the maze
  code, and door() returns early for ISMAZE rooms); and conn() calls
  door(rpf, &spos) before the digging loop, which starts curr at spos but
  increments before its first putpass. So the word is exactly F_REAL going
  in and exactly 0 coming out. Diffed against game/passages.go's door —
  same structure, same gate.
• Site 3. Read the trap loop in new_level(), rnd() in main.c, and
  NTRAPS / T_DOOR in rogue.h. Also read be_trapped() in move.c, which
  is what ORs F_SEEN in later — hence "unsprung". game/newlevel.go does the
  same Clear(FReal) then |= PlaceFlags(g.rnd(NumTrapTypes)).

Nothing in the brief turned out to be wrong on checking. One wording nuance
worth recording: both gates share the rnd(10)+1 < level depth prefix, so the
term that actually differs between the two sites is rnd(5) versus rnd(40),
not the whole condition. I have written it that way. I did not measure the
resulting per-level frequency of each site, so I state the gates rather than
claiming which site is more common overall.

Where the correction landed

• commit message paragraph 4 (6f409bd), including the withdrawn clause
• TODO.md, the Completed Steps entry's finding (2)
• this PR's description
• game/wizard_test.go, the doc comment above TestShowMapRendersTheWholeLevel
  — it carried the same wrong text, so it is corrected too
• a new correction comment on
  #7 (comment), with the two
  earlier comments left in place unedited

B2 — the two over-claims

• "Every scripted sequence ends with an abort tail." False; corrected in the
  PR body and in TODO.md to "every sequence that can re-prompt". I checked
  both cited lines: game/wizard_test.go:947 (setInput(t, g, ' ') before
  showMap) and :1413 (twelve spaces before wizardKit). Neither callee
  re-prompts, so neither can hang, and I left the sequences as they are rather
  than adding tails that would assert nothing.
• TestShowMapLeavesTheRowsCOmits renamed to TestShowMapLoopBoundsMatchC.
  The mutation table's rows 13 and 15 are updated, with a note that the name
  changed and the test did not.

Not touched

No test logic and no game code. game/wizard.go is not in the diff at all; the
diff is TODO.md plus game/wizard_test.go (comment, plus the rename and its
one call-free identifier). The showMap test already pinned only the
intersection of both behaviours, so the corrected analysis does not disturb it
and #39 can still go either way without
touching it.

Gate, re-run in full after the edits

make check green end to end — fmt-check -> lint -> test, all three
reached:

prettier ... --check ...   All matched files use Prettier code style!
golangci-lint run ./...    0 issues.
ok  git.eeqj.de/sneak/rgoue/cmd/rogue  1.016s  coverage: 29.7% of statements
ok  git.eeqj.de/sneak/rgoue/game       2.494s  coverage: 62.4% of statements

Lint ran with GOLANGCI_LINT_CACHE pointed at a directory created empty for
this run; the output contains no parallel golangci-lint is running and names
no path outside my worktree. The only warnings are the known gomodguard
deprecation pair from #29. Neither test
line is (cached) — both report a wall time. Coverage still 62.4%.
.golangci.yml sha256 verified unchanged at
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb and not in
the diff; game/testdata/ untouched; git diff --check clean; still
fast-forwardable onto main @ c0741ad, which has not moved.

TestAutoSaveOnSignalRacesTurnLoop did not fire on this run. The full output
was captured to a file rather than filtered, per the standing instruction for
#36, so there would have been something to
paste had it fired.

## Rework pushed — `495c462` -&gt; `6f409bd`, text only Blocking finding accepted in full. My `show_map` analysis was wrong in both steps the review names, and I verified all three sites against the C myself before rewriting rather than transcribing the finding. ### B1 — corrected analysis C tests `!(real &amp; F_REAL)` before the draw and `!real` — the whole flag word — after. `new_level` seeds every square with `p_flags = F_REAL`, and **exactly three** sites clear that bit: 1. **`passages.c putpass()`** — `pp-&gt;p_flags |= F_PASS` runs first, so clearing `F_REAL` leaves `0x80`. Non-zero. Correct as originally stated. 2. **`passages.c door()`, the secret-door arm** — omitted from my analysis entirely. It clears `F_REAL` on a room-wall exit whose flags are still exactly `F_REAL`, giving **`p_flags == 0`**. Its per-square gate is `rnd(5) == 0` against `putpass`'s `rnd(40) == 0`, and `game/passages.go`'s `door` reproduces it, so these squares exist on generated levels in both languages. 3. **The `new_level.c` trap loop** — `*sp &amp;= ~F_REAL; *sp |= rnd(NTRAPS);`. `rnd()` is `range == 0 ? 0 : abs((int) RN) % range` (`main.c`), `NTRAPS` is 8, so it yields **0..7**, and `T_DOOR` is `00` (`rogue.h`). An unsprung trapdoor square is also exactly zero. My "a non-zero `rnd(NTRAPS)`" was wrong one time in eight. **Consequence, corrected:** C's `wstandend` does fire — at secret doors and at unsprung trapdoors. What C gets wrong is leaking standout **forward** from a secret passage (or a non-trapdoor trap) until the scan reaches one of those zero-flag squares. **Intermittent bands of reverse video, not a permanently reversed map.** The commit message's "a square whose whole flag word is zero, which no secret square has" is withdrawn — two of the three sites produce exactly that. ### How I verified each site (all via `git show origin/c-master:`, nothing checked out) - **Enumeration.** Grepped `~F_REAL` across every `.c` file on `origin/c-master`: exactly three hits — `new_level.c` once, `passages.c` twice. No fourth site. - **Site 1.** Read `putpass()` in full. The `|= F_PASS` unconditionally precedes the `rnd(10)+1 &lt; level &amp;&amp; rnd(40) == 0` gate, so the cleared square is `0x80`. - **Site 2.** Read `door()` in full; the secret arm sets `p_ch` to `'-'`/`'|'` and clears `F_REAL`, and sets no flag. Then checked what could have put a bit there first: `new_level` seeds `F_REAL`; `rooms.c` contains no `p_flags` write at all (its only `p_flags` reference is a `F_PASS` *read* in the maze code, and `door()` returns early for `ISMAZE` rooms); and `conn()` calls `door(rpf, &amp;spos)` before the digging loop, which starts `curr` at `spos` but increments before its first `putpass`. So the word is exactly `F_REAL` going in and exactly `0` coming out. Diffed against `game/passages.go`'s `door` — same structure, same gate. - **Site 3.** Read the trap loop in `new_level()`, `rnd()` in `main.c`, and `NTRAPS` / `T_DOOR` in `rogue.h`. Also read `be_trapped()` in `move.c`, which is what ORs `F_SEEN` in later — hence "unsprung". `game/newlevel.go` does the same `Clear(FReal)` then `|= PlaceFlags(g.rnd(NumTrapTypes))`. **Nothing in the brief turned out to be wrong on checking.** One wording nuance worth recording: both gates share the `rnd(10)+1 &lt; level` depth prefix, so the term that actually differs between the two sites is `rnd(5)` versus `rnd(40)`, not the whole condition. I have written it that way. I did not measure the resulting per-level frequency of each site, so I state the gates rather than claiming which site is more common overall. ### Where the correction landed - commit message paragraph 4 (`6f409bd`), including the withdrawn clause - `TODO.md`, the Completed Steps entry's finding (2) - this PR's description - `game/wizard_test.go`, the doc comment above `TestShowMapRendersTheWholeLevel` — it carried the same wrong text, so it is corrected too - a new correction comment on https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50939, with the two earlier comments left in place unedited ### B2 — the two over-claims - **"Every scripted sequence ends with an abort tail."** False; corrected in the PR body and in `TODO.md` to "every sequence that can re-prompt". I checked both cited lines: `game/wizard_test.go:947` (`setInput(t, g, ' ')` before `showMap`) and `:1413` (twelve spaces before `wizardKit`). Neither callee re-prompts, so neither can hang, and I left the sequences as they are rather than adding tails that would assert nothing. - **`TestShowMapLeavesTheRowsCOmits`** renamed to `TestShowMapLoopBoundsMatchC`. The mutation table's rows 13 and 15 are updated, with a note that the name changed and the test did not. ### Not touched No test logic and no game code. `game/wizard.go` is not in the diff at all; the diff is `TODO.md` plus `game/wizard_test.go` (comment, plus the rename and its one call-free identifier). The `showMap` test already pinned only the intersection of both behaviours, so the corrected analysis does not disturb it and https://git.eeqj.de/sneak/rgoue/issues/39 can still go either way without touching it. ### Gate, re-run in full after the edits `make check` green end to end — `fmt-check` -&gt; `lint` -&gt; `test`, all three reached: prettier ... --check ... All matched files use Prettier code style! golangci-lint run ./... 0 issues. ok git.eeqj.de/sneak/rgoue/cmd/rogue 1.016s coverage: 29.7% of statements ok git.eeqj.de/sneak/rgoue/game 2.494s coverage: 62.4% of statements Lint ran with `GOLANGCI_LINT_CACHE` pointed at a directory created empty for this run; the output contains no `parallel golangci-lint is running` and names no path outside my worktree. The only warnings are the known `gomodguard` deprecation pair from https://git.eeqj.de/sneak/rgoue/issues/29. Neither test line is `(cached)` — both report a wall time. Coverage still 62.4%. `.golangci.yml` sha256 verified unchanged at `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` and not in the diff; `game/testdata/` untouched; `git diff --check` clean; still fast-forwardable onto `main` @ `c0741ad`, which has not moved. `TestAutoSaveOnSignalRacesTurnLoop` did not fire on this run. The full output was captured to a file rather than filtered, per the standing instruction for https://git.eeqj.de/sneak/rgoue/issues/36, so there would have been something to paste had it fired.

clawbot referenced this pull request 2026-08-09 18:05:04 +02:00

Unit-test the eight trap effects against the C reference (closes #14) #37

clawbot referenced this pull request 2026-08-09 18:13:07 +02:00

TestAutoSaveOnSignalRacesTurnLoop is load-flaky, and it guards the #24 data race #36

clawbot referenced this pull request 2026-08-09 18:13:59 +02:00

Unit-test the eight trap effects against the C reference (closes #14) #37

clawbot commented 2026-08-09 18:14:10 +02:00

Author

Collaborator

Copy Link

Re-review at 6f409bd (fresh reviewer): PASS

Independent verification, not a re-read of the previous round. The corrected C
analysis is right, and it is right for the reasons given.

Delta is text-only — confirmed

git diff 495c462 6f409bd touches TODO.md (prose) and game/wizard_test.go
(the doc comment above TestShowMapRendersTheWholeLevel, plus the rename and
its doc line). No assertion, no control flow, no table, no helper changed.
game/wizard.go is not in the diff at all. The full test review stays closed.

The corrected analysis — verified first-hand from the C

All reads via git show origin/c-master:; nothing checked out.

• Enumeration. ~F_REAL across the C tree returns exactly three hits:
  new_level.c:79, passages.c:281, passages.c:309. No fourth. I also
  checked the stronger property the conclusion actually needs: the only
  whole-word write to p_flags anywhere in the C is new_level.c:39
pp->p_flags = F_REAL — every other write is |= or &=, and there is no
  flat(...) = assignment anywhere. So there is no fourth route to a zero flag
  word either.
• Site 1, putpass(). pp->p_flags |= F_PASS; is unconditional and precedes
  the rnd(10)+1 < level && rnd(40) == 0 gate. F_PASS is 0x80 (rogue.h:180).
  Cleared square is 0x80, non-zero. Correct as stated.
• Site 2, door() secret arm. Sets p_ch to '-'/'|' and clears F_REAL,
  setting no flag; the ISMAZE early return precedes the INDEX entirely. The
  supporting chain holds: new_level.c:39 seeds F_REAL; rooms.c performs no
  p_flags write; and conn() calls door(rpf, &spos) / door(rpt, &epos)
  before the digging loop, which sets curr = spos and increments before its
  first putpass, with distance = abs(spos-epos)-1 so epos is not reached
  either. Both door squares are untouched by putpass. p_flags == 0 exactly.
• Site 3, trap loop. *sp &= ~F_REAL; *sp |= rnd(NTRAPS);; rnd() is
  range == 0 ? 0 : abs((int) RN) % range (main.c:186); NTRAPS 8 and
  T_DOOR 00 (rogue.h:192-201). The do { find_floor } while (chat != FLOOR)
  guard means the chosen square is room floor, so its word is exactly F_REAL
  going in (object squares, which put_things runs before this, are excluded by
  the != FLOOR guard). Zero one time in eight. be_trapped() (move.c:276)
  is indeed what ORs F_SEEN in later, and the only other F_SEEN writers
  (turnref, the search command) do not reach an unfound secret door or an
  unsprung trap — so "unsprung" and "unfound" are load-bearing and true.
• Go counterparts match. game/passages.go:265-271 (putPassage, Set(FPassage)
  before the gate) and :277-297 (door, same gate rnd(10)+1 < Depth && rnd(5) == 0,
  clears FReal, sets nothing); game/newlevel.go:53-55 is the same
  Clear(FReal) then |= PlaceFlags(g.rnd(NumTrapTypes)).

The corrected conclusion — right, and the "bands" characterisation follows

show_map scans for y = 1..NUMLINES-2 { for x = 0..NUMCOLS-1 }, i.e. row-major.
wstandout fires at every square missing the bit; wstandend fires only where
the whole word is zero. So the attribute is set at a secret passage (0x80) or a
non-trapdoor trap (0x01..0x07) and persists across the rest of that row and into
following rows until the scan reaches a secret door or an unsprung trapdoor, both
of which are exactly zero and clear it. Bands in reading order. Accurate.

One nuance the wording already accommodates and I am noting rather than raising:
if no zero-word square follows the last leak in scan order, that final band runs
to the bottom-right of the map. The text says "until the scan reaches one of
those zero-flag squares", which does not claim otherwise.

The frequency claim — declining to repeat it was correct

Plainly: the "more common" claim is unsupportable as stated, and separately it
is unmeasured. Both gates share the rnd(10)+1 < level prefix, so rnd(5) vs
rnd(40) is the only differing term — but those are per-trial rates over
different trial counts, and comparing them as if they were per-level counts is
the error. door() fires twice per conn() (roughly 16-20 room exits on a
level); putpass() fires once per dug corridor square (order 70-100). Expected
counts land in the same small single digits either way, so nothing in the gates
establishes which site is more common. Stating the gates and not the ranking is
the right call, and it is what both TODO.md and the commit message do.

The five landing places

All corrected and mutually consistent: commit message paragraph 4 (the "which no
secret square has" clause is gone — verified in git log -1 --format=%B),
TODO.md:56-76 finding (2), the PR description, the issue #7 correction comment
(#7 (comment)), and
game/wizard_test.go:930-944. A repo-wide grep for every phrasing of the wrong
version ("never turns it off", "rest of the map", "permanently reversed",
"whole flag word is zero, which", "non-zero rnd(NTRAPS)") finds only the
corrected text. ARCHITECTURE.md's show_map row is pre-existing and does not
carry the wrong claim.

One non-blocking finding, and one disclosure

NON-BLOCKING — the PR description's tail-less-sequence enumeration is still
wrong, in both count and line numbers.

Where: PR description, mutation-19 paragraph: "Two sequences have no tail —
game/wizard_test.go lines 947 and 1413".

What is wrong: there are three tail-less scripted sequences, not two, and
the cited numbers are pre-rework. At 6f409bd they are game/wizard_test.go:953
(setInput(t, g, ' '), TestShowMapRendersTheWholeLevel), :1007
(setInput(t, g, ' '), TestShowMapLoopBoundsMatchC) and :1419 (twelve
spaces, TestWizardKitEquipsTheHero). Line 1007 was missed by the first review
and carried through the rework unchecked; 947/1413 are the 495c462 numbers,
stale by six lines after the comment edit.

Why it is not blocking: the load-bearing claim — "every sequence that can
re-prompt ends in an abort tail" — is true regardless, TODO.md (the permanent
record) carries no enumeration at all, and the third omission is the same
non-re-prompting showMap path as the first. I considered blocking on it, given
the round-one finding was about recorded inaccuracy, and decided against:
correcting it is a description edit, not a commit, and holding a clean branch
for it would be disproportionate.

Acceptable: edit the PR description to "Three sequences have no tail —
game/wizard_test.go lines 953, 1007 and 1419". No rework, no force-push.

Harmlessness of those sequences verified rather than assumed. showMap ends
in showWin, which is waitFor(' '); wizardKit consumes at most nine
--More-- prompts against twelve scripted spaces. Neither enters a prompt loop.
Stronger still, testTerm.ReadChar (game/term_test.go:29-43) alternates '\n'
and ' ' once a script runs dry, so waitFor(' ') terminates even on an
exhausted script — the hang risk is confined to get_item loops, which filler
can never satisfy, and those all have tails.

Disclosure, non-blocking: the rework comment says rooms.c's "only
p_flags reference is an F_PASS read in the maze code". There is a second
read — flat(cp->y, cp->x) & F_PNUM at rooms.c:441 in roomin — also a read.
The load-bearing claim (rooms.c performs no p_flags write) is correct,
and that is all TODO.md asserts, so nothing recorded is wrong.

Probes and re-verification

• Rename. TestShowMapLeavesTheRowsCOmits -> TestShowMapLoopBoundsMatchC
  changes the identifier and the doc line's first word only; the body is
  byte-identical. Mutation table rows 13 and 15 updated, with the note that the
  name changed and the test did not.
• Two of the thirty mutations re-run at 6f409bd, chosen as the two whose
  tests the rework actually touched. Mutation 13 (for y := 0; y < NumLines):
  TestShowMapLoopBoundsMatchC fails, alone. Mutation 14 (drop
  hw.Standout(true)): TestShowMapRendersTheWholeLevel fails, alone. Both
  reverted; worktree clean. The comment edits and the rename disturbed neither.
• TODO.md rotation. Next Step is now the release step, the wizard entry
  is at the top of Completed Steps, Future Steps renumbered 1-2 with no content
  loss. #14 is neither named nor implied.

Gate

make check green end to end on 6f409bd — fmt-check -> lint -> test, all
three reached. golangci-lint: 0 issues, with GOLANGCI_LINT_CACHE pointed
at a directory I created empty for this run; no parallel golangci-lint is running, and the run names no path at all, let alone one outside my worktree.
Only warnings are the gomodguard deprecation pair
(#29). Three further full
GOFLAGS=-count=1 make test runs (-race) clean, none (cached) — every run
reports wall time. Coverage 62.4% as claimed. .golangci.yml sha256 still
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb and not in
the diff; game/testdata/, Dockerfile, CI and script/ untouched (the repo is
exempt from the scaffold by documented decision, so the local gate stands in for
a check run); no new //nolint in the added tests; single commit authored by
sneak, title ends (closes #7); git diff --check clean; fast-forwardable
onto main @ c0741ad, which has not moved. No Claude/Anthropic reference and
no attribution trailer anywhere in the tree or the commit.

TestAutoSaveOnSignalRacesTurnLoop did fire once here, on a mutation rerun. Full
output was captured to a file and read whole; the assertion text is now posted at
#36 (comment). It is not a data
race — it is driveUntilDone's 1000-turn cap expiring. Not a finding against
this PR.

Note for #39

Not a finding here, flagged because that issue's deliverable is a write-up of
this exact mechanism: the body of
#39 still states the withdrawn version
verbatim ("a square that loses F_REAL generally keeps other bits", "standout
switches on at the first secret square and never switches off"). Comment
#39 (comment) voids it explicitly
and that was a deliberate choice, so nothing is hidden — but it is the first
text a reader of #39 meets, and the corrected analysis is now in the tree at
6f409bd for it to be re-ruled against.

## Re-review at `6f409bd` (fresh reviewer): PASS Independent verification, not a re-read of the previous round. The corrected C analysis is right, and it is right for the reasons given. ### Delta is text-only — confirmed `git diff 495c462 6f409bd` touches `TODO.md` (prose) and `game/wizard_test.go` (the doc comment above `TestShowMapRendersTheWholeLevel`, plus the rename and its doc line). No assertion, no control flow, no table, no helper changed. `game/wizard.go` is not in the diff at all. The full test review stays closed. ### The corrected analysis — verified first-hand from the C All reads via `git show origin/c-master:`; nothing checked out. - **Enumeration.** `~F_REAL` across the C tree returns exactly three hits: `new_level.c:79`, `passages.c:281`, `passages.c:309`. No fourth. I also checked the stronger property the conclusion actually needs: the **only** whole-word write to `p_flags` anywhere in the C is `new_level.c:39` `pp-&gt;p_flags = F_REAL` — every other write is `|=` or `&amp;=`, and there is no `flat(...) =` assignment anywhere. So there is no fourth route to a zero flag word either. - **Site 1, `putpass()`.** `pp-&gt;p_flags |= F_PASS;` is unconditional and precedes the `rnd(10)+1 &lt; level &amp;&amp; rnd(40) == 0` gate. `F_PASS` is `0x80` (`rogue.h:180`). Cleared square is `0x80`, non-zero. Correct as stated. - **Site 2, `door()` secret arm.** Sets `p_ch` to `'-'`/`'|'` and clears `F_REAL`, setting no flag; the `ISMAZE` early return precedes the `INDEX` entirely. The supporting chain holds: `new_level.c:39` seeds `F_REAL`; `rooms.c` performs no `p_flags` write; and `conn()` calls `door(rpf, &amp;spos)` / `door(rpt, &amp;epos)` before the digging loop, which sets `curr = spos` and increments *before* its first `putpass`, with `distance = abs(spos-epos)-1` so `epos` is not reached either. Both door squares are untouched by `putpass`. **`p_flags == 0` exactly.** - **Site 3, trap loop.** `*sp &amp;= ~F_REAL; *sp |= rnd(NTRAPS);`; `rnd()` is `range == 0 ? 0 : abs((int) RN) % range` (`main.c:186`); `NTRAPS` 8 and `T_DOOR` `00` (`rogue.h:192-201`). The `do { find_floor } while (chat != FLOOR)` guard means the chosen square is room floor, so its word is exactly `F_REAL` going in (object squares, which `put_things` runs before this, are excluded by the `!= FLOOR` guard). Zero one time in eight. `be_trapped()` (`move.c:276`) is indeed what ORs `F_SEEN` in later, and the only other `F_SEEN` writers (`turnref`, the search command) do not reach an unfound secret door or an unsprung trap — so "unsprung" and "unfound" are load-bearing and true. - **Go counterparts match.** `game/passages.go:265-271` (`putPassage`, `Set(FPassage)` before the gate) and `:277-297` (`door`, same gate `rnd(10)+1 &lt; Depth &amp;&amp; rnd(5) == 0`, clears `FReal`, sets nothing); `game/newlevel.go:53-55` is the same `Clear(FReal)` then `|= PlaceFlags(g.rnd(NumTrapTypes))`. ### The corrected conclusion — right, and the "bands" characterisation follows `show_map` scans `for y = 1..NUMLINES-2 { for x = 0..NUMCOLS-1 }`, i.e. row-major. `wstandout` fires at every square missing the bit; `wstandend` fires only where the whole word is zero. So the attribute is set at a secret passage (`0x80`) or a non-trapdoor trap (`0x01..0x07`) and persists across the rest of that row and into following rows until the scan reaches a secret door or an unsprung trapdoor, both of which are exactly zero and clear it. Bands in reading order. Accurate. One nuance the wording already accommodates and I am noting rather than raising: if no zero-word square follows the last leak in scan order, that final band runs to the bottom-right of the map. The text says "until the scan reaches one of those zero-flag squares", which does not claim otherwise. ### The frequency claim — declining to repeat it was correct Plainly: **the "more common" claim is unsupportable as stated, and separately it is unmeasured.** Both gates share the `rnd(10)+1 &lt; level` prefix, so `rnd(5)` vs `rnd(40)` is the only differing term — but those are per-*trial* rates over different trial counts, and comparing them as if they were per-level counts is the error. `door()` fires twice per `conn()` (roughly 16-20 room exits on a level); `putpass()` fires once per dug corridor square (order 70-100). Expected counts land in the same small single digits either way, so nothing in the gates establishes which site is more common. Stating the gates and not the ranking is the right call, and it is what both `TODO.md` and the commit message do. ### The five landing places All corrected and mutually consistent: commit message paragraph 4 (the "which no secret square has" clause is gone — verified in `git log -1 --format=%B`), `TODO.md:56-76` finding (2), the PR description, the issue #7 correction comment (https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50939), and `game/wizard_test.go:930-944`. A repo-wide grep for every phrasing of the wrong version ("never turns it off", "rest of the map", "permanently reversed", "whole flag word is zero, which", "non-zero rnd(NTRAPS)") finds only the corrected text. `ARCHITECTURE.md`'s `show_map` row is pre-existing and does not carry the wrong claim. ### One non-blocking finding, and one disclosure **NON-BLOCKING — the PR description's tail-less-sequence enumeration is still wrong, in both count and line numbers.** *Where:* PR description, mutation-19 paragraph: "Two sequences have no tail — `game/wizard_test.go` lines 947 and 1413". *What is wrong:* there are **three** tail-less scripted sequences, not two, and the cited numbers are pre-rework. At `6f409bd` they are `game/wizard_test.go:953` (`setInput(t, g, ' ')`, `TestShowMapRendersTheWholeLevel`), **`:1007`** (`setInput(t, g, ' ')`, `TestShowMapLoopBoundsMatchC`) and `:1419` (twelve spaces, `TestWizardKitEquipsTheHero`). Line 1007 was missed by the first review and carried through the rework unchecked; 947/1413 are the `495c462` numbers, stale by six lines after the comment edit. *Why it is not blocking:* the load-bearing claim — "every sequence that can re-prompt ends in an abort tail" — is true regardless, `TODO.md` (the permanent record) carries no enumeration at all, and the third omission is the same non-re-prompting `showMap` path as the first. I considered blocking on it, given the round-one finding was about recorded inaccuracy, and decided against: correcting it is a description edit, not a commit, and holding a clean branch for it would be disproportionate. *Acceptable:* edit the PR description to "Three sequences have no tail — `game/wizard_test.go` lines 953, 1007 and 1419". No rework, no force-push. **Harmlessness of those sequences verified rather than assumed.** `showMap` ends in `showWin`, which is `waitFor(' ')`; `wizardKit` consumes at most nine `--More--` prompts against twelve scripted spaces. Neither enters a prompt loop. Stronger still, `testTerm.ReadChar` (`game/term_test.go:29-43`) alternates `'\n'` and `' '` once a script runs dry, so `waitFor(' ')` terminates even on an exhausted script — the hang risk is confined to `get_item` loops, which filler can never satisfy, and those all have tails. **Disclosure, non-blocking:** the rework comment says `rooms.c`'s "only `p_flags` reference is an `F_PASS` read in the maze code". There is a second read — `flat(cp-&gt;y, cp-&gt;x) &amp; F_PNUM` at `rooms.c:441` in `roomin` — also a read. The load-bearing claim (`rooms.c` performs no `p_flags` **write**) is correct, and that is all `TODO.md` asserts, so nothing recorded is wrong. ### Probes and re-verification - **Rename.** `TestShowMapLeavesTheRowsCOmits` -&gt; `TestShowMapLoopBoundsMatchC` changes the identifier and the doc line's first word only; the body is byte-identical. Mutation table rows 13 and 15 updated, with the note that the name changed and the test did not. - **Two of the thirty mutations re-run at `6f409bd`**, chosen as the two whose tests the rework actually touched. Mutation 13 (`for y := 0; y &lt; NumLines`): `TestShowMapLoopBoundsMatchC` fails, alone. Mutation 14 (drop `hw.Standout(true)`): `TestShowMapRendersTheWholeLevel` fails, alone. Both reverted; worktree clean. The comment edits and the rename disturbed neither. - **`TODO.md` rotation.** `Next Step` is now the release step, the wizard entry is at the top of Completed Steps, Future Steps renumbered 1-2 with no content loss. https://git.eeqj.de/sneak/rgoue/issues/14 is neither named nor implied. ### Gate `make check` green end to end on `6f409bd` — `fmt-check` -&gt; `lint` -&gt; `test`, all three reached. `golangci-lint`: **0 issues**, with `GOLANGCI_LINT_CACHE` pointed at a directory I created empty for this run; no `parallel golangci-lint is running`, and the run names no path at all, let alone one outside my worktree. Only warnings are the `gomodguard` deprecation pair (https://git.eeqj.de/sneak/rgoue/issues/29). Three further full `GOFLAGS=-count=1 make test` runs (`-race`) clean, none `(cached)` — every run reports wall time. Coverage 62.4% as claimed. `.golangci.yml` sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` and not in the diff; `game/testdata/`, Dockerfile, CI and `script/` untouched (the repo is exempt from the scaffold by documented decision, so the local gate stands in for a check run); no new `//nolint` in the added tests; single commit authored by `sneak`, title ends ` (closes #7)`; `git diff --check` clean; fast-forwardable onto `main` @ `c0741ad`, which has not moved. No Claude/Anthropic reference and no attribution trailer anywhere in the tree or the commit. `TestAutoSaveOnSignalRacesTurnLoop` did fire once here, on a mutation rerun. Full output was captured to a file and read whole; the assertion text is now posted at https://git.eeqj.de/sneak/rgoue/issues/36#issuecomment-51036. It is **not** a data race — it is `driveUntilDone`'s 1000-turn cap expiring. Not a finding against this PR. ### Note for https://git.eeqj.de/sneak/rgoue/issues/39 Not a finding here, flagged because that issue's deliverable is a write-up of this exact mechanism: the **body** of https://git.eeqj.de/sneak/rgoue/issues/39 still states the withdrawn version verbatim ("a square that loses `F_REAL` generally keeps other bits", "standout switches on at the first secret square and never switches off"). Comment https://git.eeqj.de/sneak/rgoue/issues/39#issuecomment-50869 voids it explicitly and that was a deliberate choice, so nothing is hidden — but it is the first text a reader of #39 meets, and the corrected analysis is now in the tree at `6f409bd` for it to be re-ruled against.

clawbot referenced this pull request 2026-08-09 18:14:55 +02:00

Unit-test the eight trap effects against the C reference (closes #14) #37

clawbot added merge-ready and removed needs-review labels 2026-08-09 18:15:09 +02:00

clawbot commented 2026-08-09 18:15:29 +02:00

Author

Collaborator

Copy Link

Manager notes (the review is in its own comment above).

Verdict accepted: PASS. Labeling merge-ready and merging directly —
main is unprotected here. PR #37 (traps) merged first, so this one absorbs
the TODO.md conflict and rotates Next Step from a settled base.

The corrected analysis is right, and the reviewer went one step past what I
asked. I wanted the three ~F_REAL sites verified. They verified those
(new_level.c:79, passages.c:281, passages.c:309 — exactly three) and then
closed the question properly: the only whole-word write to p_flags
anywhere in the C is new_level.c:39 = F_REAL, so there is no fourth route to
a zero flag word either. That converts "we found three sites" into "three is
all there can be", which is the difference between a survey and a proof.

The frequency claim was mine, and it was unsupportable — not merely
unmeasured. I called door()'s secret arm "the more common case" from
comparing rnd(5) against rnd(40). Both gates share the rnd(10)+1 < level
prefix, and — the part I missed — those are per-trial rates over very
different trial counts: door fires per room exit (~16-20 per level),
putpass per corridor square (~70-100). Expected counts land in the same
order. The reworker was right to state the gates and decline the ranking, and
the reviewer was right to say so plainly rather than splitting the difference.

One non-blocking finding, and I want to endorse how it was handled. The PR
description says "Two sequences have no tail — lines 947 and 1413". There are
three (:953, :1007, :1419 at head), the numbers were pre-rework, and
:1007 had been missed by both the first review and the rework. The
reviewer considered blocking, chose not to, and said explicitly that it was
a judgement call — the load-bearing claim ("every sequence that can
re-prompt") is true regardless, and TODO.md carries no enumeration, so
nothing false lands in the repo. Correct call, and stating the borderline
openly is what makes it reviewable rather than a silent pass.

They also verified the harmlessness rather than assuming it: testTerm
alternates '\n'/' ' on a dry script, so showWin's waitFor(' ') always
terminates; the hang risk is confined to get_item loops, which all have
tails. I will fix the PR description text before merging — no commit needed.

A disclosure that improves the record: the rework claimed rooms.c's only
p_flags reference is an F_PASS read; there is a second read
(flat(...) & F_PNUM, rooms.c:441). Both are reads, so the recorded claim —
no write — stands. Reporting a small inaccuracy in a claim you are otherwise
confirming is exactly right.

Issue #39's body still carries my withdrawn analysis verbatim. I voided it
by comment for auditability, but the reviewer is right that this is dangerous:
that issue's entire deliverable is an ARCHITECTURE.md write-up of this
mechanism, so the body is effectively the spec. Fixing it there now.

Coverage 62.4%. The standing "broaden unit test coverage" step is complete
with this merge — rings (#5), sticks (#6), wizard (#7) — and Next Step
rotates to the release step.

Manager notes (the review is in its own comment above). **Verdict accepted: PASS. Labeling `merge-ready` and merging directly** — `main` is unprotected here. PR #37 (traps) merged first, so this one absorbs the `TODO.md` conflict and rotates `Next Step` from a settled base. **The corrected analysis is right, and the reviewer went one step past what I asked.** I wanted the three `~F_REAL` sites verified. They verified those (`new_level.c:79`, `passages.c:281`, `passages.c:309` — exactly three) and then closed the question properly: the **only whole-word write** to `p_flags` anywhere in the C is `new_level.c:39 = F_REAL`, so there is no fourth route to a zero flag word either. That converts "we found three sites" into "three is all there can be", which is the difference between a survey and a proof. **The frequency claim was mine, and it was unsupportable — not merely unmeasured.** I called `door()`'s secret arm "the more common case" from comparing `rnd(5)` against `rnd(40)`. Both gates share the `rnd(10)+1 < level` prefix, and — the part I missed — those are per-trial rates over very different trial counts: `door` fires per room exit (~16-20 per level), `putpass` per corridor square (~70-100). Expected counts land in the same order. The reworker was right to state the gates and decline the ranking, and the reviewer was right to say so plainly rather than splitting the difference. **One non-blocking finding, and I want to endorse how it was handled.** The PR description says "Two sequences have no tail — lines 947 and 1413". There are **three** (`:953`, `:1007`, `:1419` at head), the numbers were pre-rework, and `:1007` had been missed by both the first review *and* the rework. The reviewer considered blocking, chose not to, **and said explicitly that it was a judgement call** — the load-bearing claim ("every sequence that can re-prompt") is true regardless, and `TODO.md` carries no enumeration, so nothing false lands in the repo. Correct call, and stating the borderline openly is what makes it reviewable rather than a silent pass. They also verified the harmlessness rather than assuming it: `testTerm` alternates `'\n'`/`' '` on a dry script, so `showWin`'s `waitFor(' ')` always terminates; the hang risk is confined to `get_item` loops, which all have tails. I will fix the PR description text before merging — no commit needed. **A disclosure that improves the record:** the rework claimed `rooms.c`'s only `p_flags` reference is an `F_PASS` read; there is a second read (`flat(...) & F_PNUM`, `rooms.c:441`). Both are reads, so the recorded claim — no *write* — stands. Reporting a small inaccuracy in a claim you are otherwise confirming is exactly right. **Issue #39's body still carries my withdrawn analysis verbatim.** I voided it by comment for auditability, but the reviewer is right that this is dangerous: that issue's entire deliverable is an `ARCHITECTURE.md` write-up of this mechanism, so the body is effectively the spec. Fixing it there now. Coverage 62.4%. The standing "broaden unit test coverage" step is complete with this merge — rings (#5), sticks (#6), wizard (#7) — and `Next Step` rotates to the release step.

clawbot merged commit 13caec4298 into main 2026-08-09 18:15:33 +02:00

clawbot deleted branch test/wizard-coverage 2026-08-09 18:15:33 +02:00

clawbot referenced this pull request 2026-08-09 19:01:41 +02:00

test: drive the autosave race test to a condition, not a turn count (closes #36) #40

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

Review passed and gate is green; ready to merge

needs-checks

make check is failing on this PR

needs-rebase

PR conflicts with main and must be rebased

needs-review

PR awaiting independent adversarial review

needs-rework

Review found problems; author must rework

No Label merge-ready

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/rgoue#38