From 651551aab47ed624ff4658c1a3e9cb928dd8fc69 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Sat, 3 Oct 2026 17:29:28 +0000 Subject: [PATCH] Answer where asked, wait on a merge, and let the driver title PRs From the new loop's first live run (sandbox #255, PR #256): - A question asked on the PR is answered on the PR (an inline one quoted with its file and line, since the API can't reply in its thread), with a pointer in the item's Matrix thread; one asked in Matrix stays there. - wait_for_review: when a review needs no change, or only the operator's merge is left, the item waits on the merge instead of on an answer. - finish_milestone takes the PR's title from the driver. The e2e test's merge waits out Gitea's brief 'try again later' after a push. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE --- nisse-dev/src/nisse_dev/loop/agent.py | 75 ++++++++++++++++-- nisse-dev/src/nisse_dev/loop/items.py | 6 ++ nisse-dev/src/nisse_dev/loop/tools.py | 12 ++- nisse-dev/tests/test_loop_milestones.py | 94 ++++++++++++++++++++++- nisse-dev/tests/test_loop_properties.py | 2 + tests/integration/test_end_to_end_loop.py | 22 +++++- 6 files changed, 198 insertions(+), 13 deletions(-) diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index 0a3f633..02530b9 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/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, ) diff --git a/nisse-dev/src/nisse_dev/loop/items.py b/nisse-dev/src/nisse_dev/loop/items.py index 1be3c32..d902a48 100644 --- a/nisse-dev/src/nisse_dev/loop/items.py +++ b/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) diff --git a/nisse-dev/src/nisse_dev/loop/tools.py b/nisse-dev/src/nisse_dev/loop/tools.py index 087f7df..79f26b8 100644 --- a/nisse-dev/src/nisse_dev/loop/tools.py +++ b/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]: diff --git a/nisse-dev/tests/test_loop_milestones.py b/nisse-dev/tests/test_loop_milestones.py index 9a77b22..f2030be 100644 --- a/nisse-dev/tests/test_loop_milestones.py +++ b/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" diff --git a/nisse-dev/tests/test_loop_properties.py b/nisse-dev/tests/test_loop_properties.py index bbcd35f..bf8c227 100644 --- a/nisse-dev/tests/test_loop_properties.py +++ b/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}")] diff --git a/tests/integration/test_end_to_end_loop.py b/tests/integration/test_end_to_end_loop.py index 7524cf7..d8d9a01 100644 --- a/tests/integration/test_end_to_end_loop.py +++ b/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( - "POST", f"/repos/{repo}/pulls/{pr}/merge", json={"Do": "merge"} - ) + merge(as_operator, repo, pr) operator.tick() deadline = time.monotonic() + DEADLINE_S nudges = 0