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

49-52: Runtime detection logic is reasonable; clarify assumptions and Windows Bun usage

The lazy initialization and per‑platform existence checks for bun, node, and uv under app.getAppPath()/runtime look consistent and should be cheap enough given the one‑time guard via runtimesInitialized.

Two small points to consider:

• On Windows you compute and populate bunRuntimePath, but it’s never actually used later (no PATH augmentation, no bun replacement branch for win32). If Bun is not intended to be supported on Windows, you could skip this detection entirely; if it is intended, you probably want to either add it to PATH or use it in replaceWithRuntimeCommand similarly to Node/UV.
• The app.getAppPath().replace('app.asar', 'app.asar.unpacked') heuristic assumes a specific packaging layout and case‑sensitive app.asar segment. If there are dev/test modes where getAppPath() doesn’t contain that substring, this still works (no replacement), but it might be worth documenting that assumption in a comment so future changes to the packaging path don’t silently break runtime discovery.

Also applies to: 197-264

---

1-1: Command replacement works, but argument rewriting and Bun→Node fallback may be fragile

The isCommandAvailable + replaceWithRuntimeCommand flow is a good way to prefer system binaries and fall back to bundled runtimes, and limiting isCommandAvailable to a known set of basenames avoids command‑injection concerns despite using where/which.

A few behavioural edge cases are worth double‑checking:

1. Rewriting arguments, not just the main command
   spawnAgentProcess runs replaceWithRuntimeCommand over every entry in agent.args. That means any arg that is exactly uv, uvx, node, npm, npx, or bun is rewritten as if it were an executable, even when it might be a plain argument (e.g. a script name or a flag value) rather than a command. Depending on how AcpAgentConfig is used in practice, this could change semantics for wrappers like my-wrapper node script.js or uv run node script.js.
   If your agents only ever use these names as actual executables (command or first arg in well‑known patterns), you’re fine; otherwise, you may want to restrict replacement to the top‑level agent.command (and possibly a very small set of known “runner” patterns) instead of all args.

2. Bun fallback to Node when only a Node runtime is bundled (non‑Windows)
   In the non‑Windows branch, when this.bunRuntimePath is falsy but this.nodeRuntimePath is set, the basename === 'bun' case falls through to the nodeRuntimePath logic and sets targetCommand = 'node'. That means a configured bun command will silently run the bundled Node binary instead, which is unlikely to be what callers expect if they rely on Bun‑specific features.
   It might be safer to either:

   • leave bun unchanged in that case and let the spawn fail deterministically, or
   • only fall back to Node when you know the agent is happy with Node semantics.

3. Availability checks per spawn
   Every spawn does a which/where for each runtime command. This is probably fine at current scale, but if agents are spawned frequently it would be trivial to add a tiny in‑memory cache keyed by command name to avoid repeated execSync calls.

If you confirm that your current agent configs don’t rely on these edge cases (especially point 1 and the Bun→Node mapping), you can leave this as is; otherwise, I’d recommend tightening the replacement rules.

Also applies to: 4-5, 265-276, 278-349

---

351-357: PATH augmentation is mostly correct; add a guard for empty paths and consider case cleanup on Windows

The approach of collecting existing PATH variants (PATH/Path/path), prepending runtime directories, and then normalizing to a single key with the right separator is sound.

Two minor robustness tweaks to consider:

• Avoid injecting an empty PATH when nothing is available
  If, for some reason, there are no PATH‑like keys in mergedEnv and no runtime paths are detected, allPaths ends up empty and normalizePathEnv will set PATH/Path to an empty string. It’s safer to skip overriding PATH entirely in that situation:

• const { key, value } = this.normalizePathEnv(allPaths)

• mergedEnv[key] = value

• if (allPaths.length > 0) {

•  const { key, value } = this.normalizePathEnv(allPaths)
  

•  mergedEnv[key] = value
  

• }

- **Windows: avoid multiple casings of PATH**  
Since you read from `PATH`, `Path`, and `path` but always write to `Path`, it’s possible for the child env to contain multiple case variants. Windows treats env keys case‑insensitively, but behaviour with duplicates isn’t always obvious. Optionally, you could delete other case variants (`PATH`, `path`) from `mergedEnv` before assigning the normalized one to make the effective PATH explicit.

Neither is a blocker, but they would make the PATH handling a bit more predictable across environments.





Also applies to: 367-400

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 547f64c83c0520cbb6d3b2863044395e32906630 and a87e062a8168d3f9c0c752b4d59814f955fb1096.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

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

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧬 Code graph analysis (1)</summary>

<details>
<summary>src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (2)</summary><blockquote>

<details>
<summary>test/mocks/electron.ts (1)</summary>

* `app` (2-10)

</details>
<details>
<summary>src/shared/types/presenters/legacy.presenters.d.ts (1)</summary>

* `AcpAgentConfig` (709-715)

</details>

</blockquote></details>

</details>

</details>

<details>
<summary>⏰ 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)</summary>

* GitHub Check: build-check (x64)

</details>

<details>
<summary>🔇 Additional comments (1)</summary><blockquote>

<details>
<summary>src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)</summary><blockquote>

`359-366`: **Spawn integration with runtime helpers looks good**

Initializing runtimes once per process, resolving a potentially substituted `processedCommand`, mapping args, and then spawning with a merged env (including updated PATH) is cohesive and keeps the new logic well‑encapsulated in `spawnAgentProcess`. The `stdio: ['pipe', 'pipe', 'pipe']` configuration and the rest of the process wiring are unchanged, so existing stream handling should continue to work as before.





Also applies to: 401-401

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->