From dd93477d57cbe407449f7339545a3b40db7080e9 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sat, 25 Apr 2026 18:48:56 +0000 Subject: [PATCH 1/2] fix(mcp): propagate cfg.DefaultTenant to MCP fallback (RAN-22) Startup never called mcpServer.SetDefaultTenant, so header-less MCP requests fell back to the hardcoded "default" tenant via storage.DefaultTenantID -- ignoring DEFAULT_TENANT in deployments that override it. Wire cfg.DefaultTenant in at MCP construction and surface the resolved value in the startup log line. Add an HTTP-transport unit test (TestSetDefaultTenant_PropagatesToHTTPTransport) that exercises the actual fallback path through ServeHTTP: it asserts the constructor default, the no-op semantics of SetDefaultTenant(""), and that a header-less tools/call for find_similar_logs scopes to the configured tenant. The explicit-header path is also re-checked to confirm precedence (header wins over default) was not inverted. The startup vector hydration concern in the original ticket is moot: RAN-20 replaced the bare-appCtx tenant-scoped GetLogsV2 call with ListRecentHighSeverityLogsAllTenants, which is cross-tenant by design and tags each row with its own TenantID via vectorIdx.Add. Co-Authored-By: Paperclip --- internal/mcp/server_ran22_test.go | 117 ++++++++++++++++++++++++++++++ main.go | 3 +- 2 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 internal/mcp/server_ran22_test.go diff --git a/internal/mcp/server_ran22_test.go b/internal/mcp/server_ran22_test.go new file mode 100644 index 0000000..b37b2e5 --- /dev/null +++ b/internal/mcp/server_ran22_test.go @@ -0,0 +1,117 @@ +package mcp + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/RandomCodeSpace/otelcontext/internal/storage" + "github.com/RandomCodeSpace/otelcontext/internal/vectordb" +) + +// TestSetDefaultTenant_PropagatesToHTTPTransport is the RAN-22 acceptance bar: +// after startup wiring calls SetDefaultTenant(cfg.DefaultTenant), a header-less +// MCP tools/call must resolve to the configured tenant rather than the +// hardcoded storage.DefaultTenantID. Also asserts the no-op semantics for +// SetDefaultTenant("") so we don't regress the documented contract. +func TestSetDefaultTenant_PropagatesToHTTPTransport(t *testing.T) { + idx := vectordb.New(100) + idx.Add(1, "acme", "checkout", "ERROR", "payment gateway timeout acme-marker-xyz") + idx.Add(2, "globex", "auth", "ERROR", "payment gateway 500 globex-marker-qqq") + idx.Add(3, "default", "svc", "ERROR", "payment gateway refused default-marker-aaa") + + srv := New(nil, nil, nil, idx) + if srv.defaultTenant != storage.DefaultTenantID { + t.Fatalf("constructor default = %q, want %q", srv.defaultTenant, storage.DefaultTenantID) + } + + // Empty argument is a no-op — preserves prior value. + srv.SetDefaultTenant("") + if srv.defaultTenant != storage.DefaultTenantID { + t.Fatalf(`SetDefaultTenant("") clobbered field to %q`, srv.defaultTenant) + } + + body := mustMarshalJSONRPC(t, "find_similar_logs", map[string]any{ + "query": "payment gateway", + "limit": float64(50), + }) + + // Step 1: configure non-default tenant; no header → should be acme-scoped. + srv.SetDefaultTenant("acme") + if srv.defaultTenant != "acme" { + t.Fatalf(`SetDefaultTenant("acme") = %q`, srv.defaultTenant) + } + resp1 := callNoHeader(t, srv, body) + mustContain(t, resp1, "acme-marker-xyz") + mustNotContain(t, resp1, "globex-marker-qqq", "default-marker-aaa") + + // Step 2: change configured tenant; same header-less call must follow. + srv.SetDefaultTenant("globex") + resp2 := callNoHeader(t, srv, body) + mustContain(t, resp2, "globex-marker-qqq") + mustNotContain(t, resp2, "acme-marker-xyz", "default-marker-aaa") + + // Step 3: explicit X-Tenant-ID header must still win over the configured + // default — proves the fix did not invert tenant precedence. + resp3 := callWithHeader(t, srv, body, "acme") + mustContain(t, resp3, "acme-marker-xyz") + mustNotContain(t, resp3, "globex-marker-qqq", "default-marker-aaa") +} + +func mustMarshalJSONRPC(t *testing.T, tool string, args map[string]any) []byte { + t.Helper() + b, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": map[string]any{"name": tool, "arguments": args}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} + +func callNoHeader(t *testing.T, srv *Server, body []byte) string { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + srv.Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("HTTP %d: %s", rr.Code, rr.Body.String()) + } + return rr.Body.String() +} + +func callWithHeader(t *testing.T, srv *Server, body []byte, tenant string) string { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Tenant-ID", tenant) + rr := httptest.NewRecorder() + srv.Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("HTTP %d: %s", rr.Code, rr.Body.String()) + } + return rr.Body.String() +} + +func mustContain(t *testing.T, body, want string) { + t.Helper() + if !strings.Contains(body, want) { + t.Fatalf("response missing expected marker %q:\n%s", want, body) + } +} + +func mustNotContain(t *testing.T, body string, forbidden ...string) { + t.Helper() + for _, f := range forbidden { + if strings.Contains(body, f) { + t.Fatalf("response leaked forbidden marker %q:\n%s", f, body) + } + } +} diff --git a/main.go b/main.go index 83f96cb..afdb491 100644 --- a/main.go +++ b/main.go @@ -378,8 +378,9 @@ func main() { // 6b. Initialize MCP Server (HTTP Streamable, JSON-RPC 2.0 + SSE) mcpServer := mcp.New(repo, metrics, svcGraph, vectorIdx) + mcpServer.SetDefaultTenant(cfg.DefaultTenant) mcpServer.SetGraphRAG(graphRAG) - slog.Info("🤖 MCP server initialized", "path", cfg.MCPPath, "enabled", cfg.MCPEnabled) + slog.Info("🤖 MCP server initialized", "path", cfg.MCPPath, "enabled", cfg.MCPEnabled, "default_tenant", cfg.DefaultTenant) // 7. Initialize OTLP Ingestion (gRPC) traceServer := ingest.NewTraceServer(repo, metrics, cfg) From 5d71d446219cfd69fdf327a85850f14bb4c2c65b Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sat, 25 Apr 2026 18:57:56 +0000 Subject: [PATCH 2/2] refactor(mcp): require defaultTenant at New(), make wiring compiler-enforced Address review feedback on RAN-22 (PR #33): the previous test only exercised SetDefaultTenant, which existed before this branch -- so removing the main.go wiring would not have failed any test. Move defaultTenant from a post-construction setter to a required leading parameter on mcp.New. This turns the wiring contract into a compile-time check: removing or omitting cfg.DefaultTenant in main.go is now a build failure, not a silent regression. The runtime SetDefaultTenant setter is retained for future config-reload paths. Tests: - TestNew_DefaultTenant_FromConstructor exercises the constructor's fallback semantics (empty -> storage.DefaultTenantID; non-empty preserved verbatim) and confirms SetDefaultTenant runtime override still works without clobbering on empty input. - TestNew_DefaultTenant_FlowsThroughHTTPTransport drives the JSON-RPC HTTP handler with no X-Tenant-ID header on a server built via New("acme", ...) and asserts acme-scoped results, header precedence over the default, and that SetDefaultTenant runtime override flows through the same transport path. internal/mcp/tenant_isolation_test.go updated for the new signature (passes "" to preserve existing storage.DefaultTenantID semantics). Co-Authored-By: Paperclip --- internal/mcp/server.go | 17 ++++-- internal/mcp/server_ran22_test.go | 81 +++++++++++++++++---------- internal/mcp/tenant_isolation_test.go | 2 +- main.go | 3 +- 4 files changed, 66 insertions(+), 37 deletions(-) diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 4aa6e9d..de247f7 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -41,24 +41,33 @@ type Server struct { defaultTenant string } -// New creates a new MCP server. +// New creates a new MCP server. defaultTenant is the fallback tenant applied +// to header-less MCP requests; an empty string falls back to +// storage.DefaultTenantID. Required at construction time so production startup +// cannot accidentally drop cfg.DefaultTenant — a missing argument is a compile +// error rather than a silent regression. func New( + defaultTenant string, repo *storage.Repository, metrics *telemetry.Metrics, svcGraph *graph.Graph, vectorIdx *vectordb.Index, ) *Server { + if defaultTenant == "" { + defaultTenant = storage.DefaultTenantID + } return &Server{ repo: repo, metrics: metrics, svcGraph: svcGraph, vectorIdx: vectorIdx, - defaultTenant: storage.DefaultTenantID, + defaultTenant: defaultTenant, } } -// SetDefaultTenant overrides the fallback tenant used when an MCP request -// carries no X-Tenant-ID header. Call from startup wiring with cfg.DefaultTenant. +// SetDefaultTenant overrides the fallback tenant at runtime. Empty strings are +// ignored so callers can pass through optional config without clobbering the +// constructor-provided value. func (s *Server) SetDefaultTenant(t string) { if t != "" { s.defaultTenant = t diff --git a/internal/mcp/server_ran22_test.go b/internal/mcp/server_ran22_test.go index b37b2e5..37f6de6 100644 --- a/internal/mcp/server_ran22_test.go +++ b/internal/mcp/server_ran22_test.go @@ -12,53 +12,74 @@ import ( "github.com/RandomCodeSpace/otelcontext/internal/vectordb" ) -// TestSetDefaultTenant_PropagatesToHTTPTransport is the RAN-22 acceptance bar: -// after startup wiring calls SetDefaultTenant(cfg.DefaultTenant), a header-less -// MCP tools/call must resolve to the configured tenant rather than the -// hardcoded storage.DefaultTenantID. Also asserts the no-op semantics for -// SetDefaultTenant("") so we don't regress the documented contract. -func TestSetDefaultTenant_PropagatesToHTTPTransport(t *testing.T) { +// TestNew_DefaultTenant_FromConstructor is the RAN-22 regression bar at the +// type level: the configured default tenant is a required leading parameter +// to New, so production startup wiring (main.go) cannot drop it without a +// compile error. Empty input falls back to storage.DefaultTenantID; a +// non-empty value is preserved verbatim. +func TestNew_DefaultTenant_FromConstructor(t *testing.T) { + t.Run("empty falls back to storage.DefaultTenantID", func(t *testing.T) { + srv := New("", nil, nil, nil, vectordb.New(1)) + if srv.defaultTenant != storage.DefaultTenantID { + t.Fatalf(`New("") defaultTenant = %q, want %q`, srv.defaultTenant, storage.DefaultTenantID) + } + }) + t.Run("non-empty value is preserved", func(t *testing.T) { + srv := New("acme", nil, nil, nil, vectordb.New(1)) + if srv.defaultTenant != "acme" { + t.Fatalf(`New("acme") defaultTenant = %q, want "acme"`, srv.defaultTenant) + } + }) + t.Run("SetDefaultTenant runtime override still works", func(t *testing.T) { + srv := New("acme", nil, nil, nil, vectordb.New(1)) + srv.SetDefaultTenant("globex") + if srv.defaultTenant != "globex" { + t.Fatalf(`SetDefaultTenant("globex") defaultTenant = %q, want "globex"`, srv.defaultTenant) + } + // Empty argument is a no-op so optional config doesn't clobber. + srv.SetDefaultTenant("") + if srv.defaultTenant != "globex" { + t.Fatalf(`SetDefaultTenant("") clobbered field to %q`, srv.defaultTenant) + } + }) +} + +// TestNew_DefaultTenant_FlowsThroughHTTPTransport proves that the constructor- +// supplied tenant is the actual fallback used by the JSON-RPC HTTP handler +// when no X-Tenant-ID header is present, and that an explicit header still +// wins over the default. This locks in the end-to-end behavior the RAN-22 +// fix delivers: a deployment with DEFAULT_TENANT=acme returns acme-scoped +// data from header-less MCP tool calls. +func TestNew_DefaultTenant_FlowsThroughHTTPTransport(t *testing.T) { idx := vectordb.New(100) idx.Add(1, "acme", "checkout", "ERROR", "payment gateway timeout acme-marker-xyz") idx.Add(2, "globex", "auth", "ERROR", "payment gateway 500 globex-marker-qqq") idx.Add(3, "default", "svc", "ERROR", "payment gateway refused default-marker-aaa") - srv := New(nil, nil, nil, idx) - if srv.defaultTenant != storage.DefaultTenantID { - t.Fatalf("constructor default = %q, want %q", srv.defaultTenant, storage.DefaultTenantID) - } - - // Empty argument is a no-op — preserves prior value. - srv.SetDefaultTenant("") - if srv.defaultTenant != storage.DefaultTenantID { - t.Fatalf(`SetDefaultTenant("") clobbered field to %q`, srv.defaultTenant) - } - body := mustMarshalJSONRPC(t, "find_similar_logs", map[string]any{ "query": "payment gateway", "limit": float64(50), }) - // Step 1: configure non-default tenant; no header → should be acme-scoped. - srv.SetDefaultTenant("acme") - if srv.defaultTenant != "acme" { - t.Fatalf(`SetDefaultTenant("acme") = %q`, srv.defaultTenant) - } + srv := New("acme", nil, nil, nil, idx) + + // Header-less tools/call must scope to the constructor-provided default. resp1 := callNoHeader(t, srv, body) mustContain(t, resp1, "acme-marker-xyz") mustNotContain(t, resp1, "globex-marker-qqq", "default-marker-aaa") - // Step 2: change configured tenant; same header-less call must follow. - srv.SetDefaultTenant("globex") - resp2 := callNoHeader(t, srv, body) + // Explicit X-Tenant-ID header beats the configured default — precedence + // invariant is preserved. + resp2 := callWithHeader(t, srv, body, "globex") mustContain(t, resp2, "globex-marker-qqq") mustNotContain(t, resp2, "acme-marker-xyz", "default-marker-aaa") - // Step 3: explicit X-Tenant-ID header must still win over the configured - // default — proves the fix did not invert tenant precedence. - resp3 := callWithHeader(t, srv, body, "acme") - mustContain(t, resp3, "acme-marker-xyz") - mustNotContain(t, resp3, "globex-marker-qqq", "default-marker-aaa") + // SetDefaultTenant runtime override flows to the same transport path so + // future runtime-config-reload paths behave correctly. + srv.SetDefaultTenant("globex") + resp3 := callNoHeader(t, srv, body) + mustContain(t, resp3, "globex-marker-qqq") + mustNotContain(t, resp3, "acme-marker-xyz", "default-marker-aaa") } func mustMarshalJSONRPC(t *testing.T, tool string, args map[string]any) []byte { diff --git a/internal/mcp/tenant_isolation_test.go b/internal/mcp/tenant_isolation_test.go index 653ab90..bb4d5cc 100644 --- a/internal/mcp/tenant_isolation_test.go +++ b/internal/mcp/tenant_isolation_test.go @@ -94,7 +94,7 @@ func setupTenantIsolationServer(t *testing.T) (*httptest.Server, *graphrag.Graph bgCtx, cancel := context.WithCancel(context.Background()) go g.Start(bgCtx) - srv := New(repo, nil, nil, vIdx) + srv := New("", repo, nil, nil, vIdx) srv.SetGraphRAG(g) httpSrv := httptest.NewServer(srv.Handler()) diff --git a/main.go b/main.go index afdb491..bafd4dc 100644 --- a/main.go +++ b/main.go @@ -377,8 +377,7 @@ func main() { apiServer.SetVectorIndex(vectorIdx) // 6b. Initialize MCP Server (HTTP Streamable, JSON-RPC 2.0 + SSE) - mcpServer := mcp.New(repo, metrics, svcGraph, vectorIdx) - mcpServer.SetDefaultTenant(cfg.DefaultTenant) + mcpServer := mcp.New(cfg.DefaultTenant, repo, metrics, svcGraph, vectorIdx) mcpServer.SetGraphRAG(graphRAG) slog.Info("🤖 MCP server initialized", "path", cfg.MCPPath, "enabled", cfg.MCPEnabled, "default_tenant", cfg.DefaultTenant)