diff --git a/CHANGELOG.md b/CHANGELOG.md index 36dcb5b0..78e7083c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,6 +217,16 @@ frozen. replay as before. A workstream that searched misses the prompt cache once, on its first request after the update. One that searched again for tools it had already found resends each of those searches, with the definitions it loaded, until it compacts. +- **A refused tool call says the tool is not available, not "Unknown tool" (#1287).** Tools can + leave a session mid-conversation: an MCP server drops one or goes away, or another user sends on + a shared workstream, whose tools follow the sender. When the model called one, it was told + "Unknown tool" and to use a listed name exactly, and the operator saw "Model called unknown + tool", as if the model had made the name up. Both now say the tool is not available now, and the + model is told it may have been removed or be misspelled, or, on a shared workstream, belong to + another user. The tools that reply lists are the ones the request offers, where it used to name + every built-in tool, including coordinator tools in a regular session, tools a persona hides and + tools an operator revoked. With tool search on, it lists the tools offered outright and says more + may be found by searching, and a task agent sees its own tools. - **A storage error while opening a workstream no longer replaces its saved settings.** Building the session saves its defaults when no settings are saved, and a read that failed counted as none saved, so one failed read could overwrite the saved model, sampling, instructions and skill. That diff --git a/tests/test_coordinator_governance.py b/tests/test_coordinator_governance.py index 6d26377a..0c93cc24 100644 --- a/tests/test_coordinator_governance.py +++ b/tests/test_coordinator_governance.py @@ -581,23 +581,27 @@ def test_prepare_tool_blocks_revoked_tool(): def test_prepare_tool_allows_non_revoked_tool(): """The revocation gate must not fire on a tool name that isn't in the revoked set. We pick a name that's also not in the preparers - dict so we can assert the 'unknown tool' result shape without + dict so we can assert the 'not available' result shape without exercising a real preparer.""" from turnstone.core.session import ChatSession session = ChatSession.__new__(ChatSession) session._revoked_tools = frozenset({"spawn_workstream"}) session._mcp_client = None + session._shared_workstream = False session.ui = MagicMock() tc = { "id": "call-2", "function": {"name": "this_tool_is_not_registered", "arguments": "{}"}, } - item = session._prepare_tool(tc) - # Unknown tool path — not the revocation error path. + # A bare session has no model lane to derive its tool offer from, so the + # refusal is handed one. + item = session._prepare_tool_for_principal(tc, "", offered=frozenset()) + # Not-available path — not the revocation error path. err = str(item.get("error") or "") assert "revoked" not in err.lower() + assert "is not available now" in err # --------------------------------------------------------------------------- diff --git a/tests/test_mcp_client.py b/tests/test_mcp_client.py index 089dd7eb..c4e5a381 100644 --- a/tests/test_mcp_client.py +++ b/tests/test_mcp_client.py @@ -723,9 +723,11 @@ def test_unknown_tool_without_mcp(self, tmp_db): } prepared = session._prepare_tool(tc) assert "error" in prepared - assert "Unknown tool" in prepared["error"] - # Error lists available tools so the model can self-correct + assert "'nonexistent' is not available now" in prepared["error"] + # Error lists the offered tools so the model can self-correct... assert "bash" in prepared["error"] + # ...and only those: an interactive session offers no coordinator tools. + assert "spawn_workstream" not in prepared["error"] # Surfaces warning to user session.ui.on_error.assert_called_once() assert "nonexistent" in session.ui.on_error.call_args[0][0] @@ -890,19 +892,18 @@ def test_session_close_removes_listener_with_same_user_id(self, tmp_db): assert registered_cb is removed_cb def test_session_unknown_tool_lists_user_scoped_catalog(self, tmp_db): - """The "Unknown tool" error message lists tools the session can - actually invoke — drawn from the merged user-scoped catalog, - not the manager's private static-only ``_tool_map``.""" + """The refusal of a tool dispatch cannot run lists the MCP tools the + session offers — drawn from the user's merged catalog, static and + pool entries alike.""" mock_mcp = MagicMock() - # Pretend the user's merged view contains a static + pool entry. - mock_mcp.get_tools.return_value = [ + # A static entry everyone sees, plus a pool entry only in user-7's view. + mock_mcp.get_tools.side_effect = lambda user_id=None: [ _fake_openai_tool("mcp__static__list"), - _fake_openai_tool("mcp__pool-srv__do"), + *([_fake_openai_tool("mcp__pool-srv__do")] if user_id == "user-7" else []), ] mock_mcp.is_mcp_tool.return_value = False - session = self._make_session(mcp_client=mock_mcp, user_id="user-7") - # Reset the call counter so we observe only the _prepare_tool call. - mock_mcp.get_tools.reset_mock() + # Tool search off, so the MCP tools are offered outright, not deferred. + session = self._make_session(mcp_client=mock_mcp, user_id="user-7", tool_search="off") tc = { "id": "call_unknown", @@ -910,14 +911,10 @@ def test_session_unknown_tool_lists_user_scoped_catalog(self, tmp_db): } prepared = session._prepare_tool(tc) assert "error" in prepared - # The error mentions both static and pool tools — proves we're - # consulting the merged catalog rather than ``_tool_map``. + # Both entries are listed, and the pool one exists only in user-7's + # view: the list is this user's merged catalog. assert "mcp__static__list" in prepared["error"] assert "mcp__pool-srv__do" in prepared["error"] - # And the catalog request was scoped to this session's user. - assert any( - call.kwargs.get("user_id") == "user-7" for call in mock_mcp.get_tools.call_args_list - ) # --------------------------------------------------------------------------- diff --git a/tests/test_task_agent_compaction.py b/tests/test_task_agent_compaction.py index cd929295..bdfe9df1 100644 --- a/tests/test_task_agent_compaction.py +++ b/tests/test_task_agent_compaction.py @@ -337,7 +337,7 @@ def test_estimator_observe_without_served_figure_keeps_ratio_and_own_estimate() assert estimator.estimate(messages) == own_estimate -def _prepared_tool(tool_call: dict[str, Any], _principal: str): +def _prepared_tool(tool_call: dict[str, Any], _principal: str, **_kwargs: Any): call_id = tool_call["id"] return { "call_id": call_id, diff --git a/tests/test_unavailable_tool_refusal.py b/tests/test_unavailable_tool_refusal.py new file mode 100644 index 00000000..a7db160d --- /dev/null +++ b/tests/test_unavailable_tool_refusal.py @@ -0,0 +1,209 @@ +"""The refusal of a tool call that dispatch cannot run (#1287). + +A session's catalog can change mid-conversation: an MCP server drops a tool or goes away, or +another user sends on a shared workstream and the catalog follows the sender. The name the model +calls may be one it saw earlier, so the refusal says the tool is not available now, and lists only +the tools the request offers, leaving deferred tools to tool search. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest + +from tests._session_helpers import make_result, make_session +from turnstone.core.personas import PersonaSnapshot +from turnstone.core.providers._protocol import ModelCapabilities +from turnstone.core.trajectory import Role, Turn + +GONE = "mcp__gone__lookup" + + +def _tool(name: str) -> dict[str, Any]: + return { + "type": "function", + "function": { + "name": name, + "description": f"The {name} tool.", + "parameters": {"type": "object", "properties": {}}, + }, + } + + +def _call(name: str = GONE) -> dict[str, Any]: + return {"id": "call-gone", "type": "function", "function": {"name": name, "arguments": "{}"}} + + +def _mcp(*names: str) -> MagicMock: + mcp = MagicMock() + mcp.get_tools.return_value = [_tool(name) for name in names] + # Dispatch finds no MCP tool by the called name: it left the catalog. + mcp.is_mcp_tool.return_value = False + mcp.resource_count_for_user.return_value = 0 + mcp.prompt_count_for_user.return_value = 0 + return mcp + + +def _listed(error: str) -> list[str]: + return error.split("Other tools offered now: ", 1)[1].split(".", 1)[0].split(", ") + + +def test_refusal_says_the_tool_is_gone_and_lists_the_offer(tmp_db) -> None: + ui = MagicMock() + session = make_session(ui=ui, mcp_client=_mcp("mcp__docs__read"), tool_search="off") + + item = session._prepare_tool(_call()) + + error = item["error"] + # A single-user workstream: another user cannot be the cause, so it is not named. + assert error.startswith( + f"Tool '{GONE}' is not available now. It may have been removed since it was offered, " + "or its name may be misspelled." + ) + assert error.endswith("To call one, use its name exactly as listed.") + listed = _listed(error) + assert "bash" in listed + assert "mcp__docs__read" in listed + # A coordinator tool has a preparer, but an interactive session does not offer it. + assert "spawn_workstream" not in listed + assert "tool search" not in error + assert item["header"].endswith(f"{GONE}: not available") + ui.on_error.assert_called_once_with(f"Model called a tool that is not available now: '{GONE}'") + + +def test_a_shared_workstream_names_another_user_as_a_cause(tmp_db) -> None: + session = make_session(mcp_client=_mcp("mcp__docs__read"), tool_search="off") + # Latched once a non-owner speaks; the catalog then follows the sender. + session._shared_workstream = True + + error = session._prepare_tool(_call())["error"] + + assert error.startswith( + f"Tool '{GONE}' is not available now. It may have been removed since it was offered, " + "or be available only to another user of this workstream, or its name may be misspelled." + ) + + +def test_revoked_tools_are_not_offered_as_alternatives(tmp_db) -> None: + session = make_session(mcp_client=_mcp("mcp__docs__read"), tool_search="off") + # Revocation leaves the tool on the wire; dispatch refuses it. + session.revoke_tools({"bash"}) + + listed = _listed(session._prepare_tool(_call())["error"]) + + assert "bash" not in listed + assert "read_file" in listed + + +def test_an_offer_of_nothing_else_says_so(tmp_db) -> None: + session = make_session() + + error = session._prepare_tool_for_principal(_call(), "user-a", offered=frozenset({GONE}))[ + "error" + ] + + # An offered name is no misspelling, and with nothing else listed there is nothing to call. + assert error == ( + f"Tool '{GONE}' is not available now. It may have been removed since it was offered. " + "No other tools are offered now." + ) + + +def test_a_persona_refusal_lists_only_the_tools_its_set_shows(tmp_db) -> None: + persona = PersonaSnapshot( + name="guard", prompt="", tools=frozenset({"bash", "read_file"}), mcp=True, memory=True + ) + session = make_session(mcp_client=_mcp("mcp__docs__read"), persona_snapshot=persona) + + error = session._prepare_tool(_call())["error"] + + assert "Other tools offered now: bash, read_file." in error + assert "tool search" not in error + + +@pytest.mark.parametrize("native", [True, False], ids=["native", "client-side"]) +def test_deferred_tools_are_left_to_tool_search(tmp_db, native: bool) -> None: + session = make_session(mcp_client=_mcp("mcp__docs__read", "mcp__docs__write"), tool_search="on") + assert session._tool_search is not None + session._tool_search.expand_visible(["mcp__docs__read"]) + caps = ModelCapabilities(supports_tool_search=native) + + with patch.object(session, "_get_capabilities", return_value=caps): + error = session._prepare_tool(_call())["error"] + + listed = _listed(error) + # A discovered tool is offered outright; a deferred one is not listed. + assert "mcp__docs__read" in listed + assert "mcp__docs__write" not in listed + # Client-side search is itself an offered tool; native search is the provider's. + assert ("tool_search" in listed) is not native + assert "More tools may be found with tool search." in error + + +def test_a_task_agent_is_shown_its_own_tools(tmp_db) -> None: + session = make_session() + contexts: list[list[Turn]] = [] + responses = [ + make_result("", tool_calls=[_call()], finish_reason="tool_calls"), + make_result("done"), + ] + + def plant(_lane: Any, turns: list[Turn], **_kwargs: Any) -> Any: + contexts.append(list(turns)) + return responses.pop(0) + + with ( + patch.object(session, "_context_window_for_lane", return_value=1_000_000), + patch("turnstone.core.session.model_turn", side_effect=plant), + ): + session._run_agent( + [Turn.system("task identity"), Turn.user("look it up")], + label="task", + tools=[_tool("read_file"), _tool(GONE)], + auto_tools={"read_file"}, + parent_call_id="task-parent", + principal_id="user-a", + ) + + assert responses == [] + [result] = [turn for turn in contexts[1] if turn.role == Role.TOOL] + assert f"Tool '{GONE}' is not available now." in result.text + # The agent was offered that exact name, so it is no misspelling. + assert "misspelled" not in result.text + # The agent's list, not the session's: the session offers bash, the agent does not. + assert "Other tools offered now: read_file." in result.text + + +def test_an_agents_offer_does_not_outlive_its_call(tmp_db) -> None: + session = make_session() + + agent_item = session._prepare_tool_for_principal( + _call(), "user-a", offered=frozenset({"read_file"}) + ) + session_item = session._prepare_tool(_call()) + + assert "Other tools offered now: read_file." in agent_item["error"] + assert "bash" in session_item["error"] + + +def test_deferred_names_survive_the_catalog_dropping_tool_search_mid_read(tmp_db) -> None: + # The refusal and every request read the deferred names. An MCP catalog refresh on another + # thread can drop the tool search manager at any point; here it lands right after the + # manager's truth test, the window a free-threaded build (or a caller passing no caps) opens. + session = make_session() + + class DroppedMidRead: + def __bool__(self) -> bool: + session._tool_search = None + return True + + def get_deferred_tools(self) -> list[dict[str, Any]]: + return [_tool("mcp__docs__write")] + + session._tool_search = DroppedMidRead() # type: ignore[assignment] + + names = session._get_deferred_names(ModelCapabilities(supports_tool_search=True)) + + assert names == frozenset({"mcp__docs__write"}) diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 70bc8b17..1ecc651c 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -1671,6 +1671,12 @@ def cancelled_disposition(self, label: str) -> str: _active_tool_prepare_principal: contextvars.ContextVar[str | None] = contextvars.ContextVar( "turnstone_active_tool_prepare_principal", default=None ) +# Tools the request being dispatched offered, when they are not the session's own (a task +# agent's list). None means the session's offer, which the refusal of a tool dispatch cannot +# run derives itself. +_active_tool_prepare_offer: contextvars.ContextVar[frozenset[str] | None] = contextvars.ContextVar( + "turnstone_active_tool_prepare_offer", default=None +) # Generation whose bounded live commit is currently staging durable closures # on this thread. State UIs use the captured value to refuse a delayed tail @@ -9565,7 +9571,9 @@ def _natively_loaded_tool_names(self) -> frozenset[str]: def _get_deferred_names(self, caps: ModelCapabilities | None = None) -> frozenset[str] | None: """Return names of deferred tools for native provider search, or None.""" - if not self._tool_search: + # Read once: an MCP catalog refresh on another thread can replace or drop the manager. + tool_search = self._tool_search + if not tool_search: return None if self._persona_tools is not None: # Persona visibility sets force client-side tool search — see @@ -9574,7 +9582,7 @@ def _get_deferred_names(self, caps: ModelCapabilities | None = None) -> frozense caps = caps if caps is not None else self._get_capabilities() if not caps.supports_tool_search: return None # Client-side mode — no deferred names for provider - deferred = self._tool_search.get_deferred_tools() + deferred = tool_search.get_deferred_tools() return frozenset(name for t in deferred if (name := t.get("function", {}).get("name", ""))) # Retryability is lane-owned and projected through ``lane_error_is_retryable``. @@ -12235,13 +12243,20 @@ def _prepare_tool_for_principal( principal_id: str, *, safe: bool = False, + offered: frozenset[str] | None = None, ) -> dict[str, Any]: - """Prepare any tool under one immutable turn principal.""" + """Prepare any tool under one immutable turn principal. + + *offered* names the tools the calling request offered when they are not the session's + own, as for a task agent, so that a refusal lists the caller's tools. + """ principal = principal_id.strip() token = _active_tool_prepare_principal.set(principal) + offer_token = _active_tool_prepare_offer.set(offered) try: item = self._safe_prepare_tool(tc) if safe else self._prepare_tool(tc) finally: + _active_tool_prepare_offer.reset(offer_token) _active_tool_prepare_principal.reset(token) # The safe path can synthesize an error item after a preparer raises; # every issued tool still carries the same execution authority field. @@ -19056,6 +19071,47 @@ def _prepare_tool(self, tc: dict[str, Any]) -> dict[str, Any]: item["_principal_id"] = self._tool_prepare_principal_id() return item + def _unavailable_tool_error(self, func_name: str) -> str: + """What the model is told when it calls a tool that dispatch cannot run. + + The name may be one the model saw earlier in the session: the catalog changes between + requests (an MCP server drops a tool or goes away, or another user sends on a shared + workstream, and the catalog follows the sender), and a replayed tool search can still + show a tool the request no longer offers. Another user is named as a cause only once the + workstream is shared, since the model passes the causes on. The other tools listed are a + task agent's own, or the ones the session's next request offers outright; deferred tools + are left to tool search. + """ + offered = _active_tool_prepare_offer.get() + deferred: frozenset[str] = frozenset() + if offered is None: + caps = self._get_capabilities() + deferred = self._get_deferred_names(caps) or frozenset() + offered = frozenset( + name + for tool in self._get_active_tools(caps) or [] + if (name := tool.get("function", {}).get("name")) and name not in deferred + ) + # A revoked tool stays on the wire but dispatch refuses it, so it is no alternative. + offered = offered - self._revoked_tools + others = sorted(offered - {func_name}) + causes = ["It may have been removed since it was offered"] + if self._shared_workstream: + causes.append("be available only to another user of this workstream") + # An exact match for an offered name, as a task agent's always is, is no misspelling. + if func_name not in offered: + causes.append("its name may be misspelled") + parts = [f"Tool {func_name!r} is not available now. {', or '.join(causes)}."] + if others: + parts.append(f"Other tools offered now: {', '.join(others)}.") + else: + parts.append("No other tools are offered now.") + if deferred or "tool_search" in offered: + parts.append("More tools may be found with tool search.") + if others: + parts.append("To call one, use its name exactly as listed.") + return " ".join(parts) + def _prepare_tool_item(self, tc: dict[str, Any]) -> dict[str, Any]: """Parse a tool call and prepare preview info for display.""" call_id = tc["id"] @@ -19220,26 +19276,14 @@ def _prepare_tool_item(self, tc: dict[str, Any]) -> dict[str, Any]: func_name, user_id=prepare_user_id ): return self._prepare_mcp_tool(call_id, func_name, args) - self.ui.on_error(f"Model called unknown tool: {func_name!r}") - available = list(preparers) - if self._mcp_client: - available.extend( - sorted( - t["function"]["name"] - for t in self._mcp_client.get_tools(user_id=prepare_user_id) - ) - ) + self.ui.on_error(f"Model called a tool that is not available now: {func_name!r}") return { "call_id": call_id, "func_name": func_name, - "header": f"\u2717 Unknown tool: {func_name}", + "header": f"\u2717 {func_name}: not available", "preview": "", "needs_approval": False, - "error": ( - f"Unknown tool: {func_name!r}. " - f"Available tools: {', '.join(available)}. " - f"Use one of the listed tool names exactly." - ), + "error": self._unavailable_tool_error(func_name), } assert args is not None # guaranteed by the early return on args is None above return preparer(call_id, args) @@ -26284,7 +26328,7 @@ def _mint_sub_id(original_id: str) -> str: # Execute tools sequentially (not parallel) to avoid # concurrent _read_files mutation from worker threads. - tool_names = {t["function"]["name"] for t in tools} + tool_names = frozenset(t["function"]["name"] for t in tools) for tc_dict in result.tool_calls: cancel_scope.check() tool_name = tc_dict["function"]["name"].strip() @@ -26322,6 +26366,7 @@ def _mint_sub_id(original_id: str) -> str: prepared = self._prepare_tool_for_principal( tc_dict, agent_principal, + offered=tool_names, ) prepared["_approval_cancel_witness"] = _ApprovalCancelWitness( self,