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')+10wraps ('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.
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.
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.
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.
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-31overstates the failure rangeThe guard comment says
'a'-'f'"indexed straight past the ends of theper-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-entrykinds (wands, potions, rings) overshoot, and only at
'e'(14) and'f'(15). The comment also mildly contradicts
game/wizard_test.go:67.2.
setKnowline reference is off by twoPR #20's body cites
setKnowatgame/wizard.go:208; the actual unguardedreads are at
:210and:212. Nothing to fix in code — but if any commentcarries that reference, correct it.
3.
MEMORY.md:32hard-codes a test count that is already staleIt claims
t.Parallel()is present "in all 32 tests". PR #20 added more, sothe number was wrong the moment that merged.
Do not simply increment it. The number is the defect, not its value —
MEMORY.mdacquired a fresh false claim within hours of #3 landingspecifically to purge false claims from it. Replace it with something that
cannot rot: state that every test calls
t.Parallel()and thatparalleltestenforces this, with no count at all.While there, sweep
MEMORY.md,TODO.md, andREADME.mdfor any otherhard-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
game/wizard.goguard comment accurately describes which kindsovershoot and at which characters, and no longer contradicts the test.
setKnowline reference in a committed comment is corrected.MEMORY.mdno longer states a test count; the claim is phrased so it staystrue as tests are added.
the answer is "nothing else found".
make checkfully green;make fmtrun for markdown.TODO.mdupdated in the same commit — Completed Steps entry, and do notrotate "Next Step".
(closes #N).Implementation requirements
wizard.gocomment.Note the trap that caused the original error:
readchar()returns abyte,so
int(ch-'a')+10wraps ('A'→ 234,'!'→ 202) and never goesnegative — and Go refuses to compile the constant form
(
byte('A') - 'a') while silently wrapping the runtime form. Do not reasonabout this from a constant expression in a scratch file; the compiler will
mislead you.
.gocomment, so it is not eligible for the docs-onlyfast path — it goes through normal adversarial review.
maketargets only. Do NOT modify.golangci.yml. No Dockerfile/CI/script/.c-masterandmodern-roguealone.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.
Adding a fifth item, from the PR #34 review (recorded there as a
non-blocking nit).
game/rings_test.go—handKeysappends to the caller's slice, andTestGethandpasses a table entry's backing array directly(
handKeys(tc.input...)). This is safe today only because every tableliteral happens to have
len == cap, soappendalways reallocates. Addone 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 relyingon 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:
mainhas moved a long way since this issue was filed (now at the merge oftest/rings-coverage). Find every item by content, not by the linenumbers quoted above —
TODO.mdandMEMORY.mdhave been reflowed bymake fmtseveral times, so those references are stale.MEMORY.mdstop hard-coding a test count. That is stillthe 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 sixth item, from the PR #35 review.
game/bolt_test.go— the bolt trail tests pin the erase loop, not thepaint. The reviewer probed it directly: deleting
g.mvaddch(pos.Y, pos.X, dirch)fromfireBoltentirely leaves thewhole suite green.
litCells/assertErasedpin thespotposgeometry, which is the partthat 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.
boltDirCharalready gives the expectedcharacter 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.mdCompleted 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:
mainhas 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.mdandMEMORY.mdhave been reflowed by
make fmtrepeatedly and every line reference here isstale.