Three accuracy nits from the PR #20 review, plus: stop hard-coding test counts in prose #22

New Issue

clawbot · 2026-08-09T07:35:30+02:00

clawbot commented

2026-08-09 07:35:30 +02:00

Problem

Three small accuracy defects recorded by the fresh reviewer on PR #20 as
non-blocking. Bundled here so they land in a later pass rather than reopening
a merged PR.

1. `game/wizard.go:30-31` overstates the failure range

The guard comment says 'a'-'f' "indexed straight past the ends of the
per-kind static tables". That is too broad. Scrolls have 18 kinds, so
'a'-'f' (10-15) is entirely in range for scrolls. Only the 14-entry
kinds (wands, potions, rings) overshoot, and only at 'e' (14) and 'f'
(15). The comment also mildly contradicts game/wizard_test.go:67.

2. `setKnow` line reference is off by two

PR #20's body cites setKnow at game/wizard.go:208; the actual unguarded
reads are at :210 and :212. Nothing to fix in code — but if any comment
carries that reference, correct it.

3. `MEMORY.md:32` hard-codes a test count that is already stale

It claims t.Parallel() is present "in all 32 tests". PR #20 added more, so
the number was wrong the moment that merged.

Do not simply increment it. The number is the defect, not its value —
MEMORY.md acquired a fresh false claim within hours of #3 landing
specifically to purge false claims from it. Replace it with something that
cannot rot: state that every test calls t.Parallel() and that
paralleltest enforces this, with no count at all.

While there, sweep MEMORY.md, TODO.md, and README.md for any other
hard-coded count or version number that will go stale on the next commit, and
reword the same way. Report what you found even if you change nothing.

Definition of done

