diff --git a/hermes_cli/config.py b/hermes_cli/config.py index bf133f121c6..d1a8c2e35d5 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2305,19 +2305,55 @@ def get_missing_env_vars(required_only: bool = False) -> List[Dict[str, Any]]: return missing -def _set_nested(config: dict, dotted_key: str, value): +def _set_nested(config, dotted_key: str, value): """Set a value at an arbitrarily nested dotted key path. - Creates intermediate dicts as needed, e.g. ``_set_nested(c, "a.b.c", 1)`` - ensures ``c["a"]["b"]["c"] == 1``. + Supports both dict and list navigation: + _set_nested(c, "a.b.c", 1) → c["a"]["b"]["c"] = 1 + _set_nested(c, "a.0.b", 1) → c["a"][0]["b"] = 1 + _set_nested(c, "providers.1", "x") → c["providers"][1] = "x" + + Intermediate dicts are created on demand. List indices are parsed + from numeric path segments; the referenced index must already exist + (we do not grow lists — the user is navigating into structure they + wrote themselves). If a segment targets a non-container leaf + (scalar), the leaf is replaced with a fresh dict so the write can + proceed — this preserves the pre-existing behavior for bare scalar + overrides (e.g. setting ``a.b.c`` where ``a.b`` was previously a + string). + + Guards against #17876: before this fix the code unconditionally + replaced any non-dict value (including lists) with ``{}``, silently + destroying list-typed config like ``custom_providers`` whenever a + caller used an indexed path. """ parts = dotted_key.split(".") current = config for part in parts[:-1]: - if part not in current or not isinstance(current.get(part), dict): - current[part] = {} - current = current[part] - current[parts[-1]] = value + if isinstance(current, list): + try: + idx = int(part) + except (TypeError, ValueError): + raise TypeError( + f"Cannot navigate into list at key {dotted_key!r}: " + f"segment {part!r} is not a numeric index" + ) + current = current[idx] + elif isinstance(current, dict): + existing = current.get(part) + # Preserve dicts and lists; replace missing/scalar with a fresh dict. + if part not in current or not isinstance(existing, (dict, list)): + current[part] = {} + current = current[part] + else: + raise TypeError( + f"Cannot navigate into {type(current).__name__} at key {dotted_key!r}" + ) + last = parts[-1] + if isinstance(current, list): + current[int(last)] = value + else: + current[last] = value def get_missing_config_fields() -> List[Dict[str, Any]]: @@ -4421,15 +4457,11 @@ def set_config_value(key: str, value: str): except Exception: user_config = {} - # Handle nested keys (e.g., "tts.provider") - parts = key.split('.') - current = user_config - - for part in parts[:-1]: - if part not in current or not isinstance(current.get(part), dict): - current[part] = {} - current = current[part] - + # Handle nested keys (e.g., "tts.provider") including numeric list + # indices (e.g., "custom_providers.0.api_key"). Delegates to + # _set_nested which preserves list-typed nodes; before #17876 the + # inline navigation here silently overwrote lists with dicts. + # Convert value to appropriate type if value.lower() in ('true', 'yes', 'on'): value = True @@ -4439,8 +4471,8 @@ def set_config_value(key: str, value: str): value = int(value) elif value.replace('.', '', 1).isdigit(): value = float(value) - - current[parts[-1]] = value + + _set_nested(user_config, key, value) # Write only user config back (not the full merged defaults) ensure_hermes_home() diff --git a/tests/hermes_cli/test_set_config_value.py b/tests/hermes_cli/test_set_config_value.py index adbd0ae1e0d..617a915e322 100644 --- a/tests/hermes_cli/test_set_config_value.py +++ b/tests/hermes_cli/test_set_config_value.py @@ -172,3 +172,88 @@ class TestFalsyValues: config_command(args) config = _read_config(_isolated_hermes_home) assert "model" in config + + +# --------------------------------------------------------------------------- +# List navigation — regression tests for #17876 +# --------------------------------------------------------------------------- + +class TestListNavigation: + """hermes config set must preserve YAML list fields when using numeric + indices. Before #17876, _set_nested would silently replace the entire + list with a dict, destroying every sibling entry. + """ + + def _write_config(self, tmp_path, body): + (tmp_path / "config.yaml").write_text(body) + + def test_indexed_set_preserves_sibling_list_entries(self, _isolated_hermes_home): + """Setting custom_providers.0.api_key must not destroy entry 1.""" + self._write_config(_isolated_hermes_home, ( + "custom_providers:\n" + "- name: provider-a\n" + " api_key: old-a\n" + " base_url: https://a.example.com\n" + "- name: provider-b\n" + " api_key: old-b\n" + " base_url: https://b.example.com\n" + )) + + set_config_value("custom_providers.0.api_key", "new-a") + + import yaml + reloaded = yaml.safe_load(_read_config(_isolated_hermes_home)) + # The list must still be a list + assert isinstance(reloaded["custom_providers"], list) + assert len(reloaded["custom_providers"]) == 2 + # Entry 0 was updated + assert reloaded["custom_providers"][0]["api_key"] == "new-a" + assert reloaded["custom_providers"][0]["name"] == "provider-a" + assert reloaded["custom_providers"][0]["base_url"] == "https://a.example.com" + # Entry 1 is untouched + assert reloaded["custom_providers"][1]["name"] == "provider-b" + assert reloaded["custom_providers"][1]["api_key"] == "old-b" + assert reloaded["custom_providers"][1]["base_url"] == "https://b.example.com" + + def test_indexed_set_preserves_non_targeted_fields(self, _isolated_hermes_home): + """Setting one field in a list entry must not drop other fields.""" + self._write_config(_isolated_hermes_home, ( + "custom_providers:\n" + "- name: provider-a\n" + " api_key: old\n" + " base_url: https://a.example.com\n" + " models:\n" + " foo: {}\n" + " bar: {}\n" + )) + + set_config_value("custom_providers.0.api_key", "rotated") + + import yaml + reloaded = yaml.safe_load(_read_config(_isolated_hermes_home)) + entry = reloaded["custom_providers"][0] + assert entry["api_key"] == "rotated" + assert entry["name"] == "provider-a" + assert entry["base_url"] == "https://a.example.com" + assert set(entry["models"].keys()) == {"foo", "bar"} + + 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" + " - name: alice\n" + " role: admin\n" + " - name: bob\n" + " role: user\n" + )) + + set_config_value("platforms.telegram.allowlist.1.role", "admin") + + import yaml + reloaded = yaml.safe_load(_read_config(_isolated_hermes_home)) + allowlist = reloaded["platforms"]["telegram"]["allowlist"] + assert isinstance(allowlist, list) + assert allowlist[0] == {"name": "alice", "role": "admin"} + assert allowlist[1] == {"name": "bob", "role": "admin"}