Skip to content

fix(auxiliary): pass base_url/api_key to _get_cached_client so multi-endpoint cache keys differ - #64242

Open
zmlgit wants to merge 2 commits into
NousResearch:mainfrom
zmlgit:fix/auxiliary-cache-key-base-url
Open

fix(auxiliary): pass base_url/api_key to _get_cached_client so multi-endpoint cache keys differ#64242
zmlgit wants to merge 2 commits into
NousResearch:mainfrom
zmlgit:fix/auxiliary-cache-key-base-url

Conversation

@zmlgit

@zmlgit zmlgit commented Jul 14, 2026

Copy link
Copy Markdown

Summary

resolve_vision_provider_client in agent/auxiliary_client.py calls _get_cached_client(...) to fetch a cached provider client, but was not passing base_url and api_key to the cache lookup. As a result, two vision calls with different base_url/api_key (e.g. two distinct custom OpenAI-compatible endpoints behind the same provider:model) would resolve to the same cached client — the first one registered — silently routing the second call to the wrong endpoint.

This PR threads the resolved base_url and api_key into _get_cached_client so the cache key distinguishes clients by endpoint + credentials, not just by provider+model.

Why

Cost-correctness for multi-endpoint auxiliary configurations. A common pattern is configuring multiple custom OpenAI-compatible endpoints (e.g. different Azure deployments, different regional gateways) under one provider block, varying only by base_url per request. Without this fix the second endpoint is never actually hit — the cached client from the first call is reused.

Changes

  • agent/auxiliary_client.py: pass base_url=resolved_base_url, api_key=resolved_api_key or None to _get_cached_client(...) in resolve_vision_provider_client. 2-line addition, no signature change to public APIs.

Tests

Local auxiliary suite: 691/692 pass on this branch. The single failure (test_skill_config_raw_cache_invalidates_on_config_edit in test_skill_utils.py) is pre-existing on origin/main and unrelated to this change.

Backward compatibility

  • Single-endpoint configurations are unaffected — base_url/api_key were already being computed for the actual client construction, they just weren'"'"'t fed back into the cache key.
  • The change is additive: existing cache entries continue to work for single-endpoint flows.

Related

Companion to #62061 (which addresses /anthropic-suffix normalization on the same call path). This PR is orthogonal: #62061 normalizes the base_url string before lookup; this PR makes the cache key honor the (already-normalized) base_url + api_key pair.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/vision Vision analysis and image generation labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the auxiliary routing path. The current implementation does not reproduce the base_url cache collision described in the PR.

Problems

  • agent/auxiliary_client.py:5476-5490 handles any non-empty resolved_base_url by calling resolve_provider_client directly with explicit_base_url and explicit_api_key; the cache call changed by this PR at agent/auxiliary_client.py:5645-5647 is therefore not reached for the reported scenario.
  • There is no regression test for the remaining possible scope. _resolve_task_provider_model can retain a per-task API key without a base URL (agent/auxiliary_client.py:6089, 6175), and that generic vision route currently omits it.

Suggested changes

  • Please re-scope the rationale to the API-key-only path, unless a current-HEAD base_url repro can be shown.
  • Add a regression test for that path; tests/agent/test_vision_resolved_args.py:40-64 is the nearby endpoint-routing coverage.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@zmlgit
zmlgit force-pushed the fix/auxiliary-cache-key-base-url branch from b5fd1b1 to a06980a Compare July 23, 2026 00:15
…y path + add regression tests

Per teknium1/hermes-sweeper review: the previous commit's narrative
described a 'base_url cache collision', but resolve_vision_provider_client
short-circuits at line 5594 (the 'if resolved_base_url:' branch) before
reaching the bottom _get_cached_client call (line 5772) that the fix
touched. So the described collision was unreachable.

The actual fix is still correct — for the API-KEY-ONLY path. When
_resolve_task_provider_model returns (provider, model, None, api_key, mode)
— i.e. config has provider + api_key but no base_url — the function
DOES fall through to the bottom _get_cached_client call. Without threading
api_key through (as the fix does), two profiles with same provider+model
but DIFFERENT api_keys would collide in the cache because the cache key
defaults api_key to empty string. Profile B's vision call would resolve
to profile A's cached client and authenticate with profile A's key.

This commit keeps the code change (already correct) and adds two
regression tests that pin the API-key-only contract:

- test_vision_api_key_only_path_threads_api_key_into_cache_call:
  patches _resolve_task_provider_model to return the api-key-only tuple,
  calls resolve_vision_provider_client, asserts _get_cached_client
  received api_key='sk_profile_A' (not None — None is the collision bug).

- test_vision_api_key_only_cache_keys_differentiate_by_api_key:
  calls _client_cache_key directly with same provider+model+empty base_url
  but different api_keys; asserts the keys differ. Sanity-checks that
  the empty-api_key path (pre-fix shape) is distinct from both, so a
  regression that drops the api_key thread would re-introduce the
  collision.

All 4 tests in tests/agent/test_vision_resolved_args.py pass; 370
adjacent tests/agent/test_auxiliary_client*.py tests pass.
@zmlgit

zmlgit commented Jul 27, 2026

Copy link
Copy Markdown
Author

Re-scoped per sweeper review (d332ce98a). Keeping the existing code change (it's correct) and adding regression tests for the ACTUAL path the fix protects.

On sweeper point 1 ('base_url collision not reproducible'): correct. resolve_vision_provider_client short-circuits at line 5594 (the if resolved_base_url: branch) and never reaches the bottom _get_cached_client call (line 5772) that this PR's diff touched. The base_url collision narrative was wrong.

On sweeper point 2 ('re-scope to API-key-only path'): done. The actual collision vector is API-key-only: when _resolve_task_provider_model returns (provider, model, None, api_key, mode) — i.e. config has provider + api_key but NO base_url — the function falls through to the bottom _get_cached_client call. Without threading api_key into that call (as this PR does), two profiles with the same provider+model but DIFFERENT api_keys would collide in the cache: _client_cache_key defaults api_key to empty string when None is passed, so both profiles map to the same cache entry, and profile B's vision call would authenticate with profile A's key. The existing one-line fix at line 5772-5776 closes that vector.

On sweeper point 3 ('add a regression test for that path'): two new tests in tests/agent/test_vision_resolved_args.py:

  • test_vision_api_key_only_path_threads_api_key_into_cache_call — patches _resolve_task_provider_model to return the API-key-only tuple, calls resolve_vision_provider_client, asserts _get_cached_client received api_key='sk_profile_A' (not None — None IS the collision bug). Also pins that base_url=None is passed through (not silently dropped), so the cache key stays well-formed.

  • test_vision_api_key_only_cache_keys_differentiate_by_api_key — verifies the fix at the cache-key layer: _client_cache_key called with same provider+model+empty base_url but different api_keys produces DIFFERENT keys. Sanity-checks that the empty-api_key path (the pre-fix shape) is distinct from both, so a regression that drops the api_key thread would surface here.

All 4 tests in tests/agent/test_vision_resolved_args.py pass; 370 adjacent tests/agent/test_auxiliary_client*.py tests pass.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants