Skip to content

fix(cli): type the mapped key, not the raw sequence, for printable aliases - #94932

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-94921
Closed

fix(cli): type the mapped key, not the raw sequence, for printable aliases#94932
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-94921

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Under modifyOtherKeys=2 (pushed for Ghostty since #87630), Shift+letter arrives as ESC[27;2;<cp>~. The alias table installed by install_modify_other_keys_aliases() maps it to a plain string ('F') in ANSI_SEQUENCES — but prompt_toolkit's Vt100Parser always attaches the raw matched bytes as KeyPress.data, and TextArea's self-insert catch-all types event.data. So the input buffer received the literal sequence [27;2;70~ instead of F. As the reporter proved programmatically, any ANSI_SEQUENCES value that is a plain printable string is broken by construction: stock prompt_toolkit values are Keys enums whose bindings ignore data — only our aliases use plain strings (#94921).

Affected surfaces (all plain-string alias values): Shift+letter A–Z (both codepoint spellings), Shift+Space, and the kitty lock-bit plain-key twins (e.g. Space with NumLock on, ESC[32;129u).

The fix installs a narrow Vt100Parser._call_handler wrapper alongside the alias table (the same monkey-patch pattern the module already uses for ANSI_SEQUENCES, including idempotency and cache hygiene): when the matched key is a plain str, the raw data is escape-prefixed, and the key is printable, the KeyPress carries the key itself as data — which is exactly what ordinary typing already produces (key == data). Everything else passes through untouched:

  • normal typing (key == data already) — unchanged
  • Keys-enum aliases (Ctrl+letter etc.) — data stays the raw sequence, their bindings ignore data
  • Alt-style tuple recursion (whose second element carries "" data) — unchanged (pinned by a test)

Related Issue

Fixs #94921

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/pt_input_extras.py — new _install_printable_alias_data_fix() (idempotent _call_handler wrapper rewriting data→key for the plain-string alias shape), invoked from install_modify_other_keys_aliases() so every installer of the broken table gets the fix.
  • tests/cli/test_modify_other_keys_aliases.py — new _parse_presses helper returning full KeyPress objects (the existing _parse only checked .key, which is why this slipped through); 26-z parametrized data pin, Shift+Space and lock-bit-space data pins, plain-typing/Ctrl-enum/Alt-tuple passthrough pins, and the reporter's end-to-end pipe-input TextArea repro (Shift+F types F, plain q still types q).

How to Test

  1. pytest tests/cli/test_modify_other_keys_aliases.py tests/cli/test_cli_terminal_shortcuts.py tests/cli/test_cli_shift_enter_newline.py tests/cli/test_cli_cmd_backspace.py -q — should pass (244 + 20).
  2. Observed result: with this PR's source reverted (plain main), the new data tests fail 29/31 — every Shift+letter KeyPress carries the raw escape sequence as data, and the end-to-end TextArea repro inserts [27;2;70~; with the change, the letter is typed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

From the issue (before): pressing Shift+F at the hermes prompt in Ghostty inserted [27;2;70~. The reporter's programmatic repro (feeding \x1b[27;2;70~ into a pipe-input TextArea) asserted area.text == '\x1b[27;2;70~' — now it yields 'F', verified end-to-end in the new test.

…iases

Vt100Parser always attaches the raw matched bytes as KeyPress.data and
TextArea's self-insert catch-all types event.data, so an ANSI_SEQUENCES
value that is a plain printable string can never work: under
modifyOtherKeys=2 (pushed for Ghostty), Shift+letter arrived as
ESC[27;2;70~ and the alias installed by install_modify_other_keys_aliases
in NousResearch#87630 keyed it to 'F' — but the buffer received the raw sequence
'[27;2;70~' instead of the letter. Stock prompt_toolkit values are Keys
enums whose bindings ignore data; only our aliases use plain strings, so
every Shift+letter, Shift+Space, and lock-bit plain-key twin was broken
by construction (NousResearch#94921).

Rewrite the data to the key for exactly that shape — plain-str key +
escape-prefixed raw data + printable key — in a Vt100Parser._call_handler
wrapper installed alongside the alias table (same monkey-patch pattern
the module already uses for ANSI_SEQUENCES). Normal typing (key == data),
Keys-enum matches, and Alt-style tuple recursion (whose second element
carries "" data) pass through untouched.

Fixes NousResearch#94921
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Aug 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #88097: both repair the same Vt100Parser KeyPress.data mechanism for printable ANSI aliases. #88097 is the earlier, broader open repair.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Confirmed duplicate — closing in favor of #88097.

The triage is right that this is the earlier, broader repair. I independently reviewed @webtecnica's PR: opened 8 days earlier (Aug 17 vs Aug 25) and it patches the exact same mechanism — Vt100Parser._call_handler carrying the raw escape text in KeyPress.data when ANSI_SEQUENCES maps an extended sequence to a single plain character, so self-insert leaks CSI bytes into the buffer. Same guard shape (plain-char key + insert_text starting with \x1b → rewrite insert_text = key), same idempotency flag pattern, plus the install wiring in the CLI bootstrap. Their test coverage is equivalent or wider (the keypad-digit PUA case is one mine didn't have).

For whoever lands #88097, two cases from my parameterized coverage are worth absorbing: a full a–z Shift+letter matrix (both CSI u \x1b[97;2u-style and modifyOtherKeys \x1b[27;2;97~-style encodings), and CapsLock-modified space (\x1b[57444u kitty PUA), which pins that lock-bit space maps to a plain ' ' like Shift+Space — plus reverse pins that Keys-enum presses (e.g. Ctrl-A) and Alt tuples keep their non-plain data untouched. My branch stays available at liuhao1024/hermes-agent:liuhao/cron-bugfix-94921 if the tests are useful.

The underlying issues (#88071 here, #94921 on my side) look like the same report family; maintainer judgment on which issue stays canonical.

@liuhao1024 liuhao1024 closed this Aug 25, 2026
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 duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants