From 65b9c9a4721d3eafee2568c440dc6df7280b3bd5 Mon Sep 17 00:00:00 2001 From: Eshwar Sundar Date: Thu, 20 Aug 2026 11:41:21 +0530 Subject: [PATCH] feat(vnext): port capability policy and Cursor HITL updates --- shared/glean/mcp/build.mjs | 87 ++-- shared/glean/mcp/src/atomic-write.ts | 52 +++ shared/glean/mcp/src/index.ts | 121 +++-- shared/glean/mcp/src/policy/cache.ts | 83 ++++ shared/glean/mcp/src/policy/context.ts | 78 ++++ shared/glean/mcp/src/policy/enforce.ts | 157 +++++++ shared/glean/mcp/src/policy/evaluate.ts | 168 +++++++ shared/glean/mcp/src/policy/key.ts | 27 ++ shared/glean/mcp/src/policy/negotiate.ts | 158 +++++++ .../glean/mcp/src/policy/protocol-version.ts | 105 +++++ shared/glean/mcp/src/policy/session.ts | 319 +++++++++++++ shared/glean/mcp/src/policy/types.ts | 147 ++++++ shared/glean/mcp/src/remote-client.ts | 19 +- .../glean/mcp/src/remote-tools-cache-store.ts | 26 +- .../glean/mcp/src/tools/remote-passthrough.ts | 13 +- shared/glean/mcp/src/tools/run-tool.ts | 108 +++-- shared/glean/mcp/src/version.ts | 39 +- shared/glean/mcp/tests/atomic-write.test.ts | 81 ++++ shared/glean/mcp/tests/find-skills.test.ts | 12 +- shared/glean/mcp/tests/policy-enforce.test.ts | 320 +++++++++++++ shared/glean/mcp/tests/policy-session.test.ts | 439 ++++++++++++++++++ shared/glean/mcp/tests/policy.test.ts | 386 +++++++++++++++ shared/glean/mcp/tests/remote-client.test.ts | 5 +- shared/glean/mcp/tests/run-tool.test.ts | 361 ++++++++++++-- shared/glean/mcp/tests/version.test.ts | 13 +- 25 files changed, 3136 insertions(+), 188 deletions(-) create mode 100644 shared/glean/mcp/src/atomic-write.ts create mode 100644 shared/glean/mcp/src/policy/cache.ts create mode 100644 shared/glean/mcp/src/policy/context.ts create mode 100644 shared/glean/mcp/src/policy/enforce.ts create mode 100644 shared/glean/mcp/src/policy/evaluate.ts create mode 100644 shared/glean/mcp/src/policy/key.ts create mode 100644 shared/glean/mcp/src/policy/negotiate.ts create mode 100644 shared/glean/mcp/src/policy/protocol-version.ts create mode 100644 shared/glean/mcp/src/policy/session.ts create mode 100644 shared/glean/mcp/src/policy/types.ts create mode 100644 shared/glean/mcp/tests/atomic-write.test.ts create mode 100644 shared/glean/mcp/tests/policy-enforce.test.ts create mode 100644 shared/glean/mcp/tests/policy-session.test.ts create mode 100644 shared/glean/mcp/tests/policy.test.ts diff --git a/shared/glean/mcp/build.mjs b/shared/glean/mcp/build.mjs index e3c4a9e..856444d 100644 --- a/shared/glean/mcp/build.mjs +++ b/shared/glean/mcp/build.mjs @@ -2,54 +2,89 @@ // // Why bundle: Cowork's plugin-install validator rejects zip entries whose // paths contain `@`, which appears in every scoped npm package's directory -// name (`node_modules/@modelcontextprotocol/...`). Inlining every dep into -// one `dist/index.js` means the shipped tree has no scoped-package paths. -// -// Bundle shape: -// - platform=node, format=esm so Node can load it with `node dist/index.js` -// and no `--experimental-*` flags, matching our package.json type:module -// - bundle=true with packages='bundled' so every import except Node -// builtins gets inlined -// - external: the `node:*` builtins (explicit for clarity; esbuild on -// platform=node treats bare `node:*` as external by default but we pin -// it so this doesn't regress silently) -// - no sourcemap or minification — the bundle is checked into git and -// should stay readable for debugging +// name (`node_modules/@modelcontextprotocol/...`). Inlining every dependency +// into one dist/index.js means the shipped tree has no scoped-package paths. import { build } from "esbuild"; import { builtinModules } from "node:module"; +import { readFileSync } from "node:fs"; const nodeBuiltins = [ ...builtinModules, ...builtinModules.map((m) => `node:${m}`), ]; +const VERSION_FILES = [ + "package.json", + "shared/glean/mcp/package.json", +]; +const SEMVER_RE = /^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$/; + +function pluginVersionFromPackages() { + const found = VERSION_FILES.map((file) => { + let raw; + try { + raw = JSON.parse(readFileSync(file, "utf-8")); + } catch (err) { + throw new Error(`build: cannot read ${file}: ${err.message}`); + } + const version = raw.version; + if (typeof version !== "string" || !SEMVER_RE.test(version)) { + throw new Error( + `build: ${file} must declare a plain x.y.z version, got ${JSON.stringify(version)}`, + ); + } + return { file, version }; + }); + + const versions = [...new Set(found.map((entry) => entry.version))]; + if (versions.length !== 1) { + throw new Error( + `build: package versions disagree, so there is no single version to bake in:\n` + + found.map((entry) => ` ${entry.version} ${entry.file}`).join("\n"), + ); + } + return versions[0]; +} + +const pluginVersion = pluginVersionFromPackages(); +const OUTFILE = "shared/glean/mcp/dist/index.js"; + await build({ entryPoints: ["shared/glean/mcp/src/index.ts"], - outfile: "shared/glean/mcp/dist/index.js", + outfile: OUTFILE, bundle: true, platform: "node", format: "esm", target: "node20", - // Not setting `packages` — esbuild only accepts `"external"` here, which - // would ship every dep as a runtime lookup (defeating the purpose). The - // default when `bundle:true` is to inline every import whose specifier - // isn't in `external`, which is exactly what we want. + define: { + __GLEAN_PLUGIN_VERSION__: JSON.stringify(pluginVersion), + }, + // Not setting `packages` — the default with bundle:true inlines every + // non-external import, which is exactly what the shipped single-file server + // needs. external: nodeBuiltins, - // Some transitive deps (e.g. `yaml`) ship CJS that does `require("node:*")` - // at module-eval time. esbuild inlines that CJS under an ESM shim that - // does NOT provide a `require`, so imports blow up with "Dynamic require - // of X is not supported". Prepending a `createRequire`-based shim gives - // the inlined CJS a working `require` for Node builtins. + // Some transitive deps ship CJS that requires node:* at module-eval time. + // Provide a working require shim inside the ESM bundle for those builtins. banner: { js: `import { createRequire as __glean_createRequire } from "node:module";\nconst require = __glean_createRequire(import.meta.url);`, }, minify: false, legalComments: "linked", logLevel: "info", - // The SDK and some transitive deps still ship CJS under their "require" - // export condition. We're emitting ESM and asking esbuild to resolve - // through each package's "import" condition first. conditions: ["import", "node", "default"], mainFields: ["module", "main"], }); + +const bundled = readFileSync(OUTFILE, "utf-8"); +if (bundled.includes("__GLEAN_PLUGIN_VERSION__")) { + throw new Error( + `build: ${OUTFILE} still contains __GLEAN_PLUGIN_VERSION__; esbuild did not substitute it`, + ); +} +if (!bundled.includes(JSON.stringify(pluginVersion))) { + throw new Error( + `build: ${OUTFILE} does not contain the version literal ${JSON.stringify(pluginVersion)}`, + ); +} +console.log(`Baked plugin version ${pluginVersion} into ${OUTFILE}`); diff --git a/shared/glean/mcp/src/atomic-write.ts b/shared/glean/mcp/src/atomic-write.ts new file mode 100644 index 0000000..3d9bad2 --- /dev/null +++ b/shared/glean/mcp/src/atomic-write.ts @@ -0,0 +1,52 @@ +import fs from "node:fs"; +import path from "node:path"; + +/** + * Write a file so no reader can observe a partial one. + * + * `fs.writeFileSync` truncates and then writes, so a process killed mid-write leaves a + * truncated file behind. Every store here parses JSON and treats a parse failure as "no + * data", so a torn write does not surface as an error — it silently discards whatever was + * stored. For the policy cache that is the one outcome we have gone out of our way to + * prevent: nothing is allowed to clear it, precisely because a cached policy may carry a + * deactivation or a version block, and a crash mid-write would clear it anyway. + * + * Writing to a sibling temp file and renaming makes the swap atomic — a reader sees either + * the old contents or the new ones, never a mixture. The temp file must live in the same + * directory, since rename is only atomic within a filesystem, and it carries the pid + * because each host session runs its own plugin process and they share these files. + * + * This is NOT mutual exclusion. Two processes doing read-modify-write can still lose one + * update, last writer winning; they simply cannot corrupt the file. A lost update + * self-heals on the next negotiation, whereas a corrupt file discards every entry for + * every URL until something rewrites it. + * + * On failure the temp file is removed and the error rethrown, leaving whatever was + * already on disk intact. Windows can fail the rename with EPERM/EBUSY when another + * process holds the target open; callers already tolerate a failed write, so that + * degrades to "this write was skipped" rather than to corruption. + */ +export function writeFileAtomicSync( + filePath: string, + data: string, + mode: number, +): void { + const tmpPath = path.join( + path.dirname(filePath), + `.${path.basename(filePath)}.${process.pid}.tmp`, + ); + try { + fs.writeFileSync(tmpPath, data, { encoding: "utf-8", mode }); + // writeFileSync only applies `mode` when it creates the file, so a leftover temp + // from a previous crash could otherwise keep looser permissions. + fs.chmodSync(tmpPath, mode); + fs.renameSync(tmpPath, filePath); + } catch (err) { + try { + fs.rmSync(tmpPath, { force: true }); + } catch { + // Nothing further to do; the target is untouched either way. + } + throw err; + } +} diff --git a/shared/glean/mcp/src/index.ts b/shared/glean/mcp/src/index.ts index e524b7f..ec88b3f 100644 --- a/shared/glean/mcp/src/index.ts +++ b/shared/glean/mcp/src/index.ts @@ -20,11 +20,7 @@ import { closeCallbackServer, } from "./auth-callback-server.js"; import { handleFindSkills } from "./tools/find-skills.js"; -import { - handleRunTool, - isCursorClient, - runToolAnnotations, -} from "./tools/run-tool.js"; +import { handleRunTool, runToolAnnotations } from "./tools/run-tool.js"; import { evictStaleSkills } from "./skill-writer.js"; import { loadServerUrl, @@ -45,7 +41,15 @@ import { } from "./tools/remote-passthrough.js"; import { resolveSessionId } from "./session-id.js"; import { resolveServerUrlFromEmail } from "./config-search.js"; -import { PLUGIN_VERSION } from "./version.js"; +import { pluginVersionString } from "./version.js"; +import { + decisionInForce, + initPolicySession, + policySummary, + protocolVersion, + setPolicyServerUrl, +} from "./policy/session.js"; +import { advertisedTools, policyRefusal } from "./policy/enforce.js"; function readEnv(...keys: string[]): string | undefined { for (const key of keys) { @@ -127,10 +131,15 @@ function resolveSkillsBaseDir(): string { } const server = new Server( - { name: "glean", version: PLUGIN_VERSION }, + { name: "glean", version: pluginVersionString() }, { capabilities: { tools: { listChanged: true } } }, ); +// Report the negotiated host/plugin context to the remote and enforce the +// capability policy returned for this Glean instance. +initPolicySession(server, logLine); +setPolicyServerUrl(resolveServerUrl()); + let oauthProvider: GleanOAuthClientProvider | undefined; // Cache of the last successful remote tools/list fetch. Persists for the @@ -293,31 +302,37 @@ const SETUP_TOOL: Tool = { }; server.setRequestHandler(ListToolsRequestSchema, async () => { - const runTool: Tool = { - ...RUN_TOOL_TOOL, - annotations: runToolAnnotations( - process.env.ENABLE_HITL === "true", - !!server.getClientCapabilities()?.elicitation, - isCursorClient(server), - ), - }; - const staticTools: Tool[] = [FIND_SKILLS_TOOL, runTool, SETUP_TOOL]; - - // One structured line on every return path, so "why don't my tools appear?" - // is answerable from the log alone: `static` is constant, `names` lists the - // dynamic tools we actually surfaced (freshly fetched or served from cache), - // and `state` names the path we took. The allow-list only ever drops tools - // outside our fixed set, so a missing allow-listed name (e.g. `chat`) means - // the backend never returned it. Only tool *names*, counts and the state - // tag are logged — never argument values, which can carry PII/secrets. + // Read the policy after any remote fetch: fetchAllowedRemoteTools records a + // policy returned by tools/list, so reading it earlier would be one request + // stale against the catalog we are about to advertise. const serve = (state: string, dynamic: Tool[]): { tools: Tool[] } => { + const decision = decisionInForce(); + const runTool: Tool = { + ...RUN_TOOL_TOOL, + annotations: runToolAnnotations( + process.env.ENABLE_HITL === "true", + !!server.getClientCapabilities()?.elicitation, + ), + }; + const { tools, withheld } = advertisedTools({ + decision, + setupTool: SETUP_TOOL, + findSkillsTool: FIND_SKILLS_TOOL, + runTool, + promoted: dynamic, + }); + const fromCatalog = new Set(dynamic.map((tool) => tool.name)); logLine("tools-list.served", { - static: staticTools.length, - dynamic: dynamic.length, - names: dynamic.map((t) => t.name), + static: tools.filter((tool) => !fromCatalog.has(tool.name)).length, + dynamic: tools.filter((tool) => fromCatalog.has(tool.name)).length, + names: dynamic.map((tool) => tool.name), + withheld, + deactivated: decision.deactivated, + versionState: decision.versionState, + features: decision.features, state, }); - return { tools: [...staticTools, ...dynamic] }; + return { tools }; }; // Pre-auth gate: tokens() is sync. When unauthenticated (or unconfigured) @@ -531,6 +546,20 @@ async function advanceSetup(): Promise { cachedRemoteTools = remoteTools; saveRemoteTools(serverUrl, remoteTools); const toolNames = remoteTools.map((t) => t.name).join(", ") || "(none)"; + const decision = decisionInForce(); + const closing = decision.deactivated + ? `This plugin version is not supported by your Glean instance, so only ` + + `\`setup\` is available. Upgrade the Glean plugin to restore the rest.` + : `You can now use ` + + [ + ...(decision.features.metaTools + ? ["find_skills_and_tools", "run_tool"] + : []), + ...(decision.features.toolPromotion && remoteTools.length > 0 + ? ["any of the listed remote tools"] + : []), + ].join(", ") + + `.`; return { content: [ { @@ -539,9 +568,9 @@ async function advanceSetup(): Promise { `Glean setup is complete.\n` + `Server URL: ${serverUrl}\n` + `Authenticated: yes\n` + - `Remote tools: ${toolNames}\n\n` + - `You can now use find_skills_and_tools, run_tool, and any of the listed ` + - `remote tools.`, + `Remote tools: ${toolNames}\n` + + `${policySummary().join("\n")}\n\n` + + closing, }, ], }; @@ -569,6 +598,25 @@ async function advanceSetup(): Promise { server.setRequestHandler(CallToolRequestSchema, async (request) => { const { name, arguments: args = {} } = request.params; + // Advertisement is advisory: a host may retain a stale tool list, so every + // policy withdrawal is also enforced at call time. Setup remains the + // recovery path and is always exempt from refusal. + const decision = decisionInForce(); + const refusal = policyRefusal({ + name, + decision, + promoted: REMOTE_TOOLS_ALLOWLIST, + }); + if (refusal) { + logLine("policy.refused", { + tool: name, + deactivated: decision.deactivated, + versionState: decision.versionState, + features: decision.features, + }); + return refusal; + } + // Allow-listed remote tools (chat/search/read_document) — only valid once // setup has provided a server URL. Auth is handled by dispatchRemoteTool // via the standard [AUTHENTICATION_REQUIRED] flow. @@ -712,7 +760,9 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { } try { const skillsBaseDir = resolveSkillsBaseDir(); - return await handleRunTool(remoteClient, server, skillsBaseDir, args); + return await handleRunTool(remoteClient, server, skillsBaseDir, args, { + fileArgs: decision.features.fileArgs, + }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); console.error(`run_tool: execution failed: ${msg}`); @@ -736,6 +786,9 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { clearRemoteTools(); oauthProvider = undefined; cachedRemoteTools = []; + // Policy survives a user reset: only a new valid remote policy may + // replace a cached deactivation or feature restriction. + setPolicyServerUrl(undefined); logLine("setup.reset"); // Fire-and-forget — tools list is shorter without the dynamic // surface; the host should re-fetch on its next idle cycle. @@ -815,6 +868,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { clearCredentials(); oauthProvider = undefined; cachedRemoteTools = loadRemoteTools(normalized); + setPolicyServerUrl(normalized); logLine("setup.configured", { serverUrl: normalized }); // Fall through to advanceSetup, which will now find URL ✓ and try // to drive auth + tool fetch in the same call. @@ -841,7 +895,8 @@ async function main() { logLine("evict-stale-skills.failed", { msg }); } - const transport = new StdioServerTransport(); + // Observe the negotiated MCP protocol revision from the initialize response. + const transport = protocolVersion.wrap(new StdioServerTransport()); await server.connect(transport); } diff --git a/shared/glean/mcp/src/policy/cache.ts b/shared/glean/mcp/src/policy/cache.ts new file mode 100644 index 0000000..0cc8d0e --- /dev/null +++ b/shared/glean/mcp/src/policy/cache.ts @@ -0,0 +1,83 @@ +import fs from "node:fs"; +import path from "node:path"; +import os from "node:os"; +import { writeFileAtomicSync } from "../atomic-write.js"; +import type { PolicyResponse } from "./types.js"; + +// The last VALID policy, keyed by remote URL so switching instances does not +// cross-contaminate. It survives a response that carried no policy and survives a +// malformed one; only a valid policy replaces it. +// +// tools/list is deliberately NOT cached here, even though the design pairs a cached +// policy with a cached surface for the unreachable case. remote-tools-cache-store.ts +// already owns that: it is typed as Tool[] rather than unknown, and is wired into the +// startup, setup, and instance-switch paths. A second copy here would shadow a live +// subsystem, and the two could then disagree about which surface belongs with which +// policy. Anything replaying a cached surface reads it from there. +interface PolicyCacheEntry { + policy?: PolicyResponse; + updatedAt?: string; +} + +type PolicyCacheFile = Record; + +// Same anchor as url-config-store and token-store: PLUGIN_DATA_DIR when the host +// provides a managed data directory, else ~/.glean. +function cachePath(): string { + const base = process.env.PLUGIN_DATA_DIR || path.join(os.homedir(), ".glean"); + return path.join(base, "policy-cache.json"); +} + +function readAll(): PolicyCacheFile { + try { + const parsed = JSON.parse(fs.readFileSync(cachePath(), "utf-8")); + return typeof parsed === "object" && parsed !== null ? parsed : {}; + } catch { + return {}; + } +} + +function writeAll(data: PolicyCacheFile): void { + const file = cachePath(); + try { + fs.mkdirSync(path.dirname(file), { recursive: true, mode: 0o700 }); + writeFileAtomicSync(file, JSON.stringify(data, null, 2), 0o600); + } catch { + // A cache that cannot be written degrades to in-session-only behavior, which + // is a worse experience but never a wrong policy decision. + } +} + +/** + * The cached policy for a remote, or undefined if none has been stored. + * + * Returns the policy itself rather than the stored entry: `updatedAt` is written for + * anyone reading the file by hand and is deliberately not part of the API, so callers + * cannot come to depend on a timestamp that says nothing about the policy's validity. + */ +export function loadCachedPolicy(serverUrl: string): PolicyResponse | undefined { + return readAll()[serverUrl]?.policy; +} + +/** + * Replace the cached policy. Only ever called with a VALIDATED policy. + * + * Reads every entry to write one, because the file is a map over remote URLs and the + * other URLs' entries have to survive the write. + */ +export function savePolicy(serverUrl: string, policy: PolicyResponse): void { + const all = readAll(); + all[serverUrl] = { policy, updatedAt: new Date().toISOString() }; + writeAll(all); +} + +// There is deliberately no clear function. Only a valid policy replaces a cached +// policy, and nothing removes one — including `setup({reset})`, which clears the +// server URL, credentials, and tools cache. A cached policy may carry a deactivation +// or a version block, so any local way to discard it is a way to shed one, and the +// remote may be unreachable afterwards with nothing to re-fetch from. The design makes +// the same point about process starts: a fresh plugin with no connection is not a +// licence to ignore a cached policy, or every restart would silently undo it. +// +// Switching instances needs no clear either: entries are keyed by remote URL, so a +// different instance simply reads a different (absent) entry. diff --git a/shared/glean/mcp/src/policy/context.ts b/shared/glean/mcp/src/policy/context.ts new file mode 100644 index 0000000..177b962 --- /dev/null +++ b/shared/glean/mcp/src/policy/context.ts @@ -0,0 +1,78 @@ +import { FEATURE_NAMES, type FeatureName } from "./key.js"; +import { pluginVersion } from "../version.js"; +import type { + ConfiguredServers, + HostIdentity, + NegotiationRequest, +} from "./types.js"; + +/** + * Host identity from the MCP `initialize` handshake -- clientInfo, declared + * capabilities, and the negotiated protocol revision. + * + * This is the reliable path and needs no host-specific code: no host is known to + * expose its own version through an environment variable, and no hook payload carries + * one either, so the handshake is the only place this information exists. + */ +export function hostIdentityFromHandshake( + clientInfo: { name?: string; version?: string } | undefined, + capabilities: Record | undefined, + mcpProtocolVersion?: string, +): HostIdentity { + if (!clientInfo?.name) { + // Report the revision even when clientInfo is unusable: it is the field most + // worth having when diagnosing a host we cannot otherwise identify. + return { id: "unknown", mcpProtocolVersion, source: "unknown" }; + } + return { + id: clientInfo.name, + version: clientInfo.version, + mcpProtocolVersion, + capabilities, + source: "handshake", + }; +} + +/** + * The configured-MCP-server inventory. Not reported yet. + * + * Reconstructing it from host configuration files was evaluated and rejected: it means + * reimplementing host merge semantics -- multiple config scopes, enablement and + * approval state, plugin installation state, two different `.mcp.json` schemas, and + * enterprise managed settings -- and every failure in that reimplementation is silent, + * producing a plausible list with wrong contents rather than an error. + * + * The accurate source is the host's own CLI (`claude mcp list`, `codex mcp list + * --json`), which is deferred because it cannot be called from this path: those + * commands health-check every server by spawning it, including this one, so invoking + * them during `tools/list` would make the plugin recursively launch itself. It needs a + * SessionStart hook that runs once per session and caches the result. + * + * Until then the field reports `unavailable`, which by contract says nothing about the + * user's setup rather than implying an empty list. + */ +export function inventory(): ConfiguredServers { + return { source: "unavailable" }; +} + +/** The features this build implements. Static: it changes only when a release does. */ +export function supportedFeatures(): Record { + return Object.fromEntries(FEATURE_NAMES.map((f) => [f, true])) as Record< + FeatureName, + boolean + >; +} + +export function buildNegotiationRequest(host: HostIdentity): NegotiationRequest { + const { version, source } = pluginVersion(); + return { + plugin: { + id: "glean", + version, + versionSource: source, + supportedFeatures: supportedFeatures(), + }, + host, + configuredServers: inventory(), + }; +} diff --git a/shared/glean/mcp/src/policy/enforce.ts b/shared/glean/mcp/src/policy/enforce.ts new file mode 100644 index 0000000..4d314a2 --- /dev/null +++ b/shared/glean/mcp/src/policy/enforce.ts @@ -0,0 +1,157 @@ +import type { CallToolResult, Tool } from "@modelcontextprotocol/sdk/types.js"; +import type { Decision } from "./types.js"; + +// The tool that policy can never withdraw. A deactivated plugin advertises only this, +// because it is how a user restores the connection that would lift the deactivation. +export const SETUP_TOOL_NAME = "setup"; + +// Gated together by `metaTools`, and gated as a pair on purpose: a surface with +// find_skills_and_tools but no run_tool can discover work it cannot perform. +export const META_TOOL_NAMES: ReadonlySet = new Set([ + "find_skills_and_tools", + "run_tool", +]); + +/** + * `run_tool` with `file_args` removed from its advertised schema, so a disabled + * fileArgs feature is genuinely inert rather than present-and-rejected. + * + * Returns a NEW tool at every level it changes. The caller clones RUN_TOOL_TOOL with a + * shallow spread, so `inputSchema.properties` is shared with that module-level const -- + * deleting from it would drop `file_args` for the life of the process and survive a + * later policy flip back to enabled. + */ +export function withoutFileArgs(tool: Tool): Tool { + const schema = tool.inputSchema as { properties?: Record }; + if (!schema?.properties || !("file_args" in schema.properties)) return tool; + const { file_args: _dropped, ...rest } = schema.properties; + return { + ...tool, + inputSchema: { ...schema, properties: rest } as Tool["inputSchema"], + }; +} + +export interface Advertisement { + tools: Tool[]; + /** Names policy withheld, for the served log line. */ + withheld: string[]; +} + +/** + * The tool surface policy allows, composed from the decision and the tools on offer. + * + * Order is preserved from the pre-policy implementation so that the no-policy case -- + * every install today -- produces a byte-identical list. + */ +export function advertisedTools(input: { + decision: Decision; + setupTool: Tool; + findSkillsTool: Tool; + runTool: Tool; + promoted: Tool[]; +}): Advertisement { + const { decision, setupTool, findSkillsTool, runTool, promoted } = input; + + if (decision.deactivated) { + return { + tools: [setupTool], + withheld: [ + findSkillsTool.name, + runTool.name, + ...promoted.map((t) => t.name), + ], + }; + } + + const tools: Tool[] = []; + const withheld: string[] = []; + + if (decision.features.metaTools) { + tools.push( + findSkillsTool, + decision.features.fileArgs ? runTool : withoutFileArgs(runTool), + ); + } else { + withheld.push(findSkillsTool.name, runTool.name); + } + + tools.push(setupTool); + + if (decision.features.toolPromotion) { + tools.push(...promoted); + } else { + withheld.push(...promoted.map((t) => t.name)); + } + + return { tools, withheld }; +} + +function refuse(text: string): CallToolResult { + return { content: [{ type: "text", text }], isError: true }; +} + +/** + * The refusal policy requires for a call, or undefined when the call may proceed. + * + * Advertisement is not the gate; this is. A host may re-fetch its tool list late or + * never, and a model can call a tool still sitting in its context from an earlier list, + * so a feature withdrawn from `tools/list` stays reachable until it is also refused + * here. `toolPromotion` is the clearest case: a promoted tool would otherwise keep + * working indefinitely after being withdrawn. + * + * Covers only the gates that make a tool uncallable. `fileArgs` is argument-level and + * belongs next to the code that reads the files; `hitl` changes how run_tool executes, + * never whether it runs, and is not declared by this build at all. + */ +export function policyRefusal(input: { + name: string; + decision: Decision; + promoted: ReadonlySet; +}): CallToolResult | undefined { + const { name, decision, promoted } = input; + + // Before every other check: setup is the recovery path, so it stays callable in + // states where nothing else is -- including deactivation. + if (name === SETUP_TOOL_NAME) return undefined; + + // Ahead of the feature checks deliberately. `evaluate` reports every feature as false + // when deactivated, so testing features first would answer "metaTools is disabled" + // for a plugin whose actual problem, and only remedy, is its version. + if (decision.deactivated) { + // The remote's own upgrade text when it supplied one -- the design assigns + // upgradeRecommendation.message this job as well as the soft recommendation, since + // the remedy is an upgrade either way, and it may carry instructions we do not know. + const remedy = + decision.upgradeMessage ?? + "Upgrade the Glean plugin, then call `setup` to confirm the connection."; + return refuse( + `[POLICY_DEACTIVATED]\n\nThis version of the Glean plugin is not supported by ` + + `your Glean instance, so only \`setup\` is available and ${name} will not run. ` + + `Do not retry. ${remedy}`, + ); + } + + if (META_TOOL_NAMES.has(name) && !decision.features.metaTools) { + return refuse( + `${name} is disabled for your Glean instance by remote policy and will not run. ` + + `Do not retry — this is not a transient failure. Call \`setup\` to see the ` + + `policy currently in force.`, + ); + } + + if (promoted.has(name) && !decision.features.toolPromotion) { + return refuse( + `${name} is not available: Glean tool promotion is disabled for your instance by ` + + `remote policy, so this call will not run. Do not retry. Call \`setup\` to see ` + + `the policy currently in force.`, + ); + } + + return undefined; +} + +/** Refusal for a `run_tool` call that passes `file_args` while the feature is disabled. */ +export const FILE_ARGS_DISABLED_TEXT = + "`file_args` is disabled for your Glean instance by remote policy, so no file was " + + "read and the tool was not executed. Retry `run_tool` with the values inline in " + + "`arguments` instead."; diff --git a/shared/glean/mcp/src/policy/evaluate.ts b/shared/glean/mcp/src/policy/evaluate.ts new file mode 100644 index 0000000..d265d04 --- /dev/null +++ b/shared/glean/mcp/src/policy/evaluate.ts @@ -0,0 +1,168 @@ +import { FEATURE_NAMES, type FeatureName } from "./key.js"; +import type { + Decision, + PolicyResponse, + VersionSource, + VersionState, +} from "./types.js"; + +// Plain x.y.z comparison. The plugin's own version scheme is plain semver with no +// pre-release or build metadata (enforced by the release tooling), so a full +// semver implementation would be dead weight. A version that does not parse is +// treated as unknown rather than guessed at. +export function parseVersion(v: string): [number, number, number] | undefined { + const m = /^(\d+)\.(\d+)\.(\d+)$/.exec(v.trim()); + if (!m) return undefined; + return [Number(m[1]), Number(m[2]), Number(m[3])]; +} + +/** -1 if a < b, 0 if equal, 1 if a > b; undefined if either is unparseable. */ +export function compareVersions(a: string, b: string): number | undefined { + const pa = parseVersion(a); + const pb = parseVersion(b); + if (!pa || !pb) return undefined; + for (let i = 0; i < 3; i++) { + const x = pa[i] as number; + const y = pb[i] as number; + if (x !== y) return x < y ? -1 : 1; + } + return 0; +} + +function allFeatures(value: boolean): Record { + return Object.fromEntries(FEATURE_NAMES.map((f) => [f, value])) as Record< + FeatureName, + boolean + >; +} + +export interface EvaluateInput { + /** The plugin's own version, and how much that value can be trusted. */ + pluginVersion: string; + versionSource: VersionSource; + /** Features this build actually implements. */ + supportedFeatures: Record; + /** The policy to apply, or undefined for the no-policy / first-run case. */ + policy?: PolicyResponse; +} + +/** + * Resolve a policy against the local build into an enforceable Decision. + * + * Two rules drive everything here: + * 1. A feature is enabled only if this build supports it AND the remote has not + * disabled it. The remote cannot switch on something that is not built. + * 2. No policy means everything supported is enabled and no version rule + * applies -- that is the compatibility path for a remote that does not + * implement negotiation yet. + */ +export function evaluate(input: EvaluateInput): Decision { + const { pluginVersion, versionSource, supportedFeatures, policy } = input; + const reasons: string[] = []; + + if (!policy) { + reasons.push( + "no policy available: enabling all supported features, applying no version policy", + ); + return { + deactivated: false, + versionState: "unenforced", + features: { ...supportedFeatures }, + showUpgrade: false, + reasons, + }; + } + + // ---- version eligibility ------------------------------------------------ + // Enforcement is gated on provenance. Deactivating a plugin on a version we + // cannot actually vouch for would break working installs for no benefit. + let versionState: VersionState = "ok"; + let deactivated = false; + + if (versionSource === "unknown") { + versionState = "unenforced"; + reasons.push( + "version source is unknown: version policy not enforced (never deactivate on an unverifiable version)", + ); + } else { + const blocked = policy.plugin?.blockedVersions ?? []; + const min = policy.plugin?.minimumSupportedVersion; + + // Minimum first: it is the compatibility floor, and "too old" is a more + // actionable message than "specifically blocked". + if (min) { + const cmp = compareVersions(pluginVersion, min); + if (cmp === undefined) { + reasons.push( + `cannot compare ${pluginVersion} against minimum ${min}: skipping minimum check`, + ); + } else if (cmp < 0) { + versionState = "below-minimum"; + deactivated = true; + reasons.push( + `version ${pluginVersion} is below the minimum supported ${min}: deactivated`, + ); + } + } + + // Exact-match list, not a threshold -- it exists for non-contiguous bad + // releases (block 1.2 while 1.1 and 1.3 stay usable). + if (!deactivated && blocked.includes(pluginVersion)) { + versionState = "blocked"; + deactivated = true; + reasons.push(`version ${pluginVersion} is explicitly blocked: deactivated`); + } + + if (!deactivated) { + const latest = policy.plugin?.latestVersion; + if (latest && compareVersions(pluginVersion, latest) === -1) { + versionState = "outdated-supported"; + reasons.push( + `version ${pluginVersion} is older than latest ${latest} but supported`, + ); + } + } + } + + // ---- feature policy ----------------------------------------------------- + // A deactivated plugin exposes only setup, so every feature is inert. Reported + // as all-false so no caller can accidentally act on a stale enablement. + if (deactivated) { + return { + deactivated: true, + versionState, + features: allFeatures(false), + showUpgrade: true, + message: policy.message, + upgradeMessage: policy.plugin?.upgradeRecommendation?.message, + reasons, + }; + } + + const features = {} as Record; + for (const name of FEATURE_NAMES) { + const supported = supportedFeatures[name] === true; + if (!supported) { + features[name] = false; + continue; + } + // An omitted feature is left enabled: the remote naming fewer features than + // the plugin supports means it has no opinion, not that it said no. + const entry = policy.features?.[name]; + const enabled = entry?.enabled !== false; + features[name] = enabled; + if (!enabled) reasons.push(`feature "${name}" disabled by remote policy`); + } + + return { + deactivated: false, + versionState, + features, + showUpgrade: + versionState === "outdated-supported" && + policy.plugin?.upgradeRecommendation?.show === true, + message: policy.message, + upgradeMessage: policy.plugin?.upgradeRecommendation?.message, + reasons, + }; +} diff --git a/shared/glean/mcp/src/policy/key.ts b/shared/glean/mcp/src/policy/key.ts new file mode 100644 index 0000000..8d939cd --- /dev/null +++ b/shared/glean/mcp/src/policy/key.ts @@ -0,0 +1,27 @@ +// The vendor-prefixed `_meta` key carrying the policy exchange. This is an +// application-level Glean extension, not a core MCP method or capability: the +// host never needs to understand it, and the plugin both creates and consumes it. +export const CAPABILITY_POLICY_KEY = "com.glean.mcp/capabilityPolicy"; + +// The features whose enablement the remote controls. Keep in lockstep with +// FeatureName below -- the array exists so the plugin can declare support for +// exactly the features it implements, no more. +// +// `hitl` is deliberately absent, though the design contract defines it. Disabling local +// HITL only makes sense once approval moves to the remote, and the remote is stateless: +// it has no back-channel to send `elicitation/create` on, so remote-side approval waits +// on MCP 2026-07-28 landing there. Until then a remote-disabled HITL would leave no gate +// at all -- and under Claude Code it would be worse than having no policy, because the +// plugin's PreToolUse hook auto-approves run_tool on the premise that the local prompt +// IS the gate. A remote that needs to stop such a plugin deactivates the version. +// +// Declaring it here is what would make the remote believe it is controllable, so the +// honest signal is to leave it out until a build can honour it. A remote that sends +// `features.hitl` anyway takes the unknown-key path: accepted, logged, ignored. +export const FEATURE_NAMES = [ + "toolPromotion", + "metaTools", + "fileArgs", +] as const; + +export type FeatureName = (typeof FEATURE_NAMES)[number]; diff --git a/shared/glean/mcp/src/policy/negotiate.ts b/shared/glean/mcp/src/policy/negotiate.ts new file mode 100644 index 0000000..121c282 --- /dev/null +++ b/shared/glean/mcp/src/policy/negotiate.ts @@ -0,0 +1,158 @@ +import { CAPABILITY_POLICY_KEY, FEATURE_NAMES } from "./key.js"; +import type { + NegotiationOutcome, + NegotiationRequest, + PolicyResponse, +} from "./types.js"; + +/** + * Wrap the negotiation payload in the `_meta` envelope carried on every outgoing + * request. Verified against MCP SDK 1.12 over StreamableHTTP: `_meta` on + * `tools/list` and `tools/call` params reaches the server, and `result._meta` + * survives Zod parsing on the way back, with nested objects intact. + */ +export function metaFor(request: NegotiationRequest): { + _meta: Record; +} { + return { _meta: { [CAPABILITY_POLICY_KEY]: request } }; +} + +function isRecord(v: unknown): v is Record { + return typeof v === "object" && v !== null && !Array.isArray(v); +} + +// Keys this version understands, by object. Anything outside these sets is still +// ACCEPTED -- forward compatibility requires that a remote may add fields without an +// older plugin calling the response malformed -- but it is collected and reported so +// the ignoring becomes observable. Silence is the failure mode worth avoiding: an +// unrecognized key's type is never checked, so a renamed or misspelled field carrying +// a bad value is indistinguishable from an absent one. +const KNOWN_TOP_LEVEL: ReadonlySet = new Set([ + "plugin", + "features", + "message", +]); +const KNOWN_PLUGIN: ReadonlySet = new Set([ + "latestVersion", + "minimumSupportedVersion", + "blockedVersions", + "upgradeRecommendation", +]); +const KNOWN_UPGRADE: ReadonlySet = new Set([ + "show", + "dailyCap", + "weeklyCap", + "message", +]); + +function unknownIn( + obj: unknown, + known: ReadonlySet, + prefix: string, +): string[] { + if (!isRecord(obj)) return []; + return Object.keys(obj) + .filter((k) => !known.has(k)) + .map((k) => `${prefix}${k}`); +} + +/** + * Validate a candidate policy object. Deliberately shallow: it rejects shapes + * that would make `evaluate` misbehave and ignores everything else, so a remote + * can add fields without old plugins calling the response malformed. + * + * On success it also returns the keys it did not recognize. Those are still ignored + * for evaluation; the caller logs them so a shape mismatch during a rollout is + * greppable instead of invisible. + */ +export function validatePolicy( + value: unknown, +): + | { ok: true; policy: PolicyResponse; unknownKeys: string[] } + | { ok: false; reason: string } { + if (!isRecord(value)) return { ok: false, reason: "policy is not an object" }; + + const plugin = value.plugin; + if (plugin !== undefined) { + if (!isRecord(plugin)) { + return { ok: false, reason: "plugin must be an object" }; + } + for (const key of ["latestVersion", "minimumSupportedVersion"] as const) { + const v = plugin[key]; + if (v !== undefined && typeof v !== "string") { + return { ok: false, reason: `plugin.${key} must be a string` }; + } + } + const blocked = plugin.blockedVersions; + if ( + blocked !== undefined && + (!Array.isArray(blocked) || blocked.some((b) => typeof b !== "string")) + ) { + return { ok: false, reason: "plugin.blockedVersions must be string[]" }; + } + const rec = plugin.upgradeRecommendation; + if (rec !== undefined && !isRecord(rec)) { + return { ok: false, reason: "plugin.upgradeRecommendation must be an object" }; + } + } + + const features = value.features; + if (features !== undefined) { + if (!isRecord(features)) { + return { ok: false, reason: "features must be an object" }; + } + for (const [name, entry] of Object.entries(features)) { + if (!isRecord(entry)) { + return { ok: false, reason: `features.${name} must be an object` }; + } + if (entry.enabled !== undefined && typeof entry.enabled !== "boolean") { + return { ok: false, reason: `features.${name}.enabled must be boolean` }; + } + } + } + + if (value.message !== undefined && typeof value.message !== "string") { + return { ok: false, reason: "message must be a string" }; + } + + // Unrecognized feature NAMES are collected too. A policy naming a feature this + // build does not implement means the plugin is older than the policy -- worth + // seeing, even though `evaluate` correctly ignores it. + const unknownKeys = [ + ...unknownIn(value, KNOWN_TOP_LEVEL, ""), + ...unknownIn(value.plugin, KNOWN_PLUGIN, "plugin."), + ...unknownIn( + isRecord(value.plugin) ? value.plugin.upgradeRecommendation : undefined, + KNOWN_UPGRADE, + "plugin.upgradeRecommendation.", + ), + ...unknownIn(value.features, new Set(FEATURE_NAMES), "features."), + ]; + + return { ok: true, policy: value as PolicyResponse, unknownKeys }; +} + +/** + * Classify what came back from a successful request. + * + * The distinction between "succeeded but no policy" and "could not reach the + * remote" is the subtlest part of the contract and is NOT decided here -- an + * unreachable remote never produces a result to classify, so the caller reports + * that case separately. Conflating them silently disables version enforcement, + * because no-policy clears version rules while unreachable must keep applying + * the last synced ones. + */ +export function classifyResult(result: unknown): NegotiationOutcome { + const meta = isRecord(result) ? result._meta : undefined; + const candidate = isRecord(meta) ? meta[CAPABILITY_POLICY_KEY] : undefined; + + if (candidate === undefined) return { kind: "no-policy" }; + + const validated = validatePolicy(candidate); + if (!validated.ok) return { kind: "malformed", reason: validated.reason }; + return { + kind: "policy", + policy: validated.policy, + unknownKeys: validated.unknownKeys, + }; +} diff --git a/shared/glean/mcp/src/policy/protocol-version.ts b/shared/glean/mcp/src/policy/protocol-version.ts new file mode 100644 index 0000000..113f46e --- /dev/null +++ b/shared/glean/mcp/src/policy/protocol-version.ts @@ -0,0 +1,105 @@ +import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; +import type { JSONRPCMessage } from "@modelcontextprotocol/sdk/types.js"; + +/** + * Observe the MCP revision the plugin and the host settle on. + * + * The SDK negotiates this internally -- it compares the host's requested version + * against SUPPORTED_PROTOCOL_VERSIONS and answers with either that version or its + * own latest -- but exposes no server-side accessor for the outcome. `Server` keeps + * only clientInfo and client capabilities. So the value is recovered by watching the + * `initialize` exchange on the transport, which is public API: `Transport` is a + * plain interface, so wrapping it needs no private-field access and no subclassing. + * + * The NEGOTIATED value is read from the initialize *response*, not the request. The + * request carries what the host proposed, which differs whenever the host asks for a + * revision this build does not implement -- exactly the case worth reporting. + */ +export class ProtocolVersionObserver { + private negotiated: string | undefined; + private proposed: string | undefined; + private initializeId: string | number | undefined; + + /** The agreed revision, or undefined when it could not be observed. */ + get version(): string | undefined { + return this.negotiated; + } + + /** What the host asked for. Retained for diagnostics, not reported as the answer. */ + get requested(): string | undefined { + return this.proposed; + } + + /** + * Wrap a transport so the initialize exchange is observed as it passes through. + * Delegates everything; the only additions are two taps that never throw, since a + * failed observation must degrade to "unknown" rather than break the connection. + */ + wrap(inner: Transport): Transport { + const observer = this; + + const wrapped: Transport = { + start: () => inner.start(), + close: () => inner.close(), + send: async (message, options) => { + observer.observeOutgoing(message); + return inner.send(message, options); + }, + get sessionId() { + return inner.sessionId; + }, + set onmessage(handler) { + inner.onmessage = handler + ? (message, extra) => { + observer.observeIncoming(message); + handler(message, extra); + } + : undefined; + }, + get onmessage() { + return inner.onmessage; + }, + set onclose(handler) { + inner.onclose = handler; + }, + get onclose() { + return inner.onclose; + }, + set onerror(handler) { + inner.onerror = handler; + }, + get onerror() { + return inner.onerror; + }, + }; + + return wrapped; + } + + private observeIncoming(message: JSONRPCMessage): void { + try { + const m = message as { id?: string | number; method?: string; params?: unknown }; + if (m.method !== "initialize" || m.id === undefined) return; + this.initializeId = m.id; + const params = m.params as { protocolVersion?: unknown } | undefined; + if (typeof params?.protocolVersion === "string") { + this.proposed = params.protocolVersion; + } + } catch { + // Observation is best-effort; the field is omitted rather than guessed. + } + } + + private observeOutgoing(message: JSONRPCMessage): void { + try { + const m = message as { id?: string | number; result?: unknown }; + if (m.id === undefined || m.id !== this.initializeId) return; + const result = m.result as { protocolVersion?: unknown } | undefined; + if (typeof result?.protocolVersion === "string") { + this.negotiated = result.protocolVersion; + } + } catch { + // As above. + } + } +} diff --git a/shared/glean/mcp/src/policy/session.ts b/shared/glean/mcp/src/policy/session.ts new file mode 100644 index 0000000..a88fb45 --- /dev/null +++ b/shared/glean/mcp/src/policy/session.ts @@ -0,0 +1,319 @@ +import type { Server } from "@modelcontextprotocol/sdk/server/index.js"; + +import { hostIdentityFromHandshake, buildNegotiationRequest, supportedFeatures } from "./context.js"; +import { classifyResult, metaFor } from "./negotiate.js"; +import { evaluate } from "./evaluate.js"; +import { loadCachedPolicy, savePolicy } from "./cache.js"; +import { ProtocolVersionObserver } from "./protocol-version.js"; +import type { Decision, NegotiationRequest, PolicyResponse } from "./types.js"; + +type LogFn = (label: string, detail?: Record) => void; + +/** + * Session-scoped state for the capability/policy exchange. + * + * Kept out of index.ts so the wiring there stays to a few lines, and so the pieces a + * later change needs -- the resolved decision, the `_meta` to attach -- have one + * obvious home. + * + * This module owns the decision's lifecycle: it reports context, records the policy a + * response carried, and resolves the decision now in force. It does not itself withhold + * or refuse anything -- the gates are pure functions in ./enforce.ts, applied by the + * handlers in index.ts. + */ +let mcpServer: Server | undefined; +let logLine: LogFn = () => {}; +let decision: Decision | undefined; +// Labels already logged in this process, so each negotiation path reports itself once +// even when the resolved decision never changes. See recordPolicyFromResult. +const loggedLabels = new Set(); +let lastRequest: NegotiationRequest | undefined; +// The remote the policy cache is keyed by. Held here rather than threaded through +// every call site so that `callRemoteTool` -- the single funnel every remote tool call +// passes through -- can record policy without knowing about configuration resolution. +// Keyed by URL so switching Glean instances cannot apply one instance's policy to +// another. +let cacheKeyUrl: string | undefined; + +export const protocolVersion = new ProtocolVersionObserver(); + +// The label recordPolicyFromResult receives for a tools/list exchange. Exported so the +// caller and the notification guard cannot drift apart on a string literal — the guard +// depends on distinguishing that path from tools/call. +export const TOOLS_LIST_LABEL = "tools/list"; + +export function initPolicySession(server: Server, log: LogFn): void { + mcpServer = server; + logLine = log; +} + +/** Called whenever the configured server URL is resolved or changed. */ +export function setPolicyServerUrl(url: string | undefined): void { + cacheKeyUrl = url; +} + +/** + * The negotiation payload for the current session, rebuilt per request so a late + * `initialize` (the handshake completes after the transport is wired) is reflected + * rather than captured as undefined at startup. + */ +export function negotiationRequest(): NegotiationRequest { + const host = hostIdentityFromHandshake( + mcpServer?.getClientVersion(), + mcpServer?.getClientCapabilities() as Record | undefined, + protocolVersion.version, + ); + lastRequest = buildNegotiationRequest(host); + return lastRequest; +} + +/** The `_meta` envelope to attach to an outgoing remote request. */ +export function negotiationMeta(): { _meta: Record } { + return metaFor(negotiationRequest()); +} + +/** + * Record the policy carried on a remote response, if any. + * + * The four outcomes are distinct on purpose: + * policy - persist it and re-evaluate. + * no-policy - no policy on THIS response. What that means depends on whether this + * remote has ever sent one; see below. The cache is never erased. + * malformed - keep the last valid policy and treat this round as no-policy, so a + * bad response can never deactivate a working plugin. + * unreachable - decided by the caller, not here: an unreachable remote produces no + * result to classify. Conflating it with no-policy would silently + * drop a previously synced version rule on any network blip. + * + * On no-policy, a cached policy is RETAINED rather than cleared. The compatibility path + * -- every supported feature enabled, no version rule -- is for a remote that does not + * implement negotiation, and the cached policy is the evidence of whether this one does. + * A remote that has already sent a policy is not disowning it by omitting it later. + * + * Reading omission as revocation looks harmless until a remote attaches policy to + * tools/list but not to tools/call -- a plausible split, since a list is answered once + * while calls are the hot path. Then every call would clear the policy, every following + * list would restore it, and each clear would emit tools/list_changed, so the host would + * re-fetch on every tool call and the advertised surface would visibly flicker. + * + * The cost is that a remote can no longer lift a restriction by going silent: it must + * send an explicit `enabled: true`. For a control plane that is the better default -- + * clearing-by-silence is exactly what makes the flicker possible -- and it makes this + * agree with the unreachable path, which already retains the cached policy. + */ +export function recordPolicyFromResult(result: unknown, label: string): void { + // No configured remote yet means nothing to key the cache by, and no exchange to + // record. Silent no-op rather than an error: this runs on every remote call. + const serverUrl = cacheKeyUrl; + if (!serverUrl) return; + + const outcome = classifyResult(result); + const cachedPolicy = loadCachedPolicy(serverUrl); + let policy: PolicyResponse | undefined; + + switch (outcome.kind) { + case "policy": + policy = outcome.policy; + savePolicy(serverUrl, outcome.policy); + // Unknown keys are accepted and ignored -- a newer remote must be able to add + // fields without an older plugin rejecting the response -- but reported, since + // an unrecognized key's type is never checked and a renamed field carrying a + // bad value would otherwise pass in total silence. + if (outcome.unknownKeys.length > 0) { + logLine("policy.unknown-keys", { label, keys: outcome.unknownKeys }); + } + reportUnenforcedCaps(outcome.policy); + break; + case "malformed": + logLine("policy.malformed", { label, reason: outcome.reason }); + policy = cachedPolicy; + break; + case "no-policy": + // Silence, not revocation. Undefined only when this remote has never sent a + // policy, which is the compatibility path's actual condition. + policy = cachedPolicy; + if (cachedPolicy) { + logLine("policy.absent-kept-cache", { label }); + } + break; + } + + const request = lastRequest ?? negotiationRequest(); + const next = evaluate({ + pluginVersion: request.plugin.version, + versionSource: request.plugin.versionSource, + supportedFeatures: request.plugin.supportedFeatures, + policy, + }); + + const previous = decision; + const changed = + !previous || surfaceKey(previous) !== surfaceKey(next); + decision = next; + + // Log on a change, and once per label per process. + // + // The second condition is what makes the mechanism observable at all. A steady-state + // exchange changes nothing -- against a remote with no policy support, every response + // resolves to the same decision -- so change-only logging goes silent after the first + // tools/list and never says a word about tools/call again. The result is a feature + // whose entire job is remote control, with no evidence in the log that it ran on a + // given path. Labels are bounded (one per distinct remote method plus tool name), so + // this is a handful of lines per process, not per call. + const firstForLabel = !loggedLabels.has(label); + loggedLabels.add(label); + + if (changed || firstForLabel) { + logLine("policy.resolved", { + label, + outcome: outcome.kind, + versionState: next.versionState, + deactivated: next.deactivated, + features: next.features, + reasons: next.reasons, + }); + } + + // Tell the host to re-fetch, but only from the tools/call path and only once a + // previous decision existed. + // + // Not from tools/list: there the response IS the update -- the host has just asked and + // is about to receive the freshly filtered surface, so a notification would only make + // it ask again for what it already holds, and notify -> tools/list -> resolve -> + // notify would be a cycle. A policy arriving on a tools/call response is the case that + // needs this, because the surface changed and the host has no reason to re-fetch. + // + // Not on the first decision either: `!previous` counts as changed, so without that + // guard every process's first gated call would notify, costing a host tools/list and a + // remote round-trip for every user in a world where no policy exists. The stale-list + // window on a first call is closed by the refusal, which is the real gate anyway. + if (changed && previous && label !== TOOLS_LIST_LABEL) { + logLine("policy.surface-changed", { + label, + from: { deactivated: previous.deactivated, features: previous.features }, + to: { deactivated: next.deactivated, features: next.features }, + }); + mcpServer?.sendToolListChanged().catch(() => { + // Transport not connected, or the host does not support the notification. + // Harmless: the next tools/list is correct, and calls are refused regardless. + }); + } +} + +// The fields that determine what the agent can reach. Reasons and messages churn between +// otherwise-equivalent responses without changing the surface, and every spurious +// notification costs a host tools/list and therefore a remote round-trip. +function surfaceKey(d: Decision): string { + return JSON.stringify([d.deactivated, d.features]); +} + +// Whether this process has already said the caps are inert. +let capsReported = false; + +/** + * Say once that display caps are accepted but not honoured. + * + * `dailyCap`/`weeklyCap` are deliberately tolerated rather than rejected: the design has + * the remote send them for forward compatibility, and only `show` needs honouring in v0. + * But a remote that sets `dailyCap: 1` and then sees the recommendation on every `setup` + * has no way to learn why, and "read the plugin source" is not an answer. Frequency + * capping also has nothing to cap yet — `setup` is the only surface and it is + * user-initiated, so rate-limiting the answer to a question the user just asked would be + * wrong. Once per process, and only when a remote actually sets one. + */ +function reportUnenforcedCaps(policy: PolicyResponse): void { + if (capsReported) return; + const rec = policy.plugin?.upgradeRecommendation; + const dailyCap = rec?.dailyCap; + const weeklyCap = rec?.weeklyCap; + if (dailyCap === undefined && weeklyCap === undefined) return; + capsReported = true; + logLine("policy.caps-not-enforced", { dailyCap, weeklyCap }); +} + +/** + * The decision now in force, seeded from the cached policy on first read. + * + * A fresh process has no decision until a remote exchange happens, and `tools/list` does + * not always reach the remote -- the unconfigured, unauthenticated and connect-error + * paths all return before any negotiation. Treating that as "no policy, everything on" + * would mean a cached deactivation, or a cached `metaTools: false`, was silently undone + * by every process start, which is exactly what the design forbids. + * + * So the first read evaluates the cached policy and memoizes the result. The cache is + * touched once per process, never per gated call: `_meta` rides every remote request, so + * a process that talks to the remote refreshes its own decision constantly and needs no + * re-read. With no configured URL there is nothing to key by, which yields the + * all-supported decision. + * + * Never throws. A failure here would break every tool call, and for a feature that is + * inert for every install today, failing open is the only defensible direction. + */ +export function decisionInForce(): Decision { + if (decision) return decision; + try { + const policy = cacheKeyUrl ? loadCachedPolicy(cacheKeyUrl) : undefined; + const request = lastRequest ?? negotiationRequest(); + decision = evaluate({ + pluginVersion: request.plugin.version, + versionSource: request.plugin.versionSource, + supportedFeatures: request.plugin.supportedFeatures, + policy, + }); + if (policy) { + logLine("policy.seeded-from-cache", { + versionState: decision.versionState, + deactivated: decision.deactivated, + features: decision.features, + }); + } + return decision; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + logLine("policy.seed-failed", { msg }); + decision = evaluate({ + pluginVersion: "0.0.0", + versionSource: "unknown", + supportedFeatures: supportedFeatures(), + policy: undefined, + }); + return decision; + } +} + +/** For `setup` output: what was reported, and what came back. */ +export function policySummary(): string[] { + const r = lastRequest; + const d = decision; + const lines = [ + `Plugin version: ${r?.plugin.version ?? "?"} (source: ${r?.plugin.versionSource ?? "?"})`, + `Host: ${r?.host.id ?? "?"} ${r?.host.version ?? ""} (source: ${r?.host.source ?? "?"})`, + `MCP revision: ${r?.host.mcpProtocolVersion ?? "not observed"}`, + `Server inventory: ${r?.configuredServers.source ?? "?"}`, + ]; + if (d) { + lines.push(`Policy: version ${d.versionState}, features ${JSON.stringify(d.features)}`); + // The recommendation is only shown here. It is computed on every exchange, so + // without a surface it would be a value the remote sets and nobody ever sees. + // Shown on every setup call: dailyCap/weeklyCap are accepted but not enforced + // (see reportUnenforcedCaps), and capping a user-initiated answer would be wrong + // anyway. + if (d.deactivated) { + lines.push( + `Deactivated: only \`setup\` is available. ${ + d.upgradeMessage ?? "Upgrade the Glean plugin to restore functionality." + }`, + ); + } else if (d.showUpgrade) { + lines.push( + `Upgrade available: ${ + d.upgradeMessage ?? "a newer version of the Glean plugin is available." + }`, + ); + } + if (d.message) lines.push(`Notice: ${d.message}`); + } else { + lines.push("Policy: not yet negotiated"); + } + return lines; +} diff --git a/shared/glean/mcp/src/policy/types.ts b/shared/glean/mcp/src/policy/types.ts new file mode 100644 index 0000000..d06ad32 --- /dev/null +++ b/shared/glean/mcp/src/policy/types.ts @@ -0,0 +1,147 @@ +import type { FeatureName } from "./key.js"; + +// ---------------------------------------------------------------- request side + +// VersionSource is defined alongside the build constant it describes, in +// ../version.ts. Imported for local use and re-exported so the payload types read +// as one unit. +import type { VersionSource } from "../version.js"; +export type { VersionSource }; + +// Where host identity came from. `handshake` is the MCP initialize exchange, +// which is the reliable path and needs no host-specific code. +export type HostSource = "handshake" | "env" | "unknown"; + +// The inventory is all-or-nothing on purpose. A partial list is worse than none +// for policy, because it is indistinguishable from a user who genuinely has +// fewer servers. +export type InventorySource = "host-cli" | "unavailable"; + +export type AuthStatus = "authenticated" | "unauthenticated" | "unknown"; + +export interface ConfiguredServer { + name: string; + url?: string; + authStatus: AuthStatus; +} + +export interface ConfiguredServers { + source: InventorySource; + servers?: ConfiguredServer[]; +} + +/** The host block of the request, as observed from the MCP handshake. */ +export interface HostIdentity { + id: string; + version?: string; + /** + * The MCP revision the plugin and host settled on for this session -- the value + * from the initialize RESULT, not what the host proposed. Omitted rather than + * guessed when it could not be observed; see protocol-version.ts. + */ + mcpProtocolVersion?: string; + capabilities?: Record; + source: HostSource; +} + +export interface NegotiationRequest { + // supportedFeatures lives HERE, not at the top level: the features a build + // implements are a property of that build, fixed at compile time and changing + // only when a new version ships -- exactly like `version` beside it. The + // response's `features` map stays top level because it is a per-session policy + // decision, not plugin metadata; the two are not peers despite the similar name. + plugin: { + id: string; + version: string; + versionSource: VersionSource; + supportedFeatures: Record; + }; + host: HostIdentity; + configuredServers: ConfiguredServers; +} + +// --------------------------------------------------------------- response side + +export interface UpgradeRecommendation { + show?: boolean; + dailyCap?: number; + weeklyCap?: number; + /** + * Text shown with a recommendation, and also the text used when version policy + * deactivates the plugin -- the remedy is an upgrade either way. + * + * The remote owns the wording, including whether to name a version. Nothing here + * interpolates `latestVersion` into it: a plugin that composed its own sentence would + * be second-guessing a message the remote wrote for its own users. + */ + message?: string; +} + +// Plugin-scoped rules are grouped under `plugin`; session-scoped policy stays at +// the top level. Version rules and upgrade guidance describe the installed +// artifact and change only when a release changes, whereas `features` and +// `message` are decisions about this session that can differ between two calls +// from the same build. The grouping is also what removes the `Plugin` prefix these +// names used to carry to compensate for having no namespace, and it leaves a slot +// for the deferred host-version rules as a `host` sibling. +export interface PluginPolicy { + latestVersion?: string; + minimumSupportedVersion?: string; + blockedVersions?: string[]; + upgradeRecommendation?: UpgradeRecommendation; +} + +export interface PolicyResponse { + plugin?: PluginPolicy; + features?: Partial>; + message?: string; +} + +// ------------------------------------------------------------------- outcomes + +// Classification of a single negotiation round-trip. These four are genuinely +// different and conflating any two of them breaks the contract: +// +// policy - a valid policy object came back; persist and apply it. +// no-policy - the request SUCCEEDED but carried no policy object. The +// remote does not implement negotiation yet. All supported +// features are enabled and NO version policy applies. Must not +// erase the cache. +// malformed - a policy object came back but failed validation. Keep the +// last valid policy and treat this round as no-policy; garbage +// must never be able to deactivate a working plugin. +// unreachable - the request failed. Fall back to the CACHED policy and +// cached tools list, which is different from no-policy: here a +// previously synced version rule still applies. +export type NegotiationOutcome = + | { kind: "policy"; policy: PolicyResponse; unknownKeys: string[] } + | { kind: "no-policy" } + | { kind: "malformed"; reason: string } + | { kind: "unreachable"; reason: string }; + +// ------------------------------------------------------------------- decision + +export type VersionState = + | "ok" + | "outdated-supported" + | "below-minimum" + | "blocked" + | "unenforced"; + +export interface Decision { + /** Deactivated plugins advertise ONLY the setup tool. */ + deactivated: boolean; + versionState: VersionState; + features: Record; + /** True when an upgrade recommendation should be surfaced this session. */ + showUpgrade: boolean; + message?: string; + /** + * The remote's upgrade text, carried separately from `message` because they answer + * different questions: `message` is about this session, this is about the installed + * artifact. Used for the recommendation and for the deactivation refusal. + */ + upgradeMessage?: string; + /** Human-readable trail of why this decision came out the way it did. */ + reasons: string[]; +} diff --git a/shared/glean/mcp/src/remote-client.ts b/shared/glean/mcp/src/remote-client.ts index e9a87f3..9e60ed2 100644 --- a/shared/glean/mcp/src/remote-client.ts +++ b/shared/glean/mcp/src/remote-client.ts @@ -3,7 +3,8 @@ import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/ import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import type { GleanOAuthClientProvider } from "./auth-provider.js"; -import { PLUGIN_VERSION } from "./version.js"; +import { pluginVersionString } from "./version.js"; +import { negotiationMeta, recordPolicyFromResult } from "./policy/session.js"; const GLEAN_PLUGIN = "GLEAN_PLUGIN"; @@ -210,7 +211,7 @@ export async function createRemoteClient( } const client = new Client( - { name: "glean", version: PLUGIN_VERSION }, + { name: "glean", version: pluginVersionString() }, { capabilities: {} }, ); @@ -234,11 +235,15 @@ export async function callRemoteTool( name: string, args: Record, ): Promise { - // Pass an explicit timeout so the call isn't capped at the SDK's 60s default. - // `undefined` for resultSchema keeps the SDK's CallToolResultSchema default. - const result = await client.callTool({ name, arguments: args }, undefined, { - timeout: remoteToolTimeoutMs(), - }); + // Every downstream tool call reports the negotiated context and records any + // capability policy returned by the remote. Keep the explicit timeout so + // long-running Glean tools are not capped at the SDK's 60s default. + const result = await client.callTool( + { name, arguments: args, ...negotiationMeta() }, + undefined, + { timeout: remoteToolTimeoutMs() }, + ); + recordPolicyFromResult(result, `tools/call(${name})`); if (!("content" in result)) { return { content: [] }; } diff --git a/shared/glean/mcp/src/remote-tools-cache-store.ts b/shared/glean/mcp/src/remote-tools-cache-store.ts index fa2900b..4eceac0 100644 --- a/shared/glean/mcp/src/remote-tools-cache-store.ts +++ b/shared/glean/mcp/src/remote-tools-cache-store.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { homedir } from "node:os"; import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import { writeFileAtomicSync } from "./atomic-write.js"; const CACHE_FILENAME = "remote-tools-cache.json"; const DIR_MODE = 0o700; @@ -15,19 +16,19 @@ function cacheFile(): string { return path.join(resolveCacheDir(), CACHE_FILENAME); } -interface CacheEntry { +interface ToolsCacheEntry { tools: Tool[]; fetchedAt: string; } -type Store = Record; +type ToolsCacheFile = Record; -function readStore(): Store { +function readStore(): ToolsCacheFile { try { const raw = fs.readFileSync(cacheFile(), "utf-8"); const data = JSON.parse(raw); if (data && typeof data === "object" && !Array.isArray(data)) { - return data as Store; + return data as ToolsCacheFile; } return {}; } catch { @@ -35,16 +36,12 @@ function readStore(): Store { } } -function writeStore(store: Store): void { +function writeStore(store: ToolsCacheFile): void { const filePath = cacheFile(); const dir = path.dirname(filePath); fs.mkdirSync(dir, { recursive: true, mode: DIR_MODE }); fs.chmodSync(dir, DIR_MODE); - fs.writeFileSync(filePath, JSON.stringify(store, null, 2), { - encoding: "utf-8", - mode: FILE_MODE, - }); - fs.chmodSync(filePath, FILE_MODE); + writeFileAtomicSync(filePath, JSON.stringify(store, null, 2), FILE_MODE); } export function loadRemoteTools(serverUrl: string): Tool[] { @@ -55,6 +52,15 @@ export function loadRemoteTools(serverUrl: string): Tool[] { return entry.tools; } +// Persist the remote catalog for a URL. Only ever the RAW allow-listed catalog, never +// a surface that capability policy has already been applied to. +// +// That distinction is what lets this cache and the policy cache (policy/cache.ts) be +// written and cleared independently: tools/list composes its answer by filtering this +// catalog through the policy in force, at read time, so a catalog from one moment and a +// policy from another still yield the surface the current policy dictates. Caching a +// post-policy surface here would reintroduce exactly the skew that avoids — a stored +// surface would keep advertising what a newer policy has withdrawn. export function saveRemoteTools(serverUrl: string, tools: Tool[]): void { if (!serverUrl) return; try { diff --git a/shared/glean/mcp/src/tools/remote-passthrough.ts b/shared/glean/mcp/src/tools/remote-passthrough.ts index 5fd8e76..1be70d2 100644 --- a/shared/glean/mcp/src/tools/remote-passthrough.ts +++ b/shared/glean/mcp/src/tools/remote-passthrough.ts @@ -6,6 +6,11 @@ import { createRemoteClient, type RemoteClientOptions, } from "../remote-client.js"; +import { + TOOLS_LIST_LABEL, + negotiationMeta, + recordPolicyFromResult, +} from "../policy/session.js"; // Remote tools promoted to first-class local tools once setup completes. // Anything the remote MCP server exposes that is not in this set is dropped @@ -59,9 +64,11 @@ export async function fetchAllowedRemoteTools( const collected: Tool[] = []; let cursor: string | undefined; do { - const page = await remoteClient.listTools( - cursor ? { cursor } : undefined, - ); + const page = await remoteClient.listTools({ + ...(cursor ? { cursor } : {}), + ...negotiationMeta(), + }); + recordPolicyFromResult(page, TOOLS_LIST_LABEL); for (const tool of page.tools) { if (!REMOTE_TOOLS_ALLOWLIST.has(tool.name)) continue; collected.push({ diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 59fd2c5..7c8d2d7 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -6,6 +6,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { callRemoteTool } from "../remote-client.js"; +import { FILE_ARGS_DISABLED_TEXT } from "../policy/enforce.js"; import { buildCompactArgs, writeApprovalArgsFile } from "./approval-args.js"; import { resolveSessionId } from "../session-id.js"; @@ -187,27 +188,23 @@ async function findToolJson( return null; } -// A stdio server's only client signal is clientInfo.name. Cursor reports -// "cursor-vscode" and already renders the tool name + arguments in its own -// expandable UI, so its approval prompt only needs a one-line review ask. +// A stdio server's only client signal is clientInfo.name; Cursor reports +// "cursor-vscode". Used to explain the known dropped-elicitation failure mode +// when an approval request waits out its full timeout. export function isCursorClient(mcpServer: Server): boolean { return (mcpServer.getClientVersion()?.name ?? "") .toLowerCase() .startsWith("cursor"); } -// Plain text, NOT Markdown: Claude Code does not reliably render Markdown in -// elicitation prompts. Kept short (a few lines) so the Accept/Decline buttons -// stay in view; full argument detail spills to a file when it can't fit. +// Plain text, NOT Markdown: every host, including Cursor, gets the action and +// arguments in the elicitation itself. Depending on a host to render them above +// the prompt left Cursor's review text pointing at content that no longer +// appeared in newer builds. async function buildApprovalMessage( - mcpServer: Server, toolName: string, args: unknown, ): Promise { - if (isCursorClient(mcpServer)) { - return `Review the tool and arguments shown above, click on Submit to allow and Cancel to deny.`; - } - const { lines, needsFile } = buildCompactArgs(args); // Indent argument lines under "Arguments:" so the structural labels stay // distinct from values; keys are uppercased (in compactArgLine) so a key @@ -281,11 +278,49 @@ async function currentPermissionMode(): Promise { } } +function humanizeMs(ms: number): string { + const seconds = Math.round(ms / 1000); + if (seconds < 120) return `${seconds}s`; + const minutes = Math.round(seconds / 60); + return `${minutes} minute${minutes === 1 ? "" : "s"}`; +} + +export function elicitationFailureText( + mcpServer: Server, + toolName: string, + detail: string, + elapsedMs: number, + timeoutMs: number, +): string { + const base = + `Action ${toolName} was not approved — the approval request failed ` + + `(${detail}). The action was NOT executed.`; + const waitedFullTimeout = elapsedMs >= timeoutMs * 0.9; + if (!waitedFullTimeout || !isCursorClient(mcpServer)) { + return `${base} Ask the user to confirm, then retry.`; + } + return ( + `${base}\n\n` + + `It waited the full ${humanizeMs(timeoutMs)} without an answer. Either the approval ` + + `prompt was shown and went unanswered, or it was never shown at all — this end ` + + `cannot tell which. One possible cause, if no prompt appeared, is a known Cursor ` + + `issue before version 3.15: a server-initiated approval prompt can be dropped ` + + `silently, leaving nothing on screen to accept or dismiss. Ask the user whether they ` + + `saw an approval prompt. If they did not, suggest checking Cursor's version and ` + + `updating if it is below 3.15 — otherwise a retry may wait out the clock again.` + ); +} + +export interface RunToolPolicy { + fileArgs: boolean; +} + export async function handleRunTool( remoteClient: Client, mcpServer: Server, skillsBaseDir: string, args: Record, + policy: RunToolPolicy, ): Promise { const serverId = args.server_id; const toolName = args.tool_name; @@ -304,6 +339,15 @@ export async function handleRunTool( // drives the HITL gate. Both paths must see it regardless of ENABLE_HITL. const toolMeta = await findToolJson(skillsBaseDir, toolName); + // Refuse before reading any model-supplied path. Disabled file_args must be + // inert, not merely absent from the advertised schema. + if (!policy.fileArgs && args.file_args !== undefined) { + return { + content: [{ type: "text", text: FILE_ARGS_DISABLED_TEXT }], + isError: true, + }; + } + // Resolve file_args up front so the approval prompt shows the COMPLETE input // (file-sourced values included, not just the inline `arguments`), and so an // unreadable file_args path fails before we prompt the user. @@ -340,17 +384,12 @@ export async function handleRunTool( typeof toolMeta?.requires_approval === "boolean" ? toolMeta.requires_approval : true; - // Cursor is gated by its OWN native run-tool approval, not our elicitation. - // We omit run_tool's readOnlyHint for Cursor (see runToolAnnotations), so - // Cursor prompts the user before it executes run_tool. Firing our elicitation - // on top would be a redundant second gate — and worse, Cursor 3.12.x silently - // drops server-initiated elicitations on the auto-run lane, hanging for the - // full HITL timeout. So skip our gate for Cursor and let its native prompt - // (already shown before this call) be the single approval. + // Cursor is deliberately not excepted: current Cursor builds can use the same + // local elicitation gate as other capable hosts. Older builds that drop the + // prompt fail closed, and the timeout response explains the upgrade path. if ( hitlEnabled && requiresApproval && - !isCursorClient(mcpServer) && mcpServer.getClientCapabilities()?.elicitation ) { // In bypassPermissions mode (`claude --dangerously-skip-permissions`) the @@ -362,16 +401,13 @@ export async function handleRunTool( // gate. Only bypassPermissions is skipped (deliberately narrow). const bypass = (await currentPermissionMode()) === "bypassPermissions"; if (!bypass) { - const message = await buildApprovalMessage( - mcpServer, - toolName, - resolvedArgs, - ); + const message = await buildApprovalMessage(toolName, resolvedArgs); const timeout = hitlTimeoutMs(); // Make a dummy empty request to burn JSON-RPC request id 0 primeElicitationCancellation(mcpServer); + const startedAt = Date.now(); try { const result = await mcpServer.elicitInput( { @@ -400,7 +436,13 @@ export async function handleRunTool( content: [ { type: "text", - text: `Action ${toolName} was not approved — the approval request failed (${detail}). The action was NOT executed. Ask the user to confirm, then retry.`, + text: elicitationFailureText( + mcpServer, + toolName, + detail, + Date.now() - startedAt, + timeout, + ), }, ], isError: true, @@ -441,23 +483,15 @@ export function buildRemoteArgs( * elicitation-capable client, our own approval prompt is the gate, so we mark * the tool `readOnlyHint` to suppress the client's native run-tool confirmation * and avoid a double prompt. Without HITL there is no gate of our own, so we - * leave annotations unset and let the client decide. - * - * TEMP (Cursor): Cursor 3.12.x silently drops the server-initiated elicitation - * for a `run_tool` marked `readOnlyHint` (it lands on the auto-run lane), so the - * approval banner never shows and the call hangs to the HITL timeout. For Cursor - * we therefore flip the whole strategy: do NOT advertise `readOnlyHint` (so - * Cursor shows its OWN native run-tool approval before executing), and skip our - * elicitation entirely (see handleRunTool) so Cursor's native prompt is the - * single gate. Claude Code is unaffected: it keeps `readOnlyHint` (its native - * prompt stays suppressed) and our elicitation remains its gate. + * leave annotations unset and let the client decide. Cursor follows the same + * path: if an older build drops the elicitation, execution remains blocked and + * elicitationFailureText explains the known pre-3.15 issue. */ export function runToolAnnotations( enableHitl: boolean, clientSupportsElicitation: boolean, - isCursor: boolean, ): Tool["annotations"] { - return enableHitl && clientSupportsElicitation && !isCursor + return enableHitl && clientSupportsElicitation ? { readOnlyHint: true } : undefined; } diff --git a/shared/glean/mcp/src/version.ts b/shared/glean/mcp/src/version.ts index 9546e73..0556f56 100644 --- a/shared/glean/mcp/src/version.ts +++ b/shared/glean/mcp/src/version.ts @@ -1,9 +1,30 @@ -// Read at runtime rather than injected at build time: `../package.json` hits the -// shipped manifest from both src/ and dist/, and that file has to exist anyway -// for Node to load the bundle as ESM. Kept in step with the root manifest by -// @release-it/bumper via the root release-it configuration. -import { readFileSync } from "node:fs"; - -export const PLUGIN_VERSION: string = JSON.parse( - readFileSync(new URL("../package.json", import.meta.url), "utf8"), -).version; +// The plugin's own version, injected by shared/glean/mcp/build.mjs. +// +// The bundled value is used for MCP serverInfo/clientInfo and remote capability +// policy. It must describe the code actually running, so shipped builds use a +// compiled literal rather than reading a mutable adjacent package.json. +declare const __GLEAN_PLUGIN_VERSION__: string | undefined; + +export type VersionSource = "build" | "unknown"; + +export interface ResolvedVersion { + version: string; + source: VersionSource; +} + +// `typeof` on an undeclared identifier is safe. Unbundled vitest/tsx runs take +// the honest unknown path; build.mjs makes this path unreachable in shipped +// output and asserts that the define landed. +const BUILD_VERSION: string | undefined = + typeof __GLEAN_PLUGIN_VERSION__ === "string" + ? __GLEAN_PLUGIN_VERSION__ + : undefined; + +export function pluginVersion(): ResolvedVersion { + if (BUILD_VERSION) return { version: BUILD_VERSION, source: "build" }; + return { version: "0.0.0", source: "unknown" }; +} + +export function pluginVersionString(): string { + return pluginVersion().version; +} diff --git a/shared/glean/mcp/tests/atomic-write.test.ts b/shared/glean/mcp/tests/atomic-write.test.ts new file mode 100644 index 0000000..ec91102 --- /dev/null +++ b/shared/glean/mcp/tests/atomic-write.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { writeFileAtomicSync } from "../src/atomic-write.js"; + +describe("writeFileAtomicSync", () => { + let dir: string; + let target: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "atomic-write-test-")); + target = path.join(dir, "store.json"); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it("writes the contents", () => { + writeFileAtomicSync(target, '{"a":1}', 0o600); + expect(JSON.parse(fs.readFileSync(target, "utf-8"))).toEqual({ a: 1 }); + }); + + it("replaces existing contents rather than appending", () => { + writeFileAtomicSync(target, '{"a":1}', 0o600); + writeFileAtomicSync(target, '{"b":2}', 0o600); + expect(JSON.parse(fs.readFileSync(target, "utf-8"))).toEqual({ b: 2 }); + }); + + // A temp file left in the data dir would be picked up by nothing, but it would + // accumulate one per crash and confuse anyone inspecting the directory. + it("leaves no temp file behind", () => { + writeFileAtomicSync(target, '{"a":1}', 0o600); + expect(fs.readdirSync(dir)).toEqual(["store.json"]); + }); + + it("applies the requested mode", () => { + writeFileAtomicSync(target, '{"a":1}', 0o600); + expect(fs.statSync(target).mode & 0o777).toBe(0o600); + }); + + // The point of the temp-and-rename dance: a write that fails must leave the previous + // contents readable, since every store treats an unparseable file as "no data" and + // would otherwise silently discard a cached policy. Forced by pre-creating the temp + // path as a directory, so writing it fails with EISDIR before the target is touched. + it("preserves the existing file when the write fails", () => { + writeFileAtomicSync(target, JSON.stringify({ keep: true }), 0o600); + + const tmpPath = path.join(dir, `.store.json.${process.pid}.tmp`); + fs.mkdirSync(tmpPath); + expect(() => + writeFileAtomicSync(target, JSON.stringify({ replaced: true }), 0o600), + ).toThrow(); + + expect(JSON.parse(fs.readFileSync(target, "utf-8"))).toEqual({ keep: true }); + }); + + // Separately: when the write succeeds but the swap fails, the temp must not be left + // behind — one would accumulate per failure in the user's data directory. + it("removes the temp file when the swap fails", () => { + const blocked = path.join(dir, "blocked.json"); + fs.mkdirSync(blocked); + + expect(() => + writeFileAtomicSync(blocked, JSON.stringify({ second: true }), 0o600), + ).toThrow(); + + expect(fs.readdirSync(dir).filter((f) => f.endsWith(".tmp"))).toEqual([]); + }); + + it("surfaces a failure instead of reporting success", () => { + expect(() => + writeFileAtomicSync( + path.join(dir, "missing-subdir", "store.json"), + "{}", + 0o600, + ), + ).toThrow(); + }); +}); diff --git a/shared/glean/mcp/tests/find-skills.test.ts b/shared/glean/mcp/tests/find-skills.test.ts index 4c9de80..31c8182 100644 --- a/shared/glean/mcp/tests/find-skills.test.ts +++ b/shared/glean/mcp/tests/find-skills.test.ts @@ -49,10 +49,10 @@ describe("handleFindSkills", () => { const result = await handleFindSkills(mockClient, tmpDir, {}); expect(mockClient.callTool).toHaveBeenCalledWith( - { + expect.objectContaining({ name: "find_skills", arguments: {}, - }, + }), undefined, expect.objectContaining({ timeout: expect.any(Number) }), ); @@ -75,10 +75,10 @@ describe("handleFindSkills", () => { }); expect(mockClient.callTool).toHaveBeenCalledWith( - { + expect.objectContaining({ name: "find_skills", arguments: { queries: ["create a calendar event"] }, - }, + }), undefined, expect.objectContaining({ timeout: expect.any(Number) }), ); @@ -92,10 +92,10 @@ describe("handleFindSkills", () => { }); expect(mockClient.callTool).toHaveBeenCalledWith( - { + expect.objectContaining({ name: "find_skills", arguments: { queries: ["search emails", "create calendar event"] }, - }, + }), undefined, expect.objectContaining({ timeout: expect.any(Number) }), ); diff --git a/shared/glean/mcp/tests/policy-enforce.test.ts b/shared/glean/mcp/tests/policy-enforce.test.ts new file mode 100644 index 0000000..c5dd7a0 --- /dev/null +++ b/shared/glean/mcp/tests/policy-enforce.test.ts @@ -0,0 +1,320 @@ +import { describe, expect, it } from "vitest"; +import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import { + advertisedTools, + policyRefusal, + withoutFileArgs, +} from "../src/policy/enforce.js"; +import { evaluate } from "../src/policy/evaluate.js"; +import type { Decision } from "../src/policy/types.js"; + +const allSupported = { toolPromotion: true, metaTools: true, fileArgs: true }; + +// Version rules cannot be exercised through evaluate() under vitest -- the build constant +// is absent, so versionSource is "unknown" and version policy is never enforced. The +// gates are therefore driven from a Decision directly, which is the reason enforce.ts is +// a pure module in the first place. +function decision(over: Partial = {}): Decision { + return { + deactivated: false, + versionState: "unenforced", + features: { ...allSupported }, + showUpgrade: false, + reasons: [], + ...over, + }; +} + +const setupTool: Tool = { + name: "setup", + inputSchema: { type: "object", properties: {} }, +}; +const findSkillsTool: Tool = { + name: "find_skills_and_tools", + inputSchema: { type: "object", properties: { queries: { type: "array" } } }, +}; + +function makeRunTool(): Tool { + return { + name: "run_tool", + annotations: { readOnlyHint: true }, + inputSchema: { + type: "object", + properties: { + server_id: { type: "string" }, + tool_name: { type: "string" }, + arguments: { type: "object" }, + file_args: { type: "object" }, + }, + required: ["server_id", "tool_name"], + }, + }; +} + +const promoted: Tool[] = [ + { name: "search", inputSchema: { type: "object", properties: {} } }, + { name: "chat", inputSchema: { type: "object", properties: {} } }, +]; +const promotedNames: ReadonlySet = new Set(["search", "chat"]); + +function advertise(d: Decision) { + return advertisedTools({ + decision: d, + setupTool, + findSkillsTool, + runTool: makeRunTool(), + promoted, + }); +} + +function names(d: Decision): string[] { + return advertise(d).tools.map((t) => t.name); +} + +function refusal(name: string, d: Decision) { + return policyRefusal({ name, decision: d, promoted: promotedNames }); +} + +function fileArgsAdvertised(d: Decision): boolean { + const tool = advertise(d).tools.find((t) => t.name === "run_tool"); + const props = (tool?.inputSchema as { properties?: Record }) + ?.properties; + return !!props && "file_args" in props; +} + +// The regression that matters most: production returns no policy, so every install today +// resolves to this decision. The surface must be exactly what shipped before enforcement. +describe("no policy", () => { + it("advertises the pre-policy surface, in the pre-policy order", () => { + const d = evaluate({ + pluginVersion: "0.0.0", + versionSource: "unknown", + supportedFeatures: allSupported, + policy: undefined, + }); + expect(names(d)).toEqual([ + "find_skills_and_tools", + "run_tool", + "setup", + "search", + "chat", + ]); + expect(advertise(d).withheld).toEqual([]); + expect(fileArgsAdvertised(d)).toBe(true); + }); + + it("refuses nothing", () => { + const d = evaluate({ + pluginVersion: "0.0.0", + versionSource: "unknown", + supportedFeatures: allSupported, + policy: undefined, + }); + for (const name of ["setup", "find_skills_and_tools", "run_tool", "search", "chat"]) { + expect(refusal(name, d)).toBeUndefined(); + } + }); +}); + +describe("deactivated", () => { + const d = decision({ + deactivated: true, + versionState: "blocked", + // evaluate() reports every feature false when deactivated; the gates must not read + // that as "the features were individually disabled". + features: { toolPromotion: false, metaTools: false, fileArgs: false }, + }); + + it("advertises setup and nothing else", () => { + expect(names(d)).toEqual(["setup"]); + expect(advertise(d).withheld.sort()).toEqual([ + "chat", + "find_skills_and_tools", + "run_tool", + "search", + ]); + }); + + it("keeps setup callable -- it is the recovery path", () => { + expect(refusal("setup", d)).toBeUndefined(); + }); + + it("refuses everything else", () => { + for (const name of ["find_skills_and_tools", "run_tool", "search", "chat"]) { + expect(refusal(name, d)?.isError).toBe(true); + } + }); + + // The ordering guard: a deactivated plugin's problem is its version, and the only + // remedy is an upgrade. Answering "metaTools is disabled" would send the model and the + // user after the wrong thing. + it("blames the version, not the features", () => { + const text = (refusal("find_skills_and_tools", d)!.content[0] as { text: string }).text; + expect(text).toContain("[POLICY_DEACTIVATED]"); + expect(text).toContain("Upgrade the Glean plugin"); + expect(text).not.toContain("metaTools"); + }); +}); + +// upgradeRecommendation.message does double duty per the design: the soft recommendation +// AND the deactivation remedy, since an upgrade is the fix either way. Before this it was +// accepted by the validator and then dropped, so a remote could write user-facing text +// into the designated field and see nothing. +describe("the remote's upgrade text", () => { + it("is used as the remedy in a deactivation refusal", () => { + const d = decision({ + deactivated: true, + versionState: "below-minimum", + features: { toolPromotion: false, metaTools: false, fileArgs: false }, + upgradeMessage: "Run `/plugin update glean` and restart Claude Code.", + }); + + const text = (refusal("find_skills_and_tools", d)!.content[0] as { text: string }).text; + expect(text).toContain("Run `/plugin update glean`"); + }); + + it("falls back to generic wording when the remote supplied none", () => { + const d = decision({ + deactivated: true, + versionState: "blocked", + features: { toolPromotion: false, metaTools: false, fileArgs: false }, + }); + + const text = (refusal("run_tool", d)!.content[0] as { text: string }).text; + expect(text).toContain("Upgrade the Glean plugin"); + }); + + // A soft recommendation must not withdraw anything -- it is advice, not a gate. + it("changes nothing about the surface or the refusals", () => { + const d = decision({ + versionState: "outdated-supported", + showUpgrade: true, + upgradeMessage: "1.4.0 is out.", + }); + + expect(names(d)).toEqual([ + "find_skills_and_tools", + "run_tool", + "setup", + "search", + "chat", + ]); + expect(refusal("search", d)).toBeUndefined(); + expect(refusal("run_tool", d)).toBeUndefined(); + }); +}); + +describe("metaTools disabled", () => { + const d = decision({ features: { ...allSupported, metaTools: false } }); + + it("withdraws both meta tools but keeps setup and promoted tools", () => { + expect(names(d)).toEqual(["setup", "search", "chat"]); + expect(advertise(d).withheld).toEqual(["find_skills_and_tools", "run_tool"]); + }); + + it("refuses both by name, and nothing else", () => { + expect(refusal("find_skills_and_tools", d)?.isError).toBe(true); + expect(refusal("run_tool", d)?.isError).toBe(true); + expect(refusal("search", d)).toBeUndefined(); + expect(refusal("setup", d)).toBeUndefined(); + }); +}); + +describe("toolPromotion disabled", () => { + const d = decision({ features: { ...allSupported, toolPromotion: false } }); + + it("promotes none, and keeps the meta tools", () => { + expect(names(d)).toEqual(["find_skills_and_tools", "run_tool", "setup"]); + expect(advertise(d).withheld).toEqual(["search", "chat"]); + }); + + // This is the case that motivated call-time enforcement at all: a host holding a stale + // list still shows `search` to the model, and without the refusal the call would be + // forwarded happily. + it("refuses every promoted name even though none are advertised", () => { + expect(refusal("search", d)?.isError).toBe(true); + expect(refusal("chat", d)?.isError).toBe(true); + }); + + it("does not refuse a name outside the promoted set", () => { + // Left to the handler's existing "Unknown tool" branch. + expect(refusal("something_else", d)).toBeUndefined(); + }); +}); + +describe("fileArgs disabled", () => { + const d = decision({ features: { ...allSupported, fileArgs: false } }); + + it("keeps run_tool but drops file_args from its schema", () => { + expect(names(d)).toEqual([ + "find_skills_and_tools", + "run_tool", + "setup", + "search", + "chat", + ]); + expect(fileArgsAdvertised(d)).toBe(false); + }); + + it("leaves the rest of the schema intact", () => { + const tool = advertise(d).tools.find((t) => t.name === "run_tool")!; + const schema = tool.inputSchema as { + properties: Record; + required: string[]; + }; + expect(Object.keys(schema.properties).sort()).toEqual([ + "arguments", + "server_id", + "tool_name", + ]); + expect(schema.required).toEqual(["server_id", "tool_name"]); + expect(tool.annotations).toEqual({ readOnlyHint: true }); + }); + + // run_tool itself stays callable -- only a call that passes file_args is rejected, and + // that check lives beside the code that reads the files. + it("does not refuse run_tool at the funnel", () => { + expect(refusal("run_tool", d)).toBeUndefined(); + }); +}); + +// The shared-schema hazard: index.ts clones RUN_TOOL_TOOL with a shallow spread, so +// inputSchema.properties is the module const's own object. A delete would drop file_args +// for the life of the process and survive a policy flip back to enabled. +describe("withoutFileArgs", () => { + it("does not mutate the tool it is given", () => { + const base = makeRunTool(); + withoutFileArgs(base); + const props = (base.inputSchema as { properties: Record }) + .properties; + expect("file_args" in props).toBe(true); + }); + + it("survives a flip back to enabled on the same base tool", () => { + const base = makeRunTool(); + const off = advertisedTools({ + decision: decision({ features: { ...allSupported, fileArgs: false } }), + setupTool, + findSkillsTool, + runTool: base, + promoted, + }); + const on = advertisedTools({ + decision: decision(), + setupTool, + findSkillsTool, + runTool: base, + promoted, + }); + const propsOf = (a: typeof off) => { + const t = a.tools.find((x) => x.name === "run_tool")!; + return (t.inputSchema as { properties: Record }).properties; + }; + expect("file_args" in propsOf(off)).toBe(false); + expect("file_args" in propsOf(on)).toBe(true); + }); + + it("is a no-op for a tool that never had file_args", () => { + expect(withoutFileArgs(findSkillsTool)).toBe(findSkillsTool); + }); +}); diff --git a/shared/glean/mcp/tests/policy-session.test.ts b/shared/glean/mcp/tests/policy-session.test.ts new file mode 100644 index 0000000..78aaa31 --- /dev/null +++ b/shared/glean/mcp/tests/policy-session.test.ts @@ -0,0 +1,439 @@ +import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { CAPABILITY_POLICY_KEY } from "../src/policy/key.js"; + +// session.ts holds module state, so every test gets a fresh module graph and its own +// PLUGIN_DATA_DIR. cachePath() reads the env per call, so stubbing it is enough to +// isolate the cache file. +async function freshSession(dir: string) { + vi.resetModules(); + vi.stubEnv("PLUGIN_DATA_DIR", dir); + const session = await import("../src/policy/session.js"); + const cache = await import("../src/policy/cache.js"); + return { session, cache }; +} + +const URL_A = "https://a-be.glean.com/mcp/gateway/proxy"; + +interface FakeServer { + getClientVersion: () => { name: string; version: string }; + getClientCapabilities: () => Record; + sendToolListChanged: ReturnType; +} + +function fakeServer(): FakeServer { + return { + getClientVersion: () => ({ name: "claude-code", version: "1.2.3" }), + getClientCapabilities: () => ({ elicitation: {} }), + sendToolListChanged: vi.fn().mockResolvedValue(undefined), + }; +} + +function resultWith(policy: unknown) { + return { tools: [], _meta: { [CAPABILITY_POLICY_KEY]: policy } }; +} + +describe("decisionInForce", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "policy-session-")); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + vi.unstubAllEnvs(); + }); + + it("enables everything supported when there is no configured remote", async () => { + const { session } = await freshSession(dir); + session.initPolicySession(fakeServer() as never, () => {}); + + const d = session.decisionInForce(); + + expect(d.deactivated).toBe(false); + expect(d.versionState).toBe("unenforced"); + expect(Object.values(d.features).every((v) => v === true)).toBe(true); + }); + + it("enables everything supported when the URL has no cached policy", async () => { + const { session } = await freshSession(dir); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + expect(session.decisionInForce().features.metaTools).toBe(true); + }); + + // The fresh-process regression the design calls out by name: a cached metaTools:false, + // or a cached deactivation, must not be undone just because this process has not talked + // to the remote yet. tools/list returns before negotiating on three of its five paths. + it("applies a cached policy before any exchange happens", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + expect(session.decisionInForce().features.metaTools).toBe(false); + }); + + it("reads the cache once per process, not per call", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + expect(session.decisionInForce().features.metaTools).toBe(false); + + // Rewrite the file behind its back. A memoized decision ignores it; a per-call read + // would pick it up. + cache.savePolicy(URL_A, { features: { metaTools: { enabled: true } } }); + + expect(session.decisionInForce().features.metaTools).toBe(false); + }); + + it("logs that the decision came from cache, so the seam is visible", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + const labels: string[] = []; + session.initPolicySession(fakeServer() as never, (l) => labels.push(l)); + session.setPolicyServerUrl(URL_A); + + session.decisionInForce(); + + expect(labels).toContain("policy.seeded-from-cache"); + }); + + it("does not claim a cache seed when there was no cached policy", async () => { + const { session } = await freshSession(dir); + const labels: string[] = []; + session.initPolicySession(fakeServer() as never, (l) => labels.push(l)); + session.setPolicyServerUrl(URL_A); + + session.decisionInForce(); + + expect(labels).not.toContain("policy.seeded-from-cache"); + }); + + // A response carrying no policy is silence, not revocation. The compatibility path -- + // everything enabled, no version rule -- is for a remote that does not implement + // negotiation at all, and the cached policy is the evidence of whether this one does. + it("keeps a cached restriction when a later response carries no policy", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + expect(session.decisionInForce().features.metaTools).toBe(false); + + session.recordPolicyFromResult({ tools: [] }, "tools/list"); + + expect(session.decisionInForce().features.metaTools).toBe(false); + }); + + // ...and the remote can still lift it, by saying so rather than by going quiet. + it("lets an explicit re-enable lift a cached restriction", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + expect(session.decisionInForce().features.metaTools).toBe(false); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: true } } }), + "tools/list", + ); + + expect(session.decisionInForce().features.metaTools).toBe(true); + }); + + // Two levels of omission, deliberately different: + // + // a feature omitted INSIDE a policy -> no opinion, so enabled + // the policy object omitted entirely -> silence, so the cache stands + // + // The first depends on savePolicy REPLACING the cached entry rather than merging into + // it, so every policy is evaluated as a complete statement of intent. Turning that into + // a merge would make restrictions sticky and leave a remote unable to lift one at all. + it("lets a later policy lift a restriction by omitting the feature", async () => { + const { session } = await freshSession(dir); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: false } } }), + "tools/list", + ); + expect(session.decisionInForce().features.metaTools).toBe(false); + + // Names only toolPromotion; says nothing at all about metaTools. + session.recordPolicyFromResult( + resultWith({ features: { toolPromotion: { enabled: false } } }), + "tools/list", + ); + + const f = session.decisionInForce().features; + expect(f.metaTools).toBe(true); + expect(f.toolPromotion).toBe(false); + + // And the same must hold once the CACHE is what answers, not the incoming policy. + // evaluate() runs on the response's policy, so the two assertions above pass even if + // savePolicy merged rather than replaced — the merge would only surface here, or on a + // later process start. This is the assertion that actually pins replace-not-merge. + session.recordPolicyFromResult({ content: [] }, "tools/call(search)"); + expect(session.decisionInForce().features.metaTools).toBe(true); + }); + + it("takes the compatibility path when no policy was ever received", async () => { + const { session } = await freshSession(dir); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + session.recordPolicyFromResult({ tools: [] }, "tools/list"); + + const d = session.decisionInForce(); + expect(d.features.metaTools).toBe(true); + expect(d.versionState).toBe("unenforced"); + }); + + // The regression this replaced a wrong assumption for: a remote that attaches policy to + // tools/list but not to tools/call is a plausible split, since a list is answered once + // while calls are the hot path. Reading omission as revocation made every call clear the + // policy and every following list restore it -- and each clear changed the surface, so + // the host re-fetched on every tool call and the advertised list visibly flickered. + it("does not flip-flop when the remote only attaches policy to tools/list", async () => { + const { session } = await freshSession(dir); + const server = fakeServer(); + session.initPolicySession(server as never, () => {}); + session.setPolicyServerUrl(URL_A); + + const policy = { features: { metaTools: { enabled: false } } }; + const seen: boolean[] = []; + + for (const label of ["tools/list", "tools/call(search)", "tools/list", "tools/call(chat)"]) { + session.recordPolicyFromResult( + label === "tools/list" ? resultWith(policy) : { content: [] }, + label, + ); + seen.push(session.decisionInForce().features.metaTools); + } + + expect(seen).toEqual([false, false, false, false]); + expect(server.sendToolListChanged).not.toHaveBeenCalled(); + }); + + it("keeps the cached policy on disk when a response carries none", async () => { + const { session, cache } = await freshSession(dir); + cache.savePolicy(URL_A, { features: { metaTools: { enabled: false } } }); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + + session.recordPolicyFromResult({ tools: [] }, "tools/list"); + + expect(cache.loadCachedPolicy(URL_A)).toEqual({ + features: { metaTools: { enabled: false } }, + }); + }); +}); + +describe("tools/list_changed notification", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "policy-notify-")); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + vi.unstubAllEnvs(); + }); + + async function armed() { + const { session } = await freshSession(dir); + const server = fakeServer(); + session.initPolicySession(server as never, () => {}); + session.setPolicyServerUrl(URL_A); + // Establish a first decision, which must not itself notify. + session.recordPolicyFromResult({ tools: [] }, "tools/call(search)"); + return { session, server }; + } + + it("does not notify on the first decision of a process", async () => { + const { session } = await freshSession(dir); + const server = fakeServer(); + session.initPolicySession(server as never, () => {}); + session.setPolicyServerUrl(URL_A); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: false } } }), + "tools/call(run_tool)", + ); + + expect(server.sendToolListChanged).not.toHaveBeenCalled(); + }); + + it("notifies when a tools/call response changes the reachable surface", async () => { + const { session, server } = await armed(); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: false } } }), + "tools/call(search)", + ); + + expect(server.sendToolListChanged).toHaveBeenCalledTimes(1); + }); + + // The response to a tools/list IS the update -- the host asked and is about to receive + // the filtered surface. Notifying would make it ask again, and notify -> list -> + // resolve -> notify is a cycle. + it("never notifies from the tools/list path, even on a change", async () => { + const { session, server } = await armed(); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: false } } }), + "tools/list", + ); + + expect(server.sendToolListChanged).not.toHaveBeenCalled(); + }); + + it("does not notify when only advisory fields differ", async () => { + const { session, server } = await armed(); + + // Same features and deactivated flag; only the message changes. + session.recordPolicyFromResult( + resultWith({ message: "a wholly different notice" }), + "tools/call(search)", + ); + + expect(server.sendToolListChanged).not.toHaveBeenCalled(); + }); + + it("does not notify when the same policy arrives twice", async () => { + const { session, server } = await armed(); + const policy = { features: { metaTools: { enabled: false } } }; + + session.recordPolicyFromResult(resultWith(policy), "tools/call(search)"); + session.recordPolicyFromResult(resultWith(policy), "tools/call(search)"); + + expect(server.sendToolListChanged).toHaveBeenCalledTimes(1); + }); +}); + +// The recommendation is computed on every exchange but has exactly one surface: the setup +// tool's output. Without these, `showUpgrade` and the remote's upgrade text are values the +// remote sets and no user ever sees -- which is what they were before this change. +describe("policySummary", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "policy-summary-")); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + vi.unstubAllEnvs(); + }); + + async function summaryAfter(policy: unknown) { + const { session } = await freshSession(dir); + session.initPolicySession(fakeServer() as never, () => {}); + session.setPolicyServerUrl(URL_A); + session.recordPolicyFromResult(resultWith(policy), "tools/call(search)"); + return session.policySummary().join("\n"); + } + + // Deliberately not asserting the upgrade or deactivation lines here. Both require + // versionState to be version-derived, and the build constant is absent under vitest, so + // pluginVersion() is {0.0.0, unknown} and no version rule ever fires through this + // module. Their inputs are covered where they ARE reachable: evaluate() carrying + // upgradeMessage (policy.test.ts) and the deactivation refusal consuming it + // (policy-enforce.test.ts). The remaining uncovered hop is this function's string + // assembly for those two branches. + it("reports the negotiated context and the resolved policy", async () => { + const summary = await summaryAfter({ features: { metaTools: { enabled: false } } }); + expect(summary).toContain("Plugin version: 0.0.0 (source: unknown)"); + expect(summary).toContain("Host: claude-code"); + expect(summary).toContain("version unenforced"); + expect(summary).toContain('"metaTools":false'); + }); + + it("shows a session message as a notice", async () => { + const summary = await summaryAfter({ message: "Maintenance window at 2am UTC." }); + expect(summary).toContain("Notice: Maintenance window at 2am UTC."); + }); + + it("says nothing about upgrades when the remote does not ask", async () => { + const summary = await summaryAfter({ features: { metaTools: { enabled: true } } }); + expect(summary).not.toContain("Upgrade available"); + expect(summary).not.toContain("Deactivated:"); + }); +}); + +// dailyCap/weeklyCap are tolerated rather than rejected -- the design has the remote send +// them for forward compatibility and only `show` needs honouring in v0. Tolerating them +// silently is the problem: a remote setting dailyCap: 1 and seeing the recommendation on +// every setup has no way to learn why. So it is stated once, in the log. +describe("unenforced display caps", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "policy-caps-")); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + vi.unstubAllEnvs(); + }); + + async function record(policy: unknown) { + const { session } = await freshSession(dir); + const labels: string[] = []; + session.initPolicySession(fakeServer() as never, (l) => labels.push(l)); + session.setPolicyServerUrl(URL_A); + session.recordPolicyFromResult(resultWith(policy), "tools/call(search)"); + return { session, labels }; + } + + it("says so when the remote sets a cap", async () => { + const { labels } = await record({ + plugin: { upgradeRecommendation: { show: true, dailyCap: 1 } }, + }); + expect(labels).toContain("policy.caps-not-enforced"); + }); + + it("still accepts the policy rather than calling it malformed", async () => { + const { labels } = await record({ + plugin: { upgradeRecommendation: { show: true, weeklyCap: 3 } }, + }); + // Tolerated, not rejected: no malformed, and not reported as an unknown key either. + expect(labels).not.toContain("policy.malformed"); + expect(labels).not.toContain("policy.unknown-keys"); + }); + + it("says nothing when the remote sets no cap", async () => { + const { labels } = await record({ + plugin: { upgradeRecommendation: { show: true } }, + }); + expect(labels).not.toContain("policy.caps-not-enforced"); + }); + + // Policy rides every remote response, so an unconditional log here would be one line + // per tool call for the life of the process. + it("says it once per process, not once per exchange", async () => { + const { session, labels } = await record({ + plugin: { upgradeRecommendation: { show: true, dailyCap: 1 } }, + }); + + session.recordPolicyFromResult( + resultWith({ plugin: { upgradeRecommendation: { show: true, dailyCap: 1 } } }), + "tools/call(chat)", + ); + + expect(labels.filter((l) => l === "policy.caps-not-enforced")).toHaveLength(1); + }); +}); + diff --git a/shared/glean/mcp/tests/policy.test.ts b/shared/glean/mcp/tests/policy.test.ts new file mode 100644 index 0000000..9b611bf --- /dev/null +++ b/shared/glean/mcp/tests/policy.test.ts @@ -0,0 +1,386 @@ +import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { evaluate } from "../src/policy/evaluate.js"; +import { classifyResult, validatePolicy } from "../src/policy/negotiate.js"; +import { CAPABILITY_POLICY_KEY } from "../src/policy/key.js"; +import { loadCachedPolicy, savePolicy } from "../src/policy/cache.js"; + +const allSupported = { + toolPromotion: true, + metaTools: true, + fileArgs: true, +}; + +describe("feature gating", () => { + it("disables exactly the feature the remote turned off", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { features: { fileArgs: { enabled: false } } }, + }); + expect(d.features).toEqual({ + toolPromotion: true, + metaTools: true, + fileArgs: false, + }); + expect(d.deactivated).toBe(false); + }); + + it("cannot enable a feature this build does not support", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: { ...allSupported, fileArgs: false }, + policy: { features: { fileArgs: { enabled: true } } }, + }); + expect(d.features.fileArgs).toBe(false); + }); + + it("treats an omitted feature as no opinion, not as disabled", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { features: {} }, + }); + expect(d.features).toEqual(allSupported); + }); +}); + +describe("version gating", () => { + it("deactivates below the minimum supported version", () => { + const d = evaluate({ + pluginVersion: "0.2.43", + versionSource: "build", + supportedFeatures: allSupported, + policy: { plugin: { minimumSupportedVersion: "9.0.0" } }, + }); + expect(d.deactivated).toBe(true); + expect(d.versionState).toBe("below-minimum"); + // Every feature reads false so no caller can act on a stale enablement. + expect(Object.values(d.features).every((v) => v === false)).toBe(true); + }); + + it("deactivates an explicitly blocked version while neighbours stay usable", () => { + const policy = { plugin: { blockedVersions: ["1.2.0"] } }; + const blocked = evaluate({ + pluginVersion: "1.2.0", + versionSource: "build", + supportedFeatures: allSupported, + policy, + }); + const neighbour = evaluate({ + pluginVersion: "1.3.0", + versionSource: "build", + supportedFeatures: allSupported, + policy, + }); + expect(blocked.versionState).toBe("blocked"); + expect(neighbour.deactivated).toBe(false); + }); + + it("NEVER enforces version policy when the version source is unknown", () => { + const d = evaluate({ + pluginVersion: "0.0.0", + versionSource: "unknown", + supportedFeatures: allSupported, + policy: { + plugin: { + minimumSupportedVersion: "9.0.0", + blockedVersions: ["0.0.0"], + }, + }, + }); + expect(d.deactivated).toBe(false); + expect(d.versionState).toBe("unenforced"); + }); + + it("shows an upgrade notice only when outdated AND the remote asks", () => { + const base = { + pluginVersion: "1.0.0", + versionSource: "build" as const, + supportedFeatures: allSupported, + }; + expect( + evaluate({ + ...base, + policy: { + plugin: { + latestVersion: "2.0.0", + upgradeRecommendation: { show: true }, + }, + }, + }).showUpgrade, + ).toBe(true); + expect( + evaluate({ + ...base, + policy: { + plugin: { + latestVersion: "2.0.0", + upgradeRecommendation: { show: false }, + }, + }, + }).showUpgrade, + ).toBe(false); + expect( + evaluate({ + ...base, + policy: { + plugin: { + latestVersion: "1.0.0", + upgradeRecommendation: { show: true }, + }, + }, + }).showUpgrade, + ).toBe(false); + }); +}); + +// The remote's upgrade text has one job in two places: the soft recommendation and the +// remedy in a deactivation refusal. It was accepted by validatePolicy and then dropped +// before reaching the Decision, so a remote could write user-facing text into the field +// the design designates for it and have it vanish. +describe("upgrade text", () => { + it("is carried out with a soft recommendation", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { + plugin: { + latestVersion: "2.0.0", + upgradeRecommendation: { show: true, message: "2.0.0 adds Codex support." }, + }, + }, + }); + expect(d.showUpgrade).toBe(true); + expect(d.upgradeMessage).toBe("2.0.0 adds Codex support."); + }); + + it("is carried out when version policy deactivates the plugin", () => { + const d = evaluate({ + pluginVersion: "0.1.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { + plugin: { + minimumSupportedVersion: "1.0.0", + upgradeRecommendation: { message: "Upgrade to 1.x; 0.x is end of life." }, + }, + }, + }); + expect(d.deactivated).toBe(true); + expect(d.upgradeMessage).toBe("Upgrade to 1.x; 0.x is end of life."); + }); + + // Distinct fields answering distinct questions: `message` is about this session, + // `upgradeMessage` about the installed artifact. Conflating them would mean a + // maintenance notice could be shown as upgrade instructions. + it("stays separate from the session message", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { + message: "Maintenance window at 2am UTC.", + plugin: { + latestVersion: "2.0.0", + upgradeRecommendation: { show: true, message: "2.0.0 is out." }, + }, + }, + }); + expect(d.message).toBe("Maintenance window at 2am UTC."); + expect(d.upgradeMessage).toBe("2.0.0 is out."); + }); + + it("is absent when the remote sends no upgrade text", () => { + const d = evaluate({ + pluginVersion: "1.0.0", + versionSource: "build", + supportedFeatures: allSupported, + policy: { + plugin: { latestVersion: "2.0.0", upgradeRecommendation: { show: true } }, + }, + }); + expect(d.showUpgrade).toBe(true); + expect(d.upgradeMessage).toBeUndefined(); + }); +}); + +describe("no-policy compatibility path", () => { + it("enables everything supported and applies no version rule", () => { + const d = evaluate({ + pluginVersion: "0.0.1", + versionSource: "build", + supportedFeatures: allSupported, + policy: undefined, + }); + expect(d.features).toEqual(allSupported); + expect(d.deactivated).toBe(false); + expect(d.versionState).toBe("unenforced"); + }); +}); + +describe("outcome classification", () => { + it("reads a valid policy off result._meta", () => { + const outcome = classifyResult({ + tools: [], + _meta: { + [CAPABILITY_POLICY_KEY]: { features: { metaTools: { enabled: false } } }, + }, + }); + expect(outcome.kind).toBe("policy"); + }); + + it("distinguishes a successful response with NO policy object", () => { + expect(classifyResult({ tools: [] }).kind).toBe("no-policy"); + expect(classifyResult({ tools: [], _meta: {} }).kind).toBe("no-policy"); + }); + + it("rejects a malformed policy instead of acting on it", () => { + const outcome = classifyResult({ + _meta: { + [CAPABILITY_POLICY_KEY]: { + plugin: { latestVersion: 123 }, + features: "everything-on", + }, + }, + }); + expect(outcome.kind).toBe("malformed"); + }); + + it("accepts unknown extra fields so a newer remote is not called malformed", () => { + const v = validatePolicy({ somethingNew: { nested: true }, features: {} }); + expect(v.ok).toBe(true); + }); +}); + +// Leniency is required for forward compatibility, but silent leniency hides a +// renamed or misspelled field: an unrecognized key's type is never checked, so a bad +// value in one is indistinguishable from its absence. The keys are therefore still +// ignored for evaluation AND reported, so the ignoring is greppable. +describe("unknown-key reporting", () => { + it("still accepts the policy, and names what it ignored", () => { + const v = validatePolicy({ + somethingNew: true, + plugin: { latestVersion: "1.0.0", futureRule: 1 }, + features: { toolPromotion: { enabled: true }, notAFeature: {} }, + }); + expect(v.ok).toBe(true); + if (!v.ok) return; + expect(v.unknownKeys.sort()).toEqual([ + "features.notAFeature", + "plugin.futureRule", + "somethingNew", + ]); + }); + + it("reports the exact case the response rename created", () => { + // A key that WAS valid before the response was nested. It is accepted, because + // rejecting unrecognized keys would break forward compatibility -- but its bad + // value would otherwise pass in total silence, which is what the report fixes. + const v = validatePolicy({ latestPluginVersion: 123 }); + expect(v.ok).toBe(true); + if (!v.ok) return; + expect(v.unknownKeys).toEqual(["latestPluginVersion"]); + }); + + it("reports nothing when the policy uses only known keys", () => { + const v = validatePolicy({ + plugin: { + latestVersion: "1.0.0", + minimumSupportedVersion: "0.1.0", + blockedVersions: [], + upgradeRecommendation: { show: true, dailyCap: 1, weeklyCap: 3, message: "x" }, + }, + features: { metaTools: { enabled: false } }, + message: "x", + }); + expect(v.ok).toBe(true); + if (!v.ok) return; + expect(v.unknownKeys).toEqual([]); + }); + + // hitl is defined by the design contract but deliberately not declared by this build, + // because the stateless remote cannot take approval over yet. It therefore travels the + // same path as any feature a policy names ahead of the plugin: accepted so a newer + // remote is not called malformed, reported so the ignoring is greppable, and inert. + // If a later build declares hitl, this test is the one that should start failing. + it("treats hitl as an unknown feature, since this build does not declare it", () => { + const v = validatePolicy({ features: { hitl: { enabled: false } } }); + expect(v.ok).toBe(true); + if (!v.ok) return; + expect(v.unknownKeys).toEqual(["features.hitl"]); + }); +}); + +describe("policy cache", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "policy-cache-test-")); + vi.stubEnv("PLUGIN_DATA_DIR", dir); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + vi.unstubAllEnvs(); + }); + + it("keys by remote URL so switching instances cannot cross-contaminate", () => { + savePolicy("https://a.glean.com", { features: { metaTools: { enabled: false } } }); + savePolicy("https://b.glean.com", { features: { metaTools: { enabled: true } } }); + + expect(loadCachedPolicy("https://a.glean.com")).toEqual({ + features: { metaTools: { enabled: false } }, + }); + expect(loadCachedPolicy("https://b.glean.com")).toEqual({ + features: { metaTools: { enabled: true } }, + }); + }); + + // Reset clears the server URL, credentials and tools cache, but NOT this. A cached + // policy can carry a deactivation or a version block, so a user-invokable way to + // discard it would be a way to shed one — and the remote may be unreachable + // afterwards, with nothing to re-fetch from. Hence no clear function at all; a + // future one would reintroduce exactly that. + it("exposes no way to discard a cached policy", async () => { + const mod = await import("../src/policy/cache.js"); + expect(Object.keys(mod).sort()).toEqual(["loadCachedPolicy", "savePolicy"]); + }); + + // Design: "A response without a policy object does not update or erase the + // corresponding cache entry." + it("keeps the cached policy when a later response carries none", () => { + savePolicy("https://a.glean.com", { features: { metaTools: { enabled: false } } }); + + // A no-policy round writes nothing, so the entry is untouched. + expect(loadCachedPolicy("https://a.glean.com")).toEqual({ + features: { metaTools: { enabled: false } }, + }); + }); + + // tools/list belongs to remote-tools-cache-store.ts. Caching it here too would + // shadow a live subsystem and let the two disagree about which surface goes with + // which policy. Asserted against the file, since that is where a stray field + // would actually appear. + it("writes policy only, never a tools list", () => { + savePolicy("https://a.glean.com", { features: { metaTools: { enabled: false } } }); + + const onDisk = JSON.parse( + fs.readFileSync(path.join(dir, "policy-cache.json"), "utf-8"), + ); + expect(Object.keys(onDisk["https://a.glean.com"]).sort()).toEqual([ + "policy", + "updatedAt", + ]); + }); + + it("returns undefined for an unknown URL rather than throwing", () => { + expect(loadCachedPolicy("https://never-seen.glean.com")).toBeUndefined(); + }); +}); diff --git a/shared/glean/mcp/tests/remote-client.test.ts b/shared/glean/mcp/tests/remote-client.test.ts index 2975974..373bd42 100644 --- a/shared/glean/mcp/tests/remote-client.test.ts +++ b/shared/glean/mcp/tests/remote-client.test.ts @@ -115,10 +115,13 @@ describe("callRemoteTool", () => { const result = await callRemoteTool(fakeClient, "some_tool", { a: 1 }); expect(calls).toHaveLength(1); - expect(calls[0].params).toEqual({ + expect(calls[0].params).toMatchObject({ name: "some_tool", arguments: { a: 1 }, }); + expect( + (calls[0].params as { _meta?: Record })._meta, + ).toHaveProperty("com.glean.mcp/capabilityPolicy"); expect(calls[0].options).toEqual({ timeout: 12345 }); expect(result.content).toEqual([{ type: "text", text: "ok" }]); }); diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index df0585b..e1ef2a4 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -8,11 +8,18 @@ import { FileArgsError, handleRunTool, runToolAnnotations, + elicitationFailureText, } from "../src/tools/run-tool.js"; import { buildCompactArgs, formatArgumentsForFile, } from "../src/tools/approval-args.js"; +import type { RunToolPolicy } from "../src/tools/run-tool.js"; + +// The decision every install resolves to today, since production returns no policy. +// Passed explicitly at each call site rather than defaulted, so a case that means to +// exercise a disabled feature has to say so. +const ALL_ON: RunToolPolicy = { fileArgs: true }; describe("resolveFileArgs", () => { let tmpDir: string; @@ -322,7 +329,7 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: false }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -334,65 +341,66 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); }); - it("fails CLOSED: elicits when the tool JSON is missing (approval requirement unknown)", async () => { + it("fails closed when the tool's approval requirement is unknown", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - // No writeToolJson: findToolJson returns null, so requires_approval is - // unknown. The native prompt is suppressed (readOnlyHint), so the gate must - // fire rather than execute ungated. - await handleRunTool(remote, server, tmpDir, baseArgs); + + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); - expect(remote.callTool).toHaveBeenCalledTimes(1); // executed on accept + expect(remote.callTool).toHaveBeenCalledTimes(1); }); - it("fails CLOSED: does NOT execute a tool with unknown approval requirement when declined", async () => { + it("does not execute unknown-approval tools when the user declines", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "decline" }); const server = makeServer({ elicitation: true, elicit }); - // Tool JSON missing -> unknown -> gate fires; user declines. - await handleRunTool(remote, server, tmpDir, baseArgs); + + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool).not.toHaveBeenCalled(); }); - it("sanitizes argument keys so newlines can't forge approval-prompt lines", async () => { + it("sanitizes argument keys so newlines cannot forge prompt labels", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, { - server_id: "s", - tool_name: "jirasearch", - arguments: { "note\nACTION: read_only_lookup": "x" }, - }); + await handleRunTool( + remote, + server, + tmpDir, + { + server_id: "s", + tool_name: "jirasearch", + arguments: { "note\nACTION: read_only_lookup": "x" }, + }, + ALL_ON, + ); const message = elicit.mock.calls[0][0].message as string; - // The forged "ACTION:" must not begin its own line: the key's newline is - // collapsed, so it stays inline on the single (uppercased) NOTE line. expect(message).not.toMatch(/^\s*ACTION: READ_ONLY_LOOKUP/m); - // The real action label appears exactly once. expect( - message.split("\n").filter((l) => l.startsWith("Action:")), + message.split("\n").filter((line) => line.startsWith("Action:")), ).toHaveLength(1); }); - it("does NOT elicit for Cursor — its native prompt is the gate; executes directly", async () => { + it("DOES elicit for Cursor — our prompt is the single gate there too", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn(); + const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, clientName: "cursor-vscode", @@ -400,14 +408,246 @@ describe("handleRunTool (HITL)", () => { }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - // Cursor: readOnlyHint is omitted (native prompt shows), so our elicitation - // is skipped and the tool runs directly. - expect(elicit).not.toHaveBeenCalled(); + // Cursor is no longer excluded: it gets readOnlyHint like every other + // elicitation-capable host, so this prompt is the only approval gate. + expect(elicit).toHaveBeenCalledTimes(1); + expect(remote.callTool).toHaveBeenCalledTimes(1); + }); + + // Cursor used to render the tool and its arguments itself, so its prompt was only a + // review ask pointing at them. It stopped doing that (confirmed by screenshot, Aug + // 2026), so it now gets the same self-describing text as every other host. + it("spells out action and arguments for Cursor too, since it no longer shows them", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const server = makeServer({ + elicitation: true, + clientName: "cursor-vscode", + elicit, + }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + arguments: { project: "ENG", summary: "ship it" }, + }, ALL_ON); + + const message = elicit.mock.calls[0][0].message as string; + expect(message).toContain("Action: jirasearch"); + expect(message).toContain("ENG"); + // Would point at something Cursor no longer draws. + expect(message).not.toContain("shown above"); + }); + + it("spells out action and arguments for a host that does not render them", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + arguments: { project: "ENG" }, + }, ALL_ON); + + const message = elicit.mock.calls[0][0].message as string; + expect(message).toContain("Action: jirasearch"); + expect(message).toContain("ENG"); + expect(message).not.toContain("shown above"); + }); + + // Cursor's pre-3.15 bug can drop the prompt, so the request burns the whole + // timeout. Its version cannot be checked (clientInfo reports a hardcoded + // "1.0.0"), so the note keys off that duration instead. + it("raises Cursor as a possible cause when the request waits out the clock", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + vi.stubEnv("HITL_TIMEOUT_MS", "40"); + const remote = makeRemote(); + const elicit = vi.fn().mockImplementation( + () => + new Promise((_resolve, reject) => + setTimeout(() => reject(new Error("Request timed out")), 60), + ), + ); + const server = makeServer({ + elicitation: true, + clientName: "cursor-vscode", + elicit, + }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const text = (result.content[0] as { text: string }).text; + + expect(result.isError).toBe(true); + expect(text).toContain("3.15"); + expect(text).toContain("NOT executed"); + expect(remote.callTool).not.toHaveBeenCalled(); + }); + + // A timeout cannot distinguish "prompt shown, nobody answered" from "prompt never + // delivered", so the text must not claim the prompt was missing. Asserting it would + // send the user chasing a Cursor upgrade for what may just be an unanswered prompt. + it("frames the missing prompt as a possibility, never as a finding", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + vi.stubEnv("HITL_TIMEOUT_MS", "40"); + const remote = makeRemote(); + const elicit = vi.fn().mockImplementation( + () => + new Promise((_resolve, reject) => + setTimeout(() => reject(new Error("Request timed out")), 60), + ), + ); + const server = makeServer({ + elicitation: true, + clientName: "cursor-vscode", + elicit, + }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const text = (result.content[0] as { text: string }).text; + + expect(text).toContain("cannot tell which"); + expect(text).toContain("One possible cause"); + expect(text).toContain("Ask the user whether they saw an approval prompt"); + // No claim about what did or did not happen on screen. + expect(text).not.toContain("never appeared"); + expect(text).not.toContain("is silently dropped"); + expect(text).not.toContain("will fix this"); + }); + + // Escape should resolve with action "cancel", but a host that delivers it as + // an abort instead reaches this path as ErrorCode.RequestTimeout — the SDK + // wraps every abort reason that way, so the code and message shape are + // identical to a real timeout. Duration is the only discriminator, and a fast + // failure must not blame Cursor's version. + it("omits Cursor guidance when the prompt failed early, e.g. dismissed", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + vi.stubEnv("HITL_TIMEOUT_MS", "10000"); + const remote = makeRemote(); + const elicit = vi + .fn() + .mockRejectedValue(new Error("MCP error -32001: Request timed out")); + const server = makeServer({ + elicitation: true, + clientName: "cursor-vscode", + elicit, + }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const text = (result.content[0] as { text: string }).text; + + expect(result.isError).toBe(true); + expect(text).not.toContain("3.15"); + expect(text).toContain("Ask the user to confirm"); + expect(remote.callTool).not.toHaveBeenCalled(); + }); + + it("never mentions Cursor to another host, even on a full-timeout hang", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + vi.stubEnv("HITL_TIMEOUT_MS", "40"); + const remote = makeRemote(); + const elicit = vi.fn().mockImplementation( + () => + new Promise((_resolve, reject) => + setTimeout(() => reject(new Error("Request timed out")), 60), + ), + ); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + + expect((result.content[0] as { text: string }).text).not.toContain("3.15"); + expect(remote.callTool).not.toHaveBeenCalled(); + }); + + // fileArgs disabled by remote policy. The refusal lives here rather than at the call + // funnel because resolveFileArgs reads model-supplied absolute paths off the user's + // disk: an inert feature has to mean the read does not happen. + it("rejects file_args when policy disables the feature", async () => { + vi.stubEnv("ENABLE_HITL", "false"); + const remote = makeRemote(); + const server = makeServer({ elicitation: false }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + + const result = await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, file_args: { body: "/tmp/whatever.md" } }, + { fileArgs: false }, + ); + + expect(result.isError).toBe(true); + expect((result.content[0] as { text: string }).text).toContain( + "`file_args` is disabled", + ); + expect(remote.callTool).not.toHaveBeenCalled(); + }); + + // Proves the refusal short-circuits BEFORE any disk access: the path does not exist, + // so a guard placed after resolveFileArgs would surface a "cannot read" FileArgsError + // instead. That difference is the whole point of where the check sits. + it("rejects without touching the filesystem", async () => { + vi.stubEnv("ENABLE_HITL", "false"); + const remote = makeRemote(); + const server = makeServer({ elicitation: false }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + + const result = await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, file_args: { body: path.join(tmpDir, "does-not-exist.md") } }, + { fileArgs: false }, + ); + + const text = (result.content[0] as { text: string }).text; + expect(text).toContain("`file_args` is disabled"); + expect(text).not.toContain("cannot read"); + }); + + it("runs normally when policy disables file_args but the call passes none", async () => { + vi.stubEnv("ENABLE_HITL", "false"); + const remote = makeRemote(); + const server = makeServer({ elicitation: false }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, { + fileArgs: false, + }); + + expect(result.isError).toBeUndefined(); expect(remote.callTool).toHaveBeenCalledTimes(1); }); + it("treats a spec-compliant cancel as a cancel, not a failure", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi.fn().mockResolvedValue({ action: "cancel" }); + const server = makeServer({ + elicitation: true, + clientName: "cursor-vscode", + elicit, + }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const text = (result.content[0] as { text: string }).text; + + expect(text).toContain("cancelled by the user"); + expect(text).not.toContain("3.15"); + expect(result.isError).toBeUndefined(); + expect(remote.callTool).not.toHaveBeenCalled(); + }); + it("prompts with action name + arguments and forwards on accept", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); @@ -418,7 +658,7 @@ describe("handleRunTool (HITL)", () => { description: "Search Jira issues", }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); const [params, options] = elicit.mock.calls[0]; expect(params.message).toContain("Action: jirasearch"); @@ -441,8 +681,8 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true, elicit, request }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); // Ping fired exactly once for this server, and it is a ping. expect(request).toHaveBeenCalledTimes(1); @@ -458,7 +698,7 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true, request }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(request).not.toHaveBeenCalled(); }); @@ -471,7 +711,7 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit.mock.calls[0][1].timeout).toBe(5000); }); @@ -486,7 +726,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit.mock.calls[0][1].timeout).toBe(300_000); } @@ -499,7 +739,7 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs); + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(remote.callTool).not.toHaveBeenCalled(); expect((result.content[0] as { text: string }).text).toContain("declined"); @@ -512,7 +752,7 @@ describe("handleRunTool (HITL)", () => { const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs); + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(remote.callTool).not.toHaveBeenCalled(); expect(result.isError).toBe(true); @@ -533,7 +773,7 @@ describe("handleRunTool (HITL)", () => { server_id: "s", tool_name: "create_doc", arguments: { title: "Report", body: bigBody }, - }); + }, ALL_ON); const message = elicit.mock.calls[0][0].message as string; expect(message).toContain("Action: create_doc"); @@ -566,7 +806,7 @@ describe("handleRunTool (HITL)", () => { tool_name: "create_doc", arguments: { title: "Doc" }, file_args: { body: bodyFile }, - }); + }, ALL_ON); const message = elicit.mock.calls[0][0].message as string; expect(message).toContain("TITLE: Doc"); @@ -590,7 +830,7 @@ describe("handleRunTool (HITL)", () => { tool_name: "save_agent", arguments: {}, file_args: { spec: specFile }, - }); + }, ALL_ON); const call = remote.callTool.mock.calls[0][0]; expect(call.name).toBe("run_tool"); @@ -612,7 +852,7 @@ describe("handleRunTool (HITL)", () => { tool_name: "create_doc", arguments: {}, file_args: { body: "/no/such/abs/path.md" }, - }); + }, ALL_ON); expect(result.isError).toBe(true); expect(elicit).not.toHaveBeenCalled(); // no prompt for unreadable input @@ -629,7 +869,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -645,7 +885,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -661,7 +901,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); }); @@ -677,7 +917,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); // gate preserved for THIS session }); @@ -759,21 +999,44 @@ describe("formatArgumentsForFile", () => { }); describe("runToolAnnotations", () => { - it("marks run_tool read-only when HITL gates an elicitation-capable non-Cursor client", () => { - expect(runToolAnnotations(true, true, false)).toEqual({ + it("marks run_tool read-only when HITL gates an elicitation-capable client", () => { + expect(runToolAnnotations(true, true)).toEqual({ readOnlyHint: true, }); }); it("leaves annotations unset when HITL is disabled", () => { - expect(runToolAnnotations(false, true, false)).toBeUndefined(); + expect(runToolAnnotations(false, true)).toBeUndefined(); }); it("leaves annotations unset when the client cannot elicit", () => { - expect(runToolAnnotations(true, false, false)).toBeUndefined(); + expect(runToolAnnotations(true, false)).toBeUndefined(); + }); + + // Cursor used to be excluded here so it would show its own native prompt. It no + // longer is: our elicitation is the single gate on every elicitation-capable host, + // and a Cursor build that drops the prompt surfaces as a timeout carrying upgrade + // guidance rather than as a permanently weaker gate. + it("advertises readOnlyHint to Cursor as well, so our prompt is the single gate", () => { + expect(runToolAnnotations(true, true)).toEqual({ readOnlyHint: true }); + }); +}); + +describe("elicitationFailureText", () => { + const cursor = { + getClientVersion: () => ({ name: "cursor-vscode", version: "1.0.0" }), + } as any; + + // The shipped HITL_TIMEOUT_MS is 300000, so this phrasing is what almost every + // real occurrence prints. A model relays it verbatim, so it reads in minutes. + it("renders the 5-minute default as minutes, not 300s", () => { + const text = elicitationFailureText(cursor, "t", "timed out", 300_000, 300_000); + expect(text).toContain("the full 5 minutes"); + expect(text).not.toContain("300s"); }); - it("does NOT advertise readOnlyHint to Cursor (its elicitation only renders on the attended lane)", () => { - expect(runToolAnnotations(true, true, true)).toBeUndefined(); + it("keeps short test timeouts in seconds", () => { + const text = elicitationFailureText(cursor, "t", "timed out", 20_000, 20_000); + expect(text).toContain("the full 20s"); }); }); diff --git a/shared/glean/mcp/tests/version.test.ts b/shared/glean/mcp/tests/version.test.ts index 41a7140..f64f268 100644 --- a/shared/glean/mcp/tests/version.test.ts +++ b/shared/glean/mcp/tests/version.test.ts @@ -12,7 +12,7 @@ import { createInterface } from "node:readline"; import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { PLUGIN_VERSION } from "../src/version.js"; +import { pluginVersion, pluginVersionString } from "../src/version.js"; // Anchor to the test file, not cwd — vitest runs with --root shared/glean/mcp // while the process cwd stays at the repo root. @@ -24,10 +24,9 @@ const readVersion = (manifest: string): string => JSON.parse(readFileSync(manifest, "utf8")).version; describe("plugin version", () => { - it("reports the shipped manifest's version when run from source", () => { - expect(PLUGIN_VERSION).toBe( - readVersion(path.join(serverDir, "package.json")), - ); + it("reports unknown when source runs without the build-time define", () => { + expect(pluginVersion()).toEqual({ version: "0.0.0", source: "unknown" }); + expect(pluginVersionString()).toBe("0.0.0"); }); it("stays in step with the repo-root version that release-it bumps", () => { @@ -38,8 +37,8 @@ describe("plugin version", () => { }); it("advertises the version over MCP from the built bundle", async () => { - // Guards esbuild's handling of import.meta.url: unbundled the read resolves - // from src/, shipped it resolves from dist/. + // The build verifies both package versions, substitutes the constant, and + // asserts that the literal landed in the emitted bundle. execFileSync("node", ["shared/glean/mcp/build.mjs"], { cwd: repoRoot, stdio: "pipe",