From 608585987442a9817779836f631e4731d492967e Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Fri, 29 May 2026 12:35:47 +0200 Subject: [PATCH] feat: notification center backend integration (PR-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wire lifecycle_hooks.py to NotificationService after event bus publish - Wire health_monitor.py to NotificationService after state changes - Category/severity mapping: instance.* → info, error → error, unhealthy → warning - Extend UserConfig API with notification_mute_categories and notification_toast_level - 6 integration tests for event-to-notification flow - All producer calls wrapped in try/except — failures logged, pipeline continues Quality gates: pytest 41 passed (monitoring + lifecycle), ruff clean --- apps/api/src/api/user_config.py | 12 +- apps/api/src/services/health_monitor.py | 34 ++ apps/api/src/services/lifecycle_hooks.py | 43 ++ .../test_notifications_lifecycle.py | 370 ++++++++++++++++++ .../changes/notification-center/apply-pr2.md | 156 ++++++++ .../notification-center/apply-progress.md | 99 +++-- 6 files changed, 690 insertions(+), 24 deletions(-) create mode 100644 apps/api/tests/integration/test_notifications_lifecycle.py create mode 100644 openspec/changes/notification-center/apply-pr2.md diff --git a/apps/api/src/api/user_config.py b/apps/api/src/api/user_config.py index 35aa93e..99a8892 100644 --- a/apps/api/src/api/user_config.py +++ b/apps/api/src/api/user_config.py @@ -14,7 +14,9 @@ logger = logging.getLogger(__name__) router = APIRouter(prefix="/users/me", tags=["user-config"]) -async def _get_or_create_config(session: AsyncSession, user_id: uuid.UUID) -> UserConfig: +async def _get_or_create_config( + session: AsyncSession, user_id: uuid.UUID +) -> UserConfig: """Get or create user config record. Args: @@ -24,7 +26,9 @@ async def _get_or_create_config(session: AsyncSession, user_id: uuid.UUID) -> Us Returns: The user's config, creating a new one if it doesn't exist. """ - result = await session.execute(select(UserConfig).where(UserConfig.user_id == user_id)) + result = await session.execute( + select(UserConfig).where(UserConfig.user_id == user_id) + ) config = result.scalar_one_or_none() if config is None: config = UserConfig(user_id=user_id, config={}) @@ -42,6 +46,8 @@ class UserConfigResponse(BaseModel): git_user_name: str | None = None git_user_email: str | None = None last_session_id: str | None = None + notification_mute_categories: list[str] | None = None + notification_toast_level: str | None = None class UserConfigUpdate(BaseModel): @@ -50,6 +56,8 @@ class UserConfigUpdate(BaseModel): git_user_name: str | None = None git_user_email: str | None = None last_session_id: str | None = None + notification_mute_categories: list[str] | None = None + notification_toast_level: str | None = None @router.get( diff --git a/apps/api/src/services/health_monitor.py b/apps/api/src/services/health_monitor.py index 96d10e6..eec060f 100644 --- a/apps/api/src/services/health_monitor.py +++ b/apps/api/src/services/health_monitor.py @@ -15,6 +15,7 @@ from src.models.tool_instance import ToolInstance from src.services.correlation import get_correlation_id from src.services.docker import check_tunnel_health, get_container_status from src.services.event_bus import InstanceEventBus, InstanceEventPayload +from src.services.notification_service import notification_service logger = logging.getLogger(__name__) @@ -217,3 +218,36 @@ class HealthMonitor: } await self._event_bus.publish(event_type, payload) + + # Create notification for instance owner (fire-and-forget) + if new_status == "error": + category = "instance" + severity = "error" + title = "Container failed" + else: + category = "health" + if new_status == "unhealthy": + severity = "warning" + title = "Container unhealthy" + else: + severity = "info" + title = "Container recovered" + + try: + await notification_service.create_notification( + session=session, + user_id=instance.owner_id, + category=category, + severity=severity, + title=title, + message=message, + source_type="tool_instances", + source_id=instance.id, + metadata=metadata, + ) + except Exception: + logger.exception( + "Failed to create notification for health event %s", + event_type, + extra={"correlation_id": correlation_id}, + ) diff --git a/apps/api/src/services/lifecycle_hooks.py b/apps/api/src/services/lifecycle_hooks.py index 8c921fe..553fec0 100644 --- a/apps/api/src/services/lifecycle_hooks.py +++ b/apps/api/src/services/lifecycle_hooks.py @@ -1,5 +1,6 @@ """Lifecycle hook helpers for instrumenting tool instance transitions.""" +import logging import uuid from datetime import datetime, timezone @@ -9,6 +10,25 @@ from src.models.instance_event import InstanceEvent from src.models.tool_instance import ToolInstance from src.services.correlation import get_correlation_id from src.services.event_bus import InstanceEventBus, InstanceEventPayload +from src.services.notification_service import notification_service + +logger = logging.getLogger(__name__) + + +def _derive_title(event_type: str) -> str: + """Map lifecycle event type to a human-readable notification title.""" + mapping = { + "instance.created": "Container created", + "instance.started": "Container started", + "instance.stopped": "Container stopped", + "instance.restarted": "Container restarted", + "instance.deleted": "Container deleted", + "instance.error": "Container error", + } + return mapping.get( + event_type, + event_type.replace("instance.", "").replace("_", " ").title(), + ) def _build_payload( @@ -96,3 +116,26 @@ async def publish_lifecycle_event( # Publish to bus await event_bus.publish(event_type, payload) + + # Create notification for instance owner (fire-and-forget) + severity = "error" if event_type == "instance.error" else "info" + title = _derive_title(event_type) + + try: + await notification_service.create_notification( + session=session, + user_id=instance.owner_id, + category="instance", + severity=severity, + title=title, + message=message, + source_type="tool_instances", + source_id=instance.id, + metadata=metadata, + ) + except Exception: + logger.exception( + "Failed to create notification for lifecycle event %s", + event_type, + extra={"correlation_id": payload.get("correlation_id", "unknown")}, + ) diff --git a/apps/api/tests/integration/test_notifications_lifecycle.py b/apps/api/tests/integration/test_notifications_lifecycle.py new file mode 100644 index 0000000..e6e9cfa --- /dev/null +++ b/apps/api/tests/integration/test_notifications_lifecycle.py @@ -0,0 +1,370 @@ +"""Integration tests for event producer → notification creation flow.""" + +import uuid +from collections.abc import Generator +from unittest.mock import patch + +import pytest +import pytest_asyncio +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from src.models.git_repository import GitRepository +from src.models.notification import Notification +from src.models.project import Project +from src.models.tool_instance import ToolInstance +from src.models.tool_type import ToolType +from src.models.user import User +from src.services.event_bus import InstanceEventBus, InstanceEventPayload +from src.services.health_monitor import HealthSnapshot + + +@pytest.fixture +def event_bus() -> Generator[InstanceEventBus, None, None]: + """Provide a fresh EventBus instance.""" + bus = InstanceEventBus() + bus._reset_for_testing() + yield bus + bus._reset_for_testing() + + +@pytest_asyncio.fixture +async def test_instance(db_session: AsyncSession) -> ToolInstance: + """Create a complete tool instance with all required relations.""" + user = User( + id=uuid.uuid4(), + email="owner@headquarter.local", + name="Owner", + authentik_id=f"authentik-{uuid.uuid4()}", + avatar_url=None, + ) + db_session.add(user) + await db_session.commit() + + project = Project( + id=uuid.uuid4(), + name="test-project", + description="Test", + owner_id=user.id, + ) + repo = GitRepository( + id=uuid.uuid4(), + name="test-repo", + path="/tmp/test-repo", + project_id=project.id, + owner_id=user.id, + remote_url="https://github.com/test/repo.git", + ) + tool_type = ToolType( + id=uuid.uuid4(), + name="test-tool", + display_name="Test Tool", + category="other", + interface_type="web", + requires_port=True, + default_port=8080, + definition_type="legacy", + compose_template="version: '3.8'\nservices:\n app:\n image: alpine\n command: sleep 3600\n", + ) + db_session.add_all([project, repo, tool_type]) + await db_session.commit() + + instance = ToolInstance( + id=uuid.uuid4(), + name="test-instance", + display_name="Test Instance", + tool_type_id=tool_type.id, + repository_id=repo.id, + project_id=project.id, + owner_id=user.id, + status="running", + compose_path="/tmp/test-compose.yml", + port=8080, + ) + db_session.add(instance) + await db_session.commit() + return instance + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_lifecycle_event_creates_notification( + db_session: AsyncSession, + event_bus: InstanceEventBus, + test_instance: ToolInstance, +) -> None: + """Triggering a lifecycle event creates a notification for the instance owner.""" + received: list[InstanceEventPayload] = [] + + def subscriber(payload: InstanceEventPayload) -> None: + received.append(payload) + + event_bus.subscribe("instance.started", subscriber) + + from src.services.lifecycle_hooks import publish_lifecycle_event + + await publish_lifecycle_event( + event_bus=event_bus, + session=db_session, + instance=test_instance, + event_type="instance.started", + status="starting", + message="Container started", + ) + + # Event still published + assert len(received) == 1 + + # Notification created + result = await db_session.execute( + select(Notification).where(Notification.user_id == test_instance.owner_id) + ) + notifications = list(result.scalars().all()) + assert len(notifications) == 1 + n = notifications[0] + assert n.category == "instance" + assert n.severity == "info" + assert n.title == "Container started" + assert n.source_type == "tool_instances" + assert n.source_id == test_instance.id + assert n.message == "Container started" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_health_monitor_error_creates_notification( + db_session: AsyncSession, + event_bus: InstanceEventBus, + test_instance: ToolInstance, +) -> None: + """Simulating a health monitor crash creates an error notification.""" + from src.services.health_monitor import HealthMonitor + + monitor = HealthMonitor(event_bus) + + received: list[InstanceEventPayload] = [] + + def subscriber(payload: InstanceEventPayload) -> None: + received.append(payload) + + event_bus.subscribe("instance.error", subscriber) + + with patch( + "src.services.health_monitor.get_container_status", + return_value={"status": "exited", "exit_code": 137, "health": None}, + ): + await monitor._check_instance(db_session, test_instance) + + # Event published + assert len(received) == 1 + + # Notification created + result = await db_session.execute( + select(Notification).where(Notification.user_id == test_instance.owner_id) + ) + notifications = list(result.scalars().all()) + assert len(notifications) == 1 + n = notifications[0] + assert n.category == "instance" + assert n.severity == "error" + assert n.source_type == "tool_instances" + assert n.source_id == test_instance.id + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_notification_failure_does_not_block_event_pipeline( + db_session: AsyncSession, + event_bus: InstanceEventBus, + test_instance: ToolInstance, +) -> None: + """If NotificationService raises, the event is still published and no exception escapes.""" + received: list[InstanceEventPayload] = [] + + def subscriber(payload: InstanceEventPayload) -> None: + received.append(payload) + + event_bus.subscribe("instance.started", subscriber) + + from src.services.lifecycle_hooks import publish_lifecycle_event + + with patch( + "src.services.lifecycle_hooks.notification_service.create_notification", + side_effect=RuntimeError("DB is down"), + ): + # Should not raise + await publish_lifecycle_event( + event_bus=event_bus, + session=db_session, + instance=test_instance, + event_type="instance.started", + status="starting", + message="Container started", + ) + + assert len(received) == 1 + assert received[0]["event"] == "instance.started" + + # No notification should have been created + result = await db_session.execute( + select(Notification).where(Notification.user_id == test_instance.owner_id) + ) + assert result.scalar_one_or_none() is None + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_notification_ownership_matches_instance_owner( + db_session: AsyncSession, + event_bus: InstanceEventBus, +) -> None: + """Notification user_id matches the instance owner, not any caller.""" + # Create a caller user (simulates the user making an API request) + caller = User( + id=uuid.uuid4(), + email="caller@headquarter.local", + name="Caller", + authentik_id=f"authentik-{uuid.uuid4()}", + avatar_url=None, + ) + db_session.add(caller) + await db_session.commit() + + # Create the actual owner + owner = User( + id=uuid.uuid4(), + email="owner@headquarter.local", + name="Owner", + authentik_id=f"authentik-{uuid.uuid4()}", + avatar_url=None, + ) + db_session.add(owner) + await db_session.commit() + + project = Project( + id=uuid.uuid4(), + name="test-project", + description="Test", + owner_id=owner.id, + ) + repo = GitRepository( + id=uuid.uuid4(), + name="test-repo", + path="/tmp/test-repo", + project_id=project.id, + owner_id=owner.id, + remote_url="https://github.com/test/repo.git", + ) + tool_type = ToolType( + id=uuid.uuid4(), + name="test-tool", + display_name="Test Tool", + category="other", + interface_type="web", + requires_port=True, + default_port=8080, + definition_type="legacy", + compose_template="version: '3.8'\nservices:\n app:\n image: alpine\n command: sleep 3600\n", + ) + db_session.add_all([project, repo, tool_type]) + await db_session.commit() + + instance = ToolInstance( + id=uuid.uuid4(), + name="test-instance", + display_name="Test Instance", + tool_type_id=tool_type.id, + repository_id=repo.id, + project_id=project.id, + owner_id=owner.id, + status="running", + compose_path="/tmp/test-compose.yml", + port=8080, + ) + db_session.add(instance) + await db_session.commit() + + from src.services.lifecycle_hooks import publish_lifecycle_event + + await publish_lifecycle_event( + event_bus=event_bus, + session=db_session, + instance=instance, + event_type="instance.created", + status="pending", + message="Instance created", + ) + + result = await db_session.execute( + select(Notification).where(Notification.source_id == instance.id) + ) + n = result.scalar_one() + assert n.user_id == owner.id + assert n.user_id != caller.id + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_lifecycle_error_creates_error_notification( + db_session: AsyncSession, + event_bus: InstanceEventBus, + test_instance: ToolInstance, +) -> None: + """An instance.error lifecycle event creates a severity=error notification.""" + from src.services.lifecycle_hooks import publish_lifecycle_event + + await publish_lifecycle_event( + event_bus=event_bus, + session=db_session, + instance=test_instance, + event_type="instance.error", + status="error", + message="Container failed", + ) + + result = await db_session.execute( + select(Notification).where(Notification.user_id == test_instance.owner_id) + ) + n = result.scalar_one() + assert n.severity == "error" + assert n.title == "Container error" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_health_monitor_unhealthy_creates_warning_notification( + db_session: AsyncSession, + event_bus: InstanceEventBus, + test_instance: ToolInstance, +) -> None: + """Health monitor marking instance unhealthy creates severity=warning notification.""" + from src.services.health_monitor import HealthMonitor + + monitor = HealthMonitor(event_bus) + monitor._last_known_state[test_instance.id] = HealthSnapshot( + container_status="running", + container_healthy=None, + tunnel_healthy=True, + exit_code=None, + ) + test_instance.public_url = "https://example.trycloudflare.com" + + with ( + patch( + "src.services.health_monitor.get_container_status", + return_value={"status": "running", "exit_code": None, "health": "healthy"}, + ), + patch( + "src.services.health_monitor.check_tunnel_health", + return_value={"healthy": False, "tunnel_status": "error_response"}, + ), + ): + await monitor._check_instance(db_session, test_instance) + + result = await db_session.execute( + select(Notification).where(Notification.user_id == test_instance.owner_id) + ) + n = result.scalar_one() + assert n.category == "health" + assert n.severity == "warning" + assert n.title == "Container unhealthy" diff --git a/openspec/changes/notification-center/apply-pr2.md b/openspec/changes/notification-center/apply-pr2.md new file mode 100644 index 0000000..13e8538 --- /dev/null +++ b/openspec/changes/notification-center/apply-pr2.md @@ -0,0 +1,156 @@ +# PR-2 Apply Report: Backend Integration for Notification Center + +## Status: COMPLETE + +All 5 tasks for PR-2 (NC-PR2-001 through NC-PR2-005) have been implemented, tested, and validated. + +## What Was Implemented + +### NC-PR2-001: Wire lifecycle_hooks.py to NotificationService + +**File:** `apps/api/src/services/lifecycle_hooks.py` + +- Imported `notification_service` singleton from `src.services.notification_service` +- Added `_derive_title(event_type)` helper mapping lifecycle events to human-readable titles: + - `instance.created` → "Container created" + - `instance.started` → "Container started" + - `instance.stopped` → "Container stopped" + - `instance.restarted` → "Container restarted" + - `instance.deleted` → "Container deleted" + - `instance.error` → "Container error" +- After `event_bus.publish(...)`, calls `notification_service.create_notification(...)` with: + - `user_id = instance.owner_id` + - `category = "instance"` + - `severity = "error"` for `instance.error`, `"info"` for all others + - `source_type = "tool_instances"`, `source_id = instance.id` +- Wrapped in `try/except`; logs failure with `correlation_id` and continues +- Event bus publish and audit row insert are unaffected by notification failure + +### NC-PR2-002: Wire health_monitor.py to NotificationService + +**File:** `apps/api/src/services/health_monitor.py` + +- Imported `notification_service` singleton +- After `self._event_bus.publish(event_type, payload)`, calls `notification_service.create_notification(...)` with: + - `user_id = instance.owner_id` + - `category = "instance"` for `new_status == "error"` + - `category = "health"` for `instance.health_changed` + - `severity` mapped: + - `"error"` for crash + - `"warning"` for unhealthy + - `"info"` for recovery (running) + - `title` mapped: + - "Container failed" for error + - "Container unhealthy" for unhealthy + - "Container recovered" for running +- Wrapped in `try/except`; logs failure with `correlation_id` and continues +- Original event bus publish and health check insert are unaffected + +### NC-PR2-003: Extend UserConfig schema for notification preferences + +**File:** `apps/api/src/api/user_config.py` + +- Added `notification_mute_categories: list[str] | None = None` to `UserConfigResponse` +- Added `notification_toast_level: str | None = None` to `UserConfigResponse` +- Added the same fields to `UserConfigUpdate` +- Existing config keys are unaffected; new fields are optional with `None` defaults + +### NC-PR2-004: Event producer integration tests (RED) + +**File:** `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)* + +6 integration tests covering: + +1. `test_lifecycle_event_creates_notification` — lifecycle hook `instance.started` creates `severity="info"` notification for owner +2. `test_health_monitor_error_creates_notification` — simulated crash creates `severity="error"` notification for owner +3. `test_notification_failure_does_not_block_event_pipeline` — mocked `create_notification` raising `RuntimeError`; event still published, no exception escapes +4. `test_notification_ownership_matches_instance_owner` — notification `user_id` equals `instance.owner_id`, not the API caller +5. `test_lifecycle_error_creates_error_notification` — `instance.error` maps to `severity="error"`, title="Container error" +6. `test_health_monitor_unhealthy_creates_warning_notification` — tunnel failure creating `severity="warning"`, category="health" + +### NC-PR2-005: Verify producer tests and clean up (GREEN / REFACTOR) + +- All 6 new integration tests pass +- 13 unit tests for `NotificationService` pass (no regressions) +- 10 integration tests for notifications API pass (no regressions) +- 6 existing health monitor unit tests pass (no regressions) +- 6 existing event integration tests pass (no regressions) +- `ruff check` passes on all modified files + +## TDD Cycle Evidence + +| Cycle | Task | Test File | RED | GREEN | Evidence | +|-------|------|-----------|-----|-------|----------| +| 1 | NC-PR2-004 (producer flow) | `tests/integration/test_notifications_lifecycle.py` | 4 tests written against unwired producers | Wired `lifecycle_hooks.py` and `health_monitor.py` | 4 passed | +| 2 | NC-PR2-004 (severity mapping) | `tests/integration/test_notifications_lifecycle.py` | Added error + warning severity tests | Already green from implementation | 6 passed | +| 3 | NC-PR2-003 (UserConfig schema) | `src/api/user_config.py` | Schema extended with new optional fields | PATCH/GET endpoints validate correctly | Verified manually | +| 4 | NC-PR2-005 (REFACTOR) | All files | — | ruff clean, no regressions across 41 related tests | All pass | + +## Changed Files + +1. `apps/api/src/services/lifecycle_hooks.py` — Wired `NotificationService` after event bus publish +2. `apps/api/src/services/health_monitor.py` — Wired `NotificationService` after state change event publish +3. `apps/api/src/api/user_config.py` — Added `notification_mute_categories` and `notification_toast_level` to Pydantic schemas +4. `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)* — 6 integration tests for event-to-notification flow + +## Test Commands & Exit Codes + +```bash +# New integration tests for event producers (6 tests) +cd apps/api && python -m pytest tests/integration/test_notifications_lifecycle.py -v +# Exit: 0 — 6 passed + +# NotificationService unit tests (no regressions) +cd apps/api && python -m pytest tests/unit/test_notification_service.py -v +# Exit: 0 — 13 passed + +# Notifications API integration tests (no regressions) +cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v +# Exit: 0 — 10 passed + +# Health monitor unit tests (no regressions) +cd apps/api && python -m pytest tests/unit/test_health_monitor.py -v +# Exit: 0 — 6 passed + +# Event integration tests (no regressions) +cd apps/api && python -m pytest tests/integration/test_events.py -v +# Exit: 0 — 6 passed + +# Combined relevant test suite +cd apps/api && python -m pytest \ + tests/unit/test_notification_service.py \ + tests/integration/test_notifications_api.py \ + tests/integration/test_notifications_lifecycle.py \ + tests/unit/test_health_monitor.py \ + tests/integration/test_events.py \ + -v +# Exit: 0 — 41 passed + +# Ruff linting on all modified files +cd apps/api && python -m ruff check \ + src/services/lifecycle_hooks.py \ + src/services/health_monitor.py \ + src/api/user_config.py \ + tests/integration/test_notifications_lifecycle.py +# Exit: 0 — All checks passed +``` + +## Deviations from Design + +None. All mappings and behaviors match the design spec (section 1.3) and task requirements exactly. + +## Surprises / Decisions + +1. **Health monitor `test_health_monitor_unhealthy_creates_warning_notification` required `public_url`:** The health monitor only checks tunnel health when `instance.public_url` is truthy. Without setting it on the test fixture instance, `_derive_status` returned `"running"` instead of `"unhealthy"`, which created an `"info"` notification. Fixed by setting `test_instance.public_url` in the test before calling `_check_instance`. + +2. **Patch target for failure test:** The `test_notification_failure_does_not_block_event_pipeline` patches `src.services.lifecycle_hooks.notification_service.create_notification`. This only works because `lifecycle_hooks.py` imports `notification_service` at module level, making the attribute resolvable by `unittest.mock.patch`. + +3. **No schema migration needed for UserConfig:** Preferences are stored in the existing JSON `config` blob, consistent with the existing pattern (theme, editor, git identity). No Alembic migration required. + +## Risks + +- **None:** All changes are additive. Event producers use `try/except` so notification failures cannot block the event pipeline. No existing test regressions introduced. + +## PR Boundary + +This PR covers PR-2 only (NC-PR2-001 through NC-PR2-005). PR-3 (frontend core) and PR-4 (toast coordination) are out of scope. diff --git a/openspec/changes/notification-center/apply-progress.md b/openspec/changes/notification-center/apply-progress.md index 4dc8909..4a7b9a7 100644 --- a/openspec/changes/notification-center/apply-progress.md +++ b/openspec/changes/notification-center/apply-progress.md @@ -1,6 +1,6 @@ -# Apply Progress: PR-1 Backend Core for Notification Center +# Apply Progress: Notification Center -## TDD Cycle Evidence +## TDD Cycle Evidence (PR-1) | Cycle | Task | Test File | RED | GREEN | Evidence | |-------|------|-----------|-----|-------|----------| @@ -10,8 +10,18 @@ | 4 | NC-PR1-008 (API edge cases) | `tests/integration/test_notifications_api.py` | Already included in cycle 3 | Pagination, 404 ownership, mute categories at API layer | Same 10 tests pass | | 5 | NC-PR1-010 (REFACTOR) | All files | — | ruff clean, no regressions | `ruff check` passes on all new files; existing unit tests 223 passed (4 pre-existing failures unrelated) | +## TDD Cycle Evidence (PR-2) + +| Cycle | Task | Test File | RED | GREEN | Evidence | +|-------|------|-----------|-----|-------|----------| +| 1 | NC-PR2-004 (producer flow) | `tests/integration/test_notifications_lifecycle.py` | 4 tests written against unwired producers | Wired `lifecycle_hooks.py` and `health_monitor.py` | 4 passed | +| 2 | NC-PR2-004 (severity mapping) | `tests/integration/test_notifications_lifecycle.py` | Added error + warning severity tests | Already green from implementation | 6 passed | +| 3 | NC-PR2-003 (UserConfig schema) | `src/api/user_config.py` | Schema extended with new optional fields | PATCH/GET endpoints validate correctly | Verified manually | +| 4 | NC-PR2-005 (REFACTOR) | All files | — | ruff clean, no regressions across 41 related tests | All pass | + ## Completed Tasks +### PR-1: Backend Core - [x] NC-PR1-001: Alembic migration for `notifications` table - [x] NC-PR1-002: SQLAlchemy `Notification` model (`apps/api/src/models/notification.py`) - [x] NC-PR1-003: Export `Notification` in `models/__init__.py` @@ -24,8 +34,16 @@ - [x] NC-PR1-010: Register router in `main.py` + import `Notification` for Alembic - [x] NC-PR1-011: Code quality pass — ruff, test regressions, smoke tests (REFACTOR) +### PR-2: Backend Integration +- [x] NC-PR2-001: Wire `lifecycle_hooks.py` to `NotificationService` +- [x] NC-PR2-002: Wire `health_monitor.py` to `NotificationService` +- [x] NC-PR2-003: Extend `UserConfig` schema for notification preferences +- [x] NC-PR2-004: Event producer integration tests (RED) +- [x] NC-PR2-005: Verify producer tests pass and clean up (GREEN / REFACTOR) + ## Files Changed +### PR-1 Files 1. `apps/api/alembic/versions/2026_05_29_add_notifications_table.py` *(new)* — Alembic migration 2. `apps/api/src/models/notification.py` *(new)* — SQLAlchemy model 3. `apps/api/src/models/__init__.py` — Export `Notification` @@ -37,8 +55,15 @@ 9. `apps/api/tests/integration/test_notifications_api.py` *(new)* — 10 integration tests 10. `apps/api/tests/integration/test_models.py` — Updated expected tables list +### PR-2 Files +11. `apps/api/src/services/lifecycle_hooks.py` — Wired `NotificationService` after event bus publish +12. `apps/api/src/services/health_monitor.py` — Wired `NotificationService` after state change event publish +13. `apps/api/src/api/user_config.py` — Added `notification_mute_categories` and `notification_toast_level` to Pydantic schemas +14. `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)* — 6 integration tests for event-to-notification flow + ## Test Commands & Exit Codes +### PR-1 ```bash # Unit tests for NotificationService (13 tests) cd apps/api && python -m pytest tests/unit/test_notification_service.py -v @@ -48,48 +73,78 @@ cd apps/api && python -m pytest tests/unit/test_notification_service.py -v cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v # Exit: 0 — 10 passed -# Combined new tests -cd apps/api && python -m pytest tests/unit/test_notification_service.py tests/integration/test_notifications_api.py -v -# Exit: 0 — 23 passed - # Existing unit tests (no regressions in our code) cd apps/api && python -m pytest tests/unit/ -v # Exit: 1 — 223 passed, 4 failed (pre-existing failures in test_config.py and test_git_repository_clone_preflight.py) +``` -# Ruff linting on all new/modified files -cd apps/api && python -m ruff check \ - src/models/notification.py \ - src/models/__init__.py \ - src/services/notification_service.py \ - src/api/notifications.py \ - src/api/__init__.py \ - src/main.py \ - alembic/versions/2026_05_29_add_notifications_table.py \ +### PR-2 +```bash +# New integration tests for event producers (6 tests) +cd apps/api && python -m pytest tests/integration/test_notifications_lifecycle.py -v +# Exit: 0 — 6 passed + +# NotificationService unit tests (no regressions) +cd apps/api && python -m pytest tests/unit/test_notification_service.py -v +# Exit: 0 — 13 passed + +# Notifications API integration tests (no regressions) +cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v +# Exit: 0 — 10 passed + +# Health monitor unit tests (no regressions) +cd apps/api && python -m pytest tests/unit/test_health_monitor.py -v +# Exit: 0 — 6 passed + +# Event integration tests (no regressions) +cd apps/api && python -m pytest tests/integration/test_events.py -v +# Exit: 0 — 6 passed + +# Combined relevant test suite (41 tests) +cd apps/api && python -m pytest \ tests/unit/test_notification_service.py \ tests/integration/test_notifications_api.py \ - tests/integration/test_models.py -# Exit: 0 — All checks passed + tests/integration/test_notifications_lifecycle.py \ + tests/unit/test_health_monitor.py \ + tests/integration/test_events.py \ + -v +# Exit: 0 — 41 passed -# Smoke tests -health: 200 -notifications unauth: 401 +# Ruff linting on all PR-2 modified files +cd apps/api && python -m ruff check \ + src/services/lifecycle_hooks.py \ + src/services/health_monitor.py \ + src/api/user_config.py \ + tests/integration/test_notifications_lifecycle.py +# Exit: 0 — All checks passed ``` ## Deviations from Design +### PR-1 - **SQLAlchemy `metadata` column name conflict:** `Base.metadata` is reserved by SQLAlchemy DeclarativeBase. Used `notification_metadata` as the Python attribute name with DB column name `"metadata"`. In the Pydantic response model, used `Field(serialization_alias="metadata")` so the JSON API still exposes `metadata` as specified. - **`created_at` type in Pydantic:** Used `datetime` instead of `str` to leverage FastAPI's automatic ISO serialization. +### PR-2 +- None. All mappings and behaviors match the design spec (section 1.3) and task requirements exactly. + ## Surprises / Decisions +### PR-1 1. **SQLite `func.now()` resolution:** `test_list_notifications_orders_by_created_at_desc` failed because multiple rapid INSERTs got identical timestamps. Fixed by explicitly setting `created_at` offsets in the test after creation. 2. **Pre-existing integration test failures:** ~40 integration tests fail due to missing `asyncpg` module and direct PostgreSQL connection attempts in their custom setup code. These are unrelated to our changes. 3. **Pre-existing `test_models.py` outdated:** The `test_expected_tables_are_registered` assertion had a hardcoded set missing many newer tables. Updated it to include all current tables (including `notifications`). +### PR-2 +1. **Health monitor `test_health_monitor_unhealthy_creates_warning_notification` required `public_url`:** The health monitor only checks tunnel health when `instance.public_url` is truthy. Without setting it on the test fixture instance, `_derive_status` returned `"running"` instead of `"unhealthy"`, which created an `"info"` notification. Fixed by setting `test_instance.public_url` in the test before calling `_check_instance`. +2. **Patch target for failure test:** The `test_notification_failure_does_not_block_event_pipeline` patches `src.services.lifecycle_hooks.notification_service.create_notification`. This only works because `lifecycle_hooks.py` imports `notification_service` at module level, making the attribute resolvable by `unittest.mock.patch`. +3. **No schema migration needed for UserConfig:** Preferences are stored in the existing JSON `config` blob, consistent with the existing pattern (theme, editor, git identity). No Alembic migration required. + ## Remaining Tasks -None — PR-1 is complete. +- [ ] PR-3: Frontend Core (NC-PR3-001 through NC-PR3-012) +- [ ] PR-4: Toast Coordination (NC-PR4-001 through NC-PR4-006) ## PR Boundary -This PR covers PR-1 only (NC-PR1-001 through NC-PR1-011). PR-2 (backend integration) and PR-3/PR-4 (frontend) are out of scope. +This progress covers PR-1 and PR-2. PR-3 (frontend core — NotificationProvider, useNotifications, NotificationCenter, NotificationItem, styles, AppShell integration) and PR-4 (toast coordination — EventToastBridge preferences, settings UI) are out of scope.