Skip to content

fix(cli): dispatch /indicator to set the busy-indicator style (#50618) - #51178

Closed
dongjiang1989 wants to merge 1 commit into
NousResearch:mainfrom
dongjiang1989:fix-indicator
Closed

fix(cli): dispatch /indicator to set the busy-indicator style (#50618)#51178
dongjiang1989 wants to merge 1 commit into
NousResearch:mainfrom
dongjiang1989:fix-indicator

Conversation

@dongjiang1989

@dongjiang1989 dongjiang1989 commented Jun 23, 2026

Copy link
Copy Markdown

Summary

Closes: #50618

The /indicator command was registered in COMMAND_REGISTRY, listed in /help, offered by tab-completion, recommended by the tips system, and even documented in config.py ("Live-swappable via /indicator <style>")
— but process_command had no branch for it. Typing /indicator (or /indicator emoji) fell through to the prefix matcher and printed "Unknown command: /indicator", leaving no CLI way to change
display.tui_status_indicator short of hand-editing config.yaml.

The styles feature (#17150) shipped the registry entry, config key, tip, and TUI/JSON-RPC wiring, but never added the CLI handler. This adds it.

What does this PR do?

Wires the advertised /indicator slash command to a real handler so it sets the TUI busy-indicator style from the CLI. /indicator (or /indicator status) shows the current style; /indicator <kaomoji|emoji|unicode|ascii> validates the choice and persists it to display.tui_status_indicator — the same key the TUI reads — so the next render picks it up. Unknown styles print usage and change nothing.

Related Issue

Fixes #50618

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

  • Add the elif canonical == "indicator" dispatch branch in process_command, so the command resolves and the REPL stays alive.

hermes_cli/cli_commands_mixin.py

  • Add _handle_indicator_command (status view, style validation, persistence via save_config_value("display.tui_status_indicator", ...) with in-memory mirror), modeled on _handle_busy_command.

tests/cli/test_indicator_command.py

  • Dispatch wiring, handler behavior, and registry consistency coverage.

How to Test

  1. scripts/run_tests.sh tests/cli/test_indicator_command.py — 8 tests pass.
  2. In the CLI, run /indicator emoji — prints a saved-to-config confirmation (previously printed "Unknown command: /indicator").
  3. Run /indicator — shows the current style; /indicator rainbow — prints usage and leaves the stored value untouched.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the test suite and the affected tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings) — handler docstring added
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A (key already existed)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture — N/A
  • I've considered cross-platform impact — N/A (no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #41869 — the _handle_indicator_command handler added here is byte-for-byte identical to that earlier open PR. There's also a third competing open impl (#40047) and the canonical wiring issue is #22960. /indicator-dispatch cluster: a maintainer should pick one of #41869 / #40047 / this and close the rest.

@dongjiang1989

Copy link
Copy Markdown
Author

Thanks @alt-glitch

Based on PR #41869, the following modifications were made:

Changes

  1. cli.py (lines 8170-8171)

Added the indicator branch in the process_command() method:

  elif canonical == "indicator":
      self._handle_indicator_command(cmd_original)                                                                                                                                                                           
  1. hermes_cli/cli_commands_mixin.py (lines 2382-2419)

Added the _handle_indicator_command method:

  • Supports /indicator or /indicator status to display the current style
  • Supports /indicator kaomoji|emoji|unicode|ascii to set the style
  • Validates input and persists to the display.tui_status_indicator config key
  • Uses the same config key as the TUI gateway to ensure consistency
  1. tests/cli/test_indicator_command.py

Created a comprehensive test file including:

  • TestIndicatorDispatch: Tests command dispatch routing
  • TestHandleIndicatorCommand: Tests handler logic
  • TestIndicatorRegistry: Tests command registration

@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: Approved

Adds /indicator command dispatch to set the busy-indicator style (3 files, 189 additions). Well-scoped feature addition with clear CLI interaction. No security concerns.


Reviewed by Hermes Agent

@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 tracing the advertised command through the classic CLI path. The premise is confirmed on current main: /indicator is registered at hermes_cli/commands.py:163 and documented at website/docs/reference/slash-commands.md:82, but cli.py:8917-8921 dispatches /voice and /busy then falls through without an indicator case.

Problems

  • hermes_cli/cli_commands_mixin.py:2398 introduces another independent style list alongside tui_gateway/server.py:1896 and hermes_cli/commands.py:163. A later style addition can drift so that the TUI accepts it while the classic CLI rejects it.
  • tests/cli/test_indicator_command.py:140-142 snapshots the current four-value catalog rather than checking that the handler accepts every registered subcommand.

Suggested changes

  • Share the Python style list between the CLI handler and TUI gateway, and test handler behavior for the registry's subcommands.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/cli_commands_mixin.py Outdated
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 15, 2026
@dongjiang1989

Copy link
Copy Markdown
Author
  • Share the Python style list between the CLI handler and TUI gateway, and test handler behavior for the registry's subcommands.

Thanks @teknium1
Fixed
Please re-check it

…search#50618)

The /indicator command was registered in COMMAND_REGISTRY, listed in
/help, offered by tab-completion, recommended by the tips system, and
even documented in config.py — but it had no actual handler. Running
/indicator in the CLI produced "Unknown command: indicator".

Add _handle_indicator_command to CLICommandsMixin that:
- Shows the current indicator style when called with no args or "status"
- Validates the requested style against the shared INDICATOR_STYLES
  allowlist (ascii | emoji | kaomoji | unicode)
- Persists the choice to display.tui_status_indicator in config.yaml
  via the existing save_config_value helper
- Falls back to session-only when config save fails

The indicator-style allowlist is defined once in hermes_constants as
INDICATOR_STYLES + DEFAULT_INDICATOR_STYLE and imported by all three
consumers (CLI handler, command registry, TUI gateway config handler),
preventing drift between the TUI and CLI validation.

Also adds tests/cli/test_indicator_command.py covering dispatch,
validation, persistence, and registry integration.

Signed-off-by: dongjiang <dongjiang1989@126.com>
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Merged via #75874 — your commit(s) were cherry-picked onto current main with your authorship preserved in git history (rebase merge). Thanks for the contribution!

@teknium1 teknium1 closed this Aug 1, 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 comp/tui Terminal UI (ui-tui/ + tui_gateway/) duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/indicator does not work on mac

4 participants