From 311756592803800754f92753dc66910f7a0c3281 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Mon, 7 Sep 2026 20:43:20 +0000 Subject: [PATCH] =?UTF-8?q?Publish=20finished=20milestones=20as=20PRs=20(B?= =?UTF-8?q?2/A3)=20=E2=80=94=20step=206a,=20part=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the B1 loop past EXECUTING, and answers the question raised reviewing #6: PR acceptance is a milestone's final gate, so there is no separate FINISHED state — completion returns to milestone planning or to IDLE. - publish.py: MilestonePublisher commits the work on a task-namespaced branch, pushes, and opens a PR carrying the approved plan. Intent is recorded before the first externally visible act, so a crash anywhere in the sequence leaves a durable record and boot reconciliation asks Gitea whether the PR exists rather than opening a second one - Nothing to commit is reported, not raised: the executor may legitimately conclude there was nothing to change - control.py: PR_OPEN state with EXECUTING -> PR_OPEN and PR_OPEN -> {MILESTONE_PLANNING, IDLE}; operator commands 'next milestone' and 'project complete' - A configured publisher always contributes its own reconciler at boot, so an interrupted publish is settled against external truth without the caller having to remember to register it - A publish failure gates to the operator rather than retrying into a possible duplicate Co-Authored-By: Claude Opus 5 --- nisse-dev/src/nisse_dev/control.py | 92 +++++++++++++- nisse-dev/src/nisse_dev/publish.py | 103 +++++++++++++++ nisse-dev/tests/test_pr_open.py | 196 +++++++++++++++++++++++++++++ nisse-dev/tests/test_publish.py | 158 +++++++++++++++++++++++ 4 files changed, 546 insertions(+), 3 deletions(-) create mode 100644 nisse-dev/src/nisse_dev/publish.py create mode 100644 nisse-dev/tests/test_pr_open.py create mode 100644 nisse-dev/tests/test_publish.py diff --git a/nisse-dev/src/nisse_dev/control.py b/nisse-dev/src/nisse_dev/control.py index f950a28..f01077f 100644 --- a/nisse-dev/src/nisse_dev/control.py +++ b/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 '") 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 ' 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) diff --git a/nisse-dev/src/nisse_dev/publish.py b/nisse-dev/src/nisse_dev/publish.py new file mode 100644 index 0000000..93b613f --- /dev/null +++ b/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) diff --git a/nisse-dev/tests/test_pr_open.py b/nisse-dev/tests/test_pr_open.py new file mode 100644 index 0000000..0233c4f --- /dev/null +++ b/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 diff --git a/nisse-dev/tests/test_publish.py b/nisse-dev/tests/test_publish.py new file mode 100644 index 0000000..57baf67 --- /dev/null +++ b/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 -- 2.54.0