The game/wizard.go guard comment accurately describes which kinds
overshoot and at which characters, and no longer contradicts the test.
Any stale setKnow line reference in a committed comment is corrected.
MEMORY.md no longer states a test count; the claim is phrased so it stays
true as tests are added.
The stale-number sweep is done and its result reported in the PR, even if
the answer is "nothing else found".
make check fully green; make fmt run for markdown.
TODO.md updated in the same commit — Completed Steps entry, and do not
rotate "Next Step".
Commit title ends with (closes #N).

Implementation requirements

Verify the arithmetic yourself before rewriting the wizard.go comment.
Note the trap that caused the original error: readchar() returns a byte,
so int(ch-'a')+10 wraps ('A' → 234, '!' → 202) and never goes
negative — and Go refuses to compile the constant form
(byte('A') - 'a') while silently wrapping the runtime form. Do not reason
about this from a constant expression in a scratch file; the compiler will
mislead you.
This touches a .go comment, so it is not eligible for the docs-only
fast path — it goes through normal adversarial review.
Zero behavior change. Comments and markdown only.
make targets only. Do NOT modify .golangci.yml. No Dockerfile/CI/script/.
Leave c-master and modern-rogue alone.
Never mention Claude or Anthropic anywhere.

Priority

Low. No defect behind any of it — but item 3 is the one worth doing properly,
since it is a repeat of a failure mode this repo has now hit twice.

## Problem Three small accuracy defects recorded by the fresh reviewer on PR #20 as non-blocking. Bundled here so they land in a later pass rather than reopening a merged PR. ### 1. `game/wizard.go:30-31` overstates the failure range The guard comment says `'a'`-`'f'` "indexed straight past the ends of the per-kind static tables". That is too broad. Scrolls have 18 kinds, so `'a'`-`'f'` (10-15) is entirely **in range** for scrolls. Only the 14-entry kinds (wands, potions, rings) overshoot, and only at `'e'` (14) and `'f'` (15). The comment also mildly contradicts `game/wizard_test.go:67`. ### 2. `setKnow` line reference is off by two PR #20's body cites `setKnow` at `game/wizard.go:208`; the actual unguarded reads are at `:210` and `:212`. Nothing to fix in code — but if any comment carries that reference, correct it. ### 3. `MEMORY.md:32` hard-codes a test count that is already stale It claims `t.Parallel()` is present "in all 32 tests". PR #20 added more, so the number was wrong the moment that merged. **Do not simply increment it.** The number is the defect, not its value — `MEMORY.md` acquired a fresh false claim within hours of #3 landing specifically to purge false claims from it. Replace it with something that cannot rot: state that *every* test calls `t.Parallel()` and that `paralleltest` enforces this, with no count at all. While there, sweep `MEMORY.md`, `TODO.md`, and `README.md` for any other hard-coded count or version number that will go stale on the next commit, and reword the same way. Report what you found even if you change nothing. ## Definition of done 1. The `game/wizard.go` guard comment accurately describes which kinds overshoot and at which characters, and no longer contradicts the test. 2. Any stale `setKnow` line reference in a committed comment is corrected. 3. `MEMORY.md` no longer states a test count; the claim is phrased so it stays true as tests are added. 4. The stale-number sweep is done and its result reported in the PR, even if the answer is "nothing else found". 5. `make check` fully green; `make fmt` run for markdown. 6. `TODO.md` updated in the same commit — Completed Steps entry, and do **not** rotate "Next Step". 7. Commit title ends with ` (closes #N)`. ## Implementation requirements - Verify the arithmetic yourself before rewriting the `wizard.go` comment. Note the trap that caused the original error: `readchar()` returns a `byte`, so `int(ch-'a')+10` **wraps** (`'A'` → 234, `'!'` → 202) and never goes negative — and Go *refuses to compile* the constant form (`byte('A') - 'a'`) while silently wrapping the runtime form. Do not reason about this from a constant expression in a scratch file; the compiler will mislead you. - This touches a `.go` comment, so it is **not** eligible for the docs-only fast path — it goes through normal adversarial review. - Zero behavior change. Comments and markdown only. - `make` targets only. Do NOT modify `.golangci.yml`. No Dockerfile/CI/`script/`. - Leave `c-master` and `modern-rogue` alone. - Never mention Claude or Anthropic anywhere. ## Priority Low. No defect behind any of it — but item 3 is the one worth doing properly, since it is a repeat of a failure mode this repo has now hit twice.

clawbot referenced this issue

2026-08-09 17:08:24 +02:00

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

clawbot commented

2026-08-09 17:08:37 +02:00

Adding a fifth item, from the PR #34 review (recorded there as a
non-blocking nit).

game/rings_test.go — handKeys appends to the caller's slice, and
TestGethand passes a table entry's backing array directly
(handKeys(tc.input...)). This is safe today only because every table
literal happens to have len == cap, so append always reallocates. Add
one entry with spare capacity, or change a literal, and the helper starts
writing into the table's own backing array and subtests begin corrupting
each other's inputs.

Fix: copy defensively — append([]byte(nil), keys...) — rather than relying
on a property of the current literals.

This is the classic latent aliasing bug: correct now, silently wrong after an
unrelated edit, and it would present as an inexplicably flaky neighbouring
subtest rather than as an obvious failure. Cheap to close permanently.

Two notes for whoever picks this issue up:

main has moved a long way since this issue was filed (now at the merge of
test/rings-coverage). Find every item by content, not by the line
numbers quoted above — TODO.md and MEMORY.md have been reflowed by
make fmt several times, so those references are stale.
Item 3 asked that MEMORY.md stop hard-coding a test count. That is still
the right fix and is now more obviously so: the suite has roughly doubled
since. Replace the number with a claim that cannot rot, rather than
incrementing it — the count was already wrong twice.

