Merge pull request 'Add executor tool loop (A12/A15) — step 5b' (#12) from executor-loop into main

nisse-dev/tests/test_executor.py

@@ -0,0 +1,173 @@
+import io
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from nisse_dev.canonical import AssistantText, CanonicalEvent, StoredEvent, ToolCall
+from nisse_dev.executor import LLMExecutor
+from nisse_dev.llm import LLMError, RoleMap
+from nisse_dev.oplog import OpLog
+from nisse_dev.state import StateStore
+from nisse_dev.tools import Workspace
+
+ROLES = RoleMap(
+    executor="llama3.2",
+    planner="claude-haiku-4-5",
+    delegate="claude-haiku-4-5",
+    consultant="claude-haiku-4-5",
+)
+
+
+class ScriptedClient:
+    def __init__(self, turns: list[list[CanonicalEvent]]) -> None:
+        self._turns = turns
+        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({"tools": tools, "model": model, "system": system})
+        if not self._turns:
+            return [AssistantText(text="done", model=model)]
+        return self._turns.pop(0)
+
+
+class FailingClient:
+    def complete(self, *args: Any, **kwargs: Any) -> list[CanonicalEvent]:
+        raise LLMError("proxy exploded")
+
+
+def call(tool: str, call_id: str = "c1", **arguments: Any) -> ToolCall:
+    return ToolCall(call_id=call_id, tool=tool, arguments=arguments)
+
+
+@pytest.fixture
+def store(tmp_path: Path) -> StateStore:
+    return StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+
+
+@pytest.fixture
+def ws(tmp_path: Path) -> Workspace:
+    return Workspace(tmp_path / "work", timeout_s=10)
+
+
+def build(client: Any, store: StateStore, ws: Workspace, **kw: Any) -> LLMExecutor:
+    return LLMExecutor(client, store, ROLES, ws, **kw)
+
+
+def test_tool_calls_are_executed_and_results_logged(
+    store: StateStore, ws: Workspace
+) -> None:
+    client = ScriptedClient(
+        [
+            [call("write_file", path="app.py", content="print(1)\n")],
+            [AssistantText(text="wrote the file", model="llama3.2")],
+        ]
+    )
+    outcome = build(client, store, ws).run(milestone=1)
+
+    assert outcome.status == "completed"
+    assert (ws.root / "app.py").read_text() == "print(1)\n"
+    kinds = [type(e.event).__name__ for e in store.read_events()]
+    assert kinds == ["ToolCall", "ToolResult", "AssistantText"]
+
+
+def test_uses_executor_role_and_reduced_manifest(
+    store: StateStore, ws: Workspace
+) -> None:
+    client = ScriptedClient([])
+    build(client, store, ws).run(milestone=2)
+    assert client.calls[0]["model"] == "llama3.2"  # never the planner
+    names = [t["function"]["name"] for t in client.calls[0]["tools"]]
+    assert names == ["read_file", "write_file", "list_dir", "run_command"]
+    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")]])
+    assert build(client, store, ws).run(milestone=1).status == "completed"
+
+
+def test_two_consecutive_command_failures_escalate(
+    store: StateStore, ws: Workspace
+) -> None:
+    client = ScriptedClient(
+        [
+            [call("run_command", command="exit 1")],
+            [call("run_command", command="exit 1")],
+            [call("run_command", command="exit 1")],
+        ]
+    )
+    outcome = build(client, store, ws, failure_limit=2).run(milestone=1)
+    assert outcome.status == "escalate"
+    assert "2 consecutive command failures" in outcome.note
+    assert len(client.calls) == 2  # stopped, did not keep trying
+
+
+def test_a_success_resets_the_failure_count(store: StateStore, ws: Workspace) -> None:
+    client = ScriptedClient(
+        [
+            [call("run_command", command="exit 1")],
+            [call("run_command", command="true")],
+            [call("run_command", command="exit 1")],
+            [AssistantText(text="done", model="m")],
+        ]
+    )
+    assert build(client, store, ws, failure_limit=2).run(milestone=1).status == (
+        "completed"
+    )
+
+
+def test_failed_reads_do_not_count_toward_escalation(
+    store: StateStore, ws: Workspace
+) -> None:
+    client = ScriptedClient(
+        [
+            [call("read_file", path="missing-a.txt")],
+            [call("read_file", path="missing-b.txt")],
+            [AssistantText(text="looked around, done", model="m")],
+        ]
+    )
+    assert build(client, store, ws, failure_limit=2).run(milestone=1).status == (
+        "completed"
+    )
+
+
+def test_turn_cap_is_enforced(store: StateStore, ws: Workspace) -> None:
+    forever = [[call("list_dir", path=".")] for _ in range(50)]
+    client = ScriptedClient(forever)
+    outcome = build(client, store, ws, max_turns=3).run(milestone=1)
+    assert outcome.status == "capped"
+    assert "3-turn cap" in outcome.note
+    assert len(client.calls) == 3
+
+
+def test_llm_error_escalates_rather_than_crashing(
+    store: StateStore, ws: Workspace
+) -> None:
+    outcome = build(FailingClient(), store, ws).run(milestone=1)
+    assert outcome.status == "escalate"
+    assert "proxy exploded" in outcome.note
+
+
+def test_multiple_calls_in_one_turn_all_run(store: StateStore, ws: Workspace) -> None:
+    client = ScriptedClient(
+        [
+            [
+                call("write_file", call_id="a", path="one.txt", content="1"),
+                call("write_file", call_id="b", path="two.txt", content="2"),
+            ],
+            [AssistantText(text="done", model="m")],
+        ]
+    )
+    build(client, store, ws).run(milestone=1)
+    assert (ws.root / "one.txt").exists() and (ws.root / "two.txt").exists()
+    results = [
+        e.event for e in store.read_events() if type(e.event).__name__ == "ToolResult"
+    ]
+    assert {r.call_id for r in results} == {"a", "b"}

nisse-dev/tests/test_planner.py

@@ -12,7 +12,14 @@ from nisse_dev.canonical import (
     ToolCall,
     UserMessage,
 )
-from nisse_dev.control import AskHuman, Orchestrator, PlanningError, SubmitPlan
+from nisse_dev.control import (
+    AWAIT_ARCH_SIGNOFF,
+    AWAIT_MILESTONE_SIGNOFF,
+    AskHuman,
+    Orchestrator,
+    PlanningError,
+    SubmitPlan,
+)
 from nisse_dev.llm import RoleMap
 from nisse_dev.oplog import OpLog
 from nisse_dev.planner import PLANNER_TOOLS, LLMPlanningAgent
@@ -108,7 +115,18 @@ def test_model_output_is_appended_to_the_log(store: StateStore) -> None:
     )
     LLMPlanningAgent(client, store, ROLES).plan("arch", "demo")
     kinds = [type(e.event).__name__ for e in store.read_events()]
