mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(cli): preserve -t/-m/--provider/--tui/--dev before chat subcommand
`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.
This commit is contained in:
briandevans
Teknium
parent
91d05b982d
commit
5ecc07986f
3 changed files with 184 additions and 6 deletions
|
|
@ -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)",
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <flag> <value> chat`:\n "
|
||||
+ "\n ".join(f"{opts} dest={dest} default={d!r}"
|
||||
for opts, dest, d in offenders)
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue