Fix: Kitty keyboard mode stack should live in the daemon, not a frontend Map - #37
Open
pullapprove5[bot] wants to merge 1 commit into
Open
Fix: Kitty keyboard mode stack should live in the daemon, not a frontend Map#37pullapprove5[bot] wants to merge 1 commit into
pullapprove5[bot] wants to merge 1 commit into
Conversation
The negotiated half of the kitty keyboard protocol lived in a module-level Map in the frontend, derived by re-parsing PTY bytes as xterm saw them. Two things follow from that. A window that reattaches cold rebuilds its xterm from the 256 KiB replay ring, so for any program that has produced more output than that, the push it made at startup has scrolled out and every chord silently reverts to legacy encoding while the program still believes the protocol is on. And a per-realm Map means two windows on one terminal each hold their own answer, at most one of which is right. The stack now lives next to the vte::Parser the status scanner already runs over every PTY byte for the session's whole life: one stack, never truncated, shared by every window. It rides out on the session status as `kittyFlags`, the way phase/cwd/title already do, and `CSI ? u` is answered on the reader thread — so a program gets its answer even with no window attached, instead of round-tripping to the frontend. The encoder stays in the frontend, where it needs a DOM KeyboardEvent; it now reads the mode the daemon reported rather than one it derived. Also handles DECSTR (`CSI ! p`), which was unhandled: like RIS it puts a stuck keyboard back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
desktop/ui/components/Terminal/kitty-keys.tskept the negotiated kitty keyboard mode stack in a module-levelMap, re-derived per window by parsing the PTY bytes xterm happened to see. Two consequences, both confirmed against the checkout:TerminalPanerebuilds a fresh xterm fromterminalReplay, which is the 256 KiBRing(core/src/terminal/ring.rs). A program that pushes kitty mode once at startup and then produces more than that much output has had its push dropped from the ring, so a new window (or a web reload) started with an empty stack and silently encoded every chord the legacy way while the program still believed the protocol was on.Mapis per-JS-realm, so each window held its own answer and at most one was right.What changed
The negotiated state moves to the daemon, next to the
vte::Parserthatstatus.rsalready runs over every PTY byte for the session's whole life. The encoder stays in the frontend, where it needs a DOMKeyboardEvent.core/src/terminal/kitty.rs(new) —KittyKeyboard: the per-screen-buffer mode stacks, with push/pop/set-in-place, screen switching, reset, and the prompt-time leak clear. Same semantics the TypeScript had (five-bit masking on push, "unwind everything" on an over-deep pop, malformed modes ignored, depth-8 wrap), ported with its reasoning and its tests.status.rs— the scanner now implementscsi_dispatch/esc_dispatch: the four…unegotiation sequences, DECSET/DECRST 47/1047/1049 for which stack is live, RIS and DECSTR. ACSI ? uquery is answered from the session's own stack; the bytes come back throughStatusScanner::take_reply.session.rs— the PTY writer moves fromSessionintoSharedso the reader thread can write the query answer on the thread that parsed it. A program now gets its answer whether or not a window is attached, instead of round-tripping to the frontend. No new lock nesting: the scanner lock is released before the writer lock is taken, in both directions.wire.rs—SessionStatus.kitty_flags(#[serde(default)]), so the mode arrives on the wire the wayphase/cwd/titlealready do.kitty-keys.tskeeps the encoder and replaces the stack with a small cache of what the daemon last reported (setKittyFlags/kittyFlags/forgetKittyState);terminalSlice.applyTerminalStatus— the one funnel both the live status stream and the cold-reattach replay pass through — records it.registry.tsno longer registers negotiation handlers.TerminalStatus.kittyFlagsadded to the types and the shared fixture.Also handles DECSTR (
CSI ! p), which the finding noted was unhandled; like RIS it clears a stuck keyboard.Deliberately left out
The finding also suggested driving the leak clear off
on_poll(at_prompt, …)rather than only OSC 133;A. I did not do that, and I think it would be a regression: the poller fires on an interval, so a tick landing after a shell that negotiates kitty mode for its own line editor (fish, zsh with kitty support) has emitted its prompt mark and pushed its flags would wipe that push — and keep wiping it. The prompt mark is a one-shot signal that arrives before the shell's push; a repeating poll observation isn't. Clearing at the OSC 133 prompt mark is retained, and the per-screen stacks (the main leak defence) are unchanged.What I could and couldn't verify
rustfmt --edition 2021 --checkis clean on every touched Rust file (it also parses them, so there are no syntax errors).cargo build --offlinefails fetching dependencies), Zig is not installed so theterminalfeature could not link libghostty even with deps, anddesktop/node_modulesis absent withnpm ci --offlinefailing on ENOTCACHED — sotsc --noEmit, vitest andcargo testall could not run. The Rust and TypeScript changes are unit-test-covered but those tests are unrun; please runscripts/testbefore merging.kitty.rsports the stack/screen-scoping/leak scenarios from the deleted TypeScript suite;status.rsadds byte-level tests for push/pop/set, the query reply, alt-screen scoping, the prompt-mark leak clear, that command marks do not clear a running program's mode, and RIS/DECSTR. The TypeScript suite keeps its encoder tests plus the real-xterm DECCKM seam test, and gains tests for the reported-mode cache.Opened by a PullApprove implementation run (implement-finding v1) for the finding PA-25 — Kitty keyboard mode stack should live in the daemon, not a frontend Map.
Merging this is what closes the finding as fixed.