From 8548b7197c81cab735e788e801c1dbf8e8124e67 Mon Sep 17 00:00:00 2001 From: Amplifier Lane adq Date: Wed, 2 Sep 2026 17:23:02 -0700 Subject: [PATCH 1/2] 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 450f309b4fb0c03112a1103a2ef01cb207002d19 Mon Sep 17 00:00:00 2001 From: Amplifier Lane adq Date: Wed, 2 Sep 2026 17:25:48 -0700 Subject: [PATCH 2/2] =?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.