From faf973be6c85b23d6db366ad248b9c543d802cc6 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Fri, 16 Oct 2026 22:16:27 +0000 Subject: [PATCH 1/2] nisse-review: match repository patterns against Gitea's search At bring-up, `dev/*` failed: listing an organization's repositories (/orgs/dev/repos) needs the read:organization token scope, which the reviewer's token doesn't have and shouldn't need. Patterns are now matched against /repos/search, which needs only the repository scope and already lists every repository the account can read. A pattern that matches nothing is logged (access.no_match). owner_repositories goes. A Gitea error at startup is now reported like a config error, not a traceback. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- docs/Architecture.md | 6 ++--- docs/Development-Plan.md | 6 ++--- nisse-core/src/nisse_core/gitea.py | 8 ------- nisse-core/tests/test_gitea.py | 13 ----------- nisse-review/src/nisse_review/access.py | 30 +++++++++++-------------- nisse-review/src/nisse_review/main.py | 3 ++- nisse-review/tests/review_fakes.py | 10 +-------- nisse-review/tests/test_review.py | 7 +++--- 8 files changed, 25 insertions(+), 58 deletions(-) diff --git a/docs/Architecture.md b/docs/Architecture.md index d3512fc..c78b665 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -882,9 +882,9 @@ else) towards better changes. a draft, and not marked work in progress. Any author's: the operator's, `nisse-dev`'s, or another tool's. Its repositories are the ones its Gitea account was given, or, when configured, those matching patterns (`dev/*`, - `*/*`) among all it can read, public ones included: Gitea leaves a public - repository's Read collaborators out of an account's own list, so access to - public repositories is managed in configuration, and to private ones in Gitea. + `*/*`) among all it can read, public ones included, so a public repository + needs no grant in Gitea: access to public repositories is managed in + configuration, and to private ones in Gitea. - **A work item per PR,** scoped to its repository (L1). It closes when the PR merges or closes. - **What starts a run:** the PR becoming ready, a new push, or a new comment or diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index 70f956c..dec30b4 100644 --- a/docs/Development-Plan.md +++ b/docs/Development-Plan.md @@ -608,9 +608,9 @@ waits until two exist to compare. Moved, unchanged in behaviour: ready unless a draft or titled WIP; closed when the PR merges or closes. Every open PR is reviewed, those open when it starts included (operator). Since #153, `repositories` holds patterns (`dev/*`, `*/*`) matched against - every repository it can read, public ones included (Gitea's own list for the - account leaves out a public repository's Read collaborators, found at - bring-up); the read-only wall is checked every round, not only at startup. + every repository it can read, public ones included, through Gitea's search + (`/repos/search`, which needs no `read:organization` scope); the read-only wall + is checked every round, not only at startup. - **Reading (R3):** the repository at the PR's head and base, from Gitea's archives, unpacked into a scratch folder and read in Python (`read_file`, `list_files`, `search_code`), and its issues (`read_issue`, diff --git a/nisse-core/src/nisse_core/gitea.py b/nisse-core/src/nisse_core/gitea.py index 45bdcaa..09c0f0c 100644 --- a/nisse-core/src/nisse_core/gitea.py +++ b/nisse-core/src/nisse_core/gitea.py @@ -421,14 +421,6 @@ class GiteaClient: return found raise GiteaError("too many repositories to list") - def owner_repositories(self, owner: str) -> list[str]: - """Every repository of an organization (or a user) this account can - read, as "owner/name".""" - try: - return self._paged_names(f"/orgs/{owner}/repos") - except GiteaNotFound: - return self._paged_names(f"/users/{owner}/repos") - def visible_repositories(self) -> list[str]: """Every repository this account can read, public ones included.""" return self._paged_names("/repos/search", key="data") diff --git a/nisse-core/tests/test_gitea.py b/nisse-core/tests/test_gitea.py index 814117c..eb1abd2 100644 --- a/nisse-core/tests/test_gitea.py +++ b/nisse-core/tests/test_gitea.py @@ -361,19 +361,6 @@ def test_readable_repositories_page_to_the_end() -> None: assert len(found) == PR_PAGE_SIZE + 2 and found[-1] == "o/r2-1" -def test_owner_repositories_try_the_organization_then_the_user() -> None: - asked: list[str] = [] - - def handler(request: httpx.Request) -> httpx.Response: - asked.append(request.url.path) - if request.url.path.startswith("/api/v1/orgs/"): - return httpx.Response(404, json={"message": "not found"}) - return httpx.Response(200, json=[{"full_name": "don/r"}]) - - assert client(handler).owner_repositories("don") == ["don/r"] - assert asked == ["/api/v1/orgs/don/repos", "/api/v1/users/don/repos"] - - def test_visible_repositories_read_the_search_and_a_404_is_not_found() -> None: def handler(request: httpx.Request) -> httpx.Response: if request.url.path.endswith("/repos/search"): diff --git a/nisse-review/src/nisse_review/access.py b/nisse-review/src/nisse_review/access.py index 7b7590e..fd3b293 100644 --- a/nisse-review/src/nisse_review/access.py +++ b/nisse-review/src/nisse_review/access.py @@ -3,7 +3,10 @@ The repositories are worked out again every round: ``repositories`` in the configuration is a list of patterns ("dev/nisse", "dev/*", "*/*"), matched against what the account can read, public repositories included; unset, it -is every repository the account was given (``/user/repos``). Access is +is every repository the account was given (``/user/repos``). Patterns are +matched against Gitea's search, which needs only the token's repository +scope: listing an organization's repositories would need +``read:organization`` too. Access is managed in Gitea for private repositories and in the configuration for public ones. @@ -35,22 +38,15 @@ def reviewed( """The repositories to review, sorted.""" if patterns is None: return sorted(gitea.readable_repositories()) - found: set[str] = set() - for pattern in patterns: - owner, _, name = pattern.partition("/") - if not _glob(owner) and not _glob(name): - found.add(pattern) # checked by ``check``: it must be readable - continue - try: - candidates = ( - gitea.visible_repositories() - if _glob(owner) - else gitea.owner_repositories(owner) - ) - except GiteaNotFound: - oplog.emit("access.no_owner", cause="no such owner", pattern=pattern) - continue - found |= {repo for repo in candidates if fnmatchcase(repo, pattern)} + found = {p for p in patterns if not _glob(p)} # ``check`` sees they're readable + globs = [p for p in patterns if _glob(p)] + if globs: + visible = gitea.visible_repositories() + for pattern in globs: + matched = {repo for repo in visible if fnmatchcase(repo, pattern)} + if not matched: + oplog.emit("access.no_match", cause="matches nothing", pattern=pattern) + found |= matched return sorted(found) diff --git a/nisse-review/src/nisse_review/main.py b/nisse-review/src/nisse_review/main.py index 7f98148..bffa6d9 100644 --- a/nisse-review/src/nisse_review/main.py +++ b/nisse-review/src/nisse_review/main.py @@ -9,6 +9,7 @@ from types import FrameType from nisse_core.config import ConfigError from nisse_core.envfile import load_env_file +from nisse_core.gitea import GiteaError from nisse_review.access import AccessRefused from nisse_review.build import StartupError, build from nisse_review.config import ReviewConfig @@ -19,7 +20,7 @@ def main() -> int: try: config = ReviewConfig.load() built = build(config) - except (ConfigError, StartupError) as exc: + except (ConfigError, StartupError, GiteaError) as exc: print(f"nisse-review: {exc}", file=sys.stderr) return 2 diff --git a/nisse-review/tests/review_fakes.py b/nisse-review/tests/review_fakes.py index 9620627..8dca73b 100644 --- a/nisse-review/tests/review_fakes.py +++ b/nisse-review/tests/review_fakes.py @@ -70,8 +70,7 @@ class FakeGitea: comments: list[tuple[str, int, str]] = field(default_factory=list) refuse_off_diff: bool = False down: bool = False - # Readable but not given (public): missing from /user/repos, as Gitea - # leaves a public repository's Read collaborators out of it. + # Readable but not given (public, no grant): missing from /user/repos. public: set[str] = field(default_factory=set) def _up(self) -> None: @@ -89,13 +88,6 @@ class FakeGitea: self._up() return list(self.repos) - def owner_repositories(self, owner: str) -> list[str]: - self._up() - found = [r for r in self.repos if r.startswith(f"{owner}/")] - if not found: - raise GiteaNotFound(f"no owner {owner}") - return found - def repository(self, repo: str) -> dict[str, Any]: if repo not in self.repos: raise GiteaNotFound(f"no repository {repo}") diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index b3ddffd..d9a2e89 100644 --- a/nisse-review/tests/test_review.py +++ b/nisse-review/tests/test_review.py @@ -311,9 +311,8 @@ def test_an_account_that_can_write_is_refused(tmp_path: Path) -> None: def test_patterns_reach_public_repositories_the_account_was_not_given( tmp_path: Path, ) -> None: - # Gitea leaves a public repository's Read collaborators out of - # /user/repos: a pattern finds it all the same (option a: the - # configuration decides). + # A public repository the account wasn't given isn't in /user/repos: a + # pattern finds it all the same (option a: the configuration decides). gitea = FakeGitea( repos={"dev/nisse": [pr(7)], "dev/wordle": [], "infra/tofu": [pr(3)]}, public={"dev/nisse", "dev/wordle", "infra/tofu"}, @@ -336,7 +335,7 @@ def test_a_configured_repository_it_cannot_read_is_left_out() -> None: patterns = {REPO, "o/gone", "nobody/*"} assert check(gitea, reviewed(gitea, patterns, log), log) == [REPO] # type: ignore[arg-type] assert '"access.unreadable"' in out.getvalue() and "o/gone" in out.getvalue() - assert '"access.no_owner"' in out.getvalue() + assert '"access.no_match"' in out.getvalue() and "nobody/*" in out.getvalue() def test_write_access_granted_while_running_stops_it(tmp_path: Path) -> None: -- 2.54.0 From 6e697b55a89f8d7ad2df5c5eb2785226118fb7af Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Fri, 16 Oct 2026 22:56:43 +0000 Subject: [PATCH 2/2] nisse-review: one unlistable repository holds up no other Found at bring-up: an empty private repository's /pulls answered 404, and intake stopped there, so the whole round failed and nothing was reviewed. A repository whose PRs can't be listed is now logged (intake.repo_failed) and skipped for the round; its items stay as they are. Also, from nisse-review's review of this PR: a two-page test pins the search listing the patterns rest on. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- nisse-core/tests/test_gitea.py | 17 +++++++++++++++++ nisse-review/src/nisse_review/scheduler.py | 10 +++++++++- nisse-review/tests/review_fakes.py | 3 +++ nisse-review/tests/test_review.py | 14 ++++++++++++++ 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/nisse-core/tests/test_gitea.py b/nisse-core/tests/test_gitea.py index eb1abd2..edb51a5 100644 --- a/nisse-core/tests/test_gitea.py +++ b/nisse-core/tests/test_gitea.py @@ -375,6 +375,23 @@ def test_visible_repositories_read_the_search_and_a_404_is_not_found() -> None: gitea.repository("d/gone") +def test_visible_repositories_page_through_the_search() -> None: + # The patterns rest on the search listing everything (#154's review). + asked: list[dict[str, str]] = [] + + def handler(request: httpx.Request) -> httpx.Response: + asked.append(dict(request.url.params)) + page = int(request.url.params["page"]) + count = PR_PAGE_SIZE if page == 1 else 3 + data = [{"full_name": f"o/r{page}-{n}"} for n in range(count)] + return httpx.Response(200, json={"ok": True, "data": data}) + + found = client(handler).visible_repositories() + assert len(found) == PR_PAGE_SIZE + 3 and found[-1] == "o/r2-2" + assert [a["page"] for a in asked] == ["1", "2"] + assert all(a["limit"] == str(PR_PAGE_SIZE) for a in asked) + + def test_a_pull_request_carries_what_a_review_needs() -> None: def handler(request: httpx.Request) -> httpx.Response: if request.url.path.endswith(".diff"): diff --git a/nisse-review/src/nisse_review/scheduler.py b/nisse-review/src/nisse_review/scheduler.py index b611b74..d51aee0 100644 --- a/nisse-review/src/nisse_review/scheduler.py +++ b/nisse-review/src/nisse_review/scheduler.py @@ -106,8 +106,16 @@ class Scheduler: repos = reviewed(self._gitea, self._patterns, self._oplog) open_items = {i.id: i for i in self._store.work_items("open")} for repo in repos: + try: + prs = self._gitea.open_pull_requests(repo) + except GiteaError as exc: + # One repository Gitea won't list (an empty one, one without + # pull requests) holds up none of the others; its items stay + # as they are. + self._oplog.emit("intake.repo_failed", cause=str(exc)[:300], repo=repo) + continue seen: set[str] = set() - for pr in self._gitea.open_pull_requests(repo): + for pr in prs: key = item_id(repo, pr.number) seen.add(key) self._see(open_items.get(key), repo, pr) diff --git a/nisse-review/tests/review_fakes.py b/nisse-review/tests/review_fakes.py index 8dca73b..5014320 100644 --- a/nisse-review/tests/review_fakes.py +++ b/nisse-review/tests/review_fakes.py @@ -72,6 +72,7 @@ class FakeGitea: down: bool = False # Readable but not given (public, no grant): missing from /user/repos. public: set[str] = field(default_factory=set) + unlisted: set[str] = field(default_factory=set) # its PRs can't be listed def _up(self) -> None: if self.down: @@ -99,6 +100,8 @@ class FakeGitea: def open_pull_requests(self, repo: str) -> list[PullRequest]: self._up() + if repo in self.unlisted: + raise GiteaNotFound(f"gitea GET /repos/{repo}/pulls returned 404") return list(self.repos[repo]) def pull_request(self, repo: str, index: int) -> PullRequest: diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index d9a2e89..b5fa65e 100644 --- a/nisse-review/tests/test_review.py +++ b/nisse-review/tests/test_review.py @@ -328,6 +328,20 @@ def test_patterns_reach_public_repositories_the_account_was_not_given( assert w["store"].work_item("infra/tofu#3") is None +def test_a_repository_whose_prs_cannot_be_listed_holds_up_no_other( + tmp_path: Path, +) -> None: + # Found at bring-up: an empty private repository's /pulls was a 404, + # and the round stopped there, so no repository was reviewed. + gitea = FakeGitea(repos={"a/empty": [], REPO: [pr()]}, unlisted={"a/empty"}) + out = io.StringIO() + w = world(tmp_path, Scripted(*REVIEW), gitea) + w["scheduler"]._oplog = OpLog("t", out) + assert w["scheduler"].round() is True # REPO's PR was reviewed + assert item(w).position == REVIEWED + assert '"intake.repo_failed"' in out.getvalue() and "a/empty" in out.getvalue() + + def test_a_configured_repository_it_cannot_read_is_left_out() -> None: gitea = FakeGitea(repos={REPO: []}) out = io.StringIO() -- 2.54.0