Skip to content

Commit 7fd7228

Browse files
committed
fix(mothership): strip only the display label, and keep a tool's own activity argument
1 parent 486d077 commit 7fd7228

4 files changed

Lines changed: 101 additions & 10 deletions

File tree

‎apps/sim/lib/mothership/request/handlers/handlers.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2252,6 +2252,58 @@ describe('sse-handlers tool lifecycle', () => {
22522252
)
22532253
})
22542254

2255+
it("keeps an integration operation's own activity argument under the gateway's grouping", async () => {
2256+
isSimExecuted.mockReturnValue(false)
2257+
executeTool.mockResolvedValueOnce({ success: true, output: {} })
2258+
2259+
await sseHandlers.tool(
2260+
{
2261+
type: MothershipStreamV1EventType.tool,
2262+
payload: {
2263+
toolCallId: 'gateway-work-item',
2264+
toolName: 'call_integration_tool',
2265+
arguments: {
2266+
toolId: 'azure_devops_create_work_item',
2267+
description: 'Filing the task',
2268+
activity: { id: 'filing', title: 'Filing tasks', completedTitle: 'Filed tasks' },
2269+
arguments: { title: 'Ship it', activity: 'Development' },
2270+
},
2271+
executor: MothershipStreamV1ToolExecutor.go,
2272+
mode: MothershipStreamV1ToolMode.sync,
2273+
phase: MothershipStreamV1ToolPhase.call,
2274+
},
2275+
} satisfies StreamEvent,
2276+
context,
2277+
execContext,
2278+
{ interactive: false, timeout: 1000 }
2279+
)
2280+
2281+
await sseHandlers.tool(
2282+
{
2283+
type: MothershipStreamV1EventType.tool,
2284+
payload: {
2285+
toolCallId: 'gateway-work-item',
2286+
toolName: 'azure_devops_create_work_item',
2287+
arguments: { title: 'Ship it', activity: 'Development' },
2288+
executor: MothershipStreamV1ToolExecutor.sim,
2289+
mode: MothershipStreamV1ToolMode.async,
2290+
phase: MothershipStreamV1ToolPhase.call,
2291+
},
2292+
} satisfies StreamEvent,
2293+
context,
2294+
execContext,
2295+
{ interactive: false, timeout: 1000 }
2296+
)
2297+
2298+
await sleep(0)
2299+
2300+
expect(executeTool).toHaveBeenCalledWith(
2301+
'azure_devops_create_work_item',
2302+
{ title: 'Ship it', activity: 'Development' },
2303+
expect.any(Object)
2304+
)
2305+
})
2306+
22552307
it('clears pending continuation state when a run resumes', async () => {
22562308
context.awaitingAsyncContinuation = {
22572309
checkpointId: 'cp-1',

‎apps/sim/lib/mothership/request/handlers/tool.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,9 @@ async function handleCallPhase(
587587
.filter((tool) => tool.agentId === agentId && tool.parentToolCallId === parentToolCallId)
588588
.map((tool) => readToolActivity(tool.params))
589589
.find((candidate) => candidate?.id === suppliedActivity.id && candidate.completedTitle)
590-
const displayArgs = activity ? { ...args, activity } : args
590+
// A tool's own `activity` argument (any value that is not a display label) is never replaced.
591+
const ownsActivity = args?.activity !== undefined && !readToolActivity(args)
592+
const displayArgs = activity && !ownsActivity ? { ...args, activity } : args
591593
const isSubagent = scope === 'subagent'
592594
const ui = getToolCallUI(data)
593595

‎apps/sim/lib/mothership/tool-executor/executor.test.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import { toolsMock, toolsMockFns } from '@sim/testing/mocks/tools.mock'
1111
import { beforeEach, describe, expect, it, vi } from 'vitest'
1212
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
13-
import { getToolMetadata } from '@/tools/metadata'
13+
import { getToolMetadata, getToolParams } from '@/tools/metadata'
1414
import { slackGetUserTool } from '@/tools/slack/get_user'
1515

1616
const { getToolEntry, isKnownTool, isSimExecuted, isClientExecuted } = vi.hoisted(() => ({
@@ -313,13 +313,33 @@ describe('copilot tool executor fallback', () => {
313313
await executeTool(
314314
'mcp-server-get_overview',
315315
{ activity: { id: 'gauge-read' }, category_id: null },
316-
{ userId: 'person', workspaceId: 'workspace', requestMode: 'agent' }
316+
{ userId: 'person', workflowId: '', workspaceId: 'workspace', requestMode: 'agent' }
317317
)
318-
const [, appParams] = executeAppTool.mock.calls[0]
318+
const appParams = executeAppTool.mock.calls[0]?.[1]
319319
expect(appParams).not.toHaveProperty('activity')
320320
expect(appParams).toMatchObject({ category_id: null })
321321
})
322322

323+
it.each([
324+
['a tool that declares it', 'azure_devops_create_work_item', { id: 'Development' }],
325+
['a value that is not a display label', 'mcp-server-log_time', 'Development'],
326+
])("keeps a tool's own activity for %s", async (_case, toolId, activity) => {
327+
vi.mocked(getToolParams).mockImplementation((id) =>
328+
id === 'azure_devops_create_work_item'
329+
? { activity: { type: 'string', required: false, visibility: 'user-or-llm' } }
330+
: undefined
331+
)
332+
isKnownTool.mockReturnValue(false)
333+
isClientExecuted.mockReturnValue(false)
334+
executeAppTool.mockResolvedValue({ success: true })
335+
await executeTool(
336+
toolId,
337+
{ activity },
338+
{ userId: 'person', workflowId: '', workspaceId: 'workspace', requestMode: 'agent' }
339+
)
340+
expect(executeAppTool.mock.calls[0]?.[1]).toMatchObject({ activity })
341+
})
342+
323343
it('projects resolved secrets before logging registered handler failures', async () => {
324344
const secret = 'mounted-secret-value'
325345
const registry = new ResolvedSecretTraceRegistry([

‎apps/sim/lib/mothership/tool-executor/executor.ts‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ import { createRegistryContentGate } from '@/lib/mothership/content-delivery'
1111
import { prepareCopilotEnvironmentContext } from '@/lib/mothership/environment-context'
1212
import { isSearchIntegrationToolsEnabled } from '@/lib/mothership/feature-flags'
1313
import { isOwnerScopedFile } from '@/lib/mothership/file-owners'
14+
import { ToolActivity } from '@/lib/mothership/generated/protocol'
1415
import { projectToolErrorMessageForCopilot } from '@/lib/mothership/request/tools/resolved-secret-result'
1516
import { recordSecretUsage } from '@/lib/secrets/usage/record'
1617
import { executeTool as executeAppTool } from '@/tools'
17-
import { getToolMetadata } from '@/tools/metadata'
18+
import { getToolMetadata, getToolParams } from '@/tools/metadata'
1819
import { getToolEntry, isClientExecuted, isKnownTool, isSimExecuted } from './router'
1920
import type { ToolExecutionContext, ToolExecutionResult, ToolHandler } from './types'
2021

@@ -42,11 +43,9 @@ export function clearHandlers(): void {
4243

4344
export async function executeTool(
4445
toolId: string,
45-
args: Record<string, unknown>,
46+
params: Record<string, unknown>,
4647
context: ToolExecutionContext
4748
): Promise<ToolExecutionResult> {
48-
/** `activity` is the chat's display label for the call; no tool takes it as an argument. */
49-
const { activity: _activity, ...params } = args
5049
if (
5150
context.benchmark &&
5251
!(context.benchmark === 'distill'
@@ -241,7 +240,7 @@ async function executeGatedTool(
241240
hasHandler(toolId) &&
242241
(!isKnownTool(toolId) || isSimExecuted(toolId) || usesHeadlessClientFallback)
243242
if (!canUseRegisteredHandler) {
244-
const appParams = buildAppToolParams(params, context)
243+
const appParams = buildAppToolParams(withoutDisplayActivity(toolId, params), context)
245244
const options = {
246245
...(context.resolvedSecretTraceRegistry
247246
? { resolvedSecretTraceRegistry: context.resolvedSecretTraceRegistry }
@@ -295,7 +294,8 @@ async function executeGatedTool(
295294
}
296295

297296
try {
298-
return await handler(params, context)
297+
const { activity: _activity, ...executionParams } = params
298+
return await handler(executionParams, context)
299299
} catch (error) {
300300
const message = toError(error).message
301301
logger.error('Tool execution failed', {
@@ -331,6 +331,23 @@ function recordAppToolSecretUsage(context: ToolExecutionContext): void {
331331
})
332332
}
333333

334+
/**
335+
* The chat's grouping label rides on a call's arguments for presentation and persistence, so an
336+
* integration or MCP tool would receive an `activity` it never declared and a strict schema
337+
* refuses. It is dropped only when it reads as that label and the tool declares no `activity` of
338+
* its own (Azure DevOps work items take an Activity field).
339+
*/
340+
function withoutDisplayActivity(
341+
toolId: string,
342+
params: Record<string, unknown>
343+
): Record<string, unknown> {
344+
if (!ToolActivity.safeParse(params.activity).success || getToolParams(toolId)?.activity) {
345+
return params
346+
}
347+
const { activity: _activity, ...rest } = params
348+
return rest
349+
}
350+
334351
function buildAppToolParams(
335352
params: Record<string, unknown>,
336353
context: ToolExecutionContext

0 commit comments

Comments
 (0)