Skip to content

fix(auxiliary_client): preserve named user-defined provider on explicit base_url - #76668

Open
Kewe63 wants to merge 2 commits into
NousResearch:mainfrom
Kewe63:fix/76602-auxiliary-named-custom-provider-key
Open

Kewe63 wants to merge 2 commits into
NousResearch:mainfrom
Kewe63:fix/76602-auxiliary-named-custom-provider-key

Conversation

@Kewe63

@Kewe63 Kewe63 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #76602.

When async_call_llm(task='vision') (and any other auxiliary path that re-passes the config-resolved provider) forwards provider=<name> + base_url=... + api_key=None to _resolve_task_provider_model, the helper consulted only the built-in provider registry via hermes_cli.providers.get_provider. A name defined in the providers: section of config.yaml (e.g. agnes-ai.cn, nvidia-nim with key_env) missed that lookup and fell through to the anonymous "custom" downgrade at agent/auxiliary_client.py:7110. The downgrade routed the call through the bare-custom branch in resolve_provider_client, which has no key, sent Authorization: Bearer no-key-required and produced a 401 from any auth-required provider.

Direct API calls with the same key (from config) succeed — the key itself is valid. The downgrade path also explains the "sometimes works / sometimes not" pattern: provider=None paths succeed (config-only resolution runs the named-custom-provider branch); explicit provider=<name> paths always 401.


Fix

Add a second lookup against hermes_cli.runtime_provider._get_named_custom_provider (the same helper call_llm uses for main-runtime resolution) so a named user-defined provider + explicit base_url stays named through to the named-custom-provider key-resolution branch in resolve_provider_client, which then picks up providers.<name>.api_key or the configured key_env.

Both lookups run inside the existing try / except guard, so a catalog-load failure (early import, missing config) still falls back to the hardcoded allowlist (xai-oauth / qwen-oauth / etc.). The "custom" / "auto" / "custom:" short-circuit is unchanged — the only behavioral change is that the second lookup widens the True set without ever widening False.


Files Changed

agent/auxiliary_client.py (+23 / -1)

  • _preserve_provider_with_base_url adds the _get_named_custom_provider lookup after the built-in registry miss. Same try/except envelope so an import-time failure doesn't widen True.

tests/agent/test_auxiliary_client.py (+146) — new TestPreserveNamedCustomProviderWithBaseUrl class with 4 tests:

  • test_user_defined_provider_named_in_config_is_preserved — the repro
  • test_built_in_provider_with_base_url_still_preserved — no built-in regression
  • test_unknown_provider_with_base_url_falls_back_to_custom_downgrade — pre-existing "custom" downgrade preserved for truly anonymous endpoints
  • test_hardcoded_allowlist_still_works_when_both_registries_unavailable — early-import path keeps xai-oauth / qwen-oauth / etc.

Test Results

pytest tests/agent/test_auxiliary_client.py

✅ 166/166 passed (162 existing + 4 new), no regressions.


Risk

  • Blast radius: contained to agent/auxiliary_client.py_preserve_provider_with_base_url is a single-purpose predicate that only widens its True set; it cannot widen False, cannot leak a key, and cannot change the "custom" downgrade for unknown names. The hardcoded allowlist fallback (except Exception → known names) is preserved verbatim.
  • Compatibility: every existing test in the file continues to pass, including all 4 TestCustomEndpointApiKeyInheritance scenarios and the 3 explicit-base-url preservation scenarios. The _get_named_custom_provider lookup is a function-local import with its own try/except — no module-load surface change.
  • Test coverage: every branch of the new logic (built-in hit, user-defined hit, both miss, both unavailable) is covered by a dedicated test with both registries patched.

Notes for Reviewer

The duplicate-triage pass surfaced open PRs touching the same file — most relevantly #73173 (named-custom-provider host-derived key fallback) and #26909 (preserve named provider on custom endpoint). This PR is upstream of both: it fixes the layer that decides whether to keep the name (so the named-custom-provider branch in resolve_provider_client runs at all). #73173 fixes the layer that decides what key to use once the name has been preserved. Both can merge independently — this PR alone resolves the repro from #76602 (reported ("custom", "no-key-required")); #73173 adds defense in depth for providers configured with key_env / _host_derived_api_key.

Repro commands (from issue body):

from agent.auxiliary_client import resolve_vision_provider_client

# Before fix → ('custom', 'no-key-required')  → 401
# After fix  → ('agnes-ai.cn', 'sk-3bO...')   → 200
p, c, m = resolve_vision_provider_client(
    provider='agnes-ai.cn', model='agnes-2.5-flash',
    base_url='https://api.agnes-ai.cn/v1', api_key=None)

Checklist

  • Tests pass — 166/166 (162 existing + 4 new)
  • Follows Conventional Commits
  • Changes scoped to this fix only

Risk & Impact

Low. Blast radius contained to a single-purpose predicate that only widens its True set — it cannot widen False, leak a key, or change the "custom" downgrade for genuinely unknown names. The hardcoded allowlist fallback is preserved verbatim for catalog-load failures.

Type: 🐛 Bug fix
Fixes: #76602

