fix(engine): scope staleness checks to the loaded graph (#158)

dejo1307 · web-flow · commit fe82cb3e5c1c · 2026-07-30T09:53:26.000+02:00
Staleness took both the age signal and the per-repo VCS check from the
machine-wide graph receipt, which every server process on the machine
rewrites and which therefore need not describe the graph this server
holds. It reported drift for repos absent from the graph, and missed
drift in the loaded ones.

Receipt entries are now filtered to repos present in the loaded graph,
and the age comes from the loaded snapshot's own generated_at, with the
receipt as a fallback only when it describes a loaded repo.

Fact output is unchanged; no cacheVersion bump.
diff --git a/internal/engine/freshness.go b/internal/engine/freshness.go
@@ -30,32 +30,43 @@ func (s Staleness) Stale() bool { return s.TooOld || len(s.Changed) > 0 }
 // CURRENT VCS state, without regenerating anything. A repo counts as changed when
 // its git HEAD has moved since the snapshot, or its working tree is NEWLY dirty
 // (a tree that was already dirty at snapshot time is ignored — that state was
-// captured by the extractors). Age is measured from the graph-wide generated_at in
-// ~/.enola/receipt.json, falling back to the in-memory snapshot meta for a
-// single-repo graph. Repos without git contribute only to the age signal.
+// captured by the extractors). Repos without git contribute only to the age signal.
+//
+// Everything it judges is scoped to the repos actually in the loaded graph. Age comes
+// from the loaded snapshot's own generated_at; the machine-wide
+// ~/.enola/receipt.json is consulted only as a fallback, and only when it describes a
+// loaded repo. That scoping is load-bearing, not defensive: the receipt is shared by
+// every enola process on the machine, so a sibling agent terminal's snapshot routinely
+// makes it describe a different graph entirely.
 func (e *Engine) Staleness(maxAge time.Duration, now time.Time) Staleness {
 	var st Staleness
 
 	gr, err := LoadGlobalReceipt()
-	if err == nil && gr.GeneratedAt != "" {
-		if t, perr := time.Parse(time.RFC3339, gr.GeneratedAt); perr == nil {
+	snap, loaded := e.loadedGraph()
+
+	// Age comes from the LOADED snapshot's own generated_at. That field is
+	// authoritative: it is stamped on every generate and restored verbatim from
+	// snapshot.meta.json on restart, so it always describes the graph being served.
+	if snap != nil && snap.Meta.GeneratedAt != "" {
+		if t, perr := time.Parse(time.RFC3339, snap.Meta.GeneratedAt); perr == nil {
 			st.GeneratedAt = t
 		}
 	}
-	if st.GeneratedAt.IsZero() {
-		// No (or unparseable) global receipt: use the loaded snapshot's own time.
-		if snap := e.Snapshot(); snap != nil && snap.Meta.GeneratedAt != "" {
-			if t, perr := time.Parse(time.RFC3339, snap.Meta.GeneratedAt); perr == nil {
-				st.GeneratedAt = t
-			}
+	// The graph-wide receipt is only a fallback, and only when it actually describes
+	// a repo in the loaded graph. ~/.enola/receipt.json is shared by every enola
+	// process on the machine and describes whichever generated last, so preferring it
+	// unconditionally reported a sibling terminal's timestamp as this graph's age.
+	if st.GeneratedAt.IsZero() && err == nil && gr != nil && gr.GeneratedAt != "" && describesAny(gr, loaded) {
+		if t, perr := time.Parse(time.RFC3339, gr.GeneratedAt); perr == nil {
+			st.GeneratedAt = t
 		}
 	}
 	if !st.GeneratedAt.IsZero() {
 		st.Age = now.Sub(st.GeneratedAt)
 		st.TooOld = st.Age > maxAge
 	}
 
-	for _, r := range e.stalenessEntries(gr, err) {
+	for _, r := range e.stalenessEntries(gr, err, snap, loaded) {
 		if r.Path == "" || r.Git == nil {
 			continue // non-git or unknown: covered by the age signal only
 		}
@@ -73,14 +84,63 @@ func (e *Engine) Staleness(maxAge time.Duration, now time.Time) Staleness {
 	return st
 }
 
-// stalenessEntries returns the per-repo entries to check: the global receipt's repo
-// list when available, otherwise a single synthetic entry from the in-memory
-// snapshot meta (single-repo / no global receipt).
-func (e *Engine) stalenessEntries(gr *facts.GraphReceipt, grErr error) []facts.GraphRepoEntry {
-	if grErr == nil && gr != nil && len(gr.Repos) > 0 {
-		return gr.Repos
+// loadedGraph returns the published snapshot together with the canonical absolute
+// paths of the repos that graph actually holds. Both come from ONE bundle load, so
+// repoPaths and snapshot are read as a consistent pair (the same reason
+// ResolveFactFile loads once).
+//
+// GenerateSnapshot only builds repoPaths in append mode — a single-repo snapshot
+// leaves it nil — so fall back to the snapshot's own RepoPath. An empty result means
+// nothing is loaded, which callers must treat as "cannot verify", never as "matches".
+func (e *Engine) loadedGraph() (*facts.Snapshot, map[string]bool) {
+	b := e.current.Load()
+	paths := make(map[string]bool, len(b.repoPaths)+1)
+	for _, p := range b.repoPaths {
+		if p != "" {
+			paths[canonicalRepoPath(p)] = true
+		}
+	}
+	if len(paths) == 0 && b.snapshot != nil && b.snapshot.Meta.RepoPath != "" {
+		paths[canonicalRepoPath(b.snapshot.Meta.RepoPath)] = true
+	}
+	return b.snapshot, paths
+}
+
+// describesAny reports whether the receipt names at least one repo that is in the
+// loaded graph. Paths are compared canonically because a receipt records the path as
+// the writing server resolved it (macOS /var vs /private/var).
+func describesAny(gr *facts.GraphReceipt, loaded map[string]bool) bool {
+	if gr == nil || len(loaded) == 0 {
+		return false
+	}
+	for _, r := range gr.Repos {
+		if r.Path != "" && loaded[canonicalRepoPath(r.Path)] {
+			return true
+		}
+	}
+	return false
+}
+
+// stalenessEntries returns the per-repo entries to check, restricted to repos that
+// are actually in the loaded graph. The global receipt supplies the snapshot-time git
+// baseline, but it is machine-wide — every enola process on the box overwrites it with
+// its OWN repo list — so taking it verbatim meant judging the freshness of a repo this
+// server had never loaded, in both directions: a false "commit moved" for a sibling
+// terminal's repo, and silence about the loaded repo because it was never checked.
+// Filtering here is what makes the wrong verdict unrepresentable rather than merely
+// unlikely. Falls back to a single synthetic entry from the snapshot meta.
+func (e *Engine) stalenessEntries(gr *facts.GraphReceipt, grErr error, snap *facts.Snapshot, loaded map[string]bool) []facts.GraphRepoEntry {
+	if grErr == nil && gr != nil && len(gr.Repos) > 0 && len(loaded) > 0 {
+		kept := make([]facts.GraphRepoEntry, 0, len(gr.Repos))
+		for _, r := range gr.Repos {
+			if r.Path != "" && loaded[canonicalRepoPath(r.Path)] {
+				kept = append(kept, r)
+			}
+		}
+		if len(kept) > 0 {
+			return kept
+		}
 	}
-	snap := e.Snapshot()
 	if snap == nil || snap.Meta.RepoPath == "" {
 		return nil
 	}
diff --git a/internal/engine/restore_test.go b/internal/engine/restore_test.go
@@ -2,8 +2,11 @@ package engine_test
 
 import (
 	"context"
+	"encoding/json"
+	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -157,3 +160,196 @@ func TestStaleness_CommitMoved(t *testing.T) {
 		t.Error("commit moved should make Stale() true")
 	}
 }
+
+// writeForeignGlobalReceipt writes ~/.enola/receipt.json describing a repo that is
+// NOT in the loaded graph — exactly what a second enola server in another agent
+// terminal leaves behind, since WriteGlobalReceipt rebuilds `repos` from its own
+// graph rather than merging. generatedAt is deliberately old and the commit is
+// deliberately wrong, so that adopting this file produces a visibly wrong verdict.
+//
+// The three staleness tests above set HOME to an empty temp dir specifically so
+// LoadGlobalReceipt MISSES; this helper is what exercises the branch where it hits.
+func writeForeignGlobalReceipt(t *testing.T, home, foreignRepoPath, generatedAt string) {
+	t.Helper()
+	receipt := map[string]any{
+		"generated_at":  generatedAt,
+		"enola_version": "dev",
+		"snapshot_id":   "sha256:sibling-terminal",
+		"fact_count":    4,
+		"repos": []map[string]any{{
+			"label": "foreign-sibling",
+			"path":  foreignRepoPath,
+			"git": map[string]any{
+				"ref":    "main",
+				"commit": "0000000000000000000000000000000000000000",
+				"dirty":  false,
+			},
+			"added_at":   generatedAt,
+			"fact_count": 4,
+		}},
+	}
+	data, err := json.Marshal(receipt)
+	if err != nil {
+		t.Fatal(err)
+	}
+	dir := filepath.Join(home, ".enola")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dir, "receipt.json"), data, 0o644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// initGitRepo creates a git repo with one commit and returns its HEAD.
+func initGitRepo(t *testing.T, repo string) string {
+	t.Helper()
+	git := func(args ...string) {
+		t.Helper()
+		cmd := exec.Command("git", append([]string{"-C", repo}, args...)...)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("git %v: %v\n%s", args, err, out)
+		}
+	}
+	git("init")
+	git("config", "user.email", "t@example.com")
+	git("config", "user.name", "t")
+	writeFile(t, filepath.Join(repo, "f.txt"), "hello\n")
+	git("add", ".")
+	git("commit", "-m", "init")
+	out, err := exec.Command("git", "-C", repo, "rev-parse", "HEAD").Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return strings.TrimSpace(string(out))
+}
+
+// TestStaleness_IgnoresForeignGlobalReceipt is the regression for the machine-wide
+// receipt cross-talk. ~/.enola/receipt.json is shared by every enola process on the
+// machine, so it routinely describes a DIFFERENT graph than the one this server
+// loaded. Staleness must judge the loaded graph, never whatever the file names.
+//
+// Observed before the fix: a graph snapshotted 13 seconds earlier over a clean tree
+// reported "graph is 5d old; <foreign> (commit moved)".
+func TestStaleness_IgnoresForeignGlobalReceipt(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+
+	loaded := t.TempDir()
+	head := initGitRepo(t, loaded)
+	foreign := t.TempDir()
+	initGitRepo(t, foreign)
+
+	now := time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC)
+	// The foreign receipt is 5 days old and names a commit that matches nothing.
+	writeForeignGlobalReceipt(t, home, foreign, now.Add(-5*24*time.Hour).Format(time.RFC3339))
+
+	cfg := config.Default()
+	eng, err := engine.New(cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// The loaded graph is fresh (1h) and its recorded commit is current HEAD.
+	eng.SetSnapshot(&facts.Snapshot{Meta: facts.SnapshotMeta{
+		RepoPath:    loaded,
+		GeneratedAt: now.Add(-1 * time.Hour).Format(time.RFC3339),
+		Git:         &facts.GitInfo{Ref: "main", Commit: head},
+	}})
+
+	st := eng.Staleness(24*time.Hour, now)
+	if st.TooOld {
+		t.Errorf("age was taken from the foreign receipt: TooOld=true for a 1h-old loaded snapshot (Age=%s)", st.Age)
+	}
+	for _, c := range st.Changed {
+		if c.Label == "foreign-sibling" {
+			t.Errorf("reported a repo that is not in the loaded graph: %+v", c)
+		}
+	}
+	if st.Stale() {
+		t.Errorf("fresh, unchanged loaded graph reported stale: TooOld=%v Changed=%+v", st.TooOld, st.Changed)
+	}
+}
+
+// TestStaleness_ReportsLoadedRepoDespiteForeignReceipt is the other direction: the
+// foreign receipt must not MASK real staleness in the loaded graph. Before the fix
+// stalenessEntries returned the receipt's repo list verbatim, so the loaded repo was
+// never git-checked at all and a moved HEAD went unreported.
+func TestStaleness_ReportsLoadedRepoDespiteForeignReceipt(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+
+	loaded := t.TempDir()
+	initGitRepo(t, loaded)
+	foreign := t.TempDir()
+	initGitRepo(t, foreign)
+
+	now := time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC)
+	writeForeignGlobalReceipt(t, home, foreign, now.Add(-5*24*time.Hour).Format(time.RFC3339))
+
+	cfg := config.Default()
+	eng, err := engine.New(cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Fresh by age, but the recorded commit does not match the loaded repo's HEAD.
+	eng.SetSnapshot(&facts.Snapshot{Meta: facts.SnapshotMeta{
+		RepoPath:    loaded,
+		GeneratedAt: now.Add(-1 * time.Hour).Format(time.RFC3339),
+		Git:         &facts.GitInfo{Ref: "main", Commit: "0000000000000000000000000000000000000000"},
+	}})
+
+	st := eng.Staleness(24*time.Hour, now)
+	if len(st.Changed) != 1 {
+		t.Fatalf("want exactly one Changed entry for the loaded repo, got %+v", st.Changed)
+	}
+	if got := st.Changed[0].Label; got != filepath.Base(loaded) {
+		t.Errorf("Changed names %q, want the loaded repo %q", got, filepath.Base(loaded))
+	}
+	if st.Changed[0].Reason != "commit moved" {
+		t.Errorf("got Reason=%q, want \"commit moved\"", st.Changed[0].Reason)
+	}
+}
+
+// TestStaleness_AgeFromLoadedSnapshotNotForeignReceipt isolates the age signal from
+// the git signal: same commit on both sides, so only the timestamp can differ. The
+// loaded graph is 1h old and the foreign receipt is 5 days old.
+func TestStaleness_AgeFromLoadedSnapshotNotForeignReceipt(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git not available")
+	}
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+
+	loaded := t.TempDir()
+	head := initGitRepo(t, loaded)
+
+	now := time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC)
+	// Foreign receipt points at the LOADED repo path but carries a 5-day-old
+	// timestamp, so only the age source distinguishes pass from fail.
+	writeForeignGlobalReceipt(t, home, loaded, now.Add(-5*24*time.Hour).Format(time.RFC3339))
+
+	cfg := config.Default()
+	eng, err := engine.New(cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	eng.SetSnapshot(&facts.Snapshot{Meta: facts.SnapshotMeta{
+		RepoPath:    loaded,
+		GeneratedAt: now.Add(-1 * time.Hour).Format(time.RFC3339),
+		Git:         &facts.GitInfo{Ref: "main", Commit: head},
+	}})
+
+	st := eng.Staleness(24*time.Hour, now)
+	if st.TooOld {
+		t.Errorf("age came from the receipt (5d), not the loaded snapshot (1h): Age=%s", st.Age)
+	}
+	if want := 1 * time.Hour; st.Age != want {
+		t.Errorf("Age=%s, want %s (the loaded snapshot's own generated_at)", st.Age, want)
+	}
+}
