From f835b2db0fc9fcedfae7e77206a74bb892e1d163 Mon Sep 17 00:00:00 2001 From: gxgeek-n <189542381+gxgeek-n@users.noreply.github.com> Date: Fri, 24 Jul 2026 03:01:04 +0800 Subject: [PATCH] fix(skill): bind ALL agent tools when multiple are registered on one skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SkillRegistration.agentTool() was a single-slot field — calling it twice on one registration overwrote the first tool, so only the last AgentTool was bound into the skill's gated tool group ({skillId}_skill_tools). Any skill declaring multiple tools silently lost all but the last one: those tools were neither gated (visible without loading the skill) nor unlockable. Since Toolkit.ToolRegistration enforces exactly-one-tool-per-registration, SkillRegistration now accumulates agentTools into a list and apply() binds each one with its own Toolkit registration. Other tool kinds (toolObject / mcpClient / subAgent) also get their own registrations instead of sharing one chain that violated the exactly-one rule when combined. Regression test (SkillBoxTest#testMultipleAgentToolsAllBoundToSkillGroup): - fails on the v2.0.0 tag with "first tool must also be bound into the skill group (regression: was overwritten)" - passes with this fix; full SkillBoxTest suite green (16/16) --- .../io/agentscope/core/skill/SkillBox.java | 65 ++++++++++++++----- .../agentscope/core/skill/SkillBoxTest.java | 29 +++++++++ 2 files changed, 78 insertions(+), 16 deletions(-) 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..b41fdce1e2 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,15 @@ 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 * @return This builder for chaining */ public SkillRegistration agentTool(AgentTool agentTool) { - this.agentTool = agentTool; + this.agentTools.add(agentTool); return this; } @@ -511,7 +513,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 +595,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 +606,48 @@ 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(); + // Toolkit.ToolRegistration 每次只能注册一个工具（exactly-one 校验）， + // 多 tool 的 skill 必须逐个独立注册，否则只有最后一个生效 + 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..5efc59dcff 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,35 @@ 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); + + // 连续两次 agentTool()：此前第二次会覆盖第一次，导致只有 second 被绑进门控组 + 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 successfully register when only mcp client is provided") void testSuccessfullyRegisterWhenOnlyMcpClientProvided() {