fix: kimi cli can not acp with terminal #1154
Conversation
WalkthroughThe ACP process manager now uses a generalized environment propagation approach instead of specialized Node.js/UV command detection. It retrieves shell environment variables, merges PATH values from multiple sources with prioritization, creates per-agent isolated temporary working directories, and integrates these into the process spawn logic with improved error handling and logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)
450-459: LGTM with optional consideration.The per-agent temp directory isolation is a good fix to prevent conflicts between concurrent agent instances. The fallback to
HOME_DIRon creation failure is appropriate.Optional: Consider cleaning up the temp directory in the
releasemethod to prevent accumulation of orphaned directories over time. This could be a follow-up improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments (Chinese text exists in legacy code, but new code should use English)
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enable and maintain strict TypeScript type checking for all files
**/*.{ts,tsx}: Always use try-catch to handle possible errors in TypeScript code
Provide meaningful error messages when catching errors
Log detailed error logs including error details, context, and stack traces
Distinguish and handle different error types (UserError, NetworkError, SystemError, BusinessError) with appropriate handlers in TypeScript
Use structured logging with logger.error(), logger.warn(), logger.info(), logger.debug() methods from logging utilities
Do not suppress errors (avoid empty catch blocks or silently ignoring errors)
Provide user-friendly error messages for user-facing errors in TypeScript components
Implement error retry mechanisms for transient failures in TypeScript
Avoid logging sensitive information (passwords, tokens, PII) in logs
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Organize core business logic into dedicated Presenter classes, with one presenter per functional domain
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use EventBus from
src/main/eventbus.tsfor main-to-renderer communication, broadcasting events viamainWindow.webContents.send()
src/main/**/*.ts: Use EventBus pattern for inter-process communication within the main process to decouple modules
Use Electron's built-in APIs for file system and native dialogs instead of Node.js or custom implementations
src/main/**/*.ts: Electron main process code belongs insrc/main/with presenters inpresenter/(Window/Tab/Thread/Mcp/Config/LLMProvider) andeventbus.tsfor app events
Use the Presenter pattern in the main process for UI coordination
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Do not include AI co-authoring information (e.g., 'Co-Authored-By: Claude') in git commits
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
**/*.{js,ts,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Write logs and comments in English
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
{src/main/presenter/**/*.ts,src/renderer/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
Implement proper inter-process communication (IPC) patterns using Electron's ipcRenderer and ipcMain APIs
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/main/presenter/llmProviderPresenter/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/llm-agent-loop.mdc)
Define the standardized
LLMCoreStreamEventinterface with fields:type(text | reasoning | tool_call_start | tool_call_chunk | tool_call_end | error | usage | stop | image_data),content(for text),reasoning_content(for reasoning),tool_call_id,tool_call_name,tool_call_arguments_chunk(for streaming),tool_call_arguments_complete(for complete arguments),error_message,usageobject with token counts,stop_reason(tool_use | max_tokens | stop_sequence | error | complete), andimage_dataobject with Base64-encoded data and mimeType
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/**/*
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
New features should be developed in the
srcdirectory
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/main/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Main process code for Electron should be placed in
src/main
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/**/*.{ts,tsx,vue,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with single quotes, no semicolons, and 100 character width
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use OxLint for linting JavaScript and TypeScript files
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use camelCase for variable and function names in TypeScript files
Use PascalCase for type and class names in TypeScript
Use SCREAMING_SNAKE_CASE for constant names
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use EventBus for inter-process communication events
Files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
🧠 Learnings (5)
📚 Learning: 2025-11-25T05:27:12.209Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-11-25T05:27:12.209Z
Learning: Implement separation of concerns where `src/main/presenter/llmProviderPresenter/index.ts` manages the Agent loop and conversation history, while Provider files handle LLM API interactions, Provider-specific request/response formatting, tool definition conversion, and native vs non-native tool call mechanisms
Applied to files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
📚 Learning: 2025-11-25T05:26:11.312Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:26:11.312Z
Learning: Applies to src/main/presenter/llmProviderPresenter/providers/*.ts : Convert MCP tools to provider-specific formats and normalize streaming responses to standard events in each provider implementation
Applied to files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
📚 Learning: 2025-11-25T05:27:12.209Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-11-25T05:27:12.209Z
Learning: Applies to src/main/presenter/llmProviderPresenter/index.ts : In `src/main/presenter/llmProviderPresenter/index.ts` (`startStreamCompletion`), implement the Agent loop that manages the overall conversation flow, including multiple rounds of LLM calls and tool usage, maintaining `conversationMessages` history, calling `provider.coreStream()` on each iteration, and controlling the loop using `needContinueConversation` and `toolCallCount` (compared against `MAX_TOOL_CALLS`)
Applied to files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
📚 Learning: 2025-11-25T05:27:12.209Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-11-25T05:27:12.209Z
Learning: Applies to src/main/presenter/llmProviderPresenter/providers/*.ts : In Provider implementations, handle native tool support by converting MCP tools to Provider format using `convertToProviderTools` and including them in the API request; for Providers without native function call support, prepare messages using `prepareFunctionCallPrompt` before making the API call
Applied to files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
📚 Learning: 2025-11-25T05:26:24.867Z
Learnt from: CR
Repo: ThinkInAIXYZ/deepchat PR: 0
File: .cursor/rules/electron-best-practices.mdc:0-0
Timestamp: 2025-11-25T05:26:24.867Z
Learning: Applies to {src/main/presenter/**/*.ts,src/renderer/**/*.ts} : Implement proper inter-process communication (IPC) patterns using Electron's ipcRenderer and ipcMain APIs
Applied to files:
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts
🧬 Code graph analysis (1)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)
src/main/presenter/llmProviderPresenter/agent/shellEnvHelper.ts (1)
getShellEnvironment(160-240)
⏰ 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 (5)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (5)
329-337: LGTM!The cross-platform PATH collection handles different casing conventions properly. Any potential duplicates from collecting multiple path keys will be handled by
normalizePathEnvlater.
339-355: LGTM!Good generalization of shell environment retrieval for all commands. The error handling gracefully degrades on failure, and the PATH exclusion correctly defers PATH handling to the dedicated merge logic.
357-363: LGTM!Shell PATH is correctly prioritized by prepending to the existing paths. The platform-specific separator handling is appropriate.
365-397: LGTM!The path merging logic correctly establishes priority (runtime > shell > existing > default). The platform-specific handling for Node runtime path (
binsubdirectory on Unix) aligns with typical Node.js installation structures.
399-403: LGTM!Clean delegation to
normalizePathEnvfor path normalization and deduplication.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.