Skip to content

refactor(core): refactor toolkit and skillBox registration #377

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

Merged
AlbumenJ merged 6 commits into from
Dec 29, 2025
Merged

refactor(core): refactor toolkit and skillBox registration #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16ede3c
feat(core): refactor toolkit and skillBox registration
fang-tech Dec 27, 2025
7e723ec
fix: fix subAgent() set
fang-tech Dec 27, 2025
1b9803a
refactor toolkit tests
fang-tech Dec 27, 2025
e7fcbf3
update exception message
fang-tech Dec 27, 2025
76055cf
fix null tool test
fang-tech Dec 27, 2025
129fd7b
fix up: reduce tool count when call tool(null)
fang-tech Dec 28, 2025
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
39 changes: 17 additions & 22 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 @@ -461,34 +461,29 @@ public void apply() {
}
skillBox.registerSkill(skill);

if (toolObject != null || agentTool != null || mcpClientWrapper != null) {
if (toolObject != null
|| agentTool != null
|| mcpClientWrapper != null
|| subAgentProvider != null) {
if (toolkit == null && (toolkit = skillBox.toolkit) == null) {
throw new IllegalStateException("Must call toolkit() before apply()");
throw new IllegalStateException(
"Must bind toolkit or call toolkit() before apply()");
}
String skillToolGroup = skill.getSkillId() + "_skill_tools";
if (toolkit.getToolGroup(skillToolGroup) == null) {
toolkit.createToolGroup(skillToolGroup, skillToolGroup, false);
}
Toolkit.ToolRegistration toolRegistration =
toolkit.registration()
.group(skillToolGroup)
.presetParameters(presetParameters)
.extendedModel(extendedModel)
.enableTools(enableTools)
.disableTools(disableTools);
if (toolObject != null) {
toolRegistration.tool(toolObject);
}
if (agentTool != null) {
toolRegistration.agentTool(agentTool);
}
if (mcpClientWrapper != null) {
toolRegistration.mcpClient(mcpClientWrapper);
}
if (subAgentProvider != null) {
toolRegistration.subAgent(subAgentProvider, subAgentConfig);
}
toolRegistration.apply();
toolkit.registration()
.group(skillToolGroup)
.presetParameters(presetParameters)
.extendedModel(extendedModel)
.enableTools(enableTools)
.disableTools(disableTools)
.agentTool(agentTool)
.tool(toolObject)
.mcpClient(mcpClientWrapper)
.subAgent(subAgentProvider, subAgentConfig)
.apply();
}
}
}
Expand Down
52 changes: 19 additions & 33 deletions agentscope-core/src/main/java/io/agentscope/core/tool/Toolkit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,13 +696,6 @@ private ToolRegistration(Toolkit toolkit) {
* @return This builder for chaining
*/
public ToolRegistration tool(Object toolObject) {
if (this.agentTool != null
|| this.mcpClientWrapper != null
|| this.subAgentProvider != null) {
throw new IllegalStateException(
"Cannot set multiple registration types. Use only one of: tool(),"
+ " agentTool(), mcpClient(), or subAgent().");
}
this.toolObject = toolObject;
return this;
}
Expand All @@ -714,13 +707,6 @@ public ToolRegistration tool(Object toolObject) {
* @return This builder for chaining
*/
public ToolRegistration agentTool(AgentTool agentTool) {
if (this.toolObject != null
|| this.mcpClientWrapper != null
|| this.subAgentProvider != null) {
throw new IllegalStateException(
"Cannot set multiple registration types. Use only one of: tool(),"
+ " agentTool(), mcpClient(), or subAgent().");
}
this.agentTool = agentTool;
return this;
}
Expand All @@ -732,13 +718,6 @@ public ToolRegistration agentTool(AgentTool agentTool) {
* @return This builder for chaining
*/
public ToolRegistration mcpClient(McpClientWrapper mcpClientWrapper) {
if (this.toolObject != null
|| this.agentTool != null
|| this.subAgentProvider != null) {
throw new IllegalStateException(
"Cannot set multiple registration types. Use only one of: tool(),"
+ " agentTool(), mcpClient(), or subAgent().");
}
this.mcpClientWrapper = mcpClientWrapper;
return this;
}
Expand Down Expand Up @@ -808,13 +787,6 @@ public ToolRegistration subAgent(SubAgentProvider<?> provider) {
* @see SubAgentConfig#defaults()
*/
public ToolRegistration subAgent(SubAgentProvider<?> provider, SubAgentConfig config) {
if (this.toolObject != null
|| this.agentTool != null
|| this.mcpClientWrapper != null) {
throw new IllegalStateException(
"Cannot set multiple registration types. Use only one of: tool(),"
+ " agentTool(), mcpClient(), or subAgent().");
}
this.subAgentProvider = provider;
this.subAgentConfig = config;
return this;
Expand Down Expand Up @@ -893,9 +865,27 @@ public ToolRegistration extendedModel(ExtendedModel extendedModel) {
/**
* Apply the registration with all configured options.
*
* @throws IllegalStateException if none of tool(), agentTool(), or mcpClient() was set
* @throws IllegalStateException if none of tool(), agentTool(), mcpClient() or subAgent() was set
* @throws IllegalStateException if set multiple of: tool(), agentTool(), mcpClient(), or subAgent().
*/
public void apply() {
int toolCount = 0;
if (toolObject != null) toolCount++;
if (agentTool != null) toolCount++;
if (mcpClientWrapper != null) toolCount++;
if (subAgentProvider != null) toolCount++;

if (toolCount == 0) {
throw new IllegalStateException(
"Must call one of: tool(), agentTool(), mcpClient(), or subAgent() before"
+ " apply()");
}
if (toolCount > 1) {
throw new IllegalStateException(
"Cannot set multiple registration types. Use only one of: tool(),"
+ " agentTool(), mcpClient(), or subAgent().");
}

if (toolObject != null) {
toolkit.registerTool(toolObject, groupName, extendedModel, presetParameters);
} else if (agentTool != null) {
Expand All @@ -917,10 +907,6 @@ public void apply() {
} else if (subAgentProvider != null) {
SubAgentTool subAgentTool = new SubAgentTool(subAgentProvider, subAgentConfig);
toolkit.registerAgentTool(subAgentTool, groupName, extendedModel, null, null);
} else {
throw new IllegalStateException(
"Must call one of: tool(), agentTool(), mcpClient(), or subAgent() before"
+ " apply()");
}
}
}
Expand Down
122 changes: 0 additions & 122 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 @@ -145,128 +145,6 @@ void testThrowExceptionForNullSkillIdInOperations() {
assertThrows(IllegalArgumentException.class, () -> skillBox.exists(null));
}

@Test
@DisplayName("Should register skill with tool object")
void testRegisterSkillWithToolObject() {
AgentSkill skill =
new AgentSkill("Test Tool Skill", "Skill with tool object", "# Test Tool", null);

skillBox.registration().skill(skill).tool(new TestToolObject()).apply();

assertTrue(skillBox.exists(skill.getSkillId()));
AgentTool tool = toolkit.getTool("test_tool_method");
assertNotNull(tool, "Tool from tool object should be registered");
assertEquals("test_tool_method", tool.getName());
}

@Test
@DisplayName("Should register skill with mcp client")
void testRegisterSkillWithMcpClient() {
// Mock MCP client
McpClientWrapper mcpClient = mock(McpClientWrapper.class);
McpSchema.Tool mockToolInfo =
new McpSchema.Tool(
"mcp_test_tool",
null,
"Test MCP tool",
new McpSchema.JsonSchema("object", null, null, null, null, null),
null,
null,
null);
when(mcpClient.listTools()).thenReturn(Mono.just(List.of(mockToolInfo)));
when(mcpClient.isInitialized()).thenReturn(true);
when(mcpClient.initialize()).thenReturn(Mono.empty());
when(mcpClient.getName()).thenReturn("test-mcp-client");

AgentSkill skill = new AgentSkill("MCP Skill", "Skill with MCP client", "# MCP Test", null);

skillBox.registration().skill(skill).mcpClient(mcpClient).apply();

assertTrue(skillBox.exists(skill.getSkillId()));
AgentTool tool = toolkit.getTool("mcp_test_tool");
assertNotNull(tool, "Tool from MCP client should be registered");
assertEquals("mcp_test_tool", tool.getName());
}

@Test
@DisplayName("Should throw exception when registering multiple tool types")
void testThrowExceptionWhenRegisteringMultipleToolTypes() {
AgentTool agentTool = createTestTool("custom_agent_tool");
TestToolObject toolObject = new TestToolObject();

AgentSkill skill =
new AgentSkill(
"Mixed Skill",
"Skill with both agent tool and tool object",
"# Mixed",
null);

// Should throw when trying to register both agentTool and tool object
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
() ->
skillBox.registration()
.skill(skill)
.agentTool(agentTool)
.tool(toolObject)
.apply());

assertTrue(
exception.getMessage().contains("Cannot set multiple registration types"),
"Exception message should mention multiple registration types");
}

@Test
@DisplayName("Should throw exception when registering tool object and mcp client")
void testThrowExceptionWhenRegisteringToolObjectAndMcpClient() {
TestToolObject toolObject = new TestToolObject();
McpClientWrapper mcpClient = mock(McpClientWrapper.class);

AgentSkill skill =
new AgentSkill("Mixed Skill", "Skill with multiple types", "# Mixed", null);

// Should throw when trying to register both tool object and mcp client
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
() ->
skillBox.registration()
.skill(skill)
.tool(toolObject)
.mcpClient(mcpClient)
.apply());

assertTrue(
exception.getMessage().contains("Cannot set multiple registration types"),
"Exception message should mention multiple registration types");
}

@Test
@DisplayName("Should throw exception when registering agent tool and mcp client")
void testThrowExceptionWhenRegisteringAgentToolAndMcpClient() {
AgentTool agentTool = createTestTool("custom_agent_tool");
McpClientWrapper mcpClient = mock(McpClientWrapper.class);

AgentSkill skill =
new AgentSkill("Mixed Skill", "Skill with multiple types", "# Mixed", null);

// Should throw when trying to register both agent tool and mcp client
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
() ->
skillBox.registration()
.skill(skill)
.agentTool(agentTool)
.mcpClient(mcpClient)
.apply());

assertTrue(
exception.getMessage().contains("Cannot set multiple registration types"),
"Exception message should mention multiple registration types");
}

@Test
@DisplayName("Should successfully register when only tool object is provided")
void testSuccessfullyRegisterWhenOnlyToolObjectProvided() {
Expand Down
Loading
Loading