From 20454eb46969b0b4552a8fdf0b44b943b950fffa Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 21 Aug 2026 08:55:34 -0500 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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 05/10] 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 06/10] 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 07/10] 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 08/10] 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, From fda59e68e8f615f3a5685a720538c99edcd64d42 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 29 Aug 2026 18:03:54 -0500 Subject: [PATCH 09/10] Address second maintainer review: separate overlay-cohort identity, restore session-CWD parity, unify isError wire contract Fixes findings #1, #3 (local half), and #4 from a second round of review on PR 649. #2 and #3's remote half need a federation-protocol decision and are tracked separately in issue #696, per discussion. 1. X-Gortex-Overlay-Session no longer substitutes for the caller's real session identity anywhere except the overlay subsystem itself. handleToolCall previously picked one winner across Mcp-Session-Id/X-Gortex-Overlay-Session/?session_id and fed it into WithSessionID -- the one context value that also drives every other per-session subsystem (tool-policy gating, token stats, agent registry, diagnostics subscriptions, notes/memory scoping, ~30 call sites in total). A restricted session paired with a permissive overlay-cohort override had its policy silently bypassed. Added WithOverlayCohortID/OverlayCohortIDFromContext in session_ctx.go, falling back to SessionIDFromContext when unset. Switched the ~8 genuinely overlay-related call sites (overlaySessionID, snapshotOverlayRequestForCtx, prepareOverlayRequest, buildOverlayViewForCtx, the simulate/ explore-literal-overlay call sites) to the new accessor; every other SessionIDFromContext caller is untouched. handleToolCall now attaches WithSessionID only from Mcp-Session-Id/?session_id, and WithOverlayCohortID only from X-Gortex-Overlay-Session when present. 2. handleToolCall never attached WithSessionCWD, unlike the Streamable HTTP transport's tryRouteToolCall (fixed in a prior round). A tool reading the session's workspace boundary from context saw nothing on this path. cwd is now computed once via peekRouteContext and attached to ctx regardless of whether a router is wired. 3. newLocalToolExecutor's response struct tagged the error field is_error (snake_case); the Streamable HTTP transport's wrapToolResultAsJSONRPC only recognizes isError and silently defaults to false on a mismatch -- a genuine tool error routed through that path was reported to the client as success. Now reuses internal/server's ToolResponse/ToolContent types directly instead of an independently-typed lookalike, so this class of drift is a compile error going forward. New tests: TestToolCall_OverlayHeaderDoesNotOverridePolicySession and TestToolCall_SessionCWDReachesHandler (internal/server, against a real gortexmcp.Server so policy gating is actually exercised, not the bare mcpserver.MCPServer the existing handler tests use) and TestLocalExecutor_ErrorResponseUsesIsErrorTag (cmd/gortex). All three verified to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NktJ7aab9oD9U1ks35g9TS --- cmd/gortex/server_router.go | 26 ++-- cmd/gortex/server_router_test.go | 37 ++++++ docs/mcp.md | 2 +- .../mcp/explore_source_literal_overlay.go | 4 +- internal/mcp/overlay_view.go | 8 +- internal/mcp/session_ctx.go | 60 +++++++++ internal/mcp/tools_overlay.go | 8 +- internal/mcp/tools_overlay_diff.go | 2 +- internal/mcp/tools_simulate.go | 4 +- internal/server/handler.go | 72 +++++++---- .../server/handler_session_identity_test.go | 122 ++++++++++++++++++ 11 files changed, 297 insertions(+), 48 deletions(-) create mode 100644 internal/server/handler_session_identity_test.go diff --git a/cmd/gortex/server_router.go b/cmd/gortex/server_router.go index 774de31be..6651321e8 100644 --- a/cmd/gortex/server_router.go +++ b/cmd/gortex/server_router.go @@ -10,6 +10,7 @@ import ( "github.com/zzet/gortex/internal/daemon" gortexmcp "github.com/zzet/gortex/internal/mcp" + "github.com/zzet/gortex/internal/server" ) // newLocalToolExecutor builds the daemon.LocalExecutor closure used by @@ -133,17 +134,24 @@ func newLocalToolExecutor(srv *gortexmcp.Server, logger *zap.Logger) daemon.Loca return out, 500, nil } - // Mirror the same response shape the HTTP handler emits so - // the proxy and local paths are indistinguishable downstream. - resp := struct { - IsError bool `json:"is_error,omitempty"` - Content []map[string]any `json:"content,omitempty"` - }{IsError: result.IsError} + // Reuse the SAME response type internal/server's HTTP handler + // and the Streamable HTTP transport's wrapToolResultAsJSONRPC + // both serialize/parse (server.ToolResponse: "content"/ + // "isError") — not an independently-typed lookalike. A prior + // version of this struct tagged the error field "is_error" + // (snake_case); wrapToolResultAsJSONRPC only recognizes + // "isError" and silently defaults IsError to false on a + // mismatch, so a genuine tool error routed through the + // Streamable HTTP transport's local-fast path was reported to + // the client as a successful result. Sharing one type makes + // that class of drift a compile error instead of a silent + // wire-format bug. + resp := server.ToolResponse{IsError: result.IsError} for _, c := range result.Content { if tc, ok := c.(mcp.TextContent); ok { - resp.Content = append(resp.Content, map[string]any{ - "type": "text", - "text": tc.Text, + resp.Content = append(resp.Content, server.ToolContent{ + Type: "text", + Text: tc.Text, }) } } diff --git a/cmd/gortex/server_router_test.go b/cmd/gortex/server_router_test.go index 13d281cd1..8eebb27d3 100644 --- a/cmd/gortex/server_router_test.go +++ b/cmd/gortex/server_router_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "encoding/json" "os" "path/filepath" "sync" @@ -255,6 +256,42 @@ func TestLocalExecutor_NullBodyRejected(t *testing.T) { assert.False(t, handlerRan, "a JSON-null body must not run the handler") } +// TestLocalExecutor_ErrorResponseUsesIsErrorTag pins the fix for a +// second review round's finding #4: the local executor's response +// must serialize the error flag as "isError" (matching the standard +// ToolResponse / Streamable HTTP wrapToolResultAsJSONRPC contract), +// not "is_error". A prior version of the response struct used the +// wrong tag, so wrapToolResultAsJSONRPC's `json:"isError"` field +// never matched, silently defaulting IsError to false — a genuine +// tool error routed through the Streamable HTTP transport's +// local-fast path was reported to the client as success. +func TestLocalExecutor_ErrorResponseUsesIsErrorTag(t *testing.T) { + srv, exec := executorTestServer(t) + srv.MCPServer().AddTool( + mcp.NewTool("boom_tool", mcp.WithDescription("test")), + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultError("boom"), nil + }, + ) + + out, status, err := exec(context.Background(), "boom_tool", []byte(`{}`)) + require.NoError(t, err) + assert.Equal(t, 200, status) + assert.Contains(t, string(out), `"isError":true`, "must use the standard isError tag") + assert.NotContains(t, string(out), "is_error", "must not use the old snake_case tag") + + var decoded struct { + IsError bool `json:"isError"` + Content []struct { + Text string `json:"text"` + } `json:"content"` + } + require.NoError(t, json.Unmarshal(out, &decoded)) + assert.True(t, decoded.IsError, "isError must round-trip through the standard tag") + require.Len(t, decoded.Content, 1) + assert.Equal(t, "boom", decoded.Content[0].Text) +} + // TestLocalExecutor_NestedArgumentsNullRejected covers the second null // form from reviewer concern #2: `{"arguments": null}` must also 400. func TestLocalExecutor_NestedArgumentsNullRejected(t *testing.T) { diff --git a/docs/mcp.md b/docs/mcp.md index b16e45b3c..13e0dee71 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -525,7 +525,7 @@ Editor extensions push in-flight (unsaved) buffers as **overlays**. Gortex compo | `overlay_drop_branch` | Delete a named branch — refuses to drop the active branch or the implicit `main` | | `compare_branches` | Run `find_usages` / `get_callers` / `get_call_chain` / `get_dependencies` / `get_dependents` against two branches and report each side plus the delta | -HTTP transport mirrors the surface at `/v1/overlay/sessions/*`; the `/v1/tools/` entry point reads the overlay session from `Mcp-Session-Id` (preferred), `X-Gortex-Overlay-Session`, or `?session_id=`. Overlays are bound to their MCP session — when the session ends the overlay is dropped synchronously. Idle TTL is a fail-safe (default 30 m, configurable via `GORTEX_OVERLAY_IDLE_TTL`); every tool call against a live overlay refreshes it. +HTTP transport mirrors the surface at `/v1/overlay/sessions/*`. The `/v1/tools/` entry point resolves the caller's real session identity from `Mcp-Session-Id` (preferred) or `?session_id=` — this identity drives every per-session subsystem: tool-policy gating, token-stats accounting, notes/memory scoping, and so on. `X-Gortex-Overlay-Session`, when present, is a narrower, independent override that scopes *only* overlay state to a different cohort id than the caller's own session (e.g. a CI harness orchestrating several overlay scopes from one connection) — it never substitutes for the real session identity anywhere else. Overlays are bound to their cohort id — when the owning session ends the overlay is dropped synchronously. Idle TTL is a fail-safe (default 30 m, configurable via `GORTEX_OVERLAY_IDLE_TTL`); every tool call against a live overlay refreshes it. ## Speculative execution diff --git a/internal/mcp/explore_source_literal_overlay.go b/internal/mcp/explore_source_literal_overlay.go index 18b928526..68f3f6a75 100644 --- a/internal/mcp/explore_source_literal_overlay.go +++ b/internal/mcp/explore_source_literal_overlay.go @@ -128,8 +128,8 @@ func (s *Server) snapshotExploreSourceLiteralOverlays( } return nil, covered, false, false, nil } - if snapshot.sessionID != SessionIDFromContext(ctx) { - return nil, covered, false, false, fmt.Errorf("overlay request snapshot belongs to session %q, not %q", snapshot.sessionID, SessionIDFromContext(ctx)) + if snapshot.sessionID != OverlayCohortIDFromContext(ctx) { + return nil, covered, false, false, fmt.Errorf("overlay request snapshot belongs to session %q, not %q", snapshot.sessionID, OverlayCohortIDFromContext(ctx)) } if !snapshot.canonical { return nil, covered, true, false, fmt.Errorf("overlay request snapshot is not canonical") diff --git a/internal/mcp/overlay_view.go b/internal/mcp/overlay_view.go index 54fe4a276..e23f4e8ae 100644 --- a/internal/mcp/overlay_view.go +++ b/internal/mcp/overlay_view.go @@ -196,7 +196,7 @@ func (s *Server) snapshotOverlayRequestForCtx(ctx context.Context) (*overlayRequ if s == nil || s.overlays == nil { return nil, nil } - sessionID := SessionIDFromContext(ctx) + sessionID := OverlayCohortIDFromContext(ctx) if sessionID == "" { return nil, nil } @@ -230,8 +230,8 @@ func (s *Server) prepareOverlayRequest(ctx context.Context) (context.Context, *g return ctx, nil, nil } snapshot, ok := overlayRequestSnapshotFromContext(ctx) - if ok && snapshot.sessionID != SessionIDFromContext(ctx) { - return ctx, nil, fmt.Errorf("overlay request snapshot belongs to session %q, not %q", snapshot.sessionID, SessionIDFromContext(ctx)) + if ok && snapshot.sessionID != OverlayCohortIDFromContext(ctx) { + return ctx, nil, fmt.Errorf("overlay request snapshot belongs to session %q, not %q", snapshot.sessionID, OverlayCohortIDFromContext(ctx)) } if !ok { if OverlayViewFromContext(ctx) != nil { @@ -352,7 +352,7 @@ func (s *Server) buildOverlayViewForCtx(ctx context.Context) (*graph.OverlaidVie return nil, fmt.Errorf("overlay request snapshot is not canonical") } files := snapshot.files - sessID := SessionIDFromContext(ctx) + sessID := OverlayCohortIDFromContext(ctx) // Drift check up front for every overlay that carries a BaseSHA. // We do it here, before parsing, so a stale overlay never costs diff --git a/internal/mcp/session_ctx.go b/internal/mcp/session_ctx.go index dc71dd229..7b64ffac6 100644 --- a/internal/mcp/session_ctx.go +++ b/internal/mcp/session_ctx.go @@ -21,6 +21,16 @@ type sessionCtxKey struct{} // calling MCPServer.HandleMessage, giving every tool handler access // to the per-session state without touching the handler signature. // +// This is the ONE universal session identity: sessionFor, +// effectiveSessionPolicy (and therefore every tool-policy gate), +// tokenStatsFor, the agent registry, diagnostics/health/readiness/ +// stale-refs subscriptions, localization state, query logging, and +// notes/memory scoping all key off SessionIDFromContext. It MUST be +// the caller's real transport/MCP session id (Mcp-Session-Id, or the +// stdio server's implicit single session) — never overridden by an +// unrelated selector. See WithOverlayCohortID for the one narrow +// exception (overlay snapshot binding) that is allowed to diverge. +// // An empty id is treated as "no session" and returns ctx unchanged — // that's the path the embedded stdio server takes, where there's only // one implicit session. @@ -44,6 +54,56 @@ func SessionIDFromContext(ctx context.Context) string { return "" } +// overlayCohortCtxKey carries an explicit override for which overlay +// cohort a request's overlay-scoped calls should bind to, when that +// differs from the caller's real session id (see WithOverlayCohortID). +// Unexported: use WithOverlayCohortID / OverlayCohortIDFromContext. +type overlayCohortCtxKey struct{} + +// WithOverlayCohortID returns a context carrying an overlay-cohort +// override distinct from the request's real session id +// (SessionIDFromContext). Only the overlay subsystem's own accessors +// (overlaySessionID, snapshotOverlayRequestForCtx, +// prepareOverlayRequest, buildOverlayViewForCtx, and the simulate / +// explore-literal-overlay call sites) consult this — every other +// SessionIDFromContext caller (policy, token stats, notes, agent +// registry, diagnostics subscriptions, ...) is intentionally +// unaffected by it. +// +// This exists for callers that legitimately want to scope overlay +// state to a cohort id that differs from their own transport session +// (e.g. a CI harness that orchestrates several overlay scopes from +// one connection) WITHOUT that selection also silently redirecting +// every other per-session subsystem to the wrong identity — which is +// exactly what happened when a single header-precedence value fed +// both purposes: a session's own tool-policy gate could be bypassed +// by pairing a restricted Mcp-Session-Id with a permissive +// X-Gortex-Overlay-Session. +// +// An empty id returns ctx unchanged; OverlayCohortIDFromContext then +// falls back to SessionIDFromContext, so a caller that never sets +// this behaves byte-identically to before this type existed. +func WithOverlayCohortID(ctx context.Context, id string) context.Context { + if id == "" { + return ctx + } + return context.WithValue(ctx, overlayCohortCtxKey{}, id) +} + +// OverlayCohortIDFromContext returns the overlay-cohort override +// attached via WithOverlayCohortID, or SessionIDFromContext(ctx) when +// none was set — the common case, where overlay state binds to the +// caller's own session exactly as it always has. +func OverlayCohortIDFromContext(ctx context.Context) string { + if ctx == nil { + return "" + } + if id, ok := ctx.Value(overlayCohortCtxKey{}).(string); ok && id != "" { + return id + } + return SessionIDFromContext(ctx) +} + // sessionCWDCtxKey carries the session's working directory. The // daemon's MCP dispatcher stashes it alongside the session ID so tool // handlers can resolve — and enforce — the workspace boundary for the diff --git a/internal/mcp/tools_overlay.go b/internal/mcp/tools_overlay.go index 04c5976f5..8cf2d4db3 100644 --- a/internal/mcp/tools_overlay.go +++ b/internal/mcp/tools_overlay.go @@ -83,11 +83,13 @@ func (s *Server) registerOverlayTools() { s.registerOverlayBranchTools() } -// overlaySessionID returns the calling MCP session ID, or a structured -// MCP error result when no session is on the context. Used by every +// overlaySessionID returns the calling request's overlay cohort id — +// SessionIDFromContext by default, or the X-Gortex-Overlay-Session +// override when one was set (see WithOverlayCohortID) — or a +// structured MCP error result when neither is present. Used by every // overlay_* handler. func (s *Server) overlaySessionID(ctx context.Context) (string, *mcp.CallToolResult) { - id := SessionIDFromContext(ctx) + id := OverlayCohortIDFromContext(ctx) if id == "" { return "", mcp.NewToolResultError("overlay tools require an MCP session — connect via the daemon or set X-Mcp-Session-Id") } diff --git a/internal/mcp/tools_overlay_diff.go b/internal/mcp/tools_overlay_diff.go index d1964f70c..225766034 100644 --- a/internal/mcp/tools_overlay_diff.go +++ b/internal/mcp/tools_overlay_diff.go @@ -47,7 +47,7 @@ func (s *Server) handleCompareWithOverlay(ctx context.Context, req mcp.CallToolR if s.overlays == nil { return mcp.NewToolResultError("overlay support is not enabled on this server"), nil } - if SessionIDFromContext(ctx) == "" { + if OverlayCohortIDFromContext(ctx) == "" { return mcp.NewToolResultError("compare_with_overlay requires an MCP session; connect via the daemon or set Mcp-Session-Id"), nil } ctx, view, viewErr := s.prepareOverlayRequest(ctx) diff --git a/internal/mcp/tools_simulate.go b/internal/mcp/tools_simulate.go index 945bdb9ee..d0ac4c92a 100644 --- a/internal/mcp/tools_simulate.go +++ b/internal/mcp/tools_simulate.go @@ -326,7 +326,7 @@ func (s *Server) buildSimulation(ctx context.Context, edits []lsp.WorkspaceEdit, current := map[string]daemon.OverlayFile{} if inherit { - if sessID := SessionIDFromContext(ctx); sessID != "" && s.overlays != nil && s.overlays.Has(sessID) { + if sessID := OverlayCohortIDFromContext(ctx); sessID != "" && s.overlays != nil && s.overlays.Has(sessID) { if err := ctx.Err(); err != nil { return nil, err } @@ -1125,7 +1125,7 @@ func (s *Server) persistSimulationOverlay(ctx context.Context, sim *simulation) if s.overlays == nil { return "", errors.New("overlay support is not enabled on this server") } - sessID := SessionIDFromContext(ctx) + sessID := OverlayCohortIDFromContext(ctx) if sessID == "" { return "", nil } diff --git a/internal/server/handler.go b/internal/server/handler.go index 1c6126ed6..4c8e7b91f 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -381,41 +381,61 @@ 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. + // Session identity for the HTTP transport. `Mcp-Session-Id` (set + // by mcp-go's Streamable HTTP client) or the `?session_id=` query + // fallback (for curl / integration tests) is the caller's REAL + // session id — this is what gortexmcp.WithSessionID carries, and + // it MUST be the caller's actual identity: it drives + // effectiveSessionPolicy / every tool-policy gate, token-stats + // accounting, the agent registry, notes/memory scoping, and + // diagnostics subscriptions (see session_ctx.go). It must never be + // substituted with an unrelated selector — doing so previously let + // a session's own tool-policy gate be bypassed by pairing a + // restricted Mcp-Session-Id with a permissive + // X-Gortex-Overlay-Session, since both fed the same context value. + // + // `X-Gortex-Overlay-Session` is a NARROWER, separate override: it + // lets a caller scope overlay state (only) to a cohort id that + // differs from their own session — e.g. a CI harness orchestrating + // several overlay scopes from one connection. It flows through + // gortexmcp.WithOverlayCohortID, which only the overlay subsystem's + // own accessors consult (overlay.go::wrapToolHandler and friends); + // every other per-session subsystem keeps reading the real session + // id above, untouched by this header. + // + // Both attachments 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 attaching either 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 != "" { + if sid := firstNonEmpty(r.Header.Get("Mcp-Session-Id"), r.URL.Query().Get("session_id")); sid != "" { ctx = gortexmcp.WithSessionID(ctx, sid) } + if cohort := r.Header.Get("X-Gortex-Overlay-Session"); cohort != "" { + ctx = gortexmcp.WithOverlayCohortID(ctx, cohort) + } + + // Peek the body for `workspace` / `cwd` overrides. `cwd` is + // attached to ctx as the session's workspace boundary + // (gortexmcp.WithSessionCWD) regardless of whether a router is + // wired — the direct in-process dispatch below needs it exactly + // as much as the router's local-fast path does; a tool that reads + // the session's workspace boundary from context otherwise sees + // nothing and enforces no boundary at all. + scope, cwd := h.peekRouteContext(body, r) + if cwd != "" { + ctx = gortexmcp.WithSessionCWD(ctx, cwd) + } - // 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 + // If a Router is wired, let it decide local vs remote using the + // scope/cwd peeked above. Local path falls through to the // existing in-process tool dispatch below; remote path returns // the proxied response verbatim. Only the proxy short-circuits // — local routing reuses the legacy code so downstream features // (combo / frecency / session state) keep working unchanged. if h.router != nil && h.decision != nil { - scope, cwd := h.peekRouteContext(body, r) outcome := h.decision.Decide(ctx, daemon.RouteInputs{ ToolName: toolName, Body: body, diff --git a/internal/server/handler_session_identity_test.go b/internal/server/handler_session_identity_test.go new file mode 100644 index 000000000..16d2d51c0 --- /dev/null +++ b/internal/server/handler_session_identity_test.go @@ -0,0 +1,122 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "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/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" +) + +// realServerTestHandler builds a Handler backed by a REAL gortexmcp.Server +// (not the bare mark3labs mcpserver.MCPServer newTestHandler uses), so +// session-policy gating (checkToolGate / effectiveSessionPolicy) is +// actually exercised. Needed for tests pinning the session-identity / +// overlay-cohort separation, which lives entirely in that machinery. +func realServerTestHandler(t *testing.T) (*Handler, *gortexmcp.Server) { + 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) + h := NewHandler(srv.MCPServer(), g, "0.0.1-test", zap.NewNop()) + return h, srv +} + +// TestToolCall_OverlayHeaderDoesNotOverridePolicySession pins the fix for +// the reviewer's finding: X-Gortex-Overlay-Session must scope ONLY overlay +// state to a different cohort id — it must never substitute for +// Mcp-Session-Id when evaluating the caller's tool-policy. Before the fix, +// handleToolCall picked one winner (the overlay header, when present) and +// fed it into gortexmcp.WithSessionID, so a restricted session paired with +// a permissive overlay-cohort override had its policy silently bypassed. +// +// Setup: session "restricted" is facade-v1/hide (blocks find_clones, +// pre-promoted here since handler.go's direct-dispatch path doesn't +// promote deferred tools — a documented, separate, pre-existing gap). +// Session "overlay-open" has no policy noted at all (unrestricted). The +// request carries Mcp-Session-Id: restricted and +// X-Gortex-Overlay-Session: overlay-open. If the overlay header were +// still hijacking policy evaluation, the call would succeed under +// overlay-open's unrestricted policy; with the fix, "restricted"'s +// hide-mode policy applies and the call is blocked. +func TestToolCall_OverlayHeaderDoesNotOverridePolicySession(t *testing.T) { + t.Setenv("GORTEX_LAZY_TOOLS", "1") + h, srv := realServerTestHandler(t) + require.Nil(t, srv.MCPServer().GetTool("find_clones"), "test setup: find_clones must start deferred") + require.True(t, srv.EnsureToolPromoted("find_clones"), "test setup: find_clones must promote cleanly") + require.NotNil(t, srv.MCPServer().GetTool("find_clones"), "test setup: find_clones must be live before the policy is applied") + + srv.NoteSessionToolPolicy("restricted", "facade-v1", "hide") + // "overlay-open" intentionally gets no NoteSessionToolPolicy call — + // it is the permissive/default session a pre-fix bug would wrongly + // evaluate policy against. + + req := httptest.NewRequest(http.MethodPost, "/v1/tools/find_clones", strings.NewReader("{}")) + req.Header.Set("Mcp-Session-Id", "restricted") + req.Header.Set("X-Gortex-Overlay-Session", "overlay-open") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "a blocked-by-preset call still dispatches to the handler, which reports the block structurally") + var resp ToolResponse + require.NoError(t, json.NewDecoder(rec.Body).Decode(&resp)) + require.True(t, resp.IsError, "restricted session's hide-mode policy must apply despite the overlay-cohort override naming a permissive session") + require.Len(t, resp.Content, 1) + assert.Contains(t, resp.Content[0].Text, "tool_blocked_by_mode", + "expected the structured block error from restricted's policy, not overlay-open's unrestricted one") +} + +// TestToolCall_SessionCWDReachesHandler pins the other local half of the +// same finding: handleToolCall must attach the resolved cwd to ctx via +// gortexmcp.WithSessionCWD so a tool handler can read (and enforce) the +// session's workspace boundary. Before the fix, cwd was computed +// (peekRouteContext) and used only to build the router's RouteInputs — +// never attached to ctx — so any tool consulting +// gortexmcp.SessionCWDFromContext saw nothing on this path. +func TestToolCall_SessionCWDReachesHandler(t *testing.T) { + h, srv := realServerTestHandler(t) + var observedCWD string + srv.MCPServer().AddTool( + mcp.NewTool("spy_cwd", mcp.WithDescription("reports the session cwd from context")), + func(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + observedCWD = gortexmcp.SessionCWDFromContext(ctx) + return mcp.NewToolResultText("ok"), nil + }, + ) + + req := httptest.NewRequest(http.MethodPost, "/v1/tools/spy_cwd", strings.NewReader("{}")) + req.Header.Set("X-Gortex-Cwd", "/repo/A") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "/repo/A", observedCWD, "the session's resolved cwd must reach the tool handler via context") +} From df66c3b4087e8985fc8f0e44a0e4d2f710069521 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 29 Aug 2026 19:42:32 -0500 Subject: [PATCH 10/10] Address review findings on fda59e68: TrimSpace parity, stale docs, missing positive overlay test An independent review of fda59e68 (the overlay-cohort/session-CWD/ isError fix commit) found no blocking issues but flagged four items: 1. peekRouteContext's X-Gortex-Cwd header read lacked the strings.TrimSpace the Streamable HTTP transport applies at every read site (transport.go:414,471,894) -- a padded value would now silently become the session's workspace boundary instead of being harmlessly unroutable. Added TrimSpace for parity. 2. dashboard.go's handleOverlayRegister doc comment still said "the MCP tool middleware reads SessionIDFromContext and resolves the overlay by that ID" -- stale after the overlay-cohort split; updated to name OverlayCohortIDFromContext. 3. The docs/mcp.md sentence this same fix commit added ("Overlays are bound to their cohort id -- when the owning session ends the overlay is dropped synchronously") asserts something the code doesn't do for an explicitly-named cohort: ReleaseSession drops by MCP session id, so a cohort named via X-Gortex-Overlay-Session outlives its owning connection until the idle TTL. Corrected. 4. TestToolCall_OverlayHeaderDoesNotOverridePolicySession only proved the overlay header can't leak into policy -- it would still pass if WithOverlayCohortID were deleted entirely and the header dropped on the floor, since it never exercises the positive path. Added TestToolCall_OverlayHeaderScopesOverlayState: pushes an overlay file via the real overlay_push tool while authenticating as a different Mcp-Session-Id, confirms the push lands under the cohort id and not the caller's session id. Verified this new test fails when WithOverlayCohortID's call site is stubbed out. A fifth finding (handler.go's cwd binding now flips sessionScope from unbound to bound for any caller-supplied cwd, without the "daemon dispatcher rejects unreachable cwds before dispatch" pre-check its own comment claims exists) was investigated and not code-changed: that pre-check doesn't actually exist anywhere in the codebase -- the daemon dispatcher only logs an uncovered cwd once and continues, it never rejects. handler.go's new behavior is therefore consistent with every other entrypoint's actual (non-rejecting) behavior, not a new divergence: the shared defensive sentinel in sessionScope already handles an unreachable cwd by scoping to nothing rather than leaking data. Worth noting in the PR description as a behavior change, not a bug. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NktJ7aab9oD9U1ks35g9TS --- docs/mcp.md | 2 +- internal/server/dashboard.go | 10 +++--- internal/server/handler.go | 5 ++- .../server/handler_session_identity_test.go | 35 +++++++++++++++++++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 13e0dee71..696ed1ef2 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -525,7 +525,7 @@ Editor extensions push in-flight (unsaved) buffers as **overlays**. Gortex compo | `overlay_drop_branch` | Delete a named branch — refuses to drop the active branch or the implicit `main` | | `compare_branches` | Run `find_usages` / `get_callers` / `get_call_chain` / `get_dependencies` / `get_dependents` against two branches and report each side plus the delta | -HTTP transport mirrors the surface at `/v1/overlay/sessions/*`. The `/v1/tools/` entry point resolves the caller's real session identity from `Mcp-Session-Id` (preferred) or `?session_id=` — this identity drives every per-session subsystem: tool-policy gating, token-stats accounting, notes/memory scoping, and so on. `X-Gortex-Overlay-Session`, when present, is a narrower, independent override that scopes *only* overlay state to a different cohort id than the caller's own session (e.g. a CI harness orchestrating several overlay scopes from one connection) — it never substitutes for the real session identity anywhere else. Overlays are bound to their cohort id — when the owning session ends the overlay is dropped synchronously. Idle TTL is a fail-safe (default 30 m, configurable via `GORTEX_OVERLAY_IDLE_TTL`); every tool call against a live overlay refreshes it. +HTTP transport mirrors the surface at `/v1/overlay/sessions/*`. The `/v1/tools/` entry point resolves the caller's real session identity from `Mcp-Session-Id` (preferred) or `?session_id=` — this identity drives every per-session subsystem: tool-policy gating, token-stats accounting, notes/memory scoping, and so on. `X-Gortex-Overlay-Session`, when present, is a narrower, independent override that scopes *only* overlay state to a different cohort id than the caller's own session (e.g. a CI harness orchestrating several overlay scopes from one connection) — it never substitutes for the real session identity anywhere else. Overlays are bound to their cohort id; the synchronous drop-on-disconnect only fires for a cohort that matches its own MCP transport session (`ReleaseSession` drops by session id) — a cohort explicitly named via `X-Gortex-Overlay-Session` lives until the idle TTL regardless of the owning connection's lifetime. Idle TTL is a fail-safe (default 30 m, configurable via `GORTEX_OVERLAY_IDLE_TTL`); every tool call against a live overlay refreshes it. ## Speculative execution diff --git a/internal/server/dashboard.go b/internal/server/dashboard.go index 937a54a1d..b3cefc202 100644 --- a/internal/server/dashboard.go +++ b/internal/server/dashboard.go @@ -217,10 +217,12 @@ func (h *Handler) handleRepos(w http.ResponseWriter, _ *http.Request) { // When `session_id` is supplied, the session is registered under that // ID instead of a freshly minted one — this is how an MCP client binds // its overlay session to its MCP session ID, so subsequent tools/call -// frames from the same MCP session automatically see the overlay -// (the MCP tool middleware reads SessionIDFromContext and resolves the -// overlay by that ID). Idempotent: registering twice with the same -// (id, workspace) tuple is a no-op; mismatched workspaces return 409. +// frames from the same MCP session automatically see the overlay (the +// MCP tool middleware reads OverlayCohortIDFromContext, which resolves +// to the real session id unless a caller explicitly overrides it via +// X-Gortex-Overlay-Session). Idempotent: registering twice with the +// same (id, workspace) tuple is a no-op; mismatched workspaces return +// 409. // // Response: {"session_id": "...", "workspace_id": "..."}. func (h *Handler) handleOverlayRegister(w http.ResponseWriter, r *http.Request) { diff --git a/internal/server/handler.go b/internal/server/handler.go index 4c8e7b91f..7616c62b1 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -250,7 +250,10 @@ func (h *Handler) peekRouteContext(body []byte, r *http.Request) (scope, cwd str if cwd == "" { // HTTP clients without an explicit cwd in the body can pass // it via header — matches the daemon's session-cwd plumbing. - cwd = r.Header.Get("X-Gortex-Cwd") + // Streamable HTTP transport trims this same header at every + // read site; match that so a padded value doesn't silently + // become the session's workspace boundary. + cwd = strings.TrimSpace(r.Header.Get("X-Gortex-Cwd")) } return scope, cwd } diff --git a/internal/server/handler_session_identity_test.go b/internal/server/handler_session_identity_test.go index 16d2d51c0..deb826f4a 100644 --- a/internal/server/handler_session_identity_test.go +++ b/internal/server/handler_session_identity_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/mark3labs/mcp-go/mcp" "github.com/stretchr/testify/assert" @@ -16,6 +17,7 @@ import ( "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" @@ -94,6 +96,39 @@ func TestToolCall_OverlayHeaderDoesNotOverridePolicySession(t *testing.T) { "expected the structured block error from restricted's policy, not overlay-open's unrestricted one") } +// TestToolCall_OverlayHeaderScopesOverlayState is the positive +// counterpart to the test above: it proves X-Gortex-Overlay-Session +// actually does something, rather than only proving it doesn't leak +// into policy. Without this, WithOverlayCohortID could be deleted +// entirely and the negative test would still pass (the header would +// simply be ignored). Pushes an overlay file under cohort +// "overlay-open" via the real overlay_push tool while authenticating +// as a different Mcp-Session-Id, then confirms the file landed under +// the cohort id, not the caller's own session id. +func TestToolCall_OverlayHeaderScopesOverlayState(t *testing.T) { + h, srv := realServerTestHandler(t) + srv.SetOverlayManager(daemon.NewOverlayManager(30 * time.Minute)) + + body := `{"arguments":{"path":"scratch.go","content":"package main\n"}}` + req := httptest.NewRequest(http.MethodPost, "/v1/tools/overlay_push", strings.NewReader(body)) + req.Header.Set("Mcp-Session-Id", "caller-session") + req.Header.Set("X-Gortex-Overlay-Session", "overlay-open") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var resp ToolResponse + require.NoError(t, json.NewDecoder(rec.Body).Decode(&resp)) + require.False(t, resp.IsError, "overlay_push must succeed: %+v", resp) + + files, err := srv.OverlayManager().Files("overlay-open") + require.NoError(t, err, "the push must land under the overlay-cohort id") + require.Contains(t, files, "scratch.go") + + _, err = srv.OverlayManager().Files("caller-session") + assert.Error(t, err, "the push must NOT land under the caller's own session id") +} + // TestToolCall_SessionCWDReachesHandler pins the other local half of the // same finding: handleToolCall must attach the resolved cwd to ctx via // gortexmcp.WithSessionCWD so a tool handler can read (and enforce) the