diff --git a/agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java b/agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java index dc61ad9178..c0e63234a9 100644 --- a/agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java +++ b/agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java @@ -383,7 +383,7 @@ public static class SkillRegistration { private Toolkit toolkit; private AgentSkill skill; private Object toolObject; - private AgentTool agentTool; + private final List agentTools = new ArrayList<>(); private McpClientWrapper mcpClientWrapper; private SubAgentProvider subAgentProvider; private SubAgentConfig subAgentConfig; @@ -424,13 +424,19 @@ public SkillRegistration tool(Object toolObject) { } /** - * Set the AgentTool instance to register. + * Add an AgentTool instance to register. May be called multiple times to bind several + * tools to the same skill — every tool is bound into the skill's gated tool group + * (previously each call overwrote the previous one, so only the last tool was bound). * - * @param agentTool The AgentTool instance + *

A {@code null} argument is ignored, mirroring {@code Toolkit.ToolRegistration}. + * + * @param agentTool The AgentTool instance; ignored when {@code null} * @return This builder for chaining */ public SkillRegistration agentTool(AgentTool agentTool) { - this.agentTool = agentTool; + if (agentTool != null) { + this.agentTools.add(agentTool); + } return this; } @@ -511,7 +517,7 @@ public SkillRegistration subAgent(SubAgentProvider provider) { */ public SkillRegistration subAgent(SubAgentProvider provider, SubAgentConfig config) { if (this.toolObject != null - || this.agentTool != null + || !this.agentTools.isEmpty() || this.mcpClientWrapper != null) { throw new IllegalStateException( "Cannot set multiple registration types. Use only one of: tool()," @@ -593,7 +599,7 @@ public void apply() { skillBox.registerSkill(skill); if (toolObject != null - || agentTool != null + || !agentTools.isEmpty() || mcpClientWrapper != null || subAgentProvider != null) { if (toolkit == null && (toolkit = skillBox.toolkit) == null) { @@ -604,17 +610,68 @@ public void apply() { if (toolkit.getToolGroup(skillToolGroup) == null) { toolkit.createToolGroup(skillToolGroup, skillToolGroup, false); } - toolkit.registration() - .group(skillToolGroup) - .presetParameters(presetParameters) - .extendedModel(extendedModel) - .enableTools(enableTools) - .disableTools(disableTools) - .agentTool(agentTool) - .tool(toolObject) - .mcpClient(mcpClientWrapper) - .subAgent(subAgentProvider, subAgentConfig) - .apply(); + // A SkillRegistration still binds exactly one registration kind. Previously all + // four values were handed to a single Toolkit.ToolRegistration, whose exactly-one + // check rejected mixtures. Registering each agent tool separately (needed so that + // several tools can share one skill) bypasses that check, so the invariant is + // enforced here instead — with an error naming the kinds that were combined. + int registrationKinds = + (agentTools.isEmpty() ? 0 : 1) + + (toolObject != null ? 1 : 0) + + (mcpClientWrapper != null ? 1 : 0) + + (subAgentProvider != null ? 1 : 0); + if (registrationKinds > 1) { + throw new IllegalStateException( + "A skill registration must bind exactly one of agentTool(), tool()," + + " mcpClient() or subAgent(), but got: " + + (agentTools.isEmpty() ? "" : "agentTool ") + + (toolObject != null ? "tool " : "") + + (mcpClientWrapper != null ? "mcpClient " : "") + + (subAgentProvider != null ? "subAgent" : "").trim()); + } + // Toolkit.ToolRegistration binds a single tool per call (exactly-one check), so a + // skill carrying several agent tools must register them one at a time — otherwise + // only the last one takes effect. + for (AgentTool tool : agentTools) { + toolkit.registration() + .group(skillToolGroup) + .presetParameters(presetParameters) + .extendedModel(extendedModel) + .enableTools(enableTools) + .disableTools(disableTools) + .agentTool(tool) + .apply(); + } + if (toolObject != null) { + toolkit.registration() + .group(skillToolGroup) + .presetParameters(presetParameters) + .extendedModel(extendedModel) + .enableTools(enableTools) + .disableTools(disableTools) + .tool(toolObject) + .apply(); + } + if (mcpClientWrapper != null) { + toolkit.registration() + .group(skillToolGroup) + .presetParameters(presetParameters) + .extendedModel(extendedModel) + .enableTools(enableTools) + .disableTools(disableTools) + .mcpClient(mcpClientWrapper) + .apply(); + } + if (subAgentProvider != null) { + toolkit.registration() + .group(skillToolGroup) + .presetParameters(presetParameters) + .extendedModel(extendedModel) + .enableTools(enableTools) + .disableTools(disableTools) + .subAgent(subAgentProvider, subAgentConfig) + .apply(); + } } } } diff --git a/agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java b/agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java index a2cee901c7..a9183e03d6 100644 --- a/agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java +++ b/agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java @@ -187,6 +187,82 @@ void testSuccessfullyRegisterWhenOnlyAgentToolProvided() { assertNotNull(toolkit.getTool("agent_tool_only"), "Agent tool should be registered"); } + @Test + @DisplayName("Should bind ALL agent tools when multiple are registered on one skill") + void testMultipleAgentToolsAllBoundToSkillGroup() { + AgentTool first = createTestTool("multi_tool_first"); + AgentTool second = createTestTool("multi_tool_second"); + AgentSkill skill = + new AgentSkill( + "Multi Tool Skill", "Skill with two agent tools", "# Multi", null); + + // Two consecutive agentTool() calls: the second used to overwrite the first, so only + // "second" ended up bound into the gated group. + skillBox.registration().skill(skill).agentTool(first).agentTool(second).apply(); + + String groupName = skill.getSkillId() + "_skill_tools"; + assertNotNull(toolkit.getToolGroup(groupName), "skill tool group should exist"); + assertTrue( + toolkit.getToolGroup(groupName).getTools().contains("multi_tool_first"), + "first tool must also be bound into the skill group (regression: was" + + " overwritten)"); + assertTrue( + toolkit.getToolGroup(groupName).getTools().contains("multi_tool_second"), + "second tool must be bound into the skill group"); + assertFalse( + toolkit.getToolGroup(groupName).isActive(), + "skill tool group must start inactive (gated until skill is loaded)"); + + assertNotNull(toolkit.getTool("multi_tool_first")); + assertNotNull(toolkit.getTool("multi_tool_second")); + } + + @Test + @DisplayName("Should ignore a null agent tool instead of failing the registration") + void testNullAgentToolIsIgnored() { + AgentTool real = createTestTool("null_guard_tool"); + AgentSkill skill = + new AgentSkill("Null Guard Skill", "Skill with a null tool", "# Null", null); + + // A null must not be appended to the tool list — otherwise apply() would hand null to + // Toolkit.registration().agentTool(null) and fail the whole skill registration. + assertDoesNotThrow( + () -> + skillBox.registration() + .skill(skill) + .agentTool(null) + .agentTool(real) + .apply()); + + String groupName = skill.getSkillId() + "_skill_tools"; + assertNotNull(toolkit.getToolGroup(groupName), "skill tool group should exist"); + assertTrue( + toolkit.getToolGroup(groupName).getTools().contains("null_guard_tool"), + "the non-null tool must still be bound"); + } + + @Test + @DisplayName("Should reject mixing two registration kinds on one skill registration") + void testMixingRegistrationKindsIsRejected() { + AgentTool agentTool = createTestTool("mixed_agent_tool"); + AgentSkill skill = new AgentSkill("Mixed Skill", "Skill mixing kinds", "# Mixed", null); + + // Registering each agent tool separately bypasses Toolkit.ToolRegistration's + // exactly-one check, so SkillRegistration enforces the invariant itself. + IllegalStateException error = + assertThrows( + IllegalStateException.class, + () -> + skillBox.registration() + .skill(skill) + .agentTool(agentTool) + .tool(new Object()) + .apply()); + assertTrue( + error.getMessage().contains("agentTool") && error.getMessage().contains("tool"), + "error should name the combined kinds, got: " + error.getMessage()); + } + @Test @DisplayName("Should successfully register when only mcp client is provided") void testSuccessfullyRegisterWhenOnlyMcpClientProvided() {