-
Notifications
You must be signed in to change notification settings - Fork 2.4k
chord: refactor tool route #3732
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
Conversation
| let (frontend_requests, remaining_requests, filtered_response) = | ||
| self.categorize_tool_requests(response).await; | ||
|
|
||
| // Record tool calls in the router selector |
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.
moved this logic outside of this function
| } else if tool_call.name == ROUTER_VECTOR_SEARCH_TOOL_NAME | ||
| || tool_call.name == ROUTER_LLM_SEARCH_TOOL_NAME | ||
| { | ||
| let selector = self.router_tool_selector.lock().await.clone(); |
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.
Moved the logic to tool_route_manager dispatch_route_search_tool function
| } | ||
| }; | ||
|
|
||
| // Append final_output tool if present (for structured output recipes, [Issue #3700](https://github.com/block/goose/issues/3700) |
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.
this is no longer needed as we used disabled the vector or llm strategy for recipe
| }) | ||
| .map_err(|e| ToolError::ExecutionError(e.to_string())); | ||
|
|
||
| drop(extension_manager); |
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.
need to release the lock as later on it uses read lock
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.
nice
* main: Must have missed this one (#3733)
wendytang
left a comment
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.
thanks for cleaning this up - we actually don't use vector search strategy anymore as well
* main: chord: refactor tool route (#3732)
* 'main' of github.com:block/goose: feat: Allow configuring hints filename(s) (#3269) Add support for mouse back nav button to Settings screen (#3195) chore: Remove the wrong tailwind package (#3754) chore: fix typo in desktop readme for goosed (#3752) feat: upgrade rmcp (#3738) feat: allow users view and edit their non-secret config's (#3005) fix: View extensions link (#3751) chord: refactor tool route (#3732)
|
Thank you @wendytang for review!
|
|
Why
What
tool_route_managerto handle tool route/search logicsNoneRouteToolStrategy to disable smart route strategies. This is for goose recipe to use the extension tools specified in the recipe.