From b36fa42fd373651ad28b3fc8533238c774ebe4ff Mon Sep 17 00:00:00 2001 From: Chris Sebok Date: Thu, 27 Aug 2026 12:52:18 +0100 Subject: [PATCH 1/2] fix(terminal): let the daemon own what a program has negotiated about the keyboard (#290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terminal scrolling dies after a project switch and only a window resize brings it back. Underneath the symptom is a belief rather than a pixel. `use-terminal.ts` decides on every keydown: const programOwnsKeyboard = kittyKeyboardActive(kitty) || altBuffer; and while that is wrongly true on the normal buffer, Ctrl+Home and Ctrl+End stop being reserved for scrollback and plain PageUp/PageDown skip the `term.scrollPages(...)` branch that is gated on it. Two of the three routes in the report die together, from one boolean. THE BELIEF DID NOT GO STALE BY BEING MISSED. IT WAS COUNTED TWICE. Rebuilding a view restored the saved state from `keyboard-mode-store` and then replayed the daemon's scrollback tail — which still contains the very sequences that produced that state, verbatim, since `appendScrollback` preserves control bytes. The kitty protocol is a STACK: `CSI > flags u` pushes and `CSI < n u` pops. Two pushes against one pop leaves it enabled, so when the program finally turned the protocol off, its pop only cancelled the duplicate. That also accounts for the recovery behaviour the report describes and nothing else explained. A repaint cannot help, because the belief is not in the pixels. A resize helps every time because — as the reset comment in this same file already records — programs re-assert their setup after one, and a mounted view parses the re-negotiation live. THE FIX IS ONE AUTHORITY INSTEAD OF TWO DERIVATIONS - core gains `scanKeyboardNegotiation`, which reads the negotiation out of the raw output stream. It is the sibling of `trackAltScreen` and exists for the same reason: the daemon sees every byte whether or not a view is mounted, and a panel in a background tab is unmounted. Unlike `trackAltScreen` it carries an unterminated trailing sequence across chunk boundaries, because a lost `1049` self-corrects on the next screen switch while a lost pop is permanent. - the daemon tracks it per session and returns it on attach, beside `altScreen`. - the view ADOPTS that answer instead of reconstructing it, and mutes the negotiation handlers for the span of the replayed tail so the replay cannot re-apply what the daemon already accounted for. The replay is paint; the daemon is truth. `keyboard-mode-store` keeps a narrower job — the value a view starts from before the attach resolves — and its comment now says so rather than claiming to be the authority. BOTH HALVES ARE LOAD-BEARING, MEASURED Each was disabled in turn, rebuilt, and the regression test re-run: adoption of res.keyboard disabled RED replay suppression disabled RED The first pass of that A/B is worth recording, because it found a hole in the test rather than in the code: with suppression disabled the test still PASSED, since a tail holding both a push and its pop replays balanced and the flag lands right by luck. What it gets wrong is the stack DEPTH, and nothing observable goes wrong until the program pops again — one program exit later than anyone would think to look. The test grew a second phase that negotiates, rebuilds, and then pops live; that phase is what pins the suppression. No user-visible behaviour changes except the defect: a terminal whose program has finished with the keyboard now gives its scrollback keys back. --- packages/core/src/index.ts | 3 + packages/core/src/terminal/index.ts | 5 + .../core/src/terminal/negotiation-scan.ts | 129 ++++++++ .../core/tests/unit/negotiation-scan.test.ts | 146 +++++++++ packages/daemon/src/terminal-service.ts | 18 ++ packages/ipc-contract/src/terminal.ts | 17 ++ packages/ui/src/renderer/global.d.ts | 6 + .../renderer/terminal/keyboard-mode-store.ts | 15 + .../ui/src/renderer/terminal/use-terminal.ts | 51 +++- packages/ui/tests/e2e/e2e-budget.json | 4 +- packages/ui/tests/e2e/parallel-plan.json | 1 + .../e2e/terminal-keyboard-negotiation.e2e.ts | 282 ++++++++++++++++++ 12 files changed, 672 insertions(+), 5 deletions(-) create mode 100644 packages/core/src/terminal/negotiation-scan.ts create mode 100644 packages/core/tests/unit/negotiation-scan.test.ts create mode 100644 packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8fa3a8b3..16ae67a1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -640,6 +640,9 @@ export { type KeyChord, appendScrollback, trackAltScreen, + createNegotiationScan, + scanKeyboardNegotiation, + type NegotiationScan, MOUSE_REPORTING_MODES, createMouseReportingState, decideWheel, diff --git a/packages/core/src/terminal/index.ts b/packages/core/src/terminal/index.ts index 712e0eea..3305bd8d 100644 --- a/packages/core/src/terminal/index.ts +++ b/packages/core/src/terminal/index.ts @@ -101,6 +101,11 @@ export { type WheelRoute, } from './wheel-decision.js'; export { trackAltScreen } from './alt-screen.js'; +export { + createNegotiationScan, + scanKeyboardNegotiation, + type NegotiationScan, +} from './negotiation-scan.js'; export { encodeModifiedKey, kittyReportsAllKeys, KITTY_REPORT_ALL_KEYS } from './kitty-keyboard.js'; export { terminalReloadAction, diff --git a/packages/core/src/terminal/negotiation-scan.ts b/packages/core/src/terminal/negotiation-scan.ts new file mode 100644 index 00000000..23a4331d --- /dev/null +++ b/packages/core/src/terminal/negotiation-scan.ts @@ -0,0 +1,129 @@ +import { + applyDecPrivateMode, + applyKittyCsi, + createKittyKeyboardState, + type KittyCsiPrefix, + type KittyKeyboardState, +} from './kitty-keyboard.js'; + +/** + * Track a terminal program's KEYBOARD NEGOTIATION from the raw output stream (#290). + * + * The renderer has always derived this by parsing the program's output through xterm's CSI handlers, + * which works exactly as long as a view exists. It does not while a panel is unmounted — a + * background tab, or a project being switched away from — and the program has no reason to wait: it + * negotiates when it starts and un-negotiates when it exits, whether or not anyone is watching. + * + * The daemon sees every byte regardless, which is the property this needs. It already tracks the + * ALTERNATE SCREEN from the same stream for the same reason ({@link trackAltScreen}); this is the + * other half of what a rebuilt view has to be told rather than left to infer. + * + * ══ WHY INFERRING IT WAS WORSE THAN MISSING IT ══ + * + * The failure this fixes is not a missed negotiation. It is a DOUBLE-COUNTED one. Rebuilding a view + * restored the saved state from the renderer's panel store and then replayed the daemon's scrollback + * tail — which still contains the very sequences that produced that state, verbatim, because + * `appendScrollback` preserves control bytes. The kitty protocol is a stack: `CSI > flags u` pushes + * and `CSI < n u` pops. Two pushes against one pop leaves it enabled, so when the program finally + * turned the protocol off its pop only cancelled the duplicate, and throng went on believing a + * program wanted enhanced key reporting after it had said otherwise. + * + * Downstream that boolean is `programOwnsKeyboard`, and while it is wrongly true on the normal + * buffer the scrollback chords are handed to the program: Ctrl+Home and Ctrl+End stop being + * reserved, and plain PageUp/PageDown skip the viewport branch that is gated on it. That is #290's + * "every scroll route dies at once", from one boolean. + * + * So the answer is a single authority rather than two derivations. The daemon tracks it here, hands + * it over on attach, and the view adopts it instead of reconstructing it. + * + * ══ SEQUENCES SPLIT ACROSS CHUNKS ══ + * + * A PTY chunk boundary can fall inside an escape sequence, and a `CSI < u` that arrives as `CSI <` + * then `u` would simply not be seen — which would silently re-create the very defect this exists to + * fix, in a form nothing would ever reproduce. So an incomplete trailing sequence is carried to the + * next call rather than dropped. {@link trackAltScreen} does not do this and is not being changed + * here: a lost `1049` self-corrects on the program's next screen switch, whereas a lost pop is + * permanent until the program re-negotiates. + */ + +/** How much unterminated trailing escape text is worth carrying. A CSI parameter list is short; */ +/** anything longer is malformed and holding it would grow without bound. */ +const MAX_PENDING = 64; + +/** ESC, and the CSI introducer that follows it. */ +const ESC = '\x1b'; + +/** + * One CSI sequence: the introducer, an optional private-marker prefix, numeric parameters, and the + * final byte. Only `u` (kitty) and `h`/`l` (DEC private modes) are acted on; every other final is + * matched so that the scan steps over it rather than mistaking its bytes for a later sequence. + */ +// This matches a CONTROL sequence, so it necessarily contains a control character — the same +// exemption `alt-screen.ts` takes for the same reason. +// eslint-disable-next-line no-control-regex +const CSI = /\u001b\[([?=><])?([0-9;]*)([A-Za-z])/g; + +/** The kitty CSI-u private markers throng honours — the same set the renderer registers. */ +const KITTY_PREFIXES = new Set(['?', '=', '>', '<']); + +export interface NegotiationScan { + /** What the program has negotiated, after this chunk. */ + readonly state: KittyKeyboardState; + /** An unterminated trailing escape sequence, to be prepended to the next chunk. */ + readonly pending: string; +} + +/** A fresh scan: nothing negotiated, nothing half-read. */ +export function createNegotiationScan(): NegotiationScan { + return { state: createKittyKeyboardState(), pending: '' }; +} + +/** `"1;2"` → `[1, 2]`; `""` → `[]`. Empty positions read as 0, as a terminal's parser does. */ +function params(raw: string): number[] { + if (raw === '') return []; + return raw.split(';').map((p) => (p === '' ? 0 : Number.parseInt(p, 10))); +} + +/** + * Apply every complete negotiation sequence in `chunk`, in the order the program emitted them. + * + * Order is load-bearing and is why this is a scan rather than a last-match-wins search like + * {@link trackAltScreen}: pushes and pops only mean anything in sequence. + */ +export function scanKeyboardNegotiation(previous: NegotiationScan, chunk: string): NegotiationScan { + const text = previous.pending + chunk; + let state = previous.state; + let consumedTo = 0; + + CSI.lastIndex = 0; + for (let m = CSI.exec(text); m !== null; m = CSI.exec(text)) { + consumedTo = m.index + m[0].length; + const prefix = m[1] ?? ''; + const final = m[3] ?? ''; + if (final === 'u') { + if (!KITTY_PREFIXES.has(prefix)) continue; // a bare `CSI u` is not ours + // The `?` query's REPLY belongs to the view, which answers the program itself. Here the + // sequence is only evidence about state, and `applyKittyCsi` leaves state untouched for it. + state = applyKittyCsi(state, prefix as KittyCsiPrefix, params(m[2] ?? '')).state; + } else if ((final === 'h' || final === 'l') && prefix === '?') { + state = applyDecPrivateMode(state, params(m[2] ?? ''), final === 'h'); + } + } + + return { state, pending: trailingPartial(text, consumedTo) }; +} + +/** + * The unterminated escape sequence at the end, if any. + * + * Searching from the last COMPLETE sequence rather than the end of the string: an ESC that has + * already been consumed as part of a match is not pending, and re-finding it would replay it. + */ +function trailingPartial(text: string, consumedTo: number): string { + const esc = text.lastIndexOf(ESC); + if (esc < consumedTo) return ''; + const tail = text.slice(esc); + // A complete sequence would have matched above, so anything here is genuinely partial — unless it + // is too long to be a CSI parameter list, in which case it is malformed and not worth carrying. + return tail.length > MAX_PENDING ? '' : tail; +} diff --git a/packages/core/tests/unit/negotiation-scan.test.ts b/packages/core/tests/unit/negotiation-scan.test.ts new file mode 100644 index 00000000..7f3010bf --- /dev/null +++ b/packages/core/tests/unit/negotiation-scan.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from 'vitest'; +import { + createNegotiationScan, + scanKeyboardNegotiation, +} from '../../src/terminal/negotiation-scan.js'; +import { + kittyKeyboardActive, + win32InputActive, + applicationReadingInput, +} from '../../src/terminal/kitty-keyboard.js'; + +/** + * #290 — reading a program's keyboard negotiation out of the raw output stream. + * + * The daemon needs this because the renderer cannot always be listening: a panel in a background + * tab is unmounted, and a program that un-negotiates while nobody is watching is simply not heard. + * The daemon reads every byte regardless, so it holds the answer and hands it over on attach. + * + * The tests that matter most here are the two the defect was actually made of — order sensitivity, + * and what happens when the same bytes are applied twice. + */ + +const PUSH = '\x1b[>1u'; // CSI > 1 u — push the disambiguate flag +const POP = '\x1b[ + chunks.reduce((scan, chunk) => scanKeyboardNegotiation(scan, chunk), createNegotiationScan()); + +describe('scanKeyboardNegotiation', () => { + it('sees a program enable enhanced key reporting', () => { + expect(kittyKeyboardActive(feed(PUSH).state)).toBe(true); + }); + + it('sees it turned back off again', () => { + expect(kittyKeyboardActive(feed(PUSH, POP).state)).toBe(false); + }); + + it('applies pushes and pops in the order they were emitted, not by last-match-wins', () => { + // Both chunks contain both sequences; only the ORDER differs, and it decides the answer. + expect(kittyKeyboardActive(feed(`${PUSH}${POP}`).state)).toBe(false); + expect(kittyKeyboardActive(feed(`${POP}${PUSH}`).state)).toBe(true); + }); + + it('sees a sequence SPLIT across two chunks — a PTY boundary must not swallow a pop', () => { + /* + * The failure this guards is silent and permanent. A missed `1049` self-corrects the next time + * the program switches screens; a missed POP leaves the protocol enabled until the program + * negotiates again, which for a program that has just exited is never. + */ + const armed = feed(PUSH); + expect(kittyKeyboardActive(armed.state)).toBe(true); + + const half = scanKeyboardNegotiation(armed, '\x1b[<'); + expect(half.pending, 'the incomplete sequence must be carried, not dropped').toBe('\x1b[<'); + expect(kittyKeyboardActive(half.state), 'a half-read sequence must not be acted on yet').toBe( + true, + ); + + expect(kittyKeyboardActive(scanKeyboardNegotiation(half, 'u').state)).toBe(false); + }); + + it('splits at every offset of a push/pop pair and still ends up off', () => { + const stream = `${PUSH}${POP}`; + for (let cut = 0; cut <= stream.length; cut++) { + const scan = feed(stream.slice(0, cut), stream.slice(cut)); + expect(kittyKeyboardActive(scan.state), `split after ${String(cut)} byte(s)`).toBe(false); + } + }); + + it('carries nothing when a chunk ends on a complete sequence', () => { + expect(feed(PUSH).pending).toBe(''); + }); + + it('drops an unterminated run too long to be a CSI parameter list', () => { + // Otherwise a stream containing a bare ESC would accumulate forever. + expect(feed(`\x1b[${'9'.repeat(200)}`).pending).toBe(''); + }); + + it('tracks win32-input-mode and bracketed paste, which travel with the same state', () => { + expect(win32InputActive(feed(WIN32_ON).state)).toBe(true); + expect(win32InputActive(feed(WIN32_ON, WIN32_OFF).state)).toBe(false); + expect(applicationReadingInput(feed(PASTE_ON).state)).toBe(true); + }); + + it('ignores sequences that are not a keyboard negotiation', () => { + // Alt-screen, a clear, a colour — none of these say anything about the keyboard, and stepping + // over them wrongly is how a scan starts inventing state. + const scan = feed(`\x1b[?1049h${PUSH}\x1b[2J\x1b[31m`); + expect(kittyKeyboardActive(scan.state)).toBe(true); + expect(win32InputActive(scan.state)).toBe(false); + }); + + it('requires the ESC — the same bytes as plain text negotiate nothing', () => { + /* + * The introducer is a control character, so it can only be written as an escape in source, and + * an editing accident that drops it leaves a regex which still compiles and still matches — just + * against ordinary output. A program printing the literal text `[>1u` would then silently switch + * enhanced key reporting on for the whole session. + */ + expect(kittyKeyboardActive(feed('[>1u').state)).toBe(false); + expect(win32InputActive(feed('[?9001h').state)).toBe(false); + }); + + it('treats a `CSI ? u` support query as evidence of nothing', () => { + // The program is ASKING. Only the view answers it, and the answer is not a negotiation. + expect(kittyKeyboardActive(feed('\x1b[?u').state)).toBe(false); + expect(kittyKeyboardActive(feed(PUSH, '\x1b[?u').state)).toBe(true); + }); + + it('THE DEFECT: applying one tail twice leaves the protocol stuck on (#290)', () => { + /* + * This is the whole reason the daemon is now the authority, expressed at the layer where it can + * be seen. A rebuilt view used to start from the state its panel store had saved AND then parse + * the replayed scrollback tail — which still contains the sequences that produced that state. + * + * The protocol is a stack, so the second push is not a no-op: it buries the first. The program's + * single pop then only cancels the duplicate, and enhanced key reporting stays on after the + * program has said it wants no such thing. Downstream that is `programOwnsKeyboard` stuck true, + * the scrollback chords surrendered, and Ctrl+Home no longer scrolling. + */ + // What the renderer's panel store held: the push was seen live, the pop never was, because by + // then the panel had been unmounted and no view existed to parse it. + const storeHeld = feed(PUSH); + expect(kittyKeyboardActive(storeHeld.state), 'the store is mid-sequence, by construction').toBe( + true, + ); + + // The daemon's tail, which spans the whole program: both the push AND the pop it never delivered. + const tail = `${PUSH}${POP}`; + + // Read on its own, the tail says exactly the right thing — which is why this was so easy to miss. + expect(kittyKeyboardActive(feed(tail).state), 'the tail alone is honest').toBe(false); + + // Applied ON TOP of the restored state, it is not. The replayed push buries the restored one, so + // the program's single pop cancels only the duplicate and the protocol survives its own exit. + const restoredThenReplayed = scanKeyboardNegotiation(storeHeld, tail); + expect( + kittyKeyboardActive(restoredThenReplayed.state), + 'restore + replay leaves enhanced key reporting on after the program turned it off (#290)', + ).toBe(true); + }); +}); diff --git a/packages/daemon/src/terminal-service.ts b/packages/daemon/src/terminal-service.ts index 944f0d34..987ece0e 100644 --- a/packages/daemon/src/terminal-service.ts +++ b/packages/daemon/src/terminal-service.ts @@ -11,6 +11,9 @@ import { type PtyHandle, appendScrollback, trackAltScreen, + createNegotiationScan, + scanKeyboardNegotiation, + type NegotiationScan, classifyFailure, type FailureCause, } from '@throng/core'; @@ -117,6 +120,17 @@ interface Session { * for nothing before the program's own redraw overwrites it. */ altScreen: boolean; + /** + * What the program has negotiated about the KEYBOARD (#290) — kitty CSI-u flags and the DEC + * private modes, plus any half-read escape sequence carried across a chunk boundary. + * + * Tracked here for the same reason as `altScreen` directly above: the daemon reads every byte + * whether or not a view exists, and a panel in a background tab is unmounted, so a program that + * un-negotiates while nobody is looking would otherwise never be heard. A rebuilt view used to + * work this out for itself from the store PLUS the replayed tail, which double-counted every + * push and left the protocol stuck on. See `negotiation-scan.ts`. + */ + negotiation: NegotiationScan; /** * The grid is stale because every view has gone (028 follow-up). The next attach MUST push a real * resize even when the recomputed grid equals the stored one, because the program needs a window @@ -437,6 +451,7 @@ export class TerminalService { grid: existing.grid, redrawn, altScreen: existing.altScreen, + keyboard: existing.negotiation.state, }; } this.terminate(existing); @@ -521,6 +536,7 @@ export class TerminalService { grid: { cols: startCols, rows: startRows }, scrollback: '', altScreen: false, + negotiation: createNegotiationScan(), gridStale: false, status: 'running', userKilled: false, @@ -543,6 +559,8 @@ export class TerminalService { host.onData(handle, (chunk) => { session.scrollback = appendScrollback(session.scrollback, chunk, MAX_SCROLLBACK); session.altScreen = trackAltScreen(session.altScreen, chunk); + // #290 — the other half a rebuilt view must be TOLD rather than left to infer. + session.negotiation = scanKeyboardNegotiation(session.negotiation, chunk); this.events.publishOutput(panelId, chunk); if (startupCommandPending) { startupCommandPending = false; diff --git a/packages/ipc-contract/src/terminal.ts b/packages/ipc-contract/src/terminal.ts index ca6823ee..e50592eb 100644 --- a/packages/ipc-contract/src/terminal.ts +++ b/packages/ipc-contract/src/terminal.ts @@ -2,6 +2,8 @@ // contracts/terminal-rpc.md). Commands are request/response; output/exit are // JSON-RPC *notifications* (no id) pushed over a long-lived subscribed socket. +import type { KittyKeyboardState } from '@throng/core'; + // --- Command methods (request → response) --- export const TERMINAL_ATTACH_METHOD = 'terminal.attach'; export const TERMINAL_WRITE_METHOD = 'terminal.write'; @@ -187,6 +189,21 @@ export interface TerminalAttachResult { * buffer and reclaims keys the program owns: measured as Ctrl+End dying after a tab switch. */ altScreen?: boolean; + /** + * What the program has negotiated about the KEYBOARD (#290) — the same fact as `altScreen` above, + * for the other half of what a rebuilt view must be told rather than left to work out. + * + * The view used to reconstruct this from two sources at once: the renderer's own panel store, and + * the replayed scrollback tail, which still contains the sequences that produced what the store + * holds. Applying both counts every `CSI > flags u` push twice, and since the protocol is a stack + * the program's matching pop then only cancels the duplicate — leaving throng convinced a program + * wants enhanced key reporting after it has said it does not. `programOwnsKeyboard` sticks true, + * the scrollback chords are surrendered, and Ctrl+Home / PageUp stop scrolling. + * + * The daemon reads every byte whether or not a view exists, so it is the one place that can hold + * this honestly. Absent only when there is no running session to ask. + */ + keyboard?: KittyKeyboardState; exit?: { code: number | null; signal?: string }; } diff --git a/packages/ui/src/renderer/global.d.ts b/packages/ui/src/renderer/global.d.ts index 80b89ab5..9a5c110b 100644 --- a/packages/ui/src/renderer/global.d.ts +++ b/packages/ui/src/renderer/global.d.ts @@ -496,6 +496,12 @@ export type TerminalAttachEnvelope = /** The program owns the alternate screen (028 follow-up) — the view must match, or it * reclaims keys the program owns. */ altScreen?: boolean; + /** + * What the program has negotiated about the keyboard (#290) — the companion to `altScreen` + * above. A rebuilt view adopts this instead of re-deriving it from the replayed tail, which + * still contains the sequences that produced it and would count every push a second time. + */ + keyboard?: import('@throng/core').KittyKeyboardState; exit?: { code: number | null }; /** A remembered directory that no longer exists; the terminal started at the root (FR-005b). */ cwdFallback?: string; diff --git a/packages/ui/src/renderer/terminal/keyboard-mode-store.ts b/packages/ui/src/renderer/terminal/keyboard-mode-store.ts index e8b2bb5e..0bbc42a1 100644 --- a/packages/ui/src/renderer/terminal/keyboard-mode-store.ts +++ b/packages/ui/src/renderer/terminal/keyboard-mode-store.ts @@ -17,6 +17,21 @@ import type { KittyKeyboardState } from '@throng/core'; * is precisely the case that matters, because full-screen programs are the ones that negotiate. * * The state belongs to the session, not to a view of it. This is the smallest thing that says so. + * + * ══ IT IS NO LONGER THE AUTHORITY, AND THAT MATTERS TO ANYONE DEBUGGING FROM HERE (#290) ══ + * + * Being per-panel rather than per-view fixed the case above and left a worse one. A rebuilt view + * restored what this map held AND re-parsed the daemon's replayed scrollback tail, which still + * contains the sequences that produced it — so every `CSI > flags u` was counted twice, and since + * the protocol is a stack, the program's matching pop then cancelled only the duplicate. Enhanced + * key reporting stayed on after the program had turned it off, `programOwnsKeyboard` stuck true, and + * the terminal's scrollback chords were handed to a program that no longer wanted them. + * + * The daemon now tracks the negotiation from the output stream — it is the only thing that sees + * every byte, including while a panel is unmounted — and returns it in the attach response, which + * the view ADOPTS (`use-terminal.ts`). What this map does now is narrower and still worth having: + * it is the value a view starts from BEFORE the attach resolves, and where the live parse keeps its + * answer between rebuilds. If it disagrees with the daemon, the daemon wins. */ const modes = new Map(); diff --git a/packages/ui/src/renderer/terminal/use-terminal.ts b/packages/ui/src/renderer/terminal/use-terminal.ts index 2e06b08d..ba3bbc6f 100644 --- a/packages/ui/src/renderer/terminal/use-terminal.ts +++ b/packages/ui/src/renderer/terminal/use-terminal.ts @@ -498,6 +498,19 @@ export function useTerminal(opts: UseTerminalOptions): void { * away from yet. */ let kitty = peekKeyboardMode(panelId) ?? createKittyKeyboardState(); + /* + * Is a REPLAYED scrollback tail being parsed right now (#290)? + * + * The tail is raw bytes and still contains every negotiation sequence the program ever emitted. + * Parsing them again applies them a SECOND time on top of the state this view starts from — and + * because the kitty protocol is a stack, two pushes against the program's one pop leave it + * enabled for good. So while the tail is being written the negotiation handlers below observe + * without mutating: the replay is paint, and the daemon's snapshot is the truth. + * + * Live output is not affected. The flag is cleared in the tail write's own callback, and xterm + * parses writes in order, so anything that arrives after the replay is parsed normally. + */ + let replayingTail = false; /** Keep the panel's copy in step whenever the program changes what it wants. */ const rememberKitty = (): void => saveKeyboardMode(panelId, kitty); // 028 (#187): which DEC mouse-reporting modes the program has enabled. Tracked at the same @@ -676,6 +689,9 @@ export function useTerminal(opts: UseTerminalOptions): void { const flatten = (params: (number | number[])[]): number[] => params.map((p) => (Array.isArray(p) ? (p[0] ?? 0) : p)); const onKittyCsi = (prefix: KittyCsiPrefix) => (params: (number | number[])[]): boolean => { + // Replayed tail: already accounted for in what the daemon handed us, and answering a `?` + // query from it would reply to a handshake the program completed long ago (#290). + if (replayingTail) return true; const { state, reply } = applyKittyCsi(kitty, prefix, flatten(params)); kitty = state; rememberKitty(); @@ -737,8 +753,16 @@ export function useTerminal(opts: UseTerminalOptions): void { * Who is READING is a different question, and the encoders answer it themselves from * bracketed paste. Tracking stays faithful; the decisions stay informed. */ - kitty = applyDecPrivateMode(kitty, modes, enable); - rememberKitty(); + // #290 — same rule as the kitty handler: a replayed tail must not re-negotiate. The + // daemon tracks win32-input and bracketed paste too, so the state it handed over already + // reflects these bytes. + if (!replayingTail) { + kitty = applyDecPrivateMode(kitty, modes, enable); + rememberKitty(); + } + // Mouse reporting is NOT suppressed: the daemon does not track it, so the replayed tail is + // this view's only source for it, and unlike the kitty stack these modes are idempotent + // flags that re-applying cannot corrupt. mouseReporting.apply(modes, enable); // 028 (issue 187) — same snoop, second question return false; // observe only — never claim the sequence }; @@ -1080,7 +1104,28 @@ export function useTerminal(opts: UseTerminalOptions): void { // overwritten anyway. Recorded so a test can assert on it rather than on flicker. (window as unknown as { __throngLastReplayBytes?: number }).__throngLastReplayBytes = res.scrollback?.length ?? 0; - if (res.scrollback) term.write(res.scrollback); + /* + * ADOPT what the program has negotiated, rather than working it out again (#290). + * + * This view may never have seen the negotiation — a panel in a background tab is unmounted, + * and a program that turns enhanced key reporting off while nobody is watching was simply + * not heard. The daemon reads every byte regardless, so it is asked instead. + * + * Before the replay, deliberately. The tail still contains the sequences that produced this + * very state, so letting it re-apply them on top would count each push twice and leave the + * protocol stuck on — which is the defect. `replayingTail` mutes the handlers for exactly + * the span of the tail. + */ + if (res.keyboard) { + kitty = res.keyboard; + rememberKitty(); + } + if (res.scrollback) { + replayingTail = true; + term.write(res.scrollback, () => { + replayingTail = false; + }); + } // Scrollback is applied — open the gate and flush any live output that // arrived during the attach window, in order, after the backlog. for (const chunk of gate.release()) writeChunk(chunk); diff --git a/packages/ui/tests/e2e/e2e-budget.json b/packages/ui/tests/e2e/e2e-budget.json index cf7e33e5..a0b68593 100644 --- a/packages/ui/tests/e2e/e2e-budget.json +++ b/packages/ui/tests/e2e/e2e-budget.json @@ -2,14 +2,14 @@ "note": "The declared ceiling on the E2E suite (034 FR-060, constitution v5.0.0 Principle V). A RATCHET: these numbers may only go DOWN. The build fails when a count exceeds its budget, AND when a count falls below it - because a ratchet that is never tightened is not a ratchet, it is a ceiling nobody is holding. Lower the number in the same commit that removes the tests. Raising one is a deliberate act that a reviewer will see in the diff, which is the whole point: the rule this replaces failed because growth was invisible. Before raising it, answer in one sentence what the new test can assert that a unit, component or integration test cannot.", "measuredFrom": "spec 034, after tagging; the migration batches lower these as they land", "countingBasis": "DECLARATIONS, not executed tests. Playwright reports a larger number for the same suite, and neither is wrong: four spec files declare tests inside a module-level loop over shell flavours, so one declaration becomes several executed tests. Declarations are what a budget should count - it is a limit on what people WRITE, and a table-driven test that gains a fifth flavour has not made the suite harder to maintain. This total also includes the @admin and @quarantine tests that grepInvert removes from a default run.", - "total": 558, + "total": 559, "core": 38, "byCategory": { "@editor": 117, "@explorer": 39, "@failure": 21, "@prefs": 88, - "@terminal": 102, + "@terminal": 103, "@window": 191 } } diff --git a/packages/ui/tests/e2e/parallel-plan.json b/packages/ui/tests/e2e/parallel-plan.json index f98d1cba..1d659e32 100644 --- a/packages/ui/tests/e2e/parallel-plan.json +++ b/packages/ui/tests/e2e/parallel-plan.json @@ -100,6 +100,7 @@ "terminal-editing-matrix.e2e.ts": "CPU", "terminal-env-freshness.e2e.ts": "CPU", "terminal-find.e2e.ts": "CPU", + "terminal-keyboard-negotiation.e2e.ts": "CPU", "terminal-mirror-survival.e2e.ts": "FOCUS", "terminal-mirror.e2e.ts": "FOCUS", "terminal-no-orphans.e2e.ts": "CPU", diff --git a/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts b/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts new file mode 100644 index 00000000..97366516 --- /dev/null +++ b/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts @@ -0,0 +1,282 @@ +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { basename, join } from 'node:path'; +import { test, expect, type Page } from '@playwright/test'; +import { + openApp, + createProject as newProject, + firstPanelId, + cleanupTemp, + type OpenApp, + TERMINAL_OUTPUT_TIMEOUT_MS, +} from './harness.js'; + +/* + * #290 — what a rebuilt terminal view believes about the KEYBOARD NEGOTIATION. + * + * The reported defect is that terminal scrolling dies after a project switch and only a window + * resize brings it back. Underneath it is a belief, not a pixel: `use-terminal.ts` decides on every + * keydown whether the running program owns the keyboard — + * + * const programOwnsKeyboard = kittyKeyboardActive(kitty) || altBuffer; + * + * — and when that is wrongly true on the normal buffer, Ctrl+Home/Ctrl+End are no longer reserved + * for scrollback (`use-terminal.ts`, the reserveKey call) and plain PageUp/PageDown skip the + * `term.scrollPages(...)` branch, which is gated on `!programOwnsKeyboard`. Two of the three routes + * in the report die together, from one stale boolean. + * + * ══ WHY THE BELIEF GOES STALE, WHICH IS NOT WHAT ANYONE FIRST THOUGHT ══ + * + * Not because the negotiation is MISSED while the panel is unmounted. Because on remount it is + * applied TWICE. Rebuilding a view restores the saved state from `keyboard-mode-store`, and then + * replays the daemon's scrollback tail — which still contains the very sequences that produced that + * state, as raw bytes (`appendScrollback` preserves control sequences verbatim). The kitty protocol + * is a STACK: `CSI > flags u` pushes, `CSI < n u` pops. Two pushes and one pop leaves it enabled, so + * when the program later turns the protocol off, its pop only cancels the duplicate. + * + * ══ WHY THIS IS AN E2E AND NOT SOMETHING CHEAPER ══ + * + * The double-count needs all three of: a real daemon holding a scrollback tail, a view genuinely + * torn down and rebuilt, and a program emitting negotiation while no view exists to parse it. A + * component test has no daemon and no replay, so it cannot produce the second application of the + * sequence — it would pass with the defect present, which is the worst thing a cheaper layer can do. + * The tracker itself is unit-tested in core; this is the wiring, and only the app has it. + * + * The fixture is a two-line Node program written to disk rather than typed as a shell one-liner, + * because cmd.exe quoting mangles escape sequences and a test that silently negotiates nothing would + * pass for the wrong reason. + */ + +test.describe.configure({ mode: 'serial' }); + +let shared: OpenApp; +test.beforeAll(async () => { + shared = await openApp(); +}); +test.afterAll(async () => { + await shared?.close(); +}); + +let projectSeq = 0; +const createProject = (win: Page, name: string, root: string): Promise => + newProject(win, `${name}-${(projectSeq += 1)}`, root); + +/** The inputs to the `programOwnsKeyboard` decision, as they were at the last keypress. */ +type KeyDecision = { + chord: string; + reserved: boolean; + kitty: boolean; + altBuffer: boolean; + programOwnsKeyboard: boolean; +}; + +async function lastKeyDecision(win: Page, pid: string): Promise { + const decision = await win.evaluate((panelId) => { + const snap = ( + window as unknown as { + __throngTerminalDiagnostics?: () => Record; + } + ).__throngTerminalDiagnostics?.(); + const keys = snap?.[panelId]?.keys ?? []; + return (keys[keys.length - 1] ?? null) as KeyDecision | null; + }, pid); + if (decision === null) { + throw new Error( + `no key decision recorded for panel ${pid} — the diagnostics ring is empty, so the ` + + 'assertions below would be about a missing record rather than about the product', + ); + } + return decision; +} + +/** `CSI > 1 u` — push the disambiguate flag, i.e. "this program wants enhanced key reporting". */ +const PUSH = "process.stdout.write('\\x1b[>1u');"; +/** `CSI < u` — pop it back off, i.e. "I am done; restore what you had". */ +const POP = "process.stdout.write('\\x1b[ {', + ` ${POP}`, + " process.stdout.write('KITTY_POPPED\\r\\n');", + " const line = 'x'.repeat(120) + '\\r\\n';", + ' for (let i = 0; i < 5; i++) process.stdout.write(line);', + " process.stdout.write('FILLER_DONE\\r\\n');", + '}, 4000);', + ].join('\n'), + 'utf8', + ); +} + +async function startTerminal(win: Page, root: string): Promise { + const pid = await firstPanelId(win); + await win.getByTestId(`panel-type-select-${pid}`).selectOption('terminal'); + await win.getByTestId('terminal-flavour').selectOption('cmd'); + await win.getByTestId(`panel-type-confirm-${pid}`).click(); + const term = win.getByTestId(`terminal-${pid}`); + await expect(term).toBeVisible(); + await expect(term).toContainText(basename(root), { timeout: TERMINAL_OUTPUT_TIMEOUT_MS }); + return pid; +} + +async function runCommand(win: Page, pid: string, cmd: string, marker: string): Promise { + await win.getByTestId(`terminal-${pid}`).click(); + await win.keyboard.type(cmd, { delay: 10 }); + await win.keyboard.press('Enter'); + await expect(win.getByTestId(`terminal-${pid}`)).toContainText(marker, { + timeout: TERMINAL_OUTPUT_TIMEOUT_MS, + }); +} + +// One line, deliberately: `e2e-budget.test.ts` and `e2e-tags.test.ts` match the declaration with a +// LINE-based regex, so a signature wrapped across lines is counted in the total and then missed by +// every category — which reads as a budget that is somehow both over and under at once. +test('a program that drops its keyboard negotiation while its panel is unmounted does not leave the belief behind (#290)', { tag: ['@extended', '@terminal', '@reserve:pty'] }, async () => { + const root = mkdtempSync(join(tmpdir(), 'throng-kbdneg-')); + writeFixtures(root); + try { + const win = shared.win; + await createProject(win, 'KbdNeg', root); + const pid = await startTerminal(win, root); + const term = win.getByTestId(`terminal-${pid}`); + + await runCommand(win, pid, 'echo TOP_OF_HISTORY', 'TOP_OF_HISTORY'); + await runCommand(win, pid, 'for /l %i in (1,1,200) do @echo filler %i', 'filler 200'); + + // ── The program negotiates, with the panel MOUNTED, so the live view parses it. + await runCommand(win, pid, 'node push.js', 'KITTY_PUSHED'); + await term.click(); + await win.keyboard.press('Control+Home'); + const armed = await lastKeyDecision(win, pid); + /* + * CONTROL. Without this the test cannot tell "the belief was correctly cleared" from "the + * fixture never negotiated anything", and the latter passes for free. + */ + expect(armed.kitty, 'the fixture never negotiated — the rest of this test would be vacuous').toBe( + true, + ); + expect(armed.programOwnsKeyboard).toBe(true); + expect(armed.reserved, 'a program that owns the keyboard must RECEIVE Ctrl+Home').toBe(false); + await win.keyboard.press('Control+End'); + + // ── Start the drop, then leave the tab so the pop lands with the panel unmounted. + await term.click(); + await win.keyboard.type('node drop.js', { delay: 10 }); + await win.keyboard.press('Enter'); + + await win.getByTestId('tab-add').click(); + const chips = win.getByTestId('tab-strip').locator('.tab-chip'); + await expect(chips).toHaveCount(2, { timeout: 20_000 }); + await chips.last().click(); + // Unmounted for real: the panel's terminal is not in the DOM at all. + await expect(win.getByTestId(`terminal-${pid}`)).toHaveCount(0, { timeout: 20_000 }); + + // ── Back, once the drop has certainly happened. The tail now contains the pop, and the + // rebuilt view must not end up believing the protocol is still on. + await expect + .poll( + async () => { + await chips.first().click(); + const back = win.getByTestId(`terminal-${pid}`); + if ((await back.count()) === 0) return false; + const text = (await back.textContent()) ?? ''; + if (text.includes('FILLER_DONE')) return true; + await chips.last().click(); + return false; + }, + { + timeout: 60_000, + message: 'the dropped negotiation never reached the terminal, so nothing was tested', + }, + ) + .toBe(true); + + const term2 = win.getByTestId(`terminal-${pid}`); + await expect(term2).toBeVisible(); + await term2.click(); + await win.keyboard.press('Control+Home'); + const after = await lastKeyDecision(win, pid); + + // THE ASSERTION. The program turned the protocol off; the rebuilt view must agree. + expect( + after.kitty, + 'the rebuilt view still believes the program wants enhanced key reporting, after the ' + + 'program turned it off while the panel was unmounted (#290)', + ).toBe(false); + expect(after.programOwnsKeyboard).toBe(false); + expect( + after.reserved, + 'Ctrl+Home must be reserved for scrollback once no program owns the keyboard (#290)', + ).toBe(true); + // …and the user-visible consequence: the viewport actually moves. + await expect(term2).toContainText('TOP_OF_HISTORY'); + + /* + * ══ PHASE 2: the STACK DEPTH has to survive a rebuild too, not just the flag ══ + * + * The phase above passes even if the rebuilt view re-parses the replayed tail, because that tail + * happens to hold a push AND its pop — replaying both is balanced, so the flag lands right by + * luck. What it does not land right is the DEPTH: the tail is a suffix applied on top of a state + * already derived from the whole stream, so its push is counted twice and the stack ends one + * deeper than the program's. + * + * Nothing observable goes wrong until the program pops again. Then the extra entry absorbs it, + * the flag stays set, and the terminal is right back in the reported state — one program exit + * later than anyone would think to look. + * + * So: negotiate again, rebuild the view again with the negotiation still ON (no pop in the + * window this time, so the tail's most recent word on the subject is a push), and then pop it + * live. A view whose stack is honest turns it off; a view carrying a duplicate does not. + */ + await runCommand(win, pid, 'node push2.js', 'KITTY_PUSHED_AGAIN'); + await term2.click(); + await win.keyboard.press('Control+Home'); + expect((await lastKeyDecision(win, pid)).kitty, 'the second negotiation did not take').toBe(true); + + await chips.last().click(); + await expect(win.getByTestId(`terminal-${pid}`)).toHaveCount(0, { timeout: 20_000 }); + await chips.first().click(); + const term3 = win.getByTestId(`terminal-${pid}`); + await expect(term3).toBeVisible({ timeout: 20_000 }); + + // One pop, live, with the panel mounted — the program saying "I am done". + await runCommand(win, pid, 'node pop.js', 'KITTY_POPPED_LIVE'); + await term3.click(); + await win.keyboard.press('Control+Home'); + const settled = await lastKeyDecision(win, pid); + expect( + settled.kitty, + 'one pop did not undo one push — the rebuilt view was carrying a duplicate on its ' + + 'negotiation stack, so the next program exit strands the protocol on (#290)', + ).toBe(false); + expect(settled.reserved).toBe(true); + } finally { + cleanupTemp(root); + } +}); From 2751506d30ba06ca9be9262d1b012f1fb791a93b Mon Sep 17 00:00:00 2001 From: Chris Sebok Date: Thu, 27 Aug 2026 15:22:29 +0100 Subject: [PATCH 2/2] test(terminal): assert every scroll route the report names, not just the flag behind them (#290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reporter, on the frozen state: "it was CTRL+Home / End, PGUP / PGDN etc, I did not exhaustively test control keys, but typing in to the prompt worked OK." The regression test asserted `Ctrl+Home` and the `programOwnsKeyboard` flag underneath it. Every other route dies from that same flag, so the test went red for the right reason — but that is an argument, not coverage, and the argument is exactly the kind that turns out to have an exception in it. So each route is now asserted as the user would see it: - Ctrl+Home reaches the top of the scrollback - Ctrl+End returns to the live bottom - plain PageUp moves off the live bottom, plain PageDown brings it back (plain, not Shift+ — that is the pair `use-terminal.ts` gates on `!programOwnsKeyboard`) - typing still reaches the shell That last one is the reporter's own control and it earns its place: a terminal frozen this way still accepts input, so if typing were broken too, the failure would be a different defect and this test would be pointing at the wrong one. The observable assertions now come BEFORE the diagnostics read, which changes what a failure says. Disabling each half of the fix in turn to confirm it is load-bearing: adoption of res.keyboard disabled Error: Ctrl+Home did not reach the top of the scrollback replay suppression disabled Error: one pop did not undo one push - the rebuilt view was carrying a duplicate on its negotiation stack The first of those is the sentence the person who filed the issue would recognise. Previously the same run reported that an internal boolean had the wrong value, which is true, less useful, and one inference away from the thing that actually happened to them. No production change; the fix is unaltered. --- .../e2e/terminal-keyboard-negotiation.e2e.ts | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts b/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts index 97366516..76982a13 100644 --- a/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts +++ b/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts @@ -220,10 +220,50 @@ test('a program that drops its keyboard negotiation while its panel is unmounted const term2 = win.getByTestId(`terminal-${pid}`); await expect(term2).toBeVisible(); await term2.click(); + + /* + * ══ EVERY ROUTE THE REPORT NAMES, ASSERTED AS THE USER WOULD SEE IT ══ + * + * These come BEFORE the diagnostics below, deliberately. All of them fail from the same stale + * boolean, so asserting the boolean alone would be enough to make the test go red — but a red on + * `programOwnsKeyboard` tells a reader that an internal flag is wrong, while a red on "Ctrl+Home + * does not reach the top of the scrollback" tells them what the person who filed #290 actually + * experienced. When each half of the fix was disabled to check it was load-bearing, this is + * where the failure landed. + * + * The reporter's own words for the frozen state: "Ctrl+Home / Ctrl+End, PageUp / PageDown" + * dead together, "typing in to the prompt worked OK". That last clause is not a throwaway — it + * is the signature that distinguishes THIS defect from a wedged or disconnected terminal, and + * it is asserted at the end. + */ await win.keyboard.press('Control+Home'); + await expect(term2, 'Ctrl+Home did not reach the top of the scrollback (#290)').toContainText( + 'TOP_OF_HISTORY', + ); const after = await lastKeyDecision(win, pid); - // THE ASSERTION. The program turned the protocol off; the rebuilt view must agree. + await win.keyboard.press('Control+End'); + await expect(term2, 'Ctrl+End did not return to the live bottom (#290)').toContainText( + 'filler 200', + ); + + // PLAIN PageUp/PageDown — not Shift+ — because that is the pair `use-terminal.ts` gates on + // `!programOwnsKeyboard`, and the pair the report names. + await win.keyboard.press('PageUp'); + await expect(term2, 'PageUp did not move the viewport off the live bottom (#290)').not.toContainText( + 'filler 200', + ); + await win.keyboard.press('PageDown'); + await expect(term2, 'PageDown did not bring the live bottom back (#290)').toContainText( + 'filler 200', + ); + + // THE CONTROL THE REPORT HANDS US. A terminal frozen this way still accepts input — so if + // typing were broken too, the failure above would be something else entirely and this test + // would be pointing at the wrong defect. + await runCommand(win, pid, 'echo STILL_ALIVE', 'STILL_ALIVE'); + + // …and the mechanism underneath all four, so a future reader knows WHY they died together. expect( after.kitty, 'the rebuilt view still believes the program wants enhanced key reporting, after the ' + @@ -234,8 +274,6 @@ test('a program that drops its keyboard negotiation while its panel is unmounted after.reserved, 'Ctrl+Home must be reserved for scrollback once no program owns the keyboard (#290)', ).toBe(true); - // …and the user-visible consequence: the viewport actually moves. - await expect(term2).toContainText('TOP_OF_HISTORY'); /* * ══ PHASE 2: the STACK DEPTH has to survive a rebuild too, not just the flag ══