diff --git a/CLAUDE.md b/CLAUDE.md index 16bd5e82..1b10106d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,7 +36,9 @@ If you are an agent working in this repo: **do not improvise architecture**. Fol ### Current Focus: Phase 4A -**Phase 5.3 (browser + in-app center) is complete** — Async notifications: `useNotifications` hook with workspace-scoped `localStorage` persistence and browser Notification dispatch (only when tab hidden + permission granted); `NotificationProvider` in root layout; `NotificationCenter` (bell icon + dropdown) mounts in sidebar footer. `BatchExecutionMonitor` dispatches `batch.completed` on terminal status transitions (distinguishing COMPLETED/FAILED/CANCELLED in both the in-app message and the success icon) and `blocker.created` on per-task BLOCKED transitions. `/execution` requests browser permission once on mount when permission is `'default'`. `/proof` dispatches `gate.run.failed` per failed gate when a proof run completes with `passed === false`. Known limitation: notifications only fire while `BatchExecutionMonitor` is mounted (cross-page background poller is out of scope; tracked for future work). Webhook (#560) deferred. Issue #559. +**Phase 5.3 is complete** — Async notifications cover both surfaces: +- **Browser + in-app center (#559)**: `useNotifications` hook with workspace-scoped `localStorage` persistence and browser Notification dispatch (only when tab hidden + permission granted); `NotificationProvider` in root layout; `NotificationCenter` (bell icon + dropdown) mounts in sidebar footer. `BatchExecutionMonitor` dispatches `batch.completed` on terminal status transitions (distinguishing COMPLETED/FAILED/CANCELLED in both the in-app message and the success icon) and `blocker.created` on per-task BLOCKED transitions. `/execution` requests browser permission once on mount when permission is `'default'`. `/proof` dispatches `gate.run.failed` per failed gate when a proof run completes with `passed === false`. Known limitation: notifications only fire while `BatchExecutionMonitor` is mounted (cross-page background poller is out of scope; tracked for future work). +- **Outbound webhook (#560)**: Settings → Notifications tab takes a single URL + enabled toggle, persisted to `.codeframe/notifications_config.json` via `atomic_write_json`. `GET/PUT /api/v2/settings/notifications` and `POST /api/v2/settings/notifications/test` (test fires a sample payload and surfaces status code). `WebhookNotificationService.send_event` is the generic backend; dispatched fire-and-forget (5s timeout) from `core/conductor.py` on `BATCH_COMPLETED` only (not PARTIAL/FAILED/CANCELLED), `core/blockers.py:create()` after `BLOCKER_CREATED`, and `ui/routers/pr_v2.py:merge_pull_request` after successful merge. Failures are logged but never break the triggering operation. **Phase 5.2 is complete** — Costs page now ships per-task and per-agent breakdowns (#558) on top of the spend summary (#557). Backend: `GET /api/v2/costs/tasks?days=N&limit=M` (top-N tasks with titles, agent, tokens, cost) and `GET /api/v2/costs/by-agent?days=N` (per-agent rollup + total input/output tokens), both via `TokenRepository.get_top_tasks_by_cost` and `get_costs_by_agent`. Task board cards show an inline `MoneyBag02Icon` cost badge with token-breakdown tooltip when cost data exists. Fixed a v2 data-loss bug where `react_agent` int-cast UUID task IDs and stored NULL in `token_usage`. diff --git a/codeframe/core/blockers.py b/codeframe/core/blockers.py index f00bdb63..cdce18af 100644 --- a/codeframe/core/blockers.py +++ b/codeframe/core/blockers.py @@ -119,9 +119,38 @@ def create( print_event=True, ) + # Outbound webhook dispatch (issue #560) — fire-and-forget, never blocks + # blocker creation. A misconfigured webhook must not break the workflow. + _dispatch_blocker_webhook(workspace, blocker_id, task_id) + return blocker +def _dispatch_blocker_webhook( + workspace: Workspace, blocker_id: str, task_id: Optional[str] +) -> None: + """Best-effort outbound webhook for ``blocker.created``.""" + try: + from codeframe.core.notifications_config import is_webhook_active + from codeframe.notifications.webhook import ( + WebhookNotificationService, + format_blocker_payload, + ) + + url = is_webhook_active(workspace) + if url is None: + return + svc = WebhookNotificationService(webhook_url=url, timeout=5) + svc.send_event_background(format_blocker_payload(blocker_id, task_id)) + except Exception: + import logging + + logging.getLogger(__name__).warning( + "Failed to dispatch blocker.created webhook for %s", blocker_id, + exc_info=True, + ) + + def get(workspace: Workspace, blocker_id: str) -> Optional[Blocker]: """Get a blocker by ID. diff --git a/codeframe/core/conductor.py b/codeframe/core/conductor.py index 6ee07823..85b1dddf 100644 --- a/codeframe/core/conductor.py +++ b/codeframe/core/conductor.py @@ -33,6 +33,41 @@ logger = logging.getLogger(__name__) + +def _dispatch_batch_completed_webhook( + workspace: Workspace, + event_type: "events.EventType", + batch_id: str, + task_count: int, +) -> None: + """Best-effort outbound webhook for ``batch.completed`` (issue #560). + + Only fires when the batch reached the full COMPLETED state — PARTIAL, + FAILED, and CANCELLED are intentionally out of scope for v1. Failures + never break the batch. + """ + if event_type != events.EventType.BATCH_COMPLETED: + return + try: + from codeframe.core.notifications_config import is_webhook_active + from codeframe.notifications.webhook import ( + WebhookNotificationService, + format_batch_payload, + ) + + url = is_webhook_active(workspace) + if url is None: + return + svc = WebhookNotificationService(webhook_url=url, timeout=5) + svc.send_event_background(format_batch_payload(batch_id, task_count)) + except Exception: + logger.warning( + "Failed to dispatch batch.completed webhook for %s", + batch_id, + exc_info=True, + ) + + # Tactical patterns that the supervisor should auto-resolve SUPERVISOR_TACTICAL_PATTERNS = [ # Virtual environment / package management @@ -1125,6 +1160,7 @@ def _execute_serial_resume( }, print_event=True, ) + _dispatch_batch_completed_webhook(workspace, event_type, batch.id, final_completed) # Print summary print(f"\nBatch resume {batch.status.value.lower()}: {final_completed}/{total} tasks completed") @@ -1282,6 +1318,7 @@ def _execute_retries( }, print_event=True, ) + _dispatch_batch_completed_webhook(workspace, event_type, batch.id, final_completed) # Print retry summary if final_failed == 0: @@ -1544,6 +1581,7 @@ def _execute_serial( }, print_event=True, ) + _dispatch_batch_completed_webhook(workspace, event_type, batch.id, completed_count) # Stop reconciliation thread reconcile_stop.set() @@ -1733,6 +1771,7 @@ def _execute_parallel( }, print_event=True, ) + _dispatch_batch_completed_webhook(workspace, event_type, batch.id, completed_count) # Stop reconciliation thread _reconcile_stop_p.set() diff --git a/codeframe/core/notifications_config.py b/codeframe/core/notifications_config.py new file mode 100644 index 00000000..cd7a717e --- /dev/null +++ b/codeframe/core/notifications_config.py @@ -0,0 +1,183 @@ +"""Per-workspace outbound webhook notification config (issue #560). + +Stored alongside other workspace UI settings under ``.codeframe/`` as a JSON +file. Headless — no FastAPI or HTTP imports. + +Schema (``.codeframe/notifications_config.json``): + + { + "webhook_url": "https://hooks.example.com/...", // or null + "webhook_enabled": true + } + +Defense-in-depth: ``is_webhook_active`` re-validates the URL scheme/host +because the JSON file can be hand-edited or migrated from an older +deployment that didn't enforce validation at write time. The router PUT +endpoint validates too — never trust just one layer. +""" + +from __future__ import annotations + +import json +import logging +import os +import tempfile +from pathlib import Path +from typing import Optional, TypedDict + +from codeframe.core.workspace import Workspace + +logger = logging.getLogger(__name__) + +NOTIFICATIONS_CONFIG_FILENAME = "notifications_config.json" + +# Intentionally duplicated with codeframe/ui/routers/settings_v2.py's +# ``_ALLOWED_WEBHOOK_SCHEMES``: core cannot import from the UI layer +# (architecture rule #1 — core must be headless). Keep both values in +# sync if extended. +_ALLOWED_SCHEMES = frozenset({"http", "https"}) + + +class NotificationsConfig(TypedDict): + webhook_url: Optional[str] + webhook_enabled: bool + + +_DEFAULT: NotificationsConfig = {"webhook_url": None, "webhook_enabled": False} + + +def _config_path(workspace: Workspace) -> Path: + return workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + + +def _is_safe_webhook_url(url: str) -> bool: + """Defence-in-depth: scheme/host check on a stored URL before we POST. + + The router PUT validates too, but a hand-edited or pre-migration JSON + file could carry a ``file://`` or schemeless URL. Used by + ``is_webhook_active`` to fail-safe to ``None``. + + TODO: This does NOT block RFC-1918 (10/8, 172.16/12, 192.168/16) or + loopback (127/8) addresses. For a self-hosted single-user tool that is + intentional — users want to point at local receivers. If CodeFRAME ever + runs as a shared / multi-tenant service, add a socket-level check here + that resolves the host and rejects private/loopback ranges. + """ + from urllib.parse import urlparse + + try: + parsed = urlparse(url) + except (TypeError, ValueError): + return False + return parsed.scheme.lower() in _ALLOWED_SCHEMES and bool(parsed.netloc) + + +def load_notifications_config(workspace: Workspace) -> NotificationsConfig: + """Read notifications config, returning defaults on missing/corrupt file. + + Never raises — a broken config should not break the triggering operation. + Non-object JSON (``[]``, ``null``, a bare integer) is treated as corrupt + and falls back to defaults rather than raising ``AttributeError`` from + a downstream ``.get()`` call. + """ + path = _config_path(workspace) + if not path.exists(): + return dict(_DEFAULT) # type: ignore[return-value] + try: + data = json.loads(path.read_text()) + if not isinstance(data, dict): + raise ValueError( + f"Expected JSON object, got {type(data).__name__}" + ) + return { + "webhook_url": data.get("webhook_url") or None, + "webhook_enabled": bool(data.get("webhook_enabled", False)), + } + except (OSError, json.JSONDecodeError, ValueError) as e: + logger.warning( + "Invalid notifications_config.json — falling back to defaults: %s", e + ) + return dict(_DEFAULT) # type: ignore[return-value] + + +def save_notifications_config( + workspace: Workspace, config: NotificationsConfig +) -> None: + """Atomically persist notifications config to disk.""" + path = _config_path(workspace) + payload = { + "webhook_url": config.get("webhook_url") or None, + "webhook_enabled": bool(config.get("webhook_enabled", False)), + } + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp( + prefix=f".{path.name}.", suffix=".tmp", dir=path.parent + ) + try: + with os.fdopen(fd, "w") as f: + f.write(json.dumps(payload, indent=2)) + os.replace(tmp_name, path) + except Exception: + try: + os.unlink(tmp_name) + except OSError: + pass + raise + + +def is_webhook_active(workspace: Workspace) -> Optional[str]: + """Return the webhook URL if URL is set, enabled flag is on, AND the URL + passes basic safety checks (``http(s)`` scheme + non-empty host). + + Returns ``None`` otherwise — callers should short-circuit on ``None`` to + avoid instantiating the webhook service for nothing. The safety check is + intentionally redundant with the PUT-endpoint validation: it protects + against hand-edited config files and pre-migration data. + """ + cfg = load_notifications_config(workspace) + url = (cfg["webhook_url"] or "").strip() + if not url or not cfg["webhook_enabled"]: + return None + if not _is_safe_webhook_url(url): + logger.warning( + "Refusing to dispatch webhook to unsafe URL: %s", + _redact_url_for_log(url), + ) + return None + return url + + +def _redact_url_for_log(url: str) -> str: + """Return a logging-safe representation of a webhook URL. + + Slack/Discord/GitHub-style webhook URLs commonly embed secrets in: + + * the path or query (Slack's ``T*/B*/...`` token, signed Zapier hooks) + * basic-auth credentials in ``user:password@host`` form + + ``parsed.netloc`` preserves the userinfo segment, so we use + ``parsed.hostname`` (which strips auth and port) and re-attach the + port explicitly. Returns ``scheme://host[:port]`` when parsable, else + ``""``. + """ + from urllib.parse import urlparse + + try: + parsed = urlparse(url) + except (TypeError, ValueError): + return "" + if not parsed.scheme or not parsed.hostname: + return "" + host = parsed.hostname + # ``parsed.port`` raises ValueError for malformed ports (e.g. + # ``file://host:abc/x``). Without this guard the "fail-safe" branch + # in ``is_webhook_active`` could end up raising instead of returning + # None, which would bubble through every dispatch site's broad + # ``except Exception``. + try: + port = parsed.port + except ValueError: + return "" + if port is not None: + host = f"{host}:{port}" + return f"{parsed.scheme}://{host}" diff --git a/codeframe/notifications/webhook.py b/codeframe/notifications/webhook.py index c0ba639f..166d9e7d 100644 --- a/codeframe/notifications/webhook.py +++ b/codeframe/notifications/webhook.py @@ -1,12 +1,18 @@ -"""Webhook notification service for blocker alerts. +"""Webhook notification service. -This module provides async webhook notification capabilities for SYNC blockers, -enabling external integrations like Zapier, Slack, email, etc. +Originally built for SYNC blocker alerts (049-human-in-loop, Phase 7), now also +powers outbound event webhooks for batch completion / blocker creation / PR +merge (issue #560). + +Delivery is fire-and-forget with a configurable timeout — failures are logged +but never break the triggering operation. """ import asyncio import logging -from datetime import datetime +import threading +from dataclasses import dataclass +from datetime import datetime, timezone from typing import Optional import aiohttp @@ -16,11 +22,77 @@ logger = logging.getLogger(__name__) +@dataclass +class WebhookSendResult: + """Result of a single ``send_event`` call. + + ``ok`` mirrors HTTP 2xx semantics. ``status_code`` is ``None`` when the + request never completed (timeout, DNS failure, connection refused, …) — + the ``error`` field carries a short human-readable reason in that case. + """ + + ok: bool + status_code: Optional[int] + error: Optional[str] = None + + +def _utc_iso_now() -> str: + """ISO-8601 UTC timestamp for webhook payloads (``Z`` suffix). + + Slack/Discord/Zapier-style consumers expect ``Z``, not ``+00:00``. + Drops sub-second precision for the same reason — consumers vary widely + in fractional-second handling. + """ + return datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + +def format_batch_payload(batch_id: str, task_count: int) -> dict: + # ``status`` is always "completed" today (the dispatcher gates on + # BATCH_COMPLETED only) but is included so consumers can self-document + # and so a future PARTIAL/FAILED extension is forward-compatible. + return { + "event": "batch.completed", + "batch_id": batch_id, + "task_count": task_count, + "status": "completed", + "timestamp": _utc_iso_now(), + } + + +def format_blocker_payload(blocker_id: str, task_id: Optional[str]) -> dict: + return { + "event": "blocker.created", + "blocker_id": blocker_id, + "task_id": task_id, + "timestamp": _utc_iso_now(), + } + + +def format_pr_payload(pr_number: int, pr_url: Optional[str] = None) -> dict: + # ``pr_number`` is always present so consumers can branch on it. + # ``pr_url`` is the canonical github.com URL when GITHUB_REPO is set, + # ``None`` otherwise (an unparseable sentinel like ``"pr#42"`` was + # actively confusing for downstream handlers). + return { + "event": "pr.merged", + "pr_number": pr_number, + "pr_url": pr_url, + "timestamp": _utc_iso_now(), + } + + +def format_test_payload() -> dict: + return {"event": "test", "timestamp": _utc_iso_now()} + + class WebhookNotificationService: - """Async webhook notification service for blocker alerts. + """Async outbound webhook service. - Sends HTTP POST notifications for SYNC blockers to configured webhook endpoints. - Uses fire-and-forget delivery with timeout to prevent blocking agent execution. + Originally built for SYNC blocker alerts (``send_blocker_notification``); + now also powers the issue #560 outbound event webhooks (batch / blocker / + PR / test) via ``send_event``. Delivery is fire-and-forget with a + configurable timeout — failures are logged but never break the triggering + operation. """ def __init__( @@ -160,6 +232,119 @@ async def send_blocker_notification( ) return False + async def send_event( + self, payload: dict, url: Optional[str] = None + ) -> WebhookSendResult: + """Generic webhook POST for outbound event notifications (issue #560). + + Unlike ``send_blocker_notification``, this method: + + * Accepts an arbitrary JSON payload (the caller composes the event). + * Returns rich status information so the Settings ``Test`` endpoint + can surface the HTTP status code or error to the user. + * Accepts an optional ``url`` override so the same service instance + can dispatch to a freshly-configured URL without rebuilding state. + + Failures (timeout, network, non-2xx) are logged but never raised — + the caller can react via the returned ``WebhookSendResult``. + """ + target_url = url or self.webhook_url + if not target_url or not target_url.strip(): + return WebhookSendResult( + ok=False, status_code=None, error="No webhook URL configured" + ) + + try: + async with aiohttp.ClientSession() as session: + async with session.post( + target_url, + json=payload, + timeout=aiohttp.ClientTimeout(total=self.timeout), + ) as response: + ok = 200 <= response.status < 300 + if not ok: + logger.warning( + "Webhook returned non-2xx status %s for event %s", + response.status, + payload.get("event"), + ) + return WebhookSendResult(ok=ok, status_code=response.status) + except asyncio.TimeoutError: + logger.error( + "Webhook timeout for event %s (exceeded %ss)", + payload.get("event"), + self.timeout, + ) + return WebhookSendResult( + ok=False, status_code=None, error=f"Timeout after {self.timeout}s" + ) + except aiohttp.ClientError as e: + logger.error("Webhook ClientError for event %s: %s", payload.get("event"), e) + return WebhookSendResult(ok=False, status_code=None, error=str(e)) + except Exception as e: + logger.error( + "Unexpected webhook error for event %s: %s", + payload.get("event"), + e, + exc_info=True, + ) + return WebhookSendResult(ok=False, status_code=None, error=str(e)) + + def send_event_background(self, payload: dict, url: Optional[str] = None) -> None: + """Fire-and-forget wrapper for ``send_event``. + + Works in both contexts: + + * **Async** (FastAPI request handler): schedules the send on the + current event loop via ``loop.create_task`` and returns + immediately. + * **Sync** (CLI batch run, sync test): spawns a daemon thread that + runs the send in a fresh event loop. The thread is daemon so it + never blocks process exit; ``timeout`` still applies inside the + loop, so the thread lives at most ``self.timeout`` seconds. + + Either way, the triggering operation never blocks on webhook + delivery and never sees an exception from this method. + """ + try: + loop = asyncio.get_running_loop() + except RuntimeError: + # No running loop — we're in sync context (CLI). Run the send + # in a daemon thread so we don't block the caller and so the + # process can exit cleanly even if the webhook hangs. + thread = threading.Thread( + target=self._run_send_event_sync, + args=(payload, url), + daemon=True, + name="webhook-send-event", + ) + thread.start() + return + task = loop.create_task(self.send_event(payload, url=url)) + # ``send_event`` already swallows all exceptions, but Python 3.11+ + # warns ``Task exception was never retrieved`` if a task ends with + # an unhandled exception and nobody awaited / called .exception(). + # Add a no-op callback so the result is always consumed. + task.add_done_callback( + lambda t: t.exception() if not t.cancelled() else None + ) + + def _run_send_event_sync(self, payload: dict, url: Optional[str]) -> None: + """Run ``send_event`` to completion in a fresh event loop. + + Used only by the sync branch of ``send_event_background`` — never + raises into the calling thread (the daemon thread is meant to die + quietly). + """ + try: + asyncio.run(self.send_event(payload, url=url)) + except Exception: + logger.warning( + "Sync webhook dispatch failed for event %s", + payload.get("event"), + exc_info=True, + ) + def send_blocker_notification_background( self, blocker_id: int, diff --git a/codeframe/ui/routers/pr_v2.py b/codeframe/ui/routers/pr_v2.py index 249c6b6c..3b6d156a 100644 --- a/codeframe/ui/routers/pr_v2.py +++ b/codeframe/ui/routers/pr_v2.py @@ -177,6 +177,43 @@ def _get_github_client() -> GitHubIntegration: ) +def _dispatch_pr_merged_webhook(workspace: Workspace, pr_number: int) -> None: + """Best-effort outbound webhook for ``pr.merged`` (issue #560). + + Builds the canonical ``https://github.com/{owner}/{repo}/pull/{N}`` URL + from the GitHub integration when available; sets ``pr_url`` to ``None`` + when the integration can't be constructed (e.g., ``GITHUB_REPO`` unset). + The always-present ``pr_number`` field lets consumers branch without + parsing a URL. + """ + try: + from codeframe.core.notifications_config import is_webhook_active + from codeframe.notifications.webhook import ( + WebhookNotificationService, + format_pr_payload, + ) + + url = is_webhook_active(workspace) + if url is None: + return + + # Canonical github.com URL when GITHUB_REPO is configured, ``None`` + # otherwise. The payload always carries ``pr_number`` so consumers + # can branch on it without parsing the URL. + try: + gh = GitHubIntegration() + pr_url: Optional[str] = f"https://github.com/{gh.repo}/pull/{pr_number}" + except Exception: + pr_url = None + + svc = WebhookNotificationService(webhook_url=url, timeout=5) + svc.send_event_background(format_pr_payload(pr_number, pr_url)) + except Exception: + logger.warning( + "Failed to dispatch pr.merged webhook for PR #%s", pr_number, exc_info=True + ) + + # ============================================================================ # Endpoints # ============================================================================ @@ -587,6 +624,11 @@ async def merge_pull_request( try: result = await client.merge_pull_request(pr_number, method=method) + # Outbound webhook for pr.merged (issue #560) — only on successful + # merge, never raises into the caller. + if result.merged: + _dispatch_pr_merged_webhook(workspace, pr_number) + return MergeResponse( sha=result.sha, merged=result.merged, diff --git a/codeframe/ui/routers/settings_v2.py b/codeframe/ui/routers/settings_v2.py index 8286a785..c0b8350d 100644 --- a/codeframe/ui/routers/settings_v2.py +++ b/codeframe/ui/routers/settings_v2.py @@ -1,20 +1,24 @@ -"""V2 Settings router — agent settings + API key management. +"""V2 Settings router — agent settings + API key management + notifications. Routes: - GET /api/v2/settings - Load agent settings (defaults if missing) - PUT /api/v2/settings - Save agent settings (merge into config) - GET /api/v2/settings/keys - List API key status for all providers - PUT /api/v2/settings/keys/{p} - Store an API key for provider p - DELETE /api/v2/settings/keys/{p} - Delete an API key for provider p - POST /api/v2/settings/verify-key - Live-verify a key against its provider + GET /api/v2/settings - Load agent settings (defaults if missing) + PUT /api/v2/settings - Save agent settings (merge into config) + GET /api/v2/settings/keys - List API key status for all providers + PUT /api/v2/settings/keys/{p} - Store an API key for provider p + DELETE /api/v2/settings/keys/{p} - Delete an API key for provider p + POST /api/v2/settings/verify-key - Live-verify a key against its provider + GET /api/v2/settings/notifications - Load outbound webhook config + PUT /api/v2/settings/notifications - Save outbound webhook config + POST /api/v2/settings/notifications/test - Fire a test payload and return HTTP status Key management is machine-wide (CredentialManager / keyring) and does not -require a workspace. Env vars take precedence at read time. +require a workspace. Env vars take precedence at read time. Notifications +config is per-workspace and persisted under .codeframe/notifications_config.json. """ import logging -from typing import cast +from typing import Optional, cast from anthropic import Anthropic as _AnthropicClient from anthropic import AuthenticationError as _AnthropicAuthError @@ -22,6 +26,7 @@ from fastapi.concurrency import run_in_threadpool from openai import AuthenticationError as _OpenAIAuthError from openai import OpenAI as _OpenAIClient +from pydantic import BaseModel, Field from codeframe.core.config import ( AgentBudgetConfig, @@ -35,7 +40,15 @@ CredentialSource, validate_credential_format, ) +from codeframe.core.notifications_config import ( + load_notifications_config, + save_notifications_config, +) from codeframe.core.workspace import Workspace +from codeframe.notifications.webhook import ( + WebhookNotificationService, + format_test_payload, +) from codeframe.lib.rate_limiter import rate_limit_ai, rate_limit_standard from codeframe.ui.dependencies import get_v2_workspace from codeframe.ui.models import ( @@ -386,3 +399,164 @@ async def verify_key( valid, message = False, "Verification not supported for this provider" return VerifyKeyResponse(provider=body.provider, valid=valid, message=message) + + +# ============================================================================ +# Outbound webhook notifications (issue #560) +# +# Per-workspace: stored under .codeframe/notifications_config.json. The URL is +# kept as plaintext for v1 — the workspace DB is already a local file, and the +# legacy BLOCKER_WEBHOOK_URL env var was plaintext too. Encryption-at-rest is +# tracked as future work. +# ============================================================================ + + +class NotificationSettingsResponse(BaseModel): + webhook_url: Optional[str] = None + webhook_enabled: bool = False + + +class UpdateNotificationSettingsRequest(BaseModel): + webhook_url: Optional[str] = Field( + default=None, + description="Outbound webhook URL. Empty/None clears the value.", + ) + webhook_enabled: bool = False + + +class TestWebhookResponse(BaseModel): + ok: bool + status_code: Optional[int] = None + error: Optional[str] = None + + +@router.get("/notifications", response_model=NotificationSettingsResponse) +@rate_limit_standard() +async def get_notification_settings( + request: Request, + workspace: Workspace = Depends(get_v2_workspace), +) -> NotificationSettingsResponse: + """Load outbound webhook config for this workspace.""" + cfg = load_notifications_config(workspace) + return NotificationSettingsResponse( + webhook_url=cfg["webhook_url"], + webhook_enabled=cfg["webhook_enabled"], + ) + + +_ALLOWED_WEBHOOK_SCHEMES = frozenset({"http", "https"}) + + +def _validate_webhook_url(url: Optional[str]) -> Optional[str]: + """Normalize and validate a user-supplied webhook URL. + + Returns the trimmed URL on success, or ``None`` if empty. Raises + ``HTTPException(400)`` for non-``http(s)`` schemes — without this guard, + a user could enter ``file:///...`` or ``ftp://...`` and aiohttp would + happily attempt the request (SSRF on local resources). + """ + from urllib.parse import urlparse + + trimmed = (url or "").strip() + if not trimmed: + return None + parsed = urlparse(trimmed) + if parsed.scheme.lower() not in _ALLOWED_WEBHOOK_SCHEMES: + raise HTTPException( + status_code=400, + detail=api_error( + f"Webhook URL must use http or https, got: {parsed.scheme!r}", + ErrorCodes.VALIDATION_ERROR, + ), + ) + if not parsed.netloc: + raise HTTPException( + status_code=400, + detail=api_error( + "Webhook URL must include a host", + ErrorCodes.VALIDATION_ERROR, + ), + ) + return trimmed + + +@router.put("/notifications", response_model=NotificationSettingsResponse) +@rate_limit_standard() +async def update_notification_settings( + request: Request, + body: UpdateNotificationSettingsRequest, + workspace: Workspace = Depends(get_v2_workspace), +) -> NotificationSettingsResponse: + """Save outbound webhook config for this workspace. + + An empty / whitespace-only URL is normalized to ``None``. The enabled + flag is preserved as-is so the user can toggle delivery without losing + the saved URL. The URL is validated for ``http(s)`` scheme to avoid + ``file://`` / ``ftp://`` SSRF on local resources. + """ + url = _validate_webhook_url(body.webhook_url) + try: + save_notifications_config( + workspace, + {"webhook_url": url, "webhook_enabled": body.webhook_enabled}, + ) + except OSError as e: + logger.error("Failed to save notifications config: %s", e, exc_info=True) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to save notifications config", + ErrorCodes.EXECUTION_FAILED, + str(e), + ), + ) + return NotificationSettingsResponse( + webhook_url=url, webhook_enabled=body.webhook_enabled + ) + + +@router.post("/notifications/test", response_model=TestWebhookResponse) +@rate_limit_standard() +async def test_notification_webhook( + request: Request, + workspace: Workspace = Depends(get_v2_workspace), +) -> TestWebhookResponse: + """Fire a test payload against the configured webhook URL. + + Returns the HTTP status code on success, or an error message on failure. + Returns 400 if no URL is configured or if the stored URL fails the same + safety checks the PUT endpoint enforces — the [Test] button should be + disabled in that case, but we guard server-side too. + """ + try: + cfg = load_notifications_config(workspace) + except Exception as e: + logger.error("Failed to load notifications config: %s", e, exc_info=True) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to load notifications config", + ErrorCodes.EXECUTION_FAILED, + str(e), + ), + ) + + url = (cfg["webhook_url"] or "").strip() + if not url: + raise HTTPException( + status_code=400, + detail=api_error( + "No webhook URL configured", ErrorCodes.VALIDATION_ERROR + ), + ) + # Re-validate at send time — defence-in-depth for hand-edited config files. + # We discard the return value: ``_validate_webhook_url`` raises + # ``HTTPException(400)`` on a bad scheme/host, which is the side-effect + # we want here. The trimmed URL it returns is already in ``url``. + _validate_webhook_url(url) + + svc = WebhookNotificationService(webhook_url=url, timeout=5) + result = await svc.send_event(format_test_payload()) + return TestWebhookResponse( + ok=result.ok, status_code=result.status_code, error=result.error + ) diff --git a/docs/PRODUCT_ROADMAP.md b/docs/PRODUCT_ROADMAP.md index 4f5ff5e3..1fd7d1bb 100644 --- a/docs/PRODUCT_ROADMAP.md +++ b/docs/PRODUCT_ROADMAP.md @@ -126,7 +126,7 @@ Without a settings page, a new user who cannot find the env vars cannot use the ### 3. Async Notifications -**Current state**: Browser notifications + in-app notification center shipped (#559). Webhook integration (#560) is the only remaining piece — out of scope for #559, tracked separately. +**Current state**: Both pieces of Phase 5.3 are shipped — browser notifications + in-app notification center (#559) and outbound webhooks (#560). **What was built (#559)**: @@ -134,11 +134,14 @@ Without a settings page, a new user who cannot find the env vars cannot use the - **In-app notification center**: bell icon in the sidebar footer with unread badge and dropdown panel; last 20 notifications, per-item dismiss, mark-all-read, and clear-all actions; persisted in `localStorage` scoped per workspace - **Permission request**: fires once on first visit to `/execution` only when permission state is `default` -**Known limitation**: notifications only fire while the `BatchExecutionMonitor` is mounted — a global background poller for cross-page notifications would require an architecture change and is out of scope for #559. +**Known limitation (#559)**: notifications only fire while the `BatchExecutionMonitor` is mounted — a global background poller for cross-page notifications would require an architecture change and is out of scope for #559. -**What's still planned (#560)**: +**What was built (#560)**: -- **Optional webhook**: a single URL the user can configure to receive JSON payloads on key events (batch done, blocker created, PR merged) — supports Slack, Discord, or any HTTP endpoint +- **Outbound webhook**: a single URL the user can configure in the Settings → Notifications tab to receive JSON payloads on three events — `batch.completed`, `blocker.created`, and `pr.merged` (Slack/Discord/any HTTP endpoint) +- **Test button**: posts a sample payload and surfaces the HTTP status code via toast +- **Storage**: per-workspace `.codeframe/notifications_config.json`, atomically written; URL is plaintext (a `TODO: encrypt at rest` is tracked as future work) +- **Reliability**: fire-and-forget with 5s timeout; failures are logged but never break the triggering operation --- @@ -200,7 +203,7 @@ These are items that were considered and excluded because they do not serve the | 4B | Post-merge glitch capture loop | ❌ Not started | — | | 5.1 | Settings page (skeleton + agent config + PROOF9/workspace tabs) | ✅ Complete | #554–556 | | 5.2 | Cost analytics | ✅ Complete | #557–558 | -| 5.3 | Async notifications | 🚧 Browser + in-app center shipped (#559); webhook (#560) deferred | #559–560 | +| 5.3 | Async notifications | ✅ Complete (browser + in-app center #559, webhook #560) | #559–560 | | 5.4 | PRD stress-test web UI | ❌ Not started | #561–562 | | 5.5 | GitHub Issues import | ❌ Not started | #563–565 | diff --git a/tests/core/test_blockers_webhook.py b/tests/core/test_blockers_webhook.py new file mode 100644 index 00000000..a46fd1e9 --- /dev/null +++ b/tests/core/test_blockers_webhook.py @@ -0,0 +1,82 @@ +"""Test outbound webhook dispatch on blocker creation (issue #560).""" + +from __future__ import annotations + +import shutil +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest + +from codeframe.core import blockers +from codeframe.core.notifications_config import save_notifications_config +from codeframe.core.workspace import create_or_load_workspace + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def workspace(): + temp_dir = Path(tempfile.mkdtemp()) + ws_path = temp_dir / "ws" + ws_path.mkdir(parents=True, exist_ok=True) + ws = create_or_load_workspace(ws_path) + try: + yield ws + finally: + shutil.rmtree(temp_dir, ignore_errors=True) + + +def test_no_webhook_dispatch_when_disabled(workspace): + """No URL configured → no WebhookNotificationService instantiated.""" + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + blockers.create(workspace, question="any?") + MockSvc.assert_not_called() + + +def test_no_webhook_dispatch_when_url_set_but_flag_off(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": False}, + ) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + blockers.create(workspace, question="any?") + MockSvc.assert_not_called() + + +def test_dispatches_webhook_when_enabled(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + instance = MockSvc.return_value + blockers.create(workspace, question="why?", task_id="t-1") + MockSvc.assert_called_once() + # The payload should be a blocker.created event for the newly created blocker. + instance.send_event_background.assert_called_once() + payload = instance.send_event_background.call_args.args[0] + assert payload["event"] == "blocker.created" + assert payload["task_id"] == "t-1" + assert "blocker_id" in payload + + +def test_webhook_failure_does_not_break_blocker_create(workspace): + """Even if the webhook plumbing explodes, blocker creation succeeds.""" + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService", + side_effect=RuntimeError("boom"), + ): + blocker = blockers.create(workspace, question="why?") + assert blocker.id # still got created diff --git a/tests/core/test_conductor_webhook.py b/tests/core/test_conductor_webhook.py new file mode 100644 index 00000000..28a599bd --- /dev/null +++ b/tests/core/test_conductor_webhook.py @@ -0,0 +1,129 @@ +"""Test outbound webhook dispatch on batch completion (issue #560). + +We don't exercise the full conductor lifecycle here — the dispatch helper +is the contract under test, and the conductor calls it at each of the four +``BATCH_COMPLETED``-capable sites (verified by grep in the implementation). +""" + +from __future__ import annotations + +import shutil +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest + +from codeframe.core import events +from codeframe.core.conductor import _dispatch_batch_completed_webhook +from codeframe.core.notifications_config import save_notifications_config +from codeframe.core.workspace import create_or_load_workspace + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def workspace(): + temp_dir = Path(tempfile.mkdtemp()) + ws_path = temp_dir / "ws" + ws_path.mkdir(parents=True, exist_ok=True) + ws = create_or_load_workspace(ws_path) + try: + yield ws + finally: + shutil.rmtree(temp_dir, ignore_errors=True) + + +def _enable_webhook(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + + +def test_does_not_fire_for_partial(workspace): + _enable_webhook(workspace) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_PARTIAL, "b-1", 5 + ) + MockSvc.assert_not_called() + + +def test_does_not_fire_for_failed(workspace): + _enable_webhook(workspace) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_FAILED, "b-1", 0 + ) + MockSvc.assert_not_called() + + +def test_does_not_fire_for_cancelled(workspace): + _enable_webhook(workspace) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_CANCELLED, "b-1", 0 + ) + MockSvc.assert_not_called() + + +def test_fires_for_completed_when_enabled(workspace): + _enable_webhook(workspace) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + instance = MockSvc.return_value + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_COMPLETED, "b-42", 7 + ) + MockSvc.assert_called_once() + instance.send_event_background.assert_called_once() + payload = instance.send_event_background.call_args.args[0] + assert payload["event"] == "batch.completed" + assert payload["batch_id"] == "b-42" + assert payload["task_count"] == 7 + + +def test_does_not_fire_when_url_missing(workspace): + # No save_notifications_config call → config is at defaults (disabled). + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_COMPLETED, "b-1", 1 + ) + MockSvc.assert_not_called() + + +def test_does_not_fire_when_disabled(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": False}, + ) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_COMPLETED, "b-1", 1 + ) + MockSvc.assert_not_called() + + +def test_failure_in_dispatch_is_swallowed(workspace): + """A misconfigured webhook must never bubble up into the batch flow.""" + _enable_webhook(workspace) + with patch( + "codeframe.notifications.webhook.WebhookNotificationService", + side_effect=RuntimeError("boom"), + ): + # Should not raise. + _dispatch_batch_completed_webhook( + workspace, events.EventType.BATCH_COMPLETED, "b-1", 1 + ) diff --git a/tests/core/test_notifications_config.py b/tests/core/test_notifications_config.py new file mode 100644 index 00000000..bd78012f --- /dev/null +++ b/tests/core/test_notifications_config.py @@ -0,0 +1,228 @@ +"""Tests for per-workspace notifications config (issue #560).""" + +from __future__ import annotations + +import shutil +import tempfile +from pathlib import Path + +import pytest + +from codeframe.core.notifications_config import ( + NOTIFICATIONS_CONFIG_FILENAME, + is_webhook_active, + load_notifications_config, + save_notifications_config, +) +from codeframe.core.workspace import create_or_load_workspace + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def workspace(): + temp_dir = Path(tempfile.mkdtemp()) + ws_path = temp_dir / "ws" + ws_path.mkdir(parents=True, exist_ok=True) + ws = create_or_load_workspace(ws_path) + try: + yield ws + finally: + shutil.rmtree(temp_dir, ignore_errors=True) + + +def test_load_returns_defaults_when_file_missing(workspace): + cfg = load_notifications_config(workspace) + assert cfg == {"webhook_url": None, "webhook_enabled": False} + + +def test_save_then_load_roundtrip(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/hook", "webhook_enabled": True}, + ) + cfg = load_notifications_config(workspace) + assert cfg["webhook_url"] == "https://example.com/hook" + assert cfg["webhook_enabled"] is True + + +def test_save_writes_to_state_dir(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://x.test/hook", "webhook_enabled": False}, + ) + expected = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + assert expected.exists() + + +def test_load_handles_corrupt_json(workspace): + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text("{ not valid json") + cfg = load_notifications_config(workspace) + assert cfg == {"webhook_url": None, "webhook_enabled": False} + + +def test_empty_url_is_normalized_to_none(workspace): + save_notifications_config( + workspace, + {"webhook_url": "", "webhook_enabled": True}, + ) + cfg = load_notifications_config(workspace) + assert cfg["webhook_url"] is None + + +def test_is_webhook_active_returns_url_when_enabled(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + assert is_webhook_active(workspace) == "https://example.com/h" + + +def test_is_webhook_active_returns_none_when_disabled(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": False}, + ) + assert is_webhook_active(workspace) is None + + +def test_is_webhook_active_returns_none_when_url_missing(workspace): + save_notifications_config( + workspace, + {"webhook_url": None, "webhook_enabled": True}, + ) + assert is_webhook_active(workspace) is None + + +def test_is_webhook_active_trims_whitespace(workspace): + save_notifications_config( + workspace, + {"webhook_url": " ", "webhook_enabled": True}, + ) + assert is_webhook_active(workspace) is None + + +def test_load_handles_non_object_json_list(workspace): + """A valid-JSON-but-not-object payload (e.g. ``[]``) must not crash.""" + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text("[]") + cfg = load_notifications_config(workspace) + assert cfg == {"webhook_url": None, "webhook_enabled": False} + + +def test_load_handles_non_object_json_null(workspace): + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text("null") + cfg = load_notifications_config(workspace) + assert cfg == {"webhook_url": None, "webhook_enabled": False} + + +def test_load_handles_non_object_json_integer(workspace): + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text("42") + cfg = load_notifications_config(workspace) + assert cfg == {"webhook_url": None, "webhook_enabled": False} + + +def test_is_webhook_active_rejects_file_scheme_in_stored_config(workspace): + """Defence-in-depth: hand-edited config with ``file://`` must not be dispatched.""" + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + # Write directly, bypassing save's normalization, to simulate hand edit. + path.write_text( + '{"webhook_url": "file:///etc/passwd", "webhook_enabled": true}' + ) + assert is_webhook_active(workspace) is None + + +def test_is_webhook_active_rejects_schemeless_url_in_stored_config(workspace): + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text( + '{"webhook_url": "example.com/hook", "webhook_enabled": true}' + ) + assert is_webhook_active(workspace) is None + + +def test_is_webhook_active_accepts_valid_https(workspace): + save_notifications_config( + workspace, + {"webhook_url": "https://hooks.example.com/h", "webhook_enabled": True}, + ) + assert is_webhook_active(workspace) == "https://hooks.example.com/h" + + +def test_unsafe_url_log_does_not_leak_basic_auth(workspace, caplog): + """Basic-auth credentials in the URL must be stripped before logging. + ``parsed.netloc`` preserves ``user:password@host``, so we use + ``parsed.hostname`` to drop the auth segment. + """ + import logging + + # Unsafe scheme so we hit the warning branch. + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text( + '{"webhook_url": "file://admin:SuperSecret@private.host/x", ' + '"webhook_enabled": true}' + ) + + with caplog.at_level(logging.WARNING): + assert is_webhook_active(workspace) is None + + log_text = "\n".join(rec.getMessage() for rec in caplog.records) + assert "SuperSecret" not in log_text + assert "admin:" not in log_text + + +def test_redact_url_for_log_preserves_port(): + """Port is non-secret and useful in logs (which environment), so keep it.""" + from codeframe.core.notifications_config import _redact_url_for_log + + assert _redact_url_for_log("https://hooks.example.com:8443/path/x?q=1") == ( + "https://hooks.example.com:8443" + ) + + +def test_redact_url_for_log_handles_malformed_port(): + """``parsed.port`` raises ValueError for invalid ports — the redaction + helper must convert that into rather than letting it bubble + out and turn ``is_webhook_active``'s fail-safe branch into an exception. + """ + from codeframe.core.notifications_config import _redact_url_for_log + + assert _redact_url_for_log("https://host:not-a-port/path") == "" + + +def test_is_webhook_active_returns_none_for_malformed_port(workspace): + """The fail-safe branch must return None even when the URL has a + malformed port that would otherwise crash ``parsed.port`` access.""" + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text( + '{"webhook_url": "file://private.host:abc/secret", "webhook_enabled": true}' + ) + assert is_webhook_active(workspace) is None + + +def test_unsafe_url_log_does_not_leak_path_or_query(workspace, caplog): + """Logs must NOT echo the full URL — webhook URLs often embed secrets + in the path or query (e.g. Slack token in path, signed Zapier hook).""" + import logging + + # Write directly to bypass save's PUT-style normalization (which would + # reject this URL). + secret_path = ( + "https://example.com/services/T123456/B999/SUPER_SECRET_TOKEN_xyz?sig=abc" + ) + # Use an unsafe scheme so the unsafe-URL branch fires. + path = workspace.state_dir / NOTIFICATIONS_CONFIG_FILENAME + path.write_text( + f'{{"webhook_url": "file://{secret_path}", "webhook_enabled": true}}' + ) + + with caplog.at_level(logging.WARNING): + assert is_webhook_active(workspace) is None + + log_text = "\n".join(rec.getMessage() for rec in caplog.records) + # The redacted form (scheme://host) is fine; the secret path/token is not. + assert "SUPER_SECRET_TOKEN" not in log_text + assert "T123456" not in log_text + assert "sig=abc" not in log_text diff --git a/tests/notifications/test_webhook_send_event.py b/tests/notifications/test_webhook_send_event.py new file mode 100644 index 00000000..b6a86f87 --- /dev/null +++ b/tests/notifications/test_webhook_send_event.py @@ -0,0 +1,261 @@ +"""Tests for the generic outbound webhook ``send_event`` method (issue #560).""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import aiohttp +import pytest + +from codeframe.notifications.webhook import ( + WebhookNotificationService, + WebhookSendResult, + format_batch_payload, + format_blocker_payload, + format_pr_payload, + format_test_payload, +) + +pytestmark = pytest.mark.v2 + + +def _mock_post(status: int): + """Build the AsyncMock chain that mirrors aiohttp's session.post() context.""" + mock_response = AsyncMock() + mock_response.status = status + mock_post_context = AsyncMock() + mock_post_context.__aenter__.return_value = mock_response + mock_post_context.__aexit__.return_value = None + mock_session = MagicMock() + mock_session.post.return_value = mock_post_context + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + return mock_session + + +@pytest.mark.asyncio +async def test_send_event_returns_ok_on_2xx(): + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + payload = format_test_payload() + with patch("aiohttp.ClientSession", return_value=_mock_post(200)): + result = await svc.send_event(payload) + assert isinstance(result, WebhookSendResult) + assert result.ok is True + assert result.status_code == 200 + assert result.error is None + + +@pytest.mark.asyncio +async def test_send_event_returns_not_ok_on_5xx(): + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + with patch("aiohttp.ClientSession", return_value=_mock_post(500)): + result = await svc.send_event({"event": "test"}) + assert result.ok is False + assert result.status_code == 500 + + +@pytest.mark.asyncio +async def test_send_event_no_url_returns_error(): + svc = WebhookNotificationService(webhook_url=None, timeout=5) + result = await svc.send_event({"event": "test"}) + assert result.ok is False + assert result.status_code is None + assert result.error and "No webhook URL" in result.error + + +@pytest.mark.asyncio +async def test_send_event_url_override_used(): + """A per-call URL override takes precedence over the constructor URL.""" + svc = WebhookNotificationService(webhook_url=None, timeout=5) + mock_session = _mock_post(200) + with patch("aiohttp.ClientSession", return_value=mock_session): + result = await svc.send_event( + {"event": "test"}, url="https://override.example/hook" + ) + assert result.ok is True + # Verify the override URL was actually passed to session.post + mock_session.post.assert_called_once() + args, _ = mock_session.post.call_args + assert args[0] == "https://override.example/hook" + + +@pytest.mark.asyncio +async def test_send_event_handles_timeout(): + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=1) + mock_session = MagicMock() + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + mock_session.post.side_effect = asyncio.TimeoutError() + with patch("aiohttp.ClientSession", return_value=mock_session): + result = await svc.send_event({"event": "test"}) + assert result.ok is False + assert result.status_code is None + assert "Timeout" in (result.error or "") + + +@pytest.mark.asyncio +async def test_send_event_handles_client_error(): + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + mock_session = MagicMock() + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + mock_session.post.side_effect = aiohttp.ClientError("connection refused") + with patch("aiohttp.ClientSession", return_value=mock_session): + result = await svc.send_event({"event": "test"}) + assert result.ok is False + assert result.status_code is None + assert "connection refused" in (result.error or "") + + +def test_send_event_background_outside_loop_runs_in_thread(): + """In sync context (no running loop), dispatch spawns a daemon thread + and runs the send to completion. The caller must not block beyond the + thread spawn cost. + + This is the CLI batch-run path — without this, webhooks from + ``cf work batch run`` would silently never fire. + """ + import threading + + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + threads_before = threading.active_count() + + with patch( + "codeframe.notifications.webhook.WebhookNotificationService._run_send_event_sync" + ) as mock_runner: + svc.send_event_background({"event": "test"}) + # Thread is spawned synchronously and starts immediately. Give it a + # beat to run. + import time + + for _ in range(50): + if mock_runner.called: + break + time.sleep(0.01) + + assert mock_runner.called, "expected daemon thread to invoke the sync runner" + # The thread spawns and dies — no leaked thread count. Give it up to a + # second to clean up before asserting (the runner is mocked so this is + # really just the thread overhead). + for _ in range(50): + leaked = threading.active_count() - threads_before + if leaked <= 0: + break + time.sleep(0.02) + assert threading.active_count() - threads_before <= 0, ( + "send_event_background leaked a thread" + ) + + +@pytest.mark.asyncio +async def test_send_event_background_schedules_task(): + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + with patch("aiohttp.ClientSession", return_value=_mock_post(200)): + svc.send_event_background({"event": "test"}) + # Yield the loop so the task actually runs before the test exits. + await asyncio.sleep(0) + await asyncio.sleep(0) + + +@pytest.mark.asyncio +async def test_send_event_background_task_has_done_callback(): + """Python 3.11+ logs ``Task exception was never retrieved`` when an + async task ends with an unhandled exception and nobody awaits or calls + .exception(). ``send_event`` swallows exceptions internally, but + defence-in-depth: attach a done-callback so the result is always + consumed and the log noise never appears in production. + """ + svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5) + + captured_tasks = [] + loop = asyncio.get_running_loop() + real_create_task = loop.create_task + + def capture(coro): + t = real_create_task(coro) + captured_tasks.append(t) + return t + + with patch.object(loop, "create_task", side_effect=capture): + with patch("aiohttp.ClientSession", return_value=_mock_post(200)): + svc.send_event_background({"event": "test"}) + + assert len(captured_tasks) == 1 + # The task's repr includes its registered callbacks. The send_event_background + # lambda must appear there — without it, Python 3.11+ would warn on task + # completion if send_event ever raised. + assert "send_event_background.." in repr(captured_tasks[0]) + + # Drain the task so the test exit is clean. After cancelling, await + # the task so cancellation actually propagates — otherwise Python may + # emit "Task was destroyed but it is pending" warnings on loop close. + try: + await asyncio.wait_for(captured_tasks[0], timeout=1.0) + except asyncio.TimeoutError: + captured_tasks[0].cancel() + try: + await captured_tasks[0] + except (asyncio.CancelledError, Exception): + pass + + +def test_format_batch_payload(): + p = format_batch_payload(batch_id="b-123", task_count=5) + assert p["event"] == "batch.completed" + assert p["batch_id"] == "b-123" + assert p["task_count"] == 5 + assert "timestamp" in p + + +def test_format_blocker_payload(): + p = format_blocker_payload(blocker_id="bk-7", task_id="t-1") + assert p["event"] == "blocker.created" + assert p["blocker_id"] == "bk-7" + assert p["task_id"] == "t-1" + assert "timestamp" in p + + +def test_format_blocker_payload_allows_null_task_id(): + p = format_blocker_payload(blocker_id="bk-7", task_id=None) + assert p["task_id"] is None + + +def test_format_pr_payload(): + p = format_pr_payload(pr_number=42, pr_url="https://github.com/o/r/pull/42") + assert p["event"] == "pr.merged" + assert p["pr_number"] == 42 + assert p["pr_url"] == "https://github.com/o/r/pull/42" + assert "timestamp" in p + + +def test_format_pr_payload_url_is_optional(): + """When the GitHub integration can't construct a URL we send None, + not an unparseable sentinel like 'pr#42' — consumers can branch on + pr_number (always present) and use pr_url when available.""" + p = format_pr_payload(pr_number=99) + assert p["event"] == "pr.merged" + assert p["pr_number"] == 99 + assert p["pr_url"] is None + + +def test_format_batch_payload_includes_status(): + """Payload self-documents the terminal state — forward-compatible + if PARTIAL/FAILED events get added later.""" + p = format_batch_payload(batch_id="b-1", task_count=3) + assert p["status"] == "completed" + + +def test_format_test_payload(): + p = format_test_payload() + assert p["event"] == "test" + assert "timestamp" in p + + +def test_timestamp_uses_z_suffix_not_offset(): + """Slack/Discord/Zapier prefer the Z suffix over +00:00 offset format.""" + p = format_test_payload() + assert p["timestamp"].endswith("Z"), ( + f"Expected ISO timestamp with Z suffix, got: {p['timestamp']!r}" + ) + assert "+00:00" not in p["timestamp"] diff --git a/tests/ui/test_pr_v2_webhook.py b/tests/ui/test_pr_v2_webhook.py new file mode 100644 index 00000000..d028b25e --- /dev/null +++ b/tests/ui/test_pr_v2_webhook.py @@ -0,0 +1,98 @@ +"""Test outbound webhook dispatch on PR merge (issue #560).""" + +from __future__ import annotations + +import shutil +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest + +from codeframe.core.notifications_config import save_notifications_config +from codeframe.core.workspace import create_or_load_workspace +from codeframe.ui.routers.pr_v2 import _dispatch_pr_merged_webhook + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def workspace(): + temp_dir = Path(tempfile.mkdtemp()) + ws_path = temp_dir / "ws" + ws_path.mkdir(parents=True, exist_ok=True) + ws = create_or_load_workspace(ws_path) + try: + yield ws + finally: + shutil.rmtree(temp_dir, ignore_errors=True) + + +def test_no_dispatch_when_disabled(workspace): + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + _dispatch_pr_merged_webhook(workspace, pr_number=42) + MockSvc.assert_not_called() + + +def test_dispatches_when_enabled_with_pr_url(workspace, monkeypatch): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + # Provide GitHub env so GitHubIntegration() succeeds and the URL has the + # owner/repo segment. + monkeypatch.setenv("GITHUB_TOKEN", "ghp_test_token") + monkeypatch.setenv("GITHUB_REPO", "frankbria/codeframe") + + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + instance = MockSvc.return_value + _dispatch_pr_merged_webhook(workspace, pr_number=42) + MockSvc.assert_called_once() + instance.send_event_background.assert_called_once() + payload = instance.send_event_background.call_args.args[0] + assert payload["event"] == "pr.merged" + assert payload["pr_number"] == 42 + assert payload["pr_url"] == "https://github.com/frankbria/codeframe/pull/42" + + +def test_dispatches_with_null_url_when_github_unconfigured(workspace, monkeypatch): + """If GitHubIntegration() can't be constructed, we still emit the event + but pr_url is None — consumers branch on pr_number (always present) + rather than parsing an unparseable sentinel.""" + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_REPO", raising=False) + + with patch( + "codeframe.notifications.webhook.WebhookNotificationService" + ) as MockSvc: + instance = MockSvc.return_value + _dispatch_pr_merged_webhook(workspace, pr_number=99) + + MockSvc.assert_called_once() + payload = instance.send_event_background.call_args.args[0] + assert payload["event"] == "pr.merged" + assert payload["pr_number"] == 99 + assert payload["pr_url"] is None + + +def test_dispatch_failure_does_not_raise(workspace, monkeypatch): + save_notifications_config( + workspace, + {"webhook_url": "https://example.com/h", "webhook_enabled": True}, + ) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_test_token") + monkeypatch.setenv("GITHUB_REPO", "frankbria/codeframe") + with patch( + "codeframe.notifications.webhook.WebhookNotificationService", + side_effect=RuntimeError("boom"), + ): + # Should not raise. + _dispatch_pr_merged_webhook(workspace, pr_number=1) diff --git a/tests/ui/test_settings_notifications.py b/tests/ui/test_settings_notifications.py new file mode 100644 index 00000000..4cbcd626 --- /dev/null +++ b/tests/ui/test_settings_notifications.py @@ -0,0 +1,243 @@ +"""Tests for the notifications endpoints in settings_v2 (issue #560). + +Covers: +- GET returns defaults when no config exists +- PUT persists URL + enabled flag and GET reads it back +- PUT with empty URL clears the value +- POST /test returns 400 when no URL configured +- POST /test surfaces the underlying webhook status code +""" + +from __future__ import annotations + +import shutil +import tempfile +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def workspace(): + temp_dir = Path(tempfile.mkdtemp()) + ws_path = temp_dir / "ws" + ws_path.mkdir(parents=True, exist_ok=True) + from codeframe.core.workspace import create_or_load_workspace + + ws = create_or_load_workspace(ws_path) + try: + yield ws + finally: + shutil.rmtree(temp_dir, ignore_errors=True) + + +@pytest.fixture +def client(workspace): + from codeframe.ui.dependencies import get_v2_workspace + from codeframe.ui.routers import settings_v2 + + app = FastAPI() + app.include_router(settings_v2.router) + app.dependency_overrides[get_v2_workspace] = lambda: workspace + return TestClient(app) + + +# Note on the "workspace_path contract" suggestion raised by reviewers: +# `get_v2_workspace` does not strictly fail when ``workspace_path`` is +# absent — it falls back to default-workspace resolution. So a test +# asserting 400/422 on missing query param wouldn't reflect reality. +# That contract is a router-wide concern; it's not specific to #560 and +# would need a fix in the dependency itself, not in this endpoint's tests. + + +class TestGetNotificationSettings: + def test_returns_defaults_when_no_config(self, client): + r = client.get("/api/v2/settings/notifications") + assert r.status_code == 200 + data = r.json() + assert data == {"webhook_url": None, "webhook_enabled": False} + + +class TestUpdateNotificationSettings: + def test_persists_url_and_flag(self, client): + r = client.put( + "/api/v2/settings/notifications", + json={ + "webhook_url": "https://hooks.example.com/abc", + "webhook_enabled": True, + }, + ) + assert r.status_code == 200 + data = r.json() + assert data["webhook_url"] == "https://hooks.example.com/abc" + assert data["webhook_enabled"] is True + + def test_roundtrip_get_after_put(self, client): + client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "https://x.test/h", "webhook_enabled": False}, + ) + r = client.get("/api/v2/settings/notifications") + data = r.json() + assert data["webhook_url"] == "https://x.test/h" + assert data["webhook_enabled"] is False + + def test_empty_url_clears_value(self, client): + client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "https://x.test/h", "webhook_enabled": True}, + ) + client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "", "webhook_enabled": True}, + ) + r = client.get("/api/v2/settings/notifications") + assert r.json()["webhook_url"] is None + + def test_whitespace_url_normalized_to_none(self, client): + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": " ", "webhook_enabled": True}, + ) + assert r.json()["webhook_url"] is None + + def test_rejects_file_scheme_url(self, client): + """SSRF guard — file:// must be rejected (CVE-class issue).""" + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "file:///etc/passwd", "webhook_enabled": True}, + ) + assert r.status_code == 400 + + def test_rejects_ftp_scheme_url(self, client): + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "ftp://example.com/h", "webhook_enabled": True}, + ) + assert r.status_code == 400 + + def test_rejects_url_without_host(self, client): + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "http://", "webhook_enabled": True}, + ) + assert r.status_code == 400 + + def test_accepts_https(self, client): + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "https://hooks.example.com/h", "webhook_enabled": True}, + ) + assert r.status_code == 200 + + def test_accepts_http(self, client): + """Plain http is allowed for local testing / internal endpoints.""" + r = client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "http://localhost:9876/h", "webhook_enabled": True}, + ) + assert r.status_code == 200 + + +class TestNotificationWebhookTest: + def test_returns_400_when_no_url_configured(self, client): + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 400 + + def test_returns_400_when_url_present_but_empty(self, client): + # PUT empty URL → /test should still 400 since nothing to call. + client.put( + "/api/v2/settings/notifications", + json={"webhook_url": "", "webhook_enabled": True}, + ) + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 400 + + def test_returns_status_code_on_success(self, client): + client.put( + "/api/v2/settings/notifications", + json={ + "webhook_url": "https://hooks.example.com/abc", + "webhook_enabled": True, + }, + ) + # Mock aiohttp so we don't actually hit the network. + mock_response = AsyncMock() + mock_response.status = 204 + mock_post_context = AsyncMock() + mock_post_context.__aenter__.return_value = mock_response + mock_post_context.__aexit__.return_value = None + mock_session = MagicMock() + mock_session.post.return_value = mock_post_context + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + with patch("aiohttp.ClientSession", return_value=mock_session): + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 200 + data = r.json() + assert data["ok"] is True + assert data["status_code"] == 204 + + def test_returns_error_on_5xx(self, client): + client.put( + "/api/v2/settings/notifications", + json={ + "webhook_url": "https://hooks.example.com/abc", + "webhook_enabled": True, + }, + ) + mock_response = AsyncMock() + mock_response.status = 500 + mock_post_context = AsyncMock() + mock_post_context.__aenter__.return_value = mock_response + mock_post_context.__aexit__.return_value = None + mock_session = MagicMock() + mock_session.post.return_value = mock_post_context + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + with patch("aiohttp.ClientSession", return_value=mock_session): + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 200 + data = r.json() + assert data["ok"] is False + assert data["status_code"] == 500 + + def test_rejects_unsafe_stored_url_at_test_time(self, client, workspace): + """Defence-in-depth: even if a hand-edited config bypassed the PUT + validation, /test must refuse to POST to an unsafe URL.""" + # Write directly to the config file to simulate a hand-edited bypass. + path = workspace.state_dir / "notifications_config.json" + path.write_text( + '{"webhook_url": "file:///etc/passwd", "webhook_enabled": true}' + ) + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 400 + + def test_test_works_even_when_enabled_flag_is_false(self, client): + """The Test button should still work when the user is verifying + a URL before turning notifications on.""" + client.put( + "/api/v2/settings/notifications", + json={ + "webhook_url": "https://hooks.example.com/abc", + "webhook_enabled": False, + }, + ) + mock_response = AsyncMock() + mock_response.status = 200 + mock_post_context = AsyncMock() + mock_post_context.__aenter__.return_value = mock_response + mock_post_context.__aexit__.return_value = None + mock_session = MagicMock() + mock_session.post.return_value = mock_post_context + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + with patch("aiohttp.ClientSession", return_value=mock_session): + r = client.post("/api/v2/settings/notifications/test") + assert r.status_code == 200 + assert r.json()["ok"] is True diff --git a/web-ui/src/__tests__/components/settings/NotificationsTab.test.tsx b/web-ui/src/__tests__/components/settings/NotificationsTab.test.tsx new file mode 100644 index 00000000..09b76bb7 --- /dev/null +++ b/web-ui/src/__tests__/components/settings/NotificationsTab.test.tsx @@ -0,0 +1,223 @@ +import React from 'react'; +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; +import useSWR from 'swr'; + +import { NotificationsTab } from '@/components/settings/NotificationsTab'; +import { notificationsApi } from '@/lib/api'; +import type { + NotificationSettingsResponse, + TestWebhookResponse, +} from '@/types'; + +jest.mock('swr'); +jest.mock('@/lib/api', () => ({ + notificationsApi: { + get: jest.fn(), + update: jest.fn(), + test: jest.fn(), + }, +})); +jest.mock('sonner', () => ({ + toast: { + success: jest.fn(), + info: jest.fn(), + error: jest.fn(), + }, +})); + +import { toast } from 'sonner'; + +const mockUseSWR = useSWR as jest.MockedFunction; +const mockUpdate = notificationsApi.update as jest.MockedFunction< + typeof notificationsApi.update +>; +const mockTest = notificationsApi.test as jest.MockedFunction< + typeof notificationsApi.test +>; + +const DEFAULT_CONFIG: NotificationSettingsResponse = { + webhook_url: null, + webhook_enabled: false, +}; + +const CONFIGURED: NotificationSettingsResponse = { + webhook_url: 'https://hooks.example.com/abc', + webhook_enabled: true, +}; + +function mockSWR( + data: NotificationSettingsResponse | undefined, + mutate = jest.fn() +) { + mockUseSWR.mockReturnValue({ + data, + error: undefined, + isLoading: data === undefined, + mutate, + } as unknown as ReturnType); + return mutate; +} + +describe('NotificationsTab', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('shows a no-workspace message when path is null', () => { + mockSWR(undefined); + render(); + expect( + screen.getByText(/select a workspace/i) + ).toBeInTheDocument(); + }); + + it('renders the URL input and toggle when data loaded', () => { + mockSWR(DEFAULT_CONFIG); + render(); + expect(screen.getByLabelText(/webhook url/i)).toBeInTheDocument(); + expect(screen.getByLabelText(/enable webhook notifications/i)).toBeInTheDocument(); + }); + + it('Test button disabled when no URL configured', () => { + mockSWR(DEFAULT_CONFIG); + render(); + expect(screen.getByRole('button', { name: /^test$/i })).toBeDisabled(); + }); + + it('Test button disabled when the user has unsaved URL changes', () => { + mockSWR(DEFAULT_CONFIG); + render(); + fireEvent.change(screen.getByLabelText(/webhook url/i), { + target: { value: 'https://new.example/h' }, + }); + expect(screen.getByRole('button', { name: /^test$/i })).toBeDisabled(); + }); + + it('Test button enabled when URL is saved and not dirty', () => { + mockSWR(CONFIGURED); + render(); + expect(screen.getByRole('button', { name: /^test$/i })).toBeEnabled(); + }); + + + it('Save and Discard disabled when not dirty', () => { + mockSWR(CONFIGURED); + render(); + expect(screen.getByRole('button', { name: /save changes/i })).toBeDisabled(); + expect(screen.getByRole('button', { name: /discard/i })).toBeDisabled(); + }); + + it('Save becomes enabled after URL change', () => { + mockSWR(DEFAULT_CONFIG); + render(); + fireEvent.change(screen.getByLabelText(/webhook url/i), { + target: { value: 'https://x.test/h' }, + }); + expect(screen.getByRole('button', { name: /save changes/i })).toBeEnabled(); + }); + + it('calls notificationsApi.update with trimmed URL on Save', async () => { + const mutate = mockSWR(DEFAULT_CONFIG); + mockUpdate.mockResolvedValueOnce({ + webhook_url: 'https://x.test/h', + webhook_enabled: true, + }); + + render(); + + fireEvent.change(screen.getByLabelText(/webhook url/i), { + target: { value: ' https://x.test/h ' }, + }); + fireEvent.click(screen.getByLabelText(/enable webhook notifications/i)); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + }); + + expect(mockUpdate).toHaveBeenCalledWith('/ws', { + webhook_url: 'https://x.test/h', + webhook_enabled: true, + }); + expect(mutate).toHaveBeenCalled(); + expect(toast.success).toHaveBeenCalledWith('Notification settings saved'); + }); + + it('shows success toast with status code when Test succeeds', async () => { + mockSWR(CONFIGURED); + const result: TestWebhookResponse = { + ok: true, + status_code: 204, + error: null, + }; + mockTest.mockResolvedValueOnce(result); + + render(); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /^test$/i })); + }); + + await waitFor(() => { + expect(toast.success).toHaveBeenCalledWith( + expect.stringContaining('204') + ); + }); + }); + + it('shows error toast with status code when Test returns non-2xx', async () => { + mockSWR(CONFIGURED); + const result: TestWebhookResponse = { + ok: false, + status_code: 500, + error: null, + }; + mockTest.mockResolvedValueOnce(result); + + render(); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /^test$/i })); + }); + + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('500') + ); + }); + }); + + it('shows error toast with message when Test has network error', async () => { + mockSWR(CONFIGURED); + const result: TestWebhookResponse = { + ok: false, + status_code: null, + error: 'Timeout after 5s', + }; + mockTest.mockResolvedValueOnce(result); + + render(); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /^test$/i })); + }); + + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('Timeout') + ); + }); + }); + + it('Discard reverts unsaved edits', () => { + mockSWR(CONFIGURED); + render(); + + const urlInput = screen.getByLabelText(/webhook url/i) as HTMLInputElement; + fireEvent.change(urlInput, { target: { value: 'https://different/h' } }); + expect(urlInput.value).toBe('https://different/h'); + + fireEvent.click(screen.getByRole('button', { name: /discard/i })); + expect(urlInput.value).toBe('https://hooks.example.com/abc'); + expect(toast.info).toHaveBeenCalledWith('Changes discarded'); + }); +}); diff --git a/web-ui/src/__tests__/components/settings/SettingsPage.test.tsx b/web-ui/src/__tests__/components/settings/SettingsPage.test.tsx index c9d9df7a..7692a6d5 100644 --- a/web-ui/src/__tests__/components/settings/SettingsPage.test.tsx +++ b/web-ui/src/__tests__/components/settings/SettingsPage.test.tsx @@ -87,11 +87,12 @@ describe('SettingsPage', () => { mockGetWorkspace.mockReturnValue(WORKSPACE); }); - it('renders all four tabs', () => { + it('renders all five tabs', () => { mockSWR(SAMPLE_SETTINGS); render(); expect(screen.getByRole('tab', { name: 'Agent' })).toBeInTheDocument(); expect(screen.getByRole('tab', { name: 'API Keys' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Notifications' })).toBeInTheDocument(); expect(screen.getByRole('tab', { name: 'PROOF9' })).toBeInTheDocument(); expect(screen.getByRole('tab', { name: 'Workspace' })).toBeInTheDocument(); }); diff --git a/web-ui/src/app/settings/page.tsx b/web-ui/src/app/settings/page.tsx index 406664ef..6713fef1 100644 --- a/web-ui/src/app/settings/page.tsx +++ b/web-ui/src/app/settings/page.tsx @@ -9,6 +9,7 @@ import { settingsApi } from '@/lib/api'; import { getSelectedWorkspacePath } from '@/lib/workspace-storage'; import type { AgentSettings, AgentTypeKey, ApiError } from '@/types'; import { ApiKeysTab } from '@/components/settings/ApiKeysTab'; +import { NotificationsTab } from '@/components/settings/NotificationsTab'; import { Proof9DefaultsTab } from '@/components/settings/Proof9DefaultsTab'; import { WorkspaceConfigTab } from '@/components/settings/WorkspaceConfigTab'; import { Button } from '@/components/ui/button'; @@ -117,6 +118,7 @@ export default function SettingsPage() { Agent API Keys + Notifications PROOF9 Workspace @@ -163,6 +165,16 @@ export default function SettingsPage() { + +
+

