feat(vnext): notify on setup policy, and print one tool list in setup

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<CallToolResult> {
     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<CallToolResult> {
             `Glean setup is complete.\n` +
             `Server URL: ${serverUrl}\n` +
             `Authenticated: yes\n` +
-            `Remote tools: ${toolNames}\n` +
             `${policySummary().join("\n")}\n\n` +
             closing,
         },

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(", ")}.`;
+}

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<string, unknown> } {
   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<string, unknown> } {
  * 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 },

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<Tool[]> {
   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({

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`.",
+    );
+  });
+});

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();