fix(cli): accept launcher flags after dashboard command

[OmarB97]

Summary

• normalize known top-level launcher flags when they appear after a subcommand before argparse runs
• keeps desktop/dashboard launchers from failing on shapes like hermes dashboard --no-open --tui
• adds parser coverage for trailing --tui and --skills ... --tui on dashboard launches

Root Cause

The macOS desktop app can launch the local backend as dashboard ... --tui. --tui is a top-level Hermes flag, but argparse rejected it after the dashboard subcommand, causing the backend to exit with rc 2 before readiness.

Validation

• python -m pytest tests/hermes_cli/test_default_interface_resolution.py tests/hermes_cli/test_subparser_routing_fallback.py tests/hermes_cli/test_startup_plugin_gating.py -q
• python -m hermes_cli.main dashboard --status --tui
• live ~/.hermes/hermes-agent cherry-pick: venv/bin/python -m pytest tests/hermes_cli/test_default_interface_resolution.py tests/hermes_cli/test_subparser_routing_fallback.py tests/hermes_cli/test_startup_plugin_gating.py -q
• direct /api/status returned gateway_running=True, gateway_state=running
• direct /api/ws probe opened and received gateway.ready
• normal /Applications/Hermes.app relaunch opened past CONNECTING into the chat shell with backend command dashboard --no-open --tui --host 127.0.0.1 --port 9120

Related

Fork PR: OmarB97#79.

OmarB97#76 / #38446 is separate and still needed for macOS bootstrap installer re-signing after copying to ~/.hermes; it is not this dashboard argument-order crash.

[OmarB97]

Review Findings

Overall: Needs rebasing onto main + has a correctness bug in the original hoisting logic

Issue 1: Hoisting ignores subcommand-native options (blocker)

The PR version of _hoist_post_subcommand_global_flags always hoists any recognized global flag that appears after the subcommand, regardless of whether the subcommand parser defines that flag. For example:

• hermes chat --provider gmi → hoists --provider gmi before chat, which is correct
• hermes dashboard --dev → hoists --dev before dashboard, which changes semantics if dashboard has its own --dev

A follow-up fix (fix(cli): preserve chat-local provider flags, 5f32ce0) already landed on main that adds subcommand_option_strings introspection to handle this correctly. The PR needs to be rebased onto current main to incorporate this fix and resolve the merge conflicts.

Issue 2: Tests use outdated 2-parameter API

The test file calls _hoist_post_subcommand_global_flags(argv, known_cmds) with 2 arguments, but main has evolved to 3 arguments (subcommand_option_strings). Tests will fail to run after rebasing unless updated.

Issue 3: Minor — return argv guard in command detection