Notifications

+

+ Outbound webhook for batch, blocker, and PR-merge events. +

+ +
+ +

PROOF9 Defaults

diff --git a/web-ui/src/components/settings/NotificationsTab.tsx b/web-ui/src/components/settings/NotificationsTab.tsx new file mode 100644 index 00000000..0ad63c50 --- /dev/null +++ b/web-ui/src/components/settings/NotificationsTab.tsx @@ -0,0 +1,195 @@ +'use client'; + +import { useEffect, useState } from 'react'; +import useSWR from 'swr'; +import { toast } from 'sonner'; + +import { notificationsApi } from '@/lib/api'; +import type { + ApiError, + NotificationSettingsResponse, +} from '@/types'; +import { Button } from '@/components/ui/button'; +import { Checkbox } from '@/components/ui/checkbox'; +import { Input } from '@/components/ui/input'; + +interface NotificationsTabProps { + workspacePath: string | null; +} + +function isDirty( + a: NotificationSettingsResponse, + b: NotificationSettingsResponse +): boolean { + return ( + (a.webhook_url ?? '') !== (b.webhook_url ?? '') || + a.webhook_enabled !== b.webhook_enabled + ); +} + +export function NotificationsTab({ workspacePath }: NotificationsTabProps) { + const swrKey = workspacePath ? ['notifications-settings', workspacePath] : null; + const { data, error, mutate } = useSWR( + swrKey, + () => notificationsApi.get(workspacePath!) + ); + + const [draft, setDraft] = useState(null); + const [saving, setSaving] = useState(false); + const [testing, setTesting] = useState(false); + + useEffect(() => { + if (data) { + setDraft({ ...data }); + } + }, [data]); + + if (!workspacePath) { + return ( +

+ Select a workspace from the sidebar to manage notifications. +

+ ); + } + if (error) { + return ( +

+ Failed to load notification settings. Check the server logs. +

+ ); + } + if (!data || !draft) { + return

Loading…

; + } + + const trimmedUrl = (draft.webhook_url ?? '').trim(); + const dirty = isDirty(data, draft); + const canSave = dirty && !saving; + // Test must require BOTH a saved URL AND no pending edits — including + // whitespace-only edits, which trim to a value that matches `data.webhook_url` + // but still leave the persisted form mid-edit. + const canTest = !testing && !dirty && !!data.webhook_url && trimmedUrl.length > 0; + + const handleSave = async () => { + setSaving(true); + try { + const saved = await notificationsApi.update(workspacePath, { + webhook_url: trimmedUrl || null, + webhook_enabled: draft.webhook_enabled, + }); + await mutate(saved, { revalidate: false }); + toast.success('Notification settings saved'); + } catch (err) { + const apiError = err as ApiError; + toast.error(apiError.detail || 'Failed to save notification settings'); + } finally { + setSaving(false); + } + }; + + const handleDiscard = () => { + setDraft({ ...data }); + toast.info('Changes discarded'); + }; + + const handleTest = async () => { + setTesting(true); + try { + const result = await notificationsApi.test(workspacePath); + if (result.ok) { + toast.success( + `✓ Webhook responded ${result.status_code ?? 'OK'}` + ); + } else if (result.status_code !== null && result.status_code !== undefined) { + toast.error( + `✗ Webhook returned ${result.status_code}` + ); + } else { + toast.error(`✗ ${result.error ?? 'Webhook request failed'}`); + } + } catch (err) { + const apiError = err as ApiError; + toast.error(apiError.detail || 'Test request failed'); + } finally { + setTesting(false); + } + }; + + return ( +
+
+ +
+ + setDraft({ ...draft, webhook_url: e.target.value || null }) + } + className="flex-1" + /> + +
+

