fix(cli): tokenized dashboard cmdline matcher — detect global flags before the subcommand#44165
Open
AIalliAI wants to merge 1 commit into
Open
fix(cli): tokenized dashboard cmdline matcher — detect global flags before the subcommand#44165AIalliAI wants to merge 1 commit into
AIalliAI wants to merge 1 commit into
Conversation
…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
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 tasks
12 tasks
Contributor
|
Code review: clean ✅ The tokenized cmdline matcher is a solid improvement over substring matching. Verified:
No issues found. |
1 task
Contributor
Author
|
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). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
🤖 Generated with Claude Code