The reviewer is read-only (R3), so Gitea refuses it resolving a thread. Now each reply says whether it settles its thread, and a settled reply carries <!-- nisse-review: settled -->. nisse-dev, as the author, resolves every comment on that thread's line when it watches the PR, and doesn't take the settled reply as a point to answer. scripts/resolve_settled.py does the same for PRs made by hand. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
520 lines
19 KiB
Python
520 lines
19 KiB
Python
"""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 SETTLED_MARKER, 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.",
|
|
settled="no",
|
|
)
|
|
],
|
|
[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_settled_reply_says_so_for_the_author(tmp_path: Path) -> None:
|
|
settle = [
|
|
[call("reply", point="P1", text="Conceded.", settled="yes")],
|
|
[call("done")],
|
|
]
|
|
w = reviewed(tmp_path, *settle)
|
|
w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2")
|
|
w["scheduler"].round()
|
|
[line] = w["gitea"].posted[-1]["comments"]
|
|
assert SETTLED_MARKER in line.body
|
|
|
|
|
|
def test_a_reply_still_holding_its_point_leaves_the_thread_open(
|
|
tmp_path: Path,
|
|
) -> None:
|
|
w = reviewed(tmp_path, *REPLY_P1)
|
|
w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2")
|
|
w["scheduler"].round()
|
|
[line] = w["gitea"].posted[-1]["comments"]
|
|
assert SETTLED_MARKER not in line.body
|
|
|
|
|
|
def test_a_reply_must_say_whether_it_settles(tmp_path: Path) -> None:
|
|
turns = [
|
|
[call("reply", point="P1", text="Conceded.", settled="maybe")],
|
|
*REPLY_P1,
|
|
]
|
|
w = reviewed(tmp_path, *turns)
|
|
w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2")
|
|
w["scheduler"].round()
|
|
assert "settled is yes or no" in last_brief(w)
|
|
[line] = w["gitea"].posted[-1]["comments"] # only the second reply posted
|
|
assert "Conceded." in line.body and "tabs never occur" in line.body
|
|
|
|
|
|
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 on a line
|
|
assert "- your review: Looks right; two small points." in brief # its summary
|
|
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_commit_whose_diff_cannot_be_read_falls_back_too(tmp_path: Path) -> None:
|
|
# nisse-review on #156: only a failed listing fell back; a failed diff
|
|
# cost a run, in the force-push case the fallback is for.
|
|
class Gone(FakeGitea):
|
|
def commit_diff(self, repo: str, sha: str) -> str:
|
|
raise GiteaError("404: no such commit")
|
|
|
|
gitea = Gone(repos={REPO: [pr()]})
|
|
w = world(tmp_path, Scripted(*REVIEW, [call("post_review", summary="ok")]), gitea)
|
|
w["scheduler"].round()
|
|
push(w)
|
|
w["scheduler"].round()
|
|
assert "the whole PR's diff instead" in last_brief(w)
|
|
assert item(w).position == REVIEWED
|
|
|
|
|
|
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 w["gitea"].reactions == [(stop, "+1")]
|
|
w["scheduler"].round()
|
|
assert w["gitea"].reactions == [(stop, "+1")] # taken in: said once
|
|
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
|
|
|
|
|
|
def test_a_second_review_at_the_same_head_has_its_own_marker(tmp_path: Path) -> None:
|
|
# nisse-review on #156 feared the first review's unnumbered marker would
|
|
# pass for a crashed second one at the same head.
|
|
head = pr().head_sha
|
|
w = reviewed(tmp_path)
|
|
reviewer = w["scheduler"]._reviewer
|
|
assert reviewer.posted(REPO, 7, head, 1)
|
|
assert not reviewer.posted(REPO, 7, head, 2) # `review` again: not yet
|
|
w["gitea"].posted.append(
|
|
{
|
|
"repo": REPO,
|
|
"pr": 7,
|
|
"body": f"again\n\n{marker(head, 2)}",
|
|
"commit": head,
|
|
"comments": [],
|
|
}
|
|
)
|
|
assert reviewer.posted(REPO, 7, head, 2)
|
|
assert marker(head, 1) not in marker(head, 2)
|
|
|
|
|
|
def test_review_with_a_comment_beside_it_is_one_fresh_review(tmp_path: Path) -> None:
|
|
# nisse-review on #156 feared a replies run, then a second, pointless
|
|
# whole-PR review: `review` makes the run a first one, which clears it.
|
|
w = reviewed(tmp_path, [call("post_review", summary="Fresh look.")])
|
|
w["gitea"].say("don", "review")
|
|
w["gitea"].say("nisse-dev", "Also, a question.")
|
|
assert w["scheduler"].round() is True
|
|
assert "Review o/demo#7" in last_brief(w) # a first review, not replies
|
|
known = pr_of(item(w))
|
|
assert (known.full, known.fresh, item(w).position) == (False, (), REVIEWED)
|
|
assert w["scheduler"].round() is False # nothing more due
|
|
|
|
|
|
# --- a held ending call (the run on #166 at bf98445) --------------------------------
|
|
|
|
|
|
def later_with_a_point(tmp_path: Path, *turns: list[Any]) -> dict[str, Any]:
|
|
"""A push and an answer on its line since its first review: a later run
|
|
with P1 to answer."""
|
|
w = reviewed(tmp_path, *turns)
|
|
push(w)
|
|
w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2")
|
|
return w
|
|
|
|
|
|
def test_a_review_refused_for_an_open_point_posts_once_it_is_answered(
|
|
tmp_path: Path,
|
|
) -> None:
|
|
# The model posted with P1 open, answered it, then only skipped answered
|
|
# points until the turn cap: the review went out partial, its summary lost.
|
|
loop = [[call("skip", point="P1", reason="already answered")]] * 5
|
|
w = later_with_a_point(
|
|
tmp_path,
|
|
[call("post_review", summary="Both commits do what they say.")],
|
|
[call("reply", point="P1", text="Fair: conceded.", settled="no")],
|
|
*loop,
|
|
)
|
|
assert w["scheduler"].round() is True
|
|
review = w["gitea"].posted[-1]
|
|
assert "Both commits do what they say." in review["body"]
|
|
assert "partial" not in review["body"].lower()
|
|
assert item(w).position == REVIEWED
|
|
model = w["scheduler"]._reviewer._driver._client
|
|
assert len(model._turns) == 5 # none of the skips ran
|
|
told = str(model.seen[-1])
|
|
assert "Your post_review is held" in told
|
|
|
|
|
|
def test_a_held_sign_off_waits_if_comments_were_drafted_since(tmp_path: Path) -> None:
|
|
# Held while P1 was open, but a drafted comment needs post_review now:
|
|
# the harness doesn't sign off over it; the model posts instead.
|
|
w = later_with_a_point(
|
|
tmp_path,
|
|
[call("sign_off", summary="All good.")],
|
|
[call("comment", path="count.py", line="2", text="One more thing.")],
|
|
[call("reply", point="P1", text="Agreed.", settled="no")],
|
|
[call("post_review", summary="One more point.")],
|
|
)
|
|
assert w["scheduler"].round() is True
|
|
review = w["gitea"].posted[-1]
|
|
assert "One more point." in review["body"]
|
|
assert ["One more thing." in c.body for c in review["comments"]] == [True]
|
|
|
|
|
|
def test_with_nothing_open_a_skip_says_how_to_end(tmp_path: Path) -> None:
|
|
w = later_with_a_point(
|
|
tmp_path,
|
|
[call("reply", point="P1", text="Agreed.", settled="no")],
|
|
[call("skip", point="P1", reason="again")],
|
|
[call("post_review", summary="Done.")],
|
|
)
|
|
w["scheduler"].round()
|
|
model = w["scheduler"]._reviewer._driver._client
|
|
assert "every point said to you is answered or skipped. End your run with" in str(
|
|
model.seen[-1]
|
|
)
|
|
assert "post_review" in str(model.seen[-1]).split("End your run with")[1]
|