feat(kit): add skill storage providers#367
Conversation
|
✅ Preview build completed successfully!
|
📦 Package Previewpnpm add https://pkg.pr.new/@opentiny/tiny-robot@e65d881 pnpm add https://pkg.pr.new/@opentiny/tiny-robot-kit@e65d881 pnpm add https://pkg.pr.new/@opentiny/tiny-robot-svgs@e65d881 commit: e65d881 |
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughAdds a complete ChangesSkill Storage Subsystem
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SkillImporter
participant SkillStorage
participant Persistence
Caller->>SkillStorage: import(options)
SkillStorage->>SkillImporter: load skill
SkillImporter-->>SkillStorage: SkillImportResult
SkillStorage->>Persistence: add(skill)
Persistence-->>SkillStorage: stored skill
SkillStorage-->>Caller: import result with persisted skill
Possibly related PRs
Suggested reviewers: 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
…urce summary listing
🧹 Preview Cleaned UpThe preview deployment has been removed. |
3 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
add/get/has/deletepluslistsummaries.