From 5b012a51b7ac36e3143fe196c48664a0ab51a865 Mon Sep 17 00:00:00 2001 From: Thorsten Date: Sat, 18 Jul 2026 11:02:11 +0200 Subject: [PATCH] Fix leading-'#' zero-match in routing.matches(); extend the smoke test routing.matches() (used by the NATS classifier dispatch and the AMQP test harness alike) only fixed up the trailing zero-match case ("nick.dice.#" matching bare "nick.dice"). Real AMQP topic exchanges let '#' match zero words at any position, so a binding key like "#.doctor.#" must also match "doctor" as the literal first word with nothing before it - that shape was silently broken under the NATS backend (confirmed live: Doctor and DidYouKnow, the only two plugins using a bare leading '#', never fired for that phrasing on munin). Fixed with a symmetric leading-anchor rewrite, plus unit test coverage. A narrower gap remains for a '#' with zero words strictly between two literals (documented in routing.py) - only affects Selfreaction's degenerate "me." phrasing, not worth a fully general regex rewrite for. Also extends muc_smoke_check.py with checks for the trickiest binding-key shapes in the codebase: Doctor (leading '#', the bug above), Selfreaction (mid-pattern '#', the shape with no NATS-subject equivalent at all), and TeaTimer (a two-phase check - immediate confirmation, then the scheduled event firing on its own via the JetStream-durable action_processing path, with no new message sent to prompt it). Confirmed live: after this fix, Doctor passes on munin (NATS) too, matching aero2k.de (AMQP) exactly. Co-Authored-By: Claude Sonnet 5 --- src/distbot/common/routing.py | 17 ++++- tests/muc_smoke_check.py | 113 +++++++++++++++++++++++--------- tests/test_unit/test_routing.py | 24 +++++++ 3 files changed, 119 insertions(+), 35 deletions(-) create mode 100644 tests/test_unit/test_routing.py diff --git a/src/distbot/common/routing.py b/src/distbot/common/routing.py index fbe6596..21592fb 100644 --- a/src/distbot/common/routing.py +++ b/src/distbot/common/routing.py @@ -6,10 +6,21 @@ def matches(binding_key: str, subject: str) -> bool: """Match a subject against an AMQP-topic-exchange-style binding key. '.' separates words, '*' matches exactly one word, '#' matches zero or - more words (including at the end, where a naive translation would - require at least one word). + more words - including at either end, where a naive translation would + require at least one word (e.g. "#.doctor.#" must match a bare + "doctor" with nothing before or after it, same as real AMQP). + + Known gap: a '#' strictly between two other segments (e.g. + "me.#..#") still requires at least one word in that middle slot; + "me." with literally nothing between them won't match, unlike + real AMQP. Only fun.py's Selfreaction uses this shape, and only for the + degenerate zero-words-between case - not worth a fully general + zero-or-more-words-anywhere regex builder for one narrow phrasing. """ regex = binding_key.replace('.', r'\.').replace('*', '[^.]+').replace('#', '.*') - # fix .# leading to \..* + # #.foo -> .*\.foo requires a word before foo; drop that forced dot so + # a leading '#' can match zero words too. + regex = re.sub(r"^\.\*\\\.", ".*", regex) + # foo.# -> foo\..* requires a word after foo; same fix at the end. regex = re.sub(r"\\\.\.\*$", ".*", regex) return re.fullmatch(regex, subject) is not None diff --git a/tests/muc_smoke_check.py b/tests/muc_smoke_check.py index 31de8f1..dab304f 100644 --- a/tests/muc_smoke_check.py +++ b/tests/muc_smoke_check.py @@ -2,11 +2,19 @@ """ Black-box system test for a live urlbot-v3 instance. -Connects to a real MUC as a separate test account, addresses the bot by -its nickname with a handful of deterministic commands, and checks the -responses against expected patterns. Exits non-zero on the first -timeout/mismatch. This is a manual tool, not part of the automated pytest -suite - it needs a real XMPP server, room, and a bot actually running. +Connects to a real MUC as a separate test account, addresses the bot with +a handful of commands (some routed the normal "nick: command" way, some +via free-form trigger phrases), and checks the responses against expected +patterns. Exits non-zero on the first timeout/mismatch. This is a manual +tool, not part of the automated pytest suite - it needs a real XMPP +server, room, and a bot actually running. + +The default checks deliberately include a few "complex edge" plugins whose +binding-key wildcard shapes are the trickiest to get right under the NATS +backend (see distbot.common.routing): a leading '#' (Doctor), a +mid-pattern '#' (Selfreaction), and a scheduled/durable action that fires +without a new incoming message at all (TeaTimer) - exactly the mechanisms +this migration had to get right. Usage: python3 tests/muc_smoke_check.py \ @@ -18,24 +26,50 @@ import asyncio import re import sys from dataclasses import dataclass -from typing import Optional, Pattern +from typing import List, Optional, Pattern import slixmpp @dataclass class Check: - command: str - expect: Pattern description: str + expect: Pattern + command: Optional[str] = None # sent as "{bot_nick}: {command}" + raw_message: Optional[str] = None # sent verbatim (for non-addressed triggers) timeout: float = 10.0 - - -DEFAULT_CHECKS = [ - Check("ping", re.compile(r"^pong$"), "basic liveness"), - Check("version", re.compile(r"^I'm running"), "version string"), - Check("dice", re.compile(r"^rolling a dice for \S+:"), "dice roll (random content, fixed prefix)"), -] + # if neither command nor raw_message is set, nothing is sent - used for + # a scheduled follow-up (e.g. TeaTimer firing) that arrives unprompted. + + +def default_checks(nick: str, bot_nick: str) -> List[Check]: + nick_re = re.escape(nick) + return [ + Check("basic liveness", re.compile(r"^pong$"), command="ping"), + Check("version string", re.compile(r"^I'm running"), command="version"), + Check("dice roll (random content, fixed prefix)", + re.compile(r"^rolling a dice for \S+:"), command="dice"), + Check( + "leading '#' wildcard, zero-words-before case (Doctor: '#.doctor.#' must match " + "'doctor' as the literal first word, same as real AMQP - this exact shape was broken " + "under NATS until routing.matches() got a leading-anchor fix)", + re.compile(r"^EXTERMINATE! EXTERMINATE!$"), + raw_message="doctor please help me", + ), + Check( + "mid-pattern '#' wildcard (Selfreaction: 'me.#..#', no NATS-subject equivalent)", + re.compile(rf"^{nick_re}: .+"), + raw_message=f"me thinks {bot_nick} is quite alright", + ), + Check("scheduled action, phase 1/2: immediate confirmation", + re.compile(rf"^{nick_re}: Tea timer set to"), command="teatimer 3"), + Check( + "scheduled action, phase 2/2: durable delayed fire (JetStream/action_processing path, " + "no new message sent - this one just waits)", + re.compile(rf"^{nick_re}: Your tea is ready!$"), + timeout=8.0, + ), + ] class SmokeTestClient(slixmpp.ClientXMPP): @@ -47,7 +81,7 @@ class SmokeTestClient(slixmpp.ClientXMPP): self.checks = checks self.verbose = verbose self.results = [] - self._pending: Optional[asyncio.Future] = None + self._queue: "asyncio.Queue[str]" = asyncio.Queue() self.register_plugin('xep_0045') self.add_event_handler("session_start", self.session_start) @@ -91,17 +125,34 @@ class SmokeTestClient(slixmpp.ClientXMPP): self.loop.stop() async def run_check(self, check: Check): + # drop any stale messages left over from a previous check (e.g. a + # slow catch-all plugin reply that arrived late) so it can't be + # mistaken for this check's response. + while not self._queue.empty(): + self._queue.get_nowait() + + if check.command is not None: + self.send_message(mto=self.room, mbody=f"{self.bot_nick}: {check.command}", mtype='groupchat') + elif check.raw_message is not None: + self.send_message(mto=self.room, mbody=check.raw_message, mtype='groupchat') + # else: nothing to send - just wait for a spontaneous match (e.g. a scheduled event firing) + loop = asyncio.get_event_loop() - self._pending = loop.create_future() - self.send_message(mto=self.room, mbody=f"{self.bot_nick}: {check.command}", mtype='groupchat') - try: - body = await asyncio.wait_for(self._pending, timeout=check.timeout) - ok = bool(check.expect.search(body)) - self.results.append((check, ok, body)) - except asyncio.TimeoutError: - self.results.append((check, False, "")) - finally: - self._pending = None + deadline = loop.time() + check.timeout + while True: + remaining = deadline - loop.time() + if remaining <= 0: + self.results.append((check, False, "")) + return + try: + body = await asyncio.wait_for(self._queue.get(), timeout=remaining) + except asyncio.TimeoutError: + self.results.append((check, False, "")) + return + if check.expect.search(body): + self.results.append((check, True, body)) + return + self._debug(f"ignoring non-matching reply while waiting: {body!r}") def on_groupchat_message(self, msg): self._debug(f"groupchat msg from {msg['mucnick']!r}: {msg['body']!r}") @@ -109,8 +160,7 @@ class SmokeTestClient(slixmpp.ClientXMPP): return # ignore our own messages if msg['mucnick'] != self.bot_nick: return # ignore other room members - if self._pending is not None and not self._pending.done(): - self._pending.set_result(str(msg['body'])) + self._queue.put_nowait(str(msg['body'])) def main(): @@ -123,16 +173,15 @@ def main(): parser.add_argument("--verbose", action="store_true", help="print every groupchat message and join details") args = parser.parse_args() - client = SmokeTestClient( - args.jid, args.password, args.room, args.nick, args.bot_nick, DEFAULT_CHECKS, verbose=args.verbose, - ) + checks = default_checks(args.nick, args.bot_nick) + client = SmokeTestClient(args.jid, args.password, args.room, args.nick, args.bot_nick, checks, args.verbose) client.connect() client.loop.run_forever() failures = 0 for check, ok, body in client.results: status = "OK" if ok else "FAIL" - print(f"[{status}] {check.command!r} ({check.description}): {body!r}") + print(f"[{status}] {check.description}: {body!r}") if not ok: failures += 1 diff --git a/tests/test_unit/test_routing.py b/tests/test_unit/test_routing.py new file mode 100644 index 0000000..18f4b57 --- /dev/null +++ b/tests/test_unit/test_routing.py @@ -0,0 +1,24 @@ +# -*- coding: utf-8 -*- +import pytest + +from distbot.common.routing import matches + + +@pytest.mark.parametrize("binding_key,subject,expected", [ + # trailing '#' matches zero or more words + ("nick.dice.#", "nick.dice", True), + ("nick.dice.#", "nick.dice.5", True), + # leading '#' matches zero or more words too, same as real AMQP + ("#.doctor.#", "doctor.help.me", True), + ("#.doctor.#", "we.need.doctor.help", True), + ("#.doctor.#", "doctor", True), + ("#.doctor.#", "we.need.a.dentist", False), + # '*' matches exactly one word, no more no less + ("nick.new-voting.*.#", "nick.new-voting.foo", True), + ("nick.new-voting.*.#", "nick.new-voting.foo.bar", True), + ("nick.new-voting.*.#", "nick.new-voting", False), + # bare catch-all + ("#", "anything.at.all", True), +]) +def test_matches(binding_key, subject, expected): + assert matches(binding_key, subject) is expected -- 2.47.3