Merge pull request 'Publish finished milestones as PRs (B2/A3) — step 6a, part 2' (#16) from pr-open into main

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

nisse-dev/src/nisse_dev/control.py

@@ -15,6 +15,8 @@ from typing import Any, Protocol
 
 from nisse_dev.canonical import GateOpened, PlanSubmitted, UserMessage
 from nisse_dev.gates import Approved, Gate, Ignored, PlanRevision, Rejected
+from nisse_dev.git import GitError
+from nisse_dev.gitea import GiteaError
 from nisse_dev.llm import RoleMap
 from nisse_dev.reconcile import Reconciler, reconcile_on_boot
 from nisse_dev.state import StateStore
@@ -26,6 +28,7 @@ AWAIT_ARCH_SIGNOFF = "AWAIT_ARCH_SIGNOFF"
 MILESTONE_PLANNING = "MILESTONE_PLANNING"
 AWAIT_MILESTONE_SIGNOFF = "AWAIT_MILESTONE_SIGNOFF"
 EXECUTING = "EXECUTING"
+PR_OPEN = "PR_OPEN"
 AWAIT_INPUT = "AWAIT_INPUT"
 
 TRANSITIONS = {
@@ -36,6 +39,12 @@ TRANSITIONS = {
     (MILESTONE_PLANNING, AWAIT_MILESTONE_SIGNOFF),
     (AWAIT_MILESTONE_SIGNOFF, MILESTONE_PLANNING),
     (AWAIT_MILESTONE_SIGNOFF, EXECUTING),
+    (EXECUTING, PR_OPEN),
+    # PR acceptance is the milestone's final gate (B1): a merged PR is the
+    # durable record, so completion returns to planning or to idle rather
+    # than passing through a separate FINISHED state.
+    (PR_OPEN, MILESTONE_PLANNING),
+    (PR_OPEN, IDLE),
 }
 
 
@@ -67,6 +76,17 @@ class MilestoneExecutor(Protocol):
     def run(self, milestone: int) -> Any: ...
 
 
+class MilestonePublisher(Protocol):
+    """What the orchestrator needs to publish a milestone; the concrete one
+    lives in nisse_dev.publish."""
+
+    def reconcilers(self) -> Mapping[str, Reconciler]: ...
+
+    def publish(
+        self, *, task_id: str, milestone: int, project: str, plan: str
+    ) -> Any: ...
+
+
 class PlanningAgent(Protocol):
     def plan(
         self,
@@ -105,6 +125,7 @@ def _fresh_data() -> dict[str, Any]:
         "milestone": 1,
         "task_id": None,
         "roles": None,  # A11 mapping, pinned at task start
+        "pr": None,  # the open PR for the current milestone, once published
     }
 
 
@@ -117,19 +138,26 @@ class Orchestrator:
         operators: Iterable[str],
         roles: RoleMap | None = None,
         executor: "MilestoneExecutor | None" = None,
+        publisher: "MilestonePublisher | None" = None,
     ) -> None:
         self._store = store
         self._transport = transport
         self._agent = agent
         self._roles = roles
         self._executor = executor
+        self._publisher = publisher
         self._operators = set(operators)
         self._data = _fresh_data()
         self._project: str | None = None
         transport.on_message(self._handle)
 
     def start(self, reconcilers: Mapping[str, Reconciler] | None = None) -> None:
-        report = reconcile_on_boot(self._store, reconcilers or {})
+        # The publisher owns the external-truth query for its own intents, so
+        # a configured publisher always contributes its reconciler (A3).
+        registered: dict[str, Reconciler] = dict(reconcilers or {})
+        if self._publisher is not None:
+            registered = {**self._publisher.reconcilers(), **registered}
+        report = reconcile_on_boot(self._store, registered)
         state = self._store.get_state()
         if state is None:
             self._transition(IDLE, cause="boot: fresh state")
@@ -189,6 +217,8 @@ class Orchestrator:
                 self._transport.send_message("no active project; say 'use <project>'")
         elif current in (AWAIT_ARCH_SIGNOFF, AWAIT_MILESTONE_SIGNOFF):
             self._handle_gate_reply(msg, text)
+        elif current == PR_OPEN:
+            self._handle_pr_open(text)
         elif current == AWAIT_INPUT:
             self._handle_answer(text)
         else:
@@ -337,9 +367,8 @@ class Orchestrator:
         self._transport.send_message(f"executing milestone {milestone}")
         outcome = self._executor.run(milestone)
         if outcome.status == "completed":
-            # PR_OPEN and the review loop arrive with the Gitea layer (B2);
-            # until then a finished milestone simply reports and waits.
             self._transport.send_message(f"milestone {milestone} done: {outcome.note}")
+            self._publish_milestone(milestone)
             return
         self._await_input(
             cause=f"milestone {milestone} {outcome.status}",
@@ -347,6 +376,63 @@ class Orchestrator:
             pending={"kind": "milestone_stopped", "milestone": milestone},
         )
 
+    def _publish_milestone(self, milestone: int) -> None:
+        if self._publisher is None:
+            return
+        gate = self._data.get("gate") or {}
+        try:
+            result = self._publisher.publish(
+                task_id=str(self._data.get("task_id")),
+                milestone=milestone,
+                project=str(self._project),
+                plan=str(gate.get("plan", "")),
+            )
+        except (GiteaError, GitError) as exc:
+            # The intent log already records what was in flight, so boot
+            # reconciliation can settle it against Gitea. Gate rather than
+            # retry blindly into a possible duplicate.
+            self._await_input(
+                cause=f"publishing milestone {milestone} failed: {exc}",
+                prompt=f"could not publish milestone {milestone}: {exc}",
+                pending={"kind": "milestone_stopped", "milestone": milestone},
+            )
+            return
+        if result.pull_request is None:
+            self._transport.send_message(result.note)
+            return
+        self._data["pr"] = {
+            "number": result.pull_request.number,
+            "url": result.pull_request.url,
+            "milestone": milestone,
+        }
+        self._transition(PR_OPEN, cause=f"milestone {milestone} published")
+        self._transport.send_message(
+            f"{result.note} — review it, then say 'next milestone' or "
+            "'project complete'"
+        )
+
+    def _handle_pr_open(self, text: str) -> None:
+        if text == "next milestone":
+            self._data["milestone"] = int(self._data.get("milestone", 1)) + 1
+            self._data["gate"] = None
+            self._data["pr"] = None
+            self._transition(MILESTONE_PLANNING, cause="operator: next milestone")
+            self._run_planning_turn(f"milestone-{self._data['milestone']}")
+        elif text == "project complete":
+            self._data = {**_fresh_data()}
+            self._project = None
+            self._store.oplog.task_id = None
+            self._transition(IDLE, cause="operator: project complete")
+            self._transport.send_message(
+                "project closed; say 'use <project>' to start another"
+            )
+        else:
+            pr = self._data.get("pr") or {}
+            self._transport.send_message(
+                f"PR #{pr.get('number')} is open; say 'next milestone' or "
+                "'project complete'"
+            )
+
     def _await_input(self, *, cause: str, prompt: str, pending: dict[str, Any]) -> None:
         self._data["pending"] = pending
         self._transition(AWAIT_INPUT, cause=cause)

nisse-dev/src/nisse_dev/publish.py

@@ -0,0 +1,103 @@
+"""Publishing a finished milestone as a Gitea PR (spec B2, A2/A3).
+
+The ordering here is the whole point. Intent is recorded *before* the first
+externally visible act, so a crash anywhere in the sequence leaves a durable
+record of what was in flight. Boot reconciliation then asks Gitea whether the
+PR exists rather than repeating the push-and-open — the spec's own example of
+an idempotent transition reconciled against external truth.
+"""
+
+from dataclasses import dataclass
+
+from nisse_dev.git import GitRepo, authenticated_url
+from nisse_dev.gitea import GiteaClient, PullRequest, pull_request_reconciler
+from nisse_dev.reconcile import Reconciler
+from nisse_dev.state import StateStore
+
+OPEN_PR_INTENT = "open_pr"
+
+
+@dataclass(frozen=True)
+class PublishResult:
+    pull_request: PullRequest | None
+    note: str
+
+
+class MilestonePublisher:
+    def __init__(
+        self,
+        store: StateStore,
+        client: GiteaClient,
+        repo: str,
+        git_repo: GitRepo,
+        *,
+        base_url: str,
+        account: str,
+        token: str,
+        base_branch: str = "main",
+    ) -> None:
+        self._store = store
+        self._client = client
+        self._repo = repo
+        self._git = git_repo
+        self._base_branch = base_branch
+        self._remote = authenticated_url(base_url, repo, account, token)
+        self._token = token
+
+    def reconcilers(self) -> dict[str, Reconciler]:
+        return {OPEN_PR_INTENT: pull_request_reconciler(self._client, self._repo)}
+
+    def branch_for(self, task_id: str, milestone: int) -> str:
+        # Namespaced by task so re-running a project against the same repo
+        # cannot collide with an earlier run's branch.
+        return f"{task_id}/milestone-{milestone}"
+
+    def publish(
+        self,
+        *,
+        task_id: str,
+        milestone: int,
+        project: str,
+        plan: str,
+        service_url: str | None = None,
+    ) -> PublishResult:
+        branch = self.branch_for(task_id, milestone)
+        if self._git.current_branch() != branch:
+            self._git.checkout_new_branch(branch)
+        sha = self._git.commit_all(f"Milestone {milestone}: {project}")
+        if sha is None:
+            # Not an error: the executor may have concluded there was nothing
+            # to change. Report it and let the operator decide.
+            return PublishResult(None, "no changes to publish")
+
+        intent_id = self._store.begin_intent(
+            OPEN_PR_INTENT,
+            {"branch": branch, "milestone": milestone, "sha": sha},
+            cause=f"publishing milestone {milestone}",
+        )
+        self._git.push(self._remote, branch, token=self._token)
+        pr = self._client.open_pull_request(
+            self._repo,
+            head=branch,
+            base=self._base_branch,
+            title=f"Milestone {milestone}: {project}",
+            body=_pr_body(milestone, plan, service_url),
+        )
+        self._store.commit_intent(intent_id, cause=f"opened PR #{pr.number}")
+        return PublishResult(pr, f"opened PR #{pr.number}: {pr.url}")
+
+
+def _pr_body(milestone: int, plan: str, service_url: str | None) -> str:
+    parts = [
+        f"Milestone {milestone}, implemented by nisse-dev against the approved plan.",
+        "",
+        "## Approved plan",
+        "",
+        plan,
+    ]
+    if service_url:
+        # B2: a runnable test service's address belongs in the PR description
+        # so a reviewer can exercise it. Nothing reports one yet; the browser
+        # tool (A6) is what will.
+        parts += ["", "## Running test service", "", service_url]
+    return "\n".join(parts)

nisse-dev/tests/test_pr_open.py

@@ -0,0 +1,196 @@
+"""The B1 loop past EXECUTING: publish, review gate, then next milestone or
+project completion."""
+
+import io
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from nisse_dev.canonical import AssistantText, CanonicalEvent, StoredEvent, ToolCall
+from nisse_dev.control import (
+    AWAIT_INPUT,
+    EXECUTING,
+    IDLE,
+    PR_OPEN,
+    Orchestrator,
+)
+from nisse_dev.executor import ExecutorOutcome
+from nisse_dev.gitea import GiteaError, PullRequest
+from nisse_dev.llm import RoleMap
+from nisse_dev.oplog import OpLog
+from nisse_dev.planner import LLMPlanningAgent
+from nisse_dev.publish import PublishResult
+from nisse_dev.state import StateStore
+from nisse_dev.transport import InMemoryTransport
+
+ROLES = RoleMap(
+    executor="llama3.2",
+    planner="haiku",
+    delegate="haiku",
+    consultant="haiku",
+)
+
+
+class ScriptedClient:
+    def __init__(self, plans: list[str]) -> None:
+        self._plans = plans
+        self.calls: list[dict[str, Any]] = []
+
+    def complete(
+        self,
+        events: list[StoredEvent],
+        tools: list[dict[str, Any]] | None,
+        model: str,
+        system: str | None = None,
+    ) -> list[CanonicalEvent]:
+        self.calls.append({"system": system})
+        if not self._plans:
+            return [AssistantText(text="no more plans", model=model)]
+        return [
+            ToolCall(
+                call_id=f"c{len(self.calls)}",
+                tool="submit_plan",
+                arguments={"type": "x", "content": self._plans.pop(0)},
+            )
+        ]
+
+
+class DoneExecutor:
+    def run(self, milestone: int) -> ExecutorOutcome:
+        return ExecutorOutcome("completed", f"milestone {milestone} done")
+
+
+class FakePublisher:
+    def __init__(
+        self, *, result: PublishResult | None = None, error: Exception | None = None
+    ) -> None:
+        self._result = result or PublishResult(
+            PullRequest(number=7, state="open", head="h", url="https://g/pulls/7"),
+            "opened PR #7: https://g/pulls/7",
+        )
+        self._error = error
+        self.calls: list[dict[str, Any]] = []
+
+    def reconcilers(self) -> dict[str, Any]:
+        return {"open_pr": lambda intent: True}
+
+    def publish(self, **kwargs: Any) -> PublishResult:
+        self.calls.append(kwargs)
+        if self._error is not None:
+            raise self._error
+        return self._result
+
+
+def build(
+    tmp_path: Path, publisher: Any, plans: list[str] | None = None
+) -> tuple[StateStore, InMemoryTransport]:
+    store = StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+    transport = InMemoryTransport()
+    client = ScriptedClient(plans or ["arch plan", "m1 plan", "m2 plan"])
+    orch = Orchestrator(
+        store,
+        transport,
+        LLMPlanningAgent(client, store, ROLES),
+        operators={"don"},
+        roles=ROLES,
+        executor=DoneExecutor(),
+        publisher=publisher,
+    )
+    orch.start()
+    return store, transport
+
+
+def run_to_pr(tmp_path: Path, publisher: Any) -> tuple[StateStore, InMemoryTransport]:
+    store, transport = build(tmp_path, publisher)
+    transport.receive("don", "use demo")
+    transport.receive("don", "approve")  # arch
+    transport.receive("don", "approve")  # milestone plan
+    transport.receive("don", "start milestone 1")
+    return store, transport
+
+
+def test_completed_milestone_publishes_and_enters_pr_open(tmp_path: Path) -> None:
+    publisher = FakePublisher()
+    store, transport = run_to_pr(tmp_path, publisher)
+
+    assert store.get_state().state == PR_OPEN
+    assert publisher.calls[0]["milestone"] == 1
+    assert publisher.calls[0]["project"] == "demo"
+    assert "m1 plan" in publisher.calls[0]["plan"]  # the approved plan travels
+    assert "PR #7" in transport.outbound[-1].text
+
+
+def test_pr_open_records_the_pull_request(tmp_path: Path) -> None:
+    store, _ = run_to_pr(tmp_path, FakePublisher())
+    assert store.get_state().data["pr"]["number"] == 7
+
+
+def test_next_milestone_replans_and_bumps_the_counter(tmp_path: Path) -> None:
+    store, transport = run_to_pr(tmp_path, FakePublisher())
+    transport.receive("don", "next milestone")
+
+    state = store.get_state()
+    assert state.state != PR_OPEN
+    assert state.data["milestone"] == 2
+    assert "m2 plan" in transport.outbound[-1].text
+
+
+def test_project_complete_returns_to_idle(tmp_path: Path) -> None:
+    store, transport = run_to_pr(tmp_path, FakePublisher())
+    transport.receive("don", "project complete")
+
+    state = store.get_state()
+    assert state.state == IDLE
+    assert state.project is None
+    assert "project closed" in transport.outbound[-1].text
+
+
+def test_unknown_command_in_pr_open_explains_the_options(tmp_path: Path) -> None:
+    store, transport = run_to_pr(tmp_path, FakePublisher())
+    transport.receive("don", "looks good!")
+    assert store.get_state().state == PR_OPEN
+    assert "next milestone" in transport.outbound[-1].text
+
+
+def test_no_changes_reports_without_entering_pr_open(tmp_path: Path) -> None:
+    publisher = FakePublisher(result=PublishResult(None, "no changes to publish"))
+    store, transport = run_to_pr(tmp_path, publisher)
+    assert store.get_state().state == EXECUTING
+    assert transport.outbound[-1].text == "no changes to publish"
+
+
+def test_publish_failure_gates_to_the_operator(tmp_path: Path) -> None:
+    publisher = FakePublisher(error=GiteaError("gitea is down"))
+    store, transport = run_to_pr(tmp_path, publisher)
+    assert store.get_state().state == AWAIT_INPUT
+    assert "could not publish" in transport.outbound[-1].text
+    # And the operator can still get back out (the deadlock regression).
+    transport.receive("don", "gitea is back")
+    assert store.get_state().state != AWAIT_INPUT
+
+
+def test_publisher_reconciler_is_registered_at_boot(tmp_path: Path) -> None:
+    store = StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+    store.begin_intent("open_pr", {"branch": "b"}, cause="crash")
+    transport = InMemoryTransport()
+    orch = Orchestrator(
+        store,
+        transport,
+        LLMPlanningAgent(ScriptedClient([]), store, ROLES),
+        operators={"don"},
+        roles=ROLES,
+        publisher=FakePublisher(),
+    )
+    orch.start()
+    # The publisher's reconciler answered "already opened", so boot resumed
+    # instead of gating on an unknown intent kind.
+    assert store.pending_intents() == []
+    assert store.get_state().state != AWAIT_INPUT
+
+
+@pytest.mark.parametrize("command", ["next milestone", "project complete"])
+def test_pr_open_commands_are_operator_only(tmp_path: Path, command: str) -> None:
+    store, transport = run_to_pr(tmp_path, FakePublisher())
+    transport.receive("mallory", command)
+    assert store.get_state().state == PR_OPEN

nisse-dev/tests/test_publish.py

@@ -0,0 +1,158 @@
+import io
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from nisse_dev.git import GitRepo
+from nisse_dev.gitea import GiteaError, PullRequest
+from nisse_dev.oplog import OpLog
+from nisse_dev.publish import OPEN_PR_INTENT, MilestonePublisher
+from nisse_dev.state import StateStore
+
+
+class FakeGitea:
+    def __init__(self, *, fail: bool = False) -> None:
+        self.opened: list[dict[str, Any]] = []
+        self.existing: list[str] = []
+        self._fail = fail
+
+    def open_pull_request(
+        self, repo: str, *, head: str, base: str, title: str, body: str
+    ) -> PullRequest:
+        if self._fail:
+            raise GiteaError("gitea is down")
+        self.opened.append(
+            {"repo": repo, "head": head, "base": base, "title": title, "body": body}
+        )
+        self.existing.append(head)
+        return PullRequest(
+            number=len(self.opened), state="open", head=head, url=f"u/{head}"
+        )
+
+    def find_pull_request(self, repo: str, head: str) -> PullRequest | None:
+        if head in self.existing:
+            return PullRequest(number=1, state="open", head=head, url="u")
+        return None
+
+
+class FakeGit:
+    """Records what would have been pushed; no network."""
+
+    def __init__(self, root: Path, *, changes: bool = True) -> None:
+        self.repo = GitRepo(root)
+        self.repo.init()
+        self.repo.configure_identity("bot", "bot@example.test")
+        if changes:
+            (root / "work.txt").write_text("done")
+        self.pushed: list[str] = []
+
+    def current_branch(self) -> str:
+        return self.repo.current_branch()
+
+    def checkout_new_branch(self, name: str) -> None:
+        self.repo.checkout_new_branch(name)
+
+    def commit_all(self, message: str) -> str | None:
+        return self.repo.commit_all(message)
+
+    def push(self, remote_url: str, branch: str, *, token: str | None = None) -> None:
+        self.pushed.append(branch)
+
+
+@pytest.fixture
+def store(tmp_path: Path) -> StateStore:
+    return StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+
+
+def build(store: StateStore, git: Any, client: Any) -> MilestonePublisher:
+    return MilestonePublisher(
+        store,
+        client,
+        "o/sandbox",
+        git,
+        base_url="https://gitea.test",
+        account="bot",
+        token="tok",
+    )
+
+
+def test_publish_commits_pushes_and_opens_a_pr(
+    store: StateStore, tmp_path: Path
+) -> None:
+    git = FakeGit(tmp_path / "work")
+    client = FakeGitea()
+    result = build(store, git, client).publish(
+        task_id="demo-abc", milestone=2, project="demo", plan="the plan"
+    )
+
+    assert git.pushed == ["demo-abc/milestone-2"]
+    assert client.opened[0]["head"] == "demo-abc/milestone-2"
+    assert client.opened[0]["base"] == "main"
+    assert "the plan" in client.opened[0]["body"]
+    assert result.pull_request is not None
+    assert "opened PR #1" in result.note
+
+
+def test_intent_is_recorded_then_committed(store: StateStore, tmp_path: Path) -> None:
+    build(store, FakeGit(tmp_path / "work"), FakeGitea()).publish(
+        task_id="t", milestone=1, project="demo", plan="p"
+    )
+    # Nothing left pending: the intent opened before the push was settled
+    # once the PR existed.
+    assert store.pending_intents() == []
+
+
+def test_failure_after_intent_leaves_it_pending_for_reconciliation(
+    store: StateStore, tmp_path: Path
+) -> None:
+    publisher = build(store, FakeGit(tmp_path / "work"), FakeGitea(fail=True))
+    with pytest.raises(GiteaError):
+        publisher.publish(task_id="t", milestone=1, project="demo", plan="p")
+
+    [pending] = store.pending_intents()
+    assert pending.kind == OPEN_PR_INTENT
+    assert pending.payload["branch"] == "t/milestone-1"
+    assert pending.payload["sha"]
+
+
+def test_nothing_to_commit_is_reported_not_raised(
+    store: StateStore, tmp_path: Path
+) -> None:
+    git = FakeGit(tmp_path / "work", changes=False)
+    client = FakeGitea()
+    result = build(store, git, client).publish(
+        task_id="t", milestone=1, project="demo", plan="p"
+    )
+    assert result.pull_request is None
+    assert result.note == "no changes to publish"
+    assert client.opened == []
+    # No externally visible act was attempted, so no intent was opened.
+    assert store.pending_intents() == []
+
+
+def test_branch_is_namespaced_by_task(store: StateStore, tmp_path: Path) -> None:
+    publisher = build(store, FakeGit(tmp_path / "work"), FakeGitea())
+    assert publisher.branch_for("greet-9f2", 3) == "greet-9f2/milestone-3"
+
+
+def test_reconciler_settles_an_intent_left_by_a_crash(
+    store: StateStore, tmp_path: Path
+) -> None:
+    # Simulate the dangerous window: the PR was opened, but the process died
+    # before the intent could be committed. On the next boot the reconciler
+    # must say "already done" so the milestone is not published twice.
+    client = FakeGitea()
+    publisher = build(store, FakeGit(tmp_path / "work"), client)
+    intent_id = store.begin_intent(
+        OPEN_PR_INTENT, {"branch": "t/milestone-1"}, cause="crash simulation"
+    )
+    client.existing.append("t/milestone-1")
+
+    reconcilers = publisher.reconcilers()
+    assert set(reconcilers) == {OPEN_PR_INTENT}
+    [pending] = store.pending_intents()
+    assert reconcilers[OPEN_PR_INTENT](pending) is True
+
+    store.commit_intent(intent_id, cause="verified")
+    assert client.opened == []  # nothing re-published