C main.c:332 installs leave(int sig) — "leave quickly but courteously" —
on SIGINT and SIGQUIT: it restores the terminal before exiting.
Go installs handlers for SIGHUP and SIGTERM only
(cmd/rogue/main.go:106-119), both routed to AutoSave. There is no SIGINT
or SIGQUIT handling.
So pressing Ctrl-C kills the process with tcell still holding the terminal
in raw mode. The user is dropped to a shell with no echo, no line
discipline, and a scrambled screen, needing a blind reset.
Ctrl-C is the single most likely way a player exits a terminal game they want
out of, so this is the most reachable robustness gap in the port. It is also
not listed in ARCHITECTURE.md §9's dropped-functionality table — it looks like
an oversight rather than a decision.
Related: C main.c:211 tstp() (SIGTSTP suspend/resume) is also unported and
unlisted. term/tcell.go:176-203 ShellEscape covers only the ! shell-escape
case, not a real Ctrl-Z suspend.
Definition of done
SIGINT (and SIGQUIT) restore the terminal before the process exits — at
minimum Terminal.Fini, matching what myExit already does.
The choice between C's exact leave() semantics and this port's
autosave-on-signal model is made deliberately and documented in a code
comment — C's leave() does not save, while this port's existing signal
handlers do. Do not silently pick one; state the reasoning.
SIGTSTP is either handled or explicitly recorded in ARCHITECTURE.md §9 as
deliberately dropped, with the reason.
ARCHITECTURE.md §9's dropped table gains rows for whatever remains dropped.
A test covers the terminal-restore path to whatever depth is practical
headlessly; if it genuinely cannot be tested, say so in the PR with the
reason rather than skipping silently.
make check fully green.
TODO.md updated in the same commit.
Commit title ends with (closes #N).
Implementation requirements
Read git show origin/c-master:main.c (leave, tstp, and the signal() wiring in main) first. Do NOT check out or modify origin/c-master.
Be careful not to break the existing SIGHUP/SIGTERM autosave behavior — it
is deliberate and tested by the save/restore suite.
Do not introduce a signal handler that can race with os.Exit in a way that
corrupts the save file. If autosave-on-SIGINT is chosen, the ordering must
be correct and stated.
make targets only. Do NOT modify .golangci.yml.
Never mention Claude or Anthropic anywhere.
Note
If you would rather Ctrl-C simply not save (C's actual behavior) than adopt
this port's autosave convention, say so on this issue and I'll pin the scope
before dispatching an implementer.
## Problem
C `main.c:332` installs `leave(int sig)` — "leave quickly but courteously" —
on SIGINT and SIGQUIT: it restores the terminal before exiting.
Go installs handlers for **SIGHUP and SIGTERM only**
(`cmd/rogue/main.go:106-119`), both routed to `AutoSave`. There is no SIGINT
or SIGQUIT handling.
So pressing **Ctrl-C** kills the process with tcell still holding the terminal
in raw mode. The user is dropped to a shell with no echo, no line
discipline, and a scrambled screen, needing a blind `reset`.
Ctrl-C is the single most likely way a player exits a terminal game they want
out of, so this is the most reachable robustness gap in the port. It is also
not listed in ARCHITECTURE.md §9's dropped-functionality table — it looks like
an oversight rather than a decision.
Related: C `main.c:211 tstp()` (SIGTSTP suspend/resume) is also unported and
unlisted. `term/tcell.go:176-203 ShellEscape` covers only the `!` shell-escape
case, not a real Ctrl-Z suspend.
## Definition of done
1. SIGINT (and SIGQUIT) restore the terminal before the process exits — at
minimum `Terminal.Fini`, matching what `myExit` already does.
2. The choice between C's exact `leave()` semantics and this port's
autosave-on-signal model is made **deliberately and documented in a code
comment** — C's `leave()` does not save, while this port's existing signal
handlers do. Do not silently pick one; state the reasoning.
3. SIGTSTP is either handled or explicitly recorded in ARCHITECTURE.md §9 as
deliberately dropped, with the reason.
4. ARCHITECTURE.md §9's dropped table gains rows for whatever remains dropped.
5. A test covers the terminal-restore path to whatever depth is practical
headlessly; if it genuinely cannot be tested, say so in the PR with the
reason rather than skipping silently.
6. `make check` fully green.
7. `TODO.md` updated in the same commit.
8. Commit title ends with ` (closes #N)`.
## Implementation requirements
- Read `git show origin/c-master:main.c` (`leave`, `tstp`, and the
`signal()` wiring in `main`) first. Do NOT check out or modify
`origin/c-master`.
- Be careful not to break the existing SIGHUP/SIGTERM autosave behavior — it
is deliberate and tested by the save/restore suite.
- Do not introduce a signal handler that can race with `os.Exit` in a way that
corrupts the save file. If autosave-on-SIGINT is chosen, the ordering must
be correct and stated.
- `make` targets only. Do NOT modify `.golangci.yml`.
- Never mention Claude or Anthropic anywhere.
## Note
If you would rather Ctrl-C simply not save (C's actual behavior) than adopt
this port's autosave convention, say so on this issue and I'll pin the scope
before dispatching an implementer.
Read the C reference first (git show origin/c-master: on main.c, mdport.c, mach_dep.c, save.c, rip.c). Two corrections to the issue's premise, both of
which change the shape of the fix but not the conclusion:
1. leave() is not installed on SIGINT/SIGQUIT during play. The signal
wiring lives in mdport.c, not main.c:
md_init() calls md_onsignal_exit() — SIGQUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to the raw exit function pointer, SIGHUP to SIG_DFL, SIGINT untouched.
setup() (mach_dep.c:136) then calls md_onsignal_default() in the shipped build — everything back to SIG_DFL. md_onsignal_autosave(), the variant that wires SIGHUP/SIGTERM to auto_save and SIGINT to quit, is compiled in only under #ifdef DUMP.
leave() is installed on SIGINT in exactly two places, both endgame: rip.c:237 (death()) and main.c:305 (inside quit() once the player has confirmed y). Its job is to be a second-Ctrl-C escape hatch while the scoreboard is being printed.
So C's shipped build has no handler at all during play, and the port's existing
SIGHUP/SIGTERM to AutoSave already follows the md_onsignal_autosave intent
rather than the shipped default. The port has to make a principled choice here;
"match C exactly" does not resolve it.
2. Ctrl-C cannot currently reach the process as SIGINT anyway. tcell's tty Start() calls golang.org/x/term.MakeRaw, which clears ISIG — so Ctrl-C
arrives as key event KeyCtrlC, which term/tcell.go translateKey already turns
into byte 0x03 for the command loop. C did the same thing (setup() calls
curses raw(), which also clears ISIG). The real exposure is therefore kill -INT / kill -QUIT from another terminal, plus the window before term.New() — still worth fixing, and still worth the terminal restore, but the
"press Ctrl-C and land in a scrambled shell" reproducer as written does not fire.
That correction goes in the PR body; the fix is unchanged.
The save-vs-no-save decision: SIGINT/SIGQUIT will NOT save
Reasoning, to be written into the code comment:
C never saves on INT or QUIT anywhere in the tree. The only INT handler in the shipped build is leave() (endwin, discard pending output, exit); the only build that wires INT during play wires it to quit(), which confirms, scores, and exits without saving; SIGQUIT under md_onsignal_autosave goes to endit() to fatal() to endwin+exit, again without saving. auto_save is reserved for HUP/TERM. Keeping HUP/TERM saving and INT/QUIT not saving reproduces C's split exactly.
Semantically HUP/TERM mean involuntary teardown (line dropped, machine going down) so preserving the game is right; INT/QUIT are a deliberate "stop now" from the player. Rogue scores a deliberate quit and its save discipline is explicitly anti-save-scum, so turning Ctrl-C into a free checkpoint would be a one-keystroke undo for a bad turn: a gameplay change, not a robustness fix, and outside this issue.
Safety: the handler goroutine runs while the main goroutine is mutating game state mid-turn. AutoSave removes the save file and then gob-encodes that live state, so doing it concurrently is a genuine race that can leave a torn file where a good save used to be. The HUP/TERM path accepts that because the process is about to die anyway and a best-effort save beats none; on INT there is nothing to rescue, so the correct move is the one with no corruption window at all.
Race/ordering design
Rather than adding a second handler goroutine next to installAutosave, all four
signals go to one buffered channel read by one goroutine that reads
exactly one signal. That removes the corruption window the issue warns about: a
SIGINT arriving while a SIGHUP-triggered AutoSave is mid-write can no longer
call os.Exit underneath it — the second signal simply sits in the buffer and is
never read. Order within the handler is AutoSave (HUP/TERM only), then Terminal.Fini, then os.Exit(0), matching myExit in game/rip.go.
Work items
cmd/rogue/main.go: replace installAutosave with a single installSignalHandlers covering SIGHUP/SIGTERM (autosave, behavior
unchanged) and SIGINT/SIGQUIT (restore only), carrying the decision comment
above. The handler body is split into small injectable pieces (saver / finisher interfaces, exit func(int)) so it is testable headlessly.
SIGTSTP: recorded in ARCHITECTURE.md section 9 as deliberately dropped, not
handled. Reason: with tcell in raw mode VSUSP/ISIG are off so Ctrl-Z never
reaches the process as a signal, only kill -TSTP can; and a correct
suspend/resume would have to call screen.Suspend/Resume from the signal
goroutine while the game goroutine may be inside Render/PollEvent, which
is exactly the kind of race this issue forbids introducing. Doing it properly
means plumbing the signal into the input loop, which is a design change beyond
this issue. C's own wiring is vestigial here too: tstp is armed only by md_tstpresume(), which runs after a successful restore(), so a fresh C
game never had it either. The ! shell escape already covers "get me to a
shell".
ARCHITECTURE.md section 9 gains rows for SIGTSTP/tstp(), for SIGINT not
routing to the interactive quit() prompt, and for auto_save on the
fault signals (SIGILL/SIGFPE/SIGSEGV/SIGBUS/SIGSYS/SIGTRAP), which in Go are
runtime panics and where encoding the state that just crashed would risk
writing a corrupt save. Also fixes the stale claim at section 5.3 that
"SIGTSTP/resume and resize are handled by tcell" — tcell registers only
SIGWINCH.
New cmd/rogue/main_test.go (white-box, //nolint:testpackage): ordering
tests over a fake terminal and a fake saver proving save-then-restore-then-exit
for HUP/TERM and restore-then-exit with no save for INT/QUIT, plus an
end-to-end test that delivers a real SIGINT and a real SIGQUIT to the test
process and asserts the terminal was restored before exit. What cannot be
tested headlessly is that the actual tty comes back out of raw mode, since
that needs a controlling terminal; that limitation will be stated in the PR
rather than skipped silently.
TODO.md Completed Steps entry in the same commit; Next Step not rotated
(out-of-band issue work).
make fmt, then make check green (.golangci.yml untouched).
## Implementation plan
Read the C reference first (`git show origin/c-master:` on `main.c`, `mdport.c`,
`mach_dep.c`, `save.c`, `rip.c`). Two corrections to the issue's premise, both of
which change the shape of the fix but not the conclusion:
**1. `leave()` is not installed on SIGINT/SIGQUIT during play.** The signal
wiring lives in `mdport.c`, not `main.c`:
- `md_init()` calls `md_onsignal_exit()` — SIGQUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to the raw `exit` function pointer, SIGHUP to `SIG_DFL`, SIGINT untouched.
- `setup()` (`mach_dep.c:136`) then calls `md_onsignal_default()` in the shipped build — everything back to `SIG_DFL`. `md_onsignal_autosave()`, the variant that wires SIGHUP/SIGTERM to `auto_save` and SIGINT to `quit`, is compiled in only under `#ifdef DUMP`.
- `leave()` is installed on SIGINT in exactly two places, both endgame: `rip.c:237` (`death()`) and `main.c:305` (inside `quit()` once the player has confirmed `y`). Its job is to be a second-Ctrl-C escape hatch while the scoreboard is being printed.
So C's shipped build has no handler at all during play, and the port's existing
SIGHUP/SIGTERM to `AutoSave` already follows the `md_onsignal_autosave` intent
rather than the shipped default. The port has to make a principled choice here;
"match C exactly" does not resolve it.
**2. Ctrl-C cannot currently reach the process as SIGINT anyway.** tcell's tty
`Start()` calls `golang.org/x/term.MakeRaw`, which clears `ISIG` — so Ctrl-C
arrives as key event `KeyCtrlC`, which `term/tcell.go translateKey` already turns
into byte `0x03` for the command loop. C did the same thing (`setup()` calls
curses `raw()`, which also clears `ISIG`). The real exposure is therefore
`kill -INT` / `kill -QUIT` from another terminal, plus the window before
`term.New()` — still worth fixing, and still worth the terminal restore, but the
"press Ctrl-C and land in a scrambled shell" reproducer as written does not fire.
That correction goes in the PR body; the fix is unchanged.
### The save-vs-no-save decision: SIGINT/SIGQUIT will NOT save
Reasoning, to be written into the code comment:
- C never saves on INT or QUIT anywhere in the tree. The only INT handler in the shipped build is `leave()` (endwin, discard pending output, exit); the only build that wires INT during play wires it to `quit()`, which confirms, scores, and exits without saving; SIGQUIT under `md_onsignal_autosave` goes to `endit()` to `fatal()` to endwin+exit, again without saving. `auto_save` is reserved for HUP/TERM. Keeping HUP/TERM saving and INT/QUIT not saving reproduces C's split exactly.
- Semantically HUP/TERM mean involuntary teardown (line dropped, machine going down) so preserving the game is right; INT/QUIT are a deliberate "stop now" from the player. Rogue scores a deliberate quit and its save discipline is explicitly anti-save-scum, so turning Ctrl-C into a free checkpoint would be a one-keystroke undo for a bad turn: a gameplay change, not a robustness fix, and outside this issue.
- Safety: the handler goroutine runs while the main goroutine is mutating game state mid-turn. `AutoSave` removes the save file and then gob-encodes that live state, so doing it concurrently is a genuine race that can leave a torn file where a good save used to be. The HUP/TERM path accepts that because the process is about to die anyway and a best-effort save beats none; on INT there is nothing to rescue, so the correct move is the one with no corruption window at all.
### Race/ordering design
Rather than adding a second handler goroutine next to `installAutosave`, all four
signals go to **one** buffered channel read by **one** goroutine that reads
exactly one signal. That removes the corruption window the issue warns about: a
SIGINT arriving while a SIGHUP-triggered `AutoSave` is mid-write can no longer
call `os.Exit` underneath it — the second signal simply sits in the buffer and is
never read. Order within the handler is `AutoSave` (HUP/TERM only), then
`Terminal.Fini`, then `os.Exit(0)`, matching `myExit` in `game/rip.go`.
### Work items
1. `cmd/rogue/main.go`: replace `installAutosave` with a single
`installSignalHandlers` covering SIGHUP/SIGTERM (autosave, behavior
unchanged) and SIGINT/SIGQUIT (restore only), carrying the decision comment
above. The handler body is split into small injectable pieces (`saver` /
`finisher` interfaces, `exit func(int)`) so it is testable headlessly.
2. SIGTSTP: recorded in ARCHITECTURE.md section 9 as deliberately dropped, not
handled. Reason: with tcell in raw mode `VSUSP`/`ISIG` are off so Ctrl-Z never
reaches the process as a signal, only `kill -TSTP` can; and a correct
suspend/resume would have to call `screen.Suspend`/`Resume` from the signal
goroutine while the game goroutine may be inside `Render`/`PollEvent`, which
is exactly the kind of race this issue forbids introducing. Doing it properly
means plumbing the signal into the input loop, which is a design change beyond
this issue. C's own wiring is vestigial here too: `tstp` is armed only by
`md_tstpresume()`, which runs after a successful `restore()`, so a fresh C
game never had it either. The `!` shell escape already covers "get me to a
shell".
3. ARCHITECTURE.md section 9 gains rows for SIGTSTP/`tstp()`, for SIGINT not
routing to the interactive `quit()` prompt, and for `auto_save` on the
fault signals (SIGILL/SIGFPE/SIGSEGV/SIGBUS/SIGSYS/SIGTRAP), which in Go are
runtime panics and where encoding the state that just crashed would risk
writing a corrupt save. Also fixes the stale claim at section 5.3 that
"SIGTSTP/resume and resize are handled by tcell" — tcell registers only
SIGWINCH.
4. New `cmd/rogue/main_test.go` (white-box, `//nolint:testpackage`): ordering
tests over a fake terminal and a fake saver proving save-then-restore-then-exit
for HUP/TERM and restore-then-exit with no save for INT/QUIT, plus an
end-to-end test that delivers a real SIGINT and a real SIGQUIT to the test
process and asserts the terminal was restored before exit. What cannot be
tested headlessly is that the actual tty comes back out of raw mode, since
that needs a controlling terminal; that limitation will be stated in the PR
rather than skipped silently.
5. `TODO.md` Completed Steps entry in the same commit; `Next Step` not rotated
(out-of-band issue work).
6. `make fmt`, then `make check` green (`.golangci.yml` untouched).
Correction pending on two premises in this issue — both mine, both possibly
false. Flagging now so nobody reads the body above as settled fact while
PR #23 is under review.
The implementer of #23 challenged them after reading the actual C wiring:
"C installs leave() on SIGINT/SIGQUIT (main.c:332)." Claimed wrong.
The signal wiring is in mdport.c, not main.c, and the shipped build's setup() calls md_onsignal_default() — everything back to SIG_DFL, no handler at all. The variant that wires HUP/TERM to auto_save and INT
to quit is #ifdef DUMP only. leave() reportedly appears on SIGINT in
just two endgame spots, as a second-Ctrl-C escape during scoring.
"Pressing Ctrl-C leaves the terminal raw." Claimed wrong, and this is
the one that matters. tcell's Start() calls x/term.MakeRaw, which
clears ISIG — so Ctrl-C never becomes SIGINT in the first place. It
arrives as KeyCtrlC and translateKey already returns 0x03. C does the
same through curses raw().
If (2) holds, the severity I asserted in this issue was overstated. I
wrote that Ctrl-C is "the single most likely way a player exits a terminal
game" and leaves them needing a blind reset. That reproducer does not fire.
The real exposure is narrower: kill -INT / kill -QUIT, and the window
before term.New() runs.
The fix is still worth having — an unhandled kill -INT mid-game does leave
the terminal raw, and the pre-init window is real — but it is a robustness
hardening, not the everyday footgun I described.
I have not accepted either correction. A fresh reviewer is verifying both
against the C sources and tcell's actual behavior, precisely because these
claims are now asserted as fact in code comments and ARCHITECTURE.md. If
either turns out to be wrong, that is a blocking finding against #23.
I will update this issue body once the verification lands, whichever way it
goes. Recording it here rather than quietly editing, so the reasoning stays
auditable — this is the second time in this backlog that a confident-sounding
premise of mine has needed checking (see PR #20, where my "negative Which"
requirement rested on arithmetic that does not happen).
Separately, the implementer found a pre-existing data race in the
SIGHUP/SIGTERM autosave path and correctly left it alone rather than fixing it
drive-by. Now tracked as #24.
**Correction pending on two premises in this issue — both mine, both possibly
false.** Flagging now so nobody reads the body above as settled fact while
PR #23 is under review.
The implementer of #23 challenged them after reading the actual C wiring:
1. **"C installs `leave()` on SIGINT/SIGQUIT (`main.c:332`)."** Claimed wrong.
The signal wiring is in `mdport.c`, not `main.c`, and the shipped build's
`setup()` calls `md_onsignal_default()` — everything back to `SIG_DFL`,
*no handler at all*. The variant that wires HUP/TERM to `auto_save` and INT
to `quit` is `#ifdef DUMP` only. `leave()` reportedly appears on SIGINT in
just two endgame spots, as a second-Ctrl-C escape during scoring.
2. **"Pressing Ctrl-C leaves the terminal raw."** Claimed wrong, and this is
the one that matters. tcell's `Start()` calls `x/term.MakeRaw`, which
clears `ISIG` — so **Ctrl-C never becomes SIGINT in the first place**. It
arrives as `KeyCtrlC` and `translateKey` already returns `0x03`. C does the
same through curses `raw()`.
If (2) holds, **the severity I asserted in this issue was overstated.** I
wrote that Ctrl-C is "the single most likely way a player exits a terminal
game" and leaves them needing a blind `reset`. That reproducer does not fire.
The real exposure is narrower: `kill -INT` / `kill -QUIT`, and the window
before `term.New()` runs.
The fix is still worth having — an unhandled `kill -INT` mid-game does leave
the terminal raw, and the pre-init window is real — but it is a robustness
hardening, not the everyday footgun I described.
I have **not** accepted either correction. A fresh reviewer is verifying both
against the C sources and tcell's actual behavior, precisely because these
claims are now asserted as fact in code comments and `ARCHITECTURE.md`. If
either turns out to be wrong, that is a blocking finding against #23.
I will update this issue body once the verification lands, whichever way it
goes. Recording it here rather than quietly editing, so the reasoning stays
auditable — this is the second time in this backlog that a confident-sounding
premise of mine has needed checking (see PR #20, where my "negative `Which`"
requirement rested on arithmetic that does not happen).
Separately, the implementer found a **pre-existing data race** in the
SIGHUP/SIGTERM autosave path and correctly left it alone rather than fixing it
drive-by. Now tracked as #24.
Both corrections are now CONFIRMED by independent review against the C
sources. The premises in the issue body above are wrong. Treat this comment as
authoritative over the body.
leave() is NOT installed on SIGINT/SIGQUIT during play.md_init()
and setup() both take the #else branch to md_onsignal_default(),
which sets HUP/QUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to SIG_DFL and never touches SIGINT. During play, INT and QUIT are both SIG_DFL — no
handler, no endwin(). md_onsignal_autosave() has exactly one call site,
inside #ifdef DUMP. signal(SIGINT, leave) appears twice in the entire
tree, both endgame paths: main.c:305 (in quit(), after y) and rip.c:237 (in death()) — a second-Ctrl-C escape during scoring. main.c:332, which I cited, is the definition of leave(), not an
installation of it.
Ctrl-C never generated SIGINT here. tcell's devTty.Start calls term.MakeRaw, which clears ISIG. Ctrl-C therefore arrives as a key
event, and term/tcell.go:169 already returns '\x03'. C does the same
through curses raw().
Consequence: the severity I claimed in this issue was overstated. I wrote
that Ctrl-C is "the single most likely way a player exits a terminal game" and
that this is "the most reachable robustness gap in the port". Neither is true.
The stated reproducer does not fire at all.
The issue is still valid, on narrower grounds. The real exposure is:
kill -INT / kill -QUIT delivered to the process — no handler, terminal
left raw.
A SIGINT delivered to the foreground process group during the ! shell
escape.
A genuinely unarmed window found during review (recorded as B3 on PR #23):
raw mode is raised at cmd/rogue/main.go:42 but handlers install at :72,
and g.DeathDemo() sits between them and never returns — so rogue -d
runs raw with no handler at all. That one is a real functional gap and is
being fixed.
Note the window is afterterm.New(), not before it as PR #23 originally
stated — nothing is raw before that call.
I am leaving the body above unedited rather than quietly rewriting it, so the
record of what was claimed, challenged, and verified stays auditable. This is
the second premise of mine in this backlog to fail verification (see PR #20,
where my "cover negative Which" requirement rested on arithmetic that cannot
occur — readchar() returns a byte, so the value wraps to 234/202 instead).
Both were caught by agents doing exactly what they should: reading the source
instead of trusting the ticket.
**Both corrections are now CONFIRMED by independent review against the C
sources. The premises in the issue body above are wrong. Treat this comment as
authoritative over the body.**
1. **`leave()` is NOT installed on SIGINT/SIGQUIT during play.** `md_init()`
and `setup()` both take the `#else` branch to `md_onsignal_default()`,
which sets HUP/QUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to `SIG_DFL` and
**never touches SIGINT**. During play, INT and QUIT are both `SIG_DFL` — no
handler, no `endwin()`. `md_onsignal_autosave()` has exactly one call site,
inside `#ifdef DUMP`. `signal(SIGINT, leave)` appears twice in the entire
tree, both endgame paths: `main.c:305` (in `quit()`, after `y`) and
`rip.c:237` (in `death()`) — a second-Ctrl-C escape during scoring.
`main.c:332`, which I cited, is the **definition** of `leave()`, not an
installation of it.
2. **Ctrl-C never generated SIGINT here.** tcell's `devTty.Start` calls
`term.MakeRaw`, which clears `ISIG`. Ctrl-C therefore arrives as a key
event, and `term/tcell.go:169` already returns `'\x03'`. C does the same
through curses `raw()`.
**Consequence: the severity I claimed in this issue was overstated.** I wrote
that Ctrl-C is "the single most likely way a player exits a terminal game" and
that this is "the most reachable robustness gap in the port". Neither is true.
The stated reproducer does not fire at all.
**The issue is still valid, on narrower grounds.** The real exposure is:
- `kill -INT` / `kill -QUIT` delivered to the process — no handler, terminal
left raw.
- A SIGINT delivered to the foreground process group during the `!` shell
escape.
- A genuinely unarmed window found during review (recorded as B3 on PR #23):
raw mode is raised at `cmd/rogue/main.go:42` but handlers install at `:72`,
and `g.DeathDemo()` sits between them and **never returns** — so `rogue -d`
runs raw with no handler at all. That one is a real functional gap and is
being fixed.
Note the window is **after** `term.New()`, not before it as PR #23 originally
stated — nothing is raw before that call.
I am leaving the body above unedited rather than quietly rewriting it, so the
record of what was claimed, challenged, and verified stays auditable. This is
the second premise of mine in this backlog to fail verification (see PR #20,
where my "cover negative `Which`" requirement rested on arithmetic that cannot
occur — `readchar()` returns a `byte`, so the value wraps to 234/202 instead).
Both were caught by agents doing exactly what they should: reading the source
instead of trusting the ticket.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
C
main.c:332installsleave(int sig)— "leave quickly but courteously" —on SIGINT and SIGQUIT: it restores the terminal before exiting.
Go installs handlers for SIGHUP and SIGTERM only
(
cmd/rogue/main.go:106-119), both routed toAutoSave. There is no SIGINTor SIGQUIT handling.
So pressing Ctrl-C kills the process with tcell still holding the terminal
in raw mode. The user is dropped to a shell with no echo, no line
discipline, and a scrambled screen, needing a blind
reset.Ctrl-C is the single most likely way a player exits a terminal game they want
out of, so this is the most reachable robustness gap in the port. It is also
not listed in ARCHITECTURE.md §9's dropped-functionality table — it looks like
an oversight rather than a decision.
Related: C
main.c:211 tstp()(SIGTSTP suspend/resume) is also unported andunlisted.
term/tcell.go:176-203 ShellEscapecovers only the!shell-escapecase, not a real Ctrl-Z suspend.
Definition of done
minimum
Terminal.Fini, matching whatmyExitalready does.leave()semantics and this port'sautosave-on-signal model is made deliberately and documented in a code
comment — C's
leave()does not save, while this port's existing signalhandlers do. Do not silently pick one; state the reasoning.
deliberately dropped, with the reason.
headlessly; if it genuinely cannot be tested, say so in the PR with the
reason rather than skipping silently.
make checkfully green.TODO.mdupdated in the same commit.(closes #N).Implementation requirements
git show origin/c-master:main.c(leave,tstp, and thesignal()wiring inmain) first. Do NOT check out or modifyorigin/c-master.is deliberate and tested by the save/restore suite.
os.Exitin a way thatcorrupts the save file. If autosave-on-SIGINT is chosen, the ordering must
be correct and stated.
maketargets only. Do NOT modify.golangci.yml.Note
If you would rather Ctrl-C simply not save (C's actual behavior) than adopt
this port's autosave convention, say so on this issue and I'll pin the scope
before dispatching an implementer.
Implementation plan
Read the C reference first (
git show origin/c-master:onmain.c,mdport.c,mach_dep.c,save.c,rip.c). Two corrections to the issue's premise, both ofwhich change the shape of the fix but not the conclusion:
1.
leave()is not installed on SIGINT/SIGQUIT during play. The signalwiring lives in
mdport.c, notmain.c:md_init()callsmd_onsignal_exit()— SIGQUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to the rawexitfunction pointer, SIGHUP toSIG_DFL, SIGINT untouched.setup()(mach_dep.c:136) then callsmd_onsignal_default()in the shipped build — everything back toSIG_DFL.md_onsignal_autosave(), the variant that wires SIGHUP/SIGTERM toauto_saveand SIGINT toquit, is compiled in only under#ifdef DUMP.leave()is installed on SIGINT in exactly two places, both endgame:rip.c:237(death()) andmain.c:305(insidequit()once the player has confirmedy). Its job is to be a second-Ctrl-C escape hatch while the scoreboard is being printed.So C's shipped build has no handler at all during play, and the port's existing
SIGHUP/SIGTERM to
AutoSavealready follows themd_onsignal_autosaveintentrather than the shipped default. The port has to make a principled choice here;
"match C exactly" does not resolve it.
2. Ctrl-C cannot currently reach the process as SIGINT anyway. tcell's tty
Start()callsgolang.org/x/term.MakeRaw, which clearsISIG— so Ctrl-Carrives as key event
KeyCtrlC, whichterm/tcell.go translateKeyalready turnsinto byte
0x03for the command loop. C did the same thing (setup()callscurses
raw(), which also clearsISIG). The real exposure is thereforekill -INT/kill -QUITfrom another terminal, plus the window beforeterm.New()— still worth fixing, and still worth the terminal restore, but the"press Ctrl-C and land in a scrambled shell" reproducer as written does not fire.
That correction goes in the PR body; the fix is unchanged.
The save-vs-no-save decision: SIGINT/SIGQUIT will NOT save
Reasoning, to be written into the code comment:
leave()(endwin, discard pending output, exit); the only build that wires INT during play wires it toquit(), which confirms, scores, and exits without saving; SIGQUIT undermd_onsignal_autosavegoes toendit()tofatal()to endwin+exit, again without saving.auto_saveis reserved for HUP/TERM. Keeping HUP/TERM saving and INT/QUIT not saving reproduces C's split exactly.AutoSaveremoves the save file and then gob-encodes that live state, so doing it concurrently is a genuine race that can leave a torn file where a good save used to be. The HUP/TERM path accepts that because the process is about to die anyway and a best-effort save beats none; on INT there is nothing to rescue, so the correct move is the one with no corruption window at all.Race/ordering design
Rather than adding a second handler goroutine next to
installAutosave, all foursignals go to one buffered channel read by one goroutine that reads
exactly one signal. That removes the corruption window the issue warns about: a
SIGINT arriving while a SIGHUP-triggered
AutoSaveis mid-write can no longercall
os.Exitunderneath it — the second signal simply sits in the buffer and isnever read. Order within the handler is
AutoSave(HUP/TERM only), thenTerminal.Fini, thenos.Exit(0), matchingmyExitingame/rip.go.Work items
cmd/rogue/main.go: replaceinstallAutosavewith a singleinstallSignalHandlerscovering SIGHUP/SIGTERM (autosave, behaviorunchanged) and SIGINT/SIGQUIT (restore only), carrying the decision comment
above. The handler body is split into small injectable pieces (
saver/finisherinterfaces,exit func(int)) so it is testable headlessly.handled. Reason: with tcell in raw mode
VSUSP/ISIGare off so Ctrl-Z neverreaches the process as a signal, only
kill -TSTPcan; and a correctsuspend/resume would have to call
screen.Suspend/Resumefrom the signalgoroutine while the game goroutine may be inside
Render/PollEvent, whichis exactly the kind of race this issue forbids introducing. Doing it properly
means plumbing the signal into the input loop, which is a design change beyond
this issue. C's own wiring is vestigial here too:
tstpis armed only bymd_tstpresume(), which runs after a successfulrestore(), so a fresh Cgame never had it either. The
!shell escape already covers "get me to ashell".
tstp(), for SIGINT notrouting to the interactive
quit()prompt, and forauto_saveon thefault signals (SIGILL/SIGFPE/SIGSEGV/SIGBUS/SIGSYS/SIGTRAP), which in Go are
runtime panics and where encoding the state that just crashed would risk
writing a corrupt save. Also fixes the stale claim at section 5.3 that
"SIGTSTP/resume and resize are handled by tcell" — tcell registers only
SIGWINCH.
cmd/rogue/main_test.go(white-box,//nolint:testpackage): orderingtests over a fake terminal and a fake saver proving save-then-restore-then-exit
for HUP/TERM and restore-then-exit with no save for INT/QUIT, plus an
end-to-end test that delivers a real SIGINT and a real SIGQUIT to the test
process and asserts the terminal was restored before exit. What cannot be
tested headlessly is that the actual tty comes back out of raw mode, since
that needs a controlling terminal; that limitation will be stated in the PR
rather than skipped silently.
TODO.mdCompleted Steps entry in the same commit;Next Stepnot rotated(out-of-band issue work).
make fmt, thenmake checkgreen (.golangci.ymluntouched).Correction pending on two premises in this issue — both mine, both possibly
false. Flagging now so nobody reads the body above as settled fact while
PR #23 is under review.
The implementer of #23 challenged them after reading the actual C wiring:
"C installs
leave()on SIGINT/SIGQUIT (main.c:332)." Claimed wrong.The signal wiring is in
mdport.c, notmain.c, and the shipped build'ssetup()callsmd_onsignal_default()— everything back toSIG_DFL,no handler at all. The variant that wires HUP/TERM to
auto_saveand INTto
quitis#ifdef DUMPonly.leave()reportedly appears on SIGINT injust two endgame spots, as a second-Ctrl-C escape during scoring.
"Pressing Ctrl-C leaves the terminal raw." Claimed wrong, and this is
the one that matters. tcell's
Start()callsx/term.MakeRaw, whichclears
ISIG— so Ctrl-C never becomes SIGINT in the first place. Itarrives as
KeyCtrlCandtranslateKeyalready returns0x03. C does thesame through curses
raw().If (2) holds, the severity I asserted in this issue was overstated. I
wrote that Ctrl-C is "the single most likely way a player exits a terminal
game" and leaves them needing a blind
reset. That reproducer does not fire.The real exposure is narrower:
kill -INT/kill -QUIT, and the windowbefore
term.New()runs.The fix is still worth having — an unhandled
kill -INTmid-game does leavethe terminal raw, and the pre-init window is real — but it is a robustness
hardening, not the everyday footgun I described.
I have not accepted either correction. A fresh reviewer is verifying both
against the C sources and tcell's actual behavior, precisely because these
claims are now asserted as fact in code comments and
ARCHITECTURE.md. Ifeither turns out to be wrong, that is a blocking finding against #23.
I will update this issue body once the verification lands, whichever way it
goes. Recording it here rather than quietly editing, so the reasoning stays
auditable — this is the second time in this backlog that a confident-sounding
premise of mine has needed checking (see PR #20, where my "negative
Which"requirement rested on arithmetic that does not happen).
Separately, the implementer found a pre-existing data race in the
SIGHUP/SIGTERM autosave path and correctly left it alone rather than fixing it
drive-by. Now tracked as #24.
Both corrections are now CONFIRMED by independent review against the C
sources. The premises in the issue body above are wrong. Treat this comment as
authoritative over the body.
leave()is NOT installed on SIGINT/SIGQUIT during play.md_init()and
setup()both take the#elsebranch tomd_onsignal_default(),which sets HUP/QUIT/ILL/TRAP/IOT/EMT/FPE/BUS/SEGV/SYS/TERM to
SIG_DFLandnever touches SIGINT. During play, INT and QUIT are both
SIG_DFL— nohandler, no
endwin().md_onsignal_autosave()has exactly one call site,inside
#ifdef DUMP.signal(SIGINT, leave)appears twice in the entiretree, both endgame paths:
main.c:305(inquit(), aftery) andrip.c:237(indeath()) — a second-Ctrl-C escape during scoring.main.c:332, which I cited, is the definition ofleave(), not aninstallation of it.
Ctrl-C never generated SIGINT here. tcell's
devTty.Startcallsterm.MakeRaw, which clearsISIG. Ctrl-C therefore arrives as a keyevent, and
term/tcell.go:169already returns'\x03'. C does the samethrough curses
raw().Consequence: the severity I claimed in this issue was overstated. I wrote
that Ctrl-C is "the single most likely way a player exits a terminal game" and
that this is "the most reachable robustness gap in the port". Neither is true.
The stated reproducer does not fire at all.
The issue is still valid, on narrower grounds. The real exposure is:
kill -INT/kill -QUITdelivered to the process — no handler, terminalleft raw.
!shellescape.
raw mode is raised at
cmd/rogue/main.go:42but handlers install at:72,and
g.DeathDemo()sits between them and never returns — sorogue -druns raw with no handler at all. That one is a real functional gap and is
being fixed.
Note the window is after
term.New(), not before it as PR #23 originallystated — nothing is raw before that call.
I am leaving the body above unedited rather than quietly rewriting it, so the
record of what was claimed, challenged, and verified stays auditable. This is
the second premise of mine in this backlog to fail verification (see PR #20,
where my "cover negative
Which" requirement rested on arithmetic that cannotoccur —
readchar()returns abyte, so the value wraps to 234/202 instead).Both were caught by agents doing exactly what they should: reading the source
instead of trusting the ticket.