From 5ecc07986f46463ca3096679b03a46402eb19cee Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Tue, 19 May 2026 08:19:31 -0700 Subject: [PATCH] fix(cli): preserve -t/-m/--provider/--tui/--dev before chat subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes -t web chat` silently dropped the toolset filter (and the same hold true for `-m`, `--provider`, `--tui`, `--dev` placed before `chat`). Reported in #28780 for `-t/--toolsets`; the others are sibling failures with the same root cause. Root cause: the chat subparser re-declared these flags with `default=None` (or `default=False` for store_true) on top of the matching top-level parser flags. When argparse dispatches into the subparser it shares the namespace via `dest`, so the subparser's default overwrites whatever the top-level parser parsed before the subcommand. `-s/--skills`, `-r/-c/-w`, `--yolo`, and `--pass-session-id` already use `default=argparse.SUPPRESS` for exactly this reason — the chat-subparser action becomes a no-op unless the user explicitly passes the flag after `chat`, and the parent value survives. Reproduction (origin/main, before fix): >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets None >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' After fix: >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets 'web' >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' Sibling flags fixed in the same commit because they share the exact same argparse pattern bug — verified via a new contract test that scans every chat-subparser action whose `dest` is also on the top-level parser and asserts `default is argparse.SUPPRESS`. The test fails on origin/main listing all five offenders and passes after this fix. Test additions in tests/hermes_cli/test_argparse_flag_propagation.py: - TestChatSubparserInheritedValueFlags exercising real `_parser` build (not the hand-rolled replica) so it catches future drift. - Parametrized before-chat / after-chat cases for `-t`, `--toolsets`, `-m`, `--model`, `--provider`. - Negative case: passing none of the flags leaves attrs at the top-level parser's `None` default (SUPPRESS does not remove existing attrs). - Combined case: all three value flags before `chat` simultaneously. - store_true cases for `--tui` / `--dev`. - Contract test asserting every shared-`dest` flag on chat uses SUPPRESS. Fixes #28780. --- hermes_cli/_parser.py | 27 +++- .../test_argparse_flag_propagation.py | 118 ++++++++++++++++++ .../test_kanban_worker_spawn_toolsets.py | 45 +++++++ 3 files changed, 184 insertions(+), 6 deletions(-) diff --git a/hermes_cli/_parser.py b/hermes_cli/_parser.py index b2be73871b2..9cc3c277900 100644 --- a/hermes_cli/_parser.py +++ b/hermes_cli/_parser.py @@ -271,12 +271,27 @@ def build_top_level_parser(): chat_parser.add_argument( "--image", help="Optional local image path to attach to a single query" ) + # `default=argparse.SUPPRESS` on flags that are ALSO declared on the + # top-level parser: when the user writes `hermes -m foo chat`, argparse + # first sets `args.model = "foo"` from the top-level parser, then + # dispatches to the chat subparser. Without SUPPRESS the chat subparser's + # own default (`None`) would silently clobber the top-level value because + # the subparser shares the same namespace and `dest`. SUPPRESS keeps the + # subparser action a no-op unless the user actually passes the flag after + # the subcommand. Matches the pattern already used for `-s/--skills` and + # the relaunch-inherited flags `-r/--resume`, `-c/--continue`, + # `-w/--worktree`, `--yolo`, etc. (see tests/hermes_cli/ + # test_argparse_flag_propagation.py). _inherited_flag( chat_parser, - "-m", "--model", help="Model to use (e.g., anthropic/claude-sonnet-4)", + "-m", "--model", + default=argparse.SUPPRESS, + help="Model to use (e.g., anthropic/claude-sonnet-4)", ) chat_parser.add_argument( - "-t", "--toolsets", help="Comma-separated toolsets to enable" + "-t", "--toolsets", + default=argparse.SUPPRESS, + help="Comma-separated toolsets to enable", ) _inherited_flag( chat_parser, @@ -293,7 +308,7 @@ def build_top_level_parser(): # are also valid values, and runtime resolution (resolve_runtime_provider) # handles validation/error reporting consistently with the top-level # `--provider` flag. - default=None, + default=argparse.SUPPRESS, help="Inference provider (default: auto). Built-in or a user-defined name from `providers:` in config.yaml.", ) chat_parser.add_argument( @@ -401,14 +416,14 @@ def build_top_level_parser(): chat_parser, "--tui", action="store_true", - default=False, + default=argparse.SUPPRESS, help="Launch the modern TUI instead of the classic REPL", ) _inherited_flag( chat_parser, "--cli", action="store_true", - default=False, + default=argparse.SUPPRESS, help="Force the classic prompt_toolkit REPL (overrides display.interface=tui)", ) _inherited_flag( @@ -416,7 +431,7 @@ def build_top_level_parser(): "--dev", dest="tui_dev", action="store_true", - default=False, + default=argparse.SUPPRESS, help="With --tui: run TypeScript sources via tsx (skip dist build)", ) diff --git a/tests/hermes_cli/test_argparse_flag_propagation.py b/tests/hermes_cli/test_argparse_flag_propagation.py index 0c62655a637..558489b84ef 100644 --- a/tests/hermes_cli/test_argparse_flag_propagation.py +++ b/tests/hermes_cli/test_argparse_flag_propagation.py @@ -219,3 +219,121 @@ print(json.dumps(results)) f"stderr: {entry['stderr']}" ) assert "unrecognized arguments" not in entry["stderr"] + + +class TestChatSubparserInheritedValueFlags: + """Verify -t/--toolsets, -m/--model and --provider survive parent→chat + subparser dispatch. + + Regression test for #28780: `hermes -t web chat` silently dropped the + toolset because the chat subparser re-declared `-t/--toolsets` with + `default=None`, which clobbered the top-level parser's value during + subparser dispatch. + + Uses the real `hermes_cli._parser.build_top_level_parser()` rather than + the hand-rolled replica above so this also fails if the production + parser drifts back to `default=None` on these flags. + """ + + @pytest.fixture + def real_parser(self): + from hermes_cli._parser import build_top_level_parser + parser, _subparsers, _chat = build_top_level_parser() + return parser + + @pytest.mark.parametrize("flag,attr,value", [ + ("-t", "toolsets", "web"), + ("--toolsets", "toolsets", "web,terminal"), + ("-m", "model", "anthropic/claude-sonnet-4"), + ("--model", "model", "openai/gpt-4"), + ("--provider", "provider", "openrouter"), + ]) + def test_flag_before_chat_is_preserved(self, real_parser, flag, attr, value): + args, _ = real_parser.parse_known_args([flag, value, "chat"]) + assert getattr(args, attr, None) == value, ( + f"`hermes {flag} {value} chat` lost the flag — got " + f"{getattr(args, attr, None)!r}, expected {value!r}" + ) + + @pytest.mark.parametrize("flag,attr,value", [ + ("-t", "toolsets", "web"), + ("--toolsets", "toolsets", "web,terminal"), + ("-m", "model", "anthropic/claude-sonnet-4"), + ("--model", "model", "openai/gpt-4"), + ("--provider", "provider", "openrouter"), + ]) + def test_flag_after_chat_still_works(self, real_parser, flag, attr, value): + args, _ = real_parser.parse_known_args(["chat", flag, value]) + assert getattr(args, attr, None) == value + + def test_no_flag_leaves_attrs_at_top_level_default(self, real_parser): + """When the user passes none of the inherited flags, the top-level + parser's `default=None` still seeds the namespace — the SUPPRESS on + the subparser must not remove existing attributes.""" + args, _ = real_parser.parse_known_args(["chat"]) + assert getattr(args, "toolsets", "MISSING") is None + assert getattr(args, "model", "MISSING") is None + assert getattr(args, "provider", "MISSING") is None + + def test_all_three_flags_before_chat(self, real_parser): + """Issue #28780 reporter's case generalized: passing every inherited + value flag before `chat` must preserve all of them simultaneously.""" + args, _ = real_parser.parse_known_args([ + "-t", "web", + "-m", "anthropic/claude-sonnet-4", + "--provider", "openrouter", + "chat", + ]) + assert args.toolsets == "web" + assert args.model == "anthropic/claude-sonnet-4" + assert args.provider == "openrouter" + + @pytest.mark.parametrize("flag,attr", [ + ("--tui", "tui"), + ("--cli", "cli"), + ("--dev", "tui_dev"), + ]) + def test_store_true_flag_before_chat_is_preserved( + self, real_parser, flag, attr, + ): + """`--tui` / `--cli` / `--dev` are store_true flags inherited by chat; the same + SUPPRESS contract applies. Without it, the subparser's `default=False` + would clobber the parent's `True` when used as `hermes --tui chat`.""" + args, _ = real_parser.parse_known_args([flag, "chat"]) + assert getattr(args, attr, None) is True, ( + f"`hermes {flag} chat` lost the flag — got " + f"{getattr(args, attr, None)!r}, expected True" + ) + + def test_chat_subparser_inherited_value_flags_use_suppress(self): + """Contract test for the underlying invariant. + + Any chat-subparser flag whose `dest` also exists on the top-level + parser MUST declare `default=argparse.SUPPRESS`, otherwise the + subparser silently overwrites the top-level value with its own + default during dispatch. This is the structural class behind #28780. + """ + from hermes_cli._parser import build_top_level_parser + parser, _subparsers, chat_parser = build_top_level_parser() + + top_level_dests = { + a.dest for a in parser._actions + if a.option_strings and a.dest != "help" + } + + offenders = [] + for action in chat_parser._actions: + if not action.option_strings or action.dest == "help": + continue + if action.dest not in top_level_dests: + continue + if action.default is not argparse.SUPPRESS: + offenders.append((action.option_strings, action.dest, action.default)) + + assert not offenders, ( + "Chat subparser redeclares these top-level flags without " + "default=argparse.SUPPRESS; they will silently clobber the " + "top-level value when used as `hermes chat`:\n " + + "\n ".join(f"{opts} dest={dest} default={d!r}" + for opts, dest, d in offenders) + ) diff --git a/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py b/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py index da9b6b957b2..78f45d34b3c 100644 --- a/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py +++ b/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py @@ -125,6 +125,51 @@ def test_default_spawn_never_boots_the_tui(monkeypatch, tmp_path): assert "HERMES_TUI" not in captured["env"] +def test_default_spawn_model_override_survives_real_cli_parse(monkeypatch, tmp_path): + """The dispatcher's pre-``chat`` model flag must reach ``args.model``. + + This is an integration contract between Kanban's worker argv builder and + the real CLI parser. A parser default once erased the explicit override, + silently sending the worker to its profile default or fallback instead. + """ + root = tmp_path / ".hermes" + (root / "profiles" / "elias").mkdir(parents=True) + root.joinpath("config.yaml").write_text("{}\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(root)) + + from hermes_cli import kanban_db as kb + from hermes_cli._parser import build_top_level_parser + + monkeypatch.setattr(kb, "_resolve_hermes_argv", lambda: ["hermes"]) + captured = {} + + class FakeProc: + pid = 4244 + + def fake_popen(cmd, *args, **kwargs): + captured["cmd"] = list(cmd) + return FakeProc() + + monkeypatch.setattr(subprocess, "Popen", fake_popen) + + workspace = tmp_path / "workspace" + workspace.mkdir() + task = _make_task(kb, assignee="elias") + task.model_override = "gpt-5.6-sol" + kb._default_spawn(task, str(workspace)) + + parser, _subparsers, _chat_parser = build_top_level_parser() + # Profile selection is attached by the outer CLI bootstrap rather than + # build_top_level_parser(); remove that already-validated prefix and parse + # the worker flags/subcommand through the real shared parser. + assert captured["cmd"][1:3] == ["-p", "elias"] + args = parser.parse_args(captured["cmd"][3:]) + + assert args.command == "chat" + assert args.model == "gpt-5.6-sol" + assert args.query == "work kanban task t_spawn_tools" + + def test_resolve_worker_cli_toolsets_uses_profile_home_not_parent_config(monkeypatch, tmp_path): root = tmp_path / ".hermes" profile = root / "profiles" / "elias"