diff --git a/docs/Architecture.md b/docs/Architecture.md index c78b665..6184447 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -888,7 +888,9 @@ else) towards better changes. - **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 - review on it. + review on it. A new head's review waits for its checks to finish (passed or + failed), so it is judged with the CI result and stays off a GPU that CI is + using; up to a tuning limit, and not at all where there is no CI. - **The first review** is given the PR (title, description, its commits and diff, its comments, and the issue it closes); the reviewer reads the rest as it needs it (R3). diff --git a/docs/Tuning-Guide.md b/docs/Tuning-Guide.md index aca2cbc..37165d8 100644 --- a/docs/Tuning-Guide.md +++ b/docs/Tuning-Guide.md @@ -638,3 +638,26 @@ reads file by file. Failed review runs on one PR, at one head, before the reviewer says on the PR that it couldn't review it, and stops trying until the next push. + +### `tuning.review.ci_wait_s` — default 3600 + +How long a new head's review waits while its checks are pending. A review +judges the CI result (R1), and waiting keeps it off a GPU that CI is using +(the `nisse` project's end-to-end tests). Finished checks, passed or failed, +end the wait; after this long it reviews anyway and the CI result reads as +pending. 0 never waits. + +- **Too low:** reviews run beside CI, slower on a shared GPU, without its result. +- **Too high:** a stuck runner delays reviews that long. +- **Deciding:** a little longer than the repository's slowest CI run (the full + end-to-end suite takes 25 minutes or more). + +### `tuning.review.ci_grace_s` — default 120 + +How long a new head with no checks at all waits for some to appear: Gitea +Actions reports a push's checks a moment after it. Past this, a repository +without CI is reviewed at once. 0 reviews such a head at once (the live +review test, whose fresh repository has no CI). + +- **Too low:** a review starts before CI has reported, and runs beside it. +- **Too high:** repositories without CI wait that long for every review. diff --git a/nisse-core/src/nisse_core/config.py b/nisse-core/src/nisse_core/config.py index aaa59eb..d17d93f 100644 --- a/nisse-core/src/nisse_core/config.py +++ b/nisse-core/src/nisse_core/config.py @@ -81,13 +81,14 @@ def read_file(path: Path, errors: list[str]) -> dict[str, Any]: return loaded -def positive(value: Any, kind: type) -> int | float | None: +def positive(value: Any, kind: type, *, zero: bool = False) -> int | float | None: + """``value`` as ``kind`` if above 0 (or 0 itself, given ``zero``).""" # bool is an int in Python; `true` for a turn limit is a mistake. if isinstance(value, bool) or not isinstance(value, int | float): return None if kind is int and not float(value).is_integer(): return None - if value <= 0: + if value < 0 or (value == 0 and not zero): return None return kind(value) @@ -132,9 +133,13 @@ def parse_tuning( if spec is None: errors.append(f"tuning.{name}.{key}: unknown setting") continue - number = positive(value, float if spec.type in (float, "float") else int) + # A field with metadata {"zero": True} may also be 0 ("don't wait"). + zero = bool(spec.metadata.get("zero")) + kind = float if spec.type in (float, "float") else int + number = positive(value, kind, zero=zero) if number is None: - errors.append(f"tuning.{name}.{key}: must be a positive number") + need = "zero or more" if zero else "a positive number" + errors.append(f"tuning.{name}.{key}: must be {need}") continue values[key] = number sections[name] = cls(**values) diff --git a/nisse-core/tests/test_shared_config.py b/nisse-core/tests/test_shared_config.py index 4ad585e..234904d 100644 --- a/nisse-core/tests/test_shared_config.py +++ b/nisse-core/tests/test_shared_config.py @@ -19,6 +19,7 @@ from nisse_core.config import ( class Review: max_rounds: int = 3 wait_s: float = 60.0 + grace_s: float = field(default=5.0, metadata={"zero": True}) @dataclass(frozen=True) @@ -45,6 +46,17 @@ def test_an_agents_own_tuning_sections() -> None: ] +def test_a_field_may_opt_in_to_zero() -> None: + errors: list[str] = [] + tuning = parse_tuning({"review": {"grace_s": 0}}, Tuning, errors) + assert tuning.review.grace_s == 0 and errors == [] + parse_tuning({"review": {"grace_s": -1, "wait_s": 0}}, Tuning, errors) + assert errors == [ + "tuning.review.grace_s: must be zero or more", + "tuning.review.wait_s: must be a positive number", + ] + + def test_references_and_people() -> None: errors: list[str] = [] env = {"TOKEN": "s3cret"} diff --git a/nisse-review.example.yaml b/nisse-review.example.yaml index f51eb93..bb6466d 100644 --- a/nisse-review.example.yaml +++ b/nisse-review.example.yaml @@ -35,6 +35,7 @@ gitea: # notes: ro # tuning: # docs/Tuning-Guide.md -# review: {poll_s: 60, max_turns: 40, diff_chars: 60000, attempts: 2} +# review: {poll_s: 60, max_turns: 40, diff_chars: 60000, attempts: 2, +# ci_wait_s: 3600, ci_grace_s: 120} # expert: {consult_budget: 3} # context: {keep_recent_turns: 6, safety_margin: 0.1} diff --git a/nisse-review/src/nisse_review/build.py b/nisse-review/src/nisse_review/build.py index 485da5e..b8d153c 100644 --- a/nisse-review/src/nisse_review/build.py +++ b/nisse-review/src/nisse_review/build.py @@ -142,6 +142,8 @@ def build( scratch=Path(scratch), poll_s=tuning.review.poll_s, attempts=tuning.review.attempts, + ci_wait_s=tuning.review.ci_wait_s, + ci_grace_s=tuning.review.ci_grace_s, patterns=config.repositories, ) return Built(scheduler, account, repos, store, gitea) diff --git a/nisse-review/src/nisse_review/config.py b/nisse-review/src/nisse_review/config.py index 011f392..0bb0f46 100644 --- a/nisse-review/src/nisse_review/config.py +++ b/nisse-review/src/nisse_review/config.py @@ -55,6 +55,10 @@ class ReviewTuning: max_turns: int = 40 # a review run's turn cap diff_chars: int = 60000 # the diff in the review's context; files on demand attempts: int = 2 # failed runs on one PR before saying so on it + # A new head waits for its CI to finish: at most ci_wait_s while checks + # are pending, and ci_grace_s for checks to appear at all. 0: don't wait. + ci_wait_s: float = field(default=3600.0, metadata={"zero": True}) + ci_grace_s: float = field(default=120.0, metadata={"zero": True}) @dataclass(frozen=True) diff --git a/nisse-review/src/nisse_review/scheduler.py b/nisse-review/src/nisse_review/scheduler.py index d51aee0..dd865f7 100644 --- a/nisse-review/src/nisse_review/scheduler.py +++ b/nisse-review/src/nisse_review/scheduler.py @@ -14,6 +14,8 @@ item waits rather than risk a second review. import re import tempfile import threading +import time +from collections.abc import Callable from pathlib import Path from nisse_core.gitea import GiteaClient, GiteaError, PullRequest @@ -56,6 +58,9 @@ class Scheduler: poll_s: float = 60.0, attempts: int = 2, patterns: frozenset[str] | None = None, + ci_wait_s: float = 3600.0, + ci_grace_s: float = 120.0, + clock: Callable[[], float] = time.time, ) -> None: self._store = store self._gitea = gitea @@ -66,6 +71,10 @@ class Scheduler: self._attempts = attempts # The repositories reviewed, as patterns; None: every one it was given. self._patterns = patterns + self._ci_wait_s = ci_wait_s + self._ci_grace_s = ci_grace_s + self._clock = clock + self._said_waiting: set[str] = set() # "item@head", logged once self._stop = threading.Event() # -- the loop --------------------------------------------------------------- @@ -131,13 +140,21 @@ class Scheduler: title=pr.title, scope=repo, position=position, - data=data(PR(repo, pr.number, head=pr.head_sha)), + data=data(PR(repo, pr.number, head=pr.head_sha, seen_at=self._clock())), cause="ready for review" if position == READY else "not ready", ) return known = pr_of(item) moved = known.head != pr.head_sha - new = PR(known.repo, known.number, pr.head_sha, known.reviewed, known.attempts) + seen_at = self._clock() if moved else known.seen_at + new = PR( + known.repo, + known.number, + pr.head_sha, + known.reviewed, + known.attempts, + seen_at, + ) if item.position in (READY, WAITING): if position != item.position or moved: self._store.update_work_item( @@ -145,7 +162,7 @@ class Scheduler: ) elif item.position == FAILED and moved: # It never got a review: new code is worth another try. - new = PR(known.repo, known.number, pr.head_sha, known.reviewed, 0) + new = PR(known.repo, known.number, pr.head_sha, known.reviewed, 0, seen_at) self._store.update_work_item( item.id, position=position, data=data(new), cause="new push" ) @@ -165,8 +182,41 @@ class Scheduler: ) def _next(self) -> WorkItem | None: + """The oldest ready item whose CI has settled.""" ready_items = [i for i in self._store.work_items("open") if i.position == READY] - return min(ready_items, key=lambda i: i.opened_at, default=None) + for item in sorted(ready_items, key=lambda i: i.opened_at): + if self._ci_settled(item): + return item + return None + + def _ci_settled(self, item: WorkItem) -> bool: + """R1: a review judges the CI result, so a new head waits for its + checks: reviewed once they've finished (passed or not), at once with + no CI at all after a short grace for checks to appear, and anyway + once they've been pending ``ci_wait_s``. Waiting also keeps reviews + off a GPU that CI is using.""" + known = pr_of(item) + waited = self._clock() - known.seen_at + try: + state = self._gitea.checks(known.repo, known.head) + except GiteaError as exc: + self._oplog.emit("review.checks_unknown", cause=str(exc)[:300]) + return False # asked again next round + if state == "pending" and waited < self._ci_wait_s: + key = f"{item.id}@{known.head}" + if key not in self._said_waiting: + self._said_waiting.add(key) + self._oplog.emit( + "review.waiting_for_ci", cause="checks pending", work_item=item.id + ) + return False + return not (state == "none" and waited < self._ci_grace_s) + + def _seen(self, known: PR, head: str) -> float: + """When ``head`` was first seen: kept for the head the item knew, now + for one pushed during the review, so a retry still waits for CI. A + retry at a head already past ``ci_wait_s`` doesn't wait again.""" + return known.seen_at if head == known.head else self._clock() # -- a review --------------------------------------------------------------- @@ -210,7 +260,16 @@ class Scheduler: self._store.update_work_item( item.id, position=REVIEWED, - data=data(PR(known.repo, known.number, pr.head_sha, pr.head_sha, 0)), + data=data( + PR( + known.repo, + known.number, + pr.head_sha, + pr.head_sha, + 0, + self._seen(known, pr.head_sha), + ) + ), cause="review posted", ) @@ -219,7 +278,14 @@ class Scheduler: ) -> None: attempts = known.attempts + 1 head = pr.head_sha if pr else known.head - new = PR(known.repo, known.number, head, known.reviewed, attempts) + new = PR( + known.repo, + known.number, + head, + known.reviewed, + attempts, + self._seen(known, head), + ) self._oplog.emit("review.failed", cause=why[:300], work_item=item.id) if attempts < self._attempts: self._store.update_work_item( @@ -269,6 +335,7 @@ class Scheduler: head, head if posted else known.reviewed, known.attempts, + self._seen(known, head), ) self._store.update_work_item( item.id, diff --git a/nisse-review/src/nisse_review/state.py b/nisse-review/src/nisse_review/state.py index e3e10ee..19487d2 100644 --- a/nisse-review/src/nisse_review/state.py +++ b/nisse-review/src/nisse_review/state.py @@ -33,6 +33,7 @@ class PR: head: str = "" # the commit last seen at the PR's head reviewed: str = "" # the commit its review was posted on attempts: int = 0 # failed runs at this head + seen_at: float = 0.0 # when this head was first seen (the CI wait) def item_id(repo: str, number: int) -> str: diff --git a/nisse-review/tests/review_fakes.py b/nisse-review/tests/review_fakes.py index 5014320..9df8de3 100644 --- a/nisse-review/tests/review_fakes.py +++ b/nisse-review/tests/review_fakes.py @@ -73,6 +73,7 @@ class FakeGitea: # 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 + ci: dict[str, str] = field(default_factory=dict) # head -> checks; success def _up(self) -> None: if self.down: @@ -124,7 +125,7 @@ class FakeGitea: return [] def checks(self, repo: str, sha: str) -> str: - return "success" + return self.ci.get(sha, "success") def archive(self, repo: str, ref: str) -> bytes: return archive(self.files) diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index b5fa65e..5b85c95 100644 --- a/nisse-review/tests/test_review.py +++ b/nisse-review/tests/test_review.py @@ -342,6 +342,99 @@ def test_a_repository_whose_prs_cannot_be_listed_holds_up_no_other( assert '"intake.repo_failed"' in out.getvalue() and "a/empty" in out.getvalue() +class Clock: + def __init__(self) -> None: + self.now = 1000.0 + + def __call__(self) -> float: + return self.now + + +def test_a_new_head_waits_for_its_ci_to_finish(tmp_path: Path) -> None: + # Follow-up to #154: judged with CI's result, and off CI's GPU. + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + head = pr().head_sha + gitea.ci[head] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock) + assert w["scheduler"].round() is False # pending: not yet + clock.now += 600 + assert w["scheduler"].round() is False + gitea.ci[head] = "failure" # finished: failing CI is worth a review too + assert w["scheduler"].round() is True + assert item(w).position == REVIEWED + + +def test_ci_pending_too_long_is_reviewed_anyway(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + gitea.ci[pr().head_sha] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock, ci_wait_s=3600) + assert w["scheduler"].round() is False + clock.now += 3600 + assert w["scheduler"].round() is True # a stuck runner blocks nothing + + +def test_no_ci_waits_only_a_grace_for_checks_to_appear(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + gitea.ci[pr().head_sha] = "none" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock, ci_grace_s=120) + assert w["scheduler"].round() is False + clock.now += 120 + assert w["scheduler"].round() is True + + +def test_a_pr_waiting_on_ci_holds_up_no_other(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr(7), pr(8, head_sha="c8")]}) + gitea.ci[pr(7).head_sha] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock) + assert w["scheduler"].round() is True # 8, though 7 is older + assert (item(w, 7).position, item(w, 8).position) == (READY, REVIEWED) + + +def test_a_failed_review_keeps_its_heads_wait(tmp_path: Path) -> None: + # nisse-review on #155: a retry lost seen_at and skipped the wait. + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + prose = [AssistantText(text="I think it's fine.", model="m")] + w = world(tmp_path, Scripted(prose, prose), gitea, clock=clock, attempts=2) + w["scheduler"].round() + assert (item(w).position, pr_of(item(w)).seen_at) == (READY, 1000.0) + + +def test_a_head_pushed_during_a_failed_review_waits_for_its_ci( + tmp_path: Path, +) -> None: + clock = Clock() + + class Pushed(FakeGitea): + def post_review(self, *args: Any, **kwargs: Any) -> int: + clock.now += 50 + self.repos[REPO] = [pr(head_sha="fff000")] + self.ci["fff000"] = "pending" + raise GiteaError("422: the head moved") + + gitea = Pushed(repos={REPO: [pr()]}) + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock) + w["scheduler"].round() + assert w["scheduler"].round() is False # the new head's CI is pending + + +def test_checks_gitea_cannot_report_wait_a_round(tmp_path: Path) -> None: + class NoChecks(FakeGitea): + def checks(self, repo: str, sha: str) -> str: + raise GiteaError("502: bad gateway") + + out = io.StringIO() + w = world(tmp_path, Scripted(*REVIEW), NoChecks(repos={REPO: [pr()]})) + w["scheduler"]._oplog = OpLog("t", out) + assert w["scheduler"].round() is False + assert item(w).position == READY + assert '"review.checks_unknown"' in out.getvalue() + + def test_a_configured_repository_it_cannot_read_is_left_out() -> None: gitea = FakeGitea(repos={REPO: []}) out = io.StringIO() @@ -379,6 +472,21 @@ def test_a_repository_pattern_must_be_owner_slash_name(tmp_path: Path) -> None: ReviewConfig.from_tree({**tree, "repositories": [bad]}, {}) +def test_the_ci_waits_may_be_zero(tmp_path: Path) -> None: + # The live test's repository has no CI: it reviews with no grace at all. + 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"]}, + "tuning": {"review": {"ci_wait_s": 0, "ci_grace_s": 0}}, + } + review = ReviewConfig.from_tree(tree, {}).tuning.review + assert (review.ci_wait_s, review.ci_grace_s) == (0, 0) + + def test_diff_lines_are_the_heads_lines_a_diff_shows() -> None: diff = ( "--- a/x.py\n+++ b/x.py\n@@ -1,3 +1,4 @@\n a\n-b\n+c\n+d\n e\n" diff --git a/tests/integration/test_review_live.py b/tests/integration/test_review_live.py index 729c6bb..2350d2b 100644 --- a/tests/integration/test_review_live.py +++ b/tests/integration/test_review_live.py @@ -56,7 +56,8 @@ def test_a_ready_pr_gets_one_comment_review( "operators": ["nisse-testing-operator"], }, "repositories": [repo.name], - "tuning": {"review": {"max_turns": 25}}, + # The fresh repository has no CI: review at once, no grace. + "tuning": {"review": {"max_turns": 25, "ci_grace_s": 0}}, } config = ReviewConfig.from_tree(tree, {}) # make_repo gives the agent write access: the reviewer refuses that.