mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
Patrick Buckley
8650370790
* feat(coordinator): audit middleware on routing proxy — phase 2
Adds per-tool-call audit attribution to the multi-node routing proxy
handlers so coordinator → server hops land observable rows in
``audit_events``. Phase 1 preserved the ``src="coordinator"`` claim
through ``_proxy_auth_headers``'s upstream re-mint; this commit
closes the recording side. Was the last real security gap from
phase 1 — an enterprise deployment with ``admin.coordinator``
granted got only the three console-side
``coordinator.{create,close,cancel}`` rows; per-tool-call
attribution was missing.
## Action-naming scheme
route.workstream.create POST /v1/api/route/workstreams/new
route.workstream.send POST /v1/api/route/send
route.workstream.close POST /v1/api/route/workstreams/close
route.workstream.delete POST /v1/api/route/workstreams/delete
route.approve POST /v1/api/route/approve
route.cancel POST /v1/api/route/cancel
route.command POST /v1/api/route/command
route.plan POST /v1/api/route/plan
Action-name conventions documented in ``turnstone/core/audit.py``
module docstring alongside the existing namespaces — the docstring
is now ``<resource>.<verb>`` shaped (non-exhaustive) rather than
trying to enumerate every prefix.
## Recording rules
- ``record_audit()`` fires only on a 2xx upstream response. 4xx/5xx
are observable via ``_record_route``'s metrics path; doubling the
audit-events table size for failure rows would dilute signal
without giving operators much extra value.
- ``detail`` JSON carries ``{src, node_id, coord_ws_id?}`` — ``src``
lands verbatim from ``auth.token_source`` so non-coordinator
origins (``"jwt"``, ``"console-proxy"``) also get attribution;
``coord_ws_id`` only appears when the inbound JWT carried it.
- Wrapped in ``try/except`` + ``log.debug("route.audit_failed", ...)``
defence-in-depth. ``record_audit`` itself is fire-and-forget;
the outer try guards against a programmer error in the call site.
## Routing-proxy specifics
- ``route_create``: emits at the post-multipart/JSON convergence
``if resp.status_code == 200`` block. Both branches set
``audit_ws_id`` correctly — multipart from the query-string ws_id,
JSON from ``body["ws_id"]`` (post-503-retry) or ``body["resume_ws"]``.
- ``route_proxy``: emits the URL-method-mapped action. ``ref`` is
reassigned to ``new_ref`` after a successful 404→cache-refresh
retry so audit attribution uses the retried node, not the failed
first node.
- ``route_workstream_delete``: emits on 2xx using the ws_id from
the request body.
- ``route_attachment_proxy``: out of scope (upstream attachment
endpoints emit their own ``workstream.attachment.*`` rows;
auditing here would double-count).
## Tests
16 new tests in ``tests/test_route_proxy_audit.py`` covering:
- Coordinator-origin emission with full detail payload.
- 502 / 400 / 503-retry-final-node-id paths.
- Parametrised method→action mapping for the 6 ``route_proxy`` URLs.
- Plain-JWT origin (no ``coord_ws_id`` in detail).
- Delete handler 2xx + 502.
- Audit-storage exception swallowed (proxied response unchanged).
- ``auth_storage`` absent → no-op (existing route-handler tests
unaffected).
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4087 passed,
3 deselected (live-backend), 0 failed.
* feat(coordinator): discovery tools and /open parity — list_nodes, list_skills, POST /coordinator/{ws_id}/open
Adds the read-side surface coordinators need to make informed
orchestration decisions plus an explicit rehydration endpoint
matching the server's ``POST /v1/api/workstreams/{ws_id}/open``.
## list_nodes (auto-approved)
``list_nodes(filters={key: value, ...})`` reads ``node_metadata`` via
``storage.filter_nodes_by_metadata`` + ``get_all_node_metadata`` —
one query each, no N+1. Each row carries its full metadata dict so
the coordinator has both auto keys (``arch`` / ``cpu_count`` /
``fqdn`` / ``hostname`` / ``os`` / ``os_release`` / ``python``;
always present) and operator-supplied user keys (``capability`` /
``region`` / ``tenant`` / ``role``) without a second round-trip.
Tool description enumerates the auto keys explicitly so the model
knows what's always available vs deployment-specific.
Storage stores metadata values as JSON-encoded strings (the write
path in ``server.py`` / ``admin.py`` / ``console/server.py`` all go
through ``json.dumps``). The client re-encodes filter values
before the stored-text comparison and decodes stored values before
returning them to the model — so ``{"capability": "gpu"}`` is the
natural form the model uses, not ``{"capability": "\"gpu\""}``.
Ints round-trip as ints.
Returns ``{nodes, truncated}``; ``truncated=True`` when the page
was full.
## list_skills (auto-approved)
``list_skills(category?, tag?, scan_status?, enabled_only?, limit?)``
surfaces the skill registry so coordinators can discover worker
profiles. New storage protocol method ``list_skills_filtered(...)``
on both SQLite and PostgreSQL backends pushes filters into SQL.
``tag`` filter matches against the JSON-array ``tags`` column with
quote-bracketed substring (``%"tag"%``) — quote-safe against
``foo`` vs ``foobar`` collisions on both backends.
Returns ``{skills, truncated}`` with ``name`` / ``category`` /
``tags`` (decoded to list) / ``version`` / ``description`` /
``model`` / ``enabled`` / ``scan_status`` / ``activation`` — the
discovery projection, not the full row.
## POST /v1/api/coordinator/{ws_id}/open
Explicit rehydration endpoint. Lazy ``GET`` rehydration works for
the UI; this gives SDK callers and operators a way to warm a
coordinator without browsing to it. Same ownership / 404-on-
mismatch / correlation-id-masked error semantics as
``coordinator_detail``. Returns ``{ws_id, name, already_loaded?}``.
Registered in ``turnstone/api/console_spec.py`` with a dedicated
``CoordinatorOpenResponse`` Pydantic model so the OpenAPI schema
matches the wire shape.
## Tests
- ``tests/test_storage_skills_filtered.py`` — 8 cases validated on
BOTH SQLite and PostgreSQL backends (``pytest --storage-backend
postgresql``). Covers no-filter ordering, category exact-match,
tag quote-safety (``"foo"`` matches ``["foo","bar"]`` but not
``["foobar"]``), scan_status, enabled_only, limit, AND semantics,
empty result.
- ``tests/test_coordinator_client.py`` — 11 new cases covering
node/skill shape decoding, JSON-encoded filter round-trip (the
``"gpu"`` vs ``'"gpu"'`` case), int filter encoding, truncation,
no-match empty, no N+1 (``get_prompt_template`` /
``get_node_metadata`` call counts asserted zero).
- ``tests/test_coordinator_tools.py`` — 11 new cases for
``_prepare``/``_exec`` dispatch, filter type-drop, limit clamping
(``limit=0`` falls back to 100, negatives clamp to 1),
truncation-signal summary.
- ``tests/test_coordinator_endpoints.py`` — 8 new cases for
``/open``: ``already_loaded`` on in-memory hit, 404 on ownership
mismatch, lazy rehydrate on miss, admin bypass, unknown ws_id,
503 on ``coord_mgr`` unavailable, 500 with correlation-id mask on
factory failure, 503 passthrough on ``ValueError``.
- ``tests/test_workstream_kind.py`` / ``test_tools_schema.py``
updated to include ``list_nodes`` and ``list_skills`` in the
disjoint-namespace regression guard and the tool-count check.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4122 passed, 3
deselected (live-backend), 0 failed. Postgres backend storage
tests green (``pytest --storage-backend postgresql
tests/test_storage_skills_filtered.py`` 8 passed).
* feat(coordinator): task_list tool — persistent planning state
Adds a coordinator-only ``task_list`` tool persisted on the
coordinator's own ``workstream_config`` row. Gives coordinators a
scratch surface for work decomposition that survives restarts so the
UI can render planned-vs-done state once the tree view lands.
## Tool surface
``task_list(action, ...)`` with five actions:
- ``list`` auto-approved read. Returns ``{tasks, truncated}``;
truncated=True when the list exceeded the 200-row
page cap.
- ``add`` needs approval. ``title`` required; optional
``status`` and ``child_ws_id``. Title clamped at
200 chars. Capacity cap at 500 tasks — hitting the
cap is an explicit signal to prune done/blocked rows.
- ``update`` needs approval. Mutate by ``task_id``; fields
``title`` / ``status`` / ``child_ws_id`` optional.
- ``remove`` needs approval. Drop by ``task_id``.
- ``reorder`` needs approval. Pass ``task_ids``; validated as an
exact permutation of the current set (rejects
partial, extra, or substituted ids — prevents silent
task loss).
Status enum: ``pending`` / ``in_progress`` / ``done`` / ``blocked``.
``child_ws_id`` links a task to the child workstream spawned for it
(no enforcement; the coordinator owns the relationship).
## Persistence
Stored as a single JSON-envelope value on ``workstream_config`` —
``{"version": 1, "tasks": [...]}``. No new table; the kanban v2
work will supersede this row via a format migration keyed on
``version``. ``_save_task_list`` writes only the ``tasks`` key so
concurrent writers to other ``workstream_config`` keys (e.g. the
admin Settings UI updating ``reasoning_effort``) aren't clobbered
by a read-modify-write on the full row.
## Corrupt-envelope safety
A hand-edited or legacy config row that doesn't parse as the
expected shape logs a warning and returns an empty envelope from
``task_list_get``. Mutators refuse to overwrite corrupt data —
they detect the sentinel and return a clear error so the operator
can inspect or clear the row rather than losing work silently.
## Concurrency
Per-(ws) ``threading.Lock`` cached on the client. The worker
thread is single-threaded for tool execs so this is mostly
defence-in-depth against future maintenance-script call sites.
Cache never grows beyond one entry per coordinator session because
the scope guard short-circuits foreign ``ws_id`` before the lock
is acquired.
## Malformed-JSON recovery
``_prepare_tool`` fallback-1 regex-extract allowlist extended with
``action`` / ``status`` / ``task_id`` / ``title`` (alphabetized) so
slightly-malformed ``task_list`` calls get the same
self-correction behaviour as the other coordinator tools.
## Tests
- ``tests/test_coordinator_client.py`` — 15 new cases covering:
fresh-envelope shape, add/get roundtrip, empty-title + invalid-
status rejection, 200-char title clamp, update by id + missing
id, remove semantics, reorder permutation validation (partial +
extra + wrong id + valid), cross-ws scope violation, corrupt-
JSON read recovery, corrupt-envelope write refusal (all four
mutators), 500-task capacity cap, workstream_config key
preservation across ``_save_task_list``.
- ``tests/test_coordinator_tools.py`` — 12 new cases covering the
dispatch layer: list auto-approved, each mutating action needs
approval, unknown-action / missing-required-arg errors, list
returns tasks, page-cap at 200 with truncated signal, add
dispatches to client, reorder surfaces permutation error,
remove-not-found.
- ``tests/test_tools_schema.py`` / ``tests/test_workstream_kind.py``
extend the tool-count + disjoint-namespace + primary-key
regression guards with ``task_list``.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` 4148 passed,
3 deselected (live-backend), 0 failed.
772 lines
28 KiB
Python
772 lines
28 KiB
Python
"""Tests for the coordinator prepare/exec dispatch on ChatSession.
|
|
|
|
We construct a ChatSession with ``kind="coordinator"`` and a mocked
|
|
``CoordinatorClient``, then drive ``_prepare_tool`` directly with tool
|
|
call dicts matching the shape the provider layer produces. This is a
|
|
unit-level test of the dispatch plumbing — end-to-end flows land in
|
|
Phase D's test_coordinator_end_to_end.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
from typing import Any
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
from turnstone.core.session import ChatSession
|
|
from turnstone.prompts import ClientType
|
|
|
|
|
|
class _StubUI:
|
|
"""Minimal SessionUI that records signals without doing anything with them."""
|
|
|
|
def __init__(self) -> None:
|
|
self._user_id = "user-1"
|
|
self.infos: list[str] = []
|
|
self.errors: list[str] = []
|
|
self.tool_results: list[tuple[str, str, str, bool]] = []
|
|
|
|
def on_info(self, msg: str) -> None:
|
|
self.infos.append(msg)
|
|
|
|
def on_error(self, msg: str) -> None:
|
|
self.errors.append(msg)
|
|
|
|
def on_tool_result(self, call_id: str, name: str, output: str, is_error: bool = False) -> None:
|
|
self.tool_results.append((call_id, name, output, is_error))
|
|
|
|
# Other SessionUI methods — only stubs, not exercised here.
|
|
def on_turn_start(self) -> None:
|
|
pass
|
|
|
|
def on_turn_end(self) -> None:
|
|
pass
|
|
|
|
def on_stream_start(self) -> None:
|
|
pass
|
|
|
|
def on_stream_end(self) -> None:
|
|
pass
|
|
|
|
def on_message_delta(self, delta: str) -> None:
|
|
pass
|
|
|
|
def on_reasoning_delta(self, delta: str) -> None:
|
|
pass
|
|
|
|
def on_tool_call(self, call_id: str, name: str, header: str, preview: str) -> None:
|
|
pass
|
|
|
|
def on_completion(self, content: str) -> None:
|
|
pass
|
|
|
|
def on_attention(self, header: str, preview: str = "") -> None:
|
|
pass
|
|
|
|
def wait_for_approval(
|
|
self,
|
|
call_id: str,
|
|
name: str,
|
|
header: str,
|
|
preview: str,
|
|
*,
|
|
label: str = "",
|
|
) -> tuple[bool, str | None]:
|
|
return True, None
|
|
|
|
|
|
@pytest.fixture
|
|
def coord_session(monkeypatch):
|
|
"""Build a coordinator ChatSession with a mocked CoordinatorClient.
|
|
|
|
Patches heavyweight init steps (_load_skills, _init_system_messages,
|
|
_save_config) to keep the test fast + isolated from the storage
|
|
registry.
|
|
"""
|
|
monkeypatch.setattr(ChatSession, "_load_skills", lambda self: None)
|
|
monkeypatch.setattr(ChatSession, "_init_system_messages", lambda self: None)
|
|
monkeypatch.setattr(ChatSession, "_save_config", lambda self: None)
|
|
|
|
ui = _StubUI()
|
|
coord_client = MagicMock()
|
|
sess = ChatSession(
|
|
client=MagicMock(),
|
|
model="gpt-test",
|
|
ui=ui, # type: ignore[arg-type]
|
|
instructions=None,
|
|
temperature=0.0,
|
|
max_tokens=1024,
|
|
tool_timeout=30,
|
|
context_window=16384,
|
|
ws_id="coord-1",
|
|
user_id="user-1",
|
|
client_type=ClientType.WEB,
|
|
kind="coordinator",
|
|
coord_client=coord_client,
|
|
)
|
|
return sess, coord_client, ui
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Tool set shape
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_coordinator_session_uses_coordinator_tools(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
names = {t["function"]["name"] for t in sess._tools}
|
|
assert names == {
|
|
"spawn_workstream",
|
|
"inspect_workstream",
|
|
"send_to_workstream",
|
|
"close_workstream",
|
|
"delete_workstream",
|
|
"list_workstreams",
|
|
"list_nodes",
|
|
"list_skills",
|
|
"task_list",
|
|
}
|
|
# Sub-agent tool sets are zeroed on coordinator sessions.
|
|
assert sess._task_tools == []
|
|
assert sess._agent_tools == []
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helper: build a ChatCompletion-style tool_call dict
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _tc(name: str, args: dict[str, Any], call_id: str = "call-1") -> dict[str, Any]:
|
|
return {
|
|
"id": call_id,
|
|
"type": "function",
|
|
"function": {"name": name, "arguments": json.dumps(args)},
|
|
}
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# spawn_workstream
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_spawn_prepare_allows_empty_initial_message(coord_session):
|
|
"""Empty initial_message creates an idle child — matches tool JSON advertisement."""
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("spawn_workstream", {"initial_message": ""}))
|
|
assert "error" not in item
|
|
assert item["needs_approval"] is True
|
|
assert "idle workstream" in item["header"]
|
|
assert item["initial_message"] == ""
|
|
|
|
|
|
def test_spawn_prepare_needs_approval(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc("spawn_workstream", {"initial_message": "do a thing", "skill": "s"})
|
|
)
|
|
assert item["needs_approval"] is True
|
|
assert item["execute"].__func__ is ChatSession._exec_spawn_workstream
|
|
assert item["skill"] == "s"
|
|
|
|
|
|
def test_spawn_exec_calls_client_and_returns_summary(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.spawn.return_value = {
|
|
"ws_id": "child-7",
|
|
"name": "c",
|
|
"node_id": "node-1",
|
|
"status": 200,
|
|
}
|
|
item = sess._prepare_tool(_tc("spawn_workstream", {"initial_message": "hi"}))
|
|
call_id, output = sess._exec_spawn_workstream(item)
|
|
coord.spawn.assert_called_once()
|
|
_, kwargs = coord.spawn.call_args
|
|
assert kwargs["parent_ws_id"] == "coord-1"
|
|
assert kwargs["user_id"] == "user-1"
|
|
assert kwargs["initial_message"] == "hi"
|
|
assert call_id == "call-1"
|
|
assert "child-7" in output
|
|
|
|
|
|
def test_spawn_exec_surfaces_client_error(coord_session):
|
|
sess, coord, ui = coord_session
|
|
coord.spawn.return_value = {"error": "upstream unreachable", "status": 502}
|
|
item = sess._prepare_tool(_tc("spawn_workstream", {"initial_message": "hi"}))
|
|
_call_id, output = sess._exec_spawn_workstream(item)
|
|
assert "upstream unreachable" in output
|
|
# UI got an error result
|
|
assert ui.tool_results[-1][3] is True # is_error
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# inspect_workstream
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_inspect_prepare_is_auto_approved(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("inspect_workstream", {"ws_id": "child-x", "message_limit": 5}))
|
|
assert item["needs_approval"] is False
|
|
assert item["execute"].__func__ is ChatSession._exec_inspect_workstream
|
|
assert item["message_limit"] == 5
|
|
|
|
|
|
def test_inspect_prepare_requires_ws_id(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("inspect_workstream", {}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_inspect_prepare_clamps_message_limit(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("inspect_workstream", {"ws_id": "x", "message_limit": 10000}))
|
|
assert item["message_limit"] == 200 # clamped
|
|
|
|
|
|
def test_inspect_exec_dispatches_to_client(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.inspect.return_value = {
|
|
"ws_id": "child-x",
|
|
"state": "idle",
|
|
"messages": [],
|
|
"verdicts": [],
|
|
}
|
|
item = sess._prepare_tool(_tc("inspect_workstream", {"ws_id": "child-x"}))
|
|
_call_id, output = sess._exec_inspect_workstream(item)
|
|
coord.inspect.assert_called_once_with("child-x", message_limit=20)
|
|
assert "child-x" in output
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# send_to_workstream
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_send_prepare_needs_approval(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("send_to_workstream", {"ws_id": "x", "message": "hello"}))
|
|
assert item["needs_approval"] is True
|
|
|
|
|
|
def test_send_prepare_rejects_empty_message(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("send_to_workstream", {"ws_id": "x", "message": ""}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_send_exec_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.send.return_value = {"status": 200}
|
|
item = sess._prepare_tool(_tc("send_to_workstream", {"ws_id": "x", "message": "hi"}))
|
|
_call_id, output = sess._exec_send_to_workstream(item)
|
|
coord.send.assert_called_once_with("x", "hi")
|
|
assert "x" in output
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# close_workstream
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_close_prepare_needs_approval(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("close_workstream", {"ws_id": "x"}))
|
|
assert item["needs_approval"] is True
|
|
|
|
|
|
def test_close_exec_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.close_workstream.return_value = {"status": 200}
|
|
item = sess._prepare_tool(_tc("close_workstream", {"ws_id": "x"}))
|
|
_call_id, output = sess._exec_close_workstream(item)
|
|
# Default (no reason) — kwargs carry empty reason through the call.
|
|
coord.close_workstream.assert_called_once_with("x", reason="")
|
|
parsed = json.loads(output)
|
|
assert parsed["closed"] is True
|
|
assert "reason" not in parsed # omitted when empty
|
|
|
|
|
|
def test_close_exec_forwards_reason(coord_session):
|
|
"""reason is wired through both CoordinatorClient.close_workstream
|
|
and the tool-result payload so the coordinator's message stream
|
|
records why the close happened."""
|
|
sess, coord, _ui = coord_session
|
|
coord.close_workstream.return_value = {"status": 200}
|
|
item = sess._prepare_tool(_tc("close_workstream", {"ws_id": "x", "reason": "task done"}))
|
|
_call_id, output = sess._exec_close_workstream(item)
|
|
coord.close_workstream.assert_called_once_with("x", reason="task done")
|
|
parsed = json.loads(output)
|
|
assert parsed["reason"] == "task done"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# delete_workstream
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_delete_prepare_needs_approval(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("delete_workstream", {"ws_id": "x"}))
|
|
assert item["needs_approval"] is True
|
|
assert "irreversible" in item["header"].lower()
|
|
|
|
|
|
def test_delete_exec_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.delete.return_value = {"status": 200}
|
|
item = sess._prepare_tool(_tc("delete_workstream", {"ws_id": "x"}))
|
|
_call_id, output = sess._exec_delete_workstream(item)
|
|
coord.delete.assert_called_once_with("x")
|
|
parsed = json.loads(output)
|
|
assert parsed["deleted"] is True
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# list_workstreams
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_list_prepare_is_auto_approved(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("list_workstreams", {}))
|
|
assert item["needs_approval"] is False
|
|
|
|
|
|
def test_list_prepare_defaults_parent_to_self_ws(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("list_workstreams", {}))
|
|
assert item["parent_ws_id"] == "coord-1"
|
|
|
|
|
|
def test_list_prepare_accepts_explicit_parent(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc("list_workstreams", {"parent_ws_id": "other-coord", "state": "idle"})
|
|
)
|
|
assert item["parent_ws_id"] == "other-coord"
|
|
assert item["state"] == "idle"
|
|
|
|
|
|
def test_list_exec_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.list_children.return_value = {
|
|
"children": [
|
|
{"ws_id": "a", "state": "idle"},
|
|
{"ws_id": "b", "state": "running"},
|
|
],
|
|
"truncated": False,
|
|
}
|
|
item = sess._prepare_tool(_tc("list_workstreams", {}))
|
|
_call_id, output = sess._exec_list_workstreams(item)
|
|
coord.list_children.assert_called_once()
|
|
parsed = json.loads(output)
|
|
assert parsed["parent_ws_id"] == "coord-1"
|
|
assert len(parsed["children"]) == 2
|
|
assert parsed["truncated"] is False
|
|
|
|
|
|
def test_list_exec_surfaces_truncated_sentinel(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.list_children.return_value = {
|
|
"children": [{"ws_id": "a", "state": "idle"}],
|
|
"truncated": True,
|
|
}
|
|
item = sess._prepare_tool(_tc("list_workstreams", {}))
|
|
_call_id, output = sess._exec_list_workstreams(item)
|
|
parsed = json.loads(output)
|
|
assert parsed["truncated"] is True
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Defensive guard: missing coord_client
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_prepare_fails_cleanly_when_coord_client_missing(monkeypatch):
|
|
"""If somehow a coordinator-kind session is built without a coord_client,
|
|
prepare methods return an error item rather than NPE."""
|
|
monkeypatch.setattr(ChatSession, "_load_skills", lambda self: None)
|
|
monkeypatch.setattr(ChatSession, "_init_system_messages", lambda self: None)
|
|
monkeypatch.setattr(ChatSession, "_save_config", lambda self: None)
|
|
ui = _StubUI()
|
|
sess = ChatSession(
|
|
client=MagicMock(),
|
|
model="m",
|
|
ui=ui, # type: ignore[arg-type]
|
|
instructions=None,
|
|
temperature=0.0,
|
|
max_tokens=1024,
|
|
tool_timeout=30,
|
|
context_window=16384,
|
|
ws_id="coord-1",
|
|
kind="coordinator",
|
|
coord_client=None,
|
|
)
|
|
for tool, args in (
|
|
("spawn_workstream", {"initial_message": "hi"}),
|
|
("inspect_workstream", {"ws_id": "x"}),
|
|
("send_to_workstream", {"ws_id": "x", "message": "m"}),
|
|
("close_workstream", {"ws_id": "x"}),
|
|
("delete_workstream", {"ws_id": "x"}),
|
|
("list_workstreams", {}),
|
|
("list_nodes", {}),
|
|
("list_skills", {}),
|
|
("task_list", {"action": "list"}),
|
|
):
|
|
item = sess._prepare_tool(_tc(tool, args))
|
|
assert "error" in item, f"{tool} did not error on missing coord_client"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# list_nodes
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_list_nodes_prepare_is_auto_approved(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("list_nodes", {}))
|
|
assert item["needs_approval"] is False
|
|
assert item["filters"] == {}
|
|
assert item["limit"] == 100
|
|
|
|
|
|
def test_list_nodes_prepare_accepts_filters(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc("list_nodes", {"filters": {"arch": "x86_64", "capability": "gpu"}})
|
|
)
|
|
assert item["filters"] == {"arch": "x86_64", "capability": "gpu"}
|
|
|
|
|
|
def test_list_nodes_prepare_drops_invalid_filter_types(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc(
|
|
"list_nodes",
|
|
{"filters": {"arch": "x86_64", "bad": {"nested": "dict"}, "": "empty-key"}},
|
|
)
|
|
)
|
|
# Nested dict values + empty keys are filtered out; string + primitive kept.
|
|
assert item["filters"] == {"arch": "x86_64"}
|
|
|
|
|
|
def test_list_nodes_prepare_clamps_limit(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
over = sess._prepare_tool(_tc("list_nodes", {"limit": 9999}))
|
|
assert over["limit"] == 500
|
|
# limit == 0 falls back to the default (100), not 1 — consistent with
|
|
# the other coordinator list tools' ``int(args.get("limit") or 100)``.
|
|
zero = sess._prepare_tool(_tc("list_nodes", {"limit": 0}))
|
|
assert zero["limit"] == 100
|
|
neg = sess._prepare_tool(_tc("list_nodes", {"limit": -5}))
|
|
assert neg["limit"] == 1 # negative values clamp to 1
|
|
|
|
|
|
def test_list_nodes_exec_dispatches_to_client(coord_session):
|
|
sess, coord, ui = coord_session
|
|
coord.list_nodes.return_value = {
|
|
"nodes": [{"node_id": "n1", "metadata": {"arch": {"value": "x86_64", "source": "auto"}}}],
|
|
"truncated": False,
|
|
}
|
|
item = sess._prepare_tool(_tc("list_nodes", {"filters": {"arch": "x86_64"}}))
|
|
call_id, output = sess._exec_list_nodes(item)
|
|
assert call_id == "call-1"
|
|
parsed = json.loads(output)
|
|
assert parsed["nodes"][0]["node_id"] == "n1"
|
|
assert parsed["truncated"] is False
|
|
coord.list_nodes.assert_called_once_with(filters={"arch": "x86_64"}, limit=100)
|
|
|
|
|
|
def test_list_nodes_exec_surfaces_truncated_sentinel(coord_session):
|
|
sess, coord, ui = coord_session
|
|
coord.list_nodes.return_value = {"nodes": [], "truncated": True}
|
|
item = sess._prepare_tool(_tc("list_nodes", {}))
|
|
_, _ = sess._exec_list_nodes(item)
|
|
# Summary reported to UI carries the "truncated" hint.
|
|
assert any("truncated" in r[2] for r in ui.tool_results)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# list_skills
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_list_skills_prepare_is_auto_approved(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("list_skills", {}))
|
|
assert item["needs_approval"] is False
|
|
assert item["category"] is None
|
|
assert item["tag"] is None
|
|
assert item["scan_status"] is None
|
|
assert item["enabled_only"] is False
|
|
assert item["limit"] == 100
|
|
|
|
|
|
def test_list_skills_prepare_accepts_filters(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc(
|
|
"list_skills",
|
|
{"category": "ops", "tag": "gpu", "scan_status": "clean", "enabled_only": True},
|
|
)
|
|
)
|
|
assert item["category"] == "ops"
|
|
assert item["tag"] == "gpu"
|
|
assert item["scan_status"] == "clean"
|
|
assert item["enabled_only"] is True
|
|
|
|
|
|
def test_list_skills_prepare_tolerates_non_string_filters(coord_session):
|
|
"""A malformed model call with non-string filter values must NOT
|
|
raise AttributeError during ``.strip()`` — the prepare path should
|
|
coerce non-strings to ``None`` and proceed."""
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(
|
|
_tc(
|
|
"list_skills",
|
|
{"category": 42, "tag": ["not", "a", "string"], "scan_status": {"bad": 1}},
|
|
)
|
|
)
|
|
assert "error" not in item
|
|
assert item["category"] is None
|
|
assert item["tag"] is None
|
|
assert item["scan_status"] is None
|
|
|
|
|
|
def test_list_skills_prepare_parses_enabled_only_string_forms(coord_session):
|
|
"""``bool("false")`` is True (non-empty string). The prepare path
|
|
must interpret common string forms the way the model would expect."""
|
|
sess, _coord, _ui = coord_session
|
|
for raw, expected in (
|
|
("true", True),
|
|
("True", True),
|
|
("1", True),
|
|
("false", False),
|
|
("False", False),
|
|
("0", False),
|
|
("", False),
|
|
(True, True),
|
|
(False, False),
|
|
):
|
|
item = sess._prepare_tool(_tc("list_skills", {"enabled_only": raw}))
|
|
assert item.get("enabled_only") is expected, (
|
|
f"enabled_only={raw!r} → {item.get('enabled_only')!r}, expected {expected!r}"
|
|
)
|
|
|
|
|
|
def test_list_skills_exec_dispatches_to_client(coord_session):
|
|
sess, coord, ui = coord_session
|
|
coord.list_skills.return_value = {
|
|
"skills": [{"name": "alpha", "tags": ["gpu"]}],
|
|
"truncated": False,
|
|
}
|
|
item = sess._prepare_tool(_tc("list_skills", {"category": "ops", "tag": "gpu"}))
|
|
call_id, output = sess._exec_list_skills(item)
|
|
assert call_id == "call-1"
|
|
parsed = json.loads(output)
|
|
assert parsed["skills"][0]["name"] == "alpha"
|
|
coord.list_skills.assert_called_once_with(
|
|
category="ops",
|
|
tag="gpu",
|
|
scan_status=None,
|
|
enabled_only=False,
|
|
limit=100,
|
|
)
|
|
|
|
|
|
def test_list_skills_exec_surfaces_truncated_sentinel(coord_session):
|
|
sess, coord, ui = coord_session
|
|
coord.list_skills.return_value = {"skills": [], "truncated": True}
|
|
item = sess._prepare_tool(_tc("list_skills", {}))
|
|
_, _ = sess._exec_list_skills(item)
|
|
assert any("truncated" in r[2] for r in ui.tool_results)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# task_list
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_task_list_list_is_auto_approved(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "list"}))
|
|
assert item["needs_approval"] is False
|
|
assert item["action"] == "list"
|
|
|
|
|
|
def test_task_list_bare_string_fallback_uses_action_primary_key(coord_session):
|
|
"""A model that emits an unquoted ``list`` as the arguments blob
|
|
lands on the ``primary_key=action`` fallback and recovers. Before
|
|
the fix primary_key was ``title`` so the fallback produced
|
|
``{"title": "list"}`` and hit the required-action rejection."""
|
|
sess, _coord, _ui = coord_session
|
|
call = {
|
|
"id": "c1",
|
|
"type": "function",
|
|
"function": {"name": "task_list", "arguments": "list"},
|
|
}
|
|
item = sess._prepare_tool(call)
|
|
assert "error" not in item
|
|
assert item["action"] == "list"
|
|
|
|
|
|
def test_task_list_mutating_actions_need_approval(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
add_item = sess._prepare_tool(_tc("task_list", {"action": "add", "title": "plan"}))
|
|
assert add_item["needs_approval"] is True
|
|
update_item = sess._prepare_tool(
|
|
_tc("task_list", {"action": "update", "task_id": "tsk_1", "status": "done"})
|
|
)
|
|
assert update_item["needs_approval"] is True
|
|
remove_item = sess._prepare_tool(_tc("task_list", {"action": "remove", "task_id": "tsk_1"}))
|
|
assert remove_item["needs_approval"] is True
|
|
reorder_item = sess._prepare_tool(
|
|
_tc("task_list", {"action": "reorder", "task_ids": ["tsk_1"]})
|
|
)
|
|
assert reorder_item["needs_approval"] is True
|
|
|
|
|
|
def test_task_list_unknown_action_errors(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "wat"}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_non_string_action_errors_cleanly(coord_session):
|
|
"""A malformed ``action=42`` must NOT raise AttributeError during
|
|
``.strip().lower()`` — coerce to the empty string and fall through
|
|
to the enum-check error."""
|
|
sess, _coord, _ui = coord_session
|
|
for bad_action in (42, None, ["list"], {"a": 1}, True):
|
|
item = sess._prepare_tool(_tc("task_list", {"action": bad_action}))
|
|
assert "error" in item, f"action={bad_action!r} did not produce a clean error"
|
|
|
|
|
|
def test_task_list_add_rejects_non_string_title_and_status(coord_session):
|
|
"""Add branch: ``title=42`` / ``status=0`` must NOT raise
|
|
AttributeError during ``.strip()``; produce a clean error item."""
|
|
sess, _coord, _ui = coord_session
|
|
for bad in ({"action": "add", "title": 42}, {"action": "add", "title": "ok", "status": 0}):
|
|
item = sess._prepare_tool(_tc("task_list", bad))
|
|
assert "error" in item, f"args={bad!r} did not produce a clean error"
|
|
|
|
|
|
def test_task_list_remove_non_string_task_id_errors_cleanly(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "remove", "task_id": 42}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_add_requires_title(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "add", "title": ""}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_update_requires_task_id(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "update", "status": "done"}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_update_rejects_non_string_field_values(coord_session):
|
|
"""Preview must not diverge from execute: reject non-string field
|
|
values at prepare time rather than silently coercing to None."""
|
|
sess, _coord, _ui = coord_session
|
|
for field in ("title", "status", "child_ws_id"):
|
|
item = sess._prepare_tool(
|
|
_tc("task_list", {"action": "update", "task_id": "t1", field: 42})
|
|
)
|
|
assert "error" in item, f"update with non-string {field} should error"
|
|
|
|
|
|
def test_task_list_update_requires_at_least_one_field(coord_session):
|
|
"""update with only task_id is a no-op — reject to save an approval prompt."""
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "update", "task_id": "t1"}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_remove_requires_task_id(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "remove"}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_reorder_requires_list_of_strings(coord_session):
|
|
sess, _coord, _ui = coord_session
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "reorder", "task_ids": [1, 2]}))
|
|
assert "error" in item
|
|
|
|
|
|
def test_task_list_exec_list_returns_tasks(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_get.return_value = {
|
|
"version": 1,
|
|
"tasks": [{"id": "tsk_1", "title": "do", "status": "pending"}],
|
|
}
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "list"}))
|
|
_, output = sess._exec_task_list(item)
|
|
parsed = json.loads(output)
|
|
assert parsed["tasks"][0]["id"] == "tsk_1"
|
|
assert parsed["truncated"] is False
|
|
|
|
|
|
def test_task_list_exec_list_page_caps_at_200(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_get.return_value = {
|
|
"version": 1,
|
|
"tasks": [{"id": f"tsk_{i}", "title": "x", "status": "pending"} for i in range(250)],
|
|
}
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "list"}))
|
|
_, output = sess._exec_task_list(item)
|
|
parsed = json.loads(output)
|
|
assert len(parsed["tasks"]) == 200
|
|
assert parsed["truncated"] is True
|
|
|
|
|
|
def test_task_list_exec_add_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_add.return_value = {"id": "tsk_new", "title": "plan"}
|
|
item = sess._prepare_tool(
|
|
_tc("task_list", {"action": "add", "title": "plan", "status": "pending"})
|
|
)
|
|
_, _ = sess._exec_task_list(item)
|
|
coord.task_list_add.assert_called_once_with(
|
|
sess._ws_id, title="plan", status="pending", child_ws_id=""
|
|
)
|
|
|
|
|
|
def test_task_list_exec_reorder_surfaces_permutation_error(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_reorder.return_value = {"error": "task_ids must be a permutation..."}
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "reorder", "task_ids": ["wrong"]}))
|
|
_, output = sess._exec_task_list(item)
|
|
parsed = json.loads(output)
|
|
assert "error" in parsed
|
|
|
|
|
|
def test_task_list_exec_remove_passes_client_dict_through(coord_session):
|
|
"""The client returns a dict; exec must pass it through without
|
|
synthesising a generic 'not found' message that would mask corrupt-
|
|
envelope errors from the LLM."""
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_remove.return_value = {
|
|
"error": "task_list envelope is corrupt on disk; refusing to overwrite."
|
|
}
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "remove", "task_id": "x"}))
|
|
_, output = sess._exec_task_list(item)
|
|
parsed = json.loads(output)
|
|
assert "corrupt" in parsed["error"]
|
|
|
|
|
|
def test_task_list_exec_remove_success_dispatches(coord_session):
|
|
sess, coord, _ui = coord_session
|
|
coord.task_list_remove.return_value = {"ok": True, "task_id": "tsk_1"}
|
|
item = sess._prepare_tool(_tc("task_list", {"action": "remove", "task_id": "tsk_1"}))
|
|
_, output = sess._exec_task_list(item)
|
|
parsed = json.loads(output)
|
|
assert parsed.get("ok") is True
|