diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index e671034..0a3f633 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/nisse-dev/src/nisse_dev/loop/agent.py @@ -70,8 +70,8 @@ After the go-ahead, work in a milestone: call start_milestone before changing anything (never create or switch branches yourself), make the change, run the project's tests, then call finish_milestone with a summary. The harness commits, pushes and opens a PR, and you wait for review. Review -comments arrive as messages: make the changes, run the tests, and call -finish_milestone again to push the fix. When the PR has merged, call +comments arrive as messages: make the changes within the same milestone (don't +start another), run the tests, and call finish_milestone again to push the fix. When the PR has merged, call close_item with what was done, what was verified, and what is left open. If the PR is closed without merging, ask the operator what next. diff --git a/nisse-dev/src/nisse_dev/views.py b/nisse-dev/src/nisse_dev/views.py index 2789774..bf13941 100644 --- a/nisse-dev/src/nisse_dev/views.py +++ b/nisse-dev/src/nisse_dev/views.py @@ -90,8 +90,9 @@ def project(events: list[StoredEvent], viewer: str) -> list[StoredEvent]: shown = _SHOWN.get(viewer) if shown is None: # A role with no view defined (the expert is stateless, the - # classifier sees only an action) reads the log as it is. - return list(events) + # classifier sees only an action) reads the log as it is, with + # results kept after their calls. + return _results_follow_calls(list(events)) results = { e.event.call_id: e.event for e in events if isinstance(e.event, ToolResult) } @@ -128,4 +129,30 @@ def project(events: list[StoredEvent], viewer: str) -> list[StoredEvent]: out.append(_said(stored, actor, line)) elif isinstance(event, AssistantText) and event.text.strip(): out.append(_said(stored, actor, event.text.strip())) - return out + return _results_follow_calls(out) + + +def _results_follow_calls(events: list[StoredEvent]) -> list[StoredEvent]: + """Keep each tool result right after its call, as both API formats + require: anything recorded while a call was in flight (a fact the + harness wrote while handling it, say) moves to just after the results. + A call with no result yet keeps its place; the renderer answers it.""" + out: list[StoredEvent] = [] + pending: set[str] = set() + held: list[StoredEvent] = [] + for stored in events: + event = stored.event + if isinstance(event, ToolCall): + pending.add(event.call_id) + out.append(stored) + elif isinstance(event, ToolResult) and event.call_id in pending: + pending.discard(event.call_id) + out.append(stored) + if not pending: + out.extend(held) + held = [] + elif pending: + held.append(stored) + else: + out.append(stored) + return out + held diff --git a/nisse-dev/tests/model_contract.py b/nisse-dev/tests/model_contract.py new file mode 100644 index 0000000..55d97ec --- /dev/null +++ b/nisse-dev/tests/model_contract.py @@ -0,0 +1,58 @@ +"""What a real model API insists on, for scripted test models. + +Both formats the agent sends require a tool call's result to come right +after it: in Anthropic's, the next message is the user's and starts with a +tool_result for every tool_use, before any text; in OpenAI's, the assistant +message with tool_calls is followed directly by a tool message for each. A +scripted model that doesn't check this lets a broken transcript pass the +unit tests and be rejected live; this makes it fail here. +""" + +from typing import Any + +from nisse_dev.render import OpenAIChatRenderer +from nisse_dev.render_anthropic import AnthropicMessagesRenderer + + +def assert_renderable(events: list[Any]) -> None: + _anthropic(AnthropicMessagesRenderer().render(list(events), None, "m")["messages"]) + _openai(OpenAIChatRenderer().render(list(events), None, "m")["messages"]) + + +def _anthropic(messages: list[dict[str, Any]]) -> None: + for index, message in enumerate(messages): + if message["role"] != "assistant": + continue + used = [b["id"] for b in message["content"] if b.get("type") == "tool_use"] + if not used: + continue + following = messages[index + 1] if index + 1 < len(messages) else None + assert following is not None and following["role"] == "user", ( + f"anthropic messages.{index}: tool_use with no user message after it" + ) + blocks = following["content"] + leading = [] + for block in blocks: + if block.get("type") != "tool_result": + break + leading.append(block["tool_use_id"]) + assert set(used) <= set(leading), ( + f"anthropic messages.{index}: tool_use ids {sorted(set(used) - set(leading))} " + "lack a tool_result at the start of the next message" + ) + + +def _openai(messages: list[dict[str, Any]]) -> None: + for index, message in enumerate(messages): + calls = [c["id"] for c in message.get("tool_calls") or []] + if not calls: + continue + answered = [] + for after in messages[index + 1 :]: + if after["role"] != "tool": + break + answered.append(after["tool_call_id"]) + assert set(calls) <= set(answered), ( + f"openai messages.{index}: tool_calls {sorted(set(calls) - set(answered))} " + "are not answered by the tool messages right after them" + ) diff --git a/nisse-dev/tests/test_loop.py b/nisse-dev/tests/test_loop.py index cbba820..5a88a13 100644 --- a/nisse-dev/tests/test_loop.py +++ b/nisse-dev/tests/test_loop.py @@ -6,6 +6,7 @@ from pathlib import Path from typing import Any import pytest +from model_contract import assert_renderable from nisse_dev.cancel import Cancel from nisse_dev.canonical import AssistantText, CanonicalEvent, Fact, ToolCall @@ -34,6 +35,7 @@ class ScriptedModel: self.seen: list[list[Any]] = [] def complete(self, events, tools, model, system=None) -> list[CanonicalEvent]: + assert_renderable(events) # as the real API would self.offered.append({t["function"]["name"] for t in tools or []}) self.seen.append(events) assert self.turns, "the model was called more often than scripted" diff --git a/nisse-dev/tests/test_loop_milestones.py b/nisse-dev/tests/test_loop_milestones.py index 8c4d643..9a77b22 100644 --- a/nisse-dev/tests/test_loop_milestones.py +++ b/nisse-dev/tests/test_loop_milestones.py @@ -8,6 +8,7 @@ from pathlib import Path from typing import Any import pytest +from model_contract import assert_renderable from nisse_dev.cancel import Cancel from nisse_dev.canonical import ToolCall @@ -42,6 +43,7 @@ class ScriptedModel: self.turns = list(turns) def complete(self, events, tools, model, system=None) -> list[Any]: + assert_renderable(events) # as the real API would assert self.turns, "the model was called more often than scripted" return self.turns.pop(0) diff --git a/nisse-dev/tests/test_loop_properties.py b/nisse-dev/tests/test_loop_properties.py index 79e055a..bbcd35f 100644 --- a/nisse-dev/tests/test_loop_properties.py +++ b/nisse-dev/tests/test_loop_properties.py @@ -13,6 +13,7 @@ from pathlib import Path from typing import Any import pytest +from model_contract import assert_renderable from test_loop_milestones import DON, Forge, call from test_loop_milestones import world as world # noqa: PLC0414 -- the fixture @@ -41,6 +42,7 @@ class PolicyModel: self.n = 0 def complete(self, events, tools, model, system=None) -> list[Any]: + assert_renderable(events) # as the real API would self.n += 1 offered = {t["function"]["name"] for t in tools} facts = [getattr(e.event, "text", "") for e in events] diff --git a/nisse-dev/tests/test_views.py b/nisse-dev/tests/test_views.py index 05f4558..838ad1a 100644 --- a/nisse-dev/tests/test_views.py +++ b/nisse-dev/tests/test_views.py @@ -282,3 +282,18 @@ def test_operator_messages_carry_their_channel(tmp_path: Path) -> None: "[operator, comment on plan issue #9] hi", "[operator, Gitea comment] hi", ] + + +def test_something_written_mid_call_moves_after_the_result(tmp_path: Path) -> None: + # Live, M2.1: start_milestone's fact landed between the call and its + # result, and the model API rejected every later request in the run. + store = new_store(tmp_path) + driver = store.transcript("a", "driver") + driver.append(ToolCall(call_id="c1", tool="start_milestone", arguments={})) + store.transcript("a", "orchestrator").append(Fact(text="milestone 1 started")) + driver.append(ToolResult(call_id="c1", result="started", success=True)) + kinds = [type(e.event).__name__ for e in driver.view()] + assert kinds == ["ToolCall", "ToolResult", "UserMessage"] + body = AnthropicMessagesRenderer().render(driver.view(), tools=None, model="m") + result_first = body["messages"][1]["content"][0] + assert result_first["type"] == "tool_result" diff --git a/scripts/e2e.sh b/scripts/e2e.sh index 55263ac..a4871f2 100755 --- a/scripts/e2e.sh +++ b/scripts/e2e.sh @@ -17,4 +17,6 @@ else export NISSE_E2E_BUILD_HOST=${NISSE_E2E_BUILD_HOST:-nisse-dev.garrett-hall.com} export NISSE_E2E_BUILD_KEY=${NISSE_E2E_BUILD_KEY:-/root/nisse-e2e/ssh/id_ed25519} fi +# Each run's state store is kept here, for nisse_dev.dump after a failure. +export NISSE_E2E_RUNS=${NISSE_E2E_RUNS:-/root/nisse-e2e/test-runs} exec uv run pytest -m e2e -rs tests/integration/test_end_to_end.py tests/integration/test_end_to_end_loop.py "$@" diff --git a/tests/integration/e2e_support.py b/tests/integration/e2e_support.py new file mode 100644 index 0000000..91f0752 --- /dev/null +++ b/tests/integration/e2e_support.py @@ -0,0 +1,42 @@ +"""Shared by the end-to-end tests: each run gets work that can't already be +done, and a base branch of its own, so runs never depend on one another or +on what earlier runs merged.""" + +import contextlib +import os +from collections.abc import Iterator + +from nisse_dev.gitea import GiteaClient, GiteaError + + +def task(run: str) -> str: + """A request no earlier run can have fulfilled.""" + return ( + f"Add a function vowels_{run}(text) to the sandbox package that returns " + "how many of a, e, i, o, u (either case) the text contains, with tests." + ) + + +def operator_gitea(url: str) -> GiteaClient: + token = os.environ.get("NISSE_OPERATOR_TOKEN", "") + assert token.strip(), ( + "NISSE_OPERATOR_TOKEN is not set: the test operator's Gitea token" + ) + return GiteaClient(url, token) + + +@contextlib.contextmanager +def base_branch(operator: GiteaClient, repo: str, run: str) -> Iterator[str]: + """A branch off main for this run's PRs to target and merge into, so + nothing accumulates in main; deleted afterwards.""" + name = f"e2e-base-{run}" + operator._request( + "POST", + f"/repos/{repo}/branches", + json={"new_branch_name": name, "old_branch_name": "main"}, + ) + try: + yield name + finally: + with contextlib.suppress(GiteaError): + operator._request("DELETE", f"/repos/{repo}/branches/{name}") diff --git a/tests/integration/test_end_to_end.py b/tests/integration/test_end_to_end.py index 12d6ba7..22abe53 100644 --- a/tests/integration/test_end_to_end.py +++ b/tests/integration/test_end_to_end.py @@ -25,6 +25,8 @@ from pathlib import Path from typing import Any import pytest +from e2e_support import base_branch, operator_gitea +from e2e_support import task as make_task from nisse_dev.agent import Agent, build_agent from nisse_dev.canonical import UserMessage @@ -43,10 +45,6 @@ pytestmark = [pytest.mark.integration, pytest.mark.e2e] OPERATOR = "@dongar:dongar.net" # an allowlisted operator in nisse-test.yaml SETTLE_S = 1200 # a milestone, with real models, inside one message -TASK = ( - "Add a function `count_vowels(text)` to the sandbox package that returns " - "how many of a, e, i, o, u (either case) the text contains, with tests." -) def build_host(ssh_host: SSHHost) -> BuildHost: @@ -70,10 +68,14 @@ def agent( ssh_host: SSHHost, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, -) -> Iterator[tuple[Agent, InMemoryTransport]]: +) -> Iterator[tuple[Agent, InMemoryTransport, str]]: run = uuid.uuid4().hex[:8] + stack = contextlib.ExitStack() + operator = operator_gitea(live_config.gitea_url) + base = stack.enter_context(base_branch(operator, live_config.gitea_repo, run)) config = replace( live_config, + base_branch=base, state_db=tmp_path / "state.db", workspace=Path(f"/tmp/nisse-e2e-{run}/sandbox"), build_host=build_host(ssh_host), @@ -84,7 +86,7 @@ def agent( try: agent = build_agent(config) agent.start() - yield agent, transport + yield agent, transport, run finally: if agent is not None: agent.stop() @@ -97,6 +99,7 @@ def agent( state_dir=tmp_path / "ssh", ) host.execute(f"rm -rf /tmp/nisse-e2e-{run}") + stack.close() # the run's base branch class Operator: @@ -148,14 +151,16 @@ def is_artifact(path: str) -> bool: ) -def test_a_milestone_end_to_end(agent: tuple[Agent, InMemoryTransport]) -> None: - running, transport = agent +def test_a_milestone_end_to_end( + agent: tuple[Agent, InMemoryTransport, str], +) -> None: + running, transport, run = agent operator = Operator(running, transport) gitea, repo = running.gitea, running.config.gitea_repo pr: dict[str, Any] = {} try: # The task first, before any project: it must be kept, not lost. - operator.say(TASK) + operator.say(make_task(run)) operator.say(f"use {running.config.project}") state = operator.drive_to_pr() pr = state.data["pr"] diff --git a/tests/integration/test_end_to_end_loop.py b/tests/integration/test_end_to_end_loop.py index a0e1b2f..7524cf7 100644 --- a/tests/integration/test_end_to_end_loop.py +++ b/tests/integration/test_end_to_end_loop.py @@ -19,10 +19,11 @@ from pathlib import Path from typing import Any import pytest +from e2e_support import base_branch, operator_gitea, task from nisse_dev.config import AgentConfig from nisse_dev.execution import SSHHost -from nisse_dev.gitea import GiteaClient, GiteaError +from nisse_dev.gitea import GiteaError from nisse_dev.loop.build import RunningLoop, build_loop_agent from nisse_dev.loop.items import ANSWER, GO_AHEAD, MERGE, WAITING, loop_of from nisse_dev.transport import InMemoryTransport @@ -30,19 +31,8 @@ from nisse_dev.transport import InMemoryTransport pytestmark = [pytest.mark.integration, pytest.mark.e2e] OPERATOR = "@dongar:dongar.net" # an allowlisted Matrix operator in nisse-test.yaml -TASK = ( - "Add a function count_vowels(text) to the sandbox package that returns how " - "many of a, e, i, o, u (either case) the text contains, with tests." -) DEADLINE_S = 1200 - - -def operator_gitea(config: AgentConfig) -> GiteaClient: - token = os.environ.get("NISSE_OPERATOR_TOKEN", "") - assert token.strip(), ( - "NISSE_OPERATOR_TOKEN is not set: the test operator's Gitea token" - ) - return GiteaClient(config.gitea_url, token) +MAX_ANSWERS = 5 # per wait for something else, before giving up @pytest.fixture @@ -51,14 +41,23 @@ def running( ssh_host: SSHHost, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, -) -> Iterator[tuple[RunningLoop, InMemoryTransport]]: +) -> Iterator[tuple[RunningLoop, InMemoryTransport, str]]: from test_end_to_end import build_host run = uuid.uuid4().hex[:8] + stack = contextlib.ExitStack() + # Kept outside pytest's temp directories, which pytest prunes, so a + # failed run's transcript can be read afterwards (nisse_dev.dump). + keep = Path(os.environ.get("NISSE_E2E_RUNS", "/tmp/nisse-e2e-runs")) / run + keep.mkdir(parents=True, exist_ok=True) + print(f"\ne2e run {run}: state store in {keep}") + operator = operator_gitea(live_config.gitea_url) + base = stack.enter_context(base_branch(operator, live_config.gitea_repo, run)) config = replace( live_config, loop="new", - state_db=tmp_path / "state.db", + base_branch=base, + state_db=keep / "state.db", workspace=Path(f"/tmp/nisse-e2e-{run}/sandbox"), build_host=build_host(ssh_host), ) @@ -70,7 +69,7 @@ def running( # The test drives intake and PR watching itself, not on a timer. agent.loop._interval = 3600 agent.start() - yield agent, transport + yield agent, transport, run finally: if agent is not None: agent.stop() @@ -82,6 +81,7 @@ def running( state_dir=tmp_path / "ssh", ) host.execute(f"rm -rf /tmp/nisse-e2e-{run}") + stack.close() # the run's base branch def is_artifact(path: str) -> bool: @@ -116,6 +116,7 @@ class Operator: def drive_until(self, wait: str) -> Any: """Answer whatever the agent asks until it waits for ``wait``.""" deadline = time.monotonic() + DEADLINE_S + answers = 0 while time.monotonic() < deadline: item = self.item() assert item is not None, "no work item" @@ -127,6 +128,9 @@ class Operator: if item.position == WAITING and loop.wait == GO_AHEAD: self.say("go") elif item.position == WAITING and loop.wait == ANSWER: + answers += 1 + # Answering forever is how a stuck agent looks: give up. + assert answers <= MAX_ANSWERS, f"stuck asking: {self.last()}" self.say("Use your best judgement and proceed.") else: self.settle() @@ -136,18 +140,18 @@ class Operator: return self.transport.outbound[-1].text if self.transport.outbound else "" -def test_the_slice_end_to_end(running: tuple[RunningLoop, InMemoryTransport]) -> None: - agent, transport = running +def test_the_slice_end_to_end( + running: tuple[RunningLoop, InMemoryTransport, str], +) -> None: + agent, transport, run = running operator = Operator(agent, transport) gitea, repo = agent.gitea, agent.config.gitea_repo - as_operator = operator_gitea(agent.config) + as_operator = operator_gitea(agent.config.gitea_url) issue = None pr = 0 try: # --- the operator files an issue and assigns it to the agent (D1) --- - issue = as_operator.create_issue( - repo, f"count_vowels ({uuid.uuid4().hex[:6]})", TASK - ) + issue = as_operator.create_issue(repo, f"vowels_{run}", task(run)) as_operator._request( "PATCH", f"/repos/{repo}/issues/{issue}", @@ -167,14 +171,18 @@ def test_the_slice_end_to_end(running: tuple[RunningLoop, InMemoryTransport]) -> assert f"Closes #{issue}" in body # --- a review comment is answered with a fix (D5) --- + # A request that can't already be met, so a fix must follow. + marker = f"reviewed-{uuid.uuid4().hex[:8]}" before = gitea._request("GET", f"/repos/{repo}/pulls/{pr}")["head"]["sha"] as_operator.post_comment( - repo, pr, "Please add a one-line docstring to count_vowels." + repo, pr, f"Please add the comment line `# {marker}` above vowels_{run}." ) operator.tick() operator.drive_until(MERGE) after = gitea._request("GET", f"/repos/{repo}/pulls/{pr}")["head"]["sha"] assert after != before, "no fix was pushed for the review comment" + diff = gitea._http.get(f"/repos/{repo}/pulls/{pr}.diff").text + assert marker in diff, "the pushed fix doesn't do what the comment asked" # --- the operator approves and merges; the agent closes the item --- with contextlib.suppress(GiteaError): # approval may not be required @@ -188,8 +196,11 @@ def test_the_slice_end_to_end(running: tuple[RunningLoop, InMemoryTransport]) -> ) operator.tick() deadline = time.monotonic() + DEADLINE_S + nudges = 0 while operator.item().status != "closed" and time.monotonic() < deadline: if loop_of(operator.item()).wait == ANSWER: + nudges += 1 + assert nudges <= MAX_ANSWERS, f"never closed: {operator.last()}" operator.say("It's merged; please close the item.") operator.settle() assert operator.item().outcome == "completed"