fix(model-switch): surface candidates on ambiguous alias instead of guessing
An alias that family-matches multiple catalog models (/model opus) used to silently pick one via _model_sort_key heuristics. The heuristics have guessed wrong repeatedly — dated snapshots like claude-opus-4-20250514 parsed as version 20,250,514 and outranked claude-opus-4-8; suffix tiebreaks landed on the cheapest tier — and every wrong guess silently switches the user to a model they did not ask for. resolve_alias now raises AmbiguousAliasError whenever more than one model matches the alias family; switch_model catches it at all three call sites (explicit-provider path, current-provider path, authenticated-provider fallback) and returns a failure result listing the candidates (best-guess-first ordering, capped at 10) with instructions to pick an exact name. A single match still resolves automatically, and DIRECT_ALIASES exact mappings are unaffected. The date-stamp split from #67571 is kept, demoted from selection logic to display ordering of the candidate list. Supersedes the auto-pick approach of #67571; credit to @Sahaun and @GottZ for the date-stamp parser analysis that this builds on.
This commit is contained in:
parent
21bc9ba341
commit
b79e83827d
|
|
@ -916,6 +916,37 @@ def _model_sort_key(model_id: str, prefix: str) -> tuple:
|
|||
return version_key + (suffix_rank, suffix) + date_key
|
||||
|
||||
|
||||
class AmbiguousAliasError(Exception):
|
||||
"""Alias family-matches multiple catalog models; caller must disambiguate.
|
||||
|
||||
Raised by :func:`resolve_alias` instead of silently picking one candidate
|
||||
via version-sort heuristics. ``candidates`` is sorted best-guess-first
|
||||
(see :func:`_model_sort_key`) for display purposes only.
|
||||
"""
|
||||
|
||||
def __init__(self, alias: str, provider: str, candidates: list[str]):
|
||||
self.alias = alias
|
||||
self.provider = provider
|
||||
self.candidates = candidates
|
||||
super().__init__(
|
||||
f"alias {alias!r} matches {len(candidates)} models on {provider}"
|
||||
)
|
||||
|
||||
|
||||
def _ambiguous_alias_message(err: "AmbiguousAliasError") -> str:
|
||||
"""User-facing disambiguation list for an ambiguous alias."""
|
||||
shown = err.candidates[:10]
|
||||
lines = "\n".join(f" {i}. {m}" for i, m in enumerate(shown, 1))
|
||||
more = ""
|
||||
if len(err.candidates) > len(shown):
|
||||
more = f"\n … and {len(err.candidates) - len(shown)} more"
|
||||
return (
|
||||
f"'{err.alias}' matches {len(err.candidates)} models on "
|
||||
f"{err.provider} — not switching automatically:\n{lines}{more}\n"
|
||||
f"Pick one with /model <exact-model-name>."
|
||||
)
|
||||
|
||||
|
||||
def resolve_alias(
|
||||
raw_input: str,
|
||||
current_provider: str,
|
||||
|
|
@ -987,9 +1018,15 @@ def resolve_alias(
|
|||
if not matches:
|
||||
return None
|
||||
|
||||
# Sort by version descending — prefer the latest/highest version
|
||||
# Sort by version descending (best guess first) for display, but NEVER
|
||||
# silently pick among multiple candidates: version-sort heuristics have
|
||||
# repeatedly guessed wrong (dated snapshots outranking point releases,
|
||||
# suffix tiebreaks landing on the cheapest tier). One match = resolve;
|
||||
# several = make the user choose.
|
||||
prefix_for_sort = f"{vendor}/{family}" if aggregator else family
|
||||
matches.sort(key=lambda m: _model_sort_key(m, prefix_for_sort))
|
||||
if len(matches) > 1:
|
||||
raise AmbiguousAliasError(key, current_provider, matches)
|
||||
return (current_provider, matches[0], key)
|
||||
|
||||
|
||||
|
|
@ -1026,6 +1063,9 @@ def _resolve_alias_fallback(
|
|||
"""
|
||||
providers = authenticated_providers or ("openrouter", "nous")
|
||||
for provider in providers:
|
||||
# AmbiguousAliasError propagates: the alias exists on this provider,
|
||||
# the user just has to choose — trying the next provider instead
|
||||
# would silently switch them somewhere they didn't ask to go.
|
||||
result = resolve_alias(raw_input, provider)
|
||||
if result is not None:
|
||||
return result
|
||||
|
|
@ -1422,7 +1462,15 @@ def switch_model(
|
|||
)
|
||||
|
||||
# Resolve alias on the TARGET provider
|
||||
alias_result = resolve_alias(new_model, target_provider)
|
||||
try:
|
||||
alias_result = resolve_alias(new_model, target_provider)
|
||||
except AmbiguousAliasError as err:
|
||||
return ModelSwitchResult(
|
||||
success=False,
|
||||
target_provider=target_provider,
|
||||
is_global=is_global,
|
||||
error_message=_ambiguous_alias_message(err),
|
||||
)
|
||||
if alias_result is not None:
|
||||
_, new_model, resolved_alias = alias_result
|
||||
|
||||
|
|
@ -1444,8 +1492,21 @@ def switch_model(
|
|||
alias_result = None
|
||||
else:
|
||||
alias_result = resolve_alias(raw_input, current_provider)
|
||||
except AmbiguousAliasError as err:
|
||||
return ModelSwitchResult(
|
||||
success=False,
|
||||
is_global=is_global,
|
||||
error_message=_ambiguous_alias_message(err),
|
||||
)
|
||||
except Exception:
|
||||
alias_result = resolve_alias(raw_input, current_provider)
|
||||
try:
|
||||
alias_result = resolve_alias(raw_input, current_provider)
|
||||
except AmbiguousAliasError as err:
|
||||
return ModelSwitchResult(
|
||||
success=False,
|
||||
is_global=is_global,
|
||||
error_message=_ambiguous_alias_message(err),
|
||||
)
|
||||
|
||||
# --- Step a: Try alias resolution on current provider ---
|
||||
|
||||
|
|
@ -1466,7 +1527,14 @@ def switch_model(
|
|||
user_providers=user_providers,
|
||||
custom_providers=custom_providers,
|
||||
)
|
||||
fallback_result = _resolve_alias_fallback(raw_input, authed)
|
||||
try:
|
||||
fallback_result = _resolve_alias_fallback(raw_input, authed)
|
||||
except AmbiguousAliasError as err:
|
||||
return ModelSwitchResult(
|
||||
success=False,
|
||||
is_global=is_global,
|
||||
error_message=_ambiguous_alias_message(err),
|
||||
)
|
||||
if fallback_result is not None:
|
||||
target_provider, new_model, resolved_alias = fallback_result
|
||||
logger.debug(
|
||||
|
|
|
|||
|
|
@ -283,36 +283,82 @@ class TestResolveAliasEdgeCases:
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestResolveAliasSorting:
|
||||
"""resolve_alias must pick the highest-version model, correctly
|
||||
ignoring YYYYMMDD snapshot stamps that would otherwise dwarf real
|
||||
version numbers (e.g. claude-opus-4-20250514 vs claude-opus-4-8)."""
|
||||
"""Aliases matching multiple catalog models must NOT silently pick one —
|
||||
resolve_alias raises AmbiguousAliasError with candidates sorted
|
||||
best-guess-first (dated snapshots demoted below real point versions)."""
|
||||
|
||||
def test_anthropic_opus_ambiguous_lists_candidates(self, monkeypatch):
|
||||
"""Multiple family matches surface a choice instead of auto-picking;
|
||||
the display ordering demotes date-stamped snapshots."""
|
||||
import pytest
|
||||
|
||||
def test_anthropic_opus_picks_latest(self, monkeypatch):
|
||||
"""Date-stamped snapshot ID must not outrank a higher point version."""
|
||||
import hermes_cli.model_switch as ms
|
||||
|
||||
monkeypatch.setattr("hermes_cli.models._PROVIDER_MODELS", {})
|
||||
monkeypatch.setattr(ms, "_ensure_direct_aliases", lambda: None)
|
||||
monkeypatch.setattr(ms, "DIRECT_ALIASES", {})
|
||||
monkeypatch.setattr(ms, "list_provider_models",
|
||||
lambda p: ["claude-opus-4-1", "claude-opus-4-7",
|
||||
"claude-opus-4-8",
|
||||
"claude-opus-4-20250514"])
|
||||
result = ms.resolve_alias("opus", "anthropic")
|
||||
assert result is not None and result[1] == "claude-opus-4-8"
|
||||
with pytest.raises(ms.AmbiguousAliasError) as exc:
|
||||
ms.resolve_alias("opus", "anthropic")
|
||||
assert exc.value.candidates[0] == "claude-opus-4-8"
|
||||
assert set(exc.value.candidates) == {
|
||||
"claude-opus-4-1", "claude-opus-4-7",
|
||||
"claude-opus-4-8", "claude-opus-4-20250514",
|
||||
}
|
||||
|
||||
def test_unsynced_new_model_sorts_first(self, monkeypatch):
|
||||
"""A just-released model missing from models.dev still ranks above
|
||||
older, dated siblings in the candidate ordering."""
|
||||
import pytest
|
||||
|
||||
def test_unsynced_new_model_wins(self, monkeypatch):
|
||||
"""A just-released model missing from models.dev still outranks
|
||||
older, dated siblings (static _PROVIDER_MODELS merge scenario)."""
|
||||
import hermes_cli.model_switch as ms
|
||||
|
||||
monkeypatch.setattr("hermes_cli.models._PROVIDER_MODELS", {})
|
||||
monkeypatch.setattr(ms, "_ensure_direct_aliases", lambda: None)
|
||||
monkeypatch.setattr(ms, "DIRECT_ALIASES", {})
|
||||
monkeypatch.setattr(ms, "list_provider_models",
|
||||
lambda p: ["claude-opus-4-7", "claude-opus-4-8",
|
||||
"claude-opus-4-20250514",
|
||||
"claude-opus-4-9"])
|
||||
with pytest.raises(ms.AmbiguousAliasError) as exc:
|
||||
ms.resolve_alias("opus", "anthropic")
|
||||
assert exc.value.candidates[0] == "claude-opus-4-9"
|
||||
|
||||
def test_single_match_resolves_without_error(self, monkeypatch):
|
||||
"""Exactly one family match still resolves automatically."""
|
||||
import hermes_cli.model_switch as ms
|
||||
|
||||
monkeypatch.setattr("hermes_cli.models._PROVIDER_MODELS", {})
|
||||
monkeypatch.setattr(ms, "_ensure_direct_aliases", lambda: None)
|
||||
monkeypatch.setattr(ms, "DIRECT_ALIASES", {})
|
||||
monkeypatch.setattr(ms, "list_provider_models",
|
||||
lambda p: ["claude-opus-4-8", "claude-sonnet-4-6"])
|
||||
result = ms.resolve_alias("opus", "anthropic")
|
||||
assert result is not None and result[1] == "claude-opus-4-9"
|
||||
assert result is not None and result[1] == "claude-opus-4-8"
|
||||
|
||||
def test_switch_model_surfaces_ambiguity_message(self, monkeypatch):
|
||||
"""switch_model returns a failure result listing the candidates
|
||||
instead of switching to a heuristic guess."""
|
||||
import hermes_cli.model_switch as ms
|
||||
|
||||
monkeypatch.setattr("hermes_cli.models._PROVIDER_MODELS", {})
|
||||
monkeypatch.setattr(ms, "_ensure_direct_aliases", lambda: None)
|
||||
monkeypatch.setattr(ms, "DIRECT_ALIASES", {})
|
||||
monkeypatch.setattr(ms, "list_provider_models",
|
||||
lambda p: ["claude-opus-4-8",
|
||||
"claude-opus-4-20250514"])
|
||||
result = ms.switch_model(
|
||||
"opus",
|
||||
current_provider="anthropic",
|
||||
current_model="claude-sonnet-4-6",
|
||||
)
|
||||
assert result.success is False
|
||||
assert "claude-opus-4-8" in result.error_message
|
||||
assert "claude-opus-4-20250514" in result.error_message
|
||||
assert "not switching automatically" in result.error_message
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue