PluginManager: harden loading (typed errors, per-manager state, cycle guard, 100% test coverage) - #3321
Open
geraldo-netto wants to merge 8 commits into
Open
Conversation
load_plugin caught LoadException and discarded it with `pass`, both for the named-plugin path and the autoload loop, so a plugin skipped due to a conflict or lower priority vanished with no trace. Log a warning naming the plugin and the reason. Add a regression test that asserts the warning fires and the plugin stays unloaded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
__load_plugin and unload_plugin raised bare `Exception(...)` for an unsatisfiable dependency and for unloading a non-unloadable plugin, so callers could not distinguish these from any other failure. Add `PluginError` and `PluginDependencyError` (both under `PluginException`) and raise them at the two sites. Tests assert the specific types and the subclass relationship. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Plugin lookup relied solely on __getattr__ magic (`Plugins.SomeName`), which is opaque to IDEs, type checkers, and refactoring tools. Add a typed `get_plugin(name) -> _T` accessor returning the loaded instance (KeyError if absent). __getattr__ is kept for backward compatibility with existing call sites. Tests cover the accessor, its equivalence to attribute access, and the missing-plugin case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Loading mutated the shared class attribute `cls.__unloadable__ = False` when a non-unloadable plugin depended on it. Because the attribute lives on the class, two PluginManager instances (applet vs mechanism) or a reload bled this state across each other, making unloadability global instead of per-manager. Track unloadability in a per-instance `__unloadable` dict, seeded from each class's declared default during discovery, and route every read through a new `is_unloadable(name)` accessor (load gate, bmexit guard, unload guard, PersistentPluginManager config handler, and the PluginDialog activatable flag). The class attribute is now never mutated. `self.__classes` remains the per-manager registry; the `__subclasses__()` discovery mechanism is unchanged (explicit registration is a larger, separate change — cross-ref ext-1). Tests assert per-instance flags, that the class attribute stays intact, and that the dependency-demotion rule is per-manager. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
__load_plugin was ~43 lines with 15+ conditionals mixing dependency resolution, conflict/priority arbitration, and activation (cyclomatic complexity 15). Extract a PluginDependencyResolver that owns the dependency/conflict graph as pure queries (`required`, `conflicts`), and split __load_plugin into three focused stages: __load_dependencies, __resolve_conflicts, and __activate. __load_plugin drops to complexity 3; each stage stays at or below 6. Behavior is unchanged. Tests cover the resolver queries, dependency load ordering, higher- priority conflict eviction, no-reload-when-loaded, and both activation failure paths (re-raise vs bmexit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plugin-load pipeline was hard-wired into PluginManager, so adding a new plugin type or an asynchronous loader meant editing the manager. Introduce a `LoadStrategy` seam with a default synchronous implementation (`DefaultLoadStrategy`) that drives the manager's now-public pipeline hooks: `is_loaded`, `load_dependencies`, `resolve_conflicts`, and `activate`. __load_plugin delegates to the configured strategy, which can be supplied via a new optional `load_strategy` constructor argument. Behavior is unchanged with the default strategy. Tests cover custom-strategy override, the default activation path, the abstract base contract, the is_loaded hook, plus broader coverage of unload (recursion, refusal), discovery import failures, and PersistentPluginManager config handling, with a fuzz pass over deep dependency chains and conflict-priority matrices. Module coverage 85%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up hardening of the plugin manager: - Dependency cycle guard: __load_plugin recursed into dependencies before marking the plugin loaded, so a cycle (A->B->A or a self-dependency) ran to a RecursionError. Track in-progress names and raise PluginDependencyError when a dependency is re-entered, clearing the tracking set on every exit. - Reduce load_plugin complexity from 24 to under the project's threshold by extracting __load_named_plugin, __import_plugin_modules, __register_classes, and __autoload_classes. Behavior unchanged. - Comprehensive tests bring module coverage to 100%: cycles (direct, self, cleanup), diamond dependencies, load/unload signals, reload, protocol filtering, __getattr__ fallback, conflict higher-priority skip, user-action ErrorDialog path, full discovery (dangling deps and autoload conflicts), and PersistentPluginManager config handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the per-item handling into __apply_config_state and __enable_from_config so on_property_changed is a flat loop. Drops the method's cognitive complexity from 17 to within the 15 limit; behavior is unchanged and the existing config-handling tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens
blueman/main/PluginManager.py— correctness, observability, decoupling, and complexity fixes plus a previously-missing test suite. Behavior is unchanged for the normal load path; each change closes a real gap or a latent bug. New module test coverage is 100% (52 unit + fuzz cases).Changes
load_plugincaughtLoadExceptionand discarded it withpass(named-plugin path and autoload loop), so a plugin skipped for a conflict or lower priority vanished silently. Now logs a warning naming the plugin and reason.__load_pluginandunload_pluginraised bareException(...)for an unsatisfiable dependency and for unloading a non-unloadable plugin. IntroducesPluginErrorandPluginDependencyError(underPluginException) so callers can distinguish them.get_plugin(name) -> _Taccessor; plugin lookup previously relied solely on__getattr__magic, which is opaque to IDEs and type checkers.__getattr__is kept for backward compatibility.cls.__unloadable__ = False, so twoPluginManagerinstances (applet vs mechanism) or a reload bled unloadability across each other. Tracks it in a per-instance dict via a newis_unloadable(name)accessor; the class attribute is never mutated. Updates thePluginDialogand config-change readers accordingly.PluginDependencyResolver(dependency/conflict graph queries) and splits__load_pluginintoload_dependencies/resolve_conflicts/activate.__load_plugindrops from cyclomatic 15 to 3;load_plugindiscovery is split into__import_plugin_modules/__register_classes/__autoload_classes.LoadStrategyseam with a default synchronous implementation, so alternative loaders (new plugin types, async) can be supplied via a new optionalload_strategyconstructor argument without editing the manager.__load_pluginrecursed into dependencies before marking the plugin loaded, so a cycle (A->B->Aor a self-dependency) ran to aRecursionError. Tracks in-progress names and raisesPluginDependencyErroron re-entry.Testing
The module had no dedicated tests before this PR. Adds
test/main/test_plugin_manager.py(registered intest/main/Makefile.am): dependency ordering (including a fuzz pass over chains of depth 1–11), diamond dependencies, dependency cycles, conflict/priority arbitration, load/unload/reload lifecycle and GObject signals, the dependency resolver and load-strategy seam,PersistentPluginManagerconfig handling, and discovery edge cases (dangling dependencies, autoload conflicts, import failures).mypy --strictclean; pycodestyle + pyflakes clean; all functions cyclomatic complexity ≤ 14🤖 Generated with Claude Code