feat(skill): add JarSkillRepositoryAdapter for loading skills from Jar#546
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds JarSkillRepositoryAdapter, a new utility class that enables loading skills from JAR files in production environments. The adapter creates a virtual filesystem for JAR resources and delegates to the existing FileSystemSkillRepository for skill loading, providing seamless environment detection and adaptation.
Changes:
- Introduces
JarSkillRepositoryAdapterclass that bridges JAR resources with filesystem-based skill loading - Adds comprehensive test suite with both filesystem and JAR environment test coverage
- Includes test resource files (SKILL.md files) for testing skill loading functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/util/JarSkillRepositoryAdapter.java | New adapter class implementing AutoCloseable for loading skills from JAR files with automatic environment detection |
| agentscope-core/src/test/java/io/agentscope/core/skill/util/JarSkillRepositoryAdapterTest.java | Comprehensive test suite covering filesystem loading, JAR loading, error handling, and AutoCloseable behavior |
| agentscope-core/src/test/resources/test-skills/writing-skill/SKILL.md | Test resource file defining a writing skill with metadata |
| agentscope-core/src/test/resources/test-skills/writing-skill/references/guide.md | Test resource file with nested skill reference documentation |
| agentscope-core/src/test/resources/test-skills/calculation-skill/SKILL.md | Test resource file defining a calculation skill with metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Use FileSystemSkillRepository for actual skill loading | ||
| this.repository = new FileSystemSkillRepository(skillBasePath.getParent(), false); |
There was a problem hiding this comment.
There's a potential NullPointerException risk if skillBasePath.getParent() returns null. This could happen if resourcePath points to a root-level resource (e.g., just "skill-name" without a parent directory). The FileSystemSkillRepository constructor would receive null and throw a NullPointerException or IllegalArgumentException. Consider adding validation to ensure the resourcePath has a parent directory, or provide a more descriptive error message for this case.
| this.repository = new FileSystemSkillRepository(skillBasePath.getParent(), false); | |
| Path skillDirectory = skillBasePath.getParent(); | |
| if (skillDirectory == null) { | |
| throw new IOException( | |
| "Resource path must have a parent directory: " + resourcePath); | |
| } | |
| this.repository = new FileSystemSkillRepository(skillDirectory, false); |
| } | ||
|
|
||
| /** | ||
| * gets a skill by name. |
There was a problem hiding this comment.
The first word of the Javadoc summary should be capitalized. Change "gets" to "Gets" to maintain consistency with other method documentation in this class (lines 116, 125, 134).
| * gets a skill by name. | |
| * Gets a skill by name. |
| * | ||
| * @param skillName The skill name (directory name) | ||
| * @return The loaded AgentSkill object | ||
| * @throws IOException if loading fails |
There was a problem hiding this comment.
The @throws IOException Javadoc tag is documented but the method signature doesn't declare this exception. The getSkill method delegates to repository.getSkill(skillName) which doesn't throw IOException. Either remove the @throws tag or add throws IOException to the method signature if exceptions should be propagated.
| * @throws IOException if loading fails |
| } | ||
|
|
||
| // Use FileSystemSkillRepository for actual skill loading | ||
| this.repository = new FileSystemSkillRepository(skillBasePath.getParent(), false); |
There was a problem hiding this comment.
Potential resource leak: If FileSystemSkillRepository constructor throws an IllegalArgumentException (lines 82-98 in FileSystemSkillRepository.java), the fileSystem created at line 87 will not be closed. Consider wrapping the FileSystemSkillRepository construction in a try-catch block that closes the fileSystem on exception, or restructuring to ensure proper cleanup.
| this.repository = new FileSystemSkillRepository(skillBasePath.getParent(), false); | |
| try { | |
| this.repository = new FileSystemSkillRepository(skillBasePath.getParent(), false); | |
| } catch (RuntimeException e) { | |
| if (this.fileSystem != null) { | |
| try { | |
| this.fileSystem.close(); | |
| } catch (IOException closeException) { | |
| e.addSuppressed(closeException); | |
| } | |
| } | |
| throw e; | |
| } |
| * <p>Usage example: | ||
| * <pre>{@code | ||
| * try (JarSkillRepositoryAdapter adapter = | ||
| * new JarSkillRepositoryAdapter("writing-skills")) { | ||
| * AgentSkill skill = adapter.getSkill("writing-skills"); | ||
| * } | ||
| * }</pre> |
There was a problem hiding this comment.
The usage example is misleading. If the adapter is created with resource path "writing-skills", the skill name passed to getSkill() should be "writing-skill" (a subdirectory inside writing-skills directory), not "writing-skills" again. Based on the implementation (line 97), the repository is created with skillBasePath.getParent(), so skills are loaded from the parent directory of the resource path. Consider updating the example to clarify this behavior, such as: new JarSkillRepositoryAdapter("skills/writing-skill") followed by adapter.getSkill("writing-skill") or adapter.getAllSkills().
| /** | ||
| * Creates an adapter for loading skills from resources. | ||
| * | ||
| * @param resourcePath The path to the skill under resources, e.g., "writing-skills" |
There was a problem hiding this comment.
The parameter documentation is misleading. The description states "The path to the skill under resources" but the actual behavior (line 97) creates a repository from skillBasePath.getParent(). This means resourcePath should point to a specific skill directory, and the adapter will load all skills from its parent directory. Consider clarifying this as: "The path to a skill directory under resources (e.g., 'skills/writing-skill'). The adapter will load all skills from the parent directory."
| /** | ||
| * Creates an adapter for loading skills from resources using a specific ClassLoader. | ||
| * | ||
| * @param resourcePath The path to the skill under resources, e.g., "writing-skills" |
There was a problem hiding this comment.
The parameter documentation is misleading. The description states "The path to the skill under resources" but the actual behavior (line 97) creates a repository from skillBasePath.getParent(). This means resourcePath should point to a specific skill directory, and the adapter will load all skills from its parent directory. Consider clarifying this as: "The path to a skill directory under resources (e.g., 'skills/writing-skill'). The adapter will load all skills from the parent directory."
| public void close() throws IOException { | ||
| if (fileSystem != null) { | ||
| fileSystem.close(); | ||
| } |
There was a problem hiding this comment.
The close() method may not be idempotent as expected by the test at line 273-285. Calling FileSystem.close() multiple times can throw ClosedFileSystemException. Consider adding a flag to track if the filesystem has already been closed, or catch and ignore ClosedFileSystemException to ensure the method can be safely called multiple times as per AutoCloseable best practices.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…nment and when using jar packages
fang-tech
deleted the
fang-tech/feat-fileSystemSkillRepo-support-jar-path
branch
2 participants
AgentScope-Java Version
1.0.7-SNAPSHOT
Description
Add
JarSkillRepositoryAdapterto enable loading skills from JAR files in production environments.What's Changed
JarSkillRepositoryAdapterclass that bridges JAR resources andFileSystemSkillRepositoryAutoCloseablefor proper resource managementclose #533
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)