Skip to content

fix(cli): preserve Ctrl-J newline in CR/LF terminals - #32860

Open
dso2ng wants to merge 2 commits into
NousResearch:mainfrom
dso2ng:fix/cli-crlf-terminal-ctrl-j-newline
Open

fix(cli): preserve Ctrl-J newline in CR/LF terminals#32860
dso2ng wants to merge 2 commits into
NousResearch:mainfrom
dso2ng:fix/cli-crlf-terminal-ctrl-j-newline

Conversation

@dso2ng

@dso2ng dso2ng commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve c-j as a newline key in known local terminals that distinguish Enter (CR) from Ctrl-J/Ctrl+Enter (LF)
  • add TERM_PROGRAM-based coverage for WezTerm and WarpTerminal plus Kitty's KITTY_WINDOW_ID
  • keep the existing fallback for unrecognized local POSIX/thin PTYs where plain Enter may arrive as LF and still needs to submit

Why

On Linux WezTerm, Ctrl-J reaches prompt_toolkit as bare LF (c-j). Hermes currently binds c-j to submit on local POSIX terminals, so Ctrl-J submits the turn instead of inserting a newline.

The existing #22379 fix already avoids binding c-j to submit in Windows, WSL, SSH, and Windows Terminal environments where LF represents a distinct newline-intent keystroke. This PR extends that same environment-gated approach to known local CR/LF-distinguishing terminal environments instead of enabling blanket POSIX behavior.

Related work

Test plan

  • python -m pytest tests/cli/test_ctrl_enter_newline.py -o addopts= -q

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: #23921 (Warp terminal Shift+Enter regression). This PR generalizes the terminal detection to also cover WezTerm and Kitty via TERM_PROGRAM / KITTY_WINDOW_ID env vars, extending the same _preserve_ctrl_enter_newline() predicate that #22777 introduced.

@dso2ng
dso2ng force-pushed the fix/cli-crlf-terminal-ctrl-j-newline branch from 862c602 to ee4b548 Compare May 29, 2026 11:17
@dso2ng

dso2ng commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Refresh update: I rebased this branch onto current origin/main and resolved the cli.py / tests/cli/test_ctrl_enter_newline.py conflicts while preserving the newer Ghostty handling and the WezTerm/Kitty/Warp Ctrl-J newline detection.

Updated head: ee4b5480f

Targeted local verification on the updated head:

  • git diff --check origin/main...HEAD -> passed
  • python -m py_compile cli.py tests/cli/test_ctrl_enter_newline.py -> passed
  • python -m pytest tests/cli/test_ctrl_enter_newline.py -o 'addopts=' -q -> 12 passed
  • non-ASCII diff guard -> non_ascii_added_lines=0

GitHub Actions is still blocked at the fork workflow approval gate (action_required, zero jobs started). Once a maintainer approves/runs the workflows, I can follow up on any real CI failures.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for extending the existing environment-gated classic-CLI behavior. Current main still binds c-j to submit when _preserve_ctrl_enter_newline() is false (cli.py:3189-3190), while its predicate has no WezTerm, Warp, or Kitty marker (cli.py:3149-3171), so the PR targets a live path.

Problems

  • cli.py:2486 adds WarpTerminal to the new allowlist, but tests/cli/test_ctrl_enter_newline.py:66-77 only adds WezTerm and Kitty coverage. The Warp branch would be unprotected.

Suggested changes

  • Add a TERM_PROGRAM=WarpTerminal regression case; ideally verify the resulting _bind_prompt_submit_keys() binding as well as the predicate result.

Automated hermes-sweeper review.

Comment thread cli.py
@@ -2483,16 +2483,19 @@ def _patched_vt100_feed(self_parser, data: str) -> None:
)


_RAW_LF_NEWLINE_TERM_PROGRAMS = {"WezTerm", "WarpTerminal"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WarpTerminal is introduced here but has no matching regression fixture: the added tests cover WezTerm and Kitty only. Please add a WarpTerminal environment case so every allowlisted TERM_PROGRAM value is covered.

@dso2ng
dso2ng force-pushed the fix/cli-crlf-terminal-ctrl-j-newline branch from ee4b548 to 94b4ef5 Compare July 13, 2026 14:36
@dso2ng

dso2ng commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed on current head 94b4ef5b9dd1c57dc9eab4cfbff007b0f777f3a9.

  • Added a TERM_PROGRAM=WarpTerminal regression case for the newly allowlisted value.
  • The regression checks both the predicate and the effective prompt binding: c-m remains submit while c-j stays unbound for the Ctrl+J/Ctrl+Enter newline handler.

Targeted verification:

python -m py_compile cli.py tests/cli/test_ctrl_enter_newline.py
python -m pytest tests/cli/test_ctrl_enter_newline.py tests/cli/test_cli_init.py::TestPromptToolkitTerminalCompatibility::test_lf_enter_binds_to_submit_handler_posix -o 'addopts=' -q
14 passed in 0.34s

git diff --check origin/main...HEAD
non_ascii_added_lines=0

Please re-review when convenient.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants