diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b8ecb6e..b5a9c3c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,21 @@ Earlier stable lines (`stable/1.6`, `stable/1.5`) are frozen. ### Fixed +- **task_agent: sub-tool ids no longer alias across a local model's reused + ids.** A local model that reissues per-response sequential tool-call ids + (`call_0` every turn) made two of a task agent's steps share one id — the + live card collapsed both onto one DOM row while `/history` recall kept them + apart, so the two views disagreed. Sub-tool ids are now minted + `{parent}::r{run}s{step}::{id}`, unique within the session (across an + agent's turns and across concurrent or sequential runs), and that one id + keys the nesting registry, the live rows, recall, and the cancel ledger. + As defensive hardening the agent loop also normalizes its self-built wire + history — projecting the composite ids to short plain tokens + (deterministically, so call/result pairing holds) and legalizing malformed + tool-call arguments, the same two validity passes the main loop already runs + — so an agent's history stays valid even on a backend stricter than the ones + it runs on today. + - **bash tool: never hang on a backgrounded child.** A command that left a long-lived process running (`server &`, a daemon) could wedge the whole workstream forever — the tool read stdout/stderr to EOF, which never arrived diff --git a/tests/test_lowering.py b/tests/test_lowering.py index fd3cd3e6..cfc0d588 100644 --- a/tests/test_lowering.py +++ b/tests/test_lowering.py @@ -12,14 +12,17 @@ this pins the behaviour the old ``_anthropic`` ``pc_tool_ids`` / from __future__ import annotations import json +import re from typing import Any from turnstone.core.lowering import ( CANCELLED_TOOL_RESULT, _find_orphaned_tool_calls, + legalize_tool_call_ids, repair_wire_messages, sanitize_tool_call_arguments, tool_args_preview, + wire_safe_tool_call_id, wire_valid_arguments, ) @@ -340,3 +343,76 @@ def test_pipeline_every_emitted_arguments_is_a_json_object() -> None: for m in out: for tc in m.get("tool_calls", []): assert isinstance(json.loads(tc["function"]["arguments"]), dict) + + +# --------------------------------------------------------------------------- # +# legalize_tool_call_ids — the agent-wire id projection (defensive hardening). +# +# Sub-agent tool ids are minted "{parent}::r{run}s{step}::{provider_id}" for +# session-unique correlation (registry / DOM / recall). These replay fine on +# the lenient anthropic-compatible deployment, but the pass projects the long, +# "::"-containing ids to a deterministic wire-safe token on the transient wire +# copy so an agent's self-built history stays valid on a hypothetically +# stricter backend, keeping assistant call / tool result pairing intact. +# --------------------------------------------------------------------------- # +def test_wire_safe_tool_call_id_passthrough_when_legal() -> None: + # Provider-issued ids (their own echo) and uuid-filled ids are untouched. + for tc_id in ("call_9dSVOCr8sPbk3f0oJU8IruBP", "toolu_01ABCdef", "call_" + "a" * 32): + assert wire_safe_tool_call_id(tc_id) is tc_id + + +def test_wire_safe_tool_call_id_rewrites_illegal_charset_and_length() -> None: + minted = "call_0::r1s1::call_0" + long_id = "call_" + "a" * 60 + for bad in (minted, long_id): + safe = wire_safe_tool_call_id(bad) + assert safe != bad + assert re.fullmatch(r"[a-zA-Z0-9_-]{1,40}", safe) + # Deterministic (pairing across messages and across requests), distinct inputs distinct. + assert wire_safe_tool_call_id(minted) == wire_safe_tool_call_id(minted) + assert wire_safe_tool_call_id(minted) != wire_safe_tool_call_id(long_id) + + +def test_legalize_ids_identity_when_all_legal() -> None: + msgs = [_assistant_calls(_call("call_1", "{}")), _tool("call_1")] + assert legalize_tool_call_ids(msgs) is msgs + + +def test_legalize_ids_rewrites_call_and_result_consistently() -> None: + minted = "task-1::r1s1::call_0" + msgs = [_assistant_calls(_call(minted, "{}")), _tool(minted)] + out = legalize_tool_call_ids(msgs) + wire_id = out[0]["tool_calls"][0]["id"] + assert re.fullmatch(r"[a-zA-Z0-9_-]{1,40}", wire_id) + assert out[1]["tool_call_id"] == wire_id # pairing survives the projection + # Copy-on-write: the input messages (the canonical-adjacent dicts) are unmutated. + assert msgs[0]["tool_calls"][0]["id"] == minted + assert msgs[1]["tool_call_id"] == minted + + +def test_legalize_ids_leaves_legal_siblings_untouched() -> None: + minted = "task-1::r1s2::call_1" + msgs = [ + _assistant_calls(_call("call_ok", "{}"), _call(minted, "{}")), + _tool("call_ok"), + _tool(minted), + ] + out = legalize_tool_call_ids(msgs) + assert out[0]["tool_calls"][0]["id"] == "call_ok" + assert out[1]["tool_call_id"] == "call_ok" + assert out[0]["tool_calls"][1]["id"] == out[2]["tool_call_id"] + assert "::" not in out[0]["tool_calls"][1]["id"] + + +def test_legalize_ids_skips_empty_and_non_string() -> None: + # Empty ids belong to repair_wire_messages' back-fill; non-strings are + # someone else's malformation — neither is this pass's to invent. + msgs = [ + _assistant_calls( + {"id": "", "type": "function", "function": {"name": "b", "arguments": "{}"}} + ), + {"role": "tool", "tool_call_id": None, "content": "x"}, + ] + out = legalize_tool_call_ids(msgs) + assert out[0]["tool_calls"][0]["id"] == "" + assert out[1]["tool_call_id"] is None diff --git a/tests/test_session.py b/tests/test_session.py index 86430dc3..889e2641 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -2450,9 +2450,230 @@ class TestAgentChildRegistration: parent_call_id="task-1", ) - # Sub-agent tool ids are namespaced by the parent so the UI registry - # can't collide across concurrent task agents (local sequential ids). - session.ui.note_agent_child.assert_called_once_with("task-1::call_1", "task-1") + # Sub-agent tool ids are minted ``{parent}::r{run}s{step}::{provider_id}`` + # so the UI registry can't collide across concurrent task agents, across + # turns within one agent (local sequential ids like "call_0"), or across + # runs whose PARENT id was itself reused. + session.ui.note_agent_child.assert_called_once_with("task-1::r1s1::call_1", "task-1") + + def test_cross_turn_reused_provider_ids_stay_distinct(self): + # A local provider reuses "call_0" verbatim every response. The minted + # id carries a per-agent step sequence, so the registry, the wire, the + # recall projection, and the cancel ledger all see two DISTINCT calls. + # Pre-mint both mapped to "task-1::call_0": the live card collapsed the + # rows (bug-3) while FIFO recall kept them apart — the two disagreed on + # identical input. + from turnstone.core.providers._openai_chat import OpenAIChatCompletionsProvider + + session = _make_session() + session._provider = OpenAIChatCompletionsProvider() + session.ui.note_agent_child = MagicMock() + + call_count = [0] + + def fake_create(**_kwargs): + call_count[0] += 1 + resp = MagicMock() + choice = MagicMock() + if call_count[0] <= 2: + choice.finish_reason = "tool_calls" + tc = MagicMock() + tc.id = "call_0" # reused verbatim across turns + tc.function.name = "read_file" + tc.function.arguments = f'{{"path": "/tmp/f{call_count[0]}"}}' + choice.message.tool_calls = [tc] + choice.message.content = None + else: + choice.finish_reason = "stop" + choice.message.tool_calls = None + choice.message.content = "done" + resp.choices = [choice] + resp.usage = MagicMock(prompt_tokens=10, completion_tokens=5) + return resp + + session.client.chat.completions.create = fake_create + + def fake_prepare(tc_dict, **_kwargs): + n = call_count[0] + return { + "call_id": tc_dict["id"], + "func_name": "read_file", + "needs_approval": False, + "execute": lambda p, n=n: (p["call_id"], f"contents-{n}"), + } + + agent_turns = [Turn.user("x")] + with patch.object(session, "_prepare_tool", side_effect=fake_prepare): + session._run_agent( + agent_turns, + tools=[{"type": "function", "function": {"name": "read_file"}}], + label="task", + parent_call_id="task-1", + ) + + # Registry: two registrations, distinct minted ids, same parent. + assert [c.args for c in session.ui.note_agent_child.call_args_list] == [ + ("task-1::r1s1::call_0", "task-1"), + ("task-1::r1s2::call_0", "task-1"), + ] + # Recall projection: two steps, each paired to its OWN result. + steps = ChatSession._project_agent_steps(agent_turns) + assert [s["id"] for s in steps] == ["task-1::r1s1::call_0", "task-1::r1s2::call_0"] + assert [s["output"] for s in steps] == ["contents-1", "contents-2"] + # Cancel ledger agrees: both calls answered, no in-flight gap. + issued, first_gap = ChatSession._cancel_ledger(agent_turns) + assert issued == [("read_file", True), ("read_file", True)] + assert first_gap is None + + @staticmethod + def _reusing_provider(session, tool_turns: int = 1): + """Fake create() reissuing id "call_0" for ``tool_turns`` turns, then + stopping — the local-server id-reuse shape. Returns the counter.""" + call_count = [0] + + def fake_create(**_kwargs): + call_count[0] += 1 + resp = MagicMock() + choice = MagicMock() + if call_count[0] <= tool_turns: + choice.finish_reason = "tool_calls" + tc = MagicMock() + tc.id = "call_0" + tc.function.name = "read_file" + tc.function.arguments = '{"path": "/tmp/x"}' + choice.message.tool_calls = [tc] + choice.message.content = None + else: + choice.finish_reason = "stop" + choice.message.tool_calls = None + choice.message.content = "done" + resp.choices = [choice] + resp.usage = MagicMock(prompt_tokens=10, completion_tokens=5) + return resp + + session.client.chat.completions.create = fake_create + return call_count + + def test_parent_id_reuse_across_runs_mints_distinct_child_ids(self): + # A local provider reuses "call_0" for the PARENT task_agent call too: + # two sequential runs share parent_call_id "call_0". The session-level + # run counter keeps their minted CHILD ids distinct — with only the + # per-run step seq (the intermediate fix, before the run counter) both + # runs minted "call_0::s1::call_0" and the second agent's sub-tool + # steps grafted onto the first agent's DOM rows. + # + # SCOPE: this fixes child (sub-tool) ids only. The parent CARD still + # keys on the raw reused parent id ("call_0") — stash_agent_trajectory, + # _tool_status, the card's own data-call-id row — so two runs with the + # same parent id still alias at the card level. Parent ids are + # main-loop ids; de-colliding them is the main-loop id-hygiene + # follow-up (see legalize_tool_call_ids' docstring), not this change. + from turnstone.core.providers._openai_chat import OpenAIChatCompletionsProvider + + session = _make_session() + session._provider = OpenAIChatCompletionsProvider() + session.ui.note_agent_child = MagicMock() + + def fake_prepare(tc_dict, **_kwargs): + return { + "call_id": tc_dict["id"], + "func_name": "read_file", + "needs_approval": False, + "execute": lambda p: (p["call_id"], "contents"), + } + + minted: list[str] = [] + for _run in range(2): + self._reusing_provider(session) + with patch.object(session, "_prepare_tool", side_effect=fake_prepare): + session._run_agent( + [Turn.user("x")], + tools=[{"type": "function", "function": {"name": "read_file"}}], + label="task", + parent_call_id="call_0", + ) + minted.append(session.ui.note_agent_child.call_args.args[0]) + + assert minted == ["call_0::r1s1::call_0", "call_0::r2s1::call_0"] + assert len(set(minted)) == 2 + + def test_agent_wire_is_validity_legalized(self): + # The agent seam bypasses the main-loop wire prep and builds its own + # history, so it runs the same two validity passes itself. Drive one + # tool turn whose call carries both a minted "::" id (projected as + # defensive hardening) and malformed non-object arguments (a strict + # renderer json.loads and 400s them), then assert the REPLAY request + # the second _api_call sends carries the projected id (call/result + # pairing intact) and object-shaped arguments. The internal id keeps + # the "::" form. + import json as _json + import re as _re + + from turnstone.core.providers._openai_chat import OpenAIChatCompletionsProvider + + session = _make_session() + session._provider = OpenAIChatCompletionsProvider() + session.ui.note_agent_child = MagicMock() + + seen_messages: list[list[dict]] = [] + call_count = [0] + + def fake_create(**kwargs): + seen_messages.append(kwargs.get("messages") or []) + call_count[0] += 1 + resp = MagicMock() + choice = MagicMock() + if call_count[0] == 1: + choice.finish_reason = "tool_calls" + tc = MagicMock() + tc.id = "call_0" + tc.function.name = "read_file" + # Malformed: unterminated JSON with a non-"length" finish + # reason — the sanitize pass's reason to exist. + tc.function.arguments = '{"path": "/tmp/x"' + choice.message.tool_calls = [tc] + choice.message.content = None + else: + choice.finish_reason = "stop" + choice.message.tool_calls = None + choice.message.content = "done" + resp.choices = [choice] + resp.usage = MagicMock(prompt_tokens=10, completion_tokens=5) + return resp + + session.client.chat.completions.create = fake_create + + def fake_prepare(tc_dict, **_kwargs): + return { + "call_id": tc_dict["id"], + "func_name": "read_file", + "needs_approval": False, + "execute": lambda p: (p["call_id"], "contents"), + } + + with patch.object(session, "_prepare_tool", side_effect=fake_prepare): + session._run_agent( + [Turn.user("x")], + tools=[{"type": "function", "function": {"name": "read_file"}}], + label="task", + parent_call_id="task-1", + ) + + # Internal id (registry) keeps the minted "::" form. + internal = session.ui.note_agent_child.call_args.args[0] + assert internal == "task-1::r1s1::call_0" + # The SECOND request replays the tool turn: wire id is the legal + # projection, consistent between the call and its result; arguments + # are legalized to a JSON object. + replay = seen_messages[1] + wire_calls = [tc for m in replay if m.get("tool_calls") for tc in m["tool_calls"]] + wire_results = [m for m in replay if m.get("role") == "tool"] + assert wire_calls and wire_results + wire_id = wire_calls[0]["id"] + assert _re.fullmatch(r"[a-zA-Z0-9_-]{1,40}", wire_id), wire_id + assert wire_results[0]["tool_call_id"] == wire_id + assert wire_id != internal + assert isinstance(_json.loads(wire_calls[0]["function"]["arguments"]), dict) class TestRunAgentDenialMessage: @@ -2633,6 +2854,9 @@ class TestProjectAgentSteps: def test_colliding_ids_paired_fifo_not_last_wins(self): # A local provider reuses id "call_0" across turns; FIFO pairing gives # each call its OWN result, not last-wins (which would show out-B twice). + # Parented runs can no longer produce this input (_run_agent mints + # unique ids), but the FIFO stays as honest pairing for input a mint + # never touched — an unparented run, or turns constructed directly. from turnstone.core.trajectory import ToolCall, Turn turns = [ diff --git a/turnstone/core/lowering.py b/turnstone/core/lowering.py index c17e2e24..c40c548e 100644 --- a/turnstone/core/lowering.py +++ b/turnstone/core/lowering.py @@ -18,7 +18,15 @@ This module owns the three provider-neutral lowering passes: ``deepseek_v4``, which ``json.loads`` the arguments at request-render time) can't reject the whole request. Mutates the transient wire copy only — the canonical trajectory keeps the raw output. See - :func:`sanitize_tool_call_arguments`. + :func:`sanitize_tool_call_arguments`. The id sibling, + :func:`legalize_tool_call_ids`, projects session-minted sub-agent tool ids + (``{parent}::r{run}s{step}::{provider_id}`` — long and ``::``-containing) to + deterministic safe tokens as DEFENSIVE hardening (no backend turnstone + targets is known to reject them; the prior ``::`` id format ran fine); it + runs at the AGENT wire seam (``ChatSession._run_agent``'s ``_api_call``) + only — main-loop ids are provider-issued or uuid-filled, and main-loop + assistant turns + carry a provider-native lane whose block ids must stay untouched. * **repair** (validity) — synthesizing cancellation results for orphaned client tool calls. See :func:`repair_wire_messages`. @@ -52,6 +60,7 @@ their own. from __future__ import annotations +import hashlib import json import re from typing import Any @@ -287,6 +296,94 @@ def sanitize_tool_call_arguments(messages: list[dict[str, Any]]) -> list[dict[st return messages if out is None else out +# A conservative wire-legal shape for tool-call ids: alphanumerics, ``_`` and +# ``-``, up to 40 chars — the tightest charset/length a provider wire is +# plausibly strict about. DEFENSIVE only: no backend turnstone targets is +# known to require it — the deployment is lenient anthropic-compatible vLLM, +# and the prior ``::``-containing id format replayed fine — so an id already +# matching this passes through untouched and the projection is belt-and-braces. +_WIRE_TOOL_ID_RE = re.compile(r"[a-zA-Z0-9_-]{1,40}") + + +def wire_safe_tool_call_id(tc_id: str) -> str: + """Return *tc_id* unchanged if it already matches the conservative + wire-legal shape (:data:`_WIRE_TOOL_ID_RE`), else a deterministic safe + token (``tid_`` + 32 hex chars of its SHA-1, 36 chars total). + + Determinism is the contract: the same original id maps to the same token + in the assistant ``tool_use`` and its ``tool_result`` (intra-request + pairing) and across successive requests that replay the same turns. + Provider-issued ids (their own echo) and ``_ensure_tool_call_ids``'s + uuid fills already match, so they pass through unchanged; only + session-minted composite ids (``{parent}::r{run}s{step}::{provider_id}``) + are projected. + """ + if _WIRE_TOOL_ID_RE.fullmatch(tc_id): + return tc_id + return "tid_" + hashlib.sha1(tc_id.encode("utf-8", "surrogatepass")).hexdigest()[:32] + + +def legalize_tool_call_ids(messages: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Return *messages* with every tool-call id made wire-legal — the id + half of the legalize pass, applied at the AGENT wire seam. + + Rewrites assistant ``tool_calls[*].id`` and tool ``tool_call_id`` through + :func:`wire_safe_tool_call_id`, so a minted ``::`` id is projected to a + plain token before the wire (call/result pairing survives via + determinism). This is DEFENSIVE hardening — the ids replay fine on the + lenient anthropic-compatible deployment today; the projection just keeps + an agent's self-built history valid on a hypothetically stricter backend. + Empty and non-string ids are left alone — the empty back-fill belongs to + :func:`repair_wire_messages`'s domain, and a non-string is someone else's + malformation to surface, not silently rename. + + Copy-on-write + identity-preserving, exactly like + :func:`sanitize_tool_call_arguments`: a conversation whose ids are all + legal returns the same object. Applied at the AGENT seam only, where the + ``::`` mint is the sole illegal-id source; NOT wired into the main-loop + wire prep, because assistant turns there can carry a provider-native block + lane whose ids must stay byte-identical to the mirrored ``tool_calls``, + and projecting them would desync the two. If the main loop ever needs the + same hygiene (a mid-session ``/model`` switch replays the prior backend's + ids — a vLLM ``chatcmpl-tool-`` + 32-hex id is 46 chars, long enough that a + backend with a short ``tool_call_id`` cap could reject it), the fix is + de-colliding + legalizing at ``_ensure_tool_call_ids`` in a way that also + rewrites the native lane — the broader main-loop id-hygiene follow-up, out + of scope here. + """ + out: list[dict[str, Any]] | None = None # copy-on-write: None until first fix + for idx, msg in enumerate(messages): + role = msg.get("role") + if role == "assistant" and msg.get("tool_calls"): + repaired: list[dict[str, Any]] | None = None + for ci, tc in enumerate(msg["tool_calls"]): + tc_id = tc.get("id") + if not isinstance(tc_id, str) or not tc_id: + continue + safe = wire_safe_tool_call_id(tc_id) + if safe is tc_id: + continue + if repaired is None: + repaired = list(msg["tool_calls"]) + log.debug("wire.tool_id_legalized", call_id=tc_id, wire_id=safe) + repaired[ci] = {**tc, "id": safe} + if repaired is not None: + if out is None: + out = list(messages) + out[idx] = {**msg, "tool_calls": repaired} + elif role == "tool": + tc_id = msg.get("tool_call_id") + if not isinstance(tc_id, str) or not tc_id: + continue + safe = wire_safe_tool_call_id(tc_id) + if safe is tc_id: + continue + if out is None: + out = list(messages) + out[idx] = {**msg, "tool_call_id": safe} + return messages if out is None else out + + # --------------------------------------------------------------------------- # # Fold — operator-context representation (A); runs BEFORE repair on the wire. # --------------------------------------------------------------------------- # diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 7df217d9..dfea000e 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -69,6 +69,7 @@ from turnstone.core.lowering import ( UNOBSERVED_OUTCOME_CLAUSE, drop_empty_user_turns, fold_system_turns, + legalize_tool_call_ids, repair_wire_messages, sanitize_tool_call_arguments, tool_args_preview, @@ -1531,6 +1532,13 @@ class ChatSession: self._apply_persona_snapshot(persona_snapshot) self._title_generated = False self._read_files: set[str] = set() + # Session-monotonic run counter for sub-agent id minting (see + # ``_run_agent``): the parent call id alone can repeat across runs (a + # local provider reuses per-response ids for the PARENT task_agent + # call too), so each run's minted child ids carry this tag. Lock, not + # bare increment: runs start on the 4-wide task pool concurrently. + self._agent_run_seq = 0 + self._agent_run_seq_lock = threading.Lock() # The canonical in-memory trajectory. Wire prep (fold/repair) + the # provider translators still consume dicts, so ``_full_messages`` lowers # Turns→dicts at that boundary until those layers migrate. @@ -15086,10 +15094,14 @@ class ChatSession: ) -> Iterator[tuple[ToolCall, Turn | None]]: """Yield ``(tool_call, result_turn_or_None)`` for every sub-tool the sub-agent issued, in order, pairing each call to its result FIFO per - call_id — a queue per id consumed once, NOT a last-wins dict, so a local - provider that reuses ids across turns (``call_0`` …) can't collapse - distinct calls onto one result. Shared by :meth:`_project_agent_steps` - (recall) and :meth:`_cancel_ledger` (cancel disposition).""" + call_id. Parented runs mint session-unique ids + (``{parent}::r{run}s{step}::{provider_id}``, see :meth:`_run_agent`), + so for them this is a plain unique-key pairing; the FIFO queue stays as + honest pairing for id-colliding input a mint never touched (an + unparented run, or turns constructed directly), where last-wins would + collapse distinct calls onto one result. Shared by + :meth:`_project_agent_steps` (recall) and :meth:`_cancel_ledger` + (cancel disposition).""" pending: dict[str, collections.deque[Turn]] = {} for t in agent_turns: if t.role is Role.TOOL and t.tool_call_id: @@ -15261,8 +15273,23 @@ class ChatSession: # is in-memory and rebuilt per ``_run_agent`` invocation. # Lower the trajectory once, not once per retry attempt — ``turns`` # is invariant across attempts (the retry path only sleeps and - # re-sends the same messages). - wire = dicts_from_turns(turns) + # re-sends the same messages). Two validity passes, the same two + # the main-loop wire prep runs, both applied here because agent + # calls bypass that prep and build their own history: + # * ``sanitize_tool_call_arguments`` — a local model can emit an + # unterminated / non-object ``arguments`` with a non-``length`` + # finish reason; a strict renderer (vLLM ``deepseek_v4``) then + # ``json.loads`` it and 400s every request that replays it. + # (Documented in-tree for the main loop; agents hit the same + # backends, so the same guard applies.) + # * ``legalize_tool_call_ids`` — project the session-minted + # ``::`` sub-tool ids to plain tokens, DEFENSIVE only: they + # replay fine on the lenient anthropic-compatible deployment, + # this just keeps the history valid on a hypothetically + # stricter backend. Deterministic, so call/result pairing + # holds within and across requests; the minted id stays the + # internal key (registry / DOM / recall) untouched. + wire = legalize_tool_call_ids(sanitize_tool_call_arguments(dicts_from_turns(turns))) last_err: Exception | None = None for attempt in range(self._MAX_RETRIES + 1): try: @@ -15299,6 +15326,16 @@ class ChatSession: raise last_err turn = 0 + # Mint tags for sub-tool ids (see the rewrite below). ``run_seq`` is + # session-unique per _run_agent invocation — the parent call id alone + # can repeat across runs when a local provider reuses per-response ids + # for the PARENT task_agent call too. ``sub_step_seq`` is monotonic + # across the WHOLE run, so ids stay distinct across turns even when + # the provider reuses per-response ids ("call_0") for sub-tools. + with self._agent_run_seq_lock: + self._agent_run_seq += 1 + run_seq = self._agent_run_seq + sub_step_seq = 0 while max_tool_turns < 0 or turn < max_tool_turns: self._check_cancelled() try: @@ -15339,17 +15376,26 @@ class ChatSession: agent_tool_calls: tuple[ToolCall, ...] = () if result.tool_calls: self._ensure_tool_call_ids(result.tool_calls) - # Namespace sub-agent tool ids by the parent task_agent so the - # UI nesting registry can't collide across concurrent task - # agents whose (local) provider reuses sequential ids ("call_0"). - # Tool-call ids are opaque correlation tokens — a provider - # validates only intra-request assistant/tool consistency on - # replay, never against its own prior generation — so rewriting - # them in this ephemeral sub-conversation is wire-safe. Skipped - # for a top-level run (no parent → no nesting). + # Mint each sub-agent tool id session-unique: + # ``{parent}::r{run}s{step}::{provider_id}``. The run tag + # de-collides RUNS (a reused parent id can't alias two agents' + # children); the step tag de-collides turns WITHIN one agent + # whose (local) provider reuses per-response sequential ids + # ("call_0") — pre-mint, that reuse collapsed the live card's + # DOM rows while FIFO recall kept them apart, so the two + # disagreed on identical input. The parent segment keeps the + # id traceable and is what the frontend's "::" child checks + # key off. Every downstream consumer (nesting registry, + # error-flags, DOM data-call-id, recall, cancel ledger) keys + # on this ONE id; the wire alone sees a deterministic legal + # projection instead (``legalize_tool_call_ids`` in + # ``_api_call`` — strict providers reject ``::`` ids), which + # preserves intra-request call/result pairing. Skipped for a + # top-level run (no parent → no nesting). if parent_call_id: for tc in result.tool_calls: - tc["id"] = f"{parent_call_id}::{tc['id']}" + sub_step_seq += 1 + tc["id"] = f"{parent_call_id}::r{run_seq}s{sub_step_seq}::{tc['id']}" agent_tool_calls = tuple( ToolCall( id=tc["id"], @@ -15718,9 +15764,10 @@ class ChatSession: unknown/none on a multi-call turn.) Shared by the disposition string and its typed status so the two can't disagree. - Pairs via :meth:`_iter_agent_tool_results` (FIFO per call_id), so on a - provider that reuses ids a half-answered colliding pair is correctly read - as one answered + one in-flight gap, not (set-membership) both answered. + Pairs via :meth:`_iter_agent_tool_results`: parented runs carry minted + unique ids, and on un-minted id-colliding input (unparented / direct + construction) the FIFO still reads a half-answered colliding pair as + one answered + one in-flight gap, not (set-membership) both answered. """ issued = [ ((tc.name or "tool").strip(), res is not None) diff --git a/turnstone/core/session_ui_base.py b/turnstone/core/session_ui_base.py index 7128618d..d8d0574b 100644 --- a/turnstone/core/session_ui_base.py +++ b/turnstone/core/session_ui_base.py @@ -926,11 +926,14 @@ class SessionUIBase: with ``parent_call_id``. Called by the session for each sub-tool a ``_run_agent`` issues, before the tool emits anything. - The session namespaces each sub-agent's child ids by parent - (``f"{parent_call_id}::{tc_id}"``) before registering them here, so the - key is unique even for local servers that assign per-response sequential - ids (``call_0``) — two task agents in the parent's 4-wide pool can't - collide and mis-nest steps.""" + The session mints each sub-agent child id session-unique + (``f"{parent_call_id}::r{run}s{step}::{tc_id}"``) before registering + it here: the run tag de-collides agent runs (concurrent in the + parent's 4-wide pool, or sequential runs whose PARENT id a local + server reused), and the step tag de-collides that server's + per-response sequential sub-tool ids (``call_0``) across the SAME + agent's turns — one key names one call, so steps can't mis-nest or + collapse.""" if not child_call_id or not parent_call_id: return with self._agent_children_lock: diff --git a/turnstone/shared_static/interactive.js b/turnstone/shared_static/interactive.js index 02eb4c81..956e8eff 100644 --- a/turnstone/shared_static/interactive.js +++ b/turnstone/shared_static/interactive.js @@ -653,7 +653,7 @@ class Pane { if (!el) { let target = this._toolRow(callId); if (!target) { - // A namespaced sub-agent child id ("::") whose row hasn't + // A minted sub-agent child id ("::::") whose row hasn't // nested yet must NOT graft its stream onto the last top-level batch — // that mislabels a sub-tool's output as a main-harness tool's. Its row // arrives via the orphan flush; skip the chunk until then. @@ -3491,7 +3491,7 @@ class Pane { } let target = this._toolRow(callId); if (!target) { - // A namespaced sub-agent child id ("::") whose row hasn't + // A minted sub-agent child id ("::::") whose row hasn't // nested yet must NOT graft its output onto the last top-level batch row // — that mislabels a sub-tool's result as a main-harness tool's. Its row // arrives via the orphan flush; skip until then.