From 3b7d6b57758e2755d6e1715fd6edb9dd70ba0c62 Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 13:34:39 +0100 Subject: [PATCH 1/7] feat(github): add read-only GitHub App facts adapter Typed GitHubFactsAdapter port and GitHub App provider registered per ADR 0014. Reads pull request, classic branch-protection required checks, check runs and commit statuses with bounded retries and secret-free errors. No production caller yet (WS-GHPROOF-001-02). Co-Authored-By: Claude Opus 5.5 --- .ci/behavior-ownership/partition.v1.json | 10 +- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 139 ++++ backend/.env.example | 11 + backend/app/adapters/github/__init__.py | 67 ++ backend/app/adapters/github/app_facts.py | 617 ++++++++++++++++ backend/app/core/config.py | 41 ++ backend/app/interfaces/github_facts.py | 161 +++++ backend/scripts/behavior_ownership.py | 5 + backend/scripts/test_lane_catalogue.py | 1 + backend/tests/test_ci_lane_catalogue.py | 4 +- backend/tests/test_github_facts_adapter.py | 658 ++++++++++++++++++ 11 files changed, 1711 insertions(+), 3 deletions(-) create mode 100644 .commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md create mode 100644 backend/app/adapters/github/__init__.py create mode 100644 backend/app/adapters/github/app_facts.py create mode 100644 backend/app/interfaces/github_facts.py create mode 100644 backend/tests/test_github_facts_adapter.py diff --git a/.ci/behavior-ownership/partition.v1.json b/.ci/behavior-ownership/partition.v1.json index bf2650013..02689409f 100644 --- a/.ci/behavior-ownership/partition.v1.json +++ b/.ci/behavior-ownership/partition.v1.json @@ -52,6 +52,10 @@ "group": "artifacts", "target": "backend/app/adapters/checkers/external_service.py" }, + { + "group": "shared", + "target": "backend/app/adapters/github/app_facts.py" + }, { "group": "shared", "target": "backend/app/adapters/guide_document_tools/child.py" @@ -384,6 +388,10 @@ "group": "artifacts", "target": "backend/app/interfaces/external_services.py" }, + { + "group": "shared", + "target": "backend/app/interfaces/github_facts.py" + }, { "group": "shared", "target": "backend/app/interfaces/guide_document_tools.py" @@ -2377,7 +2385,7 @@ "target": "backend/scripts/validate_test_lane_evidence.py" } ], - "authority_digest": "8a42d5c62f07ce625a6f837dddeb7ae6e0f1c159bb74d81cb5f9820bf03c333f", + "authority_digest": "f90219378a47878660e3a30ae85c30fc2df8b6eef1d04d414a75ca2bc927f7d0", "protected_base_commit": "7676ce4347db0c9694962a9b587a20765e16eac6", "schema": "workstream.behavior-ownership-partition.v1" } diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md new file mode 100644 index 000000000..31a88f921 --- /dev/null +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -0,0 +1,139 @@ +# WS-GHPROOF-001-02 — GitHub facts port and GitHub App adapter + +- Initiative: WS-GHPROOF-001 +- Durable disposition: Complete +- Intended merge outcome: WorkStream has a typed, secret-safe GitHub facts capability that reads one pull request's merge state and base-branch required-check results as a read-only GitHub App, registered per ADR 0014 and not yet called by a production path. + +## Intent + +Chunk 04 must read GitHub facts inside the selected POST evaluation handler. +This chunk provides the bounded, tested capability it will call, so the wiring +chunk adds no transport, credential or retry logic of its own. + +## Current behavior + +- No GitHub client, port or setting exists under `backend/app`. +- `backend/app/interfaces/external_services.py` defines the ADR 0014 adapter + protocol, shared errors and `ExternalServiceAdapterFactory`. +- `SELECTED_POST_HANDLER_TIMEOUT_SECONDS` in + `backend/app/modules/checkers/api/external_policy.py` is 3,520 seconds + (3,400 s execution budget plus 60 s prelude and 60 s finalization margins). + +## Bounded change + +### Allowed + +- `backend/app/interfaces/github_facts.py`: capability and provider keys, the + frozen `workstream.github_facts.v1` model, typed errors and the + `GitHubFactsAdapter` port. +- `backend/app/adapters/github/__init__.py` (composition root) and + `backend/app/adapters/github/app_facts.py` (GitHub App provider). +- `backend/app/core/config.py` GitHub App settings and their validator; + commented examples in `backend/.env.example`. +- `backend/tests/test_github_facts_adapter.py`, its lane-catalogue entry and + shared-family fingerprint, and the behavior-ownership partition additions. + +### Not allowed + +- Any production caller, POST wiring, ART custody, checker change, migration, + API route or hosted secret configuration (chunks 03 to 06). + +## Design and decisions + +- **Authentication.** An RS256 App JWT (PyJWT, 60 s backdated, 9 min lifetime) + is used only to look up the repository installation (unless + `WORKSTREAM_GITHUB_APP_INSTALLATION_ID` is set) and to exchange an installation + token scoped to that one repository. Tokens are cached per repository until + five minutes before GitHub's `expires_at`. A 401 on a cached token forces one + fresh exchange; a second 401 is access denied. +- **Requests.** `GET /repos/{r}/pulls/{n}`; + `GET /repos/{r}/branches/{base}/protection/required_status_checks` for the + pull request's actual base branch; then `check-runs` and the combined + `status` for the head SHA, with `per_page=100` and `Link: rel="next"` + pagination up to 10 pages. A next link to another origin is a protocol error, + so a response cannot redirect the credential. Redirects are not followed. +- **Required checks.** Protection `checks` (context plus optional App id) are + used when present, otherwise `contexts`. A check restricted to an App matches + only that App's check runs; an unrestricted check matches check runs and + commit statuses by name. Every matching result must be `success`; otherwise + the first non-success value is reported. No match is `missing`; an incomplete + run is `pending`. `neutral` and `skipped` are not `success`. +- **Missing protection fails closed.** A 404 from the protection endpoint, or an + empty required-check list, raises `GitHubRequiredChecksNotConfiguredError` + (a non-retryable configuration error). Without required checks "CI passed" + has no meaning, and retrying cannot fix it. +- **Errors.** Retries exhausted or a rate-limit wait beyond the deadline: + `GitHubFactsUnavailableError` (retryable). Pull request 404: + `GitHubPullRequestNotFoundError`. 401/403 without rate-limit headers, or an + App not installed: `GitHubFactsAccessDeniedError`. Malformed bodies, another + repository's pull request or other 4xx: `GitHubFactsProtocolError`. Chunk 04 + decides how each maps to an infrastructure failure or a checker finding. +- **Retry budget.** Transport errors, 408, 5xx and rate limits retry up to + `WORKSTREAM_GITHUB_FACTS_MAXIMUM_ATTEMPTS` (default 3, max 5) per request, + with 0.5 s exponential backoff, honoring `Retry-After` and + `x-ratelimit-reset`. One facts read, all retries included, is bounded by + `WORKSTREAM_GITHUB_FACTS_TOTAL_DEADLINE_SECONDS` (default 45 s, max 120 s), + about 3% of the 3,520 s handler timeout at the maximum. Chunk 04 must place + this budget inside the handler prelude or reserve it from the execution + budget; the 60 s prelude margin alone is already partly used. +- **Credential custody.** The key is read once from an absolute file path + (bounded to 16 KiB) and held only as a parsed `RSAPrivateKey`. Tokens live + only in `_InstallationToken`, whose representation is redacted. Every + adapter error is raised `from None` and carries only its code. Retry logs + carry only status and attempt. Token-exchange bodies are never hashed or + returned; facts hold only GitHub data and response digests. +- **Facts.** `merge_commit_sha` is null unless merged, because GitHub reports a + test-merge commit for open pull requests. `responses` lists the SHA-256 of + each raw facts response body in request order. +- Rejected: following redirects or arbitrary `Link` origins; caching tokens + across repositories; treating missing protection as "no checks required"; + logging response bodies. + +## Acceptance criteria + +- [x] Happy path returns the exact facts, response digests, a verifiable JWT + and a repository-scoped token request. +- [x] Pagination is followed within the origin; a foreign link fails. +- [x] Missing protection, unknown pull request, wrong repository and access + denial raise their typed errors. +- [x] 5xx and transport errors retry then succeed; exhausted retries and + deadline-bounded rate limits raise `GitHubFactsUnavailableError`. +- [x] The key, JWT and tokens are absent from error strings, representations, + rendered tracebacks, log records and facts. +- [x] The factory registers the sole `github_app` provider; incomplete or unsafe + settings fail. + +## Risk and review routing + +- Risk class: L1, new external credential custody without a production caller. +- Required reviewers: security, senior engineering, QA. +- Human review focus: the GitHub App permission set (classic branch protection + reads need repository administration read), fail-closed missing protection, + strict `success` versus GitHub's own acceptance of `neutral`/`skipped`, and + the retry budget placement for chunk 04. + +## Evidence + +| Claim | Command or proof | Result | Remaining uncertainty | +|---|---|---|---| +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 39 passed | Uses `httpx.MockTransport`, not live GitHub | +| Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | +| Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | +| Settings | `tests/test_config.py` plus the new settings cases | Pass | None | +| Lint | `ruff check app tests scripts`; `docstr-coverage` | Pass (80.2%) | — | + +These ran on macOS in a `uv pip` environment resolved from `pyproject.toml`, +because `uv.lock` supports Linux only. Hosted Backend CI owns the locked run. + +## Review findings + +None recorded. + +## Reconciliation + +- Current-source reconciliation: based on WS-GHPROOF-001-01 over `main` after + PR #674. +- Roadmap: no impact. No production path calls the capability, so no + capability, exposure or dependency on `main` changes. +- Next usable boundary: chunk 04 POST wiring after PR #677 and chunk 03. +- Remaining risks: live GitHub behavior is unproven until chunk 06. diff --git a/backend/.env.example b/backend/.env.example index 97c7d2ab0..02e4822e7 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -118,3 +118,14 @@ WORKSTREAM_GUIDE_DOCUMENT_PARSER_SOCKET=/var/lib/workstream/guide-runtime/guide. # WORKSTREAM_ARTIFACT_ADMISSION_PROJECT_MAXIMUM_BYTES=1073741824 # WORKSTREAM_ARTIFACT_ADMISSION_DEPLOYMENT_MAXIMUM_BYTES=4294967296 # WORKSTREAM_ARTIFACT_SCRATCH_ROOT=/tmp/workstream-local-artifact-scratch + +# Read-only GitHub App for proof facts (WS-GHPROOF-001). Not called by any +# production path yet. Keep the private key in a file outside the repository; +# never put its contents in this file. +# WORKSTREAM_GITHUB_APP_ID=123456 +# WORKSTREAM_GITHUB_APP_PRIVATE_KEY_FILE=/run/secrets/workstream-github-app.pem +# WORKSTREAM_GITHUB_APP_INSTALLATION_ID=7654321 +# WORKSTREAM_GITHUB_API_BASE_URL=https://api.github.com +# WORKSTREAM_GITHUB_API_REQUEST_TIMEOUT_SECONDS=10.0 +# WORKSTREAM_GITHUB_FACTS_TOTAL_DEADLINE_SECONDS=45.0 +# WORKSTREAM_GITHUB_FACTS_MAXIMUM_ATTEMPTS=3 diff --git a/backend/app/adapters/github/__init__.py b/backend/app/adapters/github/__init__.py new file mode 100644 index 000000000..657089f98 --- /dev/null +++ b/backend/app/adapters/github/__init__.py @@ -0,0 +1,67 @@ +"""Composition root for the read-only GitHub facts capability (ADR 0014). + +No production path calls this yet. The selected POST evaluation wiring +(WS-GHPROOF-001 chunk 04) is its first intended caller. +""" + +from __future__ import annotations + +import httpx + +from app.adapters.github.app_facts import ( + GitHubAppFactsAdapter, + GitHubAppFactsConfiguration, + load_github_app_private_key, +) +from app.core.config import Settings +from app.interfaces.external_services import ( + ExternalServiceAdapterFactory, + ExternalServiceAdapterIdentity, + ExternalServiceConfigurationError, +) +from app.interfaces.github_facts import ( + GITHUB_APP_PROVIDER_KEY, + GITHUB_FACTS_CAPABILITY_KEY, + GitHubFactsAdapter, +) + + +def _github_app_adapter( + settings: Settings, transport: httpx.AsyncBaseTransport | None +) -> GitHubAppFactsAdapter: + """Construct the provider from complete settings or fail as configuration.""" + app_id = settings.github_app_id + key_file = settings.github_app_private_key_file + if app_id is None or key_file is None: + raise ExternalServiceConfigurationError( + ExternalServiceAdapterIdentity(GITHUB_FACTS_CAPABILITY_KEY, GITHUB_APP_PROVIDER_KEY) + ) + return GitHubAppFactsAdapter( + GitHubAppFactsConfiguration( + app_id=app_id, + installation_id=settings.github_app_installation_id, + api_base_url=settings.github_api_base_url, + request_timeout_seconds=settings.github_api_request_timeout_seconds, + total_deadline_seconds=settings.github_facts_total_deadline_seconds, + maximum_attempts=settings.github_facts_maximum_attempts, + ), + load_github_app_private_key(key_file), + transport=transport, + ) + + +def github_facts_adapter_factory( + settings: Settings, + *, + transport: httpx.AsyncBaseTransport | None = None, +) -> ExternalServiceAdapterFactory[GitHubFactsAdapter]: + """Register the sole GitHub App provider for the GitHub facts capability.""" + factory = ExternalServiceAdapterFactory[GitHubFactsAdapter](GITHUB_FACTS_CAPABILITY_KEY) + factory.register( + GITHUB_APP_PROVIDER_KEY, + lambda: _github_app_adapter(settings, transport), + ) + return factory + + +__all__ = ("github_facts_adapter_factory",) diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py new file mode 100644 index 000000000..866d58cac --- /dev/null +++ b/backend/app/adapters/github/app_facts.py @@ -0,0 +1,617 @@ +"""GitHub App adapter that reads pull-request proof facts with bounded retries. + +Credential custody: the RSA private key is held only as a parsed key object, +and installation tokens only inside ``_InstallationToken``. Neither is placed in +an exception, log record, representation or returned fact. Every adapter error +is raised ``from None`` so that no HTTP exception carrying request headers is +chained to it. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import Awaitable, Callable +from dataclasses import dataclass +from datetime import UTC, datetime +import hashlib +import logging +from pathlib import Path +import time +from typing import Any +from urllib.parse import quote, urlsplit + +import httpx +import jwt +from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey +from cryptography.hazmat.primitives.serialization import load_pem_private_key +from pydantic import ValidationError + +from app.interfaces.external_services import ( + ExternalServiceAdapterIdentity, + ExternalServiceConfigurationError, +) +from app.interfaces.github_facts import ( + GITHUB_APP_PROVIDER_KEY, + GITHUB_FACTS_CAPABILITY_KEY, + GitHubFactsAccessDeniedError, + GitHubFactsProtocolError, + GitHubFactsUnavailableError, + GitHubPullRequestFacts, + GitHubPullRequestNotFoundError, + GitHubRequiredCheckFact, + GitHubRequiredChecksNotConfiguredError, + GitHubResponseDigest, + is_github_repository, +) + +logger = logging.getLogger(__name__) + +GITHUB_API_VERSION = "2022-11-28" +MAXIMUM_PAGES = 10 +PAGE_SIZE = 100 +MAXIMUM_PRIVATE_KEY_BYTES = 16_384 +# GitHub accepts App JWTs for at most ten minutes; backdate for clock drift. +JWT_BACKDATE_SECONDS = 60 +JWT_LIFETIME_SECONDS = 540 +# Refresh an installation token this long before GitHub's stated expiry. +TOKEN_REFRESH_MARGIN_SECONDS = 300 +INITIAL_BACKOFF_SECONDS = 0.5 +_CHECK_RUN_CONCLUSIONS = frozenset( + { + "success", + "failure", + "neutral", + "cancelled", + "skipped", + "timed_out", + "action_required", + "stale", + } +) +_STATUS_STATES = frozenset({"success", "failure", "error", "pending"}) +_IDENTITY = ExternalServiceAdapterIdentity(GITHUB_FACTS_CAPABILITY_KEY, GITHUB_APP_PROVIDER_KEY) + + +class _InstallationToken: + """Installation access token whose value never appears in a representation.""" + + __slots__ = ("_value", "refresh_at") + + def __init__(self, value: str, refresh_at: float) -> None: + self._value = value + self.refresh_at = refresh_at + + def header(self) -> str: + """Return the Authorization header value for one GitHub request.""" + return f"Bearer {self._value}" + + def __repr__(self) -> str: + """Redact the token value.""" + return "_InstallationToken()" + + +@dataclass(frozen=True, slots=True) +class GitHubAppFactsConfiguration: + """Validated non-secret adapter limits and endpoints.""" + + app_id: int + installation_id: int | None + api_base_url: str + request_timeout_seconds: float + total_deadline_seconds: float + maximum_attempts: int + + +def load_github_app_private_key(path: Path) -> RSAPrivateKey: + """Read and parse one bounded PEM RSA key without exposing its bytes.""" + key: object = None + try: + with path.open("rb") as handle: + data = handle.read(MAXIMUM_PRIVATE_KEY_BYTES + 1) + if len(data) <= MAXIMUM_PRIVATE_KEY_BYTES: + key = load_pem_private_key(data, password=None) + except (OSError, ValueError, TypeError): + key = None + if not isinstance(key, RSAPrivateKey): + raise ExternalServiceConfigurationError(_IDENTITY) from None + return key + + +class _Deadline: + """Monotonic budget shared by every request and retry of one facts read.""" + + def __init__(self, seconds: float, monotonic: Callable[[], float]) -> None: + self._monotonic = monotonic + self._expires_at = monotonic() + seconds + + def remaining(self) -> float: + """Return the seconds left in this budget.""" + return self._expires_at - self._monotonic() + + +class GitHubAppFactsAdapter: + """Read one pull request's merge and required-check facts as a GitHub App.""" + + __slots__ = ( + "_configuration", + "_private_key", + "_transport", + "_wall_clock", + "_monotonic", + "_sleep", + "_tokens", + "_installations", + "_token_lock", + "_api_origin", + ) + + def __init__( + self, + configuration: GitHubAppFactsConfiguration, + private_key: RSAPrivateKey, + *, + transport: httpx.AsyncBaseTransport | None = None, + wall_clock: Callable[[], float] = time.time, + monotonic: Callable[[], float] = time.monotonic, + sleep: Callable[[float], Awaitable[None]] = asyncio.sleep, + ) -> None: + if not isinstance(private_key, RSAPrivateKey): + raise ExternalServiceConfigurationError(_IDENTITY) + self._configuration = configuration + self._private_key = private_key + self._transport = transport + self._wall_clock = wall_clock + self._monotonic = monotonic + self._sleep = sleep + self._tokens: dict[str, _InstallationToken] = {} + self._installations: dict[str, int] = {} + self._token_lock = asyncio.Lock() + parsed = urlsplit(configuration.api_base_url) + self._api_origin = (parsed.scheme, parsed.netloc) + + def __repr__(self) -> str: + """Show only the non-secret App id.""" + return f"GitHubAppFactsAdapter(app_id={self._configuration.app_id})" + + @property + def identity(self) -> ExternalServiceAdapterIdentity: + """Return the fixed GitHub App facts provider identity.""" + return _IDENTITY + + async def fetch_pull_request_facts( + self, + *, + repository: str, + pull_request_number: int, + ) -> GitHubPullRequestFacts: + """Read the pull request, base-branch required checks and head-SHA results.""" + if ( + not is_github_repository(repository) + or type(pull_request_number) is not int + or pull_request_number < 1 + ): + raise ExternalServiceConfigurationError(_IDENTITY) + deadline = _Deadline(self._configuration.total_deadline_seconds, self._monotonic) + digests: list[GitHubResponseDigest] = [] + async with httpx.AsyncClient( + transport=self._transport, + follow_redirects=False, + headers={ + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": GITHUB_API_VERSION, + "User-Agent": "workstream-github-facts", + }, + ) as client: + session = _FactsSession(self, client, repository, deadline, digests) + pull = await session.read_json( + "pull_request", + f"/repos/{repository}/pulls/{pull_request_number}", + not_found=GitHubPullRequestNotFoundError, + ) + pull_facts = _pull_request_facts(pull, repository, pull_request_number) + required = _required_checks( + await session.read_json( + "required_status_checks", + f"/repos/{repository}/branches/" + f"{quote(pull_facts['base_ref'], safe='')}" + "/protection/required_status_checks", + not_found=GitHubRequiredChecksNotConfiguredError, + ) + ) + head_sha = pull_facts["head_sha"] + check_runs = await session.read_pages( + "check_runs", + f"/repos/{repository}/commits/{head_sha}/check-runs", + item_key="check_runs", + ) + statuses = await session.read_pages( + "commit_status", + f"/repos/{repository}/commits/{head_sha}/status", + item_key="statuses", + ) + try: + return GitHubPullRequestFacts( + repository=pull_facts["repository"], + pull_request_number=pull_request_number, + state=pull_facts["state"], + merged=pull_facts["merged"], + merge_commit_sha=pull_facts["merge_commit_sha"], + base_ref=pull_facts["base_ref"], + head_sha=head_sha, + required_checks=tuple( + GitHubRequiredCheckFact( + name=name, + app_id=app_id, + conclusion=_conclusion(name, app_id, check_runs, statuses), + ) + for name, app_id in required + ), + fetched_at=datetime.fromtimestamp(self._wall_clock(), UTC), + responses=tuple(digests), + ) + except (ValidationError, KeyError, TypeError): + raise GitHubFactsProtocolError(_IDENTITY) from None + + # -- credentials ------------------------------------------------------- + + def _app_jwt(self) -> str: + """Sign a short-lived RS256 App JWT; it authenticates only token exchange.""" + now = int(self._wall_clock()) + return jwt.encode( + { + "iat": now - JWT_BACKDATE_SECONDS, + "exp": now + JWT_LIFETIME_SECONDS, + "iss": str(self._configuration.app_id), + }, + self._private_key, + algorithm="RS256", + ) + + def _cached_token(self, repository: str) -> _InstallationToken | None: + """Return a repository-scoped token that is not yet due for refresh.""" + token = self._tokens.get(repository.casefold()) + if token is None or self._wall_clock() >= token.refresh_at: + return None + return token + + def _forget_token(self, repository: str) -> None: + """Drop a token that GitHub rejected.""" + self._tokens.pop(repository.casefold(), None) + + async def _installation_token(self, session: _FactsSession) -> _InstallationToken: + """Return a cached repository-scoped token or exchange a fresh App JWT.""" + repository = session.repository + async with self._token_lock: + token = self._cached_token(repository) + if token is not None: + return token + installation_id = self._configuration.installation_id + if installation_id is None: + installation_id = self._installations.get(repository.casefold()) + if installation_id is None: + body = await session.app_json( + "GET", f"/repos/{repository}/installation", self._app_jwt() + ) + installation_id = body.get("id") if isinstance(body, dict) else None + if type(installation_id) is not int or installation_id < 1: + raise GitHubFactsProtocolError(_IDENTITY) + self._installations[repository.casefold()] = installation_id + body = await session.app_json( + "POST", + f"/app/installations/{installation_id}/access_tokens", + self._app_jwt(), + json_body={"repositories": [repository.split("/", 1)[1]]}, + ) + value = body.get("token") if isinstance(body, dict) else None + expires_at = body.get("expires_at") if isinstance(body, dict) else None + try: + expiry = datetime.fromisoformat(str(expires_at).replace("Z", "+00:00")) + except ValueError: + expiry = None + if ( + not isinstance(value, str) + or not value + or any(character.isspace() for character in value) + or expiry is None + or expiry.tzinfo is None + ): + raise GitHubFactsProtocolError(_IDENTITY) + token = _InstallationToken( + value, expiry.timestamp() - TOKEN_REFRESH_MARGIN_SECONDS + ) + self._tokens[repository.casefold()] = token + return token + + +class _FactsSession: + """One bounded facts read: shared deadline, retries and response digests.""" + + def __init__( + self, + adapter: GitHubAppFactsAdapter, + client: httpx.AsyncClient, + repository: str, + deadline: _Deadline, + digests: list[GitHubResponseDigest], + ) -> None: + self._adapter = adapter + self._client = client + self.repository = repository + self._deadline = deadline + self._digests = digests + + def _url(self, path_or_url: str) -> str: + """Resolve an API path against the configured origin.""" + if path_or_url.startswith("/"): + return self._adapter._configuration.api_base_url + path_or_url + return path_or_url + + async def app_json( + self, + method: str, + path: str, + app_jwt: str, + *, + json_body: dict[str, Any] | None = None, + ) -> object: + """Send one App-JWT request; its body may hold a token and is never hashed.""" + response = await self._send( + method, + self._url(path), + authorization=lambda: f"Bearer {app_jwt}", + json_body=json_body, + not_found=GitHubFactsAccessDeniedError, + installation=False, + ) + return _json(response) + + async def read_json( + self, + label: str, + path: str, + *, + not_found: type[Exception], + ) -> object: + """Read and hash one installation-token GET response.""" + response = await self._installation_get(self._url(path), not_found=not_found) + self._record(label, response) + return _json(response) + + async def read_pages(self, label: str, path: str, *, item_key: str) -> list[dict]: + """Follow same-origin ``next`` links for a bounded number of pages.""" + url = self._url(path) + f"?per_page={PAGE_SIZE}" + items: list[dict] = [] + for page in range(1, MAXIMUM_PAGES + 1): + response = await self._installation_get(url, not_found=GitHubFactsProtocolError) + self._record(f"{label}.page.{page}", response) + body = _json(response) + page_items = body.get(item_key) if isinstance(body, dict) else None + if not isinstance(page_items, list) or not all( + isinstance(item, dict) for item in page_items + ): + raise GitHubFactsProtocolError(_IDENTITY) + items.extend(page_items) + next_url = response.links.get("next", {}).get("url") + if next_url is None: + return items + parsed = urlsplit(next_url) + if (parsed.scheme, parsed.netloc) != self._adapter._api_origin: + raise GitHubFactsProtocolError(_IDENTITY) + url = next_url + raise GitHubFactsProtocolError(_IDENTITY) + + def _record(self, label: str, response: httpx.Response) -> None: + """Keep the SHA-256 of one raw facts response body.""" + digest = hashlib.sha256(response.content).hexdigest() + self._digests.append(GitHubResponseDigest(request=label, sha256=f"sha256:{digest}")) + + async def _installation_get( + self, url: str, *, not_found: type[Exception] + ) -> httpx.Response: + """GET with the installation token, refreshing a rejected token once.""" + token = await self._adapter._installation_token(self) + try: + return await self._send( + "GET", + url, + authorization=token.header, + json_body=None, + not_found=not_found, + installation=True, + ) + except _TokenRejected: + # A revoked or expired cached token gets one fresh exchange. + self._adapter._forget_token(self.repository) + token = await self._adapter._installation_token(self) + try: + return await self._send( + "GET", + url, + authorization=token.header, + json_body=None, + not_found=not_found, + installation=True, + ) + except _TokenRejected: + raise GitHubFactsAccessDeniedError(_IDENTITY) from None + + async def _send( + self, + method: str, + url: str, + *, + authorization: Callable[[], str], + json_body: dict[str, Any] | None, + not_found: type[Exception], + installation: bool, + ) -> httpx.Response: + """Send with bounded retries for transport errors, 5xx and rate limits.""" + configuration = self._adapter._configuration + for attempt in range(1, configuration.maximum_attempts + 1): + remaining = self._deadline.remaining() + if remaining <= 0: + break + timeout = min(configuration.request_timeout_seconds, remaining) + response: httpx.Response | None = None + try: + response = await self._client.request( + method, + url, + json=json_body, + headers={"Authorization": authorization()}, + timeout=timeout, + ) + except httpx.TransportError: + response = None + delay = INITIAL_BACKOFF_SECONDS * (2 ** (attempt - 1)) + if response is not None: + status = response.status_code + if 200 <= status < 300: + return response + if status == 404: + raise not_found(_IDENTITY) from None + if status == 401: + if installation: + raise _TokenRejected() + raise GitHubFactsAccessDeniedError(_IDENTITY) from None + rate_limit_delay = _rate_limit_delay(response, self._adapter._wall_clock()) + if status in {403, 429} and rate_limit_delay is not None: + delay = max(delay, rate_limit_delay) + elif status == 403: + raise GitHubFactsAccessDeniedError(_IDENTITY) from None + elif status != 408 and status < 500: + raise GitHubFactsProtocolError(_IDENTITY) from None + logger.warning( + "github_facts_request_retryable status=%s attempt=%s", + None if response is None else response.status_code, + attempt, + ) + if attempt == configuration.maximum_attempts: + break + if delay >= self._deadline.remaining(): + break + await self._adapter._sleep(delay) + raise GitHubFactsUnavailableError(_IDENTITY) from None + + +class _TokenRejected(Exception): + """Internal signal that GitHub rejected an installation token.""" + + +def _json(response: httpx.Response) -> object: + """Parse a JSON body or fail as a protocol error.""" + try: + return response.json() + except ValueError: + raise GitHubFactsProtocolError(_IDENTITY) from None + + +def _rate_limit_delay(response: httpx.Response, now: float) -> float | None: + """Return GitHub's requested wait for a primary or secondary rate limit.""" + retry_after = response.headers.get("retry-after") + if retry_after is not None: + try: + return max(0.0, float(int(retry_after))) + except ValueError: + return None + if response.headers.get("x-ratelimit-remaining") == "0": + try: + return max(0.0, float(int(response.headers["x-ratelimit-reset"])) - now) + except (KeyError, ValueError): + return None + return None + + +def _pull_request_facts(body: object, repository: str, number: int) -> dict[str, Any]: + """Extract the pull request fields and confirm it belongs to the repository.""" + try: + if not isinstance(body, dict): + raise TypeError + base = body["base"] + head = body["head"] + full_name = base["repo"]["full_name"] + merged = body["merged"] + values = { + "repository": full_name, + "state": body["state"], + "merged": merged, + "merge_commit_sha": body["merge_commit_sha"] if merged is True else None, + "base_ref": base["ref"], + "head_sha": head["sha"], + } + valid = ( + body["number"] == number + and isinstance(full_name, str) + and full_name.casefold() == repository.casefold() + and isinstance(values["base_ref"], str) + and isinstance(values["head_sha"], str) + ) + except (KeyError, TypeError): + valid = False + if not valid: + raise GitHubFactsProtocolError(_IDENTITY) from None + return values + + +def _required_checks(body: object) -> tuple[tuple[str, int | None], ...]: + """Return required (context, app id) pairs; none configured fails closed.""" + if not isinstance(body, dict): + raise GitHubFactsProtocolError(_IDENTITY) + checks = body.get("checks") + required: dict[tuple[str, int | None], None] = {} + if isinstance(checks, list) and checks: + for item in checks: + context = item.get("context") if isinstance(item, dict) else None + app_id = item.get("app_id") if isinstance(item, dict) else None + if not isinstance(context, str) or not context: + raise GitHubFactsProtocolError(_IDENTITY) + if app_id is not None and (type(app_id) is not int or app_id < 1): + # GitHub reports -1 or null for "any source". + app_id = None + required[(context, app_id)] = None + else: + contexts = body.get("contexts") + if not isinstance(contexts, list): + raise GitHubFactsProtocolError(_IDENTITY) + for context in contexts: + if not isinstance(context, str) or not context: + raise GitHubFactsProtocolError(_IDENTITY) + required[(context, None)] = None + if not required: + raise GitHubRequiredChecksNotConfiguredError(_IDENTITY) + return tuple(sorted(required, key=lambda item: (item[0], item[1] or 0))) + + +def _conclusion( + name: str, + app_id: int | None, + check_runs: list[dict], + statuses: list[dict], +) -> str: + """Combine every matching check run and commit status; all must succeed.""" + observed: list[str] = [] + for run in check_runs: + app = run.get("app") + run_app_id = app.get("id") if isinstance(app, dict) else None + if run.get("name") != name or (app_id is not None and run_app_id != app_id): + continue + if run.get("status") != "completed": + observed.append("pending") + continue + conclusion = run.get("conclusion") + if conclusion not in _CHECK_RUN_CONCLUSIONS: + raise GitHubFactsProtocolError(_IDENTITY) + observed.append(conclusion) + if app_id is None: + # Commit statuses carry no App identity, so they satisfy only + # requirements that accept any source. + for status in statuses: + if status.get("context") != name: + continue + state = status.get("state") + if state not in _STATUS_STATES: + raise GitHubFactsProtocolError(_IDENTITY) + observed.append(state) + if not observed: + return "missing" + return next((value for value in observed if value != "success"), "success") diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 9ca7a9a0b..c819e5a19 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -287,6 +287,17 @@ class Settings(BaseSettings): gt=0.0, le=3700.0, ) + # Read-only GitHub App used to read pull-request proof facts. The private + # key stays in a file; settings hold only its absolute path. + github_app_id: int | None = Field(default=None, ge=1) + github_app_private_key_file: Path | None = None + github_app_installation_id: int | None = Field(default=None, ge=1) + github_api_base_url: str = "https://api.github.com" + github_api_request_timeout_seconds: float = Field(default=10.0, gt=0.0, le=30.0) + # One facts read, including every retry, stays far inside the selected POST + # handler budget (SELECTED_POST_HANDLER_TIMEOUT_SECONDS). + github_facts_total_deadline_seconds: float = Field(default=45.0, gt=0.0, le=120.0) + github_facts_maximum_attempts: int = Field(default=3, ge=1, le=5) artifact_pending_work_scan_interval_seconds: int = Field(default=60, gt=0, le=3600) artifact_pending_work_scan_page_size: int = Field(default=100, gt=0, le=1000) @@ -561,6 +572,36 @@ def _validate_artifact_s3_secret_contract(self) -> None: secrets = _EMPTY_ARTIFACT_S3_SECRETS raise ValueError("native AWS S3 rejects static credentials") + @model_validator(mode="after") + def validate_github_app(self) -> Settings: + """Require a complete GitHub App identity and a credential-safe API origin.""" + if (self.github_app_id is None) != (self.github_app_private_key_file is None): + raise ValueError("GitHub App id and private key file must be configured together") + if self.github_app_installation_id is not None and self.github_app_id is None: + raise ValueError("GitHub App installation id requires a GitHub App id") + key_file = self.github_app_private_key_file + if key_file is not None and not key_file.is_absolute(): + raise ValueError("GitHub App private key file must be an absolute path") + parsed = urlsplit(self.github_api_base_url) + loopback_http = ( + parsed.scheme == "http" + and parsed.hostname in {"127.0.0.1", "localhost", "::1"} + and self.environment in {"local", "dev", "development", "test"} + ) + if ( + (parsed.scheme != "https" and not loopback_http) + or not parsed.hostname + or parsed.username is not None + or parsed.password is not None + or parsed.query + or parsed.fragment + or self.github_api_base_url.endswith("/") + ): + raise ValueError("GitHub API base URL must be a credential-free HTTPS origin") + if self.github_api_request_timeout_seconds > self.github_facts_total_deadline_seconds: + raise ValueError("GitHub request timeout cannot exceed the facts deadline") + return self + @model_validator(mode="after") def validate_project_agent_runtime(self) -> Settings: """Reject partial no-inference identities and invalid adapter options.""" diff --git a/backend/app/interfaces/github_facts.py b/backend/app/interfaces/github_facts.py new file mode 100644 index 000000000..170b1c8e3 --- /dev/null +++ b/backend/app/interfaces/github_facts.py @@ -0,0 +1,161 @@ +"""Typed GitHub pull-request facts capability for proof comparison. + +A trusted WorkStream owner reads these facts outside the checker sandbox. The +networkless checker receives them as request input and compares them with the +contributor's claims. The facts never contain a credential. +""" + +from __future__ import annotations + +from datetime import datetime +import re +from typing import Annotated, Literal, Protocol + +from pydantic import ( + AfterValidator, + BaseModel, + ConfigDict, + Field, + StrictBool, + StrictInt, + StrictStr, +) + +from app.interfaces.external_services import ( + ExternalServiceAdapter, + ExternalServiceAdapterError, + ExternalServiceConfigurationError, + ExternalServiceProtocolError, + ExternalServiceUnavailableError, +) + + +GITHUB_FACTS_CAPABILITY_KEY = "github_facts" +GITHUB_APP_PROVIDER_KEY = "github_app" +GITHUB_FACTS_SCHEMA_VERSION = "workstream.github_facts.v1" + +# GitHub owner logins are at most 39 characters; repository names at most 100. +_GITHUB_REPOSITORY = re.compile( + r"^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})/(?!\.{1,2}$)[A-Za-z0-9._-]{1,100}$" +) + +GitSha = Annotated[StrictStr, Field(pattern=r"^[0-9a-f]{40}$")] +Sha256 = Annotated[StrictStr, Field(pattern=r"^sha256:[0-9a-f]{64}$")] +# Check-run conclusions and commit-status states, plus two derived values: +# ``pending`` (not completed) and ``missing`` (no matching check or status). +GitHubCheckConclusion = Literal[ + "success", + "failure", + "neutral", + "cancelled", + "skipped", + "timed_out", + "action_required", + "stale", + "error", + "pending", + "missing", +] + + +def is_github_repository(value: object) -> bool: + """Return whether a value is one canonical ``owner/name`` repository.""" + return isinstance(value, str) and _GITHUB_REPOSITORY.fullmatch(value) is not None + + +def _github_repository(value: str) -> str: + """Validate one canonical repository name.""" + if not is_github_repository(value): + raise ValueError("GitHub repository must be owner/name") + return value + + +GitHubRepository = Annotated[StrictStr, AfterValidator(_github_repository)] + + +class _FactsValue(BaseModel): + """Strict immutable facts value.""" + + model_config = ConfigDict(extra="forbid", frozen=True, revalidate_instances="always") + + +class GitHubRequiredCheckFact(_FactsValue): + """One check required by base-branch protection and its head-SHA result.""" + + name: Annotated[StrictStr, Field(min_length=1, max_length=255)] + app_id: StrictInt | None + conclusion: GitHubCheckConclusion + + +class GitHubResponseDigest(_FactsValue): + """SHA-256 of one raw GitHub response body that produced these facts.""" + + request: Annotated[StrictStr, Field(pattern=r"^[a-z_]+(?:\.page\.[1-9][0-9]*)?$")] + sha256: Sha256 + + +class GitHubPullRequestFacts(_FactsValue): + """Immutable ``workstream.github_facts.v1`` value for one pull request. + + ``merge_commit_sha`` is null unless the pull request is merged, because + GitHub reports a test-merge commit for open pull requests. Serialize with + ``model_dump(mode="json")`` to place the value in checker request input. + """ + + schema_version: Literal["workstream.github_facts.v1"] = GITHUB_FACTS_SCHEMA_VERSION + repository: GitHubRepository + pull_request_number: Annotated[StrictInt, Field(ge=1)] + state: Literal["open", "closed"] + merged: StrictBool + merge_commit_sha: GitSha | None + base_ref: Annotated[StrictStr, Field(min_length=1, max_length=255)] + head_sha: GitSha + required_checks: tuple[GitHubRequiredCheckFact, ...] = Field(min_length=1) + fetched_at: datetime + responses: tuple[GitHubResponseDigest, ...] = Field(min_length=1) + + +class GitHubFactsUnavailableError(ExternalServiceUnavailableError): + """GitHub stayed unavailable or rate-limited within the bounded retry budget.""" + + code = "github_facts_unavailable" + + +class GitHubFactsAccessDeniedError(ExternalServiceConfigurationError): + """The GitHub App credential, installation or permission is not usable.""" + + code = "github_facts_access_denied" + + +class GitHubRequiredChecksNotConfiguredError(ExternalServiceConfigurationError): + """The base branch has no classic branch-protection required checks. + + Without required checks, "CI passed" has no meaning, so facts fail closed. + """ + + code = "github_required_checks_not_configured" + + +class GitHubPullRequestNotFoundError(ExternalServiceAdapterError): + """The repository has no pull request with the requested number.""" + + code = "github_pull_request_not_found" + category = "not_found" + + +class GitHubFactsProtocolError(ExternalServiceProtocolError): + """GitHub returned a response outside the supported facts protocol.""" + + code = "github_facts_protocol_invalid" + + +class GitHubFactsAdapter(ExternalServiceAdapter, Protocol): + """Read verified facts about one pull request with a read-only credential.""" + + async def fetch_pull_request_facts( + self, + *, + repository: str, + pull_request_number: int, + ) -> GitHubPullRequestFacts: + """Return facts or raise a secret-free ``ExternalServiceAdapterError``.""" diff --git a/backend/scripts/behavior_ownership.py b/backend/scripts/behavior_ownership.py index 12101da93..a602b2699 100644 --- a/backend/scripts/behavior_ownership.py +++ b/backend/scripts/behavior_ownership.py @@ -874,6 +874,10 @@ "backend/app/modules/authorization/domain/project_catalogue.py", "backend/app/modules/authorization/project_catalogue.py", }) +GITHUB_PROOF_FACTS_TARGETS = frozenset({ + "backend/app/adapters/github/app_facts.py", + "backend/app/interfaces/github_facts.py", +}) class BehaviorOwnershipError(RuntimeError): @@ -1144,6 +1148,7 @@ def _validate_additive_partition_transition( | PILOT_02B_ATOMIC_IMPORT_TARGETS | PILOT_02C_BULK_TRANSITION_TARGETS | PROJECT_CATALOGUE_TARGETS + | GITHUB_PROOF_FACTS_TARGETS ) expected_additions = (approved_additions & additions) - set(trusted_targets) if POL_03A_DECLARATIVE_MODEL_TARGET in additions: diff --git a/backend/scripts/test_lane_catalogue.py b/backend/scripts/test_lane_catalogue.py index df7494ee3..d7b4af787 100644 --- a/backend/scripts/test_lane_catalogue.py +++ b/backend/scripts/test_lane_catalogue.py @@ -97,6 +97,7 @@ class TestLane: "tests/test_checker_image.py", "tests/test_checker_smol_cache.py", "tests/checkers/external/test_harnessy_proof_checker.py", + "tests/test_github_facts_adapter.py", "tests/test_api_controls.py", "tests/test_identifiers.py", "tests/test_identifier_schema.py", diff --git a/backend/tests/test_ci_lane_catalogue.py b/backend/tests/test_ci_lane_catalogue.py index 58c394568..dea2d6522 100644 --- a/backend/tests/test_ci_lane_catalogue.py +++ b/backend/tests/test_ci_lane_catalogue.py @@ -268,8 +268,8 @@ def test_semantic_family_membership_has_an_independent_fingerprint() -> None: """A catalogue-internal move across semantic families must fail this contract.""" expected = { "shared": ( - 260, - "2fe14cb5b5987c540fc30a153a2429adc16243b55aca0e565892338c306282dd", + 261, + "d903ceb93c8f54c401100a58226909458c4d41d10fd618421d8d0d1ec78f0c07", ), "project": ( 213, diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py new file mode 100644 index 000000000..abdf121eb --- /dev/null +++ b/backend/tests/test_github_facts_adapter.py @@ -0,0 +1,658 @@ +"""GitHub App facts adapter: facts, bounded retries and credential custody.""" + +from __future__ import annotations + +import hashlib +import json +import logging +from pathlib import Path +import traceback + +import httpx +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from app.adapters.github import github_facts_adapter_factory +from app.adapters.github.app_facts import ( + GitHubAppFactsAdapter, + GitHubAppFactsConfiguration, + _InstallationToken, +) +from app.core.config import Settings +from app.interfaces.external_services import ( + ExternalServiceAdapterError, + ExternalServiceConfigurationError, +) +from app.interfaces.github_facts import ( + GitHubFactsAccessDeniedError, + GitHubFactsProtocolError, + GitHubFactsUnavailableError, + GitHubPullRequestFacts, + GitHubPullRequestNotFoundError, + GitHubRequiredChecksNotConfiguredError, +) + +API = "https://api.github.test" +REPOSITORY = "acme/widgets" +HEAD_SHA = "a" * 40 +MERGE_SHA = "b" * 40 +TOKEN = "ghs_installationTokenSecretValue0001" +SECOND_TOKEN = "ghs_installationTokenSecretValue0002" +NOW = 1_800_000_000.0 +APP_ID = 4242 + +PRIVATE_KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048) +PRIVATE_PEM = PRIVATE_KEY.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), +).decode() +PEM_BODY_LINE = PRIVATE_PEM.splitlines()[1] + + +def _pull(**changes: object) -> dict: + body = { + "number": 12, + "state": "closed", + "merged": True, + "merge_commit_sha": MERGE_SHA, + "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, + "head": {"sha": HEAD_SHA}, + } + body.update(changes) + return body + + +class FakeGitHub: + """Route-table GitHub fake that records every request it receives.""" + + def __init__(self) -> None: + self.requests: list[httpx.Request] = [] + self.bodies: dict[str, bytes] = {} + self.tokens = [TOKEN, SECOND_TOKEN] + self.routes: dict[tuple[str, str], list] = {} + self.route("GET", f"/repos/{REPOSITORY}/installation", {"id": 77}) + self.route("POST", "/app/installations/77/access_tokens", self._token) + self.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull()) + self.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + { + "strict": True, + "contexts": ["backend", "lint"], + "checks": [ + {"context": "backend", "app_id": 15368}, + {"context": "lint", "app_id": None}, + ], + }, + ) + self.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + { + "total_count": 1, + "check_runs": [ + { + "name": "backend", + "status": "completed", + "conclusion": "success", + "app": {"id": 15368}, + } + ], + }, + ) + self.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", + {"state": "success", "statuses": [{"context": "lint", "state": "success"}]}, + ) + + def route(self, method: str, path: str, *responses: object) -> None: + """Queue responses; the last one repeats once the queue is exhausted.""" + self.routes[(method, path)] = list(responses) + + def _token(self, _request: httpx.Request) -> httpx.Response: + return httpx.Response( + 201, + json={"token": self.tokens.pop(0), "expires_at": "2027-01-15T09:00:00Z"}, + ) + + def handler(self, request: httpx.Request) -> httpx.Response: + self.requests.append(request) + queue = self.routes.get((request.method, request.url.path)) + if queue is None: + return httpx.Response(404, json={"message": "Not Found"}) + item = queue.pop(0) if len(queue) > 1 else queue[0] + if isinstance(item, Exception): + raise item + if callable(item): + response = item(request) + elif isinstance(item, httpx.Response): + response = item + else: + response = httpx.Response(200, json=item) + self.bodies[str(request.url)] = response.content + return response + + def secrets(self) -> list[str]: + """Every credential the adapter sent, plus the issued tokens.""" + sent = [ + request.headers["authorization"].removeprefix("Bearer ") + for request in self.requests + if "authorization" in request.headers + ] + return [*sent, TOKEN, SECOND_TOKEN, PEM_BODY_LINE] + + +class Clock: + def __init__(self) -> None: + self.now = 0.0 + self.sleeps: list[float] = [] + + def monotonic(self) -> float: + return self.now + + async def sleep(self, seconds: float) -> None: + self.sleeps.append(seconds) + self.now += seconds + + +def _adapter( + fake: FakeGitHub, + clock: Clock | None = None, + *, + installation_id: int | None = None, + deadline: float = 45.0, + attempts: int = 3, +) -> GitHubAppFactsAdapter: + clock = clock or Clock() + return GitHubAppFactsAdapter( + GitHubAppFactsConfiguration( + app_id=APP_ID, + installation_id=installation_id, + api_base_url=API, + request_timeout_seconds=10.0, + total_deadline_seconds=deadline, + maximum_attempts=attempts, + ), + PRIVATE_KEY, + transport=httpx.MockTransport(fake.handler), + wall_clock=lambda: NOW, + monotonic=clock.monotonic, + sleep=clock.sleep, + ) + + +async def _fetch(adapter: GitHubAppFactsAdapter) -> GitHubPullRequestFacts: + return await adapter.fetch_pull_request_facts(repository=REPOSITORY, pull_request_number=12) + + +def _sha(content: bytes) -> str: + return "sha256:" + hashlib.sha256(content).hexdigest() + + +async def test_happy_path_returns_facts_with_response_digests() -> None: + fake = FakeGitHub() + facts = await _fetch(_adapter(fake)) + + assert facts.model_dump(mode="json") == { + "schema_version": "workstream.github_facts.v1", + "repository": REPOSITORY, + "pull_request_number": 12, + "state": "closed", + "merged": True, + "merge_commit_sha": MERGE_SHA, + "base_ref": "main", + "head_sha": HEAD_SHA, + "required_checks": [ + {"name": "backend", "app_id": 15368, "conclusion": "success"}, + {"name": "lint", "app_id": None, "conclusion": "success"}, + ], + "fetched_at": "2027-01-15T08:00:00Z", + "responses": [ + { + "request": "pull_request", + "sha256": _sha(fake.bodies[f"{API}/repos/{REPOSITORY}/pulls/12"]), + }, + { + "request": "required_status_checks", + "sha256": _sha( + fake.bodies[ + f"{API}/repos/{REPOSITORY}/branches/main/protection/" + "required_status_checks" + ] + ), + }, + { + "request": "check_runs.page.1", + "sha256": _sha( + fake.bodies[ + f"{API}/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs?per_page=100" + ] + ), + }, + { + "request": "commit_status.page.1", + "sha256": _sha( + fake.bodies[f"{API}/repos/{REPOSITORY}/commits/{HEAD_SHA}/status?per_page=100"] + ), + }, + ], + } + lookup, exchange, *facts_requests = fake.requests + claims = jwt.decode( + lookup.headers["authorization"].removeprefix("Bearer "), + PRIVATE_KEY.public_key(), + algorithms=["RS256"], + options={"verify_exp": False, "verify_iat": False}, + ) + assert claims == {"iss": str(APP_ID), "iat": int(NOW) - 60, "exp": int(NOW) + 540} + assert json.loads(exchange.content) == {"repositories": ["widgets"]} + assert {request.headers["authorization"] for request in facts_requests} == { + f"Bearer {TOKEN}" + } + assert all(request.headers["x-github-api-version"] for request in fake.requests) + + +async def test_installation_token_is_cached_per_repository_until_refresh() -> None: + fake = FakeGitHub() + adapter = _adapter(fake, installation_id=77) + await _fetch(adapter) + await _fetch(adapter) + + paths = [request.url.path for request in fake.requests] + assert paths.count("/app/installations/77/access_tokens") == 1 + assert f"/repos/{REPOSITORY}/installation" not in paths + + +async def test_pagination_follows_same_origin_next_links() -> None: + fake = FakeGitHub() + runs_path = f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs" + page_two = f"{API}{runs_path}?per_page=100&page=2" + fake.route( + "GET", + runs_path, + httpx.Response( + 200, + json={ + "check_runs": [ + {"name": "other", "status": "completed", "conclusion": "failure"} + ] + }, + headers={"link": f'<{page_two}>; rel="next"'}, + ), + { + "check_runs": [ + { + "name": "backend", + "status": "completed", + "conclusion": "failure", + "app": {"id": 15368}, + } + ] + }, + ) + facts = await _fetch(_adapter(fake)) + + assert facts.required_checks[0].conclusion == "failure" + assert [item.request for item in facts.responses][2:4] == [ + "check_runs.page.1", + "check_runs.page.2", + ] + + +async def test_pagination_refuses_a_foreign_origin_link() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + httpx.Response( + 200, + json={"check_runs": []}, + headers={"link": '; rel="next"'}, + ), + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + assert all(request.url.host == "api.github.test" for request in fake.requests) + + +async def test_required_check_conclusions_fail_closed() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + {"contexts": ["backend", "lint", "docs", "e2e", "wrong-app"], "checks": []}, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + { + "check_runs": [ + {"name": "backend", "status": "completed", "conclusion": "success"}, + {"name": "e2e", "status": "in_progress", "conclusion": None}, + {"name": "wrong-app", "status": "completed", "conclusion": "skipped"}, + ] + }, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", + { + "statuses": [ + {"context": "backend", "state": "failure"}, + {"context": "lint", "state": "error"}, + ] + }, + ) + facts = await _fetch(_adapter(fake)) + + assert {item.name: item.conclusion for item in facts.required_checks} == { + "backend": "failure", + "docs": "missing", + "e2e": "pending", + "lint": "error", + "wrong-app": "skipped", + } + + +async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + { + "check_runs": [ + { + "name": "backend", + "status": "completed", + "conclusion": "success", + "app": {"id": 999}, + } + ] + }, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", + {"statuses": [{"context": "backend", "state": "success"}, {"context": "lint", "state": "success"}]}, + ) + facts = await _fetch(_adapter(fake)) + assert facts.required_checks[0].conclusion == "missing" + + +async def test_unmerged_pull_request_reports_no_merge_commit() -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull(state="open", merged=False)) + facts = await _fetch(_adapter(fake)) + assert (facts.state, facts.merged, facts.merge_commit_sha) == ("open", False, None) + + +@pytest.mark.parametrize( + "protection", + [ + httpx.Response(404, json={"message": "Branch not protected"}), + {"contexts": [], "checks": []}, + ], + ids=("not-protected", "no-required-checks"), +) +async def test_missing_required_checks_fail_closed(protection: object) -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + protection, + ) + with pytest.raises(GitHubRequiredChecksNotConfiguredError) as caught: + await _fetch(_adapter(fake)) + assert caught.value.retryable is False + + +async def test_unknown_pull_request_is_not_found() -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(404)) + with pytest.raises(GitHubPullRequestNotFoundError): + await _fetch(_adapter(fake)) + + +async def test_pull_request_from_another_repository_is_rejected() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12", + _pull(base={"ref": "main", "repo": {"full_name": "acme/other"}}), + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +async def test_server_error_is_retried_then_succeeds() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12", + httpx.Response(502), + httpx.ConnectError("connection refused"), + _pull(), + ) + facts = await _fetch(_adapter(fake, clock)) + assert facts.merged is True + assert clock.sleeps == [0.5, 1.0] + + +async def test_retries_exhausted_raise_facts_unavailable() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(503)) + with pytest.raises(GitHubFactsUnavailableError) as caught: + await _fetch(_adapter(fake, clock)) + pulls = [r for r in fake.requests if r.url.path.endswith("/pulls/12")] + assert len(pulls) == 3 + assert clock.sleeps == [0.5, 1.0] + assert caught.value.retryable is True + + +async def test_total_deadline_bounds_retries() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(500)) + with pytest.raises(GitHubFactsUnavailableError): + await _fetch(_adapter(fake, clock, deadline=1.0, attempts=5)) + assert sum(clock.sleeps) < 1.0 + + +async def test_rate_limit_waits_for_reset_within_the_deadline() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12", + httpx.Response( + 403, + headers={"x-ratelimit-remaining": "0", "x-ratelimit-reset": str(int(NOW) + 7)}, + ), + httpx.Response(429, headers={"retry-after": "3"}), + _pull(), + ) + facts = await _fetch(_adapter(fake, clock)) + assert facts.merged is True + assert clock.sleeps == [7.0, 3.0] + + +async def test_rate_limit_beyond_the_deadline_fails_without_waiting() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12", + httpx.Response(429, headers={"retry-after": "3600"}), + ) + with pytest.raises(GitHubFactsUnavailableError): + await _fetch(_adapter(fake, clock)) + assert clock.sleeps == [] + + +async def test_forbidden_without_rate_limit_is_access_denied() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + httpx.Response(403, json={"message": "Resource not accessible by integration"}), + ) + with pytest.raises(GitHubFactsAccessDeniedError): + await _fetch(_adapter(fake)) + + +async def test_rejected_cached_token_is_refreshed_once() -> None: + fake = FakeGitHub() + adapter = _adapter(fake, installation_id=77) + await _fetch(adapter) + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(401), _pull()) + await _fetch(adapter) + exchanges = [r for r in fake.requests if r.url.path.endswith("/access_tokens")] + assert len(exchanges) == 2 + assert fake.requests[-1].headers["authorization"] == f"Bearer {SECOND_TOKEN}" + + +@pytest.mark.parametrize( + ("repository", "number"), + [("acme", 12), ("acme/../x", 12), ("acme/widgets/extra", 12), (REPOSITORY, 0)], +) +async def test_invalid_request_is_refused_before_any_network_call( + repository: str, number: int +) -> None: + fake = FakeGitHub() + with pytest.raises(ExternalServiceConfigurationError): + await _adapter(fake).fetch_pull_request_facts( + repository=repository, pull_request_number=number + ) + assert fake.requests == [] + + +def _assert_secret_free(text: str, secrets: list[str]) -> None: + for secret in secrets: + assert secret not in text + + +@pytest.mark.parametrize( + "failure", + ["unavailable", "access_denied", "not_found", "protocol", "token_exchange_denied"], +) +async def test_credentials_never_reach_errors_logs_or_reprs( + failure: str, caplog: pytest.LogCaptureFixture +) -> None: + fake = FakeGitHub() + pulls = f"/repos/{REPOSITORY}/pulls/12" + if failure == "unavailable": + fake.route("GET", pulls, httpx.ConnectError(f"refused Bearer {TOKEN}")) + elif failure == "access_denied": + fake.route("GET", pulls, httpx.Response(401)) + elif failure == "not_found": + fake.route("GET", pulls, httpx.Response(404)) + elif failure == "protocol": + fake.route("GET", pulls, httpx.Response(200, content=f"not json {TOKEN}".encode())) + else: + fake.route("POST", "/app/installations/77/access_tokens", httpx.Response(403)) + adapter = _adapter(fake) + caplog.set_level(logging.DEBUG) + + with pytest.raises(ExternalServiceAdapterError) as caught: + await _fetch(adapter) + + error = caught.value + rendered = "".join(traceback.format_exception(error)) + assert error.__cause__ is None and error.__suppress_context__ is True + log_text = "\n".join( + f"{record.getMessage()} {record.args!r} {record.__dict__!r}" for record in caplog.records + ) + secrets = fake.secrets() + assert len(secrets) > 3 + for text in (str(error), repr(error), rendered, log_text, repr(adapter)): + _assert_secret_free(text, secrets) + + +async def test_successful_facts_and_token_holder_are_secret_free( + caplog: pytest.LogCaptureFixture, +) -> None: + fake = FakeGitHub() + caplog.set_level(logging.DEBUG) + adapter = _adapter(fake) + facts = await _fetch(adapter) + secrets = fake.secrets() + texts = [ + facts.model_dump_json(), + repr(facts), + repr(adapter), + repr(_InstallationToken(TOKEN, NOW)), + "\n".join(record.getMessage() for record in caplog.records), + ] + for text in texts: + _assert_secret_free(text, secrets) + + +def _settings(tmp_path: Path, **values: object) -> Settings: + key_file = tmp_path / "github-app.pem" + key_file.write_text(PRIVATE_PEM, encoding="utf-8") + configured: dict[str, object] = { + "environment": "test", + "github_app_id": APP_ID, + "github_app_private_key_file": key_file, + "github_api_base_url": API, + } + configured.update(values) + return Settings(_env_file=None, **configured) + + +async def test_factory_registers_the_github_app_provider(tmp_path: Path) -> None: + fake = FakeGitHub() + factory = github_facts_adapter_factory( + _settings(tmp_path, github_app_installation_id=77), + transport=httpx.MockTransport(fake.handler), + ) + adapter = factory.create("github_app") + assert (adapter.identity.capability_key, adapter.identity.provider_key) == ( + "github_facts", + "github_app", + ) + facts = await _fetch(adapter) + assert facts.repository == REPOSITORY + + +def test_factory_refuses_missing_or_unusable_credentials(tmp_path: Path) -> None: + unconfigured = Settings(_env_file=None, environment="test") + with pytest.raises(ExternalServiceConfigurationError): + github_facts_adapter_factory(unconfigured).create("github_app") + settings = _settings(tmp_path) + assert settings.github_app_private_key_file is not None + settings.github_app_private_key_file.write_text("not a key", encoding="utf-8") + with pytest.raises(ExternalServiceConfigurationError) as caught: + github_facts_adapter_factory(settings).create("github_app") + assert caught.value.__cause__ is None + + +@pytest.mark.parametrize( + "values", + [ + {"github_app_private_key_file": None}, + {"github_app_private_key_file": Path("relative.pem")}, + {"github_api_base_url": "http://api.github.example"}, + {"github_api_base_url": "https://user:pass@api.github.com"}, + {"github_api_base_url": "https://api.github.com/"}, + {"github_api_base_url": "http://127.0.0.1:8080", "environment": "production"}, + {"github_facts_total_deadline_seconds": 5.0}, + {"github_facts_total_deadline_seconds": 121.0}, + ], +) +def test_settings_reject_incomplete_or_unsafe_configuration( + tmp_path: Path, values: dict +) -> None: + with pytest.raises(ValueError): + _settings(tmp_path, **values) + + +def test_settings_accept_loopback_http_only_for_local_and_test(tmp_path: Path) -> None: + settings = _settings(tmp_path, github_api_base_url="http://127.0.0.1:8080") + assert settings.github_api_base_url == "http://127.0.0.1:8080" From 6a0181fa3a03e83a06277a0b2eda9ba3899a142f Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 13:40:19 +0100 Subject: [PATCH 2/7] fix(github): judge latest re-run and take App key from secret setting Only the most recent check run and commit status per required check count; required checks are the union of protection checks and contexts. The App private key is the WORKSTREAM_GITHUB_APP_PRIVATE_KEY secret setting with the existing redacted custody path, replacing the key file. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 44 ++++-- backend/.env.example | 11 +- backend/app/adapters/github/__init__.py | 6 +- backend/app/adapters/github/app_facts.py | 92 +++++++----- backend/app/core/config.py | 79 +++++++++-- backend/tests/test_github_facts_adapter.py | 134 ++++++++++++++---- 6 files changed, 273 insertions(+), 93 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index 31a88f921..2774d228b 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -28,7 +28,8 @@ chunk adds no transport, credential or retry logic of its own. `GitHubFactsAdapter` port. - `backend/app/adapters/github/__init__.py` (composition root) and `backend/app/adapters/github/app_facts.py` (GitHub App provider). -- `backend/app/core/config.py` GitHub App settings and their validator; +- `backend/app/core/config.py` GitHub App settings, the private-key secret + custody and their validators; commented examples in `backend/.env.example`. - `backend/tests/test_github_facts_adapter.py`, its lane-catalogue entry and shared-family fingerprint, and the behavior-ownership partition additions. @@ -40,6 +41,11 @@ chunk adds no transport, credential or retry logic of its own. ## Design and decisions +- **GitHub App.** The intended App is `flow-workstream-proof-reader` (App id + 5454828), installed on the Flow-Research repositories with read access to + administration, checks, metadata, pull requests and statuses. Its private key + stays in the operator's secret store; no key is committed, and tests generate + a throwaway RSA key. - **Authentication.** An RS256 App JWT (PyJWT, 60 s backdated, 9 min lifetime) is used only to look up the repository installation (unless `WORKSTREAM_GITHUB_APP_INSTALLATION_ID` is set) and to exchange an installation @@ -52,12 +58,19 @@ chunk adds no transport, credential or retry logic of its own. `status` for the head SHA, with `per_page=100` and `Link: rel="next"` pagination up to 10 pages. A next link to another origin is a protocol error, so a response cannot redirect the credential. Redirects are not followed. -- **Required checks.** Protection `checks` (context plus optional App id) are - used when present, otherwise `contexts`. A check restricted to an App matches - only that App's check runs; an unrestricted check matches check runs and - commit statuses by name. Every matching result must be `success`; otherwise - the first non-success value is reported. No match is `missing`; an incomplete - run is `pending`. `neutral` and `skipped` are not `success`. +- **Required checks.** The required set is the union of protection + `checks[].context` (with its optional App id; `-1` or null means any source) + and `contexts` (any source). A check restricted to an App matches only that + App's check runs; an unrestricted check matches check runs and commit + statuses by name. +- **Re-runs.** A head commit can carry several check runs with one name. A live + read of Flow-Research/harnessy-v2 PR #169 found 47 runs for 24 names, where a + failed first attempt was re-run successfully; judging any run per name gave a + false failure. For each required check only the most recent check run + (highest GitHub id) and the most recent commit status (highest id) count. + When both exist, both must be `success`; otherwise the first non-success value + is reported. No match is `missing`; an incomplete run is `pending`. `neutral` + and `skipped` are not `success`. - **Missing protection fails closed.** A 404 from the protection endpoint, or an empty required-check list, raises `GitHubRequiredChecksNotConfiguredError` (a non-retryable configuration error). Without required checks "CI passed" @@ -76,8 +89,14 @@ chunk adds no transport, credential or retry logic of its own. about 3% of the 3,520 s handler timeout at the maximum. Chunk 04 must place this budget inside the handler prelude or reserve it from the execution budget; the 60 s prelude margin alone is already partly used. -- **Credential custody.** The key is read once from an absolute file path - (bounded to 16 KiB) and held only as a parsed `RSAPrivateKey`. Tokens live +- **Credential custody.** The PEM key is the secret setting + `WORKSTREAM_GITHUB_APP_PRIVATE_KEY`. Settings remove it from structured input + before validation and hold it only as a private redacted `SecretStr`, + following the `pagination_cursor_hmac_secret` custody path (direct, JSON, + string and dotenv inputs; cleared on failure). It must be configured together + with the App id. Single-line values with literal `\n` separators are + accepted. The adapter parses it once (bounded to 16 KiB) and holds only an + `RSAPrivateKey`. Tokens live only in `_InstallationToken`, whose representation is redacted. Every adapter error is raised `from None` and carries only its code. Retry logs carry only status and attempt. Token-exchange bodies are never hashed or @@ -91,6 +110,8 @@ chunk adds no transport, credential or retry logic of its own. ## Acceptance criteria +- [x] Only the latest re-run per required check counts, in both directions, + and the required set is the union of `checks` and `contexts`. - [x] Happy path returns the exact facts, response digests, a verifiable JWT and a repository-scoped token request. - [x] Pagination is followed within the origin; a foreign link fails. @@ -106,6 +127,8 @@ chunk adds no transport, credential or retry logic of its own. ## Risk and review routing - Risk class: L1, new external credential custody without a production caller. + The App's administration read permission is needed for classic branch + protection reads. - Required reviewers: security, senior engineering, QA. - Human review focus: the GitHub App permission set (classic branch protection reads need repository administration read), fail-closed missing protection, @@ -116,7 +139,8 @@ chunk adds no transport, credential or retry logic of its own. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 39 passed | Uses `httpx.MockTransport`, not live GitHub | +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 44 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Re-run test detects order-based selection | Temporarily selecting the last listed run made both re-run cases fail | Detected | Manual negative control | | Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | | Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | | Settings | `tests/test_config.py` plus the new settings cases | Pass | None | diff --git a/backend/.env.example b/backend/.env.example index 02e4822e7..caa4988cb 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -120,11 +120,12 @@ WORKSTREAM_GUIDE_DOCUMENT_PARSER_SOCKET=/var/lib/workstream/guide-runtime/guide. # WORKSTREAM_ARTIFACT_SCRATCH_ROOT=/tmp/workstream-local-artifact-scratch # Read-only GitHub App for proof facts (WS-GHPROOF-001). Not called by any -# production path yet. Keep the private key in a file outside the repository; -# never put its contents in this file. -# WORKSTREAM_GITHUB_APP_ID=123456 -# WORKSTREAM_GITHUB_APP_PRIVATE_KEY_FILE=/run/secrets/workstream-github-app.pem -# WORKSTREAM_GITHUB_APP_INSTALLATION_ID=7654321 +# production path yet. The PEM private key comes from the deployment secret +# store as WORKSTREAM_GITHUB_APP_PRIVATE_KEY (literal \n separators are +# accepted); never put it in this file. The installation is looked up per +# repository when no installation id is set. +# WORKSTREAM_GITHUB_APP_ID=5454828 +# WORKSTREAM_GITHUB_APP_INSTALLATION_ID= # WORKSTREAM_GITHUB_API_BASE_URL=https://api.github.com # WORKSTREAM_GITHUB_API_REQUEST_TIMEOUT_SECONDS=10.0 # WORKSTREAM_GITHUB_FACTS_TOTAL_DEADLINE_SECONDS=45.0 diff --git a/backend/app/adapters/github/__init__.py b/backend/app/adapters/github/__init__.py index 657089f98..3cab209dd 100644 --- a/backend/app/adapters/github/__init__.py +++ b/backend/app/adapters/github/__init__.py @@ -31,8 +31,8 @@ def _github_app_adapter( ) -> GitHubAppFactsAdapter: """Construct the provider from complete settings or fail as configuration.""" app_id = settings.github_app_id - key_file = settings.github_app_private_key_file - if app_id is None or key_file is None: + private_key = settings.github_app_private_key + if app_id is None or private_key is None: raise ExternalServiceConfigurationError( ExternalServiceAdapterIdentity(GITHUB_FACTS_CAPABILITY_KEY, GITHUB_APP_PROVIDER_KEY) ) @@ -45,7 +45,7 @@ def _github_app_adapter( total_deadline_seconds=settings.github_facts_total_deadline_seconds, maximum_attempts=settings.github_facts_maximum_attempts, ), - load_github_app_private_key(key_file), + load_github_app_private_key(private_key), transport=transport, ) diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py index 866d58cac..655bab6ed 100644 --- a/backend/app/adapters/github/app_facts.py +++ b/backend/app/adapters/github/app_facts.py @@ -15,7 +15,6 @@ from datetime import UTC, datetime import hashlib import logging -from pathlib import Path import time from typing import Any from urllib.parse import quote, urlsplit @@ -24,7 +23,7 @@ import jwt from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey from cryptography.hazmat.primitives.serialization import load_pem_private_key -from pydantic import ValidationError +from pydantic import SecretStr, ValidationError from app.interfaces.external_services import ( ExternalServiceAdapterIdentity, @@ -102,16 +101,16 @@ class GitHubAppFactsConfiguration: maximum_attempts: int -def load_github_app_private_key(path: Path) -> RSAPrivateKey: - """Read and parse one bounded PEM RSA key without exposing its bytes.""" +def load_github_app_private_key(secret: SecretStr) -> RSAPrivateKey: + """Parse one bounded PEM RSA key without exposing its bytes.""" key: object = None try: - with path.open("rb") as handle: - data = handle.read(MAXIMUM_PRIVATE_KEY_BYTES + 1) + data = secret.get_secret_value().encode("ascii") if len(data) <= MAXIMUM_PRIVATE_KEY_BYTES: key = load_pem_private_key(data, password=None) - except (OSError, ValueError, TypeError): + except (UnicodeEncodeError, ValueError, TypeError): key = None + data = b"" if not isinstance(key, RSAPrivateKey): raise ExternalServiceConfigurationError(_IDENTITY) from None return key @@ -554,32 +553,41 @@ def _pull_request_facts(body: object, repository: str, number: int) -> dict[str, def _required_checks(body: object) -> tuple[tuple[str, int | None], ...]: - """Return required (context, app id) pairs; none configured fails closed.""" + """Return required (context, app id) pairs from ``checks`` and ``contexts``. + + ``checks`` entries carry an optional App restriction; a name only in + ``contexts`` accepts any source. No required check fails closed. + """ if not isinstance(body, dict): raise GitHubFactsProtocolError(_IDENTITY) - checks = body.get("checks") - required: dict[tuple[str, int | None], None] = {} - if isinstance(checks, list) and checks: - for item in checks: - context = item.get("context") if isinstance(item, dict) else None - app_id = item.get("app_id") if isinstance(item, dict) else None - if not isinstance(context, str) or not context: - raise GitHubFactsProtocolError(_IDENTITY) - if app_id is not None and (type(app_id) is not int or app_id < 1): - # GitHub reports -1 or null for "any source". - app_id = None - required[(context, app_id)] = None - else: - contexts = body.get("contexts") - if not isinstance(contexts, list): + checks = body.get("checks", []) + contexts = body.get("contexts", []) + if not isinstance(checks, list) or not isinstance(contexts, list): + raise GitHubFactsProtocolError(_IDENTITY) + required: dict[str, int | None] = {} + for item in checks: + context = item.get("context") if isinstance(item, dict) else None + app_id = item.get("app_id") if isinstance(item, dict) else None + if not isinstance(context, str) or not context: raise GitHubFactsProtocolError(_IDENTITY) - for context in contexts: - if not isinstance(context, str) or not context: - raise GitHubFactsProtocolError(_IDENTITY) - required[(context, None)] = None + if app_id is not None and (type(app_id) is not int or app_id < 1): + # GitHub reports -1 or null for "any source". + app_id = None + required[context] = app_id + for context in contexts: + if not isinstance(context, str) or not context: + raise GitHubFactsProtocolError(_IDENTITY) + required.setdefault(context, None) if not required: raise GitHubRequiredChecksNotConfiguredError(_IDENTITY) - return tuple(sorted(required, key=lambda item: (item[0], item[1] or 0))) + return tuple(sorted(required.items())) + + +def _latest(items: list[dict]) -> dict | None: + """Return the most recent item by GitHub id; re-runs get higher ids.""" + if any(type(item.get("id")) is not int for item in items): + raise GitHubFactsProtocolError(_IDENTITY) + return max(items, key=lambda item: item["id"], default=None) def _conclusion( @@ -588,26 +596,32 @@ def _conclusion( check_runs: list[dict], statuses: list[dict], ) -> str: - """Combine every matching check run and commit status; all must succeed.""" + """Judge only the latest check run and latest status for one required check. + + A re-run supersedes an earlier attempt with the same name. When both a check + run and a commit status exist for the name, both latest results must succeed. + """ observed: list[str] = [] + runs = [] for run in check_runs: app = run.get("app") run_app_id = app.get("id") if isinstance(app, dict) else None - if run.get("name") != name or (app_id is not None and run_app_id != app_id): - continue + if run.get("name") == name and (app_id is None or run_app_id == app_id): + runs.append(run) + run = _latest(runs) + if run is not None: if run.get("status") != "completed": observed.append("pending") - continue - conclusion = run.get("conclusion") - if conclusion not in _CHECK_RUN_CONCLUSIONS: - raise GitHubFactsProtocolError(_IDENTITY) - observed.append(conclusion) + else: + conclusion = run.get("conclusion") + if conclusion not in _CHECK_RUN_CONCLUSIONS: + raise GitHubFactsProtocolError(_IDENTITY) + observed.append(conclusion) if app_id is None: # Commit statuses carry no App identity, so they satisfy only # requirements that accept any source. - for status in statuses: - if status.get("context") != name: - continue + status = _latest([item for item in statuses if item.get("context") == name]) + if status is not None: state = status.get("state") if state not in _STATUS_STATES: raise GitHubFactsProtocolError(_IDENTITY) diff --git a/backend/app/core/config.py b/backend/app/core/config.py index c819e5a19..072eb7a28 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -287,10 +287,10 @@ class Settings(BaseSettings): gt=0.0, le=3700.0, ) - # Read-only GitHub App used to read pull-request proof facts. The private - # key stays in a file; settings hold only its absolute path. + # Read-only GitHub App used to read pull-request proof facts. Its PEM + # private key is the secret WORKSTREAM_GITHUB_APP_PRIVATE_KEY, held only as + # a private redacted attribute (see github_app_private_key). github_app_id: int | None = Field(default=None, ge=1) - github_app_private_key_file: Path | None = None github_app_installation_id: int | None = Field(default=None, ge=1) github_api_base_url: str = "https://api.github.com" github_api_request_timeout_seconds: float = Field(default=10.0, gt=0.0, le=30.0) @@ -316,6 +316,7 @@ class Settings(BaseSettings): _api_rate_limit_key_secret: SecretStr | None = PrivateAttr(default=None) _pagination_cursor_hmac_secret: SecretStr | None = PrivateAttr(default=None) + _github_app_private_key: SecretStr | None = PrivateAttr(default=None) _artifact_s3_access_key_id: SecretStr | None = PrivateAttr(default=None) _artifact_s3_secret_access_key: SecretStr | None = PrivateAttr(default=None) _artifact_s3_session_token: SecretStr | None = PrivateAttr(default=None) @@ -336,24 +337,29 @@ def __init__(self, **values: object) -> None: secret: SecretStr | None = None cursor_secret: SecretStr | None = None s3_secrets = _EMPTY_ARTIFACT_S3_SECRETS + github_key: SecretStr | None = None try: _normalize_observability_endpoint_input(values) _normalize_artifact_s3_endpoint_input(values) secret = _extract_api_rate_limit_key_secret(values) cursor_secret = _extract_pagination_cursor_hmac_secret(values) s3_secrets = _extract_artifact_s3_static_secrets(values) + github_key = _extract_github_app_private_key(values) super().__init__(**values) self._validate_observability_endpoint_security() self._api_rate_limit_key_secret = secret self._pagination_cursor_hmac_secret = cursor_secret self._set_artifact_s3_static_secrets(s3_secrets) + self._github_app_private_key = github_key if not _ALTERNATE_VALIDATION_RESTORES_SECRETS.get(): self._validate_artifact_s3_secret_contract() + self._validate_github_app_secret_contract() except Exception: _clear_settings_private_secrets(self) values.clear() secret = None cursor_secret = None + github_key = None s3_secrets = _EMPTY_ARTIFACT_S3_SECRETS raise @@ -372,6 +378,7 @@ def model_validate(cls, obj: object, **kwargs: object) -> Self: if isinstance(obj, Mapping) and ( "api_rate_limit_key_secret" in obj or "pagination_cursor_hmac_secret" in obj + or _GITHUB_APP_PRIVATE_KEY_FIELD in obj or _ARTIFACT_S3_SENSITIVE_INPUT_FIELDS.intersection(obj) or _OBSERVABILITY_SENSITIVE_INPUT_FIELDS.intersection(obj) ): @@ -407,6 +414,7 @@ def model_validate_json( if isinstance(parsed, Mapping) and ( "api_rate_limit_key_secret" in parsed or "pagination_cursor_hmac_secret" in parsed + or _GITHUB_APP_PRIVATE_KEY_FIELD in parsed or _ARTIFACT_S3_SENSITIVE_INPUT_FIELDS.intersection(parsed) or _OBSERVABILITY_SENSITIVE_INPUT_FIELDS.intersection(parsed) ): @@ -429,6 +437,7 @@ def model_validate_strings(cls, obj: object, **kwargs: object) -> Self: if isinstance(obj, Mapping) and ( "api_rate_limit_key_secret" in obj or "pagination_cursor_hmac_secret" in obj + or _GITHUB_APP_PRIVATE_KEY_FIELD in obj or _ARTIFACT_S3_SENSITIVE_INPUT_FIELDS.intersection(obj) or _OBSERVABILITY_SENSITIVE_INPUT_FIELDS.intersection(obj) ): @@ -453,6 +462,7 @@ def _validate_secret_mapping( secret: SecretStr | None = None cursor_secret: SecretStr | None = None s3_secrets = _EMPTY_ARTIFACT_S3_SECRETS + github_key: SecretStr | None = None settings: Settings | None = None try: _normalize_observability_endpoint_input(sanitized) @@ -468,6 +478,11 @@ def _validate_secret_mapping( else None ) s3_secrets = _extract_artifact_s3_static_secrets(sanitized) + github_key = ( + _extract_github_app_private_key(sanitized) + if _GITHUB_APP_PRIVATE_KEY_FIELD in sanitized + else None + ) if secret is not None: sanitized["api_rate_limit_key_secret"] = None if cursor_secret is not None: @@ -485,6 +500,9 @@ def _validate_secret_mapping( settings._pagination_cursor_hmac_secret = cursor_secret settings._set_artifact_s3_static_secrets(s3_secrets) settings._validate_artifact_s3_secret_contract() + if github_key is not None: + settings._github_app_private_key = github_key + settings._validate_github_app_secret_contract() return settings except Exception: if settings is not None: @@ -493,6 +511,7 @@ def _validate_secret_mapping( secret = None cursor_secret = None s3_secrets = _EMPTY_ARTIFACT_S3_SECRETS + github_key = None settings = None raise @@ -509,6 +528,7 @@ def settings_customise_sources( if isinstance(dotenv_settings, DotEnvSettingsSource): dotenv_settings.env_vars.pop("workstream_api_rate_limit_key_secret", None) dotenv_settings.env_vars.pop("workstream_pagination_cursor_hmac_secret", None) + dotenv_settings.env_vars.pop(f"workstream_{_GITHUB_APP_PRIVATE_KEY_FIELD}", None) for field_name in _ARTIFACT_S3_SECRET_FIELDS: dotenv_settings.env_vars.pop(f"workstream_{field_name}", None) return init_settings, env_settings, dotenv_settings, file_secret_settings @@ -523,6 +543,16 @@ def pagination_cursor_hmac_secret(self) -> SecretStr | None: """Return the validated, redacted pagination-cursor key setting.""" return self._pagination_cursor_hmac_secret + @property + def github_app_private_key(self) -> SecretStr | None: + """Return the redacted GitHub App PEM private key.""" + return self._github_app_private_key + + def _validate_github_app_secret_contract(self) -> None: + """Require the GitHub App id and private key together.""" + if (self.github_app_id is None) != (self._github_app_private_key is None): + raise ValueError("GitHub App id and private key must be configured together") + @property def artifact_s3_access_key_id(self) -> SecretStr | None: """Return the local/CI MinIO access-key identifier.""" @@ -575,13 +605,8 @@ def _validate_artifact_s3_secret_contract(self) -> None: @model_validator(mode="after") def validate_github_app(self) -> Settings: """Require a complete GitHub App identity and a credential-safe API origin.""" - if (self.github_app_id is None) != (self.github_app_private_key_file is None): - raise ValueError("GitHub App id and private key file must be configured together") if self.github_app_installation_id is not None and self.github_app_id is None: raise ValueError("GitHub App installation id requires a GitHub App id") - key_file = self.github_app_private_key_file - if key_file is not None and not key_file.is_absolute(): - raise ValueError("GitHub App private key file must be an absolute path") parsed = urlsplit(self.github_api_base_url) loopback_http = ( parsed.scheme == "http" @@ -931,6 +956,7 @@ def _clear_settings_private_secrets(settings: object) -> None: return private_values["_api_rate_limit_key_secret"] = None private_values["_pagination_cursor_hmac_secret"] = None + private_values["_github_app_private_key"] = None private_values["_artifact_s3_access_key_id"] = None private_values["_artifact_s3_secret_access_key"] = None private_values["_artifact_s3_session_token"] = None @@ -1114,6 +1140,43 @@ def _extract_pagination_cursor_hmac_secret( return secret +_GITHUB_APP_PRIVATE_KEY_FIELD = "github_app_private_key" +MAXIMUM_GITHUB_APP_PRIVATE_KEY_CHARACTERS = 16_384 + + +def _extract_github_app_private_key(values: dict[str, object]) -> SecretStr | None: + """Remove the GitHub App PEM key from structured input; the adapter parses it. + + A single-line value with literal ``\\n`` separators, as some secret stores + emit, is converted to a multi-line PEM document. + """ + raw_value = _pop_optional_secret_source( + values, + field_name=_GITHUB_APP_PRIVATE_KEY_FIELD, + environment_name="WORKSTREAM_GITHUB_APP_PRIVATE_KEY", + ) + if raw_value is _MISSING_SECRET or raw_value is None or raw_value == "": + return None + if isinstance(raw_value, SecretStr): + text = raw_value.get_secret_value() + elif isinstance(raw_value, str): + text = raw_value + else: + del raw_value + raise ValueError("invalid GitHub App private key") + del raw_value + if "\n" not in text and "\\n" in text: + text = text.replace("\\n", "\n") + if ( + len(text) > MAXIMUM_GITHUB_APP_PRIVATE_KEY_CHARACTERS + or "-----BEGIN" not in text + or "PRIVATE KEY-----" not in text + ): + del text + raise ValueError("invalid GitHub App private key") + return SecretStr(text) + + def _extract_artifact_s3_static_secrets( values: dict[str, object], ) -> tuple[SecretStr | None, SecretStr | None, SecretStr | None]: diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py index abdf121eb..fae1708e6 100644 --- a/backend/tests/test_github_facts_adapter.py +++ b/backend/tests/test_github_facts_adapter.py @@ -5,7 +5,6 @@ import hashlib import json import logging -from pathlib import Path import traceback import httpx @@ -50,6 +49,7 @@ serialization.NoEncryption(), ).decode() PEM_BODY_LINE = PRIVATE_PEM.splitlines()[1] +# Throwaway key generated per test run; no real key exists in the repository. def _pull(**changes: object) -> dict: @@ -95,6 +95,7 @@ def __init__(self) -> None: "total_count": 1, "check_runs": [ { + "id": 10, "name": "backend", "status": "completed", "conclusion": "success", @@ -106,7 +107,7 @@ def __init__(self) -> None: self.route( "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", - {"state": "success", "statuses": [{"context": "lint", "state": "success"}]}, + {"state": "success", "statuses": [{"id": 21, "context": "lint", "state": "success"}]}, ) def route(self, method: str, path: str, *responses: object) -> None: @@ -278,7 +279,7 @@ async def test_pagination_follows_same_origin_next_links() -> None: 200, json={ "check_runs": [ - {"name": "other", "status": "completed", "conclusion": "failure"} + {"id": 4, "name": "other", "status": "completed", "conclusion": "failure"} ] }, headers={"link": f'<{page_two}>; rel="next"'}, @@ -286,6 +287,7 @@ async def test_pagination_follows_same_origin_next_links() -> None: { "check_runs": [ { + "id": 11, "name": "backend", "status": "completed", "conclusion": "failure", @@ -331,9 +333,9 @@ async def test_required_check_conclusions_fail_closed() -> None: f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", { "check_runs": [ - {"name": "backend", "status": "completed", "conclusion": "success"}, - {"name": "e2e", "status": "in_progress", "conclusion": None}, - {"name": "wrong-app", "status": "completed", "conclusion": "skipped"}, + {"id": 1, "name": "backend", "status": "completed", "conclusion": "success"}, + {"id": 2, "name": "e2e", "status": "in_progress", "conclusion": None}, + {"id": 3, "name": "wrong-app", "status": "completed", "conclusion": "skipped"}, ] }, ) @@ -342,8 +344,8 @@ async def test_required_check_conclusions_fail_closed() -> None: f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", { "statuses": [ - {"context": "backend", "state": "failure"}, - {"context": "lint", "state": "error"}, + {"id": 23, "context": "backend", "state": "failure"}, + {"id": 24, "context": "lint", "state": "error"}, ] }, ) @@ -358,6 +360,64 @@ async def test_required_check_conclusions_fail_closed() -> None: } +@pytest.mark.parametrize( + ("older", "newer", "expected"), + [("failure", "success", "success"), ("success", "failure", "failure")], + ids=("rerun-passed", "rerun-failed"), +) +async def test_only_the_latest_rerun_of_a_required_check_counts( + older: str, newer: str, expected: str +) -> None: + fake = FakeGitHub() + runs_path = f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs" + run = {"name": "backend", "status": "completed", "app": {"id": 15368}} + # The newer run is on the first page and the older on the second, so + # response order cannot decide the result. + fake.route( + "GET", + runs_path, + httpx.Response( + 200, + json={"check_runs": [{**run, "id": 900, "conclusion": newer}]}, + headers={"link": f'<{API}{runs_path}?per_page=100&page=2>; rel="next"'}, + ), + {"check_runs": [{**run, "id": 899, "conclusion": older}]}, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", + { + "statuses": [ + {"id": 31, "context": "lint", "state": "failure"}, + {"id": 32, "context": "lint", "state": "success"}, + ] + }, + ) + facts = await _fetch(_adapter(fake)) + assert {item.name: item.conclusion for item in facts.required_checks} == { + "backend": expected, + "lint": "success", + } + + +async def test_required_checks_are_the_union_of_checks_and_contexts() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + { + "contexts": ["backend", "docs"], + "checks": [{"context": "backend", "app_id": 15368}, {"context": "lint", "app_id": -1}], + }, + ) + facts = await _fetch(_adapter(fake)) + assert [(item.name, item.app_id, item.conclusion) for item in facts.required_checks] == [ + ("backend", 15368, "success"), + ("docs", None, "missing"), + ("lint", None, "success"), + ] + + async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: fake = FakeGitHub() fake.route( @@ -366,6 +426,7 @@ async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: { "check_runs": [ { + "id": 12, "name": "backend", "status": "completed", "conclusion": "success", @@ -377,7 +438,7 @@ async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: fake.route( "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", - {"statuses": [{"context": "backend", "state": "success"}, {"context": "lint", "state": "success"}]}, + {"statuses": [{"id": 22, "context": "backend", "state": "success"}, {"id": 21, "context": "lint", "state": "success"}]}, ) facts = await _fetch(_adapter(fake)) assert facts.required_checks[0].conclusion == "missing" @@ -593,23 +654,21 @@ async def test_successful_facts_and_token_holder_are_secret_free( _assert_secret_free(text, secrets) -def _settings(tmp_path: Path, **values: object) -> Settings: - key_file = tmp_path / "github-app.pem" - key_file.write_text(PRIVATE_PEM, encoding="utf-8") +def _settings(**values: object) -> Settings: configured: dict[str, object] = { "environment": "test", "github_app_id": APP_ID, - "github_app_private_key_file": key_file, + "github_app_private_key": PRIVATE_PEM, "github_api_base_url": API, } configured.update(values) return Settings(_env_file=None, **configured) -async def test_factory_registers_the_github_app_provider(tmp_path: Path) -> None: +async def test_factory_registers_the_github_app_provider() -> None: fake = FakeGitHub() factory = github_facts_adapter_factory( - _settings(tmp_path, github_app_installation_id=77), + _settings(github_app_installation_id=77), transport=httpx.MockTransport(fake.handler), ) adapter = factory.create("github_app") @@ -621,23 +680,44 @@ async def test_factory_registers_the_github_app_provider(tmp_path: Path) -> None assert facts.repository == REPOSITORY -def test_factory_refuses_missing_or_unusable_credentials(tmp_path: Path) -> None: +def test_factory_refuses_missing_or_unusable_credentials() -> None: unconfigured = Settings(_env_file=None, environment="test") with pytest.raises(ExternalServiceConfigurationError): github_facts_adapter_factory(unconfigured).create("github_app") - settings = _settings(tmp_path) - assert settings.github_app_private_key_file is not None - settings.github_app_private_key_file.write_text("not a key", encoding="utf-8") + unusable = "-----BEGIN PRIVATE KEY-----\nnot-a-key\n-----END PRIVATE KEY-----\n" with pytest.raises(ExternalServiceConfigurationError) as caught: - github_facts_adapter_factory(settings).create("github_app") + github_facts_adapter_factory(_settings(github_app_private_key=unusable)).create( + "github_app" + ) assert caught.value.__cause__ is None +def test_private_key_setting_is_redacted_and_accepts_escaped_newlines( + monkeypatch: pytest.MonkeyPatch, +) -> None: + settings = _settings() + assert settings.github_app_private_key is not None + assert settings.github_app_private_key.get_secret_value() == PRIVATE_PEM + for text in (repr(settings), str(settings.model_dump()), settings.model_dump_json()): + assert PEM_BODY_LINE not in text + monkeypatch.setenv("WORKSTREAM_GITHUB_APP_PRIVATE_KEY", PRIVATE_PEM.replace("\n", "\\n")) + from_environment = Settings(_env_file=None, environment="test", github_app_id=APP_ID) + assert from_environment.github_app_private_key is not None + assert from_environment.github_app_private_key.get_secret_value() == PRIVATE_PEM + + +def test_invalid_private_key_setting_error_omits_the_value() -> None: + bogus = "not-a-pem-secret-value-0123456789" + with pytest.raises(ValueError) as caught: + _settings(github_app_private_key=bogus) + assert bogus not in "".join(traceback.format_exception(caught.value)) + + @pytest.mark.parametrize( "values", [ - {"github_app_private_key_file": None}, - {"github_app_private_key_file": Path("relative.pem")}, + {"github_app_private_key": None}, + {"github_app_id": None}, {"github_api_base_url": "http://api.github.example"}, {"github_api_base_url": "https://user:pass@api.github.com"}, {"github_api_base_url": "https://api.github.com/"}, @@ -646,13 +726,11 @@ def test_factory_refuses_missing_or_unusable_credentials(tmp_path: Path) -> None {"github_facts_total_deadline_seconds": 121.0}, ], ) -def test_settings_reject_incomplete_or_unsafe_configuration( - tmp_path: Path, values: dict -) -> None: +def test_settings_reject_incomplete_or_unsafe_configuration(values: dict) -> None: with pytest.raises(ValueError): - _settings(tmp_path, **values) + _settings(**values) -def test_settings_accept_loopback_http_only_for_local_and_test(tmp_path: Path) -> None: - settings = _settings(tmp_path, github_api_base_url="http://127.0.0.1:8080") +def test_settings_accept_loopback_http_only_for_local_and_test() -> None: + settings = _settings(github_api_base_url="http://127.0.0.1:8080") assert settings.github_api_base_url == "http://127.0.0.1:8080" From 61c047e75c21c8402ee7b83d776eebe7d3bc289f Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 14:55:42 +0100 Subject: [PATCH 3/7] test(architecture): register GitHub facts composition root The architecture proof counts every ExternalServiceAdapterFactory call site. Add the GitHub adapter package as one more approved composition root. Co-Authored-By: Claude Opus 5.5 --- .commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md | 2 ++ backend/tests/test_artifact_architecture.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index 2774d228b..4ad346736 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -33,6 +33,8 @@ chunk adds no transport, credential or retry logic of its own. commented examples in `backend/.env.example`. - `backend/tests/test_github_facts_adapter.py`, its lane-catalogue entry and shared-family fingerprint, and the behavior-ownership partition additions. +- `backend/tests/test_artifact_architecture.py`: the GitHub adapter package + is registered as one more approved external-service composition root. ### Not allowed diff --git a/backend/tests/test_artifact_architecture.py b/backend/tests/test_artifact_architecture.py index 4822d5695..576825c30 100644 --- a/backend/tests/test_artifact_architecture.py +++ b/backend/tests/test_artifact_architecture.py @@ -33,6 +33,7 @@ AGENT_COMPOSITION_ROOT = APP_ROOT / "adapters/project_agents/__init__.py" GUIDE_PARSER_COMPOSITION_ROOT = APP_ROOT / "adapters/guide_document_tools/__init__.py" OBSERVABILITY_COMPOSITION_ROOT = APP_ROOT / "adapters" / "observability.py" +GITHUB_COMPOSITION_ROOT = APP_ROOT / "adapters" / "github" / "__init__.py" AGENT_ADAPTER_MODULES = { "app.adapters.project_agents.manager_authored", "app.adapters.project_agents.omnicore_agent", @@ -377,6 +378,7 @@ def _assert_concrete_adapter_composition( AGENT_COMPOSITION_ROOT: 2, OBSERVABILITY_COMPOSITION_ROOT: 1, GUIDE_PARSER_COMPOSITION_ROOT: 1, + GITHUB_COMPOSITION_ROOT: 1, } assert adapter_calls == [COMPOSITION_ROOT, S3_ADAPTER_MODULE, CHECKER_COMPOSITION_ROOT] assert set(concrete_imports) == { From 31be39084aa84c1d54c05c79d2b8d35572b9c75e Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:26:54 +0100 Subject: [PATCH 4/7] feat(github): read author, reviews, changed files and check provenance Integrity facts for owner decision 20: PR author, all reviews, all changed files (fail closed when GitHub cannot list every file), and the producing App of each judged required check. The reader makes no policy decision. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 26 ++- backend/app/adapters/github/app_facts.py | 111 +++++++++-- backend/app/interfaces/github_facts.py | 55 +++++- backend/tests/test_github_facts_adapter.py | 172 +++++++++++++++++- 4 files changed, 344 insertions(+), 20 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index 4ad346736..34528ec01 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -2,7 +2,7 @@ - Initiative: WS-GHPROOF-001 - Durable disposition: Complete -- Intended merge outcome: WorkStream has a typed, secret-safe GitHub facts capability that reads one pull request's merge state and base-branch required-check results as a read-only GitHub App, registered per ADR 0014 and not yet called by a production path. +- Intended merge outcome: WorkStream has a typed, secret-safe GitHub facts capability that reads one pull request's merge state, author, reviews, changed files and base-branch required-check results as a read-only GitHub App, registered per ADR 0014 and not yet called by a production path. ## Intent @@ -103,6 +103,23 @@ chunk adds no transport, credential or retry logic of its own. adapter error is raised `from None` and carries only its code. Retry logs carry only status and attempt. Token-exchange bodies are never hashed or returned; facts hold only GitHub data and response digests. +- **Integrity facts (owner decision 20, 2026-10-24).** The reader stays + factual and makes no policy decision. It adds the pull request author + (`login`, numeric `id`, `type` such as `User` or `Bot`); every submitted review + from `GET /pulls/{n}/reviews` (review `id`, reviewer or null for a deleted + account, `state`, `commit_id`, `submitted_at`), unfiltered, because choosing + each reviewer's latest review is the checker's rule; and every changed file + from `GET /pulls/{n}/files` (`filename`, `previous_filename` for renames, + `status`). Both lists paginate with `per_page=100`; each page body is hashed. +- **Incomplete file lists fail closed.** GitHub lists at most 3,000 changed + files. If the pull request's `changed_files` exceeds 3,000, or the listing + length differs from it, the read raises `GitHubFactsIncompleteError` (a + protocol error), so a protected-path rule can never miss a file. +- **Check provenance.** Each required check also records `app_bound` (the + requirement names an App), `check_run_app_id` (the App that produced the + judged latest run, or null) and `commit_status_judged` (a commit status, which + has no App identity, was judged). The schema id stays + `workstream.github_facts.v1` because the shape is unreleased. - **Facts.** `merge_commit_sha` is null unless merged, because GitHub reports a test-merge commit for open pull requests. `responses` lists the SHA-256 of each raw facts response body in request order. @@ -114,6 +131,11 @@ chunk adds no transport, credential or retry logic of its own. - [x] Only the latest re-run per required check counts, in both directions, and the required set is the union of `checks` and `contexts`. +- [x] Author, all reviews (paginated, unfiltered) and all changed files + (paginated) are returned and hashed; more files than GitHub can list, or a + short listing, fails as incomplete. +- [x] Each required check records its App binding and the producing App or + status source of the judged result. - [x] Happy path returns the exact facts, response digests, a verifiable JWT and a repository-scoped token request. - [x] Pagination is followed within the origin; a foreign link fails. @@ -141,7 +163,7 @@ chunk adds no transport, credential or retry logic of its own. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 44 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 48 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | | Re-run test detects order-based selection | Temporarily selecting the last listed run made both re-run cases fail | Detected | Manual negative control | | Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | | Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py index 655bab6ed..a99d8bee3 100644 --- a/backend/app/adapters/github/app_facts.py +++ b/backend/app/adapters/github/app_facts.py @@ -35,9 +35,14 @@ GitHubFactsAccessDeniedError, GitHubFactsProtocolError, GitHubFactsUnavailableError, + GITHUB_MAXIMUM_LISTED_FILES, + GitHubActorFact, + GitHubChangedFileFact, + GitHubFactsIncompleteError, GitHubPullRequestFacts, GitHubPullRequestNotFoundError, GitHubRequiredCheckFact, + GitHubReviewFact, GitHubRequiredChecksNotConfiguredError, GitHubResponseDigest, is_github_repository, @@ -218,6 +223,22 @@ async def fetch_pull_request_facts( ) ) head_sha = pull_facts["head_sha"] + changed_files_count = pull_facts["changed_files_count"] + if changed_files_count > GITHUB_MAXIMUM_LISTED_FILES: + raise GitHubFactsIncompleteError(_IDENTITY) + reviews = await session.read_pages( + "reviews", + f"/repos/{repository}/pulls/{pull_request_number}/reviews", + item_key=None, + ) + files = await session.read_pages( + "files", + f"/repos/{repository}/pulls/{pull_request_number}/files", + item_key=None, + maximum_pages=GITHUB_MAXIMUM_LISTED_FILES // PAGE_SIZE, + ) + if len(files) != changed_files_count: + raise GitHubFactsIncompleteError(_IDENTITY) check_runs = await session.read_pages( "check_runs", f"/repos/{repository}/commits/{head_sha}/check-runs", @@ -237,12 +258,19 @@ async def fetch_pull_request_facts( merge_commit_sha=pull_facts["merge_commit_sha"], base_ref=pull_facts["base_ref"], head_sha=head_sha, - required_checks=tuple( - GitHubRequiredCheckFact( - name=name, - app_id=app_id, - conclusion=_conclusion(name, app_id, check_runs, statuses), + author=_actor(pull_facts["author"]), + reviews=tuple(_review(item) for item in reviews), + changed_files_count=changed_files_count, + changed_files=tuple( + GitHubChangedFileFact( + filename=item["filename"], + previous_filename=item.get("previous_filename"), + status=item["status"], ) + for item in files + ), + required_checks=tuple( + _required_check_fact(name, app_id, check_runs, statuses) for name, app_id in required ), fetched_at=datetime.fromtimestamp(self._wall_clock(), UTC), @@ -376,15 +404,29 @@ async def read_json( self._record(label, response) return _json(response) - async def read_pages(self, label: str, path: str, *, item_key: str) -> list[dict]: - """Follow same-origin ``next`` links for a bounded number of pages.""" + async def read_pages( + self, + label: str, + path: str, + *, + item_key: str | None, + maximum_pages: int = MAXIMUM_PAGES, + ) -> list[dict]: + """Follow same-origin ``next`` links for a bounded number of pages. + + ``item_key`` names the list inside an object body; None means the body + itself is the list. More pages than ``maximum_pages`` fail closed. + """ url = self._url(path) + f"?per_page={PAGE_SIZE}" items: list[dict] = [] - for page in range(1, MAXIMUM_PAGES + 1): + for page in range(1, maximum_pages + 1): response = await self._installation_get(url, not_found=GitHubFactsProtocolError) self._record(f"{label}.page.{page}", response) body = _json(response) - page_items = body.get(item_key) if isinstance(body, dict) else None + if item_key is None: + page_items = body + else: + page_items = body.get(item_key) if isinstance(body, dict) else None if not isinstance(page_items, list) or not all( isinstance(item, dict) for item in page_items ): @@ -537,6 +579,8 @@ def _pull_request_facts(body: object, repository: str, number: int) -> dict[str, "merge_commit_sha": body["merge_commit_sha"] if merged is True else None, "base_ref": base["ref"], "head_sha": head["sha"], + "author": body["user"], + "changed_files_count": body["changed_files"], } valid = ( body["number"] == number @@ -544,6 +588,8 @@ def _pull_request_facts(body: object, repository: str, number: int) -> dict[str, and full_name.casefold() == repository.casefold() and isinstance(values["base_ref"], str) and isinstance(values["head_sha"], str) + and type(values["changed_files_count"]) is int + and values["changed_files_count"] >= 0 ) except (KeyError, TypeError): valid = False @@ -590,16 +636,36 @@ def _latest(items: list[dict]) -> dict | None: return max(items, key=lambda item: item["id"], default=None) -def _conclusion( +def _actor(body: object) -> GitHubActorFact: + """Translate one GitHub user object.""" + if not isinstance(body, dict): + raise GitHubFactsProtocolError(_IDENTITY) + return GitHubActorFact(login=body["login"], id=body["id"], type=body["type"]) + + +def _review(body: dict) -> GitHubReviewFact: + """Translate one review; GitHub reports a deleted reviewer as null.""" + user = body.get("user") + return GitHubReviewFact( + id=body["id"], + reviewer=None if user is None else _actor(user), + state=body["state"], + commit_id=body.get("commit_id"), + submitted_at=body.get("submitted_at"), + ) + + +def _required_check_fact( name: str, app_id: int | None, check_runs: list[dict], statuses: list[dict], -) -> str: +) -> GitHubRequiredCheckFact: """Judge only the latest check run and latest status for one required check. A re-run supersedes an earlier attempt with the same name. When both a check run and a commit status exist for the name, both latest results must succeed. + The fact records which App produced the judged run. """ observed: list[str] = [] runs = [] @@ -609,7 +675,12 @@ def _conclusion( if run.get("name") == name and (app_id is None or run_app_id == app_id): runs.append(run) run = _latest(runs) + check_run_app_id = None if run is not None: + app = run.get("app") + check_run_app_id = app.get("id") if isinstance(app, dict) else None + if check_run_app_id is not None and type(check_run_app_id) is not int: + raise GitHubFactsProtocolError(_IDENTITY) if run.get("status") != "completed": observed.append("pending") else: @@ -617,6 +688,7 @@ def _conclusion( if conclusion not in _CHECK_RUN_CONCLUSIONS: raise GitHubFactsProtocolError(_IDENTITY) observed.append(conclusion) + status_judged = False if app_id is None: # Commit statuses carry no App identity, so they satisfy only # requirements that accept any source. @@ -626,6 +698,17 @@ def _conclusion( if state not in _STATUS_STATES: raise GitHubFactsProtocolError(_IDENTITY) observed.append(state) - if not observed: - return "missing" - return next((value for value in observed if value != "success"), "success") + status_judged = True + conclusion = ( + "missing" + if not observed + else next((value for value in observed if value != "success"), "success") + ) + return GitHubRequiredCheckFact( + name=name, + app_id=app_id, + app_bound=app_id is not None, + conclusion=conclusion, + check_run_app_id=check_run_app_id, + commit_status_judged=status_judged, + ) diff --git a/backend/app/interfaces/github_facts.py b/backend/app/interfaces/github_facts.py index 170b1c8e3..f210c5050 100644 --- a/backend/app/interfaces/github_facts.py +++ b/backend/app/interfaces/github_facts.py @@ -79,12 +79,55 @@ class _FactsValue(BaseModel): model_config = ConfigDict(extra="forbid", frozen=True, revalidate_instances="always") +# GitHub changed-file listings return at most this many files per pull request. +GITHUB_MAXIMUM_LISTED_FILES = 3000 + + +class GitHubActorFact(_FactsValue): + """One GitHub account as GitHub reports it (``type`` is e.g. User or Bot).""" + + login: Annotated[StrictStr, Field(min_length=1, max_length=100)] + id: Annotated[StrictInt, Field(ge=1)] + type: Annotated[StrictStr, Field(min_length=1, max_length=50)] + + +class GitHubReviewFact(_FactsValue): + """One submitted pull-request review, unfiltered. + + ``reviewer`` is null when GitHub no longer has the account. Policy, such as + taking each reviewer's latest review, belongs to the consumer. + """ + + id: Annotated[StrictInt, Field(ge=1)] + reviewer: GitHubActorFact | None + state: Annotated[StrictStr, Field(min_length=1, max_length=50)] + commit_id: GitSha | None + submitted_at: datetime | None + + +class GitHubChangedFileFact(_FactsValue): + """One changed file; ``previous_filename`` is set for renames.""" + + filename: Annotated[StrictStr, Field(min_length=1, max_length=4096)] + previous_filename: Annotated[StrictStr, Field(min_length=1, max_length=4096)] | None + status: Annotated[StrictStr, Field(min_length=1, max_length=50)] + + class GitHubRequiredCheckFact(_FactsValue): - """One check required by base-branch protection and its head-SHA result.""" + """One check required by base-branch protection and its head-SHA result. + + ``app_id`` is the requirement's App restriction and ``app_bound`` says + whether one exists. ``check_run_app_id`` is the App that produced the judged + latest check run (null when no run was judged). ``commit_status_judged`` is + true when a commit status, which carries no App identity, was judged. + """ name: Annotated[StrictStr, Field(min_length=1, max_length=255)] app_id: StrictInt | None + app_bound: StrictBool conclusion: GitHubCheckConclusion + check_run_app_id: StrictInt | None + commit_status_judged: StrictBool class GitHubResponseDigest(_FactsValue): @@ -110,6 +153,10 @@ class GitHubPullRequestFacts(_FactsValue): merge_commit_sha: GitSha | None base_ref: Annotated[StrictStr, Field(min_length=1, max_length=255)] head_sha: GitSha + author: GitHubActorFact + reviews: tuple[GitHubReviewFact, ...] + changed_files_count: Annotated[StrictInt, Field(ge=0, le=GITHUB_MAXIMUM_LISTED_FILES)] + changed_files: tuple[GitHubChangedFileFact, ...] required_checks: tuple[GitHubRequiredCheckFact, ...] = Field(min_length=1) fetched_at: datetime responses: tuple[GitHubResponseDigest, ...] = Field(min_length=1) @@ -149,6 +196,12 @@ class GitHubFactsProtocolError(ExternalServiceProtocolError): code = "github_facts_protocol_invalid" +class GitHubFactsIncompleteError(GitHubFactsProtocolError): + """GitHub cannot list every changed file, so the facts would be partial.""" + + code = "github_facts_incomplete" + + class GitHubFactsAdapter(ExternalServiceAdapter, Protocol): """Read verified facts about one pull request with a read-only credential.""" diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py index fae1708e6..e8c8361c3 100644 --- a/backend/tests/test_github_facts_adapter.py +++ b/backend/tests/test_github_facts_adapter.py @@ -26,6 +26,7 @@ ) from app.interfaces.github_facts import ( GitHubFactsAccessDeniedError, + GitHubFactsIncompleteError, GitHubFactsProtocolError, GitHubFactsUnavailableError, GitHubPullRequestFacts, @@ -60,11 +61,35 @@ def _pull(**changes: object) -> dict: "merge_commit_sha": MERGE_SHA, "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, "head": {"sha": HEAD_SHA}, + "user": {"login": "agent-bot", "id": 501, "type": "Bot"}, + "changed_files": 2, } body.update(changes) return body +REVIEWS = [ + { + "id": 61, + "user": {"login": "reviewer", "id": 7, "type": "User"}, + "state": "APPROVED", + "commit_id": HEAD_SHA, + "submitted_at": "2027-01-15T07:00:00Z", + }, + { + "id": 62, + "user": None, + "state": "COMMENTED", + "commit_id": MERGE_SHA, + "submitted_at": "2027-01-15T07:30:00Z", + }, +] +FILES = [ + {"filename": "src/app.py", "status": "modified"}, + {"filename": ".github/workflows/ci.yml", "previous_filename": "ci.yml", "status": "renamed"}, +] + + class FakeGitHub: """Route-table GitHub fake that records every request it receives.""" @@ -76,6 +101,8 @@ def __init__(self) -> None: self.route("GET", f"/repos/{REPOSITORY}/installation", {"id": 77}) self.route("POST", "/app/installations/77/access_tokens", self._token) self.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull()) + self.route("GET", f"/repos/{REPOSITORY}/pulls/12/reviews", REVIEWS) + self.route("GET", f"/repos/{REPOSITORY}/pulls/12/files", FILES) self.route( "GET", f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", @@ -207,9 +234,49 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "merge_commit_sha": MERGE_SHA, "base_ref": "main", "head_sha": HEAD_SHA, + "author": {"login": "agent-bot", "id": 501, "type": "Bot"}, + "reviews": [ + { + "id": 61, + "reviewer": {"login": "reviewer", "id": 7, "type": "User"}, + "state": "APPROVED", + "commit_id": HEAD_SHA, + "submitted_at": "2027-01-15T07:00:00Z", + }, + { + "id": 62, + "reviewer": None, + "state": "COMMENTED", + "commit_id": MERGE_SHA, + "submitted_at": "2027-01-15T07:30:00Z", + }, + ], + "changed_files_count": 2, + "changed_files": [ + {"filename": "src/app.py", "previous_filename": None, "status": "modified"}, + { + "filename": ".github/workflows/ci.yml", + "previous_filename": "ci.yml", + "status": "renamed", + }, + ], "required_checks": [ - {"name": "backend", "app_id": 15368, "conclusion": "success"}, - {"name": "lint", "app_id": None, "conclusion": "success"}, + { + "name": "backend", + "app_id": 15368, + "app_bound": True, + "conclusion": "success", + "check_run_app_id": 15368, + "commit_status_judged": False, + }, + { + "name": "lint", + "app_id": None, + "app_bound": False, + "conclusion": "success", + "check_run_app_id": None, + "commit_status_judged": True, + }, ], "fetched_at": "2027-01-15T08:00:00Z", "responses": [ @@ -226,6 +293,18 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: ] ), }, + { + "request": "reviews.page.1", + "sha256": _sha( + fake.bodies[f"{API}/repos/{REPOSITORY}/pulls/12/reviews?per_page=100"] + ), + }, + { + "request": "files.page.1", + "sha256": _sha( + fake.bodies[f"{API}/repos/{REPOSITORY}/pulls/12/files?per_page=100"] + ), + }, { "request": "check_runs.page.1", "sha256": _sha( @@ -299,7 +378,7 @@ async def test_pagination_follows_same_origin_next_links() -> None: facts = await _fetch(_adapter(fake)) assert facts.required_checks[0].conclusion == "failure" - assert [item.request for item in facts.responses][2:4] == [ + assert [item.request for item in facts.responses][4:6] == [ "check_runs.page.1", "check_runs.page.2", ] @@ -734,3 +813,90 @@ def test_settings_reject_incomplete_or_unsafe_configuration(values: dict) -> Non def test_settings_accept_loopback_http_only_for_local_and_test() -> None: settings = _settings(github_api_base_url="http://127.0.0.1:8080") assert settings.github_api_base_url == "http://127.0.0.1:8080" + + +async def test_reviews_and_files_paginate_and_reviews_are_returned_unfiltered() -> None: + fake = FakeGitHub() + reviews_path = f"/repos/{REPOSITORY}/pulls/12/reviews" + files_path = f"/repos/{REPOSITORY}/pulls/12/files" + fake.route( + "GET", + reviews_path, + httpx.Response( + 200, + json=[REVIEWS[0]], + headers={"link": f'<{API}{reviews_path}?per_page=100&page=2>; rel="next"'}, + ), + [{**REVIEWS[0], "id": 63, "state": "CHANGES_REQUESTED"}], + ) + page_one = [{"filename": f"f{index}.py", "status": "added"} for index in range(100)] + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull(changed_files=101)) + fake.route( + "GET", + files_path, + httpx.Response( + 200, + json=page_one, + headers={"link": f'<{API}{files_path}?per_page=100&page=2>; rel="next"'}, + ), + [{"filename": "last.py", "status": "added"}], + ) + facts = await _fetch(_adapter(fake)) + assert [(item.id, item.state) for item in facts.reviews] == [ + (61, "APPROVED"), + (63, "CHANGES_REQUESTED"), + ] + assert len(facts.changed_files) == facts.changed_files_count == 101 + assert [item.request for item in facts.responses][2:6] == [ + "reviews.page.1", + "reviews.page.2", + "files.page.1", + "files.page.2", + ] + + +async def test_more_changed_files_than_github_can_list_fails_closed() -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull(changed_files=3001)) + with pytest.raises(GitHubFactsIncompleteError): + await _fetch(_adapter(fake)) + assert not any(request.url.path.endswith("/files") for request in fake.requests) + + +async def test_changed_file_listing_shorter_than_reported_fails_closed() -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull(changed_files=3)) + with pytest.raises(GitHubFactsIncompleteError): + await _fetch(_adapter(fake)) + + +async def test_check_run_app_and_status_source_are_recorded() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + {"contexts": ["lint"], "checks": []}, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + { + "check_runs": [ + { + "id": 5, + "name": "lint", + "status": "completed", + "conclusion": "success", + "app": {"id": 777}, + } + ] + }, + ) + fake.route("GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", {"statuses": []}) + facts = await _fetch(_adapter(fake)) + check = facts.required_checks[0] + assert (check.app_bound, check.check_run_app_id, check.commit_status_judged) == ( + False, + 777, + False, + ) From 0720bdcb42deaad519724b33e5f1fe06bff8125f Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:32:01 +0100 Subject: [PATCH 5/7] fix(github): bind every fact to one pull-request state Security review (time-of-check binding): re-read the pull request after all other reads and require head, merge state, base, state and file count to match; retry the whole read within the deadline, then raise the retryable GitHubFactsInconsistentError. Check runs and combined statuses must report the head SHA, and each required check records the commit it was judged on. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 22 ++- backend/app/adapters/github/app_facts.py | 142 ++++++++++++------ backend/app/interfaces/github_facts.py | 22 ++- backend/tests/test_github_facts_adapter.py | 103 +++++++++++-- 4 files changed, 228 insertions(+), 61 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index 34528ec01..eb515bbfa 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -115,6 +115,23 @@ chunk adds no transport, credential or retry logic of its own. files. If the pull request's `changed_files` exceeds 3,000, or the listing length differs from it, the read raises `GitHubFactsIncompleteError` (a protocol error), so a protected-path rule can never miss a file. +- **One pull-request state (security review, time-of-check binding).** Facts + come from several calls; reviews and files are listed by pull-request number, + not by commit, and protection can change between calls, so one object could + mix two states. After every other read the adapter re-reads the pull request + (`pull_request_final`, hashed like the first read) and requires `head_sha`, + `merged`, `merge_commit_sha`, `base_ref`, `state` and `changed_files` to equal + the first read exactly. If they differ, it discards the whole read and starts + again, up to three whole reads within the same deadline, then raises the + retryable `GitHubFactsInconsistentError` (an availability error, so chunk 04 + maps it to an infrastructure failure). The changed-file listing must match + the count both reads reported. Every check run returned for the head SHA + must report that `head_sha`, and every combined-status page must report it as + its `sha`; otherwise the read is a protocol error. The facts' `head_sha` and + `merge_commit_sha` are the single state all facts were bound to; each + required check records `check_run_head_sha` and `commit_status_sha` so the + checker can verify the binding. Reviews keep their own `commit_id`; the + checker counts only reviews of the head SHA. - **Check provenance.** Each required check also records `app_bound` (the requirement names an App), `check_run_app_id` (the App that produced the judged latest run, or null) and `commit_status_judged` (a commit status, which @@ -134,6 +151,9 @@ chunk adds no transport, credential or retry logic of its own. - [x] Author, all reviews (paginated, unfiltered) and all changed files (paginated) are returned and hashed; more files than GitHub can list, or a short listing, fails as incomplete. +- [x] A pull request that moves during a read is re-read and bound to one + state; one that keeps moving, or whose merged flag flips, fails as + inconsistent; check runs or statuses for another commit fail. - [x] Each required check records its App binding and the producing App or status source of the judged result. - [x] Happy path returns the exact facts, response digests, a verifiable JWT @@ -163,7 +183,7 @@ chunk adds no transport, credential or retry logic of its own. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 48 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 53 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | | Re-run test detects order-based selection | Temporarily selecting the last listed run made both re-run cases fail | Detected | Manual negative control | | Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | | Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py index a99d8bee3..5197e48ab 100644 --- a/backend/app/adapters/github/app_facts.py +++ b/backend/app/adapters/github/app_facts.py @@ -39,6 +39,7 @@ GitHubActorFact, GitHubChangedFileFact, GitHubFactsIncompleteError, + GitHubFactsInconsistentError, GitHubPullRequestFacts, GitHubPullRequestNotFoundError, GitHubRequiredCheckFact, @@ -60,6 +61,16 @@ # Refresh an installation token this long before GitHub's stated expiry. TOKEN_REFRESH_MARGIN_SECONDS = 300 INITIAL_BACKOFF_SECONDS = 0.5 +# Whole facts reads attempted when the pull request moves during a read. +FACTS_CONSISTENCY_ATTEMPTS = 3 +_BOUND_PULL_FIELDS = ( + "head_sha", + "merged", + "merge_commit_sha", + "base_ref", + "state", + "changed_files_count", +) _CHECK_RUN_CONCLUSIONS = frozenset( { "success", @@ -196,7 +207,6 @@ async def fetch_pull_request_facts( ): raise ExternalServiceConfigurationError(_IDENTITY) deadline = _Deadline(self._configuration.total_deadline_seconds, self._monotonic) - digests: list[GitHubResponseDigest] = [] async with httpx.AsyncClient( transport=self._transport, follow_redirects=False, @@ -206,49 +216,84 @@ async def fetch_pull_request_facts( "User-Agent": "workstream-github-facts", }, ) as client: - session = _FactsSession(self, client, repository, deadline, digests) - pull = await session.read_json( - "pull_request", - f"/repos/{repository}/pulls/{pull_request_number}", - not_found=GitHubPullRequestNotFoundError, - ) - pull_facts = _pull_request_facts(pull, repository, pull_request_number) - required = _required_checks( - await session.read_json( - "required_status_checks", - f"/repos/{repository}/branches/" - f"{quote(pull_facts['base_ref'], safe='')}" - "/protection/required_status_checks", - not_found=GitHubRequiredChecksNotConfiguredError, + for _attempt in range(FACTS_CONSISTENCY_ATTEMPTS): + digests: list[GitHubResponseDigest] = [] + session = _FactsSession(self, client, repository, deadline, digests) + facts = await self._read_consistent_facts( + session, repository, pull_request_number, digests ) + if facts is not None: + return facts + if deadline.remaining() <= 0: + break + raise GitHubFactsInconsistentError(_IDENTITY) + + async def _read_consistent_facts( + self, + session: _FactsSession, + repository: str, + pull_request_number: int, + digests: list[GitHubResponseDigest], + ) -> GitHubPullRequestFacts | None: + """Read every fact once, bracketed by two pull-request reads. + + Reviews and files are listed by pull request number, not by commit, and + protection can change between calls. The final pull-request read must + repeat the first one exactly (head SHA, merged flag, merge commit SHA, + base branch, state, changed-file count), so every fact belongs to one + pull-request state. Returns None when the state moved. + """ + pull_path = f"/repos/{repository}/pulls/{pull_request_number}" + pull = await session.read_json( + "pull_request", pull_path, not_found=GitHubPullRequestNotFoundError + ) + pull_facts = _pull_request_facts(pull, repository, pull_request_number) + required = _required_checks( + await session.read_json( + "required_status_checks", + f"/repos/{repository}/branches/" + f"{quote(pull_facts['base_ref'], safe='')}" + "/protection/required_status_checks", + not_found=GitHubRequiredChecksNotConfiguredError, ) - head_sha = pull_facts["head_sha"] - changed_files_count = pull_facts["changed_files_count"] - if changed_files_count > GITHUB_MAXIMUM_LISTED_FILES: - raise GitHubFactsIncompleteError(_IDENTITY) - reviews = await session.read_pages( - "reviews", - f"/repos/{repository}/pulls/{pull_request_number}/reviews", - item_key=None, - ) - files = await session.read_pages( - "files", - f"/repos/{repository}/pulls/{pull_request_number}/files", - item_key=None, - maximum_pages=GITHUB_MAXIMUM_LISTED_FILES // PAGE_SIZE, - ) - if len(files) != changed_files_count: - raise GitHubFactsIncompleteError(_IDENTITY) - check_runs = await session.read_pages( - "check_runs", - f"/repos/{repository}/commits/{head_sha}/check-runs", - item_key="check_runs", - ) - statuses = await session.read_pages( - "commit_status", - f"/repos/{repository}/commits/{head_sha}/status", - item_key="statuses", - ) + ) + head_sha = pull_facts["head_sha"] + changed_files_count = pull_facts["changed_files_count"] + if changed_files_count > GITHUB_MAXIMUM_LISTED_FILES: + raise GitHubFactsIncompleteError(_IDENTITY) + reviews = await session.read_pages( + "reviews", f"{pull_path}/reviews", item_key=None + ) + files = await session.read_pages( + "files", + f"{pull_path}/files", + item_key=None, + maximum_pages=GITHUB_MAXIMUM_LISTED_FILES // PAGE_SIZE, + ) + check_runs = await session.read_pages( + "check_runs", + f"/repos/{repository}/commits/{head_sha}/check-runs", + item_key="check_runs", + ) + statuses = await session.read_pages( + "commit_status", + f"/repos/{repository}/commits/{head_sha}/status", + item_key="statuses", + require_sha=head_sha, + ) + final = _pull_request_facts( + await session.read_json( + "pull_request_final", pull_path, not_found=GitHubPullRequestNotFoundError + ), + repository, + pull_request_number, + ) + if any(final[key] != pull_facts[key] for key in _BOUND_PULL_FIELDS): + return None + if len(files) != changed_files_count: + raise GitHubFactsIncompleteError(_IDENTITY) + if any(run.get("head_sha") != head_sha for run in check_runs): + raise GitHubFactsProtocolError(_IDENTITY) try: return GitHubPullRequestFacts( repository=pull_facts["repository"], @@ -270,7 +315,7 @@ async def fetch_pull_request_facts( for item in files ), required_checks=tuple( - _required_check_fact(name, app_id, check_runs, statuses) + _required_check_fact(name, app_id, check_runs, statuses, head_sha) for name, app_id in required ), fetched_at=datetime.fromtimestamp(self._wall_clock(), UTC), @@ -411,11 +456,13 @@ async def read_pages( *, item_key: str | None, maximum_pages: int = MAXIMUM_PAGES, + require_sha: str | None = None, ) -> list[dict]: """Follow same-origin ``next`` links for a bounded number of pages. ``item_key`` names the list inside an object body; None means the body itself is the list. More pages than ``maximum_pages`` fail closed. + ``require_sha`` requires each page body's ``sha`` to equal that commit. """ url = self._url(path) + f"?per_page={PAGE_SIZE}" items: list[dict] = [] @@ -423,6 +470,10 @@ async def read_pages( response = await self._installation_get(url, not_found=GitHubFactsProtocolError) self._record(f"{label}.page.{page}", response) body = _json(response) + if require_sha is not None and ( + not isinstance(body, dict) or body.get("sha") != require_sha + ): + raise GitHubFactsProtocolError(_IDENTITY) if item_key is None: page_items = body else: @@ -660,6 +711,7 @@ def _required_check_fact( app_id: int | None, check_runs: list[dict], statuses: list[dict], + head_sha: str, ) -> GitHubRequiredCheckFact: """Judge only the latest check run and latest status for one required check. @@ -676,7 +728,9 @@ def _required_check_fact( runs.append(run) run = _latest(runs) check_run_app_id = None + check_run_head_sha = None if run is not None: + check_run_head_sha = run.get("head_sha") app = run.get("app") check_run_app_id = app.get("id") if isinstance(app, dict) else None if check_run_app_id is not None and type(check_run_app_id) is not int: @@ -710,5 +764,7 @@ def _required_check_fact( app_bound=app_id is not None, conclusion=conclusion, check_run_app_id=check_run_app_id, + check_run_head_sha=check_run_head_sha, commit_status_judged=status_judged, + commit_status_sha=head_sha if status_judged else None, ) diff --git a/backend/app/interfaces/github_facts.py b/backend/app/interfaces/github_facts.py index f210c5050..a5bad6e63 100644 --- a/backend/app/interfaces/github_facts.py +++ b/backend/app/interfaces/github_facts.py @@ -118,8 +118,10 @@ class GitHubRequiredCheckFact(_FactsValue): ``app_id`` is the requirement's App restriction and ``app_bound`` says whether one exists. ``check_run_app_id`` is the App that produced the judged - latest check run (null when no run was judged). ``commit_status_judged`` is - true when a commit status, which carries no App identity, was judged. + latest check run (null when no run was judged) and ``check_run_head_sha`` the + commit GitHub reports for that run. ``commit_status_judged`` is true when a + commit status, which carries no App identity, was judged; + ``commit_status_sha`` is the commit of that combined status. """ name: Annotated[StrictStr, Field(min_length=1, max_length=255)] @@ -127,7 +129,9 @@ class GitHubRequiredCheckFact(_FactsValue): app_bound: StrictBool conclusion: GitHubCheckConclusion check_run_app_id: StrictInt | None + check_run_head_sha: GitSha | None commit_status_judged: StrictBool + commit_status_sha: GitSha | None class GitHubResponseDigest(_FactsValue): @@ -141,7 +145,10 @@ class GitHubPullRequestFacts(_FactsValue): """Immutable ``workstream.github_facts.v1`` value for one pull request. ``merge_commit_sha`` is null unless the pull request is merged, because - GitHub reports a test-merge commit for open pull requests. Serialize with + GitHub reports a test-merge commit for open pull requests. ``head_sha`` and + ``merge_commit_sha`` are the single pull-request state every other fact was + bound to: the adapter re-reads the pull request after all other reads and + discards the read if that state moved. Serialize with ``model_dump(mode="json")`` to place the value in checker request input. """ @@ -196,6 +203,15 @@ class GitHubFactsProtocolError(ExternalServiceProtocolError): code = "github_facts_protocol_invalid" +class GitHubFactsInconsistentError(ExternalServiceUnavailableError): + """The pull request kept changing while facts were read, within the deadline. + + Retryable: a later read may observe one stable state. + """ + + code = "github_facts_inconsistent" + + class GitHubFactsIncompleteError(GitHubFactsProtocolError): """GitHub cannot list every changed file, so the facts would be partial.""" diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py index e8c8361c3..ad83a163a 100644 --- a/backend/tests/test_github_facts_adapter.py +++ b/backend/tests/test_github_facts_adapter.py @@ -27,6 +27,7 @@ from app.interfaces.github_facts import ( GitHubFactsAccessDeniedError, GitHubFactsIncompleteError, + GitHubFactsInconsistentError, GitHubFactsProtocolError, GitHubFactsUnavailableError, GitHubPullRequestFacts, @@ -124,7 +125,7 @@ def __init__(self) -> None: { "id": 10, "name": "backend", - "status": "completed", + "head_sha": HEAD_SHA, "status": "completed", "conclusion": "success", "app": {"id": 15368}, } @@ -134,7 +135,7 @@ def __init__(self) -> None: self.route( "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", - {"state": "success", "statuses": [{"id": 21, "context": "lint", "state": "success"}]}, + {"sha": HEAD_SHA, "state": "success", "statuses": [{"id": 21, "context": "lint", "state": "success"}]}, ) def route(self, method: str, path: str, *responses: object) -> None: @@ -267,7 +268,9 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "app_bound": True, "conclusion": "success", "check_run_app_id": 15368, + "check_run_head_sha": HEAD_SHA, "commit_status_judged": False, + "commit_status_sha": None, }, { "name": "lint", @@ -275,7 +278,9 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "app_bound": False, "conclusion": "success", "check_run_app_id": None, + "check_run_head_sha": None, "commit_status_judged": True, + "commit_status_sha": HEAD_SHA, }, ], "fetched_at": "2027-01-15T08:00:00Z", @@ -319,6 +324,10 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: fake.bodies[f"{API}/repos/{REPOSITORY}/commits/{HEAD_SHA}/status?per_page=100"] ), }, + { + "request": "pull_request_final", + "sha256": _sha(fake.bodies[f"{API}/repos/{REPOSITORY}/pulls/12"]), + }, ], } lookup, exchange, *facts_requests = fake.requests @@ -358,7 +367,7 @@ async def test_pagination_follows_same_origin_next_links() -> None: 200, json={ "check_runs": [ - {"id": 4, "name": "other", "status": "completed", "conclusion": "failure"} + {"id": 4, "name": "other", "head_sha": HEAD_SHA, "status": "completed", "conclusion": "failure"} ] }, headers={"link": f'<{page_two}>; rel="next"'}, @@ -368,7 +377,7 @@ async def test_pagination_follows_same_origin_next_links() -> None: { "id": 11, "name": "backend", - "status": "completed", + "head_sha": HEAD_SHA, "status": "completed", "conclusion": "failure", "app": {"id": 15368}, } @@ -412,9 +421,9 @@ async def test_required_check_conclusions_fail_closed() -> None: f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", { "check_runs": [ - {"id": 1, "name": "backend", "status": "completed", "conclusion": "success"}, - {"id": 2, "name": "e2e", "status": "in_progress", "conclusion": None}, - {"id": 3, "name": "wrong-app", "status": "completed", "conclusion": "skipped"}, + {"id": 1, "name": "backend", "head_sha": HEAD_SHA, "status": "completed", "conclusion": "success"}, + {"id": 2, "name": "e2e", "head_sha": HEAD_SHA, "status": "in_progress", "conclusion": None}, + {"id": 3, "name": "wrong-app", "head_sha": HEAD_SHA, "status": "completed", "conclusion": "skipped"}, ] }, ) @@ -422,7 +431,7 @@ async def test_required_check_conclusions_fail_closed() -> None: "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", { - "statuses": [ + "sha": HEAD_SHA, "statuses": [ {"id": 23, "context": "backend", "state": "failure"}, {"id": 24, "context": "lint", "state": "error"}, ] @@ -449,7 +458,7 @@ async def test_only_the_latest_rerun_of_a_required_check_counts( ) -> None: fake = FakeGitHub() runs_path = f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs" - run = {"name": "backend", "status": "completed", "app": {"id": 15368}} + run = {"name": "backend", "head_sha": HEAD_SHA, "status": "completed", "app": {"id": 15368}} # The newer run is on the first page and the older on the second, so # response order cannot decide the result. fake.route( @@ -466,7 +475,7 @@ async def test_only_the_latest_rerun_of_a_required_check_counts( "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", { - "statuses": [ + "sha": HEAD_SHA, "statuses": [ {"id": 31, "context": "lint", "state": "failure"}, {"id": 32, "context": "lint", "state": "success"}, ] @@ -507,7 +516,7 @@ async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: { "id": 12, "name": "backend", - "status": "completed", + "head_sha": HEAD_SHA, "status": "completed", "conclusion": "success", "app": {"id": 999}, } @@ -517,7 +526,7 @@ async def test_app_restricted_check_ignores_other_apps_and_statuses() -> None: fake.route( "GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", - {"statuses": [{"id": 22, "context": "backend", "state": "success"}, {"id": 21, "context": "lint", "state": "success"}]}, + {"sha": HEAD_SHA, "statuses": [{"id": 22, "context": "backend", "state": "success"}, {"id": 21, "context": "lint", "state": "success"}]}, ) facts = await _fetch(_adapter(fake)) assert facts.required_checks[0].conclusion == "missing" @@ -885,14 +894,14 @@ async def test_check_run_app_and_status_source_are_recorded() -> None: { "id": 5, "name": "lint", - "status": "completed", + "head_sha": HEAD_SHA, "status": "completed", "conclusion": "success", "app": {"id": 777}, } ] }, ) - fake.route("GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", {"statuses": []}) + fake.route("GET", f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", {"sha": HEAD_SHA, "statuses": []}) facts = await _fetch(_adapter(fake)) check = facts.required_checks[0] assert (check.app_bound, check.check_run_app_id, check.commit_status_judged) == ( @@ -900,3 +909,69 @@ async def test_check_run_app_and_status_source_are_recorded() -> None: 777, False, ) + + +def _pull_reads(fake: FakeGitHub) -> int: + return sum(1 for r in fake.requests if r.url.path == f"/repos/{REPOSITORY}/pulls/12") + + +async def test_pull_request_moving_once_is_reread_and_bound_to_one_state() -> None: + fake = FakeGitHub() + moved = _pull(head={"sha": "c" * 40}) + # The first whole read ends on a moved head; the second sees one state. + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", _pull(), moved, _pull()) + facts = await _fetch(_adapter(fake)) + assert facts.head_sha == HEAD_SHA + assert _pull_reads(fake) == 4 + labels = [item.request for item in facts.responses] + assert labels[0] == "pull_request" and labels[-1] == "pull_request_final" + assert labels.count("pull_request") == 1 + + +async def test_head_that_keeps_moving_fails_closed_as_inconsistent() -> None: + fake = FakeGitHub() + shas = [f"{index:x}" * 40 for index in range(1, 8)] + for sha in shas: + fake.route("GET", f"/repos/{REPOSITORY}/commits/{sha}/check-runs", {"check_runs": []}) + fake.route("GET", f"/repos/{REPOSITORY}/commits/{sha}/status", {"sha": sha, "statuses": []}) + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", *(_pull(head={"sha": sha}) for sha in shas)) + with pytest.raises(GitHubFactsInconsistentError) as caught: + await _fetch(_adapter(fake)) + assert caught.value.retryable is True + assert _pull_reads(fake) == 6 + + +async def test_merged_flag_flipping_during_the_read_is_inconsistent() -> None: + fake = FakeGitHub() + open_pull = _pull(state="open", merged=False) + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12", + open_pull, _pull(), open_pull, _pull(), open_pull, _pull(), + ) + with pytest.raises(GitHubFactsInconsistentError): + await _fetch(_adapter(fake)) + + +async def test_check_run_for_another_commit_is_a_protocol_error() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + {"check_runs": [{"id": 9, "name": "backend", "head_sha": "c" * 40, + "status": "completed", "conclusion": "success", + "app": {"id": 15368}}]}, + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +async def test_combined_status_for_another_commit_is_a_protocol_error() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status", + {"sha": "c" * 40, "statuses": []}, + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) From e736c38b20c572946a0e53f3b33de509ee79d02d Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:34:43 +0100 Subject: [PATCH 6/7] feat(github): add canonical PR URL, check-run URLs and workflow run id Security review (parser differential): facts carry GitHub's pull-request html_url and each judged run's html_url, details_url and Actions workflow run id; parse_pull_request_url is the single strict parser for claimed pull-request URLs. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 15 +++- backend/app/adapters/github/app_facts.py | 39 +++++++++- backend/app/interfaces/github_facts.py | 31 ++++++++ backend/tests/test_github_facts_adapter.py | 74 +++++++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index eb515bbfa..8539d79e1 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -132,6 +132,17 @@ chunk adds no transport, credential or retry logic of its own. required check records `check_run_head_sha` and `commit_status_sha` so the checker can verify the binding. Reviews keep their own `commit_id`; the checker counts only reviews of the head SHA. +- **One canonical pull-request identity (security review, parser + differential).** The facts carry GitHub's own pull-request `html_url`. For the + judged check run they carry `check_run_html_url`, `check_run_details_url` and + `workflow_run_id`. The App has no Actions read permission, so the workflow run + id is taken only from a GitHub Actions App (id 15368) `details_url` of the + form `https://github.com/{repository}/actions/runs/{id}[/job/{job}]` for this + repository; other producers give null. `parse_pull_request_url` in the port + module is the single parser for a claimed pull-request URL: it accepts only + ASCII `https://github.com/{owner}/{name}/pull/{number}`. Chunk 04 must take + the pull-request number from this function, and the checker re-asserts exact + equality of the claim with the facts, so fetcher and checker cannot disagree. - **Check provenance.** Each required check also records `app_bound` (the requirement names an App), `check_run_app_id` (the App that produced the judged latest run, or null) and `commit_status_judged` (a commit status, which @@ -154,6 +165,8 @@ chunk adds no transport, credential or retry logic of its own. - [x] A pull request that moves during a read is re-read and bound to one state; one that keeps moving, or whose merged flag flips, fails as inconsistent; check runs or statuses for another commit fail. +- [x] Facts carry the pull-request `html_url` and each judged run's URLs and + workflow run id; the canonical URL parser rejects every non-canonical form. - [x] Each required check records its App binding and the producing App or status source of the judged result. - [x] Happy path returns the exact facts, response digests, a verifiable JWT @@ -183,7 +196,7 @@ chunk adds no transport, credential or retry logic of its own. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 53 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 73 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | | Re-run test detects order-based selection | Temporarily selecting the last listed run made both re-run cases fail | Detected | Manual negative control | | Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | | Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py index 5197e48ab..d1fafc1c7 100644 --- a/backend/app/adapters/github/app_facts.py +++ b/backend/app/adapters/github/app_facts.py @@ -15,6 +15,7 @@ from datetime import UTC, datetime import hashlib import logging +import re import time from typing import Any from urllib.parse import quote, urlsplit @@ -61,6 +62,11 @@ # Refresh an installation token this long before GitHub's stated expiry. TOKEN_REFRESH_MARGIN_SECONDS = 300 INITIAL_BACKOFF_SECONDS = 0.5 +GITHUB_ACTIONS_APP_ID = 15368 +_ACTIONS_DETAILS_URL = re.compile( + r"https://github\.com/([A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9._-]{1,100})" + r"/actions/runs/([1-9][0-9]{0,18})(?:/job/[1-9][0-9]{0,18})?" +) # Whole facts reads attempted when the pull request moves during a read. FACTS_CONSISTENCY_ATTEMPTS = 3 _BOUND_PULL_FIELDS = ( @@ -298,6 +304,7 @@ async def _read_consistent_facts( return GitHubPullRequestFacts( repository=pull_facts["repository"], pull_request_number=pull_request_number, + html_url=pull_facts["html_url"], state=pull_facts["state"], merged=pull_facts["merged"], merge_commit_sha=pull_facts["merge_commit_sha"], @@ -315,7 +322,9 @@ async def _read_consistent_facts( for item in files ), required_checks=tuple( - _required_check_fact(name, app_id, check_runs, statuses, head_sha) + _required_check_fact( + name, app_id, check_runs, statuses, head_sha, pull_facts["repository"] + ) for name, app_id in required ), fetched_at=datetime.fromtimestamp(self._wall_clock(), UTC), @@ -630,6 +639,7 @@ def _pull_request_facts(body: object, repository: str, number: int) -> dict[str, "merge_commit_sha": body["merge_commit_sha"] if merged is True else None, "base_ref": base["ref"], "head_sha": head["sha"], + "html_url": body["html_url"], "author": body["user"], "changed_files_count": body["changed_files"], } @@ -687,6 +697,25 @@ def _latest(items: list[dict]) -> dict | None: return max(items, key=lambda item: item["id"], default=None) +def _workflow_run_id(run: dict, repository: str) -> int | None: + """Return the Actions workflow run id from a GitHub Actions run's details URL. + + The App lacks Actions read permission, so the run id comes from GitHub's own + ``details_url`` (``https://github.com/{repo}/actions/runs/{id}/job/{job}``), + and only for the GitHub Actions App and this repository. + """ + app = run.get("app") + details_url = run.get("details_url") + if not isinstance(app, dict) or app.get("id") != GITHUB_ACTIONS_APP_ID: + return None + if not isinstance(details_url, str): + return None + match = _ACTIONS_DETAILS_URL.fullmatch(details_url) + if match is None or match.group(1).casefold() != repository.casefold(): + return None + return int(match.group(2)) + + def _actor(body: object) -> GitHubActorFact: """Translate one GitHub user object.""" if not isinstance(body, dict): @@ -712,6 +741,7 @@ def _required_check_fact( check_runs: list[dict], statuses: list[dict], head_sha: str, + repository: str, ) -> GitHubRequiredCheckFact: """Judge only the latest check run and latest status for one required check. @@ -729,8 +759,12 @@ def _required_check_fact( run = _latest(runs) check_run_app_id = None check_run_head_sha = None + html_url = details_url = workflow_run_id = None if run is not None: check_run_head_sha = run.get("head_sha") + html_url = run.get("html_url") + details_url = run.get("details_url") + workflow_run_id = _workflow_run_id(run, repository) app = run.get("app") check_run_app_id = app.get("id") if isinstance(app, dict) else None if check_run_app_id is not None and type(check_run_app_id) is not int: @@ -765,6 +799,9 @@ def _required_check_fact( conclusion=conclusion, check_run_app_id=check_run_app_id, check_run_head_sha=check_run_head_sha, + check_run_html_url=html_url, + check_run_details_url=details_url, + workflow_run_id=workflow_run_id, commit_status_judged=status_judged, commit_status_sha=head_sha if status_judged else None, ) diff --git a/backend/app/interfaces/github_facts.py b/backend/app/interfaces/github_facts.py index a5bad6e63..683ab1f6a 100644 --- a/backend/app/interfaces/github_facts.py +++ b/backend/app/interfaces/github_facts.py @@ -63,6 +63,29 @@ def is_github_repository(value: object) -> bool: return isinstance(value, str) and _GITHUB_REPOSITORY.fullmatch(value) is not None +_CANONICAL_PULL_REQUEST_URL = re.compile( + r"https://github\.com/([A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9._-]{1,100})" + r"/pull/([1-9][0-9]{0,9})" +) + + +def parse_pull_request_url(value: object) -> tuple[str, int] | None: + """Return (repository, number) from one canonical pull-request URL, or None. + + This is the single parser for a claimed pull-request URL. Only + ``https://github.com/{owner}/{name}/pull/{number}`` in ASCII is accepted: + no port, user information, query, fragment, trailing slash, percent + encoding or backslash. The checker re-asserts exact equality of the claim + with the facts, so the reader and the checker cannot disagree. + """ + if not isinstance(value, str) or not value.isascii(): + return None + match = _CANONICAL_PULL_REQUEST_URL.fullmatch(value) + if match is None or not is_github_repository(match.group(1)): + return None + return match.group(1), int(match.group(2)) + + def _github_repository(value: str) -> str: """Validate one canonical repository name.""" if not is_github_repository(value): @@ -122,6 +145,10 @@ class GitHubRequiredCheckFact(_FactsValue): commit GitHub reports for that run. ``commit_status_judged`` is true when a commit status, which carries no App identity, was judged; ``commit_status_sha`` is the commit of that combined status. + ``check_run_html_url`` and ``check_run_details_url`` are GitHub's URLs for the + judged run, and ``workflow_run_id`` is the GitHub Actions workflow run that + produced it, taken from a GitHub Actions ``details_url`` of this repository + (null for other producers). """ name: Annotated[StrictStr, Field(min_length=1, max_length=255)] @@ -130,6 +157,9 @@ class GitHubRequiredCheckFact(_FactsValue): conclusion: GitHubCheckConclusion check_run_app_id: StrictInt | None check_run_head_sha: GitSha | None + check_run_html_url: Annotated[StrictStr, Field(max_length=2048)] | None + check_run_details_url: Annotated[StrictStr, Field(max_length=2048)] | None + workflow_run_id: Annotated[StrictInt, Field(ge=1)] | None commit_status_judged: StrictBool commit_status_sha: GitSha | None @@ -155,6 +185,7 @@ class GitHubPullRequestFacts(_FactsValue): schema_version: Literal["workstream.github_facts.v1"] = GITHUB_FACTS_SCHEMA_VERSION repository: GitHubRepository pull_request_number: Annotated[StrictInt, Field(ge=1)] + html_url: Annotated[StrictStr, Field(min_length=1, max_length=2048)] state: Literal["open", "closed"] merged: StrictBool merge_commit_sha: GitSha | None diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py index ad83a163a..a40972f74 100644 --- a/backend/tests/test_github_facts_adapter.py +++ b/backend/tests/test_github_facts_adapter.py @@ -33,6 +33,7 @@ GitHubPullRequestFacts, GitHubPullRequestNotFoundError, GitHubRequiredChecksNotConfiguredError, + parse_pull_request_url, ) API = "https://api.github.test" @@ -62,6 +63,7 @@ def _pull(**changes: object) -> dict: "merge_commit_sha": MERGE_SHA, "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, "head": {"sha": HEAD_SHA}, + "html_url": f"https://github.com/{REPOSITORY}/pull/12", "user": {"login": "agent-bot", "id": 501, "type": "Bot"}, "changed_files": 2, } @@ -69,6 +71,7 @@ def _pull(**changes: object) -> dict: return body +RUN_DETAILS_URL = f"https://github.com/{REPOSITORY}/actions/runs/345/job/678" REVIEWS = [ { "id": 61, @@ -124,6 +127,8 @@ def __init__(self) -> None: "check_runs": [ { "id": 10, + "html_url": f"https://github.com/{REPOSITORY}/runs/10", + "details_url": RUN_DETAILS_URL, "name": "backend", "head_sha": HEAD_SHA, "status": "completed", "conclusion": "success", @@ -230,6 +235,7 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "schema_version": "workstream.github_facts.v1", "repository": REPOSITORY, "pull_request_number": 12, + "html_url": f"https://github.com/{REPOSITORY}/pull/12", "state": "closed", "merged": True, "merge_commit_sha": MERGE_SHA, @@ -269,6 +275,9 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "conclusion": "success", "check_run_app_id": 15368, "check_run_head_sha": HEAD_SHA, + "check_run_html_url": f"https://github.com/{REPOSITORY}/runs/10", + "check_run_details_url": RUN_DETAILS_URL, + "workflow_run_id": 345, "commit_status_judged": False, "commit_status_sha": None, }, @@ -279,6 +288,9 @@ async def test_happy_path_returns_facts_with_response_digests() -> None: "conclusion": "success", "check_run_app_id": None, "check_run_head_sha": None, + "check_run_html_url": None, + "check_run_details_url": None, + "workflow_run_id": None, "commit_status_judged": True, "commit_status_sha": HEAD_SHA, }, @@ -975,3 +987,65 @@ async def test_combined_status_for_another_commit_is_a_protocol_error() -> None: ) with pytest.raises(GitHubFactsProtocolError): await _fetch(_adapter(fake)) + + +@pytest.mark.parametrize( + ("app_id", "details_url", "expected"), + [ + (15368, RUN_DETAILS_URL, 345), + (15368, f"https://github.com/{REPOSITORY}/actions/runs/345", 345), + (999, RUN_DETAILS_URL, None), + (15368, "https://github.com/acme/other/actions/runs/345/job/1", None), + (15368, f"https://example.test/{REPOSITORY}/actions/runs/345", None), + (15368, None, None), + ], + ids=("actions-job", "actions-run", "other-app", "other-repo", "other-host", "none"), +) +async def test_workflow_run_id_comes_only_from_actions_runs_of_this_repository( + app_id: int, details_url: str | None, expected: int | None +) -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks", + {"contexts": [], "checks": [{"context": "backend", "app_id": app_id}]}, + ) + fake.route( + "GET", + f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs", + {"check_runs": [{"id": 9, "name": "backend", "head_sha": HEAD_SHA, + "status": "completed", "conclusion": "success", + "details_url": details_url, "app": {"id": app_id}}]}, + ) + facts = await _fetch(_adapter(fake)) + assert facts.required_checks[0].workflow_run_id == expected + assert facts.required_checks[0].check_run_details_url == details_url + + +@pytest.mark.parametrize( + "url", + [ + "https://github.com/acme/widgets/pull/12/", + "https://github.com/acme/widgets/pull/12?x=1", + "https://github.com/acme/widgets/pull/12#top", + "https://github.com:443/acme/widgets/pull/12", + "https://user@github.com/acme/widgets/pull/12", + "https://github.com/acme/widgets/pull/%31%32", + "https://github.com/acme\\widgets/pull/12", + "https://github.com/acme/widgets/pull/012", + "https://github.com/acme/widgéts/pull/12", + "http://github.com/acme/widgets/pull/12", + "https://github.com/acme/widgets/pulls/12", + "https://github.com/acme/../pull/12", + 12, + ], +) +def test_pull_request_url_parser_rejects_noncanonical_forms(url: object) -> None: + assert parse_pull_request_url(url) is None + + +def test_pull_request_url_parser_returns_repository_and_number() -> None: + assert parse_pull_request_url("https://github.com/Acme/widgets.v2/pull/12") == ( + "Acme/widgets.v2", + 12, + ) From b905f250811532910daadeef08572f128dcc6562 Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:46:18 +0100 Subject: [PATCH 7/7] test(github): prove every fail-closed facts branch rejects Rejecting tests for malformed keys, installation lookups, token exchanges, pull requests, protection, listings, check results, rate-limit headers and deadline exhaustion; remove three unreachable branches. Branch coverage of the adapter and port is now 100%. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-02.md | 9 +- backend/app/adapters/github/app_facts.py | 21 +- backend/tests/test_github_facts_adapter.py | 306 +++++++++++++++++- 3 files changed, 323 insertions(+), 13 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md index 8539d79e1..376eae4f6 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-02.md @@ -196,7 +196,8 @@ chunk adds no transport, credential or retry logic of its own. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 73 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Adapter behavior and secret custody | `pytest tests/test_github_facts_adapter.py` | 122 passed | Uses `httpx.MockTransport`; the re-run rule comes from a separate live read, not from this suite | +| Every fail-closed branch rejects | `coverage run --branch --include='app/adapters/github/*,app/interfaces/github_facts.py' -m pytest tests/test_github_facts_adapter.py`; `coverage report -m` | 100% line and branch on `app_facts.py`, `github/__init__.py` and `github_facts.py` (before: 90% and 98%) | Coverage shows execution; each added test asserts the typed error or outcome | | Re-run test detects order-based selection | Temporarily selecting the last listed run made both re-run cases fail | Detected | Manual negative control | | Secret tests detect leaks | Temporarily unredacting the token repr, and logging the Authorization value, each made one test fail | Detected | Manual negative control | | Lane catalogue, boundaries, ownership | `tests/test_ci_lane_catalogue.py`, `tests/architecture/test_module_boundaries.py`, `tests/test_behavior_ownership.py`, `scripts.behavior_ownership validate`, `module_boundaries`, `authorization_boundary`, `test_structure_boundary` | Pass | None | @@ -208,7 +209,11 @@ because `uv.lock` supports Linux only. Hosted Backend CI owns the locked run. ## Review findings -None recorded. +- Coverage review: three unreachable branches were removed rather than kept as + dead code: a deadline check between consistency attempts (the next request + already fails with `GitHubFactsUnavailableError` once the deadline is spent), + a full-URL branch in the API-path helper (only paths reach it), and the + natural exit of the retry `for` loop (every exit is an explicit break). ## Reconciliation diff --git a/backend/app/adapters/github/app_facts.py b/backend/app/adapters/github/app_facts.py index d1fafc1c7..f51f706cd 100644 --- a/backend/app/adapters/github/app_facts.py +++ b/backend/app/adapters/github/app_facts.py @@ -230,8 +230,8 @@ async def fetch_pull_request_facts( ) if facts is not None: return facts - if deadline.remaining() <= 0: - break + # An exhausted deadline already raised GitHubFactsUnavailableError from + # the next request, so reaching here means the state kept moving. raise GitHubFactsInconsistentError(_IDENTITY) async def _read_consistent_facts( @@ -421,11 +421,9 @@ def __init__( self._deadline = deadline self._digests = digests - def _url(self, path_or_url: str) -> str: + def _url(self, path: str) -> str: """Resolve an API path against the configured origin.""" - if path_or_url.startswith("/"): - return self._adapter._configuration.api_base_url + path_or_url - return path_or_url + return self._adapter._configuration.api_base_url + path async def app_json( self, @@ -548,7 +546,9 @@ async def _send( ) -> httpx.Response: """Send with bounded retries for transport errors, 5xx and rate limits.""" configuration = self._adapter._configuration - for attempt in range(1, configuration.maximum_attempts + 1): + attempt = 0 + while True: + attempt += 1 remaining = self._deadline.remaining() if remaining <= 0: break @@ -587,9 +587,10 @@ async def _send( None if response is None else response.status_code, attempt, ) - if attempt == configuration.maximum_attempts: - break - if delay >= self._deadline.remaining(): + if ( + attempt >= configuration.maximum_attempts + or delay >= self._deadline.remaining() + ): break await self._adapter._sleep(delay) raise GitHubFactsUnavailableError(_IDENTITY) from None diff --git a/backend/tests/test_github_facts_adapter.py b/backend/tests/test_github_facts_adapter.py index a40972f74..3a9b044ee 100644 --- a/backend/tests/test_github_facts_adapter.py +++ b/backend/tests/test_github_facts_adapter.py @@ -11,12 +11,14 @@ import jwt import pytest from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import rsa +from cryptography.hazmat.primitives.asymmetric import ec, rsa +from pydantic import SecretStr, ValidationError from app.adapters.github import github_facts_adapter_factory from app.adapters.github.app_facts import ( GitHubAppFactsAdapter, GitHubAppFactsConfiguration, + load_github_app_private_key, _InstallationToken, ) from app.core.config import Settings @@ -1049,3 +1051,305 @@ def test_pull_request_url_parser_returns_repository_and_number() -> None: "Acme/widgets.v2", 12, ) + + + +# Fail-closed branches: each malformed or hostile response is rejected --------- + +PROTECTION = f"/repos/{REPOSITORY}/branches/main/protection/required_status_checks" +RUNS = f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/check-runs" +STATUS = f"/repos/{REPOSITORY}/commits/{HEAD_SHA}/status" +EC_KEY = ec.generate_private_key(ec.SECP256R1()) + + +def test_private_key_loader_rejects_oversized_and_non_rsa_keys() -> None: + oversized = PRIVATE_PEM + "\n" + "A" * 20_000 + ec_pem = EC_KEY.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ).decode() + for value in (oversized, ec_pem, "é" * 10): + with pytest.raises(ExternalServiceConfigurationError) as caught: + load_github_app_private_key(SecretStr(value)) + assert caught.value.__cause__ is None + with pytest.raises(ExternalServiceConfigurationError): + GitHubAppFactsAdapter( + GitHubAppFactsConfiguration( + app_id=APP_ID, + installation_id=None, + api_base_url=API, + request_timeout_seconds=10.0, + total_deadline_seconds=45.0, + maximum_attempts=3, + ), + EC_KEY, # type: ignore[arg-type] + ) + + +@pytest.mark.parametrize( + "lookup", + [{}, {"id": "77"}, {"id": 0}, []], + ids=("no-id", "string-id", "zero-id", "not-object"), +) +async def test_unusable_installation_lookup_is_a_protocol_error(lookup: object) -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/installation", lookup) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +@pytest.mark.parametrize( + "token_body", + [ + {"token": TOKEN, "expires_at": "later"}, + {"token": TOKEN, "expires_at": "2027-01-15T09:00:00"}, + {"token": TOKEN}, + {"token": "", "expires_at": "2027-01-15T09:00:00Z"}, + {"token": "ghs_bad token", "expires_at": "2027-01-15T09:00:00Z"}, + {"token": 5, "expires_at": "2027-01-15T09:00:00Z"}, + [], + ], + ids=("bad-expiry", "naive-expiry", "no-expiry", "empty", "whitespace", "non-string", "list"), +) +async def test_unusable_token_exchange_is_a_protocol_error(token_body: object) -> None: + fake = FakeGitHub() + fake.route( + "POST", + "/app/installations/77/access_tokens", + httpx.Response(201, json=token_body), + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + assert not any(r.url.path.endswith("/pulls/12") for r in fake.requests) + + +async def test_app_jwt_rejected_is_access_denied() -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/installation", httpx.Response(401)) + with pytest.raises(GitHubFactsAccessDeniedError): + await _fetch(_adapter(fake)) + + +async def test_unexpected_client_error_is_a_protocol_error_without_retry() -> None: + fake = FakeGitHub() + clock = Clock() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(422)) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake, clock)) + assert clock.sleeps == [] + + +@pytest.mark.parametrize( + "headers", + [ + {"retry-after": "soon"}, + {"x-ratelimit-remaining": "0"}, + {"x-ratelimit-remaining": "0", "x-ratelimit-reset": "tomorrow"}, + ], + ids=("bad-retry-after", "no-reset", "bad-reset"), +) +async def test_forbidden_with_unusable_rate_limit_headers_is_access_denied( + headers: dict, +) -> None: + fake = FakeGitHub() + clock = Clock() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", httpx.Response(403, headers=headers)) + with pytest.raises(GitHubFactsAccessDeniedError): + await _fetch(_adapter(fake, clock)) + assert clock.sleeps == [] + + +async def test_deadline_spent_by_one_slow_response_stops_further_requests() -> None: + fake = FakeGitHub() + clock = Clock() + + def slow_pull(_request: httpx.Request) -> httpx.Response: + clock.now += 60.0 + return httpx.Response(200, json=_pull()) + + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", slow_pull) + with pytest.raises(GitHubFactsUnavailableError): + await _fetch(_adapter(fake, clock, deadline=45.0)) + assert not any(r.url.path == PROTECTION for r in fake.requests) + + +@pytest.mark.parametrize( + "pull", + [ + [], + {k: v for k, v in _pull().items() if k != "base"}, + _pull(head={"sha": None}), + _pull(changed_files="2"), + _pull(changed_files=-1), + _pull(number=13), + _pull(user=None), + _pull(user={"login": "agent-bot", "id": 501}), + _pull(merged=True, merge_commit_sha="not-a-sha"), + _pull(state="draft"), + ], + ids=( + "not-object", + "no-base", + "null-head", + "string-file-count", + "negative-file-count", + "other-number", + "null-author", + "author-without-type", + "bad-merge-sha", + "unknown-state", + ), +) +async def test_malformed_pull_request_is_a_protocol_error(pull: object) -> None: + fake = FakeGitHub() + fake.route("GET", f"/repos/{REPOSITORY}/pulls/12", pull) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +@pytest.mark.parametrize( + "protection", + [ + [], + {"checks": "backend", "contexts": []}, + {"checks": [], "contexts": "backend"}, + {"checks": [{"app_id": 1}], "contexts": []}, + {"checks": [{"context": "", "app_id": 1}], "contexts": []}, + {"checks": ["backend"], "contexts": []}, + {"checks": [], "contexts": [7]}, + ], + ids=( + "not-object", + "checks-not-list", + "contexts-not-list", + "check-without-context", + "empty-context", + "check-not-object", + "context-not-string", + ), +) +async def test_malformed_branch_protection_is_a_protocol_error(protection: object) -> None: + fake = FakeGitHub() + fake.route("GET", PROTECTION, protection) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +@pytest.mark.parametrize( + ("path", "body"), + [ + (RUNS, {"check_runs": "backend"}), + (RUNS, {"check_runs": ["backend"]}), + (RUNS, []), + (f"/repos/{REPOSITORY}/pulls/12/reviews", {"reviews": []}), + (STATUS, {"sha": HEAD_SHA, "statuses": None}), + ], + ids=("runs-not-list", "run-not-object", "runs-body-list", "reviews-object", "statuses-null"), +) +async def test_malformed_listing_page_is_a_protocol_error(path: str, body: object) -> None: + fake = FakeGitHub() + fake.route("GET", path, body) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +async def test_listing_longer_than_the_page_bound_fails_closed() -> None: + fake = FakeGitHub() + reviews = f"/repos/{REPOSITORY}/pulls/12/reviews" + fake.route( + "GET", + reviews, + httpx.Response( + 200, + json=[REVIEWS[0]], + headers={"link": f'<{API}{reviews}?per_page=100&page=99>; rel="next"'}, + ), + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + assert sum(r.url.path == reviews for r in fake.requests) == 10 + + +@pytest.mark.parametrize( + ("runs", "statuses"), + [ + ([{"name": "lint", "head_sha": HEAD_SHA, "status": "completed", + "conclusion": "success"}], []), + ([{"id": "5", "name": "lint", "head_sha": HEAD_SHA, "status": "completed", + "conclusion": "success"}], []), + ([{"id": 5, "name": "lint", "head_sha": HEAD_SHA, "status": "completed", + "conclusion": "success", "app": {"id": "15368"}}], []), + ([{"id": 5, "name": "lint", "head_sha": HEAD_SHA, "status": "completed", + "conclusion": "exploded"}], []), + ([], [{"id": 1, "context": "lint", "state": "green"}]), + ([], [{"context": "lint", "state": "success"}]), + ], + ids=( + "run-without-id", + "run-string-id", + "run-string-app-id", + "unknown-conclusion", + "unknown-status-state", + "status-without-id", + ), +) +async def test_malformed_check_result_is_a_protocol_error(runs: list, statuses: list) -> None: + fake = FakeGitHub() + fake.route("GET", PROTECTION, {"contexts": ["lint"], "checks": []}) + fake.route("GET", RUNS, {"check_runs": runs}) + fake.route("GET", STATUS, {"sha": HEAD_SHA, "statuses": statuses}) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +async def test_malformed_review_is_a_protocol_error() -> None: + fake = FakeGitHub() + fake.route( + "GET", + f"/repos/{REPOSITORY}/pulls/12/reviews", + [{**REVIEWS[0], "commit_id": "not-a-sha"}], + ) + with pytest.raises(GitHubFactsProtocolError): + await _fetch(_adapter(fake)) + + +def test_facts_model_rejects_a_noncanonical_repository() -> None: + facts = GitHubPullRequestFacts.model_validate( + { + "repository": REPOSITORY, + "pull_request_number": 12, + "html_url": f"https://github.com/{REPOSITORY}/pull/12", + "state": "closed", + "merged": True, + "merge_commit_sha": MERGE_SHA, + "base_ref": "main", + "head_sha": HEAD_SHA, + "author": {"login": "a", "id": 1, "type": "User"}, + "reviews": [], + "changed_files_count": 0, + "changed_files": [], + "required_checks": [ + { + "name": "backend", + "app_id": 1, + "app_bound": True, + "conclusion": "success", + "check_run_app_id": 1, + "check_run_head_sha": HEAD_SHA, + "check_run_html_url": None, + "check_run_details_url": None, + "workflow_run_id": None, + "commit_status_judged": False, + "commit_status_sha": None, + } + ], + "fetched_at": "2027-01-15T08:00:00Z", + "responses": [{"request": "pull_request", "sha256": "sha256:" + "0" * 64}], + } + ) + for repository in ("acme", "acme/..", "-acme/widgets"): + with pytest.raises(ValidationError): + GitHubPullRequestFacts.model_validate( + {**facts.model_dump(), "repository": repository} + )