diff --git a/builders/server/core/github/__init__.py b/builders/server/core/github/__init__.py new file mode 100644 index 0000000..eb6ff35 --- /dev/null +++ b/builders/server/core/github/__init__.py @@ -0,0 +1,7 @@ +from core.github.client import ( + BranchAlreadyExistsError, + GitHubClient, + GitHubError, +) + +__all__ = ["BranchAlreadyExistsError", "GitHubClient", "GitHubError"] diff --git a/builders/server/core/github/client.py b/builders/server/core/github/client.py new file mode 100644 index 0000000..0d07d32 --- /dev/null +++ b/builders/server/core/github/client.py @@ -0,0 +1,209 @@ +"""Minimal GitHub REST client for opening dataset-proposal pull requests. + +The dataset-creation flow never writes to the server's own scripts directory; +it proposes new datasets as PRs so code review stays the gate for anything +that will execute on the server. This client holds the bot token and wraps +the handful of REST calls needed: read a branch sha, commit files to a new +branch (single commit via the git data api), and open the PR. +""" + +import os + +import requests +import structlog + +logger = structlog.get_logger() + +DEFAULT_API_ROOT = "https://api.github.com" +DEFAULT_REPO = "Wat-Street/datastream" +REQUEST_TIMEOUT_SECONDS = 15.0 + + +class GitHubError(Exception): + """Raised when a GitHub API call fails.""" + + def __init__(self, status: int, message: str) -> None: + super().__init__(f"github api error {status}: {message}") + self.status = status + self.message = message + + +class BranchAlreadyExistsError(GitHubError): + """Raised when the proposal branch already exists (open proposal).""" + + +class GitHubClient: + """Thin wrapper over the GitHub REST api, authenticated with a bot token.""" + + def __init__( + self, + token: str, + repo: str = DEFAULT_REPO, + session: requests.Session | None = None, + api_root: str = DEFAULT_API_ROOT, + ) -> None: + self.repo = repo + self._token = token + self._session = session or requests.Session() + self._api_root = api_root + + @classmethod + def from_env(cls) -> "GitHubClient": + """Build a client from GITHUB_TOKEN / GITHUB_REPO / GITHUB_API_URL env vars. + + Raises GitHubError if no token is configured. + """ + token = os.environ.get("GITHUB_TOKEN", "") + if not token: + raise GitHubError(0, "GITHUB_TOKEN is not configured on the server") + return cls( + token=token, + repo=os.environ.get("GITHUB_REPO", DEFAULT_REPO), + api_root=os.environ.get("GITHUB_API_URL", DEFAULT_API_ROOT), + ) + + def _request(self, method: str, path: str, json_body: dict | None = None) -> dict: + res = self._session.request( + method, + f"{self._api_root}{path}", + json=json_body, + headers={ + "Authorization": f"Bearer {self._token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + timeout=REQUEST_TIMEOUT_SECONDS, + ) + if res.status_code >= 400: + try: + message = str(res.json().get("message", res.text)) + except ValueError: + message = res.text + logger.error( + "github api call failed", + method=method, + path=path, + status=res.status_code, + message=message, + ) + # the git refs api reports an existing branch as a 422 + if res.status_code == 422 and "already exists" in message.lower(): + raise BranchAlreadyExistsError(res.status_code, message) + raise GitHubError(res.status_code, message) + return dict(res.json()) + + def get_branch_sha(self, branch: str) -> str: + """Return the commit sha a branch currently points at.""" + ref = self._request("GET", f"/repos/{self.repo}/git/ref/heads/{branch}") + return str(ref["object"]["sha"]) + + def commit_files_to_new_branch( + self, + branch: str, + base_sha: str, + message: str, + files: dict[str, str], + ) -> str: + """Create one commit containing `files` on a new branch off base_sha. + + `files` maps repo-relative paths to text content. Returns the commit + sha. Raises BranchAlreadyExistsError if the branch exists. + """ + tree = self._request( + "POST", + f"/repos/{self.repo}/git/trees", + { + "base_tree": base_sha, + "tree": [ + {"path": path, "mode": "100644", "type": "blob", "content": content} + for path, content in sorted(files.items()) + ], + }, + ) + commit = self._request( + "POST", + f"/repos/{self.repo}/git/commits", + {"message": message, "tree": tree["sha"], "parents": [base_sha]}, + ) + self._request( + "POST", + f"/repos/{self.repo}/git/refs", + {"ref": f"refs/heads/{branch}", "sha": commit["sha"]}, + ) + return str(commit["sha"]) + + def create_pull(self, title: str, body: str, head: str, base: str) -> dict: + """Open a PR and return the api response (html_url, number, ...).""" + return self._request( + "POST", + f"/repos/{self.repo}/pulls", + {"title": title, "body": body, "head": head, "base": base}, + ) + + def request_reviewers(self, pr_number: int, reviewers: list[str]) -> None: + """Best-effort reviewer assignment. + + github rejects the WHOLE batch if any single login is invalid (pr + author, non-collaborator), which would silently drop the valid + reviewers too — so on batch failure, retry each login individually. + Failures must not fail the proposal, so they are only logged. + """ + try: + self._request( + "POST", + f"/repos/{self.repo}/pulls/{pr_number}/requested_reviewers", + {"reviewers": reviewers}, + ) + return + except GitHubError as e: + logger.warning( + "batch reviewer request failed, retrying individually", + pr_number=pr_number, + reviewers=reviewers, + error=str(e), + ) + for reviewer in reviewers: + try: + self._request( + "POST", + f"/repos/{self.repo}/pulls/{pr_number}/requested_reviewers", + {"reviewers": [reviewer]}, + ) + except GitHubError as e: + logger.warning( + "reviewer request failed", + pr_number=pr_number, + reviewer=reviewer, + error=str(e), + ) + + def open_pr_with_files( + self, + branch: str, + base: str, + title: str, + body: str, + commit_message: str, + files: dict[str, str], + reviewers: list[str] | None = None, + ) -> str: + """Commit `files` to a new branch off `base` and open a PR. + + Returns the PR's html url. + """ + base_sha = self.get_branch_sha(base) + self.commit_files_to_new_branch(branch, base_sha, commit_message, files) + pr = self.create_pull(title=title, body=body, head=branch, base=base) + if reviewers: + # the token's own account authors the pr and cannot review it + author = str(pr.get("user", {}).get("login", "")) + eligible = [r for r in reviewers if r.lower() != author.lower()] + if eligible: + self.request_reviewers(int(pr["number"]), eligible) + logger.info( + "proposal pr opened", + repo=self.repo, + branch=branch, + pr_url=pr.get("html_url"), + ) + return str(pr["html_url"]) diff --git a/builders/server/tests/core/github/__init__.py b/builders/server/tests/core/github/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/builders/server/tests/core/github/test_client.py b/builders/server/tests/core/github/test_client.py new file mode 100644 index 0000000..39b8226 --- /dev/null +++ b/builders/server/tests/core/github/test_client.py @@ -0,0 +1,264 @@ +import json + +import pytest +import requests +from core.github.client import ( + BranchAlreadyExistsError, + GitHubClient, + GitHubError, +) +from requests.adapters import BaseAdapter + + +class FakeAdapter(BaseAdapter): + """records outgoing requests and plays back canned (status, body) responses.""" + + def __init__(self, responses: list[tuple[int, dict]]): + super().__init__() + self.responses = list(responses) + self.sent: list[requests.PreparedRequest] = [] + + def send(self, request, **kwargs): # noqa: ANN001 -- test double + self.sent.append(request) + status, body = self.responses.pop(0) + res = requests.Response() + res.status_code = status + res._content = json.dumps(body).encode() + res.headers["Content-Type"] = "application/json" + res.request = request + return res + + def close(self): + pass + + +def _client( + responses: list[tuple[int, dict]], +) -> tuple[GitHubClient, FakeAdapter]: + session = requests.Session() + adapter = FakeAdapter(responses) + session.mount("https://", adapter) + return GitHubClient(token="tok", repo="acme/data", session=session), adapter + + +def _body(request: requests.PreparedRequest) -> dict: + assert request.body is not None + return dict(json.loads(request.body)) + + +def test_open_pr_with_files_happy_path() -> None: + """the full flow: read base sha, tree, commit, branch ref, pr.""" + client, adapter = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (201, {"ref": "refs/heads/add-dataset/foo-0.1.0"}), + (201, {"html_url": "https://github.com/acme/data/pull/7", "number": 7}), + ] + ) + + url = client.open_pr_with_files( + branch="add-dataset/foo-0.1.0", + base="main", + title="feat: add dataset foo/0.1.0", + body="body", + commit_message="feat: add dataset foo/0.1.0", + files={"b/two.py": "print(2)\n", "a/one.toml": "x = 1\n"}, + ) + + assert url == "https://github.com/acme/data/pull/7" + paths = [(req.method, req.path_url) for req in adapter.sent] + assert paths == [ + ("GET", "/repos/acme/data/git/ref/heads/main"), + ("POST", "/repos/acme/data/git/trees"), + ("POST", "/repos/acme/data/git/commits"), + ("POST", "/repos/acme/data/git/refs"), + ("POST", "/repos/acme/data/pulls"), + ] + + assert adapter.sent[0].headers["Authorization"] == "Bearer tok" + + tree_body = _body(adapter.sent[1]) + assert tree_body["base_tree"] == "base-sha" + # files are sorted by path for deterministic trees + assert [entry["path"] for entry in tree_body["tree"]] == ["a/one.toml", "b/two.py"] + + commit_body = _body(adapter.sent[2]) + assert commit_body == { + "message": "feat: add dataset foo/0.1.0", + "tree": "tree-sha", + "parents": ["base-sha"], + } + + ref_body = _body(adapter.sent[3]) + assert ref_body == { + "ref": "refs/heads/add-dataset/foo-0.1.0", + "sha": "commit-sha", + } + + pr_body = _body(adapter.sent[4]) + assert pr_body["head"] == "add-dataset/foo-0.1.0" + assert pr_body["base"] == "main" + + +_PR_CREATED = ( + 201, + { + "html_url": "https://github.com/acme/data/pull/7", + "number": 7, + "user": {"login": "bot-account"}, + }, +) + + +def test_reviewers_requested_after_pr_created() -> None: + client, adapter = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (201, {"ref": "refs/heads/b"}), + _PR_CREATED, + (201, {"requested_reviewers": []}), + ] + ) + client.open_pr_with_files( + branch="b", + base="main", + title="t", + body="b", + commit_message="m", + files={"a": "1"}, + reviewers=["alice", "bob"], + ) + assert adapter.sent[-1].path_url == "/repos/acme/data/pulls/7/requested_reviewers" + assert _body(adapter.sent[-1]) == {"reviewers": ["alice", "bob"]} + + +def test_pr_author_excluded_from_reviewers() -> None: + """the token's account cannot review its own pr; it is filtered out.""" + client, adapter = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (201, {"ref": "refs/heads/b"}), + _PR_CREATED, + (201, {"requested_reviewers": []}), + ] + ) + client.open_pr_with_files( + branch="b", + base="main", + title="t", + body="b", + commit_message="m", + files={"a": "1"}, + reviewers=["alice", "Bot-Account"], + ) + assert _body(adapter.sent[-1]) == {"reviewers": ["alice"]} + + +def test_author_only_reviewer_list_skips_request() -> None: + client, adapter = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (201, {"ref": "refs/heads/b"}), + _PR_CREATED, + ] + ) + client.open_pr_with_files( + branch="b", + base="main", + title="t", + body="b", + commit_message="m", + files={"a": "1"}, + reviewers=["bot-account"], + ) + # no reviewer call was made: the pr creation is the last request + assert adapter.sent[-1].path_url == "/repos/acme/data/pulls" + + +def test_batch_reviewer_failure_retries_individually() -> None: + """one invalid login must not drop the valid reviewers with it.""" + client, adapter = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (201, {"ref": "refs/heads/b"}), + _PR_CREATED, + (422, {"message": "Reviews may only be requested from collaborators."}), + (201, {"requested_reviewers": []}), + (422, {"message": "Reviews may only be requested from collaborators."}), + ] + ) + url = client.open_pr_with_files( + branch="b", + base="main", + title="t", + body="b", + commit_message="m", + files={"a": "1"}, + reviewers=["alice", "not-a-collaborator"], + ) + assert url == "https://github.com/acme/data/pull/7" + # batch failed, then each reviewer was retried on its own + reviewer_calls = [ + _body(req) + for req in adapter.sent + if req.path_url.endswith("/requested_reviewers") + ] + assert reviewer_calls == [ + {"reviewers": ["alice", "not-a-collaborator"]}, + {"reviewers": ["alice"]}, + {"reviewers": ["not-a-collaborator"]}, + ] + + +def test_api_error_carries_status_and_message() -> None: + client, _ = _client([(404, {"message": "Not Found"})]) + with pytest.raises(GitHubError) as exc_info: + client.get_branch_sha("main") + assert exc_info.value.status == 404 + assert "Not Found" in exc_info.value.message + + +def test_existing_branch_raises_specific_error() -> None: + """the git refs api reports an existing branch as a 422.""" + client, _ = _client( + [ + (200, {"object": {"sha": "base-sha"}}), + (201, {"sha": "tree-sha"}), + (201, {"sha": "commit-sha"}), + (422, {"message": "Reference already exists"}), + ] + ) + with pytest.raises(BranchAlreadyExistsError): + client.open_pr_with_files( + branch="add-dataset/foo-0.1.0", + base="main", + title="t", + body="b", + commit_message="m", + files={"a": "1"}, + ) + + +def test_from_env_requires_token(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + with pytest.raises(GitHubError): + GitHubClient.from_env() + + +def test_from_env_reads_repo_and_api_root(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "tok") + monkeypatch.setenv("GITHUB_REPO", "acme/data") + monkeypatch.setenv("GITHUB_API_URL", "http://localhost:9999") + client = GitHubClient.from_env() + assert client.repo == "acme/data" + assert client._api_root == "http://localhost:9999"