-
Notifications
You must be signed in to change notification settings - Fork 247
refactor(skill): simplify skill lifecycle and improve tool group activation #615
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
refactor(skill): simplify skill lifecycle and improve tool group activation #615
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 skill management system to simplify its lifecycle and enhance the persistence of tool groups. By centralizing tool group activation within the SkillToolFactory and removing deactivation logic from the SkillHook, skills and their associated tools now remain active throughout a conversation once loaded. This change resolves an issue where tools would be prematurely deactivated, leading to a more robust and intuitive skill interaction experience. The accompanying test improvements ensure comprehensive validation of these new behaviors. 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 refactors the skill lifecycle management, simplifying the SkillHook and moving tool group activation logic to SkillToolFactory. The goal is to make skill tool groups persist across conversation turns. The changes are well-organized and supported by improved unit tests and new integration tests. However, I've identified a potential issue in the new activation logic where loading a new skill might deactivate previously loaded ones, which seems to contradict the main goal of this refactoring. Please see my specific comment for details.
agentscope-core/src/main/java/io/agentscope/core/skill/SkillToolFactory.java
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 skill lifecycle management to simplify how skills and their associated tool groups are activated. The main change is moving tool group activation from SkillHook to SkillToolFactory, ensuring that once a skill is loaded, its tools persist throughout the conversation instead of being deactivated between turns.
Changes:
- Removed skill deactivation logic from SkillHook (previously triggered on PreCallEvent/PostCallEvent)
- Tool group activation now happens in SkillToolFactory when a skill is loaded via load_skill_through_path
- SkillHook simplified to only inject skill prompts during PreReasoningEvent
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| SkillToolFactory.java | Added Toolkit binding support and tool group activation when loading skills |
| SkillHook.java | Removed PreCallEvent/PostCallEvent handlers, now only handles PreReasoningEvent for prompt injection |
| SkillBox.java | Added bindToolkit() method with documentation to support ReActAgent's deep-copied Toolkit |
| SkillRuntimeIntegrationTest.java | New integration test suite for end-to-end skill activation flow |
| SkillHookTest.java | Refactored to focus on unit testing prompt injection behavior |
| SkillBoxToolsTest.java | Enhanced tests to verify tool group activation when loading skills |
| SkillBoxTest.java | Added tests for bindToolkit() functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agentscope-core/src/test/java/io/agentscope/core/skill/SkillRuntimeIntegrationTest.java
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxToolsTest.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/skill/SkillToolFactory.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/skill/SkillToolFactory.java
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/skill/SkillToolFactory.java
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxToolsTest.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/skill/SkillToolFactory.java
Outdated
Show resolved
Hide resolved
…vation (agentscope-ai#615) ## AgentScope-Java Version 1.0.7 ## Description refactor(skill): simplify skill lifecycle and improve tool group activation BREAKING CHANGE: Skill tool groups now persist across conversation turns Changes: - Remove skill deactivation logic from SkillHook's PreCallEvent/PostCallEvent handlers - Move tool group activation from SkillHook to SkillToolFactory - Add Toolkit binding to SkillToolFactory for direct tool group management - Simplify SkillHook to only handle skill prompt injection in PreReasoningEvent Test improvements: - Refactor SkillHookTest to focus on unit testing hook responsibilities - Add SkillRuntimeIntegrationTest for end-to-end skill activation flow - Add tests for SkillBox.bindToolkit() method - Add tests for tool group activation when loading skills - Remove redundant integration tests from SkillHookTest This change ensures that once a skill is loaded, its associated tools remain available throughout the conversation, eliminating the need to reload skills in each turn. The tool group is now automatically activated when a skill is loaded through the load_skill_through_path tool. Fixes: Tool groups being deactivated between conversation turns relate agentscope-ai#584 ## Checklist Please check the following items before code is ready to be reviewed. - [x] Code has been formatted with `mvn spotless:apply` - [x] All tests are passing (`mvn test`) - [x] Javadoc comments are complete and follow project conventions - [x] Related documentation has been updated (e.g. links, examples, etc.) - [x] Code is ready for review
AgentScope-Java Version
1.0.7
Description
refactor(skill): simplify skill lifecycle and improve tool group activation
BREAKING CHANGE: Skill tool groups now persist across conversation turns
Changes:
Test improvements:
This change ensures that once a skill is loaded, its associated tools remain
available throughout the conversation, eliminating the need to reload skills
in each turn. The tool group is now automatically activated when a skill is
loaded through the load_skill_through_path tool.
Fixes: Tool groups being deactivated between conversation turns
relate #584
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)