diff --git a/docs/mcp.md b/docs/mcp.md index df13918d7..470fcfad6 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -512,6 +512,8 @@ Gortex captures every large tool response into a bounded per-session ring; these | `search_artifacts` | Full-text search over the context-artifacts manifest — DB schemas, API specs, infra configs, ADRs registered via `.gortex.yaml::artifacts` | | `get_artifact` | Fetch one context artifact by id, with its content and the symbols it references | +`detect_changes` and `explain_change_impact` wait up to 250 ms for edits in the answer's relation scope: the symbols' files, their callers, files naming the symbols, and the diff's changed files. Edits outside that scope do not delay the answer. Pending or failed edits return the usual analysis with `complete:false`, `graph_freshness:pending|failed`, `freshness_scope:relations`, `pending_files` (at most 16), and `pending_file_count`; failures include a recovery hint. Every answer reports `barrier_wait_ms`. An earlier `wait_deadline` shortens the wait; `require_fresh:true` instead waits until its request deadline and returns an error if the scope is still lagging. Text answers begin with `relations: pending|failed (N files)`; GCX preserves `GCX1` on line 1 and puts the relations comment on line 2. Stop and post-edit briefings bound detection to 300 ms and report pending relations instead of silently omitting the briefing. + ## Proactive safety | Tool | Description | 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/hooks/posttask.go b/internal/hooks/posttask.go index 268597984..b9827450b 100644 --- a/internal/hooks/posttask.go +++ b/internal/hooks/posttask.go @@ -136,25 +136,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. @@ -248,10 +329,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 @@ -368,8 +452,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 "" } @@ -464,7 +554,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", @@ -475,14 +567,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). diff --git a/internal/mcp/checkout_mutation_receipt_test.go b/internal/mcp/checkout_mutation_receipt_test.go index 031fb5d68..89a78996b 100644 --- a/internal/mcp/checkout_mutation_receipt_test.go +++ b/internal/mcp/checkout_mutation_receipt_test.go @@ -173,7 +173,7 @@ func TestCheckoutMutationReceiptScopeAndSummary(t *testing.T) { if pending, err := s.mutationFreshnessSummaryForRepos(ctx, "fixture"); pending || !errors.Is(err, failure) { t.Fatalf("summary did not preserve terminal cause: pending=%t err=%v", pending, err) } - if err := s.awaitMutationFreshnessForRepos(context.Background(), "fixture"); err != nil { + if pending, err := s.mutationFreshnessSummaryForRepos(context.Background(), "fixture"); pending || err != nil { t.Fatalf("sibling checkout failure blocked canonical graph: %v", err) } } @@ -265,9 +265,6 @@ func TestCheckoutMutationReceiptSupersededDoesNotPoisonCurrentView(t *testing.T) if pending, err := s.mutationFreshnessSummaryForRepos(ctx, "fixture"); pending || err != nil { t.Fatalf("historical branch poisoned current-view diagnostics: pending=%t err=%v", pending, err) } - if err := s.awaitMutationFreshnessForRepos(ctx, "fixture"); err != nil { - t.Fatalf("historical branch poisoned current-view barrier: %v", err) - } payload := s.mutationStatusPayload(record) if payload["graph_status"] != mutationGraphFailed || payload["graph_status_terminal"] != true || payload["new_sha"] != "original-bytes-sha" { t.Fatalf("historical receipt was falsely certified by current-view exemption: %#v", payload) @@ -399,8 +396,8 @@ func TestCheckoutMutationReceiptRecoveryRetiresOnlyPriorBarrierFailures(t *testi t.Fatalf("unrelated or later failure %q was retired", id) } } - } else if err := s.awaitMutationFreshnessForRepos(ctx, "fixture"); err != nil { - t.Fatalf("verified root recovery left current-view barrier poisoned: %v", err) + } else if pending, err := s.mutationFreshnessSummaryForRepos(ctx, "fixture"); pending || err != nil { + t.Fatalf("verified root recovery left current-view summary poisoned: %v", err) } }) } diff --git a/internal/mcp/edit_serialization.go b/internal/mcp/edit_serialization.go index 958884cfc..83f7aeb92 100644 --- a/internal/mcp/edit_serialization.go +++ b/internal/mcp/edit_serialization.go @@ -17,7 +17,6 @@ import ( const ( defaultMutationReindexWait = 3 * time.Second - defaultMutationSafetyWait = 3 * time.Second mutationReceiptRetention = 10 * time.Minute ) @@ -533,13 +532,6 @@ func (s *Server) mutationWaitDuration() time.Duration { return defaultMutationReindexWait } -func (s *Server) mutationSafetyWaitDuration() time.Duration { - if s.mutationSafetyWait > 0 { - return s.mutationSafetyWait - } - return defaultMutationSafetyWait -} - // mutationReindexState returns the graph-freshness state to expose after a // successful disk mutation. A live watcher already owns debouncing, patch // serialization, and latest-bytes reconciliation, so duplicating IndexFile in @@ -611,12 +603,6 @@ func (s *Server) mutationReindexState(ctx context.Context, absPath string) mutat return mutationReindexOutcome{Reindexed: s.reindexFile(absPath)} } -// awaitMutationFreshness is the conservative all-repository safety barrier. -// Callers that can resolve a target repository should use the scoped sibling. -func (s *Server) awaitMutationFreshness(ctx context.Context) error { - return s.awaitMutationFreshnessForRepos(ctx) -} - // mutationCheckoutScope is metadata-only so receipts remain inspectable while // the selected checkout has no materialized graph. An unselected request is // canonical; sharing a repository prefix does not give it sibling receipts. @@ -704,116 +690,6 @@ func (s *Server) mutationFreshnessSummaryForRepos(ctx context.Context, repos ... return pending, errors.Join(failures...) } -// awaitMutationFreshnessForRepos waits once, under one shared budget, for every -// selected ticket. On timeout it reports all pending and terminally failed -// generations rather than hiding the remaining gaps behind the first one. -func (s *Server) awaitMutationFreshnessForRepos(ctx context.Context, repos ...string) error { - receipts := s.mutationReceiptsForRepos(ctx, repos...) - if len(receipts) == 0 { - return nil - } - sort.Slice(receipts, func(i, j int) bool { - if receipts[i].repo != receipts[j].repo { - return receipts[i].repo < receipts[j].repo - } - if receipts[i].path != receipts[j].path { - return receipts[i].path < receipts[j].path - } - if receipts[i].generation != receipts[j].generation { - return receipts[i].generation < receipts[j].generation - } - return receipts[i].id < receipts[j].id - }) - - timer := time.NewTimer(s.mutationSafetyWaitDuration()) - defer timer.Stop() - waitReason := "" -waitLoop: - for _, receipt := range receipts { - select { - case <-receipt.done: - case <-timer.C: - waitReason = "wait budget expired" - break waitLoop - case <-ctx.Done(): - waitReason = "request cancelled: " + ctx.Err().Error() - break waitLoop - } - } - - issues := make([]string, 0, len(receipts)) - hasTerminalFailure := false - for _, receipt := range receipts { - issue := "" - select { - case <-receipt.done: - outcome := receipt.outcome(false) - switch { - case outcome.Err != nil: - hasTerminalFailure = true - issue = fmt.Sprintf( - "failed receipt=%s repo=%q path=%q generation=%d error=%q", - receipt.id, receipt.repo, receipt.path, receipt.generation, outcome.Err.Error()) - case !outcome.Reindexed: - hasTerminalFailure = true - issue = fmt.Sprintf( - "failed receipt=%s repo=%q path=%q generation=%d error=%q", - receipt.id, receipt.repo, receipt.path, receipt.generation, "reindex not confirmed") - } - default: - issue = fmt.Sprintf( - "pending receipt=%s repo=%q path=%q generation=%d", - receipt.id, receipt.repo, receipt.path, receipt.generation) - } - if issue != "" { - issues = append(issues, issue+s.mutationFreshnessReceiptHint(ctx, receipt.id)) - } - } - if len(issues) == 0 { - if err := ctx.Err(); err != nil { - return fmt.Errorf("graph freshness wait cancelled: %w", err) - } - return nil - } - - message := "graph freshness unavailable" - if waitReason != "" { - message += " (" + waitReason + ")" - } - for i, issue := range issues { - if i == 0 { - message += ": " - } else { - message += "; " - } - message += issue - } - if hasTerminalFailure { - message += "; " + mutationFreshnessRecoveryHint() - } - return fmt.Errorf("%s", message) -} - -// mutationReposForSymbolIDs resolves a complete repository scope for a symbol -// request. Any unresolved input returns nil, which deliberately widens the -// barrier to all receipts rather than excluding an unknown mutation. -func (s *Server) mutationReposForSymbolIDs(ctx context.Context, ids []string) []string { - repos := make(map[string]struct{}, len(ids)) - for _, id := range ids { - node := s.engineFor(ctx).GetSymbol(id) - if node == nil || node.RepoPrefix == "" { - return nil - } - repos[node.RepoPrefix] = struct{}{} - } - result := make([]string, 0, len(repos)) - for repo := range repos { - result = append(result, repo) - } - sort.Strings(result) - return result -} - // derivedFanoutKeyComplete is the wire spelling of the bounded-derived-pass // completeness half of a mutation's graph verdict. Keep the key names stable: // a client that polls change.receipt reads them next to graph_status. diff --git a/internal/mcp/edit_serialization_test.go b/internal/mcp/edit_serialization_test.go index 199f6bc3d..926995b0a 100644 --- a/internal/mcp/edit_serialization_test.go +++ b/internal/mcp/edit_serialization_test.go @@ -5,7 +5,6 @@ import ( "errors" "os" "path/filepath" - "strings" "sync/atomic" "testing" "time" @@ -230,7 +229,6 @@ func TestMutationReindexStateSlowTicketReceiptCompletes(t *testing.T) { s := &Server{ watcher: mutationTestWatcher{done: done, generation: 7}, mutationReindexWait: 5 * time.Millisecond, - mutationSafetyWait: 5 * time.Millisecond, } outcome := s.mutationReindexState(context.Background(), "/tmp/slow.go") if outcome.Reindexed || !outcome.Pending || outcome.Receipt == "" || outcome.Generation != 7 { @@ -239,9 +237,6 @@ func TestMutationReindexStateSlowTicketReceiptCompletes(t *testing.T) { if status, ok := s.mutationReceiptState(outcome.Receipt); !ok || !status.Pending || status.Generation != 7 { t.Fatalf("pending receipt state = %+v, ok=%v", status, ok) } - if err := s.awaitMutationFreshness(context.Background()); err == nil || !strings.Contains(err.Error(), "pending receipt=") { - t.Fatalf("pending safety barrier error = %v", err) - } done <- indexer.MutationResult{ RequestedGeneration: 7, @@ -253,9 +248,6 @@ func TestMutationReindexStateSlowTicketReceiptCompletes(t *testing.T) { for time.Now().Before(deadline) { status, ok := s.mutationReceiptState(outcome.Receipt) if ok && status.Reindexed && !status.Pending && status.AppliedGeneration == 8 { - if err := s.awaitMutationFreshness(context.Background()); err != nil { - t.Fatalf("completed safety barrier: %v", err) - } return } time.Sleep(time.Millisecond) @@ -263,7 +255,7 @@ func TestMutationReindexStateSlowTicketReceiptCompletes(t *testing.T) { t.Fatal("mutation receipt did not reach a terminal reindexed state") } -func TestMutationReindexStateFailureBlocksSafetyReads(t *testing.T) { +func TestMutationReindexStatePreservesFailure(t *testing.T) { patchFailure := errors.New("patch failed") done := make(chan indexer.MutationResult, 1) done <- indexer.MutationResult{RequestedGeneration: 3, AppliedGeneration: 3, Err: patchFailure} @@ -273,9 +265,6 @@ func TestMutationReindexStateFailureBlocksSafetyReads(t *testing.T) { if !errors.Is(outcome.Err, patchFailure) || outcome.Reindexed || outcome.Pending { t.Fatalf("failed ticket outcome = %+v", outcome) } - if err := s.awaitMutationFreshness(context.Background()); err == nil || !strings.Contains(err.Error(), "patch failed") { - t.Fatalf("failed safety barrier error = %v", err) - } } func TestAttachMutationFreshnessPendingOmitsStaleSyntaxHealth(t *testing.T) { 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_hints_test.go b/internal/mcp/mutation_freshness_hints_test.go index eb5f96d73..1163398ff 100644 --- a/internal/mcp/mutation_freshness_hints_test.go +++ b/internal/mcp/mutation_freshness_hints_test.go @@ -17,48 +17,8 @@ import ( "github.com/zzet/gortex/internal/indexer" ) -func TestFreshnessRefusalHintLists(t *testing.T) { - s := &Server{mutationSafetyWait: time.Nanosecond} - newID := func() string { return fmt.Sprintf("mutation-%d", mutationReceiptSequence.Add(1)) } - failed := pendingFreshnessReceipt(s, newID(), "fixture", "/fixture/failed.go", 1) - completeFreshnessReceipt(failed, indexer.MutationResult{Err: errors.New("patch failed")}) - unkeyed := pendingFreshnessReceipt(s, newID(), "fixture", "/fixture/other.go", 2) - completeFreshnessReceipt(unkeyed, indexer.MutationResult{}) // No confirmed reindex. - pending := pendingFreshnessReceipt(s, newID(), "fixture", "/fixture/pending.go", 3) - for _, tc := range []struct { - receipt *mutationReceipt - key string - }{{failed, "original-edit"}, {unkeyed, ""}} { - record := s.beginMutationCommit(context.Background(), "edit_file", tc.key, "", tc.receipt.path, tc.receipt.path) - record.recordGraph(mutationReindexOutcome{Receipt: tc.receipt.id, Err: errors.New("patch failed")}) - } - - err := s.awaitMutationFreshnessForRepos(context.Background(), "fixture") - require.Error(t, err) - message := err.Error() - require.True(t, strings.HasPrefix(message, "graph freshness unavailable"), message) - entries := strings.Split(message, "; ") - require.Len(t, entries, 4, "three receipt entries and one shared hint: %s", message) - for i, receipt := range []*mutationReceipt{failed, unkeyed, pending} { - require.Contains(t, entries[i], "receipt="+receipt.id+" ") - require.Equal(t, 1, strings.Count(message, "receipt="+receipt.id+" ")) - require.Contains(t, entries[i], "(query: change.receipt options.receipt="+receipt.id+")") - } - require.Contains(t, entries[0], " commit=commit-") - require.Contains(t, entries[0], ` mutation_id="original-edit"`) - require.Contains(t, entries[1], " commit=commit-") - require.NotContains(t, entries[1], "mutation_id=") - require.NotContains(t, entries[2], "commit=") - for _, want := range []string{"later successful edit of the same path", "workspace_admin operation=reindex", "reindex_repository", "paths=[P]", "exactly that single file", "retention", "last resort"} { - require.Contains(t, entries[3], want) - } - for _, unavailable := range []string{"reconcile", "retry_exhausted", "retry-exhausted", "ETA"} { - require.NotContains(t, message, unavailable) - } -} - -func TestFreshnessRefusalPublicationPhases(t *testing.T) { - s := &Server{mutationSafetyWait: time.Nanosecond} +func TestFreshnessReceiptHintPublicationPhases(t *testing.T) { + s := &Server{} id := fmt.Sprintf("mutation-%d", mutationReceiptSequence.Add(1)) receipt := pendingFreshnessReceipt(s, id, "fixture", "/fixture/phases.go", 7) origin := time.Now() @@ -70,10 +30,9 @@ func TestFreshnessRefusalPublicationPhases(t *testing.T) { record.MarkAt(phase, origin.Add(time.Duration(i+1)*time.Millisecond)) } completeFreshnessReceipt(receipt, indexer.MutationResult{Err: errors.New("failed publication")}) - err := s.awaitMutationFreshnessForRepos(context.Background(), "fixture") - require.Error(t, err) - require.Contains(t, err.Error(), `publication_phases=[...+3,"ticket_enqueued"@3.0ms,"admitted"@4.0ms,"ticket_failed"@5.0ms]`) - require.NotContains(t, err.Error(), `"handler_started"`) + hint := s.mutationFreshnessReceiptHint(context.Background(), id) + require.Contains(t, hint, `publication_phases=[...+3,"ticket_enqueued"@3.0ms,"admitted"@4.0ms,"ticket_failed"@5.0ms]`) + require.NotContains(t, hint, `"handler_started"`) } func TestFreshnessRefusalAnnotationBoundsAndScope(t *testing.T) { @@ -105,7 +64,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 +103,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/mutation_freshness_test.go b/internal/mcp/mutation_freshness_test.go index d9dd4b21c..ba85c430e 100644 --- a/internal/mcp/mutation_freshness_test.go +++ b/internal/mcp/mutation_freshness_test.go @@ -47,66 +47,20 @@ func freshnessToolResultText(result *mcpgo.CallToolResult) string { return text.String() } -func TestMutationFreshnessRepoScopeAggregatesPending(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} - pendingFreshnessReceipt(s, "receipt-a1", "repo-a", "/repo-a/a.go", 3) - pendingFreshnessReceipt(s, "receipt-a2", "repo-a", "/repo-a/b.go", 8) - pendingFreshnessReceipt(s, "receipt-b", "repo-b", "/repo-b/c.go", 5) - - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil { - t.Fatal("repo-scoped freshness unexpectedly succeeded") - } - message := err.Error() - for _, want := range []string{"receipt-a1", "generation=3", "receipt-a2", "generation=8"} { - if !strings.Contains(message, want) { - t.Fatalf("freshness error %q does not contain %q", message, want) - } - } - if strings.Contains(message, "receipt-b") { - t.Fatalf("repo-scoped freshness included unrelated receipt: %s", message) - } -} - -func TestMutationFreshnessScopeIncludesUnknownAndIgnoresUnrelated(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} - pendingFreshnessReceipt(s, "receipt-b", "repo-b", "/repo-b/b.go", 2) - if err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a"); err != nil { - t.Fatalf("unrelated repository blocked repo-a: %v", err) - } - - pendingFreshnessReceipt(s, "receipt-unknown", "", "/unknown/u.go", 4) - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil || !strings.Contains(err.Error(), "receipt-unknown") { - t.Fatalf("unknown-owner receipt did not fail wide: %v", err) - } - if strings.Contains(err.Error(), "receipt-b") { - t.Fatalf("unknown-owner check also included unrelated known repo: %v", err) - } -} - -func TestMutationFreshnessTerminalFailureFailsClosed(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} - failed := pendingFreshnessReceipt(s, "receipt-failed", "repo-a", "/repo-a/broken.go", 9) - completeFreshnessReceipt(failed, indexer.MutationResult{ - RequestedGeneration: 9, - AppliedGeneration: 9, - Err: errors.New("syntax patch failed"), - }) - - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil { - t.Fatal("terminal patch failure did not fail closed") +func TestMutationFreshnessSummaryKeepsCheckoutRepoScope(t *testing.T) { + s := &Server{} + pendingFreshnessReceipt(s, "unrelated", "repo-b", "/repo-b/b.go", 2) + if pending, err := s.mutationFreshnessSummaryForRepos(context.Background(), "repo-a"); pending || err != nil { + t.Fatalf("unrelated repository affected summary: pending=%t err=%v", pending, err) } - for _, want := range []string{"failed", "receipt-failed", "generation=9", "syntax patch failed"} { - if !strings.Contains(err.Error(), want) { - t.Fatalf("terminal failure %q does not contain %q", err, want) - } + pendingFreshnessReceipt(s, "unknown", "", "/unknown/u.go", 4) + if pending, err := s.mutationFreshnessSummaryForRepos(context.Background(), "repo-a"); !pending || err != nil { + t.Fatalf("unknown-owner receipt missing from summary: pending=%t err=%v", pending, err) } } func TestMutationFreshnessSuccessResolvesSupersededFailures(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} + s := &Server{} stale := pendingFreshnessReceipt(s, "receipt-stale-failed", "repo-a", "/repo-a/file.go", 6) completeFreshnessReceipt(stale, indexer.MutationResult{ RequestedGeneration: 6, @@ -146,23 +100,6 @@ func TestMutationFreshnessSuccessResolvesSupersededFailures(t *testing.T) { t.Fatal("failure newer than the succeeded generation was resolved") } - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil { - t.Fatal("remaining failures did not fail closed") - } - message := err.Error() - if strings.Contains(message, "receipt-stale-failed") { - t.Fatalf("freshness error still reports the superseded receipt: %s", message) - } - for _, want := range []string{ - "receipt-other-path", - "receipt-newer-failed", - "do not recover by waiting", - } { - if !strings.Contains(message, want) { - t.Fatalf("freshness error %q does not contain %q", message, want) - } - } } func TestTrackMutationTicketResolvesSupersededFailures(t *testing.T) { @@ -197,7 +134,7 @@ func TestTrackMutationTicketResolvesSupersededFailures(t *testing.T) { } func TestMutationFreshnessSuccessKeepsPendingSamePathReceipts(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} + s := &Server{} inflight := pendingFreshnessReceipt(s, "receipt-inflight", "repo-a", "/repo-a/file.go", 4) appliedResult := indexer.MutationResult{ @@ -215,14 +152,10 @@ func TestMutationFreshnessSuccessKeepsPendingSamePathReceipts(t *testing.T) { if outcome := inflight.outcome(true); outcome.Pending != true { t.Fatalf("a still-pending receipt was marked completed by the resolve: %+v", outcome) } - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil || !strings.Contains(err.Error(), "receipt-inflight") { - t.Fatalf("barrier no longer reports the in-flight receipt: %v", err) - } } func TestReindexedPathResolvesFailedReceiptsForThatPathOnly(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} + s := &Server{} failed := pendingFreshnessReceipt(s, "receipt-failed", "repo-a", "/repo-a/file.go", 6) completeFreshnessReceipt(failed, indexer.MutationResult{ RequestedGeneration: 6, @@ -257,18 +190,6 @@ func TestReindexedPathResolvesFailedReceiptsForThatPathOnly(t *testing.T) { t.Fatal("the resolve stamped a result onto a receipt whose ticket is still in flight") } - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil { - t.Fatal("the remaining receipts did not fail closed") - } - if strings.Contains(err.Error(), "receipt-failed") { - t.Fatalf("the barrier still reports the resolved receipt: %v", err) - } - for _, want := range []string{"receipt-other-path", "receipt-inflight"} { - if !strings.Contains(err.Error(), want) { - t.Fatalf("barrier %q no longer reports %q", err, want) - } - } } func TestReindexedReceiptPathOnlyResolvesAProvenSinglePath(t *testing.T) { @@ -326,7 +247,7 @@ func TestReindexedReceiptPathOnlyResolvesAProvenSinglePath(t *testing.T) { } func TestReindexResolveIgnoresReceiptsThatFailedDuringThePass(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} + s := &Server{} before := pendingFreshnessReceipt(s, "receipt-before", "repo-a", "/repo-a/file.go", 6) completeFreshnessReceipt(before, indexer.MutationResult{ RequestedGeneration: 6, @@ -354,17 +275,10 @@ func TestReindexResolveIgnoresReceiptsThatFailedDuringThePass(t *testing.T) { t.Fatalf("a write that failed while the pass ran was stamped fresh: %+v", outcome) } - err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") - if err == nil || !strings.Contains(err.Error(), "receipt-during") { - t.Fatalf("the barrier stopped reporting the mid-pass failure: %v", err) - } - if strings.Contains(err.Error(), "receipt-before") { - t.Fatalf("the barrier still reports the resolved receipt: %v", err) - } } func TestFailedReceiptsBeforeRefusesToSnapshotAnUnattributablePass(t *testing.T) { - s := &Server{mutationSafetyWait: time.Millisecond} + s := &Server{} failed := pendingFreshnessReceipt(s, "receipt-failed", "repo-a", "/repo-a/file.go", 6) completeFreshnessReceipt(failed, indexer.MutationResult{ RequestedGeneration: 6, @@ -397,78 +311,44 @@ func TestFailedReceiptsBeforeRefusesToSnapshotAnUnattributablePass(t *testing.T) } } -func TestMutationReposForSymbolIDsUnresolvedWidensBarrier(t *testing.T) { - g := graph.New() - g.AddNode(&graph.Node{ - ID: "repo-a::known", - Kind: graph.KindFunction, - Name: "known", - FilePath: "known.go", - StartLine: 1, - EndLine: 1, - Language: "go", - RepoPrefix: "repo-a", - }) - s := &Server{ - graph: g, - engine: query.NewEngine(g), - session: newSessionState(), - mutationSafetyWait: time.Millisecond, - } - pendingFreshnessReceipt(s, "receipt-other", "repo-b", "/repo-b/other.go", 6) - - if repos := s.mutationReposForSymbolIDs(context.Background(), []string{"missing"}); repos != nil { - t.Fatalf("unresolved symbol scope = %v, want nil fail-wide scope", repos) - } - err := s.awaitMutationFreshnessForRepos(context.Background(), s.mutationReposForSymbolIDs(context.Background(), []string{"missing"})...) - if err == nil || !strings.Contains(err.Error(), "receipt-other") { - t.Fatalf("unresolved symbol did not widen the barrier: %v", err) - } - - repos := s.mutationReposForSymbolIDs(context.Background(), []string{"repo-a::known"}) - if len(repos) != 1 || repos[0] != "repo-a" { - t.Fatalf("resolved symbol scope = %v", repos) - } - if err := s.awaitMutationFreshnessForRepos(context.Background(), repos...); err != nil { - t.Fatalf("resolved repo-a scope was blocked by repo-b: %v", err) - } -} - -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 +356,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) } - if result == nil || result.IsError { - t.Fatalf("completed impact remained blocked: %q", freshnessToolResultText(result)) + + // 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 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 +390,54 @@ 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"]) + } + // 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) } - 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 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) + } + } + + // 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 new file mode 100644 index 000000000..1b489e247 --- /dev/null +++ b/internal/mcp/relation_freshness.go @@ -0,0 +1,281 @@ +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) +} + +// 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 +// 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/server.go b/internal/mcp/server.go index d0e52bdc7..5cc78c22f 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -413,7 +413,6 @@ type Server struct { // defaults. sync.Map keeps directly-constructed servers usable. mutationReceipts sync.Map mutationReindexWait time.Duration - mutationSafetyWait time.Duration // mutationReceiptAfterFunc replaces retention scheduling in deterministic // tests. Nil uses time.AfterFunc with the unchanged retention windows. mutationReceiptAfterFunc func(time.Duration, func()) diff --git a/internal/mcp/tools_analysis.go b/internal/mcp/tools_analysis.go index feae760a1..bec7687a4 100644 --- a/internal/mcp/tools_analysis.go +++ b/internal/mcp/tools_analysis.go @@ -352,12 +352,43 @@ 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 + var freshness requestFreshness + var keys []string + var deadline time.Time + 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) + deadline = relationWaitDeadline(ctx, freshness) + label = s.awaitRelationFreshness(ctx, nil, keys, deadline) + 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 +412,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 +434,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 +443,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, @@ -432,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 { @@ -459,7 +504,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 +532,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 +560,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 +656,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)) - } - if s.isTOON(ctx, req) { - return returnTOON(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) } - - return s.respondJSONOrTOON(ctx, req, result) + return label.decorate(res), err } func impactComplete(impact *analysis.ImpactResult) bool { diff --git a/internal/mcp/worktree_agent_lifecycle_test.go b/internal/mcp/worktree_agent_lifecycle_test.go index 273e26873..76f6d8509 100644 --- a/internal/mcp/worktree_agent_lifecycle_test.go +++ b/internal/mcp/worktree_agent_lifecycle_test.go @@ -63,7 +63,6 @@ func newBlockedLifecycleFixture(t testing.TB) (*realCheckoutMutationFixture, *li // Unblock before the fixture's lifecycle cleanup waits for its real worker. t.Cleanup(e.unblock) f.srv.mutationReindexWait = 20 * time.Millisecond - f.srv.mutationSafetyWait = 20 * time.Millisecond return f, e }