fix(mcp): keep loaded tools when their server goes away, and never stop silently

AGENTS.md

@@ -390,7 +390,9 @@ cameron_analytics` migration, and `analytics.test.ts` fails if the two differ. M
   delegates to the SDK's `auth()` — never hardcode a scope in the client metadata.
 - **Progressive disclosure** ([docs/MCP_DISCLOSURE.md](docs/MCP_DISCLOSURE.md), read it before
   changing this code): the model sees a short list of capabilities plus `load_tools`, and a tool's
-  definition only once a search has loaded it; nothing connects until a tool is called. The logic
+  definition only once a search has loaded it; nothing connects until a tool is called. A loaded
+  tool's definition is saved in the thread (`toolDefinitions`), so it stays offered after its
+  server is removed or turned off, and a call to it returns `server_unavailable`. The logic
   is the workspace package `packages/mcp-disclosure`, which must not import app code or the MCP
   client library; `src/lib/mcp/connector.ts` is the only file using the adapters.
 
@@ -466,6 +468,20 @@ cameron_analytics` migration, and `analytics.test.ts` fails if the two differ. M
   client-only artifact. Waiting whenever an artifact is merely absent is indistinguishable from
   waiting for one that will never come — it parked the pump on every ordinary tool and pushed all
   results to the end of the turn. `messageStream.test.ts` pins both this and the pending contract.
+- **A result a middleware returned itself is streamed from the state.** The disclosure middleware
+  answers some calls without running the tool node (`server_unavailable`, `auth_required`,
+  `unknown_tool`), so they never appear on `run.toolCalls`. `streamMessages` also takes results
+  from `run.values` snapshots and the final state, for calls this run requested, each sent once
+  (`pushResult`). Without it the result showed only after a reload, and the call was reported as
+  `pendingToolCallIds`, the approval signal.
+- **A malformed tool call is retried, never dropped silently.** Arguments that don't parse land in
+  `invalid_tool_calls` (or, in LangChain's v1 output format, only as `invalid_tool_call` content
+  blocks: read both, via `findInvalidCalls`). The agent never runs such a call, so with no valid
+  calls the turn just ended. `invalidToolCallMiddleware` (`src/lib/agent/invalidToolCalls.ts`,
+  installed just inside disclosure so it sees the full tool list) re-asks the model up to
+  `MAX_INVALID_CALL_RETRIES` times with a correction (`invalid_arguments`, or `unknown_tool` for a
+  tool not offered) that is never saved to the thread; after that the response ends with a
+  visible notice. The stream shows the same notice live for every malformed attempt.
 - **Dedup and React keys are `type:id`, not `id`.** Ids are unique only within a type (an AI
   message's synthetic id and a tool message's call id are different namespaces).
 

docs/MCP_DISCLOSURE.md

@@ -151,7 +151,8 @@ load_tools({ query: string; server?: string; limit?: number }); // limit default
 re-sent, and it can never be removed; it also loses real tool names and `tool_choice`.
 
 **Why nothing is removed.** Changing the tool list costs a prompt-cache miss, so loaded tools stay
-for the thread and `activeTools` keeps load order, leaving the existing array as a stable prefix.
+for the thread — even after their server is removed or turned off, since their definitions live in
+the thread, not the catalog (deleting a server cascade-deletes its catalog row) and `activeTools` keeps load order, leaving the existing array as a stable prefix.
 Each load still costs one miss on that prefix: measure it with the cache attributes in
 [OBSERVABILITY.md](OBSERVABILITY.md). A thread only grows by tools the model asked for.
 
@@ -185,13 +186,18 @@ interrupt carrying the authorization URL, with a Connect button in the chat) is
 
 No MCP tool is registered with `createAgent`. Instead:
 
-1. **Stand-ins from the catalog.** `catalogTools()` builds one tool per cached entry
-   (`server_tool`, its description, and its `inputSchema` exactly as the server listed it). `wrapModelCall` adds the loaded stand-ins to `request.tools` —
-   LangChain accepts tools added there as long as a `wrapToolCall` runs them. A stand-in throws if it is ever run directly.
+1. **Stand-ins from the thread's saved definitions.** When `load_tools` loads a tool, its
+   definition (`server_tool`, its description, and its `inputSchema` exactly as the server listed
+   it) is saved in the thread's `toolDefinitions`. `wrapModelCall` builds a stand-in from each
+   (`standInTool()`) and adds them to `request.tools` — LangChain accepts tools added there as long
+   as a `wrapToolCall` runs them. A stand-in throws if it is ever run directly. A thread from
+   before definitions were saved has names only, and falls back to the catalog's stand-ins.
 2. **The real tool on call.** `wrapToolCall` asks `resolveTool(serverId, toolName)` for the real
    tool and passes it on as `request.tool`, which the tool node runs; result formatting stays the
    client library's. A tool the catalog does not list is refused (`unknown_tool`) without
