feat(kit): add core skill plugin capabilities#368
Conversation
|
✅ Preview build completed successfully!
|
📦 Package Previewpnpm add https://pkg.pr.new/@opentiny/tiny-robot@eda3c17 pnpm add https://pkg.pr.new/@opentiny/tiny-robot-kit@eda3c17 pnpm add https://pkg.pr.new/@opentiny/tiny-robot-svgs@eda3c17 commit: eda3c17 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds skill selection and resource capability modules, a request-scoped ChangesSkill Plugin and Capabilities
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MessageEngine
participant skillPlugin
participant Model
participant SkillResources
MessageEngine->>skillPlugin: initialize selection and request context
skillPlugin->>Model: provide selection tools and instructions
Model->>skillPlugin: select requested skills
skillPlugin->>SkillResources: resolve selected skill tools
SkillResources->>Model: return file listings or file content
skillPlugin->>Model: inject resolved skill instructions
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 |
|
@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/capabilities/selection.ts (1)
10-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an empty-candidates guard for consistency with
createSkillResourceInstructionsMessage.
createSkillSelectionRuntimeToolsalready short-circuits whencandidates.length === 0(no tool is exposed), butcreateSkillSelectionInstructionsMessagehas no equivalent guard — it will still instruct the model to "Use the select_skills tool before answering" even when no tool is actually registered. The resource capability module (resources.ts) applies the same guard (hasSkillResources) symmetrically to both its instructions and runtime-tools functions; this module doesn't.If callers in
skillPlugin.tsalways supply non-empty candidates before invoking this function, this is currently unreachable, but adding the guard removes the fragile cross-file assumption.♻️ Suggested guard
export const createSkillSelectionInstructionsMessage = ({ candidates, preferredSkillNames, }: { candidates: SkillCandidate[] preferredSkillNames?: string[] }): ChatCompletionSystemMessageParam => { + if (candidates.length === 0) { + return undefined + } + const lines = [Note: this would also require updating the return type to
ChatCompletionSystemMessageParam | undefined, matchingcreateSkillResourceInstructionsMessage's signature.🤖 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/capabilities/selection.ts` around lines 10 - 37, `createSkillSelectionInstructionsMessage` should mirror the empty-candidates guard used by `createSkillSelectionRuntimeTools` and `createSkillResourceInstructionsMessage`. Add a check for `candidates.length === 0` at the start of the function, return `undefined` in that case, and update the return type accordingly to `ChatCompletionSystemMessageParam | undefined`. Keep the rest of the message-building logic in `selection.ts` unchanged for non-empty candidates.packages/kit/src/skills/capabilities/resources.ts (1)
11-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider restricting
skillNameto the active skill names viaenum, mirroringselection.ts.
select_skills's parameter schema constrainsskillNamesviaenum: candidateNames(selection.ts:74), butlist_skill_files/read_skill_file'sskillNameparameter here has no such constraint, even though only names present in the currentskillsarray are ever valid (seefindSkill). Since these tools are already built per-call insidecreateSkillResourceRuntimeTools(skills), the schema could be generated dynamically withenum: skills.map((skill) => skill.name)to reduce hallucinated skill names, consistent with the guidance already established inselection.ts.Handler-side validation (
skill_not_found) already covers correctness, so this is purely a model-steering improvement, not a functional gap.Also applies to: 93-108
🤖 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/capabilities/resources.ts` around lines 11 - 51, The `skillName` parameter in `skillResourceTools` is currently unconstrained, which can lead the model to invent invalid names. Update `createSkillResourceRuntimeTools(skills)` so the schemas for `listSkillFiles` and `readSkillFile` set `skillName` to an `enum` built from `skills.map((skill) => skill.name)`, matching the pattern used in `selection.ts` for `select_skills`. Keep the existing `findSkill`/`skill_not_found` runtime validation unchanged, and apply the same schema change anywhere the same tool definitions are duplicated.
🤖 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/message/plugins/skillPlugin.ts`:
- Around line 234-258: In resolveSkillsByNames, avoid marking a skill as
unresolved when getSkillByName returns a valid SkillDefinition whose skill.name
differs from the requested name; track resolution status from the actual lookup
result rather than only the requested name. Update the unresolvedSkillNames
bookkeeping so aliases/normalized names don’t create contradictory state, and
ensure onSkillsResolved/SkillRequestContext only receive truly unresolved
entries. Also distinguish resolver failures in the try/catch from “not found”
cases by preserving the failure path in the results object instead of collapsing
every exception into a generic failed flag.
---
Nitpick comments:
In `@packages/kit/src/skills/capabilities/resources.ts`:
- Around line 11-51: The `skillName` parameter in `skillResourceTools` is
currently unconstrained, which can lead the model to invent invalid names.
Update `createSkillResourceRuntimeTools(skills)` so the schemas for
`listSkillFiles` and `readSkillFile` set `skillName` to an `enum` built from
`skills.map((skill) => skill.name)`, matching the pattern used in `selection.ts`
for `select_skills`. Keep the existing `findSkill`/`skill_not_found` runtime
validation unchanged, and apply the same schema change anywhere the same tool
definitions are duplicated.
In `@packages/kit/src/skills/capabilities/selection.ts`:
- Around line 10-37: `createSkillSelectionInstructionsMessage` should mirror the
empty-candidates guard used by `createSkillSelectionRuntimeTools` and
`createSkillResourceInstructionsMessage`. Add a check for `candidates.length ===
0` at the start of the function, return `undefined` in that case, and update the
return type accordingly to `ChatCompletionSystemMessageParam | undefined`. Keep
the rest of the message-building logic in `selection.ts` unchanged for non-empty
candidates.
🪄 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: db793450-50c7-4d95-8bf0-0c15f4acbbce
📒 Files selected for processing (10)
packages/kit/src/message/plugins/index.tspackages/kit/src/message/plugins/skillPlugin.tspackages/kit/src/message/utils.tspackages/kit/src/skills/capabilities/commands.tspackages/kit/src/skills/capabilities/resources.tspackages/kit/src/skills/capabilities/selection.tspackages/kit/src/skills/capabilities/utils.tspackages/kit/src/skills/test/resourceCapability.test.tspackages/kit/src/skills/test/skillPlugin.test.tspackages/kit/src/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kit/src/skills/capabilities/selection.ts (1)
80-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty skill selections explicitly
packages/kit/src/skills/capabilities/selection.ts:82-86—getUniqueStringArrayreturns[]for an emptyskillNamesarray, soif (!requestedSkillNames)won’t catch it. If zero selections should be invalid, add an explicitrequestedSkillNames.length === 0check before continuing.🤖 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/capabilities/selection.ts` around lines 80 - 98, The handler only rejects missing skillNames, not an explicitly empty array. Update the validation in the selection capability handler to also reject when requestedSkillNames.length === 0, returning the existing skill_names_required response before invalid-name checks.
🤖 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/package.json`:
- Line 80: Move the vite entry from dependencies to devDependencies in
packages/kit/package.json, preserving its version specification and ensuring it
is removed from the runtime dependency list.
---
Outside diff comments:
In `@packages/kit/src/skills/capabilities/selection.ts`:
- Around line 80-98: The handler only rejects missing skillNames, not an
explicitly empty array. Update the validation in the selection capability
handler to also reject when requestedSkillNames.length === 0, returning the
existing skill_names_required response before invalid-name checks.
🪄 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: 2a4f6dc7-f3e7-47f5-876a-24bcf68b7909
📒 Files selected for processing (6)
packages/kit/package.jsonpackages/kit/src/message/plugins/index.tspackages/kit/src/message/plugins/skillPlugin.tspackages/kit/src/skills/capabilities/resources.tspackages/kit/src/skills/capabilities/selection.tspackages/kit/src/skills/test/skillPlugin.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/kit/src/message/plugins/index.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kit/src/message/plugins/skillPlugin.ts (1)
221-242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve resolver errors in
resolveSkillsByNames
catchstill folds resolver exceptions intounresolvedSkillNames, so a transientgetSkillByNamefailure is indistinguishable from a missing skill. Surface the error separately or include it in the result.🤖 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/message/plugins/skillPlugin.ts` around lines 221 - 242, Update resolveSkillsByNames so getSkillByName exceptions are preserved separately from genuinely unresolved skill names: capture each resolver error in the per-name result, return the collected errors (or otherwise expose them), and keep unresolvedSkillNames limited to missing or undefined skills.
🧹 Nitpick comments (1)
packages/kit/src/skills/test/skillPlugin.test.ts (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated
onInstructionsResolvedsystem-message injection mock.The same mock pattern — prepending a system message with
skillContext.instructions.join('\n\n')tocontext.requestBody.messages— appears at lines 37-45 and 280-288. Extracting a shared helper (e.g.,createInstructionInjectingMock()) would reduce duplication and keep both tests in sync if the injection logic evolves.Also applies to: 280-288
🤖 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/test/skillPlugin.test.ts` around lines 37 - 45, Extract the duplicated onInstructionsResolved mock logic into a shared helper such as createInstructionInjectingMock(), preserving the system-message construction and prepending behavior. Replace both occurrences around the existing test sections, including the one near the later referenced location, with calls to the helper so both tests remain consistent.
🤖 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.
Outside diff comments:
In `@packages/kit/src/message/plugins/skillPlugin.ts`:
- Around line 221-242: Update resolveSkillsByNames so getSkillByName exceptions
are preserved separately from genuinely unresolved skill names: capture each
resolver error in the per-name result, return the collected errors (or otherwise
expose them), and keep unresolvedSkillNames limited to missing or undefined
skills.
---
Nitpick comments:
In `@packages/kit/src/skills/test/skillPlugin.test.ts`:
- Around line 37-45: Extract the duplicated onInstructionsResolved mock logic
into a shared helper such as createInstructionInjectingMock(), preserving the
system-message construction and prepending behavior. Replace both occurrences
around the existing test sections, including the one near the later referenced
location, with calls to the helper so both tests remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b03cfa1-0f10-413d-bffa-21d7347f093a
📒 Files selected for processing (4)
packages/kit/package.jsonpackages/kit/src/message/plugins/index.tspackages/kit/src/message/plugins/skillPlugin.tspackages/kit/src/skills/test/skillPlugin.test.ts
…emove request body modification
|
辛苦看一下，如果是特性设计，请忽略。
|
…ith new utility functions and type definitions
1. Skill instructions 未实际注入请求
2. Browser loader 与核心 skill 类型缺少公开导出
3. Auto 模式隐式依赖
|
🧹 Preview Cleaned UpThe preview deployment has been removed. |
3 participants
PR 描述
新增 core
skillPlugin与 skill capabilities，将已选择的 skills 转换为 system instructions 和 runtime tools，接入 message engine。主要改动：
message/plugins/skillPlugin.ts。manual/auto/none三种 request-level skill selection。manual模式支持直接传入完整SkillDefinition[]，或通过skillNames + getSkillByName解析。auto模式支持候选 skill summaries、偏好 skill names、select_skillsruntime tool，以及选择后解析完整 skill。SkillRequestContext，记录skills、skillNames、requestedSkillNames、unresolvedSkillNames、runtimeTools和 selection 状态。list_skill_files/read_skill_fileruntime tools，并在 system instruction 中提示先 list 再 read。execute_skill_command。getUniqueStringArray通用工具函数。验证：
pnpm -F @opentiny/tiny-robot-kit test -- --run src/skills/test/resourceCapability.test.ts src/skills/test/skillPlugin.test.ts src/message/test/toolPlugin.test.ts pnpm buildSummary by CodeRabbit
select_skills) with preferred-name support.