From e1ebb9f90a0b6e8e2fd956016a1eae2ab97892d1 Mon Sep 17 00:00:00 2001 From: Conner Swann <2635475+yourbuddyconner@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:32:50 -0700 Subject: [PATCH] fix(workflows): MCP tool node display + preflight (TKAI-172) Tool-node summaries and error messages built `${node.service}.${node.action}`, but action ids already carry the service prefix by convention (MCP: minted as `${serviceName}.${tool.name}`; native plugin actions like `slack.dm_owner` are declared the same way). Result on Linear: `linear.linear.list_issues`. Add a `formatToolCall(service, action)` helper that guards against double-prefixing, and apply it to the four client display sites and the worker-side disabled-action error string. The workflow preflight also called `source.listActions()` with no credential context. For MCP sources this returns `[]` when no token is threaded through, so the executor threw `action "linear.list_issues" not found in linear package` and every MCP tool node failed in every workflow. Fall back to the `mcp_tool_cache` (populated by interactive tool listing and the catalog endpoint) to recover the risk level without touching the remote server. Mirrors how the catalog route mixes live + cache. Added two targeted tests: one asserting the cache fallback resolves and executes; one asserting the "not found" error still fires when both live and cache miss. --- .../components/workflows/trace-node-card.tsx | 7 ++-- .../workflows/workflow-editor-model.ts | 16 +++++++- .../worker/src/workflows/nodes/tool.test.ts | 39 +++++++++++++++++++ packages/worker/src/workflows/nodes/tool.ts | 33 ++++++++++++++-- 4 files changed, 87 insertions(+), 8 deletions(-) diff --git a/packages/client/src/components/workflows/trace-node-card.tsx b/packages/client/src/components/workflows/trace-node-card.tsx index 3055154c7..8dec21467 100644 --- a/packages/client/src/components/workflows/trace-node-card.tsx +++ b/packages/client/src/components/workflows/trace-node-card.tsx @@ -6,6 +6,7 @@ import { MarkdownContent } from '@/components/chat/markdown/markdown-content'; import { formatRelativeTime } from '@/lib/format'; import { cn } from '@/lib/cn'; import { correctNodeStatusForFinishedExecution } from './workflow-execution-viewer-model'; +import { formatToolCall } from './workflow-editor-model'; import { ToolPayload } from '@/components/payload/tool-payload'; // ─── Public ────────────────────────────────────────────────────────────────── @@ -45,7 +46,7 @@ export function TraceNodeCard({ () => findDefNodeById(definition, node.nodeId), [definition, node.nodeId], ); - const toolCall = defNode?.type === 'tool' ? `${defNode.service}.${defNode.action}` : null; + const toolCall = defNode?.type === 'tool' ? formatToolCall(defNode.service, defNode.action) : null; const summary = describeNodeOutcome(node, output, defNode); const isError = status === 'failed' || !!node.error; @@ -359,7 +360,7 @@ function ToolBody({ }) { const o = asObject(output); const isTool = defNode?.type === 'tool'; - const callName = isTool ? `${defNode.service}.${defNode.action}` : null; + const callName = isTool ? formatToolCall(defNode.service, defNode.action) : null; const params = isTool && defNode.params && Object.keys(defNode.params).length > 0 ? defNode.params : null; const hasIterations = Array.isArray(iterations) && iterations.length > 0; @@ -1047,7 +1048,7 @@ function describeNodeOutcome(node: ExecutionNode, output: unknown, defNode: Work return 'Generated response'; } case 'tool': { - const callName = defNode && defNode.type === 'tool' ? `${defNode.service}.${defNode.action}` : null; + const callName = defNode && defNode.type === 'tool' ? formatToolCall(defNode.service, defNode.action) : null; if (o) { // Sheets append/clear: summarize updates. const u = asObject(o.updates); diff --git a/packages/client/src/components/workflows/workflow-editor-model.ts b/packages/client/src/components/workflows/workflow-editor-model.ts index a4d5e4d7a..8f6e732cf 100644 --- a/packages/client/src/components/workflows/workflow-editor-model.ts +++ b/packages/client/src/components/workflows/workflow-editor-model.ts @@ -821,7 +821,7 @@ function summarizeNode(node: WorkflowNode): string { case 'trigger': return 'Where the workflow starts and what data it receives'; case 'tool': - return trimSummary(node.service && node.action ? `${node.service}.${node.action}` : 'No action configured'); + return trimSummary(node.service && node.action ? formatToolCall(node.service, node.action) : 'No action configured'); case 'if': return `${node.conditions.length} condition${node.conditions.length === 1 ? '' : 's'}`; case 'foreach': @@ -1349,6 +1349,20 @@ function createToolCatalogActionKey(service: string, actionId: string): string { return `${service}:${actionId}`; } +/** + * Render `service.action` for a tool node without double-prefixing. + * + * Both MCP-derived action ids (minted as `${service}.${tool}` in + * `packages/sdk/src/mcp/action-source.ts`) and native plugin action ids + * (e.g. `slack.dm_owner`) already carry the service prefix. Naively + * concatenating `${service}.${action}` produced doubled labels like + * `linear.linear.list_issues` in the workflow editor and trace views. + */ +export function formatToolCall(service: string, action: string): string { + if (!service || !action) return action || service || ''; + return action.startsWith(`${service}.`) ? action : `${service}.${action}`; +} + function getTargetConfiguredInputExpression(node: WorkflowNode): string | undefined { if (node.type === 'foreach') return node.items.trim() || undefined; return undefined; diff --git a/packages/worker/src/workflows/nodes/tool.test.ts b/packages/worker/src/workflows/nodes/tool.test.ts index 21fb62c54..88fa36d39 100644 --- a/packages/worker/src/workflows/nodes/tool.test.ts +++ b/packages/worker/src/workflows/nodes/tool.test.ts @@ -48,6 +48,11 @@ vi.mock('../../lib/db/channels.js', () => ({ getUserIdentityLinks: (...args: unknown[]) => getUserIdentityLinksMock(...args), })); +const listMcpToolCacheMock = vi.fn(); +vi.mock('../../lib/db/mcp-tool-cache.js', () => ({ + listMcpToolCache: (...args: unknown[]) => listMcpToolCacheMock(...args), +})); + vi.mock('../approvals.js', () => ({ waitForApprovalEvent: (...args: unknown[]) => waitForApprovalEventMock(...args), })); @@ -107,6 +112,8 @@ beforeEach(() => { markFailedMock.mockReset(); getUserIdentityLinksMock.mockReset(); getUserIdentityLinksMock.mockResolvedValue([]); + listMcpToolCacheMock.mockReset(); + listMcpToolCacheMock.mockResolvedValue([]); isActionDisabledMock.mockResolvedValue(false); loadCustomMcpConnectorContextMock.mockResolvedValue({ connectors: new Map() }); listActionsMock.mockResolvedValue([{ id: 'slack.send_message', riskLevel: 'low' }, { id: 'slack.test', riskLevel: 'low' }, { id: 'gmail.send', riskLevel: 'medium' }, { id: 'sheets.clear_range', riskLevel: 'medium' }, { id: 'unknown.x', riskLevel: 'low' }, { id: 'unknown.y', riskLevel: 'low' }]); @@ -153,6 +160,38 @@ describe('executeTool', () => { await expect(executeTool(args(node))).rejects.toThrow(/not found in slack package/); }); + it('preflight falls back to mcp_tool_cache when listActions returns empty (TKAI-172)', async () => { + // Simulates an MCP-backed action source: `listActions()` returns `[]` + // because no credential context is threaded through the workflow + // preflight. Without the cache fallback the executor throws + // `action "linear.list_issues" not found in linear package` and + // every MCP tool node fails. With the cache-fallback in place the + // preflight resolves via mcp_tool_cache and execution proceeds. + listActionsMock.mockResolvedValue([]); + listMcpToolCacheMock.mockResolvedValue([ + { service: 'linear', actionId: 'linear.list_issues', name: 'list_issues', description: '', riskLevel: 'low' }, + ]); + executeMock.mockResolvedValue({ success: true, data: { issues: [] } }); + const node: ToolNode = { + id: 't', type: 'tool', service: 'linear', action: 'linear.list_issues', params: {}, + }; + const out = await executeTool(args(node)); + expect(out).toEqual({ issues: [] }); + expect(listMcpToolCacheMock).toHaveBeenCalledWith(expect.anything(), 'linear'); + expect(executeMock).toHaveBeenCalledWith('linear.list_issues', {}, expect.anything()); + }); + + it('preflight still throws when both listActions and mcp_tool_cache miss (TKAI-172)', async () => { + listActionsMock.mockResolvedValue([]); + listMcpToolCacheMock.mockResolvedValue([ + { service: 'linear', actionId: 'linear.other', name: 'other', description: '', riskLevel: 'low' }, + ]); + const node: ToolNode = { + id: 't', type: 'tool', service: 'linear', action: 'linear.list_issues', params: {}, + }; + await expect(executeTool(args(node))).rejects.toThrow(/not found in linear package/); + }); + it('fails when the action-policy resolves to denied', async () => { invokeWorkflowActionMock.mockResolvedValue({ outcome: 'denied', invocationId: 'inv-1', mode: 'deny', policyId: null }); const node: ToolNode = { id: 't', type: 'tool', service: 'slack', action: 'slack.send_message', params: {} }; diff --git a/packages/worker/src/workflows/nodes/tool.ts b/packages/worker/src/workflows/nodes/tool.ts index f5fb96433..fe2967c27 100644 --- a/packages/worker/src/workflows/nodes/tool.ts +++ b/packages/worker/src/workflows/nodes/tool.ts @@ -28,6 +28,7 @@ import { getDb } from '../../lib/drizzle.js'; import { invokeWorkflowAction, markExecuted, markFailed } from '../../services/actions.js'; import { buildActionCredentials } from '../../services/credentials.js'; import { updateInvocationStatus } from '../../lib/db/actions.js'; +import { listMcpToolCache } from '../../lib/db/mcp-tool-cache.js'; import { getUserIdentityLinks } from '../../lib/db/channels.js'; import { waitForApprovalEvent } from '../approvals.js'; import { setExecutionStatus } from '../execution-status.js'; @@ -61,7 +62,7 @@ export async function executeTool(args: NodeExecutorArgs): Promise { if (await isActionDisabled(db, node.service, node.action)) { - throw new Error(`tool node "${node.id}": action ${node.service}.${node.action} is disabled`); + throw new Error(`tool node "${node.id}": action ${formatToolCallLabel(node.service, node.action)} is disabled`); } const customCtx = await loadCustomMcpConnectorContext(env, db); const source = integrationRegistry.getActions(node.service, customCtx); @@ -70,10 +71,24 @@ export async function executeTool(args: NodeExecutorArgs): Promise a.id === node.action); - if (!def) { - throw new Error(`tool node "${node.id}": action "${node.action}" not found in ${node.service} package`); + if (def) { + return JSON.stringify({ riskLevel: def.riskLevel ?? 'medium' }); } - return JSON.stringify({ riskLevel: def.riskLevel ?? 'medium' }); + // MCP action sources return `[]` when no credential context is + // threaded through (see packages/sdk/src/mcp/action-source.ts:59-66). + // The workflow preflight doesn't resolve credentials yet — that + // happens further down after the invocation row is created — so + // every MCP tool node would throw "action not found" here. Fall + // back to the mcp_tool_cache (populated on interactive listing and + // by the catalog endpoint) to recover the risk level without + // touching the remote MCP server. Mirrors how the catalog route + // mixes live + cache in packages/worker/src/routes/integrations.ts. + const cached = await listMcpToolCache(db, node.service); + const cachedEntry = cached.find((entry) => entry.actionId === node.action); + if (cachedEntry) { + return JSON.stringify({ riskLevel: cachedEntry.riskLevel ?? 'medium' }); + } + throw new Error(`tool node "${node.id}": action "${node.action}" not found in ${node.service} package`); }); const preflight = JSON.parse(preflightJson) as { riskLevel: string }; @@ -378,3 +393,13 @@ function isAuthFailure(result: ActionResult): boolean { /\b(401|unauthorized|invalid.credentials|token.*expired|token.*revoked)\b/i.test(result.error); } +// Render `service.action` without double-prefixing. Mirrors the +// client-side `formatToolCall` in workflow-editor-model.ts. Both MCP +// action ids and native plugin action ids already carry the service +// prefix; naively concatenating produced "linear.linear.list_issues" +// in operator-facing error messages. +function formatToolCallLabel(service: string, action: string): string { + if (!service || !action) return action || service || ''; + return action.startsWith(`${service}.`) ? action : `${service}.${action}`; +} +