fix(cli): make Shift+letter actually type a capital under modifyOtherKeys - #87785
fix(cli): make Shift+letter actually type a capital under modifyOtherKeys#87785krunkosaurus wants to merge 2 commits into
Conversation
…Keys NousResearch#87511 mapped `ESC[27;2;<code>~` and `ESC[<code>;2u` to the uppercase character, but the prompt still receives the raw escape sequence. prompt_toolkit's VT100 parser reports every match as `KeyPress(key=<table value>, data=<matched bytes>)`, and the default `Keys.Any` binding inserts `event.data` — the bytes, not the key. For the hundreds of `Keys`-valued entries that is invisible, because bindings match on `key` and `data` is never read. For the character-valued entries added by `install_modify_other_keys_aliases()` it is the whole ballgame: mapping `ESC[27;2;72~` → `"H"` produces `KeyPress(key="H", data="\x1b[27;2;72~")`, so `[27;2;72~` is what lands in the buffer. Net effect: the mapping fixed what the key *is* but not what it *types*, and Shift+letter is still unusable on Ghostty / iTerm2 / WezTerm / kitty — the same symptom NousResearch#87390 reported. Verified against main at 7095e23: typing `Hello World` submits `\x1b[27;2;72~ello \x1b[27;2;87~orld`. Fix: narrow `KeyPress.data` to the character for character-valued table entries (Shift+letter, Shift+Space, keypad digits). Stock prompt_toolkit ships no character-valued entries and the parser's fallback path already calls the handler with `key is data` for ordinary typing, so nothing else changes shape. Ctrl/Alt combos, named keys, bracketed paste and the Shift+Enter aliases are unaffected. Tests assert on submitted prompt text via a real `PromptSession` over a pipe input rather than on parsed keys — key-level assertions cannot catch this class of bug, which is why it survived NousResearch#87511's 160 tests. 9 of the 14 new tests fail on unpatched main. Refs NousResearch#87511, NousResearch#87390, NousResearch#86866, NousResearch#87631, NousResearch#87637. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up on NousResearch#87637: prompt_toolkit's default Keys.Any binding inserts event.data (the matched bytes), not the key, so the parse-level key assertions cannot see what actually reaches the prompt buffer. Add an xfail(strict=False) guard on the full observable (key AND data) for Latin/Cyrillic/Greek — xfailed until the data-path fix in NousResearch#87785 lands, green automatically once applied on top.
|
This was generated by AI during triage. Summary: Evidenceno deterministic fact backs this claim — model belief, not executed or read evidence Checked against |
fix(cli): make Shift+letter actually type a capital under modifyOtherKeys
|
…ving Review feedback from @spfcraze and @Enough1122, all three points valid: - `Vt100Parser._call_handler` was fetched outside the guard, so a future prompt_toolkit that renames it would raise AttributeError through install_modify_other_keys_aliases() into cli.py's blanket `except Exception: pass`, silently skipping the installers that run after it. Fetch via getattr with a None-guard so a moved internal degrades to the same no-op as a missing module — which is what the docstring already promised. Pinned by a test that deletes the attribute. - The idempotency marker lived on the class while the wrapper lived on the method, so the two could drift: if anything replaced `_call_handler`, the marker stayed True and the replacement was never wrapped. Move the marker onto the wrapper, where it cannot outlive what it describes. - The test fixture restored ANSI_SEQUENCES but not the monkeypatch, so it leaked to the rest of the process. Restore it in teardown. Also resolve Vt100Parser per call rather than at import. tests/cli/test_bracketed_paste_timeout.py reloads prompt_toolkit.input.vt100_parser, rebinding the module's class to a new object, so a module-level import here held the pre-reload class and patched something the parser no longer used — the new marker-drift test passed alone and failed in the full suite. tests/cli: 1217 passed, same 3 pre-existing failures as pristine main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks both — all three points were valid, fixed in 472488f. 1. Unguarded 2. Marker/wrapper drift. Also correct. Fixed by moving the marker onto the wrapper function instead of the class, so it cannot outlive what it describes: if anything later replaces 3. Fixture leaked the monkeypatch. Fixed — teardown restores Writing the test for #2 turned up something worth flagging separately: it passed in isolation and failed in the full suite.
Investigated with Claude Code. |
|
Status update for reviewers — independent verification since the last push:
That makes four reports tracing to this one defect: #86866 (Shift+Space, Ctrl+K), #87390 (Ghostty capitals), #87631 (WezTerm Cyrillic), #88071 (Shift+Space again). The mapping PRs are all correct and still needed — they just cannot take effect until Investigated with Claude Code. |
|
This PR closes a second issue that nobody has linked to it, and I think that is worth stating plainly because the title makes it look narrower than it is. #90640 ("CLI prints raw CSI-u sequences for all numeric keypad keys", P2, filed today, no PR on it) is the same defect on a different symptom, and Their report is For scale, on current main there are 123 character-valued entries in the table after all four installers run: Your patch is the only thing open that addresses all of them. Overlap with #88109#88109 ("stop Shift+Space from inserting raw CSI bytes") is the same root cause on a third symptom and touches the same function. It introduces So the special-case becomes redundant if this lands first, and if #88109 lands first this still has to touch the same lines. That is a sequencing decision for the maintainers rather than something either of you can resolve alone, but it should be made deliberately — I have left the same note there. One thing I could not settle, offered as a question rather than a findingYour patch narrows for i, k in enumerate(key):
self._call_handler(k, insert_text if i == 0 else "")
# "only pass data payload to first KeyPress (so that we won't insert it
# multiple times)"Measured, unmodified Alt+a: I tried to determine whether that produces a visible double-insert for an Alt+ combination that has no binding, and I could not build a If it is easy for you to check: press an unbound if insert_text and isinstance(key, str) and len(key) == 1:
insert_text = keyEverything else here reads right to me, including fetching Not opening a competing PR; this one should land. |
|
Confirming this on Linux + Ghostty, and adding three datapoints: it still applies to today's Environment
Symptom on hardware. Shift+letter inserted Verification. Applied this branch at
All 26 Shift+letters type their capital under all three encodings (78/78); no regression in the Still applies to current #90640 confirmed as a duplicate. The keypad rows above are the same defect — One Mitigation until this lands, which also confirms the push is the trigger: # ~/.hermes/config.yaml
display:
cli_multiline_shortcuts: falseThat gates both push sites ( Minor suggestion. Worth tightening the existing helper too: Investigated with Claude Code. |
|
@krunkosaurus what is the plan / ETA for shipping this? |
Not up to me. I'm just a random dev |
|
@kshitijk4poor are you able to help get this merged? |
|
Independent confirmation from Fedora Linux + Ghostty: this is still reproducible on Hermes v0.20.5, and the Environment:
Observed with Shift+I: The existing key-only regression test passed before the fix because it discarded I applied an equivalent local
This PR addresses the root cause seen on current Ghostty/Fedora hardware, not just a synthetic parser case. |
|
Reproduced your diagnosis exactly and it's correct — this is in fact the gap that breaks Shift+letter on Ghostty. On current ANSI_SEQUENCES['\x1b[27;2;73~'] -> 'I'
parse('\x1b[27;2;73~') -> KeyPress(key='I', data='\x1b[27;2;73~')
Interim data point for the thread: a level-1 workaround ( Signed off on your approach. |
|
@alt-glitch Thanks for the triage note. One small chronology correction: #87785 was opened on 2026-08-16 at 15:54 UTC, while #88097 was opened on 2026-08-17 at 03:09 UTC. This PR therefore predates #88097 by about 11 hours and 15 minutes, rather than #88097 being the earlier PR. Regardless of which implementation the maintainers prefer, the underlying issue remains unresolved in the current v0.20.5 release, and both PRs are still open. I can still reproduce it locally in Ghostty: the parser correctly maps Shift+I to It would be helpful to consolidate on one implementation or get maintainer direction on what should change before merge. I'm happy to rebase or adapt this PR if needed; I mainly want to make sure one of the fixes lands so affected users are not left with both repairs open and the bug still present. |
…input
Under modifyOtherKeys level 2, Ghostty encodes Shift+key as ESC[27;2;<cp>~.
The alias table decoded the key (e.g. Shift+A -> 'A') but prompt_toolkit's
Vt100Parser passed the raw matched escape bytes as KeyPress.data, and the
default self-insert binding inserts event.data -- so the raw sequence landed
in the buffer (e.g. '[27;2;65~') instead of the character.
- install_literal_key_data_patch(): wrap Vt100Parser._call_handler so
single-character string keys carry the decoded char as insert_text
(type(key) is str guard leaves Keys enums untouched)
- map the 11 Ghostty-escaped shifted symbols (0x40-0x7F: @ [ \\ ] ^ _ ` { | } ~)
tilde-form only -- codepoint is the already-shifted, layout-resolved text
- add buffer-level regression tests through a real Application + TextArea,
incl. negative control (unpatched parser leaks raw bytes)
Refs upstream NousResearch/hermes-agent NousResearch#87390, NousResearch#86866, NousResearch#87785
…input
Under modifyOtherKeys level 2, Ghostty encodes Shift+key as ESC[27;2;<cp>~.
The alias table decoded the key (e.g. Shift+A -> 'A') but prompt_toolkit's
Vt100Parser passed the raw matched escape bytes as KeyPress.data, and the
default self-insert binding inserts event.data -- so the raw sequence landed
in the buffer (e.g. '[27;2;65~') instead of the character.
- install_literal_key_data_patch(): wrap Vt100Parser._call_handler so
single-character string keys carry the decoded char as insert_text
(type(key) is str guard leaves Keys enums untouched)
- map the 11 Ghostty-escaped shifted symbols (0x40-0x7F: @ [ \\ ] ^ _ ` { | } ~)
tilde-form only -- codepoint is the already-shifted, layout-resolved text
- add buffer-level regression tests through a real Application + TextArea,
incl. negative control (unpatched parser leaks raw bytes)
Refs upstream NousResearch/hermes-agent NousResearch#87390, NousResearch#86866, NousResearch#87785
What does this PR do?
Completes #87511. That PR mapped
ESC[27;2;<code>~/ESC[<code>;2uto the uppercase character, but Shift+letter still leaks its escape sequence into the prompt — the mapping fixed what the key is, not what it types.prompt_toolkit's VT100 parser reports every match as
KeyPress(key=<table value>, data=<matched bytes>), and the defaultKeys.Anybinding insertsevent.data— the bytes, not the key. For the hundreds ofKeys-valued entries ininstall_modify_other_keys_aliases()that is invisible: bindings match onkey, anddatais never read. For the character-valued entries it is the whole ballgame:Verified against
mainat 7095e23, driving a realPromptSessionover a pipe input:Hello World\x1b[27;2;72~ello \x1b[27;2;87~orldHello WorldXonbcXbcXbc(unchanged)So the symptom in #87390 — capital letters printing
[27;2;<code>~on Ghostty — is still live onmainfor anyone on an allowlisted terminal. Reported independently by a Ghostty 1.3.2 user this morning.Related Issue
Refs #87511, #87390, #86866, #87631, #87637.
Not a duplicate of #87637 (Cyrillic / all cased scripts): that PR extends the same mapping table and inherits the same gap — its assertions are also key-level, so it will report green while Cyrillic capitals still leak. This fix is orthogonal and makes both work.
Type of Change
Changes Made
hermes_cli/pt_input_extras.py— new_install_literal_key_data_patch(): narrowsKeyPress.datato the character for character-valued table entries. Called at the end ofinstall_modify_other_keys_aliases(), so it covers Shift+letter, Shift+Space and the keypad digits. Idempotent, guarded bytry/exceptand a class-level flag, and a no-op if prompt_toolkit's internals move.tests/cli/test_modify_other_keys_insert_text.py— 14 tests.Why the tests assert on submitted text
_parse()-style assertions compareKeyPress.key, which was already correct before this fix — that is precisely why this survived #87511's 160 tests, and why #87637 does not catch it either. These tests run a realPromptSessionovercreate_pipe_input()and assert on the string the user would have submitted. 9 of the 14 fail on unpatchedmain; the 5 that pass are the guard tests (Ctrl combos, named keys, plain typing, bracketed paste, Shift+Enter aliases).How to Test
pytest tests/cli/test_modify_other_keys_insert_text.py -q→ 14 passed.git stashthept_input_extras.pychange and re-run → 9 failed, 5 passed.Checklist
Code
pytest tests/cli -q— 1214 passed, 3 failed, 9 skipped. The 3 failures (test_exit_watchdog_signal_armx2,test_resume_quiet_stderr) reproduce identically on pristinemain(1200 passed, same 3 failures).Documentation & Housekeeping
keyvsdatadistinctioncli-config.yaml.example— N/ACONTRIBUTING.md/AGENTS.md— N/A🤖 Generated with Claude Code