From b61ac8964b8889840a4841cf617e1a8b4de7763c Mon Sep 17 00:00:00 2001 From: Teknium Date: Thu, 23 Apr 2026 15:11:12 -0700 Subject: [PATCH] fix(gateway/discord): read permission attrs from AppCommand, canonicalize contexts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to Magaav's safe sync policy. Two gaps in the canonicalizer caused false diffs or silent drift: 1. discord.py's AppCommand.to_dict() omits nsfw, dm_permission, and default_member_permissions — those live only on attributes. The canonicalizer was reading them via payload.get() and getting defaults (False/True/None), while the desired side from Command.to_dict(tree) had the real values. Any command using non-default permissions false-diffed on every startup. Pull them from the AppCommand attributes via _existing_command_to_payload(). 2. contexts and integration_types weren't canonicalized at all, so drift in either was silently ignored. Added both to _canonicalize_app_command_payload (sorted for stable compare). Also normalized default_member_permissions to str-or-None since the server emits strings but discord.py stores ints locally. Added regression tests for both gaps. --- gateway/platforms/discord.py | 48 ++++++- tests/gateway/test_discord_connect.py | 184 ++++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 4 deletions(-) diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index 3587f661d..f741d45b5 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -845,13 +845,21 @@ class DiscordAdapter(BasePlatformAdapter): def _canonicalize_app_command_payload(self, payload: Dict[str, Any]) -> Dict[str, Any]: """Reduce command payloads to the semantic fields Hermes manages.""" + contexts = payload.get("contexts") + integration_types = payload.get("integration_types") 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), + "default_member_permissions": self._normalize_permissions( + payload.get("default_member_permissions") + ), + "dm_permission": bool(payload.get("dm_permission", True)), "nsfw": bool(payload.get("nsfw", False)), + "contexts": sorted(int(c) for c in contexts) if contexts else None, + "integration_types": ( + sorted(int(i) for i in integration_types) if integration_types else None + ), "options": [ self._canonicalize_app_command_option(item) for item in payload.get("options", []) or [] @@ -859,6 +867,37 @@ class DiscordAdapter(BasePlatformAdapter): ], } + @staticmethod + def _normalize_permissions(value: Any) -> Optional[str]: + """Discord emits default_member_permissions as str server-side but discord.py + sets it as int locally. Normalize to str-or-None so the comparison is stable.""" + if value is None: + return None + return str(value) + + def _existing_command_to_payload(self, command: Any) -> Dict[str, Any]: + """Build a canonical-ready dict from an AppCommand. + + discord.py's AppCommand.to_dict() does NOT include nsfw, + dm_permission, or default_member_permissions (they live only on the + attributes). Pull them from the attributes so the canonicalizer sees + the real server-side values instead of defaults — otherwise any + command using non-default permissions would diff on every startup. + """ + payload = dict(command.to_dict()) + nsfw = getattr(command, "nsfw", None) + if nsfw is not None: + payload["nsfw"] = bool(nsfw) + guild_only = getattr(command, "guild_only", None) + if guild_only is not None: + payload["dm_permission"] = not bool(guild_only) + default_permissions = getattr(command, "default_member_permissions", None) + if default_permissions is not None: + payload["default_member_permissions"] = getattr( + default_permissions, "value", default_permissions + ) + return payload + def _canonicalize_app_command_option(self, payload: Dict[str, Any]) -> Dict[str, Any]: return { "type": int(payload.get("type", 0) or 0), @@ -940,13 +979,14 @@ class DiscordAdapter(BasePlatformAdapter): created += 1 continue - current_payload = self._canonicalize_app_command_payload(current.to_dict()) + current_existing_payload = self._existing_command_to_payload(current) + current_payload = self._canonicalize_app_command_payload(current_existing_payload) 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): + if self._patchable_app_command_payload(current_existing_payload) == 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 diff --git a/tests/gateway/test_discord_connect.py b/tests/gateway/test_discord_connect.py index 35a57f2ac..d769d3f44 100644 --- a/tests/gateway/test_discord_connect.py +++ b/tests/gateway/test_discord_connect.py @@ -471,3 +471,187 @@ async def test_post_connect_initialization_skips_sync_when_policy_off(monkeypatc await adapter._run_post_connect_initialization() fake_tree.sync.assert_not_called() + + +@pytest.mark.asyncio +async def test_safe_sync_reads_permission_attrs_from_existing_command(): + """Regression: AppCommand.to_dict() in discord.py does NOT include + nsfw, dm_permission, or default_member_permissions — they live only + on the attributes. Without reading those attrs, any command with + non-default permissions false-diffs on every startup. + """ + adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + + class _DesiredCommand: + def __init__(self, payload): + self._payload = payload + + def to_dict(self, tree): + return dict(self._payload) + + class _ExistingCommand: + """Mirrors discord.py's AppCommand — to_dict() omits nsfw/dm/perms.""" + + def __init__(self, command_id, name, description, *, nsfw, guild_only, default_permissions): + self.id = command_id + self.name = name + self.description = description + self.type = SimpleNamespace(value=1) + self.nsfw = nsfw + self.guild_only = guild_only + self.default_member_permissions = ( + SimpleNamespace(value=default_permissions) + if default_permissions is not None + else None + ) + + def to_dict(self): + # Match real AppCommand.to_dict() — no nsfw/dm_permission/default_member_permissions + return { + "id": self.id, + "type": 1, + "application_id": 999, + "name": self.name, + "description": self.description, + "name_localizations": {}, + "description_localizations": {}, + "options": [], + } + + desired = { + "name": "admin", + "description": "Admin-only command", + "type": 1, + "options": [], + "nsfw": True, + "dm_permission": False, + "default_member_permissions": "8", + } + # Existing command has matching attrs — should report unchanged, NOT falsely diff. + existing = _ExistingCommand( + 42, + "admin", + "Admin-only command", + nsfw=True, + guild_only=True, + default_permissions=8, + ) + + 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() + + # Without the fix, this would be unchanged=0, recreated=1 (false diff). + assert summary == { + "total": 1, + "unchanged": 1, + "updated": 0, + "recreated": 0, + "created": 0, + "deleted": 0, + } + fake_http.edit_global_command.assert_not_awaited() + fake_http.delete_global_command.assert_not_awaited() + fake_http.upsert_global_command.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_safe_sync_detects_contexts_drift(): + """Regression: contexts and integration_types must be canonicalized + so drift in those fields triggers reconciliation. Without this, the + diff silently reports 'unchanged' and never reconciles. + """ + adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token")) + + class _DesiredCommand: + def __init__(self, payload): + self._payload = payload + + def to_dict(self, tree): + return dict(self._payload) + + class _ExistingCommand: + def __init__(self, command_id, payload): + self.id = command_id + self.name = payload["name"] + self.description = payload["description"] + self.type = SimpleNamespace(value=1) + self.nsfw = payload.get("nsfw", False) + self.guild_only = not payload.get("dm_permission", True) + self.default_member_permissions = None + self._payload = payload + + def to_dict(self): + return { + "id": self.id, + "type": 1, + "application_id": 999, + "name": self.name, + "description": self.description, + "name_localizations": {}, + "description_localizations": {}, + "options": [], + "contexts": self._payload.get("contexts"), + "integration_types": self._payload.get("integration_types"), + } + + desired = { + "name": "help", + "description": "Show available commands", + "type": 1, + "options": [], + "nsfw": False, + "dm_permission": True, + "default_member_permissions": None, + "contexts": [0, 1, 2], + "integration_types": [0, 1], + } + existing = _ExistingCommand( + 77, + { + **desired, + "contexts": [0], # server-side only + "integration_types": [0], + }, + ) + + 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() + + # contexts and integration_types are not patchable by + # edit_global_command, so the command must be recreated. + assert summary["unchanged"] == 0 + assert summary["recreated"] == 1 + assert summary["updated"] == 0 + fake_http.edit_global_command.assert_not_awaited() + fake_http.delete_global_command.assert_awaited_once_with(999, 77) + fake_http.upsert_global_command.assert_awaited_once_with(999, desired)