Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
|
已补充文档 PR：文档 PR 链接为docs: update skill list front matter response by jay666mnj · Pull Request #1130 · nacos-group/nacos-group.github.io
原始邮件
发件人：github-actions[bot] ***@***.***>
发件时间：2026年7月14日 23:05
收件人：alibaba/nacos ***@***.***>
抄送：yijie zhao ***@***.***>, Author ***@***.***>
主题：Re: [alibaba/nacos] [ISSUE #15345] Return skill front matter in list response (PR #15508)
github-actions[bot] left a comment (alibaba/nacos#15508)
Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io
感谢您提交的PR。 🙏
请再次查看您的PR内容，确认是否包含任何使用方式/API/配置参数的变更，如：新增API、新增配置参数、修改默认配置等操作。
如果是，请确保在提交之前，在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档（markdown格式）。
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.
|
KomachiSion
left a comment
KomachiSion
left a comment
There was a problem hiding this comment.
I don't think current implementation is good design.
If we query for a big page, the original list query only query the resource table and handle it.
But after the change, each skill will query two more: one version and query all skill data to get the frontmatter. which cost so many and get a small frontmatter.
I suggest to redesign for this before you do coding.
be14f5a to
94ae27a
Compare
|
Thanks for pointing this out. I have redesigned the implementation and updated the PR.
The list query no longer performs extra version or storage queries for each skill. Instead, the SKILL.md front matter is parsed and cached into ai_resource.ext when a skill draft is created or updated.
The list API now reads frontMatter directly from meta.ext, so it avoids the N+1 query issue in the previous implementation.
I have also synced with the latest develop branch and resolved the conflicts.
Verified with:
./mvnw.cmd --% -pl api,ai -am -Dtest=SkillOperationServiceImplTest#testListSkillsSuccessfully -Dsurefire.failIfNoSpecifiedTests=false -DskipITs test
Result: Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
原始邮件
发件人：杨翊 SionYang ***@***.***>
发件时间：2026年7月15日 10:04
收件人：alibaba/nacos ***@***.***>
抄送：yijie zhao ***@***.***>, Author ***@***.***>
主题：Re: [alibaba/nacos] [ISSUE #15345] Return skill front matter in list response (PR #15508)
@KomachiSion requested changes on this pull request.
I don't think current implementation is good design.
If we query for a big page, the original list query only query the resource table and handle it.
But after the change, each skill will query two more: one version and query all skill data to get the frontmatter. which cost so many and get a small frontmatter.
I suggest to redesign for this before you do coding.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.
|
|
Thanks for redesigning the implementation. Reading front matter directly from However, the cached data lifecycle is not complete yet:
I suggest storing the corresponding Please also add lifecycle and large-page tests that verify:
Additionally, the current PR fails |
|
Thanks for the detailed review. I have updated the implementation according to your suggestions. The list API still reads frontMatter only from ai_resource.ext and does not perform per-item version-table or storage queries. I added frontMatterVersion into ext and refreshed the cached display metadata during lifecycle transitions, including publish, force publish, draft deletion, redraft, and bootstrap. Draft create/update with an existing latest version no longer exposes unpublished front matter. For legacy skills without cached frontMatter, the list API may return frontMatter = null until a later lifecycle transition or bootstrap repair, and it will not do lazy per-item backfill in the list path. I also changed bumpMetaDescriptionAndExt to fail explicitly when CAS retries are exhausted, added lifecycle/large-page/legacy/bootstrap tests, updated the skill spec and Admin/Console OpenAPI IT scenarios, and fixed the SkillSummary spotless issue. |
|
Thanks for the update. The list hot path now avoids per-item version/storage queries, and the display-version lifecycle is improved. However, there are still several blockers:
Please restart the whole display-version calculation on CAS conflict, or update the lifecycle state and corresponding cache atomically. The list path can also compare
I also ran the complete related unit tests:
Please also update the Admin/Console API scenario matrices and coverage registry for the changed response contract, and clean up the unrelated whitespace churn and corrupted So I prefer to discuss full solution before you do PR. We can discuss in issue first. Can you submit one design plan spec into issue first, we disscuss it first. |
|
This PR has conflicts with the git fetch origin
git checkout fix-skill-list-frontmatter
git rebase origin/develop
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
|
Status check on this PR:
Automated notification by github-manager-bot |
nacos-community
left a comment
nacos-community
left a comment
There was a problem hiding this comment.
Summary
Adds frontMatter to the skill list response by parsing SKILL.md front matter at write time (bootstrap / draft create-update / publish / redraft / delete-draft) and caching it into ai_resource.ext, so the list path performs no per-row version or storage queries. This addresses the N+1 concern raised in the earlier review round — the new unit tests explicitly verify findVersion / storage.get are never called during list pagination. Specs (specs/en|zh-cn/ai/skill-spec.md) and openapi-test cases are updated accordingly, and SkillSummary (api module) remains Java 8 compatible. Overall the redesigned implementation looks solid; a few non-blocking suggestions below.
Findings
- [Warning]
ai/src/main/java/com/alibaba/nacos/ai/service/skills/SkillOperationServiceImpl.java:1911— no size cap on cached front matter; long YAML scalars would bloatextand every list response row - [Warning]
ai/src/main/java/com/alibaba/nacos/ai/service/skills/SkillOperationServiceImpl.java:1888—refreshDisplayMetadataCachefailures after an already-persisted lifecycle transition can surface as 5xx and mask a successful publish - [Info]
test/openapi-test/src/test/java/com/alibaba/nacos/test/adminapi/ai/skill/SkillAdminApiOpenApiITCase.java:246—frontMatterassertion can NPE instead of failing with a readable message - [Info]
test/openapi-test/src/test/java/com/alibaba/nacos/test/consoleapi/ai/skill/SkillConsoleApiOpenApiITCase.java:246— same NPE risk as the admin-API counterpart - [Info]
ai/src/main/java/com/alibaba/nacos/ai/service/skills/SkillOperationServiceImpl.java:1925—parseFrontMatterFromExtsilently stringifies non-string scalars exposed via publicMap<String, String> - [Info]
ai/src/main/java/com/alibaba/nacos/ai/service/resource/AiResourceManager.java:309— mojibake (鈫?) corrupted the→arrows in Javadoc (also near line 1254)
Suggestions
- Cap the cached front matter (e.g. ~500 chars per scalar value and/or ~4 KB total serialized), or keep only an allow-list of display-relevant keys (
name,description,alias,version) and drop the rest. - Wrap
refreshDisplayMetadataCachein try/catch + WARN log so a cache-refresh failure does not mask an already-successful lifecycle operation; the cache self-repairs on the next lifecycle transition, which is the contract already documented for legacy rows. - In the openapi ITs, add
assertNotNull(found.get("frontMatter"), found.toString())before dereferencing, matching the existinglabelspattern one line above. - Restore the corrupted
→characters inAiResourceManager.javaJavadoc (encoding artifact unrelated to this feature) before merge.
Automated review by github-manager-bot
|
|
||
| private static Map<String, String> parseFrontMatterFromExt(String extJson) { | ||
| Map<String, Object> ext = parseExt(extJson); | ||
| Object frontMatter = ext.get(EXT_FRONT_MATTER_KEY); |
There was a problem hiding this comment.
buildSkillMetaExt stores the entire parsed front matter map into ai_resource.ext with no per-value length cap and no total-size cap. Since ext is echoed verbatim in every list response row and is also read back by every lifecycle transition, a SKILL.md whose YAML scalars are very long (e.g. an inline multi-KB description, a giant metadata.* block, or an accidentally embedded blob) would bloat every paginated list row and the meta row itself. The upload zip is capped at ~50 MB uncompressed, but that is far too generous for metadata echoed in list responses. Suggest: reject or truncate individual front-matter scalar values above a small limit (e.g. 500 chars) and/or cap the serialised frontMatter payload (e.g. 4 KB); alternatively, store only an allow-list of display-relevant keys (name, description, alias, version) and drop the rest.
| if (meta == null) { | ||
| return false; | ||
| } | ||
| ResourceVersionInfo info = AiResourceManager.parseVersionInfo(meta.getVersionInfo()); |
There was a problem hiding this comment.
Every call site of refreshDisplayMetadataCache in this diff runs the refresh AFTER the primary lifecycle transition (doPublish, doForcePublish, doRedraft, doDeleteDraft, publishApprovedBySystem, overwriteEditingDraft, bootstrapSkillFromZip's early-return branch) has already mutated persisted state. Inside the refresh, loadSkillFromStorage reads storage bytes for the display version; if that read throws (storage plugin error, transient IO, concurrent repair job, or a corrupt storage row), the exception currently propagates out of the lifecycle method. Concretely in publish(...) the publish itself has already succeeded — the version row is online and the manifest may not have been updated yet — but the caller receives a 5xx. This masks a successful state change as a failure and leaves the ext cache stale. Suggest: wrap the refresh in a try/catch that logs at WARN and lets the primary operation's success propagate; the cache will be repaired on the next lifecycle transition (or by a future bootstrap repair), which is the same contract already documented in the spec for legacy rows.
| assertFalse(found.isMissingNode(), page.toString()); | ||
| assertEquals(skillName, found.get("name").asText(), found.toString()); | ||
| assertNotNull(found.get("labels"), found.toString()); | ||
| assertEquals(skillName, found.get("frontMatter").get("name").asText(), found.toString()); |
There was a problem hiding this comment.
found.get("frontMatter").get("name").asText() does not null-check frontMatter. Per the updated skill-spec, list responses MAY include frontMatter; legacy rows (or rows whose front matter failed to parse) legitimately return frontMatter = null. If a regression ever causes the cache to be missing for the test skill, this assertion NPEs with a generic stack trace instead of a clear assertion message, hiding the root cause. Suggest: assertNotNull(found.get("frontMatter"), found.toString()); before the assertEquals, matching the existing pattern used for labels one line above.
| assertFalse(found.isMissingNode(), page.toString()); | ||
| assertEquals(skillName, found.get("name").asText(), found.toString()); | ||
| assertNotNull(found.get("labels"), found.toString()); | ||
| assertEquals(skillName, found.get("frontMatter").get("name").asText(), found.toString()); |
There was a problem hiding this comment.
Same issue as the admin-API counterpart: found.get("frontMatter").get("name").asText() will NPE if the cache is missing instead of producing a readable assertion failure. Add an explicit assertNotNull on frontMatter to keep the test robust against legitimate frontMatter = null rows.
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static Map<String, Object> parseExt(String extJson) { |
There was a problem hiding this comment.
parseFrontMatterFromExt silently coerces every cached value to String.valueOf(value), including non-string YAML scalars (numbers, booleans) and — more importantly — values that were already non-string in the stored ext JSON (Jackson deserialises unquoted numbers as Integer/Long, true/false as Boolean). The display layer will see "true" or "42" rather than typed values, and any future caller that relies on typed front-matter values will get surprises. Since SkillSummary.frontMatter is declared Map<String, String> in the public api module, please either (a) document this stringification in the Javadoc on SkillSummary.getFrontMatter() so downstream consumers know values are always stringified, or (b) change the API model to Map<String, Object> before it ships (breaking change later).
| }); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The Javadoc comment on doCasLoop was corrupted by this diff: the arrow (newValue, latestMeta) → refresh non-target fields became (newValue, latestMeta) 鈫?refresh non-target fields (mojibake). The same corruption appears at line 1254. This is unrelated to the feature and appears to be an encoding artefact from the diff/edit tooling, but it will ship in source and rendered Javadoc. Please restore the original → character (or rewrite as ->) before merge.
|
@jay666mnj @KomachiSion The current PR already reads
Frontmatter search will remain outside this change. I plan to submit a follow-up PR from a fresh branch based on current Does this scope match your intended simplification? 😄 |
4 participants
What is the purpose of the change
Fixes #15345
Support returning front matter parsed from SKILL.md in the skill list response, so the console can display metadata such as alias and other custom fields.
Brief changelog
frontMattertoSkillSummary.Verifying this change
git diff --checkmvn -pl ai -am "-Dtest=SkillOperationServiceImplTest#testListSkillsSuccessfully" "-Dsurefire.failIfNoSpecifiedTests=false" testResult:
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0.