From b4e2370ee81d093ab5fd1df56c78a9a1ff612881 Mon Sep 17 00:00:00 2001 From: AIOSAI Date: Fri, 10 Jul 2026 21:19:04 -0700 Subject: [PATCH] =?UTF-8?q?#644:=20gate=20Telegram=20/create=20+=20/cancel?= =?UTF-8?q?=20to=20base=20@aipass=20bot=20only=20=E2=80=94=20base=5Fbot.py?= =?UTF-8?q?=20guards=20on=20branch=5Fname=20(branch=20bots=20return=20Fals?= =?UTF-8?q?e=20in=20=5Fdispatch=5Fcommand=20+=20omit=20from=20get=5Fcustom?= =?UTF-8?q?=5Fcommands;=20base=20bot=20None=20still=20routes=20both).=20Ri?= =?UTF-8?q?des=20along=20@skills=20#669.2/#669.3=20fail-loud=20(botfather?= =?UTF-8?q?=5Fclient=20=5Fload=5Ftelethon=5Fconfig=20raises=20RuntimeError?= =?UTF-8?q?=20naming=20config=20path=20vs=20silent=20None)=20+=20#668=20of?= =?UTF-8?q?fset=20tests.=20133=20TG=20tests,=20seedgo=2031/31=20both=20fil?= =?UTF-8?q?es.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 13 ++ .../lib/telegram/apps/handlers/base_bot.py | 20 +- .../apps/handlers/botfather_client.py | 62 +++-- .../telegram/tests/test_botfather_client.py | 60 ++--- .../telegram/tests/test_multibot_config.py | 12 +- .../lib/telegram/tests/test_poll_and_gate.py | 216 ++++++++++++++++++ 6 files changed, 310 insertions(+), 73 deletions(-) create mode 100644 src/aipass/skills/lib/telegram/tests/test_poll_and_gate.py diff --git a/CHANGELOG.md b/CHANGELOG.md index fcf87688..3d628e9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,19 @@ PyPI version — not the changelog header. ### Fixed +- **Telegram `/create` + `/cancel` are now gated to the base @aipass bot (issue + #644).** Every per-branch bot inherited `BaseBot`'s `/create` + `/cancel` and + could mint new bots — but Patrick designated the base @aipass bot as the *sole* + spawner. `base_bot.py` now guards on bot identity (branch bots carry a + `branch_name`; the base bot's is `None`): `_dispatch_command` returns `False` for + `create`/`cancel` on a branch bot (falls through to normal handling), and + `get_custom_commands` advertises them only for the base bot. Rode along in the + same @skills pass: fail-loud fixes to `botfather_client.py` (issues #669.2/#669.3, + already closed) — `_load_telethon_config` now raises `RuntimeError` naming the + config path and the `drone @api set-secret telegram telethon_config` command + instead of silently returning `None` — plus poll-offset test coverage (#668). + 133 telegram tests pass, seedgo 31/31 on both source files. + - **seedgo no longer lints throwaway code (issue #675).** A single disposable POC used to fire 8 standard violations (architecture, meta, shebang…). The audit and checklist now skip any file resolved under a system temp dir diff --git a/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py b/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py index e1b3686c..451dc12d 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py @@ -576,12 +576,14 @@ class BaseBot: self._handle_monitor_command(chat_id, cmd_args) return True - # /create command — multi-step bot creation + # /create and /cancel — base bot only (branch bots must not spawn bots) + if cmd_name in ("create", "cancel") and self.branch_name is not None: + return False + if cmd_name == "create": self._handle_create_command(chat_id, cmd_args) return True - # /cancel command — cancel active /create flow if cmd_name == "cancel": if chat_id in self._create_state: del self._create_state[chat_id] @@ -2335,20 +2337,22 @@ class BaseBot: Returns: Dict of commands in telegram_standards format """ - return { + commands = { "monitor": { "description": "Subscribe to system-wide log alerts — /monitor on, off, all, status", "menu_text": "Log monitor", }, - "create": { + } + if self.branch_name is None: + commands["create"] = { "description": "Create a Telegram bot for a branch — e.g. /create chat devpulse", "menu_text": "New branch bot", - }, - "cancel": { + } + commands["cancel"] = { "description": "Cancel an in-progress /create", "menu_text": "Cancel create", - }, - } + } + return commands # ============================================= # LOCK FILE MANAGEMENT diff --git a/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py b/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py index 96242747..c434238f 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py @@ -71,7 +71,7 @@ MAX_USERNAME_ATTEMPTS = 3 # ============================================= -def _load_telethon_config() -> Optional[dict]: +def _load_telethon_config() -> dict: """ Load Telethon API credentials from the @api secrets store. @@ -79,31 +79,34 @@ def _load_telethon_config() -> Optional[dict]: {"api_id": 12345, "api_hash": "abc123..."} Returns: - Dict with "api_id" (int) and "api_hash" (str), or None on failure. + Dict with "api_id" (int) and "api_hash" (str). + + Raises: + RuntimeError: If config is missing, incomplete, or unreadable. """ + config = _get_secret("telethon_config") + if config is None: + raise RuntimeError( + "Telethon config not found in secrets store (telegram/telethon_config). " + "Set it with: drone @api set-secret telegram telethon_config " + '\'{"api_id": ..., "api_hash": "..."}\'' + ) + + api_id = config.get("api_id") + api_hash = config.get("api_hash") + + if not api_id or not api_hash: + raise RuntimeError("Telethon config incomplete — missing api_id or api_hash (telegram/telethon_config)") + try: - config = _get_secret("telethon_config") - if config is None: - logger.warning("Telethon config not found in secrets store") - return None - - api_id = config.get("api_id") - api_hash = config.get("api_hash") - - if not api_id or not api_hash: - logger.warning("Telethon config missing api_id or api_hash") - return None - - # Ensure api_id is an integer config["api_id"] = int(api_id) - config["api_hash"] = str(api_hash) + except (ValueError, TypeError) as e: + raise RuntimeError(f"Telethon config api_id is not a valid integer: {e}") from e - logger.info("Telethon config loaded successfully") - return config + config["api_hash"] = str(api_hash) - except (ValueError, OSError) as e: - logger.warning("Failed to load Telethon config: %s", e) - return None + logger.info("Telethon config loaded successfully") + return config # ============================================= @@ -127,13 +130,11 @@ def check_telethon_setup() -> tuple[bool, str]: if not TELETHON_AVAILABLE: return (False, "Telethon library not installed. Run: pip install telethon") - config = _get_secret("telethon_config") - if config is None: - return (False, "Telethon config not found in secrets store") - - config = _load_telethon_config() - if config is None: - return (False, "Telethon config is invalid (missing api_id or api_hash)") + try: + _load_telethon_config() + except RuntimeError as e: + logger.warning("Telethon setup check failed: %s", e) + return (False, str(e)) # Telethon creates session files with .session extension session_file = Path(str(SESSION_PATH) + ".session") @@ -478,11 +479,6 @@ def create_bot_via_botfather(branch_name: str) -> Optional[dict]: raise RuntimeError(f"Telethon setup failed: {reason}") config = _load_telethon_config() - if config is None: - raise RuntimeError( - "Telethon config could not be loaded (missing api_id or api_hash). " - 'Set it with: drone @api set-secret telethon_config \'{"api_id": ..., "api_hash": "..."}\'' - ) api_id = config["api_id"] api_hash = config["api_hash"] diff --git a/src/aipass/skills/lib/telegram/tests/test_botfather_client.py b/src/aipass/skills/lib/telegram/tests/test_botfather_client.py index 2fe31c05..f7d638c4 100644 --- a/src/aipass/skills/lib/telegram/tests/test_botfather_client.py +++ b/src/aipass/skills/lib/telegram/tests/test_botfather_client.py @@ -47,18 +47,22 @@ class TestLoadTelethonConfig: mock_get_secret.assert_called_once_with("telethon_config") @patch("apps.handlers.botfather_client._get_secret") - def test_returns_none_when_secret_missing(self, mock_get_secret): - """Returns None when the secret doesn't exist.""" + def test_raises_when_secret_missing(self, mock_get_secret): + """Raises RuntimeError when the secret doesn't exist.""" mock_get_secret.return_value = None - result = _load_telethon_config() - assert result is None + import pytest + + with pytest.raises(RuntimeError, match="not found in secrets store"): + _load_telethon_config() @patch("apps.handlers.botfather_client._get_secret") - def test_returns_none_when_api_id_missing(self, mock_get_secret): - """Returns None when api_id is missing from config.""" + def test_raises_when_api_id_missing(self, mock_get_secret): + """Raises RuntimeError when api_id is missing from config.""" mock_get_secret.return_value = {"api_hash": "abc123def"} - result = _load_telethon_config() - assert result is None + import pytest + + with pytest.raises(RuntimeError, match="missing api_id or api_hash"): + _load_telethon_config() @patch("apps.handlers.botfather_client._get_secret") def test_coerces_api_id_from_string(self, mock_get_secret): @@ -78,14 +82,10 @@ class TestLoadTelethonConfig: class TestCheckTelethonSetup: """Test check_telethon_setup: checks library, config, session file.""" - @patch("apps.handlers.botfather_client._get_secret") @patch("apps.handlers.botfather_client._load_telethon_config") - def test_returns_ready_when_all_in_place(self, mock_load_config, mock_get_secret, tmp_path, monkeypatch): + def test_returns_ready_when_all_in_place(self, mock_load_config, tmp_path, monkeypatch): """Returns (True, 'ready') when Telethon is available, config valid, session exists.""" monkeypatch.setattr("apps.handlers.botfather_client.TELETHON_AVAILABLE", True) - # _get_secret called directly in check_telethon_setup for existence check - mock_get_secret.return_value = {"api_id": 12345, "api_hash": "abc123"} - # _load_telethon_config called for validation check mock_load_config.return_value = {"api_id": 12345, "api_hash": "abc123"} # Create session file at the new path @@ -108,34 +108,30 @@ class TestCheckTelethonSetup: assert ready is False assert "not installed" in reason.lower() or "telethon" in reason.lower() - @patch("apps.handlers.botfather_client._get_secret") - def test_returns_false_when_config_not_in_secrets(self, mock_get_secret, monkeypatch): + @patch("apps.handlers.botfather_client._load_telethon_config") + def test_returns_false_when_config_not_in_secrets(self, mock_load_config, monkeypatch): """Returns (False, ...) when secret store has no telethon config.""" monkeypatch.setattr("apps.handlers.botfather_client.TELETHON_AVAILABLE", True) - mock_get_secret.return_value = None + mock_load_config.side_effect = RuntimeError( + "Telethon config not found in secrets store (telegram/telethon_config)" + ) ready, reason = check_telethon_setup() assert ready is False - assert "config" in reason.lower() or "not found" in reason.lower() + assert "not found" in reason.lower() @patch("apps.handlers.botfather_client._load_telethon_config") - @patch("apps.handlers.botfather_client._get_secret") - def test_returns_false_when_config_invalid(self, mock_get_secret, mock_load_config, monkeypatch): + def test_returns_false_when_config_invalid(self, mock_load_config, monkeypatch): """Returns (False, ...) when config exists but is invalid (missing fields).""" monkeypatch.setattr("apps.handlers.botfather_client.TELETHON_AVAILABLE", True) - # _get_secret returns something (config exists) but _load_telethon_config - # returns None (validation fails due to missing api_id) - mock_get_secret.return_value = {"api_hash": "abc123"} - mock_load_config.return_value = None + mock_load_config.side_effect = RuntimeError("Telethon config incomplete — missing api_id or api_hash") ready, reason = check_telethon_setup() assert ready is False - assert "invalid" in reason.lower() + assert "missing api_id or api_hash" in reason @patch("apps.handlers.botfather_client._load_telethon_config") - @patch("apps.handlers.botfather_client._get_secret") - def test_returns_false_when_session_file_missing(self, mock_get_secret, mock_load_config, tmp_path, monkeypatch): + def test_returns_false_when_session_file_missing(self, mock_load_config, tmp_path, monkeypatch): """Returns (False, ...) when session file doesn't exist.""" monkeypatch.setattr("apps.handlers.botfather_client.TELETHON_AVAILABLE", True) - mock_get_secret.return_value = {"api_id": 12345, "api_hash": "abc123"} mock_load_config.return_value = {"api_id": 12345, "api_hash": "abc123"} session_path = tmp_path / ".telethon" @@ -614,18 +610,22 @@ class TestCreateBotViaBotfather: create_bot_via_botfather("dev_central") def test_raises_when_config_load_fails(self, monkeypatch): - """Raises RuntimeError when _load_telethon_config returns None.""" + """Raises RuntimeError when _load_telethon_config raises.""" monkeypatch.setattr( "apps.handlers.botfather_client.check_telethon_setup", lambda: (True, "ready"), ) + + def _raise(): + raise RuntimeError("Telethon config not found in secrets store (telegram/telethon_config)") + monkeypatch.setattr( "apps.handlers.botfather_client._load_telethon_config", - lambda: None, + _raise, ) import pytest - with pytest.raises(RuntimeError, match="could not be loaded"): + with pytest.raises(RuntimeError, match="not found in secrets store"): create_bot_via_botfather("dev_central") def test_returns_result_on_success(self, monkeypatch): diff --git a/src/aipass/skills/lib/telegram/tests/test_multibot_config.py b/src/aipass/skills/lib/telegram/tests/test_multibot_config.py index 46ddfe74..4b18a00a 100644 --- a/src/aipass/skills/lib/telegram/tests/test_multibot_config.py +++ b/src/aipass/skills/lib/telegram/tests/test_multibot_config.py @@ -899,8 +899,11 @@ class TestCommandMenuSync: build_help_text, ) from apps.handlers.base_bot import BaseBot + from unittest.mock import MagicMock - custom = BaseBot.get_custom_commands(None) + mock_self = MagicMock(spec=BaseBot) + mock_self.branch_name = None + custom = BaseBot.get_custom_commands(mock_self) menu_commands = build_botfather_commands(custom_commands=custom) menu_names = {c["command"] for c in menu_commands} @@ -917,8 +920,11 @@ class TestCommandMenuSync: """The /help text includes the enriched descriptions.""" from apps.handlers.telegram_standards import build_help_text from apps.handlers.base_bot import BaseBot + from unittest.mock import MagicMock - custom = BaseBot.get_custom_commands(None) + mock_self = MagicMock(spec=BaseBot) + mock_self.branch_name = None + custom = BaseBot.get_custom_commands(mock_self) help_text = build_help_text(custom_commands=custom) assert "what this bot is and how to use it" in help_text.lower() @@ -966,6 +972,7 @@ class TestBaseBotStartupMenu: bot = BaseBot.__new__(BaseBot) bot.bot_token = "123:ABC" bot.custom_commands = {} + bot.branch_name = None bot._set_command_menu() @@ -982,6 +989,7 @@ class TestBaseBotStartupMenu: bot = BaseBot.__new__(BaseBot) bot.bot_token = "123:ABC" bot.custom_commands = {} + bot.branch_name = None bot._set_command_menu() diff --git a/src/aipass/skills/lib/telegram/tests/test_poll_and_gate.py b/src/aipass/skills/lib/telegram/tests/test_poll_and_gate.py new file mode 100644 index 00000000..601fecff --- /dev/null +++ b/src/aipass/skills/lib/telegram/tests/test_poll_and_gate.py @@ -0,0 +1,216 @@ +""" +Tests for poll-offset advancement (#668) and /create+/cancel base-bot gate (#644). + +Tests cover: + - Poll loop advances offset past rate-limited updates + - Poll loop advances offset past rejected (unauthorized) updates + - Poll loop advances offset on normal processing + - _dispatch_command gates /create to base bot only (branch_name is None) + - _dispatch_command gates /cancel to base bot only + - _dispatch_command allows /create on base bot + - get_custom_commands omits /create+/cancel for branch bots + - get_custom_commands includes /create+/cancel for base bot +""" + +import pytest +from unittest.mock import patch, MagicMock + +from apps.handlers.base_bot import BaseBot # type: ignore[import-not-found] + + +@pytest.fixture +def _patch_base_bot_deps(tmp_path): + patches = [ + patch("apps.handlers.base_bot.PENDING_DIR", tmp_path), + patch("apps.handlers.base_bot.signal.signal"), + patch("apps.handlers.base_bot.atexit.register"), + ] + for p in patches: + p.start() + yield + for p in patches: + p.stop() + + +def _make_bot(tmp_path, _patch_base_bot_deps, branch_name=None): + workdir = tmp_path / "workdir" + workdir.mkdir(exist_ok=True) + bot = BaseBot( + bot_id="test_bot", + bot_token="123:FAKETOKEN", + work_dir=workdir, + bot_name="Test Bot", + branch_name=branch_name, + ) + bot.verify_connection = lambda timeout=15: True + bot._set_command_menu = lambda: None + bot._boot_monitor = lambda: None + bot._check_lock = lambda: False + bot._create_lock = lambda: None + bot._remove_lock = lambda: None + return bot + + +# ============================================= +# 1. Poll offset advancement (#668) +# ============================================= + + +class TestPollOffsetAdvancement: + """Offset advances past every consumed update regardless of processing outcome.""" + + def test_offset_advances_past_rate_limited_update(self, tmp_path, _patch_base_bot_deps): + """Offset advances even when process_update hits the rate limiter.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps) + + updates = [ + {"update_id": 100, "message": {"chat": {"id": 1}, "from": {"id": 99}, "text": "hi"}}, + {"update_id": 101, "message": {"chat": {"id": 1}, "from": {"id": 99}, "text": "hi2"}}, + ] + + call_count = 0 + + def fake_poll(offset): + nonlocal call_count + call_count += 1 + if call_count == 1: + return updates + bot.state["running"] = False + return [] + + bot.poll_updates = fake_poll + bot._load_offset = lambda: 0 + saved_offsets = [] + bot._save_offset = lambda o: saved_offsets.append(o) + bot.check_rate_limit = lambda uid: False + bot.send_message = MagicMock() + + bot.run() + + assert 102 in saved_offsets + assert saved_offsets[-1] == 102 + + def test_offset_advances_past_unauthorized_update(self, tmp_path, _patch_base_bot_deps): + """Offset advances even when user is not in the allowlist.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps) + bot.allowed_user_ids = [777] + + updates = [ + {"update_id": 200, "message": {"chat": {"id": 1}, "from": {"id": 999}, "text": "intruder"}}, + ] + + call_count = 0 + + def fake_poll(offset): + nonlocal call_count + call_count += 1 + if call_count == 1: + return updates + bot.state["running"] = False + return [] + + bot.poll_updates = fake_poll + bot._load_offset = lambda: 0 + saved_offsets = [] + bot._save_offset = lambda o: saved_offsets.append(o) + + bot.run() + + assert saved_offsets == [201] + + def test_offset_advances_on_normal_message(self, tmp_path, _patch_base_bot_deps): + """Offset advances on successfully processed messages.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps) + + updates = [ + {"update_id": 50, "message": {"chat": {"id": 1}, "from": {"id": 1}, "text": "/help"}}, + ] + + call_count = 0 + + def fake_poll(offset): + nonlocal call_count + call_count += 1 + if call_count == 1: + return updates + bot.state["running"] = False + return [] + + bot.poll_updates = fake_poll + bot._load_offset = lambda: 0 + saved_offsets = [] + bot._save_offset = lambda o: saved_offsets.append(o) + bot.send_message = MagicMock() + + bot.run() + + assert saved_offsets == [51] + + +# ============================================= +# 2. /create + /cancel base-bot gate (#644) +# ============================================= + + +class TestCreateCancelGate: + """Only the base bot (branch_name is None) routes /create and /cancel.""" + + def test_branch_bot_does_not_route_create(self, tmp_path, _patch_base_bot_deps): + """A branch bot (branch_name set) falls through on /create.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name="devpulse") + bot.send_message = MagicMock() + result = bot._dispatch_command(42, ("create", "chat devpulse")) + assert result is False + bot.send_message.assert_not_called() + + def test_branch_bot_does_not_route_cancel(self, tmp_path, _patch_base_bot_deps): + """A branch bot (branch_name set) falls through on /cancel.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name="devpulse") + bot.send_message = MagicMock() + result = bot._dispatch_command(42, ("cancel", "")) + assert result is False + bot.send_message.assert_not_called() + + def test_base_bot_routes_create(self, tmp_path, _patch_base_bot_deps): + """The base bot (branch_name is None) handles /create.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name=None) + bot.send_message = MagicMock() + bot._handle_create_command = MagicMock() + result = bot._dispatch_command(42, ("create", "chat devpulse")) + assert result is True + bot._handle_create_command.assert_called_once_with(42, "chat devpulse") + + def test_base_bot_routes_cancel(self, tmp_path, _patch_base_bot_deps): + """The base bot (branch_name is None) handles /cancel.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name=None) + bot.send_message = MagicMock() + result = bot._dispatch_command(42, ("cancel", "")) + assert result is True + + def test_branch_bot_still_routes_monitor(self, tmp_path, _patch_base_bot_deps): + """Branch bots still handle /monitor (not gated).""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name="devpulse") + bot._handle_monitor_command = MagicMock() + result = bot._dispatch_command(42, ("monitor", "status")) + assert result is True + bot._handle_monitor_command.assert_called_once() + + +class TestGetCustomCommandsGate: + """get_custom_commands only advertises /create+/cancel for the base bot.""" + + def test_base_bot_includes_create_cancel(self, tmp_path, _patch_base_bot_deps): + """Base bot (branch_name is None) advertises /create and /cancel.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name=None) + commands = bot.get_custom_commands() + assert "create" in commands + assert "cancel" in commands + assert "monitor" in commands + + def test_branch_bot_omits_create_cancel(self, tmp_path, _patch_base_bot_deps): + """Branch bot (branch_name set) does NOT advertise /create or /cancel.""" + bot = _make_bot(tmp_path, _patch_base_bot_deps, branch_name="devpulse") + commands = bot.get_custom_commands() + assert "create" not in commands + assert "cancel" not in commands + assert "monitor" in commands