Restore three lost C behaviors: schtick message, forced redraw, greeting (closes #13) #32

Merged

clawbot merged 1 commits from fix/lost-c-behaviors into main 2026-08-09 12:14:00 +02:00

Conversation 3 Commits 1 Files Changed 15 +437 -12

clawbot commented 2026-08-09 12:05:06 +02:00

Collaborator

Copy Link

Closes #13.

Three behaviours from 5.4.4 that the port dropped silently, one commit
(the WIP checkpoint 3f0a14c is amended away, not preserved).

1. sticks.c 237 — the otherwise arm closing do_zap's switch
   prints "what a bizarre schtick!". It is under #ifdef MASTER, not
   under a runtime wizard test, so it is unconditional in this port,
   and WS_NOP is a case of the switch in its own right, so it stays
   silent and still spends a charge.
2. command.c 288-291 — CTRL('R') forces a full repaint through a
   new Terminal.Repaint (tcell Screen.Sync), not the diffing refresh.
3. main.c 107-113 — the startup greeting, both wordings, printed on
   stdout before the screen is taken.

Full detail is in the commit message and the TODO.md Completed Steps
entry. Next Step is deliberately not rotated (out-of-band issue work);
nothing here is deliberately dropped, so ARCHITECTURE.md section 9 is
unchanged, while 5.3 gains Repaint.

Two defects in the inherited WIP checkpoint were found and fixed —
see the PR comment below.

Closes #13. Three behaviours from 5.4.4 that the port dropped silently, one commit (the WIP checkpoint `3f0a14c` is amended away, not preserved). 1. **`sticks.c` 237** — the `otherwise` arm closing `do_zap`'s switch prints `"what a bizarre schtick!"`. It is under `#ifdef MASTER`, not under a runtime `wizard` test, so it is unconditional in this port, and `WS_NOP` is a case of the switch in its own right, so it stays silent and still spends a charge. 2. **`command.c` 288-291** — `CTRL('R')` forces a full repaint through a new `Terminal.Repaint` (tcell `Screen.Sync`), not the diffing refresh. 3. **`main.c` 107-113** — the startup greeting, both wordings, printed on stdout before the screen is taken. Full detail is in the commit message and the TODO.md Completed Steps entry. `Next Step` is deliberately not rotated (out-of-band issue work); nothing here is deliberately dropped, so ARCHITECTURE.md section 9 is unchanged, while 5.3 gains `Repaint`. **Two defects in the inherited WIP checkpoint were found and fixed** — see the PR comment below.

clawbot added 1 commit 2026-08-09 12:05:06 +02:00

Restore three lost C behaviors: schtick message, forced redraw, greeting (closes #13) ... 1142f43aed

Three behaviors from 5.4.4 that the port dropped silently. Each is a few
lines; grouped because they are all "restore something C did".

1. sticks.c 237: the "otherwise" arm closing do_zap's switch printed
   "what a bizarre schtick!", and doZap had turned it into doing nothing.
   The arm is under #ifdef MASTER, not under a runtime wizard test, so in
   the MASTER build this port is it printed for every player and must not
   be gated on g.Wizard. WS_NOP is a case of that switch in its own right
   ("when WS_NOP: break;"), so "no handler ran" cannot be the trigger:
   the wand of nothing does nothing quietly. C's switch covers all 14 WS_
   values, so its otherwise is reachable only for an o_which outside the
   table, which is what Object.hasValidWhich already screens for. All
   three arms fall through to obj.Charges--, as C's do.

2. command.c 288-291: CTRL('R') is "after = FALSE; clearok(curscr, TRUE);
   wrefresh(curscr);" — a forced full repaint. The port called
   g.refresh(), the ordinary diffing blit, which cannot fix the only
   situation the command exists for: a screen corrupted by another
   program's output leaves the game's record of it still correct, so the
   diff sends nothing. New Terminal.Repaint (tcell Screen.Sync, which
   discards tcell's record of the terminal rather than diffing against
   it), Screen.Repaint and g.repaint(), implemented in term.Tcell and in
   both headless test terminals. Named for the curses operation: the
   interface is the game's abstraction, not tcell's. It repaints what was
   last rendered — C repainted curscr, not stdscr — so it takes no
   window.

3. main.c 107-113: the startup greeting existed nowhere in the tree. New
   game.Greeting, printed on stdout by cmd/rogue/main.go before
   term.New(), the port's initscr(). Only the wizard wording is #ifdef
   MASTER; the other is unconditional. The %d is dnum, which main.c has
   just assigned to seed, so it is Params.Seed. Neither wording ends in a
   newline. Two placement details the tests pin: the printf sits after
   parse_opts, so a ROGUEOPTS name= is what the player is greeted by; and
   it sits after the -s/-d handling and after restore(), which never
   returns, so a resumed game does not announce that a dungeon is being
   dug (digsNewDungeon).

Greeting parses ROGUEOPTS into a throwaway game built the way New builds
the real one, tables and home directory included: ParseOpts handles every
option, not just the one the greeting reads, and inven= is matched
against inv_t_name[], which lives on the game.

All three message strings verified byte-for-byte against origin/c-master
sticks.c and main.c. No RNG call is added on any path and nothing under
game/testdata/ changed; TestSeedCompatItemTables is green against the
untouched golden.

Mutation-proved, each behavior removed in turn with only its own test
failing: dropping the message arm fails
TestZapUnhandledWandSaysBizarreSchtick; extending the message to WS_NOP
fails TestZapWandOfNothingIsSilent; putting g.refresh() back fails
TestRedrawCommandForcesFullRepaint; swapping the two wordings, and
ignoring the ROGUEOPTS name, both fail TestGreeting; greeting on the
restore path fails TestDigsNewDungeon.

ARCHITECTURE.md 5.3 gains Repaint and why a blit cannot substitute for
it; nothing here is deliberately dropped, so section 9 is unchanged.
TODO.md gets a Completed Steps entry; Next Step deliberately not rotated,
this being out-of-band issue work.

clawbot added the needs-review label 2026-08-09 12:05:19 +02:00

clawbot self-assigned this 2026-08-09 12:05:20 +02:00

clawbot commented 2026-08-09 12:05:55 +02:00

Author

Collaborator

Copy Link

Two defects in the inherited WIP checkpoint

The 3f0a14c checkpoint was treated as a starting point, not a foundation.
Both of these are fixed here.

1. Greeting crashed the game at startup for any player with inven= in
ROGUEOPTS. It parsed the options into a bare &RogueGame{Whoami: ...},
but ParseOpts handles every option, not just the name= the greeting
reads. The inven= arm matches the value against inv_t_name[]
(options.c parse_opts), which lives on g.data — nil on a bare game.
ROGUEOPTS="inven=slow" therefore nil-dereferenced before the player saw a
single character. Reproduced first as a failing test case, which is now a
regression test ("ROGUEOPTS inventory style parses without a fault"); the
throwaway game is now built the way New builds the real one, tables and
home directory included.

2. The checkpoint did not pass lint either, not only fmt-check. Two
issues: forbidigo on fmt.Print in cmd/rogue/main.go (the repo's
convention for stdout is _, _ = fmt.Fprint(os.Stdout, ...), as in
game/score.go and term/tcell.go), and goconst on the repeated account
name in greeting_test.go. Both fixed; the greeting keeps its lack of a
trailing newline. C's fflush(stdout) has no counterpart because
os.Stdout is unbuffered.

Everything else in the checkpoint was read line by line and stands.

C verification

Sources read via git show origin/c-master:... (never checked out), with
rogue.h's #define when break;case / #define otherwise break;default in
mind.

Message text, byte-for-byte. Each Go literal was matched with a
fixed-string grep against the exact C statement:

C source	C literal	Go site
sticks.c 237	msg("what a bizarre schtick!");	game/sticks.go
main.c 109 (MASTER)	printf("Hello %s, welcome to dungeon #%d", whoami, dnum);	game/game.go
main.c 112	printf("Hello %s, just a moment while I dig the dungeon...", whoami);	game/game.go

Placement and gating.

• The schtick arm sits inside #ifdef MASTER, not inside a runtime
  wizard test, so this MASTER-compiled port prints it for every player.
  It is not gated on g.Wizard — that would be #11's trap in reverse.
• WS_NOP is when WS_NOP: break;, a case of the switch in its own right,
  and all arms fall out into obj->o_charges--. C's switch covers all 14
  WS_ values (0..13, verified against rogue.h 294-307, which the Go
  WandKind enum matches one-for-one), so otherwise is reachable only for
  an o_which outside the table — exactly what Object.hasValidWhich
  screens for. WandNothing is the one kind with no handler and a valid
  Which, so the three-way switch needs no new state.
• command.c 288-291 is after = FALSE; clearok(curscr,TRUE); wrefresh(curscr); — no refresh() of stdscr, and the command loop
  refreshes before the next key read anyway.
• main.c: only the wizard arm is #ifdef MASTER; the normal wording is
  unconditional. Neither has a trailing newline. The %d is dnum, and
  seed = dnum is assigned a few lines above, so it is Params.Seed. The
  printf sits after parse_opts(env) (so a ROGUEOPTS name= is what the
  player is greeted by), and after the -s/-d handling (both exit())
  and after restore() (which never returns) — so only a new dungeon
  greets.

Mutation results

Each behaviour was removed in turn, make test run, and the mutation
reverted. In every case exactly the intended test failed and nothing else
did.

#	Mutation	Failed	Collateral
1	drop g.msg("what a bizarre schtick!") from the default arm	TestZapUnhandledWandSaysBizarreSchtick	none
2	drop the case obj.hasValidWhich() arm, so WandNothing speaks	TestZapWandOfNothingIsSilent	none
3	CTRL('R') back to g.repaint() → g.refresh()	TestRedrawCommandForcesFullRepaint	none
4	swap the wizard and normal wordings	TestGreeting (all 5 sub-cases)	none
5	greeting ignores ROGUEOPTS, always uses the account name	TestGreeting (the 2 sub-cases that set name=)	none
6	digsNewDungeon drops the restore test	TestDigsNewDungeon/restore_a_save	none

Mutation 2 is the one the previous session died on and never confirmed: it
is now proved that the WandNothing silence is pinned by a test of its own,
independently of the message's presence for unhandled kinds.

Separately, the nil-dereference in defect 1 above was proved by a failing
test before the fix, which is the same discipline in reverse.

Verification

• make check fully green: fmt-check clean (make fmt was run and folded
  in), golangci-lint 0 issues, tests pass with -timeout 30s -race -cover.
• Lint hygiene: every accepted run used a private GOLANGCI_LINT_CACHE in a
  fresh empty directory outside the worktree, and was retried until it
  reported neither the parallel-lint error nor any path outside this
  worktree. One run did hit parallel golangci-lint is running and was
  discarded and retried, so the isolation-plus-retry pair earned its keep.
  The gomodguard deprecation warning (#29) is present and untouched.
• TestSeedCompatItemTables passes against its untouched golden;
  git diff against main shows zero changes under game/testdata/. No
  RNG call is added on any path: Greeting runs before New, and
  ParseOpts never reaches rnd.
• .golangci.yml unmodified. make targets only; no raw go/linter
  invocations.
• TODO.md has a Completed Steps entry in this same commit and Next Step
  is not rotated (out-of-band issue work).
• ARCHITECTURE.md: 5.3 gains Repaint and the reason a blit cannot
  substitute for it. Section 9 is unchanged — item 2 was not split out,
  so nothing from this issue remains deliberately dropped.
• Single commit 1142f43; the WIP checkpoint is amended away, not preserved.

## Two defects in the inherited WIP checkpoint The `3f0a14c` checkpoint was treated as a starting point, not a foundation. Both of these are fixed here. **1. `Greeting` crashed the game at startup for any player with `inven=` in `ROGUEOPTS`.** It parsed the options into a bare `&RogueGame{Whoami: ...}`, but `ParseOpts` handles every option, not just the `name=` the greeting reads. The `inven=` arm matches the value against `inv_t_name[]` (`options.c parse_opts`), which lives on `g.data` — nil on a bare game. `ROGUEOPTS="inven=slow"` therefore nil-dereferenced before the player saw a single character. Reproduced first as a failing test case, which is now a regression test ("ROGUEOPTS inventory style parses without a fault"); the throwaway game is now built the way `New` builds the real one, tables and home directory included. **2. The checkpoint did not pass lint either, not only `fmt-check`.** Two issues: `forbidigo` on `fmt.Print` in `cmd/rogue/main.go` (the repo's convention for stdout is `_, _ = fmt.Fprint(os.Stdout, ...)`, as in `game/score.go` and `term/tcell.go`), and `goconst` on the repeated account name in `greeting_test.go`. Both fixed; the greeting keeps its lack of a trailing newline. C's `fflush(stdout)` has no counterpart because `os.Stdout` is unbuffered. Everything else in the checkpoint was read line by line and stands. ## C verification Sources read via `git show origin/c-master:...` (never checked out), with `rogue.h`'s `#define when break;case` / `#define otherwise break;default` in mind. **Message text, byte-for-byte.** Each Go literal was matched with a fixed-string grep against the exact C statement: | C source | C literal | Go site | | ----------------------- | ------------------------------------------------------ | --------------- | | `sticks.c` 237 | `msg("what a bizarre schtick!");` | `game/sticks.go` | | `main.c` 109 (MASTER) | `printf("Hello %s, welcome to dungeon #%d", whoami, dnum);` | `game/game.go` | | `main.c` 112 | `printf("Hello %s, just a moment while I dig the dungeon...", whoami);` | `game/game.go` | **Placement and gating.** - The schtick arm sits inside `#ifdef MASTER`, **not** inside a runtime `wizard` test, so this MASTER-compiled port prints it for every player. It is not gated on `g.Wizard` — that would be #11's trap in reverse. - `WS_NOP` is `when WS_NOP: break;`, a case of the switch in its own right, and all arms fall out into `obj->o_charges--`. C's switch covers all 14 `WS_` values (0..13, verified against `rogue.h` 294-307, which the Go `WandKind` enum matches one-for-one), so `otherwise` is reachable only for an `o_which` outside the table — exactly what `Object.hasValidWhich` screens for. `WandNothing` is the one kind with no handler and a valid `Which`, so the three-way switch needs no new state. - `command.c` 288-291 is `after = FALSE; clearok(curscr,TRUE); wrefresh(curscr);` — no `refresh()` of stdscr, and the command loop refreshes before the next key read anyway. - `main.c`: only the wizard arm is `#ifdef MASTER`; the normal wording is unconditional. Neither has a trailing newline. The `%d` is `dnum`, and `seed = dnum` is assigned a few lines above, so it is `Params.Seed`. The printf sits after `parse_opts(env)` (so a `ROGUEOPTS` `name=` is what the player is greeted by), and after the `-s`/`-d` handling (both `exit()`) and after `restore()` (which never returns) — so only a new dungeon greets. ## Mutation results Each behaviour was removed in turn, `make test` run, and the mutation reverted. In every case exactly the intended test failed and nothing else did. | # | Mutation | Failed | Collateral | | - | -------------------------------------------------------------- | ----------------------------------------------- | ---------- | | 1 | drop `g.msg("what a bizarre schtick!")` from the default arm | `TestZapUnhandledWandSaysBizarreSchtick` | none | | 2 | drop the `case obj.hasValidWhich()` arm, so `WandNothing` speaks | `TestZapWandOfNothingIsSilent` | none | | 3 | `CTRL('R')` back to `g.repaint()` → `g.refresh()` | `TestRedrawCommandForcesFullRepaint` | none | | 4 | swap the wizard and normal wordings | `TestGreeting` (all 5 sub-cases) | none | | 5 | greeting ignores `ROGUEOPTS`, always uses the account name | `TestGreeting` (the 2 sub-cases that set `name=`) | none | | 6 | `digsNewDungeon` drops the restore test | `TestDigsNewDungeon/restore_a_save` | none | Mutation 2 is the one the previous session died on and never confirmed: it is now proved that the `WandNothing` silence is pinned by a test of its own, independently of the message's presence for unhandled kinds. Separately, the nil-dereference in defect 1 above was proved by a failing test before the fix, which is the same discipline in reverse. ## Verification - `make check` fully green: `fmt-check` clean (`make fmt` was run and folded in), `golangci-lint` **0 issues**, tests pass with `-timeout 30s -race -cover`. - Lint hygiene: every accepted run used a private `GOLANGCI_LINT_CACHE` in a fresh empty directory outside the worktree, and was retried until it reported neither the parallel-lint error nor any path outside this worktree. One run did hit `parallel golangci-lint is running` and was discarded and retried, so the isolation-plus-retry pair earned its keep. The `gomodguard` deprecation warning (#29) is present and untouched. - `TestSeedCompatItemTables` passes against its **untouched** golden; `git diff` against `main` shows zero changes under `game/testdata/`. No RNG call is added on any path: `Greeting` runs before `New`, and `ParseOpts` never reaches `rnd`. - `.golangci.yml` unmodified. `make` targets only; no raw `go`/linter invocations. - `TODO.md` has a Completed Steps entry in this same commit and `Next Step` is **not** rotated (out-of-band issue work). - ARCHITECTURE.md: 5.3 gains `Repaint` and the reason a blit cannot substitute for it. Section 9 is unchanged — item 2 was **not** split out, so nothing from this issue remains deliberately dropped. - Single commit `1142f43`; the WIP checkpoint is amended away, not preserved.

clawbot commented 2026-08-09 12:13:06 +02:00

Author

Collaborator

Copy Link

Verdict: PASS

Independent adversarial review of 1142f43 against main @ 727dfb2. Reviewed in a throwaway worktree; nothing changed, nothing committed. Every claim in the PR body and the PR comment was re-derived from the C sources and from a local run, not taken on trust.

Definition of done (issue #13)

DoD item	Result
1. Unhandled kinds emit the message; WandNothing does not	met (game/sticks.go 48-58)
2. CTRL('R') forces a full repaint via a new Terminal method	met (game/screen.go, term/tcell.go, game/tables.go)
3. Greeting at the right point, both wordings, C's seed number	met (game/game.go Greeting, cmd/rogue/main.go 47-49)
4. Tests for all of the above, present and absent	met
5. make check green	verified locally, see below
6. TODO.md updated in the same commit	met
7. Title ends (closes #13)	met

Primary focus 1 — the inherited nil dereference

(a) The crash was real. Reproduced by reverting the fix to a bare &RogueGame{Whoami: params.Name}: TestGreeting/ROGUEOPTS_inventory_style_parses_without_a_fault fails with panic: runtime error: invalid memory address or nil pointer dereference. The path is ParseOpts → parseOptName → parseOptValue (op.kind == optInvT) → parseInvType, which ranges over g.data.invTName (game/options.go 434) on a nil g.data.

(b) The fix is complete. I enumerated everything ParseOpts can reach, via optList (game/options.go 30-45), and checked each against the throwaway game in game/game.go 185-189:

• &o.Terse, &o.FightFlush, &o.Jump, &o.SeeFloor, &o.PassGo, &o.Tombstone, &o.InvType — Options is an embedded value struct, so field pointers into a zero-valued RogueGame are valid; no nil possible.
• &g.Whoami, &g.Fruit, &g.FileName — plain string fields; assignment only.
• g.Home — read by parseOptValue's ~ expansion (game/options.go 402); a string, so at worst a wrong prefix, never a fault. Populated anyway.
• g.data.invTName — the only pointer dereference, populated by newGameData().

That is the complete reachable set. No other option can fault a hand-built game, so this is not a partial fix.

(c) The regression test genuinely covers it. The inven=slow,name=Rodney case exercises the exact arm that faulted, and it is the only sub-case that fails under the reverted fix — so it is pinned by that case specifically, not incidentally by another.

Primary focus 2 — the WandNothing distinction

• Enum matches C one-for-one. rogue.h 294-307 WS_LIGHT..WS_CANCEL (0..13) plus MAXSTICKS 14 against game/types.go 324-339 WandLight..WandCancellation plus NumWandTypes. Index-for-index identical, including WS_MISSILE = WandMagicMissile (6) and WS_NOP = WandNothing (10).
• WandNothing is genuinely the only valid Which with no handler. zapHandlers (game/tables.go 670-684) has an entry for every index except WandNothing; the array is [NumWandTypes], and zapHandler (game/tables.go 894-900) returns nil for anything failing hasValidWhich. whichLimit(KindWand) is NumWandTypes (game/object.go 188-189), so the three arms of the switch partition the space exactly.
• The switch is correct and ungated. game/sticks.go 50-58: handler / valid-Which / neither, all three falling out to obj.Charges-- on line 60, matching C's fall-out to obj->o_charges--. Not wrapped in if g.Wizard — correct, and see below.
• Charges decrement for WandNothing — asserted in TestZapWandOfNothingIsSilent, and structurally guaranteed by the default-less fallthrough to line 60.
• Mutation reproduced independently. Deleting case obj.hasValidWhich(): so WandNothing speaks: make test fails with exactly --- FAIL: TestZapWandOfNothingIsSilent and nothing else (2 FAIL lines in the log, both that same test across the run and the verbose rerun). Collateral: none. Mutation reverted; worktree clean.

Message strings byte-for-byte

Read via git show origin/c-master:...; c-master never checked out.

• sticks.c — the arm is at line 237 and is msg("what a bizarre schtick!");, wrapped in #ifdef MASTER / #endif, not in a runtime wizard test. Confirmed by reading the raw bytes: the preceding lines are when WS_NOP: / break; and the #ifdef MASTER opens immediately after, with #endif before the closing brace. The distinction is exactly as the finisher stated, and the Go code is correctly unconditional.
• main.c 107-113 — printf("Hello %s, welcome to dungeon #%d", whoami, dnum); and printf("Hello %s, just a moment while I dig the dungeon...", whoami);. Both match the Go literals character for character, including the three-dot ellipsis, the # before %d, the comma placement, and the absence of any \n. TestGreeting asserts the no-newline property explicitly.

Greeting placement and the %d

Every one of the four claims verified against main.c:

1. Only the wizard arm is inside #ifdef MASTER; the else and the normal printf sit outside it, so the normal wording is unconditional. Go mirrors this with a plain if params.Wizard.
2. %d is dnum, and seed = dnum; is assigned at main.c line 65, above the printf. Params.Seed is the right value; New also sets Dnum: int(params.Seed), so the greeting and the game agree.
3. The printf is after parse_opts(env) (line 55), so a ROGUEOPTS name= is what greets — matched by Greeting re-running ParseOpts, and pinned by two TestGreeting cases.
4. It is after the -s and -d handling (both exit(0)) and after restore() (never returns). digsNewDungeon reproduces this, and -s is additionally excluded by returning before the greeting in run. TestDigsNewDungeon covers all four combinations.

command.c 288-291 also confirmed verbatim: after = FALSE; clearok(curscr,TRUE); wrefresh(curscr);, with no refresh() of stdscr.

RNG order

git diff origin/main..HEAD --name-only lists nothing under game/testdata/. TestSeedCompatItemTables PASSes in the verbose log. Greeting runs before New, and the ParseOpts chain reaches no RNG call — verified by reading parseOptName, parseOptValue, parseInvType, strucpy.

Forced repaint

term.Tcell.Repaint is t.screen.Sync(), which discards tcell's model of the physical screen and redraws every cell — a genuine forced repaint, not Show()'s diff. All three Terminal implementations are updated: term/tcell.go 87, game/term_test.go 21, game/autosave_test.go 462. Grep for func (…) Render( returns exactly those three types, so no implementation was missed. Screen.Repaint nil-guards the device the same way Refresh/Fini do. TestRedrawCommandForcesFullRepaint watches the terminal, not the window, which is the only place the difference is observable, and also asserts After stays false.

Lint fixes

• forbidigo: the new stdout write is _, _ = fmt.Fprint(os.Stdout, …) // CLI output. The convention claim is true — game/score.go 232 is _, _ = fmt.Fprintln(os.Stdout, line) // CLI output and term/tcell.go 218 is the same shape. Not suppressed.
• goconst: fixed by the const account = "conan" in greeting_test.go, and const bizarreSchtick in effects_test.go shares the literal between the present/absent pair. Not suppressed.
• No new //nolint beyond the two //nolint:testpackage file headers on the new white-box test files, which is the approved 2026-07-07 convention.

Commit and branch hygiene

• Single commit; 3f0a14c is not an ancestor of 1142f43 — the WIP checkpoint is gone, not preserved.
• Title: Restore three lost C behaviors: schtick message, forced redraw, greeting (closes #13).
• Author and committer sneak <sneak@sneak.berlin>. No trailers.
• Full-tree and full-diff grep for Claude / Anthropic / Co-authored / "generated with": zero hits.
• merge-base(origin/main, HEAD) == origin/main, so fast-forwardable; Gitea reports mergeable.
• git diff --check clean. No scripted-edit artifacts.
• .golangci.yml sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, not in the diff.
• No Dockerfile/CI/script/ in this repo (Makefile records the exemption); no such files touched. The head commit carries no CI statuses because the repo has no workflow, so needs-checks does not apply.
• All new tests call t.Parallel(), including sub-tests.

Docs

ARCHITECTURE.md §5.3 gains Repaint in both the Terminal listing and the Screen listing, plus the paragraph on why a blit cannot substitute. §9 is untouched, which is right: item 2 was delivered rather than split out, so nothing new is deliberately dropped. TODO.md gains a Completed Steps entry and Next Step is not rotated.

Local gate

Run in a private worktree with GOLANGCI_LINT_CACHE pointed at a fresh empty directory outside the worktree.

• make fmt-check: clean (gofmt and prettier).
• make lint: 0 issues. No parallel golangci-lint is running, and no reported path outside the worktree, so the run is valid. The expected gomodguard deprecation warning (#29) is present and is not a finding.
• make test with GOFLAGS=-count=1, three separate runs: all green, no (cached) markers, race detector clean each time. cmd/rogue 29.7%, game 49.4%.

Non-blocking observations (not defects, no rework required)

1. game/effects_test.go TestZapWandOfNothingIsSilent asserts Msgs.Huh != bizarreSchtick rather than Msgs.Huh == "". It catches the mutation that matters and the one C actually forbids, but a hypothetical mutation emitting some other message would slip past. Tightening to an empty-string assertion would close that, since the test already clears Huh first.
2. Greeting inherits New's handling of a degenerate ROGUEOPTS="name=": C falls back to the account name when whoami[0] == '\0' after parse_opts (main.c 55-57), whereas the port pre-seeds Whoami and lets an explicit empty name= blank it. This is pre-existing in New and the greeting is consistent with the game it announces, so changing it here would be scope creep and would desynchronise the two. Worth an issue only if seed-faithfulness of whoami ever matters.

## Verdict: PASS Independent adversarial review of `1142f43` against `main` @ `727dfb2`. Reviewed in a throwaway worktree; nothing changed, nothing committed. Every claim in the PR body and the PR comment was re-derived from the C sources and from a local run, not taken on trust. ### Definition of done (issue #13) | DoD item | Result | | --- | --- | | 1. Unhandled kinds emit the message; `WandNothing` does not | met (`game/sticks.go` 48-58) | | 2. `CTRL('R')` forces a full repaint via a new `Terminal` method | met (`game/screen.go`, `term/tcell.go`, `game/tables.go`) | | 3. Greeting at the right point, both wordings, C's seed number | met (`game/game.go` `Greeting`, `cmd/rogue/main.go` 47-49) | | 4. Tests for all of the above, present and absent | met | | 5. `make check` green | verified locally, see below | | 6. `TODO.md` updated in the same commit | met | | 7. Title ends ` (closes #13)` | met | ### Primary focus 1 — the inherited nil dereference **(a) The crash was real.** Reproduced by reverting the fix to a bare `&RogueGame{Whoami: params.Name}`: `TestGreeting/ROGUEOPTS_inventory_style_parses_without_a_fault` fails with `panic: runtime error: invalid memory address or nil pointer dereference`. The path is `ParseOpts` → `parseOptName` → `parseOptValue` (`op.kind == optInvT`) → `parseInvType`, which ranges over `g.data.invTName` (`game/options.go` 434) on a nil `g.data`. **(b) The fix is complete.** I enumerated everything `ParseOpts` can reach, via `optList` (`game/options.go` 30-45), and checked each against the throwaway game in `game/game.go` 185-189: - `&o.Terse`, `&o.FightFlush`, `&o.Jump`, `&o.SeeFloor`, `&o.PassGo`, `&o.Tombstone`, `&o.InvType` — `Options` is an embedded value struct, so field pointers into a zero-valued `RogueGame` are valid; no nil possible. - `&g.Whoami`, `&g.Fruit`, `&g.FileName` — plain string fields; assignment only. - `g.Home` — read by `parseOptValue`'s `~` expansion (`game/options.go` 402); a string, so at worst a wrong prefix, never a fault. Populated anyway. - `g.data.invTName` — the only pointer dereference, populated by `newGameData()`. That is the complete reachable set. No other option can fault a hand-built game, so this is not a partial fix. **(c) The regression test genuinely covers it.** The `inven=slow,name=Rodney` case exercises the exact arm that faulted, and it is the only sub-case that fails under the reverted fix — so it is pinned by that case specifically, not incidentally by another. ### Primary focus 2 — the `WandNothing` distinction - **Enum matches C one-for-one.** `rogue.h` 294-307 `WS_LIGHT`..`WS_CANCEL` (0..13) plus `MAXSTICKS 14` against `game/types.go` 324-339 `WandLight`..`WandCancellation` plus `NumWandTypes`. Index-for-index identical, including `WS_MISSILE` = `WandMagicMissile` (6) and `WS_NOP` = `WandNothing` (10). - **`WandNothing` is genuinely the only valid `Which` with no handler.** `zapHandlers` (`game/tables.go` 670-684) has an entry for every index except `WandNothing`; the array is `[NumWandTypes]`, and `zapHandler` (`game/tables.go` 894-900) returns nil for anything failing `hasValidWhich`. `whichLimit(KindWand)` is `NumWandTypes` (`game/object.go` 188-189), so the three arms of the switch partition the space exactly. - **The switch is correct and ungated.** `game/sticks.go` 50-58: handler / valid-`Which` / neither, all three falling out to `obj.Charges--` on line 60, matching C's fall-out to `obj->o_charges--`. Not wrapped in `if g.Wizard` — correct, and see below. - **Charges decrement for `WandNothing`** — asserted in `TestZapWandOfNothingIsSilent`, and structurally guaranteed by the `default`-less fallthrough to line 60. - **Mutation reproduced independently.** Deleting `case obj.hasValidWhich():` so `WandNothing` speaks: `make test` fails with exactly `--- FAIL: TestZapWandOfNothingIsSilent` and nothing else (2 FAIL lines in the log, both that same test across the run and the verbose rerun). Collateral: none. Mutation reverted; worktree clean. ### Message strings byte-for-byte Read via `git show origin/c-master:...`; `c-master` never checked out. - `sticks.c` — the arm is at line 237 and is `msg("what a bizarre schtick!");`, wrapped in `#ifdef MASTER` / `#endif`, **not** in a runtime `wizard` test. Confirmed by reading the raw bytes: the preceding lines are `when WS_NOP:` / `break;` and the `#ifdef MASTER` opens immediately after, with `#endif` before the closing brace. The distinction is exactly as the finisher stated, and the Go code is correctly unconditional. - `main.c` 107-113 — `printf("Hello %s, welcome to dungeon #%d", whoami, dnum);` and `printf("Hello %s, just a moment while I dig the dungeon...", whoami);`. Both match the Go literals character for character, including the three-dot ellipsis, the `#` before `%d`, the comma placement, and the absence of any `\n`. `TestGreeting` asserts the no-newline property explicitly. ### Greeting placement and the `%d` Every one of the four claims verified against `main.c`: 1. Only the wizard arm is inside `#ifdef MASTER`; the `else` and the normal `printf` sit outside it, so the normal wording is unconditional. Go mirrors this with a plain `if params.Wizard`. 2. `%d` is `dnum`, and `seed = dnum;` is assigned at `main.c` line 65, above the printf. `Params.Seed` is the right value; `New` also sets `Dnum: int(params.Seed)`, so the greeting and the game agree. 3. The printf is after `parse_opts(env)` (line 55), so a ROGUEOPTS `name=` is what greets — matched by `Greeting` re-running `ParseOpts`, and pinned by two `TestGreeting` cases. 4. It is after the `-s` and `-d` handling (both `exit(0)`) and after `restore()` (never returns). `digsNewDungeon` reproduces this, and `-s` is additionally excluded by returning before the greeting in `run`. `TestDigsNewDungeon` covers all four combinations. `command.c` 288-291 also confirmed verbatim: `after = FALSE; clearok(curscr,TRUE); wrefresh(curscr);`, with no `refresh()` of stdscr. ### RNG order `git diff origin/main..HEAD --name-only` lists nothing under `game/testdata/`. `TestSeedCompatItemTables` PASSes in the verbose log. `Greeting` runs before `New`, and the `ParseOpts` chain reaches no RNG call — verified by reading `parseOptName`, `parseOptValue`, `parseInvType`, `strucpy`. ### Forced repaint `term.Tcell.Repaint` is `t.screen.Sync()`, which discards tcell's model of the physical screen and redraws every cell — a genuine forced repaint, not `Show()`'s diff. All three `Terminal` implementations are updated: `term/tcell.go` 87, `game/term_test.go` 21, `game/autosave_test.go` 462. Grep for `func (…) Render(` returns exactly those three types, so no implementation was missed. `Screen.Repaint` nil-guards the device the same way `Refresh`/`Fini` do. `TestRedrawCommandForcesFullRepaint` watches the terminal, not the window, which is the only place the difference is observable, and also asserts `After` stays false. ### Lint fixes - `forbidigo`: the new stdout write is `_, _ = fmt.Fprint(os.Stdout, …) // CLI output`. The convention claim is true — `game/score.go` 232 is `_, _ = fmt.Fprintln(os.Stdout, line) // CLI output` and `term/tcell.go` 218 is the same shape. Not suppressed. - `goconst`: fixed by the `const account = "conan"` in `greeting_test.go`, and `const bizarreSchtick` in `effects_test.go` shares the literal between the present/absent pair. Not suppressed. - No new `//nolint` beyond the two `//nolint:testpackage` file headers on the new white-box test files, which is the approved 2026-07-07 convention. ### Commit and branch hygiene - Single commit; `3f0a14c` is not an ancestor of `1142f43` — the WIP checkpoint is gone, not preserved. - Title: `Restore three lost C behaviors: schtick message, forced redraw, greeting (closes #13)`. - Author and committer `sneak <sneak@sneak.berlin>`. No trailers. - Full-tree and full-diff grep for Claude / Anthropic / Co-authored / "generated with": zero hits. - `merge-base(origin/main, HEAD) == origin/main`, so fast-forwardable; Gitea reports mergeable. - `git diff --check` clean. No scripted-edit artifacts. - `.golangci.yml` sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, not in the diff. - No Dockerfile/CI/`script/` in this repo (Makefile records the exemption); no such files touched. The head commit carries no CI statuses because the repo has no workflow, so `needs-checks` does not apply. - All new tests call `t.Parallel()`, including sub-tests. ### Docs ARCHITECTURE.md §5.3 gains `Repaint` in both the `Terminal` listing and the `Screen` listing, plus the paragraph on why a blit cannot substitute. §9 is untouched, which is right: item 2 was delivered rather than split out, so nothing new is deliberately dropped. TODO.md gains a Completed Steps entry and `Next Step` is not rotated. ### Local gate Run in a private worktree with `GOLANGCI_LINT_CACHE` pointed at a fresh empty directory outside the worktree. - `make fmt-check`: clean (gofmt and prettier). - `make lint`: `0 issues`. No `parallel golangci-lint is running`, and no reported path outside the worktree, so the run is valid. The expected `gomodguard` deprecation warning (#29) is present and is not a finding. - `make test` with `GOFLAGS=-count=1`, three separate runs: all green, no `(cached)` markers, race detector clean each time. `cmd/rogue` 29.7%, `game` 49.4%. ### Non-blocking observations (not defects, no rework required) 1. `game/effects_test.go` `TestZapWandOfNothingIsSilent` asserts `Msgs.Huh != bizarreSchtick` rather than `Msgs.Huh == ""`. It catches the mutation that matters and the one C actually forbids, but a hypothetical mutation emitting some *other* message would slip past. Tightening to an empty-string assertion would close that, since the test already clears `Huh` first. 2. `Greeting` inherits `New`'s handling of a degenerate `ROGUEOPTS="name="`: C falls back to the account name when `whoami[0] == '\0'` after `parse_opts` (`main.c` 55-57), whereas the port pre-seeds `Whoami` and lets an explicit empty `name=` blank it. This is pre-existing in `New` and the greeting is consistent with the game it announces, so changing it here would be scope creep and would desynchronise the two. Worth an issue only if seed-faithfulness of `whoami` ever matters.

clawbot added merge-ready and removed needs-review labels 2026-08-09 12:13:39 +02:00

clawbot commented 2026-08-09 12:13:54 +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 despite the
reviewer's suggestion.

This PR nearly shipped a startup crash, and the recovery process is what
caught it. The original session died mid-verification, leaving ~300
uncommitted lines in a temp worktree. Had that been treated as "almost done"
and finished mechanically, Greeting would have gone in building a bare
&RogueGame{} and handing it to ParseOpts — which handles every option,
not just the name= the greeting reads. Any player with inven= in
ROGUEOPTS would have hit a nil-pointer panic before the game started.

The finisher reproduced it as a failing test before fixing it, which is
the right order and is what MEMORY.md asks for.

The check I most wanted was completeness, not correctness, and it was done
properly. A fix that stops inven= crashing while leaving some other option
crashing is the same bug wearing a different hat. The reviewer enumerated the
entire reachable set through optList — seven &o.* Options pointers (a
value struct, valid on a zero RogueGame), &g.Whoami, &g.Fruit,
&g.FileName, g.Home, and g.data.invTName as the only pointer deref — and
confirmed every one is now populated. That is the difference between "the
reported crash is fixed" and "this class of crash is closed".

The WandNothing case is finally proven. It was the exact mutation the
dead session never reached, and it is the whole point of item 1 — the message
must fire for an unhandled kind but not for one that deliberately does
nothing. The reviewer reproduced the mutation independently (deleting the
hasValidWhich arm fails only TestZapWandOfNothingIsSilent) and verified
the partition is exact: the WandKind enum matches rogue.h 294-307
index-for-index, zapHandlers omits only WandNothing, and
whichLimit(KindWand) == NumWandTypes. Charges decrement on all three arms,
matching C.

One C detail worth recording because it changes when the message fires:
sticks.c:237 is inside #ifdef MASTER, not gated on a runtime wizard
test. So it is unconditional in this port and correctly not gated on
g.Wizard. Getting that wrong would have hidden the message from ordinary
play.

A correction to my own status comment on #13: I wrote that the WIP
checkpoint failed only fmt-check. It also failed lint — forbidigo on a
bare fmt.Print. I had run make check, seen it stop at the first failing
target, and reported that as the whole story. make check short-circuits;
"it fails fmt-check" is not the same as "it fails only fmt-check".

The two non-blocking observations (a slightly loose assertion in
TestZapWandOfNothingIsSilent, and a pre-existing ROGUEOPTS="name="
fallback divergence inherited from New) are noted and not folded in. The
second is pre-existing and belongs to the options handling, not this change.

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` despite the reviewer's suggestion. **This PR nearly shipped a startup crash, and the recovery process is what caught it.** The original session died mid-verification, leaving ~300 uncommitted lines in a temp worktree. Had that been treated as "almost done" and finished mechanically, `Greeting` would have gone in building a bare `&RogueGame{}` and handing it to `ParseOpts` — which handles *every* option, not just the `name=` the greeting reads. Any player with `inven=` in `ROGUEOPTS` would have hit a nil-pointer panic before the game started. The finisher reproduced it as a failing test **before** fixing it, which is the right order and is what `MEMORY.md` asks for. **The check I most wanted was completeness, not correctness, and it was done properly.** A fix that stops `inven=` crashing while leaving some other option crashing is the same bug wearing a different hat. The reviewer enumerated the entire reachable set through `optList` — seven `&o.*` Options pointers (a value struct, valid on a zero `RogueGame`), `&g.Whoami`, `&g.Fruit`, `&g.FileName`, `g.Home`, and `g.data.invTName` as the only pointer deref — and confirmed every one is now populated. That is the difference between "the reported crash is fixed" and "this class of crash is closed". **The `WandNothing` case is finally proven.** It was the exact mutation the dead session never reached, and it is the whole point of item 1 — the message must fire for an *unhandled* kind but not for one that deliberately does nothing. The reviewer reproduced the mutation independently (deleting the `hasValidWhich` arm fails only `TestZapWandOfNothingIsSilent`) and verified the partition is exact: the `WandKind` enum matches `rogue.h` 294-307 index-for-index, `zapHandlers` omits only `WandNothing`, and `whichLimit(KindWand) == NumWandTypes`. Charges decrement on all three arms, matching C. One C detail worth recording because it changes *when* the message fires: `sticks.c:237` is inside `#ifdef MASTER`, **not** gated on a runtime `wizard` test. So it is unconditional in this port and correctly not gated on `g.Wizard`. Getting that wrong would have hidden the message from ordinary play. **A correction to my own status comment on #13:** I wrote that the WIP checkpoint failed only `fmt-check`. It also failed `lint` — `forbidigo` on a bare `fmt.Print`. I had run `make check`, seen it stop at the first failing target, and reported that as the whole story. `make check` short-circuits; "it fails fmt-check" is not the same as "it fails only fmt-check". The two non-blocking observations (a slightly loose assertion in `TestZapWandOfNothingIsSilent`, and a pre-existing `ROGUEOPTS="name="` fallback divergence inherited from `New`) are noted and not folded in. The second is pre-existing and belongs to the options handling, not this change.

clawbot merged commit a653cc76f2 into main 2026-08-09 12:14:00 +02:00

clawbot deleted branch fix/lost-c-behaviors 2026-08-09 12:14:00 +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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/rgoue#32