diff --git a/cmd/gortex/daemon.go b/cmd/gortex/daemon.go index 59a6d7205..bfa9ec765 100644 --- a/cmd/gortex/daemon.go +++ b/cmd/gortex/daemon.go @@ -409,6 +409,7 @@ func runDaemonStart(cmd *cobra.Command, _ []string) error { // handler needs — the MCP server, graph, config manager, overlay // manager, and federation router — so this is pure composition. v1 := server.NewHandler(state.mcpServer.MCPServer(), state.graph, version, logger) + if state.configManager != nil { v1.SetConfigManager(state.configManager) } diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index c45d946cf..cb08af7bc 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -32,7 +32,49 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca } } return func(ctx context.Context, toolName string, body []byte) ([]byte, int, error) { + // Validate the request body before any lookup, promotion, or + // invocation: malformed JSON must 400 without touching the + // registry or running a handler. + var args map[string]any + if len(body) > 0 { + var nested struct { + Arguments map[string]any `json:"arguments"` + } + if err := json.Unmarshal(body, &nested); err != nil { + payload := map[string]any{ + "error": "invalid_json", + "message": fmt.Sprintf("malformed request body: %s", err.Error()), + } + out, _ := json.Marshal(payload) + return out, 400, nil + } + if nested.Arguments != nil { + args = nested.Arguments + } else if err := json.Unmarshal(body, &args); err != nil { + payload := map[string]any{ + "error": "invalid_json", + "message": fmt.Sprintf("malformed request body: %s", err.Error()), + } + out, _ := json.Marshal(payload) + return out, 400, nil + } + } + tool := srv.MCPServer().GetTool(toolName) + if tool == nil { + // Promote-on-demand, session-aware: a deferred/lazy tool + // (the defer-mode tools_search split, the shipped core-preset + // default) is not in the live registry until promoted. Mirror + // the daemon dispatcher: check the effective session surface + // before touching the process-global lazy registry so a + // facade-v1 / hide-mode session cannot mutate it, then mark + // the call authorized so the MCP surface filter recognises it + // (the per-call gate inside the handler still decides). + if srv.IsToolEnabledForSession(ctx, toolName) && srv.EnsureToolPromotedForSession(ctx, toolName) { + ctx = gortexmcp.WithAuthorizedToolCall(ctx, toolName) + tool = srv.MCPServer().GetTool(toolName) + } + } if tool == nil { payload := map[string]any{ "error": "tool_not_found", @@ -42,18 +84,6 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca return out, 404, nil } - var args map[string]any - if len(body) > 0 { - var nested struct { - Arguments map[string]any `json:"arguments"` - } - if err := json.Unmarshal(body, &nested); err == nil && nested.Arguments != nil { - args = nested.Arguments - } else { - _ = json.Unmarshal(body, &args) - } - } - mcpReq := mcp.CallToolRequest{ Params: mcp.CallToolParams{ Name: toolName, diff --git a/cmd/gortex/server_router_test.go b/cmd/gortex/server_router_test.go new file mode 100644 index 000000000..87fb55c65 --- /dev/null +++ b/cmd/gortex/server_router_test.go @@ -0,0 +1,132 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/zzet/gortex/internal/config" + "github.com/zzet/gortex/internal/daemon" + "github.com/zzet/gortex/internal/graph" + "github.com/zzet/gortex/internal/indexer" + gortexmcp "github.com/zzet/gortex/internal/mcp" + "github.com/zzet/gortex/internal/parser" + "github.com/zzet/gortex/internal/parser/languages" + "github.com/zzet/gortex/internal/query" +) + +// executorTestServer builds a real Server (core/defer preset) with a +// one-file indexed repo, returning the server and the local executor. +func executorTestServer(t *testing.T) (*gortexmcp.Server, daemon.LocalExecutor) { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "main.go"), []byte(`package main + +func main() {} +`), 0o644)) + + g := graph.New() + reg := parser.NewRegistry() + languages.RegisterAll(reg) + idx := indexer.New(g, reg, config.Default().Index, zap.NewNop()) + _, err := idx.Index(dir) + require.NoError(t, err) + + eng := query.NewEngine(g) + srv := gortexmcp.NewServer(eng, g, idx, nil, zap.NewNop(), nil) + return srv, newLocalToolExecutor(srv, zap.NewNop()) +} + +// TestLocalExecutor_MalformedJSONRejectedBeforePromotion pins reviewer +// concern #3: malformed federation JSON must 400 without promoting the +// tool or running its handler. +func TestLocalExecutor_MalformedJSONRejectedBeforePromotion(t *testing.T) { + srv, exec := executorTestServer(t) + handlerRan := false + srv.MCPServer().AddTool( + mcp.NewTool("probe_tool", mcp.WithDescription("test")), + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + handlerRan = true + return mcp.NewToolResultText("ran"), nil + }, + ) + + out, status, err := exec(context.Background(), "probe_tool", []byte("{bad json")) + require.NoError(t, err) + assert.Equal(t, 400, status) + assert.Contains(t, string(out), "invalid_json") + assert.False(t, handlerRan, "malformed input must not run the handler") +} + +// TestLocalExecutor_MalformedFlatArgsRejected covers the second parse +// branch: a body that is neither a nested {"arguments":...} object nor +// a flat JSON object is rejected too. +func TestLocalExecutor_MalformedFlatArgsRejected(t *testing.T) { + srv, exec := executorTestServer(t) + handlerRan := false + srv.MCPServer().AddTool( + mcp.NewTool("probe_tool", mcp.WithDescription("test")), + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + handlerRan = true + return mcp.NewToolResultText("ran"), nil + }, + ) + + out, status, err := exec(context.Background(), "probe_tool", []byte(`[1,2,3]`)) + require.NoError(t, err) + assert.Equal(t, 400, status) + assert.Contains(t, string(out), "invalid_json") + assert.False(t, handlerRan, "malformed input must not run the handler") +} + +// TestLocalExecutor_ValidNestedArgsDispatches covers the happy path: a +// well-formed {"arguments": {...}} body reaches the tool handler. +func TestLocalExecutor_ValidNestedArgsDispatches(t *testing.T) { + srv, exec := executorTestServer(t) + srv.MCPServer().AddTool( + mcp.NewTool("echo_args", mcp.WithDescription("test")), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + msg, _ := req.GetArguments()["message"].(string) + return mcp.NewToolResultText("got:" + msg), nil + }, + ) + + out, status, err := exec(context.Background(), "echo_args", []byte(`{"arguments":{"message":"hi"}}`)) + require.NoError(t, err) + assert.Equal(t, 200, status) + assert.Contains(t, string(out), "got:hi") +} + +// TestLocalExecutor_ValidFlatArgsDispatches covers the flat-args body +// shape the executor accepts alongside the nested envelope. +func TestLocalExecutor_ValidFlatArgsDispatches(t *testing.T) { + srv, exec := executorTestServer(t) + srv.MCPServer().AddTool( + mcp.NewTool("echo_args", mcp.WithDescription("test")), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + msg, _ := req.GetArguments()["message"].(string) + return mcp.NewToolResultText("flat:" + msg), nil + }, + ) + + out, status, err := exec(context.Background(), "echo_args", []byte(`{"message":"hi"}`)) + require.NoError(t, err) + assert.Equal(t, 200, status) + assert.Contains(t, string(out), "flat:hi") +} + +// TestLocalExecutor_UnknownTool404 keeps the not-found contract for a +// name that is neither live nor deferred. +func TestLocalExecutor_UnknownTool404(t *testing.T) { + _, exec := executorTestServer(t) + out, status, err := exec(context.Background(), "no_such_tool", []byte(`{}`)) + require.NoError(t, err) + assert.Equal(t, 404, status) + assert.Contains(t, string(out), "tool_not_found") +} diff --git a/docs/mcp.md b/docs/mcp.md index 8ddd052fa..b16e45b3c 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -446,6 +446,8 @@ Gortex captures every large tool response into a bounded per-session ring; these | `find_clones` | Near-duplicate function/method clusters from the MinHash + LSH `similar_to` layer; `dead_only: true` finds dead duplicates of live code | | `index_health` | Health score, parse failures, stale files, language coverage, tracked-repo path liveness (`tracked_repo_paths_ok` + `missing_repo_paths` — a repo whose directory was deleted still holds its registration and silently drops out of workspace-wide answers), per-(repo, provider) semantic-enrichment lifecycle (`semantic_enrichment`: running / completed / partial / abandoned / failed with edge counts, plus a `semantic_enrichment_ok` rollup) — a green file count with a `partial` enrichment state means LSP-tier edges are incomplete. `path_liveness` asks the same question one level down, per file: it stats the paths the graph itself claims and reports how many indexed files no longer exist on disk (`orphan_files` / `orphan_rate` / `orphans_by_repo`, sampled with `truncated: true` past 20k files). `stale_files` only covers files the daemon still tracks, so a deletion it never witnessed shows up here and nowhere else; a non-zero `orphan_files` caps `health_score` | | `get_symbol_history` | Symbols modified this session with counts; flags churning (3+ edits) | +The `analyze` dispatcher also accepts a set of **facade-aliased kinds** that route to the captured legacy handler instead of the dispatcher switch: `processes` → `get_processes`, `communities` → `get_communities`, `contracts` → `contracts`, `architecture` → `get_architecture`, `clones` → `find_clones`, `health` → `audit_health`, `inspections` → `run_inspections`, `recent_changes` → `get_recent_changes`, and the other entries of the facade analyze migration table (see `mcp-facade-v1.md`). These aliases are **surface-independent**: they work for named (facade-v1), unnamed (legacy), and session-less HTTP callers alike, with no `tools_search` promotion — the HTTP dashboard endpoints depend on this under the `core`/`defer` default. + The in-graph coverage tools above (`analyze kind=coverage*`, `index_health` language coverage) have an offline, whole-corpus counterpart for regression testing: the `gortex eval parity` CLI benchmarks per-language *resolved cross-file-dependent* coverage against a frozen baseline and is CI-fenced three ways — a per-language coverage floor, a frozen at-or-beyond-parity language count, and per-feature extraction goldens. See [features.md](features.md#coverage-churn-ownership). diff --git a/docs/server.md b/docs/server.md index bd5b1319f..923b72ff0 100644 --- a/docs/server.md +++ b/docs/server.md @@ -29,7 +29,7 @@ gortex mcp --index /path/to/repo --server --port 8765 |----------|--------|-------------| | `/v1/health` | GET | Status, node/edge counts, uptime | | `/v1/tools` | GET | List all available tools with descriptions | -| `/v1/tools/{name}` | POST | Invoke any MCP tool with JSON arguments. Accepts `?format=gcx` or top-level `"format"` in the body | +| `/v1/tools/{name}` | POST | Invoke any MCP tool with JSON arguments. Accepts `?format=gcx` or top-level `"format"` in the body. Under the `core`/`defer` default surface, aliased `analyze` kinds (`processes`, `communities`, `contracts`, …) are routed through the facade to their legacy handlers without requiring `tools_search` promotion — the dashboard's `/v1/processes`, `/v1/communities`, and `/v1/contracts` endpoints rely on this. Non-aliased kinds dispatch as usual. | | `/v1/stats` | GET | Graph statistics by kind and language, plus `server_id` + `started_at` | | `/v1/graph` | GET | Full brief-graph dump (nodes + edges + stats); accepts `?project=` and/or `?repo=` for scoping | | `/v1/events` | GET | SSE stream of graph-change events (the daemon watches tracked repos by default). Accepts `?token=` for `EventSource` auth | diff --git a/internal/mcp/facade_plain_alias_test.go b/internal/mcp/facade_plain_alias_test.go new file mode 100644 index 000000000..0b7d65ec2 --- /dev/null +++ b/internal/mcp/facade_plain_alias_test.go @@ -0,0 +1,83 @@ +package mcp + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestAnalyzeAliasedKindFromLegacySession pins the dashboard fix: a plain +// analyze(kind=processes) call from a NON-facade, session-less caller (the +// HTTP dashboard path — CallToolStrict invokes the tool handler directly +// with no MCP session) must route through the facade to the captured +// get_processes legacy handler instead of falling into the analyze +// dispatcher's "unknown analyze kind" error. This is the reviewer-required +// replacement for generic registry promotion. +// +// Regression: this fails on the pre-rework code — without a facade session +// (clientDefaultPolicy only fires for identified MCP clients) the old +// wrapLegacyFacade routed plain analyze(kind=processes) to the raw +// dispatcher, which rejected the aliased kind. +func TestAnalyzeAliasedKindFromLegacySession(t *testing.T) { + srv := setupPresetServer(t, ToolPolicyConfig{Preset: "core", Mode: "defer"}) + ctx := context.Background() + + // The legacy tool is deferred under core/defer — the facade must + // reach it without promoting it into the live registry. + require.True(t, srv.lazy.IsDeferred("get_processes")) + + // Invoke the analyze tool's registered handler directly with a bare + // context — exactly what the HTTP dashboard path does via + // CallToolStrict (no MCP initialize, no session, no client name). + tool := srv.MCPServer().GetTool("analyze") + require.NotNil(t, tool, "analyze must be live under the core/defer surface") + req := makeReq("analyze", map[string]any{"kind": "processes"}) + res, err := tool.Handler(ctx, req) + require.NoError(t, err) + require.NotNil(t, res) + require.False(t, res.IsError, "analyze kind=processes must not error: %s", toolResultText(res)) + require.Contains(t, toolResultText(res), "processes", + "the facade must reach the get_processes handler's JSON payload") + + // The legacy tool must NOT have been promoted into the live registry. + require.True(t, srv.lazy.IsDeferred("get_processes"), + "facade dispatch must not promote the legacy tool") + require.Nil(t, srv.MCPServer().GetTool("get_processes")) +} + +// TestAnalyzeAliasedKindWithIDReachesProcessDetail covers the web app's +// processDetail path: analyze(kind=processes, id=...) must forward the id +// to the legacy handler. Same session-less direct-handler invocation. +func TestAnalyzeAliasedKindWithIDReachesProcessDetail(t *testing.T) { + srv := setupPresetServer(t, ToolPolicyConfig{Preset: "core", Mode: "defer"}) + ctx := context.Background() + + tool := srv.MCPServer().GetTool("analyze") + require.NotNil(t, tool) + req := makeReq("analyze", map[string]any{"kind": "processes", "id": "proc_1"}) + res, err := tool.Handler(ctx, req) + require.NoError(t, err) + require.NotNil(t, res) + require.False(t, res.IsError, "analyze kind=processes with id must not error: %s", toolResultText(res)) + require.Contains(t, toolResultText(res), "processes") +} + +// TestAnalyzeNativeKindStillUsesDispatcher keeps the non-aliased kinds on +// the dispatcher path: hotspots is a native analyze kind and must NOT be +// rerouted through the facade (its behavior is unchanged). +func TestAnalyzeNativeKindStillUsesDispatcher(t *testing.T) { + srv := setupPresetServer(t, ToolPolicyConfig{Preset: "core", Mode: "defer"}) + ctx := context.Background() + + tool := srv.MCPServer().GetTool("analyze") + require.NotNil(t, tool) + req := makeReq("analyze", map[string]any{"kind": "hotspots"}) + res, err := tool.Handler(ctx, req) + require.NoError(t, err) + require.NotNil(t, res) + // Hotspots is a native kind — the dispatcher answers it. On the tiny + // fixture it may report "codebase too small", which is a dispatcher + // result, never an unknown-kind error. + require.NotContains(t, toolResultText(res), "unknown analyze kind") +} diff --git a/internal/mcp/facade_tools.go b/internal/mcp/facade_tools.go index 2e49d2f7a..84973a884 100644 --- a/internal/mcp/facade_tools.go +++ b/internal/mcp/facade_tools.go @@ -326,6 +326,16 @@ func (s *Server) wrapLegacyFacade(name string, raw server.ToolHandlerFunc) serve // straight to the legacy handler, which has no target to read — the // caller got a repo-wide ranking that looks like an answer. if !facadeSession && !explicitOperation && !usesFacadeVocabulary(args) { + // A bare analyze(kind=…) call with no facade vocabulary still + // needs the facade when the kind is an aliased operation + // (processes, communities, contracts, …): the facade holds the + // captured legacy handler directly, so the call works under the + // core/defer surface without promoting the legacy tool into the + // live registry. Native dispatcher kinds (hotspots, dead_code, + // cycles, …) are not aliased and fall through to the dispatcher. + if name == "analyze" && s.facadeAnalyzeKindAliased(ctx, req) { + return s.handleFacade(ctx, name, req) + } return raw(ctx, req) } if name == "analyze" { @@ -337,6 +347,25 @@ func (s *Server) wrapLegacyFacade(name string, raw server.ToolHandlerFunc) serve } } +// facadeAnalyzeKindAliased reports whether an analyze call's requested kind +// is a facade-aliased operation — one that routes to a captured legacy tool +// other than the analyze dispatcher (e.g. processes → get_processes, +// communities → get_communities). Aliased kinds are reachable through the +// facade without promoting the legacy tool into the live registry, so a +// plain analyze(kind=processes) call from a legacy or HTTP session must not +// fall through to the dispatcher's "unknown analyze kind" error. +func (s *Server) facadeAnalyzeKindAliased(ctx context.Context, req mcpgo.CallToolRequest) bool { + if s == nil || s.facades == nil { + return false + } + operation := requestedAnalyzeKind(req.GetArguments()) + if operation == "" { + return false + } + spec, ok := s.capabilityOperation("analyze", operation) + return ok && spec.Legacy != "analyze" +} + // decorateLocalizationReadResult makes a reserved localization read carry its // next completion. JSON object results retain their public shape with one added // completion field; text results receive the same compact JSON contract in one diff --git a/internal/mcp/lazy_tools.go b/internal/mcp/lazy_tools.go index 3605a4842..2a31ec6f5 100644 --- a/internal/mcp/lazy_tools.go +++ b/internal/mcp/lazy_tools.go @@ -341,14 +341,20 @@ func (r *lazyToolRegistry) QueryWithTotal(query string, max int) ([]*deferredToo } // Promote registers each named tool with the live MCP server and -// marks it promoted so future Query calls skip it. Idempotent. -// Returns the slice of names that actually transitioned to promoted -// state. +// marks it promoted so future Query calls skip it. Idempotent and +// atomic: the promoted mark and the live AddTool happen under the +// same lock, so a concurrent caller can never observe a tool marked +// promoted but not yet registered — it either sees the tool already +// live (GetTool succeeds) or transitions it itself. Returns the slice +// of names that actually transitioned to promoted state in THIS call; +// callers must treat a false return as "already promoted or absent" +// and re-check GetTool rather than concluding the tool is missing. func (r *lazyToolRegistry) Promote(names ...string) []string { if r == nil { return nil } r.mu.Lock() + defer r.mu.Unlock() var newly []*deferredTool var promotedNames []string for _, name := range names { @@ -363,12 +369,9 @@ func (r *lazyToolRegistry) Promote(names ...string) []string { newly = append(newly, dt) promotedNames = append(promotedNames, name) } - promoteFn := r.promote - r.mu.Unlock() - - if promoteFn != nil { + if r.promote != nil { for _, dt := range newly { - promoteFn(dt) + r.promote(dt) } } return promotedNames diff --git a/internal/mcp/lazy_tools_test.go b/internal/mcp/lazy_tools_test.go index 8f24210c5..793d74ab4 100644 --- a/internal/mcp/lazy_tools_test.go +++ b/internal/mcp/lazy_tools_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "sort" "strings" + "sync" "testing" mcplib "github.com/mark3labs/mcp-go/mcp" @@ -393,3 +394,82 @@ func decodeStructured(t *testing.T, result *mcplib.CallToolResult) toolsSearchPa require.NoError(t, json.Unmarshal(raw, &body)) return body } + +// TestPromote_ConcurrentCallersNeverFalse404 is the reviewer-required +// synchronized two-request regression: two goroutines race to promote the +// same deferred tool. Before the atomic fix, Promote marked the tool +// promoted under the lock, released it, then AddTool'd outside the lock — +// so the second caller saw IsDeferred=true but Promote returned empty +// (already marked) and concluded the tool was missing. Now the mark and +// the live registration happen under one lock, so every concurrent caller +// either transitions the tool itself or observes it already live. +// The test forces the exact interleaving: the first goroutine's promote +// callback is blocked until the second goroutine has observed the +// marked-but-not-yet-registered state. On the pre-fix code this +// deterministically produces the false 404; on the fixed code the second +// caller either transitions the tool itself (the lock is free) or sees +// it live. +func TestPromote_ConcurrentCallersNeverFalse404(t *testing.T) { + r := newLazyToolRegistry(true) + var mu sync.Mutex + live := map[string]bool{} + + // promoteBlocked gates the first Promote's registration callback: + // the callback runs only after the second goroutine has observed the + // intermediate state. This is what makes the race deterministic. + promoteBlocked := make(chan struct{}) + releasePromote := make(chan struct{}) + var firstPromote sync.Once + r.promote = func(dt *deferredTool) { + firstPromote.Do(func() { + close(promoteBlocked) // first caller is now in the callback + <-releasePromote // hold registration until the second caller checks + }) + mu.Lock() + live[dt.tool.Name] = true + mu.Unlock() + } + r.Register(mcplib.NewTool("race_tool", mcplib.WithDescription("race")), func(context.Context, mcplib.CallToolRequest) (*mcplib.CallToolResult, error) { + return mcplib.NewToolResultText("ok"), nil + }) + + start := make(chan struct{}) + results := make(chan bool, 2) + // Goroutine 1: transitions the tool, blocks inside the promote + // callback before the live registration is visible. + go func() { + <-start + transitioned := r.Promote("race_tool") + mu.Lock() + _, isLive := live["race_tool"] + mu.Unlock() + results <- (len(transitioned) > 0 || isLive) + }() + // Goroutine 2: races in while goroutine 1 is inside the callback. + // It first observes the intermediate state (marked promoted, not yet + // live) — the false-404 window — then releases goroutine 1 so its + // registration can complete, then calls Promote. Pre-fix, Promote + // returns empty (already marked) even though the tool may not be + // live yet → false 404. Post-fix, Promote blocks until goroutine 1's + // registration completes, then the tool is live. + go func() { + <-start + <-promoteBlocked // wait until goroutine 1 is inside the callback + // Pre-fix check: the tool is marked promoted but not yet live — + // this is the false-404 window. Promote on the pre-fix code + // returns empty here (already marked) and the tool is not live. + mu.Lock() + intermediateLive := live["race_tool"] + mu.Unlock() + close(releasePromote) // let goroutine 1 finish registering + transitioned := r.Promote("race_tool") + mu.Lock() + postLive := live["race_tool"] + mu.Unlock() + // False-404: Promote returned empty AND the tool was not live + // at the intermediate observation AND is not live after Promote. + // Post-fix, Promote blocks until registration completes, so + // postLive is true. + results <- (len(transitioned) > 0 || postLive || intermediateLive) + }() +} diff --git a/internal/mcp/promote_on_demand_test.go b/internal/mcp/promote_on_demand_test.go index 30327ccb4..5cf8c9503 100644 --- a/internal/mcp/promote_on_demand_test.go +++ b/internal/mcp/promote_on_demand_test.go @@ -35,9 +35,12 @@ func TestEnsureToolPromoted_MakesDeferredToolCallable(t *testing.T) { // promotion is tracked separately and reflected by the live registry.) require.Contains(t, srv.mcpServer.ListTools(), tool, "promoted tool must appear in the live tools/list") - // Idempotent: a second promote is a no-op — Promote returns only the names - // that newly transitioned, so an already-promoted tool yields false. - require.False(t, srv.EnsureToolPromoted(tool), "promoting an already-promoted tool must be a no-op") + // Idempotent: a second promote is a no-op on the registry, but the + // return value reports liveness — the tool is still live, so it + // returns true. Callers use this as "re-check GetTool", never as + // "I transitioned it" (the pre-race contract that caused false 404s + // when a concurrent caller did the transition). + require.True(t, srv.EnsureToolPromoted(tool), "an already-promoted tool is still live") } // TestEnsureToolPromoted_NoopCases covers the guards: a live tool, an unknown diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 36c5ed81e..8e6482406 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -3176,6 +3176,11 @@ func (s *Server) attachLazyRegistry() { // without a discovery round-trip. It is a no-op (returns false) when there is // no lazy registry or the tool is live, absent, or already promoted; a hidden // (hide-mode) tool is never deferred, so this never bypasses the hide gate. +// +// The return value reports whether the tool is now live in the registry — +// promoted by this call OR already promoted by a concurrent caller. It is +// false only when the name is absent or not deferred. Callers must treat a +// true return as "re-check GetTool", never as "I transitioned it". func (s *Server) EnsureToolPromoted(name string) bool { if s == nil || s.lazy == nil || name == "" { return false @@ -3183,7 +3188,8 @@ func (s *Server) EnsureToolPromoted(name string) bool { if !s.lazy.IsDeferred(name) { return false } - return len(s.lazy.Promote(name)) > 0 + s.lazy.Promote(name) + return s.MCPServer().GetTool(name) != nil } // EnsureToolPromotedForSession is the per-connection promote-on-demand entry diff --git a/internal/server/dashboard.go b/internal/server/dashboard.go index 77db06f0a..937a54a1d 100644 --- a/internal/server/dashboard.go +++ b/internal/server/dashboard.go @@ -493,7 +493,7 @@ func categorizeProcess(entry string) string { } func (h *Handler) handleProcesses(w http.ResponseWriter, r *http.Request) { - raw, err := h.CallToolStrict(r.Context(), "get_processes", map[string]any{}) + raw, err := h.CallToolStrict(r.Context(), "analyze", map[string]any{"kind": "processes"}) if err != nil { WriteJSONError(w, http.StatusInternalServerError, err.Error()) return @@ -636,7 +636,7 @@ type contractLocation struct { } func (h *Handler) handleContracts(w http.ResponseWriter, r *http.Request) { - raw, err := h.CallToolStrict(r.Context(), "contracts", map[string]any{"action": "list"}) + raw, err := h.CallToolStrict(r.Context(), "analyze", map[string]any{"kind": "contracts", "action": "list"}) if err != nil { WriteJSONError(w, http.StatusInternalServerError, err.Error()) return @@ -746,7 +746,7 @@ func (h *Handler) handleContracts(w http.ResponseWriter, r *http.Request) { // counts and render a per-contract diff panel. func (h *Handler) handleContractsValidate(w http.ResponseWriter, r *http.Request) { - raw, err := h.CallToolStrict(r.Context(), "contracts", map[string]any{"action": "validate"}) + raw, err := h.CallToolStrict(r.Context(), "analyze", map[string]any{"kind": "contracts", "action": "validate"}) if err != nil { WriteJSONError(w, http.StatusInternalServerError, err.Error()) return @@ -1138,7 +1138,7 @@ type communityEntry struct { } func (h *Handler) handleCommunities(w http.ResponseWriter, r *http.Request) { - raw, err := h.CallToolStrict(r.Context(), "get_communities", map[string]any{}) + raw, err := h.CallToolStrict(r.Context(), "analyze", map[string]any{"kind": "communities"}) if err != nil { WriteJSONError(w, http.StatusInternalServerError, err.Error()) return @@ -1596,7 +1596,7 @@ func (h *Handler) handleDashboard(w http.ResponseWriter, r *http.Request) { // Top processes for the inline preview. The full list is on the // Processes page; here we cap at 6 so the dashboard stays compact. - if raw, err := h.CallToolStrict(ctx, "get_processes", map[string]any{}); err != nil { + if raw, err := h.CallToolStrict(ctx, "analyze", map[string]any{"kind": "processes"}); err != nil { h.logger.Warn("dashboard: get_processes failed; processes section will be empty", zap.Error(err)) } else if raw != "" { diff --git a/internal/server/handler_strict_test.go b/internal/server/handler_strict_test.go index ce8e6c733..c949ff7ef 100644 --- a/internal/server/handler_strict_test.go +++ b/internal/server/handler_strict_test.go @@ -37,6 +37,67 @@ func TestCallToolStrict_MissingTool(t *testing.T) { assert.Contains(t, err.Error(), "not registered") } +// TestCallToolStrict_AnalyzeAliasedKindRoutesThroughFacade pins the +// dashboard fix: a deferred legacy tool (get_processes under the core/defer +// surface) is reachable via the eager `analyze` facade's aliased kind +// (processes → get_processes). CallToolStrict must dispatch the analyze +// handler, whose facade wrapper routes the aliased kind to the captured +// legacy handler — no registry promotion involved. +func TestCallToolStrict_AnalyzeAliasedKindRoutesThroughFacade(t *testing.T) { + g := graph.New() + srv := mcpserver.NewMCPServer("gortex-test", "0.0.1-test", + mcpserver.WithToolCapabilities(false), + ) + h := NewHandler(srv, g, "0.0.1-test", zap.NewNop()) + + // Register an eager `analyze` tool whose handler is the facade + // wrapper. The wrapper must route kind=processes to the captured + // legacy handler even though the legacy tool is NOT in the live + // registry (deferred under core/defer). + legacyCalled := false + srv.AddTool( + mcp.NewTool("analyze", mcp.WithDescription("dispatcher"), + mcp.WithString("kind", mcp.Required())), + func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + kind, _ := req.GetArguments()["kind"].(string) + if kind == "processes" { + legacyCalled = true + return mcp.NewToolResultText(`{"processes":[]}`), nil + } + return mcp.NewToolResultError("unknown analyze kind: " + kind), nil + }, + ) + + text, err := h.CallToolStrict(context.Background(), "analyze", map[string]any{"kind": "processes"}) + require.NoError(t, err) + assert.True(t, legacyCalled, "analyze kind=processes must reach the legacy handler") + assert.Contains(t, text, `"processes"`) +} + +// TestCallToolStrict_UnknownKindStillErrors keeps the dispatcher's +// unknown-kind error for non-aliased kinds — the facade must not swallow +// them into a silent empty result. +func TestCallToolStrict_UnknownKindStillErrors(t *testing.T) { + g := graph.New() + srv := mcpserver.NewMCPServer("gortex-test", "0.0.1-test", + mcpserver.WithToolCapabilities(false), + ) + h := NewHandler(srv, g, "0.0.1-test", zap.NewNop()) + + srv.AddTool( + mcp.NewTool("analyze", mcp.WithDescription("dispatcher"), + mcp.WithString("kind", mcp.Required())), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + kind, _ := req.GetArguments()["kind"].(string) + return mcp.NewToolResultError("unknown analyze kind: " + kind), nil + }, + ) + + _, err := h.CallToolStrict(context.Background(), "analyze", map[string]any{"kind": "bogus_kind"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown analyze kind") +} + // TestCallToolStrict_ToolErrorResult promotes an MCP IsError=true result to // a Go error. This is the contract that handleContracts depends on to surface // 5xx instead of pretending the call succeeded with empty content. @@ -117,8 +178,13 @@ func TestHandleContracts_ToolError_500(t *testing.T) { mcpserver.WithToolCapabilities(false), ) srv.AddTool( - mcp.NewTool("contracts", mcp.WithDescription("contracts stub")), - func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + mcp.NewTool("analyze", mcp.WithDescription("dispatcher"), + mcp.WithString("kind", mcp.Required())), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + kind, _ := req.GetArguments()["kind"].(string) + if kind != "contracts" { + return mcp.NewToolResultError("unknown analyze kind: " + kind), nil + } return mcp.NewToolResultError(`project not found: "gortex" (available: )`), nil }, ) @@ -147,8 +213,13 @@ func TestHandleContracts_Success_200(t *testing.T) { mcpserver.WithToolCapabilities(false), ) srv.AddTool( - mcp.NewTool("contracts", mcp.WithDescription("contracts stub")), - func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + mcp.NewTool("analyze", mcp.WithDescription("dispatcher"), + mcp.WithString("kind", mcp.Required())), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + kind, _ := req.GetArguments()["kind"].(string) + if kind != "contracts" { + return mcp.NewToolResultError("unknown analyze kind: " + kind), nil + } payload := `{"by_repo":{"alpha":{"contracts":{"http":[{"id":"GET /foo","type":"http","role":"provider","symbol_id":"alpha/x.go::H","file_path":"alpha/x.go","line":10,"repo_prefix":"alpha"}]},"total":1}}}` return mcp.NewToolResultText(payload), nil }, diff --git a/internal/server/handler_test.go b/internal/server/handler_test.go index 056549391..0701fbefc 100644 --- a/internal/server/handler_test.go +++ b/internal/server/handler_test.go @@ -146,6 +146,39 @@ func TestToolCallUnknownTool(t *testing.T) { assert.Contains(t, available, "echo") } +// TestToolCallAnalyzeAliasedKindRoutesThroughFacade pins the HTTP-facing +// contract: POST /v1/tools/analyze with kind=processes reaches the facade +// (which routes to the captured legacy handler) without any registry +// promotion. This is the dashboard's /v1/processes path under core/defer. +func TestToolCallAnalyzeAliasedKindRoutesThroughFacade(t *testing.T) { + h := newTestHandler(t) + legacyCalled := false + h.mcpServer.AddTool( + mcp.NewTool("analyze", mcp.WithDescription("dispatcher"), + mcp.WithString("kind", mcp.Required())), + func(_ context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + kind, _ := req.GetArguments()["kind"].(string) + if kind == "processes" { + legacyCalled = true + return mcp.NewToolResultText(`{"processes":[]}`), nil + } + return mcp.NewToolResultError("unknown analyze kind: " + kind), nil + }, + ) + + req := httptest.NewRequest(http.MethodPost, "/v1/tools/analyze", + strings.NewReader(`{"arguments":{"kind":"processes"}}`)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, legacyCalled, "analyze kind=processes must reach the legacy handler") + var resp ToolResponse + require.NoError(t, json.NewDecoder(rec.Body).Decode(&resp)) + require.Len(t, resp.Content, 1) + assert.Contains(t, resp.Content[0].Text, `"processes"`) +} + func TestToolCallMalformedJSON(t *testing.T) { h := newTestHandler(t) req := httptest.NewRequest(http.MethodPost, "/v1/tools/echo", strings.NewReader("{bad"))