Line return argv in the head scan loop is a conservative early-exit when a non-flag, non-command token appears before any known command. This is correct and intentional (don't hoist if argv structure is unexpected), but worth documenting since it could look like a bug.

Recommendation

1. Rebase onto current main — the subcommand_option_strings fix is already there
2. Update tests to use the 3-parameter signature
3. The core approach (flag hoisting) is sound and addresses the reported issue

[OmarB97]

Review: Post-subcommand flag hoisting

Verdict: Approve (with follow-up suggestions)

What works

• Core logic correctly identifies subcommand position, separates prefix/remainder, and hoists recognized top-level flags before the subcommand.
• -- separator respected — nothing after -- is touched.
• Inline value flags (--model=gpt-5) handled correctly.
• Conservative flag whitelist prevents hoisting unknown flags.
• Tests cover the main scenarios (basic hoisting, multi-flag, separator).

Follow-up suggestions (non-blocking)

1. Subcommand flag collision avoidance — If a subcommand defines its own --tui or --model, the hoister moves it to the parent level regardless. Consider introspecting subcommand parsers to skip flags the subcommand owns. The working tree already has subcommand_option_strings for this — ensure it lands before this PR merges.

2. Position-0 blind spot — hermes dashboard --tui --no-open won't hoist --tui because i > 0 guards prevent hoisting at position 0 (right after the subcommand name). This is a design choice that works for the common case (--tui at the end) but may confuse users who put global flags first.

3. Test gap — The test_dashboard_accepts_trailing_tui_flag test uses --host and --port as separate flags/values but the hoisting function only tests that hoisting itself works. Consider adding an end-to-end test that runs parser.parse_args() with a dashboard subparser that defines --port/--host/--no-open to verify no subcommand flag is accidentally consumed.

[OmarB97]

Review Summary (self-review, OmarB97)

Verdict: Approve with 1 concern for follow-up.

What works

• Flag hoisting logic is correct for all tested patterns: trailing bool flags, trailing value flags, double-dash separator stops hoisting.
• Integration point is correct: hoist runs after coalesce and before bpo-9338 workaround, with known_cmds properly computed.
• Tests cover the primary use cases and verify that argparse can successfully parse the hoisted argv.
• Directly addresses the task goal: dashboard with trailing --tui now works instead of dying with rc=2.

Concerns (non-blocking)

Subcommand option collision risk (suggestion): The hoisting function does not check whether a flag is native to the subcommand before extracting it. If a future subcommand defines --dev, --tui, or any flag in the hoistable sets, the hoister would incorrectly pull it before the subcommand. The main branch has already addressed this with subcommand_option_strings plus native_options awareness. Consider backporting that improvement to this PR before merge, or ensure the follow-up lands immediately after.

Minor test gaps (info): Missing test for short-form hoisting after subcommand, and no test verifying that a value flag without a following value stays in remainder gracefully. Low priority.

No blockers found.

[OmarB97]

Review Summary

Verdict: Approve (with non-blocking suggestions below)

The PR correctly solves the reported problem: hermes dashboard --no-open --tui was failing because argparse rejects parent-parser flags after a subcommand. The hoisting approach is conservative and well-scoped.

What Works Well

• Conservative allowlist - only known top-level flags are hoisted, command-specific flags are left alone
• -- separator respected - tokens after -- are never touched
• Tests cover the key scenarios: trailing bool flag, trailing value+bool flags, and -- escape
• All 24 tests in the file pass (including the 3 new ones)

Non-blocking Suggestions

1. Subcommand flag collision (suggestion): If a subcommand defines its own --model or --tui, the hoister would still move the post-subcommand instance before the subcommand. This is a known limitation but could bite users who add overlapping flags to subcommands in the future.

2. Missing -c/--continue (info): _TOP_LEVEL_VALUE_FLAGS includes -c/--continue but _POST_SUBCOMMAND_TOP_LEVEL_VALUE_FLAGS does not. Intentional, but worth documenting why.

3. Test coverage gap (suggestion): Consider a test for the case where the first token after subcommand is a value flag: dashboard -m gpt5.

[OmarB97]

Review Summary

Verdict: Approve with minor suggestions (non-blocking).

What the PR does

Adds to move known top-level launcher flags (like , , ) that appear after a subcommand (e.g., ) to before the subcommand. Fixes argparse rejecting parent-parser options that trail the subcommand.

Correctness

• Core hoisting logic is sound: O(N) scan, handles separator, inline syntax, and value flags with separate arguments.
• Pipeline order is correct: runs before hoisting, so multi-word session names are already joined.
• Integration replaces assignment correctly; all downstream calls use the processed argv.
• Conservative flag set: deliberately excludes , , from post-subcommand hoisting to avoid ambiguity.

Non-blocking suggestions

1. Test: inline syntax — The function handles for inline values, but there is no test exercising this path. A test case like would add coverage.

2. Test: flags already before subcommand — No test verifies that passes through unchanged (hoisting is a no-op when flags are already in the right position).

3. Test: short flag — The bool flags set includes short forms (, , , ) but tests only use long forms.

4. Minor: ordering — The frozenset is unordered, but the listing alternates short/long form inconsistently (e.g., together, but alone). Minor readability nit — not a bug.

No blockers found.

The change is well-contained, conservative, and solves the reported issue.

[OmarB97]

Review Summary

Verdict: Approve with minor suggestions (non-blocking).

What the PR does

Adds _hoist_post_subcommand_global_flags() to move known top-level launcher flags (like --tui, --yolo, --model) that appear after a subcommand to before the subcommand. Fixes argparse rejecting parent-parser options that trail the subcommand.

Correctness

• Core hoisting logic is sound: O(N) scan, handles -- separator, inline = syntax, and value flags with separate arguments.
• Pipeline order is correct: _coalesce_session_name_args() runs before hoisting, so multi-word session names are already joined.
• Integration replaces _processed_argv assignment correctly; all downstream parse_args() calls use the processed argv.
• Conservative flag set: deliberately excludes -z/--oneshot, -r/--resume, -c/--continue from post-subcommand hoisting to avoid ambiguity.

Non-blocking suggestions

1. Test: inline -m=value syntax — The function handles inline values but there is no test exercising this path. A test case like ['dashboard', '-m=gpt5', '--tui'] would add coverage.

2. Test: flags already before subcommand — No test verifies that ['--tui', 'dashboard'] passes through unchanged.

3. Test: short -w flag — Tests only use long forms.

4. Minor: _POST_SUBCOMMAND_TOP_LEVEL_BOOL_FLAGS ordering — Alternates short/long form inconsistently. Minor readability nit.

No blockers found.

The change is well-contained, conservative, and solves the reported issue.

[OmarB97]

Review: fix(cli): accept launcher flags after dashboard command

Overall: Looks good. Conservative, well-scoped fix that solves the reported issue.

Findings

1. Missing test for value flag hoisting (suggestion)
The test suite covers (bool) and (value) but not . A quick test for would add confidence without much cost.

2. includes (suggestion)
The short form is listed alongside . The existing set does NOT include (it's a bool flag, not a value flag), so this is correct. However, the scanning loop in step 2 (finding the command) only skips value-taking flags from — it doesn't know about as a bool flag. This means would incorrectly treat as an unknown flag and bail early.

Wait — actually, the scanning loop for finding the command does:

For , it starts with , is NOT in , so — it correctly skips over it. The next token would be which IS in known_cmds. So this works correctly.

3. Edge case: consumed as skills value (info)
would hoist together, interpreting as the skills value. This is a pathological case that no user would realistically encounter.

4. No test for the full integration path (suggestion)
The tests call directly and then . Consider adding an integration test that runs with to verify the end-to-end flow.

[OmarB97]

Review: fix(cli): accept launcher flags after dashboard command

Overall: Looks good. Conservative, well-scoped fix that solves the reported issue.

Findings

1. Missing test for --provider value flag hoisting (suggestion)
The test suite covers --tui (bool) and --skills (value) but not --provider. A quick test for the provider flag would add confidence.

2. Edge case: -s --tui consumed as skills value (info)
If someone writes hermes dashboard -s --tui, the hoister treats --tui as the skills value since -s is a value-taking flag. Pathological case that no user would encounter in practice.

3. No test for the full integration path (suggestion)
The tests call _hoist_post_subcommand_global_flags() directly and then parser.parse_args(). Consider adding an integration test that exercises the full sys.argv path through cmd_acp to verify the end-to-end flow including the _coalesce_session_name_args interaction.

4. Test uses _dashboard_parser that adds dashboard subparser dynamically (info)
The test fixture adds a dashboard subparser with --port, --host, --no-open args. This is a good approach since it tests against a minimal parser rather than the full 30+ subparser tree.

[OmarB97]

Review: Post-subcommand flag hoisting (hermes-local-longctx-ko-mac)

Verdict: Approve (non-blocking follow-ups noted)

What works

• Core hoisting logic correctly identifies subcommand boundary, separates prefix/hoisted/remainder, and reconstructs argv with global flags before the subcommand.
• -- separator respected — nothing after -- is hoisted.
• Inline value flags (--model=gpt-5) handled correctly.
• Conservative whitelist prevents hoisting arbitrary flags.
• Tests pass (4/4) and cover trailing bool flags, trailing value flags, and separator.

Follow-up (non-blocking)

1. Test gap: inline value syntax — No test for --model=gpt5 hoisting from post-subcommand position. The code path exists (inline_value_match) but is untested.

2. Test gap: position-0 after subcommand — hermes dashboard --tui --no-open won't hoist --tui since the i > 0 guard skips position 0 (the subcommand token itself). This is intentional (position 0 is the command name), but a test confirming that flags immediately after the command name ARE hoisted would add confidence.

3. Branch has accumulated unrelated changes — The branch diff vs main includes workflow files, website content, model catalog updates, desktop app changes, and achievement JSON. These are unrelated to the flag hoisting fix. Consider squashing or cherry-picking only the relevant commit for a cleaner review surface.

4. --pass-session-id in bool flags — This flag takes no value, but verify it doesn't have a hidden value-taking variant that the hoister would miss. A quick grep confirms it's store_true only — safe.

Verdict: The core approach is sound and addresses the reported issue (argparse rejecting parent-level flags after subcommand). Merge with the understanding that test coverage could be expanded in follow-up.

[OmarB97]

Review: fix(cli): accept launcher flags after dashboard command

Verdict: Approve (with non-blocking suggestions below)

Summary

Clean implementation that solves the real problem: argparse rejecting global flags placed after subcommand names. The approach of post-hoc argv reordering before argparse sees it is the right call - avoids touching every subparser and is conservative about what gets hoisted.

Strengths

• Native option awareness - The subcommand_option_strings integration correctly keeps provider/model flags attached to chat instead of hoisting them. Critical correctness property.
• Double dash hard stop - -- correctly prevents hoisting, preserving argparse passthrough semantics.
• Conservative flag lists - Only known top-level flags are hoisted; unknown flags stay put. Early exit on unknown positional prevents accidental reordering.
• Tests pass - All 4 new tests pass covering trailing flags, native flag preservation, and double dash stop.

Non-blocking suggestions

1. Missing --yolo test - Listed in bool flags set but no explicit test case.
2. Missing --worktree test - Same as above for -w/--worktree.
3. Value flag without value at end - dashboard --model (no value) stays in place and fails at argparse. Acceptable (no regression), but worth documenting.
4. Comment the flag list split - _POST_SUBCOMMAND_* excludes --oneshot/--resume/--continue that are in _TOP_LEVEL_*. Correct but undocumented why.
5. Performance note - inline_value_match generator iterates ~8 flags per token. Negligible now, but a dict keyed by prefix would be O(1) if the list grows.

Alignment with task goal

Desktop invoked hermes dashboard --no-open --tui and argparse rejected --tui after the subcommand. This fix resolves it. Directly addresses the reported problem.

No blocking issues found. Ready to merge.

[OmarB97]

Closing: the motivating rc-2 crash was fixed upstream in 2820d87 (hidden accepted-and-ignored --tui on the dashboard subparser, now in hermes_cli/subcommands/dashboard.py), and the per-subparser tolerance approach maintainers chose makes the generic post-subcommand flag hoisting here a separate behavior-change proposal. If the generic hoist is still wanted it deserves a fresh feat(cli) PR with its own motivation.