Skip to content

Fix llm chat cursor-key binding on Windows - #1669

Open
dchaudhari7177 wants to merge 1 commit into
simonw:mainfrom
dchaudhari7177:fix/1639-windows-readline-bindings
Open

dchaudhari7177 wants to merge 1 commit into
simonw:mainfrom
dchaudhari7177:fix/1639-windows-readline-bindings

Conversation

@dchaudhari7177

Copy link
Copy Markdown

Refs #1639. I'm on Windows, so I could check this against a real pyreadline3 rather than reason about it — and what I found is a bit different from the issue's diagnosis, so the fix is shaped differently too.

What's actually happening

The Windows branch never did anything. It called:

readline.parse_and_bind("bind -x '\e[D: backward-char'")

pyreadline3 matches a binding line against \s*(\S+)\s*:\s*([-a-zA-Z]+)\s*$. bind -x '...' doesn't match, so it's logged as unparseable and dropped. On pyreadline3 3.5.4 the call returns without raising and binds nothing — so the arrow keys were never being configured on Windows, silently.

The GNU form isn't the answer either. pyreadline3 names keys left/right, not escape sequences:

>>> readline.parse_and_bind("\e[D: backward-char")
IndexError: Not a valid key: '\e[d'

And nothing needs binding there. pyreadline3's default keymap already has what the code was trying to add:

(False, False, False, 'left')  -> backward_char
(False, False, False, 'right') -> forward_char

The fix

Drop the Windows branch rather than correct it, with a comment recording why — otherwise the natural next move is to "fix" it to left: backward-char, which would be redundant with pyreadline3's own defaults.

Keep the POSIX bindings, wrapped. Binding cursor keys is a convenience; a readline shim that doesn't understand the syntax shouldn't stop llm chat from starting. That's the failure the issue reports, and the guard covers it regardless of which shim is involved — I couldn't reproduce a raise with pyreadline3 specifically, so I'd rather make the call non-fatal than guess at the reporter's environment.

Extracted into _bind_cursor_keys() so it's testable without invoking the whole chat command.

Tests

Three, all of which fail without the change:

FAILED TestBindCursorKeys::test_windows_binds_nothing
FAILED TestBindCursorKeys::test_posix_binds_the_arrow_keys
FAILED TestBindCursorKeys::test_a_readline_that_rejects_the_syntax_does_not_stop_chat
pytest tests/test_chat.py    5 passed, 8 xpassed
ruff check llm tests         All checks passed!
black --check                2 files would be left unchanged

Incidentally: tests/test_chat.py is decorated @pytest.mark.xfail(sys.platform == "win32"), and 8 of those xpass on this machine. Not touching that here, but it may be worth revisiting whether the Windows xfail is still earned.

The Windows branch called

    readline.parse_and_bind("bind -x '\e[D: backward-char'")

which never did anything. pyreadline3 matches a binding line against
`\s*(\S+)\s*:\s*([-a-zA-Z]+)\s*$`; `bind -x '...'` does not match, so it was
logged as unparseable and dropped. Verified on Windows with pyreadline3
3.5.4: the call returns without raising and binds nothing.

The GNU escape-sequence form is not the answer either. pyreadline3 names
keys `left`/`right`, so the escape sequence is rejected:

    >>> readline.parse_and_bind("\e[D: backward-char")
    IndexError: Not a valid key: '\e[d'

And nothing needs binding there anyway: pyreadline3's default keymap
already has

    (False, False, False, 'left')  -> backward_char
    (False, False, False, 'right') -> forward_char

So the Windows branch is removed rather than corrected, with a comment
recording why, so it does not get "fixed" back later.

The remaining POSIX call is wrapped. Binding cursor keys is a convenience,
and a readline shim that does not understand the syntax should not stop
`llm chat` from starting -- which is the failure the issue reports.

Three tests: Windows binds nothing, POSIX binds both arrows, and a
parse_and_bind that raises does not propagate. All three fail without the
change.

tests/test_chat.py: 5 passed, 8 xpassed. black clean.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant