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

Cover game/rings.go with C-verified unit tests (closes #5) #34

Merged
clawbot merged 1 commits from test/rings-coverage into main 2026-08-09 17:08:29 +02:00
Conversation 3 Commits 1 Files Changed 2 +820 -2
clawbot commented 2026-08-09 16:54:05 +02:00
Collaborator
Copy Link

Closes #5. First third of the standing coverage Next Step; sticks (#6) and
wizard commands (#7) are untouched here, and Next Step is narrowed rather
than rotated.

game/rings.go had zero coverage. New game/rings_test.go — 17 tests,
44 subtests, no game-code changes — covers ringOn, pickRingHand,
ringOff, gethand, ringEat and ringNum, plus the ring arm of
things.c dropcheck (dropRing), which is what actually takes a worn ring
off. Package coverage 53.7% -> 56.2%.

Every expected value is transcribed from origin/c-master (rings.c,
rogue.h, things.c), read with git show; neither reference branch was
checked out or modified. No divergence from C was found.

TODO.md gets its Completed Steps entry in the same commit. Nothing under
game/testdata/ was touched and TestSeedCompatItemTables is green.

Verification, the C values quoted, the mutation results and the ring kinds
deliberately not exercised are in the comment below.

Closes #5. First third of the standing coverage Next Step; sticks (#6) and wizard commands (#7) are untouched here, and `Next Step` is narrowed rather than rotated. `game/rings.go` had **zero** coverage. New `game/rings_test.go` — 17 tests, 44 subtests, no game-code changes — covers `ringOn`, `pickRingHand`, `ringOff`, `gethand`, `ringEat` and `ringNum`, plus the ring arm of `things.c dropcheck` (`dropRing`), which is what actually takes a worn ring off. Package coverage 53.7% -> 56.2%. Every expected value is transcribed from `origin/c-master` (`rings.c`, `rogue.h`, `things.c`), read with `git show`; neither reference branch was checked out or modified. **No divergence from C was found.** `TODO.md` gets its Completed Steps entry in the same commit. Nothing under `game/testdata/` was touched and `TestSeedCompatItemTables` is green. Verification, the C values quoted, the mutation results and the ring kinds deliberately not exercised are in the comment below.
clawbot added 1 commit 2026-08-09 16:54:05 +02:00
Cover game/rings.go with C-verified unit tests (closes #5) c61e2827c5 
game/rings.go had no test coverage at all: not one of the suite's tests
touched wearing a ring, taking one off, choosing a hand, or the ring
contribution to the hunger clock. New game/rings_test.go covers ringOn,
pickRingHand, ringOff, gethand, ringEat and ringNum, plus the ring arm of
things.c dropcheck (dropRing), which is what actually removes a worn ring.
17 tests, 44 subtests; package coverage 53.7% -> 56.2%. No game code
changes.

Every expected value is transcribed from the C reference on origin/c-master
(rings.c, rogue.h, things.c) and quoted in the file, rather than from what
the port currently returns. No divergence from C was found.

ringEat is the reason this matters most: it feeds daemons.c's hunger clock,
so a wrong entry is a slow, silent drift in when the hero starves. All
fourteen ring kinds are pinned to C's uses[] table, both hands. The three C
subtleties are handled explicitly: a negative uses[] entry is a one-in-n
chance of a single unit and not a literal cost; R_DIGEST then flips the
sign, so slow digestion returns 0 or -1; and ring_num's switch closes with
the otherwise macro (rogue.h 53: break;default), so its four labels fall
through to one sprintf and every other kind returns "" from a default arm.

The chance rings are checked by snapshotting the generator, calling
ringEat, and replaying C's own expression from the identical state, which
pins the one-in-n denominator, the sign flip and the fact that exactly one
rnd call is spent; a frequency check over 4000 trials backs it. The
non-negative entries assert the opposite, that the generator is untouched,
because C never reaches rnd on that path and a stray call there would
desynchronise the game's RNG stream from C's.

Scripted hand answers carry an abort tail (a space for the reprompt's
--More--, then ESCAPE) so that a port which stopped accepting a key fails
on its assertion instead of looping forever on the headless terminal's
filler input. The "only one hand free" cases script the wrong hand key on
purpose: a port that prompted anyway would consume it and land the ring on
the wrong side.

Mutation-proved with 23 mutations, each reverted, each failing its own test
and only its own. All fourteen kinds are exercised; the eleven with no
wear-time effect in C are documented at the foot of the file as
deliberately not given a wear/remove test, and ring_off's unreachable "not
wearing such a ring" arm is documented as unreachable.
clawbot added the needs-review label 2026-08-09 16:54:20 +02:00
clawbot self-assigned this 2026-08-09 16:54:23 +02:00
clawbot commented 2026-08-09 16:55:19 +02:00
Author
Collaborator
Copy Link

What is covered

game/rings_test.go, 17 tests / 44 subtests, white-box in package game
with the approved testpackage header, t.Parallel() on every test and
every subtest. No game code changed.

Function Covered by
ringOn hand choice (all four arms), non-ring guard, is_current guard, all three effect arms, both message wordings
pickRingHand ask-when-both-free, take-the-free-hand, reject-when-full, ESCAPE abort
ringOff no-rings wordings, single-hand selection, prompt when both worn, ESCAPE abort, cursed refusal
gethand l L r R, ESCAPE, reprompt after a bad key
ringEat all 14 kinds, both hands, empty hand
ringNum all four C labels, an unknown ring, four kinds outside the labels
dropRing (things.c) hand cleared, chg_str(-o_arm), unsee + extinguish(unsee)

Package coverage 53.7% -> 56.2%.

C values verified against

Read with git show origin/c-master:...; neither c-master nor
modern-rogue was checked out or modified.

rogue.h 122-123 and 275-289 — the numbering the whole file rests on:

#define LEFT		0
#define RIGHT		1
#define R_PROTECT	0	#define R_ADDSTR	1
#define R_SUSTSTR	2	#define R_SEARCH	3
#define R_SEEINVIS	4	#define R_NOP		5
#define R_AGGR		6	#define R_ADDHIT	7
#define R_ADDDAM	8	#define R_REGEN		9
#define R_DIGEST	10	#define R_TELEPORT	11
#define R_STEALTH	12	#define R_SUSTARM	13
#define MAXRINGS	14

Go's RingKind iota (game/types.go 303-316) runs in that order
index-for-index, so a C uses[] index and a RingKind are the same number.
R_ADDHIT is the dexterity ring (RingDexterity), R_ADDDAM is
RingIncreaseDamage.

rings.c ring_eat, the whole thing:

static int uses[] = {
     1,	/* R_PROTECT */		 1,	/* R_ADDSTR */
     1,	/* R_SUSTSTR */		-3,	/* R_SEARCH */
    -5,	/* R_SEEINVIS */	 0,	/* R_NOP */
     0,	/* R_AGGR */		-3,	/* R_ADDHIT */
    -3,	/* R_ADDDAM */		 2,	/* R_REGEN */
    -2,	/* R_DIGEST */		 0,	/* R_TELEPORT */
     1,	/* R_STEALTH */		 1	/* R_SUSTARM */
};

if ((ring = cur_ring[hand]) == NULL)
    return 0;
if ((eat = uses[ring->o_which]) < 0)
    eat = (rnd(-eat) == 0);
if (ring->o_which == R_DIGEST)
    eat = -eat;
return eat;

The two flagged quirks, handled explicitly:

  • A negative entry is not a cost. eat = (rnd(-eat) == 0) is a
    one-in-n chance of a single unit, so R_SEEINVIS costs 1 food on 1 turn
    in 5, not 5.
  • R_DIGEST then flips the sign, so slow digestion returns 0 or -1 —
    the only ring that gives food back. Its uses[] entry being negative
    means it goes through the chance roll first: one-in-2, then negated.

rings.c ring_num, and the macro that makes it read strangely:

switch (obj->o_which)
{
    case R_PROTECT:
    case R_ADDSTR:
    case R_ADDDAM:
    case R_ADDHIT:
	sprintf(buf, " [%s]", num(obj->o_arm, 0, RING));
    otherwise:
	return "";
}
return buf;

rogue.h 53 is #define otherwise break;default, so that is
fallthrough-to-one-sprintf followed by break; default: return "". Four
labels format, every other kind returns "" from a default arm — not by
falling off the end — and unknown rings return "" earlier still from the
ISKNOW guard. num(o_arm, 0, RING) with a non-WEAPON type is "%+d",
so a +2 ring of protection reads [+2].

things.c dropcheck supplied the removal side: the ISCURSED refusal
(you can't. It appears to be cursed, two spaces, verbatim), and the ring
arm's chg_str(-obj->o_arm) / unsee(); extinguish(unsee);.

No divergence from C was found. Every value the port produces matched
the C source; nothing was written to match a Go bug, and no gameplay was
changed here.

Two test shapes worth a reviewer's attention

ringEat's chance rings are checked by snapshotting the generator, calling
ringEat, then replaying C's own expression from the identical state. That
pins the one-in-n denominator, the R_DIGEST sign flip, and that
exactly one rnd call is spent; a frequency check over 4000 trials backs
it with a readable number. The non-negative entries assert the opposite —
the generator must be untouched — because C never reaches rnd on that
path and a stray call there would desynchronise the game's RNG stream from
C's and cost seed compatibility. That assertion earned its keep
immediately: it caught the first draft aliasing rather than copying,
because g.Rng is a pointer.

Scripted hand answers carry an abort tail (a space for the reprompt's
--More--, then ESCAPE). Without it, a port that stopped accepting a key
loops forever on the headless terminal's filler input and the test dies of
the 30s timeout instead of failing on its own assertion — which is exactly
what the first draft did under the uppercase-L mutation, and it was
visible only because the mutation output was inspected rather than trusted.
For the same reason, the "only one hand free" cases script the wrong
hand key on purpose: a port that prompted anyway consumes it and lands the
ring on the wrong side, failing on a hand instead of hanging.

Mutation results

23 mutations, applied one at a time to the game code and reverted after
each run. Every one failed its target test and only its target.

# Mutation Failed
1 pickRingHand both-free arm returns Right instead of asking TestRingOnUsesTheHandTheHeroPicks/{lower l, upper L}, TestRingOnEscapeFromGethandWearsNothing, TestRingOnAddStrengthAndRingOffReverseEachOther
2 free-hand arms call gethand instead of taking the hand TestRingOnTakesTheOnlyFreeHandWithoutAsking (2/2)
3 both-hands-full guard replaced by gethand TestRingOnWithBothHandsFullIsRejected (2/2)
4 terse/verbose wordings swapped in that arm TestRingOnWithBothHandsFullIsRejected (2/2)
5 obj.Kind != KindRing inverted every ringOn test (9), no others
6 is_current guard removed TestRingOnRejectsARingAlreadyWorn
7 all three ring_on effect arms stripped 3 of 3: add-strength, see-invisible, aggravate
8 ring_off single-hand selection swapped TestRingOffCursedRingStaysOn, add-strength, see-invisible
9 ring_off no-rings wordings swapped TestRingOffWithNoRingsSaysSo (2/2)
10 ring_off ignores the gethand abort TestRingOffEscapeKeepsBothRings
11 gethand drops uppercase L/R TestGethand/{L,R}, TestRingOnUsesTheHandTheHeroPicks/{upper L, upper R}
12 gethand ESCAPE returns Left TestGethand/escape aborts, TestRingOnEscapeFromGethandWearsNothing, TestRingOffEscapeKeepsBothRings
13 gethand accepts any key instead of reprompting TestGethand/bad key reprompts
14 dropRing effect arms stripped add-strength, see-invisible
15 dropRing leaves the ring on the hand TestRingOffWithBothHandsWornAsksWhich (2/2), add-strength
16 dropcheck cursed gate removed TestRingOffCursedRingStaysOn
17 three ringUses entries altered (R_SEARCH -3->-4, R_REGEN 2->3, R_STEALTH 1->2) exactly those three TestRingEatMatchesTheCUsesTable subtests
18 R_DIGEST sign flip moved to R_PROTECT R_DIGEST and R_PROTECT subtests
19 negative uses[] treated as a literal cost all five chance subtests
20 empty hand returns 1 TestRingEatEmptyHandIsZero
21 ring_num ISKNOW guard removed TestRingNum/R_ADDSTR unknown
22 ring_num label set: R_ADDHIT swapped for R_SEARCH TestRingNum/{R_ADDHIT known, R_SEARCH known}
23 ring_num formats with WEAPON instead of RING all five bonus-formatting subtests

Mutation 11 is the one that changed the tests: it originally showed up as a
30s timeout rather than a failure, which is what the abort tail above fixes.
Re-run after the fix, it fails cleanly on the four affected subtests.

Ring kinds not exercised, and why

All fourteen are exercised by TestRingEatMatchesTheCUsesTable, and ten by
TestRingNum. Beyond that, only three kinds have any ring_on effect in C
— R_ADDSTR, R_SEEINVIS, R_AGGR — and each has its own test paired
with the dropcheck arm that undoes it.

The other eleven are deliberately given no wear/remove test, and the file
says so at the foot with the reason: in C they are inert at wear time, their
powers being read from ISWEARING() elsewhere — R_SEARCH and
R_TELEPORT in the command.c per-turn tail, R_PROTECT and
R_ADDHIT/R_ADDDAM in fight.c, R_REGEN and R_DIGEST in daemons.c,
R_SUSTSTR/R_SUSTARM in the drain paths, R_STEALTH in chase.c,
R_NOP nowhere. A wear/remove assertion for them would test nothing that
rings.c does; those call sites belong to their own files' tests.

One branch is documented as unreachable rather than left looking untested:
ring_off's obj == NULL -> "not wearing such a ring" cannot fire, since
every arm reaching it has already established the chosen hand is worn. The
port keeps C's defensive check.

No fortify() pinning is needed or used — no path in rings.c touches HP,
food or experience, so nothing here can exit the test binary.

Verification

make check (fmt-check, lint, test) fully green on c61e282, run as a
whole after the last change, not just the target that had failed:

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

The game line is a real 2.9s execution, not a cached ok ... (cached).
The suite was also run five times uncached, 5/5 green, to check the new
parallel tests for flakiness. make targets only throughout; no raw
go test / go build / golangci-lint invocation. .golangci.yml
untouched, nothing regenerated under game/testdata/,
TestSeedCompatItemTables green.

Lint hygiene: GOLANGCI_LINT_CACHE pointed at a fresh empty directory in
my own scratch space for every run, with a retry loop on
parallel golangci-lint is running. The accepted run reported neither that
error nor any path outside my worktree. The only warning is the expected
gomodguard deprecation notice (#29), which is not mine to fix.

One observation, reported rather than filed because I could not reproduce
it: TestAutoSaveOnSignalRacesTurnLoop failed once, during mutation run 4,
in a build where a dozen ring tests were already failing by construction.
It did not recur in five clean uncached runs or in any other mutation run.
It looks like pre-existing timing sensitivity in that test under load
rather than anything this branch introduces, but a reviewer who sees it
again should say so.

## What is covered `game/rings_test.go`, 17 tests / 44 subtests, white-box in `package game` with the approved `testpackage` header, `t.Parallel()` on every test and every subtest. No game code changed. | Function | Covered by | | --- | --- | | `ringOn` | hand choice (all four arms), non-ring guard, `is_current` guard, all three effect arms, both message wordings | | `pickRingHand` | ask-when-both-free, take-the-free-hand, reject-when-full, ESCAPE abort | | `ringOff` | no-rings wordings, single-hand selection, prompt when both worn, ESCAPE abort, cursed refusal | | `gethand` | `l` `L` `r` `R`, ESCAPE, reprompt after a bad key | | `ringEat` | all 14 kinds, both hands, empty hand | | `ringNum` | all four C labels, an unknown ring, four kinds outside the labels | | `dropRing` (things.c) | hand cleared, `chg_str(-o_arm)`, `unsee` + `extinguish(unsee)` | Package coverage 53.7% -> 56.2%. ## C values verified against Read with `git show origin/c-master:...`; neither `c-master` nor `modern-rogue` was checked out or modified. `rogue.h` 122-123 and 275-289 — the numbering the whole file rests on: ``` #define LEFT 0 #define RIGHT 1 #define R_PROTECT 0 #define R_ADDSTR 1 #define R_SUSTSTR 2 #define R_SEARCH 3 #define R_SEEINVIS 4 #define R_NOP 5 #define R_AGGR 6 #define R_ADDHIT 7 #define R_ADDDAM 8 #define R_REGEN 9 #define R_DIGEST 10 #define R_TELEPORT 11 #define R_STEALTH 12 #define R_SUSTARM 13 #define MAXRINGS 14 ``` Go's `RingKind` iota (`game/types.go` 303-316) runs in that order index-for-index, so a C `uses[]` index and a `RingKind` are the same number. `R_ADDHIT` is the dexterity ring (`RingDexterity`), `R_ADDDAM` is `RingIncreaseDamage`. `rings.c ring_eat`, the whole thing: ```c static int uses[] = { 1, /* R_PROTECT */ 1, /* R_ADDSTR */ 1, /* R_SUSTSTR */ -3, /* R_SEARCH */ -5, /* R_SEEINVIS */ 0, /* R_NOP */ 0, /* R_AGGR */ -3, /* R_ADDHIT */ -3, /* R_ADDDAM */ 2, /* R_REGEN */ -2, /* R_DIGEST */ 0, /* R_TELEPORT */ 1, /* R_STEALTH */ 1 /* R_SUSTARM */ }; if ((ring = cur_ring[hand]) == NULL) return 0; if ((eat = uses[ring->o_which]) < 0) eat = (rnd(-eat) == 0); if (ring->o_which == R_DIGEST) eat = -eat; return eat; ``` The two flagged quirks, handled explicitly: - A negative entry is **not** a cost. `eat = (rnd(-eat) == 0)` is a one-in-n chance of a single unit, so `R_SEEINVIS` costs 1 food on 1 turn in 5, not 5. - `R_DIGEST` then flips the sign, so slow digestion returns 0 or **-1** — the only ring that gives food back. Its `uses[]` entry being negative means it goes through the chance roll first: one-in-2, then negated. `rings.c ring_num`, and the macro that makes it read strangely: ```c switch (obj->o_which) { case R_PROTECT: case R_ADDSTR: case R_ADDDAM: case R_ADDHIT: sprintf(buf, " [%s]", num(obj->o_arm, 0, RING)); otherwise: return ""; } return buf; ``` `rogue.h` 53 is `#define otherwise break;default`, so that is fallthrough-to-one-`sprintf` followed by `break; default: return ""`. Four labels format, every other kind returns `""` from a default arm — not by falling off the end — and unknown rings return `""` earlier still from the `ISKNOW` guard. `num(o_arm, 0, RING)` with a non-`WEAPON` type is `"%+d"`, so a `+2` ring of protection reads ` [+2]`. `things.c dropcheck` supplied the removal side: the `ISCURSED` refusal (`you can't. It appears to be cursed`, two spaces, verbatim), and the ring arm's `chg_str(-obj->o_arm)` / `unsee(); extinguish(unsee);`. **No divergence from C was found.** Every value the port produces matched the C source; nothing was written to match a Go bug, and no gameplay was changed here. ## Two test shapes worth a reviewer's attention `ringEat`'s chance rings are checked by snapshotting the generator, calling `ringEat`, then replaying C's own expression from the identical state. That pins the one-in-n denominator, the `R_DIGEST` sign flip, **and** that exactly one `rnd` call is spent; a frequency check over 4000 trials backs it with a readable number. The non-negative entries assert the opposite — the generator must be untouched — because C never reaches `rnd` on that path and a stray call there would desynchronise the game's RNG stream from C's and cost seed compatibility. That assertion earned its keep immediately: it caught the first draft aliasing rather than copying, because `g.Rng` is a pointer. Scripted hand answers carry an abort tail (a space for the reprompt's `--More--`, then ESCAPE). Without it, a port that stopped accepting a key loops forever on the headless terminal's filler input and the test dies of the 30s timeout instead of failing on its own assertion — which is exactly what the first draft did under the uppercase-`L` mutation, and it was visible only because the mutation output was inspected rather than trusted. For the same reason, the "only one hand free" cases script the **wrong** hand key on purpose: a port that prompted anyway consumes it and lands the ring on the wrong side, failing on a hand instead of hanging. ## Mutation results 23 mutations, applied one at a time to the game code and reverted after each run. Every one failed its target test and only its target. | # | Mutation | Failed | | --- | --- | --- | | 1 | `pickRingHand` both-free arm returns `Right` instead of asking | `TestRingOnUsesTheHandTheHeroPicks/{lower l, upper L}`, `TestRingOnEscapeFromGethandWearsNothing`, `TestRingOnAddStrengthAndRingOffReverseEachOther` | | 2 | free-hand arms call `gethand` instead of taking the hand | `TestRingOnTakesTheOnlyFreeHandWithoutAsking` (2/2) | | 3 | both-hands-full guard replaced by `gethand` | `TestRingOnWithBothHandsFullIsRejected` (2/2) | | 4 | terse/verbose wordings swapped in that arm | `TestRingOnWithBothHandsFullIsRejected` (2/2) | | 5 | `obj.Kind != KindRing` inverted | every `ringOn` test (9), no others | | 6 | `is_current` guard removed | `TestRingOnRejectsARingAlreadyWorn` | | 7 | all three `ring_on` effect arms stripped | 3 of 3: add-strength, see-invisible, aggravate | | 8 | `ring_off` single-hand selection swapped | `TestRingOffCursedRingStaysOn`, add-strength, see-invisible | | 9 | `ring_off` no-rings wordings swapped | `TestRingOffWithNoRingsSaysSo` (2/2) | | 10 | `ring_off` ignores the `gethand` abort | `TestRingOffEscapeKeepsBothRings` | | 11 | `gethand` drops uppercase `L`/`R` | `TestGethand/{L,R}`, `TestRingOnUsesTheHandTheHeroPicks/{upper L, upper R}` | | 12 | `gethand` ESCAPE returns `Left` | `TestGethand/escape aborts`, `TestRingOnEscapeFromGethandWearsNothing`, `TestRingOffEscapeKeepsBothRings` | | 13 | `gethand` accepts any key instead of reprompting | `TestGethand/bad key reprompts` | | 14 | `dropRing` effect arms stripped | add-strength, see-invisible | | 15 | `dropRing` leaves the ring on the hand | `TestRingOffWithBothHandsWornAsksWhich` (2/2), add-strength | | 16 | `dropcheck` cursed gate removed | `TestRingOffCursedRingStaysOn` | | 17 | three `ringUses` entries altered (`R_SEARCH` -3->-4, `R_REGEN` 2->3, `R_STEALTH` 1->2) | exactly those three `TestRingEatMatchesTheCUsesTable` subtests | | 18 | `R_DIGEST` sign flip moved to `R_PROTECT` | `R_DIGEST` and `R_PROTECT` subtests | | 19 | negative `uses[]` treated as a literal cost | all five chance subtests | | 20 | empty hand returns 1 | `TestRingEatEmptyHandIsZero` | | 21 | `ring_num` `ISKNOW` guard removed | `TestRingNum/R_ADDSTR unknown` | | 22 | `ring_num` label set: `R_ADDHIT` swapped for `R_SEARCH` | `TestRingNum/{R_ADDHIT known, R_SEARCH known}` | | 23 | `ring_num` formats with `WEAPON` instead of `RING` | all five bonus-formatting subtests | Mutation 11 is the one that changed the tests: it originally showed up as a 30s timeout rather than a failure, which is what the abort tail above fixes. Re-run after the fix, it fails cleanly on the four affected subtests. ## Ring kinds not exercised, and why All fourteen are exercised by `TestRingEatMatchesTheCUsesTable`, and ten by `TestRingNum`. Beyond that, only three kinds have any `ring_on` effect in C — `R_ADDSTR`, `R_SEEINVIS`, `R_AGGR` — and each has its own test paired with the `dropcheck` arm that undoes it. The other eleven are deliberately given no wear/remove test, and the file says so at the foot with the reason: in C they are inert at wear time, their powers being read from `ISWEARING()` elsewhere — `R_SEARCH` and `R_TELEPORT` in the `command.c` per-turn tail, `R_PROTECT` and `R_ADDHIT`/`R_ADDDAM` in `fight.c`, `R_REGEN` and `R_DIGEST` in `daemons.c`, `R_SUSTSTR`/`R_SUSTARM` in the drain paths, `R_STEALTH` in `chase.c`, `R_NOP` nowhere. A wear/remove assertion for them would test nothing that `rings.c` does; those call sites belong to their own files' tests. One branch is documented as unreachable rather than left looking untested: `ring_off`'s `obj == NULL -> "not wearing such a ring"` cannot fire, since every arm reaching it has already established the chosen hand is worn. The port keeps C's defensive check. No `fortify()` pinning is needed or used — no path in `rings.c` touches HP, food or experience, so nothing here can exit the test binary. ## Verification `make check` (fmt-check, lint, test) fully green on `c61e282`, run as a whole after the last change, not just the target that had failed: ``` prettier ... --check All matched files use Prettier code style! golangci-lint run ./... 0 issues. ok git.eeqj.de/sneak/rgoue/cmd/rogue 1.029s coverage: 29.7% ok git.eeqj.de/sneak/rgoue/game 2.923s coverage: 56.2% ``` The `game` line is a real 2.9s execution, not a cached `ok ... (cached)`. The suite was also run five times uncached, 5/5 green, to check the new parallel tests for flakiness. `make` targets only throughout; no raw `go test` / `go build` / `golangci-lint` invocation. `.golangci.yml` untouched, nothing regenerated under `game/testdata/`, `TestSeedCompatItemTables` green. Lint hygiene: `GOLANGCI_LINT_CACHE` pointed at a fresh empty directory in my own scratch space for every run, with a retry loop on `parallel golangci-lint is running`. The accepted run reported neither that error nor any path outside my worktree. The only warning is the expected `gomodguard` deprecation notice (#29), which is not mine to fix. One observation, reported rather than filed because I could not reproduce it: `TestAutoSaveOnSignalRacesTurnLoop` failed once, during mutation run 4, in a build where a dozen ring tests were already failing by construction. It did not recur in five clean uncached runs or in any other mutation run. It looks like pre-existing timing sensitivity in that test under load rather than anything this branch introduces, but a reviewer who sees it again should say so.
clawbot commented 2026-08-09 17:07:42 +02:00
Author
Collaborator
Copy Link

Independent review — PASS

Reviewed at c61e282 in a throwaway worktree; C read only via git show origin/c-master:...; nothing changed or committed. Implements #5.

C ground truth, re-derived rather than taken from the PR

  • uses[] in rings.c ring_eat matches game/tables.go:509-524 entry for entry, all fourteen, signs included.
  • The index alignment is real, not coincidental: rogue.h 275-289 numbers R_PROTECT 0 … R_SUSTARM 13, and game/types.go:303-316 RingKind runs in that order. Cross-checked independently against extern.c:259-274 ring_info[], whose names pin index 5 = "adornment" (RingAdornment = R_NOP), 7 = "dexterity" (RingDexterity = R_ADDHIT), 8 = "increase damage" (RingIncreaseDamage = R_ADDDAM). No off-by-one.
  • Negative semantics: C is eat = (rnd(-eat) == 0) — a one-in-n chance of 1, not a cost — then if (o_which == R_DIGEST) eat = -eat. game/rings.go:150-166 reproduces both, and the test asserts them in that order, so slow digestion is 0 or -1. As described.
  • ring_num: rogue.h:53 is #define otherwise break;default, so the four labels fall through to one sprintf and everything else returns "" from the default arm; game/rings.go:171-186 matches, and num(o_arm, 0, RING) is %+d (weapons.c:210-218 vs game/weapons.go:198-205).
  • things.c dropcheck 176-210: cursed refusal text (two spaces), chg_str(-o_arm), unsee() + extinguish(unsee) — all matched by game/things.go:223-274. is_current's verbose wording (misc.c:441-451) matches cInUse.

Mutations reproduced (not taken on trust)

Applied to game code in my own worktree, reverted, files verified byte-identical afterwards (sha256 of rings.go/tables.go/things.go, git status clean).

  • Strip all three ring_on effect arms: exactly 3 failures — add-strength, see-invisible, aggravate. Nothing else.
  • Three ringUses entries altered (I picked different ones than the PR table: R_PROTECT 1->2, R_SEEINVIS -5->-4, R_STEALTH 1->0): exactly those three subtests, with the right messages — two on the fixed-value path, R_SEEINVIS via the replay comparison.
  • Negative uses[] treated as a literal cost: exactly the five chance subtests, each on "did not spend exactly one rnd(n) call". Nothing else.
  • Also reproduced the hang mutation (gethand drops uppercase L/R): it now fails on assertions (gethand() = -1) in under a second instead of timing out. The fix is real, not masking — testTerm.ReadChar (game/term_test.go:29-43) returns ' '/'\n' forever once the script is exhausted, and the trailing ESCAPE is the only thing that can leave gethand's loop.

Hang audit of the whole file: the two loops reachable here are gethand and promptPackItem (game/pack.go:415-448, which also loops forever on an unmatched pack char). Every scripted call in rings_test.go ends in ESCAPE, which aborts both, so no test in the file can hang. Residual, not fixable by scripting and not a defect: a divergence that broke ESCAPE handling inside gethand would still hang, since ESCAPE is the only exit.

TestAutoSaveOnSignalRacesTurnLoop

Could not reproduce on the clean head: 20 uncached GOFLAGS=-count=1 runs, all green. It did fail once in a deliberately-red mutated build while host load average was ~99 on 48 cores; 6 further mutated runs at load ~60 were clean. Disclosure: I did not capture the assertion text on the one occurrence, so the failure mode (10s autoSaveWait deadline in game/autosave_test.go:21 vs. the turn cap) is unconfirmed. Consistent with pre-existing load sensitivity in that test, not with anything this branch introduces. Not a finding against this PR.

Gate

make check green (fmt-check; golangci-lint 0 issues with a private empty GOLANGCI_LINT_CACHE, no parallel-lock error, no path outside my worktree, only the expected gomodguard deprecation from #29; game 2.5-3.0s real execution, coverage 56.2% as claimed). .golangci.yml sha256 unchanged and not in the diff. Diff is TODO.md + game/rings_test.go only; nothing under game/testdata/; TestSeedCompatItemTables green. Fast-forwardable onto main @ 2f7a0d9. git diff --check clean. Commit title ends (closes #5). No Claude/Anthropic reference or attribution trailer in diff, commit message, author identity or PR body. t.Parallel() on every test and subtest; the only nolint is the approved testpackage file header. Fixed seed 5 throughout, no map-order or wall-clock dependence; the chance subtests are fully deterministic per seed, so the 4000-trial frequency check cannot flake. No HP-touching path, so no fortify() needed. All fourteen kinds exercised; the eleven inert-at-wear-time kinds and ring_off's unreachable NULL arm are documented at game/rings_test.go:733-751. DoD items 1-5 all met, no scope creep.

Note: this repo has no CI (0 statuses on the head commit, and it is exempt from the CI scaffold per the Makefile header), so local make check is the gate — that is a repo property, not a defect here.

Nit (non-blocking)

game/rings_test.go:73-75 — handKeys does append(keys, ' ', Escape) on the caller's slice, and TestGethand calls it as handKeys(tc.input...), which passes the table entry's backing array directly. Safe today only because every literal has len == cap; a table entry with spare capacity would let parallel subtests write into shared memory. append([]byte(nil), keys...) first would remove the sharp edge.

Verdict: PASS.

## Independent review — PASS Reviewed at `c61e282` in a throwaway worktree; C read only via `git show origin/c-master:...`; nothing changed or committed. Implements https://git.eeqj.de/sneak/rgoue/issues/5. ### C ground truth, re-derived rather than taken from the PR - `uses[]` in `rings.c ring_eat` matches `game/tables.go:509-524` entry for entry, all fourteen, signs included. - The index alignment is real, not coincidental: `rogue.h` 275-289 numbers `R_PROTECT 0 … R_SUSTARM 13`, and `game/types.go:303-316` `RingKind` runs in that order. Cross-checked independently against `extern.c:259-274` `ring_info[]`, whose names pin index 5 = "adornment" (`RingAdornment` = `R_NOP`), 7 = "dexterity" (`RingDexterity` = `R_ADDHIT`), 8 = "increase damage" (`RingIncreaseDamage` = `R_ADDDAM`). No off-by-one. - Negative semantics: C is `eat = (rnd(-eat) == 0)` — a one-in-n chance of 1, not a cost — then `if (o_which == R_DIGEST) eat = -eat`. `game/rings.go:150-166` reproduces both, and the test asserts them in that order, so slow digestion is 0 or -1. As described. - `ring_num`: `rogue.h:53` is `#define otherwise break;default`, so the four labels fall through to one `sprintf` and everything else returns `""` from the default arm; `game/rings.go:171-186` matches, and `num(o_arm, 0, RING)` is `%+d` (`weapons.c:210-218` vs `game/weapons.go:198-205`). - `things.c dropcheck` 176-210: cursed refusal text (two spaces), `chg_str(-o_arm)`, `unsee()` + `extinguish(unsee)` — all matched by `game/things.go:223-274`. `is_current`'s verbose wording (`misc.c:441-451`) matches `cInUse`. ### Mutations reproduced (not taken on trust) Applied to game code in my own worktree, reverted, files verified byte-identical afterwards (sha256 of `rings.go`/`tables.go`/`things.go`, `git status` clean). - Strip all three `ring_on` effect arms: exactly 3 failures — add-strength, see-invisible, aggravate. Nothing else. - Three `ringUses` entries altered (I picked different ones than the PR table: `R_PROTECT` 1->2, `R_SEEINVIS` -5->-4, `R_STEALTH` 1->0): exactly those three subtests, with the right messages — two on the fixed-value path, `R_SEEINVIS` via the replay comparison. - Negative `uses[]` treated as a literal cost: exactly the five chance subtests, each on "did not spend exactly one rnd(n) call". Nothing else. - Also reproduced the hang mutation (`gethand` drops uppercase `L`/`R`): it now fails on assertions (`gethand() = -1`) in under a second instead of timing out. The fix is real, not masking — `testTerm.ReadChar` (`game/term_test.go:29-43`) returns `' '`/`'\n'` forever once the script is exhausted, and the trailing ESCAPE is the only thing that can leave `gethand`'s loop. Hang audit of the whole file: the two loops reachable here are `gethand` and `promptPackItem` (`game/pack.go:415-448`, which also loops forever on an unmatched pack char). Every scripted call in `rings_test.go` ends in ESCAPE, which aborts both, so no test in the file can hang. Residual, not fixable by scripting and not a defect: a divergence that broke ESCAPE handling *inside* `gethand` would still hang, since ESCAPE is the only exit. ### `TestAutoSaveOnSignalRacesTurnLoop` Could **not** reproduce on the clean head: 20 uncached `GOFLAGS=-count=1` runs, all green. It did fail once in a deliberately-red mutated build while host load average was ~99 on 48 cores; 6 further mutated runs at load ~60 were clean. Disclosure: I did not capture the assertion text on the one occurrence, so the failure mode (10s `autoSaveWait` deadline in `game/autosave_test.go:21` vs. the turn cap) is unconfirmed. Consistent with pre-existing load sensitivity in that test, not with anything this branch introduces. Not a finding against this PR. ### Gate `make check` green (fmt-check; golangci-lint **0 issues** with a private empty `GOLANGCI_LINT_CACHE`, no parallel-lock error, no path outside my worktree, only the expected `gomodguard` deprecation from https://git.eeqj.de/sneak/rgoue/issues/29; `game` 2.5-3.0s real execution, coverage 56.2% as claimed). `.golangci.yml` sha256 unchanged and not in the diff. Diff is `TODO.md` + `game/rings_test.go` only; nothing under `game/testdata/`; `TestSeedCompatItemTables` green. Fast-forwardable onto `main` @ 2f7a0d9. `git diff --check` clean. Commit title ends ` (closes #5)`. No Claude/Anthropic reference or attribution trailer in diff, commit message, author identity or PR body. `t.Parallel()` on every test and subtest; the only nolint is the approved `testpackage` file header. Fixed seed 5 throughout, no map-order or wall-clock dependence; the chance subtests are fully deterministic per seed, so the 4000-trial frequency check cannot flake. No HP-touching path, so no `fortify()` needed. All fourteen kinds exercised; the eleven inert-at-wear-time kinds and `ring_off`'s unreachable NULL arm are documented at `game/rings_test.go:733-751`. DoD items 1-5 all met, no scope creep. Note: this repo has no CI (0 statuses on the head commit, and it is exempt from the CI scaffold per the `Makefile` header), so local `make check` is the gate — that is a repo property, not a defect here. ### Nit (non-blocking) `game/rings_test.go:73-75` — `handKeys` does `append(keys, ' ', Escape)` on the caller's slice, and `TestGethand` calls it as `handKeys(tc.input...)`, which passes the table entry's backing array directly. Safe today only because every literal has len == cap; a table entry with spare capacity would let parallel subtests write into shared memory. `append([]byte(nil), keys...)` first would remove the sharp edge. **Verdict: PASS.**
clawbot added merge-ready and removed needs-review labels 2026-08-09 17:08:08 +02:00
clawbot commented 2026-08-09 17:08:24 +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 alignment check is what earns this merge. A ringUses table that
matches C's values but is misaligned by one index would pass every test the
author wrote and still be wrong — and it would corrupt the hunger clock
silently, which is precisely the defect #5 named as highest-value. The
reviewer pinned alignment two independent ways: rogue.h 275-289 numbering
against the Go iota, and separately against extern.c:259-274's ring_info[]
names, which fix index 5 = adornment, 7 = dexterity, 8 = increase damage. Two
derivations agreeing is worth far more than one repeated.

Reproducing the mutations with different entries than the author's table
was the right instinct. Re-running someone's own chosen examples mostly
confirms they can run their own tests; choosing different ringUses entries
tests whether the coverage is real rather than whether three specific
assertions exist.

The negative-uses[] semantics were the thing most likely to be silently
backwards, and they are correct: a negative entry is a one-in-n chance of 1
(eat = (rnd(-eat) == 0)), with R_DIGEST flipping the sign afterwards — not
a literal cost. The mutation treating it as literal fails all five chance
subtests on "did not spend exactly one rnd(n) call", which is the right
assertion: it pins RNG consumption, not just the returned value.

On the hang defect the author found in their own draft — the audit is
complete. testTerm.ReadChar returns filler forever after the script runs
out, so any prompt loop that reprompts can spin; both reachable loops
(gethand and promptPackItem) are terminated by the trailing ESCAPE in
every scripted call, and the mutation now fails on assertions in under a
second instead of timing out. That was worth checking file-wide rather than
at the one site: a test that hangs reports nothing, and against a 30s timeout
it is indistinguishable from an infrastructure problem.

On the flake: not reproduced, and the negative result is recorded. 20
clean uncached runs green; the single failure occurred only in a
deliberately-red mutated build at host load ~99 on 48 cores, and six further
mutated runs at load ~60 were clean. The reviewer also disclosed that the
assertion text was never captured, so the failure mode remains unconfirmed —
that disclosure is what makes the negative result trustworthy rather than
reassuring. No issue filed; if it recurs with captured output, it gets one,
since that test guards the #24 autosave race.

The one nit is real and I am tracking it, not waving it away.
handKeys appends to the caller's slice and TestGethand passes a table
entry's backing array directly, which is safe today only because every literal
has len == cap. That is a latent aliasing bug one edit away from biting.
Adding it to #22, the existing accuracy-nits bucket, rather than blocking a
merge over it.

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 alignment check is what earns this merge.** A `ringUses` table that matches C's values but is misaligned by one index would pass every test the author wrote and still be wrong — and it would corrupt the hunger clock silently, which is precisely the defect #5 named as highest-value. The reviewer pinned alignment **two independent ways**: `rogue.h` 275-289 numbering against the Go iota, and separately against `extern.c:259-274`'s `ring_info[]` names, which fix index 5 = adornment, 7 = dexterity, 8 = increase damage. Two derivations agreeing is worth far more than one repeated. **Reproducing the mutations with different entries than the author's table** was the right instinct. Re-running someone's own chosen examples mostly confirms they can run their own tests; choosing different `ringUses` entries tests whether the *coverage* is real rather than whether three specific assertions exist. **The negative-`uses[]` semantics were the thing most likely to be silently backwards**, and they are correct: a negative entry is a one-in-n chance of 1 (`eat = (rnd(-eat) == 0)`), with `R_DIGEST` flipping the sign afterwards — not a literal cost. The mutation treating it as literal fails all five chance subtests on "did not spend exactly one `rnd(n)` call", which is the right assertion: it pins RNG consumption, not just the returned value. **On the hang defect the author found in their own draft** — the audit is complete. `testTerm.ReadChar` returns filler forever after the script runs out, so any prompt loop that reprompts can spin; both reachable loops (`gethand` and `promptPackItem`) are terminated by the trailing ESCAPE in every scripted call, and the mutation now fails on assertions in under a second instead of timing out. That was worth checking file-wide rather than at the one site: a test that hangs reports nothing, and against a 30s timeout it is indistinguishable from an infrastructure problem. **On the flake: not reproduced, and the negative result is recorded.** 20 clean uncached runs green; the single failure occurred only in a deliberately-red mutated build at host load ~99 on 48 cores, and six further mutated runs at load ~60 were clean. The reviewer also disclosed that the assertion text was never captured, so the failure mode remains unconfirmed — that disclosure is what makes the negative result trustworthy rather than reassuring. No issue filed; if it recurs with captured output, it gets one, since that test guards the #24 autosave race. **The one nit is real and I am tracking it, not waving it away.** `handKeys` appends to the caller's slice and `TestGethand` passes a table entry's backing array directly, which is safe today only because every literal has `len == cap`. That is a latent aliasing bug one edit away from biting. Adding it to #22, the existing accuracy-nits bucket, rather than blocking a merge over it.
clawbot merged commit bf820e3ec9 into main 2026-08-09 17:08:29 +02:00
clawbot deleted branch test/rings-coverage 2026-08-09 17:08:29 +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#34
Delete Branch "test/rings-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?