From c2b4de7af65f1a472da94d06217beb2f0b991471 Mon Sep 17 00:00:00 2001 From: Sam Schillace Date: Thu, 6 Aug 2026 11:18:48 -0700 Subject: [PATCH] fix(routing): compose routing behaviors before prepare and treat routing failure as required Ensure routing behavior bundles are composed before bundle preparation so routing hook modules are available during prepare(). Add required_behaviors parameter to load_and_prepare_bundle() so selected behaviors (routing) propagate load/composition errors instead of being silently ignored. This prevents a source-less hook being appended after prepare by composing routing ahead of prepare, and preserves compatibility by treating only routing as required while leaving other behaviors optional. Tests: Added unit tests to assert required behavior failures propagate and optional behavior failures remain non-fatal. Integration tests verify routing is composed when active and not when inactive, and that preparation errors propagate when routing is active. Acceptance: 38 tests passed locally (independently verified). Compatibility risk: low. The behavior tightens semantics for routing (will fail fast if routing composition fails) which may cause failures in environments that relied on silent warnings from earlier behavior composition. Other behaviors remain optional. --- .gitignore | 1 + .../lib/bundle_loader/prepare.py | 9 +- amplifier_app_cli/runtime/config.py | 33 +++++- tests/lib/bundle_loader/test_prepare.py | 64 +++++++++++ tests/test_general_config_overrides.py | 102 +++++++++++++++++- 5 files changed, 201 insertions(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 6802ccb9..f918ec76 100644 --- a/.gitignore +++ b/.gitignore @@ -69,5 +69,6 @@ next-steps.md /amplifier-app-cli/ # Working folders +.next/ ai_working/tmp tests/recipes/DECISIONS.md diff --git a/amplifier_app_cli/lib/bundle_loader/prepare.py b/amplifier_app_cli/lib/bundle_loader/prepare.py index 1fad6265..0fea85d2 100644 --- a/amplifier_app_cli/lib/bundle_loader/prepare.py +++ b/amplifier_app_cli/lib/bundle_loader/prepare.py @@ -51,6 +51,7 @@ 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, ) -> PreparedBundle: """Load bundle by name or URI and prepare it for execution. @@ -83,6 +84,10 @@ 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 backward compatibility with optional + policies such as notifications. Returns: PreparedBundle ready for create_session(). @@ -168,8 +173,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/runtime/config.py b/amplifier_app_cli/runtime/config.py index 9d7afa4a..5f833a2d 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -93,8 +93,15 @@ def _on_progress(action: str, detail: str) -> None: _build_notification_behaviors(app_settings.get_notification_flags()) ) + # Routing matrix behavior. Resolve routing once so the same active + # configuration drives both pre-prepare composition and the + # post-prepare hooks-routing config merge below. + routing_config = app_settings.get_routing_config() + routing_behaviors = _build_routing_behaviors(routing_config) + 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 policy behaviors app_bundles = app_settings.get_app_bundles() if app_bundles: compose_behaviors = compose_behaviors + app_bundles @@ -133,6 +140,7 @@ 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, @@ -231,7 +239,6 @@ def _on_progress(action: str, detail: str) -> None: hook_overrides = app_settings.get_notification_hook_overrides() # Routing matrix config injection - routing_config = app_settings.get_routing_config() if routing_config: routing_hook_override: dict[str, Any] = { "module": "hooks-routing", @@ -314,9 +321,9 @@ 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(). - # The behavior bundles handle root-session-only logic internally via parent_id check. + # Note: Notification and routing hooks are composed via compose_behaviors + # before load_and_prepare_bundle() prepares their modules. + # Notification behaviors handle root-session-only logic internally via parent_id. return bundle_config, prepared @@ -884,6 +891,22 @@ def _build_modes_behaviors() -> list[str]: ] +def _build_routing_behaviors(routing_config: dict[str, Any]) -> list[str]: + """Return the routing behavior URI when routing configuration is active. + + Routing is an app-level policy. Its behavior must be composed before + preparation so ``hooks-routing`` and its module source are available to + the prepared bundle. The resolved config is also reused after preparation + to apply matrix selection and routing overrides. + """ + 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..ef3847d1 100644 --- a/tests/lib/bundle_loader/test_prepare.py +++ b/tests/lib/bundle_loader/test_prepare.py @@ -241,3 +241,67 @@ 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() + load_error = RuntimeError("hooks-routing module unavailable") + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_bundle", + new_callable=AsyncMock, + side_effect=[mock_bundle, load_error], + ), + 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_optional_behavior_load_failure_is_still_ignored(self): + """Unrelated optional behavior failures preserve existing semantics.""" + 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_general_config_overrides.py b/tests/test_general_config_overrides.py index c6b79abe..df0d6afd 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 # ═══════════════════════════════════════════════════════════════════════════ @@ -318,6 +319,11 @@ def _make_app_settings(config_overrides=None, **kwargs): "hook_overrides", [] ) settings.get_routing_config.return_value = kwargs.get("routing_config", 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 +337,98 @@ 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 sources its behavior through the prepare workflow.""" + 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 + ) + + compose_behaviors = prepare.await_args_list[0].kwargs["compose_behaviors"] + required_behaviors = prepare.await_args_list[0].kwargs["required_behaviors"] + assert ( + "git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main" + "#subdirectory=behaviors/routing.yaml" + ) in compose_behaviors + assert required_behaviors <= set(compose_behaviors) + assert any( + "amplifier-bundle-routing-matrix" in uri for uri in required_behaviors + ) + assert result["hooks"][0]["config"]["default_matrix"] == "balanced" + assert result["hooks"][0]["config"]["overrides"] == {"coding": "quality"} + settings.get_routing_config.assert_called_once_with() + + @pytest.mark.asyncio + async def test_inactive_routing_not_composed_before_prepare(self): + """Absent routing config leaves the routing behavior out of composition.""" + 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 + ) + + compose_behaviors = prepare.await_args_list[0].kwargs["compose_behaviors"] + required_behaviors = prepare.await_args_list[0].kwargs["required_behaviors"] + assert all( + "amplifier-bundle-routing-matrix" not in uri for uri in compose_behaviors + ) + assert required_behaviors is None + assert result["hooks"] == [] + settings.get_routing_config.assert_called_once_with() + + @pytest.mark.asyncio + async def test_routing_preparation_error_propagates(self): + """Errors from preparation 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) + + compose_behaviors = prepare.await_args_list[0].kwargs["compose_behaviors"] + assert any( + "amplifier-bundle-routing-matrix" in uri for uri in compose_behaviors + ) + @pytest.mark.asyncio async def test_hook_override_flows_through_full_pipeline(self): """Config override for a hook reaches final bundle_config."""