diff --git a/scripts/release.py b/scripts/release.py index 59cc1e1610..d9ea666e5f 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -58,6 +58,7 @@ AUTHOR_MAP = { "nbot@liizfq.top": "liizfq", "274096618+hermes-agent-dhabibi@users.noreply.github.com": "dhabibi", "dejie.guo@gmail.com": "JayGwod", + "aamirjawaid@microsoft.com": "heyitsaamir", "johnnncenaaa77@gmail.com": "johnncenae", "thomasjhon6666@gmail.com": "ThomassJonax", "focusflow.app.help@gmail.com": "yes999zc", diff --git a/tests/gateway/_plugin_adapter_loader.py b/tests/gateway/_plugin_adapter_loader.py new file mode 100644 index 0000000000..4174a7161c --- /dev/null +++ b/tests/gateway/_plugin_adapter_loader.py @@ -0,0 +1,72 @@ +"""Shared helper for loading platform-plugin ``adapter.py`` modules in tests. + +Every platform plugin under ``plugins/platforms//`` ships its own +``adapter.py``. If two tests independently do:: + + sys.path.insert(0, "plugins/platforms/irc") + from adapter import IRCAdapter + + sys.path.insert(0, "plugins/platforms/teams") + from adapter import TeamsAdapter + +…then whichever collects first in an xdist worker wins +``sys.modules["adapter"]``, and the other raises ``ImportError`` at +collection time. The fallout cascades across unrelated tests sharing that +worker because ``sys.path`` is still polluted. + +Use :func:`load_plugin_adapter` instead of ad-hoc ``sys.path`` tricks. +It loads the adapter from an explicit file path under a unique module +name (``plugin_adapter_``), so it cannot collide with any +other plugin's adapter module. + +The ``tests/gateway/conftest.py`` guard rejects the anti-pattern at +collection time so this can't regress when new plugin adapter tests are +added. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from types import ModuleType + + +_REPO_ROOT = Path(__file__).resolve().parents[2] +_PLUGINS_DIR = _REPO_ROOT / "plugins" / "platforms" + + +def load_plugin_adapter(plugin_name: str) -> ModuleType: + """Import ``plugins/platforms//adapter.py`` in isolation. + + The module is registered under the unique name + ``plugin_adapter_`` in ``sys.modules``. No ``sys.path`` + mutation. Safe to call multiple times — repeat calls return the + already-loaded module. + """ + module_name = f"plugin_adapter_{plugin_name}" + cached = sys.modules.get(module_name) + if cached is not None: + return cached + + adapter_path = _PLUGINS_DIR / plugin_name / "adapter.py" + if not adapter_path.is_file(): + raise FileNotFoundError( + f"Plugin adapter not found: {adapter_path}. " + f"Known plugins: {sorted(p.name for p in _PLUGINS_DIR.iterdir() if p.is_dir())}" + ) + + spec = importlib.util.spec_from_file_location(module_name, adapter_path) + if spec is None or spec.loader is None: + raise ImportError(f"Could not build import spec for {adapter_path}") + + module = importlib.util.module_from_spec(spec) + # Register BEFORE exec so the module can find itself if needed (some + # modules do ``sys.modules[__name__]`` reflection during import). + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except Exception: + sys.modules.pop(module_name, None) + raise + return module diff --git a/tests/gateway/conftest.py b/tests/gateway/conftest.py index 3e734e0d40..da8a2d3364 100644 --- a/tests/gateway/conftest.py +++ b/tests/gateway/conftest.py @@ -12,11 +12,32 @@ ImportError fallback, causing 30+ downstream test failures wherever Individual test files may still call their own ``_ensure_telegram_mock`` — it short-circuits when the mock is already present. + +Plugin-adapter anti-pattern guard +--------------------------------- +Tests for platform plugins (``plugins/platforms//adapter.py``) +must load the adapter via +:func:`tests.gateway._plugin_adapter_loader.load_plugin_adapter`, not by +adding the plugin directory to ``sys.path`` and doing a bare +``from adapter import ...``. The guard at the bottom of this file +scans test module ASTs at collection time and fails collection with a +pointer to the helper if the anti-pattern is detected. + +Rationale: every plugin ships its own ``adapter.py``, and two tests each +inserting their plugin dir on ``sys.path[0]`` race for +``sys.modules["adapter"]`` in the same xdist worker. Whichever collects +first wins; the other fails with ``ImportError``, and the polluted +``sys.path`` cascades into unrelated tests. See PR #17764 for the +incident. """ +import ast import sys +from pathlib import Path from unittest.mock import MagicMock +import pytest + def _ensure_telegram_mock() -> None: """Install a comprehensive telegram mock in sys.modules. @@ -197,3 +218,128 @@ def _ensure_discord_mock() -> None: # Run at collection time — before any test file's module-level imports. _ensure_telegram_mock() _ensure_discord_mock() + + +# --------------------------------------------------------------------------- +# Plugin-adapter anti-pattern guard +# --------------------------------------------------------------------------- + +_GATEWAY_DIR = Path(__file__).resolve().parent +_GUARD_HINT = ( + "Plugin adapter tests must use " + "``from tests.gateway._plugin_adapter_loader import load_plugin_adapter`` " + "and call ``load_plugin_adapter('')`` instead of inserting " + "``plugins/platforms//`` on sys.path and doing a bare ``import " + "adapter`` / ``from adapter import ...``. See the 'Plugin-adapter " + "anti-pattern guard' docstring in tests/gateway/conftest.py." +) + + +def _scan_for_plugin_adapter_antipattern(source: str) -> list[str]: + """Return a list of offending-line descriptions, or [] if clean. + + Flags two things: + 1. ``sys.path.insert(..., )`` + 2. ``import adapter`` or ``from adapter import ...`` at module level. + """ + try: + tree = ast.parse(source) + except SyntaxError: + return [] # Let pytest surface the real syntax error. + + offenses: list[str] = [] + + for node in ast.walk(tree): + # sys.path.insert(0, ".../plugins/platforms/...") + if isinstance(node, ast.Call): + func = node.func + target_name: str | None = None + if isinstance(func, ast.Attribute): + # sys.path.insert / sys.path.append + if ( + isinstance(func.value, ast.Attribute) + and isinstance(func.value.value, ast.Name) + and func.value.value.id == "sys" + and func.value.attr == "path" + and func.attr in ("insert", "append", "extend") + ): + target_name = f"sys.path.{func.attr}" + + if target_name is not None: + call_src = ast.unparse(node) + # Match both the string-literal form + # ``.../plugins/platforms/...`` and the Path-operator form + # ``Path(...) / 'plugins' / 'platforms' / ...`` that + # plugin tests typically use. + _src_no_ws = "".join(call_src.split()) + if ( + "plugins/platforms" in call_src + or "plugins\\platforms" in call_src + or "'plugins'/'platforms'" in _src_no_ws + or '"plugins"/"platforms"' in _src_no_ws + ): + offenses.append( + f"line {node.lineno}: {target_name}(...) points into " + f"plugins/platforms/" + ) + + # Bare `import adapter` / `from adapter import ...` anywhere (module level + # OR inside functions — both are symptoms of the same pattern). + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "adapter": + offenses.append( + f"line {node.lineno}: ``import adapter`` " + f"(bare — resolves to whichever plugin's adapter.py " + f"is first on sys.path)" + ) + elif isinstance(node, ast.ImportFrom): + if node.module == "adapter" and node.level == 0: + offenses.append( + f"line {node.lineno}: ``from adapter import ...`` " + f"(bare — resolves to whichever plugin's adapter.py " + f"is first on sys.path)" + ) + + return offenses + + +def pytest_configure(config): + """Reject plugin-adapter tests that use the sys.path anti-pattern. + + Runs once per pytest session on the controller, BEFORE any xdist + worker is spawned. If any file under ``tests/gateway/`` matches the + anti-pattern, we fail the whole session with a clear message — + before a polluted ``sys.path`` can cascade across workers. + """ + # Only run on the xdist controller (or in non-xdist runs). Skip on + # worker subprocesses so we don't scan the filesystem N times. + if hasattr(config, "workerinput"): + return + + violations: list[str] = [] + for path in _GATEWAY_DIR.rglob("test_*.py"): + if path.name in {"_plugin_adapter_loader.py", "conftest.py"}: + continue + try: + source = path.read_text(encoding="utf-8") + except OSError: + continue + if "adapter" not in source and "plugins/platforms" not in source: + continue + offenses = _scan_for_plugin_adapter_antipattern(source) + if offenses: + violations.append( + f" {path.relative_to(_GATEWAY_DIR.parent.parent)}:\n " + + "\n ".join(offenses) + ) + + if violations: + raise pytest.UsageError( + "Plugin-adapter-import anti-pattern detected in gateway tests:\n" + + "\n".join(violations) + + "\n\n" + + _GUARD_HINT + ) + diff --git a/tests/gateway/test_irc_adapter.py b/tests/gateway/test_irc_adapter.py index 795c1b56b0..a1718fbdaf 100644 --- a/tests/gateway/test_irc_adapter.py +++ b/tests/gateway/test_irc_adapter.py @@ -7,16 +7,19 @@ import pytest from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch -# Ensure the plugins directory is on sys.path for direct import -_REPO_ROOT = Path(__file__).resolve().parents[2] -_IRC_PLUGIN_DIR = _REPO_ROOT / "plugins" / "platforms" / "irc" -if str(_IRC_PLUGIN_DIR) not in sys.path: - sys.path.insert(0, str(_IRC_PLUGIN_DIR)) +from tests.gateway._plugin_adapter_loader import load_plugin_adapter +# Load plugins/platforms/irc/adapter.py under a unique module name +# (plugin_adapter_irc) so it cannot collide with other plugin adapters +# loaded by sibling tests in the same xdist worker. +_irc_mod = load_plugin_adapter("irc") -# ── IRC protocol helpers ───────────────────────────────────────────────── - -from adapter import _parse_irc_message, _extract_nick +_parse_irc_message = _irc_mod._parse_irc_message +_extract_nick = _irc_mod._extract_nick +IRCAdapter = _irc_mod.IRCAdapter +check_requirements = _irc_mod.check_requirements +validate_config = _irc_mod.validate_config +register = _irc_mod.register class TestIRCProtocolHelpers: @@ -52,8 +55,6 @@ class TestIRCProtocolHelpers: # ── IRC Adapter ────────────────────────────────────────────────────────── -from adapter import IRCAdapter, check_requirements, validate_config - class TestIRCAdapterInit: @@ -494,8 +495,6 @@ class TestIRCPluginRegistration: # Clean up if already registered platform_registry.unregister("irc") - from adapter import register - ctx = MagicMock() register(ctx) ctx.register_platform.assert_called_once() diff --git a/tests/gateway/test_teams.py b/tests/gateway/test_teams.py index 3dec68f820..7a035142ed 100644 --- a/tests/gateway/test_teams.py +++ b/tests/gateway/test_teams.py @@ -10,12 +10,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from gateway.config import Platform, PlatformConfig, HomeChannel - -# Ensure the plugin directory is on sys.path for direct import (mirrors IRC pattern) -_REPO_ROOT = Path(__file__).resolve().parents[2] -_TEAMS_PLUGIN_DIR = _REPO_ROOT / "plugins" / "platforms" / "teams" -if str(_TEAMS_PLUGIN_DIR) not in sys.path: - sys.path.insert(0, str(_TEAMS_PLUGIN_DIR)) +from tests.gateway._plugin_adapter_loader import load_plugin_adapter # --------------------------------------------------------------------------- @@ -160,13 +155,18 @@ def _ensure_teams_mock(): _ensure_teams_mock() -# Now safe to import the adapter -import adapter as _teams_mod +# Load plugins/platforms/teams/adapter.py under a unique module name +# (plugin_adapter_teams) so it cannot collide with sibling plugin adapters. +_teams_mod = load_plugin_adapter("teams") _teams_mod.TEAMS_SDK_AVAILABLE = True _teams_mod.AIOHTTP_AVAILABLE = True -from adapter import TeamsAdapter, check_requirements, check_teams_requirements, validate_config +TeamsAdapter = _teams_mod.TeamsAdapter +check_requirements = _teams_mod.check_requirements +check_teams_requirements = _teams_mod.check_teams_requirements +validate_config = _teams_mod.validate_config +register = _teams_mod.register # --------------------------------------------------------------------------- @@ -276,20 +276,17 @@ class TestTeamsAdapterInit: class TestTeamsPluginRegistration: def test_register_calls_ctx(self): - from adapter import register ctx = MagicMock() register(ctx) ctx.register_platform.assert_called_once() def test_register_name(self): - from adapter import register ctx = MagicMock() register(ctx) kwargs = ctx.register_platform.call_args[1] assert kwargs["name"] == "teams" def test_register_auth_env_vars(self): - from adapter import register ctx = MagicMock() register(ctx) kwargs = ctx.register_platform.call_args[1] @@ -297,21 +294,18 @@ class TestTeamsPluginRegistration: assert kwargs["allow_all_env"] == "TEAMS_ALLOW_ALL_USERS" def test_register_max_message_length(self): - from adapter import register ctx = MagicMock() register(ctx) kwargs = ctx.register_platform.call_args[1] assert kwargs["max_message_length"] == 28000 def test_register_has_setup_fn(self): - from adapter import register ctx = MagicMock() register(ctx) kwargs = ctx.register_platform.call_args[1] assert callable(kwargs.get("setup_fn")) def test_register_has_platform_hint(self): - from adapter import register ctx = MagicMock() register(ctx) kwargs = ctx.register_platform.call_args[1]