sneak/rgoue
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 03:41:28 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 03:41:28 +02:00
Collaborator
Copy Link
Copy Source

Problem

game/wizard.go (230 lines, 8 functions) has no test coverage. The wizard
commands are the debug surface used to set up game states, so a break here
silently costs future debugging and playtesting — and nothing would catch it.

Third of three units splitting the TODO.md Next Step into commit-sized
pieces. Completing this one closes out that step.

Untested surface

Function What it does
createObj() wizard item creation from a type prompt
createWeaponArmor(obj) fill in weapon/armor specifics
createRing(obj) fill in ring specifics
showMap() reveal the whole level
whatis(insist, kind) identify an item
whatisPick(insist, kind) the item-selection half of whatis
setKnow(obj, info) mark an item type as known
teleport() wizard teleport

Note whatis/whatisPick/setKnow are not purely debug — setKnow mutates
the per-game "discovered" tables that drive item naming in normal play, so a
defect leaks into ordinary gameplay.

Definition of done

  1. A new game/wizard_test.go covering, at minimum:
    • createObj produces an object of the requested kind, correctly placed.
    • createWeaponArmor and createRing populate their kind-specific fields
      (damage dice / armor class / ring bonus).
    • showMap marks the level's squares seen — assert against a generated
      level, not a hand-built stub.
    • whatis on an unidentified item marks the type known and changes the
      item's displayed name.
    • setKnow mutates the correct per-game table entry (and, critically, that
      it does not leak across games — build two games and confirm independence).
    • teleport relocates the hero to a valid floor square, and updates
      whatever room/position bookkeeping the move requires.
  2. Wizard mode is entered the way the real code does (see the ROGUE_WIZARD
    handling), not by poking a flag, unless poking is the only option — in
    which case say so in a comment.
  3. make check fully green.
  4. TODO.md updated in the same commit — with #5 and #6 done, the
    "broaden unit test coverage (rings, sticks, wizard commands)" step is
    complete and should be rotated into Completed Steps, promoting the next
    Future Step.
  5. Commit title ends with (closes #N).

Implementation requirements

  • Cross-check against the C reference (git show origin/c-master:wizard.c).
    Do NOT check out or modify origin/c-master. Do not write tests that merely
    restate current Go behavior.
  • If you find a real divergence from C, stop and report it rather than
    fixing gameplay in a test-coverage commit.
  • Tests must call t.Parallel(); keep the lint run at 0 issues.
  • White-box tests need the
    //nolint:testpackage // white-box tests reach unexported state (approved 2026-07-07)
    header.
  • make targets only. Do NOT modify .golangci.yml. No game behavior changes.
  • Watch for os.Exit paths — wizard commands that can end the game must not
    terminate the test binary.
  • Never mention Claude or Anthropic anywhere.

Depends on

Best done after #5 and #6 so the TODO.md step rotation is a single clean
edit, but it is independently implementable if picked up first (in which case
narrow the step instead of closing it).

## Problem `game/wizard.go` (230 lines, 8 functions) has **no test coverage**. The wizard commands are the debug surface used to set up game states, so a break here silently costs future debugging and playtesting — and nothing would catch it. Third of three units splitting the `TODO.md` Next Step into commit-sized pieces. Completing this one closes out that step. ## Untested surface | Function | What it does | | --- | --- | | `createObj()` | wizard item creation from a type prompt | | `createWeaponArmor(obj)` | fill in weapon/armor specifics | | `createRing(obj)` | fill in ring specifics | | `showMap()` | reveal the whole level | | `whatis(insist, kind)` | identify an item | | `whatisPick(insist, kind)` | the item-selection half of `whatis` | | `setKnow(obj, info)` | mark an item type as known | | `teleport()` | wizard teleport | Note `whatis`/`whatisPick`/`setKnow` are not purely debug — `setKnow` mutates the per-game "discovered" tables that drive item naming in normal play, so a defect leaks into ordinary gameplay. ## Definition of done 1. A new `game/wizard_test.go` covering, at minimum: - `createObj` produces an object of the requested kind, correctly placed. - `createWeaponArmor` and `createRing` populate their kind-specific fields (damage dice / armor class / ring bonus). - `showMap` marks the level's squares seen — assert against a generated level, not a hand-built stub. - `whatis` on an unidentified item marks the type known and changes the item's displayed name. - `setKnow` mutates the correct per-game table entry (and, critically, that it does not leak across games — build two games and confirm independence). - `teleport` relocates the hero to a valid floor square, and updates whatever room/position bookkeeping the move requires. 2. Wizard mode is entered the way the real code does (see the `ROGUE_WIZARD` handling), not by poking a flag, unless poking is the only option — in which case say so in a comment. 3. `make check` fully green. 4. `TODO.md` updated in the same commit — with #5 and #6 done, the "broaden unit test coverage (rings, sticks, wizard commands)" step is complete and should be rotated into Completed Steps, promoting the next Future Step. 5. Commit title ends with ` (closes #N)`. ## Implementation requirements - Cross-check against the C reference (`git show origin/c-master:wizard.c`). Do NOT check out or modify `origin/c-master`. Do not write tests that merely restate current Go behavior. - If you find a real divergence from C, **stop and report it** rather than fixing gameplay in a test-coverage commit. - Tests must call `t.Parallel()`; keep the lint run at 0 issues. - White-box tests need the `//nolint:testpackage // white-box tests reach unexported state (approved 2026-07-07)` header. - `make` targets only. Do NOT modify `.golangci.yml`. No game behavior changes. - Watch for `os.Exit` paths — wizard commands that can end the game must not terminate the test binary. - Never mention Claude or Anthropic anywhere. ## Depends on Best done after #5 and #6 so the `TODO.md` step rotation is a single clean edit, but it is independently implementable if picked up first (in which case narrow the step instead of closing it).
clawbot commented 2026-08-09 17:14:42 +02:00
Author
Collaborator
Copy Link
Copy Source

Implementation plan

Branch test/wizard-coverage off main @ bf820e3. Everything below is
transcribed from git show origin/c-master:wizard.c (plus command.c for the
^I kit and rogue.h for the R_* numbering and F_* place flags), not from
what the port currently returns.

Additions go into the existing game/wizard_test.go (created by
#20, extended by
#30) so the mustNotPanic / malformed
harness is reused rather than duplicated. Nothing already there is touched.

  1. createObj — the existing tests already pin kind/Which selection and the
    bounds guard. Adding: the object is actually filed in the pack with a pack
    letter and Count == 1, Group == 0 (C sets both before the kind switch),
    and the gold arm ('*') sets GoldValue from the typed number, C's
    get_num(&obj->o_goldval, stdscr).
  2. createWeaponArmor — the sign convention is the substantive thing here and
    it is inverted between the two arms in C: a cursed weapon does
    o_hplus -= rnd(3)+1 and a blessed one +=, while cursed armor does
    o_arm += rnd(3)+1 and blessed -= (lower AC is better). Plus ISCURSED
    on '-', the init_weapon damage dice for the weapon arm, and
    o_arm = a_class[o_which] as the armor baseline.
  3. createRing — o_arm = (bless == '-' ? -1 : rnd(2)+1) for the four bonus
    rings (R_PROTECT, R_ADDSTR, R_ADDHIT, R_ADDDAM); ISCURSED and
    no prompt at all for R_AGGR/R_TELEPORT; nothing for every other
    kind. The "no prompt" half is asserted by checking the scripted input
    cursor did not advance, which is what would catch a stray readchar
    desynchronising every later answer.
  4. showMap — against a generated level (fixed seed, depth deep enough that
    the generator actually produces non-F_REAL squares), asserting every cell
    in y 1..NumLines-2 of the hw window carries the map char, and that
    rows 0 and NumLines-1 are left alone, since C's loop excludes them.
  5. whatis / whatisPick — an unidentified item becomes Know in the right
    per-game table and its inventoryName changes; the insist loop's three
    C arms (n_objs == 0 gives up, NULL gives "you must identify something",
    wrong kind gives "you must identify a %s"); the empty-pack early return.
  6. setKnow — right table, right index, Guess cleared, ISKNOW set, and
    two independent games confirming no cross-game leak.
  7. teleport — lands on a square stepOk accepts, Player.Room agrees with
    roomIn(pos) afterwards, the vacated square is redrawn with floorAt()
    and the new one with PLAYER, and the Flytrap unhold (ISHELD cleared,
    vf_hit = 0, monster 'F' damage reset to 000x0) which is the one part
    of the function that touches ordinary play.
  8. wizardKit (game/command.go) — nine raise_level() calls, the
    (+1,+1) two-handed sword as cur_weapon, plate mail at o_arm == -5 with
    ISKNOW as cur_armor.

Wizard mode is entered through the real path: Params{Wizard: true} is the
field cmd/rogue/main.go fills from ROGUE_WIZARD, so no flag poking is
needed. Every scripted input sequence gets an abort tail, because
testTerm.ReadChar returns filler forever once the script runs out and a
re-prompting loop would spin to the 30s timeout instead of failing. Each
substantive assertion will be mutation-checked (break the behaviour, confirm
that test and only that test fails, revert) and the results posted.

Two corrections to the issue before I start

  • DoD item 1 says showMap "marks the level's squares seen". It does not,
    in either language: C's show_map writes into the hw window and calls
    show_win, and the Go port does the same. No F_SEEN is set and no
    Place is modified — the whole function is display-only. The test will
    assert what it does do (renders the whole map into hw) against a
    generated level as asked, but it cannot assert squares becoming seen.
  • DoD item 4's rotation assumes #6 has landed. As of bf820e3 it has
    not, so per the issue's own "in which case narrow the step instead of
    closing it" I will narrow Next Step to sticks only unless #6 merges
    before I commit.

One divergence found while reading the C (not fixing it here)

show_map's standout handling is asymmetric in C and symmetric in the port:

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, so real is only
ever zero where F_REAL was cleared from a word that held nothing else.
putpass clears it on a square that already has F_PASS (secret passage),
and the trap loop clears it on a square it then ORs a non-zero rnd(NTRAPS)
into — in both cases real != 0, so C turns standout on and never off,
and the remainder of the map renders in reverse video. game/wizard.go uses
isReal for both tests, so it highlights only the individual square.

The port's behaviour is the sane one and this is a wizard-only display quirk,
so I am not touching it in a test-coverage commit and not filing it as a
defect. What I will not do either is write a test asserting the symmetric
behaviour as if it were C-verified: the showMap test asserts the map
characters unconditionally and the standout attribute only up to the first
non-real square, with the quirk quoted in a comment. Flagging it here so it
can be decided on deliberately.

## Implementation plan Branch `test/wizard-coverage` off `main` @ `bf820e3`. Everything below is transcribed from `git show origin/c-master:wizard.c` (plus `command.c` for the `^I` kit and `rogue.h` for the `R_*` numbering and `F_*` place flags), not from what the port currently returns. Additions go into the existing `game/wizard_test.go` (created by https://git.eeqj.de/sneak/rgoue/pulls/20, extended by https://git.eeqj.de/sneak/rgoue/pulls/30) so the `mustNotPanic` / `malformed` harness is reused rather than duplicated. Nothing already there is touched. 1. `createObj` — the existing tests already pin kind/`Which` selection and the bounds guard. Adding: the object is actually filed in the pack with a pack letter and `Count == 1`, `Group == 0` (C sets both before the kind switch), and the gold arm (`'*'`) sets `GoldValue` from the typed number, C's `get_num(&obj-&gt;o_goldval, stdscr)`. 2. `createWeaponArmor` — the sign convention is the substantive thing here and it is inverted between the two arms in C: a cursed weapon does `o_hplus -= rnd(3)+1` and a blessed one `+=`, while cursed **armor** does `o_arm += rnd(3)+1` and blessed `-=` (lower AC is better). Plus `ISCURSED` on `'-'`, the `init_weapon` damage dice for the weapon arm, and `o_arm = a_class[o_which]` as the armor baseline. 3. `createRing` — `o_arm = (bless == '-' ? -1 : rnd(2)+1)` for the four bonus rings (`R_PROTECT`, `R_ADDSTR`, `R_ADDHIT`, `R_ADDDAM`); `ISCURSED` and **no prompt at all** for `R_AGGR`/`R_TELEPORT`; nothing for every other kind. The "no prompt" half is asserted by checking the scripted input cursor did not advance, which is what would catch a stray `readchar` desynchronising every later answer. 4. `showMap` — against a generated level (fixed seed, depth deep enough that the generator actually produces non-`F_REAL` squares), asserting every cell in `y` 1..`NumLines-2` of the `hw` window carries the map char, and that rows 0 and `NumLines-1` are left alone, since C's loop excludes them. 5. `whatis` / `whatisPick` — an unidentified item becomes `Know` in the right per-game table and its `inventoryName` changes; the `insist` loop's three C arms (`n_objs == 0` gives up, `NULL` gives "you must identify something", wrong kind gives "you must identify a %s"); the empty-pack early return. 6. `setKnow` — right table, right index, `Guess` cleared, `ISKNOW` set, and two independent games confirming no cross-game leak. 7. `teleport` — lands on a square `stepOk` accepts, `Player.Room` agrees with `roomIn(pos)` afterwards, the vacated square is redrawn with `floorAt()` and the new one with `PLAYER`, and the Flytrap unhold (`ISHELD` cleared, `vf_hit = 0`, monster `'F'` damage reset to `000x0`) which is the one part of the function that touches ordinary play. 8. `wizardKit` (`game/command.go`) — nine `raise_level()` calls, the (+1,+1) two-handed sword as `cur_weapon`, plate mail at `o_arm == -5` with `ISKNOW` as `cur_armor`. Wizard mode is entered through the real path: `Params{Wizard: true}` is the field `cmd/rogue/main.go` fills from `ROGUE_WIZARD`, so no flag poking is needed. Every scripted input sequence gets an abort tail, because `testTerm.ReadChar` returns filler forever once the script runs out and a re-prompting loop would spin to the 30s timeout instead of failing. Each substantive assertion will be mutation-checked (break the behaviour, confirm that test and only that test fails, revert) and the results posted. ## Two corrections to the issue before I start - **DoD item 1 says `showMap` "marks the level's squares seen".** It does not, in either language: C's `show_map` writes into the `hw` window and calls `show_win`, and the Go port does the same. No `F_SEEN` is set and no `Place` is modified — the whole function is display-only. The test will assert what it does do (renders the whole map into `hw`) against a generated level as asked, but it cannot assert squares becoming seen. - **DoD item 4's rotation** assumes #6 has landed. As of `bf820e3` it has not, so per the issue's own "in which case narrow the step instead of closing it" I will narrow `Next Step` to sticks only unless #6 merges before I commit. ## One divergence found while reading the C (not fixing it here) `show_map`'s standout handling is asymmetric in C and symmetric in the port: 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`, so `real` is only ever zero where `F_REAL` was cleared from a word that held nothing else. `putpass` clears it on a square that already has `F_PASS` (secret passage), and the trap loop clears it on a square it then ORs a non-zero `rnd(NTRAPS)` into — in both cases `real != 0`, so C turns standout **on and never off**, and the remainder of the map renders in reverse video. `game/wizard.go` uses `isReal` for both tests, so it highlights only the individual square. The port's behaviour is the sane one and this is a wizard-only display quirk, so I am not touching it in a test-coverage commit and not filing it as a defect. What I will not do either is write a test asserting the symmetric behaviour as if it were C-verified: the `showMap` test asserts the map characters unconditionally and the standout attribute only up to the first non-real square, with the quirk quoted in a comment. Flagging it here so it can be decided on deliberately.
clawbot commented 2026-08-09 17:41:38 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in #38 (branch
test/wizard-coverage, commit 495c462). One commit, tests plus the
TODO.md rotation; no game behaviour changed.

What was built. Additions to the existing game/wizard_test.go covering
all eight functions of game/wizard.go plus wizardKit from
game/command.go: createObj's shared tail (o_group = 0, o_count = 1,
add_pack filing) and its GOLD arm; createWeaponArmor's dice and the
inverted blessing sign between the weapon and armor arms; createRing's three
case groups; showMap's rendering and loop bounds; whatis's per-kind table
dispatch, its weapon/armor arm and its empty-pack return; all three arms of
whatisPick's insist loop; setKnow's entry, guess-clearing and
cross-game independence; and teleport's destination, room bookkeeping,
redraw, run-state reset and Flytrap unhold. Wizard mode is entered through
Params{Wizard: true} — the field cmd/rogue/main.go fills from
ROGUE_WIZARD — so nothing pokes the flag.

