diff --git a/README.md b/README.md index 8a6cab06..47fc6a4a 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,39 @@ amplifier routing list # List available ma amplifier routing use [--local|--project|--global] # Select active matrix amplifier routing show [] # Show resolved roles for a matrix amplifier routing manage # Interactive routing dashboard +``` + +### Routing matrix precedence + +A bundle may declare a default routing matrix (a top-level `routing:` section +in its frontmatter). This is a DEFAULT ONLY -- it is the weakest source in +the precedence chain, so any settings-scope selection always wins over it. +Precedence, weakest to strongest: + +``` +built-in default +< bundle-declared routing.matrix +< ~/.amplifier/settings.yaml (global) +< .amplifier/settings.yaml (project) +< .amplifier/settings.local.yaml (project-local, gitignored) +``` + +If a bundle declares no routing matrix, behavior is exactly as before this +feature existed -- the bundle default only ever applies when nothing else in +settings has set `routing.matrix`. + +`amplifier routing show` (with no explicit matrix name) prints a `Source:` +line above the resolved role table, naming the settings scope file that set +the active matrix (or `built-in default` when none did). `amplifier bundle +show` prints a `Routing matrix: (bundle default)` line when the +active bundle declares one. +When a bundle's declared matrix isn't installed (not found under +`~/.amplifier/routing` or the cached routing-matrix bundle), the CLI drops +it, warns loudly, and falls back to the next-weakest configured matrix (or +no routing) -- a bad bundle default can never brick a session. + +```bash # Module management amplifier module add [--local|--project|--global] amplifier module remove [--scope] diff --git a/amplifier_app_cli/commands/bundle.py b/amplifier_app_cli/commands/bundle.py index 64d1262c..02ed4aa1 100644 --- a/amplifier_app_cli/commands/bundle.py +++ b/amplifier_app_cli/commands/bundle.py @@ -582,6 +582,17 @@ def _fmt_step(s: Any) -> str: active = bundle_obj.name == active_bundle console.print(f"{indent}active: {'yes' if active else 'no'}") + # Bundle-declared routing default (a top-level ``routing:`` section in + # bundle frontmatter). Read defensively via getattr() -- forward-compat + # with installs of amplifier-foundation that don't declare a ``routing`` + # field on Bundle yet. + bundle_routing = getattr(bundle_obj, "routing", None) or {} + if isinstance(bundle_routing, dict) and bundle_routing.get("matrix"): + console.print( + f"{indent}Routing matrix: {escape_markup(str(bundle_routing['matrix']))} " + "(bundle default)" + ) + if view == "detailed": # Full mount plan details providers = mount_plan.get("providers", []) diff --git a/amplifier_app_cli/commands/routing.py b/amplifier_app_cli/commands/routing.py index b5f991d5..aca04042 100644 --- a/amplifier_app_cli/commands/routing.py +++ b/amplifier_app_cli/commands/routing.py @@ -13,28 +13,22 @@ from rich.prompt import Confirm, Prompt from rich.table import Table -from ..lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES -from ..lib.settings import AppSettings, Scope, get_custom_routing_dir +from ..lib.routing_matrices import discover_matrix_files as _discover_matrix_files_impl +from ..lib.settings import AppSettings, Scope from ..provider_loader import get_provider_info, get_provider_models from ..provider_manager import resolve_provider_entry from ..ui.item_renderer import ItemRenderer -from ..ui.view_policy import resolve_view, view_flags from ..ui.scope import ( is_scope_change_available, print_scope_indicator, prompt_scope_change, validate_scope_cli, ) +from ..ui.view_policy import resolve_view, view_flags console = Console() logger = logging.getLogger(__name__) -# Single source of truth for the routing-matrix bundle URL lives in -# WELL_KNOWN_BUNDLES (discovery.py). The CLI lazy-fetches on first use so -# that `amplifier routing list` works on a clean install without requiring -# a prior `amplifier update`. -_ROUTING_BUNDLE_URI = str(WELL_KNOWN_BUNDLES["routing-matrix"]["remote"]) - INFRASTRUCTURE_CONFIG_FIELDS = frozenset( { "base_url", @@ -73,72 +67,17 @@ def _get_settings() -> AppSettings: return AppSettings() -def _ensure_routing_bundle_cached() -> None: - """Fetch the routing-matrix bundle into the cache if not yet present. - - Called lazily from _discover_matrix_files() so `amplifier routing list` - works on a clean install without requiring the user to run - `amplifier update` first. FoundationGitSource.resolve() is a sync - wrapper that is safe to call from a synchronous CLI command (it spawns - a ThreadPoolExecutor internally if an event loop is already running). - - Failures are reported both to the debug log and visibly to the user so - that a silent blocking clone followed by a silent empty list can never - happen. - """ - from ..lib.bundle_loader.resolvers import FoundationGitSource - - try: - FoundationGitSource(_ROUTING_BUNDLE_URI).resolve() - except Exception as e: - logger.warning("Could not fetch routing-matrix bundle: %s", e) - console.print(f"[yellow]Could not fetch routing-matrix bundle: {e}[/yellow]") - - def _discover_matrix_files() -> list[Path]: - """Discover available routing matrix YAML files. - - Looks in: - 1. ~/.amplifier/cache/amplifier-bundle-routing-matrix-*/routing/*.yaml (bundle) - 2. ~/.amplifier/routing/*.yaml (custom user matrices) - - On a clean install where the bundle is not yet cached, this lazily - fetches the routing-matrix bundle on first use. That makes `amplifier - routing list` work out of the box instead of silently returning an - empty list and telling the user to run a different command. + """Discover available routing matrix YAML files, fetching if needed. + + Thin wrapper around ``lib.routing_matrices.discover_matrix_files()`` + with ``fetch=True`` -- kept as a module-level name here for backward + compatibility with existing call sites in this module. The actual + filesystem-scanning + lazy-fetch implementation lives in + ``lib/routing_matrices.py`` so ``runtime/config.py`` can share it (via + ``known_matrix_names()``) without ever triggering the fetch. """ - home = Path.home() - files: list[Path] = [] - - # Bundle cache matrices (lazy-fetch on first use) - cache_base = home / ".amplifier" / "cache" - bundle_dirs = ( - list(cache_base.glob("amplifier-bundle-routing-matrix-*")) - if cache_base.exists() - else [] - ) - if not bundle_dirs: - # First run on a clean install — fetch the bundle with visible feedback. - # This can take 5-30s on a slow network so we must NOT block silently. - console.print("[dim]Fetching routing-matrix bundle...[/dim]") - _ensure_routing_bundle_cached() - bundle_dirs = ( - list(cache_base.glob("amplifier-bundle-routing-matrix-*")) - if cache_base.exists() - else [] - ) - - for bundle_dir in bundle_dirs: - routing_dir = bundle_dir / "routing" - if routing_dir.is_dir(): - files.extend(routing_dir.glob("*.yaml")) - - # Custom user matrices (single source of truth: get_custom_routing_dir()) - custom_dir = get_custom_routing_dir() - if custom_dir.is_dir(): - files.extend(custom_dir.glob("*.yaml")) - - return sorted(files) + return _discover_matrix_files_impl(fetch=True) def _load_matrix(path: Path) -> dict[str, Any] | None: @@ -355,9 +294,14 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st console.print("[yellow]No routing matrices found.[/yellow]") return - # Determine which matrix to show + # Determine which matrix to show. When no explicit name is given, this is + # the active matrix -- track where that selection came from (settings + # scope file, or the built-in default) so the non-detailed view below can + # show a "Source:" line. + source_path: str | None = None + auto_detected = matrix_name is None if matrix_name is None: - routing_config = settings.get_routing_config() + routing_config, source_path = settings.get_routing_config_with_source() matrix_name = routing_config.get("matrix", "balanced") if matrix_name not in matrices: @@ -381,14 +325,36 @@ def routing_show(matrix_name: str | None, compact: bool, detailed: bool, fmt: st if view == "detailed": _show_matrix_details(matrix_data, settings) else: - _show_matrix_resolution(matrix_data, settings) + _show_matrix_resolution( + matrix_data, settings, show_source=auto_detected, source_path=source_path + ) -def _show_matrix_resolution(matrix_data: dict[str, Any], settings: AppSettings) -> None: - """Display a role-by-role resolution table for a matrix.""" +def _show_matrix_resolution( + matrix_data: dict[str, Any], + settings: AppSettings, + *, + show_source: bool = False, + source_path: str | None = None, +) -> None: + """Display a role-by-role resolution table for a matrix. + + Args: + show_source: When True, print a "Source:" line above the table + identifying where the active-matrix selection came from (a + settings scope file, or "built-in default" when none set it). + Only ``amplifier routing show`` (with no explicit matrix name) + passes this -- other call sites (``routing use`` preview, + ``routing manage``) are unaffected. + source_path: The settings scope file path from + ``AppSettings.get_routing_config_with_source()``, or None. + """ matrix_name = matrix_data.get("name", "unknown") provider_types = _get_configured_provider_types(settings) + if show_source: + console.print(f"[dim]Source: {source_path or 'built-in default'}[/dim]") + roles = matrix_data.get("roles", {}) if not roles: console.print(f"[yellow]Matrix '{matrix_name}' has no roles defined.[/yellow]") diff --git a/amplifier_app_cli/lib/bundle_loader/prepare.py b/amplifier_app_cli/lib/bundle_loader/prepare.py index 2ad5fcfd..85dbdb2f 100644 --- a/amplifier_app_cli/lib/bundle_loader/prepare.py +++ b/amplifier_app_cli/lib/bundle_loader/prepare.py @@ -76,6 +76,8 @@ async def load_and_prepare_bundle( source_overrides: dict[str, str] | None = None, progress_callback: Callable[[str, str], None] | None = None, bundle_source_overrides: dict[str, str] | None = None, + required_behaviors: set[str] | None = None, + on_bundle_loaded: Callable[[Bundle], list[str]] | None = None, ) -> PreparedBundle: """Load bundle by name or URI and prepare it for execution. @@ -108,6 +110,22 @@ async def load_and_prepare_bundle( Keys are matched as substrings of include URIs. If matched, the override URI is used instead. Example: {"amplifier-bundle-superpowers": "/local/path"} + required_behaviors: Optional subset of ``compose_behaviors`` whose load + or composition failures must propagate. Other behavior failures + remain warnings for optional policies such as notifications. + on_bundle_loaded: Optional callback invoked with the freshly loaded + ``Bundle`` immediately after step 2 (load) -- BEFORE any + ``compose_behaviors`` (app-policy behaviors like modes, + notifications, routing) are composed onto it. This is the one + point where a caller can inspect bundle-declared defaults (e.g. + a bundle's own ``routing:`` section) and decide whether to + compose an additional behavior in response, without triggering + a second bundle load. The callback returns a list of additional + behavior URIs to compose (may be empty); these are appended to + ``compose_behaviors`` (de-duplicated) and to ``required_behaviors`` + for this call. Ordering matters: running this before app-policy + composition means an app-injected behavior can never masquerade + as a bundle default. Returns: PreparedBundle ready for create_session(). @@ -160,6 +178,13 @@ async def load_and_prepare_bundle( logger.info(f"Loading bundle '{bundle_name}' from {uri}") + # Normalize to mutable local collections so on_bundle_loaded() below can + # append additional behaviors/requirements without the caller needing to + # pre-size compose_behaviors/required_behaviors for a callback result it + # can't know in advance. + compose_behaviors = list(compose_behaviors) if compose_behaviors else [] + required_behaviors = set(required_behaviors) if required_behaviors else set() + if progress_callback: progress_callback("loading", bundle_name) @@ -177,6 +202,20 @@ async def load_and_prepare_bundle( bundle = await load_bundle(uri, registry=discovery.registry) logger.debug(f"Loaded bundle: {bundle.name} v{bundle.version}") + # 2b. Let the caller react to the freshly loaded bundle's OWN declared + # defaults (e.g. a bundle-declared routing matrix) before any app-policy + # behavior is composed onto it. Firing this here -- after includes are + # composed by load_bundle() but BEFORE compose_behaviors below -- means + # an app-injected behavior can never masquerade as a bundle default: the + # callback only ever sees what the bundle itself (plus its includes) + # declared. + if on_bundle_loaded: + extra_behaviors = on_bundle_loaded(bundle) or [] + for extra_uri in extra_behaviors: + if extra_uri not in compose_behaviors: + compose_behaviors.append(extra_uri) + required_behaviors.add(extra_uri) + # 3. Compose additional behavior bundles (app-level policies like notifications) if compose_behaviors: for behavior_uri in compose_behaviors: @@ -193,8 +232,10 @@ async def load_and_prepare_bundle( f"Composed behavior '{behavior_bundle.name}' onto '{bundle.name}'" ) except Exception as e: + if required_behaviors and behavior_uri in required_behaviors: + raise logger.warning(f"Failed to compose behavior '{behavior_uri}': {e}") - # Continue without this behavior - notifications are optional + # Continue without optional behaviors such as notifications. # 3b. Load agent metadata BEFORE prepare so the agent's declared modules # (tools/providers/hooks with `source:` URIs in their .md frontmatter) are diff --git a/amplifier_app_cli/lib/routing_matrices.py b/amplifier_app_cli/lib/routing_matrices.py new file mode 100644 index 00000000..beebfc5e --- /dev/null +++ b/amplifier_app_cli/lib/routing_matrices.py @@ -0,0 +1,139 @@ +"""Routing matrix file discovery. + +Single source of truth for locating routing matrix YAML files on disk. +Shared by: + - ``commands/routing.py`` (``amplifier routing list/show/use/...``) which + may lazily fetch the routing-matrix bundle on first use. + - ``runtime/config.py`` (session preparation), which validates a + bundle-declared ``routing.matrix`` name and must NEVER touch the + network on this hot path -- see ``known_matrix_names()``. + +Extracted from ``commands/routing.py``'s ``_discover_matrix_files()`` so +both call sites share one filesystem-scanning implementation instead of +risking the "listable but not loadable" bug this module's sibling, +``get_custom_routing_dir()`` (in ``lib/settings.py``), already guards +against for the custom-matrix directory. +""" + +from __future__ import annotations + +import logging +from pathlib import Path +from typing import Any + +import yaml +from rich.console import Console + +from .bundle_loader.discovery import WELL_KNOWN_BUNDLES +from .settings import get_custom_routing_dir + +console = Console() +logger = logging.getLogger(__name__) + +# Single source of truth for the routing-matrix bundle URL lives in +# WELL_KNOWN_BUNDLES (bundle_loader/discovery.py). +_ROUTING_BUNDLE_URI = str(WELL_KNOWN_BUNDLES["routing-matrix"]["remote"]) + + +def _ensure_routing_bundle_cached() -> None: + """Fetch the routing-matrix bundle into the cache if not yet present. + + Called lazily from ``discover_matrix_files(fetch=True)`` so `amplifier + routing list` works on a clean install without requiring the user to + run `amplifier update` first. FoundationGitSource.resolve() is a sync + wrapper that is safe to call from a synchronous CLI command (it spawns + a ThreadPoolExecutor internally if an event loop is already running). + + Failures are reported both to the debug log and visibly to the user so + that a silent blocking clone followed by a silent empty list can never + happen. + """ + from .bundle_loader.resolvers import FoundationGitSource + + try: + FoundationGitSource(_ROUTING_BUNDLE_URI).resolve() + except Exception as e: + logger.warning("Could not fetch routing-matrix bundle: %s", e) + console.print(f"[yellow]Could not fetch routing-matrix bundle: {e}[/yellow]") + + +def discover_matrix_files(fetch: bool = False) -> list[Path]: + """Discover available routing matrix YAML files. + + Looks in: + 1. ~/.amplifier/cache/amplifier-bundle-routing-matrix-*/routing/*.yaml (bundle) + 2. ~/.amplifier/routing/*.yaml (custom user matrices) + + Args: + fetch: When True and the routing-matrix bundle is not yet cached, + lazily fetch it (network I/O; prints progress/failure feedback + via the console). This is what makes `amplifier routing list` + work out of the box on a clean install. When False (the + default), this function NEVER touches the network -- callers + on a session-start hot path (see ``known_matrix_names()``) + must not silently block on git I/O just to validate a name. + """ + home = Path.home() + files: list[Path] = [] + + # Bundle cache matrices (lazy-fetch on first use, only when fetch=True) + cache_base = home / ".amplifier" / "cache" + bundle_dirs = ( + list(cache_base.glob("amplifier-bundle-routing-matrix-*")) + if cache_base.exists() + else [] + ) + if not bundle_dirs and fetch: + # First run on a clean install -- fetch the bundle with visible + # feedback. This can take 5-30s on a slow network so we must NOT + # block silently. + console.print("[dim]Fetching routing-matrix bundle...[/dim]") + _ensure_routing_bundle_cached() + bundle_dirs = ( + list(cache_base.glob("amplifier-bundle-routing-matrix-*")) + if cache_base.exists() + else [] + ) + + for bundle_dir in bundle_dirs: + routing_dir = bundle_dir / "routing" + if routing_dir.is_dir(): + files.extend(routing_dir.glob("*.yaml")) + + # Custom user matrices (single source of truth: get_custom_routing_dir()) + custom_dir = get_custom_routing_dir() + if custom_dir.is_dir(): + files.extend(custom_dir.glob("*.yaml")) + + return sorted(files) + + +def known_matrix_names() -> set[str]: + """Return the set of matrix names discoverable on disk, WITHOUT fetching. + + Used at session-start (``runtime/config.py``) to validate a + bundle-declared ``routing.matrix`` name before letting it win as the + effective default. Never touches the network -- if the routing-matrix + bundle isn't cached yet (clean install, no prior `amplifier routing + list`/`update`), this simply returns an empty set. Callers treat an + empty set as "nothing to validate against" and skip the check, rather + than treating "unknown on an uncached install" the same as "genuinely + unknown name". + """ + names: set[str] = set() + for path in discover_matrix_files(fetch=False): + try: + with open(path, encoding="utf-8") as f: + data: dict[str, Any] = yaml.safe_load(f) or {} + except Exception: + continue + name = data.get("name") + if name: + names.add(str(name)) + return names + + +__all__ = [ + "discover_matrix_files", + "known_matrix_names", +] diff --git a/amplifier_app_cli/lib/settings.py b/amplifier_app_cli/lib/settings.py index 59411010..1e043186 100644 --- a/amplifier_app_cli/lib/settings.py +++ b/amplifier_app_cli/lib/settings.py @@ -560,10 +560,57 @@ def get_routing_config(self) -> dict[str, Any]: matrix: balanced overrides: coding: special-config + + Thin wrapper around get_routing_config_with_source() -- kept so + existing callers (which only care about the merged config, not + which scope set it) don't need to change. + """ + routing, _source = self.get_routing_config_with_source() + return routing + + def get_routing_config_with_source(self) -> tuple[dict[str, Any], str | None]: + """Return (merged routing config, source) for the routing: section. + + ``source`` is the path of the highest-precedence scope file that + sets ``routing.matrix`` specifically, or ``None`` if no scope sets + it (e.g. only a bundle-declared default applies, or routing.matrix + is unset everywhere). + + Reuses the same scope list and order as get_merged_settings() + (global -> project -> local -> session, most specific wins). The + merged config is exactly what get_routing_config() has always + returned. Separately, this walks the same scopes to find the LAST + (i.e. highest-precedence) scope file whose YAML actually contains + a ``routing.matrix`` key -- callers use this to attribute "who set + the active matrix" in user-facing messages (see + runtime/config.py's bundle-declared routing matrix precedence). """ merged = self.get_merged_settings() routing = merged.get("routing", {}) - return routing if isinstance(routing, dict) else {} + routing = routing if isinstance(routing, dict) else {} + + source: str | None = None + paths_to_check = [ + self.paths.global_settings, + self.paths.project_settings, + self.paths.local_settings, + ] + if self.paths.session_settings: + paths_to_check.append(self.paths.session_settings) + + for path in paths_to_check: + if not path.exists(): + continue + try: + with open(path, encoding="utf-8") as f: + content = yaml.safe_load(f) or {} + except Exception: + continue # Skip malformed files, same as get_merged_settings() + scope_routing = content.get("routing") + if isinstance(scope_routing, dict) and "matrix" in scope_routing: + source = str(path) + + return routing, source def set_routing_matrix(self, matrix_name: str, scope: Scope = "global") -> None: """Write routing: {matrix: } to settings at specified scope. diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 17dfdfba..4dcb8c51 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -16,6 +16,7 @@ from ..lib.merge_utils import merge_module_items from ..lib.merge_utils import merge_tool_configs from ..lib.merge_utils import _normalize_module_entry +from ..lib.routing_matrices import known_matrix_names if TYPE_CHECKING: @@ -94,8 +95,29 @@ def _on_progress(action: str, detail: str) -> None: _build_notification_behaviors(app_settings.get_notification_flags()) ) + # Routing precedence, weakest -> strongest: built-in default < + # bundle-declared routing.matrix < global settings < project settings + # < project-local settings. ``user_routing`` is already the merge of + # every settings scope (global -> project -> local -> session), so it + # sits above any bundle default by construction -- the bundle only + # contributes when user_routing is empty (see on_bundle_loaded() + # below). ``user_source`` is the highest-precedence settings file + # that set routing.matrix specifically, used for the "user setting + # overrides bundle default" warning below. + user_routing, user_source = app_settings.get_routing_config_with_source() + + # Routing is required when active: compose its canonical behavior before + # prepare() so hooks-routing and its source are available to all sessions. + # This decision is made from user_routing alone, so existing users (who + # already set routing.matrix themselves) see byte-identical behavior. + # The bundle-declared-default path is handled separately by + # on_bundle_loaded() below, which composes the SAME behavior only when + # the bundle turns out to be the sole source of routing. + routing_behaviors = _build_routing_behaviors(user_routing) + compose_behaviors.extend(routing_behaviors) + # Add app bundles (user-configured bundles that are always composed) - # App bundles are explicit user configuration, composed AFTER notification behaviors + # App bundles are explicit user configuration, composed after app policies. app_bundles = app_settings.get_app_bundles() if app_bundles: compose_behaviors = compose_behaviors + app_bundles @@ -126,6 +148,57 @@ def _on_progress(action: str, detail: str) -> None: # Get bundle source overrides from settings (sources.bundles in settings.yaml) bundle_sources = app_settings.get_bundle_sources() + # Bundle-declared routing default (a top-level ``routing:`` section in + # bundle frontmatter, deep-merged onto ``Bundle.routing`` by + # foundation on compose). Captured by _on_bundle_loaded() below, which + # fires right after the primary bundle (and its includes) load but + # BEFORE any app-policy behavior (modes/notify/routing) is composed + # onto it -- so an app-injected behavior can never masquerade as a + # bundle default. ``bundle_routing_state`` is populated as a side + # effect for the observability messages printed after the spinner + # stops, further down. + bundle_routing_state: dict[str, Any] = { + "routing": {}, + "bundle_name": None, + "unknown_matrix": None, + } + + def _on_bundle_loaded(loaded_bundle: Any) -> list[str]: + """Read the bundle's own routing default and decide composition. + + Forward-compat: reads ``routing`` via getattr() with a ``{}`` + default so this works whether or not the installed + amplifier-foundation declares a ``routing`` field on ``Bundle`` + yet. Returns additional behavior URIs for the caller to compose + (empty when there's nothing new to add). + """ + raw_routing = getattr(loaded_bundle, "routing", None) or {} + declared_routing = ( + dict(raw_routing) if isinstance(raw_routing, dict) else {} + ) + bundle_routing_state["bundle_name"] = getattr(loaded_bundle, "name", None) + + # Unknown-matrix handling applies ONLY when the bundle's matrix is + # about to win (no user-set matrix anywhere in settings). A + # user-set matrix is validated by the existing hooks-routing + # "matrix file not found" path at runtime, unchanged. + matrix_name = declared_routing.get("matrix") + if matrix_name and not user_routing.get("matrix"): + known = known_matrix_names() + if known and matrix_name not in known: + declared_routing = { + k: v for k, v in declared_routing.items() if k != "matrix" + } + bundle_routing_state["unknown_matrix"] = matrix_name + + bundle_routing_state["routing"] = declared_routing + + if user_routing or not declared_routing: + # User settings already trigger composition above (routing_behaviors), + # or the bundle has nothing left to contribute after validation. + return [] + return _build_routing_behaviors(declared_routing) + # Load and prepare bundle (downloads modules from git sources) # If compose_behaviors is provided, those behaviors are composed onto the bundle # BEFORE prepare() runs, so their modules get installed correctly @@ -134,9 +207,11 @@ def _on_progress(action: str, detail: str) -> None: bundle_name, discovery, compose_behaviors=compose_behaviors if compose_behaviors else None, + required_behaviors=set(routing_behaviors) if routing_behaviors else None, source_overrides=combined_sources if combined_sources else None, bundle_source_overrides=bundle_sources if bundle_sources else None, progress_callback=_on_progress if status else None, + on_bundle_loaded=_on_bundle_loaded, ) # Load full agent metadata from .md files (for descriptions) @@ -149,6 +224,22 @@ def _on_progress(action: str, detail: str) -> None: if status: status.stop() + # Effective routing config: bundle default merged UNDER user settings. + # user_routing is already the settings-scope merge result, so a plain + # shallow merge here is correct -- user keys win key-by-key over the + # bundle's declared defaults (see _on_bundle_loaded() above). + effective_routing: dict[str, Any] = { + **bundle_routing_state["routing"], + **user_routing, + } + + # Bundle-declared routing observability -- printed only after the + # spinner stops (matches the "prepared successfully" message below), + # and only when there's something worth telling the user about. + _report_bundle_routing_observability( + console, bundle_routing_state, user_routing, user_source + ) + # ── General config overrides ────────────────────────────────────────── # The overrides..config section in settings.yaml provides a single # consistent path for overriding ANY module's config — providers, tools, @@ -231,17 +322,21 @@ def _on_progress(action: str, detail: str) -> None: # This maps config.notifications.ntfy.* to hooks-notify-push config etc. hook_overrides = app_settings.get_notification_hook_overrides() - # Routing matrix config injection - routing_config = app_settings.get_routing_config() - if routing_config: + # Routing matrix config injection (effective_routing = bundle default + # merged under user settings -- see precedence comment above). + if effective_routing: routing_hook_override: dict[str, Any] = { "module": "hooks-routing", "config": {}, } - if "matrix" in routing_config: - routing_hook_override["config"]["default_matrix"] = routing_config["matrix"] - if "overrides" in routing_config: - routing_hook_override["config"]["overrides"] = routing_config["overrides"] + if "matrix" in effective_routing: + routing_hook_override["config"]["default_matrix"] = effective_routing[ + "matrix" + ] + if "overrides" in effective_routing: + routing_hook_override["config"]["overrides"] = effective_routing[ + "overrides" + ] # Always advertise the user's custom routing dir so a matrix named by # routing.matrix that ONLY exists at get_custom_routing_dir() (e.g. # written by `amplifier init`/`amplifier routing save`) is resolvable @@ -315,13 +410,64 @@ def _on_progress(action: str, detail: str) -> None: prepared, bundle_config, sync_tools=bool(bundle_config.get("tools")) ) - # Note: Notification hooks are now composed via compose_behaviors parameter - # to load_and_prepare_bundle(), so they get properly installed during prepare(). + # Note: Notification and routing hooks are composed via compose_behaviors + # before prepare(), so they get properly installed during preparation. # The behavior bundles handle root-session-only logic internally via parent_id check. return bundle_config, prepared +def _report_bundle_routing_observability( + console: Console | None, + bundle_routing_state: dict[str, Any], + user_routing: dict[str, Any], + user_source: str | None, +) -> None: + """Print bundle-declared routing precedence outcomes, if any. + + Silent when there's nothing to report (no bundle default declared, no + unknown-matrix situation, and no conflict between a bundle default and a + user setting). Uses the same ``console`` the caller uses for its other + status messages -- a ``None`` console (non-interactive / programmatic + callers) means these are simply skipped. + """ + if console is None: + return + + bundle_name = bundle_routing_state.get("bundle_name") or "unknown" + bundle_routing = bundle_routing_state.get("routing") or {} + unknown_matrix = bundle_routing_state.get("unknown_matrix") + bundle_matrix = bundle_routing.get("matrix") + user_matrix = user_routing.get("matrix") + + if unknown_matrix: + fallback = user_matrix or "no routing" + console.print( + f"[yellow]Bundle '{bundle_name}' requests routing matrix " + f"'{unknown_matrix}', which is not installed.[/yellow]\n" + " [dim]Searched: ~/.amplifier/routing, " + "~/.amplifier/cache/amplifier-bundle-routing-matrix-*/routing[/dim]\n" + f" [dim]Falling back to: {fallback}[/dim]" + ) + return + + if bundle_matrix and user_matrix and user_source and user_matrix != bundle_matrix: + console.print( + f"[yellow]Routing matrix: '{user_matrix}' from {user_source}[/yellow]\n" + f" [yellow]overrides bundle '{bundle_name}' default " + f"'{bundle_matrix}'.[/yellow]\n" + f" [dim]Change it there, or run: amplifier routing use " + f"{bundle_matrix}[/dim]" + ) + return + + if bundle_matrix and not user_matrix: + console.print( + f"[dim]Routing matrix: '{bundle_matrix}' " + f"(default from bundle '{bundle_name}')[/dim]" + ) + + def _sync_overrides_to_bundle( prepared: "PreparedBundle", bundle_config: dict[str, Any], @@ -897,6 +1043,24 @@ def _build_modes_behaviors() -> list[str]: ] +def _build_routing_behaviors(routing_config: dict[str, Any]) -> list[str]: + """Return the canonical routing behavior URI when routing is active. + + Composing this app-level policy before preparation makes ``hooks-routing`` + and its module source available in the prepared resolver for root and + delegated sessions. + """ + if not routing_config: + return [] + + return [ + ( + "git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main" + "#subdirectory=behaviors/routing.yaml" + ) + ] + + def _build_notification_behaviors(flags: NotificationFlags) -> list[str]: """Build list of notification behavior URIs based on resolved flags. diff --git a/tests/lib/bundle_loader/test_prepare.py b/tests/lib/bundle_loader/test_prepare.py index 1875c3f2..a7d5f941 100644 --- a/tests/lib/bundle_loader/test_prepare.py +++ b/tests/lib/bundle_loader/test_prepare.py @@ -241,3 +241,100 @@ async def test_no_bundle_overrides_skips_resolver(self): # set_include_source_resolver was NOT called mock_registry.set_include_source_resolver.assert_not_called() + + +class TestLoadAndPrepareBundleRequiredBehaviors: + """Required behavior failures must not be hidden by the loader.""" + + @pytest.mark.asyncio + async def test_required_behavior_load_failure_propagates(self): + """A routing behavior load error aborts before bundle preparation.""" + from amplifier_app_cli.lib.bundle_loader.prepare import load_and_prepare_bundle + + routing_uri = ( + "git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main" + "#subdirectory=behaviors/routing.yaml" + ) + mock_discovery = MagicMock() + mock_discovery.find.return_value = "file:///path/to/bundle.yaml" + mock_bundle = MagicMock() + mock_bundle.prepare = AsyncMock() + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_bundle", + new_callable=AsyncMock, + side_effect=[ + mock_bundle, + RuntimeError("hooks-routing module unavailable"), + ], + ), + pytest.raises(RuntimeError, match="hooks-routing module unavailable"), + ): + await load_and_prepare_bundle( + "my-bundle", + mock_discovery, + compose_behaviors=[routing_uri], + required_behaviors={routing_uri}, + ) + + mock_bundle.prepare.assert_not_awaited() + + @pytest.mark.asyncio + async def test_required_behavior_compose_failure_propagates(self): + """A routing behavior composition error aborts before preparation.""" + from amplifier_app_cli.lib.bundle_loader.prepare import load_and_prepare_bundle + + routing_uri = "file:///routing.yaml" + mock_discovery = MagicMock() + mock_discovery.find.return_value = "file:///path/to/bundle.yaml" + mock_bundle = MagicMock() + mock_bundle.compose.side_effect = RuntimeError("routing compose failed") + mock_bundle.prepare = AsyncMock() + behavior_bundle = MagicMock() + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_bundle", + new_callable=AsyncMock, + side_effect=[mock_bundle, behavior_bundle], + ), + pytest.raises(RuntimeError, match="routing compose failed"), + ): + await load_and_prepare_bundle( + "my-bundle", + mock_discovery, + compose_behaviors=[routing_uri], + required_behaviors={routing_uri}, + ) + + mock_bundle.prepare.assert_not_awaited() + + @pytest.mark.asyncio + async def test_optional_behavior_load_failure_is_still_ignored(self): + """Optional notification behavior failures remain warning-and-continue.""" + from amplifier_app_cli.lib.bundle_loader.prepare import load_and_prepare_bundle + + optional_uri = "file:///optional-notifications.yaml" + mock_discovery = MagicMock() + mock_discovery.find.return_value = "file:///path/to/bundle.yaml" + mock_bundle = MagicMock() + mock_prepared = MagicMock() + mock_bundle.prepare = AsyncMock(return_value=mock_prepared) + + with patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_bundle", + new_callable=AsyncMock, + side_effect=[ + mock_bundle, + RuntimeError("optional behavior unavailable"), + ], + ): + result = await load_and_prepare_bundle( + "my-bundle", + mock_discovery, + compose_behaviors=[optional_uri], + ) + + assert result is mock_prepared + mock_bundle.prepare.assert_awaited_once() diff --git a/tests/test_bundle_routing_matrix.py b/tests/test_bundle_routing_matrix.py new file mode 100644 index 00000000..58531ca8 --- /dev/null +++ b/tests/test_bundle_routing_matrix.py @@ -0,0 +1,372 @@ +"""Tests for bundle-declared routing matrix defaults. + +A bundle may declare a default routing matrix (a top-level ``routing:`` +section in its frontmatter, exposed as ``Bundle.routing`` by +amplifier-foundation). This is consumed by ``runtime/config.py`` as the +WEAKEST source in the routing precedence chain: + + built-in default < bundle-declared routing.matrix + < user ~/.amplifier/settings.yaml < project .amplifier/settings.yaml + < project .amplifier/settings.local.yaml + +These tests exercise that precedence through the real +``resolve_bundle_config()`` function. ``load_and_prepare_bundle()`` itself is +mocked out (it has its own dedicated tests in +``tests/lib/bundle_loader/test_prepare.py`` covering the +``on_bundle_loaded``/``required_behaviors`` plumbing) -- the mock's +``side_effect`` simulates the ONE thing these tests care about: invoking the +``on_bundle_loaded`` callback with a stub ``Bundle``, exactly as the real +``load_and_prepare_bundle()`` does right after loading it. +""" + +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from amplifier_app_cli.lib.settings import AppSettings, NotificationFlags, SettingsPaths +from amplifier_app_cli.runtime.config import resolve_bundle_config + +ROUTING_URI = ( + "git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main" + "#subdirectory=behaviors/routing.yaml" +) + + +# --------------------------------------------------------------------------- # +# Helpers +# --------------------------------------------------------------------------- # + + +def _make_app_settings( + *, + routing_config: dict[str, Any] | None = None, + routing_source: str | None = None, + **kwargs: Any, +) -> MagicMock: + """Build a mock AppSettings with controlled routing (and other) config.""" + settings = MagicMock() + settings.get_config_overrides.return_value = kwargs.get("config_overrides", {}) + settings.get_provider_overrides.return_value = kwargs.get("provider_overrides", []) + settings.get_tool_overrides.return_value = kwargs.get("tool_overrides", []) + settings.get_notification_hook_overrides.return_value = kwargs.get( + "hook_overrides", [] + ) + routing_config = routing_config or {} + settings.get_routing_config.return_value = routing_config + settings.get_routing_config_with_source.return_value = ( + routing_config, + routing_source, + ) + settings.get_notification_flags.return_value = NotificationFlags( + desktop_enabled=False, + push_enabled=False, + ) + settings.get_app_bundles.return_value = [] + settings.get_source_overrides.return_value = {} + settings.get_module_sources.return_value = {} + settings.get_bundle_sources.return_value = {} + return settings + + +def _real_app_settings(tmp_path: Path) -> AppSettings: + """Build a REAL AppSettings backed by tmp_path scope files. + + Needed for tests that must attribute a warning to a genuine settings + scope file path (e.g. .amplifier/settings.local.yaml). + """ + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + return AppSettings(paths=paths) + + +def _write_yaml(path: Path, data: dict) -> None: + import yaml + + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "w", encoding="utf-8") as f: + yaml.safe_dump(data, f) + + +def _fake_prepare(mount_plan: dict[str, Any], bundle_stub: Any | None): + """Build an AsyncMock-compatible side_effect for load_and_prepare_bundle. + + Simulates the ONE behavior these tests need from the real function: + invoking ``on_bundle_loaded(bundle_stub)`` right after "loading" the + bundle. Everything else (required_behaviors bookkeeping inside + prepare.py, actual git/network I/O) is out of scope here -- see + tests/lib/bundle_loader/test_prepare.py for that. + + Returns (side_effect_coroutine_fn, captured) where captured accumulates + {"kwargs": ..., "callback_result": ...} after the call. + """ + mock_prepared = MagicMock() + mock_prepared.mount_plan = mount_plan + mock_prepared.bundle.load_agent_metadata = MagicMock() + + captured: dict[str, Any] = {} + + async def _prepare(*_args: Any, **kwargs: Any) -> Any: + captured["kwargs"] = kwargs + on_loaded = kwargs.get("on_bundle_loaded") + if on_loaded is not None and bundle_stub is not None: + captured["callback_result"] = on_loaded(bundle_stub) + return mock_prepared + + return _prepare, captured + + +def _printed_text(console_mock: MagicMock) -> str: + """Flatten all console.print() call args into one searchable string.""" + parts = [] + for call in console_mock.print.call_args_list: + if call.args: + parts.append(str(call.args[0])) + return "\n".join(parts) + + +async def _run( + settings: AppSettings, + bundle_stub: Any | None, + mount_plan: dict[str, Any] | None = None, + known_matrices: set[str] | None = None, +) -> tuple[dict[str, Any], dict[str, Any], MagicMock]: + """Run resolve_bundle_config() with the fake prepare + patched collaborators. + + Returns (result_config, captured, console_mock). + """ + prepare_fn, captured = _fake_prepare(mount_plan or {"hooks": []}, bundle_stub) + console = MagicMock() + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + AsyncMock(side_effect=prepare_fn), + ), + patch("amplifier_app_cli.paths.get_bundle_search_paths", return_value=[]), + patch("amplifier_app_cli.lib.bundle_loader.AppBundleDiscovery"), + patch( + "amplifier_app_cli.runtime.config.known_matrix_names", + return_value=known_matrices if known_matrices is not None else set(), + ), + ): + result, _ = await resolve_bundle_config( + bundle_name="test", app_settings=settings, console=console + ) + + return result, captured, console + + +# --------------------------------------------------------------------------- # +# Tests +# --------------------------------------------------------------------------- # + + +@pytest.mark.asyncio +async def test_bundle_without_routing_behaves_identically_to_today(): + """MOST IMPORTANT: a bundle with no routing default -> byte-identical + to pre-change behavior. No routing behavior composed, no hooks-routing + entry injected, no observability output.""" + settings = _make_app_settings() # no user routing config + bundle_stub = SimpleNamespace(name="test-bundle", routing={}) + + result, captured, console = await _run(settings, bundle_stub) + + compose_behaviors = captured["kwargs"]["compose_behaviors"] + assert all( + "amplifier-bundle-routing-matrix" not in uri for uri in compose_behaviors + ) + assert captured["kwargs"]["required_behaviors"] is None + assert captured["callback_result"] == [] + assert result["hooks"] == [] + assert _printed_text(console) == "" or "Routing matrix" not in _printed_text( + console + ) + + +@pytest.mark.asyncio +async def test_bundle_routing_used_when_no_user_setting(): + """No user setting anywhere -> the bundle's declared matrix wins as the + default, and the callback requests the canonical routing behavior.""" + settings = _make_app_settings() # user_routing == {} + bundle_stub = SimpleNamespace(name="my-bundle", routing={"matrix": "quality"}) + + result, captured, console = await _run( + settings, bundle_stub, known_matrices={"quality", "balanced"} + ) + + assert captured["callback_result"] == [ROUTING_URI] + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + assert len(routing_entries) == 1 + assert routing_entries[0]["config"]["default_matrix"] == "quality" + + printed = _printed_text(console) + assert "quality" in printed + assert "default from bundle 'my-bundle'" in printed + + +@pytest.mark.asyncio +async def test_user_settings_matrix_beats_bundle_routing(): + """A user setting (any scope) always wins over the bundle default.""" + settings = _make_app_settings( + routing_config={"matrix": "anthropic"}, + routing_source="/home/u/.amplifier/settings.yaml", + ) + bundle_stub = SimpleNamespace(name="my-bundle", routing={"matrix": "openai"}) + + result, captured, console = await _run( + settings, bundle_stub, known_matrices={"anthropic", "openai"} + ) + + # User settings already trigger composition/require independent of the + # bundle -- the callback must not add a second, redundant behavior entry + # (dedup is prepare.py's job; here we assert the callback's own decision). + assert captured["callback_result"] == [] + + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + assert len(routing_entries) == 1 + assert routing_entries[0]["config"]["default_matrix"] == "anthropic" + + printed = _printed_text(console) + assert "anthropic" in printed + assert "/home/u/.amplifier/settings.yaml" in printed + assert "overrides bundle 'my-bundle' default 'openai'" in printed + assert "amplifier routing use openai" in printed + + +@pytest.mark.asyncio +async def test_project_local_settings_matrix_beats_bundle_routing(tmp_path: Path): + """Project-local settings.local.yaml (highest precedence scope) beats a + bundle default, and the warning names that exact file path.""" + settings = _real_app_settings(tmp_path) + _write_yaml(settings.paths.local_settings, {"routing": {"matrix": "anthropic"}}) + bundle_stub = SimpleNamespace(name="my-bundle", routing={"matrix": "openai"}) + + result, _captured, console = await _run( + settings, bundle_stub, known_matrices={"anthropic", "openai"} + ) + + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + assert routing_entries[0]["config"]["default_matrix"] == "anthropic" + + printed = _printed_text(console) + assert str(settings.paths.local_settings) in printed + assert "overrides bundle 'my-bundle' default 'openai'" in printed + + +@pytest.mark.asyncio +async def test_bundle_overrides_merge_under_user_overrides(): + """Shallow merge at the top level: user_routing keys win key-by-key over + bundle_routing -- NOT a deep per-role merge of `overrides`.""" + settings = _make_app_settings( + routing_config={"overrides": {"docs": "bar"}}, + ) + bundle_stub = SimpleNamespace( + name="my-bundle", + routing={"matrix": "quality", "overrides": {"coding": "foo"}}, + ) + + result, _captured, _console = await _run( + settings, bundle_stub, known_matrices={"quality"} + ) + + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + cfg = routing_entries[0]["config"] + # matrix: only the bundle set it -> bundle value survives + assert cfg["default_matrix"] == "quality" + # overrides: user_routing HAS the "overrides" key -> it wins wholesale, + # bundle's {"coding": "foo"} is NOT deep-merged in. + assert cfg["overrides"] == {"docs": "bar"} + + +@pytest.mark.asyncio +async def test_unknown_bundle_matrix_is_dropped_and_warns(): + """Bundle's matrix isn't a known/installed matrix -> dropped, warns, + session is not bricked (falls back to no routing).""" + settings = _make_app_settings() # no user matrix + bundle_stub = SimpleNamespace(name="my-bundle", routing={"matrix": "foo"}) + + result, captured, console = await _run( + settings, bundle_stub, known_matrices={"balanced", "quality"} + ) + + # Matrix key dropped -> nothing left to contribute -> no behavior, no hook. + assert captured["callback_result"] == [] + assert result["hooks"] == [] + + printed = _printed_text(console) + assert "Bundle 'my-bundle' requests routing matrix 'foo'" in printed + assert "not installed" in printed + assert "~/.amplifier/routing" in printed + assert "amplifier-bundle-routing-matrix-*/routing" in printed + assert "Falling back to: no routing" in printed + + +@pytest.mark.asyncio +async def test_unknown_bundle_matrix_does_not_disable_user_matrix(): + """Unknown-matrix validation applies ONLY when the bundle's matrix is + about to win. When the user has already set a matrix, the bundle's + (invalid) matrix is never checked, and the user's matrix keeps working.""" + settings = _make_app_settings( + routing_config={"matrix": "anthropic"}, + routing_source="/home/u/.amplifier/settings.yaml", + ) + bundle_stub = SimpleNamespace(name="my-bundle", routing={"matrix": "foo"}) + + # known_matrices deliberately does NOT include "foo" -- if the unknown + # check ran anyway, it still must not affect the user's own matrix. + result, _captured, console = await _run( + settings, bundle_stub, known_matrices={"anthropic"} + ) + + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + assert routing_entries[0]["config"]["default_matrix"] == "anthropic" + + printed = _printed_text(console) + assert "not installed" not in printed # unknown-matrix check never ran + + +@pytest.mark.asyncio +async def test_composed_bundles_overlay_matrix_wins(): + """foundation deep-merges routing: across composed includes before + app-cli ever sees Bundle.routing -- app-cli just takes whatever the + final composed value is at face value.""" + settings = _make_app_settings() # no user matrix + # Simulates the state AFTER foundation has already composed multiple + # bundles' routing: sections -- the overlay bundle's matrix is what + # survives onto the final Bundle.routing. + bundle_stub = SimpleNamespace(name="overlay-bundle", routing={"matrix": "overlay"}) + + result, _captured, _console = await _run( + settings, bundle_stub, known_matrices={"overlay", "base"} + ) + + routing_entries = [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + assert routing_entries[0]["config"]["default_matrix"] == "overlay" + + +@pytest.mark.asyncio +async def test_old_foundation_without_routing_attr_does_not_crash(): + """Forward-compat: a Bundle stub with NO `routing` attribute at all + (the CURRENTLY installed amplifier-foundation) must not crash -- the + getattr(bundle, "routing", {}) default path is what makes this work.""" + settings = _make_app_settings() + + class _OldBundleStub: + def __init__(self, name: str) -> None: + self.name = name + # Deliberately no `.routing` attribute. + + bundle_stub = _OldBundleStub("old-bundle") + + result, captured, _console = await _run(settings, bundle_stub) + + assert captured["callback_result"] == [] + assert result["hooks"] == [] diff --git a/tests/test_general_config_overrides.py b/tests/test_general_config_overrides.py index c6b79abe..a5157b3d 100644 --- a/tests/test_general_config_overrides.py +++ b/tests/test_general_config_overrides.py @@ -5,10 +5,12 @@ """ from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch import pytest -from unittest.mock import AsyncMock, MagicMock, patch + from amplifier_app_cli.lib.merge_utils import deep_merge +from amplifier_app_cli.lib.settings import NotificationFlags from amplifier_app_cli.runtime.config import ( _apply_hook_overrides, _apply_provider_overrides, @@ -16,7 +18,6 @@ resolve_bundle_config, ) - # ═══════════════════════════════════════════════════════════════════════════ # PART 1: Direct logic tests — exercise the exact code path added in the fix # ═══════════════════════════════════════════════════════════════════════════ @@ -317,7 +318,17 @@ def _make_app_settings(config_overrides=None, **kwargs): settings.get_notification_hook_overrides.return_value = kwargs.get( "hook_overrides", [] ) - settings.get_routing_config.return_value = kwargs.get("routing_config", None) + routing_config = kwargs.get("routing_config", None) + settings.get_routing_config.return_value = routing_config + settings.get_routing_config_with_source.return_value = ( + routing_config or {}, + kwargs.get("routing_source", None), + ) + settings.get_notification_flags.return_value = NotificationFlags( + desktop_enabled=False, + push_enabled=False, + ) + settings.get_app_bundles.return_value = [] settings.get_source_overrides.return_value = {} settings.get_module_sources.return_value = {} settings.get_bundle_sources.return_value = {} @@ -331,6 +342,99 @@ class TestFullPipelineIntegration: at their SOURCE modules since they're imported inside the function body. """ + @pytest.mark.asyncio + async def test_active_routing_composed_before_prepare(self): + """Active routing is composed and required before preparation.""" + mock_prepared = MagicMock() + mock_prepared.mount_plan = {"hooks": []} + mock_prepared.bundle.load_agent_metadata = MagicMock() + settings = _make_app_settings( + routing_config={ + "matrix": "balanced", + "overrides": {"coding": "quality"}, + } + ) + prepare = AsyncMock(return_value=mock_prepared) + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + prepare, + ), + patch("amplifier_app_cli.paths.get_bundle_search_paths", return_value=[]), + patch("amplifier_app_cli.lib.bundle_loader.AppBundleDiscovery"), + ): + result, _ = await resolve_bundle_config( + bundle_name="test", app_settings=settings + ) + + assert prepare.await_args is not None + compose_behaviors = prepare.await_args.kwargs["compose_behaviors"] + required_behaviors = prepare.await_args.kwargs["required_behaviors"] + routing_uri = ( + "git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main" + "#subdirectory=behaviors/routing.yaml" + ) + assert routing_uri in compose_behaviors + assert required_behaviors == {routing_uri} + assert required_behaviors <= set(compose_behaviors) + assert result["hooks"][0]["config"]["default_matrix"] == "balanced" + assert result["hooks"][0]["config"]["overrides"] == {"coding": "quality"} + settings.get_routing_config_with_source.assert_called_once_with() + + @pytest.mark.asyncio + async def test_inactive_routing_not_composed_before_prepare(self): + """Inactive routing is neither composed nor marked required.""" + mock_prepared = MagicMock() + mock_prepared.mount_plan = {"hooks": []} + mock_prepared.bundle.load_agent_metadata = MagicMock() + settings = _make_app_settings(routing_config={}) + prepare = AsyncMock(return_value=mock_prepared) + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + prepare, + ), + patch("amplifier_app_cli.paths.get_bundle_search_paths", return_value=[]), + patch("amplifier_app_cli.lib.bundle_loader.AppBundleDiscovery"), + ): + result, _ = await resolve_bundle_config( + bundle_name="test", app_settings=settings + ) + + assert prepare.await_args is not None + compose_behaviors = prepare.await_args.kwargs["compose_behaviors"] + assert all( + "amplifier-bundle-routing-matrix" not in uri for uri in compose_behaviors + ) + assert prepare.await_args.kwargs["required_behaviors"] is None + assert result["hooks"] == [] + settings.get_routing_config_with_source.assert_called_once_with() + + @pytest.mark.asyncio + async def test_routing_preparation_error_propagates(self): + """Preparation errors remain fatal when routing is active.""" + settings = _make_app_settings(routing_config={"matrix": "balanced"}) + prepare = AsyncMock(side_effect=RuntimeError("hooks-routing unavailable")) + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + prepare, + ), + patch("amplifier_app_cli.paths.get_bundle_search_paths", return_value=[]), + patch("amplifier_app_cli.lib.bundle_loader.AppBundleDiscovery"), + pytest.raises(RuntimeError, match="hooks-routing unavailable"), + ): + await resolve_bundle_config(bundle_name="test", app_settings=settings) + + assert prepare.await_args is not None + assert any( + "amplifier-bundle-routing-matrix" in uri + for uri in prepare.await_args.kwargs["compose_behaviors"] + ) + @pytest.mark.asyncio async def test_hook_override_flows_through_full_pipeline(self): """Config override for a hook reaches final bundle_config.""" diff --git a/tests/test_routing_matrix_registration.py b/tests/test_routing_matrix_registration.py index c2f4dd17..22ba6551 100644 --- a/tests/test_routing_matrix_registration.py +++ b/tests/test_routing_matrix_registration.py @@ -22,7 +22,6 @@ from amplifier_app_cli.lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES - # --------------------------------------------------------------------------- # Bug 3(a) — WELL_KNOWN_BUNDLES registration # --------------------------------------------------------------------------- @@ -56,33 +55,38 @@ def test_routing_matrix_registration_has_required_fields(): def test_discover_matrix_files_triggers_fetch_on_empty_cache(tmp_path): - """When the bundle cache is empty, `_discover_matrix_files()` must call - `_ensure_routing_bundle_cached()` exactly once before giving up. + """When the bundle cache is empty, `discover_matrix_files(fetch=True)` + must call `_ensure_routing_bundle_cached()` exactly once before giving up. We don't exercise the real git clone here — we just assert that the function actually tries to populate the cache instead of silently returning []. + + NOTE: the filesystem-scanning + lazy-fetch implementation now lives in + ``lib/routing_matrices.py`` (extracted from ``commands/routing.py``'s + former ``_discover_matrix_files()``) so ``runtime/config.py`` can share + it via ``known_matrix_names()`` without ever triggering the fetch. """ - from amplifier_app_cli.commands import routing + from amplifier_app_cli.lib import routing_matrices fake_home = tmp_path # No .amplifier/cache directory under tmp_path with ( - patch.object(routing.Path, "home", return_value=fake_home), - patch.object(routing, "_ensure_routing_bundle_cached") as mock_fetch, + patch.object(routing_matrices.Path, "home", return_value=fake_home), + patch.object(routing_matrices, "_ensure_routing_bundle_cached") as mock_fetch, ): - result = routing._discover_matrix_files() + result = routing_matrices.discover_matrix_files(fetch=True) mock_fetch.assert_called_once() assert result == [] # Fetch was a no-op (mocked), so still nothing to find def test_discover_matrix_files_skips_fetch_when_cache_exists(tmp_path): - """When the bundle is already cached, `_discover_matrix_files()` must NOT - trigger a fresh fetch. This preserves idempotency and keeps repeated - `amplifier routing list` calls fast. + """When the bundle is already cached, `discover_matrix_files(fetch=True)` + must NOT trigger a fresh fetch. This preserves idempotency and keeps + repeated `amplifier routing list` calls fast. """ - from amplifier_app_cli.commands import routing + from amplifier_app_cli.lib import routing_matrices # Simulate a populated cache cache_dir = ( @@ -93,22 +97,38 @@ def test_discover_matrix_files_skips_fetch_when_cache_exists(tmp_path): (routing_dir / "anthropic.yaml").write_text("name: anthropic\nroles: {}\n") with ( - patch.object(routing.Path, "home", return_value=tmp_path), - patch.object(routing, "_ensure_routing_bundle_cached") as mock_fetch, + patch.object(routing_matrices.Path, "home", return_value=tmp_path), + patch.object(routing_matrices, "_ensure_routing_bundle_cached") as mock_fetch, ): - result = routing._discover_matrix_files() + result = routing_matrices.discover_matrix_files(fetch=True) mock_fetch.assert_not_called() assert len(result) == 1 assert result[0].name == "anthropic.yaml" +def test_discover_matrix_files_never_fetches_by_default(tmp_path): + """discover_matrix_files() defaults to fetch=False -- the session-start + hot path (known_matrix_names()) must never silently block on git I/O + just to validate a bundle-declared routing matrix name.""" + from amplifier_app_cli.lib import routing_matrices + + with ( + patch.object(routing_matrices.Path, "home", return_value=tmp_path), + patch.object(routing_matrices, "_ensure_routing_bundle_cached") as mock_fetch, + ): + result = routing_matrices.discover_matrix_files() + + mock_fetch.assert_not_called() + assert result == [] + + def test_ensure_routing_bundle_cached_swallows_errors(capsys): """A failed fetch (network down, git missing, corporate firewall) must NOT crash the CLI — the user gets a visible yellow warning via console, not a stack trace. This is the exact UX promise COE asked for. """ - from amplifier_app_cli.commands import routing + from amplifier_app_cli.lib import routing_matrices class _BoomResolver: def __init__(self, *_args, **_kwargs) -> None: @@ -122,7 +142,7 @@ def resolve(self) -> None: _BoomResolver, ): # Must not raise - routing._ensure_routing_bundle_cached() + routing_matrices._ensure_routing_bundle_cached() # User MUST see the failure — silent-block + silent-fail is the # anti-pattern this test is guarding against. diff --git a/tests/test_settings_routing_source.py b/tests/test_settings_routing_source.py new file mode 100644 index 00000000..051b615b --- /dev/null +++ b/tests/test_settings_routing_source.py @@ -0,0 +1,88 @@ +"""Tests for AppSettings.get_routing_config_with_source(). + +Covers the "source" half of bundle-declared routing matrix precedence: the +highest-precedence settings scope file that set routing.matrix, used by +runtime/config.py to attribute "who set the active matrix" and by +`amplifier routing show` to display a Source: line. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths + + +def _make_settings(tmp_path: Path) -> AppSettings: + """Create AppSettings with isolated paths for testing.""" + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + return AppSettings(paths=paths) + + +def _write_yaml(path: Path, data: dict) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "w", encoding="utf-8") as f: + yaml.safe_dump(data, f) + + +def test_source_is_none_when_unset(tmp_path): + """No scope sets routing.matrix -> source is None, config is empty.""" + settings = _make_settings(tmp_path) + + config, source = settings.get_routing_config_with_source() + + assert config == {} + assert source is None + + +def test_source_reports_highest_precedence_scope(tmp_path): + """global -> project -> local; local (highest precedence) wins as source.""" + settings = _make_settings(tmp_path) + _write_yaml(settings.paths.global_settings, {"routing": {"matrix": "openai"}}) + _write_yaml(settings.paths.project_settings, {"routing": {"matrix": "gemini"}}) + _write_yaml(settings.paths.local_settings, {"routing": {"matrix": "anthropic"}}) + + config, source = settings.get_routing_config_with_source() + + assert config["matrix"] == "anthropic" + assert source == str(settings.paths.local_settings) + + +def test_source_ignores_scopes_setting_only_overrides(tmp_path): + """A scope that sets routing.overrides (no matrix) must not become the + reported source -- only a scope that actually sets `matrix` counts.""" + settings = _make_settings(tmp_path) + _write_yaml(settings.paths.global_settings, {"routing": {"matrix": "openai"}}) + # Local scope only overrides per-role behavior, doesn't set a matrix. + _write_yaml( + settings.paths.local_settings, + {"routing": {"overrides": {"coding": "quality"}}}, + ) + + config, source = settings.get_routing_config_with_source() + + # Merged config still carries the local overrides (deep-merged as before). + assert config["matrix"] == "openai" + assert config["overrides"] == {"coding": "quality"} + # But the reported *source* of the matrix selection is the global file, + # since local never set routing.matrix itself. + assert source == str(settings.paths.global_settings) + + +def test_get_routing_config_thin_wrapper_matches_source_variant(tmp_path): + """get_routing_config() must keep returning exactly the merged config + half of get_routing_config_with_source() -- no behavior change for + existing callers.""" + settings = _make_settings(tmp_path) + _write_yaml(settings.paths.project_settings, {"routing": {"matrix": "balanced"}}) + + config_only = settings.get_routing_config() + config_with_source, _source = settings.get_routing_config_with_source() + + assert config_only == config_with_source