refactor(skill): consolidate skill discovery tools into single load_skill_through_path tool#505
Conversation
…e tool. Place the Skill built-in tool into the SkilltoolFactory to create it. Structure the skill system prompt using XML tags
There was a problem hiding this comment.
Pull request overview
This PR consolidates the skill discovery mechanism by merging three separate tools (skill_md_load_tool, skill_resources_load_tool, and get_all_resources_path_tool) into a single unified load_skill_through_path tool. The refactoring extracts tool creation logic into a dedicated SkillToolFactory class and restructures the skill system prompt with XML tags for improved LLM parsing.
Changes:
- Consolidated three skill access tools into one unified tool with automatic resource listing on failure
- Extracted tool creation logic from
SkillBoxto newSkillToolFactoryclass - Restructured skill prompts using XML tags (
<skill>,<name>,<description>,<skill-id>) - Added
getAllSkillIds()method toSkillBoxAPI
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
SkillToolFactory.java |
New factory class for creating the consolidated skill access tool |
SkillBox.java |
Removed three @Tool methods and added registerSkillLoadTool() and getAllSkillIds() |
AgentSkillPromptProvider.java |
Restructured prompt template with XML tags for better LLM parsing |
ReActAgent.java |
Updated integration to call registerSkillLoadTool() instead of registerTool() |
SkillBoxToolsTest.java |
Updated tests to validate the new consolidated tool behavior |
SkillBoxTest.java |
Added tests for getAllSkillIds() method |
AgentSkillPromptProviderTest.java |
Updated tests to validate new XML-based prompt structure |
docs/en/task/agent-skill.md |
Updated documentation to reference new tool name and API |
docs/zh/task/agent-skill.md |
Updated Chinese documentation to match English version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| toolkit.createToolGroup( | ||
| "skill-build-in-tools", | ||
| "skill build-in tools, could contain(load_skill_through_path)"); |
There was a problem hiding this comment.
Spelling error: "build-in" should be "built-in". The correct term is "built-in tools" not "build-in tools".
| return Mono.just(ToolResultBlock.error(e.getMessage())); | ||
| } | ||
| } | ||
| // ==================== Skill Build-In Tools ==================== |
There was a problem hiding this comment.
Spelling error: "Build-In" should be "Built-In". The correct term is "Built-In Tools" not "Build-In Tools".
| skillId)); | ||
| } | ||
| return skill; | ||
| logger.info("Registered skill load tools to toolkit"); |
There was a problem hiding this comment.
Spelling error: "skill load tools" should be "skill load tool" (singular). The method registers one tool (load_skill_through_path), not multiple tools.
| + " usage instructions)\n" | ||
| + "- 'SKILL.md': The skill's markdown documentation (name, description," |
There was a problem hiding this comment.
The description text has lines 63-64 in the wrong order. Line 63 should come after line 64 to maintain proper sentence structure. The current text reads "2. Returns the requested resource content\n usage instructions)\n- 'SKILL.md': The skill's markdown documentation (name, description," which is incomplete and confusing.
| + " usage instructions)\n" | |
| + "- 'SKILL.md': The skill's markdown documentation (name, description," | |
| + "- 'SKILL.md': The skill's markdown documentation (name, description," | |
| + " usage instructions)\n" |
| + " skill (e.g., 'SKILL.md," | ||
| + " references/references.md')")), |
There was a problem hiding this comment.
The description string for the 'path' parameter is incomplete - it ends with an opening single quote and comma without closing. The string reads "'SKILL.md," but should likely be "'SKILL.md', 'references/references.md')". This will result in a malformed parameter description for API consumers.
| + " skill (e.g., 'SKILL.md," | |
| + " references/references.md')")), | |
| + " skill (e.g., 'SKILL.md'," | |
| + " 'references/references.md')")), |
| "type", | ||
| "string", | ||
| "description", | ||
| "The unique identifier of the" + " skill.", |
There was a problem hiding this comment.
The string concatenation on line 84 has an unnecessary split with "The unique identifier of the" + " skill." This should be a single string "The unique identifier of the skill." for better readability and consistency with the rest of the code.
| "The unique identifier of the" + " skill.", | |
| "The unique identifier of the skill.", |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
AgentScope-Java Version
1.0.7-SNAPSHOT
Description
Summary
This PR refactors the skill access mechanism by consolidating three separate tools into a single unified tool, significantly reducing the tool call chain in worst-case scenarios.
Changes
Tool Consolidation
skill_md_load_tool,skill_resources_load_tool, andget_all_resources_path_toolinto a singleload_skill_through_pathtoolSkillBoxinto newSkillToolFactoryclass for better separation of concernsPrompt Structure Improvement
<skill>,<name>,<description>,<skill-id>) for improved LLM parsing accuracySkillBox API
Key Benefit: Reduced Tool Call Chain
Best-case improvement:
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)