Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the sync command (issue #90), which performs bidirectional SSH key synchronization: it compares local and server-side keys using timestamps, decides whether each key should be uploaded, downloaded, or skipped, applies those decisions, and writes updated config/known_hosts from the server. The PR also refactors the inline upload HTTP call in upload.go into a reusable UploadData method on RetrievalClient, adds --path support to the download command, and improves GetAndCreateSshDirectory to accept absolute paths.
Changes:
pkg/actions/sync.go+sync_test.go: Newsynccommand withbuildSyncDecisions/applyDownloadshelpers and comprehensive unit tests.pkg/retrieval/data.go+data_test.go: NewUploadDatamethod onRetrievalClient, extracted from inline code inupload.go, with tests.pkg/actions/upload.go,pkg/actions/download.go,pkg/utils/write.go,main.go: Plumbing changes — upload reusesUploadData, download gains--pathflag,GetAndCreateSshDirectoryhandles absolute paths, and thesynccommand is registered in the CLI.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/actions/sync.go |
Core sync logic: decision-making, download application, upload dispatch, config/known_hosts write |
pkg/actions/sync_test.go |
Unit tests for buildSyncDecisions and applyDownloads helper functions |
pkg/retrieval/data.go |
UploadData method extracted from upload.go for reuse by both upload and sync |
pkg/retrieval/data_test.go |
Tests for the new UploadData method |
pkg/actions/upload.go |
Refactored to use retrieval.UploadData instead of inline HTTP call |
pkg/actions/download.go |
Added --path flag support to override the download destination directory |
pkg/utils/write.go |
GetAndCreateSshDirectory now treats absolute paths directly, enabling --path /abs/dir |
main.go |
Registers the new sync command and adds --path flag definition to download |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isSafeMode { | ||
| fmt.Println("Executing in safe mode (downloads writing to .ssh-sync-data)") | ||
| downloadDir = ".ssh-sync-data" | ||
| } else { | ||
| downloadDir = ".ssh" |
There was a problem hiding this comment.
The --path flag is only used to determine where local keys are read from for upload decisions (p), but the download destination (downloadDir) always defaults to .ssh or .ssh-sync-data regardless of --path. When a user runs ssh-sync sync --path /custom/dir, uploads read from /custom/dir but downloads are written to ~/.ssh (or ~/.ssh-sync-data). This is inconsistent behavior: the flag silently controls only half of the operation.
The download command (which was updated in this same PR) correctly treats --path as the full destination for all writes. The sync command should behave consistently: when --path is provided, it should be used as both the local source for uploads and the destination for downloads (overriding safe-mode as download does).
| if isSafeMode { | |
| fmt.Println("Executing in safe mode (downloads writing to .ssh-sync-data)") | |
| downloadDir = ".ssh-sync-data" | |
| } else { | |
| downloadDir = ".ssh" | |
| if c.IsSet("path") && p != "" { | |
| // When --path is provided, use it as both the upload source and download destination, | |
| // ignoring safe-mode (to match the behavior of the download command). | |
| downloadDir = p | |
| } else { | |
| if isSafeMode { | |
| fmt.Println("Executing in safe mode (downloads writing to .ssh-sync-data)") | |
| downloadDir = ".ssh-sync-data" | |
| } else { | |
| downloadDir = ".ssh" | |
| } |
| } | ||
| multipartWriter.Close() | ||
|
|
||
| client := retrieval.NewRetrievalClient() |
There was a problem hiding this comment.
A second RetrievalClient is constructed inside the if len(toUpload) > 0 block (line 127), but the outer client declared at line 74 is still in scope and is identical in behavior (retrieval.NewRetrievalClient() with the same default token and master-key functions). The inner client shadows the outer one unnecessarily. The outer client should be reused for the upload call to avoid allocating a redundant struct.
| client := retrieval.NewRetrievalClient() |
| // ssh_config is required by the server. Echo back the server's current | ||
| // config so the field is always present without overwriting remote changes. | ||
| sshConfigJSON, err := json.Marshal(serverData.SshConfig) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cfgField, err := multipartWriter.CreateFormField("ssh_config") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := cfgField.Write(sshConfigJSON); err != nil { | ||
| return err | ||
| } | ||
| multipartWriter.Close() |
There was a problem hiding this comment.
The sync command's upload path only echoes back the server's existing ssh_config and never includes the local known_hosts file. As a result, ssh-sync sync will never push local changes to ~/.ssh/config or ~/.ssh/known_hosts to the server, even when those files have changed locally. This is a significant functional gap compared to the upload command, which parses and uploads both. A user who switches from running upload + download to sync would silently lose the ability to propagate local SSH config and known_hosts updates to the server.
| // Apply downloads | ||
| downloadDirPath, err := utils.GetAndCreateSshDirectory(downloadDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := applyDownloads(decisions, serverMap, downloadDirPath); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The issue specification (#90) explicitly states that uploads should happen before downloads: "Running upload before download ensures the server always reflects the latest local state before the local filesystem is reconciled against it, minimising data loss." However, the implementation applies downloads first (line 90–97) and uploads second (line 99–131). If the upload step fails after a successful download, local keys that should have been sent to the server remain un-uploaded while the local state has already been modified by the download step, which is contrary to the design intent.
| // Summary | ||
| var nUploaded, nDownloaded, nSkipped int | ||
| for _, d := range decisions { | ||
| switch d.action { | ||
| case "upload": | ||
| nUploaded++ | ||
| case "download": | ||
| nDownloaded++ | ||
| case "skip": | ||
| nSkipped++ | ||
| } | ||
| } | ||
| fmt.Printf("Sync complete: %d uploaded, %d downloaded, %d skipped.\n", | ||
| nUploaded, nDownloaded, nSkipped) | ||
| return nil |
There was a problem hiding this comment.
The sync command does not call checkForDeletedKeys (or equivalent logic), so if a key is deleted on another machine and is absent from the server, running sync will not prompt the user to delete it locally. The issue specification (#90) explicitly lists "Prompt before deleting local keys absent from server" as a safety feature that should carry over to sync. Without this, sync silently diverges from download behavior and leaves stale local keys that the user would need to clean up manually or by running download separately.
Implements #90