fix(mcp): self-heal stale read-tool responses after external file edits

internal/mcp/ensure_fresh_self_heal_test.go

@@ -0,0 +1,103 @@
+package mcp
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/zzet/gortex/internal/config"
+	"github.com/zzet/gortex/internal/graph"
+	"github.com/zzet/gortex/internal/indexer"
+	"github.com/zzet/gortex/internal/parser"
+	"github.com/zzet/gortex/internal/parser/languages"
+	"github.com/zzet/gortex/internal/query"
+	"github.com/zzet/gortex/internal/search"
+	"os"
+	"path/filepath"
+)
+
+// TestEnsureFresh_MultiRepoSelfHealsStaleFile is the regression test for the
+// stale-read class: in multi-repo mode ensureFresh used to return early and do
+// nothing, so a file changed on disk since indexing kept serving its old body
+// through the index-backed read tools. It must now route the path to its owning
+// per-repo indexer, detect the drift, and re-index — leaving the graph current.
+func TestEnsureFresh_MultiRepoSelfHealsStaleFile(t *testing.T) {
+	repoA := setupMiniRepo(t, "repo-a")
+	repoB := setupMiniRepo(t, "repo-b")
+
+	tmpCfg := filepath.Join(t.TempDir(), "config.yaml")
+	gc := &config.GlobalConfig{Repos: []config.RepoEntry{
+		{Path: repoA, Name: "repo-a"},
+		{Path: repoB, Name: "repo-b"},
+	}}
+	gc.SetConfigPath(tmpCfg)
+	require.NoError(t, gc.Save())
+
+	cm, err := config.NewConfigManager(tmpCfg)
+	require.NoError(t, err)
+
+	reg := parser.NewRegistry()
+	reg.Register(languages.NewGoExtractor())
+
+	g := graph.New()
+	mi := indexer.NewMultiIndexer(g, reg, search.NewBM25(), cm, zap.NewNop())
+	_, err = mi.IndexAll()
+	require.NoError(t, err)
+	require.True(t, mi.IsMultiRepo())
+
+	eng := query.NewEngine(g)
+	srv := NewServer(eng, g, nil, nil, zap.NewNop(), nil, MultiRepoOptions{
+		ConfigManager: cm,
+		MultiIndexer:  mi,
+	})
+
+	require.NotEmpty(t, g.FindNodesByName("Hello"), "baseline symbol indexed")
+	require.Empty(t, g.FindNodesByName("HelloAgain"), "the new symbol is not on disk yet")
+
+	// Change the file outside the indexer (as a native edit / external write
+	// would), so only an on-read freshness check can notice.
+	bumpFile(t, filepath.Join(repoA, "main.go"),
+		"package main\n\nfunc Hello() {}\n\nfunc HelloAgain() {}\n")
+
+	refreshed := srv.ensureFresh([]string{"repo-a/main.go"})
+	assert.Equal(t, []string{"repo-a/main.go"}, refreshed,
+		"multi-repo ensureFresh must re-index the drifted file (pre-fix it returned nil)")
+	assert.NotEmpty(t, g.FindNodesByName("HelloAgain"),
+		"the graph reflects the new on-disk content after self-heal")
+
+	// A follow-up call is a no-op: the recorded mtime advanced, so the file is
+	// no longer classified stale and is not re-indexed again.
+	assert.Empty(t, srv.ensureFresh([]string{"repo-a/main.go"}),
+		"an already-fresh file is not re-indexed a second time")
+}
+
+// TestEnsureFresh_SingleRepoSelfHealsStaleFile covers the single-repo path with
+// no active watcher: the on-read freshness check still re-indexes a drifted
+// file so the graph does not serve a stale body.
+func TestEnsureFresh_SingleRepoSelfHealsStaleFile(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "main.go"),
+		[]byte("package main\n\nfunc Hello() {}\n"), 0o644))
+
+	g := graph.New()
+	reg := parser.NewRegistry()
+	languages.RegisterAll(reg)
+	idx := indexer.New(g, reg, config.Default().Index, zap.NewNop())
+	_, err := idx.Index(dir)
+	require.NoError(t, err)
+
+	eng := query.NewEngine(g)
+	srv := NewServer(eng, g, idx, nil, zap.NewNop(), nil) // watcher nil → on-read refresh active
+
+	require.Empty(t, g.FindNodesByName("HelloAgain"))
+
+	bumpFile(t, filepath.Join(dir, "main.go"),
+		"package main\n\nfunc Hello() {}\n\nfunc HelloAgain() {}\n")
+
+	refreshed := srv.ensureFresh([]string{"main.go"})
+	assert.Equal(t, []string{"main.go"}, refreshed)
+	assert.NotEmpty(t, g.FindNodesByName("HelloAgain"),
+		"single-repo self-heal re-indexed the drifted file")
+}

