Skip to content

fix: improve binary detection accuracy using OS-level PATH search#95

Merged
0xGaspar merged 1 commit intomainfrom
fix/binary-detection
Apr 6, 2026
Merged

fix: improve binary detection accuracy using OS-level PATH search#95
0xGaspar merged 1 commit intomainfrom
fix/binary-detection

Conversation

@0xGaspar
Copy link
Copy Markdown
Contributor

@0xGaspar 0xGaspar commented Apr 6, 2026

Replace manual PATH environment variable traversal with a call to the binary's version flag. This leverages the OS-level execvp mechanism to correctly handle symlinks and execute permissions during AI provider auto-detection.

Replace manual PATH environment variable traversal with a call to the binary's version flag. This leverages the OS-level `execvp` mechanism to correctly handle symlinks and execute permissions during AI provider auto-detection.
@0xGaspar
Copy link
Copy Markdown
Contributor Author

0xGaspar commented Apr 6, 2026

@corvid-agent review this

@corvid-agent
Copy link
Copy Markdown
Collaborator

👋 CorvidAgent is looking into this.

Copy link
Copy Markdown
Collaborator

@corvid-agent corvid-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by CorvidAgent

Clean improvement. The switch from manual PATH traversal to Command::new(name).arg("--version").status().is_ok() is a win on multiple fronts:

What's good:

  • OS-level execvp correctly handles symlinks, execute permissions, and platform-specific PATH resolution (the old code split on : only, which would fail on Windows)
  • Simpler and more idiomatic Rust — less surface area for bugs
  • status().is_ok() is the right check: Ok means the binary was found and executed, regardless of exit code
  • Spec updated to match

Minor observations (non-blocking):

  • No timeout on the --version spawn. If a misbehaving binary hangs on --version, auto-detection stalls. For the known CLI providers (ollama, aichat, etc.) this is fine, but worth a comment noting the assumption.
  • Some exotic binaries might not recognize --version and could produce unexpected behavior (e.g., treating it as a filename). Again, unlikely for the providers in scope.

Neither point warrants blocking — the new approach is strictly better than the old one. LGTM.

@0xGaspar 0xGaspar merged commit b38067f into main Apr 6, 2026
9 checks passed
@0xGaspar 0xGaspar deleted the fix/binary-detection branch April 6, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants