From 8bd9acedbe379eac69e9b68696b8c3b2ca5fc686 Mon Sep 17 00:00:00 2001 From: "mohit.gupta@glean.com" Date: Thu, 20 Aug 2026 21:43:50 +0530 Subject: [PATCH] feat(vnext): notify on setup policy, and print one tool list in setup Ports gleanwork/glean-plugins-vnext#54 and #55. #54 -- the tools/list_changed notification was suppressed whenever the label said "tools/list", on the reasoning that there the response IS the update: the host asked and is about to receive the filtered surface, so notifying would make it ask again and notify -> list -> resolve -> notify is a cycle. The reasoning holds; the label does not identify the case. `setup` fetches the catalog through the same fetchAllowedRemoteTools helper and so carries the same label, while the host is receiving setup's TEXT. A policy that changed the reachable surface during setup therefore left the host holding a stale list with nothing to prompt a refresh. Observed against a real remote rather than reasoned about: an experimental QE pod flipped toolPromotion false -> true, setup picked it up and the plugin would have executed the promoted tools, but they stayed invisible until a later unrelated list. Suppression now takes an explicit hostReceivingList flag, passed by the tools/list handler alone; setup and every tools/call take the default and notify. Defaulting to notify is the safe direction -- a new call site has to opt into silence rather than inherit it. #55 -- 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". The two disagreed the moment policy withheld anything, and the closing pointed at the catalog with "any of the listed remote tools", so a model was handed names it may call that were neither advertised nor callable. The catalog is gone and setupClosingLine() in enforce.ts is the single authoritative list, scoped to naming tools: deactivation status and the remote's upgrade message stay with policySummary(), which prints them a few lines above. It needs no deactivation branch, because evaluate() already reports every feature as false when deactivated. Port notes. session.ts and remote-passthrough.ts were identical to vnext and copy over whole. enforce.ts differed only by META_TOOL_NAMES naming find_skills_and_tools, and setupClosingLine derives its names from that set rather than repeating them, so the function ports unchanged. The test files differed from vnext by exactly two substitutions, established by diffing before porting: the tool name, and `/plugin update glean-vnext` -> `/plugin update glean`. tests/setup-output.test.ts is new. It asserts the ASSEMBLED setup text, because the duplication both PRs touch was a property of policySummary() and the closing together and no unit test on either half could see it. Mocking version.js is what makes the deactivation lines reachable under vitest at all -- deactivation is gated on version provenance. Verified here, not just upstream: typecheck, 333 tests, check:no-shell, build, and validate on all three targets. Mutation-checked in this repo -- restoring the label check fails exactly the setup-notification test, and ignoring toolPromotion fails six across both the unit and composition layers. --- shared/glean/mcp/src/index.ts | 32 +++-- shared/glean/mcp/src/policy/enforce.ts | 41 +++++++ shared/glean/mcp/src/policy/session.ts | 47 +++++-- .../glean/mcp/src/tools/remote-passthrough.ts | 9 +- shared/glean/mcp/tests/policy-enforce.test.ts | 96 +++++++++++++++ shared/glean/mcp/tests/policy-session.test.ts | 30 ++++- shared/glean/mcp/tests/setup-output.test.ts | 115 ++++++++++++++++++ 7 files changed, 337 insertions(+), 33 deletions(-) create mode 100644 shared/glean/mcp/tests/setup-output.test.ts 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:"); + }); +});