From ae940f45644971d5937b755d2459eceffeb6500a Mon Sep 17 00:00:00 2001 From: drew Date: Mon, 18 May 2026 14:00:11 -0400 Subject: [PATCH] =?UTF-8?q?feat(controller):=20Phase=201d-3c-4=20=E2=80=94?= =?UTF-8?q?=20HTTP=20adapter=20wiring=20callbacks=20to=20=5Fclaim=5Fruntim?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The production glue between the controller's callback protocols (discovery / forgejo_writes / merging) and the existing Forgejo HTTP client in _claim_runtime. Single build_callbacks(cfg) factory; tests use the same module with a fake runtime stub. tools/controller/master/forgejo_http.py: - ForgejoCallbacks dataclass bundling every callback the controller needs: list_prs, list_issues, list_comments, post_comment, get_labels, add_label, remove_label, merge_pr. - build_callbacks(cfg, runtime=None): wires each callback as a thin closure over runtime.get/post/delete. Production omits ``runtime`` to use the real _claim_runtime module; tests inject a fake. - Forgejo path conventions match the API: GET /repos/{o}/{r}/pulls?state=open GET /repos/{o}/{r}/issues?state=open&type=issues (excludes PRs) GET/POST /repos/{o}/{r}/issues/{n}/comments GET/POST /repos/{o}/{r}/issues/{n}/labels DELETE /repos/{o}/{r}/issues/{n}/labels/{name} (URL-encoded) POST /repos/{o}/{r}/pulls/{n}/merge (body: {"Do": "merge"}) - Robust response handling: - list endpoints: non-200 → empty list; non-list body → empty; non-dict items filtered out. - post_comment: 200/201 ok; other → RuntimeError. - add_label / remove_label: 200/201/204 → True; remove-404 → True (label already gone = goal achieved); else False. - merge_pr: returns normalized MergeResponse. On 404, fetches the PR's actual state (merged=True → pr_state='merged'; state='closed' → 'closed'; PR fetch failure or non-200 → leave pr_state=None so the merging handler defaults to ABANDONED conservatively). - Any callback exception → synthetic 503 so the merging handler's retry logic kicks in cleanly. 23 new tests in test_master_forgejo_http.py: - list_prs (path format, non-200 → empty, non-list body → empty, filters non-dict items) - list_issues (type=issues filter) - list_comments (path format) - post_comment (201, 200, non-2xx raises, non-dict body) - labels (get / add 201/500 / remove 204/404/URL-encoded) - merge (200, 409, 500, callback-raises-as-503, 404+merged, 404+closed, 404+pull-fetch-failure) Plus a bug fix surfaced by the test_404_with_pull_fetch_failure test: the PR-state-fetch branch was returning 'open' on a 500 response; now correctly checks status==200 before inspecting the body. Total: 366 controller tests; full auto_agents suite 2728 pass. --- .../controller/test_master_forgejo_http.py | 240 ++++++++++++++++ tools/controller/master/__init__.py | 7 + tools/controller/master/forgejo_http.py | 259 ++++++++++++++++++ 3 files changed, 506 insertions(+) create mode 100644 tests/auto_agents/controller/test_master_forgejo_http.py create mode 100644 tools/controller/master/forgejo_http.py diff --git a/tests/auto_agents/controller/test_master_forgejo_http.py b/tests/auto_agents/controller/test_master_forgejo_http.py new file mode 100644 index 000000000..988288274 --- /dev/null +++ b/tests/auto_agents/controller/test_master_forgejo_http.py @@ -0,0 +1,240 @@ +"""Tests for the HTTP adapter wiring controller callbacks to +_claim_runtime. + +Uses a fake runtime to stub out network calls. Verifies the +adapter: +- builds correct Forgejo paths +- decodes the {"status": int, "body": ...} shape +- handles 404 / 5xx / list-vs-dict body shapes +- distinguishes externally-merged vs externally-closed on 404 merge +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +import pytest + +from tools.controller.master import ( + ForgejoCallbacks, + MergeResponse, + build_callbacks, +) + + +# ─── fake runtime ──────────────────────────────────────────────────── + + +@dataclass +class FakeRuntime: + """Stub of ``_claim_runtime`` for tests. + + Records every call; returns scripted responses keyed by + (method, path). Default response = HTTP 200 + empty body. + """ + + responses: dict[tuple[str, str], dict[str, Any]] = field(default_factory=dict) + calls: list[dict[str, Any]] = field(default_factory=list) + + def stub(self, method: str, path: str, status: int, body: Any) -> None: + self.responses[(method, path)] = {"status": status, "body": body} + + def _record(self, method: str, path: str, body: Any = None) -> dict: + self.calls.append({"method": method, "path": path, "body": body}) + return self.responses.get((method, path), {"status": 200, "body": None}) + + def get(self, path: str, _cfg: Any) -> dict: + return self._record("GET", path) + + def post(self, path: str, _cfg: Any, body: Any) -> dict: + return self._record("POST", path, body) + + def delete(self, path: str, _cfg: Any) -> dict: + return self._record("DELETE", path) + + +@pytest.fixture +def runtime(): + return FakeRuntime() + + +@pytest.fixture +def cb(runtime): + """Built callbacks against the fake runtime. cfg=None — we don't + use it in the fake.""" + return build_callbacks(cfg=None, runtime=runtime) + + +# ─── discovery ─────────────────────────────────────────────────────── + + +class TestListPRs: + def test_path_format(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/pulls?state=open", 200, + [{"number": 30, "title": "x"}]) + prs = cb.list_prs("o", "r") + assert prs == [{"number": 30, "title": "x"}] + assert runtime.calls[0]["path"] == "/repos/o/r/pulls?state=open" + + def test_non_200_returns_empty(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/pulls?state=open", 500, None) + assert cb.list_prs("o", "r") == [] + + def test_non_list_body_returns_empty(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/pulls?state=open", 200, {"err": "x"}) + assert cb.list_prs("o", "r") == [] + + def test_filters_non_dict_items(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/pulls?state=open", 200, + [{"number": 1}, "junk", {"number": 2}]) + assert len(cb.list_prs("o", "r")) == 2 + + +class TestListIssues: + def test_path_includes_type_issues_filter(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/issues?state=open&type=issues", 200, []) + cb.list_issues("o", "r") + # The path uses ``type=issues`` so PRs aren't double-counted. + assert "type=issues" in runtime.calls[0]["path"] + + +# ─── comments ──────────────────────────────────────────────────────── + + +class TestListComments: + def test_path_format(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/issues/30/comments", 200, + [{"id": 1, "body": "hi"}]) + comments = cb.list_comments("o", "r", 30) + assert comments == [{"id": 1, "body": "hi"}] + + +class TestPostComment: + def test_201_returns_body(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/comments", 201, + {"id": 99, "body": "posted"}) + result = cb.post_comment("o", "r", 30, "msg") + assert result["id"] == 99 + assert runtime.calls[0]["body"] == {"body": "msg"} + + def test_200_also_accepted(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/comments", 200, {"id": 1}) + result = cb.post_comment("o", "r", 30, "msg") + assert result["id"] == 1 + + def test_non_2xx_raises(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/comments", 500, + {"err": "boom"}) + with pytest.raises(RuntimeError, match="HTTP 500"): + cb.post_comment("o", "r", 30, "msg") + + def test_non_dict_body_yields_id_none(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/comments", 201, "ok-string") + result = cb.post_comment("o", "r", 30, "msg") + assert result == {"id": None} + + +# ─── labels ────────────────────────────────────────────────────────── + + +class TestLabels: + def test_get_labels(self, runtime, cb): + runtime.stub("GET", "/repos/o/r/issues/30/labels", 200, + [{"name": "lint", "id": 1}]) + assert cb.get_labels("o", "r", 30) == [{"name": "lint", "id": 1}] + + def test_add_label_201_returns_true(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/labels", 201, []) + assert cb.add_label("o", "r", 30, "controller-managed") is True + assert runtime.calls[0]["body"] == {"labels": ["controller-managed"]} + + def test_add_label_500_returns_false(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/issues/30/labels", 500, None) + assert cb.add_label("o", "r", 30, "controller-managed") is False + + def test_remove_label_204_returns_true(self, runtime, cb): + runtime.stub( + "DELETE", "/repos/o/r/issues/30/labels/controller-managed", + 204, None, + ) + assert cb.remove_label("o", "r", 30, "controller-managed") is True + + def test_remove_label_404_returns_true(self, runtime, cb): + """If the label is already gone, remove is a successful no-op.""" + runtime.stub( + "DELETE", "/repos/o/r/issues/30/labels/already-gone", + 404, None, + ) + assert cb.remove_label("o", "r", 30, "already-gone") is True + + def test_remove_label_url_encodes_name(self, runtime, cb): + runtime.stub( + "DELETE", "/repos/o/r/issues/30/labels/needs%20review", + 204, None, + ) + assert cb.remove_label("o", "r", 30, "needs review") is True + # Path includes the encoded name. + assert "needs%20review" in runtime.calls[0]["path"] + + +# ─── merge ─────────────────────────────────────────────────────────── + + +class TestMerge: + def test_200_returns_success(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 200, {"sha": "abc"}) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 200 + assert r.pr_state is None + + def test_409_returns_conflict(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 409, + {"message": "conflict"}) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 409 + assert "conflict" in (r.error_message or "") + + def test_500_treated_as_5xx(self, runtime, cb): + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 500, None) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 500 + + def test_callback_raises_yields_synthetic_503(self, runtime, cb): + class _Boom: + def get(self, *a, **k): return {"status": 200, "body": []} + def post(self, *a, **k): + raise RuntimeError("network unreachable") + def delete(self, *a, **k): return {"status": 200, "body": None} + cb = build_callbacks(cfg=None, runtime=_Boom()) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 503 + assert "raised" in (r.error_message or "") + + def test_404_with_externally_merged_pr(self, runtime, cb): + """404 on merge + pull says merged=True → pr_state='merged'.""" + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 404, + {"message": "Not Found"}) + runtime.stub("GET", "/repos/o/r/pulls/30", 200, + {"number": 30, "merged": True, "state": "closed"}) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 404 + assert r.pr_state == "merged" + + def test_404_with_externally_closed_pr(self, runtime, cb): + """404 on merge + pull says state='closed', merged=False + → pr_state='closed'.""" + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 404, None) + runtime.stub("GET", "/repos/o/r/pulls/30", 200, + {"number": 30, "merged": False, "state": "closed"}) + r = cb.merge_pr("o", "r", 30) + assert r.pr_state == "closed" + + def test_404_with_pull_fetch_failure_yields_no_pr_state(self, runtime, cb): + """If we can't fetch the PR state, leave pr_state=None (the + merging handler defaults to ABANDONED conservatively).""" + runtime.stub("POST", "/repos/o/r/pulls/30/merge", 404, None) + runtime.stub("GET", "/repos/o/r/pulls/30", 500, None) + r = cb.merge_pr("o", "r", 30) + assert r.status_code == 404 + # 500 body → not a merged dict; pr_state stays None. + assert r.pr_state is None diff --git a/tools/controller/master/__init__.py b/tools/controller/master/__init__.py index 65ccc759b..18f44c9f5 100644 --- a/tools/controller/master/__init__.py +++ b/tools/controller/master/__init__.py @@ -61,6 +61,10 @@ from .merging import ( MergingHandlerReport, run_merging_tick, ) +from .forgejo_http import ( + ForgejoCallbacks, + build_callbacks, +) from .loop import ( MasterConfig, MasterTickReport, @@ -112,4 +116,7 @@ __all__ = [ "MergeResponse", "MergingHandlerReport", "run_merging_tick", + # HTTP adapter (Forgejo wiring) + "ForgejoCallbacks", + "build_callbacks", ] diff --git a/tools/controller/master/forgejo_http.py b/tools/controller/master/forgejo_http.py new file mode 100644 index 000000000..8931a9538 --- /dev/null +++ b/tools/controller/master/forgejo_http.py @@ -0,0 +1,259 @@ +"""HTTP adapters wiring the controller's callback protocols to +``tools/_claim_runtime``. + +The discovery / scheduler-prefetch / forgejo_writes / merging modules +all take callback functions for Forgejo I/O. Tests inject synthetic +callbacks; production wires them via this module's +``build_callbacks(cfg)`` factory. + +The factory takes a ``RuntimeContext`` (from ``_claim_runtime``) and +returns a frozen ``ForgejoCallbacks`` dataclass with every callback +the controller needs. The callbacks are thin closures over +``_claim_runtime.get / post / patch / delete``. + +Path conventions match Forgejo's API: +- list PRs: GET /repos/{owner}/{repo}/pulls?state=open +- list issues: GET /repos/{owner}/{repo}/issues?state=open&type=issues +- list comments: GET /repos/{owner}/{repo}/issues/{n}/comments +- post comment: POST /repos/{owner}/{repo}/issues/{n}/comments +- get labels: GET /repos/{owner}/{repo}/issues/{n}/labels +- add label: POST /repos/{owner}/{repo}/issues/{n}/labels +- remove label: DELETE /repos/{owner}/{repo}/issues/{n}/labels/{label_id} +- merge PR: POST /repos/{owner}/{repo}/pulls/{n}/merge + +Forgejo treats issues + PRs interchangeably at the timeline level +(labels + comments use issue endpoints for both). +""" +from __future__ import annotations + +import logging +from dataclasses import dataclass +from typing import Any, Protocol + +from . import forgejo_writes as fw +from . import merging as mg +from . import discovery as ds + +logger = logging.getLogger(__name__) + + +class _ClaimRuntime(Protocol): + """Just the parts of ``_claim_runtime`` we use. Lets tests inject + a fake without importing the real module.""" + + def get(self, path: str, cfg: Any) -> dict[str, Any]: ... + def post(self, path: str, cfg: Any, body: Any) -> dict[str, Any]: ... + def delete(self, path: str, cfg: Any) -> dict[str, Any]: ... + + +@dataclass +class ForgejoCallbacks: + """Bundle of every callback the controller needs. Built by + ``build_callbacks``. Each callback is a thin closure over + ``_claim_runtime``.""" + + list_prs: ds.ListPRsCallback + list_issues: ds.ListIssuesCallback + list_comments: fw.ListCommentsCallback + post_comment: fw.PostCommentCallback + get_labels: fw.GetLabelsCallback + add_label: fw.AddLabelCallback + remove_label: fw.RemoveLabelCallback + merge_pr: mg.MergeCallback + + +def build_callbacks( + cfg: Any, *, runtime: _ClaimRuntime | None = None, +) -> ForgejoCallbacks: + """Wire callbacks for a given RuntimeContext. + + Tests pass a fake ``runtime`` with ``.get / .post / .delete`` + methods that return the standard ``{"status": int, "body": ...}`` + shape. Production omits ``runtime`` and the wiring uses the real + module. + """ + if runtime is None: + import sys + from pathlib import Path + tools_dir = Path(__file__).resolve().parents[2] + if str(tools_dir) not in sys.path: + sys.path.insert(0, str(tools_dir)) + from tools import _claim_runtime as runtime # type: ignore[no-redef] + + return ForgejoCallbacks( + list_prs=_make_list_prs(cfg, runtime), + list_issues=_make_list_issues(cfg, runtime), + list_comments=_make_list_comments(cfg, runtime), + post_comment=_make_post_comment(cfg, runtime), + get_labels=_make_get_labels(cfg, runtime), + add_label=_make_add_label(cfg, runtime), + remove_label=_make_remove_label(cfg, runtime), + merge_pr=_make_merge_pr(cfg, runtime), + ) + + +# ─── discovery ─────────────────────────────────────────────────────── + + +def _make_list_prs(cfg, runtime): + def list_prs(owner: str, repo: str) -> list[dict]: + path = f"/repos/{owner}/{repo}/pulls?state=open" + resp = runtime.get(path, cfg) + return _list_or_empty(resp, "list_prs") + return list_prs + + +def _make_list_issues(cfg, runtime): + def list_issues(owner: str, repo: str) -> list[dict]: + # Forgejo's /issues endpoint returns both issues + PRs by + # default; type=issues filters out PRs (which list_prs handles). + path = f"/repos/{owner}/{repo}/issues?state=open&type=issues" + resp = runtime.get(path, cfg) + return _list_or_empty(resp, "list_issues") + return list_issues + + +# ─── status comments ───────────────────────────────────────────────── + + +def _make_list_comments(cfg, runtime): + def list_comments(owner: str, repo: str, pr_number: int) -> list[dict]: + path = f"/repos/{owner}/{repo}/issues/{int(pr_number)}/comments" + resp = runtime.get(path, cfg) + return _list_or_empty(resp, "list_comments") + return list_comments + + +def _make_post_comment(cfg, runtime): + def post_comment( + owner: str, repo: str, pr_number: int, body: str, + ) -> dict: + path = f"/repos/{owner}/{repo}/issues/{int(pr_number)}/comments" + resp = runtime.post(path, cfg, {"body": body}) + if int(resp.get("status") or 0) not in (200, 201): + raise RuntimeError( + f"post_comment HTTP {resp.get('status')}: " + f"{_summarise_body(resp.get('body'))}" + ) + body_obj = resp.get("body") or {} + if not isinstance(body_obj, dict): + return {"id": None} + return body_obj + return post_comment + + +# ─── labels ────────────────────────────────────────────────────────── + + +def _make_get_labels(cfg, runtime): + def get_labels(owner: str, repo: str, pr_number: int) -> list[dict]: + path = f"/repos/{owner}/{repo}/issues/{int(pr_number)}/labels" + resp = runtime.get(path, cfg) + return _list_or_empty(resp, "get_labels") + return get_labels + + +def _make_add_label(cfg, runtime): + def add_label( + owner: str, repo: str, pr_number: int, label_name: str, + ) -> bool: + path = f"/repos/{owner}/{repo}/issues/{int(pr_number)}/labels" + # Forgejo accepts {"labels": ["label_name"]} on POST. + resp = runtime.post(path, cfg, {"labels": [label_name]}) + return int(resp.get("status") or 0) in (200, 201) + return add_label + + +def _make_remove_label(cfg, runtime): + def remove_label( + owner: str, repo: str, pr_number: int, label_name: str, + ) -> bool: + # Forgejo's DELETE-by-name endpoint: + # DELETE /repos/{owner}/{repo}/issues/{n}/labels/{label_name} + # (some Forgejo deployments use label_id; if so, the caller + # would need to look it up first via get_labels) + from urllib.parse import quote + path = ( + f"/repos/{owner}/{repo}/issues/{int(pr_number)}/" + f"labels/{quote(label_name, safe='')}" + ) + resp = runtime.delete(path, cfg) + return int(resp.get("status") or 0) in (200, 204, 404) + return remove_label + + +# ─── merge ─────────────────────────────────────────────────────────── + + +def _make_merge_pr(cfg, runtime): + def merge_pr(owner: str, repo: str, pr_number: int) -> mg.MergeResponse: + path = f"/repos/{owner}/{repo}/pulls/{int(pr_number)}/merge" + try: + resp = runtime.post(path, cfg, {"Do": "merge"}) + except Exception as exc: # noqa: BLE001 — bubble as transient + # Synthetic 503 — the merging handler treats this as a + # retryable failure. + return mg.MergeResponse( + status_code=503, + error_message=f"merge HTTP call raised: {exc}", + ) + status = int(resp.get("status") or 0) + body = resp.get("body") + err = _summarise_body(body) if status >= 400 else None + + # For 404, try to fetch the PR state to distinguish + # externally-merged vs externally-closed. If the PR fetch + # itself fails (5xx / 404 / other), leave pr_state=None + # so the merging handler defaults to ABANDONED conservatively. + pr_state: str | None = None + if status == 404: + try: + pr_resp = runtime.get( + f"/repos/{owner}/{repo}/pulls/{int(pr_number)}", cfg, + ) + if int(pr_resp.get("status") or 0) == 200: + pr_body = pr_resp.get("body") or {} + if isinstance(pr_body, dict): + if pr_body.get("merged") is True: + pr_state = "merged" + elif pr_body.get("state") == "closed": + pr_state = "closed" + else: + pr_state = "open" + except Exception: + # Couldn't determine; leave None → handler defaults + # to ABANDONED per its conservative policy. + pass + + return mg.MergeResponse( + status_code=status, pr_state=pr_state, error_message=err, + ) + return merge_pr + + +# ─── helpers ───────────────────────────────────────────────────────── + + +def _list_or_empty(resp: dict, action: str) -> list[dict]: + """Normalize a list-GET response into a list of dicts.""" + status = int(resp.get("status") or 0) + if status != 200: + logger.warning("%s: HTTP %s; treating as empty", action, status) + return [] + body = resp.get("body") + if not isinstance(body, list): + return [] + return [item for item in body if isinstance(item, dict)] + + +def _summarise_body(body: Any) -> str: + """Truncate a Forgejo error body to fit in a one-line error.""" + if body is None: + return "" + s = str(body) + if len(s) <= 200: + return s + return s[:200] + f"… ({len(s)} chars)" + + +__all__ = ["ForgejoCallbacks", "build_callbacks"]