…it base_url (NousResearch#76602)

When async_call_llm(task='vision') (and any other auxiliary path that
re-passes the config-resolved provider) forwards provider=<name> +
base_url=... + api_key=None to _resolve_task_provider_model,
the helper consulted only the built-in provider registry via
hermes_cli.providers.get_provider. A name defined in the
providers: section of config.yaml (e.g. agnes-ai.cn,
nvidia-nim) missed that lookup and fell through to the anonymous
"custom" downgrade. The downgrade routed the call through the
bare-custom branch in resolve_provider_client, which has no key,
sent Authorization: Bearer no-key-required and produced a 401 from
any auth-required provider.

The fix adds a second lookup against
hermes_cli.runtime_provider._get_named_custom_provider (the same
helper call_llm already uses for main-runtime resolution) so a
named user-defined provider + explicit base_url stays named through
to the named-custom-provider key-resolution branch, which then picks up
providers.<name>.api_key or the configured key_env.

Both lookups run inside the existing try / except guard, so a
catalog-load failure (early import, missing config) still falls back to
the hardcoded allowlist (xai-oauth / qwen-oauth / etc.). The
"custom" / "auto" / "custom:" short-circuit is unchanged;
the only behavioral change is that the second lookup widens the True
set without ever widening a False.

Tests cover:
- user-defined provider from providers: section is preserved (the
  repro from NousResearch#76602)
- built-in registry hit still wins (no built-in regression)
- unknown provider with explicit base_url still downgrades to
  "custom" (pre-existing behavior preserved)
- hardcoded allowlist still works when both registries are unavailable
  (early-import path)

@pestoura pestoura left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still a fail-open-to-downgrade path when the built-in catalog lookup raises. _get_named_custom_provider() is nested after get_provider(normalized), so any exception from get_provider jumps to the outer allowlist fallback and the user-defined provider is never consulted, even if its config is available. That contradicts the stated goal of preserving named custom providers through partial catalog-load failures.

Please isolate the two lookups so a built-in-registry exception does not suppress the named-custom lookup, and add a regression where get_provider raises while _get_named_custom_provider returns the configured entry. The expected provider should remain named rather than becoming custom. This is from source inspection; I did not run the auxiliary-client suite locally.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing this to _resolve_task_provider_model; current main still has the named-provider downgrade at agent/auxiliary_client.py:7121-7124.

Problems

  • agent/auxiliary_client.py:7089 in this diff leaves a partial-load failure path: if get_provider(normalized) raises, the outer except returns the hardcoded allowlist before _get_named_custom_provider is tried. A configured provider can therefore still become custom even when its named-provider configuration is available.

Suggested changes

  • Isolate the built-in and named-custom lookups, and add a regression where the built-in lookup raises while _get_named_custom_provider returns an entry.
  • Add a real temporary-HERMES_HOME config test through resolve_vision_provider_client; existing named-provider tests use that pattern in tests/agent/test_auxiliary_named_custom_providers.py:8-22 and exercise actual credential resolution.

Automated hermes-sweeper review.

Comment thread agent/auxiliary_client.py Outdated
# provider from the ``providers:`` section of config.yaml.
# Without this, an explicit provider + base_url (the shape the
# auxiliary vision path passes after resolving the task config)
# falls through to the anonymous ``"custom"`` downgrade, the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please guard this lookup independently from the named-custom lookup. If get_provider() raises, the outer except returns the hardcoded allowlist and _get_named_custom_provider() is never called, so an otherwise resolvable configured provider is still downgraded to custom. Add the corresponding regression.

@teknium1 teknium1 added 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 sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Aug 2, 2026
@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 area/config Config system, migrations, profiles needs-decision Awaiting maintainer decision before any implementation labels Aug 2, 2026
…erve check (NousResearch#76602 review)

Addresses review feedback from @teknium1 and @pestoura on PR NousResearch#76668.

The previous shape nested the new `_get_named_custom_provider` lookup
inside the outer try-block that also called `get_provider()`. A partial
catalog-load failure in the built-in registry therefore jumped the outer
`except` straight to the hardcoded allowlist fallback and never
consulted the user-defined providers: entry — so a configured named
provider was still downgraded to `"custom"` whenever the built-in
catalog failed to load. This contradicts the stated goal of preserving
named custom providers through partial catalog-load failures and
re-introduces the very 401 path the PR exists to fix.

Refactor: extract two module-level helpers — `_builtin_provider_present`
and `_named_custom_provider_present` — each with its own try/except.
The preserve check is now a flat `if/return True / if/return True /
return False` so a failure in one lookup cannot suppress the other.

Two new tests:

- `test_builtin_registry_raises_does_not_suppress_named_custom_lookup`:
  `get_provider` raises, `_get_named_custom_provider` returns an entry
  → the named provider is preserved (the partial-load regression the
  reviewer flagged).

- `test_resolve_vision_provider_client_preserves_named_provider_via_config`:
  integration test through the real `resolve_vision_provider_client`
  entry point with a real `HERMES_HOME` config.yaml, mirroring the
  pattern from `tests/agent/test_auxiliary_named_custom_providers.py`.
  Exercises the actual credential-resolution path (the inline
  `api_key` from the providers: entry reaches the client) instead of
  only asserting on the resolved provider name.

Test results:
```
pytest tests/agent/test_auxiliary_client.py
```
✅ 168/168 passed (162 existing + 6 new), no regressions.

Copy link
Copy Markdown
Contributor

Consolidation disposition — evidence/provenance donor to #67055

#67055 is the single semantic carrier for #76602/#100858 because it contains this bare-name preservation plus the custom:<name> second-pass and transport/precedence cases. This PR must not land independently as a narrower competing mechanism.

Preserve this contributor's focused registry-lookup regression and credit when the class is restacked onto current main in #67055. After that transfer is visible, close this PR as superseded. No third carrier.

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

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

auxiliary vision with custom provider + base_url loses api_key (downgraded to 'custom' → 'no-key-required' → 401)

5 participants