Comments and documentation only; the docker build invocation and its three
flags are byte-identical and .dockerignore's effective rules are unchanged.
script/lint and Dockerfile.lint stated how the shape was derived — why two
stages, why `golangci-lint config verify` was omitted, what earlier drafts
of the comments claimed. That is in the history. What survives is the three
traps, each of which yields a green run over an unlinted or partly linted
tree: --target and --no-cache-filter must both stay with $stage matching the
stage name in Dockerfile.lint; --target checks that the stage exists, not
that it runs golangci-lint, and halts the build there; and .dockerignore
decides what reaches the container, so excluding a self-contained Go file
drops it from the lint silently.
The TODO.md entry loses its "Hardened" and "Corrected" paragraphs, which
argued with earlier versions of themselves, and keeps the flags, the durable
property, the three unguarded seams, and the evidence that the gate was
verified rather than assumed.
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.
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.
`--no-cache-filter=lint` is silently ignored by BuildKit when no stage
matches the name, so the entire anti-false-green mechanism hung on one
unvalidated magic string: renaming or mistyping the `lint` stage would
have left the lint layer served from cache and `script/lint` reporting
green having linted nothing. Reproduced here — with the filter pointed at
a nonexistent stage and no `--target`, an unchanged tree built with
`RUN golangci-lint run ... CACHED` and exited 0.
`--target lint` closes it: a stage name that does not exist now fails
loudly (`target stage "nosuchstage" could not be found`, exit 1) instead
of passing. The two flags name the same stage from the same string and
validate each other; both the script and the stage definition in
Dockerfile.lint carry a comment saying they must be kept in sync.
`--output=type=cacheonly` drops the image export. Nothing consumes the
image — the deliverable of this build is an exit code — and the export
cost seconds per run and left one dangling image behind every time, on a
host where pruning is prohibited. The lint stage still executes and a
lint failure still exits non-zero, both verified rather than assumed.
TODO.md: the 2026-08-07 entry's claim that the repo has no linter pin and
lints on the host is marked superseded in place rather than rewritten;
the narrowed scaffold exemption now names `.dockerignore` alongside
`Dockerfile.lint` and `script/lint`; and the specific wall-clock timings
are replaced by the durable property they were evidence for, since they
vary per host and per run.
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` becomes a thin shim over script/lint. This removes the host
linter install that produced a false green here, 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 shape:
- Two stages rather than one. A cached `deps` stage holds
`go mod download`, then `FROM deps AS lint` carries the source 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 re-fetch the module cache over the
network on every run.
- No `golangci-lint config verify` step. It resolves its JSON schema
over a live, unpinned HTTPS call: 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. The reason is recorded in a
comment in Dockerfile.lint.
.dockerignore excludes .git only; the lint reads the Go sources,
go.mod/go.sum and .golangci.yml, none of which come from there.
The TODO.md scaffold-exemption note is narrowed rather than dropped:
Dockerfile.lint and script/lint are now permitted and required, while CI
config, REPO_POLICIES.md, an application Dockerfile and any other
script/ entrypoint still are not.
Verified, since a green docker build is the classic false green: two
consecutive script/lint runs on an unchanged tree each showed the
`golangci-lint run` layer executing (9.8s and 7.9s, both "0 issues.")
while the deps layers reported CACHED; a deliberate indent-error-flow
violation failed the build naming that finding and the unused one, and a
revert went clean again. `make check` green.
TestAutoSaveOnSignalRacesTurnLoop failed intermittently under load. The
captured failure text settles what it 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 is fine; the
test's own drive loop ran out of its fixed 1000-turn budget first.
Confirmed by instrumenting the loop to report the turns it actually
used. The count tracks 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 under the doubled
load of the verbose rerun the test target performs after a failure. The
turns spent 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. Raising it
would hide the flake, not fix it. Each old-code failure took 0.12
seconds - 1000 turns burned in a tenth of a second - which is why every
attempt to reproduce this by loading the host failed: the cap was never
a wall-clock allowance at all.
So the budget is gone rather than larger. driveUntilDone drives until
the saving goroutine finishes and nothing else. Termination still holds,
it just belongs to the code under 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, because g.sigSave is one deep and an unserviced request stays in
the channel for every later call to find full and fail on at once; what
fails is then the real assertion, "saves taken = 0, want 25", rather
than "out of turns". That is not the worst case, and the comment states
the bound that actually holds: 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. It
needs about ten seconds of scheduler starvation per save against the
0.12s-per-1000-turns regime above, so it is remote, and a turn cap did
not bound it either.
Removing the cap exposed a second assumption underneath it. 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. An uncapped drive wedged inside a single command() call. The two
drive tests now use driveTerm, a headless terminal whose script repeats.
Repeating is necessary and not sufficient, and the comment on driveTerm
says which property is load-bearing: ' ' clears After outright and all
eight movement keys clear it on a refused step, so a script of only
those keys wedges exactly as testTerm's tail did - with the script set
to " " the drive hits the 30s timeout inside command(). What makes the
wedge impossible is that the cycle always contains an unconditional
turn-taker, and these scripts contain two, '.' and 's', neither of which
can be refused by being blocked in all directions, Held, in a bear trap,
or under NoCommand > 0. Trimming both out would bring the wedge back.
The guard is undiminished, shown by mutation and reverted afterwards.
Reverting the fix from the earlier signal-autosave work - AutoSaveOnSignal
replaced by a direct g.autoSave(), encoding on the calling goroutine -
still fails the test with a flood of DATA RACE reports (139 here, 62-110
on another machine; the property is what is pinned, not the number),
the encoder reading what the turn loop writes. Removing
serviceAutoSaveRequest from command() still fails it too, now in 10s
with "saves taken = 0, want 25" instead of by hanging.
Under load: at GOMAXPROCS=2 on a 48-core host at load ~150, with an
unrelated deliberate failure in the tree so every run took the verbose
rerun, the old code failed 8 of 8 runs and the new code 0 of 8. 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.
No non-test code changed. make check green, lint 0 issues, .golangci.yml
byte-identical.
`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` covers all eight arms of `move.c be_trapped`, the
prologue every trap runs through, and the `rust_armor` tail `T_RUST`
calls. Test-only: no game code changes.
Every expected value is transcribed from `origin/c-master` (`move.c`,
`misc.c`, `fight.c`, `monsters.c`, `rogue.h`) and quoted in the file. No
divergence from C was found.
The trap set is `rogue.h` 192-200: there is no separate "poison dart"
kind — `T_DART` is the poisoned dart — and `T_MYST`, the eleven-way
`rnd(11)` message switch, is the eighth. Details the tests are built
around: `BEARTIME`/`SLEEPTIME` are `spread(3)`/`spread(5)`, both of which
reduce to `rnd(0)` and so cost no random number, which is asserted as
well as their values; `T_ARROW` swings at `s_lvl - 1` and `T_DART` at
`s_lvl + 1`; and the strength loss is gated on `!ISWEARING(R_SUSTSTR) &&
!save(VS_POISON)`, whose short circuit means the ring saves a random draw
as well as the strength.
Damage dice and swing arguments are checked by sweeps rather than single
shots: `rnd(n)` is "raw value % n", so one draw cannot separate a d6 from
a d5, and a forced hit or miss cannot see a wrong `at_lvl`. Both shapes
were forced by mutation runs that the single-shot versions survived.
`be_trapped` takes a coordinate, and which coordinate decides whether
`T_TELEP`'s `mvaddch(tc, TRAP)` does anything. Sprung under the hero
(`move.go` 105-108, `case Floor`) the line is redundant: `tc` is the
hero's square, already stamped `TRAP` by the prologue and redrawn by
`teleport()`'s opening `mvaddch(hero, floor_at())`. Walked onto
(`move.go` 94-98, `case Trap`) it is the only writer: `tc` is the square
being stepped onto while the hero still stands on the previous one,
`leave_room` writes blanks and never `TRAP`, and the arm returns before
`finishMove` so no `look()` follows. Both shapes are tested.
The two death messages are deliberately uncovered: each is printed
immediately before `death()`, which reaches `myExit` and `os.Exit`, so
provoking either would kill the test binary. The hero is pinned with
`fortify()` and the damage is checked by replaying C's arithmetic. They
are the only two: `rust_armor`'s `|| ISWEARING(R_SUSTARM)` operand and
its `if (!to_death)` message suppression are covered as well.
game/wizard.go had no tests of its own. It 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, so a defect in either leaks into a normal game.
Adds coverage for createObj (pack filing and the gold arm),
createWeaponArmor, createRing, showMap, whatis, whatisPick, setKnow,
teleport and command.go's wizardKit. Package coverage 60.6% -> 62.4%.
Expected values are transcribed from wizard.c, command.c, extern.c,
weapons.c and rogue.h rather than read off the port; wizard mode is
entered through Params.Wizard, the field main.go fills from
ROGUE_WIZARD, so no test pokes the flag.
Two notes from the C. A wizard-created "cursed" weapon is not cursed in
either language: init_weapon assigns o_flags over the ISCURSED bit
create_obj had just set, leaving only the o_hplus penalty, and the test
pins the whole flag word to the init_dam[] row to say so. And show_map
turns standout on for a square missing F_REAL but off only for a square
whose whole flag word is zero. Exactly three sites clear F_REAL:
putpass, which sets F_PASS first and so leaves 0x80; door's secret-door
arm, on a room-wall exit still holding exactly F_REAL, leaving zero;
and new_level's trap loop, whose rnd(NTRAPS) is 0..7, so the T_DOOR
(00) case leaves zero as well. C's wstandend therefore does fire, 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. That display-only difference is reported on
the issue and left alone here, and the test asserts standout only up to
the first secret square.
No game behavior is changed.
game/sticks.go was the largest under-tested file in the repo: 534 lines,
23 functions and a single test. It now has two test files, both written
against the C reference (git show origin/c-master:sticks.c) rather than
against the current Go code, so they can catch divergence instead of
recording it.
game/sticks_test.go covers every zap handler that had none — light in a
room and in a corridor, drain-life's too-weak refusal (which returns
before o_charges--), drain's hit-point split and its kill arm,
drainReaches for all three of C's clauses, invisibility and the flytrap
release, polymorph's detach/re-attach dance with the pack, under-
character and delta-clobbering it does on the way, cancellation, both
teleport wands, magic missile, haste/slow in both directions, fix_stick's
damage and charge formulas, and charge_str.
game/bolt_test.go covers fire_bolt: dirch for all eight directions,
boltBounces including the door the hero stands on, an end-to-end flight
asserting the path and resting square, bounces off both wall
orientations, off a corner and diagonally off a wall (which pins C's rule
that a bounce negates both components rather than reflecting), a bounced
bolt striking the hero who fired it, the strike and miss arms, and the
dragon that shrugs off a flame but not a lightning bolt.
The tests read the flight path off the screen: fire_bolt paints its trail
and then paints chat() back over every square it recorded, so on an
otherwise blank screen the non-blank cells are exactly the squares the
bolt occupied, and the walls it bounced off are absent because C undoes
the record before the mvaddch. Determinism comes from a pinRng helper
that searches for a seed whose next draw is the wanted value, and from a
level the tests carve themselves with the generator's own drawRoom. The
hero is fortified wherever a bolt can reach him, since death exits the
process.
No divergence from C was found. Two notes are recorded in the test
comments: fire_bolt's "ch != 'M'" guard is a tautology, because winat is
t_disguise whenever a monster stands there, and the door-under-hero
exception can only be tested by the fact that the run terminates.
game/rings.go had no test coverage at all: not one of the suite's tests
touched wearing a ring, taking one off, choosing a hand, or the ring
contribution to the hunger clock. New game/rings_test.go covers ringOn,
pickRingHand, ringOff, gethand, ringEat and ringNum, plus the ring arm of
things.c dropcheck (dropRing), which is what actually removes a worn ring.
17 tests, 44 subtests; package coverage 53.7% -> 56.2%. No game code
changes.
Every expected value is transcribed from the C reference on origin/c-master
(rings.c, rogue.h, things.c) and quoted in the file, rather than from what
the port currently returns. No divergence from C was found.
ringEat is the reason this matters most: it feeds daemons.c's hunger clock,
so a wrong entry is a slow, silent drift in when the hero starves. All
fourteen ring kinds are pinned to C's uses[] table, both hands. The three C
subtleties are handled explicitly: a negative uses[] entry is a one-in-n
chance of a single unit and not a literal cost; R_DIGEST then flips the
sign, so slow digestion returns 0 or -1; and ring_num's switch closes with
the otherwise macro (rogue.h 53: break;default), so its four labels fall
through to one sprintf and every other kind returns "" from a default arm.
The chance rings are checked by snapshotting the generator, calling
ringEat, and replaying C's own expression from the identical state, which
pins the one-in-n denominator, the sign flip and the fact that exactly one
rnd call is spent; a frequency check over 4000 trials backs it. The
non-negative entries assert the opposite, that the generator is untouched,
because C never reaches rnd on that path and a stray call there would
desynchronise the game's RNG stream from C's.
Scripted hand answers carry an abort tail (a space for the reprompt's
--More--, then ESCAPE) so that a port which stopped accepting a key fails
on its assertion instead of looping forever on the headless terminal's
filler input. The "only one hand free" cases script the wrong hand key on
purpose: a port that prompted anyway would consume it and land the ring on
the wrong side.
Mutation-proved with 23 mutations, each reverted, each failing its own test
and only its own. All fourteen kinds are exercised; the eleven with no
wear-time effect in C are documented at the foot of the file as
deliberately not given a wear/remove test, and ring_off's unreachable "not
wearing such a ring" arm is documented as unreachable.
Audits every case label in C's command.c against this port's dispatch and
leaves the audit behind as a standing test, so the two lists cannot drift
again. A missing dispatch entry is the one porting error that leaves no
trace at build time: the port is function-by-function, so every C function
has a Go counterpart and a dropped key dangles nothing, fails to compile
nowhere, and simply answers "illegal command" the first time a player
presses it. That is how '+' (#11) survived until PR #30 found it by
accident.
Result: no further missing keys. '+' was the only one. Recording that as a
negative result — the class of bug is real, it has now been searched for
exhaustively rather than stumbled upon, and the search came back empty.
Eighty labels: sixty-five in the main switch, fifteen in the wizard
sub-switch.
commandHandlers is pinned by set equality in both directions. A missing key
is the '+' bug; an extra key is the same bug mirrored, since the likeliest
way to acquire one is promoting a key out of the wizard sub-switch, which
would expose a MASTER debug command in ordinary play. The main-switch keys
that commandHandlers cannot hold - the goto-over re-dispatches, F-to-f, 'a'
and 'm' - are covered separately through dispatchKey, because dropping one
of those is just as silent as dropping a map entry.
Two traps 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: '+' was a divergence in ordinary play,
not just wizard mode, precisely because it is a main-switch key.
Confirms this port targets the MASTER build: all four #ifdef MASTER sites
in command.c are ported unconditionally, as is sticks.c 237.
Three behaviors from 5.4.4 that the port dropped silently. Each is a few
lines; grouped because they are all "restore something C did".
1. sticks.c 237: the "otherwise" arm closing do_zap's switch printed
"what a bizarre schtick!", and doZap had turned it into doing nothing.
The arm is under #ifdef MASTER, not under a runtime wizard test, so in
the MASTER build this port is it printed for every player and must not
be gated on g.Wizard. 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. C's switch covers all 14 WS_
values, so its otherwise is reachable only for an o_which outside the
table, which is what Object.hasValidWhich already screens for. All
three arms fall through to obj.Charges--, as C's do.
2. command.c 288-291: CTRL('R') is "after = FALSE; clearok(curscr, TRUE);
wrefresh(curscr);" — a forced full repaint. The port called
g.refresh(), the ordinary diffing blit, which cannot fix the only
situation the command exists for: a screen corrupted by another
program's output leaves the game's record of it still correct, so the
diff sends nothing. New Terminal.Repaint (tcell Screen.Sync, which
discards tcell's record of the terminal rather than diffing against
it), Screen.Repaint and g.repaint(), implemented in term.Tcell and in
both headless test terminals. Named for the curses operation: the
interface is the game's abstraction, not tcell's. It repaints what was
last rendered — C repainted curscr, not stdscr — so it takes no
window.
3. main.c 107-113: the startup greeting existed nowhere in the tree. New
game.Greeting, printed on stdout 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. Neither wording ends in a
newline. Two placement details the tests pin: the printf sits after
parse_opts, so a ROGUEOPTS name= is what the player is greeted by; 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).
Greeting parses ROGUEOPTS into a throwaway game built the way New builds
the real one, tables and home directory included: ParseOpts handles every
option, not just the one the greeting reads, and inven= is matched
against inv_t_name[], which lives on the game.
All three message strings verified byte-for-byte against origin/c-master
sticks.c and main.c. No RNG call is added on any path and nothing under
game/testdata/ changed; TestSeedCompatItemTables is green against the
untouched golden.
Mutation-proved, each behavior removed in turn with only its own test
failing: dropping the message arm fails
TestZapUnhandledWandSaysBizarreSchtick; extending the message to WS_NOP
fails TestZapWandOfNothingIsSilent; putting g.refresh() back fails
TestRedrawCommandForcesFullRepaint; swapping the two wordings, and
ignoring the ROGUEOPTS name, both fail TestGreeting; greeting on the
restore path fails TestDigsNewDungeon.
ARCHITECTURE.md 5.3 gains Repaint and why a blit cannot substitute for
it; nothing here is deliberately dropped, so section 9 is unchanged.
TODO.md gets a Completed Steps entry; Next Step deliberately not rotated,
this being out-of-band issue work.
C's command.c 317-338 has a `when '+'` arm in the main command switch,
under #ifdef MASTER, that toggles wizard mode. The port had no '+' at
all, so the key fell through dispatchKey's default to illcom and
answered "illegal command '+'".
The password half of that arm was dropped deliberately (wizard mode is
ROGUE_WIZARD configuration) and is recorded in ARCHITECTURE.md section
9. The leave half was lost silently, and it is a different decision: it
does not touch the password machinery. The substantive part of it is
turn_see(TRUE) rather than the flag -- wizard sight draws every monster
the hero cannot see, so without the re-hide there is no way back to
normal visibility, and clearing the flag alone would leave the screen
lying.
New wizardToggleCommand, registered in commandHandlers between '^' and
Escape, which is C's own switch order. Because C's arm sits in the main
switch rather than the `if (wizard) switch (ch)` sub-switch that
wizardCommand ports, it is reachable whether or not wizard is set, so
the non-wizard case was a divergence too. 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", with 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 doc
comment and in section 9.
Two tests 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, then asserts the flag cleared, SenseMonsters cleared, the
cell restored to the map char under the monster with standout off, the
exact message text, and After false. Deleting the turnSee(true) call
fails it on all three visibility assertions. The other pins "sorry".
No RNG call is added: the turn_off arm of turn_see never reaches rnd.
TestSeedCompatItemTables is green against the untouched golden.
Four cleanups recorded as advisories during the PR #26 review and
deliberately kept out of it. No behaviour change.
The 2026-08-09 sig-leave (closes#12) TODO entry still argued in the
present tense that declining to save on SIGINT/SIGQUIT was also the safe
choice, "because AutoSave gob-encodes live state that the main goroutine
is still mutating, after removing the old file". Both halves stopped
being true with #24: the encode runs on the game goroutine and saveFile
is CreateTemp/Sync/Chmod/Rename with no Remove. The paragraph is now in
the past tense and marked superseded, pointing at the fix/autosave-race
entry, and says the split stands on C and on semantics alone — which is
what the current savesOnSignal comment says. The false claim was in the
#12 entry, not the #24 one; the latter's account of the old
remove-then-write is correctly historical and is untouched, as is the
err113 mention in the 2026-07-06 entry.
encodeSnapshot becomes writeSnapshotFile: it encodes, fsyncs, chmods
0400 and closes, and the old name claimed only the first of those. Its
doc comment now names all four and why the fsync is there.
TestAutoSaveOnSignalWhileInShellEscape used t.Error for a precondition,
so a save that was never taken fell through into assertRestorable, which
can then only report a second, derived failure. t.Fatal, matching the
identical assertion in the blocked-on-input test.
serviceAutoSaveRequest's doc comment carried 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 have
caught it; the paragraph is rewrapped to the block's width.
Next Step deliberately not rotated: out-of-band issue work.
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. The suite has run under
-race since 2026-08-09 and was green because nothing had ever driven the
turn loop concurrently with a signal: evidence of untested, not of safe.
The handler no longer writes anything. AutoSaveOnSignal posts a request,
wakes the input read, and waits up to signalSaveTimeout for the game
goroutine to take it; the encode runs on the goroutine that owns the
state, at the three points where that goroutine can sit: between turns
(command), on waking from a blocked readchar, and while parked in the `!`
shell escape (runShellEscape, which now runs the shell on a helper
goroutine so a hangup during it still rescues the game).
Blocked on input is the case that matters — a dropped connection lands
while the player is thinking, so a flag checked only between turns would
never be looked at. Terminal.ReadChar therefore returns (byte, bool),
with ok false meaning "woken by Interrupt, no key", and term.Tcell posts
a tcell.EventInterrupt onto tcell's own event queue to unpark PollEvent.
readchar services the request and reads again, so no caller sees it.
Running the shell on a helper goroutine would also have moved
term.Tcell.ShellEscape's panic on a failed Screen.Resume onto it, and a
panic at the top of any goroutine terminates the process without running
the deferred calls of the others — including cmd/rogue/main.go's
`defer t.Fini()`. The tty would have been left raw on precisely the path
where the terminal is already broken, which is issue #12's failure on a
path this change created. runShellEscape therefore recovers the helper's
panic and re-raises it on the game goroutine, whose stack has the restore
in it, so "every path restores the terminal via Terminal.Fini before
exiting" stays true.
saveFile writes a temporary file in the save's own directory, fsyncs it
and renames it over the target instead of truncating in place, so a save
that fails — or never happens because the deadline ran out — leaves the
player's previous save whole.
What the handoff guarantees is stated exactly rather than flatteringly:
the encode runs on the state-owning goroutine, so the snapshot is
internally consistent and restorable, but it is not necessarily taken
between commands. Only the check at the top of command is; the other two
service points both sit inside a command call already under way. readchar
is reached from mid-command prompts (--More--, askOverwrite, getStr, the
direction and pack prompts) with the command's mutations already applied,
and runShellEscape is reached from shell, an ordinary '!' command handler
dispatched inside command, with that turn's DoDaemons(Before) and
DoFuses(Before) already fired and its AFTER pass not yet. Restoring
re-enters playit at the top of command, so either way the rest of that
command is lost and a fresh BEFORE pass runs on top of the one in the
snapshot.
The SIGINT/SIGQUIT no-save decision and the single-signal-read ordering
guarantee are untouched. pendingSaver reads the game out from under its
mutex rather than delegating with it held, because the delegated call now
blocks until the save is taken.
The port installed handlers for SIGHUP and SIGTERM only, so SIGINT and
SIGQUIT killed the process with tcell still holding the tty and dropped
the user into a shell with no echo and a scrambled screen. All four
signals now go to one os/signal channel read by one goroutine, and every
path calls Terminal.Fini before os.Exit(0) -- C's leave(), "leave
quickly but curteously" (main.c).
The handlers are installed immediately after term.New(), the call that
raises raw mode, rather than after the game exists. Everything between
those two points ran raw with no handler at all: the save-restore path,
and -d's DeathDemo(), which never returns -- death() blocks in
waitFor('\n') (game/rip.go) -- so a kill -INT during the death demo left
exactly the scrambled terminal this fixes. The game is handed to the
handler afterwards through pendingSaver, whose AutoSave is a no-op until
then: a signal before the game is built restores the terminal and exits
with nothing to save. SIGHUP/SIGTERM autosave on the play path is
unchanged.
The save decision, written into the savesOnSignal comment: SIGHUP and
SIGTERM keep autosaving; SIGINT and SIGQUIT restore and exit without
saving. No path in C saves on INT or QUIT (leave() is endwin-and-exit,
quit() confirms/scores/exits, endit() goes through fatal(), and
save.c auto_save is reserved for HUP/TERM), the semantics agree
(involuntary teardown is worth rescuing a game from; a deliberate "stop
now" must not become a one-keystroke checkpoint against an anti-save-scum
save discipline), and it is the safe choice, since AutoSave gob-encodes
live state the main goroutine is still mutating after removing the old
file.
One reader of one signal is also what closes the corruption window: a
second signal arriving while a SIGHUP's AutoSave is mid-write stays
unread in the buffer instead of exiting out from under the writer.
cmd/rogue/main_test.go pins the membership of handledSignals() itself --
the rest of the file iterates that set, so without that assertion the
suite would pass against a set that had lost SIGINT and SIGQUIT again,
which is the regression this issue exists to prevent -- and covers the
ordering per signal, the save/no-save split (driven from the expectation
table so every entry is read), the mid-save second-signal interleaving,
the pre-game pendingSaver window, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM
delivered to the test process through the same notifySignals wiring the
game uses.
Two premises behind the report were wrong and are recorded rather than
silently fixed: 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 and 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()
described above. Nothing is raw before term.New(), so there was never
anything to cover there.
ARCHITECTURE.md section 9 gains rows for SIGTSTP/tstp() (deliberately
dropped: raw mode means Ctrl-Z cannot reach the process, suspending the
screen from the signal goroutine is a logical race against the drawing
goroutine -- not a data race, since tcell guards Suspend/Resume and Fini
alike -- and C armed tstp only after a successful restore(); the ! shell
escape covers the need), for SIGINT not routing to the interactive
quit() prompt, and for auto_save on the fault signals. Section 5.3's
claim that tcell handles SIGTSTP was false -- tcell registers only
SIGWINCH -- and is corrected, and its "every path restores the terminal"
claim now holds because of the install ordering above.
createObj stored the raw 0-f nibble as Object.Which with no bounds
check, so wizard mode -> C -> / -> f made 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, giving 234
for 'A' and 202 for '!', and panicked the same way. C's create_obj()
was equally unchecked, but its consumers were either switches (defined
for any value) or static-array reads past the end (undefined, and
survivable in practice). Since one game is now one process, the Go
panic kills the game outright and leaves the terminal in raw mode.
Reject at the two boundaries a bad Which can enter through. createObj
now refuses an out-of-range choice with a message drawn from C's own
type_name() vocabulary and adds nothing to the pack, a deliberate
divergence recorded in a comment because C had no defined behavior here
to be faithful to. Restore refuses a snapshot describing such an object
(ErrSaveCorrupt) rather than loading a game that would explode later. A
decoded snapshot is also the only source of a genuinely negative Which,
Which being a plain int off the wire, so it is what the Which >= 0 arm
of hasValidWhich defends against.
Behind those, whichLimit/hasValidWhich back defensive guards at every
dispatch the issue names: the quaffHandler/readHandler/zapHandler
accessors return no handler instead of indexing (for wands that is
exactly what non-MASTER C did, matching no case and still running
o_charges--), the callIt lore lookups, identifyType, armorClass for the
a_class[] reads, initWeapon against the missing init_dam[] row for
WeaponFlame, fixStick's ws_type[] read, and inventoryName and
objectWorth, hoisted so one check each covers the whole family of
per-kind name and appraisal tables. identifyType's bound is defensive
rather than live: readHandlers registers readIdentify only for the
identify scrolls, all of which sit inside the shorter idType table.
No in-range input changes behavior and no guard consumes a random
number: the rejection precedes every rnd() call. TestSeedCompatItemTables
stays green untouched.
New game/wizard_test.go covers the exact reproducer, a rejection sweep
over every indexed kind including the wrapped values from input outside
0-f, an acceptance sweep proving valid choices still build the right
item, one no-panic test per guarded family, the fixStick crash site, the
corrupt-save rejection over both the wrapped values and a negative
Which, and a check that whichLimit still agrees with the table sizes.
Each guard was confirmed load-bearing by reverting it and watching the
test fail.
TODO.md records the step; Next Step is deliberately left alone, since
this arrived out of band via an issue.
Four documented claims had gone false and were actively misdirecting agents
working this repo; the independent reviewer on PR #9 repeated one of them
verbatim. Each claim was re-verified against the tree before rewriting.
MEMORY.md "Error handling" described C's exit() calls being unwound by a
gameEnd panic recovered in Run. Refactor step 8 removed that: gameEnd appears
nowhere in the sources, myExit (game/rip.go) restores the terminal via
Terminal.Fini and calls os.Exit(0), and Run() has no return values and never
returns. The section now states that model and its testing consequence -- a
death exits the test binary, which is why tests drive command() directly and
crash sweeps pin the hero with fortify() in game/run_test.go.
MEMORY.md "Linting" said approved exceptions are recorded in a "Repo-specific
exceptions" block in .golangci.yml. There is no such block: the config is
byte-identical to canonical (sha256 021cc83f...46bcb) and the approvals live in
in-code //nolint directives carrying their dates. Following the old text would
have meant editing the canonical config. The same paragraph listed paralleltest
as an approved disable when it was fixed instead -- no paralleltest token
exists in the tree and all 32 tests call t.Parallel().
MEMORY.md "Debugging" and README.md both told the reader to run go test
directly. Since PR #9 the test target carries -timeout 30s -race -cover, so a
raw invocation silently drops the race detector while appearing to verify the
change. Both now point at make test / make check.
TODO.md asserted the host golangci-lint is "currently v2.12.2". It is v2.10.1
and the repo pins nothing, so the claim documented an accident of one machine.
Only the false claim is removed; the pin question is tracked separately.
Documentation only: no code, Makefile, or config change. Next Step is
deliberately not rotated, per the precedent for out-of-band issue work.
The test: target was a bare `go test $(GO_PKGS)`, diverging from the
mandated shape in four ways: no -timeout 30s, no -race, no -cover, and no
conditional verbose rerun. It now runs
go test -timeout 30s -race -cover $(GO_PKGS)
and, on failure, reruns with -v and then exits 1 — so the build still
fails even if a flaky test happens to pass on the second attempt. The
repo's existing $(GO_PKGS) variable is kept rather than hardcoding ./...,
and the recipe is @-prefixed so the rerun banner is the only noise.
The substance here is -race, not the Makefile edit: this is the first
time the suite has run under the race detector. It is clean, across five
consecutive uncached runs, including the tcell terminal layer and the
os.Exit-path playthrough tests that were the suspected risk.
Timing against the 20-second budget: 5.1s cold (including the race
build), ~2.3s warm. The failure path was exercised with a throwaway
failing test to confirm the verbose rerun fires and make exits non-zero.
Build tooling only; no game behavior change. .golangci.yml is untouched.
Replace .golangci.yml with the shared canonical config. The old
config's top-level linters-settings block was silently ignored under
the v2 schema, so the lll/funlen/cyclop/dupl thresholds now actually
apply. The four repo-specific disables (mnd, exhaustive, paralleltest,
testpackage) move out of the config into targeted in-code nolint
directives carrying their original approval dates, keeping the config
byte-identical to the canonical one.
Fixes surfaced by the stricter settings: t.Parallel() added to all 32
tests, 24 overlong lines wrapped or their comments tightened, tcell
control-code returns rewritten as character literals, dupl markers on
the identically-shaped item data tables, and two wsl_v5 defer cuddles.
No behavior changes. The repo has no golangci-lint version pin (no
Dockerfile or CI; make lint runs the host binary, currently v2.12.2),
so there was nothing to bump.
Adds a minimal Makefile wrapping the toolchain the way sneak's other
repos expose it:
- fmt gofmt -w plus prettier (4-space tabs, proseWrap: always)
- fmt-check fail if any Go or Markdown file is unformatted
- lint golangci-lint run ./...
- test go test ./...
- check fmt-check + lint + test (the local pre-commit gate)
Running make fmt normalizes the four existing Markdown docs to the
shared prettier style (80-column proseWrap: always, aligned tables) —
a one-time reflow with no content change. The repo remains exempt from
the rest of the policy scaffold (no Dockerfile, CI, or REPO_POLICIES).
Refactor step 6. MessageLine (was MsgLine) owns the msg/addmsg/endmsg
machinery, wired to its screen, pre---More-- redraw, and input via
attach(); RogueGame keeps one-line msg/addmsgf/endmsg shorthands so
the ~400 call sites are unchanged. Player gains nextPackChar and
removeFromPack (the state half of pack.c leave_pack); leavePack keeps
only the LastPick repeat-command tracking. Level gains ObjectAt
(misc.c find_obj) and AddObject/RemoveObject/AddMonster/RemoveMonster,
replacing direct attach/detach calls on the level lists. Inventory and
pickup UI flows stay on RogueGame: display and orchestration, not
state surgery. Behavior and RNG order unchanged; suite green.
Refactor step 4. Renames (C breadcrumbs kept in doc comments):
doMove→moveHero, beTrapped→springTrap, rndmove→randomStep,
doRooms→digRooms, doPassages→digPassages, doMaze→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 are gone: moveHero uses a retry loop with the
PASSGO corner logic extracted into passageTurn; dispatch re-dispatches
via a loop; chaseStep re-checks via a loop; saveGame uses a labeled
prompt loop. Control flow and RNG call order are unchanged; suite
green.
The 289 findings are C-faithful gameplay literals (probability rolls,
damage spreads, screen coordinates); naming them would invent constants
the C never had and hurt greppability against the reference sources.
gochecknoglobals: all 37 package-level tables consolidated into the
gameData struct (game/tables.go), built by newGameData() and carried
on RogueGame as g.data (set in NewGame and Restore). ObjectKind
Glyph()/objectKindForGlyph are now switches; the table-reading subtype
Stringer methods are gone; isMagic is a RogueGame method.
goconst: repeated words named (potionName/scrollName/ringName/goldName
in object.go, wandName/staffName in sticks.go, ripWall in tables.go).
exhaustive, testpackage: disabled in .golangci.yml with sneak's
approval (2026-07-07).
Also reverts misspell's silent corruption of the "ther" scroll-name
syllable (it had become "there", changing generated scroll names vs C).
Remaining red: cyclop/gocognit/nestif until refactor step 7; mnd
awaits a ruling. TODO.md rotated; MEMORY.md lint notes updated.
.golangci.yml is the prompts-repo standard verbatim plus one approved
exception (paralleltest, sneak 2026-07-06). Roughly 1,500 findings
fixed:
- autofix sweep (wsl_v5/nlreturn/intrange/modernize formatting), with
the misspell autofix REVERTED where it rewrote authentic C game text
("missle vanishes" stays, with nolint and a comment)
- error handling: errcheck sites get real handling — saveFile now
closes explicitly and removes corrupt saves, scoreboard writes are
documented best-effort, err113 sentinel errors (ErrSaveOutOfDate,
ErrScreenTooSmall); panics allowed for unrecoverable states per
MEMORY.md policy
- gosec: real fixes (ParseInt for SEED, 0600 scorefile) and justified
per-line nolints for provably-bounded conversions (randomMonsterLetter
helper collapses eight rnd(26)+'A' sites)
- API tidying from linters: pointer receivers on flag types
(recvcheck), msg helpers renamed addmsgf/doaddf/Printwf/MvPrintwf
(goprintffuncname), myExit()/findFloor(monst)/mkGameInput(t) drop
always-constant params (unparam), gameEnd carries no status
- gocritic/staticcheck: if-else chains to switches, the pack.c
inventory filter untangled into matchesFilter, main.go split so
defers run before exit (exitAfterDefer, funlen)
- revive doc comments on all exported flag types/consts/methods
Remaining findings are confined to linters pending sneak's exception
decision (mnd, gochecknoglobals, cyclop, nestif, gocognit, exhaustive,
goconst, testpackage); TODO.md records the state. MEMORY.md added at
repo root as the project's agent working notes.
go.mod, term/ and cmd/ imports, the ARCHITECTURE.md module references,
and the in-game version string. User request; the repo already lives
at this remote.
Object.Arm carried four meanings in C (o_arm/o_charges/o_goldval plus
ring bonuses); it is now four fields: ArmorClass, Charges, GoldValue,
and Bonus. Stats.Arm becomes Stats.ArmorClass. The Charges()/GoldVal()
accessor pair is gone.
Damage dice strings ("1x4/1x2") are parsed once into DiceSpec — at
table definition for the bestiary and weapon tables, at creation for
items — instead of re-parsed with atoi on every swing as fight.c
roll_em did. ParseDice preserves the C parse semantics exactly,
including the junk-tolerant "%%%x0" bestiary placeholder and the
"000x0" flytrap reset (regression-tested in dice_test.go); the
flytrap's growing grip becomes DiceSpec{{VfHit, 1}}.
Save format bumps to 5.4.4-go3 (field renames and retypes would
silently zero under gob's match-by-name decoding).
No behavior change; full suite green.
The rotation should have ridden the step-2 commit per the Workflow
section, but the file was edited concurrently and the update was lost;
recording it now. Step 3 (un-overload Object fields) is Next Step.
Itemize the refactor from transliterated port to idiomatic Go: eight
stepped feature branches (descriptive constant renames, typed kind
enums, un-overloaded Object fields, per-subsystem method renames,
god-object extraction, effects dispatch tables, constructor/style
pass, docs refresh), slotted ahead of the existing playtest/lint/
release steps. Also add the styleguide-required .gitignore (a stray
built binary was sitting untracked in the root).
