[sftp] add support#8
Merged
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🤖 Pull request artifacts
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds SFTP protocol support alongside FTP: new sftp client, auth/config parsing, refactored FTP client into an ftp package, centralized error types, scheme-based client factory, CLI/docs updates, and dependency/tooling bumps. ChangesSFTP protocol support and FTP refactor
Sequence Diagram(s)sequenceDiagram
participant Factory as Client Factory
participant FTPClient as ftp.Client
participant SFTPClient as sftp.Client
participant SSH as SSH (net/ssh)
Factory->>Factory: Parse destination URL
alt scheme == ftp
Factory->>FTPClient: NewClient(url, logger)
FTPClient->>FTPClient: init() — dial FTP, login, ChangeDir if needed
else scheme == sftp
Factory->>SFTPClient: NewClient(url, logger)
SFTPClient->>SFTPClient: init() on first operation (ping)
SFTPClient->>SSH: Dial TCP, authenticate (agent/key/password), create SFTP session
SSH->>SFTPClient: return *sftp.Client
else unsupported
Factory->>Factory: Return wrapped ErrUnsupportedScheme
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
77-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate About section to include SFTP support.
Line 81 still says “remote FTP server,” which conflicts with the updated FTP+SFTP positioning in the rest of the README.
Suggested patch
-sftp-sync is a command-line utility for syncing a local folder with a remote FTP server on every change of files or directories. +sftp-sync is a command-line utility for syncing a local folder with a remote FTP or SFTP server on every change of files or directories.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 77 - 82, The About section still calls out "remote FTP server"; update the text under "About The Project" (the sftp-sync project description) to reflect SFTP support by replacing that phrase with a combined term such as "remote FTP or SFTP server" or "remote FTP/SFTP server" so it matches the README's FTP+SFTP positioning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/sftp/sftp.go`:
- Around line 70-72: The current parsing in sftp.go that checks if
strings.Contains(host, ":") and then does strings.Cut(host, ":") incorrectly
splits IPv6 addresses; replace that logic by using the URL object's helpers:
call u.Hostname() to get the host (with IPv6 brackets removed) and u.Port() to
get the port string (or blank if none) and assign to the existing host and port
variables (fall back to default port if u.Port() is empty). Update references
around variables named host, port and the URL variable u to use these methods
rather than manual string slicing.
- Around line 159-166: The recursive delete in removeDirRecur is listing the
parent directory because it calls c.sftp.ReadDirContext(ctx, path.Dir(p))
instead of the target path; update the ReadDirContext call in
Client.removeDirRecur to use p (the directory being removed) so entries are read
from the correct directory, and ensure any subsequent path joins or recursive
calls (inside removeDirRecur) continue to operate on p and its child paths
rather than path.Dir(p).
- Line 82: Replace the insecure HostKeyCallback currently set to
ssh.InsecureIgnoreHostKey() in the SSH client configuration with a known-hosts
based callback using knownhosts.New. Import golang.org/x/crypto/ssh/knownhosts
and build the known_hosts path (e.g., filepath.Join(os.UserHomeDir(), ".ssh",
"known_hosts")), call knownhosts.New(...) to get the callback, handle any error
(return or log) from knownhosts.New, and set the config's HostKeyCallback to the
returned callback instead of ssh.InsecureIgnoreHostKey(); update imports to
include os and path/filepath as needed.
- Around line 274-279: Update the error-matching logic around the notFoundErrors
slice: remove the overly broad "failure" entry and the case-sensitive "No such
file", normalize the error string with strings.ToLower(err.Error()) before
comparing, and additionally check os.IsNotExist(err) to reliably detect
file-not-found cases; i.e., keep only specific patterns like "does not exist",
"not found", "no such file" in notFoundErrors, use strings.ToLower when scanning
those patterns, and short-circuit to treat the error as a not-found only if
strings match or os.IsNotExist(err) returns true (do not suppress permission or
connection errors).
---
Outside diff comments:
In `@README.md`:
- Around line 77-82: The About section still calls out "remote FTP server";
update the text under "About The Project" (the sftp-sync project description) to
reflect SFTP support by replacing that phrase with a combined term such as
"remote FTP or SFTP server" or "remote FTP/SFTP server" so it matches the
README's FTP+SFTP positioning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a61725ef-8cc6-495c-be81-e98004aebbec
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/sftp/sftp.gointernal/client/types/errors.gomain.go
capcom6
force-pushed
the
sftp/add-support
branch
2 times, most recently
from
533b055 to
bb43881
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/ftp/ftp.go (1)
76-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose and discard partially initialized FTP sessions on setup failure.
If
LoginorChangeDirfails, the new connection remains assigned toc.client. A laterinit()can passping()and skip setup, causing operations to run in the wrong remote directory.Suggested fix
- c.client, err = ftp.Dial(u.Host, ftp.DialWithContext(ctx)) + conn, err := ftp.Dial(u.Host, ftp.DialWithContext(ctx)) if err != nil { return fmt.Errorf("can't connect to %s: %w", u.Host, err) } @@ - if loginErr := c.client.Login(u.User.Username(), password); loginErr != nil { + if loginErr := conn.Login(u.User.Username(), password); loginErr != nil { + _ = conn.Quit() return fmt.Errorf("can't login as %s: %w", u.User.Username(), loginErr) } if u.Path != "" && u.Path != "/" { - if chErr := c.client.ChangeDir(u.Path); chErr != nil { + if chErr := conn.ChangeDir(u.Path); chErr != nil { + _ = conn.Quit() return fmt.Errorf("remote path %s does not exist or is not accessible: %w", u.Path, chErr) } } + + c.client = conn🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/ftp/ftp.go` around lines 76 - 83, When setup fails during ftp session initialization (calls to c.client.Login or c.client.ChangeDir), ensure the partially-initialized c.client is closed and discarded so subsequent init()/ping() do not reuse it; on both the Login failure branch and the ChangeDir failure branch call c.client.Close() (handle/ignore Close() error as appropriate) and then set c.client = nil before returning the wrapped error from Login or ChangeDir, so the client state is cleaned up on setup failure.
🧹 Nitpick comments (3)
internal/client/sftp/sftp_test.go (1)
49-69: ⚡ Quick winTest currently validates duplicated logic, not implementation behavior.
TestURLPathValidationRulesre-derives the rule inline, so it won’t catch regressions in the actual SFTP path-handling code. Please assert this through the real path-validation/dir-selection behavior (or a helper used by production code) instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp_test.go` around lines 49 - 69, The test is re-deriving the path rule instead of exercising the real code; update TestURLPathValidationRules to call the actual production path-validation or dir-selection code (replace the inline computation `shouldUse := tt.path != "" && tt.path != "/"` in TestURLPathValidationRules with a call to the helper/function used by the SFTP client to decide whether to change directories) and assert that its return matches tt.shouldUse; if the decision is encapsulated in a method on the SFTP client, construct the minimal client or invoke the exported helper so the test validates implementation behavior rather than duplicated logic.internal/client/sftp/sftp.go (2)
92-96: 💤 Low valueConsider adding a connection timeout.
The SSH
ClientConfiglacks aTimeoutfield. While OS-level TCP timeouts exist, an explicit timeout provides predictable behavior and prevents indefinite hangs against unresponsive servers.♻️ Proposed fix
config := &ssh.ClientConfig{ User: username, Auth: authMethods, HostKeyCallback: hostKeyCallback, + Timeout: 30 * time.Second, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp.go` around lines 92 - 96, The ssh.ClientConfig named config currently has no connection timeout; to avoid hangs, implement a TCP-level timeout by creating a net.Dialer with a reasonable Timeout and use dialer.Dial to open the TCP connection and then wrap it with ssh.NewClientConn and ssh.NewClient (instead of calling ssh.Dial). Update the code path that uses config (the SFTP connection setup / connect function where config is passed) to use this dialer-based flow so the SSH handshake respects the configured timeout.
81-84: ⚡ Quick winUse
os.UserHomeDir()for cross-platform compatibility.
os.Getenv("HOME")doesn't work on Windows and may be empty in some environments.os.UserHomeDir()handles platform differences correctly.♻️ Proposed fix
- hostKeyCallback, err := knownhosts.New(filepath.Join(os.Getenv("HOME"), ".ssh", "known_hosts")) + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("can't determine home directory: %w", err) + } + hostKeyCallback, err := knownhosts.New(filepath.Join(home, ".ssh", "known_hosts")) if err != nil { return fmt.Errorf("can't load known_hosts: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp.go` around lines 81 - 84, Replace the use of os.Getenv("HOME") when building the known_hosts path with os.UserHomeDir() and handle its error: call os.UserHomeDir(), check for an error, then use filepath.Join(userHome, ".ssh", "known_hosts") when calling knownhosts.New (the hostKeyCallback initialization). Ensure any error from os.UserHomeDir() is returned (similar to the existing error handling around knownhosts.New) so the function fails cleanly if the home directory can't be resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/sftp/auth.go`:
- Around line 84-100: The buildAgentSigners function leaks the agent socket
connection because conn is never closed; after successfully dialing the SSH
agent in buildAgentSigners (the conn returned by dialer.DialContext) defer
conn.Close() immediately (or otherwise close conn before each return) so the
file descriptor is released; ensure both the success path (after agent.NewClient
and Signers()) and the error paths close the connection by placing the defer
right after the dial succeeds.
---
Outside diff comments:
In `@internal/client/ftp/ftp.go`:
- Around line 76-83: When setup fails during ftp session initialization (calls
to c.client.Login or c.client.ChangeDir), ensure the partially-initialized
c.client is closed and discarded so subsequent init()/ping() do not reuse it; on
both the Login failure branch and the ChangeDir failure branch call
c.client.Close() (handle/ignore Close() error as appropriate) and then set
c.client = nil before returning the wrapped error from Login or ChangeDir, so
the client state is cleaned up on setup failure.
---
Nitpick comments:
In `@internal/client/sftp/sftp_test.go`:
- Around line 49-69: The test is re-deriving the path rule instead of exercising
the real code; update TestURLPathValidationRules to call the actual production
path-validation or dir-selection code (replace the inline computation `shouldUse
:= tt.path != "" && tt.path != "/"` in TestURLPathValidationRules with a call to
the helper/function used by the SFTP client to decide whether to change
directories) and assert that its return matches tt.shouldUse; if the decision is
encapsulated in a method on the SFTP client, construct the minimal client or
invoke the exported helper so the test validates implementation behavior rather
than duplicated logic.
In `@internal/client/sftp/sftp.go`:
- Around line 92-96: The ssh.ClientConfig named config currently has no
connection timeout; to avoid hangs, implement a TCP-level timeout by creating a
net.Dialer with a reasonable Timeout and use dialer.Dial to open the TCP
connection and then wrap it with ssh.NewClientConn and ssh.NewClient (instead of
calling ssh.Dial). Update the code path that uses config (the SFTP connection
setup / connect function where config is passed) to use this dialer-based flow
so the SSH handshake respects the configured timeout.
- Around line 81-84: Replace the use of os.Getenv("HOME") when building the
known_hosts path with os.UserHomeDir() and handle its error: call
os.UserHomeDir(), check for an error, then use filepath.Join(userHome, ".ssh",
"known_hosts") when calling knownhosts.New (the hostKeyCallback initialization).
Ensure any error from os.UserHomeDir() is returned (similar to the existing
error handling around knownhosts.New) so the function fails cleanly if the home
directory can't be resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a065f693-732f-40b6-a4dd-d0c5dc4050b2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/ftp/ftp_test.gointernal/client/sftp/auth.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.gointernal/client/types/errors.gomain.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- internal/cli/commands/sync/sync.go
- .golangci.yml
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/ftp/ftp.go (1)
71-78:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
u.Userbefore callingPassword()/Username()in FTP login to avoid nil panic.
internal/client/ftp/ftp.godereferencesu.User.Password()andu.User.Username()(forconn.Loginand error formatting) without anyu.User == nilguard; a URL without userinfo (e.g.,ftp://host/path) can therefore panic during connection initialization. Add a validation check and return an invalid-params error consistent with existing error handling in this package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/ftp/ftp.go` around lines 71 - 78, Guard against a nil User before calling Password() and Username(): check if u.User == nil at the start of the FTP login sequence in internal/client/ftp/ftp.go and return the package's standard "invalid-params" error (consistent with existing error handling) instead of proceeding; then use local username/password variables (from u.User.Username() and u.User.Password()) when calling conn.Login and when formatting the login error so no nil dereference can occur during conn.Login or the error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/sftp/sftp.go`:
- Around line 83-97: Before constructing ssh.ClientConfig and calling ssh.Dial,
validate that ParseConfigFromURL returned non-empty config.Host and
config.Username and return types.ErrInvalidParams if either is empty;
specifically, check config.Host and config.Username (derived by
ParseConfigFromURL from u.Hostname() and u.User.Username()), and only proceed to
build ssh.ClientConfig and call ssh.Dial(net.JoinHostPort(config.Host,
config.Port), sshConfig) when both are present — otherwise return
types.ErrInvalidParams to fail fast on malformed SFTP URLs.
---
Outside diff comments:
In `@internal/client/ftp/ftp.go`:
- Around line 71-78: Guard against a nil User before calling Password() and
Username(): check if u.User == nil at the start of the FTP login sequence in
internal/client/ftp/ftp.go and return the package's standard "invalid-params"
error (consistent with existing error handling) instead of proceeding; then use
local username/password variables (from u.User.Username() and u.User.Password())
when calling conn.Login and when formatting the login error so no nil
dereference can occur during conn.Login or the error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f717e7f-d1d6-40ec-8c26-fbec69f16df0
📒 Files selected for processing (5)
internal/client/ftp/ftp.gointernal/client/sftp/auth.gointernal/client/sftp/config.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.go
💤 Files with no reviewable changes (1)
- internal/client/sftp/sftp_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/ftp/ftp.go`:
- Around line 71-80: The code currently skips Login when u.User is nil which
allows init to succeed unauthenticated and fail later; change the logic in the
FTP initialization (the block referencing u.User, conn.Login and conn.Quit) to
fail fast: if u.User is nil either perform an explicit anonymous login via
conn.Login("anonymous","") or return a clear error immediately (e.g., "missing
FTP credentials"), and ensure you still call conn.Quit() on error and wrap the
login error (as with fmt.Errorf("can't login as %s: %w", ...)) for consistent
diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34107b97-ff55-4dce-8436-f765d5143714
📒 Files selected for processing (3)
internal/client/ftp/ftp.gointernal/client/sftp/config.gointernal/client/sftp/sftp.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/client/sftp/sftp.go (1)
306-325: 💤 Low valueConsider case-insensitive matching and
os.IsNotExist()for robustness.The "failure" pattern is correctly removed. For additional robustness, consider normalizing error strings with
strings.ToLower()and addingos.IsNotExist()check for wrapped filesystem errors:♻️ Suggested improvement
func isIgnorableError(err error) bool { if err == nil { return true } + if os.IsNotExist(err) { + return true + } - errStr := err.Error() + errStr := strings.ToLower(err.Error()) notFoundErrors := []string{ "does not exist", "not found", - "No such file", + "no such file", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp.go` around lines 306 - 325, The isIgnorableError function only checks error string substrings case-sensitively; update it to normalize the error text with strings.ToLower before matching and add a filesystem-aware check using os.IsNotExist(err) (which also handles wrapped errors) so that the notFoundErrors slice (used in isIgnorableError) compares against lowercase patterns and the function returns true when os.IsNotExist(err) is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/sftp/config.go`:
- Around line 18-21: ParseConfigFromURL dereferences the incoming *url.URL (u)
without a nil check which can cause a panic; update ParseConfigFromURL to first
validate that u != nil and if nil return a typed validation error (e.g.,
wrap/return types.ErrInvalidParams with a clear message like "sftp URL is nil"),
then proceed to use u.Hostname() and construct Config as before so callers
receive a proper error instead of a panic.
In `@internal/client/sftp/sftp.go`:
- Around line 93-102: The ssh.Dial call in the sftp connection path can hang
because it lacks a TCP connect timeout; replace the single ssh.Dial(...) usage
with an explicit net.DialTimeout to establish the TCP connection (using
config.Host and config.Port) and then pass that net.Conn into ssh.NewClientConn
along with the existing ssh.ClientConfig to create the SSH client; update where
conn is created/used (currently from ssh.Dial) to handle the net.DialTimeout
error, close the underlying net.Conn on error, and ensure the timeout value is
configurable or set to a sensible default.
---
Nitpick comments:
In `@internal/client/sftp/sftp.go`:
- Around line 306-325: The isIgnorableError function only checks error string
substrings case-sensitively; update it to normalize the error text with
strings.ToLower before matching and add a filesystem-aware check using
os.IsNotExist(err) (which also handles wrapped errors) so that the
notFoundErrors slice (used in isIgnorableError) compares against lowercase
patterns and the function returns true when os.IsNotExist(err) is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a220454f-15db-4795-9277-e032f1cc7f37
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/ftp/ftp_test.gointernal/client/sftp/auth.gointernal/client/sftp/config.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.gointernal/client/types/errors.gomain.go
✅ Files skipped from review due to trivial changes (2)
- internal/cli/commands/sync/sync.go
- README.md
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
internal/client/sftp/sftp_test.go (1)
8-47: ⚡ Quick winPoint these cases at the project code, not
net/url.This table mostly re-verifies
url.Parse, so it won't catch regressions inParseConfigFromURL,ParseAuthFromURL, or the base-path handling added inClient. Reusing the same inputs against your parser/helper layer would give this test real coverage of the PR behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp_test.go` around lines 8 - 47, The test currently only exercises net/url.Parse; update it to call the project parsing helpers so it verifies our behavior: replace the direct url.Parse usage in TestURLPathExtraction with calls to ParseConfigFromURL (and where relevant ParseAuthFromURL) and/or instantiate a Client (to validate its base-path handling) using the same rawURL inputs, then assert the returned config.Path (or client.BasePath) and auth fields equal the expected wantPath and expected auth values; keep the existing table of cases but assert against ParseConfigFromURL/ParseAuthFromURL/Client behavior instead of u.Path so regressions in our parsing and base-path logic are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 3: The README's prerequisites still list Go 1.24.3 while the module
requires Go 1.25.0; update the documented minimum Go version in the README (and
any other docs or CI/readme snippets) to 1.25.0 so it matches the go.mod
requirement and avoid setup confusion.
In `@internal/client/ftp/ftp.go`:
- Around line 82-85: The failure branch that handles changing to the remote base
path (where conn.ChangeDir(u.Path) returns chErr) needs to wrap the returned
error with the sentinel types.ErrInvalidPath so callers can detect invalid
configured paths; update the error return in the ftp.go branch that currently
returns fmt.Errorf("remote path %s does not exist or is not accessible: %w",
u.Path, chErr) to wrap chErr with types.ErrInvalidPath (e.g., using
fmt.Errorf("%w: remote path %s does not exist or is not accessible: %v",
types.ErrInvalidPath, u.Path, chErr) or errors.Join/Wrap as appropriate) so
errors.Is(err, types.ErrInvalidPath) will succeed while preserving the original
chErr.
- Around line 66-68: The FTP dial should default to port 21 when the URL omits
one: before calling ftp.Dial in the function where conn is created (the block
that currently calls ftp.Dial(u.Host, ...)), check u.Host for an explicit port
and, if missing, build a host:port using net.JoinHostPort(u.Host, "21"); then
pass that resulting host:port string to ftp.Dial. Also add an import for net.
This ensures ftp.Dial receives host:port (hostname:21) when the URL has no port.
In `@internal/client/sftp/auth.go`:
- Around line 64-75: When privateKey is empty, don't probe default keys in a way
that can short-circuit explicit password/agent auth: only attempt
findDefaultKeyPath() and parseKeyAtPath(...) when no other auth methods have
been appended (i.e., methods is empty and no explicit password/agent is
configured). Move or guard the default-key discovery in the auth assembly so
that if a.keyPass (password) or agent-based auth is present you skip default
lookup; if you do probe and parseKeyAtPath fails, treat it as a non-fatal,
best-effort failure (do not return the error) so password/agent auth can still
be used. Ensure you still append ssh.PublicKeys(signer) when a privateKey
(explicit) is provided and preserve returning a parse error for an explicitly
specified key.
In `@internal/client/sftp/sftp.go`:
- Around line 165-169: The resolvePath method currently returns an empty string
when remotePath is empty even if c.basePath is configured, causing
Remove/RemoveDir to act on session cwd and UploadFile to try creating an empty
path; update resolvePath (method on Client) so that if remotePath == "" and
c.basePath != "" it returns c.basePath, otherwise fall back to returning
remotePath (and still return remotePath when c.basePath == ""); adjust callers
Remove, RemoveDir, and UploadFile to rely on the corrected resolvePath behavior
or validate/reject empty remotePath before destructive operations if desired.
In `@README.md`:
- Around line 157-179: Replace examples that embed secrets in the --dest URL
(the "FTP", "SFTP (password)" examples and any other occurrences) with versions
that avoid inline passwords/passphrases and instead demonstrate safer options
(SSH agent or key-based auth) and/or use placeholder tokens like
<PASSWORD_REDACTED>; add a short warning note above the examples advising not to
put secrets on the command line because they can be exposed via shell history
and process lists, and update the "SFTP (SSH key)" and "SFTP (SSH agent)"
examples to be the primary shown patterns (referencing the sftp-sync --dest
examples and query params ?key= and ?agent=true for exact placements).
---
Nitpick comments:
In `@internal/client/sftp/sftp_test.go`:
- Around line 8-47: The test currently only exercises net/url.Parse; update it
to call the project parsing helpers so it verifies our behavior: replace the
direct url.Parse usage in TestURLPathExtraction with calls to ParseConfigFromURL
(and where relevant ParseAuthFromURL) and/or instantiate a Client (to validate
its base-path handling) using the same rawURL inputs, then assert the returned
config.Path (or client.BasePath) and auth fields equal the expected wantPath and
expected auth values; keep the existing table of cases but assert against
ParseConfigFromURL/ParseAuthFromURL/Client behavior instead of u.Path so
regressions in our parsing and base-path logic are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74345cde-7d11-4c5d-a3ca-2f8e73a9a402
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/ftp/ftp_test.gointernal/client/sftp/auth.gointernal/client/sftp/config.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.gointernal/client/types/errors.gomain.go
✅ Files skipped from review due to trivial changes (1)
- main.go
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/sftp/auth.go (1)
64-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThis guard disables key-based auth entirely.
After
keyPathis populated above, this branch only runs when it is still empty, soparseKeyAtPathis called with""while explicit?key=...and discovered default keys are skipped. Key-only SFTP URLs will therefore never appendssh.PublicKeys(...).🐛 Minimal fix
keyPath := a.privateKey -if keyPath == "" { +if keyPath == "" && len(methods) == 0 && a.password == "" { keyPath = findDefaultKeyPath() } -if keyPath == "" && len(methods) == 0 && a.password == "" { +if keyPath != "" { signer, keyErr := parseKeyAtPath(keyPath, a.keyPass) if keyErr != nil { return nil, keyErr }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/auth.go` around lines 64 - 75, The current guard incorrectly checks keyPath == "" so parseKeyAtPath is only attempted when no key exists; change the condition to attempt key parsing when a key path is present (whether from a.privateKey or findDefaultKeyPath()) by replacing the keyPath empty check with a non-empty check so that when keyPath != "" and len(methods) == 0 and a.password == "" you call parseKeyAtPath(keyPath, a.keyPass) and append ssh.PublicKeys(signer); refer to the keyPath variable, findDefaultKeyPath(), parseKeyAtPath(), methods slice, and a.password/a.privateKey to locate and fix the logic.
♻️ Duplicate comments (1)
README.md (1)
194-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid normalizing inline secret usage in URL examples.
Even with the security note, examples still show inline password/passphrase patterns (
username:password,key_pass=mysecret). Prefer redacted placeholders (e.g.,<PASSWORD_REDACTED>,<PASSPHRASE_REDACTED>) and keep key/agent examples as primary.Also applies to: 205-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 194 - 199, Update the README `--dest` URL examples to avoid inline secrets by replacing `username:password` and `key_pass=<passphrase>` with redacted placeholders (e.g., `<PASSWORD_REDACTED>`, `<PASSPHRASE_REDACTED>`), keep the SFTP SSH key and agent examples as the preferred patterns, and apply the same redactions to the other set of examples around lines 205-209; locate the `--dest` examples and adjust the URL strings accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/ftp/ftp.go`:
- Around line 67-70: Reject empty FTP host before building the dial address: in
the code block around host := u.Host (function handling FTP URIs in
internal/client/ftp/ftp.go) add a guard that checks if u.Hostname() == "" and
immediately return types.ErrInvalidParams (rather than proceeding to
net.JoinHostPort or ftp.Dial); keep the existing logic that defaults port to
"21" only after this hostname validation so ftp.Dial never receives ":21" or
":2121" with an empty host.
In `@internal/client/sftp/sftp.go`:
- Around line 165-172: The resolvePath function can allow escaping basePath via
parent segments; change resolvePath to first clean remotePath (remove leading
"/" so it stays relative), join with c.basePath using path.Join, then verify the
resulting path does not escape by ensuring the joined result equals c.basePath
or has the c.basePath + "/" prefix (use strings.HasPrefix on the joined value);
if the check fails, return c.basePath (or otherwise clamp to basePath) so any
../ or absolute inputs cannot traverse outside the configured basePath.
Reference: Client.resolvePath and Client.basePath.
In `@README.md`:
- Around line 108-110: Replace the FTP-only prerequisite text "Access to an FTP
server with valid credentials" in the README (the prerequisite list around the
Go version entry) with the updated string "Access to an FTP or SFTP server with
valid credentials." to reflect support for both protocols.
---
Outside diff comments:
In `@internal/client/sftp/auth.go`:
- Around line 64-75: The current guard incorrectly checks keyPath == "" so
parseKeyAtPath is only attempted when no key exists; change the condition to
attempt key parsing when a key path is present (whether from a.privateKey or
findDefaultKeyPath()) by replacing the keyPath empty check with a non-empty
check so that when keyPath != "" and len(methods) == 0 and a.password == "" you
call parseKeyAtPath(keyPath, a.keyPass) and append ssh.PublicKeys(signer); refer
to the keyPath variable, findDefaultKeyPath(), parseKeyAtPath(), methods slice,
and a.password/a.privateKey to locate and fix the logic.
---
Duplicate comments:
In `@README.md`:
- Around line 194-199: Update the README `--dest` URL examples to avoid inline
secrets by replacing `username:password` and `key_pass=<passphrase>` with
redacted placeholders (e.g., `<PASSWORD_REDACTED>`, `<PASSPHRASE_REDACTED>`),
keep the SFTP SSH key and agent examples as the preferred patterns, and apply
the same redactions to the other set of examples around lines 205-209; locate
the `--dest` examples and adjust the URL strings accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb5304a8-c65d-4185-a7e3-d4902fe8ce1c
📒 Files selected for processing (4)
README.mdinternal/client/ftp/ftp.gointernal/client/sftp/auth.gointernal/client/sftp/sftp.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/sftp/auth.go (1)
40-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential nil pointer dereference if
u.Useris nil.
ParseAuthFromURLis exported and could be called directly with a URL whereUseris nil, causing a panic atu.User.Password(). WhileParseConfigFromURLvalidates this upstream, adding a defensive check improves robustness.🛡️ Proposed fix
func ParseAuthFromURL(u *url.URL) AuthConfig { cfg := AuthConfig{ password: "", privateKey: "", keyPass: "", useAgent: false, } - if pw, ok := u.User.Password(); ok { + if u.User != nil { + if pw, ok := u.User.Password(); ok { + cfg.password = pw + } - cfg.password = pw }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/auth.go` around lines 40 - 42, ParseAuthFromURL may dereference u.User and panic; add a nil check before calling u.User.Password(). Wrap the existing password extraction in a guard like "if u.User != nil { if pw, ok := u.User.Password(); ok { cfg.password = pw } }" (and similarly guard any other uses of u.User) so ParseAuthFromURL handles URLs with no user info without panicking.
🧹 Nitpick comments (1)
internal/client/sftp/auth.go (1)
22-22: 💤 Low valueUnused error variable
ErrNoSignersFound.This error is defined but never used in the current implementation. Either remove it or use it in
buildAgentSignerswhen no signers are returned (currently returns an empty slice without error).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/auth.go` at line 22, The declared ErrNoSignersFound is unused; update buildAgentSigners to return ErrNoSignersFound when it ends up with zero signers (adjust its signature to return an error and propagate that error to callers), or if the design prefers no-change to control flow, remove the ErrNoSignersFound declaration entirely; reference buildAgentSigners and ErrNoSignersFound when making the change so callers and error handling are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/client/sftp/auth.go`:
- Around line 40-42: ParseAuthFromURL may dereference u.User and panic; add a
nil check before calling u.User.Password(). Wrap the existing password
extraction in a guard like "if u.User != nil { if pw, ok := u.User.Password();
ok { cfg.password = pw } }" (and similarly guard any other uses of u.User) so
ParseAuthFromURL handles URLs with no user info without panicking.
---
Nitpick comments:
In `@internal/client/sftp/auth.go`:
- Line 22: The declared ErrNoSignersFound is unused; update buildAgentSigners to
return ErrNoSignersFound when it ends up with zero signers (adjust its signature
to return an error and propagate that error to callers), or if the design
prefers no-change to control flow, remove the ErrNoSignersFound declaration
entirely; reference buildAgentSigners and ErrNoSignersFound when making the
change so callers and error handling are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eef276b-789f-43b3-8f91-132eebdf689d
📒 Files selected for processing (2)
README.mdinternal/client/sftp/auth.go
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/sftp/sftp.go`:
- Around line 99-104: The error returned when client.Stat(u.Path) fails should
be wrapped with types.ErrInvalidPath so callers can reliably detect invalid-path
errors; update the return in the client.Stat error branch (the block using
client.Stat, client.Close(), conn.Close()) to return an error that includes
types.ErrInvalidPath via %w and also preserves the original statErr details
(e.g. fmt.Errorf("%w: remote path %s does not exist: %v", types.ErrInvalidPath,
u.Path, statErr)), referencing the client.Stat/u.Path error branch in sftp.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3b5d4ca-eb18-43c5-8ccf-103a44b921d8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/ftp/ftp_test.gointernal/client/sftp/auth.gointernal/client/sftp/config.gointernal/client/sftp/sftp.gointernal/client/types/errors.gomain.go
✅ Files skipped from review due to trivial changes (4)
- internal/cli/commands/sync/sync.go
- main.go
- README.md
- .golangci.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/sftp/sftp.go (1)
98-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly map actual not-found failures to
ErrInvalidPath.Line 103 now treats every
client.Stat(u.Path)error as “does not exist”. That misclassifies permission or transport failures as bad input and gives callers a falseerrors.Is(err, types.ErrInvalidPath)signal. Wraptypes.ErrInvalidPathonly for genuine missing-path cases; otherwise return the original stat failure.Suggested patch
if u.Path != "" && u.Path != "/" { info, statErr := client.Stat(u.Path) if statErr != nil { _ = client.Close() _ = conn.Close() - return fmt.Errorf("%w: remote path %s does not exist: %w", types.ErrInvalidPath, u.Path, statErr) + if isIgnorableError(statErr) { + return fmt.Errorf("%w: remote path %s does not exist: %w", types.ErrInvalidPath, u.Path, statErr) + } + return fmt.Errorf("can't stat remote path %s: %w", u.Path, statErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/sftp/sftp.go` around lines 98 - 103, The code currently maps any client.Stat(u.Path) error to types.ErrInvalidPath; instead, detect genuine "not found" errors (e.g. via os.IsNotExist(statErr) or errors.Is(statErr, os.ErrNotExist)) and only then wrap and return types.ErrInvalidPath; for all other statErr values return the original statErr (after closing client and conn as already done). Update the branch around client.Stat(u.Path) to check the nature of statErr before wrapping with types.ErrInvalidPath so callers only get ErrInvalidPath for actual missing-path cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/client/sftp/sftp.go`:
- Around line 98-103: The code currently maps any client.Stat(u.Path) error to
types.ErrInvalidPath; instead, detect genuine "not found" errors (e.g. via
os.IsNotExist(statErr) or errors.Is(statErr, os.ErrNotExist)) and only then wrap
and return types.ErrInvalidPath; for all other statErr values return the
original statErr (after closing client and conn as already done). Update the
branch around client.Stat(u.Path) to check the nature of statErr before wrapping
with types.ErrInvalidPath so callers only get ErrInvalidPath for actual
missing-path cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fc0b1df-806e-4b13-b187-183d3faf3a99
📒 Files selected for processing (1)
internal/client/sftp/sftp.go
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
2 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 by CodeRabbit
New Features
Documentation
Tests
Chores