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

Author: fang-tech

agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java

@@ -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();
             }
         }
     }

agentscope-core/src/main/java/io/agentscope/core/tool/Toolkit.java

@@ -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;
         }
@@ -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;
         }
@@ -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;
         }
@@ -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;
@@ -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) {
@@ -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()");
             }
         }
     }

agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java

@@ -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() {