diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index 4664ae3..5b00a9f 100644 --- a/docs/Development-Plan.md +++ b/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, - so a run re-done after a crash answers nothing twice. + answered or skipped before a run may end. An ending call refused for that is + 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). diff --git a/nisse-core/src/nisse_core/loop/driver.py b/nisse-core/src/nisse_core/loop/driver.py index da54c83..374b9c2 100644 --- a/nisse-core/src/nisse_core/loop/driver.py +++ b/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 " diff --git a/nisse-core/tests/test_driver.py b/nisse-core/tests/test_driver.py index 37b1a8e..e381699 100644 --- a/nisse-core/tests/test_driver.py +++ b/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"}) diff --git a/nisse-review/src/nisse_review/review.py b/nisse-review/src/nisse_review/review.py index 4afcb1b..5762acd 100644 --- a/nisse-review/src/nisse_review/review.py +++ 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: - 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] diff --git a/nisse-review/tests/test_review_rounds.py b/nisse-review/tests/test_review_rounds.py index 513a9fc..4548c6f 100644 --- a/nisse-review/tests/test_review_rounds.py +++ b/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]