Skip to content

fix(skill): bind ALL agent tools when multiple are registered on one skill #2372

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
gxgeek-n wants to merge 1 commit into
base: main
Choose a base branch
from
+78 −16
Open

fix(skill): bind ALL agent tools when multiple are registered on one skill #2372

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
65 changes: 49 additions & 16 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 @@ -383,7 +383,7 @@ public static class SkillRegistration {
private Toolkit toolkit;
private AgentSkill skill;
private Object toolObject;
private AgentTool agentTool;
private final List<AgentTool> agentTools = new ArrayList<>();
private McpClientWrapper mcpClientWrapper;
private SubAgentProvider<?> subAgentProvider;
private SubAgentConfig subAgentConfig;
Expand Down Expand Up @@ -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;
Comment on lines 434 to 436
}

Expand Down Expand Up @@ -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(),"
Expand Down Expand Up @@ -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) {
Comment on lines 597 to 601
Expand All @@ -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 必须逐个独立注册，否则只有最后一个生效
Comment on lines +609 to +610
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();
}
}
}
}
Expand Down
29 changes: 29 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 @@ -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() {
Expand Down
Loading