Comments and documentation only; the docker build invocation and its flags are untouched, and .dockerignore's effective rules are unchanged. Two prose gaps from review. First, "both flags below must stay" had lost its anchor: it ended a paragraph naming only --no-cache-filter, --target was not introduced until the next one, and three flags follow on the command, so a reader could pick the wrong pair. It now names --target and --no-cache-filter explicitly. Second, the list of things the tooling does not check covered the $stage seam but not .dockerignore, which sits in the same trust boundary and is the more likely thing to be edited — the first review on this change actively suggested extending it for build artifacts. Only what reaches the container is linted, so excluding a Go source there removes it from the lint with no warning. Verified rather than asserted: a planted violation plus that one path in .dockerignore yields `0 issues.` at exit 0 with the violation still in the working tree, while excluding a file other code still references fails loudly on `undefined:` typecheck errors instead. The warning is recorded in script/lint alongside the $stage seam and in .dockerignore itself, where the edit would actually be made.
931 lines
63 KiB
Markdown
931 lines
63 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. The same applies to `.dockerignore`, which is part of this gate
|
||
rather than housekeeping: only what reaches the container is linted, so
|
||
excluding a Go source there drops it from the lint silently — verified, a
|
||
planted violation plus that one path in `.dockerignore` gives `0 issues.` at
|
||
exit 0 with the violation still in the tree, and it fails loudly only when
|
||
other code still references the excluded file.
|
||
|
||
- 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.
|