[sftp] add support#8
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🤖 Pull request artifacts
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR implements SFTP protocol support alongside the existing FTP functionality. The changes include refactoring the FTP client to use a shared package structure, adding a complete new SFTP client implementation with SSH connection management and remote filesystem operations, routing FTP/SFTP schemes through a factory, and updating documentation and CLI help to reflect both protocols. Dependencies are bumped to support SSH/SFTP connectivity. ChangesSFTP protocol support
Sequence DiagramsequenceDiagram
participant Caller
participant ClientFactory as client.New()
participant FtpClient as ftp.NewFtpClient
participant SftpClient as sftp.NewSftpClient
Caller->>ClientFactory: New(address, logger)
alt URL scheme is "ftp"
ClientFactory->>FtpClient: NewFtpClient(url, logger)
FtpClient-->>ClientFactory: *ftp.Client
else URL scheme is "sftp"
ClientFactory->>SftpClient: NewSftpClient(url, logger)
SftpClient-->>ClientFactory: *sftp.Client
else unsupported scheme
ClientFactory-->>Caller: error(ErrUnsupportedScheme with "ftp, sftp")
end
ClientFactory-->>Caller: Client interface, error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR introduces a substantial new SFTP client implementation (312 lines) alongside refactored FTP logic and factory routing, requiring careful review of connection initialization, mutex-protected state, error handling strategies, and filesystem operation semantics across both protocols. The changes are moderately heterogeneous (new implementation, refactoring, documentation, configuration) but follow a clear pattern; most complexity is localized to the SFTP client's initialization and recursive operations. 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 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
2 participants
Summary by CodeRabbit
New Features
Documentation