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 2 commits into
base: main
Choose a base branch
from
+150 −17
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
2 commits
Select commit Hold shift + click to select a range
f835b2d
fix(skill): bind ALL agent tools when multiple are registered on one …
gxgeek-n Jul 23, 2026
708f36c
fix(skill): address review — null tool, kind exclusivity, English com…
gxgeek-n Jul 29, 2026
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
91 changes: 74 additions & 17 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,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
* <p>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;
Comment on lines 436 to 440
}

Expand Down Expand Up @@ -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(),"
Expand Down Expand Up @@ -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) {
Comment on lines 601 to 605
Expand All @@ -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();
}
}
}
}
Expand Down
76 changes: 76 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,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() {
Expand Down
Loading