diff --git a/docs/bulk-endpoints.md b/docs/bulk-endpoints.md index d70932f1..eaad1af5 100644 --- a/docs/bulk-endpoints.md +++ b/docs/bulk-endpoints.md @@ -113,8 +113,8 @@ owns it; the node is just currently unreachable. ```json { "results": { - "0": {"ws_id": "d4e5f6...", "name": "csrf-audit", "node_id": "gpu-3"}, - "2": {"ws_id": "f1a2b3...", "name": "xss-audit", "node_id": "gpu-1"} + "0": {"child_ws_id": "d4e5f6...", "name": "csrf-audit", "node_id": "gpu-3"}, + "2": {"child_ws_id": "f1a2b3...", "name": "xss-audit", "node_id": "gpu-1"} }, "denied": [ {"idx": 1, "reason": "skill not found: nonexistent-skill"} diff --git a/docs/coordinator-skills.md b/docs/coordinator-skills.md index e2a1c89d..b00c20f0 100644 --- a/docs/coordinator-skills.md +++ b/docs/coordinator-skills.md @@ -169,11 +169,15 @@ validates ws_id against `parent_ws_id=coord_ws_id` AND the wait into reporting "complete". Pattern: capture each spawn result in the next tool call's input. -The JSON tool-result carries `{"ws_id": "...", "name": "...", +The JSON tool-result carries `{"child_ws_id": "...", "name": "...", "node_id": "...", "routing_strategy": "..."}`; the model should -extract the ws_id and pass it to `inspect_workstream` / -`wait_for_workstream` / `send_to_workstream` / `close_workstream` -verbatim. +extract the `child_ws_id` and pass it as `ws_id` (or in the `ws_ids` +list) to `inspect_workstream` / `wait_for_workstream` / +`send_to_workstream` / `close_workstream` verbatim. The asymmetry +— spawn returns `child_ws_id` but the other tools accept `ws_id` / +`ws_ids` — is intentional: it defuses a coordinator-LLM recency +bias where seeing `ws_id` in a spawn return primed re-spawn loops +instead of progression to the wait phase. A UI that wants human-readable identifiers should render the `name` field and keep the ws_id as the click-through key. diff --git a/tests/test_coordinator_tools.py b/tests/test_coordinator_tools.py index ffa351c9..521728ee 100644 --- a/tests/test_coordinator_tools.py +++ b/tests/test_coordinator_tools.py @@ -215,7 +215,12 @@ def test_spawn_exec_does_not_surface_misleading_status_field(coord_session): summary tempted callers to write ``if result["status"] == "idle"`` which silently never matched. The summary now omits the field entirely; lifecycle state lives on the workstream row and is read - via inspect_workstream.""" + via inspect_workstream. + + Also asserts the return key is ``child_ws_id`` (not ``ws_id``) so + the coordinator LLM doesn't recency-bias toward feeding the spawn + output back into another ``spawn_workstream(ws_id=...)`` call. + """ sess, coord, _ui = coord_session coord.spawn.return_value = { "ws_id": "child-7", @@ -227,8 +232,9 @@ def test_spawn_exec_does_not_surface_misleading_status_field(coord_session): _call_id, output = sess._exec_spawn_workstream(item) body = json.loads(output) assert "status" not in body + assert "ws_id" not in body # The substantive fields are still here. - assert body["ws_id"] == "child-7" + assert body["child_ws_id"] == "child-7" assert body["node_id"] == "node-1" @@ -248,6 +254,10 @@ def test_spawn_batch_exec_does_not_surface_misleading_status_field(coord_session body = json.loads(output) assert "0" in body["results"] assert "status" not in body["results"]["0"] + # Per-result entries surface ``child_ws_id``, not ``ws_id`` — same + # recency-bias rationale as the spawn_workstream test above. + assert body["results"]["0"]["child_ws_id"] == "c-x" + assert "ws_id" not in body["results"]["0"] def test_spawn_exec_surfaces_client_error(coord_session): @@ -1410,9 +1420,13 @@ def test_spawn_batch_exec_serialises_spawns_and_returns_results(coord_session): assert body["denied"] == [] # Keyed by input index (stringified). assert set(body["results"].keys()) == {"0", "1", "2"} - assert body["results"]["0"]["ws_id"] == "child-0" + assert body["results"]["0"]["child_ws_id"] == "child-0" assert body["results"]["1"]["node_id"] == "n-1" - assert body["results"]["2"]["ws_id"] == "child-2" + assert body["results"]["2"]["child_ws_id"] == "child-2" + # Confirm we don't leak the old ``ws_id`` key alongside the new + # ``child_ws_id`` — see test_spawn_exec_does_not_surface_misleading_status_field + # for the rationale on the rename. + assert "ws_id" not in body["results"]["0"] def test_spawn_batch_exec_surfaces_per_item_errors_in_denied(coord_session): diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 79429087..43da1cd7 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -7005,9 +7005,9 @@ class ChatSession: msg = f"Error: {result['error']}" self._report_tool_result(call_id, "spawn_workstream", msg, is_error=True) return call_id, msg - # Successful spawn — surface ws_id + node_id + name + routing - # strategy so the coordinator can follow up with inspect / send - # and explain why a given node was chosen. ``status`` was + # Successful spawn — surface child_ws_id + node_id + name + + # routing strategy so the coordinator can follow up with inspect + # / send and explain why a given node was chosen. ``status`` was # historically included but it was the routing-proxy's HTTP # code (always 200 on this branch); the absence of an # ``error`` field is the success signal. Dropped here to @@ -7018,7 +7018,12 @@ class ChatSession: # ``inspect_workstream``. summary = json.dumps( { - "ws_id": result.get("ws_id"), + # Key is ``child_ws_id`` (not ``ws_id``) so the coordinator + # LLM doesn't recency-bias toward feeding the spawn-return + # straight back into another ``spawn_workstream(ws_id=...)`` + # call. On large fan-outs this cascaded into self-inflicted + # re-spawn loops instead of progressing to ``wait_for_workstream``. + "child_ws_id": result.get("ws_id"), "name": result.get("name"), "node_id": result.get("node_id"), "routing_strategy": result.get("routing_strategy"), @@ -7156,7 +7161,9 @@ class ChatSession: denied.append({"idx": idx, "reason": "spawn returned no ws_id"}) continue results[str(idx)] = { - "ws_id": ws_id, + # ``child_ws_id`` (not ``ws_id``) — see the matching + # comment in ``_exec_spawn_workstream``. + "child_ws_id": ws_id, "name": result.get("name", ""), "node_id": result.get("node_id", ""), # ``status`` deliberately omitted — see the matching diff --git a/turnstone/tools/spawn_batch.json b/turnstone/tools/spawn_batch.json index 664c37a3..5bf16e5e 100644 --- a/turnstone/tools/spawn_batch.json +++ b/turnstone/tools/spawn_batch.json @@ -1,6 +1,6 @@ { "name": "spawn_batch", - "description": "Create up to 10 child workstreams in one call. Serialised in input order so sibling ordering (by `created_at`) is deterministic. Returns `{results: {idx: {ws_id, name, node_id}}, denied: [{idx, reason}]}` — `results` keyed by stringified input-array index, `denied` collects per-item validation / spawn failures. For >10 children make multiple calls (the batch hard-errors rather than truncating). Pair with `wait_for_workstream(ws_ids=[...], mode='all')` to synthesise the N outputs once every child has finished. Lifecycle state at spawn isn't returned — call inspect_workstream if you need it.", + "description": "Create up to 10 child workstreams in one call. Serialised in input order so sibling ordering (by `created_at`) is deterministic. Returns `{results: {idx: {child_ws_id, name, node_id}}, denied: [{idx, reason}]}` — `results` keyed by stringified input-array index, `denied` collects per-item validation / spawn failures. Collect the `child_ws_id` values and pass them as a list to `wait_for_workstream(ws_ids=[...], mode='all')` to synthesise the N outputs once every child has finished. For >10 children make multiple calls (the batch hard-errors rather than truncating). Lifecycle state at spawn isn't returned — call inspect_workstream if you need it.", "parameters": { "type": "object", "properties": { diff --git a/turnstone/tools/spawn_workstream.json b/turnstone/tools/spawn_workstream.json index e011c5a6..7c40b6b4 100644 --- a/turnstone/tools/spawn_workstream.json +++ b/turnstone/tools/spawn_workstream.json @@ -1,6 +1,6 @@ { "name": "spawn_workstream", - "description": "Create a new child workstream, optionally dispatching an initial message. Kick off a focused sub-task on a different skill / model / node while the coordinator stays in charge. The child runs independently — drive it with send_to_workstream / inspect_workstream / close_workstream. Returns `{ws_id, name, node_id, routing_strategy}`. `routing_strategy` is `rendezvous` (default placement on the live-node set), `target_node` (your hint was honored), or `resume` (rebound to a still-alive prior owner on rehydrate). The returned `node_id` is the spawn-time binding; subsequent ops re-route via rendezvous over the live-node set, so a node join or drop after spawn can shift the active owner. Conversation state lives in storage (the new owner rehydrates lazily). Don't cache `node_id` for long-running callbacks — re-read with inspect_workstream. Lifecycle state (idle / running / etc.) is not in this response — read it via inspect_workstream.", + "description": "Create a new child workstream, optionally dispatching an initial message. Kick off a focused sub-task on a different skill / model / node while the coordinator stays in charge. The child runs independently — drive it with send_to_workstream / inspect_workstream / close_workstream. Returns `{child_ws_id, name, node_id, routing_strategy}` — pass `child_ws_id` into `wait_for_workstream(ws_ids=[...])` and the other `ws_id`-taking tools. `routing_strategy` is `rendezvous` (default placement on the live-node set), `target_node` (your hint was honored), or `resume` (rebound to a still-alive prior owner on rehydrate). The returned `node_id` is the spawn-time binding; subsequent ops re-route via rendezvous over the live-node set, so a node join or drop after spawn can shift the active owner. Conversation state lives in storage (the new owner rehydrates lazily). Don't cache `node_id` for long-running callbacks — re-read with inspect_workstream. Lifecycle state (idle / running / etc.) is not in this response — read it via inspect_workstream.", "parameters": { "type": "object", "properties": {