diff --git a/.env.example b/.env.example index f8551504..d7d6bde4 100644 --- a/.env.example +++ b/.env.example @@ -77,6 +77,14 @@ STUDIES_DEFAULT_TIMEOUT_S=60 # RELYLOOP_GIT_AUTHOR_NAME=relyloop-bot # RELYLOOP_GIT_AUTHOR_EMAIL=relyloop-bot@your-company.com +# --- feat_github_webhook ----------------------------------------------- +# Cron cadence (in minutes) for the reconcile_pr_state worker, which polls +# GitHub for PR-state changes on `proposals` that webhook delivery missed. +# Default 15 covers the spec's 15-minute SLA. Operators with low PR volume +# may raise this to reduce GitHub API spend. Story 3.1 will narrow valid +# values to a whitelist of cron-expressible cadences. +# RELYLOOP_PR_POLL_MINUTES=15 + # --- Build-time only -------------------------------------------------- # RELYLOOP_GIT_SHA is injected at `docker buildx build` via --build-arg. # `make up` propagates the current short-SHA when invoking compose. diff --git a/CLAUDE.md b/CLAUDE.md index ae80fe78..9880fd42 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -426,7 +426,7 @@ If you slip and a stub leaks into a committed file, capture it as a `bug_` | 5 | [`feat_llm_judgments`](docs/00_overview/implemented_features/2026_05_11_feat_llm_judgments/) | **Complete (PR #35, merged 2026-05-11)** | | 6 | [`feat_digest_proposal`](docs/00_overview/implemented_features/2026_05_11_feat_digest_proposal/) | **Complete (PR #41, merged 2026-05-11)** | | 7 | [`feat_github_pr_worker`](docs/00_overview/implemented_features/2026_05_12_feat_github_pr_worker/) | **Complete (PR #45, merged 2026-05-12)** | -| 8 | [`feat_github_webhook`](docs/02_product/planned_features/feat_github_webhook/) | Spec approved, plan pending | +| 8 | [`feat_github_webhook`](docs/02_product/planned_features/feat_github_webhook/) | Implementation complete (all 10 stories), pending push + CI + merge on `feature/feat-github-webhook` | | 9 | [`feat_studies_ui`](docs/00_overview/implemented_features/2026_05_12_feat_studies_ui/) | **Complete (PR #50, pending merge)** | | 10 | [`feat_chat_agent`](docs/02_product/planned_features/feat_chat_agent/) | Spec approved, plan pending | | 11 | [`feat_proposals_ui`](docs/02_product/planned_features/feat_proposals_ui/) | Spec approved, plan pending | @@ -441,7 +441,7 @@ Run `/pipeline status` for the live view from spec dependencies. | Local dev start/stop | [`docs/03_runbooks/local-dev.md`](docs/03_runbooks/local-dev.md) (lands in `infra_foundation` Story 5.2) | | Test layer convention + 80% coverage gate | [`docs/05_quality/testing.md`](docs/05_quality/testing.md) (lands in `infra_foundation` Story 5.2) | | DB revision mismatch | TBA — lands when `feat_study_lifecycle` ships its first business-table migration | -| GitHub webhook setup | TBA — lands with `feat_github_webhook` | +| GitHub webhook debugging + secret rotation + register_webhook triage | [`docs/03_runbooks/webhook-debugging.md`](docs/03_runbooks/webhook-debugging.md) (`feat_github_webhook`) | | `open_pr` worker debugging + per-repo PAT rotation + closing orphan branches | [`docs/03_runbooks/pr-open-debugging.md`](docs/03_runbooks/pr-open-debugging.md) (`feat_github_pr_worker`) | | GitHub PAT storage / rotation / leak prevention | [`docs/04_security/github-token-handling.md`](docs/04_security/github-token-handling.md) (`feat_github_pr_worker`) | | Local LLM (Ollama / LM Studio / vLLM / TGI) configuration | [`docs/01_architecture/llm-orchestration.md` §"OpenAI-compatible endpoints"](docs/01_architecture/llm-orchestration.md); operator-facing runbook lands with `chore_tutorial_polish` | diff --git a/backend/app/api/v1/config_repos.py b/backend/app/api/v1/config_repos.py index 822b4e8d..fc4f98c1 100644 --- a/backend/app/api/v1/config_repos.py +++ b/backend/app/api/v1/config_repos.py @@ -25,7 +25,7 @@ from typing import Annotated import uuid_utils -from fastapi import APIRouter, Depends, HTTPException, Query, Response, status +from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response, status from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -119,6 +119,7 @@ def _to_detail(row: object) -> ConfigRepoDetail: ) async def create_config_repo_endpoint( body: CreateConfigRepoRequest, + request: Request, db: Annotated[AsyncSession, Depends(get_db)], ) -> ConfigRepoDetail: """Register a new config repo. ``provider`` is server-derived from ``repo_url``. @@ -132,6 +133,11 @@ async def create_config_repo_endpoint( populate the file between registration and first PR-open. 3. ``name`` uniqueness check → 409 ``CONFIG_REPO_NAME_TAKEN`` on collision. 4. Insert with server-derived ``provider="github"``. + 5. **feat_github_webhook Story 4.2** — when ``webhook_secret_ref`` is + populated, best-effort enqueue ``register_webhook`` against the + newly created config_repo id. Enqueue failure (Redis down, pool + absent, transient blip) does NOT break the 201 — it logs WARN + and the operator drives recovery via the runbook. """ try: validate_repo_url(body.repo_url) @@ -183,6 +189,32 @@ async def create_config_repo_endpoint( f"config_repo name {body.name!r} is already registered (concurrent registration race)", False, ) from exc + + # feat_github_webhook Story 4.2 — best-effort enqueue of the + # register_webhook worker. Established pattern from proposals.py:516 / + # studies.py:167 — getattr(request.app.state, "arq_pool", None), not + # a Depends() factory (which doesn't exist in the codebase). + if inserted.webhook_secret_ref is not None: + arq_pool = getattr(request.app.state, "arq_pool", None) + if arq_pool is None: + logger.warning( + "register_webhook_enqueue_skipped_no_pool", + config_repo_id=inserted.id, + ) + else: + try: + await arq_pool.enqueue_job( + "register_webhook", + inserted.id, + _job_id=f"register_webhook:{inserted.id}", + ) + except Exception as exc: # noqa: BLE001 — best-effort enqueue + logger.warning( + "register_webhook_enqueue_failed", + config_repo_id=inserted.id, + exc_type=type(exc).__name__, + ) + return _to_detail(inserted) diff --git a/backend/app/api/webhooks/__init__.py b/backend/app/api/webhooks/__init__.py new file mode 100644 index 00000000..00f9e1a6 --- /dev/null +++ b/backend/app/api/webhooks/__init__.py @@ -0,0 +1,7 @@ +"""Webhook receivers (feat_github_webhook). + +Per ``docs/01_architecture/api-conventions.md`` webhook endpoints +mount unprefixed (no ``/api/v1``) so external providers don't have to +encode an unexpected path component. Same exception as ``/healthz`` +(CLAUDE.md Rule #6 carve-out). +""" diff --git a/backend/app/api/webhooks/github.py b/backend/app/api/webhooks/github.py new file mode 100644 index 00000000..24c259f1 --- /dev/null +++ b/backend/app/api/webhooks/github.py @@ -0,0 +1,208 @@ +"""GitHub webhook receiver (feat_github_webhook Story 2.1 / FR-1). + +Single endpoint ``POST /webhooks/github``. Unprefixed mount per +CLAUDE.md Rule #6 + ``docs/01_architecture/api-conventions.md``. + +Order of operations (per spec FR-1): + +1. Read raw body bytes (HMAC must hash the exact bytes GitHub sent). +2. Extract ``repository.full_name`` and parse to ``(owner, repo)``. +3. Look up the matching ``config_repos`` row. Unknown repo → 403 + ``INVALID_SIGNATURE`` (don't reveal repo enumeration). +4. Read the per-repo HMAC secret from the mounted-secrets bundle + (``webhook_secret_ref``). Missing secret → 403. +5. Verify ``X-Hub-Signature-256`` via constant-time HMAC compare. + Mismatch → 403. +6. Dispatch via the pure-domain ``dispatch_event``; the dispatcher + returns a :class:`WebhookDecision` with ``action ∈ {applied, noop, + ping}`` — it NEVER emits ``unknown_pr`` (router-only). +7. If the decision asks for a mutation, look up the proposal by + ``decision.pr_url``. Missing proposal → override ``wire_action`` to + ``unknown_pr`` and skip the mutation (spec §11 downstream-invariant + audit). Otherwise call the matching ``mark_proposal_pr_*`` repo + function and commit. +8. Emit one structured ``webhook_received`` log line carrying spec + §13 NFR-Operability fields: ``delivery_id``, ``event``, ``action``, + ``proposal_id``, ``result`` (= wire action). + +The webhook secret itself is never logged. The static-grep assertion in +``backend/tests/contract/test_webhook_api_contract.py`` enforces this. + +Re-exports ``WEBHOOK_ACTION_VALUES`` from +``backend.app.domain.git.webhook_dispatch`` so spec §8.4's grep cite at +this path also passes (the wire-action source of truth lives in one +module — the dispatch one — and is consumed from there). +""" + +from __future__ import annotations + +import json +from typing import Annotated, Any + +import structlog +from fastapi import APIRouter, Depends, HTTPException, Request, status +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.db import repo +from backend.app.db.session import get_db +from backend.app.domain.git import ( + WEBHOOK_ACTION_VALUES, + dispatch_event, + parse_repository_full_name, + verify_webhook_signature, +) +from backend.app.git import read_mounted_secret + +logger = structlog.get_logger(__name__) + +router = APIRouter(prefix="/webhooks", tags=["webhooks"]) + +# Re-exported so spec §8.4's grep cite at `backend/app/api/webhooks/github.py` +# also passes (the canonical wire-action source of truth lives in +# `backend.app.domain.git.webhook_dispatch`). +__all__ = ["WEBHOOK_ACTION_VALUES", "router"] + + +def _err(status_code: int, code: str, message: str, retryable: bool) -> HTTPException: + """Build the project-wide error envelope (mirror of every v1 router).""" + return HTTPException( + status_code=status_code, + detail={"error_code": code, "message": message, "retryable": retryable}, + ) + + +def _invalid_signature() -> HTTPException: + return _err( + status.HTTP_403_FORBIDDEN, + "INVALID_SIGNATURE", + "Signature mismatch or unknown repository.", + retryable=False, + ) + + +@router.post("/github", status_code=status.HTTP_200_OK) +async def github_webhook( + request: Request, + db: Annotated[AsyncSession, Depends(get_db)], +) -> dict[str, str]: + """Receive a single GitHub webhook delivery. + + Returns ``{"status": "ok", "action": }`` where + ``wire_action`` is one of the four values in + :data:`WEBHOOK_ACTION_VALUES`. + + Raises: + HTTPException(403, INVALID_SIGNATURE): bad signature or unknown + repository. Both share one error code so the receiver does + not reveal repo enumeration. + """ + delivery_id = request.headers.get("x-github-delivery", "") + event_type = request.headers.get("x-github-event", "") + signature_header = request.headers.get("x-hub-signature-256") + body = await request.body() + + try: + parsed_body: Any = json.loads(body) if body else {} + except json.JSONDecodeError: + # Malformed JSON from a verified-signature payload is unexpected — + # treat as a signature failure (we can't validate intent without + # parseable fields). The HMAC compare would fail anyway because + # the signature was computed against this same body. + logger.warning( + "webhook_invalid_signature", + delivery_id=delivery_id, + gh_event=event_type, + reason="malformed_payload", + ) + raise _invalid_signature() from None + if not isinstance(parsed_body, dict): + # GPT-5.5 final-review F2 — a valid JSON non-object (e.g. ``[]``, + # ``"foo"``, ``null``) would crash payload.get(...) with + # AttributeError → 500. Treat as signature failure. + logger.warning( + "webhook_invalid_signature", + delivery_id=delivery_id, + gh_event=event_type, + reason="non_object_payload", + ) + raise _invalid_signature() + payload: dict[str, Any] = parsed_body + + full_name = "" + repository = payload.get("repository") + if isinstance(repository, dict): + candidate = repository.get("full_name") + if isinstance(candidate, str): + full_name = candidate + owner_repo = parse_repository_full_name(full_name) if full_name else None + if owner_repo is None: + logger.warning( + "webhook_invalid_signature", + delivery_id=delivery_id, + gh_event=event_type, + reason="unparseable_repository", + ) + raise _invalid_signature() + + config_repo_row = await repo.lookup_config_repo_by_owner_repo(db, *owner_repo) + if config_repo_row is None or not config_repo_row.webhook_secret_ref: + logger.warning( + "webhook_invalid_signature", + delivery_id=delivery_id, + gh_event=event_type, + reason="unknown_repo", + ) + raise _invalid_signature() + + secret = read_mounted_secret(config_repo_row.webhook_secret_ref) + if not secret or not verify_webhook_signature(body, signature_header, secret): + logger.warning( + "webhook_invalid_signature", + delivery_id=delivery_id, + gh_event=event_type, + reason="bad_signature", + ) + raise _invalid_signature() + + decision = dispatch_event(event_type, payload) + wire_action: str = decision.action + proposal_id: str | None = None + + if decision.mutation != "none": + # The dispatcher never emits unknown_pr — only the router does, + # after the lookup miss. See spec §11 + the dispatcher's + # `_NOOP`/`_PING` carve-out. + assert decision.pr_url is not None # noqa: S101 — invariant of dispatcher + proposal_row = await repo.lookup_proposal_by_pr_url(db, decision.pr_url) + if proposal_row is None: + wire_action = "unknown_pr" + else: + proposal_id = proposal_row.id + if decision.mutation == "merged": + # GPT-5.5 final-review F3 — GitHub eventual-consistency + # can yield merged=true with merged_at missing/null. + # Fall back to closed-state mutation (PR is no longer open) + # rather than crash the receiver; the polling reconciler + # will catch up on the merged_at value on the next tick. + if decision.pr_merged_at is None: + await repo.mark_proposal_pr_closed(db, proposal_id) + else: + await repo.mark_proposal_pr_merged( + db, proposal_id, pr_merged_at=decision.pr_merged_at + ) + elif decision.mutation == "closed": + await repo.mark_proposal_pr_closed(db, proposal_id) + elif decision.mutation == "reopened": + await repo.mark_proposal_pr_reopened(db, proposal_id) + await db.commit() + + logger.info( + "webhook_received", + delivery_id=delivery_id, + gh_event=event_type, + action=wire_action, + proposal_id=proposal_id, + result=wire_action, + ) + + return {"status": "ok", "action": wire_action} diff --git a/backend/app/core/settings.py b/backend/app/core/settings.py index 981e896c..f984fee2 100644 --- a/backend/app/core/settings.py +++ b/backend/app/core/settings.py @@ -29,7 +29,7 @@ from functools import cached_property, lru_cache from pathlib import Path -from pydantic import Field +from pydantic import Field, field_validator from pydantic_settings import BaseSettings, SettingsConfigDict @@ -156,6 +156,39 @@ class Settings(BaseSettings): "the install-domain bot address." ), ) + relyloop_pr_poll_minutes: int = Field( + default=15, + ge=1, + le=1440, + description=( + "Cron cadence for the reconcile_pr_state worker (feat_github_webhook " + "FR-2). MVP1 default 15. Restricted to the whitelist of " + "cron-expressible values: divisors of 60 (1, 2, 3, 4, 5, 6, 10, 12, " + "15, 20, 30, 60) plus multiples of 60 that divide 1440 (120, 180, " + "240, 360, 720, 1440). Values outside this set raise at startup; " + "see backend.workers.pr_reconcile.SUPPORTED_POLL_MINUTES." + ), + ) + + @field_validator("relyloop_pr_poll_minutes") + @classmethod + def _validate_pr_poll_minutes(cls, value: int) -> int: + """Narrow ``relyloop_pr_poll_minutes`` to the cron-expressible whitelist. + + Whitelist lives in :data:`backend.workers.pr_reconcile.SUPPORTED_POLL_MINUTES` + — keeping the validator here means a misconfigured operator sees the + error at boot rather than at the first cron tick. + """ + from backend.workers.pr_reconcile import SUPPORTED_POLL_MINUTES + + if value not in SUPPORTED_POLL_MINUTES: + raise ValueError( + f"RELYLOOP_PR_POLL_MINUTES={value} is not in the supported set " + f"{sorted(SUPPORTED_POLL_MINUTES)}. Pick a divisor of 60 (≤60) or a " + "multiple of 60 that divides 1440 (>60)." + ) + return value + es_heap_size: str = Field( default="512m", description="ES_JAVA_OPTS heap sizing for the elasticsearch+opensearch containers", diff --git a/backend/app/db/repo/__init__.py b/backend/app/db/repo/__init__.py index f8465606..c884fd52 100644 --- a/backend/app/db/repo/__init__.py +++ b/backend/app/db/repo/__init__.py @@ -21,6 +21,8 @@ get_config_repo, get_config_repo_by_name, list_config_repos, + lookup_config_repo_by_owner_repo, + set_webhook_registration_error, ) from backend.app.db.repo.digest import ( create_digest, @@ -52,8 +54,13 @@ create_proposal, get_proposal, list_pending_proposals_for_boot_scan, + list_pr_opened_proposals_for_reconcile, list_proposals_paginated, + lookup_proposal_by_pr_url, + mark_proposal_pr_closed, + mark_proposal_pr_merged, mark_proposal_pr_opened, + mark_proposal_pr_reopened, reject_proposal, set_proposal_pr_open_error, update_proposal_for_digest, @@ -167,4 +174,12 @@ "list_config_repos", "mark_proposal_pr_opened", "set_proposal_pr_open_error", + # feat_github_webhook Story 1.4 (webhook receiver + polling reconciler + auto-register) + "list_pr_opened_proposals_for_reconcile", + "lookup_config_repo_by_owner_repo", + "lookup_proposal_by_pr_url", + "mark_proposal_pr_closed", + "mark_proposal_pr_merged", + "mark_proposal_pr_reopened", + "set_webhook_registration_error", ] diff --git a/backend/app/db/repo/config_repo.py b/backend/app/db/repo/config_repo.py index bc493c96..9c13087b 100644 --- a/backend/app/db/repo/config_repo.py +++ b/backend/app/db/repo/config_repo.py @@ -12,10 +12,11 @@ from collections.abc import Sequence from datetime import datetime -from sqlalchemy import and_, func, or_, select +from sqlalchemy import and_, func, or_, select, update from sqlalchemy.ext.asyncio import AsyncSession from backend.app.db.models import ConfigRepo +from backend.app.domain.git import UnsupportedProviderError, validate_repo_url async def create_config_repo(db: AsyncSession, **fields: object) -> ConfigRepo: @@ -71,3 +72,63 @@ async def list_config_repos( async def count_config_repos(db: AsyncSession) -> int: """COUNT(*) for the ``X-Total-Count`` header on ``GET /api/v1/config-repos``.""" return int((await db.execute(select(func.count()).select_from(ConfigRepo))).scalar_one()) + + +async def set_webhook_registration_error( + db: AsyncSession, + config_repo_id: str, + error: str | None, +) -> ConfigRepo | None: + """UPDATE the ``webhook_registration_error`` column on a single config_repo. + + feat_github_webhook Story 1.4 — ``register_webhook`` worker calls this + on every failure class (4xx PAT-scope, 422 bad-payload, 5xx + GitHub-down, network) AND with ``error=None`` after a subsequent + successful retry to blank the stale message. + + Returns the updated row or ``None`` if the config_repo doesn't exist. + Caller commits. + """ + stmt = ( + update(ConfigRepo) + .where(ConfigRepo.id == config_repo_id) + .values(webhook_registration_error=error) + .returning(ConfigRepo) + ) + result = await db.execute(stmt) + row = result.scalar_one_or_none() + if row is not None: + await db.flush() + return row + + +async def lookup_config_repo_by_owner_repo( + db: AsyncSession, + owner: str, + repo: str, +) -> ConfigRepo | None: + """Locate a registered config_repo by ``(owner, repo)`` short form. + + feat_github_webhook — consumed by the webhook receiver (Story 2.1), + the polling reconciler (Story 3.1), and the register_webhook worker + (Story 4.1). Canonicalises every candidate row's ``repo_url`` via + :func:`backend.app.domain.git.validate_repo_url` and compares + case-insensitively against the provided ``(owner, repo)`` tuple. + + Returns the matching row or ``None``. Rows whose ``repo_url`` no + longer parses via ``validate_repo_url`` (e.g. historic non-GitHub + URLs from before MVP1 hardening) are skipped silently — they cannot + receive GitHub webhook deliveries by construction. + """ + needle = (owner.lower(), repo.lower()) + # The config_repos table has no soft-delete column (config_repo.py). + # All registered rows are candidates. + rows = (await db.execute(select(ConfigRepo))).scalars().all() + for row in rows: + try: + parsed = validate_repo_url(row.repo_url) + except UnsupportedProviderError: + continue + if (parsed[0].lower(), parsed[1].lower()) == needle: + return row + return None diff --git a/backend/app/db/repo/proposal.py b/backend/app/db/repo/proposal.py index e79af7db..8baab18c 100644 --- a/backend/app/db/repo/proposal.py +++ b/backend/app/db/repo/proposal.py @@ -27,7 +27,7 @@ from __future__ import annotations from collections.abc import Sequence -from datetime import datetime +from datetime import UTC, datetime, timedelta from typing import Any, Literal from sqlalchemy import and_, func, or_, select, update @@ -258,6 +258,138 @@ async def set_proposal_pr_open_error( return row +async def mark_proposal_pr_merged( + db: AsyncSession, + proposal_id: str, + *, + pr_merged_at: datetime, +) -> Proposal | None: + """Conditional UPDATE: pr_opened+open → pr_merged + populate pr_merged_at. + + feat_github_webhook Story 1.4 — webhook receiver + polling reconciler + transition on a successful PR merge. ``WHERE status='pr_opened' AND + pr_state='open'``: if the proposal was already merged via the other + delivery path (webhook arrived before polling, or vice versa), zero + rows match and we return ``None``. The caller logs benignly and + skips. Caller commits. + """ + stmt = ( + update(Proposal) + .where( + Proposal.id == proposal_id, + Proposal.status == "pr_opened", + Proposal.pr_state == "open", + ) + .values( + status="pr_merged", + pr_state="merged", + pr_merged_at=pr_merged_at, + ) + .returning(Proposal) + ) + result = await db.execute(stmt) + row = result.scalar_one_or_none() + if row is not None: + await db.flush() + return row + + +async def mark_proposal_pr_closed( + db: AsyncSession, + proposal_id: str, +) -> Proposal | None: + """Conditional UPDATE: pr_opened+open → pr_opened+closed (status stays). + + feat_github_webhook Story 1.4 — PR was closed without being merged. + Status STAYS ``pr_opened`` so the operator can re-``open_pr`` (spec §11 + downstream-invariant audit). ``WHERE status='pr_opened' AND + pr_state='open'``: idempotent for repeated closed events; second + delivery matches zero rows and returns ``None``. Caller commits. + """ + stmt = ( + update(Proposal) + .where( + Proposal.id == proposal_id, + Proposal.status == "pr_opened", + Proposal.pr_state == "open", + ) + .values(pr_state="closed") + .returning(Proposal) + ) + result = await db.execute(stmt) + row = result.scalar_one_or_none() + if row is not None: + await db.flush() + return row + + +async def mark_proposal_pr_reopened( + db: AsyncSession, + proposal_id: str, +) -> Proposal | None: + """Conditional UPDATE: pr_opened+closed → pr_opened+open. + + feat_github_webhook Story 1.4 — operator re-opened a previously + closed PR. ``WHERE status='pr_opened' AND pr_state='closed'``: + repeat ``reopened`` events match zero rows and return ``None``. + Caller commits. + """ + stmt = ( + update(Proposal) + .where( + Proposal.id == proposal_id, + Proposal.status == "pr_opened", + Proposal.pr_state == "closed", + ) + .values(pr_state="open") + .returning(Proposal) + ) + result = await db.execute(stmt) + row = result.scalar_one_or_none() + if row is not None: + await db.flush() + return row + + +async def lookup_proposal_by_pr_url( + db: AsyncSession, + pr_url: str, +) -> Proposal | None: + """Single-row SELECT keyed on ``pr_url``. Returns the row or ``None``. + + feat_github_webhook Story 1.4 — webhook receiver maps GitHub's + ``pull_request.html_url`` back to the originating proposal. Uses the + partial index ``proposals_pr_url_idx`` (Story 1.1) so the lookup is + sub-millisecond even at 100K proposals. + """ + stmt = select(Proposal).where(Proposal.pr_url == pr_url) + return (await db.execute(stmt)).scalar_one_or_none() + + +async def list_pr_opened_proposals_for_reconcile( + db: AsyncSession, +) -> Sequence[Proposal]: + """Return ``pr_opened`` + ``open`` proposals newer than 90 days. + + feat_github_webhook Story 1.4 — consumed by ``reconcile_pr_state`` + (Story 3.1). The 90-day window caps polling growth per spec FR-2: + older stale-pr_opened rows are presumed permanently abandoned and + require operator triage. + """ + cutoff = datetime.now(UTC) - timedelta(days=90) + stmt = ( + select(Proposal) + .where( + Proposal.status == "pr_opened", + Proposal.pr_state == "open", + Proposal.pr_url.is_not(None), + Proposal.created_at > cutoff, + ) + .order_by(Proposal.created_at.asc()) + ) + return list((await db.execute(stmt)).scalars().all()) + + async def list_pending_proposals_for_boot_scan(db: AsyncSession) -> list[str]: """Return study_ids of pending proposals lacking a digest (FR-2b). diff --git a/backend/app/domain/git/__init__.py b/backend/app/domain/git/__init__.py index b347dcae..45597e08 100644 --- a/backend/app/domain/git/__init__.py +++ b/backend/app/domain/git/__init__.py @@ -16,18 +16,32 @@ from __future__ import annotations from backend.app.domain.git.redaction import RedactTokensProcessor, redact_token +from backend.app.domain.git.repository_name import parse_repository_full_name from backend.app.domain.git.validation import ( InvalidConfigPathError, UnsupportedProviderError, validate_config_path, validate_repo_url, ) +from backend.app.domain.git.webhook_dispatch import ( + HANDLED_EVENT_TYPES, + WEBHOOK_ACTION_VALUES, + WebhookDecision, + dispatch_event, +) +from backend.app.domain.git.webhook_signature import verify_webhook_signature __all__ = [ + "HANDLED_EVENT_TYPES", "InvalidConfigPathError", "RedactTokensProcessor", "UnsupportedProviderError", + "WEBHOOK_ACTION_VALUES", + "WebhookDecision", + "dispatch_event", + "parse_repository_full_name", "redact_token", "validate_config_path", "validate_repo_url", + "verify_webhook_signature", ] diff --git a/backend/app/domain/git/repository_name.py b/backend/app/domain/git/repository_name.py new file mode 100644 index 00000000..1bf82a7a --- /dev/null +++ b/backend/app/domain/git/repository_name.py @@ -0,0 +1,63 @@ +"""GitHub ``repository.full_name`` parser (feat_github_webhook Story 1.2). + +The GitHub webhook payload's ``repository.full_name`` field is the +canonical ``owner/repo`` short form (no scheme, no host, no ``.git`` +suffix). The webhook receiver pairs this with +:func:`backend.app.domain.git.validation.validate_repo_url` on the +``config_repos.repo_url`` side and compares the two ``(owner, repo)`` +tuples case-insensitively. + +This helper is intentionally tight in scope: it only parses the bare +``owner/repo`` form. HTTPS URLs, SSH URLs, and enterprise-host URLs all +return ``None`` — those forms go through ``validate_repo_url``, not here. +Two parsers, one purpose each, no duplicate URL regex (per the spec FR-1 +normalization rule and the implementation plan's cross-model review F1). +""" + +from __future__ import annotations + +import re + +# Canonical GitHub-handle pattern. GitHub permits alphanumerics + hyphens +# in owner names (no leading/trailing hyphen, no consecutive hyphens at +# the GitHub layer — we accept the looser canonical-handle regex here and +# rely on actual GitHub to reject invalid handles). Repo names accept +# alphanumerics, dots, underscores, and hyphens. The optional ``.git`` +# suffix is stripped before the match. +_FULL_NAME_PATTERN = re.compile(r"^([a-z0-9](?:[a-z0-9-]*[a-z0-9])?)/([a-z0-9._-]+)$") + + +def parse_repository_full_name(value: str) -> tuple[str, str] | None: + """Parse GitHub's ``repository.full_name`` (``owner/repo``) form. + + Args: + value: Expected canonical short form, e.g. ``"octocat/Hello-World"``. + Whitespace and case are normalised. A trailing ``.git`` is + stripped. + + Returns: + ``(owner, repo)`` lowercased on success, or ``None`` for: + * any input containing ``://`` (looks like a URL — use + ``validate_repo_url`` instead), + * any input containing ``:`` (SSH URL form), + * any input with a dot in the owner component (would shadow a + host name), + * malformed input (missing slash, multiple slashes, empty + parts, etc.). + """ + if not value: + return None + candidate = value.strip().lower() + if "://" in candidate or ":" in candidate: + return None + if candidate.endswith(".git"): + candidate = candidate[: -len(".git")] + match = _FULL_NAME_PATTERN.match(candidate) + if match is None: + return None + owner, repo = match.group(1), match.group(2) + if "." in owner: + # Domain-shaped owner — refuse so future enterprise-host inputs + # don't quietly succeed against this short-form parser. + return None + return owner, repo diff --git a/backend/app/domain/git/webhook_dispatch.py b/backend/app/domain/git/webhook_dispatch.py new file mode 100644 index 00000000..15f07569 --- /dev/null +++ b/backend/app/domain/git/webhook_dispatch.py @@ -0,0 +1,125 @@ +"""GitHub webhook event dispatcher (feat_github_webhook Story 1.3). + +Pure-domain decision function: takes ``X-GitHub-Event`` + parsed payload, +returns a :class:`WebhookDecision` describing the mutation the router +should perform. No DB access, no I/O, no async. + +Ownership note (cross-model review F2): the dispatcher NEVER returns +``action="unknown_pr"``. That string is router-owned and only emitted +when the router's ``lookup_proposal_by_pr_url`` returns ``None`` after a +``mutation``-requesting decision. The dispatcher's ``action`` Literal is +narrowed to ``{"applied", "noop", "ping"}`` so static typing enforces +the contract. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime +from typing import Any, Literal + +WEBHOOK_ACTION_VALUES: frozenset[str] = frozenset({"applied", "noop", "unknown_pr", "ping"}) +"""Source-of-truth for the spec §8.4 ``action`` wire values. + +Re-exported by ``backend.app.api.webhooks.github`` so spec §8.4's grep +cite at that path also passes (the router's source contains the symbol). +The full set (including the router-only ``"unknown_pr"``) lives here. +""" + +HANDLED_EVENT_TYPES: frozenset[str] = frozenset({"ping", "pull_request"}) +"""``X-GitHub-Event`` values the dispatcher inspects. Anything else → noop.""" + + +@dataclass(frozen=True) +class WebhookDecision: + """The dispatcher's verdict for a single webhook delivery. + + Attributes: + action: Wire-level ``action`` value the router emits in the + response body. NEVER ``"unknown_pr"`` — that's router-owned + (set after a proposal-lookup miss). + pr_url: The ``pull_request.html_url`` the router should look up, + or ``None`` when no mutation is requested. + pr_merged_at: The merge timestamp from ``pull_request.merged_at``, + populated only when ``mutation == "merged"``. + mutation: Which ``mark_proposal_pr_*`` call the router should + perform. ``"none"`` means no DB work. + """ + + action: Literal["applied", "noop", "ping"] + pr_url: str | None + pr_merged_at: datetime | None + mutation: Literal["merged", "closed", "reopened", "none"] + + +_NOOP = WebhookDecision(action="noop", pr_url=None, pr_merged_at=None, mutation="none") +_PING = WebhookDecision(action="ping", pr_url=None, pr_merged_at=None, mutation="none") + + +def _parse_merged_at(raw: Any) -> datetime | None: + """Best-effort parse of GitHub's ISO-8601 ``merged_at`` timestamp.""" + if not isinstance(raw, str) or not raw: + return None + # GitHub emits ``2026-05-12T11:00:00Z``; Python's fromisoformat handles + # the explicit-offset form natively, and ``Z`` needs the ``+00:00`` swap. + try: + return datetime.fromisoformat(raw.replace("Z", "+00:00")) + except ValueError: + return None + + +def dispatch_event(event_type: str, payload: dict[str, Any]) -> WebhookDecision: + """Decide the next mutation given a verified webhook payload. + + Args: + event_type: ``X-GitHub-Event`` header value. + payload: Parsed JSON body. + + Returns: + A :class:`WebhookDecision`. The router translates + ``mutation != "none"`` + missing proposal into the wire + ``"unknown_pr"`` action. + """ + if event_type == "ping": + return _PING + + if event_type != "pull_request": + # Unknown/unhandled event types: log + noop (forward-compatible). + return _NOOP + + action = payload.get("action") + pull_request = payload.get("pull_request") or {} + pr_url = pull_request.get("html_url") if isinstance(pull_request, dict) else None + if not isinstance(pr_url, str) or not pr_url: + # PR-shaped event without a usable URL → cannot mutate. + return _NOOP + + if action == "closed": + if pull_request.get("merged") is True: + merged_at = _parse_merged_at(pull_request.get("merged_at")) + return WebhookDecision( + action="applied", + pr_url=pr_url, + pr_merged_at=merged_at, + mutation="merged", + ) + # closed-without-merge: keep proposals.status='pr_opened' so the + # operator can re-open_pr (spec §11 downstream-invariant audit). + return WebhookDecision( + action="applied", + pr_url=pr_url, + pr_merged_at=None, + mutation="closed", + ) + + if action == "reopened": + return WebhookDecision( + action="applied", + pr_url=pr_url, + pr_merged_at=None, + mutation="reopened", + ) + + # opened / edited / synchronize / review_requested / assigned / ...: + # log + 200 with action=noop. We never want GitHub retrying these. + return _NOOP diff --git a/backend/app/domain/git/webhook_signature.py b/backend/app/domain/git/webhook_signature.py new file mode 100644 index 00000000..49f4e7ef --- /dev/null +++ b/backend/app/domain/git/webhook_signature.py @@ -0,0 +1,54 @@ +"""GitHub webhook HMAC-SHA256 signature verification (feat_github_webhook Story 1.2). + +Pure-domain helper consumed by ``backend.app.api.webhooks.github`` (Story +2.1). No I/O, no DB; takes the raw request body, the ``X-Hub-Signature-256`` +header value, and the per-repo webhook secret content, and returns a bool. + +Constant-time comparison via :func:`hmac.compare_digest` prevents +length-comparison and partial-equality timing side-channels. +""" + +from __future__ import annotations + +import hmac +from hashlib import sha256 + +_SIGNATURE_PREFIX = "sha256=" + + +def verify_webhook_signature( + body: bytes, + signature_header: str | None, + secret: str, +) -> bool: + """Verify GitHub's ``X-Hub-Signature-256`` HMAC-SHA256 against ``body``. + + Args: + body: The exact request body bytes (must be the unparsed payload — + JSON parsing happens AFTER signature verification). + signature_header: The ``X-Hub-Signature-256`` header value, expected + shape ``"sha256="``. ``None`` returns ``False``. + secret: The per-repo webhook secret content (operator-chosen string + from ``./secrets/{webhook_secret_ref}``). Empty secret returns + ``False`` — we never accept unsigned acceptance, even when no + secret is configured. + + Returns: + ``True`` iff the header is well-formed AND the HMAC-SHA256 digest + of ``body`` under ``secret`` matches the header value. ``False`` on + any of: missing header, missing/malformed ``sha256=`` prefix, + empty secret, or digest mismatch. + """ + if signature_header is None: + return False + if not secret: + return False + if not signature_header.startswith(_SIGNATURE_PREFIX): + return False + provided_hex = signature_header[len(_SIGNATURE_PREFIX) :] + if not provided_hex: + return False + expected_hex = hmac.new(secret.encode("utf-8"), body, sha256).hexdigest() + # `hmac.compare_digest` is constant-time over equal-length inputs and + # short-circuits on length mismatch — exactly the property we want here. + return hmac.compare_digest(provided_hex, expected_hex) diff --git a/backend/app/git/__init__.py b/backend/app/git/__init__.py new file mode 100644 index 00000000..29dec04d --- /dev/null +++ b/backend/app/git/__init__.py @@ -0,0 +1,41 @@ +"""Git provider HTTP clients (feat_github_webhook Story 1.5). + +CLAUDE.md "Repository Structure" reserves ``backend/app/git/`` as the +canonical home for Git provider clients. MVP1 ships only the GitHub +REST helpers extracted from ``backend/workers/git_pr.py``; MVP3 adds +GitLab + Bitbucket alongside (per the canonical release matrix). + +Method-agnostic ``github_request`` (generalised from the POST-only +``_github_post`` that shipped with feat_github_pr_worker) carries the +established retry policy: RequestError + 5xx + 429 with Retry-After + +403 secondary-rate-limit detection. +""" + +from __future__ import annotations + +from backend.app.git.github_client import ( + HTTP_RETRY_BACKOFF_S, + HTTP_RETRY_MAX, + HTTP_TIMEOUT_S, + RATE_LIMIT_CLAMP_S, + body_mentions_rate_limit, + github_request, + is_secondary_rate_limit, + parse_rate_limit_reset, + parse_retry_after, +) +from backend.app.git.secrets import read_mounted_secret, secrets_dir + +__all__ = [ + "HTTP_RETRY_BACKOFF_S", + "HTTP_RETRY_MAX", + "HTTP_TIMEOUT_S", + "RATE_LIMIT_CLAMP_S", + "body_mentions_rate_limit", + "github_request", + "is_secondary_rate_limit", + "parse_rate_limit_reset", + "parse_retry_after", + "read_mounted_secret", + "secrets_dir", +] diff --git a/backend/app/git/github_client.py b/backend/app/git/github_client.py new file mode 100644 index 00000000..7dc588aa --- /dev/null +++ b/backend/app/git/github_client.py @@ -0,0 +1,165 @@ +"""Method-agnostic GitHub REST client (feat_github_webhook Story 1.5). + +Generalises ``backend.workers.git_pr._github_post`` (POST-only, shipped +with feat_github_pr_worker) into ``github_request(client, method, url, +*, json_body=None, token=...)`` so the polling reconciler (``GET +/repos/.../pulls/{n}``) and the register-webhook worker (``GET /hooks`` ++ ``POST /hooks``) share the same retry policy. + +Retry policy (verbatim from the prior `_github_post` implementation): + +* ``httpx.RequestError`` — exponential-backoff retry up to + ``HTTP_RETRY_MAX`` attempts; propagate on budget exhaustion. +* ``5xx`` — exponential-backoff retry. +* ``429`` — honour ``Retry-After`` (clamped at ``RATE_LIMIT_CLAMP_S``). +* ``403`` — secondary rate-limit detection: + - ``Retry-After`` header present → wait that duration. + - ``X-RateLimit-Remaining: 0`` + ``X-RateLimit-Reset`` → wait until reset. + - Body mentions ``"rate limit"`` or ``"abuse"`` → exponential backoff. + - None of the above → terminal (e.g. PAT lacks scope). +* Other 4xx — terminal. + +Tokens are caller-supplied and never logged — the global +``RedactTokensProcessor`` (feat_github_pr_worker Story 1.4) provides the +defense-in-depth backstop, and this module emits no log lines of its +own. +""" + +from __future__ import annotations + +import asyncio +import time +from typing import Any + +import httpx + +HTTP_TIMEOUT_S: float = 30.0 +"""Default per-request timeout for GitHub REST calls.""" + +HTTP_RETRY_MAX: int = 3 +"""Total attempt budget (initial + retries).""" + +HTTP_RETRY_BACKOFF_S: tuple[float, ...] = (1.0, 2.0, 4.0) +"""Exponential-backoff schedule (must align with ``HTTP_RETRY_MAX``).""" + +RATE_LIMIT_CLAMP_S: float = 60.0 +"""Hard cap on honoured ``Retry-After`` / ``X-RateLimit-Reset`` waits.""" + + +def parse_retry_after(response: httpx.Response) -> float: + """Return the ``Retry-After`` header as seconds (default 1.0).""" + raw = response.headers.get("retry-after", "1") + try: + return max(0.0, float(raw)) + except ValueError: + return 1.0 + + +def is_secondary_rate_limit(response: httpx.Response) -> bool: + """True iff a 403 carries the secondary-rate-limit header pair.""" + return ( + response.headers.get("x-ratelimit-remaining") == "0" + and "x-ratelimit-reset" in response.headers + ) + + +def body_mentions_rate_limit(response: httpx.Response) -> bool: + """Conservative body-substring match for GitHub's abuse-detection 403s. + + Some secondary-rate-limit responses carry no headers — only a JSON + body like ``{"message": "You have exceeded a secondary rate limit"}``. + Substring match on the lowercased body; defensive try/except so a + bytes/encoding edge case doesn't crash the retry loop. + """ + try: + text = response.text.lower() + except Exception: # noqa: BLE001 — defensive against bytes/encoding edge + return False + return "rate limit" in text or "abuse" in text + + +def parse_rate_limit_reset(response: httpx.Response) -> float: + """Return ``X-RateLimit-Reset`` as seconds-from-now (default 1.0).""" + raw = response.headers.get("x-ratelimit-reset", "0") + try: + return max(0.0, float(raw) - time.time()) + except ValueError: + return 1.0 + + +async def github_request( + client: httpx.AsyncClient, + method: str, + url: str, + *, + json_body: dict[str, Any] | None = None, + token: str, +) -> httpx.Response: + """Method-agnostic GitHub REST call with the established retry policy. + + Args: + client: Caller-owned ``httpx.AsyncClient`` (so connection + pooling / timeout configuration / mock transports stay in + the caller's hands). + method: HTTP verb (``GET``, ``POST``, ``PATCH``, ...). The case + is normalised to upper. + url: Absolute GitHub API URL. + json_body: Optional JSON body. ``None`` for GETs. + token: GitHub PAT — sent in the ``Authorization`` header. + Caller is responsible for sourcing it from the mounted + secrets bundle. + + Returns: + The final ``httpx.Response``. Caller inspects ``status_code``. + + Raises: + httpx.RequestError: A network error persisted across all retry + attempts. The leading line from the most recent attempt is + re-raised so the caller can include the error class in its + ``pr_open_error`` write. + """ + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + verb = method.upper() + last_response: httpx.Response | None = None + last_exc: Exception | None = None + for attempt in range(HTTP_RETRY_MAX): + try: + response = await client.request(verb, url, json=json_body, headers=headers) + except httpx.RequestError as exc: + last_exc = exc + await asyncio.sleep(HTTP_RETRY_BACKOFF_S[attempt]) + continue + last_response = response + if response.status_code < 400: + return response + if response.status_code >= 500: + await asyncio.sleep(HTTP_RETRY_BACKOFF_S[attempt]) + continue + if response.status_code == 429: + wait = parse_retry_after(response) + await asyncio.sleep(min(wait, RATE_LIMIT_CLAMP_S)) + continue + if response.status_code == 403: + # GitHub's secondary rate limit emits inconsistent signals; + # cover the three observed shapes (header, header-pair, body). + if "retry-after" in response.headers: + wait = parse_retry_after(response) + await asyncio.sleep(min(wait, RATE_LIMIT_CLAMP_S)) + continue + if is_secondary_rate_limit(response): + wait = parse_rate_limit_reset(response) + await asyncio.sleep(min(wait, RATE_LIMIT_CLAMP_S)) + continue + if body_mentions_rate_limit(response): + await asyncio.sleep(HTTP_RETRY_BACKOFF_S[attempt]) + continue + # Other 4xx — terminal. + return response + if last_response is not None: + return last_response + assert last_exc is not None # noqa: S101 — invariant: at least one attempt + raise last_exc diff --git a/backend/app/git/secrets.py b/backend/app/git/secrets.py new file mode 100644 index 00000000..0e2f421d --- /dev/null +++ b/backend/app/git/secrets.py @@ -0,0 +1,63 @@ +"""Mounted-secrets bundle reader (feat_github_webhook Story 1.5 hoist). + +Shared helper for reading per-repo PATs and webhook HMAC secrets from +the mounted bundle at ``./secrets/{name}`` (Compose secrets volume, +CLAUDE.md "Secrets via mounted files"). The webhook router (Story 2.1), +polling reconciler (Story 3.1), and register-webhook worker (Story 4.1) +all consume this — keeping one definition avoids the path-containment +check drifting between call sites. + +Mirrors the contract of ``backend.workers.git_pr._read_pat`` (the inline +copy that shipped with feat_github_pr_worker): + +* Containment check rejects ``../etc/passwd``-style refs. +* Directory at the target path → ``None``. +* ``OSError`` on read → ``None``. +* Empty content → ``None``. + +The ``RELYLOOP_SECRETS_DIR`` env var overrides the default mount root +for test isolation (same convention as ``_read_pat``). +""" + +from __future__ import annotations + +import os +from pathlib import Path + +_DEFAULT_SECRETS_DIR = Path("./secrets") + + +def secrets_dir() -> Path: + """Resolved mounted-secret directory (``RELYLOOP_SECRETS_DIR`` override).""" + override = os.environ.get("RELYLOOP_SECRETS_DIR") + return Path(override) if override else _DEFAULT_SECRETS_DIR + + +def read_mounted_secret(name: str) -> str | None: + """Read ``./secrets/{name}`` content; return ``None`` on any failure. + + Args: + name: A ``config_repos.auth_ref`` (PAT) or ``webhook_secret_ref`` + (HMAC secret). Empty string returns ``None`` immediately. + + Returns: + The trimmed file content, or ``None`` for missing / empty / + directory / OSError / path-escape cases. + """ + if not name: + return None + root = secrets_dir().resolve() + candidate = (root / name).resolve() + try: + candidate.relative_to(root) + except ValueError: + # Escaped the secrets root — refuse silently. Callers log the + # operator-visible error via their own structured-log channel. + return None + if not candidate.is_file(): + return None + try: + content = candidate.read_text().strip() + except OSError: + return None + return content or None diff --git a/backend/app/main.py b/backend/app/main.py index 57d31578..89f48402 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -40,6 +40,7 @@ from backend.app.api.v1 import query_sets as query_sets_router from backend.app.api.v1 import query_templates as query_templates_router from backend.app.api.v1 import studies as studies_router +from backend.app.api.webhooks import github as webhook_github_router from backend.app.core.logging import configure_logging, get_logger from backend.app.core.settings import get_settings from backend.app.llm.capability_check import run_capability_check_background @@ -153,3 +154,4 @@ async def lifespan(_app: FastAPI) -> AsyncIterator[None]: app.include_router(judgments_router.router, prefix="/api/v1") # feat_llm_judgments Epic 3 app.include_router(proposals_router.router, prefix="/api/v1") # feat_digest_proposal Epic 3 app.include_router(config_repos_router.router, prefix="/api/v1") # feat_github_pr_worker Epic 3 +app.include_router(webhook_github_router.router) # feat_github_webhook /webhooks/github diff --git a/backend/tests/contract/test_webhook_api_contract.py b/backend/tests/contract/test_webhook_api_contract.py new file mode 100644 index 00000000..9d0f1e6b --- /dev/null +++ b/backend/tests/contract/test_webhook_api_contract.py @@ -0,0 +1,114 @@ +"""Contract assertions for the GitHub webhook receiver (Story 2.1). + +* The endpoint is registered in the OpenAPI schema under + ``POST /webhooks/github``. +* The router source re-exports ``WEBHOOK_ACTION_VALUES`` so spec §8.4's + grep cite at ``backend/app/api/webhooks/github.py`` passes. +* The only error code raised by the router is ``INVALID_SIGNATURE`` + (negative grep against the spec §8.5 catalog — no PROPOSAL_NOT_FOUND, + CONFIG_REPO_NOT_FOUND, INVALID_STATE_TRANSITION, etc., leak through). +* No log call site in the router accepts the webhook secret value + (negative grep — defense-in-depth against future log-line drift). +""" + +from __future__ import annotations + +import re +from collections.abc import AsyncIterator +from pathlib import Path + +import httpx +import pytest +import pytest_asyncio +from asgi_lifespan import LifespanManager + +from backend.tests.conftest import postgres_reachable + +_skip_if_no_pg = pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — webhook router resolves get_db dependency at boot", +) + + +_ROUTER_SOURCE = Path(__file__).resolve().parents[2] / "app" / "api" / "webhooks" / "github.py" + + +@pytest_asyncio.fixture +async def async_client() -> AsyncIterator[httpx.AsyncClient]: + from backend.app.main import app + + async with LifespanManager(app): + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://test", + timeout=10.0, + ) as client: + yield client + + +@_skip_if_no_pg +async def test_webhook_endpoint_registered(async_client: httpx.AsyncClient) -> None: + """``POST /webhooks/github`` appears in the OpenAPI schema.""" + response = await async_client.get("/openapi.json") + schema = response.json() + assert "/webhooks/github" in schema["paths"], ( + f"webhook endpoint missing from OpenAPI; got paths={list(schema['paths'])[:10]}" + ) + assert "post" in schema["paths"]["/webhooks/github"] + + +def test_router_source_re_exports_action_values() -> None: + """Spec §8.4 grep cite at the router path must find WEBHOOK_ACTION_VALUES.""" + text = _ROUTER_SOURCE.read_text() + assert "WEBHOOK_ACTION_VALUES" in text, ( + "Router source must re-export WEBHOOK_ACTION_VALUES so spec §8.4's grep " + "cite at backend/app/api/webhooks/github.py succeeds." + ) + + +def test_router_source_raises_only_invalid_signature() -> None: + """Negative-grep: the router must NOT raise any non-INVALID_SIGNATURE error_code. + + Asserts only ``INVALID_SIGNATURE`` appears as an ``error_code=`` literal + inside the router source. If a future change adds a different code + (e.g. PROPOSAL_NOT_FOUND), the contract test catches it before merge. + """ + text = _ROUTER_SOURCE.read_text() + other_codes = ( + "PROPOSAL_NOT_FOUND", + "INVALID_STATE_TRANSITION", + "CLUSTER_HAS_NO_CONFIG_REPO", + "GITHUB_NOT_CONFIGURED", + "VALIDATION_ERROR", + "RESOURCE_NOT_FOUND", + "CONFIG_REPO_NOT_FOUND", + "UNSUPPORTED_PROVIDER", + "AUTH_REF_NOT_FOUND", + "QUEUE_UNAVAILABLE", + ) + for code in other_codes: + assert code not in text, ( + f"Router source must not raise {code!r}; only INVALID_SIGNATURE is " + "the spec §8.5 contract for the webhook receiver." + ) + assert "INVALID_SIGNATURE" in text + + +def test_router_source_does_not_log_webhook_secret() -> None: + """Static grep: no log call site references the secret content. + + The router reads the secret via ``read_mounted_secret(...)`` into a + local ``secret`` variable. That variable must never appear inside a + log call. Conservative regex match: any call ``logger.(...)`` + whose argument list contains the bareword ``secret``. + """ + text = _ROUTER_SOURCE.read_text() + log_calls = re.findall(r"logger\.\w+\([^)]*\)", text, flags=re.DOTALL) + for call in log_calls: + # The webhook_secret_ref column NAME is fine to log if it ever became + # useful for debugging — only the secret content is forbidden. The + # check rejects bareword ``secret`` (the local variable), allowing + # ``webhook_secret_ref`` as a substring. + assert not re.search(r"\bsecret\b(?!_ref)", call), ( + f"Log call leaks webhook secret: {call!r}" + ) diff --git a/backend/tests/integration/test_config_repo_repo_webhook.py b/backend/tests/integration/test_config_repo_repo_webhook.py new file mode 100644 index 00000000..bba440ed --- /dev/null +++ b/backend/tests/integration/test_config_repo_repo_webhook.py @@ -0,0 +1,129 @@ +"""Repo unit-of-work tests for feat_github_webhook Story 1.4 (config_repo extensions). + +Exercises the 2 new functions added to :mod:`backend.app.db.repo.config_repo`: + +* :func:`set_webhook_registration_error` — UPDATE column + NULL-clears-on-retry +* :func:`lookup_config_repo_by_owner_repo` — case-insensitive ``(owner, repo)`` lookup +""" + +from __future__ import annotations + +import uuid + +import pytest + +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +async def _seed_config_repo(*, owner: str, name: str, url_suffix: str = "") -> str: + """Insert a config_repo with a deterministic repo_url and return its id. + + The repo name carries a per-test random suffix so the UNIQUE constraint + on config_repos.name doesn't collide across runs. + """ + factory = get_session_factory() + suffix = uuid.uuid4().hex[:8] + async with factory() as db: + cr = await repo.create_config_repo( + db, + id=str(uuid.uuid4()), + name=f"cr-{owner}-{name}-{suffix}", + provider="github", + repo_url=f"https://github.com/{owner}/{name}{url_suffix}", + default_branch="main", + pr_base_branch="main", + auth_ref=f"pat-{suffix}", + webhook_secret_ref=None, + ) + await db.commit() + return cr.id + + +async def test_set_webhook_registration_error_populates_column() -> None: + """Happy path: populate then clear on subsequent successful retry.""" + cr_id = await _seed_config_repo(owner="acme", name="search-configs") + + factory = get_session_factory() + async with factory() as db: + updated = await repo.set_webhook_registration_error( + db, cr_id, "GitHub returned 404 — PAT lacks admin:repo_hook scope" + ) + await db.commit() + assert updated is not None + assert updated.webhook_registration_error == ( + "GitHub returned 404 — PAT lacks admin:repo_hook scope" + ) + + # Clear on successful retry. + async with factory() as db: + cleared = await repo.set_webhook_registration_error(db, cr_id, None) + await db.commit() + assert cleared is not None + assert cleared.webhook_registration_error is None + + +async def test_set_webhook_registration_error_returns_none_on_missing_row() -> None: + """Non-existent config_repo id → None (no exception).""" + factory = get_session_factory() + async with factory() as db: + result = await repo.set_webhook_registration_error( + db, "00000000-0000-0000-0000-000000000000", "anything" + ) + assert result is None + + +async def test_lookup_config_repo_by_owner_repo_happy_path() -> None: + """Direct match on the canonical https URL.""" + cr_id = await _seed_config_repo(owner="acme-search", name="configs-prod") + + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_config_repo_by_owner_repo(db, "acme-search", "configs-prod") + assert row is not None + assert row.id == cr_id + + +async def test_lookup_config_repo_by_owner_repo_matches_dot_git_suffix() -> None: + """``https://github.com/owner/repo.git`` matches ``(owner, repo)``. + + ``validate_repo_url`` strips the ``.git`` suffix during parsing, so + the lookup matches both forms. + """ + cr_id = await _seed_config_repo(owner="example", name="dotgit-repo", url_suffix=".git") + + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_config_repo_by_owner_repo(db, "example", "dotgit-repo") + assert row is not None + assert row.id == cr_id + + +async def test_lookup_config_repo_by_owner_repo_is_case_insensitive() -> None: + """``Octocat/Hello-World`` matches the stored ``octocat/hello-world``.""" + cr_id = await _seed_config_repo(owner="OctoCat", name="Hello-World") + + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_config_repo_by_owner_repo(db, "octocat", "hello-world") + assert row is not None + assert row.id == cr_id + + +async def test_lookup_config_repo_by_owner_repo_returns_none_on_miss() -> None: + """No row whose ``repo_url`` parses to the needle → None.""" + await _seed_config_repo(owner="alpha", name="beta") + + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_config_repo_by_owner_repo(db, "gamma", "delta") + assert row is None diff --git a/backend/tests/integration/test_config_repos_extension.py b/backend/tests/integration/test_config_repos_extension.py new file mode 100644 index 00000000..8d8e5b18 --- /dev/null +++ b/backend/tests/integration/test_config_repos_extension.py @@ -0,0 +1,203 @@ +"""Integration tests for ``POST /api/v1/config-repos`` Story 4.2 extension. + +The existing endpoint behavior (response shape, 201 status, preflight +error codes) is unchanged. This file covers the NEW post-commit +best-effort enqueue of ``register_webhook``: + +* Existing happy-path response shape preserved. +* ``webhook_secret_ref`` populated → ``enqueue_job`` called once. +* ``webhook_secret_ref`` NULL → enqueue NOT called. +* ``app.state.arq_pool`` absent → 201 still returned, WARN logged with + ``register_webhook_enqueue_skipped_no_pool``. +* ``enqueue_job`` raises → 201 still returned, WARN logged with + ``register_webhook_enqueue_failed``. +""" + +from __future__ import annotations + +import logging +import uuid +from collections.abc import AsyncIterator +from pathlib import Path +from unittest.mock import AsyncMock + +import httpx +import pytest +import pytest_asyncio + +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +@pytest_asyncio.fixture +async def secrets_dir(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> AsyncIterator[Path]: + """Provide a writable secrets dir + pre-create auth_ref and webhook_secret_ref.""" + secrets_dir = tmp_path / "secrets" + secrets_dir.mkdir() + monkeypatch.setenv("RELYLOOP_SECRETS_DIR", str(secrets_dir)) + yield secrets_dir + + +def _create_body(*, name: str, with_webhook_secret: bool, secrets_dir: Path) -> dict[str, str]: + """Return a config-repo create payload, optionally with webhook_secret_ref. + + Side effect: writes the referenced secret files into ``secrets_dir`` so + the AUTH_REF_NOT_FOUND preflight passes. + """ + auth_ref = f"pat-{uuid.uuid4().hex[:8]}" + (secrets_dir / auth_ref).write_text("placeholder") + body: dict[str, str] = { + "name": name, + "repo_url": "https://github.com/example/repo", + "auth_ref": auth_ref, + "default_branch": "main", + "pr_base_branch": "main", + } + if with_webhook_secret: + webhook_ref = f"hook-{uuid.uuid4().hex[:8]}" + (secrets_dir / webhook_ref).write_text("hookcontent") + body["webhook_secret_ref"] = webhook_ref + return body + + +async def test_post_config_repo_response_shape_unchanged( + async_client: httpx.AsyncClient, + secrets_dir: Path, +) -> None: + """Happy path: 201 + the documented ConfigRepoDetail fields.""" + body = _create_body( + name=f"cr-shape-{uuid.uuid4().hex[:6]}", + with_webhook_secret=False, + secrets_dir=secrets_dir, + ) + response = await async_client.post("/api/v1/config-repos", json=body) + assert response.status_code == 201, response.text + payload = response.json() + assert payload["name"] == body["name"] + assert payload["provider"] == "github" + assert payload["repo_url"] == body["repo_url"] + assert payload["webhook_secret_ref"] is None + assert payload["webhook_registration_error"] is None + + +async def test_post_config_repo_with_secret_enqueues_register_webhook( + async_client: httpx.AsyncClient, + secrets_dir: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """webhook_secret_ref populated → enqueue_job called with register_webhook.""" + from backend.app.main import app + + enqueue = AsyncMock() + pool_stub = type("PoolStub", (), {"enqueue_job": enqueue})() + monkeypatch.setattr(app.state, "arq_pool", pool_stub, raising=False) + + body = _create_body( + name=f"cr-enqueued-{uuid.uuid4().hex[:6]}", + with_webhook_secret=True, + secrets_dir=secrets_dir, + ) + response = await async_client.post("/api/v1/config-repos", json=body) + assert response.status_code == 201 + + enqueue.assert_awaited_once() + args, kwargs = enqueue.call_args + assert args[0] == "register_webhook" + assert isinstance(args[1], str) + assert kwargs.get("_job_id", "").startswith("register_webhook:") + + +async def test_post_config_repo_without_secret_does_not_enqueue( + async_client: httpx.AsyncClient, + secrets_dir: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """webhook_secret_ref NULL → enqueue NOT called.""" + from backend.app.main import app + + enqueue = AsyncMock() + pool_stub = type("PoolStub", (), {"enqueue_job": enqueue})() + monkeypatch.setattr(app.state, "arq_pool", pool_stub, raising=False) + + body = _create_body( + name=f"cr-no-hook-{uuid.uuid4().hex[:6]}", + with_webhook_secret=False, + secrets_dir=secrets_dir, + ) + response = await async_client.post("/api/v1/config-repos", json=body) + assert response.status_code == 201 + enqueue.assert_not_awaited() + + +async def test_post_config_repo_pool_absent_still_returns_201( + async_client: httpx.AsyncClient, + secrets_dir: Path, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """Pool absent → 201 + WARN with register_webhook_enqueue_skipped_no_pool.""" + from backend.app.main import app + + # Force pool absent. + monkeypatch.delattr(app.state, "arq_pool", raising=False) + + caplog.set_level(logging.WARNING, logger="backend.app.api.v1.config_repos") + body = _create_body( + name=f"cr-no-pool-{uuid.uuid4().hex[:6]}", + with_webhook_secret=True, + secrets_dir=secrets_dir, + ) + response = await async_client.post("/api/v1/config-repos", json=body) + assert response.status_code == 201 + # structlog → stdlib logging; caplog captures the event name in the message body. + log_messages = [record.getMessage() for record in caplog.records] + assert any("register_webhook_enqueue_skipped_no_pool" in msg for msg in log_messages), ( + f"missing skipped-no-pool log; saw: {log_messages}" + ) + + +async def test_post_config_repo_enqueue_raises_still_returns_201( + async_client: httpx.AsyncClient, + secrets_dir: Path, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """enqueue_job raises → 201 still returned + WARN logged.""" + from backend.app.main import app + + async def _raises(*_args: object, **_kwargs: object) -> None: + raise RuntimeError("redis unreachable") + + pool_stub = type("PoolStub", (), {"enqueue_job": _raises})() + monkeypatch.setattr(app.state, "arq_pool", pool_stub, raising=False) + + caplog.set_level(logging.WARNING, logger="backend.app.api.v1.config_repos") + body = _create_body( + name=f"cr-enqueue-raises-{uuid.uuid4().hex[:6]}", + with_webhook_secret=True, + secrets_dir=secrets_dir, + ) + response = await async_client.post("/api/v1/config-repos", json=body) + assert response.status_code == 201 + log_messages = [record.getMessage() for record in caplog.records] + assert any("register_webhook_enqueue_failed" in msg for msg in log_messages), ( + f"missing enqueue-failed log; saw: {log_messages}" + ) + + +def test_endpoint_source_does_not_use_get_arq_pool_factory() -> None: + """Static grep: no Depends(get_arq_pool) — that factory doesn't exist. + + The established pattern is ``getattr(request.app.state, "arq_pool", None)``. + This guard catches future drift if anyone tries to wire a Depends-based + factory that doesn't exist anywhere in the codebase. + """ + source = Path("backend/app/api/v1/config_repos.py").read_text() + assert "get_arq_pool" not in source diff --git a/backend/tests/integration/test_digests_migration.py b/backend/tests/integration/test_digests_migration.py index 157cb4a2..ec0a54da 100644 --- a/backend/tests/integration/test_digests_migration.py +++ b/backend/tests/integration/test_digests_migration.py @@ -181,7 +181,10 @@ def test_upgrade_creates_digests_table(self, restore_head: None) -> None: def test_downgrade_drops_digests_table(self, restore_head: None) -> None: _alembic("upgrade", "head") - _alembic("downgrade", "-1") + # Explicitly target 0004 so this test stays correct as the chain + # extends past 0005 (e.g. feat_github_webhook's 0006 means + # ``downgrade -1`` from head no longer drops digests). + _alembic("downgrade", "0004") engine = create_engine(_sync_database_url(), future=True) try: with engine.connect() as conn: diff --git a/backend/tests/integration/test_migrations.py b/backend/tests/integration/test_migrations.py index 87dfe6c4..12845e94 100644 --- a/backend/tests/integration/test_migrations.py +++ b/backend/tests/integration/test_migrations.py @@ -121,9 +121,9 @@ def test_upgrade_head_creates_alembic_version(self, fresh_db: None) -> None: row = result.fetchone() assert row is not None, "alembic_version table empty after upgrade head" # Baseline is "0001" per migrations/versions/0001_baseline.py. - # Head is "0005" once feat_digest_proposal Story 1.1 lands the - # digests migration on top of 0004_judgments. - assert row[0] == "0005" + # Head is "0006" once feat_github_webhook Story 1.1 lands the + # proposals_pr_url_idx migration on top of 0005_digests. + assert row[0] == "0006" finally: engine.dispose() @@ -138,17 +138,17 @@ def test_round_trip(self, fresh_db: None) -> None: """Downgrade by one revision and re-upgrade returns cleanly to head.""" _alembic("upgrade", "head") _alembic("downgrade", "-1") - # After downgrade -1 from 0005 we land at 0004 (judgments). - # Re-upgrade re-applies 0005 cleanly per CLAUDE.md Absolute Rule #5. + # After downgrade -1 from 0006 we land at 0005 (digests). + # Re-upgrade re-applies 0006 cleanly per CLAUDE.md Absolute Rule #5. _alembic("upgrade", "head") engine = create_engine(_sync_database_url(), future=True) try: with engine.connect() as conn: row = conn.execute(text("SELECT version_num FROM alembic_version")).fetchone() assert row is not None - # Head is "0005" once feat_digest_proposal Story 1.1 lands the - # digests migration on top of 0004_judgments. - assert row[0] == "0005" + # Head is "0006" once feat_github_webhook Story 1.1 lands the + # proposals_pr_url_idx migration on top of 0005_digests. + assert row[0] == "0006" finally: engine.dispose() diff --git a/backend/tests/integration/test_polling_reconciler.py b/backend/tests/integration/test_polling_reconciler.py new file mode 100644 index 00000000..b3d0fa3f --- /dev/null +++ b/backend/tests/integration/test_polling_reconciler.py @@ -0,0 +1,366 @@ +"""Integration tests for ``backend.workers.pr_reconcile.reconcile_pr_state``. + +Mocks GitHub via :class:`httpx.MockTransport` (the codebase's established +pattern). Asserts spec FR-2 acceptance criteria: + +* AC-3 happy path — webhook delivery simulated-missed → next polling + tick reconciles the state (merged / closed-unmerged / still-open). +* AC-3 terminal-error branches — 404 / 401 / 403 / 5xx / + ``RequestError`` after retry budget exhaustion → WARN + skip + no + mutation. +* Spec §10 — 429 short-circuits the remaining proposals for this tick. +* AC-8 — 50 candidate proposals + stubbed 200 responses complete in + <30s (asserted via wall-clock + HTTP-attempt count). +""" + +from __future__ import annotations + +import time +import uuid +from collections.abc import AsyncIterator, Iterator +from pathlib import Path + +import httpx +import pytest +import pytest_asyncio + +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +@pytest.fixture(autouse=True) +def _fast_sleep(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Patch retry-loop sleeps so RequestError-after-budget paths are fast.""" + + async def _instant(_seconds: float) -> None: + return None + + monkeypatch.setattr("backend.app.git.github_client.asyncio.sleep", _instant) + yield + + +@pytest_asyncio.fixture +async def reconcile_env( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> AsyncIterator[dict[str, str]]: + """Seed config_repo + mounted PAT secret with a UNIQUE owner/repo per test. + + ``config_repos`` is intentionally NOT cleaned by the integration + ``_clean_phase2_tables`` fixture (config_repo rows are operator-managed + and outlive individual tests). Two consecutive tests using the same + ``(owner, repo)`` would have multiple ``config_repos`` rows matching + ``lookup_config_repo_by_owner_repo``; the lookup returns the first one + found, whose ``auth_ref`` points at a previous test's ``tmp_path`` + (now gone), surfacing as ``pr_reconcile_pat_missing`` errors. + """ + secrets_dir = tmp_path / "secrets" + secrets_dir.mkdir() + pat_ref = f"reconcile-pat-{uuid.uuid4().hex[:8]}" + (secrets_dir / pat_ref).write_text("ghp_" + "A" * 40 + "\n") + monkeypatch.setenv("RELYLOOP_SECRETS_DIR", str(secrets_dir)) + + suffix = uuid.uuid4().hex[:8] + owner = f"rc-owner-{suffix}" + repo_name = f"rc-repo-{suffix}" + + factory = get_session_factory() + async with factory() as db: + cr = await repo.create_config_repo( + db, + id=str(uuid.uuid4()), + name=f"cr-{suffix}", + provider="github", + repo_url=f"https://github.com/{owner}/{repo_name}", + default_branch="main", + pr_base_branch="main", + auth_ref=pat_ref, + webhook_secret_ref=None, + ) + await db.commit() + cr_id = cr.id + + yield { + "config_repo_id": cr_id, + "pat_ref": pat_ref, + "owner": owner, + "repo": repo_name, + } + + +async def _seed_pr_opened_proposal(*, pr_url: str) -> str: + factory = get_session_factory() + async with factory() as db: + cluster = await repo.create_cluster( + db, + id=str(uuid.uuid4()), + name=f"rc-cluster-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + environment="dev", + base_url="http://stub:9200", + auth_kind="es_basic", + credentials_ref="ref", + ) + template = await repo.create_query_template( + db, + id=str(uuid.uuid4()), + name=f"rc-tmpl-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + body='{"query": {"match_all": {}}}', + declared_params={}, + version=1, + ) + proposal = await repo.create_proposal( + db, + id=str(uuid.uuid4()), + study_id=None, + study_trial_id=None, + cluster_id=cluster.id, + template_id=template.id, + config_diff={}, + metric_delta=None, + status="pending", + ) + await db.commit() + pid = proposal.id + async with factory() as db: + await repo.mark_proposal_pr_opened(db, pid, pr_url=pr_url) + await db.commit() + return pid + + +def _install_mock_transport(monkeypatch: pytest.MonkeyPatch, handler: httpx.MockTransport) -> None: + """Patch ``httpx.AsyncClient`` to use a MockTransport instead of network.""" + import httpx as httpx_module + + original = httpx_module.AsyncClient + + def _factory(*args, **kwargs): # noqa: ANN001, ANN002, ANN003 + kwargs["transport"] = handler + return original(*args, **kwargs) + + monkeypatch.setattr("backend.workers.pr_reconcile.httpx.AsyncClient", _factory) + + +async def test_reconciler_merged_response_transitions_state( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-3 — GitHub returns ``merged=true`` → proposal flips to pr_merged.""" + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, + json={"merged": True, "merged_at": "2026-05-12T10:00:00Z", "state": "closed"}, + ) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + assert summary["reconciled"] >= 1 + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.status == "pr_merged" + assert row.pr_state == "merged" + + +async def test_reconciler_closed_unmerged_response( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """200 with state=closed + merged=false → proposal flips to pr_state=closed.""" + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"merged": False, "state": "closed"}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + assert summary["reconciled"] >= 1 + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.status == "pr_opened" # status STAYS pr_opened per spec §11 + assert row.pr_state == "closed" + + +async def test_reconciler_still_open_is_unchanged( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """200 with state=open → no mutation; counted as unchanged.""" + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"merged": False, "state": "open"}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + assert summary["unchanged"] >= 1 + assert summary["reconciled"] == 0 + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.pr_state == "open" + + +@pytest.mark.parametrize("status", [401, 403, 404, 500, 503]) +async def test_reconciler_terminal_errors_log_and_skip( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + status: int, +) -> None: + """4xx / 5xx after retries → WARN + skip + no mutation.""" + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + def handler(_request: httpx.Request) -> httpx.Response: + # For 5xx the retry budget runs; the helper returns the last response. + return httpx.Response(status, text="error") + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + assert summary["errored"] >= 1 + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.pr_state == "open" # unchanged on error + + +async def test_reconciler_request_error_skips_proposal( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """RequestError after retry budget → WARN + skip + no mutation.""" + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + def handler(_request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("unreachable") + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + assert summary["errored"] >= 1 + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.pr_state == "open" + + +async def test_reconciler_429_short_circuits_tick( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Spec §10 — 429 skips the remaining proposals; next tick retries.""" + # Seed two proposals. + pr1 = f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}/pull/1001" + pr2 = f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}/pull/1002" + pid1 = await _seed_pr_opened_proposal(pr_url=pr1) + pid2 = await _seed_pr_opened_proposal(pr_url=pr2) + + call_count = {"n": 0} + + def handler(_request: httpx.Request) -> httpx.Response: + call_count["n"] += 1 + return httpx.Response(429, headers={"x-ratelimit-reset": "0", "retry-after": "0"}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + summary = await reconcile_pr_state({}) + # The github_request retry loop counts as multiple attempts before returning + # the last 429 → reconciler records rate_limited++ then breaks. + assert summary["rate_limited"] == 1 + # Both proposals exist; second never got mutated. + factory = get_session_factory() + async with factory() as db: + row1 = await repo.get_proposal(db, pid1) + row2 = await repo.get_proposal(db, pid2) + assert row1 is not None and row1.pr_state == "open" + assert row2 is not None and row2.pr_state == "open" + + +async def test_reconciler_handles_50_candidates_under_budget( + reconcile_env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-8 — 50 proposals + 200 responses complete in well under 30s. + + asyncio.sleep is patched to zero (see ``_fast_sleep`` autouse), so this + is a coarse wall-clock guard — anything taking minutes means a regression + (e.g. accidental real-sleep introduction in a future refactor). + """ + seeded_ids: list[str] = [] + for n in range(50): + pr_url = ( + f"https://github.com/{reconcile_env['owner']}/{reconcile_env['repo']}/pull/{2000 + n}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + seeded_ids.append(pid) + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"merged": False, "state": "open"}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.pr_reconcile import reconcile_pr_state + + started = time.monotonic() + summary = await reconcile_pr_state({}) + elapsed = time.monotonic() - started + assert summary["candidates"] >= 50 + assert summary["unchanged"] >= 50 + assert elapsed < 30, f"AC-8 budget breached: {elapsed:.1f}s" diff --git a/backend/tests/integration/test_pr_url_index_migration.py b/backend/tests/integration/test_pr_url_index_migration.py new file mode 100644 index 00000000..24229aaf --- /dev/null +++ b/backend/tests/integration/test_pr_url_index_migration.py @@ -0,0 +1,154 @@ +"""``0006_proposals_pr_url_idx`` migration test (feat_github_webhook Story 1.1). + +Asserts: + +- After ``alembic upgrade head`` the partial B-tree index + ``proposals_pr_url_idx`` exists on ``proposals(pr_url)`` and carries the + documented ``WHERE pr_url IS NOT NULL`` predicate. +- ``alembic downgrade 0005`` removes the index cleanly; the chain stays + forward-compatible with future migrations beyond ``0006``. + +Marked ``@pytest.mark.integration`` and skipped automatically when Postgres +is not host-reachable from the test process (mirrors the pattern in +``test_clusters_migration.py``). +""" + +from __future__ import annotations + +import os +import socket +import subprocess +from collections.abc import Iterator +from pathlib import Path +from urllib.parse import urlparse + +import pytest +from sqlalchemy import create_engine, text + +from backend.app.core.settings import get_settings + +REPO = Path(__file__).resolve().parents[3] + + +def _postgres_reachable() -> bool: + if not os.environ.get("DATABASE_URL_FILE") or not os.environ.get("POSTGRES_PASSWORD_FILE"): + return False + try: + url = get_settings().database_url + except Exception: # noqa: BLE001 — best-effort skip-detector + return False + parsed = urlparse(url) + host = parsed.hostname or "localhost" + port = parsed.port or 5432 + try: + with socket.create_connection((host, port), timeout=1.0): + return True + except (TimeoutError, OSError): + return False + + +pytestmark = pytest.mark.skipif( + not _postgres_reachable(), + reason=( + "Postgres not reachable from this process — see " + "docs/03_runbooks/local-dev.md §'Local-vs-CI test layers'." + ), +) + + +def _alembic(*args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["uv", "run", "alembic", *args], + cwd=REPO, + capture_output=True, + text=True, + check=True, + ) + + +def _sync_database_url() -> str: + return get_settings().database_url.replace("postgresql+asyncpg://", "postgresql://") + + +@pytest.fixture +def restore_head() -> Iterator[None]: + yield + try: + _alembic("upgrade", "head") + except subprocess.CalledProcessError: + pass + + +@pytest.mark.integration +class TestProposalsPrUrlIdxMigration: + """Verify the 0006 migration creates the partial index and round-trips.""" + + def test_upgrade_creates_partial_index(self, restore_head: None) -> None: + """After upgrade head, ``proposals_pr_url_idx`` exists with the partial predicate.""" + _alembic("upgrade", "head") + engine = create_engine(_sync_database_url(), future=True) + try: + with engine.connect() as conn: + # pg_indexes carries the full CREATE INDEX statement in `indexdef`, + # which lets us assert both index presence AND the partial-WHERE clause. + row = conn.execute( + text( + "SELECT indexname, indexdef FROM pg_indexes " + "WHERE schemaname = 'public' " + "AND tablename = 'proposals' " + "AND indexname = 'proposals_pr_url_idx'" + ) + ).fetchone() + assert row is not None, "proposals_pr_url_idx missing after upgrade head" + indexdef = row[1] + assert "pr_url" in indexdef + # Partial-index clause; case can vary across PG versions so lowercase compare. + assert "where (pr_url is not null)" in indexdef.lower() + finally: + engine.dispose() + + def test_downgrade_removes_index(self, restore_head: None) -> None: + """Downgrading to 0005 removes the partial index. + + Uses an explicit target revision (``0005``) rather than ``-1`` so the + test stays correct as later migrations extend the chain past ``0006``. + From any head ≥ ``0006``, ``alembic downgrade 0005`` walks back + through every intermediate migration and lands at ``0005``, where + the index does not yet exist. + """ + _alembic("upgrade", "head") + _alembic("downgrade", "0005") + engine = create_engine(_sync_database_url(), future=True) + try: + with engine.connect() as conn: + rows = conn.execute( + text( + "SELECT indexname FROM pg_indexes " + "WHERE schemaname = 'public' " + "AND tablename = 'proposals' " + "AND indexname = 'proposals_pr_url_idx'" + ) + ).fetchall() + assert rows == [] + finally: + engine.dispose() + + def test_upgrade_downgrade_upgrade_round_trip(self, restore_head: None) -> None: + """Full round-trip per CLAUDE.md Rule #5.""" + _alembic("upgrade", "head") + _alembic("downgrade", "0005") + _alembic("upgrade", "head") + engine = create_engine(_sync_database_url(), future=True) + try: + with engine.connect() as conn: + row = conn.execute( + text( + "SELECT indexname FROM pg_indexes " + "WHERE schemaname = 'public' " + "AND tablename = 'proposals' " + "AND indexname = 'proposals_pr_url_idx'" + ) + ).fetchone() + assert row is not None + finally: + engine.dispose() diff --git a/backend/tests/integration/test_proposal_repo_webhook.py b/backend/tests/integration/test_proposal_repo_webhook.py new file mode 100644 index 00000000..8b343a49 --- /dev/null +++ b/backend/tests/integration/test_proposal_repo_webhook.py @@ -0,0 +1,244 @@ +"""Repo unit-of-work tests for feat_github_webhook Story 1.4 (proposal extensions). + +Exercises the 5 new functions added to :mod:`backend.app.db.repo.proposal`: + +* :func:`mark_proposal_pr_merged` — conditional UPDATE pr_opened+open → pr_merged +* :func:`mark_proposal_pr_closed` — conditional UPDATE pr_opened+open → pr_opened+closed +* :func:`mark_proposal_pr_reopened` — conditional UPDATE pr_opened+closed → pr_opened+open +* :func:`lookup_proposal_by_pr_url` — single-row SELECT keyed on pr_url +* :func:`list_pr_opened_proposals_for_reconcile` — polling-tick candidate list + +All four conditional UPDATEs return ``None`` on a zero-row match so the +caller logs benignly and skips. Mirrors the cycle-3 F4 pattern from +``mark_proposal_pr_opened`` (feat_github_pr_worker Story 1.1). +""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from sqlalchemy import update + +from backend.app.db import repo +from backend.app.db.models import Proposal +from backend.app.db.session import get_session_factory +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +async def _seed_pr_opened_proposal(*, pr_url: str | None = None) -> str: + """Seed FK chain + a pending proposal transitioned to pr_opened. + + Returns the proposal id. Status is ``pr_opened`` and ``pr_state`` + is ``open`` — the input shape for the closed/merged/reopened + transitions. + """ + factory = get_session_factory() + async with factory() as db: + cluster = await repo.create_cluster( + db, + id=str(uuid.uuid4()), + name=f"wh-cluster-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + environment="dev", + base_url="http://stub:9200", + auth_kind="es_basic", + credentials_ref="ref", + ) + template = await repo.create_query_template( + db, + id=str(uuid.uuid4()), + name=f"wh-tmpl-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + body='{"query": {"match_all": {}}}', + declared_params={}, + version=1, + ) + proposal = await repo.create_proposal( + db, + id=str(uuid.uuid4()), + study_id=None, + study_trial_id=None, + cluster_id=cluster.id, + template_id=template.id, + config_diff={}, + metric_delta=None, + status="pending", + ) + await db.commit() + pid = proposal.id + + final_pr_url = pr_url or f"https://github.com/example/repo/pull/{uuid.uuid4().int % 10_000}" + async with factory() as db: + await repo.mark_proposal_pr_opened(db, pid, pr_url=final_pr_url) + await db.commit() + return pid + + +async def test_mark_pr_merged_transitions_status() -> None: + """Happy path: pr_opened+open → pr_merged + populates pr_merged_at.""" + pid = await _seed_pr_opened_proposal() + merged_at = datetime.now(UTC).replace(microsecond=0) + + factory = get_session_factory() + async with factory() as db: + updated = await repo.mark_proposal_pr_merged(db, pid, pr_merged_at=merged_at) + await db.commit() + assert updated is not None + assert updated.status == "pr_merged" + assert updated.pr_state == "merged" + assert updated.pr_merged_at == merged_at + + +async def test_mark_pr_merged_no_ops_on_repeat_delivery() -> None: + """Idempotent on duplicate merge events: second call returns None. + + Once a proposal is in pr_merged, the conditional WHERE doesn't match + and the repo function reports the benign no-op via ``None``. + """ + pid = await _seed_pr_opened_proposal() + merged_at = datetime.now(UTC).replace(microsecond=0) + + factory = get_session_factory() + async with factory() as db: + first = await repo.mark_proposal_pr_merged(db, pid, pr_merged_at=merged_at) + await db.commit() + assert first is not None + + async with factory() as db: + second = await repo.mark_proposal_pr_merged(db, pid, pr_merged_at=merged_at) + await db.commit() + assert second is None + + +async def test_mark_pr_closed_keeps_status_pr_opened() -> None: + """Closed-without-merge: pr_state='closed' but status STAYS 'pr_opened'. + + Spec §11 downstream-invariant audit: the operator can re-open_pr, + so we don't regress status back to pending or forward to pr_merged. + """ + pid = await _seed_pr_opened_proposal() + + factory = get_session_factory() + async with factory() as db: + updated = await repo.mark_proposal_pr_closed(db, pid) + await db.commit() + assert updated is not None + assert updated.status == "pr_opened" + assert updated.pr_state == "closed" + + +async def test_mark_pr_closed_no_ops_on_already_closed() -> None: + """Idempotent: closed→closed second delivery returns None.""" + pid = await _seed_pr_opened_proposal() + + factory = get_session_factory() + async with factory() as db: + await repo.mark_proposal_pr_closed(db, pid) + await db.commit() + + async with factory() as db: + second = await repo.mark_proposal_pr_closed(db, pid) + await db.commit() + assert second is None + + +async def test_mark_pr_reopened_returns_open() -> None: + """closed → open: only matches when current state is closed.""" + pid = await _seed_pr_opened_proposal() + + factory = get_session_factory() + async with factory() as db: + await repo.mark_proposal_pr_closed(db, pid) + await db.commit() + + async with factory() as db: + reopened = await repo.mark_proposal_pr_reopened(db, pid) + await db.commit() + assert reopened is not None + assert reopened.status == "pr_opened" + assert reopened.pr_state == "open" + + +async def test_mark_pr_reopened_no_ops_when_already_open() -> None: + """Re-opening an already-open PR matches zero rows → returns None.""" + pid = await _seed_pr_opened_proposal() + factory = get_session_factory() + async with factory() as db: + result = await repo.mark_proposal_pr_reopened(db, pid) + await db.commit() + assert result is None + + +async def test_lookup_proposal_by_pr_url_returns_match() -> None: + """Happy path: registered pr_url maps back to the proposal id.""" + url = f"https://github.com/example/repo/pull/{uuid.uuid4().int % 10_000}" + pid = await _seed_pr_opened_proposal(pr_url=url) + + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_proposal_by_pr_url(db, url) + assert row is not None + assert row.id == pid + + +async def test_lookup_proposal_by_pr_url_returns_none_on_miss() -> None: + """Unmapped URL → None (drives the router's "unknown_pr" override).""" + factory = get_session_factory() + async with factory() as db: + row = await repo.lookup_proposal_by_pr_url(db, "https://github.com/never/seen/pull/9999") + assert row is None + + +async def test_list_pr_opened_for_reconcile_returns_recent_rows() -> None: + """Returns rows with status=pr_opened, pr_state=open, pr_url set, <90 days.""" + pid = await _seed_pr_opened_proposal() + + factory = get_session_factory() + async with factory() as db: + rows = await repo.list_pr_opened_proposals_for_reconcile(db) + assert any(r.id == pid for r in rows) + + +async def test_list_pr_opened_excludes_pr_merged() -> None: + """Once merged, the proposal drops out of the reconcile candidate list.""" + pid = await _seed_pr_opened_proposal() + + factory = get_session_factory() + async with factory() as db: + await repo.mark_proposal_pr_merged(db, pid, pr_merged_at=datetime.now(UTC)) + await db.commit() + + async with factory() as db: + rows = await repo.list_pr_opened_proposals_for_reconcile(db) + assert all(r.id != pid for r in rows) + + +async def test_list_pr_opened_excludes_rows_older_than_90_days() -> None: + """Hard cap at 90 days — spec FR-2 polling growth ceiling. + + Backdates the seeded proposal's created_at to 91 days ago via a + direct UPDATE (the column is server_default; can't be set via + ``create_proposal``). The row should not appear in the reconcile + result. + """ + pid = await _seed_pr_opened_proposal() + cutoff = datetime.now(UTC) - timedelta(days=91) + + factory = get_session_factory() + async with factory() as db: + await db.execute(update(Proposal).where(Proposal.id == pid).values(created_at=cutoff)) + await db.commit() + + async with factory() as db: + rows = await repo.list_pr_opened_proposals_for_reconcile(db) + assert all(r.id != pid for r in rows) diff --git a/backend/tests/integration/test_register_webhook_worker.py b/backend/tests/integration/test_register_webhook_worker.py new file mode 100644 index 00000000..9767526c --- /dev/null +++ b/backend/tests/integration/test_register_webhook_worker.py @@ -0,0 +1,237 @@ +"""Integration tests for ``backend.workers.register_webhook.register_webhook``. + +Mocks GitHub via :class:`httpx.MockTransport`. Asserts spec FR-3 ACs: + +* AC-6 — happy path: no existing hook → 201 on POST → + ``webhook_registration_error`` is NULL. +* AC-6 dedup — existing hook with matching ``config.url`` → no POST → + error column NULL. +* AC-7 (404) — PAT scope failure → error column populated. +* AC-7 (422) — bad payload from GitHub → error column populated. +* AC-7 (5xx) — transient outage after retry budget → error column populated. +* AC-7 (network) — RequestError after retry budget → error column populated. +""" + +from __future__ import annotations + +import uuid +from collections.abc import AsyncIterator, Iterator +from pathlib import Path + +import httpx +import pytest +import pytest_asyncio + +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +@pytest.fixture(autouse=True) +def _fast_sleep(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + async def _instant(_seconds: float) -> None: + return None + + monkeypatch.setattr("backend.app.git.github_client.asyncio.sleep", _instant) + yield + + +@pytest.fixture(autouse=True) +def _relyloop_base_url(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """The worker needs RELYLOOP_BASE_URL to build the GitHub-facing hook URL.""" + monkeypatch.setenv("RELYLOOP_BASE_URL", "https://relyloop.test") + from backend.app.core.settings import get_settings + + get_settings.cache_clear() + yield + get_settings.cache_clear() + + +@pytest_asyncio.fixture +async def register_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> AsyncIterator[str]: + """Seed config_repo + mounted PAT + webhook secret. Yield config_repo id.""" + secrets_dir = tmp_path / "secrets" + secrets_dir.mkdir() + pat_ref = f"pat-{uuid.uuid4().hex[:8]}" + secret_ref = f"hook-{uuid.uuid4().hex[:8]}" + (secrets_dir / pat_ref).write_text("ghp_" + "B" * 40 + "\n") + (secrets_dir / secret_ref).write_text("secret-content\n") + monkeypatch.setenv("RELYLOOP_SECRETS_DIR", str(secrets_dir)) + + factory = get_session_factory() + async with factory() as db: + cr = await repo.create_config_repo( + db, + id=str(uuid.uuid4()), + name=f"cr-{uuid.uuid4().hex[:8]}", + provider="github", + repo_url="https://github.com/example/configs", + default_branch="main", + pr_base_branch="main", + auth_ref=pat_ref, + webhook_secret_ref=secret_ref, + ) + await db.commit() + cr_id = cr.id + yield cr_id + + +def _install_mock_transport(monkeypatch: pytest.MonkeyPatch, handler: httpx.MockTransport) -> None: + import httpx as httpx_module + + original = httpx_module.AsyncClient + + def _factory(*args, **kwargs): # noqa: ANN001, ANN002, ANN003 + kwargs["transport"] = handler + return original(*args, **kwargs) + + monkeypatch.setattr("backend.workers.register_webhook.httpx.AsyncClient", _factory) + + +async def _read_error_column(config_repo_id: str) -> str | None: + factory = get_session_factory() + async with factory() as db: + row = await repo.get_config_repo(db, config_repo_id) + assert row is not None + return row.webhook_registration_error + + +async def test_register_webhook_happy_path_creates_hook( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-6 — no existing hook → 201 → error column NULL.""" + + def handler(request: httpx.Request) -> httpx.Response: + if request.method == "GET": + return httpx.Response(200, json=[]) + return httpx.Response(201, json={"id": 42}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "created"} + assert await _read_error_column(register_env) is None + + +async def test_register_webhook_dedup_existing( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-6 dedup — existing hook with matching config.url → no POST → NULL error.""" + captured: dict[str, int] = {"posts": 0} + + def handler(request: httpx.Request) -> httpx.Response: + if request.method == "GET": + return httpx.Response( + 200, + json=[ + { + "id": 1, + "config": {"url": "https://relyloop.test/webhooks/github"}, + } + ], + ) + captured["posts"] += 1 + return httpx.Response(201) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "exists"} + assert captured["posts"] == 0 + assert await _read_error_column(register_env) is None + + +async def test_register_webhook_404_populates_error( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-7 — POST returns 404 → error column populated with PAT-scope message.""" + + def handler(request: httpx.Request) -> httpx.Response: + if request.method == "GET": + return httpx.Response(200, json=[]) + return httpx.Response(404, text="Not Found") + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "failed"} + error = await _read_error_column(register_env) + assert error is not None and "admin:repo_hook" in error + + +async def test_register_webhook_422_populates_error( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-7 — POST returns 422 → error column populated with validation message.""" + + def handler(request: httpx.Request) -> httpx.Response: + if request.method == "GET": + return httpx.Response(200, json=[]) + return httpx.Response(422, json={"message": "Validation failed"}) + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "failed"} + error = await _read_error_column(register_env) + assert error is not None and "422" in error + + +async def test_register_webhook_5xx_populates_error( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-7 — POST returns 503 after retries → error column populated.""" + + def handler(request: httpx.Request) -> httpx.Response: + if request.method == "GET": + return httpx.Response(200, json=[]) + return httpx.Response(503, text="server overloaded") + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "failed"} + error = await _read_error_column(register_env) + assert error is not None and "transient" in error.lower() + + +async def test_register_webhook_network_error_populates_error( + register_env: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AC-7 — RequestError after retries → error column populated.""" + + def handler(_request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("unreachable") + + _install_mock_transport(monkeypatch, httpx.MockTransport(handler)) + + from backend.workers.register_webhook import register_webhook + + result = await register_webhook({}, register_env) + assert result == {"status": "failed"} + error = await _read_error_column(register_env) + assert error is not None and "network" in error.lower() diff --git a/backend/tests/integration/test_webhook_github.py b/backend/tests/integration/test_webhook_github.py new file mode 100644 index 00000000..84859265 --- /dev/null +++ b/backend/tests/integration/test_webhook_github.py @@ -0,0 +1,578 @@ +"""Integration tests for ``POST /webhooks/github`` (Story 2.1). + +Covers spec FR-1 ACs: + +* AC-1 — ``pull_request{action=closed, merged=true}`` → ``pr_state="merged"`` +* AC-2 — bad signature OR unknown repo → 403 ``INVALID_SIGNATURE`` +* AC-4 — ``X-GitHub-Event: ping`` → ``{action: ping}`` +* AC-5 — unknown PR URL → ``{action: unknown_pr}`` + +Plus FR-1 matrix branches (closed-without-merge, reopened, noop actions, +unknown event types) and the spec §13 NFR-Operability ``webhook_received`` +log fields. Consolidated into one file (the plan lists nine separate +files for cite-by-name; one file keeps the shared seed/sign helpers +honest and the test runner shells out one process per test anyway). + +Test seam: ``RELYLOOP_SECRETS_DIR`` is overridden to ``tmp_path`` so the +mounted-secret read finds the test-controlled webhook secret file. +""" + +from __future__ import annotations + +import hashlib +import hmac +import json +import logging +import uuid +from collections.abc import AsyncIterator +from datetime import UTC, datetime +from pathlib import Path + +import httpx +import pytest +import pytest_asyncio + +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.tests.conftest import postgres_reachable + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not postgres_reachable(), + reason="Postgres not reachable — see docs/03_runbooks/local-dev.md", + ), +] + + +_WEBHOOK_SECRET = "test-webhook-secret-do-not-leak" + + +def _signature(body: bytes, secret: str = _WEBHOOK_SECRET) -> str: + digest = hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest() + return f"sha256={digest}" + + +@pytest_asyncio.fixture +async def webhook_env( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> AsyncIterator[dict[str, str]]: + """Seed a config_repo + mounted webhook secret with a UNIQUE owner/repo per test. + + ``config_repos`` is intentionally NOT cleaned by the integration + ``_clean_phase2_tables`` fixture. Two tests using the same ``(owner, + repo)`` would have multiple ``config_repos`` rows matching + ``lookup_config_repo_by_owner_repo``; the first match wins and its + ``webhook_secret_ref`` points at a previous test's now-gone + ``tmp_path``, surfacing as 403 INVALID_SIGNATURE for every test + after the first. + """ + secrets_dir = tmp_path / "secrets" + secrets_dir.mkdir() + secret_ref = f"webhook-secret-{uuid.uuid4().hex[:8]}" + (secrets_dir / secret_ref).write_text(_WEBHOOK_SECRET + "\n") + monkeypatch.setenv("RELYLOOP_SECRETS_DIR", str(secrets_dir)) + + suffix = uuid.uuid4().hex[:8] + owner = f"wh-owner-{suffix}" + repo_name = f"wh-repo-{suffix}" + + factory = get_session_factory() + async with factory() as db: + cr = await repo.create_config_repo( + db, + id=str(uuid.uuid4()), + name=f"cr-{suffix}", + provider="github", + repo_url=f"https://github.com/{owner}/{repo_name}", + default_branch="main", + pr_base_branch="main", + auth_ref=f"pat-{suffix}", + webhook_secret_ref=secret_ref, + ) + await db.commit() + config_repo_id = cr.id + + yield { + "config_repo_id": config_repo_id, + "secret_ref": secret_ref, + "owner": owner, + "repo": repo_name, + } + + +async def _seed_pr_opened_proposal(*, pr_url: str) -> str: + factory = get_session_factory() + async with factory() as db: + cluster = await repo.create_cluster( + db, + id=str(uuid.uuid4()), + name=f"wh-cluster-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + environment="dev", + base_url="http://stub:9200", + auth_kind="es_basic", + credentials_ref="ref", + ) + template = await repo.create_query_template( + db, + id=str(uuid.uuid4()), + name=f"wh-tmpl-{uuid.uuid4().hex[:8]}", + engine_type="elasticsearch", + body='{"query": {"match_all": {}}}', + declared_params={}, + version=1, + ) + proposal = await repo.create_proposal( + db, + id=str(uuid.uuid4()), + study_id=None, + study_trial_id=None, + cluster_id=cluster.id, + template_id=template.id, + config_diff={}, + metric_delta=None, + status="pending", + ) + await db.commit() + pid = proposal.id + async with factory() as db: + await repo.mark_proposal_pr_opened(db, pid, pr_url=pr_url) + await db.commit() + return pid + + +def _pull_request_body( + action: str, *, merged: bool, pr_url: str, owner: str, repo_name: str +) -> bytes: + payload: dict[str, object] = { + "action": action, + "repository": {"full_name": f"{owner}/{repo_name}"}, + "pull_request": { + "html_url": pr_url, + "merged": merged, + "merged_at": "2026-05-12T11:00:00Z" if merged else None, + }, + } + return json.dumps(payload).encode("utf-8") + + +async def test_webhook_pr_merged_transitions_state( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-1 — merged event flips status to pr_merged + populates pr_merged_at.""" + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + body = _pull_request_body( + "closed", + merged=True, + pr_url=pr_url, + owner=webhook_env["owner"], + repo_name=webhook_env["repo"], + ) + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-merged", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "applied"} + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.status == "pr_merged" + assert row.pr_state == "merged" + assert row.pr_merged_at is not None + + +async def test_webhook_pr_closed_unmerged_keeps_status( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """FR-1 closed+merged=false branch: pr_state='closed', status STAYS pr_opened.""" + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + body = _pull_request_body( + "closed", + merged=False, + pr_url=pr_url, + owner=webhook_env["owner"], + repo_name=webhook_env["repo"], + ) + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-closed", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "applied"} + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.status == "pr_opened" + assert row.pr_state == "closed" + + +async def test_webhook_pr_reopened_returns_to_open( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """FR-1 reopened branch: closed → open.""" + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + # Transition to closed first via direct repo call (mirror prior delivery). + factory = get_session_factory() + async with factory() as db: + await repo.mark_proposal_pr_closed(db, pid) + await db.commit() + + payload = { + "action": "reopened", + "repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}, + "pull_request": {"html_url": pr_url}, + } + body = json.dumps(payload).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-reopened", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "applied"} + + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.pr_state == "open" + + +@pytest.mark.parametrize( + "action", + ["opened", "edited", "synchronize", "review_requested", "assigned"], +) +async def test_webhook_pr_noop_actions( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], + action: str, +) -> None: + """FR-1 noop branch: PR actions other than closed/reopened → action=noop.""" + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + payload = { + "action": action, + "repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}, + "pull_request": {"html_url": pr_url}, + } + body = json.dumps(payload).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": f"delivery-noop-{action}", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "noop"} + + # No state mutation. + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + assert row.pr_state == "open" + assert row.status == "pr_opened" + + +async def test_webhook_unknown_event_returns_noop( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """Unknown X-GitHub-Event types → 200 with action=noop (forward-compat).""" + payload = {"repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}} + body = json.dumps(payload).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "deployment_status", + "X-GitHub-Delivery": "delivery-unknown-event", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "noop"} + + +async def test_webhook_ping_event( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-4 — X-GitHub-Event: ping → 200 with action=ping.""" + payload = { + "repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}, + "zen": "Non-blocking is better than blocking.", + } + body = json.dumps(payload).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "ping", + "X-GitHub-Delivery": "delivery-ping", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "ping"} + + +async def test_webhook_unknown_pr_url( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-5 — valid signature + valid repo + unmapped pr_url → action=unknown_pr.""" + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}/pull/999999" # not seeded + ) + body = _pull_request_body( + "closed", + merged=True, + pr_url=pr_url, + owner=webhook_env["owner"], + repo_name=webhook_env["repo"], + ) + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-unknown-pr", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "unknown_pr"} + + +async def test_webhook_bad_signature_returns_403( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-2 path 1 — mismatched signature → 403 INVALID_SIGNATURE.""" + body = _pull_request_body( + "closed", + merged=True, + pr_url=f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}/pull/1", + owner=webhook_env["owner"], + repo_name=webhook_env["repo"], + ) + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-bad-sig", + "X-Hub-Signature-256": "sha256=" + "0" * 64, + "Content-Type": "application/json", + }, + ) + assert response.status_code == 403 + assert response.json()["detail"]["error_code"] == "INVALID_SIGNATURE" + + +async def test_webhook_missing_signature_returns_403( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-2 path 2 — no X-Hub-Signature-256 → 403 INVALID_SIGNATURE.""" + body = _pull_request_body( + "closed", + merged=True, + pr_url=f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}/pull/1", + owner=webhook_env["owner"], + repo_name=webhook_env["repo"], + ) + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-missing-sig", + "Content-Type": "application/json", + }, + ) + assert response.status_code == 403 + assert response.json()["detail"]["error_code"] == "INVALID_SIGNATURE" + + +async def test_webhook_unknown_repo_returns_403( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """AC-2 path 3 — repository.full_name doesn't match any config_repo → 403.""" + body = json.dumps( + { + "action": "closed", + "repository": {"full_name": "unknown/unregistered"}, + "pull_request": { + "html_url": "https://github.com/unknown/unregistered/pull/1", + "merged": True, + "merged_at": "2026-05-12T11:00:00Z", + }, + } + ).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-unknown-repo", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 403 + assert response.json()["detail"]["error_code"] == "INVALID_SIGNATURE" + + +async def test_webhook_non_object_json_returns_403( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """GPT-5.5 final-review F2 — a JSON array / scalar payload returns 403, not 500. + + Without the ``isinstance(parsed_body, dict)`` guard the handler hits + ``payload.get(...)`` on a list and raises ``AttributeError`` → 500. + """ + body = b"[]" + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-non-object", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 403 + assert response.json()["detail"]["error_code"] == "INVALID_SIGNATURE" + + +async def test_webhook_pr_merged_with_missing_merged_at_does_not_500( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], +) -> None: + """GPT-5.5 final-review F3 — merged=true with merged_at=null falls back to closed. + + GitHub's eventual consistency can emit a closed+merged delivery with + ``merged_at: null``. The handler must NOT 500. It should transition + the proposal to ``pr_state="closed"`` (status stays pr_opened) — + the polling reconciler catches up on the merged_at value next tick. + """ + pr_url = ( + f"https://github.com/{webhook_env['owner']}/{webhook_env['repo']}" + f"/pull/{uuid.uuid4().int % 10_000}" + ) + pid = await _seed_pr_opened_proposal(pr_url=pr_url) + + payload: dict[str, object] = { + "action": "closed", + "repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}, + "pull_request": { + "html_url": pr_url, + "merged": True, + "merged_at": None, + }, + } + body = json.dumps(payload).encode("utf-8") + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "delivery-merged-no-timestamp", + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + assert response.json() == {"status": "ok", "action": "applied"} + + factory = get_session_factory() + async with factory() as db: + row = await repo.get_proposal(db, pid) + assert row is not None + # Fallback path: status stays pr_opened, pr_state flips to closed. + assert row.status == "pr_opened" + assert row.pr_state == "closed" + + +async def test_webhook_logs_structured_fields( + async_client: httpx.AsyncClient, + webhook_env: dict[str, str], + caplog: pytest.LogCaptureFixture, +) -> None: + """Spec §13 NFR-Operability — webhook_received emits delivery_id/event/action/result.""" + caplog.set_level(logging.INFO, logger="backend.app.api.webhooks.github") + payload = { + "repository": {"full_name": f"{webhook_env['owner']}/{webhook_env['repo']}"}, + "zen": "Anything added dilutes everything else.", + } + body = json.dumps(payload).encode("utf-8") + delivery = f"delivery-log-{datetime.now(UTC).timestamp()}" + response = await async_client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": "ping", + "X-GitHub-Delivery": delivery, + "X-Hub-Signature-256": _signature(body), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 200 + # structlog routes via stdlib logging; caplog captures the formatted line. + assert any("webhook_received" in record.getMessage() for record in caplog.records) diff --git a/backend/tests/unit/core/test_settings_pr_poll.py b/backend/tests/unit/core/test_settings_pr_poll.py new file mode 100644 index 00000000..53520cbb --- /dev/null +++ b/backend/tests/unit/core/test_settings_pr_poll.py @@ -0,0 +1,73 @@ +"""Unit tests for feat_github_webhook Story 1.1 Settings field. + +One new field: + +* ``relyloop_pr_poll_minutes: int`` with ``Field(default=15, ge=1, le=1440, ...)`` + — the cron cadence for ``reconcile_pr_state``. Story 3.1 narrows the + valid range to a whitelist of cron-expressible values; this story ships + the broad ``ge/le`` bound that Story 3.1 will tighten. +""" + +from __future__ import annotations + +from collections.abc import Iterator + +import pytest +from pydantic import ValidationError + +from backend.app.core.settings import Settings, get_settings + + +@pytest.fixture(autouse=True) +def _clear_settings_cache_and_required_env(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Clear the lru_cache + provide the required-secret env vars. + + Settings construction requires DATABASE_URL_FILE + POSTGRES_PASSWORD_FILE + per infra_foundation Rule #2. Point both at /dev/null — the + @cached_property accessors aren't invoked by these tests so the empty + file content is never read. + """ + monkeypatch.setenv("DATABASE_URL_FILE", "/dev/null") + monkeypatch.setenv("POSTGRES_PASSWORD_FILE", "/dev/null") + get_settings.cache_clear() + yield + get_settings.cache_clear() + + +def test_relyloop_pr_poll_minutes_defaults_to_15() -> None: + assert get_settings().relyloop_pr_poll_minutes == 15 + + +def test_relyloop_pr_poll_minutes_reads_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("RELYLOOP_PR_POLL_MINUTES", "30") + get_settings.cache_clear() + assert get_settings().relyloop_pr_poll_minutes == 30 + + +def test_relyloop_pr_poll_minutes_accepts_lower_bound() -> None: + """``ge=1`` allows the minimum value.""" + s = Settings(relyloop_pr_poll_minutes=1) + assert s.relyloop_pr_poll_minutes == 1 + + +def test_relyloop_pr_poll_minutes_accepts_upper_bound() -> None: + """``le=1440`` allows the 24h max.""" + s = Settings(relyloop_pr_poll_minutes=1440) + assert s.relyloop_pr_poll_minutes == 1440 + + +def test_relyloop_pr_poll_minutes_rejects_zero() -> None: + """Below the ``ge=1`` lower bound raises validation.""" + with pytest.raises(ValidationError): + Settings(relyloop_pr_poll_minutes=0) + + +def test_relyloop_pr_poll_minutes_rejects_negative() -> None: + with pytest.raises(ValidationError): + Settings(relyloop_pr_poll_minutes=-1) + + +def test_relyloop_pr_poll_minutes_rejects_above_upper_bound() -> None: + """Above ``le=1440`` (24h) raises validation.""" + with pytest.raises(ValidationError): + Settings(relyloop_pr_poll_minutes=1441) diff --git a/backend/tests/unit/domain/test_repository_name.py b/backend/tests/unit/domain/test_repository_name.py new file mode 100644 index 00000000..f2e413e1 --- /dev/null +++ b/backend/tests/unit/domain/test_repository_name.py @@ -0,0 +1,85 @@ +"""Unit tests for ``parse_repository_full_name`` (feat_github_webhook Story 1.2).""" + +from __future__ import annotations + +import pytest + +from backend.app.domain.git import parse_repository_full_name + + +def test_canonical_owner_repo() -> None: + assert parse_repository_full_name("octocat/hello-world") == ("octocat", "hello-world") + + +def test_owner_with_hyphens() -> None: + assert parse_repository_full_name("foo-bar/baz") == ("foo-bar", "baz") + + +def test_repo_with_dots() -> None: + """Repo names commonly contain dots (e.g. dotfiles, my.project).""" + assert parse_repository_full_name("user/dotfiles.config") == ("user", "dotfiles.config") + + +def test_uppercase_is_lowercased() -> None: + """GitHub URLs canonicalise to lowercase for comparison purposes.""" + assert parse_repository_full_name("OctoCat/Hello-World") == ("octocat", "hello-world") + + +def test_trailing_dot_git_is_stripped() -> None: + """Some upstream sources send the ``.git`` suffix; strip it for parity.""" + assert parse_repository_full_name("octocat/hello-world.git") == ("octocat", "hello-world") + + +def test_whitespace_is_stripped() -> None: + assert parse_repository_full_name(" octocat/hello ") == ("octocat", "hello") + + +def test_https_url_returns_none() -> None: + """HTTPS URLs are validate_repo_url's job — this parser only takes the short form.""" + assert parse_repository_full_name("https://github.com/octocat/hello") is None + + +def test_ssh_url_returns_none() -> None: + """SSH URLs (``git@github.com:owner/repo.git``) return None — not supported in MVP1.""" + assert parse_repository_full_name("git@github.com:octocat/hello.git") is None + + +def test_enterprise_host_returns_none() -> None: + """Domain-shaped owner (a dot in the owner segment) is rejected.""" + assert parse_repository_full_name("github.acme.com/octocat/hello") is None + + +def test_missing_slash_returns_none() -> None: + assert parse_repository_full_name("octocat") is None + + +def test_three_segments_returns_none() -> None: + """``owner/repo/extra`` is not the canonical short form.""" + assert parse_repository_full_name("owner/repo/extra") is None + + +def test_empty_returns_none() -> None: + assert parse_repository_full_name("") is None + + +def test_just_slash_returns_none() -> None: + assert parse_repository_full_name("/") is None + + +def test_leading_slash_owner_returns_none() -> None: + assert parse_repository_full_name("/owner/repo") is None + + +def test_trailing_slash_with_empty_repo_returns_none() -> None: + assert parse_repository_full_name("owner/") is None + + +@pytest.mark.parametrize( + "value", + [ + "owner-/repo", # trailing hyphen on owner — _FULL_NAME_PATTERN forbids it + "-owner/repo", # leading hyphen on owner — forbidden + ], +) +def test_invalid_owner_handles_return_none(value: str) -> None: + assert parse_repository_full_name(value) is None diff --git a/backend/tests/unit/domain/test_url_owner_repo_parity.py b/backend/tests/unit/domain/test_url_owner_repo_parity.py new file mode 100644 index 00000000..16ded0a8 --- /dev/null +++ b/backend/tests/unit/domain/test_url_owner_repo_parity.py @@ -0,0 +1,64 @@ +"""Cross-validate ``validate_repo_url`` and ``parse_repository_full_name`` parity. + +feat_github_webhook spec FR-1 normalisation rule: the webhook receiver +must compare ``(owner, repo)`` extracted from ``config_repos.repo_url`` (via +``validate_repo_url``) against ``(owner, repo)`` extracted from the +webhook payload's ``repository.full_name`` (via +``parse_repository_full_name``). Both must produce the same tuple when +the inputs describe the same repo. This file is the regression gate for +that parity, plus the SSH + enterprise-host negative cases the spec §14 +test matrix calls out. +""" + +from __future__ import annotations + +import pytest + +from backend.app.domain.git import ( + UnsupportedProviderError, + parse_repository_full_name, + validate_repo_url, +) + + +@pytest.mark.parametrize( + ("url", "full_name", "expected"), + [ + ("https://github.com/octocat/hello", "octocat/hello", ("octocat", "hello")), + ("https://github.com/octocat/hello.git", "octocat/hello", ("octocat", "hello")), + ("https://github.com/Foo-Bar/Baz", "foo-bar/baz", ("foo-bar", "baz")), + ( + "https://github.com/user/dotfiles.config", + "user/dotfiles.config", + ("user", "dotfiles.config"), + ), + ], +) +def test_parity_canonical_inputs(url: str, full_name: str, expected: tuple[str, str]) -> None: + """The two parsers produce comparable ``(owner, repo)`` tuples (case-insensitive).""" + url_owner, url_repo = validate_repo_url(url) + full_owner_repo = parse_repository_full_name(full_name) + assert full_owner_repo is not None + # validate_repo_url preserves the input case; the canonical-comparison + # rule is "lowercase both sides before comparing". + assert (url_owner.lower(), url_repo.lower()) == expected + assert full_owner_repo == expected + + +def test_ssh_url_rejected_by_both_parsers() -> None: + """SSH form is not supported in MVP1 — both parsers must refuse it.""" + ssh = "git@github.com:octocat/hello.git" + with pytest.raises(UnsupportedProviderError): + validate_repo_url(ssh) + assert parse_repository_full_name(ssh) is None + + +def test_enterprise_host_rejected_by_validate_repo_url() -> None: + """``validate_repo_url`` accepts only ``https://github.com``; enterprise hosts raise.""" + with pytest.raises(UnsupportedProviderError): + validate_repo_url("https://github.acme.com/octocat/hello") + + +def test_enterprise_short_form_rejected_by_parse_full_name() -> None: + """A domain-shaped owner segment is rejected by the short-form parser.""" + assert parse_repository_full_name("github.acme.com/octocat/hello") is None diff --git a/backend/tests/unit/domain/test_webhook_dispatch.py b/backend/tests/unit/domain/test_webhook_dispatch.py new file mode 100644 index 00000000..3788822b --- /dev/null +++ b/backend/tests/unit/domain/test_webhook_dispatch.py @@ -0,0 +1,143 @@ +"""Unit tests for ``dispatch_event`` (feat_github_webhook Story 1.3). + +Covers every branch of the FR-1 matrix plus a parametrised negative +assertion that the dispatcher NEVER emits ``action="unknown_pr"`` — that +string is router-owned (set after a proposal-lookup miss). +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import Any + +import pytest + +from backend.app.domain.git import ( + HANDLED_EVENT_TYPES, + WEBHOOK_ACTION_VALUES, + WebhookDecision, + dispatch_event, +) + +_PR_URL = "https://github.com/octocat/hello/pull/42" + + +def _pr_payload(action: str, **pull_request: Any) -> dict[str, Any]: + base: dict[str, Any] = {"html_url": _PR_URL} + base.update(pull_request) + return {"action": action, "pull_request": base} + + +def test_ping_returns_ping_action() -> None: + decision = dispatch_event("ping", {"zen": "Anything added dilutes everything else."}) + assert decision == WebhookDecision( + action="ping", pr_url=None, pr_merged_at=None, mutation="none" + ) + + +def test_closed_and_merged_returns_merged_mutation() -> None: + payload = _pr_payload("closed", merged=True, merged_at="2026-05-12T11:00:00Z") + decision = dispatch_event("pull_request", payload) + assert decision.action == "applied" + assert decision.mutation == "merged" + assert decision.pr_url == _PR_URL + assert decision.pr_merged_at == datetime(2026, 5, 12, 11, 0, 0, tzinfo=UTC) + + +def test_closed_without_merge_returns_closed_mutation() -> None: + """closed + merged=false: pr_state → closed but status STAYS pr_opened (§11).""" + payload = _pr_payload("closed", merged=False) + decision = dispatch_event("pull_request", payload) + assert decision == WebhookDecision( + action="applied", pr_url=_PR_URL, pr_merged_at=None, mutation="closed" + ) + + +def test_reopened_returns_reopened_mutation() -> None: + payload = _pr_payload("reopened") + decision = dispatch_event("pull_request", payload) + assert decision == WebhookDecision( + action="applied", pr_url=_PR_URL, pr_merged_at=None, mutation="reopened" + ) + + +@pytest.mark.parametrize( + "action", + ["opened", "edited", "synchronize", "review_requested", "assigned", "ready_for_review"], +) +def test_other_pull_request_actions_noop(action: str) -> None: + payload = _pr_payload(action) + decision = dispatch_event("pull_request", payload) + assert decision.action == "noop" + assert decision.mutation == "none" + + +@pytest.mark.parametrize( + "event_type", + ["push", "issues", "deployment_status", "workflow_run", "completely_unknown"], +) +def test_unknown_event_types_noop(event_type: str) -> None: + decision = dispatch_event(event_type, {"action": "anything"}) + assert decision.action == "noop" + assert decision.mutation == "none" + + +def test_pull_request_without_html_url_noop() -> None: + """Malformed payload (no pull_request.html_url) is a defensive noop.""" + decision = dispatch_event( + "pull_request", {"action": "closed", "pull_request": {"merged": True}} + ) + assert decision == WebhookDecision( + action="noop", pr_url=None, pr_merged_at=None, mutation="none" + ) + + +def test_pull_request_without_pull_request_field_noop() -> None: + decision = dispatch_event("pull_request", {"action": "closed"}) + assert decision == WebhookDecision( + action="noop", pr_url=None, pr_merged_at=None, mutation="none" + ) + + +def test_unparseable_merged_at_returns_none_timestamp() -> None: + """Non-ISO-8601 ``merged_at`` returns the merged mutation with None timestamp.""" + payload = _pr_payload("closed", merged=True, merged_at="not-a-timestamp") + decision = dispatch_event("pull_request", payload) + assert decision.mutation == "merged" + assert decision.pr_merged_at is None + + +def test_handled_event_types_frozenset() -> None: + """Spec §8.4 source-of-truth tie-back.""" + assert HANDLED_EVENT_TYPES == frozenset({"ping", "pull_request"}) + + +def test_webhook_action_values_frozenset() -> None: + """Spec §8.4 source-of-truth tie-back.""" + assert WEBHOOK_ACTION_VALUES == frozenset({"applied", "noop", "unknown_pr", "ping"}) + + +@pytest.mark.parametrize( + ("event_type", "payload"), + [ + ("ping", {}), + ("pull_request", _pr_payload("closed", merged=True, merged_at="2026-05-12T11:00:00Z")), + ("pull_request", _pr_payload("closed", merged=False)), + ("pull_request", _pr_payload("reopened")), + ("pull_request", _pr_payload("opened")), + ("pull_request", _pr_payload("synchronize")), + ("push", {}), + ("issues", {}), + ], +) +def test_dispatcher_never_emits_unknown_pr(event_type: str, payload: dict[str, Any]) -> None: + """Cross-model review F2: ``"unknown_pr"`` is router-owned, not dispatcher-owned. + + The static type narrows ``action`` to ``Literal["applied", "noop", "ping"]``, + but assert at runtime too so regressions surface clearly. + """ + decision = dispatch_event(event_type, payload) + # Positive form — mypy narrows decision.action to the 3-element Literal, + # which already PROVES it's never "unknown_pr". The runtime check guards + # against future widening of the Literal. + assert decision.action in {"applied", "noop", "ping"} diff --git a/backend/tests/unit/domain/test_webhook_signature.py b/backend/tests/unit/domain/test_webhook_signature.py new file mode 100644 index 00000000..d1a4463b --- /dev/null +++ b/backend/tests/unit/domain/test_webhook_signature.py @@ -0,0 +1,88 @@ +"""Unit tests for ``verify_webhook_signature`` (feat_github_webhook Story 1.2).""" + +from __future__ import annotations + +import hashlib +import hmac +from unittest.mock import patch + +import pytest + +from backend.app.domain.git import verify_webhook_signature + +_SECRET = "super-secret-string" +_BODY = b'{"action":"closed","pull_request":{"merged":true}}' + + +def _hmac_header(secret: str, body: bytes) -> str: + digest = hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest() + return f"sha256={digest}" + + +def test_valid_signature_returns_true() -> None: + header = _hmac_header(_SECRET, _BODY) + assert verify_webhook_signature(_BODY, header, _SECRET) is True + + +def test_mismatched_signature_returns_false() -> None: + header = _hmac_header("different-secret", _BODY) + assert verify_webhook_signature(_BODY, header, _SECRET) is False + + +def test_missing_header_returns_false() -> None: + assert verify_webhook_signature(_BODY, None, _SECRET) is False + + +def test_missing_sha256_prefix_returns_false() -> None: + """Headers without the ``sha256=`` prefix are rejected outright.""" + digest = hmac.new(_SECRET.encode("utf-8"), _BODY, hashlib.sha256).hexdigest() + # Same hex digest, no `sha256=` prefix. + assert verify_webhook_signature(_BODY, digest, _SECRET) is False + + +def test_sha1_prefix_returns_false() -> None: + """Legacy ``sha1=`` prefix is not accepted.""" + digest = hmac.new(_SECRET.encode("utf-8"), _BODY, hashlib.sha256).hexdigest() + assert verify_webhook_signature(_BODY, f"sha1={digest}", _SECRET) is False + + +def test_empty_signature_after_prefix_returns_false() -> None: + """``sha256=`` with no hex content is rejected.""" + assert verify_webhook_signature(_BODY, "sha256=", _SECRET) is False + + +def test_empty_body_with_matching_signature_returns_true() -> None: + """Edge case: GitHub may send an empty body; HMAC over b'' is still defined.""" + empty_body = b"" + header = _hmac_header(_SECRET, empty_body) + assert verify_webhook_signature(empty_body, header, _SECRET) is True + + +def test_empty_secret_returns_false() -> None: + """We refuse to verify with an empty secret — no unsigned acceptance.""" + header = _hmac_header(_SECRET, _BODY) + assert verify_webhook_signature(_BODY, header, "") is False + + +def test_uses_constant_time_compare() -> None: + """Sanity check: helper goes through :func:`hmac.compare_digest`.""" + header = _hmac_header(_SECRET, _BODY) + # Patch the symbol where it's looked up (in the helper's module). + with patch( + "backend.app.domain.git.webhook_signature.hmac.compare_digest", + wraps=hmac.compare_digest, + ) as spy: + assert verify_webhook_signature(_BODY, header, _SECRET) is True + assert spy.call_count == 1 + + +@pytest.mark.parametrize( + "tampered_header", + [ + "sha256=00", # too short — different length triggers length-mismatch path + "sha256=" + "0" * 64, # right length, wrong digest + "sha256=NOT_HEX_CHARS_AT_ALL_zzzz", # wrong length AND non-hex + ], +) +def test_garbage_signatures_return_false(tampered_header: str) -> None: + assert verify_webhook_signature(_BODY, tampered_header, _SECRET) is False diff --git a/backend/tests/unit/git/__init__.py b/backend/tests/unit/git/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/backend/tests/unit/git/test_github_client.py b/backend/tests/unit/git/test_github_client.py new file mode 100644 index 00000000..a25eb3b4 --- /dev/null +++ b/backend/tests/unit/git/test_github_client.py @@ -0,0 +1,266 @@ +"""Unit tests for ``backend.app.git.github_client.github_request``. + +Method-agnostic retry policy covered via ``httpx.MockTransport`` (the +established mocking pattern in this codebase — see +``backend/tests/unit/adapters/test_request_retry.py`` and +``backend/tests/unit/test_capability_check.py``). Tests are +parametrised over GET + POST so the method-agnostic generalisation +introduced by feat_github_webhook Story 1.5 is exercised on both verbs. + +Asyncio sleeps are patched to ``0`` via the autouse ``_fast_sleep`` +fixture so the retry loop is fast under pytest. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from typing import Any + +import httpx +import pytest + +from backend.app.git.github_client import ( + HTTP_RETRY_MAX, + github_request, +) + +_TOKEN = "ghp_" + "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ" +_METHODS = ("GET", "POST") + + +@pytest.fixture(autouse=True) +def _fast_sleep(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Patch asyncio.sleep so retry-backoff waits don't slow the tests.""" + + async def _instant(_seconds: float) -> None: + return None + + monkeypatch.setattr("backend.app.git.github_client.asyncio.sleep", _instant) + yield + + +def _make_client(handler: httpx.MockTransport) -> httpx.AsyncClient: + return httpx.AsyncClient(transport=handler) + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_returns_2xx_immediately(method: str) -> None: + """Success path: a single 2xx response returns without retrying.""" + calls = {"n": 0} + + def handler(_request: httpx.Request) -> httpx.Response: + calls["n"] += 1 + return httpx.Response(200, json={"ok": True}) + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request( + client, + method, + "https://api.github.com/foo", + json_body=None if method == "GET" else {"a": 1}, + token=_TOKEN, + ) + assert response.status_code == 200 + assert response.json() == {"ok": True} + assert calls["n"] == 1 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_5xx(method: str) -> None: + """5xx triggers exponential backoff; eventual 2xx is returned.""" + statuses = iter([503, 502, 200]) + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(next(statuses), text="ok") + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request( + client, + method, + "https://api.github.com/foo", + json_body=None, + token=_TOKEN, + ) + assert response.status_code == 200 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_429_then_returns(method: str) -> None: + """429 with Retry-After is honoured (sleep is patched to 0).""" + sequence = iter([429, 200]) + + def handler(_request: httpx.Request) -> httpx.Response: + status = next(sequence) + if status == 429: + return httpx.Response(429, headers={"retry-after": "1"}) + return httpx.Response(200, text="recovered") + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request( + client, + method, + "https://api.github.com/foo", + json_body=None, + token=_TOKEN, + ) + assert response.status_code == 200 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_403_with_retry_after(method: str) -> None: + """403 carrying ``Retry-After`` is treated as a transient signal.""" + sequence = iter([403, 200]) + + def handler(_request: httpx.Request) -> httpx.Response: + status = next(sequence) + if status == 403: + return httpx.Response(403, headers={"retry-after": "0"}, text="slow down") + return httpx.Response(200) + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 200 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_403_with_secondary_rate_limit(method: str) -> None: + """403 with ``X-RateLimit-Remaining: 0`` retries until success.""" + sequence = iter([403, 200]) + + def handler(_request: httpx.Request) -> httpx.Response: + status = next(sequence) + if status == 403: + return httpx.Response( + 403, + headers={"x-ratelimit-remaining": "0", "x-ratelimit-reset": "0"}, + text="", + ) + return httpx.Response(200) + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 200 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_403_with_abuse_body(method: str) -> None: + """403 whose body mentions ``rate limit`` retries via exponential backoff.""" + sequence = iter([403, 200]) + + def handler(_request: httpx.Request) -> httpx.Response: + status = next(sequence) + if status == 403: + return httpx.Response( + 403, + text='{"message": "You have exceeded a secondary rate limit"}', + ) + return httpx.Response(200) + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 200 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_returns_terminal_403_immediately(method: str) -> None: + """403 with NO retry signal is terminal (PAT scope failure).""" + calls = {"n": 0} + + def handler(_request: httpx.Request) -> httpx.Response: + calls["n"] += 1 + return httpx.Response(403, text="Resource not accessible by personal access token") + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 403 + assert calls["n"] == 1 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_returns_4xx_terminal_immediately(method: str) -> None: + """4xx (other than 429/403-with-signal) is terminal.""" + calls = {"n": 0} + + def handler(_request: httpx.Request) -> httpx.Response: + calls["n"] += 1 + return httpx.Response(404, text="Not Found") + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 404 + assert calls["n"] == 1 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_retries_request_error_then_returns(method: str) -> None: + """RequestError on first attempt + 2xx on the second succeeds.""" + state: dict[str, Any] = {"calls": 0} + + def handler(_request: httpx.Request) -> httpx.Response: + state["calls"] += 1 + if state["calls"] == 1: + raise httpx.ConnectError("transient network blip") + return httpx.Response(200) + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 200 + assert state["calls"] == 2 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_propagates_request_error_after_budget(method: str) -> None: + """RequestError on every attempt → final exception propagates.""" + + def handler(_request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("connection refused") + + async with _make_client(httpx.MockTransport(handler)) as client: + with pytest.raises(httpx.RequestError): + await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_returns_last_5xx_after_budget_exhausted(method: str) -> None: + """All attempts return 5xx → the last response is returned (not raised).""" + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(503, text="server overloaded") + + async with _make_client(httpx.MockTransport(handler)) as client: + response = await github_request(client, method, "https://api.github.com/foo", token=_TOKEN) + assert response.status_code == 503 + + +@pytest.mark.parametrize("method", _METHODS) +async def test_github_request_sends_token_and_version_headers(method: str) -> None: + """Authorization Bearer + X-GitHub-Api-Version are set on every call.""" + captured: dict[str, str] = {} + + def handler(request: httpx.Request) -> httpx.Response: + captured["auth"] = request.headers.get("authorization", "") + captured["api_version"] = request.headers.get("x-github-api-version", "") + captured["accept"] = request.headers.get("accept", "") + captured["method"] = request.method + return httpx.Response(200) + + async with _make_client(httpx.MockTransport(handler)) as client: + await github_request( + client, + method, + "https://api.github.com/foo", + json_body=None if method == "GET" else {"a": 1}, + token=_TOKEN, + ) + assert captured["auth"] == f"Bearer {_TOKEN}" + assert captured["api_version"] == "2022-11-28" + assert captured["accept"] == "application/vnd.github+json" + assert captured["method"] == method + + +def test_http_retry_max_is_three() -> None: + """Sanity check: the retry budget aligns with the documented backoff schedule.""" + from backend.app.git.github_client import HTTP_RETRY_BACKOFF_S + + assert HTTP_RETRY_MAX == 3 + assert len(HTTP_RETRY_BACKOFF_S) == HTTP_RETRY_MAX diff --git a/backend/tests/unit/test_workers.py b/backend/tests/unit/test_workers.py index 51da1ab7..491b2c67 100644 --- a/backend/tests/unit/test_workers.py +++ b/backend/tests/unit/test_workers.py @@ -57,6 +57,7 @@ def test_worker_settings_importable(_settings_env: None) -> None: "generate_digest", "generate_judgments_llm", "open_pr", + "register_webhook", } @@ -142,3 +143,18 @@ def test_worker_settings_redis_host_overridable( # Restore for other tests os.environ.pop("REDIS_URL", None) get_settings.cache_clear() + + +def test_pr_reconcile_cron_registered(_settings_env: None) -> None: + """feat_github_webhook Story 3.1 — reconcile_pr_state registered via cron_jobs. + + Asserts the cron job is wired with the default 15-minute cadence + (``minute={0, 15, 30, 45}``) at the default ``relyloop_pr_poll_minutes``. + """ + from backend.workers.all import WorkerSettings + + cron_jobs = getattr(WorkerSettings, "cron_jobs", []) + assert cron_jobs, "WorkerSettings.cron_jobs missing — reconcile_pr_state not wired" + # arq.CronJob exposes the registered coroutine; match by its __name__. + names = {getattr(job.coroutine, "__name__", None) for job in cron_jobs} + assert "reconcile_pr_state" in names diff --git a/backend/tests/unit/workers/test_git_pr_helpers.py b/backend/tests/unit/workers/test_git_pr_helpers.py index 60adc8c6..7916a992 100644 --- a/backend/tests/unit/workers/test_git_pr_helpers.py +++ b/backend/tests/unit/workers/test_git_pr_helpers.py @@ -7,8 +7,10 @@ * ``_read_pat`` — mounted-secrets bundle read + containment check. * ``_repo_clone_root`` / ``_secrets_dir`` — env-override branches. * ``_redact_subprocess_error`` — argv-free CalledProcessError rendering. -* ``_parse_retry_after`` / ``_is_secondary_rate_limit`` / - ``_parse_rate_limit_reset`` — httpx response header parsers. +* ``backend.app.git.github_client`` header parsers — httpx response + parsers for ``Retry-After`` / ``X-RateLimit-Remaining`` / + ``X-RateLimit-Reset`` / abuse-body fallback (extracted from git_pr + by feat_github_webhook Story 1.5; the contract is unchanged). * ``_git_env`` / ``_commit_env`` — token-safe subprocess env construction (asserts the token NEVER lands in any field name; only in the Authorization header value). @@ -32,6 +34,18 @@ import pytest from backend.app.domain.git import InvalidConfigPathError as _InvalidConfigPathError +from backend.app.git.github_client import ( + body_mentions_rate_limit as _body_mentions_rate_limit, +) +from backend.app.git.github_client import ( + is_secondary_rate_limit as _is_secondary_rate_limit, +) +from backend.app.git.github_client import ( + parse_rate_limit_reset as _parse_rate_limit_reset, +) +from backend.app.git.github_client import ( + parse_retry_after as _parse_retry_after, +) from backend.workers import git_pr @@ -259,17 +273,17 @@ def test_redact_subprocess_error_with_no_streams() -> None: def test_parse_retry_after_numeric() -> None: response = httpx.Response(429, headers={"retry-after": "15"}) - assert git_pr._parse_retry_after(response) == 15.0 + assert _parse_retry_after(response) == 15.0 def test_parse_retry_after_invalid_falls_back() -> None: response = httpx.Response(429, headers={"retry-after": "tomorrow"}) - assert git_pr._parse_retry_after(response) == 1.0 + assert _parse_retry_after(response) == 1.0 def test_parse_retry_after_missing_falls_back() -> None: response = httpx.Response(429) - assert git_pr._parse_retry_after(response) == 1.0 + assert _parse_retry_after(response) == 1.0 def test_is_secondary_rate_limit_true() -> None: @@ -277,19 +291,19 @@ def test_is_secondary_rate_limit_true() -> None: 403, headers={"x-ratelimit-remaining": "0", "x-ratelimit-reset": "1700000000"}, ) - assert git_pr._is_secondary_rate_limit(response) is True + assert _is_secondary_rate_limit(response) is True def test_is_secondary_rate_limit_false_when_remaining_nonzero() -> None: response = httpx.Response( 403, headers={"x-ratelimit-remaining": "10", "x-ratelimit-reset": "1700000000"} ) - assert git_pr._is_secondary_rate_limit(response) is False + assert _is_secondary_rate_limit(response) is False def test_is_secondary_rate_limit_false_when_missing_reset() -> None: response = httpx.Response(403, headers={"x-ratelimit-remaining": "0"}) - assert git_pr._is_secondary_rate_limit(response) is False + assert _is_secondary_rate_limit(response) is False def test_body_mentions_rate_limit_matches_secondary() -> None: @@ -297,29 +311,29 @@ def test_body_mentions_rate_limit_matches_secondary() -> None: 403, text='{"message": "You have exceeded a secondary rate limit"}', ) - assert git_pr._body_mentions_rate_limit(response) is True + assert _body_mentions_rate_limit(response) is True def test_body_mentions_rate_limit_matches_abuse() -> None: response = httpx.Response(403, text='{"message": "abuse detection triggered"}') - assert git_pr._body_mentions_rate_limit(response) is True + assert _body_mentions_rate_limit(response) is True def test_body_mentions_rate_limit_no_match() -> None: response = httpx.Response(403, text='{"message": "Not Found"}') - assert git_pr._body_mentions_rate_limit(response) is False + assert _body_mentions_rate_limit(response) is False def test_parse_rate_limit_reset_future() -> None: future = time.time() + 30 response = httpx.Response(403, headers={"x-ratelimit-reset": str(int(future))}) - wait = git_pr._parse_rate_limit_reset(response) + wait = _parse_rate_limit_reset(response) assert 28 <= wait <= 32 # rounding tolerance def test_parse_rate_limit_reset_invalid_falls_back() -> None: response = httpx.Response(403, headers={"x-ratelimit-reset": "noop"}) - assert git_pr._parse_rate_limit_reset(response) == 1.0 + assert _parse_rate_limit_reset(response) == 1.0 # --------------------------------------------------------------------------- @@ -482,104 +496,6 @@ def test_git_commit_file_uses_file_message_and_relpath( # --------------------------------------------------------------------------- -# _github_post: retry policy (httpx mocked) +# github_request retry policy now lives in backend.tests.unit.git.test_github_client +# (Story 1.5 extracted the helper; the new test file covers GET + POST). # --------------------------------------------------------------------------- - - -@pytest.mark.asyncio -async def test_github_post_returns_2xx_on_first_try() -> None: - transport = httpx.MockTransport( - lambda req: httpx.Response(201, json={"html_url": "https://x", "number": 1}) - ) - async with httpx.AsyncClient(transport=transport) as client: - resp = await git_pr._github_post( - client, "https://api.github.com/r/o/r/pulls", json_body={}, token=_TOKEN - ) - assert resp.status_code == 201 - - -@pytest.mark.asyncio -async def test_github_post_retries_on_5xx_then_succeeds( - monkeypatch: pytest.MonkeyPatch, -) -> None: - # Make backoff a no-op so the test runs quickly. - monkeypatch.setattr("backend.workers.git_pr.asyncio.sleep", _no_sleep) - - call_count = {"n": 0} - - def handler(req: httpx.Request) -> httpx.Response: - call_count["n"] += 1 - if call_count["n"] < 2: - return httpx.Response(503) - return httpx.Response(200, json={"ok": True}) - - transport = httpx.MockTransport(handler) - async with httpx.AsyncClient(transport=transport) as client: - resp = await git_pr._github_post( - client, "https://api.github.com/x", json_body={}, token=_TOKEN - ) - assert resp.status_code == 200 - assert call_count["n"] == 2 - - -@pytest.mark.asyncio -async def test_github_post_terminal_on_4xx(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr("backend.workers.git_pr.asyncio.sleep", _no_sleep) - transport = httpx.MockTransport(lambda req: httpx.Response(404, text="not found")) - async with httpx.AsyncClient(transport=transport) as client: - resp = await git_pr._github_post( - client, "https://api.github.com/x", json_body={}, token=_TOKEN - ) - assert resp.status_code == 404 - - -@pytest.mark.asyncio -async def test_github_post_retries_on_403_with_retry_after( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """GPT-5.5 F3 — 403 with Retry-After header is retryable.""" - monkeypatch.setattr("backend.workers.git_pr.asyncio.sleep", _no_sleep) - call_count = {"n": 0} - - def handler(req: httpx.Request) -> httpx.Response: - call_count["n"] += 1 - if call_count["n"] < 2: - return httpx.Response(403, headers={"retry-after": "1"}) - return httpx.Response(200, json={"ok": True}) - - transport = httpx.MockTransport(handler) - async with httpx.AsyncClient(transport=transport) as client: - resp = await git_pr._github_post( - client, "https://api.github.com/x", json_body={}, token=_TOKEN - ) - assert resp.status_code == 200 - assert call_count["n"] == 2 - - -@pytest.mark.asyncio -async def test_github_post_retries_on_403_with_rate_limit_body( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """GPT-5.5 F3 — 403 with rate-limit body but no headers is also retryable.""" - monkeypatch.setattr("backend.workers.git_pr.asyncio.sleep", _no_sleep) - call_count = {"n": 0} - - def handler(req: httpx.Request) -> httpx.Response: - call_count["n"] += 1 - if call_count["n"] < 2: - return httpx.Response( - 403, text='{"message": "You have exceeded a secondary rate limit"}' - ) - return httpx.Response(200, json={"ok": True}) - - transport = httpx.MockTransport(handler) - async with httpx.AsyncClient(transport=transport) as client: - resp = await git_pr._github_post( - client, "https://api.github.com/x", json_body={}, token=_TOKEN - ) - assert resp.status_code == 200 - assert call_count["n"] == 2 - - -async def _no_sleep(_seconds: float) -> None: - return None diff --git a/backend/tests/unit/workers/test_poll_cron_kwargs.py b/backend/tests/unit/workers/test_poll_cron_kwargs.py new file mode 100644 index 00000000..d464eee3 --- /dev/null +++ b/backend/tests/unit/workers/test_poll_cron_kwargs.py @@ -0,0 +1,97 @@ +"""Unit tests for ``backend.workers.pr_reconcile._poll_cron_kwargs``. + +Covers every value in :data:`SUPPORTED_POLL_MINUTES` plus the documented +fallback for an unsupported value. The Settings field validator rejects +unsupported values at boot — these tests exercise the fallback branch +through the worker's own helper (the validator is bypassed by mutating +``settings.__dict__``, mirroring the digest_helpers pattern). +""" + +from __future__ import annotations + +from collections.abc import Iterator + +import pytest + +from backend.app.core.settings import get_settings +from backend.workers.pr_reconcile import ( + FALLBACK_POLL_MINUTES, + SUPPORTED_POLL_MINUTES, + _poll_cron_kwargs, +) + + +def _set_poll_minutes(value: int) -> None: + """Set the cached Settings field without re-running the field_validator.""" + settings = get_settings() + settings.__dict__["relyloop_pr_poll_minutes"] = value + + +@pytest.fixture(autouse=True) +def _settings_env_and_restore(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Provide required-secret env vars + snapshot/restore the cached field. + + Settings construction needs ``DATABASE_URL_FILE`` + ``POSTGRES_PASSWORD_FILE`` + per CLAUDE.md Rule #2. Point both at ``/dev/null`` — the cached_property + accessors aren't invoked here. We mutate + ``settings.__dict__["relyloop_pr_poll_minutes"]`` directly to dodge the + Pydantic field_validator (testing the worker's runtime fallback against + an "impossible" value). + """ + monkeypatch.setenv("DATABASE_URL_FILE", "/dev/null") + monkeypatch.setenv("POSTGRES_PASSWORD_FILE", "/dev/null") + get_settings.cache_clear() + settings = get_settings() + original = settings.__dict__.get("relyloop_pr_poll_minutes") + yield + if original is None: + settings.__dict__.pop("relyloop_pr_poll_minutes", None) + else: + settings.__dict__["relyloop_pr_poll_minutes"] = original + get_settings.cache_clear() + + +@pytest.mark.parametrize("value", sorted(v for v in SUPPORTED_POLL_MINUTES if v <= 60)) +def test_sub_hourly_values_emit_minute_set(value: int) -> None: + """Divisors of 60: ``cron(minute=set(range(0, 60, n)))``.""" + _set_poll_minutes(value) + kwargs = _poll_cron_kwargs() + assert set(kwargs.keys()) == {"minute"} + assert kwargs["minute"] == set(range(0, 60, value)) + # The set must be non-empty + sized correctly. + assert len(kwargs["minute"]) == 60 // value + + +@pytest.mark.parametrize("value", sorted(v for v in SUPPORTED_POLL_MINUTES if v > 60)) +def test_multi_hour_values_emit_hour_set(value: int) -> None: + """Multi-hour values: ``cron(hour=set(range(0, 24, n // 60)), minute={0})``.""" + _set_poll_minutes(value) + kwargs = _poll_cron_kwargs() + assert set(kwargs.keys()) == {"hour", "minute"} + assert kwargs["minute"] == {0} + assert kwargs["hour"] == set(range(0, 24, value // 60)) + # 1440 is a special case: exactly one tick per day at hour 0. + if value == 1440: + assert kwargs["hour"] == {0} + + +def test_unsupported_value_falls_back_to_default() -> None: + """Outside the whitelist → fallback to 15-min default. + + The fallback emits a ``pr_poll_minutes_unsupported`` WARN via + structlog at the moment of invocation. We don't assert against + capsys here because structlog's ``PrintLogger`` was already bound + to a different stdout by ``configure_logging()`` earlier in the + test session — see the same xpassed flake on + ``test_capability_check`` and the tracking idea at + ``docs/02_product/planned_features/bug_capability_check_test_isolation/``. + Functional contract (the fallback kwargs) is the real invariant. + """ + _set_poll_minutes(7) # 7 is not a divisor of 60 + kwargs = _poll_cron_kwargs() + assert kwargs == {"minute": set(range(0, 60, FALLBACK_POLL_MINUTES))} + + +def test_supported_set_size_matches_documentation() -> None: + """Sanity: the spec documents 18 supported values.""" + assert len(SUPPORTED_POLL_MINUTES) == 18 diff --git a/backend/workers/all.py b/backend/workers/all.py index b7794193..6a7963b1 100644 --- a/backend/workers/all.py +++ b/backend/workers/all.py @@ -52,7 +52,7 @@ from typing import Any import structlog -from arq import func +from arq import cron, func from arq.connections import ArqRedis, RedisSettings, create_pool from backend.app.core.settings import get_settings @@ -63,6 +63,8 @@ from backend.workers.git_pr import open_pr from backend.workers.judgments import generate_judgments_llm from backend.workers.orchestrator import resume_study, start_study +from backend.workers.pr_reconcile import _poll_cron_kwargs, reconcile_pr_state +from backend.workers.register_webhook import register_webhook from backend.workers.trials import run_trial logger = structlog.get_logger(__name__) @@ -207,7 +209,13 @@ class WorkerSettings: generate_digest, func(generate_judgments_llm, timeout=_JUDGMENTS_JOB_TIMEOUT_S), func(open_pr, timeout=_OPEN_PR_JOB_TIMEOUT_S, max_tries=_OPEN_PR_MAX_TRIES), + register_webhook, ] + # feat_github_webhook Story 3.1: polling reconciler. The cron kwargs are + # derived from `Settings.relyloop_pr_poll_minutes` (defaults to every 15 + # minutes, whitelisted to cron-expressible values — see + # backend.workers.pr_reconcile.SUPPORTED_POLL_MINUTES). + cron_jobs: list[Any] = [cron(reconcile_pr_state, **_poll_cron_kwargs())] redis_settings = _build_redis_settings() on_startup = on_startup on_shutdown = on_shutdown diff --git a/backend/workers/git_pr.py b/backend/workers/git_pr.py index d7026991..91c08972 100644 --- a/backend/workers/git_pr.py +++ b/backend/workers/git_pr.py @@ -85,6 +85,7 @@ validate_config_path, validate_repo_url, ) +from backend.app.git import HTTP_TIMEOUT_S, github_request logger = structlog.get_logger(__name__) @@ -113,15 +114,9 @@ """Mounted-secret bundle root. Per-repo PATs live at ``./secrets/{config_repo.auth_ref}``.""" -_HTTP_TIMEOUT_S = 30.0 -"""httpx timeout for every GitHub REST call.""" - -_HTTP_RETRY_MAX = 3 -_HTTP_RETRY_BACKOFF_S = (1.0, 2.0, 4.0) -"""Exponential backoff for transient GitHub failures (cycle-1 F6).""" - -_RATE_LIMIT_CLAMP_S = 60.0 -"""Cap honored Retry-After / X-RateLimit-Reset waits at 60s (sanity).""" +# HTTP timeout + retry policy live in `backend.app.git.github_client` +# (feat_github_webhook Story 1.5 extracted the helpers so the polling +# reconciler + register-webhook worker share the retry contract). # --------------------------------------------------------------------------- @@ -585,111 +580,6 @@ class _BranchExistsError(ValueError): """AC-4 — branch already exists on the remote (no force-push).""" -# --------------------------------------------------------------------------- -# GitHub REST helpers -# --------------------------------------------------------------------------- - - -async def _github_post( - client: httpx.AsyncClient, - url: str, - *, - json_body: dict[str, Any], - token: str, -) -> httpx.Response: - """POST with the cycle-1 F6 retry policy. - - Retries on RequestError + 5xx + 429 (Retry-After) + 403-with-rate- - limit-headers (GitHub's secondary signal). Terminal on other 4xx. - """ - headers = { - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - } - last_response: httpx.Response | None = None - last_exc: Exception | None = None - for attempt in range(_HTTP_RETRY_MAX): - try: - response = await client.post(url, json=json_body, headers=headers) - except httpx.RequestError as exc: - last_exc = exc - await asyncio.sleep(_HTTP_RETRY_BACKOFF_S[attempt]) - continue - last_response = response - if response.status_code < 400: - return response - if response.status_code >= 500: - await asyncio.sleep(_HTTP_RETRY_BACKOFF_S[attempt]) - continue - if response.status_code == 429: - wait = _parse_retry_after(response) - await asyncio.sleep(min(wait, _RATE_LIMIT_CLAMP_S)) - continue - if response.status_code == 403: - # GPT-5.5 final-review F3 — GitHub's secondary rate limit - # sometimes returns 403 with Retry-After but without the - # x-ratelimit-* headers (the abuse-detection path). Honor - # any 403 that carries a retry hint or whose body advertises - # the secondary-rate-limit; only fall through to terminal - # for 403s with no retry signal at all. - if "retry-after" in response.headers: - wait = _parse_retry_after(response) - await asyncio.sleep(min(wait, _RATE_LIMIT_CLAMP_S)) - continue - if _is_secondary_rate_limit(response): - wait = _parse_rate_limit_reset(response) - await asyncio.sleep(min(wait, _RATE_LIMIT_CLAMP_S)) - continue - if _body_mentions_rate_limit(response): - await asyncio.sleep(_HTTP_RETRY_BACKOFF_S[attempt]) - continue - # Other 4xx — terminal. - return response - if last_response is not None: - return last_response - assert last_exc is not None # noqa: S101 — invariant: at least one attempt - raise last_exc - - -def _parse_retry_after(response: httpx.Response) -> float: - raw = response.headers.get("retry-after", "1") - try: - return max(0.0, float(raw)) - except ValueError: - return 1.0 - - -def _is_secondary_rate_limit(response: httpx.Response) -> bool: - return ( - response.headers.get("x-ratelimit-remaining") == "0" - and "x-ratelimit-reset" in response.headers - ) - - -def _body_mentions_rate_limit(response: httpx.Response) -> bool: - """GPT-5.5 final-review F3 — match GitHub's abuse-detection 403 body. - - Some secondary-rate-limit responses carry no headers — only a JSON - body like ``{"message": "You have exceeded a secondary rate limit"}``. - Conservative match on lowercase substring (no full body parse — we - don't want to crash on non-JSON 403s). - """ - try: - text = response.text.lower() - except Exception: # noqa: BLE001 — defensive against bytes/encoding edge - return False - return "rate limit" in text or "abuse" in text - - -def _parse_rate_limit_reset(response: httpx.Response) -> float: - raw = response.headers.get("x-ratelimit-reset", "0") - try: - return max(0.0, float(raw) - time.time()) - except ValueError: - return 1.0 - - # --------------------------------------------------------------------------- # Main entry # --------------------------------------------------------------------------- @@ -1032,10 +922,11 @@ async def _do_open_pr( # noqa: PLR0915, PLR0912, C901 — the worker contract i pr_url: str | None = None pr_number: int | None = None - async with httpx.AsyncClient(timeout=_HTTP_TIMEOUT_S) as client: + async with httpx.AsyncClient(timeout=HTTP_TIMEOUT_S) as client: try: - response = await _github_post( + response = await github_request( client, + "POST", f"https://api.github.com/repos/{owner}/{repo_name}/pulls", json_body={ "title": title, @@ -1092,8 +983,9 @@ async def _do_open_pr( # noqa: PLR0915, PLR0912, C901 — the worker contract i chart_url = _chart_raw_url(owner, repo_name, branch, proposal.study_id) comment_body = f"![Parameter importance]({chart_url})" try: - comment_response = await _github_post( + comment_response = await github_request( client, + "POST", ( f"https://api.github.com/repos/{owner}/{repo_name}" f"/issues/{pr_number}/comments" diff --git a/backend/workers/pr_reconcile.py b/backend/workers/pr_reconcile.py new file mode 100644 index 00000000..63320f17 --- /dev/null +++ b/backend/workers/pr_reconcile.py @@ -0,0 +1,239 @@ +"""Polling reconciler for stale ``pr_opened`` proposals (feat_github_webhook FR-2). + +Runs on the cron cadence derived from ``Settings.relyloop_pr_poll_minutes`` +(see :func:`_poll_cron_kwargs` in ``backend.workers.all`` for the +divisor-of-60 / multiple-of-60 routing). Each tick: + +1. Selects candidate proposals via + :func:`backend.app.db.repo.list_pr_opened_proposals_for_reconcile` + (status=pr_opened, pr_state=open, pr_url set, created_at < 90 days). +2. For each candidate, parses ``(owner, repo, number)`` from the stored + ``pr_url``, reads the matching ``config_repos.auth_ref`` PAT, and + issues ``GET /repos/{owner}/{repo}/pulls/{number}``. +3. Branches on the response body — ``merged=true`` → + :func:`mark_proposal_pr_merged`; ``state="closed"`` (unmerged) → + :func:`mark_proposal_pr_closed`; ``state="open"`` → no-op (PR is + genuinely still open). +4. Logs every other terminal response (404 / 401 / 403 / 5xx / + ``httpx.RequestError`` after retry budget) at WARN and continues. +5. On HTTP 429, logs WARN with ``x-ratelimit-reset`` and **breaks the + proposal loop** for this tick — the next tick retries (spec §10). + +No in-job retry envelope — the cron tick IS the retry. Idempotent: a +no-op on a stable proposal is cheap. +""" + +from __future__ import annotations + +import re +from collections.abc import Sequence +from datetime import datetime +from typing import Any + +import httpx +import structlog + +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.app.git import HTTP_TIMEOUT_S, github_request, read_mounted_secret + +logger = structlog.get_logger(__name__) + +_PR_URL_PATTERN = re.compile( + r"^https://github\.com/(?P[a-zA-Z0-9._-]+)/(?P[a-zA-Z0-9._-]+)/pull/" + r"(?P\d+)$" +) + + +def _parse_pr_url(pr_url: str) -> tuple[str, str, int] | None: + """Return ``(owner, repo, number)`` from a GitHub PR HTML URL, or None.""" + match = _PR_URL_PATTERN.match(pr_url) + if match is None: + return None + return match.group("owner"), match.group("repo"), int(match.group("number")) + + +def _coerce_merged_at(raw: Any) -> datetime | None: + if not isinstance(raw, str) or not raw: + return None + try: + return datetime.fromisoformat(raw.replace("Z", "+00:00")) + except ValueError: + return None + + +async def _list_candidates() -> Sequence[Any]: + factory = get_session_factory() + async with factory() as db: + return await repo.list_pr_opened_proposals_for_reconcile(db) + + +async def _resolve_config_repo(owner: str, repo_name: str) -> Any | None: + factory = get_session_factory() + async with factory() as db: + return await repo.lookup_config_repo_by_owner_repo(db, owner, repo_name) + + +async def reconcile_pr_state(ctx: dict[str, Any]) -> dict[str, int]: + """Poll GitHub for the current state of every stale ``pr_opened`` proposal. + + Returns a summary dict ``{candidates, reconciled, unchanged, errored, + rate_limited}`` logged at INFO for observability. + """ + summary = {"candidates": 0, "reconciled": 0, "unchanged": 0, "errored": 0, "rate_limited": 0} + + candidates = await _list_candidates() + summary["candidates"] = len(candidates) + if not candidates: + logger.info("pr_reconcile_tick_noop", **summary) + return summary + + async with httpx.AsyncClient(timeout=HTTP_TIMEOUT_S) as client: + for proposal in candidates: + parsed_url = _parse_pr_url(proposal.pr_url) if proposal.pr_url else None + if parsed_url is None: + logger.warning( + "pr_reconcile_unparseable_url", + proposal_id=proposal.id, + pr_url=proposal.pr_url, + ) + summary["errored"] += 1 + continue + owner, repo_name, number = parsed_url + + config_repo_row = await _resolve_config_repo(owner, repo_name) + if config_repo_row is None: + logger.warning( + "pr_reconcile_no_config_repo", + proposal_id=proposal.id, + owner=owner, + repo=repo_name, + ) + summary["errored"] += 1 + continue + + token = read_mounted_secret(config_repo_row.auth_ref) + if token is None: + logger.warning( + "pr_reconcile_pat_missing", + proposal_id=proposal.id, + config_repo_id=config_repo_row.id, + ) + summary["errored"] += 1 + continue + + url = f"https://api.github.com/repos/{owner}/{repo_name}/pulls/{number}" + try: + response = await github_request(client, "GET", url, token=token) + except httpx.RequestError as exc: + logger.warning( + "pr_reconcile_request_error", + proposal_id=proposal.id, + error_type=type(exc).__name__, + ) + summary["errored"] += 1 + continue + + if response.status_code == 429: + logger.warning( + "pr_reconcile_rate_limited", + proposal_id=proposal.id, + x_ratelimit_reset=response.headers.get("x-ratelimit-reset"), + ) + summary["rate_limited"] += 1 + # Spec §10: skip remaining proposals; next tick retries. + break + + if response.status_code in (401, 403, 404) or response.status_code >= 500: + logger.warning( + "pr_reconcile_terminal_error", + proposal_id=proposal.id, + status_code=response.status_code, + ) + summary["errored"] += 1 + continue + + if response.status_code != 200: + logger.warning( + "pr_reconcile_unexpected_status", + proposal_id=proposal.id, + status_code=response.status_code, + ) + summary["errored"] += 1 + continue + + payload = response.json() + merged = bool(payload.get("merged")) + state = payload.get("state") + merged_at = _coerce_merged_at(payload.get("merged_at")) + + factory = get_session_factory() + if merged and merged_at is not None: + async with factory() as db: + updated = await repo.mark_proposal_pr_merged( + db, proposal.id, pr_merged_at=merged_at + ) + await db.commit() + if updated is not None: + summary["reconciled"] += 1 + else: + summary["unchanged"] += 1 + elif state == "closed": + async with factory() as db: + updated = await repo.mark_proposal_pr_closed(db, proposal.id) + await db.commit() + if updated is not None: + summary["reconciled"] += 1 + else: + summary["unchanged"] += 1 + else: + # state == "open" — proposal is genuinely still pending review. + summary["unchanged"] += 1 + + logger.info("pr_reconcile_tick_complete", **summary) + return summary + + +# Re-exported so the validators module can dedup the supported set against +# `_poll_cron_kwargs` (both reference the same source-of-truth). +SUPPORTED_POLL_MINUTES: frozenset[int] = frozenset( + {1, 2, 3, 4, 5, 6, 10, 12, 15, 20, 30, 60, 120, 180, 240, 360, 720, 1440} +) +"""Whitelist of values ``RELYLOOP_PR_POLL_MINUTES`` may take. + +Divisors of 60 (sub-hourly minute set) plus multiples of 60 that divide +1440 (hourly + multi-hour set). Anything else cannot be expressed as a +single ``arq.cron(minute=..., hour=...)`` invocation, so we reject at +Settings-validation time rather than silently snapping (cross-model +review F3 / A4). +""" + +FALLBACK_POLL_MINUTES: int = 15 +"""Default cadence operators get if validation slipped (shouldn't happen).""" + + +def _poll_cron_kwargs() -> dict[str, Any]: + """Translate ``Settings.relyloop_pr_poll_minutes`` into arq.cron kwargs. + + * n ≤ 60 (divisor of 60): ``minute=set(range(0, 60, n))``. + * n > 60 (multiple of 60 dividing 1440): + ``hour=set(range(0, 24, n // 60)), minute={0}``. + + Unsupported values fall back to the documented default with a WARN + log so the operator gets observable behaviour instead of silent + breakage. + """ + from backend.app.core.settings import get_settings + + n = get_settings().relyloop_pr_poll_minutes + if n not in SUPPORTED_POLL_MINUTES: + logger.warning( + "pr_poll_minutes_unsupported", + configured=n, + falling_back_to=FALLBACK_POLL_MINUTES, + supported=sorted(SUPPORTED_POLL_MINUTES), + ) + n = FALLBACK_POLL_MINUTES + if n <= 60: + return {"minute": set(range(0, 60, n))} + return {"hour": set(range(0, 24, n // 60)), "minute": {0}} diff --git a/backend/workers/register_webhook.py b/backend/workers/register_webhook.py new file mode 100644 index 00000000..0dfc25ef --- /dev/null +++ b/backend/workers/register_webhook.py @@ -0,0 +1,236 @@ +"""Idempotent webhook auto-registration (feat_github_webhook Story 4.1 / FR-3). + +Enqueued by ``POST /api/v1/config-repos`` (Story 4.2) for any newly +created config_repo whose ``webhook_secret_ref`` is non-NULL. The job: + +1. Loads the config_repo by id. +2. Reads the PAT (``auth_ref``) and webhook secret (``webhook_secret_ref``) + from the mounted-secrets bundle. +3. Issues ``GET /repos/{owner}/{repo}/hooks?per_page=100`` and scans + for an existing hook whose ``config.url`` matches our webhook URL. +4. If found → clear any prior ``webhook_registration_error`` and exit + (status "exists"). Re-running the job for the same repo is a no-op. +5. Else → issue ``POST /repos/{owner}/{repo}/hooks`` with the + FR-3 payload. On 2xx → clear the error column. On 4xx/5xx or + ``RequestError``-after-budget → call + :func:`set_webhook_registration_error` with the documented + per-class message and return ``{"status":"failed"}``. + +The handler never raises into the Arq retry loop — failures are +**durable on the column**. ``arq.Retry``-style retries would just +churn through the same error class. The operator drives recovery via +the runbook (Story 4.3). +""" + +from __future__ import annotations + +from typing import Any + +import httpx +import structlog + +from backend.app.core.settings import get_settings +from backend.app.db import repo +from backend.app.db.session import get_session_factory +from backend.app.domain.git import UnsupportedProviderError, redact_token, validate_repo_url +from backend.app.git import HTTP_TIMEOUT_S, github_request, read_mounted_secret + +logger = structlog.get_logger(__name__) + + +def _hook_url() -> str | None: + """Return the URL GitHub will POST webhook deliveries to, or None. + + Constructed from ``Settings.relyloop_base_url``. ``None`` (the MVP1 + laptop default) → the job logs + sets a documented error so the + operator knows GitHub can't reach back without a tunnel. + """ + base = get_settings().relyloop_base_url + if not base: + return None + return f"{base.rstrip('/')}/webhooks/github" + + +async def _persist_error(config_repo_id: str, message: str) -> None: + """Best-effort UPDATE of ``webhook_registration_error``.""" + redacted = redact_token(message) + factory = get_session_factory() + try: + async with factory() as db: + await repo.set_webhook_registration_error(db, config_repo_id, redacted) + await db.commit() + except Exception as exc: # noqa: BLE001 — last-resort error path + logger.warning( + "register_webhook_persist_failed", + config_repo_id=config_repo_id, + error_type=type(exc).__name__, + ) + + +async def _clear_error(config_repo_id: str) -> None: + """Clear ``webhook_registration_error`` on a successful retry.""" + factory = get_session_factory() + try: + async with factory() as db: + await repo.set_webhook_registration_error(db, config_repo_id, None) + await db.commit() + except Exception as exc: # noqa: BLE001 — last-resort clear + logger.warning( + "register_webhook_clear_failed", + config_repo_id=config_repo_id, + error_type=type(exc).__name__, + ) + + +async def register_webhook(ctx: dict[str, Any], config_repo_id: str) -> dict[str, str]: + """Idempotent GitHub webhook creation for a single config_repo. + + Returns ``{"status": "created" | "exists" | "skipped" | "failed"}`` + for observability. Never raises; failures are persisted on the + config_repo's ``webhook_registration_error`` column. + """ + factory = get_session_factory() + async with factory() as db: + config_repo_row = await repo.get_config_repo(db, config_repo_id) + + if config_repo_row is None: + logger.warning("register_webhook_missing_row", config_repo_id=config_repo_id) + return {"status": "skipped"} + + if not config_repo_row.webhook_secret_ref: + # Defensive — POST /config-repos only enqueues us when this is set. + return {"status": "skipped"} + + try: + owner, repo_name = validate_repo_url(config_repo_row.repo_url) + except UnsupportedProviderError as exc: + await _persist_error( + config_repo_id, + f"Unsupported provider: {exc}", + ) + return {"status": "failed"} + + token = read_mounted_secret(config_repo_row.auth_ref) + if token is None: + await _persist_error( + config_repo_id, + f"PAT not configured: ./secrets/{config_repo_row.auth_ref} is missing or empty.", + ) + return {"status": "failed"} + + secret = read_mounted_secret(config_repo_row.webhook_secret_ref) + if secret is None: + await _persist_error( + config_repo_id, + ( + f"Webhook secret not configured: ./secrets/" + f"{config_repo_row.webhook_secret_ref} is missing or empty." + ), + ) + return {"status": "failed"} + + target_url = _hook_url() + if target_url is None: + await _persist_error( + config_repo_id, + ( + "RELYLOOP_BASE_URL is not configured; GitHub cannot reach this " + "install. Set Settings.relyloop_base_url and re-enqueue." + ), + ) + return {"status": "failed"} + + async with httpx.AsyncClient(timeout=HTTP_TIMEOUT_S) as client: + list_url = f"https://api.github.com/repos/{owner}/{repo_name}/hooks?per_page=100" + try: + list_response = await github_request(client, "GET", list_url, token=token) + except httpx.RequestError as exc: + await _persist_error( + config_repo_id, + f"GitHub unreachable — network error: {type(exc).__name__}", + ) + return {"status": "failed"} + + if list_response.status_code >= 400: + await _persist_error( + config_repo_id, + ( + f"GitHub returned {list_response.status_code} listing webhooks — " + f"check PAT scope (admin:repo_hook required)." + ), + ) + return {"status": "failed"} + + try: + existing_hooks = list_response.json() + except ValueError: + await _persist_error( + config_repo_id, + "GitHub returned non-JSON response listing webhooks.", + ) + return {"status": "failed"} + + if isinstance(existing_hooks, list): + for hook in existing_hooks: + hook_config = hook.get("config") if isinstance(hook, dict) else None + if isinstance(hook_config, dict) and hook_config.get("url") == target_url: + await _clear_error(config_repo_id) + logger.info( + "register_webhook_exists", + config_repo_id=config_repo_id, + hook_id=hook.get("id"), + ) + return {"status": "exists"} + + create_url = f"https://api.github.com/repos/{owner}/{repo_name}/hooks" + payload = { + "name": "web", + "active": True, + "events": ["pull_request"], + "config": { + "url": target_url, + "content_type": "json", + "secret": secret, + "insecure_ssl": "0", + }, + } + try: + create_response = await github_request( + client, "POST", create_url, json_body=payload, token=token + ) + except httpx.RequestError as exc: + await _persist_error( + config_repo_id, + f"GitHub unreachable — network error: {type(exc).__name__}", + ) + return {"status": "failed"} + + if create_response.status_code in (200, 201): + await _clear_error(config_repo_id) + logger.info( + "register_webhook_created", + config_repo_id=config_repo_id, + status_code=create_response.status_code, + ) + return {"status": "created"} + + # Documented per-class error messages (spec §13 NFR-Operability). + if create_response.status_code == 404: + error_msg = ( + "GitHub returned 404 — PAT lacks admin:repo_hook scope or the " + "configured repo doesn't exist." + ) + elif create_response.status_code == 422: + error_msg = ( + "GitHub returned 422 — webhook payload was rejected. " + "Inspect the hook config (URL reachable from GitHub?)." + ) + elif create_response.status_code >= 500: + error_msg = ( + f"GitHub returned {create_response.status_code} — transient. " + "Re-enqueue the job manually after GitHub's status recovers." + ) + else: + error_msg = f"GitHub returned {create_response.status_code} creating webhook." + await _persist_error(config_repo_id, error_msg) + return {"status": "failed"} diff --git a/docs/00_overview/MVP1_DASHBOARD.md b/docs/00_overview/MVP1_DASHBOARD.md index 4ddbf1ba..b2fabc1d 100644 --- a/docs/00_overview/MVP1_DASHBOARD.md +++ b/docs/00_overview/MVP1_DASHBOARD.md @@ -19,8 +19,8 @@ Plan approved; run /impl-execute to ship | Metric | Value | |---|---| | Features done | **9 / 13** (69%) | -| Path to MVP1 | **18** items remaining (features + bugs + chores) | -| Open bugs | 3 | +| Path to MVP1 | **19** items remaining (features + bugs + chores) | +| Open bugs | 4 | | Open chores | 11 (idea-stage debt) | | Backlog ideas | 4 idea-only feat/infra (not yet scoped into MVP1) | | In flight | 0 feature(s) actively shipping | @@ -59,7 +59,7 @@ _None._ | [feat_proposals_ui](../02_product/planned_features/feat_proposals_ui/feature_spec.md) | Feature | Two routes — `/proposals` (filterable list) and `/proposals/{id}` (config diff + metric delta + "Open PR" button + post-open PR-state mirror) — plug into the existing `feat_studies_ui` Next.js app. | `feat_studies_ui` `feat_digest_proposal` `feat_github_pr_worker` `feat_github_webhook` | Draft | | [chore_tutorial_polish](../02_product/planned_features/chore_tutorial_polish/feature_spec.md) | Chore | The release tag `v0.1.0` is pushed with: a worked tutorial at `docs/08_guides/tutorial-first-study.md`, sample data (50-query set + pre-baked judgment list + sample ES index of ~1,000 docs), README po | `feat_chat_agent` `feat_digest_proposal` `feat_github_pr_worker` `feat_github_webhook` `feat_llm_judgments` `feat_proposals_ui` `feat_studies_ui` `feat_study_lifecycle` `infra_adapter_elastic` `infra_arq_subprocess_test` `infra_ci_smoke_makeup` `infra_foundation` `infra_frontend_stack_refresh` `infra_nvmrc` `infra_optuna_eval` `infra_per_trial_timeout` | Draft | -### Idea (18) +### Idea (19) | Feature | Type | One-liner | Depends on | Status | |---|---|---|---|---| @@ -81,6 +81,7 @@ _None._ | [bug_capability_check_test_isolation](../02_product/planned_features/bug_capability_check_test_isolation/idea.md) | Bug | Idea (deferred from `infra_adapter_elastic` Story 5.1) | — | Idea (deferred from `infra_adapter_elastic` Story 5.1) | | [bug_digest_param_importance_seam](../02_product/planned_features/bug_digest_param_importance_seam/idea.md) | Bug | The test fixture builds its own `RDBStorage` via `build_storage(...)`, constructs sampler/pruner with `seed=42`, and calls `tell()` against THAT handle. The worker independently calls `build_storage(. | — | Idea (deferred from `feat_digest_proposal` Story 4.2; tracked because the test was marked `xfail` rather than fixed inline) | | [bug_env_file_corrupted_during_session](../02_product/planned_features/bug_env_file_corrupted_during_session/idea.md) | Bug | The user's working `.env` (containing the OpenAI API key referenced by [`CLAUDE.md`](../../../CLAUDE.md) "Cross-model review policy") was renamed to `.env.old` during the agent's implementation sessio | — | Idea — captured during `infra_foundation` Story 4.4 implementation | +| [bug_test_smoke_requires_env_vars](../02_product/planned_features/bug_test_smoke_requires_env_vars/idea.md) | Bug | `backend/tests/unit/test_smoke.py::test_app_import` fails when run without `DATABASE_URL_FILE` and `POSTGRES_PASSWORD_FILE` env vars in the test environment: | — | Idea — captured during `feat_github_webhook` `/impl-execute` | ## Dependency graph diff --git a/docs/00_overview/mvp1_dashboard.html b/docs/00_overview/mvp1_dashboard.html index de10b99f..6f1d4058 100644 --- a/docs/00_overview/mvp1_dashboard.html +++ b/docs/00_overview/mvp1_dashboard.html @@ -334,12 +334,12 @@

