Merge pull request 'Externalise plans to Gitea and accept approvals there (A4, Part H)' (#18) from gitea-approvals into review-loop

Reviewed-on: dongar/nisse#18
Reviewed-by: Don Garrett <dgarrett@acm.org>

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
+        ]

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.
 

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(

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"})
 

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"

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:

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)
 
 

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)