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
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):
passages.cputpass() — pp->p_flags |= F_PASS runs first, so clearing F_REAL leaves 0x80. Non-zero, so a secret passage never reaches wstandend.
passages.cdoor(), 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.
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 wstandenddoes 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.
(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.
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.
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.
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.
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.
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:
passages.cputpass() — sets F_PASS first, then clears F_REAL, leaving 0x80. Non-zero. The PR is right about this one.
passages.cdoor() — 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.
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:177weap.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.
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.
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:
passages.c putpass() — sets F_PASS, then clears F_REAL → 0x80.
Correct as stated.
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.
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.**
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:
passages.c putpass() — pp->p_flags |= F_PASS runs first, so clearing F_REAL leaves 0x80. Non-zero. Correct as originally stated.
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.
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_PASSread 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` -> `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
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` -> `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 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.
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_DOOR00 (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_flagswrite) 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.
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->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
(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->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. https://git.eeqj.de/sneak/rgoue/issues/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
(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.
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 main2026-08-09 18:15:33 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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 adebug surface:
set_knowwrites the per-game discovered tables that nameitems in ordinary play, and
teleportis what the teleport ring calls everyfiftieth 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 theTODO.mdrotation.What is covered
createObjo_group = 0,o_count = 1, filed byadd_packwith a pack letter; the GOLD arm'so_goldvalcreateWeaponArmorinit_dam[]dice; the inverted sign convention (cursed weapon-=hplus, cursed armor+=arm);a_class[]baselinecreateRingo_arm = (bless == '-' ? -1 : rnd(2)+1)on the four bonus rings; silent curse onR_AGGR/R_TELEPORT; every other kind untouchedshowMaphw, the loop bounds,show_win's promptwhatisISKNOW, the empty-pack early returnwhatisPickinsistloopsetKnowoi_guessfreed, and independence across two gamesteleportwizardKit(command.go)raise_levels, the (+1,+1) two-handed sword, plate mail ato_arm -5Wizard mode is entered the way the program does it:
Params{Wizard: true}isthe field
cmd/rogue/main.gofills fromROGUE_WIZARD, so nothing here pokesg.Wizard.Verification
make checkfully green (fmt-check, thenlint, thentest— the gateshort-circuits, so all three ran), re-run in full after the rework edits.
golangci-lint: 0 issues, run withGOLANGCI_LINT_CACHEpointed at afreshly created empty directory; no
parallel golangci-lint is running, andno reported path outside the worktree. The
gomodguarddeprecation warningis the known one from #29.
c0741ad, with and without the new file)..golangci.ymluntouched (sha256 still021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb);game/testdata/goldens untouched andTestSeedCompatItemTablesstillgreen.
Verified against C
wizard.cforcreate_obj/whatis/set_know/teleport/show_map,command.cfor theCTRL('I')kit,extern.cfora_class[],weapons.cfor
init_dam[]andinit_weapon,rogue.hfor theR_*numbering, theF_*place flags and theT_*trap numbering,main.cforrnd(), andnew_level.c/passages.c/rooms.cfor whereF_REALis cleared and whatelse writes
p_flags. All read viagit show origin/c-master:<file>;nothing was checked out.
One genuine divergence found, not fixed here.
show_map's standouthandling is asymmetric in C:
new_level.cseeds every square withp_flags = F_REAL, and exactly threesites clear that bit (grepping
~F_REALacross every.cfile finds threehits, and no more):
passages.cputpass()—pp->p_flags |= F_PASSruns first, so clearingF_REALleaves0x80. Non-zero, so a secret passage never reacheswstandend.passages.cdoor(), the secret-door arm — it clearsF_REALon aroom-wall exit whose flags are still exactly
F_REAL:new_levelseededthat,
rooms.cwrites nop_flagsat all (onlyp_ch), andconn()callsdoor()before the digging loop, which starts atsposbut incrementsbefore its first
putpass. Result:p_flags == 0exactly. Itsper-square gate is
rnd(5) == 0againstputpass'srnd(40) == 0, andgame/passages.go'sdoorreproduces it, so these squares occur ongenerated levels in both languages.
new_level.c's trap loop —*sp &= ~F_REAL; *sp |= rnd(NTRAPS);.rnd()is
range == 0 ? 0 : abs((int) RN) % range(main.c) andNTRAPSis 8, soit yields 0..7, and
T_DOORis00(rogue.h). An unsprung trapdoorsquare is therefore also exactly zero —
be_trappedis what later ORsF_SEENinto it.So C's
wstandenddoes fire: at secret doors and at unsprung trapdoors. WhatC 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.gotestsisRealon both sides and so highlights only theindividual 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.goto the C-faithful whole-word test andfinding the whole suite still green.)
One C quirk the port already reproduces exactly. A wizard-created "cursed"
weapon is not cursed.
create_objsetsISCURSED, theninit_weapondoesweap->o_flags = iwp->iw_flags— an assignment — and overwrites it; only theo_hpluspenalty 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's0whatever blessing was answered. Deleting theISCURSEDline from the portleaves 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 testrun, then reverted. In every casethe listed test failed and nothing else did.
obj.Group = 0->1CreateObjFilesTheItemInThePackcAtoi(buf)->0CreateObjGoldAsksHowMuch-=->+=CreateWeaponBlessing/cursed+=->-=CreateArmorBlessing/cursedarmorClass(Which)->0CreateArmorBlessing(all 3)initWeaponCreateWeaponBlessing(all 3)ISCURSEDsetCreateArmorBlessing/cursedonlyBonus = -1->1CreateRingBonuscursed x4rnd(2)+1->rnd(2)+3CreateRingBonusblessed/unblessed x8readcharin the silent-curse armCreateRingCursedKindsSkipThePromptx2RingSearchingto the bonus case listCreateRingOtherKindsAreLeftAlone/R_3show_mapy start1->2ShowMapRendersTheWholeLevelshow_mapbounds widened to0..NumLinesShowMapLoopBoundsMatchCShowMapRendersTheWholeLevel---More (level map)---textShowMapLoopBoundsMatchCPotionstableWhatisMarksTheRightTable/scroll,WhatisIdentifiesOnlyTheChosenEntry,WhatisInsistReprompts...setKnowWhatisWeaponAndArmorOnlySetTheFlagWhatisInsistRepromptsUntilAMatchn_objs == 0exitWhatisInsistGivesUpWhenNothingMatches— see belowoi_guessclearingWhatisMarksTheRightTablex3,SetKnowDoesNotLeakinfo[obj.Which]->info[0]WhatisMarksTheRightTablex3,SetKnowDoesNotLeak,WhatisIdentifiesOnlyTheChosenEntry,WhatisInsistReprompts...teleport's else branch (no room change)TeleportLandsTheHeroSomewhereLegalTeleportLandsTheHeroSomewhereLegalTeleportLeavesTheFlytrapAloneWhenFreeISHELDclearingTeleportReleasesTheFlytrapno_move/count/runningresetTeleportLandsTheHeroSomewhereLegalwizardKitnine levels -> eightWizardKitEquipsTheHero-5->-4WizardKitEquipsTheHeroWhatisEmptyPackSaysSo!insistearly return unconditionalWhatisInsistRepromptsUntilAMatch(Mutations 13 and 15 named
ShowMapLeavesTheRowsCOmitswhen they were run;that test has since been renamed to
ShowMapLoopBoundsMatchC— the old nameparsed 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 == 0is theinsist 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 becausetestTerm.ReadCharhands 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.golines 947 and 1413 — because neithershowMapnorwizardKitre-prompts, so neither can hang; an earlier revision of thisdescription 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 makesmutation 13 detectable.
TODO rotation — rotated, not narrowed
The issue said to rotate only if #6 had
landed. It had:
mainmoved frombf820e3toc0741ad(
test/sticks-coverage) while this was in progress, so this branch was movedonto
c0741adand re-verified there. With rings, sticks and wizard all donethe "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 FutureSteps renumbered. Traps (#14) was never
part of that step and is neither claimed nor implied by the rotation.
Left uncovered, and why
insistre-prompt messages ("you mustidentify something", "you must identify a %s"). Both are immediately
overwritten by the next
get_itemprompt, andMessageLinekeeps onlyHuh, so there is no seam to observe them through without adding one tonon-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.
show_map, deliberately, per the divergencesection above.
showMapmarking squares seen, because it does not: the functiontouches no
PLACEin either language. Noted on the issue as a correction tothe definition of done.
Reviewer's summary — details and the full tables are in the description
above.
Coverage.
gamepackage 60.6% -> 62.4%, measured on this branch's basec0741adwith and withoutgame/wizard_test.go's additions. Nothing alreadyin that file was modified.
Gate.
make checkfully green end to end (fmt-check->lint->test, and it short-circuits, so all three ran).golangci-lint: 0issues, run with
GOLANGCI_LINT_CACHEpointed at a private empty directory;the run reported no
parallel golangci-lint is runningand named no pathoutside the worktree. The only warning is the known
gomodguarddeprecation,#29.
.golangci.ymlis untouched andgame/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.candpassages.c(whereF_REALis cleared) — all viagit 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 == 0exit) fails by the 30s timeout andcannot do otherwise — that arm is the
insistloop's only exit besides amatching pick; the same test passes in 0.04s with the code correct.
Two things worth a reviewer's attention.
show_map's standout handling, describedin 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.
init_weaponassigns over theISCURSEDbitcreate_objjust set, so a wizard-created cursed weaponkeeps only its hit penalty. Mutation 7 is the evidence — deleting the
ISCURSEDline 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
insistre-promptmessages (each is overwritten by the following
get_itemprompt andMessageLineretains onlyHuh, so observing them would mean adding a seamto non-test code — both arms are covered behaviourally instead); C's leaked
standout, deliberately; and
showMapmarking squares seen, because it doesnot do that in either language.
Review: FAIL —
needs-reworkOne 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_mapdivergence is real, but its stated cause is wrong in two placesWhere: commit
495c462message, paragraph 4;TODO.mdCompleted Steps, the"(2)
show_map's standout is asymmetric" paragraph; the same wording in the PRdescription 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-138testsisRealboth times. Real, and reported ratherthan fixed, which is correct for a test-coverage commit.
What is wrong is the enumeration of squares that lose
F_REAL. The claim isthat they all retain other bits, so
real != 0everywhere and C "turns standouton 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:passages.cputpass()— setsF_PASSfirst, then clearsF_REAL, leaving0x80. Non-zero. The PR is right about this one.passages.cdoor()— the secret-door arm doespp->p_flags &= ~F_REALon asquare whose flags are exactly
F_REALat that point (new_levelseedsF_REAL;do_roomswrites onlyp_ch; nothing setsF_PASSon a room-wallexit before
door()runs). Result:p_flags == 0exactly. This site isomitted from the analysis entirely, and it is the most common one — its gate
is
rnd(10)+1 < level && rnd(5) == 0againstputpass'srnd(40) == 0.game/passages.go:285-296reproduces it, so these squares exist on generatedlevels in both languages.
new_level.c's trap loop —*sp &= ~F_REAL; *sp |= rnd(NTRAPS);.rnd()isabs((int) RN) % range(main.c), sornd(NTRAPS)withNTRAPS 8yields0..7, and 0 is
T_DOOR(rogue.h:#define T_DOOR 00). A trapdoorsquare is therefore also
p_flags == 0. "a non-zerornd(NTRAPS)" is wrongone 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.mdiswhere these C findings live permanently. A recorded-as-fact misreading of the
reference is the same class of defect
MEMORY.mdalready carries a standingprohibition 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 testchange is needed — see below.
Probes run
Rewriting
game/wizard.go:136to the C-faithfulif *g.Level.FlagsAt(y, x) == 0leaves the entire suite green.
TestShowMapRendersTheWholeLevelasserts only theintersection, and a future faithfulness fix would not have to touch it. The
approach is principled; the divergence is disclosed in four places, not hidden.
init_weaponclobber is confirmed in both languages.weapons.c:171weap->o_flags = iwp->iw_flagsandgame/weapons.go:177weap.Flags = iwp.flagsare both assignments over the
ISCURSEDset three lines earlier. Reproducedmutation 7 independently: deleting
obj.Flags.Set(Cursed)fromgame/wizard.go:73-75failsTestCreateArmorBlessing/cursed_raises_itandnothing else. The evidence does distinguish the two arms.
y := 0; y < NumLines):TestShowMapLeavesTheRowsCOmitsfails, alone. The plantedmarker is what makes it work.
(21 including the wand subtest not failing,
WandLightbeing index 0).precedent: the panic header is
running tests: TestWhatisInsistGivesUpWhenNothingMatches (30s)and the stack namesgame/wizard_test.go:1247->whatisPickatgame/wizard.go:181. Boundingthe 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 independencestructurally, not behaviourally:
ItemLore's tables are value arrays(
game/game.go:15-21) andinit.go:162-163assigns by copy, so no behaviouralmutation 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.
Two do not: line 947 (
setInput(t, g, ' ')) and line 1413 (twelve spaces, noEscape). Neither path re-prompts, so neither can hang. Narrative over-claimonly.
TestShowMapLeavesTheRowsCOmitsreads as "C Omits" onfirst pass.
Verified and passing
DoD item 1's correction confirmed (
show_mapwrites only intohw, touches noPLACE, in either language); all eightwizard.gofunctions pluswizardKitgenuinely covered; wizard mode entered via
Params{Wizard: true}with at.Fatalguard, no flag poking;make checkfully green withgolangci-lint0 issues on a private empty cache, no lock collision and no path outside the
worktree (
gomodguarddeprecation is #29);coverage 62.4% as claimed; three further
GOFLAGS=-count=1race runs clean;.golangci.ymlsha256 unchanged and not in the diff;game/testdata/untouchedand
TestSeedCompatItemTablesgreen; diff isgame/wizard_test.go+TODO.mdonly with no pre-existing test modified and no new nolints;
t.Parallel()everywhere (paralleltest clean);
//nolint:testpackageheader intact; TODOrotation correct — #5 and
#6 are merged and this closes
#7, and traps
(#14) is neither claimed nor implied;
git diff --checkclean; fast-forwardable ontomain@c0741ad; commit titleends
(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.
TestAutoSaveOnSignalRacesTurnLoopfailed oncehere, during a
-vrerun under load, and I lost the assertion text — my filterkept only the
--- FAILline. Nine further full runs did not reproduce it. Theissue still has no recorded failure text.
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_REALnames two sites.There are three:
passages.c putpass()— setsF_PASS, then clearsF_REAL→0x80.Correct as stated.
passages.c door(), the secret-door arm — omitted entirely. It clearsF_REALon a square whose flags are exactlyF_REAL, givingp_flags == 0. And it is the more common case: gaternd(10)+1 < level && rnd(5) == 0againstputpass'srnd(40) == 0.game/passages.go:285-296reproduces it, so these squares exist in bothlanguages.
new_level.ctrap loop —*sp |= rnd(NTRAPS)yields 0..7 andT_DOORis
00, so a trapdoor square is alsop_flags == 0. "a non-zerornd(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.mdentry, the PR body, and bothissue #7 comments — and it overstates the severity of the divergence, which is
precisely the input to the fix/no-fix ruling on #39.
MEMORY.mdcarries astanding 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:
"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) == 0leaves the entire suitegreen. 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.
insist-loop timeout is acceptable, and for a checkable reason: thepanic header names
TestWhatisInsistGivesUpWhenNothingMatcheswith a stackreaching
whatisPick. That is the PR #35 precedent (attributable slowfailure), 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 (;;).TestSetKnowDoesNotLeakAcrossGamesis a type-level tautology today —ItemLoreholds value arrays andinit.go:162-163assigns by copy, so nobehavioural 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
TestShowMapLeavesTheRowsCOmitsparses as "C Omits".
#36 lost its failure text for the fourth time. The reviewer saw
TestAutoSaveOnSignalRacesTurnLoopfail during a-vrerun under load andtheir grep kept only the
--- FAILline. 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.
495c4629e4to6f409bda9eRework pushed —
495c462->6f409bd, text onlyBlocking finding accepted in full. My
show_mapanalysis was wrong in bothsteps 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_levelseeds every square withp_flags = F_REAL, and exactlythree sites clear that bit:
passages.c putpass()—pp->p_flags |= F_PASSruns first, so clearingF_REALleaves0x80. Non-zero. Correct as originally stated.passages.c door(), the secret-door arm — omitted from my analysisentirely. It clears
F_REALon a room-wall exit whose flags are stillexactly
F_REAL, givingp_flags == 0. Its per-square gate isrnd(5) == 0againstputpass'srnd(40) == 0, andgame/passages.go'sdoorreproduces it, so these squares exist on generated levels in bothlanguages.
new_level.ctrap loop —*sp &= ~F_REAL; *sp |= rnd(NTRAPS);.rnd()isrange == 0 ? 0 : abs((int) RN) % range(main.c),NTRAPSis8, so it yields 0..7, and
T_DOORis00(rogue.h). An unsprungtrapdoor square is also exactly zero. My "a non-zero
rnd(NTRAPS)" waswrong one time in eight.
Consequence, corrected: C's
wstandenddoes fire — at secret doors and atunsprung 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)~F_REALacross every.cfile onorigin/c-master:exactly three hits —
new_level.conce,passages.ctwice. No fourth site.putpass()in full. The|= F_PASSunconditionally precedesthe
rnd(10)+1 < level && rnd(40) == 0gate, so the cleared square is0x80.door()in full; the secret arm setsp_chto'-'/'|'and clears
F_REAL, and sets no flag. Then checked what could have put a bitthere first:
new_levelseedsF_REAL;rooms.ccontains nop_flagswrite at all (its only
p_flagsreference is aF_PASSread in the mazecode, and
door()returns early forISMAZErooms); andconn()callsdoor(rpf, &spos)before the digging loop, which startscurratsposbutincrements before its first
putpass. So the word is exactlyF_REALgoingin and exactly
0coming out. Diffed againstgame/passages.go'sdoor—same structure, same gate.
new_level(),rnd()inmain.c, andNTRAPS/T_DOORinrogue.h. Also readbe_trapped()inmove.c, whichis what ORs
F_SEENin later — hence "unsprung".game/newlevel.godoes thesame
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 < leveldepth prefix, so theterm that actually differs between the two sites is
rnd(5)versusrnd(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
6f409bd), including the withdrawn clauseTODO.md, the Completed Steps entry's finding (2)game/wizard_test.go, the doc comment aboveTestShowMapRendersTheWholeLevel— it carried the same wrong text, so it is corrected too
#7 (comment), with the two
earlier comments left in place unedited
B2 — the two over-claims
PR body and in
TODO.mdto "every sequence that can re-prompt". I checkedboth cited lines:
game/wizard_test.go:947(setInput(t, g, ' ')beforeshowMap) and:1413(twelve spaces beforewizardKit). Neither calleere-prompts, so neither can hang, and I left the sequences as they are rather
than adding tails that would assert nothing.
TestShowMapLeavesTheRowsCOmitsrenamed toTestShowMapLoopBoundsMatchC.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.gois not in the diff at all; thediff is
TODO.mdplusgame/wizard_test.go(comment, plus the rename and itsone call-free identifier). The
showMaptest already pinned only theintersection 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 checkgreen end to end —fmt-check->lint->test, all threereached:
Lint ran with
GOLANGCI_LINT_CACHEpointed at a directory created empty forthis run; the output contains no
parallel golangci-lint is runningand namesno path outside my worktree. The only warnings are the known
gomodguarddeprecation pair from #29. Neither test
line is
(cached)— both report a wall time. Coverage still 62.4%..golangci.ymlsha256 verified unchanged at021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcband not inthe diff;
game/testdata/untouched;git diff --checkclean; stillfast-forwardable onto
main@c0741ad, which has not moved.TestAutoSaveOnSignalRacesTurnLoopdid not fire on this run. The full outputwas captured to a file rather than filtered, per the standing instruction for
#36, so there would have been something to
paste had it fired.
Re-review at
6f409bd(fresh reviewer): PASSIndependent 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 6f409bdtouchesTODO.md(prose) andgame/wizard_test.go(the doc comment above
TestShowMapRendersTheWholeLevel, plus the rename andits doc line). No assertion, no control flow, no table, no helper changed.
game/wizard.gois 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.~F_REALacross the C tree returns exactly three hits:new_level.c:79,passages.c:281,passages.c:309. No fourth. I alsochecked the stronger property the conclusion actually needs: the only
whole-word write to
p_flagsanywhere in the C isnew_level.c:39pp->p_flags = F_REAL— every other write is|=or&=, and there is noflat(...) =assignment anywhere. So there is no fourth route to a zero flagword either.
putpass().pp->p_flags |= F_PASS;is unconditional and precedesthe
rnd(10)+1 < level && rnd(40) == 0gate.F_PASSis0x80(rogue.h:180).Cleared square is
0x80, non-zero. Correct as stated.door()secret arm. Setsp_chto'-'/'|'and clearsF_REAL,setting no flag; the
ISMAZEearly return precedes theINDEXentirely. Thesupporting chain holds:
new_level.c:39seedsF_REAL;rooms.cperforms nop_flagswrite; andconn()callsdoor(rpf, &spos)/door(rpt, &epos)before the digging loop, which sets
curr = sposand increments before itsfirst
putpass, withdistance = abs(spos-epos)-1soeposis not reachedeither. Both door squares are untouched by
putpass.p_flags == 0exactly.*sp &= ~F_REAL; *sp |= rnd(NTRAPS);;rnd()isrange == 0 ? 0 : abs((int) RN) % range(main.c:186);NTRAPS8 andT_DOOR00(rogue.h:192-201). Thedo { find_floor } while (chat != FLOOR)guard means the chosen square is room floor, so its word is exactly
F_REALgoing in (object squares, which
put_thingsruns before this, are excluded bythe
!= FLOORguard). Zero one time in eight.be_trapped()(move.c:276)is indeed what ORs
F_SEENin later, and the only otherF_SEENwriters(
turnref, the search command) do not reach an unfound secret door or anunsprung trap — so "unsprung" and "unfound" are load-bearing and true.
game/passages.go:265-271(putPassage,Set(FPassage)before the gate) and
:277-297(door, same gaternd(10)+1 < Depth && rnd(5) == 0,clears
FReal, sets nothing);game/newlevel.go:53-55is the sameClear(FReal)then|= PlaceFlags(g.rnd(NumTrapTypes)).The corrected conclusion — right, and the "bands" characterisation follows
show_mapscansfor y = 1..NUMLINES-2 { for x = 0..NUMCOLS-1 }, i.e. row-major.wstandoutfires at every square missing the bit;wstandendfires only wherethe whole word is zero. So the attribute is set at a secret passage (
0x80) or anon-trapdoor trap (
0x01..0x07) and persists across the rest of that row and intofollowing 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 < levelprefix, sornd(5)vsrnd(40)is the only differing term — but those are per-trial rates overdifferent trial counts, and comparing them as if they were per-level counts is
the error.
door()fires twice perconn()(roughly 16-20 room exits on alevel);
putpass()fires once per dug corridor square (order 70-100). Expectedcounts 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.mdand 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-76finding (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 wrongversion ("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'sshow_maprow is pre-existing and does notcarry 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.golines 947 and 1413".What is wrong: there are three tail-less scripted sequences, not two, and
the cited numbers are pre-rework. At
6f409bdthey aregame/wizard_test.go:953(
setInput(t, g, ' '),TestShowMapRendersTheWholeLevel),:1007(
setInput(t, g, ' '),TestShowMapLoopBoundsMatchC) and:1419(twelvespaces,
TestWizardKitEquipsTheHero). Line 1007 was missed by the first reviewand carried through the rework unchecked; 947/1413 are the
495c462numbers,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 permanentrecord) carries no enumeration at all, and the third omission is the same
non-re-prompting
showMappath as the first. I considered blocking on it, giventhe 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.golines 953, 1007 and 1419". No rework, no force-push.Harmlessness of those sequences verified rather than assumed.
showMapendsin
showWin, which iswaitFor(' ');wizardKitconsumes 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, sowaitFor(' ')terminates even on anexhausted script — the hang risk is confined to
get_itemloops, which fillercan never satisfy, and those all have tails.
Disclosure, non-blocking: the rework comment says
rooms.c's "onlyp_flagsreference is anF_PASSread in the maze code". There is a secondread —
flat(cp->y, cp->x) & F_PNUMatrooms.c:441inroomin— also a read.The load-bearing claim (
rooms.cperforms nop_flagswrite) is correct,and that is all
TODO.mdasserts, so nothing recorded is wrong.Probes and re-verification
TestShowMapLeavesTheRowsCOmits->TestShowMapLoopBoundsMatchCchanges 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.
6f409bd, chosen as the two whosetests the rework actually touched. Mutation 13 (
for y := 0; y < NumLines):TestShowMapLoopBoundsMatchCfails, alone. Mutation 14 (drophw.Standout(true)):TestShowMapRendersTheWholeLevelfails, alone. Bothreverted; worktree clean. The comment edits and the rename disturbed neither.
TODO.mdrotation.Next Stepis now the release step, the wizard entryis at the top of Completed Steps, Future Steps renumbered 1-2 with no content
loss. #14 is neither named nor implied.
Gate
make checkgreen end to end on6f409bd—fmt-check->lint->test, allthree reached.
golangci-lint: 0 issues, withGOLANGCI_LINT_CACHEpointedat 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
gomodguarddeprecation pair(#29). Three further full
GOFLAGS=-count=1 make testruns (-race) clean, none(cached)— every runreports wall time. Coverage 62.4% as claimed.
.golangci.ymlsha256 still021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcband not inthe diff;
game/testdata/, Dockerfile, CI andscript/untouched (the repo isexempt from the scaffold by documented decision, so the local gate stands in for
a check run); no new
//nolintin the added tests; single commit authored bysneak, title ends(closes #7);git diff --checkclean; fast-forwardableonto
main@c0741ad, which has not moved. No Claude/Anthropic reference andno attribution trailer anywhere in the tree or the commit.
TestAutoSaveOnSignalRacesTurnLoopdid fire once here, on a mutation rerun. Fulloutput 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 againstthis 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_REALgenerally keeps other bits", "standoutswitches 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
6f409bdfor it to be re-ruled against.Manager notes (the review is in its own comment above).
Verdict accepted: PASS. Labeling
merge-readyand merging directly —mainis unprotected here. PR #37 (traps) merged first, so this one absorbsthe
TODO.mdconflict and rotatesNext Stepfrom a settled base.The corrected analysis is right, and the reviewer went one step past what I
asked. I wanted the three
~F_REALsites verified. They verified those(
new_level.c:79,passages.c:281,passages.c:309— exactly three) and thenclosed the question properly: the only whole-word write to
p_flagsanywhere in the C is
new_level.c:39 = F_REAL, so there is no fourth route toa 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" fromcomparing
rnd(5)againstrnd(40). Both gates share thernd(10)+1 < levelprefix, and — the part I missed — those are per-trial rates over very
different trial counts:
doorfires per room exit (~16-20 per level),putpassper corridor square (~70-100). Expected counts land in the sameorder. 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,:1419at head), the numbers were pre-rework, and:1007had been missed by both the first review and the rework. Thereviewer 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.mdcarries no enumeration, sonothing 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:
testTermalternates
'\n'/' 'on a dry script, soshowWin'swaitFor(' ')alwaysterminates; the hang risk is confined to
get_itemloops, which all havetails. I will fix the PR description text before merging — no commit needed.
A disclosure that improves the record: the rework claimed
rooms.c's onlyp_flagsreference is anF_PASSread; 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.mdwrite-up of thismechanism, 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 Steprotates to the release step.