diff --git a/amplifier_app_cli/lib/bundle_loader/resolvers.py b/amplifier_app_cli/lib/bundle_loader/resolvers.py index ad135987..d0f56c55 100644 --- a/amplifier_app_cli/lib/bundle_loader/resolvers.py +++ b/amplifier_app_cli/lib/bundle_loader/resolvers.py @@ -23,6 +23,7 @@ from typing import Protocol from typing import runtime_checkable +from amplifier_core.module_sources import ModuleNotFoundError as CoreModuleNotFoundError from amplifier_foundation.paths.resolution import get_amplifier_home from amplifier_foundation.sources import SimpleSourceResolver @@ -451,6 +452,18 @@ def resolve(self, module_id: str, hint: Any = None) -> Any: ... +# The kernel's ``amplifier_core.module_sources.ModuleNotFoundError`` is NOT a +# subclass of the builtin ``ModuleNotFoundError`` (it derives from ``Exception`` +# directly). Foundation's ``BundleModuleResolver`` raises the kernel class, so an +# ``except ModuleNotFoundError:`` written against the builtin let it escape -- +# and the settings fallback below never ran for a module the bundle did not +# pre-activate. Catch both, so the fallback policy actually applies. +_MODULE_NOT_FOUND: tuple[type[BaseException], ...] = ( + ModuleNotFoundError, + CoreModuleNotFoundError, +) + + class AppModuleResolver: """Composes bundle resolver with settings-based fallback. @@ -515,7 +528,7 @@ def resolve( # Try bundle first (primary source) try: return self._bundle.resolve(module_id, hint) - except ModuleNotFoundError: + except _MODULE_NOT_FOUND: pass # Fall through to settings resolver # Try settings resolver (fallback) @@ -536,6 +549,53 @@ def resolve( f"Ensure the module is included in the bundle or configure a provider in settings." ) + async def async_resolve( + self, module_id: str, source_hint: Any = None, profile_hint: Any = None + ) -> Any: + """Resolve with the bundle resolver's lazy activation, then the same fallback. + + The kernel loader prefers ``async_resolve`` when a resolver exposes it + (amplifier_core/loader.py) and only then can a module that was NOT + pre-activated at ``Bundle.prepare()`` time still load: foundation's + ``BundleModuleResolver.async_resolve`` activates such a module on demand + from its ``source_hint`` (clone, install deps, register in ``_paths``). + + Without this method the loader fell to the sync ``resolve()`` above, where + the wrapped bundle resolver can only answer for already-activated modules + -- so any hook/tool that reaches the mount plan AFTER prepare() (the + settings-injected ``hooks-routing`` from ``routing.matrix`` is the + measured case) failed at mount with "not found in prepared bundle" even + though its mount-plan entry carried a perfectly good ``source``. This + wrapper is mounted as the session's ``module-source-resolver`` at root + AND inherited by every child session, so the gap was total. + + Policy is unchanged: bundle first, settings second, informative error. + """ + hint = profile_hint if profile_hint is not None else source_hint + + bundle_async = getattr(self._bundle, "async_resolve", None) + try: + if callable(bundle_async): + return await bundle_async(module_id, source_hint=hint) + return self._bundle.resolve(module_id, hint) + except _MODULE_NOT_FOUND: + pass # Fall through to settings resolver + + if self._settings is not None: + try: + result = self._settings.resolve(module_id, hint) + logger.debug(f"Resolved '{module_id}' from settings fallback") + return result + except Exception as e: + logger.debug(f"Settings fallback failed for '{module_id}': {e}") + + available = list(getattr(self._bundle, "_paths", {}).keys()) + raise ModuleNotFoundError( + f"Module '{module_id}' not found in bundle or user settings. " + f"Bundle contains: {available}. " + f"Ensure the module is included in the bundle or configure a provider in settings." + ) + def get_module_source(self, module_id: str) -> str | None: """Get module source path as string. diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 839cfa27..07988e23 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -13,6 +13,7 @@ from amplifier_core.utils.truncate import SENSITIVE_KEYS from rich.console import Console +from ..lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES from ..lib.settings import AppSettings, NotificationFlags, get_custom_routing_dir from ..lib.merge_utils import merge_module_items from ..lib.merge_utils import merge_tool_configs @@ -291,6 +292,36 @@ def _on_progress(action: str, detail: str) -> None: **hooks_routing_extra, **routing_hook_override["config"], } + # A hook the bundle does not already carry needs a `source`, or the + # module cannot load at all. `_apply_hook_overrides` APPENDS an + # override whose module is absent from the bundle's hooks list, and + # the appended dict is the mount-plan entry verbatim -- so without a + # source here, `hooks-routing` reaches the session as + # `{"module": "hooks-routing", "config": {...}}` and the kernel fails + # it by name at mount: + # + # Failed to load hook 'hooks-routing': Module 'hooks-routing' not + # found in prepared bundle. Available modules: [...] + # + # Measured on a bundle that does not include routing-matrix + # (`anchors-amp-dev`) with `routing.matrix: anthropic` in + # settings.yaml: the routing banner left the system prompt, the + # delegate tool dropped its `model_role` parameter (no + # `model_role_resolver` capability), and every `model_role` fell + # through to the default provider. Silent -- the user sees no + # routing, not an error. + # + # Only attached when the bundle has no hooks-routing of its own: the + # in-place merge path in `_apply_hook_overrides` lets the override's + # top-level keys win (merge_module_items: "child overrides parent, + # including 'source'"), so attaching unconditionally would clobber a + # bundle's deliberately pinned source. The URI is the routing-matrix + # bundle's canonical remote from WELL_KNOWN_BUNDLES -- the same one + # `amplifier routing` and `amplifier update` fetch -- narrowed to the + # hook module's subdirectory, matching the bundle's own + # behaviors/routing.yaml declaration. + if not _bundle_declares_hook(bundle_config.get("hooks"), "hooks-routing"): + routing_hook_override["source"] = _routing_hook_source() if routing_hook_override["config"]: hook_overrides.append(routing_hook_override) @@ -680,6 +711,25 @@ def narrow_overrides_to_secrets( return narrowed +def _routing_hook_source() -> str: + """Canonical source URI for the ``hooks-routing`` module. + + Derived from the routing-matrix bundle's registered remote so the CLI has + exactly one place that knows where that bundle lives (``amplifier routing`` + and ``amplifier update`` read the same entry). The ``#subdirectory`` + fragment mirrors the bundle's own ``behaviors/routing.yaml``. + """ + remote = str(WELL_KNOWN_BUNDLES["routing-matrix"]["remote"]) + return f"{remote}#subdirectory=modules/hooks-routing" + + +def _bundle_declares_hook(hooks: Any, module_id: str) -> bool: + """True when *hooks* (a bundle's hooks list, possibly absent) names *module_id*.""" + if not isinstance(hooks, list): + return False + return any(isinstance(h, dict) and h.get("module") == module_id for h in hooks) + + def _apply_hook_overrides( hooks: list[dict[str, Any]], overrides: list[dict[str, Any]] ) -> list[dict[str, Any]]: diff --git a/tests/lib/bundle_loader/test_resolvers.py b/tests/lib/bundle_loader/test_resolvers.py index 7fd916b5..266ce9ac 100644 --- a/tests/lib/bundle_loader/test_resolvers.py +++ b/tests/lib/bundle_loader/test_resolvers.py @@ -5,10 +5,13 @@ """ from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock import pytest +from amplifier_core.module_sources import ( + ModuleNotFoundError as CoreModuleNotFoundError, +) from amplifier_app_cli.lib.bundle_loader.resolvers import ( AppModuleResolver, FoundationFileSource, @@ -363,3 +366,131 @@ def test_get_module_source_falls_back_to_settings(self): result = app_resolver.get_module_source("my-module") assert result == "/path/to/module" + + +class TestAppModuleResolverLazyActivation: + """A module that reaches the mount plan AFTER Bundle.prepare() -- e.g. the + settings-injected hooks-routing -- is not in the bundle resolver's _paths. + The kernel loader prefers ``async_resolve`` when present; that is the only + path that can lazily activate such a module from its ``source_hint``. + + The bundle resolver raises the KERNEL's ModuleNotFoundError + (amplifier_core.module_sources), which is NOT a subclass of the builtin -- + the pre-existing tests above only ever used the builtin, which is how the + mismatch survived. + """ + + def test_kernel_exception_is_not_the_builtin(self): + """Pin the fact this fix exists for. If this ever changes, the tuple + catch below becomes redundant but stays harmless.""" + assert not issubclass(CoreModuleNotFoundError, ModuleNotFoundError) + + def test_sync_resolve_falls_back_on_kernel_exception(self, tmp_path: Path): + """THE sync-path defect: the kernel class escaped ``except ModuleNotFoundError`` + and the settings fallback never ran.""" + module_dir = tmp_path / "m" + module_dir.mkdir() + bundle_resolver = MagicMock() + bundle_resolver.resolve.side_effect = CoreModuleNotFoundError("not prepared") + settings_resolver = MagicMock() + settings_resolver.resolve.return_value = FoundationFileSource(module_dir) + + app = AppModuleResolver(bundle_resolver, settings_resolver=settings_resolver) + result = app.resolve("late-module", source_hint="git+https://x/y@main") + + assert isinstance(result, FoundationFileSource) + settings_resolver.resolve.assert_called_once_with( + "late-module", "git+https://x/y@main" + ) + + @pytest.mark.asyncio + async def test_async_resolve_uses_bundle_lazy_activation(self, tmp_path: Path): + """The primary path: delegate to the bundle resolver's async_resolve so + foundation activates the module from the hint (clone, deps, _paths).""" + module_dir = tmp_path / "m" + module_dir.mkdir() + bundle_resolver = MagicMock() + bundle_resolver.async_resolve = AsyncMock( + return_value=FoundationFileSource(module_dir) + ) + settings_resolver = MagicMock() + + app = AppModuleResolver(bundle_resolver, settings_resolver=settings_resolver) + result = await app.async_resolve( + "hooks-routing", + source_hint="git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main#subdirectory=modules/hooks-routing", + ) + + assert isinstance(result, FoundationFileSource) + bundle_resolver.async_resolve.assert_awaited_once_with( + "hooks-routing", + source_hint="git+https://github.com/microsoft/amplifier-bundle-routing-matrix@main#subdirectory=modules/hooks-routing", + ) + settings_resolver.resolve.assert_not_called() + + @pytest.mark.asyncio + async def test_async_resolve_falls_back_to_settings_on_kernel_exception( + self, tmp_path: Path + ): + module_dir = tmp_path / "m" + module_dir.mkdir() + bundle_resolver = MagicMock() + bundle_resolver.async_resolve = AsyncMock( + side_effect=CoreModuleNotFoundError("activation failed") + ) + settings_resolver = MagicMock() + settings_resolver.resolve.return_value = FoundationFileSource(module_dir) + + app = AppModuleResolver(bundle_resolver, settings_resolver=settings_resolver) + result = await app.async_resolve("late-module", source_hint="hint") + + assert isinstance(result, FoundationFileSource) + settings_resolver.resolve.assert_called_once_with("late-module", "hint") + + @pytest.mark.asyncio + async def test_async_resolve_without_bundle_async_uses_sync_bundle( + self, tmp_path: Path + ): + """A bundle resolver lacking async_resolve (older foundation) still works.""" + module_dir = tmp_path / "m" + module_dir.mkdir() + bundle_resolver = MagicMock(spec=["resolve", "_paths"]) + bundle_resolver.resolve.return_value = FoundationFileSource(module_dir) + bundle_resolver._paths = {} + + app = AppModuleResolver(bundle_resolver) + result = await app.async_resolve("m", source_hint="hint") + + assert isinstance(result, FoundationFileSource) + bundle_resolver.resolve.assert_called_once_with("m", "hint") + + @pytest.mark.asyncio + async def test_async_resolve_raises_when_both_fail(self): + bundle_resolver = MagicMock() + bundle_resolver.async_resolve = AsyncMock( + side_effect=CoreModuleNotFoundError("nope") + ) + bundle_resolver._paths = {"tool-bash": Path("/x")} + settings_resolver = MagicMock() + settings_resolver.resolve.side_effect = ModuleResolutionError("nope") + + app = AppModuleResolver(bundle_resolver, settings_resolver=settings_resolver) + with pytest.raises( + ModuleNotFoundError, match="not found in bundle or user settings" + ): + await app.async_resolve("ghost") + + @pytest.mark.asyncio + async def test_async_resolve_profile_hint_alias(self, tmp_path: Path): + """Backward compat: the deprecated profile_hint still feeds the hint.""" + module_dir = tmp_path / "m" + module_dir.mkdir() + bundle_resolver = MagicMock() + bundle_resolver.async_resolve = AsyncMock( + return_value=FoundationFileSource(module_dir) + ) + app = AppModuleResolver(bundle_resolver) + await app.async_resolve("m", profile_hint="legacy-hint") + bundle_resolver.async_resolve.assert_awaited_once_with( + "m", source_hint="legacy-hint" + ) diff --git a/tests/test_general_config_overrides.py b/tests/test_general_config_overrides.py index 412765e7..c7b95c4f 100644 --- a/tests/test_general_config_overrides.py +++ b/tests/test_general_config_overrides.py @@ -8,11 +8,14 @@ import pytest from unittest.mock import AsyncMock, MagicMock, patch +from amplifier_app_cli.lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES from amplifier_app_cli.lib.merge_utils import deep_merge from amplifier_app_cli.runtime.config import ( _apply_hook_overrides, _apply_provider_overrides, _apply_tool_overrides, + _bundle_declares_hook, + _routing_hook_source, resolve_bundle_config, ) @@ -701,3 +704,132 @@ async def test_dedicated_overrides_win_in_full_pipeline(self): assert hook["config"]["enabled"] is True # dedicated wins assert hook["config"]["topic"] == "general" # general fills in assert hook["config"]["base"] is True # original preserved + + +# ═══════════════════════════════════════════════════════════════════════════ +# PART 4: settings-injected hooks-routing must carry a loadable source +# +# Regression. `routing.matrix` in settings.yaml injects a `hooks-routing` +# override. When the active bundle does not already include the routing-matrix +# bundle, `_apply_hook_overrides` APPENDS that override verbatim as the mount- +# plan entry -- and it had no `source`, so the kernel failed it by name at +# mount ("Module 'hooks-routing' not found in prepared bundle"). Measured on +# `anchors-amp-dev`: no routing banner, no `model_role` on the delegate tool, +# every model_role silently falling through to the default provider. +# +# Contract pinned here: +# * bundle has NO hooks-routing -> injected entry carries the canonical source +# * bundle HAS hooks-routing -> the bundle's own source is left untouched +# * the canonical source is the routing-matrix bundle's registered remote, +# narrowed to the hook module's subdirectory +# ═══════════════════════════════════════════════════════════════════════════ + +_ROUTING_HOOK_SOURCE = ( + str(WELL_KNOWN_BUNDLES["routing-matrix"]["remote"]) + + "#subdirectory=modules/hooks-routing" +) + + +class TestRoutingHookSource: + def test_source_derives_from_well_known_remote(self): + """One place knows where routing-matrix lives; the hook source is that + remote plus the module subdirectory -- byte-identical to the bundle's + own behaviors/routing.yaml declaration.""" + src = _routing_hook_source() + assert src == _ROUTING_HOOK_SOURCE + assert src.startswith("git+https://") + assert src.endswith("#subdirectory=modules/hooks-routing") + + def test_bundle_declares_hook(self): + assert _bundle_declares_hook([{"module": "hooks-routing"}], "hooks-routing") + assert not _bundle_declares_hook([{"module": "hooks-ci"}], "hooks-routing") + assert not _bundle_declares_hook([], "hooks-routing") + assert not _bundle_declares_hook(None, "hooks-routing") + # Non-dict entries (e.g. a bare string) never match, never raise. + assert not _bundle_declares_hook(["hooks-routing"], "hooks-routing") + + +class TestRoutingHookSourceIntegration: + """Through the real resolve_bundle_config(); same patch set as PART 2.""" + + @staticmethod + def _run(mount_plan, routing_config): + mock_prepared = MagicMock() + mock_prepared.mount_plan = mount_plan + mock_prepared.bundle.load_agent_metadata = MagicMock() + settings = _make_app_settings(routing_config=routing_config) + + async def go(): + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + new_callable=AsyncMock, + return_value=mock_prepared, + ), + 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 + ) + return [h for h in result["hooks"] if h.get("module") == "hooks-routing"] + + return go() + + @pytest.mark.asyncio + async def test_injected_hook_carries_source_when_bundle_lacks_it(self): + """THE defect: the appended entry must be loadable.""" + entries = await self._run( + mount_plan={"hooks": [{"module": "hooks-ci", "config": {}}]}, + routing_config={"matrix": "anthropic"}, + ) + assert len(entries) == 1 + assert entries[0].get("source") == _ROUTING_HOOK_SOURCE, ( + "A settings-injected hooks-routing the bundle does not carry must " + "have a source, or the kernel fails it by name at mount" + ) + assert entries[0]["config"]["default_matrix"] == "anthropic" + + @pytest.mark.asyncio + async def test_injected_hook_carries_source_when_bundle_has_no_hooks_at_all(self): + entries = await self._run(mount_plan={}, routing_config={"matrix": "anthropic"}) + assert len(entries) == 1 + assert entries[0].get("source") == _ROUTING_HOOK_SOURCE + + @pytest.mark.asyncio + async def test_bundle_pinned_source_is_never_clobbered(self): + """When the bundle already ships hooks-routing (foundation does), its + own -- possibly deliberately pinned -- source must survive the merge. + merge_module_items lets the override's top-level keys win, so the + override must not carry a source on this path.""" + pinned = ( + "git+https://github.com/someone/fork-routing-matrix@v1.2.3" + "#subdirectory=modules/hooks-routing" + ) + entries = await self._run( + mount_plan={ + "hooks": [ + { + "module": "hooks-routing", + "source": pinned, + "config": {"default_matrix": "balanced"}, + } + ] + }, + routing_config={"matrix": "anthropic"}, + ) + assert len(entries) == 1, "no duplicate entry" + assert entries[0]["source"] == pinned + # settings still win on the config key, as before + assert entries[0]["config"]["default_matrix"] == "anthropic" + + @pytest.mark.asyncio + async def test_no_routing_config_injects_nothing(self): + """Unchanged: without routing.matrix there is no hooks-routing at all.""" + entries = await self._run( + mount_plan={"hooks": [{"module": "hooks-ci", "config": {}}]}, + routing_config=None, + ) + assert entries == []