mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
test(gateway): regression: every adapter.connect() must accept is_reconnect
The gateway reconnect watcher forwards is_reconnect=True to every adapter.connect() call on every retry. Adapters whose signature omits the kwarg raise TypeError at every reconnect attempt and stay silently disconnected — the exact bug that shipped for QQAdapter and only surfaced after messages stopped flowing on the QQ channel for hours. This test statically parses every adapter.py under gateway/platforms/ and plugins/platforms/ (via AST, so third-party SDKs like slack_sdk, matrix-nio, aiohttp, telegram, etc. are NOT required in the test env) and asserts every *Adapter class with an async connect() accepts is_reconnect — either as a keyword-only argument or absorbed by **kwargs. Also fixes plugins/platforms/wecom/callback_adapter.py:WecomCallbackAdapter, which the new test caught as a second offender. Same class of bug: bare 'async def connect(self)' signature would die on the first reconnect. Companion to #59429 (which fixed the original QQAdapter offender).
This commit is contained in:
parent
276542c729
commit
0f8603c571
2 changed files with 164 additions and 1 deletions
|
|
@ -115,7 +115,13 @@ class WecomCallbackAdapter(BasePlatformAdapter):
|
|||
# Lifecycle
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
async def connect(self) -> bool:
|
||||
async def connect(self, *, is_reconnect: bool = False) -> bool:
|
||||
# ``is_reconnect`` is forwarded by GatewayRunner on every retry per
|
||||
# the BasePlatformAdapter.connect contract. Callback adapters have
|
||||
# no server-side queue to preserve, so the flag is accepted-and-
|
||||
# ignored — but the kwarg MUST be present or the reconnect watcher
|
||||
# dies with TypeError and the platform silently stays offline.
|
||||
del is_reconnect
|
||||
if not self._apps:
|
||||
logger.warning("[WecomCallback] No callback apps configured")
|
||||
return False
|
||||
|
|
|
|||
157
tests/gateway/test_adapter_connect_is_reconnect_contract.py
Normal file
157
tests/gateway/test_adapter_connect_is_reconnect_contract.py
Normal file
|
|
@ -0,0 +1,157 @@
|
|||
"""Regression: every platform adapter's ``connect()`` must accept the
|
||||
``is_reconnect`` keyword-only argument.
|
||||
|
||||
The gateway reconnect watcher forwards ``is_reconnect=True`` to every
|
||||
adapter on every retry (see ``GatewayRunner._call_adapter_connect`` in
|
||||
``gateway/run.py``). An adapter whose ``connect()`` signature omits
|
||||
``is_reconnect`` blows up on the first reconnect attempt with::
|
||||
|
||||
TypeError: <Foo>Adapter.connect() got an unexpected
|
||||
keyword argument 'is_reconnect'
|
||||
|
||||
…and never recovers, leaving that platform silently disconnected until
|
||||
the operator manually restarts the gateway. This exact bug shipped for
|
||||
``QQAdapter`` and was only discovered after messages stopped flowing on
|
||||
the QQ channel for hours.
|
||||
|
||||
To prevent this class of bug from regressing, we statically parse every
|
||||
``adapter.py`` under ``gateway/platforms/`` and ``plugins/platforms/``
|
||||
and assert that its ``connect()`` method accepts an ``is_reconnect``
|
||||
keyword. Doing this via AST (rather than importing) avoids pulling every
|
||||
platform's optional third-party SDK (aiohttp, slack_sdk, telegram,
|
||||
matrix-nio, etc.) into the test environment.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
# Directories that hold platform adapters. Each entry is a directory
|
||||
# whose immediate children are either ``adapter.py`` files or
|
||||
# sub-packages that expose one.
|
||||
ADAPTER_ROOTS = [
|
||||
REPO_ROOT / "gateway" / "platforms",
|
||||
REPO_ROOT / "plugins" / "platforms",
|
||||
]
|
||||
|
||||
|
||||
def _iter_adapter_files() -> list[Path]:
|
||||
"""Every ``*adapter*.py`` under the two adapter roots.
|
||||
|
||||
We intentionally cast a wide net (any ``adapter.py`` / ``*_adapter.py``
|
||||
inside these trees) so a new platform can't sneak in without the
|
||||
contract check firing.
|
||||
"""
|
||||
files: list[Path] = []
|
||||
for root in ADAPTER_ROOTS:
|
||||
if not root.is_dir():
|
||||
continue
|
||||
for path in root.rglob("*.py"):
|
||||
if path.name == "adapter.py" or path.stem.endswith("_adapter"):
|
||||
files.append(path)
|
||||
return sorted(files)
|
||||
|
||||
|
||||
def _find_adapter_classes(module: ast.Module) -> list[ast.ClassDef]:
|
||||
"""Classes that look like a platform adapter.
|
||||
|
||||
Heuristic: any class whose name ends in ``Adapter`` and that defines
|
||||
an ``async def connect`` method. This catches every subclass of
|
||||
``BasePlatformAdapter`` in the tree today (QQAdapter, TelegramAdapter,
|
||||
SlackAdapter, …) without importing the base class.
|
||||
"""
|
||||
hits: list[ast.ClassDef] = []
|
||||
for node in ast.walk(module):
|
||||
if not isinstance(node, ast.ClassDef):
|
||||
continue
|
||||
if not node.name.endswith("Adapter"):
|
||||
continue
|
||||
has_connect = any(
|
||||
isinstance(item, ast.AsyncFunctionDef) and item.name == "connect"
|
||||
for item in node.body
|
||||
)
|
||||
if has_connect:
|
||||
hits.append(node)
|
||||
return hits
|
||||
|
||||
|
||||
def _connect_accepts_is_reconnect(cls: ast.ClassDef) -> bool:
|
||||
"""True iff the class's own ``connect()`` accepts ``is_reconnect``.
|
||||
|
||||
Accepts the kwarg via:
|
||||
- keyword-only argument named ``is_reconnect`` (the canonical form
|
||||
used by ``BasePlatformAdapter``), OR
|
||||
- ``**kwargs`` catch-all (also safe — the kwarg is absorbed).
|
||||
"""
|
||||
for item in cls.body:
|
||||
if not (isinstance(item, ast.AsyncFunctionDef) and item.name == "connect"):
|
||||
continue
|
||||
args = item.args
|
||||
if any(a.arg == "is_reconnect" for a in args.kwonlyargs):
|
||||
return True
|
||||
if any(a.arg == "is_reconnect" for a in args.args):
|
||||
return True
|
||||
if args.kwarg is not None: # **kwargs
|
||||
return True
|
||||
return False
|
||||
return False
|
||||
|
||||
|
||||
ADAPTER_FILES = _iter_adapter_files()
|
||||
|
||||
|
||||
def test_adapter_discovery_finds_platforms():
|
||||
"""Sanity: the discovery walker actually found a meaningful set of
|
||||
adapters. If this drops to a trivial number, the glob broke and the
|
||||
contract test below is silently passing on nothing.
|
||||
"""
|
||||
assert len(ADAPTER_FILES) >= 20, (
|
||||
f"Expected to discover >=20 platform adapter files under "
|
||||
f"{[str(p) for p in ADAPTER_ROOTS]}, found {len(ADAPTER_FILES)}. "
|
||||
f"The discovery glob is likely broken."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"adapter_file",
|
||||
ADAPTER_FILES,
|
||||
ids=lambda p: str(p.relative_to(REPO_ROOT)),
|
||||
)
|
||||
def test_adapter_connect_accepts_is_reconnect(adapter_file: Path):
|
||||
"""Every ``*Adapter.connect()`` must accept ``is_reconnect``.
|
||||
|
||||
This is the contract enforced by ``BasePlatformAdapter.connect`` and
|
||||
relied on by ``GatewayRunner._call_adapter_connect``. Violating it
|
||||
silently disables the affected platform after its first reconnect.
|
||||
"""
|
||||
source = adapter_file.read_text(encoding="utf-8")
|
||||
try:
|
||||
tree = ast.parse(source, filename=str(adapter_file))
|
||||
except SyntaxError as exc:
|
||||
pytest.fail(f"Could not parse {adapter_file}: {exc}")
|
||||
|
||||
classes = _find_adapter_classes(tree)
|
||||
if not classes:
|
||||
pytest.skip(
|
||||
f"{adapter_file.relative_to(REPO_ROOT)} has no *Adapter class "
|
||||
f"with an async connect() — nothing to check."
|
||||
)
|
||||
|
||||
offenders = [cls.name for cls in classes if not _connect_accepts_is_reconnect(cls)]
|
||||
|
||||
assert not offenders, (
|
||||
f"{adapter_file.relative_to(REPO_ROOT)}: the following adapter "
|
||||
f"class(es) define `async def connect()` WITHOUT accepting the "
|
||||
f"`is_reconnect` kwarg: {offenders}. "
|
||||
f"Add `*, is_reconnect: bool = False` to the signature "
|
||||
f"(matching BasePlatformAdapter.connect). "
|
||||
f"The gateway reconnect watcher forwards this kwarg on every "
|
||||
f"retry — an adapter that rejects it silently disconnects after "
|
||||
f"the first outage."
|
||||
)
|
||||
Loading…
Add table
Add a link
Reference in a new issue