src/main/presenter/mcpPresenter/mcpClient.ts (1)

171-245: Runtime unification looks good; tighten Node/UV command detection to avoid false positives

The runtime handling refactor is generally solid:

• replaceWithRuntimeCommand now consistently routes node, npm, npx and uv/uvx through the bundled runtimes (with correct .exe/.cmd handling on Windows and bin/* on Unix).
• processCommandWithArgs normalizes both the main command and any bare node/npm/npx/uv arguments, which is reasonable for commands that directly invoke these tools.
• For Node/UV commands, you build a controlled env with a composed PATH and limited extra vars, while non‑Node commands inherit the full process env with PATH just augmented — this cleanly separates “managed” runtime invocations from everything else.

One thing worth tightening is isNodeCommand:

const isNodeCommand = ['node', 'npm', 'npx', 'uv', 'uvx'].some(
  (cmd) => command.includes(cmd) || args.some((arg) => arg.includes(cmd))
)

Using substring .includes on the full path/arg can accidentally classify unrelated binaries as “Node/UV commands” (e.g. a command under /opt/rnode/bin or an arg containing some-node-tool). That would push them through the Node/UV env whitelist, possibly dropping needed env vars.

A safer pattern is to base this on path.basename (similar to what you already do in replaceWithRuntimeCommand), e.g.:

const isNodeCommand = ['node', 'npm', 'npx', 'uv', 'uvx'].some((cmd) => {
  if (path.basename(command) === cmd) return true
  return args.some((arg) => path.basename(arg) === cmd)
})

This keeps the new runtime unification but avoids surprising reclassification of arbitrary commands.

Also applies to: 388-401, 415-418, 427-434, 472-479

src/main/lib/terminalHelper.ts (1)

205-211: Temp script naming & lifecycle could be more robust

createTempScript uses only Date.now() for filenames and never cleans up the temp files, so repeated calls can accumulate scripts in the temp directory and there is a (low) collision risk under high frequency.

Consider:

• Adding a random suffix to the filename.
• Optionally making the scripts self‑delete at the end (e.g., via an rm "$0"/del "%~f0" tail) if you don’t need them after execution.

Example change for safer naming:

-    const scriptPath = path.join(tempDir, `deepchat-acp-init-${Date.now()}${extension}`)
+    const random = Math.random().toString(36).slice(2, 8)
+    const scriptPath = path.join(
+      tempDir,
+      `deepchat-acp-init-${Date.now()}-${random}${extension}`
+    )

src/main/presenter/configPresenter/acpInitHelper.ts (3)

214-241: Avoid unbounded PTY output buffering for shell readiness

outputBuffer grows without bound and is only used for heuristic readiness detection and logging. Long-running installs (npm install -g ..., uv tool run ...) can easily produce multi‑MB output, which is unnecessary to keep in memory.

You can cap the buffer while preserving readiness behavior, e.g.:

-    let shellReady = false
-    let outputBuffer = ''
+    let shellReady = false
+    let outputBuffer = ''
+    const MAX_BUFFER = 8192
@@
-    pty.onData((data: string) => {
-      outputBuffer += data
+    pty.onData((data: string) => {
+      outputBuffer = (outputBuffer + data).slice(-MAX_BUFFER)
@@
-      if (!shellReady && outputBuffer.length > 0) {
+      if (!shellReady && outputBuffer.length > 0) {
         // Check for common shell prompt patterns or any non-empty output

This keeps memory bounded while still allowing prompt detection and logging.

---

334-345: Simplify env construction to avoid double-merging process.env

buildEnvironmentVariables already starts from process.env and layers runtime/registry/profile env on top, but spawn is called with { ...process.env, ...envVars }, effectively merging the system env twice.

You could simplify to reduce confusion and potential key inconsistencies:

-      env: { ...process.env, ...envVars } as Record<string, string>
+      env: envVars

and, if desired, move the initial process.env copy out of buildEnvironmentVariables so its responsibility is clearly “build complete env for PTY”.

Also applies to: 200-206

---

137-152: Single global PTY session – confirm this is intentional

AcpInitHelper tracks a single activeShell and startInteractiveSession always calls killTerminal() before spawning a new PTY. This means:

• Starting initialization for one agent will immediately kill any in‑progress initialization for another agent/window.
• All writeToTerminal / killTerminal calls operate on this single shared PTY.

If the UX is meant to support only one ACP initialization session globally, this is fine; otherwise, consider keying sessions by agent/conversation or webContents.id instead of a single global activeShell.

Also applies to: 173-176, 506-555

src/shared/types/presenters/legacy.presenters.d.ts (1)

535-535: Implementation verified—typing improvement remains optional

The initializeAcpAgent(agentId: string, isBuiltin: boolean) method has a concrete implementation wired correctly:

• Implementation at src/main/presenter/configPresenter/index.ts:1021 validates the agentId parameter at runtime (throws if the agent is not found in builtins or customs list)
• Call site at src/renderer/settings/components/AcpSettings.vue:699 invokes it with the correct contract
• Parameters originate from user selection of builtin/custom agents in the UI

The implementation works as designed. However, the type signature could be tightened: the code uses an as AcpBuiltinAgentId cast in the builtin path (line 1054), which validates at runtime. A discriminated union would provide compile-time safety and eliminate the cast:

initializeAcpAgent(agentId: AcpBuiltinAgentId, isBuiltin: true): Promise<void>
initializeAcpAgent(agentId: string, isBuiltin: false): Promise<void>

This shifts validation from runtime to compile-time but is optional given the current runtime checks.

src/renderer/settings/components/AcpTerminalDialog.vue (3)

107-116: Consider always sending acp-terminal:kill on close instead of gating on isRunning

Right now the close button only sends acp-terminal:kill when isRunning is true. Given that process startup and the acp-init:start event are slightly delayed, there’s a small window where a process may already be spawning but isRunning is still false, so closing the dialog won’t request a kill. Since killTerminal() is already defensive, you can simplify and make this more robust by always sending the kill on manual close:

-const handleManualClose = () => {
-  // Kill process if running
-  if (isRunning.value) {
-    if (window.electron) {
-      window.electron.ipcRenderer.send('acp-terminal:kill')
-    }
-  }
+const handleManualClose = () => {
+  // Always request backend to stop the active shell; it will no-op if none exists
+  if (window.electron) {
+    window.electron.ipcRenderer.send('acp-terminal:kill')
+  }
   emit('update:open', false)
   emit('close')
}

---

165-220: Guard terminal writes a bit more defensively to avoid noise and logs on malformed data

handleOutput already copes with both string and { type, data } formats and falls back to per‑character writes for xterm parsing errors, which is good. Two small improvements to reduce noise and make it even more defensive:

• Drop or downgrade the per‑chunk console.log (length/preview) to debug or behind a flag; high‑volume PTY output will spam the console.
• Before logging data.type, guard against type being missing/undefined on future payloads, to avoid logging undefined or needing narrower typings later.

These are minor, but will keep logs cleaner under heavy initialization output.

---

251-275: IPC listener lifecycle is correct but could be made idempotent-safe

setupIpcListeners and removeIpcListeners are paired and gated on window.electron, which is good. Just be aware that if this component is ever opened/closed very quickly or reused in another context, setupIpcListeners can be called multiple times before removeIpcListeners runs, leading to duplicate handlers.

This is largely mitigated by always calling removeAllListeners for the channels on close, but if this dialog is ever used from multiple windows you may want to scope the listeners to a per‑webContents identifier instead of global channels. Not blocking for this PR.

src/renderer/settings/components/AcpSettings.vue (1)

317-319: Initialization state tracking is clear; consider preventing concurrent initializations across agents

The initializing map and helpers (isInitializing/setInitializing) give precise, per‑agent “starting…” state while you open the terminal and call configPresenter.initializeAcpAgent, which is good.

Currently, nothing prevents starting initialization of another agent while one is already in progress (only per‑agent). Because the underlying init helper maintains a single active PTY, concurrent calls might be confusing (shared terminal, last call wins).

If you want to keep UX simple, consider a lightweight global guard (e.g., const initializingAny = computed(() => Object.keys(initializing).length > 0)) and disable all Initialize buttons while initializingAny is true, or at least while terminalDialogOpen is true.

Also applies to: 668-709

src/renderer/src/i18n/pt-BR/settings.json (1)

886-908: ACP inicialização/terminal keys match structure; minor PT-BR phrasing tweaks possible

The added initialize* and nested terminal.* keys mirror the other locales and line up with how AcpSettings and AcpTerminalDialog use them, so structurally this is correct.

If you want to polish PT‑BR wording later (optional):

• Use imperative or title‑case for button labels and titles, e.g. "initialize": "Inicializar" and "terminal.title": "Terminal de inicialização" for consistency with the rest of the locale.
• Consider "close": "Fechar" and "running": "Em execução" to match common UI phrasing.

Functionally everything is fine; this is just language quality polish.

src/main/presenter/configPresenter/index.ts (2)

26-27: IPC wiring for terminal input/kill is straightforward; ensure single presenter instance

Importing ipcMain, calling setupIpcHandlers() in the constructor, and routing:

• 'acp-terminal:input' → writeToTerminal
• 'acp-terminal:kill' → killTerminal

is a clean way to centralize PTY control in the config presenter. Just make sure ConfigPresenter is only instantiated once in the main process; otherwise you’ll accumulate duplicate ipcMain.on handlers for these channels.

Also applies to: 45-50, 188-191, 236-243

---

1018-1074: initializeAcpAgent logic is correct; consider handling missing webContents explicitly

This method:

• Reads useBuiltinRuntime from ACP settings.
• Optionally pulls npm/uv registries from mcpPresenter.
• Locates the settings window’s webContents for streaming.
• For built‑in agents: validates existence, finds active profile, and calls initializeBuiltinAgent.
• For custom agents: validates existence and calls initializeCustomAgent.

The happy path is solid, and the error messages for missing agents/profiles are clear.

Two small robustness improvements you might consider:

1. Guard on missing settings window
   If settingsWindow is missing or destroyed, webContents is undefined, so init will still start but nothing will be streamed back to the terminal dialog (if someone calls this from another context). You may want to fail fast with a clear error instead:

   if (!webContents) {
     throw new Error('[ACP Init] Settings window not available for terminal streaming')
   }

2. Optional: log whether the target agent is enabled
   For debugging, logging agent.enabled (for custom/built‑in) when initializing could help explain why an initialization is running for a disabled agent, if that ever becomes an issue.

Neither is blocking, but they’ll make failures easier to diagnose if this method is reused beyond the settings UI.

src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (3)

264-355: Runtime command replacement for node/npm/npx/uv looks sane; minor nits only

The node/npm/npx and uv/uvx remapping against this.nodeRuntimePath / this.uvRuntimePath is consistent and correctly falls back to the original command when embedded binaries are absent. This should avoid breaking user-installed runtimes when the bundled ones are missing or incomplete.

If you want to trim a bit of duplication / I/O:

• The existence checks here repeat what setupRuntimes already ensured. You could skip the fs.existsSync calls in this method and trust the precomputed nodeRuntimePath / uvRuntimePath, or gate them behind a debug-only assertion.
• On non‑Windows, the targetCommand branching could be collapsed to just const targetCommand = basename without changing behavior.

These are minor readability/perf nits; behavior looks good as-is.

---

454-480: Tighten isNodeCommand detection to avoid false positives and unintended env whitelisting

The current detection:

const isNodeCommand = ['node', 'npm', 'npx', 'uv', 'uvx'].some(
  (cmd) =>
    processedCommand.includes(cmd) ||
    processedArgs.some((arg) => typeof arg === 'string' && arg.includes(cmd))
)

is quite broad:

• Any command whose full path contains node, npm, npx, uv, or uvx as a substring (e.g. /opt/tools/node_exporter, a custom script with uv in its name, etc.) will be treated as a Node/UV command.
• Any argument containing those substrings will also flip isNodeCommand to true, even when the actual executable is unrelated (e.g. python script.py --mode=node).

Because isNodeCommand drives the strict env whitelist logic, this can unintentionally strip most of process.env for non‑Node tools, which is a behavioral change and potentially hard to debug.

You can make this more robust by classifying based on the executable name (with extension stripping on Windows), and ignoring arguments entirely:

-    // Determine if it's Node.js/UV related command
-    const isNodeCommand = ['node', 'npm', 'npx', 'uv', 'uvx'].some(
-      (cmd) =>
-        processedCommand.includes(cmd) ||
-        processedArgs.some((arg) => typeof arg === 'string' && arg.includes(cmd))
-    )
+    // Determine if it's Node.js/UV related command based on the executable name
+    const commandBase = path.basename(processedCommand).toLowerCase()
+    const commandName = commandBase.replace(/\.(exe|cmd)$/i, '')
+    const isNodeCommand = ['node', 'npm', 'npx', 'uv', 'uvx'].includes(commandName)

This keeps the intended behavior for node, npm, npx, uv, uvx (including absolute paths and .exe/.cmd wrappers) while avoiding accidental matches on unrelated commands or arguments.

The rest of the env/PATH shaping logic (whitelisting for Node/UV commands, full process.env for others, and gating builtin runtime paths on useBuiltinRuntime) looks consistent and should help with the nvm-related spawn failures.

Also applies to: 486-539, 561-585

---

595-628: Empty-string env handling changes semantics; confirm this is intended

The new guards:

if (value !== undefined && value !== '') { ... }
...
if (npmRegistry && npmRegistry !== '') { ... }
...
if (uvRegistry && uvRegistry !== '') { ... }

mean that:

• Agent-provided env vars with '' as value are now silently ignored instead of being passed through.
• NPM/UV registry helpers returning '' will no longer clear or override any existing registry-related vars.

This is probably what you want to avoid accidentally clobbering env with empty values, but it does remove the ability for an agent config to explicitly force an env var to empty (e.g., to clear a proxy or registry setting).

If explicit clearing is a use case you care about, consider treating undefined/null as “not set” and letting empty strings pass through:

-      Object.entries(agent.env).forEach(([key, value]) => {
-        if (value !== undefined && value !== '') {
+      Object.entries(agent.env).forEach(([key, value]) => {
+        if (value != null) {
           // ...
         }
       })

and drop the !== '' checks for the registry values as well. Otherwise, it would be good to document somewhere that null/omission means “don’t touch env” and empty strings are ignored.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: zerob13
Repo: ThinkInAIXYZ/deepchat PR: 977
File: package.json:137-137
Timestamp: 2025-10-12T00:58:06.513Z
Learning: In this Electron project, renderer process dependencies (used only in src/renderer/) should be placed in devDependencies because they are bundled by electron-vite during the build process. Only main process dependencies (used in src/main/) need to be in the dependencies section.

src/main/presenter/llmProviderPresenter/agent/shellEnvHelper.ts (2)

51-77: Shell init coverage for zsh/other shells is minimal (optional enhancement).

For zsh and other shells you only source ~/.zshrc or fall back to plain env. Many users have PATH/NVM setup in ~/.zprofile/~/.profile (especially on macOS login shells), so ACP might still miss some PATH customizations in those setups. Consider optionally sourcing ~/.zprofile (and maybe ~/.profile) for zsh and adding a fish-specific branch (config.fish) if you see reports from those users.

---

160-239: Env filtering is conservative; confirm it matches ACP needs.

getShellEnvironment only preserves PATH/Path and a small Node/npm-related subset, dropping proxies, UV, and other app-specific env. That’s great for safety but may surprise users who rely on additional env (e.g., HTTP(S)_PROXY for CLIs). If ACP agents ever need more, consider expanding the allowlist or making it configurable.

src/main/presenter/configPresenter/index.ts (2)

26-52: IPC handlers are fine but assume a single ConfigPresenter instance.

Wiring acp-terminal:input/acp-terminal:kill directly in the constructor is reasonable, but if ConfigPresenter is ever constructed more than once (tests, HMR, multiple windows), you’ll accumulate duplicate ipcMain.on handlers. If that’s a possibility, consider guarding with a static flag or exposing a dispose method that removes the listeners.

Also applies to: 189-192, 237-244

---

1022-1078: ACP agent initialization flow is reasonable; consider exposing the PTY handle later.

The builtin/custom agent resolution, registry lookup, and webContents wiring into initializeBuiltinAgent / initializeCustomAgent look coherent. Right now you ignore the returned IPty | null; if future features need to inspect or manage the underlying PTY from configPresenter (e.g., to show status), you may want to surface that value or some status back to the caller, but it’s not required for this fix.

src/main/presenter/mcpPresenter/mcpClient.ts (1)

25-26: RuntimeHelper integration looks correct; consider deduplicating env-building logic with ACP.

Using RuntimeHelper for runtime discovery, path expansion, PATH normalization, and node/uv command replacement is a solid improvement and reads clearly. The PATH/env shaping for stdio transports, however, is now very similar to what AcpProcessManager does, but with slightly different allowlists and behavior. To avoid future drift and hard‑to‑debug differences between MCP and ACP runtimes, consider factoring out a shared helper for “build env + PATH for (node/uv vs generic) commands” that both classes can call.

Also applies to: 124-125, 176-214, 215-266, 286-314

src/main/presenter/configPresenter/acpInitHelper.ts (2)

197-204: Duplicate environment variable merging.

Line 203 spreads process.env before envVars, but buildEnvironmentVariables already includes process.env (lines 338-342). This causes environment variables to be included twice, which is redundant but shouldn't cause issues since the second merge overwrites with the same values.

Simplify by using only envVars:

     const pty = spawn(shell, shellArgs, {
       name: 'xterm-color',
       cols: 80,
       rows: 24,
       cwd: process.cwd(),
-      env: { ...process.env, ...envVars } as Record<string, string>
+      env: envVars as Record<string, string>
     })

---

319-431: Consider normalizing PATH case variants in profile env.

The method correctly merges environment variables with appropriate precedence: profile env → runtime paths → system env. PATH merging (lines 406-411) handles multiple case variants (PATH, Path, path), but if a profile specifies multiple variants, they'll all be merged without warning.

Consider logging a warning if multiple PATH case variants are detected in profile.env:

const pathKeys = Object.keys(profile.env).filter(k => ['PATH', 'Path', 'path'].includes(k))
if (pathKeys.length > 1) {
  console.warn('[ACP Init] Multiple PATH variants detected in profile:', pathKeys)
}

Otherwise, the environment building logic is sound.

src/main/lib/runtimeHelper.ts (1)

262-282: Consider expanding lowercase environment variables.

Line 277 uses regex /\$([A-Z_][A-Z0-9_]*)/g which only matches uppercase environment variable names like $PATH but not lowercase variants like $path or $home. Unix systems can have lowercase environment variables.

Consider allowing any case:

-    expandedPath = expandedPath.replace(/\$([A-Z_][A-Z0-9_]*)/g, (match, varName) => {
+    expandedPath = expandedPath.replace(/\$([A-Za-z_][A-Za-z0-9_]*)/g, (match, varName) => {
       return process.env[varName] || match
     })

Or rely solely on the ${VAR} syntax (lines 272-274) for explicit variable expansion if case-insensitivity is a concern.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro