sneak
20cfb47912
The previous commit claimed --target and --no-cache-filter "validate each
other's magic string". They do not. --target validates only its own
argument; a typo confined to --no-cache-filter left the build green and
linting nothing:
docker build --target lint --no-cache-filter=lnit ...
#10 [lint 2/2] RUN golangci-lint run ... CACHED exit 0
Three of the four edit paths were caught and one was not, so the original
false green survived in the narrow case.
The duplication was the defect: the stage name appeared twice on one
command line and nothing tied the copies together. Correcting only the
prose would have left the hazard live and merely warned about, so the name
is now written once, as `stage=lint`, and passed to both flags. Divergence
is unrepresentable rather than documented — there is a single name to get
wrong, and --target rejects it loudly when it is not a stage in
Dockerfile.lint, which now covers the filter too because it is the same
string.
The comments in script/lint and Dockerfile.lint and the TODO.md entry drop
the false "validate each other" claim and state the real property, along
with the residual hazard that is genuinely unguarded: --target checks that
the name exists, not that it names the stage which actually runs
golangci-lint, and it stops the build there, so relocating the lint step
or appending a stage after it would go unnoticed.
926 lines
62 KiB
Markdown
926 lines
62 KiB
Markdown
# Workflow
|
||
|
||
- branch (from `main`)
|
||
- do the work in Next Step
|
||
- move Next Step to the top of Completed Steps
|
||
- move the top item of Future Steps into Next Step
|
||
- commit (`TODO.md` changes in the same commit as the work)
|
||
- merge to `main` if the branch is not protected, otherwise open a PR
|
||
- push
|
||
|
||
# Status
|
||
|
||
pre-1.0
|
||
|
||
The port on main is complete and faithful (function-by-function from Rogue 5.4.4
|
||
C; reference sources on c-master/modern-rogue). Current phase: refactor from a
|
||
transliterated port into idiomatic Go — one feature branch per step below,
|
||
descriptive naming, real types, house style per
|
||
~/dev/prompts/prompts/CODE_STYLEGUIDE_GO.md.
|
||
|
||
Refactor ground rules:
|
||
|
||
- Behavior must not change unless a step says so. The full test suite (scripted
|
||
sessions, generation invariants, C-compatible RNG goldens) gates every step;
|
||
80x24 seed-compatible gameplay stays intact.
|
||
- Renames keep the C lineage greppable: doc comments retain their "(file.c
|
||
func_name)" breadcrumbs, and the docs refresh step adds a C-name → Go-name
|
||
table to ARCHITECTURE.md.
|
||
|
||
# Next Step
|
||
|
||
Tag a release once a full game (Amulet retrieval and score entry) completes
|
||
without defects. Promoted from Future Steps now that the coverage step above it
|
||
is finished.
|
||
|
||
# Completed Steps
|
||
|
||
- 2026-08-10 Linting moved into a container
|
||
(https://git.eeqj.de/sneak/rgoue/issues/41). `golangci-lint` is no longer
|
||
invoked on the host anywhere in the repo: `Dockerfile.lint` pins
|
||
`golangci/golangci-lint:v2.12.2` by digest and runs the linter as a build
|
||
step, so a successful build is a clean lint, and `make lint` is now a shim
|
||
over `script/lint`. This is what killed the false green seen earlier, where a
|
||
branch that was genuinely red with a `goconst` finding reported `0 issues` off
|
||
the shared host cache; a container per run has its own cache and lock.
|
||
|
||
Two deliberate divergences from the `sneak/homoicon` reference. The image
|
||
has two stages rather than one — a cached `deps` stage holding
|
||
`go mod download`, then `FROM deps AS lint` with the copy and the lint run —
|
||
and `script/lint` builds with `--no-cache-filter=lint`. Caching of the lint
|
||
result is explicitly waived (a cached build lints nothing), and splitting
|
||
the stages means busting the lint layer does not also re-fetch the module
|
||
cache over the network on every run. And `golangci-lint config verify` is
|
||
left out: it resolves its JSON schema over a live, unpinned HTTPS call,
|
||
which is an unpinned network input inside the one step whose purpose is a
|
||
pinned reproducible gate, and a schema-host outage would surface as a red
|
||
build. `golangci-lint run` already fails on a malformed config.
|
||
|
||
Verified rather than assumed, since a green docker build is the classic
|
||
false green: two consecutive runs on an unchanged tree each showed the
|
||
`golangci-lint run` layer executing and reporting `0 issues.` while the
|
||
`deps` layers reported `CACHED`, and a deliberate `indent-error-flow`
|
||
violation failed the build naming that finding plus the `unused` one before
|
||
a revert went clean again. The durable property to check when touching any
|
||
of this is that the lint stage executes on every run and is never served
|
||
from cache; wall-clock durations vary per host and per run, so they are not
|
||
recorded here.
|
||
|
||
Hardened 2026-08-10 after review. `script/lint` now also passes `--target`
|
||
and `--output=type=cacheonly`. `--target` is what makes `--no-cache-filter`
|
||
trustworthy: BuildKit silently ignores the filter when no stage matches its
|
||
argument, so a rename or typo of the `lint` stage would have left the lint
|
||
layer cached and `script/lint` green having linted nothing — the same false
|
||
green in a new place. `--target` fails loudly on a name that is not in the
|
||
file. `--output=type=cacheonly` skips the image export: nothing consumes the
|
||
image (the deliverable is an exit code), and exporting it cost seconds per
|
||
run and left a dangling image behind each time.
|
||
|
||
Corrected 2026-08-10 after a second review, which was right to reject the
|
||
claim first made here that the two flags "validate each other". They did
|
||
not: `--target` validates only its own argument, so a typo confined to
|
||
`--no-cache-filter` still built `CACHED` at exit 0 — the original defect,
|
||
surviving in the narrow case. The duplicated stage name was the defect, so
|
||
it is now written once, as `stage=lint` in `script/lint`, and passed to both
|
||
flags. The true property is that there is only one name to get wrong, and
|
||
`--target` rejects it loudly if it is not a stage in `Dockerfile.lint`, so a
|
||
typo or a stale rename is a hard error rather than a silent skip. What
|
||
remains on the editor, and is not checked by anything: `$stage` must name
|
||
the stage that actually runs `golangci-lint`. `--target` verifies the name
|
||
exists, not that it is the right stage, and it stops the build there — so
|
||
moving the lint step to another stage, or adding a stage after it, would go
|
||
unnoticed.
|
||
|
||
- 2026-08-09 `TestAutoSaveOnSignalRacesTurnLoop` de-flaked at the cause
|
||
(`fix/autosave-turn-budget-36`, closes #36). The failure text was captured
|
||
before anything was changed and it is **not** a data race: the assertion was
|
||
`driveUntilDone`'s
|
||
`t.Fatal("the turn loop ran out of turns before the saves were taken")`, with
|
||
no `WARNING: DATA RACE` anywhere in the log. The handoff fixed in #24 was
|
||
working; the test's own drive loop was running out of its fixed 1000-turn
|
||
budget first.
|
||
|
||
Confirmed rather than taken on trust. Instrumenting the loop to report the
|
||
turns it actually used showed the count tracking scheduling pressure and
|
||
nothing else: about 60-120 turns at host load ~57 with the whole machine to
|
||
spread over, 418 at `GOMAXPROCS=4`, 539 and 655 at 2 and 1, and past 1000 —
|
||
the recorded failure — under the doubled load of the verbose rerun that the
|
||
test target performs after a failure. The turns between one save being
|
||
answered and the next request arriving are not work; they are the saving
|
||
goroutine's wake-up latency, so a fixed turn count is a wall-clock
|
||
assumption in disguise, which is why raising it would have hidden the flake
|
||
rather than fixed it.
|
||
|
||
So the budget is gone rather than larger. `driveUntilDone` now drives until
|
||
the saving goroutine finishes and nothing else. Termination is not lost, it
|
||
just belongs to the code under test instead of to the test: every
|
||
`AutoSaveOnSignal` returns within the timeout it is handed, so the saving
|
||
goroutine always finishes. A handoff that has stopped answering costs one
|
||
`autoSaveWait` in total — `g.sigSave` is one deep, so an unserviced request
|
||
stays in the channel and every later call finds it full and fails at once —
|
||
and the failure is then the real assertion (`saves taken = 0, want 25`)
|
||
instead of "out of turns". The worst case is not that one: a handoff that
|
||
drains each request but slower than `autoSaveWait` costs one timeout per
|
||
save, `wantSaves × autoSaveWait` = 250s, which would run past the 30s
|
||
package timeout instead of reaching the assertion. It takes ~10s of
|
||
scheduler starvation per save against a measured 0.12s per 1000 turns, so it
|
||
is remote, and the turn cap did not bound it either. The comment in the test
|
||
states that bound rather than the optimistic one.
|
||
|
||
Removing the cap exposed a second assumption underneath it, which is the
|
||
reason this is not a one-line diff. `testTerm` answers space and newline for
|
||
ever once its script is exhausted, and neither key takes a turn, so
|
||
`command()` — which loops until the player consumes one — never returns; the
|
||
old cap was silently sized to the script (4000 characters, two per turn,
|
||
against 1000 turns). An uncapped drive wedged inside a single `command()`
|
||
call. The two drive tests therefore use a new `driveTerm`, a headless
|
||
terminal whose script repeats. Repeating is necessary but not sufficient,
|
||
and the test says so: `' '` clears `After` outright and all eight movement
|
||
keys clear it on a refused step, so a script of only those keys wedges just
|
||
as `testTerm`'s tail did. What makes the wedge impossible is that the cycle
|
||
always holds an _unconditional_ turn-taker, and these scripts hold two —
|
||
`'.'` (empty handler) and `'s'` (`search`, which writes `After` on no path),
|
||
neither refusable by blocked-in-all-directions, `Held`, a bear trap, or
|
||
`NoCommand > 0`. Removing both would bring the wedge back.
|
||
|
||
Both halves of the definition of done were demonstrated by mutation, with
|
||
the deliberately-broken tree reverted afterwards and `.golangci.yml` left
|
||
byte-identical (sha256 `021cc83f...46bcb`). Reverting #24 —
|
||
`AutoSaveOnSignal` replaced by a direct `g.autoSave()`, encoding on the
|
||
calling goroutine — still fails the test with 139 `WARNING: DATA RACE`
|
||
reports naming `snapshotHeader` reading what `executeCommand` writes, so the
|
||
guard is undiminished. Removing the `serviceAutoSaveRequest` call from
|
||
`command()` still fails it too, now in 10s with `saves taken = 0, want 25`
|
||
rather than by hanging.
|
||
|
||
Under load, an A/B at `GOMAXPROCS=2` on a 48-core host at load ~150, with an
|
||
unrelated deliberate failure in the tree so that every run took the verbose
|
||
rerun: the old code failed 8 of 8 runs with "ran out of turns"; the new code
|
||
failed 0 of 8, the only failure being the planted one. Also green across 24
|
||
concurrent unconstrained runs at load ~120, 10 runs alongside a spinner
|
||
load, and 5 runs each at `GOMAXPROCS` 1, 2 and 4. `make check` green, lint 0
|
||
issues.
|
||
|
||
- 2026-08-09 Wizard commands under test (`test/wizard-coverage`, closes #7): the
|
||
last of the three thin spots, so the coverage step is now closed rather than
|
||
narrowed. `game/wizard.go`'s eight functions had no tests of their own, and
|
||
the file is not purely a debug surface — `set_know` writes the per-game
|
||
discovered tables that name items in ordinary play, and `teleport` is what the
|
||
teleport ring calls every fiftieth turn. Package coverage 60.6% -> 62.4%.
|
||
Everything expected was transcribed from `wizard.c`, `command.c` (the
|
||
`CTRL('I')` kit), `extern.c` (`a_class[]`), `weapons.c` (`init_dam[]`) and
|
||
`rogue.h`; 30 mutations were tried and all 30 were caught.
|
||
|
||
Two findings came out of the reading. (1) **A wizard-created cursed weapon
|
||
is not cursed, in C or here.** `create_obj` sets `ISCURSED` and then calls
|
||
`init_weapon`, which _assigns_ `weap->o_flags = iwp->iw_flags` and so
|
||
overwrites the bit it just set; only the `o_hplus` penalty survives, and the
|
||
"cursed" weapon can still be dropped and unwielded. The port reproduces this
|
||
exactly. The test asserts the whole flag word comes back as the `init_dam[]`
|
||
row's value whatever blessing was answered, and deleting the `ISCURSED` line
|
||
from the port leaves every weapon test green — which is the evidence that
|
||
the line is dead for weapons. The armor arm has no such clobber and does
|
||
keep the curse. (2) **`show_map`'s standout is asymmetric in C and symmetric
|
||
here.** C tests `!(real & F_REAL)` before drawing and `!real` — the whole
|
||
flag word — after. `new_level` seeds every square with `p_flags = F_REAL`,
|
||
and exactly three sites clear that bit. `passages.c putpass` sets `F_PASS`
|
||
first, so its secret passage is left at `0x80`. `passages.c door`'s
|
||
secret-door arm clears it on a room-wall exit whose flags are still exactly
|
||
`F_REAL` (`rooms.c` writes no `p_flags` at all), leaving `p_flags == 0`; its
|
||
per-square gate is `rnd(5) == 0` against `putpass`'s `rnd(40) == 0`, and
|
||
`game/passages.go`'s `door` reproduces it. `new_level`'s trap loop then ORs
|
||
in `rnd(NTRAPS)`, which is `abs((int) RN) % 8` and so yields `0..7`, and
|
||
`T_DOOR` is `00` — an unsprung trapdoor square is also exactly zero
|
||
(`be_trapped` is what later ORs `F_SEEN` into it). So C _does_ turn standout
|
||
off again, at secret doors and unsprung trapdoors; what it gets wrong is
|
||
leaking the attribute forward from a secret passage or a non-trapdoor trap
|
||
until it reaches one of those. Intermittent bands of reverse video, not a
|
||
permanently reversed map. `game/wizard.go` tests `isReal` both times and
|
||
highlights the one square. That is a display-only difference in a
|
||
wizard-only command and was reported on the issue rather than changed here;
|
||
the test asserts the map characters unconditionally but the standout
|
||
attribute only up to the first secret square, so it pins nothing that C
|
||
contradicts.
|
||
|
||
Two things the tests had to be built around. The `insist` arm of `whatis` is
|
||
a loop whose only exits are picking a matching item and `n_objs == 0`, so a
|
||
script that runs dry hangs instead of failing — every sequence that can
|
||
re-prompt ends in an abort tail, the `n_objs == 0` exit is reached the way a
|
||
player reaches it (`*` for a list with nothing appropriate in the pack)
|
||
rather than by poking the counter, and the one mutation that deletes that
|
||
exit is the only one of the 30 that fails by timeout instead of fast,
|
||
necessarily so. And `show_map` does **not** mark squares seen — it writes
|
||
into `hw` and touches no `PLACE` at all — so the issue's wording for it
|
||
could not be tested as written; the loop bounds are asserted instead by
|
||
planting a marker in the rows C's loop excludes, since those rows are blank
|
||
on a real level and copying blanks over blanks would have made the bound
|
||
unfalsifiable.
|
||
|
||
- 2026-08-09 Wands and staffs under test (`test/sticks-coverage`, closes #6):
|
||
the second of the three thin spots the Next Step names. `game/sticks.go` was
|
||
the largest under-tested file in the repo — 534 lines, 23 functions, one test
|
||
— and now has `game/sticks_test.go` (the zap handlers, `drain`, `fix_stick`,
|
||
`charge_str`) and `game/bolt_test.go` (the `fire_bolt` geometry). Every
|
||
expectation was read out of `sticks.c` rather than off the Go code; **no
|
||
divergence from C was found**, and three things worth knowing came out of the
|
||
reading. (1) **The bolt trail is the test instrument.** `fire_bolt` paints
|
||
each square with `dirch` and then paints `chat()` back over every square it
|
||
recorded, so on a screen nothing else has drawn on, the non-blank cells
|
||
afterwards are exactly the squares the bolt occupied — and the walls it
|
||
bounced off are absent, because C undoes the record with `c1--` and `break`s
|
||
before the `mvaddch`. That gives an exact assertion of the path and the
|
||
resting place without touching game code, and it is why the tests fire from a
|
||
square that is not the hero's (which is what `chase.c` does for dragon
|
||
breath): with the hero off the ray the run produces one message and the screen
|
||
stays readable. (2) **A bounce reverses both components of the direction, not
|
||
one.** A bolt entering a wall at 45 degrees goes back the way it came instead
|
||
of reflecting off the surface, so the diagonal-into-a-vertical-wall case is
|
||
the one that separates C's rule from the plausible wrong one, and it is
|
||
tested. (3) **The `ch != 'M'` guard on the miss message is a tautology.** `ch`
|
||
comes from `winat`, and `winat` _is_ `t_disguise` when a monster stands there
|
||
(`rogue.h` 57), so `ch == 'M'` implies `t_disguise == 'M'` and the arm can
|
||
never go quiet; it is vestigial from when 'M' was the mimic, and the test pins
|
||
the port to speaking, so nobody "tidies" it into a real silence. The
|
||
door-under-hero exception has no assertion of its own because it cannot have
|
||
one: without it the bolt bounces on the hero's own square forever, recording
|
||
nothing, and `fire_bolt` never returns — the test for it hangs rather than
|
||
fails, which the comment on it says. Determinism comes from a `pinRng` helper
|
||
that searches for a seed whose next draw is the wanted value (running the real
|
||
`Rng`, never predicting it) and from a level the tests carve themselves
|
||
through `drawRoom`, since bounce geometry and `drain`'s room/passage/door
|
||
reach only mean something against known walls and a known passage number. All
|
||
27 mutations tried against the new tests were caught.
|
||
|
||
- 2026-08-09 Trap unit-test coverage (`test/traps-coverage`, closes #14):
|
||
`trapHandlers` had eight entries and **zero** direct tests, on the one
|
||
subsystem besides combat that can kill the hero outright. New
|
||
`game/traps_test.go` (19 tests, 15 subtests) covers all eight arms of
|
||
`move.c be_trapped`, the prologue every trap runs through, and the
|
||
`rust_armor` tail `T_RUST` calls. Package coverage 56.2% -> 57.9% measured on
|
||
`main` at `bf820e3`, the branch point, before the sticks tests landed. Every
|
||
expected value is transcribed from `origin/c-master` and quoted in the file.
|
||
**No divergence from C was found.**
|
||
|
||
The issue body's trap list was wrong and the correction is the first thing
|
||
worth recording: there is no separate "poison dart" trap — `T_DART` **is**
|
||
the poisoned dart, its death message being "a poisoned dart killed you" —
|
||
and the list omitted `T_MYST`, the mystery trap, whose arm is an eleven-way
|
||
`rnd(11)` message switch. `rogue.h` 192-200 is the authority
|
||
(`T_DOOR`/`T_ARROW`/`T_SLEEP`/`T_BEAR`/`T_TELEP`/`T_DART`/`T_RUST`/`T_MYST`,
|
||
`NTRAPS` 8) and the Go `TrapKind` iota matches it index-for-index.
|
||
|
||
Three C details the tests are built around. (1) `BEARTIME` and `SLEEPTIME`
|
||
are `spread(3)` and `spread(5)` (`rogue.h` 108-109), and `spread` is
|
||
`nm - nm/20 + rnd(nm/10)`; for both, `nm/10` is 0 and C's `rnd` short
|
||
circuits a zero range without touching the generator, so each is an exact
|
||
constant that costs **no** random number — and the tests assert the no-draw
|
||
half as well as the value, because a stray draw desynchronises the
|
||
seed-compatible stream. (2) `T_ARROW` swings at `s_lvl - 1` and `T_DART` at
|
||
`s_lvl + 1`: opposite signs, which is exactly the kind of detail a
|
||
transliterating port drops. (3) The strength loss is gated on
|
||
`!ISWEARING(R_SUSTSTR) && !save(VS_POISON)`, and the `&&` is load-bearing —
|
||
with the ring on, C never rolls the save, so the arm must spend two random
|
||
numbers and not three.
|
||
|
||
Two shapes worth keeping, both forced by mutation results rather than
|
||
foresight. Damage dice are checked by a **sweep**, not one shot: `rnd(n)` is
|
||
"raw value % n", so a single draw agrees between a d6 and a d5 five times in
|
||
six and leaves the generator identical either way — the first draft's
|
||
single-trial arrow test passed with `roll(1,6)` mutated to `roll(1,5)`.
|
||
Likewise the swing arguments are pinned by a 200-trial boundary sweep at a
|
||
mid-range to-hit target: a forced hit and a forced miss cannot see a wrong
|
||
`at_lvl` or a dropped `op_arm`, because both arms are reachable at any level
|
||
and swing spends one `rnd(20)` regardless.
|
||
|
||
Mutation-proved, 33 mutations, each reverted, and every one of them is now
|
||
caught. Three were **not** caught on the first pass and the tests were
|
||
strengthened until they were, which is the useful part of the record. (a)
|
||
Deleting `new_level()` from `T_DOOR` left the suite green: `be_trapped`'s
|
||
own prologue stamps the trap glyph into the cell the hero fell through, so
|
||
"the map changed" is true even with no new level dug. The test now counts
|
||
differing cells — exactly one can change that way — and also requires the
|
||
staircase to move and the hero to be re-placed. (b) The `roll(1,6)` case
|
||
above.
|
||
|
||
(c) **`be_trapped` takes a coordinate, and which coordinate decides whether
|
||
`T_TELEP`'s `mvaddch(tc, TRAP)` does anything.** Deleting that line first
|
||
left the suite green, and the first draft wrote that off as an unavoidable
|
||
redundancy — wrongly, because the test only exercised one of the two call
|
||
sites. `move.go` 105-108 (`case Floor`) springs a trap under the hero and
|
||
passes `p.Pos`; there `tc` **is** the hero's square, the prologue has
|
||
already set its `p_ch` to `TRAP`, and `teleport()` opens by drawing
|
||
`floor_at()` — which returns `chat(hero)` — over it, so the glyph is on
|
||
screen before the line runs. But `move.go` 94-98 (`case Trap`), the ordinary
|
||
walk onto a hidden trap, passes `nh`, the square being stepped **onto**,
|
||
with the hero still on the previous square: `teleport()`'s opening `mvaddch`
|
||
paints the old square, `leave_room` writes blanks and never `TRAP`, and
|
||
nothing calls `look()` afterwards because the `case Trap` arm returns before
|
||
`finishMove` for a teleporter. There `mvaddch(tc, TRAP)` is the only writer,
|
||
exactly as C's comment says.
|
||
`TestTrapTeleportDrawsTheTrapOnTheSquareSteppedOnto` springs the trap at a
|
||
floor square next to the hero and pins it: unmutated the screen at `tc`
|
||
reads `^`, with the line deleted it reads `.`.
|
||
|
||
The other 30 each failed their own test and only their own; two also moved
|
||
`TestAutoSaveOnSignalRacesTurnLoop`, which drives real turns and is
|
||
legitimately sensitive to `BEARTIME` and to armor rusting.
|
||
|
||
Deliberately uncovered: the two death messages, "an arrow killed you" and "a
|
||
poisoned dart killed you". Each is printed immediately before `death()`,
|
||
which reaches `myExit` and `os.Exit`, so provoking either would take the
|
||
test binary with it; the hero is pinned with `fortify()` and the damage
|
||
rolls are checked by replaying C's arithmetic instead of by letting HP reach
|
||
zero. They are the only two: `rust_armor`'s `|| ISWEARING(R_SUSTARM)`
|
||
operand and its `if (!to_death)` suppression of the rust-vanishes message,
|
||
the last predicates that had no assertion, are pinned by
|
||
`TestTrapRustHonoursTheRingAndTheToDeathFlag`. This entry does **not**
|
||
rotate `Next Step`: #14 was an out-of-band gap found while surveying, not
|
||
part of the rings/sticks/wizard step.
|
||
|
||
- 2026-08-09 Ring unit-test coverage (`test/rings-coverage`, closes #5): the
|
||
first third of the standing coverage step. `game/rings.go` had **zero** tests
|
||
— not one of the 32 in the suite touched wear, removal, hand choice, or the
|
||
ring contribution to the hunger clock. New `game/rings_test.go` (17 tests, 44
|
||
subtests) covers `ringOn`, `pickRingHand`, `ringOff`, `gethand`, `ringEat` and
|
||
`ringNum`, plus the ring arm of `things.c dropcheck` (`dropRing`), which is
|
||
what actually takes a ring off. Package coverage 53.7% -> 56.2%. `Next Step`
|
||
narrowed rather than rotated: #6 and #7 are the other two thirds.
|
||
|
||
Every expected value is transcribed from `origin/c-master` (`rings.c`,
|
||
`rogue.h`, `things.c`), never from what the port returns, and the C is
|
||
quoted in the file. **No divergence from C was found**, which is the result
|
||
and is worth recording as a negative: `ringEat` is the one function here
|
||
whose being wrong would be invisible — it feeds `daemons.c`'s hunger clock,
|
||
so a bad entry is a slow drift in when the hero starves rather than anything
|
||
a playtest would notice — and it now has all fourteen ring kinds pinned to
|
||
C's table.
|
||
|
||
Three C details the tests were written around. (1) `ring_eat`'s `uses[]`
|
||
holds negatives, and a negative is **not** a cost: C computes
|
||
`eat = (rnd(-eat) == 0)`, a one-in-n chance of a single unit. (2) `R_DIGEST`
|
||
then flips the sign, so slow digestion returns 0 or **-1** and is the only
|
||
ring that gives food back. (3) `ring_num`'s switch closes with the
|
||
`otherwise` macro, which `rogue.h` 53 defines as `break;default` — so its
|
||
four labels fall through to one `sprintf` and every other kind returns `""`
|
||
from a default arm, not by falling off the end. The `RingKind` iota matches
|
||
C's `R_` numbering index-for-index, so a `uses[]` index and a `RingKind` are
|
||
the same number; `R_ADDHIT` is `RingDexterity` and `R_ADDDAM` is
|
||
`RingIncreaseDamage`.
|
||
|
||
The chance rings are checked two ways at once. Each call snapshots the
|
||
generator, runs `ringEat`, and replays 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 — and a frequency check over 4000
|
||
trials backs it with a number a human can read. The non-negative entries
|
||
assert the reverse: the generator must be **untouched**, because C never
|
||
reaches `rnd` on that path and a stray call there would desynchronise the
|
||
whole game's RNG stream from C's and cost seed compatibility. That assertion
|
||
is what caught the one real bug in this work, which was in the test and not
|
||
the game: `g.Rng` is a pointer, so the first draft's snapshots aliased
|
||
instead of copying.
|
||
|
||
Two shapes worth keeping. 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 would loop forever on the headless terminal's filler input
|
||
and the test would die of the 30s timeout instead of failing on its
|
||
assertion — which is exactly what the first draft did, and it was only
|
||
visible because the mutation run was inspected rather than trusted. And the
|
||
"only one hand free" case scripts the _wrong_ hand key deliberately: a port
|
||
that asked anyway consumes it and lands the ring on the wrong side, so the
|
||
test fails on a hand rather than on a hang.
|
||
|
||
Mutation-proved, 23 mutations, each reverted: breaking `pickRingHand`'s
|
||
ask/auto/reject arms, `ring_on`'s type guard, `is_current` guard and all
|
||
three effect arms, `ring_off`'s no-rings message, hand selection and ESCAPE
|
||
abort, `gethand`'s uppercase keys, ESCAPE and reprompt, `dropRing`'s hand
|
||
clearing and both effect arms, `dropcheck`'s cursed gate, three `ringUses`
|
||
entries, the `R_DIGEST` sign flip, the one-in-n roll, the empty-hand zero,
|
||
and `ring_num`'s `ISKNOW` guard, label set and `RING`-vs-`WEAPON`
|
||
formatting. Each failed its own test and only its own; stripping all three
|
||
`ring_on` effect arms failed 3 of 3. All fourteen ring 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, with the files their
|
||
powers actually live in, and `ring_off`'s unreachable "not wearing such a
|
||
ring" arm is documented as unreachable rather than left looking untested.
|
||
|
||
- 2026-08-09 Command dispatch audit (`audit/command-switch-coverage`, closes
|
||
#31): checked every case label in C's `command.c` against this port's
|
||
dispatch, and left the audit behind as a standing test
|
||
(`game/dispatch_test.go`) so the two cannot silently drift again. **No further
|
||
missing keys were found** — `'+'` (#11) was the only one. That is the result,
|
||
and it is worth recording as a negative: the class of bug exists, it has now
|
||
been searched for exhaustively rather than stumbled upon, and the search came
|
||
back empty. Three tables transcribe C's labels with their line numbers:
|
||
main-switch keys answered from `commandHandlers`, main-switch keys whose arms
|
||
need `dispatchKey`'s own switch (the `goto over` re-dispatches, `F`-to-`f`,
|
||
`a`, `m`), and the `if (wizard)` sub-switch. `commandHandlers` is pinned by
|
||
set equality in **both** directions: a missing key is the `'+'` bug, and an
|
||
extra key is the same bug mirrored — a MASTER debug command leaking into
|
||
ordinary play. Two traps make this audit harder than it sounds and are
|
||
documented in the file: `rogue.h` 52-53 defines `when` as `break;case`, so a
|
||
grep for `case ` finds ten of the eighty labels; and the main/wizard split is
|
||
load-bearing, since `'+'` was a divergence in ordinary play precisely because
|
||
it is a main-switch key. Confirms the port targets the MASTER build — all four
|
||
`#ifdef MASTER` sites in `command.c` are ported unconditionally, as is
|
||
`sticks.c` 237.
|
||
|
||
- 2026-08-09 Three small lost C behaviors (`fix/lost-c-behaviors`, closes #13):
|
||
grouped because each is a few lines and all are "restore something the port
|
||
dropped silently". (1) **"what a bizarre schtick!"**, `sticks.c` 237 — the
|
||
`otherwise` arm that closes `do_zap`'s switch, which `doZap` had turned into
|
||
doing nothing at all. Two things about it are easy to get wrong and are why
|
||
the fix is not one line. It is under `#ifdef MASTER`, **not** under a `wizard`
|
||
test, so in the MASTER build this port is it printed for every player — gating
|
||
it on `g.Wizard` would be issue #11's trap in reverse. And `WS_NOP` is a case
|
||
of that switch in its own right (`when WS_NOP: break;`), so "no handler ran"
|
||
cannot be the trigger: the wand of nothing does nothing _quietly_, and only a
|
||
kind C had no case for is bizarre. Since C's switch covers all 14 `WS_`
|
||
values, its `otherwise` is reachable only for an `o_which` outside the table,
|
||
which is exactly what `Object.hasValidWhich` already screens for — so the
|
||
split needed no new state, just a three-way switch on handler / valid-Which /
|
||
neither. All three arms fall through to `obj.Charges--`, as C's do: even the
|
||
bizarre schtick costs a charge. Replaces the deferral comment PR #20 left
|
||
there. (2) **`CTRL('R')` now actually redraws.** C is
|
||
`after = FALSE; clearok(curscr, TRUE); wrefresh(curscr);` (`command.c`
|
||
288-291); the port called `g.refresh()`, the ordinary diffing blit, **which
|
||
cannot fix the only situation the command exists for** — a screen corrupted by
|
||
something else's output leaves the game's record of it still correct, so the
|
||
diff sends nothing and the corruption stays. New `Terminal.Repaint` (tcell
|
||
`Screen.Sync`, which discards tcell's record of the terminal instead of
|
||
diffing against it), `Screen.Repaint`, `g.repaint()`; three implementations to
|
||
update, the same shape as PR #26's `ReadChar` change, so no split was needed.
|
||
Named for the curses operation, not for tcell: the interface is the game's
|
||
abstraction. It repaints what was last rendered — C repainted `curscr`, not
|
||
`stdscr` — so it takes no window, and the arm drops the `refresh()` C never
|
||
had there (`command` refreshes before the next key read anyway). (3) **The
|
||
startup greeting**, `main.c` 107-113, which existed nowhere in the tree. New
|
||
`game.Greeting`, printed by `cmd/rogue/main.go` before `term.New()` — the
|
||
port's `initscr()`. Only the wizard wording is `#ifdef MASTER`; the other is
|
||
unconditional. The `%d` is `dnum`, which `main.c` has just assigned to `seed`,
|
||
so it is `Params.Seed`. Two placement details the issue did not mention and
|
||
the tests now pin: the printf sits **after** `parse_opts`, so a ROGUEOPTS
|
||
`name=` is what the player is greeted by and the account name is only the
|
||
fallback (`Greeting` re-runs `ParseOpts`, which does nothing but assign into
|
||
fields — no RNG, no screen); and it sits after the `-s`/`-d` handling and
|
||
after `restore()`, which never returns, so a resumed game does not announce
|
||
that a dungeon is being dug (`digsNewDungeon`). The game `Greeting` parses
|
||
into is a throwaway but is built the way `New` builds the real one, tables and
|
||
home directory included, because `ParseOpts` handles every option and not just
|
||
the one the greeting reads: `inven=` is matched against `inv_t_name[]`, which
|
||
lives on the game, so a bare `&RogueGame{}` turned a legal `ROGUEOPTS` into a
|
||
nil dereference before the player saw a character. No RNG call is added on any
|
||
path and nothing under `game/testdata/` moved; `TestSeedCompatItemTables` is
|
||
green against the untouched golden. Mutation-proved, each new behaviour
|
||
deleted in turn and only its own test failing: dropping the message arm fails
|
||
`TestZapUnhandledWandSaysBizarreSchtick`; extending it to `WandNothing` fails
|
||
`TestZapWandOfNothingIsSilent`; putting `g.refresh()` back fails
|
||
`TestRedrawCommandForcesFullRepaint`; swapping the two wordings, or the
|
||
ROGUEOPTS name for the account name, fails `TestGreeting`; greeting on the
|
||
restore path fails `TestDigsNewDungeon`. ARCHITECTURE.md §5.3 gains `Repaint`
|
||
and the paragraph on why a blit cannot substitute for it. `Next Step`
|
||
deliberately not rotated: out-of-band issue work.
|
||
|
||
- 2026-08-09 The `'+'` wizard-mode toggle (`fix/wizard-toggle-off`, closes #11):
|
||
C's `command.c` 317-338 has a `when '+'` arm that leaves wizard mode —
|
||
`wizard = FALSE`, `turn_see(TRUE)`, `msg("not wizard any more")` — and the
|
||
port had no `'+'` anywhere, so the key fell through `dispatchKey`'s default to
|
||
`illcom` and answered "illegal command '+'". The password half of that arm was
|
||
dropped on purpose (wizard mode is `ROGUE_WIZARD` configuration) and is in
|
||
ARCHITECTURE.md §9; the leave half was lost silently and is not the same
|
||
decision — it does not touch the password machinery at all. **The substantive
|
||
part is `turn_see(TRUE)`**, not the flag: wizard sight draws every monster the
|
||
hero cannot see, so without the re-hide there is no way back to normal
|
||
visibility once wizard mode is on, and clearing the flag alone would have left
|
||
the screen lying. New `wizardToggleCommand` in `game/command.go`, registered
|
||
in `commandHandlers` between `'^'` and `Escape` — C's own switch order, and
|
||
note that C's arm sits in the **main** command switch under `#ifdef MASTER`,
|
||
not in the `if (wizard) switch (ch)` sub-switch that `wizardCommand` ports, so
|
||
it is reachable whether or not `wizard` is set. That makes the non-wizard case
|
||
a divergence too, and it resolves the way the dropped `passwd()` forces: a
|
||
password check that no longer exists can never succeed, so the else arm is
|
||
what C did on a wrong answer, the message "sorry" — no prompt, since nothing
|
||
typed into one could change the outcome, and none of the
|
||
`noscore`/`turn_see(FALSE)` bookkeeping of C's unreachable success branch. The
|
||
choice is stated in the function's doc comment and in §9, whose password row
|
||
now names the `'+'` enter arm and whose new paragraph records that the leave
|
||
arm is ported in full. Two tests in `game/wizard_test.go` drive `'+'` through
|
||
`g.dispatch`: the wizard one spawns a phantom (`ISINVIS` straight from the
|
||
monster table, so `seeMonst` is false and it is on screen only because wizard
|
||
sight put it there), asserts the precondition — monster glyph drawn in
|
||
standout at its cell, `SenseMonsters` set — and then asserts the flag cleared,
|
||
`SenseMonsters` cleared, the cell back to the map char under the monster with
|
||
standout off, the exact message, and `After` false; the non-wizard one pins
|
||
"sorry" and that `'+'` is no longer an illegal command. Mutation-proved:
|
||
deleting the `turnSee(true)` call fails the test on all three visibility
|
||
assertions, which is the half a flag-only test would have missed. No RNG call
|
||
is added — the `turn_off` arm of `turn_see` never reaches `rnd`, only the
|
||
turn-on arm does — and `TestSeedCompatItemTables` stays green against the
|
||
untouched golden. `Next Step` deliberately not rotated: out-of-band issue
|
||
work.
|
||
|
||
- 2026-08-09 Cleanups deferred from the PR #26 review (`cleanup/pr26-followups`,
|
||
closes #27): four items, no behaviour change. (1) The `sig-leave` entry below
|
||
still argued, in the present tense, that declining to save on SIGINT/SIGQUIT
|
||
was the safe choice because `AutoSave` encodes live state after removing the
|
||
file — both halves untrue since #24, and the entry read as a claim about how
|
||
the code works now rather than a record of what was weighed then. It is in the
|
||
past tense and marked superseded, pointing at the `fix/autosave-race` entry.
|
||
Nothing else in the file was touched — in particular the `err113` linter name
|
||
in the 2026-07-06 entry, which a `grep` for `113` still matches, and the "over
|
||
a hundred reports" wording the #26 rework had already corrected. Note for
|
||
anyone chasing this class of bug: the false claim was in the #12 entry, not
|
||
the #24 one, whose account of the old remove-then-write is correctly past
|
||
tense — find these by content, since `make fmt` reflows the file and cited
|
||
line numbers rot. (2) `encodeSnapshot` is `writeSnapshotFile`: it encodes,
|
||
fsyncs, chmods 0400 and closes, and the old name claimed only the first of
|
||
those. One call site (`saveFile`), and the doc comment now lists what it does
|
||
and why the fsync is there. (3) `TestAutoSaveOnSignalWhileInShellEscape` used
|
||
`t.Error` for its precondition, so a save that was never taken fell through
|
||
into `assertRestorable`, which can then only report a second, derived failure;
|
||
it is `t.Fatal`, matching the identical assertion in the blocked-on-input
|
||
test. (4) `serviceAutoSaveRequest`'s doc comment had a 24-column stub line
|
||
("The result is still a") left by an earlier edit — `gofmt` does not rewrap
|
||
comments, so `fmt-check` was legitimately green and nothing would ever have
|
||
caught it. Rewrapped to the block's width. `Next Step` deliberately not
|
||
rotated: out-of-band issue work.
|
||
|
||
- 2026-08-09 Signal-time autosave moved onto the game goroutine
|
||
(`fix/autosave-race`, closes #24): the SIGHUP/SIGTERM handler gob-encoded the
|
||
live game tree from the signal goroutine while the game goroutine was mid-turn
|
||
mutating it, and `AutoSave` **removed** the save file before encoding — so the
|
||
failure mode was not a stale save but a deleted one followed by a possibly
|
||
torn replacement, with a window in which the player had neither. `make test`
|
||
has run with `-race` since 2026-08-09 and was green, because no test had ever
|
||
driven the turn loop concurrently with a signal: evidence of untested, not of
|
||
safe. The handler now writes nothing itself. `AutoSaveOnSignal` posts a
|
||
request on a one-deep channel, wakes the input read, and waits up to
|
||
`signalSaveTimeout` (3s) for the game goroutine to take it; the encode happens
|
||
on the goroutine that owns the state. **The blocked-on-input case is the whole
|
||
point** — a dropped connection lands while the player is thinking, so a flag
|
||
checked only between turns would never be looked at — and it is handled by
|
||
making the read interruptible: `Terminal.ReadChar` returns `(byte, bool)` with
|
||
`ok == false` meaning "woken by `Interrupt`, no key", `term.Tcell.Interrupt`
|
||
posts a `tcell.EventInterrupt` onto tcell's own event queue to unpark
|
||
`PollEvent`, and `readchar` services the request and reads again, so no caller
|
||
sees the wake-up. The other unbounded park is the `!` shell escape, where a
|
||
hangup used to save and would otherwise have regressed to not saving: the
|
||
shell now runs on a helper goroutine and `runShellEscape` selects on {shell
|
||
finished, save request}, keeping the encode on the game goroutine while it
|
||
draws nothing. Between turns (`command`) covers a game that is busy rather
|
||
than parked. The wait is bounded so that a game goroutine wedged with no
|
||
service point can never stop a signal from getting the process out; giving up
|
||
costs nothing now that `saveFile` writes a temporary file in the save's own
|
||
directory, fsyncs it, and renames it over the target instead of truncating in
|
||
place — a failed or skipped save leaves the previous save whole. New
|
||
`game/autosave_test.go` drives the real turn loop while a second goroutine
|
||
asks for 25 saves (the interleaving that never existed before), plus the
|
||
parked-on-input case with a terminal fake that genuinely blocks, the shell
|
||
case, the deadline case (previous save byte-for-byte intact), the no-file-name
|
||
case, and the rename discipline — the last pinned by a handle opened before
|
||
the save, which still reads the old file whole after it. Each was
|
||
mutation-proved: reverting `AutoSaveOnSignal` to encode on the calling
|
||
goroutine (the pre-fix behavior) makes the turn-loop test fail under `-race`
|
||
with over a hundred reports, and removing each of the three service points
|
||
fails exactly the test for that park with its own message. `pendingSaver` now
|
||
reads the game out from under its mutex instead of delegating with it held,
|
||
because the delegated call blocks until the save is taken — the PR #23
|
||
review's N3 note, load-bearing rather than hypothetical, and pinned by a test.
|
||
The SIGINT/SIGQUIT no-save decision and the single-signal-read ordering
|
||
guarantee are untouched; `savesOnSignal`'s third ground ("safety") is
|
||
rewritten, since the corruption window it weighed no longer exists.
|
||
`MEMORY.md` stops listing signal-time autosave among the deliberate `_ =`
|
||
discards and states the new discipline; `ARCHITECTURE.md` §5.3, the `Terminal`
|
||
sketch, the C-to-Go mapping row and §9's SIGTSTP paragraph are corrected to
|
||
match. Two things review caught and this entry records so they are not undone:
|
||
moving the shell onto a helper goroutine also moved `term.Tcell.ShellEscape`'s
|
||
`panic` on a failed `Screen.Resume` there, and a panic at the top of any
|
||
goroutine kills the process without running the deferred calls of the others —
|
||
including `cmd/rogue/main.go`'s `defer t.Fini()`, so the tty would have been
|
||
left raw on exactly the path where the terminal is already broken (issue #12's
|
||
failure, reintroduced on a new path). `runShellEscape` recovers the helper's
|
||
panic and re-raises it on the game goroutine, pinned by
|
||
`TestShellEscapePanicUnwindsTheGameGoroutine`. And the doc comment took two
|
||
rounds to get right: the first version claimed in four places that nothing is
|
||
half-mutated at the `readchar` service point, and the revision that fixed that
|
||
claimed two of the three service points were between-commands. Both are false.
|
||
Only the check at the top of `command` is between commands — `readchar` is
|
||
reached from mid-command prompts, and `runShellEscape` is reached from
|
||
`shell`, an ordinary `'!'` command handler dispatched inside `command`, with
|
||
that turn's `DoDaemons(Before)`/`DoFuses(Before)` already fired and its AFTER
|
||
pass and ring effects not yet. What is actually guaranteed is that the encode
|
||
runs on the state-owning goroutine, so the snapshot is internally consistent
|
||
and restorable, though it may freeze a command half applied. `Next Step`
|
||
deliberately not rotated: out-of-band issue work.
|
||
|
||
- 2026-08-09 Signal-time terminal restore (`sig-leave`, closes #12): the port
|
||
handled only SIGHUP and SIGTERM, so SIGINT and SIGQUIT killed the process with
|
||
tcell still holding the tty, leaving the user at a shell with no echo. All
|
||
four signals now go to one `os/signal` channel read by one goroutine in
|
||
`cmd/rogue/main.go`, and every path calls `Terminal.Fini` before `os.Exit(0)`
|
||
— C's `leave()`, "leave quickly but curteously". **The decision** (written
|
||
into the `savesOnSignal` comment): SIGHUP/SIGTERM keep autosaving,
|
||
SIGINT/SIGQUIT restore and exit **without** saving. C never saves on INT or
|
||
QUIT anywhere — `leave()` is endwin-and-exit, `quit()` confirms/scores/exits,
|
||
`endit()` goes through `fatal()`, and `save.c auto_save` is reserved for
|
||
HUP/TERM — and the semantics agree: HUP/TERM are involuntary teardown worth
|
||
rescuing a game from, while INT/QUIT are a deliberate "stop now" that must not
|
||
become a one-keystroke checkpoint against a save discipline built to be
|
||
anti-save-scum. A third ground was weighed at the time and has since been
|
||
superseded: back then `AutoSave` gob-encoded live state that the main
|
||
goroutine was still mutating, after removing the old file, so declining to
|
||
save on the signals with nothing to rescue was also the option with no
|
||
corruption window. That window is gone as of the `fix/autosave-race` entry
|
||
above (#24) — the encode now runs on the game goroutine and `saveFile` renames
|
||
a temporary file into place — so nothing here should be read as a statement
|
||
about how saving works now; the split stands on C and on semantics alone, as
|
||
the current `savesOnSignal` comment says. The single-reader design closes the
|
||
window the issue warned about: a second signal arriving mid-save stays unread
|
||
in the buffer instead of exiting out from under the writer
|
||
(`TestLeaveOnSignalIgnoresLaterSignals` reproduces exactly that interleaving).
|
||
New `cmd/rogue/main_test.go` pins the membership of `handledSignals()` itself
|
||
(`TestHandledSignalsSet` — without it the rest of the file, which iterates
|
||
that set, would pass against a set that had silently lost SIGINT and SIGQUIT
|
||
again), and covers the ordering for each signal, the save/no-save split
|
||
against `savesOnSignal`, the mid-save-second-signal case, the pre-game
|
||
`pendingSaver` window, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the
|
||
test process through the same `notifySignals` wiring the game uses; the tty
|
||
leaving raw mode is the one step not checkable headlessly (it needs a
|
||
controlling terminal), and `term.Tcell.Fini` is a direct pass-through to
|
||
tcell's `Screen.Fini` that `myExit` already depends on. Two premises in the
|
||
issue turned out to be wrong and are recorded in ARCHITECTURE.md: `leave()` is
|
||
not installed on SIGINT/SIGQUIT during play (the wiring is in `mdport.c`, the
|
||
shipped build calls `md_onsignal_default()` and installs nothing, and
|
||
`leave()` appears only in the endgame paths of `rip.c`/`main.c`), and Ctrl-C
|
||
never generated SIGINT here anyway, since tcell's raw mode clears `ISIG` and
|
||
the key arrives as byte `0x03` — as it did in C, whose `setup()` calls curses
|
||
`raw()`. The real exposure is `kill -INT`/`kill -QUIT`, a SIGINT to the
|
||
process group while the `!` shell escape has the screen suspended, and the
|
||
window **after** `term.New()`: nothing is raw before it, and the handlers used
|
||
to be installed only once the game existed, leaving the restore path and
|
||
`-d`'s `DeathDemo()` — which never returns, blocking in `waitFor` inside
|
||
`death()` — running raw with no handler at all. The handlers are therefore
|
||
installed immediately after `term.New()`, with the game handed to them
|
||
afterwards via `pendingSaver`; a signal before the game exists restores the
|
||
terminal and exits with nothing to save, and the SIGHUP/SIGTERM autosave
|
||
behavior on the play path is unchanged. ARCHITECTURE.md §9 gained rows for
|
||
SIGTSTP/`tstp()` (dropped: raw mode means Ctrl-Z cannot reach us, a suspend
|
||
from the signal goroutine would race the drawing goroutine, and C armed `tstp`
|
||
only after a `restore()`; the `!` shell escape covers the need), for SIGINT
|
||
not routing to the interactive `quit()` prompt, and for `auto_save` on the
|
||
fault signals; §5.3's claim that tcell handles SIGTSTP was false — tcell
|
||
registers only SIGWINCH — and is corrected. `Next Step` deliberately not
|
||
rotated: out-of-band issue work.
|
||
|
||
- 2026-08-09 Wizard-create bounds fix (`fix/wizard-which-bounds`, closes #10):
|
||
`createObj` stored the raw `0-f` nibble as `Object.Which` with no bounds
|
||
check, so wizard mode -> `C` -> `/` -> `f` produced a wand numbered 15 against
|
||
a 14-entry table and panicked in `fixStick`. Input outside `0-f` overshoots
|
||
much further rather than going negative: `readchar` returns a `byte`, so the
|
||
`int(ch-'a') + 10` branch is byte arithmetic and wraps — `'A'` gives 234 and
|
||
`'!'` gives 202 — and panicked the same way. C's `create_obj()` was equally
|
||
unchecked, but every C consumer was either a `switch` (defined for any value)
|
||
or a static-array read past the end (undefined, and survivable in practice),
|
||
whereas since refactor step 8 one game is one process, so the Go panic kills
|
||
the game with the terminal still in raw mode. Fixed at the two boundaries a
|
||
bad `Which` can enter through: `createObj` now rejects an out-of-range choice
|
||
with a message built from C's own `type_name()` vocabulary and adds nothing to
|
||
the pack (a deliberate, commented divergence, since C had no defined behavior
|
||
here to be faithful to), and `Restore` refuses a snapshot describing such an
|
||
object (`ErrSaveCorrupt`) instead of loading a game that would explode later.
|
||
Behind those, `whichLimit`/`hasValidWhich` back defensive guards at every
|
||
dispatch named in the issue: the three effect tables (the new `quaffHandler`,
|
||
`readHandler`, and `zapHandler` accessors return no handler rather than
|
||
indexing — for wands that is exactly non-`MASTER` C, which matched no case and
|
||
still ran `o_charges--`), the `callIt` lore lookups, `identifyType` (whose
|
||
table is shorter than the scroll table keying it, though no scroll that can
|
||
reach `readIdentify` overshoots it, so that one is defensive rather than a
|
||
live bound), `armorClass` for the four `a_class[]` reads, `initWeapon` against
|
||
the missing `init_dam[]` row for `WeaponFlame`, `fixStick`'s `ws_type[]` read,
|
||
and `inventoryName`, hoisted so one check covers the scroll-title read the
|
||
issue listed plus its potion-color, ring-stone, wand-material, weapon and
|
||
armor siblings. `objectWorth` got the same hoisted guard, since the
|
||
death-screen appraisal reads the identical per-kind tables. No in-range input
|
||
changes behavior and no guard consumes a random number — the rejection
|
||
precedes every `rnd()` call, verified both by an explicit seed-unchanged test
|
||
and by `TestSeedCompatItemTables` staying green untouched. New
|
||
`game/wizard_test.go`: the exact reproducer, a rejection sweep over every
|
||
indexed kind including both wrapping-input forms, an acceptance sweep proving
|
||
valid choices still build the right item, one no-panic test per guarded family
|
||
(wand/potion/scroll/armor/weapon), the `fixStick` crash site, the corrupt-save
|
||
rejection over the wrapped values and a negative `Which` (a decoded snapshot
|
||
is the only source of one, so it is what exercises the `Which >= 0` arm of
|
||
`hasValidWhich`), and a check that `whichLimit` still agrees with the table
|
||
sizes. Each guard was confirmed load-bearing by reverting it and watching the
|
||
test panic. `Next Step` deliberately not rotated: this was out-of-band issue
|
||
work.
|
||
|
||
- 2026-08-09 Stale-docs correction (`docs-staleness`, closes #3): four claims in
|
||
`MEMORY.md`/`TODO.md`/`README.md` had gone false and were misdirecting agents
|
||
— the reviewer on PR #9 repeated one of them verbatim. Each was re-verified
|
||
against the tree before rewriting. (1) `MEMORY.md` described C's `exit()`
|
||
being unwound by a `gameEnd` panic recovered in `Run`; refactor step 8 deleted
|
||
that, `gameEnd` appears nowhere in the sources, and `myExit` (`game/rip.go`)
|
||
now calls `Terminal.Fini` then `os.Exit(0)` while `Run()` never returns — so
|
||
the section states the exit model and its testing consequence (a death exits
|
||
the test binary; hence `fortify()` in `game/run_test.go`). (2) `MEMORY.md`
|
||
said approved lint exceptions live in a "Repo-specific exceptions" block in
|
||
`.golangci.yml`; no such block exists and the config is byte-identical to
|
||
canonical (sha256 `021cc83f…46bcb`), the approvals having moved to in-code
|
||
`//nolint` directives carrying their dates — and `paralleltest` was listed as
|
||
an approved disable when it was in fact fixed (no `paralleltest` token in the
|
||
tree; 32 `t.Parallel()` calls against 32 tests). (3) `MEMORY.md` "Debugging"
|
||
and (4) `README.md` both told the reader to run `go test` directly, which
|
||
since PR #9 silently drops `-timeout 30s -race -cover`; both now point at
|
||
`make test`/`make check`. Also dropped the false "currently v2.12.2" host
|
||
linter claim from the 2026-08-07 entry (the host is v2.10.1 and nothing is
|
||
pinned; the pin question is tracked separately). Documentation only — no code,
|
||
`Makefile`, or config change; `Next Step` deliberately not rotated, since this
|
||
was out-of-band issue work.
|
||
|
||
- 2026-08-09 Policy-shaped `make test` (`make-test-policy-pattern`): the `test:`
|
||
target was a bare `go test $(GO_PKGS)` and now runs
|
||
`-timeout 30s -race -cover` with the mandated conditional verbose rerun (on
|
||
failure it reruns with `-v` and then `exit 1`, so a flaky pass on the second
|
||
attempt cannot rescue the build). `$(GO_PKGS)` is kept rather than hardcoding
|
||
`./...`. The substance was `-race`, not the Makefile edit: this is the first
|
||
time the suite has run under the race detector, and it is clean — no data
|
||
races across five consecutive uncached runs, including the tcell terminal
|
||
layer and the `os.Exit`-path playthrough tests. Wall clock 5.1s cold
|
||
(including the race build) and ~2.3s warm, against the 20s policy budget. The
|
||
failure path was exercised with a throwaway failing test to confirm the rerun
|
||
fires and `make` exits non-zero. Build tooling only; no game behavior change.
|
||
|
||
- 2026-08-07 Canonical linter config (`golangci-v2.12.2`): replaced
|
||
`.golangci.yml` with the shared canonical config (v2 schema; settings now live
|
||
under `linters.settings`, so the `lll`/`funlen`/`cyclop`/`dupl` thresholds
|
||
actually apply — the old top-level `linters-settings` block was silently
|
||
ignored). The four repo-specific disables (`mnd`, `exhaustive`,
|
||
`paralleltest`, `testpackage`) moved out of the config into targeted in-code
|
||
`//nolint` directives carrying the original approval dates, so the config
|
||
stays byte-identical to canonical. Real fixes: `t.Parallel()` in all 32 tests,
|
||
24 long lines wrapped or their comments tightened, control bytes in
|
||
`term/tcell.go` as character literals, and two `wsl_v5` defer cuddles. The
|
||
repo has no golangci-lint version pin to bump (no Dockerfile or CI;
|
||
`make lint` runs whatever `golangci-lint` is on the host). Superseded
|
||
2026-08-10: there is a pin now, and no host lint path — `Dockerfile.lint` pins
|
||
the linter image by digest and `script/lint` runs it in a container. See the
|
||
2026-08-10 entry at the top of this section
|
||
(https://git.eeqj.de/sneak/rgoue/issues/41).
|
||
|
||
- 2026-07-24 Seed compatibility — item tables (seed-compat): instrumented the C
|
||
reference on modern-rogue with a DUMP mode (testdata/c_seedcompat.patch) that
|
||
forces the RNG seed and prints the per-seed item appearance tables (potion
|
||
colors, scroll names, ring stones, wand/staff materials) before initscr, and
|
||
captured its output for four seeds as testdata/item_tables.golden.
|
||
TestSeedCompatItemTables regenerates the same tables from the Go port and they
|
||
match byte for byte — proving the LCG and its consumption order through the
|
||
whole init sequence agree with C. The remaining "same dungeon (map)" half
|
||
would need the harder headless-curses C dump (new_level draws to curses);
|
||
deferred — the item-table match already validates RNG-order faithfulness
|
||
through init, and the Go generation goldens guard determinism thereafter.
|
||
|
||
- 2026-07-23 Playtest hardening (playtest-hardening): added two death-safe
|
||
crash-sweep drives through the real turn loop, within the step-8 os.Exit
|
||
constraint (a fortify() helper pins HP/food/exp and clears the freeze/stuck
|
||
counters each turn so no death exits the test binary; fixed seeds keep them
|
||
deterministic). TestDeepPlaythrough uses quaff/read/zap through command
|
||
dispatch, then descends to depth 8 with a save/restore at depth 4;
|
||
TestTurnLoopCrashSweep mashes movement/search/rest for 200 turns on four
|
||
seeds. Neither surfaced a panic. The interactive "play several games at a real
|
||
tcell terminal" portion needs a human at an 80x24 terminal and is left to the
|
||
maintainer; the binary's non-interactive paths (`-s` scores) were
|
||
smoke-tested.
|
||
|
||
- 2026-07-23 Docs refresh (docs-refresh): rewrote ARCHITECTURE.md Part 2 (the
|
||
pre-implementation design sketch) to match the final code — current type/field
|
||
names (ObjectKind, DiceSpec, split o_arm, step-1 flag names, TrapCount, Level
|
||
list methods), the static tables now on the per-game gameData struct, the
|
||
daemon/effect handler tables, the MessageLine extraction, the Terminal
|
||
interface, the flat gob SaveState, and the New(Params) + os.Exit design. Added
|
||
§7.1, a C-name → Go-name rename table, and a README note on the make targets.
|
||
|
||
- 2026-07-23 Refactor step 8 (refactor/constructor-style): constructor and exit
|
||
pass. NewGame(Config) → New(Params) and Restore takes Params, so the package's
|
||
primary type gets the canonical New() constructor with a named-field Params
|
||
struct (styleguide 139/159). The gameEnd panic unwind is gone: one game run is
|
||
one process, so myExit restores the terminal (new Terminal.Fini) and calls
|
||
os.Exit(0), and Run() no longer returns; the four Run()-to-completion tests
|
||
were reworked/dropped since death (combat or starvation) now exits the process
|
||
(TestScoreRendersList and TestRunDownStairs preserve what is still drivable;
|
||
save/restore stays covered by TestSaveRestoreRoundTrip). The 77-column wrap
|
||
sweep was dropped per sneak (2026-07-23): line lengths left as-is (lll caps at
|
||
88 and passes).
|
||
|
||
- 2026-07-07 Refactor step 7 (refactor/effects-dispatch): effects dispatch
|
||
tables plus a full decomposition sweep — the quaff / readScroll / doZap
|
||
switches, the attack monster-power switch, the be_trapped switch, the daemon
|
||
d_func switch, and the command-key switch all became handler tables on
|
||
gameData (quaffHandlers, readHandlers, zapHandlers, hitHandlers, trapHandlers,
|
||
daemonHandlers, commandHandlers), one small named method per case. Every
|
||
remaining cyclop/gocognit/nestif hot spot was split into named helpers across
|
||
fight, misc (look), command, chase, move, passages, options, pack, things,
|
||
save, daemons, rooms, score, monsters, rings, rip, io, object, weapons,
|
||
wizard, and term/tcell, plus three test functions. Effect order and RNG call
|
||
sequence preserved throughout; the whole golangci-lint run is now 0 issues.
|
||
|
||
- 2026-07-07 Refactor step 6 (refactor/god-object-extraction): MessageLine (was
|
||
MsgLine) owns the msg/addmsg/endmsg machinery, wired to its screen/look/input
|
||
needs via attach(); RogueGame keeps one-line msg/addmsgf/endmsg shorthands so
|
||
call sites are unchanged. Player owns pack bookkeeping (nextPackChar,
|
||
removeFromPack — the state half of leave_pack; leavePack keeps only LastPick
|
||
tracking). Level owns object/monster list management and lookup (ObjectAt
|
||
replaces findObj; AddObject/RemoveObject/AddMonster/RemoveMonster replace
|
||
direct attachObj/detachObj/attachMon/detachMon on level lists).
|
||
Inventory/pickup UI flows stay on RogueGame deliberately: they are display and
|
||
turn orchestration, not state surgery.
|
||
|
||
- 2026-07-07 Refactor step 5 (refactor/item-combat-ui-renames, three commits,
|
||
one subsystem each): items — getItem→promptPackItem now returning (obj, ok),
|
||
invName→inventoryName, doPot→applyPotionFuse; combat — rollEm→rollAttacks,
|
||
attack/moveMonster/chaseStep return (removed bool) instead of C -1/0 int
|
||
codes; UI — getDir→promptDirection. C breadcrumbs kept; suite green.
|
||
|
||
- 2026-07-07 Refactor step 4 (refactor/movement-renames): movement/world renames
|
||
(doMove→moveHero, beTrapped→springTrap, rndmove→randomStep,
|
||
doRooms/doPassages/doMaze→digRooms/digPassages/digMaze, chgStr→changeStrength,
|
||
doRun→startRun, moveStuff→finishMove, turnref→turnRefresh,
|
||
moveMonst→moveMonster, doChase→chaseStep, setOldch→setOldChar, cansee→canSee,
|
||
roomin→roomIn, runto→runTo, conn→connectRooms, putpass→putPassage,
|
||
passnum→numberPassages, numpass→numberPassage, rndPos→randomPos,
|
||
rndRoom→randomRoom, treasRoom→treasureRoom, accntMaze→accountMaze); all
|
||
goto/label flows replaced with loops (moveHero retry loop + extracted
|
||
passageTurn, dispatch re-dispatch loop, chaseStep passage loop, saveGame
|
||
labeled prompt loop); C breadcrumbs kept in doc comments.
|
||
- 2026-07-07 Lint adoption finished (refactor/no-package-globals): all 37
|
||
package-level vars moved into `gameData` (built by `newGameData`, hung on
|
||
RogueGame as `g.data`, set in NewGame and Restore); ObjectKind
|
||
Glyph()/objectKindForGlyph became switches; the table-reading subtype
|
||
Stringers were removed; isMagic became a RogueGame method; goconst fixed with
|
||
named word constants (potionName, goldName, staffName, ripWall, ...);
|
||
testpackage and exhaustive disabled in .golangci.yml with sneak's approval
|
||
(2026-07-07); misspell's corruption of the "ther" scroll syllable reverted.
|
||
mnd disabled with sneak's approval (2026-07-07, follow-up commit). Remaining
|
||
red: cyclop (36), nestif (30), gocognit (23) stay until step 7 fixes them per
|
||
sneak's ruling.
|
||
- 2026-07-06 Lint adoption bulk (refactor/lint-adoption, 5ba9fe8): .golangci.yml
|
||
copied verbatim from the prompts repo (plus the sneak-approved paralleltest
|
||
exception, 2026-07-06); ~1,500 findings fixed (autofix formatting sweep,
|
||
errcheck/err113/noinlineerr error handling, forbidigo, funcorder, recvcheck
|
||
pointer receivers, goprintffuncname renames msg helpers to *f, revive doc
|
||
comments, gocritic switch rewrites, gosec real fixes plus justified nolints,
|
||
unparam signature tightening, C-faithful "missle" spellings restored after
|
||
misspell autofix changed game text).
|
||
- 2026-07-06 Module base path updated to git.eeqj.de/sneak/rgoue (go.mod, term/
|
||
and cmd/ imports, ARCHITECTURE.md, version string).
|
||
- 2026-07-06 Refactor step 3 (refactor/object-fields): Object.Arm split into
|
||
ArmorClass/Charges/GoldValue/Bonus (rings); Stats.Arm → ArmorClass; damage
|
||
strings parsed once into DiceSpec at table definition (ParseDice keeps C
|
||
roll_em parse semantics, incl. "%%%x0" and "000x0" edge cases,
|
||
regression-tested); save format 5.4.4-go3.
|
||
- 2026-07-06 Refactor step 2 (refactor/typed-kinds, b940cfc): ObjectKind
|
||
separates item category from map glyph (Object.Type byte → Kind ObjectKind
|
||
with Glyph()); PotionKind/ScrollKind/RingKind/
|
||
WandKind/WeaponKind/ArmorKind/TrapKind typed iota enums with Stringer; typed
|
||
accessors on Object; getItem/inventory/whatis filters take ObjectKind
|
||
(KindCallable/KindRingOrStick replace CALLABLE/R_OR_S); save format bumped to
|
||
5.4.4-go2. Suite green.
|
||
- 2026-07-06 Refactor step 1 (refactor/descriptive-constants): renamed all flag
|
||
bits, trap types, item subtype constants, and Max* counts to descriptive names
|
||
(IsHuh→Confused, SeeMonst→SenseMonsters, WsHasteM→WandHasteMonster,
|
||
MaxSticks→NumWandTypes, ...); Level.NTraps→TrapCount; C names kept as comment
|
||
breadcrumbs. Pure rename, suite green.
|
||
- 2026-07-06 Made the rgoue branch Go-only: removed C sources and the
|
||
autoconf/VS build system (they remain on master and modern-rogue), ported the
|
||
last wizard command (item-probability listing), rewrote README.md for the Go
|
||
port (c0b533e)
|
||
- 2026-07-06 Ported the command loop, save/restore, the tcell terminal layer,
|
||
and the playable binary at cmd/rogue (41fc104)
|
||
- 2026-07-06 Ported item effects: potions, scrolls, options, call_it (cdf9bf7)
|
||
- 2026-07-06 Ported combat, the chase driver, traps, zapping, death and scores
|
||
(3c5add8)
|
||
- 2026-07-06 Ported dungeon generation, base items, the pack, and monster
|
||
creation (a69ef7d)
|
||
- 2026-07-06 Ported the foundation: types, seed-compatible RNG, item tables,
|
||
daemon scheduler (7fa2048)
|
||
- 2026-07-06 Wrote ARCHITECTURE.md Parts 1 and 2: complete map of the C program
|
||
and the Go port design (91eeee0, 45dba95)
|
||
- Fork base: Davidslv/rogue C 5.4.4 with modernization fixes (C23 prototypes,
|
||
ncurses compat), preserved on master/modern-rogue
|
||
|
||
# Future Steps
|
||
|
||
1. Full-terminal-size support (deferred by explicit decision 2026-07-06):
|
||
per-game dungeon dimensions instead of the 80x24 constants; open design
|
||
questions are resize policy, gameplay tuning at larger sizes, and a --classic
|
||
80x24 mode.
|
||
2. Note: this repo is exempt from the standard policy scaffold, but the
|
||
exemption is narrower than it was. A minimal dev Makefile
|
||
(fmt/fmt-check/lint/test/check targets) exists per sneak's 2026-07-07
|
||
request. `Dockerfile.lint` and `script/lint` are now also permitted, and
|
||
required, along with the `.dockerignore` that scopes their build context:
|
||
sneak's 2026-08-09 ruling (https://git.eeqj.de/sneak/rgoue/issues/41) is that
|
||
every repo lints in a container invoked through `script/lint`, and being
|
||
later and explicit it overrides the 2026-07-07 exemption for those three
|
||
files only. Still do not add: CI config, `REPO_POLICIES.md`, an application
|
||
`Dockerfile`, or any other `script/` entrypoint.
|