From 725bf4cd9ce346e9df4306084f557dd5f17b7985 Mon Sep 17 00:00:00 2001 From: Jerry Yan <792602257@qq.com> Date: Tue, 7 Jul 2026 08:31:00 +0800 Subject: [PATCH] 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 --- cloud/plugins.py | 17 +++- driver/registry.py | 20 +++++ tests/test_plugin_registry.py | 157 ++++++++++++++++++++++++++++------ 3 files changed, 168 insertions(+), 26 deletions(-) diff --git a/cloud/plugins.py b/cloud/plugins.py index e5d9a96..186b631 100644 --- a/cloud/plugins.py +++ b/cloud/plugins.py @@ -95,6 +95,19 @@ class PluginTargetResolutionError(RuntimeError): """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: """Validates, persists, and wires plugins discovered or submitted directly.""" @@ -128,7 +141,7 @@ class PluginRegistry: try: self.register(manifest) result.registered.append(manifest) - except (PluginValidationError, DuplicatePluginError) as exc: + except _PLUGIN_REGISTRATION_FAILURES as exc: result.errors.append( f"entry-point plugin {manifest.name!r}: {exc}" ) @@ -137,7 +150,7 @@ class PluginRegistry: try: self.register(manifest) result.registered.append(manifest) - except (PluginValidationError, DuplicatePluginError) as exc: + except _PLUGIN_REGISTRATION_FAILURES as exc: result.errors.append( f"manifest-file plugin {manifest.name!r}: {exc}" ) diff --git a/driver/registry.py b/driver/registry.py index 3f180c4..8237c60 100644 --- a/driver/registry.py +++ b/driver/registry.py @@ -42,3 +42,23 @@ def build_driver_factory( if not builder: raise ValueError(f"unsupported driver_type: {driver_type}") 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 diff --git a/tests/test_plugin_registry.py b/tests/test_plugin_registry.py index 02a841a..52cc1ba 100644 --- a/tests/test_plugin_registry.py +++ b/tests/test_plugin_registry.py @@ -15,6 +15,7 @@ from cloud.plugins import ( DuplicatePluginError, PluginManifest, PluginRegistry, + PluginTargetResolutionError, PluginValidationError, ) 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: 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")) -def test_driver_kind_wires_into_register_driver_type( - tmp_path, - monkeypatch, -) -> None: - """A driver-kind manifest calls driver.registry.register_driver_type.""" +def test_driver_kind_wires_into_register_driver_type(tmp_path, monkeypatch) -> None: + """A driver-kind manifest is wired end-to-end into the real driver registry: + after registration, build_driver_factory() can construct the new type.""" - calls: list[tuple[str, object]] = [] - - 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, + monkeypatch.delitem( + driver_registry_module.SUPPORTED_DRIVER_TYPES, "custom-driver", raising=False ) registry = PluginRegistry(CloudStore(tmp_path / "cloud.sqlite3")) - registry.register(_manifest(entry_point_kind="driver", name="custom-driver")) - - assert len(calls) == 1 - registered_name, registered_builder = calls[0] - assert registered_name == "custom-driver" - assert callable(registered_builder) + registry.register( + _manifest( + entry_point_kind="driver", + name="custom-driver", + target="tests.test_plugin_registry:_fake_driver_factory_builder", + ) + ) stored = registry.store.get_plugin("custom-driver") assert stored is not None 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( tmp_path, @@ -255,3 +259,108 @@ def test_discover_combines_entry_points_and_manifest_files( names = {m.name for m in result.registered} assert names == {"via-entry-point", "via-file"} 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