+ A JSON payload is POSTed to this URL on batch completion, blocker + creation, and PR merge. +

+
+ +
+ +

+ Events: batch.completed, blocker.created, pr.merged. Failures are + logged but never break the triggering operation. +

+
+ +
+ + +
+
+ ); +} diff --git a/web-ui/src/lib/api.ts b/web-ui/src/lib/api.ts index 50463fe8..d896a522 100644 --- a/web-ui/src/lib/api.ts +++ b/web-ui/src/lib/api.ts @@ -70,6 +70,9 @@ import type { CostSummaryResponse, TaskCostsResponse, AgentCostsResponse, + NotificationSettingsResponse, + UpdateNotificationSettingsRequest, + TestWebhookResponse, } from '@/types'; // FastAPI validation error format @@ -912,6 +915,40 @@ export const workspaceConfigApi = { }, }; +// Outbound webhook notifications API (issue #560) +export const notificationsApi = { + get: async ( + workspacePath: string + ): Promise => { + const response = await api.get( + '/api/v2/settings/notifications', + { params: { workspace_path: workspacePath } } + ); + return response.data; + }, + + update: async ( + workspacePath: string, + body: UpdateNotificationSettingsRequest + ): Promise => { + const response = await api.put( + '/api/v2/settings/notifications', + body, + { params: { workspace_path: workspacePath } } + ); + return response.data; + }, + + test: async (workspacePath: string): Promise => { + const response = await api.post( + '/api/v2/settings/notifications/test', + undefined, + { params: { workspace_path: workspacePath } } + ); + return response.data; + }, +}; + // Cost analytics API (issues #557, #558) export const costsApi = { /** diff --git a/web-ui/src/types/index.ts b/web-ui/src/types/index.ts index bf43542d..1b0c06d8 100644 --- a/web-ui/src/types/index.ts +++ b/web-ui/src/types/index.ts @@ -620,6 +620,23 @@ export interface UpdateWorkspaceConfigRequest { tech_stack_override: string | null; } +// Outbound webhook notifications (issue #560) +export interface NotificationSettingsResponse { + webhook_url: string | null; + webhook_enabled: boolean; +} + +export interface UpdateNotificationSettingsRequest { + webhook_url: string | null; + webhook_enabled: boolean; +} + +export interface TestWebhookResponse { + ok: boolean; + status_code: number | null; + error: string | null; +} + // Cost analytics types (issue #557) export interface DailyCostPoint { date: string; // ISO YYYY-MM-DD