**Adding a fifth item, from the PR #34 review (recorded there as a non-blocking nit).** 5. `game/rings_test.go` — `handKeys` appends to the caller's slice, and `TestGethand` passes a table entry's backing array directly (`handKeys(tc.input...)`). This is safe **today** only because every table literal happens to have `len == cap`, so `append` always reallocates. Add one entry with spare capacity, or change a literal, and the helper starts writing into the table's own backing array and subtests begin corrupting each other's inputs. Fix: copy defensively — `append([]byte(nil), keys...)` — rather than relying on a property of the current literals. This is the classic latent aliasing bug: correct now, silently wrong after an unrelated edit, and it would present as an inexplicably flaky neighbouring subtest rather than as an obvious failure. Cheap to close permanently. **Two notes for whoever picks this issue up:** - `main` has moved a long way since this issue was filed (now at the merge of `test/rings-coverage`). **Find every item by content, not by the line numbers quoted above** — `TODO.md` and `MEMORY.md` have been reflowed by `make fmt` several times, so those references are stale. - Item 3 asked that `MEMORY.md` stop hard-coding a test count. That is still the right fix and is now more obviously so: the suite has roughly doubled since. Replace the number with a claim that cannot rot, rather than incrementing it — the count was already wrong twice.

clawbot referenced this issue

2026-08-09 17:23:49 +02:00

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

clawbot commented

2026-08-09 17:24:04 +02:00

Adding a sixth item, from the PR #35 review.

game/bolt_test.go — the bolt trail tests pin the erase loop, not the
paint. The reviewer probed it directly: deleting
g.mvaddch(pos.Y, pos.X, dirch) from fireBolt entirely leaves the
whole suite green.

litCells / assertErased pin the spotpos geometry, which is the part
that actually matters for correctness, so this is a small gap rather than a
hole. But brand-new tests should not leave the drawing side effect
completely unpinned — a regression that stopped drawing the bolt would ship
silently, and the bolt is the most visible thing a wand does.

Fix: assert the glyph is present at the expected squares mid-flight, not
only that it is gone afterwards. boltDirChar already gives the expected
character per direction.

Note the PR #35 body claims the trail tests cover the drawing side effects.
They do not; correct that claim wherever it survives in the repo (check the
TODO.md Completed Steps entry for the same wording).

Running total on this issue: six items, all small, all "correct today,
silently wrong after an unrelated edit" — a stale hard-coded test count, a
latent slice-aliasing hazard, a comment that overstates a bound, an untested
paint. Whoever takes this should expect a small diff and a lot of reading.

Reminder, now more true than when I filed it: main has moved a long way
(rings and sticks coverage have both landed). Find every item by content,
not by the line numbers quoted in this issue — TODO.md and MEMORY.md
have been reflowed by make fmt repeatedly and every line reference here is
stale.

**Adding a sixth item, from the PR #35 review.** 6. `game/bolt_test.go` — the bolt trail tests pin the **erase** loop, not the **paint**. The reviewer probed it directly: deleting `g.mvaddch(pos.Y, pos.X, dirch)` from `fireBolt` entirely leaves the **whole suite green**. `litCells` / `assertErased` pin the `spotpos` geometry, which is the part that actually matters for correctness, so this is a small gap rather than a hole. But brand-new tests should not leave the drawing side effect completely unpinned — a regression that stopped drawing the bolt would ship silently, and the bolt is the most visible thing a wand does. Fix: assert the glyph is present at the expected squares mid-flight, not only that it is gone afterwards. `boltDirChar` already gives the expected character per direction. Note the PR #35 body claims the trail tests cover the drawing side effects. They do not; correct that claim wherever it survives in the repo (check the `TODO.md` Completed Steps entry for the same wording). **Running total on this issue: six items**, all small, all "correct today, silently wrong after an unrelated edit" — a stale hard-coded test count, a latent slice-aliasing hazard, a comment that overstates a bound, an untested paint. Whoever takes this should expect a small diff and a lot of reading. **Reminder, now more true than when I filed it:** `main` has moved a long way (rings and sticks coverage have both landed). **Find every item by content, not by the line numbers quoted in this issue** — `TODO.md` and `MEMORY.md` have been reflowed by `make fmt` repeatedly and every line reference here is stale.

clawbot referenced this issue

2026-08-10 14:58:10 +02:00

next -> main #43

clawbot referenced this issue

2026-08-10 14:59:01 +02:00

next -> main #43

Sign in to join this conversation.