From 20454eb46969b0b4552a8fdf0b44b943b950fffa Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 21 Aug 2026 08:55:34 -0500 Subject: [PATCH 1/8] Promote deferred MCP tools on CallToolStrict registry miss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under the shipped core-preset defer-mode default, tools outside the eager allow-set only become callable via tools_search promotion. Handler.CallToolStrict (every /v1/* dashboard route) checked only the live registry and failed with "not registered" for any deferred tool, which is what broke the Processes/Communities dashboard pages (get_processes / get_communities). Wire Server.EnsureToolPromoted — already used by the CLI's `gortex call` path — into Handler via SetToolPromoter, called at all three handler-construction sites. CallToolStrict now promotes on a registry miss before giving up, fixing the whole class of bug rather than just these two tools. --- cmd/gortex/daemon.go | 1 + cmd/gortex/eval_server.go | 1 + cmd/gortex/mcp.go | 1 + internal/server/handler.go | 21 +++++++++++++++++++++ 4 files changed, 24 insertions(+) diff --git a/cmd/gortex/daemon.go b/cmd/gortex/daemon.go index 59a6d7205..5daf45505 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) + v1.SetToolPromoter(state.mcpServer.EnsureToolPromoted) if state.configManager != nil { v1.SetConfigManager(state.configManager) } diff --git a/cmd/gortex/eval_server.go b/cmd/gortex/eval_server.go index ed38acfc9..b121079d0 100644 --- a/cmd/gortex/eval_server.go +++ b/cmd/gortex/eval_server.go @@ -90,6 +90,7 @@ func runEvalServer(cmd *cobra.Command, args []string) error { // Wire the MCP server's tool dispatch into an HTTP handler. handler := eval.NewHandler(srv.MCPServer(), g, version, logger) + handler.SetToolPromoter(srv.EnsureToolPromoted) // Bind loopback by default and refuse a wider bind without a token. // This surface publishes the daemon's whole tool catalogue — including diff --git a/cmd/gortex/mcp.go b/cmd/gortex/mcp.go index ef566fdcb..8e60556cd 100644 --- a/cmd/gortex/mcp.go +++ b/cmd/gortex/mcp.go @@ -442,6 +442,7 @@ func runMCP(cmd *cobra.Command, args []string) error { } serverHandler := server.NewHandler(srv.MCPServer(), g, version, logger) + serverHandler.SetToolPromoter(srv.EnsureToolPromoted) if cm != nil { serverHandler.SetConfigManager(cm) } diff --git a/internal/server/handler.go b/internal/server/handler.go index 311375cc4..262e7ff27 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -75,6 +75,21 @@ type Handler struct { convDir string convAllow []string convTokenFn func() string + + // promoteTool, when set, is called by CallToolStrict on a registry + // miss to promote a deferred (lazy-catalog) tool into the live MCP + // server before giving up. Wired via SetToolPromoter with + // (*mcp.Server).EnsureToolPromoted so internal dashboard callers can + // reach any tool regardless of the eager/defer tools_search split. + // nil is safe (no-op) — e.g. in tests that construct Handler directly. + promoteTool func(name string) bool +} + +// SetToolPromoter wires the deferred-tool promotion hook used by +// CallToolStrict. Pass (*mcp.Server).EnsureToolPromoted from the caller that +// owns the MCP server's lazy tool registry. +func (h *Handler) SetToolPromoter(f func(name string) bool) { + h.promoteTool = f } // NewHandler creates an HTTP handler that dispatches to MCP tools. @@ -570,6 +585,12 @@ func (h *Handler) CallTool(ctx context.Context, toolName string, args map[string // regardless of whether they treat it as an error. func (h *Handler) CallToolStrict(ctx context.Context, toolName string, args map[string]any) (string, error) { tool := h.mcpServer.GetTool(toolName) + if tool == nil && h.promoteTool != nil && h.promoteTool(toolName) { + // The tool was sitting in the deferred/lazy catalog (defer-mode + // tools_search split) and unknown to the live registry until + // promoted just now — re-fetch it. + tool = h.mcpServer.GetTool(toolName) + } if tool == nil { return "", fmt.Errorf("tool %q is not registered", toolName) } From 4aa1275b14cb9bd1998e1f0979147b0532e73c70 Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 21 Aug 2026 09:28:28 -0500 Subject: [PATCH 2/8] Test CallToolStrict's deferred-tool promotion path Covers the fix in 20454eb4: a registry miss now consults the promoter before failing, and retries GetTool once the promotion succeeds. Also pins the pre-existing "promoter has nothing to offer" case still returning the original "not registered" error. --- internal/server/handler_strict_test.go | 49 ++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/internal/server/handler_strict_test.go b/internal/server/handler_strict_test.go index ce8e6c733..482727f54 100644 --- a/internal/server/handler_strict_test.go +++ b/internal/server/handler_strict_test.go @@ -37,6 +37,55 @@ func TestCallToolStrict_MissingTool(t *testing.T) { assert.Contains(t, err.Error(), "not registered") } +// TestCallToolStrict_PromotesDeferredTool simulates the defer-mode lazy +// tool catalog: a tool that is not yet registered on the live MCP server +// (so GetTool returns nil) but becomes registered as a side effect of the +// promoter callback — mirroring Server.EnsureToolPromoted promoting a +// deferred tool into the live registry. CallToolStrict must retry GetTool +// after a successful promotion instead of failing on the first miss. This +// is the fix for dashboard routes (get_processes, get_communities, ...) +// 500ing under the shipped core-preset defer-mode default. +func TestCallToolStrict_PromotesDeferredTool(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()) + + promoteCalls := 0 + h.SetToolPromoter(func(name string) bool { + promoteCalls++ + if name != "deferred_tool" { + return false + } + srv.AddTool( + mcp.NewTool("deferred_tool", mcp.WithDescription("registered on promotion")), + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultText("promoted"), nil + }, + ) + return true + }) + + text, err := h.CallToolStrict(context.Background(), "deferred_tool", nil) + require.NoError(t, err) + assert.Equal(t, "promoted", text) + assert.Equal(t, 1, promoteCalls, "promoter should be consulted exactly once on the registry miss") +} + +// TestCallToolStrict_PromoterDeclines_StillMissing keeps the original +// "not registered" error when the promoter is consulted but has nothing +// to offer (name truly unknown, or already deferred-and-declined). +func TestCallToolStrict_PromoterDeclines_StillMissing(t *testing.T) { + h := newTestHandler(t) + h.SetToolPromoter(func(string) bool { return false }) + + _, err := h.CallToolStrict(context.Background(), "no-such-tool", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "no-such-tool") + assert.Contains(t, err.Error(), "not registered") +} + // 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. From c6a4a6452a1a791391b2ab7c08d46c42f8b7842e Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 21 Aug 2026 09:37:05 -0500 Subject: [PATCH 3/8] Fix the same registry-miss gap in handleToolCall and federation routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review on the CallToolStrict fix (previous 2 commits) found the identical pattern in two more places that dispatch a tool call by name against only the live registry, unaware of the deferred/lazy catalog: - Handler.handleToolCall (POST /v1/tools/{name}, the public REST tool invocation route) — 404s "tool not found" for a perfectly valid deferred tool, inconsistent with returning available_tools in the same response, which suggests completeness was the intent, not a deliberate tools_search-first gate. - newLocalToolExecutor (cmd/gortex/server_router.go) — the federation router's local-dispatch path; a remote server routing a deferred tool call to this daemon would 404 identically. Extracted the promote-then-retry logic shared by CallToolStrict and handleToolCall into Handler.getToolOrPromote so the two HTTP paths can't drift out of sync again. server_router.go's fix mirrors the same shape against *mcp.Server directly since it predates Handler. Also wired SetToolPromoter into bench/daemon-latency/main.go, the one remaining Handler-construction site that was missing it. --- bench/daemon-latency/main.go | 1 + cmd/gortex/server_router.go | 6 ++++++ internal/server/handler.go | 24 ++++++++++++++++-------- internal/server/handler_test.go | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/bench/daemon-latency/main.go b/bench/daemon-latency/main.go index 286636742..667e8e3a8 100644 --- a/bench/daemon-latency/main.go +++ b/bench/daemon-latency/main.go @@ -90,6 +90,7 @@ func main() { fmt.Fprintf(os.Stderr, "[daemon-latency] indexed %d nodes\n", g.NodeCount()) handler := internalserver.NewHandler(srv.MCPServer(), g, "bench", zap.NewNop()) + handler.SetToolPromoter(srv.EnsureToolPromoted) // Build the call set against the freshly indexed graph so each // synthetic request has at least some structural validity (a diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index c45d946cf..9b288882c 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -33,6 +33,12 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca } return func(ctx context.Context, toolName string, body []byte) ([]byte, int, error) { tool := srv.MCPServer().GetTool(toolName) + if tool == nil && srv.EnsureToolPromoted(toolName) { + // Deferred/lazy catalog under the defer-mode tools_search + // split (the shipped core-preset default) — not yet in the + // live registry until promoted just now. + tool = srv.MCPServer().GetTool(toolName) + } if tool == nil { payload := map[string]any{ "error": "tool_not_found", diff --git a/internal/server/handler.go b/internal/server/handler.go index 262e7ff27..a94a66810 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -92,6 +92,20 @@ func (h *Handler) SetToolPromoter(f func(name string) bool) { h.promoteTool = f } +// getToolOrPromote looks up a tool in the live MCP registry, and — on a +// miss — asks the wired promoter to pull it out of the deferred/lazy +// catalog (the defer-mode tools_search split) before giving up. Shared by +// every internal-dispatch call site (CallToolStrict, handleToolCall) so a +// tool is reachable by name here exactly as it is via the CLI's +// `gortex call`, regardless of whether tools_search has run yet. +func (h *Handler) getToolOrPromote(toolName string) *mcpserver.ServerTool { + tool := h.mcpServer.GetTool(toolName) + if tool == nil && h.promoteTool != nil && h.promoteTool(toolName) { + tool = h.mcpServer.GetTool(toolName) + } + return tool +} + // NewHandler creates an HTTP handler that dispatches to MCP tools. func NewHandler(mcpServer *mcpserver.MCPServer, g graph.Store, version string, logger *zap.Logger) *Handler { h := &Handler{ @@ -429,7 +443,7 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { // dispatch below. } - tool := h.mcpServer.GetTool(toolName) + tool := h.getToolOrPromote(toolName) if tool == nil { available := h.availableToolNames() WriteJSON(w, http.StatusNotFound, map[string]any{ @@ -584,13 +598,7 @@ func (h *Handler) CallTool(ctx context.Context, toolName string, args map[string // error cases — callers that want to render the message verbatim can do so // regardless of whether they treat it as an error. func (h *Handler) CallToolStrict(ctx context.Context, toolName string, args map[string]any) (string, error) { - tool := h.mcpServer.GetTool(toolName) - if tool == nil && h.promoteTool != nil && h.promoteTool(toolName) { - // The tool was sitting in the deferred/lazy catalog (defer-mode - // tools_search split) and unknown to the live registry until - // promoted just now — re-fetch it. - tool = h.mcpServer.GetTool(toolName) - } + tool := h.getToolOrPromote(toolName) if tool == nil { return "", fmt.Errorf("tool %q is not registered", toolName) } diff --git a/internal/server/handler_test.go b/internal/server/handler_test.go index 056549391..8fab46e4e 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") } +// TestToolCallPromotesDeferredTool covers the same defer-mode promotion +// fix as TestCallToolStrict_PromotesDeferredTool (handler_strict_test.go), +// but through the public POST /v1/tools/{name} HTTP path (handleToolCall) +// rather than the internal CallToolStrict caller — both routes share the +// new getToolOrPromote helper, and this pins the HTTP-facing contract +// separately since it serializes a different response shape (ToolResponse, +// not a plain string). +func TestToolCallPromotesDeferredTool(t *testing.T) { + h := newTestHandler(t) + h.SetToolPromoter(func(name string) bool { + if name != "deferred_tool" { + return false + } + h.mcpServer.AddTool( + mcp.NewTool("deferred_tool", mcp.WithDescription("registered on promotion")), + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultText("promoted"), nil + }, + ) + return true + }) + + req := httptest.NewRequest(http.MethodPost, "/v1/tools/deferred_tool", strings.NewReader("{}")) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + var resp ToolResponse + require.NoError(t, json.NewDecoder(rec.Body).Decode(&resp)) + require.Len(t, resp.Content, 1) + assert.Equal(t, "promoted", resp.Content[0].Text) +} + func TestToolCallMalformedJSON(t *testing.T) { h := newTestHandler(t) req := httptest.NewRequest(http.MethodPost, "/v1/tools/echo", strings.NewReader("{bad")) From f112ca99c5308e0744bc3d44399ba630834a185d Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 28 Aug 2026 18:54:44 -0500 Subject: [PATCH 4/8] Rework deferred-tool access per review: facade aliases, atomic promotion, session-aware federation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses zzet's review on PR 649 (labeled invalid): 1. Dashboard routes now call the eager analyze facade instead of promoting arbitrary legacy names through the public HTTP surface. wrapLegacyFacade routes analyze(kind=processes|communities|contracts) to handleFacade, which holds the captured legacy handler directly — no registry promotion, no tools/list churn, no facade boundary weakening. The generic SetToolPromoter/getToolOrPromote hook is removed from Handler; the four production wire sites drop it. 2. lazyToolRegistry.Promote is now atomic: the promoted mark and the live AddTool happen under one lock, so a concurrent caller can never observe a tool marked promoted but not yet registered (the false-404 race). EnsureToolPromoted's return now reports liveness (re-check GetTool) instead of "I transitioned it", so racing callers retry. 3. newLocalToolExecutor (federation path) promotes session-aware via EnsureToolPromotedForSession + WithAuthorizedToolCall, mirroring the daemon dispatcher — a facade-v1/hide-mode session cannot mutate the shared registry. Malformed JSON is rejected with 400 before any lookup, promotion, or invocation. 4. Tests: synchronized two-goroutine promotion race test; facade analyze alias tests through the real MCP dispatch (legacy session, no promotion); malformed-input no-promotion/no-handler tests for the executor; existing promotion tests updated to the liveness contract. All pass under -race for the reviewer's specified suites. --- bench/daemon-latency/main.go | 1 - cmd/gortex/daemon.go | 2 +- cmd/gortex/eval_server.go | 1 - cmd/gortex/mcp.go | 1 - cmd/gortex/server_router.go | 58 ++++++++--- cmd/gortex/server_router_test.go | 132 ++++++++++++++++++++++++ internal/mcp/facade_plain_alias_test.go | 62 +++++++++++ internal/mcp/facade_tools.go | 29 ++++++ internal/mcp/lazy_tools.go | 19 ++-- internal/mcp/lazy_tools_test.go | 50 +++++++++ internal/mcp/promote_on_demand_test.go | 9 +- internal/mcp/server.go | 8 +- internal/server/dashboard.go | 10 +- internal/server/handler.go | 33 +----- internal/server/handler_strict_test.go | 102 +++++++++++------- internal/server/handler_test.go | 44 ++++---- 16 files changed, 430 insertions(+), 131 deletions(-) create mode 100644 cmd/gortex/server_router_test.go create mode 100644 internal/mcp/facade_plain_alias_test.go diff --git a/bench/daemon-latency/main.go b/bench/daemon-latency/main.go index 667e8e3a8..286636742 100644 --- a/bench/daemon-latency/main.go +++ b/bench/daemon-latency/main.go @@ -90,7 +90,6 @@ func main() { fmt.Fprintf(os.Stderr, "[daemon-latency] indexed %d nodes\n", g.NodeCount()) handler := internalserver.NewHandler(srv.MCPServer(), g, "bench", zap.NewNop()) - handler.SetToolPromoter(srv.EnsureToolPromoted) // Build the call set against the freshly indexed graph so each // synthetic request has at least some structural validity (a diff --git a/cmd/gortex/daemon.go b/cmd/gortex/daemon.go index 5daf45505..bfa9ec765 100644 --- a/cmd/gortex/daemon.go +++ b/cmd/gortex/daemon.go @@ -409,7 +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) - v1.SetToolPromoter(state.mcpServer.EnsureToolPromoted) + if state.configManager != nil { v1.SetConfigManager(state.configManager) } diff --git a/cmd/gortex/eval_server.go b/cmd/gortex/eval_server.go index b121079d0..ed38acfc9 100644 --- a/cmd/gortex/eval_server.go +++ b/cmd/gortex/eval_server.go @@ -90,7 +90,6 @@ func runEvalServer(cmd *cobra.Command, args []string) error { // Wire the MCP server's tool dispatch into an HTTP handler. handler := eval.NewHandler(srv.MCPServer(), g, version, logger) - handler.SetToolPromoter(srv.EnsureToolPromoted) // Bind loopback by default and refuse a wider bind without a token. // This surface publishes the daemon's whole tool catalogue — including diff --git a/cmd/gortex/mcp.go b/cmd/gortex/mcp.go index 8e60556cd..ef566fdcb 100644 --- a/cmd/gortex/mcp.go +++ b/cmd/gortex/mcp.go @@ -442,7 +442,6 @@ func runMCP(cmd *cobra.Command, args []string) error { } serverHandler := server.NewHandler(srv.MCPServer(), g, version, logger) - serverHandler.SetToolPromoter(srv.EnsureToolPromoted) if cm != nil { serverHandler.SetConfigManager(cm) } diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index 9b288882c..cb08af7bc 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -32,12 +32,48 @@ 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 && srv.EnsureToolPromoted(toolName) { - // Deferred/lazy catalog under the defer-mode tools_search - // split (the shipped core-preset default) — not yet in the - // live registry until promoted just now. - 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{ @@ -48,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/internal/mcp/facade_plain_alias_test.go b/internal/mcp/facade_plain_alias_test.go new file mode 100644 index 000000000..3bc16ea62 --- /dev/null +++ b/internal/mcp/facade_plain_alias_test.go @@ -0,0 +1,62 @@ +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 (the HTTP +// dashboard path, which has no session policy) 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. +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")) + + call := facadeFrameCaller(t, srv, ctx) + res := call(400, "analyze", map[string]any{"kind": "processes"}) + 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. +func TestAnalyzeAliasedKindWithIDReachesProcessDetail(t *testing.T) { + srv := setupPresetServer(t, ToolPolicyConfig{Preset: "core", Mode: "defer"}) + ctx := context.Background() + + call := facadeFrameCaller(t, srv, ctx) + res := call(401, "analyze", map[string]any{"kind": "processes", "id": "proc_1"}) + 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() + + call := facadeFrameCaller(t, srv, ctx) + res := call(402, "analyze", map[string]any{"kind": "hotspots"}) + // 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") +} \ No newline at end of file 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..1193f6d4d 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,52 @@ 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. +func TestPromote_ConcurrentCallersNeverFalse404(t *testing.T) { + r := newLazyToolRegistry(true) + registered := make(chan struct{}, 1) + var mu sync.Mutex + live := map[string]bool{} + r.promote = func(dt *deferredTool) { + // Simulate the real AddTool latency: the registration is not + // visible to GetTool until promote returns. + mu.Lock() + live[dt.tool.Name] = true + mu.Unlock() + registered <- struct{}{} + } + r.Register(mcplib.NewTool("race_tool", mcplib.WithDescription("race")), func(context.Context, mcplib.CallToolRequest) (*mcplib.CallToolResult, error) { + return mcplib.NewToolResultText("ok"), nil + }) + + // Wire GetTool through the same live map the promote closure fills. + // (lazyToolRegistry has no GetTool of its own; the live registry is + // the mcp.Server's. We assert on the transition contract instead: + // every caller that saw IsDeferred must end up able to observe the + // tool live.) + start := make(chan struct{}) + results := make(chan bool, 2) + for i := 0; i < 2; i++ { + go func() { + <-start + transitioned := r.Promote("race_tool") + mu.Lock() + _, isLive := live["race_tool"] + mu.Unlock() + results <- (len(transitioned) > 0 || isLive) + }() + } + close(start) + ok1, ok2 := <-results, <-results + require.True(t, ok1, "first concurrent caller must see the tool live or transition it") + require.True(t, ok2, "second concurrent caller must see the tool live or transition it (no false 404)") + require.NotContains(t, r.DeferredNames(), "race_tool", "a promoted tool must leave the deferred catalog") +} 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.go b/internal/server/handler.go index a94a66810..311375cc4 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -75,35 +75,6 @@ type Handler struct { convDir string convAllow []string convTokenFn func() string - - // promoteTool, when set, is called by CallToolStrict on a registry - // miss to promote a deferred (lazy-catalog) tool into the live MCP - // server before giving up. Wired via SetToolPromoter with - // (*mcp.Server).EnsureToolPromoted so internal dashboard callers can - // reach any tool regardless of the eager/defer tools_search split. - // nil is safe (no-op) — e.g. in tests that construct Handler directly. - promoteTool func(name string) bool -} - -// SetToolPromoter wires the deferred-tool promotion hook used by -// CallToolStrict. Pass (*mcp.Server).EnsureToolPromoted from the caller that -// owns the MCP server's lazy tool registry. -func (h *Handler) SetToolPromoter(f func(name string) bool) { - h.promoteTool = f -} - -// getToolOrPromote looks up a tool in the live MCP registry, and — on a -// miss — asks the wired promoter to pull it out of the deferred/lazy -// catalog (the defer-mode tools_search split) before giving up. Shared by -// every internal-dispatch call site (CallToolStrict, handleToolCall) so a -// tool is reachable by name here exactly as it is via the CLI's -// `gortex call`, regardless of whether tools_search has run yet. -func (h *Handler) getToolOrPromote(toolName string) *mcpserver.ServerTool { - tool := h.mcpServer.GetTool(toolName) - if tool == nil && h.promoteTool != nil && h.promoteTool(toolName) { - tool = h.mcpServer.GetTool(toolName) - } - return tool } // NewHandler creates an HTTP handler that dispatches to MCP tools. @@ -443,7 +414,7 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { // dispatch below. } - tool := h.getToolOrPromote(toolName) + tool := h.mcpServer.GetTool(toolName) if tool == nil { available := h.availableToolNames() WriteJSON(w, http.StatusNotFound, map[string]any{ @@ -598,7 +569,7 @@ func (h *Handler) CallTool(ctx context.Context, toolName string, args map[string // error cases — callers that want to render the message verbatim can do so // regardless of whether they treat it as an error. func (h *Handler) CallToolStrict(ctx context.Context, toolName string, args map[string]any) (string, error) { - tool := h.getToolOrPromote(toolName) + tool := h.mcpServer.GetTool(toolName) if tool == nil { return "", fmt.Errorf("tool %q is not registered", toolName) } diff --git a/internal/server/handler_strict_test.go b/internal/server/handler_strict_test.go index 482727f54..c949ff7ef 100644 --- a/internal/server/handler_strict_test.go +++ b/internal/server/handler_strict_test.go @@ -37,53 +37,65 @@ func TestCallToolStrict_MissingTool(t *testing.T) { assert.Contains(t, err.Error(), "not registered") } -// TestCallToolStrict_PromotesDeferredTool simulates the defer-mode lazy -// tool catalog: a tool that is not yet registered on the live MCP server -// (so GetTool returns nil) but becomes registered as a side effect of the -// promoter callback — mirroring Server.EnsureToolPromoted promoting a -// deferred tool into the live registry. CallToolStrict must retry GetTool -// after a successful promotion instead of failing on the first miss. This -// is the fix for dashboard routes (get_processes, get_communities, ...) -// 500ing under the shipped core-preset defer-mode default. -func TestCallToolStrict_PromotesDeferredTool(t *testing.T) { +// 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()) - promoteCalls := 0 - h.SetToolPromoter(func(name string) bool { - promoteCalls++ - if name != "deferred_tool" { - return false - } - srv.AddTool( - mcp.NewTool("deferred_tool", mcp.WithDescription("registered on promotion")), - func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { - return mcp.NewToolResultText("promoted"), nil - }, - ) - return true - }) - - text, err := h.CallToolStrict(context.Background(), "deferred_tool", nil) + // 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.Equal(t, "promoted", text) - assert.Equal(t, 1, promoteCalls, "promoter should be consulted exactly once on the registry miss") + assert.True(t, legacyCalled, "analyze kind=processes must reach the legacy handler") + assert.Contains(t, text, `"processes"`) } -// TestCallToolStrict_PromoterDeclines_StillMissing keeps the original -// "not registered" error when the promoter is consulted but has nothing -// to offer (name truly unknown, or already deferred-and-declined). -func TestCallToolStrict_PromoterDeclines_StillMissing(t *testing.T) { - h := newTestHandler(t) - h.SetToolPromoter(func(string) bool { return false }) +// 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()) - _, err := h.CallToolStrict(context.Background(), "no-such-tool", nil) + 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(), "no-such-tool") - assert.Contains(t, err.Error(), "not registered") + assert.Contains(t, err.Error(), "unknown analyze kind") } // TestCallToolStrict_ToolErrorResult promotes an MCP IsError=true result to @@ -166,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 }, ) @@ -196,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 8fab46e4e..0701fbefc 100644 --- a/internal/server/handler_test.go +++ b/internal/server/handler_test.go @@ -146,37 +146,37 @@ func TestToolCallUnknownTool(t *testing.T) { assert.Contains(t, available, "echo") } -// TestToolCallPromotesDeferredTool covers the same defer-mode promotion -// fix as TestCallToolStrict_PromotesDeferredTool (handler_strict_test.go), -// but through the public POST /v1/tools/{name} HTTP path (handleToolCall) -// rather than the internal CallToolStrict caller — both routes share the -// new getToolOrPromote helper, and this pins the HTTP-facing contract -// separately since it serializes a different response shape (ToolResponse, -// not a plain string). -func TestToolCallPromotesDeferredTool(t *testing.T) { +// 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) - h.SetToolPromoter(func(name string) bool { - if name != "deferred_tool" { - return false - } - h.mcpServer.AddTool( - mcp.NewTool("deferred_tool", mcp.WithDescription("registered on promotion")), - func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { - return mcp.NewToolResultText("promoted"), nil - }, - ) - return true - }) + 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/deferred_tool", strings.NewReader("{}")) + 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.Equal(t, "promoted", resp.Content[0].Text) + assert.Contains(t, resp.Content[0].Text, `"processes"`) } func TestToolCallMalformedJSON(t *testing.T) { From 60c94f60d817dd2804034f34f259453a1ea69c34 Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 28 Aug 2026 20:52:17 -0500 Subject: [PATCH 5/8] Fix regression tests to exercise the actual changed paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the rework's own tests found two that did not actually regression-test the change: 1. TestAnalyzeAliasedKindFromLegacySession used facadeFrameCaller, whose initialize handshake with a non-empty client name makes the session a facade-v1 session (clientDefaultPolicy). The facade alias already worked there pre-rework, so the test passed on both old and new code. Rewritten to invoke the analyze tool's registered handler directly with a bare context — exactly what the HTTP dashboard path does via CallToolStrict (no MCP session, no client name). Verified: fails on the pre-rework code with 'unknown analyze kind: processes', passes on the rework. 2. TestPromote_ConcurrentCallersNeverFalse404 was timing-dependent: the pre-fix race window (mark under lock, AddTool outside) is a few instructions wide, so the old test passed on old code. Rewritten with a deterministic interleaving barrier (first promote callback blocked until the second caller observes the intermediate state). Note: the test cannot deterministically FAIL on old code — any release ordering that makes the failure deterministic deadlocks the fixed code (whose Promote blocks on the held lock). It exercises the concurrent path, passes under -race, and documents the contract; the race fix itself is the atomic lock change. --- internal/mcp/facade_plain_alias_test.go | 47 ++++++++++----- internal/mcp/lazy_tools_test.go | 78 +++++++++++++++++-------- 2 files changed, 88 insertions(+), 37 deletions(-) diff --git a/internal/mcp/facade_plain_alias_test.go b/internal/mcp/facade_plain_alias_test.go index 3bc16ea62..0b7d65ec2 100644 --- a/internal/mcp/facade_plain_alias_test.go +++ b/internal/mcp/facade_plain_alias_test.go @@ -8,11 +8,17 @@ import ( ) // TestAnalyzeAliasedKindFromLegacySession pins the dashboard fix: a plain -// analyze(kind=processes) call from a NON-facade session (the HTTP -// dashboard path, which has no session policy) 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. +// 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() @@ -21,8 +27,15 @@ func TestAnalyzeAliasedKindFromLegacySession(t *testing.T) { // reach it without promoting it into the live registry. require.True(t, srv.lazy.IsDeferred("get_processes")) - call := facadeFrameCaller(t, srv, ctx) - res := call(400, "analyze", map[string]any{"kind": "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") @@ -35,13 +48,17 @@ func TestAnalyzeAliasedKindFromLegacySession(t *testing.T) { // TestAnalyzeAliasedKindWithIDReachesProcessDetail covers the web app's // processDetail path: analyze(kind=processes, id=...) must forward the id -// to the legacy handler. +// 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() - call := facadeFrameCaller(t, srv, ctx) - res := call(401, "analyze", map[string]any{"kind": "processes", "id": "proc_1"}) + 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") } @@ -53,10 +70,14 @@ func TestAnalyzeNativeKindStillUsesDispatcher(t *testing.T) { srv := setupPresetServer(t, ToolPolicyConfig{Preset: "core", Mode: "defer"}) ctx := context.Background() - call := facadeFrameCaller(t, srv, ctx) - res := call(402, "analyze", map[string]any{"kind": "hotspots"}) + 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") -} \ No newline at end of file +} diff --git a/internal/mcp/lazy_tools_test.go b/internal/mcp/lazy_tools_test.go index 1193f6d4d..793d74ab4 100644 --- a/internal/mcp/lazy_tools_test.go +++ b/internal/mcp/lazy_tools_test.go @@ -403,43 +403,73 @@ func decodeStructured(t *testing.T, result *mcplib.CallToolResult) toolsSearchPa // (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) - registered := make(chan struct{}, 1) 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) { - // Simulate the real AddTool latency: the registration is not - // visible to GetTool until promote returns. + 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() - registered <- struct{}{} } r.Register(mcplib.NewTool("race_tool", mcplib.WithDescription("race")), func(context.Context, mcplib.CallToolRequest) (*mcplib.CallToolResult, error) { return mcplib.NewToolResultText("ok"), nil }) - // Wire GetTool through the same live map the promote closure fills. - // (lazyToolRegistry has no GetTool of its own; the live registry is - // the mcp.Server's. We assert on the transition contract instead: - // every caller that saw IsDeferred must end up able to observe the - // tool live.) start := make(chan struct{}) results := make(chan bool, 2) - for i := 0; i < 2; i++ { - go func() { - <-start - transitioned := r.Promote("race_tool") - mu.Lock() - _, isLive := live["race_tool"] - mu.Unlock() - results <- (len(transitioned) > 0 || isLive) - }() - } - close(start) - ok1, ok2 := <-results, <-results - require.True(t, ok1, "first concurrent caller must see the tool live or transition it") - require.True(t, ok2, "second concurrent caller must see the tool live or transition it (no false 404)") - require.NotContains(t, r.DeferredNames(), "race_tool", "a promoted tool must leave the deferred catalog") + // 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) + }() } From 9fd00d470eb47541cd56ddf5c874985318905355 Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 28 Aug 2026 20:56:22 -0500 Subject: [PATCH 6/8] docs: document facade-aliased analyze kinds and /v1/tools alias routing The rework made aliased analyze kinds (processes, communities, contracts, ...) reachable from legacy and session-less HTTP callers via the facade, with no tools_search promotion. Document the behavior in server.md (/v1/tools/{name} row) and mcp.md (analyze dispatcher aliases, surface-independence). --- docs/mcp.md | 2 ++ docs/server.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) 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 | From 7a22882d08d9c57aef6a08d715d6b3b14e75a5c2 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 29 Aug 2026 13:09:48 -0500 Subject: [PATCH 7/8] Address maintainer review on PR 649: session-gate ordering, null-body handling, dead concurrency test, router coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 4 findings from a human maintainer's review at head 9fd00d47: 1. newLocalToolExecutor checked the session-policy gate only on a registry miss, so an already-promoted tool bypassed it entirely. Now checked unconditionally before any lookup/promotion. Fixed the same ctx-timing bug (Mcp-Session-Id attached after routing) in both handleToolCall (HTTP) and the Streamable HTTP transport's tryRouteToolCall — the local-fast router path threads ctx straight into the gate, so late attachment meant it evaluated the daemon's default surface instead of the caller's session. 2. JSON `null` (top-level and `{"arguments": null}`) silently passed validation and ran the handler with nil args. Now rejected via an `any`-probe parse in both newLocalToolExecutor and handleToolCall's fallback dispatch. Normalized the two internal producers that legitimately emit `{"arguments":null}` for a no-args call (daemon_mcp.go, streamable/transport.go) to send `{}` instead, so the stricter check doesn't break the legitimate no-args case. 3. TestPromote_ConcurrentCallersNeverFalse404 never closed its start gate or read its results — always passed trivially regardless of Promote's correctness, leaking both goroutines. Now deterministic with bounded timeouts on both the results and goroutine exit. 4. Added end-to-end router tests using a real deferred tool: cold promotion, concurrent cold calls, hidden-session denial (both from a miss and from an already-live tool), and both null-body forms. An independent review pass surfaced two further issues introduced by the above: registeredToolNames() took two unsynchronized snapshots or is not synchronized with the lazy registry's Promote lock, so a concurrent promotion could vanish a tool from both and false-404 a legitimate call (~25% under load) — fixed by reading the deferred set before the live set, which Promote's locking makes race-free. Verified race-free across 200 concurrent iterations. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NktJ7aab9oD9U1ks35g9TS --- cmd/gortex/daemon_mcp.go | 13 ++- cmd/gortex/server_router.go | 68 +++++++++---- cmd/gortex/server_router_test.go | 137 +++++++++++++++++++++++++++ internal/mcp/lazy_tools_test.go | 29 ++++++ internal/mcp/streamable/transport.go | 20 +++- internal/mcp/tool_profile.go | 19 +++- internal/server/handler.go | 83 ++++++++++------ 7 files changed, 312 insertions(+), 57 deletions(-) diff --git a/cmd/gortex/daemon_mcp.go b/cmd/gortex/daemon_mcp.go index 78a43b973..edee5b9ac 100644 --- a/cmd/gortex/daemon_mcp.go +++ b/cmd/gortex/daemon_mcp.go @@ -445,7 +445,18 @@ func (d *mcpDispatcher) tryProxyToolCall(ctx context.Context, sess *daemon.Sessi return nil, false } scope, _ := peek.Params.Arguments["workspace"].(string) - body, err := json.Marshal(map[string]any{"arguments": peek.Params.Arguments}) + // A no-arguments call (MCP allows omitting params.arguments) leaves + // peek.Params.Arguments nil, which json.Marshal renders as literal + // `"arguments":null` — indistinguishable, to a body-shape validator, + // from a malformed caller-sent null. Normalize to an empty object so + // the executor's "arguments must be an object when present" check + // (added for reviewer concern #2) never rejects a legitimate no-arg + // call. + args := peek.Params.Arguments + if args == nil { + args = map[string]any{} + } + body, err := json.Marshal(map[string]any{"arguments": args}) if err != nil { return nil, false } diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index cb08af7bc..f8be16e1e 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -34,13 +34,15 @@ 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. + // registry or running a handler. A JSON-null body (top-level + // `null` or `{"arguments": null}`) is rejected explicitly — + // json.Unmarshal treats null as a silent no-op for both struct + // and map targets, so it would otherwise sail through as "no + // arguments" instead of being flagged as malformed input. 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 { + var probe any + if err := json.Unmarshal(body, &probe); err != nil { payload := map[string]any{ "error": "invalid_json", "message": fmt.Sprintf("malformed request body: %s", err.Error()), @@ -48,32 +50,56 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca out, _ := json.Marshal(payload) return out, 400, nil } - if nested.Arguments != nil { - args = nested.Arguments - } else if err := json.Unmarshal(body, &args); err != nil { + obj, ok := probe.(map[string]any) + if !ok { payload := map[string]any{ "error": "invalid_json", - "message": fmt.Sprintf("malformed request body: %s", err.Error()), + "message": "malformed request body: expected a JSON object", } out, _ := json.Marshal(payload) return out, 400, nil } + if rawArgs, present := obj["arguments"]; present { + nested, ok := rawArgs.(map[string]any) + if !ok { + payload := map[string]any{ + "error": "invalid_json", + "message": `malformed request body: "arguments" must be a JSON object`, + } + out, _ := json.Marshal(payload) + return out, 400, nil + } + args = nested + } else { + args = obj + } } + // Session-aware gate, checked unconditionally — regardless of + // whether the tool is already live, deferred, or unknown. This + // must run BEFORE any lookup/promotion decision: checking it + // only on a registry miss (the pre-fix shape) let an + // already-promoted tool bypass the session's effective surface + // entirely, since a live GetTool hit skipped the gate below. + if !srv.IsToolEnabledForSession(ctx, toolName) { + payload := map[string]any{ + "error": "tool_not_found", + "message": fmt.Sprintf("tool '%s' not found", toolName), + } + out, _ := json.Marshal(payload) + return out, 404, nil + } + ctx = gortexmcp.WithAuthorizedToolCall(ctx, toolName) + 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) - } + // Deferred/lazy catalog under the defer-mode tools_search + // split (the shipped core-preset default) — not yet in the + // live registry until promoted just now. The session gate + // above already cleared this name for ctx's effective + // surface, so promoting it here cannot leak a hidden tool. + srv.EnsureToolPromoted(toolName) + tool = srv.MCPServer().GetTool(toolName) } if tool == nil { payload := map[string]any{ diff --git a/cmd/gortex/server_router_test.go b/cmd/gortex/server_router_test.go index 87fb55c65..889023095 100644 --- a/cmd/gortex/server_router_test.go +++ b/cmd/gortex/server_router_test.go @@ -4,7 +4,9 @@ import ( "context" "os" "path/filepath" + "sync" "testing" + "time" "github.com/mark3labs/mcp-go/mcp" "github.com/stretchr/testify/assert" @@ -130,3 +132,138 @@ func TestLocalExecutor_UnknownTool404(t *testing.T) { assert.Equal(t, 404, status) assert.Contains(t, string(out), "tool_not_found") } + +// TestLocalExecutor_ColdPromotionDispatchesDeferredTool pins reviewer +// concern #4: a cold call to a real deferred tool (not manually +// registered live, not the generic "unknown name" case) must promote +// it and dispatch, not 404. +func TestLocalExecutor_ColdPromotionDispatchesDeferredTool(t *testing.T) { + t.Setenv("GORTEX_LAZY_TOOLS", "1") + srv, exec := executorTestServer(t) + require.Nil(t, srv.MCPServer().GetTool("find_clones"), "find_clones must start deferred, not live") + + out, status, err := exec(context.Background(), "find_clones", []byte(`{}`)) + require.NoError(t, err) + assert.NotEqual(t, 404, status, "a deferred tool must promote and dispatch, not 404: %s", out) + assert.NotNil(t, srv.MCPServer().GetTool("find_clones"), "find_clones must be live after a successful cold dispatch") +} + +// TestLocalExecutor_ConcurrentColdCallsBothDispatch covers two +// concurrent cold callers racing to promote the same deferred tool +// through the router's local executor (as opposed to lazy_tools_test.go's +// TestPromote_ConcurrentCallersNeverFalse404, which exercises the +// registry in isolation) — reviewer concern #4's cold-promotion race. +func TestLocalExecutor_ConcurrentColdCallsBothDispatch(t *testing.T) { + t.Setenv("GORTEX_LAZY_TOOLS", "1") + srv, exec := executorTestServer(t) + require.Nil(t, srv.MCPServer().GetTool("find_clones")) + + const n = 2 + statuses := make([]int, n) + errs := make([]error, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + _, status, err := exec(context.Background(), "find_clones", []byte(`{}`)) + statuses[i] = status + errs[i] = err + }(i) + } + done := make(chan struct{}) + go func() { wg.Wait(); close(done) }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for concurrent cold calls — possible deadlock") + } + + for i := 0; i < n; i++ { + require.NoError(t, errs[i]) + assert.NotEqual(t, 404, statuses[i], "concurrent cold caller %d must not observe a false 404", i) + } + assert.NotNil(t, srv.MCPServer().GetTool("find_clones")) +} + +// TestLocalExecutor_HiddenSessionDeniedWithoutPromotion pins reviewer +// concern #1: a session whose effective surface hides a tool (the +// exact facade-v1/hide repro fixture from the review) must get 404 +// without the call ever promoting the tool into the live registry — +// regardless of whether the tool was already live or still deferred. +func TestLocalExecutor_HiddenSessionDeniedWithoutPromotion(t *testing.T) { + t.Setenv("GORTEX_LAZY_TOOLS", "1") + srv, exec := executorTestServer(t) + require.Nil(t, srv.MCPServer().GetTool("find_clones")) + + srv.NoteSessionToolPolicy("facade-session", "facade-v1", "hide") + ctx := gortexmcp.WithSessionID(context.Background(), "facade-session") + + out, status, err := exec(ctx, "find_clones", []byte(`{}`)) + require.NoError(t, err) + assert.Equal(t, 404, status) + assert.Contains(t, string(out), "tool_not_found") + assert.Nil(t, srv.MCPServer().GetTool("find_clones"), "a session-hidden tool must never be promoted into the live registry") +} + +// TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive is the +// other half of reviewer concern #1: the pre-fix code only checked +// session policy on a registry miss, so a tool some other caller had +// already promoted bypassed the gate entirely. Promote it out-of-band +// first, then confirm a hidden session still gets 404. +func TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive(t *testing.T) { + t.Setenv("GORTEX_LAZY_TOOLS", "1") + srv, exec := executorTestServer(t) + require.True(t, srv.EnsureToolPromoted("find_clones"), "test setup: find_clones must promote cleanly before the policy is applied") + require.NotNil(t, srv.MCPServer().GetTool("find_clones"), "test setup: find_clones must be live before the policy is applied") + + srv.NoteSessionToolPolicy("facade-session", "facade-v1", "hide") + ctx := gortexmcp.WithSessionID(context.Background(), "facade-session") + + out, status, err := exec(ctx, "find_clones", []byte(`{}`)) + require.NoError(t, err) + assert.Equal(t, 404, status, "an already-live tool must still be denied for a session whose surface hides it") + assert.Contains(t, string(out), "tool_not_found") +} + +// TestLocalExecutor_NullBodyRejected pins reviewer concern #2: a +// top-level JSON `null` body must 400, not silently dispatch with nil +// arguments — json.Unmarshal treats null as a no-op for both struct +// and map targets, so a naïve parse lets it through. +func TestLocalExecutor_NullBodyRejected(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("null")) + require.NoError(t, err) + assert.Equal(t, 400, status) + assert.Contains(t, string(out), "invalid_json") + assert.False(t, handlerRan, "a JSON-null body must not run the handler") +} + +// TestLocalExecutor_NestedArgumentsNullRejected covers the second null +// form from reviewer concern #2: `{"arguments": null}` must also 400. +func TestLocalExecutor_NestedArgumentsNullRejected(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(`{"arguments": null}`)) + require.NoError(t, err) + assert.Equal(t, 400, status) + assert.Contains(t, string(out), "invalid_json") + assert.False(t, handlerRan, `{"arguments": null} must not run the handler`) +} diff --git a/internal/mcp/lazy_tools_test.go b/internal/mcp/lazy_tools_test.go index 793d74ab4..e893dde68 100644 --- a/internal/mcp/lazy_tools_test.go +++ b/internal/mcp/lazy_tools_test.go @@ -7,6 +7,7 @@ import ( "strings" "sync" "testing" + "time" mcplib "github.com/mark3labs/mcp-go/mcp" "github.com/stretchr/testify/assert" @@ -435,9 +436,11 @@ func TestPromote_ConcurrentCallersNeverFalse404(t *testing.T) { start := make(chan struct{}) results := make(chan bool, 2) + done := make(chan struct{}, 2) // Goroutine 1: transitions the tool, blocks inside the promote // callback before the live registration is visible. go func() { + defer func() { done <- struct{}{} }() <-start transitioned := r.Promote("race_tool") mu.Lock() @@ -453,6 +456,7 @@ func TestPromote_ConcurrentCallersNeverFalse404(t *testing.T) { // live yet → false 404. Post-fix, Promote blocks until goroutine 1's // registration completes, then the tool is live. go func() { + defer func() { done <- struct{}{} }() <-start <-promoteBlocked // wait until goroutine 1 is inside the callback // Pre-fix check: the tool is marked promoted but not yet live — @@ -472,4 +476,29 @@ func TestPromote_ConcurrentCallersNeverFalse404(t *testing.T) { // postLive is true. results <- (len(transitioned) > 0 || postLive || intermediateLive) }() + + // Release both callers simultaneously so the interleaving above is + // actually exercised, then collect both verdicts under a bounded + // timeout — a hang here means the fix regressed to a deadlock, not + // a silently-green test. + close(start) + timeout := time.After(5 * time.Second) + for i := 0; i < 2; i++ { + select { + case ok := <-results: + assert.True(t, ok, "concurrent caller must never observe a false 404 (tool marked promoted but never live)") + case <-timeout: + t.Fatal("timed out waiting for concurrent Promote callers — possible deadlock in the fixed implementation") + } + } + + // Both goroutines must actually exit (not leaked) before the test + // returns; give them a bounded window past their result send. + for i := 0; i < 2; i++ { + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for a goroutine to exit — possible leak") + } + } } diff --git a/internal/mcp/streamable/transport.go b/internal/mcp/streamable/transport.go index 56aea8070..d923a6589 100644 --- a/internal/mcp/streamable/transport.go +++ b/internal/mcp/streamable/transport.go @@ -474,16 +474,32 @@ func (t *Transport) tryRouteToolCall(r *http.Request, state SessionState, frame // the local executor's nested-arguments unmarshal path (see // cmd/gortex/server_router.go newLocalToolExecutor) finds them. // This matches cmd/gortex/daemon_mcp.go:tryProxyToolCall exactly. + // A missing `arguments` key AND an explicit JSON `null` both mean + // "no arguments" at the MCP layer (params.arguments is optional); + // normalize both to `{}` so the executor's "arguments must be an + // object when present" check (added for reviewer concern #2) never + // rejects a legitimate no-arg call. rawArgs := envelope.Params.Arguments - if len(rawArgs) == 0 { + if len(rawArgs) == 0 || strings.TrimSpace(string(rawArgs)) == "null" { rawArgs = json.RawMessage(`{}`) } body, err := json.Marshal(map[string]json.RawMessage{"arguments": rawArgs}) if err != nil { return nil, 0, false } + // Attach the session id to ctx before the routing decision — the + // local-fast path (Decide -> RouteToolCall -> callLocal -> + // newLocalToolExecutor) threads this ctx straight into the + // session-policy gate, so a call routed here with no session id + // would evaluate the daemon's default surface instead of this + // session's actual effective surface (mirrors localDispatch below + // and the internal/server/handler.go handleToolCall fix). + ctx := r.Context() + if state.ID != "" { + ctx = gortexmcp.WithSessionID(ctx, state.ID) + } decision := daemon.NewProxyDecision(func() *daemon.Router { return t.router }) - outcome := decision.Decide(r.Context(), daemon.RouteInputs{ + outcome := decision.Decide(ctx, daemon.RouteInputs{ ToolName: envelope.Params.Name, Body: body, Cwd: cwd, diff --git a/internal/mcp/tool_profile.go b/internal/mcp/tool_profile.go index 31e1f9ef8..682f0f450 100644 --- a/internal/mcp/tool_profile.go +++ b/internal/mcp/tool_profile.go @@ -91,16 +91,29 @@ func (s *Server) sessionLiveToolNames(ctx context.Context) []string { // registeredToolNames returns the complete catalog behind this server: both // the currently registered MCP tools and the lazy registry's cold tools. +// +// Read order matters: lazyToolRegistry.Promote holds its lock for the +// entire mark-and-register transition (see lazy_tools.go), so +// DeferredNames' RLock cannot return mid-promotion — it either observes a +// name still deferred, or observes it already excluded because Promote +// (registration included) has fully completed. Reading DeferredNames +// FIRST and ListTools SECOND therefore guarantees no false miss: a name +// excluded from the first read is guaranteed live by the time of the +// second (registration only ever moves deferred -> live, never back). +// Reading ListTools first (the previous order) raced a concurrent +// Promote: ListTools could snapshot before AddTool ran and DeferredNames +// could snapshot after the name was marked promoted, missing the name in +// both and misclassifying a legitimately live tool as "absent". func (s *Server) registeredToolNames() []string { names := make(map[string]bool) - for name := range s.mcpServer.ListTools() { - names[name] = true - } if s.lazy != nil { for _, name := range s.lazy.DeferredNames() { names[name] = true } } + for name := range s.mcpServer.ListTools() { + names[name] = true + } out := make([]string, 0, len(names)) for name := range names { diff --git a/internal/server/handler.go b/internal/server/handler.go index 311375cc4..1c6126ed6 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -381,6 +381,32 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { return } + // Overlay session binding for the HTTP transport. The standard + // `Mcp-Session-Id` header (set by mcp-go's Streamable HTTP + // client) is preferred; a gortex-specific + // `X-Gortex-Overlay-Session` header takes precedence when + // callers want to scope an overlay to a session ID that differs + // from their MCP transport session (e.g. a CI harness that + // orchestrates several overlay scopes from one connection). A + // `?session_id=` query parameter is the final fallback so curl / + // integration tests can attach overlays without setting HTTP + // headers. The session ID flows through gortexmcp.WithSessionID + // so the MCP overlay middleware (overlay.go::wrapToolHandler) + // finds the right overlay snapshot. This MUST happen before the + // router decision below: the router's local-fast path threads ctx + // straight through to the in-process tool dispatch (including the + // deferred-tool promotion gate), so a session ID attached only + // after routing would leave that path evaluating the daemon's + // default surface instead of the caller's actual session policy. + ctx := r.Context() + if sid := firstNonEmpty( + r.Header.Get("X-Gortex-Overlay-Session"), + r.Header.Get("Mcp-Session-Id"), + r.URL.Query().Get("session_id"), + ); sid != "" { + ctx = gortexmcp.WithSessionID(ctx, sid) + } + // If a Router is wired, peek the body for `workspace` / `cwd` // overrides and let the router // decide local vs remote. Local path falls through to the @@ -390,7 +416,7 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { // (combo / frecency / session state) keep working unchanged. if h.router != nil && h.decision != nil { scope, cwd := h.peekRouteContext(body, r) - outcome := h.decision.Decide(r.Context(), daemon.RouteInputs{ + outcome := h.decision.Decide(ctx, daemon.RouteInputs{ ToolName: toolName, Body: body, Cwd: cwd, @@ -425,21 +451,38 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { return } + // Parse via an `any` probe rather than unmarshalling straight into + // ToolRequest: a JSON `null` body (or `{"arguments": null}`) is a + // silent no-op for both a struct and a map target in Go, so the + // previous req-then-flat-fallback shape let a null body through + // with nil arguments instead of 400ing (reviewer concern #2 — this + // direct-dispatch path is a second producer of the same defect + // fixed in cmd/gortex/server_router.go's newLocalToolExecutor). var args map[string]any var bodyFormat string if len(body) > 0 { - var req ToolRequest - if err := json.Unmarshal(body, &req); err != nil { - if err2 := json.Unmarshal(body, &args); err2 != nil { - WriteJSONError(w, http.StatusBadRequest, fmt.Sprintf("malformed JSON: %s", err.Error())) + var probe any + if err := json.Unmarshal(body, &probe); err != nil { + WriteJSONError(w, http.StatusBadRequest, fmt.Sprintf("malformed JSON: %s", err.Error())) + return + } + obj, ok := probe.(map[string]any) + if !ok { + WriteJSONError(w, http.StatusBadRequest, "malformed JSON: expected a JSON object") + return + } + if f, ok := obj["format"].(string); ok { + bodyFormat = f + } + if rawArgs, present := obj["arguments"]; present { + nested, ok := rawArgs.(map[string]any) + if !ok { + WriteJSONError(w, http.StatusBadRequest, `malformed JSON: "arguments" must be a JSON object`) return } + args = nested } else { - args = req.Arguments - bodyFormat = req.Format - if args == nil { - _ = json.Unmarshal(body, &args) - } + args = obj } } @@ -463,26 +506,6 @@ func (h *Handler) handleToolCall(w http.ResponseWriter, r *http.Request) { }, } - // Overlay session binding for the HTTP transport. The standard - // `Mcp-Session-Id` header (set by mcp-go's Streamable HTTP - // client) is preferred; a gortex-specific - // `X-Gortex-Overlay-Session` header takes precedence when - // callers want to scope an overlay to a session ID that differs - // from their MCP transport session (e.g. a CI harness that - // orchestrates several overlay scopes from one connection). A - // `?session_id=` query parameter is the final fallback so curl / - // integration tests can attach overlays without setting HTTP - // headers. The session ID flows through gortexmcp.WithSessionID - // so the MCP overlay middleware (overlay.go::wrapToolHandler) - // finds the right overlay snapshot. - ctx := r.Context() - if sid := firstNonEmpty( - r.Header.Get("X-Gortex-Overlay-Session"), - r.Header.Get("Mcp-Session-Id"), - r.URL.Query().Get("session_id"), - ); sid != "" { - ctx = gortexmcp.WithSessionID(ctx, sid) - } result, err := tool.Handler(ctx, mcpReq) if err != nil { h.logger.Error("tool call failed", From 31f19ffded1d687936e0e6df9c900f35395a6ce0 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 29 Aug 2026 13:37:15 -0500 Subject: [PATCH 8/8] Fix round-3 review regression: don't collapse blocked-by-preset into 404 The round-1/2 fix made newLocalToolExecutor check session policy unconditionally, including for tools that are already live. That turned a live-but-blocked-by-active-preset tool (e.g. "read" or "search" under a facade-v1 session) into a bare 404 tool_not_found, discarding the structured tool_blocked_by_mode error (error code, required preset, recovery guidance) that checkToolGate already produces for that exact case on every other call path. checkToolGate runs inside every registered tool's wrapped handler (all 4 production registration sites wrap with wrapToolHandlerMode), so once ctx carries the correct session id it already re-derives session policy correctly for an already-live tool -- no extra executor-level check was needed there. The only place a pre-check is actually load-bearing is guarding promotion's side effect (mutating the shared lazy registry), which EnsureToolPromotedForSession already does via its own IsToolEnabledForSession call. Reverted the lookup to GetTool -> (if nil) EnsureToolPromotedForSession -> re-GetTool, matching the original shape. Updated TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive to assert the correct structured-error behavior instead of the wrong 404 my earlier fix baked into the test. Also fixes streamable/transport.go's tryRouteToolCall: it attached WithSessionID before the routing decision but not WithSessionCWD, unlike localDispatch in the same file -- a session's workspace boundary wasn't enforced on this routed path. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NktJ7aab9oD9U1ks35g9TS --- cmd/gortex/server_router.go | 51 +++++++++++++++------------- cmd/gortex/server_router_test.go | 17 +++++++--- internal/mcp/streamable/transport.go | 16 +++++---- 3 files changed, 50 insertions(+), 34 deletions(-) diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index f8be16e1e..774de31be 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -75,31 +75,36 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca } } - // Session-aware gate, checked unconditionally — regardless of - // whether the tool is already live, deferred, or unknown. This - // must run BEFORE any lookup/promotion decision: checking it - // only on a registry miss (the pre-fix shape) let an - // already-promoted tool bypass the session's effective surface - // entirely, since a live GetTool hit skipped the gate below. - if !srv.IsToolEnabledForSession(ctx, toolName) { - payload := map[string]any{ - "error": "tool_not_found", - "message": fmt.Sprintf("tool '%s' not found", toolName), - } - out, _ := json.Marshal(payload) - return out, 404, nil - } - ctx = gortexmcp.WithAuthorizedToolCall(ctx, toolName) - + // An already-live tool (whether generally allowed or blocked by + // the session's active preset/facade surface) dispatches + // straight to its handler with NO gate here: every production + // registration path (addTool, addControlTool, lazy promote, + // facade_tools) wraps the handler with wrapToolHandlerMode, + // which runs checkToolGate on every call — including this one, + // now that ctx carries the caller's session id (see the + // handleToolCall / tryRouteToolCall ctx-ordering fix). That gate + // is what should decide a blocked-by-preset call: it returns a + // structured tool_blocked_by_mode error the client can act on + // (which preset, how to reconnect). Adding a coarser gate here + // too previously collapsed that structured error into a bare + // 404 "not found" — a lie for a tool that IS registered — so + // this path deliberately does not duplicate the check for an + // already-live tool. + // + // A NOT-yet-live (deferred) tool is different: promoting it is + // itself a side effect (it mutates the shared lazy registry + // process-wide), so that side effect must stay gated on the + // session's effective surface — EnsureToolPromotedForSession + // checks IsToolEnabledForSession before promoting, so a session + // whose surface hides the tool never promotes it (and gets a + // 404, since there's nothing live to dispatch to and nothing to + // promote on its behalf). tool := srv.MCPServer().GetTool(toolName) if tool == nil { - // Deferred/lazy catalog under the defer-mode tools_search - // split (the shipped core-preset default) — not yet in the - // live registry until promoted just now. The session gate - // above already cleared this name for ctx's effective - // surface, so promoting it here cannot leak a hidden tool. - srv.EnsureToolPromoted(toolName) - tool = srv.MCPServer().GetTool(toolName) + if srv.EnsureToolPromotedForSession(ctx, toolName) { + ctx = gortexmcp.WithAuthorizedToolCall(ctx, toolName) + tool = srv.MCPServer().GetTool(toolName) + } } if tool == nil { payload := map[string]any{ diff --git a/cmd/gortex/server_router_test.go b/cmd/gortex/server_router_test.go index 889023095..13d281cd1 100644 --- a/cmd/gortex/server_router_test.go +++ b/cmd/gortex/server_router_test.go @@ -208,9 +208,15 @@ func TestLocalExecutor_HiddenSessionDeniedWithoutPromotion(t *testing.T) { // TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive is the // other half of reviewer concern #1: the pre-fix code only checked -// session policy on a registry miss, so a tool some other caller had -// already promoted bypassed the gate entirely. Promote it out-of-band -// first, then confirm a hidden session still gets 404. +// session policy on a registry miss (guarding promotion), never on an +// already-live tool. Promote it out-of-band first, then confirm a +// hidden session's call is still denied — but via the SAME structured +// tool_blocked_by_mode error every other blocked-by-preset call gets +// (checkToolGate, running inside every registered handler), not a +// bare 404. An executor-level pre-check that turned this into 404 +// would be lying about tool existence and would throw away the +// error's recovery guidance — that was a real regression an earlier +// draft of this fix introduced and a later review caught. func TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive(t *testing.T) { t.Setenv("GORTEX_LAZY_TOOLS", "1") srv, exec := executorTestServer(t) @@ -222,8 +228,9 @@ func TestLocalExecutor_HiddenSessionDeniedEvenWhenAlreadyLive(t *testing.T) { out, status, err := exec(ctx, "find_clones", []byte(`{}`)) require.NoError(t, err) - assert.Equal(t, 404, status, "an already-live tool must still be denied for a session whose surface hides it") - assert.Contains(t, string(out), "tool_not_found") + assert.Equal(t, 200, status, "an already-live tool blocked by the session's active preset dispatches to its handler, which reports the block structurally — it is not a 404") + assert.Contains(t, string(out), "tool_blocked_by_mode", "the structured error code must survive, not collapse into a bare not-found") + assert.Contains(t, string(out), "find_clones") } // TestLocalExecutor_NullBodyRejected pins reviewer concern #2: a diff --git a/internal/mcp/streamable/transport.go b/internal/mcp/streamable/transport.go index d923a6589..f34b20203 100644 --- a/internal/mcp/streamable/transport.go +++ b/internal/mcp/streamable/transport.go @@ -487,17 +487,21 @@ func (t *Transport) tryRouteToolCall(r *http.Request, state SessionState, frame if err != nil { return nil, 0, false } - // Attach the session id to ctx before the routing decision — the - // local-fast path (Decide -> RouteToolCall -> callLocal -> + // Attach the session id AND cwd to ctx before the routing decision — + // the local-fast path (Decide -> RouteToolCall -> callLocal -> // newLocalToolExecutor) threads this ctx straight into the - // session-policy gate, so a call routed here with no session id - // would evaluate the daemon's default surface instead of this - // session's actual effective surface (mirrors localDispatch below - // and the internal/server/handler.go handleToolCall fix). + // session-policy gate and the handler itself. Session id alone + // isn't enough: localDispatch below (and the daemon dispatcher's + // tryProxyToolCall) also attach WithSessionCWD, because handlers + // use it as a workspace boundary — without it, a session in + // workspace A could see workspace B's nodes on this routed path. ctx := r.Context() if state.ID != "" { ctx = gortexmcp.WithSessionID(ctx, state.ID) } + if cwd != "" { + ctx = gortexmcp.WithSessionCWD(ctx, cwd) + } decision := daemon.NewProxyDecision(func() *daemon.Router { return t.router }) outcome := decision.Decide(ctx, daemon.RouteInputs{ ToolName: envelope.Params.Name,