Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughTerminal interrupt payloads now use the target PTY’s Kitty keyboard flags. The shared helper selects Kitty Ctrl+C bytes when flag bit 1 or 8 is set, and ETX otherwise. The runtime reads flags for direct and leaf PTYs. The headless emulator exposes the flags and reapplies numeric flags during snapshot restoration and renderer hydration. RuntimeTerminalWriter now writes the suffix already present in the payload. The renderer also resolves interrupt input from current Kitty keyboard flags. Tests cover flag access, snapshot restoration, interrupt-byte selection, and payload writing. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Ctrl+C can still be ignored by a Kitty-aware TUI after a snapshot and nested keyboard-mode change. The issue is limited to that state sequence and can be addressed as follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes preserve terminal destinations and existing input checks. No introduced security vulnerability was established, but successful interruption still depends on synchronized keyboard state during recovery, and coverage is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test suggestion inline, plus a scope observation.
Reviewed changes
- Shared interrupt bytes helper — new
src/shared/terminal-interrupt-bytes.tsexportsTERMINAL_INTERRUPT_ETX,TERMINAL_INTERRUPT_KITTY_CTRL_C(\x1b[99;5u), andterminalInterruptBytes(flags), which picks the kitty encoding only whenflags > 0. - Runtime write path encodes Ctrl+C —
RuntimeTerminalWritergains agetKittyKeyboardFlags(ptyId)getter and routesaction.interruptthrough the helper;buildTerminalSendPayload(action, kittyKeyboardFlags)uses it too, sobytesWrittenreflects the bytes actually written. - Flag source —
HeadlessEmulator.kittyKeyboardFlags()exposes the flags xterm parsed from PTY output (the emulator opts intovtExtensions.kittyKeyboard), andreadPtyKittyKeyboardFlagsreads them fromheadlessTerminals, falling back to0when no model exists yet. - Wiring — the getter is injected into the
RuntimeIdwriter and bothsendTerminalcall sites. - Tests — the three new test files cover the helper, the writer with/without flags, and the emulator reader (5 pass locally).
The flags > 0 gate matches xterm's own KittyKeyboard.shouldUseProtocol(flags), so the runtime now agrees with what xterm would encode for Ctrl+C under the same flags. The no-flags path still writes ETX, so plain shells are unaffected.
ℹ️ The renderer's Ctrl+C path still sends bare ETX, so the "shared helper" is only half wired
The PR body says in-app Ctrl+C "already arrives as CSI 99;5u because xterm encodes it", but the renderer actually intercepts plain Ctrl+C in xterm-bypass-policy.ts (shouldHandleTerminalInterruptKeyboardEvent) and writes a bare ETX through pane.terminal.input(TERMINAL_INTERRUPT_INPUT) — and the kitty reset that used to accompany that write was removed in 3eac4d93d3 (#23584). So the two interrupt paths now produce different bytes, and issue #17665 specifically asked for "one shared helper so keyboard, RPC and CLI all produce the same bytes." This is not a bug in the runtime fix you shipped, but it is worth confirming the keyboard path shouldn't use terminalInterruptBytes as well.
Technical details
# Keyboard interrupt path diverges from the runtime helper
## Affected sites
- `src/renderer/src/components/terminal-pane/xterm-bypass-policy.ts:306` — `shouldHandleTerminalInterruptKeyboardEvent` bypasses xterm's kitty encoder for plain Ctrl+C.
- `src/renderer/src/components/terminal-pane/terminal-pane-pane-input.ts:140-143` — sends `TERMINAL_INTERRUPT_INPUT` (`\x03`) via `pane.terminal.input()`; the `resetTerminalKeyboardProtocolAfterInterrupt` call was deleted in `3eac4d93d3`.
- `src/shared/terminal-interrupt-bytes.ts` — the new helper is only consumed by `runtime-terminal-writer.ts` and `terminal-send-payload.ts`.
## Open questions for the human
- Is the in-app Ctrl+C path out of scope for this PR by design, or should it also encode `CSI 99;5u` for a kitty PTY?
- If in-app Ctrl+C keeps sending bare ETX and still works against Claude Code, that would contradict the issue's premise that ETX is swallowed — worth reconciling before calling this a parity fix.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
149878f to
7958201
Compare
Sync update (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d786b6dd-ce16-43f1-a37e-a3b0c01db0c2
📒 Files selected for processing (11)
src/main/daemon/headless-emulator-kitty-flags.test.tssrc/main/daemon/headless-emulator.tssrc/main/runtime/headless-seed-ownership.test.tssrc/main/runtime/orca-runtime-controller-knows-pty-is-live.tssrc/main/runtime/orca-runtime-create-pty-headless-terminal-state.tssrc/main/runtime/orca-runtime-fit-override-listeners.tssrc/main/runtime/runtime-terminal-writer.test.tssrc/main/runtime/runtime-terminal-writer.tssrc/main/runtime/terminal-send-payload.tssrc/shared/terminal-interrupt-bytes.test.tssrc/shared/terminal-interrupt-bytes.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
ℹ️ No critical issues — the incremental commit cleanly addresses both prior findings; one open question on a sibling hydration path is inline.
Reviewed changes
- Precomputed interrupt payload —
buildTerminalSendPayloadnow takes the PTY's kitty flags, andRuntimeTerminalWriterderives the suffix aspayload.slice(text.length)rather than re-reading flags, so the bytes written always match the reportedbytesWritten. - Flags restored across a replaced model —
replaceHeadlessTerminalAfterExecutionContextChangere-appliessnapshot.kittyKeyboardFlagsafter the provider seed, since rehydrate sequences omit kitty pushes. - Flag lookup moved to the runtime — new
readPtyKittyKeyboardFlagson the fit-override base class readsheadlessTerminals; bothsendTerminalpayload builds pass it; the writer's flag getter was removed. - Tests tightened and added — exact
toBe(1)assertion inheadless-emulator-kitty-flags.test.ts, and a provider-replacement restore test inheadless-seed-ownership.test.ts.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
7958201 to
df4a0e1
Compare
Sync update (
|
There was a problem hiding this comment.
ℹ️ No critical issues — the incremental commit is correct; one description nit below.
Reviewed changes
- Narrowed the kitty gate to the flags that actually change Ctrl+C —
terminalInterruptBytesnow testsflags & (1 | 8)instead offlags > 0, so alternate-key (4), event-type (2), and associated-text (16) modes keep the bare ETX. - Regression tests for the untouched bits —
terminal-interrupt-bytes.test.tspins flag8to the kitty encoding and flags4/2|4|16to ETX.
This matches the kitty spec: only "disambiguate escape codes" (1) and "report all keys as escape codes" (8, which implies disambiguation) turn legacy Ctrl+C into CSI 99;5u; alternate keys (4) are a pure enhancement to already-escaped events, and 2/16 alone do not change Ctrl+C. The four interrupt/emulator test files pass locally.
ℹ️ Nitpicks
- The PR description still says an interrupt writes
CSI 99;5uwhenever the kitty flags are "non-zero"; after this commit only flags1/8do (2,4,16stay ETX). A one-line description update would keep it matching the code.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Sync update (
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Renderer hydration now restores kitty flags —
maybeHydrateHeadlessFromRendererre-appliesrendered.kittyKeyboardFlagsafter the seed write, matching the provider-snapshot path, so an interrupt for a still-running kitty TUI encodesCSI 99;5uinstead of falling back to ETX. - Regression test —
headless-hydration-ownership.test.tsseeds a renderer snapshot carryingkittyKeyboardFlags: 1and asserts the model reports1andterminalInterruptBytesyieldsCSI 99;5u.
The new reapply is placed after the seed and before the live writes chain, so a kitty push from later live output still wins. The renderer serializer only publishes these flags when the renderer is the ordered-stream authority and has a proven baseline, so the guard fires exactly in the renderer-authoritative reattach case it targets. All five interrupt/emulator test files pass locally (30 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f5641587-c5c0-47c3-b2d3-0b389036df53
📒 Files selected for processing (2)
src/main/runtime/headless-hydration-ownership.test.tssrc/main/runtime/orca-runtime-maybe-hydrate-headless-from-renderer.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
6f6e819 to
4e73ab6
Compare
Sync update (
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Empty renderer-buffer hydration now applies kitty flags —
maybeHydrateHeadlessFromRenderersplits the empty-datareturn so a blank renderer buffer that still carries provenkittyKeyboardFlagsseeds them into the fresh emulator, instead of leaving it at0and falling back to ETX on interrupt. - Regression test —
headless-hydration-ownership.test.tsresolves an emptydata/kittyKeyboardFlags: 1snapshot and asserts the model reports1andterminalInterruptBytesyieldsCSI 99;5u.
The new branch places the flag application before its early return and mirrors the non-empty path's this.headlessTerminals.get(ptyId) !== state guard outcome (both end the chain link via finally), so a replacement model during the write cannot be seeded twice. All five interrupt/emulator test files pass locally (31 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
4e73ab6 to
7f93f86
Compare
|
Thanks for pointing out the renderer path and the flag wording. The renderer intercepts Ctrl+C before xterm can encode it. I routed that key path through the shared interrupt encoder using the pane's current Kitty flags, so flags I also updated the PR description to match the exact flag behavior and current scope. The focused run passed all 27 tests across the renderer, shared helper, runtime writer, and headless emulator test files. I have not run a manual TUI session or manual tests on other platforms. |
There was a problem hiding this comment.
Important
The new renderer encoder emits the kitty Ctrl+C bytes, but the renderer's own Ctrl+C recognizers nowhere else learned to read them, so interrupt intent is dropped on a supported input path.
Reviewed changes
- Renderer Ctrl+C now uses the shared encoder —
terminal-pane-pane-input.tsreplaces the bareTERMINAL_INTERRUPT_INPUTwithresolveTerminalInterruptKeyboardInput(paneKittyKeyboardModesRef.current.get(pane.id)?.flags ?? 0), so an in-app interrupt to a kitty PTY writes\x1b[99;5u, closing the parity gap raised in an earlier pullfrog review. - Constant replaced by a resolver —
xterm-bypass-policy.tsdropsTERMINAL_INTERRUPT_INPUTfor a documentedresolveTerminalInterruptKeyboardInputthat delegates toterminalInterruptBytes, and the unit test pins kitty flags1/8to\x1b[99;5uand4to ETX. - Comment tidy-ups — JSDoc rewritten in
orca-runtime-maybe-hydrate-headless-from-renderer.tsandterminal-send-payload.ts.
ℹ️ Nitpicks
shouldHandleTerminalInterruptKeyboardEvent's JSDoc (xterm-bypass-policy.ts:307-310) still says the chord is "sent as ETX throughTerminal.input()"; it now sendsCSI 99;5uwhenever the pane's kitty flags are set.- The PR description's "Out" list still names "Keyboard handling in the renderer" as out of scope, but this commit changes exactly that.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
7f93f86 to
080b232
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this commit — it closes the interrupt-intent and bracketed-paste recognition gap. The prior review thread stays open: two downstream Ctrl+C consumers (
terminal-input-quarantine.ts,shell-command-inference.ts) still match only the literal ETX byte, so a Kitty-encoded interrupt is not fully recognized across the input path.
Reviewed changes
- Shared Ctrl+C predicate —
src/shared/terminal-interrupt-bytes.tsgainsisTerminalInterruptInput(data), which recognizes bothTERMINAL_INTERRUPT_ETXandTERMINAL_INTERRUPT_KITTY_CTRL_Cas Ctrl+C. - Renderer recognizers routed through the predicate —
interrupt-input-intent.tsnow usesisTerminalInterruptInputininputMatchesIntent,inferIntentFromExactTerminalInput, andobserveAcceptedTerminalInput, so a Kitty Ctrl+C still recordsctrl-cintent and marks bracketed paste stale on the fallback path where no keydown intent was captured. - Regression test for the non-Latin key path —
pty-connection-interrupt-inference.test.tsdispatchesㅊ/KeyC(whichisCtrlCKeyEventrejects, so no pending intent is set), feeds the\x1b[99;5ubytes, and assertsinferInterruptreceivesintent: 'ctrl-c'plus the bracketed-paste stale marking. Unit tests interminal-interrupt-bytes.test.tscover the predicate. - JSDoc tidy —
shouldHandleTerminalInterruptKeyboardEvent's comment now describes ETX-or-Kitty selection.
Note the commit is correct for the consumers it touches; the open thread is not re-raised inline per the re-review dedup rule.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
terminal send --interrupt wrote a bare ETX. A TUI that enabled the kitty keyboard protocol ignores that byte, so the CLI reported success while the turn kept running. When the PTY's headless model has kitty flags, write CSI 99;5u. Flags of 0 still send ETX. Fixes stablyai#17665
…odel A namespace change rebuilt the headless terminal from a provider snapshot and dropped the kitty keyboard flags, so a later interrupt sent ETX. The replacement now reapplies those flags, and the writer sends the interrupt bytes already chosen for that send.
Flag 4 does not change legacy Ctrl+C encoding, so an interrupt in that mode was sending CSI 99;5u instead of ETX.
…odel A mounted renderer snapshot omits kitty pushes. The provider replacement path already restores the flags carried beside the payload. Renderer hydration now does the same, so an interrupt still encodes Ctrl+C.
A blank serialized buffer can still carry the keyboard flags beside the payload. Hydration used to return before applying them, so a fresh emulator stayed at 0 and an interrupt sent ETX.
080b232 to
17d74ca
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the delta since pullfrog review 5406270718 (080b232), which is head commit 17d74ca teaching the remaining downstream Ctrl+C consumers to recognize the Kitty encoding.
- Shared contains predicate —
src/shared/terminal-interrupt-bytes.tsgainscontainsTerminalInterruptInput(data), recognizing either Ctrl+C encoding anywhere in an accepted chunk (alongside the existing whole-stringisTerminalInterruptInput). - Quarantine disarms on Kitty Ctrl+C —
terminal-input-quarantine.ts'sLINE_TERMINATORSnow lists bothTERMINAL_INTERRUPT_ETXandTERMINAL_INTERRUPT_KITTY_CTRL_C, so a Kitty interrupt drops the surviving line tail and disarms exactly as ETX did. - Shell-command inference recognizes Kitty Ctrl+C —
shell-command-inference.tsroutes its top-level reconfirmation and suspended-inference checks throughcontainsTerminalInterruptInput, and its per-char scanner consumes the Kitty sequence before falling through to CSI parsing. - Pure-function extraction —
observeAcceptedShellCommandInputis now an exported function over aShellCommandInputStateadapter, making the Ctrl+C handling directly unit-testable;installShellCommandInferencebinds it to the live session. - Regression tests — new
pty-connection-shell-command-inference.test.tscovers both encodings for line reset and suspended-inference cancel; the quarantine suite adds a Kitty-tail case;terminal-interrupt-bytes.test.tscovers the contains predicate.
The prior open thread (Kitty Ctrl+C not recognized by terminal-input-quarantine.ts and shell-command-inference.ts) is closed by this commit. Focused tests pass locally: 42 across 4 files.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
ELI5
Ctrl+C in the in-app terminal now interrupts a Kitty-mode terminal application using the Kitty Ctrl+C sequence, including when the key event comes from a non-Latin input source.
What Changed
Why
The renderer intercepts Ctrl+C before xterm can encode it. On a non-Latin key path, the renderer could send the Kitty sequence without setting a pending Ctrl+C intent, while the exact-byte fallback recognized only ETX. The interrupt was delivered, but intent tracking and bracketed-paste recovery were skipped. The shared recognition keeps the byte and the renderer's follow-up behavior aligned.
Linked Issue
Fixes #17665
Visual Proof
N/A — the layout does not change, and a screenshot cannot show terminal input bytes or intent tracking. Regression tests assert both byte encodings and the non-Latin renderer path.
Testing
Ran on macOS:
All 86 tests passed. I did not manually test this on macOS, Linux, Windows, SSH, or mobile sessions. The full lint, typecheck, test, and build commands were not run locally.
AI Disclosure
OpenAI Codex (GPT-6) assisted with the implementation and review.
Review
Self-reviewed byte selection, the renderer's intent recognition, and bracketed-paste interruption. The integration test uses a non-Latin key event with code KeyC, sends Kitty Ctrl+C, and checks both inferred intent and paste-state reset.
Agent skill upstream boundary
Notes
The wire format remains ETX when Kitty disambiguation flags are not enabled. Automated tests cover the shared helper, renderer, runtime writer, headless emulator, and hydration paths; no live TUI or remote SSH session was tested.
Checklist