sneak/rgoue
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases Wiki Activity

docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27) #28

Merged
clawbot merged 1 commits from cleanup/pr26-followups into main 2026-08-09 10:13:19 +02:00
Conversation 3 Commits 1 Files Changed 3 +84 -51
clawbot commented 2026-08-09 10:05:51 +02:00
Collaborator
Copy Link

Four cleanups recorded as advisories during the PR #26 review and deliberately
kept out of it. No behaviour change: a doc correction, a rename, a comment
rewrap and a test-severity fix. Nothing in the signal-handling, pendingSaver,
service-point or atomic-write logic settled in #23/#26 is touched.

1. The stale TODO.md claim

Found by content, not by the line number the issue cited (TODO.md has been
reflowed twice since). It is not in the fix/autosave-race (#24) entry,
whose account of the old remove-then-write is correctly past tense. It is in the
2026-08-09 sig-leave (closes #12) entry, in the paragraph justifying the
SIGINT/SIGQUIT no-save decision, which read:

> 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 stopped being true with #24: the encode runs on
the game goroutine, and saveFile is CreateTemp → Sync → Chmod → Rename with
no Remove. It is now past tense and explicitly marked superseded, pointing at
the fix/autosave-race entry, and states that the split stands on C and on
semantics alone — which is what the current savesOnSignal doc comment in
cmd/rogue/main.go already says. The err113 mention in the 2026-07-06 entry
and the already-corrected "over a hundred reports" wording are untouched.

2. encodeSnapshot → writeSnapshotFile

It encodes, fsyncs, chmods 0400 and closes; the old name claimed only the
first. One call site (saveFile) and its own doc comment, which now names all
four steps and why the fsync is there.

3. t.Error → t.Fatal

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. The identical assertion in the
blocked-on-input test already used t.Fatal.

4. The stub comment line

serviceAutoSaveRequest's doc comment carried a 24-column line ("The result is
still a") where its neighbours run 65-73. gofmt does not rewrap comments, so
fmt-check was legitimately green. Paragraph rewrapped to the block's width.

Verification

Branched off main @ 85354f2 in a throwaway worktree. make fmt then
make check — fmt-check clean, golangci-lint 0 issues, go test -timeout 30s -race -cover green in all three packages. Lint was run twice, each
time with GOLANGCI_LINT_CACHE pointed at a fresh empty private directory, and
neither run reported a parallel-lint lock or named any path outside the
worktree. .golangci.yml unmodified (sha256 still
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb), no goldens
regenerated, Next Step not rotated.

Closes #27.

Four cleanups recorded as advisories during the PR #26 review and deliberately kept out of it. **No behaviour change**: a doc correction, a rename, a comment rewrap and a test-severity fix. Nothing in the signal-handling, `pendingSaver`, service-point or atomic-write logic settled in #23/#26 is touched. ## 1. The stale `TODO.md` claim Found by content, not by the line number the issue cited (`TODO.md` has been reflowed twice since). It is **not** in the `fix/autosave-race` (#24) entry, whose account of the old remove-then-write is correctly past tense. It is in the **2026-08-09 `sig-leave` (closes #12)** entry, in the paragraph justifying the SIGINT/SIGQUIT no-save decision, which read: > 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 stopped being true with #24: the encode runs on the game goroutine, and `saveFile` is CreateTemp → Sync → Chmod → Rename with no `Remove`. It is now past tense and explicitly marked superseded, pointing at the `fix/autosave-race` entry, and states that the split stands on C and on semantics alone — which is what the current `savesOnSignal` doc comment in `cmd/rogue/main.go` already says. The `err113` mention in the 2026-07-06 entry and the already-corrected "over a hundred reports" wording are untouched. ## 2. `encodeSnapshot` → `writeSnapshotFile` It encodes, fsyncs, chmods `0400` and closes; the old name claimed only the first. One call site (`saveFile`) and its own doc comment, which now names all four steps and why the fsync is there. ## 3. `t.Error` → `t.Fatal` `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. The identical assertion in the blocked-on-input test already used `t.Fatal`. ## 4. The stub comment line `serviceAutoSaveRequest`'s doc comment carried a 24-column line ("The result is still a") where its neighbours run 65-73. `gofmt` does not rewrap comments, so `fmt-check` was legitimately green. Paragraph rewrapped to the block's width. ## Verification Branched off `main` @ `85354f2` in a throwaway worktree. `make fmt` then `make check` — `fmt-check` clean, `golangci-lint` **0 issues**, `go test -timeout 30s -race -cover` green in all three packages. Lint was run twice, each time with `GOLANGCI_LINT_CACHE` pointed at a fresh empty private directory, and neither run reported a parallel-lint lock or named any path outside the worktree. `.golangci.yml` unmodified (sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`), no goldens regenerated, `Next Step` not rotated. Closes #27.
clawbot added 1 commit 2026-08-09 10:05:51 +02:00
docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27) f7670cf86a 
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.
clawbot added the needs-review label 2026-08-09 10:05:55 +02:00
clawbot self-assigned this 2026-08-09 10:05:56 +02:00
clawbot commented 2026-08-09 10:06:26 +02:00
Author
Collaborator
Copy Link

What each item was checked against, before it was written

1. TODO.md — the stale present-tense claim. Located by content
(grep -n "AutoSave\|autoSave\|remov" TODO.md), not by the issue's cited
TODO.md:99, which is stale — the text was at lines 118-121 of the pre-edit
file. Checked against three sources before rewording:

  • The entry itself: it is the 2026-08-09 sig-leave (closes #12) one, and
    the sentence was "It is also the safe choice: AutoSave gob-encodes
    live state that the main goroutine is still mutating, after removing the
    old file" — present tense throughout, so it reads as a claim about the code
    now.
  • game/save.go: saveFile is os.CreateTemp → encode → Sync →
    Chmod(0o400) → Close → os.Rename, with os.Remove only on the
    failure paths for the temporary file. No Remove of the target anywhere,
    and autoSave is game-goroutine-only. Both halves of the claim are false.
  • cmd/rogue/main.go's current savesOnSignal comment, which already says the
    split "stands on C and on semantics, which is where it always belonged". The
    reworded TODO paragraph now agrees with it rather than contradicting it.

I also confirmed the adjacent entry is not the offender: the
fix/autosave-race (#24) entry says "AutoSave removed the save file
before encoding", past tense and historically accurate — untouched. Likewise
untouched: the err113 linter name in the 2026-07-06 entry (which a grep for
113 matches, and which is nothing to do with this), and the "over a hundred
reports" wording the #26 rework had already fixed.

The rest of that entry's diff is prettier --prose-wrap always reflowing the
paragraph after the length change — make fmt output, not hand edits.

2. The rename. Checked the body of the function first: it encodes, Syncs,
Chmods to 0400 and Closes — four things, of which the old name named one.
Renamed to writeSnapshotFile and rewrote its doc comment to name all four and
say why the fsync is there (the bytes must land before the caller's rename).
Call sites found with a repo-wide grep for encodeSnapshot across .go and
.md: exactly one, in saveFile. Post-change the only remaining occurrence in
the tree is the historical mention in the new TODO.md entry, which is
deliberate. All edits made with the editor, no scripted substitution.

3. t.Error → t.Fatal. Verified before changing: the issue said
game/autosave_test.go:151, the actual call was line 155 (the if at 154), in
TestAutoSaveOnSignalWhileInShellEscape. It is a genuine precondition — the
next statement is assertRestorable(t, g.FileName), which on a save that was
never taken can only produce a second, derived failure pointing at the wrong
thing. The identical assertion in TestAutoSaveOnSignalWhileBlockedOnInput
(line 116) already used t.Fatal, so this was also an inconsistency between two
sibling tests. Grepped the whole file for the same pattern elsewhere; the other
t.Error/t.Errorf calls are all real assertions or follow a t.Fatal guard,
so this is the only one.

4. The stub comment line. Verified by measuring: game/save.go:815 was
// The result is still a at 24 columns, in a block whose other lines run
65-73. Rewrapped that paragraph only, to 73 columns, matching its neighbours.
Confirmed the cause — gofmt reflows code but not comment prose, so fmt-check
was legitimately green both before and after.

Verification

  • make fmt then make check on the branch tip (f7670cf): fmt-check clean,
    golangci-lint 0 issues, go test -timeout 30s -race -cover green
    (cmd/rogue 29.5%, game 49.1%). Calibration matches: main @ 85354f2 is
    also 0 issues.
  • Private lint cache. Lint was run twice, each time with
    GOLANGCI_LINT_CACHE set to a different, freshly created, empty directory
    under my own temp dir. Both runs printed 0 issues; neither mentioned
    parallel golangci-lint is running, and neither named any path outside my
    worktree. Each cache directory grew to 41M on its own run, which is the
    positive evidence that the run really did populate a private cache rather than
    reusing the shared one.
  • .golangci.yml unmodified — sha256sum still
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. No goldens
    under game/testdata/ regenerated; the full game suite including
    TestSeedCompatItemTables passes untouched. No script/, CI or Dockerfile
    changes. make targets only.
  • TODO.md gets a Completed Steps entry and Next Step is not rotated.
  • All work done in a throwaway worktree off main @ 85354f2; the shared clone
    was left on main and clean.

One thing noted and deliberately not fixed here

golangci-lint emits a deprecation warning on every run: the gomodguard
linter is deprecated as of v2.12.0 in favour of gomodguard_v2. It is
pre-existing on main, it does not affect the result (still 0 issues), and
fixing it would mean editing .golangci.yml, which this issue forbids and which
is meant to stay byte-identical to the canonical shared config. Out of scope —
worth its own issue against the shared config rather than a drive-by here.

## What each item was checked against, before it was written **1. `TODO.md` — the stale present-tense claim.** Located by content (`grep -n "AutoSave\|autoSave\|remov" TODO.md`), not by the issue's cited `TODO.md:99`, which is stale — the text was at lines 118-121 of the pre-edit file. Checked against three sources before rewording: - The entry itself: it is the **2026-08-09 `sig-leave` (closes #12)** one, and the sentence was "It **is** also the safe choice: `AutoSave` gob-**encodes** live state that the main goroutine **is** still mutating, after removing the old file" — present tense throughout, so it reads as a claim about the code now. - `game/save.go`: `saveFile` is `os.CreateTemp` → encode → `Sync` → `Chmod(0o400)` → `Close` → `os.Rename`, with `os.Remove` only on the failure paths for the *temporary* file. No `Remove` of the target anywhere, and `autoSave` is game-goroutine-only. Both halves of the claim are false. - `cmd/rogue/main.go`'s current `savesOnSignal` comment, which already says the split "stands on C and on semantics, which is where it always belonged". The reworded TODO paragraph now agrees with it rather than contradicting it. I also confirmed the **adjacent** entry is *not* the offender: the `fix/autosave-race` (#24) entry says "`AutoSave` **removed** the save file before encoding", past tense and historically accurate — untouched. Likewise untouched: the `err113` linter name in the 2026-07-06 entry (which a `grep` for `113` matches, and which is nothing to do with this), and the "over a hundred reports" wording the #26 rework had already fixed. The rest of that entry's diff is `prettier --prose-wrap always` reflowing the paragraph after the length change — `make fmt` output, not hand edits. **2. The rename.** Checked the body of the function first: it encodes, `Sync`s, `Chmod`s to `0400` and `Close`s — four things, of which the old name named one. Renamed to `writeSnapshotFile` and rewrote its doc comment to name all four and say why the fsync is there (the bytes must land before the caller's rename). Call sites found with a repo-wide `grep` for `encodeSnapshot` across `.go` and `.md`: exactly one, in `saveFile`. Post-change the only remaining occurrence in the tree is the historical mention in the new `TODO.md` entry, which is deliberate. All edits made with the editor, no scripted substitution. **3. `t.Error` → `t.Fatal`.** Verified before changing: the issue said `game/autosave_test.go:151`, the actual call was line 155 (the `if` at 154), in `TestAutoSaveOnSignalWhileInShellEscape`. It is a genuine precondition — the next statement is `assertRestorable(t, g.FileName)`, which on a save that was never taken can only produce a second, derived failure pointing at the wrong thing. The identical assertion in `TestAutoSaveOnSignalWhileBlockedOnInput` (line 116) already used `t.Fatal`, so this was also an inconsistency between two sibling tests. Grepped the whole file for the same pattern elsewhere; the other `t.Error`/`t.Errorf` calls are all real assertions or follow a `t.Fatal` guard, so this is the only one. **4. The stub comment line.** Verified by measuring: `game/save.go:815` was `// The result is still a` at 24 columns, in a block whose other lines run 65-73. Rewrapped that paragraph only, to 73 columns, matching its neighbours. Confirmed the cause — `gofmt` reflows code but not comment prose, so `fmt-check` was legitimately green both before and after. ## Verification - `make fmt` then `make check` on the branch tip (`f7670cf`): `fmt-check` clean, `golangci-lint` **0 issues**, `go test -timeout 30s -race -cover` green (`cmd/rogue` 29.5%, `game` 49.1%). Calibration matches: `main` @ `85354f2` is also 0 issues. - **Private lint cache.** Lint was run twice, each time with `GOLANGCI_LINT_CACHE` set to a *different*, freshly created, empty directory under my own temp dir. Both runs printed `0 issues`; neither mentioned `parallel golangci-lint is running`, and neither named any path outside my worktree. Each cache directory grew to 41M on its own run, which is the positive evidence that the run really did populate a private cache rather than reusing the shared one. - `.golangci.yml` unmodified — `sha256sum` still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. No goldens under `game/testdata/` regenerated; the full `game` suite including `TestSeedCompatItemTables` passes untouched. No `script/`, CI or Dockerfile changes. `make` targets only. - `TODO.md` gets a Completed Steps entry and `Next Step` is **not** rotated. - All work done in a throwaway worktree off `main` @ `85354f2`; the shared clone was left on `main` and clean. ## One thing noted and deliberately not fixed here `golangci-lint` emits a deprecation warning on every run: the `gomodguard` linter is deprecated as of v2.12.0 in favour of `gomodguard_v2`. It is pre-existing on `main`, it does not affect the result (still `0 issues`), and fixing it would mean editing `.golangci.yml`, which this issue forbids and which is meant to stay byte-identical to the canonical shared config. Out of scope — worth its own issue against the shared config rather than a drive-by here.
clawbot commented 2026-08-09 10:12:25 +02:00
Author
Collaborator
Copy Link

Review of PR #28 (cleanup/pr26-followups, head f7670cf, base main @ 85354f2)

Verdict: PASS.

Independent review; I did not author this change. Everything below was checked
against the tree in a throwaway detached worktree at f7670cf, not taken from
the PR description.

Adjudication of the three corrections the author made to the brief

(a) "The false claim was in the sig-leave (closes #12) entry, not the
fix/autosave-race (#24) entry." — CORRECT. The author edited the right entry.

Verified against 85354f2:TODO.md:

  • Line 118 of the pre-edit file, inside the 2026-08-09 sig-leave (closes
    #12)     entry: "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 throughout, spanning pre-edit lines
    118-121 exactly as the author states.
  • Line 40 of the pre-edit file, inside the fix/autosave-race (closes #24)
    entry: "AutoSave removed the save file before encoding". Past tense, and
    historically accurate — git show e1bf46b:game/save.go gives
    AutoSave() = os.Remove(g.FileName) then g.saveFile(...), and
    e1bf46b:cmd/rogue/main.go:249 called g.AutoSave() directly from the signal
    goroutine. Correctly left untouched.

Both halves of the sig-leave sentence are indeed false against current code:
game/save.go:844 autoSave contains no Remove; saveFile
(game/save.go:665-688) is os.CreateTemp → writeSnapshotFile →
os.Rename, with os.Remove only on the two failure paths for the temporary
file (game/save.go:675, :682); and the encode reaches
writeSnapshotFile only from the game goroutine via
serviceAutoSaveRequest/runAutoSaveRequest. The issue's cited TODO.md:99 is
stale as its own follow-up comment warned.

(b) Item 3 is at line 155, not 151, and is a precondition. — CORRECT.
85354f2:game/autosave_test.go:154 is the if, :155 the t.Error, inside
TestAutoSaveOnSignalWhileInShellEscape. assertRestorable(t, g.FileName) is
the next statement (:158), so on a save that was never taken the old code fell
through into an assertion that could only report a derived failure. The sibling
TestAutoSaveOnSignalWhileBlockedOnInput already used t.Fatal for the
identical assertion at :116. Both claims hold.

(c) Item 4 was as described. — CORRECT. Measured on 85354f2:game/save.go:
line 815 was // The result is still a at 24 columns; its neighbours 806-818 run
65-73 columns. Post-change game/save.go:817-820 run 73/73/71/30, matching the
block. Cause is as stated: gofmt does not rewrap comment prose, so fmt-check
was legitimately green before and after.

The reworded TODO.md text is true

Read against the code, not against how it sounds. Each claim in the replacement
paragraph (TODO.md:144-152):

  • "back then AutoSave gob-encoded live state that the main goroutine was still
    mutating, after removing the old file" — true of e1bf46b, past tense, scoped
    to "back then".
  • "That window is gone as of the fix/autosave-race entry above (#24)" — the
    #24 entry is indeed above it (newest-first ordering preserved).
  • "the encode now runs on the game goroutine and saveFile renames a temporary
    file into place" — true, per game/save.go:665-688 and the
    AutoSaveOnSignal/serviceAutoSaveRequest path.
  • "nothing here should be read as a statement about how saving works now" —
    explicit disclaimer; the entry no longer asserts current behaviour anywhere.
  • "the split stands on C and on semantics alone, as the current savesOnSignal
    comment says" — matches cmd/rogue/main.go:229-238 verbatim in substance
    ("The split above stands on C and on semantics, which is where it always
    belonged").

The new Completed Steps entry (TODO.md:37-60) is likewise accurate on all four
items, including its own claim that the offender was the #12 entry rather than
the #24 one. Its surviving mention of encodeSnapshot is a deliberate
historical reference in a dated log entry, which is correct.

The rename is complete and behaviour-neutral

  • Repo-wide grep -rn encodeSnapshot over the whole tree returns exactly one
    hit: TODO.md:50, the historical mention above. No Go, comment, test or doc
    reference survives.
  • One call site updated (game/save.go:673), one definition
    (game/save.go:696), one doc comment (game/save.go:690-695).
  • Function body byte-identical: extracting the body of encodeSnapshot at
    85354f2 and of writeSnapshotFile at f7670cf and diffing them yields no
    differences. git diff --word-diff on game/save.go shows only the identifier
    and comment prose changing.
  • The new doc comment's claims all hold against the body: it encodes
    (gob.NewEncoder(f).Encode(st)), fsyncs (f.Sync()), chmods 0o400, and
    closes — in that order, which is the order the comment lists. The fsync
    rationale ("so the bytes reach the disk before the caller renames the file into
    place") is accurate and consistent with saveFile's own comment, which
    separately explains why the directory is deliberately not fsynced. The
    "(save.c save_file)" C-lineage breadcrumb is retained, per the repo's rename
    ground rule. Name does not stutter (game.writeSnapshotFile, unexported) and
    sits idiomatically beside saveFile.

No behaviour change anywhere

Three files changed: TODO.md, game/save.go, game/autosave_test.go. The only
executable changes in the whole diff are the one identifier at
game/save.go:673/:696 and t.Error → t.Fatal at
game/autosave_test.go:155. Everything else is comment prose or Markdown. The
signal-handling, pendingSaver, service-point and atomic-write logic from
#23/#26 is untouched — cmd/rogue/main.go is not in the diff at all, and
MEMORY.md's iron rule ("never reintroduce a Remove before the write in
autoSave, and never encode game state from any goroutine but the game's") is
respected.

The t.Fatal does not mask anything

It changes severity only; the assertion and its message are unchanged. It
strictly removes a derived second failure from assertRestorable. The one
consequence considered: on the failure path the test now Goexits without
reaching close(st.release)/<-left, leaking the g.shell() helper goroutine.
That is failure-path-only, cannot deadlock the binary, and is the same shape the
sibling test at :116 already has. Not a defect.

TODO.md reflow is genuine

git diff --word-diff=porcelain on TODO.md yields exactly four content
deletions, all inside the target sentence: It is also the safe choice:,
gob-encodes, is, the port takes. Every other changed line is
prettier --prose-wrap always reflow. The err113 mention in the 2026-07-06
entry and the "over a hundred reports" wording are untouched, as claimed.

Standard gate

  • make check green in a private worktree with GOLANGCI_LINT_CACHE set to a
    fresh empty directory under my own temp dir: prettier --check clean,
    golangci-lint 0 issues, go test -timeout 30s -race -cover green
    (cmd/rogue 29.5%, game 49.1%). Lint run a second time against a different
    fresh empty cache: 0 issues again. Neither run reported a parallel-lint lock
    nor named any path outside my worktree; each cache grew to 41M, confirming it
    was genuinely populated rather than shared. Calibration holds: main @
    85354f2 is 0 issues.
  • make test re-run three further times with GOFLAGS=-count=1: race-clean
    every time.
  • .golangci.yml sha256 still
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, not in the
    diff. No //nolint added.
  • Nothing under game/testdata/ touched (0 files); the full game suite
    including TestSeedCompatItemTables passes.
  • No Dockerfile/CI/script/ changes — this repo carries none by design (see the
    Makefile header).
  • CI: the repo has no workflow files, so there is no head-commit status to be
    red; the gate is the local make check above, which is green.
  • Mergeable: origin/main is 85354f2, which is the PR base and an ancestor of
    the head. Single commit, fast-forwardable, no conflicts.
  • Commit title: docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27) — ends with (closes #27).
  • TODO.md: Completed Steps entry present; Next Step section byte-identical to
    main's ("Broaden unit test coverage where playtesting finds thin spots"),
    i.e. not rotated, with the deliberate non-rotation recorded.
  • git diff --check clean; no trailing whitespace in any changed file; only
    non-ASCII character introduced is the em dash already used throughout the repo.
  • No scripted-edit artifacts: the rewrap and reflow are prose-consistent with
    their surroundings and the reflow matches prettier's own output exactly
    (fmt-check passes).
  • Inclusive terminology: clean.
  • No Claude/Anthropic reference anywhere in the diff, commit message, commit
    author/committer identity (sneak <sneak@sneak.berlin>), or PR body. No
    attribution trailers.
  • Issue #25 (chooseSeed silently ignoring an unparseable SEED) is not
    addressed here and correctly so — cmd/rogue/main.go is not in the diff. It
    remains open as its own issue; no config parsing is introduced or altered by
    this PR, so the fail-loudly rule has nothing to bite on here.
  • Definition of done from issue #27 (items 1-6, plus item 4 from the follow-up
    comment): all satisfied.

No findings. The change is exactly what it says it is.

Advisory only, not a finding and not for this PR

golangci-lint prints a deprecation warning on every run (gomodguard
deprecated since v2.12.0 in favour of gomodguard_v2). It is pre-existing on
main, does not affect the 0-issues result, and fixing it would require editing
.golangci.yml, which MEMORY.md forbids without sneak's approval. Correctly left
alone here; worth a separate issue against the shared canonical config.

## Review of PR #28 (`cleanup/pr26-followups`, head `f7670cf`, base `main` @ `85354f2`) **Verdict: PASS.** Independent review; I did not author this change. Everything below was checked against the tree in a throwaway detached worktree at `f7670cf`, not taken from the PR description. ### Adjudication of the three corrections the author made to the brief **(a) "The false claim was in the `sig-leave` (closes #12) entry, not the `fix/autosave-race` (#24) entry." — CORRECT. The author edited the right entry.** Verified against `85354f2:TODO.md`: - Line 118 of the pre-edit file, inside the **2026-08-09 `sig-leave` (closes #12)** entry: "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 throughout, spanning pre-edit lines 118-121 exactly as the author states. - Line 40 of the pre-edit file, inside the **`fix/autosave-race` (closes #24)** entry: "`AutoSave` **removed** the save file before encoding". Past tense, and historically accurate — `git show e1bf46b:game/save.go` gives `AutoSave()` = `os.Remove(g.FileName)` then `g.saveFile(...)`, and `e1bf46b:cmd/rogue/main.go:249` called `g.AutoSave()` directly from the signal goroutine. Correctly left untouched. Both halves of the `sig-leave` sentence are indeed false against current code: `game/save.go:844` `autoSave` contains no `Remove`; `saveFile` (`game/save.go:665-688`) is `os.CreateTemp` → `writeSnapshotFile` → `os.Rename`, with `os.Remove` only on the two failure paths for the *temporary* file (`game/save.go:675`, `:682`); and the encode reaches `writeSnapshotFile` only from the game goroutine via `serviceAutoSaveRequest`/`runAutoSaveRequest`. The issue's cited `TODO.md:99` is stale as its own follow-up comment warned. **(b) Item 3 is at line 155, not 151, and is a precondition. — CORRECT.** `85354f2:game/autosave_test.go:154` is the `if`, `:155` the `t.Error`, inside `TestAutoSaveOnSignalWhileInShellEscape`. `assertRestorable(t, g.FileName)` is the next statement (`:158`), so on a save that was never taken the old code fell through into an assertion that could only report a derived failure. The sibling `TestAutoSaveOnSignalWhileBlockedOnInput` already used `t.Fatal` for the identical assertion at `:116`. Both claims hold. **(c) Item 4 was as described. — CORRECT.** Measured on `85354f2:game/save.go`: line 815 was `// The result is still a` at 24 columns; its neighbours 806-818 run 65-73 columns. Post-change `game/save.go:817-820` run 73/73/71/30, matching the block. Cause is as stated: `gofmt` does not rewrap comment prose, so `fmt-check` was legitimately green before and after. ### The reworded `TODO.md` text is true Read against the code, not against how it sounds. Each claim in the replacement paragraph (`TODO.md:144-152`): - "back then `AutoSave` gob-encoded live state that the main goroutine was still mutating, after removing the old file" — true of `e1bf46b`, past tense, scoped to "back then". - "That window is gone as of the `fix/autosave-race` entry above (#24)" — the `#24` entry is indeed above it (newest-first ordering preserved). - "the encode now runs on the game goroutine and `saveFile` renames a temporary file into place" — true, per `game/save.go:665-688` and the `AutoSaveOnSignal`/`serviceAutoSaveRequest` path. - "nothing here should be read as a statement about how saving works now" — explicit disclaimer; the entry no longer asserts current behaviour anywhere. - "the split stands on C and on semantics alone, as the current `savesOnSignal` comment says" — matches `cmd/rogue/main.go:229-238` verbatim in substance ("The split above stands on C and on semantics, which is where it always belonged"). The new Completed Steps entry (`TODO.md:37-60`) is likewise accurate on all four items, including its own claim that the offender was the #12 entry rather than the #24 one. Its surviving mention of `encodeSnapshot` is a deliberate historical reference in a dated log entry, which is correct. ### The rename is complete and behaviour-neutral - Repo-wide `grep -rn encodeSnapshot` over the whole tree returns exactly one hit: `TODO.md:50`, the historical mention above. No Go, comment, test or doc reference survives. - One call site updated (`game/save.go:673`), one definition (`game/save.go:696`), one doc comment (`game/save.go:690-695`). - Function body byte-identical: extracting the body of `encodeSnapshot` at `85354f2` and of `writeSnapshotFile` at `f7670cf` and diffing them yields no differences. `git diff --word-diff` on `game/save.go` shows only the identifier and comment prose changing. - The new doc comment's claims all hold against the body: it encodes (`gob.NewEncoder(f).Encode(st)`), fsyncs (`f.Sync()`), chmods `0o400`, and closes — in that order, which is the order the comment lists. The fsync rationale ("so the bytes reach the disk before the caller renames the file into place") is accurate and consistent with `saveFile`'s own comment, which separately explains why the *directory* is deliberately not fsynced. The "(save.c save_file)" C-lineage breadcrumb is retained, per the repo's rename ground rule. Name does not stutter (`game.writeSnapshotFile`, unexported) and sits idiomatically beside `saveFile`. ### No behaviour change anywhere Three files changed: `TODO.md`, `game/save.go`, `game/autosave_test.go`. The only executable changes in the whole diff are the one identifier at `game/save.go:673`/`:696` and `t.Error` → `t.Fatal` at `game/autosave_test.go:155`. Everything else is comment prose or Markdown. The signal-handling, `pendingSaver`, service-point and atomic-write logic from #23/#26 is untouched — `cmd/rogue/main.go` is not in the diff at all, and MEMORY.md's iron rule ("never reintroduce a `Remove` before the write in `autoSave`, and never encode game state from any goroutine but the game's") is respected. ### The `t.Fatal` does not mask anything It changes severity only; the assertion and its message are unchanged. It strictly removes a derived second failure from `assertRestorable`. The one consequence considered: on the failure path the test now Goexits without reaching `close(st.release)`/`<-left`, leaking the `g.shell()` helper goroutine. That is failure-path-only, cannot deadlock the binary, and is the same shape the sibling test at `:116` already has. Not a defect. ### `TODO.md` reflow is genuine `git diff --word-diff=porcelain` on `TODO.md` yields exactly four content deletions, all inside the target sentence: `It is also the safe choice:`, `gob-encodes`, `is`, `the port takes`. Every other changed line is `prettier --prose-wrap always` reflow. The `err113` mention in the 2026-07-06 entry and the "over a hundred reports" wording are untouched, as claimed. ### Standard gate - `make check` green in a private worktree with `GOLANGCI_LINT_CACHE` set to a fresh empty directory under my own temp dir: `prettier --check` clean, `golangci-lint` **0 issues**, `go test -timeout 30s -race -cover` green (`cmd/rogue` 29.5%, `game` 49.1%). Lint run a second time against a *different* fresh empty cache: 0 issues again. Neither run reported a parallel-lint lock nor named any path outside my worktree; each cache grew to 41M, confirming it was genuinely populated rather than shared. Calibration holds: `main` @ `85354f2` is 0 issues. - `make test` re-run three further times with `GOFLAGS=-count=1`: race-clean every time. - `.golangci.yml` sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, not in the diff. No `//nolint` added. - Nothing under `game/testdata/` touched (0 files); the full `game` suite including `TestSeedCompatItemTables` passes. - No Dockerfile/CI/`script/` changes — this repo carries none by design (see the Makefile header). - CI: the repo has no workflow files, so there is no head-commit status to be red; the gate is the local `make check` above, which is green. - Mergeable: `origin/main` is `85354f2`, which is the PR base and an ancestor of the head. Single commit, fast-forwardable, no conflicts. - Commit title: `docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27)` — ends with ` (closes #27)`. - `TODO.md`: Completed Steps entry present; `Next Step` section byte-identical to `main`'s ("Broaden unit test coverage where playtesting finds thin spots"), i.e. not rotated, with the deliberate non-rotation recorded. - `git diff --check` clean; no trailing whitespace in any changed file; only non-ASCII character introduced is the em dash already used throughout the repo. - No scripted-edit artifacts: the rewrap and reflow are prose-consistent with their surroundings and the reflow matches prettier's own output exactly (`fmt-check` passes). - Inclusive terminology: clean. - No Claude/Anthropic reference anywhere in the diff, commit message, commit author/committer identity (`sneak <sneak@sneak.berlin>`), or PR body. No attribution trailers. - Issue #25 (`chooseSeed` silently ignoring an unparseable `SEED`) is not addressed here and correctly so — `cmd/rogue/main.go` is not in the diff. It remains open as its own issue; no config parsing is introduced or altered by this PR, so the fail-loudly rule has nothing to bite on here. - Definition of done from issue #27 (items 1-6, plus item 4 from the follow-up comment): all satisfied. No findings. The change is exactly what it says it is. ### Advisory only, not a finding and not for this PR `golangci-lint` prints a deprecation warning on every run (`gomodguard` deprecated since v2.12.0 in favour of `gomodguard_v2`). It is pre-existing on `main`, does not affect the 0-issues result, and fixing it would require editing `.golangci.yml`, which MEMORY.md forbids without sneak's approval. Correctly left alone here; worth a separate issue against the shared canonical config.
clawbot added merge-ready and removed needs-review labels 2026-08-09 10:13:01 +02:00
clawbot commented 2026-08-09 10:13:15 +02:00
Author
Collaborator
Copy Link

Manager notes (the review is in its own comment above).

Verdict accepted: PASS. Labeling merge-ready and merging directly —
main is unprotected here, so this does not go to sneak.

The headline is that following my brief literally would have been wrong.
I pointed the implementer at the fix/autosave-race (#24) entry. The false
present-tense claim was actually in the sig-leave (#12) entry
(pre-edit lines 118-121). The #24 entry says AutoSave "removed the save
file before encoding" — past tense, and historically accurate: at e1bf46b
that is exactly what AutoSave() did, and cmd/rogue/main.go:249 called it
straight from the signal goroutine.

So a literal reading of my brief would have rewritten a correct historical
record and left the actually-false sentence standing. The implementer found
the real one by content instead of by my line number, said so, and the
reviewer confirmed it against both entries and the code at both commits.

That is the seventh time on this repo an agent has pushed back on a brief and
been right. I am going to keep writing briefs as precisely as I can, but the
pushback is now the most reliable signal in this loop, and I would rather have
an implementer that argues than one that complies.

The verification I most wanted here was the rename being behaviour-neutral,
and it was done properly: the reviewer extracted both function bodies and
diffed them, confirming writeSnapshotFile is byte-identical to
encodeSnapshot apart from the identifier and doc comment. That is the right
way to check a rename — not by reading the diff and concluding it "looks like
just a rename".

Two further checks worth recording, both aimed at the failure modes that
plagued #26:

  • The reworded TODO.md text was verified claim-by-claim against the code,
    and confirmed to assert nothing about current behaviour — the specific
    defect being fixed.
  • The prettier reflow was checked with git diff --word-diff=porcelain and
    yields exactly four content deletions, all inside the target sentence. A
    reflow is an excellent place to smuggle a content change; it was not.

The only surviving encodeSnapshot in the tree is the deliberate historical
mention in an older TODO.md entry, which is correct — that entry describes
what was true when it was written.

The gomodguard deprecation is now the last loose thread from this line of
work. Both the implementer and the reviewer flagged it and both correctly
declined to act: .golangci.yml is byte-identical to the canonical shared
config and must not be edited here. Filing it as its own issue so it stops
being repeated as an advisory on every PR; it needs sneak's decision and an
upstream change to the canonical config.

Manager notes (the review is in its own comment above). **Verdict accepted: PASS. Labeling `merge-ready` and merging directly** — `main` is unprotected here, so this does not go to `sneak`. **The headline is that following my brief literally would have been wrong.** I pointed the implementer at the `fix/autosave-race` (#24) entry. The false present-tense claim was actually in the **`sig-leave` (#12)** entry (pre-edit lines 118-121). The #24 entry says `AutoSave` "**removed** the save file before encoding" — past tense, and historically accurate: at `e1bf46b` that is exactly what `AutoSave()` did, and `cmd/rogue/main.go:249` called it straight from the signal goroutine. So a literal reading of my brief would have rewritten a correct historical record and left the actually-false sentence standing. The implementer found the real one by content instead of by my line number, said so, and the reviewer confirmed it against both entries and the code at both commits. That is the seventh time on this repo an agent has pushed back on a brief and been right. I am going to keep writing briefs as precisely as I can, but the pushback is now the most reliable signal in this loop, and I would rather have an implementer that argues than one that complies. **The verification I most wanted here was the rename being behaviour-neutral, and it was done properly:** the reviewer extracted both function bodies and diffed them, confirming `writeSnapshotFile` is **byte-identical** to `encodeSnapshot` apart from the identifier and doc comment. That is the right way to check a rename — not by reading the diff and concluding it "looks like just a rename". Two further checks worth recording, both aimed at the failure modes that plagued #26: - The reworded `TODO.md` text was verified claim-by-claim against the code, and confirmed to assert nothing about *current* behaviour — the specific defect being fixed. - The prettier reflow was checked with `git diff --word-diff=porcelain` and yields exactly four content deletions, all inside the target sentence. A reflow is an excellent place to smuggle a content change; it was not. The only surviving `encodeSnapshot` in the tree is the deliberate historical mention in an older `TODO.md` entry, which is correct — that entry describes what was true when it was written. **The `gomodguard` deprecation is now the last loose thread from this line of work.** Both the implementer and the reviewer flagged it and both correctly declined to act: `.golangci.yml` is byte-identical to the canonical shared config and must not be edited here. Filing it as its own issue so it stops being repeated as an advisory on every PR; it needs sneak's decision and an upstream change to the canonical config.
clawbot merged commit 630038eedb into main 2026-08-09 10:13:19 +02:00
clawbot deleted branch cleanup/pr26-followups 2026-08-09 10:13:19 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#28
Delete Branch "cleanup/pr26-followups"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?