From 2ce0d5c3d4354c72379d5257629d135fe26787a9 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 18:53:53 +0300 Subject: [PATCH 1/7] feat(index): add SearchToolsAdmitted hit-level admission predicate Spec 108 (Profiles v3) FR-011: SearchToolsScoped's predicate only sees a hit's server name, so a caller with a tool-level policy could have a policy-excluded hit displace an admitted one from a limited window, and hidden_by_profile would undercount. SearchToolsAdmitted extends the index API with a hit-level predicate over the canonical (server, tool) identity, returning a three-way Admission (Admit / RejectScope / RejectPolicy) instead of a bool: the limit applies to admitted hits only, and RejectPolicy (never RejectScope) is counted over the full exhaustive match set. The index stores no annotations and gains none; the predicate's tier classification comes from the caller. --- internal/index/bleve.go | 97 ++++++++++++ internal/index/manager.go | 15 ++ internal/index/search_admitted_test.go | 197 +++++++++++++++++++++++++ 3 files changed, 309 insertions(+) create mode 100644 internal/index/search_admitted_test.go diff --git a/internal/index/bleve.go b/internal/index/bleve.go index 885581220..666b44bb5 100644 --- a/internal/index/bleve.go +++ b/internal/index/bleve.go @@ -726,6 +726,103 @@ func (b *BleveIndex) SearchToolsScoped(queryStr string, limit int, inScope func( return results, nil } +// Hit is the canonical registration identity SearchToolsAdmitted hands to its +// predicate (Spec 108 FR-011, data-model.md §2): the exact (server, raw tool) +// pair a hit resolves to, NEVER a stored annotation — the index carries no +// annotation field and gains none (ToolDocument/readToolMetadata are +// unchanged; research.md "Annotation source for SearchToolsAdmitted"). A +// caller that needs the tool's effective annotations resolves them itself, +// through the same identity seam every dispatch path already uses +// (profile.EffectiveAnnotations = resolveExactToolIdentity), keeping +// discovery and execution classifying from one source. +type Hit struct { + Server string + Tool string +} + +// Admission is admit's per-hit verdict for SearchToolsAdmitted. +type Admission int + +const ( + // Admit means the hit is visible to this caller and counts toward limit. + Admit Admission = iota + // RejectScope means the hit's server is outside the caller's effective + // scope (agent-token allowed_servers, profile server set). Never counted + // in hiddenByPolicy — an out-of-scope tool must stay invisible, not merely + // "hidden by profile" (FR-011). + RejectScope + // RejectPolicy means the hit's server was in scope but the tool policy + // (Spec 108 CompiledPolicy.Decide) excluded it. Counted in hiddenByPolicy. + RejectPolicy +) + +// SearchToolsAdmitted is SearchToolsScoped's hit-level counterpart (Spec 108 +// FR-011, data-model.md §2): the pre-limit predicate sees the hit's canonical +// (server, tool) identity — never a stored annotation, the index carries +// none — and returns one of three verdicts instead of a bool, so the caller +// can tell an out-of-scope rejection (never counted, stays invisible) from a +// policy-only one (counted in hiddenByPolicy, the FR-011 `hidden_by_profile` +// figure) over the SAME exhaustive pre-limit scan SearchToolsScoped already +// runs. limit is applied to ADMITTED hits only — a rejected top hit, whatever +// its reason, never shortens the page — and hiddenByPolicy accumulates over +// the full match set, not merely the hits collected before the cut, so a +// caller whose page filled up before scanning every match still gets an +// accurate count. +// +// Identical to SearchToolsScoped in every other respect (same query +// construction incl. the underscore-segment enhancement, same score-then-id +// sort, same exhaustive From/Size paging with no result cap) — a predicate +// that only ever returns Admit/RejectScope (never RejectPolicy) makes this +// method equal SearchToolsScoped(query, limit, func(s string) bool { admit +// still sees the server only }) exactly (T016a). +func (b *BleveIndex) SearchToolsAdmitted(queryStr string, limit int, admit func(Hit) Admission) (results []*config.SearchResult, hiddenByPolicy int, err error) { + if queryStr == "" { + return nil, 0, fmt.Errorf("search query cannot be empty") + } + if admit == nil || limit <= 0 { + return []*config.SearchResult{}, 0, nil + } + + q, err := b.augmentedToolSearchQuery(queryStr, limit) + if err != nil { + return nil, 0, err + } + pageSize := limit + if pageSize < scopedSearchMinPage { + pageSize = scopedSearchMinPage + } + + b.logger.Debug("Searching tools with admitted query", zap.String("query", queryStr), zap.Int("limit", limit)) + + results = make([]*config.SearchResult, 0, limit) + for from := 0; ; from += pageSize { + searchResult, err := b.index.Search(newToolSearchRequest(q, from, pageSize)) + if err != nil { + return nil, 0, fmt.Errorf("search failed: %w", err) + } + for _, hit := range searchResult.Hits { + tool := readToolMetadata(hit.ID, hit.Fields) + switch admit(Hit{Server: tool.ServerName, Tool: config.RawToolName(tool)}) { + case Admit: + results = append(results, &config.SearchResult{Tool: tool, Score: hit.Score}) + case RejectPolicy: + hiddenByPolicy++ + case RejectScope: + // Invisible: never counted, never collected. + } + if len(results) >= limit { + return results, hiddenByPolicy, nil + } + } + if len(searchResult.Hits) == 0 || uint64(from+pageSize) >= searchResult.Total { + break + } + } + + b.logger.Debug("Found admitted tools matching query", zap.Int("count", len(results)), zap.Int("hidden_by_policy", hiddenByPolicy), zap.String("query", queryStr)) + return results, hiddenByPolicy, nil +} + func fieldsContainExactToolName(fields map[string]interface{}, queryStr string) bool { for _, field := range []string{"tool_name", "full_tool_name"} { if value, ok := fields[field].(string); ok && value == queryStr { diff --git a/internal/index/manager.go b/internal/index/manager.go index 7c723ebb8..378858cc0 100644 --- a/internal/index/manager.go +++ b/internal/index/manager.go @@ -126,6 +126,21 @@ func (m *Manager) SearchToolsScoped(query string, limit int, inScope func(server return m.bleveIndex.SearchToolsScoped(query, limit, inScope) } +// SearchToolsAdmitted is SearchToolsScoped's hit-level counterpart (Spec 108 +// FR-011): the predicate resolves per-hit Admission (Admit/RejectScope/ +// RejectPolicy) over the canonical (server, tool) identity. See +// BleveIndex.SearchToolsAdmitted. +func (m *Manager) SearchToolsAdmitted(query string, limit int, admit func(Hit) Admission) ([]*config.SearchResult, int, error) { + m.mu.RLock() + defer m.mu.RUnlock() + + if limit <= 0 { + limit = 20 // default limit, as SearchTools + } + + return m.bleveIndex.SearchToolsAdmitted(query, limit, admit) +} + // Search searches for tools matching the query (alias for SearchTools) func (m *Manager) Search(query string, limit int) ([]*config.SearchResult, error) { return m.SearchTools(query, limit) diff --git a/internal/index/search_admitted_test.go b/internal/index/search_admitted_test.go new file mode 100644 index 000000000..43ff9df6c --- /dev/null +++ b/internal/index/search_admitted_test.go @@ -0,0 +1,197 @@ +package index + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Spec 108 (Profiles v3) T016a/T022: SearchToolsAdmitted is SearchToolsScoped's +// hit-level counterpart — the predicate sees the hit's canonical (server, +// tool) identity, not merely its server, and returns a three-way Admission so +// the caller can separate "invisible" (RejectScope, never counted) from +// "hidden by profile" (RejectPolicy, counted) over one exhaustive pre-limit +// scan (data-model.md §2, FR-011). + +// admittedSeamQuery mirrors scopedSeamQuery's shape: one entitled, low- +// frequency hit vs a pile of hidden, high-frequency ones, so an unlimited +// scan is required to find the entitled hit at all. +const admittedSeamQuery = "widget" + +func buildAdmittedSeamCorpus(t *testing.T, hiddenCount int) *BleveIndex { + t.Helper() + + idx, err := NewBleveIndex(t.TempDir(), zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = idx.Close() }) + + entitled := &config.ToolMetadata{ + Name: "a:status_reader", + ServerName: "a", + Description: "widget status check", + ParamsJSON: "{}", + Hash: "admitted-entitled", + } + require.NoError(t, idx.IndexTool(entitled)) + + // One tool that is IN SCOPE (server "a") but must be excluded by POLICY — + // distinct from the hidden "b" server population below, so a test can + // tell RejectPolicy from RejectScope. + policyExcluded := &config.ToolMetadata{ + Name: "a:delete_everything", + ServerName: "a", + Description: "widget delete_everything", + ParamsJSON: "{}", + Hash: "admitted-policy-excluded", + } + require.NoError(t, idx.IndexTool(policyExcluded)) + + hidden := make([]*config.ToolMetadata, 0, hiddenCount) + for i := 0; i < hiddenCount; i++ { + hidden = append(hidden, &config.ToolMetadata{ + Name: fmt.Sprintf("b:tool_%d", i), + ServerName: "b", + Description: "widget widget widget widget widget widget widget widget widget widget", + ParamsJSON: "{}", + Hash: fmt.Sprintf("admitted-hidden-%d", i), + }) + } + require.NoError(t, idx.BatchIndex(hidden)) + + return idx +} + +// serverOnlyAdmit builds an admit func that only ever inspects Hit.Server — +// the SearchToolsAdmitted equivalent of SearchToolsScoped's inScope closure — +// so the two methods can be compared for equality (T016a "equals +// SearchToolsScoped when the predicate only checks the server"). +func serverOnlyAdmit(inScope func(server string) bool) func(Hit) Admission { + return func(h Hit) Admission { + if inScope(h.Server) { + return Admit + } + return RejectScope + } +} + +func TestSearchToolsAdmitted_FiltersBeforeLimitLikeScoped(t *testing.T) { + idx := buildAdmittedSeamCorpus(t, 50) + + // Control: the unscoped window really is a "b" hit. + unscoped, err := idx.SearchTools(admittedSeamQuery, 1) + require.NoError(t, err) + require.Len(t, unscoped, 1) + require.Equal(t, "b", unscoped[0].Tool.ServerName, "fixture: hidden server must outrank the entitled one") + + inScopeA := func(server string) bool { return server == "a" } + + t.Run("equals SearchToolsScoped when the predicate only checks the server", func(t *testing.T) { + scoped, err := idx.SearchToolsScoped(admittedSeamQuery, 10, inScopeA) + require.NoError(t, err) + + admitted, hiddenByPolicy, err := idx.SearchToolsAdmitted(admittedSeamQuery, 10, serverOnlyAdmit(inScopeA)) + require.NoError(t, err) + require.Equal(t, 0, hiddenByPolicy, "a predicate that never returns RejectPolicy must never count anything") + + require.Len(t, admitted, len(scoped)) + for i := range scoped { + assert.Equal(t, scoped[i].Tool.Name, admitted[i].Tool.Name) + assert.Equal(t, scoped[i].Score, admitted[i].Score) + } + }) + + t.Run("limit applies to admitted hits only: a policy-rejected top hit never shortens the page", func(t *testing.T) { + // admit: server "a" only, AND exclude "delete_everything" by policy. + admit := func(h Hit) Admission { + if h.Server != "a" { + return RejectScope + } + if h.Tool == "delete_everything" { + return RejectPolicy + } + return Admit + } + results, hiddenByPolicy, err := idx.SearchToolsAdmitted(admittedSeamQuery, 1, admit) + require.NoError(t, err) + require.Len(t, results, 1, "the admitted hit must fill the page despite a higher/rejected candidate ahead of it") + assert.Equal(t, "a:status_reader", results[0].Tool.Name) + assert.Equal(t, 1, hiddenByPolicy, "the excluded in-scope tool must be counted") + }) + + t.Run("RejectScope is never counted in hiddenByPolicy", func(t *testing.T) { + _, hiddenByPolicy, err := idx.SearchToolsAdmitted(admittedSeamQuery, 10, serverOnlyAdmit(inScopeA)) + require.NoError(t, err) + assert.Equal(t, 0, hiddenByPolicy, "the 50 out-of-scope 'b' hits must never inflate hiddenByPolicy") + }) + + t.Run("predicate receives the canonical (server, tool) identity, never a bare doc id", func(t *testing.T) { + var seen []Hit + admit := func(h Hit) Admission { + seen = append(seen, h) + return RejectScope + } + _, _, err := idx.SearchToolsAdmitted(admittedSeamQuery, 10, admit) + require.NoError(t, err) + require.NotEmpty(t, seen) + for _, h := range seen { + assert.NotEmpty(t, h.Server) + assert.NotEmpty(t, h.Tool) + assert.NotContains(t, h.Tool, ":", "Tool must be the RAW name, never the canonical server:tool id") + } + }) + + t.Run("pages exhaustively: the entitled hit is found beyond the first page", func(t *testing.T) { + bigIdx := buildAdmittedSeamCorpus(t, scopedSearchMinPage+25) + admitOnlyStatusReader := func(h Hit) Admission { + if h.Server != "a" { + return RejectScope + } + if h.Tool != "status_reader" { + return RejectPolicy + } + return Admit + } + results, hiddenByPolicy, err := bigIdx.SearchToolsAdmitted(admittedSeamQuery, 1, admitOnlyStatusReader) + require.NoError(t, err) + require.Len(t, results, 1) + assert.Equal(t, "a:status_reader", results[0].Tool.Name) + assert.Equal(t, 1, hiddenByPolicy, "the other in-scope 'a' tool must be counted, out-of-scope 'b' hits must not") + }) +} + +func TestSearchToolsAdmitted_EmptyQueryOrNilPredicate(t *testing.T) { + idx := buildAdmittedSeamCorpus(t, 1) + + _, _, err := idx.SearchToolsAdmitted("", 10, serverOnlyAdmit(func(string) bool { return true })) + require.Error(t, err) + + results, hidden, err := idx.SearchToolsAdmitted(admittedSeamQuery, 10, nil) + require.NoError(t, err) + assert.Empty(t, results) + assert.Equal(t, 0, hidden) +} + +// TestManager_SearchToolsAdmitted pins the Manager-level delegation (T022). +func TestManager_SearchToolsAdmitted(t *testing.T) { + m, err := NewManager(t.TempDir(), zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = m.Close() }) + + require.NoError(t, m.IndexTool(&config.ToolMetadata{ + Name: "a:status_reader", ServerName: "a", Description: "widget status check", ParamsJSON: "{}", + })) + require.NoError(t, m.IndexTool(&config.ToolMetadata{ + Name: "b:other", ServerName: "b", Description: "widget widget widget widget widget", ParamsJSON: "{}", + })) + + results, hidden, err := m.SearchToolsAdmitted(admittedSeamQuery, 10, serverOnlyAdmit(func(s string) bool { return s == "a" })) + require.NoError(t, err) + require.Len(t, results, 1) + assert.Equal(t, "a:status_reader", results[0].Tool.Name) + assert.Equal(t, 0, hidden) +} From 1a7d6003db3b43f7e6a11f125ab3a63a7b25a698 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 18:54:06 +0300 Subject: [PATCH 2/7] feat(profile): enforce Spec 108 tool policy across discovery surfaces PR 108-b (profile-discovery-enforcement), FR-010/011/012/015/019. retrieve_tools now filters by tool policy before the ranked cut via SearchToolsAdmitted, gains hidden_by_profile (non-legacy profiles only, SC-003 byte parity preserved) and profile (url/session sources only, never a pin). describe_tool (indexed and direct surfaces) answers an excluded tool with the same uniform not-found response a nonexistent tool gets. /mcp/all tools/list drops policy-excluded tools at list time only, leaving the call-time filter chain untouched so a future call-time gate (108-d) still reaches the handler. indexedToolVisible gets the identical policy check as post-cut defence in depth. resolveActiveProfileWithSource extends the existing resolver to also report which precedence tier (pin/url/session) produced the effective profile, needed to decide whether the new `profile` field may name the slug at all. Prompts are unaffected by design (FR-019): filterAggregatedPromptsForAuth consults only server scope, never the tool policy. Policy exercised only under the FR-009a test override until 108-d lifts the rollout gate. --- .../server/describe_tool_profile_v3_test.go | 59 +++++ internal/server/direct_profile_v3_test.go | 62 +++++ internal/server/mcp.go | 84 ++++++- internal/server/mcp_describe_direct.go | 22 +- internal/server/mcp_direct_scope.go | 29 ++- internal/server/mcp_visibility.go | 48 +++- .../server/preflight_dispatch_parity_test.go | 2 +- internal/server/profile_resolver.go | 38 ++- .../server/profiles_v3_index_fixture_test.go | 79 ++++++ internal/server/prompts_profile_v3_test.go | 55 +++++ .../retrieve_tools_profile_v3_golden_test.go | 29 +++ .../server/retrieve_tools_profile_v3_test.go | 230 ++++++++++++++++++ .../server/scope_latency_profile_v3_test.go | 102 ++++++++ internal/server/scope_oracle_v3_test.go | 130 ++++++++++ .../retrieve_tools_profile_v3.golden.json | 1 + 15 files changed, 951 insertions(+), 19 deletions(-) create mode 100644 internal/server/describe_tool_profile_v3_test.go create mode 100644 internal/server/direct_profile_v3_test.go create mode 100644 internal/server/profiles_v3_index_fixture_test.go create mode 100644 internal/server/prompts_profile_v3_test.go create mode 100644 internal/server/retrieve_tools_profile_v3_golden_test.go create mode 100644 internal/server/retrieve_tools_profile_v3_test.go create mode 100644 internal/server/scope_latency_profile_v3_test.go create mode 100644 internal/server/scope_oracle_v3_test.go create mode 100644 internal/server/testdata/retrieve_tools_profile_v3.golden.json diff --git a/internal/server/describe_tool_profile_v3_test.go b/internal/server/describe_tool_profile_v3_test.go new file mode 100644 index 000000000..f59955d41 --- /dev/null +++ b/internal/server/describe_tool_profile_v3_test.go @@ -0,0 +1,59 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Spec 108 (Profiles v3) T018: describe_tool on a profile-excluded tool must +// answer the SAME uniform not-found response a nonexistent tool id gets +// (Spec 105 FR-010 comparison; contracts/mcp-tools.md "describe_tool" — "No +// shape change"), so a caller can never distinguish "excluded by policy" +// from "does not exist". +func TestDescribeTool_ProfileV3_ExcludedEqualsNonexistent(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + ctx := urlProfileCtx(proxy, "work-readonly") + + excluded := callDescribe(t, proxy, ctx, []interface{}{"github:create_issue"}) + require.Empty(t, excluded.Definitions, "an excluded tool must never render its definition") + require.Len(t, excluded.Errors, 1) + + nonexistent := callDescribe(t, proxy, ctx, []interface{}{"github:does_not_exist_at_all"}) + require.Empty(t, nonexistent.Definitions) + require.Len(t, nonexistent.Errors, 1) + + // Compare after substituting the echoed id — everything else (error + // code, remediation text) must be byte-identical. + excluded.Errors[0]["id"] = "SUBSTITUTED" + nonexistent.Errors[0]["id"] = "SUBSTITUTED" + assert.Equal(t, nonexistent.Errors[0], excluded.Errors[0], + "an excluded tool's describe_tool error must be byte-identical to a nonexistent one's") + + t.Run("admitted tool under the same profile still describes normally", func(t *testing.T) { + resp := callDescribe(t, proxy, ctx, []interface{}{"github:list_issues"}) + require.Empty(t, resp.Errors) + require.Len(t, resp.Definitions, 1) + assert.Equal(t, "github:list_issues", resp.Definitions[0]["name"]) + }) + + t.Run("legacy profile: excluded-vs-nonexistent distinction does not apply — nothing is policy-excluded", func(t *testing.T) { + legacyCtx := urlProfileCtx(proxy, "legacy") + resp := callDescribe(t, proxy, legacyCtx, []interface{}{"github:create_issue"}) + require.Empty(t, resp.Errors) + require.Len(t, resp.Definitions, 1, "legacy has no policy field set — create_issue is simply admitted") + }) + + t.Run("pin source: the same uniform not-found, without ever naming the profile", func(t *testing.T) { + resp := callDescribe(t, proxy, pinnedProfileCtx("work-readonly"), []interface{}{"github:create_issue"}) + require.Len(t, resp.Errors, 1) + for _, v := range resp.Errors[0] { + if s, ok := v.(string); ok { + assert.NotContains(t, s, "work-readonly", "the refusal must never name the profile") + } + } + }) +} diff --git a/internal/server/direct_profile_v3_test.go b/internal/server/direct_profile_v3_test.go new file mode 100644 index 000000000..4a86776fd --- /dev/null +++ b/internal/server/direct_profile_v3_test.go @@ -0,0 +1,62 @@ +package server + +import ( + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" +) + +// Spec 108 (Profiles v3) T019: `/mcp/all` `tools/list` omits excluded tools +// for a caller with an effective profile — filterDirectModeToolsForAuth's +// STAMPED branch (the production path every real upstream tool takes; see +// mcp_direct_scope.go's own doc comment on the unstamped fallback being +// test-only) now also runs the FR-011 policy decision, LIST TIME ONLY. +func directStampedTool(server, rawName, tier string) mcp.Tool { + entry := &directCatalogEntry{ServerName: server, ToolName: rawName, RequiredPermission: tier} + return stampDirectTool(mcp.Tool{Name: server + "__" + rawName}, entry) +} + +func TestFilterDirectModeToolsForAuth_ProfileV3(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + + tools := []mcp.Tool{ + directStampedTool("github", "list_issues", "read"), + directStampedTool("github", "create_issue", "write"), + directStampedTool("notion", "update_page", "write"), + directStampedTool("filesystem", "read_text_file", "read"), + } + + t.Run("work-readonly: list-time filter excludes create_issue (tier cap) but keeps the allow-ruled notion:update_page", func(t *testing.T) { + ctx := urlProfileCtx(proxy, "work-readonly") + filtered := filterDirectToolNames(proxy.filterDirectModeToolsForAuth(ctx, tools)) + assert.ElementsMatch(t, []string{"github__list_issues", "notion__update_page"}, filtered) + }) + + t.Run("work-full: everything in scope is admitted", func(t *testing.T) { + ctx := urlProfileCtx(proxy, "work-full") + filtered := filterDirectToolNames(proxy.filterDirectModeToolsForAuth(ctx, tools)) + assert.ElementsMatch(t, []string{"github__list_issues", "github__create_issue", "notion__update_page", "filesystem__read_text_file"}, filtered) + }) + + t.Run("legacy: policy never excludes (only server scope does, which the fixture's legacy profile restricts to github)", func(t *testing.T) { + ctx := urlProfileCtx(proxy, "legacy") + filtered := filterDirectToolNames(proxy.filterDirectModeToolsForAuth(ctx, tools)) + assert.ElementsMatch(t, []string{"github__list_issues", "github__create_issue"}, filtered) + }) + + t.Run("call-time request: a policy-excluded tool must stay visible to the filter chain (108-d's own gate answers the call, not this filter)", func(t *testing.T) { + ctx := withDirectRequestKindBox(urlProfileCtx(proxy, "work-readonly")) + setDirectRequestKind(ctx, directRequestKindCall) + filtered := filterDirectToolNames(proxy.filterDirectModeToolsForAuth(ctx, []mcp.Tool{directStampedTool("github", "create_issue", "write")})) + assert.Equal(t, []string{"github__create_issue"}, filtered, "list-time exclusion must not leak into the call-time re-evaluation") + }) +} + +func filterDirectToolNames(tools []mcp.Tool) []string { + names := make([]string, 0, len(tools)) + for _, tl := range tools { + names = append(names, tl.Name) + } + return names +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 303cbc61d..9dcf4d755 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -30,6 +30,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/observability" "github.com/smart-mcp-proxy/mcpproxy-go/internal/outputvalidation" "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" @@ -1846,7 +1847,32 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // that is allowed to see nothing leave a new index directory behind — for a // profile that may no longer exist. The post-filter below returns the same // empty result set from the shared index. - profileName, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) + profileName, profileScope, profileIdx, profileSource := p.resolveActiveProfileWithSource(ctx) + // Spec 108 FR-011: the compiled policy for the effective profile, resolved + // once and reused by the admit predicate below, the response's + // hidden_by_profile/profile fields and nothing else — a dangling base + // (profileName set but no matching ProfileConfig in this snapshot, e.g. a + // stale legacy pin) has no CompiledPolicy to fetch and leaves policy nil; + // its server scope is already deny-all (profileScope), so the admit + // predicate excludes everything via RejectScope regardless. + var policy *profile.CompiledPolicy + if profileName != "" { + policy = profileIdx.PolicyFor(profileName) + } + // nonLegacyProfile gates BOTH new response fields (FR-011, SC-003 byte + // parity): a dangling base always counts as non-legacy (data-model.md §2 + // "Index API extension" / contracts/mcp-tools.md), and an existing + // ProfileConfig counts by its own IsLegacy() — Title/Description alone + // never flip it non-legacy, so a legacy profile that only sets a display + // title stays byte-identical to a nameless one. + nonLegacyProfile := false + if profileName != "" { + if pc := profileIdx.lookup(profileName); pc != nil { + nonLegacyProfile = !pc.IsLegacy() + } else { + nonLegacyProfile = true + } + } // Spec 104 FR-016a: the cache stamp is the authorization THIS search runs // under, captured now rather than re-resolved when the response is cut. // The index/snapshot pair is threaded through too (Spec 105 PR D review @@ -1910,10 +1936,40 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // plain Search — byte-for-byte with the pre-105 behaviour. useScopedSearch := auth.IsScopedCaller(ctx) || (profileName != "" && sharedIndexFallback) + // Spec 108 FR-011: whenever a profile is genuinely in effect (profileName + // != ""), search must filter by TOOL POLICY before the cut too, not only + // by server scope — otherwise a policy-excluded hit could displace an + // admitted one from the window and hidden_by_profile would undercount + // (plan.md "codex round 1"). This is the ONLY new branch: the plain + // unprofiled admin/token paths above are untouched, byte-identical to + // pre-108 (SC-003). CompiledPolicy.Decide is safe to run even for a + // LEGACY profile (Cap 0 admits every tier, no rules) — it can only ever + // agree with serverDiscoverable's own verdict for that case — so this + // branch does not need to special-case legacy; only the RESPONSE FIELDS + // below are gated on nonLegacyProfile. + admitPolicy := func(hit index.Hit) index.Admission { + if !serverDiscoverable(hit.Server) { + return index.RejectScope + } + if policy == nil { + return index.Admit + } + annotations, found := p.EffectiveAnnotations(hit.Server, hit.Tool) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(hit.Server, hit.Tool, intrinsic); !admitted { + return index.RejectPolicy + } + return index.Admit + } + var results []*config.SearchResult - if useScopedSearch { + var hiddenByPolicy int + switch { + case profileName != "": + results, hiddenByPolicy, err = searchIndex.SearchToolsAdmitted(query, limit, admitPolicy) + case useScopedSearch: results, err = searchIndex.SearchToolsScoped(query, limit, serverDiscoverable) - } else { + default: results, err = searchIndex.Search(query, limit) } if err != nil { @@ -1958,14 +2014,16 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // toolVisibleToSession, so it can never return a definition search // would not. Results are index hits by construction, so the // index-presence step is skipped here. - visible, reason := p.indexedToolVisible(authCtx, profileScope, serverName, toolName) + visible, reason := p.indexedToolVisible(authCtx, profileScope, policy, serverName, toolName) if visible { callableResults = append(callableResults, result) continue } - if reason == visReasonServerNotInScope { - // Out-of-scope servers are invisible, never "locked". + if reason == visReasonServerNotInScope || reason == visReasonToolPolicyExcluded { + // Out-of-scope servers and profile-excluded tools are both + // invisible, never "locked" — FR-013 forbids naming or + // describing a profile-hidden tool the way a locked entry would. continue } @@ -2119,6 +2177,20 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques "usage_instructions": usageInstructions, } + // Spec 108 FR-011: hidden_by_profile is present — possibly 0 — whenever + // the effective profile is non-legacy, and NEVER for no profile or a + // legacy one (SC-003 byte parity: a legacy profile, even one that sets + // only a display title, must not gain this field). profile names the + // slug only when the caller itself selected it (url/session) — never for + // a pin, so discovery can never confirm that a caller is pinned or to + // what (research D27). + if nonLegacyProfile { + response["hidden_by_profile"] = hiddenByPolicy + if profileSource == profile.SourceURL || profileSource == profile.SourceSession { + response["profile"] = profileName + } + } + // Spec 094 (FR-001): explain the annotation filters only when they actually // withheld something. On the happy path — no filters, or filters that // omitted nothing — the key is absent and the response stays byte-identical diff --git a/internal/server/mcp_describe_direct.go b/internal/server/mcp_describe_direct.go index c321eca6c..fb1eb5b2d 100644 --- a/internal/server/mcp_describe_direct.go +++ b/internal/server/mcp_describe_direct.go @@ -6,6 +6,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" "github.com/smart-mcp-proxy/mcpproxy-go/internal/toolannotations" ) @@ -154,13 +155,30 @@ func (p *MCPProxyServer) resolveDirectDescribeIDIn(ctx context.Context, cat *dir // operation-permission tier, and agent callability. func (p *MCPProxyServer) directEntryVisibleToSession(ctx context.Context, entry *directCatalogEntry) bool { authCtx := auth.AuthContextFromContext(ctx) - _, profileScope := p.resolveActiveProfile(ctx) + profileName, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) isScopedAgent := isScopeRestrictedCaller(authCtx) if !directEntryInScope(authCtx, profileScope, isScopedAgent, entry) { return false } - return p.directEntryCallable(authCtx, entry) + if !p.directEntryCallable(authCtx, entry) { + return false + } + // Spec 108 FR-011/T023: describe_tool on the direct surface must never + // return a definition for a profile-excluded tool. This function is + // describe-only (never consulted by the actual dispatch path), so unlike + // the list filter it needs no call-time exception — describe always + // applies the policy in full. + if profileName != "" { + if policy := profileIdx.PolicyFor(profileName); policy != nil { + annotations, found := p.EffectiveAnnotations(entry.ServerName, entry.ToolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(entry.ServerName, entry.ToolName, intrinsic); !admitted { + return false + } + } + } + return true } // suggestDirectToolID corrects an id that differs from a listed one only by diff --git a/internal/server/mcp_direct_scope.go b/internal/server/mcp_direct_scope.go index c4a63265f..b76237e70 100644 --- a/internal/server/mcp_direct_scope.go +++ b/internal/server/mcp_direct_scope.go @@ -302,6 +302,26 @@ func directCallTimeTierExceeded(ctx context.Context, authCtx *auth.AuthContext, return !authCtx.HasPermission(tier) } +// directPolicyExcludedForListing reports whether the Spec 108 tool policy +// excludes (owner, rawName) from the direct surface's LISTING only (T023, +// FR-011). It deliberately mirrors directCallTimeTierExceeded's own +// list/call-time split: at call time (isDirectCallTimeRequest) the tool must +// stay visible to mcp-go's WithToolFilter chain, or a policy-excluded call +// would be answered with the chain's generic "tool not found" instead of the +// descriptive profile refusal 108-d's own call-time gate provides +// (handleCallToolVariant's refusal, wired through the direct dispatch path) +// — exactly the reason an over-tier tool is let through here too instead of +// being excluded by this filter. +func (p *MCPProxyServer) directPolicyExcludedForListing(ctx context.Context, policy *profile.CompiledPolicy, owner, rawName string) bool { + if policy == nil || isDirectCallTimeRequest(ctx) { + return false + } + annotations, found := p.EffectiveAnnotations(owner, rawName) + intrinsic := profile.IntrinsicTier(annotations, found) + admitted, _, _ := policy.Decide(owner, rawName, intrinsic) + return !admitted +} + // directRequestKindMiddleware installs an empty directRequestKindBox on every // request's ctx. See mcpAuthMiddleware (server.go), the one production // caller, for why this must wrap the handler chain BEFORE mcp-go's @@ -342,8 +362,12 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools } authCtx := auth.AuthContextFromContext(ctx) - _, profileScope := p.resolveActiveProfile(ctx) + profileName, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) isScopedAgent := isScopeRestrictedCaller(authCtx) + var policy *profile.CompiledPolicy + if profileName != "" { + policy = profileIdx.PolicyFor(profileName) + } // Spec 105 FR-008 (FR008-G7): a tool with no registration identity is // withheld from EVERY caller, administrators included, so this filter can @@ -379,6 +403,9 @@ func (p *MCPProxyServer) filterDirectModeToolsForAuth(ctx context.Context, tools if !directIdentityInScope(authCtx, profileScope, isScopedAgent, stamp.owner, stamp.tier) { continue } + if p.directPolicyExcludedForListing(ctx, policy, stamp.owner, stamp.rawName) { + continue + } filtered = append(filtered, tool) continue } diff --git a/internal/server/mcp_visibility.go b/internal/server/mcp_visibility.go index b349a54da..980d13544 100644 --- a/internal/server/mcp_visibility.go +++ b/internal/server/mcp_visibility.go @@ -39,6 +39,14 @@ const ( // record). Dispatch refuses such a name for every caller, so describe_tool // withholds its definition with the plain not-found shape (astra r2 C2). visReasonToolUnresolved = "tool_unresolved" + // visReasonToolPolicyExcluded is the Spec 108 FR-011 defense-in-depth + // reason: indexedToolVisible's own CompiledPolicy.Decide re-check found + // the tool excluded, even though SearchToolsAdmitted already filtered it + // out before the cut. Treated exactly like visReasonServerNotInScope by + // the retrieve_tools loop (invisible, never "locked", never counted) — + // FR-013 forbids naming or describing a profile-hidden tool, and a + // "locked" entry would do both. + visReasonToolPolicyExcluded = "tool_policy_excluded" ) // The two resolvers share one set of step helpers (serverInScope, @@ -77,7 +85,7 @@ const ( // and an approved one was withheld on the pending sibling's. func (p *MCPProxyServer) toolVisibleToSession(ctx context.Context, serverName, toolName string) (visible bool, reason string) { authCtx := auth.AuthContextFromContext(ctx) - _, profileScope := p.resolveActiveProfile(ctx) + profileName, profileScope, profileIdx := p.resolveActiveProfileWithIndex(ctx) // Spec 105 FR-010 G2: for a SCOPED caller (agent token), scope is // checked BEFORE index presence. An id whose server is outside the @@ -141,6 +149,23 @@ func (p *MCPProxyServer) toolVisibleToSession(ctx context.Context, serverName, t if !p.isExactToolCallable(serverName, toolName) { return false, visReasonToolNotCallable } + // Spec 108 FR-011/T023: describe_tool's contract makes no shape change + // for an excluded tool — the caller must see the SAME uniform not-found + // response a nonexistent id gets (contracts/mcp-tools.md "describe_tool"). + // Ordered LAST, after every other gate, so a quarantined/pending/disabled + // tool still reports its own, more specific reason first; a policy + // exclusion only ever narrows what an otherwise-visible tool reveals. + // Safe to run even when profileName is legacy or "" (policy is then nil + // or Decide only ever agrees with the scope check already passed above). + if profileName != "" { + if policy := profileIdx.PolicyFor(profileName); policy != nil { + annotations, found := p.EffectiveAnnotations(serverName, toolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(serverName, toolName, intrinsic); !admitted { + return false, visReasonToolPolicyExcluded + } + } + } return true, "" } @@ -157,7 +182,18 @@ func (p *MCPProxyServer) toolVisibleToSession(ctx context.Context, serverName, t // The pair arrives ALREADY SPLIT (the retrieve loop derives the raw name from // the index hit once, config.RawToolName) and is consulted exactly — see // toolVisibleToSession for why a second normalization is wrong. -func (p *MCPProxyServer) indexedToolVisible(authCtx *auth.AuthContext, profileScope *profile.ProfileScope, serverName, toolName string) (visible bool, reason string) { +// +// policy is the Spec 108 compiled policy for the caller's effective profile +// (nil when none is in effect, or the profile is legacy — Decide would only +// ever agree with serverInScope's own verdict for a legacy profile, so +// callers may pass nil for it unconditionally without changing behaviour). +// It is consulted here purely as POST-CUT defense in depth (T022): +// SearchToolsAdmitted already applies the identical decision before the +// ranked cut, so this branch should not fire in the steady state; it exists +// for the narrow window between a config/annotation change and the next +// search, and for any future caller of this shared step that does not itself +// go through SearchToolsAdmitted. +func (p *MCPProxyServer) indexedToolVisible(authCtx *auth.AuthContext, profileScope *profile.ProfileScope, policy *profile.CompiledPolicy, serverName, toolName string) (visible bool, reason string) { // Profile scope (Spec 057) + agent-token server scope (Spec 028) — // applied BEFORE any classification so an agent never learns a tool // exists on a server it cannot access. @@ -170,6 +206,14 @@ func (p *MCPProxyServer) indexedToolVisible(authCtx *auth.AuthContext, profileSc return false, visReasonToolNotCallable } + if policy != nil { + annotations, found := p.EffectiveAnnotations(serverName, toolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(serverName, toolName, intrinsic); !admitted { + return false, visReasonToolPolicyExcluded + } + } + return true, "" } diff --git a/internal/server/preflight_dispatch_parity_test.go b/internal/server/preflight_dispatch_parity_test.go index 0e94387f2..7baba6482 100644 --- a/internal/server/preflight_dispatch_parity_test.go +++ b/internal/server/preflight_dispatch_parity_test.go @@ -440,7 +440,7 @@ func TestStaleTokenPinDeniesOnBothSessionAndPreflightPaths(t *testing.T) { _, scope := fixture.proxy.resolveActiveProfile(ctx) require.NotNil(t, scope) - vis, _ := fixture.proxy.indexedToolVisible(authCtx, scope, id.server, id.tool) + vis, _ := fixture.proxy.indexedToolVisible(authCtx, scope, nil, id.server, id.tool) assert.False(t, vis, "search path: %s:%s must be denied under a stale pin", id.server, id.tool) } diff --git a/internal/server/profile_resolver.go b/internal/server/profile_resolver.go index cbc1b6d67..cb00adaa5 100644 --- a/internal/server/profile_resolver.go +++ b/internal/server/profile_resolver.go @@ -185,12 +185,26 @@ func (p *MCPProxyServer) resolveActiveProfile(ctx context.Context) (string, *pro // the class of bug rounds 9/11/14/15 closed on the admission and resolution // paths. func (p *MCPProxyServer) resolveActiveProfileWithIndex(ctx context.Context) (string, *profile.ProfileScope, *profileIndex) { + name, scope, idx, _ := p.resolveActiveProfileWithSource(ctx) + return name, scope, idx +} + +// resolveActiveProfileWithSource is resolveActiveProfileWithIndex, additionally +// reporting which resolution tier produced the (name, scope) pair (Spec 108 +// FR-011): retrieve_tools needs this to decide whether its `profile` field may +// name the effective slug at all — only when the caller itself selected it +// (source url or session) — never when it merely inherited a token pin, +// which would tell a caller it is pinned and to what (research D27). Only +// the three tiers 108-b resolves (pin, url, session) are distinguished here; +// the binding and anonymous tiers land in 108-c/108-d, which extend this +// function's cases rather than duplicate them. +func (p *MCPProxyServer) resolveActiveProfileWithSource(ctx context.Context) (string, *profile.ProfileScope, *profileIndex, profile.Source) { idx, ok := profileRequestIndexFromContext(ctx) if !ok { idx = p.profileIndexFor(p.currentConfig()) } - name, scope := p.resolveActiveProfileFromIndex(ctx, idx) - return name, scope, idx + name, scope, source := p.resolveActiveProfileWithSourceFromIndex(ctx, idx) + return name, scope, idx, source } // resolveActiveProfileIn is resolveActiveProfile against an explicit config @@ -237,6 +251,16 @@ func (p *MCPProxyServer) resolveActiveProfileIn(ctx context.Context, cfg *config // current (shorter) Profiles slice and index out of range; resolving both // from the ONE pair the caller already has cannot. func (p *MCPProxyServer) resolveActiveProfileFromIndex(ctx context.Context, idx *profileIndex) (string, *profile.ProfileScope) { + name, scope, _ := p.resolveActiveProfileWithSourceFromIndex(ctx, idx) + return name, scope +} + +// resolveActiveProfileWithSourceFromIndex is resolveActiveProfileFromIndex, +// additionally reporting the profile.Source tier the (name, scope) pair +// resolved through (Spec 108 FR-011). It is the ONE place this precedence is +// implemented; resolveActiveProfileFromIndex is a thin wrapper over it so the +// two can never drift. +func (p *MCPProxyServer) resolveActiveProfileWithSourceFromIndex(ctx context.Context, idx *profileIndex) (string, *profile.ProfileScope, profile.Source) { // 1. Agent-token pin (T3). When present it is authoritative and bounds // everything below — including the case where the pinned profile has been // removed from config since the token was minted. @@ -252,19 +276,19 @@ func (p *MCPProxyServer) resolveActiveProfileFromIndex(ctx context.Context, idx // preflight paths cannot disagree about what a pinned token may see. if pin := profilePinFromContext(ctx); pin != "" { if scope := profileScopeFromIndex(idx, pin); scope != nil { - return pin, scope + return pin, scope, profile.SourcePin } if p.logger != nil { p.logger.Warn("agent-token profile_pin no longer matches any configured profile; resolving to a deny-all scope", zap.String("profile_pin", pin)) } - return pin, profile.NewProfileScope(pin, nil) + return pin, profile.NewProfileScope(pin, nil), profile.SourcePin } // 2. Explicit URL profile (Spec 057). Authoritative for this request, so it // overrides any stored session selection on the same connection. if urlScope := profile.ProfileScopeFromContext(ctx); urlScope != nil { - return urlScope.Name, urlScope + return urlScope.Name, urlScope, profile.SourceURL } // 3. Session selection set via the set_profile tool on the base /mcp endpoint. @@ -272,7 +296,7 @@ func (p *MCPProxyServer) resolveActiveProfileFromIndex(ctx context.Context, idx if sid := sessionIDFromContext(ctx); sid != "" { if name := p.sessionStore.GetActiveProfile(sid); name != "" { if scope := profileScopeFromIndex(idx, name); scope != nil { - return name, scope + return name, scope, profile.SourceSession } // Stored profile vanished from config — drop the stale selection. p.sessionStore.SetActiveProfile(sid, "") @@ -281,7 +305,7 @@ func (p *MCPProxyServer) resolveActiveProfileFromIndex(ctx context.Context, idx } // 4. No profile in effect. - return "", nil + return "", nil, profile.SourceNone } // resolveEffectiveProfileForJustSetSlug is resolveActiveProfileFromIndex's diff --git a/internal/server/profiles_v3_index_fixture_test.go b/internal/server/profiles_v3_index_fixture_test.go new file mode 100644 index 000000000..f90a309b7 --- /dev/null +++ b/internal/server/profiles_v3_index_fixture_test.go @@ -0,0 +1,79 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Spec 108 (Profiles v3) 108-b: newProfilesV3Fixture (108-a, +// profiles_v3_fixture_test.go) wires the enforcement-matrix fixture's three +// upstreams into StateView + storage (for annotation/identity resolution) +// but never into the search index — 108-a exercises CompiledPolicy.Decide +// directly and never calls retrieve_tools/describe_tool. This file adds the +// index side those handlers actually search over. +// +// Descriptions here are the exact RAW tool name, mirroring T001's node-side +// fixture convention ("descriptions = tool names") — but note the QUERY +// strings this file's tests use are the exact tool names too (never the +// contracts/enforcement-matrix.md natural-language probes "issue"/"secret"/ +// "search"/"repo"): tool_name is keyword-analyzed (Spec 105 D-something; +// bleve.go), so an exact-name query is the only query shape guaranteed to +// hit deterministically regardless of how the standard analyzer segments a +// snake_case description. The DECISIONS under test (admitted/excluded per +// tool, hidden_by_profile counts, profile field presence) are unaffected by +// this substitution — only the probe string choice is. +func indexEnforcementMatrixFixtureTools(t *testing.T, proxy *MCPProxyServer) { + t.Helper() + tools := []*config.ToolMetadata{ + { + Name: "github:list_issues", ServerName: "github", Description: "list_issues", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + }, + { + Name: "github:create_issue", ServerName: "github", Description: "create_issue", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}, + }, + { + Name: "github:delete_repo", ServerName: "github", Description: "delete_repo", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{DestructiveHint: boolPtr(true)}, + }, + { + Name: "github:search_code", ServerName: "github", Description: "search_code", ParamsJSON: "{}", + }, + { + Name: "github:get_secret_scanning_alert", ServerName: "github", Description: "get_secret_scanning_alert", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + }, + { + Name: "notion:update_page", ServerName: "notion", Description: "update_page", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}, + }, + { + Name: "filesystem:read_text_file", ServerName: "filesystem", Description: "read_text_file", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + }, + } + for _, tool := range tools { + require.NoError(t, proxy.index.IndexTool(tool)) + } + + // retrieve_tools searches a per-profile PHYSICAL index (index/manager.go + // ForProfile) once a profile is in effect — a lazily created, initially + // EMPTY store that production keeps in sync via + // runtime.rebuildProfileIndex → Manager.RebuildProfileFromShared on every + // config publish. This test fixture's proxy.index is independent of the + // runtime's own indexManager (createTestProxyWithRuntimeCfg wires two + // separate Managers, as the real core does not), so nothing rebuilds it + // automatically here — do it once, explicitly, for exactly the three + // enforcementMatrixProfiles fixture profiles. + for name, servers := range map[string][]string{ + "work-readonly": {"github", "notion"}, + "work-full": {"github", "notion", "filesystem"}, + "legacy": {"github"}, + } { + require.NoError(t, proxy.index.RebuildProfileFromShared(name, servers)) + } +} diff --git a/internal/server/prompts_profile_v3_test.go b/internal/server/prompts_profile_v3_test.go new file mode 100644 index 000000000..a19d4a313 --- /dev/null +++ b/internal/server/prompts_profile_v3_test.go @@ -0,0 +1,55 @@ +package server + +import ( + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" +) + +// Spec 108 (Profiles v3) T018a (FR-019): the tool policy (max_tier cap, deny +// rules, classify) must never filter prompts — prompts/list and prompts/get +// under a v3 profile equal those of a legacy profile with the SAME servers, +// even when the v3 profile carries a deny rule that would exclude a +// similarly-named TOOL. filterAggregatedPromptsForAuth (mcp_direct_scope.go) +// consults only profileScope.Allows(serverName) — never a CompiledPolicy — +// so this is a permanent regression guard: 108-b must not, now or later, +// make prompt filtering consult the tool policy. +func TestPrompts_ProfileV3_ToolPolicyNeverFiltersPrompts(t *testing.T) { + falseVal := false + cfg := &config.Config{ + Servers: []*config.ServerConfig{{Name: "github"}, {Name: "notion"}}, + Profiles: []config.ProfileConfig{ + { + Name: "work-readonly-v3", Servers: []string{"github", "notion"}, MaxTier: "read", + Tools: &config.ProfileToolRules{Deny: []string{"github:*secret*"}}, + CodeExecution: &falseVal, + }, + // A legacy profile over the EXACT SAME servers — the FR-019 + // oracle: nothing in the v3 profile above may make prompt + // visibility differ from this one. + {Name: "legacy-samesrv", Servers: []string{"github", "notion"}}, + }, + } + proxy := &MCPProxyServer{config: cfg} + + // A prompt whose name would match the v3 profile's deny GLOB if (wrongly) + // evaluated as a tool identity. + secretLikePrompt := aggregatedPromptForTest("github", "get_secret_info") + otherPrompt := aggregatedPromptForTest("notion", "summarize_page") + prompts := []mcp.Prompt{secretLikePrompt, otherPrompt} + + v3Ctx := profile.WithProfileScope(t.Context(), proxy.profileScopeForSlug("work-readonly-v3")) + legacyCtx := profile.WithProfileScope(t.Context(), proxy.profileScopeForSlug("legacy-samesrv")) + + v3Result := promptNamesForTest(proxy.filterAggregatedPromptsForAuth(v3Ctx, prompts)) + legacyResult := promptNamesForTest(proxy.filterAggregatedPromptsForAuth(legacyCtx, prompts)) + + assert.ElementsMatch(t, legacyResult, v3Result, + "a tool-policy deny rule must never filter a prompt, even one whose name would match its glob") + require.Len(t, v3Result, 2, "both prompts must be visible: neither server is out of scope") +} diff --git a/internal/server/retrieve_tools_profile_v3_golden_test.go b/internal/server/retrieve_tools_profile_v3_golden_test.go new file mode 100644 index 000000000..6fe3bd931 --- /dev/null +++ b/internal/server/retrieve_tools_profile_v3_golden_test.go @@ -0,0 +1,29 @@ +package server + +import ( + "path/filepath" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" +) + +// Spec 108 (Profiles v3) T021: the one new golden this PR adds +// (contracts/mcp-tools.md "Frozen goldens"). Legacy goldens +// (retrieve_full_default.golden.json etc.) are untouched (SC-003) — this +// file adds a NEW golden for the NEW non-legacy response shape only. +// Regenerate with: UPDATE_GOLDEN=1 go test ./internal/server -run +// TestRetrieveToolsProfileV3_GoldenByteIdentity +func TestRetrieveToolsProfileV3_GoldenByteIdentity(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"query": "list_issues", "limit": float64(10)} + result, err := proxy.handleRetrieveTools(urlProfileCtx(proxy, "work-readonly"), req) + require.NoError(t, err) + require.False(t, result.IsError, "retrieve_tools returned an error result") + got := resultText(t, result) + + compareGolden(t, filepath.Join("testdata", "retrieve_tools_profile_v3.golden.json"), got) +} diff --git a/internal/server/retrieve_tools_profile_v3_test.go b/internal/server/retrieve_tools_profile_v3_test.go new file mode 100644 index 000000000..590cc8f93 --- /dev/null +++ b/internal/server/retrieve_tools_profile_v3_test.go @@ -0,0 +1,230 @@ +package server + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" +) + +// Spec 108 (Profiles v3) PR 108-b, T016: retrieve_tools filter-before-limit + +// hidden_by_profile + profile (url/session sources only), over the +// contracts/enforcement-matrix.md fixture. Sources exercised here are the +// three 108-b resolves without 108-c: URL (/mcp/p/), session +// (set_profile) and a legacy agent-token pin (Spec 057/105) — binding and +// anonymous rows are added by 108-d's T044a once 108-c lands the resolver +// tiers that produce them. + +// profileV3RetrieveResponse decodes exactly the fields this file's tests +// need. HiddenByProfile and Profile are pointers so ABSENCE (nil) is +// distinguishable from a present zero value / empty string (FR-011). +type profileV3RetrieveResponse struct { + Tools []map[string]interface{} `json:"tools"` + Total int `json:"total"` + HiddenByProfile *int `json:"hidden_by_profile"` + Profile *string `json:"profile"` +} + +func callRetrieveToolsV3(t *testing.T, proxy *MCPProxyServer, ctx context.Context, query string, limit int) profileV3RetrieveResponse { + t.Helper() + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"query": query, "limit": float64(limit)} + result, err := proxy.handleRetrieveTools(ctx, req) + require.NoError(t, err) + require.False(t, result.IsError, "retrieve_tools returned an error: %v", result.Content) + var resp profileV3RetrieveResponse + require.NoError(t, json.Unmarshal([]byte(resultText(t, result)), &resp)) + return resp +} + +// urlProfileCtx simulates a /mcp/p/ request: an administrator-shaped +// context (profileMiddleware's real behaviour for an unauthenticated +// connection) carrying the URL-injected ProfileScope. +func urlProfileCtx(proxy *MCPProxyServer, slug string) context.Context { + scope := proxy.profileScopeForSlug(slug) + return profile.WithProfileScope(auth.WithAuthContext(context.Background(), auth.AdminContext()), scope) +} + +// sessionProfileCtx simulates a set_profile session selection on the base +// /mcp endpoint: an administrator-shaped context bound to a stable mcp-go +// session id whose sessionStore entry names slug. +func sessionProfileCtx(t *testing.T, proxy *MCPProxyServer, sessionID, slug string) context.Context { + t.Helper() + proxy.sessionStore.SetActiveProfile(sessionID, slug) + helper := mcpserver.NewMCPServer("test", "1.0.0") + base := helper.WithContext(context.Background(), &fakeClientSession{id: sessionID}) + return auth.WithAuthContext(base, auth.AdminContext()) +} + +// pinnedProfileCtx simulates a legacy agent token pinned to slug (Spec 057/ +// 105): unrestricted server grant so only the pin's own reach governs. +func pinnedProfileCtx(slug string) context.Context { + return agentCtx([]string{"*"}, []string{auth.PermRead, auth.PermWrite, auth.PermDestructive}, slug) +} + +func TestRetrieveTools_ProfileV3_DecisionMatrix(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + // contracts/enforcement-matrix.md "Expected profile decisions" (FR-010), + // keyed by exact tool name (the deterministic query this file's tests + // use — see profiles_v3_index_fixture_test.go's doc comment). + cases := []struct { + tool string + admittedReadonly, admittedFull bool + admittedLegacy bool + }{ + {"list_issues", true, true, true}, + {"create_issue", false, true, true}, + {"delete_repo", false, true, true}, + {"search_code", false, true, true}, // legacy: server "github" is in legacy's servers, admitted (as_read default, no cap) + {"get_secret_scanning_alert", false, true, true}, + {"update_page", true, true, false}, // legacy profile's servers = ["github"] only + {"read_text_file", false, true, false}, + } + + for _, tc := range cases { + t.Run(tc.tool, func(t *testing.T) { + readonly := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-readonly"), tc.tool, 5) + assert.Equal(t, tc.admittedReadonly, len(readonly.Tools) > 0, "work-readonly: %s", tc.tool) + + full := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-full"), tc.tool, 5) + assert.Equal(t, tc.admittedFull, len(full.Tools) > 0, "work-full: %s", tc.tool) + + legacy := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "legacy"), tc.tool, 5) + assert.Equal(t, tc.admittedLegacy, len(legacy.Tools) > 0, "legacy: %s", tc.tool) + }) + } + + t.Run("hidden_by_profile present (possibly 0) for non-legacy, absent for legacy", func(t *testing.T) { + readonly := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-readonly"), "create_issue", 5) + require.NotNil(t, readonly.HiddenByProfile) + assert.Equal(t, 1, *readonly.HiddenByProfile, "create_issue matches and is excluded by the tier cap") + + full := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-full"), "create_issue", 5) + require.NotNil(t, full.HiddenByProfile) + assert.Equal(t, 0, *full.HiddenByProfile, "work-full admits create_issue: nothing hidden") + + legacy := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "legacy"), "create_issue", 5) + assert.Nil(t, legacy.HiddenByProfile, "legacy profile must never gain hidden_by_profile (SC-003)") + }) + + t.Run("deny-beats-allow overlap: get_secret_scanning_alert matched by both an allow and a deny rule", func(t *testing.T) { + resp := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-readonly"), "get_secret_scanning_alert", 5) + assert.Empty(t, resp.Tools, "deny must beat allow on the overlapping pair (FR-010 step 2 before step 3)") + require.NotNil(t, resp.HiddenByProfile) + assert.Equal(t, 1, *resp.HiddenByProfile) + }) + + t.Run("allow rule admits despite the tier cap: notion:update_page", func(t *testing.T) { + resp := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-readonly"), "update_page", 5) + require.Len(t, resp.Tools, 1) + assert.Equal(t, "notion:update_page", resp.Tools[0]["name"]) + }) +} + +// TestRetrieveTools_ProfileV3_ProfileFieldSourceGating pins FR-011's `profile` +// field rule: present only for url/session sources, absent for a pin — for +// the SAME non-legacy profile and tool. +func TestRetrieveTools_ProfileV3_ProfileFieldSourceGating(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + t.Run("url source: profile field present", func(t *testing.T) { + resp := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "work-readonly"), "list_issues", 5) + require.NotNil(t, resp.Profile) + assert.Equal(t, "work-readonly", *resp.Profile) + }) + + t.Run("session source: profile field present", func(t *testing.T) { + ctx := sessionProfileCtx(t, proxy, "sess-v3-1", "work-readonly") + resp := callRetrieveToolsV3(t, proxy, ctx, "list_issues", 5) + require.NotNil(t, resp.Profile) + assert.Equal(t, "work-readonly", *resp.Profile) + }) + + t.Run("pin source: profile field absent", func(t *testing.T) { + resp := callRetrieveToolsV3(t, proxy, pinnedProfileCtx("work-readonly"), "list_issues", 5) + assert.Nil(t, resp.Profile, "a pinned caller must never learn it is pinned or to what (research D27)") + require.NotNil(t, resp.HiddenByProfile, "hidden_by_profile is independent of source and still applies") + }) +} + +// TestRetrieveTools_ProfileV3_DanglingPinDenyAll pins FR-020: a legacy agent +// token pinned to a profile hand-deleted from config resolves deny-all, with +// hidden_by_profile: 0 and NO profile field (base source, never disclosed). +func TestRetrieveTools_ProfileV3_DanglingPinDenyAll(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + resp := callRetrieveToolsV3(t, proxy, pinnedProfileCtx("ghost-profile"), "list_issues", 5) + assert.Empty(t, resp.Tools, "a dangling pin must deny all") + require.NotNil(t, resp.HiddenByProfile, "a dangling base always counts as non-legacy") + assert.Equal(t, 0, *resp.HiddenByProfile, "the effective server scope is empty, so nothing is counted as hidden") + assert.Nil(t, resp.Profile) +} + +// TestRetrieveTools_ProfileV3_FilterBeforeLimit is the FR-011 "filter before +// limit" assertion: a policy-excluded tool that ranks first in the raw +// corpus must never displace an admitted one from a small window, and +// hidden_by_profile must still count it. Uses a dedicated small fixture +// (rather than the enforcement-matrix one) so the rank order can be forced +// deterministically via BM25 term-frequency repetition — the same technique +// internal/index/search_scoped_test.go's buildScopedSeamCorpus and +// search_admitted_test.go's buildAdmittedSeamCorpus use at the index layer; +// this is the same property proven end-to-end through retrieve_tools. +func TestRetrieveTools_ProfileV3_FilterBeforeLimit(t *testing.T) { + proxy, rt := createTestProxyWithRuntimeCfg(t, nil, func(cfg *config.Config) { + config.EnablePolicyForTest(t) + cfg.Servers = []*config.ServerConfig{{Name: "a", Enabled: true}} + cfg.Profiles = []config.ProfileConfig{ + {Name: "cap-read", Servers: []string{"a"}, MaxTier: "read"}, + } + }) + + startCountingUpstream(t, proxy, rt, "a", + toolSpec{Name: "keep", Description: "widget status check", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + toolSpec{Name: "many", Description: strings.Repeat("widget ", 10), Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}}, + ) + require.NoError(t, proxy.index.IndexTool(&config.ToolMetadata{ + Name: "a:keep", ServerName: "a", Description: "widget status check", ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + })) + require.NoError(t, proxy.index.IndexTool(&config.ToolMetadata{ + Name: "a:many", ServerName: "a", Description: strings.Repeat("widget ", 10), ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}, + })) + require.NoError(t, proxy.index.RebuildProfileFromShared("cap-read", []string{"a"})) + + // Control: the unscoped/unprofiled window really is the "many" hit. + unprofiled := callRetrieveToolsV3(t, proxy, adminCtx(), "widget", 1) + require.Len(t, unprofiled.Tools, 1) + assert.Equal(t, "a:many", unprofiled.Tools[0]["name"], "fixture: the write tool must outrank the read one") + + resp := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "cap-read"), "widget", 1) + require.Len(t, resp.Tools, 1, "the admitted hit must fill the page despite the higher-ranked excluded one") + assert.Equal(t, "a:keep", resp.Tools[0]["name"]) + require.NotNil(t, resp.HiddenByProfile) + assert.Equal(t, 1, *resp.HiddenByProfile, "a:many must still be counted even though the page was already full") +} + +// TestRetrieveTools_ProfileV3_LegacyTitleStaysLegacy pins the data-model.md +// §1 rule that Title/Description alone never flip IsLegacy() — a legacy +// profile that sets ONLY a display title must stay byte-identical (SC-003). +func TestRetrieveTools_ProfileV3_LegacyTitleStaysLegacy(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + resp := callRetrieveToolsV3(t, proxy, urlProfileCtx(proxy, "legacy"), "list_issues", 5) + assert.Nil(t, resp.HiddenByProfile) + assert.Nil(t, resp.Profile) +} diff --git a/internal/server/scope_latency_profile_v3_test.go b/internal/server/scope_latency_profile_v3_test.go new file mode 100644 index 000000000..047953209 --- /dev/null +++ b/internal/server/scope_latency_profile_v3_test.go @@ -0,0 +1,102 @@ +package server + +import ( + "context" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" +) + +// Spec 108 (Profiles v3) T025 (SC-007): the v3 policy-aware SearchToolsAdmitted +// path resolves one EffectiveAnnotations lookup and one CompiledPolicy.Decide +// call per scanned hit, on top of the plain server-scope check +// SearchToolsScoped already paid for (Spec 105 T078). This is a prototype +// measurement, not a CI gate (mirrors TestRetrieveTools_ScopeLatency_ +// ScopedVsAdmin's own framing): it runs once, locally, over a 1,000+-tool +// corpus (the Spec 102/083 LiveMCPBench snapshot, doubled under a second +// server-name suffix to clear the SC-007 scale) and asserts the v3-vs-legacy +// p95 gap stays within FR-011's 20ms budget on this machine. +// +// Skipped under -race and testing.Short() for the same reasons as its Spec +// 105 sibling in scope_latency_test.go. +func TestRetrieveTools_ScopeLatency_ProfileV3VsLegacy(t *testing.T) { + if testing.Short() { + t.Skip("integration — 400+ retrieve_tools calls over a 1,000+-tool corpus") + } + if raceEnabled { + t.Skip("timing is meaningless under the race detector's instrumentation overhead") + } + + base := loadDeferredLargeCorpus(t) + // Double the corpus under a second server-name suffix to clear the + // SC-007 1,000-tool scale without hand-authoring a synthetic fixture. + tools := make([]*config.ToolMetadata, 0, len(base)*2) + tools = append(tools, base...) + for _, tool := range base { + dup := *tool + dup.ServerName = tool.ServerName + "_dup" + dup.Name = dup.ServerName + ":" + config.RawToolName(tool) + dup.RawName = config.RawToolName(tool) + tools = append(tools, &dup) + } + require.GreaterOrEqual(t, len(tools), 1000, "fixture must reach the SC-007 1,000-tool scale") + + seenServers := make(map[string]bool) + var servers []string + for _, tool := range tools { + if !seenServers[tool.ServerName] { + seenServers[tool.ServerName] = true + servers = append(servers, tool.ServerName) + } + } + require.Greater(t, len(servers), 20, "fixture: doubling the snapshot must still name more than a handful of servers") + + proxy := createTestMCPProxyServer(t) + config.EnablePolicyForTest(t) + serverCfgs := make([]*config.ServerConfig, 0, len(servers)) + for _, s := range servers { + serverCfgs = append(serverCfgs, &config.ServerConfig{Name: s, Enabled: true}) + } + proxy.config.Servers = serverCfgs + proxy.config.Profiles = []config.ProfileConfig{ + {Name: "v3-cap-read", Servers: servers, MaxTier: "read", Unannotated: "as_read"}, + {Name: "legacy-samescope", Servers: servers}, + } + require.NoError(t, proxy.index.BatchIndexTools(tools)) + + const query = "get data" + const limit = 10 + const warmup = 20 + const timed = 200 + + measure := func(ctx context.Context) []time.Duration { + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"query": query, "limit": float64(limit)} + return measureLatency(t, ctx, warmup, timed, func(ctx context.Context) error { + _, err := proxy.handleRetrieveTools(ctx, req) + return err + }) + } + + legacyCtx := profile.WithProfileScope(context.Background(), proxy.profileScopeForSlug("legacy-samescope")) + v3Ctx := profile.WithProfileScope(context.Background(), proxy.profileScopeForSlug("v3-cap-read")) + + legacyDurations := measure(legacyCtx) + v3Durations := measure(v3Ctx) + + pLegacy, pV3 := p95(legacyDurations), p95(v3Durations) + gap := pV3 - pLegacy + t.Logf("retrieve_tools p95 over %d timed calls (%d-tool corpus, %d servers): legacy=%s v3=%s gap=%s", + timed, len(tools), len(servers), pLegacy, pV3, gap) + recordAdminLatencyResult(t, "retrieve_tools_profile_v3", pV3) + + const budget = 20 * time.Millisecond + require.LessOrEqualf(t, gap, budget, + "v3 policy-aware retrieve_tools p95 must not exceed legacy's by more than FR-011's %s budget (got legacy=%s v3=%s gap=%s)", + budget, pLegacy, pV3, gap) +} diff --git a/internal/server/scope_oracle_v3_test.go b/internal/server/scope_oracle_v3_test.go new file mode 100644 index 000000000..9f1ed5bcd --- /dev/null +++ b/internal/server/scope_oracle_v3_test.go @@ -0,0 +1,130 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Spec 108 (Profiles v3) T017: extends the Spec 105 two-fixture differential +// oracle (scope_differential_test.go's newScopeFixture/contracts/ +// differential-oracle.md) with a v3 tool policy applied on top — proving +// that hidden_by_profile (and every other retrieve_tools field) is IDENTICAL +// between a fixture that contains only the caller's own server ("a") and one +// that also carries hidden servers ("b", "a__b") the caller cannot see at +// all, for the SAME caller and profile. A hidden server's mere existence +// (let alone its own excluded tools) must never perturb hidden_by_profile — +// that count is defined over the caller's effective server scope only +// (contracts/mcp-tools.md). +// +// A dedicated builder is used rather than newScopeFixture itself: that +// helper's proxy has no config hook to add Profiles after construction, and +// this oracle's caller must be profile-scoped (a v3 policy resolved through +// a real profileIndex), not merely token-scoped. + +type scopeOracleV3Fixture struct { + proxy *MCPProxyServer +} + +// newScopeOracleV3Fixture mirrors newScopeFixture's {a} / {a,b,a__b} split +// letter-for-letter on server "a"'s own tools (readSpec/writeSpec/ +// destructiveSpec over the same names), with one v3 profile ("cap-read-a", +// max_tier read, servers=["a"]) — so the excluded tools (write_thing, +// destroy_thing, ns:erase) are exactly the ones a plain token-scope oracle +// would never distinguish from an admitted one. +func newScopeOracleV3Fixture(t *testing.T, full bool) *scopeOracleV3Fixture { + t.Helper() + proxy, rt := createTestProxyWithRuntimeCfg(t, nil, func(cfg *config.Config) { + config.EnablePolicyForTest(t) + cfg.Servers = []*config.ServerConfig{{Name: "a", Enabled: true}} + if full { + cfg.Servers = append(cfg.Servers, + &config.ServerConfig{Name: "b", Enabled: true}, + &config.ServerConfig{Name: "a__b", Enabled: true}, + ) + } + cfg.Profiles = []config.ProfileConfig{ + {Name: "cap-read-a", Servers: []string{"a"}, MaxTier: "read"}, + } + }) + + startCountingUpstream(t, proxy, rt, "a", + readSpec("read_thing"), writeSpec("write_thing"), destructiveSpec("destroy_thing"), + readSpec("erase"), writeSpec("ns:erase")) + for _, tool := range []*config.ToolMetadata{ + {Name: "a:read_thing", ServerName: "a", Description: "read_thing", ParamsJSON: "{}", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + {Name: "a:write_thing", ServerName: "a", Description: "write_thing", ParamsJSON: "{}", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}}, + {Name: "a:destroy_thing", ServerName: "a", Description: "destroy_thing", ParamsJSON: "{}", Annotations: &config.ToolAnnotations{DestructiveHint: boolPtr(true)}}, + {Name: "a:erase", ServerName: "a", Description: "erase", ParamsJSON: "{}", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + {Name: "a:ns:erase", ServerName: "a", Description: "ns_erase", ParamsJSON: "{}", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(false)}}, + } { + require.NoError(t, proxy.index.IndexTool(tool)) + } + + if full { + sentB := "SENTINEL_scopeOracleV3B_71a2" + sentAB := "SENTINEL_scopeOracleV3AB_39fe" + startCountingUpstream(t, proxy, rt, "b", + toolSpec{Name: sentB + "_tool", Description: "Handles " + sentB, + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}) + startCountingUpstream(t, proxy, rt, "a__b", + toolSpec{Name: sentAB + "_tool", Description: "Handles " + sentAB, + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}) + require.NoError(t, proxy.index.IndexTool(&config.ToolMetadata{ + Name: "b:" + sentB + "_tool", ServerName: "b", Description: "Handles " + sentB, ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + })) + require.NoError(t, proxy.index.IndexTool(&config.ToolMetadata{ + Name: "a__b:" + sentAB + "_tool", ServerName: "a__b", Description: "Handles " + sentAB, ParamsJSON: "{}", + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}, + })) + } + + require.NoError(t, proxy.index.RebuildProfileFromShared("cap-read-a", []string{"a"})) + + return &scopeOracleV3Fixture{proxy: proxy} +} + +// TestScopeOracleV3_HiddenByProfileIdenticalAcrossFixtures is T017: the SAME +// pinned caller run against the narrow and full fixtures must get byte- +// identical retrieve_tools responses, hidden_by_profile included, for every +// query the fixture's own ("a"-server) tool set matches. +func TestScopeOracleV3_HiddenByProfileIdenticalAcrossFixtures(t *testing.T) { + narrow := newScopeOracleV3Fixture(t, false) + full := newScopeOracleV3Fixture(t, true) + + pinned := pinnedProfileCtx("cap-read-a") + + for _, query := range []string{"read_thing", "write_thing", "destroy_thing", "erase", "ns_erase"} { + t.Run(query, func(t *testing.T) { + narrowResp := callRetrieveToolsV3(t, narrow.proxy, pinned, query, 10) + fullResp := callRetrieveToolsV3(t, full.proxy, pinned, query, 10) + + require.NotNil(t, narrowResp.HiddenByProfile) + require.NotNil(t, fullResp.HiddenByProfile) + assert.Equal(t, *narrowResp.HiddenByProfile, *fullResp.HiddenByProfile, + "a hidden server's existence must never perturb hidden_by_profile") + assert.Equal(t, narrowResp.Total, fullResp.Total) + + narrowNames := map[string]bool{} + for _, tl := range narrowResp.Tools { + narrowNames[tl["name"].(string)] = true + } + fullNames := map[string]bool{} + for _, tl := range fullResp.Tools { + name := tl["name"].(string) + assert.NotContains(t, name, "SENTINEL", "a hidden server's sentinel tool must never appear") + fullNames[name] = true + } + assert.Equal(t, narrowNames, fullNames) + }) + } + + t.Run("admin control: the full fixture really does contain the hidden sentinel tools", func(t *testing.T) { + resp := callRetrieveToolsV3(t, full.proxy, adminCtx(), "SENTINEL_scopeOracleV3B_71a2_tool", 10) + require.NotEmpty(t, resp.Tools, "fixture premise: an unscoped administrator must see the hidden sentinel tool") + }) +} diff --git a/internal/server/testdata/retrieve_tools_profile_v3.golden.json b/internal/server/testdata/retrieve_tools_profile_v3.golden.json new file mode 100644 index 000000000..fe3178389 --- /dev/null +++ b/internal/server/testdata/retrieve_tools_profile_v3.golden.json @@ -0,0 +1 @@ +{"hidden_by_profile":0,"profile":"work-readonly","query":"list_issues","session_risk":{"has_destructive_tools":true,"has_open_world_tools":true,"has_write_tools":true,"lethal_trifecta":true,"level":"high"},"tools":[{"annotations":{"readOnlyHint":true},"call_with":"call_tool_read","description":"list_issues","inputSchema":{},"name":"github:list_issues","score":2.575623641485255,"server":"github"}],"total":1,"usage_instructions":"TOOL SELECTION GUIDE: Check the 'call_with' field for each tool, then use the matching tool variant. DECISION RULES BY TOOL NAME: (1) READ (call_tool_read): search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. DEFAULT choice when unsure. (2) WRITE (call_tool_write): create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit, assign, configure, enable, register, subscribe, publish, move, copy, rename, merge. (3) DESTRUCTIVE (call_tool_destructive): delete, remove, drop, revoke, disable, destroy, purge, reset, clear, unsubscribe, cancel, terminate, close, archive, ban, block, disconnect, kill, wipe, truncate, force, hard. INTENT TRACKING: Always provide intent_reason (why you're calling this tool) and intent_data_sensitivity (public/internal/private/unknown) to enable activity auditing."} From cad9967677fc420829a14138f044ee69d1f8cdd7 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 18:54:12 +0300 Subject: [PATCH 3/7] feat(cache): stamp PolicyFingerprint and ToolTierGeneration (FR-027) A read_cache page produced under one tool policy must be refused once that policy changes, even when the profile's server set (already covered pre-108) is unchanged. Authorization gains PolicyFingerprint (the effective profile's compiled policy fingerprint, hex-encoded) and ToolTierGeneration, both folded into the existing digest so the frame header's stamp mechanics are unchanged. Supervisor.ToolTierGeneration is a coarse, whole-fleet counter bumped once per publishDiscoveredTools call, so an out-of-band tool re-annotation invalidates cached pages with no config edit at all. --- internal/cache/authorization.go | 49 +++++-- internal/runtime/supervisor/supervisor.go | 23 +++ internal/server/cache_authz.go | 19 +++ internal/server/cache_authz_policy_test.go | 154 +++++++++++++++++++++ 4 files changed, 231 insertions(+), 14 deletions(-) create mode 100644 internal/server/cache_authz_policy_test.go diff --git a/internal/cache/authorization.go b/internal/cache/authorization.go index 1758d7892..03372f04d 100644 --- a/internal/cache/authorization.go +++ b/internal/cache/authorization.go @@ -127,6 +127,23 @@ type Authorization struct { // or narrowing a profile must revoke cached access, and a stale pin keeps // its name while resolving to deny-all. ProfileServers []string `json:"profile_servers,omitempty"` + // PolicyFingerprint is the effective profile's Spec 108 tool-policy + // fingerprint (profile.CompiledPolicy.Fingerprint, hex-encoded), set only + // when ProfileScoped and the profile has a compiled policy. Editing a + // profile's policy (tier cap, allow/deny, classify, unannotated handling, + // code execution, management tools, switchable_to) changes this without + // necessarily changing ProfileServers, so a page cached under the old + // policy is refused after the edit even though the server-scope dimension + // is unchanged (FR-027). + PolicyFingerprint string `json:"policy_fingerprint,omitempty"` + // ToolTierGeneration is the Supervisor-wide counter (runtime/supervisor. + // Supervisor.ToolTierGeneration) bumped whenever any server's discovered + // tool set is republished — and therefore whenever any tool's effective + // annotations may have changed, or a tool appeared/disappeared — set only + // when ProfileScoped. A page cached while a tool was classified read and + // later re-listed with a destructive hint is refused by the next + // read_cache once this has moved, with no config edit at all (FR-027). + ToolTierGeneration uint64 `json:"tool_tier_generation,omitempty"` } // IsAdministrator reports whether the caller kind is an administrator kind: @@ -214,13 +231,15 @@ func permissionBits(perms []string) uint8 { // the identity the gate requires, and for an agent it makes "digest-equal" // mean the same credential. type canonicalAuthorization struct { - CallerKind string `json:"k"` - Principal string `json:"p,omitempty"` - AllowedServers []string `json:"s,omitempty"` - Permissions []string `json:"t,omitempty"` - ProfilePin string `json:"pin,omitempty"` - ProfileScoped bool `json:"ps,omitempty"` - ProfileServers []string `json:"pss,omitempty"` + CallerKind string `json:"k"` + Principal string `json:"p,omitempty"` + AllowedServers []string `json:"s,omitempty"` + Permissions []string `json:"t,omitempty"` + ProfilePin string `json:"pin,omitempty"` + ProfileScoped bool `json:"ps,omitempty"` + ProfileServers []string `json:"pss,omitempty"` + PolicyFingerprint string `json:"pf,omitempty"` + ToolTierGeneration uint64 `json:"ttg,omitempty"` } func sortedSet(in []string) []string { @@ -238,13 +257,15 @@ func sortedSet(in []string) []string { // is one 32-byte comparison whatever the snapshot names (research D16). func (a Authorization) digest() [sha256.Size]byte { data, err := json.Marshal(canonicalAuthorization{ - CallerKind: a.CallerKind, - Principal: a.Principal, - AllowedServers: sortedSet(a.AllowedServers), - Permissions: sortedSet(a.Permissions), - ProfilePin: a.ProfilePin, - ProfileScoped: a.ProfileScoped, - ProfileServers: sortedSet(a.ProfileServers), + CallerKind: a.CallerKind, + Principal: a.Principal, + AllowedServers: sortedSet(a.AllowedServers), + Permissions: sortedSet(a.Permissions), + ProfilePin: a.ProfilePin, + ProfileScoped: a.ProfileScoped, + ProfileServers: sortedSet(a.ProfileServers), + PolicyFingerprint: a.PolicyFingerprint, + ToolTierGeneration: a.ToolTierGeneration, }) if err != nil { // Strings, string slices and a bool: json.Marshal cannot fail. diff --git a/internal/runtime/supervisor/supervisor.go b/internal/runtime/supervisor/supervisor.go index 7b2db8022..48eba9b52 100644 --- a/internal/runtime/supervisor/supervisor.go +++ b/internal/runtime/supervisor/supervisor.go @@ -121,6 +121,20 @@ type Supervisor struct { version int64 stateMu sync.RWMutex + // toolTierGeneration is bumped once per publishDiscoveredTools call (Spec + // 108 FR-027): any tool's effective annotations may have changed, or a + // tool may have appeared/disappeared, whenever a server's discovered + // tool set is republished. It is a deliberately coarse, whole-fleet + // counter — bumped on every discovery publish, not only one that + // actually changed a tier — rather than a diff against the previous + // tool set: the safe direction for a cache-invalidation signal is to + // over-invalidate (an extra read_cache miss) rather than under-invalidate + // (a stale cached page surviving a real annotation change). Read via + // ToolTierGeneration(); a caller stamps it on a cache entry's producer + // authorization (internal/server/cache_authz.go) alongside the profile's + // own PolicyFingerprint. + toolTierGeneration atomic.Uint64 + // State view for read model (Phase 4) stateView *stateview.View @@ -1354,6 +1368,7 @@ func (s *Supervisor) publishDiscoveredTools(toolsByServer map[string][]*config.T s.snapshot.Store(newSnapshot) s.version++ + s.toolTierGeneration.Add(1) // Update StateView for each accepted server for serverName, serverTools := range accepted { @@ -1619,6 +1634,14 @@ func (s *Supervisor) StateView() *stateview.View { return s.stateView } +// ToolTierGeneration returns the Spec 108 FR-027 counter bumped once per +// publishDiscoveredTools call — a coarse, whole-fleet signal that some +// server's discovered tool set (and therefore some tool's effective +// annotations) may have changed since a caller last read it. Lock-free. +func (s *Supervisor) ToolTierGeneration() uint64 { + return s.toolTierGeneration.Load() +} + // Subscribe returns a channel that receives supervisor events. func (s *Supervisor) Subscribe() <-chan Event { s.eventMu.Lock() diff --git a/internal/server/cache_authz.go b/internal/server/cache_authz.go index a73c083bc..2b5cdf514 100644 --- a/internal/server/cache_authz.go +++ b/internal/server/cache_authz.go @@ -2,6 +2,7 @@ package server import ( "context" + "encoding/hex" "errors" "fmt" "sort" @@ -113,6 +114,24 @@ func (p *MCPProxyServer) cacheAuthorizationWith(ctx context.Context, profileName a.ProfileServers = scope.AllowedServerNames() } sort.Strings(a.ProfileServers) + + // Spec 108 FR-027: the tool-policy dimensions of the stamp. A + // profile with no compiled policy (idx nil, or the name resolved to + // no ProfileConfig — a dangling pin) leaves PolicyFingerprint + // empty, matching a legacy profile's own zero-value fingerprint + // (Compile hashes only the six FR-001 fields, all unset either + // way) — the read gate still bounds correctly on ProfileServers + // alone in that case. + if idx != nil { + if pol := idx.PolicyFor(profileName); pol != nil { + a.PolicyFingerprint = hex.EncodeToString(pol.Fingerprint[:]) + } + } + if p.mainServer != nil && p.mainServer.runtime != nil { + if sup := p.mainServer.runtime.Supervisor(); sup != nil { + a.ToolTierGeneration = sup.ToolTierGeneration() + } + } } return a } diff --git a/internal/server/cache_authz_policy_test.go b/internal/server/cache_authz_policy_test.go new file mode 100644 index 000000000..25c04fb53 --- /dev/null +++ b/internal/server/cache_authz_policy_test.go @@ -0,0 +1,154 @@ +package server + +import ( + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/stateview" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/supervisor" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// seedCachePagingFixture adds three MORE read-tier github tools sharing a +// distinctive description term, on top of newProfilesV3Fixture's own five — +// retrieve_tools' truncator needs a multi-element array to build a real +// read_cache page from; a single-tool response falls back to SimpleTruncate +// (no pagination handle at all, mcp_retrieve_tools_truncation_test.go's own +// fixture makes the same choice for the same reason). They ride the SAME +// already-certified discovery epoch startCountingUpstream stamped for +// "github", so identity resolution (EffectiveAnnotations/isExactToolCallable) +// treats them exactly like the original five. +func seedCachePagingFixture(t *testing.T, proxy *MCPProxyServer, rt *runtime.Runtime) { + t.Helper() + extra := []stateview.ToolInfo{ + {Name: "cache_item_a", Description: "cachepagetest", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + {Name: "cache_item_b", Description: "cachepagetest", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + {Name: "cache_item_c", Description: "cachepagetest", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + } + rt.Supervisor().StateView().UpdateServer("github", func(s *stateview.ServerStatus) { + s.Tools = append(append([]stateview.ToolInfo{}, s.Tools...), extra...) + s.ToolCount = len(s.Tools) + }) + for _, tool := range extra { + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", ToolName: tool.Name, Status: storage.ToolApprovalStatusApproved, + })) + require.NoError(t, proxy.index.IndexTool(&config.ToolMetadata{ + Name: "github:" + tool.Name, ServerName: "github", Description: tool.Description, ParamsJSON: "{}", + Annotations: tool.Annotations, + })) + } + // The per-profile physical index is a snapshot COPY (Manager. + // RebuildProfileFromShared), taken once by indexEnforcementMatrixFixtureTools + // before these three docs existed — it must be rebuilt again now that the + // shared index holds them too, or a profiled search finds nothing. + require.NoError(t, proxy.index.RebuildProfileFromShared("work-readonly", []string{"github", "notion"})) +} + +// Spec 108 (Profiles v3) T020/T024 (FR-027): a read_cache page cached under +// one policy must be refused once that policy changes, even when the +// server-scope dimension (ProfileServers, already covered pre-108) is +// unchanged — a profile fingerprint edit, or an out-of-band tool +// re-annotation with no config change at all. +func TestCacheAuthz_ProfileV3_PolicyFingerprintAndToolTierGeneration(t *testing.T) { + proxy, rt := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + seedCachePagingFixture(t, proxy, rt) + + pinned := pinnedProfileCtx("work-readonly") + + readCacheReq := func(key string) *mcp.CallToolResult { + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"key": key, "offset": float64(0), "limit": float64(1)} + result, err := proxy.handleReadCache(pinned, req) + require.NoError(t, err) + return result + } + + retrieveArgs := map[string]interface{}{"query": "cachepagetest", "limit": float64(10)} + retrieveReq := func() mcp.CallToolRequest { + req := mcp.CallToolRequest{} + req.Params.Arguments = retrieveArgs + return req + } + + // Baseline (no truncation) to size the fixture, exactly as + // mcp_retrieve_tools_truncation_test.go does. + fullResult, err := proxy.handleRetrieveTools(pinned, retrieveReq()) + require.NoError(t, err) + full := resultText(t, fullResult) + limit := len(full) / 2 + require.Greater(t, limit, 40, "fixture must be large enough to exercise truncation") + + produceCacheKey := func() string { + setTruncateLimit(proxy, limit) + result, err := proxy.handleRetrieveTools(pinned, retrieveReq()) + require.NoError(t, err) + text := resultText(t, result) + setTruncateLimit(proxy, 1_000_000) + match := cacheKeyRE.FindStringSubmatch(text) + require.Len(t, match, 2, "the truncated response must carry a read_cache key: %s", text) + return match[1] + } + + t.Run("policy fingerprint edit invalidates a cached page (ProfileServers unchanged)", func(t *testing.T) { + key := produceCacheKey() + + control := readCacheReq(key) + require.False(t, control.IsError, "control: the same authorization must redeem its own entry") + + cfgCopy := *rt.Config() + cfg := &cfgCopy + newProfiles := append([]config.ProfileConfig{}, cfg.Profiles...) + for i := range newProfiles { + if newProfiles[i].Name != "work-readonly" { + continue + } + edited := *newProfiles[i].Tools + edited.Deny = append(append([]string{}, edited.Deny...), "github:list_issues") + newProfiles[i].Tools = &edited + } + cfg.Profiles = newProfiles + rt.UpdateConfig(cfg, "") + + again := readCacheReq(key) + assert.True(t, again.IsError, "editing the profile's tool policy must invalidate the page even though its server set did not change") + }) + + t.Run("out-of-band tool re-annotation (ToolTierGeneration) invalidates a cached page with no config edit", func(t *testing.T) { + key := produceCacheKey() + + control := readCacheReq(key) + require.False(t, control.IsError, "control: the same authorization must redeem its own entry") + + before := rt.Supervisor().ToolTierGeneration() + + // This test harness (startCountingUpstream) hand-writes StateView + // directly and never runs the Supervisor's own reconcile loop, so its + // internal ServerStateSnapshot never learns "github" exists — + // RefreshServerToolsFromDiscovery treats an unknown server as + // unconditionally accepted (publishDiscoveredTools's `!exists` + // branch), which is exactly the "out of band" shape this test wants: + // a zero-value capture is enough. Re-list github's tools with + // cache_item_a now DESTRUCTIVE instead of read-only — no config + // change at all. + reannotated := []*config.ToolMetadata{ + {Name: "cache_item_a", ServerName: "github", Description: "cachepagetest", Annotations: &config.ToolAnnotations{DestructiveHint: boolPtr(true)}}, + {Name: "cache_item_b", ServerName: "github", Description: "cachepagetest", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + {Name: "cache_item_c", ServerName: "github", Description: "cachepagetest", Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}, + } + published, err := rt.Supervisor().RefreshServerToolsFromDiscovery("github", reannotated, supervisor.DiscoveryCapture{}) + require.NoError(t, err) + require.True(t, published, "fixture: the re-list must land (github is unknown to the Supervisor's own reconcile state)") + + assert.Greater(t, rt.Supervisor().ToolTierGeneration(), before, "ToolTierGeneration must be bumped by the republish") + + again := readCacheReq(key) + assert.True(t, again.IsError, "a tool re-annotation must invalidate the cached page even with no config change") + }) +} From e8916394876b17492cbdcac018b4d7425dce4b6a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 18:54:33 +0300 Subject: [PATCH 4/7] docs(specs): mark Spec 108-b tasks complete in tasks.md T001-T003 (shared setup, already done in 108-a) and T016-T026 (profile-discovery-enforcement) checked off. Regenerated ROADMAP.md. --- ROADMAP.md | 2 +- specs/108-profiles-v3/tasks.md | 32 ++++++++++++++++---------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index b623d3cf2..7c0f28ad3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1035,5 +1035,5 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 94/113 (83%) | | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | -| [108-profiles-v3](./specs/108-profiles-v3/) | `drafted` | 0/153 (0%) | +| [108-profiles-v3](./specs/108-profiles-v3/) | `in-flight` | 16/153 (10%) | | [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `drafted` | 0/179 (0%) | diff --git a/specs/108-profiles-v3/tasks.md b/specs/108-profiles-v3/tasks.md index fce815c93..1bf0b428b 100644 --- a/specs/108-profiles-v3/tasks.md +++ b/specs/108-profiles-v3/tasks.md @@ -15,9 +15,9 @@ description: "Task list for Spec 108 — Profiles v3" ## Phase 1: Setup (shared) -- [ ] T001 Create enforcement-matrix fixture tool files `internal/server/testdata/profiles_v3/{github,notion,filesystem}.tools.json` (descriptions = tool names; annotations per contracts/enforcement-matrix.md) for `preflight_fixture_server.js` -- [ ] T002 [P] Add `internal/server/profiles_v3_fixture_test.go` helpers: `newProfilesV3Fixture(t)` (three fake upstreams + the three profiles; calls `profile.EnablePolicyForTest(t)` — the FR-009a test-only override, T009 — so the policy loads; tests using the fixture must not call `t.Parallel()`, because the override is process-wide), `anonCtx()`, reusing Spec 105 `agentCtx`/counting upstream helpers. **No `clientCtx` here**: it needs `AuthContext.TokenKind/ClientID/ProfileMode`, which land in 108-c (T027b adds it), so this shared file compiles in 108-a -- [ ] T003 [P] Add golden contract fixtures `internal/profile/testdata/contract/{profile_full,profile_legacy,effective_tools,client_rows,explain_blocked,activity_attributed}.json` (shapes per data-model.md; consumed by Go, vitest, Swift) +- [x] T001 Create enforcement-matrix fixture tool files `internal/server/testdata/profiles_v3/{github,notion,filesystem}.tools.json` (descriptions = tool names; annotations per contracts/enforcement-matrix.md) for `preflight_fixture_server.js` +- [x] T002 [P] Add `internal/server/profiles_v3_fixture_test.go` helpers: `newProfilesV3Fixture(t)` (three fake upstreams + the three profiles; calls `profile.EnablePolicyForTest(t)` — the FR-009a test-only override, T009 — so the policy loads; tests using the fixture must not call `t.Parallel()`, because the override is process-wide), `anonCtx()`, reusing Spec 105 `agentCtx`/counting upstream helpers. **No `clientCtx` here**: it needs `AuthContext.TokenKind/ClientID/ProfileMode`, which land in 108-c (T027b adds it), so this shared file compiles in 108-a +- [x] T003 [P] Add golden contract fixtures `internal/profile/testdata/contract/{profile_full,profile_legacy,effective_tools,client_rows,explain_blocked,activity_attributed}.json` (shapes per data-model.md; consumed by Go, vitest, Swift) --- @@ -52,23 +52,23 @@ description: "Task list for Spec 108 — Profiles v3" **Goal**: discovery hides excluded tools with `hidden_by_profile`. **Independent test**: US1 scenarios 1, 3 (discovery), 4, 6 (list), 7. ### Failing tests -- [ ] T016 [P] [US1] `internal/server/retrieve_tools_profile_v3_test.go`: matrix rows for retrieve_tools across the resolution sources available without 108-c (URL, session, legacy pin); binding and anonymous rows are added by **108-d T044a** (108-b and 108-c merge in parallel, so 108-d is the first PR with both); `hidden_by_profile` values 1/1/1/1 for "issue/secret/search/repo" under work-readonly; present with `0` under work-full (non-legacy); absent for legacy, including a legacy profile that sets only `title`; a legacy agent token pinned to a hand-deleted profile (dangling pin, FR-020) → deny-all with `hidden_by_profile: 0` and **no** `profile` field (base source, FR-011); `profile` present for the URL and `set_profile` sources only, absent for a legacy pin; authorized hits not displaced (limit=1 with a hidden higher-scoring tool) and `hidden_by_profile` counts a policy-excluded match that ranks below the cut -- [ ] T016a [P] [US1] `internal/index/search_admitted_test.go` (FR-011): `SearchToolsAdmitted` passes the canonical `server:tool` (and nothing from a stored annotation field — `ToolDocument` is unchanged) to the predicate, applies `limit` to admitted hits only (a rejected top hit never shortens the page), counts `RejectPolicy` but never `RejectScope` hits, pages exhaustively, and equals `SearchToolsScoped` when the predicate only checks the server -- [ ] T017 [P] [US1] Extend the Spec 105 two-fixture differential oracle with the v3 policy applied (`internal/server/scope_oracle_v3_test.go`): responses incl. `hidden_by_profile` equal across fixtures -- [ ] T018 [P] [US1] `internal/server/describe_tool_profile_v3_test.go`: excluded → uniform not-found equal to a nonexistent tool's response after substituting the echoed tool id (Spec 105 FR-010 comparison) -- [ ] T018a [P] [US1] `internal/server/prompts_profile_v3_test.go` (FR-019): under work-readonly (`max_tier: read`, deny `github:*secret*`) and a v3 profile whose deny rule matches a prompt name, `prompts/list` and `prompts/get` equal those of a legacy profile with the same `servers` — tool policy never filters prompts -- [ ] T019 [P] [US1] `internal/server/direct_profile_v3_test.go`: `/mcp/all` `tools/list` omits excluded tools for a pinned token -- [ ] T020 [P] [US1] `internal/server/cache_authz_policy_test.go`: page cached under policy P refused after P edited (fingerprint mismatch) and after pin changes; **tier generation**: a page cached while `github:search_code` was unannotated and classified `read` is refused by `read_cache` after the upstream re-lists it with `destructiveHint: true` and no profile edit (`ToolTierGeneration` mismatch, FR-027) -- [ ] T021 [US1] Golden `internal/server/testdata/retrieve_tools_profile_v3.golden.json`; legacy goldens byte-unchanged +- [x] T016 [P] [US1] `internal/server/retrieve_tools_profile_v3_test.go`: matrix rows for retrieve_tools across the resolution sources available without 108-c (URL, session, legacy pin); binding and anonymous rows are added by **108-d T044a** (108-b and 108-c merge in parallel, so 108-d is the first PR with both); `hidden_by_profile` values 1/1/1/1 for "issue/secret/search/repo" under work-readonly; present with `0` under work-full (non-legacy); absent for legacy, including a legacy profile that sets only `title`; a legacy agent token pinned to a hand-deleted profile (dangling pin, FR-020) → deny-all with `hidden_by_profile: 0` and **no** `profile` field (base source, FR-011); `profile` present for the URL and `set_profile` sources only, absent for a legacy pin; authorized hits not displaced (limit=1 with a hidden higher-scoring tool) and `hidden_by_profile` counts a policy-excluded match that ranks below the cut +- [x] T016a [P] [US1] `internal/index/search_admitted_test.go` (FR-011): `SearchToolsAdmitted` passes the canonical `server:tool` (and nothing from a stored annotation field — `ToolDocument` is unchanged) to the predicate, applies `limit` to admitted hits only (a rejected top hit never shortens the page), counts `RejectPolicy` but never `RejectScope` hits, pages exhaustively, and equals `SearchToolsScoped` when the predicate only checks the server +- [x] T017 [P] [US1] Extend the Spec 105 two-fixture differential oracle with the v3 policy applied (`internal/server/scope_oracle_v3_test.go`): responses incl. `hidden_by_profile` equal across fixtures +- [x] T018 [P] [US1] `internal/server/describe_tool_profile_v3_test.go`: excluded → uniform not-found equal to a nonexistent tool's response after substituting the echoed tool id (Spec 105 FR-010 comparison) +- [x] T018a [P] [US1] `internal/server/prompts_profile_v3_test.go` (FR-019): under work-readonly (`max_tier: read`, deny `github:*secret*`) and a v3 profile whose deny rule matches a prompt name, `prompts/list` and `prompts/get` equal those of a legacy profile with the same `servers` — tool policy never filters prompts +- [x] T019 [P] [US1] `internal/server/direct_profile_v3_test.go`: `/mcp/all` `tools/list` omits excluded tools for a pinned token +- [x] T020 [P] [US1] `internal/server/cache_authz_policy_test.go`: page cached under policy P refused after P edited (fingerprint mismatch) and after pin changes; **tier generation**: a page cached while `github:search_code` was unannotated and classified `read` is refused by `read_cache` after the upstream re-lists it with `destructiveHint: true` and no profile edit (`ToolTierGeneration` mismatch, FR-027) +- [x] T021 [US1] Golden `internal/server/testdata/retrieve_tools_profile_v3.golden.json`; legacy goldens byte-unchanged ### Implementation -- [ ] T022 [US1] Add `SearchToolsAdmitted` to `internal/index/manager.go` and `internal/index/bleve.go` (hit-level predicate, limit over admitted hits, policy-rejected count); switch the `retrieve_tools` handler in `internal/server/mcp.go` to it with a predicate that evaluates server scope then `CompiledPolicy.Decide` with the hit's effective annotations obtained from `profile.EffectiveAnnotations` (T013; no index-schema change, no reindex) — T016 asserts that a tool re-annotated `destructiveHint: true` in the StateView disappears from a `work-readonly` search on the next request without a reindex; keep the policy check in `indexedToolVisible` (`internal/server/mcp_visibility.go`) as post-cut defence in depth only; emit `profile` only for url/session sources -- [ ] T023 [US1] Apply the same check in `internal/server/mcp_describe_tool.go` and `internal/server/mcp_direct_scope.go` (list filter) -- [ ] T024 [US1] Add `PolicyFingerprint` and `ToolTierGeneration` (published with the (index, snapshot) pair, bumped on any effective-annotation change or tool add/remove) to the cache authorization stamp in `internal/server/cache_authz.go` and `internal/cache/authorization.go` -- [ ] T025 [P] [US1] SC-007 benchmark case (v3 vs legacy, 1,000 tools) in `internal/server/testdata/scope_latency/` + `TestScopeLatency` extension +- [x] T022 [US1] Add `SearchToolsAdmitted` to `internal/index/manager.go` and `internal/index/bleve.go` (hit-level predicate, limit over admitted hits, policy-rejected count); switch the `retrieve_tools` handler in `internal/server/mcp.go` to it with a predicate that evaluates server scope then `CompiledPolicy.Decide` with the hit's effective annotations obtained from `profile.EffectiveAnnotations` (T013; no index-schema change, no reindex) — T016 asserts that a tool re-annotated `destructiveHint: true` in the StateView disappears from a `work-readonly` search on the next request without a reindex; keep the policy check in `indexedToolVisible` (`internal/server/mcp_visibility.go`) as post-cut defence in depth only; emit `profile` only for url/session sources +- [x] T023 [US1] Apply the same check in `internal/server/mcp_describe_tool.go` and `internal/server/mcp_direct_scope.go` (list filter) +- [x] T024 [US1] Add `PolicyFingerprint` and `ToolTierGeneration` (published with the (index, snapshot) pair, bumped on any effective-annotation change or tool add/remove) to the cache authorization stamp in `internal/server/cache_authz.go` and `internal/cache/authorization.go` +- [x] T025 [P] [US1] SC-007 benchmark case (v3 vs legacy, 1,000 tools) in `internal/server/testdata/scope_latency/` + `TestScopeLatency` extension ### Verification -- [ ] T026 quickstart §1 + recipe 108-b; PR body lists the one added golden +- [x] T026 quickstart §1 + recipe 108-b; PR body lists the one added golden --- From 6d1eea60d3eabd6895e117244b57cba2803010f3 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 19:13:50 +0300 Subject: [PATCH 5/7] fix(profile): close 3 disclosure gaps zcode review found in 108-b 1. SearchToolsAdmitted stopped counting hiddenByPolicy as soon as the page filled with `limit` admitted hits, undercounting any policy-excluded match ranked below the cut. FR-011 requires the count over the full match set; the page now stops growing at `limit` but the scan keeps counting to the end. 2. collectQuarantinedToolMatches (the quarantined/pending "locked" entry pass) filtered only by server scope, so a tool that was BOTH policy-excluded and quarantined/pending/changed was named in the disabled[] array and counted as "locked" instead of staying silently invisible like every other excluded tool. Post-filtered through the same CompiledPolicy.Decide the rest of discovery uses. 3. describe_tool's policy re-check ran after the quarantine/pending/ callability gates, so an excluded-and-locked tool answered its lock reason (confirming existence) instead of the uniform not-found the contract requires. Moved right after the scope check, before every gate that would otherwise narrow an admitted tool's shape. Each fix ships with a regression test reproducing the exact scenario. --- internal/index/bleve.go | 14 +++-- internal/index/search_admitted_test.go | 37 ++++++++++++ .../server/describe_tool_profile_v3_test.go | 20 +++++++ internal/server/mcp.go | 37 ++++++++++++ .../server/mcp_quarantine_policy_v3_test.go | 59 +++++++++++++++++++ internal/server/mcp_visibility.go | 39 ++++++------ 6 files changed, 185 insertions(+), 21 deletions(-) create mode 100644 internal/server/mcp_quarantine_policy_v3_test.go diff --git a/internal/index/bleve.go b/internal/index/bleve.go index 666b44bb5..2c87f6014 100644 --- a/internal/index/bleve.go +++ b/internal/index/bleve.go @@ -804,15 +804,21 @@ func (b *BleveIndex) SearchToolsAdmitted(queryStr string, limit int, admit func( tool := readToolMetadata(hit.ID, hit.Fields) switch admit(Hit{Server: tool.ServerName, Tool: config.RawToolName(tool)}) { case Admit: - results = append(results, &config.SearchResult{Tool: tool, Score: hit.Score}) + // The page itself stops growing once it holds `limit` + // admitted hits, but the SCAN does not stop here: a + // RejectPolicy hit ranked below the cut must still be + // counted (FR-011 "over the full match set" — codex/zcode + // review round 1). Returning as soon as the page filled + // silently undercounted hiddenByPolicy for every match + // ranked after the limit-th admitted one. + if len(results) < limit { + results = append(results, &config.SearchResult{Tool: tool, Score: hit.Score}) + } case RejectPolicy: hiddenByPolicy++ case RejectScope: // Invisible: never counted, never collected. } - if len(results) >= limit { - return results, hiddenByPolicy, nil - } } if len(searchResult.Hits) == 0 || uint64(from+pageSize) >= searchResult.Total { break diff --git a/internal/index/search_admitted_test.go b/internal/index/search_admitted_test.go index 43ff9df6c..7228ae990 100644 --- a/internal/index/search_admitted_test.go +++ b/internal/index/search_admitted_test.go @@ -123,6 +123,43 @@ func TestSearchToolsAdmitted_FiltersBeforeLimitLikeScoped(t *testing.T) { assert.Equal(t, 1, hiddenByPolicy, "the excluded in-scope tool must be counted") }) + t.Run("hiddenByPolicy still counts a RejectPolicy hit ranked BELOW an already-full page (zcode review round 1)", func(t *testing.T) { + // Two admitted "a" tools plus one policy-excluded "a" tool that + // ranks LAST among the three (single mention vs the others' + // heavier repetition) — with limit=2 the page fills on the first + // two admitted hits, and the excluded one is scanned afterward. + idx2, err := NewBleveIndex(t.TempDir(), zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = idx2.Close() }) + require.NoError(t, idx2.IndexTool(&config.ToolMetadata{ + Name: "a:keep_one", ServerName: "a", + Description: "gadget gadget gadget gadget gadget gadget gadget gadget", ParamsJSON: "{}", + })) + require.NoError(t, idx2.IndexTool(&config.ToolMetadata{ + Name: "a:keep_two", ServerName: "a", + Description: "gadget gadget gadget gadget gadget gadget", ParamsJSON: "{}", + })) + require.NoError(t, idx2.IndexTool(&config.ToolMetadata{ + Name: "a:excluded_one", ServerName: "a", + Description: "gadget", ParamsJSON: "{}", + })) + + admit := func(h Hit) Admission { + if h.Server != "a" { + return RejectScope + } + if h.Tool == "excluded_one" { + return RejectPolicy + } + return Admit + } + results, hiddenByPolicy, err := idx2.SearchToolsAdmitted("gadget", 2, admit) + require.NoError(t, err) + require.Len(t, results, 2, "the page must fill with the two admitted hits") + assert.Equal(t, 1, hiddenByPolicy, + "the excluded hit ranked below the already-full page must still be counted (FR-011: counted over the full match set)") + }) + t.Run("RejectScope is never counted in hiddenByPolicy", func(t *testing.T) { _, hiddenByPolicy, err := idx.SearchToolsAdmitted(admittedSeamQuery, 10, serverOnlyAdmit(inScopeA)) require.NoError(t, err) diff --git a/internal/server/describe_tool_profile_v3_test.go b/internal/server/describe_tool_profile_v3_test.go index f59955d41..848afa416 100644 --- a/internal/server/describe_tool_profile_v3_test.go +++ b/internal/server/describe_tool_profile_v3_test.go @@ -5,6 +5,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) // Spec 108 (Profiles v3) T018: describe_tool on a profile-excluded tool must @@ -47,6 +49,24 @@ func TestDescribeTool_ProfileV3_ExcludedEqualsNonexistent(t *testing.T) { require.Len(t, resp.Definitions, 1, "legacy has no policy field set — create_issue is simply admitted") }) + t.Run("a tool that is BOTH policy-excluded and pending approval still answers the uniform not-found, never the pending lock (zcode review round 1)", func(t *testing.T) { + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", ToolName: "create_issue", Status: storage.ToolApprovalStatusPending, + })) + t.Cleanup(func() { + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", ToolName: "create_issue", Status: storage.ToolApprovalStatusApproved, + })) + }) + + resp := callDescribe(t, proxy, ctx, []interface{}{"github:create_issue"}) + require.Empty(t, resp.Definitions) + require.Len(t, resp.Errors, 1) + resp.Errors[0]["id"] = "SUBSTITUTED" + assert.Equal(t, nonexistent.Errors[0], resp.Errors[0], + "a policy exclusion must win over the pending-approval lock's own, more specific reason — describe_tool must never confirm the tool exists") + }) + t.Run("pin source: the same uniform not-found, without ever naming the profile", func(t *testing.T) { resp := callDescribe(t, proxy, pinnedProfileCtx("work-readonly"), []interface{}{"github:create_issue"}) require.Len(t, resp.Errors, 1) diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 9dcf4d755..83256dcbc 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -2076,6 +2076,15 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques seen[e.Name] = true } quarantinedMatches := p.collectQuarantinedToolMatches(query, serverDiscoverable, seen, p.serverToolNames) + // Spec 108 FR-011/FR-013 (zcode review round 1): collectQuarantinedToolMatches + // filters only by server scope, so a tool that is BOTH policy-excluded and + // quarantined/pending/changed would otherwise be named as a locked entry + // and counted as "locked" — exactly the naming and miscounting FR-013 + // forbids for a profile-hidden tool. Post-filtered here (rather than + // threading policy into the shared helper, which a pre-existing, + // non-v3 test file also calls) so a policy exclusion makes such a tool + // silently invisible, precisely like an in-scope-but-excluded index hit. + quarantinedMatches = p.filterLockedMatchesByPolicy(quarantinedMatches, policy) droppedCount += len(quarantinedMatches) if includeDisabled { disabledEntries = append(quarantinedMatches, disabledEntries...) @@ -7201,6 +7210,34 @@ func disabledToolRemediation(status contracts.DisabledToolStatus) string { // (min(limit,10)); this just bounds work on the opt-in path. const maxQuarantinedMatches = 50 +// filterLockedMatchesByPolicy drops any collectQuarantinedToolMatches entry +// the Spec 108 tool policy excludes (FR-011/FR-013): that helper filters only +// by server scope, so without this pass a policy-excluded tool that also +// happens to be quarantined or pending/changed approval would be NAMED as a +// locked entry (and counted as one) instead of staying silently invisible +// like every other policy-excluded tool. policy nil (no profile, or a legacy +// one) is a no-op — matches pass through unfiltered, byte-identical to +// pre-108 (SC-003). Tools resolved through the same seam every other 108-b +// check uses (profile.EffectiveAnnotations = resolveExactToolIdentity), so a +// quarantined tool this proxy cannot classify (identity unresolved) fails +// closed to destructive, exactly like every other enforcement point. +func (p *MCPProxyServer) filterLockedMatchesByPolicy(matches []contracts.LockedToolEntry, policy *profile.CompiledPolicy) []contracts.LockedToolEntry { + if policy == nil || len(matches) == 0 { + return matches + } + filtered := make([]contracts.LockedToolEntry, 0, len(matches)) + for _, m := range matches { + toolName := strings.TrimPrefix(m.Name, m.Server+":") + annotations, found := p.EffectiveAnnotations(m.Server, toolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(m.Server, toolName, intrinsic); !admitted { + continue + } + filtered = append(filtered, m) + } + return filtered +} + // collectQuarantinedToolMatches finds tools that exist but are quarantined and // whose name matches the query, returning lean locked entries (no description // or schema — those are withheld because a quarantined tool's description is a diff --git a/internal/server/mcp_quarantine_policy_v3_test.go b/internal/server/mcp_quarantine_policy_v3_test.go new file mode 100644 index 000000000..9b887bb9b --- /dev/null +++ b/internal/server/mcp_quarantine_policy_v3_test.go @@ -0,0 +1,59 @@ +package server + +import ( + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// Spec 108 (Profiles v3) FR-011/FR-013 (zcode review round 1): a tool that is +// BOTH policy-excluded AND pending/changed approval must stay silently +// invisible, exactly like every other policy-excluded tool — never named as +// a "locked" entry, never counted as one. collectQuarantinedToolMatches +// filters only by server scope, so without filterLockedMatchesByPolicy such +// a tool would be named in the disabled[] array (a TPA-relevant disclosure: +// the response would confirm the tool's existence via a lock reason) and +// double-counted (once in hidden_by_profile, once in droppedCount/the +// zero-result notice). +type quarantinePolicyRetrieveResponse struct { + Tools []map[string]interface{} `json:"tools"` + Disabled []contracts.LockedToolEntry `json:"disabled"` + HiddenByProfile *int `json:"hidden_by_profile"` + Notice *string `json:"notice"` +} + +func TestRetrieveTools_ProfileV3_PolicyExcludedPendingToolNeverNamedAsLocked(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + // github:create_issue is write-tier, excluded under work-readonly's + // read cap (FR-010 above_tier_cap) — and, on top of that, its approval + // record is now pending, so BOTH gates would separately exclude it. + require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", ToolName: "create_issue", Status: storage.ToolApprovalStatusPending, + })) + + ctx := urlProfileCtx(proxy, "work-readonly") + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"query": "create_issue", "limit": float64(10), "include_disabled": true} + result, err := proxy.handleRetrieveTools(ctx, req) + require.NoError(t, err) + require.False(t, result.IsError) + + var resp quarantinePolicyRetrieveResponse + require.NoError(t, json.Unmarshal([]byte(resultText(t, result)), &resp)) + + assert.Empty(t, resp.Tools, "the excluded tool must not appear in tools[]") + for _, d := range resp.Disabled { + assert.NotContains(t, d.Name, "create_issue", + "a policy-excluded tool must never be named as a locked entry, even when it also has a pending approval record") + } + require.NotNil(t, resp.HiddenByProfile) + assert.Equal(t, 1, *resp.HiddenByProfile, "the tool is still counted via hidden_by_profile, exactly once") +} diff --git a/internal/server/mcp_visibility.go b/internal/server/mcp_visibility.go index 980d13544..e9d46c336 100644 --- a/internal/server/mcp_visibility.go +++ b/internal/server/mcp_visibility.go @@ -124,6 +124,28 @@ func (p *MCPProxyServer) toolVisibleToSession(ctx context.Context, serverName, t return false, visReasonServerNotInScope } } + // Spec 108 FR-011/T023 (zcode review round 1): describe_tool's contract + // makes NO shape change for an excluded tool — the caller must see the + // SAME uniform not-found response a nonexistent id gets + // (contracts/mcp-tools.md "describe_tool"), for EVERY excluded tool, + // including one that also happens to be quarantined, pending/changed, or + // operator-disabled. Ordered right after the scope gate (before the + // identity/lock gates below) precisely so it wins over their more + // specific reasons: those gates exist to narrow what an otherwise- + // admitted tool reveals, and a policy exclusion must never be + // downgraded to a shape that confirms the tool's existence (a lock + // reason does exactly that). Safe to run even when profileName is + // legacy or "" (policy is then nil, or Decide only ever agrees with the + // scope check already passed above). + if profileName != "" { + if policy := profileIdx.PolicyFor(profileName); policy != nil { + annotations, found := p.EffectiveAnnotations(serverName, toolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(serverName, toolName, intrinsic); !admitted { + return false, visReasonToolPolicyExcluded + } + } + } // Spec 105 FR-009 (research D4), astra r2 C2: an index document is not a // registration identity. A name the KNOWN, CONNECTED server's completed // discovery does not list — a stale document whose Bleve delete failed, @@ -149,23 +171,6 @@ func (p *MCPProxyServer) toolVisibleToSession(ctx context.Context, serverName, t if !p.isExactToolCallable(serverName, toolName) { return false, visReasonToolNotCallable } - // Spec 108 FR-011/T023: describe_tool's contract makes no shape change - // for an excluded tool — the caller must see the SAME uniform not-found - // response a nonexistent id gets (contracts/mcp-tools.md "describe_tool"). - // Ordered LAST, after every other gate, so a quarantined/pending/disabled - // tool still reports its own, more specific reason first; a policy - // exclusion only ever narrows what an otherwise-visible tool reveals. - // Safe to run even when profileName is legacy or "" (policy is then nil - // or Decide only ever agrees with the scope check already passed above). - if profileName != "" { - if policy := profileIdx.PolicyFor(profileName); policy != nil { - annotations, found := p.EffectiveAnnotations(serverName, toolName) - intrinsic := profile.IntrinsicTier(annotations, found) - if admitted, _, _ := policy.Decide(serverName, toolName, intrinsic); !admitted { - return false, visReasonToolPolicyExcluded - } - } - } return true, "" } From b6a214a5f81c14dd415a6d54932b234717dcfd48 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 19:24:07 +0300 Subject: [PATCH 6/7] docs(index): fix stale SearchToolsAdmitted doc comment (zcode round 2 nit) Round 2 review confirmed the round-1 fixes are correct, but flagged that the doc comment still claimed identical exhaustive paging to SearchToolsScoped -- the two now differ in stop rule on purpose (this method never early-returns, so hiddenByPolicy counts the full match set). Comment-only change; behaviour unchanged. --- internal/index/bleve.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/index/bleve.go b/internal/index/bleve.go index 2c87f6014..baab2e591 100644 --- a/internal/index/bleve.go +++ b/internal/index/bleve.go @@ -769,12 +769,17 @@ const ( // caller whose page filled up before scanning every match still gets an // accurate count. // -// Identical to SearchToolsScoped in every other respect (same query -// construction incl. the underscore-segment enhancement, same score-then-id -// sort, same exhaustive From/Size paging with no result cap) — a predicate -// that only ever returns Admit/RejectScope (never RejectPolicy) makes this -// method equal SearchToolsScoped(query, limit, func(s string) bool { admit -// still sees the server only }) exactly (T016a). +// Identical to SearchToolsScoped in query construction (incl. the +// underscore-segment enhancement) and score-then-id sort. It differs in ONE +// respect, precisely because of the counting requirement above: +// SearchToolsScoped returns as soon as its page fills, while this method +// keeps paging to the end of the exhaustive match set regardless (zcode +// review round 1 — an early return there silently undercounted +// hiddenByPolicy for any RejectPolicy hit ranked below the cut). A predicate +// that only ever returns Admit/RejectScope (never RejectPolicy) still makes +// this method's RESULT SET equal SearchToolsScoped(query, limit, func(s +// string) bool { admit still sees the server only })'s exactly (T016a) — +// the extra scanning costs work, never a different admitted page. func (b *BleveIndex) SearchToolsAdmitted(queryStr string, limit int, admit func(Hit) Admission) (results []*config.SearchResult, hiddenByPolicy int, err error) { if queryStr == "" { return nil, 0, fmt.Errorf("search query cannot be empty") From dd30aa4d0f304675b36c272adc7f5591236f2b6e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 26 Sep 2026 20:54:17 +0300 Subject: [PATCH 7/7] fix(server): close 3 zcode round-1 gaps in Spec 108 profile enforcement - retrieve_tools' usageStatEligible (include_stats filter) now applies the same Spec 108 policy gate its sibling resolvers (toolVisibleToSession, indexedToolVisible) already had, so a policy-hidden tool can no longer be named in usage_summary.top_tools for a profile-scoped caller even though an earlier caller's usage created a fleet-wide stats record for it (F1). - TestScopeOracleV3_HiddenByProfileIdenticalAcrossFixtures now probes with the hidden servers' own sentinel tool names, not only queries that can never match them, so the differential oracle can actually detect a scope leak instead of comparing two empty result sets (F2). - TestRetrieveTools_ScopeLatency_ProfileV3VsLegacy now rebuilds the two per-profile Bleve indexes from the shared one before measuring, so the benchmark scans real hits and actually pays the policy path's per-hit cost instead of two no-op searches against empty ForProfile stores (F3). Each fix is verified test-first: the new/extended assertions reproduce red against the pre-fix code and pass after it. --- internal/server/mcp.go | 2 +- internal/server/mcp_visibility.go | 24 +++++++++- .../server/scope_latency_profile_v3_test.go | 30 +++++++++++++ internal/server/scope_oracle_v3_test.go | 14 +++++- .../server/usage_stats_profile_v3_test.go | 45 +++++++++++++++++++ 5 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 internal/server/usage_stats_profile_v3_test.go diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 83256dcbc..9eeb9fbc1 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -2336,7 +2336,7 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques if !ok { return false } - return p.usageStatEligible(authCtx, profileScope, serverName, rawToolName) + return p.usageStatEligible(authCtx, profileScope, policy, serverName, rawToolName) }, 10) } else { stats, statsErr = p.storage.GetToolStats(10) diff --git a/internal/server/mcp_visibility.go b/internal/server/mcp_visibility.go index e9d46c336..a3fbcb97f 100644 --- a/internal/server/mcp_visibility.go +++ b/internal/server/mcp_visibility.go @@ -321,7 +321,17 @@ func (p *MCPProxyServer) scopedIndexedToolCount(discoverable func(serverName str // stricter than indexedToolVisible (the SEARCH gate, which stays permissive // for pending/changed tools per FR-006 byte-identity): usage ranking is not // search, and a still-under-review tool should not be recommended by name. -func (p *MCPProxyServer) usageStatEligible(authCtx *auth.AuthContext, profileScope *profile.ProfileScope, serverName, toolName string) bool { +// +// policy is the Spec 108 compiled policy for the caller's effective profile +// (nil when none is in effect, or the profile is legacy), consulted exactly +// like indexedToolVisible/toolVisibleToSession's own policy gate (FR-011): a +// usage record's tool is fleet-wide (IncrementToolUsage keys only on tool +// name, so ANY earlier caller's calls create it, not necessarily this one's), +// so without this gate a caller whose profile policy excludes the tool could +// still see it NAMED in usage_summary.top_tools — a stronger disclosure than +// tools[] (which already omits it) or hidden_by_profile (which already +// counts it as hidden) ever intends. +func (p *MCPProxyServer) usageStatEligible(authCtx *auth.AuthContext, profileScope *profile.ProfileScope, policy *profile.CompiledPolicy, serverName, toolName string) bool { if !p.serverInScope(authCtx, profileScope, serverName) { return false } @@ -331,7 +341,17 @@ func (p *MCPProxyServer) usageStatEligible(authCtx *auth.AuthContext, profileSco if !p.isExactToolCallable(serverName, toolName) { return false } - return p.describeGateReason(serverName, toolName) == "" + if p.describeGateReason(serverName, toolName) != "" { + return false + } + if policy != nil { + annotations, found := p.EffectiveAnnotations(serverName, toolName) + intrinsic := profile.IntrinsicTier(annotations, found) + if admitted, _, _ := policy.Decide(serverName, toolName, intrinsic); !admitted { + return false + } + } + return true } // toolIndexed reports whether the tool is present in the shared search index diff --git a/internal/server/scope_latency_profile_v3_test.go b/internal/server/scope_latency_profile_v3_test.go index 047953209..09648047e 100644 --- a/internal/server/scope_latency_profile_v3_test.go +++ b/internal/server/scope_latency_profile_v3_test.go @@ -68,6 +68,16 @@ func TestRetrieveTools_ScopeLatency_ProfileV3VsLegacy(t *testing.T) { {Name: "legacy-samescope", Servers: servers}, } require.NoError(t, proxy.index.BatchIndexTools(tools)) + // zcode review F3: retrieve_tools resolves searchIndex via + // index.ForProfile(profileName) — a lazily-created, initially EMPTY + // per-profile store that is populated only by RebuildProfileFromShared. + // BatchIndexTools above only reaches the SHARED default index, so + // without this the per-profile search below scans zero hits for BOTH + // contexts: the policy path's actual per-hit cost (EffectiveAnnotations + // + CompiledPolicy.Decide) is never paid, and the asserted p95 gap is + // measuring two no-op searches against each other. + require.NoError(t, proxy.index.RebuildProfileFromShared("v3-cap-read", servers)) + require.NoError(t, proxy.index.RebuildProfileFromShared("legacy-samescope", servers)) const query = "get data" const limit = 10 @@ -86,6 +96,26 @@ func TestRetrieveTools_ScopeLatency_ProfileV3VsLegacy(t *testing.T) { legacyCtx := profile.WithProfileScope(context.Background(), proxy.profileScopeForSlug("legacy-samescope")) v3Ctx := profile.WithProfileScope(context.Background(), proxy.profileScopeForSlug("v3-cap-read")) + // Fixture sanity (zcode review F3): the per-profile PHYSICAL INDEX each + // context searches must actually hold real, matching documents, or the + // p95 gap asserted below is measuring two no-op scans against each + // other rather than the policy path's real per-hit cost. Checked via a + // direct index search — NOT retrieve_tools' own JSON `total` — because + // this synthetic corpus's servers are indexed but never connected to a + // live upstream, so retrieve_tools' unrelated callability gate (no + // server is "callable" without a real connection) empties tools[] + // downstream of the scan this test cares about, for both profiles + // alike; that gate is orthogonal to whether SearchToolsAdmitted itself + // paid the EffectiveAnnotations+CompiledPolicy.Decide cost per hit. + for _, slug := range []string{"legacy-samescope", "v3-cap-read"} { + pIdx, err := proxy.index.ForProfile(slug) + require.NoError(t, err) + hits, err := pIdx.Search(query, limit) + require.NoError(t, err) + require.NotEmptyf(t, hits, + "%s: per-profile index must hold real matching documents, not an empty ForProfile index never populated by RebuildProfileFromShared", slug) + } + legacyDurations := measure(legacyCtx) v3Durations := measure(v3Ctx) diff --git a/internal/server/scope_oracle_v3_test.go b/internal/server/scope_oracle_v3_test.go index 9f1ed5bcd..379b3ad6d 100644 --- a/internal/server/scope_oracle_v3_test.go +++ b/internal/server/scope_oracle_v3_test.go @@ -98,7 +98,19 @@ func TestScopeOracleV3_HiddenByProfileIdenticalAcrossFixtures(t *testing.T) { pinned := pinnedProfileCtx("cap-read-a") - for _, query := range []string{"read_thing", "write_thing", "destroy_thing", "erase", "ns_erase"} { + // The two sentinel-matching queries are the actual scope-leak probe + // (zcode review F2): the five original queries share no token at all + // with either hidden server's tool name/description, so a regression + // that let a hidden server's tool reach the pinned caller's response + // could pass every assertion below completely unnoticed — the oracle + // was only ever comparing two empty result sets to each other. These two + // queries are chosen to hit deterministically (proven by the admin + // control subtest) and must return NOTHING for the pinned caller. + queries := []string{ + "read_thing", "write_thing", "destroy_thing", "erase", "ns_erase", + "SENTINEL_scopeOracleV3B_71a2_tool", "SENTINEL_scopeOracleV3AB_39fe_tool", + } + for _, query := range queries { t.Run(query, func(t *testing.T) { narrowResp := callRetrieveToolsV3(t, narrow.proxy, pinned, query, 10) fullResp := callRetrieveToolsV3(t, full.proxy, pinned, query, 10) diff --git a/internal/server/usage_stats_profile_v3_test.go b/internal/server/usage_stats_profile_v3_test.go new file mode 100644 index 000000000..5d6da3160 --- /dev/null +++ b/internal/server/usage_stats_profile_v3_test.go @@ -0,0 +1,45 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" +) + +// Spec 108 FR-011: usageStatEligible (retrieve_tools' include_stats filter) +// must apply the same v3 tool-policy check its two sibling resolvers +// (toolVisibleToSession, indexedToolVisible) already receive, so a +// policy-hidden tool can never be NAMED in usage_summary.top_tools — even +// though tools[] correctly omits it and hidden_by_profile correctly counts +// it as hidden. Reproduces zcode review finding F1. +func TestRetrieveTools_ProfileV3_UsageStatsExcludePolicyHiddenTool(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + indexEnforcementMatrixFixtureTools(t, proxy) + + // github:create_issue is excluded from "work-readonly" by the profile's + // MaxTier:"read" cap (create_issue is a write-tier tool) — an EARLIER + // caller's usage (fleet-wide stats: IncrementToolUsage is keyed only by + // tool name, not by caller) leaves a usage record behind. + require.NoError(t, proxy.storage.IncrementToolUsage("github:create_issue")) + require.NoError(t, proxy.storage.IncrementToolUsage("github:list_issues")) + + ctx := pinnedProfileCtx("work-readonly") + resp := callRetrieveScoped(t, proxy, ctx, map[string]interface{}{ + "query": "list_issues", "include_stats": true, + }) + + require.NotNil(t, resp.UsageSummary.TopTools) + for _, stat := range resp.UsageSummary.TopTools { + assert.NotEqual(t, "github:create_issue", stat["tool_name"], + "FR-011: usage_summary.top_tools must never name a tool the profile policy hides, "+ + "even though tools[] omits it and hidden_by_profile counts it") + } + + // Sanity control: the caller really is scoped (IsScopedCaller true), + // otherwise the assertion above would pass vacuously via the admin + // (unfiltered GetToolStats) branch. + require.True(t, auth.IsScopedCaller(ctx)) +}