fix: windows acp process failed #1109
Conversation
WalkthroughThis PR adds support for using DeepChat's built-in Node and UV runtimes when spawning ACP processes. It introduces a configuration flag, cross-spawn library dependency, async process spawning, environment variable and PATH management, and UI controls to toggle this feature with i18n support for ten languages. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Settings UI
participant CP as ConfigPresenter
participant Store as ACP Store
participant PM as AcpProcessManager
participant Provider as AcpProvider
participant Spawn as cross-spawn
rect rgb(200, 220, 255)
Note over UI,Store: Configuration Phase
UI->>CP: setAcpUseBuiltinRuntime(true)
CP->>Store: store.useBuiltinRuntime = true
CP-->>UI: Promise<void> resolved
end
rect rgb(200, 255, 220)
Note over Provider,Spawn: Process Spawning Phase
Provider->>PM: new AcpProcessManager({getUseBuiltinRuntime, getNpmRegistry, getUvRegistry})
PM->>PM: spawnProcess()
PM->>PM: await spawnAgentProcess(agent)
PM->>CP: getUseBuiltinRuntime() → Promise<boolean>
CP-->>PM: returns useBuiltinRuntime flag
alt useBuiltinRuntime = true
PM->>PM: resolveRuntimePaths()
PM->>PM: replaceWithRuntimeCommand(cmd)
PM->>PM: augmentEnvironment(process.env)
Note right of PM: Merge registry vars<br/>Prepend runtime PATH
else useBuiltinRuntime = false
Note right of PM: Use system defaults
end
PM->>Spawn: cross-spawn(command, args, {env, cwd})
Spawn-->>PM: ChildProcess
PM-->>Provider: Promise<ChildProcess> resolved
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 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/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)
333-364: Fix incorrect bun-to-node command mapping.On line 353, when only Node.js runtime is available,
buncommands are mapped tonode. This will fail because Bun and Node have different CLI arguments and behavior. For example,bun run script.jsuses different semantics thannode run script.js.Consider either:
- Skip replacement and let system PATH find
bunif node runtime is used- Return the original command when mapping would be incorrect
- Remove support for replacing
buncommands entirely} else if (this.nodeRuntimePath) { // Use Node.js runtime let targetCommand: string if (basename === 'node') { targetCommand = 'node' } else if (basename === 'npm') { targetCommand = 'npm' } else if (basename === 'npx') { targetCommand = 'npx' } else if (basename === 'bun') { - targetCommand = 'node' // Map bun command to node + // Bun and Node have incompatible CLIs, don't replace + return command } else { targetCommand = basename } const nodePath = path.join(this.nodeRuntimePath, 'bin', targetCommand) if (fs.existsSync(nodePath)) { return nodePath } // If doesn't exist, return original command to let system find it via PATH return command }
🧹 Nitpick comments (2)
src/main/presenter/configPresenter/index.ts (1)
950-956: ACP builtin-runtime getters/setters are wired correctly; consider future-proofingsetThe new accessors cleanly delegate to
AcpConfHelperand match the Promise-basedIConfigPresentersurface. IfAcpConfHelper.setUseBuiltinRuntimeever becomes asynchronous or returns a status, you may want toawaitit and/or propagate a boolean to the caller to detect no-op/failed writes; for the current likely-synchronous implementation this is fine.src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)
493-517: Consider refining the Node command detection heuristic.The substring-based detection (line 493-497) could have false positives when paths or arguments contain strings like "node" or "npm" (e.g.,
/usr/bin/nodeinfoor--enable-node-addons). While the whitelist prevents actual harm, this could cause unnecessary PATH modifications for unrelated commands.Consider using a more precise check, such as matching only the basename of the command.
- const isNodeCommand = ['node', 'npm', 'npx', 'bun', 'uv', 'uvx'].some( - (cmd) => - processedCommand.includes(cmd) || - processedArgs.some((arg) => typeof arg === 'string' && arg.includes(cmd)) - ) + const cmdBasename = path.basename(processedCommand) + const isNodeCommand = ['node', 'npm', 'npx', 'bun', 'uv', 'uvx'].includes(cmdBasename)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
package.json(1 hunks)src/main/presenter/configPresenter/acpConfHelper.ts(4 hunks)src/main/presenter/configPresenter/index.ts(1 hunks)src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts(8 hunks)src/main/presenter/llmProviderPresenter/providers/acpProvider.ts(2 hunks)src/main/presenter/mcpPresenter/index.ts(1 hunks)src/renderer/settings/components/AcpSettings.vue(4 hunks)src/renderer/src/i18n/en-US/settings.json(1 hunks)src/renderer/src/i18n/fa-IR/settings.json(1 hunks)src/renderer/src/i18n/fr-FR/settings.json(1 hunks)src/renderer/src/i18n/ja-JP/settings.json(1 hunks)src/renderer/src/i18n/ko-KR/settings.json(1 hunks)src/renderer/src/i18n/pt-BR/settings.json(1 hunks)src/renderer/src/i18n/ru-RU/settings.json(1 hunks)src/renderer/src/i18n/zh-CN/settings.json(1 hunks)src/renderer/src/i18n/zh-HK/settings.json(1 hunks)src/renderer/src/i18n/zh-TW/settings.json(1 hunks)src/shared/types/presenters/legacy.presenters.d.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/presenter/llmProviderPresenter/providers/acpProvider.ts (2)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)
AcpProcessManager(55-786)src/main/presenter/index.ts (1)
presenter(223-223)
src/main/presenter/configPresenter/acpConfHelper.ts (1)
src/shared/types/presenters/legacy.presenters.d.ts (2)
AcpStoreData(704-709)AcpAgentConfig(711-717)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (3)
src/main/presenter/llmProviderPresenter/agent/types.ts (1)
AgentProcessManager(32-41)src/shared/types/presenters/legacy.presenters.d.ts (1)
AcpAgentConfig(711-717)test/mocks/electron.ts (1)
app(2-10)
⏰ 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 (29)
src/renderer/src/i18n/zh-TW/settings.json (1)
829-830: ACP “built-in runtime” TW strings are clear and consistentThe new
useBuiltinRuntimeTitle/Descriptionaccurately describe bypassing systemnode/uvand match the English intent. No issues found.src/renderer/src/i18n/zh-HK/settings.json (1)
829-830: HK locale builtin-runtime strings look goodThe Cantonese wording for using DeepChat’s built-in runtime is natural and aligned with other locales; semantics match the feature.
src/renderer/src/i18n/en-US/settings.json (1)
803-804: Builtin runtime toggle copy is clearThe title and description clearly explain that ACP will prefer bundled
node/uvover system commands when enabled. No wording or UX issues.src/renderer/src/i18n/ko-KR/settings.json (1)
835-836: Korean builtin-runtime translations are accurateThe new strings correctly describe bypassing system
node/uvin favor of DeepChat’s bundled runtime and fit the existing style.src/renderer/src/i18n/ja-JP/settings.json (1)
835-836: Japanese builtin-runtime translations read wellThe title/description clearly convey using DeepChat’s bundled
node/uvinstead of system commands; style matches the rest of the JA settings.package.json (1)
76-76: cross-spawn 7.0.6 is compatible with Node.js 20.x and Windows 10/11cross-spawn 7.0.6 is intended to work on Node.js 20.x and Windows 10/11 and remains the recommended cross‑platform shim for spawn/spawnSync. The dependency addition is appropriate. Note that there have been Node 20.x Windows child_process bugs that can affect any spawn wrapper, and cross-spawn 7.0.6 hasn't seen frequent releases recently, so monitor for upstream Node.js fixes and consider testing thoroughly on your target platforms.
src/renderer/src/i18n/pt-BR/settings.json (1)
835-836: ACP built‑in runtime pt‑BR strings look goodTitle and description are clear, idiomatic, and consistent with the feature as described in other locales.
src/renderer/src/i18n/fa-IR/settings.json (1)
835-836: ACP built‑in runtime fa‑IR strings are consistentThe added title/description accurately reflect the behavior and match the tone/style of surrounding translations.
src/main/presenter/mcpPresenter/index.ts (1)
1305-1313: Thin registry accessors are correct and low‑riskThe new
getNpmRegistry/getUvRegistrymethods are simple pass‑throughs toServerManager, match existing patterns (e.g.,getNpmRegistryStatus), and introduce no observable side effects.src/renderer/src/i18n/fr-FR/settings.json (1)
835-836: ACP built‑in runtime FR strings read wellThe French title/description clearly describe bypassing system
node/uvin favor of bundled versions and are consistent with other locales.src/renderer/src/i18n/zh-CN/settings.json (1)
803-804: ACP built‑in runtime zh‑CN copy is clearThe added title/description accurately explain the toggle behavior in natural zh‑CN and align with the rest of the ACP section.
src/renderer/settings/components/AcpSettings.vue (1)
19-33: Builtin runtime toggle wiring is correct and consistent
- UI for the “use builtin runtime” switch is gated on
acpEnabledand uses the new i18n keys consistently.- State is loaded once on mount via
configPresenter.getAcpUseBuiltinRuntime()and stored in a dedicated ref.handleUseBuiltinRuntimeTogglecorrectly guards against concurrent toggles, persists viasetAcpUseBuiltinRuntime, updates local state on success, and rolls back on failure using the sharedhandleErrorpath.No issues spotted in this integration; behavior matches the rest of the ACP settings patterns.
Also applies to: 278-281, 375-395, 657-661
src/renderer/src/i18n/ru-RU/settings.json (1)
835-836: ACP built‑in runtime RU strings are accurateThe Russian title and description correctly describe using DeepChat’s bundled runtimes instead of system
node/uvand are stylistically consistent with the rest of the locale.src/shared/types/presenters/legacy.presenters.d.ts (2)
501-502: LGTM - New ACP runtime configuration methods.The new methods follow established patterns in the interface and provide a clean API for toggling the built-in runtime feature.
425-427: LGTM - Registry accessor methods.The optional synchronous getters are appropriate for internal registry configuration access. The
string | nullreturn type correctly handles unconfigured registries.src/main/presenter/configPresenter/acpConfHelper.ts (4)
38-38: LGTM - Cleaner default configuration.Removing the empty string API key placeholders is appropriate. API keys should be provided through actual environment configuration rather than as placeholders in defaults.
Also applies to: 47-47
55-55: LGTM - Type definition is correct.The optional boolean field is properly typed for the new runtime toggle flag.
75-75: LGTM - Sensible default value.Defaulting to
falsemakes the built-in runtime feature opt-in, which is appropriate for a new capability.
97-103: LGTM - Clean accessor implementation.The getter uses
Boolean()coercion for type safety, and the setter is straightforward. The implementation follows established patterns in the class.src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (10)
24-29: LGTM - Well-structured configuration options.The new options use callback functions for dynamic configuration retrieval, which is a good pattern. The optional registry getters and lazy runtime initialization are appropriate design choices.
Also applies to: 57-74
46-48: LGTM - Appropriate Electron detection.The
'type' in processcheck is a standard way to detect Electron environments. The reference to the MCP SDK shows this pattern is used elsewhere.
172-213: LGTM - Async spawn correctly handled.The function properly awaits the now-async
spawnAgentProcess. The rest of the process lifecycle management is unchanged and appropriate.
215-281: LGTM - Robust runtime detection.The setup logic properly handles:
- Lazy initialization with guard clause
- Electron
.asar.unpackedpath resolution- Platform-specific executable paths and extensions
- File existence validation before setting paths
- Both
uvanduvxvalidation for UV runtime
404-446: LGTM - Comprehensive path handling.The default paths cover common locations for each platform. The path expansion correctly handles:
- Tilde (
~) expansion using Electron's home path${VAR}format environment variables$VARformat (uppercase only, which is standard practice)Note: The
$VARpattern intentionally only matches uppercase variables ([A-Z_][A-Z0-9_]*), which is a common security practice that covers most standard environment variables.
448-488: LGTM - Good process initialization with comprehensive logging.The async refactor properly:
- Validates commands before spawning
- Expands paths with
~and environment variables- Replaces commands with built-in runtimes when enabled
- Logs command processing for debugging
The detailed logging will be valuable for troubleshooting spawn issues.
519-634: LGTM - Well-designed environment variable handling.The dual approach is appropriate:
- Node/Bun/UV commands: Whitelist for security, preventing environment pollution
- Other commands: Preserve system environment to avoid breaking tools
The platform-specific PATH construction and runtime path prepending are correct. Note that built-in runtimes will shadow system installations when enabled, which is the intended behavior.
655-670: LGTM - Registry configuration properly scoped.Registry environment variables are only set when using built-in runtimes, and the async registry getters are properly awaited. The variables set (
npm_config_registry,UV_DEFAULT_INDEX,PIP_INDEX_URL) cover the main package managers used by ACP agents.
696-707: LGTM - Secure spawn configuration.The spawn options are properly configured:
shell: falseprevents shell injection attackswindowsHide: truein Electron prevents console windows from appearingstdio: ['pipe', 'pipe', 'pipe']enables proper I/O stream handling
1-2: cross-spawn dependency is properly declared in package.json.The switch to
cross-spawn(v^7.0.6 in dependencies) is correctly implemented and will improve cross-platform compatibility, especially on Windows. The separate type import is also appropriate.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.