Skip to content

fix(cli): Ghostty gets modifyOtherKeys level 1 — Shift+letters leak as literal '[27;2;<code>~' when IME splits the ESC byte - #91937

Open
rosymorning wants to merge 1 commit into
NousResearch:mainfrom
rosymorning:fix/ghostty-modifyotherkeys-level1
Open

rosymorning wants to merge 1 commit into
NousResearch:mainfrom
rosymorning:fix/ghostty-modifyotherkeys-level1

Conversation

@rosymorning

Copy link
Copy Markdown

Summary

On Ghostty, Shift+letter in the classic CLI inserts the literal text [27;2;65~ instead of the capital (Shift+W → [27;2;87~, etc.). Regression class of #87390: the alias table added in #87511 fixed the whole-sequence case, but the split-delivery path remains broken.

Root cause chain

  1. _enable_extended_enter_keys() pushes xterm modifyOtherKeys level 2 (CSI >4;2m) for Ghostty (allowlist entry), so Ghostty re-encodes every modified key (Shift+A) as ESC[27;2;65~.
  2. On macOS with an IME active, Ghostty can deliver that sequence with the ESC byte in a separate read from the [27;...~ body.
  3. prompt_toolkit's read loop calls vt100_parser.flush() after each chunk (Vt100Input.flush_keys — 'Used for flushing the escape key'), so the lone \x1b is emitted as the Escape key; the body then arrives as literal characters and is inserted into the prompt.

Parser repro (prompt_toolkit 3.0.52 + current alias table):

  • feed('\x1b[27;2;65~') + flush['A']
  • feed('\x1b') + flush; feed('[27;2;65~') + flush[Escape,'[','2','7',';','2',';','6','5','~'] ✗ (exact symptom)

Fix

Ghostty branch now pushes modifyOtherKeys level 1 (CSI >4;1m) instead of level 2. Level 1 only re-encodes modified keys whose unmodified form is a control character (Enter 13, Backspace 127, Tab 9) — exactly the set this CLI decodes — while Shift+letters arrive as plain capitals again, immune to the split-ESC/IME path. Kitty protocol is still not pushed for Ghostty (keeps the #87630 Backspace-Alt workaround). Other allowlisted terminals (iTerm2, WezTerm, tmux, VS Code) keep the dual kitty+level-2 push and the full alias table.

Preserved: Shift+Enter → newline, Ctrl+Enter → newline, Cmd+Backspace kill, Shift+Tab BackTab, etc. — all level-1 control-key encodings already in pt_input_extras.py.

Verification

  • Reproduced the leak at parser level (above), then confirmed level-1 push output for Ghostty (\x1b[>4;1m) and plain-'A' delivery for Shift+A.
  • Updated ghostty push assertions in tests/cli/test_ctrl_enter_newline.py.
  • pytest tests/cli/test_ctrl_enter_newline.py tests/cli/test_cli_shift_enter_newline.py tests/cli/test_cli_cmd_backspace.py tests/cli/test_modify_other_keys_aliases.py → 244 passed, 2 skipped.
  • Manual (local Ghostty 1.3.1, Korean IME): Shift+letters insert capitals; Shift+Enter still inserts a newline.

Closes the #87390 leftover (split-delivery path).

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Aug 22, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

No blocking issues found.

The root-cause analysis is convincing and the fix is minimal: dropping Ghostty from modifyOtherKeys level 2 to level 1 keeps every alias the CLI maps (Shift/Ctrl+Enter, modified Backspace/Tab are still re-encoded under level 1) while Shift+letters return to plain text, so the IME-split-ESC leak path has nothing left to emit. Both detection paths (TERM_PROGRAM and TERM) are covered by updated assertions, including the negative check that level 2 is gone.

Nit: tests/cli/test_ctrl_enter_newline.py:171-193 — the suite pins which escape sequence is written, but nothing reproduces the actual failure mode: two consecutive _patched_vt100_feed calls with the ESC byte split from the CSI body ("\x1b" then "[27;2;65~") asserting no literal leaks into the prompt. Since the strategy here is "avoid the trigger" rather than "survive the trigger", a follow-up that raises Ghostty back to level 2 would pass the current tests while reintroducing the bug. Related pre-existing observation (out of scope, no action required): nothing ever pops modifyOtherKeys on shutdown — the only teardown sequence is the Kitty-mode \x1b[<u at cli.py:4187 — so the pushed level persists in the terminal after exit today, regardless of which level this PR selects.

@rosymorning
rosymorning force-pushed the fix/ghostty-modifyotherkeys-level1 branch from c566ada to d77d987 Compare August 22, 2026 12:50
@rosymorning

Copy link
Copy Markdown
Author

Thanks for the review! Addressed both points.

Split-delivery regression test (nit, accepted): added test_split_esc_delivery_does_not_leak to tests/cli/test_modify_other_keys_aliases.py. It feeds the exact "\x1b" + "[27;2;65~" split across two consecutive read chunks (with _patched_vt100_feed-style sequential feeds, no flush between) and asserts the parser still yields ['A'] with no literal leakage — so dropping the ESC[27;2;<cp>~ mappings now fails the suite even while whole-sequence tests stay green. It also pins the reversed case (split + intervening flush → Escape + literal body) as the documented reason the Ghostty push is level 1, so the strategy is deliberate rather than implicit, and a future parser-level fix that survives the trigger would have its milestone visible in this test.

modifyOtherKeys pop on shutdown (observation): the teardown sequence actually does reset it — _TERMINAL_INPUT_MODE_RESET_SEQ (cli.py:4179) contains both \x1b[<u (kitty pop, line 4187) and \x1b[>4m (modifyOtherKeys reset, line 4188), and it is written from the atexit/cleanup path (cli.py:1486/1493). The only bypass is the os._exit(0) fast path, which per the existing comment at cli.py:1470 skips all mode resets by design. Left untouched as out of scope — agreed, no action needed here.

244 passed, 2 skipped with the new test in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants