Skip to content

fix(mcp): improve stdio command lookup and error guidance - #2

Merged
LiusCraft merged 2 commits into
mainfrom
fix/mcp-stdio-path-resolution
Mar 14, 2026
Merged

fix(mcp): improve stdio command lookup and error guidance#2
LiusCraft merged 2 commits into
mainfrom
fix/mcp-stdio-path-resolution

Conversation

@LiusCraft

Copy link
Copy Markdown
Owner

Summary

  • Improve MCP stdio process startup by resolving executable paths from merged PATH entries (explicit Env PATH, system PATH, and common macOS/bin defaults).
  • Ensure spawned child processes inherit a deterministic merged PATH while still preserving user-defined non-PATH env vars.
  • Add clearer NotFound guidance for common misconfiguration cases (arguments entered in Command vs missing executable/PATH).
  • Add unit tests for PATH priority, command resolution behavior, and user-facing spawn error messages.

Testing

  • Not run (not requested)

@opencode-agent

Copy link
Copy Markdown

The full build fails due to missing system dependencies (ALSA), but I can analyze the code directly. Here's my review:

Review Summary

Correctness Issues

  1. PATH resolution doesn't verify execute permissions (mod.rs:975-982)

    • resolve_stdio_command_path only checks is_file(), not whether the file is executable
    • A non-executable file could be returned, causing spawn to fail anyway
  2. Hardcoded Unix paths missing Windows support (mod.rs:928-935)

    • Only macOS/Unix paths are hardcoded (/opt/homebrew/bin, /usr/bin, etc.)
    • No fallback for Windows (C:\Windows\System32, etc.)
  3. Test cleanup doesn't propagate errors (mod.rs:1289-1290)

    • Uses let _ = which silently ignores cleanup failures; minor but could mask issues

Missing Tests

  1. command_looks_like_path - No direct test for path detection logic
  2. Absolute path handling - No test that absolute paths pass through unchanged
  3. User env preservation - No test verifying non-PATH env vars are passed to child process
  4. Empty PATH edge case - No test for empty/null PATH scenarios

Code Quality

  • Good: PATH priority is correct (explicit env → system → fallbacks)
  • Good: Deduplication preserves first occurrence (correct priority)
  • Good: Error messages are helpful and actionable

Recommendations

  1. Add +x permission check or use std::process::Command::new which handles this
  2. Consider conditional Windows paths via cfg!(target_os = "windows")
  3. Add tests for edge cases mentioned above

New%20session%20-%202026-03-14T11%3A24%3A24.845Z
opencode session  |  github run

@LiusCraft
LiusCraft merged commit 71aadd7 into main Mar 14, 2026
1 check passed
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.

1 participant