fix(skill): bind ALL agent tools when multiple are registered on one skill#2372
fix(skill): bind ALL agent tools when multiple are registered on one skill#2372gxgeek-n wants to merge 1 commit into
Conversation
…skill
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)
|
|
There was a problem hiding this comment.
Pull request overview
Fixes a regression in SkillBox.SkillRegistration where registering multiple AgentTools on a single skill would overwrite earlier tools, causing only the last tool to be bound into the skill’s gated tool group.
Changes:
- Change
SkillRegistration.agentToolfrom a single field to a list and register each agent tool into the gated group. - Split
apply()into multipleToolkit.registration()calls (one per tool/type) to satisfy Toolkit’s exactly-one-tool-per-registration constraint. - Add a regression test ensuring multiple agent tools are all present in the skill’s gated tool group and that the group remains inactive by default.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java | Accumulates multiple AgentTools and registers them individually into the skill tool group. |
| agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java | Adds regression coverage for binding multiple agent tools to a single skill group. |
| public SkillRegistration agentTool(AgentTool agentTool) { | ||
| this.agentTool = agentTool; | ||
| this.agentTools.add(agentTool); | ||
| return this; |
| if (toolObject != null | ||
| || agentTool != null | ||
| || !agentTools.isEmpty() | ||
| || mcpClientWrapper != null | ||
| || subAgentProvider != null) { | ||
| if (toolkit == null && (toolkit = skillBox.toolkit) == null) { |
| // Toolkit.ToolRegistration 每次只能注册一个工具（exactly-one 校验）， | ||
| // 多 tool 的 skill 必须逐个独立注册，否则只有最后一个生效 |
| new AgentSkill( | ||
| "Multi Tool Skill", "Skill with two agent tools", "# Multi", null); | ||
|
|
||
| // 连续两次 agentTool()：此前第二次会覆盖第一次，导致只有 second 被绑进门控组 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @gxgeek-n please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
LGTM. Looks good.
Automated review by "github-manager-bot"
Summary
SkillBox.SkillRegistration.agentTool()was a single-slot field. Calling it twice on one registration (e.g. a skill that declares two tools) overwrote the first tool, so only the lastAgentToolwas bound into the skill's gated tool group ({skillId}_skill_tools).Impact on users: any skill declaring multiple tools silently loses all but the last one — those tools are neither gated (invisible before the skill is loaded) nor unlockable via
load_skill_through_path. We hit this in production with skills whosetoolslist has more than one entry.Root cause
SkillRegistration.agentToolwasprivate AgentTool agentTool;— each call overwrote the previous value.apply()bound only that one surviving field into the group.Toolkit.ToolRegistrationenforces exactly-one-tool-per-registration, so chaining multiple tools on one registration can't work anyway — each tool needs its own registration.Fix
agentToolsis now aList<AgentTool>; the singularagentTool()method appends (backward compatible).apply()binds each accumulated tool with its ownToolkit.registration()into the skill group.toolObject/mcpClient/subAgent) also get their own registrations instead of sharing one chain (which violated the exactly-one rule whenever more than one kind was set).Tests
New regression test
SkillBoxTest#testMultipleAgentToolsAllBoundToSkillGroup:first tool must also be bound into the skill group (regression: was overwritten) ==> expected: <true> but was: <false>SkillBoxTestsuite green (16/16)Also verified: group still starts inactive (gating semantics unchanged), single-tool registration unchanged.