How it was verified. make check fully green; golangci-lint 0 issues on
a private cache with no lock collision and no path outside the worktree;
package coverage 60.6% -> 62.4% measured against the branch base with and
without the new file. Every expected value transcribed from wizard.c,
command.c, extern.c, weapons.c, rogue.h, new_level.c and
passages.c via git show origin/c-master:. 30 mutations run one at a
time, 30 caught, each with the intended test failing and nothing else; 29
fail in under a second, and the one that fails by timeout does so because the
arm it deletes is the insist loop's only exit.

On the two corrections raised in the plan.

  • showMap marking squares seen: confirmed it does not, so that part of the
    definition of done could not be met as written. The loop bounds are pinned
    instead by planting a marker in the rows C's loop excludes — needed because
    those rows are blank on a real level, so "untouched" was otherwise
    unfalsifiable (a mutation caught that weakness in my own first draft).
  • The rotation: #6 merged while this was
    in flight, so main moved to c0741ad. The branch was moved onto it and
    re-verified, and the step is therefore rotated rather than narrowed,
    with the release step promoted into Next Step.

One further C finding, beyond the show_map standout divergence already
reported above. A wizard-created "cursed" weapon is not cursed, in C or in
the port: create_obj sets ISCURSED and init_weapon then assigns
weap-&gt;o_flags = iwp-&gt;iw_flags straight over it, leaving only the o_hplus
penalty. The port is faithful; the test pins the whole flag word, and deleting
the ISCURSED line leaves every weapon test green, which is the evidence the
line is dead for weapons. Recorded, not changed.

