From ea2dbdb4b5b96ea8632a4455fdd6212d9f0ffad2 Mon Sep 17 00:00:00 2001 From: Trip T Date: Wed, 11 Mar 2026 20:33:59 -0400 Subject: [PATCH] fix: test DB isolation, Discord recovery, and over-mocked tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test data was bleeding into production tasks.db because swarm.task_queue.models.DB_PATH (relative path) was never patched in conftest.clean_database. Fixed by switching to absolute paths via settings.repo_root and adding the missing module to the patching list. Discord bot could leak orphaned clients on retry after ERROR state. Added _cleanup_stale() to close stale client/task before each start() attempt, with improved logging in the token watcher. Rewrote test_paperclip_client.py to use httpx.MockTransport instead of patching _get/_post/_delete — tests now exercise real HTTP status codes, error handling, and JSON parsing. Added end-to-end test for capture_error → create_task DB isolation. Co-Authored-By: Claude Opus 4.6 --- src/dashboard/app.py | 8 + src/dashboard/routes/work_orders.py | 3 +- .../chat_bridge/vendors/discord.py | 26 ++- src/swarm/task_queue/models.py | 9 +- tests/conftest.py | 6 +- tests/infrastructure/test_error_capture.py | 34 +++ tests/integrations/test_discord_vendor.py | 36 ++++ tests/integrations/test_paperclip_client.py | 197 ++++++++++++------ 8 files changed, 253 insertions(+), 66 deletions(-) diff --git a/src/dashboard/app.py b/src/dashboard/app.py index cc2d3de7..b7b3f398 100644 --- a/src/dashboard/app.py +++ b/src/dashboard/app.py @@ -235,10 +235,18 @@ async def _discord_token_watcher() -> None: if token: try: + logger.info( + "Discord watcher: token found, attempting start (state=%s)", + discord_bot.state.name, + ) success = await discord_bot.start(token=token) if success: logger.info("Discord bot auto-started (token detected)") return # Done — stop watching + logger.warning( + "Discord watcher: start() returned False (state=%s)", + discord_bot.state.name, + ) except Exception as exc: logger.warning("Discord auto-start failed: %s", exc) diff --git a/src/dashboard/routes/work_orders.py b/src/dashboard/routes/work_orders.py index 30e144fa..7fe3c5fb 100644 --- a/src/dashboard/routes/work_orders.py +++ b/src/dashboard/routes/work_orders.py @@ -9,13 +9,14 @@ from pathlib import Path from fastapi import APIRouter, Form, HTTPException, Request from fastapi.responses import HTMLResponse +from config import settings from dashboard.templating import templates logger = logging.getLogger(__name__) router = APIRouter(tags=["work-orders"]) -DB_PATH = Path("data/work_orders.db") +DB_PATH = Path(settings.repo_root) / "data" / "work_orders.db" PRIORITIES = ["low", "medium", "high", "critical"] CATEGORIES = ["bug", "feature", "suggestion", "maintenance", "security"] diff --git a/src/integrations/chat_bridge/vendors/discord.py b/src/integrations/chat_bridge/vendors/discord.py index 38e1b2e5..1ecec9ce 100644 --- a/src/integrations/chat_bridge/vendors/discord.py +++ b/src/integrations/chat_bridge/vendors/discord.py @@ -134,6 +134,12 @@ class DiscordVendor(ChatPlatform): logger.warning("Discord bot: no token configured, skipping start.") return False + # Clean up any stale client/task from a previous failed attempt + # so we don't leak background tasks or hold orphaned connections. + # Must happen before the import check — the old client could exist + # from when discord.py was available. + await self._cleanup_stale() + try: import discord except ImportError: @@ -163,9 +169,10 @@ class DiscordVendor(ChatPlatform): logger.info("Discord bot connected (%d guilds).", self._guild_count) return True if self._state == PlatformState.ERROR: + logger.warning("Discord bot: entered ERROR state during connection.") return False - logger.warning("Discord bot: connection timed out.") + logger.warning("Discord bot: connection timed out after 15s.") self._state = PlatformState.ERROR return False @@ -176,6 +183,23 @@ class DiscordVendor(ChatPlatform): self._client = None return False + async def _cleanup_stale(self) -> None: + """Close any orphaned client/task from a previous failed start.""" + if self._client and not self._client.is_closed(): + try: + await self._client.close() + except Exception: + pass + self._client = None + + if self._task and not self._task.done(): + self._task.cancel() + try: + await self._task + except (asyncio.CancelledError, Exception): + pass + self._task = None + async def stop(self) -> None: """Gracefully disconnect the Discord bot.""" if self._client and not self._client.is_closed(): diff --git a/src/swarm/task_queue/models.py b/src/swarm/task_queue/models.py index 95fd5df5..c9f1b38c 100644 --- a/src/swarm/task_queue/models.py +++ b/src/swarm/task_queue/models.py @@ -13,7 +13,14 @@ from pathlib import Path logger = logging.getLogger(__name__) -DB_PATH = Path("data/tasks.db") +# Use absolute path via settings.repo_root so tests can reliably redirect it +# and relative-path CWD differences don't cause DB leaks. +try: + from config import settings as _settings + + DB_PATH = Path(_settings.repo_root) / "data" / "tasks.db" +except Exception: + DB_PATH = Path("data/tasks.db") @dataclass diff --git a/tests/conftest.py b/tests/conftest.py index f9aebeef..1861b434 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -104,10 +104,14 @@ def clean_database(tmp_path): except Exception: pass - # Redirect task queue and work orders DBs to temp dir + # Redirect task queue and work orders DBs to temp dir. + # IMPORTANT: swarm.task_queue.models also has a DB_PATH that writes to + # tasks.db — it MUST be patched too, or error_capture.capture_error() + # will write test data to the production database. for mod_name, tmp_db in [ ("dashboard.routes.tasks", tmp_tasks_db), ("dashboard.routes.work_orders", tmp_work_orders_db), + ("swarm.task_queue.models", tmp_tasks_db), ]: try: mod = __import__(mod_name, fromlist=["DB_PATH"]) diff --git a/tests/infrastructure/test_error_capture.py b/tests/infrastructure/test_error_capture.py index f04a4df6..7b31dfc8 100644 --- a/tests/infrastructure/test_error_capture.py +++ b/tests/infrastructure/test_error_capture.py @@ -1,5 +1,7 @@ """Tests for infrastructure.error_capture module.""" +import sqlite3 + from infrastructure.error_capture import ( _dedup_cache, _get_git_context, @@ -117,5 +119,37 @@ class TestCaptureError: except RuntimeError as e: capture_error(e, source="test_module", context={"path": "/api/foo"}) + def test_capture_creates_task_in_temp_db(self): + """capture_error should write a task to the isolated temp DB, not production. + + This validates that conftest.clean_database properly redirects + swarm.task_queue.models.DB_PATH to the per-test temp directory. + """ + _dedup_cache.clear() + + try: + raise ValueError("Test error for DB isolation check") + except ValueError as e: + task_id = capture_error(e, source="test") + + # task_id can be None if error_feedback_enabled is False — that's fine, + # but if it was created, verify it landed in the temp DB + if task_id: + import swarm.task_queue.models as tq_models + + db_path = tq_models.DB_PATH + conn = sqlite3.connect(str(db_path)) + try: + row = conn.execute( + "SELECT id, title FROM tasks WHERE id = ?", (task_id,) + ).fetchone() + assert row is not None, ( + f"Task {task_id} not found in DB at {db_path} — " + "check conftest.clean_database patches swarm.task_queue.models.DB_PATH" + ) + assert "[BUG]" in row[1] + finally: + conn.close() + def teardown_method(self): _dedup_cache.clear() diff --git a/tests/integrations/test_discord_vendor.py b/tests/integrations/test_discord_vendor.py index e248bb0d..69547178 100644 --- a/tests/integrations/test_discord_vendor.py +++ b/tests/integrations/test_discord_vendor.py @@ -96,6 +96,42 @@ class TestDiscordVendor: await vendor.stop() assert vendor.state == PlatformState.DISCONNECTED + @pytest.mark.asyncio + async def test_cleanup_stale_closes_orphaned_client(self): + """_cleanup_stale should close any leftover client from a failed start.""" + from integrations.chat_bridge.vendors.discord import DiscordVendor + + vendor = DiscordVendor() + mock_client = MagicMock() + mock_client.is_closed.return_value = False + mock_client.close = AsyncMock() + vendor._client = mock_client + + await vendor._cleanup_stale() + mock_client.close.assert_called_once() + assert vendor._client is None + + @pytest.mark.asyncio + async def test_start_from_error_state_cleans_up(self): + """start() after ERROR should clean up stale state before retrying.""" + from integrations.chat_bridge.vendors.discord import DiscordVendor + + vendor = DiscordVendor() + vendor._state = PlatformState.ERROR + # Stale client from previous failed attempt + mock_old_client = MagicMock() + mock_old_client.is_closed.return_value = False + mock_old_client.close = AsyncMock() + vendor._client = mock_old_client + + # start() should clean up the old client even though discord.py import + # will fail (we're in test mode with MagicMock stub) + with patch.dict("sys.modules", {"discord": None}): + result = await vendor.start(token="fake-token") + + assert result is False + mock_old_client.close.assert_called_once() + def test_get_oauth2_url_no_client(self): from integrations.chat_bridge.vendors.discord import DiscordVendor diff --git a/tests/integrations/test_paperclip_client.py b/tests/integrations/test_paperclip_client.py index daa0f7a2..5acd298e 100644 --- a/tests/integrations/test_paperclip_client.py +++ b/tests/integrations/test_paperclip_client.py @@ -1,140 +1,213 @@ -"""Tests for the Paperclip API client.""" +"""Tests for the Paperclip API client. -from unittest.mock import AsyncMock, patch +Uses httpx.MockTransport so every test exercises the real HTTP path +(_get/_post/_delete, status-code handling, JSON parsing, error paths) +instead of patching the transport methods away. +""" -import pytest +import json +from unittest.mock import patch + +import httpx from integrations.paperclip.client import PaperclipClient from integrations.paperclip.models import CreateIssueRequest - -@pytest.fixture -def client(): - return PaperclipClient(base_url="http://fake:3100", api_key="test-key") +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- -# ── health ─────────────────────────────────────────────────────────────────── +def _mock_transport(routes: dict[str, tuple[int, dict | list | None]]): + """Build an httpx.MockTransport from a {method+path: (status, body)} map. + + Example: + _mock_transport({ + "GET /api/health": (200, {"status": "ok"}), + "DELETE /api/issues/i1": (204, None), + }) + """ + + def handler(request: httpx.Request) -> httpx.Response: + key = f"{request.method} {request.url.path}" + if key in routes: + status, body = routes[key] + content = json.dumps(body).encode() if body is not None else b"" + return httpx.Response( + status, content=content, headers={"content-type": "application/json"} + ) + return httpx.Response(404, json={"error": "not found"}) + + return httpx.MockTransport(handler) -async def test_healthy_returns_true_on_success(client): - with patch.object(client, "_get", new_callable=AsyncMock, return_value={"status": "ok"}): - assert await client.healthy() is True +def _client_with(routes: dict[str, tuple[int, dict | list | None]]) -> PaperclipClient: + """Create a PaperclipClient whose internal httpx.AsyncClient uses a mock transport.""" + client = PaperclipClient(base_url="http://fake:3100", api_key="test-key") + client._client = httpx.AsyncClient( + transport=_mock_transport(routes), + base_url="http://fake:3100", + headers={"Accept": "application/json", "Authorization": "Bearer test-key"}, + ) + return client -async def test_healthy_returns_false_on_failure(client): - with patch.object(client, "_get", new_callable=AsyncMock, return_value=None): - assert await client.healthy() is False +# --------------------------------------------------------------------------- +# health +# --------------------------------------------------------------------------- -# ── agents ─────────────────────────────────────────────────────────────────── +async def test_healthy_returns_true_on_200(): + client = _client_with({"GET /api/health": (200, {"status": "ok"})}) + assert await client.healthy() is True -async def test_list_agents_returns_list(client): +async def test_healthy_returns_false_on_500(): + client = _client_with({"GET /api/health": (500, {"error": "down"})}) + assert await client.healthy() is False + + +async def test_healthy_returns_false_on_404(): + client = _client_with({}) # no routes → 404 + assert await client.healthy() is False + + +# --------------------------------------------------------------------------- +# agents +# --------------------------------------------------------------------------- + + +async def test_list_agents_parses_response(): raw = [{"id": "a1", "name": "Codex", "role": "engineer", "status": "active"}] - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - with patch("integrations.paperclip.client.settings") as mock_settings: - mock_settings.paperclip_company_id = "comp-1" - agents = await client.list_agents(company_id="comp-1") + client = _client_with({"GET /api/companies/comp-1/agents": (200, raw)}) + agents = await client.list_agents(company_id="comp-1") assert len(agents) == 1 assert agents[0].name == "Codex" + assert agents[0].id == "a1" -async def test_list_agents_graceful_on_none(client): - with patch.object(client, "_get", new_callable=AsyncMock, return_value=None): - agents = await client.list_agents(company_id="comp-1") +async def test_list_agents_empty_on_server_error(): + client = _client_with({"GET /api/companies/comp-1/agents": (503, None)}) + agents = await client.list_agents(company_id="comp-1") assert agents == [] -# ── issues ─────────────────────────────────────────────────────────────────── +async def test_list_agents_graceful_on_404(): + client = _client_with({}) + agents = await client.list_agents(company_id="comp-1") + assert agents == [] -async def test_list_issues(client): +# --------------------------------------------------------------------------- +# issues +# --------------------------------------------------------------------------- + + +async def test_list_issues(): raw = [{"id": "i1", "title": "Fix bug"}] - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - issues = await client.list_issues(company_id="comp-1") + client = _client_with({"GET /api/companies/comp-1/issues": (200, raw)}) + issues = await client.list_issues(company_id="comp-1") assert len(issues) == 1 assert issues[0].title == "Fix bug" -async def test_get_issue(client): +async def test_get_issue(): raw = {"id": "i1", "title": "Fix bug", "description": "It's broken"} - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - issue = await client.get_issue("i1") + client = _client_with({"GET /api/issues/i1": (200, raw)}) + issue = await client.get_issue("i1") assert issue is not None assert issue.id == "i1" -async def test_get_issue_not_found(client): - with patch.object(client, "_get", new_callable=AsyncMock, return_value=None): - issue = await client.get_issue("nonexistent") +async def test_get_issue_not_found(): + client = _client_with({"GET /api/issues/nonexistent": (404, None)}) + issue = await client.get_issue("nonexistent") assert issue is None -async def test_create_issue(client): +async def test_create_issue(): raw = {"id": "i2", "title": "New feature"} - with patch.object(client, "_post", new_callable=AsyncMock, return_value=raw): - req = CreateIssueRequest(title="New feature") - issue = await client.create_issue(req, company_id="comp-1") + client = _client_with({"POST /api/companies/comp-1/issues": (201, raw)}) + req = CreateIssueRequest(title="New feature") + issue = await client.create_issue(req, company_id="comp-1") assert issue is not None assert issue.id == "i2" -async def test_create_issue_no_company_id(client): +async def test_create_issue_no_company_id(): + """Missing company_id returns None without making any HTTP call.""" + client = _client_with({}) with patch("integrations.paperclip.client.settings") as mock_settings: mock_settings.paperclip_company_id = "" - issue = await client.create_issue( - CreateIssueRequest(title="Test"), - ) + issue = await client.create_issue(CreateIssueRequest(title="Test")) assert issue is None -async def test_delete_issue(client): - with patch.object(client, "_delete", new_callable=AsyncMock, return_value=True): - result = await client.delete_issue("i1") +async def test_delete_issue_returns_true_on_success(): + client = _client_with({"DELETE /api/issues/i1": (204, None)}) + result = await client.delete_issue("i1") assert result is True -# ── comments ───────────────────────────────────────────────────────────────── +async def test_delete_issue_returns_false_on_error(): + client = _client_with({"DELETE /api/issues/i1": (500, None)}) + result = await client.delete_issue("i1") + assert result is False -async def test_add_comment(client): +# --------------------------------------------------------------------------- +# comments +# --------------------------------------------------------------------------- + + +async def test_add_comment(): raw = {"id": "c1", "issue_id": "i1", "content": "Done"} - with patch.object(client, "_post", new_callable=AsyncMock, return_value=raw): - comment = await client.add_comment("i1", "Done") + client = _client_with({"POST /api/issues/i1/comments": (201, raw)}) + comment = await client.add_comment("i1", "Done") assert comment is not None assert comment.content == "Done" -async def test_list_comments(client): +async def test_list_comments(): raw = [{"id": "c1", "issue_id": "i1", "content": "LGTM"}] - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - comments = await client.list_comments("i1") + client = _client_with({"GET /api/issues/i1/comments": (200, raw)}) + comments = await client.list_comments("i1") assert len(comments) == 1 -# ── goals ──────────────────────────────────────────────────────────────────── +# --------------------------------------------------------------------------- +# goals +# --------------------------------------------------------------------------- -async def test_list_goals(client): +async def test_list_goals(): raw = [{"id": "g1", "title": "Ship MVP"}] - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - goals = await client.list_goals(company_id="comp-1") + client = _client_with({"GET /api/companies/comp-1/goals": (200, raw)}) + goals = await client.list_goals(company_id="comp-1") assert len(goals) == 1 assert goals[0].title == "Ship MVP" -async def test_create_goal(client): +async def test_create_goal(): raw = {"id": "g2", "title": "Scale to 1000 users"} - with patch.object(client, "_post", new_callable=AsyncMock, return_value=raw): - goal = await client.create_goal("Scale to 1000 users", company_id="comp-1") + client = _client_with({"POST /api/companies/comp-1/goals": (201, raw)}) + goal = await client.create_goal("Scale to 1000 users", company_id="comp-1") assert goal is not None -# ── heartbeat runs ─────────────────────────────────────────────────────────── +# --------------------------------------------------------------------------- +# heartbeat runs +# --------------------------------------------------------------------------- -async def test_list_heartbeat_runs(client): +async def test_list_heartbeat_runs(): raw = [{"id": "r1", "agent_id": "a1", "status": "running"}] - with patch.object(client, "_get", new_callable=AsyncMock, return_value=raw): - runs = await client.list_heartbeat_runs(company_id="comp-1") + client = _client_with({"GET /api/companies/comp-1/heartbeat-runs": (200, raw)}) + runs = await client.list_heartbeat_runs(company_id="comp-1") assert len(runs) == 1 + + +async def test_list_heartbeat_runs_server_error(): + client = _client_with({"GET /api/companies/comp-1/heartbeat-runs": (500, None)}) + runs = await client.list_heartbeat_runs(company_id="comp-1") + assert runs == []