From 16cb03a33437b18ca104b8d8a2abe21a1b2a6aea Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sat, 31 Oct 2026 17:26:23 +0100 Subject: [PATCH 1/6] mcp: label change answers instead of refusing on pending edits explain_change_impact and detect_changes waited up to 3 s on every pending reindex receipt in the repository and then failed with "refused a stale graph". The edit that triggered the wait was usually in a file unrelated to the answer, and an id that did not resolve widened the wait to every receipt, so concurrent agents got refusals for most impact calls and each one held a dispatch slot for the full wait. Both handlers now wait at most 250 ms, and only for the edits that can change the answer: the queried symbols' own files, files with edges into them, files whose current bytes name them as a whole identifier (a call the graph has not read yet), and for detect_changes the changed files of the diff, which is now mapped files-first so the scope is known before the wait. An unresolved id contributes the file and name its spelling carries instead of widening the scope. Whatever in scope is still pending or failed labels the answer (complete:false, graph_freshness, freshness_scope, pending_files, pending_file_count, and recovery_hint when failed); the walk always runs. An earlier wait_deadline ends the wait sooner, including one that passed between admission and the handler reading it. TOON and plain text answers lead with a "relations: (N files)" line, and GCX keeps GCX1 as line 1 with the label as header meta and the line as a comment on line 2. Every answer reports barrier_wait_ms. Only require_fresh refuses, once its wait_deadline passes with the scope still lagging. The label lives in relation_freshness.go so the relation-reading mutators can share it. --- internal/analysis/diffmap.go | 7 + internal/mcp/gcx.go | 5 + internal/mcp/mutation_freshness_test.go | 103 +++++++--- internal/mcp/relation_freshness.go | 258 ++++++++++++++++++++++++ internal/mcp/relation_freshness_test.go | 194 ++++++++++++++++++ internal/mcp/tools_analysis.go | 72 +++++-- 6 files changed, 590 insertions(+), 49 deletions(-) create mode 100644 internal/mcp/relation_freshness.go create mode 100644 internal/mcp/relation_freshness_test.go diff --git a/internal/analysis/diffmap.go b/internal/analysis/diffmap.go index ca9b76fe7..b58f8643a 100644 --- a/internal/analysis/diffmap.go +++ b/internal/analysis/diffmap.go @@ -113,6 +113,13 @@ func MapGitDiffContext(ctx context.Context, g graph.Reader, repoRoot, repoPrefix return joinHunksToSymbolsContext(ctx, g, repoPrefix, hunks, files) } +// JoinDiffSymbolsContext maps a file-only diff (MapGitDiffContext with a nil +// reader) to the symbols g indexes, without running git again. A caller that +// needs the changed files before it can pick its reader diffs once this way. +func JoinDiffSymbolsContext(ctx context.Context, g graph.Reader, repoPrefix string, diff *DiffResult) (*DiffResult, error) { + return joinHunksToSymbolsContext(ctx, g, repoPrefix, diff.Hunks, diff.FileChanges) +} + // PathDomain names the vocabulary a file path is spelled in. The two overlap, // so the domain has to travel with the path rather than be recovered from it: // in a repo whose tree carries a top-level directory named like the repo diff --git a/internal/mcp/gcx.go b/internal/mcp/gcx.go index ef2b16ae0..2a4caddb3 100644 --- a/internal/mcp/gcx.go +++ b/internal/mcp/gcx.go @@ -1937,8 +1937,13 @@ func encodeChangeImpact(result map[string]any) ([]byte, error) { communityNote, _ := result["community_note"].(string) contractRiskUpgrade, _ := result["contract_risk_upgrade"].(string) + var meta []string + if waited, ok := result["barrier_wait_ms"].(int64); ok { + meta = append(meta, "barrier_wait_ms", strconv.FormatInt(waited, 10)) + } sumEnc := newGCX(&buf, "explain_change_impact.summary", []string{"risk", "summary", "total_affected", "cross_repo", "processes", "communities", "test_files", "warning", "note", "risk_upgrade"}, + meta..., ) if err := sumEnc.WriteRow( string(risk), diff --git a/internal/mcp/mutation_freshness_test.go b/internal/mcp/mutation_freshness_test.go index d9dd4b21c..230398750 100644 --- a/internal/mcp/mutation_freshness_test.go +++ b/internal/mcp/mutation_freshness_test.go @@ -434,41 +434,44 @@ func TestMutationReposForSymbolIDsUnresolvedWidensBarrier(t *testing.T) { } } -func TestChangeImpactFreshnessGuardAndRetry(t *testing.T) { +func TestChangeImpactLabelsLaggingReceiptsInScope(t *testing.T) { g := graph.New() g.AddNode(&graph.Node{ ID: "repo-a::target", Kind: graph.KindFunction, Name: "target", - FilePath: "target.go", + FilePath: "/repo-a/target.go", StartLine: 1, EndLine: 1, Language: "go", RepoPrefix: "repo-a", }) s := &Server{ - graph: g, - engine: query.NewEngine(g), - session: newSessionState(), - mutationSafetyWait: time.Millisecond, + graph: g, + engine: query.NewEngine(g), + session: newSessionState(), } receipt := pendingFreshnessReceipt(s, "receipt-impact", "repo-a", "/repo-a/target.go", 11) req := mcpgo.CallToolRequest{Params: mcpgo.CallToolParams{ Name: "explain_change_impact", Arguments: map[string]any{"ids": "repo-a::target", "format": "json"}, }} - - result, err := s.handleEnhancedChangeImpact(context.Background(), req) - if err != nil { - t.Fatalf("pending impact handler error: %v", err) + impact := func() map[string]any { + t.Helper() + result, err := s.handleEnhancedChangeImpact(context.Background(), req) + if err != nil { + t.Fatalf("impact handler error: %v", err) + } + return lifecycleResultPayload(t, result) } - if result == nil || !result.IsError { - t.Fatalf("pending impact result = %#v, want MCP error", result) + + // A pending edit to the symbol's own file is labelled, never refused. + payload := impact() + if payload["graph_freshness"] != "pending" || payload["complete"] != false { + t.Fatalf("pending impact = %v, want a pending partial", payload) } - for _, want := range []string{"change impact refused a stale graph", "receipt-impact", "generation=11"} { - if !strings.Contains(freshnessToolResultText(result), want) { - t.Fatalf("pending impact response %q does not contain %q", freshnessToolResultText(result), want) - } + if files, _ := payload["pending_files"].([]any); len(files) != 1 || files[0] != "/repo-a/target.go" { + t.Fatalf("pending impact files = %v", payload["pending_files"]) } completeFreshnessReceipt(receipt, indexer.MutationResult{ @@ -476,22 +479,31 @@ func TestChangeImpactFreshnessGuardAndRetry(t *testing.T) { AppliedGeneration: 11, Reindexed: true, }) - result, err = s.handleEnhancedChangeImpact(context.Background(), req) - if err != nil { - t.Fatalf("completed impact handler error: %v", err) + if payload = impact(); payload["graph_freshness"] != nil { + t.Fatalf("published edit still labelled: %v", payload) + } + + // A terminally failed edit in scope is a labelled partial with a + // recovery hint, not a tool error. + failed := pendingFreshnessReceipt(s, "receipt-failed", "repo-a", "/repo-a/target.go", 12) + completeFreshnessReceipt(failed, indexer.MutationResult{ + RequestedGeneration: 12, + AppliedGeneration: 12, + Err: errors.New("syntax patch failed"), + }) + payload = impact() + if payload["graph_freshness"] != "failed" || payload["complete"] != false { + t.Fatalf("failed impact = %v, want a failed partial", payload) } - if result == nil || result.IsError { - t.Fatalf("completed impact remained blocked: %q", freshnessToolResultText(result)) + if hint, _ := payload["recovery_hint"].(string); !strings.Contains(hint, "reindex") { + t.Fatalf("failed impact recovery hint = %q", hint) } } -func TestDetectChangesFreshnessGuard(t *testing.T) { - s := &Server{ - graph: graph.New(), - session: newSessionState(), - mutationSafetyWait: time.Millisecond, - } - pendingFreshnessReceipt(s, "receipt-detect", "repo-a", "/repo-a/changed.go", 12) +func TestDetectChangesLabelsPendingChangedFile(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-detect", "", + writeUnindexed(t, root, "caller.go", "package p\n\nfunc Caller() {\n\tTarget()\n\tTarget()\n}\n"), 12) req := mcpgo.CallToolRequest{Params: mcpgo.CallToolParams{ Name: "detect_changes", Arguments: map[string]any{"format": "json"}, @@ -501,12 +513,37 @@ func TestDetectChangesFreshnessGuard(t *testing.T) { if err != nil { t.Fatalf("detect handler error: %v", err) } - if result == nil || !result.IsError { - t.Fatalf("pending detect result = %#v, want MCP error", result) + payload := lifecycleResultPayload(t, result) + for key, want := range map[string]any{ + "complete": false, + "graph_freshness": "pending", + "pending_file_count": float64(1), + } { + if payload[key] != want { + t.Fatalf("detect %s = %v, want %v: %v", key, payload[key], want, payload) + } + } + if files, _ := payload["changed_files"].([]any); len(files) != 1 || files[0] != "caller.go" { + t.Fatalf("detect changed_files = %v", payload["changed_files"]) } - for _, want := range []string{"change detection refused a stale graph", "receipt-detect", "generation=12"} { - if !strings.Contains(freshnessToolResultText(result), want) { - t.Fatalf("pending detect response %q does not contain %q", freshnessToolResultText(result), want) + // The walk still ran: the changed symbol is mapped and risk is scored. + if symbols, _ := payload["changed_symbols"].([]any); len(symbols) == 0 || payload["risk"] == "UNKNOWN" { + t.Fatalf("labelled detect skipped the walk: %v", payload) + } + + // The Stop hook bounds the wait with an earlier wait_deadline, which can + // pass between admission and the handler reading it. Either way the + // answer is labelled without sitting out the default budget. + for _, offset := range []time.Duration{50 * time.Millisecond, -time.Second} { + req.Params.Arguments = map[string]any{"format": "json", + "wait_deadline": time.Now().Add(offset).Format(time.RFC3339Nano)} + result, err = s.handleDetectChanges(context.Background(), req) + if err != nil { + t.Fatalf("detect handler error: %v", err) + } + payload = lifecycleResultPayload(t, result) + if waited, _ := payload["barrier_wait_ms"].(float64); payload["graph_freshness"] != "pending" || waited >= 200 { + t.Fatalf("detect with wait_deadline %v: graph_freshness %v after %v ms", offset, payload["graph_freshness"], waited) } } } diff --git a/internal/mcp/relation_freshness.go b/internal/mcp/relation_freshness.go new file mode 100644 index 000000000..024c687f0 --- /dev/null +++ b/internal/mcp/relation_freshness.go @@ -0,0 +1,258 @@ +package mcp + +import ( + "bytes" + "context" + "fmt" + "os" + "sort" + "strings" + "time" + + wire "github.com/gortexhq/gcx-go" + "github.com/mark3labs/mcp-go/mcp" + + "github.com/zzet/gortex/internal/graph" +) + +// relationFreshnessWait bounds how long a change tool waits for the edits in +// its relation scope before it answers with a label instead. +const relationFreshnessWait = 250 * time.Millisecond + +// maxLabelledPendingFiles caps the pending_files list of a label; +// pending_file_count always carries the full count. +const maxLabelledPendingFiles = 16 + +// relationFreshness labels an answer read from graph relations while edits in +// its scope are still being published. The zero value is a fresh scope. +type relationFreshness struct { + state string // "", "pending" or "failed" + files []string // graph-keyed paths of the lagging in-scope edits, sorted + waited time.Duration +} + +// relationWaitDeadline is when a change tool stops waiting for its relation +// scope: relationFreshnessWait from now, or an earlier wait_deadline. With +// require_fresh it is the request's own freshness deadline instead. The +// middleware has already refused a malformed or past wait_deadline, so one +// the handler cannot take passed after admission and ends the wait now. +func relationWaitDeadline(ctx context.Context, freshness requestFreshness) time.Time { + now := time.Now() + if freshness.err != nil { + return now + } + if freshness.requireFresh { + return freshness.effectiveDeadline(now, ctx) + } + deadline := now.Add(relationFreshnessWait) + if freshness.hasDeadline && freshness.deadline.Before(deadline) { + return freshness.deadline + } + return deadline +} + +// awaitRelationFreshness waits until deadline for the edits in the relation +// scope of ids and files, then labels whatever in that scope is still pending +// or failed. A deadline already passed labels without waiting. Edits outside +// the scope are neither waited on nor reported. +func (s *Server) awaitRelationFreshness(ctx context.Context, ids, files []string, deadline time.Time) relationFreshness { + started := time.Now() + lagging := s.laggingMutationReceipts(ctx) + if len(lagging) == 0 { + return relationFreshness{waited: time.Since(started)} + } + inScope := s.relationScope(ctx, ids, files) + if remaining := time.Until(deadline); remaining > 0 { + timer := time.NewTimer(remaining) + defer timer.Stop() + wait: + for _, receipt := range lagging { + if !inScope(receipt.path) { + continue + } + select { + case <-receipt.done: + case <-timer.C: + break wait + case <-ctx.Done(): + break wait + } + } + lagging = s.laggingMutationReceipts(ctx) + } + + states := make(map[string]string) + for _, receipt := range lagging { + if !inScope(receipt.path) { + continue + } + key := s.repoRelative(receipt.path) + if receipt.outcome(true).Pending { + states[key] = "pending" + } else if states[key] == "" { + states[key] = "failed" + } + } + label := relationFreshness{waited: time.Since(started)} + for path, state := range states { + label.files = append(label.files, path) + if label.state != "failed" { + label.state = state + } + } + sort.Strings(label.files) + return label +} + +// laggingMutationReceipts returns the request's mutation receipts whose edit +// the graph has not published: still pending, or terminally failed. +func (s *Server) laggingMutationReceipts(ctx context.Context) []*mutationReceipt { + var lagging []*mutationReceipt + for _, receipt := range s.mutationReceiptsForRepos(ctx) { + if outcome := receipt.outcome(true); outcome.Pending || outcome.Err != nil || !outcome.Reindexed { + lagging = append(lagging, receipt) + } + } + return lagging +} + +// relationScope reports whether an edit to a file can change the relations +// of ids: the file defines one of them, holds an edge into one of them, is one +// of files (graph-keyed), or its current bytes name one of them as a whole +// identifier, which is how a call added by an edit the graph has not read yet +// shows up. An id that does not resolve contributes the file and name its +// spelling carries, so it narrows the scope instead of widening it. +func (s *Server) relationScope(ctx context.Context, ids, files []string) func(absPath string) bool { + scoped := make(map[string]bool, len(files)+len(ids)) + for _, file := range files { + scoped[file] = true + } + var names []string + reader := s.readerFor(ctx) + for _, id := range ids { + id = strings.TrimSpace(id) + if id == "" { + continue + } + node := reader.GetNode(id) + if node == nil { + if file := graph.IDFile(id); file != "" { + scoped[file] = true + } + names = append(names, identifierName(id[strings.LastIndex(id, ":")+1:])) + continue + } + scoped[node.FilePath] = true + names = append(names, identifierName(node.Name)) + for _, edge := range reader.GetInEdges(id) { + file := edge.FilePath + if file == "" { + file = graph.IDFile(edge.From) + } + if file != "" { + scoped[file] = true + } + } + } + named := make(map[string]bool) + return func(absPath string) bool { + if scoped[s.repoRelative(absPath)] { + return true + } + hit, seen := named[absPath] + if !seen && len(names) > 0 { + data, err := os.ReadFile(absPath) + hit = err == nil && namesAnyIdentifier(data, names) + named[absPath] = hit + } + return hit + } +} + +// identifierName is the last identifier of a possibly qualified name, the +// spelling a call site uses: Recv.Method is called as Method. +func identifierName(name string) string { + return name[strings.LastIndex(name, ".")+1:] +} + +// namesAnyIdentifier reports whether data holds one of names as a whole +// identifier rather than as part of a longer one. +func namesAnyIdentifier(data []byte, names []string) bool { + for _, name := range names { + if name == "" { + continue + } + needle := []byte(name) + for offset := 0; offset < len(data); { + i := bytes.Index(data[offset:], needle) + if i < 0 { + break + } + start, end := offset+i, offset+i+len(needle) + if (start == 0 || !isIdentByte(data[start-1])) && (end == len(data) || !isIdentByte(data[end])) { + return true + } + offset = start + 1 + } + } + return false +} + +// apply writes the label into an answer's fields. A fresh scope writes +// nothing, so an answer with nothing lagging keeps its shape. +func (f relationFreshness) apply(result map[string]any) { + if f.state == "" { + return + } + result["complete"] = false + result["graph_freshness"] = f.state + result["freshness_scope"] = "relations" + result["pending_file_count"] = len(f.files) + result["pending_files"] = f.files[:min(maxLabelledPendingFiles, len(f.files))] + if f.state == "failed" { + result["recovery_hint"] = mutationFreshnessRecoveryHint() + } +} + +// line is the relations line a text answer leads with. +func (f relationFreshness) line() string { + return fmt.Sprintf("relations: %s (%d files)", f.state, len(f.files)) +} + +// requireFreshError is the answer to a require_fresh call whose wait ended +// with relations in its scope still lagging. +func (f relationFreshness) requireFreshError() *mcp.CallToolResult { + message := fmt.Sprintf("require_fresh: %s after waiting %d ms: %s", + f.line(), f.waited.Milliseconds(), strings.Join(f.files[:min(maxLabelledPendingFiles, len(f.files))], ", ")) + if f.state == "failed" { + message += "; " + mutationFreshnessRecoveryHint() + } + return mcp.NewToolResultError(message) +} + +// decorate makes a rendered text answer lead with its label: TOON and plain +// text start with the relations line; GCX keeps GCX1 as line 1, carries the +// label as header meta and the relations line as a comment on line 2. JSON +// already carries the label in its fields and passes through unchanged. +func (f relationFreshness) decorate(res *mcp.CallToolResult) *mcp.CallToolResult { + if f.state == "" || res == nil || res.IsError || len(res.Content) == 0 { + return res + } + text, ok := res.Content[0].(mcp.TextContent) + if !ok || strings.HasPrefix(strings.TrimSpace(text.Text), "{") { + return res + } + body := text.Text + if strings.HasPrefix(body, wire.Tag) { + body, _ = injectGCXHeaderMeta(body, map[string]any{ + "graph_freshness": f.state, + "freshness_scope": "relations", + "pending_file_count": len(f.files), + }) + head, rows, _ := strings.Cut(body, wire.RowSep) + body = head + wire.RowSep + wire.CommentPrefix + " " + f.line() + wire.RowSep + rows + } else { + body = f.line() + "\n" + body + } + return rebuildTextResult(res, body) +} diff --git a/internal/mcp/relation_freshness_test.go b/internal/mcp/relation_freshness_test.go new file mode 100644 index 000000000..4eef99956 --- /dev/null +++ b/internal/mcp/relation_freshness_test.go @@ -0,0 +1,194 @@ +package mcp + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + mcpgo "github.com/mark3labs/mcp-go/mcp" + "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/query" +) + +// newRelationFixture indexes a committed git repository in which Target +// (target.go) is called from caller.go, beside the unrelated other.go and +// namer.go. It returns the server and the indexed root receipts are keyed by. +func newRelationFixture(t *testing.T) (*Server, string) { + t.Helper() + dir := t.TempDir() + for name, src := range map[string]string{ + "target.go": "package p\n\nfunc Target() {}\n", + "caller.go": "package p\n\nfunc Caller() {\n\tTarget()\n}\n", + "other.go": "package p\n\nfunc Other() {}\n", + "namer.go": "package p\n\nfunc Namer() {}\n", + } { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(src), 0o644)) + } + checkoutMutationGit(t, dir, "init", "-q") + checkoutMutationGit(t, dir, "add", ".") + checkoutMutationGit(t, dir, "commit", "-q", "-m", "base") + + g := graph.New() + idx := indexer.New(g, testRegistry(), config.Default().Index, zap.NewNop()) + _, err := idx.Index(dir) + require.NoError(t, err) + return NewServer(query.NewEngine(g), g, idx, nil, zap.NewNop(), nil), idx.RootPath() +} + +// writeUnindexed changes a file on disk without telling the graph, the state +// a pending edit leaves behind. +func writeUnindexed(t *testing.T, root, name, src string) string { + t.Helper() + path := filepath.Join(root, name) + require.NoError(t, os.WriteFile(path, []byte(src), 0o644)) + return path +} + +func targetID(t *testing.T, s *Server) string { + t.Helper() + for _, n := range s.graph.AllNodes() { + if n.Name == "Target" && n.Kind == graph.KindFunction { + return n.ID + } + } + t.Fatal("Target is not indexed") + return "" +} + +func callChangeTool(t *testing.T, handler func(context.Context, mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error), name string, args map[string]any) *mcpgo.CallToolResult { + t.Helper() + req := mcpgo.CallToolRequest{Params: mcpgo.CallToolParams{Name: name, Arguments: args}} + result, err := handler(context.Background(), req) + require.NoError(t, err) + require.NotNil(t, result) + return result +} + +func TestChangeToolsIgnorePendingReceiptOutsideScope(t *testing.T) { + s, root := newRelationFixture(t) + writeUnindexed(t, root, "caller.go", "package p\n\nfunc Caller() {\n\tTarget()\n\tTarget()\n}\n") + pendingFreshnessReceipt(s, "receipt-other", "", filepath.Join(root, "other.go"), 1) + + impact := lifecycleResultPayload(t, callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": targetID(t, s), "format": "json"})) + detect := lifecycleResultPayload(t, callChangeTool(t, s.handleDetectChanges, "detect_changes", + map[string]any{"format": "json"})) + for tool, payload := range map[string]map[string]any{"impact": impact, "detect": detect} { + require.NotContains(t, payload, "graph_freshness", "%s labelled an edit outside its scope: %v", tool, payload) + require.Less(t, payload["barrier_wait_ms"], float64(100), "%s waited on an edit outside its scope", tool) + } + require.Equal(t, []any{"caller.go"}, detect["changed_files"]) + require.NotEmpty(t, detect["changed_symbols"]) +} + +func TestChangeToolsScopeIncludesFilesNamingTheSymbol(t *testing.T) { + s, root := newRelationFixture(t) + // namer.go now calls Target, but the graph has no edge for it yet; + // other.go names only a longer identifier. + pendingFreshnessReceipt(s, "receipt-namer", "", + writeUnindexed(t, root, "namer.go", "package p\n\nfunc Namer() {\n\tTarget()\n}\n"), 1) + pendingFreshnessReceipt(s, "receipt-other", "", + writeUnindexed(t, root, "other.go", "package p\n\nfunc Other() {\n\tTargetX()\n}\n"), 2) + + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": targetID(t, s), "format": "json"})) + require.Equal(t, "pending", payload["graph_freshness"]) + require.Equal(t, "relations", payload["freshness_scope"]) + require.Equal(t, false, payload["complete"]) + require.Equal(t, []any{"namer.go"}, payload["pending_files"]) + require.Equal(t, float64(1), payload["pending_file_count"]) + require.Less(t, payload["barrier_wait_ms"], float64(500)) +} + +func TestChangeToolsUnresolvedIDDoesNotWidenBarrier(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-other", "", filepath.Join(root, "other.go"), 1) + pendingFreshnessReceipt(s, "receipt-fresh", "", + writeUnindexed(t, root, "fresh.go", "package p\n\nfunc Fresh() {}\n"), 2) + + // Defined by a pending edit the graph has not read: labelled, and bounded + // by the budget rather than by every receipt in the repository. + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": "fresh.go::Fresh", "format": "json"})) + require.Equal(t, "pending", payload["graph_freshness"]) + require.Equal(t, []any{"fresh.go"}, payload["pending_files"]) + require.Less(t, payload["barrier_wait_ms"], float64(500)) + + // No pending edit relates to it: answered at once, unlabelled. + payload = lifecycleResultPayload(t, callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": "gone.go::Gone", "format": "json"})) + require.NotContains(t, payload, "graph_freshness") + require.Less(t, payload["barrier_wait_ms"], float64(100)) +} + +func TestChangeToolsRequireFreshWaitsUntilWaitDeadline(t *testing.T) { + s, root := newRelationFixture(t) + target := pendingFreshnessReceipt(s, "receipt-target", "", filepath.Join(root, "target.go"), 1) + caller := pendingFreshnessReceipt(s, "receipt-caller", "", + writeUnindexed(t, root, "caller.go", "package p\n\nfunc Caller() {\n\tTarget()\n\tTarget()\n}\n"), 2) + deadline := func(d time.Duration) string { return time.Now().Add(d).Format(time.RFC3339Nano) } + + // The receipt never lands: the wait runs past the default budget to the + // caller's deadline, then refuses. + started := time.Now() + result := callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": targetID(t, s), "require_fresh": true, "wait_deadline": deadline(700 * time.Millisecond)}) + require.True(t, result.IsError) + require.GreaterOrEqual(t, time.Since(started), 600*time.Millisecond) + require.Contains(t, freshnessToolResultText(result), "relations: pending (2 files)") + require.Contains(t, freshnessToolResultText(result), "target.go") + + result = callChangeTool(t, s.handleDetectChanges, "detect_changes", + map[string]any{"require_fresh": true, "wait_deadline": deadline(500 * time.Millisecond)}) + require.True(t, result.IsError) + require.Contains(t, freshnessToolResultText(result), "caller.go") + + // The receipts land after the default budget but before the deadline: + // the answer waits for them and carries no label. + go func() { + time.Sleep(400 * time.Millisecond) + completeFreshnessReceipt(target, indexer.MutationResult{RequestedGeneration: 1, AppliedGeneration: 1, Reindexed: true}) + completeFreshnessReceipt(caller, indexer.MutationResult{RequestedGeneration: 2, AppliedGeneration: 2, Reindexed: true}) + }() + started = time.Now() + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": targetID(t, s), "format": "json", "require_fresh": true, "wait_deadline": deadline(5 * time.Second)})) + require.GreaterOrEqual(t, time.Since(started), 350*time.Millisecond) + require.NotContains(t, payload, "graph_freshness") +} + +func TestChangeAnswersStartWithRelationsLine(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-caller", "", + writeUnindexed(t, root, "caller.go", "package p\n\nfunc Caller() {\n\tTarget()\n\tTarget()\n}\n"), 1) + const line = "relations: pending (1 files)" + id := targetID(t, s) + + for _, format := range []string{"toon", "gcx"} { + impact := freshnessToolResultText(callChangeTool(t, s.handleEnhancedChangeImpact, "explain_change_impact", + map[string]any{"ids": id, "format": format})) + lines := strings.SplitN(impact, "\n", 3) + require.GreaterOrEqual(t, len(lines), 2, impact) + if format == "gcx" { + require.True(t, strings.HasPrefix(lines[0], "GCX1 "), impact) + require.Contains(t, lines[0], " graph_freshness=pending") + require.Contains(t, lines[0], " pending_file_count=1") + require.Contains(t, lines[0], " barrier_wait_ms=") + require.Equal(t, "# "+line, lines[1]) + } else { + require.Equal(t, line, lines[0], impact) + } + } + + detect := freshnessToolResultText(callChangeTool(t, s.handleDetectChanges, "detect_changes", + map[string]any{"format": "toon"})) + require.Equal(t, line, strings.SplitN(detect, "\n", 2)[0], detect) +} diff --git a/internal/mcp/tools_analysis.go b/internal/mcp/tools_analysis.go index feae760a1..942467169 100644 --- a/internal/mcp/tools_analysis.go +++ b/internal/mcp/tools_analysis.go @@ -352,12 +352,39 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques if scopeErr := s.repoPrefixInSessionScope(ctx, repoPrefix, repoPrefix); scopeErr != nil { return mcp.NewToolResultError(scopeErr.Error()), nil } + checkoutScoped := control != nil && control.CheckoutScoped var pending bool var freshnessErr error - if control != nil && control.CheckoutScoped { + var label relationFreshness + var diff *analysis.DiffResult + if checkoutScoped { pending, freshnessErr = s.mutationFreshnessSummaryForRepos(ctx, repoPrefix) - } else if err := s.awaitMutationFreshnessForRepos(ctx, repoPrefix); err != nil { - return mcp.NewToolResultError("change detection refused a stale graph: " + err.Error()), nil + } else { + // Diff the files first: they are the relation scope the wait below + // is bounded to, so an edit elsewhere in the repository neither + // delays nor labels this answer. + files, err := analysis.MapGitDiffContext(ctx, nil, repoRoot, repoPrefix, scope, baseRef) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + keys := make([]string, len(files.ChangedFiles)) + for i, file := range files.ChangedFiles { + keys[i] = analysis.GraphKey(repoPrefix, file, analysis.RepoRelativePath) + } + freshness := takeRequestFreshness(&req) + label = s.awaitRelationFreshness(ctx, nil, keys, relationWaitDeadline(ctx, freshness)) + if freshness.requireFresh && label.state != "" { + return label.requireFreshError(), nil + } + diff = files + } + respond := func(result map[string]any) (*mcp.CallToolResult, error) { + if !checkoutScoped { + result["barrier_wait_ms"] = label.waited.Milliseconds() + label.apply(result) + } + res, err := s.respondJSONOrTOON(ctx, req, result) + return label.decorate(res), err } reader := s.readerFor(ctx) graphStatus, graphDetail := "ready", "" @@ -381,7 +408,12 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques reader = nil } - diff, err := analysis.MapGitDiffContext(ctx, reader, repoRoot, repoPrefix, scope, baseRef) + var err error + if diff == nil { + diff, err = analysis.MapGitDiffContext(ctx, reader, repoRoot, repoPrefix, scope, baseRef) + } else { + diff, err = analysis.JoinDiffSymbolsContext(ctx, reader, repoPrefix, diff) + } if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -398,7 +430,7 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques fileChanges = []analysis.FileChange{} } if graphStatus != "ready" { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "changed_symbols": []any{}, "changed_files": changedFiles, "file_changes": fileChanges, "risk": "UNKNOWN", "complete": false, "graph_status": graphStatus, "summary": "Git file changes are available; symbol mapping and impact await an exact published graph", @@ -407,7 +439,7 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques } if len(diff.ChangedSymbols) == 0 { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "changed_symbols": []any{}, "changed_files": changedFiles, "file_changes": fileChanges, @@ -459,7 +491,7 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques req.GetBool("summary_only", false), req.GetInt("offset", 0), req.GetInt("limit", 100)) - return s.respondJSONOrTOON(ctx, req, detectResult) + return respond(detectResult) } // tryImpactAnalysisSnapshots reads optional cached enrichments without waiting @@ -487,8 +519,12 @@ func (s *Server) handleEnhancedChangeImpact(ctx context.Context, req mcp.CallToo ids[i] = strings.TrimSpace(ids[i]) } - if freshnessErr := s.awaitMutationFreshnessForRepos(ctx, s.mutationReposForSymbolIDs(ctx, ids)...); freshnessErr != nil { - return mcp.NewToolResultError("change impact refused a stale graph: " + freshnessErr.Error()), nil + // Wait briefly for the edits these symbols' relations depend on, then + // answer; whatever is still lagging is labelled, not refused. + freshness := takeRequestFreshness(&req) + label := s.awaitRelationFreshness(ctx, ids, nil, relationWaitDeadline(ctx, freshness)) + if freshness.requireFresh && label.state != "" { + return label.requireFreshError(), nil } // Keep the mandatory pre-edit safety gate well below host transport @@ -511,7 +547,9 @@ func (s *Server) handleEnhancedChangeImpact(ctx context.Context, req mcp.CallToo "test_files": impact.TestFiles, "total_affected": impact.TotalAffected, "cross_repo_impact": impact.CrossRepoImpact, + "barrier_wait_ms": label.waited.Milliseconds(), } + label.apply(result) // GNX-3: by_depth_counts is the headline; the heavy by_depth rows are // paged (offset / limit) or dropped (summary_only) so the agent gets the @@ -605,18 +643,20 @@ func (s *Server) handleEnhancedChangeImpact(ctx context.Context, req mcp.CallToo } } - if s.isGCX(ctx, req) { + var res *mcp.CallToolResult + switch { + case s.isGCX(ctx, req): // encodeChangeImpact reads the same map shape we'd return as // JSON; routing through it keeps a single source of truth for // field names and avoids divergence on the next analyzer // addition. - return s.gcxResponseWithBudget(req)(encodeChangeImpact(result)) + res, err = s.gcxResponseWithBudget(req)(encodeChangeImpact(result)) + case s.isTOON(ctx, req): + res, err = returnTOON(result) + default: + res, err = s.respondJSONOrTOON(ctx, req, result) } - if s.isTOON(ctx, req) { - return returnTOON(result) - } - - return s.respondJSONOrTOON(ctx, req, result) + return label.decorate(res), err } func impactComplete(impact *analysis.ImpactResult) bool { From 408aafbb89b401c17cb37b03847baab04137db81 Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sat, 31 Oct 2026 17:26:23 +0100 Subject: [PATCH 2/6] hooks: bound the briefing's detect_changes call and report lagging relations The Stop briefing and the Codex PostToolUse briefing call detect_changes with only the 5 s transport bound, and fold every failure into an empty string. While edits were pending the change tool refused, the hook could not decode the error, and the agent was told nothing about its change set. detect_changes now gets 300 ms through callServerToolWithin, which tells a reply that did not arrive in time apart from an unreachable bridge. The call carries a wait_deadline 150 ms out, so the daemon labels the edits it is still publishing instead of waiting 250 ms for them on top of the diff and the walk, which would land the labelled answer past the budget. An empty reply after that deadline, which is how the daemon refuses a deadline that passed before it took the call up, counts as a stall. A stall, or an answer labelled pending or failed (graph_freshness on a primary checkout, graph_status on a worktree), yields a short briefing with the relations line and the changed files instead of silence or diagnostics over lagging relations. A stall says only that detect_changes did not answer, and stays silent for a session that recorded no writes, so a slow daemon does not put a warning into every read-only turn. renderDeadCodeHits never reports a symbol listed on a "pending_unknown " line of analyze dead_code as dead. --- internal/hooks/posttask.go | 132 ++++++++++++++++++++++++++++---- internal/hooks/posttask_test.go | 111 +++++++++++++++++++++++++++ internal/hooks/precompact.go | 17 ++++ 3 files changed, 244 insertions(+), 16 deletions(-) diff --git a/internal/hooks/posttask.go b/internal/hooks/posttask.go index f8494cb04..c3ab29170 100644 --- a/internal/hooks/posttask.go +++ b/internal/hooks/posttask.go @@ -133,25 +133,106 @@ type changeSet struct { Scope string `json:"scope"` Repo string `json:"repo"` RepoRoot string `json:"repo_root"` + // The label of an answer whose relations lag: graph_freshness on a + // primary checkout, graph_status on a worktree. + GraphFreshness string `json:"graph_freshness"` + GraphStatus string `json:"graph_status"` + PendingFileCount int `json:"pending_file_count"` } +// detectChangesBudget bounds the detect_changes round trip of a briefing. A +// daemon still publishing edits answers inside it with a labelled partial; a +// stalled one is reported rather than waited on. +const detectChangesBudget = 300 * time.Millisecond + +// detectChangesWait is how far into the budget the briefing's wait_deadline +// lets the daemon wait for pending edits before it labels them. The rest of +// the budget is for the diff walk and the transport. +const detectChangesWait = detectChangesBudget / 2 + // detectChangeSet asks the bridge for the current diff and decodes it. -// Returns nil when the bridge is unreachable or the payload is unusable. -func detectChangeSet(port int) *changeSet { - raw := callServerTool(port, "detect_changes", map[string]any{ +// answered is false when the bridge did not reply within detectChangesBudget, +// or replied with nothing once detectChangesWait had passed; changes is nil +// when it is unreachable or the payload is unusable. +func detectChangeSet(port int) (changes *changeSet, answered bool) { + started := time.Now() + raw, answered := callServerToolWithin(port, "detect_changes", map[string]any{ "scope": postTaskDiffScope, // The briefings never read by_depth; summary_only drops the // largest part of the response. - "summary_only": true, - }) + "summary_only": true, + "wait_deadline": started.Add(detectChangesWait).UTC().Format(time.RFC3339Nano), + }, detectChangesBudget) if raw == "" { - return nil + // The daemon refuses a wait_deadline that passed before it took + // the call up: a busy daemon, not an unreachable one. + return nil, answered && time.Since(started) < detectChangesWait } - var changes changeSet if err := json.Unmarshal([]byte(raw), &changes); err != nil { - return nil + return nil, true } - return &changes + return changes, true +} + +// relationsLine is the label line of a change set whose relations lag, or "" +// when the graph has published every change it reports. +func (c *changeSet) relationsLine() string { + state := c.GraphFreshness + if state == "" && c.GraphStatus != "" && c.GraphStatus != "ready" { + state = c.GraphStatus + } + if state == "" { + return "" + } + files := c.PendingFileCount + if files == 0 { + files = len(c.ChangedFiles) + } + return fmt.Sprintf("relations: %s (%d files)", state, files) +} + +// laggingChangesBriefing is the briefing for a change set the graph has not +// caught up with: the relations line and the changed files, with the +// symbol-level diagnostics left to a turn that can trust them. A stalled +// detect_changes (nil changes) is reported the same way instead of as +// silence, except to a session that recorded no writes: it has no change set +// to warn about. lagging is false when the change set is complete. +func laggingChangesBriefing(heading string, scope sessionScope, changes *changeSet) (briefing string, lagging bool) { + line := fmt.Sprintf("relations: pending (detect_changes did not answer within %v)", detectChangesBudget) + why := "" + var files []string + if changes == nil { + if scope.SessionID != "" && len(loadSessionState(scope.SessionID).WrittenPaths) == 0 { + return "", true + } + } else { + if line = changes.relationsLine(); line == "" { + return "", false + } + why = " the graph is still publishing edits, so" + files = changes.ChangedFiles + if attr := attributeChanges(scope, changes); attr.Attributed { + if len(attr.OwnedFiles) == 0 { + return "", true + } + files = attr.OwnedFiles + } + } + var sb strings.Builder + sb.WriteString(heading + "\n\n") + sb.WriteString(line + " —" + why + " symbol-level diagnostics are skipped; run detect_changes again before handoff.\n") + if len(files) > 0 { + sb.WriteString("\nChanged files:\n") + const show = 10 + for i, f := range files { + if i == show { + fmt.Fprintf(&sb, "- (+%d more)\n", len(files)-show) + break + } + fmt.Fprintf(&sb, "- `%s`\n", f) + } + } + return sb.String(), true } // briefingScope names the working tree detect_changes actually diffed. @@ -245,10 +326,13 @@ func renderUnattributedFiles(files []string, capped bool) string { // named rather than dropped. When the session cannot be identified at all the // briefing falls back to the whole tree and labels itself as such. func buildPostTaskBriefing(port int, scope sessionScope) string { - changes := detectChangeSet(port) - if changes == nil { + changes, answered := detectChangeSet(port) + if answered && changes == nil { return "" } + if briefing, lagging := laggingChangesBriefing("## Gortex Working-Tree Diagnostics", scope, changes); lagging { + return briefing + } if len(changes.ChangedSymbols) == 0 { // No indexed symbols touched — skip silently. The agent doesn't need @@ -365,8 +449,14 @@ func buildPostTaskBriefing(port int, scope sessionScope) string { // so nothing is ever recorded for it and it always resolves to the labeled // whole-tree fallback — this does not give Codex attribution coverage. func buildMutationBriefing(port int, scope sessionScope) string { - changes := detectChangeSet(port) - if changes == nil || len(changes.ChangedSymbols) == 0 { + changes, answered := detectChangeSet(port) + if answered && changes == nil { + return "" + } + if briefing, lagging := laggingChangesBriefing("## Gortex mutation follow-up", scope, changes); lagging { + return briefing + } + if len(changes.ChangedSymbols) == 0 { return "" } @@ -461,7 +551,9 @@ func renderGuardViolations(port int, idsCSV string) string { // renderDeadCodeHits filters analyze:dead_code results to the intersection // with the currently-changed symbols (i.e. "did this task leave anything -// orphaned"). Emits nothing when the intersection is empty. +// orphaned"). Emits nothing when the intersection is empty. A symbol listed +// on a "pending_unknown " line has unknown liveness while edits are pending +// and is never reported as dead. func renderDeadCodeHits(port int, ids []string) string { raw := callServerTool(port, "analyze", map[string]any{ "kind": "dead_code", @@ -472,14 +564,22 @@ func renderDeadCodeHits(port int, ids []string) string { return "" } + lines := strings.Split(raw, "\n") idSet := make(map[string]bool, len(ids)) for _, id := range ids { idSet[id] = true } + for _, line := range lines { + if rest, unknown := strings.CutPrefix(line, "pending_unknown "); unknown { + for _, field := range strings.Fields(rest) { + delete(idSet, field) + } + } + } var hits []string - for _, line := range strings.Split(raw, "\n") { - if line == "" { + for _, line := range lines { + if line == "" || strings.HasPrefix(line, "pending_unknown ") { continue } // Each compact line starts with the symbol descriptor including the diff --git a/internal/hooks/posttask_test.go b/internal/hooks/posttask_test.go index b55f14b90..2dda76944 100644 --- a/internal/hooks/posttask_test.go +++ b/internal/hooks/posttask_test.go @@ -3,9 +3,12 @@ package hooks import ( "encoding/json" "fmt" + "net/http" + "net/http/httptest" "path/filepath" "strings" "testing" + "time" "github.com/zzet/gortex/internal/daemon" "github.com/zzet/gortex/internal/localizationauth" @@ -189,6 +192,114 @@ func TestRunPostTask_DeadCodeFiltersToChanged(t *testing.T) { if strings.Contains(out, "API Contract Issues") { t.Errorf("contract section should be omitted when clean:\n%s", out) } + + // A changed symbol whose liveness is unknown while edits are pending is + // never reported as dead. + pending := newFakeServer(map[string]string{ + "analyze": "relations: pending (1 files)\npending_unknown function foo.go::Foo foo.go:3\n", + }) + defer pending.Close() + if hits := renderDeadCodeHits(portFromURL(t, pending.URL), []string{"foo.go::Foo"}); hits != "" { + t.Errorf("pending_unknown symbol reported as dead:\n%s", hits) + } +} + +// While the graph is still publishing edits, detect_changes answers with a +// labelled partial. Both briefings must say so and name the changed files +// rather than render diagnostics over lagging relations or fall silent. +func TestPostTaskBriefingPrintsPendingRelations(t *testing.T) { + for name, detect := range map[string]string{ + "primary": `{"changed_files":["a.go","b.go"],"changed_symbols":[{"id":"a.go::A","name":"A","kind":"function"}], + "risk":"LOW","complete":false,"graph_freshness":"pending","pending_file_count":2,"pending_files":["a.go","b.go"]}`, + "worktree": `{"changed_files":["a.go"],"changed_symbols":[],"risk":"UNKNOWN","complete":false,"graph_status":"pending"}`, + } { + srv := newRecordingFakeServer(map[string]string{"detect_changes": detect, "get_test_targets": "a_test.go TestA\n"}) + port := portFromURL(t, srv.URL) + started := time.Now() + for builder, briefing := range map[string]string{ + "stop": buildPostTaskBriefing(port, sessionScope{}), + "mutation": buildMutationBriefing(port, sessionScope{}), + } { + want := "relations: pending (2 files)" + if name == "worktree" { + want = "relations: pending (1 files)" + } + if !strings.Contains(briefing, want) || !strings.Contains(briefing, "`a.go`") { + t.Errorf("%s %s briefing missing %q and the changed files:\n%s", name, builder, want, briefing) + } + } + if n := srv.callCount("get_test_targets"); n != 0 { + t.Errorf("%s: diagnostics ran over lagging relations (%d get_test_targets calls)", name, n) + } + // The daemon waits for pending edits until wait_deadline and labels + // the rest. Without one it waits past the budget, and the labelled + // answer arrives after the briefing has given up on it. + for _, args := range srv.argsFor("detect_changes") { + deadline, err := time.Parse(time.RFC3339Nano, fmt.Sprint(args["wait_deadline"])) + if err != nil || !deadline.After(started) || !deadline.Before(started.Add(detectChangesBudget)) { + t.Errorf("%s: detect_changes wait_deadline %v does not end inside the %v budget", name, args["wait_deadline"], detectChangesBudget) + } + } + srv.Close() + } +} + +// A detect_changes that does not answer within its budget is reported, and +// the briefing returns well inside the half-second hook budget. So is one the +// daemon reached only after its wait_deadline had passed, which it refuses. +// A session that recorded no writes is not told about a slow daemon. +func TestPostTaskBriefingBoundedWhenDetectStalls(t *testing.T) { + t.Setenv(hookSessionDirEnvVar, t.TempDir()) + saveSessionState("writer", sessionState{WrittenPaths: []string{"a.go"}}) + release := make(chan struct{}) + stalled := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-release: + case <-time.After(5 * time.Second): + } + http.NotFound(w, r) + })) + defer stalled.Close() + defer close(release) + late := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req struct { + Arguments map[string]any `json:"arguments"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + if deadline, err := time.Parse(time.RFC3339Nano, fmt.Sprint(req.Arguments["wait_deadline"])); err == nil { + time.Sleep(time.Until(deadline) + 10*time.Millisecond) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"content":[{"type":"text","text":"wait_deadline is not in the future"}],"isError":true}`)) + })) + defer late.Close() + + for name, srv := range map[string]*httptest.Server{"stalled": stalled, "refused late": late} { + port := portFromURL(t, srv.URL) + for builder, build := range map[string]func(int, sessionScope) string{ + "stop": buildPostTaskBriefing, + "mutation": buildMutationBriefing, + } { + for session, report := range map[string]bool{"": true, "writer": true, "reader": false} { + if name != "stalled" && session != "" { + continue + } + started := time.Now() + briefing := build(port, sessionScope{SessionID: session}) + if elapsed := time.Since(started); elapsed > 500*time.Millisecond { + t.Errorf("%s %s briefing took %v", name, builder, elapsed) + } + switch { + case !report && briefing != "": + t.Errorf("%s %s briefing warned session %q, which wrote nothing:\n%q", name, builder, session, briefing) + case report && !strings.Contains(briefing, "relations: pending"): + t.Errorf("%s %s briefing did not report the busy detect_changes to session %q:\n%q", name, builder, session, briefing) + case report && strings.Contains(briefing, "publishing"): + t.Errorf("%s %s briefing claims edits are publishing without an answer:\n%q", name, builder, briefing) + } + } + } + } } // contractsCompact builds a realistic `contracts check --compact` payload: diff --git a/internal/hooks/precompact.go b/internal/hooks/precompact.go index a086c2393..a7cc8ddd4 100644 --- a/internal/hooks/precompact.go +++ b/internal/hooks/precompact.go @@ -232,6 +232,23 @@ func callServerTool(port int, name string, args map[string]any) string { return callServerToolHTTP(port, name, args) } +// callServerToolWithin is callServerTool bounded by timeout. answered is false +// when the call did not return in time: the bridge is reachable but busy, +// which a briefing reports instead of folding it into the silence of an +// unreachable bridge. The abandoned call ends with the hook process. +func callServerToolWithin(port int, name string, args map[string]any, timeout time.Duration) (raw string, answered bool) { + done := make(chan string, 1) + go func() { done <- callServerTool(port, name, args) }() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case raw = <-done: + return raw, true + case <-timer.C: + return "", false + } +} + // callServerToolHTTP issues a POST /v1/tools/{name} against the server and // returns the text content from the first response block. Returns empty string // on any error (server unreachable, non-200, missing content). From 549b0bbee748e080ef1f34d0f1e1652fa2c5537f Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sat, 31 Oct 2026 19:08:39 +0100 Subject: [PATCH 3/6] mcp: label detect_changes for pending edits to the changed symbols' callers detect_changes scoped its wait and label to the diff's changed files, but its risk and blast radius come from walking the changed symbols' callers. With scope staged or compare the working tree is not in the diff, so a pending edit in a caller file left the walk reading the old relations without a label. After the symbol join it now also waits for, and labels, the relation scope of the changed symbols: their files, the files with edges into them and the files that name them. That wait ends at the same deadline as the changed files' wait, so the answer still waits at most once. --- internal/mcp/mutation_freshness_test.go | 17 +++++++++++++++++ internal/mcp/relation_freshness.go | 23 +++++++++++++++++++++++ internal/mcp/tools_analysis.go | 19 ++++++++++++++++--- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/internal/mcp/mutation_freshness_test.go b/internal/mcp/mutation_freshness_test.go index 230398750..011c5e5fa 100644 --- a/internal/mcp/mutation_freshness_test.go +++ b/internal/mcp/mutation_freshness_test.go @@ -546,4 +546,21 @@ func TestDetectChangesLabelsPendingChangedFile(t *testing.T) { t.Fatalf("detect with wait_deadline %v: graph_freshness %v after %v ms", offset, payload["graph_freshness"], waited) } } + + // A staged diff leaves the pending caller.go out of the changed files, + // but the walk still reads it as a caller of the changed Target. + writeUnindexed(t, root, "target.go", "package p\n\nfunc Target() { _ = 0 }\n") + checkoutMutationGit(t, root, "add", "target.go") + req.Params.Arguments = map[string]any{"format": "json", "scope": "staged"} + result, err = s.handleDetectChanges(context.Background(), req) + if err != nil { + t.Fatalf("detect handler error: %v", err) + } + payload = lifecycleResultPayload(t, result) + if files, _ := payload["changed_files"].([]any); len(files) != 1 || files[0] != "target.go" { + t.Fatalf("staged detect changed_files = %v", payload["changed_files"]) + } + if pending, _ := payload["pending_files"].([]any); payload["graph_freshness"] != "pending" || len(pending) != 1 || pending[0] != "caller.go" { + t.Fatalf("staged detect left the pending caller unlabelled: %v", payload) + } } diff --git a/internal/mcp/relation_freshness.go b/internal/mcp/relation_freshness.go index 024c687f0..1b489e247 100644 --- a/internal/mcp/relation_freshness.go +++ b/internal/mcp/relation_freshness.go @@ -230,6 +230,29 @@ func (f relationFreshness) requireFreshError() *mcp.CallToolResult { return mcp.NewToolResultError(message) } +// merge folds the label of another scope the same answer read into f: the +// files of both, failed over pending, and the time spent waiting on each. +func (f relationFreshness) merge(other relationFreshness) relationFreshness { + f.waited += other.waited + if other.state == "" { + return f + } + if f.state != "failed" { + f.state = other.state + } + seen := make(map[string]bool, len(f.files)) + for _, file := range f.files { + seen[file] = true + } + for _, file := range other.files { + if !seen[file] { + f.files = append(f.files, file) + } + } + sort.Strings(f.files) + return f +} + // decorate makes a rendered text answer lead with its label: TOON and plain // text start with the relations line; GCX keeps GCX1 as line 1, carries the // label as header meta and the relations line as a comment on line 2. JSON diff --git a/internal/mcp/tools_analysis.go b/internal/mcp/tools_analysis.go index 942467169..bec7687a4 100644 --- a/internal/mcp/tools_analysis.go +++ b/internal/mcp/tools_analysis.go @@ -357,6 +357,9 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques var freshnessErr error var label relationFreshness var diff *analysis.DiffResult + var freshness requestFreshness + var keys []string + var deadline time.Time if checkoutScoped { pending, freshnessErr = s.mutationFreshnessSummaryForRepos(ctx, repoPrefix) } else { @@ -367,12 +370,13 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques if err != nil { return mcp.NewToolResultError(err.Error()), nil } - keys := make([]string, len(files.ChangedFiles)) + keys = make([]string, len(files.ChangedFiles)) for i, file := range files.ChangedFiles { keys[i] = analysis.GraphKey(repoPrefix, file, analysis.RepoRelativePath) } - freshness := takeRequestFreshness(&req) - label = s.awaitRelationFreshness(ctx, nil, keys, relationWaitDeadline(ctx, freshness)) + freshness = takeRequestFreshness(&req) + deadline = relationWaitDeadline(ctx, freshness) + label = s.awaitRelationFreshness(ctx, nil, keys, deadline) if freshness.requireFresh && label.state != "" { return label.requireFreshError(), nil } @@ -464,6 +468,15 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques for i, cs := range diff.ChangedSymbols { symbolIDs[i] = cs.ID } + if !checkoutScoped { + // The walk reads the changed symbols' callers, and an edit outside + // the diff (a staged or compare scope leaves the working tree out) + // can be changing those. They share what is left of the budget. + label = label.merge(s.awaitRelationFreshness(ctx, symbolIDs, keys, deadline)) + if freshness.requireFresh && label.state != "" { + return label.requireFreshError(), nil + } + } impact, err := s.analyzeDiffImpact(ctx, symbolIDs) if err != nil { From c60eb826ea092c1c0db7e207748c33763f8005bb Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sat, 31 Oct 2026 17:29:40 +0100 Subject: [PATCH 4/6] mcp: label relation-reading mutators and block deletes on pending callers change_contract, preview_edit and safe_delete_symbol read callers and references from the graph and act on them or give a verdict, with no word about edits the graph has not published yet. Once edit acknowledgements stop waiting for the reindex, those windows become routine and the tools would answer from the old graph without saying so; safe_delete could delete a symbol whose only caller was added a moment ago in another file. Each tool now snapshots the unpublished edits before it walks, and once the walk has named the symbols involved labels the answer with the in-scope ones (graph_freshness, freshness_scope, pending_files, pending_file_count), leading TOON and plain text with the relations line. There is no default wait. safe_delete_symbol also scans the files of those edits for lines that name the target as a whole identifier and reports each one as a provisional reference, so the delete is rejected through the existing rejected_has_references status, and in propagate mode as a manual patch that blocks the propagation. An edit to the target's own file is one provisional reference for the whole file: it may add a caller there, and the graph's span of the target, which the delete cuts by, may no longer hold. A line the graph already holds a reference for is left to the graph, so it is neither counted twice nor turned into a manual patch. With cascade:"apply" every closure member gets the same scan, since the closure is orphaned by the graph's edges only, and a hit refuses the cascade. The scan stands in for the unresolved call stubs of a pending projection until those exist. preview_edit maps the touched files to graph keys before scoping the label, so a file named by an absolute URI still counts. --- internal/mcp/change_contract.go | 7 +- internal/mcp/relation_freshness.go | 101 ++++++++++++++++++- internal/mcp/relation_freshness_test.go | 127 ++++++++++++++++++++++++ internal/mcp/tools_safe_delete.go | 76 ++++++++++++-- internal/mcp/tools_simulate.go | 17 ++++ 5 files changed, 315 insertions(+), 13 deletions(-) diff --git a/internal/mcp/change_contract.go b/internal/mcp/change_contract.go index 81d47c99e..325397fba 100644 --- a/internal/mcp/change_contract.go +++ b/internal/mcp/change_contract.go @@ -897,6 +897,9 @@ func (s *Server) handleChangeContract(ctx context.Context, req mcp.CallToolReque if s == nil || s.graph == nil { return mcp.NewToolResultError("change_contract: server not fully initialised"), nil } + // Snapshot the unpublished edits before the walk; the changed symbols + // that scope the label are known only once it has run. + gaps := s.laggingMutationReceipts(ctx) p, err := s.lowerChange(ctx, req) if err != nil { if ctxErr := requestContextError(ctx, err); ctxErr != nil { @@ -909,5 +912,7 @@ func (s *Server) handleChangeContract(ctx context.Context, req mcp.CallToolReque } violations := s.evaluateChange(ctx, p) env := s.assembleEnvelope(ctx, p, violations) - return s.respondJSONOrTOON(ctx, req, env) + label := s.labelRelations(ctx, gaps, p.changedIDs, nil) + res, err := s.respondJSONOrTOON(ctx, req, label.labelled(env)) + return label.decorate(res), err } diff --git a/internal/mcp/relation_freshness.go b/internal/mcp/relation_freshness.go index 1b489e247..b586cf68b 100644 --- a/internal/mcp/relation_freshness.go +++ b/internal/mcp/relation_freshness.go @@ -3,6 +3,7 @@ package mcp import ( "bytes" "context" + "encoding/json" "fmt" "os" "sort" @@ -80,20 +81,41 @@ func (s *Server) awaitRelationFreshness(ctx context.Context, ids, files []string } lagging = s.laggingMutationReceipts(ctx) } + label := s.relationLabel(lagging, inScope) + label.waited = time.Since(started) + return label +} +// labelRelations labels an answer that read relations without waiting. gaps +// is the laggingMutationReceipts snapshot taken before the walk; the scope of +// ids and files, known only once the walk has run, filters it afterwards. An +// edit that published during the walk still labels the answer, which may +// have read around it. +func (s *Server) labelRelations(ctx context.Context, gaps []*mutationReceipt, ids, files []string) relationFreshness { + if len(gaps) == 0 { + return relationFreshness{} + } + return s.relationLabel(gaps, s.relationScope(ctx, ids, files)) +} + +// relationLabel labels the receipts that fall in scope: failed when the edit +// failed to publish, pending otherwise. One pending receipt for a path +// outweighs an older failed one, and any failed path makes the label failed. +func (s *Server) relationLabel(receipts []*mutationReceipt, inScope func(absPath string) bool) relationFreshness { states := make(map[string]string) - for _, receipt := range lagging { + for _, receipt := range receipts { if !inScope(receipt.path) { continue } key := s.repoRelative(receipt.path) - if receipt.outcome(true).Pending { + outcome := receipt.outcome(true) + if outcome.Pending || (outcome.Err == nil && outcome.Reindexed) { states[key] = "pending" } else if states[key] == "" { states[key] = "failed" } } - label := relationFreshness{waited: time.Since(started)} + var label relationFreshness for path, state := range states { label.files = append(label.files, path) if label.state != "failed" { @@ -214,6 +236,25 @@ func (f relationFreshness) apply(result map[string]any) { } } +// labelled is payload with the label's fields merged in, a struct payload +// being re-read as its JSON object first. A fresh scope returns payload +// unchanged. +func (f relationFreshness) labelled(payload any) any { + if f.state == "" { + return payload + } + data, err := json.Marshal(payload) + if err != nil { + return payload + } + var fields map[string]any + if json.Unmarshal(data, &fields) != nil || fields == nil { + return payload + } + f.apply(fields) + return fields +} + // line is the relations line a text answer leads with. func (f relationFreshness) line() string { return fmt.Sprintf("relations: %s (%d files)", f.state, len(f.files)) @@ -279,3 +320,57 @@ func (f relationFreshness) decorate(res *mcp.CallToolResult) *mcp.CallToolResult } return rebuildTextResult(res, body) } + +// provisionalSite is a line of a file whose edit the graph has not published +// that names a symbol as a whole identifier: a reference the graph cannot show +// yet, reported so a mutator neither misses it nor rewrites it. Line is 0 for +// the symbol's own file, which is provisional as a whole. +type provisionalSite struct { + File string `json:"file"` + Line int `json:"line"` + Text string `json:"text"` +} + +// provisionalSites scans the files of gaps for lines naming node, leaving out +// the lines where g already holds a reference to it. When the node's own file +// is among them it is one site without a line: until that edit publishes, the +// graph's span of the node may not hold, and a line that names the node there +// may be a new caller or the definition itself. +func (s *Server) provisionalSites(g graph.Reader, gaps []*mutationReceipt, node *graph.Node) []provisionalSite { + names := []string{identifierName(node.Name)} + known := make(map[string]bool) + for _, e := range g.GetInEdges(node.ID) { + if caller := g.GetNode(e.From); caller != nil && isReferencingEdgeKind(e.Kind) { + known[fmt.Sprintf("%s:%d", caller.FilePath, e.Line)] = true + } + } + seen := make(map[string]bool) + var sites []provisionalSite + for _, receipt := range gaps { + file := s.repoRelative(receipt.path) + if seen[file] { + continue + } + seen[file] = true + if file == node.FilePath { + sites = append(sites, provisionalSite{File: file}) + continue + } + data, err := os.ReadFile(receipt.path) + if err != nil || !namesAnyIdentifier(data, names) { + continue + } + for i, line := range strings.Split(string(data), "\n") { + if namesAnyIdentifier([]byte(line), names) && !known[fmt.Sprintf("%s:%d", file, i+1)] { + sites = append(sites, provisionalSite{File: file, Line: i + 1, Text: strings.TrimSpace(line)}) + } + } + } + sort.Slice(sites, func(i, j int) bool { + if sites[i].File != sites[j].File { + return sites[i].File < sites[j].File + } + return sites[i].Line < sites[j].Line + }) + return sites +} diff --git a/internal/mcp/relation_freshness_test.go b/internal/mcp/relation_freshness_test.go index 4eef99956..1414642c2 100644 --- a/internal/mcp/relation_freshness_test.go +++ b/internal/mcp/relation_freshness_test.go @@ -192,3 +192,130 @@ func TestChangeAnswersStartWithRelationsLine(t *testing.T) { map[string]any{"format": "toon"})) require.Equal(t, line, strings.SplitN(detect, "\n", 2)[0], detect) } + +func nodeID(t *testing.T, s *Server, name string) string { + t.Helper() + for _, n := range s.graph.AllNodes() { + if n.Name == name && n.Kind == graph.KindFunction { + return n.ID + } + } + t.Fatalf("%s is not indexed", name) + return "" +} + +// A call to a symbol that an edit added and the graph has not read yet must +// block safe_delete like a graph reference, in plain and propagate mode. An +// edit to the symbol's own file blocks it as a whole: it may add a caller +// there, and the graph's span of the symbol may no longer hold. +func TestSafeDeleteRejectsTargetCalledFromPendingFile(t *testing.T) { + for name, edit := range map[string]struct { + file, src string + ref map[string]any + }{ + "caller file": {"namer.go", "package p\n\nfunc Namer() {\n\tOther()\n}\n", + map[string]any{"kind": "provisional", "file_path": "namer.go", "line": float64(4)}}, + "definition file": {"other.go", "package p\n\nfunc Extra() {\n\tOther()\n}\n\nfunc Other() {}\n", + map[string]any{"kind": "provisional", "file_path": "other.go"}}, + } { + t.Run(name, func(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-"+edit.file, "", writeUnindexed(t, root, edit.file, edit.src), 1) + otherPath := filepath.Join(root, "other.go") + before, err := os.ReadFile(otherPath) + require.NoError(t, err) + id := nodeID(t, s, "Other") + + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleSafeDeleteSymbol, "safe_delete_symbol", + map[string]any{"id": id, "dry_run": false, "format": "json"})) + require.Equal(t, "rejected_has_references", payload["status"]) + require.Equal(t, []any{edit.ref}, payload["references"]) + require.Equal(t, "pending", payload["graph_freshness"]) + + payload = lifecycleResultPayload(t, callChangeTool(t, s.handleSafeDeleteSymbol, "safe_delete_symbol", + map[string]any{"id": id, "propagate": true, "dry_run": false, "format": "json"})) + require.Equal(t, "propagation_blocked_manual", payload["status"]) + require.Equal(t, float64(1), payload["manual"]) + + after, err := os.ReadFile(otherPath) + require.NoError(t, err) + require.Equal(t, string(before), string(after), "the target was deleted with a pending caller") + }) + } + + // A call the graph already holds is listed once, as the graph's + // reference, when its file has an unrelated pending edit; propagate can + // still remove it. + t.Run("graph reference", func(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-caller", "", + writeUnindexed(t, root, "caller.go", "package p\n\nfunc Caller() {\n\tTarget()\n}\n\nfunc More() {}\n"), 1) + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleSafeDeleteSymbol, "safe_delete_symbol", + map[string]any{"id": targetID(t, s), "format": "json"})) + require.Equal(t, "rejected_has_references", payload["status"]) + require.Equal(t, float64(1), payload["reference_count"], "%v", payload["references"]) + payload = lifecycleResultPayload(t, callChangeTool(t, s.handleSafeDeleteSymbol, "safe_delete_symbol", + map[string]any{"id": targetID(t, s), "propagate": true, "format": "json"})) + require.Equal(t, "propagation_plan", payload["status"]) + require.Equal(t, float64(0), payload["manual"], "%v", payload["caller_patches"]) + }) + + // Target is orphaned with Caller by the graph's edges, but a pending edit + // now calls it: the cascade must not delete it with Caller. + t.Run("cascade member", func(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-namer", "", + writeUnindexed(t, root, "namer.go", "package p\n\nfunc Namer() {\n\tTarget()\n}\n"), 1) + sources := map[string]string{} + for _, name := range []string{"caller.go", "target.go"} { + data, err := os.ReadFile(filepath.Join(root, name)) + require.NoError(t, err) + sources[name] = string(data) + } + payload := lifecycleResultPayload(t, callChangeTool(t, s.handleSafeDeleteSymbol, "safe_delete_symbol", + map[string]any{"id": nodeID(t, s, "Caller"), "cascade": "apply", "dry_run": false, "format": "json"})) + require.Equal(t, "rejected_has_references", payload["status"], "%v", payload) + require.Equal(t, []any{map[string]any{"kind": "provisional", "file_path": "namer.go", "line": float64(4)}}, payload["references"]) + for name, src := range sources { + data, err := os.ReadFile(filepath.Join(root, name)) + require.NoError(t, err) + require.Equal(t, src, string(data), "%s changed although the cascade was refused", name) + } + }) +} + +// The relation-reading mutators answer at once and label the answer while an +// edit in their scope never publishes; they never wait it out or refuse. +func TestRelationToolsLabelAndNeverRefuseWithUnresolvedReceipt(t *testing.T) { + s, root := newRelationFixture(t) + pendingFreshnessReceipt(s, "receipt-namer", "", + writeUnindexed(t, root, "namer.go", "package p\n\nfunc Namer() {\n\tTarget()\n}\n"), 1) + id := targetID(t, s) + rename := `{"changes":{"target.go":[{"range":{"start":{"line":2,"character":5},"end":{"line":2,"character":11}},"newText":"Renamed"}]}}` + comment := `{"changes":{"file://` + filepath.ToSlash(filepath.Join(root, "namer.go")) + + `":[{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"newText":"// Namer names Target.\n"}]}}` + + for tool, call := range map[string]func() *mcpgo.CallToolResult{ + "change_contract": func() *mcpgo.CallToolResult { + return callChangeTool(t, s.handleChangeContract, "change_contract", + map[string]any{"source": "symbols", "symbols": id, "format": "json"}) + }, + "preview_edit": func() *mcpgo.CallToolResult { + return callChangeTool(t, s.handlePreviewEdit, "preview_edit", + map[string]any{"workspace_edit": rename, "diagnostics": false}) + }, + // No symbol is removed or renamed: only the touched file, named by + // an absolute file URI, brings the pending edit into scope. + "preview_edit file uri": func() *mcpgo.CallToolResult { + return callChangeTool(t, s.handlePreviewEdit, "preview_edit", + map[string]any{"workspace_edit": comment, "diagnostics": false}) + }, + } { + started := time.Now() + payload := lifecycleResultPayload(t, call()) + require.Less(t, time.Since(started), 2*time.Second, "%s waited on a receipt that never publishes", tool) + require.Equal(t, "pending", payload["graph_freshness"], "%s: %v", tool, payload) + require.Equal(t, []any{"namer.go"}, payload["pending_files"], tool) + require.Equal(t, false, payload["complete"], tool) + } +} diff --git a/internal/mcp/tools_safe_delete.go b/internal/mcp/tools_safe_delete.go index 17b194870..08ac47ad4 100644 --- a/internal/mcp/tools_safe_delete.go +++ b/internal/mcp/tools_safe_delete.go @@ -52,10 +52,11 @@ func (s *Server) registerSafeDeleteSymbolTool() { // information for the caller to navigate to it and remove it before // retrying the delete. type safeDeleteReference struct { - FromID string `json:"from_id"` + FromID string `json:"from_id,omitempty"` Kind string `json:"kind"` FromName string `json:"from_name,omitempty"` FilePath string `json:"file_path,omitempty"` + Line int `json:"line,omitempty"` } // cascadeClosureEntry describes a symbol that the cascade pass added @@ -109,6 +110,20 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq // EdgeDefines / EdgeMemberOf are skipped (they don't represent // "someone calls this"). refs := collectReferencingEdges(g, id) + // A call that an edit added and the graph has not read yet blocks the + // delete like a graph reference does: a provisional reference here, a + // manual patch in propagate mode. The same snapshot labels the answer. + gaps := s.laggingMutationReceipts(ctx) + provisional := s.provisionalSites(g, gaps, node) + for _, site := range provisional { + refs = append(refs, safeDeleteReference{Kind: "provisional", FilePath: site.File, Line: site.Line}) + } + respond := func(result map[string]any) (*mcp.CallToolResult, error) { + label := s.labelRelations(ctx, gaps, []string{id}, nil) + label.apply(result) + res, err := s.respondJSONOrTOON(ctx, req, result) + return label.decorate(res), err + } // SRN-7: propagate-delete — patch the surviving call sites instead of // refusing. Builds a per-caller plan, applies the removable standalone @@ -116,9 +131,18 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq var appliedPatches, failedPatches []callerPatch if req.GetBool("propagate", false) && len(refs) > 0 { plan := s.buildPropagationPlan(ctx, g, node) + for _, site := range provisional { + reason := "provisional: named in a file whose edit the graph has not read yet — patch by hand" + if site.Line == 0 { + reason = "provisional: the graph has not read the latest edit to the symbol's own file — retry once it publishes" + } + plan = append(plan, callerPatch{ + File: site.File, Line: site.Line, CurrentText: site.Text, Action: "manual", Reason: reason, + }) + } manual := countManual(plan) if dryRun { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "status": "propagation_plan", "symbol": id, "file": node.FilePath, @@ -131,7 +155,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq }) } if manual > 0 && !force { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "status": "propagation_blocked_manual", "symbol": id, "caller_patches": plan, @@ -145,7 +169,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq } appliedPatches, failedPatches = applied, failed if len(failed) > 0 && !force { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "status": "propagation_partial", "symbol": id, "patches_applied": applied, @@ -158,7 +182,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq force = true node = g.GetNode(id) if node == nil { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "status": "deleted_by_propagation", "symbol": id, "patches_applied": appliedPatches, @@ -171,7 +195,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq } if len(refs) > 0 && !force { - return s.respondJSONOrTOON(ctx, req, map[string]any{ + return respond(map[string]any{ "status": "rejected_has_references", "symbol": id, "file": node.FilePath, @@ -195,6 +219,40 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq if cascadeMode != cascadeModeOff { closure, cascadeTruncated = computeCascadeClosure(g, node, cascadeIntoTests) } + if cascadeMode == cascadeModeApply { + // The closure is orphaned by the graph's edges only. A call that an + // unpublished edit added keeps a member alive, and force does not + // cover the members, so the cascade waits for that edit. + var blocked []safeDeleteReference + var members []string + for _, entry := range closure { + member := g.GetNode(entry.ID) + if member == nil { + continue + } + sites := s.provisionalSites(g, gaps, member) + if len(sites) > 0 { + members = append(members, entry.ID) + } + for _, site := range sites { + blocked = append(blocked, safeDeleteReference{Kind: "provisional", FilePath: site.File, Line: site.Line}) + } + } + if len(blocked) > 0 { + return respond(map[string]any{ + "status": "rejected_has_references", + "symbol": id, + "file": node.FilePath, + "references": blocked, + "reference_count": len(blocked), + "dry_run": dryRun, + "force": force, + "cascade_mode": cascadeMode, + "cascade_closure": closure, + "hint": "cascade member(s) " + strings.Join(members, ", ") + " are named in files whose edits the graph has not read yet; retry once those publish, or delete without cascade", + }) + } + } absPath, err := s.resolveNodePath(ctx, node) if err != nil { @@ -242,7 +300,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq if dryRun { result["status"] = "preview" - return s.respondJSONOrTOON(ctx, req, result) + return respond(result) } // Commit phase. cascadeModeApply expands the work to every entry @@ -282,7 +340,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq result["status"] = "partial_failure" result["cascade_deleted"] = deletedIDs result["error"] = err.Error() - return s.respondJSONOrTOON(ctx, req, result) + return respond(result) } // Persist session state and re-index every touched file. @@ -310,7 +368,7 @@ func (s *Server) handleSafeDeleteSymbol(ctx context.Context, req mcp.CallToolReq if len(failedPatches) > 0 { result["patches_failed"] = failedPatches } - return s.respondJSONOrTOON(ctx, req, result) + return respond(result) } // normaliseCascadeMode coerces the cascade parameter to a known diff --git a/internal/mcp/tools_simulate.go b/internal/mcp/tools_simulate.go index fdfadea7c..4156dfb43 100644 --- a/internal/mcp/tools_simulate.go +++ b/internal/mcp/tools_simulate.go @@ -107,6 +107,9 @@ func (s *Server) handlePreviewEdit(ctx context.Context, req mcp.CallToolRequest) wantDiag := req.GetBool("diagnostics", true) diagTimeout := time.Duration(req.GetInt("diagnostics_timeout_ms", 1500)) * time.Millisecond + // Snapshot the unpublished edits before the simulation reads relations; + // the symbols that scope the label are known only once it has run. + gaps := s.laggingMutationReceipts(ctx) sim, simErr := s.buildSimulation(ctx, []lsp.WorkspaceEdit{edit}, inherit) if simErr != nil { if ctxErr := requestContextError(ctx, simErr); ctxErr != nil { @@ -138,6 +141,20 @@ func (s *Server) handlePreviewEdit(ctx context.Context, req mcp.CallToolRequest) result["diagnostics_summary"] = summariseDiagnostics(diags) } + // broken_callers walks the callers of the removed and renamed symbols. + seeds := append([]string{}, step.symbolsRemoved...) + for _, renamed := range step.symbolsRenamed { + seeds = append(seeds, renamed["from"]) + } + // touched_files keeps each path as the client spelled it; the scope + // takes graph keys. + var touched []string + for _, path := range step.touchedFiles { + if abs, err := s.resolveOverlayAbsPath(path); err == nil && abs != "" { + touched = append(touched, s.repoRelative(abs)) + } + } + s.labelRelations(ctx, gaps, seeds, touched).apply(result) return mcp.NewToolResultText(jsonOK(result)), nil } From 800ebcfc1b97f7b1898304380aa7cd7d663a1a58 Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sun, 1 Nov 2026 18:18:41 +0100 Subject: [PATCH 5/6] fix(indexer): release identity permits before reporting completion --- internal/indexer/go_module_extern_test.go | 11 ++- .../prefix_observation_publication_test.go | 71 +++++++++++++++++++ internal/indexer/prefix_ownership.go | 11 ++- internal/indexer/track_naming.go | 8 ++- 4 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 internal/indexer/prefix_observation_publication_test.go diff --git a/internal/indexer/go_module_extern_test.go b/internal/indexer/go_module_extern_test.go index e8546be5f..147ee509e 100644 --- a/internal/indexer/go_module_extern_test.go +++ b/internal/indexer/go_module_extern_test.go @@ -153,8 +153,17 @@ func Caller() { library.Hello(); child.Nested() } } case "parallel": errors := make(chan error, len(repos)) + // This is a module-binding test, not an identity-admission + // saturation test. The decoy case has three roots; retain + // parallel tracking within the two native observation slots. + admitted := make(chan struct{}, 2) for _, entry := range repos { - go func() { _, e := mi.TrackRepoCtx(context.Background(), entry); errors <- e }() + go func() { + admitted <- struct{}{} + defer func() { <-admitted }() + _, e := mi.TrackRepoCtx(context.Background(), entry) + errors <- e + }() } for range repos { require.NoError(t, <-errors) diff --git a/internal/indexer/prefix_observation_publication_test.go b/internal/indexer/prefix_observation_publication_test.go new file mode 100644 index 000000000..26df8a0ec --- /dev/null +++ b/internal/indexer/prefix_observation_publication_test.go @@ -0,0 +1,71 @@ +package indexer + +import ( + "context" + "os" + "testing" + "testing/synctest" + "time" + + "github.com/stretchr/testify/require" +) + +// Publication must mean the native worker has returned its allowance. A caller +// often starts another observation immediately; announcing completion before +// permit return can spuriously exhaust both slots under parallel tracking. +func TestIdentityObservationPublishesOnlyAfterPermitReturn(t *testing.T) { + root := t.TempDir() + for _, operation := range []string{"stat", "naming"} { + t.Run(operation, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var allowance prefixStatAllowance + allowance.once.Do(func() { allowance.slots = make(chan struct{}, 2) }) + nativeDone := make(chan struct{}) + // Temporarily intercept this worker's token so its release must + // wait for the test. This makes publication ordering observable + // without scheduler timing, polling or an extra production hook. + finishNative := func() { + <-allowance.slots + close(nativeDone) + } + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + result := make(chan error, 1) + if operation == "stat" { + allowance.stat = func(path string) (os.FileInfo, error) { + info, err := os.Stat(path) + finishNative() + return info, err + } + go func() { result <- allowance.observe(ctx, root).err }() + } else { + go func() { + _, err := allowance.observeNaming(ctx, root, func(context.Context, string) trackNamingFacts { + finishNative() + return trackNamingFacts{canonical: root} + }) + result <- err + }() + } + <-nativeDone + synctest.Wait() + var publishedEarly bool + var err error + select { + case err = <-result: + publishedEarly = true + default: + } + // Always let the worker finish before assertions, including RED. + allowance.slots <- struct{}{} + synctest.Wait() + if !publishedEarly { + err = <-result + } + require.NoError(t, err) + require.Zero(t, len(allowance.slots)) + require.False(t, publishedEarly, "completed identity result escaped before its permit was returned") + }) + }) + } +} diff --git a/internal/indexer/prefix_ownership.go b/internal/indexer/prefix_ownership.go index a8da85371..75beaf395 100644 --- a/internal/indexer/prefix_ownership.go +++ b/internal/indexer/prefix_ownership.go @@ -60,9 +60,14 @@ func (a *prefixStatAllowance) observe(ctx context.Context, path string) prefixSt slots := a.slots done := make(chan prefixStatResult, 1) go func() { - defer func() { <-slots }() - info, err := stat(path) - done <- prefixStatResult{info: info, err: err} + result := func() prefixStatResult { + defer func() { <-slots }() + info, err := stat(path) + return prefixStatResult{info: info, err: err} + }() + // A completed result authorizes the caller's next observation. + // Return this permit first, even when the receiver runs immediately. + done <- result }() select { case <-ctx.Done(): diff --git a/internal/indexer/track_naming.go b/internal/indexer/track_naming.go index 3505de97f..72b9dfb9b 100644 --- a/internal/indexer/track_naming.go +++ b/internal/indexer/track_naming.go @@ -86,8 +86,12 @@ func (a *prefixStatAllowance) observeNaming(ctx context.Context, root string, re workerCtx, cancel := context.WithDeadline(context.Background(), deadline) defer cancel() go func() { - defer func() { <-slots }() - done <- read(workerCtx, root) + facts := func() trackNamingFacts { + defer func() { <-slots }() + return read(workerCtx, root) + }() + // Completion includes permit return, as for root Stat observations. + done <- facts }() select { case <-ctx.Done(): From 7578e2758b29be9d4acba9bf82ec780d46645695 Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sun, 1 Nov 2026 19:39:50 +0100 Subject: [PATCH 6/6] test: align relation recovery and native file URI controls --- internal/mcp/mutation_freshness_hints_test.go | 14 +++++--------- internal/mcp/relation_freshness_test.go | 2 +- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/internal/mcp/mutation_freshness_hints_test.go b/internal/mcp/mutation_freshness_hints_test.go index eb5f96d73..9e1cd9ffb 100644 --- a/internal/mcp/mutation_freshness_hints_test.go +++ b/internal/mcp/mutation_freshness_hints_test.go @@ -105,7 +105,7 @@ func TestFreshnessRefusalAnnotationBoundsAndScope(t *testing.T) { // Exercise the public call shapes against real handlers and an owned working // tree. The existing watcher seam supplies a failed publication result after // the edit acknowledges; this does not inject a real store patch failure. -func TestFreshnessRefusalRecoveryFlow(t *testing.T) { +func TestRelationFreshnessRecoveryFlow(t *testing.T) { s, root := setupTestServer(t) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -144,14 +144,10 @@ func TestFreshnessRefusalRecoveryFlow(t *testing.T) { waitCheckoutReceipt(t, s, id) detectArgs := map[string]any{"operation": "detect", "options": map[string]any{"scope": "unstaged"}} - refused := call("change", detectArgs) - require.True(t, refused.IsError) - for _, want := range []string{ - "graph freshness unavailable", "change.receipt options.receipt=" + id, - "workspace_admin operation=reindex", "paths=[P]", `mutation_id="recovery-flow"`, - } { - require.Contains(t, resultText(refused), want) - } + partial := resultJSON(t, call("change", detectArgs)) + require.Equal(t, false, partial["complete"]) + require.Equal(t, "failed", partial["graph_freshness"]) + require.NotEmpty(t, partial["recovery_hint"]) status := resultJSON(t, call("change", map[string]any{ "operation": "receipt", "options": map[string]any{"mutation_id": id}, })) diff --git a/internal/mcp/relation_freshness_test.go b/internal/mcp/relation_freshness_test.go index 1414642c2..368e8b28a 100644 --- a/internal/mcp/relation_freshness_test.go +++ b/internal/mcp/relation_freshness_test.go @@ -292,7 +292,7 @@ func TestRelationToolsLabelAndNeverRefuseWithUnresolvedReceipt(t *testing.T) { writeUnindexed(t, root, "namer.go", "package p\n\nfunc Namer() {\n\tTarget()\n}\n"), 1) id := targetID(t, s) rename := `{"changes":{"target.go":[{"range":{"start":{"line":2,"character":5},"end":{"line":2,"character":11}},"newText":"Renamed"}]}}` - comment := `{"changes":{"file://` + filepath.ToSlash(filepath.Join(root, "namer.go")) + + comment := `{"changes":{"` + nativeFileURI(filepath.Join(root, "namer.go")) + `":[{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"newText":"// Namer names Target.\n"}]}}` for tool, call := range map[string]func() *mcpgo.CallToolResult{