-   connecting. A connection failure comes back as an error result (`connect_failed`,
+   connecting; with no catalog this request, the live server decides. A loaded tool whose server
+   was removed or turned off is answered with `server_unavailable` (naming the server and the
+   Connections page), never passed on to the tool node, which knows no MCP tools. A connection failure comes back as an error result (`connect_failed`,
    `auth_required`, `timeout`) and does not load the tool.
 3. **Auto-load.** A call to a catalog tool that was not loaded still runs it and loads it
    (`autoLoadOnCall`), which only happens when the model guesses a name.
@@ -245,8 +251,20 @@ Code defaults in `packages/mcp-disclosure/src/types.ts`; the pool's idle timeout
 ## State (per thread, via the checkpointer)
 
 `activeTools: string[]` holds `server_tool` names in load order, through an appending, de-duplicating
-reducer, so two `load_tools` calls in one step both apply. A tool whose server was disabled or
-removed is left out when the list is built; no state change is needed.
+reducer, so two `load_tools` calls in one step both apply.
+
+`toolDefinitions` maps each loaded name to `{ server, tool }`, the definition as it was loaded. Its
+reducer keeps the **first** definition saved for a name, so what the model is shown never changes,
+even if the server later lists a different schema (the real tool still validates the call against
+its current one). `load_tools` also back-fills a definition for a tool loaded before this field
+existed. Neither field is ever pruned.
+
+Why it is in the state and not the catalog: a tool removed from the model's list mid-thread is
+still in the conversation, so the model calls it from memory with no schema. That produced an
+unparseable call (an `invalid_tool_call`) which the agent does not run, ending the turn with no
+error shown. Such calls are now retried by the app's `invalidToolCallMiddleware` (see
+`AGENTS.md`, Streaming Architecture), which tells the model a tool it isn't offered is not
+available.
 
 ## The catalog (`mcp_server_catalog`)
 

packages/mcp-disclosure/src/catalogTools.ts

@@ -1,6 +1,6 @@
 import { DynamicStructuredTool, type StructuredToolInterface } from "@langchain/core/tools";
 import { prefixedToolName } from "./policy";
-import type { DisclosureServer } from "./types";
+import type { DisclosureServer, McpToolDefinition } from "./types";
 
 export class CatalogToolNotRunnable extends Error {}
 
@@ -16,20 +16,22 @@ export function catalogTools(servers: DisclosureServer[]): Map<string, Structure
     if (!server.catalog) continue;
     byServer.set(
       server.id,
-      server.catalog.tools.map((t) => {
-        const name = prefixedToolName(server.name, t.name);
-        return new DynamicStructuredTool({
-          name,
-          description: t.description ?? "",
-          schema: t.inputSchema as Record<string, unknown>,
-          func: async () => {
-            throw new CatalogToolNotRunnable(
-              `${name} is a catalog stand-in; it must be run through the disclosure middleware.`,
-            );
-          },
-        });
-      }),
+      server.catalog.tools.map((t) => standInTool(prefixedToolName(server.name, t.name), t)),
     );
   }
   return byServer;
 }
+
+/** One stand-in. The same definition always yields the same tool the model sees. */
+export function standInTool(name: string, definition: McpToolDefinition): StructuredToolInterface {
+  return new DynamicStructuredTool({
+    name,
+    description: definition.description ?? "",
+    schema: definition.inputSchema as Record<string, unknown>,
+    func: async () => {
+      throw new CatalogToolNotRunnable(
+        `${name} is a catalog stand-in; it must be run through the disclosure middleware.`,
+      );
+    },
+  });
+}

packages/mcp-disclosure/src/middleware.test.ts

@@ -13,11 +13,14 @@ import type { DisclosureOptions, DisclosureServer, ResolveMcpTool } from "./type
 type State = { activeTools: string[] };
 type Step = { tools?: [string, Record<string, unknown>][]; text?: string };
 
