diff --git a/internal/mcp/ensure_fresh_self_heal_test.go b/internal/mcp/ensure_fresh_self_heal_test.go new file mode 100644 index 000000000..2af049b42 --- /dev/null +++ b/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") +} diff --git a/internal/mcp/read_file_overlay_freshness_test.go b/internal/mcp/read_file_overlay_freshness_test.go new file mode 100644 index 000000000..ed46ae738 --- /dev/null +++ b/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") +} diff --git a/internal/mcp/tools_enhancements.go b/internal/mcp/tools_enhancements.go index d31940796..a40545468 100644 --- a/internal/mcp/tools_enhancements.go +++ b/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() { diff --git a/internal/mcp/tools_fileops.go b/internal/mcp/tools_fileops.go index b896fa407..389e296c7 100644 --- a/internal/mcp/tools_fileops.go +++ b/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"))