Conversation
Every non-gateway_only CommandDef must have a handler branch in process_command(). Without this gate, commands can be added to the registry — appearing in /help, autocomplete, and platform surfaces — but returning "Unknown command" at runtime because no one wired the dispatch. Currently FAILS on main due to two pre-existing gaps: indicator — NousResearch#22960, NousResearch#50618 (5 open PRs) whoami — NousResearch#51190 This PR is blocked until those handlers are merged. Once they land, the gate self-resolves and any future handler-less CommandDef fails CI immediately. Prior art in this codebase: test_commands_dict_includes_all_cli_commands and tests/providers/test_provider_profiles.py use the same enumeration- gate pattern against their respective registries.
Related: #51009 tracks the currently missing slash-command dispatches. This gate intentionally fails until those handlers land; maintainers should choose the landing sequence. |
1a1047c to
00071cc
Compare
|
Ignore the force push, hermes attempted to "helpfully" commit and push a local fix that was unrelated to this PR. The force push was needed to restore the branch to the clean state. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Good defensive test that catches a known bug class (missing dispatch handler) before it reaches production. Uses inspect.getsource to verify registry-to-dispatch parity. References prior instances of the bug class in the docstring. No security concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real registry/dispatch drift: current main registers /indicator and /whoami in hermes_cli/commands.py:121,163-165, while classic HermesCLI.process_command() falls through to its unknown-command path at cli.py:8958-9131.
Problems
tests/hermes_cli/test_commands.py:178usesinspect.getsource()and regexesprocess_command.AGENTS.md:1380-1384explicitly bans source-reading tests because they assert implementation shape rather than behavior.- The proposed assertion deliberately fails against current main until the existing dispatch gaps are resolved, so it cannot land independently as a green CI change.
Suggested changes
- Replace source inspection with an executable dispatch contract; extract a narrow resolver/dispatch seam if needed and test actual handler behavior.
- Carry the gate with, or after, the
/indicatorand/whoamidispatch or correct surface-classification fixes.
Automated hermes-sweeper review.
|
|
||
| from cli import HermesCLI | ||
|
|
||
| src = inspect.getsource(HermesCLI.process_command) |
There was a problem hiding this comment.
inspect.getsource() makes this a source-shape test, which AGENTS.md:1380-1384 explicitly bans. Please test an executable dispatch contract instead; extract a small resolver/dispatch seam if the current method is too large to exercise directly.
The gate read process_command()'s source with inspect.getsource and regexed the `canonical == "..."` chain. AGENTS.md bans source-reading tests outright, and the rationale applies directly: the regex matches a call site that exists but is wired wrong, and breaks on a formatting-only refactor. Both failure directions are wrong. Dispatch each non-gateway_only command for real against a MagicMock(spec=HermesCLI) bound as self, then assert the observable "Unknown command" fallback never fires. The alias resolution and dispatch chain execute; only the handler bodies collapse to mock no-ops. The unbound-with-mock-self form is deliberate and differs from the HermesCLI.__new__ helper in tests/cli/test_cli_prefix_matching.py. A real instance runs the handler bodies, which is correct for single-command behavior tests but not for a 74-command sweep: /undo and /update block on stdin prompts and /voice reaches the network. Verdict is unchanged: ['indicator', 'whoami']. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@teknium1 The source-shape defect has been resolved with proper exercising of the code. Regarding the green CI merge, this is a draft PR and this can't actually merge until I advance the draft. The current plan is to keep the draft in a WiP state until the missing dispatches land. Let me know if you'd rather I handle this in a different way. I decided against xfail or whitelisting approaches because I don't want to risk agents adopting that as a pattern when interacting with this test. On the topic of draft PRs, there may be a gap in the |
|
RE: gate verification, I had an agent do a quick local fix of the missing dispatches for /indicator and /whoami. I chose not to submit PRs for these as there is a demonstrated arms race of PRs competing for these fixes. The higher value contribution was closing the testing gap. (this PR)
Verification patch (local only — not proposed for merge)Verification was three-way:
The first two rows are the part that matters for "does this gate actually work": it goes green purely by wiring the handlers, with no changes to the test. The third row is why the rewrite was worth doing rather than just relocating the assertion. I commented out the So the original gate would have gone green on a build where One implementation note on the rewrite, since it deviates from the verification patchCaveat: this is a fixture, not a contribution. It prints via bare diff --git a/cli.py b/cli.py
--- a/cli.py
+++ b/cli.py
@@ -8895,10 +8895,14 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin):
self._handle_subgoal_command(cmd_original)
elif canonical == "skin":
self._handle_skin_command(cmd_original)
+ elif canonical == "indicator":
+ self._handle_indicator_command(cmd_original)
elif canonical == "voice":
self._handle_voice_command(cmd_original)
elif canonical == "busy":
self._handle_busy_command(cmd_original)
+ elif canonical == "whoami":
+ self._handle_whoami_command(cmd_original)
else:
# Check for user-defined quick commands (bypass agent loop, no LLM call)
base_cmd = cmd_lower.split()[0]
diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py
--- a/hermes_cli/cli_commands_mixin.py
+++ b/hermes_cli/cli_commands_mixin.py
@@ -2734,3 +2734,59 @@ class CLICommandsMixin:
else:
_cprint(f"Unknown voice subcommand: {subcommand}")
_cprint("Usage: /voice [on|off|tts|status]")
+
+ def _handle_whoami_command(self, command: str):
+ """Handle /whoami — show the active profile and session identity."""
+
+ profile = os.environ.get("HERMES_PROFILE", "default")
+ session = getattr(self, "session_id", "no session")
+ model = getattr(self, "model", "?")
+ provider = getattr(self, "provider", "?")
+ cwd = os.getcwd()
+ toolsets = getattr(self, "enabled_toolsets", []) or []
+
+ print()
+ print(f" Profile: {profile}")
+ print(f" Session: {session}")
+ print(f" Model: {model} ({provider})")
+ print(f" Directory: {cwd}")
+ print(f" Toolsets: {', '.join(sorted(toolsets)) if toolsets else 'default'}")
+ print()
+
+ def _handle_indicator_command(self, command: str):
+ """Handle /indicator [kaomoji|emoji|unicode|ascii] — set the TUI busy-indicator style."""
+ from cli import save_config_value
+
+ VALID = ("kaomoji", "emoji", "unicode", "ascii")
+
+ parts = command.strip().split(maxsplit=1)
+ style = parts[1].strip().lower() if len(parts) > 1 else ""
+
+ if not style:
+ import cli as _cli
+
+ current = _cli.load_cli_config().get("display", {}).get(
+ "tui_status_indicator", "kaomoji"
+ )
+ print()
+ print(f" Current indicator: {current}")
+ print(" Available styles:")
+ for s in VALID:
+ marker = " ●" if s == current else " "
+ print(f" {marker} {s}")
+ print()
+ print(" Usage: /indicator <style>")
+ print()
+ return
+
+ if style not in VALID:
+ print(f" Unknown indicator: {style}")
+ print(f" Pick one of: {'|'.join(VALID)}")
+ return
+
+ if save_config_value("display.tui_status_indicator", style):
+ print(f" Indicator set to: {style} (saved)")
+ else:
+ print(f" Indicator set to: {style}")
+ if self._apply_tui_skin_style():
+ print(" TUI indicator updated.") |
|
Just an activity ping. Still blocked on 22960 and 35892. If new breaking cases emerge in the interim, I will implement a temporary blacklist of the offenders so that we can get this shipped. (it will prove the coverage gap is leaking too regularly) |
Summary
Adds
test_every_cli_command_has_dispatch_handler, an enumeration gate that prevents the recurring bug class where commands are added toCOMMAND_REGISTRYbut never wired intoHermesCLI.process_command().The Bug Class
COMMAND_REGISTRY(source of truth for what commands exist) and theelifchain inprocess_command()(source of truth for what commands work) are maintained independently with no automated reconciliation. When they drift, the command breaks silently — appearing in/help, autocomplete, and all derived surfaces (Telegram/Slack) as functional, but returning "Unknown command" at runtime.This has occurred at least five times:
/model/resume/sessions/indicator/whoamiThe documented cost of not having this gate is not only recurrences of this bug class, but ballooning PRs from users converging on the same bug from everyday use. #51009 by @GitWhoIsThis (24d ago) is a superset issue that enumerated the full set of missing handlers. It remains open.
This PR
A single test in
TestDerivedDictsthat usesinspect.getsourceto extract every dispatched canonical name fromprocess_command()and asserts coverage over everygateway_only=Falseentry inCOMMAND_REGISTRY. The test currently fails honestly, and that's the point.Currently FAILS on main:
['indicator', 'whoami']. This PR is blocked until those handlers are merged (see #41869 / #40047 / #51178 and #35996). Once they land, the gate self-resolves and any future handler-lessCommandDeffails CI immediately.Prior Art in This Codebase
The enumeration-gate pattern is well-established here:
test_commands_dict_includes_all_cli_commands— assertsCOMMAND_REGISTRY⊆COMMANDStest_commands_by_category_covers_all_categories— asserts category completenesstests/providers/test_provider_profiles.py— asserts provider registry invariantstests/hermes_cli/test_auth_commands.py— asserts auth-step registry invariantsAll follow
for <item> in <REGISTRY>: assert <invariant>. This PR extends that pattern to the dispatch gap.Test Plan