Skip to content

fix(cli): make string-valued ANSI mappings insert the mapped character - #94076

Open
salch-cred wants to merge 1 commit into
NousResearch:mainfrom
salch-cred:fix/shift-letter-csi-self-insert
Open

fix(cli): make string-valued ANSI mappings insert the mapped character#94076
salch-cred wants to merge 1 commit into
NousResearch:mainfrom
salch-cred:fix/shift-letter-csi-self-insert

Conversation

@salch-cred

Copy link
Copy Markdown
Contributor

Fixes #92343

What & why

After #87511 mapped the Shift+letter CSI sequences in ANSI_SEQUENCES, the parser emits the right key — but Vt100Parser._call_handler always passes the raw matched byte sequence as KeyPress.data. The default self-insert binding inserts event.data, so Shift+letter still leaked [27;2;97~ into the prompt buffer on Ghostty.

Stock prompt_toolkit has zero plain-string mappings (all 180 string values in ANSI_SEQUENCES are Keys members — verified against 3.0.52), so the only plain-string values are Hermes' own Shift+letter table. That gives the fix a perfect blast radius:

install_vt100_str_key_data_fix() patches _call_handler so a plain-string mapped value becomes its own data payload. Keys members are str subclasses and are excluded explicitly — every Keys.* mapping keeps its raw-byte data. Wired into the existing startup install block in cli.py; idempotent.

How to test

scripts/run_tests.sh tests/hermes_cli/test_pt_input_extras_str_key_data.py -q

Tests drive a real Vt100Parser with Hermes' actual mappings installed: without the fix, feeding ESC[27;2;97~ yields KeyPress("A", "\x1b[27;2;97~") (the leak); with it, KeyPress("A", "A"). Also covered: Keys.* data payloads unchanged, install idempotency.

Manual repro: Ghostty + hermes → type Shift+M. Before: [27;2;109~ lands in the buffer. After: M.

Platforms tested

  • Windows 11, Python 3.12: new suite 4/4.
  • Parser semantics are host-independent; the reported terminal was macOS/Ghostty, but the data-payload defect is in the parser, not the terminal.

@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 labels Aug 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #87785 and #88097: all normalize raw VT100 KeyPress.data for character-valued aliases. #88097 covers a broader set of extended-key aliases; these are competing implementations.

Vt100Parser._call_handler always passes the raw matched byte sequence as
KeyPress.data. For Keys.* targets that is harmless (bound handlers ignore
data), but for mappings whose value is a plain string - Hermes' Shift+
letter table: ESC[27;2;97~ -> "A" - the default self-insert binding
inserts event.data, i.e. the literal escape text. Shift+letter therefore
still leaked "[27;2;97~" into the prompt buffer on Ghostty even though
the sequence itself was decoded correctly (NousResearch#92343): the parser emitted
the right key, but the data payload the insert path reads was never the
mapped character.

Add install_vt100_str_key_data_fix(), which patches _call_handler so a
plain-string mapped value becomes its own data payload. Keys members are
str subclasses, so they are excluded explicitly - every Keys.* mapping
keeps its raw-byte data, and stock prompt_toolkit has zero plain-string
mappings (all 180 string values are Keys members), so the blast radius is
exactly Hermes' own tables. Wired into the existing startup install block
in cli.py; idempotent.
@salch-cred
salch-cred force-pushed the fix/shift-letter-csi-self-insert branch from 78df404 to 8f5b5cb Compare August 24, 2026 19:09
@salch-cred

Copy link
Copy Markdown
Contributor Author

Rebased on latest main — tests 4/4 passing. Thanks @isimoesg-amzn for the thorough independent verification (the A/B probe and the 384-test regression run are exactly the confidence this needed). The blast-radius argument holding on 3.0.52 is good to have on record.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Aug 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #88097 — both patch Vt100Parser._call_handler to normalize raw CSI KeyPress.data; #88097 covers the broader key-alias set.

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Aug 24, 2026
@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Aug 25, 2026
@salch-cred

Copy link
Copy Markdown
Contributor Author

Saw the triage flag about #88097. My PR takes a different approach: instead of expanding the key-alias set, it patches the data payload at the parser level so ANY plain-string mapped value (not just the ones #88097 covers) inserts the mapped character. If #88097 is preferred, happy to consolidate — but the two fixes operate at different layers and could complement each other.

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.

Classic CLI: Shift+letter still leaks literal CSI text after #87511 — ANSI_SEQUENCES mapping is defeated by self-insert pasting KeyPress.data

2 participants