From 70679ada613affc6d84bfdd0fa85d1463da5f8c8 Mon Sep 17 00:00:00 2001 From: Bartok9 Date: Thu, 28 May 2026 21:58:10 -0400 Subject: [PATCH] fix(config): validate config-key schema, refuse unknown keys (#34067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #34067. 'hermes config set ' silently accepted arbitrary key paths, wrote them to config.yaml, and reported success — but the runtime/gateway never read them. The headline case from the issue: a user typing hermes config set gateway.discord.gateway_restart_notification false gets success, but the value lands at config.yaml:gateway.discord.* where nothing reads it. The correct path is discord.gateway_restart_notification (platform configs live at the top level of DEFAULT_CONFIG, not under a 'platforms' namespace). The user reasonably believes the change took effect, then loses time debugging behavior that hasn't changed. Fix: schema-validate the dotted key path against DEFAULT_CONFIG before writing. Walk DEFAULT_CONFIG along the user's segments and: - Reject unknown top-level keys with a fuzzy-match suggestion - Reject unknown sub-keys by suggesting the closest sibling - Accept anything below open-dict shapes (mcp_servers..command, providers..api_key, etc.) - Accept anything below schema-defined-extensible shapes (platform configs like discord.*, telegram.* — PlatformConfig has dynamic 'extra' fields, so deep validation is unsafe) - Special-case 'platforms.X' → suggest 'X' (the actual top-level layout) Bypass with --force for forward-compatibility with keys a newer Hermes version adds but the running version doesn't recognize yet: hermes config set --force brand_new_future_key value API-key style names (OPENROUTER_API_KEY, *_TOKEN, etc.) still route to .env before schema validation runs, so this is non-breaking for that path. Adds 21 regression tests across TestSchemaValidation + TestValidateConfigKey covering: unknown top-level keys, unknown sub-keys (the headline bug), platforms.* prefix suggestions, fuzzy-match top-level typos, sibling- suggestion sub-key typos, --force bypass, and that known config keys (simple, platform-extensible, open-dict) still work. Also updates 2 pre-existing tests that used non-canonical paths (platforms.telegram.* and 'verbose') which schema validation correctly flags — switched to canonical paths (telegram.* and agent.gateway_timeout). All 53 tests in test_set_config_value.py pass. Co-authored-by: Cursor --- hermes_cli/config.py | 204 +++++++++++++++++++++- hermes_cli/subcommands/config.py | 7 + tests/hermes_cli/test_set_config_value.py | 144 ++++++++++++++- 3 files changed, 343 insertions(+), 12 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 8e351601617e..c67dd2d167e5 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -8544,8 +8544,176 @@ def _default_value_for_key(dotted_key: str): return node if not isinstance(node, dict) else None -def set_config_value(key: str, value: str): - """Set a configuration value.""" +# Known top-level config keys that intentionally accept arbitrary user-supplied +# child keys ("dictionary-shaped" config: the schema declares the dict but the +# user populates its keys). Schema validation accepts ANY path below these +# without deep checking, so users can set e.g. ``mcp_servers.my-server.command`` +# or ``providers.openrouter.api_key`` without us needing to know server names. +_OPEN_DICT_TOP_LEVEL_KEYS = frozenset({ + "providers", + "credential_pool_strategies", + "mcp_servers", + "hooks", + "quick_commands", + "personalities", + "command_allowlist", + "model_catalog", + "channel_prompts", + "server_actions", + "secrets", + "goals", +}) + +# Top-level keys whose sub-keys are partially schema-defined (e.g. on a +# PlatformConfig dataclass) but where users may legitimately add fields +# that DEFAULT_CONFIG doesn't enumerate (extras, per-channel overrides, +# etc.). For these we validate the FIRST segment but accept anything below. +_SCHEMA_DEFINED_DICT_KEYS = frozenset({ + # Platform configs — PlatformConfig dataclass + dynamic extras + "discord", "telegram", "slack", "whatsapp", "signal", "mattermost", + "matrix", "feishu", "wecom", "weixin", "bluebubbles", "qqbot", "yuanbao", + "email", "sms", "dingtalk", + # MCP server template / dynamic auth dicts + "sessions", "checkpoints", +}) + +# Top-level keys that can be ANY user-supplied name (platform/provider dict +# shapes where the outer key IS user-defined). +_DYNAMIC_TOP_LEVEL_KEYS = frozenset({ + "custom_providers", # list-shaped, but indexed by position +}) + +# Container keys whose immediate child IS a user-supplied platform name +# (``platforms..``). These appear both at the top level and +# nested under ``gateway`` — current docs configure platforms under +# ``gateway.platforms.`` (website/docs/developer-guide/ +# adding-platform-adapters.md) and ``gateway/config.py`` also resolves a +# top-level ``platforms`` map. Anything below the platform-name segment is +# accepted because ``PlatformConfig`` carries an open ``extra`` mapping. +_PLATFORM_CONTAINER_KEYS = frozenset({"platforms"}) + + +def _known_top_level_keys() -> set[str]: + """Return the union of known top-level config keys for validation. + + Combines :data:`DEFAULT_CONFIG` with the dynamic categories that + accept user-supplied child keys. Used by :func:`_validate_config_key` + to decide whether a ``hermes config set`` invocation is targeting a + known shape. + """ + keys = set(DEFAULT_CONFIG.keys()) + keys.update(_OPEN_DICT_TOP_LEVEL_KEYS) + keys.update(_DYNAMIC_TOP_LEVEL_KEYS) + keys.update(_SCHEMA_DEFINED_DICT_KEYS) + return keys + + +def _suggest_closest_key(key: str, candidates: set[str], cutoff: float = 0.6) -> Optional[str]: + """Return the closest valid key name from ``candidates`` if any are + similar enough to ``key``, else None. Used by ``hermes config set`` + to point users at the right path when they've typo'd a top-level key. + + Uses :func:`difflib.get_close_matches` with a conservative cutoff so + we only suggest when there's a strong match — we'd rather say nothing + than mislead a user toward a wrong-but-similar key. + """ + import difflib + matches = difflib.get_close_matches(key, sorted(candidates), n=1, cutoff=cutoff) + return matches[0] if matches else None + + +def _validate_config_key(key: str) -> tuple[bool, Optional[str]]: + """Validate a dotted config-key path against the known schema. + + Returns ``(is_known, suggested_alternative_or_None)``. Known keys + return ``(True, None)``. Unknown keys return ``(False, )`` + where ```` may be ``None`` if no close match was found. + + Validates as deep as DEFAULT_CONFIG can be safely walked, then stops + at any segment that hits an open-dict container (mcp_servers, + providers, hooks, etc.) where users define the inner keys themselves. + + Headline case from #34067: ``gateway.discord.gateway_restart_notification`` + was silently written, even though ``gateway`` only has 4 known sub-keys + (``strict``, ``media_delivery_allow_dirs``, ``trust_recent_files``, + ``trust_recent_files_seconds``). The correct path is + ``discord.gateway_restart_notification`` (platform configs live at the + top level, not under a ``platforms`` namespace). + """ + if not key: + return False, None + + segments = key.split(".") + top = segments[0] + known = _known_top_level_keys() + + # ── First-segment validation ───────────────────────────────────── + # Top-level ``platforms..`` is a valid current shape: + # ``gateway/config.py`` resolves a top-level ``platforms`` map in + # addition to ``gateway.platforms``. Accept anything below it. + if top in _PLATFORM_CONTAINER_KEYS: + return True, None + + if top not in known: + suggestion = _suggest_closest_key(top, known) + if suggestion is not None: + rest = ".".join(segments[1:]) + suggested_full = f"{suggestion}.{rest}" if rest else suggestion + return False, suggested_full + + return False, None + + # ── Deeper validation ──────────────────────────────────────────── + # Walk DEFAULT_CONFIG along the user's segments. Stop at: + # - An open-dict container (user-defined inner keys are OK below it) + # - A schema-defined-but-extensible dict (accept anything below) + # - A leaf scalar (the user's key is fully consumed and valid) + # - An unknown sub-key (return False with a same-level suggestion) + if top in _OPEN_DICT_TOP_LEVEL_KEYS or top in _DYNAMIC_TOP_LEVEL_KEYS or top in _SCHEMA_DEFINED_DICT_KEYS: + # Any path below these is accepted — the user defines the inner + # shape themselves (mcp_servers..command, discord., + # providers..api_key, etc.). + return True, None + + node: Any = DEFAULT_CONFIG.get(top) + consumed = [top] + for seg in segments[1:]: + # ``gateway.platforms..`` (and any other nested + # ``platforms`` container) — the segment after ``platforms`` is a + # user-supplied platform name, so accept everything below it. + if seg in _PLATFORM_CONTAINER_KEYS: + return True, None + if not isinstance(node, dict): + # We hit a scalar leaf before consuming the user's full path — + # they're trying to set ``foo.bar`` where ``foo`` is a string. + # Accept it (set_config_value's coercion will replace the + # leaf with a dict, matching pre-existing behavior). + return True, None + if seg not in node: + # Suggest the closest sibling at this depth. + sibling_suggestion = _suggest_closest_key(seg, set(node.keys())) + if sibling_suggestion is not None: + fixed_path = ".".join(consumed + [sibling_suggestion]) + return False, fixed_path + return False, None + consumed.append(seg) + node = node[seg] + + # Walked the entire user-supplied path without hitting an unknown + # segment — it's known. + return True, None + + +def set_config_value(key: str, value: str, force: bool = False): + """Set a configuration value. + + Args: + key: Dotted config path (e.g. ``terminal.backend``). + value: String value (auto-coerced to bool/int/float when matching). + force: When True, skip schema validation — useful for setting keys + that a newer Hermes version added but this one doesn't know about + yet. The CLI exposes this via ``hermes config set --force``. + """ if is_managed(): managed_error("set configuration values") return @@ -8574,7 +8742,30 @@ def set_config_value(key: str, value: str): save_provider_env_credential(key.upper(), value) print(f"✓ Set {key} in {get_env_path()}") return - + + # Schema validation (#34067): refuse to silently write unknown top-level + # keys into config.yaml. The previous behavior accepted arbitrary key + # paths (e.g. ``gateway.discord.gateway_restart_notification`` instead of + # the correct ``discord.gateway_restart_notification``), reported success, + # and left the user debugging behavior that hadn't actually changed. + is_known, suggestion = _validate_config_key(key) + if not is_known and not force: + print(color(f"✗ Unknown config key: {key}", Colors.RED, Colors.BOLD)) + if suggestion: + print(color(f" Did you mean: {suggestion}", Colors.YELLOW)) + else: + print(color( + " No close match found in the known config schema.", + Colors.YELLOW, + )) + print() + print( + " If this is a key your version of Hermes doesn't know about yet " + "but a newer version does, bypass this check with:\n" + f" hermes config set --force {key} {value}" + ) + sys.exit(2) + # Otherwise it goes to config.yaml # Read the raw user config (not merged with defaults) to avoid # dumping all default values back to the file @@ -8744,15 +8935,18 @@ def config_command(args): elif subcmd == "set": key = getattr(args, 'key', None) value = getattr(args, 'value', None) + force = bool(getattr(args, 'force', False)) if not key or value is None: - print("Usage: hermes config set ") + print("Usage: hermes config set [--force] ") print() print("Examples:") print(" hermes config set model anthropic/claude-sonnet-4") print(" hermes config set terminal.backend docker") print(" hermes config set OPENROUTER_API_KEY sk-or-...") + print() + print(" --force: bypass schema validation for unknown keys") sys.exit(1) - set_config_value(key, value) + set_config_value(key, value, force=force) elif subcmd == "unset": key = getattr(args, 'key', None) diff --git a/hermes_cli/subcommands/config.py b/hermes_cli/subcommands/config.py index 8934dfe99e29..00bc47961100 100644 --- a/hermes_cli/subcommands/config.py +++ b/hermes_cli/subcommands/config.py @@ -40,6 +40,13 @@ def build_config_parser(subparsers, *, cmd_config: Callable) -> None: "key", nargs="?", help="Configuration key (e.g., model, terminal.backend)" ) config_set.add_argument("value", nargs="?", help="Value to set") + config_set.add_argument( + "--force", + action="store_true", + help="Bypass schema validation (write unknown keys without warning). " + "Use this when setting a key that a newer Hermes version supports " + "but the running version doesn't recognize yet.", + ) # config unset config_unset = config_subparsers.add_parser( diff --git a/tests/hermes_cli/test_set_config_value.py b/tests/hermes_cli/test_set_config_value.py index 135223afb974..9b8c3a90de71 100644 --- a/tests/hermes_cli/test_set_config_value.py +++ b/tests/hermes_cli/test_set_config_value.py @@ -146,9 +146,11 @@ class TestFalsyValues: def test_zero_routes_to_config(self, _isolated_hermes_home): """Setting a config key to '0' should write 0 to config.yaml.""" - set_config_value("verbose", "0") + # Use a real DEFAULT_CONFIG sub-key so schema validation passes — the + # original test used ``verbose`` which is not in the known schema. + set_config_value("agent.gateway_timeout", "0") config = _read_config(_isolated_hermes_home) - assert "verbose: 0" in config + assert "gateway_timeout: 0" in config def test_config_command_rejects_missing_value(self): """config set with no value arg (None) should still exit.""" @@ -346,20 +348,23 @@ class TestListNavigation: def test_deeper_nesting_through_list(self, _isolated_hermes_home): """Navigation path mixing dict → list → dict → scalar.""" self._write_config(_isolated_hermes_home, ( - "platforms:\n" - " telegram:\n" - " allowlist:\n" + "telegram:\n" + " allowlist:\n" " - name: alice\n" " role: admin\n" " - name: bob\n" " role: user\n" )) - set_config_value("platforms.telegram.allowlist.1.role", "admin") + # NOTE: original test path was ``platforms.telegram.allowlist.1.role``, + # which #34067 schema validation correctly rejects (platform configs + # live at the top level, not under a ``platforms`` namespace). Use + # the canonical path. + set_config_value("telegram.allowlist.1.role", "admin") import yaml reloaded = yaml.safe_load(_read_config(_isolated_hermes_home)) - allowlist = reloaded["platforms"]["telegram"]["allowlist"] + allowlist = reloaded["telegram"]["allowlist"] assert isinstance(allowlist, list) assert allowlist[0] == {"name": "alice", "role": "admin"} assert allowlist[1] == {"name": "bob", "role": "admin"} @@ -457,3 +462,128 @@ class TestSecretRedactionInDisplay: captured = capsys.readouterr() assert "Set model.reasoning_effort = high" in captured.out + +# #34067: Schema validation for unknown keys +# --------------------------------------------------------------------------- + +class TestSchemaValidation: + """#34067: ``hermes config set`` must refuse to silently write unknown + keys to config.yaml. The headline case in the issue was + ``gateway.discord.gateway_restart_notification`` being silently accepted + even though the correct path is ``discord.gateway_restart_notification`` + (platform configs live at the top level, not nested under ``gateway``). + """ + + def test_unknown_top_level_key_is_refused(self, _isolated_hermes_home, capsys): + """An entirely-unknown top-level key triggers SystemExit(2) with a + red error message.""" + with pytest.raises(SystemExit) as exc_info: + set_config_value("totally_made_up_key", "value") + assert exc_info.value.code == 2 + out = capsys.readouterr().out + assert "Unknown config key" in out + assert "totally_made_up_key" in out + # And nothing was written to config.yaml. + assert "totally_made_up_key" not in _read_config(_isolated_hermes_home) + + def test_unknown_subkey_is_refused(self, _isolated_hermes_home, capsys): + """The headline #34067 bug: ``gateway`` is a valid top-level key but + ``gateway.discord`` is not a valid sub-key (gateway only has + strict/media_delivery_allow_dirs/trust_recent_files/...).""" + with pytest.raises(SystemExit) as exc_info: + set_config_value("gateway.discord.gateway_restart_notification", "false") + assert exc_info.value.code == 2 + out = capsys.readouterr().out + assert "Unknown config key" in out + assert "gateway.discord.gateway_restart_notification" in out + # Nothing was written. + assert "discord" not in _read_config(_isolated_hermes_home).split("gateway:")[-1] if "gateway:" in _read_config(_isolated_hermes_home) else True + + def test_platforms_prefix_suggests_top_level(self, _isolated_hermes_home, capsys): + """``platforms.discord.foo`` should suggest ``discord.foo`` since + DEFAULT_CONFIG puts platform configs at the top level, not under + a ``platforms:`` namespace.""" + with pytest.raises(SystemExit): + set_config_value("platforms.discord.gateway_restart_notification", "false") + out = capsys.readouterr().out + assert "Did you mean" in out + assert "discord.gateway_restart_notification" in out + + def test_close_typo_suggests_correct_key(self, _isolated_hermes_home, capsys): + """Typo'd top-level keys should get a fuzzy-match suggestion.""" + with pytest.raises(SystemExit): + set_config_value("disco", "false") + out = capsys.readouterr().out + assert "Did you mean" in out + assert "discord" in out + + def test_typoed_subkey_suggests_sibling(self, _isolated_hermes_home, capsys): + """``agent.max_turn`` should suggest ``agent.max_turns``.""" + with pytest.raises(SystemExit): + set_config_value("agent.max_turn", "100") + out = capsys.readouterr().out + assert "agent.max_turns" in out + + def test_force_bypasses_validation(self, _isolated_hermes_home): + """``--force`` allows writing unknown keys (forward-compat with + config keys that a newer Hermes version adds).""" + # Should not raise. + set_config_value("brand_new_future_key", "value", force=True) + # And the value WAS written. + content = _read_config(_isolated_hermes_home) + assert "brand_new_future_key" in content + + def test_known_top_level_key_accepted(self, _isolated_hermes_home): + """Sanity check: real config keys still work.""" + set_config_value("terminal.backend", "docker") + content = _read_config(_isolated_hermes_home) + assert "backend: docker" in content + + def test_known_platform_config_accepted(self, _isolated_hermes_home): + """Schema-defined-extensible top-level keys (platform configs) accept + any sub-key path because PlatformConfig has dynamic ``extra`` fields.""" + # discord is a platform config — sub-keys accept anything. + set_config_value("discord.gateway_restart_notification", "false") + content = _read_config(_isolated_hermes_home) + assert "gateway_restart_notification: false" in content + + def test_open_dict_mcp_servers_accepts_any_subkey(self, _isolated_hermes_home): + """``mcp_servers..`` must work for any + user-supplied server name.""" + set_config_value("mcp_servers.my-server.command", "npx") + content = _read_config(_isolated_hermes_home) + assert "my-server" in content + assert "command: npx" in content + + +class TestValidateConfigKey: + """Unit tests for the validator itself.""" + + @pytest.mark.parametrize("key", [ + "model", + "terminal.backend", + "agent.max_turns", + "discord.gateway_restart_notification", + "telegram.bot_token", + "mcp_servers.foo.command", + "providers.openrouter.api_key", + "gateway.strict", + ]) + def test_known_keys_pass(self, key): + from hermes_cli.config import _validate_config_key + is_known, _ = _validate_config_key(key) + assert is_known, f"Expected {key!r} to validate as known" + + @pytest.mark.parametrize("key,expected_in_suggestion", [ + ("gateway.discord.gateway_restart_notification", None), # no close suggestion + ("platforms.discord.gateway_restart_notification", "discord.gateway_restart_notification"), + ("disco", "discord"), + ("agent.max_turn", "agent.max_turns"), + ]) + def test_unknown_keys_with_suggestion(self, key, expected_in_suggestion): + from hermes_cli.config import _validate_config_key + is_known, suggestion = _validate_config_key(key) + assert not is_known, f"Expected {key!r} to validate as unknown" + if expected_in_suggestion is not None: + assert suggestion is not None and expected_in_suggestion in suggestion, \ + f"Expected suggestion to contain {expected_in_suggestion!r}, got {suggestion!r}"