hermes config set corrupts YAML list fields

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #17893 fix(config): preserve YAML lists in hermes config set (#17876)

  merged to main

  files: hermes_cli/config.py, tests/hermes_cli/test_set_config_value.py

  Show the diff

  • 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()

  • tests/hermes_cli/test_set_config_value.py

    @@ -172,3 +172,88 @@ def test_config_command_accepts_empty_string(self, _isolated_hermes_home):
             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: [redacted:secret-assignment]"
    +            "  base_url: https://a.example.com\n"
    +            "- name: provider-b\n"
    +            "  api_key: [redacted:secret-assignment]"
    +            "  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"}