Skip to content

fix(doctor): honor a provider profile's models_url when base_url is empty - #76582

Open
HumphreySun98 wants to merge 1 commit into
NousResearch:mainfrom
HumphreySun98:fix/doctor-models-url-without-base
Open

fix(doctor): honor a provider profile's models_url when base_url is empty#76582
HumphreySun98 wants to merge 1 commit into
NousResearch:mainfrom
HumphreySun98:fix/doctor-models-url-without-base

Conversation

@HumphreySun98

Copy link
Copy Markdown
Contributor

What does this PR do?

_build_apikey_providers_list computed the models-endpoint URL as:

_models_url = (
    (_pp.models_url or (_pp.base_url.rstrip("/") + "/models"))
    if _pp.base_url else None
)

The if _pp.base_url else None guard wraps the whole expression, so a
ProviderProfile that sets an explicit models_url but no base_url ends up
with default_url=None. But ProviderProfile.models_url is documented as the
explicit models endpoint, with base_url only supplying the
{base_url}/models fallback (providers/base.py:55). So a valid endpoint is
silently dropped: _probe_apikey_provider then calls httpx.get(None, ...)
and doctor always shows a spurious yellow "⚠ … couldn't verify" for that
provider, even with a valid key and a perfectly good models endpoint.

Fix

Honor models_url independently; the if base_url guard now wraps only the
fallback:

_models_url = (
    _pp.models_url
    or (_pp.base_url.rstrip("/") + "/models" if _pp.base_url else None)
)

Orthogonal to the open #9823 (which fixes double-appending /models in the
probe-time _build_models_url) — different function, different bug.

Related Issue

No existing issue — found via a code audit.

Type of Change

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

Changes Made

  • hermes_cli/doctor.py — honor models_url when base_url is empty.
  • tests/hermes_cli/test_doctor_apikey_models_url.py — regression tests for the
    models_url-only and base_url-only cases.

How to Test

  1. scripts/run_tests.sh tests/hermes_cli/test_doctor_apikey_models_url.py -q → all pass.
  2. A profile with models_url set and base_url="" now yields that
    models_url as the doctor probe URL instead of None.

Checklist

  • I've read the Contributing Guide
  • Conventional Commits (fix(doctor):)
  • I searched for existing PRs (orthogonal to fix: don't double-append /models when base_url already contains it #9823; none fix this)
  • Only changes related to this fix
  • Affected tests pass (3, incl. the adjacent dedicated-skip test). Full suite not run locally.
  • Added regression tests
  • Tested on: Ubuntu (WSL2), against current main

…mpty

`_build_apikey_providers_list` computed the models-endpoint as:

    _models_url = (
        (_pp.models_url or (_pp.base_url.rstrip("/") + "/models"))
        if _pp.base_url else None
    )

The `if _pp.base_url else None` guard wraps the WHOLE expression, so a
ProviderProfile that sets an explicit `models_url` but no `base_url` gets
`default_url=None`. `ProviderProfile.models_url` is documented as the explicit
endpoint with `base_url` only providing the `{base_url}/models` fallback
(providers/base.py), so this drops a valid endpoint: doctor then probes `None`
and always shows a spurious yellow "couldn't verify" for that provider, even
with a valid key.

Honor `models_url` independently; the `if base_url` guard now wraps only the
fallback. Adds regression tests for the models_url-only and base_url-only cases.
@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for the focused fix and regression coverage. Current main still gates the explicit endpoint on base_url in hermes_cli/doctor.py:670-673, despite the provider contract resolving models_url independently in providers/base.py:202-207. The changed expression matches that contract and preserves the existing {base_url}/models fallback.

This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels Aug 2, 2026
@teknium1 teknium1 added sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/install-update Installer, updater, packaging, wheels, doctor area/profiles Multi-profile isolation, HERMES_HOME scoping labels Aug 2, 2026
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 area/install-update Installer, updater, packaging, wheels, doctor area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants