feat(skill): add draft confirmation card with view/install/discard#1687
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (13)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds an in-chat skill draft confirmation card flow: tool responses include structured draft metadata, runtime dispatch enqueues a question_request with view/install/discard options, presenter methods for draft view/install/discard are implemented, renderer shows preview and localized option labels, and tests cover end-to-end behavior. ChangesSkill Draft Confirmation Card
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/renderer/src/i18n/zh-CN/chat.json (1)
73-73: 💤 Low valueConsider consistent section ordering across locales.
The
skillDraftsection appears after theinputsection (line 73) in zh-CN, but appears after theonboardingsection (line ~384) in all other locale files. While JSON key order has no functional impact, consistent ordering improves maintainability.🤖 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 `@src/renderer/src/i18n/zh-CN/chat.json` at line 73, Move the "skillDraft" object in zh-CN chat.json from its current position after the "input" section to follow the "onboarding" section so the key ordering matches other locales; specifically locate the "skillDraft" key and cut/paste the entire object to immediately after the "onboarding" key, adjust surrounding commas to keep valid JSON, and run a quick JSON lint to confirm no syntax errors.
🤖 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 `@src/main/presenter/agentRuntimePresenter/dispatch.ts`:
- Around line 416-435: The extractor currently accepts any MCPToolResponse that
contains toolResult.skillDraft; restrict it to only trust responses from the
known/trusted skill-draft tool by checking the tool identity first (e.g.,
compare rawData.toolName or rawData.toolExecution.name/id against a single
trusted constant like SKILL_DRAFT_TOOL_NAME or an allowlist) before reading
toolResult.skillDraft; if the tool identity does not match, return undefined.
Apply the same guard in the other occurrence referenced (around the other
extract call at the later location) so only the trusted tool can trigger the
draft prompt flow.
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 302-310: The code exposes draft-confirmation actions through an
optional skillPresenter which can be undefined and cause runtime throws when a
user triggers a draft action; either make the draft methods required on
skillPresenter or short-circuit before showing/enqueuing the confirmation. Fix
by updating the presenter field or call sites that enqueue confirmations: ensure
the private readonly skillPresenter property includes non-optional
'viewDraftSkill', 'installDraftSkill', and 'discardDraftSkill' (so callers must
supply them), or add a guard in the confirmation/enqueue logic (check if
!this.skillPresenter or missing methods) and immediately resolve/return a clean
error/disabled response instead of displaying the confirmation UI. Update the
same pattern where draft actions are enqueued (the other occurrences referenced
around the file) so every draft-confirmation path checks skillPresenter presence
before proceeding.
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 2467-2477: The cleanup function removeEmptyDraftConversationDir
currently runs for install/discard flows but not when manageDraftSkill is
invoked with action 'delete', leaving empty conversation folders; update the
manageDraftSkill handler (the function named manageDraftSkill and its 'delete'
branch) to call this.removeEmptyDraftConversationDir(conversationId) after the
deletion completes (same place it’s called for 'install'/'discard') so the
per-conversation draft directory is removed when empty.
In `@src/renderer/src/i18n/da-DK/chat.json`:
- Around line 384-401: The skillDraft localization block is still in English;
update the da-DK chat.json "skillDraft" entries to Danish by replacing each
string: set confirmationTitle to "Udkast til færdighed", confirmationQuestion to
"Et færdighedsudkast er oprettet: {name}", previewTitle to "Forhåndsvisning af
udkastindhold", actions.view to "Vis indhold", actions.viewDescription to
"Forhåndsvis udkastets indhold", actions.install to "Installer som færdighed",
actions.installDescription to "Installer udkastet i færdighedsmappen",
actions.discard to "Forkast", actions.discardDescription to "Slet det
midlertidige udkast", and result.viewed/installed/discarded/failed to "Udkast
vist"/"Udkast installeret"/"Udkast forkastet"/"Udkast-handling mislykkedes"
respectively so Danish users see fully localized strings for the new flow.
In `@src/renderer/src/i18n/de-DE/chat.json`:
- Around line 384-401: The German locale's skillDraft block contains English
strings; update the "skillDraft" object keys (confirmationTitle,
confirmationQuestion, previewTitle, actions.view, actions.viewDescription,
actions.install, actions.installDescription, actions.discard,
actions.discardDescription, and result.*) with proper German translations,
preserving placeholders like {name} exactly and keeping
punctuation/capitalization consistent with other de-DE entries; ensure messages
for "viewed", "installed", "discarded", and "failed" are translated into German
as well.
In `@src/renderer/src/i18n/es-ES/chat.json`:
- Around line 384-401: Translate the English strings under the "skillDraft" key
into Spanish: update "confirmationTitle", "confirmationQuestion",
"previewTitle", all entries under "actions" (view, viewDescription, install,
installDescription, discard, discardDescription) and all entries under "result"
(viewed, installed, discarded, failed) so the es-ES locale is fully Spanish;
keep placeholders like {name} intact and use natural Spanish phrases (e.g.,
"Borrador de habilidad", "Se ha generado un borrador: {name}", "Vista previa del
contenido", "Ver contenido", "Instalar como habilidad", "Descartar", "Acción de
borrador fallida", etc.) while preserving key names ("skillDraft" and nested
keys) exactly.
In `@src/renderer/src/i18n/pl-PL/chat.json`:
- Around line 384-401: The skillDraft block contains English strings; replace
each value under "skillDraft" with Polish translations while preserving
placeholders (e.g., {name}) and the JSON keys: confirmationTitle,
confirmationQuestion, previewTitle, actions.view, actions.viewDescription,
actions.install, actions.installDescription, actions.discard,
actions.discardDescription, result.viewed, result.installed, result.discarded,
result.failed; for example use Polish equivalents like "Szkic umiejętności" for
confirmationTitle and "Wygenerowano szkic umiejętności: {name}" for
confirmationQuestion, and provide Polish texts for previewTitle ("Podgląd treści
szkicu"), actions.view ("Wyświetl treść"), actions.viewDescription ("Podgląd
treści szkicu umiejętności"), actions.install ("Zainstaluj jako umiejętność"),
actions.installDescription ("Zainstaluj szkic w katalogu umiejętności"),
actions.discard ("Usuń"), actions.discardDescription ("Usuń tymczasowy szkic"),
and result messages like "Szkic wyświetlony", "Szkic zainstalowany", "Szkic
usunięty", "Akcja na szkicu nie powiodła się" respectively.
---
Nitpick comments:
In `@src/renderer/src/i18n/zh-CN/chat.json`:
- Line 73: Move the "skillDraft" object in zh-CN chat.json from its current
position after the "input" section to follow the "onboarding" section so the key
ordering matches other locales; specifically locate the "skillDraft" key and
cut/paste the entire object to immediately after the "onboarding" key, adjust
surrounding commas to keep valid JSON, and run a quick JSON lint to confirm no
syntax errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87476125-5229-49ca-91c2-40933936c2d4
📒 Files selected for processing (45)
docs/features/skill-draft-confirmation-card/plan.mddocs/features/skill-draft-confirmation-card/spec.mddocs/features/skill-draft-confirmation-card/tasks.mdsrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/routes/chat/chatService.tssrc/renderer/src/components/chat/ChatToolInteractionOverlay.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/components/message/MessageBlockQuestionRequest.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/pages/ChatPage.vuesrc/shared/chat.d.tssrc/shared/contracts/common.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat.tssrc/shared/types/skill.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/message/MessageBlockBasics.test.tstsconfig.node.tsbuildinfo
Comment on lines
+416
to
+435
| function extractSkillDraftPromptPayload( | ||
| rawData: MCPToolResponse | ||
| ): SkillDraftPromptPayload | undefined { | ||
| const toolResult = rawData.toolResult as SkillDraftToolResult | null | undefined | ||
| const skillDraft = toolResult?.skillDraft | ||
| if (!skillDraft || typeof skillDraft !== 'object') { | ||
| return undefined | ||
| } | ||
| if (skillDraft.status !== 'created') { | ||
| return undefined | ||
| } | ||
| if (typeof skillDraft.draftId !== 'string' || typeof skillDraft.skillName !== 'string') { | ||
| return undefined | ||
| } | ||
| const draftId = skillDraft.draftId.trim() | ||
| const skillName = skillDraft.skillName.trim() | ||
| if (!draftId || !skillName) { | ||
| return undefined | ||
| } | ||
| return { draftId, skillName } |
Contributor
There was a problem hiding this comment.
Only accept draft prompts from the expected tool origin.
This trusts any tool result that happens to expose toolResult.skillDraft, then turns it into a privileged local draft action flow. A buggy or untrusted MCP tool can spoof this shape and surface view/install/discard against an arbitrary draft id. Gate the extraction on the executed tool name/source before enqueueing the question.
Suggested fix
-function extractSkillDraftPromptPayload(
- rawData: MCPToolResponse
-): SkillDraftPromptPayload | undefined {
+function extractSkillDraftPromptPayload(
+ toolName: string,
+ rawData: MCPToolResponse
+): SkillDraftPromptPayload | undefined {
+ if (toolName !== 'skill_manage') {
+ return undefined
+ }
const toolResult = rawData.toolResult as SkillDraftToolResult | null | undefined
const skillDraft = toolResult?.skillDraft
if (!skillDraft || typeof skillDraft !== 'object') {
return undefined
}
@@
- skillDraftPrompt: extractSkillDraftPromptPayload(toolRawData),
+ skillDraftPrompt: extractSkillDraftPromptPayload(completedToolCall.name, toolRawData),Also applies to: 1058-1058
🤖 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 `@src/main/presenter/agentRuntimePresenter/dispatch.ts` around lines 416 - 435,
The extractor currently accepts any MCPToolResponse that contains
toolResult.skillDraft; restrict it to only trust responses from the
known/trusted skill-draft tool by checking the tool identity first (e.g.,
compare rawData.toolName or rawData.toolExecution.name/id against a single
trusted constant like SKILL_DRAFT_TOOL_NAME or an allowlist) before reading
toolResult.skillDraft; if the tool identity does not match, return undefined.
Apply the same guard in the other occurrence referenced (around the other
extract call at the later location) so only the trusted tool can trigger the
draft prompt flow.
Comment on lines
302
to
310
| private readonly skillPresenter?: Pick< | ||
| ISkillPresenter, | ||
| 'getMetadataList' | 'getActiveSkills' | 'loadSkillContent' | ||
| | 'getMetadataList' | ||
| | 'getActiveSkills' | ||
| | 'loadSkillContent' | ||
| | 'viewDraftSkill' | ||
| | 'installDraftSkill' | ||
| | 'discardDraftSkill' | ||
| > |
Contributor
There was a problem hiding this comment.
Don’t expose a draft-confirmation flow that depends on an optional port.
This PR can enqueue skill-draft confirmations, but the runtime still allows skillPresenter to be absent and then throws when the user clicks one of the draft actions. That leaves the message in a dead-end state instead of resolving the interaction cleanly. Make these draft methods required for this path, or short-circuit before the confirmation block is shown.
Also applies to: 325-333, 1011-1013
🤖 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 `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 302 - 310,
The code exposes draft-confirmation actions through an optional skillPresenter
which can be undefined and cause runtime throws when a user triggers a draft
action; either make the draft methods required on skillPresenter or
short-circuit before showing/enqueuing the confirmation. Fix by updating the
presenter field or call sites that enqueue confirmations: ensure the private
readonly skillPresenter property includes non-optional 'viewDraftSkill',
'installDraftSkill', and 'discardDraftSkill' (so callers must supply them), or
add a guard in the confirmation/enqueue logic (check if !this.skillPresenter or
missing methods) and immediately resolve/return a clean error/disabled response
instead of displaying the confirmation UI. Update the same pattern where draft
actions are enqueued (the other occurrences referenced around the file) so every
draft-confirmation path checks skillPresenter presence before proceeding.
Comment on lines
+2467
to
+2477
| private removeEmptyDraftConversationDir(conversationId: string): void { | ||
| const safeConversationId = this.validateDraftConversationId(conversationId) | ||
| if (!safeConversationId) { | ||
| return | ||
| } | ||
|
|
||
| const conversationDir = path.join(this.draftsRoot, safeConversationId) | ||
| if (fs.existsSync(conversationDir) && fs.readdirSync(conversationDir).length === 0) { | ||
| fs.rmSync(conversationDir, { recursive: true, force: true }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Apply empty conversation-dir cleanup on manageDraftSkill('delete') as well.
removeEmptyDraftConversationDir is wired for install/discard, but delete still leaves empty per-conversation draft folders behind.
Suggested patch
case 'delete': {
@@
fs.rmSync(draftPath, { recursive: true, force: true })
+ this.removeEmptyDraftConversationDir(conversationId)
return { success: true, action, draftId }
}🤖 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 `@src/main/presenter/skillPresenter/index.ts` around lines 2467 - 2477, The
cleanup function removeEmptyDraftConversationDir currently runs for
install/discard flows but not when manageDraftSkill is invoked with action
'delete', leaving empty conversation folders; update the manageDraftSkill
handler (the function named manageDraftSkill and its 'delete' branch) to call
this.removeEmptyDraftConversationDir(conversationId) after the deletion
completes (same place it’s called for 'install'/'discard') so the
per-conversation draft directory is removed when empty.
Comment on lines
+384
to
+401
| "skillDraft": { | ||
| "confirmationTitle": "Skill Draft", | ||
| "confirmationQuestion": "A skill draft has been generated: {name}", | ||
| "previewTitle": "Draft Content Preview", | ||
| "actions": { | ||
| "view": "View Content", | ||
| "viewDescription": "Preview the draft skill content", | ||
| "install": "Install as Skill", | ||
| "installDescription": "Install the draft into the skills directory", | ||
| "discard": "Discard", | ||
| "discardDescription": "Delete the temporary draft" | ||
| }, | ||
| "result": { | ||
| "viewed": "Draft viewed", | ||
| "installed": "Draft installed", | ||
| "discarded": "Draft discarded", | ||
| "failed": "Draft action failed" | ||
| } |
Contributor
There was a problem hiding this comment.
Provide Spanish translations for the new skillDraft entries.
These strings are currently English, so the es-ES locale will show mixed-language content in the draft-confirmation flow.
🤖 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 `@src/renderer/src/i18n/es-ES/chat.json` around lines 384 - 401, Translate the
English strings under the "skillDraft" key into Spanish: update
"confirmationTitle", "confirmationQuestion", "previewTitle", all entries under
"actions" (view, viewDescription, install, installDescription, discard,
discardDescription) and all entries under "result" (viewed, installed,
discarded, failed) so the es-ES locale is fully Spanish; keep placeholders like
{name} intact and use natural Spanish phrases (e.g., "Borrador de habilidad",
"Se ha generado un borrador: {name}", "Vista previa del contenido", "Ver
contenido", "Instalar como habilidad", "Descartar", "Acción de borrador
fallida", etc.) while preserving key names ("skillDraft" and nested keys)
exactly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests