feat(agents): chooser for multiple detected binary paths - #1317
feat(agents): chooser for multiple detected binary paths#1317pedramamini wants to merge 1 commit into
Conversation
Detection now enumerates every valid installation instead of stopping at the first hit: direct probes of Homebrew, npm-global, ~/.local/bin and nvm/fnm/volta bins, plus everything `which -a` returns against the expanded shell PATH. Results are de-duplicated by canonical resolved path so symlink aliases collapse to one entry. When more than one installation is found, the Path field in the agent config panel renders a chooser listing the alternatives. Picking one writes through to the existing per-agent customPath store, so it persists across restarts and becomes the default for future agents. Manual entry still works for wrappers like codex-multi-auth-codex. A hand-typed path that detection did not find is surfaced as an explicit "Custom: <path>" entry rather than letting the select silently display the first detected option. The chooser is hidden for SSH-backed agents, where the path field means a remote command rather than a local binary. Closes #1048
📝 WalkthroughWalkthroughAgent detection now enumerates multiple binary installations, exposes them through ChangesAgent path detection and selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentDetector
participant findAllBinaryPaths
participant AgentConfigPanel
AgentDetector->>findAllBinaryPaths: enumerate binary installations
findAllBinaryPaths-->>AgentDetector: return ordered unique paths
AgentDetector->>AgentConfigPanel: provide agent config with allPaths
AgentConfigPanel->>AgentConfigPanel: display and save selected path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryAdds multi-install binary discovery and a persistent local-agent path chooser.
Confidence Score: 2/5The PR should not merge until chooser persistence stores the selected path and discovery can actually enumerate differently named wrappers. The chooser persists state from the previous render, while the probing command cannot return the differently named authentication wrapper central to the feature, leaving both selection durability and the primary detection scenario broken. Files Needing Attention: src/renderer/components/shared/AgentConfigPanel.tsx and src/main/agents/path-prober.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Detector
participant Prober
participant Panel
participant Store
Detector->>Prober: findAllBinaryPaths(binaryName)
Prober-->>Detector: canonicalized paths
Detector-->>Panel: AgentConfig.allPaths
Panel->>Panel: onCustomPathChange(selectedPath)
Panel->>Store: onCustomPathBlur()
Note over Panel,Store: Persistence currently reads the prior render's path
Reviews (1): Last reviewed commit: "feat(agents): chooser for multiple detec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onCustomPathChange(next); | ||
| // Persist immediately - selecting from the chooser is an explicit commit | ||
| onCustomPathBlur(); |
There was a problem hiding this comment.
When a user selects another installation in the configuration wizard, onCustomPathChange(next) schedules a React state update and onCustomPathBlur() immediately persists the path captured by the current render. This writes the previous path to agentConfigsStore, so subsequent detection and launches continue using the old binary.
Knowledge Base Used: Agent Run: Driving, Parsing, and Tracking AI Agent CLIs
| // returns every match by default. | ||
| const args = isWindows() ? [binaryName] : ['-a', binaryName]; |
There was a problem hiding this comment.
When codex and a differently named wrapper such as codex-multi-auth-codex are both installed, the direct probes construct only codex paths and which -a codex/where codex searches only that canonical name. The wrapper is therefore omitted from allPaths, so the chooser does not offer the primary wrapper scenario this feature is intended to support.
Knowledge Base Used: Agent Run: Driving, Parsing, and Tracking AI Agent CLIs
There was a problem hiding this comment.
Pull request overview
This PR improves agent binary detection by enumerating multiple valid installation paths (including PATH and common install locations) and surfaces those options in the renderer so users can choose the intended binary when more than one is present.
Changes:
- Add
AgentConfig.allPathsto carry multiple detected binary paths from main to renderer. - Implement
findAllBinaryPaths()to combine direct probes withwhich -a(Unix) /where(Windows), with canonical-path de-duplication. - Add a “Detected installations” chooser under the Path field in
AgentConfigPanel, with accompanying main and renderer test coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/types.ts | Extends AgentConfig with optional allPaths for multi-install UI. |
| src/main/agents/path-prober.ts | Adds multi-path probing and findAllBinaryPaths() (probe + which/where, de-dupe). |
| src/main/agents/index.ts | Re-exports new prober APIs (*All, findAllBinaryPaths). |
| src/main/agents/detector.ts | Populates allPaths when multiple installs are detected. |
| src/renderer/components/shared/AgentConfigPanel.tsx | Renders the multi-install chooser and wires selection into custom path updates. |
| src/tests/main/agents/path-prober.test.ts | Adds tests for ordering, de-dupe, empty results, and which failure tolerance. |
| src/tests/renderer/components/shared/AgentConfigPanel.test.tsx | Adds tests for chooser visibility, options, preselection, persistence, custom entry, and SSH hiding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const next = e.target.value; | ||
| if (next === CUSTOM_PATH_OPTION) return; | ||
| onCustomPathChange(next); | ||
| // Persist immediately - selecting from the chooser is an explicit commit | ||
| onCustomPathBlur(); |
| const found = await findAllBinaryPaths(agentDef.binaryName); | ||
| // Always include the active path (custom or detected) so the | ||
| // chooser reflects what is currently in use, even if it isn't | ||
| // one of the auto-probed locations. | ||
| const active = detection.path; | ||
| const merged = active && !found.includes(active) ? [active, ...found] : found; | ||
| if (merged.length > 1) { | ||
| allPaths = merged; | ||
| } |
| <p className="text-xs opacity-50 mt-1"> | ||
| Multiple {agent.binaryName} binaries were found. Your selection is saved as the | ||
| default for future agents. | ||
| </p> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/agents/detector.ts (1)
213-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-fatal catch skips Sentry reporting used elsewhere in this file.
findAllBinaryPathsalready swallows its own expected failures internally (probe misses viaPromise.allSettled,which/wherefailures via its own try/catch), so a throw reaching this outercatchis more likely an unexpected bug than a routine "binary not found" case.runModelDiscoveryandrunConfigOptionDiscoveryin this same file both callcaptureExceptionfor their non-fatal catches; this new catch only logs vialogger.debug, losing production visibility into genuinely unexpected failures.As per coding guidelines, "Do not silently swallow unexpected exceptions... rethrow unexpected errors so Sentry can capture them, and use the Sentry reporting utilities for intentional exception or event reporting."
♻️ Proposed fix
} catch (err) { // Non-fatal: chooser is just a nice-to-have, single-path mode still works. logger.debug(`findAllBinaryPaths failed for ${agentDef.binaryName}`, LOG_CONTEXT, { err, }); + captureException(err, { operation: 'agent:findAllBinaryPaths', agentId: agentDef.id }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/agents/detector.ts` around lines 213 - 218, Update the outer catch around findAllBinaryPaths to report the caught exception through the same captureException Sentry utility used by runModelDiscovery and runConfigOptionDiscovery, while retaining the existing debug context. Do not treat unexpected errors as silently swallowed; preserve the non-fatal flow only if that matches the established handling in those neighboring discovery methods.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/agents/detector.ts`:
- Around line 208-209: Update the active-path merge in the detection flow around
detection.path so deduplication compares canonical filesystem targets rather
than raw strings. Resolve active and existing found entries with the same
realpath behavior used by findAllBinaryPaths, retain only one path for
equivalent targets, and preserve the existing ordering and handling of
unresolved paths.
In `@src/main/agents/path-prober.ts`:
- Around line 665-713: Update the which/where lookup in findAllBinaryPaths to
pass an explicit timeout through the ExecOptions-compatible argument shape
recognized by execFileNoThrow, while preserving the expanded environment. Use
the existing timeout convention or constant if available, and ensure the lookup
returns through its existing non-fatal handling when the timeout is reached.
---
Nitpick comments:
In `@src/main/agents/detector.ts`:
- Around line 213-218: Update the outer catch around findAllBinaryPaths to
report the caught exception through the same captureException Sentry utility
used by runModelDiscovery and runConfigOptionDiscovery, while retaining the
existing debug context. Do not treat unexpected errors as silently swallowed;
preserve the non-fatal flow only if that matches the established handling in
those neighboring discovery methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d246f75e-8ffb-40a8-9085-e8e0fb18bec9
📒 Files selected for processing (7)
src/__tests__/main/agents/path-prober.test.tssrc/__tests__/renderer/components/shared/AgentConfigPanel.test.tsxsrc/main/agents/detector.tssrc/main/agents/index.tssrc/main/agents/path-prober.tssrc/renderer/components/shared/AgentConfigPanel.tsxsrc/shared/types.ts
| const active = detection.path; | ||
| const merged = active && !found.includes(active) ? [active, ...found] : found; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Active-path merge dedups by string, not canonical path.
found.includes(active) is a strict string comparison. If active (custom or fallback-detected path) is a different string that resolves to the same binary as an entry already in found (e.g. a symlink alias), it will show up twice in allPaths/the chooser — the exact "different path, same canonical target" case findAllBinaryPaths already handles internally for its own candidates via fs.promises.realpath, just not applied to this final merge step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/agents/detector.ts` around lines 208 - 209, Update the active-path
merge in the detection flow around detection.path so deduplication compares
canonical filesystem targets rather than raw strings. Resolve active and
existing found entries with the same realpath behavior used by
findAllBinaryPaths, retain only one path for equivalent targets, and preserve
the existing ordering and handling of unresolved paths.
| export async function findAllBinaryPaths(binaryName: string): Promise<string[]> { | ||
| // 1. Direct probes | ||
| const probedPaths = isWindows() | ||
| ? await probeWindowsPathsAll(binaryName) | ||
| : await probeUnixPathsAll(binaryName); | ||
|
|
||
| // 2. which/where lookup | ||
| const fromShell: string[] = []; | ||
| try { | ||
| const command = getWhichCommand(); | ||
| const env = await getExpandedEnvWithShell(); | ||
| // On Unix, `which -a` returns every match in PATH. On Windows, `where` | ||
| // returns every match by default. | ||
| const args = isWindows() ? [binaryName] : ['-a', binaryName]; | ||
| const result = await execFileNoThrow(command, args, undefined, env); | ||
| if (result.exitCode === 0 && result.stdout.trim()) { | ||
| const matches = result.stdout | ||
| .trim() | ||
| .split(/\r?\n/) | ||
| .map((p) => p.trim()) | ||
| .filter((p) => p); | ||
| fromShell.push(...matches); | ||
| } | ||
| } catch { | ||
| // which/where failures are non-fatal; we still have direct probe results | ||
| } | ||
|
|
||
| // 3. De-duplicate by canonical resolved path, preserving order | ||
| const seenKeys = new Set<string>(); | ||
| const result: string[] = []; | ||
| const candidates = [...probedPaths, ...fromShell]; | ||
|
|
||
| for (const candidate of candidates) { | ||
| let key: string; | ||
| try { | ||
| // realpath collapses symlinks (e.g., volta/bin/codex → volta/tools/...) | ||
| key = await fs.promises.realpath(candidate); | ||
| } catch { | ||
| key = candidate; | ||
| } | ||
| // Windows is case-insensitive | ||
| const normalizedKey = isWindows() ? key.toLowerCase() : key; | ||
| if (seenKeys.has(normalizedKey)) continue; | ||
| seenKeys.add(normalizedKey); | ||
| result.push(candidate); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the which/where lookup.
execFileNoThrow(command, args, undefined, env) passes env as the 4th argument, which execFileNoThrow only treats as ExecOptions (enabling timeout) when the object has an input or timeout key — otherwise it's treated as the legacy env-only signature, so no timeout is applied here. If which/where hangs (e.g. a broken PATH entry pointing at an unresponsive network mount, or a shell profile that never returns), this call — and the sequential for loop in doDetectAgents that awaits it once per detected agent — can block agent detection indefinitely.
🕒 Proposed fix: bound the which/where lookup with a timeout
- const result = await execFileNoThrow(command, args, undefined, env);
+ const result = await execFileNoThrow(command, args, undefined, {
+ ...env,
+ timeout: 5000,
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/agents/path-prober.ts` around lines 665 - 713, Update the
which/where lookup in findAllBinaryPaths to pass an explicit timeout through the
ExecOptions-compatible argument shape recognized by execFileNoThrow, while
preserving the expanded environment. Use the existing timeout convention or
constant if available, and ensure the lookup returns through its existing
non-fatal handling when the timeout is reached.
Closes #1048
Supersedes #1050, which was closed after its branch picked up an unrelated 57k-line test-coverage dump. This is the same approach, rebuilt cleanly on top of
main, plus a fix for the custom-wrapper case and renderer test coverage.Problem
Agent detection stopped at the first binary it found. Users with more than one valid install - most commonly an nvm-managed global
codexalongside an auth wrapper likecodex-multi-auth-codex- got whichever one happened to sort first, and had to paste the correct path by hand for every new agent.What changed
Detection enumerates everything (
src/main/agents/path-prober.ts)New
findAllBinaryPaths()combines direct probes of known install locations (Homebrew, npm-global,~/.local/bin, nvm/fnm/volta bins) with everythingwhich -a/wherereturns against the expanded shell PATH. Results are de-duplicated by canonical resolved path viafs.realpath, so a symlink alias and its target collapse to one entry. Case-insensitive on Windows.The existing
probeUnixPaths/probeWindowsPathsare now thin first-match wrappers over new*Allvariants, so single-path callers are unaffected.Detector populates
allPaths(src/main/agents/detector.ts)Only set when more than one install is found. The currently active path (custom or detected) is always merged in first, so the chooser reflects what is actually in use. Failures are non-fatal and logged at debug - the chooser is an enhancement, and single-path detection still works if the extra probe throws. Skipped for
bash, which is on every system and not user-selectable.Chooser in the agent config panel (
AgentConfigPanel.tsx)Renders under the Path field only when
allPaths.length > 1. Selecting a path writes throughonCustomPathChangeand commits immediately viaonCustomPathBlur, which persists to the existing per-agentcustomPathstore - so it survives restarts and becomes the default for the next agent.Hidden for SSH-backed agents, where the path field means a remote command rather than a local binary.
Custom wrappers stay first-class. A hand-typed path that detection did not find (not on PATH, or a
~-relative path that detection reports expanded) will not match any<option>. Rather than let the browser silently render the first option - misrepresenting which binary is active - it is surfaced as an explicitCustom: <path>entry. The text input remains the way to enter one.Mapping to the request
customPathstoreCustom:entryThe chooser stays visible whenever multiple installs exist rather than appearing once and hiding. It is a compact collapsed
<select>, so it is not intrusive, and a persistently visible control is discoverable later when a user adds a second install. Happy to gate it behind the existing Detect button if you'd rather have the original behavior.Testing
path-prober.test.ts: 4 new cases - priority ordering, symlink de-duplication, empty result, andwhichfailure falling back to probe results. 28 pass.AgentConfigPanel.test.tsx: 8 new cases - hidden at 0/1 paths, lists all options, preselection with and without a custom path, immediate persistence on change, theCustom:entry for an undetected wrapper, and hidden under SSH. 33 pass.npx eslintclean on all changed files; prettier reports no changes.Note:
src/__tests__/main/agents/claude-usage-startup.test.tsfails 20 tests on this branch. It fails identically onmainat39c9a6744- pre-existing and unrelated.Summary by CodeRabbit
New Features
Bug Fixes
Tests