Fix the run-breaking tool-result order; e2e test runs the slice (M2.1, PR D)

The first live run of the new loop's e2e test found that a fact recorded
while start_milestone was in flight landed between the tool call and its
result; both API formats reject that, so every later request in the run
failed. Views now keep each result right after its call. Scripted test
models check the APIs' ordering rules, so this can't pass unit tests
again.

The e2e tests no longer depend on the sandbox's history: each run asks for
a uniquely named function and works against a base branch of its own,
deleted afterwards. The review step asks for a change that can't already
be there. The scripted operator gives up after five fruitless answers, and
each run keeps its state store for inspection. The driver's prompt says
review fixes stay in the current milestone.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE

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.
 

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

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

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"

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)
 

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]

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"

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 "$@"

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

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

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"