Merge pull request 'Answer where asked, wait on a merge, and let the driver title PRs' (#100) from loop-review-replies into main

Reviewed-on: dongar/nisse#100
Reviewed-by: Don Garrett <dgarrett@acm.org>

docs/Development-Plan.md

@@ -130,7 +130,8 @@ Built alongside `control.py`, reusing the state store, log, views, publisher,
 pollers and recovery. The first milestone is the smallest end-to-end path; each
 later one widens it and extends the end-to-end test (M1.6) to cover it.
 
-**M2.1 The slice.** *Needs discussion* (the loop's shape in code).
+**M2.1 The slice.** *Needs discussion* (the loop's shape in code). **Done** (#96,
+#97, #98, #99; first live run: sandbox #255, follow-ups in #100).
 - An issue assigned to `dev-agent` → a work item (D1); a restatement in the item's
   thread; a go-ahead bound to its revision (L3, L5); one milestone through
   `start_milestone` / `finish_milestone` on a harness branch (D4); the PR; a review

nisse-dev/src/nisse_dev/loop/agent.py

@@ -43,6 +43,7 @@ from nisse_dev.loop.tools import (
     FINISH_MILESTONE,
     RESTATE,
     START_MILESTONE,
+    WAIT_FOR_REVIEW,
 )
 from nisse_dev.oplog import OpLog
 from nisse_dev.state import StateStore, WorkItem
@@ -73,7 +74,9 @@ The harness commits, pushes and opens a PR, and you wait for review. Review
 comments arrive as messages: make the changes within the same milestone (don't
 start another), run the tests, and call finish_milestone again to push the fix. When the PR has merged, call
 close_item with what was done, what was verified, and what is left open. If
-the PR is closed without merging, ask the operator what next.
+the PR is closed without merging, ask the operator what next. When a review
+needs no change from you, or you are only waiting for the operator to merge
+(you can't merge it yourself), call wait_for_review.
 
 {workspace}"""
 
@@ -265,6 +268,12 @@ class LoopAgent:
                             source=f"gitea:pr:{loop.pr}:comment:{comment.id}",
                         )
                     )
+                    loop = replace(
+                        loop,
+                        asked_on_pr=loop.pr,
+                        asked_where="",
+                        asked_text=comment.body[:200],
+                    )
                     new = True
             for note in notes:
                 if note.key in loop.notes:
@@ -278,6 +287,12 @@ class LoopAgent:
                             source=f"gitea:pr:{loop.pr}:review:{note.key}",
                         )
                     )
+                    loop = replace(
+                        loop,
+                        asked_on_pr=loop.pr,
+                        asked_where=note.where,
+                        asked_text=note.body[:200],
+                    )
                     new = True
             position = item.position
             if pr.merged:
@@ -372,7 +387,10 @@ class LoopAgent:
         self._store.transcript(item.id, "operator").append(
             UserMessage(text=text, source=f"transport:{msg.sender}")
         )
-        loop = loop_of(item)
+        loop = replace(loop_of(item), asked_on_pr=0, asked_where="", asked_text="")
+        if loop != loop_of(item):
+            save(self._store, item.id, item.position, loop, cause="operator spoke")
+            item = self._store.work_item(item.id) or item
         if command == "abandon":
             self._close(item, loop, "abandoned", f"abandoned by {msg.sender}")
         elif item.position == WAITING and loop.wait == GO_AHEAD and command in _GO:
@@ -454,7 +472,13 @@ class LoopAgent:
             *(
                 _LOOK
                 if not loop.started
-                else [START_MILESTONE, FINISH_MILESTONE, CLOSE_ITEM, *EXECUTOR_TOOLS]
+                else [
+                    START_MILESTONE,
+                    FINISH_MILESTONE,
+                    CLOSE_ITEM,
+                    *([WAIT_FOR_REVIEW] if loop.pr else []),
+                    *EXECUTOR_TOOLS,
+                ]
             ),
         ]
         system = SYSTEM.format(
@@ -497,9 +521,23 @@ class LoopAgent:
             self._wait(item, replace(loop, rev=rev, wait=GO_AHEAD, note=""), None)
         elif outcome.kind == "wait" and outcome.tool == "ask_human":
             question = str(args.get("question", "")).strip()
-            self._wait(item, replace(loop, wait=ANSWER, note=question), question)
+            self._answer_where_asked(loop, question)
+            self._wait(item, replace(loop, wait=ANSWER, note=question), None)
+        elif outcome.kind == "wait" and outcome.tool == "wait_for_review":
+            note = str(args.get("note", "")).strip() or f"waiting on PR #{loop.pr}"
+            if loop.pr:
+                self._answer_where_asked(loop, note)
+                self._wait(item, replace(loop, wait=MERGE, note=f"PR #{loop.pr}"), None)
+            else:
+                say = f"{note} (there's no open PR, so I need you to tell me what next)"
+                self._wait(item, replace(loop, wait=ANSWER, note=say), say)
         elif outcome.kind == "wait" and outcome.tool == "finish_milestone":
-            self._finish(item, loop, str(args.get("summary", "")).strip())
+            self._finish(
+                item,
+                loop,
+                str(args.get("summary", "")).strip(),
+                str(args.get("title", "")).strip(),
+            )
         elif outcome.kind == "wait" and outcome.tool == "close_item":
             summary = str(args.get("summary", "")).strip()
             self._transport.post(f"Done: {summary}", thread=loop.thread)
@@ -538,7 +576,30 @@ class LoopAgent:
         )
         return f"started milestone {number} on branch {branch}", True
 
-    def _finish(self, item: WorkItem, loop: Loop, summary: str) -> None:
+    def _answer_where_asked(self, loop: Loop, text: str) -> None:
+        """D5: answer where the operator asked. A PR question is answered on
+        the PR, quoting the line it was about (an inline thread can't be
+        replied to through the API), with a pointer in the Matrix thread."""
+        if not loop.asked_on_pr:
+            self._transport.post(text, thread=loop.thread)
+            return
+        quoted = [f"> on `{loop.asked_where}`:"] if loop.asked_where else []
+        quoted += [f"> {line}" for line in loop.asked_text.splitlines()]
+        body = "\n".join([*quoted, "", text])
+        try:
+            self._gitea.post_comment(self._repo, loop.asked_on_pr, body)
+        except GiteaError:
+            self._transport.post(text, thread=loop.thread)  # the PR is unreachable
+            return
+        url = loop.pr_url or f"PR #{loop.asked_on_pr}"
+        self._transport.post(
+            f"I answered your comment on PR #{loop.asked_on_pr}: {url}",
+            thread=loop.thread,
+        )
+
+    def _finish(
+        self, item: WorkItem, loop: Loop, summary: str, title: str = ""
+    ) -> None:
         """finish_milestone: publish the milestone, or push a review fix."""
         if self._milestones is None:
             note = "milestones aren't available here"
@@ -591,7 +652,7 @@ class LoopAgent:
         published = self._milestones.publish(
             item_id=item.id,
             branch=loop.branch,
-            title=f"{item.title} (milestone {loop.milestone})",
+            title=title or f"{item.title} (milestone {loop.milestone})",
             summary=summary,
             issue=loop.issue,
         )

nisse-dev/src/nisse_dev/loop/items.py

@@ -37,6 +37,12 @@ class Loop:
     comments: int = 0  # the last PR conversation comment taken (L8)
     notes: tuple[str, ...] = ()  # review notes taken (#69)
     fixes: int = 0  # review fixes pushed, for follow-up branch names
+    # Where the operator last spoke (D5: answer where asked): "" for the
+    # Matrix thread, or the PR it came from, with the inline location and a
+    # little of what they said, for quoting.
+    asked_on_pr: int = 0
+    asked_where: str = ""
+    asked_text: str = ""
 
     def to_data(self) -> dict[str, Any]:
         return asdict(self)

nisse-dev/src/nisse_dev/loop/tools.py

@@ -55,9 +55,19 @@ FINISH_MILESTONE = _tool(
     "your fix to the PR. Ends your turn; you continue when the PR is "
     "reviewed or merged.",
     summary="What the milestone did and how it was verified, for the PR.",
+    title="A short title for the PR (ignored when pushing a review fix).",
+)
+WAIT_FOR_REVIEW = _tool(
+    "wait_for_review",
+    "Nothing more to do until the operator reviews or merges the PR (you "
+    "can't merge it yourself): say so and wait. You continue when there's a "
+    "new comment, or when it merges or closes.",
+    note="What you're waiting on, for the operator.",
 )
 
-WAITING = frozenset({"restate", "ask_human", "close_item", "finish_milestone"})
+WAITING = frozenset(
+    {"restate", "ask_human", "close_item", "finish_milestone", "wait_for_review"}
+)
 
 
 def names(tools: list[dict[str, Any]]) -> set[str]:

nisse-dev/tests/test_loop_milestones.py

@@ -80,7 +80,13 @@ class Forge:
 
     def open_pull_request(self, repo: str, *, head, base, title, body) -> PullRequest:
         n = 100 + len(self.prs)
-        self.prs[n] = {"head": head, "state": "open", "merged": False, "body": body}
+        self.prs[n] = {
+            "head": head,
+            "state": "open",
+            "merged": False,
+            "body": body,
+            "title": title,
+        }
         return self._pr(n)
 
     def pull_request(self, repo: str, index: int) -> PullRequest:
@@ -476,3 +482,89 @@ def test_abandoning_an_item_closes_its_open_pr(world: dict[str, Any]) -> None:
     assert item(world).outcome == "abandoned"
     assert forge.prs[pr]["state"] == "closed"
     assert (pr, "abandoned: abandoned by @don:test") in forge.posted
+
+
+# --- answering where asked (D5), waiting for review, PR titles -----------------
+
+
+def test_a_question_on_the_pr_is_answered_on_the_pr(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        WORK + FINISH,
+        [call("ask_human", question="bool is a subclass of int; keep the check?")],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [pr] = forge.prs
+    forge.notes[pr] = [ReviewNote("line-9", "don", "Is bool an int?", "text.py:9")]
+    agent.watch()
+    agent._schedule()
+    number, body = forge.posted[-1]
+    assert number == pr
+    assert body.startswith("> on `text.py:9`:\n> Is bool an int?\n\n")
+    assert body.endswith("keep the check?")
+    # Matrix gets a pointer, not the conversation.
+    assert (
+        transport.outbound[-1].text
+        == f"I answered your comment on PR #{pr}: https://g/pr/{pr}"
+    )
+    assert transport.outbound[-1].thread == loop_of(item(world)).thread
+
+
+def test_after_a_matrix_reply_questions_go_back_to_matrix(
+    world: dict[str, Any],
+) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        WORK + FINISH,
+        [call("ask_human", question="first?")],
+        [call("ask_human", question="second?")],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [pr] = forge.prs
+    forge.comments[pr] = [Comment(3, "don", "why this?", "t")]
+    agent.watch()
+    agent._schedule()
+    posted_on_pr = len(forge.posted)
+    say(transport, world, "answering in Matrix")  # the operator moved
+    assert len(forge.posted) == posted_on_pr
+    assert transport.outbound[-1].text == "second?"
+
+
+def test_wait_for_review_waits_on_the_merge_then_closes(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        WORK + FINISH,
+        [call("wait_for_review", note="no change needed; ready to merge")],
+        [call("close_item", summary="merged")],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [pr] = forge.prs
+    forge.comments[pr] = [Comment(4, "don", "looks fine, merge it?", "t")]
+    agent.watch()
+    agent._schedule()
+    loop = loop_of(item(world))
+    assert (loop.wait, loop.note) == (MERGE, f"PR #{pr}")
+    assert forge.posted[-1][1].endswith("no change needed; ready to merge")
+    forge.merge(pr)
+    agent.watch()
+    agent._schedule()
+    assert item(world).status == "closed"
+
+
+def test_the_driver_titles_its_pr(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    finish = [call("finish_milestone", summary="s", title="Add count_words")]
+    agent, transport = loop_agent(world, ScriptedModel(RESTATE, WORK + finish))
+    agent.intake()
+    say(transport, world, "go")
+    [pr] = forge.prs
+    assert forge.prs[pr]["title"] == "Add count_words"

nisse-dev/tests/test_loop_properties.py

@@ -53,6 +53,8 @@ class PolicyModel:
             return [ToolCall(call_id=f"p{self.n}", tool="nonsense", arguments={})]
         if "finish_milestone" not in offered:
             return [call("restate", text=f"restatement {self.n}")]
+        if "wait_for_review" in offered and self.rng.random() < 0.2:
+            return [call("wait_for_review", note=f"waiting {self.n}")]
         merged = any("merged" in t for t in facts[-6:])
         if merged and self.rng.random() < 0.8:
             return [call("close_item", summary=f"done {self.n}")]

tests/integration/test_end_to_end_loop.py

@@ -23,7 +23,7 @@ from e2e_support import base_branch, operator_gitea, task
 
 from nisse_dev.config import AgentConfig
 from nisse_dev.execution import SSHHost
-from nisse_dev.gitea import GiteaError
+from nisse_dev.gitea import GiteaClient, GiteaError
 from nisse_dev.loop.build import RunningLoop, build_loop_agent
 from nisse_dev.loop.items import ANSWER, GO_AHEAD, MERGE, WAITING, loop_of
 from nisse_dev.transport import InMemoryTransport
@@ -93,6 +93,22 @@ def is_artifact(path: str) -> bool:
     )
 
 
+def merge(gitea: GiteaClient, repo: str, pr: int, wait_s: float = 60) -> None:
+    """Merge, waiting out Gitea's "Please try again later": it answers that
+    while it is still checking a freshly pushed PR can merge."""
+    deadline = time.monotonic() + wait_s
+    while True:
+        try:
+            gitea._request(
+                "POST", f"/repos/{repo}/pulls/{pr}/merge", json={"Do": "merge"}
+            )
+            return
+        except GiteaError as exc:
+            if "try again later" not in str(exc) or time.monotonic() > deadline:
+                raise
+            time.sleep(3)
+
+
 class Operator:
     def __init__(self, agent: RunningLoop, transport: InMemoryTransport) -> None:
         self.agent = agent
@@ -191,9 +207,7 @@ def test_the_slice_end_to_end(
                 f"/repos/{repo}/pulls/{pr}/reviews",
                 json={"event": "APPROVED", "body": "looks good"},
             )
-        as_operator._request(
+        merge(as_operator, repo, pr)
-            "POST", f"/repos/{repo}/pulls/{pr}/merge", json={"Do": "merge"}
-        )
         operator.tick()
         deadline = time.monotonic() + DEADLINE_S
         nudges = 0