fix(cli): tokenized dashboard cmdline matcher — detect global flags before the subcommand - #44165
fix(cli): tokenized dashboard cmdline matcher — detect global flags before the subcommand#44165AIalliAI wants to merge 2 commits into
Conversation
|
Code review: clean ✅ The tokenized cmdline matcher is a solid improvement over substring matching. Verified:
No issues found. |
|
Requesting maintainer review — this is ready to land from my side. Standalone fork CI is pending first-run approval here; the rollup branch in #44061 carrying this session's batch is fully green on upstream CI (all test shards, typecheck, e2e). |
09ff58d to
f3e6e40
Compare
…the subcommand
The stale-dashboard scan in _find_stale_dashboard_pids() matched fixed
substrings ("hermes dashboard", "hermes_cli.main dashboard", ...), so any
invocation with global options between the entrypoint and the subcommand —
e.g. `python -m hermes_cli.main --profile work dashboard --port 9119` —
was invisible to `hermes dashboard --status`, `--stop`, and the
post-update stale-backend cleanup. After `hermes update`, a
profile-scoped dashboard kept serving the old Python backend against the
new JS bundle.
Replace the substring patterns with a tokenized matcher that finds the
hermes entrypoint (binary, -m module, or script path) and then walks
known top-level flags — introspected from the real parser, the same way
hermes_cli.relaunch builds its inherited-flag table — until it hits the
subcommand. Unknown flags and free-text arguments bail out, so cmdlines
that merely mention "dashboard" (`hermes -z "fix my dashboard"`, which
the old substring match would have killed) are never matched.
Fixes NousResearch#44035
f3e6e40 to
fe149e5
Compare
|
Independently reproduced this bug and verified the fix approach is correct. Reproduction: My dashboard runs as a systemd user service with the command shape Verification of this PR's approach: The tokenized matcher correctly strips Complementary work: Issue #40449 and PR #39166 address the other half — when the dashboard IS detected but runs as a systemd service, raw-killing the PID leaves it dead (systemd records it as a clean stop, no restart under The false-positive guard (bail on unknown flags, strict subcommand match) is important — |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real current-main detection gap: hermes_cli/main.py:5877-5880 still uses contiguous substring patterns, so --profile before dashboard is missed.
Problems
- The PR's equals-form branch (
hermes_cli/main.py:5373on the PR head) skips validation before the unknown-flag bailout.hermes --future-flag=x dashboardwould match; validate the flag and value arity first. - Current main added
hermes servecleanup athermes_cli/main.py:5881-5886in dff491a. Replacing the whole pattern list with a dashboard-only matcher would regress headless desktop backend cleanup. - The entrypoint scan accepts
hermesanywhere in the command line (hermes_cli/main.py:5352-5361on the PR head), so shell wrappers such assh -c hermes dashboard --statusremain false positives described in #44035.
Suggested changes
- Preserve both current stale-server targets (
dashboardandserve), validate equals-form flags, and add regressions for unknown--flag=value, profile-prefixedserve, and a shell wrapper.
Automated hermes-sweeper review.
|
|
||
| flag_arity = _dashboard_global_flag_arity() | ||
| i = entry_idx + 1 | ||
| while i < len(tokens): |
There was a problem hiding this comment.
This accepts every equals-form option before checking flag_arity, so hermes --future-flag=x dashboard returns true despite the documented unknown-flag bailout. Split on =, require the flag to be known and value-taking, then add a negative regression.
| parser, _subparsers, _chat_parser = build_top_level_parser() | ||
| for action in parser._actions: | ||
| takes_value = action.nargs != 0 # store_true/false set nargs=0 | ||
| for opt in action.option_strings: |
There was a problem hiding this comment.
Current main also reaps hermes serve as the desktop headless backend (hermes_cli/main.py:5881-5886, dff491a). A dashboard-only matcher would drop that behavior; generalize this matcher to the supported stale-server subcommands and cover serve.
| entry_idx = i | ||
| break | ||
| if entry_idx is None: | ||
| return False |
There was a problem hiding this comment.
Because the entrypoint search accepts hermes anywhere in the command line, sh -c hermes dashboard --status still reaches this return path. #44035 calls out wrapper false positives; please add a regression and constrain the matcher accordingly.
Three fixes per @teknium1 review: 1. Validate equals-form flags before accepting them — require the flag name to be known and value-taking (rejects ). 2. Generalize to and accept both and as the subcommand, preserving current main's headless desktop backend cleanup support. 3. Reject shell-wrapped cmdlines ( ╭─ Hermes Agent v0.18.2 (2026.7.7.2) · upstream eb52760 · local 935e424 (+2 ─╮ │ Available Tools │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣀⡀⠀⣀⣀⠀⢀⣀⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ browser: browser_back, browser_click, │ │ ⠀⠀⠀⠀⠀⠀⢀⣠⣴⣾⣿⣿⣇⠸⣿⣿⠇⣸⣿⣿⣷⣦⣄⡀⠀⠀⠀⠀⠀⠀ ... │ │ ⠀⢀⣠⣴⣶⠿⠋⣩⡿⣿⡿⠻⣿⡇⢠⡄⢸⣿⠟⢿⣿⢿⣍⠙⠿⣶⣦⣄⡀⠀ clarify: clarify │ │ ⠀⠀⠉⠉⠁⠶⠟⠋⠀⠉⠀⢀⣈⣁⡈⢁⣈⣁⡀⠀⠉⠀⠙⠻⠶⠈⠉⠉⠀⠀ code_execution: execute_code │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣴⣿⡿⠛⢁⡈⠛⢿⣿⣦⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ computer_use: computer_use │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠿⣿⣦⣤⣈⠁⢠⣴⣿⠿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ cronjob: cronjob │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠉⠻⢿⣿⣦⡉⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ delegation: delegate_task │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠘⢷⣦⣈⠛⠃⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ file: patch, read_file, search_files, │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢠⣴⠦⠈⠙⠿⣦⡄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ write_file │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠸⣿⣤⡈⠁⢤⣿⠇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ image_gen: image_generate │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠉⠛⠷⠄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ (and 9 more toolsets...) │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣀⠑⢶⣄⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣿⠁⢰⡆⠈⡿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ Available Skills │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠳⠈⣡⠞⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ apple: apple-notes, +3 more │ │ ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ autonomous-ai-agents: claude-code, +7 │ │ more │ │ MiniMax-M3 · Nous Research creative: architecture-diagram, +16 │ │ /Users/adalsteinnhelgason/hermes-4… more │ │ Session: 20260726_130302_26b8d2 data-science: jupyter-live-kernel │ │ devops: ci-cd-pipeline-audit, +3 more │ │ email: himalaya │ │ general: code-quality-analysis, +7 │ │ more │ │ github: bulk-pr-review-fix, +20 more │ │ media: gif-search, heartmula, +2 more │ │ mlops: audiocraft-audio-generation, │ │ +8 more │ │ mlops-inference: api-fleet-routing │ │ note-taking: local-knowledge-base, +2 │ │ more │ │ productivity: airtable, docx, +10 │ │ more │ │ research: │ │ acquisition-target-analysis, +45 more │ │ smart-home: openhue │ │ social-media: │ │ organization-social-audit, +2 more │ │ software-development: │ │ android-foreground-service, +57 more │ │ │ │ 33 tools · 201 skills · /help for │ │ commands │ │ ⚠ 1 commit behind — run hermes update │ │ to update │ ╰──────────────────────────────────────────────────────────────────────────────╯ Welcome to Hermes Agent! Type your message or /help for commands. ✦ Tip: The status bar turns yellow, then orange, then red as context fills up. ⚕ MiniMax-M3 │ ctx -- │ [░░░░░░░░░░] -- │ 2s │ ⏲ 0s ─────────────────────────────────────────────────────────────────────────────── ─ ❯ ─────────────────────────────────────────────────────────────────────────────── ─ �[2;3mShutting down… (finalizing session)�[0m Goodbye! ⚕) whose parent shell is the real process — fixes false positive noted in NousResearch#44035. Add regression tests for all three fixes: - - - All 34 tests in test_update_stale_dashboard.py pass.
Problem
_find_stale_dashboard_pids()matches fixed substrings ("hermes dashboard","hermes_cli.main dashboard","hermes_cli/main.py dashboard"), so any invocation with global options between the entrypoint and the subcommand —— is invisible to
hermes dashboard --status,hermes dashboard --stop, and the post-update stale-backend cleanup. Afterhermes update, a profile-scoped dashboard keeps serving the old Python backend against the freshly-built JS bundle.Fix
Replace the substring patterns with a tokenized matcher (
_is_dashboard_cmdline):hermes/hermes.exebasename,hermes_cli.main(after-m), or ahermes_cli/main.pyscript path.dashboard.Flag arity (does
--profileconsume a value? is--tuiboolean?) is introspected from the real top-level parser plusPRE_ARGPARSE_INHERITED_FLAGS— the same approachhermes_cli.relaunchuses for its inherited-flag table — so the matcher can't drift out of sync as global options are added.Unknown flags and free-text arguments bail out. This matters because the scan feeds a SIGTERM/SIGKILL pass:
psoutput does not preserve shell quoting, so a looser "both words appear" match would killhermes -z "summarize my dashboard"mid-session duringhermes update. The matcher is strictly tighter than the old patterns on this front — the old"hermes dashboard"substring already false-matched cmdlines likehermes -z fix hermes dashboard, which now stays alive (covered by a regression test).Relation to #44048: alternative implementation. That PR's helper accepts any cmdline where an entrypoint substring appears anywhere (including e.g. paths containing
.hermes/) anddashboardappears as a whitespace-delimited word anywhere — which false-positive-kills oneshot/chat sessions that mention "dashboard", since realpsoutput strips the quotes its guard relies on. Happy to converge the two PRs either way.Both detection consumers are covered (
--status/--stopvia_report_dashboard_status/_kill_stale_dashboard_processes, update cleanup via the same kill helper), Windows wmic and POSIX ps branches share the matcher.Tests
-p,--profile=, boolean flags, script-path form, plus negative cases (other subcommands, prompts mentioning "dashboard", profile literally nameddashboard, unknown flags).tests/hermes_cli/test_update_stale_dashboard.py+test_dashboard_lifecycle_flags.py: 41/41 pass.tests/hermes_cli/suite: failure set identical toorigin/mainbaseline on the same machine (161 pre-existing environment failures, zero new), 9 more tests passing.Fixes #44035