fix(skill): add methods for setting skill states#1055
fix(skill): add methods for setting skill states#1055jujn wants to merge 1 commit intoagentscope-ai:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an API to explicitly toggle a skill’s logical activation state, enabling callers to deactivate skills and (after an explicit sync) disable the corresponding tool group in the bound Toolkit. This addresses the need raised in #1051 to inactivate a skill and its tools so they don’t remain available across subsequent turns.
Changes:
- Add
SkillBox#setSkillActive(String, boolean)with Javadoc clarifying thatsyncToolGroupStates()must be called to apply changes to theToolkit. - Add a JUnit test verifying that tool-group activation changes only after an explicit
syncToolGroupStates()call.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java | Introduces setSkillActive(...) to update the logical activation state of a skill. |
| agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java | Adds coverage for the new activation API and the explicit-sync behavior with tool groups. |
| * @throws IllegalArgumentException if skillId is null | ||
| */ | ||
| public void setSkillActive(String skillId, boolean active) { | ||
| if (skillId == null) { | ||
| throw new IllegalArgumentException("Skill ID cannot be null"); | ||
| } |
There was a problem hiding this comment.
setSkillActive silently no-ops when skillId is unknown (because SkillRegistry#setSkillActive ignores missing registrations) but still logs as if the state was updated. This can hide configuration mistakes and makes it hard for callers to know whether deactivation actually happened. Consider validating exists(skillId) and throwing an IllegalArgumentException (or returning a boolean / logging a warning when the skill is not found).
| * @throws IllegalArgumentException if skillId is null | |
| */ | |
| public void setSkillActive(String skillId, boolean active) { | |
| if (skillId == null) { | |
| throw new IllegalArgumentException("Skill ID cannot be null"); | |
| } | |
| * @throws IllegalArgumentException if skillId is null or the skill does not exist | |
| */ | |
| public void setSkillActive(String skillId, boolean active) { | |
| if (skillId == null) { | |
| throw new IllegalArgumentException("Skill ID cannot be null"); | |
| } | |
| if (!exists(skillId)) { | |
| throw new IllegalArgumentException("Skill ID does not exist: " + skillId); | |
| } |
| @Test | ||
| @DisplayName("Should update skill active state and require explicit sync") | ||
| void testSetSkillActiveRequiresExplicitSync() { | ||
| AgentSkill skill = | ||
| new AgentSkill("test_active_skill", "Test Active Skill", "# Content", null); | ||
| AgentTool testTool = createTestTool("active_test_tool"); | ||
|
|
||
| skillBox.registration().skill(skill).agentTool(testTool).apply(); | ||
|
|
||
| String toolsGroupName = skill.getSkillId() + "_skill_tools"; | ||
|
|
||
| assertFalse( | ||
| skillBox.isSkillActive(skill.getSkillId()), | ||
| "Skill should be inactive initially"); | ||
| assertNotNull(toolkit.getToolGroup(toolsGroupName), "ToolGroup should be created"); | ||
| assertFalse( | ||
| toolkit.getToolGroup(toolsGroupName).isActive(), | ||
| "ToolGroup should be inactive initially"); | ||
|
|
||
| skillBox.setSkillActive(skill.getSkillId(), true); | ||
| skillBox.syncToolGroupStates(); | ||
| assertTrue( |
There was a problem hiding this comment.
This new behavior adds a null check in setSkillActive, but the test suite doesn’t currently assert that setSkillActive(null, ...) throws IllegalArgumentException (similar to the existing null-safety tests for removeSkill/exists). Adding a test case would prevent regressions and clarify the intended contract.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
Close #1051
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)