From 8dd5e3eb154faf429656ea411c6e2a5f405f85b5 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Mon, 7 Sep 2026 20:31:31 +0000 Subject: [PATCH] Page to the end when searching for an existing PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find_pull_request read only the newest 50 per state, so once a repo passed that many pull requests a "not found" would mean "not in the newest page" — and the A3 reconciler would read that as "the PR was never opened" and open a duplicate. Exactly the side effect the intent log exists to prevent. It now pages until a short page proves the end, and raises rather than answering "absent" from an incomplete search; the reconciler turns that into unverifiable and gates to the human. Found while wiring CI: every CI run opens a PR in the sandbox, so the threshold would have been crossed in weeks rather than never. Co-Authored-By: Claude Opus 5 --- nisse-dev/src/nisse_dev/gitea.py | 30 ++++++++++++---- nisse-dev/tests/test_gitea.py | 61 +++++++++++++++++++++++--------- 2 files changed, 68 insertions(+), 23 deletions(-) diff --git a/nisse-dev/src/nisse_dev/gitea.py b/nisse-dev/src/nisse_dev/gitea.py index a2d244d..80ab20f 100644 --- a/nisse-dev/src/nisse_dev/gitea.py +++ b/nisse-dev/src/nisse_dev/gitea.py @@ -20,6 +20,9 @@ import httpx from nisse_dev.reconcile import Reconciler from nisse_dev.state import Intent +PR_PAGE_SIZE = 50 +MAX_PR_PAGES = 40 # 2000 PRs; beyond that, say so rather than guess + class GiteaError(RuntimeError): pass @@ -92,16 +95,31 @@ class GiteaClient: def find_pull_request(self, repo: str, head: str) -> PullRequest | None: """External truth for A3: did the PR for this branch already get - opened? Checks closed PRs too — a crash after opening must not be - answered with "no PR is open, so open another".""" - for state in ("open", "closed"): + opened? Includes closed PRs — a crash after opening must not be + answered with "no PR is open, so open another". + + Pages to the end rather than sampling the first page: a "not found" + that only means "not in the newest 50" would licence a duplicate PR. + If the history is longer than this walk, raise instead of guessing — + the reconciler turns that into "unverifiable" and gates to the human. + """ + for page in range(1, MAX_PR_PAGES + 1): data = self._request( - "GET", f"/repos/{repo}/pulls", params={"state": state, "limit": 50} + "GET", + f"/repos/{repo}/pulls", + params={"state": "all", "limit": PR_PAGE_SIZE, "page": page}, ) - for item in data or []: + items = data or [] + for item in items: if (item.get("head") or {}).get("ref") == head: return _pull_request(item) - return None + if len(items) < PR_PAGE_SIZE: + return None # reached the end: definitively absent + raise GiteaError( + f"searched {MAX_PR_PAGES * PR_PAGE_SIZE} pull requests in {repo} " + f"without reaching the end; cannot confirm whether {head!r} was " + "already opened" + ) def list_comments( self, repo: str, index: int, since: str | None = None diff --git a/nisse-dev/tests/test_gitea.py b/nisse-dev/tests/test_gitea.py index db463c8..2f2190c 100644 --- a/nisse-dev/tests/test_gitea.py +++ b/nisse-dev/tests/test_gitea.py @@ -4,6 +4,7 @@ import httpx import pytest from nisse_dev.gitea import ( + PR_PAGE_SIZE, Comment, GiteaClient, GiteaError, @@ -61,27 +62,58 @@ def test_open_pull_request_posts_expected_body() -> None: def test_find_pull_request_matches_head_branch() -> None: - def handler(request: httpx.Request) -> httpx.Response: - if request.url.params.get("state") == "open": - return httpx.Response(200, json=[pr_json(3, "other")]) - return httpx.Response(200, json=[]) - + handler = lambda request: httpx.Response(200, json=[pr_json(3, "other")]) assert client(handler).find_pull_request("o/r", "milestone-1") is None -def test_find_pull_request_also_checks_closed() -> None: +def test_find_pull_request_also_finds_closed() -> None: # A crash after opening must not be answered with "nothing open, open # another" — the effect already happened. - def handler(request: httpx.Request) -> httpx.Response: - if request.url.params.get("state") == "open": - return httpx.Response(200, json=[]) - return httpx.Response(200, json=[pr_json(4, "milestone-1", "closed")]) - + handler = lambda request: httpx.Response( + 200, json=[pr_json(4, "milestone-1", "closed")] + ) found = client(handler).find_pull_request("o/r", "milestone-1") assert found is not None assert found.number == 4 +def test_find_pull_request_pages_past_the_first_page() -> None: + # The match sits on page 2; sampling only the newest page would report + # "absent" and licence a duplicate PR. + def handler(request: httpx.Request) -> httpx.Response: + page = int(request.url.params.get("page", 1)) + if page == 1: + return httpx.Response( + 200, json=[pr_json(i, f"other-{i}") for i in range(PR_PAGE_SIZE)] + ) + return httpx.Response(200, json=[pr_json(999, "milestone-1", "closed")]) + + found = client(handler).find_pull_request("o/r", "milestone-1") + assert found is not None + assert found.number == 999 + + +def test_short_page_ends_the_search_definitively() -> None: + calls: list[int] = [] + + def handler(request: httpx.Request) -> httpx.Response: + calls.append(int(request.url.params.get("page", 1))) + return httpx.Response(200, json=[pr_json(1, "other")]) + + assert client(handler).find_pull_request("o/r", "milestone-1") is None + assert calls == [1] # one short page proves absence; no further paging + + +def test_unbounded_history_raises_rather_than_guessing() -> None: + # Never answer "absent" from an incomplete search: the reconciler turns + # this error into "unverifiable" and gates to the human. + handler = lambda request: httpx.Response( + 200, json=[pr_json(i, f"other-{i}") for i in range(PR_PAGE_SIZE)] + ) + with pytest.raises(GiteaError, match="without reaching the end"): + client(handler).find_pull_request("o/r", "milestone-1") + + def test_error_status_raises_gitea_error() -> None: handler = lambda request: httpx.Response(403, text="forbidden") with pytest.raises(GiteaError, match="403"): @@ -139,12 +171,7 @@ def test_from_env_requires_config() -> None: def test_reconciler_reports_existing_pr_as_done() -> None: - handler = lambda request: httpx.Response( - 200, - json=[pr_json(5, "milestone-1")] - if request.url.params.get("state") == "open" - else [], - ) + handler = lambda request: httpx.Response(200, json=[pr_json(5, "milestone-1")]) assert pull_request_reconciler(client(handler), "o/r")(INTENT) is True