fix(api): wire suppression labels, fix commit ordering, harden proxy and CORS (#64)

Closes the remaining correctness findings from the backend audit.

- suppression_labels was stored, exposed via the API and editable in the UI,
  but handle_pull_request_opened never read it: configuring labels had zero
  effect. Now read (case-insensitively) before any session is created.
- The PR comment was posted while the session row was still uncommitted, so a
  commit failure left a public link to a session that never existed. It also
  held a pool connection across ~40s of GitHub I/O. The session is committed
  before anything is announced.
- uvicorn trusted X-Forwarded-* from 127.0.0.1 only, so behind Traefik every
  client shared the proxy IP and all rate limits collapsed into one global
  bucket — a single client could lock everyone out of login.
- The pool was sized 20+10 per worker against a stock PostgreSQL: 4 workers
  could ask for 120 of 100 connections. Now configurable, defaulting to 10+5.
- Unhandled exceptions returned a 500 without CORS headers (the handler runs
  outside the middleware stack), so real errors reached the browser as opaque
  CORS failures.
- get_current_user accepted ?access_token= on every route, leaking JWTs into
  proxy logs and browser history. Split into CurrentUser (header only) and
  StreamUser (header or query), the latter used by the SSE endpoint alone.

363 tests pass, coverage 84%, ruff and mypy clean.

Author: mariuspruvot

CLAUDE.md

@@ -45,8 +45,8 @@ 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)
-- **Flat modules** (identity, installation, webhook, container): `router.py`, `service.py`, `models.py`, `schemas.py`
-- **Container orchestration**: `container` module manages ephemeral Docker lifecycle, credential injection, result relay
+- **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.
+- **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)
 - **Conversation UI**: frontend renders session output as a structured conversation with markdown (react-markdown + remark-gfm), syntax highlighting (shiki with JS regex engine), and diff coloring. Components: `ConversationOutput` (scroll container) -> `MessageBlock` (role dispatch) -> `MarkdownContent` / `CodeBlock`. Data flows as `StreamMessage[]` (structured content blocks) instead of flat text lines.
@@ -57,7 +57,7 @@ infra/
 - **API prefix**: all routes under `/api/v1`
 - **Admin panel**: SQLAdmin at `/admin`, configured in `admin/views.py`
 - **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**: installation module queries `ContainerSession` model directly (inline import in service functions) for session counts and lists. This avoids circular imports while keeping the API surface on the installation router.
+- **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.
 - **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.
@@ -121,11 +121,9 @@ Key additions for production: `ENVIRONMENT=production`, `ADMIN_PASSWORD`, `CORS_
 - **Worktrees and node_modules**: `npm install` must be run in each worktree separately — `node_modules` aren't shared from the main tree
 - **Shallow clone + `gh pr checkout --detach`**: `gh pr checkout` (without `--detach`) fails on `--depth=1` clones because git can't set up tracking branches from shallow refs. Always use `--detach` — containers don't need tracking branches, just files on disk.
 - **Installation IDs in URLs**: frontend routes (`/installations/:id`) use `github_installation_id` (integer, e.g. `123093268`), NOT the internal UUID. All API endpoints (installation AND container session creation) expect the GitHub integer ID. The service layer resolves to internal UUID via `get_installation_by_github_id()`.
-- **Fast-failing container race**: if a container exits before the SSE stream fully drains, the client may disconnect before `mark_completed()` runs, leaving the session stuck as RUNNING with 0 persisted events. The 5-minute cleanup task marks these as TIMEOUT. Root cause: generator cancellation on client disconnect skips the post-stream `mark_completed` call in `_event_stream()`.
-- **Flaky dispatcher tests**: `test_issues_opened_is_ignored_and_logged` and `test_pull_request_closed_is_ignored` fail when run as part of the full suite due to structlog `configure_logging()` state contamination from `create_app()` in earlier tests. They pass in isolation.
 - **Multi-worker background tasks**: with `--workers N`, each uvicorn worker runs its own lifespan (webhook reaper + container cleanup). Both are idempotent: reaper uses atomic row-level claim (`mark_processing`), cleanup suppresses double-stop exceptions.
 - **Run migrations after checkout**: `docker exec helprs-api-1 uv run alembic current` vs `alembic heads` — if they differ, run `make migrate`. Missing columns cause 500s that surface as browser CORS errors (response lacks CORS headers on unhandled exceptions).
-- **API rebuild invalidates tokens**: `docker compose up --build api` may regenerate SECRET_KEY, invalidating all JWTs and refresh cookies. Re-authenticate after API restarts.
+- **API rebuild invalidates tokens**: re-authenticate after an API restart if the deploy changed `SECRET_KEY`. Nothing in the code generates one — `Settings.SECRET_KEY` is required and the app refuses to boot without it.
 - **Coolify `--project-directory`**: Coolify sets `--project-directory` to the repo root, not the compose file location. Relative paths in the compose (`context`, `volumes`) must be relative to the repo root (`./apps/api`, not `../../apps/api`).
 - **Coolify domain persistence**: Domains set in the Coolify UI may be cleared on redeploy/reload. Verify after each deploy. If persistent issues, add Traefik labels directly in the compose.
 - **`.dockerignore` vs `pyproject.toml`**: `apps/api/.dockerignore` excludes `*.md` but `pyproject.toml` references `readme = "README.md"` — `!README.md` exception is required in `.dockerignore` or `uv sync` fails.

apps/api/src/helprs/core/config.py

@@ -46,6 +46,11 @@ class Settings(BaseSettings):
     # Container orchestration
     CONTAINER_TTL_SECONDS: int = 900  # 15 minutes
 
+    # Database pool, per uvicorn worker. workers x (size + overflow) must stay
+    # below the server's max_connections.
+    DB_POOL_SIZE: int = 10
+    DB_MAX_OVERFLOW: int = 5
+
     # Environment
     ENVIRONMENT: str = "development"
 

apps/api/src/helprs/core/database.py

@@ -6,7 +6,13 @@
 from datetime import datetime
 
 from sqlalchemy import DateTime, func
-from sqlalchemy.ext.asyncio import AsyncAttrs, AsyncSession, async_sessionmaker, create_async_engine
+from sqlalchemy.ext.asyncio import (
+    AsyncAttrs,
+    AsyncEngine,
+    AsyncSession,
+    async_sessionmaker,
+    create_async_engine,
+)
 from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
 
 from helprs.core.config import get_settings
@@ -85,15 +91,21 @@ class Base(AsyncAttrs, DeclarativeBase):
     )
 
 
-def create_engine():
-    """Create async SQLAlchemy engine from settings."""
+def create_engine() -> AsyncEngine:
+    """Create the async engine from settings.
+
+    Pool sizing is a per-worker budget: every uvicorn worker builds its own
+    engine, so ``workers x (pool_size + max_overflow)`` must stay under the
+    server's ``max_connections``. The defaults are sized for four workers
+    against a stock PostgreSQL (4 x 15 = 60 of 100).
+    """
     settings = get_settings()
     return create_async_engine(
         settings.DATABASE_URL,
         echo=False,
         pool_pre_ping=True,
-        pool_size=20,
-        max_overflow=10,
+        pool_size=settings.DB_POOL_SIZE,
+        max_overflow=settings.DB_MAX_OVERFLOW,
         pool_recycle=3600,
     )
 

apps/api/src/helprs/core/dependencies.py

@@ -33,33 +33,8 @@ async def get_db(request: Request) -> AsyncGenerator[AsyncSession, None]:
 DbSession = Annotated[AsyncSession, Depends(get_db)]
 
 
-async def get_current_user(
-    request: Request,
-    session: DbSession,
-    settings: GetSettings,
-) -> GitHubUser:
-    """Extract and validate Bearer token, return authenticated GitHubUser.
-
-    Token sources, in priority order:
-
-    1. ``Authorization: Bearer <token>`` header — preferred for all
-       normal API calls (apiFetch sets it).
-    2. ``?access_token=<token>`` query parameter — fallback used by SSE
-       endpoints. ``EventSource`` cannot set custom request headers, so
-       the SSE caller must put the JWT in the URL. The query-param path
-       was added 2026-04-11 alongside the Story 3-3 SSE manual-QA fix.
-       Trade-off: query params land in access logs and browser history,
-       which is acceptable for a 30-min JWT but not ideal — preferred
-       long-term solution is fetch+ReadableStream (deferred).
-    """
-    auth_header = request.headers.get("Authorization")
-    if auth_header and auth_header.startswith("Bearer "):
-        token = auth_header.removeprefix("Bearer ")
-    else:
-        token = request.query_params.get("access_token") or ""
-        if not token:
-            raise UnauthorizedError("Missing or invalid Authorization header")
-
+async def _authenticate(request: Request, session: AsyncSession, settings: Settings, token: str) -> GitHubUser:
+    """Resolve a bearer token to the user it belongs to."""
     try:
         payload = decode_access_token(token, settings.SECRET_KEY)
     except JWTError as e:
@@ -85,6 +60,35 @@ async def get_current_user(
     return user
 
 
+def _bearer_token(request: Request) -> str:
+    auth_header = request.headers.get("Authorization", "")
+    if not auth_header.startswith("Bearer "):
+        raise UnauthorizedError("Missing or invalid Authorization header")
+    return auth_header.removeprefix("Bearer ")
+
+
+async def get_current_user(request: Request, session: DbSession, settings: GetSettings) -> GitHubUser:
+    """Authenticate from the ``Authorization`` header."""
+    return await _authenticate(request, session, settings, _bearer_token(request))
+
+
+async def get_current_user_for_stream(request: Request, session: DbSession, settings: GetSettings) -> GitHubUser:
+    """Authenticate an SSE request, allowing the token in the query string.
+
+    ``EventSource`` cannot set request headers, so a browser opening a stream
+    has nowhere else to put the JWT. The cost is real — query strings land in
+    proxy logs, browser history and ``Referer`` — so this is granted to the
+    streaming endpoints only, never to the whole API.
+    """
+    token = request.headers.get("Authorization", "").removeprefix("Bearer ") or request.query_params.get(
+        "access_token", ""
+    )
+    if not token:
+        raise UnauthorizedError("Missing or invalid Authorization header")
+    return await _authenticate(request, session, settings, token)
+
+
 # Routes take `user: CurrentUser` rather than `user=Depends(get_current_user)`:
 # the Annotated form is a real type for the checker and does not trip B008.
 CurrentUser = Annotated[GitHubUser, Depends(get_current_user)]
+StreamUser = Annotated[GitHubUser, Depends(get_current_user_for_stream)]

apps/api/src/helprs/main.py

@@ -198,10 +198,27 @@ async def _lifespan(app: FastAPI) -> AsyncIterator[None]:
 
 
 async def _handle_unhandled_exception(request: Request, exc: Exception) -> JSONResponse:
+    """Return a 500 the browser can actually read.
+
+    A handler registered for ``Exception`` runs inside Starlette's
+    ServerErrorMiddleware, which sits *outside* the user middleware stack — so
+    this response never passes through CORSMiddleware. Without the header
+    below, an unhandled error reaches the frontend as an opaque CORS failure
+    instead of the real status.
+    """
     logger.exception("unhandled_exception", path=request.url.path)
+
+    headers: dict[str, str] = {}
+    origin = request.headers.get("origin")
+    if origin and origin in get_settings().CORS_ORIGINS:
+        headers["Access-Control-Allow-Origin"] = origin
+        headers["Access-Control-Allow-Credentials"] = "true"
+        headers["Vary"] = "Origin"
+
     return JSONResponse(
         status_code=500,
         content={"error": "internal_error", "message": "An unexpected error occurred"},
+        headers=headers,
     )
 
 

apps/api/src/helprs/modules/container/router.py

@@ -12,7 +12,7 @@
 from fastapi import APIRouter, Request, Response
 from fastapi.responses import StreamingResponse
 
-from helprs.core.dependencies import CurrentUser, DbSession, GetSettings
+from helprs.core.dependencies import CurrentUser, DbSession, GetSettings, StreamUser
 from helprs.core.exceptions import ConflictError, NotFoundError
 from helprs.core.middleware import limiter
 from helprs.core.security import fernet_decrypt
@@ -153,7 +153,7 @@ async def stream_container_output(
     request: Request,
     db: DbSession,
     settings: GetSettings,
-    user: CurrentUser,
+    user: StreamUser,
     offset: int = 0,
 ) -> StreamingResponse:
     """Relay the container's output as Server-Sent Events."""

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

@@ -3,6 +3,8 @@
 import structlog
 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.service import (
     create_installation_from_webhook,
     get_installation_by_github_id,
@@ -15,6 +17,9 @@
 
 logger = structlog.get_logger()
 
+# Skill run for PRs that arrive through a webhook rather than a user choice.
+DEFAULT_SKILL = "challenge-me"
+
 
 def _extract_installation_id(payload: dict) -> int:
     """Extract installation ID from webhook payload, raising ValueError on missing fields."""
@@ -82,28 +87,22 @@ async def handle_installation_unsuspended(payload: dict, session: AsyncSession)
         )
 
 
-async def handle_pull_request_opened(payload: dict, session: AsyncSession) -> None:
-    """Handle pull_request.opened webhook event.
+def _pr_label_names(pr: dict) -> set[str]:
+    """Lowercased label names on the pull request."""
+    return {label["name"].lower() for label in pr.get("labels", []) if isinstance(label, dict) and label.get("name")}
 
-    Creates a container session with the default skill (challenge-me) and posts
-    a PR comment with a link to the session.
-    """
-    from helprs.core.config import get_settings
-    from helprs.modules.container.service import create_session
 
+async def handle_pull_request_opened(payload: dict, session: AsyncSession) -> None:
+    """Create a session for a newly opened PR and announce it in a comment."""
     github_id = _extract_installation_id(payload)
     installation = await get_installation_by_github_id(session, github_id)
     if installation is None:
-        await logger.awarning(
-            "webhook_pr_opened_installation_not_found",
-            github_installation_id=github_id,
-        )
+        await logger.awarning("webhook_pr_opened_installation_not_found", github_installation_id=github_id)
         return
 
     pr = payload.get("pull_request") or {}
     pr_number = pr.get("number")
-    repo = payload.get("repository") or {}
-    repo_full_name = repo.get("full_name")
+    repo_full_name = (payload.get("repository") or {}).get("full_name")
 
     if not pr_number or not repo_full_name:
         await logger.awarning(
@@ -113,42 +112,58 @@ async def handle_pull_request_opened(payload: dict, session: AsyncSession) -> No
         )
         return
 
+    suppressed_by = _pr_label_names(pr) & {label.lower() for label in installation.suppression_labels or []}
+    if suppressed_by:
+        await logger.ainfo(
+            "webhook_pr_opened_suppressed",
+            github_installation_id=github_id,
+            repo=repo_full_name,
+            pr=pr_number,
+            labels=sorted(suppressed_by),
+        )
+        return
+
     cs = await create_session(
         db=session,
         installation_id=installation.id,
         pr_number=pr_number,
         repo_full_name=repo_full_name,
-        skill_name="challenge-me",
+        skill_name=DEFAULT_SKILL,
+    )
+
+    # Commit before announcing. Posting the comment first would risk a public
+    # link to a session row that a later commit failure never created, and it
+    # would hold this connection open across up to ~40s of GitHub I/O.
+    await session.commit()
+
+    await logger.ainfo(
+        "webhook_pr_session_created",
+        session_id=str(cs.id),
+        repo=repo_full_name,
+        pr=pr_number,
     )
 
+    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:
+    """Post the session link on the PR. Best effort: never fails the event."""
     settings = get_settings()
     owner, repo_name = repo_full_name.split("/", 1)
+    session_path = f"/session/{installation.github_installation_id}/{repo_full_name}/{pr_number}"
 
     try:
         token = await mint_installation_token(installation.github_installation_id, settings)
-        session_path = f"/session/{installation.github_installation_id}/{repo_full_name}/{pr_number}"
-        comment_body = (
-            f"**helPRs** session created for this PR.\n\n"
-            f"Skill: `challenge-me` | "
-            f"[Open session]({settings.APP_BASE_URL}{session_path})"
-        )
         await post_pr_comment_with_retry(
             owner=owner,
             repo=repo_name,
             pr_number=pr_number,
-            body=comment_body,
+            body=(
+                f"**helPRs** session created for this PR.\n\n"
+                f"Skill: `{DEFAULT_SKILL}` | "
+                f"[Open session]({settings.APP_BASE_URL}{session_path})"
+            ),
             installation_token=token,
         )
     except Exception:
-        # PR comment is best-effort — never block session creation.
-        await logger.aexception(
-            "webhook_pr_comment_failed",
-            session_id=str(cs.id),
-        )
-
-    await logger.ainfo(
-        "webhook_pr_session_created",
-        session_id=str(cs.id),
-        repo=repo_full_name,
-        pr=pr_number,
-    )
+        await logger.aexception("webhook_pr_comment_failed", session_id=str(session_id))