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

Three cleanups deferred from the PR #26 review #27

New Issue
Closed
opened 2026-08-09 09:11:27 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 09:11:27 +02:00
Collaborator
Copy Link

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.

## 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.
clawbot commented 2026-08-09 10:01:44 +02:00
Author
Collaborator
Copy Link

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):

  1. 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.
clawbot commented 2026-08-09 10:03:35 +02:00
Author
Collaborator
Copy Link

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.

## 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#27
Delete Branch "%!s()"

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?