Skip to content

[codex] fix sessions browse raw arrow navigation - #54711

Open
hzhaoy wants to merge 4 commits into
NousResearch:mainfrom
hzhaoy:fix/session-browse-raw-arrow-keys
Open

hzhaoy wants to merge 4 commits into
NousResearch:mainfrom
hzhaoy:fix/session-browse-raw-arrow-keys

Conversation

@hzhaoy

@hzhaoy hzhaoy commented Jun 29, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes raw CSI arrow-key handling in hand-rolled Hermes curses pickers. In affected terminals, Up/Down can arrive as raw escape sequences (ESC [ A / ESC [ B) instead of translated curses.KEY_UP / curses.KEY_DOWN constants. The old loops treated the leading ESC byte as cancel/exit before reading the CSI continuation.

This PR now routes both affected pickers through the existing shared curses menu decoder:

  • hermes sessions browse
  • hermes plugins composite toggle picker

The change preserves existing text-filter behavior in the session browser and existing plugin picker behavior for page/home/end, toggle, select, and cancel.

Related Issue

N/A. Reproduced locally with scripts/keystroke_diagnostic.py: Up emitted \x1b[A, Down emitted \x1b[B.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/main.py: route non-printable keys in _session_browse_picker through the existing curses escape-sequence decoder, while keeping printable filter input, q, Backspace, Enter, and lone Esc semantics intact.
  • tests/hermes_cli/test_session_browse.py: add regression coverage for raw ESC [ B and ESC [ A arrow sequences, and make the fake curses input model support short escape continuation timeouts.
  • hermes_cli/plugins_cmd.py: route the composite plugins picker through the same shared decoder so raw CSI arrows navigate instead of exiting, while preserving PageUp/PageDown/Home/End handling on raw curses keys.
  • tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py: add regression coverage proving raw ESC [ B toggles the second plugin and raw ESC [ A wraps to the last plugin instead of exiting.

How to Test

  1. scripts/run_tests.sh tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_curses_arrow_keys.py
  2. $HOME/.hermes/hermes-agent/venv/bin/python -m ruff check hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py hermes_cli/main.py
  3. $HOME/.hermes/hermes-agent/venv/bin/python scripts/check-windows-footguns.py hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py
  4. git diff --check -- hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py

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 (not run; targeted tests above passed)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS, targeted automated checks

Documentation & Housekeeping

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

Screenshots / Logs

Relevant verification output:

=== Summary: 3 files, 58 tests passed, 0 failed (100% complete)
All checks passed!
✓ No Windows footguns found (4 file(s) scanned).

Session browse uses a hand-rolled curses loop for live filtering, but some terminals deliver arrows as raw CSI bytes. Reusing the existing curses menu decoder preserves prompt text behavior while making raw ESC [ A/B navigate like translated curses key constants.

Constraint: Terminal arrow keys may arrive as raw CSI/SS3 sequences even under curses.
Rejected: Switch the browser to the shared menu driver | it would be a wider rewrite of the live filter/table UI.
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep printable search handling before nav decoding so j/k/space continue to filter rather than becoming menu shortcuts.
Tested: scripts/run_tests.sh tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_curses_arrow_keys.py
Tested: python -m ruff check hermes_cli/main.py tests/hermes_cli/test_session_browse.py
Tested: python scripts/check-windows-footguns.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py
Tested: git diff --check -- hermes_cli/main.py tests/hermes_cli/test_session_browse.py
Not-tested: Full repository test suite.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 29, 2026
@hzhaoy
hzhaoy marked this pull request as ready for review June 29, 2026 08:17

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: LGTM

Fixes raw CSI arrow key navigation in the sessions browse curses picker. Uses _decode_menu_key to translate raw escape sequences (ESC [ A/B) into NAV_UP/NAV_DOWN actions, making navigation work in terminals that don't emit curses.KEY_UP/KEY_DOWN constants.

Looks Good

  • Correct ordering: check backspace and printable chars before key decode
  • Tests use FakeStdscr that simulates real terminal behavior (timeout-based getch)
  • Raw CSI sequence tests confirm fix works

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression fix. The reported session-browser bug is present on current main: hermes_cli/main.py:1083-1104 handles raw ESC as cancellation before it can read a CSI continuation. The proposed _decode_menu_key integration matches the existing decoder in hermes_cli/curses_ui.py:295-342, and the added CSI tests cover the stated behavior.

Problems

  • A sibling hand-rolled picker remains vulnerable: hermes_cli/plugins_cmd.py:1532-1539 only handles translated KEY_UP/KEY_DOWN, while :1615-1618 treats raw ESC as exit despite advertising arrow navigation at :1440.

Suggested changes

  • Please cover that selector with the same shared decoder in a focused follow-up (or extend this change if desired), including raw ESC [ A/B regression coverage.

Automated hermes-sweeper review.

@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 15, 2026
The plugins composite picker had the same hand-rolled curses input gap as the session browser: terminals that surface arrows as raw CSI bytes would hit the ESC exit path before navigation could happen. Route its key dispatch through the shared curses decoder while preserving page/home/end handling and existing toggle/select behavior.

Constraint: Keep the composite plugin picker shape unchanged for this focused PR follow-up.
Rejected: Convert the picker to the generic curses menu driver | broader UI rewrite than the review comment requires.
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep PageUp/PageDown/Home/End checks on the raw key because the shared decoder only normalizes common menu actions.
Tested: scripts/run_tests.sh tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_curses_arrow_keys.py
Tested: python -m ruff check hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py hermes_cli/main.py
Tested: python scripts/check-windows-footguns.py hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py
Tested: git diff --check -- hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py
Not-tested: Full repository test suite.

hzhaoy commented Jul 15, 2026

Copy link
Copy Markdown
Author

Addressed the sweeper follow-up for the sibling hermes plugins composite picker.

Changes added in db151519f:

  • routes _run_composite_ui() through the shared curses _decode_menu_key() for raw CSI arrows
  • preserves existing PageUp/PageDown/Home/End raw-key handling
  • adds regression coverage for raw ESC [ B toggling the second plugin and raw ESC [ A wrapping to the last plugin

Validation rerun:

  • scripts/run_tests.sh tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_curses_arrow_keys.py
  • python -m ruff check hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py hermes_cli/main.py
  • python scripts/check-windows-footguns.py hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py
  • git diff --check -- hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py

Resolve the plugins composite menu test conflict by keeping upstream canonical-key coverage and the raw CSI arrow regression coverage.

Constraint: Preserve already-reviewed PR commits while clearing GitHub's merge-conflict state

Confidence: high

Scope-risk: narrow

Tested: scripts/run_tests.sh tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_curses_arrow_keys.py

Tested: python -m ruff check hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py tests/hermes_cli/test_session_browse.py hermes_cli/main.py

Tested: python scripts/check-windows-footguns.py hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py

Tested: git diff --check -- hermes_cli/plugins_cmd.py tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py hermes_cli/main.py tests/hermes_cli/test_session_browse.py

Not-tested: Full repository test suite
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have 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.

4 participants