Skip to content

fix(skill): add methods for setting skill states #1055

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.

Sign up for GitHub

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

Open
jujn wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −0
Open

fix(skill): add methods for setting skill states #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,29 @@ public boolean exists(String skillId) {
return skillRegistry.exists(skillId);
}

/**
* Sets the logical activation state of a specific skill.
*
* <p>When a skill is set to inactive, it is marked for deactivation. However,
* its associated tool group will <b>not</b> be immediately disabled in the underlying
* toolkit.
*
* <p><b>Important:</b> You must explicitly call {@link #syncToolGroupStates()} after
* modifying skill states to synchronize these changes with the bound toolkit. Only then
* will the agent be prevented from accessing the tools of inactive skills.
*
* @param skillId The ID of the skill to modify
* @param active true to mark the skill as active, false to mark as inactive
* @throws IllegalArgumentException if skillId is null
*/
public void setSkillActive(String skillId, boolean active) {
if (skillId == null) {
throw new IllegalArgumentException("Skill ID cannot be null");
}
Comment on lines +306 to +311
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
* @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);
}

Copilot uses AI. Check for mistakes.
skillRegistry.setSkillActive(skillId, active);
logger.debug("Skill '{}' active state set to {}", skillId, active);
}

/**
* Deactivates all skills.
*
Expand Down
46 changes: 46 additions & 0 deletions agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,52 @@ void testGetAllSkillIdsWithMultipleSkills() {
assertTrue(skillIds.contains(skill2.getSkillId()), "Should contain second skill ID");
assertTrue(skillIds.contains(skill3.getSkillId()), "Should contain third skill ID");
}

@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(
Comment on lines +262 to +283
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
skillBox.isSkillActive(skill.getSkillId()),
"Skill logical state should be active now");
assertTrue(
toolkit.getToolGroup(toolsGroupName).isActive(),
"ToolGroup should be active after sync");

skillBox.setSkillActive(skill.getSkillId(), false);

// The physical state (Toolkit) has not changed yet (the framework will not
// automatically synchronize for you)
assertFalse(
skillBox.isSkillActive(skill.getSkillId()),
"Skill logical state should be inactive");
assertTrue(
toolkit.getToolGroup(toolsGroupName).isActive(),
"ToolGroup should STILL be active before explicit sync");

skillBox.syncToolGroupStates();

assertFalse(
toolkit.getToolGroup(toolsGroupName).isActive(),
"ToolGroup should be inactive after sync");
}
}

@Nested
Expand Down
Loading