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..76982a13 --- /dev/null +++ b/packages/ui/tests/e2e/terminal-keyboard-negotiation.e2e.ts @@ -0,0 +1,320 @@ +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(); + + /* + * ══ 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); + + 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 ' + + '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); + + /* + * ══ 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); + } +});