Add webhook integration tests covering retry logic and Slack/Discord payloads

22 test cases covering:
- Slack/Discord URL detection
- Generic vs Slack payload formatting
- Exponential backoff retry logic with network/timeout error handling
- Multi-URL dispatch with format auto-detection
- notify_job_completed() and notify_alert() helpers

Closes leeworks-agents/SPARC#1657

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ROADMAP.md

@@ -7,131 +7,86 @@ Semiconductor Patent & Analytics Report Core -- development priorities.
 SPARC is a patent analysis platform with a working end-to-end pipeline:
 Python/FastAPI backend, React/TypeScript frontend, PostgreSQL for persistence
 and caching, Docker Compose for local development, and Gitea Actions CI/CD for
-image builds and testing. Core features include patent retrieval via SerpAPI,
+image builds. Core features (patent retrieval via SerpAPI, PDF parsing, LLM
-PDF parsing, LLM analysis via OpenRouter (multi-model: Claude, GPT-4o, Gemini,
+analysis via OpenRouter/Claude, batch processing, JWT authentication, analytics
-Llama), batch processing, JWT authentication, analytics dashboard with patent
+dashboard) are all implemented and functional.
-trend charts, scheduled recurring analysis with alerting, webhook notifications
-(Slack/Discord), CSV and PDF export, S3/MinIO storage, side-by-side company
-comparison, and dark mode.
-
----
-
-## Completed
-
-Items that have been implemented and merged into main.
-
-### Security hardening
-
-- ~~Rotate default JWT secret.~~ Startup check refuses to start with the
-  default secret in non-development environments.
-- ~~CORS allow-origins are hardcoded.~~ Allowed origins are now configurable
-  via environment variable.
-- ~~Database credentials in docker-compose.yml.~~ Compose references `.env`
-  for sensitive values.
-
-### Error handling and resilience
-
-- ~~`get_db_client()` creates a new `DatabaseClient` on every call.~~ Refactored
-  to a shared pooled singleton initialized at startup.
-- ~~No rate limiting on auth endpoints.~~ Rate limiting middleware added to
-  `/auth/login` and `/auth/register`.
-
-### Test coverage
-
-- ~~API tests bypass authentication.~~ JWT auth integration tests added (33
-  cases covering registration, login, protected routes, token refresh, and
-  admin-only endpoints).
-- ~~No test stage in CI.~~ Gitea Actions workflow now runs `pytest` and gates
-  the build.
-- ~~No linting or type checking in CI.~~ `ruff` (Python) and `tsc --noEmit`
-  (TypeScript) added to CI pipeline.
-
-### Backend
-
-- ~~Add structured logging.~~ Python `logging` module used throughout.
-- ~~Make LLM model configurable.~~ `MODEL` environment variable accepted;
-  multi-model support with per-analysis selection (GPT-4o, Gemini, Claude,
-  Llama).
-- ~~SERP cache TTL hardcoded.~~ `SERP_CACHE_TTL_HOURS` exposed as env var.
-- ~~Patent PDF storage.~~ S3/MinIO object storage backend added alongside
-  local filesystem. Volume mount requirement documented.
-- ~~`analyze_single_patent` assumes local file.~~ Auto-download from cached
-  metadata link integrated.
-- ~~`Patent.patent_id` typed as `int`.~~ Fixed to `str`.
-
-### Frontend
-
-- ~~No loading/error states.~~ Skeleton loaders and error states added to
-  Batch and Analytics pages.
-- ~~No dark mode.~~ Full dark mode support with theme-aware chart colors.
-- ~~Missing lockfile.~~ `package-lock.json` committed.
-
-### Features (formerly P3)
-
-- ~~Export analysis reports.~~ CSV and PDF export endpoints implemented.
-- ~~Comparison view.~~ Side-by-side company patent portfolio comparison added.
-- ~~Scheduled/recurring analysis.~~ APScheduler-based periodic re-analysis
-  with configurable interval and change-threshold alerting.
-- ~~Webhook/notification support.~~ Slack, Discord, and generic HTTP POST
-  webhooks with retry logic.
-- ~~Multi-model support.~~ Model picker in Analysis and Batch pages; backend
-  allow-list validation.
-- ~~Patent trend charts.~~ Filing frequency and category distribution
-  visualizations added to Analytics page.
-- ~~OpenAPI client generation.~~ TypeScript API client auto-generated from
-  FastAPI spec with CI freshness check.
 
 ---
 
 ## P1 -- High Priority
 
