feat(kit): add skill storage providers#367
Conversation
|
✅ Preview build completed successfully!
|
📦 Package Previewpnpm add https://pkg.pr.new/@opentiny/tiny-robot@f385dfe pnpm add https://pkg.pr.new/@opentiny/tiny-robot-kit@f385dfe pnpm add https://pkg.pr.new/@opentiny/tiny-robot-svgs@f385dfe commit: f385dfe |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a complete ChangesSkill Storage Subsystem
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…kill-storage-foundation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kit/src/skills/storage/indexedDB.ts (1)
161-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
import()resolves with the raw loaded skill, not the stored one.Unlike
FsSkillStorage.import()(which re-reads viaadd()and returns the persisted skill with lazy readers), here the resolvedresult.skillis the loader's skill whileadd()'s return value is discarded. Consumers awaiting the job get eager content rather than the storage-backed lazy descriptors, which is an inconsistency across backends.♻️ Align with fs backend
import(options: TImportOptions): SkillImportJob { const task = this.importer(options) return Object.assign( task.then(async (result) => { - await this.add(result.skill) - return result + const skill = await this.add(result.skill) + return { ...result, name: skill.name, skill } }), { cancel: task.cancel }, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/skills/storage/indexedDB.ts` around lines 161 - 171, The IndexedDBStorage import() flow returns the loader’s raw skill instead of the stored version, unlike FsSkillStorage.import(). Update IndexedDBStorage.import() so it awaits add(result.skill) and returns the persisted skill from add() (or otherwise rehydrates the stored record) before resolving the job, while keeping the cancel passthrough from importer() intact.packages/kit/src/skills/storage/fs.ts (1)
96-121: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
list()performs a full per-skill read to produce summaries.Each summary requires
get(), which readsSKILL.mdand recursively walks/stats every resource file. For roots with many skills this is significantly more I/O than the IndexedDB backend (which reads inline metadata). Also note that if any skill'sSKILL.mdhas empty instructions,readSkillDirectorythrows (Line 147) and that error escapesget()(onlyENOENTis swallowed), breaking the entirelist().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/skills/storage/fs.ts` around lines 96 - 121, The fs-backed skill listing in list() is doing expensive full skill reads via get() for every directory, and any parsing failure from readSkillDirectory can abort the whole list. Update list() to build summaries without loading each full SKILL.md/resource tree, using only lightweight metadata or directory inspection where possible, and make get()/readSkillDirectory failures for a single skill non-fatal so one bad skill does not break the entire listing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/kit/src/skills/test/fsStorage.test.ts`:
- Around line 66-105: The fsStorage test relies on fixture skill directories
under the test .cache path, but the referenced weather and vue-best-practices
SKILL.md fixtures are missing from the repo. Add those committed fixture
directories/files so createTempRoot(), storage.list(), and storage.import() in
fsStorage.test.ts can copy and validate real skill data during the test.
---
Nitpick comments:
In `@packages/kit/src/skills/storage/fs.ts`:
- Around line 96-121: The fs-backed skill listing in list() is doing expensive
full skill reads via get() for every directory, and any parsing failure from
readSkillDirectory can abort the whole list. Update list() to build summaries
without loading each full SKILL.md/resource tree, using only lightweight
metadata or directory inspection where possible, and make
get()/readSkillDirectory failures for a single skill non-fatal so one bad skill
does not break the entire listing.
In `@packages/kit/src/skills/storage/indexedDB.ts`:
- Around line 161-171: The IndexedDBStorage import() flow returns the loader’s
raw skill instead of the stored version, unlike FsSkillStorage.import(). Update
IndexedDBStorage.import() so it awaits add(result.skill) and returns the
persisted skill from add() (or otherwise rehydrates the stored record) before
resolving the job, while keeping the cancel passthrough from importer() intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7db29771-794a-48ea-b87e-8863b9a8686c
📒 Files selected for processing (13)
packages/kit/package.jsonpackages/kit/src/index.tspackages/kit/src/node.tspackages/kit/src/skills/storage/fs.tspackages/kit/src/skills/storage/importSkill.tspackages/kit/src/skills/storage/index.tspackages/kit/src/skills/storage/indexedDB.tspackages/kit/src/skills/storage/memory.tspackages/kit/src/skills/storage/node.tspackages/kit/src/skills/storage/types.tspackages/kit/src/skills/test/fsStorage.test.tspackages/kit/src/skills/test/indexedDBStorage.test.tspackages/kit/src/skills/test/memoryStorage.test.ts
2 participants
PR 描述
新增 skill storage 层，将 storage 作为和 loader 平级的
SkillDefinitionprovider，支持持久化、恢复和从 loader source 导入 skill。主要改动：
SkillStorage接口，提供add/get/has/delete/list/import。storage.import()，内部复用 loader 的loadSkillWithDetails()，导入后写入 storage。fake-indexeddb作为 IndexedDB storage 测试依赖。验证：
pnpm -F @opentiny/tiny-robot-kit test -- --run src/skills/test/memoryStorage.test.ts src/skills/test/indexedDBStorage.test.ts src/skills/test/fsStorage.test.ts pnpm buildSummary by CodeRabbit
listsummaries.