diff --git a/src/api/routes.ts b/src/api/routes.ts index 33025838bd..6c8b99b5cd 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -176,7 +176,7 @@ import { validateOrbRelayEnrollment, } from "../orb/relay"; import { computeFleetAnalytics } from "../orb/analytics"; -import { handleMcpRequest } from "../mcp/server"; +import { handleMcpRequest, isMcpAdminEnabled } from "../mcp/server"; import { simulateOpenPrPressureSchema } from "../mcp/server"; import { simulateOpenPrPressure, type OpenPrPressureInput } from "../services/open-pr-pressure-scenarios"; import { DISCOVERY_PATHS, discoveryDocumentsFor, respondWithDocument, toolsForDeployment } from "../mcp/discovery-routes"; @@ -644,11 +644,15 @@ export function createApp() { // app (src/server.ts serves this very Hono instance), so the deployment has to be read at request // time rather than assumed. const deployment = isSelfHostedReviewRuntime(c.env) ? "selfhost" : "cloud"; + // #10039: same request-time read `createServer()` uses to gate admin-tool REGISTRATION, so a + // self-host card never advertises the five admin tools when the flag that would register them is off. + const adminEnabled = isMcpAdminEnabled(c.env); const documents = discoveryDocumentsFor({ version: LATEST_RECOMMENDED_MCP_VERSION, deployment, + adminEnabled, baseUrl: c.env.PUBLIC_API_ORIGIN ?? new URL(c.req.url).origin, - tools: toolsForDeployment(deployment), + tools: toolsForDeployment(deployment, adminEnabled), }); return respondWithDocument(documents[path]!, c.req.header("if-none-match") ?? null); }); diff --git a/src/mcp/discovery-routes.ts b/src/mcp/discovery-routes.ts index 819c5de702..fe1ebef646 100644 --- a/src/mcp/discovery-routes.ts +++ b/src/mcp/discovery-routes.ts @@ -8,7 +8,8 @@ // mounts the SAME routes over its own availability-filtered tool list. That is what makes a self-hosted // card truthful rather than a copy of the cloud one — a `cloud`-only tool is absent from a self-host card // because it is absent from that deployment's list, not because a second implementation remembered to -// exclude it. +// exclude it. The same truthfulness applies to a tool that is available but not REGISTERED (#10039's +// admin category): a self-host card omits it too, because it is absent from what `/mcp` actually serves. import { buildAgentToolsIndex, buildAnthropicTools, @@ -26,19 +27,31 @@ export type DiscoveryContext = { deployment: DiscoveryDeployment; baseUrl: string; tools: readonly McpToolDefinition[]; + /** Whether THIS deployment currently registers the "admin" category (#10039's `isMcpAdminEnabled`). + * Carried on the context (rather than re-derived from `tools`) so it can also feed the memo key below. */ + adminEnabled: boolean; }; /** - * The tools a deployment truthfully serves: `both` plus its own kind. + * The tools a deployment truthfully serves: `both` plus its own kind, minus whatever it does not actually + * REGISTER (#10039). * * Locality is deliberately NOT filtered here. A `local-git` tool is still part of the catalog a client * discovers — the remote simply expects the caller to supply the branch metadata rather than reading a * checkout — so hiding it would under-describe the server. + * + * `availability` is not the only thing that decides whether a deployment serves a tool: "admin" is + * registered in `createServer()` only when `isMcpAdminEnabled(env)` is true, so a card built without regard + * to that flag would advertise five tools `/mcp` refuses as unknown on a default self-host deployment. This + * mirrors that same registration condition rather than re-deriving a category allowlist, so a second + * conditionally-registered category needs only a change here, not a new hardcoded filter at each caller. */ -export function toolsForDeployment(deployment: DiscoveryDeployment): McpToolDefinition[] { +export function toolsForDeployment(deployment: DiscoveryDeployment, adminEnabled: boolean): McpToolDefinition[] { // `both` is not listed: the registry's filter treats it as the ABSENCE of a restriction, so it already // satisfies either constraint. Naming it here would read as if it were a third deployment. - return listToolDefinitions({ availability: [deployment] }); + const tools = listToolDefinitions({ availability: [deployment] }); + if (adminEnabled) return tools; + return tools.filter((tool) => tool.category !== "admin"); } /** @@ -100,7 +113,10 @@ export function respondWithDocument(document: DiscoveryDocument, ifNoneMatch: st const DOCUMENT_CACHE = new Map>(); export function discoveryDocumentsFor(context: DiscoveryContext): Record { - const key = `${context.deployment}|${context.version}|${context.baseUrl}`; + // `adminEnabled` rides along: without it, the first request on an isolate would pick a tool list and the + // memo would keep serving it to every later request on the same (deployment, version, baseUrl), even one + // that arrives after the flag flips. + const key = `${context.deployment}|${context.version}|${context.baseUrl}|${context.adminEnabled}`; let documents = DOCUMENT_CACHE.get(key); if (!documents) { documents = buildDiscoveryDocuments(context); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index d279e6854a..a0a60a1aa7 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -785,8 +785,10 @@ void _INTERNAL_JOB_MESSAGE_TYPES_ARE_REAL; /** Master opt-in for the "admin" tool category (#7721), default OFF. Same truthy-string convention as every * other LOOPOVER_* flag in this repo. Gates tool REGISTRATION in createServer() below; each admin tool - * handler additionally requires actor === "mcp-admin" at call time regardless of this flag. */ -function isMcpAdminEnabled(env: Env): boolean { + * handler additionally requires actor === "mcp-admin" at call time regardless of this flag. Exported so the + * `.well-known` discovery routes (#10039) can mirror this exact registration gate instead of growing a + * second copy of the truthy-string regex. */ +export function isMcpAdminEnabled(env: Env): boolean { return /^(1|true|yes|on)$/i.test((env.LOOPOVER_MCP_ADMIN_ENABLED ?? "").trim()); } diff --git a/test/integration/mcp-discovery-routes.test.ts b/test/integration/mcp-discovery-routes.test.ts index 4f98cfc224..88ce85c956 100644 --- a/test/integration/mcp-discovery-routes.test.ts +++ b/test/integration/mcp-discovery-routes.test.ts @@ -122,3 +122,85 @@ describe("discovery routes (#9526)", () => { expect(card.remotes[0]!.url).toBe("https://api.loopover.ai/mcp"); }); }); + +// #10039: a self-host card must describe only what THIS deployment's /mcp actually registers. "admin" is +// the one category gated behind LOOPOVER_MCP_ADMIN_ENABLED (createServer's isMcpAdminEnabled) rather than +// availability alone, so a card built from availability filtering only would advertise five tools the +// server refuses as unknown on a default (flag-unset) self-host deployment. +describe("admin-tool exclusion from a self-host card when the admin surface is not enabled (#10039)", () => { + const ADMIN_TOOL_NAMES = [ + "loopover_admin_get_config", + "loopover_admin_write_config", + "loopover_admin_list_config_backups", + "loopover_admin_trigger_redeploy", + "loopover_admin_rotate_secret", + ]; + + it("lists none of the five admin tools when LOOPOVER_MCP_ADMIN_ENABLED is unset on a self-host env", async () => { + const env = createTestEnv(); + expect(env.SELFHOST_TRANSIENT_CACHE, "the test env is the self-host runtime").toBeTruthy(); + expect(env.LOOPOVER_MCP_ADMIN_ENABLED, "the flag defaults off").toBeFalsy(); + + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const index = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/agent-tools/index.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const cardNames = card.tools.map((tool) => tool.name); + const indexNames = index.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(cardNames, `${name} must not be on the flag-off self-host card`).not.toContain(name); + expect(indexNames, `${name} must not be on the flag-off self-host index`).not.toContain(name); + } + }); + + it("lists all five admin tools when LOOPOVER_MCP_ADMIN_ENABLED=1 on the same self-host env", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" }); + expect(env.SELFHOST_TRANSIENT_CACHE, "the test env is the self-host runtime").toBeTruthy(); + + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const index = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/agent-tools/index.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const cardNames = card.tools.map((tool) => tool.name); + const indexNames = index.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(cardNames, `${name} must be on the flag-on self-host card`).toContain(name); + expect(indexNames, `${name} must be on the flag-on self-host index`).toContain(name); + } + }); + + it("the flag-on and flag-off documents do not leak through the memo, with no reset needed", async () => { + // Same module instance, same (deployment, version, baseUrl) -- only the flag differs. Deliberately does + // NOT call resetDiscoveryCacheForTesting between the two requests: that would mask a memo key that + // forgot to carry the flag, since a fresh cache always misses regardless. + const off = await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), createTestEnv()); + const on = await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" })); + + const offBody = await off.text(); + const onBody = await on.text(); + expect(onBody).not.toBe(offBody); + expect(on.headers.get("etag")).not.toBe(off.headers.get("etag")); + + const offNames = (JSON.parse(offBody) as { tools: Array<{ name: string }> }).tools.map((tool) => tool.name); + const onNames = (JSON.parse(onBody) as { tools: Array<{ name: string }> }).tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(offNames).not.toContain(name); + expect(onNames).toContain(name); + } + }); + + it("does not affect the cloud deployment's documents, which never listed the selfhost-only admin tools", async () => { + const cloudEnv = { ...createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" }), SELFHOST_TRANSIENT_CACHE: undefined } as unknown as Env; + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), cloudEnv)).json()) as { + deployment: string; + tools: Array<{ name: string }>; + }; + expect(card.deployment).toBe("cloud"); + const names = card.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) expect(names).not.toContain(name); + }); +}); diff --git a/test/unit/mcp-discovery-surfaces.test.ts b/test/unit/mcp-discovery-surfaces.test.ts index b31555086c..d4decbff26 100644 --- a/test/unit/mcp-discovery-surfaces.test.ts +++ b/test/unit/mcp-discovery-surfaces.test.ts @@ -30,7 +30,7 @@ import { buildOpenApiSpec } from "../../src/openapi/spec"; // that is answering, it is stable enough to cache, and it never under- or over-states the tool set. const TOOLS = listToolDefinitions({ availability: ["cloud"] }); -const CONTEXT = { version: "3.15.2", deployment: "cloud" as const, baseUrl: "https://api.loopover.ai", tools: TOOLS }; +const CONTEXT = { version: "3.15.2", deployment: "cloud" as const, baseUrl: "https://api.loopover.ai", tools: TOOLS, adminEnabled: false }; beforeEach(() => { resetDiscoveryCacheForTesting(); @@ -38,8 +38,8 @@ beforeEach(() => { describe("availability filtering (#9526)", () => { it("a cloud card excludes selfhost-only tools, and a selfhost card excludes cloud-only ones", () => { - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); - const selfhost = new Set(toolsForDeployment("selfhost").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); + const selfhost = new Set(toolsForDeployment("selfhost", false).map((tool) => tool.name)); // The registry's availability filter is INCLUSIVE (`both` satisfies any constraint), so "only" has to // be derived from the raw field rather than by filtering. @@ -54,8 +54,8 @@ describe("availability filtering (#9526)", () => { it("both deployments carry every `both` tool", () => { const shared = listToolDefinitions().filter((tool) => tool.availability === "both").map((tool) => tool.name); - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); - const selfhost = new Set(toolsForDeployment("selfhost").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); + const selfhost = new Set(toolsForDeployment("selfhost", false).map((tool) => tool.name)); for (const name of shared) { expect(cloud.has(name)).toBe(true); expect(selfhost.has(name)).toBe(true); @@ -67,7 +67,7 @@ describe("availability filtering (#9526)", () => { // reading a checkout. Hiding them would under-describe the server. const localGit = listToolDefinitions({ locality: ["local-git"] }).filter((tool) => tool.availability === "both").map((tool) => tool.name); expect(localGit.length).toBeGreaterThan(0); - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); for (const name of localGit) expect(cloud.has(name)).toBe(true); }); }); @@ -215,7 +215,7 @@ describe("the per-origin memo (#9526)", () => { it("keeps deployments separate, so a self-host card is not a copy of the cloud one", () => { const cloud = JSON.parse(discoveryDocumentsFor(CONTEXT)["/.well-known/mcp.json"]!.body); const selfhost = JSON.parse( - discoveryDocumentsFor({ ...CONTEXT, deployment: "selfhost", tools: toolsForDeployment("selfhost") })["/.well-known/mcp.json"]!.body, + discoveryDocumentsFor({ ...CONTEXT, deployment: "selfhost", tools: toolsForDeployment("selfhost", false) })["/.well-known/mcp.json"]!.body, ); expect(selfhost.deployment).toBe("selfhost"); expect(selfhost.tools.length).not.toBe(cloud.tools.length);