diff --git a/nisse-dev/src/nisse_dev/approvals.py b/nisse-dev/src/nisse_dev/approvals.py new file mode 100644 index 0000000..5b5edf1 --- /dev/null +++ b/nisse-dev/src/nisse_dev/approvals.py @@ -0,0 +1,80 @@ +"""Plan externalisation and Gitea-backed approvals (spec A4, Part H). + +Plans are posted as comments on a per-project Gitea issue. The approval +comment — its author, timestamp, and revision tag — is the durable off-box +record, authenticated by Gitea's own auth. The state store keeps only a +pointer to it, so boot reconciliation can re-derive gate state from Gitea +even after the state volume is lost: external truth wins. + +Approvals arriving here bind to a revision exactly as they do over transport +(A4). The first valid approval on any channel wins; the orchestrator echoes +the outcome to the other channel so the records converge. +""" + +from dataclasses import dataclass + +from nisse_dev.gitea import Comment, GiteaClient, GiteaError + +APPROVAL_HINT = ( + "Reply `approve {tag}` to approve this revision, or comment with feedback " + "to reject it." +) + + +@dataclass(frozen=True) +class PlanPost: + issue: int + comment_id: int + + +class PlanBoard: + """The Gitea side of plan gating: one issue per project, one comment per + plan revision.""" + + def __init__(self, client: GiteaClient, repo: str) -> None: + self._client = client + self._repo = repo + + @property + def repo(self) -> str: + return self._repo + + def open_project_issue(self, project: str, task_id: str) -> int: + return self._client.create_issue( + self._repo, + title=f"nisse-dev: {project}", + body=( + f"Plan gate for project `{project}` (task `{task_id}`).\n\n" + "Each plan revision is posted here as a comment. Approving a " + "revision by comment is equivalent to approving it in Matrix; " + "the first valid approval on either channel wins." + ), + ) + + def post_plan(self, issue: int, gate: str, rev: int, plan: str) -> PlanPost: + tag = f"rev-{rev}" + body = "\n".join( + [ + f"## Plan for `{gate}` ({tag})", + "", + plan, + "", + "---", + APPROVAL_HINT.format(tag=tag), + ] + ) + comment = self._client.post_comment(self._repo, issue, body) + return PlanPost(issue=issue, comment_id=comment.id) + + def echo(self, issue: int, text: str) -> None: + """Mirror an outcome decided on another channel, so the Gitea record + does not silently disagree with Matrix.""" + try: + self._client.post_comment(self._repo, issue, text) + except GiteaError: + pass + + def comments_since(self, issue: int, after_id: int) -> list[Comment]: + return [ + c for c in self._client.list_comments(self._repo, issue) if c.id > after_id + ] diff --git a/nisse-dev/src/nisse_dev/control.py b/nisse-dev/src/nisse_dev/control.py index c33b884..ff9ffd0 100644 --- a/nisse-dev/src/nisse_dev/control.py +++ b/nisse-dev/src/nisse_dev/control.py @@ -94,6 +94,17 @@ class MilestonePublisher(Protocol): def comment(self, index: int, body: str) -> None: ... +class PlanBoard(Protocol): + """The Gitea side of plan gating; the concrete one is in + nisse_dev.approvals.""" + + def open_project_issue(self, project: str, task_id: str) -> int: ... + + def post_plan(self, issue: int, gate: str, rev: int, plan: str) -> Any: ... + + def echo(self, issue: int, text: str) -> None: ... + + class PlanningAgent(Protocol): def plan( self, @@ -133,6 +144,10 @@ def _fresh_data() -> dict[str, Any]: "task_id": None, "roles": None, # A11 mapping, pinned at task start "pr": None, # the open PR for the current milestone, once published + # Pointer to the Gitea plan record (Part H): the state store holds + # only this, so gate state can be re-derived from Gitea after a + # state-volume loss. + "board": None, } @@ -147,6 +162,7 @@ class Orchestrator: executor: "MilestoneExecutor | None" = None, publisher: "MilestonePublisher | None" = None, gitea_operators: Iterable[str] = (), + board: "PlanBoard | None" = None, ) -> None: self._store = store self._transport = transport @@ -159,6 +175,7 @@ class Orchestrator: # are different names for the same person, and neither implies # the other. self._gitea_operators = set(gitea_operators) + self._board = board # Transport and the comment poller deliver on separate threads; # state transitions must not interleave. self._lock = threading.RLock() @@ -260,6 +277,19 @@ class Orchestrator: self._data = {**_fresh_data(), "task_id": task_id, "roles": pinned} self._project = project self._store.oplog.task_id = task_id + if self._board is not None: + try: + self._data["board"] = { + "issue": self._board.open_project_issue(project, task_id), + "last_comment": 0, + } + except GiteaError as exc: + # Gitea being down must not block planning; Matrix remains a + # complete approval channel on its own. + self._transport.send_message( + f"note: could not open the Gitea plan issue ({exc}); " + "approvals will be Matrix-only for this task" + ) self._transition(PLANNING_ARCH, cause=f"operator command: use {project}") self._run_planning_turn("arch") @@ -308,6 +338,7 @@ class Orchestrator: f"{action.content}\n\nreply 'approve' to approve rev-{rev}; " "any other reply is rejection feedback" ) + self._post_plan_to_board(gate_name, rev, action.content) self._store.oplog.emit( "gate.open", cause=f"plan submitted (rev-{rev})", @@ -317,6 +348,12 @@ class Orchestrator: ) def _handle_gate_reply(self, msg: InboundMessage, text: str) -> None: + self._apply_gate_reply(sender=msg.sender, text=text, channel="transport") + + def _apply_gate_reply(self, *, sender: str, text: str, channel: str) -> None: + """Shared by every approval source (A4). The allowlist is per channel: + a Matrix MXID and a Gitea login are different names, and holding one + says nothing about holding the other.""" gate_data = self._data["gate"] name, rev = gate_data["name"], gate_data["rev"] milestone = self._data["milestone"] @@ -332,9 +369,8 @@ class Orchestrator: ) return revision = PlanRevision(gate=name, rev=rev, content=gate_data["plan"]) - result = Gate(revision, self._operators).evaluate( - msg.sender, text, channel="transport" - ) + allowlist = self._operators if channel == "transport" else self._gitea_operators + result = Gate(revision, allowlist).evaluate(sender, text, channel=channel) if isinstance(result, Ignored): if result.notice: self._transport.send_message(result.notice) @@ -347,12 +383,19 @@ class Orchestrator: rev=f"rev-{rev}", channel=result.channel, ) + self._echo_outcome( + channel, + f"{name} rev-{rev} rejected by {result.approver}: {result.feedback}", + ) self._data["gate"] = None back = PLANNING_ARCH if name == "arch" else MILESTONE_PLANNING self._transition(back, cause=f"plan rejected ({name} rev-{rev})") self._run_planning_turn(name, feedback=result.feedback) return assert isinstance(result, Approved) + self._echo_outcome( + channel, f"{name} rev-{rev} approved by {result.approver} via {channel}" + ) self._store.oplog.emit( "gate.approve", cause=f"approved by {result.approver}", @@ -395,6 +438,52 @@ class Orchestrator: pending={"kind": "milestone_stopped", "milestone": milestone}, ) + def _echo_outcome(self, deciding_channel: str, text: str) -> None: + """A gate satisfied on one channel is echoed to the other, so neither + record silently disagrees about what was approved (A4).""" + board_state = self._data.get("board") + if deciding_channel != "gitea" and self._board is not None and board_state: + self._board.echo(board_state["issue"], text) + elif deciding_channel == "gitea": + self._transport.send_message(text) + + def _post_plan_to_board(self, gate_name: str, rev: int, plan: str) -> None: + board_state = self._data.get("board") + if self._board is None or not board_state: + return + try: + post = self._board.post_plan(board_state["issue"], gate_name, rev, plan) + except GiteaError: + # Matrix already carries the plan; a Gitea outage degrades the + # durability of the record, not the ability to approve. + return + board_state["comment_id"] = post.comment_id + + def handle_plan_comment(self, comment: Any) -> None: + """A comment on the project issue may satisfy an open plan gate (A4). + + Approvals bind to a revision here exactly as over transport, and the + first valid approval on either channel wins. + """ + with self._lock: + self._handle_plan_comment_locked(comment) + + def _handle_plan_comment_locked(self, comment: Any) -> None: + if comment.author not in self._gitea_operators: + return + state = self._store.get_state() + if state is None or state.state not in ( + AWAIT_ARCH_SIGNOFF, + AWAIT_MILESTONE_SIGNOFF, + ): + return + self._store.append_event( + UserMessage(text=comment.body, source=f"gitea:comment:{comment.id}") + ) + self._apply_gate_reply( + sender=comment.author, text=comment.body.strip(), channel="gitea" + ) + def handle_review_comment(self, comment: Any) -> None: """React to a PR review comment (B2). Called from the poller thread. diff --git a/nisse-dev/src/nisse_dev/executor.py b/nisse-dev/src/nisse_dev/executor.py index 28c78ab..c622282 100644 --- a/nisse-dev/src/nisse_dev/executor.py +++ b/nisse-dev/src/nisse_dev/executor.py @@ -30,6 +30,15 @@ plain-text summary and no tool call — that ends the milestone.""" DEFAULT_MAX_TURNS = 24 DEFAULT_FAILURE_LIMIT = 2 +# A turn with no tool call means "the plan is done". Believing that before +# any tool has been called would record an empty milestone as a success, so +# the claim is challenged once first. +_NUDGE = ( + "\n\nYou have not made any change yet, so the plan cannot be complete. " + "Carry out the next step of the plan with a tool call, or run the tests " + "to show the work is already done." +) + @dataclass(frozen=True) class ExecutorOutcome: @@ -59,6 +68,8 @@ class LLMExecutor: model = self._roles.resolve("executor") system = f"{EXECUTOR_SYSTEM}\n\nYou are executing milestone {milestone}." consecutive_failures = 0 + did_work = False + challenged = False for _turn in range(self._max_turns): try: @@ -78,9 +89,23 @@ class LLMExecutor: calls.append(event) if not calls: - return ExecutorOutcome("completed", "executor reported the plan done") + if did_work: + return ExecutorOutcome( + "completed", "executor reported the plan done" + ) + if not challenged: + # Claiming completion without having touched anything is + # not evidence of completion; ask once before believing it. + challenged = True + system += _NUDGE + continue + return ExecutorOutcome( + "escalate", + "executor claimed the milestone was done without making any change", + ) for call in calls: + did_work = True outcome = self._workspace.dispatch(call.tool, call.arguments) self._store.append_event( ToolResult( diff --git a/nisse-dev/src/nisse_dev/gitea.py b/nisse-dev/src/nisse_dev/gitea.py index 80ab20f..d5fb62b 100644 --- a/nisse-dev/src/nisse_dev/gitea.py +++ b/nisse-dev/src/nisse_dev/gitea.py @@ -151,6 +151,12 @@ class GiteaClient: created_at=data.get("created_at", ""), ) + def create_issue(self, repo: str, title: str, body: str) -> int: + data = self._request( + "POST", f"/repos/{repo}/issues", json={"title": title, "body": body} + ) + return int(data["number"]) + def close_pull_request(self, repo: str, index: int) -> None: self._request("PATCH", f"/repos/{repo}/pulls/{index}", json={"state": "closed"}) diff --git a/nisse-dev/tests/test_approvals.py b/nisse-dev/tests/test_approvals.py new file mode 100644 index 0000000..0aa5032 --- /dev/null +++ b/nisse-dev/tests/test_approvals.py @@ -0,0 +1,210 @@ +"""Plan externalisation to Gitea and approvals arriving on either channel +(spec A4, Part H).""" + +import io +from pathlib import Path +from typing import Any + +from nisse_dev.approvals import PlanBoard +from nisse_dev.canonical import CanonicalEvent, ToolCall +from nisse_dev.control import ( + AWAIT_ARCH_SIGNOFF, + AWAIT_MILESTONE_SIGNOFF, + MILESTONE_PLANNING, + Orchestrator, +) +from nisse_dev.gitea import Comment, GiteaError +from nisse_dev.llm import RoleMap +from nisse_dev.oplog import OpLog +from nisse_dev.planner import LLMPlanningAgent +from nisse_dev.state import StateStore +from nisse_dev.transport import InMemoryTransport + +ROLES = RoleMap(executor="x", planner="y", delegate="y", consultant="y") + + +class FakeGitea: + def __init__(self, *, fail: bool = False) -> None: + self.issues: list[dict[str, str]] = [] + self.comments: list[tuple[int, str]] = [] + self._fail = fail + self._next_id = 100 + + def create_issue(self, repo: str, title: str, body: str) -> int: + if self._fail: + raise GiteaError("gitea is down") + self.issues.append({"title": title, "body": body}) + return len(self.issues) + + def post_comment(self, repo: str, index: int, body: str) -> Comment: + if self._fail: + raise GiteaError("gitea is down") + self.comments.append((index, body)) + self._next_id += 1 + return Comment( + id=self._next_id, author="nisse-dev-testing", body=body, created_at="t" + ) + + def list_comments(self, repo: str, index: int, since: str | None = None): + return [] + + +class ScriptedClient: + def __init__(self) -> None: + self.calls = 0 + + def complete(self, events, tools, model, system=None) -> list[CanonicalEvent]: + self.calls += 1 + return [ + ToolCall( + call_id=f"c{self.calls}", + tool="submit_plan", + arguments={"type": "x", "content": f"plan {self.calls}"}, + ) + ] + + +def build( + tmp_path: Path, gitea: Any +) -> tuple[Orchestrator, StateStore, InMemoryTransport, PlanBoard]: + store = StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO())) + transport = InMemoryTransport() + board = PlanBoard(gitea, "o/sandbox") + orch = Orchestrator( + store, + transport, + LLMPlanningAgent(ScriptedClient(), store, ROLES), + operators={"don"}, + roles=ROLES, + gitea_operators={"dongar"}, + board=board, + ) + orch.start() + return orch, store, transport, board + + +def comment(body: str, author: str = "dongar", cid: int = 900) -> Comment: + return Comment(id=cid, author=author, body=body, created_at="t") + + +def test_project_start_opens_an_issue_and_posts_the_plan(tmp_path: Path) -> None: + gitea = FakeGitea() + _, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + + assert gitea.issues[0]["title"] == "nisse-dev: demo" + issue, body = gitea.comments[0] + assert issue == 1 + assert "plan 1" in body + assert "approve rev-1" in body # the tag is printed for copy-paste (A4) + # The store keeps a pointer, not the plan record itself (Part H). + assert store.get_state().data["board"]["issue"] == 1 + + +def test_approval_by_gitea_comment_satisfies_the_gate(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, store, _, _ = build(tmp_path, gitea) + orch._transport.receive("don", "use demo") + assert store.get_state().state == AWAIT_ARCH_SIGNOFF + + orch.handle_plan_comment(comment("approve rev-1")) + + assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF + + +def test_stale_revision_from_gitea_is_refused(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + transport.receive("don", "too vague") # now at rev-2 + + orch.handle_plan_comment(comment("approve rev-1")) + assert store.get_state().state == AWAIT_ARCH_SIGNOFF # not advanced + + orch.handle_plan_comment(comment("approve rev-2", cid=901)) + assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF + + +def test_non_operator_gitea_comment_cannot_approve(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + + orch.handle_plan_comment(comment("approve rev-1", author="drive-by")) + assert store.get_state().state == AWAIT_ARCH_SIGNOFF + + +def test_agents_own_comment_cannot_approve_its_plan(tmp_path: Path) -> None: + # The agent posts plans as nisse-dev-testing; that login is not on the + # operator allowlist, so its own comments can never satisfy its gate (A4). + gitea = FakeGitea() + orch, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + + orch.handle_plan_comment(comment("approve rev-1", author="nisse-dev-testing")) + assert store.get_state().state == AWAIT_ARCH_SIGNOFF + + +def test_rejection_by_gitea_comment_replans(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + + orch.handle_plan_comment(comment("needs more detail on the schema")) + + assert store.get_state().state == AWAIT_ARCH_SIGNOFF + assert "plan 2" in transport.outbound[-1].text # replanned + + +def test_matrix_approval_is_echoed_to_gitea(tmp_path: Path) -> None: + gitea = FakeGitea() + _, _, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + transport.receive("don", "approve") + + echoes = [body for _, body in gitea.comments if "approved by don" in body] + assert echoes, "the Gitea record must not silently disagree with Matrix" + + +def test_gitea_approval_is_echoed_to_matrix(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, _, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + orch.handle_plan_comment(comment("approve rev-1")) + + assert any("approved by dongar via gitea" in m.text for m in transport.outbound) + + +def test_gitea_outage_leaves_matrix_a_complete_channel(tmp_path: Path) -> None: + gitea = FakeGitea(fail=True) + _, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + + # Planning proceeded and the gate is usable despite Gitea being down. + assert store.get_state().state == AWAIT_ARCH_SIGNOFF + assert any("Matrix-only" in m.text for m in transport.outbound) + transport.receive("don", "approve") + assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF + + +def test_comments_are_ignored_outside_a_gate(tmp_path: Path) -> None: + gitea = FakeGitea() + orch, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + transport.receive("don", "approve") # arch approved; now at milestone gate + transport.receive("don", "approve") # milestone approved, awaiting start + before = store.get_state().state + + orch.handle_plan_comment(comment("approve rev-1", cid=902)) + assert store.get_state().state == before + + +def test_board_records_the_milestone_plan_too(tmp_path: Path) -> None: + gitea = FakeGitea() + _, store, transport, _ = build(tmp_path, gitea) + transport.receive("don", "use demo") + transport.receive("don", "approve") + + assert store.get_state().state in (MILESTONE_PLANNING, AWAIT_MILESTONE_SIGNOFF) + posted = [body for _, body in gitea.comments if "milestone-1" in body] + assert posted, "the milestone plan must be externalised as well" diff --git a/nisse-dev/tests/test_executor.py b/nisse-dev/tests/test_executor.py index 70b43f4..409160f 100644 --- a/nisse-dev/tests/test_executor.py +++ b/nisse-dev/tests/test_executor.py @@ -88,11 +88,46 @@ def test_uses_executor_role_and_reduced_manifest( assert "milestone 2" in client.calls[0]["system"] -def test_no_tool_call_ends_the_milestone(store: StateStore, ws: Workspace) -> None: - client = ScriptedClient([[AssistantText(text="all steps done", model="m")]]) +def test_no_tool_call_after_work_ends_the_milestone( + store: StateStore, ws: Workspace +) -> None: + client = ScriptedClient( + [ + [call("write_file", path="a.py", content="x")], + [AssistantText(text="all steps done", model="m")], + ] + ) assert build(client, store, ws).run(milestone=1).status == "completed" +def test_completion_claimed_without_any_work_is_challenged( + store: StateStore, ws: Workspace +) -> None: + # A model that answers with prose on turn one has done nothing; believing + # it would record an empty milestone as a success. + client = ScriptedClient( + [ + [AssistantText(text="looks done to me", model="m")], + [call("write_file", path="a.py", content="x")], + [AssistantText(text="now it is done", model="m")], + ] + ) + outcome = build(client, store, ws).run(milestone=1) + assert outcome.status == "completed" + assert (ws.root / "a.py").exists() # the nudge produced actual work + assert "not made any change yet" in client.calls[1]["system"] + + +def test_persisting_in_an_empty_claim_escalates( + store: StateStore, ws: Workspace +) -> None: + prose = [AssistantText(text="nothing to do", model="m")] + client = ScriptedClient([prose, prose]) + outcome = build(client, store, ws).run(milestone=1) + assert outcome.status == "escalate" + assert "without making any change" in outcome.note + + def test_two_consecutive_command_failures_escalate( store: StateStore, ws: Workspace ) -> None: diff --git a/nisse-dev/tests/test_tools.py b/nisse-dev/tests/test_tools.py index 5e66c4f..6827c6b 100644 --- a/nisse-dev/tests/test_tools.py +++ b/nisse-dev/tests/test_tools.py @@ -1,4 +1,5 @@ import os +import time from pathlib import Path import pytest @@ -97,6 +98,12 @@ def test_timeout_kills_the_whole_process_group(tmp_path: Path) -> None: assert pidfile.exists() pid = int(pidfile.read_text().strip()) assert _is_running(os.getpid()), "liveness check is broken, not the kill" + # SIGKILL to a process group is asynchronous, and the grandchild is an + # orphan by now, so its exit is not synchronised with our return. Wait + # for death rather than assuming it is instantaneous. + deadline = time.monotonic() + 5 + while _is_running(pid) and time.monotonic() < deadline: + time.sleep(0.05) assert not _is_running(pid) diff --git a/tests/integration/test_gitea_live.py b/tests/integration/test_gitea_live.py index f13dbf0..c1f3d0f 100644 --- a/tests/integration/test_gitea_live.py +++ b/tests/integration/test_gitea_live.py @@ -12,7 +12,7 @@ from pathlib import Path import pytest from nisse_dev.envfile import load_env_file -from nisse_dev.git import GitRepo, authenticated_url +from nisse_dev.git import GitError, GitRepo, authenticated_url from nisse_dev.gitea import GiteaClient, pull_request_reconciler from nisse_dev.state import Intent @@ -88,6 +88,19 @@ def test_push_open_pr_and_reconcile(tmp_path: Path) -> None: # Now the same reconciler must see the effect and refuse to repeat it. assert reconcile(intent_for(branch)) is True + + # A4 is only real if the server enforces it. The agent's own token + # must not be able to merge the PR it just opened, and must not be + # able to push to the protected default branch at all. + merged = client._http.post( + f"/repos/{repo_path}/pulls/{pr.number}/merge", json={"Do": "merge"} + ) + assert merged.status_code >= 400, ( + "the agent merged its own PR: branch protection is not enforcing " + "the human gate" + ) + with pytest.raises(GitError): + repo.push(url, "main", token=token) finally: if pr is not None: client.close_pull_request(repo_path, pr.number)