Implemented in https://git.eeqj.de/sneak/rgoue/pulls/38 (branch `test/wizard-coverage`, commit `495c462`). One commit, tests plus the `TODO.md` rotation; no game behaviour changed. **What was built.** Additions to the existing `game/wizard_test.go` covering all eight functions of `game/wizard.go` plus `wizardKit` from `game/command.go`: `createObj`'s shared tail (`o_group = 0`, `o_count = 1`, `add_pack` filing) and its GOLD arm; `createWeaponArmor`'s dice and the inverted blessing sign between the weapon and armor arms; `createRing`'s three case groups; `showMap`'s rendering and loop bounds; `whatis`'s per-kind table dispatch, its weapon/armor arm and its empty-pack return; all three arms of `whatisPick`'s `insist` loop; `setKnow`'s entry, guess-clearing and cross-game independence; and `teleport`'s destination, room bookkeeping, redraw, run-state reset and Flytrap unhold. Wizard mode is entered through `Params{Wizard: true}` — the field `cmd/rogue/main.go` fills from `ROGUE_WIZARD` — so nothing pokes the flag. **How it was verified.** `make check` fully green; `golangci-lint` 0 issues on a private cache with no lock collision and no path outside the worktree; package coverage 60.6% -&gt; 62.4% measured against the branch base with and without the new file. Every expected value transcribed from `wizard.c`, `command.c`, `extern.c`, `weapons.c`, `rogue.h`, `new_level.c` and `passages.c` via `git show origin/c-master:`. **30 mutations run one at a time, 30 caught**, each with the intended test failing and nothing else; 29 fail in under a second, and the one that fails by timeout does so because the arm it deletes is the `insist` loop's only exit. **On the two corrections raised in the plan.** - `showMap` marking squares seen: confirmed it does not, so that part of the definition of done could not be met as written. The loop bounds are pinned instead by planting a marker in the rows C's loop excludes — needed because those rows are blank on a real level, so "untouched" was otherwise unfalsifiable (a mutation caught that weakness in my own first draft). - The rotation: https://git.eeqj.de/sneak/rgoue/issues/6 merged while this was in flight, so `main` moved to `c0741ad`. The branch was moved onto it and re-verified, and the step is therefore **rotated** rather than narrowed, with the release step promoted into `Next Step`. **One further C finding, beyond the `show_map` standout divergence already reported above.** A wizard-created "cursed" weapon is not cursed, in C or in the port: `create_obj` sets `ISCURSED` and `init_weapon` then *assigns* `weap-&gt;o_flags = iwp-&gt;iw_flags` straight over it, leaving only the `o_hplus` penalty. The port is faithful; the test pins the whole flag word, and deleting the `ISCURSED` line leaves every weapon test green, which is the evidence the line is dead for weapons. Recorded, not changed.
clawbot commented 2026-08-09 18:03:52 +02:00
Author
Collaborator
Copy Link
Copy Source

Correction: my show_map C analysis above was wrong

This corrects the two comments above
(#7 (comment) and
#7 (comment)), which are left in
place. It was caught in review of #38 and
is now fixed in the commit message, TODO.md, the PR body and the showMap
test's comment at 6f409bd.

The divergence is real and unchanged. C tests !(real &amp; F_REAL) before the
draw and !real — the whole flag word — after; game/wizard.go tests isReal
on both sides. What I got wrong was the enumeration of squares that lose
F_REAL, and therefore the consequence.

I claimed two sites and said every such square keeps other bits, so real is
never zero and C latches standout on at the first secret square and never turns
it off, reversing the rest of the map. There are three sites — grepping
~F_REAL across every .c file finds exactly three hits — and two of them
leave the flag word at zero:

  1. passages.c putpass() — pp-&gt;p_flags |= F_PASS runs first, so clearing
    F_REAL leaves 0x80. Non-zero. This one I had right.
  2. passages.c door(), the secret-door arm — I omitted it entirely. It
    clears F_REAL on a room-wall exit whose flags are still exactly F_REAL:
    new_level seeds 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. The new_level.c trap loop — *sp &amp;= ~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 in. My "a non-zero rnd(NTRAPS)" was wrong one time in eight.

Corrected behaviour: 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. That is intermittent bands of reverse video, not a
permanently reversed map — a materially milder defect than I described, which
matters because that severity is the input to
#39.

Also withdrawn: the commit message's "a square whose whole flag word is zero,
which no secret square has". Two of the three sites produce exactly that.

How I verified it, first-hand rather than from the review. All reads via
git show origin/c-master:&lt;file&gt;, nothing checked out.

  • Site enumeration: ~F_REAL over every .c file in origin/c-master returns
    three hits and no others — new_level.c, and passages.c twice.
  • Site 1: read putpass() in full; the |= F_PASS precedes the gate.
  • Site 2: read door() in full, plus conn() for the call order and rooms.c
    for what touches p_flags (nothing — the only p_flags reference there is a
    read of F_PASS in the maze code, and door() returns early for ISMAZE
    rooms anyway). Compared against game/passages.go's door, which matches
    line for line.
  • Site 3: read new_level()'s trap loop, rnd() in main.c, and NTRAPS /
    T_DOOR in rogue.h; and be_trapped() in move.c for where F_SEEN
    arrives afterwards. game/newlevel.go does the same
    Clear(FReal) / |= PlaceFlags(g.rnd(NumTrapTypes)).

No test or game code changed. The showMap test was already written to pin
only what both languages agree on, so the corrected analysis does not disturb
it; the only test-file edit is the comment above it, plus renaming
TestShowMapLeavesTheRowsCOmits to TestShowMapLoopBoundsMatchC (the old name
parsed as "C Omits"). make check green again after the edits.

## Correction: my `show_map` C analysis above was wrong This corrects the two comments above (https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50493 and https://git.eeqj.de/sneak/rgoue/issues/7#issuecomment-50707), which are left in place. It was caught in review of https://git.eeqj.de/sneak/rgoue/pulls/38 and is now fixed in the commit message, `TODO.md`, the PR body and the `showMap` test's comment at `6f409bd`. **The divergence is real and unchanged.** C tests `!(real &amp; F_REAL)` before the draw and `!real` — the whole flag word — after; `game/wizard.go` tests `isReal` on both sides. What I got wrong was the enumeration of squares that lose `F_REAL`, and therefore the consequence. I claimed two sites and said every such square keeps other bits, so `real` is never zero and C latches standout on at the first secret square and never turns it off, reversing the rest of the map. There are **three** sites — grepping `~F_REAL` across every `.c` file finds exactly three hits — and two of them leave the flag word at zero: 1. **`passages.c putpass()`** — `pp-&gt;p_flags |= F_PASS` runs first, so clearing `F_REAL` leaves `0x80`. Non-zero. This one I had right. 2. **`passages.c door()`, the secret-door arm** — I omitted it entirely. It clears `F_REAL` on a room-wall exit whose flags are still exactly `F_REAL`: `new_level` seeds 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. **The `new_level.c` trap loop** — `*sp &amp;= ~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` in. My "a non-zero `rnd(NTRAPS)`" was wrong one time in eight. **Corrected behaviour:** 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. That is **intermittent bands of reverse video, not a permanently reversed map** — a materially milder defect than I described, which matters because that severity is the input to https://git.eeqj.de/sneak/rgoue/issues/39. Also withdrawn: the commit message's "a square whose whole flag word is zero, which no secret square has". Two of the three sites produce exactly that. **How I verified it, first-hand rather than from the review.** All reads via `git show origin/c-master:&lt;file&gt;`, nothing checked out. - Site enumeration: `~F_REAL` over every `.c` file in `origin/c-master` returns three hits and no others — `new_level.c`, and `passages.c` twice. - Site 1: read `putpass()` in full; the `|= F_PASS` precedes the gate. - Site 2: read `door()` in full, plus `conn()` for the call order and `rooms.c` for what touches `p_flags` (nothing — the only `p_flags` reference there is a read of `F_PASS` in the maze code, and `door()` returns early for `ISMAZE` rooms anyway). Compared against `game/passages.go`'s `door`, which matches line for line. - Site 3: read `new_level()`'s trap loop, `rnd()` in `main.c`, and `NTRAPS` / `T_DOOR` in `rogue.h`; and `be_trapped()` in `move.c` for where `F_SEEN` arrives afterwards. `game/newlevel.go` does the same `Clear(FReal)` / `|= PlaceFlags(g.rnd(NumTrapTypes))`. **No test or game code changed.** The `showMap` test was already written to pin only what both languages agree on, so the corrected analysis does not disturb it; the only test-file edit is the comment above it, plus renaming `TestShowMapLeavesTheRowsCOmits` to `TestShowMapLoopBoundsMatchC` (the old name parsed as "C Omits"). `make check` green again after the edits.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#7