diff --git a/shared/glean/mcp/src/index.ts b/shared/glean/mcp/src/index.ts index ed43b07..4d5255b 100644 --- a/shared/glean/mcp/src/index.ts +++ b/shared/glean/mcp/src/index.ts @@ -50,7 +50,11 @@ import { protocolVersion, setPolicyServerUrl, } from "./policy/session.js"; -import { advertisedTools, policyRefusal } from "./policy/enforce.js"; +import { + advertisedTools, + policyRefusal, + setupClosingLine, +} from "./policy/enforce.js"; function readEnv(...keys: string[]): string | undefined { for (const key of keys) { @@ -366,7 +370,11 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { } try { - const remoteTools = await fetchAllowedRemoteTools(remoteClient); + // The host asked for this list and is about to receive it, so any policy learned here + // needs no notification -- this response IS the update. + const remoteTools = await fetchAllowedRemoteTools(remoteClient, { + hostReceivingList: true, + }); cachedRemoteTools = remoteTools; saveRemoteTools(serverUrl, remoteTools); return serve("fetched", remoteTools); @@ -545,21 +553,10 @@ async function advanceSetup(): Promise { const remoteTools = await fetchAllowedRemoteTools(remoteClient); 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(", ") + - `.`; + const closing = setupClosingLine({ + decision: decisionInForce(), + promoted: remoteTools.map((t) => t.name), + }); return { content: [ { @@ -568,7 +565,6 @@ async function advanceSetup(): Promise { `Glean setup is complete.\n` + `Server URL: ${serverUrl}\n` + `Authenticated: yes\n` + - `Remote tools: ${toolNames}\n` + `${policySummary().join("\n")}\n\n` + closing, }, diff --git a/shared/glean/mcp/src/policy/enforce.ts b/shared/glean/mcp/src/policy/enforce.ts index 4d314a2..4e854e7 100644 --- a/shared/glean/mcp/src/policy/enforce.ts +++ b/shared/glean/mcp/src/policy/enforce.ts @@ -155,3 +155,44 @@ 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."; + +/** + * The closing sentence of `setup`: exactly what the caller may invoke right now. + * + * There used to be two lists here, and they disagreed the moment policy withheld + * anything -- `setup` printed the remote's whole catalog ("Remote tools: search, chat, + * ...") and then closed with "You can now use find_skills_and_tools, run_tool". A model reading + * that has been handed names it may call, most of which are not advertised and would be + * refused on call. The remote's catalog is the remote's business, so it is gone and this + * is the single authoritative list. + * + * Scoped to naming tools and nothing else. Deactivation status and the remote's upgrade + * message belong to policySummary(), which prints them a few lines above -- stating the + * consequence here as well duplicated it, and when the remote supplied its own wording + * the specific instruction ("Run `claude plugin update glean`") was immediately followed + * by a vaguer restatement of it. + * + * No deactivation branch is needed to achieve that: evaluate() reports every feature as + * false when deactivated, so the empty case below is reached without asking. Meta-tool + * names come from META_TOOL_NAMES so this cannot drift from what advertisedTools() + * actually serves. + */ +export function setupClosingLine(input: { + decision: Decision; + promoted: readonly string[]; +}): string { + const { decision, promoted } = input; + const usable = [ + ...(decision.features.metaTools ? [...META_TOOL_NAMES] : []), + ...(decision.features.toolPromotion ? promoted : []), + ]; + // Two ways to get here, deliberately answered the same way: a deactivated plugin, and + // a policy that disables metaTools and toolPromotion together without deactivating. + // The cause is on the `Policy:`/`Deactivated:` lines above; this states only the + // consequence, because without it the second case leaves the feature JSON as the sole + // hint that nothing is callable. Unguarded, the sentence degrades to "You can now use ." + if (usable.length === 0) { + return `No tools are available beyond \`${SETUP_TOOL_NAME}\`.`; + } + return `You can now use ${usable.join(", ")}.`; +} diff --git a/shared/glean/mcp/src/policy/session.ts b/shared/glean/mcp/src/policy/session.ts index 23e862b..54d3c1c 100644 --- a/shared/glean/mcp/src/policy/session.ts +++ b/shared/glean/mcp/src/policy/session.ts @@ -38,9 +38,10 @@ 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. +// The label recordPolicyFromResult receives for a tools/list exchange, shared by the +// tools/list handler and by setup's own catalog fetch. Exported so both spell it the same +// way in the log. The notification guard deliberately does NOT key off it -- see +// RecordPolicyOptions.hostReceivingList, which is what distinguishes those two callers. export const TOOLS_LIST_LABEL = "tools/list"; export function initPolicySession(server: Server, log: LogFn): void { @@ -108,6 +109,19 @@ export function negotiationMeta(): { _meta: Record } { return metaFor(negotiationRequest()); } +export interface RecordPolicyOptions { + /** + * True only when the host requested this tool list and is about to receive the surface + * this decision produces. It suppresses the re-fetch notification, because the response + * itself is the update. + * + * An explicit flag rather than a label check: `setup` fetches the catalog through the + * same helper and carries the same label, but the host is receiving setup's text, so it + * does need telling. + */ + hostReceivingList?: boolean; +} + /** * Record the policy carried on a remote response, if any. * @@ -137,7 +151,11 @@ export function negotiationMeta(): { _meta: Record } { * 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 { +export function recordPolicyFromResult( + result: unknown, + label: string, + { hostReceivingList = false }: RecordPolicyOptions = {}, +): 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; @@ -210,20 +228,25 @@ export function recordPolicyFromResult(result: unknown, label: string): void { }); } - // Tell the host to re-fetch, but only from the tools/call path and only once a - // previous decision existed. + // Tell the host to re-fetch when the reachable surface changed under it. + // + // Suppressed for exactly one case: the host asked for the list and is about to receive + // the freshly filtered surface, so a notification would make it ask again for what it + // already holds, and notify -> tools/list -> resolve -> notify would be a cycle. That + // is what `hostReceivingList` means, and it is passed by the tools/list handler alone. // - // 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. + // It deliberately is NOT inferred from the label. `setup` fetches the remote catalog + // through the same helper and therefore carries the same `tools/list` label, but the + // host is receiving setup's text, not a tool list -- so a policy that changes the + // surface during setup used to leave the host holding a stale list with nothing to + // prompt a refresh. Observed against a real remote: setup learned `toolPromotion: true` + // and the promoted tools stayed invisible until the next unrelated list. // // 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) { + if (changed && previous && !hostReceivingList) { logLine("policy.surface-changed", { label, from: { deactivated: previous.deactivated, features: previous.features }, diff --git a/shared/glean/mcp/src/tools/remote-passthrough.ts b/shared/glean/mcp/src/tools/remote-passthrough.ts index 1be70d2..e3adcf1 100644 --- a/shared/glean/mcp/src/tools/remote-passthrough.ts +++ b/shared/glean/mcp/src/tools/remote-passthrough.ts @@ -57,18 +57,25 @@ export function augmentSchemaForLocal(schema: unknown): ToolInputSchema { * each surviving tool with its input schema augmented for local exposure. * * Walks pagination cursors to exhaustion in case the remote ever paginates. + * + * `hostReceivingList` must be true only when the caller is answering a host `tools/list`. + * Setup calls this too, and there the host is receiving setup's text rather than a tool + * list, so a surface-changing policy learned here has to notify. */ export async function fetchAllowedRemoteTools( remoteClient: Client, + { hostReceivingList = false }: { hostReceivingList?: boolean } = {}, ): Promise { const collected: Tool[] = []; let cursor: string | undefined; do { + // Negotiation metadata rides on tools/list as well as tools/call, so a session + // that only ever lists tools still reports its context and still receives policy. const page = await remoteClient.listTools({ ...(cursor ? { cursor } : {}), ...negotiationMeta(), }); - recordPolicyFromResult(page, TOOLS_LIST_LABEL); + recordPolicyFromResult(page, TOOLS_LIST_LABEL, { hostReceivingList }); for (const tool of page.tools) { if (!REMOTE_TOOLS_ALLOWLIST.has(tool.name)) continue; collected.push({ diff --git a/shared/glean/mcp/tests/policy-enforce.test.ts b/shared/glean/mcp/tests/policy-enforce.test.ts index c5dd7a0..2e3812b 100644 --- a/shared/glean/mcp/tests/policy-enforce.test.ts +++ b/shared/glean/mcp/tests/policy-enforce.test.ts @@ -3,6 +3,7 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js"; import { advertisedTools, policyRefusal, + setupClosingLine, withoutFileArgs, } from "../src/policy/enforce.js"; import { evaluate } from "../src/policy/evaluate.js"; @@ -318,3 +319,98 @@ describe("withoutFileArgs", () => { expect(withoutFileArgs(findSkillsTool)).toBe(findSkillsTool); }); }); + +// setup's closing sentence is the only place a user or a model is TOLD what it may call, +// and it had no coverage while it was assembled inline in index.ts -- which is how it came +// to contradict the advertised surface. The contract asserted here is agreement: whatever +// this sentence names, advertisedTools() serves. +describe("setupClosingLine", () => { + const promoted = ["search", "chat"]; + + it("names the meta tools and the promoted tools when policy allows both", () => { + expect(setupClosingLine({ decision: decision(), promoted })).toBe( + "You can now use find_skills_and_tools, run_tool, search, chat.", + ); + }); + + // The defect this change fixes: setup printed the remote's catalog, so a withheld + // feature produced a sentence naming tools the very next call would refuse. Asserted + // against the gate as well, so the sentence and the served surface cannot drift. + it("omits the promoted tools when toolPromotion is off, and says nothing about them", () => { + const d = decision({ features: { ...allSupported, toolPromotion: false } }); + + const line = setupClosingLine({ decision: d, promoted }); + + expect(line).toBe("You can now use find_skills_and_tools, run_tool."); + for (const name of promoted) { + expect(line).not.toContain(name); + expect(names(d)).not.toContain(name); + } + }); + + it("omits the meta tools when metaTools is off", () => { + const d = decision({ features: { ...allSupported, metaTools: false } }); + + const line = setupClosingLine({ decision: d, promoted }); + + expect(line).toBe("You can now use search, chat."); + expect(names(d)).not.toContain("find_skills_and_tools"); + expect(names(d)).not.toContain("run_tool"); + }); + + // A remote that promotes nothing differs from one whose promotion was withheld, and + // neither may leave a dangling reference to a list setup no longer prints. + it("names only the meta tools when the remote promotes nothing", () => { + expect(setupClosingLine({ decision: decision(), promoted: [] })).toBe( + "You can now use find_skills_and_tools, run_tool.", + ); + }); + + it("says only setup is available when policy disables both features", () => { + const line = setupClosingLine({ + decision: decision({ + features: { ...allSupported, metaTools: false, toolPromotion: false }, + }), + promoted, + }); + + // Not "You can now use ." -- the empty join is the failure this branch exists for. + // Reached without the deactivated flag, which is why the branch cannot key on it. + expect(line).toBe("No tools are available beyond `setup`."); + expect(line).not.toContain("You can now use"); + }); + + // Deactivation reaches the empty case through evaluate(), which reports every feature + // as false -- so this needs no branch of its own, and asserting it here is what pins + // that. The status and the upgrade instruction are policySummary()'s, and saying them + // here as well was the duplication this scoping removes. + it("names no tools for a deactivated install, and does not restate the upgrade", () => { + const line = setupClosingLine({ + decision: decision({ + deactivated: true, + features: { toolPromotion: false, metaTools: false, fileArgs: false }, + showUpgrade: true, + upgradeMessage: "Run `claude plugin update glean`.", + }), + promoted, + }); + + expect(line).toBe("No tools are available beyond `setup`."); + expect(line).not.toContain("Upgrade"); + expect(line).not.toContain("claude plugin update"); + expect(line).not.toContain("find_skills_and_tools"); + }); + + // The reason a deactivated decision must not be special-cased: evaluate() has already + // zeroed the features, so a branch keyed on the flag would be a second source of truth + // for the same fact. + it("is driven by the features, not by the deactivated flag", () => { + const asIfDeactivated = decision({ + features: { toolPromotion: false, metaTools: false, fileArgs: false }, + }); + + expect(setupClosingLine({ decision: asIfDeactivated, promoted })).toBe( + "No tools are available beyond `setup`.", + ); + }); +}); diff --git a/shared/glean/mcp/tests/policy-session.test.ts b/shared/glean/mcp/tests/policy-session.test.ts index 78aaa31..aa14e49 100644 --- a/shared/glean/mcp/tests/policy-session.test.ts +++ b/shared/glean/mcp/tests/policy-session.test.ts @@ -214,9 +214,14 @@ describe("decisionInForce", () => { const seen: boolean[] = []; for (const label of ["tools/list", "tools/call(search)", "tools/list", "tools/call(chat)"]) { + const isList = label === "tools/list"; session.recordPolicyFromResult( - label === "tools/list" ? resultWith(policy) : { content: [] }, + isList ? resultWith(policy) : { content: [] }, label, + // Mirrors the real handlers: a host-requested list suppresses the notification, + // a tool call does not. Without this the test would exercise a combination that + // never occurs in production. + isList ? { hostReceivingList: true } : undefined, ); seen.push(session.decisionInForce().features.metaTools); } @@ -289,17 +294,38 @@ describe("tools/list_changed notification", () => { // 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 () => { + // The suppression is about whether the HOST is receiving the surface, not about which + // remote method produced it. When it asked for the list, the response is the update, and + // notifying would make it ask again -- notify -> list -> resolve -> notify is a cycle. + it("does not notify when the host is receiving the list it asked for", async () => { const { session, server } = await armed(); session.recordPolicyFromResult( resultWith({ features: { metaTools: { enabled: false } } }), "tools/list", + { hostReceivingList: true }, ); expect(server.sendToolListChanged).not.toHaveBeenCalled(); }); + // The regression this replaced a label check for. `setup` fetches the remote catalog + // through the same helper, so it carries the same "tools/list" label -- but the host is + // receiving setup's text, not a tool list. Keying suppression off the label therefore + // left the host holding a stale list with nothing to prompt a refresh. Observed against + // a real remote: setup learned toolPromotion: true and the promoted tools stayed + // invisible until some later unrelated list. + it("notifies when setup learns a surface change, despite the tools/list label", async () => { + const { session, server } = await armed(); + + session.recordPolicyFromResult( + resultWith({ features: { metaTools: { enabled: false } } }), + "tools/list", + ); + + expect(server.sendToolListChanged).toHaveBeenCalledTimes(1); + }); + it("does not notify when only advisory fields differ", async () => { const { session, server } = await armed(); diff --git a/shared/glean/mcp/tests/setup-output.test.ts b/shared/glean/mcp/tests/setup-output.test.ts new file mode 100644 index 0000000..9ca50c1 --- /dev/null +++ b/shared/glean/mcp/tests/setup-output.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it, 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"; + +// policySummary() and setupClosingLine() are only ever printed together, by the setup +// handler, and the defect they had was one of composition rather than of either half: +// each said "only `setup` is available" and each restated the upgrade instruction, so a +// deactivated install was told twice -- and when the remote supplied its own wording, its +// specific instruction was followed by a vaguer generic one. Neither unit test could see +// that. This asserts the assembled text. +// +// Deactivation is gated on version provenance, and the build constant is absent under +// vitest, which is why policy-session.test.ts documents these lines as unreachable there. +// Mocking the version module is what unlocks them. +vi.mock("../src/version.js", () => ({ + pluginVersion: () => ({ version: "0.2.49", source: "build" }), + pluginVersionString: () => "0.2.49", +})); + +const URL_A = "https://a-be.glean.com/mcp/gateway/proxy"; +const REMOTE_UPGRADE_TEXT = "Run `claude plugin update glean` to reach 9.9.9 or later."; + +function occurrences(haystack: string, needle: string): number { + return haystack.split(needle).length - 1; +} + +/** The setup handler's text, assembled exactly as index.ts assembles it. */ +async function setupText(policy: unknown, promoted: string[]) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "setup-output-")); + vi.resetModules(); + vi.stubEnv("PLUGIN_DATA_DIR", dir); + const session = await import("../src/policy/session.js"); + const { setupClosingLine } = await import("../src/policy/enforce.js"); + + session.initPolicySession( + { + getClientVersion: () => ({ name: "claude-code", version: "1.2.3" }), + getClientCapabilities: () => ({}), + sendToolListChanged: async () => {}, + } as never, + () => {}, + ); + session.setPolicyServerUrl(URL_A); + session.recordPolicyFromResult( + { tools: [], _meta: { [CAPABILITY_POLICY_KEY]: policy } }, + "tools/call(search)", + ); + + const text = + `Glean setup is complete.\n` + + `Server URL: ${URL_A}\n` + + `Authenticated: yes\n` + + `${session.policySummary().join("\n")}\n\n` + + setupClosingLine({ decision: session.decisionInForce(), promoted }); + + fs.rmSync(dir, { recursive: true, force: true }); + return text; +} + +describe("the assembled setup output", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + const deactivating = { + plugin: { + minimumSupportedVersion: "9.9.9", + upgradeRecommendation: { show: true, message: REMOTE_UPGRADE_TEXT }, + }, + }; + + it("gives the remote's upgrade instruction exactly once when deactivated", async () => { + const text = await setupText(deactivating, ["search", "chat"]); + + expect(text).toContain("Deactivated:"); + expect(occurrences(text, REMOTE_UPGRADE_TEXT)).toBe(1); + // The generic fallback must not appear alongside the remote's own wording, which is + // what the closing used to add. + expect(text).not.toContain("Upgrade the Glean plugin"); + }); + + it("names no tools when deactivated, and states it once", async () => { + const text = await setupText(deactivating, ["search", "chat"]); + + expect(text).toContain("No tools are available beyond `setup`."); + expect(text).not.toContain("You can now use"); + for (const name of ["find_skills_and_tools", "run_tool", "search", "chat"]) { + expect(text).not.toContain(name); + } + }); + + it("never prints the remote's catalog alongside the usable list", async () => { + const text = await setupText( + { features: { toolPromotion: { enabled: false } } }, + ["search", "chat", "employee_search"], + ); + + expect(text).toContain("You can now use find_skills_and_tools, run_tool."); + // The regression: a withheld feature used to leave these named in "Remote tools: ..." + // while being unusable and unadvertised. + expect(text).not.toContain("Remote tools:"); + for (const name of ["search", "chat", "employee_search"]) { + expect(text).not.toContain(name); + } + }); + + it("promotes the remote's tools into the one usable list when policy allows", async () => { + const text = await setupText({ features: {} }, ["search", "chat"]); + + expect(text).toContain("You can now use find_skills_and_tools, run_tool, search, chat."); + expect(text).not.toContain("Remote tools:"); + }); +});