diff --git a/docs/adding-tools.md b/docs/adding-tools.md index 5026881b..0ce7dfe4 100644 --- a/docs/adding-tools.md +++ b/docs/adding-tools.md @@ -13,6 +13,7 @@ Not every tool is exposed to every consumer. We rely on several mechanisms to ke - **`requiredCapabilities`** — Tools declare which project capabilities they need (e.g. `profiles`, `replays`, `traces`). If the upstream project doesn't have a capability enabled, the tool is automatically hidden. - **`experimental` / `hideInExperimentalMode`** — Feature flags for tools that are being tested or replaced. - **Skills & constraints** — The server filters tools based on granted skills and org/project constraints. +- **Experimental skill merging** — A skill can set `mergedIntoSkillInExperimentalMode` to make its tools available through another skill in experimental mode only. For example, `preprod` currently merges into `inspect` for `?experimental=1` sessions. We also expect upstream consumers (Claude Code plugins, Cursor, etc.) to use **tool selection** or **progressive disclosure** on their end. The catalog can contain more tools than the direct MCP surface, but the registered top-level tool count must still stay within the limits below. diff --git a/packages/mcp-cloudflare/src/server/lib/approval-dialog.test.ts b/packages/mcp-cloudflare/src/server/lib/approval-dialog.test.ts index a14556b5..824316e1 100644 --- a/packages/mcp-cloudflare/src/server/lib/approval-dialog.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/approval-dialog.test.ts @@ -104,6 +104,41 @@ describe("approval-dialog", () => { expect(html).toContain(">javascript project"); expect(html).toContain(">sentry organization"); }); + + it("should render merged skills separately outside experimental mode", async () => { + const response = await renderApprovalDialog( + new Request("https://example.com/oauth/authorize"), + { + client: mockClient, + server: { name: "Sentry MCP" }, + state: { oauthReqInfo: { clientId: "test-client" } }, + cookieSecret: TEST_SECRET, + }, + ); + + const html = await response.text(); + + expect(html).toContain("Inspect Issues & Events"); + expect(html).toContain("Preprod Snapshots"); + }); + + it("should hide merged skills in experimental mode", async () => { + const response = await renderApprovalDialog( + new Request("https://example.com/oauth/authorize"), + { + client: mockClient, + server: { name: "Sentry MCP" }, + experimentalMode: true, + state: { oauthReqInfo: { clientId: "test-client" } }, + cookieSecret: TEST_SECRET, + }, + ); + + const html = await response.text(); + + expect(html).toContain("Inspect Issues & Events"); + expect(html).not.toContain("Preprod Snapshots"); + }); }); describe("CSRF protection with HMAC-signed state", () => { diff --git a/packages/mcp-cloudflare/src/server/lib/approval-dialog.ts b/packages/mcp-cloudflare/src/server/lib/approval-dialog.ts index e0471671..b9422d32 100644 --- a/packages/mcp-cloudflare/src/server/lib/approval-dialog.ts +++ b/packages/mcp-cloudflare/src/server/lib/approval-dialog.ts @@ -199,6 +199,10 @@ export interface ApprovalDialogOptions { organizationSlug: string; projectSlug: string | null; } | null; + /** + * Whether the OAuth grant is for an experimental MCP endpoint. + */ + experimentalMode?: boolean; /** * The specific redirect URI selected for this authorization request. */ @@ -214,6 +218,56 @@ export interface ApprovalDialogOptions { cookieSecret: string; } +function getSkillsForApproval( + skills: SkillDefinition[], + experimentalMode: boolean, +): SkillDefinition[] { + if (!experimentalMode) { + return skills; + } + + const visibleSkills = skills + .filter((skill) => !skill.mergedIntoSkillInExperimentalMode) + .map((skill) => ({ ...skill })); + + for (const mergedSkill of skills) { + const targetSkillId = mergedSkill.mergedIntoSkillInExperimentalMode; + if (!targetSkillId) { + continue; + } + + const targetSkill = visibleSkills.find( + (skill) => skill.id === targetSkillId, + ); + if (!targetSkill) { + continue; + } + + if (targetSkill.tools || mergedSkill.tools) { + const toolsByName = new Map( + (targetSkill.tools ?? []).map((tool) => [tool.name, tool]), + ); + for (const tool of mergedSkill.tools ?? []) { + toolsByName.set(tool.name, tool); + } + targetSkill.tools = Array.from(toolsByName.values()).sort((a, b) => + a.name.localeCompare(b.name), + ); + targetSkill.toolCount = targetSkill.tools.length; + continue; + } + + if (mergedSkill.toolCount === undefined) { + continue; + } + + targetSkill.toolCount = + (targetSkill.toolCount ?? 0) + mergedSkill.toolCount; + } + + return visibleSkills; +} + /** * Encodes and signs arbitrary data to a HMAC-signed compact string. * @param data - The data to encode (will be stringified). @@ -279,10 +333,21 @@ export async function renderApprovalDialog( request: Request, options: ApprovalDialogOptions, ): Promise { - const { client, server, scope, redirectUri, state, cookieSecret } = options; + const { + client, + server, + scope, + experimentalMode = false, + redirectUri, + state, + cookieSecret, + } = options; // Use static skill definitions bundled at build time - const skills: SkillDefinition[] = skillDefinitions as SkillDefinition[]; + const skills = getSkillsForApproval( + skillDefinitions as SkillDefinition[], + experimentalMode, + ); // Generate HTML for all skills (checked if defaultEnabled) const skillsHtml = skills diff --git a/packages/mcp-cloudflare/src/server/oauth/resource-scope.test.ts b/packages/mcp-cloudflare/src/server/oauth/resource-scope.test.ts new file mode 100644 index 00000000..7e68dbf2 --- /dev/null +++ b/packages/mcp-cloudflare/src/server/oauth/resource-scope.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; +import { + parseResourceExperimentalMode, + parseResourceMcpConstraints, +} from "./resource-scope"; + +describe("resource-scope", () => { + describe("parseResourceMcpConstraints", () => { + it("parses organization and project constraints from MCP resource URLs", () => { + expect( + parseResourceMcpConstraints("https://example.com/mcp/sentry/frontend"), + ).toEqual({ + organizationSlug: "sentry", + projectSlug: "frontend", + }); + }); + }); + + describe("parseResourceExperimentalMode", () => { + it("detects experimental MCP resource URLs", () => { + expect( + parseResourceExperimentalMode( + "https://example.com/mcp/sentry/frontend?experimental=1", + ), + ).toBe(true); + }); + + it("returns false for stable or invalid resource URLs", () => { + expect( + parseResourceExperimentalMode("https://example.com/mcp/sentry"), + ).toBe(false); + expect(parseResourceExperimentalMode("not a url")).toBe(false); + }); + }); +}); diff --git a/packages/mcp-cloudflare/src/server/oauth/resource-scope.ts b/packages/mcp-cloudflare/src/server/oauth/resource-scope.ts index 925f5be6..00de40eb 100644 --- a/packages/mcp-cloudflare/src/server/oauth/resource-scope.ts +++ b/packages/mcp-cloudflare/src/server/oauth/resource-scope.ts @@ -36,3 +36,17 @@ export function parseResourceMcpConstraints( return null; } } + +export function parseResourceExperimentalMode( + resource: string | null | undefined, +): boolean { + if (!resource) { + return false; + } + + try { + return new URL(resource).searchParams.get("experimental") === "1"; + } catch { + return false; + } +} diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index 5a3a9553..ad5fee73 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -16,7 +16,10 @@ import { import { SCOPES } from "../../../constants"; import { signState, type OAuthState } from "../state"; import { logWarn } from "@sentry/mcp-core/telem/logging"; -import { parseResourceMcpConstraints } from "../resource-scope"; +import { + parseResourceExperimentalMode, + parseResourceMcpConstraints, +} from "../resource-scope"; /** * Extended AuthRequest that includes skills and resource parameter @@ -143,6 +146,7 @@ export default new Hono<{ Bindings: Env }>() ...(resourceParam ? { resource: resourceParam } : {}), }; const approvalScope = parseResourceMcpConstraints(resourceParam); + const experimentalMode = parseResourceExperimentalMode(resourceParam); // XXX(dcramer): we want to confirm permissions on each time // so you can always choose new ones @@ -168,6 +172,7 @@ export default new Hono<{ Bindings: Env }>() name: "Sentry MCP", }, scope: approvalScope, + experimentalMode, redirectUri: oauthReqInfoWithResource.redirectUri, state: { oauthReqInfo: oauthReqInfoWithResource }, cookieSecret: c.env.COOKIE_SECRET, diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts index 856d9efc..bbf9e6b3 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts @@ -14,7 +14,10 @@ import { getOAuthCallbackFailureDetails, validateResourceParameter, } from "../helpers"; -import { parseResourceMcpConstraints } from "../resource-scope"; +import { + parseResourceExperimentalMode, + parseResourceMcpConstraints, +} from "../resource-scope"; import { type OAuthState, verifyAndParseState } from "../state"; /** @@ -264,13 +267,19 @@ export default new Hono<{ Bindings: Env }>().get("/", async (c) => { ); } + const resource = (oauthReqInfo as AuthRequestWithSkills).resource; + const experimentalMode = parseResourceExperimentalMode( + typeof resource === "string" ? resource : undefined, + ); + // Calculate Sentry API scopes from validated skills - const grantedScopes = await getScopesForSkills(validSkills); + const grantedScopes = await getScopesForSkills(validSkills, { + experimentalMode, + }); // Convert valid skills Set to array for OAuth props const grantedSkills = Array.from(validSkills); - const resource = (oauthReqInfo as AuthRequestWithSkills).resource; const resourceScope = parseResourceMcpConstraints( typeof resource === "string" ? resource : undefined, ); diff --git a/packages/mcp-core/src/server.test.ts b/packages/mcp-core/src/server.test.ts index 36e9ffe8..0187c0bd 100644 --- a/packages/mcp-core/src/server.test.ts +++ b/packages/mcp-core/src/server.test.ts @@ -3,6 +3,7 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { setUser } from "@sentry/core"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { buildServer } from "./server"; +import type { Skill } from "./skills"; import tools from "./tools"; import { CATALOG_INFRASTRUCTURE_TOOL_NAMES, @@ -440,6 +441,59 @@ describe("buildServer", () => { }); }); + describe("experimental skill collapsing", () => { + it("does not expose preprod tools through inspect in stable mode", () => { + const server = buildServer({ + context: { + ...baseContext, + grantedSkills: new Set(["inspect"]), + }, + experimentalMode: false, + tools: { + preprod_tool: createMockTool("preprod_tool", { + skills: ["preprod"], + }), + }, + }); + + expect(getRegisteredToolNames(server)).not.toContain("preprod_tool"); + }); + + it("exposes preprod tools through inspect in experimental mode", () => { + const server = buildServer({ + context: { + ...baseContext, + grantedSkills: new Set(["inspect"]), + }, + experimentalMode: true, + tools: { + preprod_tool: createMockTool("preprod_tool", { + skills: ["preprod"], + }), + }, + }); + + expect(getRegisteredToolNames(server)).toContain("preprod_tool"); + }); + + it("continues to expose preprod tools to explicit preprod grants", () => { + const server = buildServer({ + context: { + ...baseContext, + grantedSkills: new Set(["preprod"]), + }, + experimentalMode: false, + tools: { + preprod_tool: createMockTool("preprod_tool", { + skills: ["preprod"], + }), + }, + }); + + expect(getRegisteredToolNames(server)).toContain("preprod_tool"); + }); + }); + describe("hideInExperimentalMode filtering", () => { it("hides tools with hideInExperimentalMode when experimentalMode is true", () => { const server = buildServer({ @@ -684,57 +738,20 @@ describe("buildServer", () => { expect(registeredTools.map((tool) => tool.name)).toEqual(["use_sentry"]); }); - it("keeps preprod tools catalog-only while enforcing their skill gate", async () => { - const withoutPreprod = buildServer({ + it("keeps preprod tools catalog-only while exposing them through inspect in experimental mode", async () => { + const server = buildServer({ context: baseContext, experimentalMode: true, }); - const withoutPreprodToolNames = getRegisteredToolNames(withoutPreprod); - expect(withoutPreprodToolNames).not.toContain("get_snapshot"); - expect(withoutPreprodToolNames).not.toContain("get_snapshot_image"); - expect(withoutPreprodToolNames).not.toContain("get_latest_base_snapshot"); - - const hiddenResult = await callRegisteredTool( - withoutPreprod, - "search_tools", - { - query: "snapshot", - limit: 10, - }, - ); - const hiddenPayload = getStructuredContent<{ - results: Array<{ name: string }>; - }>(hiddenResult); - expect(hiddenPayload.results.map((tool) => tool.name)).not.toContain( - "get_snapshot", - ); + const topLevelToolNames = getRegisteredToolNames(server); + expect(topLevelToolNames).not.toContain("get_snapshot"); + expect(topLevelToolNames).not.toContain("get_snapshot_image"); + expect(topLevelToolNames).not.toContain("get_latest_base_snapshot"); - const withPreprod = buildServer({ - context: { - ...baseContext, - grantedSkills: new Set([ - "inspect", - "triage", - "project-management", - "seer", - "preprod", - ]), - }, - experimentalMode: true, + const visibleResult = await callRegisteredTool(server, "search_tools", { + query: "snapshot", + limit: 10, }); - const withPreprodToolNames = getRegisteredToolNames(withPreprod); - expect(withPreprodToolNames).not.toContain("get_snapshot"); - expect(withPreprodToolNames).not.toContain("get_snapshot_image"); - expect(withPreprodToolNames).not.toContain("get_latest_base_snapshot"); - - const visibleResult = await callRegisteredTool( - withPreprod, - "search_tools", - { - query: "snapshot", - limit: 10, - }, - ); const visiblePayload = getStructuredContent<{ results: Array<{ name: string }>; }>(visibleResult); diff --git a/packages/mcp-core/src/skillDefinitions.json b/packages/mcp-core/src/skillDefinitions.json index 657ca77e..54bf5dcd 100644 --- a/packages/mcp-core/src/skillDefinitions.json +++ b/packages/mcp-core/src/skillDefinitions.json @@ -310,6 +310,7 @@ "description": "Inspect visual regression snapshot tests from CI — view changed images and diff masks", "defaultEnabled": false, "order": 6, + "mergedIntoSkillInExperimentalMode": "inspect", "toolCount": 7, "tools": [ { diff --git a/packages/mcp-core/src/skillDefinitions.ts b/packages/mcp-core/src/skillDefinitions.ts index b1792f98..88aa17aa 100644 --- a/packages/mcp-core/src/skillDefinitions.ts +++ b/packages/mcp-core/src/skillDefinitions.ts @@ -8,6 +8,7 @@ export interface SkillDefinition { defaultEnabled: boolean; order: number; toolCount?: number; + mergedIntoSkillInExperimentalMode?: string; tools?: Array<{ name: string; description: string; diff --git a/packages/mcp-core/src/skills.test.ts b/packages/mcp-core/src/skills.test.ts index ad5cb313..3777dd36 100644 --- a/packages/mcp-core/src/skills.test.ts +++ b/packages/mcp-core/src/skills.test.ts @@ -5,6 +5,7 @@ import { isValidSkill, parseSkills, isEnabledBySkills, + getEffectiveToolSkills, type Skill, } from "./skills"; @@ -16,6 +17,7 @@ describe("skills module", () => { expect(SKILLS["project-management"]).toBeDefined(); expect(SKILLS.seer).toBeDefined(); expect(SKILLS.docs).toBeDefined(); + expect(SKILLS.preprod).toBeDefined(); }); it("includes metadata for each skill", () => { @@ -36,6 +38,7 @@ describe("skills module", () => { expect(DEFAULT_SKILLS).not.toContain("docs"); expect(DEFAULT_SKILLS).not.toContain("triage"); expect(DEFAULT_SKILLS).not.toContain("project-management"); + expect(DEFAULT_SKILLS).not.toContain("preprod"); }); it("has exactly 2 default skills", () => { @@ -50,6 +53,7 @@ describe("skills module", () => { expect(isValidSkill("project-management")).toBe(true); expect(isValidSkill("seer")).toBe(true); expect(isValidSkill("docs")).toBe(true); + expect(isValidSkill("preprod")).toBe(true); }); it("returns false for invalid skills", () => { @@ -139,5 +143,31 @@ describe("skills module", () => { const grantedSkills = new Set(["inspect"]); expect(isEnabledBySkills(grantedSkills, [])).toBe(false); }); + + it("does not collapse preprod into inspect outside experimental mode", () => { + const grantedSkills = new Set(["inspect"]); + expect(isEnabledBySkills(grantedSkills, ["preprod"])).toBe(false); + }); + + it("collapses preprod into inspect in experimental mode", () => { + const grantedSkills = new Set(["inspect"]); + expect( + isEnabledBySkills(grantedSkills, ["preprod"], { + experimentalMode: true, + }), + ).toBe(true); + }); + }); + + describe("getEffectiveToolSkills", () => { + it("preserves tool skills outside experimental mode", () => { + expect(getEffectiveToolSkills(["preprod"])).toEqual(["preprod"]); + }); + + it("adds merged target skills in experimental mode", () => { + expect( + getEffectiveToolSkills(["preprod"], { experimentalMode: true }), + ).toEqual(["preprod", "inspect"]); + }); }); }); diff --git a/packages/mcp-core/src/skills.ts b/packages/mcp-core/src/skills.ts index 66cd5b77..ecb0a84b 100644 --- a/packages/mcp-core/src/skills.ts +++ b/packages/mcp-core/src/skills.ts @@ -22,6 +22,12 @@ export interface SkillDefinition { defaultEnabled: boolean; order: number; toolCount?: number; // Number of tools enabled by this skill (calculated dynamically) + /** + * Experimental-mode consolidation target. When set, tools assigned to this + * skill are also exposed to sessions granted the target skill, and OAuth + * consent can hide the source skill to keep the visible permission set small. + */ + mergedIntoSkillInExperimentalMode?: Skill; } export const SKILLS: Record = { @@ -68,6 +74,7 @@ export const SKILLS: Record = { "Inspect visual regression snapshot tests from CI — view changed images and diff masks", defaultEnabled: false, order: 6, + mergedIntoSkillInExperimentalMode: "inspect", }, }; @@ -124,13 +131,40 @@ export function isValidSkill(skill: string): skill is Skill { return skill in SKILLS; } +export interface SkillModeOptions { + experimentalMode?: boolean; +} + +// Expand tool skills based on active server mode. +export function getEffectiveToolSkills( + toolSkills: Skill[], + options: SkillModeOptions = {}, +): Skill[] { + if (!options.experimentalMode) { + return toolSkills; + } + + const effectiveSkills = new Set(toolSkills); + for (const skill of toolSkills) { + const mergedSkill = SKILLS[skill]?.mergedIntoSkillInExperimentalMode; + if (mergedSkill) { + effectiveSkills.add(mergedSkill); + } + } + + return Array.from(effectiveSkills); +} + // Check if tool is enabled by granted skills (ANY match = enabled) export function isEnabledBySkills( grantedSkills: Set | undefined, toolSkills: Skill[], + options: SkillModeOptions = {}, ): boolean { if (!grantedSkills || toolSkills.length === 0) return false; - return toolSkills.some((skill) => grantedSkills.has(skill)); + return getEffectiveToolSkills(toolSkills, options).some((skill) => + grantedSkills.has(skill), + ); } // Parse and validate skills from input @@ -166,6 +200,7 @@ export function parseSkills(input: unknown): { // Calculate required scopes from granted skills export async function getScopesForSkills( grantedSkills: Set, + options: SkillModeOptions = {}, ): Promise> { // Import here to avoid circular dependency at module load time const { DEFAULT_SCOPES } = await import("./constants.js"); @@ -184,7 +219,9 @@ export async function getScopesForSkills( continue; } // Check if any of the tool's skills are granted - const toolEnabled = tool.skills.some((skill) => grantedSkills.has(skill)); + const toolEnabled = getEffectiveToolSkills(tool.skills, options).some( + (skill) => grantedSkills.has(skill), + ); // If tool is enabled by granted skills, add its required scopes if (toolEnabled) { diff --git a/packages/mcp-core/src/tools/catalog-runtime/availability.ts b/packages/mcp-core/src/tools/catalog-runtime/availability.ts index 9f00238d..cec4b872 100644 --- a/packages/mcp-core/src/tools/catalog-runtime/availability.ts +++ b/packages/mcp-core/src/tools/catalog-runtime/availability.ts @@ -88,10 +88,12 @@ function isAllowedBySkills({ tool, context, agentMode, + experimentalMode, }: { tool: ToolConfig; context: ServerContext; agentMode: boolean; + experimentalMode: boolean; }): boolean { if (agentMode) { return true; @@ -106,7 +108,8 @@ function isAllowedBySkills({ } return ( - tool.skills.length > 0 && isEnabledBySkills(grantedSkills, tool.skills) + tool.skills.length > 0 && + isEnabledBySkills(grantedSkills, tool.skills, { experimentalMode }) ); } @@ -176,7 +179,7 @@ export function getAvailableTools({ continue; } - if (!isAllowedBySkills({ tool, context, agentMode })) { + if (!isAllowedBySkills({ tool, context, agentMode, experimentalMode })) { continue; }