fix(plugins): register dynamically-loaded modules in sys.modules before exec#17099
fix(plugins): register dynamically-loaded modules in sys.modules before exec#17099jquesnelle wants to merge 1 commit into
Conversation
…re exec
Dashboard plugin API routes (web_server._mount_plugin_api_routes) and
gateway event hooks (gateway.hooks.HookRegistry.discover_and_load) both
loaded Python files via importlib.util.spec_from_file_location +
exec_module without registering the resulting module in sys.modules.
That breaks any plugin or hook handler that uses `from __future__ import
annotations` together with a Pydantic BaseModel / dataclass / anything
that introspects `__module__`: at first request Pydantic tries to
resolve string-form type hints against the defining module's namespace,
can't find it by name, and raises:
PydanticUserError: TypeAdapter[...] is not fully defined;
you should define ... and all referenced types,
then call `.rebuild()` on the instance.
This is what broke the kanban dashboard's 'triage' button — POST
/api/plugins/kanban/tasks validated against CreateTaskBody (a Pydantic
model in a file using `from __future__ import annotations`) and
returned 500 on every click.
The fix, applied symmetrically to both loaders:
1. Compute module_name once.
2. Register the module in sys.modules BEFORE exec_module.
3. On exec_module failure, pop the half-initialized stub so subsequent
reloads don't pick up broken state.
GETs were unaffected because they don't build a body TypeAdapter, which
is why this only surfaced when users started POSTing.
There was a problem hiding this comment.
Solid fix — the root cause analysis in the description is excellent, and the implementation cleanly mirrors what CPython’s own importlib._bootstrap._load() does internally. Two files, symmetric changes, correct error cleanup. 👍
The code is correct. I verified the bug reproduces on Python 3.11 — a handler with from __future__ import annotations + @dataclass crashes immediately at exec_module time:
AttributeError: 'NoneType' object has no attribute '__dict__'
in dataclasses._is_type() → sys.modules.get(cls.__module__).__dict__
And confirmed the fix resolves it. I also checked the other 6 spec_from_file_location call sites in the codebase (plugins.py, claw.py, setup.py, context_engine/__init__.py, memory/__init__.py, rl_training_tool.py) — they all already have the sys.modules registration, so this PR closes the last two gaps. Nice.
One ask: a regression test
The existing tests/gateway/test_hooks.py suite doesn’t exercise from __future__ import annotations in any handler, so if someone refactors the loader later and drops the sys.modules line, no test catches it. Here’s a minimal one that slots right into TestDiscoverAndLoad:
def test_loads_handler_with_future_annotations(self, tmp_path):
"""Regression: PEP 563 annotations + @dataclass must not crash the loader."""
_create_hook(tmp_path, "annotated-hook", '["agent:start"]',
"from __future__ import annotations\n"
"from dataclasses import dataclass\n\n"
"@dataclass\n"
"class Payload:\n"
" name: str\n\n"
"def handle(event_type, context):\n"
" return Payload(name=event_type)\n"
)
reg = HookRegistry()
with patch("gateway.hooks.HOOKS_DIR", tmp_path), _patch_no_builtins(reg):
reg.discover_and_load()
assert len(reg.loaded_hooks) == 1Also left two small inline suggestions on the comment wording — the @dataclass failure is actually a load-time crash (not a first-dispatch issue, as the comments currently imply). Feel free to take or leave those.
| # Dynamically load the handler module. | ||
| # Register in sys.modules BEFORE exec_module so Pydantic / | ||
| # dataclasses / typing introspection can resolve forward | ||
| # references (triggered by `from __future__ import annotations` | ||
| # in the handler). Without this, a handler that declares a | ||
| # Pydantic BaseModel for webhook/event payloads fails at first | ||
| # dispatch with "TypeAdapter ... is not fully defined". |
There was a problem hiding this comment.
Nit: with @dataclass, the crash is actually at exec_module time (load-time), not at first dispatch. The current wording ("fails at first dispatch") might confuse future readers. Suggested tightening:
| # Dynamically load the handler module. | |
| # Register in sys.modules BEFORE exec_module so Pydantic / | |
| # dataclasses / typing introspection can resolve forward | |
| # references (triggered by `from __future__ import annotations` | |
| # in the handler). Without this, a handler that declares a | |
| # Pydantic BaseModel for webhook/event payloads fails at first | |
| # dispatch with "TypeAdapter ... is not fully defined". | |
| # Dynamically load the handler module. | |
| # Register in sys.modules BEFORE exec_module so that forward | |
| # references from `from __future__ import annotations` resolve | |
| # correctly. Without this, @dataclass crashes during module | |
| # loading and Pydantic BaseModels fail at first validation | |
| # ("TypeAdapter ... is not fully defined"). |
| # Register in sys.modules BEFORE exec_module so pydantic/FastAPI | ||
| # can resolve forward references (e.g. models defined in a file | ||
| # that uses `from __future__ import annotations`). Without this, | ||
| # TypeAdapter lazy-build fails at first request with | ||
| # "is not fully defined" because the module namespace isn't | ||
| # reachable by name for string-annotation resolution. |
There was a problem hiding this comment.
Same here — the comment says "fails at first request" but with @dataclass the crash happens during loading, not at request time:
| # Register in sys.modules BEFORE exec_module so pydantic/FastAPI | |
| # can resolve forward references (e.g. models defined in a file | |
| # that uses `from __future__ import annotations`). Without this, | |
| # TypeAdapter lazy-build fails at first request with | |
| # "is not fully defined" because the module namespace isn't | |
| # reachable by name for string-annotation resolution. | |
| # Register in sys.modules BEFORE exec_module so that forward | |
| # references from `from __future__ import annotations` resolve | |
| # correctly. Without this, @dataclass crashes during module | |
| # loading and Pydantic/FastAPI models fail at first request | |
| # ("TypeAdapter ... is not fully defined"). |
Dashboard plugin API routes (web_server._mount_plugin_api_routes) and gateway event hooks (gateway.hooks.HookRegistry.discover_and_load) both loaded Python files via importlib.util.spec_from_file_location + exec_module without registering the resulting module in sys.modules.
That breaks any plugin or hook handler that uses
from __future__ import annotationstogether with a Pydantic BaseModel / dataclass / anything that introspects__module__: at first request Pydantic tries to resolve string-form type hints against the defining module's namespace, can't find it by name, and raises:PydanticUserError: TypeAdapter[...] is not fully defined;
you should define ... and all referenced types,
then call
.rebuild()on the instance.This is what broke the kanban dashboard's 'triage' button — POST /api/plugins/kanban/tasks validated against CreateTaskBody (a Pydantic model in a file using
from __future__ import annotations) and returned 500 on every click.The fix, applied symmetrically to both loaders:
GETs were unaffected because they don't build a body TypeAdapter, which is why this only surfaced when users started POSTing.