Conversation
|
|
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
agentscope-extensions/agentscope-extensions-nacos/agentscope-extensions-nacos-skill/pom.xml
Outdated
Show resolved
Hide resolved
sign CLA pls. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Nacos-backed AgentSkillRepository implementation so AgentScope can read Skills from Nacos (read-only), and wires the new module into the build/distribution BOMs.
Changes:
- Add new
agentscope-extensions-nacos-skillmodule implementingNacosSkillRepositoryand aSkill→AgentSkillconverter. - Register the new module in the Nacos extensions parent POM and include it in the BOM and
agentscope-alldistribution. - Update the managed
nacos-clientversion in the dependencies BOM.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| agentscope-extensions/agentscope-extensions-nacos/pom.xml | Adds the new agentscope-extensions-nacos-skill module to the reactor build. |
| agentscope-extensions/agentscope-extensions-nacos/agentscope-extensions-nacos-skill/src/main/java/io/agentscope/core/nacos/skill/NacosSkillToAgentSkillConverter.java | Introduces conversion logic from Nacos AI Skill model to AgentSkill. |
| agentscope-extensions/agentscope-extensions-nacos/agentscope-extensions-nacos-skill/src/main/java/io/agentscope/core/nacos/skill/NacosSkillRepository.java | Adds the Nacos-based repository implementation for loading skills. |
| agentscope-extensions/agentscope-extensions-nacos/agentscope-extensions-nacos-skill/pom.xml | Declares the new skill module artifact and dependencies. |
| agentscope-distribution/agentscope-bom/pom.xml | Adds the new artifact to the AgentScope BOM. |
| agentscope-distribution/agentscope-all/pom.xml | Includes the new module as an optional dependency in the “all” distribution. |
| agentscope-dependencies-bom/pom.xml | Bumps the managed nacos-client version used across the repo. |
| @Override | ||
| public void setWriteable(boolean writeable) { | ||
| this.writeable = writeable; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isWriteable() { | ||
| return writeable; | ||
| } |
There was a problem hiding this comment.
setWriteable() and isWriteable() expose a mutable writeable flag, but this repository still throws UnsupportedOperationException for save/delete and listing methods. This can mislead callers into attempting writes based on getRepositoryInfo().isWritable()/isWriteable(). Consider making the repository explicitly read-only (ignore setWriteable, always return false for isWriteable, and ensure repository info matches), or implement the corresponding write/list operations when writeable is enabled.
| @Override | ||
| public List<String> getAllSkillNames() { | ||
| throw new UnsupportedOperationException( | ||
| "getAllSkillNames is not implemented for NacosSkillRepository"); | ||
| } | ||
|
|
||
| @Override | ||
| public List<AgentSkill> getAllSkills() { | ||
| throw new UnsupportedOperationException( | ||
| "getAllSkills is not implemented for NacosSkillRepository"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean save(List<AgentSkill> skills, boolean force) { | ||
| throw new UnsupportedOperationException("save is not implemented for NacosSkillRepository"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean delete(String skillName) { | ||
| throw new UnsupportedOperationException( | ||
| "delete is not implemented for NacosSkillRepository"); | ||
| } |
There was a problem hiding this comment.
For read-only repositories in this codebase, write/list operations are typically implemented as no-ops that log a warning and return false/empty results (e.g., GitSkillRepository and ClasspathSkillRepository), rather than throwing UnsupportedOperationException. Throwing here can cause unexpected crashes for callers that iterate repositories uniformly. Consider returning Collections.emptyList() for list methods and false for save/delete, with a warn log indicating the repository is read-only.
| @Override | ||
| public AgentSkill getSkill(String name) { | ||
| if (name == null || name.isBlank()) { | ||
| throw new IllegalArgumentException("Skill name cannot be null or empty"); | ||
| } | ||
| try { | ||
| Skill nacosSkill = aiService.loadSkill(name.trim()); | ||
| if (nacosSkill == null) { | ||
| throw new IllegalArgumentException("Skill not found: " + name); | ||
| } | ||
| return NacosSkillToAgentSkillConverter.toAgentSkill(nacosSkill, getSource()); | ||
| } catch (NacosException e) { | ||
| if (e.getErrCode() == NacosException.NOT_FOUND) { | ||
| throw new IllegalArgumentException("Skill not found: " + name, e); | ||
| } | ||
| throw new RuntimeException("Failed to load skill from Nacos: " + name, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This new repository/converter module doesn’t appear to include tests. The Nacos extensions already have unit tests (e.g., nacos-a2a and nacos-prompt), so adding tests here would help prevent regressions around getSkill() error handling and the Nacos→AgentSkill conversion (defaults for blank fields, resource mapping).
| <artifactId>agentscope-extensions-nacos-skill</artifactId> | ||
| <name>AgentScope Java - Nacos A2A skill</name> | ||
| <description>agentscope-extensions-nacos-skill</description> |
There was a problem hiding this comment.
The Maven <name> looks like a copy/paste from the A2A module ("Nacos A2A skill"). Since this artifact is a skill repository integration, consider renaming it to something like "AgentScope Java - Nacos Skill Repository" to avoid confusion in published artifacts.
| <quartz.version>2.5.2</quartz.version> | ||
| <spring.version>7.0.3</spring.version> | ||
| <spring-boot.version>4.0.3</spring-boot.version> | ||
| <nacos-client.version>3.1.1</nacos-client.version> | ||
| <nacos-client.version>3.2.0-BETA</nacos-client.version> | ||
| <json-schema-validator.version>3.0.0</json-schema-validator.version> |
There was a problem hiding this comment.
Updating the BOM-managed nacos-client.version to a beta release will affect all modules consuming com.alibaba.nacos:nacos-client (e.g., existing Nacos A2A / prompt integrations). Please confirm compatibility across those modules, or consider scoping the newer version to just the new skill module if only it requires newer APIs.
提供 NacosSkillRepository，实现 AgentSkillRepository，从 Nacos 读取 Skill（只读）