Add the operator kill switch, stop (A4)
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 49s
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 49s
`stop` could not have worked by adding the command alone. Planning turns, milestones and review responses ran synchronously on the thread that delivered the message, so the Matrix sync loop read nothing until the work finished. The state store was also opened on the main thread and used from the transport and poller threads, which SQLite refuses with its default same-thread check; the in-memory test transport never showed it. A single worker thread now runs every handler and owns the store: - The worker opens the store, so the same-thread check stays on and fails loudly if anything touches the store from elsewhere - Matrix messages are handed to the worker; pollers hand over and wait, so their cursor never passes an unhandled comment - A message that arrives during long work gets an immediate "busy; say 'stop' to halt" reply and is recorded, not applied later in a state the operator was not answering - Tests keep running inline through InlineWorker, which defers a job submitted from inside another as the real worker would queue it `stop` sets a shared Cancel flag straight from the transport's thread. The executor and delegation loops check it before each model call and between tool calls; the planner checks it before each turn. Calls left unrun get a result saying so, so the transcript still renders. A stopped delegation is discarded like a failed one. The orchestrator moves to AWAIT_INPUT and asks why the work was stopped. The reply is recorded for the model, and a stopped planning turn resumes with it as the answer. `stop` with nothing running says so and changes nothing; at a gate it is no longer taken as rejection feedback. Closes #40 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
d2fe3d5642
commit
abb7fa0216
12 files changed
+887
-39
No files matched your search
@@ -3,7 +3,6 @@ delegation harness and the comment poller — were fully built, fully tested,
|
||||
and reachable from nothing but their own tests. Unit tests that construct a
|
||||
component directly cannot catch that; only assembly can."""
|
||||
|
||||
import sqlite3
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
@@ -163,8 +162,7 @@ def test_stop_closes_everything_and_releases_the_wait(
|
||||
|
||||
assert agent.transport.closed
|
||||
agent.wait() # returns immediately rather than hanging
|
||||
with pytest.raises(sqlite3.ProgrammingError):
|
||||
agent.store.get_state() # the store really is closed
|
||||
assert agent.store.closed
|
||||
|
||||
|
||||
def test_poller_factory_ignores_the_agents_own_comments(
|
||||
@@ -268,7 +266,8 @@ def test_the_resolved_model_is_pinned_at_task_start(
|
||||
tmp_path: Path, origin: Path, monkeypatch
|
||||
) -> None:
|
||||
agent = build_offline(tmp_path, origin, monkeypatch)
|
||||
agent.orchestrator._begin_project("greet")
|
||||
# The store belongs to the worker thread, so the work has to run there.
|
||||
agent.worker.call(lambda: agent.orchestrator._begin_project("greet"))
|
||||
|
||||
pinned = agent.orchestrator._data["models"]
|
||||
assert pinned["planner"] == {
|
||||
|
||||
@@ -4,6 +4,7 @@ from typing import Any
|
||||
|
||||
import pytest
|
||||
|
||||
from nisse_dev.cancel import STOPPED_RESULT, Cancel
|
||||
from nisse_dev.canonical import (
|
||||
AssistantText,
|
||||
CanonicalEvent,
|
||||
@@ -134,6 +135,46 @@ def test_abort_discards_the_branch_and_its_work(
|
||||
assert not (repo.root / "half-done.py").exists()
|
||||
|
||||
|
||||
def test_stop_discards_the_branch_and_settles_the_intent(
|
||||
store: StateStore, ws: Workspace, repo: GitRepo
|
||||
) -> None:
|
||||
# A4 stop is treated like a failure, not like an outage: the operator
|
||||
# stopped it for a reason, so nothing is kept for resuming.
|
||||
cancel = Cancel()
|
||||
client = ScriptedClient(
|
||||
[
|
||||
[call("write_file", path="half-done.py", content="oops")],
|
||||
[call("list_dir", path=".")],
|
||||
]
|
||||
)
|
||||
original = client.complete
|
||||
|
||||
def complete_then_stop(*args: Any, **kwargs: Any) -> list[CanonicalEvent]:
|
||||
produced = original(*args, **kwargs)
|
||||
if len(client.calls) == 2:
|
||||
cancel.set() # the operator says stop during turn 2's model call
|
||||
return produced
|
||||
|
||||
client.complete = complete_then_stop # type: ignore[method-assign]
|
||||
result = build(client, store, ws, repo, cancel=cancel).run(
|
||||
brief="b", branch="deleg-stop"
|
||||
)
|
||||
|
||||
assert result.status == "stopped"
|
||||
assert not result.usable
|
||||
assert repo.current_branch() == "main"
|
||||
assert not repo.branch_exists("deleg-stop")
|
||||
assert not (repo.root / "half-done.py").exists()
|
||||
assert store.pending_intents() == []
|
||||
# The unrun call still has a result, so the transcript renders.
|
||||
skipped = [
|
||||
e.event
|
||||
for e in store.read_events()
|
||||
if isinstance(e.event, ToolResult) and e.event.result == STOPPED_RESULT
|
||||
]
|
||||
assert len(skipped) == 1
|
||||
|
||||
|
||||
def test_turn_cap_is_hard(store: StateStore, ws: Workspace, repo: GitRepo) -> None:
|
||||
forever = [[call("list_dir", path=".")] for _ in range(20)]
|
||||
client = ScriptedClient(forever)
|
||||
|
||||
@@ -0,0 +1,356 @@
|
||||
"""The operator's kill switch, `stop` (spec A4)."""
|
||||
|
||||
import io
|
||||
import threading
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
from nisse_dev.cancel import STOPPED_RESULT, Cancel
|
||||
from nisse_dev.canonical import AssistantText, ToolCall, ToolResult, UserMessage
|
||||
from nisse_dev.control import (
|
||||
AWAIT_ARCH_SIGNOFF,
|
||||
AWAIT_INPUT,
|
||||
AWAIT_MILESTONE_SIGNOFF,
|
||||
Orchestrator,
|
||||
PlanningStopped,
|
||||
StubAgent,
|
||||
SubmitPlan,
|
||||
)
|
||||
from nisse_dev.delegation import DelegationResult
|
||||
from nisse_dev.executor import ExecutorOutcome, LLMExecutor
|
||||
from nisse_dev.llm import RoleMap
|
||||
from nisse_dev.oplog import OpLog
|
||||
from nisse_dev.planner import LLMPlanningAgent
|
||||
from nisse_dev.state import StateStore
|
||||
from nisse_dev.tools import ToolOutcome
|
||||
from nisse_dev.transport import InMemoryTransport
|
||||
from nisse_dev.worker import SerialWorker
|
||||
|
||||
ROLES = RoleMap(
|
||||
executor="llama3.2",
|
||||
planner="claude-haiku-4-5",
|
||||
delegate="claude-haiku-4-5",
|
||||
consultant="claude-haiku-4-5",
|
||||
)
|
||||
|
||||
|
||||
def new_store(path: Path) -> StateStore:
|
||||
return StateStore(path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
|
||||
|
||||
|
||||
def user_messages(store: StateStore) -> list[str]:
|
||||
return [
|
||||
e.event.text for e in store.read_events() if isinstance(e.event, UserMessage)
|
||||
]
|
||||
|
||||
|
||||
# --- the orchestrator ------------------------------------------------------
|
||||
|
||||
|
||||
class OperatorDuringWork:
|
||||
"""An executor that hears from the operator mid-milestone, then does what
|
||||
the real loop does at its next tool boundary."""
|
||||
|
||||
def __init__(self, cancel: Cancel, says: str) -> None:
|
||||
self.cancel = cancel
|
||||
self.says = says
|
||||
self.transport: InMemoryTransport | None = None
|
||||
|
||||
def run(self, milestone: int) -> ExecutorOutcome:
|
||||
assert self.transport is not None
|
||||
self.transport.receive("don", self.says)
|
||||
if self.cancel.requested:
|
||||
return ExecutorOutcome("stopped", "during turn 3")
|
||||
return ExecutorOutcome("completed", "done")
|
||||
|
||||
|
||||
def at_start_of_milestone(
|
||||
tmp_path: Path, says: str
|
||||
) -> tuple[StateStore, InMemoryTransport, Cancel]:
|
||||
store = new_store(tmp_path)
|
||||
transport = InMemoryTransport()
|
||||
cancel = Cancel()
|
||||
executor = OperatorDuringWork(cancel, says)
|
||||
executor.transport = transport
|
||||
orch = Orchestrator(
|
||||
store,
|
||||
transport,
|
||||
StubAgent(),
|
||||
operators={"don"},
|
||||
executor=executor,
|
||||
cancel=cancel,
|
||||
)
|
||||
orch.start()
|
||||
for text in ("use demo", "approve", "approve"):
|
||||
transport.receive("don", text)
|
||||
return store, transport, cancel
|
||||
|
||||
|
||||
def test_stop_halts_a_running_milestone_and_asks_why(tmp_path: Path) -> None:
|
||||
store, transport, cancel = at_start_of_milestone(tmp_path, "stop")
|
||||
transport.receive("don", "start milestone 1")
|
||||
|
||||
assert store.get_state().state == AWAIT_INPUT
|
||||
prompt = transport.outbound[-1]
|
||||
assert prompt.kind == "question"
|
||||
assert "stopped milestone 1 (during turn 3)" in prompt.text
|
||||
assert "Why did you stop it?" in prompt.text
|
||||
assert user_messages(store)[-1] == "stop"
|
||||
assert not cancel.requested # the next piece of work is not pre-stopped
|
||||
|
||||
|
||||
def test_the_reason_is_recorded_and_the_plan_stays_approved(tmp_path: Path) -> None:
|
||||
store, transport, _ = at_start_of_milestone(tmp_path, "stop")
|
||||
transport.receive("don", "start milestone 1")
|
||||
transport.receive("don", "it was editing the wrong module")
|
||||
|
||||
assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF
|
||||
# The next run renders this, so the model hears why it was stopped.
|
||||
assert user_messages(store)[-1] == "it was editing the wrong module"
|
||||
assert "start milestone 1" in transport.outbound[-1].text
|
||||
|
||||
|
||||
def test_stop_with_nothing_running_says_so_and_changes_nothing(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
store = new_store(tmp_path)
|
||||
transport = InMemoryTransport()
|
||||
orch = Orchestrator(store, transport, StubAgent(), operators={"don"})
|
||||
orch.start()
|
||||
transport.receive("don", "use demo")
|
||||
transport.receive("don", "stop")
|
||||
|
||||
# At a gate, "stop" is the command, not rejection feedback.
|
||||
assert store.get_state().state == AWAIT_ARCH_SIGNOFF
|
||||
assert orch._data["revs"]["arch"] == 1
|
||||
assert (
|
||||
transport.outbound[-1].text == f"nothing to stop (state={AWAIT_ARCH_SIGNOFF})"
|
||||
)
|
||||
|
||||
|
||||
def test_stop_from_a_non_operator_is_ignored(tmp_path: Path) -> None:
|
||||
_, transport, cancel = at_start_of_milestone(tmp_path, "approve")
|
||||
before = len(transport.outbound)
|
||||
transport.receive("mallory", "stop")
|
||||
assert len(transport.outbound) == before
|
||||
assert not cancel.requested
|
||||
|
||||
|
||||
def test_other_messages_during_work_are_answered_not_acted_on(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
store, transport, _ = at_start_of_milestone(tmp_path, "approve")
|
||||
transport.receive("don", "start milestone 1")
|
||||
|
||||
texts = [m.text for m in transport.outbound]
|
||||
assert "busy (EXECUTING); say 'stop' to halt" in texts
|
||||
# Recorded, as every operator message is, but not applied to anything.
|
||||
assert user_messages(store)[-1] == "approve"
|
||||
|
||||
|
||||
class StopsBeforePlanning:
|
||||
def __init__(self, cancel: Cancel) -> None:
|
||||
self.cancel = cancel
|
||||
self.transport: InMemoryTransport | None = None
|
||||
self.answers: list[str | None] = []
|
||||
|
||||
def plan(
|
||||
self,
|
||||
gate: str,
|
||||
project: str,
|
||||
feedback: str | None = None,
|
||||
answer: str | None = None,
|
||||
) -> SubmitPlan:
|
||||
self.answers.append(answer)
|
||||
if len(self.answers) == 1:
|
||||
assert self.transport is not None
|
||||
self.transport.receive("don", "stop")
|
||||
if self.cancel.requested:
|
||||
raise PlanningStopped("stopped by the operator")
|
||||
return SubmitPlan(f"{gate} plan, after: {answer}")
|
||||
|
||||
|
||||
def test_stop_during_planning_resumes_planning_with_the_reason(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
store = new_store(tmp_path)
|
||||
transport = InMemoryTransport()
|
||||
cancel = Cancel()
|
||||
planner = StopsBeforePlanning(cancel)
|
||||
planner.transport = transport
|
||||
orch = Orchestrator(store, transport, planner, operators={"don"}, cancel=cancel)
|
||||
orch.start()
|
||||
transport.receive("don", "use demo")
|
||||
|
||||
assert store.get_state().state == AWAIT_INPUT
|
||||
assert "stopped planning arch" in transport.outbound[-1].text
|
||||
|
||||
transport.receive("don", "keep it to one module")
|
||||
assert store.get_state().state == AWAIT_ARCH_SIGNOFF
|
||||
assert planner.answers[-1] == "keep it to one module"
|
||||
|
||||
|
||||
# --- on real threads -------------------------------------------------------
|
||||
|
||||
|
||||
class BlocksUntilStopped:
|
||||
def __init__(self, cancel: Cancel) -> None:
|
||||
self.cancel = cancel
|
||||
self.running = threading.Event()
|
||||
|
||||
def run(self, milestone: int) -> ExecutorOutcome:
|
||||
self.running.set()
|
||||
for _ in range(500):
|
||||
if self.cancel.requested:
|
||||
return ExecutorOutcome("stopped", "at a tool boundary")
|
||||
threading.Event().wait(0.01)
|
||||
return ExecutorOutcome("completed", "never stopped")
|
||||
|
||||
|
||||
def test_stop_reaches_work_running_on_the_worker(tmp_path: Path) -> None:
|
||||
# The bug this design fixes: the work used to run on the transport's own
|
||||
# thread, so no message, stop included, was read until it finished.
|
||||
worker = SerialWorker()
|
||||
worker.start()
|
||||
store = worker.call(lambda: new_store(tmp_path))
|
||||
transport = InMemoryTransport()
|
||||
cancel = Cancel()
|
||||
executor = BlocksUntilStopped(cancel)
|
||||
orch = Orchestrator(
|
||||
store,
|
||||
transport,
|
||||
StubAgent(),
|
||||
operators={"don"},
|
||||
executor=executor,
|
||||
worker=worker,
|
||||
cancel=cancel,
|
||||
)
|
||||
worker.call(orch.start)
|
||||
for text in ("use demo", "approve", "approve", "start milestone 1"):
|
||||
transport.receive("don", text)
|
||||
assert executor.running.wait(5)
|
||||
|
||||
transport.receive("don", "status") # delivered while the milestone runs
|
||||
transport.receive("don", "stop")
|
||||
worker.call(lambda: None) # wait for the stop to be recorded
|
||||
|
||||
texts = [m.text for m in transport.outbound]
|
||||
assert "busy (EXECUTING); say 'stop' to halt" in texts
|
||||
assert worker.call(store.get_state).state == AWAIT_INPUT
|
||||
assert "stopped milestone 1 (at a tool boundary)" in texts[-1]
|
||||
worker.call(store.close)
|
||||
worker.stop(timeout=5)
|
||||
|
||||
|
||||
# --- the loops -------------------------------------------------------------
|
||||
|
||||
|
||||
class ScriptedClient:
|
||||
def __init__(self, turns: list[list[Any]], on_call: Any = None) -> None:
|
||||
self.turns = turns
|
||||
self.on_call = on_call
|
||||
self.calls = 0
|
||||
|
||||
def complete(self, events: Any, tools: Any, model: str, system: Any = None):
|
||||
self.calls += 1
|
||||
if self.on_call:
|
||||
self.on_call()
|
||||
return self.turns.pop(0) if self.turns else [AssistantText("done", model)]
|
||||
|
||||
|
||||
class StopsAfterFirstCall:
|
||||
"""A workspace whose first tool call is where the operator says stop."""
|
||||
|
||||
def __init__(self, cancel: Cancel) -> None:
|
||||
self.cancel = cancel
|
||||
self.ran: list[str] = []
|
||||
|
||||
def describe(self) -> str:
|
||||
return "workspace"
|
||||
|
||||
def dispatch(self, tool: str, arguments: dict[str, Any]) -> ToolOutcome:
|
||||
self.ran.append(tool)
|
||||
self.cancel.set()
|
||||
return ToolOutcome("ok", success=True)
|
||||
|
||||
|
||||
def test_executor_stops_before_a_turn_without_calling_the_model(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
cancel = Cancel()
|
||||
cancel.set()
|
||||
client = ScriptedClient([])
|
||||
executor = LLMExecutor(
|
||||
client, new_store(tmp_path), ROLES, StopsAfterFirstCall(cancel), cancel=cancel
|
||||
)
|
||||
outcome = executor.run(1)
|
||||
assert outcome == ExecutorOutcome("stopped", "before turn 1")
|
||||
assert client.calls == 0
|
||||
|
||||
|
||||
def test_executor_stops_between_tool_calls_and_records_the_skipped_ones(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
cancel = Cancel()
|
||||
store = new_store(tmp_path)
|
||||
ws = StopsAfterFirstCall(cancel)
|
||||
calls = [
|
||||
ToolCall(call_id="a", tool="run_command", arguments={"command": "make"}),
|
||||
ToolCall(call_id="b", tool="write_file", arguments={"path": "x"}),
|
||||
]
|
||||
executor = LLMExecutor(ScriptedClient([calls]), store, ROLES, ws, cancel=cancel)
|
||||
outcome = executor.run(1)
|
||||
|
||||
assert outcome == ExecutorOutcome("stopped", "during turn 1")
|
||||
assert ws.ran == ["run_command"]
|
||||
results = {
|
||||
e.event.call_id: e.event.result
|
||||
for e in store.read_events()
|
||||
if isinstance(e.event, ToolResult)
|
||||
}
|
||||
# Every call has a result, so the transcript still renders.
|
||||
assert results == {"a": "ok", "b": STOPPED_RESULT}
|
||||
|
||||
|
||||
class StoppedDelegate:
|
||||
def run(self, *, brief: str, branch: str) -> DelegationResult:
|
||||
return DelegationResult("stopped", "stopped by the operator during turn 2")
|
||||
|
||||
|
||||
def test_a_stopped_delegation_stops_the_milestone(tmp_path: Path) -> None:
|
||||
failing = [
|
||||
[ToolCall(call_id=f"c{n}", tool="run_command", arguments={"command": "x"})]
|
||||
for n in range(2)
|
||||
]
|
||||
|
||||
class Failing:
|
||||
def describe(self) -> str:
|
||||
return "workspace"
|
||||
|
||||
def dispatch(self, tool: str, arguments: dict[str, Any]) -> ToolOutcome:
|
||||
return ToolOutcome("exit 1", success=False)
|
||||
|
||||
executor = LLMExecutor(
|
||||
ScriptedClient(failing),
|
||||
new_store(tmp_path),
|
||||
ROLES,
|
||||
Failing(),
|
||||
delegate=StoppedDelegate(),
|
||||
)
|
||||
outcome = executor.run(1)
|
||||
assert outcome.status == "stopped"
|
||||
assert "during a delegation" in outcome.note
|
||||
|
||||
|
||||
def test_planner_does_not_start_a_turn_once_stopped(tmp_path: Path) -> None:
|
||||
cancel = Cancel()
|
||||
cancel.set()
|
||||
client = ScriptedClient([])
|
||||
planner = LLMPlanningAgent(client, new_store(tmp_path), ROLES, cancel=cancel)
|
||||
try:
|
||||
planner.plan("arch", "demo")
|
||||
except PlanningStopped:
|
||||
pass
|
||||
else:
|
||||
raise AssertionError("planning ran after stop")
|
||||
assert client.calls == 0
|
||||
@@ -0,0 +1,83 @@
|
||||
import io
|
||||
import threading
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from nisse_dev.oplog import OpLog
|
||||
from nisse_dev.state import StateStore
|
||||
from nisse_dev.worker import InlineWorker, SerialWorker
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def worker() -> SerialWorker:
|
||||
w = SerialWorker()
|
||||
w.start()
|
||||
yield w
|
||||
w.stop(timeout=5)
|
||||
|
||||
|
||||
def test_jobs_run_in_order_on_the_worker_thread(worker: SerialWorker) -> None:
|
||||
seen: list[tuple[int, str]] = []
|
||||
for n in range(3):
|
||||
worker.submit(lambda n=n: seen.append((n, threading.current_thread().name)))
|
||||
worker.call(lambda: None) # drains everything queued before it
|
||||
|
||||
assert [n for n, _ in seen] == [0, 1, 2]
|
||||
assert {name for _, name in seen} == {"nisse-worker"}
|
||||
|
||||
|
||||
def test_call_returns_the_result_and_raises_the_error(worker: SerialWorker) -> None:
|
||||
assert worker.call(lambda: 42) == 42
|
||||
|
||||
def boom() -> None:
|
||||
raise ValueError("bad job")
|
||||
|
||||
with pytest.raises(ValueError, match="bad job"):
|
||||
worker.call(boom)
|
||||
|
||||
|
||||
def test_a_failed_job_is_reported_and_the_worker_carries_on() -> None:
|
||||
errors: list[BaseException] = []
|
||||
worker = SerialWorker(on_error=errors.append)
|
||||
worker.start()
|
||||
|
||||
def boom() -> None:
|
||||
raise RuntimeError("lost job")
|
||||
|
||||
worker.submit(boom)
|
||||
assert worker.call(lambda: "still here") == "still here"
|
||||
assert [str(e) for e in errors] == ["lost job"]
|
||||
worker.stop(timeout=5)
|
||||
|
||||
|
||||
def test_call_from_the_worker_itself_runs_inline(worker: SerialWorker) -> None:
|
||||
# Queueing behind itself would deadlock.
|
||||
assert worker.call(lambda: worker.call(lambda: "nested")) == "nested"
|
||||
|
||||
|
||||
def test_the_store_is_usable_only_from_the_thread_that_owns_it(
|
||||
worker: SerialWorker, tmp_path: Path
|
||||
) -> None:
|
||||
store = worker.call(
|
||||
lambda: StateStore(tmp_path / "state.db", oplog=OpLog("t", io.StringIO()))
|
||||
)
|
||||
assert worker.call(store.get_state) is None
|
||||
# The tripwire the worker design keeps on: any other thread fails loudly.
|
||||
with pytest.raises(Exception, match="same thread"):
|
||||
store.get_state()
|
||||
worker.call(store.close)
|
||||
|
||||
|
||||
def test_inline_worker_defers_a_job_submitted_from_inside_a_job() -> None:
|
||||
worker = InlineWorker()
|
||||
order: list[str] = []
|
||||
|
||||
def outer() -> None:
|
||||
order.append("outer start")
|
||||
worker.submit(lambda: order.append("inner"))
|
||||
order.append("outer end")
|
||||
|
||||
worker.submit(outer)
|
||||
assert order == ["outer start", "outer end", "inner"]
|
||||
assert not worker.busy
|
||||
Reference in new issue
Block a user