Skip to content

fix(model-switch): merge providers.<slug>.models into HERMES_OVERLAYS picker rows - #64877

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/model-picker-configured-models-overlay
Open

fix(model-switch): merge providers.<slug>.models into HERMES_OVERLAYS picker rows#64877
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/model-picker-configured-models-overlay

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

What does this PR do?

list_authenticated_providers() (hermes_cli/model_switch.py) merges a providers.<slug>.models config block into a built-in provider's (section 1, PROVIDER_TO_MODELS_DEV) discovered catalog (#63055), so a model declared there shows up in the /model picker. Section 2 (HERMES_OVERLAYSnous, openai-codex, copilot, opencode-go, etc.) builds its model_ids through the same shape of code but never applies this merge.

Root cause: a model declared under providers.nous.models (or providers.opencode-go.models, providers.openai-codex.models, providers.copilot.models) is already typeable via /model <name> today — _configured_provider_matches() (line 728) scans user_providers generically by slug, with no built-in/overlay distinction, specifically to route a typed model name to the right provider (#45006). But it never appears in the picker list for that provider, because section 2's model_ids construction has no equivalent merge. This is inconsistent with the identical config surface already 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, keyed by hermes_slug — the same slug the row itself reports as "slug" in the picker, and the same namespace providers.nous/providers.openai-codex are documented as (hermes_cli/auth.py's own comments reference these exact keys).

Scope note

Section 2b (CANONICAL_PROVIDERS) has the same "providers.<slug>.models is invisible to the picker" symptom for a different subset of providers. I found and fixed it too, but two other open PRs already actively rework that exact code region with a different approach:

Both touch the same few lines my section-2b fix would have touched, with a philosophy that conflicts with a pure merge (skip-and-defer vs. merge-and-keep-both). Rather than add a third competing change to contested territory, I scoped this PR to section 2 only, where there's no existing open PR.

Related Issue

None filed — found via code review while auditing today's (2026-07-12) #63055/#63052/#63046 model-picker series for consistency across all four sections of list_authenticated_providers().

Type of Change

  • 🐛 Bug fix

Changes Made

  • hermes_cli/model_switch.py: section 2 (HERMES_OVERLAYS loop) now merges providers.<hermes_slug>.models into model_ids before capping to max_models, mirroring section 1's existing merge (+13 lines)
  • tests/hermes_cli/test_configured_builtin_models.py: two new regression tests — test_configured_models_extend_hermes_overlay_provider_row and test_configured_models_are_merged_before_picker_limit_for_overlay, mirroring the existing section-1 tests in the same file but against a synthetic HermesOverlay entry

How to Test

python3.11 -m pytest tests/hermes_cli/test_configured_builtin_models.py tests/hermes_cli/test_overlay_slug_resolution.py tests/hermes_cli/test_model_switch_custom_providers.py tests/hermes_cli/test_opencode_zen_model_limit.py tests/hermes_cli/test_ollama_cloud_provider.py tests/hermes_cli/test_model_switch_filter_unresolved.py tests/hermes_cli/test_openai_picker_curated.py tests/hermes_cli/test_list_picker_providers.py tests/hermes_cli/test_user_providers_model_switch.py tests/hermes_cli/test_codex_cli_model_picker.py tests/hermes_cli/test_bedrock_model_picker.py tests/hermes_cli/test_picker_prewarm.py tests/hermes_cli/test_copilot_in_model_list.py tests/hermes_cli/test_models_dev_preferred_merge.py tests/hermes_cli/test_opencode_go_in_model_list.py tests/hermes_cli/test_authenticated_providers_exhausted_pool.py -v --override-ini="addopts="

199 passed. New tests mutation-verified: stashing the fix reproduces the exact bug — both new tests fail with row["models"] == ["live-a", "shared"] (configured models missing), confirming they genuinely catch the regression.

Checklist

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing this to the overlay path. Current main still has the gap: section 2 builds the HERMES_OVERLAYS catalog and applies its picker limit without the configured-model merge (hermes_cli/model_switch.py:1855-1922), whereas section 1 already performs that merge before truncation (hermes_cli/model_switch.py:1745-1758). The proposed implementation matches that established precedence and deduplication behavior.

Problems

  • The new fixture uses test-overlay for both the overlay key and the configured slug. It does not exercise the key distinction that section 2 explicitly supports: a models.dev key such as github-copilot resolved to the config/Hermes slug copilot (hermes_cli/model_switch.py:1781-1792).

Suggested changes

  • Add a mapped-slug regression case: overlay key github-copilot, PROVIDER_TO_MODELS_DEV mapping copilot -> github-copilot, and providers.copilot.models. Assert configured IDs precede and deduplicate discovered IDs before the limit.

Automated hermes-sweeper review.

@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 16, 2026
@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 and removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 16, 2026
@pierrenode
pierrenode force-pushed the fix/model-picker-configured-models-overlay branch from 1c5fd16 to 1ee3b90 Compare July 27, 2026 00:27

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address the configured-model picker gap across distinct provider paths. #63055 fixed built-in provider rows, while #64877 extends the same configured-first, deduplicated merge to HERMES_OVERLAYS rows.

Related pull requests

  • #63055 [merged] related — (+53/-0) — merged reference implementation: merges providers..models into built-in picker rows before max_models truncation, preserving configured models and deduplicating discovered entries. It remains relevant because #64877 applies this established behavior to the separate overlay path.
  • #64877 related — (+103/-0) — keep open pending test correction: the implementation merges providers.<hermes_slug>.models into overlay rows before truncation and correctly keys lookup by the resolved Hermes slug. The contributor keep_open review is not fully addressed: the mapped-slug test uses max_models=None, while the separate limit test uses an unmapped synthetic slug, so no test verifies mapped-slug resolution and configured-model precedence under an active picker limit together.

Suggested consolidation

Do not merge #64877 yet. Update its mapped-slug regression to use a restrictive max_models value and assert that providers.copilot.models entries precede and deduplicate discovered models before truncation; after that contributor review requirement is covered and tests pass, merge #64877. #63055 is complementary merged groundwork, not a duplicate, so no PR should be closed as a duplicate.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 9 kB of PR diffs, 6 kB of issue/PR text, 1 kB of discussion (1 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

… 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).
@pierrenode
pierrenode force-pushed the fix/model-picker-configured-models-overlay branch from 1ee3b90 to ae50d83 Compare August 11, 2026 23:17
@pierrenode

