mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: make the transcript's MCP mark survive a reload without a warm cache
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw
This commit is contained in:
co-authored by
Claude Opus 5
parent
3e93457898
commit
bcc51e3b31
@@ -41,7 +41,6 @@
|
||||
import ExpandableImage from '$lib/components/common/image/ExpandableImage.svelte'
|
||||
import McpServerIcon from '$lib/components/mcp/McpServerIcon.svelte'
|
||||
import { resolveMcpServerMark } from '$lib/components/mcp/serverMark'
|
||||
import { cachedProviderMark } from '$lib/components/mcp/iconCache'
|
||||
import { mcpServerForToolName } from './global/mcpTools'
|
||||
|
||||
interface Props {
|
||||
@@ -50,34 +49,17 @@
|
||||
|
||||
let { message }: Props = $props()
|
||||
|
||||
// A call records its server on the row, which is what survives a reload. The rest
|
||||
// is for rows written before it did: the generic wrappers name their server in the
|
||||
// arguments, and a registered tool is resolved through the in-memory registry.
|
||||
const MCP_CALL_TOOLS = ['call_mcp_read_tool', 'call_mcp_write_tool']
|
||||
const mcpServerPath = $derived.by(() => {
|
||||
const claimed = (() => {
|
||||
if (message.mcpServer) return message.mcpServer
|
||||
const name = message.toolName
|
||||
if (!name) return undefined
|
||||
if (MCP_CALL_TOOLS.includes(name)) {
|
||||
const server = message.parameters?.server
|
||||
return typeof server === 'string' ? server : undefined
|
||||
}
|
||||
return mcpServerForToolName(aiChatManager.mcpOwnerId, name)
|
||||
})()
|
||||
if (!claimed) return undefined
|
||||
// Both sources are ultimately a path the model wrote, and resolving one reads
|
||||
// that resource — so it has to name a server the user connected, not any
|
||||
// workspace resource the model can point at.
|
||||
if (aiChatManager.mcpServers.some((s) => s.path === claimed)) return claimed
|
||||
// A session runtime does not populate `mcpServers` until its first send, so on a
|
||||
// reloaded transcript the live list is empty and the rows this path was persisted
|
||||
// for would go unmarked. The local icon cache is the second witness: it only ever
|
||||
// holds servers this user's own MCP list resolved, and answering from it costs
|
||||
// neither a read nor a request.
|
||||
const workspace = aiChatManager.operatingWorkspace
|
||||
return workspace && cachedProviderMark(workspace, claimed) ? claimed : undefined
|
||||
})
|
||||
const mcpServerPath = $derived(
|
||||
// `mcpServer` is written by the call itself, from the connected-server list rather
|
||||
// than from the model's arguments, so it needs no second opinion here — which is
|
||||
// what lets a reloaded transcript resolve it. Nothing else is consulted: the live
|
||||
// server list is empty on a session until its first send, and the model's own
|
||||
// `parameters.server` is a path it could point anywhere.
|
||||
message.mcpServer ??
|
||||
(message.toolName
|
||||
? mcpServerForToolName(aiChatManager.mcpOwnerId, message.toolName)
|
||||
: undefined)
|
||||
)
|
||||
|
||||
const isPlanReview = $derived(message.toolName === EXIT_PLAN_MODE_TOOL)
|
||||
const isPlanCard = $derived(isPlanCardTool(message.toolName))
|
||||
|
||||
@@ -173,10 +173,9 @@ describe('loaded remote tools', () => {
|
||||
expect(registered?.def.function.parameters).toEqual(TOOLS[0].inputSchema)
|
||||
})
|
||||
|
||||
// The row's provider icon is resolved from this, and the registry of loaded tools
|
||||
// lives only in memory — so without it on the message, a reloaded transcript loses
|
||||
// every mark. Recorded before the server is resolved, so an unreachable server
|
||||
// still marks its own failure row.
|
||||
// The row's provider mark is resolved from this alone — the registry lives only in
|
||||
// memory and the live server list is empty on a reloaded session — so it has to land
|
||||
// on the message, and before the listing, which an unreachable server never answers.
|
||||
it('records the server on the row, even when it cannot be reached', async () => {
|
||||
getMcpToolsMock.mockRejectedValue(new Error('connection refused'))
|
||||
const callbacks = createToolCallbacks()
|
||||
@@ -194,6 +193,24 @@ describe('loaded remote tools', () => {
|
||||
})
|
||||
})
|
||||
|
||||
// The transcript trusts this field without re-checking it, so it must come from the
|
||||
// connected list rather than from the arguments — otherwise a path the model made up
|
||||
// would be resolved, and resolving one reads that workspace resource.
|
||||
it('records nothing for a server the user has not connected', async () => {
|
||||
const callbacks = createToolCallbacks()
|
||||
|
||||
await getTool('call_mcp_read_tool').fn({
|
||||
args: { server: 'u/hugo/not_connected', tool: 'get_issue', arguments: {} },
|
||||
workspace: 'test-ws',
|
||||
helpers: {},
|
||||
toolCallbacks: callbacks,
|
||||
toolId: 'tool-1'
|
||||
})
|
||||
|
||||
const recorded = callbacks.setToolStatus.mock.calls.map(([, meta]: [string, any]) => meta)
|
||||
expect(recorded.some((m: any) => m?.mcpServer !== undefined)).toBe(false)
|
||||
})
|
||||
|
||||
// Several chats are live at once — the docked one plus a warm runtime per session,
|
||||
// all in GLOBAL mode. A shared registry would put one chat's remote tools into
|
||||
// another's request, and let either one's "New chat" drop the other's.
|
||||
|
||||
@@ -523,10 +523,12 @@ function createCallTool(owner: string, servers: McpServer[], mode: 'read' | 'wri
|
||||
}),
|
||||
fn: async ({ args, workspace, toolId, toolCallbacks }) => {
|
||||
const parsed = callMcpToolSchema.parse(args)
|
||||
// Recorded from the arguments, before resolution: resolving needs a live
|
||||
// listing, so a server that cannot be reached would otherwise leave its own
|
||||
// failure row unmarked.
|
||||
toolCallbacks.setToolStatus(toolId, { mcpServer: parsed.server })
|
||||
// Matched against the connected list rather than taken from the arguments, so
|
||||
// what lands on the row is a server the user connected and the transcript can
|
||||
// resolve it without re-deciding that. Done before the listing, which needs the
|
||||
// server to be reachable — an unreachable one still marks its own failure row.
|
||||
const named = servers.find((s) => s.path === parsed.server)
|
||||
if (named) toolCallbacks.setToolStatus(toolId, { mcpServer: named.path })
|
||||
// Listing is a live call to a third party: a server that has gone away
|
||||
// must fail this tool, not the chat loop around it.
|
||||
let resolved: Awaited<ReturnType<typeof resolveTool>>
|
||||
|
||||
Reference in New Issue
Block a user