internal/mcp/read_file_overlay_freshness_test.go

@@ -0,0 +1,77 @@
+package mcp
+
+import (
+	"context"
+	"crypto/sha1"
+	"encoding/hex"
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/zzet/gortex/internal/daemon"
+)
+
+func readFileResultMap(t *testing.T, srv *Server, ctx context.Context, path string) map[string]any {
+	t.Helper()
+	res := callToolByName(t, srv, ctx, "read_file", map[string]any{"path": path})
+	require.False(t, res.IsError, "read_file: %s", toolText(res))
+	var out map[string]any
+	require.NoError(t, json.Unmarshal([]byte(toolText(res)), &out))
+	return out
+}
+
+// TestReadFile_OverlayDriftSurfacesError pins the existing safety: when an
+// editor-buffer overlay has drifted from disk (its captured base no longer
+// matches), read_file must not serve the stale buffer — the overlay view guard
+// turns the call into a drift error so the client re-reads, exactly as the
+// graph tools do.
+func TestReadFile_OverlayDriftSurfacesError(t *testing.T) {
+	srv, _, targetFile, _ := setupOverlayServer(t)
+	sessID := "rf-drift"
+	require.NoError(t, srv.OverlayManager().RegisterWithID(sessID, ""))
+	require.NoError(t, srv.OverlayManager().Push(sessID, daemon.OverlayFile{
+		Path:    targetFile,
+		Content: "package main\n\nfunc OverlayOnly() {}\n",
+		BaseSHA: "0000000000000000000000000000000000000000", // never matches disk → drift
+	}, nil))
+
+	ctx := WithSessionID(context.Background(), sessID)
+	res := callToolByName(t, srv, ctx, "read_file", map[string]any{"path": filepath.Base(targetFile)})
+	require.True(t, res.IsError, "a drifted overlay must not be served silently")
+	assert.Contains(t, toolText(res), "overlay base SHA mismatch")
+}
+
+// TestReadFile_FreshOverlayIsServed: a non-drifted overlay (its base matches
+// disk) is still served as the buffer view, flagged so the agent knows it is
+// reading an editor buffer rather than the file currently on disk.
+func TestReadFile_FreshOverlayIsServed(t *testing.T) {
+	srv, _, targetFile, _ := setupOverlayServer(t)
+
+	data, err := os.ReadFile(targetFile)
+	require.NoError(t, err)
+	h := sha1.New()
+	fmt.Fprintf(h, "blob %d\x00", len(data))
+	_, _ = h.Write(data)
+	baseSHA := hex.EncodeToString(h.Sum(nil))
+
+	sessID := "rf-fresh"
+	require.NoError(t, srv.OverlayManager().RegisterWithID(sessID, ""))
+	require.NoError(t, srv.OverlayManager().Push(sessID, daemon.OverlayFile{
+		Path:    targetFile,
+		Content: "package main\n\nfunc Target() {}\n\nfunc OverlayOnly() {}\n",
+		BaseSHA: baseSHA,
+	}, nil))
+
+	ctx := WithSessionID(context.Background(), sessID)
+	out := readFileResultMap(t, srv, ctx, filepath.Base(targetFile))
+
+	content, _ := out["content"].(string)
+	assert.Contains(t, content, "OverlayOnly", "the fresh overlay buffer is served")
+	assert.Equal(t, "overlay", out["served_from"], "served_from flags the overlay provenance")
+	assert.Nil(t, out["overlay_bypassed"], "no drift bypass on a fresh overlay")
+}

