mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-16 09:31:37 +00:00
fix(config): preserve YAML lists in hermes config set (#17876)
_set_nested unconditionally replaced any non-dict value with an empty dict when walking the dotted path, which silently destroyed list-typed config nodes the moment someone set a value with a numeric index (e.g. 'hermes config set custom_providers.0.api_key NEW'). Any sibling entries and any fields inside the targeted entry that the user didn't write were lost. Fix: - _set_nested now detects list nodes and navigates by numeric index, and preserves both dicts AND lists at intermediate positions (scalars are still replaced so bare-scalar -> nested overrides keep working). - set_config_value drops its duplicated navigation logic and calls _set_nested instead -- single source of truth for the rules. Regression tests (tests/hermes_cli/test_set_config_value.py): - test_indexed_set_preserves_sibling_list_entries -- exact #17876 repro - test_indexed_set_preserves_non_targeted_fields -- inner-dict fields survive - test_deeper_nesting_through_list -- dict -> list -> dict -> scalar path 35/35 existing + new tests pass. E2E-verified with the issue's repro against a real on-disk config.yaml -- list stays a list, entry 0 updated, entry 1 intact. Closes #17876
This commit is contained in:
Teknium
parent
3fc4c63d38
commit
b50bc13ef9
2 changed files with 135 additions and 18 deletions
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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"}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue