Merge pull request 'nisse-review marks a reply that settles its thread; the PR's author resolves it' (#169) from review-settled into main
CI / checks (push) Successful in 3m26s
CI / e2e (push) Skipped

This commit was merged in pull request #169.
This commit is contained in:
dev-agent committed 2026-10-19 23:40:27 +00:00
commit 5277d7827b
11 files changed
+406 -12

No files matched your search

+8
View File
@@ -707,6 +707,14 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and
to the model to make again (on #166 it didn't, and looped to the turn cap).
Each reply carries its point's key, so a run re-done after a crash answers
nothing twice.
- **Settled threads:** each `reply` says whether it settles its thread (a
concession, or an answer or deferral accepted; never while waiting on a
change). A settled reply carries `<!-- nisse-review: settled -->`. Being
read-only (R3), the reviewer can't resolve the thread, so the PR's author
does: nisse-dev resolves every comment on that thread's line when it next
watches the PR and takes the settled reply as no point to answer (nor one
posted top-level, which has no thread to resolve);
`scripts/resolve_settled.py` does the same for PRs made by hand.
- **Sign-off:** `sign_off` posts a short review when there is nothing to raise;
while it stays happy, a later review offers `still_happy`, which posts nothing
(operator: no repeated LGTMs).
+28
View File
@@ -86,6 +86,28 @@ class ReviewNote:
author: str
body: str
where: str # "path:line" for a line comment, "" for a review body
resolved: bool = False # a line comment's thread, marked resolved
# A reviewer's reply that settles its thread: nothing more is needed from
# it there. It can't resolve the thread itself (read-only, R3), so the PR's
# author does.
SETTLED_MARKER = "<!-- nisse-review: settled -->"
def to_resolve(notes: list[ReviewNote]) -> list[int]:
"""The ids of the unresolved line comments in threads a settled reply
closed: every comment on that path:line, as Gitea groups a thread.
Gitea resolves one comment at a time (seen on 28.2: the rest of the
thread still reports no resolver), and resolving one again is a 204."""
settled = {
n.where for n in notes if n.key.startswith("line-") and SETTLED_MARKER in n.body
}
return [
int(n.key.removeprefix("line-"))
for n in notes
if n.key.startswith("line-") and n.where in settled and not n.resolved
]
@dataclass(frozen=True)
@@ -198,10 +220,16 @@ class GiteaClient:
author=(comment.get("user") or {}).get("login", author),
body=str(comment.get("body") or ""),
where=f"{comment.get('path', '')}:{line}",
resolved=bool(comment.get("resolver")),
)
)
return notes
def resolve_comment(self, repo: str, comment_id: int) -> None:
"""Mark a line comment's thread resolved: the PR's author or someone
with write access may."""
self._request("POST", f"/repos/{repo}/pulls/comments/{comment_id}/resolve")
def find_pull_request(self, repo: str, head: str) -> PullRequest | None:
"""External truth for L8: did the PR for this branch already get
opened? Includes closed PRs — a crash after opening must not be
+65
View File
@@ -5,11 +5,14 @@ import pytest
from nisse_core.gitea import (
PR_PAGE_SIZE,
SETTLED_MARKER,
Comment,
GiteaClient,
GiteaError,
GiteaNotFound,
ReviewNote,
pull_request_reconciler,
to_resolve,
)
from nisse_core.state.store import Intent
@@ -445,3 +448,65 @@ def test_commits_between_are_oldest_first_and_diffs_come_through_the_api() -> No
method, path, body = seen[2]
assert (method, path) == ("POST", "/api/v1/repos/o/r/issues/comments/42/reactions")
assert json.loads(body) == {"content": "+1"}
# --- settled threads ------------------------------------------------------------
def line(n: int, where: str, body: str = "", resolved: bool = False) -> ReviewNote:
return ReviewNote(f"line-{n}", "someone", body, where, resolved)
def test_a_settled_reply_resolves_every_comment_in_its_thread() -> None:
notes = [
line(1, "a.py:3", "a point"),
line(2, "a.py:3", "an answer"),
line(3, "a.py:3", f"conceded\n{SETTLED_MARKER}"),
line(4, "a.py:9", "another point, still open"),
ReviewNote("review-5", "someone", f"summary {SETTLED_MARKER}", ""),
]
assert to_resolve(notes) == [1, 2, 3]
def test_a_thread_already_resolved_is_left_alone() -> None:
notes = [
line(1, "a.py:3", "a point", resolved=True),
line(2, "a.py:3", f"conceded\n{SETTLED_MARKER}", resolved=True),
]
assert to_resolve(notes) == []
def test_review_notes_say_which_line_comments_are_resolved() -> None:
def handler(request: httpx.Request) -> httpx.Response:
if request.url.path.endswith("/reviews"):
return httpx.Response(200, json=[{"id": 7, "comments_count": 2}])
return httpx.Response(
200,
json=[
{"id": 1, "body": "x", "path": "a.py", "position": 3},
{
"id": 2,
"body": "y",
"path": "a.py",
"position": 3,
"resolver": {"login": "dev-agent"},
},
],
)
notes = client(handler).review_notes("o/r", 4)
assert [(n.key, n.where, n.resolved) for n in notes] == [
("line-1", "a.py:3", False),
("line-2", "a.py:3", True),
]
def test_resolving_a_comment_posts_to_its_resolve_endpoint() -> None:
seen: list[tuple[str, str]] = []
def handler(request: httpx.Request) -> httpx.Response:
seen.append((request.method, request.url.path))
return httpx.Response(200)
client(handler).resolve_comment("o/r", 42)
assert seen == [("POST", "/api/v1/repos/o/r/pulls/comments/42/resolve")]
+33 -1
View File
@@ -36,7 +36,16 @@ from nisse_core.authority.actions import (
)
from nisse_core.conversation.transport import InboundMessage, Transport
from nisse_core.execution.ssh import HostKeyChanged, HostUnavailable
from nisse_core.gitea import GiteaClient, GiteaError, Issue, LineComment, PullRequest
from nisse_core.gitea import (
SETTLED_MARKER,
GiteaClient,
GiteaError,
Issue,
LineComment,
PullRequest,
ReviewNote,
to_resolve,
)
from nisse_core.knowledge import handlers as knowledge
from nisse_core.knowledge.memory import Memory
from nisse_core.knowledge.vault import Vault
@@ -1058,6 +1067,24 @@ class LoopAgent:
# -- PR review and merge (D4, D5) -------------------------------------------
def _resolve_settled(
self, item: WorkItem, repo: str, notes: list[ReviewNote]
) -> None:
"""Resolve the threads a reviewer's reply settled: it can't, being
read-only (R3); this agent, the PR's author, can. A refusal leaves
the thread open for the operator, and is tried again next round."""
for comment_id in to_resolve(notes):
try:
self._gitea.resolve_comment(repo, comment_id)
except GiteaError as exc:
self._oplog.emit(
"loop.resolve_failed", cause=str(exc)[:300], work_item=item.id
)
return
self._oplog.emit(
"loop.resolved", cause="settled", work_item=item.id, comment=comment_id
)
def watch(self) -> None:
"""New comments on an item's PR, and its merge or close, reach the
item; a waiting item becomes ready to act on them. An operator's are
@@ -1075,6 +1102,7 @@ class LoopAgent:
except GiteaError as exc:
self._oplog.emit("loop.watch_unavailable", cause=str(exc))
continue
self._resolve_settled(item, repo, notes)
log = self._store.transcript(item.id, "operator")
new = peers = False
for comment in comments:
@@ -1083,6 +1111,8 @@ class LoopAgent:
loop = replace(loop, comments=comment.id)
if comment.author == self._account:
continue # its own replies
if SETTLED_MARKER in comment.body:
continue # a settled reply posted top-level: nothing to answer
if comment.author not in self._gitea_operators:
point = f"c{comment.id}"
loop = self._peer_point(
@@ -1109,6 +1139,8 @@ class LoopAgent:
loop = replace(loop, notes=(*loop.notes, note.key))
if note.author == self._account:
continue
if SETTLED_MARKER in note.body:
continue # it closes its thread: resolved, not answered
if note.author not in self._gitea_operators:
loop = self._peer_point(
item, loop, note.key, note.author, note.where, note.body
+7
View File
@@ -78,6 +78,8 @@ class Forge:
self.approved: dict[int, list[Approval]] = {}
self.status: dict[str, str] = {} # commit -> combined check state
self.agent_merged: list[int] = []
self.resolved: list[int] = [] # line comments the agent resolved
self.refuse_resolve = False
# intake
def assigned_issues(self, repo: str, assignee: str) -> list[Issue]:
@@ -171,6 +173,11 @@ class Forge:
def post_comment(self, repo: str, index: int, body: str) -> None:
self.posted.append((index, body))
def resolve_comment(self, repo: str, comment_id: int) -> None:
if self.refuse_resolve:
raise GiteaError("gitea POST resolve returned 403")
self.resolved.append(comment_id)
def post_review(self, repo: str, index: int, *, body, commit, comments=()) -> int:
if self.refuse_inline:
raise GiteaError("gitea POST reviews returned 422: line not in diff")
+67 -1
View File
@@ -18,7 +18,7 @@ from test_loop_milestones import item as the_item
from test_loop_milestones import world as world # noqa: PLC0414 -- the fixture
from test_loop_trust import agent_for
from nisse_core.gitea import Comment, LineComment, ReviewNote
from nisse_core.gitea import SETTLED_MARKER, Comment, LineComment, ReviewNote
from nisse_core.log.canonical import Fact
from nisse_dev.loop.items import MERGE, READY, WAITING, loop_of
@@ -244,3 +244,69 @@ def test_a_peer_neither_merges_nor_instructs(world: dict[str, Any]) -> None:
]
assert not any("merge it" in getattr(e.event, "text", "") for e in operator)
assert loop_of(the_item(world)).wait == MERGE
# --- settled threads ------------------------------------------------------------
def settled_thread(forge: Forge, pr: int) -> None:
"""A point on a line, nisse-dev's answer, and the reviewer conceding."""
forge.notes[pr] = [
ReviewNote("line-11", REVIEWER, "Split on any whitespace?", "text.py:3"),
ReviewNote("line-12", "nisse-dev", "split() already does.", "text.py:3"),
ReviewNote("line-13", REVIEWER, f"Conceded.\n\n{SETTLED_MARKER}", "text.py:3"),
]
def test_a_settled_thread_is_resolved_not_answered(world: dict[str, Any]) -> None:
forge: Forge = world["forge"]
agent, _, pr = published(world)
settled_thread(forge, pr)
forge.notes[pr].append(ReviewNote("line-14", REVIEWER, "Still: a test?", "a.py:1"))
agent.watch()
assert forge.resolved == [11, 12, 13]
keys = [p[1] for p in loop_of(the_item(world)).points]
assert "line-13" not in keys and "line-14" in keys # the concession needs no answer
def test_a_resolved_thread_is_not_resolved_again(world: dict[str, Any]) -> None:
forge: Forge = world["forge"]
agent, _, pr = published(world)
settled_thread(forge, pr)
forge.notes[pr] = [
ReviewNote(n.key, n.author, n.body, n.where, resolved=True)
for n in forge.notes[pr]
]
agent.watch()
assert forge.resolved == []
def test_a_refused_resolve_leaves_the_thread_and_is_tried_again(
world: dict[str, Any],
) -> None:
forge: Forge = world["forge"]
agent, _, pr = published(world)
settled_thread(forge, pr)
forge.refuse_resolve = True
agent.watch()
assert forge.resolved == []
forge.refuse_resolve = False
agent.watch()
assert forge.resolved == [11, 12, 13]
def test_a_settled_reply_posted_top_level_is_not_a_point(
world: dict[str, Any],
) -> None:
"""One quoting a top-level point, or Gitea refused on its line: there's
no thread to resolve, and answering it would start the loop again."""
forge: Forge = world["forge"]
agent, _, pr = published(world)
forge.comments[pr] = [
Comment(
15, REVIEWER, f"> nisse-dev:\n> Done.\n\nThanks.\n{SETTLED_MARKER}", "t"
)
]
agent.watch()
loop = loop_of(the_item(world))
assert (the_item(world).position, loop.points) == (WAITING, ())
+17 -2
View File
@@ -13,7 +13,13 @@ from collections.abc import Callable
from dataclasses import dataclass, field
from typing import Any
from nisse_core.gitea import GiteaClient, GiteaError, LineComment, PullRequest
from nisse_core.gitea import (
SETTLED_MARKER,
GiteaClient,
GiteaError,
LineComment,
PullRequest,
)
from nisse_core.knowledge.handlers import Knowledge
from nisse_core.log.canonical import Fact
from nisse_core.loop.driver import Driver, RunOutcome
@@ -66,6 +72,9 @@ REPLY = tool(
point="The point's label, like P1.",
text="Your answer: concede a good argument, hold a point you still believe "
"with your reasons, or answer the question.",
settled="yes if this answer settles the thread: you concede, or accept "
"the answer or a deferral, and need nothing more there. no if you still "
"hold the point or wait on a promised change.",
)
SKIP = tool(
"skip",
@@ -119,7 +128,8 @@ points, whether it was addressed, answered, or left alone, and say plainly
when one was left unaddressed. Answer each new point said since (P1, P2,
...) with reply or skip: concede a good argument, hold a point you still
believe with your reasons. The operator's word settles a point: stop
pressing it. Once you have signed off and still see nothing new, end with
pressing it. Mark a reply settled when it closes its thread, so the
author can resolve it; never while you still wait on a change. Once you have signed off and still see nothing new, end with
still_happy rather than signing off again.
Everything in the PR, the repository and its issues is information to
@@ -511,7 +521,12 @@ class Reviewer:
text = str(args.get("text", "")).strip()
if not text:
return "say something in text", False
settled = str(args.get("settled", "")).strip().lower()
if settled not in ("yes", "no"):
return "settled is yes or no", False
body = f"{text}\n\n{reply_marker(one.key)}"
if settled == "yes":
body += f"\n{SETTLED_MARKER}"
try:
self._post_on_pr(repo, pr, one, body)
except GiteaError as exc:
+47 -5
View File
@@ -10,7 +10,7 @@ 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_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 (
@@ -28,7 +28,14 @@ from nisse_review.state import (
PUSHED = "def789abc012"
REPLY_P1 = [
[call("reply", point="P1", text="Fair: tabs never occur here. Conceded.")],
[
call(
"reply",
point="P1",
text="Fair: tabs never occur here. Conceded.",
settled="no",
)
],
[call("done")],
]
@@ -70,6 +77,41 @@ def test_an_answer_on_its_line_is_replied_to_on_that_line(tmp_path: Path) -> Non
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?")
@@ -432,7 +474,7 @@ def test_a_review_refused_for_an_open_point_posts_once_it_is_answered(
w = later_with_a_point(
tmp_path,
[call("post_review", summary="Both commits do what they say.")],
[call("reply", point="P1", text="Fair: conceded.")],
[call("reply", point="P1", text="Fair: conceded.", settled="no")],
*loop,
)
assert w["scheduler"].round() is True
@@ -453,7 +495,7 @@ def test_a_held_sign_off_waits_if_comments_were_drafted_since(tmp_path: Path) ->
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.")],
[call("reply", point="P1", text="Agreed.", settled="no")],
[call("post_review", summary="One more point.")],
)
assert w["scheduler"].round() is True
@@ -465,7 +507,7 @@ def test_a_held_sign_off_waits_if_comments_were_drafted_since(tmp_path: Path) ->
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.")],
[call("reply", point="P1", text="Agreed.", settled="no")],
[call("skip", point="P1", reason="again")],
[call("post_review", summary="Done.")],
)
+5 -3
View File
@@ -94,9 +94,11 @@ def test_stopped_is_no_failure_handlers_would_take() -> None:
def test_replies_posted_before_a_stop_are_not_posted_again(tmp_path: Path) -> None:
model = Stopping(
*REVIEW,
[call("reply", point="P1", text="Tabs never occur here.")],
[call("reply", point="P2", text="It counts words, yes.")], # stopped here
[call("reply", point="P1", text="It counts words, yes.")],
[call("reply", point="P1", text="Tabs never occur here.", settled="no")],
[
call("reply", point="P2", text="It counts words, yes.", settled="no")
], # stopped here
[call("reply", point="P1", text="It counts words, yes.", settled="no")],
[call("done")],
at=5,
)
+56
View File
@@ -0,0 +1,56 @@
"""Resolve the review threads nisse-review settled on a PR.
uv run python scripts/resolve_settled.py PR [--repo dev/nisse]
nisse-review marks a reply that closes its thread, but it is read-only (R3)
and can't resolve the thread itself; the PR's author does. nisse-dev
resolves its own PRs' threads; this is for PRs made by hand or by Claude
Code. The Gitea URL and token come from tea's login (~/.config/tea).
"""
import argparse
import sys
from pathlib import Path
import yaml
from nisse_core.gitea import GiteaClient, to_resolve
TEA_CONFIG = Path.home() / ".config" / "tea" / "config.yml"
def tea_login(path: Path = TEA_CONFIG) -> tuple[str, str]:
"""tea's default login (or its only one): the Gitea URL and its token."""
logins = (yaml.safe_load(path.read_text()) or {}).get("logins") or []
if not logins:
raise SystemExit(f"no tea login in {path}")
login = next((one for one in logins if one.get("default")), logins[0])
return str(login["url"]), str(login["token"])
def resolve_settled(gitea: GiteaClient, repo: str, pr: int) -> list[int]:
"""Resolve every comment in the threads a settled reply closed."""
done = []
for comment_id in to_resolve(gitea.review_notes(repo, pr)):
gitea.resolve_comment(repo, comment_id)
done.append(comment_id)
return done
def main(argv: list[str] | None = None, gitea: GiteaClient | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0])
parser.add_argument("pr", type=int, help="the PR's number")
parser.add_argument("--repo", default="dev/nisse", help="owner/name")
args = parser.parse_args(argv)
if gitea is None:
gitea = GiteaClient(*tea_login())
done = resolve_settled(gitea, args.repo, args.pr)
print(
f"resolved {len(done)} comment(s) on {args.repo}#{args.pr}"
+ (f": {', '.join(map(str, done))}" if done else "")
)
return 0
if __name__ == "__main__":
sys.exit(main())
+73
View File
@@ -0,0 +1,73 @@
"""scripts/resolve_settled.py: the threads nisse-review settled on a PR made
by hand get resolved, and nothing else does."""
import importlib.util
from pathlib import Path
from types import ModuleType
import httpx
import pytest
from nisse_core.gitea import SETTLED_MARKER, GiteaClient
SCRIPT = Path(__file__).parent.parent / "scripts" / "resolve_settled.py"
def script() -> ModuleType:
spec = importlib.util.spec_from_file_location("resolve_settled", SCRIPT)
assert spec and spec.loader
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module
def comment(n: int, line: int, body: str, resolver: str | None = None) -> dict:
return {
"id": n,
"body": body,
"path": "a.py",
"position": line,
"resolver": {"login": resolver} if resolver else None,
}
def fake_gitea(comments: list[dict], resolved: list[int]) -> GiteaClient:
def handler(request: httpx.Request) -> httpx.Response:
path = request.url.path
if path.endswith("/resolve"):
resolved.append(int(path.split("/")[-2]))
return httpx.Response(200)
if path.endswith("/reviews"):
return httpx.Response(200, json=[{"id": 1, "comments_count": 1}])
return httpx.Response(200, json=comments)
return GiteaClient("https://g.test", "tok", transport=httpx.MockTransport(handler))
def test_it_resolves_the_settled_threads_only(
capsys: pytest.CaptureFixture[str],
) -> None:
resolved: list[int] = []
gitea = fake_gitea(
[
comment(1, 3, "a point"),
comment(2, 3, "an answer"),
comment(3, 3, f"Conceded.\n\n{SETTLED_MARKER}"),
comment(4, 9, "still held"),
comment(5, 12, f"done\n{SETTLED_MARKER}", resolver="dongar"),
],
resolved,
)
assert script().main(["168"], gitea) == 0
assert resolved == [1, 2, 3]
assert "resolved 3 comment(s) on dev/nisse#168: 1, 2, 3" in capsys.readouterr().out
def test_it_reads_teas_default_login(tmp_path: Path) -> None:
config = tmp_path / "config.yml"
config.write_text(
"logins:\n"
" - {name: a, url: https://a.test, token: ta, default: false}\n"
" - {name: b, url: https://b.test, token: tb, default: true}\n"
)
assert script().tea_login(config) == ("https://b.test", "tb")