Repository navigation
fix(plugin-sdk): file-path fallback located the plugin file and could not execute it (#10294) - #13944
Merged
Merged
Conversation
…d not exec it (#10294) The dotted entry point still fails exactly as reported — core plugins live in `plugins/core-plugins/<name>/` and declare `plugins.core_plugins.<name>.main`, with no `__init__.py`, namespace package or path hook mapping one to the other. The `spec_from_file_location` fallback suggested on the issue was already implemented, and it does locate the correct file. What failed was `exec_module`: the plugin's OWN imports also reference `plugins.core_plugins.*`, and those parent packages exist in no `sys.modules`. The log read File-path fallback failed for 'plugins.core_plugins.image_generation_plugin.main' at plugins/core-plugins/image-generation-plugin/main.py: No module named 'plugins.core_plugins' — the loader reporting it could not load a file it had already found, which is why the fallback looked broken rather than incomplete. Each parent now gets a namespace module whose `__path__` points at the matching real directory, walked UP from the module file so the hyphen/underscore mapping is derived rather than guessed — a plugin dir that is not simply `name.replace('_','-')` works without a special case. An existing real `plugins` package is never replaced, and a failed exec withdraws what it created rather than leaving half a tree for the next import to resolve against. Also: the fallback hardcoded `main.py`, so `telemetry-prompt-middleware` — which declares `...telemetry_prompt_middleware.plugin` and ships `plugin.py` — could never have been found by it. The filename now comes from the entry point's last segment. Verified against the shipped tree, not only fixtures: both plugins named in the issue now import through the fallback. HONEST SCOPE — this does not make any plugin load yet, and the count is still 0 of 7. Driving the real loader over the real tree shows three distinct causes, and this fixes one: route 2 (this) the fallback could not exec what it found — 2 plugins route 1 plugins subclass a SECOND copy of BasePlugin — 6 plugins route 3 a missing optional dependency — 1 plugin Route 1 is the gate: `ImageGenerationPlugin.__mro__` carries `plugin_sdk.base.BasePlugin` while the loader tests against `autobot_shared.plugin_sdk.base.BasePlugin`, so `issubclass` is False and the loader reports "No plugin class found in module" for a class that is right there. The two plugins fixed here moved from route 2's error to route 1's, which is the observable proof this change works. Routes 1 and 3, and the missing `loaded N of M` signal that makes any of it visible, are #13677.
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
mrveiss
added a commit
that referenced
this pull request
Aug 10, 2026
…led (#13677) Two independent defects, and the second is what made the first invisible. **The loader could not recognise the plugins.** `docs/developer/PLUGIN_SDK.md` tells authors to write `from plugin_sdk.base import BasePlugin`, and every core plugin does. That name resolves — autobot_shared is installed editable and exposes `plugin_sdk` as a second top-level package over the same source — but to a SECOND set of module objects. So `plugin_sdk.base.BasePlugin` and `autobot_shared.plugin_sdk.base.BasePlugin` are different classes, the loader's `issubclass` check against the canonical one returns False, and it reports "No plugin class found in module" for a class sitting in the module it just imported. The plugins were never wrong; the loader was. `canonicalise_plugin_sdk()` binds the documented names to the canonical modules before any plugin is imported. Submodules are bound too — aliasing the parent alone lets a later `import plugin_sdk.base` load a fresh object from the canonical path and reintroduce the split. Result on the real tree: **0 of 7 loaded becomes 3 of 7.** The two generation plugins need #10294 (PR #13944) as well; logger-plugin fails on a /tmp permission and telemetry-prompt-middleware on a missing aiohttp — both real, both now visible rather than silent. **Nothing totalled the result.** `startup()` logged discovery and per-plugin successes, and the far commoner failure — `load_plugin` returning None rather than raising — logged NOTHING at that level. So a run where every plugin failed produced the same shape of output as a healthy one, and a host with no plugins produced the same shape as a host where all seven were broken. Now: loaded 0 of N -> CRITICAL, "the plugin subsystem is delivering nothing" loaded X of N -> WARNING naming the casualties loaded N of N -> INFO no plugins -> INFO, explicitly NOT a failure and `get_load_report()` makes it a QUERY rather than a log-reading exercise, which is what #13852's acceptance actually asks for. Two test-quality notes, both the failure this umbrella is about showing up in its own fix: - The first version of the canonicalisation guard SURVIVED deleting the loader's call. Earlier tests in the file called the helper directly, leaving the alias bound process-wide, so every later load test passed regardless. The guard now asserts the loader's own effect on sys.modules, and an autouse fixture returns each test to the split state a real process starts in. - Plugin names are unique per test because `get_registry()` is a process global; a shared name failed the second test with "Plugin already registered" — a test-wiring failure wearing the costume of a product bug.
mrveiss
added a commit
that referenced
this pull request
Aug 11, 2026
…re installed (#13677) (#13952) * chore: claim worktree for issue-13677 * fix(plugin-sdk): 7 plugins failed and it looked like none were installed (#13677) Two independent defects, and the second is what made the first invisible. **The loader could not recognise the plugins.** `docs/developer/PLUGIN_SDK.md` tells authors to write `from plugin_sdk.base import BasePlugin`, and every core plugin does. That name resolves — autobot_shared is installed editable and exposes `plugin_sdk` as a second top-level package over the same source — but to a SECOND set of module objects. So `plugin_sdk.base.BasePlugin` and `autobot_shared.plugin_sdk.base.BasePlugin` are different classes, the loader's `issubclass` check against the canonical one returns False, and it reports "No plugin class found in module" for a class sitting in the module it just imported. The plugins were never wrong; the loader was. `canonicalise_plugin_sdk()` binds the documented names to the canonical modules before any plugin is imported. Submodules are bound too — aliasing the parent alone lets a later `import plugin_sdk.base` load a fresh object from the canonical path and reintroduce the split. Result on the real tree: **0 of 7 loaded becomes 3 of 7.** The two generation plugins need #10294 (PR #13944) as well; logger-plugin fails on a /tmp permission and telemetry-prompt-middleware on a missing aiohttp — both real, both now visible rather than silent. **Nothing totalled the result.** `startup()` logged discovery and per-plugin successes, and the far commoner failure — `load_plugin` returning None rather than raising — logged NOTHING at that level. So a run where every plugin failed produced the same shape of output as a healthy one, and a host with no plugins produced the same shape as a host where all seven were broken. Now: loaded 0 of N -> CRITICAL, "the plugin subsystem is delivering nothing" loaded X of N -> WARNING naming the casualties loaded N of N -> INFO no plugins -> INFO, explicitly NOT a failure and `get_load_report()` makes it a QUERY rather than a log-reading exercise, which is what #13852's acceptance actually asks for. Two test-quality notes, both the failure this umbrella is about showing up in its own fix: - The first version of the canonicalisation guard SURVIVED deleting the loader's call. Earlier tests in the file called the helper directly, leaving the alias bound process-wide, so every later load test passed regardless. The guard now asserts the loader's own effect on sys.modules, and an autouse fixture returns each test to the split state a real process starts in. - Plugin names are unique per test because `get_registry()` is a process global; a shared name failed the second test with "Plugin already registered" — a test-wiring failure wearing the costume of a product bug. * fix(plugin-sdk): reset the load tally on shutdown, and drop a duplicate annotation (#13677) mypy caught `_load_report` annotated twice — an earlier edit matched `self._started = False` in both `__init__` and `shutdown()`, so the whole block including its annotation and comment landed in each. Resetting the tally on shutdown is right and stays: after shutdown the previous run's counts describe a subsystem that is no longer running, and "loaded 5 of 7" from a dead manager is a worse answer than none — the same conflation this issue is about, one state along. The duplicated annotation and the copy-pasted comment are gone. Found by running the full gate set locally BEFORE pushing rather than after CI said so: black, isort, flake8, mypy, and the six repo convention checkers. * fix(plugin-sdk): the new signal named five healthy plugins as casualties (#13677) Review found two blockers and a major, and the major is the same failure this issue is about, inside its own fix. **The summary was wrong in the configuration the live backend uses.** `lifespan.py` passes the deployed plugin root AND the dev fallback, which overlap — this issue's own title says "discovery runs twice". Undeduped, the second registration of a HEALTHY plugin raises "Plugin already registered", is swallowed by the loader, returns None, and lands in `failed`. The live config reported `discovered=14` with FIVE perfectly-loaded plugins named as casualties. A signal built to end a misdiagnosis must not manufacture one. Manifests are now deduped by name; verified against the two-dir shape lifespan.py actually passes. **Canonicalisation re-created the singleton split #11636 fixed.** Binding the parent first meant any submodule imported during the alias loop that does `from plugin_sdk.X import ...` loaded a fresh X from the canonical path AND set it as an attribute on the parent — by then the canonical package. Repairing sys.modules does not repair that attribute, so `autobot_shared.plugin_sdk.registry` held a SECOND Registry class with its own `_instance`, and a late importer could read an empty registry. Submodules are bound before the parent now, and the parent attribute is re-asserted after each. **The alias loop imported the package's own tests, and pytest, into production.** ~100ms per startup where pytest exists; where it does not the imports fail, are swallowed at DEBUG, and — since failed imports are not cached — retry for every plugin on every startup. That was also what triggered the clobber above. Also: `get_load_report()` had no non-test callers, which does not satisfy "a check can query it" — it is now logged at startup alongside the status map. The returned dict shared its `failed` list with the manager, so a caller appending to it mutated the tally. `started` is derived from `_started` rather than stored, which previously disagreed for the whole load window and permanently if discovery raised. All three fixes are mutation-verified. The `setattr` repair needed its trigger constructed explicitly — filtering out the package's tests removes today's only clobber source, so without that the guard passed whatever the loader did. Full gate set run locally before pushing; mypy caught a `list(object)` overload in the copy fix.
10 of 11 tasks
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.
Closes #10294 · umbrella #13852 (Wave 2) · unblocks #13677
Thinking Path
The dotted entry point fails exactly as the issue reports. What the issue could not know is that the
suggested fix was already implemented —
_import_from_fileusesspec_from_file_locationand locates thecorrect file. What fails is
exec_module:The loader reporting it could not load a file it had already found. The plugin's own imports also
reference
plugins.core_plugins.*, and those parent packages exist in nosys.modules. So the fix is not"load from the located file" — it is "make the dotted path resolvable for the module's own imports".
What Changed
Each parent gets a namespace module whose
__path__points at the matching real directory, walked up fromthe module file so the hyphen/underscore mapping is derived rather than guessed — a plugin dir that is not
simply
name.replace('_','-')needs no special case. An existing realpluginspackage is never replaced,and a failed exec withdraws what it created rather than leaving half a tree for the next import to resolve
against.
The fallback also hardcoded
main.py, sotelemetry-prompt-middleware— which declares...telemetry_prompt_middleware.pluginand shipsplugin.py— could never have been found by it. Thefilename now comes from the entry point's last segment.
Verification
Mutation-tested; each part of the fix is guarded:
main.pyagainOne test runs against the shipped tree, not fixtures: both plugins named in the issue now import through
the fallback.
Honest scope — this does not make any plugin load yet
Driving the real loader over the real
plugins/tree still gives 0 of 7 loaded. There are three distinctcauses and this fixes one:
BasePluginaiohttp)Route 1 is the gate:
The plugins import
from plugin_sdk.base import BasePlugin; the loader tests againstautobot_shared.plugin_sdk.base.BasePlugin. Different class objects, soissubclassis False and the loaderreports "No plugin class found in module" for a class sitting right there — an error that sends the reader
hunting for the wrong thing.
The two plugins fixed here moved from route 2's error to route 1's, which is the observable proof this
change works. Routes 1 and 3, plus the missing
loaded N of Msignal that makes any of this visible at all,are #13677 — next.
Model Used
Claude Opus 5 (1M context)