Conversation
Add a safe, validated API for modifying DeepChat application settings via natural language. Settings changes are controlled by a dedicated 'deepchat-settings' skill to ensure tools are only available when contextually relevant. Key features: - Skill-gated tool injection: settings tools only appear when deepchat-settings skill is active - Safe settings apply API with Zod validation and strict allowlist - Support for toggles (sound, copy COT, chat mode) and enums (language, theme, font size) - Defense-in-depth: runtime skill verification before applying changes - Deep-linked settings navigation for unsupported/complex settings - Comprehensive test coverage for validation, mapping, and skill gating Changes: - Add ChatSettingsToolHandler with validated apply/open methods - Integrate with AgentToolManager for tool definition gating - Create deepchat-settings built-in skill with clear activation rules - Add shared types for requests/responses (chatSettings.ts) - Implement settings window navigation with SECTION_ALIASES - Add unit tests for handler and integration tests for tool gating - Translate spec documents (plan.md, spec.md, tasks.md) to Chinese - Fix type errors in getCurrentValue and OPEN_SECTION_VALUES
…rvice Remove chatMode setting from the allowlist as it requires conversation-scoped updates that are better handled separately. Add permission checking for settings window opening to provide user control over settings navigation. Key changes: - Remove setChatMode tool and related schemas from ChatSettingsToolHandler - Add SettingsPermissionService for managing tool approvals (one-time and session) - Add permission check for deepchat_settings_open tool - Update PermissionHandler to handle settings permission grants - Add rememberable flag to permission request structure - Update AgentToolManager to consume approvals before opening settings - Add settingsPermissionService to main presenter index - Clear settings approvals when conversation ends - Update spec documents to reflect removed chatMode feature - Remove chatMode-related tests and types This ensures settings window opening requires explicit user approval and provides a cleaner separation of concerns for chat mode management.
📝 WalkthroughWalkthroughAdds conversational chat-settings control: new DeepChat skill (deepchat-settings), tool definitions/handler, permission service, types, presenter plumbing to propagate conversationId, UI event for navigating settings, and tests. Validated main-process API, skill gating, and session-scoped approvals are implemented. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent
participant ToolHandler as ChatSettingsToolHandler
participant Config as ConfigPresenter
participant Perm as SettingsPermissionService
participant UI as SettingsUI
User->>Agent: Ask to change setting (e.g., enable sound)
Agent->>ToolHandler: callChatSettingsTool(toggle, payload)
ToolHandler->>ToolHandler: Validate skill active & input
ToolHandler->>Config: apply setting change
Config-->>ToolHandler: applied (previousValue, timestamp)
ToolHandler-->>Agent: success result
Agent->>User: Confirm applied
alt Open settings flow
User->>Agent: Ask to open settings (section)
Agent->>ToolHandler: callChatSettingsTool(open, {section})
ToolHandler->>Perm: consumeApproval(conversationId, toolName)
Perm-->>ToolHandler: approved / not approved
alt approved
ToolHandler->>UI: send NAVIGATE event with route
UI-->>User: Settings opened at section
else not approved
ToolHandler-->>Agent: return permissionRequest
Agent->>User: Request approval
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/presenter/agentPresenter/acp/agentToolManager.ts`:
- Around line 300-325: The fallback that merges requiredSettingsTools into
allowedTools when hasNonOpenSettingsTool is false (calculating
effectiveAllowedTools) needs an explicit comment explaining its intent or else
change to respect allowedTools as-is; update the block around the
hasNonOpenSettingsTool check (variables/functions: allowedTools,
requiredSettingsTools, CHAT_SETTINGS_TOOL_NAMES, effectiveAllowedTools,
buildChatSettingsToolDefinitions,
presenter.skillPresenter.getActiveSkillsAllowedTools) to either add a brief
comment describing why we add all requiredSettingsTools when only "open" is
allowed (e.g., to ensure UI/settings completeness) or remove the merge and use
allowedTools directly so skill metadata is never expanded silently. Ensure the
comment clearly states the design choice and any security/UX rationale.
🧹 Nitpick comments (6)
test/main/presenter/agentPresenter/agentToolManagerSettings.test.ts (1)
1-83: LGTM!The test file provides good coverage of the DeepChat settings tool gating behavior in
AgentToolManager. The two test cases effectively verify:
- Tools are excluded when the skill is inactive
- Only explicitly allowed tools are included when the skill is active
Consider adding additional test cases for broader coverage:
- When all settings tools are in the allowed list
- When
chatModeis not'agent'(tools should not be included)- When
conversationIdis not provided💡 Example additional test case
it('does not include settings tools when chatMode is not agent', async () => { ;(presenter.skillPresenter.getActiveSkills as any).mockResolvedValue([CHAT_SETTINGS_SKILL_NAME]) ;(presenter.skillPresenter.getActiveSkillsAllowedTools as any).mockResolvedValue([ CHAT_SETTINGS_TOOL_NAMES.toggle ]) const manager = new AgentToolManager({ yoBrowserPresenter, agentWorkspacePath: null, configPresenter }) const defs = await manager.getAllToolDefinitions({ chatMode: 'chat', supportsVision: false, agentWorkspacePath: null, conversationId: 'conv-1' }) const names = defs.map((def) => def.function.name) expect(names).not.toContain(CHAT_SETTINGS_TOOL_NAMES.toggle) })src/main/presenter/mcpPresenter/index.ts (1)
648-659: Normalizerequiredto an array to keep schemas valid.Defensive defaults look good; consider guarding against non-array
requiredfrom malformed tool definitions to avoid downstream schema validation issues.♻️ Suggested guard for non-array
required- required: toolParameters?.required ?? [] + required: Array.isArray(toolParameters?.required) ? toolParameters?.required : []Also applies to: 677-677
docs/specs/chat-settings-control/plan.md (1)
96-96: Minor grammar nit in Chinese text.Per static analysis, the adverb form should use "地" instead of "的" when modifying a verb: "精确地说明" instead of "精确的说明".
📝 Suggested fix
-- 提供精确的说明告诉用户在哪里更改。 +- 提供精确地说明告诉用户在哪里更改。src/main/presenter/agentPresenter/acp/chatSettingsTools.ts (1)
349-358: Redundant guard check inopenmethod.The
guardreturned fromensureSkillActiveis alwaysnull(skill active) or an object withok: false. The checkguard && !guard.okis redundant since ifguardis truthy, it will always haveok: false.♻️ Simplify guard check
async open(raw: unknown, conversationId?: string): Promise<OpenChatSettingsResult> { const guard = await this.ensureSkillActive(conversationId) - if (guard && !guard.ok) { + if (guard) { return { ok: false, errorCode: 'skill_inactive', message: guard.message, details: guard.details } }src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
960-968: Consider usingObject.valuesfor maintainability.The
isChatSettingsToolcheck explicitly lists all tool names. Using the constant ensures consistency.♻️ Use CHAT_SETTINGS_TOOL_NAMES for consistency
private isChatSettingsTool(toolName: string): boolean { - return ( - toolName === CHAT_SETTINGS_TOOL_NAMES.toggle || - toolName === CHAT_SETTINGS_TOOL_NAMES.setLanguage || - toolName === CHAT_SETTINGS_TOOL_NAMES.setTheme || - toolName === CHAT_SETTINGS_TOOL_NAMES.setFontSize || - toolName === CHAT_SETTINGS_TOOL_NAMES.open - ) + return Object.values(CHAT_SETTINGS_TOOL_NAMES).includes(toolName as any) }src/shared/types/chatSettings.ts (1)
40-42: Consider constraininglevelinSetFontSizeRequest.The implementation in
chatSettingsTools.tsvalidateslevelas one of[0, 1, 2, 3, 4], but this type uses a genericnumber. For stronger type safety, consider using a literal union.♻️ Constrain font size level type
+export type FontSizeLevel = 0 | 1 | 2 | 3 | 4 + export type SetFontSizeRequest = { - level: number + level: FontSizeLevel }
| // 4. DeepChat settings tools (agent mode only, skill gated) | ||
| if (isAgentMode && this.isSkillsEnabled() && context.conversationId) { | ||
| try { | ||
| const activeSkills = await presenter.skillPresenter.getActiveSkills(context.conversationId) | ||
| if (activeSkills.includes(CHAT_SETTINGS_SKILL_NAME)) { | ||
| const allowedTools = await presenter.skillPresenter.getActiveSkillsAllowedTools( | ||
| context.conversationId | ||
| ) | ||
| const requiredSettingsTools = Object.values(CHAT_SETTINGS_TOOL_NAMES) | ||
| const nonOpenSettingsTools = requiredSettingsTools.filter( | ||
| (tool) => tool !== CHAT_SETTINGS_TOOL_NAMES.open | ||
| ) | ||
| const hasNonOpenSettingsTool = nonOpenSettingsTools.some((tool) => | ||
| allowedTools.includes(tool) | ||
| ) | ||
| const effectiveAllowedTools = hasNonOpenSettingsTool | ||
| ? allowedTools | ||
| : Array.from(new Set([...allowedTools, ...requiredSettingsTools])) | ||
|
|
||
| const settingsDefs = buildChatSettingsToolDefinitions(effectiveAllowedTools) | ||
| defs.push(...settingsDefs) | ||
| } | ||
| } catch (error) { | ||
| logger.warn('[AgentToolManager] Failed to load DeepChat settings tools', { error }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the skill definition file
fd -t f "SKILL.md" -p "deepchat-settings" 2>/dev/null || echo "SKILL.md not found in deepchat-settings, searching more broadly..."
find . -type f -name "*SKILL*" -o -name "skill*" | grep -i "deepchat\|settings" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Let's examine the agentToolManager.ts file around lines 300-325 and the logic context
cat -n src/main/presenter/agentPresenter/acp/agentToolManager.ts | sed -n '300,330p'Repository: ThinkInAIXYZ/deepchat
Length of output: 1547
🏁 Script executed:
#!/bin/bash
# Also check for any constants defining CHAT_SETTINGS_TOOL_NAMES
grep -n "CHAT_SETTINGS_TOOL_NAMES" src/main/presenter/agentPresenter/acp/agentToolManager.ts | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 855
🏁 Script executed:
#!/bin/bash
# Search for where CHAT_SETTINGS_TOOL_NAMES is defined
rg "CHAT_SETTINGS_TOOL_NAMES\s*=" -A 10 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 1212
🏁 Script executed:
#!/bin/bash
# Look for skill definitions and deepchat-settings configuration
find . -type d -name "*skill*" -o -name "*deepchat*" 2>/dev/null | grep -E "skill|deepchat" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 408
🏁 Script executed:
#!/bin/bash
cat -n ./resources/skills/deepchat-settings/SKILL.mdRepository: ThinkInAIXYZ/deepchat
Length of output: 3494
🏁 Script executed:
#!/bin/bash
# Also check if there are any comments or documentation about the fallback logic
grep -B 5 -A 5 "allowedTools\|requiredSettingsTools" src/main/presenter/agentPresenter/acp/agentToolManager.ts | head -40Repository: ThinkInAIXYZ/deepchat
Length of output: 1196
🏁 Script executed:
#!/bin/bash
# Check the skillPresenter to understand how getActiveSkillsAllowedTools works
rg "getActiveSkillsAllowedTools" -A 15 src/main/presenter/skillPresenter/Repository: ThinkInAIXYZ/deepchat
Length of output: 1292
🏁 Script executed:
#!/bin/bash
# Search for comments around the fallback logic in the file
grep -B 10 "effectiveAllowedTools" src/main/presenter/agentPresenter/acp/agentToolManager.ts | head -30Repository: ThinkInAIXYZ/deepchat
Length of output: 810
🏁 Script executed:
#!/bin/bash
# Check git blame/history if available to see when this was added
git log -p --all -S "effectiveAllowedTools" -- src/main/presenter/agentPresenter/acp/agentToolManager.ts 2>/dev/null | head -100 || echo "Git history not available"Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about skill tool restrictions
rg "allowedTools.*restrict\|restrict.*allowedTools\|fallback.*tools" -iRepository: ThinkInAIXYZ/deepchat
Length of output: 47
The fallback logic lacks documentation and should clarify its intent.
Lines 315-317 silently add all requiredSettingsTools if no non-open tools appear in allowedTools. With the current SKILL.md listing all 5 tools, this fallback never executes. However, if the skill metadata were restricted (e.g., to only allow deepchat_settings_open), the code would bypass that restriction without explanation.
Either document why this fallback exists or reconsider whether allowedTools should be respected as-is. Add a comment explaining the design intent behind this logic, as it's not self-evident from the code.
🤖 Prompt for AI Agents
In `@src/main/presenter/agentPresenter/acp/agentToolManager.ts` around lines 300 -
325, The fallback that merges requiredSettingsTools into allowedTools when
hasNonOpenSettingsTool is false (calculating effectiveAllowedTools) needs an
explicit comment explaining its intent or else change to respect allowedTools
as-is; update the block around the hasNonOpenSettingsTool check
(variables/functions: allowedTools, requiredSettingsTools,
CHAT_SETTINGS_TOOL_NAMES, effectiveAllowedTools,
buildChatSettingsToolDefinitions,
presenter.skillPresenter.getActiveSkillsAllowedTools) to either add a brief
comment describing why we add all requiredSettingsTools when only "open" is
allowed (e.g., to ensure UI/settings completeness) or remove the merge and use
allowedTools directly so skill metadata is never expanded silently. Ensure the
comment clearly states the design choice and any security/UX rationale.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/specs/chat-settings-control/spec.md`:
- Line 20: The line "Use skills to control context:" contains a typographical
error with a double backtick after the inline code token for
`deepchat-settings`; edit the markdown in spec.md to replace the double backtick
sequence following "deepchat-settings" with a single backtick so the inline code
is correctly formatted (locate the occurrence of `deepchat-settings`` and change
it to `deepchat-settings`).
In `@docs/specs/chat-settings-control/tasks.md`:
- Line 21: Replace the unclear abbreviation "best-eff-mn navigation" in the
tasks.md text with a spelled-out phrase; locate the string "best-eff-mn
navigation" and change it to "best-effort navigation" (or "best-effort
navigation (best-eff-mn)" if you want to preserve the shorthand), updating any
related wording nearby such as the sentence "Add 'open settings' helper/tool for
unsupported settings (MCP/prompts, etc.), including best-eff-mn navigation" to
the clarified version.
- Line 20: Fix the stray double backtick typo around the identifier
"deepchat-settings" in the tasks text (the line that currently reads
`deepchat-settings``) by replacing the trailing double backtick with a single
backtick so it appears as `deepchat-settings`; also mirror the same correction
in spec.md where the same double-backtick issue occurs.
* chore: release v0.5.6-beta.5 * chore: typecheck with tsgo (#1278) * feat: image left align * feat: app settings skills (#1283) * docs: add spec for app-settings * feat: implement chat-driven settings control with skill gating Add a safe, validated API for modifying DeepChat application settings via natural language. Settings changes are controlled by a dedicated 'deepchat-settings' skill to ensure tools are only available when contextually relevant. Key features: - Skill-gated tool injection: settings tools only appear when deepchat-settings skill is active - Safe settings apply API with Zod validation and strict allowlist - Support for toggles (sound, copy COT, chat mode) and enums (language, theme, font size) - Defense-in-depth: runtime skill verification before applying changes - Deep-linked settings navigation for unsupported/complex settings - Comprehensive test coverage for validation, mapping, and skill gating Changes: - Add ChatSettingsToolHandler with validated apply/open methods - Integrate with AgentToolManager for tool definition gating - Create deepchat-settings built-in skill with clear activation rules - Add shared types for requests/responses (chatSettings.ts) - Implement settings window navigation with SECTION_ALIASES - Add unit tests for handler and integration tests for tool gating - Translate spec documents (plan.md, spec.md, tasks.md) to Chinese - Fix type errors in getCurrentValue and OPEN_SECTION_VALUES * refactor: remove chatMode from settings control and add permission service Remove chatMode setting from the allowlist as it requires conversation-scoped updates that are better handled separately. Add permission checking for settings window opening to provide user control over settings navigation. Key changes: - Remove setChatMode tool and related schemas from ChatSettingsToolHandler - Add SettingsPermissionService for managing tool approvals (one-time and session) - Add permission check for deepchat_settings_open tool - Update PermissionHandler to handle settings permission grants - Add rememberable flag to permission request structure - Update AgentToolManager to consume approvals before opening settings - Add settingsPermissionService to main presenter index - Clear settings approvals when conversation ends - Update spec documents to reflect removed chatMode feature - Remove chatMode-related tests and types This ensures settings window opening requires explicit user approval and provides a cleaner separation of concerns for chat mode management. * docs: translate to en * feat: voice ai text to speech * feat: voice ai call phone * Initial plan * perf: optimize message cache and cloning operations - Add reverse index for O(1) thread lookup in messageRuntimeCache - Extract duplicate cloneMessageWithContent to shared utility - Replace JSON.parse/stringify with structuredClone - Filter before cloning in messageTruncator to avoid wasted work Co-authored-by: zhangmo8 <43628500+zhangmo8@users.noreply.github.com> * docs: add comprehensive performance optimization analysis Add detailed optimization analysis document covering: - Implemented optimizations (cache, cloning, filtering) - High priority opportunities (provider consolidation, IPC batching) - Medium/low priority improvements (DB indices, token caching) - Performance metrics and recommendations Co-authored-by: zhangmo8 <43628500+zhangmo8@users.noreply.github.com> * docs: add Chinese optimization summary Add user-friendly Chinese summary of optimization work Co-authored-by: zhangmo8 <43628500+zhangmo8@users.noreply.github.com> --------- Co-authored-by: zerob13 <zerob13@gmail.com> Co-authored-by: xiaomo <wegi866@gmail.com> Co-authored-by: wanna <wanna.w@binarywalk.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: zhangmo8 <43628500+zhangmo8@users.noreply.github.com>
* chore: typecheck with tsgo (#1278) * feat: image left align * feat: app settings skills (#1283) * docs: add spec for app-settings * feat: implement chat-driven settings control with skill gating Add a safe, validated API for modifying DeepChat application settings via natural language. Settings changes are controlled by a dedicated 'deepchat-settings' skill to ensure tools are only available when contextually relevant. Key features: - Skill-gated tool injection: settings tools only appear when deepchat-settings skill is active - Safe settings apply API with Zod validation and strict allowlist - Support for toggles (sound, copy COT, chat mode) and enums (language, theme, font size) - Defense-in-depth: runtime skill verification before applying changes - Deep-linked settings navigation for unsupported/complex settings - Comprehensive test coverage for validation, mapping, and skill gating Changes: - Add ChatSettingsToolHandler with validated apply/open methods - Integrate with AgentToolManager for tool definition gating - Create deepchat-settings built-in skill with clear activation rules - Add shared types for requests/responses (chatSettings.ts) - Implement settings window navigation with SECTION_ALIASES - Add unit tests for handler and integration tests for tool gating - Translate spec documents (plan.md, spec.md, tasks.md) to Chinese - Fix type errors in getCurrentValue and OPEN_SECTION_VALUES * refactor: remove chatMode from settings control and add permission service Remove chatMode setting from the allowlist as it requires conversation-scoped updates that are better handled separately. Add permission checking for settings window opening to provide user control over settings navigation. Key changes: - Remove setChatMode tool and related schemas from ChatSettingsToolHandler - Add SettingsPermissionService for managing tool approvals (one-time and session) - Add permission check for deepchat_settings_open tool - Update PermissionHandler to handle settings permission grants - Add rememberable flag to permission request structure - Update AgentToolManager to consume approvals before opening settings - Add settingsPermissionService to main presenter index - Clear settings approvals when conversation ends - Update spec documents to reflect removed chatMode feature - Remove chatMode-related tests and types This ensures settings window opening requires explicit user approval and provides a cleaner separation of concerns for chat mode management. * docs: translate to en * chore: yo browser less context (#1284) * docs: add specs for yo browesr context manager * feat(yo-browser): improve skill description and add yo_browser_cdp_send to offload whitelist * refactor(yobrowser): remove skill gating and make CDP tools always available in agent mode * refactor(yobrowser): add CDP method schema validation with strict enums - Add enum-based validation for cdp_send method (11 common CDP methods) - Add detailed union schemas for each method's parameters with examples - Add normalizeCdpParams method to handle both object and JSON string inputs - Prevent method typos and provide better type safety for CDP interactions * fix(yobrowser): add strict tab ID validation in CDP send handler * chore: update deps * refactor: agent provider simplification (#1286) * docs: add spec for agent provider * refactor(agent): remove BaseAgentProvider layer and simplify provider hierarchy * feat(mcp): implement real Apple Maps search using URL scheme (#1289) * feat: support voice.ai (#1291) * feat: remove custome tiptap (#1295) * feat: settings auto scroll toggle (#1293) * feat: settings auto scroll toggle * feat: i18n support * fix(renderer): remove specific event listeners instead of all * feat: add tooltip for filling default API URL in settings (#1296) * fix: fix model list refresh failed (#1297) * feat: add question tools (#1298) * feat(agent): add question tool flow * feat(mcp): implement real Apple Maps search using URL scheme (#1289) * feat: support voice.ai (#1291) * feat: remove custome tiptap (#1295) * feat: settings auto scroll toggle (#1293) * feat: settings auto scroll toggle * feat: i18n support * fix(renderer): remove specific event listeners instead of all * feat: add tooltip for filling default API URL in settings (#1296) * refactor(question): simplify question request UI to single-choice interface * fix(chat): restore pending question state * fix: review issues --------- Co-authored-by: Qi Jin <jin.qi1@northeastern.edu> Co-authored-by: xiaomo <wegi866@gmail.com> * chore: integrated vue-tsgo (#1299) * chore: integrated vue-tsgo * chore: update recommendation exteion * chore: update * feat: add prompt for ask tool * chore: version to 0.5.7 --------- Co-authored-by: xiaomo <wegi866@gmail.com> Co-authored-by: wanna <wanna.w@binarywalk.com> Co-authored-by: Qi Jin <jin.qi1@northeastern.edu>
finish #1275
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.