-    assert kinds == ["AssistantText", "ToolCall"]
+    assert kinds == ["AssistantText", "ToolCall", "ToolResult"]
+
+
+def test_gate_tool_call_is_answered_in_the_log(store: StateStore) -> None:
+    # submit_plan ends the turn and has no natural result, but a tool call
+    # left dangling makes the next render invalid for the provider.
+    client = ScriptedClient([[plan_call("the plan")]])
+    LLMPlanningAgent(client, store, ROLES).plan("arch", "demo")
+    events = [e.event for e in store.read_events()]
+    assert [type(e).__name__ for e in events] == ["ToolCall", "ToolResult"]
+    assert events[1].call_id == events[0].call_id
+    assert "posted for approval" in events[1].result
 
 
 def test_prose_only_turn_retries_once_then_raises(store: StateStore) -> None:
@@ -170,7 +188,13 @@ def test_use_project_produces_llm_plan_and_transcript(tmp_path: Path) -> None:
 
     events = [e.event for e in store.read_events()]
     kinds = [type(e).__name__ for e in events]
-    assert kinds == ["UserMessage", "ToolCall", "PlanSubmitted", "GateOpened"]
+    assert kinds == [
+        "UserMessage",
+        "ToolCall",
+        "ToolResult",
+        "PlanSubmitted",
+        "GateOpened",
+    ]
     submitted = next(e for e in events if isinstance(e, PlanSubmitted))
     assert submitted.model == "anthropic/claude-opus-5"  # pinned planner
 
@@ -195,6 +219,53 @@ def test_planning_failure_gates_to_await_input(tmp_path: Path) -> None:
     assert transport.outbound[-1].kind == "question"
 
 
