diff --git a/features/plugins_loader_coverage.feature b/features/plugins_loader_coverage.feature index 87cd662d1..5ea201011 100644 --- a/features/plugins_loader_coverage.feature +++ b/features/plugins_loader_coverage.feature @@ -1,8 +1,9 @@ Feature: Plugin Loader Coverage Boost Scenarios targeting uncovered lines in the PluginLoader class: - Lines 203-209: entry point load failure exception handler - - Lines 242, 244-246: validate_protocol fallback to issubclass when instantiation fails - - Lines 247-248: validate_protocol issubclass raises TypeError + - validate_protocol: issubclass succeeds (class satisfies protocol structurally) + - validate_protocol: issubclass raises TypeError with unverifiable protocol + - validate_protocol: issubclass returns False (class missing required members) Background: Given the plugin loader module is imported @@ -18,17 +19,17 @@ Feature: Plugin Loader Coverage Boost And the failed entry point should have been logged as a warning # ----------------------------------------------------------------------- - # validate_protocol: instantiation fails, issubclass succeeds (lines 242, 244-246) + # validate_protocol: issubclass succeeds for class satisfying protocol # ----------------------------------------------------------------------- - Scenario: validate_protocol falls back to issubclass when instantiation fails + Scenario: validate_protocol returns True when class satisfies protocol via issubclass Given I have a class that requires constructor arguments And I have a runtime checkable protocol the class satisfies via issubclass When I call validate_protocol with the non-instantiable class and protocol Then validate_protocol should return True # ----------------------------------------------------------------------- - # validate_protocol: instantiation fails, issubclass raises TypeError (lines 247-248) + # validate_protocol: issubclass raises TypeError for unverifiable protocol # ----------------------------------------------------------------------- Scenario: validate_protocol raises ProtocolMismatchError when issubclass raises TypeError @@ -38,7 +39,7 @@ Feature: Plugin Loader Coverage Boost Then a plugin-loader ProtocolMismatchError should be raised # ----------------------------------------------------------------------- - # validate_protocol: instantiation fails, issubclass returns False + # validate_protocol: issubclass returns False (class missing required members) # ----------------------------------------------------------------------- Scenario: validate_protocol raises ProtocolMismatchError when issubclass returns False diff --git a/features/steps/plugins_loader_coverage_steps.py b/features/steps/plugins_loader_coverage_steps.py index 89cf3107f..456950f2f 100644 --- a/features/steps/plugins_loader_coverage_steps.py +++ b/features/steps/plugins_loader_coverage_steps.py @@ -4,9 +4,9 @@ These steps target specific uncovered lines in cleveragents/infrastructure/plugins/loader.py: - Lines 203-209: except block in load_from_entry_points when ep.load() fails -- Lines 242, 244-246: validate_protocol fallback to issubclass on - instantiation failure -- Lines 247-248: validate_protocol when issubclass raises TypeError +- validate_protocol: issubclass succeeds (class satisfies protocol structurally) +- validate_protocol: issubclass raises TypeError for unverifiable protocol +- validate_protocol: issubclass returns False (class missing required members) """ from typing import Protocol, runtime_checkable @@ -72,13 +72,13 @@ def step_verify_warning_logged(context): # --------------------------------------------------------------------------- -# validate_protocol: instantiation fails, issubclass succeeds (lines 242, 244-246) +# validate_protocol: issubclass succeeds for class satisfying protocol # --------------------------------------------------------------------------- @runtime_checkable class _SampleProtocol(Protocol): - """A simple runtime-checkable Protocol for testing issubclass fallback.""" + """A simple runtime-checkable Protocol for testing issubclass check.""" def do_work(self) -> str: ... @@ -105,7 +105,7 @@ def step_protocol_satisfied_by_subclass(context): @when("I call validate_protocol with the non-instantiable class and protocol") def step_call_validate_protocol_subclass_fallback(context): - """Call validate_protocol; expect it to fall back to issubclass and succeed.""" + """Call validate_protocol; issubclass succeeds and returns True.""" context.validate_result = PluginLoader.validate_protocol( context.non_instantiable_class, context.target_protocol, @@ -114,18 +114,18 @@ def step_call_validate_protocol_subclass_fallback(context): @then("validate_protocol should return True") def step_verify_validate_true(context): - """The fallback issubclass check should have returned True.""" + """The issubclass check should have returned True.""" assert context.validate_result is True # --------------------------------------------------------------------------- -# validate_protocol: instantiation fails, issubclass raises TypeError (lines 247-248) +# validate_protocol: issubclass raises TypeError for unverifiable protocol # --------------------------------------------------------------------------- @given("I have a class that cannot be instantiated without arguments") def step_class_cannot_instantiate(context): - """Create a class whose __init__ raises when called with no args.""" + """Create a class whose __init__ requires mandatory arguments.""" class NeedsArgs: def __init__(self, required): @@ -141,10 +141,10 @@ def step_protocol_causes_typeerror(context): We need an object that: - Has a __name__ attribute (so the error message in validate_protocol works) - Causes issubclass() to raise TypeError when used as second arg - - Also causes isinstance() to raise TypeError - A class with a metaclass that raises TypeError on __instancecheck__ - and __subclasscheck__ achieves this. + A class with a metaclass that raises TypeError on __subclasscheck__ + achieves this. Since this protocol declares no members, the structural + fallback cannot verify conformance and must raise ProtocolMismatchError. """ class TypeErrorMeta(type): @@ -186,7 +186,7 @@ def step_verify_protocol_mismatch(context): # --------------------------------------------------------------------------- -# validate_protocol: instantiation fails, issubclass returns False +# validate_protocol: issubclass returns False (class missing required members) # --------------------------------------------------------------------------- diff --git a/src/cleveragents/infrastructure/plugins/loader.py b/src/cleveragents/infrastructure/plugins/loader.py index 83329662d..3fac5a6bc 100644 --- a/src/cleveragents/infrastructure/plugins/loader.py +++ b/src/cleveragents/infrastructure/plugins/loader.py @@ -241,10 +241,9 @@ class PluginLoader: def validate_protocol(klass: type[Any], protocol: type[Any]) -> bool: """Check whether *klass* satisfies a ``@runtime_checkable`` Protocol. - Creates a temporary instance of *klass* (using a no-arg - constructor) and checks it against the protocol via - ``isinstance``. If instantiation fails, falls back to a - structural check using ``issubclass``. + Uses structural type checking via ``issubclass`` to validate the + protocol without instantiating the class. This prevents arbitrary + code execution from untrusted plugin constructors during validation. Args: klass: The class to validate. @@ -256,23 +255,84 @@ class PluginLoader: Raises: ProtocolMismatchError: If *klass* does not satisfy the protocol. """ - # Try instance check first (most reliable for runtime_checkable) + # Try structural check first (safe, no instantiation). + # This prevents arbitrary code execution from plugin constructors. + issubclass_raised_type_error = False try: - instance = klass() - if isinstance(instance, protocol): + if issubclass(klass, protocol): return True - except Exception: - # If instantiation fails, try subclass check - try: - if issubclass(klass, protocol): - return True - except TypeError: - pass + except TypeError: + # issubclass raises TypeError if protocol is not a valid + # @runtime_checkable Protocol (e.g. has a custom metaclass that + # overrides __subclasscheck__). Fall through to structural check. + issubclass_raised_type_error = True + + # Fallback: perform a conservative structural check against the + # Protocol definition without instantiating the class. We inspect + # the protocol's declared members (callables and annotated names) + # and ensure the candidate class exposes them as class-level + # attributes or descriptors. This approximates structural + # conformance while avoiding constructor execution. + required: set[str] = set() + + # Collect names from protocol __dict__ (methods, properties, etc.) + prot_dict = getattr(protocol, "__dict__", {}) + for name, _value in prot_dict.items(): + if name.startswith("__"): + continue + # Methods and descriptors will appear in the dict; annotations + # are handled below. We treat any non-data-magic name as a + # required member. + required.add(name) + + # Also include annotated names (those declared with type hints) + ann = getattr(protocol, "__annotations__", {}) or {} + for name in ann: + if name.startswith("__"): + continue + required.add(name) + + # If issubclass raised TypeError and the protocol declares no + # inspectable members, we cannot validate conformance — treat this + # as a mismatch rather than silently returning True for an + # unverifiable protocol. + if issubclass_raised_type_error and not required: + msg = ( + f"Class '{getattr(klass, '__name__', str(klass))}' cannot be " + f"validated against protocol " + f"'{getattr(protocol, '__name__', str(protocol))}': " + f"issubclass raised TypeError and the protocol declares no " + f"inspectable members. Ensure the protocol is a valid " + f"@runtime_checkable Protocol." + ) + raise ProtocolMismatchError(msg) + + # Validate that the candidate class exposes each required name. + missing: list[str] = [] + for name in sorted(required): + # getattr on the class returns descriptors (functions, property, + # staticmethod, etc.) without instantiation. hasattr is safe. + if not hasattr(klass, name): + missing.append(name) + continue + # If the protocol declared a callable (method), ensure the + # attribute on the class is callable or a descriptor. + prot_val = prot_dict.get(name) + if callable(prot_val): + cand = getattr(klass, name) + # Properties are descriptors and typically not callable; we + # accept them as present. For methods, require callable. + if not (callable(cand) or hasattr(cand, "fget")): + missing.append(name) + + if not missing: + return True msg = ( - f"Class '{klass.__name__}' does not satisfy protocol " - f"'{protocol.__name__}'. Ensure the class implements all " - f"required methods and attributes." + f"Class '{getattr(klass, '__name__', str(klass))}' does not " + f"satisfy protocol '{getattr(protocol, '__name__', str(protocol))}'. " + f"Missing attributes: {', '.join(missing)}. Ensure the class " + f"implements all required methods and descriptors." ) raise ProtocolMismatchError(msg)