refractor skill#907
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the skill system by replacing the legacy AutoSkills implementation with a new SkillCatalog and SkillToolSet architecture. This change integrates skills as standard tools and injects them into the system prompt, removing the need for special routing logic. While the architectural shift is sound, I have identified several areas for improvement: the blocking I/O operations in prepare_skills should be offloaded to a thread, the direct manipulation of ToolManager internals should be replaced with a proper API, and the resource management for temporary directories and metadata parsing needs optimization to prevent leaks and performance bottlenecks.
| Format skill execution result as messages for agent history. | ||
| skills_config = self.config.skills | ||
| self._skill_catalog = SkillCatalog(config=skills_config) | ||
| self._skill_catalog.load_from_config(skills_config) |
There was a problem hiding this comment.
The load_from_config method performs blocking I/O operations, including filesystem scans, snapshot_download from ModelScope, and git clone. Since prepare_skills is an async method called during the agent's interaction loop, these blocking calls will stall the event loop. It is recommended to run this operation in a separate thread using asyncio.to_thread.
| self._skill_catalog.load_from_config(skills_config) | |
| await asyncio.to_thread(self._skill_catalog.load_from_config, skills_config) |
| self.tool_manager._tool_index[key] = ( | ||
| skill_toolset, server_name, tool) |
There was a problem hiding this comment.
Directly accessing and modifying the private _tool_index attribute of ToolManager is fragile and breaks encapsulation. This logic relies on the internal implementation details of ToolManager, which may change in future versions. It is recommended to use a public registration API if available, or refactor ToolManager to support this namespacing logic natively.
| return self._loader.load_skills(local_path) | ||
|
|
||
| def _load_from_git(self, source: SkillSource) -> Dict[str, SkillSchema]: | ||
| dest = Path(tempfile.mkdtemp(prefix="ms_agent_skill_")) |
There was a problem hiding this comment.
The temporary directory created via tempfile.mkdtemp for git-based skill sources is never cleaned up. This will lead to a leak of temporary directories on the host machine, especially if the catalog is reloaded multiple times. Consider using tempfile.TemporaryDirectory and managing its lifecycle within the SkillCatalog class to ensure proper cleanup during reload() or when the catalog is destroyed.
| frontmatter = SkillSchemaParser.parse_yaml_frontmatter( | ||
| skill.content) |
There was a problem hiding this comment.
The get_always_skills method re-parses the YAML frontmatter of every enabled skill on every call. Since this is used during prompt injection for every user message, it can become a performance bottleneck as the number of skills grows. It is recommended to parse this metadata once during skill loading and store it as an attribute, or cache the result of this method.
2 participants
Change Summary
Refractor skills.
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.