diff --git a/amplifier_foundation/spawn_utils.py b/amplifier_foundation/spawn_utils.py index 6f4f336..ead4ddc 100644 --- a/amplifier_foundation/spawn_utils.py +++ b/amplifier_foundation/spawn_utils.py @@ -618,10 +618,10 @@ def _find_provider_instance( # --------------------------------------------------------------------------- -# "Which instance does a BARE module type mean?" -- one answer, three callers +# "Which instance does a preference mean?" -- one answer, every caller # --------------------------------------------------------------------------- # -# WHY THIS EXISTS (model_performance-67u) +# WHY THIS EXISTS (model_performance-67u, then recipes-0ac) # # A routing matrix addresses providers by bare module type (`provider: # anthropic`), but a mount plan may carry SEVERAL instances of that module, @@ -629,8 +629,8 @@ def _find_provider_instance( # routing-matrix bundle asks for (see _find_provider_instance's docstring: # distinct `id:`s exist "for routing-matrix disambiguation"). # -# Three helpers in this file used to answer "which instance is `anthropic`?" -# three DIFFERENT ways: +# ROUND 1 (model_performance-67u): three helpers in this file answered +# "which instance is `anthropic`?" three DIFFERENT ways: # # _find_provider_instance -> highest priority (lowest number) # _find_provider_index -> first declared @@ -646,10 +646,38 @@ def _find_provider_instance( # and B was promoted to priority 0 -- right model, wrong instance, and with # it B's base_url / long-context / cache-retention settings. Silently. # -# The rule below is now the single answer, and it is the one the caller -# already expressed: HIGHEST PRIORITY WINS, ties broken by declaration order. -# An explicit instance `id:` is a more specific address than a bare module -# type and always beats it. +# ROUND 2 (recipes-0ac), fixed here: even with ONE agreed rule, the rule was +# MODEL-BLIND. A preference is a (provider, model) PAIR, but only the +# `provider` half ever reached the resolution -- so on a measured 14-provider +# host (module `provider-anthropic` mounted as opus/priority 1, sonnet/5, +# fable/6) a preference {provider: anthropic, model: claude-sonnet-4-5} +# promoted `opus` (the highest-priority anthropic mount) and stamped +# `claude-sonnet-4-5` onto opus's config. Same substitution class as round 1 +# -- right model name, wrong instance, and with it that instance's base_url / +# context-window / cache-retention settings -- reached by a different route. +# +# THE RULE, in one place (:func:`_resolve_provider_index`): +# +# 1. An explicit instance `id:` is the most specific address there is and +# always wins outright -- unchanged behaviour. +# 2. Otherwise the name is a MODULE (module id or short name). Among that +# module's mounted instances, prefer the ones whose locally-known models +# satisfy the preference's model hint. This is the half that was +# missing: it is what makes `{anthropic, claude-sonnet-4-5}` mean +# `fable` rather than "whichever anthropic mount ranks first". +# 3. Among whatever survives step 2, HIGHEST PRIORITY WINS, ties broken by +# declaration order -- never "last declared". +# +# Step 2 only ever NARROWS an already-correct candidate set: if no instance +# advertises a matching model (the common case -- most mount configs carry no +# model metadata at all), every candidate survives and step 3 decides exactly +# as it did before. Single-instance plans are therefore untouched by +# construction, whatever the model hint says. +# +# Step 2 is deliberately SYNCHRONOUS and local: it reads only what the mount +# plan already states. It never queries a provider's live catalog -- that is +# resolve_model_pattern()'s job, it is async, and _build_provider_lookup / +# _find_provider_index are sync helpers with sync callers. def _provider_priority(provider: dict[str, Any]) -> int: @@ -666,39 +694,129 @@ def _provider_priority(provider: dict[str, Any]) -> int: return 0 +def _declared_models(provider: dict[str, Any]) -> list[str]: + """Model names a mount-plan entry states it serves, WITHOUT any I/O. + + Reads only the mount plan itself: the instance's ``default_model`` plus, + when a plan happens to declare one, a ``models`` list. Returns ``[]`` when + the entry says nothing about models -- which is the common case, and which + callers must treat as "no information", never as "matches nothing". + """ + config = provider.get("config") or {} + models: list[str] = [] + + declared = config.get("models") + if isinstance(declared, (list, tuple)): + models.extend(m for m in declared if isinstance(m, str) and m) + + default_model = config.get("default_model") + if isinstance(default_model, str) and default_model: + models.append(default_model) + + return models + + +def _model_hint_matches(model_name: str, model_hint: str) -> bool: + """Does a concrete model name satisfy a preference's model hint? + + Uses the same case-insensitive glob convention + :func:`resolve_model_pattern` already applies to a provider's live model + list, so a hint that would resolve against the live catalog is the same + hint that selects the instance here. Exact (non-glob) hints work too -- + fnmatch treats a pattern with no wildcard as an equality test. + """ + return fnmatch.fnmatch(model_name.lower(), model_hint.lower()) + + +def _resolve_provider_index( + providers: list[dict[str, Any]], + provider_id: str, + model_hint: str | None = None, +) -> int | None: + """THE answer to "which mounted instance does this preference name?". + + Every name-to-instance resolution in this module funnels through here so + the helpers cannot drift apart again (see the module comment above). + + Args: + providers: List of provider configs from mount plan. + provider_id: Provider to find -- an instance ``id``, a module id, or + a module short name ("anthropic" for "provider-anthropic"). + model_hint: Optional model name or glob from the same preference. + Used ONLY to choose between several instances of one module, and + only when at least one of them declares a matching model. Never + causes a miss: a hint nothing matches is simply not consulted. + + Returns: + Index of the resolved provider, or None if no entry matches the name. + """ + # 1. An explicit instance id is the most specific address there is. + for i, p in enumerate(providers): + if p.get("id", "") == provider_id: + return i + + # 2. Otherwise the name addresses a MODULE -- gather every instance of it. + candidates = [ + i + for i, p in enumerate(providers) + if provider_id + in (p.get("module", ""), p.get("module", "").replace("provider-", "")) + ] + if not candidates: + return None + + # 3. Narrow by the model half of the preference, when it discriminates. + # An empty result means the plan simply carries no model metadata to + # judge by, so every candidate stays in the running. + if model_hint: + matching = [ + i + for i in candidates + if any( + _model_hint_matches(model, model_hint) + for model in _declared_models(providers[i]) + ) + ] + if matching: + if len(matching) < len(candidates): + logger.debug( + "Provider %r narrowed to %d/%d instance(s) by model hint %r", + provider_id, + len(matching), + len(candidates), + model_hint, + ) + candidates = matching + + # 4. Highest priority wins; ties broken by declaration order. + return min(candidates, key=lambda i: (_provider_priority(providers[i]), i)) + + def _find_provider_index( providers: list[dict[str, Any]], provider_id: str, + model_hint: str | None = None, ) -> int | None: """Find the index of a provider in the providers list. Supports flexible matching: "anthropic", "provider-anthropic", or full module ID. - When several instances of the same module type are mounted, the - highest-priority one wins (ties: declaration order) -- see the - module comment above :func:`_provider_priority`. An exact instance - ``id`` match is more specific and beats any module-type match. + Thin wrapper over :func:`_resolve_provider_index` -- kept as the named + entry point its existing callers and tests use. ``model_hint`` is + optional; omitting it asks the module-type question on its own, exactly + as this helper always did. Args: providers: List of provider configs from mount plan. provider_id: Provider to find. + model_hint: Optional model name/glob to disambiguate between several + instances of the same module. Returns: Index of the provider, or None if not found. """ - for i, p in enumerate(providers): - if p.get("id", "") == provider_id: - return i - - best: tuple[int, int] | None = None - for i, p in enumerate(providers): - module_id = p.get("module", "") - if provider_id in (module_id, module_id.replace("provider-", "")): - priority = _provider_priority(p) - if best is None or priority < best[0]: - best = (priority, i) - return None if best is None else best[1] + return _resolve_provider_index(providers, provider_id, model_hint) def _build_provider_lookup( @@ -706,12 +824,20 @@ def _build_provider_lookup( ) -> dict[str, int]: """Build a lookup dict mapping provider names to indices. - Module-type keys ("anthropic", "provider-anthropic", the full module - id) resolve to the HIGHEST-PRIORITY instance of that module, not the - last-declared one -- see the module comment above - :func:`_provider_priority` for the defect that motivated this. - Instance ``id`` keys are the most specific address and always win, - even when an id collides with a module-type name. + Every value is produced by :func:`_resolve_provider_index`, so this + lookup and :func:`_find_provider_index` cannot disagree -- they are the + same function, and agreement is structural rather than two + implementations that happen to coincide. + + Module-type keys ("anthropic", "provider-anthropic", the full module id) + resolve to the HIGHEST-PRIORITY instance of that module, not the + last-declared one. Instance ``id`` keys are the most specific address and + always win, even when an id collides with a module-type name. + + This lookup is model-BLIND by construction: a dict keyed by provider name + alone cannot express "which instance for THIS model". Callers holding a + (provider, model) preference should call :func:`_resolve_provider_index` + with the model hint instead -- see the module comment above. Args: providers: List of provider configs from mount plan. @@ -719,29 +845,28 @@ def _build_provider_lookup( Returns: Dict mapping various name formats to provider index. """ - # Pass 1: module-type keys, resolved by priority rather than by - # whichever entry happened to be written to the dict last. - best: dict[str, tuple[int, int]] = {} - for i, p in enumerate(providers): + lookup: dict[str, int] = {} + + for p in providers: module_id = p.get("module", "") short_name = module_id.replace("provider-", "") - keys = [module_id, f"provider-{short_name}"] - if short_name != module_id: - keys.append(short_name) - priority = _provider_priority(p) - for key in keys: - current = best.get(key) - if current is None or priority < current[0]: - best[key] = (priority, i) - - lookup: dict[str, int] = {key: idx for key, (_, idx) in best.items()} - - # Pass 2: an explicit instance id is the most specific address there - # is, so it overwrites any module-type key it collides with. + for key in (module_id, f"provider-{short_name}", short_name): + if not key or key in lookup: + continue + resolved = _resolve_provider_index(providers, key) + if resolved is not None: + lookup[key] = resolved + + # An explicit instance id is the most specific address there is, so it + # overwrites any module-type key it collides with. (_resolve_provider_index + # already applies this precedence; re-asserting it here keeps every + # instance addressable by its own id even if its id never appears as a + # module-type key above.) for i, p in enumerate(providers): instance_id = p.get("id") if instance_id: lookup[instance_id] = i + return lookup @@ -778,13 +903,12 @@ def apply_provider_preferences( logger.warning("Provider preferences specified but no providers in mount plan") return mount_plan - # Build lookup for efficient matching - lookup = _build_provider_lookup(providers) - - # Find first matching preference + # Find first matching preference. The preference is resolved as a PAIR: + # its model participates in choosing WHICH instance of a module-named + # provider is meant, not just what gets stamped onto the winner. for pref in preferences: - if pref.provider in lookup: - target_idx = lookup[pref.provider] + target_idx = _resolve_provider_index(providers, pref.provider, pref.model) + if target_idx is not None: return _apply_single_override( mount_plan, providers, target_idx, pref.model, pref.config ) @@ -926,9 +1050,6 @@ async def apply_provider_preferences_with_resolution( logger.warning("Provider preferences specified but no providers in mount plan") return mount_plan - # Build lookup for efficient matching - lookup = _build_provider_lookup(providers) - # Find first matching preference whose model actually resolves, and # apply it. A preference whose provider is present but whose glob # pattern fails to resolve (no matching models) is NOT applied with @@ -937,9 +1058,12 @@ async def apply_provider_preferences_with_resolution( # the ordered list, mirroring resolve_model_role()'s `continue` # behavior in the sibling routing-matrix resolver. for pref in preferences: - if pref.provider in lookup: - target_idx = lookup[pref.provider] - + # Resolved as a PAIR: pref.model participates in choosing which + # instance of a module-named provider is meant (see the module + # comment above _provider_priority), so the model glob below is + # resolved against the very instance that will be promoted. + target_idx = _resolve_provider_index(providers, pref.provider, pref.model) + if target_idx is not None: # Resolve model pattern if it's a glob resolved_model = pref.model if is_glob_pattern(pref.model): diff --git a/tests/test_spawn_utils.py b/tests/test_spawn_utils.py index e3bbd3d..f5b9b14 100644 --- a/tests/test_spawn_utils.py +++ b/tests/test_spawn_utils.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +from typing import Any from unittest.mock import AsyncMock import asyncio from unittest.mock import MagicMock @@ -1508,3 +1509,284 @@ def test_override_selecting_primary_itself_no_demotion_needed( "No tie-break demotion should occur when the override target " "is already the sole priority=0 instance" ) + + +# ============================================================================= +# recipes-0ac -- a preference is a (provider, model) PAIR +# ============================================================================= +# +# Measured 2026-09-02 on a 14-provider host. Module `provider-anthropic` is +# mounted three times with distinct ids and priorities; the routing matrix +# addresses it by MODULE name ("anthropic") and discriminates with the model +# glob. Before the fix the model half never reached instance resolution, so +# every {anthropic, *} preference landed on whichever anthropic mount ranked +# first and stamped the requested model onto THAT instance's config -- right +# model name, wrong instance, and with it the wrong base_url / context window +# / cache-retention settings. Downstream this put a reasoning-role agent on a +# 65K-context mount and produced 400s. + + +MEASURED_HOST: list[dict[str, Any]] = [ + { + "id": "opus", + "module": "provider-anthropic", + "config": {"priority": 1, "default_model": "claude-opus-5"}, + }, + { + "id": "sonnet", + "module": "provider-anthropic", + "config": {"priority": 5, "default_model": "claude-sonnet-5"}, + }, + { + "id": "fable", + "module": "provider-anthropic", + "config": {"priority": 6, "default_model": "claude-sonnet-4-5"}, + }, + { + "id": "gemini", + "module": "provider-gemini", + "config": {"priority": 3, "default_model": "gemini-3-pro"}, + }, +] + + +def _measured_host() -> dict[str, Any]: + """A fresh, deeply-copied copy of the measured mount plan.""" + return {"providers": [{**p, "config": dict(p["config"])} for p in MEASURED_HOST]} + + +def _promoted(plan: dict[str, Any]) -> dict[str, Any]: + """The single instance the override promoted to priority 0.""" + winners = [p for p in plan["providers"] if p["config"].get("priority") == 0] + assert len(winners) == 1, f"expected exactly one promoted mount, got {winners}" + return winners[0] + + +def _by_id(plan: dict[str, Any], instance_id: str) -> dict[str, Any]: + return next(p for p in plan["providers"] if p["id"] == instance_id) + + +class TestModuleNamedPreferenceResolvesToMatchingInstance: + """Module-named preferences pick the instance that serves the model.""" + + def test_model_glob_selects_matching_instance_not_first_ranked(self) -> None: + """{anthropic, claude-opus-*} means `opus`, and only `opus`.""" + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="claude-opus-*")], + ) + + assert _promoted(result)["id"] == "opus" + + # The instance that does NOT serve this model keeps its own config -- + # no stray promotion, no stamped-on model, no borrowed settings. + fable = _by_id(result, "fable") + assert fable["config"]["priority"] == 6 + assert fable["config"]["default_model"] == "claude-sonnet-4-5" + + def test_model_selects_lower_priority_instance_that_serves_it(self) -> None: + """The fix proper: the model half outranks bare priority order. + + Fails before the fix -- `opus` (priority 1, the highest-ranked + anthropic mount) was promoted and `claude-sonnet-4-5` written onto + ITS config, even though `fable` is the mount that serves that model. + """ + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="claude-sonnet-4-5")], + ) + + promoted = _promoted(result) + assert promoted["id"] == "fable" + assert promoted["config"]["default_model"] == "claude-sonnet-4-5" + + opus = _by_id(result, "opus") + assert opus["config"]["priority"] == 1 + assert opus["config"]["default_model"] == "claude-opus-5" + + def test_no_model_falls_back_to_highest_priority_instance(self) -> None: + """With nothing to discriminate on, highest priority wins.""" + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="")], + ) + assert _promoted(result)["id"] == "opus" + + def test_unmatched_model_still_falls_back_to_highest_priority(self) -> None: + """A model no mount declares must never turn into a MISS. + + Model metadata in a mount plan is optional and often absent; a hint + that matches nothing carries no information and must not stop the + preference from being applied at all. + """ + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="claude-unknown-9")], + ) + assert _promoted(result)["id"] == "opus" + + def test_instance_id_preference_is_exact_and_ignores_model(self) -> None: + """Naming an instance id addresses that instance, full stop.""" + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="fable", model="claude-sonnet-4-5")], + ) + assert _promoted(result)["id"] == "fable" + + # Even a model only a SIBLING serves does not redirect an explicit id. + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="fable", model="claude-opus-5")], + ) + assert _promoted(result)["id"] == "fable" + + def test_other_module_untouched(self) -> None: + """Narrowing within one module never reaches across modules.""" + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="claude-sonnet-4-5")], + ) + gemini = _by_id(result, "gemini") + assert gemini["config"]["priority"] == 3 + assert gemini["config"]["default_model"] == "gemini-3-pro" + + def test_single_instance_module_is_unchanged_by_any_model(self) -> None: + """The common single-mount case resolves regardless of the model.""" + plan = { + "providers": [ + { + "module": "provider-anthropic", + "config": {"default_model": "claude-opus-5"}, + }, + {"module": "provider-openai", "config": {}}, + ] + } + for model in ("claude-opus-5", "claude-sonnet-4-5", "totally-unknown", ""): + result = apply_provider_preferences( + { + "providers": [ + {**p, "config": dict(p["config"])} for p in plan["providers"] + ] + }, + [ProviderPreference(provider="anthropic", model=model)], + ) + promoted = _promoted(result) + assert promoted["module"] == "provider-anthropic", f"model={model!r}" + assert promoted["config"]["default_model"] == model, f"model={model!r}" + + def test_declared_models_list_participates_when_present(self) -> None: + """A mount that declares a `models` list is selectable by any of them.""" + plan = { + "providers": [ + { + "id": "primary", + "module": "provider-anthropic", + "config": {"priority": 0, "default_model": "claude-opus-5"}, + }, + { + "id": "long-context", + "module": "provider-anthropic", + "config": { + "priority": 9, + "default_model": "claude-opus-5", + "models": ["claude-opus-5", "claude-opus-5-1m"], + }, + }, + ] + } + result = apply_provider_preferences( + plan, [ProviderPreference(provider="anthropic", model="claude-opus-5-1m")] + ) + assert _promoted(result)["id"] == "long-context" + + def test_model_matching_is_case_insensitive(self) -> None: + """Model globs fold case, matching resolve_model_pattern().""" + result = apply_provider_preferences( + _measured_host(), + [ProviderPreference(provider="anthropic", model="CLAUDE-SONNET-4-5")], + ) + assert _promoted(result)["id"] == "fable" + + +class TestProviderResolutionHelpersAgree: + """`_find_provider_index` and `_build_provider_lookup` are one function.""" + + def test_helpers_agree_on_every_addressable_name(self) -> None: + lookup = _build_provider_lookup(MEASURED_HOST) + names = [ + "anthropic", + "provider-anthropic", + "gemini", + "provider-gemini", + "opus", + "sonnet", + "fable", + ] + for name in names: + assert _find_provider_index(MEASURED_HOST, name) == lookup[name], name + + def test_module_name_resolves_to_highest_priority_instance(self) -> None: + """Never the last-declared one (`fable`, priority 6).""" + lookup = _build_provider_lookup(MEASURED_HOST) + assert lookup["anthropic"] == 0 + assert lookup["provider-anthropic"] == 0 + assert _find_provider_index(MEASURED_HOST, "anthropic") == 0 + + def test_find_provider_index_honours_the_model_hint(self) -> None: + """The hint is optional; supplying it narrows to the serving mount.""" + assert _find_provider_index(MEASURED_HOST, "anthropic") == 0 + assert ( + _find_provider_index(MEASURED_HOST, "anthropic", "claude-sonnet-4-5") == 2 + ) + assert _find_provider_index(MEASURED_HOST, "anthropic", "claude-opus-*") == 0 + + def test_unknown_name_is_still_a_miss(self) -> None: + assert _find_provider_index(MEASURED_HOST, "cohere") is None + assert _find_provider_index(MEASURED_HOST, "cohere", "command-r") is None + assert "cohere" not in _build_provider_lookup(MEASURED_HOST) + + +class TestModuleNamedPreferenceWithAsyncResolution: + """The async path resolves the glob against the instance it promotes.""" + + @pytest.mark.asyncio + async def test_async_path_promotes_the_model_matching_instance(self) -> None: + provider = MagicMock() + provider.list_models = AsyncMock( + return_value=["claude-opus-5", "claude-sonnet-5", "claude-sonnet-4-5"] + ) + coordinator = MagicMock() + coordinator.get = MagicMock(return_value={"anthropic": provider}) + + result = await apply_provider_preferences_with_resolution( + _measured_host(), + [ProviderPreference(provider="anthropic", model="claude-sonnet-4-*")], + coordinator, + ) + + promoted = _promoted(result) + assert promoted["id"] == "fable" + assert promoted["config"]["default_model"] == "claude-sonnet-4-5" + + @pytest.mark.asyncio + async def test_async_path_preserves_protected_config_keys(self) -> None: + """PROTECTED_CONFIG_KEYS survive selection by model, as ever.""" + plan = _measured_host() + _by_id(plan, "fable")["config"]["api_key"] = "fable-secret" + + result = await apply_provider_preferences_with_resolution( + plan, + [ + ProviderPreference( + provider="anthropic", + model="claude-sonnet-4-5", + config={"api_key": "injected", "reasoning_effort": "high"}, + ) + ], + MagicMock(get=MagicMock(return_value={})), + ) + + promoted = _promoted(result) + assert promoted["id"] == "fable" + assert promoted["config"]["api_key"] == "fable-secret" + assert promoted["config"]["reasoning_effort"] == "high"