From 0a1b377005ed8fccaa788b93209317e5c4de68fa Mon Sep 17 00:00:00 2001 From: mike-diff Date: Thu, 18 Jun 2026 15:35:19 -0700 Subject: [PATCH] fix(tui): make steer, escape, enter, and double ctrl-c work under extended-keys The interactive control plane did not reliably drive a running turn. A typed steer only took effect at the next iteration boundary, and Escape, Enter, and Ctrl-C did nothing under terminals using the extended-keys (CSI-u) keyboard protocol, where those keys arrive as ESC[27u, ESC[57414u (kitty functional Enter) or ESC[13u, and ESC[99;5u key sequences rather than a bare byte or an OS signal. - steer: typing during a turn now cancels the in-flight turn at once and runs the queued text as the next turn; the queued note is highlighted in cyan. A sticky abort flag closes the between-iteration window where a cancel was previously dropped, so Escape aborts the judge and worker phases too. - escape: detection keys off the sequence introducer rather than a fragile timer, and the CSI-u Escape (ESC[27u) is decoded and routed to cancel. - enter: the CSI-u Enter (codepoint 13 or kitty functional 57414) is normalized back to a plain Return so the one submit/steer path handles it; a modified Enter still inserts a newline. - ctrl-c: force-exit no longer blocks on the kill grace period; the CSI-u Ctrl-C (ESC[99;5u) and a raw 0x03 are routed to the same quit path as the OS signal, so a double press exits even under extended-keys. - add a /stop command (the typed twin of Escape) and an opt-in SESH_DEBUG trace of the control-plane events. The proc tool now threads the turn context so it shares bash's cancellable contract. --- harness/drive_test.go | 84 ++++++++++++ harness/harness.go | 83 +++++++++-- harness/proc.go | 33 ++++- harness/proc_test.go | 41 +++++- harness/repl.go | 13 +- harness/tools.go | 2 +- harness/tui.go | 177 +++++++++++++++++++++--- harness/tui_test.go | 313 ++++++++++++++++++++++++++++++++++++++++-- 8 files changed, 697 insertions(+), 49 deletions(-) diff --git a/harness/drive_test.go b/harness/drive_test.go index d1445ec..0c2da24 100644 --- a/harness/drive_test.go +++ b/harness/drive_test.go @@ -4,6 +4,7 @@ import ( "context" "strings" "testing" + "time" "github.com/mike-diff/sesh/agent" ) @@ -345,6 +346,89 @@ func TestInterruptsQueue(t *testing.T) { } } +// TestInterruptsCancelInGap: an Escape that lands between iterations, after one +// turn's context is detached and before the next is minted, still aborts the +// next iteration. Without the sticky-abort window fix the cancel hits a nil +// context and vanishes, and the freshly minted context comes back live. +// Breaker: drop the aborted flag (or its consume in turnContext) and the next +// context is not cancelled. +func TestInterruptsCancelInGap(t *testing.T) { + in := &interrupts{} + _, done := in.turnContext() // a turn runs + done() // the worker detaches its context: in.cancel is now nil + in.cancelCurrent() // Escape in the gap: no live context, but the abort must stick + + ctx, ndone := in.turnContext() // the next iteration mints a fresh context + defer ndone() + select { + case <-ctx.Done(): + // good: the gap-Escape aborted the next iteration before it could run + default: + t.Fatal("an Escape between iterations must cancel the next context, not vanish") + } + + // The abort is consumed once: a later request after a reset starts live again. + in.resetAbort() + ctx2, ndone2 := in.turnContext() + defer ndone2() + select { + case <-ctx2.Done(): + t.Fatal("a fresh request after reset must not inherit a stale abort") + default: + } +} + +// TestInterruptsResetClearsStaleAbort: an Escape that arrives as a drive is +// already stopping must not cancel the next request the user types. resetAbort +// at the top-level boundary clears it. Breaker: drop the resetAbort call and a +// steer-cancel's aborted flag leaks into the next turn's context. +func TestInterruptsResetClearsStaleAbort(t *testing.T) { + in := &interrupts{} + in.cancelCurrent() // an abort with no live turn (the gap, or a steer that just cancelled) + in.resetAbort() // the turn is fully over; the abort is stale + ctx, done := in.turnContext() + defer done() + select { + case <-ctx.Done(): + t.Fatal("a reset must clear the abort so the next context starts live") + default: + } +} + +// TestInterruptsCtrlCDoublePress: one Ctrl-C warns and does not quit; a second +// within the window force-quits via cleanup. The spy cleanup is the only +// observable effect, so os.Exit stays out of the unit (the signal goroutine and +// the editor hook own the exit at the call boundary). Breaker: drop the +// double-press check (quit on the first press) and the single-press case sees +// cleanup fire; drop the double branch and the second press never fires it. +func TestInterruptsCtrlCDoublePress(t *testing.T) { + var cleaned int + in := newInterruptsTest(func() { cleaned++ }) + + if forced := in.ctrlC(); forced || cleaned != 0 { + t.Fatalf("first Ctrl-C must warn, not quit: forced=%v cleaned=%d", forced, cleaned) + } + if forced := in.ctrlC(); !forced || cleaned != 1 { + t.Fatalf("second Ctrl-C within the window must quit via cleanup: forced=%v cleaned=%d", forced, cleaned) + } + + // A press after the window lapses is a fresh first press: it warns, not quits. + in.last = time.Now().Add(-2 * doublePressWindow) + if forced := in.ctrlC(); forced { + t.Fatal("a Ctrl-C after the window lapses must warn, not quit") + } + if cleaned != 1 { + t.Fatalf("a lapsed press must not re-fire cleanup: cleaned=%d", cleaned) + } +} + +// newInterruptsTest builds an interrupts with the given cleanup spy but without +// the signal.Notify wiring of the real constructor, so the unit observes ctrlC +// in isolation (no live OS-signal goroutine, no os.Exit). +func newInterruptsTest(cleanup func()) *interrupts { + return &interrupts{cleanup: cleanup} +} + // TestDriveJudgeUnavailable: no verdict means no mandate to keep spending. func TestDriveJudgeUnavailable(t *testing.T) { p := &seqChat{} // judge call errors immediately diff --git a/harness/harness.go b/harness/harness.go index f111e61..9169e91 100644 --- a/harness/harness.go +++ b/harness/harness.go @@ -36,12 +36,14 @@ var ( yellow string red string green string + cyan string reset string ) func initColor() { if useColor() { dim, yellow, red, green, reset = "\033[2m", "\033[33m", "\033[31m", "\033[32m", "\033[0m" + cyan = "\033[36m" } } @@ -417,12 +419,31 @@ func Main() { }() } - // Escape cancels the running turn; Ctrl-C quits (twice within two seconds, - // after restoring the terminal). + // Escape cancels the running turn; Ctrl-C quits (twice within two seconds). + // The second press is a force quit: restore the terminal first so raw mode + // never leaks, then signal owned process groups best-effort with no grace + // window, then release the lock. The OS reaps any group that has not died + // yet, so a stubborn background process cannot delay the exit. The graceful + // reapAll stays on the clean single-quit and normal-exit paths. intr := newInterrupts(func() { con.Close() - r.goodbye() + if r.procs != nil { + r.procs.killAllNow() + } + releaseLock(r.sess.ID) }) + // Extended-keys terminals deliver Ctrl-C as a keystroke, not an OS signal, so + // the editor needs a console-level hook to reach the same quit semantics. It + // is console-level (not per-turn) because Ctrl-C must work at the idle prompt + // and during a turn alike; os.Exit stays here at the boundary, never inside + // ctrlC, so the editor goroutine that calls this never holds its own mutex. + if t, ok := con.(*tuiConsole); ok { + t.onCtrlC = func() { + if intr.ctrlC() { + os.Exit(130) + } + } + } say := func(f string, a ...any) { emit("%s "+f+"%s\n", append(append([]any{dim}, a...), reset)...) @@ -490,6 +511,7 @@ func Main() { tc.attendTurn(turnAttend{done: doneCh, cancel: intr.cancelCurrent, queue: intr.enqueue}) <-doneCh // the worker is finished (attendTurn can also return on EOF); never overlap turns stopSpin() + intr.resetAbort() // the turn is over; an Escape now is for the next prompt, not a stale carry-over if q := intr.drain(); len(q) > 0 { // typed after the last boundary: run next pending = strings.Join(q, "\n") } @@ -514,6 +536,7 @@ func Main() { type interrupts struct { mu sync.Mutex cancel context.CancelFunc // non-nil while a turn is running + aborted bool // an Escape landed between iterations; the next context starts cancelled last time.Time cleanup func() queued []string // messages typed during a turn, drained at the next boundary @@ -529,25 +552,45 @@ func newInterrupts(cleanup func()) *interrupts { sigc := make(chan os.Signal, 1) signal.Notify(sigc, os.Interrupt) go func() { + // Some terminals deliver Ctrl-C as an OS signal; others (extended-keys + // mode) deliver it as a keystroke that the editor routes to ctrlC. Both + // triggers share this one body, and os.Exit lives at the call boundary so + // ctrlC itself stays unit-testable around the spy-observable cleanup. for range sigc { - in.mu.Lock() - double := time.Since(in.last) < doublePressWindow - in.last = time.Now() - in.mu.Unlock() - if double { - in.cleanup() + if in.ctrlC() { os.Exit(130) } - emit("\n%s (ctrl-c again to quit; esc cancels the turn)%s\n", yellow, reset) } }() return in } -// cancelCurrent aborts the turn in flight, if any (the live editor's Escape). +// ctrlC applies one Ctrl-C press: a stray press warns, a second within the +// window force-quits via cleanup. It reports whether this press force-quit so +// the caller can os.Exit(130) at the boundary, keeping os.Exit out of the unit +// under test. Both the signal goroutine and the live editor call this. +func (in *interrupts) ctrlC() bool { + in.mu.Lock() + double := time.Since(in.last) < doublePressWindow + in.last = time.Now() + in.mu.Unlock() + if double { + in.cleanup() + return true + } + emit("\n%s (ctrl-c again to quit; esc cancels the turn)%s\n", yellow, reset) + return false +} + +// cancelCurrent aborts the turn in flight (the live editor's Escape). It also +// records the abort even when no context is live: between iterations the worker +// detaches one context before the next is minted, and an Escape in that window +// would otherwise vanish. The sticky flag makes the next turnContext start +// already cancelled, so the abort reaches the iteration it was meant for. func (in *interrupts) cancelCurrent() { in.mu.Lock() c := in.cancel + in.aborted = true in.mu.Unlock() if c != nil { c() @@ -571,12 +614,19 @@ func (in *interrupts) drain() []string { } // turnContext hands out a cancellable context for one turn and the cleanup -// that detaches it from the watcher. +// that detaches it from the watcher. An Escape that arrived between iterations +// (the sticky aborted flag) cancels the fresh context at once, so the abort is +// honored by the iteration it was aimed at instead of being dropped in the gap. func (in *interrupts) turnContext() (context.Context, func()) { ctx, cancel := context.WithCancel(context.Background()) in.mu.Lock() in.cancel = cancel + wasAborted := in.aborted + in.aborted = false in.mu.Unlock() + if wasAborted { + cancel() + } return ctx, func() { in.mu.Lock() in.cancel = nil @@ -584,3 +634,12 @@ func (in *interrupts) turnContext() (context.Context, func()) { cancel() } } + +// resetAbort clears a stale abort once a turn has fully ended, so an Escape that +// landed as the drive was already stopping cannot cancel the next request the +// user types. Called at the top-level boundary, not inside a drive cycle. +func (in *interrupts) resetAbort() { + in.mu.Lock() + in.aborted = false + in.mu.Unlock() +} diff --git a/harness/proc.go b/harness/proc.go index a23017c..d181cad 100644 --- a/harness/proc.go +++ b/harness/proc.go @@ -409,6 +409,32 @@ func (m *procManager) reapAll() { os.RemoveAll(runDir(m.sessionID)) } +// killAllNow is the force-quit reaper: SIGTERM then SIGKILL every owned group +// back to back, with no grace window, so a second Ctrl-C exits at once instead +// of blocking on a stubborn process. The signals are sent in the same loop so +// the kernel queues SIGKILL right behind SIGTERM; the caller exits immediately +// after and the OS reaps any group that has not died yet. Returns the number of +// groups it signalled, so a test can prove it acted without sleeping. +func (m *procManager) killAllNow() int { + m.mu.Lock() + procs := append([]*Proc(nil), m.procs...) + m.mu.Unlock() + signalled := 0 + for _, p := range procs { + p.mu.Lock() + pgid := p.pgid + dead := p.ended || pgid <= 0 + p.mu.Unlock() + if dead { + continue + } + syscall.Kill(-pgid, syscall.SIGTERM) + syscall.Kill(-pgid, syscall.SIGKILL) + signalled++ + } + return signalled +} + func (m *procManager) byID(id string) *Proc { m.mu.Lock() defer m.mu.Unlock() @@ -531,7 +557,12 @@ type procArgs struct { Filter string `json:"filter"` } -func (m *procManager) runTool(raw json.RawMessage) (string, bool) { +// runTool dispatches the proc tool. ctx carries the turn's cancellation; every +// action here is a fast bookkeeping call (start spawns and returns, the rest +// only read or signal), so none blocks on it. The parameter keeps the proc +// tool's Run on the same cancellable contract as bash, so a future blocking +// action inherits cancellation instead of having to retrofit the signature. +func (m *procManager) runTool(ctx context.Context, raw json.RawMessage) (string, bool) { var a procArgs if err := json.Unmarshal(raw, &a); err != nil { return "invalid proc input: " + err.Error(), true diff --git a/harness/proc_test.go b/harness/proc_test.go index 8f329ec..c003139 100644 --- a/harness/proc_test.go +++ b/harness/proc_test.go @@ -61,6 +61,34 @@ func TestProcStopKillsGroup(t *testing.T) { } } +// TestKillAllNowReturnsPromptly: the force-quit reaper signals every owned group +// and returns without the killGrace sleep, so a second Ctrl-C exits at once even +// with a stubborn process running. Breaker: put a time.Sleep(killGrace) back on +// this path and the elapsed time exceeds the grace window. +func TestKillAllNowReturnsPromptly(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + m := newProcManager("scale-force") + // A process that ignores SIGTERM and keeps running, so only SIGKILL ends it: + // the helper must not wait for it to die before returning. + p, _, err := m.start("trap '' TERM; sleep 30", "") + if err != nil { + t.Fatal(err) + } + if !waitFor(2*time.Second, func() bool { return p.pgid > 0 }) { + t.Fatal("process group never came up") + } + start := time.Now() + n := m.killAllNow() + elapsed := time.Since(start) + if n != 1 { + t.Fatalf("must signal the one owned group, got %d", n) + } + if elapsed >= killGrace { + t.Fatalf("force reaper blocked for %v; it must not sleep killGrace (%v)", elapsed, killGrace) + } + m.reapAll() // clean up the run dir and make sure nothing leaks past the test +} + // TestAutoPromote: a command that outlives the promote window becomes a tracked // background process (handle returned, kept in the registry); a quick command // returns synchronously and leaves nothing behind. Breaker: revert doBash to @@ -311,24 +339,25 @@ func TestForegroundNoTrailingNewline(t *testing.T) { func TestProcToolDispatch(t *testing.T) { t.Setenv("HOME", t.TempDir()) m := newProcManager("scale-tool") - out, isErr := m.runTool([]byte(`{"action":"start","command":"echo hello; sleep 30","name":"svc"}`)) + ctx := context.Background() + out, isErr := m.runTool(ctx, []byte(`{"action":"start","command":"echo hello; sleep 30","name":"svc"}`)) if isErr || !strings.Contains(out, "started proc-1") { t.Fatalf("start: %q err=%v", out, isErr) } - if out, _ := m.runTool([]byte(`{"action":"list"}`)); !strings.Contains(out, "proc-1") || !strings.Contains(out, "svc") { + if out, _ := m.runTool(ctx, []byte(`{"action":"list"}`)); !strings.Contains(out, "proc-1") || !strings.Contains(out, "svc") { t.Fatalf("list must show the process: %q", out) } waitFor(2*time.Second, func() bool { - o, _ := m.runTool([]byte(`{"action":"logs","id":"proc-1"}`)) + o, _ := m.runTool(ctx, []byte(`{"action":"logs","id":"proc-1"}`)) return strings.Contains(o, "hello") }) - if out, _ := m.runTool([]byte(`{"action":"logs","id":"proc-1"}`)); strings.Contains(out, "hello") { + if out, _ := m.runTool(ctx, []byte(`{"action":"logs","id":"proc-1"}`)); strings.Contains(out, "hello") { t.Fatalf("a second logs read must not repeat consumed output: %q", out) } - if out, isErr := m.runTool([]byte(`{"action":"stop","id":"proc-1"}`)); isErr || !strings.Contains(out, "stopped proc-1") { + if out, isErr := m.runTool(ctx, []byte(`{"action":"stop","id":"proc-1"}`)); isErr || !strings.Contains(out, "stopped proc-1") { t.Fatalf("stop: %q err=%v", out, isErr) } - if out, _ := m.runTool([]byte(`{"action":"bogus"}`)); !strings.Contains(out, "unknown proc action") { + if out, _ := m.runTool(ctx, []byte(`{"action":"bogus"}`)); !strings.Contains(out, "unknown proc action") { t.Fatalf("unknown action must be reported: %q", out) } m.reapAll() diff --git a/harness/repl.go b/harness/repl.go index 98ca2fa..495fe69 100644 --- a/harness/repl.go +++ b/harness/repl.go @@ -248,7 +248,7 @@ func (r *repl) spin() (stop func()) { r.spinMu.Lock() base := r.spinBase r.spinMu.Unlock() - r.con.SetStatus(fmt.Sprintf("%s · working %ds (ctrl-c cancels)", base, secs)) + r.con.SetStatus(fmt.Sprintf("%s · working %ds (press esc or type /stop to cancel)", base, secs)) r.con.SetTitle(fmt.Sprintf("sesh · working %ds", secs)) } } @@ -315,6 +315,7 @@ var slashCommands = []slashCommand{ {name: "/compact", run: func(r *repl, _ string) { r.compactCmd() }}, {name: "/settings", run: func(r *repl, _ string) { r.settingsCmd() }}, {name: "/copy", run: func(r *repl, _ string) { r.copyCmd() }}, + {name: "/stop", run: func(r *repl, _ string) { r.stopCmd() }}, {name: "/help", run: func(r *repl, _ string) { r.helpCmd() }}, {name: "/exit", quit: true}, {name: "/quit", quit: true}, @@ -404,6 +405,13 @@ func (r *repl) copyCmd() { emit("%s copied last response to clipboard, %d lines (%s)%s\n\n", dim, 1+strings.Count(text, "\n"), via, reset) } +// stopCmd handles /stop typed between turns. The version that aborts a running +// turn lives in the live editor (a line typed during a turn never reaches this +// dispatch); here there is nothing to stop, so it only says so. +func (r *repl) stopCmd() { + emit("%s no turn is running%s\n\n", dim, reset) +} + // lastAssistantText returns the most recent assistant turn that carried text; // turns that only made tool calls have none. Empty when nothing has been said. func (r *repl) lastAssistantText() string { @@ -467,6 +475,7 @@ func (r *repl) helpCmd() { /compact summarize history in place (lossier than /handoff) /settings session settings picker (show thinking) /copy copy the last response to the clipboard (clean source) + /stop abort the running turn (same as esc); nothing sent next /help this help exit, /exit quit (ctrl-d works too); prints how to resume keys: while a turn runs, type to queue a steer (sent at the next step) or esc @@ -512,7 +521,7 @@ func (r *repl) banner(cwd string, ask bool, resumed int, buildErr error) { emit("%sno active provider (%v). run /provider add, or /provider .%s\n", yellow, buildErr, reset) } } - emit("%scommands: /provider · /model · /compact · /settings · /help · exit — ctrl-c cancels a turn%s\n\n", dim, reset) + emit("%scommands: /provider · /model · /compact · /settings · /help · exit · esc or /stop cancels a turn, ctrl-c quits%s\n\n", dim, reset) } // applyProvider rebuilds the live provider from a profile, re-discovers diff --git a/harness/tools.go b/harness/tools.go index 8fc1b1a..f7b1900 100644 --- a/harness/tools.go +++ b/harness/tools.go @@ -76,7 +76,7 @@ func builtinTools(unsafePaths bool, pm *procManager) []agent.Tool { if pm != nil { tools = append(tools, agent.Tool{ Def: agent.ToolDef{Name: "proc", Description: procToolDesc, Schema: procSchema()}, - Run: func(_ context.Context, raw json.RawMessage) (string, bool) { return pm.runTool(raw) }, + Run: func(ctx context.Context, raw json.RawMessage) (string, bool) { return pm.runTool(ctx, raw) }, Parallel: false, }) } diff --git a/harness/tui.go b/harness/tui.go index c8e83e4..d8e9a7a 100644 --- a/harness/tui.go +++ b/harness/tui.go @@ -179,6 +179,7 @@ type tuiConsole struct { col int // logical column at the content tail (0 = fresh line) pad bool // footer was drawn after a partial line, behind a pushed \n atExit func() // run on signal-driven exit (reap owned processes) + onCtrlC func() // a Ctrl-C keystroke (extended-keys CSI-u, or raw 0x03) reaches quit through this; nil disables // the input editor's state prompt string @@ -790,15 +791,16 @@ func (t *tuiConsole) endInput(echo string) { t.drawFooterLocked() } -// noteQueuedLocked drops a dim transcript line confirming a typed message was +// noteQueuedLocked drops a transcript line confirming a typed message was // queued to steer the running turn, so the user sees it was captured rather than -// dropped. Caller holds the mutex. +// dropped. Cyan, not dim, because a steer interrupts the turn now and that is +// worth more than a muted aside. Caller holds the mutex. func (t *tuiConsole) noteQueuedLocked(msg string) { if r := []rune(msg); len(r) > 60 { msg = string(r[:60]) + "..." } t.removeFooterLocked() - t.writeLocked(fmt.Sprintf("%s queued, will steer at the next step: %s%s\n", dim, msg, reset)) + t.writeLocked(fmt.Sprintf("%s queued, steering now: %s%s\n", cyan, msg, reset)) t.drawFooterLocked() } @@ -914,13 +916,29 @@ func (t *tuiConsole) readLineMode(prompt string, mask bool, turn *turnAttend) (s t.endInput("") t.mu.Unlock() return "", io.EOF - case r == '\r' && turn != nil: // Enter while working: queue the message, keep editing + case r == '\r' && turn != nil: // Enter while working: queue the steer and cancel now // A queued steer carries text only: images pasted into it are not sent // (per the feature's non-goal), but their labels still render so no raw // token rune leaks into the steer text. text, imgs := t.composeMessage() - if line := strings.TrimSpace(text); line != "" { + if line := strings.TrimSpace(text); strings.EqualFold(line, "/stop") { + // /stop is the typed twin of Escape: abort the in-flight turn and + // return to the prompt, but do NOT queue anything, so the model gets + // no next turn. It is intercepted here because a line typed during a + // turn never reaches the between-turns command dispatch. + turn.cancel() + t.removeFooterLocked() + t.writeLocked(fmt.Sprintf("%s stopping the current turn%s\n", cyan, reset)) + t.drawFooterLocked() + t.buf, t.pos, t.snippets, t.images = nil, 0, nil, nil + } else if line != "" { turn.queue(line) + // Cancel the in-flight turn so the steer is acted on now, not at the + // next natural boundary. The queued text is not lost: the worker's + // runTurn returns a cancel error, the live loop drains the queue into + // the next turn's input, and the cancel takes a different lock than + // this one so holding the editor mutex here is safe. + turn.cancel() t.noteQueuedLocked(line) if len(imgs) > 0 { t.removeFooterLocked() @@ -954,6 +972,16 @@ func (t *tuiConsole) readLineMode(prompt string, mask bool, turn *turnAttend) (s return line, nil case r == '\n': // Ctrl-J: newline everywhere, no protocol needed t.insertLocked('\n') + case r == 0x03: // Ctrl-C: a raw 0x03 byte (terminals that do not send a signal) + // Route to the same quit path as the signal and the CSI-u form. onCtrlC + // must run without t.mu: its force-quit closes the console and the warning + // Prints, both taking the console mutex, which would deadlock under t.mu. + // Mirror the Ctrl+V / Escape branches that already drop the lock. + t.mu.Unlock() + if t.onCtrlC != nil { + t.onCtrlC() + } + t.mu.Lock() case r == 0x04: // Ctrl-D on an empty line ends the session (not mid-turn) if turn == nil && len(t.buf) == 0 { t.endInput("") @@ -991,7 +1019,14 @@ func (t *tuiConsole) readLineMode(prompt string, mask bool, turn *turnAttend) (s if turn != nil && t.escIsBare() { // bare Escape cancels the running turn turn.cancel() } else { - t.handleEscape() + // onEscape fires only if the sequence decodes to the Escape KEY + // itself (CSI-u mode delivers it as a sequence, not a bare 0x1b); + // it cancels a live turn and is a no-op between turns. + t.handleEscape(func() { + if turn != nil { + turn.cancel() + } + }) } t.mu.Lock() case r >= 0x20: // printable, unicode included via ReadRune @@ -1129,10 +1164,20 @@ func (t *tuiConsole) takeImages() []agent.Image { // arrows move the cursor or walk history, home/end/delete edit, Shift+Enter // inserts a newline (CSI 13;2u from the Kitty disambiguation flag, or CSI // 27;2;13~ from terminals/tmux in extended-keys mode), and a bracketed-paste -// begin marker pulls the whole paste into the buffer. Alt+V (ESC then a plain -// v) is the Ctrl+V fallback for terminals like Windows Terminal that swallow -// Ctrl+V for their own paste; it runs the same image-capture pipeline. -func (t *tuiConsole) handleEscape() { +// begin marker pulls the whole paste into the buffer. Under extended-keys mode +// plain Enter also arrives here as a CSI-u sequence (codepoint 13 or kitty +// functional 57414): an unmodified one is normalized back to a plain Return and +// ungot so readLineMode's single submit/steer path handles it, while a modified +// one inserts a newline. Alt+V (ESC then a plain v) is the Ctrl+V fallback for +// terminals like Windows Terminal that swallow Ctrl+V for their own paste; it +// runs the same image-capture pipeline. +// +// onEscape, when set, is invoked instead of any editing action if the sequence +// decodes to the Escape KEY itself: under extended-keys (CSI-u) mode the +// Escape key arrives as a sequence (CSI 27 u), not a bare 0x1b, so the bare +// path in readLineMode never sees it. Routing it here keeps a single consumer +// of the sequence bytes (no double-read) while still reaching cancel. +func (t *tuiConsole) handleEscape(onEscape func()) { // Peek the byte after ESC: a plain v is Alt+V, not a CSI/SS3 introducer, so // route it to the capture pipeline before the sequence decoder. The capture // writes to the transcript (Print relocks), so it must run unlocked, exactly @@ -1162,11 +1207,47 @@ func (t *tuiConsole) handleEscape() { t.mu.Unlock() return } + if isCSIUEscape(params, final) { + // The Escape KEY under extended-keys mode. It must reach cancel exactly + // like a bare 0x1b; with no live turn it is a harmless no-op (the bytes + // are already consumed, so nothing strays into the buffer). + if onEscape != nil { + onEscape() + } + return + } + if isCSIUCtrlC(params, final) { + // Ctrl-C under extended-keys mode arrives here as a keystroke, never as an + // OS signal, so it must reach the same quit path the signal handler uses. + // onCtrlC is called WITHOUT t.mu held (this branch runs before the lock + // below): its force-quit closes the console and the first-press warning + // Prints, both of which take the console mutex and would deadlock under t.mu. + if t.onCtrlC != nil { + t.onCtrlC() + } + return + } + if enter, mod := isCSIUEnter(params, final); enter { + // Under extended-keys / kitty mode the Enter key is reported as a CSI-u + // sequence (codepoint 13 or functional 57414), never a bare \r, so the + // bare submit/steer path in readLineMode never sees it. Normalize an + // unmodified Enter back to a plain \r and unget it: readLineMode's next + // loop iteration reads it from t.ungot and runs the single submit/steer + // path unchanged. A modified Enter (Shift/Alt) inserts a newline instead, + // matching Ctrl-J and the kitty 13;2 / 13;3 forms below. + if mod { + t.mu.Lock() + t.insertLocked('\n') + t.mu.Unlock() + } else { + t.unget('\r') + } + return + } t.mu.Lock() defer t.mu.Unlock() switch { - case (final == 'u' && (params == "13;2" || params == "13;3")) || // kitty: shift/alt+enter - (final == '~' && (params == "27;2;13" || params == "27;3;13")): // extended keys: tmux/xterm + case final == '~' && (params == "27;2;13" || params == "27;3;13"): // shift/alt+enter, the legacy '~' form (the kitty 'u' form is decoded above) t.insertLocked('\n') case final == 'A': // up: move a visual row, else walk history at the top if !t.cursorUpLocked() { @@ -1195,6 +1276,54 @@ func (t *tuiConsole) handleEscape() { } } +// isCSIUEscape reports whether a decoded sequence (params, final) is the Escape +// KEY under extended-keys / CSI-u mode. There the key is reported by its Unicode +// codepoint in the first parameter field, and Escape is codepoint 27: bare as +// CSI 27 u, or with a modifier as CSI 27; u. Keying on the codepoint, not +// just the final 'u', is load-bearing: extended Enter is also final 'u' but +// codepoint 13 (CSI 13;2u), and must keep inserting a newline rather than cancel. +func isCSIUEscape(params string, final rune) bool { + return final == 'u' && (params == "27" || strings.HasPrefix(params, "27;")) +} + +// isCSIUCtrlC reports whether a decoded sequence is the Ctrl-C key under +// extended-keys / CSI-u mode. The key is codepoint 99 ('c') with a Ctrl +// modifier, reported as CSI 99;5 u (modifier 5 == Ctrl). Both the codepoint and +// the modifier are load-bearing: unmodified 'c' is CSI 99 u or CSI 99;1 u and +// must keep typing the letter, not quit, so a bare or modifier-1 form is excluded. +func isCSIUCtrlC(params string, final rune) bool { + if final != 'u' { + return false + } + cp, mods, hasMods := strings.Cut(params, ";") + if cp != "99" || !hasMods { + return false + } + // The modifier field is the bitmask + 1; Ctrl is bit 4, so any reported value + // with that bit set (e.g. 5 = Ctrl, 7 = Ctrl+Alt) counts. Modifier 1 is "none". + m, err := strconv.Atoi(mods) + return err == nil && (m-1)&4 != 0 +} + +// isCSIUEnter reports whether a decoded sequence is the Enter key under +// extended-keys / CSI-u mode, and whether a modifier is held. Enter is reported +// by codepoint in the first field: standard codepoint 13, or kitty's functional +// code 57414 (0xE006). modified is true when a modifier field is present and is +// not "1" ("none"): Shift+Enter and Alt+Enter (e.g. 13;2, 57414;3) are modified +// and must insert a newline, while bare 13/57414 or explicit 13;1/57414;1 are +// unmodified and must submit. The codepoint check excludes Escape (27) and +// Ctrl-C (99), which also end in 'u'. +func isCSIUEnter(params string, final rune) (enter, modified bool) { + if final != 'u' { + return false, false + } + cp, mods, hasMods := strings.Cut(params, ";") + if cp != "13" && cp != "57414" { + return false, false + } + return true, hasMods && mods != "1" +} + // snippet thresholds: pastes beyond either collapse to an atomic token so the // input row stays readable; the full content is sent on submit. const ( @@ -1491,20 +1620,34 @@ func (t *tuiConsole) Select(title string, items []string, current int) (int, err } } -// escIsBare reports whether an Esc keypress arrived alone: escape sequences -// land as a burst, so if no rune follows within a short window it was the bare -// key. A peeked rune is put back for the real decoder. +// escIsBare reports whether an Esc keypress arrived alone (the user pressing the +// key to cancel) rather than as the lead byte of an escape sequence. It decides +// by the byte that FOLLOWS Esc, not by timing: a CSI or SS3 sequence's introducer +// ([ or O) arrives contiguously behind Esc, while a bare Escape is followed by +// nothing or by an unrelated keystroke. Keying off the introducer is what makes +// this robust under tmux/SSH, where tmux's escape-time can delay a lone Esc past +// any tight timer and a presence-based test would misread it as a sequence. A +// peeked introducer is put back for handleEscape; a peeked non-introducer is put +// back so it is processed as the user's next keystroke. func (t *tuiConsole) escIsBare() bool { if len(t.ungot) > 0 { - return false + // A pushed-back rune only exists because a prior peek read it; treat its + // value the same as a freshly peeked byte so a non-introducer still reads + // as bare. + r := t.ungot[len(t.ungot)-1] + return r != '[' && r != 'O' } select { case r, ok := <-t.runes: if ok { t.unget(r) + return r != '[' && r != 'O' // only [ (CSI) and O (SS3) introduce a sequence } - return !ok // a closed stream after Esc counts as bare + return true // a closed stream after Esc counts as bare case <-time.After(25 * time.Millisecond): + // No byte arrived contiguously: a real sequence's introducer would already + // be here, so this was the bare key. The window only needs to cover the + // contiguous burst, not tmux's delay before the lone Esc reaches us. return true } } diff --git a/harness/tui_test.go b/harness/tui_test.go index ff253ab..bc03d36 100644 --- a/harness/tui_test.go +++ b/harness/tui_test.go @@ -191,10 +191,11 @@ func TestCursorVerticalNav(t *testing.T) { // breaks on its own one-line change (drop the \+Enter branch; drop a CSI match). func TestEditorNewlineKeys(t *testing.T) { for _, c := range []struct{ name, keys string }{ - {"ctrl-j", "ab\ncd\r"}, // \n: any terminal - {"backslash-enter", "ab\\\rcd\r"}, // \ + Enter: universal fallback - {"kitty shift+enter", "ab\033[13;2ucd\r"}, // CSI 13;2u - {"extended shift+enter", "ab\033[27;2;13~cd\r"}, // CSI 27;2;13~ (tmux/xterm) + {"ctrl-j", "ab\ncd\r"}, // \n: any terminal + {"backslash-enter", "ab\\\rcd\r"}, // \ + Enter: universal fallback + {"kitty shift+enter", "ab\033[13;2ucd\r"}, // CSI 13;2u + {"extended shift+enter", "ab\033[27;2;13~cd\r"}, // CSI 27;2;13~ (tmux/xterm) + {"kitty functional shift+enter", "ab\033[57414;3ucd\r"}, // CSI 57414;3u (alt+enter, functional codepoint) } { if line, _ := driveKeys(t, nil, c.keys); line != "ab\ncd" { t.Fatalf("%s: line = %q, want %q", c.name, line, "ab\ncd") @@ -217,17 +218,42 @@ func TestEditorSecretSubmitsOnBackslashEnter(t *testing.T) { } } -// TestAttendTurnQueuesAndCancels: while a turn runs, the live editor queues a -// typed message on Enter (instead of submitting) and cancels the turn on a bare -// Escape. Breaker: drop the turn-mode Enter branch and "fix it" is never queued; -// drop the bare-Escape branch and cancel is never called. +// TestAttendTurnQueuesAndCancels: while a turn runs, the live editor cancels the +// turn on a bare Escape. Breaker: drop the bare-Escape branch and cancel is never +// called. func TestAttendTurnQueuesAndCancels(t *testing.T) { f, err := os.CreateTemp(t.TempDir(), "tui-out") if err != nil { t.Fatal(err) } defer f.Close() - tc := &tuiConsole{out: f, in: bufio.NewReader(strings.NewReader("fix it\r\x1b")), cols: 80} + tc := &tuiConsole{out: f, in: bufio.NewReader(strings.NewReader("\x1b")), cols: 80} + canceled := false + if err := tc.attendTurn(turnAttend{ + done: make(chan struct{}), // never closes; the script's EOF ends attend + cancel: func() { canceled = true }, + queue: func(string) {}, + }); err != errTurnOver { + t.Fatalf("attendTurn err = %v, want errTurnOver", err) + } + if !canceled { + t.Fatal("bare Escape must cancel the turn") + } +} + +// TestAttendTurnSteerCancelsImmediately: a message typed and submitted with Enter +// while a turn runs both queues the steer AND cancels the in-flight turn, so the +// steer is acted on now instead of waiting for the next iteration boundary. The +// script has no Escape: the cancel must come from the steer itself. Breaker: +// remove the turn.cancel() call in the turn-mode Enter branch and canceled stays +// false while the message is still queued. +func TestAttendTurnSteerCancelsImmediately(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "tui-out") + if err != nil { + t.Fatal(err) + } + defer f.Close() + tc := &tuiConsole{out: f, in: bufio.NewReader(strings.NewReader("fix it\r")), cols: 80} var queued []string canceled := false if err := tc.attendTurn(turnAttend{ @@ -241,7 +267,274 @@ func TestAttendTurnQueuesAndCancels(t *testing.T) { t.Fatalf("Enter must queue the message, got %v", queued) } if !canceled { - t.Fatal("bare Escape must cancel the turn") + t.Fatal("queuing a steer must also cancel the in-flight turn") + } +} + +// TestAttendTurnStopCommandCancelsWithoutQueuing: typing "/stop" and Enter while +// a turn runs aborts it like Escape and sends nothing to the model. Unlike an +// ordinary steer it must cancel WITHOUT queuing, so the next turn gets no input. +// The match is case-insensitive and exact. Breaker: route /stop through the steer +// path and it queues "/stop" (queued non-empty), or drop the cancel and canceled +// stays false. +func TestAttendTurnStopCommandCancelsWithoutQueuing(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "tui-out") + if err != nil { + t.Fatal(err) + } + defer f.Close() + tc := &tuiConsole{out: f, in: bufio.NewReader(strings.NewReader("/STOP\r")), cols: 80} + var queued []string + canceled := false + if err := tc.attendTurn(turnAttend{ + done: make(chan struct{}), // never closes; the script's EOF ends attend + cancel: func() { canceled = true }, + queue: func(s string) { queued = append(queued, s) }, + }); err != errTurnOver { + t.Fatalf("attendTurn err = %v, want errTurnOver", err) + } + if !canceled { + t.Fatal("/stop must cancel the in-flight turn") + } + if len(queued) != 0 { + t.Fatalf("/stop must not queue anything, got %v", queued) + } +} + +// TestEscIsBareKeysOffIntroducer: bare-Escape detection decides by the byte +// following Esc, not by timing, so it holds up under tmux/SSH latency. A lone +// Esc with the stream then closed is bare; an Esc followed by a CSI introducer +// ([) is a sequence (not bare); an Esc followed by a printable like 'x' is bare, +// because only [ and O introduce a sequence. The channel is pre-seeded so the +// result never depends on the wall clock. Breaker: revert escIsBare to treating +// any present byte as non-bare and the 'x' case flips to false. +func TestEscIsBareKeysOffIntroducer(t *testing.T) { + cases := []struct { + name string + next rune // the byte queued after Esc; 0 means "queue nothing, close" + want bool + }{ + {"lone esc then closed", 0, true}, + {"esc then CSI introducer", '[', false}, + {"esc then SS3 introducer", 'O', false}, + {"esc then printable", 'x', true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + tc := &tuiConsole{runes: make(chan rune, 1)} + if c.next != 0 { + tc.runes <- c.next + } + close(tc.runes) // the closed-stream branch must not misread a queued byte + if got := tc.escIsBare(); got != c.want { + t.Fatalf("escIsBare after Esc+%q = %v, want %v", c.next, got, c.want) + } + }) + } +} + +// attendKeys drives attendTurn headless over a scripted keystroke stream and +// reports whether the turn was canceled and what (if anything) was queued. The +// done channel never closes, so the script's trailing EOF is what ends attend: +// this lets a test feed a sequence and then observe state without racing a +// real turn boundary. +func attendKeys(t *testing.T, keys string) (canceled bool, queued []string) { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "tui-out") + if err != nil { + t.Fatal(err) + } + defer f.Close() + tc := &tuiConsole{out: f, in: bufio.NewReader(strings.NewReader(keys)), cols: 80} + if err := tc.attendTurn(turnAttend{ + done: make(chan struct{}), // never closes; the script's EOF ends attend + cancel: func() { canceled = true }, + queue: func(s string) { queued = append(queued, s) }, + }); err != errTurnOver { + t.Fatalf("attendTurn err = %v, want errTurnOver", err) + } + return canceled, queued +} + +// TestAttendTurnCSIUEscapeCancels: under extended-keys (CSI-u) mode the Escape +// KEY arrives as a sequence, not a bare 0x1b, so the bare path never sees it. +// The bare form CSI 27 u and the modified form CSI 27;2 u must both cancel the +// turn exactly like a bare Escape, and neither queues nor inserts anything. +// Breaker: remove the isCSIUEscape case in handleEscape and canceled stays false. +func TestAttendTurnCSIUEscapeCancels(t *testing.T) { + for _, c := range []struct{ name, keys string }{ + {"bare csi-u escape", "\033[27u"}, // CSI 27 u + {"modified csi-u escape", "\033[27;2u"}, // CSI 27;2 u (a modifier held) + } { + t.Run(c.name, func(t *testing.T) { + canceled, queued := attendKeys(t, c.keys) + if !canceled { + t.Fatal("CSI-u Escape must cancel the in-flight turn") + } + if len(queued) != 0 { + t.Fatalf("CSI-u Escape must not queue anything, got %v", queued) + } + }) + } +} + +// TestAttendTurnShiftEnterDoesNotCancel: extended Enter (CSI 13;2u) shares the +// final 'u' with the CSI-u Escape but carries codepoint 13, not 27, so it must +// insert a newline and leave the turn running. Feeding only the sequence (no +// submitting Enter) isolates it from the steer-cancel that a real Enter would +// trigger. Breaker: match the cancel on final=='u' alone and this wrongly cancels. +func TestAttendTurnShiftEnterDoesNotCancel(t *testing.T) { + canceled, queued := attendKeys(t, "ab\033[13;2ucd") + if canceled { + t.Fatal("Shift+Enter (codepoint 13) must not be treated as Escape") + } + if len(queued) != 0 { + t.Fatalf("Shift+Enter must not queue anything on its own, got %v", queued) + } +} + +// TestAttendTurnEditingKeysDoNotCancel: ordinary CSI editing sequences during a +// turn (up-arrow CSI A, delete CSI 3~) edit the buffer and never cancel, so the +// CSI-u Escape routing did not swallow the rest of the editing keys. Breaker: +// route every decoded sequence to cancel and these flip canceled to true. +func TestAttendTurnEditingKeysDoNotCancel(t *testing.T) { + for _, c := range []struct{ name, keys string }{ + {"up arrow", "\033[A"}, // CSI A: history / cursor up + {"delete", "x\033[3~"}, // CSI 3~: delete at cursor + } { + t.Run(c.name, func(t *testing.T) { + if canceled, _ := attendKeys(t, c.keys); canceled { + t.Fatalf("%s must not cancel the turn", c.name) + } + }) + } +} + +// TestIsCSIUCtrlC: only Ctrl-modified codepoint 99 ('c') under final 'u' is +// Ctrl-C. Plain 'c' (99 or 99;1), the Escape key (27), and extended Enter +// (13;2) must all be excluded so they keep their own meaning. Breaker: match on +// final=='u' alone and the escape/enter cases flip to true. +func TestIsCSIUCtrlC(t *testing.T) { + cases := []struct { + params string + final rune + want bool + }{ + {"99;5", 'u', true}, // Ctrl+c: codepoint 99, modifier 5 (Ctrl) + {"99;7", 'u', true}, // Ctrl+Alt+c: the Ctrl bit is still set + {"99", 'u', false}, // plain 'c', no modifier + {"99;1", 'u', false}, // explicit "no modifier" + {"27", 'u', false}, // the Escape key + {"13;2", 'u', false}, // Shift+Enter + {"99;5", '~', false}, // right codepoint+mod but not a CSI-u final + } + for _, c := range cases { + if got := isCSIUCtrlC(c.params, c.final); got != c.want { + t.Errorf("isCSIUCtrlC(%q, %q) = %v, want %v", c.params, c.final, got, c.want) + } + } +} + +// TestIsCSIUEnter: under final 'u', codepoint 13 (standard) and 57414 (kitty's +// functional Enter, 0xE006) are both Enter; a present modifier other than "1" +// marks it modified (Shift/Alt -> newline) while bare or "1" is unmodified +// (submit). Escape (27) and Ctrl-C (99) must be excluded. Breaker: drop the +// 57414 codepoint and the functional case flips to enter=false. +func TestIsCSIUEnter(t *testing.T) { + cases := []struct { + params string + final rune + enter, modded bool + }{ + {"13", 'u', true, false}, // standard Enter, unmodified + {"57414", 'u', true, false}, // kitty functional Enter, unmodified + {"13;2", 'u', true, true}, // Shift+Enter + {"57414;3", 'u', true, true}, // Alt+Enter (functional codepoint) + {"13;1", 'u', true, false}, // explicit "no modifier" + {"27", 'u', false, false}, // the Escape key + {"99;5", 'u', false, false}, // Ctrl-C + {"13", '~', false, false}, // right codepoint but not a CSI-u final + } + for _, c := range cases { + enter, modded := isCSIUEnter(c.params, c.final) + if enter != c.enter || modded != c.modded { + t.Errorf("isCSIUEnter(%q, %q) = (%v, %v), want (%v, %v)", + c.params, c.final, enter, modded, c.enter, c.modded) + } + } +} + +// TestReadLineCSIUEnterSubmits: under extended-keys mode an unmodified Enter +// arrives as a CSI-u sequence (kitty functional 57414), never a bare \r, so the +// editor must normalize it back to a Return and submit the typed line through +// the single submit path. Breaker: remove the unget('\r') for an unmodified +// CSI-u Enter and ReadLine never returns the line. +func TestReadLineCSIUEnterSubmits(t *testing.T) { + if line, _ := driveKeys(t, nil, "hi\033[57414u"); line != "hi" { + t.Fatalf("CSI-u Enter must submit: line = %q, want %q", line, "hi") + } +} + +// TestAttendTurnCSIUEnterSteers: during a turn an unmodified CSI-u Enter must +// take the same steer path a bare \r would (queue the text and cancel the +// in-flight turn), not insert a newline. Breaker: route an unmodified CSI-u +// Enter to the newline case and nothing is queued. +func TestAttendTurnCSIUEnterSteers(t *testing.T) { + canceled, queued := attendKeys(t, "fix it\033[57414u") + if len(queued) != 1 || queued[0] != "fix it" { + t.Fatalf("CSI-u Enter mid-turn must queue the steer, got %v", queued) + } + if !canceled { + t.Fatal("queuing a steer via CSI-u Enter must cancel the in-flight turn") + } +} + +// ctrlCKeys drives readLineMode headless over a scripted keystroke stream with a +// spy onCtrlC installed, reporting whether the hook fired and what the editor +// returned. The stream's trailing EOF ends the idle read (turn == nil), so the +// test can feed a Ctrl-C form and observe the hook without racing a real turn. +func ctrlCKeys(t *testing.T, keys string) (fired bool, line string) { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "tui-out") + if err != nil { + t.Fatal(err) + } + defer f.Close() + tc := &tuiConsole{ + out: f, + in: bufio.NewReader(strings.NewReader(keys)), + cols: 80, + onCtrlC: func() { fired = true }, + } + line, _ = tc.readLineMode("-> ", false, nil) + return fired, line +} + +// TestHandleEscapeCSIUCtrlCInvokesHook: under extended-keys mode Ctrl-C arrives +// as CSI 99;5 u (never an OS signal), so the editor must route it to onCtrlC and +// not type a 'c' or run any editing action. Breaker: drop the isCSIUCtrlC branch +// in handleEscape and the hook never fires. +func TestHandleEscapeCSIUCtrlCInvokesHook(t *testing.T) { + fired, line := ctrlCKeys(t, "\033[99;5u") + if !fired { + t.Fatal("CSI-u Ctrl-C must invoke onCtrlC") + } + if line != "" { + t.Fatalf("CSI-u Ctrl-C must not edit the buffer, got %q", line) + } +} + +// TestReadLineRawCtrlCInvokesHook: a raw 0x03 byte (terminals that send no +// signal) also routes to onCtrlC, without holding the editor mutex so the +// force-quit and warning Prints cannot deadlock. Breaker: drop the 0x03 case in +// readLineMode and the hook never fires. +func TestReadLineRawCtrlCInvokesHook(t *testing.T) { + fired, line := ctrlCKeys(t, "\x03") + if !fired { + t.Fatal("raw 0x03 must invoke onCtrlC") + } + if line != "" { + t.Fatalf("raw 0x03 must not produce input, got %q", line) } }