feat(skills): isolate agent skill roots#2007
Merged
Merged
Conversation
Restructure renderer scope boundaries per docs/architecture/renderer-scope-optimization: - Move ChatTabView into apps/chat-main; drop unused views/SettingsTabView - Extract foundation/appearance/documentAppearance shared by all renderer entries - Session store: paginated list with cursor, tombstoned deletes, targeted activate/deactivate IPC; refetch when a pending first page is invalidated - Chat search: canonical debounced query, incremental per-row highlighting with a MutationObserver; drain pending observer work on same-query re-apply and count only rendered user-message content - Message window fast path returns a fresh array so totalHeight and the window range react to tail growth; batch measures no longer force per-row full layout rebuilds - Scroll state machine rejects a late session-restore scroll after the user explicitly navigated (search/spotlight) - Split row-level isStreamingMessage (activity grouping) from thread-level isInGeneratingThread (fork gating, variant auto-switch) - Always clear plan snapshots for deleted messages even after switching views
Sidebar search is a pure client-side filter, so pagination must keep auto-filling while a query is active or matches are limited to already loaded pages. Collapsed-group guards still apply when not searching.
Preserve targeted session updates across stale page loads and fence environment reorder rollbacks. Align search indexing with activatable DOM content and refresh the renderer architecture baseline.
…e-optimization Resolve language/i18n conflicts to preserve this PR's scope-boundary race fixes (languageInitialization promise cache, listener-before-snapshot registration, documentAppearance abstraction) alongside the lazy renderer locale loading (#2003) and dashboard CPU reduction (#2004) perf wins from dev.
Address coderabbit review comments on PR #2000: - ChatInputToolbar.test: trigger steer click and assert no emission while pending (was only checking static attributes) - session store: rename isNewerSessionUpdate -> isStaleOrSameSessionUpdate so the predicate name matches its "skip stale/equal update" semantics - extract shared createDeferred helper (with reject) to test/renderer/utils, replacing three local copies in useComposerSubmit/useMessageActions/ useToolInteraction tests - useMessageVirtualization.test: cover layoutSegments provided but returning null at runtime (inline-stream reinsertion fallback) - SettingsApp.test: reset document.documentElement lang/dir in afterEach to match sibling documentAppearance cleanup
Contributor
📝 WalkthroughWalkthroughThe PR replaces shared DeepChat Skill ownership and configuration inheritance with independent Agent-scoped roots, catalogs, migration, runtime validation, import workflows, lifecycle fencing, protected filesystem access, typed routes, and renderer support. ChangesIndependent Agent Skills
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)src/renderer/src/i18n/da-DK/settings.jsonTraceback (most recent call last): src/renderer/src/i18n/de-DE/settings.jsonTraceback (most recent call last): src/renderer/src/i18n/en-US/settings.jsonTraceback (most recent call last):
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/agent/deepchat/resources/systemPromptBuilder.ts (1)
104-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled rejection from
resolveSessionAgentIdbreaks the fail-safe pattern used everywhere else in this function.Every other
skillServicecall inbuildSystemPromptWithSkills(getMetadataList,getActiveSkills,loadSkillContent) is wrapped in try/catch with a warn-and-continue fallback. The newskillService.resolveSessionAgentId(sessionId)call is not, despite running on every prompt build. An error here (e.g. transient lookup/session-state issue) will propagate uncaught and fail the entire turn instead of degrading gracefully.🛡️ Proposed fix
const skillsEnabled = dependencies.skillSettings.isEnabled(); const skillService = dependencies.skillService; - const sessionAgentId = skillsEnabled - ? await skillService.resolveSessionAgentId(sessionId) - : null; + let sessionAgentId: string | null = null; + if (skillsEnabled) { + try { + sessionAgentId = await skillService.resolveSessionAgentId(sessionId); + } catch (error) { + console.warn( + `[DeepChatAgent] Failed to resolve agent id for skills in session ${sessionId}:`, + error, + ); + } + }🤖 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/agent/deepchat/resources/systemPromptBuilder.ts` around lines 104 - 141, Wrap the skillService.resolveSessionAgentId call in buildSystemPromptWithSkills with the same warn-and-continue error handling used for getMetadataList, getActiveSkills, and loadSkillContent. On failure, log the session context and continue with sessionAgentId set to null so prompt construction remains fail-safe.src/main/agent/acp/compatibility/dependencies.ts (1)
105-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist only the validated
activeSkillsinuserContent.
runtimeActiveSkillsis already the validated, permission-checked set used for prompt/tool resolution, butuserContent.activeSkillsstill stores the raw request array. StoreruntimeActiveSkillsinstead so persisted messages don’t list skills that were rejected for the session.🛡️ Proposed fix
userContent: { text: content.text, files: content.files ?? [], links: [], search: false, think: false, - ...(content.activeSkills?.length ? { activeSkills: content.activeSkills } : {}), + ...(runtimeActiveSkills.length ? { activeSkills: runtimeActiveSkills } : {}), ...(content.inlineItems?.length ? { inlineItems: content.inlineItems } : {}) },🤖 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/agent/acp/compatibility/dependencies.ts` around lines 105 - 163, Update the userContent construction in the session request flow to persist the validated runtimeActiveSkills result instead of the raw content.activeSkills array. Use runtimeActiveSkills for the activeSkills field while preserving the existing omission behavior when no validated skills remain.
🧹 Nitpick comments (6)
src/renderer/src/i18n/tr-TR/settings.json (1)
2146-2176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew
skills.agentImportblock shipped untranslated in Turkish and Vietnamese locales. Both files copy the English source strings verbatim for the entire new "Import Skills from Agent" feature, while the same block is fully localized inzh-CN/zh-HK/zh-TW.
src/renderer/src/i18n/tr-TR/settings.json#L2146-L2176: translate allskills.agentImportstrings into Turkish.src/renderer/src/i18n/vi-VN/settings.json#L2146-L2176: translate allskills.agentImportstrings into Vietnamese.As per coding guidelines, "After completing a feature, run pnpm run format, pnpm run i18n, and pnpm run lint" — this translation gap should be caught by that step.
🤖 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/tr-TR/settings.json` around lines 2146 - 2176, Translate every string in the skills.agentImport block from English into Turkish in src/renderer/src/i18n/tr-TR/settings.json (lines 2146-2176), preserving keys and placeholders; translate the corresponding entire block into Vietnamese in src/renderer/src/i18n/vi-VN/settings.json (lines 2146-2176). Run the required format, i18n, and lint commands afterward.Source: Coding guidelines
test/main/skill/skillTools.test.ts (1)
161-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the "current-message active skills" guard test with a non-empty payload.
Passing
[]as the second argument tohandleSkillList('conv-123', [])can't actually demonstrate that current-message active skills aren't merged into the catalog — the assertions would pass identically even without the guard this test is meant to protect. Use a non-empty skill-name array (e.g.['git-commit']) and assert it isn't reflected inresult.skills/activeCountto meaningfully exercise the intended behavior.🤖 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 `@test/main/skill/skillTools.test.ts` around lines 161 - 169, Strengthen the test around handleSkillList by passing a non-empty current-message skill array such as ['git-commit'] instead of []. Keep the catalog assertions and explicitly verify that this payload is not reflected in result.skills or activeCount, while preserving the expected catalog entries and total count.src/main/skill/index.ts (1)
1047-1057: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReads and fully sanitizes management state once per skill during visibility filtering.
getVisibleMetadataFromCachecallsisSkillVisible→isSkillDisabledfor every metadata entry, andisSkillDisabled(Line 1288-1290) re-invokesgetStoredManagementState(), which re-sanitizes the entire stored state (all agents, skills, migration) on each call. That makes each catalog read O(N²) on a hot path (getMetadataList,discoverSkills,discoverScopedSkills).getUnifiedSkillCatalog(Line 1339) already resolves state once and maps — mirror that here by resolving the agent's disabled set once.♻️ Resolve state once and pass disabled lookup into the filter
- private getVisibleMetadataFromCache(agentId: string): SkillMetadata[] { - return this.sortSkillMetadata( - Array.from(this.getMetadataCacheForAgent(agentId).values()).filter((skill) => - this.isSkillVisible(skill, agentId) - ) - ) - } - - private isSkillVisible(metadata: SkillMetadata, agentId: string): boolean { - return Boolean(metadata) && !this.isSkillDisabled(agentId, metadata.name) - } + private getVisibleMetadataFromCache(agentId: string): SkillMetadata[] { + const agentSkills = this.getStoredManagementState().agents[agentId]?.skills + return this.sortSkillMetadata( + Array.from(this.getMetadataCacheForAgent(agentId).values()).filter( + (skill) => Boolean(skill) && agentSkills?.[skill.name]?.disabled !== true + ) + ) + } + + private isSkillVisible(metadata: SkillMetadata, agentId: string): boolean { + return Boolean(metadata) && !this.isSkillDisabled(agentId, metadata.name) + }🤖 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/skill/index.ts` around lines 1047 - 1057, Update getVisibleMetadataFromCache to resolve the agent’s disabled-skill state once before filtering, then pass that lookup into isSkillVisible (and use it when checking visibility) instead of calling isSkillDisabled for every metadata entry. Preserve the existing sorting and visibility behavior while avoiding repeated getStoredManagementState sanitization.src/main/tool/agentTools/agentToolManager.ts (1)
1009-1018: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
resolveActiveSkillRootswork per filesystem tool call.
buildAllowedDirectories(viaresolveActiveSkillRootswhenincludeSkillRootsis true) andbuildProtectedSkillDirectoryRules(which also callsresolveActiveSkillRoots) are invoked back-to-back with the sameconversationId/activeSkillNames, each redoingresolveSessionAgentId,getMetadataList, and per-skillfs.existsSync/fs.statSynccalls. This doubles the I/O cost on everyread/write/edit/glob/greptool invocation.Consider resolving the active skill roots once per call and passing the result into both builders.
Also applies to: 1358-1381
🤖 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/tool/agentTools/agentToolManager.ts` around lines 1009 - 1018, Resolve active skill roots once in the tool-call flow before invoking buildAllowedDirectories and buildProtectedSkillDirectoryRules, then pass the shared result into both builders through their existing or updated parameters. Ensure this applies to both affected call sites and preserves the exec behavior where skill roots are excluded, avoiding repeated resolveActiveSkillRoots work for the same conversationId and activeSkillNames.src/main/skill/skillTools.ts (1)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate builtin-agent constant and reuse the shared one.
src/main/skill/skillTools.tsstill falls back to the literal'deepchat'inhandleSkillList, whilesrc/main/skill/index.tsandsrc/main/skill/agentSkillRoots.tsalready define and useBUILTIN_SKILL_AGENT_ID. Import the exported constant instead to keep the fallback value in one place.♻️ Suggested fix
- const resolvedAgentId = conversationId - ? await this.skillService.resolveSessionAgentId(conversationId) - : 'deepchat' + const resolvedAgentId = conversationId + ? await this.skillService.resolveSessionAgentId(conversationId) + : BUILTIN_SKILL_AGENT_ID🤖 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/skill/skillTools.ts` around lines 21 - 27, Update handleSkillList in skillTools.ts to import and use the shared BUILTIN_SKILL_AGENT_ID from the skill module instead of the literal 'deepchat' fallback, keeping the agent identifier defined in one place.src/main/agent/settings.ts (1)
547-563: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWire
retryPendingDeletedAgentSkillCleanup()into startupThe retry method isn't referenced anywhere in the checked-in code, so pending Agent Skill cleanups added via
addPendingAgentSkillCleanupwill not be retried until the app exits or they are removed directly. Call it from the startup flow (for examplesrc/main/app/composition.ts) instead of leaving it unreachable.🤖 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/agent/settings.ts` around lines 547 - 563, Invoke retryPendingDeletedAgentSkillCleanup() from the application startup flow, such as the composition/bootstrap initialization path, after AgentSettings and its repository are available. Ensure startup awaits or otherwise handles the method’s Promise so pending cleanups registered by addPendingAgentSkillCleanup are retried automatically.
🤖 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 `@docs/architecture/agent-system.md`:
- Around line 32-33: Clarify the migration contract in the architecture
documentation: legacy migration must materialize each readable manual Agent’s
effective config using the built-in config and prior fail-closed fallback rules,
including for corrupt rows where applicable. State that the prohibition on
silently applying built-in values applies only to the post-migration independent
resolver, and align the wording with the contracts in plan.md and spec.md.
In `@src/main/agent/settings.ts`:
- Around line 201-212: Wrap the one-time migration call
materializeIndependentDeepChatAgentConfigs() in start() with try/catch, logging
a warning and continuing startup when it fails. Apply the same error-handling
pattern to the related migration flow around
reconcileLegacyBuiltinAgentSelections and ensure a partial failure does not
rethrow on subsequent launches.
- Around line 449-469: Update the config construction in the agent creation
method so both autoCompactionTriggerThreshold and
autoCompactionRetainRecentPairs are normalized after resolving either the
explicit input.config value or the settings fallback. Preserve the existing
defaults while ensuring explicitly supplied values are passed through
normalizeAutoCompactionTriggerThreshold and
normalizeAutoCompactionRetainRecentPairs before repository.createDeepChatAgent
stores them.
In `@src/main/skill/index.ts`:
- Around line 635-639: Update migrateLegacyAgentSkillScopes’ builtinCatalog loop
to catch failures from migrateLegacySkillExtension per skill, log a warning, and
continue processing remaining skills. Ensure the migration still records
completion and initialization can proceed despite an unreadable sidecar, without
changing successful migration behavior.
In `@src/main/tool/agentTools/agentToolManager.ts`:
- Around line 1441-1472: Update buildProtectedSkillDirectoryRules so failure to
resolve the skills root never returns an empty protected-rule set. When both
skillService.getSkillsDir() and skillSettings.getPath() fail, fail closed by
propagating the resolution error or returning a deny-all rule rooted at a safe
best-effort skills directory; preserve the configured fallback path when
available.
In `@src/renderer/settings/components/skills/InstallFromGitDialog.vue`:
- Around line 181-205: Update the repository URL watcher and scan flow around
scanRequestId and requestedRepoUrl so changing the URL invalidates any in-flight
scan, even while scanning is true. Increment or otherwise invalidate
scanRequestId before the watcher exits, clear stale scan results and selections
as appropriate, and ensure late responses cannot populate results for a
different URL or allow installation from the old scanResult.repoUrl.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 1849-1879: Translate every string in the agentImport block into
the appropriate locale, preserving all keys, placeholders, nested structures,
and JSON validity. Update src/renderer/src/i18n/da-DK/settings.json lines
1849-1879 in Danish; src/renderer/src/i18n/de-DE/settings.json lines 2146-2176
in German; src/renderer/src/i18n/es-ES/settings.json lines 2146-2176 in Spanish;
src/renderer/src/i18n/fa-IR/settings.json lines 1916-1946 in Persian;
src/renderer/src/i18n/fr-FR/settings.json lines 1916-1946 in French; and
src/renderer/src/i18n/he-IL/settings.json lines 1916-1946 in Hebrew.
In `@src/renderer/src/i18n/id-ID/settings.json`:
- Around line 2146-2176: Translate every string in the agentImport section for
the affected locales: src/renderer/src/i18n/id-ID/settings.json lines 2146-2176,
src/renderer/src/i18n/it-IT/settings.json lines 2146-2176, and
src/renderer/src/i18n/ja-JP/settings.json lines 1916-1946. Preserve all keys,
placeholders, nesting, and JSON validity while replacing the English values with
natural Indonesian, Italian, and Japanese translations respectively.
---
Outside diff comments:
In `@src/main/agent/acp/compatibility/dependencies.ts`:
- Around line 105-163: Update the userContent construction in the session
request flow to persist the validated runtimeActiveSkills result instead of the
raw content.activeSkills array. Use runtimeActiveSkills for the activeSkills
field while preserving the existing omission behavior when no validated skills
remain.
In `@src/main/agent/deepchat/resources/systemPromptBuilder.ts`:
- Around line 104-141: Wrap the skillService.resolveSessionAgentId call in
buildSystemPromptWithSkills with the same warn-and-continue error handling used
for getMetadataList, getActiveSkills, and loadSkillContent. On failure, log the
session context and continue with sessionAgentId set to null so prompt
construction remains fail-safe.
---
Nitpick comments:
In `@src/main/agent/settings.ts`:
- Around line 547-563: Invoke retryPendingDeletedAgentSkillCleanup() from the
application startup flow, such as the composition/bootstrap initialization path,
after AgentSettings and its repository are available. Ensure startup awaits or
otherwise handles the method’s Promise so pending cleanups registered by
addPendingAgentSkillCleanup are retried automatically.
In `@src/main/skill/index.ts`:
- Around line 1047-1057: Update getVisibleMetadataFromCache to resolve the
agent’s disabled-skill state once before filtering, then pass that lookup into
isSkillVisible (and use it when checking visibility) instead of calling
isSkillDisabled for every metadata entry. Preserve the existing sorting and
visibility behavior while avoiding repeated getStoredManagementState
sanitization.
In `@src/main/skill/skillTools.ts`:
- Around line 21-27: Update handleSkillList in skillTools.ts to import and use
the shared BUILTIN_SKILL_AGENT_ID from the skill module instead of the literal
'deepchat' fallback, keeping the agent identifier defined in one place.
In `@src/main/tool/agentTools/agentToolManager.ts`:
- Around line 1009-1018: Resolve active skill roots once in the tool-call flow
before invoking buildAllowedDirectories and buildProtectedSkillDirectoryRules,
then pass the shared result into both builders through their existing or updated
parameters. Ensure this applies to both affected call sites and preserves the
exec behavior where skill roots are excluded, avoiding repeated
resolveActiveSkillRoots work for the same conversationId and activeSkillNames.
In `@src/renderer/src/i18n/tr-TR/settings.json`:
- Around line 2146-2176: Translate every string in the skills.agentImport block
from English into Turkish in src/renderer/src/i18n/tr-TR/settings.json (lines
2146-2176), preserving keys and placeholders; translate the corresponding entire
block into Vietnamese in src/renderer/src/i18n/vi-VN/settings.json (lines
2146-2176). Run the required format, i18n, and lint commands afterward.
In `@test/main/skill/skillTools.test.ts`:
- Around line 161-169: Strengthen the test around handleSkillList by passing a
non-empty current-message skill array such as ['git-commit'] instead of []. Keep
the catalog assertions and explicitly verify that this payload is not reflected
in result.skills or activeCount, while preserving the expected catalog entries
and total count.
🪄 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 Plus
Run ID: cb0f8a17-bcf1-4466-8b34-f0feb0726782
📒 Files selected for processing (142)
docs/FLOWS.mddocs/architecture/agent-system.mddocs/architecture/independent-agent-skills/plan.mddocs/architecture/independent-agent-skills/spec.mddocs/architecture/independent-agent-skills/tasks.mddocs/architecture/tool-system.mddocs/features/deepchat-skills-management/spec.mdsrc/main/agent/acp/compatibility/dependencies.tssrc/main/agent/deepchat/deepChatAgentRepository.tssrc/main/agent/deepchat/defaults.tssrc/main/agent/deepchat/resources/systemPromptBuilder.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/sessionSettingsCoordinator.tssrc/main/agent/deepchat/runtime/toolResolver.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/types.tssrc/main/agent/lifecycleGate.tssrc/main/agent/repository/index.tssrc/main/agent/settings.tssrc/main/app/composition.tssrc/main/memory/index.tssrc/main/memory/services/maintenanceService.tssrc/main/session/assignment.tssrc/main/session/assignmentPolicy.tssrc/main/session/contracts.tssrc/main/session/lifecycle.tssrc/main/session/turn.tssrc/main/skill/agentSkillImportService.tssrc/main/skill/agentSkillRoots.tssrc/main/skill/discoveryWorker.tssrc/main/skill/index.tssrc/main/skill/routes.tssrc/main/skill/settings.tssrc/main/skill/skillExecutionService.tssrc/main/skill/skillTools.tssrc/main/skill/sync/index.tssrc/main/tool/agentTools/agentFffSearchHandler.tssrc/main/tool/agentTools/agentFileSystemHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/index.tssrc/renderer/api/SkillClient.tssrc/renderer/api/SkillSyncClient.tssrc/renderer/settings/components/DeepChatAgentsSettings.vuesrc/renderer/settings/components/skills/AdoptSkillDialog.vuesrc/renderer/settings/components/skills/AgentSkillTable.vuesrc/renderer/settings/components/skills/ImportSkillsFromAgentDialog.vuesrc/renderer/settings/components/skills/InstallFromGitDialog.vuesrc/renderer/settings/components/skills/InstallSkillToAgentDialog.vuesrc/renderer/settings/components/skills/SkillAgentsTab.vuesrc/renderer/settings/components/skills/SkillCard.vuesrc/renderer/settings/components/skills/SkillDetailDialog.vuesrc/renderer/settings/components/skills/SkillInstallDialog.vuesrc/renderer/settings/components/skills/SkillSyncDialog/ConflictResolver.vuesrc/renderer/settings/components/skills/SkillSyncDialog/ExportWizard.vuesrc/renderer/settings/components/skills/SkillSyncDialog/ImportWizard.vuesrc/renderer/settings/components/skills/SkillSyncDialog/SkillSelector.vuesrc/renderer/settings/components/skills/SkillSyncDialog/SkillSyncDialog.vuesrc/renderer/settings/components/skills/SkillSyncDialog/SyncResult.vuesrc/renderer/settings/components/skills/SkillSyncDialog/ToolSelector.vuesrc/renderer/settings/components/skills/SkillSyncDialog/index.tssrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/src/components/chat-input/SkillsIndicator.vuesrc/renderer/src/components/chat-input/composables/useSkillsData.tssrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/composables/useChatInputMentions.tssrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/skillsStore.tssrc/shared/contracts/events/skills.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/skillSync.routes.tssrc/shared/contracts/routes/skills.routes.tssrc/shared/types/agentSkillImport.tssrc/shared/types/skill.tssrc/shared/types/skillManagement.tssrc/shared/types/tool.d.tssrc/types/i18n.d.tstest/e2e/specs/16-skills-readonly-route.smoke.spec.tstest/main/agent/deepchat/deepChatAgentRepository.test.tstest/main/agent/deepchat/defaults.test.tstest/main/agent/deepchat/resources/systemPromptBuilder.test.tstest/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/deepchat/runtime/toolResolver.test.tstest/main/agent/lifecycleGate.test.tstest/main/agent/repository.test.tstest/main/agent/settings.test.tstest/main/app/compositionBoundaries.test.tstest/main/memory/maintenanceService.test.tstest/main/routes/contracts.test.tstest/main/routes/dispatcher.test.tstest/main/session/assignment.test.tstest/main/session/assignmentPolicy.test.tstest/main/session/lifecycle.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/sessionFixture.tstest/main/session/turn.test.tstest/main/skill/agentSkillImportService.test.tstest/main/skill/agentSkillRoots.test.tstest/main/skill/discoveryWorker.test.tstest/main/skill/settings.test.tstest/main/skill/skillExecutionService.test.tstest/main/skill/skillService.test.tstest/main/skill/skillServiceAgentScopes.test.tstest/main/skill/skillTools.test.tstest/main/skill/sync/skillSyncService.test.tstest/main/tool/agentTools/agentToolManagerSettings.test.tstest/main/tool/agentTools/agentToolManagerSkillAccess.test.tstest/renderer/api/clients.test.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/DeepChatAgentsSettings.test.tstest/renderer/components/ImportSkillsFromAgentDialog.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/components/SkillInstallTargetScope.test.tstest/renderer/components/SkillSyncSettings.test.tstest/renderer/components/SkillsSettingsAgentScope.test.tstest/renderer/composables/useChatInputSkillScope.test.tstest/renderer/stores/skillsStore.test.ts
💤 Files with no reviewable changes (20)
- src/renderer/settings/components/skills/SkillSyncDialog/SkillSyncDialog.vue
- src/renderer/settings/components/skills/SkillSyncDialog/index.ts
- src/renderer/settings/components/skills/SkillSyncDialog/SyncResult.vue
- src/renderer/settings/components/skills/SkillSyncDialog/ExportWizard.vue
- src/renderer/settings/components/skills/SkillSyncDialog/SkillSelector.vue
- src/renderer/settings/components/skills/AdoptSkillDialog.vue
- src/renderer/settings/components/skills/SkillSyncDialog/ToolSelector.vue
- src/main/agent/deepchat/runtime/types.ts
- src/main/agent/deepchat/runtime/deferredToolExecutor.ts
- src/renderer/settings/components/skills/SkillCard.vue
- src/renderer/settings/components/skills/InstallSkillToAgentDialog.vue
- src/renderer/settings/components/skills/SkillSyncDialog/ConflictResolver.vue
- src/main/memory/services/maintenanceService.ts
- src/renderer/settings/components/skills/SkillSyncDialog/ImportWizard.vue
- src/main/agent/deepchat/runtime/dispatch.ts
- src/main/memory/index.ts
- test/main/agent/deepchat/runtime/toolAdapters.test.ts
- src/renderer/api/SkillSyncClient.ts
- src/shared/types/tool.d.ts
- src/shared/contracts/routes/skillSync.routes.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/renderer/stores/skillsStore.test.ts (1)
84-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the catalog refresh target.
This verifies only the call count, so a regression that refreshes the wrong Agent would still pass. Assert that the
deepchatscope is forwarded to the catalog request.Proposed test assertion
catalogListener?.({ agentIds: ['deepchat'] }) await Promise.resolve() expect(skillClient.getUnifiedSkillCatalog).toHaveBeenCalledTimes(1) +expect(skillClient.getUnifiedSkillCatalog).toHaveBeenCalledWith('deepchat')🤖 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 `@test/renderer/stores/skillsStore.test.ts` around lines 84 - 86, Update the test around catalogListener to assert that skillClient.getUnifiedSkillCatalog was called with the deepchat agent scope, not only that it was called once. Preserve the existing call-count assertion while verifying the forwarded agentIds value.test/main/skill/toolNameMapping.test.ts (1)
44-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact canonical tool list.
toHaveLength(5)would still pass if a settings name were dropped and replaced with another value. Assertresult.toolsagainst the five expected names to protect the normalization contract.Proposed test assertion
- expect(result.tools).toHaveLength(5) + expect(result.tools).toEqual([ + 'deepchat_settings_toggle', + 'deepchat_settings_set_language', + 'deepchat_settings_set_theme', + 'deepchat_settings_set_font_size', + 'deepchat_settings_open' + ])🤖 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 `@test/main/skill/toolNameMapping.test.ts` around lines 44 - 45, Update the assertions in the tool-name mapping test to compare result.tools against the exact five expected canonical tool names, rather than checking only its length. Keep the existing empty result.warnings assertion unchanged.
🤖 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.
Nitpick comments:
In `@test/main/skill/toolNameMapping.test.ts`:
- Around line 44-45: Update the assertions in the tool-name mapping test to
compare result.tools against the exact five expected canonical tool names,
rather than checking only its length. Keep the existing empty result.warnings
assertion unchanged.
In `@test/renderer/stores/skillsStore.test.ts`:
- Around line 84-86: Update the test around catalogListener to assert that
skillClient.getUnifiedSkillCatalog was called with the deepchat agent scope, not
only that it was called once. Preserve the existing call-count assertion while
verifying the forwarded agentIds value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a29ddeb4-3e22-450d-83e4-33e8878c4bbc
📒 Files selected for processing (8)
docs/issues/skill-tool-activation-refresh/spec.mdsrc/main/skill/toolNameMapping.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/agentTools/chatSettingsTools.tssrc/renderer/src/stores/skillsStore.tstest/main/skill/toolNameMapping.test.tstest/main/tool/agentTools/chatSettingsTools.test.tstest/renderer/stores/skillsStore.test.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/stores/skillsStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/tool/agentTools/agentToolManager.ts
…-skills # Conflicts: # src/main/session/lifecycle.ts # src/main/session/turn.ts # test/main/session/lifecycle.test.ts # test/renderer/components/NewThreadPage.test.ts
Contributor
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)
src/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts (1)
2011-2015: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact queue-drain errors before
console.error.
deepChatRuntimeCoordinator.tsandturnCoordinator.tsboth log the rawerrorplussessionIdin production logs. Reuse an existing redaction/logging path and avoid logging the raw error object so failure diagnostics cannot persist sensitive content embedded in provider/user data.🤖 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/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts` around lines 2011 - 2015, Redact queue-drain errors before logging: update the error handlers around drainPendingQueueIfPossible in src/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts:2011-2015 and src/main/agent/deepchat/runtime/turnCoordinator.ts:1398-1402 to reuse the existing redaction/logging path, avoid passing raw error objects to console.error, and preserve the session and reason context without exposing sensitive provider or user data.Source: Linters/SAST tools
🤖 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/session/lifecycle.ts`:
- Around line 60-63: Resolve the assignment before acquiring the lifecycle gate,
and use the canonical resolved agent ID for locking: in
src/main/session/lifecycle.ts lines 60-63 and 165-170, gate on
assignment.agentId; in lines 241-246, gate on runtimeConfig.agentId and pass
that resolved config into the gated helper so assignment resolution is not
repeated. Update the create, detached-session, and subagent lifecycle flows
accordingly.
---
Outside diff comments:
In `@src/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts`:
- Around line 2011-2015: Redact queue-drain errors before logging: update the
error handlers around drainPendingQueueIfPossible in
src/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts:2011-2015 and
src/main/agent/deepchat/runtime/turnCoordinator.ts:1398-1402 to reuse the
existing redaction/logging path, avoid passing raw error objects to
console.error, and preserve the session and reason context without exposing
sensitive provider or user data.
🪄 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 Plus
Run ID: 617c9369-5467-4d56-84e9-fb7ad00a521d
📒 Files selected for processing (47)
docs/issues/skill-tool-activation-refresh/spec.mdsrc/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/app/composition.tssrc/main/session/contracts.tssrc/main/session/lifecycle.tssrc/main/session/turn.tssrc/main/skill/skillTools.tssrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/NewThreadPage.vuesrc/shared/contracts/routes.tstest/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.test.tstest/main/routes/contracts.test.tstest/main/routes/dispatcher.test.tstest/main/session/assignmentPolicy.test.tstest/main/session/lifecycle.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/sessionFixture.tstest/main/session/turn.test.tstest/main/skill/skillTools.test.tstest/renderer/api/clients.test.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/components/SkillsSettingsAgentScope.test.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- docs/issues/skill-tool-activation-refresh/spec.md
- src/main/session/contracts.ts
- src/renderer/src/features/chat-page/ChatPage.vue
- test/main/session/turn.test.ts
- src/renderer/src/i18n/fr-FR/settings.json
- test/main/session/assignmentPolicy.test.ts
- src/renderer/src/i18n/vi-VN/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/de-DE/settings.json
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/pl-PL/settings.json
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/id-ID/settings.json
- src/renderer/src/i18n/es-ES/settings.json
- test/renderer/components/ChatPage.test.ts
- src/renderer/src/i18n/pt-BR/settings.json
- src/shared/contracts/routes.ts
- src/renderer/src/i18n/he-IL/settings.json
- src/main/session/turn.ts
- test/main/session/sessionFixture.ts
- src/renderer/src/i18n/zh-CN/settings.json
- src/renderer/src/i18n/ms-MY/settings.json
- test/main/routes/contracts.test.ts
- test/main/session/runtimeIntegration.test.ts
- test/renderer/components/ChatInputBox.test.ts
- src/renderer/src/i18n/fa-IR/settings.json
- test/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.test.ts
- src/main/skill/skillTools.ts
- src/renderer/src/components/chat/ChatInputBox.vue
- test/main/skill/skillTools.test.ts
- test/renderer/api/clients.test.ts
- src/main/app/composition.ts
- test/renderer/components/SkillsSettingsAgentScope.test.ts
- src/renderer/settings/components/skills/SkillsSettings.vue
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
UI
BEFORE
AFTER
Validation
pnpm run formatpnpm run i18npnpm run lintpnpm run typecheckSummary by CodeRabbit