Three small items recorded as advisories during the PR #26 review and
deliberately not folded into it. Bundled here as one commit-sized cleanup.
1. TODO.md:99's Completed Steps entry is now false
That entry still asserts, in the present tense, that AutoSave encodes live
state after removing the file. PR #26 changed both halves: the encode moved
to the game goroutine, and autoSave no longer removes anything (the write is
now CreateTemp → Sync → Chmod → Rename).
Historical entries describing what a past step did are fine, but this one
reads as a statement about how the code works now, and it is wrong. Reword
so it describes what that step did at the time without asserting current
behaviour — or note the supersession explicitly.
2. encodeSnapshot is misnamed
It does not just encode: it also syncs, chmods, and closes the file. Rename to writeSnapshotFile (or similar) so the name matches the responsibility.
3. game/autosave_test.go uses t.Error where t.Fatal is correct
In the shell-escape test, a failed precondition should stop the test rather
than let it continue into assertions that cannot be meaningful.
Definition of done
TODO.md:99 no longer asserts current behaviour that is false.
encodeSnapshot renamed to something that reflects that it writes, syncs,
chmods, and closes; all call sites and any doc comments referencing it
updated.
The t.Error → t.Fatal fix applied where the assertion is a precondition.
make check fully green.
TODO.md updated in the same commit — Completed Steps entry, and do not
rotate "Next Step".
Commit title ends with (closes #N).
Implementation requirements
Expected to touch only TODO.md, game/save.go, and game/autosave_test.go. If the rename pulls in more files, that is fine —
but nothing else should change behaviour.
Zero behaviour change. A rename plus doc corrections plus a test-severity
fix. If anything else seems necessary, stop and report.
Do NOT touch the signal-handling, pendingSaver, service-point, or atomic-
write logic settled in PRs #23 and #26.
Lint environment warning: golangci-lint on this host shares one cache and
lock across ~18 concurrent sessions. Run make lint/make check in a retry
loop and accept only a run that neither reports parallel golangci-lint is running nor mentions paths outside your own
worktree. A false green has already happened in this repo.
make targets only. Do NOT modify .golangci.yml. No Dockerfile/CI/script/.
Do NOT regenerate goldens under game/testdata/.
Leave c-master and modern-rogue alone.
Never mention Claude or Anthropic anywhere.
Depends on
PR #26 (#24) — land that first; all three items are against code it
introduces.
Priority
Low. Item 1 is the only one with any real cost, and it is the same
false-claim-in-TODO.md failure mode this repo has now hit repeatedly.
## Problem
Three small items recorded as advisories during the PR #26 review and
deliberately not folded into it. Bundled here as one commit-sized cleanup.
### 1. `TODO.md:99`'s Completed Steps entry is now false
That entry still asserts, in the present tense, that `AutoSave` encodes live
state *after removing the file*. PR #26 changed both halves: the encode moved
to the game goroutine, and `autoSave` no longer removes anything (the write is
now CreateTemp → Sync → Chmod → Rename).
Historical entries describing what a past step did are fine, but this one
reads as a statement about how the code works **now**, and it is wrong. Reword
so it describes what that step did at the time without asserting current
behaviour — or note the supersession explicitly.
### 2. `encodeSnapshot` is misnamed
It does not just encode: it also syncs, chmods, and closes the file. Rename to
`writeSnapshotFile` (or similar) so the name matches the responsibility.
### 3. `game/autosave_test.go` uses `t.Error` where `t.Fatal` is correct
In the shell-escape test, a failed precondition should stop the test rather
than let it continue into assertions that cannot be meaningful.
## Definition of done
1. `TODO.md:99` no longer asserts current behaviour that is false.
2. `encodeSnapshot` renamed to something that reflects that it writes, syncs,
chmods, and closes; all call sites and any doc comments referencing it
updated.
3. The `t.Error` → `t.Fatal` fix applied where the assertion is a precondition.
4. `make check` fully green.
5. `TODO.md` updated in the same commit — Completed Steps entry, and do **not**
rotate "Next Step".
6. Commit title ends with ` (closes #N)`.
## Implementation requirements
- Expected to touch only `TODO.md`, `game/save.go`, and
`game/autosave_test.go`. If the rename pulls in more files, that is fine —
but nothing else should change behaviour.
- **Zero behaviour change.** A rename plus doc corrections plus a test-severity
fix. If anything else seems necessary, stop and report.
- Do NOT touch the signal-handling, `pendingSaver`, service-point, or atomic-
write logic settled in PRs #23 and #26.
- **Lint environment warning:** golangci-lint on this host shares one cache and
lock across ~18 concurrent sessions. Run `make lint`/`make check` in a retry
loop and accept only a run that neither reports
`parallel golangci-lint is running` nor mentions paths outside your own
worktree. A false green has already happened in this repo.
- `make` targets only. Do NOT modify `.golangci.yml`. No Dockerfile/CI/`script/`.
- Do NOT regenerate goldens under `game/testdata/`.
- Leave `c-master` and `modern-rogue` alone.
- Never mention Claude or Anthropic anywhere.
## Depends on
PR #26 (#24) — land that first; all three items are against code it
introduces.
## Priority
Low. Item 1 is the only one with any real cost, and it is the same
false-claim-in-`TODO.md` failure mode this repo has now hit repeatedly.
PR #26 has landed (3a01283, merged as 85354f2), so this is now unblocked.
Adding a fourth item, from the final review (recorded there as N2):
game/save.go:815 leaves a stub comment line — // The result is still a —
at roughly a third the width of its neighbours, left behind by an edit to
the surrounding paragraph. gofmt does not rewrap comments, so fmt-check
is legitimately green and nothing catches it. Rewrap the paragraph.
Two corrections to this issue's original text, both now settled by the
review rounds on #26:
Item 1 said TODO.md:99's entry asserts in the present tense that AutoSave encodes live state after removing the file. Re-check the current
line number before editing — TODO.md has been reflowed by make fmt
twice since this issue was filed, so the line has moved. Find it by content,
not by number.
The final rework already corrected one adjacent claim (113 reports → over a hundred reports), so do not re-do that.
A warning worth heeding on this issue specifically. PR #26 failed five
consecutive review rounds, and every single failure was a confidently-worded
claim that nobody had verified — including two asserted by me, and one
malformed done-criterion of mine that the implementer was right to refuse.
Item 1 here is precisely that genre: a claim about what a TODO.md entry
asserts. Read the entry and the current code before rewording it. If this
issue's description turns out to be wrong when you check it, say so and stop
rather than writing something you cannot stand behind.
Also note grep-based done-criteria on TODO.md are treacherous: a search
for 113 still matches the linter name err113 in the 2026-07-06 entry,
which is pre-existing and must not be touched.
PR #26 has landed (`3a01283`, merged as `85354f2`), so this is now unblocked.
**Adding a fourth item, from the final review (recorded there as N2):**
4. `game/save.go:815` leaves a stub comment line — `// The result is still a` —
at roughly a third the width of its neighbours, left behind by an edit to
the surrounding paragraph. `gofmt` does not rewrap comments, so `fmt-check`
is legitimately green and nothing catches it. Rewrap the paragraph.
**Two corrections to this issue's original text**, both now settled by the
review rounds on #26:
- Item 1 said `TODO.md:99`'s entry asserts in the present tense that
`AutoSave` encodes live state after removing the file. Re-check the current
line number before editing — `TODO.md` has been reflowed by `make fmt`
twice since this issue was filed, so the line has moved. Find it by content,
not by number.
- The final rework already corrected one adjacent claim (`113 reports` →
`over a hundred reports`), so do not re-do that.
**A warning worth heeding on this issue specifically.** PR #26 failed **five**
consecutive review rounds, and every single failure was a confidently-worded
claim that nobody had verified — including two asserted by me, and one
malformed done-criterion of mine that the implementer was right to refuse.
Item 1 here is precisely that genre: a claim about what a `TODO.md` entry
asserts. **Read the entry and the current code before rewording it.** If this
issue's description turns out to be wrong when you check it, say so and stop
rather than writing something you cannot stand behind.
Also note `grep`-based done-criteria on `TODO.md` are treacherous: a search
for `113` still matches the linter name `err113` in the 2026-07-06 entry,
which is pre-existing and must not be touched.
Plan (verified against the tree at 85354f2 before writing anything)
All four items check out as described. What I found, item by item:
1. The false TODO.md entry — found by content, not line number. It is not in the fix/autosave-race (#24) entry, which describes the old
behaviour correctly in the past tense ("AutoSaveremoved the save file
before encoding"). It is in the 2026-08-09 sig-leave (closes#12) entry,
now at lines 118-121, in the paragraph justifying the SIGINT/SIGQUIT no-save
decision:
> It is also the safe choice: AutoSave gob-encodes live state that the main
> goroutine is still mutating, after removing the old file, so on the signals
> with nothing to rescue the port takes the option with no corruption window.
Present tense, and both halves are false as of #26: the encode is on the game
goroutine, and saveFile is CreateTemp → Sync → Chmod → Rename with no Remove. I will put it in the past tense and add an explicit supersession
pointing at the fix/autosave-race entry (which sits above it, newest
first), noting that the decision now rests on C and semantics alone — matching
what the current savesOnSignal doc comment in cmd/rogue/main.go already
says. I will not touch the 113/err113 text or the 2026-07-06 entry.
2. encodeSnapshot → writeSnapshotFile. Confirmed: it encodes, syncs,
chmods and closes (game/save.go:694). One call site (saveFile, game/save.go:673) and its own doc comment at game/save.go:690; a repo-wide
grep finds no other reference, in Go or Markdown. Rename by hand with Edit.
3. t.Error → t.Fatal. Confirmed at game/autosave_test.go:155 (the
issue said ~151; the if is at 154 and the call at 155), in TestAutoSaveOnSignalWhileInShellEscape. It is a precondition: the very next
statement is assertRestorable(t, g.FileName), which is meaningless if the
save was never taken. The sibling test at line 116 already uses t.Fatal for
the identical assertion, so this is also an inconsistency.
4. The stub comment line. Confirmed: game/save.go:815 is // The result is still a at 24 columns where its neighbours run 65-73. Rewrap that paragraph
only, to the same width as the rest of the block.
Verification
Branch cleanup/pr26-followups off main @ 85354f2 in a throwaway worktree. make fmt then make check (fmt-check, lint, test with -timeout 30s -race -cover), with GOLANGCI_LINT_CACHE pointed at a fresh
empty private directory and the output checked for foreign paths. No goldens
regenerated, .golangci.yml untouched, Next Step not rotated.
## Plan (verified against the tree at `85354f2` before writing anything)
All four items check out as described. What I found, item by item:
**1. The false `TODO.md` entry — found by content, not line number.** It is
**not** in the `fix/autosave-race` (#24) entry, which describes the old
behaviour correctly in the past tense ("`AutoSave` **removed** the save file
before encoding"). It is in the **2026-08-09 `sig-leave` (closes #12)** entry,
now at lines 118-121, in the paragraph justifying the SIGINT/SIGQUIT no-save
decision:
> It is also the safe choice: `AutoSave` gob-encodes live state that the main
> goroutine is still mutating, after removing the old file, so on the signals
> with nothing to rescue the port takes the option with no corruption window.
Present tense, and both halves are false as of #26: the encode is on the game
goroutine, and `saveFile` is CreateTemp → Sync → Chmod → Rename with no
`Remove`. I will put it in the past tense and add an explicit supersession
pointing at the `fix/autosave-race` entry (which sits *above* it, newest
first), noting that the decision now rests on C and semantics alone — matching
what the current `savesOnSignal` doc comment in `cmd/rogue/main.go` already
says. I will not touch the `113`/`err113` text or the 2026-07-06 entry.
**2. `encodeSnapshot` → `writeSnapshotFile`.** Confirmed: it encodes, syncs,
chmods and closes (`game/save.go:694`). One call site (`saveFile`,
`game/save.go:673`) and its own doc comment at `game/save.go:690`; a repo-wide
grep finds no other reference, in Go or Markdown. Rename by hand with Edit.
**3. `t.Error` → `t.Fatal`.** Confirmed at `game/autosave_test.go:155` (the
issue said ~151; the `if` is at 154 and the call at 155), in
`TestAutoSaveOnSignalWhileInShellEscape`. It is a precondition: the very next
statement is `assertRestorable(t, g.FileName)`, which is meaningless if the
save was never taken. The sibling test at line 116 already uses `t.Fatal` for
the identical assertion, so this is also an inconsistency.
**4. The stub comment line.** Confirmed: `game/save.go:815` is `// The result
is still a` at 24 columns where its neighbours run 65-73. Rewrap that paragraph
only, to the same width as the rest of the block.
## Verification
Branch `cleanup/pr26-followups` off `main` @ `85354f2` in a throwaway worktree.
`make fmt` then `make check` (`fmt-check`, `lint`, `test` with
`-timeout 30s -race -cover`), with `GOLANGCI_LINT_CACHE` pointed at a fresh
empty private directory and the output checked for foreign paths. No goldens
regenerated, `.golangci.yml` untouched, `Next Step` not rotated.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
Three small items recorded as advisories during the PR #26 review and
deliberately not folded into it. Bundled here as one commit-sized cleanup.
1.
TODO.md:99's Completed Steps entry is now falseThat entry still asserts, in the present tense, that
AutoSaveencodes livestate after removing the file. PR #26 changed both halves: the encode moved
to the game goroutine, and
autoSaveno longer removes anything (the write isnow CreateTemp → Sync → Chmod → Rename).
Historical entries describing what a past step did are fine, but this one
reads as a statement about how the code works now, and it is wrong. Reword
so it describes what that step did at the time without asserting current
behaviour — or note the supersession explicitly.
2.
encodeSnapshotis misnamedIt does not just encode: it also syncs, chmods, and closes the file. Rename to
writeSnapshotFile(or similar) so the name matches the responsibility.3.
game/autosave_test.gousest.Errorwheret.Fatalis correctIn the shell-escape test, a failed precondition should stop the test rather
than let it continue into assertions that cannot be meaningful.
Definition of done
TODO.md:99no longer asserts current behaviour that is false.encodeSnapshotrenamed to something that reflects that it writes, syncs,chmods, and closes; all call sites and any doc comments referencing it
updated.
t.Error→t.Fatalfix applied where the assertion is a precondition.make checkfully green.TODO.mdupdated in the same commit — Completed Steps entry, and do notrotate "Next Step".
(closes #N).Implementation requirements
TODO.md,game/save.go, andgame/autosave_test.go. If the rename pulls in more files, that is fine —but nothing else should change behaviour.
fix. If anything else seems necessary, stop and report.
pendingSaver, service-point, or atomic-write logic settled in PRs #23 and #26.
lock across ~18 concurrent sessions. Run
make lint/make checkin a retryloop and accept only a run that neither reports
parallel golangci-lint is runningnor mentions paths outside your ownworktree. A false green has already happened in this repo.
maketargets only. Do NOT modify.golangci.yml. No Dockerfile/CI/script/.game/testdata/.c-masterandmodern-roguealone.Depends on
PR #26 (#24) — land that first; all three items are against code it
introduces.
Priority
Low. Item 1 is the only one with any real cost, and it is the same
false-claim-in-
TODO.mdfailure mode this repo has now hit repeatedly.PR #26 has landed (
3a01283, merged as85354f2), so this is now unblocked.Adding a fourth item, from the final review (recorded there as N2):
game/save.go:815leaves a stub comment line —// The result is still a—at roughly a third the width of its neighbours, left behind by an edit to
the surrounding paragraph.
gofmtdoes not rewrap comments, sofmt-checkis legitimately green and nothing catches it. Rewrap the paragraph.
Two corrections to this issue's original text, both now settled by the
review rounds on #26:
TODO.md:99's entry asserts in the present tense thatAutoSaveencodes live state after removing the file. Re-check the currentline number before editing —
TODO.mdhas been reflowed bymake fmttwice since this issue was filed, so the line has moved. Find it by content,
not by number.
113 reports→over a hundred reports), so do not re-do that.A warning worth heeding on this issue specifically. PR #26 failed five
consecutive review rounds, and every single failure was a confidently-worded
claim that nobody had verified — including two asserted by me, and one
malformed done-criterion of mine that the implementer was right to refuse.
Item 1 here is precisely that genre: a claim about what a
TODO.mdentryasserts. Read the entry and the current code before rewording it. If this
issue's description turns out to be wrong when you check it, say so and stop
rather than writing something you cannot stand behind.
Also note
grep-based done-criteria onTODO.mdare treacherous: a searchfor
113still matches the linter nameerr113in the 2026-07-06 entry,which is pre-existing and must not be touched.
Plan (verified against the tree at
85354f2before writing anything)All four items check out as described. What I found, item by item:
1. The false
TODO.mdentry — found by content, not line number. It isnot in the
fix/autosave-race(#24) entry, which describes the oldbehaviour correctly in the past tense ("
AutoSaveremoved the save filebefore encoding"). It is in the 2026-08-09
sig-leave(closes #12) entry,now at lines 118-121, in the paragraph justifying the SIGINT/SIGQUIT no-save
decision:
> It is also the safe choice:
AutoSavegob-encodes live state that the main> goroutine is still mutating, after removing the old file, so on the signals
> with nothing to rescue the port takes the option with no corruption window.
Present tense, and both halves are false as of #26: the encode is on the game
goroutine, and
saveFileis CreateTemp → Sync → Chmod → Rename with noRemove. I will put it in the past tense and add an explicit supersessionpointing at the
fix/autosave-raceentry (which sits above it, newestfirst), noting that the decision now rests on C and semantics alone — matching
what the current
savesOnSignaldoc comment incmd/rogue/main.goalreadysays. I will not touch the
113/err113text or the 2026-07-06 entry.2.
encodeSnapshot→writeSnapshotFile. Confirmed: it encodes, syncs,chmods and closes (
game/save.go:694). One call site (saveFile,game/save.go:673) and its own doc comment atgame/save.go:690; a repo-widegrep finds no other reference, in Go or Markdown. Rename by hand with Edit.
3.
t.Error→t.Fatal. Confirmed atgame/autosave_test.go:155(theissue said ~151; the
ifis at 154 and the call at 155), inTestAutoSaveOnSignalWhileInShellEscape. It is a precondition: the very nextstatement is
assertRestorable(t, g.FileName), which is meaningless if thesave was never taken. The sibling test at line 116 already uses
t.Fatalforthe identical assertion, so this is also an inconsistency.
4. The stub comment line. Confirmed:
game/save.go:815is// The result is still aat 24 columns where its neighbours run 65-73. Rewrap that paragraphonly, to the same width as the rest of the block.
Verification
Branch
cleanup/pr26-followupsoffmain@85354f2in a throwaway worktree.make fmtthenmake check(fmt-check,lint,testwith-timeout 30s -race -cover), withGOLANGCI_LINT_CACHEpointed at a freshempty private directory and the output checked for foreign paths. No goldens
regenerated,
.golangci.ymluntouched,Next Stepnot rotated.