diff --git a/autodev/activities.py b/autodev/activities.py index 2a281c3..f782204 100644 --- a/autodev/activities.py +++ b/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) diff --git a/autodev/cli.py b/autodev/cli.py index 990e6b9..6b0fa29 100644 --- a/autodev/cli.py +++ b/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: diff --git a/autodev/config.py b/autodev/config.py index 420bb49..dfca960 100644 --- a/autodev/config.py +++ b/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: diff --git a/autodev/github.py b/autodev/github.py index 8168bf5..0e3afd1 100644 --- a/autodev/github.py +++ b/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") diff --git a/autodev/review.py b/autodev/review.py index c26cd1e..daeb2a3 100644 --- a/autodev/review.py +++ b/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 ` comment. `/approve` comments count only if posted after `since`: a short prefix of an older head must not approve @@ -67,33 +68,21 @@ 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. @@ -101,9 +90,7 @@ def feedback( 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 ` handled -->` 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 diff --git a/mia_data/cli.py b/mia_data/cli.py index ffda616..e9d0b62 100644 --- a/mia_data/cli.py +++ b/mia_data/cli.py @@ -24,7 +24,7 @@ import yaml from autodev.auth import bot_login, token_fn -from autodev.cli import _client, _setup_logging +from autodev.cli import _client, _setup_logging, run_checks from autodev.github import GitHub from .config import EXTRA_LABELS, HUMAN_LABELS, STAGES, load_config @@ -139,14 +139,6 @@ async def cmd_check(cfg: Config, args: argparse.Namespace) -> int: from .jobspec import harness_env, harness_pin - rows: list[tuple[str, bool, str]] = [] - - async def row(name: str, check: Callable[[], Awaitable[Any]]) -> None: - 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}")) - def repo_check(repo: str) -> Callable[[], Awaitable[str]]: async def check(): gh = _gh(cfg, repo) @@ -187,15 +179,12 @@ async def pin(): ) return f"{commit[:12]} env {digest[:12]}" - for kind in ("data", "snippets", "catalog"): - await row(f"github {kind}", repo_check(getattr(cfg.github, f"{kind}_repo"))) - await row("temporal", temporal) - await row("csub broker", csub) - await row("directories", dirs) - await row("harness pin", pin) - 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 + repos = [ + (f"github {kind}", repo_check(getattr(cfg.github, f"{kind}_repo"))) for kind in ("data", "snippets", "catalog") + ] + return await run_checks( + [*repos, ("temporal", temporal), ("csub broker", csub), ("directories", dirs), ("harness pin", pin)] + ) async def cmd_crops_by_snippet(cfg: Config, args: argparse.Namespace) -> int: diff --git a/tests/test_autodev_review.py b/tests/test_autodev_review.py index 96cbd5e..e6359d4 100644 --- a/tests/test_autodev_review.py +++ b/tests/test_autodev_review.py @@ -5,11 +5,12 @@ from autodev.github import GitHub from autodev.models import Feedback -from autodev.review import approved, combine_checks, feedback, handled_ids, latest_round_at +from autodev.review import Reviewers, approved, combine_checks, feedback, handled_ids, latest_round_at MP = "", at="2026-10-10T04:00:00Z"), cm(4, "bot", f"{MP} started k-3 -->", at="2026-10-10T05:00:00Z"), ] - assert latest_round_at(comments, MP, "bot") == "2026-10-10T03:00:00Z" - assert latest_round_at([], MP, "bot") is None + assert latest_round_at(comments, BOT) == "2026-10-10T03:00:00Z" + assert latest_round_at([], BOT) is None def test_feedback_from_reviews_and_revise_comments_oldest_first(): @@ -83,7 +84,7 @@ def test_feedback_from_reviews_and_revise_comments_oldest_first(): cm(11, "alice", "looks good"), cm(12, "bot", f"/revise {MP} x -->"), ] - fb = feedback(reviews, inline, comments, W, "bot", False, MP) + fb = feedback(reviews, inline, comments, BOT) assert fb == [Feedback("comment-10", "also update docs"), Feedback("review-3", "rename foo\n- `a.py` line 4: here")] @@ -93,7 +94,7 @@ def test_handled_ids_come_from_our_markers(): cm(2, "alice", ""), # a person pasting our marker does not count cm(3, "bot", "see and handled review-7 -->"), ] # not anchored to the prefix - assert handled_ids(comments, MP, "bot") == {"review-3"} + assert handled_ids(comments, BOT) == {"review-3"} @pytest.mark.parametrize(