fix(message): skill activation follows messages, not sessions - #1831
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds message-scoped ChangesfetchModels async/await fix
Message-scoped active skills and runtime refresh
Sequence Diagram(s)sequenceDiagram
participant ChatInputBox
participant AgentSessionPresenter
participant AgentRuntimePresenter
participant AgentToolManager
participant processStream
ChatInputBox->>AgentSessionPresenter: sendMessage(text, files, activeSkills)
AgentSessionPresenter->>AgentRuntimePresenter: processMessage(normalizedInput)
AgentRuntimePresenter->>AgentToolManager: getAllToolDefinitions(activeSkillNames)
AgentRuntimePresenter->>processStream: runStreamForMessage(...)
processStream->>AgentToolManager: callTool(skill_view, activeSkillNames)
AgentToolManager-->>processStream: activation metadata
processStream->>AgentRuntimePresenter: hooks.activateSkill(skillName)
processStream->>AgentRuntimePresenter: refreshTools(activeSkillNames)
processStream->>AgentRuntimePresenter: refreshSystemPrompt(activeSkillNames, tools)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/stores/ui/message.ts (1)
216-241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefault
activeSkillsin the fallback user-content path.If
record.contentis legacy plain text or otherwise fails parsing, the fallback object still omitsactiveSkills. That means older user messages can surfaceundefinedhere instead of the empty array this PR expects for message-scoped skills.Suggested fix
entry.userContent = { text: '', files: [], links: [], search: false, - think: false + think: false, + activeSkills: [] }🤖 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/stores/ui/message.ts` around lines 216 - 241, The fallback user-content path in message parsing omits activeSkills, so legacy or unparsable content can leave it undefined. Update the fallback assignment in the message store’s parsing flow (the try/catch around JSON.parse in the user-content builder) to always initialize activeSkills to an empty array, matching the parsed branch and the expected message-scoped skills shape.
🧹 Nitpick comments (4)
src/main/presenter/toolPresenter/index.ts (1)
617-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor wording nit on the updated skill prompts: "installed skills and manual pinned status" reads awkwardly (should be "manually pinned status" or "manual pin status"). These are model-facing instruction strings, so clarity helps prompt comprehension.
🤖 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/toolPresenter/index.ts` around lines 617 - 642, The wording in buildSkillsPrompt is awkward in the skill_list instruction string, which can reduce clarity for model-facing prompts. Update the text in buildSkillsPrompt so “installed skills and manual pinned status” reads naturally, such as by using “manually pinned status” or “manual pin status,” while keeping the meaning unchanged.test/main/presenter/agentRuntimePresenter/process.test.ts (1)
502-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the hook-backed skill list in this test.
This now locks in the fallback path (
undefined) instead of the production path, whereactivateSkill/getActiveSkillNamesfeed the refreshed skill list. Please add hooks to the fixture and assert thatrefreshTools/refreshSystemPromptreceive the activated skill name as well, so regressions in the new plumbing fail here.🤖 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/presenter/agentRuntimePresenter/process.test.ts` around lines 502 - 569, The test in processStream is still using the fallback skill list path, so it does not cover the hook-backed flow. Update the fixture around process.test.ts and createParams/processStream setup to provide the active-skill hooks used by the production path, specifically activateSkill and getActiveSkillNames, so refreshTools and refreshSystemPrompt are exercised with the activated skill name instead of undefined. Then tighten the assertions in the coreStream and refreshSystemPrompt expectations to verify the refreshed tool list includes the activated skill plus the new deepchat_settings_set_theme tool.src/main/presenter/agentRuntimePresenter/index.ts (1)
1114-1134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant runtime-skill merge across the two
refreshSystemPromptlayers.
runStreamForMessagealready callsgetEffectiveRuntimeSkillNames(activeSkillNames)before invoking this provided callback (Line 3083), so theactiveSkillNamesyou receive here already includes runtime-activated skills. Re-runningresolveEffectiveActiveSkillNames(activeSkillNames ?? sessionActiveSkillNames, sessionId)merges them again. It's idempotent due to dedup, so no behavioral bug, but the reassignedeffectiveActiveSkillNamesis never read after this point either. Consider passing the already-effective list straight through to avoid the double merge and dead write.🤖 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 1114 - 1134, The refreshSystemPrompt callback in agentRuntimePresenter is redundantly re-merging runtime skills even though runStreamForMessage already passes an effective active-skill list. Update the refreshSystemPrompt implementation to use the received activeSkillNames directly (or the existing session fallback only if truly needed) when calling buildSystemPromptWithSkills, and remove the unnecessary reassignment of effectiveActiveSkillNames since it is never used afterward.src/renderer/src/components/chat-input/composables/useSkillsData.ts (1)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the public
activeSkillssurface.
activeSkillsnow resolves topendingSkills, while the composable still keeps a separate session-scopedactiveSkillsref internally. That name collision makes it easy for the next caller to read the wrong list and accidentally reintroduce session-scoped behavior. Expose the composer list under a distinct name instead.Also applies to: 177-190
🤖 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/components/chat-input/composables/useSkillsData.ts` around lines 39 - 43, The composable still exposes the composer list through a surface named `activeSkills` even though it now points to `pendingSkills`, while a separate session-scoped `activeSkills` ref exists internally; rename the public/computed surface in `useSkillsData` to a distinct composer-specific name and update any returned properties or call sites that reference it so the session list and composer list cannot be confused. Make sure the change is applied consistently around `effectiveActiveSkills`, the internal `activeSkills` ref, and the related return/export shape so callers only see the intended list.
🤖 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/llmProviderPresenter/baseProvider.ts`:
- Around line 250-266: The current try/catch in baseProvider.ts is too broad and
also suppresses validation and config persistence failures after fetch. Narrow
the error handling around fetchProviderModels() only, so models.map(...) and
configPresenter.setProviderModels(...) can surface errors normally; keep
suppressErrors behavior limited to provider fetch failures while preserving the
existing this.models update flow in fetchModels().
In `@src/main/presenter/skillPresenter/skillExecutionService.ts`:
- Around line 134-138: The rejection message in skillExecutionService should
match the new message-scoped activation model instead of saying the skill is
pinned. Update the error thrown in the activeSkills check inside
skillExecutionService so it refers to the skill not being active in the current
message/tool loop, using the same wording as the skill_run and skill prompt
updates. Keep the logic that uses activeSkillNames/getActiveSkills and only
change the user-facing wording in the Error message.
---
Outside diff comments:
In `@src/renderer/src/stores/ui/message.ts`:
- Around line 216-241: The fallback user-content path in message parsing omits
activeSkills, so legacy or unparsable content can leave it undefined. Update the
fallback assignment in the message store’s parsing flow (the try/catch around
JSON.parse in the user-content builder) to always initialize activeSkills to an
empty array, matching the parsed branch and the expected message-scoped skills
shape.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 1114-1134: The refreshSystemPrompt callback in
agentRuntimePresenter is redundantly re-merging runtime skills even though
runStreamForMessage already passes an effective active-skill list. Update the
refreshSystemPrompt implementation to use the received activeSkillNames directly
(or the existing session fallback only if truly needed) when calling
buildSystemPromptWithSkills, and remove the unnecessary reassignment of
effectiveActiveSkillNames since it is never used afterward.
In `@src/main/presenter/toolPresenter/index.ts`:
- Around line 617-642: The wording in buildSkillsPrompt is awkward in the
skill_list instruction string, which can reduce clarity for model-facing
prompts. Update the text in buildSkillsPrompt so “installed skills and manual
pinned status” reads naturally, such as by using “manually pinned status” or
“manual pin status,” while keeping the meaning unchanged.
In `@src/renderer/src/components/chat-input/composables/useSkillsData.ts`:
- Around line 39-43: The composable still exposes the composer list through a
surface named `activeSkills` even though it now points to `pendingSkills`, while
a separate session-scoped `activeSkills` ref exists internally; rename the
public/computed surface in `useSkillsData` to a distinct composer-specific name
and update any returned properties or call sites that reference it so the
session list and composer list cannot be confused. Make sure the change is
applied consistently around `effectiveActiveSkills`, the internal `activeSkills`
ref, and the related return/export shape so callers only see the intended list.
In `@test/main/presenter/agentRuntimePresenter/process.test.ts`:
- Around line 502-569: The test in processStream is still using the fallback
skill list path, so it does not cover the hook-backed flow. Update the fixture
around process.test.ts and createParams/processStream setup to provide the
active-skill hooks used by the production path, specifically activateSkill and
getActiveSkillNames, so refreshTools and refreshSystemPrompt are exercised with
the activated skill name instead of undefined. Then tighten the assertions in
the coreStream and refreshSystemPrompt expectations to verify the refreshed tool
list includes the activated skill plus the new deepchat_settings_set_theme tool.
🪄 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: 8e6e2d07-e457-47c5-b7d6-91f2b7baa4e4
📒 Files selected for processing (45)
docs/issues/model-list-fetch-404/plan.mddocs/issues/model-list-fetch-404/spec.mddocs/issues/model-list-fetch-404/tasks.mddocs/issues/skill-scope-and-refresh/plan.mddocs/issues/skill-scope-and-refresh/spec.mddocs/issues/skill-scope-and-refresh/tasks.mdsrc/main/presenter/agentRuntimePresenter/contextBuilder.tssrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/messageStore.tssrc/main/presenter/agentRuntimePresenter/pendingInputCoordinator.tssrc/main/presenter/agentRuntimePresenter/pendingInputStore.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/llmProviderPresenter/baseProvider.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/src/components/chat-input/composables/useSkillsData.tssrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/message.tssrc/shared/contracts/common.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat.tssrc/shared/types/presenters/tool.presenter.d.tssrc/shared/types/skill.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/agentRuntimePresenter/messageStore.test.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/agentSessionPresenter/agentSessionPresenter.test.tstest/main/presenter/llmProviderPresenter/baseProvider.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/components/message/MessageItemUser.test.ts
Contributor
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)
src/renderer/src/components/chat-input/composables/useSkillsData.ts (1)
29-43: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse a shared composer skill state instead of per-call local state.
pendingSkillsis local to eachuseSkillsData(...)invocation. SinceSkillsIndicator.vueandChatInputBox.vueboth instantiate this composable, panel toggles and slash-activated chips/consume/clear paths can operate on different arrays. Move this composer state to one shared source, or pass/provide the same ref to both components.🤖 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/components/chat-input/composables/useSkillsData.ts` around lines 29 - 43, The composer skill state is currently split because `useSkillsData()` creates its own `pendingSkills` ref per call, so `SkillsIndicator.vue` and `ChatInputBox.vue` can read/write different arrays. Move this state to a shared source used by the composable, or accept/provide a shared `Ref<string[]>` into `useSkillsData` so both components operate on the same composer skills. Keep `composerActiveSkills` and all consume/clear/toggle paths wired to that shared ref.
🤖 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 `@src/renderer/src/components/chat-input/composables/useSkillsData.ts`:
- Around line 29-43: The composer skill state is currently split because
`useSkillsData()` creates its own `pendingSkills` ref per call, so
`SkillsIndicator.vue` and `ChatInputBox.vue` can read/write different arrays.
Move this state to a shared source used by the composable, or accept/provide a
shared `Ref<string[]>` into `useSkillsData` so both components operate on the
same composer skills. Keep `composerActiveSkills` and all consume/clear/toggle
paths wired to that shared ref.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed425b62-c05c-4c94-8727-26987388b333
📒 Files selected for processing (12)
docs/issues/model-list-fetch-404/tasks.mddocs/issues/skill-scope-and-refresh/tasks.mdsrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/llmProviderPresenter/baseProvider.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/src/components/chat-input/SkillsIndicator.vuesrc/renderer/src/components/chat-input/composables/useSkillsData.tssrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/stores/ui/message.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/llmProviderPresenter/baseProvider.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/issues/model-list-fetch-404/tasks.md
- docs/issues/skill-scope-and-refresh/tasks.md
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/src/stores/ui/message.ts
- src/main/presenter/skillPresenter/skillExecutionService.ts
- test/main/presenter/agentRuntimePresenter/process.test.ts
- test/main/presenter/llmProviderPresenter/baseProvider.test.ts
- src/main/presenter/toolPresenter/index.ts
- src/main/presenter/llmProviderPresenter/baseProvider.ts
- src/main/presenter/agentRuntimePresenter/index.ts
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
Validation
Summary by CodeRabbit
Summary by CodeRabbit