Skip to content

fix(ssh): select a satisfying node when the login-shell node fails the engine range#3710

Open
arturskruze wants to merge 1 commit into
pingdotgg:mainfrom
arturskruze:fix/remote-node-engine-gate
Open

fix(ssh): select a satisfying node when the login-shell node fails the engine range#3710
arturskruze wants to merge 1 commit into
pingdotgg:mainfrom
arturskruze:fix/remote-node-engine-gate

Conversation

@arturskruze

@arturskruze arturskruze commented Jul 5, 2026

Copy link
Copy Markdown

What

ensure_remote_node_path (in REMOTE_NODE_ENV_SCRIPT) only fell back to version managers (mise/fnm/nodenv/nvm, and the nvm version scan) when there was no node on PATH at all (! command -v node). When a login shell already exposes a node that fails the engine range — e.g. an nvm default below the minimum, or a system /usr/bin/node — every fallback was skipped and the unsatisfying node was exec'd anyway (… || true).

This gates the fallbacks on a combined "node present and satisfies engine" check (remote_node_ready), so a satisfying node that's already installed via a version manager actually gets selected. The nvm version scan also stops at the first satisfying version instead of relying on glob ordering.

Why

On Windows+WSL this made the desktop backend launch under an unsupported node: e.g. the nvm default was 22.12.0 while a satisfying 22.21.1 was installed but not selected. Because the server bundle gates startup on import.meta.main (added in Node 22.16), it then loaded its module graph and exited 0 silently without binding — and the desktop respawned it forever, showing "Connecting to WSL…" with no diagnostic.

Full trace and repro in #3709. (That issue also notes a second, separate hardening idea: have bin.ts fail loudly on an unsupported node instead of exiting silently — kept out of this PR to stay focused.)

Scope / risk

  • Single function; behavior-preserving for the "satisfying node already on PATH" and "no node at all" cases. Only the "unsatisfying node present" case changes.
  • No new deps, no interface changes. Existing packages/ssh/src/tunnel.test.ts assertions (ensure_remote_node_path(), if ! ensure_remote_node_path; then, remote_node_satisfies_engine()) are unaffected.

🤖 Generated with Claude Code


Note

Low Risk
Single embedded shell function in SSH remote bootstrap; behavior only changes when an unsatisfying node is already on PATH, with no API or dependency changes.

Overview
Remote SSH Node selection now treats “node on PATH” and “node satisfies the engine range” as one gate via a new remote_node_ready helper in the embedded REMOTE_NODE_ENV_SCRIPT.

Previously, ensure_remote_node_path only ran mise/fnm/nodenv/nvm fallbacks when node was missing entirely. If the login shell already had a node that failed the semver check (e.g. nvm default below minimum while a newer install existed), fallbacks were skipped and the bad binary was used—leading to silent exit and stuck “Connecting to WSL…” on Windows+WSL.

Fallback activation is now ! remote_node_ready at each step. The nvm version-directory scan stops at the first PATH prefix where remote_node_ready succeeds instead of depending on glob order. Behavior is unchanged when no node exists or when the current node already satisfies the range.

Reviewed by Cursor Bugbot for commit 27be28c. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix SSH tunnel node selection when the login-shell node fails the engine range

  • Adds a remote_node_ready helper in tunnel.ts that checks both node presence and engine compatibility in one place.
  • ensure_remote_node_path now continues activating version managers (mise, fnm, nodenv, nvm) even when a node binary exists but doesn't satisfy the engine requirement, instead of short-circuiting on presence alone.
  • The NVM versions loop now breaks as soon as a satisfying version is found rather than iterating all candidates.
  • Behavioral Change: SSH sessions that previously used an incompatible node binary will now attempt to resolve a compatible one via available version managers.
📊 Macroscope summarized 27be28c. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

… range

ensure_remote_node_path only fell back to version managers (nvm/fnm/mise/
nodenv) when there was no node on PATH at all. When a login shell already
exposes a node that fails the engine range (e.g. an nvm `default` below the
minimum, or a system /usr/bin/node), the fallbacks were skipped and the
unsatisfying node was exec'd anyway.

Gate the fallbacks on a combined "node present AND satisfies engine" check
(remote_node_ready) so a satisfying node already installed via a version
manager is selected, and stop the nvm version scan at the first satisfying
version.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 16e2da8f-cbf6-4b82-abc0-8a789dd98199

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Jul 5, 2026
fi

command -v node >/dev/null 2>&1 && remote_node_satisfies_engine
remote_node_ready

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium src/tunnel.ts:414

remote_node_ready redirects remote_node_satisfies_engine stderr to /dev/null, and ensure_remote_node_path returns via remote_node_ready at line 414. When no Node version satisfies the engine range, the Remote node <version> does not satisfy required range ... error from the engine check is suppressed, so the caller falls back to printing only the generic "missing node on PATH" message. This hides the specific version/range diagnostic needed to understand why remote startup failed. Consider having ensure_remote_node_path call remote_node_satisfies_engine without suppressing stderr on the final check, so the engine-range error surfaces to the caller.

Suggested change
remote_node_ready
command -v node >/dev/null 2>&1 && remote_node_satisfies_engine
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/ssh/src/tunnel.ts around line 414:

`remote_node_ready` redirects `remote_node_satisfies_engine` stderr to `/dev/null`, and `ensure_remote_node_path` returns via `remote_node_ready` at line 414. When no Node version satisfies the engine range, the `Remote node <version> does not satisfy required range ...` error from the engine check is suppressed, so the caller falls back to printing only the generic "missing node on PATH" message. This hides the specific version/range diagnostic needed to understand why remote startup failed. Consider having `ensure_remote_node_path` call `remote_node_satisfies_engine` without suppressing stderr on the final check, so the engine-range error surfaces to the caller.

@macroscopeapp

macroscopeapp Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This PR changes runtime behavior in SSH tunneling by altering how node version selection works on remote machines. Additionally, there's an unresolved comment about error message suppression that could hide diagnostic information when node version checks fail.

You can customize Macroscope's approvability policy. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant