autodev: reviewer policy object, config checks split, shared check runner

autodev/activities.py

@@ -44,7 +44,7 @@
     Workspace,
     check_key,
 )
-from .review import WRITE_PERMISSIONS, approved, feedback, handled_ids, latest_round_at
+from .review import WRITE_PERMISSIONS, Reviewers, approved, feedback, handled_ids, latest_round_at
 from .workflows import IssueWorkflow
 
 if TYPE_CHECKING:
@@ -318,26 +318,11 @@ async def _snapshot(self, number: int) -> PrSnapshot:
         logins = {login for item in [*reviews, *comments] if (login := (item.get("user") or {}).get("login"))}
         # ponytail: one permission lookup per participant per poll; cache if the rate limit ever matters.
         permissions = {login: await self.gh.permission(login) for login in logins}
-        marker_prefix = self.cfg.marker_prefix
-        allow_self = (
-            self.cfg.github.auth == "token"
-        )  # under a personal token the reviewer and the bot are the same user
-        handled = handled_ids(comments, marker_prefix, self.bot)
-        pending = [
-            item
-            for item in feedback(reviews, inline_comments, comments, permissions, self.bot, allow_self, marker_prefix)
-            if item.id not in handled
-        ]
-        is_approved = approved(
-            head_sha,
-            reviews,
-            comments,
-            permissions,
-            self.bot,
-            allow_self,
-            marker_prefix,
-            latest_round_at(comments, marker_prefix, self.bot),
-        )
+        # under a personal token the reviewer and the bot are the same user
+        reviewers = Reviewers(permissions, self.bot, self.cfg.github.auth == "token", self.cfg.marker_prefix)
+        handled = handled_ids(comments, reviewers)
+        pending = [item for item in feedback(reviews, inline_comments, comments, reviewers) if item.id not in handled]
+        is_approved = approved(head_sha, reviews, comments, reviewers, latest_round_at(comments, reviewers))
         checks = await self.gh.check_state(head_sha, self.cfg.github.required_checks)
         return PrSnapshot(number, "open", head_sha, is_approved, checks, pending)
 

autodev/cli.py

@@ -11,7 +11,7 @@
 import os
 import signal
 import sys
-from collections.abc import Awaitable, Callable, Mapping
+from collections.abc import Awaitable, Callable, Mapping, Sequence
 from datetime import timedelta
 from pathlib import Path
 from typing import TYPE_CHECKING, Any
@@ -79,26 +79,32 @@ def _csub_problems(cfg: Config, probe: Any) -> list[str]:
     ]
 
 
-async def cmd_check(cfg: Config, args: argparse.Namespace) -> int:
-    """Try every external dependency and print one ok/BAD line per check."""
-    from .agent import csub_client
-    from .auth import bot_login
-    from .github import GitHub
-
+async def run_checks(checks: Sequence[tuple[str, Callable[[], Awaitable[Any]]]]) -> int:
+    """Run every named check (none is skipped because an earlier one failed), print one ok/BAD line each
+    and return 1 if any failed. Shared with mia-data's `check`."""
     rows: list[tuple[str, bool, str]] = []
-
-    async def row(name: str, check: Callable[[], Awaitable[Any]]) -> None:
+    for name, check in checks:
         try:
             rows.append((name, True, str(await check())))
         except Exception as error:  # report every check, then fail
             rows.append((name, False, f"{type(error).__name__}: {error}"))
+    for name, ok, detail in rows:
+        print(f"{'ok ' if ok else 'BAD'}  {name:16} {detail}")
+    return 0 if all(ok for _, ok, _ in rows) else 1
+
 
-    gh = None
+async def cmd_check(cfg: Config, args: argparse.Namespace) -> int:
+    """Try every external dependency and print one ok/BAD line per check."""
+    from .agent import csub_client
+    from .auth import bot_login
+    from .github import GitHub
 
     async def repo():
-        nonlocal gh
         gh = GitHub.for_autodev(cfg)
-        info = (await gh.request("GET", f"/repos/{cfg.github.repo}")).json()
+        try:
+            info = (await gh.request("GET", f"/repos/{cfg.github.repo}")).json()
+        finally:
+            await gh.aclose()
         return f"{info['full_name']} (default branch {info['default_branch']})"
 
     async def csub_ok():
@@ -119,16 +125,15 @@ async def dirs():
             (directory / ".write-test").unlink()
         return f"{cfg.paths.state_dir}, {cfg.paths.work_root}"
 
-    await row("github identity", lambda: bot_login(cfg))
-    await row("github repo", repo)
-    await row("temporal", temporal)
-    await row("csub broker", csub_ok)
-    await row("directories", dirs)
-    if gh:
-        await gh.aclose()
-    for name, ok, detail in rows:
-        print(f"{'ok ' if ok else 'BAD'}  {name:16} {detail}")
-    return 0 if all(ok for _, ok, _ in rows) else 1
+    return await run_checks(
+        [
+            ("github identity", lambda: bot_login(cfg)),
+            ("github repo", repo),
+            ("temporal", temporal),
+            ("csub broker", csub_ok),
+            ("directories", dirs),
+        ]
+    )
 
 
 async def cmd_init_labels(cfg: Config, args: argparse.Namespace) -> int:

autodev/config.py

@@ -135,25 +135,39 @@ def _finish(self) -> Config:
         """Cross-field checks and path normalization that pydantic's per-field validation cannot do."""
         if not re.fullmatch(r"[a-z][a-z0-9-]{1,30}", self.deployment):
             raise ValueError(f"deployment {self.deployment!r}: use 2-31 chars of a-z, 0-9, '-'")
+        self._check_github()
+        self._resolve_paths()
+        self._check_agent()
+        self._check_mounts()
+        return self
+
+    def _check_github(self) -> None:
         github = self.github
         if github.auth == "app" and not (github.app_id and github.installation_id and github.private_key_path):
             raise ValueError("github.auth=app needs github.app_id, installation_id and private_key_path")
         if bad := [login for login in github.trusted_bots if not login.endswith("[bot]")]:
             raise ValueError(f"github.trusted_bots: {bad} must be app logins ending in '[bot]'")
-        if self.agent.max_budget_usd is not None and self.agent.max_budget_usd <= 0:
-            raise ValueError("agent.max_budget_usd must be positive")
-        # Paths are made canonical because csub rejects non-canonical mounts, and round dirs (under work_root)
-        # are mounted into jobs.
-        self.paths.state_dir = self.paths.state_dir.expanduser().resolve()
-        self.paths.work_root = self.paths.work_root.expanduser().resolve()
         for attr in ("private_key_path", "token_file"):
             if getattr(github, attr):
                 setattr(github, attr, getattr(github, attr).expanduser())
+
+    def _resolve_paths(self) -> None:
+        """Paths are made canonical because csub rejects non-canonical mounts, and round dirs (under work_root)
+        are mounted into jobs."""
+        self.paths.state_dir = self.paths.state_dir.expanduser().resolve()
+        self.paths.work_root = self.paths.work_root.expanduser().resolve()
+
+    def _check_agent(self) -> None:
+        if self.agent.max_budget_usd is not None and self.agent.max_budget_usd <= 0:
+            raise ValueError("agent.max_budget_usd must be positive")
         for host in self.agent.allow_hosts:
             if is_github_host(host):
                 raise ValueError(f"agent.allow_hosts: {host.strip()!r} would let the agent reach GitHub")
-        # An extra mount must not expose anything that holds credentials or trusted state.
-        secrets = [self.paths.state_dir, github.token_file, github.private_key_path]
+
+    def _check_mounts(self) -> None:
+        """Extra mounts are read-only, absolute, and must not expose anything that holds credentials or trusted
+        state."""
+        secrets = [self.paths.state_dir, self.github.token_file, self.github.private_key_path]
         for index, mount in enumerate(self.agent.extra_mounts):
             if not mount.endswith(":ro"):
                 raise ValueError(f"agent.extra_mounts: {mount!r} must end with ':ro'")
@@ -166,7 +180,6 @@ def _finish(self) -> Config:
                 secret = Path(secret).resolve()
                 if mount_path == secret or mount_path in secret.parents or secret in mount_path.parents:
                     raise ValueError(f"agent.extra_mounts: {mount!r} overlaps {secret}, which the agent must not see")
-        return self
 
     @property
     def task_queue(self) -> str:

autodev/github.py

@@ -96,8 +96,11 @@ async def _throttle(self, create: bool) -> None:
         self._last_write = time.monotonic()
 
     @staticmethod
-    def _rate_limit_delay(response: httpx.Response, attempt: int) -> float | None:
-        """Seconds to wait if this response is a (primary or secondary) rate limit, else None."""
+    def _retry_delay(response: httpx.Response, attempt: int) -> float | None:
+        """Seconds to wait before retrying this response, or None when it is final. A 5xx backs off
+        exponentially; a (primary or secondary) rate limit waits as long as GitHub says."""
+        if response.status_code >= 500:
+            return min(2**attempt, 60)
         if response.status_code not in (403, 429):
             return None
         if "retry-after" in response.headers:
