fix(models): merge curated + live catalog for Copilot provider - #44257
fix(models): merge curated + live catalog for Copilot provider#44257nguyenhung2904 wants to merge 1 commit into
Conversation
Previously, get_available_models() for the 'copilot' provider returned the live API catalog verbatim and discarded the curated static list (_PROVIDER_MODELS['copilot']) whenever the live fetch succeeded. This mirrors the approach already used for the 'anthropic' provider: curated entries appear first (preserving intended ordering / pinned aliases), then any live-only models are appended so newly-enabled account models surface automatically without requiring a Hermes release. Root cause observed: claude-fable-5 (model_picker_enabled=True, type=chat, supported on /chat/completions) was absent from the /model picker despite being enabled on the GitHub Copilot account, because the OAuth token used for live catalog fetching returned a subset of the account's allowed models. The curated list included it; the live catalog did not.
✅ Verification: Clean ReviewReviewed this PR's diff. Two clean changes:
The merge strategy (curated order preserved, live-only models appended) is the right approach — it ensures pinned models always appear while still surfacing newly-enabled models from the live API. No issues found. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the Copilot-specific catalog path. The underlying issue is still present on current main: provider_model_ids() returns a nonempty Copilot live list directly at hermes_cli/models.py:2319-2323, before the generic live+curated merge path used by merged PR #46857.
Problems
- The PR changes that result to a merged catalog but does not update
tests/hermes_cli/test_model_validation.py:208-216. Those tests currently mock a two-model live response and assert that it is the complete result, which conflicts with the proposed curated-first behavior.
Suggested changes
- Replace that live-only assertion with a Copilot regression test that mocks both the curated and live lists and verifies curated-first ordering, live-only preservation, and case-insensitive deduplication. Exercise
copilot-acptoo, since it shares the branch athermes_cli/models.py:2319-2327.
Automated hermes-sweeper review.
| if m.lower() not in curated_lower: | ||
| merged.append(m) | ||
| return merged | ||
| except Exception: |
There was a problem hiding this comment.
Please update tests/hermes_cli/test_model_validation.py:208-216 with this behavior: it currently asserts the mocked live list is returned verbatim, so it will fail once this merged result is returned. A regression test should mock curated and live lists and assert curated-first ordering plus live-only retention and deduplication.
Problem
When
get_available_models()is called for thecopilotprovider, it fetches the live catalog from the GitHub Copilot API and returns it verbatim — discarding the curated static list (_PROVIDER_MODELS['copilot']) entirely whenever the live fetch succeeds.This means models that are enabled on the account but absent from the live OAuth token's catalog response never appear in
/model.Observed symptom:
claude-fable-5(model_picker_enabled=True,type=chat, listed under/chat/completions) was missing from the model picker despite being enabled on the GitHub Copilot account. The curated list included it; the live catalog from the OAuth token did not.Fix
Mirrors the approach already used for the
anthropicprovider: curated entries appear first (preserving intended ordering and pinned aliases), then any live-only models are appended so newly-enabled account models still surface automatically without requiring a Hermes release.Testing
Verified locally:
claude-fable-5now appears in the/modelpicker after the fix, whereas it was absent before.