Skip to content

Commit c3ca817

Browse files
committed
fix(diff): detect a stale current snapshot at the diff boundary
diff_snapshot and validate_architecture_change both labelled one side "current" without ever checking that it still matched the working tree. The only age check was relative — baseline vs current — so two equally stale snapshots compared clean, and an inverted pair was silent because a non-positive gap shared its signal with "no timestamp recorded". Engine.Drift re-walks under the current config and re-hashes, comparing against the snapshot's recorded file hashes, and both tools now surface a comparability warning naming the drifted files. This is a filesystem comparison, not a VCS one: the snapshot already records a hash per walked file, so it answers exactly and works for repos that are not git working trees. Exact drift also removes the need for a wall-clock threshold — unchanged content makes age irrelevant, and changed content makes it beside the point. The per-tool-call freshness banner is unchanged; this runs only at a deliberate decision point, where re-hashing is affordable. Known limit, documented: recorded hashes cover the repo a snapshot indexed, so in append mode drift checks the most-recently-indexed repo and reports "cannot verify" for the others. Fact output is unchanged; no cacheVersion bump.
1 parent 189be2a commit c3ca817

6 files changed

Lines changed: 438 additions & 12 deletions

File tree

internal/diff/diff.go

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -277,29 +277,62 @@ func compareMeta(base, cur facts.SnapshotMeta) Comparability {
277277
// GeneratedAt was printed on line 3 and never compared. An 11-day-old baseline from
278278
// the same enola version with the same extractors yields Comparable:true, zero
279279
// warnings, and a confident 24-regression report for a change that touched no facts.
280-
if age, ok := baselineAgeDays(base.GeneratedAt, cur.GeneratedAt); ok && age >= staleBaselineDays {
280+
switch gap, ok := baselineGap(base.GeneratedAt, cur.GeneratedAt); {
281+
case !ok:
282+
// Unparseable or missing timestamp — the pre-receipt case, which already has
283+
// its own softer note above. Nothing further to say.
284+
case gap <= 0:
285+
// The baseline is NEWER than the snapshot being compared, i.e. the "current"
286+
// side is stale. This used to be silent: it shared the ok=false signal with
287+
// "no timestamp recorded", so the provenance line rendered backwards and
288+
// nothing flagged it.
289+
c.Warnings = append(c.Warnings,
290+
"the baseline is newer than the snapshot it is being compared against — the current snapshot "+
291+
"predates the baseline, so it does not contain your change; re-run generate_snapshot")
292+
case int(gap.Hours()/24) >= staleBaselineDays:
281293
c.Warnings = append(c.Warnings, fmt.Sprintf(
282294
"baseline is %d days older than the current snapshot — anything the repo itself changed in "+
283295
"between will appear as part of this delta; re-pin the baseline (set_baseline) to compare only your change",
284-
age))
296+
int(gap.Hours()/24)))
285297
}
286298

287299
c.Comparable = len(c.Warnings) == 0
288300
return c
289301
}
290302

303+
// AddWarning appends a comparability warning and clears Comparable, preserving the
304+
// `Comparable == (len(Warnings) == 0)` invariant that compareMeta establishes.
305+
//
306+
// It exists so callers outside this package can contribute a caveat without setting
307+
// the two fields independently — a caller that appended to Warnings and forgot
308+
// Comparable would render the caveat above the delta while still reporting the pair as
309+
// comparable in JSON. Used for facts that only the caller can know, notably whether
310+
// the current snapshot still matches the working tree (engine.Drift).
311+
func (d *SnapshotDiff) AddWarning(w string) {
312+
if w == "" {
313+
return
314+
}
315+
d.Comparability.Warnings = append(d.Comparability.Warnings, w)
316+
d.Comparability.Comparable = false
317+
}
318+
291319
// missingFrom returns the elements of want that are absent from have, sorted.
292320
// staleBaselineDays is how far apart two snapshots may be before the delta is more
293321
// likely to describe the repo's own drift than the caller's change. Deliberately a
294322
// warning, not a refusal: a long-lived baseline is a legitimate way to measure a
295323
// multi-day refactor, and the caller is the only one who knows which they meant.
296324
const staleBaselineDays = 3
297325