+def test_operator_can_recover_from_a_planning_failure(tmp_path: Path) -> None:
+    # Regression: AWAIT_INPUT reached by any path other than ask_human used to
+    # record no pending item, so every operator reply was refused with "no
+    # pending question" and the agent was stuck for good.
+    prose = [AssistantText(text="prose", model="m")]
+    client = ScriptedClient([prose, prose, [plan_call("recovered plan")]])
+    _, store, transport = build(tmp_path, client)
+    transport.receive("don", "use demo")
+    assert store.get_state().state == "AWAIT_INPUT"
+
+    transport.receive("don", "try again, keep it short")
+
+    assert store.get_state().state == AWAIT_ARCH_SIGNOFF
+    assert "recovered plan" in transport.outbound[-1].text
+
+
+def test_operator_can_recover_after_a_stopped_milestone(tmp_path: Path) -> None:
+    class StoppedExecutor:
+        def run(self, milestone: int) -> Any:
+            from nisse_dev.executor import ExecutorOutcome
+
+            return ExecutorOutcome("escalate", "tests failed twice")
+
+    store = StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+    transport = InMemoryTransport()
+    client = ScriptedClient([[plan_call("p1")], [plan_call("p2")]])
+    orch = Orchestrator(
+        store,
+        transport,
+        LLMPlanningAgent(client, store, ROLES),
+        operators={"don"},
+        roles=ROLES,
+        executor=StoppedExecutor(),
+    )
+    orch.start()
+    transport.receive("don", "use demo")
+    transport.receive("don", "approve")
+    transport.receive("don", "approve")
+    transport.receive("don", "start milestone 1")
+    assert store.get_state().state == "AWAIT_INPUT"
+
+    transport.receive("don", "ok, I fixed the dependency")
+
+    assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF
+    assert "start milestone 1" in transport.outbound[-1].text
+
+
 def test_rejection_feedback_reaches_the_planner_via_the_log(
     tmp_path: Path,
 ) -> None:

nisse-dev/tests/test_render.py

@@ -71,6 +71,33 @@ def test_compaction_replaces_its_range_only() -> None:
     assert contents == ["kept", "[summary of earlier conversation]\nthe old stuff"]
 
 
+def test_tool_call_without_a_result_gets_a_placeholder() -> None:
+    # Providers reject a dangling tool_use; this happens after a crash (A3)
+    # or with turn-ending gate tools like submit_plan.
+    events = stored(
+        [
+            UserMessage(text="go", source="m:1"),
+            ToolCall(call_id="call-x", tool="submit_plan", arguments={}),
+        ]
+    )
+    body = OpenAIChatRenderer().render(events, tools=None, model="q")
+    assert [m["role"] for m in body["messages"]] == ["user", "assistant", "tool"]
+    assert body["messages"][2]["tool_call_id"] == "call-x"
+    assert body["messages"][2]["content"] == "[no result recorded]"
+
+
+def test_answered_tool_call_gets_no_placeholder() -> None:
+    events = stored(
+        [
+            ToolCall(call_id="call-x", tool="run_command", arguments={}),
+            ToolResult(call_id="call-x", result="ok", success=True),
+        ]
+    )
+    body = OpenAIChatRenderer().render(events, tools=None, model="q")
+    assert [m["role"] for m in body["messages"]] == ["assistant", "tool"]
+    assert body["messages"][1]["content"] == "ok"
+
+
 def test_gate_event_renders_as_context() -> None:
     events = stored([GateOpened(gate="arch", reason="plan submitted (rev-1)")])
     body = OpenAIChatRenderer().render(events, tools=None, model="q")

nisse-dev/tests/test_tools.py

