From 629246ad8a650049869c942f2bbe7466dd460d8d Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 23 Sep 2026 18:38:13 +0200 Subject: [PATCH 1/2] Validate all hookimpls before registering a plugin register() stored the plugin and installed each hookimpl while walking the plugin, so a PluginValidationError on a later hookimpl left the plugin registered with its earlier hookimpls and hook callers in place. Split registration into two passes: collect and verify every hookimpl without touching state, then store the plugin, create missing hook callers, replay history and add the impls. A validation failure now leaves the manager unchanged and the fixed plugin can be registered. Fixes #733 Co-Authored-By: Claude Opus 5.5 (1M context) via Claude Code --- changelog/733.bugfix.rst | 1 + src/pluggy/_manager.py | 25 ++++++++++++++----------- testing/test_pluginmanager.py | 27 +++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 11 deletions(-) create mode 100644 changelog/733.bugfix.rst diff --git a/changelog/733.bugfix.rst b/changelog/733.bugfix.rst new file mode 100644 index 00000000..ba4902c3 --- /dev/null +++ b/changelog/733.bugfix.rst @@ -0,0 +1 @@ +:meth:`PluginManager.register() ` now validates all hook implementations of a plugin before registering any of them, so a plugin that fails validation no longer stays partially registered. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 080ef333..893b2a13 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -215,11 +215,9 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: f"{plugin_name}={plugin}\n{self._name2plugin}" ) - # XXX if an error happens we should make sure no state has been - # changed at point of return - self._name2plugin[plugin_name] = plugin - - # register matching hook implementations of the plugin + # Validate every hookimpl before changing any state, so a plugin that + # fails validation leaves the manager untouched. + hookimpls: list[tuple[str, HookImpl]] = [] for attr_name in dir(plugin): hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) if hookimpl_opts is not None: @@ -234,13 +232,18 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) hook_name = hookimpl_opts.get("specname") or attr_name hook: HookCaller | None = getattr(self.hook, hook_name, None) - if hook is None: - hook = HookCaller(hook_name, self._hookexec) - setattr(self.hook, hook_name, hook) - elif hook.has_spec(): + if hook is not None and hook.has_spec(): self._verify_hook(hook, hookimpl) - hook._maybe_apply_history(hookimpl) - hook._add_hookimpl(hookimpl) + hookimpls.append((hook_name, hookimpl)) + + self._name2plugin[plugin_name] = plugin + for hook_name, hookimpl in hookimpls: + hook = getattr(self.hook, hook_name, None) + if hook is None: + hook = HookCaller(hook_name, self._hookexec) + setattr(self.hook, hook_name, hook) + hook._maybe_apply_history(hookimpl) + hook._add_hookimpl(hookimpl) return plugin_name def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None: diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 65b322d4..3315ef6f 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -371,6 +371,33 @@ def he_method1(self, qlwkje): assert excinfo.value.plugin is plugin +def test_register_validation_failure_leaves_no_state(he_pm: PluginManager) -> None: + """The invalid impl sorts last in dir(), after a valid impl and an impl + for an unknown hook; a failed register must install none of them (#733).""" + + class hello: + @hookimpl + def a_new_hook(self): + pass # pragma: no cover + + @hookimpl + def he_method1(self, arg): + pass # pragma: no cover + + @hookimpl(specname="he_method1") + def he_method1_invalid(self, qlwkje): + pass # pragma: no cover + + plugin = hello() + + with pytest.raises(PluginValidationError) as excinfo: + he_pm.register(plugin) + assert excinfo.value.plugin is plugin + assert not he_pm.is_registered(plugin) + assert he_pm.hook.he_method1.get_hookimpls() == [] + assert not hasattr(he_pm.hook, "a_new_hook") + + def test_register_hookwrapper_not_a_generator_function(he_pm: PluginManager) -> None: class hello: @hookimpl(hookwrapper=True) From a4df832005f9cc7a46546d70c394b756267d84e9 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Wed, 23 Sep 2026 18:45:33 +0200 Subject: [PATCH 2/2] Verify hookimpls whose spec appears during historic replay The first pass verifies before any historic replay runs. If replaying one impl calls add_hookspecs for a hook that a later impl of the same plugin implements, that impl was never checked. The single-pass loop caught this whenever dir() order put the replayed impl first. Verify such impls in the second pass, before they are added. Co-Authored-By: Claude Opus 5.5 (1M context) via Claude Code --- src/pluggy/_manager.py | 13 ++++++++++--- testing/test_pluginmanager.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 893b2a13..8a0959f7 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -217,7 +217,7 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: # Validate every hookimpl before changing any state, so a plugin that # fails validation leaves the manager untouched. - hookimpls: list[tuple[str, HookImpl]] = [] + hookimpls: list[tuple[str, HookImpl, bool]] = [] for attr_name in dir(plugin): hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) if hookimpl_opts is not None: @@ -232,16 +232,23 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) hook_name = hookimpl_opts.get("specname") or attr_name hook: HookCaller | None = getattr(self.hook, hook_name, None) + verified = False if hook is not None and hook.has_spec(): self._verify_hook(hook, hookimpl) - hookimpls.append((hook_name, hookimpl)) + verified = True + hookimpls.append((hook_name, hookimpl, verified)) self._name2plugin[plugin_name] = plugin - for hook_name, hookimpl in hookimpls: + for hook_name, hookimpl, verified in hookimpls: hook = getattr(self.hook, hook_name, None) if hook is None: hook = HookCaller(hook_name, self._hookexec) setattr(self.hook, hook_name, hook) + elif not verified and hook.has_spec(): + # An earlier impl's historic replay added this spec after the + # first pass. Failing here leaves the plugin partially + # registered, but only user code during replay can get here. + self._verify_hook(hook, hookimpl) hook._maybe_apply_history(hookimpl) hook._add_hookimpl(hookimpl) return plugin_name diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 3315ef6f..07d11b4e 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -398,6 +398,36 @@ def he_method1_invalid(self, qlwkje): assert not hasattr(he_pm.hook, "a_new_hook") +def test_register_verifies_spec_added_by_historic_replay(pm: PluginManager) -> None: + """A spec added while replaying one impl's historic call must still be + checked for the impls registered after it.""" + + class Specs: + @hookspec(historic=True) + def configure(self): + pass # pragma: no cover + + class LateSpecs: + @hookspec + def late(self, arg): + pass # pragma: no cover + + pm.add_hookspecs(Specs) + pm.hook.configure.call_historic() + + class hello: + @hookimpl + def configure(self): + pm.add_hookspecs(LateSpecs) + + @hookimpl + def late(self, qlwkje): + pass # pragma: no cover + + with pytest.raises(PluginValidationError, match="qlwkje"): + pm.register(hello()) + + def test_register_hookwrapper_not_a_generator_function(he_pm: PluginManager) -> None: class hello: @hookimpl(hookwrapper=True)