diff --git a/application/agents/tool_executor.py b/application/agents/tool_executor.py index a28d32ba..c86c298b 100644 --- a/application/agents/tool_executor.py +++ b/application/agents/tool_executor.py @@ -24,6 +24,10 @@ from application.storage.db.session import db_readonly, db_session logger = logging.getLogger(__name__) +# Tightest provider limit on function-call names (OpenAI: ^[a-zA-Z0-9_-]{1,64}$). +_MAX_LLM_NAME_LEN = 64 + + def _sanitize_tool_prefix(tool_name: Optional[str]) -> str: """Reduce a tool name to characters allowed in function-call names.""" return re.sub(r"[^a-zA-Z0-9_-]+", "_", str(tool_name or "")).strip("_") @@ -263,6 +267,7 @@ class ToolExecutor: - Duplicate action names are disambiguated with the owning tool's name (e.g. ``brave_search``, ``duckduckgo_search``); a numeric suffix only breaks ties between same-named tools. + - Every name is clamped to the 64-character provider limit. A reverse mapping is stored in ``_name_to_tool`` so that tool calls can be routed back to the correct ``(tool_id, action_name)`` without @@ -303,15 +308,25 @@ class ToolExecutor: result = [] for tool_id, tool_name, action_name, action, is_client in entries: - if name_counts[action_name] == 1: + if ( + name_counts[action_name] == 1 + and len(action_name) <= _MAX_LLM_NAME_LEN + ): llm_name = action_name else: - prefix = _sanitize_tool_prefix(tool_name) + # An over-long unique name skips the prefix — it needs + # truncation, not disambiguation. + prefix = ( + _sanitize_tool_prefix(tool_name) + if name_counts[action_name] > 1 + else "" + ) base = ( f"{prefix}_{action_name}" if prefix and not action_name.startswith(f"{prefix}_") else action_name ) + base = base[:_MAX_LLM_NAME_LEN] # A duplicated bare name stays ambiguous, and a candidate # must not steal a unique action's name or one already taken. candidate = base @@ -321,7 +336,8 @@ class ToolExecutor: or candidate in all_llm_names or name_counts.get(candidate, 0) == 1 ): - candidate = f"{base}_{counter}" + suffix = f"_{counter}" + candidate = base[: _MAX_LLM_NAME_LEN - len(suffix)] + suffix counter += 1 llm_name = candidate diff --git a/application/api/answer/services/stream_processor.py b/application/api/answer/services/stream_processor.py index 9d7fc570..bdd0a176 100644 --- a/application/api/answer/services/stream_processor.py +++ b/application/api/answer/services/stream_processor.py @@ -982,9 +982,10 @@ class StreamProcessor: return self._prompt_content if not isinstance(self.agent_config, dict): return None - prompt_id = self.agent_config.get("prompt_id") - if not prompt_id: - return None + # PG ``agents.prompt_id`` is NULL for agents that never chose a + # prompt — treat missing/empty as the default preset so the + # agentic swap below still applies. + prompt_id = self.agent_config.get("prompt_id") or "default" # Agentic/research agents use the agentic preset variants (search # tool guidance instead of a pre-fetched document block); custom # prompt ids pass through unchanged. @@ -1234,8 +1235,9 @@ class StreamProcessor: agent_type = self.agent_config["agent_type"] # _get_prompt_content handles the agentic preset swap and caching; - # it returns None only for unknown custom ids — re-fetch strictly so - # the underlying error surfaces to the caller. + # it returns None only when the prompt couldn't be fetched (unknown + # or broken custom ids) — re-fetch strictly so the underlying error + # surfaces to the caller. raw_prompt = self._get_prompt_content() if raw_prompt is None: raw_prompt = get_prompt( diff --git a/tests/agents/test_tool_executor.py b/tests/agents/test_tool_executor.py index 1af36025..7c9dc245 100644 --- a/tests/agents/test_tool_executor.py +++ b/tests/agents/test_tool_executor.py @@ -359,6 +359,52 @@ class TestToolExecutorPrepare: assert "get_weather" in names assert "send_email" in names + def test_prepare_tools_prefixed_names_clamped_to_64_chars(self): + """Prefixed names must fit the 64-char provider function-name limit.""" + executor = ToolExecutor() + name_a = "server_" + "x" * 60 + name_b = "server_" + "x" * 60 + "_b" # same first 64 chars as name_a + tools_dict = { + "t1": { + "name": name_a, + "actions": [ + {"name": "search_documents_in_collection", "description": "D", "active": True, "parameters": {"properties": {}}}, + ], + }, + "t2": { + "name": name_b, + "actions": [ + {"name": "search_documents_in_collection", "description": "D", "active": True, "parameters": {"properties": {}}}, + ], + }, + } + result = executor.prepare_tools_for_llm(tools_dict) + names = [r["function"]["name"] for r in result] + assert all(len(n) <= 64 for n in names) + assert len(set(names)) == 2 + # Routing still resolves each clamped name to its original action. + assert {executor._name_to_tool[n] for n in names} == { + ("t1", "search_documents_in_collection"), + ("t2", "search_documents_in_collection"), + } + + def test_prepare_tools_long_unique_name_clamped(self): + """A unique action name over the limit is truncated, not passed through.""" + executor = ToolExecutor() + long_action = "fetch_" + "y" * 70 + tools_dict = { + "t1": { + "name": "tool_a", + "actions": [ + {"name": long_action, "description": "D", "active": True, "parameters": {"properties": {}}}, + ], + }, + } + result = executor.prepare_tools_for_llm(tools_dict) + names = [r["function"]["name"] for r in result] + assert names == [long_action[:64]] + assert executor._name_to_tool[long_action[:64]] == ("t1", long_action) + def test_prepare_tools_prefix_skips_collision_with_unique_name(self): """A prefixed candidate must not steal another action's unique name.""" executor = ToolExecutor() diff --git a/tests/api/answer/services/test_stream_processor_utils.py b/tests/api/answer/services/test_stream_processor_utils.py index 2894037d..d459fed5 100644 --- a/tests/api/answer/services/test_stream_processor_utils.py +++ b/tests/api/answer/services/test_stream_processor_utils.py @@ -561,6 +561,29 @@ class TestGetPromptContent: content = sp._get_prompt_content() assert "source.summaries" in content + def test_null_prompt_id_agentic_agent_gets_agentic_preset(self): + from application.api.answer.services.stream_processor import ( + StreamProcessor, + ) + # PG ``agents.prompt_id`` is NULL for agents that never chose a + # prompt — the agentic swap must still apply. + sp = StreamProcessor({}, {"sub": "u"}) + sp.agent_config = {"prompt_id": None, "agent_type": "agentic"} + content = sp._get_prompt_content() + assert content is not None + assert "`search` tool" in content + assert "source.summaries" not in content + + def test_null_prompt_id_classic_agent_gets_default_preset(self): + from application.api.answer.services.stream_processor import ( + StreamProcessor, + ) + sp = StreamProcessor({}, {"sub": "u"}) + sp.agent_config = {"prompt_id": None} + content = sp._get_prompt_content() + assert content is not None + assert "source.summaries" in content + class TestPreFetchDocs: def test_skips_when_no_active_docs(self): @@ -627,6 +650,7 @@ class TestPreFetchTools: ) sp = StreamProcessor({}, {"sub": "no-tools-user"}) + sp._prompt_content = "No template syntax here" with _patch_db(pg_conn), patch( "application.api.answer.services.stream_processor.settings.ENABLE_TOOL_PREFETCH", True, @@ -634,7 +658,7 @@ class TestPreFetchTools: got = sp.pre_fetch_tools() assert got is None - def test_no_template_skips_only_default_rows_not_explicit(self, pg_conn): + def test_unresolvable_prompt_prefetches_only_explicit_rows(self, pg_conn): from application.api.answer.services.stream_processor import ( StreamProcessor, ) @@ -646,6 +670,11 @@ class TestPreFetchTools: user_id="u-explicit-prefetch", name="read_webpage", status=True ) sp = StreamProcessor({}, {"sub": "u-explicit-prefetch"}) + # A broken custom prompt id disables action filtering; explicit + # rows still prefetch, defaults stay skipped. + sp.agent_config = { + "prompt_id": "00000000-0000-0000-0000-000000000000" + } fetched = [] def _fake_fetch(tool_doc, required_actions): diff --git a/tests/api/answer/test_stream_processor.py b/tests/api/answer/test_stream_processor.py index 2e949b77..de069a3a 100644 --- a/tests/api/answer/test_stream_processor.py +++ b/tests/api/answer/test_stream_processor.py @@ -335,7 +335,7 @@ class TestGetPromptContent: assert result1 is not None @pytest.mark.unit - def test_no_prompt_id(self): + def test_no_prompt_id_falls_back_to_default_preset(self): mock_db = MagicMock() with patch("application.api.answer.services.stream_processor.MongoDB") as MockMongo, \ patch("application.api.answer.services.stream_processor.settings") as mock_settings: @@ -345,7 +345,9 @@ class TestGetPromptContent: from application.api.answer.services.stream_processor import StreamProcessor sp = StreamProcessor(request_data={}, decoded_token={"sub": "u"}) sp.agent_config = {} - assert sp._get_prompt_content() is None + content = sp._get_prompt_content() + assert content is not None + assert "source.summaries" in content @pytest.mark.unit def test_invalid_prompt_id_returns_none(self): @@ -381,7 +383,7 @@ class TestGetPromptContent: class TestGetRequiredToolActions: @pytest.mark.unit - def test_no_prompt_returns_none(self): + def test_no_prompt_id_uses_default_preset(self): mock_db = MagicMock() with patch("application.api.answer.services.stream_processor.MongoDB") as MockMongo, \ patch("application.api.answer.services.stream_processor.settings") as mock_settings: @@ -391,7 +393,9 @@ class TestGetRequiredToolActions: from application.api.answer.services.stream_processor import StreamProcessor sp = StreamProcessor(request_data={}, decoded_token={"sub": "u"}) sp.agent_config = {} - assert sp._get_required_tool_actions() is None + # Missing prompt_id resolves to the default preset, so filtering + # stays enabled instead of falling back to "prefetch everything". + assert sp._get_required_tool_actions() is not None @pytest.mark.unit def test_no_template_syntax_returns_empty(self):