From 8d67c3dd4f793607156ca33f8f3dc473296c6518 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:04:04 -0700 Subject: [PATCH] fix(routing): make the settings-injected hooks-routing loadable on a bundle that does not ship it `routing.matrix: ` in settings.yaml injects a `hooks-routing` override (runtime/config.py). When the active bundle does not already include the routing-matrix bundle, `_apply_hook_overrides` APPENDS that override verbatim as the mount-plan entry. Two independent gaps then combined to make the hook fail at mount, silently: 1. The override carried no `source`. Nothing in the resolver chain knows where `hooks-routing` lives, so the kernel failed it by name: Failed to load hook 'hooks-routing': Module 'hooks-routing' not found in prepared bundle. Available modules: [...] 2. Even WITH a source, the module could not load. It reaches the mount plan AFTER Bundle.prepare() (settings are applied to the prepared mount plan), so it is not in the bundle resolver's activated `_paths`. Foundation's `BundleModuleResolver.async_resolve` exists precisely to lazily activate such a module from its source hint -- but the kernel loader only takes that path when the mounted resolver exposes `async_resolve`, and `AppModuleResolver` (mounted at root as `module-source-resolver`, inherited by every child) did not. The loader fell to the sync path, and there `except ModuleNotFoundError` caught the BUILTIN class while the bundle resolver raises `amplifier_core.module_sources.ModuleNotFoundError`, which is not a subclass of it -- so the settings fallback (which does honor source hints) never ran either. Why it looked like it worked: any session on a bundle that DOES include routing-matrix (foundation) editable-installs `amplifier_module_hooks_routing` into the app venv as a side effect, registering an `amplifier.modules` entry point. From then on the source-less injection resolves via the installed-package fallback for every bundle -- until `uv tool install` / auto-update recreates the venv, at which point routing silently vanishes for every bundle that does not ship the hook. Measured on `anchors-amp-dev` with `routing.matrix: anthropic` across exactly such a reinstall: the routing banner left the system prompt (101,106 -> 99,832 chars), the delegate tool dropped its `model_role` parameter (no `model_role_resolver` capability), and every `model_role` fell through to the default provider. No error reached the user. THE FIX (two halves, both needed) runtime/config.py: when the bundle has no hooks-routing of its own, attach the canonical source -- `WELL_KNOWN_BUNDLES["routing-matrix"]["remote"]` (the same entry `amplifier routing` / `amplifier update` use) narrowed to `#subdirectory=modules/hooks-routing`, matching the bundle's own behaviors/routing.yaml. Only on the append path: merge_module_items lets an override's top-level keys win, so attaching unconditionally would clobber a bundle's deliberately pinned source. lib/bundle_loader/resolvers.py: give `AppModuleResolver` an `async_resolve` that delegates to the bundle resolver's lazy activation (falling back to sync resolve for an older foundation), then the settings fallback, then the same informative error. Catch BOTH ModuleNotFoundError classes on both paths so the fallback policy actually applies. MEASURED, anchors-amp-dev, app venv wiped of the side-effect install (the post-reinstall condition), same prompt: delegate(agent=self, model_role=fast) before Failed to load hook 'hooks-routing' ... not found in prepared bundle routing banner in system prompt: False after hooks-routing source = git+...routing-matrix@main#subdirectory=modules/hooks-routing routing banner in system prompt: True delegate:agent_spawned model_role='fast' provider_preferences=[{provider: haiku, model: claude-haiku-4-5-20251001}] child: pri=0 id=haiku effort=high fallback_on_overload=None child llm:request: anthropic/claude-haiku-4-5-20251001, 0 [PROVIDER] warnings Tests: 13 new. 1647 pass, up from 1634; no existing test changed. The new resolver tests fail against the unfixed resolvers.py (6/7; the 7th pins the exception-class fact). The pre-existing AppModuleResolver tests only ever raised the builtin class, which is how the mismatch survived. --- .../lib/bundle_loader/resolvers.py | 62 +++++++- amplifier_app_cli/runtime/config.py | 50 +++++++ tests/lib/bundle_loader/test_resolvers.py | 133 +++++++++++++++++- tests/test_general_config_overrides.py | 132 +++++++++++++++++ 4 files changed, 375 insertions(+), 2 deletions(-) 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 == []