diff --git a/packages/forestadmin-client/src/permissions/forest-http-api.ts b/packages/forestadmin-client/src/permissions/forest-http-api.ts index c97cc9f131..379df4aac4 100644 --- a/packages/forestadmin-client/src/permissions/forest-http-api.ts +++ b/packages/forestadmin-client/src/permissions/forest-http-api.ts @@ -177,12 +177,12 @@ export default class ForestHttpApi implements ForestAdminServerInterface { workflowId: string, recordId: string, ): Promise { - // The orchestrator returns a numeric runId; normalize it to the string form - // expected by getMcpWorkflowRun. - const result = await ServerUtils.queryWithBearerToken<{ - runId: number | string; - runState: WorkflowRunTriggerResult['runState']; - }>({ + // The orchestrator returns a numeric runId; normalize it to the string form expected by + // getMcpWorkflowRun. workflowName/collectionName are passed through when present so the + // caller can label the audit log without listing every workflow first. + const result = await ServerUtils.queryWithBearerToken< + Omit & { runId: number | string } + >({ forestServerUrl: options.forestServerUrl, method: 'post', path: `/api/workflow-orchestrator/mcp-workflows/${encodeURIComponent(workflowId)}/start`, @@ -191,7 +191,7 @@ export default class ForestHttpApi implements ForestAdminServerInterface { headers: { 'forest-rendering-id': renderingId, ...options.headers }, }); - return { runId: String(result.runId), runState: result.runState }; + return { ...result, runId: String(result.runId) }; } async getMcpWorkflowRun( diff --git a/packages/forestadmin-client/src/types.ts b/packages/forestadmin-client/src/types.ts index 560c27b892..53965363f2 100644 --- a/packages/forestadmin-client/src/types.ts +++ b/packages/forestadmin-client/src/types.ts @@ -307,10 +307,14 @@ export type WorkflowRunState = 'started' | 'pending' | 'loading' | 'aborted' | ' /** * The outcome of starting a workflow run: the run continues asynchronously server-side. * `runId` is normalized to a string so it can be fed back to `getMcpWorkflowRun` as-is. + * `workflowName`/`collectionName` are echoed by the start endpoint to build the audit label + * without a second round-trip; they are optional so older servers degrade gracefully. */ export interface WorkflowRunTriggerResult { runId: string; runState: WorkflowRunState; + workflowName?: string; + collectionName?: string | null; } export interface TriggerMcpWorkflowParams { diff --git a/packages/mcp-server/src/tools/trigger-workflow.ts b/packages/mcp-server/src/tools/trigger-workflow.ts index ded9736ff6..b3ba206637 100644 --- a/packages/mcp-server/src/tools/trigger-workflow.ts +++ b/packages/mcp-server/src/tools/trigger-workflow.ts @@ -1,11 +1,15 @@ +import type { WorkflowRunTriggerResult } from '../http-client'; import type { ToolContext } from '../tool-context'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import { NotFoundError } from '@forestadmin/forestadmin-client'; import { z } from 'zod'; +import createPendingActivityLog, { + markActivityLogAsSucceeded, +} from '../utils/activity-logs-creator'; import getAuthContext from '../utils/auth-context'; import registerToolWithLogging from '../utils/tool-with-logging'; -import withActivityLog from '../utils/with-activity-log'; const WORKFLOW_ID_DESCRIPTION = 'The id of the workflow to start, as returned by listWorkflows. The workflow must have the MCP ' + @@ -41,48 +45,62 @@ export default function declareTriggerWorkflowTool(mcpServer: McpServer, ctx: To async (args: TriggerWorkflowArgument, extra) => { const { forestServerToken, renderingId } = getAuthContext(extra); - // We list workflows first to resolve the name/collection needed for the activity-log label - // (the trigger endpoint returns neither). The server also validates access at trigger time, - // so this lookup is primarily for enrichment; targeting the workflow by id directly would - // save a round-trip — tracked in PRD-831. - const workflows = await forestServerClient.listMcpWorkflows({ - forestServerToken, - renderingId, - }); - const workflow = workflows.find(candidate => candidate.workflowId === args.workflowId); + let result: WorkflowRunTriggerResult; - // Rejected before withActivityLog: with no resolved workflow there is no collection to - // attach, and the server drops MCP activity logs that carry no resource (see PRD-49), so a - // pre-trigger rejection cannot be audited. Only real triggers are logged (incl. server-side - // 403/409, which fail inside withActivityLog below). - if (!workflow) { - throw new Error( - `Workflow "${args.workflowId}" is not an MCP-enabled workflow you can access. ` + - 'Use listWorkflows to discover triggerable workflows.', - ); + try { + result = await forestServerClient.triggerWorkflow({ + forestServerToken, + renderingId, + workflowId: args.workflowId, + recordId: args.recordId, + }); + } catch (error) { + // The server answers 404 both for an unknown workflow and for one whose MCP trigger is + // disabled (indistinguishable on purpose). Surface the same guidance the tool gave when + // it validated the id client-side, so the LLM-facing contract stays identical. + if (error instanceof NotFoundError) { + throw new Error( + `Workflow "${args.workflowId}" is not an MCP-enabled workflow you can access. ` + + 'Use listWorkflows to discover triggerable workflows.', + ); + } + + throw error; } - return withActivityLog({ - forestServerClient, - request: extra, - action: 'triggerWorkflow', - context: { - collectionName: workflow.collectionName ?? undefined, - recordId: args.recordId, - label: `triggered the workflow "${workflow.name}"`, - }, - logger, - operation: async () => { - const result = await forestServerClient.triggerWorkflow({ - forestServerToken, - renderingId, - workflowId: args.workflowId, + // Audit the successful trigger. The start endpoint echoes the workflow name/collection so we + // can label the log without a prior listing; fall back to the id when an older server omits + // them. The run is already started, so a logging hiccup must not fail the tool. + try { + const activityLog = await createPendingActivityLog( + forestServerClient, + extra, + 'triggerWorkflow', + { + collectionName: result.collectionName ?? undefined, recordId: args.recordId, - }); + label: `triggered the workflow "${result.workflowName ?? args.workflowId}"`, + }, + ); + + markActivityLogAsSucceeded({ forestServerClient, request: extra, activityLog, logger }); + } catch (error) { + logger( + 'Warn', + `Failed to record triggerWorkflow activity log: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } - return { content: [{ type: 'text', text: JSON.stringify(result) }] }; - }, - }); + return { + content: [ + { + type: 'text', + text: JSON.stringify({ runId: result.runId, runState: result.runState }), + }, + ], + }; }, logger, ); diff --git a/packages/mcp-server/test/tools/trigger-workflow.test.ts b/packages/mcp-server/test/tools/trigger-workflow.test.ts index e254eadb75..2accd00dec 100644 --- a/packages/mcp-server/test/tools/trigger-workflow.test.ts +++ b/packages/mcp-server/test/tools/trigger-workflow.test.ts @@ -8,13 +8,9 @@ import type { ServerNotification, ServerRequest } from '@modelcontextprotocol/sd import { NotFoundError } from '@forestadmin/forestadmin-client'; import declareTriggerWorkflowTool from '../../src/tools/trigger-workflow'; -import withActivityLog from '../../src/utils/with-activity-log'; import createMockForestServerClient from '../helpers/forest-server-client'; -jest.mock('../../src/utils/with-activity-log'); - const mockLogger: Logger = jest.fn(); -const mockWithActivityLog = withActivityLog as jest.MockedFunction; describe('declareTriggerWorkflowTool', () => { let mcpServer: McpServer; @@ -33,9 +29,6 @@ describe('declareTriggerWorkflowTool', () => { registeredToolHandler = handler; }), } as unknown as McpServer; - - // By default, withActivityLog executes the operation and returns its result - mockWithActivityLog.mockImplementation(async options => options.operation()); }); describe('tool registration', () => { @@ -111,13 +104,15 @@ describe('declareTriggerWorkflowTool', () => { logger: mockLogger, collectionNames: [], }); - mockForestServerClient.listMcpWorkflows.mockResolvedValue([ - { workflowId: 'wf-1', name: 'Refund order', collectionName: 'orders' }, - ]); - mockForestServerClient.triggerWorkflow.mockResolvedValue({ runId: '7', runState: 'loading' }); + mockForestServerClient.triggerWorkflow.mockResolvedValue({ + runId: '7', + runState: 'loading', + workflowName: 'Refund order', + collectionName: 'orders', + }); }); - it('should call triggerWorkflow with the identity from the auth context and the args', async () => { + it('should start the workflow directly with the identity from the auth context and the args', async () => { await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); expect(mockForestServerClient.triggerWorkflow).toHaveBeenCalledWith({ @@ -128,7 +123,13 @@ describe('declareTriggerWorkflowTool', () => { }); }); - it('should return the runId and runState as JSON text content', async () => { + it('should not list workflows before triggering', async () => { + await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); + + expect(mockForestServerClient.listMcpWorkflows).not.toHaveBeenCalled(); + }); + + it('should return only the runId and runState as JSON text content', async () => { const result = await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); expect(result).toEqual({ @@ -136,38 +137,46 @@ describe('declareTriggerWorkflowTool', () => { }); }); - it('should wrap the trigger in an activity log carrying the resolved collection and record', async () => { + it('should record an activity log labelled from the response name and collection', async () => { await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); - expect(mockWithActivityLog).toHaveBeenCalledWith({ - forestServerClient: mockForestServerClient, - request: mockExtra, - action: 'triggerWorkflow', - context: { + expect(mockForestServerClient.createMcpActivityLog).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'triggerWorkflow', + type: 'write', collectionName: 'orders', recordId: '42', label: 'triggered the workflow "Refund order"', - }, - logger: mockLogger, - operation: expect.any(Function), + }), + ); + }); + + it('should fall back to the workflowId in the label when the response omits name/collection', async () => { + mockForestServerClient.triggerWorkflow.mockResolvedValue({ + runId: '7', + runState: 'loading', }); + + await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); + + expect(mockForestServerClient.createMcpActivityLog).toHaveBeenCalledWith( + expect.objectContaining({ + collectionName: undefined, + recordId: '42', + label: 'triggered the workflow "wf-1"', + }), + ); }); - it('should error without triggering when the workflow is not among accessible workflows', async () => { - mockForestServerClient.listMcpWorkflows.mockResolvedValue([ - { workflowId: 'other-wf', name: 'Other', collectionName: 'orders' }, - ]); + it('should still return the run when recording the activity log fails', async () => { + mockForestServerClient.createMcpActivityLog.mockRejectedValue(new Error('audit down')); const result = await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); expect(result).toEqual({ - content: [ - { type: 'text', text: expect.stringContaining('is not an MCP-enabled workflow') }, - ], - isError: true, + content: [{ type: 'text', text: JSON.stringify({ runId: '7', runState: 'loading' }) }], }); - expect(mockForestServerClient.triggerWorkflow).not.toHaveBeenCalled(); - expect(mockWithActivityLog).not.toHaveBeenCalled(); + expect(mockLogger).toHaveBeenCalledWith('Warn', expect.stringContaining('audit down')); }); it('should return an error result when the auth context is missing the token', async () => { @@ -187,32 +196,36 @@ describe('declareTriggerWorkflowTool', () => { expect(mockForestServerClient.triggerWorkflow).not.toHaveBeenCalled(); }); - it('should map a 409 already-ongoing run to an error tool result', async () => { + it('should map a server 404 to the "is not an MCP-enabled workflow" tool error', async () => { mockForestServerClient.triggerWorkflow.mockRejectedValue( - new Error('A run is already ongoing on this record'), + new NotFoundError('Workflow MCP trigger not found or disabled'), ); const result = await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); expect(result).toEqual({ content: [ - { type: 'text', text: expect.stringContaining('already ongoing on this record') }, + { type: 'text', text: expect.stringContaining('is not an MCP-enabled workflow') }, ], isError: true, }); + expect(mockForestServerClient.createMcpActivityLog).not.toHaveBeenCalled(); }); - it('should map a 404 non-mcp-enabled workflow to an error tool result', async () => { + it('should pass a 409 already-ongoing run through as an error tool result', async () => { mockForestServerClient.triggerWorkflow.mockRejectedValue( - new NotFoundError('Workflow MCP trigger not found or disabled'), + new Error('A run is already ongoing on this record'), ); const result = await registeredToolHandler({ workflowId: 'wf-1', recordId: '42' }, mockExtra); expect(result).toEqual({ - content: [{ type: 'text', text: expect.stringContaining('not found or disabled') }], + content: [ + { type: 'text', text: expect.stringContaining('already ongoing on this record') }, + ], isError: true, }); + expect(mockForestServerClient.createMcpActivityLog).not.toHaveBeenCalled(); }); }); });