Put the managed Node bin dir on PATH when spawning stdio MCP servers - #9304
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a514ca85d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| present = binary.is_file() | ||
| except OSError: | ||
| return False | ||
| _managed_node_ok = present and _node_version_ok(str(binary)) |
There was a problem hiding this comment.
When setup has switched to a valid system Node but a leftover managed installation has a working node binary and a damaged executable npx/npx.cmd, this check still marks the managed directory usable. _stdio_argv() then resolves the broken managed npx ahead of the working system copy, so stdio MCP startup fails. The fresh evidence after bd9148f is that the new validation probes only node -v and never verifies the npx executable that this change actually prefers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. It needs a managed install where node clears the floor but npx exists and is broken, which no setup path produces: the Node tarball ships node, npm and npx together, so a working node -v implies a working npx. The reachable variants are already handled. A missing managed npx falls through to the system copy, since shutil.which scans the remaining PATH entries (verified: with an empty managed dir ahead of a system dir, npx resolves to the system copy). A stale managed install is what bd9148f covers, via the version floor. That leaves only a selectively corrupted install, which is a damaged host state rather than input this code receives, and probing npx on every client build would add a second subprocess for it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
…ep in the PATH tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
for more information, see https://pre-commit.ci
|
@codex review |
2 participants
Setup installs an isolated Node into <UNSLOTH_HOME>/node when the system Node is missing or too old, but that directory was never added to the env the backend spawns stdio MCP servers with _client() built the StdioTransport with the
inherited env only. So on machines without a usable system Node, any
npx -y ...MCP server failed with "Connection closed", hitting exactly the users the bundled Node exists for.Fix: _client() now prepends the managed Node bin dir to the child's PATH when it exists, keeping any PATH the server's own env sets. New helpers in utils/node_runtime (managed_node_bin_dir, path_with_managed_node) plus tests.