fix(RemoteTreeBrowser): add sh -c prefix (#65)#66
Conversation
We use `add-sh-prefix` branch to test the functionality.
# PR Summary: Fix RemoteTreeBrowser for Fish Shell Compatibility
## Problem
RemoteTreeBrowser fails with "Failed to load initial tree" when the
remote server uses fish shell as the default shell. The shell commands
used POSIX/Bash-specific syntax (e.g., ${f#./}) that fish doesn't
support.
## Solution
Wrap all shell commands with sh -c '...' to ensure execution in a
POSIX-compatible shell, regardless of the user's login shell.
## New API
```lua
-- lua/async-remote-write/ssh_utils.lua
ssh_utils.build_list_dir_cmd(path, opts)
-- opts.sorted: boolean (default: true)
```
## Key Change
Before:
```lua
local cmd = 'cd ' .. path .. ' && find . -maxdepth 1 | while read f; do ...'
```
After:
```lua
local cmd = ssh_utils.build_list_dir_cmd(path)
-- Generates: sh -c '...' _ '/path/to/dir'
```
|
@YaQia I've tested the changed and will merge this into main branch. Thanks again for the contribution! |
There was a problem hiding this comment.
Pull request overview
This PR fixes RemoteTreeBrowser failures on remotes where the user’s login shell is fish by ensuring directory listing commands run under a POSIX sh via sh -c.
Changes:
- Introduces
ssh_utils.build_list_dir_cmd(path, opts)to generate POSIX-compatible directory listing commands usingsh -c. - Refactors tree browsing and remote directory browsing to use the new helper instead of inlined shell snippets.
- Updates tests to assert the new command structure and argument-passing behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lua/async-remote-write/ssh_utils.lua |
Adds build_list_dir_cmd and embedded shell scripts to run listings via sh -c with $1 for paths. |
lua/async-remote-write/tree_browser.lua |
Switches directory listing command construction to ssh_utils.build_list_dir_cmd. |
lua/async-remote-write/browse.lua |
Replaces several inlined listing scripts with ssh_utils.build_list_dir_cmd(..., { sorted = false }). |
tests/test_ssh_robust_connection.lua |
Updates test to use build_list_dir_cmd and assert sh -c usage. |
tests/test_ssh_command_escaping.lua |
Refactors escaping tests to use build_list_dir_cmd and adds assertions around $1 argument usage. |
tests/test_file_browser_ssh.lua |
Removes local ssh_utils mock and uses real ssh_utils + updated assertions for sh -c. |
tests/test_file_browser_debug.lua |
Updates debug tests to use build_list_dir_cmd and adjusts localhost SSH cmd mock argument naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- Use ssh_utils helper to build directory listing command | ||
| local sh_cmd = ssh_utils.build_list_dir_cmd(path, { sorted = false }) |
There was a problem hiding this comment.
sorted = false currently selects LIST_DIR_SCRIPT_UNSORTED, which removes sorting entirely. In the code being replaced here, the output was sorted (there was a | sort), so this is a behavior change that can affect UI ordering. If the intent is to preserve existing sorted output, call ssh_utils.build_list_dir_cmd(path) (default sorted) or pass { sorted = true }.
| -- Use ssh_utils helper to build directory listing command | |
| local sh_cmd = ssh_utils.build_list_dir_cmd(path, { sorted = false }) | |
| -- Use ssh_utils helper to build directory listing command (default: sorted) | |
| local sh_cmd = ssh_utils.build_list_dir_cmd(path) |
| -- Uses sh -c to ensure POSIX shell compatibility (works with fish, zsh, etc.) | ||
| -- Uses IFS= read -r to handle filenames with spaces/backslashes | ||
| local LIST_DIR_SCRIPT = [[ | ||
| cd "$1" && find . -maxdepth 1 | sort | while IFS= read -r f; do |
There was a problem hiding this comment.
Using cd \"$1\" can mis-handle paths that begin with - (e.g. - and -P are treated specially by some shells). To make this robust for all valid directory names, use cd -- \"$1\".
|
|
||
| -- Shell script for listing directory contents (unsorted, with error suppression) | ||
| local LIST_DIR_SCRIPT_UNSORTED = [[ | ||
| cd "$1" 2>/dev/null && find . -maxdepth 1 -not -name "." | while IFS= read -r f; do |
There was a problem hiding this comment.
Same issue as the sorted script: cd \"$1\" can treat paths starting with - as options/special args in some shells. Use cd -- \"$1\" (keeping the 2>/dev/null redirection if desired).
| --- Build a shell command for listing directory contents via SSH | ||
| --- @param path string The remote directory path | ||
| --- @param opts? {sorted?: boolean} Options: sorted (default true) | ||
| --- @return string The shell command to execute | ||
| function M.build_list_dir_cmd(path, opts) | ||
| opts = opts or {} | ||
| local sorted = opts.sorted ~= false -- default to true | ||
| local script = sorted and LIST_DIR_SCRIPT or LIST_DIR_SCRIPT_UNSORTED |
There was a problem hiding this comment.
The opts.sorted flag does more than control sorting: when sorted is false it also changes behaviors like suppressing cd errors (2>/dev/null) and filtering out . via -not -name \".\". This makes the option name misleading for callers. Consider splitting this into separate options (e.g. opts.sorted, opts.suppress_errors, opts.exclude_dot) or keeping the semantics aligned so sorted only affects sorting.
| --- @param path string The remote directory path | ||
| --- @param opts? {sorted?: boolean} Options: sorted (default true) | ||
| --- @return string The shell command to execute | ||
| function M.build_list_dir_cmd(path, opts) |
There was a problem hiding this comment.
There are updated tests for the default command shape, but no test coverage validating the opts.sorted = false branch (e.g., that it includes 2>/dev/null, omits sort, and excludes .). Adding a focused test for the sorted=false behavior would help prevent accidental behavior changes.
We use
add-sh-prefixbranch to test the functionality.PR Summary: Fix RemoteTreeBrowser for Fish Shell Compatibility ## Problem
RemoteTreeBrowser fails with "Failed to load initial tree" when the remote server uses fish shell as the default shell. The shell commands used POSIX/Bash-specific syntax (e.g., ${f#./}) that fish doesn't support.
Solution
Wrap all shell commands with sh -c '...' to ensure execution in a POSIX-compatible shell, regardless of the user's login shell. ## New API
Key Change
Before:
After: