Skip to content

fix(auth): preserve custom:<subname> in credential pool lookup - #33700

Closed
Pluviobyte wants to merge 2 commits into
NousResearch:mainfrom
Pluviobyte:fix/auth-preserve-custom-subname
Closed

fix(auth): preserve custom:<subname> in credential pool lookup#33700
Pluviobyte wants to merge 2 commits into
NousResearch:mainfrom
Pluviobyte:fix/auth-preserve-custom-subname

Conversation

@Pluviobyte

Copy link
Copy Markdown
Contributor

What does this PR do?

When a model_aliases: entry resolves through the direct-alias bare-custom branch in hermes_cli/runtime_provider.py::_resolve_named_custom_runtime (i.e. requested_provider like custom:bobapi-deepseek gets normalised to requested_norm == \"custom\" via an alias that maps to \"custom\"), the credential-pool lookup was called with provider_name=None and silently fell back to base_url-only matching.

When two or more custom_providers share a single base_url (a common shape for Chinese aggregator APIs where each key is bound to a different group on the same domain — the exact case from #29872), the url-only fallback always picked whichever entry happened to appear first in iteration order, sending the request with the wrong API key. The upstream then returns a 503 / 404 that looks like a model-availability problem but is actually a credential-misroute.

The fix splits the sub-name off requested_provider and threads it through _try_resolve_from_custom_pool(..., provider_name=<subname>) so name-based pool selection wins over the url-only fallback. The kwarg form mirrors the sibling pool-lookup site in the same function (the named-custom-provider path) that already passes the name correctly via custom_provider.get(\"name\").

This PR salvages the auth-only subset of #29893 (credit @vanhci). The original PR was closed with CHANGES_REQUESTED because it mixed in unrelated WhatsApp / uv pip --system / i18n flag changes; per reviewer feedback ("split it into individual PRs"), this PR ships only the hermes_cli/runtime_provider.py fix plus its regression tests. Two small refinements over the original diff: the sub-name is passed as a keyword argument (provider_name=) instead of as the third positional (which would have aliased to api_mode_override), and .strip() is applied so trailing whitespace in the alias name does not break pool selection.

Related Issue

Fixes #29872

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/runtime_provider.py — in _resolve_named_custom_runtime, extract <subname> from requested_provider (e.g. bobapi-deepseek from custom:bobapi-deepseek) and thread it through _try_resolve_from_custom_pool(..., provider_name=_pool_provider_name) for the direct-alias bare-custom branch.
  • tests/hermes_cli/test_runtime_provider_resolution.py — added three regression tests:
    • test_direct_alias_custom_subname_passed_to_pool_lookup — asserts the sub-name reaches the pool lookup as the provider_name= kwarg.
    • test_direct_alias_bare_custom_keeps_provider_name_none — baseline guard so bare provider: custom (no sub-name) still passes None.
    • test_direct_alias_subname_resolves_correct_pool_when_url_shared — end-to-end check: two custom_providers behind one base_url, the deepseek alias must select the deepseek pool, not whichever entry iterates first.

How to Test

  1. Configure two or more custom_providers sharing the same base_url, each with a distinct key_env, plus a model_aliases: entry that references one by sub-name (the exact reproduction recipe is in [Bug]: model_aliases provider: custom:<subname> normalized to bare 'custom', wrong API key picked from pool #29872).
  2. Run hermes chat -m <alias> (without the --provider workaround) and confirm the request fires with the alias's intended key instead of whichever custom_providers entry appears first.
  3. Local verification run:
uv run --extra dev python -m pytest tests/hermes_cli/test_runtime_provider_resolution.py -q
uv run --extra dev ruff check hermes_cli/runtime_provider.py tests/hermes_cli/test_runtime_provider_resolution.py
git diff main --check

Regression-verified the new tests by temporarily reverting the prod change (git stash push hermes_cli/runtime_provider.py) — two of the three new tests fail with the unfixed code (third stays green because it covers the bare-custom baseline). With the fix restored, all three pass.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux 6.1 (Amazon Linux), Python 3.12 via uv

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A

Screenshots / Logs

Targeted-suite output:

```text
$ uv run --extra dev python -m pytest tests/hermes_cli/test_runtime_provider_resolution.py -q
........................................................................ [ 55%]
......................................................... [100%]
129 passed in 3.10s

$ uv run --extra dev ruff check hermes_cli/runtime_provider.py tests/hermes_cli/test_runtime_provider_resolution.py
All checks passed!

$ git diff main --check
(no whitespace issues)
```

Regression check (prod fix temporarily reverted) — confirms the new tests catch the bug:

```text
$ git stash push hermes_cli/runtime_provider.py
$ pytest -k "direct_alias_custom_subname or direct_alias_subname_resolves" -q
FAILED ...::test_direct_alias_custom_subname_passed_to_pool_lookup
FAILED ...::test_direct_alias_subname_resolves_correct_pool_when_url_shared
AssertionError: alias sub-name must select the deepseek pool, not the first url match

  • key-for-custom:bobapi-deepseek
  • key-for-custom:bobapi-claude
    2 failed, 1 passed
    ```

Made with Cursor

Pluviobyte and others added 2 commits May 28, 2026 06:32
When a `model_aliases:` entry resolves through the direct-alias bare-custom
branch in `_resolve_named_custom_runtime` (i.e. `requested_provider` like
`custom:bobapi-deepseek` gets reduced to `requested_norm == "custom"` via
an alias that maps to "custom"), the credential-pool lookup was called
with `provider_name=None` and silently fell back to base_url-only matching.

When two or more `custom_providers` share a single `base_url` (a common
shape for Chinese aggregator APIs where each key is bound to a different
"分组"/group on the same domain), the url-only fallback always picked
whichever entry happened to appear first in iteration order, sending the
request with the wrong API key — surfacing upstream as a 503 / 404 that
looks like a model-availability problem but is actually a key-misroute.

Fix: split the sub-name off `requested_provider` and thread it through
`_try_resolve_from_custom_pool(..., provider_name=<subname>)` so name-based
pool selection wins over the url-only fallback. The kwarg form mirrors the
second pool-lookup site in the same file (line 695) that already passes
the name correctly via `custom_provider.get("name")`. Three regression
tests in `test_runtime_provider_resolution.py` cover the fix, the bare
`provider: custom` (no sub-name) baseline, and the end-to-end pool
selection when multiple providers share one base_url.

Salvages the auth-only subset of NousResearch#29893 (credit @vanhci); the original
PR was closed in review for mixing in unrelated WhatsApp / `uv pip` /
i18n changes — this PR ships only the `hermes_cli/runtime_provider.py`
fix plus its tests.

Fixes NousResearch#29872

Co-authored-by: Cursor <cursoragent@cursor.com>
The contributor-check workflow rejects noreply emails without a
numeric ID prefix. Adding this mapping unblocks CI for this PR.

Co-authored-by: Cursor <cursoragent@cursor.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard labels May 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Salvage of closed-not-merged #29893 — same credential pool fix for #29872 (custom:subname normalization drops sub-provider name). This PR strips the unrelated changes that caused #29893's closure and adds keyword-argument refinements.

@teknium1

Copy link
Copy Markdown
Contributor

This is an automated hermes-sweeper review. Thank you for isolating the credential-pool change and preserving the prior contributor credit.

  • Current main already implements the required custom:<subname> behavior through commit e38ea38079b8683fba48a245c19ff5a2a8f50d39 (fix(credential_pool): resolve key mix-up when custom providers share base_url), shipped in v2026.5.7.
  • A model_aliases direct alias preserves provider: custom:<name> (hermes_cli/model_switch.py:281-287; hermes_cli/oneshot.py:357-379). That form does not normalize to bare custom in hermes_cli/auth.py:1714-1730; runtime therefore reaches the named-custom path.
  • The named-custom path passes custom_provider.get("name") into pool resolution (hermes_cli/runtime_provider.py:974-986), and the pool selects the exact named provider before URL fallback (agent/credential_pool.py:393-406).
  • Existing regression coverage for two providers sharing one base URL is in tests/agent/test_credential_pool.py:2152-2186.

The PR's new tests force resolve_provider("custom:<subname>") to return custom, which is not the stock resolution path. The current implementation already satisfies #29872 without this additional branch.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: model_aliases provider: custom:<subname> normalized to bare 'custom', wrong API key picked from pool

3 participants