fix(plugins): discovery must not instantiate an imported base class

apps/predbat/plugin_system.py

@@ -121,20 +121,36 @@ def load_plugin(self, plugin_dir: str, plugin_name: str):
 
         # Plugin discovery fallthrough (try in order of preference):
 
-        # 1. MOST PREFERRED: Class name ends with "Plugin"
+        # 1. MOST PREFERRED: Class name ends with "Plugin" AND is defined in this
+        # module. inspect.getmembers() returns members ALPHABETICALLY, so without the
+        # __module__ check a plugin whose class sorts after an imported one silently
+        # gets the wrong class instantiated: `from plugin_system import PredBatPlugin`
+        # puts the base class in the namespace, so e.g. SocKeepPublishPlugin (sorting
+        # after PredBatPlugin) had the abstract base constructed instead — the plugin
+        # logged as loaded successfully and did nothing. Whether a plugin works should
+        # not depend on the alphabet.
         for name, obj in inspect.getmembers(plugin_module, inspect.isclass):
-            if name.endswith("Plugin"):
+            if name.endswith("Plugin") and obj.__module__ == plugin_module.__name__:
                 try:
                     self.log(f"Initialising plugin class (name-based): {name}")
                     plugin_instance = obj(self.base)
                     break
                 except Exception as e:
                     self.log(f"Failed to initialise plugin class {name}: {e}")
 
-        # 2. FALLBACK: Class has PREDBAT_PLUGIN = True attribute
+        # 2. FALLBACK: Class has PREDBAT_PLUGIN = True attribute AND is defined in
+        # this module. The same __module__ guard as strategy 1, and for the same
+        # reason: PredBatPlugin itself sets PREDBAT_PLUGIN = True as its discovery
+        # marker, so every plugin that does `from plugin_system import PredBatPlugin`
+        # has a matching class sitting in its namespace. Without the guard this
+        # fallback re-introduces exactly the bug strategy 1 just fixed, and it is
+        # reachable in the two cases where strategy 1 declines: the plugin class does
+        # not end in "Plugin", or it raised during __init__ and left plugin_instance
+        # None. Both end with the abstract base constructed, the plugin logged as
+        # loaded, and nothing registered.
         if plugin_instance is None:
             for name, obj in inspect.getmembers(plugin_module, inspect.isclass):
-                if hasattr(obj, "PREDBAT_PLUGIN") and getattr(obj, "PREDBAT_PLUGIN"):
+                if getattr(obj, "PREDBAT_PLUGIN", False) and obj.__module__ == plugin_module.__name__:
                     try:
                         self.log(f"Initialising plugin class (attribute-based): {name}")
                         plugin_instance = obj(self.base)

apps/predbat/tests/test_plugin_startup.py

@@ -7,8 +7,12 @@
 # pylint: disable=consider-using-f-string
 # pylint: disable=line-too-long
 # pylint: disable=attribute-defined-outside-init
+import os
+import shutil
+import tempfile
 from unittest.mock import MagicMock, patch
 from predbat import PredBat
+from plugin_system import PluginSystem
 
 
 def test_plugin_startup_order(my_predbat):
@@ -143,3 +147,92 @@ def register_hooks(self, plugin_system):
         print(f"OK: Plugin successfully registered endpoint /test")
 
     return failed
