-
Notifications
You must be signed in to change notification settings - Fork 2.5k
chore: small refactor on agent.rs #3703
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
| *tool_monitor = Some(ToolMonitor::new(max_repetitions)); | ||
| } | ||
|
|
||
| pub async fn get_tool_stats(&self) -> Option<HashMap<String, u32>> { |
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.
these functions are not used
| } | ||
|
|
||
| if let Some(response) = response { | ||
| let (tools_with_readonly_annotation, tools_without_annotation) = |
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.
extracted to process_tool_requests function
| ) | ||
| .await; | ||
|
|
||
| let mut tool_futures: Vec<(String, ToolStream)> = Vec::new(); |
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.
extracted to execute_approved_tools function
| } // Write lock is released here! | ||
| } | ||
|
|
||
| /// Get current progress information |
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.
it is not used
| self.conversation.lock().await.clone() | ||
| } | ||
|
|
||
| /// Check if the subagent has completed its task |
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.
not used
DOsinga
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 doing this. two small suggestions
crates/goose/src/agents/agent.rs
Outdated
| } | ||
|
|
||
| /// Execute approved tools and handle denied tools | ||
| async fn execute_approved_tools( |
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.
remove the comment and rename to handle approved_and_denied_tools?
| } | ||
|
|
||
| /// Process tool requests by categorizing them and recording them in the router selector | ||
| async fn process_tool_requests( |
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.
we're doing two things here, maybe would be better to just do record_tool_calls and move that to router_tool_selector to record more than one call and then we can just call that directly from agent with frontend_requests.iter().chain(&remaining_requests)
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 the review! Yes, the function has a couple of responsibilities. I feel the tool handling in reply could be cleaner with more refactoring and will have a followup pr to refactor more in this area.
* main: chore: small refactor on agent.rs (#3703) docs: Add GitMCP Tutorial to Extensions Library (#3716) chore: Speed up CI (#3711) Fix tool vector tests (#3709) docs: GitMCP Tutorial (#3708) Remove unused dependencies (#3626) feat: update Groq models for better tool calling support (#3676) chore: remove ffi libraries and related code (#3699) only run google analytics in prod (#3395) Fix typo in quickstart document (#3447) fix: pricing estimation for OpenRouter in goose-cli (#3675) fix: escape control characters in LLM tool call arguments JSON (#2893) feat(githubcopilot): add ability to fetch supported models (#2717) Create a message ID for tool response messages (#3591) fix: Fixed 404 broken link to extensions page in index.md (#3623)
Why
Start small refactoring to see whether we can reuse some logics of agent.rs in subagent.rs to avoid duplications
What