refactor: deduplicate entry-point loading (#460)

* refactor: deduplicate entry-point loading

* fix: preserve config conflict error priority

reme/config/config_parser.py

@@ -4,13 +4,13 @@ import json
 import os
 import re
 from collections.abc import Mapping
-from importlib import metadata
+from importlib.metadata import EntryPoint
 from pathlib import Path
 from typing import Any
 
 import yaml
 
-from ..entry_point import load_entry_point
+from ..entry_point import find_entry_points, load_entry_point, unique_entry_point
 
 # Config files are looked up relative to this module's directory
 _CONFIG_DIR = Path(__file__).parent
@@ -127,20 +127,11 @@ def _convert_value(value_str: str) -> Any:
     return s
 
 
-def _external_config_entries(name: str) -> list[metadata.EntryPoint]:
-    """Find installed config providers without importing their packages."""
-    return list(metadata.entry_points().select(group=_CONFIG_ENTRY_POINT_GROUP, name=name))
-
-
-def _external_config_path(name: str, entries: list[metadata.EntryPoint] | None = None) -> Path | None:
+def _external_config_path(name: str, entry: EntryPoint | None) -> Path | None:
     """Resolve an installed plugin config exposed through ``reme.configs``."""
-    entries = _external_config_entries(name) if entries is None else entries
-    if not entries:
+    if entry is None:
         return None
-    if len(entries) > 1:
-        providers = ", ".join(sorted(entry.value for entry in entries))
-        raise ValueError(f"Config '{name}' has multiple installed providers: {providers}")
-    value = load_entry_point(entries[0], invoke=True)
+    value = load_entry_point(entry, invoke=True)
     path = Path(value)
     if path.suffix not in _SUPPORTED_EXTS or not path.is_file():
         raise ValueError(f"Config entry point '{name}' did not resolve to a YAML or JSON file")
@@ -154,13 +145,14 @@ def _load_config(name_or_path: str, encoding: str = "utf-8", _stack: tuple[str,
         raise ValueError(f"Circular config inheritance: {chain}")
 
     built_in = _CONFIG_REGISTRY.get(name_or_path)
-    external_entries = _external_config_entries(name_or_path)
+    external_entries = find_entry_points(_CONFIG_ENTRY_POINT_GROUP, name_or_path)
     if built_in is not None and external_entries:
         raise ValueError(f"Config '{name_or_path}' is provided by both ReMe and an installed distribution")
     if built_in is not None:
         return _load_config_path(built_in, name_or_path, encoding, _stack)
 
-    external = _external_config_path(name_or_path, external_entries)
+    external_entry = unique_entry_point(external_entries, name_or_path, provider="Config")
+    external = _external_config_path(name_or_path, external_entry)
     if external is not None:
         return _load_config_path(external, name_or_path, encoding, _stack)
 

reme/entry_point.py

@@ -4,6 +4,24 @@ from importlib import metadata
 from typing import Any
 
 
+def find_entry_points(group: str, name: str) -> list[metadata.EntryPoint]:
+    """Return all matching entry points without importing their providers."""
+    return list(metadata.entry_points().select(group=group, name=name))
+
+
+def unique_entry_point(
+    entries: list[metadata.EntryPoint],
+    name: str,
+    *,
+    provider: str,
+) -> metadata.EntryPoint | None:
+    """Return the sole matching entry point, rejecting ambiguous providers."""
+    if len(entries) > 1:
+        values = ", ".join(sorted(entry.value for entry in entries))
+        raise ValueError(f"{provider} '{name}' has multiple installed providers: {values}")
+    return entries[0] if entries else None
+
+
 def load_entry_point(entry: metadata.EntryPoint, *, invoke: bool = False) -> Any:
     """Load and optionally invoke an entry point without retaining registrations."""
     # Import lazily so config parser imports do not pull in the component graph.

reme/plugin.py

@@ -4,13 +4,12 @@ from __future__ import annotations
 
 from collections.abc import Iterable, Mapping
 from dataclasses import dataclass, field
-from importlib import metadata
 from typing import Any
 
 from .components.base_component import ComponentMixin
 from .components.component_registry import ComponentRegistry
 from .config import deep_merge_config, expand_env_vars
-from .entry_point import load_entry_point
+from .entry_point import find_entry_points, load_entry_point, unique_entry_point
 
 PLUGIN_ENTRY_POINT_GROUP = "reme.plugins"
 
@@ -32,11 +31,6 @@ class Plugin:
     config: Mapping[str, Any] = field(default_factory=dict)
 
 
-def _entry_points(group: str, name: str) -> list[metadata.EntryPoint]:
-    """Return matching entry points for one group and name."""
-    return list(metadata.entry_points().select(group=group, name=name))
-
-
 class PluginManager:
     """Resolve enabled plugins and apply their contributions to one application."""
 
@@ -55,13 +49,11 @@ class PluginManager:
                 raise ValueError("Plugin name cannot be empty")
             if name in seen:
                 raise ValueError(f"Plugin '{name}' is enabled more than once")
-            entries = _entry_points(PLUGIN_ENTRY_POINT_GROUP, name)
-            if not entries:
+            entries = find_entry_points(PLUGIN_ENTRY_POINT_GROUP, name)
+            entry = unique_entry_point(entries, name, provider="Plugin")
+            if entry is None:
                 raise ValueError(f"Plugin '{name}' is not installed")
-            if len(entries) > 1:
-                providers = ", ".join(sorted(entry.value for entry in entries))
-                raise ValueError(f"Plugin '{name}' has multiple installed providers: {providers}")
-            plugin = load_entry_point(entries[0], invoke=True)
+            plugin = load_entry_point(entry, invoke=True)
             if not isinstance(plugin, Plugin):
                 raise TypeError(f"Plugin entry point '{name}' did not return reme.plugin.Plugin")
             if plugin.name != name:

tests/unit/test_config_parser.py

@@ -21,7 +21,8 @@ def test_load_builtin_config_by_filename_with_suffix():
     assert cfg["service"]["backend"] == "http"
 
 
-def test_builtin_and_external_config_name_collision_fails(monkeypatch):
+@pytest.mark.parametrize("provider_count", [1, 2])
+def test_builtin_and_external_config_name_collision_fails(monkeypatch, provider_count):
     """An installed config cannot be silently shadowed by a built-in name."""
 
     class FakeEntryPoint:
@@ -44,8 +45,8 @@ def test_builtin_and_external_config_name_collision_fails(monkeypatch):
             return [entry for entry in self if entry.name == name]
 
     monkeypatch.setattr(
-        "reme.config.config_parser.metadata.entry_points",
-        lambda: FakeEntryPoints([FakeEntryPoint()]),
+        "reme.entry_point.metadata.entry_points",
+        lambda: FakeEntryPoints([FakeEntryPoint() for _ in range(provider_count)]),
     )
 
     with pytest.raises(ValueError, match="provided by both ReMe and an installed distribution"):

tests/unit/test_plugin.py

@@ -2,7 +2,6 @@
 
 # pylint: disable=missing-class-docstring,missing-function-docstring
 
-from importlib.metadata import EntryPoint
 from pathlib import Path
 
 import pytest
@@ -23,8 +22,35 @@ class _PluginComponent(BaseComponent):
     component_type = "example.reranker"
 
 
+class _FakeEntryPoint:
+    def __init__(self, name, value, loader, group):
+        self.name = name
+        self.value = value
+        self._loader = loader
+        self.group = group
+
+    def load(self):
+        return self._loader()
+
+
+class _FakeEntryPoints(list):
+    def select(self, *, group, name):
+        return [entry for entry in self if entry.group == group and entry.name == name]
+
+
+def _set_entry_points(monkeypatch, *entries):
+    monkeypatch.setattr("reme.entry_point.metadata.entry_points", lambda: _FakeEntryPoints(entries))
+
+
 def test_plugin_defaults_are_below_application_config():
-    manager = PluginManager([Plugin(name="example", config={"jobs": {"task": {"backend": "base", "value": 1}}})])
+    manager = PluginManager(
+        [
+            Plugin(
+                name="example",
+                config={"jobs": {"task": {"backend": "base", "value": 1}}},
+            ),
+        ],
+    )
 
     merged = manager.merge_config({"jobs": {"task": {"value": 2}}})
 
@@ -102,48 +128,45 @@ def test_plugin_backend_collision_fails_with_both_owners():
 
 def test_plugin_manager_loads_explicit_entry_point(monkeypatch):
     descriptor = Plugin(name="example", backends=(Backend("example_step", _PluginStep),))
-
-    class FakeEntryPoint:
-        name = "example"
-        value = "example:plugin"
-
-        @staticmethod
-        def load():
-            return descriptor
-
-    class FakeEntryPoints(list):
-        def select(self, *, group, name):
-            assert group == "reme.plugins"
-            return [entry for entry in self if entry.name == name]
-
-    monkeypatch.setattr("reme.plugin.metadata.entry_points", lambda: FakeEntryPoints([FakeEntryPoint()]))
+    _set_entry_points(
+        monkeypatch,
+        _FakeEntryPoint("example", "example:plugin", lambda: descriptor, "reme.plugins"),
+    )
 
     manager = PluginManager.discover(["example"])
 
     assert manager.plugins == (descriptor,)
 
 
+def test_plugin_manager_rejects_multiple_entry_point_providers(monkeypatch):
+    descriptor = Plugin(name="example")
+    _set_entry_points(
+        monkeypatch,
+        _FakeEntryPoint("example", "first:plugin", lambda: descriptor, "reme.plugins"),
+        _FakeEntryPoint("example", "second:plugin", lambda: descriptor, "reme.plugins"),
+    )
+
+    with pytest.raises(
+        ValueError,
+        match="Plugin 'example' has multiple installed providers: first:plugin, second:plugin",
+    ):
+        PluginManager.discover(["example"])
+
+
 def test_plugin_entry_point_import_side_effect_does_not_leak(monkeypatch, tmp_path):
     class UndeclaredClient(ComponentMixin):
         component_type = ComponentEnum.CLIENT
 
     descriptor = Plugin(name="example")
 
-    class FakeEntryPoint:
-        name = "example"
-        value = "example:plugin"
+    def load_plugin():
+        R.register(UndeclaredClient, "undeclared-client")
+        return descriptor
 
-        @staticmethod
-        def load():
-            R.register(UndeclaredClient, "undeclared-client")
-            return descriptor
-
-    class FakeEntryPoints(list):
-        def select(self, *, group, name):
-            assert group == "reme.plugins"
-            return [entry for entry in self if entry.name == name]
-
-    monkeypatch.setattr("reme.plugin.metadata.entry_points", lambda: FakeEntryPoints([FakeEntryPoint()]))
+    _set_entry_points(
+        monkeypatch,
+        _FakeEntryPoint("example", "example:plugin", load_plugin, "reme.plugins"),
+    )
 
     app = Application(
         plugins=["example"],
@@ -175,24 +198,9 @@ def test_config_can_extend_another_config(tmp_path: Path):
 def test_config_can_come_from_installed_entry_point(tmp_path: Path, monkeypatch):
     config = tmp_path / "example.yaml"
     config.write_text("plugins: [example]\n", encoding="utf-8")
-    entry = EntryPoint(name="example", value="pathlib:Path", group="reme.configs")
-
-    class LoadedEntryPoint:
-        name = entry.name
-        value = entry.value
-
-        @staticmethod
-        def load():
-            return config
-
-    class FakeEntryPoints(list):
-        def select(self, *, group, name):
-            assert group == "reme.configs"
-            return [item for item in self if item.name == name]
-
-    monkeypatch.setattr(
-        "reme.config.config_parser.metadata.entry_points",
-        lambda: FakeEntryPoints([LoadedEntryPoint()]),
+    _set_entry_points(
+        monkeypatch,
+        _FakeEntryPoint("example", "example:CONFIG_PATH", lambda: config, "reme.configs"),
     )
 
     assert _load_config("example") == {"plugins": ["example"]}
@@ -205,23 +213,13 @@ def test_config_entry_point_import_side_effect_does_not_leak(tmp_path: Path, mon
     class UndeclaredClient(ComponentMixin):
         component_type = ComponentEnum.CLIENT
 
-    class LoadedEntryPoint:
-        name = "side-effect"
-        value = "example:CONFIG_PATH"
+    def load_config():
+        R.register(UndeclaredClient, "config-side-effect-client")
+        return config
 
-        @staticmethod
-        def load():
-            R.register(UndeclaredClient, "config-side-effect-client")
-            return config
-
-    class FakeEntryPoints(list):
-        def select(self, *, group, name):
-            assert group == "reme.configs"
-            return [item for item in self if item.name == name]
-
-    monkeypatch.setattr(
-        "reme.config.config_parser.metadata.entry_points",
-        lambda: FakeEntryPoints([LoadedEntryPoint()]),
+    _set_entry_points(
+        monkeypatch,
+        _FakeEntryPoint("side-effect", "example:CONFIG_PATH", load_config, "reme.configs"),
     )
 
     loaded = _load_config("side-effect")

tests/unit/test_reme_cli.py

@@ -16,6 +16,37 @@ from reme.enumeration import ComponentEnum
 from reme.plugin import Backend, Plugin, PluginManager
 
 
+def _recording_client(seen, output="ok", base=object):
+    """Build an async client stub that records construction and calls."""
+
+    class RecordingClient(base):
+        """Async client stub backed by a shared call record."""
+
+        def __init__(self, **kwargs):
+            seen["client_kwargs"] = kwargs
+
+        async def __aenter__(self):
+            return self
+
+        async def __aexit__(self, exc_type, exc_val, exc_tb):
+            return None
+
+        async def __call__(self, action: str, **kwargs):
+            seen["action"] = action
+            seen["payload"] = kwargs
+            yield output
+
+    return RecordingClient
+
+
+def _set_client_backend(monkeypatch, client_cls):
+    monkeypatch.setattr(
+        reme_module,
+        "create_application_registry",
+        lambda: SimpleNamespace(get=lambda component_type, backend: client_cls),
+    )
+
+
 def test_package_import_does_not_load_optional_core_dependencies():
     """The base package leaves optional core dependencies unloaded."""
     script = """
@@ -242,29 +273,7 @@ def test_cli_service_exits_nonzero_on_failed_response(capsys):
 def test_call_server_passes_client_kwargs_to_client(monkeypatch, capsys):
     """CLI helper forwards connection options to the selected client."""
     seen = {}
-
-    class FakeClient:
-        """Async client stub that records call arguments."""
-
-        def __init__(self, **kwargs):
-            seen["client_kwargs"] = kwargs
-
-        async def __aenter__(self):
-            return self
-
-        async def __aexit__(self, exc_type, exc_val, exc_tb):
-            return None
-
-        async def __call__(self, action: str, **kwargs):
-            seen["action"] = action
-            seen["payload"] = kwargs
-            yield "ok"
-
-    monkeypatch.setattr(
-        reme_module,
-        "create_application_registry",
-        lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient),
-    )
+    _set_client_backend(monkeypatch, _recording_client(seen))
     monkeypatch.setattr(reme_module, "running_app_config", lambda: None)
 
     async def run():
@@ -288,29 +297,7 @@ def test_call_server_passes_client_kwargs_to_client(monkeypatch, capsys):
 def test_call_server_treats_show_metadata_as_client_kwarg(monkeypatch, capsys):
     """show_metadata controls client display and is not sent as a tool argument."""
     seen = {}
-
-    class FakeClient:
-        """Async client stub that records call arguments."""
-
-        def __init__(self, **kwargs):
-            seen["client_kwargs"] = kwargs
-
-        async def __aenter__(self):
-            return self
-
-        async def __aexit__(self, exc_type, exc_val, exc_tb):
-            return None
-
-        async def __call__(self, action: str, **kwargs):
-            seen["action"] = action
-            seen["payload"] = kwargs
-            yield "ok"
-
-    monkeypatch.setattr(
-        reme_module,
-        "create_application_registry",
-        lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient),
-    )
+    _set_client_backend(monkeypatch, _recording_client(seen))
     monkeypatch.setattr(reme_module, "running_app_config", lambda: None)
 
     async def run():
@@ -327,29 +314,7 @@ def test_call_server_treats_show_metadata_as_client_kwarg(monkeypatch, capsys):
 def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys):
     """Shell-specific parameter names do not collide with client options."""
     seen = {}
-
-    class FakeClient:
-        """Async client stub that records shell request arguments."""
-
-        def __init__(self, **kwargs):
-            seen["client_kwargs"] = kwargs
-
-        async def __aenter__(self):
-            return self
-
-        async def __aexit__(self, exc_type, exc_val, exc_tb):
-            return None
-
-        async def __call__(self, action: str, **kwargs):
-            seen["action"] = action
-            seen["payload"] = kwargs
-            yield "ok"
-
-    monkeypatch.setattr(
-        reme_module,
-        "create_application_registry",
-        lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient),
-    )
+    _set_client_backend(monkeypatch, _recording_client(seen))
     monkeypatch.setattr(reme_module, "running_app_config", lambda: None)
 
     async def run():
@@ -365,37 +330,29 @@ def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys):
 def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch, capsys):
     """A bare client call can load the Client backend enabled by the running app."""
     seen = {}
-
-    class PluginClient(ComponentMixin):
-        """Client backend supplied by an enabled plugin."""
-
-        component_type = ComponentEnum.CLIENT
-
-        def __init__(self, **kwargs):
-            super().__init__(**kwargs)
-            seen["client_kwargs"] = kwargs
-
-        async def __aenter__(self):
-            return self
-
-        async def __aexit__(self, exc_type, exc_val, exc_tb):
-            return None
-
-        async def __call__(self, action: str, **kwargs):
-            seen["action"] = action
-            seen["payload"] = kwargs
-            yield "plugin-ok"
+    plugin_client = _recording_client(seen, output="plugin-ok", base=ComponentMixin)
+    plugin_client.component_type = ComponentEnum.CLIENT
 
     manager = PluginManager(
         [
             Plugin(
                 name="example",
-                backends=(Backend("plugin-client", PluginClient),),
-                config={"service": {"backend": "plugin-client", "host": "127.0.0.9", "port": 9911}},
+                backends=(Backend("plugin-client", plugin_client),),
+                config={
+                    "service": {
+                        "backend": "plugin-client",
+                        "host": "127.0.0.9",
+                        "port": 9911,
+                    },
+                },
             ),
         ],
     )
-    monkeypatch.setattr(reme_module, "resolve_app_config", lambda **_kwargs: {"service": {"backend": "http"}})
+    monkeypatch.setattr(
+        reme_module,
+        "resolve_app_config",
+        lambda **_kwargs: {"service": {"backend": "http"}},
+    )
     monkeypatch.setattr(reme_module, "running_app_config", lambda: {"plugins": ["example"]})
     monkeypatch.setattr(reme_module.PluginManager, "discover", lambda _specs: manager)
 
@@ -410,29 +367,8 @@ def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch
 
 def test_call_server_skips_local_fallback_when_server_is_running(monkeypatch, capsys):
     """A usable running config prevents eager parsing of the local fallback."""
-
-    class FakeClient:
-        """Minimal running-service client."""
-
-        def __init__(self, **_kwargs):
-            pass
-
-        async def __aenter__(self):
-            return self
-
-        async def __aexit__(self, exc_type, exc_val, exc_tb):
-            return None
-
-        async def __call__(self, action: str, **kwargs):
-            assert action == "version"
-            assert not kwargs
-            yield "ok"
-
-    monkeypatch.setattr(
-        reme_module,
-        "create_application_registry",
-        lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient),
-    )
+    seen = {}
+    _set_client_backend(monkeypatch, _recording_client(seen))
     monkeypatch.setattr(reme_module, "running_app_config", lambda: {"service": {"backend": "http"}})
 
     def fail_local_resolution(**_kwargs):
@@ -442,4 +378,6 @@ def test_call_server_skips_local_fallback_when_server_is_running(monkeypatch, ca
 
     asyncio.run(reme_module.call_server("version"))
 
+    assert seen["action"] == "version"
+    assert seen["payload"] == {}
     assert capsys.readouterr().out == "ok\n"