fix(skill): remove FileSystemSkillRepository empty directory validation #397
Conversation
fang-tech
changed the title
There was a problem hiding this comment.
Pull request overview
This PR removes the validation that required FileSystemSkillRepository base directories to contain at least one subdirectory, enabling users to initialize repositories from empty folders and subsequently save skills to them.
Key Changes:
- Removed the empty directory validation check from the FileSystemSkillRepository constructor
- Updated the corresponding test to verify that empty directories are now accepted
- Added a test for relative-to-absolute path transformation in the constructor
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/repository/FileSystemSkillRepository.java | Removed the validation logic that threw IllegalArgumentException when base directory contained no subdirectories, enabling empty directory initialization |
| agentscope-core/src/test/java/io/agentscope/core/skill/repository/FileSystemSkillRepositoryTest.java | Updated test expectations to assert successful construction from empty directories and added test for path transformation behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| @DisplayName("Should transform relative path to absolute in constructor") | ||
| void testConstructor_RelativePath() throws IOException { | ||
| Path relativePath = Path.of("relative-skills"); | ||
| Files.createDirectories(relativePath); | ||
|
|
||
| try { | ||
| FileSystemSkillRepository fileSystemSkillRepository = | ||
| new FileSystemSkillRepository(relativePath); | ||
| assertNotNull(fileSystemSkillRepository); | ||
| assertEquals( | ||
| relativePath.toAbsolutePath().normalize().toString(), | ||
| fileSystemSkillRepository.getRepositoryInfo().getLocation()); | ||
| } finally { | ||
| if (Files.exists(relativePath)) { | ||
| Files.delete(relativePath); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This test case appears to be out of scope for this PR. The PR description states the purpose is to "Allow users to create a FileSystemSkillRepository based on an empty folder, and complete the usage process of 'empty directory -> save skills'". However, this test validates relative-to-absolute path transformation, which is existing functionality (already implemented at line 87 of FileSystemSkillRepository.java) and unrelated to the empty directory validation removal. Consider moving this test to a separate PR focused on path handling, or remove it from this PR to maintain focus on the stated objective.
| @Test | |
| @DisplayName("Should transform relative path to absolute in constructor") | |
| void testConstructor_RelativePath() throws IOException { | |
| Path relativePath = Path.of("relative-skills"); | |
| Files.createDirectories(relativePath); | |
| try { | |
| FileSystemSkillRepository fileSystemSkillRepository = | |
| new FileSystemSkillRepository(relativePath); | |
| assertNotNull(fileSystemSkillRepository); | |
| assertEquals( | |
| relativePath.toAbsolutePath().normalize().toString(), | |
| fileSystemSkillRepository.getRepositoryInfo().getLocation()); | |
| } finally { | |
| if (Files.exists(relativePath)) { | |
| Files.delete(relativePath); | |
| } | |
| } | |
| } |
| @Test | ||
| @DisplayName("Should throw exception when base directory is empty") | ||
| @DisplayName("Should not throw exception when base directory is empty") | ||
| void testConstructor_EmptyBaseDir() throws IOException { | ||
| Path emptyDir = tempDir.resolve("empty"); | ||
| Files.createDirectories(emptyDir); | ||
| assertThrows(IllegalArgumentException.class, () -> new FileSystemSkillRepository(emptyDir)); | ||
| FileSystemSkillRepository fileSystemSkillRepository = | ||
| new FileSystemSkillRepository(emptyDir); | ||
| assertNotNull(fileSystemSkillRepository); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the core use case described in the PR: "empty directory -> save skills". While this test verifies that an empty directory can be used to construct a FileSystemSkillRepository, it doesn't validate that skills can actually be saved to it. Consider adding a test that creates a repository from an empty directory and then successfully saves a skill to it, verifying the complete workflow mentioned in the PR description.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
AgentScope-Java Version
1.0.5-SNAPSHOT
Description
Allow users to create a FileSystemSkillRepository based on an empty folder, and complete the usage process of "empty directory -> save skills".
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)