+let modelSeq = 0;
+
 /** Plays a script of tool calls and records what each model call was shown. */
 class ScriptedModel extends BaseChatModel {
   seen: { tools: string[]; system: string }[] = [];
   private bound: string[] = [];
   private n = 0;
+  private seq = ++modelSeq;
 
   constructor(private script: Step[]) {
     super({});
@@ -37,7 +40,8 @@ class ScriptedModel extends BaseChatModel {
     const system = messages.find((m) => m.getType() === "system");
     this.seen.push({ tools: [...this.bound], system: system?.text ?? "" });
     const step = this.script.shift() ?? { text: "done" };
-    const id = `m${++this.n}`;
+    // Unique across models, so a second agent on the same thread appends rather than replaces.
+    const id = `s${this.seq}-m${++this.n}`;
     const message = new AIMessage({
       id,
       content: step.text ?? "",
@@ -312,9 +316,10 @@ describe("disclosure", () => {
     expect(result).toMatchObject({ ok: false, error: "tool_not_loaded" });
   });
 
-  it("drops a loaded tool whose server is no longer available", async () => {
-    const checkpointer = new MemorySaver();
-    const make = (using: DisclosureServer[], model: ScriptedModel) =>
+  describe("a loaded tool outlives its server", () => {
+    const config = { configurable: { thread_id: "t1" } };
+    const input = { messages: [{ role: "user", content: "hi" }] };
+    const make = (checkpointer: MemorySaver, using: DisclosureServer[], model: ScriptedModel) =>
       createAgent({
         model,
         tools: [fakeTool("log_expense")],
@@ -327,15 +332,85 @@ describe("disclosure", () => {
           }),
         ],
       });
-    const config = { configurable: { thread_id: "t1" } };
-    const input = { messages: [{ role: "user", content: "hi" }] };
-    const first = new ScriptedModel([{ tools: [["load_tools", { query: "notes" }]] }]);
-    await make(servers, first).invoke(input, config);
-    expect(first.seen.at(-1)!.tools).toContain("notes_search_notes");
+    const loadNotes = async (checkpointer: MemorySaver) => {
+      const first = new ScriptedModel([{ tools: [["load_tools", { query: "notes" }]] }]);
+      await make(checkpointer, servers, first).invoke(input, config);
+      expect(first.seen.at(-1)!.tools).toContain("notes_search_notes");
+    };
+    const lastToolResult = (out: { messages: BaseMessage[] }) =>
+      JSON.parse(String(out.messages.filter((m) => m.getType() === "tool").at(-1)!.content));
+
+    it("keeps showing it after the server is removed, and saves its definition", async () => {
+      const checkpointer = new MemorySaver();
+      await loadNotes(checkpointer);
+      const state = (await make(checkpointer, servers, new ScriptedModel([])).getState(
+        config,
+      )) as unknown as { values: { toolDefinitions: Record<string, unknown> } };
+      expect(state.values.toolDefinitions.notes_search_notes).toEqual({
+        server: "notes",
+        tool: expect.objectContaining({ name: "search_notes", description: "fake search_notes" }),
+      });
+
+      // Removed (or turned off): the server is no longer in the list the agent is built with.
+      for (const using of [[servers[0]], []]) {
+        const next = new ScriptedModel([]);
+        await make(checkpointer, using, next).invoke(input, config);
+        expect(next.seen[0].tools).toContain("notes_search_notes");
+      }
+    });
 
-    const second = new ScriptedModel([]);
-    await make([servers[0]], second).invoke(input, config);
-    expect(second.seen[0].tools).not.toContain("notes_search_notes");
+    it("answers a call to it with server_unavailable, without connecting", async () => {
+      const checkpointer = new MemorySaver();
+      await loadNotes(checkpointer);
+      connected.length = 0;
+      const next = new ScriptedModel([{ tools: [["notes_search_notes", {}]] }]);
+      const out = await make(checkpointer, [servers[0]], next).invoke(input, config);
+      expect(lastToolResult(out)).toMatchObject({
+        ok: false,
+        error: "server_unavailable",
+        server: "notes",
+      });
+      expect(connected).toEqual([]);
+    });
+
+    it("handles a thread saved before definitions were, by name only", async () => {
+      const checkpointer = new MemorySaver();
+      const agent = make(checkpointer, servers, new ScriptedModel([]));
+      await agent.invoke(input, config);
+      // The old state shape: a loaded name, no saved definition.
+      await (
+        agent as unknown as { updateState: (c: object, v: object) => Promise<unknown> }
+      ).updateState(config, { activeTools: ["notes_search_notes"] });
+
+      // Server still there: shown from the catalog, as before.
+      const present = new ScriptedModel([]);
+      await make(checkpointer, servers, present).invoke(input, config);
+      expect(present.seen[0].tools).toContain("notes_search_notes");
+
+      // Server gone: nothing to show it from, but a call still gets a clear answer.
+      const gone = new ScriptedModel([{ tools: [["notes_search_notes", {}]] }]);
+      const out = await make(checkpointer, [servers[0]], gone).invoke(input, config);
+      expect(gone.seen[0].tools).not.toContain("notes_search_notes");
+      expect(lastToolResult(out)).toMatchObject({
+        error: "server_unavailable",
+        tool: "notes_search_notes",
+      });
+    });
+
+    it("keeps the definition first saved, even if the catalog changes", async () => {
+      const checkpointer = new MemorySaver();
+      await loadNotes(checkpointer);
+      const changed = makeServer("notes", { toolNames: ["search_notes"] });
+      changed.catalog!.tools[0].description = "a newer description";
+      const next = new ScriptedModel([{ tools: [["load_tools", { query: "search_notes" }]] }]);
+      await make(checkpointer, [servers[0], changed], next).invoke(input, config);
+      const state = (await make(checkpointer, servers, new ScriptedModel([])).getState(
+        config,
+      )) as unknown as { values: { toolDefinitions: Record<string, { tool: object }> } };
+      expect(state.values.toolDefinitions.notes_search_notes.tool).toMatchObject({
+        description: "fake search_notes",
+      });
+    });
   });
 });