-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix deferred tool search keyword matching #5014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||||||||
| from dataclasses import dataclass | ||||||||||||||||||||||||||||||||||||||||||||
| from typing import Annotated, Any | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -18,6 +19,7 @@ | |||||||||||||||||||||||||||||||||||||||||||
| _DISCOVERED_TOOLS_METADATA_KEY = 'discovered_tools' | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| _MAX_SEARCH_RESULTS = 10 | ||||||||||||||||||||||||||||||||||||||||||||
| _SEARCH_TOKEN_RE = re.compile(r'[a-z0-9]+') | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| class _SearchToolArgs(TypedDict): | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -42,9 +44,8 @@ class _SearchToolArgs(TypedDict): | |||||||||||||||||||||||||||||||||||||||||||
| @dataclass(kw_only=True) | ||||||||||||||||||||||||||||||||||||||||||||
| class _SearchIndexEntry: | ||||||||||||||||||||||||||||||||||||||||||||
| name: str | ||||||||||||||||||||||||||||||||||||||||||||
| name_lower: str | ||||||||||||||||||||||||||||||||||||||||||||
| description: str | None | ||||||||||||||||||||||||||||||||||||||||||||
| description_lower: str | None | ||||||||||||||||||||||||||||||||||||||||||||
| search_terms: set[str] | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @dataclass(kw_only=True) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -90,9 +91,8 @@ async def get_tools(self, ctx: RunContext[AgentDepsT]) -> dict[str, ToolsetTool[ | |||||||||||||||||||||||||||||||||||||||||||
| search_index = [ | ||||||||||||||||||||||||||||||||||||||||||||
| _SearchIndexEntry( | ||||||||||||||||||||||||||||||||||||||||||||
| name=name, | ||||||||||||||||||||||||||||||||||||||||||||
| name_lower=name.lower(), | ||||||||||||||||||||||||||||||||||||||||||||
| description=tool.tool_def.description, | ||||||||||||||||||||||||||||||||||||||||||||
| description_lower=tool.tool_def.description.lower() if tool.tool_def.description else None, | ||||||||||||||||||||||||||||||||||||||||||||
| search_terms=self._search_terms(name, tool.tool_def.description), | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| for name, tool in deferred.items() | ||||||||||||||||||||||||||||||||||||||||||||
| if name not in discovered | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -148,6 +148,13 @@ async def call_tool( | |||||||||||||||||||||||||||||||||||||||||||
| return await self._search_tools(tool_args, tool) | ||||||||||||||||||||||||||||||||||||||||||||
| return await self.wrapped.call_tool(name, tool_args, ctx, tool) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||||||||||||||||||||||||
| def _search_terms(name: str, description: str | None) -> set[str]: | ||||||||||||||||||||||||||||||||||||||||||||
| search_terms = set(_SEARCH_TOKEN_RE.findall(name.lower())) | ||||||||||||||||||||||||||||||||||||||||||||
| if description: | ||||||||||||||||||||||||||||||||||||||||||||
| search_terms.update(_SEARCH_TOKEN_RE.findall(description.lower())) | ||||||||||||||||||||||||||||||||||||||||||||
| return search_terms | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| async def _search_tools(self, tool_args: dict[str, Any], search_tool: _SearchTool[AgentDepsT]) -> ToolReturn: | ||||||||||||||||||||||||||||||||||||||||||||
| """Search for tools matching the keywords. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -159,15 +166,16 @@ async def _search_tools(self, tool_args: dict[str, Any], search_tool: _SearchToo | |||||||||||||||||||||||||||||||||||||||||||
| if not keywords: | ||||||||||||||||||||||||||||||||||||||||||||
| raise ModelRetry('Please provide search keywords.') | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| terms = keywords.lower().split() | ||||||||||||||||||||||||||||||||||||||||||||
| terms = self._search_terms(keywords, None) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| matches: list[dict[str, str | None]] = [] | ||||||||||||||||||||||||||||||||||||||||||||
| scored_matches: list[tuple[int, dict[str, str | None]]] = [] | ||||||||||||||||||||||||||||||||||||||||||||
| for entry in search_tool.search_index: | ||||||||||||||||||||||||||||||||||||||||||||
| searchable = entry.name_lower + (' ' + entry.description_lower if entry.description_lower else '') | ||||||||||||||||||||||||||||||||||||||||||||
| if any(term in searchable for term in terms): | ||||||||||||||||||||||||||||||||||||||||||||
| matches.append({'name': entry.name, 'description': entry.description}) | ||||||||||||||||||||||||||||||||||||||||||||
| if len(matches) >= _MAX_SEARCH_RESULTS: | ||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||
| score = len(terms & entry.search_terms) | ||||||||||||||||||||||||||||||||||||||||||||
| if score == len(terms): | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+169
to
+174
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Non-alphanumeric keywords produce empty token set, matching ALL tools When the model sends keywords containing only non-alphanumeric characters (e.g.,
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||||||||||||||||
| scored_matches.append((score, {'name': entry.name, 'description': entry.description})) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| scored_matches.sort(key=lambda item: item[0], reverse=True) | ||||||||||||||||||||||||||||||||||||||||||||
| matches = [match for _, match in scored_matches[:_MAX_SEARCH_RESULTS]] | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+177
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Sorting by score is a no-op — all matched entries have identical scores At Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+169
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Behavioral change from OR to AND matching semantics may affect recorded VCR cassettes The old search used Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| tool_names = [match['name'] for match in matches] | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Docstring says "matches any term" but code requires ALL terms to match
The docstring at line 161 states the search "matches any term against tool names and descriptions", implying OR semantics. However, the new implementation at line 174 uses
if score == len(terms), which requires ALL search terms to be present in the entry'ssearch_termsset (AND semantics). The old code usedany(term in searchable for term in terms)which did match the "any term" description. This is a stale docstring that now contradicts the actual behavior, violating the coding guideline rule:198 ("Rename methods/functions when their behavior changes — names must reflect actual scope") fromagent_docs/index.md.(Refers to line 161)
Was this helpful? React with 👍 or 👎 to provide feedback.