From 0dd87c983feb2009e707a613df38399be74e568b Mon Sep 17 00:00:00 2001 From: Ari Mayer Date: Fri, 26 Jun 2026 18:19:51 -0400 Subject: [PATCH 1/3] fix(sync): detect remote changes by content hash via a name-encoded marker (#102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SmartPull decided "did the remote change?" purely from (ModTime, Size). Because the vault is AES-GCM (ciphertext length = plaintext length + fixed overhead), a same-length remote edit (rotate a password to an equal-length one, flip a fixed-width field) keeps Size byte-identical, leaving ModTime as the only signal — and some rclone backends preserve ModTime poorly. A false "unchanged" on a write path skips conflict detection and lets the blind SmartPush overwrite a real remote change: silent cross-device data loss. Fix: make pass-cli's own sha256 the authoritative remote-change signal, read for zero extra round-trips. On push, write a zero-byte marker named `..synchash` into the vault dir; rclone sync carries it to the remote. SmartPull reads the remote content hash straight from the marker name in the single `rclone lsjson` listing it already fetches: - marker present → remote changed iff markerHash != LastPushHash (content- authoritative; ignores modtime noise, so no false conflicts). - marker absent → fall back to the legacy (ModTime, Size) heuristic (old vaults / devices that pushed before markers existed). SmartPush writes exactly one marker and removes any stale one, so the dir (and, after sync, the remote) holds a single current marker. Known limitation (documented in code): an interrupted remote push that uploaded vault.enc but not the marker leaves a stale marker; the next successful push self-heals it. Accepted in exchange for zero-round-trip, false-conflict-free detection. Tests: marker parse (present / legacy / backup-lookalike / ambiguous); the #102 regression (identical Size+ModTime but differing marker → detected as changed); marker authoritative skip on modtime noise; marker conflict; push writes/cleans the marker. Closes #102. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019sxsM218vNzDbuMZ2nhMzx --- internal/sync/sync.go | 117 ++++++++++++++++++++++- internal/sync/sync_test.go | 186 +++++++++++++++++++++++++++++++++++++ 2 files changed, 299 insertions(+), 4 deletions(-) diff --git a/internal/sync/sync.go b/internal/sync/sync.go index 85acd8d..0d28b2f 100644 --- a/internal/sync/sync.go +++ b/internal/sync/sync.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "time" "github.com/arimxyer/pass-cli/internal/config" @@ -47,6 +48,87 @@ type RemoteFileInfo struct { IsDir bool `json:"IsDir"` } +// markerSuffix is appended to a content-hash marker object. The marker is a +// zero-byte file whose NAME encodes pass-cli's own sha256 of the vault: +// +// .<64-hex-sha256> e.g. vault.enc.9f3a…e21.synchash +// +// It is synced alongside vault.enc (it lives in the vault dir, only .sync-state +// is excluded), so the single `rclone lsjson` call SmartPull already makes also +// lists the marker — letting us read the remote's content identity for ZERO +// extra round-trips. This fixes the (ModTime, Size) false-negative in #102: +// a same-length, same-modtime remote edit changes the marker name, so it can no +// longer read as "unchanged". +const markerSuffix = ".synchash" + +// markerFileName builds the marker object name for a vault file + content hash. +func markerFileName(vaultFileName, hash string) string { + return vaultFileName + "." + hash + markerSuffix +} + +// isHex64 reports whether s is exactly 64 lowercase-or-uppercase hex chars +// (a sha256 hex digest), guarding the marker parse against unrelated files. +func isHex64(s string) bool { + if len(s) != 64 { + return false + } + for _, c := range s { + switch { + case c >= '0' && c <= '9', c >= 'a' && c <= 'f', c >= 'A' && c <= 'F': + default: + return false + } + } + return true +} + +// parseRemoteMarkerHash scans an rclone lsjson listing for the vault's content +// marker and returns the embedded sha256. ok is false when no marker is present +// (legacy remotes, or a device that pushed before markers existed) or when the +// listing is ambiguous (more than one distinct marker hash — an abnormal, +// interrupted state) — callers then fall back to the (ModTime, Size) heuristic. +func parseRemoteMarkerHash(files []RemoteFileInfo, vaultFileName string) (string, bool) { + prefix := vaultFileName + "." + found := "" + for i := range files { + name := files[i].Name + if !strings.HasPrefix(name, prefix) || !strings.HasSuffix(name, markerSuffix) { + continue + } + hash := name[len(prefix) : len(name)-len(markerSuffix)] + if !isHex64(hash) { + continue + } + if found != "" && found != hash { + return "", false // ambiguous — fall back to the heuristic + } + found = hash + } + if found == "" { + return "", false + } + return found, true +} + +// writeLocalMarker drops a zero-byte marker named for hash into vaultDir and +// removes any stale markers for the same vault, so the directory holds exactly +// one marker. The subsequent `rclone sync` of the dir mirrors that single marker +// to the remote (deleting the old one there too). +func writeLocalMarker(vaultDir, vaultFileName, hash string) error { + prefix := vaultFileName + "." + entries, err := os.ReadDir(vaultDir) + if err == nil { + for _, e := range entries { + name := e.Name() + if strings.HasPrefix(name, prefix) && strings.HasSuffix(name, markerSuffix) { + _ = os.Remove(filepath.Join(vaultDir, name)) + } + } + } + markerPath := filepath.Join(vaultDir, markerFileName(vaultFileName, hash)) + return os.WriteFile(markerPath, nil, 0600) +} + // Service provides vault synchronization using rclone. type Service struct { config config.SyncConfig @@ -193,16 +275,35 @@ func (s *Service) SmartPull(vaultPath string) error { state = &SyncState{} } - // 3. Check if remote is unchanged - if remoteVault.ModTime.Equal(state.RemoteModTime) && remoteVault.Size == state.RemoteSize { + // 3. Decide whether the remote changed since our last push. + // + // Prefer the content marker (#102): its name encodes the remote's own + // sha256, so a same-size + same-modtime remote edit (which the legacy + // heuristic would miss) still reads as changed. Fall back to the + // (ModTime, Size) heuristic only when no marker is present (legacy remotes + // or a device that pushed before markers existed). + // + // Limitation: an interrupted remote push that uploaded vault.enc but not the + // marker can leave a stale marker == LastPushHash, read here as "unchanged." + // The next successful push self-heals the marker; we accept this rare window + // in exchange for content-authoritative detection with no extra round-trip + // and no false conflicts from modtime noise. + remoteHash, hasMarker := parseRemoteMarkerHash(remoteFiles, vaultFileName) + var remoteChanged bool + if hasMarker { + remoteChanged = remoteHash != state.LastPushHash + } else { + remoteChanged = !(remoteVault.ModTime.Equal(state.RemoteModTime) && remoteVault.Size == state.RemoteSize) + } + if !remoteChanged { return nil // Remote unchanged, skip pull } - // 4. Check for local changes (conflict detection) + // 4. Remote changed — if local also diverged from our last push, it's a + // conflict (both sides changed). This is content-based on both sides. if _, statErr := os.Stat(vaultPath); statErr == nil { localHash, hashErr := HashFile(vaultPath) if hashErr == nil && state.LastPushHash != "" && localHash != state.LastPushHash { - // Local has unpushed changes AND remote has changed = conflict return ErrSyncConflict } } @@ -266,6 +367,14 @@ func (s *Service) SmartPush(vaultPath string) (bool, error) { return false, nil } + // 3b. Write the content marker (#102) into the vault dir so the rclone sync + // below carries it to the remote, where the next device's SmartPull reads our + // content hash straight from its name. Best-effort: a marker write failure + // degrades change-detection to the legacy heuristic but must not block a push. + if err := writeLocalMarker(vaultDir, filepath.Base(vaultPath), localHash); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to write sync content marker: %v\n", err) + } + // 4. Push if err := s.executor.RunNoOutput("rclone", "sync", vaultDir, s.config.Remote, "--exclude", syncStateFile); err != nil { fmt.Fprintf(os.Stderr, "Warning: sync push failed: %v\n", err) diff --git a/internal/sync/sync_test.go b/internal/sync/sync_test.go index 1395fca..cb2e2ad 100644 --- a/internal/sync/sync_test.go +++ b/internal/sync/sync_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "time" @@ -373,3 +374,188 @@ func TestSmartPull_LsjsonFailure(t *testing.T) { t.Errorf("SmartPull should allow offline operation, got error: %v", err) } } + +// --- #102: content-marker tests --- + +func hashA() string { return strings.Repeat("a", 64) } +func hashB() string { return strings.Repeat("b", 64) } + +func TestParseRemoteMarkerHash(t *testing.T) { + tests := []struct { + name string + files []RemoteFileInfo + wantHash string + wantOK bool + }{ + { + name: "marker present", + files: []RemoteFileInfo{ + {Name: "vault.enc"}, + {Name: markerFileName("vault.enc", hashA())}, + }, + wantHash: hashA(), wantOK: true, + }, + { + name: "no marker (legacy remote)", + files: []RemoteFileInfo{{Name: "vault.enc"}}, + wantOK: false, + }, + { + name: "ignores backup and non-hex lookalikes", + files: []RemoteFileInfo{ + {Name: "vault.enc"}, + {Name: "vault.enc.backup"}, + {Name: "vault.enc.not-a-hash.synchash"}, // wrong length / non-hex + }, + wantOK: false, + }, + { + name: "ambiguous (two distinct markers) falls back", + files: []RemoteFileInfo{ + {Name: markerFileName("vault.enc", hashA())}, + {Name: markerFileName("vault.enc", hashB())}, + }, + wantOK: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotHash, gotOK := parseRemoteMarkerHash(tt.files, "vault.enc") + if gotOK != tt.wantOK || (tt.wantOK && gotHash != tt.wantHash) { + t.Errorf("parseRemoteMarkerHash() = (%q, %v), want (%q, %v)", gotHash, gotOK, tt.wantHash, tt.wantOK) + } + }) + } +} + +// THE #102 regression: a remote edit that keeps vault.enc's Size AND ModTime +// identical (the legacy heuristic's blind spot) must still be detected as +// changed, because the marker name carries a different content hash. Without the +// marker this skips the pull (silent stale read; on a write path, silent clobber). +func TestSmartPull_MarkerCatchesSameSizeSameModtimeEdit(t *testing.T) { + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + _ = os.WriteFile(vaultPath, []byte("vault-data"), 0600) + + localHash, _ := HashFile(vaultPath) + sameTime := time.Date(2026, 1, 15, 12, 0, 0, 0, time.UTC) + + // Local matches last push (no local changes). Recorded remote modtime/size + // match what lsjson will report — so the (ModTime, Size) heuristic says + // "unchanged" and would skip. + _ = SaveState(tmpDir, &SyncState{ + LastPushHash: localHash, + RemoteModTime: sameTime, + RemoteSize: 100, + }) + + // Remote vault.enc is byte-identical in Size+ModTime, but the marker encodes + // a DIFFERENT content hash → remote really changed. + lsjsonOutput, _ := json.Marshal([]RemoteFileInfo{ + {Name: "vault.enc", Size: 100, ModTime: sameTime}, + {Name: markerFileName("vault.enc", hashB())}, + }) + mock := &mockExecutor{runOutput: lsjsonOutput} + service := NewServiceWithExecutor(enabledConfig(), mock) + + if err := service.SmartPull(vaultPath); err != nil { + t.Fatalf("SmartPull returned error: %v", err) + } + // Must have pulled (sync) despite identical modtime+size. + if len(mock.runNoOutCalls) != 1 { + t.Errorf("expected 1 sync call (pull) — marker should force detection, got %d", len(mock.runNoOutCalls)) + } +} + +// The marker is authoritative: when it matches LastPushHash the content is +// unchanged, so SmartPull must skip even if ModTime/Size differ (modtime noise +// must not trigger a needless pull or a false conflict). +func TestSmartPull_MarkerSkipsOnModtimeNoise(t *testing.T) { + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + _ = os.WriteFile(vaultPath, []byte("vault-data"), 0600) + + localHash, _ := HashFile(vaultPath) + _ = SaveState(tmpDir, &SyncState{ + LastPushHash: localHash, + RemoteModTime: time.Date(2026, 1, 15, 12, 0, 0, 0, time.UTC), + RemoteSize: 100, + }) + + // vault.enc reports a different modtime+size, but the marker == LastPushHash. + lsjsonOutput, _ := json.Marshal([]RemoteFileInfo{ + {Name: "vault.enc", Size: 999, ModTime: time.Date(2026, 2, 2, 2, 0, 0, 0, time.UTC)}, + {Name: markerFileName("vault.enc", localHash)}, + }) + mock := &mockExecutor{runOutput: lsjsonOutput} + service := NewServiceWithExecutor(enabledConfig(), mock) + + if err := service.SmartPull(vaultPath); err != nil { + t.Fatalf("SmartPull returned error: %v", err) + } + if len(mock.runNoOutCalls) != 0 { + t.Errorf("expected no sync (marker says unchanged), got %d", len(mock.runNoOutCalls)) + } +} + +// Marker says remote changed AND local diverged from last push → conflict. +func TestSmartPull_MarkerConflict(t *testing.T) { + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + _ = os.WriteFile(vaultPath, []byte("local-modified"), 0600) + + // Local differs from last push (local changed). + _ = SaveState(tmpDir, &SyncState{LastPushHash: hashA()}) + + // Remote marker differs from last push (remote changed). + lsjsonOutput, _ := json.Marshal([]RemoteFileInfo{ + {Name: "vault.enc", Size: 200}, + {Name: markerFileName("vault.enc", hashB())}, + }) + mock := &mockExecutor{runOutput: lsjsonOutput} + service := NewServiceWithExecutor(enabledConfig(), mock) + + if err := service.SmartPull(vaultPath); !errors.Is(err, ErrSyncConflict) { + t.Errorf("expected ErrSyncConflict, got: %v", err) + } + if len(mock.runNoOutCalls) != 0 { + t.Errorf("expected no sync on conflict, got %d", len(mock.runNoOutCalls)) + } +} + +// SmartPush writes exactly one content marker named for the new hash and removes +// any stale marker from a previous push. +func TestSmartPush_WritesContentMarker(t *testing.T) { + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + _ = os.WriteFile(vaultPath, []byte("vault-data"), 0600) + + // A stale marker from a previous push that must be cleaned up. + staleMarker := filepath.Join(tmpDir, markerFileName("vault.enc", hashA())) + _ = os.WriteFile(staleMarker, nil, 0600) + + _ = SaveState(tmpDir, &SyncState{LastPushHash: "old-hash"}) + mock := &mockExecutor{runOutput: []byte(`[{"Name":"vault.enc","Size":10}]`)} + service := NewServiceWithExecutor(enabledConfig(), mock) + + if _, err := service.SmartPush(vaultPath); err != nil { + t.Fatalf("SmartPush returned error: %v", err) + } + + expectedHash, _ := HashFile(vaultPath) + wantMarker := markerFileName("vault.enc", expectedHash) + + entries, _ := os.ReadDir(tmpDir) + var markers []string + for _, e := range entries { + if strings.HasSuffix(e.Name(), markerSuffix) { + markers = append(markers, e.Name()) + } + } + if len(markers) != 1 || markers[0] != wantMarker { + t.Errorf("expected exactly one marker %q, got %v", wantMarker, markers) + } + if _, err := os.Stat(staleMarker); !os.IsNotExist(err) { + t.Errorf("stale marker should have been removed") + } +} From f00d7eaa43ab729b27e13bd1b09847d4dfa43b7a Mon Sep 17 00:00:00 2001 From: Ari Mayer Date: Fri, 26 Jun 2026 18:20:30 -0400 Subject: [PATCH 2/3] docs(sync): explain the .synchash content marker in the sync guide (#102) Note that a zero-byte vault.enc..synchash object now appears in the remote bucket, how it powers zero-round-trip content-based change detection, and that it is safe to leave alone. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019sxsM218vNzDbuMZ2nhMzx --- docs/02-guides/sync-guide.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/02-guides/sync-guide.md b/docs/02-guides/sync-guide.md index 824b5ba..ff940a8 100644 --- a/docs/02-guides/sync-guide.md +++ b/docs/02-guides/sync-guide.md @@ -411,6 +411,17 @@ Pass-CLI uses rclone's sync behavior which **overwrites** the destination with t - **Pull**: Cloud overwrites local (ensures you have latest) - **Push**: Local overwrites cloud (your changes take precedence) +### Change detection + +To decide whether the remote actually changed (and avoid needless pulls), pass-cli +compares its own SHA-256 of the vault content rather than trusting the file's +modification time and size. On each push it writes a tiny zero-byte marker named +`vault.enc..synchash` next to `vault.enc` in your remote; the content hash +is read straight from that filename during the pre-unlock listing, so detection +costs no extra network round-trip. **You may see this `*.synchash` object in your +cloud bucket — it is expected and safe to leave alone** (pass-cli replaces it on +every push). Older vaults without a marker fall back to the modtime+size heuristic. + **To avoid conflicts**: 1. Always use the same device for a session 2. Don't run pass-cli simultaneously on multiple devices From 8ac78d3e4c022c3defb91de6ab8561292fca8cfb Mon Sep 17 00:00:00 2001 From: Ari Mayer Date: Fri, 26 Jun 2026 18:40:30 -0400 Subject: [PATCH 3/3] style(sync): apply De Morgan's law to satisfy staticcheck QF1001 (#102) CI golangci-lint v2.5 flagged the negated conjunction in the marker-absent fallback. Rewrite !(A && B) as !A || !B; behavior identical. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019sxsM218vNzDbuMZ2nhMzx --- internal/sync/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sync/sync.go b/internal/sync/sync.go index 0d28b2f..a24a09e 100644 --- a/internal/sync/sync.go +++ b/internal/sync/sync.go @@ -293,7 +293,7 @@ func (s *Service) SmartPull(vaultPath string) error { if hasMarker { remoteChanged = remoteHash != state.LastPushHash } else { - remoteChanged = !(remoteVault.ModTime.Equal(state.RemoteModTime) && remoteVault.Size == state.RemoteSize) + remoteChanged = !remoteVault.ModTime.Equal(state.RemoteModTime) || remoteVault.Size != state.RemoteSize } if !remoteChanged { return nil // Remote unchanged, skip pull