fix(models): default model rehydration when providers share slash-qualified IDs (#1313)
From PR #1326. Co-authored-by: hacker2005 <chen20057275@outlook.com>
This commit is contained in:
1 parent
e86de0aff3
commit
4683a4a0d0
5 files changed
+180
-53
No files matched your search
+35
-30
@@ -880,23 +880,23 @@ def _apply_provider_prefix(
|
||||
def _deduplicate_model_ids(groups: list[dict]) -> None:
|
||||
"""Ensure every model ID across groups is globally unique.
|
||||
|
||||
When multiple providers expose the same bare model ID (e.g. two
|
||||
custom providers both listing ``gpt-5.4``), the dropdown cannot
|
||||
distinguish them. This post-process detects such collisions and
|
||||
prefixes colliding entries with ``@provider_id:`` so the frontend
|
||||
can treat them as distinct options.
|
||||
When multiple providers expose the same model ID (either bare names like
|
||||
``gpt-5.4`` or slash-qualified IDs like ``google/gemma-4-27b``), the
|
||||
dropdown cannot distinguish them. This post-process detects such
|
||||
collisions and prefixes colliding entries with ``@provider_id:`` so the
|
||||
frontend can treat them as distinct options.
|
||||
|
||||
The first occurrence (in group order) is left bare for backward
|
||||
compatibility with sessions that already store the bare model name.
|
||||
If that provider is later removed from the config, the next cache
|
||||
rebuild re-runs dedup — the remaining provider becomes the sole
|
||||
occurrence and is left bare, so the session still matches.
|
||||
The first occurrence (in provider-id order) is left unchanged for backward
|
||||
compatibility with sessions that already store the original bare/slash
|
||||
model name. If that provider is later removed from the config, the next
|
||||
cache rebuild re-runs dedup — the remaining provider becomes the sole
|
||||
occurrence and is left unchanged, so the session still matches.
|
||||
|
||||
.. note::
|
||||
The "first occurrence wins" rule means the bare ID is not stable
|
||||
The "first occurrence wins" rule means the unchanged ID is not stable
|
||||
across config changes (adding, removing, or reordering providers).
|
||||
This is acceptable because the dedup runs on every cache rebuild,
|
||||
so sessions always resolve to the current canonical bare ID.
|
||||
so sessions always resolve to the current canonical unchanged ID.
|
||||
|
||||
The ``@provider_id:model`` format is consistent with the existing
|
||||
``_apply_provider_prefix()`` function and is handled by
|
||||
@@ -908,8 +908,8 @@ def _deduplicate_model_ids(groups: list[dict]) -> None:
|
||||
if not groups:
|
||||
return
|
||||
|
||||
# Collect {bare_id: [(group_idx, model_idx), ...]} in alphabetical
|
||||
# provider_id order so that the "first occurrence stays bare" rule is
|
||||
# Collect {model_id: [(group_idx, model_idx), ...]} in alphabetical
|
||||
# provider_id order so that the "first occurrence stays unchanged" rule is
|
||||
# deterministic across config edits (adding/removing/reordering providers).
|
||||
sorted_group_indices = sorted(
|
||||
range(len(groups)),
|
||||
@@ -918,34 +918,29 @@ def _deduplicate_model_ids(groups: list[dict]) -> None:
|
||||
id_map: dict[str, list[tuple[int, int]]] = {}
|
||||
for gi in sorted_group_indices:
|
||||
group = groups[gi]
|
||||
pid = group.get("provider_id", "")
|
||||
for mi, model in enumerate(group.get("models", [])):
|
||||
mid = model.get("id", "")
|
||||
# Skip IDs that are already provider-qualified
|
||||
if mid.startswith("@") or "/" in mid:
|
||||
mid = str(model.get("id", "") or "").strip()
|
||||
# Skip IDs that are already provider-qualified.
|
||||
if not mid or mid.startswith("@"):
|
||||
continue
|
||||
id_map.setdefault(mid, []).append((gi, mi))
|
||||
|
||||
# For any bare ID appearing in 2+ groups, prefix all but the first
|
||||
# occurrence. The first stays bare for backward compat; the rest
|
||||
# get ``@provider_id:id`` and a disambiguated label.
|
||||
# For any ID appearing in 2+ groups, prefix all but the first occurrence.
|
||||
# This handles N>2 providers correctly: the loop iterates over all
|
||||
# occurrences after the first, prefixing each with its own provider_id.
|
||||
for bare_id, locations in id_map.items():
|
||||
for original_id, locations in id_map.items():
|
||||
if len(locations) < 2:
|
||||
continue
|
||||
# Prefix all occurrences after the first
|
||||
for gi, mi in locations[1:]:
|
||||
group = groups[gi]
|
||||
model = group["models"][mi]
|
||||
pid = group.get("provider_id", "")
|
||||
model["id"] = f"@{pid}:{bare_id}"
|
||||
model["id"] = f"@{pid}:{original_id}"
|
||||
provider_name = group.get("provider", pid)
|
||||
# Update label to show provider for clarity
|
||||
if model.get("label") != bare_id:
|
||||
if model.get("label") != original_id:
|
||||
model["label"] = f"{model['label']} ({provider_name})"
|
||||
else:
|
||||
model["label"] = f"{bare_id} ({provider_name})"
|
||||
model["label"] = f"{original_id} ({provider_name})"
|
||||
|
||||
|
||||
def resolve_model_provider(model_id: str) -> tuple:
|
||||
@@ -1475,6 +1470,12 @@ def get_available_models() -> dict:
|
||||
|
||||
option_ids = [m.get("id", "") for g in groups for m in g.get("models", []) if m.get("id")]
|
||||
option_lookup = {str(opt_id): str(opt_id) for opt_id in option_ids}
|
||||
option_provider_lookup = {
|
||||
str(m.get("id")): str(g.get("provider_id") or "")
|
||||
for g in groups
|
||||
for m in g.get("models", [])
|
||||
if m.get("id")
|
||||
}
|
||||
norm_lookup: dict[str, list[str]] = {}
|
||||
for opt_id in option_ids:
|
||||
norm_lookup.setdefault(_norm_model_id(opt_id), []).append(opt_id)
|
||||
@@ -1493,8 +1494,9 @@ def get_available_models() -> dict:
|
||||
raw_candidates.append(candidate)
|
||||
|
||||
match_id = None
|
||||
exact_match = next((option_lookup[c] for c in raw_candidates if c in option_lookup), None)
|
||||
for candidate in raw_candidates:
|
||||
if candidate in option_lookup:
|
||||
if candidate in option_lookup and option_provider_lookup.get(candidate) == provider:
|
||||
match_id = option_lookup[candidate]
|
||||
break
|
||||
if match_id is None:
|
||||
@@ -1504,15 +1506,18 @@ def get_available_models() -> dict:
|
||||
if not matches:
|
||||
continue
|
||||
provider_match = next(
|
||||
(m for m in matches if m.startswith(f"@{provider}:") or m.startswith(f"{provider}/")),
|
||||
(m for m in matches if option_provider_lookup.get(m) == provider),
|
||||
None,
|
||||
)
|
||||
match_id = provider_match or matches[0]
|
||||
match_id = provider_match or exact_match or matches[0]
|
||||
if match_id:
|
||||
break
|
||||
|
||||
badge_payload = {"role": entry["role"], "label": entry["label"], "provider": provider}
|
||||
for candidate in raw_candidates:
|
||||
candidate_provider = option_provider_lookup.get(candidate)
|
||||
if candidate_provider and candidate_provider != provider:
|
||||
continue
|
||||
badges[candidate] = badge_payload
|
||||
if match_id:
|
||||
badges[match_id] = badge_payload
|
||||
|
||||
+2
-2
@@ -2244,7 +2244,7 @@ async function switchToProfile(name) {
|
||||
// ── Apply model ────────────────────────────────────────────────────────
|
||||
if (data.default_model) {
|
||||
const sel = $('modelSelect');
|
||||
const resolved = _applyModelToDropdown(data.default_model, sel);
|
||||
const resolved = _applyModelToDropdown(data.default_model, sel, window._activeProvider||null);
|
||||
const modelToUse = resolved || data.default_model;
|
||||
S._pendingProfileModel = modelToUse;
|
||||
// Only patch the in-memory session model if we're NOT about to replace the session
|
||||
@@ -2753,7 +2753,7 @@ async function loadSettingsPanel(){
|
||||
// picker renders blank for any user whose default was persisted without the
|
||||
// @-prefix — CLI-first users, legacy installs, etc.
|
||||
if(typeof _applyModelToDropdown==='function'){
|
||||
_applyModelToDropdown(_settingsHermesDefaultModelOnOpen, modelSel);
|
||||
_applyModelToDropdown(_settingsHermesDefaultModelOnOpen, modelSel, (models&&models.active_provider)||window._activeProvider||null);
|
||||
}else{
|
||||
modelSel.value=_settingsHermesDefaultModelOnOpen;
|
||||
}
|
||||
|
||||
+43
-15
@@ -192,16 +192,34 @@ window._configuredModelBadges=window._configuredModelBadges||{};
|
||||
// ── Smart model resolver ────────────────────────────────────────────────────
|
||||
// Finds the best matching option value in a <select> for a given model ID.
|
||||
// Handles mismatches like 'claude-sonnet-4-6' vs 'anthropic/claude-sonnet-4.6'.
|
||||
// Returns the matched option's value (already in the list), or null if no match.
|
||||
function _findModelInDropdown(modelId, sel){
|
||||
// When a preferred provider is supplied, duplicate normalized IDs prefer that
|
||||
// provider's option so Settings/profile rehydration doesn't snap back to the
|
||||
// first colliding entry.
|
||||
function _getOptionProviderId(opt){
|
||||
if(!opt) return '';
|
||||
const group=opt.parentElement;
|
||||
if(group && group.tagName==='OPTGROUP' && group.dataset && group.dataset.provider){
|
||||
return group.dataset.provider;
|
||||
}
|
||||
const value=String(opt.value||'');
|
||||
if(value.startsWith('@') && value.includes(':')) return value.slice(1,value.indexOf(':'));
|
||||
return '';
|
||||
}
|
||||
function _findModelInDropdown(modelId, sel, preferredProviderId){
|
||||
if(!modelId||!sel) return null;
|
||||
const opts=Array.from(sel.options).map(o=>o.value);
|
||||
// 1. Exact match
|
||||
if(opts.includes(modelId)) return modelId;
|
||||
// 2. Normalize: lowercase, strip namespace prefix, replace hyphens→dots
|
||||
// Also strip @provider: prefix from deduplicated model IDs (#1228).
|
||||
const options=Array.from(sel.options);
|
||||
const opts=options.map(o=>o.value);
|
||||
// 1. Normalize: lowercase, strip namespace prefix, replace hyphens→dots.
|
||||
// Also strip @provider: prefix from deduplicated model IDs (#1228, #1313).
|
||||
const norm=s=>s.toLowerCase().replace(/^[^/]+\//,'').replace(/^@([^:]+:)+/,'').replace(/-/g,'.');
|
||||
const target=norm(modelId);
|
||||
const preferred=String(preferredProviderId||'').toLowerCase();
|
||||
if(preferred){
|
||||
const providerMatch=options.find(o=>norm(o.value)===target && _getOptionProviderId(o).toLowerCase()===preferred);
|
||||
if(providerMatch) return providerMatch.value;
|
||||
}
|
||||
// 2. Exact match
|
||||
if(opts.includes(modelId)) return modelId;
|
||||
const exact=opts.find(o=>norm(o)===target);
|
||||
if(exact) return exact;
|
||||
// 3. Prefix/substring: require the candidate to start with the FULL normalized target
|
||||
@@ -217,9 +235,9 @@ function _findModelInDropdown(modelId, sel){
|
||||
|
||||
// Set the model picker to the best match for modelId.
|
||||
// Returns the resolved value that was actually set, or null if nothing matched.
|
||||
function _applyModelToDropdown(modelId, sel){
|
||||
function _applyModelToDropdown(modelId, sel, preferredProviderId){
|
||||
if(!modelId||!sel) return null;
|
||||
const resolved=_findModelInDropdown(modelId,sel);
|
||||
const resolved=_findModelInDropdown(modelId,sel,preferredProviderId);
|
||||
if(resolved){
|
||||
sel.value=resolved;
|
||||
if(sel.id==='modelSelect' && typeof syncModelChip==='function') syncModelChip();
|
||||
@@ -260,7 +278,7 @@ async function populateModelDropdown(){
|
||||
}
|
||||
// Set default model from server if no localStorage preference
|
||||
if(data.default_model && !localStorage.getItem('hermes-webui-model')){
|
||||
_applyModelToDropdown(data.default_model, sel);
|
||||
_applyModelToDropdown(data.default_model, sel, data.active_provider||null);
|
||||
}
|
||||
if(typeof syncModelChip==='function') syncModelChip();
|
||||
// Kick off a background live-model fetch for the active provider.
|
||||
@@ -410,15 +428,24 @@ function _normalizeConfiguredModelKey(modelId){
|
||||
return s.replace(/-/g,'.');
|
||||
}
|
||||
|
||||
function _getConfiguredModelBadge(modelId,badgeMap){
|
||||
function _getConfiguredModelBadge(modelId,badgeMap,providerId){
|
||||
const map=badgeMap||window._configuredModelBadges||{};
|
||||
if(!modelId||!map) return null;
|
||||
if(map[modelId]) return map[modelId];
|
||||
const provider=String(providerId||'').toLowerCase();
|
||||
const exact=map[modelId];
|
||||
if(exact && (!provider || !exact.provider || String(exact.provider).toLowerCase()===provider)) return exact;
|
||||
const targetNorm=_normalizeConfiguredModelKey(modelId);
|
||||
const matches=[];
|
||||
for(const [candidate,badge] of Object.entries(map)){
|
||||
if(_normalizeConfiguredModelKey(candidate)===targetNorm) return badge;
|
||||
if(_normalizeConfiguredModelKey(candidate)===targetNorm) matches.push(badge);
|
||||
}
|
||||
return null;
|
||||
if(!matches.length) return null;
|
||||
if(provider){
|
||||
const providerMatch=matches.find(badge=>String(badge&&badge.provider||'').toLowerCase()===provider);
|
||||
if(providerMatch) return providerMatch;
|
||||
return matches.length===1 ? matches[0] : null;
|
||||
}
|
||||
return matches[0];
|
||||
}
|
||||
|
||||
function syncModelChip(){
|
||||
@@ -461,8 +488,9 @@ function renderModelDropdown(){
|
||||
const _badgeMap=window._configuredModelBadges||{};
|
||||
for(const child of Array.from(sel.children)){
|
||||
if(child.tagName==='OPTGROUP'){
|
||||
const providerId=child.dataset&&child.dataset.provider?child.dataset.provider:'';
|
||||
for(const opt of Array.from(child.children)){
|
||||
_modelData.push({value:opt.value,name:esc(opt.textContent||getModelLabel(opt.value)),id:esc(opt.value),group:child.label||'',badge:_getConfiguredModelBadge(opt.value,_badgeMap)});
|
||||
_modelData.push({value:opt.value,name:esc(opt.textContent||getModelLabel(opt.value)),id:esc(opt.value),group:child.label||'',badge:_getConfiguredModelBadge(opt.value,_badgeMap,providerId)});
|
||||
}
|
||||
}
|
||||
if(child.tagName==='OPTION'){
|
||||
|
||||
@@ -93,11 +93,10 @@ class TestDeduplicateModelIds(unittest.TestCase):
|
||||
assert result[1]["models"][0]["id"] == "@beta:gpt-5.4"
|
||||
assert result[2]["models"][0]["id"] == "@gamma:gpt-5.4"
|
||||
|
||||
# ── Already-prefixed IDs are skipped ───────────────────────────
|
||||
# ── Already-prefixed IDs / slash IDs ───────────────────────────
|
||||
|
||||
def test_already_prefixed_ids_skipped(self):
|
||||
"""Model IDs already starting with @ or containing / are not
|
||||
considered for deduplication."""
|
||||
def test_already_prefixed_ids_and_unique_slash_ids_unchanged(self):
|
||||
"""Already-qualified IDs stay untouched; unique slash IDs are still allowed."""
|
||||
groups = [
|
||||
{"provider": "Anthropic", "provider_id": "anthropic", "models": [
|
||||
{"id": "@anthropic:claude-sonnet-4.6", "label": "Claude Sonnet 4.6"},
|
||||
@@ -107,10 +106,24 @@ class TestDeduplicateModelIds(unittest.TestCase):
|
||||
]},
|
||||
]
|
||||
result = self._call(groups)
|
||||
# Neither should be modified
|
||||
assert result[0]["models"][0]["id"] == "@anthropic:claude-sonnet-4.6"
|
||||
assert result[1]["models"][0]["id"] == "anthropic/claude-sonnet-4.6"
|
||||
|
||||
def test_two_providers_same_slash_qualified_model_prefixes_second(self):
|
||||
"""Slash-qualified duplicates must also be made unique (#1313)."""
|
||||
groups = [
|
||||
{"provider": "Alpha", "provider_id": "custom:alpha", "models": [
|
||||
{"id": "google/gemma-4-27b", "label": "Gemma 4 27B"},
|
||||
]},
|
||||
{"provider": "Beta", "provider_id": "custom:beta", "models": [
|
||||
{"id": "google/gemma-4-27b", "label": "Gemma 4 27B"},
|
||||
]},
|
||||
]
|
||||
result = self._call(groups)
|
||||
assert result[0]["models"][0]["id"] == "google/gemma-4-27b"
|
||||
assert result[1]["models"][0]["id"] == "@custom:beta:google/gemma-4-27b"
|
||||
assert result[1]["models"][0]["label"] == "Gemma 4 27B (Beta)"
|
||||
|
||||
# ── Mixed: some unique, some colliding ─────────────────────────
|
||||
|
||||
def test_mixed_unique_and_colliding(self):
|
||||
@@ -217,6 +230,39 @@ class TestFrontendNormRegex(unittest.TestCase):
|
||||
assert r1.stdout.strip() == r2.stdout.strip(), f"{r1.stdout.strip()} != {r2.stdout.strip()}"
|
||||
|
||||
|
||||
class TestFrontendPreferredProviderMatch(unittest.TestCase):
|
||||
"""Frontend: provider-aware rehydration should prefer the saved provider."""
|
||||
|
||||
@staticmethod
|
||||
def _read_js():
|
||||
import pathlib
|
||||
return (pathlib.Path(__file__).parent.parent / "static" / "ui.js").read_text()
|
||||
|
||||
def test_find_model_prefers_matching_provider_for_slash_collision(self):
|
||||
import re
|
||||
import subprocess
|
||||
|
||||
src = self._read_js()
|
||||
helper = re.search(r"function _getOptionProviderId\(opt\)\{.*?\n\}", src, re.S)
|
||||
finder = re.search(r"function _findModelInDropdown\(modelId, sel, preferredProviderId\)\{.*?\n\}", src, re.S)
|
||||
assert helper, "_getOptionProviderId() not found in ui.js"
|
||||
assert finder, "_findModelInDropdown() not found in ui.js"
|
||||
|
||||
script = f"""
|
||||
{helper.group(0)}
|
||||
{finder.group(0)}
|
||||
const sel = {{
|
||||
options: [
|
||||
{{ value: 'google/gemma-4-27b', parentElement: {{ tagName: 'OPTGROUP', dataset: {{ provider: 'custom:alpha' }} }} }},
|
||||
{{ value: '@custom:beta:google/gemma-4-27b', parentElement: {{ tagName: 'OPTGROUP', dataset: {{ provider: 'custom:beta' }} }} }},
|
||||
]
|
||||
}};
|
||||
console.log(_findModelInDropdown('google/gemma-4-27b', sel, 'custom:beta') || '');
|
||||
"""
|
||||
resolved = subprocess.run(["node", "-e", script], capture_output=True, text=True, check=True)
|
||||
assert resolved.stdout.strip() == "@custom:beta:google/gemma-4-27b"
|
||||
|
||||
|
||||
class TestResolveModelProviderColonInProviderId(unittest.TestCase):
|
||||
"""resolve_model_provider() must handle provider_ids containing ':'.
|
||||
|
||||
|
||||
@@ -15,8 +15,10 @@ def _models_with_cfg(model_cfg=None, fallback_providers=None, custom_providers=N
|
||||
config.cfg = {
|
||||
"model": model_cfg or {"provider": "openai-codex", "default": "gpt-5.4"},
|
||||
"fallback_providers": fallback_providers or [],
|
||||
"providers": custom_providers or {},
|
||||
"providers": {},
|
||||
}
|
||||
if custom_providers is not None:
|
||||
config.cfg["custom_providers"] = custom_providers
|
||||
if active_provider:
|
||||
config.cfg["model"]["provider"] = active_provider
|
||||
return config.get_available_models()
|
||||
@@ -49,6 +51,52 @@ def test_available_models_exposes_primary_and_fallback_badges():
|
||||
assert badges.get("anthropic/claude-haiku-4.5", {}).get("label") == "Fallback 2"
|
||||
|
||||
|
||||
def test_duplicate_slash_id_primary_badge_sticks_to_matching_provider_only():
|
||||
import textwrap
|
||||
|
||||
root = Path(__file__).resolve().parent.parent
|
||||
src = (root / "api" / "config.py").read_text(encoding="utf-8")
|
||||
start = src.index("def _build_configured_model_badges() -> dict[str, dict[str, str]]:")
|
||||
end = src.index(" return badges", start) + len(" return badges")
|
||||
fn_src = textwrap.dedent(src[start:end])
|
||||
|
||||
scope = {
|
||||
"active_provider": "custom:beta",
|
||||
"default_model": "google/gemma-4-27b",
|
||||
"cfg": {"fallback_providers": []},
|
||||
"groups": [
|
||||
{"provider": "Alpha", "provider_id": "custom:alpha", "models": [{"id": "google/gemma-4-27b"}]},
|
||||
{"provider": "Beta", "provider_id": "custom:beta", "models": [{"id": "@custom:beta:google/gemma-4-27b"}]},
|
||||
],
|
||||
"_resolve_provider_alias": lambda provider: provider,
|
||||
}
|
||||
exec(
|
||||
"def _norm_model_id(model_id):\n"
|
||||
" s=str(model_id or '').strip().lower()\n"
|
||||
" if s.startswith('@') and ':' in s: s=s.split(':',1)[1]\n"
|
||||
" if '/' in s: s=s.split('/',1)[1]\n"
|
||||
" return s.replace('-', '.')\n",
|
||||
scope,
|
||||
)
|
||||
exec(fn_src, scope)
|
||||
|
||||
badges = scope["_build_configured_model_badges"]()
|
||||
assert badges.get("@custom:beta:google/gemma-4-27b", {}).get("role") == "primary"
|
||||
assert "google/gemma-4-27b" not in badges, (
|
||||
"When duplicate slash-qualified IDs are deduplicated across providers, "
|
||||
"the shared raw ID must not keep the PRIMARY badge for the wrong provider."
|
||||
)
|
||||
|
||||
|
||||
def test_ui_badge_lookup_prefers_row_provider_for_duplicate_model_ids():
|
||||
root = Path(__file__).resolve().parent.parent
|
||||
js = (root / "static" / "ui.js").read_text(encoding="utf-8")
|
||||
|
||||
assert "function _getConfiguredModelBadge(modelId,badgeMap,providerId){" in js
|
||||
assert "child.dataset&&child.dataset.provider?child.dataset.provider:''" in js
|
||||
assert "const providerMatch=matches.find(badge=>String(badge&&badge.provider||'').toLowerCase()===provider);" in js
|
||||
|
||||
|
||||
def test_get_available_models_cache_preserves_configured_model_badges(tmp_path, monkeypatch):
|
||||
cache_path = tmp_path / "models_cache.json"
|
||||
old_cfg = config.cfg
|
||||
|
||||
Reference in new issue
Block a user