@@ -0,0 +1,129 @@
+import os
+from pathlib import Path
+
+import pytest
+
+from nisse_dev.tools import EXECUTOR_TOOLS, MAX_OUTPUT_CHARS, Workspace
+
+
+@pytest.fixture
+def ws(tmp_path: Path) -> Workspace:
+    return Workspace(tmp_path / "work", timeout_s=10)
+
+
+def test_manifest_is_small_and_flat() -> None:
+    # A15: fewer tools, flatter arguments for the local executor.
+    names = [t["function"]["name"] for t in EXECUTOR_TOOLS]
+    assert names == ["read_file", "write_file", "list_dir", "run_command"]
+    for tool in EXECUTOR_TOOLS:
+        props = tool["function"]["parameters"]["properties"]
+        assert all(p["type"] == "string" for p in props.values())
+
+
+def test_write_then_read_round_trip(ws: Workspace) -> None:
+    assert ws.write_file("src/app.py", "print('hi')\n").success
+    outcome = ws.read_file("src/app.py")
+    assert outcome.success
+    assert outcome.result == "print('hi')\n"
+
+
+def test_relative_paths_land_in_the_workspace(ws: Workspace) -> None:
+    ws.write_file("notes.txt", "x")
+    assert (ws.root / "notes.txt").exists()
+
+
+def test_read_missing_file_is_a_failed_result_not_an_exception(
+    ws: Workspace,
+) -> None:
+    outcome = ws.read_file("nope.txt")
+    assert not outcome.success
+    assert "could not read" in outcome.result
+
+
+def test_list_dir(ws: Workspace) -> None:
+    ws.write_file("a.txt", "a")
+    ws.write_file("sub/b.txt", "b")
+    outcome = ws.list_dir(".")
+    assert outcome.success
+    assert outcome.result.splitlines() == ["a.txt", "sub/"]
+
+
+def test_list_empty_dir(ws: Workspace) -> None:
+    assert ws.list_dir(".").result == "(empty)"
+
+
+def test_run_command_success_reports_exit_code_and_stdout(ws: Workspace) -> None:
+    outcome = ws.run_command("echo hello")
+    assert outcome.success
+    assert "exit code: 0" in outcome.result
+    assert "hello" in outcome.result
+
+
+def test_nonzero_exit_is_a_result_not_an_error(ws: Workspace) -> None:
+    outcome = ws.run_command("exit 3")
+    assert not outcome.success  # feeds the A12 failure count
+    assert "exit code: 3" in outcome.result
+
+
+def test_command_runs_in_the_workspace(ws: Workspace) -> None:
+    ws.write_file("marker.txt", "here")
+    assert "marker.txt" in ws.run_command("ls").result
+
+
+def test_command_timeout_is_reported(tmp_path: Path) -> None:
+    ws = Workspace(tmp_path / "work", timeout_s=1)
+    outcome = ws.run_command("sleep 30")
+    assert not outcome.success
+    assert "timed out" in outcome.result
+
+
+def _is_running(pid: int) -> bool:
+    """os.kill(pid, 0) is not enough: a killed orphan stays a zombie until
+    something reaps it, and no init does that inside a CI container. Read the
+    process state instead."""
+    try:
+        stat = Path(f"/proc/{pid}/stat").read_text()
+    except FileNotFoundError:
+        return False
+    # State is the first field after the parenthesised comm.
+    return stat.rsplit(")", 1)[1].split()[0] != "Z"
+
+
+def test_timeout_kills_the_whole_process_group(tmp_path: Path) -> None:
+    ws = Workspace(tmp_path / "work", timeout_s=1)
+    pidfile = ws.root / "child.pid"
+    # A backgrounded grandchild must not outlive the timeout.
+    ws.run_command(f"(sleep 30 & echo $! > {pidfile}); sleep 30")
+    assert pidfile.exists()
+    pid = int(pidfile.read_text().strip())
+    assert _is_running(os.getpid()), "liveness check is broken, not the kill"
+    assert not _is_running(pid)
+
+
+def test_agent_credentials_are_not_inherited(
+    ws: Workspace, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    monkeypatch.setenv("NISSE_LLM_TOKEN", "sk-secret")
+    monkeypatch.setenv("PATH", os.environ["PATH"])
+    outcome = ws.run_command("echo [${NISSE_LLM_TOKEN}]")
+    assert "sk-secret" not in outcome.result
+    assert "[]" in outcome.result
+
+
+def test_stdin_is_closed_so_prompts_fail_fast(ws: Workspace) -> None:
+    outcome = ws.run_command("read line && echo got")
+    assert not outcome.success
+    assert "got" not in outcome.result
+
+
+def test_long_output_is_truncated(ws: Workspace) -> None:
+    outcome = ws.run_command("printf 'x%.0s' $(seq 1 20000)")
+    assert len(outcome.result) < MAX_OUTPUT_CHARS + 200
+    assert "truncated" in outcome.result
+
+
+def test_dispatch_unknown_tool_and_missing_argument(ws: Workspace) -> None:
+    assert not ws.dispatch("fly", {}).success
+    missing = ws.dispatch("read_file", {})
+    assert not missing.success
+    assert "missing argument" in missing.result