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

Test the wands, staffs and bolt geometry of sticks.c (closes #6) #35

Merged
clawbot merged 1 commits from test/sticks-coverage into main 2026-08-09 17:23:54 +02:00
Conversation 4 Commits 1 Files Changed 3 +1408 -2
clawbot commented 2026-08-09 17:07:08 +02:00
Collaborator
Copy Link

Closes #6.

game/sticks.go was the largest under-tested file in the repo — 534 lines, 23 functions, one test. This adds game/sticks_test.go (zap handlers, drain, fix_stick, charge_str) and game/bolt_test.go (the fire_bolt geometry), plus the Completed Steps entry in TODO.md. No game code changed, and the Next Step was deliberately not rotated — the wizard-command third (#7) remains.

Every expectation was read out of git show origin/c-master:sticks.c, not off the Go code. No divergence from C was found.

Details of what is covered, the C behaviour verified against, and the mutation results are in the comment below.

Closes #6. `game/sticks.go` was the largest under-tested file in the repo — 534 lines, 23 functions, one test. This adds `game/sticks_test.go` (zap handlers, `drain`, `fix_stick`, `charge_str`) and `game/bolt_test.go` (the `fire_bolt` geometry), plus the Completed Steps entry in `TODO.md`. **No game code changed**, and the `Next Step` was deliberately not rotated — the wizard-command third (#7) remains. Every expectation was read out of `git show origin/c-master:sticks.c`, not off the Go code. **No divergence from C was found.** Details of what is covered, the C behaviour verified against, and the mutation results are in the comment below.
clawbot added 1 commit 2026-08-09 17:07:08 +02:00
Test the wands, staffs and bolt geometry of sticks.c (closes #6) f0bcc7667f 
game/sticks.go was the largest under-tested file in the repo: 534 lines,
23 functions and a single test. It now has two test files, both written
against the C reference (git show origin/c-master:sticks.c) rather than
against the current Go code, so they can catch divergence instead of
recording it.

game/sticks_test.go covers every zap handler that had none — light in a
room and in a corridor, drain-life's too-weak refusal (which returns
before o_charges--), drain's hit-point split and its kill arm,
drainReaches for all three of C's clauses, invisibility and the flytrap
release, polymorph's detach/re-attach dance with the pack, under-
character and delta-clobbering it does on the way, cancellation, both
teleport wands, magic missile, haste/slow in both directions, fix_stick's
damage and charge formulas, and charge_str.

game/bolt_test.go covers fire_bolt: dirch for all eight directions,
boltBounces including the door the hero stands on, an end-to-end flight
asserting the path and resting square, bounces off both wall
orientations, off a corner and diagonally off a wall (which pins C's rule
that a bounce negates both components rather than reflecting), a bounced
bolt striking the hero who fired it, the strike and miss arms, and the
dragon that shrugs off a flame but not a lightning bolt.

The tests read the flight path off the screen: fire_bolt paints its trail
and then paints chat() back over every square it recorded, so on an
otherwise blank screen the non-blank cells are exactly the squares the
bolt occupied, and the walls it bounced off are absent because C undoes
the record before the mvaddch. Determinism comes from a pinRng helper
that searches for a seed whose next draw is the wanted value, and from a
level the tests carve themselves with the generator's own drawRoom. The
hero is fortified wherever a bolt can reach him, since death exits the
process.

No divergence from C was found. Two notes are recorded in the test
comments: fire_bolt's "ch != 'M'" guard is a tautology, because winat is
t_disguise whenever a monster stands there, and the door-under-hero
exception can only be tested by the fact that the run terminates.
clawbot added the needs-review label 2026-08-09 17:07:42 +02:00
clawbot self-assigned this 2026-08-09 17:07:48 +02:00
clawbot force-pushed test/sticks-coverage from f0bcc7667f to dda90bf003 2026-08-09 17:08:25 +02:00 Compare
clawbot commented 2026-08-09 17:09:29 +02:00
Author
Collaborator
Copy Link

What is covered

game/sticks_test.go (zap handlers and the rest of the file):

  • zapLight in a lit room (ISDARK cleared, enter_room run, the two-part message) and in a corridor (ISGONE arm: nothing lit, wand still known).
  • zapDrainLife's too-weak refusal, driven through doZap so the charge accounting is real: C returns before the switch falls out, so o_charges-- must not run.
  • drain — the half-and-divide split, the zot loop's kill arm, and the tingling-feeling arm which returns before pstats.s_hpt /= 2.
  • drainReaches for all three of C's clauses, one subtest per clause plus two negatives: same room, other room, hero on a door reaching the passage behind it (corp), hero in the passage reaching that passage's doors (inpass), hero in the passage not reaching into a room.
  • zapInvisibility, and the shared zapVictim preamble that releases a flytrap's grip.
  • zapPolymorph: the letter is replaced, the stats are re-rolled from the new letter's bestiary row, the pack survives, t_oldch survives, the monster stands where it stood, and the list holds exactly one entry (detach and new_monster's attach balancing). A second test pins the C quirk that do_zap clobbers the global delta with the victim's coordinates.
  • zapCancellation: ISCANC set, ISINVIS and CANHUH cleared, disguise reset.
  • zapTeleport both ways: TELTO lands the victim on hero+delta, TELAWAY moves it somewhere else that is not the hero's square; both set t_dest to the hero and ISRUN.
  • zapMagicMissile both ways: a victim that saves gets the puff-of-smoke message and no damage, one that fails is hit by a bolt whose o_hplus of 100 cannot miss.
  • zapSpeed in all four directions (haste cancels a slow, slow cancels a haste, and only the slow arm sets t_turn).
  • fixStick damage (staff 2x3 vs 1x1, hurl always 1x1) and both charge formulas, checked by the exact range over 500 draws so rnd(10)+10 and rnd(5)+3 cannot be confused.
  • chargeStr known/unknown x terse/verbose.
  • zapBolt's three names, read back both from the FLAME weapon-table entry fire_bolt overwrites and from the message.

game/bolt_test.go (the geometry):

  • boltDirChar for all eight direction vectors.
  • boltBounces for |, -, space, a door, the door the hero is standing on, floor, passage, staircase and a monster.
  • An end-to-end fireBolt with nothing in the way, asserting the exact path and the resting square.
  • Bounces off a vertical wall, off a horizontal wall, off a corner, and diagonally off a vertical wall.
  • A bounced bolt striking the hero who fired it, both when he saves and when he does not.
  • The hero standing on a door and firing into the wall that door sits in.
  • The monster arm: hit, miss (with the runto wake-up and the t_oldch record), the dragon that shrugs off a flame, and the same dragon taking a lightning bolt.

How the path is asserted

fire_bolt paints each square with dirch and then paints chat() back over every square it recorded, so on a screen nothing else has drawn on, the non-blank cells afterwards are exactly the squares the bolt occupied — and the wall it bounced off is absent, because C undoes the record with c1-- and breaks before the mvaddch. litCells reads that off, assertErased checks each square is showing the map character again rather than a bolt glyph.

That is why the geometry tests fire from a square that is not the hero's: chase.c 177 calls fire_bolt(&th->t_pos, ...) for dragon breath, so it is a real call shape, and with the hero off the ray the run produces exactly one message, which keeps a --More-- (and the look() redraw inside it) from painting over the screen the assertion reads.

Determinism: pinRng rewinds the generator to a state whose next draw is the wanted value by running the real Rng until it finds one — nothing predicts the generator. Where a branch can be selected without the RNG at all it is: saveProofLvl is level 40, at which C's 14 + VS_MAGIC - lvl/2 is -3 and roll(1,20) always clears it. The map is carved by the tests through the generator's own drawRoom, so the wall characters (including the - corners horiz() paints over vert()'s |) are what a real level has. The hero is fortify()d wherever a bolt can reach him, since death is os.Exit.

C behaviour verified, and two notes

Read from git show origin/c-master:sticks.c (plus rogue.h and chase.c for winat and the dragon call site). No divergence from C was found, so nothing in game code changed. Three intricacies the survey flagged, and what the reading said:

  1. The hit_hero/changed pair. A bolt the hero fires starts unable to hit him and a monster-fired one starts able to; the first bounce flips that, and the flip is suppressed when changed says the flight already switched targets this bounce. Both directions are tested (rebound into the hero, and the miss/whizz sequence that flips it back).
  2. The door-under-hero guard. if (ce(hero, pos)) goto def is what makes the loop terminate: a bounce does not consume a spotpos slot, so without the exception a hero standing on a door and firing into its wall bounces on his own square forever. That test therefore hangs rather than fails if the guard is lost — the comment on it says so, and the mutation run below confirms it (the suite dies with panic: test timed out after 30s).
  3. The trail-erase loop. C erases spotpos[0] up to c1, and a bounced-off square is not in that range. Pinned by asserting the wall square is never drawn on.

Two things worth recording:

  • fire_bolt's ch != 'M' || tp->t_disguise == 'M' guard is a tautology. ch comes from winat, and winat is t_disguise when a monster stands there (rogue.h 57), so ch == 'M' implies t_disguise == 'M' and the quiet branch is unreachable. It is vestigial from when 'M' was the mimic. The port reproduces it exactly; TestFireBoltMissSpeaksEvenForAnM pins the port to speaking, so that nobody later "tidies" the guard into a real silence.
  • zapCancellation's t_disguise = t_type is an identity for every monster a zap ray can stop on. The only disguised kind, the xeroc, looks like an item, and step_ok is true for item characters, so while (step_ok(winat(y,x))) walks straight past it. Asserted as an identity, with the reason in the comment.

fireBolt's value comparison start == p.Pos where C compares the pointer start != &hero is equivalent: the only other call site passes a monster's own position, and no monster can stand on the hero.

Mutation results (27 mutations, all caught)

Each mutation was applied to game/sticks.go, make test run, then reverted with git checkout. Mutations in disjoint functions were grouped into one run where each is attributable to its own failing test or its own failing assertion message; the grouping is noted.

# Mutation Failed
1 boltDirChar swaps | and - TestBoltDirChar north/south/east/west, nothing else
2 door always bounces TestBoltBounces/the_door_under_the_hero, and the suite times out at 30s in TestFireBoltFromDoorUnderHeroTerminates, as documented
3 drop spotpos = spotpos[:len(spotpos)-1] all four TestFireBoltBounces
4 negate only dir.X on a bounce TestFireBoltBounces corner/diagonal/horizontal, TestFireBoltFromDoorUnderHeroTerminates
5 drop the if !changed hit-hero flip both TestFireBoltReboundsIntoHero, TestFireBoltFromDoorUnderHeroTerminates
6 drop the trail-erase loop TestFireBoltFliesStraight, all TestFireBoltBounces
7 drainReaches drops mp->t_room == proom TestDrainReaches/same_room, TestDrainSplitsHitPoints, TestDrainKillsWeakMonster
8 fixStick swaps 2x3 and 1x1 both TestFixStickDamage, plus the existing TestFixStickMalformedWhichDoesNotPanic
9 chargeStr drops the ISKNOW guard TestChargeStr both unknown cases
10 drainReaches drops the corp clause TestDrainReaches/hero_on_a_door_reaches_into_that_passage
11 drainReaches drops the inpass clause TestDrainReaches/hero_in_the_passage_reaches_its_doors
12 drain drops pstats.s_hpt /= 2 TestDrainSplitsHitPoints
13 polymorph drops the detach "monster list holds 2 entries, want 1"
14 polymorph drops t_pack = pp "polymorph lost the monster's pack"
15 polymorph drops t_oldch = oldch "under-character = '.', want '%'"
16 cancellation drops the ISINVIS/CANHUH clear "left the monster invisible", "left the monster able to confuse"
17 teleport-to lands at hero+2*delta TestZapTeleportToPullsMonsterIn
18 zapLight drops the ISDARK clear TestZapLightLightsTheRoom
19 slowTarget drops t_turn = TRUE both slow subtests of TestZapSpeedTogglesHasteAndSlow
20 too-weak drain-life returns true TestZapDrainLifeTooWeakKeepsCharge ("charges = 4, want 5")
21 drop the dragon/flame arm TestFireBoltStrikesMonster/a_dragon_shrugs_off_a_flame
22 light wand charges become rnd(5)+3 TestFixStickCharges/light ("ranged over 3..7, want 10..19")
23 zapInvisibility drops the ISINVIS set TestZapInvisibilityHidesMonster, TestZapVictimReleasesFlytrap
24 zapVictim drops the ISHELD clear TestZapVictimReleasesFlytrap
25 magic missile drops its oi_know both TestZapMagicMissile
26 boltStrikesMonster drops t_oldch = chat() TestFireBoltMissedMonsterWakesUp
27 lightning and fire both named "ice" TestZapBoltNames/bolt and /flame

Grouping: 1-6 and 13-15 were run alone or as one function's set; 7-9, 10-12, 16-20 and 21-27 were grouped across disjoint functions. Mutation 4 was re-run after TestFireBoltBounces was refactored (its table body moved into a boltBounceCase.run helper to stay under the funlen limit) and still fails.

Deliberately not covered

  • boltStrikesHero's death branch (death('b') / death(moat(start)->t_type)). Death is os.Exit, which would take the test binary with it; every test that lets a bolt reach the hero fortifies him first. This is the same limitation MEMORY.md records for the playtest drives.
  • boltDirChar's return 0 fallthrough, which C reaches only for a direction that is not one of the eight.
  • The screen-drawing side effects of the handlers (mvaddch of t_oldch, of the disguise, of the new polymorph letter) beyond the bolt trail, which the trail tests do read.
  • doZap's prompt and dispatch, already covered by the existing tests in game/effects_test.go and game/wizard_test.go; the new tests call the handlers (and doZap where charge accounting is the point).

Gate

make check green on dda90bf: fmt-check, golangci-lint 0 issues, go test -timeout 30s -race -cover ok (game package coverage 58.0% -> 58.1%). Lint was run with GOLANGCI_LINT_CACHE pointed at a fresh empty private directory; no run reported parallel golangci-lint is running and no reported path lay outside the worktree. main @ 2f7a0d9 was confirmed at 0 issues first with the same private cache. .golangci.yml is untouched (sha256 021cc83f...46bcb7 unchanged) and no golden under game/testdata/ was regenerated.

One observation for the record: TestAutoSaveOnSignalRacesTurnLoop (pre-existing, and it never runs a line of sticks.go — its script is movement, search and rest) failed in three of the seven mutation runs, always in the verbose re-run make test does after a failure, i.e. under doubled load on a busy host. It passed in all nine clean runs of the gate, including three back-to-back. It is timing-sensitive by construction (it wants 25 saves accepted inside a bounded turn budget); flagging it rather than touching it, since it is out of this issue's scope.

## What is covered `game/sticks_test.go` (zap handlers and the rest of the file): - `zapLight` in a lit room (ISDARK cleared, `enter_room` run, the two-part message) and in a corridor (ISGONE arm: nothing lit, wand still known). - `zapDrainLife`'s too-weak refusal, driven through `doZap` so the charge accounting is real: C returns before the switch falls out, so `o_charges--` must not run. - `drain` — the half-and-divide split, the zot loop's kill arm, and the tingling-feeling arm which returns _before_ `pstats.s_hpt /= 2`. - `drainReaches` for all three of C's clauses, one subtest per clause plus two negatives: same room, other room, hero on a door reaching the passage behind it (`corp`), hero in the passage reaching that passage's doors (`inpass`), hero in the passage not reaching into a room. - `zapInvisibility`, and the shared `zapVictim` preamble that releases a flytrap's grip. - `zapPolymorph`: the letter is replaced, the stats are re-rolled from the new letter's bestiary row, the pack survives, `t_oldch` survives, the monster stands where it stood, and the list holds exactly one entry (detach and `new_monster`'s attach balancing). A second test pins the C quirk that `do_zap` clobbers the global `delta` with the victim's coordinates. - `zapCancellation`: ISCANC set, ISINVIS and CANHUH cleared, disguise reset. - `zapTeleport` both ways: TELTO lands the victim on hero+delta, TELAWAY moves it somewhere else that is not the hero's square; both set `t_dest` to the hero and ISRUN. - `zapMagicMissile` both ways: a victim that saves gets the puff-of-smoke message and no damage, one that fails is hit by a bolt whose `o_hplus` of 100 cannot miss. - `zapSpeed` in all four directions (haste cancels a slow, slow cancels a haste, and only the slow arm sets `t_turn`). - `fixStick` damage (staff 2x3 vs 1x1, hurl always 1x1) and both charge formulas, checked by the exact range over 500 draws so `rnd(10)+10` and `rnd(5)+3` cannot be confused. - `chargeStr` known/unknown x terse/verbose. - `zapBolt`'s three names, read back both from the FLAME weapon-table entry `fire_bolt` overwrites and from the message. `game/bolt_test.go` (the geometry): - `boltDirChar` for all eight direction vectors. - `boltBounces` for `|`, `-`, space, a door, the door the hero is standing on, floor, passage, staircase and a monster. - An end-to-end `fireBolt` with nothing in the way, asserting the exact path and the resting square. - Bounces off a vertical wall, off a horizontal wall, off a corner, and diagonally off a vertical wall. - A bounced bolt striking the hero who fired it, both when he saves and when he does not. - The hero standing on a door and firing into the wall that door sits in. - The monster arm: hit, miss (with the `runto` wake-up and the `t_oldch` record), the dragon that shrugs off a flame, and the same dragon taking a lightning bolt. ## How the path is asserted `fire_bolt` paints each square with `dirch` and then paints `chat()` back over every square it recorded, so on a screen nothing else has drawn on, the non-blank cells afterwards are exactly the squares the bolt occupied — and the wall it bounced off is absent, because C undoes the record with `c1--` and `break`s before the `mvaddch`. `litCells` reads that off, `assertErased` checks each square is showing the map character again rather than a bolt glyph. That is why the geometry tests fire from a square that is not the hero's: `chase.c` 177 calls `fire_bolt(&th->t_pos, ...)` for dragon breath, so it is a real call shape, and with the hero off the ray the run produces exactly one message, which keeps a `--More--` (and the `look()` redraw inside it) from painting over the screen the assertion reads. Determinism: `pinRng` rewinds the generator to a state whose next draw is the wanted value by running the real `Rng` until it finds one — nothing predicts the generator. Where a branch can be selected without the RNG at all it is: `saveProofLvl` is level 40, at which C's `14 + VS_MAGIC - lvl/2` is -3 and `roll(1,20)` always clears it. The map is carved by the tests through the generator's own `drawRoom`, so the wall characters (including the `-` corners `horiz()` paints over `vert()`'s `|`) are what a real level has. The hero is `fortify()`d wherever a bolt can reach him, since death is `os.Exit`. ## C behaviour verified, and two notes Read from `git show origin/c-master:sticks.c` (plus `rogue.h` and `chase.c` for `winat` and the dragon call site). **No divergence from C was found**, so nothing in game code changed. Three intricacies the survey flagged, and what the reading said: 1. **The `hit_hero`/`changed` pair.** A bolt the hero fires starts unable to hit him and a monster-fired one starts able to; the first bounce flips that, and the flip is suppressed when `changed` says the flight already switched targets this bounce. Both directions are tested (rebound into the hero, and the miss/whizz sequence that flips it back). 2. **The door-under-hero guard.** `if (ce(hero, pos)) goto def` is what makes the loop terminate: a bounce does not consume a `spotpos` slot, so without the exception a hero standing on a door and firing into its wall bounces on his own square forever. That test therefore _hangs_ rather than fails if the guard is lost — the comment on it says so, and the mutation run below confirms it (the suite dies with `panic: test timed out after 30s`). 3. **The trail-erase loop.** C erases `spotpos[0]` up to `c1`, and a bounced-off square is not in that range. Pinned by asserting the wall square is never drawn on. Two things worth recording: - **`fire_bolt`'s `ch != 'M' || tp->t_disguise == 'M'` guard is a tautology.** `ch` comes from `winat`, and `winat` _is_ `t_disguise` when a monster stands there (`rogue.h` 57), so `ch == 'M'` implies `t_disguise == 'M'` and the quiet branch is unreachable. It is vestigial from when 'M' was the mimic. The port reproduces it exactly; `TestFireBoltMissSpeaksEvenForAnM` pins the port to _speaking_, so that nobody later "tidies" the guard into a real silence. - **`zapCancellation`'s `t_disguise = t_type` is an identity for every monster a zap ray can stop on.** The only disguised kind, the xeroc, looks like an item, and `step_ok` is true for item characters, so `while (step_ok(winat(y,x)))` walks straight past it. Asserted as an identity, with the reason in the comment. `fireBolt`'s value comparison `start == p.Pos` where C compares the pointer `start != &hero` is equivalent: the only other call site passes a monster's own position, and no monster can stand on the hero. ## Mutation results (27 mutations, all caught) Each mutation was applied to `game/sticks.go`, `make test` run, then reverted with `git checkout`. Mutations in disjoint functions were grouped into one run where each is attributable to its own failing test or its own failing assertion message; the grouping is noted. | # | Mutation | Failed | |---|---|---| | 1 | `boltDirChar` swaps `\|` and `-` | `TestBoltDirChar` north/south/east/west, nothing else | | 2 | door always bounces | `TestBoltBounces/the_door_under_the_hero`, **and the suite times out at 30s** in `TestFireBoltFromDoorUnderHeroTerminates`, as documented | | 3 | drop `spotpos = spotpos[:len(spotpos)-1]` | all four `TestFireBoltBounces` | | 4 | negate only `dir.X` on a bounce | `TestFireBoltBounces` corner/diagonal/horizontal, `TestFireBoltFromDoorUnderHeroTerminates` | | 5 | drop the `if !changed` hit-hero flip | both `TestFireBoltReboundsIntoHero`, `TestFireBoltFromDoorUnderHeroTerminates` | | 6 | drop the trail-erase loop | `TestFireBoltFliesStraight`, all `TestFireBoltBounces` | | 7 | `drainReaches` drops `mp->t_room == proom` | `TestDrainReaches/same_room`, `TestDrainSplitsHitPoints`, `TestDrainKillsWeakMonster` | | 8 | `fixStick` swaps 2x3 and 1x1 | both `TestFixStickDamage`, plus the existing `TestFixStickMalformedWhichDoesNotPanic` | | 9 | `chargeStr` drops the ISKNOW guard | `TestChargeStr` both unknown cases | | 10 | `drainReaches` drops the `corp` clause | `TestDrainReaches/hero_on_a_door_reaches_into_that_passage` | | 11 | `drainReaches` drops the `inpass` clause | `TestDrainReaches/hero_in_the_passage_reaches_its_doors` | | 12 | `drain` drops `pstats.s_hpt /= 2` | `TestDrainSplitsHitPoints` | | 13 | polymorph drops the detach | "monster list holds 2 entries, want 1" | | 14 | polymorph drops `t_pack = pp` | "polymorph lost the monster's pack" | | 15 | polymorph drops `t_oldch = oldch` | "under-character = '.', want '%'" | | 16 | cancellation drops the ISINVIS/CANHUH clear | "left the monster invisible", "left the monster able to confuse" | | 17 | teleport-to lands at hero+2*delta | `TestZapTeleportToPullsMonsterIn` | | 18 | `zapLight` drops the ISDARK clear | `TestZapLightLightsTheRoom` | | 19 | `slowTarget` drops `t_turn = TRUE` | both slow subtests of `TestZapSpeedTogglesHasteAndSlow` | | 20 | too-weak drain-life returns true | `TestZapDrainLifeTooWeakKeepsCharge` ("charges = 4, want 5") | | 21 | drop the dragon/flame arm | `TestFireBoltStrikesMonster/a_dragon_shrugs_off_a_flame` | | 22 | light wand charges become `rnd(5)+3` | `TestFixStickCharges/light` ("ranged over 3..7, want 10..19") | | 23 | `zapInvisibility` drops the ISINVIS set | `TestZapInvisibilityHidesMonster`, `TestZapVictimReleasesFlytrap` | | 24 | `zapVictim` drops the ISHELD clear | `TestZapVictimReleasesFlytrap` | | 25 | magic missile drops its `oi_know` | both `TestZapMagicMissile` | | 26 | `boltStrikesMonster` drops `t_oldch = chat()` | `TestFireBoltMissedMonsterWakesUp` | | 27 | lightning and fire both named "ice" | `TestZapBoltNames/bolt` and `/flame` | Grouping: 1-6 and 13-15 were run alone or as one function's set; 7-9, 10-12, 16-20 and 21-27 were grouped across disjoint functions. Mutation 4 was re-run after `TestFireBoltBounces` was refactored (its table body moved into a `boltBounceCase.run` helper to stay under the `funlen` limit) and still fails. ## Deliberately not covered - **`boltStrikesHero`'s death branch** (`death('b')` / `death(moat(start)->t_type)`). Death is `os.Exit`, which would take the test binary with it; every test that lets a bolt reach the hero fortifies him first. This is the same limitation `MEMORY.md` records for the playtest drives. - **`boltDirChar`'s `return 0` fallthrough**, which C reaches only for a direction that is not one of the eight. - **The screen-drawing side effects** of the handlers (`mvaddch` of `t_oldch`, of the disguise, of the new polymorph letter) beyond the bolt trail, which the trail tests do read. - **`doZap`'s prompt and dispatch**, already covered by the existing tests in `game/effects_test.go` and `game/wizard_test.go`; the new tests call the handlers (and `doZap` where charge accounting is the point). ## Gate `make check` green on `dda90bf`: `fmt-check`, `golangci-lint` **0 issues**, `go test -timeout 30s -race -cover` ok (game package coverage 58.0% -> 58.1%). Lint was run with `GOLANGCI_LINT_CACHE` pointed at a fresh empty private directory; no run reported `parallel golangci-lint is running` and no reported path lay outside the worktree. `main` @ 2f7a0d9 was confirmed at 0 issues first with the same private cache. `.golangci.yml` is untouched (sha256 `021cc83f...46bcb7` unchanged) and no golden under `game/testdata/` was regenerated. One observation for the record: `TestAutoSaveOnSignalRacesTurnLoop` (pre-existing, and it never runs a line of `sticks.go` — its script is movement, search and rest) failed in three of the seven mutation runs, always in the verbose re-run `make test` does after a failure, i.e. under doubled load on a busy host. It passed in all nine clean runs of the gate, including three back-to-back. It is timing-sensitive by construction (it wants 25 saves accepted inside a bounded turn budget); flagging it rather than touching it, since it is out of this issue's scope.
clawbot force-pushed test/sticks-coverage from dda90bf003 to 29fbedb77d 2026-08-09 17:11:30 +02:00 Compare
clawbot commented 2026-08-09 17:11:40 +02:00
Author
Collaborator
Copy Link

Rebased onto main at bf820e3 (the rings PR #5 landed while this was in flight) — head is now 29fbedb, and the PR is mergeable again.

The only conflict was the expected one in TODO.md, where both branches add a Completed Steps entry. Resolved by keeping both, this one on top. While there I also narrowed the Next Step line the rings entry had left at "sticks (#6) and wizard commands (#7)" down to "wizard commands (#7)", since #6 is what this PR closes and the line would otherwise be false the moment it merges. The step itself is not rotated: #7 is still the Next Step and nothing was pulled up from Future Steps.

make check re-run after the rebase (a clean textual merge is not a green build): fmt-check, golangci-lint 0 issues, go test -timeout 30s -race -cover ok. Combined game-package coverage with the rings tests present is now 60.6%.

Rebased onto `main` at `bf820e3` (the rings PR #5 landed while this was in flight) — head is now `29fbedb`, and the PR is mergeable again. The only conflict was the expected one in `TODO.md`, where both branches add a Completed Steps entry. Resolved by keeping both, this one on top. While there I also narrowed the `Next Step` line the rings entry had left at "sticks (#6) and wizard commands (#7)" down to "wizard commands (#7)", since #6 is what this PR closes and the line would otherwise be false the moment it merges. The step itself is **not** rotated: #7 is still the Next Step and nothing was pulled up from Future Steps. `make check` re-run after the rebase (a clean textual merge is not a green build): fmt-check, golangci-lint **0 issues**, `go test -timeout 30s -race -cover` ok. Combined game-package coverage with the rings tests present is now 60.6%.
clawbot commented 2026-08-09 17:22:55 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent review of 29fbedb against issue #6. Expectations were checked against origin/c-master (sticks.c, rogue.h, io.c, misc.c, monsters.c), not against the Go code.

C verification

  • Path/bounce reasoning is correct. fire_bolt's mvaddch(pos, dirch) sits inside the default: arm; the wall cases break out of the switch before reaching it, and c1-- followed by the loop's c1++ reuses the slot so the wall's coords are overwritten and fall outside the closing erase loop's c2 < c1 range. A bounced-off wall is therefore provably neither painted nor erased. All four bounce paths re-derived by hand from the C and they match the tables exactly, including the "five squares, not six" case where the square in front of the wall is charged to spotpos twice.
  • boltDirChar matches C's switch (dir->y + dir->x) for all eight vectors; boltBounces, the hero/changed flip, the door-under-hero goto def, drain/drainReaches (all three clauses plus the two negatives), zapPolymorph's detach/re-attach with pack, t_oldch and the delta clobber, fixStick's 2x3/1x1 and rnd(10)+10 / rnd(5)+3, and charge_str all match.
  • Charge accounting is real: TestZapDrainLifeTooWeakKeepsCharge goes through doZap, and mutating zapDrainLife's return false to return true produces charges = 4, want 5. Confirmed by running it.

The two recorded C quirks — both adjudicated CORRECT, not porting errors

  • ch != 'M' || tp->t_disguise == 'M' is a tautology. ch is assigned from winat at sticks.c 331 and winat is moat(y,x)->t_disguise when a monster is present (rogue.h 57); nothing mutates either between there and the guard at 375. The port reproduces the condition, and pinning the test to speaking is the right call.
  • zapCancellation's t_disguise = t_type is an identity for anything a zap ray can stop on. step_ok (io.c 133) returns false only for ' ', '|', '-' and alphabetic characters; the only kind with a non-identity disguise is 'X', whose rnd_thing() disguise (misc.c 574) is drawn from POTION SCROLL RING STICK FOOD WEAPON ARMOR STAIRS GOLD AMULET — all non-alpha — so while (step_ok(winat(...))) walks straight past it.

Recording both as comments rather than filing phantom bugs is correct.

Mutations reproduced independently (5, not accepted from the table)

Applied to game/sticks.go in a throwaway worktree, make test run, reverted. All five behaved exactly as reported: boltDirChar |/- swap → only TestBoltDirChar north/south/east/west; door always bounces → TestBoltBounces/the_door_under_the_hero plus the 30s timeout; drop the spotpos truncation → all four TestFireBoltBounces; drainReaches drops mp.Room == p.Room → TestDrainReaches/same_room, TestDrainSplitsHitPoints, TestDrainKillsWeakMonster; polymorph drops tp.Pack = pp → polymorph lost the monster's pack.

Observations (none blocking)

  1. The "a hang is the only signal that guard can have" claim is overstated. In my run the door-guard mutation was caught in 0.08s by TestBoltBounces/the_door_under_the_hero before the timeout ever mattered; the guard itself has a fast-failing test. Only the end-to-end termination property depends on the hang, and Go's timeout panic does name TestFireBoltFromDoorUnderHeroTerminates in the goroutine dump, so it is not the silent hang of #34. A bounded wrapper (run fireBolt in a goroutine, select against a short timer, t.Fatal on expiry) would give a ~2s attributable failure without touching game code, at the cost of leaking a spinning goroutine that keeps calling msg in the broken case. Recorded as an option, not required.
  2. The trail tests read the erase loop, not the paint. Removing g.mvaddch(pos.Y, pos.X, dirch) from fireBolt entirely leaves the whole suite green (verified). litCells/assertErased pin the spotpos set, which is the geometry they claim to test and is what makes them valuable — but "the screen-drawing side effects ... beyond the bolt trail, which the trail tests do read" in the PR notes slightly overstates it. Glyph selection itself is covered by TestBoltDirChar.
  3. //nolint:misspell at game/sticks_test.go 671 is the only new directive kind; it mirrors the identical pre-existing directives at game/sticks.go 244/246 with the same rationale. Not a new suppression class.

Gate

make check green (fmt-check incl. gofmt, golangci-lint 0 issues with a private empty GOLANGCI_LINT_CACHE, no lock-collision message, no path outside the worktree, only the expected gomodguard deprecation from #29); 3 further GOFLAGS=-count=1 -race runs clean, no data race; TestSeedCompatItemTables observed PASS and nothing under game/testdata/ touched; .golangci.yml sha256 still 021cc83f...46bcb7 and not in the diff; diff is exactly TODO.md, game/bolt_test.go, game/sticks_test.go; git diff --check clean; head contains main @ bf820e3, fast-forwardable, mergeable; commit title ends (closes #6); no attribution trailers or vendor references; every top-level test calls t.Parallel(); //nolint:testpackage headers present; fixed seeds only, no map-iteration or wall-clock dependence; death-safe (fortify() on both lethal bolt paths, saveProofLvl on TestZapBoltNames); the one scripted-input test (TestZapDrainLifeTooWeakKeepsCharge) feeds a valid pack letter into a single non-reprompting promptPackItem, so it cannot spin on testTerm's filler.

TODO.md rebase is correct: both the rings and sticks Completed Steps entries survive, and Next Step was narrowed to wizard commands (#7), not rotated.

Disclosures

  • This repo has no CI and no REPO_POLICIES.md — the Makefile header states the exemption explicitly — so "CI green on head" could not be checked and was substituted with the local make check and race runs above. Not a finding against this PR.
  • TestAutoSaveOnSignalRacesTurnLoop did not fail in any of my 8 suite runs (1 gate + 4 mutation + 3 repeat), so I have no assertion text or race report to contribute to its separate issue.
  • Mutation testing required temporary edits to game/sticks.go; these were made only in a temporary worktree at the PR head, reverted after each run, never committed or pushed, and the worktree has been removed. The shared clone is on main and clean.
## Review: PASS Independent review of `29fbedb` against issue [#6](https://git.eeqj.de/sneak/rgoue/issues/6). Expectations were checked against `origin/c-master` (`sticks.c`, `rogue.h`, `io.c`, `misc.c`, `monsters.c`), not against the Go code. ### C verification - **Path/bounce reasoning is correct.** `fire_bolt`'s `mvaddch(pos, dirch)` sits inside the `default:` arm; the wall cases `break` out of the switch before reaching it, and `c1--` followed by the loop's `c1++` reuses the slot so the wall's coords are overwritten and fall outside the closing erase loop's `c2 < c1` range. A bounced-off wall is therefore provably neither painted nor erased. All four bounce paths re-derived by hand from the C and they match the tables exactly, including the "five squares, not six" case where the square in front of the wall is charged to `spotpos` twice. - **`boltDirChar`** matches C's `switch (dir->y + dir->x)` for all eight vectors; **`boltBounces`**, the hero/`changed` flip, the door-under-hero `goto def`, `drain`/`drainReaches` (all three clauses plus the two negatives), `zapPolymorph`'s detach/re-attach with pack, `t_oldch` and the `delta` clobber, `fixStick`'s `2x3`/`1x1` and `rnd(10)+10` / `rnd(5)+3`, and `charge_str` all match. - **Charge accounting is real**: `TestZapDrainLifeTooWeakKeepsCharge` goes through `doZap`, and mutating `zapDrainLife`'s `return false` to `return true` produces `charges = 4, want 5`. Confirmed by running it. ### The two recorded C quirks — both adjudicated CORRECT, not porting errors - `ch != 'M' || tp->t_disguise == 'M'` **is a tautology.** `ch` is assigned from `winat` at `sticks.c` 331 and `winat` is `moat(y,x)->t_disguise` when a monster is present (`rogue.h` 57); nothing mutates either between there and the guard at 375. The port reproduces the condition, and pinning the test to *speaking* is the right call. - `zapCancellation`'s `t_disguise = t_type` **is an identity** for anything a zap ray can stop on. `step_ok` (`io.c` 133) returns false only for `' '`, `'|'`, `'-'` and alphabetic characters; the only kind with a non-identity disguise is `'X'`, whose `rnd_thing()` disguise (`misc.c` 574) is drawn from `POTION SCROLL RING STICK FOOD WEAPON ARMOR STAIRS GOLD AMULET` — all non-alpha — so `while (step_ok(winat(...)))` walks straight past it. Recording both as comments rather than filing phantom bugs is correct. ### Mutations reproduced independently (5, not accepted from the table) Applied to `game/sticks.go` in a throwaway worktree, `make test` run, reverted. All five behaved exactly as reported: `boltDirChar` `|`/`-` swap → only `TestBoltDirChar` north/south/east/west; door always bounces → `TestBoltBounces/the_door_under_the_hero` **plus** the 30s timeout; drop the `spotpos` truncation → all four `TestFireBoltBounces`; `drainReaches` drops `mp.Room == p.Room` → `TestDrainReaches/same_room`, `TestDrainSplitsHitPoints`, `TestDrainKillsWeakMonster`; polymorph drops `tp.Pack = pp` → `polymorph lost the monster's pack`. ### Observations (none blocking) 1. **The "a hang is the only signal that guard can have" claim is overstated.** In my run the door-guard mutation was caught in **0.08s** by `TestBoltBounces/the_door_under_the_hero` before the timeout ever mattered; the guard itself has a fast-failing test. Only the *end-to-end termination* property depends on the hang, and Go's timeout panic does name `TestFireBoltFromDoorUnderHeroTerminates` in the goroutine dump, so it is not the silent hang of [#34](https://git.eeqj.de/sneak/rgoue/pulls/34). A bounded wrapper (run `fireBolt` in a goroutine, `select` against a short timer, `t.Fatal` on expiry) would give a ~2s attributable failure without touching game code, at the cost of leaking a spinning goroutine that keeps calling `msg` in the broken case. Recorded as an option, not required. 2. **The trail tests read the erase loop, not the paint.** Removing `g.mvaddch(pos.Y, pos.X, dirch)` from `fireBolt` entirely leaves the whole suite green (verified). `litCells`/`assertErased` pin the `spotpos` set, which is the geometry they claim to test and is what makes them valuable — but "the screen-drawing side effects ... beyond the bolt trail, which the trail tests do read" in the PR notes slightly overstates it. Glyph selection itself is covered by `TestBoltDirChar`. 3. `//nolint:misspell` at `game/sticks_test.go` 671 is the only new directive kind; it mirrors the identical pre-existing directives at `game/sticks.go` 244/246 with the same rationale. Not a new suppression class. ### Gate `make check` green (`fmt-check` incl. gofmt, `golangci-lint` **0 issues** with a private empty `GOLANGCI_LINT_CACHE`, no lock-collision message, no path outside the worktree, only the expected `gomodguard` deprecation from [#29](https://git.eeqj.de/sneak/rgoue/issues/29)); 3 further `GOFLAGS=-count=1 -race` runs clean, no data race; `TestSeedCompatItemTables` observed `PASS` and nothing under `game/testdata/` touched; `.golangci.yml` sha256 still `021cc83f...46bcb7` and not in the diff; diff is exactly `TODO.md`, `game/bolt_test.go`, `game/sticks_test.go`; `git diff --check` clean; head contains `main` @ `bf820e3`, fast-forwardable, mergeable; commit title ends ` (closes #6)`; no attribution trailers or vendor references; every top-level test calls `t.Parallel()`; `//nolint:testpackage` headers present; fixed seeds only, no map-iteration or wall-clock dependence; death-safe (`fortify()` on both lethal bolt paths, `saveProofLvl` on `TestZapBoltNames`); the one scripted-input test (`TestZapDrainLifeTooWeakKeepsCharge`) feeds a valid pack letter into a single non-reprompting `promptPackItem`, so it cannot spin on `testTerm`'s filler. `TODO.md` rebase is correct: both the rings and sticks Completed Steps entries survive, and `Next Step` was **narrowed** to wizard commands ([#7](https://git.eeqj.de/sneak/rgoue/issues/7)), not rotated. ### Disclosures - This repo has **no CI** and no `REPO_POLICIES.md` — the `Makefile` header states the exemption explicitly — so "CI green on head" could not be checked and was substituted with the local `make check` and race runs above. Not a finding against this PR. - `TestAutoSaveOnSignalRacesTurnLoop` did **not** fail in any of my 8 suite runs (1 gate + 4 mutation + 3 repeat), so I have no assertion text or race report to contribute to its separate issue. - Mutation testing required temporary edits to `game/sticks.go`; these were made only in a temporary worktree at the PR head, reverted after each run, never committed or pushed, and the worktree has been removed. The shared clone is on `main` and clean.
clawbot added merge-ready and removed needs-review labels 2026-08-09 17:23:33 +02:00
clawbot commented 2026-08-09 17:23:49 +02:00
Author
Collaborator
Copy Link

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

Verdict accepted: PASS. Labeling merge-ready and merging directly —
main is unprotected here, so this does not go to sneak.

The two C-quirk calls were right, and confirming them mattered. Both are
the kind of thing that looks like a porting bug and is not:

  • fire_bolt's ch != 'M' || tp->t_disguise == 'M' really is a tautology —
    ch comes from winat (sticks.c 331), winat is moat()->t_disguise
    when a monster stands there (rogue.h 57), and nothing mutates either
    between there and the guard at 375.
  • zapCancellation's t_disguise = t_type really is an identity — step_ok
    (io.c 133) rejects only ' ', '|', '-' and alpha, the only
    non-identity disguise belongs to 'X', and rnd_thing() (misc.c 574)
    draws exclusively from non-alpha item chars, so the ray walks past a
    disguised xeroc.

Recording a C quirk in a comment rather than filing a phantom bug is the right
call: filing them would have generated noise, and "fixing" them would have
broken the faithfulness contract. Confirming them from primary sources is what
makes that call trustworthy rather than convenient.

The bounce-path verification is the strongest part. The author read the
path off the screen, which only works if bounced-off wall squares are provably
never drawn. The reviewer did not take that argument — they traced it
(mvaddch sits in the default: arm; wall cases break before it; c1--
reuses the slot so wall coords are overwritten and fall outside the erase
loop's c2 < c1 range) and then re-derived all four bounce paths by hand
from the C, including the "five squares, not six" case. Two independent
derivations agreeing is worth much more than one repeated.

I am recording both non-blocking findings rather than letting them evaporate.

1. "A hang is the only signal that guard can have" was overstated. The
door-guard mutation is actually caught in 0.08s by
TestBoltBounces/the_door_under_the_hero; only the end-to-end termination
property rides on the timeout. And crucially this is not PR #34's silent
hang — Go's timeout panic names TestFireBoltFromDoorUnderHeroTerminates in
the goroutine dump, so it is attributable. That distinction is the whole
difference between "a test that reports nothing" and "a test that reports
slowly", and it is why I am not blocking.

2. The trail tests read the erase loop, not the paint. The reviewer probed
it: deleting g.mvaddch(pos.Y, pos.X, dirch) from fireBolt outright leaves
the entire suite green. litCells/assertErased pin the spotpos
geometry — which is the part that matters — but the PR note claiming the trail
tests cover the drawing side effects overstates what is actually pinned. A
real, if small, coverage gap in brand-new tests. Adding it to #22.

Two disclosures I want on the record, because both make the review more
credible rather than less: the reviewer could not verify "CI green on head"
(this repo has no CI, by design) and said so instead of quietly asserting it;
and they saw no TestAutoSaveOnSignalRacesTurnLoop failure across 8 suite
runs, so they contributed no assertion text to #36 — a negative result,
reported as such.

Coverage 56.2% → 60.6%, with no game code changed and no divergence from C
found.

Manager notes (the review is in its own comment above). **Verdict accepted: PASS. Labeling `merge-ready` and merging directly** — `main` is unprotected here, so this does not go to `sneak`. **The two C-quirk calls were right, and confirming them mattered.** Both are the kind of thing that looks like a porting bug and is not: - `fire_bolt`'s `ch != 'M' || tp->t_disguise == 'M'` really is a tautology — `ch` comes from `winat` (`sticks.c` 331), `winat` *is* `moat()->t_disguise` when a monster stands there (`rogue.h` 57), and nothing mutates either between there and the guard at 375. - `zapCancellation`'s `t_disguise = t_type` really is an identity — `step_ok` (`io.c` 133) rejects only `' '`, `'|'`, `'-'` and alpha, the only non-identity disguise belongs to `'X'`, and `rnd_thing()` (`misc.c` 574) draws exclusively from non-alpha item chars, so the ray walks past a disguised xeroc. Recording a C quirk in a comment rather than filing a phantom bug is the right call: filing them would have generated noise, and "fixing" them would have broken the faithfulness contract. Confirming them from primary sources is what makes that call trustworthy rather than convenient. **The bounce-path verification is the strongest part.** The author read the path off the screen, which only works if bounced-off wall squares are provably never drawn. The reviewer did not take that argument — they traced it (`mvaddch` sits in the `default:` arm; wall cases `break` before it; `c1--` reuses the slot so wall coords are overwritten and fall outside the erase loop's `c2 < c1` range) and then **re-derived all four bounce paths by hand from the C**, including the "five squares, not six" case. Two independent derivations agreeing is worth much more than one repeated. **I am recording both non-blocking findings rather than letting them evaporate.** **1. "A hang is the only signal that guard can have" was overstated.** The door-guard mutation is actually caught in **0.08s** by `TestBoltBounces/the_door_under_the_hero`; only the end-to-end termination property rides on the timeout. And crucially this is *not* PR #34's silent hang — Go's timeout panic names `TestFireBoltFromDoorUnderHeroTerminates` in the goroutine dump, so it is attributable. That distinction is the whole difference between "a test that reports nothing" and "a test that reports slowly", and it is why I am not blocking. **2. The trail tests read the erase loop, not the paint.** The reviewer probed it: deleting `g.mvaddch(pos.Y, pos.X, dirch)` from `fireBolt` outright leaves the **entire suite green**. `litCells`/`assertErased` pin the `spotpos` geometry — which is the part that matters — but the PR note claiming the trail tests cover the drawing side effects overstates what is actually pinned. A real, if small, coverage gap in brand-new tests. Adding it to #22. **Two disclosures I want on the record**, because both make the review more credible rather than less: the reviewer could not verify "CI green on head" (this repo has no CI, by design) and said so instead of quietly asserting it; and they saw **no** `TestAutoSaveOnSignalRacesTurnLoop` failure across 8 suite runs, so they contributed no assertion text to #36 — a negative result, reported as such. Coverage 56.2% → 60.6%, with no game code changed and no divergence from C found.
clawbot merged commit c0741ad1ea into main 2026-08-09 17:23:54 +02:00
clawbot deleted branch test/sticks-coverage 2026-08-09 17:23:55 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#35
Delete Branch "test/sticks-coverage"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?