Skip to content

Commit 3e1a1a7

Browse files
Homeclaude
authored andcommitted
fix(plugins): guard the attribute-based fallback the same way
Addresses review feedback on the name-based fix. PredBatPlugin sets PREDBAT_PLUGIN = True as its own discovery marker, so it matches strategy 2 in every plugin that does `from plugin_system import PredBatPlugin`. Guarding only strategy 1 left the identical bug 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 no hooks registered. Same __module__ check, same reason. Also replaces the redundant hasattr/getattr pair with a single getattr default. The new test needs three properties together, and getting any of them wrong hides the bug: the file must end "_plugin.py" or discovery never reads it; the class must not end in "Plugin" so strategy 1 declines; and the class must sort AFTER "PredBatPlugin", because inspect.getmembers() is alphabetical and strategy 2 breaks on its first match. A first draft named the class AttributeOnlyWorker, which sorts before the base and so passed with and without the fix. Verified failing without the guard ("fallback instantiated PredBatPlugin, expected ZzAttributeWorker") and passing with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVDP1s2fmBMFmbHNAirnUt
1 parent 0666c8c commit 3e1a1a7

3 files changed

Lines changed: 61 additions & 3 deletions

File tree

‎apps/predbat/plugin_system.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,19 @@ def load_plugin(self, plugin_dir: str, plugin_name: str):
138138
except Exception as e:
139139
self.log(f"Failed to initialise plugin class {name}: {e}")
140140

141-
# 2. FALLBACK: Class has PREDBAT_PLUGIN = True attribute
141+
# 2. FALLBACK: Class has PREDBAT_PLUGIN = True attribute AND is defined in
142+
# this module. The same __module__ guard as strategy 1, and for the same
143+
# reason: PredBatPlugin itself sets PREDBAT_PLUGIN = True as its discovery
144+
# marker, so every plugin that does `from plugin_system import PredBatPlugin`
145+
# has a matching class sitting in its namespace. Without the guard this
146+
# fallback re-introduces exactly the bug strategy 1 just fixed, and it is
147+
# reachable in the two cases where strategy 1 declines: the plugin class does
148+
# not end in "Plugin", or it raised during __init__ and left plugin_instance
149+
# None. Both end with the abstract base constructed, the plugin logged as
150+
# loaded, and nothing registered.
142151
if plugin_instance is None:
143152
for name, obj in inspect.getmembers(plugin_module, inspect.isclass):
144-
if hasattr(obj, "PREDBAT_PLUGIN") and getattr(obj, "PREDBAT_PLUGIN"):
153+
if getattr(obj, "PREDBAT_PLUGIN", False) and obj.__module__ == plugin_module.__name__:
145154
try:
146155
self.log(f"Initialising plugin class (attribute-based): {name}")
147156
plugin_instance = obj(self.base)

‎apps/predbat/tests/test_plugin_startup.py‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,3 +188,51 @@ def test_plugin_discovery_skips_imported_base_class(my_predbat):
188188
shutil.rmtree(tmpdir, ignore_errors=True)
189189

190190
return failed
191+
192+
193+
def test_plugin_discovery_fallback_skips_imported_base_class(my_predbat):
194+
"""
195+
The SAME guard must hold on the attribute-based fallback (strategy 2).
196+
197+
PredBatPlugin sets PREDBAT_PLUGIN = True as its own discovery marker, so it
198+
matches strategy 2 in every plugin that imports it. Guarding only the
199+
name-based strategy leaves the identical bug reachable whenever strategy 1
200+
declines — here the plugin class does not end in "Plugin", so discovery falls
201+
straight through to the fallback and, unguarded, constructs the abstract base:
202+
plugin logged as loaded, no hooks registered, silently does nothing.
203+
"""
204+
print("*** Running test: Plugin discovery fallback skips imported base classes")
205+
failed = 0
206+
207+
base = MagicMock()
208+
base.log = MagicMock()
209+
210+
tmpdir = tempfile.mkdtemp()
211+
try:
212+
# Three properties here are all load-bearing:
213+
# - the FILE ends "_plugin.py", or discover_plugins never reads it;
214+
# - the CLASS does not end in "Plugin", so strategy 1 finds nothing and
215+
# discovery falls through to the PREDBAT_PLUGIN attribute match;
216+
# - the CLASS sorts AFTER "PredBatPlugin", because inspect.getmembers()
217+
# is alphabetical and strategy 2 breaks on its first match. A name
218+
# sorting before the base hides the bug entirely - which is how the
219+
# first draft of this test passed with and without the fix.
220+
with open(os.path.join(tmpdir, "attribute_only_plugin.py"), "w", encoding="utf-8") as fh:
221+
fh.write("from plugin_system import PredBatPlugin\n\n\nclass ZzAttributeWorker(PredBatPlugin):\n pass\n")
222+
223+
plugin_system = PluginSystem(base)
224+
plugin_system.discover_plugins([tmpdir])
225+
loaded = list(plugin_system.plugins.values())
226+
227+
if len(loaded) != 1:
228+
print("ERROR: expected exactly one discovered plugin, got {}".format(len(loaded)))
229+
failed = 1
230+
elif type(loaded[0]).__name__ != "ZzAttributeWorker":
231+
print("ERROR: fallback instantiated {}, expected ZzAttributeWorker".format(type(loaded[0]).__name__))
232+
failed = 1
233+
else:
234+
print("OK: attribute-based fallback instantiated the plugin class, not the imported base")
235+
finally:
236+
shutil.rmtree(tmpdir, ignore_errors=True)
237+
238+
return failed

‎apps/predbat/unit_test.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
from tests.test_nordpool import run_nordpool_test
5050
from tests.test_futurerate_auto import test_futurerate_auto
5151
from tests.test_car_charging_smart import run_car_charging_smart_tests
52-
from tests.test_plugin_startup import test_plugin_startup_order, test_plugin_discovery_skips_imported_base_class
52+
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
5353
from tests.test_active_flag import test_active_flag
5454
from tests.test_component_health_status import test_component_health_status
5555
from tests.test_optimise_levels import run_optimise_levels_tests
@@ -399,6 +399,7 @@ def main():
399399
("octopus_slots_change", test_octopus_slots_change, "Octopus slots change-detection signature tests (in-progress re-clock vs genuine change)", False),
400400
("plugin_startup", test_plugin_startup_order, "Plugin startup order tests", False),
401401
("plugin_discovery", test_plugin_discovery_skips_imported_base_class, "Plugin discovery skips imported base classes", False),
402+
("plugin_discovery_fallback", test_plugin_discovery_fallback_skips_imported_base_class, "Plugin discovery fallback skips imported base classes", False),
402403
("active_flag", test_active_flag, "Active flag cleared on exception tests", False),
403404
("component_health_status", test_component_health_status, "Component errors fail the recorded run status tests", False),
404405
("dynamic_load_car", test_dynamic_load_car_slot_cancellation, "Dynamic load car slot cancellation tests", False),

0 commit comments

Comments
 (0)