298-
// baselineAgeDays returns how many whole days older the baseline is than the current
299-
// snapshot. Both timestamps are RFC3339 UTC (facts.SnapshotMeta.GeneratedAt); an
300-
// unparseable or missing one yields ok=false, which is the pre-receipt baseline case
301-
// that already has its own warning.
302-
func baselineAgeDays(baseTS, curTS string) (int, bool) {
326+
// baselineGap returns how much older the baseline is than the current snapshot: a
327+
// positive duration when the baseline came first, non-positive when the pair is
328+
// inverted (a stale "current" side). Both timestamps are RFC3339 UTC
329+
// (facts.SnapshotMeta.GeneratedAt).
330+
//
331+
// ok=false means only that a timestamp was missing or unparseable — the pre-receipt
332+
// case, which has its own note. It deliberately does NOT fold in the inverted case:
333+
// conflating "cannot tell" with "baseline is newer" is what made an inverted pair
334+
// silent, so callers must distinguish the two.
335+
func baselineGap(baseTS, curTS string) (time.Duration, bool) {
303336
b, err := time.Parse(time.RFC3339, baseTS)
304337
if err != nil {
305338
return 0, false
@@ -308,11 +341,7 @@ func baselineAgeDays(baseTS, curTS string) (int, bool) {
308341
if err != nil {
309342
return 0, false
310343
}
311-
d := c.Sub(b)
312-
if d <= 0 {
313-
return 0, false
314-
}
315-
return int(d.Hours() / 24), true
344+
return c.Sub(b), true
316345
}
317346

318347
func missingFrom(want, have []string) []string {

internal/diff/diff_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,3 +554,41 @@ func TestCompareMeta_StaleBaselineWarns(t *testing.T) {
554554
}
555555
}
556556
}
557+
558+
// TestCompareMeta_BaselineNewerThanCurrentWarns covers the inverted pair, which used to
559+
// be silent: baselineAgeDays returns ok=false for a non-positive gap, sharing that
560+
// signal with "no timestamp recorded", so a baseline NEWER than the snapshot being
561+
// compared produced zero warnings. That is the sharpest form of a stale current
562+
// snapshot — the provenance line renders backwards ("Baseline <newer> → current
563+
// <older>") and nothing says so.
564+
func TestCompareMeta_BaselineNewerThanCurrentWarns(t *testing.T) {
565+
meta := func(ts string) facts.SnapshotMeta {
566+
return facts.SnapshotMeta{
567+
RepoPath: "/repo", EnolaVersion: "dev", GeneratedAt: ts,
568+
Extractors: []string{"go"},
569+
}
570+
}
571+
// Baseline is 13 minutes NEWER than "current".
572+
inverted := compareMeta(meta("2026-07-30T06:30:00Z"), meta("2026-07-30T06:17:24Z"))
573+
var found bool
574+
for _, w := range inverted.Warnings {
575+
if strings.Contains(w, "newer") {
576+
found = true
577+
}
578+
}
579+
if !found {
580+
t.Errorf("a baseline newer than the current snapshot produced no warning: %v", inverted.Warnings)
581+
}
582+
if inverted.Comparable {
583+
t.Error("an inverted baseline/current pair must not be reported as comparable")
584+
}
585+
586+
// A missing timestamp is a DIFFERENT condition and keeps its own softer note; it
587+
// must not start claiming the pair is inverted.
588+
missing := compareMeta(meta(""), meta("2026-07-30T06:17:24Z"))
589+
for _, w := range missing.Warnings {
590+
if strings.Contains(w, "newer") {
591+
t.Errorf("a missing baseline timestamp was reported as an inverted pair: %q", w)
592+
}
593+
}
594+
}

internal/engine/drift.go

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
package engine
2+
3+
import (
4+
"fmt"
5+
"sort"
6+
)
7+
8+
// Drift reports how a repository's files have moved since its snapshot was taken:
9+
// which were added, removed, or edited. It is the answer to "does the graph I am
10+
// about to diff still describe the code on disk?".
11+
//
12+
// It is deliberately a FILESYSTEM comparison, not a VCS one. The snapshot already
13+
// records a content hash per walked file (facts.SnapshotMeta.FileHashes), so the
14+
// question can be answered exactly, from enola's own data, for repos that are not
15+
// git working trees at all. The git-derived signals cannot answer it: a commit that
16+
// has not moved says nothing about the working tree, and Git.Dirty is a single
17+
// boolean, so a tree that was ALREADY modified when the snapshot ran cannot be
18+
// distinguished from the same tree modified further (see internal/engine/freshness.go
19+
// and the freshness dossier's GAP-FR-03).
20+
//
21+
// Unknown means the comparison could not be made — the snapshot recorded no file
22+
// hashes, which is the case for a pre-receipt snapshot or a restore whose
23+
// snapshot.meta.json failed to load. Callers must surface that as "cannot verify"
24+
// and never as a clean tree: silence and confirmation are different answers.
25+
type Drift struct {
26+
Added []string // walked now, absent from the snapshot
27+
Removed []string // in the snapshot, absent now
28+
Modified []string // present in both, different content hash
29+
Unknown bool // no recorded hashes to compare against
30+
}
31+
32+
// Any reports whether any file was added, removed, or edited. It is false when
33+
// Unknown — an unanswerable comparison is not evidence of change.
34+
func (d Drift) Any() bool {
35+
return len(d.Added) > 0 || len(d.Removed) > 0 || len(d.Modified) > 0
36+
}
37+
38+
// Count is the total number of drifted paths.
39+
func (d Drift) Count() int {
40+
return len(d.Added) + len(d.Removed) + len(d.Modified)
41+
}
42+
43+
// Summary renders a one-line description for a comparability warning, naming counts
44+
// and a bounded sample of paths so the caller can see WHICH files moved without the
45+
// message becoming unbounded on a large refactor.
46+
func (d Drift) Summary(maxPaths int) string {
47+
if d.Unknown {
48+
return "the snapshot recorded no file hashes, so whether it still matches the working tree cannot be verified"
49+
}
50+
if !d.Any() {
51+
return ""
52+
}
53+
parts := make([]string, 0, 3)
54+
if n := len(d.Modified); n > 0 {
55+
parts = append(parts, fmt.Sprintf("%d modified", n))
56+
}
57+
if n := len(d.Added); n > 0 {
58+
parts = append(parts, fmt.Sprintf("%d added", n))
59+
}
60+
if n := len(d.Removed); n > 0 {
61+
parts = append(parts, fmt.Sprintf("%d removed", n))
62+
}
63+
sample := make([]string, 0, maxPaths)
64+
for _, group := range [][]string{d.Modified, d.Added, d.Removed} {
65+
for _, p := range group {
66+
if len(sample) >= maxPaths {
67+
break
68+
}
69+
sample = append(sample, p)
70+
}
71+
}
72+
msg := ""
73+
for i, p := range parts {
74+
if i > 0 {
75+
msg += ", "
76+
}
77+
msg += p
78+
}
79+
if len(sample) > 0 {
80+
msg += " (e.g. "
81+
for i, p := range sample {
82+
if i > 0 {
83+
msg += ", "
84+
}
85+
msg += p
86+
}
87+
if d.Count() > len(sample) {
88+
msg += ", …"
89+
}
90+
msg += ")"
91+
}
92+
return msg
93+
}
94+
95+
// Drift re-walks repoPath under the current config and compares each file's content
96+
// hash against the hashes the loaded snapshot recorded, returning the exact set of
97+
// differences.
98+
//
99+
// This re-reads and re-hashes every walked file, which costs roughly the walk+hash
100+
// stages of a snapshot. That is why it is an on-demand call for the tools that are
101+
// ABOUT to make a claim about the change (diff_snapshot,
102+
// validate_architecture_change) rather than part of the per-tool-call freshness
103+
// banner: the banner is a cheap proactive nudge, this is an exact answer at a
104+
// decision point.
105+
//
106+
// The snapshot's recorded hashes cover the repo that snapshot indexed. In multi-repo
107+
// (append) mode that is the most recently indexed repo, so a caller asking about a
108+
// different repo in the graph gets Unknown rather than a wrong answer.
109+
func (e *Engine) Drift(repoPath string) (Drift, error) {
110+
b := e.current.Load()
111+
if b.snapshot == nil {
112+
return Drift{Unknown: true}, nil
113+
}
114+
115+
recorded := make(map[string]string, len(b.snapshot.Meta.FileHashes))
116+
for _, fh := range b.snapshot.Meta.FileHashes {
117+
recorded[fh.Path] = fh.Hash
118+
}
119+
if len(recorded) == 0 {
120+
return Drift{Unknown: true}, nil
121+
}
122+
123+
files, _, _, err := e.walkRepo(repoPath)
124+
if err != nil {
125+
return Drift{}, fmt.Errorf("walking %s: %w", repoPath, err)
126+
}
127+
current := e.computeFileHashes(repoPath, files)
128+
129+
var d Drift
130+
for path, curHash := range current {
131+
prevHash, existed := recorded[path]
132+
switch {
133+
case !existed:
134+
d.Added = append(d.Added, path)
135+
case prevHash != curHash:
136+
d.Modified = append(d.Modified, path)
137+
}
138+
}
139+
for path := range recorded {
140+
if _, stillThere := current[path]; !stillThere {
141+
d.Removed = append(d.Removed, path)
142+
}
143+
}
144+
145+
// Deterministic order: these paths reach a user-facing warning.
146+
sort.Strings(d.Added)
147+
sort.Strings(d.Removed)
148+
sort.Strings(d.Modified)
149+
return d, nil
150+
}

0 commit comments

Comments
 (0)