From c5fe3e70cf7664b8e614dfdbc8328b87bb2811b9 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Tue, 28 Apr 2026 14:13:17 -0700 Subject: [PATCH] fix(coord): list_nodes accepts flat-arg filters too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator's harness shakedown found list_nodes filters silently ignored on every call: list_nodes(os="Linux") → returns ALL 10 nodes list_nodes(has_gpu=true) → returns ALL 10 nodes list_nodes(memory_gb=751) → returns ALL 10 nodes (no node has 751 GiB; should be 0) Storage filter pipeline is fine (pinned by an existing test_list_nodes_filter_uses_natural_value_not_quoted). The bug is upstream in ``_prepare_list_nodes``: it only honoured ``args["filters"]`` (the canonical nested shape). Several models drop the nesting and emit each filter as a top-level kwarg — ``list_nodes(os="Linux")`` instead of ``list_nodes(filters={"os": "Linux"})`` — and the strict prepare silently degraded those calls to "no filter" → full-cluster return. Fix: any top-level kwarg that ISN'T one of the four reserved control parameters (``filters``, ``limit``, ``include_network_detail``, ``include_inactive``) is now treated as a flat filter. Nested entries still win on key collision so the canonical shape stays deterministic. Tool description unchanged so well-behaved models keep using ``filters={...}``; the relaxation is purely receiver-side. Tests: 4818 pass (+5 net). Five new tests pin both shapes plus the collision-precedence rule and the prepare→exec wiring. Ruff + mypy clean. --- tests/test_coordinator_tools.py | 89 +++++++++++++++++++++++++++++++++ turnstone/core/session.py | 35 +++++++++++-- 2 files changed, 121 insertions(+), 3 deletions(-) diff --git a/tests/test_coordinator_tools.py b/tests/test_coordinator_tools.py index 7b85dbff..ea4be919 100644 --- a/tests/test_coordinator_tools.py +++ b/tests/test_coordinator_tools.py @@ -649,6 +649,95 @@ def test_list_nodes_prepare_drops_invalid_filter_types(coord_session): assert item["filters"] == {"arch": "x86_64"} +def test_list_nodes_prepare_accepts_flat_args_as_filters(coord_session): + """The model frequently drops the ``filters`` nesting and passes + each filter as a top-level kwarg (``list_nodes(os="Linux", + has_gpu=true)``). Operators saw this surface during shakedown: + flat-arg calls returned the full cluster because the strict- + nested prepare silently dropped the filter. The relaxed prepare + treats every top-level non-reserved kwarg as a flat filter.""" + sess, _coord, _ui = coord_session + item = sess._prepare_tool( + _tc("list_nodes", {"os": "Linux", "gpu_has_nvidia": True, "memory_gb": 64}) + ) + assert item["filters"] == {"os": "Linux", "gpu_has_nvidia": True, "memory_gb": 64} + + +def test_list_nodes_prepare_reserves_paging_and_visibility_kwargs(coord_session): + """Top-level reserved kwargs (``limit``, ``include_network_detail``, + ``include_inactive``, ``filters``) are control parameters, NOT + filters. A flat call like ``list_nodes(limit=10, os="Linux")`` + must put ``limit`` on the paging path and ``os`` in the + filter dict — not vice-versa.""" + sess, _coord, _ui = coord_session + item = sess._prepare_tool( + _tc( + "list_nodes", + { + "limit": 10, + "include_network_detail": True, + "include_inactive": True, + "os": "Linux", + }, + ) + ) + assert item["limit"] == 10 + assert item["include_network_detail"] is True + assert item["include_inactive"] is True + assert item["filters"] == {"os": "Linux"} + + +def test_list_nodes_prepare_nested_wins_on_key_collision(coord_session): + """When the model accidentally passes the same filter key both + nested AND flat (rare but possible mid-refactor), the canonical + nested form wins so the call is deterministic.""" + sess, _coord, _ui = coord_session + item = sess._prepare_tool( + _tc( + "list_nodes", + { + "filters": {"os": "Linux"}, # canonical + "os": "DifferentOS", # flat — should NOT override + }, + ) + ) + assert item["filters"] == {"os": "Linux"} + + +def test_list_nodes_prepare_mixes_nested_and_flat(coord_session): + """A model can split filters across both shapes. Both contribute + to the final filter set; nested wins only on direct collisions.""" + sess, _coord, _ui = coord_session + item = sess._prepare_tool( + _tc( + "list_nodes", + { + "filters": {"os": "Linux"}, + "gpu_has_nvidia": True, + "memory_gb": 64, + }, + ) + ) + assert item["filters"] == { + "os": "Linux", + "gpu_has_nvidia": True, + "memory_gb": 64, + } + + +def test_list_nodes_exec_dispatches_flat_arg_filters(coord_session): + """End-to-end: flat-arg filters must actually flow through to the + coordinator client's ``list_nodes(filters=...)`` call. The bug + operators reported was the filters being silently dropped on the + way to storage; this test pins the prepare→exec wiring.""" + sess, coord, _ui = coord_session + coord.list_nodes.return_value = {"nodes": [], "truncated": False} + item = sess._prepare_tool(_tc("list_nodes", {"os": "Linux"})) + sess._exec_list_nodes(item) + kwargs = coord.list_nodes.call_args.kwargs + assert kwargs["filters"] == {"os": "Linux"} + + def test_list_nodes_prepare_clamps_limit(coord_session): sess, _coord, _ui = coord_session over = sess._prepare_tool(_tc("list_nodes", {"limit": 9999})) diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 520a0e87..73735dea 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -203,6 +203,15 @@ _VALID_MEMORY_SCOPES: tuple[str, ...] = ("global", "workstream", "user", "coordi # :meth:`ChatSession._implicit_scope_walk`. _IMPLICIT_SCOPE_WALK: tuple[str, ...] = ("workstream", "user", "global") +# ``list_nodes`` reserves four top-level kwargs for control parameters +# (filters / paging / output verbosity / liveness toggle). Anything +# else the model passes at the top level is treated as a flat filter +# entry — see :meth:`ChatSession._prepare_list_nodes`. +_LIST_NODES_RESERVED_ARGS: frozenset[str] = frozenset( + {"filters", "limit", "include_network_detail", "include_inactive"} +) + + # ``tasks`` action classifier — partitions actions into read vs write # so the parallel-batch guard can permit homogeneous batches (all # writes serialise under the per-ws lock and converge to a consistent @@ -6059,15 +6068,35 @@ class ChatSession: def _prepare_list_nodes(self, call_id: str, args: dict[str, Any]) -> dict[str, Any]: if self._coord_client is None: return self._coord_tool_error(call_id, "list_nodes", "coordinator client unavailable") + # Metadata values are JSON-encoded at rest; the client handles + # the encode/decode so preserve the model's natural types (``4`` + # stays an int, ``"gpu"`` stays a string) rather than + # stringifying here. + # + # Two accepted shapes: + # + # list_nodes(filters={"os": "Linux"}) ← canonical, nested + # list_nodes(os="Linux") ← flat top-level args + # + # Several models drop the ``filters`` nesting and emit each + # filter as a top-level kwarg; the strict-nested-only shape + # silently degraded those calls to "no filter" and returned + # the full cluster, which an operator hit during shakedown + # ("``os="DefinitelyNotAnOS"`` returned all 10 nodes"). + # Treating top-level non-reserved args as filters fixes the + # natural mistake without changing the canonical shape; + # nested entries still win on key collision. raw_filters = args.get("filters") - # Metadata values are JSON-encoded at rest; the client handles the - # encode/decode so preserve the model's natural types (``4`` stays - # an int, ``"gpu"`` stays a string) rather than stringifying here. filters: dict[str, Any] = {} if isinstance(raw_filters, dict): for k, v in raw_filters.items(): if isinstance(k, str) and k and isinstance(v, (str, int, float, bool)): filters[k] = v + for k, v in args.items(): + if k in _LIST_NODES_RESERVED_ARGS: + continue + if isinstance(k, str) and k and isinstance(v, (str, int, float, bool)): + filters.setdefault(k, v) try: limit = int(args.get("limit") or 100) except (TypeError, ValueError):