Validate all hookimpls before registering a plugin - #741
Merged
RonnyPfannschmidt merged 2 commits intoSep 25, 2026
Merged
Conversation
2 tasks done
bluetech
approved these changes
Sep 24, 2026
|
|
||
|
|
||
| def test_register_validation_failure_leaves_no_state(he_pm: PluginManager) -> None: | ||
| """dir() order puts the valid impls and the unknown hook before the |
Member
There was a problem hiding this comment.
A bit hard to parse. Better not to refer to "old pass" if it's unexplained.
| assert he_pm.hook.he_method1.get_hookimpls() == [] | ||
| assert not hasattr(he_pm.hook, "a_new_hook") | ||
|
|
||
| del hello.he_method1_invalid |
Member
There was a problem hiding this comment.
This is a bit weird. Is this section necessary for the test?
|
|
||
|
|
||
| def test_register_verifies_spec_added_by_historic_replay(pm: PluginManager) -> None: | ||
| """The first pass verifies before any replay runs, so a spec added by |
Member
There was a problem hiding this comment.
The "pass" parts are referring to the implementation, which can change. I think can just drop the "The first pass verifies before any replay runs, so" part.
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 pytest-dev#733 Co-Authored-By: Claude Opus 5.5 (1M context) via Claude Code <noreply@anthropic.com>
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 <noreply@anthropic.com>
RonnyPfannschmidt
force-pushed
the
register-two-pass
branch
from
September 24, 2026 17:44
3a4c997 to
a4df832
Compare
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
2 participants
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.
Written by Claude Opus 5.5 via Claude Code for the pluggy maintainers; I prompted it, it did the work, I read it.
Fixes #733. Supersedes #740 with a smaller change.
register()now works in two passes. The first collects every hookimpl and verifies those whose hook has a spec, without touching manager state. The second stores the plugin, creates missing hook callers, replays history and adds the impls. APluginValidationErrornow leaves the manager unchanged, and the fixed plugin can be registered again.Unlike #740 there is no rollback. If a historic impl raises during replay, the plugin stays registered, as it does on main today.
The second commit covers an ordering case the split would otherwise have broken. When replaying one impl calls
add_hookspecsfor a hook that a later impl of the same plugin implements, that later impl is verified in the second pass. On main this was only caught whendir()order put the replayed impl first. A failure there still leaves the plugin partially registered, as on main, but only user code running during replay can trigger it.