Merge pull request 'nisse-review: repository patterns, and the wall checked every round' (#153) from review-repo-patterns into main

docs/Architecture.md

@@ -878,9 +878,13 @@ else) towards better changes.
 
 ### R1. How it works
 
-- **The duty:** watching the repositories its Gitea account can read for PRs
+- **The duty:** watching its repositories for PRs ready for review: open, not
-  ready for review: open, not a draft, and not marked work in progress. Any
+  a draft, and not marked work in progress. Any author's: the operator's,
-  author's: the operator's, `nisse-dev`'s, or another tool'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.
 - **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
@@ -912,7 +916,10 @@ Two agents can argue forever, so a PR's exchanges are capped.
 ### R3. Authority and reach
 
 - **Read-only on the repository:** code at any ref (not only what the PR touches),
-  documents, issues, other PRs, CI status, through its Gitea account. Reads are
+  documents, issues, other PRs, CI status, through its Gitea account. The wall is
+  checked at startup and every round: an account that can push to, or
+  administer, a repository it reviews is refused, and stops if granted that
+  while running. Reads are
   bound by the harness to the item's repository, so what it might repeat in a
   comment is already visible to that repository's readers.
 - **It writes only comments and reviews on the item's PR.** Its reviews never

docs/Development-Plan.md

