refactor(webhook): give the module a service layer and a typed payload

The webhook module was the only one without a service layer: the router
*was* the use case. One handler did header validation, JSON decoding, a
type guard, field extraction, log binding, persistence and task scheduling,
then returned an untyped dict with no response_model. None of that could be
tested without an HTTP request carrying a valid HMAC signature.

`parse_delivery` and `record_delivery` now hold those use cases, the router
validates and calls them, and the response is a `WebhookAck` model.

Raw GitHub JSON also stopped travelling into the domain. installation's
`create_installation_from_webhook` indexed the payload itself, which made a
GitHub shape change a KeyError in the middle of a use case -- the one place
in the codebase where a use case parsed an external payload. It is now
`create_installation(session, InstallationPayload)`. The model lives in
installation's own schemas, not webhook's, so the dependency runs
webhook -> installation rather than the other way round.

The three lifecycle handlers (deleted / suspended / unsuspended) were
byte-identical apart from one service call and two log-event names, sitting
directly above `_apply_lifecycle_change`, which already parameterises the
same axis. They share one helper now.

Docs: CLAUDE.md described a layering the webhook module did not have, and
still said Python 3.12 and "per-module mypy overrides".

Author: mariuspruvot

CLAUDE.md

@@ -13,7 +13,7 @@ make typecheck                   # mypy (API only)
 Monorepo with two apps, skills, and shared infra. See [ADR-001](docs/adr-001-claude-code-container-pivot.md) for the pivot decision.
 
 ```
-apps/api/          — FastAPI backend (Python 3.12, uv)
+apps/api/          — FastAPI backend (Python 3.13, uv)
   src/helprs/
     core/          — config, database, dependencies, exceptions, middleware, security
     modules/       — domain modules: identity, installation, webhook, container
@@ -45,7 +45,7 @@ infra/
 ## Key Patterns
 
 - **App factory**: `helprs.main:create_app()` — module-level `_lifespan` owns the engine and background loops via `AsyncExitStack`: each resource registers its cleanup at acquisition, teardown runs LIFO (cancel loops → drain replay tasks → stop containers → clear factory → dispose engine)
-- **Layered modules**: each domain module is `router.py` (thin — validate, call one use case, shape the response) → `service.py` (use cases, no SQL, no HTTP) → `repository.py` (every query, including the soft-delete predicate) → boundary modules for external systems (`github.py`, `anthropic.py`, `docker_client.py`), all returning typed objects rather than dicts. `container` additionally splits `streaming.py` (SSE pipeline) and `cleanup.py` (reaping) out of the service.
+- **Layered modules**: each domain module is `router.py` (thin — validate, call one use case, shape the response) → `service.py` (use cases, no SQL, no HTTP) → `repository.py` (every query, including the soft-delete predicate) → boundary modules for external systems (`github.py`, `anthropic.py`, `docker_client.py`), all returning typed objects rather than dicts. `container` additionally splits `streaming.py` (SSE pipeline) and `cleanup.py` (reaping) out of the service. Every module now has this layering, `webhook` included.
 - **Container orchestration**: `container` module manages ephemeral Docker lifecycle, credential injection, result relay. `finalize_session()` (mark completed → scorecard → PR comment) is deliberately detached from the HTTP request, so a client disconnect cannot leave a session stuck RUNNING.
 - **Skills as agents**: each skill is a self-contained folder with workflow definitions, mounted into containers
 - **SSE passthrough**: backend relays container output to frontend (no AI response generation in backend)
@@ -59,6 +59,10 @@ infra/
 - **Dashboard**: user-facing installation management at `/installations` -- installation list, session history, session replay. Authenticated users redirect from `/` to `/installations`. SQLAdmin remains at `/admin` as superadmin escape hatch.
 - **Cross-module queries**: a module never writes SQL over another module's tables. `container/repository.py` owns every `ContainerSession` query, including the aggregates the identity dashboard and the installation router consume.
 - **Auth on all REST routes**: identity and installation routers use `Depends(get_current_user)`, container router uses it too. The webhook handler bypasses REST routes entirely — it calls `create_session()` directly (DB record only, no container start). Container start happens when the authenticated frontend calls the REST endpoint.
+- **No module `__init__` imports**: the four `modules/*/__init__.py` are docstring-only. Re-exporting a router there pulled the whole router graph back through `core.dependencies` (which imports `identity.models`), so `import helprs.core.dependencies` failed on its own and startup depended on `main.py`'s import order. `tests/test_import_graph.py` guards this.
+- **JWT**: PyJWT, not python-jose (unmaintained since 2021, and the source of an unfixable `ecdsa` advisory). `PyJWTError` is the failure type.
+- **Secrets are `SecretStr`**: read them with `.get_secret_value()`. `SecretStr` defines `__len__`, so truthiness checks work unchanged. `repr(Settings())` used to print every credential, and Sentry uploads locals on any unhandled 500.
+- **SSE takes no DB dependency**: FastAPI tears yield-dependencies down only after the streaming body ends, so `Depends(get_db)` — including one behind an auth dependency — pins a pooled connection for the whole stream. The SSE route calls `authenticate_token`/`stream_token` inside a short `get_db_context()` instead.
 - **Production env validation**: `Settings` has a `model_validator` that enforces non-empty secrets when `ENVIRONMENT=production`. Tests use `ENVIRONMENT=test` to skip this.
 - **Graceful lifecycle**: lifespan reconciles stale RUNNING/PENDING sessions on boot (marks FAILED), and stops all running containers on shutdown. Periodic cleanup uses configurable `CONTAINER_TTL_SECONDS` from settings.
 
@@ -76,9 +80,9 @@ Skills are pluggable Claude Code agent definitions in `skills/`. See `skills/SKI
 
 ## Code Style
 
-- Python: ruff with `line-length = 120`, target Python 3.12
+- Python: ruff with `line-length = 120`, target Python 3.13
 - Lint rules: E, F, I, N, UP, B, A, SIM, TCH
-- mypy: non-strict with pydantic plugin, per-module overrides for third-party lib typing issues (see `pyproject.toml`)
+- mypy: non-strict with pydantic plugin, **no per-module overrides** — the five that existed were hiding four real errors, all since fixed
 - `asyncio_mode = "auto"` in pytest — no need for `@pytest.mark.asyncio`
 
 ## Testing

apps/api/src/helprs/modules/installation/schemas.py

@@ -4,7 +4,7 @@
 import uuid
 from datetime import datetime
 
-from pydantic import BaseModel, field_validator
+from pydantic import BaseModel, ConfigDict, Field, field_validator
 
 
 class BYOKConfigureRequest(BaseModel):
@@ -129,3 +129,32 @@ class PaginatedSessionsResponse(BaseModel):
     page: int
     per_page: int
     total_pages: int
+
+
+# --- Inbound: the GitHub App installation object -----------------------------
+#
+# Modelled here rather than in the webhook module so the dependency runs
+# webhook -> installation. Only the fields helPRs reads are declared; the rest
+# of GitHub's payload is ignored.
+
+
+class InstallationAccount(BaseModel):
+    model_config = ConfigDict(extra="ignore")
+
+    account_id: int = Field(alias="id")
+    login: str
+    account_type: str = Field(alias="type")
+
+
+class InstallationPayload(BaseModel):
+    """The ``installation`` object GitHub sends with App lifecycle events."""
+
+    model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+    github_installation_id: int = Field(alias="id")
+    account: InstallationAccount
+    app_slug: str = ""
+    target_type: str = ""
+    repository_selection: str = "all"
+    permissions: dict[str, str] = Field(default_factory=dict)
+    events: list[str] = Field(default_factory=list)

apps/api/src/helprs/modules/installation/service.py

@@ -24,6 +24,7 @@
 from helprs.modules.installation import anthropic, github, repository
 from helprs.modules.installation.github import RUNNER_TOKEN_PERMISSIONS
 from helprs.modules.installation.models import BYOKConfig, Installation
+from helprs.modules.installation.schemas import InstallationPayload
 
 if TYPE_CHECKING:
     from helprs.modules.container.models import ContainerSession
@@ -38,7 +39,7 @@
 __all__ = [
     "RUNNER_TOKEN_PERMISSIONS",
     "configure_byok",
-    "create_installation_from_webhook",
+    "create_installation",
     "decrypt_byok_key",
     "delete_byok_config",
     "get_byok_config",
@@ -54,6 +55,7 @@
     "verify_admin_permission",
     "verify_installation_access",
     "verify_repo_access",
+    "verify_session_access",
 ]
 
 # Re-exported so callers that only need to post a comment do not have to know
@@ -64,19 +66,18 @@
 # --- Lifecycle -------------------------------------------------------------
 
 
-async def create_installation_from_webhook(session: AsyncSession, webhook_data: dict) -> Installation:
-    """Create an installation from an ``installation.created`` webhook.
+async def create_installation(session: AsyncSession, payload: InstallationPayload) -> Installation:
+    """Create an installation from a parsed ``installation.created`` event.
+
+    Takes a validated model rather than a raw webhook body. Parsing external
+    JSON is a boundary concern: doing it here turned a GitHub shape change
+    into a KeyError in the middle of a use case, and this was the only place
+    in the codebase where a use case read an unparsed payload.
 
     Idempotent: a duplicate delivery, or a concurrent one racing us to the
     unique index, resolves to the existing row.
     """
-    try:
-        inst_data = webhook_data["installation"]
-        account = inst_data["account"]
-        github_installation_id = inst_data["id"]
-    except (KeyError, TypeError) as e:
-        await logger.awarning("webhook_payload_malformed", error=str(e))
-        raise ValueError(f"Malformed webhook payload: missing {e}") from e
+    github_installation_id = payload.github_installation_id
 
     existing = await repository.get_by_github_id(session, github_installation_id)
     if existing:
@@ -87,14 +88,14 @@ async def create_installation_from_webhook(session: AsyncSession, webhook_data:
         session,
         Installation(
             github_installation_id=github_installation_id,
-            account_login=account["login"],
-            account_id=account["id"],
-            account_type=account["type"],
-            repository_selection=inst_data.get("repository_selection", "all"),
-            app_slug=inst_data.get("app_slug", ""),
-            target_type=inst_data.get("target_type", "Organization"),
-            permissions=inst_data.get("permissions"),
-            events=inst_data.get("events"),
+            account_login=payload.account.login,
+            account_id=payload.account.account_id,
+            account_type=payload.account.account_type,
+            repository_selection=payload.repository_selection,
+            app_slug=payload.app_slug,
+            target_type=payload.target_type or "Organization",
+            permissions=payload.permissions,
+            events=payload.events,
             suspended_at=None,
         ),
     )
@@ -108,7 +109,7 @@ async def create_installation_from_webhook(session: AsyncSession, webhook_data:
     await logger.ainfo(
         "installation_created",
         github_installation_id=github_installation_id,
-        account_login=account["login"],
+        account_login=created.account_login,
     )
     return created
 

apps/api/src/helprs/modules/webhook/handlers.py

@@ -1,12 +1,18 @@
 """Webhook event handlers."""
 
+from collections.abc import Awaitable, Callable
+from uuid import UUID
+
 import structlog
+from pydantic import ValidationError
 from sqlalchemy.ext.asyncio import AsyncSession
 
 from helprs.core.config import get_settings
 from helprs.modules.container.service import create_session
+from helprs.modules.installation.models import Installation
+from helprs.modules.installation.schemas import InstallationPayload
 from helprs.modules.installation.service import (
-    create_installation_from_webhook,
+    create_installation,
     get_installation_by_github_id,
     mint_installation_token,
     post_pr_comment_with_retry,
@@ -31,60 +37,72 @@ def _extract_installation_id(payload: dict) -> int:
 
 async def handle_installation_created(payload: dict, session: AsyncSession) -> None:
     """Handle installation.created webhook event."""
-    installation = await create_installation_from_webhook(session, payload)
+    try:
+        parsed = InstallationPayload.model_validate(payload["installation"])
+    except (KeyError, TypeError, ValidationError) as e:
+        raise ValueError(f"Malformed installation payload: {e}") from e
+
+    installation = await create_installation(session, parsed)
     await logger.ainfo(
         "webhook_installation_created",
         installation_id=str(installation.id),
         github_installation_id=installation.github_installation_id,
     )
 
 
-async def handle_installation_deleted(payload: dict, session: AsyncSession) -> None:
-    """Handle installation.deleted webhook event."""
+async def _handle_lifecycle_change(
+    payload: dict,
+    session: AsyncSession,
+    *,
+    apply: Callable[[AsyncSession, int], Awaitable[Installation | None]],
+    applied_event: str,
+    missing_event: str,
+) -> None:
+    """Shared body of the delete/suspend/unsuspend handlers.
+
+    The three differed only in which service call they made and which two log
+    events they emitted, so they were byte-identical otherwise -- and they sit
+    one layer above ``_apply_lifecycle_change``, which already parameterises
+    the same axis in the service.
+    """
     github_id = _extract_installation_id(payload)
-    result = await soft_delete_installation(session, github_id)
-    if result:
-        await logger.ainfo(
-            "webhook_installation_deleted",
-            github_installation_id=github_id,
-        )
+    if await apply(session, github_id):
+        await logger.ainfo(applied_event, github_installation_id=github_id)
     else:
-        await logger.awarning(
-            "webhook_installation_delete_not_found",
-            github_installation_id=github_id,
-        )
+        await logger.awarning(missing_event, github_installation_id=github_id)
+
+
+async def handle_installation_deleted(payload: dict, session: AsyncSession) -> None:
+    """Handle installation.deleted webhook event."""
+    await _handle_lifecycle_change(
+        payload,
+        session,
+        apply=soft_delete_installation,
+        applied_event="webhook_installation_deleted",
+        missing_event="webhook_installation_delete_not_found",
+    )
 
 
 async def handle_installation_suspended(payload: dict, session: AsyncSession) -> None:
     """Handle installation.suspended webhook event."""
-    github_id = _extract_installation_id(payload)
-    result = await suspend_installation(session, github_id)
-    if result:
-        await logger.ainfo(
-            "webhook_installation_suspended",
-            github_installation_id=github_id,
-        )
-    else:
-        await logger.awarning(
-            "webhook_installation_suspend_not_found",
-            github_installation_id=github_id,
-        )
+    await _handle_lifecycle_change(
+        payload,
+        session,
+        apply=suspend_installation,
+        applied_event="webhook_installation_suspended",
+        missing_event="webhook_installation_suspend_not_found",
+    )
 
 
 async def handle_installation_unsuspended(payload: dict, session: AsyncSession) -> None:
     """Handle installation.unsuspended webhook event."""
-    github_id = _extract_installation_id(payload)
-    result = await unsuspend_installation(session, github_id)
-    if result:
-        await logger.ainfo(
-            "webhook_installation_unsuspended",
-            github_installation_id=github_id,
-        )
-    else:
-        await logger.awarning(
-            "webhook_installation_unsuspend_not_found",
-            github_installation_id=github_id,
-        )
+    await _handle_lifecycle_change(
+        payload,
+        session,
+        apply=unsuspend_installation,
+        applied_event="webhook_installation_unsuspended",
+        missing_event="webhook_installation_unsuspend_not_found",
+    )
 
 
 def _pr_label_names(pr: dict) -> set[str]:
@@ -146,7 +164,7 @@ async def handle_pull_request_opened(payload: dict, session: AsyncSession) -> No
     await _announce_session(installation, cs.id, repo_full_name, pr_number)
 
 
-async def _announce_session(installation, session_id, repo_full_name: str, pr_number: int) -> None:
+async def _announce_session(installation: Installation, session_id: UUID, repo_full_name: str, pr_number: int) -> None:
     """Post the session link on the PR. Best effort: never fails the event."""
     settings = get_settings()
     owner, repo_name = repo_full_name.split("/", 1)