Copy link
Copy Markdown
Contributor Author

Rebased onto current `upstream/main` and squashed to a single commit.

`hermes_cli/model_switch.py` auto-merged cleanly. `tests/hermes_cli/test_configured_builtin_models.py` had one conflict: the pre-existing test this PR's diff shows as unchanged context (`test_configured_models_are_merged_before_picker_limit`, section-1/built-in coverage, not part of this PR) has been pruned from `upstream/main` (test-pruning wave). Kept upstream's pruning and restored only this PR's own new additions (the `_overlay_row` helper + all 3 section-2/HERMES_OVERLAYS regression tests, starting from `_overlay_row` onward) — verified against the original commit's diff hunk boundaries to isolate exactly what this PR added.

Targeted suite (4 tests) passes, mutation-verified (removing the `_configured_models` merge makes all 3 overlay tests fail — picker rows come back without the configured models prepended). Broader sweep (`tests/hermes_cli/` filtered to model_switch/picker/configured_builtin, 217 tests) passes clean. Ruff clean.

Disclosure — genuine duplicate found: fresh competitor search turned up #72049 ("Show configured overlay models in the desktop picker", opened 2026-07-26, ~11 days after this PR), which implements the identical fix — same file, same insertion point (right before `total = len(model_ids)` in the HERMES_OVERLAYS section), same mechanism (`configured_models = _declared_model_ids(...)`, `model_ids = list(dict.fromkeys([*configured_models, *model_ids]))`). This PR is the older of the two. I'm not in a position to close another author's PR, so flagging here — one of the two is fully redundant once either merges.

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 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants