starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: reconcile MCP refusals against the connected servers

Browse Source 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw
This commit is contained in:
Guilhem Lemouel
2026-09-10 21:02:15 +02:00
co-authored by Claude Opus 5
parent a3eebf8428
commit 3647067714
5 changed files with 112 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts
+8 -17
View File
@@ -145,8 +145,8 @@ import {
forgetLoadedMcpTools,
invalidateMcpRegistrations,
isRequestBodyRejection,
reconcileMcpRegistry,
withdrawMcpToolsAfterRejection,
loadedMcpServers,
loadedMcpTools,
loadMcpServers,
type McpServer
@@ -2261,16 +2261,10 @@ export class AIChatManager {
// workspace's servers installed would go on advertising its paths against
// the workspace switched to.
this.mcpServers = workspace === (this.operatingWorkspace ?? '') ? servers : []
// A tool registered from a server that has since been turned off, deleted, or
// left behind by a workspace switch would still be callable, and would run
// against whichever workspace the chat is on now. The revision is checked too:
// a registered call bypasses the listing cache, so a connection edited elsewhere
// would otherwise keep running against the schema it was frozen with.
const live = new Map(this.mcpServers.map((s) => [s.path, s.editedAt]))
// The reconcile below only reaches servers that already registered something. A
// search still awaiting its listing has registered nothing yet, so a server turned
// off during that await would register after the fact and be advertised on the next
// iteration — bumping the generation makes that search drop its results instead.
// A search still awaiting its listing has registered nothing yet, so the reconcile
// below cannot reach it: a server turned off during that await would register after
// the fact and be advertised on the next iteration. Bumping the generation makes
// that search drop its results instead.
const signature = this.mcpServers.map((s) => `${s.path}@${s.editedAt ?? ''}`).join(',')
if (signature !== this.mcpServersSignature) {
this.mcpServersSignature = signature
@@ -2278,17 +2272,14 @@ export class AIChatManager {
}
// A registered call runs against the workspace the chat is on when it is made, not
// the one it was registered in, and a fork carries the same resource path and
// `edited_at` as its parent — so the per-path reconcile below cannot tell those two
// `edited_at` as its parent — so the per-path reconcile cannot tell those two
// servers apart. A workspace change drops the lot instead.
if (this.mcpRegistryWorkspace !== undefined && this.mcpRegistryWorkspace !== workspace) {
forgetLoadedMcpTools(this.mcpOwnerId)
}
this.mcpRegistryWorkspace = workspace
for (const { path, editedAt } of loadedMcpServers(this.mcpOwnerId)) {
if (!live.has(path) || live.get(path) !== editedAt) {
forgetLoadedMcpTools(this.mcpOwnerId, path)
}
}
// Tools and refusals frozen against a server that is now gone or has changed.
reconcileMcpRegistry(this.mcpOwnerId, this.mcpServers)
if (this.mode === AIMode.GLOBAL) {
this.configureGlobalMode()
}
frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte
+4 -1
View File
@@ -30,6 +30,7 @@ switch that decides whether this chat carries its tools.
import { untrack } from 'svelte'
import { getAiChatManager } from './aiChatManagerContext'
import { clearMcpToolsCache } from './global/mcpTools'
import { forgetMcpServerMarks } from '$lib/components/mcp/serverMark'
let {
ws,
@@ -390,8 +391,10 @@ switch that decides whether this chat carries its tools.
async function refresh(target = ws) {
// A path can be reconnected to a different server, so the cached tool list
// (and the readOnlyHint the confirmation gate reads) must not survive.
// (and the readOnlyHint the confirmation gate reads) must not survive — nor the
// provider mark the transcript's call rows show.
clearMcpToolsCache()
forgetMcpServerMarks()
await loadServers(target)
// `refreshMcpServers` blanks the list when the workspace it is handed is not
// the one the chat is on, so a refresh landing after a switch would take B's
frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts
+47 -2
View File
@@ -37,6 +37,7 @@ import {
forgetLoadedMcpTools,
invalidateMcpRegistrations,
isRequestBodyRejection,
reconcileMcpRegistry,
withdrawMcpToolsAfterRejection,
loadedMcpServers,
mcpRegistryGeneration,
@@ -480,18 +481,62 @@ describe('request rejections that withdraw registered tools', () => {
expect(loadedMcpTools(OWNER)).toEqual([])
})
it('registers a refused tool again once its server is reconnected', () => {
// Two servers whose schemas the provider will not take: keeping only the latest
// refusal lets them take turns poisoning the conversation forever.
it('keeps refusing every tool a rejection withdrew, not just the last', () => {
const [a, b] = [{ path: 'u/hugo/a_mcp' }, { path: 'u/hugo/b_mcp' }]
registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), a, [TOOLS[0]])
withdrawMcpToolsAfterRejection(OWNER)
registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), b, [TOOLS[0]])
withdrawMcpToolsAfterRejection(OWNER)
expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), a, [TOOLS[0]])).toEqual([
undefined
])
expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), b, [TOOLS[0]])).toEqual([
undefined
])
})
// A refused tool is not registered, so the reconcile has to lift its refusal from the
// connected list rather than from the registry — turning the server off is the escape.
it('registers a refused tool again once its server is turned off and back on', () => {
const server = SERVERS[0]
registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])
withdrawMcpToolsAfterRejection(OWNER)
forgetLoadedMcpTools(OWNER, server.path)
reconcileMcpRegistry(OWNER, [])
reconcileMcpRegistry(OWNER, [server])
expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([
'mcp_u_hugo_github_mcp__get_issue'
])
})
it('registers a refused tool again once its server is edited', () => {
const server = { path: 'u/hugo/github_mcp', editedAt: '2026-01-01T00:00:00Z' }
registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])
withdrawMcpToolsAfterRejection(OWNER)
reconcileMcpRegistry(OWNER, [{ ...server, editedAt: '2026-01-02T00:00:00Z' }])
expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([
'mcp_u_hugo_github_mcp__get_issue'
])
})
it('keeps refusing while its server is unchanged', () => {
const server = SERVERS[0]
registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])
withdrawMcpToolsAfterRejection(OWNER)
reconcileMcpRegistry(OWNER, [server])
expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([
undefined
])
})
it('separates a refused body from a refused account', () => {
expect(isRequestBodyRejection(400)).toBe(true)
expect(isRequestBodyRejection(422)).toBe(true)
frontend/src/lib/components/copilot/chat/global/mcpTools.ts
+44 -7
View File
@@ -125,8 +125,12 @@ const registries = new Map<string, OwnerRegistry>()
* otherwise a rotation or disposal during that await is silently undone.
*/
const generations = new Map<string, number>()
/** Registry keys `withdrawMcpToolsAfterRejection` will not let an owner register again. */
const refused = new Map<string, Set<string>>()
/**
* Registry keys `withdrawMcpToolsAfterRejection` will not let an owner register again,
* against the server revision each was refused at — so the refusal lifts when that
* server is edited, turned off, or reconnected, and holds otherwise.
*/
const refused = new Map<string, Map<string, string | undefined>>()
/** Bumped by the all-owner clear. Folded into the token below so it also invalidates an
* owner that has registered nothing yet, and is therefore in neither map. */
let allGeneration = 0
@@ -227,13 +231,46 @@ export function isRequestBodyRejection(status: number | undefined): boolean {
* was just refused, and the next request fails the same way. Which schema was at fault
* is not knowable from the error, so every tool that was loaded goes on the list — each
* falls back to the free-form wrapper, which is how they were reached before they could
* be registered at all. A server turned off, edited, or reconnected clears its entries,
* as does a new conversation.
* be registered at all. Refusals accumulate across rejections: dropping the earlier ones
* would let two bad schemas take turns poisoning the conversation forever.
*
* `reconcileMcpRegistry` lifts a refusal when its server changes, and a new conversation
* clears the lot.
*/
export function withdrawMcpToolsAfterRejection(owner: string) {
const keys = [...(registries.get(owner)?.tools.keys() ?? [])]
const registry = registries.get(owner)
const frozen = [...(registry?.tools.keys() ?? [])].map(
(key) => [key, registry?.editedAt.get(key)] as const
)
// Held across the drop, which clears the owner's refusals along with its tools.
const carried = refused.get(owner) ?? new Map<string, string | undefined>()
forgetLoadedMcpTools(owner)
if (keys.length > 0) refused.set(owner, new Set(keys))
for (const [key, editedAt] of frozen) carried.set(key, editedAt)
if (carried.size > 0) refused.set(owner, carried)
}
/**
* Drop what a change in the connected servers makes stale: tools registered from a server
* that is gone or has been edited since, and the refusals recorded against it. Both are
* frozen copies of what a server said — a registered call bypasses the listing cache, and
* a refusal outlives the registry it was taken from, so neither may survive its server.
*
* The refusals are reconciled here rather than through the registry: a refused tool is by
* construction not registered, so nothing else in the chat can reach it.
*/
export function reconcileMcpRegistry(owner: string, servers: McpServer[]) {
const live = new Map(servers.map((s) => [s.path, s.editedAt]))
const stale = (path: string, editedAt: string | undefined) =>
!live.has(path) || live.get(path) !== editedAt
for (const { path, editedAt } of loadedMcpServers(owner)) {
if (stale(path, editedAt)) forgetLoadedMcpTools(owner, path)
}
const keys = refused.get(owner)
if (!keys) return
for (const [key, editedAt] of [...keys]) {
if (stale(serverPathOfKey(key), editedAt)) keys.delete(key)
}
if (keys.size === 0) refused.delete(owner)
}
/** Drop one owner's loaded tools, or only those belonging to one server. */
@@ -270,7 +307,7 @@ function clearRefused(owner: string, serverPath?: string) {
const keys = refused.get(owner)
if (!keys) return
const prefix = `${serverPath}::`
for (const key of [...keys]) {
for (const key of [...keys.keys()]) {
if (key.startsWith(prefix)) keys.delete(key)
}
if (keys.size === 0) refused.delete(owner)
frontend/src/lib/components/mcp/serverMark.ts
+9
View File
@@ -26,6 +26,15 @@ export function resolveMcpServerMark(workspace: string, path: string): Promise<M
return pending
}
/**
* Forget what was resolved, for the settings section to call when it reloads the
* connections: a path can be reconnected to a different provider, and a mark held for
* the life of the page would go on marking new call rows with the old provider's icon.
*/
export function forgetMcpServerMarks() {
marks.clear()
}
async function load(workspace: string, path: string): Promise<McpServerMark> {
const cached = cachedProviderMark(workspace, path)
if (cached) return { icon: await loadProviderIcon(cached.key) }