-These items address correctness, reliability, and coverage gaps that should be
+These items address correctness, security, and reliability gaps that should be
 resolved before broader production use.
 
-### Resilience
+### Security hardening
 
-- **`_jobs` dict is in-memory only.** Job state is lost on API restart.
+- **Rotate default JWT secret.** `auth.py` ships a fallback
-  Persist job status in PostgreSQL or Redis so async batch results survive
+  `sparc-secret-key-change-in-production` that will be used if `JWT_SECRET` is
-  restarts.
+  unset. Add a startup check that refuses to start with the default secret in
+  non-development environments.
+- **CORS allow-origins are hardcoded.** `api.py` only permits
+  `localhost:3000` and `localhost:5173`. Make the allowed origins configurable
+  via environment variable so the dashboard works when deployed behind a real
+  domain.
+- **Database credentials in docker-compose.yml.** The compose file embeds
+  `postgres:postgres` in plain text. Reference a `.env` file or Docker secrets
+  instead.
 
-### Test coverage gaps
+### Error handling and resilience
 
-- **Export endpoint tests.** The CSV and PDF export endpoints (`/export/`)
+- **`get_db_client()` in `auth.py` creates a new `DatabaseClient` on every
-  lack test coverage. Add tests covering auth, success, 404, and edge cases.
+  call.** This bypasses the connection pool and can exhaust database
-  *(Issue #1655)*
+  connections under load. Refactor to share a single pooled client.
-- **Tracked company admin endpoint tests.** The `/admin/tracked` CRUD
+- **`_jobs` dict is in-memory only.** Job state is lost on API restart. Persist
-  endpoints and scheduler integration lack test coverage. *(Issue #1656)*
+  job status in PostgreSQL or Redis so async batch results survive restarts.
+- **No rate limiting on auth endpoints.** `/auth/login` and `/auth/register`
+  are unprotected against brute-force or abuse. Add rate limiting middleware.
+
+### Test coverage for auth and admin
+
+- The existing API tests (`tests/test_api.py`) bypass authentication entirely.
+  Add tests that exercise the JWT flow: registration, login, protected-route
+  access, token refresh, and admin-only endpoints.
 
 ---
 
 ## P2 -- Medium Priority
 
-Improvements to reliability, test coverage, and code quality.
+Improvements to usability, performance, and developer experience.
 
-### Test coverage
+### Backend
 
-- **Webhook integration tests.** The retry logic, Slack/Discord payload
+- **Add structured logging.** Replace `print()` calls throughout `analyzer.py`,
-  format, and multi-URL dispatch in `webhooks.py` need test coverage.
+  `serp_api.py`, and `llm.py` with Python `logging` so log levels and
-  *(Issue #1657)*
+  formatting are consistent.
-- **S3/MinIO storage backend tests.** `storage.py` has local filesystem tests
+- **Make LLM model configurable.** `llm.py` hardcodes
-  but no unit tests for the S3 backend (read, write, exists, delete,
+  `anthropic/claude-3.5-sonnet`. Accept a `MODEL` environment variable to allow
-  error handling). *(Issue #1660)*
+  switching models without code changes.
-- **`analyze_single_patent` auto-download path tests.** The auto-download
+- **SERP cache TTL is hardcoded to 24 hours.** Expose `SERP_CACHE_TTL_HOURS`
-  fallback (cache lookup, PDF download, FileNotFoundError) in
+  as an environment variable in `config.py`.
-  `analyzer.py` lacks test coverage. *(Issue #1661)*
+- **Patent PDF storage.** PDFs are saved to a local `patents/` directory. For
+  containerized deployments, consider object storage (S3/MinIO) or at minimum
+  document the volume mount requirement more prominently.
+- **`analyze_single_patent` assumes local file path.** The method constructs
+  `patents/{patent_id}.pdf` and reads from disk, but does not download the PDF
+  first. Either integrate the download step or document the prerequisite.
+- **`Patent.patent_id` typed as `int` in `types.py` but used as `str`
+  everywhere.** Fix the type annotation to `str`.
 
-### Code quality
+### Frontend
 
-- **Scheduler creates its own DatabaseClient.** `scheduler.py` bypasses the
+- **No loading/error states on several pages.** The Batch and Analytics pages
-  application-level pooled client, creating a new connection on every tick.
+  would benefit from skeleton loaders and user-friendly error messages.
-  Refactor to use `get_db_client()`. *(Issue #1658)*
+- **No dark mode.** Tailwind is configured but no dark variant is applied.
+- **Missing `package-lock.json` or `pnpm-lock.yaml`.** The frontend has no
+  lockfile committed, leading to non-reproducible builds.
 
-### API improvements
+### CI/CD
 
-- **API pagination.** The `/analyze/batch` and `/jobs` endpoints could benefit
+- **No test stage in the Gitea Actions workflow.** `build.yaml` builds and
-  from cursor-based pagination for large result sets.
+  pushes images but never runs `pytest`. Add a test job that gates the build.
-- **Request validation improvements.** Add stricter input validation for
+- **No linting or type checking.** Add `ruff` (Python) and `tsc --noEmit`
-  company names (disallow special characters, enforce length limits).
+  (TypeScript) to CI.
 
 ---
 
@@ -139,20 +94,23 @@ Improvements to reliability, test coverage, and code quality.
 
 Lower-urgency enhancements and future features.
 
-- **Historical analysis diffing.** Show what changed between two analysis runs
+- **Export analysis reports.** Allow users to download analysis results as PDF
-  for the same company, highlighting new patents and score shifts.
+  or CSV from the dashboard.
-- **Patent classification tagging.** Automatically tag patents by technology
+- **Comparison view.** Side-by-side comparison of two companies' patent
-  domain (AI, semiconductors, materials science) using LLM classification.
+  portfolios.
-- **User-level API keys.** Allow users to generate personal API keys for
+- **Scheduled/recurring analysis.** Periodically re-analyze tracked companies
-  programmatic access without JWT token refresh.
+  and alert on significant changes.
-- **Batch export.** Export analysis results for multiple companies at once as
+- **Webhook/notification support.** Send alerts (Slack, Discord, email) when
-  a ZIP archive.
+  batch jobs complete or when a company's innovation score changes
-- **Rate limiting dashboard.** Surface rate limit status and usage statistics
+  significantly.
-  in the admin panel.
+- **Multi-model support.** Let users choose between LLM providers per analysis
-- **Async webhook delivery.** Move webhook delivery to a background task queue
+  (e.g., GPT-4o, Gemini, Claude) and compare outputs.
-  (e.g., Celery, arq) to avoid blocking the scheduler.
+- **Patent trend charts.** Visualize patent filing frequency and technology
-- **Multi-tenant support.** Scope analysis results and tracked companies per
+  category distribution over time in the Analytics page.
-  user or organization.
+- **API pagination.** The `/analyze/batch` and `/jobs` endpoints could benefit
+  from cursor-based pagination for large result sets.
+- **OpenAPI client generation.** Auto-generate the TypeScript API client from
+  the FastAPI OpenAPI spec to keep frontend types in sync.
 
 ---
 

tests/test_webhooks.py

@@ -0,0 +1,280 @@
+"""Tests for webhook notification system: retry logic and Slack/Discord payload format.
+
+Covers issue #1657:
+- Retry logic with exponential backoff in _send_with_retry
+- Slack/Discord payload formatting in _build_payload
+- Generic HTTP POST payload formatting
+- notify() dispatching to multiple URLs
+- notify_job_completed() and notify_alert() convenience helpers
+"""
+
+from datetime import datetime
+from unittest.mock import MagicMock, patch, call
+
+import pytest
+import requests
+
+from SPARC.webhooks import (
+    MAX_RETRIES,
+    _build_payload,
+    _is_slack_url,
+    _send_with_retry,
+    notify,
+    notify_alert,
+    notify_job_completed,
+)
+
+
+class TestIsSlackUrl:
+    """Tests for Slack/Discord URL detection."""
+
+    def test_slack_webhook_url(self):
+        assert _is_slack_url("https://hooks.slack.com/services/T00/B00/xxx") is True
+
+    def test_discord_webhook_url(self):
+        assert _is_slack_url("https://discord.com/api/webhooks/123/abc") is True
+
+    def test_generic_url(self):
+        assert _is_slack_url("https://example.com/webhook") is False
+
+    def test_empty_url(self):
+        assert _is_slack_url("") is False
+
+
+class TestBuildPayload:
+    """Tests for payload construction."""
+
+    def test_generic_payload_structure(self):
+        """Generic payload includes event type, timestamp, and data."""
+        payload = _build_payload("job_completed", {"job_id": "abc123"})
+
+        assert payload["event"] == "job_completed"
+        assert payload["job_id"] == "abc123"
+        assert "timestamp" in payload
+        # Timestamp should be ISO format ending with Z
+        assert payload["timestamp"].endswith("Z")
+
+    def test_slack_payload_wraps_in_text(self):
+        """Slack payload wraps content in a 'text' field."""
+        payload = _build_payload("patent_alert", {"company_name": "NVIDIA"}, slack=True)
+
+        assert "text" in payload
+        assert "patent_alert" in payload["text"]
+        assert "NVIDIA" in payload["text"]
+        # Slack payload should NOT have the event/timestamp at top level
+        assert "event" not in payload
+        assert "timestamp" not in payload
+
+    def test_generic_payload_does_not_have_text_field(self):
+        """Non-Slack payload does not wrap in text."""
+        payload = _build_payload("job_completed", {"status": "done"})
+
+        assert "text" not in payload
+        assert payload["status"] == "done"
+
+    def test_slack_payload_contains_bold_header(self):
+        """Slack payload starts with bold event header using Slack markdown."""
+        payload = _build_payload("job_completed", {"count": 5}, slack=True)
+
+        assert payload["text"].startswith("*[SPARC] job_completed*")
+
+    def test_payload_merges_all_data_keys(self):
+        """All data keys are included in the generic payload."""
+        data = {"key1": "val1", "key2": 42, "key3": True}
+        payload = _build_payload("test_event", data)
+
+        assert payload["key1"] == "val1"
+        assert payload["key2"] == 42
+        assert payload["key3"] is True
+
+
+class TestSendWithRetry:
+    """Tests for retry logic in _send_with_retry."""
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_success_on_first_attempt(self, mock_post, mock_sleep):
+        """Successful delivery on first attempt, no retries."""
+        mock_post.return_value = MagicMock(status_code=200)
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is True
+        mock_post.assert_called_once()
+        mock_sleep.assert_not_called()
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_success_on_second_attempt(self, mock_post, mock_sleep):
+        """Fails first, succeeds on retry."""
+        mock_post.side_effect = [
+            MagicMock(status_code=500),
+            MagicMock(status_code=200),
+        ]
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is True
+        assert mock_post.call_count == 2
+        mock_sleep.assert_called_once()
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_all_retries_exhausted(self, mock_post, mock_sleep):
+        """Returns False after all retries fail."""
+        mock_post.return_value = MagicMock(status_code=500)
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is False
+        assert mock_post.call_count == MAX_RETRIES
+        assert mock_sleep.call_count == MAX_RETRIES - 1
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_exponential_backoff_timing(self, mock_post, mock_sleep):
+        """Backoff wait times follow exponential pattern (2^attempt)."""
+        mock_post.return_value = MagicMock(status_code=500)
+
+        _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        # With BACKOFF_BASE=2: attempt 1 -> sleep(2), attempt 2 -> sleep(4)
+        expected_waits = [call(2 ** i) for i in range(1, MAX_RETRIES)]
+        assert mock_sleep.call_args_list == expected_waits
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_network_error_triggers_retry(self, mock_post, mock_sleep):
+        """Network exceptions trigger retry, not immediate failure."""
+        mock_post.side_effect = [
+            requests.ConnectionError("Connection refused"),
+            MagicMock(status_code=200),
+        ]
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is True
+        assert mock_post.call_count == 2
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_timeout_error_triggers_retry(self, mock_post, mock_sleep):
+        """Timeout exceptions trigger retry."""
+        mock_post.side_effect = [
+            requests.Timeout("Request timed out"),
+            MagicMock(status_code=200),
+        ]
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is True
+        assert mock_post.call_count == 2
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_2xx_status_codes_accepted(self, mock_post, mock_sleep):
+        """Any 2xx status code is treated as success."""
+        mock_post.return_value = MagicMock(status_code=204)
+
+        result = _send_with_retry("https://example.com/hook", {"event": "test"})
+
+        assert result is True
+        mock_post.assert_called_once()
+
+    @patch("SPARC.webhooks.time.sleep")
+    @patch("SPARC.webhooks.requests.post")
+    def test_posts_json_payload(self, mock_post, mock_sleep):
+        """Payload is sent as JSON with correct timeout."""
+        mock_post.return_value = MagicMock(status_code=200)
+        payload = {"event": "test", "data": "value"}
+
+        _send_with_retry("https://example.com/hook", payload)
+
+        mock_post.assert_called_once_with(
+            "https://example.com/hook", json=payload, timeout=10
+        )
+
+
+class TestNotify:
+    """Tests for the notify() dispatcher."""
+
+    @patch("SPARC.webhooks._send_with_retry")
+    @patch("SPARC.webhooks.WEBHOOK_URLS", ["https://example.com/hook1", "https://example.com/hook2"])
+    def test_dispatches_to_all_urls(self, mock_send):
+        """notify() sends to every configured webhook URL."""
+        mock_send.return_value = True
+
+        notify("job_completed", {"job_id": "test123"})
+
+        assert mock_send.call_count == 2
+
+    @patch("SPARC.webhooks._send_with_retry")
+    @patch("SPARC.webhooks.WEBHOOK_URLS", [])
+    def test_no_urls_configured_returns_immediately(self, mock_send):
+        """No-op when no webhook URLs are configured."""
+        notify("job_completed", {"job_id": "test123"})
+
+        mock_send.assert_not_called()
+
+    @patch("SPARC.webhooks._send_with_retry")
+    @patch("SPARC.webhooks.WEBHOOK_URLS", [
+        "https://hooks.slack.com/services/T00/B00/xxx",
+        "https://example.com/generic",
+    ])
+    def test_slack_url_gets_slack_payload(self, mock_send):
+        """Slack URLs receive Slack-formatted payloads, others get generic."""
+        mock_send.return_value = True
+
+        notify("test_event", {"key": "val"})
+
+        # First call (Slack URL) should have "text" key
+        slack_payload = mock_send.call_args_list[0][0][1]
+        assert "text" in slack_payload
+
+        # Second call (generic URL) should have "event" key
+        generic_payload = mock_send.call_args_list[1][0][1]
+        assert "event" in generic_payload
+        assert generic_payload["event"] == "test_event"
+
+
+class TestNotifyJobCompleted:
+    """Tests for notify_job_completed() convenience function."""
+
+    @patch("SPARC.webhooks.notify")
+    def test_sends_correct_event_and_data(self, mock_notify):
+        """Job completion sends proper event type and summary."""
+        notify_job_completed(
+            job_id="batch-001",
+            status="completed",
+            total_companies=10,
+            successful=8,
+            failed=2,
+        )
+
+        mock_notify.assert_called_once()
+        event, data = mock_notify.call_args[0]
+        assert event == "job_completed"
+        assert data["job_id"] == "batch-001"
+        assert data["successful"] == 8
+        assert data["failed"] == 2
+        assert "8/10" in data["summary"]
+
+
+class TestNotifyAlert:
+    """Tests for notify_alert() convenience function."""
+
+    @patch("SPARC.webhooks.notify")
+    def test_sends_correct_event_and_data(self, mock_notify):
+        """Alert notification sends patent_alert event type."""
+        notify_alert(
+            company_name="NVIDIA",
+            alert_type="patent_count_change",
+            message="Patent count increased by 30%",
+        )
+
+        mock_notify.assert_called_once()
+        event, data = mock_notify.call_args[0]
+        assert event == "patent_alert"
+        assert data["company_name"] == "NVIDIA"
+        assert data["alert_type"] == "patent_count_change"
+        assert "30%" in data["message"]