Skip to content

fix: group custom_providers by (endpoint, name) to support proxy setups - #21544

Closed
chiyema wants to merge 1 commit into
NousResearch:mainfrom
chiyema:fix/custom-provider-grouping-by-name
Closed

fix: group custom_providers by (endpoint, name) to support proxy setups#21544
chiyema wants to merge 1 commit into
NousResearch:mainfrom
chiyema:fix/custom-provider-grouping-by-name

Conversation

@chiyema

@chiyema chiyema commented May 7, 2026

Copy link
Copy Markdown

Problem

When multiple custom_providers entries share the same base_url and api_key (e.g. all routing through a LiteLLM proxy), but have distinct provider names (guava-litellm, copilot-litellm, elderberry-mtp), the /model picker collapses them into one giant row instead of showing them as separate selectable providers.

Root Cause

Section 4 of list_authenticated_providers() groups custom_providers entries by (base_url, api_key) tuple. This works for the "Ollama — Model X" pattern (same endpoint, same logical provider), but breaks the LiteLLM/proxy pattern where a single gateway proxies multiple logically distinct provider groups.

Fix

Add the cleaned display name to the group key: (base_url, api_key, cleaned_name). This preserves both behaviors:

  • Ollama — GLM 5.1 + Ollama — Qwen3 → cleaned to "Ollama" → same group key → collapse
  • guava-litellm + copilot-litellm → no em-dash → different cleaned names → separate rows

Test

Added test_list_same_endpoint_distinct_names_stay_separate which verifies 4 providers sharing one LiteLLM proxy endpoint produce 4 picker rows with correct model counts. All 19 existing tests pass unchanged.

When multiple custom_providers entries share the same base_url and
api_key (e.g. all routing through a LiteLLM proxy), but have distinct
provider names (guava-litellm, copilot-litellm, elderberry-mtp), they
should appear as separate rows in the /model picker.

Previously, grouping keyed only on (base_url, api_key), collapsing all
entries behind a shared proxy into one giant row. This adds the cleaned
display name to the group key so:

- 'Ollama — GLM 5.1' + 'Ollama — Qwen3' still collapse (same cleaned
  name 'Ollama', same endpoint)
- 'guava-litellm' + 'copilot-litellm' stay separate (different names,
  same endpoint)

Fixes the LiteLLM proxy use case where a single gateway proxies
multiple logical provider groups (bedrock, copilot, local models).
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels May 7, 2026
@dianjixz

Copy link
Copy Markdown

Thanks for working on this — this fixes an important part of the custom provider grouping problem.

I think there is one adjacent case this PR does not cover yet: custom_providers entries that share the same base_url but use different key_env credentials.

Right now this PR changes the grouping key from:

(api_url, api_key)

to:

(api_url, api_key, cleaned_name)

That solves the shared proxy / distinct provider-name case, but api_key is still only read from the inline api_key field:

api_key = (entry.get("api_key") or "").strip()

So if users configure providers like this:

custom_providers:
  - name: Gateway
    base_url: http://gateway.example/v1
    key_env: KEY_A
    model: alpha

  - name: Gateway
    base_url: http://gateway.example/v1
    key_env: KEY_B
    model: beta

and KEY_A / KEY_B resolve to different credentials, both entries still get grouped as if their key were empty. I tested this on top of this PR and the picker still collapses them into one row:

Gateway custom:gateway ['alpha', 'beta']

I think the grouping key should use the resolved credential value, matching the existing inline api_key behavior:

api_key = (entry.get("api_key") or "").strip()
if not api_key:
    key_env = (entry.get("key_env") or "").strip()
    api_key = os.environ.get(key_env, "").strip() if key_env else ""

group_key = (api_url, api_key, cleaned_name)

This would combine both fixes:

  • keep this PR's cleaned_name discriminator for shared proxy endpoints with distinct logical provider names
  • also avoid collapsing same-endpoint providers that use different env-backed credentials via key_env

I also have a small regression test for this case if useful.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the shared-proxy picker case. The issue still exists on current main: hermes_cli/model_switch.py:2196 groups Section 4 rows without the cleaned display name.

Problems

  • The proposed key reads only inline api_key. As @dianjixz noted, same-name entries using distinct key_env credentials still collapse.
  • This patch predates aa283d1e4, which added credential_identity and api_mode to prevent same-host providers from being routed through the wrong credential or protocol. Current main additionally keys headers at hermes_cli/model_switch.py:2187-2196; replacing that tuple with the proposed triple would regress those safeguards.

Suggested changes

  • Salvage the display-name discriminator by extending current main's tuple at hermes_cli/model_switch.py:2196, rather than replacing it.
  • Add coverage alongside tests/hermes_cli/test_model_switch_custom_providers.py:578 for the shared-proxy/distinct-name case while preserving distinct key_env, mode, and header identities.

Automated hermes-sweeper review.

if _sep in _cleaned_name:
_cleaned_name = _cleaned_name.split(_sep)[0].strip()
break

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This key still treats all key_env-backed entries as having an empty credential because api_key above only reads the inline field. More importantly, when salvaging onto current main, extend its existing credential_identity/api_mode/headers_identity key rather than replacing those discriminators with this triple.

@teknium1 teknium1 added 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 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing — the shared-proxy grouping fix has landed on main via PR #67925. You were the first to identify this bug and propose the display-name discriminator (May 7), and that credit is noted in the merged PR. The implementation that landed extends the current identity tuple (url, credential, api_mode, headers, name-prefix) rather than replacing it, because main gained credential/mode/headers safeguards after your branch point (see the sweeper's note above) — replacing the tuple would have regressed the wrong-credential-routing protection. Thanks for surfacing the LiteLLM proxy case!

@teknium1 teknium1 closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants