nisse-review: a review refused for an open point posts once the point is answered

On #166 the reviewer called post_review with two points unanswered, was
refused, answered them, then only skipped already-answered points until the
40-turn cap: the review went out as partial, its summary lost. A refused
ending call is now held, and the driver ends the run with it as soon as the
last point is settled (a new due hook in nisse-core's driver; nisse-dev
doesn't use it). A held call that is wrong for another reason by then
(sign_off over drafted comments) is left to the model. With nothing open,
reply and skip say which tool ends the run.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

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:
-            left = draft.open_points()
-            if left:
-                return "first answer or skip each point said to you: " + ", ".join(left)
-            return None
+        draft.ends = tuple(t for t in names(tools) if t in WAITING)
 
-        def no_comments(args: dict[str, Any]) -> str | None:
-            if draft.comments:
+        def answered_first(tool: str) -> Callable[[dict[str, Any]], str | None]:
+            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 "
-                    "with post_review"
+                    "first answer or skip each point said to you: "
+                    + ", ".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={
-                "post_review": answered_first,
-                "sign_off": no_comments,
-                "still_happy": no_comments,
-                "done": answered_first,
-            },
+            checks=checks,
             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"
-        return f"no open point {wanted or '(none given)'}; open: {left}"
+        left = draft.open_points()
+        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]