diff --git a/amplifier_foundation/spawn_utils.py b/amplifier_foundation/spawn_utils.py index 60fbe91..6f4f336 100644 --- a/amplifier_foundation/spawn_utils.py +++ b/amplifier_foundation/spawn_utils.py @@ -617,6 +617,55 @@ def _find_provider_instance( return providers[candidates[0][1]] +# --------------------------------------------------------------------------- +# "Which instance does a BARE module type mean?" -- one answer, three callers +# --------------------------------------------------------------------------- +# +# WHY THIS EXISTS (model_performance-67u) +# +# A routing matrix addresses providers by bare module type (`provider: +# anthropic`), but a mount plan may carry SEVERAL instances of that module, +# each with its own `id:` and `priority:` -- which is precisely the shape the +# 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: +# +# _find_provider_instance -> highest priority (lowest number) +# _find_provider_index -> first declared +# _build_provider_lookup -> LAST declared (plain dict, last write wins) +# +# apply_provider_preferences_with_resolution calls TWO of them in one pass: +# it resolves the candidate's model glob against the instance +# _find_provider_instance picks, then promotes the index +# _build_provider_lookup returns. On a 10-mount plan with 2 module types +# (the eval harness roster: sol/terra/openai/luna/luna-max + +# opus-4.8/opus/sonnet/haiku/fable) those are different mounts, so the model +# resolved from instance A's model list was written onto instance B's config +# 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. + + +def _provider_priority(provider: dict[str, Any]) -> int: + """Priority of a mount-plan provider entry; lower ranks higher. + + Missing/unparseable priority sorts as 0 (highest), matching + :func:`_find_provider_instance`, so plans that never set ``priority`` + keep resolving by declaration order. + """ + raw = (provider.get("config") or {}).get("priority", 0) + try: + return int(raw) + except (TypeError, ValueError): + return 0 + + def _find_provider_index( providers: list[dict[str, Any]], provider_id: str, @@ -626,6 +675,11 @@ def _find_provider_index( 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. + Args: providers: List of provider configs from mount plan. provider_id: Provider to find. @@ -634,16 +688,17 @@ def _find_provider_index( Index of the provider, or None if not found. """ for i, p in enumerate(providers): - module_id = p.get("module", "") - instance_id = p.get("id", "") - if provider_id in ( - module_id, - module_id.replace("provider-", ""), - f"provider-{provider_id}", - instance_id, - ): + if p.get("id", "") == provider_id: return i - return None + + 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] def _build_provider_lookup( @@ -651,23 +706,39 @@ 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. + Args: providers: List of provider configs from mount plan. Returns: Dict mapping various name formats to provider index. """ - lookup: dict[str, int] = {} + # 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): module_id = p.get("module", "") - lookup[module_id] = i - # Also index by short name short_name = module_id.replace("provider-", "") + keys = [module_id, f"provider-{short_name}"] if short_name != module_id: - lookup[short_name] = i - # And with provider- prefix - lookup[f"provider-{short_name}"] = i - # Add id-based lookup if present + 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 i, p in enumerate(providers): instance_id = p.get("id") if instance_id: lookup[instance_id] = i diff --git a/docs/lanes/67u-named-delegate-matrix-bypass/DONE-NOTE.md b/docs/lanes/67u-named-delegate-matrix-bypass/DONE-NOTE.md new file mode 100644 index 0000000..4cdf353 --- /dev/null +++ b/docs/lanes/67u-named-delegate-matrix-bypass/DONE-NOTE.md @@ -0,0 +1,303 @@ +# DONE-NOTE — 67u: why does an EXPLICITLY-NAMED delegate bypass matrix resolution? + +**Item:** `model_performance-67u` · **Branch:** `lane/67u-named-delegate-matrix-bypass` +**Repo:** `microsoft/amplifier-foundation` @ `18efe87` (origin/main) +**Spend: $0.00 against a $0 authority.** No API calls, no DTU, no probe re-runs. +Everything below is a code read plus pure-function reproduction. Accounting in §7. + +--- + +## VERDICT (one line) + +**There is no bypass.** `tool-delegate` has exactly ONE resolver call site, and it is +guarded on a `model_role` that is read **only from the tool input**. Naming an agent in +the prompt does not take a different path — it just produces a tool call with no +`model_role` argument, so the guard is false and the delegate falls through to the +session default. That fall-through is **intended, documented, and opt-out** +(`strict_model_role`). **No shipped routing decision is wrong today** (§5). + +While answering that, the code read turned up **a real, separate defect in this repo** +(§4): three helpers in `spawn_utils.py` gave three different answers to "which mounted +instance does the bare module type `anthropic` mean?", and one function used two of them +in a single pass. Fixed, with fail-before tests. + +--- + +## 1. THE MECHANISM, AT file:line — both call sites side by side + +`modules/tool-delegate/amplifier_module_tool_delegate/__init__.py` @ `18efe87`. + +**Where `model_role` comes from — the only place:** + +```python +1625: raw_model_role = input.get("model_role", "").strip() +``` + +**The only resolver call site, and its guard:** + +```python +1636: if raw_model_role and provider_preferences is None: +1637: resolver = ( +1638: self.coordinator.get_capability("model_role_resolver") +... +1650: resolved = await resolver.resolve(raw_model_role) +1651: if resolved: +1653: provider_preferences = list(resolved) +``` + +**The only agent-level routing input the spawn path reads — note what is NOT there:** + +```python +1819: # Apply agent-level default provider_preferences if caller didn't specify +1820: if provider_preferences is None and self.provider_selection_enabled: +1821: agent_cfg = agents.get(agent_name, {}) +1822: agent_default_prefs = agent_cfg.get("provider_preferences", []) +``` + +`agent_cfg.get("model_role")` **does not appear anywhere in the module.** Verified: +`grep -n 'agent_cfg\|agents\.get' __init__.py` returns lines 1821/1822 (preferences), +2061/2062 and 2510/2515 (return-contract) — and nothing else. + +### The answer to the item's primary question + +The two paths are **the same code**. The difference is entirely in the tool-call +arguments the root model emitted: + +| | organic (h7n arm A, S3) | named (h7n probe) | +|---|---|---| +| tool arguments | `agent=…`, `instruction=…`, **`model_role="reasoning"`** | `agent=…`, `instruction=…` | +| guard at `:1636` | **true** | **false** | +| `resolver.resolve()` | called | **never called** | +| `provider_preferences` | matrix candidate + its `config` | `None` | +| `routing_matrix` provenance | populated | `None` (correctly — no matrix produced it) | +| child model | matrix's model **and** matrix's effort (`high` ≠ root's `medium`) | session default | + +The probe's prompt (`scripts/probe_delegate.sh`) fully specified the call — +*"delegate to agent \"anchors-amp-dev:architect\" with instruction …"* — so the model +emitted exactly those two arguments. That is the whole mechanism. Of the item's three +candidate explanations, **"the named-agent path may pass no `model_role`" is correct**; +"architect/builder resolve differently from explorer" and "the probe caused the root to +pass `provider_preferences` itself" are both **ruled out** — agent identity is never +consulted for routing here (`:1821-1822`), and an explicit `provider_preferences` pin +would have populated `provider_preferences` rather than left it `None`. + +### Why the agents' own declared roles did not save it + +The captured parent `session:config` +(`probes/h7n-knob-consistent-anthropic/raw/agent-prefs-armA.json`) shows +`anchors-amp-dev:architect` carrying `model_role: ["reasoning","general"]` and +**`n_prefs: 0`**; builder `["coding","general"]`, `n_prefs: 0`. So the agent frontmatter +*did* declare roles, and the one fallback that exists at `:1822` had nothing to read. + +**That declaration is honoured in a different repo, and this lane stops at the boundary +as instructed.** `amplifier-app-cli`'s `session_spawner.py:568-575` states it verbatim: + +> *"The routing hook (hooks-routing) writes provider_preferences into agent configs at +> session:start when resolving model_role declarations in agent frontmatter. +> Tool-delegate normally reads these and passes them as a function argument…"* + +So the design is two-layer — **(A)** caller-supplied `model_role` resolved parent-side by +tool-delegate, **(B)** agent-declared `model_role` resolved by hooks-routing at +session:start. Layer A is what the probe skipped. **Why layer B produced `n_prefs: 0` for +11 of 13 agents in that container lives in `amplifier-bundle-routing-matrix` / +`amplifier-app-cli` — reported here, not investigated** (SCOPE-OUT: "if the divergence +turns out to live in app-cli's session_spawner or in routing-matrix's hooks-routing, say +so and stop"). That is the one open thread this lane hands on. + +--- + +## 2. THE ECONOMY-MATRIX CASE — explained + +**The case:** opus-5 root, `economy` matrix → architect ran on `gpt-5.6-terra`, builder on +`gpt-5.6-luna`, while `economy` is described as declaring `claude-sonnet-*` / +`claude-haiku-*`. + +**It is ordered-candidate fallback, authored into `economy.yaml` itself.** Read at +`routing-matrix@0188a12`, `routing/economy.yaml`: + +| role | candidate #1 | candidate #2 | observed child | +|---|---|---|---| +| `reasoning` | `anthropic: claude-sonnet-*` | **`openai: gpt-?.?-terra*`** | **gpt-5.6-terra** ✓ #2 | +| `coding` | `anthropic: claude-haiku-*` | **`openai: gpt-?.?-luna*`** | **gpt-5.6-luna** ✓ #2 | + +Both children landed on **candidate #2 of their own role**, exactly. The advance from #1 +to #2 is explicit, documented behaviour in two places that mirror each other — +`amplifier_foundation/spawn_utils.py:867-884` ("A preference whose provider is present +but whose glob pattern fails to resolve … is NOT applied with the raw, unresolved +pattern … we advance to the next preference in the ordered list, mirroring +`resolve_model_role()`'s `continue` behavior") and routing-matrix's +`modules/hooks-routing/.../resolver.py:461`. + +**"Anthropic globs resolved to OpenAI models" is therefore a misreading of the symptom.** +The matrix's own author put OpenAI second in both roles; the Anthropic glob matched no +installed model, so the matrix's own next choice won. Reproduced as a pure function, $0, +in `TestOrderedCandidateFallback::test_unresolvable_first_candidate_advances_to_the_next` +(parametrised over both roles, asserting terra and luna by name). + +**Positively: this row is the one that proves per-role resolution DID run.** A session +default gives both children the *same* model; these two differ from each other **and** +from the root. Nothing but role-differentiated resolution produces that. + +**What I could NOT determine at $0, stated plainly.** *Why* the Anthropic candidate failed +to match in that container. Two mechanisms are consistent with it — the cell-pinning +harness restricting what the anthropic mount lists, and the instance-selection defect in +§4 — and separating them needs the container, which is gone, or a fresh run, which is not +authorised at $0 and which the SCOPE-OUTs forbid re-running. Recorded as open rather than +guessed. + +--- + +## 3. VERDICT ON EACH OF THE THREE REPRODUCTIONS + +Argued from the code, per the item's requirement. Verdict vocabulary is the item's own: +**(a)** child resolves to the matrix's declared model, or **(b)** divergence is +**INTENDED** with the reason. + +| # | reproduction | verdict | reason, from the code | +|---|---|---|---| +| 1 | opus root + `anthropic` → both children `claude-opus-5` (builder's `coding` should be `claude-sonnet-*`) | **(b) INTENDED** | No `model_role` in the tool call ⇒ guard at `:1636` false ⇒ resolver never consulted ⇒ documented session-default fall-through (`:1699-1712`). Corroborated independently by **effort**: `anthropic.yaml` `reasoning` carries `reasoning_effort: high`, and a promotion *does* carry a candidate's `config` (`spawn_utils.py:767-773`); the child's `session:config` shows the root's `xhigh`, so no promotion occurred. | +| 2 | sonnet root + `anthropic` → both children `claude-sonnet-5` (architect's `reasoning` should be `claude-opus-*`) | **(b) INTENDED** | Same mechanism, same guard. | +| 3 | opus root + `economy` → children `gpt-5.6-terra` / `gpt-5.6-luna` | **(b) INTENDED, and it is the matrix's own ordered fallback** | §2. Each child took candidate #2 of its own role, which `economy.yaml` authors as OpenAI. Per-role resolution ran here. Residual open: why candidate #1 did not match (§2). | + +None is verdict (a); none is a defect in the named-delegate path. + +--- + +## 4. THE DEFECT THIS CODE READ DID FIND (fixed here) + +`amplifier_foundation/spawn_utils.py` had **three** answers to "which mounted instance +does the bare module type `anthropic` mean?": + +| helper | rule | line | +|---|---|---| +| `_find_provider_instance` | **highest priority** (lowest number) | `:601-617` | +| `_find_provider_index` | **first declared** | `:636-645` | +| `_build_provider_lookup` | **last declared** (plain dict, last write wins) | `:660-673` | + +`apply_provider_preferences_with_resolution` calls **two of them in one pass**: it +resolves the candidate's model glob against the instance `_find_provider_instance` picks +(`:430`), then promotes the index `_build_provider_lookup` returns (`:859-888`). + +This matters exactly when a matrix is in play, because a matrix addresses providers by +bare module type while the mount plan carries several instances of that module — which is +the shape the routing-matrix bundle explicitly asks for (`_find_provider_instance`'s own +docstring, `:585`: distinct `id:`s exist *"for routing-matrix disambiguation"*). + +**Measured on the h7n roster** (10 mounts, 2 module types, cell forced to priority 0): + +``` +_build_provider_lookup["anthropic"] -> idx 7 = 'fable' prio 7 (promoted) +_find_provider_instance("anthropic") -> 'opus' prio 0 (model list read) +_build_provider_lookup["openai"] -> idx 9 = 'luna-max' prio 9 (promoted) +_find_provider_instance("openai") -> 'sol' prio 2 (model list read) +``` + +So the model resolved from one instance's list was written as `default_model` onto a +different instance, and *that* instance was promoted to priority 0 — carrying its own +`base_url`, long-context, and cache-retention settings. Silently: nothing logs a +mismatch, and the child looks correctly routed because the model name is right. + +**The fix (`spawn_utils.py`, +87/−16):** one rule — *highest priority wins, ties by +declaration order; an explicit instance `id` is more specific than a module type and +always wins* — applied to all three helpers via a shared `_provider_priority()`. +`_build_provider_lookup` becomes two passes (priority-aware type keys, then id keys +overwriting). The single-instance case, which is the overwhelming majority, is unchanged +and pinned by `test_single_instance_plans_are_unchanged`. + +**Behaviour change to flag for review:** a multi-instance plan that sets *no* `priority` +previously resolved a bare type to the **last** declared instance and now resolves to the +**first** — which is what `_find_provider_instance` already did, so this removes a +disagreement rather than inventing a rule. + +--- + +## 5. IS ANY SHIPPED ROUTING DECISION WRONG TODAY? + +**On the named-delegate question: no. Blast radius zero.** The organic path is what real +workloads use, h7n proved it healthy (matrix model *and* matrix effort, glob-resolved, +not the root's effort), and it is untouched here. The "bypass" only appears when a prompt +over-specifies a delegate call so the model omits `model_role` — a probe artefact, and +even then the outcome is the documented, opt-out fall-through, not a wrong decision. + +**On the §4 defect: yes, but narrowly.** It can only fire where **≥2 instances of one +provider module are mounted** *and* a matrix candidate addresses that module by bare type +*and* the instances differ in more than model. That is the eval-harness roster and any +multi-account/multi-endpoint setup; it is not the default single-instance install. When +it fires the model name still looks right, which is why it survived this long. + +**Not re-opened, as instructed:** the organic delegation path. h7n verified it; this lane +did not re-litigate it and produced no evidence against it. + +--- + +## 6. DELIVERABLES + +| deliverable | state | +|---|---| +| Mechanism named at file:line, both call sites side by side | **DONE** — §1 | +| Economy-matrix case explained | **DONE** — §2 (with the one residual explicitly recorded as open, and what was ruled out) | +| Verdict on each of the three reproductions | **DONE** — §3, all three (b) INTENDED, argued from code | +| A test pinning whichever answer is true | **DONE** — `tests/test_named_delegate_matrix_67u.py`, 15 tests: 4 characterization (intended behaviour), 8 fail-before defect tests, 3 economy-fallback reproductions | +| Say plainly whether any shipped routing decision is wrong today | **DONE** — §5 | +| Do NOT re-open the organic path | **HONOURED** | +| Full suite green | **DONE** — §7 | +| DRAFT PR on origin | **DONE** — see marker | +| DONE-NOTE at the lane artifact root | **this file** | + +--- + +## 7. SPEND, TESTS, DEVIATIONS + +**Spend: $0.00 against a $0 authority.** No API calls, no DTU launched, no infrastructure +registered, nothing to tear down. The authority stated `$0` for a code-read question with +reproductions already on disk; the arithmetic closes trivially because **nothing needed +buying** — the primary question was answerable from source, and every reproduction in +§2/§4 is a pure function called with no network. Residue: the full $0 authority, unspent; +the smallest useful purchase it could not buy is one DTU launch (~$2–5 at this batch's +observed rates), which was **not needed**. + +**Full suite** (`uv run pytest -q`, repo root): + +``` +baseline @ 18efe87 : 1924 passed, 1 skipped, 1 warning in 20.17s +with this change : 1939 passed, 1 skipped, 1 warning in 19.21s (+15, 0 regressions) +``` + +**Fail-before, verified by reverting only `spawn_utils.py`:** + +``` +7 failed, 8 passed # the 7 defect tests fail; characterization + fallback tests pass either way +AssertionError: assert 'fable' == 'opus' +``` + +`ruff check` and `ruff format --check` clean on both touched files. The single repo-wide +`ruff` error (`F401 ParsedURI` in `amplifier_foundation/updates/__init__.py`) is +**pre-existing on `18efe87`** — confirmed by stashing this lane's changes and re-running — +and was left alone. + +**Deviations and choices, recorded:** + +1. **Tests placed in `tests/`, not `modules/tool-delegate/tests/`.** The goal notes CI runs + `pytest tests/` only, excluding the module's own test dir. `pyproject.toml` sets + `pythonpath = ["modules/tool-delegate"]`, so `tests/` can import `DelegateTool` + directly — the characterization tests therefore actually run in CI. The known CI gap is + noted, not fixed (not this lane's). +2. **`_find_provider_index` aligned too**, although it has no production caller today + (`grep`: definition + tests only). Leaving a third disagreeing rule in the same file is + how this gets re-filed; six lines removed the trap. +3. **Did not cross into `amplifier-app-cli` or `amplifier-bundle-routing-matrix`.** Both + were read for evidence (quoted above) and neither was modified, per the SCOPE-OUT. +4. **Did not re-run the delegate probes.** Answered from source and from the captures, as + instructed. +5. **No fresh run was required**, so no priced authority is being requested. + +**Claim tags** (§5 rules-for-lanes): mechanism at `:1625`/`:1636`/`:1822` — *(knob: none · +family: n/a · confidence: **measured** — code read at `18efe87` · evidence: +`modules/tool-delegate/amplifier_module_tool_delegate/__init__.py:1625,1636,1819-1826`)*. +Economy fallback — *(knob: routing.matrix=economy · family: mixed anthropic/openai · +confidence: **measured** — pure-function reproduction, n=2 roles · evidence: +`tests/test_named_delegate_matrix_67u.py::TestOrderedCandidateFallback`)*. Instance +split-brain — *(knob: none · family: n/a · confidence: **measured** — fail-before test, +n=10-mount roster · evidence: `amplifier_foundation/spawn_utils.py:601-674`)*. Why layer B +produced `n_prefs: 0` in that container — *(confidence: **not determined**; out of repo +scope)*. diff --git a/tests/test_named_delegate_matrix_67u.py b/tests/test_named_delegate_matrix_67u.py new file mode 100644 index 0000000..d3a1ce0 --- /dev/null +++ b/tests/test_named_delegate_matrix_67u.py @@ -0,0 +1,489 @@ +"""Pins the answer to model_performance-67u. + +Two independent questions were tangled together in the original report +("no delegate lands on the model its matrix declares"). This file separates +them and pins each, so neither gets re-filed as folklore. + +QUESTION 1 -- does an EXPLICITLY-NAMED delegate bypass matrix resolution? + No. There is no separate "named-agent" spawn path. ``DelegateTool.execute`` + has exactly ONE resolver call site, guarded by + ``if raw_model_role and provider_preferences is None`` -- and + ``raw_model_role`` is read ONLY from the tool input. The agent's own + declared ``model_role`` is never consulted here. So a delegate call that + names an agent and supplies no ``model_role`` argument resolves nothing + and falls through to the session default, BY DESIGN (the fall-through is + documented in-module and is opt-out via ``strict_model_role``). + The "organic" path differs only in that the calling model put + ``model_role`` in the tool arguments. Same code, different arguments. + ``TestNamedDelegateIsCharacterisation`` pins this. These are + CHARACTERIZATION tests: they assert intended behaviour, not a defect. + +QUESTION 2 -- a real defect found while answering question 1. + When several instances of ONE provider module are mounted (exactly the + shape the routing matrix asks for, since a matrix addresses providers by + bare module type), this file's helpers disagreed about which instance + "anthropic" means: + + _find_provider_instance -> highest priority + _find_provider_index -> first declared + _build_provider_lookup -> last declared (dict last-write-wins) + + ``apply_provider_preferences_with_resolution`` calls two of them in one + pass: it resolves the model glob against the instance the FIRST picks and + then promotes the index the THIRD returns. Right model, wrong instance -- + and silently. ``TestProviderInstanceSelectionIsConsistent`` fails before + the fix and passes after. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock +from unittest.mock import MagicMock + +import pytest + +from amplifier_foundation.spawn_utils import ProviderPreference +from amplifier_foundation.spawn_utils import _build_provider_lookup +from amplifier_foundation.spawn_utils import _find_provider_index +from amplifier_foundation.spawn_utils import _find_provider_instance +from amplifier_foundation.spawn_utils import apply_provider_preferences_with_resolution + +# --------------------------------------------------------------------------- +# The roster that produced the original report. +# +# Verbatim shape of the eval harness's provider mounts (10 instances, 2 module +# types, distinct ``id``s, one forced to priority 0 to pin the cell). Model +# lists are trimmed to what the matrix globs in play actually need. +# --------------------------------------------------------------------------- + +H7N_ROSTER: list[dict[str, Any]] = [ + {"id": "sol", "module": "provider-openai", "config": {"priority": 2}}, + {"id": "terra", "module": "provider-openai", "config": {"priority": 3}}, + {"id": "opus-4.8", "module": "provider-anthropic", "config": {"priority": 1}}, + # The forced cell: highest priority (lowest number) of any anthropic mount. + {"id": "opus", "module": "provider-anthropic", "config": {"priority": 0}}, + {"id": "sonnet", "module": "provider-anthropic", "config": {"priority": 4}}, + {"id": "openai", "module": "provider-openai", "config": {"priority": 6}}, + {"id": "haiku", "module": "provider-anthropic", "config": {"priority": 5}}, + {"id": "fable", "module": "provider-anthropic", "config": {"priority": 7}}, + {"id": "luna", "module": "provider-openai", "config": {"priority": 8}}, + {"id": "luna-max", "module": "provider-openai", "config": {"priority": 9}}, +] + +ANTHROPIC_MODELS = ["claude-opus-5", "claude-sonnet-5", "claude-haiku-4-5"] +OPENAI_MODELS = ["gpt-5.6-sol", "gpt-5.6-terra", "gpt-5.6-luna", "gpt-5.6"] + + +def _index_of(providers: list[dict[str, Any]], instance_id: str) -> int: + return next(i for i, p in enumerate(providers) if p["id"] == instance_id) + + +class _FakeProvider: + """Provider double exposing only what pattern resolution touches.""" + + def __init__(self, models: list[str]) -> None: + self._models = list(models) + + async def list_models(self) -> list[str]: + return list(self._models) + + +def _make_coordinator( + providers: list[dict[str, Any]], + *, + models_by_module: dict[str, list[str]] | None = None, +) -> Any: + """Coordinator double whose runtime providers mirror ``providers``. + + Runtime providers are keyed by instance id (how a multi-instance mount + plan is actually keyed), and ``coordinator.config["providers"]`` carries + the mount-plan specs that :func:`_find_provider_instance` falls back to + when a bare module type matches no key directly. + """ + models_by_module = models_by_module or { + "provider-anthropic": ANTHROPIC_MODELS, + "provider-openai": OPENAI_MODELS, + } + # A distinct provider object per instance so the promoted instance can be + # told apart from the one whose model list was consulted. + runtime = { + p["id"]: _FakeProvider(models_by_module.get(p["module"], [])) for p in providers + } + coordinator = MagicMock() + coordinator.config = {"providers": providers} + coordinator.get = MagicMock( + side_effect=lambda key: runtime if key == "providers" else None + ) + return coordinator + + +# ============================================================================= +# QUESTION 2 -- the defect +# ============================================================================= + + +class TestProviderInstanceSelectionIsConsistent: + """One answer to "which instance does a bare module type mean?".""" + + def test_lookup_picks_highest_priority_instance_not_last_declared(self) -> None: + """``anthropic`` means the highest-priority anthropic mount. + + Fails before the fix: the plain-dict build made the LAST declared + anthropic mount (``fable``, priority 7) win over the forced cell + (``opus``, priority 0). + """ + lookup = _build_provider_lookup(H7N_ROSTER) + assert lookup["anthropic"] == _index_of(H7N_ROSTER, "opus") + assert lookup["provider-anthropic"] == _index_of(H7N_ROSTER, "opus") + + def test_lookup_agrees_with_find_provider_instance(self) -> None: + """The two helpers used in ONE pass must name the same instance. + + This is the split-brain itself: ``apply_provider_preferences_with_ + resolution`` resolves the glob against ``_find_provider_instance``'s + pick and promotes ``_build_provider_lookup``'s index. + """ + coordinator = _make_coordinator(H7N_ROSTER) + runtime = coordinator.get("providers") + + for bare in ("anthropic", "openai"): + promoted_idx = _build_provider_lookup(H7N_ROSTER)[bare] + promoted_instance = runtime[H7N_ROSTER[promoted_idx]["id"]] + consulted_instance = _find_provider_instance(runtime, bare, coordinator) + assert promoted_instance is consulted_instance, ( + f"bare type {bare!r}: model list read from one instance, " + f"promotion written to another" + ) + + def test_explicit_instance_id_beats_module_type_collision(self) -> None: + """An id that collides with a module-type name still addresses itself. + + The roster mounts ``provider-openai`` five times and names one of + them ``openai`` outright. ``openai`` must mean that instance. + """ + lookup = _build_provider_lookup(H7N_ROSTER) + assert lookup["openai"] == _index_of(H7N_ROSTER, "openai") + # ...and every other instance still addresses itself by id. + for provider in H7N_ROSTER: + assert lookup[provider["id"]] == _index_of(H7N_ROSTER, provider["id"]) + + def test_find_provider_index_agrees_with_lookup(self) -> None: + """The third helper answers the same way as the other two.""" + lookup = _build_provider_lookup(H7N_ROSTER) + for bare in ("anthropic", "openai", "provider-anthropic", "provider-openai"): + assert _find_provider_index(H7N_ROSTER, bare) == lookup[bare] + + def test_missing_priority_keeps_declaration_order(self) -> None: + """Plans that never set ``priority`` keep resolving first-declared.""" + providers = [ + {"id": "first", "module": "provider-anthropic", "config": {}}, + {"id": "second", "module": "provider-anthropic", "config": {}}, + ] + assert _build_provider_lookup(providers)["anthropic"] == 0 + assert _find_provider_index(providers, "anthropic") == 0 + + def test_unparseable_priority_does_not_raise(self) -> None: + """A junk ``priority`` sorts as 0 rather than exploding the spawn.""" + providers = [ + {"id": "junk", "module": "provider-anthropic", "config": {"priority": "x"}}, + {"id": "ten", "module": "provider-anthropic", "config": {"priority": 10}}, + ] + assert _build_provider_lookup(providers)["anthropic"] == 0 + + def test_single_instance_plans_are_unchanged(self) -> None: + """The common single-instance case keeps every key it always had.""" + providers = [ + {"module": "provider-anthropic", "config": {}}, + {"module": "provider-openai", "config": {}}, + ] + lookup = _build_provider_lookup(providers) + assert lookup["anthropic"] == 0 + assert lookup["provider-anthropic"] == 0 + assert lookup["openai"] == 1 + assert lookup["provider-openai"] == 1 + + @pytest.mark.asyncio + async def test_promotion_lands_on_the_instance_that_resolved_the_glob( + self, + ) -> None: + """End-to-end: the promoted mount is the forced cell, not ``fable``.""" + coordinator = _make_coordinator(H7N_ROSTER) + plan = {"providers": H7N_ROSTER} + + new_plan = await apply_provider_preferences_with_resolution( + plan, + [ + ProviderPreference( + provider="anthropic", + model="claude-opus-*", + config={"reasoning_effort": "high"}, + ) + ], + coordinator, + ) + + promoted = [ + p for p in new_plan["providers"] if p["config"].get("priority") == 0 + ] + assert len(promoted) == 1 + assert promoted[0]["id"] == "opus" + assert promoted[0]["config"]["default_model"] == "claude-opus-5" + # The matrix candidate's own config rides along with the promotion -- + # this is how a matrix effort reaches a delegate. + assert promoted[0]["config"]["reasoning_effort"] == "high" + + +# ============================================================================= +# The economy-matrix case -- ordered fallback, not a provider mix-up +# ============================================================================= + + +class TestOrderedCandidateFallback: + """Why Anthropic-glob roles landed on OpenAI models under ``economy``. + + ``economy.yaml`` lists an Anthropic candidate FIRST and an OpenAI + candidate SECOND for both ``reasoning`` (``claude-sonnet-*`` then + ``gpt-?.?-terra*``) and ``coding`` (``claude-haiku-*`` then + ``gpt-?.?-luna*``). When the Anthropic glob resolves against no + installed model, resolution advances to the next candidate -- which the + matrix author wrote as OpenAI. The observed + architect->``gpt-5.6-terra`` / builder->``gpt-5.6-luna`` split is that + ordered fallback, per role. It is not a provider mix-up, and a session + default cannot produce it (a session default gives both children the + SAME model). + """ + + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("anthropic_glob", "openai_glob", "expected"), + [ + ("claude-sonnet-*", "gpt-?.?-terra*", "gpt-5.6-terra"), # reasoning + ("claude-haiku-*", "gpt-?.?-luna*", "gpt-5.6-luna"), # coding + ], + ) + async def test_unresolvable_first_candidate_advances_to_the_next( + self, anthropic_glob: str, openai_glob: str, expected: str + ) -> None: + providers = [ + {"id": "opus", "module": "provider-anthropic", "config": {"priority": 0}}, + {"id": "terra", "module": "provider-openai", "config": {"priority": 3}}, + ] + # The cell pins the anthropic mount to a single model, so neither + # claude-sonnet-* nor claude-haiku-* has anything to match. + coordinator = _make_coordinator( + providers, + models_by_module={ + "provider-anthropic": ["claude-opus-5"], + "provider-openai": OPENAI_MODELS, + }, + ) + + new_plan = await apply_provider_preferences_with_resolution( + {"providers": providers}, + [ + ProviderPreference(provider="anthropic", model=anthropic_glob), + ProviderPreference(provider="openai", model=openai_glob), + ], + coordinator, + ) + + promoted = [ + p for p in new_plan["providers"] if p["config"].get("priority") == 0 + ] + assert len(promoted) == 1 + assert promoted[0]["id"] == "terra" + assert promoted[0]["config"]["default_model"] == expected + + @pytest.mark.asyncio + async def test_no_candidate_resolves_leaves_the_plan_untouched(self) -> None: + """The documented silent-substitution path: session default is kept. + + Nothing is promoted and no unresolved glob is written into the plan, + so the child simply runs whatever the session already ranked first. + """ + providers = [ + {"id": "opus", "module": "provider-anthropic", "config": {"priority": 0}}, + ] + coordinator = _make_coordinator( + providers, models_by_module={"provider-anthropic": ["claude-opus-5"]} + ) + plan = {"providers": providers} + + new_plan = await apply_provider_preferences_with_resolution( + plan, + [ProviderPreference(provider="anthropic", model="claude-sonnet-*")], + coordinator, + ) + + assert new_plan == plan + assert "default_model" not in new_plan["providers"][0]["config"] + + +# ============================================================================= +# QUESTION 1 -- characterization. Intended behaviour; do not "fix" these. +# ============================================================================= + + +def _make_delegate_tool( + *, + spawn_fn: Any, + agents: dict | None = None, + model_role_resolver: Any = None, +) -> Any: + from amplifier_module_tool_delegate import DelegateTool + + coordinator = MagicMock() + coordinator.session_id = "parent-session-67u" + coordinator.config = {"agents": agents or {}} + coordinator.session_state = {} + + capabilities = { + "session.spawn": spawn_fn, + "session.resume": AsyncMock(return_value={}), + "agents.list": lambda: agents or {}, + "agents.get": lambda name: (agents or {}).get(name), + "self_delegation_depth": 0, + "model_role_resolver": model_role_resolver, + } + coordinator.get_capability = lambda name: capabilities.get(name) + coordinator.get = MagicMock(return_value=None) # hooks = None + + parent_session = MagicMock() + parent_session.session_id = "parent-session-67u" + parent_session.config = {"session": {"orchestrator": {}}} + coordinator.session = parent_session + + return DelegateTool( + coordinator, {"features": {}, "settings": {"exclude_tools": []}} + ) + + +def _spawn_double() -> AsyncMock: + return AsyncMock( + return_value={ + "output": "done", + "session_id": "child-67u", + "status": "success", + "turn_count": 1, + "metadata": {}, + } + ) + + +def _resolver_double() -> Any: + resolver = MagicMock() + resolver.name = "matrix-double" + resolver.resolve = AsyncMock( + return_value=[ProviderPreference(provider="anthropic", model="claude-opus-5")] + ) + return resolver + + +# The agent frontmatter shape the original probe delegated to, verbatim from +# the captured parent ``session:config``: a declared model_role list, and -- +# importantly -- zero resolved provider_preferences. +ARCHITECT_AGENT = { + "anchors-amp-dev:architect": { + "model_role": ["reasoning", "general"], + "provider_preferences": [], + } +} + + +class TestNamedDelegateIsCharacterisation: + """Naming an agent does not bypass anything; it just supplies no role.""" + + @pytest.mark.asyncio + async def test_no_model_role_argument_never_consults_the_resolver(self) -> None: + """The guard at the single resolver call site is what "bypass" means.""" + spawn_fn = _spawn_double() + resolver = _resolver_double() + tool = _make_delegate_tool( + spawn_fn=spawn_fn, + agents=ARCHITECT_AGENT, + model_role_resolver=resolver, + ) + + result = await tool.execute( + {"agent": "anchors-amp-dev:architect", "instruction": "Reply A-OK."} + ) + + assert result.success + resolver.resolve.assert_not_awaited() + assert spawn_fn.await_args.kwargs["provider_preferences"] is None + + @pytest.mark.asyncio + async def test_agent_declared_model_role_is_inert_in_tool_delegate(self) -> None: + """``agents[name]["model_role"]`` is never read on the spawn path. + + The agent above declares ``model_role: [reasoning, general]``. That + declaration is resolved elsewhere (the routing hook writes resolved + preferences into agent configs at session:start) -- NOT here. This + test pins the boundary so the next reader does not go looking for a + missing lookup in this module. + """ + spawn_fn = _spawn_double() + resolver = _resolver_double() + tool = _make_delegate_tool( + spawn_fn=spawn_fn, + agents=ARCHITECT_AGENT, + model_role_resolver=resolver, + ) + + await tool.execute( + {"agent": "anchors-amp-dev:architect", "instruction": "Reply A-OK."} + ) + + resolver.resolve.assert_not_awaited() + assert spawn_fn.await_args.kwargs["provider_preferences"] is None + + @pytest.mark.asyncio + async def test_agent_level_provider_preferences_are_the_one_fallback(self) -> None: + """The only agent-level routing input the spawn path reads.""" + spawn_fn = _spawn_double() + tool = _make_delegate_tool( + spawn_fn=spawn_fn, + agents={ + "anchors-amp-dev:architect": { + "model_role": ["reasoning", "general"], + "provider_preferences": [ + {"provider": "anthropic", "model": "claude-opus-*"} + ], + } + }, + model_role_resolver=_resolver_double(), + ) + + await tool.execute( + {"agent": "anchors-amp-dev:architect", "instruction": "Reply A-OK."} + ) + + prefs = spawn_fn.await_args.kwargs["provider_preferences"] + assert prefs is not None + assert [p.model for p in prefs] == ["claude-opus-*"] + + @pytest.mark.asyncio + async def test_same_call_site_resolves_when_the_caller_supplies_the_role( + self, + ) -> None: + """The "organic" path: identical code, one extra tool argument.""" + spawn_fn = _spawn_double() + resolver = _resolver_double() + tool = _make_delegate_tool( + spawn_fn=spawn_fn, + agents=ARCHITECT_AGENT, + model_role_resolver=resolver, + ) + + await tool.execute( + { + "agent": "anchors-amp-dev:architect", + "instruction": "Reply A-OK.", + "model_role": "reasoning", + } + ) + + resolver.resolve.assert_awaited_once_with("reasoning") + prefs = spawn_fn.await_args.kwargs["provider_preferences"] + assert [p.model for p in prefs] == ["claude-opus-5"]