refactor(skill): refactor SkillBox with Skill util class#724
refactor(skill): refactor SkillBox with Skill util class#724fang-tech wants to merge 1 commit intoagentscope-ai:mainfrom
Conversation
… tests for RepoClasses
Summary of ChangesHello @fang-tech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 SkillBox to delegate file system operations to a new SkillFileSystemHelper utility class, which improves code organization and reusability. The changes also standardize the source string format in FileSystemSkillRepository for better consistency. While the refactoring is a good step, it has introduced a critical compilation error in SkillBox by referencing a variable that was removed. I've added a comment with a suggested fix for this issue. The test updates are comprehensive and correctly validate the new behavior.
There was a problem hiding this comment.
Pull request overview
Refactors skill repository “source” formatting and consolidates temp-directory cleanup logic into SkillFileSystemHelper, updating unit tests accordingly.
Changes:
- Update
FileSystemSkillRepository#getSource()to usefilesystem:and an underscore-based suffix format. - Refactor
SkillBoxtemp work directory cleanup to useSkillFileSystemHelper.registerTempDirectoryCleanup(...)and simplify directory initialization logic. - Add/adjust unit tests for
getSource()behavior in filesystem and classpath repositories.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/repository/FileSystemSkillRepository.java | Changes default source format from filesystem_... to filesystem:... and updates suffix delimiter. |
| agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java | Replaces inline shutdown-hook cleanup with SkillFileSystemHelper and refactors work/upload dir creation. |
| agentscope-core/src/test/java/io/agentscope/core/skill/repository/FileSystemSkillRepositoryTest.java | Updates and expands getSource()/suffix unit tests to match new filesystem source format. |
| agentscope-core/src/test/java/io/agentscope/core/skill/repository/ClasspathSkillRepositoryTest.java | Adds getSource() unit tests for default/custom source and trailing-slash handling. |
Comments suppressed due to low confidence (1)
agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java:690
- In the non-temp branch,
Files.exists(workDir),createDirectories(workDir), and the log statement still referenceworkDir, but there is no local variable defined anymore. This is a compile error; switch these references tothis.workDir(or define a local variable before theif).
// Create directory if it doesn't exist
if (!Files.exists(workDir)) {
try {
Files.createDirectories(workDir);
logger.info("Created working directory: {}", workDir);
| this.workDir = Files.createTempDirectory("agentscope-code-execution-"); | ||
|
|
||
| SkillFileSystemHelper.registerTempDirectoryCleanup(workDir); | ||
|
|
||
| logger.info("Created temporary working directory: {}", workDir); |
There was a problem hiding this comment.
workDir is referenced but no longer declared in this method (e.g., in registerTempDirectoryCleanup(workDir) and the log statement). This will not compile; use this.workDir (or introduce a local Path workDir = this.workDir) consistently after creating the temp directory.
| assertEquals("filesystem_" + expectedSuffix, source); | ||
| @DisplayName("Should return default source with format: filesystem:parent_child") | ||
| void testGetSource_DefaultSource() { | ||
| assertTrue(repository.getSource().matches("filesystem:[^_]+_skills")); |
There was a problem hiding this comment.
The regex filesystem:[^_]+_skills assumes the parent directory name contains no underscores, but buildDefaultSourceSuffix() does not enforce that. This can make the test flaky depending on the temp directory name. Consider asserting against the exact expected suffix derived from skillsBaseDir.getParent().getFileName() and skillsBaseDir.getFileName() (or relax the regex to avoid assumptions about underscores).
| assertTrue(repository.getSource().matches("filesystem:[^_]+_skills")); | |
| String parentName = skillsBaseDir.getParent().getFileName().toString(); | |
| String childName = skillsBaseDir.getFileName().toString(); | |
| String expectedSource = "filesystem:" + parentName + "_" + childName; | |
| assertEquals(expectedSource, repository.getSource()); |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.8, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)