@@ -108,6 +111,12 @@ def _rate_limit_delay(response: httpx.Response, attempt: int) -> float | None:
             return min(60.0 * 2**attempt, 900.0)  # GitHub: wait ≥1 min, then back off exponentially
         return None
 
+    def _remember_etag(self, cache_key: tuple, response: httpx.Response) -> None:
+        if response.headers.get("etag"):
+            self._etags[cache_key] = (response.headers["etag"], response)
+        else:
+            self._etags.pop(cache_key, None)
+
     @overload
     async def request(
         self,
@@ -172,14 +181,10 @@ async def request(
                 log.warning("github %s %s transport error %s (attempt %d)", method, path, error, attempt + 1)
                 await self._sleep(min(2**attempt, 60))
                 continue
-            delay = self._rate_limit_delay(response, attempt)
-            if delay is not None:
-                log.warning("github rate limited on %s %s; sleeping %.0fs", method, path, delay)
+            if (delay := self._retry_delay(response, attempt)) is not None:
+                log.warning("github %s %s -> %d; retrying in %.0fs", method, path, response.status_code, delay)
                 await self._sleep(delay)
                 continue
-            if response.status_code >= 500:
-                await self._sleep(min(2**attempt, 60))
-                continue
             if response.status_code == 304 and cache_key in self._etags:
                 return self._etags[cache_key][1]
             if response.status_code == 404 and ok404:
@@ -191,10 +196,7 @@ async def request(
             if write:
                 log.info("github %s %s -> %d", method, path, response.status_code)
             if cache_key is not None:
-                if response.headers.get("etag"):
-                    self._etags[cache_key] = (response.headers["etag"], response)
-                else:
-                    self._etags.pop(cache_key, None)
+                self._remember_etag(cache_key, response)
             return response
         raise GitHubError(f"{method} {path}: gave up after {attempts} attempts")
 

autodev/review.py

@@ -2,14 +2,15 @@
 how CI looks.
 
 These are pure functions over the dicts GitHub returns, so the rules (who counts as a reviewer, which
-approval counts for which commit) are in one place and easy to test. `marker_prefix` identifies the bot's
-own comments; `bot` is its login.
+approval counts for which commit) are in one place and easy to test. A `Reviewers` says who counts: the
+participants' permissions, the bot's login and the marker prefix that identifies the bot's own comments.
 """
 
 from __future__ import annotations
 
 import re
 from collections.abc import Sequence
+from dataclasses import dataclass
 
 from .models import Feedback
 
@@ -24,40 +25,40 @@ def _login(item: dict) -> str | None:
     return (item.get("user") or {}).get("login")
 
 
-def _is_human_writer(login: str | None, permissions: dict[str, str], bot: str, allow_self: bool) -> bool:
-    """A person with write access, and not the bot itself unless `allow_self` (under a personal token the
-    reviewer and the bot are the same account)."""
-    if not login:
-        return False
-    return permissions.get(login) in WRITE_PERMISSIONS and (allow_self or login != bot)
+@dataclass(frozen=True)
+class Reviewers:
+    """Who counts on a PR: `permissions` maps each participant's login to their repo permission, `bot` is our
+    login and `marker_prefix` starts every comment we posted. `allow_self` lets the bot's own account review
+    (under a personal token the reviewer and the bot are the same user)."""
 
+    permissions: dict[str, str]
+    bot: str
+    allow_self: bool
+    marker_prefix: str
 
-def _is_ours(body: str | None, marker_prefix: str) -> bool:
-    return marker_prefix in (body or "")
+    def is_human_writer(self, login: str | None) -> bool:
+        """A person with write access, and not the bot itself unless `allow_self`."""
+        if not login:
+            return False
+        return self.permissions.get(login) in WRITE_PERMISSIONS and (self.allow_self or login != self.bot)
 
+    def is_ours(self, body: str | None) -> bool:
+        return self.marker_prefix in (body or "")
 
-def latest_round_at(comments: list[dict], marker_prefix: str, bot: str) -> str | None:
+
+def latest_round_at(comments: list[dict], reviewers: Reviewers) -> str | None:
     """When the current head was published: the time of the bot's latest round-summary comment."""
     return max(
         (
             comment.get("created_at") or ""
             for comment in comments
-            if _login(comment) == bot and f"{marker_prefix} round " in (comment.get("body") or "")
+            if _login(comment) == reviewers.bot and f"{reviewers.marker_prefix} round " in (comment.get("body") or "")
         ),
         default=None,
     )
 
 
-def approved(
-    head_sha: str,
-    reviews: list[dict],
-    comments: list[dict],
-    permissions: dict[str, str],
-    bot: str,
-    allow_self: bool,
-    marker_prefix: str,
-    since: str | None,
-) -> bool:
+def approved(head_sha: str, reviews: list[dict], comments: list[dict], reviewers: Reviewers, since: str | None) -> bool:
     """Whether a human with write access approved exactly `head_sha`, by review or by `/approve <sha>` comment.
 
     `/approve` comments count only if posted after `since`: a short prefix of an older head must not approve
@@ -67,43 +68,29 @@ def approved(
         if review.get("state") in ("APPROVED", "CHANGES_REQUESTED", "DISMISSED") and (login := _login(review)):
             latest[login] = review
     if any(
-        review["state"] == "APPROVED"
-        and review.get("commit_id") == head_sha
-        and _is_human_writer(login, permissions, bot, allow_self)
+        review["state"] == "APPROVED" and review.get("commit_id") == head_sha and reviewers.is_human_writer(login)
         for login, review in latest.items()
     ):
         return True
     for comment in comments:
-        if _is_ours(comment.get("body"), marker_prefix) or (since and (comment.get("created_at") or "") <= since):
+        if reviewers.is_ours(comment.get("body")) or (since and (comment.get("created_at") or "") <= since):
             continue
         match = APPROVE_RE.search(comment.get("body") or "")
-        if (
-            match
-            and head_sha.startswith(match.group(1))
-            and _is_human_writer(_login(comment), permissions, bot, allow_self)
-        ):
+        if match and head_sha.startswith(match.group(1)) and reviewers.is_human_writer(_login(comment)):
             return True
     return False
 
 
 def feedback(
-    reviews: list[dict],
-    review_comments: list[dict],
-    comments: list[dict],
-    permissions: dict[str, str],
-    bot: str,
-    allow_self: bool,
-    marker_prefix: str,
+    reviews: list[dict], review_comments: list[dict], comments: list[dict], reviewers: Reviewers
 ) -> list[Feedback]:
     """Change requests from people with write access, oldest first.
 
     A "changes requested" review becomes one Feedback with its inline comments attached; a `/revise` comment
     becomes one Feedback with the rest of the comment as its text."""
     items: list[tuple[str, Feedback]] = []  # (timestamp, feedback) for sorting
     for review in reviews:
-        if review.get("state") != "CHANGES_REQUESTED" or not _is_human_writer(
-            _login(review), permissions, bot, allow_self
-        ):
+        if review.get("state") != "CHANGES_REQUESTED" or not reviewers.is_human_writer(_login(review)):
             continue
         inline = [
             f"- `{comment.get('path')}` line {comment.get('line') or comment.get('original_line')}: "
@@ -114,10 +101,10 @@ def feedback(
         text = "\n".join(filter(None, [review.get("body") or "", *inline])) or "(changes requested, no details)"
         items.append((review.get("submitted_at") or "", Feedback(f"review-{review['id']}", text)))
     for comment in comments:
-        if _is_ours(comment.get("body"), marker_prefix):
+        if reviewers.is_ours(comment.get("body")):
             continue
         match = REVISE_RE.search(comment.get("body") or "")
-        if match and _is_human_writer(_login(comment), permissions, bot, allow_self):
+        if match and reviewers.is_human_writer(_login(comment)):
             items.append(
                 (
                     comment.get("created_at") or "",
@@ -127,12 +114,12 @@ def feedback(
     return [item for _, item in sorted(items, key=lambda pair: pair[0])]
 
 
-def handled_ids(comments: list[dict], marker_prefix: str, bot: str) -> set[str]:
+def handled_ids(comments: list[dict], reviewers: Reviewers) -> set[str]:
     """Feedback ids that a round already addressed: only the bot's own `<prefix> handled <id> -->` markers count."""
-    handled_re = re.compile(re.escape(marker_prefix) + r" handled (\S+) -->")
+    handled_re = re.compile(re.escape(reviewers.marker_prefix) + r" handled (\S+) -->")
     handled: set[str] = set()
     for comment in comments:
-        if _login(comment) == bot:
+        if _login(comment) == reviewers.bot:
             handled.update(handled_re.findall(comment.get("body") or ""))
     return handled