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.
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.
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
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.
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:
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
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_numISKNOW 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.
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-316RingKind runs in that order. Cross-checked independently against extern.c:259-274ring_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 insidegethand 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.**
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 main2026-08-09 17:08:29 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #5. First third of the standing coverage Next Step; sticks (#6) and
wizard commands (#7) are untouched here, and
Next Stepis narrowed ratherthan rotated.
game/rings.gohad zero coverage. Newgame/rings_test.go— 17 tests,44 subtests, no game-code changes — covers
ringOn,pickRingHand,ringOff,gethand,ringEatandringNum, plus the ring arm ofthings.c dropcheck(dropRing), which is what actually takes a worn ringoff. Package coverage 53.7% -> 56.2%.
Every expected value is transcribed from
origin/c-master(rings.c,rogue.h,things.c), read withgit show; neither reference branch waschecked out or modified. No divergence from C was found.
TODO.mdgets its Completed Steps entry in the same commit. Nothing undergame/testdata/was touched andTestSeedCompatItemTablesis green.Verification, the C values quoted, the mutation results and the ring kinds
deliberately not exercised are in the comment below.
What is covered
game/rings_test.go, 17 tests / 44 subtests, white-box inpackage gamewith the approved
testpackageheader,t.Parallel()on every test andevery subtest. No game code changed.
ringOnis_currentguard, all three effect arms, both message wordingspickRingHandringOffgethandlLrR, ESCAPE, reprompt after a bad keyringEatringNumdropRing(things.c)chg_str(-o_arm),unsee+extinguish(unsee)Package coverage 53.7% -> 56.2%.
C values verified against
Read with
git show origin/c-master:...; neitherc-masternormodern-roguewas checked out or modified.rogue.h122-123 and 275-289 — the numbering the whole file rests on:Go's
RingKindiota (game/types.go303-316) runs in that orderindex-for-index, so a C
uses[]index and aRingKindare the same number.R_ADDHITis the dexterity ring (RingDexterity),R_ADDDAMisRingIncreaseDamage.rings.c ring_eat, the whole thing:The two flagged quirks, handled explicitly:
eat = (rnd(-eat) == 0)is aone-in-n chance of a single unit, so
R_SEEINVIScosts 1 food on 1 turnin 5, not 5.
R_DIGESTthen flips the sign, so slow digestion returns 0 or -1 —the only ring that gives food back. Its
uses[]entry being negativemeans it goes through the chance roll first: one-in-2, then negated.
rings.c ring_num, and the macro that makes it read strangely:rogue.h53 is#define otherwise break;default, so that isfallthrough-to-one-
sprintffollowed bybreak; default: return "". Fourlabels format, every other kind returns
""from a default arm — not byfalling off the end — and unknown rings return
""earlier still from theISKNOWguard.num(o_arm, 0, RING)with a non-WEAPONtype is"%+d",so a
+2ring of protection reads[+2].things.c dropchecksupplied the removal side: theISCURSEDrefusal(
you can't. It appears to be cursed, two spaces, verbatim), and the ringarm'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, callingringEat, then replaying C's own expression from the identical state. Thatpins the one-in-n denominator, the
R_DIGESTsign flip, and thatexactly one
rndcall is spent; a frequency check over 4000 trials backsit with a readable number. The non-negative entries assert the opposite —
the generator must be untouched — because C never reaches
rndon thatpath 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.Rngis 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 keyloops 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-
Lmutation, and it wasvisible 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.
pickRingHandboth-free arm returnsRightinstead of askingTestRingOnUsesTheHandTheHeroPicks/{lower l, upper L},TestRingOnEscapeFromGethandWearsNothing,TestRingOnAddStrengthAndRingOffReverseEachOthergethandinstead of taking the handTestRingOnTakesTheOnlyFreeHandWithoutAsking(2/2)gethandTestRingOnWithBothHandsFullIsRejected(2/2)TestRingOnWithBothHandsFullIsRejected(2/2)obj.Kind != KindRinginvertedringOntest (9), no othersis_currentguard removedTestRingOnRejectsARingAlreadyWornring_oneffect arms strippedring_offsingle-hand selection swappedTestRingOffCursedRingStaysOn, add-strength, see-invisiblering_offno-rings wordings swappedTestRingOffWithNoRingsSaysSo(2/2)ring_offignores thegethandabortTestRingOffEscapeKeepsBothRingsgethanddrops uppercaseL/RTestGethand/{L,R},TestRingOnUsesTheHandTheHeroPicks/{upper L, upper R}gethandESCAPE returnsLeftTestGethand/escape aborts,TestRingOnEscapeFromGethandWearsNothing,TestRingOffEscapeKeepsBothRingsgethandaccepts any key instead of repromptingTestGethand/bad key repromptsdropRingeffect arms strippeddropRingleaves the ring on the handTestRingOffWithBothHandsWornAsksWhich(2/2), add-strengthdropcheckcursed gate removedTestRingOffCursedRingStaysOnringUsesentries altered (R_SEARCH-3->-4,R_REGEN2->3,R_STEALTH1->2)TestRingEatMatchesTheCUsesTablesubtestsR_DIGESTsign flip moved toR_PROTECTR_DIGESTandR_PROTECTsubtestsuses[]treated as a literal costTestRingEatEmptyHandIsZeroring_numISKNOWguard removedTestRingNum/R_ADDSTR unknownring_numlabel set:R_ADDHITswapped forR_SEARCHTestRingNum/{R_ADDHIT known, R_SEARCH known}ring_numformats withWEAPONinstead ofRINGMutation 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 byTestRingNum. Beyond that, only three kinds have anyring_oneffect in C—
R_ADDSTR,R_SEEINVIS,R_AGGR— and each has its own test pairedwith the
dropcheckarm 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_SEARCHandR_TELEPORTin thecommand.cper-turn tail,R_PROTECTandR_ADDHIT/R_ADDDAMinfight.c,R_REGENandR_DIGESTindaemons.c,R_SUSTSTR/R_SUSTARMin the drain paths,R_STEALTHinchase.c,R_NOPnowhere. A wear/remove assertion for them would test nothing thatrings.cdoes; those call sites belong to their own files' tests.One branch is documented as unreachable rather than left looking untested:
ring_off'sobj == NULL -> "not wearing such a ring"cannot fire, sinceevery 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 inrings.ctouches HP,food or experience, so nothing here can exit the test binary.
Verification
make check(fmt-check, lint, test) fully green onc61e282, run as awhole after the last change, not just the target that had failed:
The
gameline is a real 2.9s execution, not a cachedok ... (cached).The suite was also run five times uncached, 5/5 green, to check the new
parallel tests for flakiness.
maketargets only throughout; no rawgo test/go build/golangci-lintinvocation..golangci.ymluntouched, nothing regenerated under
game/testdata/,TestSeedCompatItemTablesgreen.Lint hygiene:
GOLANGCI_LINT_CACHEpointed at a fresh empty directory inmy own scratch space for every run, with a retry loop on
parallel golangci-lint is running. The accepted run reported neither thaterror nor any path outside my worktree. The only warning is the expected
gomodguarddeprecation notice (#29), which is not mine to fix.One observation, reported rather than filed because I could not reproduce
it:
TestAutoSaveOnSignalRacesTurnLoopfailed 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.
Independent review — PASS
Reviewed at
c61e282in a throwaway worktree; C read only viagit show origin/c-master:...; nothing changed or committed. Implements #5.C ground truth, re-derived rather than taken from the PR
uses[]inrings.c ring_eatmatchesgame/tables.go:509-524entry for entry, all fourteen, signs included.rogue.h275-289 numbersR_PROTECT 0 … R_SUSTARM 13, andgame/types.go:303-316RingKindruns in that order. Cross-checked independently againstextern.c:259-274ring_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.eat = (rnd(-eat) == 0)— a one-in-n chance of 1, not a cost — thenif (o_which == R_DIGEST) eat = -eat.game/rings.go:150-166reproduces both, and the test asserts them in that order, so slow digestion is 0 or -1. As described.ring_num:rogue.h:53is#define otherwise break;default, so the four labels fall through to onesprintfand everything else returns""from the default arm;game/rings.go:171-186matches, andnum(o_arm, 0, RING)is%+d(weapons.c:210-218vsgame/weapons.go:198-205).things.c dropcheck176-210: cursed refusal text (two spaces),chg_str(-o_arm),unsee()+extinguish(unsee)— all matched bygame/things.go:223-274.is_current's verbose wording (misc.c:441-451) matchescInUse.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 statusclean).ring_oneffect arms: exactly 3 failures — add-strength, see-invisible, aggravate. Nothing else.ringUsesentries altered (I picked different ones than the PR table:R_PROTECT1->2,R_SEEINVIS-5->-4,R_STEALTH1->0): exactly those three subtests, with the right messages — two on the fixed-value path,R_SEEINVISvia the replay comparison.uses[]treated as a literal cost: exactly the five chance subtests, each on "did not spend exactly one rnd(n) call". Nothing else.gethanddrops uppercaseL/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 leavegethand's loop.Hang audit of the whole file: the two loops reachable here are
gethandandpromptPackItem(game/pack.go:415-448, which also loops forever on an unmatched pack char). Every scripted call inrings_test.goends 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 insidegethandwould still hang, since ESCAPE is the only exit.TestAutoSaveOnSignalRacesTurnLoopCould not reproduce on the clean head: 20 uncached
GOFLAGS=-count=1runs, 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 (10sautoSaveWaitdeadline ingame/autosave_test.go:21vs. 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 checkgreen (fmt-check; golangci-lint 0 issues with a private emptyGOLANGCI_LINT_CACHE, no parallel-lock error, no path outside my worktree, only the expectedgomodguarddeprecation from #29;game2.5-3.0s real execution, coverage 56.2% as claimed)..golangci.ymlsha256 unchanged and not in the diff. Diff isTODO.md+game/rings_test.goonly; nothing undergame/testdata/;TestSeedCompatItemTablesgreen. Fast-forwardable ontomain@2f7a0d9.git diff --checkclean. 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 approvedtestpackagefile 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 nofortify()needed. All fourteen kinds exercised; the eleven inert-at-wear-time kinds andring_off's unreachable NULL arm are documented atgame/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
Makefileheader), so localmake checkis the gate — that is a repo property, not a defect here.Nit (non-blocking)
game/rings_test.go:73-75—handKeysdoesappend(keys, ' ', Escape)on the caller's slice, andTestGethandcalls it ashandKeys(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.
Manager notes (the review is in its own comment above).
Verdict accepted: PASS. Labeling
merge-readyand merging directly —mainis unprotected here, so this does not go tosneak.The alignment check is what earns this merge. A
ringUsestable thatmatches 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.h275-289 numberingagainst the Go iota, and separately against
extern.c:259-274'sring_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
ringUsesentriestests whether the coverage is real rather than whether three specific
assertions exist.
The negative-
uses[]semantics were the thing most likely to be silentlybackwards, and they are correct: a negative entry is a one-in-n chance of 1
(
eat = (rnd(-eat) == 0)), withR_DIGESTflipping the sign afterwards — nota 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 rightassertion: 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.ReadCharreturns filler forever after the script runsout, so any prompt loop that reprompts can spin; both reachable loops
(
gethandandpromptPackItem) are terminated by the trailing ESCAPE inevery 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.
handKeysappends to the caller's slice andTestGethandpasses a tableentry'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.