Skip to content

fix(config): stop _normalize_custom_provider_entry mutating the caller's dict - #881

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57096
Open

fix(config): stop _normalize_custom_provider_entry mutating the caller's dict#881
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57096

Conversation

@hashbender

Copy link
Copy Markdown
Owner

What does this PR do?

Stops _normalize_custom_provider_entry (hermes_cli/config.py) from mutating the dict it is passed.

The normalizer builds and returns a fresh normalized dict, but on the way it writes alias keys into its input:

if "api_key_env" in entry and "key_env" not in entry:
    entry["key_env"] = entry["api_key_env"]
...
for camel, snake in _CAMEL_ALIASES.items():
    if camel in entry and snake not in entry:
        entry[snake] = entry[camel]

Two of its three callers — get_compatible_custom_providers and providers_dict_to_custom_providers — pass live sub-dicts from load_config_readonly()'s shared cache, which has an explicit no-mutation contract. (The third caller, _custom_provider_entry_to_provider_config, already defends itself with dict(entry) — evidence the hazard was known but two paths were missed.)

Consequences for a config written with the documented camelCase / api_key_env aliases:

  • the cached config gains injected duplicate keys (baseUrl and base_url, apiKeyEnv and key_env), so every later load_config() deepcopy inherits them, and any save_config(load_config()) flow (setup wizard, dashboard config writes, model persist) writes the duplicates back to config.yaml;
  • the recent custom-provider TLS work made this hot-path: _resolve_aux_verify runs it on every auxiliary-client build (compression, vision, title-gen), i.e. an unlocked dict mutation on a shared object from worker threads;
  • open PR perf: use load_config_readonly() at read-only call sites in agent/ NousResearch/hermes-agent#56085 (load_config_readonly() at 31 read-only call sites in agent/) widens the blast radius further — several of those sites reach this normalizer.

Repro on current main:

cfg = {"custom_providers": [{"name": "x", "baseUrl": "https://a/v1", "apiKeyEnv": "K"}]}
get_compatible_custom_providers(cfg)
cfg["custom_providers"][0].keys()
# dict_keys(['name', 'baseUrl', 'apiKeyEnv', 'base_url', 'key_env'])  ← polluted

The fix is a shallow entry = dict(entry) at the top of the normalizer. The return value is a separately-built dict, so nothing else changes.

Related Issue

None filed. Alias handling introduced for NousResearch#9332; the readonly-cache interaction is what makes it bite.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

  • hermes_cli/config.py — shallow-copy the entry before alias normalization (+ comment explaining why).
  • tests/hermes_cli/test_provider_config_validation.py — 3 regression tests: camelCase entry unchanged, api_key_env entry unchanged, and get_compatible_custom_providers leaves the whole config deep-equal to a snapshot.

How to Test

scripts/run_tests.sh tests/hermes_cli/test_provider_config_validation.py

All 3 new tests fail on current main and pass with the fix; the file's other 21 tests pass on both. Also green: tests/hermes_cli/test_config.py, test_custom_provider_tls.py, test_runtime_provider_resolution.py, tests/agent/test_auxiliary_named_custom_providers.py (317 tests, macOS 15). scripts/check-windows-footguns.py clean.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (N/A — internal contract fix)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

Mirror-of: NousResearch#57096
NousResearch#57096

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant