fix(model): make picker discovery config-aware - #60656
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the remaining config-aware picker gap. The built-in provider gate addresses a current-main defect: hermes_cli/model_switch.py:1693-1734 emits env-authenticated mapped providers without checking the supplied config context.
Problems
- The custom-provider explicit-model hunks duplicate behavior already present on current main at
hermes_cli/model_switch.py:2210-2240and2320-2325. The cross-referenced PR #61928 merged this focused work as5f00f36ba9f; its summary also notes that #60656's provider-config gating remains separate. Keeping the duplicate hunks will cause avoidable salvage conflicts.
Suggested changes
- Retain the config gate and gateway context propagation, but drop the already-landed custom discovery changes and their duplicate regression coverage.
- Add a gateway-level assertion that a loaded config with no
providers:section passes{}into the picker path; the existing gateway offload test does not assert that argument.
This is an automated hermes-sweeper review.
| @@ -1996,7 +2030,9 @@ def _has_aws_sdk_creds_for_listing(slug: str) -> bool: | |||
| # custom_providers entries use, so accept either. | |||
There was a problem hiding this comment.
This explicit-model distinction is already present on current main (hermes_cli/model_switch.py:2040), with the analogous grouped custom-provider implementation at :2210-2240 and :2320-2325 from merged #61928 (5f00f36ba9f). Please omit the duplicate custom-discovery hunks when salvaging this PR.
97b4d8d to
355e51d
Compare
|
Rebased onto current main and removed the custom-provider discovery changes already landed in #61928, retaining the configuration-aware built-in picker gate and empty providers context propagation. The targeted CI test and focused picker/gateway tests pass (71 tests). The full suite cannot collect locally because fastapi/uvicorn are unavailable and this environment blocks the lazy dependency install. |
rajv2er
left a comment
There was a problem hiding this comment.
I encountered this exact bug in production and want to confirm the need for #60656.
Symptom: The desktop model picker only shows 1 model per custom provider (the model: config value). Users must click Refresh Models every time to see the full catalog.
Root cause: In web_server.py the /api/model/options endpoint passes probe_custom_providers=bool(refresh), which means when refresh=false (the default picker behavior), custom providers are NOT probed. Their model list is silently empty or just the single configured model.
There's also a commit fe5054b (fix(desktop): avoid probing custom providers on model picker open) that made this intentional for performance reasons, but it broke the user experience entirely.
Why #60656 is the right fix: The config-aware approach preserves performance (only probe configured providers) while fixing the broken UX. The built-in provider gate is a great addition too.
The reviewer's suggestion to drop the duplicate custom-discovery hunks (already landed in #61928) makes sense and simplifies the diff.
Confirmed on macOS 15.2 with Hermes main at HEAD. The bug is reproducible and #60656 addresses it properly.
rajv2er
left a comment
There was a problem hiding this comment.
Quick follow-up: I also worked out a simpler 6-line fix that directly changes to in both web_server.py and tui_gateway/server.py.
I didn't open a competing PR because I saw #60656 first and didn't realize I should have mentioned both options — sorry about that. Both approaches fix the same root cause:
- #60656: Config-aware (probe only configured providers, not all env-var-found ones). Larger scope, preserves the performance optimization from fe5054b.
- Simpler fix: Always probe custom providers. 6 lines, surgical, but reverts the performance optimization from fe5054b.
Whichever lands first will fix the bug. If the maintainers prefer the simpler approach, the 6-line change is trivial to backport.
rajv2er
left a comment
There was a problem hiding this comment.
Alternative simpler fix (6 lines):
If maintainers prefer a surgical approach over the broader config-aware refactor in #60656, here's a 6-line diff that directly fixes the root cause:
hermes_cli/web_server.py (line ~5575):
- probe_custom_providers=bool(refresh),
- probe_current_custom_provider=not bool(refresh),
+ probe_custom_providers=True,
+ probe_current_custom_provider=False,tui_gateway/server.py (line ~13405):
- probe_custom_providers=bool(params.get("refresh")),
- probe_current_custom_provider=not bool(params.get("refresh")),
+ probe_custom_providers=True,
+ probe_current_custom_provider=False,tests/test_tui_gateway_server.py (line ~7356):
- assert listing.call_args.kwargs["probe_custom_providers"] is False
- assert listing.call_args.kwargs["probe_current_custom_provider"] is True
+ assert listing.call_args.kwargs["probe_custom_providers"] is True
+ assert listing.call_args.kwargs["probe_current_custom_provider"] is FalseTrade-off: This reverts the performance optimization from commit fe5054b ("avoid probing custom providers on model picker open"). #60656's config-aware approach is smarter — it only probes configured providers, not all. But either fix resolves the reported symptom.
|
Rebased onto current main and resolved the picker conflict while retaining the configuration-aware built-in provider gate and empty provider-config context propagation. Removed the custom-provider discovery portion already covered upstream, and updated two existing text opens in the touched test file for the current Windows-footguns rule. Focused picker/gateway coverage passes (137 tests); full-suite collection is unavailable locally because fastapi/uvicorn are missing and this managed Python environment blocks installation. |
355e51d to
7786a64
Compare
|
Rebased cleanly onto current main and repaired the configuration-aware picker gate: stored credential pools remain selectable in the interactive picker even when temporarily exhausted, while env-only providers remain filtered. Verified the targeted CI node and focused picker coverage locally (73 tests), plus ruff and the Windows-footguns scan. |
7786a64 to
aab3067
Compare
|
Rebased onto current main, preserving its intentional removal of the conflicting pruned test cases. Retained the configuration-aware provider gate and empty |
aab3067 to
553c62f
Compare
… picker rows list_authenticated_providers() merges a providers.<slug>.models config block into a built-in (section 1, PROVIDER_TO_MODELS_DEV) row's discovered catalog, so a model declared there shows up in the /model picker. Section 2 (HERMES_OVERLAYS — nous, openai-codex, copilot, opencode-go, etc.) builds its model_ids the same way but never applies this merge: a model declared under providers.nous.models (or providers.opencode-go.models, etc.) is already typeable via /model <name> (_configured_provider_matches() scans user_providers generically, by design) but never appears in the picker list — inconsistent with the identical config surface working correctly for built-in providers. Fix: apply the same merge (configured models first, deduplicated against the discovered list) right before model_ids is capped to max_models, using hermes_slug as the config key — the same slug the row itself reports and the same namespace _configured_provider_matches() already scans generically (providers.nous, providers.openai-codex are documented config keys, see hermes_cli/auth.py's own comments). Scoped to section 2 only. Section 2b (CANONICAL_PROVIDERS) has the same "providers.<slug>.models is invisible to the picker" symptom for a different subset of providers, but two other open PRs (NousResearch#21983, NousResearch#60656) are already actively reworking that exact code region with a different approach (skip-and-defer to section 3 vs. merge) — not touching it here to avoid overlapping scope on the same lines. Adds a mapped-slug regression case: HERMES_OVERLAYS key "github-copilot" (a models.dev ID) that PROVIDER_TO_MODELS_DEV maps back to the Hermes/ config slug "copilot", with providers.copilot.models — proving the providers.<hermes_slug>.models lookup keys off the RESOLVED slug, not the raw overlay/pid key, and exercising the hermes_slug in {"openai-codex", "copilot", "copilot-acp"} special live-discovery branch specifically (the existing overlay tests only exercised the generic fallback branch via a synthetic 1:1 slug).
Make the in-session /model picker use the same config-aware discovery contract for provider rows and custom endpoint model lists.
What does this PR do?
The /model picker now hides built-in providers that are only present because an API-key env var exists when the caller has loaded config.yaml and that provider is not configured. Configured built-ins still route through cached_provider_model_ids(), including explicit refresh, so their model lists come from the shared discovery path rather than static picker-only lists.
Custom providers now distinguish the singular model/default_model seed from an explicit models allowlist. A custom endpoint with only model: custom-model-1 will probe /v1/models and show the full endpoint catalog, while an explicit models: list/dict still acts as the user's narrowed subset.
Shared root cause
How this fixes each issue
Related Issue
Type of Change
Changes Made
How to Test
What platforms tested on
Checklist
This coordinated PR bundles a fix that spans several issues. Happy to split it back into focused per-issue PRs if you'd prefer to review them separately.
Refs #6799
Refs #20582