@@ -607,6 +607,10 @@ waits until two exist to compare. Moved, unchanged in behaviour:
   account can read (or the configured `repositories`); a work item per PR,
   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.
 - **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`,

nisse-core/src/nisse_core/gitea.py

@@ -28,6 +28,10 @@ class GiteaError(RuntimeError):
     pass
 
 
+class GiteaNotFound(GiteaError):
+    """A 404: no such thing, or nothing this account may see."""
+
+
 @dataclass(frozen=True)
 class PullRequest:
     number: int
@@ -123,7 +127,8 @@ class GiteaClient:
         except httpx.HTTPError as exc:
             raise GiteaError(f"gitea request failed: {exc}") from exc
         if response.status_code >= 400:
-            raise GiteaError(
+            error = GiteaNotFound if response.status_code == 404 else GiteaError
+            raise error(
                 f"gitea {method} {path} returned {response.status_code}: "
                 f"{response.text[:300]}"
             )
@@ -416,6 +421,30 @@ 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")
+
+    def _paged_names(self, path: str, key: str | None = None) -> list[str]:
+        found: list[str] = []
+        for page in range(1, MAX_PR_PAGES + 1):
+            answer = self._request(
+                "GET", path, params={"limit": PR_PAGE_SIZE, "page": page}
+            )
+            items = (answer or {}).get(key, []) if key else (answer or [])
+            found += [str(item["full_name"]) for item in items]
+            if len(items) < PR_PAGE_SIZE:
+                return found
+        raise GiteaError("too many repositories to list")
+
     def open_pull_requests(self, repo: str) -> list[PullRequest]:
         found: list[PullRequest] = []
         for page in range(1, MAX_PR_PAGES + 1):

nisse-core/tests/test_gitea.py

@@ -8,6 +8,7 @@ from nisse_core.gitea import (
     Comment,
     GiteaClient,
     GiteaError,
+    GiteaNotFound,
     pull_request_reconciler,
 )
 from nisse_core.state.store import Intent
@@ -360,6 +361,33 @@ 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"):
+            return httpx.Response(
+                200, json={"ok": True, "data": [{"full_name": "d/n"}]}
+            )
+        return httpx.Response(404, json={"message": "not found"})
+
+    gitea = client(handler)
+    assert gitea.visible_repositories() == ["d/n"]
+    with pytest.raises(GiteaNotFound):
+        gitea.repository("d/gone")
+
+
 def test_a_pull_request_carries_what_a_review_needs() -> None:
     def handler(request: httpx.Request) -> httpx.Response:
         if request.url.path.endswith(".diff"):

nisse-review.example.yaml

@@ -4,8 +4,8 @@
 # - Secrets are ${VARIABLE} references, resolved from the environment (or the
 #   .env file NISSE_ENV names) at startup. Nothing else goes there.
 # - Unknown keys are errors.
-# - Its Gitea account must have read access only, to every repository it can
+# - Its Gitea account must have read access only to every repository it
-#   see: startup refuses an account that can push to one (R3).
+#   reviews: it refuses to start, and stops, if it can push to one (R3).
 
 state_db: ~/nisse-review/state.db
 
@@ -24,7 +24,9 @@ gitea:
   token: ${NISSE_GITEA_TOKEN}         # the nisse-review account's
   operators: [operator]               # whose comments are the operator's (R4)
 
-# repositories: [owner/repo]          # only these; unset, every one it can read
+# repositories: ["dev/*"]             # patterns: owner/name, owner/*, */*;
+#                                     # public repositories included. Unset:
+#                                     # every one its account was given.
 
 # vault:                              # L10; memory/nisse-review rw for its memory
 #   root: ~/vault

nisse-review/src/nisse_review/access.py

@@ -0,0 +1,79 @@
+"""What nisse-review reviews, and the wall around it (R1, R3).
+
+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
+managed in Gitea for private repositories and in the configuration for
+public ones.
+
+The wall is checked as often: an account that can push to, or administer,
+any repository it reviews is refused, at startup and in every round, so a
+grant made while it runs stops it.
+"""
+
+from collections.abc import Iterable
+from fnmatch import fnmatchcase
+
+from nisse_core.gitea import GiteaClient, GiteaNotFound
+from nisse_core.oplog import OpLog
+
+_GLOB = frozenset("*?[")
+
+
+class AccessRefused(RuntimeError):
+    pass
+
+
+def _glob(text: str) -> bool:
+    return any(c in _GLOB for c in text)
+
+
+def reviewed(
+    gitea: GiteaClient, patterns: Iterable[str] | None, oplog: OpLog
+) -> list[str]:
+    """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)}
+    return sorted(found)
+
+
+def check(gitea: GiteaClient, repos: list[str], oplog: OpLog) -> list[str]:
+    """The repositories it can read, after the wall: refused outright if it
+    could write to any of them. A configured one it can't read is left out."""
+    readable: list[str] = []
+    too_much: list[str] = []
+    for repo in repos:
+        try:
+            permissions = gitea.repository(repo).get("permissions") or {}
+        except GiteaNotFound:
+            oplog.emit(
+                "access.unreadable", cause="not found or not readable", repo=repo
+            )
+            continue
+        if permissions.get("push") or permissions.get("admin"):
+            too_much.append(repo)
+        elif permissions.get("pull", True):
+            readable.append(repo)
+    if too_much:
+        raise AccessRefused(
+            f"{gitea.whoami()} can write to {', '.join(too_much)}: nisse-review's "
+            "account must have read access only (Part R, R3)"
+        )
+    return readable

nisse-review/src/nisse_review/build.py

@@ -15,6 +15,7 @@ from nisse_core.models.context import BudgetedClient
 from nisse_core.models.llm import LiteLLMClient
 from nisse_core.oplog import OpLog
 from nisse_core.state.cancel import Cancel
+from nisse_review.access import AccessRefused, check, reviewed
 from nisse_review.config import ReviewConfig
 from nisse_review.review import Reviewer
 from nisse_review.scheduler import Scheduler
@@ -41,23 +42,18 @@ class Built:
         self.store.close()
 
 
-def check_access(gitea: GiteaClient) -> tuple[str, list[str]]:
+def check_access(
-    """Who it is and what it reads (R3): read only. An account that can push
+    gitea: GiteaClient, patterns: frozenset[str] | None, oplog: OpLog
-    to, or administer, a repository it reviews is refused: its hard wall is
+) -> tuple[str, list[str]]:
-    that it can't change what it reviews."""
+    """Who it is and what it reviews (R1, R3): read only. An account that
+    can push to, or administer, a repository it reviews is refused: its hard
+    wall is that it can't change what it reviews. The scheduler checks again
+    every round (``access``)."""
     account = gitea.whoami()
-    repos = gitea.readable_repositories()
+    try:
-    too_much = [
+        repos = check(gitea, reviewed(gitea, patterns, oplog), oplog)
-        repo
+    except AccessRefused as exc:
-        for repo in repos
+        raise StartupError(str(exc)) from exc
-        if (perms := gitea.repository(repo).get("permissions") or {}).get("push")
-        or perms.get("admin")
-    ]
-    if too_much:
-        raise StartupError(
-            f"{account} can write to {', '.join(too_much)}: nisse-review's "
-            "account must have read access only (Part R, R3)"
-        )
     return account, repos
 
 
@@ -100,7 +96,7 @@ def build(
 ) -> Built:
     oplog = oplog or OpLog(AGENT)
     gitea = gitea or GiteaClient(config.gitea_url, config.gitea_token)
-    account, repos = check_access(gitea)
+    account, repos = check_access(gitea, config.repositories, oplog)
     client = client if client is not None else llm_client(config)
     config.state_db.parent.mkdir(parents=True, exist_ok=True)
     store = StateStore(config.state_db, oplog=oplog)
@@ -146,6 +142,6 @@ def build(
         scratch=Path(scratch),
         poll_s=tuning.review.poll_s,
         attempts=tuning.review.attempts,
-        only=config.repositories,
+        patterns=config.repositories,
     )
     return Built(scheduler, account, repos, store, gitea)

nisse-review/src/nisse_review/config.py

@@ -43,8 +43,8 @@ REQUIRED_KEYS = (
 OPTIONAL_KEYS = (
     "llm.max_output_tokens",
     "llm.request_timeout_s",
-    # Only these of the repositories it can read; unset, every one (R1).
+    # Patterns of the repositories it reviews ("dev/nisse", "dev/*", "*/*"),
-    # For tests and a first bring-up.
+    # public ones included; unset, every one it was given (R1, ``access``).
     "repositories",
 )
 
@@ -132,6 +132,11 @@ class ReviewConfig:
         if missing:
             errors.append("unset or empty — " + ", ".join(missing))
         max_output, timeout = llm_numbers(values, errors)
+        patterns = people(values["repositories"]) if "repositories" in values else None
+        for pattern in sorted(patterns or ()):
+            owner, slash, name = pattern.partition("/")
+            if not (owner and slash and name) or "/" in name:
+                errors.append(f"repositories: {pattern!r} is not owner/name")
         if errors:
             raise ConfigError("cannot start: " + "; ".join(errors))
         reviewer = str(values["roles.reviewer"])
@@ -149,7 +154,5 @@ class ReviewConfig:
             request_timeout_s=timeout,
             tuning=tuning,
             vault=vault,
-            repositories=(
+            repositories=patterns,
-                people(values["repositories"]) if "repositories" in values else None
-            ),
         )

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_review.access import AccessRefused
 from nisse_review.build import StartupError, build
 from nisse_review.config import ReviewConfig
 
@@ -35,6 +36,10 @@ def main() -> int:
     )
     try:
         built.scheduler.run_forever()
+    except AccessRefused as exc:
+        # R3: a grant made while it ran. It stops rather than review on.
+        print(f"nisse-review: stopping: {exc}", file=sys.stderr)
+        return 3
     finally:
         built.close()
     return 0

nisse-review/src/nisse_review/scheduler.py

@@ -1,7 +1,7 @@
 """nisse-review's scheduler (R1): its one duty, and its items one at a time.
 
-Every round, it asks Gitea for the open PRs in every repository its account
+Every round, it works out the repositories it reviews and checks it still
-can read. A PR that is ready (not a draft, not marked work in progress) gets
+can't write to any (``access``), then asks Gitea for their open PRs. A PR that is ready (not a draft, not marked work in progress) gets
 a work item; the oldest ready item is reviewed, and its review posted. An
 item closes when its PR merges or closes. Later reviews, replies and rounds
 come with M7.5.
@@ -19,6 +19,7 @@ from pathlib import Path
 from nisse_core.gitea import GiteaClient, GiteaError, PullRequest
 from nisse_core.oplog import OpLog
 from nisse_core.state.store import WorkItem
+from nisse_review.access import check, reviewed
 from nisse_review.reader import Repository
 from nisse_review.review import Reviewer
 from nisse_review.state import (
@@ -54,7 +55,7 @@ class Scheduler:
         scratch: Path,
         poll_s: float = 60.0,
         attempts: int = 2,
-        only: frozenset[str] | None = None,
+        patterns: frozenset[str] | None = None,
     ) -> None:
         self._store = store
         self._gitea = gitea
@@ -63,7 +64,8 @@ class Scheduler:
         self._scratch = scratch
         self._poll_s = poll_s
         self._attempts = attempts
-        self._only = only  # the repositories reviewed; None: every readable one
+        # The repositories reviewed, as patterns; None: every one it was given.
+        self._patterns = patterns
         self._stop = threading.Event()
 
     # -- the loop ---------------------------------------------------------------
@@ -77,10 +79,17 @@ class Scheduler:
         self._stop.set()
 
     def round(self) -> bool:
-        """Intake, then one review if one is ready; True if one ran."""
+        """The wall, intake, then one review if one is ready; True if one
+        ran. An account that can write to a repository it reviews raises
+        ``AccessRefused``: it stops, never reviews on (R3)."""
         try:
+            repos = check(
+                self._gitea,
+                reviewed(self._gitea, self._patterns, self._oplog),
+                self._oplog,
+            )
             self.recover()
-            self.intake()
+            self.intake(repos)
         except GiteaError as exc:
             self._oplog.emit("intake.failed", cause=str(exc)[:300])
             return False
@@ -92,11 +101,11 @@ class Scheduler:
 
     # -- intake (R1) ------------------------------------------------------------
 
-    def intake(self) -> None:
+    def intake(self, repos: list[str] | None = None) -> None:
+        if repos is None:
+            repos = reviewed(self._gitea, self._patterns, self._oplog)
         open_items = {i.id: i for i in self._store.work_items("open")}
-        for repo in self._gitea.readable_repositories():
+        for repo in repos:
-            if self._only is not None and repo not in self._only:
-                continue
             seen: set[str] = set()
             for pr in self._gitea.open_pull_requests(repo):
                 key = item_id(repo, pr.number)

nisse-review/tests/review_fakes.py

@@ -8,6 +8,7 @@ from typing import Any
 from nisse_core.gitea import (
     Comment,
     GiteaError,
+    GiteaNotFound,
     Issue,
     LineComment,
     PullRequest,
@@ -69,6 +70,9 @@ 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.
+    public: set[str] = field(default_factory=set)
 
     def _up(self) -> None:
         if self.down:
@@ -78,10 +82,23 @@ class FakeGitea:
         return self.account
 
     def readable_repositories(self) -> list[str]:
+        self._up()
+        return [r for r in self.repos if r not in self.public]
+
+    def visible_repositories(self) -> list[str]:
         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}")
         return {
             "permissions": self.permissions.get(
                 repo, {"pull": True, "push": False, "admin": False}

nisse-review/tests/test_review.py

@@ -15,6 +15,7 @@ from nisse_core.knowledge.vault import Vault
 from nisse_core.log.canonical import AssistantText
 from nisse_core.loop.driver import Driver
 from nisse_core.oplog import OpLog
+from nisse_review.access import AccessRefused, check, reviewed
 from nisse_review.build import StartupError, check_access
 from nisse_review.review import Reviewer, _diff_lines, marker
 from nisse_review.scheduler import Scheduler, ready
@@ -102,7 +103,7 @@ def test_intake_opens_an_item_per_pr_and_closes_it_with_the_pr(
     assert (item(w, 7).position, item(w, 8).position) == (READY, WAITING)
     assert item(w, 7).scope == REPO
     gitea.repos["o/elsewhere"] = [pr(9)]
-    w["scheduler"]._only = frozenset({REPO})
+    w["scheduler"]._patterns = frozenset({REPO})
     w["scheduler"].intake()
     assert w["store"].work_item("o/elsewhere#9") is None
     gitea.repos[REPO] = [pr(8, draft=False)]  # 7 merged; 8 is ready now
@@ -300,10 +301,69 @@ def test_with_gitea_down_a_crashed_review_waits(tmp_path: Path) -> None:
 
 def test_an_account_that_can_write_is_refused(tmp_path: Path) -> None:
     gitea = FakeGitea(repos={REPO: [], "o/other": []})
-    assert check_access(gitea) == ("nisse-review", [REPO, "o/other"])  # type: ignore[arg-type]
+    log = OpLog("t", io.StringIO())
+    assert check_access(gitea, None, log) == ("nisse-review", [REPO, "o/other"])  # type: ignore[arg-type]
     gitea.permissions["o/other"] = {"pull": True, "push": True}
     with pytest.raises(StartupError, match="read access only"):
-        check_access(gitea)  # type: ignore[arg-type]
+        check_access(gitea, None, log)  # type: ignore[arg-type]
+
+
+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).
+    gitea = FakeGitea(
+        repos={"dev/nisse": [pr(7)], "dev/wordle": [], "infra/tofu": [pr(3)]},
+        public={"dev/nisse", "dev/wordle", "infra/tofu"},
+    )
+    log = OpLog("t", io.StringIO())
+    assert reviewed(gitea, None, log) == []  # type: ignore[arg-type]
+    assert reviewed(gitea, {"dev/*"}, log) == ["dev/nisse", "dev/wordle"]  # type: ignore[arg-type]
+    assert reviewed(gitea, {"*/*"}, log) == ["dev/nisse", "dev/wordle", "infra/tofu"]  # type: ignore[arg-type]
+    assert reviewed(gitea, {"dev/n*", "infra/tofu"}, log) == ["dev/nisse", "infra/tofu"]  # type: ignore[arg-type]
+    w = world(tmp_path, Scripted(), gitea, patterns=frozenset({"dev/*"}))
+    w["scheduler"].round()
+    assert w["store"].work_item("dev/nisse#7") is not None
+    assert w["store"].work_item("infra/tofu#3") is None
+
+
+def test_a_configured_repository_it_cannot_read_is_left_out() -> None:
+    gitea = FakeGitea(repos={REPO: []})
+    out = io.StringIO()
+    log = OpLog("t", out)
+    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()
+
+
+def test_write_access_granted_while_running_stops_it(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    w = world(tmp_path, Scripted(), gitea)
+    w["scheduler"].intake()
+    gitea.permissions[REPO] = {"pull": True, "push": True}
+    with pytest.raises(AccessRefused, match="read access only"):
+        w["scheduler"].round()
+    assert item(w).position == READY  # nothing reviewed
+
+
+def test_a_repository_pattern_must_be_owner_slash_name(tmp_path: Path) -> None:
+    from nisse_core.config import ConfigError
+    from nisse_review.config import ReviewConfig
+
+    tree = {
+        "state_db": str(tmp_path / "s.db"),
+        "llm": {"base_url": "http://llm", "token": "k"},
+        "roles": {"reviewer": "local", "expert": "frontier"},
+        "gitea": {"url": "https://g", "token": "t", "operators": ["don"]},
+    }
+    ok = ReviewConfig.from_tree({**tree, "repositories": ["dev/*", "*/*"]}, {})
+    assert ok.repositories == {"dev/*", "*/*"}
+    for bad in ("dev", "dev/", "/nisse", "a/b/c"):
+        with pytest.raises(ConfigError, match="is not owner/name"):
+            ReviewConfig.from_tree({**tree, "repositories": [bad]}, {})
 
 
 def test_diff_lines_are_the_heads_lines_a_diff_shows() -> None: