Skip to content

fix(providers): resolve ProviderProfile identity consistently - #52549

Closed
WolframRavenwolf wants to merge 1 commit into
NousResearch:mainfrom
WolframRavenwolf:fix/provider-profile-cli-identity
Closed

WolframRavenwolf wants to merge 1 commit into
NousResearch:mainfrom
WolframRavenwolf:fix/provider-profile-cli-identity

Conversation

@WolframRavenwolf

@WolframRavenwolf WolframRavenwolf commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

A provider declared through ProviderProfile can be available at startup yet fail the shared CLI/gateway identity lookup. Resolve profile names and aliases through resolve_provider_full() while keeping built-in lookup free of plugin discovery. Preserve explicit configuration, built-in provider IDs, bare custom endpoints and display metadata.

This PR covers identity and configuration. Runtime transport-mode propagation remains separate work in #53055; this branch does not take over that implementation. Its discovery-fixture coverage is credited to david-bowiegxw, with the contributor mapping and Co-authored-by trailer retained. #34368 identified the model-switch lookup gap and is credited to Kyzcreig. #65956 overlaps the separate runtime-mode work.

The new test module is tests/hermes_cli/test_provider_profile_identity.py, avoiding the filename collision with #53055. Compatibility with that unmerged transport change is not asserted here.

Validation: 246 tests passed across model-switch custom providers, runtime provider resolution, profile identity, runtime transport precedence and CommandCode profile suites; scoped Ruff and git diff --check passed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jun 25, 2026
@WolframRavenwolf
WolframRavenwolf force-pushed the fix/provider-profile-cli-identity branch 4 times, most recently from 47a2348 to 066465e Compare June 25, 2026 19:15
@WolframRavenwolf
WolframRavenwolf requested a review from a team June 25, 2026 19:15
@WolframRavenwolf
WolframRavenwolf force-pushed the fix/provider-profile-cli-identity branch from 066465e to 73713bc Compare June 25, 2026 19:16
@WolframRavenwolf
WolframRavenwolf force-pushed the fix/provider-profile-cli-identity branch from 73713bc to ecbf2e1 Compare July 5, 2026 12:33

@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 isolating the generic CLI identity gap; current resolve_provider_full() still reaches get_provider() without a ProviderProfile fallback (hermes_cli/providers.py:742-745, :477).

Problems

  • hermes_cli/runtime_provider.py:2001-2032 makes profile_api_mode win over URL detection. Current main intentionally honors a selected provider's model.base_url (:2028-2032) and then detects the endpoint protocol (:2057-2062). With this PR, an auto-registered profile declaring (for example) anthropic_messages cannot switch to a configured OpenAI/xAI endpoint, because :2029 skips detection.

Suggested changes

  • Make profile_api_mode a fallback only after explicit configured mode and _detect_api_mode_for_url(base_url), and add a regression covering a non-default profile mode plus a same-provider base-URL override requiring a different detected mode.
  • The new switch test manually inserts ProviderConfig.extra; add coverage that exercises the auth auto-registration path in hermes_cli/auth.py:461-473 as well.

Automated hermes-sweeper review.

Comment thread hermes_cli/runtime_provider.py Outdated
@@ -1998,7 +1998,8 @@ def resolve_runtime_provider(
if cfg_provider == provider:
cfg_base_url = (model_cfg.get("base_url") or "").strip().rstrip("/")
base_url = cfg_base_url or creds.get("base_url", "").rstrip("/")
api_mode = "chat_completions"
profile_api_mode = str((pconfig.extra or {}).get("api_mode") or "").strip()
api_mode = profile_api_mode or "chat_completions"

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 keep _detect_api_mode_for_url(base_url) ahead of this profile fallback. model.base_url is intentionally honored for the configured provider, so an overridden endpoint may require a different protocol than the profile's default; the new if not profile_api_mode guard at line 2029 prevents that detection entirely.

@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 15, 2026
@WolframRavenwolf
WolframRavenwolf force-pushed the fix/provider-profile-cli-identity branch from ecbf2e1 to 60149e1 Compare July 18, 2026 14:16
@WolframRavenwolf WolframRavenwolf changed the title fix(providers): resolve ProviderProfile plugins in CLI identity fix(providers): unify ProviderProfile identity and api mode Jul 18, 2026
@WolframRavenwolf

Copy link
Copy Markdown
Contributor Author

Addressed the review points and coordinated the overlapping provider work in the rebased branch:

  • plugin discovery is limited to the full CLI resolver, not get_provider()
  • real auth auto-registration is exercised by the /model regression
  • persisted config and URL detection win over the profile-declared api_mode; the profile remains the fallback for non-self-describing endpoints
  • stale persisted modes from a different provider are rejected on the explicit-key path
  • pooled, explicit-key, and no-pool routes are covered
  • explicit user-provider IDs also beat matching profile aliases; bare custom semantics are preserved

Canonical provider test runner: 208 passed.

@WolframRavenwolf

Copy link
Copy Markdown
Contributor Author

Fresh head-matched Devin review found one actionable integration mismatch: CLI identity accepted profiles that the auth bridge intentionally does not register.

Addressed in the latest head by applying the same auth_type == "api_key" plus non-empty env_vars eligibility contract in both paths, with focused regressions for no-auth and uncredentialed profiles.

Verification:

  • canonical provider suite: 210 passed
  • Ruff: passed
  • contributor mapping gate reproduced locally: all author emails mapped

@WolframRavenwolf

Copy link
Copy Markdown
Contributor Author

A delayed independent review found one additional provider-family edge case after the previous update: a persisted api_mode keyed by a registered ProviderProfile alias was not recognized as belonging to the canonical runtime provider.

Fixed test-first in the latest head:

  • canonical and alias registry entries are compared through their shared ProviderConfig.id
  • persisted alias configuration now wins on pooled, explicit-key, and no-pool API-key routes
  • unrelated providers remain isolated
  • bare custom behavior remains unchanged

Verification:

@WolframRavenwolf
WolframRavenwolf force-pushed the fix/provider-profile-cli-identity branch from 44107c3 to 54dd21f Compare August 11, 2026 21:19
Keep canonical plugin profile identity through CLI aliases, separate URL
variables from credentials, and honor matching persisted configuration
across registered aliases. Retain native custom and built-in routing.

Retain real discovery/runtime test provenance from the contributor while
omitting the already absorbed bridge and separately owned transport fix.

Co-authored-by: david-bowiegxw <834563048@qq.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Salvaged into #116853 (main 9f0cd9d773) by cherry-pick, so your commit 7d29404 (alias identity: persisted config applies to the canonical provider, _same_registered_provider) landed with your authorship; the 266-line test module was trimmed to three invariants. Thank you @WolframRavenwolf. Closing as landed.

@teknium1 teknium1 closed this Sep 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 P3 Low — cosmetic, nice to have 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.

3 participants