nisse-review: later reviews, replies, rounds, stop and review (M7.5)

Back on a PR it reviewed: a new head gets a later review of the commits
since (each one's diff, through the API) and of its earlier points; what
was said since gets replies, answered where it was said or skipped with a
reason. It signs off once and stays quiet while still happy. Past
tuning.review.max_rounds it sums up for the operator and goes quiet,
until a push or someone other than the PR's author joins in. An
operator's `stop` quiets it; `review` asks for a fresh review.

Reviews carry numbered markers and replies their point's key, so
recovery posts nothing twice.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

nisse-review/tests/review_fakes.py

@@ -74,6 +74,14 @@ class FakeGitea:
     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
+    # What others said: top-level comments and comments on lines (M7.5).
+    thread: list[Comment] = field(
+        default_factory=lambda: [Comment(1, "don", "Please keep it simple.", "t")]
+    )
+    notes: list[ReviewNote] = field(default_factory=list)
+    since: list[str] = field(default_factory=list)  # commits_between's answer
+    reactions: list[tuple[int, str]] = field(default_factory=list)
+    _ids: int = 1000
 
     def _up(self) -> None:
         if self.down:
@@ -118,11 +126,48 @@ class FakeGitea:
     def pull_request_diff(self, repo: str, index: int) -> str:
         return self.diff
 
+    def _id(self) -> int:
+        self._ids += 1
+        return self._ids
+
+    def say(self, author: str, body: str) -> int:
+        """Someone else's top-level comment on the PR."""
+        n = self._id()
+        self.thread.append(Comment(n, author, body, "t"))
+        return n
+
+    def note(self, author: str, body: str, where: str) -> str:
+        """Someone else's comment on a line."""
+        key = f"line-{self._id()}"
+        self.notes.append(ReviewNote(key, author, body, where))
+        return key
+
     def list_comments(self, repo: str, index: int, since: str | None = None):
-        return [Comment(1, "don", "Please keep it simple.", "t")]
+        self._up()
+        return list(self.thread)
 
     def review_notes(self, repo: str, index: int) -> list[ReviewNote]:
-        return []
+        self._up()
+        mine: list[ReviewNote] = []
+        for n, p in enumerate(self.posted, 1):
+            if (p["repo"], p["pr"]) != (repo, index):
+                continue
+            if p["body"].strip():
+                mine.append(ReviewNote(f"review-{n}", self.account, p["body"], ""))
+            mine += [
+                ReviewNote(f"line-{n}-{i}", self.account, c.body, f"{c.path}:{c.line}")
+                for i, c in enumerate(p["comments"])
+            ]
+        return mine + list(self.notes)
+
+    def commits_between(self, repo: str, base: str, head: str) -> list[str]:
+        return list(self.since)
+
+    def commit_diff(self, repo: str, sha: str) -> str:
+        return f"diff of {sha}\n"
+
+    def react(self, repo: str, comment_id: int, content: str) -> None:
+        self.reactions.append((comment_id, content))
 
     def checks(self, repo: str, sha: str) -> str:
         return self.ci.get(sha, "success")
@@ -169,7 +214,9 @@ class FakeGitea:
 
     def post_comment(self, repo: str, index: int, body: str) -> Comment:
         self.comments.append((repo, index, body))
-        return Comment(99, self.account, body, "t")
+        said = Comment(self._id(), self.account, body, "t")
+        self.thread.append(said)
+        return said
 
     def close(self) -> None:
         pass

nisse-review/tests/test_review_rounds.py

@@ -0,0 +1,358 @@
+"""M7.5: nisse-review back on a PR it reviewed (R1, R2, R5): later reviews
+of what changed, replies to what was said, the round cap and its resets,
+and an operator's `stop` and `review`."""
+
+from dataclasses import replace
+from pathlib import Path
+from typing import Any
+
+import pytest
+from review_fakes import FakeGitea, Scripted, call, pr
+from test_review import REPO, REVIEW, item, world
+
+from nisse_core.gitea import GiteaError
+from nisse_review.review import Said, marker, reply_marker
+from nisse_review.scheduler import command
+from nisse_review.state import (
+    CAPPED,
+    FAILED,
+    READY,
+    REPLIES,
+    REVIEWED,
+    RUNNING,
+    STOPPED,
+    WAITING,
+    data,
+    pr_of,
+)
+
+PUSHED = "def789abc012"
+REPLY_P1 = [
+    [call("reply", point="P1", text="Fair: tabs never occur here. Conceded.")],
+    [call("done")],
+]
+
+
+def reviewed(tmp_path: Path, *turns: list[Any], **more: Any) -> dict[str, Any]:
+    """A PR given its first review; ``turns`` are the model's after that."""
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    w = world(
+        tmp_path, Scripted(*REVIEW, *turns), gitea, operators=frozenset({"don"}), **more
+    )
+    assert w["scheduler"].round() is True
+    assert item(w).position == REVIEWED
+    return w
+
+
+def push(w: dict[str, Any], head: str = PUSHED) -> None:
+    w["gitea"].repos[REPO] = [pr(head_sha=head)]
+    w["gitea"].since = [f"{head} Handle tabs"]
+
+
+def last_brief(w: dict[str, Any]) -> str:
+    model = w["scheduler"]._reviewer._driver._client
+    return str(model.seen[-1])
+
+
+# --- replies (R1) -----------------------------------------------------------------
+
+
+def test_an_answer_on_its_line_is_replied_to_on_that_line(tmp_path: Path) -> None:
+    w = reviewed(tmp_path, *REPLY_P1)
+    key = w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2")
+    assert w["scheduler"].round() is True  # replies wait for no CI
+    reply = w["gitea"].posted[-1]
+    [line] = reply["comments"]
+    assert (line.path, line.line) == ("count.py", 2)
+    assert reply_marker(key) in line.body and "Conceded" in line.body
+    assert (item(w).position, pr_of(item(w)).rounds) == (REVIEWED, 2)
+    assert "P1 — nisse-dev on count.py:2" in last_brief(w)
+    assert w["scheduler"].round() is False  # its own reply wakes nothing
+
+
+def test_a_top_level_comment_is_answered_quoting_it(tmp_path: Path) -> None:
+    w = reviewed(tmp_path, *REPLY_P1)
+    w["gitea"].say("nisse-dev", "Why flag the README?")
+    w["scheduler"].round()
+    [(_, _, body)] = w["gitea"].comments
+    assert body.startswith("> nisse-dev:\n> Why flag the README?")
+
+
+def test_every_point_is_answered_or_skipped_before_done(tmp_path: Path) -> None:
+    turns = [
+        [call("done")],  # P1 still open: refused, the run goes on
+        [call("skip", point="P1", reason="a CI notice; nothing to answer")],
+        [call("done")],
+    ]
+    w = reviewed(tmp_path, *turns)
+    w["gitea"].say("bot-ci", "Build 12 passed.")
+    w["scheduler"].round()
+    assert "first answer or skip each point said to you: P1" in str(
+        w["scheduler"]._reviewer._driver._client.seen[-1]
+    )
+    assert item(w).position == REVIEWED
+    assert pr_of(item(w)).rounds == 0  # a bot joined in: reset; nothing posted
+
+
+def test_a_point_answered_before_a_crash_is_not_answered_again(
+    tmp_path: Path,
+) -> None:
+    w = reviewed(tmp_path)  # no more model turns: a run would fail
+    key = w["gitea"].note("nisse-dev", "Done.", "count.py:2")
+    w["scheduler"].intake()
+    w["gitea"].say("nisse-review", f"Thanks.\n\n{reply_marker(key)}")
+    known = pr_of(item(w))
+    w["store"].update_work_item(
+        item(w).id, position=RUNNING, data=data(replace(known, run=REPLIES)), cause="t"
+    )
+    posts = len(w["gitea"].posted)
+    w["scheduler"].round()  # recovered, then nothing left to answer
+    assert item(w).position == REVIEWED and len(w["gitea"].posted) == posts
+
+
+# --- later reviews (R1) -----------------------------------------------------------
+
+
+def test_a_push_gets_a_later_review_of_what_changed(tmp_path: Path) -> None:
+    later = [[call("post_review", summary="P1 addressed: tabs handled.")]]
+    w = reviewed(tmp_path, *later)
+    push(w)
+    assert w["scheduler"].round() is True
+    brief = last_brief(w)
+    assert "The commits since" in brief and f"diff of {PUSHED}" in brief
+    assert "split() handles tabs too." in brief  # its earlier point
+    review = w["gitea"].posted[-1]
+    assert review["commit"] == PUSHED and marker(PUSHED, 2) in review["body"]
+    known = pr_of(item(w))
+    assert (known.reviewed, known.posts, known.rounds) == (PUSHED, 2, 1)
+
+
+def test_without_the_commits_since_it_reads_the_whole_diff(tmp_path: Path) -> None:
+    w = reviewed(tmp_path, [call("post_review", summary="ok")])
+    push(w)
+    w["gitea"].since = []  # a force-push dropped the old head
+    w["scheduler"].round()
+    assert "the whole PR's diff instead" in last_brief(w)
+
+
+def test_a_sign_off_is_said_once_while_it_stays_happy(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    model = Scripted(
+        [call("sign_off", summary="LGTM: small and tested.")],
+        [call("still_happy")],
+    )
+    w = world(tmp_path, model, gitea)
+    w["scheduler"].round()
+    assert pr_of(item(w)).happy and len(gitea.posted) == 1
+    push(w)
+    w["scheduler"].round()
+    assert len(gitea.posted) == 1  # nothing new: not said again
+    assert "still_happy" in model.offered[-1] and "sign_off" not in model.offered[-1]
+    assert pr_of(item(w)).reviewed == PUSHED
+
+
+def test_a_sign_off_with_drafted_comments_is_refused(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    model = Scripted(
+        [call("comment", path="count.py", line="2", text="Tabs?")],
+        [call("sign_off", summary="LGTM")],
+        [call("post_review", summary="One point.")],
+    )
+    w = world(tmp_path, model, gitea)
+    w["scheduler"].round()
+    assert "post them with post_review" in str(model.seen[-1])
+    assert not pr_of(item(w)).happy and len(gitea.posted[-1]["comments"]) == 1
+
+
+# --- rounds (R2) -------------------------------------------------------------------
+
+
+def test_after_the_cap_it_sums_up_and_goes_quiet(tmp_path: Path) -> None:
+    summary = [[call("post_summary", summary="Open: README wording. Don decides.")]]
+    w = reviewed(tmp_path, *REPLY_P1, *summary, max_rounds=2)
+    w["gitea"].note("nisse-dev", "No tabs here.", "count.py:2")
+    w["scheduler"].round()  # round 2: replies
+    w["gitea"].say("nisse-dev", "Still disagree on the README.")
+    w["scheduler"].round()  # past the cap: the summary
+    assert item(w).position == CAPPED
+    assert "Don decides" in w["gitea"].posted[-1]["body"]
+    w["gitea"].say("nisse-dev", "And another thing.")
+    assert w["scheduler"].round() is False  # the author alone: still quiet
+    assert item(w).position == CAPPED
+
+
+@pytest.mark.parametrize("who", ["don", "bot-ci"])
+def test_someone_else_joining_in_resets_the_count(tmp_path: Path, who: str) -> None:
+    summary = [[call("post_summary", summary="Open: README.")]]
+    w = reviewed(tmp_path, *summary, max_rounds=1)
+    w["gitea"].say("nisse-dev", "Disagree.")
+    w["scheduler"].round()
+    assert item(w).position == CAPPED
+    w["gitea"].say(who, "What's left here?")
+    w["scheduler"].intake()
+    assert (item(w).position, pr_of(item(w)).rounds) == (READY, 0)
+
+
+def test_a_push_resets_the_count(tmp_path: Path) -> None:
+    summary = [[call("post_summary", summary="Open: README.")]]
+    w = reviewed(tmp_path, *summary, max_rounds=1)
+    w["gitea"].say("nisse-dev", "Disagree.")
+    w["scheduler"].round()
+    push(w)
+    w["scheduler"].intake()
+    assert (item(w).position, pr_of(item(w)).rounds) == (READY, 0)
+
+
+# --- stop and review (R5) ----------------------------------------------------------
+
+
+def test_stop_quiets_it_until_review_asks_again(tmp_path: Path) -> None:
+    fresh = [
+        [call("read_file", path="count.py", ref="head")],
+        [call("post_review", summary="Fresh look: fine.")],
+    ]
+    w = reviewed(tmp_path, *fresh)
+    stop = w["gitea"].say("don", "stop")
+    w["gitea"].say("nisse-dev", "Any thoughts?")
+    push(w)
+    assert w["scheduler"].round() is False
+    assert item(w).position == STOPPED
+    assert (stop, "+1") in w["gitea"].reactions
+    w["gitea"].say("don", "Review.")
+    assert w["scheduler"].round() is True
+    assert "Review o/demo#7" in last_brief(w)  # the whole PR, afresh
+    assert w["gitea"].posted[-1]["commit"] == PUSHED
+    assert (item(w).position, pr_of(item(w)).rounds) == (REVIEWED, 1)
+
+
+def test_review_retries_a_failed_item(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    w = world(
+        tmp_path, Scripted(*REVIEW), gitea, attempts=1, operators=frozenset({"don"})
+    )
+    w["store"].open_work_item(
+        f"{REPO}#7",
+        title="t",
+        scope=REPO,
+        position=FAILED,
+        data=data(pr_of_fresh(attempts=1)),
+        cause="t",
+    )
+    gitea.say("don", "review")
+    assert w["scheduler"].round() is True
+    assert item(w).position == REVIEWED
+
+
+def pr_of_fresh(**more: Any):
+    from nisse_review.state import PR
+
+    return PR(REPO, 7, pr().head_sha, seen=("comment-1",), **more)
+
+
+@pytest.mark.parametrize(
+    ("said", "expected"),
+    [
+        (Said("comment-5", "don", " Stop "), "stop"),
+        (Said("comment-5", "don", "`review`"), "review"),
+        (Said("comment-5", "don", "please stop"), None),
+        (Said("comment-5", "nisse-dev", "stop"), None),  # not an operator
+        (Said("line-5", "don", "stop", "a.py:1"), None),  # not top-level
+    ],
+)
+def test_a_command_is_an_operators_whole_comment(said: Said, expected) -> None:
+    assert command(said, frozenset({"don"})) == expected
+
+
+# --- upgrades, failures and dead ends ----------------------------------------------
+
+
+def test_an_item_reviewed_before_m7_5_answers_nothing_old(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    w = world(tmp_path, Scripted(), gitea)
+    old = {
+        "repo": REPO,
+        "number": 7,
+        "head": pr().head_sha,
+        "reviewed": pr().head_sha,
+        "attempts": 0,
+        "seen_at": 0.0,
+    }
+    w["store"].open_work_item(
+        f"{REPO}#7",
+        title="t",
+        scope=REPO,
+        position=REVIEWED,
+        data={"pr": old},
+        cause="t",
+    )
+    gitea.say("nisse-dev", "An old answer.")
+    assert w["scheduler"].round() is False
+    known = pr_of(item(w))
+    assert (known.fresh, known.posts) == ((), 1)
+
+
+def test_what_was_said_unreadable_holds_up_no_other_pr(tmp_path: Path) -> None:
+    class Flaky(FakeGitea):
+        def list_comments(self, repo: str, index: int, since: str | None = None):
+            if index == 7:
+                raise GiteaError("502")
+            return super().list_comments(repo, index)
+
+    gitea = Flaky(repos={REPO: [pr(7), pr(8, head_sha="c8")]})
+    w = world(tmp_path, Scripted(*REVIEW), gitea)
+    assert w["scheduler"].round() is True
+    assert w["store"].work_item(f"{REPO}#7") is None  # next round
+    assert item(w, 8).position == REVIEWED
+
+
+def test_no_position_is_a_dead_end(tmp_path: Path) -> None:
+    """Every position an open item can hold has an event that moves it on."""
+    w = reviewed(tmp_path, *REPLY_P1, max_rounds=1)
+    s, g = w["scheduler"], w["gitea"]
+
+    def at(position: str, **more: Any) -> None:
+        w["store"].update_work_item(
+            item(w).id,
+            position=position,
+            data=data(replace(pr_of(item(w)), **more)),
+            cause="t",
+        )
+
+    at(STOPPED)
+    g.say("don", "review")
+    s.intake()
+    assert item(w).position == READY
+    at(CAPPED, full=False)
+    push(w, "cap0000")
+    s.intake()
+    assert item(w).position == READY
+    at(FAILED)
+    push(w, "fail000")
+    s.intake()
+    assert item(w).position == READY
+    at(REVIEWED, reviewed="fail000")
+    g.say("bot-ci", "Coverage 90%.")
+    s.intake()
+    assert item(w).position == READY
+    at(WAITING)
+    s.intake()
+    assert item(w).position == READY  # ready again: back in line
+    at(RUNNING, run=REPLIES)
+    s.recover()
+    assert item(w).position == READY
+
+
+def test_a_summary_answers_nothing_and_a_first_review_answers_no_points(
+    tmp_path: Path,
+) -> None:
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    model = Scripted(*REVIEW, [call("post_summary", summary="Open: README.")])
+    w = world(tmp_path, model, gitea, max_rounds=0)
+    w["scheduler"].intake()
+    gitea.say("nisse-dev", "Said before any review.")
+    w["scheduler"].round()  # its first review: no points to answer one by one
+    assert "reply" not in model.offered[0] and item(w).position == REVIEWED
+    gitea.say("nisse-dev", "Disagree.")
+    w["scheduler"].round()
+    assert "reply" not in model.offered[-1] and "post_summary" in model.offered[-1]
+    assert item(w).position == CAPPED