fix: window and toolcall bug #1252
Conversation
📝 WalkthroughWalkthroughAdds a function-calling toggle in post-tool message construction, changes message sequencing for function-call vs fallback flows, tightens tab/window activation and local-tab handling, adjusts chat tab deduplication, and introduces per-conversation/baseDirectory workdir handling while changing several agent filesystem/command tool behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Ctx as PostToolExecutionContext
participant Builder as MessageBuilder
participant Tool as ToolExecutor
participant Messages as MessageQueue
User->>Ctx: tool execution completes (result + metadata)
Ctx->>Builder: buildPostToolExecutionContext(ctx)
Builder->>Builder: check supportsFunctionCall flag
alt supportsFunctionCall == true
rect rgb(200,220,255)
Note over Builder,Messages: Native function-calling flow
Builder->>Builder: ensure/generate tool_call_id (nanoid if missing)
Builder->>Messages: push assistant msg with tool_calls (name + args)
Builder->>Messages: push tool msg with tool response (tool_call_id)
Builder->>Tool: (tool response already present) finalize sequence
end
else supportsFunctionCall == false
rect rgb(255,230,200)
Note over Builder,Messages: Legacy serialized fallback
Builder->>Builder: serialize function_call_record into assistant content
Builder->>Messages: push assistant msg (embedded serialized record)
Builder->>Messages: push user prompt msg to continue flow
end
end
Builder->>Ctx: return updated message sequence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/main/presenter/agentPresenter/messageBuilder.test.ts (1)
1-86: LGTM! Comprehensive test coverage for the new function-calling flows.The test suite effectively covers:
- OpenAI-compatible flow with explicit tool call ID
- Stable ID generation when upstream ID is missing
- Fallback path for models without function-calling support
The mock strategy and dynamic imports correctly isolate the unit under test from Electron dependencies.
Consider extracting the dynamic import to a
beforeEachblock to reduce repetition:🔎 Optional refactor for DRYer test setup
describe('messageBuilder.buildPostToolExecutionContext', () => { + let buildPostToolExecutionContext: typeof import('@/presenter/agentPresenter/message/messageBuilder')['buildPostToolExecutionContext'] + + beforeEach(async () => { + const module = await import('@/presenter/agentPresenter/message/messageBuilder') + buildPostToolExecutionContext = module.buildPostToolExecutionContext + }) + it('emits assistant(tool_calls) -> tool(tool_call_id) pairing when functionCall is enabled', async () => { - const { buildPostToolExecutionContext } = - await import('@/presenter/agentPresenter/message/messageBuilder') - const messages = await buildPostToolExecutionContext({ // ... })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.tssrc/main/presenter/windowPresenter/index.tssrc/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.ts
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments in TypeScript/JavaScript code
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict type checking enabled
Use OxLint for linting JavaScript and TypeScript files; ensure lint-staged hooks and typecheck pass before commits
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
**/*.{js,ts,tsx,jsx,vue,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
All logs and comments must be in English
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use OxLint as the linter
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
**/*.{js,ts,tsx,jsx,vue,json,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use Prettier as the code formatter
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
src/renderer/shell/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Shell UI code should be located in
src/renderer/shell/
Files:
src/renderer/shell/stores/tab.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Use camelCase for variable and function names; use PascalCase for types and classes; use SCREAMING_SNAKE_CASE for constants
Configure Prettier with single quotes, no semicolons, and line width of 100 characters. Runpnpm run formatafter completing features
Files:
src/renderer/shell/stores/tab.tstest/main/presenter/agentPresenter/messageBuilder.test.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
test/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files in
test/directory with corresponding structure to source files
Files:
test/main/presenter/agentPresenter/messageBuilder.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest as the testing framework for unit and integration tests
Files:
test/main/presenter/agentPresenter/messageBuilder.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Vitest test suites should be organized in
test/main/**andtest/renderer/**mirroring source structure, with file names following*.test.tsor*.spec.tspattern
Files:
test/main/presenter/agentPresenter/messageBuilder.test.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/presenter/**/*.ts: Use EventBus to broadcast events from main to renderer viamainWindow.webContents.send()
Implement one presenter per functional domain in the main process
Files:
src/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/**/*.ts: Use EventBus fromsrc/main/eventbus.tsfor decoupled inter-process communication
Context isolation must be enabled with preload scripts for secure IPC communicationElectron main process code should reside in
src/main/, with presenters organized inpresenter/subdirectory (Window, Tab, Thread, Mcp, Config, LLMProvider), and app events managed viaeventbus.ts
Files:
src/main/presenter/windowPresenter/index.tssrc/main/presenter/agentPresenter/message/messageBuilder.tssrc/main/presenter/tabPresenter.ts
🧠 Learnings (3)
📚 Learning: 2025-06-21T15:48:29.950Z
Learnt from: neoragex2002
Repo: ThinkInAIXYZ/deepchat PR: 550
File: src/main/presenter/mcpPresenter/inMemoryServers/meetingServer.ts:250-252
Timestamp: 2025-06-21T15:48:29.950Z
Learning: In the meeting server implementation (src/main/presenter/mcpPresenter/inMemoryServers/meetingServer.ts), when multiple tabs have the same title, the user prefers to let the code silently select the first match without adding warnings or additional ambiguity handling.
Applied to files:
src/renderer/shell/stores/tab.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/tabPresenter.ts
📚 Learning: 2025-08-28T05:55:31.482Z
Learnt from: zerob13
Repo: ThinkInAIXYZ/deepchat PR: 804
File: src/main/presenter/llmProviderPresenter/providers/tokenfluxProvider.ts:153-156
Timestamp: 2025-08-28T05:55:31.482Z
Learning: TokenFlux models generally support function calling by default, so it's reasonable to assume hasFunctionCalling = true for TokenFlux provider implementations in src/main/presenter/llmProviderPresenter/providers/tokenfluxProvider.ts
Applied to files:
src/main/presenter/agentPresenter/message/messageBuilder.ts
📚 Learning: 2026-01-05T02:40:52.831Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T02:40:52.831Z
Learning: Applies to src/main/presenter/configPresenter/**/*.ts : Custom prompts are managed independently of MCP through config data source using `configPresenter.getCustomPrompts()`
Applied to files:
src/main/presenter/agentPresenter/message/messageBuilder.ts
🧬 Code graph analysis (2)
test/main/presenter/agentPresenter/messageBuilder.test.ts (1)
src/main/presenter/agentPresenter/message/messageBuilder.ts (1)
buildPostToolExecutionContext(238-326)
src/main/presenter/tabPresenter.ts (3)
test/mocks/electron.ts (1)
BrowserWindow(36-51)src/main/events.ts (1)
TAB_EVENTS(180-188)src/renderer/src/events.ts (1)
TAB_EVENTS(145-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (6)
src/renderer/shell/stores/tab.ts (1)
14-21: LGTM! Clear deduplication logic for tab types.The guard correctly allows multiple
chatandplaygroundtabs while deduplicating other local view types (e.g., settings). This makes sense for user experience—users may want multiple chat sessions open simultaneously.src/main/presenter/agentPresenter/message/messageBuilder.ts (3)
13-13: LGTM! Appropriate use of nanoid for stable ID generation.Using
nanoidwith a fixed length (8) is a good choice for generating unique tool call IDs when the upstream doesn't provide one.
275-298: LGTM! OpenAI-compatible function-calling flow is well-structured.The assistant → tool message pairing with matching
tool_call_idcorrectly follows the OpenAI function-calling protocol. The fallback ID generation withnanoid(8)ensures robustness when providers don't return an ID.
317-322: Verify: Chinese prompt in fallback flow may need internationalization.The user prompt
'以上是你刚执行的工具调用及其响应信息...'is in Chinese. Per coding guidelines, comments should be in English. However, this is a user-facing LLM prompt, not a log or code comment.Please verify:
- Is this intentional for specific Chinese LLM providers (e.g., DeepSeek)?
- Should this prompt be internationalized based on user locale or model configuration?
src/main/presenter/windowPresenter/index.ts (1)
1031-1057: LGTM! Correctly prevents duplicate tab activation for moved tabs.The condition
!options?.forMovedTabproperly defers activation to theattachTabflow when a tab is dragged to a new window. This prevents the race condition where bothcreateShellWindowandattachTabwould attempt to activate the same tab.The inline comment clearly documents the reasoning behind this change.
src/main/presenter/tabPresenter.ts (1)
696-717: LGTM! Correctly preserves local:// tab state during navigation.The guard prevents
did-navigateevents from overwritinglocal://URLs with internal renderer URLs (e.g., hash-based routes). This is the correct fix because:
- Local tabs use
local://as a logical URL scheme, not a real navigable URL- Internal navigation within the renderer shouldn't modify the tab's identity
- Title updates for local tabs are still handled by the
page-title-updatedevent handler, which updates state.title unconditionally for all tabs including local ones
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
536-548: Redundant try-catch block.The
getWorkdirForConversationmethod already has internal error handling that returnsnullon failure. The outer try-catch is redundant.🔎 Proposed simplification
let dynamicWorkdir: string | null = null if (conversationId) { - try { - dynamicWorkdir = await this.getWorkdirForConversation(conversationId) - } catch (error) { - logger.warn('[AgentToolManager] Failed to get workdir for conversation:', { - conversationId, - error - }) - } + dynamicWorkdir = await this.getWorkdirForConversation(conversationId) } const baseDirectory = dynamicWorkdir ?? undefined
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments in TypeScript/JavaScript code
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict type checking enabled
Use OxLint for linting JavaScript and TypeScript files; ensure lint-staged hooks and typecheck pass before commits
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/presenter/**/*.ts: Use EventBus to broadcast events from main to renderer viamainWindow.webContents.send()
Implement one presenter per functional domain in the main process
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/**/*.ts: Use EventBus fromsrc/main/eventbus.tsfor decoupled inter-process communication
Context isolation must be enabled with preload scripts for secure IPC communicationElectron main process code should reside in
src/main/, with presenters organized inpresenter/subdirectory (Window, Tab, Thread, Mcp, Config, LLMProvider), and app events managed viaeventbus.ts
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
**/*.{js,ts,tsx,jsx,vue,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
All logs and comments must be in English
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use OxLint as the linter
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
**/*.{js,ts,tsx,jsx,vue,json,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use Prettier as the code formatter
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Use camelCase for variable and function names; use PascalCase for types and classes; use SCREAMING_SNAKE_CASE for constants
Configure Prettier with single quotes, no semicolons, and line width of 100 characters. Runpnpm run formatafter completing features
Files:
src/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts
🧠 Learnings (5)
📚 Learning: 2026-01-05T02:41:45.204Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T02:41:45.204Z
Learning: Applies to src/main/**/*.ts : Electron main process code should reside in `src/main/`, with presenters organized in `presenter/` subdirectory (Window, Tab, Thread, Mcp, Config, LLMProvider), and app events managed via `eventbus.ts`
Applied to files:
src/main/presenter/agentPresenter/acp/agentToolManager.ts
📚 Learning: 2026-01-05T02:40:52.831Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T02:40:52.831Z
Learning: Applies to src/renderer/src/**/*.{ts,tsx,vue} : Use `usePresenter.ts` composable for renderer-to-main IPC communication via direct presenter method calls
Applied to files:
src/main/presenter/agentPresenter/acp/agentToolManager.ts
📚 Learning: 2026-01-05T02:41:31.619Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: .cursor/rules/vue-stack-guide.mdc:0-0
Timestamp: 2026-01-05T02:41:31.619Z
Learning: Applies to src/renderer/src/**/*.{vue,ts,tsx} : Use `usePresenter` composable for main process communication instead of direct IPC calls
Applied to files:
src/main/presenter/agentPresenter/acp/agentToolManager.ts
📚 Learning: 2026-01-05T02:40:52.831Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T02:40:52.831Z
Learning: Applies to src/main/presenter/**/*.ts : Implement one presenter per functional domain in the main process
Applied to files:
src/main/presenter/agentPresenter/acp/agentToolManager.ts
📚 Learning: 2026-01-05T02:40:52.831Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T02:40:52.831Z
Learning: Applies to src/main/presenter/**/*.ts : Use EventBus to broadcast events from main to renderer via `mainWindow.webContents.send()`
Applied to files:
src/main/presenter/agentPresenter/acp/agentToolManager.ts
🧬 Code graph analysis (1)
src/main/presenter/agentPresenter/acp/agentToolManager.ts (2)
test/mocks/electron.ts (1)
session(34-34)src/main/presenter/index.ts (1)
presenter(297-297)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (12)
src/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts (6)
167-201: LGTM -baseDirectoryintegration in path validation.The
validatePathmethod correctly resolves relative paths against the providedbaseDirectory, falling back to the first allowed directory when not specified. The existing security checks (allowed directory validation, symlink resolution, parent directory verification) remain intact.
634-652: LGTM - ConsistentbaseDirectorypropagation.The
readFile,writeFile,listDirectory, andcreateDirectorymethods correctly propagate the optionalbaseDirectorytovalidatePath, maintaining a consistent pattern throughout.Also applies to: 654-662, 664-678, 680-688
690-711: LGTM -moveFilescorrectly appliesbaseDirectoryto both source and destination.Both source paths and the constructed destination path are resolved against the same
baseDirectory, ensuring consistent path scoping.
816-846: LGTM - Recursive tree building with proper path validation.The
buildTreeinner function correctly capturesbaseDirectoryfrom the closure and validates each path in the recursive traversal.
861-873: LGTM -globSearchroot resolution handles all cases.The search root is correctly resolved:
- If
rootis provided, it's validated withbaseDirectory- Otherwise,
baseDirectoryis used (or falls back toallowedDirectories[0])The subsequent
validatePathcalls at line 901 work correctly since glob results are absolute paths (absolute: truein options).
713-749: LGTM - Remaining methods follow consistent pattern.The
editText,grepSearch,textReplace, andgetFileInfomethods all correctly accept and propagatebaseDirectoryto path validation.Also applies to: 751-793, 795-814, 848-859
src/main/presenter/agentPresenter/acp/agentToolManager.ts (3)
254-281: LGTM - Per-conversation workdir resolution.The
getWorkdirForConversationmethod correctly retrieves the working directory based on the conversation's chat mode:
- For
acp agent: looks up workdir fromacpWorkdirMapusing the model ID- For
agent: returnsagentWorkspacePathError handling with null fallback is appropriate.
576-582: Verify:execute_commanddoes not respect per-conversation workdir.While all file system tools receive the dynamic
baseDirectory, theexecute_commandtool uses the bash handler's fixedcwd(set toallowedDirectories[0]at initialization). This means shell commands may execute in a different directory than where file operations are scoped.If this is intentional (e.g., for security reasons), please add a code comment explaining the rationale. Otherwise, consider passing the workdir to the bash handler for consistency.
550-575: LGTM - ConsistentbaseDirectorypropagation to file system tools.All file system tool invocations correctly pass the
baseDirectoryderived from the conversation's workdir configuration.src/main/presenter/agentPresenter/acp/agentBashHandler.ts (3)
21-25: LGTM - Simplified command execution schema.The schema correctly defines the required fields for command execution without the previously removed
workdirfield.
52-52: LGTM - Fixed cwd from allowed directories.Using
allowedDirectories[0]as the working directory is safe since allowed directories are validated and normalized at construction time. This simplification removes per-command workdir logic.Also applies to: 75-75
46-166: LGTM - Robust command execution with proper safeguards.The implementation includes:
- Permission checking before execution
- Timeout handling with graceful kill followed by SIGKILL
- Output truncation at 30KB
- Terminal snippet emission for UI updates
- Proper process registration/unregistration for lifecycle management
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.