Merge pull request 'nisse-review: a review refused for an open point posts once the point is answered' (#168) from review-held-end into main

docs/Development-Plan.md

@@ -683,8 +683,11 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and
   can't be listed, and its earlier points with what was said on their lines.
 - **Replies:** `reply` answers a point where it was made (inline on its line,
   else quoting it), `skip` leaves it with a reason, `done` ends; every point is
-  answered or skipped before a run may end. Each reply carries its point's key,
+  answered or skipped before a run may end. An ending call refused for that is
-  so a run re-done after a crash answers nothing twice.
+  held, and goes ahead as soon as the last point is settled, rather than left
+  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.
 - **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).

nisse-core/src/nisse_core/loop/driver.py

@@ -108,6 +108,7 @@ class Driver:
         on_turn: Callable[[str], None] | None = None,
         screen: Callable[[ToolCall], Screening] | None = None,
         max_turns: int | None = None,
+        due: Callable[[], tuple[str, dict[str, Any], str] | None] | None = None,
     ) -> RunOutcome:
         """``waiting``: the control tools that end the run, for the
         scheduler to act on. ``handlers`` answer those that don't (e.g.
@@ -115,7 +116,10 @@ class Driver:
         ``checks`` vet a waiting tool's arguments: a problem is the call's
         failed result and the run goes on, so the model puts it right.
         ``screen`` judges each execution call before it reaches the tool VM
-        (P4)."""
+        (P4). ``due``, asked after each handled call, names a waiting call
+        the harness held back that may now go ahead (tool, arguments, what
+        to tell the model): the run ends with it, as if the model had made
+        it again."""
         handlers = handlers or {}
         checks = checks or {}
         model = model or self._model  # the work item's pinned driver (L9)
@@ -193,6 +197,23 @@ class Driver:
                         result, ok = handlers[call.tool](dict(call.arguments))
                     except VMUnreachable as exc:
                         return self._unreachable(transcript, calls, index, str(exc))
+                    held = due() if ok and due is not None else None
+                    if held is not None:
+                        tool, arguments, said = held
+                        transcript.append(
+                            ToolResult(
+                                call_id=call.call_id,
+                                result=f"{result}\n\n[harness] {said}; your turn "
+                                "has ended",
+                                success=True,
+                            )
+                        )
+                        skip_calls(
+                            transcript,
+                            calls[index + 1 :],
+                            "not run: the turn had ended",
+                        )
+                        return RunOutcome("wait", tool, arguments)
                 elif call.tool not in offered or workspace is None:
                     result, ok = (
                         f"no such tool: {call.tool}; the tools are "

nisse-core/tests/test_driver.py

@@ -81,3 +81,72 @@ def test_a_tool_not_named_waiting_doesnt_end_the_run(tmp_path: Path) -> None:
     )
     assert ran.tool == "post_review"
     assert results(log)[0].startswith("no such tool: ask_human")
+
+
+NOTE = tool("note", "Note something.", text="The note.")
+
+
+def test_a_held_call_goes_ahead_once_due_says_so(tmp_path: Path) -> None:
+    # post_review is refused while a point is open and held by the agent;
+    # the handled call that settles the point ends the run with it.
+    open_points = ["P1"]
+    held: dict[str, Any] = {}
+
+    def refuse(args: dict[str, Any]) -> str | None:
+        if open_points:
+            held["call"] = ("post_review", args)
+            return "answer P1 first"
+        return None
+
+    def settle(args: dict[str, Any]) -> tuple[str, bool]:
+        open_points.clear()
+        return "answered P1", True
+
+    def due() -> tuple[str, dict[str, Any], str] | None:
+        if "call" in held and not open_points:
+            tool_, args = held.pop("call")
+            return tool_, args, "your held post_review went ahead"
+        return None
+
+    model = Scripted(
+        [call("post_review", body="the summary")],
+        [call("note", text="P1"), call("read_file", path="a.py")],
+        # never reached: the run ended on the note
+        [call("note", text="again")],
+    )
+    log = transcript(tmp_path)
+    reader = Reader()
+    ran = Driver(model, "m").run(
+        log,
+        system="s",
+        tools=[READ, POST, NOTE],
+        waiting={"post_review"},
+        workspace=reader,
+        handlers={"note": settle},
+        checks={"post_review": refuse},
+        due=due,
+    )
+    assert (ran.kind, ran.tool, ran.arguments) == (
+        "wait",
+        "post_review",
+        {"body": "the summary"},
+    )
+    assert results(log) == [
+        "answer P1 first",
+        "answered P1\n\n[harness] your held post_review went ahead; your turn has ended",
+        "not run: the turn had ended",
+    ]
+    assert reader.calls == []  # the read after it never ran
+
+
+def test_without_due_a_settled_point_leaves_the_run_going(tmp_path: Path) -> None:
+    model = Scripted([call("note", text="x")], [call("post_review", body="b")])
+    ran = Driver(model, "m").run(
+        transcript(tmp_path),
+        system="s",
+        tools=[POST, NOTE],
+        waiting={"post_review"},
+        workspace=None,
+        handlers={"note": lambda args: ("noted", True)},
+    )
+    assert (ran.tool, ran.arguments) == ("post_review", {"body": "b"})

nisse-review/src/nisse_review/review.py

@@ -18,7 +18,7 @@ from nisse_core.knowledge.handlers import Knowledge
 from nisse_core.log.canonical import Fact
 from nisse_core.loop.driver import Driver, RunOutcome
 from nisse_core.loop.expert import BudgetExhausted, ExpertDesk
-from nisse_core.loop.tools import ASK_EXPERT, tool
+from nisse_core.loop.tools import ASK_EXPERT, names, tool
 from nisse_core.models.llm import LLMError
 from nisse_core.oplog import OpLog
 from nisse_core.state.store import Transcript
@@ -168,6 +168,10 @@ class Draft:
     points: dict[str, Said] = field(default_factory=dict)  # P1 -> the point
     replied: list[str] = field(default_factory=list)  # keys answered
     skipped: list[str] = field(default_factory=list)  # keys left, with reason
+    # An ending call refused only because points were open: it goes ahead
+    # once the last is answered or skipped, not left to the model to repeat.
+    held: tuple[str, dict[str, Any]] | None = None
+    ends: tuple[str, ...] = ()  # the run's ending tools, for the messages
 
     def open_points(self) -> list[str]:
         settled = {*self.replied, *self.skipped}
@@ -283,19 +287,53 @@ class Reviewer:
         elif kind == LATER:
             tools.append(STILL_HAPPY)
 
-        def answered_first(args: dict[str, Any]) -> str | None:
+        draft.ends = tuple(t for t in names(tools) if t in WAITING)
-            left = draft.open_points()
-            if left:
-                return "first answer or skip each point said to you: " + ", ".join(left)
-            return None
 
-        def no_comments(args: dict[str, Any]) -> str | None:
+        def answered_first(tool: str) -> Callable[[dict[str, Any]], str | None]:
-            if draft.comments:
+            def check(args: dict[str, Any]) -> str | None:
+                left = draft.open_points()
+                if not left:
+                    return None
+                draft.held = (tool, args)
                 return (
-                    f"you drafted {len(draft.comments)} comment(s): post them "
+                    "first answer or skip each point said to you: "
-                    "with post_review"
+                    + ", ".join(left)
+                    + f". Your {tool} is held: it goes ahead once the last is "
+                    "answered or skipped"
                 )
-            return answered_first(args)
+
+            return check
+
+        def no_comments(tool: str) -> Callable[[dict[str, Any]], str | None]:
+            def check(args: dict[str, Any]) -> str | None:
+                if draft.comments:
+                    return (
+                        f"you drafted {len(draft.comments)} comment(s): post them "
+                        "with post_review"
+                    )
+                return answered_first(tool)(args)
+
+            return check
+
+        checks = {
+            "post_review": answered_first("post_review"),
+            "sign_off": no_comments("sign_off"),
+            "still_happy": no_comments("still_happy"),
+            "done": answered_first("done"),
+        }
+
+        def due() -> tuple[str, dict[str, Any], str] | None:
+            if draft.held is None or draft.open_points():
+                return None
+            tool, args = draft.held
+            draft.held = None
+            if checks[tool](args) is not None:
+                return None  # wrong for another reason now: the model's to fix
+            return (
+                tool,
+                args,
+                f"that was the last open point, so your held {tool} went ahead",
+            )
 
         outcome = self._driver.run(
             transcript,
@@ -304,13 +342,9 @@ class Reviewer:
             waiting=WAITING,
             workspace=repo,
             handlers=handlers,
-            checks={
+            checks=checks,
-                "post_review": answered_first,
-                "sign_off": no_comments,
-                "still_happy": no_comments,
-                "done": answered_first,
-            },
             max_turns=self._max_turns,
+            due=due,
         )
         return outcome, draft
 
@@ -535,8 +569,14 @@ class Reviewer:
             wanted = f"P{wanted}"
         if wanted in draft.open_points():
             return wanted, draft.points[wanted]
-        left = ", ".join(draft.open_points()) or "none"
+        left = draft.open_points()
-        return f"no open point {wanted or '(none given)'}; open: {left}"
+        if not left:
+            return (
+                f"no open point {wanted or '(none given)'}: every point said to "
+                "you is answered or skipped. End your run with "
+                + " or ".join(draft.ends or ("your ending tool",))
+            )
+        return f"no open point {wanted or '(none given)'}; open: {', '.join(left)}"
 
     def _comment(
         self, repo: Repository, draft: Draft, args: dict[str, Any]

nisse-review/tests/test_review_rounds.py

@@ -409,3 +409,69 @@ def test_review_with_a_comment_beside_it_is_one_fresh_review(tmp_path: Path) ->
     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.")],
+        *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.")],
+        [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.")],
+        [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]