fix(sync): content-hash change detection via a name-encoded marker (#102)

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.<sha256>.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

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:
+//
+//	<vaultFileName>.<64-hex-sha256><markerSuffix>   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)

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")
+	}
+}