Skip to content

fix(cli): keep slash confirmation modal on app loop - #32808

Closed
stephenschoettler wants to merge 1 commit into
NousResearch:mainfrom
stephenschoettler:fix/22958-slash-confirm-modal-app-loop
Closed

fix(cli): keep slash confirmation modal on app loop#32808
stephenschoettler wants to merge 1 commit into
NousResearch:mainfrom
stephenschoettler:fix/22958-slash-confirm-modal-app-loop

Conversation

@stephenschoettler

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the destructive slash confirmation modal for /clear, /new, /reset, and /undo when those commands are processed from the CLI worker thread on POSIX terminals.

The existing code falls back to raw input() whenever _prompt_text_input_modal() is called off the main thread. That is the broken path in #22958: prompt_toolkit still owns stdin, so 1, 2, or 3 can leak into the composer instead of reaching the confirmation prompt.

This keeps the Windows stdin fallback for #30768, but on POSIX with a running prompt_toolkit app it schedules modal open and close work onto app.loop.call_soon_threadsafe(...) and waits on the existing modal response queue.

This is intentionally narrower than #25404. That PR changes _prompt_text_input() in a mixed-scope branch, but does not fix the worker-thread _prompt_text_input_modal() fallback that handles destructive slash confirmations.

Related Issue

Fixes #22958

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

  • cli.py
    • Keeps no-app and native Windows paths on the existing stdin fallback.
    • Removes the POSIX worker-thread raw input() fallback while prompt_toolkit owns stdin.
    • Schedules slash confirmation modal open and close work on the prompt_toolkit app loop via call_soon_threadsafe.
    • Adds small normalization in the modal display renderer for title, detail, choices, and selected values used by the modal state.
  • tests/cli/test_slash_confirm_windows.py
    • Updates the non-main POSIX regression to assert that the worker schedules modal work on the app loop and does not call raw stdin.
    • Keeps Windows stdin fallback coverage.
    • Keeps main-thread modal, no-app fallback, empty choices, and destructive slash normalization coverage.
    • Adds coverage for app-loop scheduling failure returning None without falling back to raw stdin.

How to Test

  1. Run the focused regression tests:
    • /home/w0lf/hermes-agent/venv/bin/python -m pytest tests/cli/test_slash_confirm_windows.py tests/cli/test_destructive_slash_inline_skip_e2e.py -q -o 'addopts='
  2. Compile the changed Python files:
    • /home/w0lf/hermes-agent/venv/bin/python -m py_compile cli.py tests/cli/test_slash_confirm_windows.py
  3. Check whitespace:
    • git diff --check
  4. Check Windows footguns on changed files:
    • /home/w0lf/hermes-agent/venv/bin/python scripts/check-windows-footguns.py cli.py tests/cli/test_slash_confirm_windows.py
  5. Manual PTY smoke:
    • Start Hermes in a tmux session from this branch with a temporary HERMES_HOME.
    • Enter /new.
    • Verify the modal renders.
    • Enter 1.
    • Verify the CLI starts a new session and 1 does not appear as a chat message.

Local validation performed on Linux:

/home/w0lf/hermes-agent/venv/bin/python -m pytest tests/cli/test_slash_confirm_windows.py tests/cli/test_destructive_slash_inline_skip_e2e.py -q -o 'addopts='
12 passed, 1 warning in 0.74s

/home/w0lf/hermes-agent/venv/bin/python -m py_compile cli.py tests/cli/test_slash_confirm_windows.py
passed, emitted pre-existing SyntaxWarning: cli.py:10857: 'return' in a 'finally' block

git diff --check
passed

/home/w0lf/hermes-agent/venv/bin/python scripts/check-windows-footguns.py cli.py tests/cli/test_slash_confirm_windows.py
✓ No Windows footguns found (2 file(s) scanned).

tmux PTY smoke with temporary HERMES_HOME
/new modal rendered, choice 1 submitted, CLI printed "New session started!", and the composer was clean afterward.

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: Linux 7.0.9-arch2-1, tmux PTY smoke

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

For New Skills

N/A

Screenshots / Logs

│ ❯ [1] Approve Once — proceed this time only                            │
│   [2] Always Approve — proceed and silence this prompt permanently     │
│   [3] Cancel — keep current conversation                               │
│                                                                        │
│ Type 1/2/3 or use ↑/↓ then Enter. ESC/Ctrl+C cancels.                  │
╰────────────────────────────────────────────────────────────────────────╯

⚙️  /new
(^_^)v New session started!
❯

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround labels May 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #32387 — both fix the POSIX worker-thread slash confirmation modal freeze (same root cause from #32013/#32383) using call_soon_threadsafe on the prompt_toolkit app loop. Same files changed: cli.py + tests/cli/test_slash_confirm_windows.py. #32387 was opened first.

Related: #32383 (issue), #22958 (original issue), #32013 (merged PR that introduced the regression).

@stephenschoettler

Copy link
Copy Markdown
Contributor Author

Thanks - agreed. I missed #32387/#32383 when checking only #25404 and the older #22958 thread.

This is duplicative of #32387 for the main fix path, so I am closing this one rather than adding PR noise. The only small delta here is that this branch returns None instead of falling back to raw stdin if app-loop scheduling fails off the main thread; if maintainers think that guard/test is useful, feel free to salvage it, but #32387 looks like the right earlier narrow fix to carry forward.

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 P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: /clear, /new, /reset, /undo confirmation prompt cannot be answered — keystrokes leak into chat composer

2 participants