diff --git a/CLAUDE.md b/CLAUDE.md index 160e031..30975f0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,7 @@ mise run gh # GitHub CLI operations - `SmartPush` hashes local file, skips network if hash matches `LastPushHash` in `.sync-state` - `SmartPull` runs before unlock, `SmartPush` runs after command completes (synchronous, blocks prompt) - `RecordFieldAccess` (called by `get`) writes usage timestamps, changing vault hash and triggering push -- Two network round-trips per push: `rclone sync` + `rclone lsjson --hash` +- Two network round-trips per push: `rclone sync` + `rclone lsjson` - Async push risks: silent failures, false conflicts (kill between push and SaveState), partial uploads on some backends ## Environment Setup diff --git a/cmd/helpers.go b/cmd/helpers.go index b8ce7a5..4e88340 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -399,11 +399,40 @@ func unlockVault(vaultService *vault.VaultService) error { // syncPullBeforeUnlock performs a smart sync pull before vault unlock. // This ensures we have the latest version from remote before reading. +// +// When --offline is set, the pull is skipped entirely (no network either +// direction — see syncPushAfterCommand for the matching push skip). +// +// When sync is enabled and not offline, the network probe always runs +// (CheckRemoteMetadata is a round-trip; only the file transfer is conditional), +// so the user gets feedback that the command is hitting the network: +// - verbose: a descriptive message on stderr +// - otherwise: a transient indicator on stderr, cleared when the probe returns +// +// Nothing is ever written to stdout (it must stay byte-clean for pipes), and +// nothing is written at all when sync is disabled or --offline is set. func syncPullBeforeUnlock(vaultService *vault.VaultService) { + if IsOffline() { + return + } + if !vaultService.IsSyncEnabled() { + return + } + if IsVerbose() { fmt.Fprintln(os.Stderr, "🔄 Checking remote for vault changes...") + if err := vaultService.SyncPull(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", err) + } + return } - if err := vaultService.SyncPull(); err != nil { + + // Transient progress indicator on stderr, cleared with carriage-return + + // ANSI erase-to-end-of-line (same technique as syncPushAfterCommand). + fmt.Fprint(os.Stderr, "Checking remote...") + err := vaultService.SyncPull() + fmt.Fprint(os.Stderr, "\r\033[K") + if err != nil { fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", err) } } @@ -411,7 +440,16 @@ func syncPullBeforeUnlock(vaultService *vault.VaultService) { // syncPushAfterCommand performs a smart sync push after a command completes. // This ensures local changes are pushed to remote once per command, not per-save. // Shows "Syncing... done" feedback on stderr when a push actually occurs. +// +// When --offline is set, the push is skipped entirely. This is a correctness +// requirement, not just an optimization: SmartPush has no independent +// remote-conflict check — its only safety is that SmartPull ran first. If +// --offline skipped the pull but still pushed, a write could blind-overwrite a +// newer remote (silent cross-device data loss). So --offline means fully local. func syncPushAfterCommand(vaultService *vault.VaultService) { + if IsOffline() { + return + } if !vaultService.IsSyncEnabled() { return } diff --git a/cmd/root.go b/cmd/root.go index 065501e..ef1190f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -16,6 +16,7 @@ import ( var ( cfgFile string verbose bool + offline bool // Version information (set via ldflags during build) version = "dev" @@ -91,9 +92,11 @@ Original error: %w`, os.Getenv("HOME"), err) // Global flags rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.pass-cli/config.yaml)") rootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "verbose output") + rootCmd.PersistentFlags().BoolVar(&offline, "offline", false, "skip cloud sync for this command (run fully local; offline changes won't sync until the next online run)") // Bind flags to viper _ = viper.BindPFlag("verbose", rootCmd.PersistentFlags().Lookup("verbose")) + _ = viper.BindPFlag("offline", rootCmd.PersistentFlags().Lookup("offline")) // Configure command groups for better help organization rootCmd.AddGroup( @@ -236,6 +239,13 @@ func IsVerbose() bool { return verbose || viper.GetBool("verbose") } +// IsOffline returns whether offline mode is enabled. When true, both the +// pre-unlock pull and the post-command push are skipped so the command runs +// fully local. Offline changes will not sync until the next online run. +func IsOffline() bool { + return offline || viper.GetBool("offline") +} + // runRootCommand runs when pass-cli is invoked with no subcommand // Launches TUI mode by default, with first-run detection func runRootCommand(cmd *cobra.Command, args []string) { diff --git a/cmd/sync_feedback_test.go b/cmd/sync_feedback_test.go new file mode 100644 index 0000000..097542a --- /dev/null +++ b/cmd/sync_feedback_test.go @@ -0,0 +1,216 @@ +package cmd + +import ( + "bytes" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/arimxyer/pass-cli/internal/vault" +) + +// captureOutErr captures both os.Stdout and os.Stderr produced by fn. +// Declared once here; cmd/exec_test.go already owns captureStdout — do not +// redeclare shared helpers. +func captureOutErr(t *testing.T, fn func()) (stdoutStr, stderrStr string) { + t.Helper() + + origOut, origErr := os.Stdout, os.Stderr + rOut, wOut, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create stdout pipe: %v", err) + } + rErr, wErr, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create stderr pipe: %v", err) + } + os.Stdout = wOut + os.Stderr = wErr + + outDone := make(chan string, 1) + errDone := make(chan string, 1) + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, rOut) + outDone <- buf.String() + }() + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, rErr) + errDone <- buf.String() + }() + + fn() + + _ = wOut.Close() + _ = wErr.Close() + os.Stdout = origOut + os.Stderr = origErr + stdoutStr = <-outDone + stderrStr = <-errDone + _ = rOut.Close() + _ = rErr.Close() + return stdoutStr, stderrStr +} + +// newVaultServiceForSyncTest builds a real VaultService whose sync is enabled or +// disabled per the flag, by pointing config loading at a temp config file via +// PASS_CLI_CONFIG. A bogus remote is used: when sync runs it fails fast (no real +// network), which is fine — these tests assert on the cmd-layer feedback/no-op +// behavior, not on rclone itself. +func newVaultServiceForSyncTest(t *testing.T, syncEnabled bool) *vault.VaultService { + t.Helper() + + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + cfgPath := filepath.Join(tmpDir, "config.yml") + + cfg := "vault_path: " + vaultPath + "\n" + if syncEnabled { + cfg += "sync:\n enabled: true\n remote: \"mock-remote:bucket\"\n" + } + if err := os.WriteFile(cfgPath, []byte(cfg), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + t.Setenv("PASS_CLI_CONFIG", cfgPath) + + vs, err := vault.New(vaultPath) + if err != nil { + t.Fatalf("failed to create VaultService: %v", err) + } + if vs.IsSyncEnabled() != syncEnabled { + t.Fatalf("IsSyncEnabled() = %v, want %v", vs.IsSyncEnabled(), syncEnabled) + } + return vs +} + +// setOffline sets the package-level offline flag and restores it after the test. +// IsOffline() reads the package var directly, so toggling it (not viper) is the +// reliable lever in a unit test where flags are never parsed. +func setOffline(t *testing.T, v bool) { + t.Helper() + orig := offline + offline = v + t.Cleanup(func() { offline = orig }) +} + +func setVerbose(t *testing.T, v bool) { + t.Helper() + orig := verbose + verbose = v + t.Cleanup(func() { verbose = orig }) +} + +func TestIsOffline(t *testing.T) { + setOffline(t, false) + if IsOffline() { + t.Error("IsOffline() = true with offline=false, want false") + } + offline = true + if !IsOffline() { + t.Error("IsOffline() = false with offline=true, want true") + } +} + +// --offline must skip the pre-unlock pull entirely (no output, no rclone), +// even when sync is enabled. A sync-enabled service is required to prove the +// no-op comes from the offline check and not from sync being disabled. +func TestSyncPullBeforeUnlock_OfflineNoOp(t *testing.T) { + vs := newVaultServiceForSyncTest(t, true) + setOffline(t, true) + setVerbose(t, false) + + stdout, stderr := captureOutErr(t, func() { + syncPullBeforeUnlock(vs) + }) + + if stdout != "" { + t.Errorf("expected empty stdout when offline, got %q", stdout) + } + if stderr != "" { + t.Errorf("expected empty stderr when offline, got %q", stderr) + } +} + +// --offline must skip the post-command push entirely (correctness: no blind +// overwrite of a newer remote). Sync enabled to prove the offline check fires. +func TestSyncPushAfterCommand_OfflineNoOp(t *testing.T) { + vs := newVaultServiceForSyncTest(t, true) + setOffline(t, true) + setVerbose(t, false) + + stdout, stderr := captureOutErr(t, func() { + syncPushAfterCommand(vs) + }) + + if stdout != "" { + t.Errorf("expected empty stdout when offline, got %q", stdout) + } + if stderr != "" { + t.Errorf("expected empty stderr when offline, got %q", stderr) + } +} + +// When sync is enabled and not offline, the pull shows a transient indicator on +// stderr and nothing on stdout. +func TestSyncPullBeforeUnlock_FeedbackOnStderr(t *testing.T) { + vs := newVaultServiceForSyncTest(t, true) + setOffline(t, false) + setVerbose(t, false) + + stdout, stderr := captureOutErr(t, func() { + syncPullBeforeUnlock(vs) + }) + + if stdout != "" { + t.Errorf("expected empty stdout, got %q", stdout) + } + if !strings.Contains(stderr, "Checking remote") { + t.Errorf("expected stderr to contain the progress indicator, got %q", stderr) + } + // Indicator must be transient: cleared with CR + ANSI erase-to-end-of-line. + if !strings.Contains(stderr, "\r\033[K") { + t.Errorf("expected stderr to contain the clear sequence, got %q", stderr) + } +} + +// When sync is enabled and not offline, the push shows the "Syncing..." indicator +// on stderr and nothing on stdout. Mirror of the pull feedback test — locks the +// normal-path push behavior so a regression in the offline-gate ordering is caught. +func TestSyncPushAfterCommand_FeedbackOnStderr(t *testing.T) { + vs := newVaultServiceForSyncTest(t, true) + setOffline(t, false) + setVerbose(t, false) + + stdout, stderr := captureOutErr(t, func() { + syncPushAfterCommand(vs) + }) + + if stdout != "" { + t.Errorf("expected empty stdout, got %q", stdout) + } + if !strings.Contains(stderr, "Syncing...") { + t.Errorf("expected stderr to contain the syncing indicator, got %q", stderr) + } +} + +// When sync is disabled, the pull shows nothing at all — even under verbose. +func TestSyncPullBeforeUnlock_DisabledNoOutput(t *testing.T) { + vs := newVaultServiceForSyncTest(t, false) + setOffline(t, false) + + for _, v := range []bool{false, true} { + setVerbose(t, v) + stdout, stderr := captureOutErr(t, func() { + syncPullBeforeUnlock(vs) + }) + if stdout != "" { + t.Errorf("verbose=%v: expected empty stdout, got %q", v, stdout) + } + if stderr != "" { + t.Errorf("verbose=%v: expected empty stderr when sync disabled, got %q", v, stderr) + } + } +} diff --git a/cmd/tui.go b/cmd/tui.go index f0e9e0b..cb93f51 100644 --- a/cmd/tui.go +++ b/cmd/tui.go @@ -63,10 +63,9 @@ func runTUI(cmd *cobra.Command, args []string) { os.Exit(1) } - // Smart sync pull before unlock - if syncErr := vaultService.SyncPull(); syncErr != nil { - fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", syncErr) - } + // Smart sync pull before unlock. Delegates to the shared helper so the TUI + // honors --offline (skip sync entirely) and shows the same feedback as the CLI. + syncPullBeforeUnlock(vaultService) // Try keychain unlock first err = vaultService.UnlockWithKeychain() @@ -170,14 +169,11 @@ func launchTUI(vaultService *vault.VaultService) error { // Run application (blocking) runErr := app.Run() - // Push any changes made during the TUI session (only if writes occurred) - if appState.HasWriteOperations() && vaultService.IsSyncEnabled() { - fmt.Fprint(os.Stderr, "Syncing...") - if vaultService.SyncPush() { - fmt.Fprintln(os.Stderr, " done") - } else { - fmt.Fprint(os.Stderr, "\r\033[K") - } + // Push any changes made during the TUI session (only if writes occurred). + // syncPushAfterCommand handles the --offline gate, the sync-enabled check, and + // the "Syncing..." feedback — keeping TUI and CLI push behavior identical. + if appState.HasWriteOperations() { + syncPushAfterCommand(vaultService) } return runErr diff --git a/docs/02-guides/sync-guide.md b/docs/02-guides/sync-guide.md index 935eb3b..824b5ba 100644 --- a/docs/02-guides/sync-guide.md +++ b/docs/02-guides/sync-guide.md @@ -431,6 +431,22 @@ pass-cli add service --username user --password pass Your local vault is updated successfully. Changes sync on next successful push. +### Skipping sync explicitly with `--offline` + +When you *know* you're offline — or you just want to avoid the network round-trip +(e.g. in CI or a tight scripted loop) — pass the global `--offline` flag: + +```bash +pass-cli --offline get github # no pre-unlock pull, no post-command push +pass-cli --offline list -q +``` + +`--offline` makes the command **fully local in both directions**: it skips the +pre-unlock pull *and* the post-command push. Skipping only the pull would be +unsafe — the push has no independent remote-conflict check, so it could blind- +overwrite a newer remote — which is why `--offline` disables both. Any local +changes you make offline simply aren't propagated until the next online run. + ## Troubleshooting ### Sync Not Working diff --git a/docs/03-reference/command-reference.md b/docs/03-reference/command-reference.md index eab6126..42bb3d6 100644 --- a/docs/03-reference/command-reference.md +++ b/docs/03-reference/command-reference.md @@ -14,6 +14,7 @@ Available for all commands: | Flag | Description | Example | |------|-------------|---------| | `--verbose` | Enable verbose output | `--verbose` | +| `--offline` | Skip cloud sync for this command (run fully local) | `--offline` | | `--help`, `-h` | Show help | `--help` | ### Global Flag Examples @@ -22,6 +23,10 @@ Available for all commands: # Enable verbose logging pass-cli --verbose get github +# Run fully local — skip the pre-unlock pull and post-command push +# (useful in CI or tight loops; offline changes sync on the next online run) +pass-cli --offline get github + # Get help for any command pass-cli get --help ``` diff --git a/internal/sync/sync.go b/internal/sync/sync.go index fc3f877..85acd8d 100644 --- a/internal/sync/sync.go +++ b/internal/sync/sync.go @@ -135,7 +135,10 @@ func (s *Service) Push(vaultDir string) error { // CheckRemoteMetadata fetches remote file metadata using rclone lsjson. func (s *Service) CheckRemoteMetadata() ([]RemoteFileInfo, error) { - output, err := s.executor.Run("rclone", "lsjson", s.config.Remote, "--hash") + // Note: no "--hash" flag — RemoteFileInfo carries no hash field and no + // decision uses a remote hash (skip compares ModTime+Size; conflict detection + // uses a local sha256 via HashFile). Requesting it only adds backend cost. + output, err := s.executor.Run("rclone", "lsjson", s.config.Remote) if err != nil { return nil, fmt.Errorf("rclone lsjson failed: %w", err) } diff --git a/internal/sync/sync_test.go b/internal/sync/sync_test.go index 05afdce..1395fca 100644 --- a/internal/sync/sync_test.go +++ b/internal/sync/sync_test.go @@ -197,6 +197,37 @@ func TestSmartPush_PushesWhenChanged(t *testing.T) { } } +// --- CheckRemoteMetadata tests --- + +func TestCheckRemoteMetadata_DoesNotRequestHash(t *testing.T) { + // lsjson must NOT pass "--hash": RemoteFileInfo has no hash field and no + // decision uses a remote hash, so requesting it only adds backend cost. + mock := &mockExecutor{runOutput: []byte("[]")} + service := NewServiceWithExecutor(enabledConfig(), mock) + + if _, err := service.CheckRemoteMetadata(); err != nil { + t.Fatalf("CheckRemoteMetadata returned error: %v", err) + } + + if len(mock.runCalls) != 1 { + t.Fatalf("expected 1 Run call (lsjson), got %d", len(mock.runCalls)) + } + + args := mock.runCalls[0] + foundLsjson := false + for _, arg := range args { + if arg == "--hash" { + t.Errorf("lsjson must not pass --hash, got args %v", args) + } + if arg == "lsjson" { + foundLsjson = true + } + } + if !foundLsjson { + t.Errorf("expected lsjson in args, got %v", args) + } +} + // --- SmartPull tests --- func TestSmartPull_Disabled(t *testing.T) {