internal/mcp/tools_enhancements.go

@@ -21,68 +21,92 @@ import (
 	"github.com/zzet/gortex/internal/coverage"
 	"github.com/zzet/gortex/internal/excludes"
 	"github.com/zzet/gortex/internal/graph"
+	"github.com/zzet/gortex/internal/indexer"
 	"github.com/zzet/gortex/internal/persistence"
 	"github.com/zzet/gortex/internal/query"
 	"github.com/zzet/gortex/internal/tokens"
 	"go.uber.org/zap"
 )
 
-// ensureFresh checks if any of the given file paths are stale (modified on disk
-// since last index) and re-indexes up to 5 of them when watch mode is not active.
-// Returns the list of file paths that were refreshed.
+// ensureFresh re-indexes any of the given file paths whose on-disk content has
+// drifted from the indexed snapshot, so a subsequent graph read serves current
+// data instead of a stale body. Returns the paths that were refreshed (capped at
+// a handful per call).
+//
+// The self-heal is gated on IsTrackedStale, which is false for untracked, new,
+// or already-current files — only a genuinely changed, already-indexed file is
+// re-indexed. That gate is what makes this safe in multi-repo mode: an earlier
+// version keyed the staleness check off the lone single-Indexer, whose mtime
+// map is empty for cross-repo paths, so every file looked stale and the
+// resulting mass re-index raced the live read surface and crashed the transport.
+// Routing each path to its owning per-repo indexer keeps the check accurate and
+// the work bounded to the file the caller is about to read.
 func (s *Server) ensureFresh(filePaths []string) []string {
-	// Skip when watcher is active — it handles updates.
-	if s.watcher != nil {
-		return nil
-	}
-	// In multi-repo mode the legacy single-Indexer's fileMtimes is
-	// always empty for cross-repo paths, so IsStale returns true for
-	// every file → IndexFile fires → race with the daemon's read
-	// surface, which has been observed to crash the MCP transport
-	// (a concurrency hazard against the live read surface). The MultiIndexer's own
-	// per-repo watcher / Reconcile path owns freshness here; the
-	// single-Indexer auto-refresh is dead weight that does more harm
-	// than good.
-	if s.multiIndexer != nil {
-		return nil
-	}
-	if s.indexer == nil {
-		return nil
-	}
-
 	var refreshed []string
-	limit := 5
+	const limit = 5
 	for _, fp := range filePaths {
 		if len(refreshed) >= limit {
 			break
 		}
-		if s.indexer.IsStale(fp) {
-			absPath := fp
-			if root := s.indexer.RootPath(); root != "" {
-				absPath = filepath.Join(root, fp)
-			}
-
-			// In multi-repo mode, the file path may be prefixed with a repo name
-			// (e.g. "ade/internal/..."). If the resolved path doesn't exist, try
-			// resolving via the MultiIndexer which knows each repo's root.
-			if _, statErr := os.Stat(absPath); statErr != nil && s.multiIndexer != nil {
-				resolved := s.multiIndexer.ResolveFilePath(fp)
-				if resolved != "" {
-					absPath = resolved
-				}
-			}
+		idx, absPath := s.freshnessIndexer(fp)
+		if idx == nil {
+			continue
+		}
+		root := idx.RootPath()
+		if root == "" {
+			continue
+		}
+		rel, err := filepath.Rel(root, absPath)
+		if err != nil || strings.HasPrefix(rel, "..") {
+			continue
+		}
+		// IsTrackedStale is false for untracked / new / current files, so
+		// only a known-and-changed file triggers a re-index — no mass churn.
+		if !idx.IsTrackedStale(rel) {
+			continue
+		}
+		if err := idx.IndexFile(absPath); err != nil {
+			s.logger.Warn("auto re-index failed",
+				zap.String("file", fp),
+				zap.String("resolved", absPath),
+				zap.Error(err))
+			continue
+		}
+		// Advance the recorded mtime so a follow-up read in the same window
+		// sees the file as current and skips a redundant re-index.
+		idx.RefreshFileMtime(absPath)
+		refreshed = append(refreshed, fp)
+	}
+	return refreshed
+}
 
-			if err := s.indexer.IndexFile(absPath); err != nil {
-				s.logger.Warn("auto re-index failed",
-					zap.String("file", fp),
-					zap.String("resolved", absPath),
-					zap.Error(err))
-				continue
+// freshnessIndexer resolves a repo-prefixed, repo-relative, or absolute path to
+// the indexer that owns it and the file's absolute path, for both single- and
+// multi-repo daemons. In single-repo mode an active file watcher already owns
+// freshness, so on-read auto-refresh stands down rather than fight it; in
+// multi-repo mode each path is routed to its per-repo indexer, whose mtime map
+// is populated — which is what makes the staleness check trustworthy.
+func (s *Server) freshnessIndexer(fp string) (*indexer.Indexer, string) {
+	if s.multiIndexer != nil {
+		abs := fp
+		if !filepath.IsAbs(abs) {
+			if resolved := s.multiIndexer.ResolveFilePath(fp); resolved != "" {
+				abs = resolved
 			}
-			refreshed = append(refreshed, fp)
 		}
+		idx, _ := s.multiIndexer.IndexerForFile(abs)
+		return idx, abs
 	}
-	return refreshed
+	if s.indexer == nil || s.watcher != nil {
+		return nil, ""
+	}
+	abs := fp
+	if !filepath.IsAbs(abs) {
+		if root := s.indexer.RootPath(); root != "" {
+			abs = filepath.Join(root, fp)
+		}
+	}
+	return s.indexer, abs
 }
 
 func (s *Server) registerEnhancementTools() {

internal/mcp/tools_fileops.go

@@ -947,10 +947,15 @@ func (s *Server) handleReadFile(ctx context.Context, req mcp.CallToolRequest) (*
 	if info.IsDir() {
 		return mcp.NewToolResultError(fmt.Sprintf("path %q is a directory", rawPath)), nil
 	}
-	// Honour the editor-buffer overlay if one is active for this path.
+	// Honour the editor-buffer overlay if one is active for this path. A
+	// drifted overlay is already rejected upstream by the overlay view
+	// guard; what reaches here is a live buffer, which we flag as such so
+	// the caller knows the bytes are an unsaved editor view, not disk.
 	var content []byte
+	servedFromOverlay := false
 	if buf, ok := s.overlayContentFor(ctx, absPath); ok {
 		content = []byte(buf)
+		servedFromOverlay = true
 	} else {
 		b, rerr := os.ReadFile(absPath)
 		if rerr != nil {
@@ -1040,6 +1045,9 @@ func (s *Server) handleReadFile(ctx context.Context, req mcp.CallToolRequest) (*
 	if secretsRedacted {
 		result["secrets_redacted"] = true
 	}
+	if servedFromOverlay {
+		result["served_from"] = "overlay"
+	}
 	if bodiesElided {
 		result["bodies_elided"] = true
 		if len(keptSymbols) > 0 {
@@ -1060,6 +1068,10 @@ func (s *Server) handleReadFile(ctx context.Context, req mcp.CallToolRequest) (*
 	// Omission notes: tell the model what the payload deliberately
 	// leaves out or reshapes, so it does not reason about absent code.
 	omissions := pathOmissions(relPath)
+	if servedFromOverlay {
+		omissions = append(omissions, omission("overlay",
+			"served from an active editor-buffer overlay, not the file currently on disk"))
+	}
 	if isBinary {
 		omissions = append(omissions, omission("binary",
 			"file is binary — the content field holds raw bytes, not source text"))