fix(skill): bind ALL agent tools when multiple are registered on one skill - #2372
Open
gxgeek-n wants to merge 2 commits into
Open
fix(skill): bind ALL agent tools when multiple are registered on one skill#2372gxgeek-n wants to merge 2 commits into
gxgeek-n wants to merge 2 commits 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)
|
|
Contributor
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. |
Comment on lines
434
to
436
| public SkillRegistration agentTool(AgentTool agentTool) { | ||
| this.agentTool = agentTool; | ||
| this.agentTools.add(agentTool); | ||
| return this; |
Comment on lines
597
to
601
| if (toolObject != null | ||
| || agentTool != null | ||
| || !agentTools.isEmpty() | ||
| || mcpClientWrapper != null | ||
| || subAgentProvider != null) { | ||
| if (toolkit == null && (toolkit = skillBox.toolkit) == null) { |
Comment on lines
+609
to
+610
| // 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! |
This comment was marked as abuse.
This comment was marked as abuse.
oss-maintainer
approved these changes
Jul 24, 2026
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"
Contributor
Author
|
Friendly ping — this one looks ready to merge from my side:
I also checked the 15 commits merged into The branch is currently 14 commits behind For context on why this matters: with multiple |
…ments Three review items on this PR: 1. agentTool(null) appended null to the list, so apply() handed null to Toolkit.registration().agentTool(null) and failed the whole skill registration. Now ignored, mirroring Toolkit.ToolRegistration. 2. Registering each agent tool via its own Toolkit.ToolRegistration bypassed that class's exactly-one check, so mixing kinds (agentTool + tool + mcpClient + subAgent) silently started to succeed and behaved differently depending on call order. That was an unintended side effect of this PR — the goal was only "several agent tools on one skill", not relaxing kind exclusivity. The invariant is now enforced in SkillRegistration.apply() with an error naming the combined kinds. 3. Translated the two non-English comments to English per repo convention. Adds two tests: null agent tool is ignored, and mixing two kinds is rejected. SkillBoxTest: 18 tests, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.