From b469ced2ef046b24ccd26c66d5144ebbd1be1d79 Mon Sep 17 00:00:00 2001 From: Amplifier Lane adq Date: Wed, 2 Sep 2026 17:23:02 -0700 Subject: [PATCH 1/3] fix(routing): mark a matrix that shadows a same-named matrix in `routing list`/`show` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `amplifier routing list` showed a user matrix and a bundle matrix of the same name as peers. Only one is ever loaded: hooks-routing's mount() searches `[*custom_routing_dirs, bundle routing/]` and takes the first hit, so a file in ~/.amplifier/routing/ silently makes the shipped bundle matrix dead -- and nothing in the CLI said so. That is why a matrix change shipped in the bundle can be completely inert on a host. The precedence rule is NOT re-derived here. It is consumed from hooks-routing's own `resolve_matrix_source()` (routing-matrix PR #52), loaded by file path out of the same cached bundle directory the CLI already globs -- app-cli does not depend on hooks-routing as a distribution, and `routing list` never mounts a bundle, so neither an import nor the session-time `model_role_resolver` capability is reachable from this process. When the cached bundle predates PR #52 (no `resolve_matrix_source`), the CLI draws no marker at all rather than guessing the search order: a wrong shadowing claim is worse than none. Unshadowed output is byte-identical to before, in both text and JSON. - amplifier_app_cli/lib/routing_provenance.py: locate + load the bundle's matrix_loader, classify custom vs bundle routing dirs, resolve one MatrixSource per matrix name. - commands/routing.py: row marker (`⚠ shadows bundle`), a footer naming the file in use and each file it suppresses, the same note on `routing show`, and MatrixSource.to_dict() in `--format json`. - tests/test_routing_shadowing.py: 15 tests -- shadowed marks the winner, unshadowed output byte-identical, no user routing dir, and the old-bundle degradation path. --- amplifier_app_cli/commands/routing.py | 157 ++++++- amplifier_app_cli/lib/routing_provenance.py | 220 +++++++++ tests/test_routing_shadowing.py | 477 ++++++++++++++++++++ 3 files changed, 832 insertions(+), 22 deletions(-) create mode 100644 amplifier_app_cli/lib/routing_provenance.py create mode 100644 tests/test_routing_shadowing.py diff --git a/amplifier_app_cli/commands/routing.py b/amplifier_app_cli/commands/routing.py index b5f991d5..4aa759c7 100644 --- a/amplifier_app_cli/commands/routing.py +++ b/amplifier_app_cli/commands/routing.py @@ -14,6 +14,7 @@ from rich.table import Table from ..lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES +from ..lib.routing_provenance import resolve_matrix_origins from ..lib.settings import AppSettings, Scope, get_custom_routing_dir from ..provider_loader import get_provider_info, get_provider_models from ..provider_manager import resolve_provider_entry @@ -150,16 +151,82 @@ def _load_matrix(path: Path) -> dict[str, Any] | None: return None -def _load_all_matrices(matrix_files: list[Path]) -> dict[str, dict[str, Any]]: - """Load all matrix files into a name -> data dict.""" - matrices: dict[str, dict[str, Any]] = {} +def _load_all_matrices_with_paths( + matrix_files: list[Path], +) -> dict[str, tuple[dict[str, Any], Path]]: + """Load all matrix files into a name -> (data, source_path) dict. + + Same last-write-wins keying as :func:`_load_all_matrices`, but it also + keeps the file each listed entry actually came from, which is what + shadowing provenance is looked up by (hooks-routing resolves matrices by + file *stem*, while this dict is keyed by the ``name:`` field inside the + YAML -- the two usually agree but are not the same thing). + """ + matrices: dict[str, tuple[dict[str, Any], Path]] = {} for path in matrix_files: data = _load_matrix(path) if data and "name" in data: - matrices[data["name"]] = data + matrices[data["name"]] = (data, path) return matrices +def _load_all_matrices(matrix_files: list[Path]) -> dict[str, dict[str, Any]]: + """Load all matrix files into a name -> data dict.""" + return { + name: data + for name, (data, _) in _load_all_matrices_with_paths(matrix_files).items() + } + + +def _display_path(path: Path) -> str: + """Render a path with ``~`` for the home directory, for readable output.""" + try: + return f"~/{path.relative_to(Path.home())}" + except ValueError: + return str(path) + + +def _shadow_label(origin: Any) -> str: + """Short marker text naming what kind of file this matrix suppresses.""" + sources = sorted({src for _, src in origin.shadowed}) + return "/".join(sources) if sources else "matrix" + + +def _print_shadowing_footer(shadowed: list[tuple[str, Any]]) -> None: + """Print the shadowing relationships: winner path, then what it suppresses. + + Only called when at least one matrix is shadowed, so an unshadowed host + sees exactly the output it saw before. + """ + if not shadowed: + return + + count = len(shadowed) + noun = "matrix is" if count == 1 else "matrices are" + console.print( + f"[yellow]⚠ {count} {noun} shadowed — only the 'in use' file is loaded:[/yellow]" + ) + for name, origin in shadowed: + console.print(f" [bold]{name}[/bold]") + _print_origin_paths(origin) + console.print() + + +def _print_origin_paths(origin: Any) -> None: + """Print the winning path, then each path it suppresses, one per line. + + ``soft_wrap`` keeps Rich from reflowing a long absolute path into the + middle of the label column. + """ + winner = _display_path(origin.path) if origin.path else "(none)" + console.print(f" [green]in use[/green] {winner}", soft_wrap=True) + for path, source in origin.shadowed: + console.print( + f" [red]suppressed[/red] {_display_path(path)} [dim]({source})[/dim]", + soft_wrap=True, + ) + + def _get_configured_provider_types(settings: AppSettings) -> set[str]: """Get the set of configured provider identifiers a matrix candidate may reference. @@ -251,11 +318,17 @@ def routing_list(compact: bool, detailed: bool, fmt: str): ) return - matrices = _load_all_matrices(matrix_files) - if not matrices: + loaded = _load_all_matrices_with_paths(matrix_files) + if not loaded: console.print("[yellow]No valid routing matrices found.[/yellow]") return + # Shadowing provenance, derived from hooks-routing's own + # resolve_matrix_source() -- never from a search order re-derived here. + # An empty dict means "provenance unknown" (bundle too old / unreachable), + # NOT "nothing is shadowed", so no marker is drawn in that case. + origins = resolve_matrix_origins(matrix_files) + routing_config = settings.get_routing_config() active_matrix = routing_config.get("matrix", "balanced") provider_types = _get_configured_provider_types(settings) @@ -265,7 +338,8 @@ def routing_list(compact: bool, detailed: bool, fmt: str): renderer = ItemRenderer(console) items: list[dict[str, Any]] = [] - for name, data in sorted(matrices.items()): + shadowed: list[tuple[str, Any]] = [] + for name, (data, source_path) in sorted(loaded.items()): is_active = name == active_matrix description = data.get("description", "") updated = str(data.get("updated", "")) @@ -275,18 +349,32 @@ def routing_list(compact: bool, detailed: bool, fmt: str): covered, total = _check_compatibility(data, provider_types) compat_str = f"{covered}/{total} roles" - items.append( - { - "name": ("→ " if is_active else " ") + name, - "enabled": is_active, - "behaviors": ["active" if is_active else "available"], - "config_summary": { - "description": description, - "compatibility": compat_str, - "updated": updated, - }, - } - ) + origin = origins.get(source_path.stem) + is_shadowing = origin is not None and origin.is_shadowed + + config_summary: dict[str, Any] = { + "description": description, + "compatibility": compat_str, + "updated": updated, + } + item: dict[str, Any] = { + "name": ("→ " if is_active else " ") + + name + + (f" ⚠ shadows {_shadow_label(origin)}" if is_shadowing else ""), + "enabled": is_active, + "behaviors": ["active" if is_active else "available"], + "config_summary": config_summary, + } + + if is_shadowing: + shadowed.append((name, origin)) + config_summary["shadows"] = ", ".join( + _display_path(p) for p, _ in origin.shadowed + ) + # Provenance payload, verbatim from hooks-routing's MatrixSource. + item["routing_source"] = origin.to_dict() + + items.append(item) if fmt == "json": renderer.render_json(items) @@ -295,6 +383,7 @@ def routing_list(compact: bool, detailed: bool, fmt: str): renderer.render( items, view=view, category="routing", section_title="routing matrices" ) + _print_shadowing_footer(shadowed) # ============================================================ @@ -349,7 +438,8 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st """ settings = _get_settings() matrix_files = _discover_matrix_files() - matrices = _load_all_matrices(matrix_files) + loaded = _load_all_matrices_with_paths(matrix_files) + matrices = {name: data for name, (data, _) in loaded.items()} if not matrices: console.print("[yellow]No routing matrices found.[/yellow]") @@ -370,13 +460,19 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st view = resolve_view( ("routing", "show"), compact_flag=compact, detailed_flag=detailed ) - matrix_data = matrices[matrix_name] + matrix_data, source_path = loaded[matrix_name] + origin = resolve_matrix_origins(matrix_files).get(source_path.stem) if fmt == "json": renderer = ItemRenderer(console) - renderer.render_json({"matrix": matrix_name, "data": matrix_data}) + payload: dict[str, Any] = {"matrix": matrix_name, "data": matrix_data} + if origin is not None and origin.is_shadowed: + payload["routing_source"] = origin.to_dict() + renderer.render_json(payload) return + _print_shadowing_note(origin) + # detailed view → full waterfall; regular/compact → resolved routing if view == "detailed": _show_matrix_details(matrix_data, settings) @@ -384,6 +480,23 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st _show_matrix_resolution(matrix_data, settings) +def _print_shadowing_note(origin: Any) -> None: + """Name the file in use and what it suppresses, for one matrix. + + Prints nothing at all when the matrix is not shadowed (or when provenance + is unavailable), so unshadowed output is unchanged. + """ + if origin is None or not origin.is_shadowed: + return + + console.print( + f"\n[yellow]⚠ '{origin.name}' shadows a {_shadow_label(origin)} matrix " + f"of the same name — only the 'in use' file is loaded:[/yellow]" + ) + _print_origin_paths(origin) + console.print() + + def _show_matrix_resolution(matrix_data: dict[str, Any], settings: AppSettings) -> None: """Display a role-by-role resolution table for a matrix.""" matrix_name = matrix_data.get("name", "unknown") diff --git a/amplifier_app_cli/lib/routing_provenance.py b/amplifier_app_cli/lib/routing_provenance.py new file mode 100644 index 00000000..281b032e --- /dev/null +++ b/amplifier_app_cli/lib/routing_provenance.py @@ -0,0 +1,220 @@ +"""Routing-matrix provenance: which matrix file wins, and what it shadows. + +``amplifier routing list`` used to show a user matrix and a same-named bundle +matrix as peers. At load time only one of them is ever read: hooks-routing's +``mount()`` searches ``[*custom_routing_dirs, bundle routing/]`` and takes the +first hit, so a same-named file in ``~/.amplifier/routing/`` silently makes the +shipped bundle matrix dead. Every matrix change shipped in the bundle is inert +on such a host and nothing in the CLI said so. + +**The precedence rule is not re-implemented here.** It lives in exactly one +place -- ``resolve_matrix_source()`` in the routing-matrix bundle's +``amplifier_module_hooks_routing/matrix_loader.py`` (routing-matrix PR #52). +This module's entire job is to reach that function from a CLI process that +never starts a session, and to degrade visibly-by-omission when it cannot. + +Why a dynamic load rather than an ``import``: + hooks-routing is a *bundle module*. It is not a distribution app-cli + depends on, it is not on ``sys.path``, and ``amplifier routing list`` never + mounts a bundle -- so the ``model_role_resolver`` capability that publishes + ``matrix_path`` / ``matrix_source`` / ``shadowed_paths`` at session start + does not exist in this process either. The bundle *is* on disk, in the + same cache directory ``_discover_matrix_files()`` already globs, so + ``matrix_loader.py`` is loaded from there by file path. + +When the cached bundle predates PR #52 (no ``resolve_matrix_source``), or the +load fails for any reason, :func:`resolve_matrix_origins` returns ``{}`` and +the listing is byte-identical to what it was before. We never guess the +precedence rule ourselves -- a wrong shadowing marker is worse than none. +""" + +from __future__ import annotations + +import importlib.util +import logging +import sys +from collections.abc import Callable, Sequence +from pathlib import Path +from typing import Any + +logger = logging.getLogger(__name__) + +# Cache directory name prefix for the routing-matrix bundle, as written by the +# bundle loader (see lib/bundle_loader/discovery.py WELL_KNOWN_BUNDLES). +BUNDLE_CACHE_PREFIX = "amplifier-bundle-routing-matrix" + +# Where hooks-routing's loader lives, relative to the bundle root. +_MATRIX_LOADER_RELPATH = ( + Path("modules") + / "hooks-routing" + / "amplifier_module_hooks_routing" + / "matrix_loader.py" +) + +# Loaded-module cache keyed by the resolved matrix_loader.py path, so a listing +# that resolves many matrix names pays the file load exactly once. +_loader_cache: dict[str, Any] = {} + + +def _key(path: Path) -> Path: + """Best-effort canonical form for identity comparison.""" + try: + return path.resolve() + except OSError: # pragma: no cover - exotic filesystem states + return path + + +def is_bundle_routing_dir(routing_dir: Path) -> bool: + """True when *routing_dir* is a ``routing/`` dir inside a routing-matrix bundle. + + Two signals, either sufficient: the cache-directory naming convention, or + the presence of the hooks-routing module beside it (which covers a bundle + checked out somewhere other than the cache, e.g. a dev worktree). + """ + parent = routing_dir.parent + if parent.name.startswith(BUNDLE_CACHE_PREFIX): + return True + return (parent / _MATRIX_LOADER_RELPATH).exists() + + +def classify_routing_dirs( + matrix_files: Sequence[Path], +) -> tuple[list[Path], list[Path]]: + """Split the dirs *matrix_files* came from into ``(custom_dirs, bundle_dirs)``. + + This classifies *where a directory lives*; it does not decide precedence -- + that stays entirely inside ``resolve_matrix_source``, which also owns the + aliasing rules (a "custom" dir that really is the bundle dir is labelled + ``bundle``, and a file reached twice is counted once). + + Order is first-seen, deduplicated by resolved path. + """ + custom_dirs: list[Path] = [] + bundle_dirs: list[Path] = [] + seen: set[Path] = set() + + for file_path in matrix_files: + parent = file_path.parent + parent_key = _key(parent) + if parent_key in seen: + continue + seen.add(parent_key) + if is_bundle_routing_dir(parent): + bundle_dirs.append(parent) + else: + custom_dirs.append(parent) + + return custom_dirs, bundle_dirs + + +def load_resolve_matrix_source( + bundle_dirs: Sequence[Path], +) -> Callable[..., Any] | None: + """Load hooks-routing's ``resolve_matrix_source`` from a cached bundle. + + Returns ``None`` -- never a fallback implementation -- when no cached + routing-matrix bundle carries the function (e.g. a bundle older than + routing-matrix PR #52), or when the module cannot be loaded. + """ + for bundle_dir in bundle_dirs: + loader_path = bundle_dir.parent / _MATRIX_LOADER_RELPATH + if not loader_path.exists(): + continue + + cache_key = str(_key(loader_path)) + if cache_key in _loader_cache: + module = _loader_cache[cache_key] + else: + module = _load_module(loader_path) + _loader_cache[cache_key] = module + + if module is None: + continue + + fn = getattr(module, "resolve_matrix_source", None) + if callable(fn): + return fn + + logger.debug( + "routing-matrix bundle at %s has no resolve_matrix_source " + "(bundle predates PR #52); shadowing will not be marked", + bundle_dir.parent, + ) + + return None + + +def _load_module(loader_path: Path) -> Any | None: + """Import ``matrix_loader.py`` by file path, under a synthetic module name. + + A synthetic top-level name is safe because ``matrix_loader`` has no + module-level relative imports (its one ``from .resolver import ...`` is + inside a function body). Should that ever change, the load raises and we + degrade to "no markers" rather than reporting a guess. + + The module is registered in ``sys.modules`` *before* execution because + ``matrix_loader`` defines a ``@dataclass`` (``MatrixSource``), and + ``dataclasses`` looks its own module up by name while building the class. + Without the registration the decorator fails with an opaque + ``'NoneType' object has no attribute '__dict__'``. + """ + module_name = f"_amplifier_cli_routing_matrix_loader_{abs(hash(str(loader_path)))}" + try: + spec = importlib.util.spec_from_file_location(module_name, loader_path) + if spec is None or spec.loader is None: + return None + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except Exception: + sys.modules.pop(module_name, None) + raise + return module + except Exception as e: # pragma: no cover - defensive + logger.debug("Could not load routing matrix_loader from %s: %s", loader_path, e) + return None + + +def resolve_matrix_origins(matrix_files: Sequence[Path]) -> dict[str, Any]: + """Map matrix file stem -> ``MatrixSource`` for every discovered matrix. + + The returned objects are hooks-routing's own ``MatrixSource`` dataclass: + ``.path`` (the winner), ``.source`` (``"user"`` / ``"bundle"``), + ``.shadowed`` (``(path, source)`` for every same-named file that lost), + ``.is_shadowed``, and ``.to_dict()``. + + Returns ``{}`` when ``resolve_matrix_source`` is unreachable -- callers + must treat an absent entry as "no provenance known", never as "unshadowed". + """ + custom_dirs, bundle_dirs = classify_routing_dirs(matrix_files) + resolve = load_resolve_matrix_source(bundle_dirs) + if resolve is None or not bundle_dirs: + return {} + + stems: list[str] = [] + seen_stems: set[str] = set() + for file_path in matrix_files: + if file_path.stem not in seen_stems: + seen_stems.add(file_path.stem) + stems.append(file_path.stem) + + origins: dict[str, Any] = {} + for stem in stems: + best: Any | None = None + # A host normally has exactly one cached routing-matrix bundle. If it + # has several, the CLI cannot know which one a session would mount, so + # report the outcome that shows the most suppression -- the claim + # "a same-named bundle matrix exists and loses" is true either way. + for bundle_dir in bundle_dirs: + try: + origin = resolve(stem, custom_dirs, bundle_dir) + except Exception as e: # pragma: no cover - defensive + logger.debug("resolve_matrix_source failed for %r: %s", stem, e) + continue + if best is None or len(origin.shadowed) > len(best.shadowed): + best = origin + if best is not None: + origins[stem] = best + + return origins diff --git a/tests/test_routing_shadowing.py b/tests/test_routing_shadowing.py new file mode 100644 index 00000000..9da041fd --- /dev/null +++ b/tests/test_routing_shadowing.py @@ -0,0 +1,477 @@ +"""`amplifier routing list` / `show` mark a matrix that shadows another. + +Background. hooks-routing's ``mount()`` searches +``[*custom_routing_dirs, bundle routing/]`` and loads the FIRST ``.yaml`` +it finds, so a same-named file in ``~/.amplifier/routing/`` silently makes the +shipped bundle matrix dead. The CLI used to list both as peers with no +indication that one suppressed the other. + +What these tests pin: + +* a shadowed setup marks the suppressed matrix AND names the file that wins; +* an unshadowed setup's output is byte-identical to the pre-change output; +* the command still works with no user routing dir at all; +* provenance comes from hooks-routing's ``resolve_matrix_source`` -- when that + function is unreachable (bundle older than routing-matrix PR #52) the CLI + draws no marker rather than guessing the precedence rule itself. + +About ``_STAND_IN_MATRIX_LOADER`` below: hooks-routing is a *bundle* module, +not a distribution this repo depends on, so there is nothing to import in a +test environment. The stand-in reproduces the contract of +``amplifier_module_hooks_routing.matrix_loader`` as of routing-matrix +``d17d03c`` (PR #52) so the CLI's own consumption path -- locate the bundle, +load the module by file path, call the function, render what it returns -- is +exercised end to end. Production code never uses this file; it loads the real +module out of the cached bundle. +""" + +from pathlib import Path +from unittest.mock import patch + +import yaml +from click.testing import CliRunner + +from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths + +# --------------------------------------------------------------------------- +# Stand-in for the bundle's matrix_loader.py (contract: routing-matrix d17d03c) +# --------------------------------------------------------------------------- + +_STAND_IN_MATRIX_LOADER = '''\ +"""Stand-in for amplifier_module_hooks_routing.matrix_loader (test fixture).""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +USER_SOURCE = "user" +BUNDLE_SOURCE = "bundle" + + +@dataclass(frozen=True) +class MatrixSource: + name: str + path: Path | None = None + source: str | None = None + shadowed: tuple[tuple[Path, str], ...] = () + searched: tuple[Path, ...] = field(default=()) + + @property + def is_shadowed(self) -> bool: + return bool(self.shadowed) + + def to_dict(self) -> dict[str, Any]: + return { + "matrix_name": self.name, + "matrix_path": str(self.path) if self.path is not None else None, + "matrix_source": self.source, + "matrix_shadowed": self.is_shadowed, + "shadowed_paths": [str(p) for p, _ in self.shadowed], + } + + +def resolve_matrix_source( + name: str, + custom_routing_dirs: Sequence[Path], + bundle_routing_dir: Path, +) -> MatrixSource: + filename = f"{name}.yaml" + + def _key(path: Path) -> Path: + try: + return path.resolve() + except OSError: + return path + + bundle_key = _key(bundle_routing_dir) + candidates: list[tuple[Path, str]] = [ + ( + Path(d) / filename, + BUNDLE_SOURCE if _key(Path(d)) == bundle_key else USER_SOURCE, + ) + for d in custom_routing_dirs + ] + candidates.append((bundle_routing_dir / filename, BUNDLE_SOURCE)) + + searched: list[Path] = [] + present: list[tuple[Path, str]] = [] + seen: set[Path] = set() + for candidate, origin in candidates: + candidate_key = _key(candidate) + if candidate_key in seen: + continue + seen.add(candidate_key) + searched.append(candidate) + if candidate.exists(): + present.append((candidate, origin)) + + if not present: + return MatrixSource(name=name, searched=tuple(searched)) + + winner, winner_source = present[0] + return MatrixSource( + name=name, + path=winner, + source=winner_source, + shadowed=tuple(present[1:]), + searched=tuple(searched), + ) +''' + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +def _matrix(name: str, description: str) -> dict: + return { + "name": name, + "description": description, + "updated": "2026-09-02", + "roles": { + "general": { + "description": "Catch-all", + "candidates": [{"provider": "anthropic", "model": "claude-sonnet-*"}], + }, + "fast": { + "description": "Quick tasks", + "candidates": [{"provider": "anthropic", "model": "claude-haiku-*"}], + }, + }, + } + + +def _make_bundle(tmp_path: Path, *, with_loader: bool = True) -> Path: + """Create a cached routing-matrix bundle tree. Returns its routing/ dir.""" + bundle_root = ( + tmp_path / ".amplifier" / "cache" / "amplifier-bundle-routing-matrix-test" + ) + routing_dir = bundle_root / "routing" + routing_dir.mkdir(parents=True) + (routing_dir / "openai.yaml").write_text( + yaml.dump(_matrix("openai", "Shipped OpenAI routing.")) + ) + (routing_dir / "balanced.yaml").write_text( + yaml.dump(_matrix("balanced", "Shipped balanced routing.")) + ) + + if with_loader: + pkg = bundle_root / "modules" / "hooks-routing" + pkg = pkg / "amplifier_module_hooks_routing" + pkg.mkdir(parents=True) + (pkg / "matrix_loader.py").write_text(_STAND_IN_MATRIX_LOADER) + + return routing_dir + + +def _make_user_dir(tmp_path: Path, names: list[str]) -> Path: + user_dir = tmp_path / ".amplifier" / "routing" + user_dir.mkdir(parents=True, exist_ok=True) + for name in names: + (user_dir / f"{name}.yaml").write_text( + yaml.dump(_matrix(name, f"Custom matrix: {name}")) + ) + return user_dir + + +def _make_settings(tmp_path: Path) -> AppSettings: + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + settings = AppSettings(paths=paths) + scope_settings = settings._read_scope("global") + scope_settings["config"] = { + "providers": [{"module": "provider-anthropic", "config": {"priority": 1}}] + } + settings._write_scope("global", scope_settings) + return settings + + +def _discovered(tmp_path: Path) -> list[Path]: + """Everything _discover_matrix_files() would return, same sort order.""" + amp = tmp_path / ".amplifier" + files = list(amp.glob("cache/*/routing/*.yaml")) + list(amp.glob("routing/*.yaml")) + return sorted(files) + + +def _invoke(tmp_path: Path, args: list[str], *, provenance: bool = True): + """Run a routing subcommand against the tmp tree.""" + from amplifier_app_cli.commands import routing as routing_mod + + settings = _make_settings(tmp_path) + patches = [ + patch.object(routing_mod, "_get_settings", return_value=settings), + patch.object( + routing_mod, "_discover_matrix_files", return_value=_discovered(tmp_path) + ), + patch.object(routing_mod.Path, "home", return_value=tmp_path), + ] + if not provenance: + # Simulates "provenance unreachable" -- the pre-change code path. + patches.append( + patch.object(routing_mod, "resolve_matrix_origins", return_value={}) + ) + + runner = CliRunner() + with patches[0], patches[1], patches[2]: + if not provenance: + with patches[3]: + return runner.invoke(routing_mod.routing_group, args) + return runner.invoke(routing_mod.routing_group, args) + + +# --------------------------------------------------------------------------- +# routing list -- shadowed +# --------------------------------------------------------------------------- + + +class TestRoutingListShadowed: + def test_shadowed_matrix_is_marked_and_winner_named(self, tmp_path): + """A user matrix that suppresses a bundle matrix is flagged, both paths shown.""" + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["openai"]) + + result = _invoke(tmp_path, ["list"]) + + assert result.exit_code == 0, result.output + out = result.output + # The row itself carries the marker. + assert "shadows bundle" in out + # And the relationship is spelled out: winner, then what it suppresses. + assert "in use" in out + assert "suppressed" in out + assert "~/.amplifier/routing/openai.yaml" in out + assert ( + "~/.amplifier/cache/amplifier-bundle-routing-matrix-test/routing/openai.yaml" + in out.replace("\n", "") + ) + + # Non-vacuity: the SAME shadowed tree, rendered through the pre-change + # path (no provenance), shows the two files as peers with no marker. + # That difference is the whole defect this change fixes. + before = _invoke(tmp_path, ["list"], provenance=False) + assert "shadow" not in before.output + assert "suppressed" not in before.output + + def test_unshadowed_matrix_in_same_listing_is_not_marked(self, tmp_path): + """Only the colliding name is flagged; peers are left alone.""" + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["openai"]) + + result = _invoke(tmp_path, ["list"]) + + assert result.exit_code == 0, result.output + # 'balanced' exists only in the bundle -- it must not be flagged. + assert "1 matrix is shadowed" in result.output + marked_lines = [ + line for line in result.output.splitlines() if "shadows bundle" in line + ] + assert len(marked_lines) == 1 + assert "balanced" not in marked_lines[0] + + def test_two_shadowed_matrices_are_counted(self, tmp_path): + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["openai", "balanced"]) + + result = _invoke(tmp_path, ["list"]) + + assert result.exit_code == 0, result.output + assert "2 matrices are shadowed" in result.output + + def test_json_carries_hooks_routing_provenance_fields(self, tmp_path): + """JSON output exposes MatrixSource.to_dict() verbatim for shadowed entries.""" + import json + + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["openai"]) + + result = _invoke(tmp_path, ["list", "--format", "json"]) + + assert result.exit_code == 0, result.output + items = json.loads(result.output) + by_shadowed = [i for i in items if "routing_source" in i] + assert len(by_shadowed) == 1 + source = by_shadowed[0]["routing_source"] + assert source["matrix_name"] == "openai" + assert source["matrix_source"] == "user" + assert source["matrix_shadowed"] is True + assert source["matrix_path"].endswith("/.amplifier/routing/openai.yaml") + assert len(source["shadowed_paths"]) == 1 + assert "amplifier-bundle-routing-matrix-test" in source["shadowed_paths"][0] + + +# --------------------------------------------------------------------------- +# routing list -- unshadowed output must not change +# --------------------------------------------------------------------------- + + +class TestRoutingListUnshadowed: + def test_unshadowed_output_is_byte_identical_to_pre_change(self, tmp_path): + """No name collision => output identical to the no-provenance code path.""" + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["my-custom"]) + + with_provenance = _invoke(tmp_path, ["list"]) + without_provenance = _invoke(tmp_path, ["list"], provenance=False) + + assert with_provenance.exit_code == 0, with_provenance.output + assert with_provenance.output == without_provenance.output + assert "shadow" not in with_provenance.output + + def test_unshadowed_json_is_byte_identical_to_pre_change(self, tmp_path): + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["my-custom"]) + + with_provenance = _invoke(tmp_path, ["list", "--format", "json"]) + without_provenance = _invoke( + tmp_path, ["list", "--format", "json"], provenance=False + ) + + assert with_provenance.exit_code == 0, with_provenance.output + assert with_provenance.output == without_provenance.output + assert "routing_source" not in with_provenance.output + + def test_no_user_routing_dir_at_all(self, tmp_path): + """The command still works when ~/.amplifier/routing does not exist.""" + _make_bundle(tmp_path) + assert not (tmp_path / ".amplifier" / "routing").exists() + + result = _invoke(tmp_path, ["list"]) + baseline = _invoke(tmp_path, ["list"], provenance=False) + + assert result.exit_code == 0, result.output + assert "openai" in result.output + assert "balanced" in result.output + assert "shadow" not in result.output + assert result.output == baseline.output + + +# --------------------------------------------------------------------------- +# Graceful degradation -- bundle predates routing-matrix PR #52 +# --------------------------------------------------------------------------- + + +class TestProvenanceUnreachable: + def test_no_marker_when_bundle_has_no_matrix_loader(self, tmp_path): + """Old cached bundle: no resolve_matrix_source => no marker, not a guess.""" + _make_bundle(tmp_path, with_loader=False) + _make_user_dir(tmp_path, ["openai"]) + + result = _invoke(tmp_path, ["list"]) + + assert result.exit_code == 0, result.output + assert "openai" in result.output + assert "shadow" not in result.output + + def test_no_marker_when_loader_lacks_the_function(self, tmp_path): + """A matrix_loader.py without resolve_matrix_source is not an error.""" + from amplifier_app_cli.lib.routing_provenance import resolve_matrix_origins + + routing_dir = _make_bundle(tmp_path, with_loader=False) + pkg = ( + routing_dir.parent + / "modules" + / "hooks-routing" + / "amplifier_module_hooks_routing" + ) + pkg.mkdir(parents=True) + (pkg / "matrix_loader.py").write_text("def load_matrix(path):\n return {}\n") + _make_user_dir(tmp_path, ["openai"]) + + assert resolve_matrix_origins(_discovered(tmp_path)) == {} + + +# --------------------------------------------------------------------------- +# routing show +# --------------------------------------------------------------------------- + + +class TestRoutingShow: + def test_show_names_the_winner_and_the_suppressed_file(self, tmp_path): + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["openai"]) + + result = _invoke(tmp_path, ["show", "openai"]) + + assert result.exit_code == 0, result.output + out = result.output + assert "shadows a bundle matrix" in out + assert "~/.amplifier/routing/openai.yaml" in out + assert "suppressed" in out + + def test_show_unshadowed_output_is_unchanged(self, tmp_path): + _make_bundle(tmp_path) + _make_user_dir(tmp_path, ["my-custom"]) + + with_provenance = _invoke(tmp_path, ["show", "balanced"]) + without_provenance = _invoke(tmp_path, ["show", "balanced"], provenance=False) + + assert with_provenance.exit_code == 0, with_provenance.output + assert with_provenance.output == without_provenance.output + assert "shadow" not in with_provenance.output + + +# --------------------------------------------------------------------------- +# routing_provenance unit tests +# --------------------------------------------------------------------------- + + +class TestRoutingProvenance: + def test_classify_splits_bundle_and_custom_dirs(self, tmp_path): + from amplifier_app_cli.lib.routing_provenance import classify_routing_dirs + + bundle_routing = _make_bundle(tmp_path) + user_dir = _make_user_dir(tmp_path, ["openai"]) + + custom_dirs, bundle_dirs = classify_routing_dirs(_discovered(tmp_path)) + + assert custom_dirs == [user_dir] + assert bundle_dirs == [bundle_routing] + + def test_loads_the_bundles_own_resolve_matrix_source(self, tmp_path): + """The function really comes from the bundle on disk, not from this repo.""" + from amplifier_app_cli.lib.routing_provenance import load_resolve_matrix_source + + bundle_routing = _make_bundle(tmp_path) + fn = load_resolve_matrix_source([bundle_routing]) + + assert fn is not None + loader_path = ( + bundle_routing.parent + / "modules" + / "hooks-routing" + / "amplifier_module_hooks_routing" + / "matrix_loader.py" + ) + assert Path(fn.__code__.co_filename) == loader_path + + def test_origins_report_winner_and_shadowed(self, tmp_path): + from amplifier_app_cli.lib.routing_provenance import resolve_matrix_origins + + bundle_routing = _make_bundle(tmp_path) + user_dir = _make_user_dir(tmp_path, ["openai"]) + + origins = resolve_matrix_origins(_discovered(tmp_path)) + + assert set(origins) == {"openai", "balanced"} + assert origins["openai"].is_shadowed is True + assert origins["openai"].path == user_dir / "openai.yaml" + assert origins["openai"].source == "user" + assert origins["openai"].shadowed == ( + (bundle_routing / "openai.yaml", "bundle"), + ) + assert origins["balanced"].is_shadowed is False + assert origins["balanced"].source == "bundle" + + def test_no_bundle_dir_yields_no_origins(self, tmp_path): + """Without a cached bundle there is no loader to consume -- report nothing.""" + from amplifier_app_cli.lib.routing_provenance import resolve_matrix_origins + + _make_user_dir(tmp_path, ["openai"]) + assert resolve_matrix_origins(_discovered(tmp_path)) == {} From 7aa6bd95421dd615c4e99b3f95825050c3de3c09 Mon Sep 17 00:00:00 2001 From: Amplifier Lane adq Date: Wed, 2 Sep 2026 17:25:48 -0700 Subject: [PATCH 2/3] =?UTF-8?q?docs(adq):=20lane=20DONE-NOTE=20=E2=80=94?= =?UTF-8?q?=20findings,=20seam=20rationale,=20honest=20test=20limitation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../adq-routing-list-shadowing/DONE-NOTE.md | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 ai_working/adq-routing-list-shadowing/DONE-NOTE.md diff --git a/ai_working/adq-routing-list-shadowing/DONE-NOTE.md b/ai_working/adq-routing-list-shadowing/DONE-NOTE.md new file mode 100644 index 00000000..97104fa2 --- /dev/null +++ b/ai_working/adq-routing-list-shadowing/DONE-NOTE.md @@ -0,0 +1,198 @@ +# DONE-NOTE — `model_performance-adq` + +`amplifier routing list` does not mark a user matrix as shadowing a bundle matrix +(app-cli, cross-repo). Lane dir: `lanes/adq-routing-list-shadowing/`. +Worktree: `amplifier-app-cli` @ branch `lane/adq-routing-list-shadowing`. + +**Spend: $0.00.** No API calls beyond this session's own reasoning, no DTU, no +containers, no infrastructure registered or created. Nothing to tear down. +(Two scratch dirs under `/tmp` — `/tmp/adq-demo-home`, `/tmp/rm-upstream` — were +removed at the end; they are not infrastructure and were never registered.) + +--- + +## Deliverables + +| # | deliverable | status | +|---|---|---| +| 1 | routing list implementation named at file:line + what it displays | **DONE** | +| 2 | DRAFT PR on origin, `lane/adq-routing-list-shadowing`, tests green | **DONE** — [microsoft/amplifier-app-cli#293](https://github.com/microsoft/amplifier-app-cli/pull/293) | +| 3 | a test for the shadowed case AND a test proving unshadowed output unchanged | **DONE** | +| 4 | a before/after sample of the new output in the PR body | **DONE** | + +--- + +## (1) The implementation, at file:line + +*(confidence: measured · evidence: files read in this worktree at `0d93352`)* + +| what | where | +|---|---| +| the `list` command | `amplifier_app_cli/commands/routing.py:297-354` | +| where the files come from | `_discover_matrix_files()` — same file, `:98-141` | +| where they become rows | `_load_all_matrices()` — same file, `:153-160` | + +**What it displayed before this change.** `_discover_matrix_files()` globs +`~/.amplifier/cache/amplifier-bundle-routing-matrix-*/routing/*.yaml` (lazily +fetching the bundle on a clean install) plus `get_custom_routing_dir()` → +`~/.amplifier/routing/*.yaml`, and returns `sorted(files)` — a flat list with +no record of which directory each file came from. `_load_all_matrices()` +collapses that list into a `name -> data` dict keyed by the `name:` field +*inside* each YAML, last write wins. `routing_list` then renders one row per +dict entry: active arrow, name, `description`, `covered/total roles` +compatibility, `updated`. + +**Consequence:** two same-named files produce exactly **one** row. The reader +cannot see that a collision happened, which file won, or that the other one is +dead. That is the defect. + +--- + +## (2) Reachability of ell's published fields — the finding that shaped the design + +*(confidence: measured · evidence: import attempt + upstream clone at `d17d03c`)* + +The item asked to consume ell's fields rather than re-implement discovery, and +to say so precisely if they are not reachable. **They are not reachable from +this process.** Two independent reasons: + +1. **`resolve_matrix_source` is not importable.** + `uv run python -c "import amplifier_module_hooks_routing"` → + `ModuleNotFoundError`. `hooks-routing` is a *bundle module*, not a + distribution `amplifier-app-cli` depends on (see `pyproject.toml` + `dependencies`), and it is not on `sys.path`. +2. **The capability fields are not readable either.** `matrix_path` / + `matrix_source` / `shadowed_paths` are published on the + `model_role_resolver` capability **at session start**. `amplifier routing + list` never mounts a bundle — there is no coordinator and no capability in + that process at all. + +**Smallest seam that still avoids re-implementing precedence (what shipped):** +the routing-matrix bundle *is on disk*, in the same cache directory +`_discover_matrix_files()` already globs. So the CLI loads +`modules/hooks-routing/amplifier_module_hooks_routing/matrix_loader.py` **by +file path** (`importlib.util.spec_from_file_location`) and calls the real +`resolve_matrix_source`. Precedence stays in exactly one home; this change adds +no search-order logic of its own. + +**Smaller long-term seam, proposed but not taken (cross-repo, not this lane's +call):** hooks-routing shipping `resolve_matrix_source` somewhere app-cli can +import outright — e.g. a tiny published helper distribution, or app-cli +vendoring the pure function under an explicit sync test. Raised in the PR body +for a maintainer's opinion. + +**Two non-obvious things measured while building the seam:** + +- The **local cache on this host is stale**: + `~/.amplifier/cache/amplifier-bundle-routing-matrix-972b0ce7f0cbc2f7` is at + `99d9b08` (routing-matrix #46) and **has no `resolve_matrix_source`** — + PR #52 (`d17d03c`) is not in it. So on this very host the feature degrades + to "no markers" until the bundle is refreshed. That is by design (see + below), and it is why the before/after demo was run against a fixture home + carrying the actual upstream `matrix_loader.py` rather than the host's. +- **Loading `matrix_loader.py` by path fails unless the module is registered + in `sys.modules` first.** It defines a `@dataclass` (`MatrixSource`), and + `dataclasses` looks its own module up by name while building the class; + without the registration the decorator dies with an opaque + `'NoneType' object has no attribute '__dict__'`. Fixed and commented in + `routing_provenance.py::_load_module`. + +**Degradation is deliberate and silent-by-omission.** When the function is +unreachable — old bundle, no bundle, load failure — `resolve_matrix_origins()` +returns `{}` and **no marker is drawn**. An empty result means "provenance +unknown", never "nothing is shadowed". A wrong shadowing claim is worse than +none, and guessing the search order is exactly what the item forbade. + +--- + +## (3) What shipped + +- **`amplifier_app_cli/lib/routing_provenance.py`** (new) — locate the cached + bundle's `matrix_loader.py`, load it, classify discovered dirs into custom vs + bundle, and return one `MatrixSource` per matrix name. Classifying *where a + directory lives* is not precedence; the aliasing rules (a "custom" dir that + really is the bundle dir; the same file reached twice) stay inside + `resolve_matrix_source`. +- **`commands/routing.py`** — a row marker (`⚠ shadows bundle`, visible in + every view including `--compact`), a footer naming the file **in use** and + each file it **suppresses** (one path per line, `soft_wrap` so Rich cannot + reflow a path into the label column), the same note on `routing show`, and + `MatrixSource.to_dict()` verbatim in `--format json` — the same field + vocabulary ell publishes on the capability. +- **`tests/test_routing_shadowing.py`** (new, 15 tests). + +### Before / after (fixture home, real upstream `matrix_loader.py` @ `d17d03c`) + +Before: + +``` +$ amplifier routing list +── routing matrices (1 active, 1 disabled) ── + [off] balanced (available) ← disabled + [on] → openai (active) +``` + +After: + +``` +$ amplifier routing list +── routing matrices (1 active, 1 disabled) ── + [off] balanced (available) ← disabled + [on] → openai ⚠ shadows bundle (active) + +⚠ 1 matrix is shadowed — only the 'in use' file is loaded: + openai + in use ~/.amplifier/routing/openai.yaml + suppressed ~/.amplifier/cache/amplifier-bundle-routing-matrix-demo/routing/openai.yaml (bundle) +``` + +## (4) Tests, and why they are not vacuous + +*(confidence: measured · evidence: `uv run pytest -q -p no:randomly`)* + +- **shadowed case** — marks the row, names the winner and the suppressed path. + Carries an explicit **non-vacuity assertion**: the *same* shadowed tree + rendered through the pre-change path (provenance forced unavailable) shows no + marker at all. Without that, the test could pass on code that marks + everything. +- **unshadowed unchanged** — not "looks the same": the output string is + compared **byte-for-byte** against the same tree rendered with provenance + unavailable. Text *and* JSON. +- **no user routing dir at all** — `~/.amplifier/routing` absent; command exits + 0, lists both bundle matrices, output identical to baseline. +- **old-bundle degradation** — bundle without `matrix_loader.py`, and a + `matrix_loader.py` present but lacking the function: no marker, no error. +- **the function really comes from the bundle** — asserted via + `fn.__code__.co_filename`, so a future accidental local fallback fails loudly. + +**Honest limitation of the test fixture, stated rather than hidden.** Nothing +importable exists in a test environment (the whole finding above), so the tests +write a **stand-in** `matrix_loader.py` reproducing the contract at +routing-matrix `d17d03c`. It exercises the CLI's real consumption path — +locate the bundle, load by file path, call, render — but it is a test double, +not upstream's code. Two things mitigate it: the before/after demo was produced +against the **actual upstream file** cloned from `d17d03c`, and +`test_loads_the_bundles_own_resolve_matrix_source` pins the loaded function's +`co_filename` to the bundle path. **A contract drift in upstream's +`resolve_matrix_source` would not be caught by these tests** — that is the cost +of the file-path seam, and the argument for the smaller importable seam +proposed above. + +Suite: **1588 passed, 1 skipped, 1 xfailed**. `ruff check` clean on all touched +files; the 8 findings in `commands/session.py` are pre-existing at `0d93352` +and unchanged. `ruff format` applied to touched files only (repo baseline is +not format-clean: 28 files would reformat). + +--- + +## Left open (deliberately not fixed here) + +`_load_all_matrices()` (`routing.py:153-160`) keys rows by the `name:` field +inside the YAML with **last-write-wins over `sorted(files)`**. That is *not* +the loader's rule — the loader resolves by **file stem** — and it agrees with +it today only by alphabetical accident (`cache` < `routing`). This change reads +provenance by the winning row's file stem, so the marker is correct either way, +but the row-selection defect is untouched. It was flagged in the item's own +description and **deserves its own work item**; fixing it changes which matrix +a listing shows, which is a behavior change beyond this item's acceptance +criterion. From fbbcf1af753e502779b6dcdb21b916ed86b84c6b Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:46:05 -0700 Subject: [PATCH 3/3] fix(routing): select the listed matrix by the loader's rule, not last-write-wins `amplifier routing list` built its rows with matrices[data["name"]] = (data, path) # over sorted(discovered files) which diverges from hooks-routing in two independent ways at once: it keyed on the `name:` field INSIDE each YAML (the loader resolves by file STEM), and it let the LAST file in sort order win (the loader takes the FIRST hit in `[*custom_routing_dirs, bundle routing/]`). The two rules agreed only by alphabetical accident -- `~/.amplifier/cache/...` sorts before `~/.amplifier/routing/...` because "c" < "r". When they disagree the command asserts something false: it names a file as in use that the loader would never read. A user file `my-fast.yaml` declaring `name: balanced` overwrote the row for the real `balanced` matrix outright. Rows are now keyed by file stem, and the file behind each row is `MatrixSource.path` -- the value hooks-routing's own `resolve_matrix_source()` assigns to `matrix_path` and loads. Those fields are reachable from the CLI through the seam PR #293 already built, so precedence is not re-derived a third time. Only the winning file is parsed, so a shadowed file can no longer supply a row's description, `updated:` date or compatibility count. Also: `routing use` now writes the filename the loader resolves (it could write an unloadable internal name before), a `name:`/stem disagreement is surfaced rather than silently keyed on the internal name, and every JSON row carries `matrix_file`. When the cached bundle predates routing-matrix PR #52 there is no `resolve_matrix_source` to ask. The shadowing MARKER is still withheld (#293's rule: a wrong marker is worse than none), but a row must point at some file, so selection falls back to the first candidate in `[*custom_dirs, *bundle_dirs]` -- the same list hooks-routing builds as `search_dirs`, in one labelled function. Tests: `tests/test_routing_winner_selection.py` constructs both disagreement classes explicitly, each with a non-vacuity gate that re-runs the old algorithm inline and asserts it picks the other file. Full suite green (1605 passed); existing routing list/show/use tests unmodified. --- .../9kk-routing-list-lastwrite/DONE-NOTE.md | 187 ++++++ amplifier_app_cli/commands/routing.py | 166 +++++- amplifier_app_cli/lib/routing_provenance.py | 100 +++- tests/test_routing_winner_selection.py | 552 ++++++++++++++++++ 4 files changed, 969 insertions(+), 36 deletions(-) create mode 100644 ai_working/9kk-routing-list-lastwrite/DONE-NOTE.md create mode 100644 tests/test_routing_winner_selection.py diff --git a/ai_working/9kk-routing-list-lastwrite/DONE-NOTE.md b/ai_working/9kk-routing-list-lastwrite/DONE-NOTE.md new file mode 100644 index 00000000..3c0f0e86 --- /dev/null +++ b/ai_working/9kk-routing-list-lastwrite/DONE-NOTE.md @@ -0,0 +1,187 @@ +# DONE-NOTE — `model_performance-9kk` + +`amplifier routing list` picked the winning matrix row by last-write-wins, not by the +loader's rule. It could therefore name a file as in use that the loader would never read. + +**Spend: $0.00.** No API calls, no DTU, no infrastructure created or registered. +Everything here is code reading, unit tests, and one local CLI invocation. + +**Stacked on adq's open PR #293** (`lane/adq-routing-list-shadowing`), as instructed — +#293 was still open at the time of writing. Base retargets to `main` once #293 merges. + +--- + +## Deliverable 1 — the last-write-wins resolution, quoted, and why it diverges + +**`amplifier_app_cli/commands/routing.py:153-160`** (as of `origin/main` @ `31ad917`; +`154-170` after #293 renamed it to `_load_all_matrices_with_paths`): + +```python +def _load_all_matrices(matrix_files: list[Path]) -> dict[str, dict[str, Any]]: + """Load all matrix files into a name -> data dict.""" + matrices: dict[str, dict[str, Any]] = {} + for path in matrix_files: + data = _load_matrix(path) + if data and "name" in data: + matrices[data["name"]] = data # <-- last write wins + return matrices +``` + +fed by **`routing.py:141`**: + +```python + return sorted(files) # <-- bundle dirs, then custom dir +``` + +Two independent divergences from the loader, in one line: + +| | the CLI did | hooks-routing does | +|---|---|---| +| **key** | the `name:` field *inside* the YAML | the **file stem** — `search_dir / f"{default_matrix_name}.yaml"` | +| **winner** | whichever file comes **last** in `sorted()` | the **first** hit in `[*custom_routing_dirs, routing_dir]` | + +The loader's rule, quoted from the shipped bundle +(`amplifier_module_hooks_routing/__init__.py:88-96`, the pre-#52 inline form): + +```python + search_dirs = [*custom_routing_dirs, routing_dir] + matrix_path = next( + ( + candidate + for search_dir in search_dirs + if (candidate := search_dir / f"{default_matrix_name}.yaml").exists() + ), + None, + ) +``` + +and its post-#52 form (`routing-matrix` @ `320f24e`, +`__init__.py:108-111` → `matrix_loader.py:73-144`): + +```python + matrix_origin = resolve_matrix_source( + default_matrix_name, custom_routing_dirs, routing_dir + ) + matrix_path = matrix_origin.path +``` + +**Why they agreed until now, by accident:** `sorted()` puts +`~/.amplifier/cache/…` before `~/.amplifier/routing/…` only because `"c" < "r"`. +The user file therefore landed last and won under both rules. Nothing enforced that; +renaming either directory silently flips the CLI's answer with no error. + +**The two ways it broke, both now covered by tests:** + +1. **`name:` ≠ stem.** A user file `my-fast.yaml` declaring `name: balanced` sorted last + and *overwrote the row for the real `balanced` matrix*. `routing list` and + `routing show balanced` then displayed a file the loader can never resolve as + `balanced` — the tool actively asserting something false. And `routing use` wrote that + internal name into settings, which the loader appends `.yaml` to → "Matrix file not + found — routing disabled". +2. **Sort order flips.** Move the bundle cache to any path sorting after + `~/.amplifier/routing/` and last-write-wins hands back the *bundle* file while the + loader loads the *user* file. + +--- + +## Deliverable 2 — the fix (DRAFT PR, branch `lane/9kk-routing-list-lastwrite`) + +Rows are now keyed by **file stem**, and the file behind each row is the one +`resolve_matrix_source()` would load. + +`resolve_matrix_origins()` (shipped by #293) already reaches hooks-routing's own +`resolve_matrix_source` by loading `matrix_loader.py` out of the cached bundle, and its +`MatrixSource.path` is literally the value the loader assigns to `matrix_path`. So the +fields ell published **are** reachable from the CLI — via #293's seam — and this change +consumes them rather than re-deriving precedence a third time. No new seam was needed. + +- `lib/routing_provenance.py` — new `resolve_winning_paths(matrix_files, origins)`: + `{stem: winning_path}`, taken verbatim from `MatrixSource.path` when available. +- `commands/routing.py` — `_load_all_matrices_with_paths()` now selects the winner first + and parses **only** that file, so a shadowed file can no longer supply a row's + description, `updated:` date, or compatibility count. +- `_load_all_matrices()` is keyed by stem, which also fixes `routing use` writing a value + the loader cannot resolve. +- A `name:`/stem disagreement is surfaced (row marker, footer note, `declared_name` in + JSON) instead of being silently keyed on the internal name. +- Every row's JSON now carries `matrix_file` — which file this row is. +- `routing use ` is refused *and* names the filename to use instead. + +### Decision recorded: what happens when `resolve_matrix_source` is unreachable + +A cached bundle older than routing-matrix PR #52 has no function to ask. **This is the +live state on the measurement host** — `~/.amplifier/cache/amplifier-bundle-routing-matrix-972b0ce7f0cbc2f7` +carries no `resolve_matrix_source`, so `resolve_matrix_origins()` returns `{}` there today. + +#293's rule is "a wrong shadowing marker is worse than none", and that is kept — no marker +is drawn. But row *selection* is not symmetric with a marker: a marker may be omitted, +because "no claim" is truthful; a listing row cannot be omitted, so something must be +chosen. Choosing by alphabetical accident is what this item is about. + +**Chosen:** fall back to the first candidate in `[*custom_dirs, *bundle_dirs]` — the same +list hooks-routing builds as `search_dirs`, using #293's existing `classify_routing_dirs()`. +It lives in one function, is labelled as a fallback in its docstring, and is only reached +when the authoritative answer is unavailable. Recorded here rather than escalated, per the +lane's no-waiting rule. + +--- + +## Deliverable 3 — the disagreement test + +`tests/test_routing_winner_selection.py` (17 tests). The two rules are made to +**disagree explicitly**, and each disagreement class carries a non-vacuity gate that +re-runs the old algorithm inline (`_last_write_wins()`) and asserts it picks the *other* +file — so the tests cannot quietly stop testing anything if the trees stop colliding. + +| construction | last-write-wins picks | loader picks | test | +|---|---|---|---| +| bundle under `.amplifier/zz-cache/…` so it sorts **after** `.amplifier/routing/` | bundle file | **user file** | `TestSortOrderDisagreement` (5) | +| user `my-fast.yaml` declaring `name: balanced` | `my-fast.yaml`, keyed `balanced`, real `balanced` row gone | **bundle `balanced.yaml`**, plus a separate `my-fast` row | `TestNameStemDisagreement` (6) | + +Also pinned: + +- **provenance unreachable** — fallback still picks the user file, still draws no + shadowing marker (`TestProvenanceUnreachableFallback`, 2); +- **agreeing tree unchanged**, **no user routing dir at all**, **stock shadowed layout + still picks the user file**, and an **unparseable winner drops the row** rather than + letting the shadowed loser stand in (`TestUnchangedBehaviour`, 4). + +### Honest limitations + +- `resolve_matrix_source` is exercised through a **stand-in** reproducing the upstream + contract (`routing-matrix` `d17d03c` / verified against `320f24e`), because + hooks-routing is a bundle module with nothing to import in a test environment. This + mirrors #293's own approach; the CLI's real consumption path (locate bundle → load + module by file path → call → render) is exercised end to end. +- The stand-in string is duplicated between `test_routing_shadowing.py` and + `test_routing_winner_selection.py`. Deliberate: de-duplicating it means editing + #293's test file while #293 is open. +- No session was actually started against a disagreeing tree — the claim "the loader + would load X" rests on quoting `resolve_matrix_source` and on #293's dynamic load of + the real function, not on a booted session. **(confidence: measured for the CLI's own + selection; inferred for the loader's end behaviour.)** + +--- + +## Verification + +- `uv run pytest -q` → **1605 passed, 1 skipped, 13 deselected, 1 xfailed**. +- Existing `routing list`/`show`/`use` tests (`test_routing_commands.py`, + `test_routing_shadowing.py`, `test_routing_matrix_registration.py`) → 101 passed, + unmodified. +- `ruff check` clean on all three touched files; the repo's 14 pre-existing findings + are unchanged (verified by stashing). +- **Live smoke on the measurement host** (12 matrices, 6 user files, pre-#52 bundle): + console output byte-identical to the pre-change branch, and `--format json` now names + the winner per row — `anthropic` → `~/.amplifier/routing/anthropic.yaml`, + `openai` → `~/.amplifier/routing/openai.yaml`, the other ten → the bundle cache. Both + shadowed matrices resolve to the user file, which is what a session loads. + +## What remains open + +- Once a post-#52 routing-matrix bundle is cached, the shadowing markers this host cannot + currently draw will appear for `anthropic` and `openai`. Worth re-running the smoke check + then — it is the first host state where the authoritative path, not the fallback, is live. +- `_show_matrix_details()` still titles the panel from `matrix_data["name"]`, so a + `name:`/stem mismatch shows the internal name in that one header. The disagreement is + reported alongside it; unifying the header was left out of scope. diff --git a/amplifier_app_cli/commands/routing.py b/amplifier_app_cli/commands/routing.py index 4aa759c7..f353e4cc 100644 --- a/amplifier_app_cli/commands/routing.py +++ b/amplifier_app_cli/commands/routing.py @@ -4,6 +4,7 @@ import logging import re +from collections.abc import Mapping from pathlib import Path from typing import Any, cast @@ -14,7 +15,7 @@ from rich.table import Table from ..lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES -from ..lib.routing_provenance import resolve_matrix_origins +from ..lib.routing_provenance import resolve_matrix_origins, resolve_winning_paths from ..lib.settings import AppSettings, Scope, get_custom_routing_dir from ..provider_loader import get_provider_info, get_provider_models from ..provider_manager import resolve_provider_entry @@ -153,31 +154,128 @@ def _load_matrix(path: Path) -> dict[str, Any] | None: def _load_all_matrices_with_paths( matrix_files: list[Path], + origins: Mapping[str, Any] | None = None, ) -> dict[str, tuple[dict[str, Any], Path]]: - """Load all matrix files into a name -> (data, source_path) dict. + """Load one row per matrix: file stem -> (data, the file that wins). + + Keyed by **file stem**, and the file behind each row is the one + ``resolve_matrix_source()`` would load. Both halves of that sentence are + the fix; the previous implementation was:: + + matrices: dict[str, tuple[dict[str, Any], Path]] = {} + for path in matrix_files: + data = _load_matrix(path) + if data and "name" in data: + matrices[data["name"]] = (data, path) + + which keyed on the ``name:`` field *inside* each YAML and let the LAST + file in ``sorted(_discover_matrix_files())`` overwrite the entry. Neither + half matches the loader: + + * hooks-routing resolves ``f"{matrix_name}.yaml"`` -- by **file stem**. A + file whose internal ``name:`` disagrees with its stem was listed under a + name the loader can never resolve: listable, not loadable. + * hooks-routing takes the **first** hit in ``[*custom_routing_dirs, + bundle routing/]``. Last-write-wins agreed with that only because + ``sorted()`` happens to put ``~/.amplifier/cache/...`` before + ``~/.amplifier/routing/...``. + + Only the winning file is parsed into the row, so a shadowed file can never + supply the description, ``updated:`` date or compatibility count shown for + a matrix. A stem whose winner is unparseable or carries no ``name:`` is + dropped entirely rather than falling back to a file the loader would not + read -- that keeps the old "must have ``name:`` to be listed" rule without + letting a loser back in through it. - Same last-write-wins keying as :func:`_load_all_matrices`, but it also - keeps the file each listed entry actually came from, which is what - shadowing provenance is looked up by (hooks-routing resolves matrices by - file *stem*, while this dict is keyed by the ``name:`` field inside the - YAML -- the two usually agree but are not the same thing). + Args: + matrix_files: Every discovered matrix file. + origins: Result of ``resolve_matrix_origins()``, if already computed. + + Returns: + ``{stem: (parsed_yaml, winning_path)}``. """ + winners = resolve_winning_paths(matrix_files, origins) + matrices: dict[str, tuple[dict[str, Any], Path]] = {} - for path in matrix_files: + for stem, path in winners.items(): data = _load_matrix(path) if data and "name" in data: - matrices[data["name"]] = (data, path) + matrices[stem] = (data, path) return matrices def _load_all_matrices(matrix_files: list[Path]) -> dict[str, dict[str, Any]]: - """Load all matrix files into a name -> data dict.""" + """Load all matrix files into a stem -> data dict. + + The key is the file stem because that is the string every consumer of this + dict compares against ``settings.routing.matrix`` -- and that setting is + what hooks-routing appends ``.yaml`` to. Keying on the YAML's internal + ``name:`` made ``amplifier routing use `` able to write a value the + loader cannot resolve. + """ return { name: data for name, (data, _) in _load_all_matrices_with_paths(matrix_files).items() } +def _declared_name(data: Mapping[str, Any]) -> str: + """The ``name:`` field inside a matrix YAML (may differ from its stem).""" + return str(data.get("name", "")) + + +def _print_matrix_not_found( + matrix_name: str, + loaded: Mapping[str, tuple[dict[str, Any], Path]], +) -> None: + """Report an unknown matrix, and rescue the ``name:``-vs-filename case. + + Rows are keyed by file stem because that is what hooks-routing resolves. + A user who read the old listing (keyed by the YAML's internal ``name:``) + may type that name instead; rather than the bare "not found" they get told + which filename actually carries it. + """ + available = ", ".join(sorted(loaded.keys())) if loaded else "none" + console.print( + f"[red]Matrix '{matrix_name}' not found.[/red] Available: {available}" + ) + + by_declared = [ + stem + for stem, (data, _) in loaded.items() + if _declared_name(data) == matrix_name + ] + if by_declared: + console.print( + f"[yellow]'{matrix_name}' is the 'name:' inside " + f"{', '.join(sorted(by_declared))}.yaml — routing resolves by " + f"filename, so use: {sorted(by_declared)[0]}[/yellow]" + ) + + +def _print_name_stem_note(mismatched: list[tuple[str, str]]) -> None: + """Report every row whose YAML ``name:`` disagrees with its filename. + + Silent on the normal case, so an agreeing tree's output is unchanged. + """ + if not mismatched: + return + + count = len(mismatched) + noun = "matrix declares a" if count == 1 else "matrices declare a" + console.print( + f"[yellow]⚠ {count} {noun} 'name:' that differs from its filename — " + f"routing resolves by filename:[/yellow]" + ) + for stem, declared in mismatched: + console.print( + f" [green]resolves as[/green] {stem}" + f" [dim](file says name: {declared})[/dim]", + soft_wrap=True, + ) + console.print() + + def _display_path(path: Path) -> str: """Render a path with ``~`` for the home directory, for readable output.""" try: @@ -318,17 +416,19 @@ def routing_list(compact: bool, detailed: bool, fmt: str): ) return - loaded = _load_all_matrices_with_paths(matrix_files) - if not loaded: - console.print("[yellow]No valid routing matrices found.[/yellow]") - return - # Shadowing provenance, derived from hooks-routing's own # resolve_matrix_source() -- never from a search order re-derived here. # An empty dict means "provenance unknown" (bundle too old / unreachable), # NOT "nothing is shadowed", so no marker is drawn in that case. + # It is resolved BEFORE the matrices are loaded because it also decides + # which file each row is loaded FROM. origins = resolve_matrix_origins(matrix_files) + loaded = _load_all_matrices_with_paths(matrix_files, origins) + if not loaded: + console.print("[yellow]No valid routing matrices found.[/yellow]") + return + routing_config = settings.get_routing_config() active_matrix = routing_config.get("matrix", "balanced") provider_types = _get_configured_provider_types(settings) @@ -339,6 +439,7 @@ def routing_list(compact: bool, detailed: bool, fmt: str): items: list[dict[str, Any]] = [] shadowed: list[tuple[str, Any]] = [] + mismatched: list[tuple[str, str]] = [] for name, (data, source_path) in sorted(loaded.items()): is_active = name == active_matrix description = data.get("description", "") @@ -351,6 +452,8 @@ def routing_list(compact: bool, detailed: bool, fmt: str): origin = origins.get(source_path.stem) is_shadowing = origin is not None and origin.is_shadowed + declared = _declared_name(data) + name_disagrees = declared != name config_summary: dict[str, Any] = { "description": description, @@ -360,11 +463,19 @@ def routing_list(compact: bool, detailed: bool, fmt: str): item: dict[str, Any] = { "name": ("→ " if is_active else " ") + name - + (f" ⚠ shadows {_shadow_label(origin)}" if is_shadowing else ""), + + (f" ⚠ shadows {_shadow_label(origin)}" if is_shadowing else "") + + (f" ⚠ file says name: {declared}" if name_disagrees else ""), "enabled": is_active, "behaviors": ["active" if is_active else "available"], "config_summary": config_summary, } + # The row key is the file stem -- what hooks-routing resolves and what + # `routing use` must write. `declared_name` is reported alongside so a + # disagreement is visible instead of silently keying on the wrong one. + item["matrix_file"] = _display_path(source_path) + if name_disagrees: + mismatched.append((name, declared)) + config_summary["declared_name"] = declared if is_shadowing: shadowed.append((name, origin)) @@ -384,6 +495,7 @@ def routing_list(compact: bool, detailed: bool, fmt: str): items, view=view, category="routing", section_title="routing matrices" ) _print_shadowing_footer(shadowed) + _print_name_stem_note(mismatched) # ============================================================ @@ -404,13 +516,11 @@ def routing_use(matrix_name: str, scope: str): validate_scope_cli(scope) settings = _get_settings() matrix_files = _discover_matrix_files() - matrices = _load_all_matrices(matrix_files) + loaded = _load_all_matrices_with_paths(matrix_files) + matrices = {name: data for name, (data, _) in loaded.items()} if matrix_name not in matrices: - available = ", ".join(sorted(matrices.keys())) if matrices else "none" - console.print( - f"[red]Matrix '{matrix_name}' not found.[/red] Available: {available}" - ) + _print_matrix_not_found(matrix_name, loaded) return settings.set_routing_matrix(matrix_name, scope=cast(Scope, scope)) @@ -438,7 +548,8 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st """ settings = _get_settings() matrix_files = _discover_matrix_files() - loaded = _load_all_matrices_with_paths(matrix_files) + origins = resolve_matrix_origins(matrix_files) + loaded = _load_all_matrices_with_paths(matrix_files, origins) matrices = {name: data for name, (data, _) in loaded.items()} if not matrices: @@ -451,27 +562,28 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st matrix_name = routing_config.get("matrix", "balanced") if matrix_name not in matrices: - available = ", ".join(sorted(matrices.keys())) - console.print( - f"[red]Matrix '{matrix_name}' not found.[/red] Available: {available}" - ) + _print_matrix_not_found(matrix_name, loaded) return view = resolve_view( ("routing", "show"), compact_flag=compact, detailed_flag=detailed ) matrix_data, source_path = loaded[matrix_name] - origin = resolve_matrix_origins(matrix_files).get(source_path.stem) + origin = origins.get(source_path.stem) if fmt == "json": renderer = ItemRenderer(console) payload: dict[str, Any] = {"matrix": matrix_name, "data": matrix_data} + payload["matrix_file"] = _display_path(source_path) if origin is not None and origin.is_shadowed: payload["routing_source"] = origin.to_dict() renderer.render_json(payload) return _print_shadowing_note(origin) + declared = _declared_name(matrix_data) + if declared != matrix_name: + _print_name_stem_note([(matrix_name, declared)]) # detailed view → full waterfall; regular/compact → resolved routing if view == "detailed": diff --git a/amplifier_app_cli/lib/routing_provenance.py b/amplifier_app_cli/lib/routing_provenance.py index 281b032e..30237814 100644 --- a/amplifier_app_cli/lib/routing_provenance.py +++ b/amplifier_app_cli/lib/routing_provenance.py @@ -33,7 +33,7 @@ import importlib.util import logging import sys -from collections.abc import Callable, Sequence +from collections.abc import Callable, Mapping, Sequence from pathlib import Path from typing import Any @@ -176,6 +176,17 @@ def _load_module(loader_path: Path) -> Any | None: return None +def _stems_in_order(matrix_files: Sequence[Path]) -> list[str]: + """Every distinct file stem, in first-seen order.""" + stems: list[str] = [] + seen: set[str] = set() + for file_path in matrix_files: + if file_path.stem not in seen: + seen.add(file_path.stem) + stems.append(file_path.stem) + return stems + + def resolve_matrix_origins(matrix_files: Sequence[Path]) -> dict[str, Any]: """Map matrix file stem -> ``MatrixSource`` for every discovered matrix. @@ -192,15 +203,8 @@ def resolve_matrix_origins(matrix_files: Sequence[Path]) -> dict[str, Any]: if resolve is None or not bundle_dirs: return {} - stems: list[str] = [] - seen_stems: set[str] = set() - for file_path in matrix_files: - if file_path.stem not in seen_stems: - seen_stems.add(file_path.stem) - stems.append(file_path.stem) - origins: dict[str, Any] = {} - for stem in stems: + for stem in _stems_in_order(matrix_files): best: Any | None = None # A host normally has exactly one cached routing-matrix bundle. If it # has several, the CLI cannot know which one a session would mount, so @@ -218,3 +222,81 @@ def resolve_matrix_origins(matrix_files: Sequence[Path]) -> dict[str, Any]: origins[stem] = best return origins + + +def resolve_winning_paths( + matrix_files: Sequence[Path], + origins: Mapping[str, Any] | None = None, +) -> dict[str, Path]: + """Map matrix file stem -> the file hooks-routing would actually load. + + This is the answer to "which file wins?", and it is deliberately NOT + derived from the order ``_discover_matrix_files()`` happens to return. + That order is ``sorted()``, and ``sorted()`` puts + ``~/.amplifier/cache/...`` before ``~/.amplifier/routing/...`` only + because ``"c" < "r"``. Any rename of either directory silently flips the + answer, with no error. + + Two sources, in order: + + 1. **The loader's own function.** ``origins`` (from + :func:`resolve_matrix_origins`) carries hooks-routing's + ``MatrixSource.path`` -- literally the value its ``mount()`` assigns to + ``matrix_path`` and loads (routing-matrix ``__init__.py``: ``matrix_origin + = resolve_matrix_source(...)`` then ``matrix_path = matrix_origin.path``). + When present, that path is used verbatim. + + 2. **Directory precedence, as a labelled fallback.** When the cached + bundle predates routing-matrix PR #52 there is no + ``resolve_matrix_source`` to ask, yet the CLI must still put *some* file + in each row. It then picks the first candidate whose directory appears + earliest in ``[*custom_dirs, *bundle_dirs]`` -- the same list + hooks-routing builds as ``search_dirs = [*custom_routing_dirs, + routing_dir]``. + + The distinction between this and the shadowing *marker* is deliberate. A + marker can be omitted when provenance is unknown (and is -- see + :func:`resolve_matrix_origins`), because "no claim" is a truthful state. A + listing row cannot be omitted, so the fallback picks by the documented rule + rather than by an alphabetical accident. + + Args: + matrix_files: Every discovered matrix file. + origins: Result of :func:`resolve_matrix_origins`, if already computed. + Passing it avoids re-loading the bundle module. + + Returns: + ``{stem: winning_path}``, one entry per distinct stem. + """ + if origins is None: + origins = resolve_matrix_origins(matrix_files) + + custom_dirs, bundle_dirs = classify_routing_dirs(matrix_files) + dir_rank = {_key(d): i for i, d in enumerate([*custom_dirs, *bundle_dirs])} + + by_stem: dict[str, list[Path]] = {} + for file_path in matrix_files: + by_stem.setdefault(file_path.stem, []).append(file_path) + + winners: dict[str, Path] = {} + for stem in _stems_in_order(matrix_files): + candidates = by_stem[stem] + + origin = origins.get(stem) + loader_path = getattr(origin, "path", None) if origin is not None else None + if loader_path is not None: + # The loader's own answer. Prefer the discovered Path object that + # denotes the same file, so callers keep the path they globbed. + loader_key = _key(Path(loader_path)) + match = next((c for c in candidates if _key(c) == loader_key), None) + winners[stem] = match if match is not None else Path(loader_path) + continue + + # Fallback: earliest directory in [*custom_dirs, *bundle_dirs]. + # Ties (same directory reached twice) keep discovery order. + winners[stem] = min( + candidates, + key=lambda p: dir_rank.get(_key(p.parent), len(dir_rank)), + ) + + return winners diff --git a/tests/test_routing_winner_selection.py b/tests/test_routing_winner_selection.py new file mode 100644 index 00000000..59dfc8f5 --- /dev/null +++ b/tests/test_routing_winner_selection.py @@ -0,0 +1,552 @@ +"""`amplifier routing list`/`show`/`use` pick the row the LOADER would load. + +Background. The listing used to build its rows like this:: + + matrices[data["name"]] = (data, path) # over sorted(discovered files) + +Two things in one line, and neither matches hooks-routing: + +* the key is the ``name:`` field *inside* the YAML, but the loader resolves + ``f"{matrix_name}.yaml"`` -- by **file stem**; +* the winner is whichever file comes LAST in ``sorted()``, but the loader takes + the FIRST hit in ``[*custom_routing_dirs, bundle routing/]``. + +On a stock host those two rules agree by accident: ``~/.amplifier/cache/...`` +sorts before ``~/.amplifier/routing/...`` because ``"c" < "r"``, so the user +file lands last and wins either way. This file constructs the cases where they +DISAGREE, which is the whole point -- when they disagree, the old listing told +the user that a file was in use which the loader would never read. + +Each disagreement test re-runs the old algorithm inline +(:func:`_last_write_wins`) and asserts it picks the *other* file first, so the +test cannot quietly become vacuous if the trees stop colliding. + +About ``_STAND_IN_MATRIX_LOADER``: hooks-routing is a *bundle* module, not a +distribution this repo depends on, so there is nothing to import in a test +environment. The stand-in reproduces the contract of +``amplifier_module_hooks_routing.matrix_loader.resolve_matrix_source`` as of +routing-matrix ``d17d03c`` (PR #52). It mirrors the copy in +``test_routing_shadowing.py``; both are stand-ins for the same upstream +function, which is the single source of truth for the precedence rule. +Production code never uses either -- it loads the real module out of the +cached bundle. +""" + +import json +from pathlib import Path +from unittest.mock import patch + +import yaml +from click.testing import CliRunner + +from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths + +# --------------------------------------------------------------------------- +# Stand-in for the bundle's matrix_loader.py (contract: routing-matrix d17d03c) +# --------------------------------------------------------------------------- + +_STAND_IN_MATRIX_LOADER = '''\ +"""Stand-in for amplifier_module_hooks_routing.matrix_loader (test fixture).""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +USER_SOURCE = "user" +BUNDLE_SOURCE = "bundle" + + +@dataclass(frozen=True) +class MatrixSource: + name: str + path: Path | None = None + source: str | None = None + shadowed: tuple[tuple[Path, str], ...] = () + searched: tuple[Path, ...] = field(default=()) + + @property + def is_shadowed(self) -> bool: + return bool(self.shadowed) + + def to_dict(self) -> dict[str, Any]: + return { + "matrix_name": self.name, + "matrix_path": str(self.path) if self.path is not None else None, + "matrix_source": self.source, + "matrix_shadowed": self.is_shadowed, + "shadowed_paths": [str(p) for p, _ in self.shadowed], + } + + +def resolve_matrix_source( + name: str, + custom_routing_dirs: Sequence[Path], + bundle_routing_dir: Path, +) -> MatrixSource: + filename = f"{name}.yaml" + + def _key(path: Path) -> Path: + try: + return path.resolve() + except OSError: + return path + + bundle_key = _key(bundle_routing_dir) + candidates: list[tuple[Path, str]] = [ + ( + Path(d) / filename, + BUNDLE_SOURCE if _key(Path(d)) == bundle_key else USER_SOURCE, + ) + for d in custom_routing_dirs + ] + candidates.append((bundle_routing_dir / filename, BUNDLE_SOURCE)) + + searched: list[Path] = [] + present: list[tuple[Path, str]] = [] + seen: set[Path] = set() + for candidate, origin in candidates: + candidate_key = _key(candidate) + if candidate_key in seen: + continue + seen.add(candidate_key) + searched.append(candidate) + if candidate.exists(): + present.append((candidate, origin)) + + if not present: + return MatrixSource(name=name, searched=tuple(searched)) + + winner, winner_source = present[0] + return MatrixSource( + name=name, + path=winner, + source=winner_source, + shadowed=tuple(present[1:]), + searched=tuple(searched), + ) +''' + + +# --------------------------------------------------------------------------- +# The algorithm this change removes, kept so disagreement can be PROVEN +# --------------------------------------------------------------------------- + + +def _last_write_wins(matrix_files: list[Path]) -> dict[str, Path]: + """The pre-change row selection, verbatim in behaviour. + + ``matrices[data["name"]] = (data, path)`` over the discovered files, so the + last file in sort order overwrites the entry, keyed by the YAML's internal + ``name:``. + """ + winners: dict[str, Path] = {} + for path in matrix_files: + data = yaml.safe_load(path.read_text()) or {} + if data and "name" in data: + winners[data["name"]] = path + return winners + + +# --------------------------------------------------------------------------- +# Tree fixtures +# --------------------------------------------------------------------------- + + +def _matrix(name: str, description: str) -> dict: + return { + "name": name, + "description": description, + "updated": "2026-09-02", + "roles": { + "general": { + "description": "Catch-all", + "candidates": [{"provider": "anthropic", "model": "claude-sonnet-*"}], + }, + "fast": { + "description": "Quick tasks", + "candidates": [{"provider": "anthropic", "model": "claude-haiku-*"}], + }, + }, + } + + +def _make_bundle( + tmp_path: Path, + *, + cache_dirname: str = "cache", + with_loader: bool = True, + matrices: dict[str, str] | None = None, +) -> Path: + """Create a cached routing-matrix bundle tree. Returns its ``routing/`` dir. + + ``cache_dirname`` is a knob on purpose. The bundle normally lives under + ``~/.amplifier/cache/``, which sorts before ``~/.amplifier/routing/``; + passing ``"zz-cache"`` reverses that, which is exactly the "any change to + the cache path silently flips which file the CLI shows" case. + """ + bundle_root = ( + tmp_path / ".amplifier" / cache_dirname / "amplifier-bundle-routing-matrix-test" + ) + routing_dir = bundle_root / "routing" + routing_dir.mkdir(parents=True) + + contents = matrices or { + "openai": "Shipped OpenAI routing.", + "balanced": "Shipped balanced routing.", + } + for stem, description in contents.items(): + (routing_dir / f"{stem}.yaml").write_text(yaml.dump(_matrix(stem, description))) + + if with_loader: + pkg = bundle_root / "modules" / "hooks-routing" + pkg = pkg / "amplifier_module_hooks_routing" + pkg.mkdir(parents=True) + (pkg / "matrix_loader.py").write_text(_STAND_IN_MATRIX_LOADER) + + return routing_dir + + +def _make_user_dir(tmp_path: Path, files: dict[str, dict]) -> Path: + """Write ``{stem: matrix_dict}`` into ``/.amplifier/routing/``.""" + user_dir = tmp_path / ".amplifier" / "routing" + user_dir.mkdir(parents=True, exist_ok=True) + for stem, data in files.items(): + (user_dir / f"{stem}.yaml").write_text(yaml.dump(data)) + return user_dir + + +def _make_settings(tmp_path: Path) -> AppSettings: + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + settings = AppSettings(paths=paths) + scope_settings = settings._read_scope("global") + scope_settings["config"] = { + "providers": [{"module": "provider-anthropic", "config": {"priority": 1}}] + } + settings._write_scope("global", scope_settings) + return settings + + +def _discovered(tmp_path: Path) -> list[Path]: + """Everything ``_discover_matrix_files()`` would return, same sort order. + + The real function globs the bundle cache and then the custom dir and + returns ``sorted(files)`` -- so only the sort order matters here, not the + order the two globs ran in. + """ + amp = tmp_path / ".amplifier" + return sorted(amp.glob("**/routing/*.yaml")) + + +def _invoke(tmp_path: Path, args: list[str]): + """Run a routing subcommand against the tmp tree.""" + from amplifier_app_cli.commands import routing as routing_mod + + settings = _make_settings(tmp_path) + with ( + patch.object(routing_mod, "_get_settings", return_value=settings), + patch.object( + routing_mod, "_discover_matrix_files", return_value=_discovered(tmp_path) + ), + patch.object(routing_mod.Path, "home", return_value=tmp_path), + ): + return CliRunner().invoke(routing_mod.routing_group, args) + + +def _flat(output: str) -> str: + """Collapse Rich's console wrapping so substring asserts are width-proof.""" + return " ".join(output.split()) + + +def _row_key(rendered_name: str) -> str: + """Strip the ``→ `` active marker and any ``⚠ ...`` suffixes from a row name.""" + name = rendered_name.strip().removeprefix("→ ").strip() + return name.split(" ")[0].strip() + + +def _rows(tmp_path: Path) -> dict[str, dict]: + """`routing list --format json`, keyed by the row's bare name.""" + result = _invoke(tmp_path, ["list", "--format", "json"]) + assert result.exit_code == 0, result.output + return {_row_key(item["name"]): item for item in json.loads(result.output)} + + +# --------------------------------------------------------------------------- +# Disagreement 1 -- sort order no longer puts the user file last +# --------------------------------------------------------------------------- + + +class TestSortOrderDisagreement: + """The bundle file sorts LAST, so last-write-wins picks it; the loader does not.""" + + def test_the_two_rules_actually_disagree_on_this_tree(self, tmp_path): + """Non-vacuity gate: if this fails, the tests below prove nothing.""" + bundle = _make_bundle(tmp_path, cache_dirname="zz-cache") + user = _make_user_dir( + tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")} + ) + files = _discovered(tmp_path) + + # The old rule: last file in sort order wins. + assert _last_write_wins(files)["openai"] == bundle / "openai.yaml" + + # The loader's rule: first hit in [*custom_dirs, bundle routing/]. + from amplifier_app_cli.lib.routing_provenance import resolve_matrix_origins + + assert resolve_matrix_origins(files)["openai"].path == user / "openai.yaml" + + def test_resolve_winning_paths_follows_the_loader(self, tmp_path): + from amplifier_app_cli.lib.routing_provenance import resolve_winning_paths + + _make_bundle(tmp_path, cache_dirname="zz-cache") + user = _make_user_dir( + tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")} + ) + + winners = resolve_winning_paths(_discovered(tmp_path)) + + assert winners["openai"] == user / "openai.yaml" + assert winners["balanced"].name == "balanced.yaml" + + def test_list_shows_the_loaders_winner_not_the_last_written(self, tmp_path): + _make_bundle(tmp_path, cache_dirname="zz-cache") + _make_user_dir(tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")}) + + rows = _rows(tmp_path) + + assert rows["openai"]["matrix_file"] == "~/.amplifier/routing/openai.yaml" + summary = rows["openai"]["config_summary"] + assert summary["description"] == "Custom matrix: openai" + assert summary["description"] != "Shipped OpenAI routing." + + def test_show_renders_the_loaders_winner(self, tmp_path): + _make_bundle(tmp_path, cache_dirname="zz-cache") + _make_user_dir(tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")}) + + result = _invoke(tmp_path, ["show", "openai", "--detailed"]) + + assert result.exit_code == 0, result.output + assert "Custom matrix: openai" in result.output + assert "Shipped OpenAI routing." not in result.output + + def test_shadowing_marker_still_names_the_same_winner(self, tmp_path): + """The row and the shadowing footer must not contradict each other.""" + _make_bundle(tmp_path, cache_dirname="zz-cache") + _make_user_dir(tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")}) + + rows = _rows(tmp_path) + source = rows["openai"]["routing_source"] + + assert source["matrix_path"].endswith("/.amplifier/routing/openai.yaml") + assert rows["openai"]["matrix_file"] == "~/.amplifier/routing/openai.yaml" + assert len(source["shadowed_paths"]) == 1 + assert "amplifier-bundle-routing-matrix-test" in source["shadowed_paths"][0] + + +# --------------------------------------------------------------------------- +# Disagreement 2 -- the YAML's `name:` differs from its filename +# --------------------------------------------------------------------------- + + +class TestNameStemDisagreement: + """A user file named ``my-fast.yaml`` that declares ``name: balanced``. + + Under last-write-wins it sorted last and OVERWROTE the row for the real + ``balanced`` matrix, so ``routing list``/``show balanced`` displayed a file + the loader would never resolve as ``balanced``. + """ + + def _tree(self, tmp_path: Path) -> tuple[Path, Path]: + bundle = _make_bundle( + tmp_path, matrices={"balanced": "Shipped balanced routing."} + ) + user = _make_user_dir( + tmp_path, {"my-fast": _matrix("balanced", "Custom matrix: my-fast")} + ) + return bundle, user + + def test_the_two_rules_actually_disagree_on_this_tree(self, tmp_path): + """Non-vacuity gate: the old rule really did hand back the wrong file.""" + bundle, user = self._tree(tmp_path) + files = _discovered(tmp_path) + + old = _last_write_wins(files) + # One row, keyed "balanced", pointing at my-fast.yaml. + assert old["balanced"] == user / "my-fast.yaml" + assert "my-fast" not in old + + from amplifier_app_cli.lib.routing_provenance import resolve_winning_paths + + new = resolve_winning_paths(files) + assert new["balanced"] == bundle / "balanced.yaml" + assert new["my-fast"] == user / "my-fast.yaml" + + def test_list_keys_rows_by_filename_and_flags_the_disagreement(self, tmp_path): + self._tree(tmp_path) + + rows = _rows(tmp_path) + + assert set(rows) == {"balanced", "my-fast"} + assert ( + rows["balanced"]["config_summary"]["description"] + == "Shipped balanced routing." + ) + assert rows["balanced"]["matrix_file"].endswith( + "amplifier-bundle-routing-matrix-test/routing/balanced.yaml" + ) + assert rows["my-fast"]["config_summary"]["declared_name"] == "balanced" + assert "declared_name" not in rows["balanced"]["config_summary"] + + def test_console_listing_surfaces_the_disagreement(self, tmp_path): + self._tree(tmp_path) + + result = _invoke(tmp_path, ["list"]) + + assert result.exit_code == 0, result.output + assert "file says name: balanced" in _flat(result.output) + assert "routing resolves by filename" in _flat(result.output) + + def test_show_balanced_renders_the_bundle_file(self, tmp_path): + self._tree(tmp_path) + + result = _invoke(tmp_path, ["show", "balanced", "--detailed"]) + + assert result.exit_code == 0, result.output + assert "Shipped balanced routing." in result.output + assert "Custom matrix: my-fast" not in result.output + + def test_use_writes_the_filename_the_loader_resolves(self, tmp_path): + self._tree(tmp_path) + + result = _invoke(tmp_path, ["use", "my-fast"]) + + assert result.exit_code == 0, result.output + assert "set to 'my-fast'" in result.output + + def test_use_of_a_declared_name_is_refused_with_the_filename_to_use(self, tmp_path): + """The old listing advertised ``turbo``; the loader can never load it.""" + _make_bundle(tmp_path, matrices={"balanced": "Shipped balanced routing."}) + _make_user_dir( + tmp_path, {"my-fast": _matrix("turbo", "Custom matrix: my-fast")} + ) + + result = _invoke(tmp_path, ["use", "turbo"]) + + assert result.exit_code == 0, result.output + assert "not found" in _flat(result.output) + assert "routing resolves by filename" in _flat(result.output) + assert "use: my-fast" in _flat(result.output) + + +# --------------------------------------------------------------------------- +# Provenance unreachable -- still not last-write-wins +# --------------------------------------------------------------------------- + + +class TestProvenanceUnreachableFallback: + """A cached bundle older than routing-matrix PR #52 has no function to ask. + + The shadowing MARKER is still withheld (a wrong marker is worse than none), + but a row has to point at some file, and the fallback picks by the same + ``[*custom_dirs, *bundle_dirs]`` precedence the loader uses -- never by + sort order. + """ + + def test_fallback_picks_the_user_file_when_it_sorts_first(self, tmp_path): + from amplifier_app_cli.lib.routing_provenance import ( + resolve_matrix_origins, + resolve_winning_paths, + ) + + bundle = _make_bundle(tmp_path, cache_dirname="zz-cache", with_loader=False) + user = _make_user_dir( + tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")} + ) + files = _discovered(tmp_path) + + assert resolve_matrix_origins(files) == {} # nothing to ask + assert _last_write_wins(files)["openai"] == bundle / "openai.yaml" + assert resolve_winning_paths(files)["openai"] == user / "openai.yaml" + + def test_listing_shows_the_user_file_but_draws_no_shadow_marker(self, tmp_path): + _make_bundle(tmp_path, cache_dirname="zz-cache", with_loader=False) + _make_user_dir(tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")}) + + result = _invoke(tmp_path, ["list"]) + rows = _rows(tmp_path) + + assert result.exit_code == 0, result.output + assert "shadow" not in result.output + assert ( + rows["openai"]["config_summary"]["description"] == "Custom matrix: openai" + ) + + +# --------------------------------------------------------------------------- +# Cases that must NOT change +# --------------------------------------------------------------------------- + + +class TestUnchangedBehaviour: + def test_agreeing_tree_lists_every_matrix_once(self, tmp_path): + """Stock layout: cache sorts first, names match stems -- nothing moves.""" + _make_bundle(tmp_path) + _make_user_dir( + tmp_path, {"my-custom": _matrix("my-custom", "Custom matrix: my-custom")} + ) + + result = _invoke(tmp_path, ["list"]) + rows = _rows(tmp_path) + + assert result.exit_code == 0, result.output + assert set(rows) == {"openai", "balanced", "my-custom"} + assert "shadow" not in result.output + assert "file says name:" not in result.output + + def test_no_user_routing_dir_at_all(self, tmp_path): + _make_bundle(tmp_path) + assert not (tmp_path / ".amplifier" / "routing").exists() + + result = _invoke(tmp_path, ["list"]) + rows = _rows(tmp_path) + + assert result.exit_code == 0, result.output + assert set(rows) == {"openai", "balanced"} + assert "shadow" not in result.output + assert "file says name:" not in result.output + + def test_stock_shadowed_layout_still_picks_the_user_file(self, tmp_path): + """The case where the two rules AGREE must keep agreeing.""" + from amplifier_app_cli.lib.routing_provenance import resolve_winning_paths + + _make_bundle(tmp_path) + user = _make_user_dir( + tmp_path, {"openai": _matrix("openai", "Custom matrix: openai")} + ) + files = _discovered(tmp_path) + + assert _last_write_wins(files)["openai"] == user / "openai.yaml" + assert resolve_winning_paths(files)["openai"] == user / "openai.yaml" + + def test_unparseable_winner_drops_the_row_rather_than_showing_the_loser( + self, tmp_path + ): + """A broken user file must not let the shadowed bundle file stand in. + + The loader would fail on that matrix; showing the bundle file instead + would be the same class of lie in the other direction. + """ + _make_bundle(tmp_path) + user = _make_user_dir(tmp_path, {}) + (user / "openai.yaml").write_text("[not, a, mapping]\n") + + rows = _rows(tmp_path) + + assert "openai" not in rows + assert "balanced" in rows