From fae8a2a044f45ede2330c8a5b732752dfa7301e1 Mon Sep 17 00:00:00 2001 From: Ali Khokhar Date: Sun, 1 Mar 2026 12:34:00 -0800 Subject: [PATCH] =?UTF-8?q?Remove=20over-engineering:=20drop=20tree=5Fqueu?= =?UTF-8?q?e=20setter,=20=5Fset=5Fconnected(),=20fi=E2=80=A6=20(#63)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …x cancel_all() TOCTOU - Remove tree_queue property setter (backward-compat hack; all callers already migrated to replace_tree_queue()); keep property getter only - Update 2 remaining tests that still used direct assignment to use replace_tree_queue() - Remove _set_connected() 1-line wrapper on DiscordPlatform; assign _connected directly - Fix cancel_all() TOCTOU: hold self._lock for the full loop so newly created trees cannot slip through between the snapshot and cancellation --------- Co-authored-by: Claude --- messaging/handler.py | 5 ----- messaging/platforms/discord.py | 10 +++------- messaging/trees/queue_manager.py | 9 ++++----- tests/messaging/test_handler.py | 4 ++-- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/messaging/handler.py b/messaging/handler.py index 9275b86..7eae77e 100644 --- a/messaging/handler.py +++ b/messaging/handler.py @@ -157,11 +157,6 @@ class ClaudeMessageHandler: """Accessor for the current tree queue manager.""" return self._tree_queue - @tree_queue.setter - def tree_queue(self, tree_queue: TreeQueueManager) -> None: - """Backward-compatible setter routed through explicit replacement API.""" - self.replace_tree_queue(tree_queue) - def replace_tree_queue(self, tree_queue: TreeQueueManager) -> None: """Replace tree queue manager via explicit API.""" self._tree_queue = tree_queue diff --git a/messaging/platforms/discord.py b/messaging/platforms/discord.py index d06134a..b0bbd1d 100644 --- a/messaging/platforms/discord.py +++ b/messaging/platforms/discord.py @@ -66,7 +66,7 @@ if DISCORD_AVAILABLE and _discord_module is not None: async def on_ready(self) -> None: """Called when the bot is ready.""" - self._platform._set_connected(True) + self._platform._connected = True logger.info("Discord platform connected") async def on_message(self, message: Any) -> None: @@ -118,10 +118,6 @@ class DiscordPlatform(MessagingPlatform): self._pending_voice: dict[tuple[str, str], tuple[str, str]] = {} self._pending_voice_lock = asyncio.Lock() - def _set_connected(self, connected: bool) -> None: - """Update connection state via an explicit accessor.""" - self._connected = connected - async def _handle_client_message(self, message: Any) -> None: """Adapter entry point used by the internal discord client.""" await self._on_discord_message(message) @@ -367,7 +363,7 @@ class DiscordPlatform(MessagingPlatform): async def stop(self) -> None: """Stop the bot.""" if self._client.is_closed(): - self._set_connected(False) + self._connected = False return await self._client.close() @@ -379,7 +375,7 @@ class DiscordPlatform(MessagingPlatform): with contextlib.suppress(asyncio.CancelledError): await self._start_task - self._set_connected(False) + self._connected = False logger.info("Discord platform stopped") async def send_message( diff --git a/messaging/trees/queue_manager.py b/messaging/trees/queue_manager.py index cf2450f..14b0f10 100644 --- a/messaging/trees/queue_manager.py +++ b/messaging/trees/queue_manager.py @@ -314,11 +314,10 @@ class TreeQueueManager: """Cancel all messages in all trees.""" async with self._lock: root_ids = list(self._repository.tree_ids()) - - all_cancelled: list[MessageNode] = [] - for root_id in root_ids: - all_cancelled.extend(await self.cancel_tree(root_id)) - return all_cancelled + all_cancelled: list[MessageNode] = [] + for root_id in root_ids: + all_cancelled.extend(await self.cancel_tree(root_id)) + return all_cancelled def cleanup_stale_nodes(self) -> int: """ diff --git a/tests/messaging/test_handler.py b/tests/messaging/test_handler.py index 70fe377..7695dd0 100644 --- a/tests/messaging/test_handler.py +++ b/tests/messaging/test_handler.py @@ -24,7 +24,7 @@ def test_get_initial_status_reply_tree_busy_queued(handler): mock_queue = MagicMock() mock_queue.is_node_tree_busy.return_value = True mock_queue.get_queue_size.return_value = 2 - handler.tree_queue = mock_queue + handler.replace_tree_queue(mock_queue) result = handler._get_initial_status(MagicMock(), "parent_1") assert "Queued" in result assert "position 3" in result @@ -34,7 +34,7 @@ def test_get_initial_status_reply_tree_not_busy_continuing(handler): """Reply to tree when not busy returns continuing message.""" mock_queue = MagicMock() mock_queue.is_node_tree_busy.return_value = False - handler.tree_queue = mock_queue + handler.replace_tree_queue(mock_queue) result = handler._get_initial_status(MagicMock(), "parent_1") assert "Continuing" in result