-
Notifications
You must be signed in to change notification settings - Fork 259
refactor(tool): make tool:tool groups = n:m, not n:1 #701
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?
refactor(tool): make tool:tool groups = n:m, not n:1 #701
Conversation
Summary of ChangesHello @fang-tech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the tool management system to support a more flexible many-to-many relationship between tools and groups. Previously, a tool could only belong to one group; now, a tool can be part of multiple groups, and its availability is determined by the activation status of any of its associated groups. This change improves the modularity and reusability of tools within the system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request successfully refactors the tool-to-group relationship from a 1-to-n mapping to a more flexible n-to-m mapping. The changes are well-implemented across the core logic in RegisteredToolFunction, ToolGroupManager, ToolExecutor, and ToolSchemaProvider. The removal of groupName from RegisteredToolFunction and the introduction of a new mapping in ToolGroupManager are clean and effective. I particularly appreciate the thorough updates to the test suite, including new tests that specifically validate the many-to-many behavior. I have a couple of minor suggestions in a new test file to improve consistency and robustness. Overall, this is a solid and well-executed refactoring.
agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java
Show resolved
Hide resolved
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.
Pull request overview
This PR refactors the tool-to-tool-group relationship from n:1 (many tools to one group) to n:m (many-to-many), enabling tools to belong to multiple groups simultaneously. A tool is now considered active if it's ungrouped or belongs to at least one active group.
Changes:
- Removed the
groupNamefield fromRegisteredToolFunction, shifting group membership management toToolGroupManager - Added bidirectional mapping in
ToolGroupManager(tool -> groups) with new methodsisActiveTool(),isGroupedTool(), andgetActiveToolNames() - Updated tool activation logic in
ToolExecutorandToolSchemaProviderto check if tools belong to any active group - Added comprehensive tests demonstrating n:m relationships for both direct tool registration and skill-based tool sharing
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/tool/RegisteredToolFunction.java | Removed groupName field and related methods; simplified constructor to focus on schema extension and MCP metadata |
| agentscope-core/src/main/java/io/agentscope/core/tool/ToolGroupManager.java | Added bidirectional tool-group mapping and new methods for checking tool activation across multiple groups |
| agentscope-core/src/main/java/io/agentscope/core/tool/ToolExecutor.java | Updated authorization check to use isActiveTool() instead of checking single group membership |
| agentscope-core/src/main/java/io/agentscope/core/tool/ToolSchemaProvider.java | Updated filtering logic to check if tools are in any active group using new group manager methods |
| agentscope-core/src/main/java/io/agentscope/core/tool/Toolkit.java | Updated tool registration to remove groupName parameter from RegisteredToolFunction constructor |
| agentscope-core/src/test/java/io/agentscope/core/tool/ToolkitTest.java | Added test verifying tools can belong to multiple groups and are accessible when any assigned group is active |
| agentscope-core/src/test/java/io/agentscope/core/tool/ToolSchemaProviderTest.java | Updated tests to use addToolToGroup() instead of passing groupName in constructor |
| agentscope-core/src/test/java/io/agentscope/core/tool/ToolRegistryTest.java | Updated test fixture constructors to remove groupName parameter |
| agentscope-core/src/test/java/io/agentscope/core/tool/ToolGroupManagerTest.java | Renamed test methods and added test for isActiveTool() behavior |
| agentscope-core/src/test/java/io/agentscope/core/tool/RegisteredToolFunctionTest.java | Updated tests to reflect constructor changes and removed test for deprecated getGroupName() |
| agentscope-core/src/test/java/io/agentscope/core/tool/MetaToolFactoryTest.java | Updated tests to use addToolToGroup() and removed empty lines |
| agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java | Added comprehensive test demonstrating shared tool across multiple skills with proper activation semantics |
agentscope-core/src/main/java/io/agentscope/core/tool/ToolGroupManager.java
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.8, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
close&fix #612 #671 #682
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)