fix(plugin-system): implement register_driver_type and stop discover() aborting on one bad manifest
- driver/registry.py never actually defined register_driver_type, so the plugin-system's 'driver plugin registered successfully' scenario was unreachable in production (only simulated via test monkeypatch). Added a real implementation wired into the existing driver factory registry. - PluginRegistry.discover() only caught PluginValidationError/ DuplicatePluginError, so a driver-kind manifest that failed wiring (DriverRegistryUnavailableError/PluginTargetResolutionError) aborted the whole scan, silently skipping co-located tool/skill manifests. Now caught and skipped per manifest instead. openspec: plugin-system capability, archived change cloud-runtime
This commit is contained in:
+15
-2
@@ -95,6 +95,19 @@ class PluginTargetResolutionError(RuntimeError):
|
|||||||
"""Raised when a plugin target cannot be resolved to a callable."""
|
"""Raised when a plugin target cannot be resolved to a callable."""
|
||||||
|
|
||||||
|
|
||||||
|
# Exception types `register()` can raise for a single bad manifest (validation
|
||||||
|
# failure, name conflict, or driver-wiring failure). `discover()` catches only
|
||||||
|
# these so one bad manifest is skipped/logged, per spec's "malformed manifest
|
||||||
|
# file is skipped, not fatal" resilience intent -- anything else propagates as
|
||||||
|
# a genuine programmer error.
|
||||||
|
_PLUGIN_REGISTRATION_FAILURES = (
|
||||||
|
PluginValidationError,
|
||||||
|
DuplicatePluginError,
|
||||||
|
DriverRegistryUnavailableError,
|
||||||
|
PluginTargetResolutionError,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class PluginRegistry:
|
class PluginRegistry:
|
||||||
"""Validates, persists, and wires plugins discovered or submitted directly."""
|
"""Validates, persists, and wires plugins discovered or submitted directly."""
|
||||||
|
|
||||||
@@ -128,7 +141,7 @@ class PluginRegistry:
|
|||||||
try:
|
try:
|
||||||
self.register(manifest)
|
self.register(manifest)
|
||||||
result.registered.append(manifest)
|
result.registered.append(manifest)
|
||||||
except (PluginValidationError, DuplicatePluginError) as exc:
|
except _PLUGIN_REGISTRATION_FAILURES as exc:
|
||||||
result.errors.append(
|
result.errors.append(
|
||||||
f"entry-point plugin {manifest.name!r}: {exc}"
|
f"entry-point plugin {manifest.name!r}: {exc}"
|
||||||
)
|
)
|
||||||
@@ -137,7 +150,7 @@ class PluginRegistry:
|
|||||||
try:
|
try:
|
||||||
self.register(manifest)
|
self.register(manifest)
|
||||||
result.registered.append(manifest)
|
result.registered.append(manifest)
|
||||||
except (PluginValidationError, DuplicatePluginError) as exc:
|
except _PLUGIN_REGISTRATION_FAILURES as exc:
|
||||||
result.errors.append(
|
result.errors.append(
|
||||||
f"manifest-file plugin {manifest.name!r}: {exc}"
|
f"manifest-file plugin {manifest.name!r}: {exc}"
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -42,3 +42,23 @@ def build_driver_factory(
|
|||||||
if not builder:
|
if not builder:
|
||||||
raise ValueError(f"unsupported driver_type: {driver_type}")
|
raise ValueError(f"unsupported driver_type: {driver_type}")
|
||||||
return builder(connection_info)
|
return builder(connection_info)
|
||||||
|
|
||||||
|
|
||||||
|
def register_driver_type(driver_type: str, factory: DriverFactoryBuilder) -> None:
|
||||||
|
"""Register a new ``driver_type`` -> factory-builder mapping.
|
||||||
|
|
||||||
|
This is the extension point external code (e.g. ``cloud.plugins``'
|
||||||
|
driver-kind plugin wiring) uses to add a new driver type without editing
|
||||||
|
this module. ``factory`` must be a callable accepting a
|
||||||
|
``connection_info`` dict and returning a ``DriverFactory`` (the same
|
||||||
|
shape as :func:`build_wda_driver_factory`); once registered,
|
||||||
|
``build_driver_factory(driver_type, ...)`` can construct drivers of the
|
||||||
|
new type.
|
||||||
|
"""
|
||||||
|
if not driver_type:
|
||||||
|
raise ValueError("driver_type must be a non-empty string")
|
||||||
|
if not callable(factory):
|
||||||
|
raise ValueError(f"factory for driver_type {driver_type!r} must be callable")
|
||||||
|
if driver_type in SUPPORTED_DRIVER_TYPES:
|
||||||
|
raise ValueError(f"driver_type {driver_type!r} is already registered")
|
||||||
|
SUPPORTED_DRIVER_TYPES[driver_type] = factory
|
||||||
|
|||||||
+133
-24
@@ -15,6 +15,7 @@ from cloud.plugins import (
|
|||||||
DuplicatePluginError,
|
DuplicatePluginError,
|
||||||
PluginManifest,
|
PluginManifest,
|
||||||
PluginRegistry,
|
PluginRegistry,
|
||||||
|
PluginTargetResolutionError,
|
||||||
PluginValidationError,
|
PluginValidationError,
|
||||||
)
|
)
|
||||||
from cloud.store import CloudStore
|
from cloud.store import CloudStore
|
||||||
@@ -36,6 +37,15 @@ def _manifest(
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _fake_driver_factory_builder(connection_info: dict) -> object:
|
||||||
|
"""A real ``DriverFactoryBuilder``-shaped function used as a plugin target
|
||||||
|
in tests: takes ``connection_info``, returns a zero-arg factory callable
|
||||||
|
that "constructs" a driver (here, just the connection info itself, so
|
||||||
|
tests can assert on it without needing a real WDA server)."""
|
||||||
|
|
||||||
|
return lambda: dict(connection_info)
|
||||||
|
|
||||||
|
|
||||||
def test_valid_manifest_registers(tmp_path) -> None:
|
def test_valid_manifest_registers(tmp_path) -> None:
|
||||||
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
||||||
|
|
||||||
@@ -67,39 +77,33 @@ def test_duplicate_name_rejected(tmp_path) -> None:
|
|||||||
registry.register(_manifest(entry_point_kind="tool"))
|
registry.register(_manifest(entry_point_kind="tool"))
|
||||||
|
|
||||||
|
|
||||||
def test_driver_kind_wires_into_register_driver_type(
|
def test_driver_kind_wires_into_register_driver_type(tmp_path, monkeypatch) -> None:
|
||||||
tmp_path,
|
"""A driver-kind manifest is wired end-to-end into the real driver registry:
|
||||||
monkeypatch,
|
after registration, build_driver_factory() can construct the new type."""
|
||||||
) -> None:
|
|
||||||
"""A driver-kind manifest calls driver.registry.register_driver_type."""
|
|
||||||
|
|
||||||
calls: list[tuple[str, object]] = []
|
monkeypatch.delitem(
|
||||||
|
driver_registry_module.SUPPORTED_DRIVER_TYPES, "custom-driver", raising=False
|
||||||
def fake_register(name: str, builder) -> None:
|
|
||||||
calls.append((name, builder))
|
|
||||||
|
|
||||||
# driver/registry.py does not yet expose register_driver_type in the real
|
|
||||||
# codebase, so injecting it via monkeypatch simulates the future state
|
|
||||||
# where it does (per design.md D6 / Open Questions).
|
|
||||||
monkeypatch.setattr(
|
|
||||||
driver_registry_module,
|
|
||||||
"register_driver_type",
|
|
||||||
fake_register,
|
|
||||||
raising=False,
|
|
||||||
)
|
)
|
||||||
|
|
||||||
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
||||||
registry.register(_manifest(entry_point_kind="driver", name="custom-driver"))
|
registry.register(
|
||||||
|
_manifest(
|
||||||
assert len(calls) == 1
|
entry_point_kind="driver",
|
||||||
registered_name, registered_builder = calls[0]
|
name="custom-driver",
|
||||||
assert registered_name == "custom-driver"
|
target="tests.test_plugin_registry:_fake_driver_factory_builder",
|
||||||
assert callable(registered_builder)
|
)
|
||||||
|
)
|
||||||
|
|
||||||
stored = registry.store.get_plugin("custom-driver")
|
stored = registry.store.get_plugin("custom-driver")
|
||||||
assert stored is not None
|
assert stored is not None
|
||||||
assert stored[1] is True # wired
|
assert stored[1] is True # wired
|
||||||
|
|
||||||
|
factory = driver_registry_module.build_driver_factory(
|
||||||
|
"custom-driver", {"server_url": "http://example"}
|
||||||
|
)
|
||||||
|
driver = factory()
|
||||||
|
assert driver == {"server_url": "http://example"}
|
||||||
|
|
||||||
|
|
||||||
def test_driver_kind_raises_when_extension_point_missing(
|
def test_driver_kind_raises_when_extension_point_missing(
|
||||||
tmp_path,
|
tmp_path,
|
||||||
@@ -255,3 +259,108 @@ def test_discover_combines_entry_points_and_manifest_files(
|
|||||||
names = {m.name for m in result.registered}
|
names = {m.name for m in result.registered}
|
||||||
assert names == {"via-entry-point", "via-file"}
|
assert names == {"via-entry-point", "via-file"}
|
||||||
assert result.errors == []
|
assert result.errors == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_discover_does_not_abort_when_driver_wiring_fails(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch,
|
||||||
|
) -> None:
|
||||||
|
"""A driver-kind manifest that fails wiring must not abort the whole
|
||||||
|
discover() scan: a co-located tool manifest should still get registered,
|
||||||
|
and the failure should surface as a discovery error, not an exception."""
|
||||||
|
|
||||||
|
# Force the driver-registry extension point to look unavailable so the
|
||||||
|
# driver-kind manifest below fails wiring with DriverRegistryUnavailableError.
|
||||||
|
monkeypatch.delattr(
|
||||||
|
driver_registry_module,
|
||||||
|
"register_driver_type",
|
||||||
|
raising=False,
|
||||||
|
)
|
||||||
|
|
||||||
|
plugins_dir = tmp_path / "plugins"
|
||||||
|
(plugins_dir / "a-broken-driver").mkdir(parents=True)
|
||||||
|
(plugins_dir / "a-broken-driver" / "plugin.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"name": "broken-driver",
|
||||||
|
"version": "1.0.0",
|
||||||
|
"entry_point_kind": "driver",
|
||||||
|
"target": "cloud.store:CloudStore",
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
(plugins_dir / "b-ok-tool").mkdir(parents=True)
|
||||||
|
(plugins_dir / "b-ok-tool" / "plugin.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"name": "ok-tool",
|
||||||
|
"version": "1.0.0",
|
||||||
|
"entry_point_kind": "tool",
|
||||||
|
"target": "cloud.store:CloudStore",
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
|
||||||
|
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
||||||
|
result = registry.discover(scan_path=plugins_dir)
|
||||||
|
|
||||||
|
# The broken driver manifest is recorded as a discovery error, not raised.
|
||||||
|
assert any("broken-driver" in err for err in result.errors)
|
||||||
|
# The co-located tool manifest still registered despite the driver failure.
|
||||||
|
assert any(m.name == "ok-tool" for m in result.registered)
|
||||||
|
assert registry.store.get_plugin("ok-tool") is not None
|
||||||
|
assert registry.store.get_plugin("broken-driver") is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_discover_reports_target_resolution_failure_without_aborting(
|
||||||
|
tmp_path,
|
||||||
|
) -> None:
|
||||||
|
"""A driver-kind manifest whose target can't resolve to a callable
|
||||||
|
(PluginTargetResolutionError) is likewise skipped/logged, not fatal."""
|
||||||
|
|
||||||
|
plugins_dir = tmp_path / "plugins"
|
||||||
|
(plugins_dir / "bad-target-driver").mkdir(parents=True)
|
||||||
|
(plugins_dir / "bad-target-driver" / "plugin.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"name": "bad-target-driver",
|
||||||
|
"version": "1.0.0",
|
||||||
|
"entry_point_kind": "driver",
|
||||||
|
# os.path.sep exists but is not callable -> PluginTargetResolutionError
|
||||||
|
"target": "os.path:sep",
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
(plugins_dir / "ok-tool-2").mkdir(parents=True)
|
||||||
|
(plugins_dir / "ok-tool-2" / "plugin.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"name": "ok-tool-2",
|
||||||
|
"version": "1.0.0",
|
||||||
|
"entry_point_kind": "tool",
|
||||||
|
"target": "cloud.store:CloudStore",
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
|
||||||
|
registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3"))
|
||||||
|
|
||||||
|
with pytest.raises(PluginTargetResolutionError):
|
||||||
|
# Direct register() call still raises loudly for a single manifest...
|
||||||
|
registry._wire_driver(
|
||||||
|
_manifest(
|
||||||
|
name="direct-check",
|
||||||
|
entry_point_kind="driver",
|
||||||
|
target="os.path:sep",
|
||||||
|
)
|
||||||
|
)
|
||||||
|
|
||||||
|
result = registry.discover(scan_path=plugins_dir)
|
||||||
|
|
||||||
|
assert any("bad-target-driver" in err for err in result.errors)
|
||||||
|
assert any(m.name == "ok-tool-2" for m in result.registered)
|
||||||
|
assert registry.store.get_plugin("ok-tool-2") is not None
|
||||||
|
|||||||
Reference in New Issue
Block a user