mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 20:14:16 +00:00
fix: mini tool call issues
This commit is contained in:
1 parent
636d4d35d1
commit
b8eaa5a68d
5 files changed
+110
-13
No files matched your search
@@ -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
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in new issue
Block a user