Merge pull request 'nisse-review: a new head's review waits for its CI' (#155) from review-waits-for-ci into main

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).

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.

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)

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"}

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}

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)

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)

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,

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:

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)

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"

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.