From 1f9aa45b9daa613af2e5c5a725b26621ff51c5cf Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:14:09 +0200 Subject: [PATCH] refactor(ai-chat): remove the API catalog tools from the global chat (#11247) * refactor(ai-chat): remove the API catalog tools from the global chat Co-Authored-By: Claude Opus 5 (1M context) * refactor(ai-chat): drop a stale comment from the benchmark fetch tests Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- ai_evals/adapters/frontend/mockBackend.ts | 124 +----- .../adapters/frontend/mockBackendApi.test.ts | 27 +- .../adapters/frontend/vitestAdapter.test.ts | 13 +- ai_evals/cases/global.yaml | 26 +- .../chat/global/apiCatalogTools.test.ts | 360 ---------------- .../copilot/chat/global/apiCatalogTools.ts | 396 ------------------ .../copilot/chat/global/core.test.ts | 8 +- .../components/copilot/chat/global/core.ts | 7 +- .../copilot/chat/global/flowRunTree.ts | 3 +- 9 files changed, 21 insertions(+), 943 deletions(-) delete mode 100644 frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts delete mode 100644 frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts diff --git a/ai_evals/adapters/frontend/mockBackend.ts b/ai_evals/adapters/frontend/mockBackend.ts index 950cc2e1b5..07ea82a58f 100644 --- a/ai_evals/adapters/frontend/mockBackend.ts +++ b/ai_evals/adapters/frontend/mockBackend.ts @@ -14,7 +14,6 @@ import type { DataMetric, DataTableTables, DataTableTableSchema, - EndpointTool, GetDraftForUserResponse, GetOwnDraftResponse, ListDraftsResponse, @@ -1020,119 +1019,6 @@ function buildBenchmarkApp(app: BenchmarkWorkspaceApp): AppWithLastVersion { } } -// ============= API endpoint catalog (McpService.listMcpTools + raw fetch) ============= -// The global chat's API catalog tools list endpoints via McpService and execute -// them with a plain relative fetch('/api/...'), which has no meaning in the -// vitest environment. A representative slice of the real catalog is served here, -// and `handleBenchmarkApiFetch` answers the executed calls. - -const BENCHMARK_MCP_TOOLS: EndpointTool[] = [ - { - name: 'listWorkers', - description: 'List workers', - instructions: 'List all workers with their last ping and job counts.', - path: '/workers/list', - method: 'GET', - query_params_schema: { - type: 'object', - properties: { page: { type: 'integer' }, per_page: { type: 'integer' } } - } - }, - { - name: 'listQueue', - description: 'List queued jobs', - instructions: '', - path: '/w/{workspace}/jobs/queue/list', - method: 'GET', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' } }, - required: ['workspace'] - } - }, - { - name: 'getJob', - description: 'get job', - instructions: '', - path: '/w/{workspace}/jobs_u/get/{id}', - method: 'GET', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, id: { type: 'string', format: 'uuid' } }, - required: ['workspace', 'id'] - }, - query_params_schema: { - type: 'object', - properties: { - no_logs: { type: 'boolean' }, - no_code: { type: 'boolean' }, - approval_token: { type: 'string' } - }, - required: [] - } - }, - { - name: 'runScriptByPath', - description: 'Run the deployed version of a script by path', - instructions: '', - path: '/w/{workspace}/jobs/run/p/{path}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, path: { type: 'string' } }, - required: ['workspace', 'path'] - }, - body_schema: { type: 'object', properties: {} } - }, - { - name: 'runFlowByPath', - description: 'Run the deployed version of a flow by path', - instructions: '', - path: '/w/{workspace}/jobs/run/f/{path}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, path: { type: 'string' } }, - required: ['workspace', 'path'] - }, - body_schema: { type: 'object', properties: {} } - }, - // Draft-covered endpoints, present so steering cases exercise the guard the - // way production does (hidden from search, refused at call time). - { - name: 'getScriptByPath', - description: 'Get a script by path', - instructions: '', - path: '/w/{workspace}/scripts/get/p/{path}', - method: 'GET' - }, - { - name: 'createFlow', - description: 'Create a flow', - instructions: '', - path: '/w/{workspace}/flows/create', - method: 'POST' - }, - { - name: 'deleteSchedule', - description: 'Delete a schedule', - instructions: '', - path: '/w/{workspace}/schedules/delete/{path}', - method: 'DELETE' - }, - { - name: 'getVariable', - description: 'Get a variable', - instructions: '', - path: '/w/{workspace}/variables/get/{path}', - method: 'GET' - } -] - -export function listBenchmarkMcpTools(): EndpointTool[] { - return BENCHMARK_MCP_TOOLS -} - /** A stand-in Windmill hub. `search_hub_scripts` and a `hub/` read go out over * relative `/api/...` fetches, which have no origin here, so without these the * hub tools throw and no case can exercise hub reuse. Serving fixtures rather @@ -1295,7 +1181,8 @@ const BENCHMARK_WORKERS = [ const BENCHMARK_JOB_GET_PATH = /^\/api\/w\/([^/]+)\/jobs_u\/get\/([^/]+)$/ const BENCHMARK_RUN_BY_PATH = /^\/api\/w\/([^/]+)\/jobs\/run\/(p|f)\/([^/]+)$/ -/** `executeEndpoint` sends a JSON string; anything else means no args were supplied. */ +/** An intercepted request carries its args as a JSON string; anything else means none + * were supplied. */ function parseBenchmarkRequestBody( body: BodyInit | null | undefined ): Record | undefined { @@ -1325,15 +1212,13 @@ export function hasBenchmarkApiHandler(url: string): boolean { path === '/api/workers/list' || BENCHMARK_JOB_GET_PATH.test(path) || BENCHMARK_RUN_BY_PATH.test(path) || - /^\/api\/w\/[^/]+\/jobs\/queue\/list$/.test(path) || path === '/api/embeddings/query_hub_scripts' || path.startsWith('/api/scripts/hub/get_full/') || BENCHMARK_AI_MODELS_PATH.test(path) ) } -/** Answer a relative `/api/...` fetch — from the API catalog executor, or from the - * chat's hub tools. */ +/** Answer the relative `/api/...` fetches no mocked service covers. */ export function handleBenchmarkApiFetch(url: string, init?: RequestInit): Response { const path = url.split('?')[0] if (path === '/api/workers/list') { @@ -1350,9 +1235,6 @@ export function handleBenchmarkApiFetch(url: string, init?: RequestInit): Respon ?.aiProviders?.find((entry) => entry.path === resourcePath) return Response.json({ data: (seed?.models ?? []).map((id) => ({ id })) }) } - if (/^\/api\/w\/[^/]+\/jobs\/queue\/list$/.test(path)) { - return Response.json([]) - } const jobGet = BENCHMARK_JOB_GET_PATH.exec(path) if (jobGet) { const id = decodeURIComponent(jobGet[2]) diff --git a/ai_evals/adapters/frontend/mockBackendApi.test.ts b/ai_evals/adapters/frontend/mockBackendApi.test.ts index d9cfa08bf4..0e489a4408 100644 --- a/ai_evals/adapters/frontend/mockBackendApi.test.ts +++ b/ai_evals/adapters/frontend/mockBackendApi.test.ts @@ -4,40 +4,17 @@ import { getBenchmarkCompletedJob, handleBenchmarkApiFetch, hasBenchmarkApiHandler, - listBenchmarkMcpTools, resetBenchmarkMockBackend, registerBenchmarkWorkspaceRunnables } from './mockBackend' const WORKSPACE = 'benchmark-api-ws' -// A catalog entry with no fetch handler is a dead end: the catalog executor builds a -// relative `/api/...` url, the stub declines it, and node's fetch throws on the relative -// url instead of returning a result the model can act on. Mutating entries are reachable -// too — the eval runners define no `requestConfirmation`, so `call_api_endpoint` executes -// unconfirmed. -describe('benchmark API catalog', () => { +describe('benchmark API fetch handlers', () => { beforeEach(() => resetBenchmarkMockBackend()) afterEach(() => resetBenchmarkMockBackend()) - it('answers every endpoint it advertises', () => { - const unanswered = listBenchmarkMcpTools() - .map((tool) => - `/api${tool.path.replace('{workspace}', WORKSPACE)}`.replace(/\{[^}]+\}/g, 'x') - ) - .filter((url) => !hasBenchmarkApiHandler(url)) - - // The draft-covered entries are refused by name before any fetch, so they are - // advertised without a handler on purpose. - expect(unanswered).toEqual([ - `/api/w/${WORKSPACE}/scripts/get/p/x`, - `/api/w/${WORKSPACE}/flows/create`, - `/api/w/${WORKSPACE}/schedules/delete/x`, - `/api/w/${WORKSPACE}/variables/get/x` - ]) - }) - - it('runs a deployed script by path, the way call_api_endpoint reaches it', async () => { + it('runs a deployed script by path', async () => { registerBenchmarkWorkspaceRunnables(WORKSPACE, { scripts: [ { diff --git a/ai_evals/adapters/frontend/vitestAdapter.test.ts b/ai_evals/adapters/frontend/vitestAdapter.test.ts index c9e889b9a0..0c8a22a987 100644 --- a/ai_evals/adapters/frontend/vitestAdapter.test.ts +++ b/ai_evals/adapters/frontend/vitestAdapter.test.ts @@ -5,8 +5,8 @@ import { mkdir, writeFile } from 'fs/promises' import { dirname, resolve } from 'path' import { handleBenchmarkApiFetch, hasBenchmarkApiHandler } from './mockBackend' -// The API catalog executor issues relative fetch('/api/...') calls, which have -// no meaning in the vitest environment — serve the ones the benchmark handles. +// Some tools reach the backend by relative fetch('/api/...'), which has no meaning +// in the vitest environment — serve the ones the benchmark handles. // Every other relative fetch keeps its normal behavior (it fails the same way // it does without this stub) so unrelated tools see an unchanged environment. // The frontend builds API URLs from location.origin (fetchAvailableModels does), and node has no @@ -92,8 +92,7 @@ vi.mock('$lib/gen', async () => { runBenchmarkFlowPreview, runBenchmarkScriptByPath, runBenchmarkScriptPreview, - updateBenchmarkDraft, - listBenchmarkMcpTools + updateBenchmarkDraft } = await import('./mockBackend') function wrapService(target: T, overrides: Record): T { @@ -434,12 +433,6 @@ vi.mock('$lib/gen', async () => { queryResourceTypes: async (data: { workspace: string }) => hasBenchmarkWorkspace(data.workspace) ? [] : actual.ResourceService.queryResourceTypes(data) }), - McpService: wrapService(actual.McpService, { - listMcpTools: async (data: { workspace: string }) => - hasBenchmarkWorkspace(data.workspace) - ? listBenchmarkMcpTools() - : actual.McpService.listMcpTools(data) - }), VariableService: wrapService(actual.VariableService, { existsVariable: async (data: { workspace: string; path: string }) => hasBenchmarkWorkspace(data.workspace) diff --git a/ai_evals/cases/global.yaml b/ai_evals/cases/global.yaml index fdb2442729..b17d37cb29 100644 --- a/ai_evals/cases/global.yaml +++ b/ai_evals/cases/global.yaml @@ -1919,11 +1919,10 @@ - when the lookup fails, tells the user instead of inventing table names - does not write scripts or resources to answer a read-only question -# --- Dedicated tools preferred over the API catalog --- -# The harness serves worker/queue reads itself (benchmark fetch handlers in -# adapters/frontend), so these cases do not require an mcp-enabled eval backend. -# The stale `api-catalog` in the id below is kept so results stay comparable -# across benchmark runs. +# --- Dedicated tools for workspace, run and instance state --- +# The harness serves the worker read itself (a benchmark fetch handler in +# adapters/frontend). The stale `api-catalog` in the id below is kept so +# results stay comparable across benchmark runs. - id: global-test30-api-catalog-workers prompt: |- @@ -1937,8 +1936,6 @@ requiredToolsUsed: - list_workers forbiddenToolsUsed: - - call_api_get - - call_api_endpoint - write_script - deploy_workspace_item # Read-only workspace inspection produces no draft; validate via tool use. @@ -1990,13 +1987,11 @@ requiredToolsUsed: - test_run_script forbiddenToolsUsed: - - call_api_endpoint - deploy_workspace_item - delete_workspace_item # The judge only sees the drafts artifact and cannot observe runs, so it always # docks the prompt's "run it" requirement — validate deterministically instead: - # draft content via valueIncludes, the test run via toolExpect (test_run_script - # required, call_api_endpoint forbidden). + # draft content via valueIncludes, the test run via toolExpect. skipJudge: true judgeChecklist: - creates an AI draft of f/evals/global/format_greeting with the name uppercased in the greeting @@ -2014,13 +2009,11 @@ requiredToolsUsed: - delete_workspace_item forbiddenToolsUsed: - - call_api_endpoint - - search_api_endpoints - write_script # Deletion produces no draft; validate via tool use. skipJudge: true judgeChecklist: - - deletes the deployed script via delete_workspace_item rather than a raw API endpoint + - deletes the deployed script via delete_workspace_item - id: global-test33-run-deployed-script-with-form prompt: |- @@ -2041,7 +2034,6 @@ - read_workspace_item forbiddenToolsUsed: - test_run_script - - call_api_endpoint - write_script - deploy_workspace_item # An empty form pushes the work back onto the user, so the prefill is part of @@ -2054,7 +2046,7 @@ # Running produces no draft, and the judge cannot observe runs; validate via tool use. skipJudge: true judgeChecklist: - - runs the deployed script through run_script rather than a preview test run or a raw API endpoint + - runs the deployed script through run_script rather than a preview test run - passes the name "ada" so the confirmation form comes up prefilled - id: global-test34-run-with-secret-from-variable @@ -2113,7 +2105,6 @@ - read_workspace_item forbiddenToolsUsed: - test_run_flow - - call_api_endpoint - write_flow - deploy_workspace_item # An empty form pushes the work back onto the user, so the prefill is part of @@ -2126,7 +2117,7 @@ # Running produces no draft, and the judge cannot observe runs; validate via tool use. skipJudge: true judgeChecklist: - - runs the deployed flow through run_flow rather than a preview test run or a raw API endpoint + - runs the deployed flow through run_flow rather than a preview test run - passes the customer "acme" so the confirmation form comes up prefilled - id: global-test36-draft-flow-test-run-not-deployed @@ -2151,7 +2142,6 @@ # version instead — the edit would not be in what ran. forbiddenToolsUsed: - run_flow - - call_api_endpoint - deploy_workspace_item # The judge cannot observe runs, and the edit's content is already pinned by # global-test5 on this fixture; what this case guards is where the run went. diff --git a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts deleted file mode 100644 index e61015bba7..0000000000 --- a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts +++ /dev/null @@ -1,360 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' - -const { listMcpToolsMock } = vi.hoisted(() => ({ - listMcpToolsMock: vi.fn() -})) - -vi.mock('../shared', () => ({ - createToolDef: (_schema: unknown, name: string, description: string) => ({ - type: 'function', - function: { name, description, parameters: {} } - }) -})) - -vi.mock('$lib/gen', () => ({ - McpService: { - listMcpTools: listMcpToolsMock - } -})) - -import { apiCatalogTools, clearApiCatalogCache } from './apiCatalogTools' - -const CATALOG = [ - { - name: 'listWorkers', - description: 'List workers', - instructions: 'List all workers with their ping status', - path: '/workers/list', - method: 'GET', - query_params_schema: { - type: 'object', - properties: { page: { type: 'integer' }, per_page: { type: 'integer' } } - } - }, - { - name: 'listDataMetrics', - description: 'List declared measures and dimensions', - instructions: '', - path: '/w/{workspace}/data_metrics/list', - method: 'GET' - }, - { - name: 'listQueue', - description: 'List queued jobs', - instructions: 'List the jobs waiting in the queue', - path: '/w/{workspace}/jobs/queue/list', - method: 'GET', - query_params_schema: { - type: 'object', - properties: { running: { type: 'boolean' }, per_page: { type: 'integer' } } - } - }, - { - name: 'getJobUpdates', - description: 'Get job updates', - instructions: '', - path: '/w/{workspace}/jobs_u/getupdate/{id}', - method: 'GET', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, id: { type: 'string' } }, - required: ['workspace', 'id'] - } - }, - { - name: 'getJob', - description: 'Get job details', - instructions: '', - path: '/w/{workspace}/jobs_u/get/{id}', - method: 'GET', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, id: { type: 'string' } }, - required: ['workspace', 'id'] - } - }, - { - name: 'deleteSchedule', - description: 'Delete a schedule', - instructions: '', - path: '/w/{workspace}/schedules/delete/{path}', - method: 'DELETE', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, path: { type: 'string' } }, - required: ['workspace', 'path'] - } - }, - { - name: 'createFlow', - description: 'Create a flow', - instructions: '', - path: '/w/{workspace}/flows/create', - method: 'POST' - }, - { - name: 'getVariable', - description: 'Get variable', - instructions: '', - path: '/w/{workspace}/variables/get/{path}', - method: 'GET' - }, - { - name: 'getScriptByPath', - description: 'Get script by path', - instructions: '', - path: '/w/{workspace}/scripts/get/p/{path}', - method: 'GET' - }, - { - name: 'getAppByPath', - description: 'Get app by path', - instructions: 'Returns the app source', - path: '/w/{workspace}/apps/get/p/{path}', - method: 'GET' - }, - { - name: 'deleteScriptByHash', - description: 'Delete a script by hash', - instructions: '', - path: '/w/{workspace}/scripts/delete/h/{hash}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, hash: { type: 'string' } }, - required: ['workspace', 'hash'] - } - }, - { - name: 'runScriptByPath', - description: 'Run script by path', - instructions: 'Trigger a run of a deployed script', - path: '/w/{workspace}/jobs/run/p/{path}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, path: { type: 'string' } }, - required: ['workspace', 'path'] - }, - body_schema: { type: 'object', additionalProperties: true } - }, - { - name: 'cancelQueuedJob', - description: 'Cancel a queued job', - instructions: '', - path: '/w/{workspace}/jobs_u/queue/cancel/{id}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, id: { type: 'string' } }, - required: ['workspace', 'id'] - }, - body_schema: { type: 'object', additionalProperties: true } - }, - { - name: 'runFlowByPath', - description: 'Run flow by path', - instructions: 'Trigger a run of a deployed flow', - path: '/w/{workspace}/jobs/run/f/{path}', - method: 'POST', - path_params_schema: { - type: 'object', - properties: { workspace: { type: 'string' }, path: { type: 'string' } }, - required: ['workspace', 'path'] - }, - body_schema: { - type: 'object', - properties: { args: { type: 'object' } } - } - } -] - -function createToolCallbacks() { - return { - setToolStatus: vi.fn(), - removeToolStatus: vi.fn() - } as any -} - -function getTool(name: string) { - const tool = apiCatalogTools.find((entry) => entry.def.function.name === name) - if (!tool) throw new Error(`${name} tool not found`) - return tool -} - -async function run(name: string, args: Record) { - const raw = await getTool(name).fn({ - args, - workspace: 'test-ws', - helpers: {}, - toolCallbacks: createToolCallbacks(), - toolId: 'tool-1' - }) - return JSON.parse(raw) -} - -beforeEach(() => { - vi.clearAllMocks() - clearApiCatalogCache() - listMcpToolsMock.mockResolvedValue(CATALOG) - vi.unstubAllGlobals() -}) - -describe('search_api_endpoints', () => { - it('matches on name/path tokens, plural-insensitively, and excludes covered endpoints', async () => { - // Singular "job" matches the plural "jobs" path segment. - const result = await run('search_api_endpoints', { query: 'list queued job' }) - expect(result.matches[0].name).toBe('listQueue') - expect(result.matches[0].endpoint).toBe('GET /w/{workspace}/jobs/queue/list') - expect(result.matches[0].params).toEqual(['running', 'per_page']) - expect(result.matches[0].instructions).toContain('waiting in the queue') - - const flows = await run('search_api_endpoints', { query: 'create flow' }) - expect(flows.matches.map((m: any) => m.name)).not.toContain('createFlow') - expect(flows.covered_by_dedicated_tools).toContain('createFlow → use write_flow') - }) - - it('returns endpoint categories when nothing matches', async () => { - const result = await run('search_api_endpoints', { query: 'kubernetes' }) - expect(result.matches).toEqual([]) - expect(result.hint).toContain('jobs_u') - // Categories are built from the uncovered endpoints only, so a covered one - // must not be advertised as somewhere to retry. - expect(result.hint).not.toContain('workers') - expect(result.hint).not.toContain('data_metrics') - }) -}) - -describe('call_api_get', () => { - it('rejects covered, unknown, and non-GET endpoints with a pointer', async () => { - const covered = await run('call_api_get', { name: 'createFlow' }) - expect(covered.error).toContain('write_flow') - - const unknown = await run('call_api_get', { name: 'nope' }) - expect(unknown.error).toContain('search_api_endpoints') - - const mutating = await run('call_api_get', { name: 'cancelQueuedJob' }) - expect(mutating.error).toContain('call_api_endpoint') - - const job = await run('call_api_get', { name: 'getJob' }) - expect(job.error).toContain('get_run') - - const deleting = await run('call_api_endpoint', { name: 'deleteSchedule' }) - expect(deleting.error).toContain('delete_workspace_item') - - const byHash = await run('call_api_endpoint', { name: 'deleteScriptByHash' }) - expect(byHash.error).toContain('delete_workspace_item') - const search = await run('search_api_endpoints', { query: 'delete script' }) - expect(search.matches.map((m: any) => m.name)).not.toContain('deleteScriptByHash') - }) - - // Left reachable, these endpoints are the way around the argument form: they run the - // deployed runnable on the model's arguments, unstripped and unshown. - it('refuses a deployed run, pointing at run_script and run_flow', async () => { - for (const [name, tool, query] of [ - ['runScriptByPath', 'run_script', 'run deployed script'], - ['runFlowByPath', 'run_flow', 'run deployed flow'] - ]) { - const called = await run('call_api_endpoint', { name }) - expect(called.error).toContain(tool) - expect(called.success).toBe(false) - - // And it is gone from search, so the model is redirected before it ever calls. - const search = await run('search_api_endpoints', { query }) - expect(search.matches.map((m: any) => m.name)).not.toContain(name) - expect(search.covered_by_dedicated_tools?.join(' ')).toContain(tool) - } - }) - - it('refuses draft-blind item reads and lists, pointing at the draft-aware tools', async () => { - for (const name of ['getScriptByPath', 'getResource', 'getSchedule', 'getAppByPath']) { - const result = await run('call_api_get', { name }) - expect(result.success).toBe(false) - expect(result.error).toContain('read_workspace_item') - } - for (const name of ['listScripts', 'listFlows', 'listResource', 'listSchedules', 'listApps']) { - const result = await run('call_api_get', { name }) - expect(result.success).toBe(false) - expect(result.error).toContain('list_workspace_items') - } - - const search = await run('search_api_endpoints', { query: 'get script' }) - expect(search.matches.map((m: any) => m.name)).not.toContain('getScriptByPath') - // getAppByPath would hand the model the whole app source, so it must not even surface. - const appSearch = await run('search_api_endpoints', { query: 'get app' }) - expect(appSearch.matches.map((m: any) => m.name)).not.toContain('getAppByPath') - }) - - it('refuses the worker and data-metric reads, pointing at their dedicated tools', async () => { - for (const [name, tool, query] of [ - ['listWorkers', 'list_workers', 'workers'], - ['listDataMetrics', 'list_data_metrics', 'data metrics'] - ]) { - const result = await run('call_api_get', { name }) - expect(result.success).toBe(false) - expect(result.error).toContain(tool) - - const search = await run('search_api_endpoints', { query }) - expect(search.matches.map((m: any) => m.name)).not.toContain(name) - expect(search.covered_by_dedicated_tools?.join(' ')).toContain(tool) - } - }) - - it('refuses variable reads so variable values never reach the model', async () => { - const result = await run('call_api_get', { name: 'getVariable' }) - expect(result.success).toBe(false) - expect(result.error).toContain('never readable') - - const search = await run('search_api_endpoints', { query: 'variable' }) - expect(search.matches.map((m: any) => m.name)).not.toContain('getVariable') - }) - - it('returns the endpoint schema when a required path param is missing', async () => { - const result = await run('call_api_get', { name: 'getJobUpdates' }) - expect(result.success).toBe(false) - expect(result.error).toContain('id') - expect(result.schema.path_params_schema.required).toContain('id') - }) - - it('substitutes path params and sends the rest as query params', async () => { - const fetchMock = vi.fn().mockResolvedValue({ - ok: true, - headers: new Headers({ 'content-type': 'application/json' }), - json: async () => [{ id: 'job-1' }] - }) - vi.stubGlobal('fetch', fetchMock) - const result = await run('call_api_get', { name: 'listQueue', params: { running: true } }) - expect(fetchMock).toHaveBeenCalledWith('/api/w/test-ws/jobs/queue/list?running=true', { - method: 'GET' - }) - expect(result).toEqual({ success: true, data: [{ id: 'job-1' }] }) - }) -}) - -describe('call_api_endpoint', () => { - it('requires confirmation and executes mutating endpoints with a body', async () => { - expect(getTool('call_api_endpoint').requiresConfirmation).toBe(true) - const fetchMock = vi.fn().mockResolvedValue({ - ok: true, - headers: new Headers({ 'content-type': 'text/plain' }), - text: async () => 'job-id-1' - }) - vi.stubGlobal('fetch', fetchMock) - const result = await run('call_api_endpoint', { - name: 'cancelQueuedJob', - params: { id: 'job/1' }, - body: { args: { n: 1 } } - }) - expect(fetchMock).toHaveBeenCalledWith('/api/w/test-ws/jobs_u/queue/cancel/job%2F1', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ args: { n: 1 } }) - }) - expect(result).toEqual({ success: true, data: 'job-id-1' }) - }) - - it('redirects GET endpoints to call_api_get', async () => { - const result = await run('call_api_endpoint', { name: 'listQueue' }) - expect(result.error).toContain('call_api_get') - }) -}) diff --git a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts deleted file mode 100644 index 05eea477a5..0000000000 --- a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts +++ /dev/null @@ -1,396 +0,0 @@ -import { z } from 'zod' -import { McpService, type EndpointTool } from '$lib/gen' -import { createToolDef, type Tool } from '../shared' - -/** - * Generic access to the backend's MCP endpoint catalog (the endpoints marked - * `x-mcp-tool` in openapi.yaml) as three small static tools — search, GET - * call, mutating call — instead of one registered tool per endpoint. Search - * results carry parameter names and usage instructions; full parameter schemas - * enter the model's context only when a call fails validation. This keeps the - * per-iteration tool-schema cost constant. - */ - -// Endpoints whose job a dedicated global tool already does, plus the variable -// read endpoints. Hidden from search and refused at call time: the authoring -// and delete ones would bypass the draft lifecycle (conflict detection, -// explicit deploy, draft cleanup on delete), the variable reads would expose -// variable values to the model (getVariable even decrypts secrets by default), -// and the rest are exact duplicates that would fragment behavior across two -// code paths. -const COVERED_ENDPOINTS: Record = { - getVariable: 'read_workspace_item (variable values are never readable in chat)', - listVariable: 'list_workspace_items (variable values are never readable in chat)', - // The item read/list endpoints return deployed state only, blind to the user's - // drafts; read_workspace_item / list_workspace_items merge drafts, and for - // flows return the compact JSON that patch_flow_json matches against. - getScriptByPath: - 'read_workspace_item (reads your draft when one exists; pass version: "deployed" for the deployed state)', - getFlowByPath: - 'read_workspace_item (reads your draft when one exists; pass version: "deployed" for the deployed state)', - getResource: - 'read_workspace_item (reads your draft when one exists; pass version: "deployed" for the deployed state)', - getSchedule: - 'read_workspace_item (reads your draft when one exists; pass version: "deployed" for the deployed state)', - // getAppByPath returns the entire app source — every file, every runnable's - // script, lock and schema — where read_workspace_item returns paths and sizes. - getAppByPath: - 'read_workspace_item (app metadata only; read_app_file for contents. Reads your draft when one exists; pass version: "deployed" for the deployed state)', - listApps: 'list_workspace_items (it includes your drafts)', - listScripts: 'list_workspace_items (it includes your drafts)', - listFlows: 'list_workspace_items (it includes your drafts)', - listResource: 'list_workspace_items (it includes your drafts)', - listSchedules: 'list_workspace_items (it includes your drafts)', - runScriptByPath: 'run_script (it shows the user an argument form to confirm)', - runFlowByPath: 'run_flow (it shows the user an argument form to confirm)', - deleteScriptByPath: 'delete_workspace_item', - deleteScriptByHash: 'delete_workspace_item', - deleteFlowByPath: 'delete_workspace_item', - deleteSchedule: 'delete_workspace_item', - deleteVariable: 'delete_workspace_item', - deleteResource: 'delete_workspace_item', - createScript: 'write_script', - createFlow: 'write_flow', - updateFlow: 'patch_flow_json or write_flow', - createApp: 'init_app and the app draft tools', - updateApp: 'write_app_file / write_app_runnable', - createVariable: 'write_variable', - updateVariable: 'write_variable', - createResource: 'write_resource', - updateResource: 'write_resource', - createSchedule: 'write_schedule', - updateSchedule: 'write_schedule', - searchDocs: 'search_docs', - readDocsPage: 'read_docs_page', - listJobs: 'list_runs', - listWorkers: 'list_workers', - listDataMetrics: 'list_data_metrics', - getJob: 'get_run', - getJobLogs: 'get_run', - runScriptPreviewAndWaitResult: 'test_run_script' -} - -const MAX_SEARCH_RESULTS = 10 -const MAX_DESCRIPTION_CHARS = 200 -const MAX_INSTRUCTIONS_CHARS = 400 -const MAX_RESULT_CHARS = 20_000 - -let catalogCache: { workspace: string; endpoints: EndpointTool[] } | undefined - -async function loadCatalog(workspace: string): Promise { - if (catalogCache?.workspace !== workspace) { - const endpoints = await McpService.listMcpTools({ workspace }) - catalogCache = { workspace, endpoints } - } - return catalogCache.endpoints -} - -export function clearApiCatalogCache() { - catalogCache = undefined -} - -function tokenize(text: string): string[] { - return text - .replace(/([a-z0-9])([A-Z])/g, '$1 $2') - .toLowerCase() - .split(/[^a-z0-9]+/) - .filter((t) => t.length > 1) -} - -// Cheap plural-insensitive comparison so "workers" matches "worker" and vice versa. -function tokenMatches(token: string, queryToken: string): boolean { - const strip = (t: string) => (t.length > 3 && t.endsWith('s') ? t.slice(0, -1) : t) - return strip(token) === strip(queryToken) -} - -function pathSegments(path: string): string[] { - return path.split('/').filter((seg) => seg && seg !== 'w' && !seg.startsWith('{')) -} - -// Name and path tokens identify the operation; description tokens only support it. -function scoreEndpoint(endpoint: EndpointTool, queryTokens: string[]): number { - const nameTokens = [...tokenize(endpoint.name), ...pathSegments(endpoint.path).flatMap(tokenize)] - const descTokens = tokenize(`${endpoint.description} ${endpoint.instructions}`) - let score = 0 - for (const qt of queryTokens) { - if (nameTokens.some((t) => tokenMatches(t, qt))) score += 3 - else if (descTokens.some((t) => tokenMatches(t, qt))) score += 1 - } - return score -} - -function schemaPropertyNames(schema: unknown): string[] { - const properties = (schema as { properties?: Record } | null | undefined) - ?.properties - return properties ? Object.keys(properties).filter((k) => k !== 'workspace') : [] -} - -function truncate(text: string, max: number): string { - return text.length > max ? text.slice(0, max) + '…' : text -} - -function summarizeEndpoint(endpoint: EndpointTool) { - const params = [ - ...schemaPropertyNames(endpoint.path_params_schema), - ...schemaPropertyNames(endpoint.query_params_schema) - ] - const bodyParams = schemaPropertyNames(endpoint.body_schema) - const instructions = endpoint.instructions.trim() - return { - name: endpoint.name, - endpoint: `${endpoint.method.toUpperCase()} ${endpoint.path}`, - description: truncate(endpoint.description, MAX_DESCRIPTION_CHARS), - ...(instructions ? { instructions: truncate(instructions, MAX_INSTRUCTIONS_CHARS) } : {}), - ...(params.length > 0 ? { params } : {}), - ...(bodyParams.length > 0 ? { body_params: bodyParams } : {}) - } -} - -function endpointSchemaHelp(endpoint: EndpointTool) { - return { - name: endpoint.name, - endpoint: `${endpoint.method.toUpperCase()} ${endpoint.path}`, - path_params_schema: endpoint.path_params_schema ?? undefined, - query_params_schema: endpoint.query_params_schema ?? undefined, - body_schema: endpoint.body_schema ?? undefined - } -} - -async function resolveEndpoint( - workspace: string, - name: string -): Promise<{ endpoint: EndpointTool } | { error: string }> { - const covered = COVERED_ENDPOINTS[name] - if (covered) { - return { error: `"${name}" is covered by the dedicated ${covered} tool — use it instead.` } - } - const catalog = await loadCatalog(workspace) - const endpoint = catalog.find((e) => e.name === name) - if (!endpoint) { - return { - error: `Unknown endpoint "${name}". Use search_api_endpoints to find the endpoint name.` - } - } - return { endpoint } -} - -async function executeEndpoint( - endpoint: EndpointTool, - workspace: string, - params: Record, - body?: unknown -): Promise { - let url = `/api${endpoint.path.replace('{workspace}', encodeURIComponent(workspace))}` - const queryParams = new URLSearchParams() - for (const [key, value] of Object.entries(params)) { - if (value === undefined || value === null) continue - if (url.includes(`{${key}}`)) { - url = url.replace(`{${key}}`, encodeURIComponent(String(value))) - } else { - queryParams.append(key, String(value)) - } - } - const unresolved = [...url.matchAll(/\{([^}]+)\}/g)].map((m) => m[1]) - if (unresolved.length > 0) { - return JSON.stringify({ - success: false, - error: `Missing required path parameter(s): ${unresolved.join(', ')}`, - schema: endpointSchemaHelp(endpoint) - }) - } - const search = queryParams.toString() - if (search) url += `?${search}` - - const method = endpoint.method.toUpperCase() - const fetchOptions: RequestInit = { method } - if (body !== undefined && method !== 'GET') { - fetchOptions.headers = { 'Content-Type': 'application/json' } - fetchOptions.body = JSON.stringify(body) - } - - const response = await fetch(url, fetchOptions) - const raw = response.headers.get('content-type')?.includes('application/json') - ? await response.json() - : await response.text() - if (!response.ok) { - return JSON.stringify({ - success: false, - status: response.status, - error: typeof raw === 'string' ? raw : JSON.stringify(raw), - // 4xx usually means wrong arguments — echo the schema so the model can - // self-correct in the next call without a separate schema-fetch tool. - ...(response.status >= 400 && response.status < 500 - ? { schema: endpointSchemaHelp(endpoint) } - : {}) - }) - } - const result = JSON.stringify({ success: true, data: raw }) - if (result.length <= MAX_RESULT_CHARS) return result - return JSON.stringify({ - success: true, - truncated: true, - data: (typeof raw === 'string' ? raw : JSON.stringify(raw)).slice(0, MAX_RESULT_CHARS), - note: `Result truncated to ${MAX_RESULT_CHARS} characters. Use filter or pagination parameters to narrow it.` - }) -} - -const searchApiEndpointsSchema = z.object({ - query: z - .string() - .describe( - 'Keywords matched against endpoint names, paths, and descriptions (e.g. "queue", "run flow", "audit log"). Jobs are called "runs" in the UI.' - ) -}) - -const callApiGetSchema = z.object({ - name: z.string().describe('Endpoint name as returned by search_api_endpoints'), - params: z - .record(z.string(), z.any()) - .optional() - .describe( - 'Path and query parameter values, keyed by parameter name. The workspace parameter is filled automatically.' - ) -}) - -const callApiEndpointSchema = z.object({ - name: z.string().describe('Endpoint name as returned by search_api_endpoints'), - params: z - .record(z.string(), z.any()) - .optional() - .describe( - 'Path and query parameter values, keyed by parameter name. The workspace parameter is filled automatically.' - ), - body: z - .record(z.string(), z.any()) - .optional() - .describe('JSON request body, when the endpoint takes one') -}) - -export const apiCatalogTools: Tool<{}>[] = [ - { - def: createToolDef( - searchApiEndpointsSchema, - 'search_api_endpoints', - 'Search the Windmill REST API endpoint catalog for operations no dedicated tool covers (queue state, job details, running deployed items, deletions, ...). Returns endpoint names to pass to call_api_get or call_api_endpoint.' - ), - planModeSafe: true, - fn: async ({ args, workspace, toolId, toolCallbacks }) => { - const parsed = searchApiEndpointsSchema.parse(args) - toolCallbacks.setToolStatus(toolId, { content: 'Searching API endpoints...' }) - const catalog = await loadCatalog(workspace) - const queryTokens = tokenize(parsed.query) - const available = catalog.filter((e) => !COVERED_ENDPOINTS[e.name]) - const scored = available - .map((endpoint) => ({ endpoint, score: scoreEndpoint(endpoint, queryTokens) })) - .filter((s) => s.score > 0) - .sort((a, b) => b.score - a.score || a.endpoint.name.localeCompare(b.endpoint.name)) - const coveredHits = catalog - .filter((e) => COVERED_ENDPOINTS[e.name] && scoreEndpoint(e, queryTokens) > 0) - .map((e) => `${e.name} → use ${COVERED_ENDPOINTS[e.name]}`) - - if (scored.length === 0) { - const categories = [...new Set(available.map((e) => pathSegments(e.path)[0]))].sort() - const result = JSON.stringify( - { - matches: [], - hint: `No endpoint matched. Available endpoint categories: ${categories.join(', ')}. Retry with different keywords, or use a dedicated tool if one covers the need.`, - ...(coveredHits.length > 0 ? { covered_by_dedicated_tools: coveredHits } : {}) - }, - null, - 2 - ) - toolCallbacks.setToolStatus(toolId, { content: 'No matching API endpoint', result }) - return result - } - const top = scored.slice(0, MAX_SEARCH_RESULTS) - const result = JSON.stringify( - { - matches: top.map((s) => summarizeEndpoint(s.endpoint)), - ...(scored.length > top.length - ? { - note: `${scored.length - top.length} more match(es) — refine the query to see them.` - } - : {}), - ...(coveredHits.length > 0 ? { covered_by_dedicated_tools: coveredHits } : {}) - }, - null, - 2 - ) - toolCallbacks.setToolStatus(toolId, { - content: `Found ${top.length} API endpoint(s) for "${parsed.query}"`, - result - }) - return result - } - }, - { - def: createToolDef( - callApiGetSchema, - 'call_api_get', - 'Call a read-only GET endpoint from the API catalog by name. Use search_api_endpoints first to find the endpoint name; a failed call returns the parameter schema.' - ), - // Readonly rests on the method check below plus the catalog only exposing - // side-effect-free GETs; never mark a mutating GET as an `x-mcp-tool`. - planModeSafe: true, - showDetails: true, - fn: async ({ args, workspace, toolId, toolCallbacks }) => { - const parsed = callApiGetSchema.parse(args) - const resolved = await resolveEndpoint(workspace, parsed.name) - if ('error' in resolved) { - toolCallbacks.setToolStatus(toolId, { content: resolved.error, error: resolved.error }) - return JSON.stringify({ success: false, error: resolved.error }) - } - if (resolved.endpoint.method.toUpperCase() !== 'GET') { - const error = `"${parsed.name}" is a ${resolved.endpoint.method.toUpperCase()} endpoint — use call_api_endpoint for mutating calls.` - toolCallbacks.setToolStatus(toolId, { content: error, error }) - return JSON.stringify({ success: false, error }) - } - toolCallbacks.setToolStatus(toolId, { content: `Calling ${parsed.name}...` }) - const result = await executeEndpoint(resolved.endpoint, workspace, parsed.params ?? {}) - const ok = JSON.parse(result).success === true - toolCallbacks.setToolStatus(toolId, { - content: ok ? `Called ${parsed.name}` : `Call to ${parsed.name} failed`, - result, - ...(ok ? {} : { error: `Call to ${parsed.name} failed` }) - }) - return result - } - }, - { - def: createToolDef( - callApiEndpointSchema, - 'call_api_endpoint', - 'Call a mutating (POST/PUT/PATCH/DELETE) endpoint from the API catalog by name; the user is asked to confirm. Use search_api_endpoints first to find the endpoint name; a failed call returns the parameter schema.' - ), - requiresConfirmation: true, - confirmationMessage: (args) => `Call API endpoint ${args?.name ?? ''}`, - showDetails: true, - fn: async ({ args, workspace, toolId, toolCallbacks }) => { - const parsed = callApiEndpointSchema.parse(args) - const resolved = await resolveEndpoint(workspace, parsed.name) - if ('error' in resolved) { - toolCallbacks.setToolStatus(toolId, { content: resolved.error, error: resolved.error }) - return JSON.stringify({ success: false, error: resolved.error }) - } - if (resolved.endpoint.method.toUpperCase() === 'GET') { - const error = `"${parsed.name}" is a GET endpoint — use call_api_get (no confirmation needed).` - toolCallbacks.setToolStatus(toolId, { content: error, error }) - return JSON.stringify({ success: false, error }) - } - toolCallbacks.setToolStatus(toolId, { content: `Calling ${parsed.name}...` }) - const result = await executeEndpoint( - resolved.endpoint, - workspace, - parsed.params ?? {}, - parsed.body - ) - const ok = JSON.parse(result).success === true - toolCallbacks.setToolStatus(toolId, { - content: ok ? `Called ${parsed.name}` : `Call to ${parsed.name} failed`, - result, - ...(ok ? {} : { error: `Call to ${parsed.name} failed` }) - }) - return result - } - } -] diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 7f2a1ff3d0..7a86e0ba76 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -3764,9 +3764,8 @@ describe('global AI tools', () => { expect(getBackendDraft('raw_app', 'f/apps/legacy', { workspace: WORKSPACE })).toBeUndefined() }) - // getAppByPath and listApps are refused by the catalog, so this read is the only way - // left to ask who may open a deployed app. Both kinds answer: a drag-and-drop app can - // be anonymous too, and no other tool here can inspect one. + // This read is the only way to ask who may open a deployed app. Both kinds answer: a + // drag-and-drop app can be anonymous too, and no other tool here can inspect one. it('reports who may open an app, whichever kind it is', async () => { const readMode = async (app: any) => { vi.mocked(AppService.getAppByPath).mockResolvedValueOnce(app) @@ -7129,8 +7128,7 @@ describe('session-only preview tools gating', () => { expect(names).not.toContain('list_app_runs') expect(names).not.toContain('search_dom') expect(names).not.toContain('read_dom') - // Not withheld: without it the side panel's only route to a deployed run is the raw - // endpoint, which confirms an opaque request body instead of the arguments. + // Not withheld: without it the side panel has no route to a deployed run at all. expect(names).toContain('run_script') // other tools are still present expect(names).toContain('write_script') diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 6c85a75a4e..5b0490c724 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -254,7 +254,6 @@ const VARIABLE_MASKED_NOTE = 'Note: variable values are never shown in chat — the diff marks whether the value changed without revealing it.\n\n' const SECRET_UNCOMPARABLE_NOTE = 'Note: this is a SECRET variable — its value is never shown and cannot be compared, so it may ALSO have changed beyond what this diff shows.\n\n' -import { apiCatalogTools } from './apiCatalogTools' const ITEM_TYPES = [ 'script', @@ -1386,7 +1385,6 @@ ${pipelineBullet} ? ' By default it preselects the items this chat modified; pass items (":" entries) to control the selection' : ' Pass items (":" entries naming the items you changed) so the review is scoped to them — omitting items preselects every pending change in the workspace' }, or mode ("draft" or "fork") to force which comparison is shown. Prefer offering this review page over calling deploy_workspace_item directly when several items changed. -- For a Windmill operation no other tool covers (queue state, a run's args, ...), use search_api_endpoints to find a REST endpoint, then call_api_get for reads or call_api_endpoint for mutations (the user is asked to confirm those). Always prefer a dedicated tool when one exists; endpoints for authoring or deleting scripts, flows, apps, schedules, resources, or variables are not available through the API catalog tools — use the draft tools and delete_workspace_item instead. - Default to test_run_script, test_run_flow, or test_run_step for any run request, an existing script included; they prefer drafts and need no deployment. Use run_script or run_flow only when the user names the deployed version ("the deployed X", "in production", "for real") — a bare "run X" is not that. For those two, read the item with read_workspace_item version: "deployed" first so the arguments match the deployed schema. test_run_script, test_run_flow, test_run_step, run_script and run_flow all show the user an argument form prefilled with what you sent, so fill in every argument you can infer rather than asking for it in chat. test_run_step's form is the step's own inputs, not the flow's. - When a required decision is ambiguous, use askUserQuestion with two to ten clear proposed answer strings instead of guessing. The user can also type a custom answer when none of the proposed answers fit. Set multiSelect: true only when the answers can genuinely co-apply and the user may pick several (not mutually exclusive). - When the user asks you to remember a lasting preference, always/never do something, or change/stop a behavior going forward, call update_user_instructions to persist it. It edits only the USER INSTRUCTIONS block (not WORKSPACE INSTRUCTIONS). Keep each instruction concise; do not use it for one-off requests scoped to the current task. @@ -4427,10 +4425,7 @@ export const globalTools: Tool<{}>[] = [ // Workspace DuckLake: pipeline storage prerequisite, and declared measures ...getDucklakeTools(), // Read-only tools over files the user attached to the conversation - ...fileTools, - // Search + call access to the backend API endpoint catalog, for operations - // no dedicated tool covers - ...apiCatalogTools + ...fileTools ] // Tools that only make sense inside an AI session (they drive the session's diff --git a/frontend/src/lib/components/copilot/chat/global/flowRunTree.ts b/frontend/src/lib/components/copilot/chat/global/flowRunTree.ts index 7b81a70d7c..af203e2c8c 100644 --- a/frontend/src/lib/components/copilot/chat/global/flowRunTree.ts +++ b/frontend/src/lib/components/copilot/chat/global/flowRunTree.ts @@ -400,8 +400,7 @@ function shapeRunResult(job: Job): Record { : { result: cap(stringify(job.result)) } } -/** Why a run ended badly. get_run is the only route to these — the catalog - * refuses getJob. Kept out of summarizeRun, which list_runs pays per job. */ +/** Why a run ended badly. Kept out of summarizeRun, which list_runs pays per job. */ function diagnoseRun(job: Job): Record { return { ...(job.canceled_by ? { canceled_by: job.canceled_by } : {}),