Merge cleanup/pr26-followups (post-#26 doc and naming cleanups)

TODO.md

@@ -34,6 +34,32 @@ wizard commands).
 
 # Completed Steps
 
+- 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
@@ -115,45 +141,51 @@ wizard commands).
   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. It is also the safe choice: `AutoSave` gob-encodes live state
+  anti-save-scum. A third ground was weighed at the time and has since been
-  that the main goroutine is still mutating, after removing the old file, so on
+  superseded: back then `AutoSave` gob-encoded live state that the main
-  the signals with nothing to rescue the port takes the option with no
+  goroutine was still mutating, after removing the old file, so declining to
-  corruption window. The single-reader design closes the window the issue warned
+  save on the signals with nothing to rescue was also the option with no
-  about: a second signal arriving mid-save stays unread in the buffer instead of
+  corruption window. That window is gone as of the `fix/autosave-race` entry
-  exiting out from under the writer (`TestLeaveOnSignalIgnoresLaterSignals`
+  above (#24) — the encode now runs on the game goroutine and `saveFile` renames
-  reproduces exactly that interleaving). New `cmd/rogue/main_test.go` pins the
+  a temporary file into place — so nothing here should be read as a statement
-  membership of `handledSignals()` itself (`TestHandledSignalsSet` — without it
+  about how saving works now; the split stands on C and on semantics alone, as
-  the rest of the file, which iterates that set, would pass against a set that
+  the current `savesOnSignal` comment says. The single-reader design closes the
-  had silently lost SIGINT and SIGQUIT again), and covers the ordering for each
+  window the issue warned about: a second signal arriving mid-save stays unread
-  signal, the save/no-save split against `savesOnSignal`, the
+  in the buffer instead of exiting out from under the writer
-  mid-save-second-signal case, the pre-game `pendingSaver` window, and real
+  (`TestLeaveOnSignalIgnoresLaterSignals` reproduces exactly that interleaving).
-  SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the test process through the same
+  New `cmd/rogue/main_test.go` pins the membership of `handledSignals()` itself
-  `notifySignals` wiring the game uses; the tty leaving raw mode is the one step
+  (`TestHandledSignalsSet` — without it the rest of the file, which iterates
-  not checkable headlessly (it needs a controlling terminal), and
+  that set, would pass against a set that had silently lost SIGINT and SIGQUIT
-  `term.Tcell.Fini` is a direct pass-through to tcell's `Screen.Fini` that
+  again), and covers the ordering for each signal, the save/no-save split
-  `myExit` already depends on. Two premises in the issue turned out to be wrong
+  against `savesOnSignal`, the mid-save-second-signal case, the pre-game
-  and are recorded in ARCHITECTURE.md: `leave()` is not installed on
+  `pendingSaver` window, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the
-  SIGINT/SIGQUIT during play (the wiring is in `mdport.c`, the shipped build
+  test process through the same `notifySignals` wiring the game uses; the tty
-  calls `md_onsignal_default()` and installs nothing, and `leave()` appears only
+  leaving raw mode is the one step not checkable headlessly (it needs a
-  in the endgame paths of `rip.c`/`main.c`), and Ctrl-C never generated SIGINT
+  controlling terminal), and `term.Tcell.Fini` is a direct pass-through to
-  here anyway, since tcell's raw mode clears `ISIG` and the key arrives as byte
+  tcell's `Screen.Fini` that `myExit` already depends on. Two premises in the
-  `0x03` — as it did in C, whose `setup()` calls curses `raw()`. The real
+  issue turned out to be wrong and are recorded in ARCHITECTURE.md: `leave()` is
-  exposure is `kill -INT`/`kill -QUIT`, a SIGINT to the process group while the
+  not installed on SIGINT/SIGQUIT during play (the wiring is in `mdport.c`, the
-  `!` shell escape has the screen suspended, and the window **after**
+  shipped build calls `md_onsignal_default()` and installs nothing, and
-  `term.New()`: nothing is raw before it, and the handlers used to be installed
+  `leave()` appears only in the endgame paths of `rip.c`/`main.c`), and Ctrl-C
-  only once the game existed, leaving the restore path and `-d`'s `DeathDemo()`
+  never generated SIGINT here anyway, since tcell's raw mode clears `ISIG` and
-  — which never returns, blocking in `waitFor` inside `death()` — running raw
+  the key arrives as byte `0x03` — as it did in C, whose `setup()` calls curses
-  with no handler at all. The handlers are therefore installed immediately after
+  `raw()`. The real exposure is `kill -INT`/`kill -QUIT`, a SIGINT to the
-  `term.New()`, with the game handed to them afterwards via `pendingSaver`; a
+  process group while the `!` shell escape has the screen suspended, and the
-  signal before the game exists restores the terminal and exits with nothing to
+  window **after** `term.New()`: nothing is raw before it, and the handlers used
-  save, and the SIGHUP/SIGTERM autosave behavior on the play path is unchanged.
+  to be installed only once the game existed, leaving the restore path and
-  ARCHITECTURE.md §9 gained rows for SIGTSTP/`tstp()` (dropped: raw mode means
+  `-d`'s `DeathDemo()` — which never returns, blocking in `waitFor` inside
-  Ctrl-Z cannot reach us, a suspend from the signal goroutine would race the
+  `death()` — running raw with no handler at all. The handlers are therefore
-  drawing goroutine, and C armed `tstp` only after a `restore()`; the `!` shell
+  installed immediately after `term.New()`, with the game handed to them
-  escape covers the need), for SIGINT not routing to the interactive `quit()`
+  afterwards via `pendingSaver`; a signal before the game exists restores the
-  prompt, and for `auto_save` on the fault signals; §5.3's claim that tcell
+  terminal and exits with nothing to save, and the SIGHUP/SIGTERM autosave
-  handles SIGTSTP was false — tcell registers only SIGWINCH — and is corrected.
+  behavior on the play path is unchanged. ARCHITECTURE.md §9 gained rows for
-  `Next Step` deliberately not rotated: out-of-band issue work.
+  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

game/autosave_test.go

@@ -152,7 +152,7 @@ func TestAutoSaveOnSignalWhileInShellEscape(t *testing.T) {
 	<-st.entered
 
 	if !g.AutoSaveOnSignal(autoSaveWait) {
-		t.Error("the save was not taken while the game was in the shell escape")
+		t.Fatal("the save was not taken while the game was in the shell escape")
 	}
 
 	assertRestorable(t, g.FileName)

game/save.go

@@ -670,7 +670,7 @@ func (g *RogueGame) saveFile(path string) error {
 
 	tmp := f.Name()
 
-	writeErr := encodeSnapshot(f, g.snapshot())
+	writeErr := writeSnapshotFile(f, g.snapshot())
 	if writeErr != nil {
 		_ = os.Remove(tmp) // never leave a half-written file behind
 
@@ -687,11 +687,13 @@ func (g *RogueGame) saveFile(path string) error {
 	return nil
 }
 
-// encodeSnapshot encodes the snapshot into an open temporary file and
+// writeSnapshotFile writes the snapshot into an open temporary file: it
-// closes it, leaving it read-only as the C game's saves were (save.c
+// encodes, fsyncs so the bytes reach the disk before the caller renames
-// save_file). It never removes the file: its caller owns the cleanup, so
+// the file into place, chmods it read-only as the C game's saves were
-// that one place decides what happens to a failed write.
+// (save.c save_file), and closes it. It never removes the file: its
-func encodeSnapshot(f *os.File, st *SaveState) error {
+// caller owns the cleanup, so that one place decides what happens to a
+// failed write.
+func writeSnapshotFile(f *os.File, st *SaveState) error {
 	encErr := gob.NewEncoder(f).Encode(st)
 	if encErr == nil {
 		encErr = f.Sync()
@@ -812,11 +814,10 @@ func (g *RogueGame) AutoSaveOnSignal(timeout time.Duration) bool {
 // handler that clears After before prompting, as identifyTrapCommand
 // does ahead of promptDirection.
 //
-// The result is still a
+// The result is still a coherent game state, one turn's worth of effects
-// coherent game state, one turn's worth of effects off — strictly better
+// off — strictly better than the torn encode this replaced, and the cost
-// than the torn encode this replaced, and the cost of being able to save
+// of being able to save a player whose line dropped mid-prompt, or who
-// a player whose line dropped mid-prompt, or who is away in a shell, at
+// is away in a shell, at all.
-// all.
 func (g *RogueGame) serviceAutoSaveRequest() {
 	select {
 	case req := <-g.sigSave: