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 0000000..3c0f0e8 --- /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 4aa759c..f353e4c 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 281b032..3023781 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 0000000..59dfc8f --- /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