+
+
+def test_plugin_discovery_skips_imported_base_class(my_predbat):
+    """
+    Discovery must instantiate the class DEFINED in the plugin file, not a class
+    it merely imports.
+
+    inspect.getmembers() returns members alphabetically and the name-based strategy
+    takes the first class ending in "Plugin". Every plugin does
+    `from plugin_system import PredBatPlugin`, so any plugin class sorting AFTER
+    "PredBatPlugin" had the abstract base instantiated instead of itself — the
+    plugin logged as loaded successfully, registered no hooks, and did nothing.
+    """
+    print("*** Running test: Plugin discovery skips imported base classes")
+    failed = 0
+
+    base = MagicMock()
+    base.log = MagicMock()
+
+    tmpdir = tempfile.mkdtemp()
+    try:
+        # Class name deliberately sorts AFTER "PredBatPlugin".
+        with open(os.path.join(tmpdir, "zz_sorts_last_plugin.py"), "w", encoding="utf-8") as fh:
+            fh.write("from plugin_system import PredBatPlugin\n\n\nclass ZzSortsLastPlugin(PredBatPlugin):\n    pass\n")
+
+        plugin_system = PluginSystem(base)
+        plugin_system.discover_plugins([tmpdir])
+        loaded = list(plugin_system.plugins.values())
+
+        if len(loaded) != 1:
+            print("ERROR: expected exactly one discovered plugin, got {}".format(len(loaded)))
+            failed = 1
+        elif type(loaded[0]).__name__ != "ZzSortsLastPlugin":
+            print("ERROR: discovery instantiated {}, expected ZzSortsLastPlugin".format(type(loaded[0]).__name__))
+            failed = 1
+        else:
+            print("OK: discovery instantiated the plugin class, not the imported base")
+    finally:
+        shutil.rmtree(tmpdir, ignore_errors=True)
+
+    return failed
+
+
+def test_plugin_discovery_fallback_skips_imported_base_class(my_predbat):
+    """
+    The SAME guard must hold on the attribute-based fallback (strategy 2).
+
+    PredBatPlugin sets PREDBAT_PLUGIN = True as its own discovery marker, so it
+    matches strategy 2 in every plugin that imports it. Guarding only the
+    name-based strategy leaves the identical bug reachable whenever strategy 1
+    declines — here the plugin class does not end in "Plugin", so discovery falls
+    straight through to the fallback and, unguarded, constructs the abstract base:
+    plugin logged as loaded, no hooks registered, silently does nothing.
+    """
+    print("*** Running test: Plugin discovery fallback skips imported base classes")
+    failed = 0
+
+    base = MagicMock()
+    base.log = MagicMock()
+
+    tmpdir = tempfile.mkdtemp()
+    try:
+        # Three properties here are all load-bearing:
+        #   - the FILE ends "_plugin.py", or discover_plugins never reads it;
+        #   - the CLASS does not end in "Plugin", so strategy 1 finds nothing and
+        #     discovery falls through to the PREDBAT_PLUGIN attribute match;
+        #   - the CLASS sorts AFTER "PredBatPlugin", because inspect.getmembers()
+        #     is alphabetical and strategy 2 breaks on its first match. A name
+        #     sorting before the base hides the bug entirely - which is how the
+        #     first draft of this test passed with and without the fix.
+        with open(os.path.join(tmpdir, "attribute_only_plugin.py"), "w", encoding="utf-8") as fh:
+            fh.write("from plugin_system import PredBatPlugin\n\n\nclass ZzAttributeWorker(PredBatPlugin):\n    pass\n")
+
+        plugin_system = PluginSystem(base)
+        plugin_system.discover_plugins([tmpdir])
+        loaded = list(plugin_system.plugins.values())
+
+        if len(loaded) != 1:
+            print("ERROR: expected exactly one discovered plugin, got {}".format(len(loaded)))
+            failed = 1
+        elif type(loaded[0]).__name__ != "ZzAttributeWorker":
+            print("ERROR: fallback instantiated {}, expected ZzAttributeWorker".format(type(loaded[0]).__name__))
+            failed = 1
+        else:
+            print("OK: attribute-based fallback instantiated the plugin class, not the imported base")
+    finally:
+        shutil.rmtree(tmpdir, ignore_errors=True)
+
+    return failed

apps/predbat/unit_test.py

@@ -65,7 +65,7 @@
 from tests.test_futurerate_auto import test_futurerate_auto
 from tests.test_car_charging_smart import run_car_charging_smart_tests
 from tests.test_battery_accuracy import run_battery_accuracy_tests
-from tests.test_plugin_startup import test_plugin_startup_order
+from tests.test_plugin_startup import test_plugin_startup_order, test_plugin_discovery_skips_imported_base_class, test_plugin_discovery_fallback_skips_imported_base_class
 from tests.test_active_flag import test_active_flag
 from tests.test_time_loop_guard import test_time_loop_guard
 from tests.test_component_health_status import test_component_health_status, test_record_status_state_clamped
@@ -520,6 +520,8 @@ def main():
         ("load_free_slot", test_load_free_slot, "Load free slot tests", False),
         ("octopus_slots_change", test_octopus_slots_change, "Octopus slots change-detection signature tests (in-progress re-clock vs genuine change)", False),
         ("plugin_startup", test_plugin_startup_order, "Plugin startup order tests", False),
+        ("plugin_discovery", test_plugin_discovery_skips_imported_base_class, "Plugin discovery skips imported base classes", False),
+        ("plugin_discovery_fallback", test_plugin_discovery_fallback_skips_imported_base_class, "Plugin discovery fallback skips imported base classes", False),
         ("active_flag", test_active_flag, "Active flag cleared on exception tests", False),
         ("time_loop_guard", test_time_loop_guard, "Run-loop guard reports a missing HA interface and sets fatal_error (#5135)", False),
         ("component_health_status", test_component_health_status, "Component errors fail the recorded run status tests", False),