merge: 🛡️ devbox stale-ours no longer destroys an edit in the pull window

Author: shoemoney

internal/manifest/manifest.go

@@ -102,6 +102,35 @@ func Build(root string, ig *ignore.Matcher, guard *secret.Guard) (Manifest, []st
 	return Manifest{Entries: entries}, blocked, nil
 }
 
+// BuildFile re-stats and re-hashes a single file at root/relPath, the way Build
+// would if it walked to just that path. Used to re-check one path against a
+// previously snapshotted Entry without re-walking (and re-hashing) the whole
+// tree. ok is false if the path is no longer a regular file — moved, deleted,
+// or replaced by a directory — since callers that hold a snapshot from an
+// earlier Build have no lock on the tree in between; that's not an error, just
+// "nothing here to compare".
+func BuildFile(root, relPath string) (Entry, bool, error) {
+	p := filepath.Join(root, filepath.FromSlash(relPath))
+	info, err := os.Stat(p)
+	if err != nil || !info.Mode().IsRegular() {
+		return Entry{}, false, nil
+	}
+	f, err := os.Open(p)
+	if err != nil {
+		return Entry{}, false, nil
+	}
+	cs, err := chunk.SplitReader(f)
+	f.Close()
+	if err != nil {
+		return Entry{}, false, err
+	}
+	var hashes []string
+	for _, c := range cs {
+		hashes = append(hashes, c.Hash)
+	}
+	return Entry{Path: relPath, Mode: uint32(info.Mode().Perm()), Size: info.Size(), Chunks: hashes}, true, nil
+}
+
 // Marshal returns the manifest's canonical JSON bytes. Build sorts Entries, so
 // this is deterministic and serves as the content-addressed manifest blob.
 func (m Manifest) Marshal() ([]byte, error) {

internal/syncer/pull.go

@@ -81,6 +81,23 @@ func Pull(c *transport.Client, root, share, subpath, base, host string, now int6
 			continue // hub didn't touch p; local state (changed or not) stands
 		}
 
+		// `ours` was snapshotted once by manifest.Build before this loop started
+		// (pull.go:60); a local edit landing after that snapshot but before we
+		// get here would otherwise look unchanged and get silently clobbered
+		// below. Re-check the on-disk file against its snapshotted entry right
+		// before we might overwrite it — only when hub is about to write (tOK)
+		// and ours already had an entry to compare against (oOK); a pure hub-only
+		// add (!oOK) has nothing on disk yet to race with. Folding a detected
+		// edit into oursCh routes it through the existing conflict-preserving
+		// branch below instead of adding a second write path. This narrows the
+		// clobber window; it doesn't close it (a later edit can still land
+		// between this check and the writeEntry rename at pull.go:424-451).
+		if !oursCh && tOK && oOK {
+			if cur, ok, serr := manifest.BuildFile(root, p); serr == nil && ok && !manifest.SameContent(cur, oe) {
+				oursCh = true
+			}
+		}
+
 		switch {
 		case !oursCh:
 			// Hub-only change: apply it verbatim.

internal/syncer/zz_repro_stale_ours_test.go

@@ -0,0 +1,95 @@
+package syncer
+
+import (
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/shoemoney/devbox/internal/hooks"
+	"github.com/shoemoney/devbox/internal/hub"
+	"github.com/shoemoney/devbox/internal/hub/blobstore"
+	"github.com/shoemoney/devbox/internal/hub/meta"
+	"github.com/shoemoney/devbox/internal/secret"
+)
+
+// Repro for audit finding: `ours` is snapshotted once (manifest.Build, pull.go:60)
+// before any writes; a local edit landing after Build but before writeEntry still
+// looks "unchanged vs base", so the hub version is applied verbatim with no
+// conflict copy. We make the timing deterministic with a pre-pull hook, which
+// runs exactly in that window (after Build, before the apply loop).
+func TestAuditReproStaleOursOverwritesFreshEdit(t *testing.T) {
+	db, _ := meta.Open(":memory:")
+	defer db.Close()
+	store, _ := blobstore.NewDisk(t.TempDir())
+	srv := httptest.NewServer(hub.NewServer(db, store).Handler())
+	defer srv.Close()
+	guard, _ := secret.New(nil)
+	ig, _ := LoadIgnore(t.TempDir())
+
+	A := joinDevice(t, db, srv.URL, "alice")
+	B := joinDevice(t, db, srv.URL, "bob")
+	if err := A.Publish("s"); err != nil {
+		t.Fatal(err)
+	}
+
+	// A publishes f.txt = v1 (this becomes A's base).
+	rootA := t.TempDir()
+	writeFile(t, rootA, "f.txt", "v1\n")
+	snapA, _, err := Sync(A, rootA, "s", "", "", "alice", 1, ig, guard, nil)
+	if err != nil {
+		t.Fatalf("A sync: %v", err)
+	}
+
+	// B pulls, edits f.txt = v2-hub, pushes: hub-only change relative to A's base.
+	rootB := t.TempDir()
+	baseB, _, err := Sync(B, rootB, "s", "", "", "bob", 2, ig, guard, nil)
+	if err != nil {
+		t.Fatalf("B sync: %v", err)
+	}
+	writeFile(t, rootB, "f.txt", "v2-hub\n")
+	if _, _, err := Sync(B, rootB, "s", "", baseB, "bob", 3, ig, guard, nil); err != nil {
+		t.Fatalf("B sync2: %v", err)
+	}
+
+	// A's pre-pull hook simulates the user saving f.txt AFTER manifest.Build has
+	// snapshotted `ours` but BEFORE writeEntry applies the hub version.
+	hookDir := hooks.Dir(rootA)
+	if err := os.MkdirAll(hookDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	hookScript := "#!/usr/bin/env bash\nprintf 'USER-EDIT-IN-WINDOW\\n' > \"$DEVBOX_MOUNT/f.txt\"\n"
+	if err := os.WriteFile(filepath.Join(hookDir, "pre-pull"), []byte(hookScript), 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	hk := hooks.New(rootA, "s", "alice", srv.URL)
+	pr, err := Pull(A, rootA, "s", "", snapA, "alice", 4, ig, guard, hk)
+	if err != nil {
+		t.Fatalf("A pull: %v", err)
+	}
+	t.Logf("pull: written=%v deleted=%v conflicts=%v skipped=%v", pr.Written, pr.Deleted, pr.Conflicts, pr.Skipped)
+
+	got := readFile(t, rootA, "f.txt")
+	entries, _ := os.ReadDir(rootA)
+	var conflictFiles []string
+	for _, e := range entries {
+		if strings.Contains(e.Name(), ".conflict-") {
+			conflictFiles = append(conflictFiles, e.Name())
+		}
+	}
+	t.Logf("f.txt=%q conflictFiles=%v", got, conflictFiles)
+
+	// The user's bytes must survive somewhere: either f.txt still holds them,
+	// or a conflict copy does. If neither, the edit was destroyed.
+	if got == "USER-EDIT-IN-WINDOW\n" {
+		return // edit survived in place
+	}
+	for _, cf := range conflictFiles {
+		if readFile(t, rootA, cf) == "USER-EDIT-IN-WINDOW\n" {
+			return // preserved as conflict copy
+		}
+	}
+	t.Fatalf("DATA LOSS CONFIRMED: local edit made during pull window destroyed — f.txt=%q, conflicts=%v (bytes exist in no file and no snapshot)", got, conflictFiles)
+}