diff --git a/cli.py b/cli.py index e0f9be91bb57..86978ced6553 100644 --- a/cli.py +++ b/cli.py @@ -1498,6 +1498,26 @@ def run(self): def _build_cli_from_args(model, toolsets, provider, reasoning, api_key, base_url, max_turns, run_budget, verbose, compact, resume, checkpoints, pass_session_id, ignore_rules, skills): """Resolve the toolset list (explicit / coding posture / platform default), construct HermesCLI, and start the background skills preload.""" + from hermes_cli.oneshot import _normalize_toolsets + from toolsets import ZERO_TOOLSET + + # A flag that was PASSED but names nothing is not "unspecified". `hermes chat -q ... -t ""` + # must not silently receive the coding posture or the platform default, which is the same + # footgun -z had: the caller asked for a restricted session and got a fully equipped one. + # The parser already distinguishes absent (None) from empty (""), so the resolver must too. + explicit = toolsets is not None + normalized = _normalize_toolsets(toolsets) if explicit else None + if explicit and not normalized: + raise ValueError( + "--toolsets was passed an empty value, which means \"unspecified\" — " + f"pass a toolset name, or '{ZERO_TOOLSET}' for an explicitly tool-less session." + ) + if normalized and ZERO_TOOLSET in normalized and len(normalized) > 1: + raise ValueError( + f"--toolsets: '{ZERO_TOOLSET}' means zero tools and cannot be combined with " + f"other toolsets: {', '.join(n for n in normalized if n != ZERO_TOOLSET)}" + ) + toolsets_list = None if isinstance(toolsets, str) and toolsets: toolsets_list = [t.strip() for t in toolsets.split(",")] diff --git a/hermes_cli/_parser.py b/hermes_cli/_parser.py index ade6dd98b529..7fcf1d8e89a5 100644 --- a/hermes_cli/_parser.py +++ b/hermes_cli/_parser.py @@ -171,7 +171,8 @@ def _add_top_level_flags(parser: argparse.ArgumentParser) -> None: "config.yaml for this run only; the persistent level lives there " "(or per-model under agent.reasoning_overrides).")) add("-t", "--toolsets", default=None, - help="Comma-separated toolsets to enable for this invocation. Applies to -z/--oneshot and --tui.") + help=("Comma-separated toolsets to enable for this invocation. Pass 'none' for an " + "explicitly tool-less session. Applies to -z/--oneshot and --tui.")) add("--resume", "-r", metavar="SESSION", default=None, help=( "Resume a previous session by ID or title, or pass 'latest' for " "the most recent session (workspace-scoped, like -c with no name)")) diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index cfddfc975caa..e89a731af584 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -162,10 +162,30 @@ def _configured_mcp_servers() -> tuple[set[str], set[str]]: def _validate_explicit_toolsets(toolsets: object = None) -> tuple[list[str] | None, str | None]: + from toolsets import ZERO_TOOLSET + + # A flag that was PASSED but names nothing is not "unspecified". Folding it into the + # config default is how `-t ""` — the natural way to ask for a tool-less probe — silently + # ran with the full configured toolset. Fail closed instead, and name the way to actually + # ask for zero tools (#126122). + if toolsets is not None and not _normalize_toolsets(toolsets): + return None, ( + "hermes -z: --toolsets was passed an empty value, which means \"unspecified\" — " + f"pass a toolset name, or '{ZERO_TOOLSET}' for an explicitly tool-less session.\n" + ) + normalized = _normalize_toolsets(toolsets) if normalized is None: return None, None + if ZERO_TOOLSET in normalized: + if len(normalized) > 1: + return None, ( + f"hermes -z: '{ZERO_TOOLSET}' means zero tools and cannot be combined with " + f"other toolsets: {', '.join(n for n in normalized if n != ZERO_TOOLSET)}\n" + ) + return [ZERO_TOOLSET], None + try: from toolsets import validate_toolset except Exception as exc: diff --git a/model_tools.py b/model_tools.py index 8566c62842cc..65293927d9e2 100644 --- a/model_tools.py +++ b/model_tools.py @@ -317,11 +317,16 @@ def _select_tool_names(enabled_toolsets: Optional[list[str]], disabled_toolsets: """Tool names requested by the toolset selection (before check_fn filtering).""" tools: set = set() if enabled_toolsets is not None: + from toolsets import ZERO_TOOLSET + enabled = list(enabled_toolsets) # Dispatcher-spawned kanban workers always get the lifecycle handoff # tools, even when the assignee profile restricts its chat toolsets. + # The exception is an explicit zero-tool request: the caller asked for a session that + # cannot act at all, and quietly adding kanban_* would contradict that (#126122). if (os.environ.get("HERMES_KANBAN_TASK") and not _is_delegated_child_context() - and _is_dispatcher_owned_worker() and "kanban" not in enabled): + and _is_dispatcher_owned_worker() and "kanban" not in enabled + and ZERO_TOOLSET not in enabled): enabled.append("kanban") _apply_toolset_selection(tools, enabled, quiet_mode, disable=False) else: diff --git a/tests/test_toolsets_zero_selection.py b/tests/test_toolsets_zero_selection.py new file mode 100644 index 000000000000..bee4b9efe67c --- /dev/null +++ b/tests/test_toolsets_zero_selection.py @@ -0,0 +1,156 @@ +"""``--toolsets`` must be able to say "zero tools", and must never silently mean the opposite. + +``hermes -z '' -t ''`` is the idiom people reach for when they want a tool-less +liveness probe or classifier. It did the opposite: ``_normalize_toolsets`` folds a falsy value +to ``None``, and ``None`` means "use the user's configured toolsets" — so the probe ran with +terminal, file writes and everything else, with nothing in the output saying so. + +Two contracts, both about the relationship between what was asked for and what is sent: + +* an EXPLICITLY EMPTY selection is an error, not a silent fallback to the config default; +* ``none`` is a real toolset meaning zero tools, and it survives the kanban worker's + automatic toolset injection (which exists so a worker can always hand its card back). + +Absent-vs-empty is a real distinction at the parser (``default=None``), so it is preserved +here: ``None`` still means "not specified". + +The same footgun existed on the interactive/one-query entry point (``hermes chat -q``), where +an empty value fell through to the coding posture or the platform default instead. Both call +sites now share one rule. + +Regression for #126122. +""" + +import pytest + +import cli +from hermes_cli.oneshot import _validate_explicit_toolsets +from model_tools import _select_tool_names +from toolsets import get_toolset, resolve_toolset, validate_toolset + + +class TestNoneIsARealToolset: + """``none`` must exist and mean exactly zero tools.""" + + def test_none_is_registered_and_validates(self): + assert validate_toolset("none"), "the sentinel must pass the same validation as any name" + assert get_toolset("none") is not None + + def test_none_resolves_to_zero_tools(self): + # resolve_toolset is documented to return a sorted List[str], not a set. + assert resolve_toolset("none") == [], ( + "the whole point of the sentinel is an empty tool list" + ) + + +class TestExplicitEmptyIsRejected: + """``-t ''`` asked for nothing; silently granting the configured toolsets is the bug.""" + + def test_empty_string_is_an_error_not_a_config_fallback(self): + toolsets, error = _validate_explicit_toolsets("") + assert error is not None, ( + f"an explicitly empty --toolsets silently resolved to {toolsets!r} " + "(None means 'use the configured toolsets')" + ) + assert toolsets is None + + @pytest.mark.parametrize("value", [" ", ",", " , "]) + def test_whitespace_and_bare_separators_are_also_errors(self, value): + _, error = _validate_explicit_toolsets(value) + assert error is not None, f"{value!r} carries no toolset name and must not fall back" + + def test_absent_flag_still_means_config_default(self): + """The absent case is unchanged — this is the behavior everything else relies on.""" + assert _validate_explicit_toolsets(None) == (None, None) + + +class TestNoneSentinelThroughTheOneShotPath: + """``-t none`` must survive validation as a real, empty selection.""" + + def test_none_alone_is_accepted(self): + toolsets, error = _validate_explicit_toolsets("none") + assert error is None + assert toolsets == ["none"] + + def test_none_combined_with_another_toolset_is_rejected(self): + """``none`` plus anything else is contradictory — ``none,web`` cannot mean zero tools.""" + _, error = _validate_explicit_toolsets("none,web") + assert error is not None, "combining the zero-tool sentinel with a real toolset must fail" + + +class TestNoneSurvivesTheKanbanInjection: + """A worker asked for zero tools gets zero tools. + + The dispatcher appends ``kanban`` so a worker can always hand its card back, which is + right for a normal selection and wrong for an explicit zero-tool request: the caller + asked for a session that cannot act at all. + """ + + def test_dispatcher_worker_gets_no_kanban_tools_when_none_requested(self, monkeypatch): + monkeypatch.setenv("HERMES_KANBAN_TASK", "card-123") + monkeypatch.setattr("model_tools._is_delegated_child_context", lambda: False) + monkeypatch.setattr("model_tools._is_dispatcher_owned_worker", lambda: True) + + assert _select_tool_names(["none"], None, quiet_mode=True) == set(), ( + "the kanban toolset was injected into an explicit zero-tool selection" + ) + + def test_dispatcher_worker_still_gets_kanban_for_a_normal_selection(self, monkeypatch): + """The negative case: the injection must survive for every other selection.""" + monkeypatch.setenv("HERMES_KANBAN_TASK", "card-123") + monkeypatch.setattr("model_tools._is_delegated_child_context", lambda: False) + monkeypatch.setattr("model_tools._is_dispatcher_owned_worker", lambda: True) + + tools = _select_tool_names(["web"], None, quiet_mode=True) + assert tools, "a normal toolset selection must still resolve tools" + assert any(t.startswith("kanban") for t in tools), ( + "a dispatcher-owned worker must keep the kanban lifecycle tools for a normal selection" + ) + + +def _build_cli(toolsets): + """Run cli._build_cli_from_args with a stub CLI and return the toolsets it was handed. + + The guard under test raises BEFORE HermesCLI is constructed, so the two error cases cost + nothing. The accept cases need construction, hence the stub: the assertion is on the + ``toolsets=`` the real call site would pass to the agent. + """ + captured = {} + + class _StubCLI: + def __init__(self, **kwargs): + captured.update(kwargs) + self.ignore_rules = kwargs.get("ignore_rules", False) + + original = cli.HermesCLI + cli.HermesCLI = _StubCLI + try: + cli._build_cli_from_args( + None, toolsets, None, None, None, None, None, None, + False, False, None, False, False, False, None, + ) + finally: + cli.HermesCLI = original + return captured.get("toolsets") + + +class TestChatOneQueryPath: + """``hermes chat -q`` resolves the same flag and must apply the same rule.""" + + def test_empty_value_is_rejected_before_anything_is_built(self): + with pytest.raises(ValueError, match="empty value"): + _build_cli("") + + def test_none_combined_with_another_toolset_is_rejected(self): + with pytest.raises(ValueError, match="cannot be combined"): + _build_cli("none,web") + + def test_none_alone_reaches_the_agent_as_the_sentinel(self): + assert _build_cli("none") == ["none"], ( + "an explicit zero-tool request must survive the resolver untouched" + ) + + def test_absent_flag_still_resolves_the_platform_default(self): + """The negative control: absent is NOT empty, and must keep working.""" + resolved = _build_cli(None) + assert resolved, "an absent --toolsets must still resolve a real toolset list" diff --git a/toolsets.py b/toolsets.py index 7572ad12850d..8ff837201e42 100644 --- a/toolsets.py +++ b/toolsets.py @@ -70,8 +70,16 @@ def _core_without(*excluded, kanban=True): # config: another surface lacking them made no configuration choice. CLIENT_SURFACE_TOOLSETS = frozenset({"project", "desktop_ui", "catalog"}) +# The toolset name that means "explicitly zero tools". Registered in TOOLSETS so it resolves +# through the same validate_toolset()/resolve_toolset() path as every other name, letting a +# caller SAY "no tools" instead of passing an empty value that reads as "unspecified" and +# silently receives the configured toolsets (#126122). +ZERO_TOOLSET = "none" + # Core toolset definitions: individual tools or references to other toolsets. TOOLSETS = { + ZERO_TOOLSET: _ts( + "No tools at all — an explicitly tool-less session (scripted one-shots, liveness probes)"), # Basic toolsets - individual tool categories "web": _ts("Web research and content extraction tools", ["web_search", "web_extract"]), "search": _ts("Web search only (no content extraction/scraping)", ["web_search"]),