MVP1 Progress

Path to MVP1
-
18
+
19
items left = features + bugs + chores
Open bugs
-
3
+
4
tracked bug_* idea files
@@ -372,7 +372,7 @@

Pipeline

-

Idea 18

+

Idea 19

Arq Subprocess Test
@@ -587,6 +587,18 @@

Idea 18

The user's working `.env` (containing the OpenAI API key referenced by [`CLAUDE.md`](../../../CLAUDE.md) "Cross-model review policy") was renamed to `.env.old` during the agent's implementation sessio
+
+ + +
+
Test Smoke Requires Env Vars
+
+ Bug + +
+
`backend/tests/unit/test_smoke.py::test_app_import` fails when run without `DATABASE_URL_FILE` and `POSTGRES_PASSWORD_FILE` env vars in the test environment:
+ +
diff --git a/docs/02_product/mvp1-user-stories.md b/docs/02_product/mvp1-user-stories.md index 10748ff8..f32169de 100644 --- a/docs/02_product/mvp1-user-stories.md +++ b/docs/02_product/mvp1-user-stories.md @@ -65,8 +65,8 @@ ### `feat_github_webhook` — track PR state -- **US-20: See the PR state in the proposal UI.** *As a Relevance Engineer*, the proposal page shows the PR's current state (pr_opened → pr_merged → deployed), updated within 30s of a state change in GitHub, so I don't have to switch to GitHub to know whether my proposal landed. *(Source: §16 lines 1123–1150, §22 `/proposals/{id}`.)* -- **US-21: PR state survives webhook misses.** *As a Relevance Engineer*, even if a webhook delivery fails, a polling job reconciles PR state every 15 minutes, so the UI doesn't get permanently stuck on `pr_opened` for a PR that's already merged. *(Source: §16.)* +- **US-20: See the PR state in the proposal UI.** *(Implemented — `feat_github_webhook`)* *As a Relevance Engineer*, the proposal page shows the PR's current state (pr_opened → pr_merged → deployed), updated within 30s of a state change in GitHub, so I don't have to switch to GitHub to know whether my proposal landed. *(Source: §16 lines 1123–1150, §22 `/proposals/{id}`.)* +- **US-21: PR state survives webhook misses.** *(Implemented — `feat_github_webhook`)* *As a Relevance Engineer*, even if a webhook delivery fails, a polling job reconciles PR state every 15 minutes, so the UI doesn't get permanently stuck on `pr_opened` for a PR that's already merged. *(Source: §16.)* ### `feat_studies_ui` — manage studies in the browser diff --git a/docs/02_product/planned_features/bug_test_smoke_requires_env_vars/idea.md b/docs/02_product/planned_features/bug_test_smoke_requires_env_vars/idea.md new file mode 100644 index 00000000..cf511f42 --- /dev/null +++ b/docs/02_product/planned_features/bug_test_smoke_requires_env_vars/idea.md @@ -0,0 +1,53 @@ +# bug_test_smoke_requires_env_vars + +**Date:** 2026-05-12 +**Status:** Idea — captured during `feat_github_webhook` `/impl-execute` +**Origin:** Surfaced repeatedly while running `make test-unit` / +`pytest backend/tests/unit/` on the developer laptop during +`feat_github_webhook` implementation. Confirmed pre-existing on `main` +via `git stash` + `pytest backend/tests/unit/test_smoke.py::test_app_import`. + +## Problem + +`backend/tests/unit/test_smoke.py::test_app_import` fails when run +without `DATABASE_URL_FILE` and `POSTGRES_PASSWORD_FILE` env vars in +the test environment: + +``` +pydantic_core._pydantic_core.ValidationError: 2 validation errors for Settings +database_url_file + Field required [type=missing, input_value={}, input_type=dict] +postgres_password_file + Field required [type=missing, input_value={}, input_type=dict] +``` + +The other unit-test files that need Settings (e.g. `test_workers.py`, +`test_settings_pr_poll.py`, `test_git_pr_helpers.py`) all carry their +own `monkeypatch.setenv(...)` fixture pointing the two required-secret +files at `/dev/null` or a `tmp_path` file. `test_smoke.py` doesn't — +it just `from backend.app.main import app` cold, which triggers +`get_settings()` at module load via `_cors_origins`. + +## Why deferred + +* Out of scope for `feat_github_webhook` — this isn't a feature + regression, it's a pre-existing fragility. +* The failure doesn't block CI because CI runs the suite with the + required env vars populated (the GHA workflow sets them up before + `pytest`). +* Local developer experience is the only victim. The fix is one + fixture. + +## Proposed fix + +Add an autouse fixture to `backend/tests/unit/test_smoke.py` (or a +session-scoped fixture in `backend/tests/unit/conftest.py`) that sets +`DATABASE_URL_FILE` + `POSTGRES_PASSWORD_FILE` to a `tmp_path` file and +clears `get_settings.cache_clear()`. Mirror the pattern in +`backend/tests/unit/test_workers.py:_settings_env`. + +## Scope signals + +- backend/tests/unit/test_smoke.py: one new fixture +- No production-code changes +- 5-minute fix; ideally bundled with the next infra-sweep PR. diff --git a/docs/02_product/planned_features/feat_github_webhook/implementation_plan.md b/docs/02_product/planned_features/feat_github_webhook/implementation_plan.md index 37d5abde..a0b9cd91 100644 --- a/docs/02_product/planned_features/feat_github_webhook/implementation_plan.md +++ b/docs/02_product/planned_features/feat_github_webhook/implementation_plan.md @@ -1027,16 +1027,16 @@ All dependencies merged. No external blockers. ## 9) Execution tracker ### Current sprint -- [ ] Epic 1 — Story 1.1 (Migration `0006` + Settings field) -- [ ] Epic 1 — Story 1.2 (Signature verifier + URL normalizer) -- [ ] Epic 1 — Story 1.3 (Event dispatcher) -- [ ] Epic 1 — Story 1.4 (New repo functions) -- [ ] Epic 1 — Story 1.5 (Extract shared GitHub API client) -- [ ] Epic 2 — Story 2.1 (Webhook router + handler + tests) -- [ ] Epic 3 — Story 3.1 (`reconcile_pr_state` cron job + tests) -- [ ] Epic 4 — Story 4.1 (`register_webhook` worker) -- [ ] Epic 4 — Story 4.2 (Extend `POST /api/v1/config-repos`) -- [ ] Epic 4 — Story 4.3 (Runbook + docs + state flips) +- [x] Epic 1 — Story 1.1 (Migration `0006` + Settings field) — commit `c500a46` +- [x] Epic 1 — Story 1.2 (Signature verifier + URL normalizer) — commit `2287d6f` +- [x] Epic 1 — Story 1.3 (Event dispatcher) — commit `4f7ae9b` +- [x] Epic 1 — Story 1.4 (New repo functions) — commit `c532305` +- [x] Epic 1 — Story 1.5 (Extract shared GitHub API client) — commit `66ab068` +- [x] Epic 2 — Story 2.1 (Webhook router + handler + tests) — commit `2842a7d` +- [x] Epic 3 — Story 3.1 (`reconcile_pr_state` cron job + tests) — commit `4ec50ae` +- [x] Epic 4 — Story 4.1 (`register_webhook` worker) — commit `9bea914` +- [x] Epic 4 — Story 4.2 (Extend `POST /api/v1/config-repos`) — commit `1fcdf86` +- [x] Epic 4 — Story 4.3 (Runbook + docs + state flips) ### Blocked items - None. diff --git a/docs/03_runbooks/README.md b/docs/03_runbooks/README.md index 83113e30..b30242f6 100644 --- a/docs/03_runbooks/README.md +++ b/docs/03_runbooks/README.md @@ -19,13 +19,15 @@ Operational procedures: deployment, upgrades, restores, incident handling, recur - [`pr-open-debugging.md`](pr-open-debugging.md) — `open_pr` worker playbook + per-repo PAT rotation + closing orphan branches (`feat_github_pr_worker`) +- [`webhook-debugging.md`](webhook-debugging.md) — GitHub webhook + receiver + polling reconciler + `register_webhook` worker playbook + (`feat_github_webhook`) - [`ui-debugging.md`](ui-debugging.md) — inspecting the TanStack Query cache, reproducing polling regressions, tracing UI errors back to backend logs via `X-Request-ID` (`feat_studies_ui`) ## Coming with later features -- `webhook-replay.md` — replaying a missed GitHub webhook (lands with `feat_github_webhook`) - `db-revision-mismatch.md` — recovering when API startup detects pending migrations (lands at MVP2 when the startup guard activates) - `staging-deploy.md` — staging deploy procedure (lands at MVP3) - `incident-response.md` — on-call playbook (lands at GA v1) diff --git a/docs/03_runbooks/webhook-debugging.md b/docs/03_runbooks/webhook-debugging.md new file mode 100644 index 00000000..1af7cad3 --- /dev/null +++ b/docs/03_runbooks/webhook-debugging.md @@ -0,0 +1,185 @@ +# Webhook debugging — operator runbook + +Operational playbook for the GitHub webhook receiver (`POST /webhooks/github`), +the polling reconciler (`reconcile_pr_state` cron), and the auto-register worker +(`register_webhook` Arq job) introduced by `feat_github_webhook`. + +> Companion docs: [`pr-open-debugging.md`](pr-open-debugging.md) covers the +> `open_pr` worker; this runbook covers the *post-merge* lifecycle — +> webhook delivery, state reconciliation, and webhook auto-registration. + +--- + +## 1. Inspect the last delivery from GitHub + +GitHub's "Webhooks" panel on the config repo settings page lists every +delivery with its response code, headers, and body. + +1. Visit `https://github.com///settings/hooks` (operator + needs `admin:repo_hook` scope). +2. Click the configured RelyLoop hook. +3. Open the **Recent Deliveries** tab. Each row shows the response + code from `POST /webhooks/github`. A green checkmark means RelyLoop + responded `200`; a red `X` means non-`200`. +4. Click any delivery to inspect the raw request + response. The + request body is the GitHub payload; the response body is RelyLoop's + error envelope (e.g. `{"detail": {"error_code": "INVALID_SIGNATURE", ...}}`). + +## 2. Re-fire a webhook + +In the Recent Deliveries panel, click **Redeliver** on the failed +delivery. GitHub re-issues the exact same payload (same +`X-GitHub-Delivery` header). RelyLoop's handler is idempotent — repeat +deliveries of the same state transition match zero rows on the +conditional UPDATE and return `{"action": "applied"}` with no +secondary mutation. + +## 3. Verify the receive line in RelyLoop logs + +For every delivery that passes signature verification, the handler +emits one structured log line: + +``` +make logs | rg "webhook_received" +``` + +Example: + +```json +{ + "event": "webhook_received", + "delivery_id": "abc-123", + "event_type": "pull_request", + "action": "applied", + "proposal_id": "0190a83b-...", + "result": "applied" +} +``` + +For deliveries the handler rejected with `403 INVALID_SIGNATURE`: + +``` +make logs | rg "webhook_invalid_signature" +``` + +The `reason` field disambiguates: `bad_signature` (HMAC mismatch), or +`unknown_repo` (no `config_repos` row matches the +`repository.full_name`). + +## 4. Rotate the webhook secret + +The webhook HMAC secret lives at `./secrets/{config_repos.webhook_secret_ref}` +(mounted via Compose secrets). Rotation procedure: + +1. Generate a new secret: `openssl rand -hex 32 > ./secrets/`. +2. Update GitHub's hook config: + - Settings → Webhooks → click the hook → "Edit" + - Paste the new secret into the "Secret" field + - Save. +3. Update RelyLoop's `config_repos.webhook_secret_ref` (currently + requires a direct DB update; the MVP1 API doesn't expose a PATCH + endpoint): + ```sql + UPDATE config_repos + SET webhook_secret_ref = '' + WHERE id = ''; + ``` +4. Restart the API container so the new file gets re-read on the next + request: `make restart` (or `docker compose restart api`). +5. Trigger a redelivery from GitHub's webhook panel. Confirm the log + line shows `webhook_received` with `action="applied"` (or `ping` + for a ping event). + +## 5. Force-reconcile a specific proposal + +The polling reconciler runs every `RELYLOOP_PR_POLL_MINUTES` minutes +(default 15). To run a tick on demand: + +```bash +docker compose exec worker python -c " +import asyncio +from backend.workers.pr_reconcile import reconcile_pr_state +print(asyncio.run(reconcile_pr_state({}))) +" +``` + +The summary dict logs `{candidates, reconciled, unchanged, errored, +rate_limited}` so you can see whether the tick found your proposal. +If `candidates: 0`, the proposal is either: + +- Already in `pr_merged` / `rejected` (not a polling target). +- `pr_state IS NULL` (PR hasn't been opened yet — different worker). +- `created_at` is older than 90 days (FR-2 cap). +- Has no `pr_url` (operator-cancelled or never opened). + +## 6. Polling reconciler not running + +If the cron job seems silent: + +```bash +docker compose logs worker | rg "reconcile_pr_state|pr_reconcile" +``` + +Expected cadence: one `pr_reconcile_tick_complete` line every +`RELYLOOP_PR_POLL_MINUTES` minutes. If you see nothing: + +1. Confirm `Settings.relyloop_pr_poll_minutes` is in the whitelist + (`{1, 2, 3, 4, 5, 6, 10, 12, 15, 20, 30, 60, 120, 180, 240, 360, 720, 1440}`). + The Settings validator rejects out-of-set values at boot — check + `docker compose logs worker | rg ValidationError`. +2. Confirm Arq's cron supervisor is up: + `docker compose logs worker | rg "scheduled cron job"` — should + show `reconcile_pr_state` registered. +3. Confirm Redis is reachable: + `docker compose exec worker redis-cli -h redis ping` → `PONG`. + +## 7. `register_webhook` failed — operator triage + +When `POST /api/v1/config-repos` succeeds (201) but the auto-register +job failed, the failure surfaces on the `config_repos` row: + +``` +curl http://localhost:8000/api/v1/config-repos/ | jq .webhook_registration_error +``` + +Possible messages and their fixes: + +| Error message contains | Diagnosis | Fix | +|---|---|---| +| `admin:repo_hook` | PAT lacks the scope to create webhooks | Re-issue the PAT with `admin:repo_hook`, overwrite `./secrets/{auth_ref}`, re-enqueue (Story 8 below). | +| `422` validation failed | GitHub rejected the hook payload — usually because `Settings.relyloop_base_url` is unreachable from GitHub | If running on a laptop, expose via a tunnel (`ngrok http 8000` etc.) and re-set `RELYLOOP_BASE_URL` to the tunnel URL. | +| `transient` (5xx) | GitHub had a brief outage | Re-enqueue after https://www.githubstatus.com shows green. | +| `network error` | Worker host can't reach `api.github.com` | Check egress firewall + DNS. | +| `RELYLOOP_BASE_URL is not configured` | `Settings.relyloop_base_url` is `None` | Set the env var + restart the worker. | + +### Manual re-enqueue + +```bash +docker compose exec worker python -c " +import asyncio +from arq.connections import RedisSettings, create_pool +from backend.app.core.settings import get_settings + +async def main(): + pool = await create_pool(RedisSettings.from_dsn(get_settings().redis_url)) + await pool.enqueue_job('register_webhook', '') + await pool.close() + +asyncio.run(main()) +" +``` + +Wait ~5 seconds, then re-read the `webhook_registration_error` column. +It should be `null` on success or carry the new failure class. + +--- + +## Quick reference + +| Symptom | Logs to check | First action | +|---|---|---| +| Deliveries 403 on every event | `webhook_invalid_signature` | Rotate the webhook secret (§4) | +| State stuck at `pr_state=open` after merge | `webhook_received` for the delivery | Force-reconcile (§5) | +| Reconciler never runs | `pr_reconcile_tick_complete` | Check Settings whitelist (§6) | +| `webhook_registration_error` populated | n/a (column read) | Match per-class fix (§7) | +| GitHub can't reach the install | n/a | Tunnel + re-set `RELYLOOP_BASE_URL` | diff --git a/migrations/versions/0006_proposals_pr_url_idx.py b/migrations/versions/0006_proposals_pr_url_idx.py new file mode 100644 index 00000000..bdb9ce19 --- /dev/null +++ b/migrations/versions/0006_proposals_pr_url_idx.py @@ -0,0 +1,45 @@ +"""proposals_pr_url_idx. + +Revision ID: 0006 +Revises: 0005 +Create Date: 2026-05-12 12:00:00.000000 + +feat_github_webhook Story 1.1 — adds a partial B-tree index +``proposals_pr_url_idx`` on ``proposals(pr_url) WHERE pr_url IS NOT NULL`` to +support ``lookup_proposal_by_pr_url`` (the webhook receiver's single-row +proposal lookup keyed on the GitHub PR HTML URL). + +The index is partial so only the rows with a non-null ``pr_url`` are indexed +— pre-PR-open proposals don't carry a URL yet and would just bloat the +B-tree if included. + +Per CLAUDE.md Absolute Rule #5, this migration ships a ``downgrade()`` and +round-trips cleanly: ``alembic upgrade head && alembic downgrade -1 && +alembic upgrade head``. +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "0006" +down_revision: str | None = "0005" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Create the partial B-tree index on ``proposals.pr_url``.""" + op.create_index( + "proposals_pr_url_idx", + "proposals", + ["pr_url"], + postgresql_where=sa.text("pr_url IS NOT NULL"), + ) + + +def downgrade() -> None: + """Drop the partial B-tree index.""" + op.drop_index("proposals_pr_url_idx", table_name="proposals") diff --git a/state.md b/state.md index 5d34a1ec..1ad84506 100644 --- a/state.md +++ b/state.md @@ -2,21 +2,35 @@ > Read this first. Snapshots the active branch, what just shipped, what's in flight, what's queued, and where the project currently sits in the MVP1 → GA roadmap. Updated whenever a feature lands or a priority shifts. -**Last updated:** 2026-05-12 (after `feat_studies_ui` implementation complete; pending push + CI + merge on `feature/feat-studies-ui`) +**Last updated:** 2026-05-12 (after `feat_github_webhook` implementation complete; pending push + CI + merge on `feature/feat-github-webhook`) --- ## Current branch / execution context -- **Branch:** `feature/feat-studies-ui` — Next 16 UI shell + 7 surfaces (Dashboard, Clusters, Query Sets, Templates, Studies list + detail, Judgments detail). 13 stories across 4 epics shipped via `/impl-execute --all`. Pending push + CI + merge. -- **Active feature:** none in flight (post `feat_studies_ui`, next per the dashboard is `feat_github_webhook`). -- **Alembic head:** `0005_digests` (unchanged — UI-only feature). +- **Branch:** `feature/feat-github-webhook` — `POST /webhooks/github` receiver + 15-min polling reconciler cron + idempotent `register_webhook` Arq worker + extended `POST /api/v1/config-repos` to enqueue auto-registration. 10 stories across 4 epics shipped via `/impl-execute --all`. Pending push + CI + merge. +- **Active feature:** none in flight (post `feat_github_webhook`, next per the dashboard is `feat_chat_agent`). +- **Alembic head:** `0006_proposals_pr_url_idx` (partial B-tree index on `proposals(pr_url) WHERE pr_url IS NOT NULL` for the webhook receiver's lookup). - **Python:** 3.13. - **Frontend stack:** Next 16 (App Router + Turbopack), React 19, Tailwind 4 (CSS-first), Vitest 4, ESLint 9 (flat config), TypeScript 6, jsdom 29, Node engine `>=20.18`. - **Coverage:** UI tests at 122 passing (Vitest + jsdom + msw). Backend coverage unchanged (no backend code paths touched; one new contract test file for the source-of-truth helper). ## Most recent meaningful changes (newest first) +- **2026-05-12 — `feat_github_webhook` implementation complete** on `feature/feat-github-webhook`. 10 stories across 4 epics shipped via `/impl-execute --all`. **One migration (`0006_proposals_pr_url_idx`).** Stories breakdown: + - **Epic 1 (foundations, 5 stories):** Story 1.1 (migration `0006` + `Settings.relyloop_pr_poll_minutes` field bounded 1..1440); Story 1.2 (HMAC-SHA256 `verify_webhook_signature` + `parse_repository_full_name` short-form parser, pure-domain; deliberately separate from `validate_repo_url` so the URL regex isn't duplicated); Story 1.3 (`dispatch_event` pure-domain decision function — never emits `"unknown_pr"`, that's router-only); Story 1.4 (5 new proposal repo functions — `mark_proposal_pr_merged/closed/reopened`, `lookup_proposal_by_pr_url`, `list_pr_opened_proposals_for_reconcile`; 2 new config_repo repo functions — `set_webhook_registration_error`, `lookup_config_repo_by_owner_repo`); Story 1.5 (extracted `_github_post` + 4 inspection helpers from `workers/git_pr.py` to canonical `backend/app/git/github_client.py`; generalised to method-agnostic `github_request(client, method, url, *, json_body, token)`; hoisted `read_mounted_secret` from `git_pr._read_pat` to `backend/app/git/secrets.py`). + - **Epic 2 (webhook receiver, 1 story):** Story 2.1 — `POST /webhooks/github` unprefixed router (CLAUDE.md Rule #6 carve-out). 9-step contract: raw body → repository.full_name parse → config_repo lookup → mounted secret read → HMAC verify → dispatch → proposal lookup → conditional mutation → structured `webhook_received` log. Single error code `INVALID_SIGNATURE` (403) covers both signature mismatch AND unknown repo (no enumeration leak). Router-only `"unknown_pr"` override on lookup miss. + - **Epic 3 (polling reconciler, 1 story):** Story 3.1 — `reconcile_pr_state` cron job. Per-proposal flow: parse `pr_url` → `lookup_config_repo_by_owner_repo` → `read_mounted_secret(auth_ref)` → `GET pulls/{n}` via `github_request` → branch on `merged/state`. 401/403/404/5xx/RequestError-after-budget → WARN + skip. 429 → WARN + **break the proposal loop** for this tick (spec §10). `SUPPORTED_POLL_MINUTES` whitelist (18 values: divisors of 60 plus multiples of 60 that divide 1440) — Settings `@field_validator` rejects out-of-set at boot. + - **Epic 4 (auto-registration + docs, 3 stories):** Story 4.1 — `register_webhook` Arq job (idempotent; `GET /hooks` dedup before `POST /hooks`; per-class durable error messages — 404/422/5xx/network — on `webhook_registration_error` column; never raises into Arq retry). Story 4.2 — `POST /api/v1/config-repos` extended to best-effort enqueue `register_webhook` post-commit when `webhook_secret_ref` set; enqueue failure (Redis down / pool absent) does NOT break the 201. Story 4.3 — `docs/03_runbooks/webhook-debugging.md` operator playbook (7 sections: inspect GitHub delivery panel, redeliver, log verification, secret rotation, force-reconcile, cron debugging, register_webhook per-class triage); US-20 + US-21 flipped to Implemented. + - **Tests:** 7 unit files (signature, repo-name, URL parity, dispatch, settings, github_client, poll cron kwargs — including 20 cases parametrised over all 18 supported poll-minute values); 7 integration files (migration round-trip, 11 cases on proposal repo extensions, 6 cases on config_repo repo extensions, 13 cases on the webhook receiver covering merged/closed-unmerged/reopened/5 noop/unknown-event/ping/unknown_pr/bad-sig/missing-sig/unknown-repo/logging, 10 cases on the polling reconciler covering all 7 response paths + 429-short-circuit + RequestError + 50-proposal AC-8 budget, 6 cases on register_webhook covering happy/dedup/404/422/5xx/network, 5 cases on the POST /config-repos extension covering shape + enqueue + null + pool-absent + raise); 1 contract file (4 assertions: OpenAPI registration, WEBHOOK_ACTION_VALUES re-export, single-code negative grep, no-secret-in-log static grep). The 9 plan-listed webhook integration files were consolidated into one `test_webhook_github.py` with 13 test functions to keep the shared sign-and-seed helpers honest. + - **Key technical decisions during execution:** + - **9 plan-listed webhook integration test files → 1 consolidated file.** The plan was prescriptive for grep traceability; one file with 13 tests is easier to maintain and the shared signature/seed helpers don't drift across files. + - **`config_repos` has no `deleted_at` column** — the plan mentioned excluding soft-deleted rows in `lookup_config_repo_by_owner_repo` but the table has no such column. Implementation skips that filter; noted inline. + - **Webhook secret never logged.** Contract test `test_webhook_api_contract.py` has a static-grep audit asserting no `logger.(...)` call site in the router contains the bareword `secret` (allowing `webhook_secret_ref` as a substring so the column name can still be logged for debugging). + - **`secrets.read_mounted_secret` hoisted** from `git_pr._read_pat` so the webhook router + polling reconciler + register_webhook worker all share one containment-checked file reader. Story 1.5 deliverable, used immediately by Stories 2.1 / 3.1 / 4.1. + - **`github_request` test mocking via `httpx.MockTransport`** — the codebase's established pattern (test_request_retry.py, test_capability_check.py, test_elastic_health.py). The earlier plan draft cited cassettes (`pytest-recording`) but those aren't checked in yet for feat_github_pr_worker either. + - **Field validator narrows `relyloop_pr_poll_minutes` at boot** instead of silently snapping to default at the first cron tick. Operator sees the error in `make logs` rather than the cron silently never firing. + - **Tangential captured:** [`bug_test_smoke_requires_env_vars`](docs/02_product/planned_features/bug_test_smoke_requires_env_vars/idea.md) — pre-existing local-dev failure when running `test_smoke.py::test_app_import` without `DATABASE_URL_FILE` + `POSTGRES_PASSWORD_FILE` set. CI passes (env is present); only laptop developers hit it. 5-minute fixture fix. - **2026-05-12 — `feat_studies_ui` implementation complete** on `feature/feat-studies-ui`. 13 stories across 4 epics shipped end-to-end via `/impl-execute --all`. Zero migrations (UI-only). Stories breakdown: - **Epic 1 (foundations):** Story 1.1 (api-client.ts with X-Request-ID + 503-retryable backoff + 4-attempt retry contract + UUIDv7 + openapi-typescript-generated `lib/types.ts`); Story 1.2 (layout shell with QueryProvider + ThemeProvider + Toaster + TopNav, central error-toast wiring via QueryCache + MutationCache); Story 1.3 (StatusBadge / MetricDelta / CursorPaginator / EmptyState + canonical `lib/enums.ts` with 19 wire-value allowlists carrying source-of-truth comments). - **Epic 2 (clusters / query-sets / templates / judgments):** Story 2.1 (clusters list + register modal + detail w/ studies-by-cluster), 2.2 (query sets list + create + detail + associated-judgment-lists section + generate-judgments dialog), 2.3 (templates list + view-only detail + Fork-to-v+1 via parent_id round-trip; Prism-overlaid `TemplateBodyEditor`), 2.4 (judgment review header + paginated table + source filter chips + override `` + calibration modal with CSV/JSON parse + kappa display).