fix(ssh): select a satisfying node when the login-shell node fails the engine range#3710
fix(ssh): select a satisfying node when the login-shell node fails the engine range#3710arturskruze wants to merge 1 commit into
Conversation
… 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>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
| fi | ||
|
|
||
| command -v node >/dev/null 2>&1 && remote_node_satisfies_engine | ||
| remote_node_ready |
There was a problem hiding this comment.
🟡 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.
| 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.
ApprovabilityVerdict: 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. |
What
ensure_remote_node_path(inREMOTE_NODE_ENV_SCRIPT) only fell back to version managers (mise/fnm/nodenv/nvm, and the nvm version scan) when there was nonodeonPATHat all (! command -v node). When a login shell already exposes anodethat fails the engine range — e.g. annvm defaultbelow 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
defaultwas22.12.0while a satisfying22.21.1was installed but not selected. Because the server bundle gates startup onimport.meta.main(added in Node 22.16), it then loaded its module graph and exited0silently 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.tsfail loudly on an unsupported node instead of exiting silently — kept out of this PR to stay focused.)Scope / risk
packages/ssh/src/tunnel.test.tsassertions (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_readyhelper in the embeddedREMOTE_NODE_ENV_SCRIPT.Previously,
ensure_remote_node_pathonly ran mise/fnm/nodenv/nvm fallbacks whennodewas missing entirely. If the login shell already had anodethat failed the semver check (e.g. nvmdefaultbelow 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_readyat each step. The nvm version-directory scan stops at the first PATH prefix whereremote_node_readysucceeds instead of depending on glob order. Behavior is unchanged when no node exists or when the currentnodealready 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
remote_node_readyhelper in tunnel.ts that checks both node presence and engine compatibility in one place.ensure_remote_node_pathnow 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.📊 Macroscope summarized 27be28c. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.