fix: detect npm/nvm-installed agents on SSH remotes#366
Open
chr1syy wants to merge 3 commits intoRunMaestro:mainfrom
Open
fix: detect npm/nvm-installed agents on SSH remotes#366chr1syy wants to merge 3 commits intoRunMaestro:mainfrom
chr1syy wants to merge 3 commits intoRunMaestro:mainfrom
Conversation
Contributor
|
believe this would also resolve #403 - may be worth checking the investigation in the issue doesn't uncover any other needed changes. |
Collaborator
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds dynamic detection and prepending of Node version-manager binary directories to the remote PATH used by SSH-invoked commands, via new helper functions that emit single-line and multi-line shell snippets; SSH command builders now incorporate these snippets before running the remote command. (46 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor LocalBuilder as Local
participant SSH as SSHClient
participant Remote as RemoteShell
participant NodeVMs as VersionManagers
LocalBuilder->>SSH: build SSH command (includes BASE dirs + snippet)
SSH->>Remote: open session, send command
Remote->>NodeVMs: run detection snippet (nvm/fnm/volta/mise/asdf/n)
NodeVMs-->>Remote: return detected bin dirs
Remote->>Remote: prepend BASE dirs + detected dirs to PATH
Remote->>Remote: execute provided remote command
Remote-->>SSH: command stdout/stderr/exit
SSH-->>LocalBuilder: relay output and exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/utils/ssh-command-builder.ts (3)
339-345: Redundantexport PATHstatement.Line 341 already exports PATH with
export PATH="...". The version manager lines modify this already-exported variable, so modifications are automatically exported. The finalexport PATHon line 345 is unnecessary.♻️ Remove redundant export
// PATH setup - base directories + dynamic Node version manager detection scriptLines.push( `export PATH="${BASE_SSH_PATH_DIRS.join(':')}:$PATH"` ); // Dynamically detect Node version manager paths (nvm, fnm, volta, etc.) scriptLines.push(...buildNodeVersionManagerPathLines()); - scriptLines.push('export PATH');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/ssh-command-builder.ts` around lines 339 - 345, The final redundant export statement should be removed: the code already exports PATH when pushing `export PATH="${BASE_SSH_PATH_DIRS.join(':')}:$PATH"` and `buildNodeVersionManagerPathLines()` appends modifications to that variable, so the trailing `scriptLines.push('export PATH')` is unnecessary; delete the `scriptLines.push('export PATH')` call in ssh-command-builder.ts to avoid the duplicate export while keeping the existing `scriptLines` entries and the `buildNodeVersionManagerPathLines()` usage intact.
57-61: Consider version-aware sorting for Node directories.Using
ls -rsorts reverse-alphabetically, not by semantic version. For example, directoriesv9.0.0,v18.0.0,v12.0.0would be sorted asv9, v18, v12, resulting inv12being first in PATH—which is neither the newest nor oldest version.Since
current/defaultsymlinks are checked first and typically exist in properly configured environments, this is acceptable as a fallback. However, if predictable version selection matters when no symlink exists, consider sorting by version number.💡 Optional: Version-aware sorting
- for _v in $(ls -r "$_nvm_dir/versions/node/" 2>/dev/null); do + for _v in $(ls -v "$_nvm_dir/versions/node/" 2>/dev/null | tac); doNote:
ls -vdoes natural version sorting (v1 < v2 < v10), andtacreverses to get newest first. However,-vmay not be available on all systems (GNU coreutils). The current approach is more portable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/ssh-command-builder.ts` around lines 57 - 61, The loop that picks node versions uses "for _v in $(ls -r \"$ _nvm_dir/versions/node/\" ...)" which sorts alphabetically rather than by semantic version; update that command string in ssh-command-builder.ts to use a version-aware sort (e.g., replace ls -r with ls -v and pipe through tac to get newest-first, or use ls | sort -V | tac as a more portable alternative) so the PATH picks the intended newest version when no symlink exists. Ensure the change targets the exact string containing 'for _v in $(ls -r "$_nvm_dir/versions/node/" 2>/dev/null); do' so other logic (the surrounding if/fi and PATH assignment) remains unchanged.
605-608: Redundantexport PATHin command string.Since
pathSetupalready includesexport PATH="...", the subsequent; export PATH;in line 608 is redundant.♻️ Remove redundant export
const pathSetup = `export PATH="${BASE_SSH_PATH_DIRS.join(':')}:$PATH"`; const versionManagerSetup = buildNodeVersionManagerPathSnippet(); - const fullBashCommand = `${pathSetup}; ${versionManagerSetup}; export PATH; ${remoteCommand}`; + const fullBashCommand = `${pathSetup}; ${versionManagerSetup}; ${remoteCommand}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/ssh-command-builder.ts` around lines 605 - 608, The fullBashCommand concatenation contains a redundant `export PATH;` because pathSetup already sets PATH; update the construction of fullBashCommand (where pathSetup, versionManagerSetup and remoteCommand are combined) to remove the extra `export PATH;` token—i.e., join BASE_SSH_PATH_DIRS pathSetup, the result of buildNodeVersionManagerPathSnippet() (versionManagerSetup), and remoteCommand with appropriate semicolons/spaces so the final command executes the path setup, version manager setup, then remoteCommand without duplicating `export PATH`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/utils/ssh-command-builder.ts`:
- Around line 339-345: The final redundant export statement should be removed:
the code already exports PATH when pushing `export
PATH="${BASE_SSH_PATH_DIRS.join(':')}:$PATH"` and
`buildNodeVersionManagerPathLines()` appends modifications to that variable, so
the trailing `scriptLines.push('export PATH')` is unnecessary; delete the
`scriptLines.push('export PATH')` call in ssh-command-builder.ts to avoid the
duplicate export while keeping the existing `scriptLines` entries and the
`buildNodeVersionManagerPathLines()` usage intact.
- Around line 57-61: The loop that picks node versions uses "for _v in $(ls -r
\"$ _nvm_dir/versions/node/\" ...)" which sorts alphabetically rather than by
semantic version; update that command string in ssh-command-builder.ts to use a
version-aware sort (e.g., replace ls -r with ls -v and pipe through tac to get
newest-first, or use ls | sort -V | tac as a more portable alternative) so the
PATH picks the intended newest version when no symlink exists. Ensure the change
targets the exact string containing 'for _v in $(ls -r
"$_nvm_dir/versions/node/" 2>/dev/null); do' so other logic (the surrounding
if/fi and PATH assignment) remains unchanged.
- Around line 605-608: The fullBashCommand concatenation contains a redundant
`export PATH;` because pathSetup already sets PATH; update the construction of
fullBashCommand (where pathSetup, versionManagerSetup and remoteCommand are
combined) to remove the extra `export PATH;` token—i.e., join BASE_SSH_PATH_DIRS
pathSetup, the result of buildNodeVersionManagerPathSnippet()
(versionManagerSetup), and remoteCommand with appropriate semicolons/spaces so
the final command executes the path setup, version manager setup, then
remoteCommand without duplicating `export PATH`.
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/utils/ssh-command-builder.ts`:
- Around line 54-70: The nvm/fnm PATH-building loops in ssh-command-builder.ts
are iterating in descending order and using non-portable commands; remove use of
tac in the nvm block (the loop over _nvm_dir versions/node) and instead iterate
using a portable sort to ensure newest versions are prepended to PATH, and
replace ls -r in the fnm loop (the loop over _fnm_dir/node-versions) with a
portable ls | sort -V (or equivalent) so versions are sorted ascending then
prepended to put newest first; make the same edits to the single-line snippet
variant later in the file so both the nvm loop (variable _nvm_dir) and the fnm
loop (variable _fnm_dir) are portable and produce the correct PATH priority.
Comment on lines
54
to
70
| '_nvm_dir="${NVM_DIR:-$HOME/.nvm}"', | ||
| 'if [ -d "$_nvm_dir" ]; then', | ||
| ' [ -d "$_nvm_dir/current/bin" ] && PATH="$_nvm_dir/current/bin:$PATH"', | ||
| ' if [ -d "$_nvm_dir/versions/node" ]; then', | ||
| ' for _v in $(ls "$_nvm_dir/versions/node/" 2>/dev/null | sort -V | tac); do', | ||
| ' [ -d "$_nvm_dir/versions/node/$_v/bin" ] && PATH="$_nvm_dir/versions/node/$_v/bin:$PATH"', | ||
| ' done', | ||
| ' fi', | ||
| 'fi', | ||
| // fnm: check aliases/default, then iterate node-versions | ||
| 'for _fnm_dir in "$HOME/Library/Application Support/fnm" "$HOME/.local/share/fnm" "$HOME/.fnm"; do', | ||
| ' if [ -d "$_fnm_dir" ]; then', | ||
| ' [ -d "$_fnm_dir/aliases/default/bin" ] && PATH="$_fnm_dir/aliases/default/bin:$PATH"', | ||
| ' if [ -d "$_fnm_dir/node-versions" ]; then', | ||
| ' for _v in $(ls -r "$_fnm_dir/node-versions/" 2>/dev/null); do', | ||
| ' [ -d "$_fnm_dir/node-versions/$_v/installation/bin" ] && PATH="$_fnm_dir/node-versions/$_v/installation/bin:$PATH"', | ||
| ' done', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd ssh-command-builder.ts -x wc -l {}Repository: RunMaestro/Maestro
Length of output: 105
🏁 Script executed:
fd ssh-command-builder.ts -x head -n 120 {}Repository: RunMaestro/Maestro
Length of output: 5222
🌐 Web query:
tac command availability macOS BSD default systems
💡 Result:
- macOS (default /usr/bin tools):
tacis not included; the usual built-in equivalent istail -r file(prints the file in reverse line order). You can also install GNU coreutils (e.g., via Homebrew), which provides it asgtac. [2][5] - NetBSD (base system): includes a native
tac(1)(reverse line order; not GNU-option-compatible). [1] - FreeBSD (base system): historically does not include
tac; common guidance is to usetail -rinstead. [3] - GNU/Linux (coreutils):
tacis provided by GNU coreutils. [4] - DragonFly BSD: added a
tac(1)(implemented viatail) in 2020. [6]
Sources: NetBSD tac(1) man page [1], macOS discussion + gtac via coreutils [2], FreeBSD mailing list note [3], GNU coreutils tac docs [4], tail -r on macOS reference [5], DragonFly BSD commit adding tac(1) [6].
PATH priority is inverted due to descending iteration, and tac is not portable to macOS/FreeBSD.
Each loop prepends to PATH, so iterating in descending order (newest-first) actually leaves the oldest version at the front. Additionally, tac is not available by default on macOS or FreeBSD systems, which breaks the script on these platforms. Remove tac from the nvm loop and replace ls -r with ls | sort -V in the fnm loop to ensure the newest version ends up first in PATH and maintain portability across all systems.
Proposed fix
- ' for _v in $(ls "$_nvm_dir/versions/node/" 2>/dev/null | sort -V | tac); do',
+ ' for _v in $(ls "$_nvm_dir/versions/node/" 2>/dev/null | sort -V); do',- ' for _v in $(ls -r "$_fnm_dir/node-versions/" 2>/dev/null); do',
+ ' for _v in $(ls "$_fnm_dir/node-versions/" 2>/dev/null | sort -V); do',Apply the same changes to the single-line snippet version (lines ~103–118).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| '_nvm_dir="${NVM_DIR:-$HOME/.nvm}"', | |
| 'if [ -d "$_nvm_dir" ]; then', | |
| ' [ -d "$_nvm_dir/current/bin" ] && PATH="$_nvm_dir/current/bin:$PATH"', | |
| ' if [ -d "$_nvm_dir/versions/node" ]; then', | |
| ' for _v in $(ls "$_nvm_dir/versions/node/" 2>/dev/null | sort -V | tac); do', | |
| ' [ -d "$_nvm_dir/versions/node/$_v/bin" ] && PATH="$_nvm_dir/versions/node/$_v/bin:$PATH"', | |
| ' done', | |
| ' fi', | |
| 'fi', | |
| // fnm: check aliases/default, then iterate node-versions | |
| 'for _fnm_dir in "$HOME/Library/Application Support/fnm" "$HOME/.local/share/fnm" "$HOME/.fnm"; do', | |
| ' if [ -d "$_fnm_dir" ]; then', | |
| ' [ -d "$_fnm_dir/aliases/default/bin" ] && PATH="$_fnm_dir/aliases/default/bin:$PATH"', | |
| ' if [ -d "$_fnm_dir/node-versions" ]; then', | |
| ' for _v in $(ls -r "$_fnm_dir/node-versions/" 2>/dev/null); do', | |
| ' [ -d "$_fnm_dir/node-versions/$_v/installation/bin" ] && PATH="$_fnm_dir/node-versions/$_v/installation/bin:$PATH"', | |
| ' done', | |
| '_nvm_dir="${NVM_DIR:-$HOME/.nvm}"', | |
| 'if [ -d "$_nvm_dir" ]; then', | |
| ' [ -d "$_nvm_dir/current/bin" ] && PATH="$_nvm_dir/current/bin:$PATH"', | |
| ' if [ -d "$_nvm_dir/versions/node" ]; then', | |
| ' for _v in $(ls "$_nvm_dir/versions/node/" 2>/dev/null | sort -V); do', | |
| ' [ -d "$_nvm_dir/versions/node/$_v/bin" ] && PATH="$_nvm_dir/versions/node/$_v/bin:$PATH"', | |
| ' done', | |
| ' fi', | |
| 'fi', | |
| // fnm: check aliases/default, then iterate node-versions | |
| 'for _fnm_dir in "$HOME/Library/Application Support/fnm" "$HOME/.local/share/fnm" "$HOME/.fnm"; do', | |
| ' if [ -d "$_fnm_dir" ]; then', | |
| ' [ -d "$_fnm_dir/aliases/default/bin" ] && PATH="$_fnm_dir/aliases/default/bin:$PATH"', | |
| ' if [ -d "$_fnm_dir/node-versions" ]; then', | |
| ' for _v in $(ls "$_fnm_dir/node-versions/" 2>/dev/null | sort -V); do', | |
| ' [ -d "$_fnm_dir/node-versions/$_v/installation/bin" ] && PATH="$_fnm_dir/node-versions/$_v/installation/bin:$PATH"', | |
| ' done', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/utils/ssh-command-builder.ts` around lines 54 - 70, The nvm/fnm
PATH-building loops in ssh-command-builder.ts are iterating in descending order
and using non-portable commands; remove use of tac in the nvm block (the loop
over _nvm_dir versions/node) and instead iterate using a portable sort to ensure
newest versions are prepended to PATH, and replace ls -r in the fnm loop (the
loop over _fnm_dir/node-versions) with a portable ls | sort -V (or equivalent)
so versions are sorted ascending then prepended to put newest first; make the
same edits to the single-line snippet variant later in the file so both the nvm
loop (variable _nvm_dir) and the fnm loop (variable _fnm_dir) are portable and
produce the correct PATH priority.
SSH remote agent detection and execution used a hardcoded PATH that only included static directories like ~/.local/bin and /usr/local/bin. This meant agents installed via Node version managers (nvm, fnm, volta, mise, asdf, n) were not found on remote hosts. Add dynamic Node version manager PATH detection to the SSH command builder. Both buildSshCommand() and buildSshCommandWithStdin() now probe the remote host for version manager installations and prepend their bin directories to PATH before executing commands. This mirrors the existing local detection logic in pathUtils.ts detectNodeVersionManagerBinPaths() but runs as shell commands on the remote host rather than Node.js filesystem calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove duplicate 'export PATH' statement in buildSshCommandWithStdin (line 345) - Remove redundant 'export PATH' from fullBashCommand in buildSshCommand (line 608) - Fix NVM version sorting to use semantic versioning (sort -V | tac) instead of lexicographic (ls -r) - This ensures the newest Node version is picked when no symlink exists - Applies to both buildNodeVersionManagerPathLines and buildNodeVersionManagerPathSnippet - Clean up unnecessary line continuations in pathSetup variable These fixes ensure PATH setup is properly deduplicated while maintaining correct version-aware selection for nvm, fnm, volta, mise, asdf, and n on SSH remotes.
Replace non-portable 'tac' and 'ls -r' with portable 'sort -rV' in both multi-line and single-line versions of nvm and fnm PATH-building loops. This ensures correct version prioritization (newest first) across all Unix-like systems without requiring GNU-specific tools.
chr1syy
force-pushed
the
fix-ssh-codex
branch
from
fbe20d3 to
e084077
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.nvm/versions/node/v22.21.1/bin/)npmthrough nvm, fnm, volta, mise, asdf, or n were not detected on remote hostsbuildSshCommand()andbuildSshCommandWithStdin(), mirroring the existing local detection logic indetectNodeVersionManagerBinPaths()Details
The SSH command builder in
src/main/utils/ssh-command-builder.tshad a hardcoded PATH that only included static directories:This meant
which codex(used for agent detection) and agent execution both failed on remotes where the agent was installed via npm/nvm into a path like/home/user/.nvm/versions/node/v22.21.1/bin/codex.The fix adds shell snippets that dynamically probe the remote host for version manager installations:
$NVM_DIR/current/bin+ all$NVM_DIR/versions/node/v*/binaliases/default/bin+ allnode-versions/v*/installation/bin~/.volta/bin~/.local/share/mise/shims~/.asdf/shims$N_PREFIX/binTwo variants are provided:
buildNodeVersionManagerPathLines()— multi-line script for stdin-based SSH executionbuildNodeVersionManagerPathSnippet()— compact single-line version forbash -c '...'Test plan
ssh-command-buildertests passssh-remote-resolvertests pass🤖 Generated with Claude Code
Summary by CodeRabbit