From a1ff6b45eaf7f4876315e8f8da61b2ec26b49674 Mon Sep 17 00:00:00 2001 From: Magaav <73175452+Magaav@users.noreply.github.com> Date: Thu, 23 Apr 2026 15:09:04 -0700 Subject: [PATCH] fix(gateway/discord): add safe startup slash sync policy Replaces blind tree.sync() on every Discord reconnect with a diff-based reconcile. In safe mode (default), fetch existing global commands, compare desired vs existing payloads, skip unchanged, PATCH changed, recreate when non-patchable metadata differs, POST missing, and delete stale commands one-by-one. Keeps 'bulk' for legacy behavior and 'off' to skip startup sync entirely. Fixes restart-heavy workflows that burn Discord's command write budget and can surface 429s when iterating on native slash commands. Env var: DISCORD_COMMAND_SYNC_POLICY (safe|bulk|off), default 'safe'. Co-authored-by: Codex --- gateway/platforms/discord.py | 161 +++++++++++- tests/gateway/test_discord_connect.py | 245 ++++++++++++++++++ .../docs/reference/environment-variables.md | 1 + website/docs/user-guide/messaging/discord.md | 2 +- 4 files changed, 406 insertions(+), 3 deletions(-) diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index a148c5f4b..3587f661d 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -23,6 +23,7 @@ from typing import Callable, Dict, Optional, Any logger = logging.getLogger(__name__) VALID_THREAD_AUTO_ARCHIVE_MINUTES = {60, 1440, 4320, 10080} +_DISCORD_COMMAND_SYNC_POLICIES = {"safe", "bulk", "off"} try: import discord @@ -802,8 +803,27 @@ class DiscordAdapter(BasePlatformAdapter): if not self._client: return try: - synced = await asyncio.wait_for(self._client.tree.sync(), timeout=30) - logger.info("[%s] Synced %d slash command(s)", self.name, len(synced)) + sync_policy = self._get_discord_command_sync_policy() + if sync_policy == "off": + logger.info("[%s] Skipping Discord slash command sync (policy=off)", self.name) + return + + if sync_policy == "bulk": + synced = await asyncio.wait_for(self._client.tree.sync(), timeout=30) + logger.info("[%s] Synced %d slash command(s) via bulk tree sync", self.name, len(synced)) + return + + summary = await asyncio.wait_for(self._safe_sync_slash_commands(), timeout=30) + logger.info( + "[%s] Safely reconciled %d slash command(s): unchanged=%d updated=%d recreated=%d created=%d deleted=%d", + self.name, + summary["total"], + summary["unchanged"], + summary["updated"], + summary["recreated"], + summary["created"], + summary["deleted"], + ) except asyncio.TimeoutError: logger.warning("[%s] Slash command sync timed out after 30s", self.name) except asyncio.CancelledError: @@ -811,6 +831,143 @@ class DiscordAdapter(BasePlatformAdapter): except Exception as e: # pragma: no cover - defensive logging logger.warning("[%s] Slash command sync failed: %s", self.name, e, exc_info=True) + def _get_discord_command_sync_policy(self) -> str: + raw = str(os.getenv("DISCORD_COMMAND_SYNC_POLICY", "safe") or "").strip().lower() + if raw in _DISCORD_COMMAND_SYNC_POLICIES: + return raw + if raw: + logger.warning( + "[%s] Invalid DISCORD_COMMAND_SYNC_POLICY=%r; falling back to 'safe'", + self.name, + raw, + ) + return "safe" + + def _canonicalize_app_command_payload(self, payload: Dict[str, Any]) -> Dict[str, Any]: + """Reduce command payloads to the semantic fields Hermes manages.""" + return { + "type": int(payload.get("type", 1) or 1), + "name": str(payload.get("name", "") or ""), + "description": str(payload.get("description", "") or ""), + "default_member_permissions": payload.get("default_member_permissions"), + "dm_permission": payload.get("dm_permission", True), + "nsfw": bool(payload.get("nsfw", False)), + "options": [ + self._canonicalize_app_command_option(item) + for item in payload.get("options", []) or [] + if isinstance(item, dict) + ], + } + + def _canonicalize_app_command_option(self, payload: Dict[str, Any]) -> Dict[str, Any]: + return { + "type": int(payload.get("type", 0) or 0), + "name": str(payload.get("name", "") or ""), + "description": str(payload.get("description", "") or ""), + "required": bool(payload.get("required", False)), + "autocomplete": bool(payload.get("autocomplete", False)), + "choices": [ + { + "name": str(choice.get("name", "") or ""), + "value": choice.get("value"), + } + for choice in payload.get("choices", []) or [] + if isinstance(choice, dict) + ], + "channel_types": list(payload.get("channel_types", []) or []), + "min_value": payload.get("min_value"), + "max_value": payload.get("max_value"), + "min_length": payload.get("min_length"), + "max_length": payload.get("max_length"), + "options": [ + self._canonicalize_app_command_option(item) + for item in payload.get("options", []) or [] + if isinstance(item, dict) + ], + } + + def _patchable_app_command_payload(self, payload: Dict[str, Any]) -> Dict[str, Any]: + """Fields supported by discord.py's edit_global_command route.""" + canonical = self._canonicalize_app_command_payload(payload) + return { + "name": canonical["name"], + "description": canonical["description"], + "options": canonical["options"], + } + + async def _safe_sync_slash_commands(self) -> Dict[str, int]: + """Diff existing global commands and only mutate the commands that changed.""" + if not self._client: + return { + "total": 0, + "unchanged": 0, + "updated": 0, + "recreated": 0, + "created": 0, + "deleted": 0, + } + + tree = self._client.tree + app_id = getattr(self._client, "application_id", None) or getattr(getattr(self._client, "user", None), "id", None) + if not app_id: + raise RuntimeError("Discord application ID is unavailable for slash command sync") + + desired_payloads = [command.to_dict(tree) for command in tree.get_commands()] + desired_by_key = { + (int(payload.get("type", 1) or 1), str(payload.get("name", "") or "").lower()): payload + for payload in desired_payloads + } + existing_commands = await tree.fetch_commands() + existing_by_key = { + ( + int(getattr(getattr(command, "type", None), "value", getattr(command, "type", 1)) or 1), + str(command.name or "").lower(), + ): command + for command in existing_commands + } + + unchanged = 0 + updated = 0 + recreated = 0 + created = 0 + deleted = 0 + http = self._client.http + + for key, desired in desired_by_key.items(): + current = existing_by_key.pop(key, None) + if current is None: + await http.upsert_global_command(app_id, desired) + created += 1 + continue + + current_payload = self._canonicalize_app_command_payload(current.to_dict()) + desired_payload = self._canonicalize_app_command_payload(desired) + if current_payload == desired_payload: + unchanged += 1 + continue + + if self._patchable_app_command_payload(current.to_dict()) == self._patchable_app_command_payload(desired): + await http.delete_global_command(app_id, current.id) + await http.upsert_global_command(app_id, desired) + recreated += 1 + continue + + await http.edit_global_command(app_id, current.id, desired) + updated += 1 + + for current in existing_by_key.values(): + await http.delete_global_command(app_id, current.id) + deleted += 1 + + return { + "total": len(desired_payloads), + "unchanged": unchanged, + "updated": updated, + "recreated": recreated, + "created": created, + "deleted": deleted, + } + async def _add_reaction(self, message: Any, emoji: str) -> bool: """Add an emoji reaction to a Discord message.""" if not message or not hasattr(message, "add_reaction"): diff --git a/tests/gateway/test_discord_connect.py b/tests/gateway/test_discord_connect.py index 0ac1c9ba3..35a57f2ac 100644 --- a/tests/gateway/test_discord_connect.py +++ b/tests/gateway/test_discord_connect.py @@ -73,18 +73,29 @@ from gateway.platforms.discord import DiscordAdapter # noqa: E402 class FakeTree: def __init__(self): self.sync = AsyncMock(return_value=[]) + self.fetch_commands = AsyncMock(return_value=[]) + self._commands = [] def command(self, *args, **kwargs): return lambda fn: fn + def get_commands(self, *args, **kwargs): + return list(self._commands) + class FakeBot: def __init__(self, *, intents, proxy=None, allowed_mentions=None, **_): self.intents = intents self.allowed_mentions = allowed_mentions + self.application_id = 999 self.user = SimpleNamespace(id=999, name="Hermes") self._events = {} self.tree = FakeTree() + self.http = SimpleNamespace( + upsert_global_command=AsyncMock(), + edit_global_command=AsyncMock(), + delete_global_command=AsyncMock(), + ) def event(self, fn): self._events[fn.__name__] = fn @@ -199,6 +210,7 @@ async def test_connect_releases_token_lock_on_timeout(monkeypatch): async def test_connect_does_not_wait_for_slash_sync(monkeypatch): adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + monkeypatch.setenv("DISCORD_COMMAND_SYNC_POLICY", "bulk") monkeypatch.setattr("gateway.status.acquire_scoped_lock", lambda scope, identity, metadata=None: (True, None)) monkeypatch.setattr("gateway.status.release_scoped_lock", lambda scope, identity: None) @@ -226,3 +238,236 @@ async def test_connect_does_not_wait_for_slash_sync(monkeypatch): created["bot"].tree.allow_finish.set() await asyncio.sleep(0) await adapter.disconnect() + + +@pytest.mark.asyncio +async def test_connect_respects_slash_commands_opt_out(monkeypatch): + adapter = DiscordAdapter( + PlatformConfig(enabled=True, token="test-token", extra={"slash_commands": False}) + ) + + monkeypatch.setenv("DISCORD_COMMAND_SYNC_POLICY", "off") + monkeypatch.setattr("gateway.status.acquire_scoped_lock", lambda scope, identity, metadata=None: (True, None)) + monkeypatch.setattr("gateway.status.release_scoped_lock", lambda scope, identity: None) + + intents = SimpleNamespace(message_content=False, dm_messages=False, guild_messages=False, members=False, voice_states=False) + monkeypatch.setattr(discord_platform.Intents, "default", lambda: intents) + monkeypatch.setattr( + discord_platform.commands, + "Bot", + lambda **kwargs: FakeBot( + intents=kwargs["intents"], + proxy=kwargs.get("proxy"), + allowed_mentions=kwargs.get("allowed_mentions"), + ), + ) + register_mock = MagicMock() + monkeypatch.setattr(adapter, "_register_slash_commands", register_mock) + monkeypatch.setattr(adapter, "_resolve_allowed_usernames", AsyncMock()) + + ok = await adapter.connect() + + assert ok is True + register_mock.assert_not_called() + + await adapter.disconnect() + + +@pytest.mark.asyncio +async def test_safe_sync_slash_commands_only_mutates_diffs(): + adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + + class _DesiredCommand: + def __init__(self, payload): + self._payload = payload + + def to_dict(self, tree): + assert tree is not None + return dict(self._payload) + + class _ExistingCommand: + def __init__(self, command_id, payload): + self.id = command_id + self.name = payload["name"] + self.type = SimpleNamespace(value=payload["type"]) + self._payload = payload + + def to_dict(self): + return { + "id": self.id, + "application_id": 999, + **self._payload, + "name_localizations": {}, + "description_localizations": {}, + } + + desired_same = { + "name": "status", + "description": "Show Hermes session status", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": None, + } + desired_updated = { + "name": "help", + "description": "Show available commands", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": None, + } + desired_created = { + "name": "metricas", + "description": "Show Colmeio metrics dashboard", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": None, + } + existing_same = _ExistingCommand(11, desired_same) + existing_updated = _ExistingCommand( + 12, + { + **desired_updated, + "description": "Old help text", + }, + ) + existing_deleted = _ExistingCommand( + 13, + { + "name": "old-command", + "description": "To be deleted", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": None, + }, + ) + + fake_tree = SimpleNamespace( + get_commands=lambda: [ + _DesiredCommand(desired_same), + _DesiredCommand(desired_updated), + _DesiredCommand(desired_created), + ], + fetch_commands=AsyncMock(return_value=[existing_same, existing_updated, existing_deleted]), + ) + fake_http = SimpleNamespace( + upsert_global_command=AsyncMock(), + edit_global_command=AsyncMock(), + delete_global_command=AsyncMock(), + ) + adapter._client = SimpleNamespace( + tree=fake_tree, + http=fake_http, + application_id=999, + user=SimpleNamespace(id=999), + ) + + summary = await adapter._safe_sync_slash_commands() + + assert summary == { + "total": 3, + "unchanged": 1, + "updated": 1, + "recreated": 0, + "created": 1, + "deleted": 1, + } + fake_http.edit_global_command.assert_awaited_once_with(999, 12, desired_updated) + fake_http.upsert_global_command.assert_awaited_once_with(999, desired_created) + fake_http.delete_global_command.assert_awaited_once_with(999, 13) + + +@pytest.mark.asyncio +async def test_safe_sync_slash_commands_recreates_metadata_only_diffs(): + adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + + class _DesiredCommand: + def __init__(self, payload): + self._payload = payload + + def to_dict(self, tree): + assert tree is not None + return dict(self._payload) + + class _ExistingCommand: + def __init__(self, command_id, payload): + self.id = command_id + self.name = payload["name"] + self.type = SimpleNamespace(value=payload["type"]) + self._payload = payload + + def to_dict(self): + return { + "id": self.id, + "application_id": 999, + **self._payload, + "name_localizations": {}, + "description_localizations": {}, + } + + desired = { + "name": "help", + "description": "Show available commands", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": "8", + } + existing = _ExistingCommand( + 12, + { + **desired, + "default_member_permissions": None, + }, + ) + + fake_tree = SimpleNamespace( + get_commands=lambda: [_DesiredCommand(desired)], + fetch_commands=AsyncMock(return_value=[existing]), + ) + fake_http = SimpleNamespace( + upsert_global_command=AsyncMock(), + edit_global_command=AsyncMock(), + delete_global_command=AsyncMock(), + ) + adapter._client = SimpleNamespace( + tree=fake_tree, + http=fake_http, + application_id=999, + user=SimpleNamespace(id=999), + ) + + summary = await adapter._safe_sync_slash_commands() + + assert summary == { + "total": 1, + "unchanged": 0, + "updated": 0, + "recreated": 1, + "created": 0, + "deleted": 0, + } + fake_http.edit_global_command.assert_not_awaited() + fake_http.delete_global_command.assert_awaited_once_with(999, 12) + fake_http.upsert_global_command.assert_awaited_once_with(999, desired) + + +@pytest.mark.asyncio +async def test_post_connect_initialization_skips_sync_when_policy_off(monkeypatch): + adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + monkeypatch.setenv("DISCORD_COMMAND_SYNC_POLICY", "off") + + fake_tree = SimpleNamespace(sync=AsyncMock()) + adapter._client = SimpleNamespace(tree=fake_tree) + + await adapter._run_post_connect_initialization() + + fake_tree.sync.assert_not_called() diff --git a/website/docs/reference/environment-variables.md b/website/docs/reference/environment-variables.md index 886db482c..42280b4df 100644 --- a/website/docs/reference/environment-variables.md +++ b/website/docs/reference/environment-variables.md @@ -206,6 +206,7 @@ For cloud sandbox backends, persistence is filesystem-oriented. `TERMINAL_LIFETI | `DISCORD_PROXY` | Proxy URL for Discord connections — overrides `HTTPS_PROXY`. Supports `http://`, `https://`, `socks5://` | | `DISCORD_HOME_CHANNEL` | Default Discord channel for cron delivery | | `DISCORD_HOME_CHANNEL_NAME` | Display name for the Discord home channel | +| `DISCORD_COMMAND_SYNC_POLICY` | Discord slash-command startup sync policy: `safe` (diff and reconcile), `bulk` (legacy `tree.sync()`), or `off` | | `DISCORD_REQUIRE_MENTION` | Require an @mention before responding in server channels | | `DISCORD_FREE_RESPONSE_CHANNELS` | Comma-separated channel IDs where mention is not required | | `DISCORD_AUTO_THREAD` | Auto-thread long replies when supported | diff --git a/website/docs/user-guide/messaging/discord.md b/website/docs/user-guide/messaging/discord.md index 2a38b9798..d2b06f023 100644 --- a/website/docs/user-guide/messaging/discord.md +++ b/website/docs/user-guide/messaging/discord.md @@ -275,6 +275,7 @@ Discord behavior is controlled through two files: **`~/.hermes/.env`** for crede | `DISCORD_ALLOWED_ROLES` | No | — | Comma-separated Discord role IDs. Any member with one of these roles is authorized — OR semantics with `DISCORD_ALLOWED_USERS`. Auto-enables the **Server Members Intent** on connect. Useful when moderation teams churn: new mods get access as soon as the role is granted, no config push needed. | | `DISCORD_HOME_CHANNEL` | No | — | Channel ID where the bot sends proactive messages (cron output, reminders, notifications). | | `DISCORD_HOME_CHANNEL_NAME` | No | `"Home"` | Display name for the home channel in logs and status output. | +| `DISCORD_COMMAND_SYNC_POLICY` | No | `"safe"` | Controls native slash-command startup sync. `"safe"` diffs existing global commands and only updates what changed, recreating commands when Discord metadata changes cannot be applied via patch. `"bulk"` preserves the old `tree.sync()` behavior. `"off"` skips startup sync entirely. | | `DISCORD_REQUIRE_MENTION` | No | `true` | When `true`, the bot only responds in server channels when `@mentioned`. Set to `false` to respond to all messages in every channel. | | `DISCORD_FREE_RESPONSE_CHANNELS` | No | — | Comma-separated channel IDs where the bot responds without requiring an `@mention`, even when `DISCORD_REQUIRE_MENTION` is `true`. | | `DISCORD_IGNORE_NO_MENTION` | No | `true` | When `true`, the bot stays silent if a message `@mentions` other users but does **not** mention the bot. Prevents the bot from jumping into conversations directed at other people. Only applies in server channels, not DMs. | @@ -628,4 +629,3 @@ Leave `everyone` and `roles` at `false` unless you know exactly why you need the For more information on securing your Hermes Agent deployment, see the [Security Guide](../security.md). -