Skip to content

Commit 30640ea

Browse files
committed
fix(mothership): hold an inherited activity beside a call's arguments instead of stripping it at execution
1 parent 064b52c commit 30640ea

7 files changed

Lines changed: 115 additions & 63 deletions

File tree

‎apps/sim/lib/mothership/chat/persisted-message.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,31 @@ describe('persisted-message', () => {
101101
}
102102
)
103103

104+
it("persists a resolved operation's inherited grouping with its arguments", () => {
105+
const persisted = buildPersistedAssistantMessage({
106+
success: true,
107+
content: '',
108+
toolCalls: [],
109+
contentBlocks: [
110+
{
111+
type: 'tool_call',
112+
timestamp: 1,
113+
toolCall: {
114+
id: 'gateway-call',
115+
name: 'mcp-server-get_overview',
116+
status: 'success',
117+
params: { category_id: null },
118+
activity: { id: 'gauge-read', title: 'Reading Gauge', completedTitle: 'Read Gauge' },
119+
},
120+
},
121+
],
122+
})
123+
expect(persisted.contentBlocks?.[0].toolCall?.params).toEqual({
124+
category_id: null,
125+
activity: { id: 'gauge-read', title: 'Reading Gauge', completedTitle: 'Read Gauge' },
126+
})
127+
})
128+
104129
it('preserves activity metadata on legacy tool blocks without inferring it from old titles', () => {
105130
const message = normalizeMessage({
106131
id: 'message-1',

‎apps/sim/lib/mothership/chat/persisted-message.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
MothershipStreamV1ToolOutcome,
2525
MothershipStreamV1ToolPhase,
2626
} from '@/lib/mothership/generated/mothership-stream-v1'
27+
import type { ToolActivity } from '@/lib/mothership/generated/protocol'
2728
import type {
2829
ContentBlock,
2930
LocalToolCallStatus,
@@ -273,6 +274,14 @@ export function displayContentOf(content: PersistedMessage): PersistedMessage |
273274

274275
// Write: OrchestratorResult → PersistedMessage
275276

277+
/** A call's arguments as history reads them, with an inherited display grouping restored. */
278+
function persistedParams(
279+
params: Record<string, unknown>,
280+
activity: ToolActivity | undefined
281+
): Record<string, unknown> {
282+
return activity && params.activity === undefined ? { ...params, activity } : params
283+
}
284+
276285
function resolveToolState(block: ContentBlock): PersistedToolState {
277286
const tc = block.toolCall
278287
if (!tc) return 'pending'
@@ -391,7 +400,7 @@ function mapContentBlockBody(block: ContentBlock): PersistedContentBlock {
391400
...(isSubagentTool && isNonTerminal
392401
? {}
393402
: block.toolCall.params
394-
? { params: block.toolCall.params }
403+
? { params: persistedParams(block.toolCall.params, block.toolCall.activity) }
395404
: {}),
396405
...(block.calledBy ? { calledBy: block.calledBy } : {}),
397406
...(block.toolCall.displayTitle

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

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { flushMacrotask } from '@sim/testing/helpers/async'
12
import { createDeferred } from '@sim/testing/helpers/deferred'
23
import {
34
mothershipAsyncRunsMock,
@@ -2252,6 +2253,60 @@ describe('sse-handlers tool lifecycle', () => {
22522253
)
22532254
})
22542255

2256+
it("executes a resolved MCP operation with its own arguments under the gateway's grouping", async () => {
2257+
isSimExecuted.mockReturnValue(false)
2258+
executeTool.mockResolvedValueOnce({ success: true, output: {} })
2259+
const grouping = { id: 'gauge-read', title: 'Reading Gauge', completedTitle: 'Read Gauge' }
2260+
2261+
await sseHandlers.tool(
2262+
{
2263+
type: MothershipStreamV1EventType.tool,
2264+
payload: {
2265+
toolCallId: 'gateway-mcp',
2266+
toolName: 'call_integration_tool',
2267+
arguments: {
2268+
toolId: 'mcp-server-get_overview',
2269+
description: 'Pulling the overview',
2270+
activity: grouping,
2271+
arguments: { category_id: null },
2272+
},
2273+
executor: MothershipStreamV1ToolExecutor.go,
2274+
mode: MothershipStreamV1ToolMode.sync,
2275+
phase: MothershipStreamV1ToolPhase.call,
2276+
},
2277+
} satisfies StreamEvent,
2278+
context,
2279+
execContext,
2280+
{ interactive: false, timeout: 1000 }
2281+
)
2282+
2283+
await sseHandlers.tool(
2284+
{
2285+
type: MothershipStreamV1EventType.tool,
2286+
payload: {
2287+
toolCallId: 'gateway-mcp',
2288+
toolName: 'mcp-server-get_overview',
2289+
arguments: { category_id: null },
2290+
executor: MothershipStreamV1ToolExecutor.sim,
2291+
mode: MothershipStreamV1ToolMode.async,
2292+
phase: MothershipStreamV1ToolPhase.call,
2293+
},
2294+
} satisfies StreamEvent,
2295+
context,
2296+
execContext,
2297+
{ interactive: false, timeout: 1000 }
2298+
)
2299+
2300+
await flushMacrotask()
2301+
2302+
expect(executeTool).toHaveBeenCalledWith(
2303+
'mcp-server-get_overview',
2304+
{ category_id: null },
2305+
expect.any(Object)
2306+
)
2307+
expect(context.toolCalls.get('gateway-mcp')?.activity).toEqual(grouping)
2308+
})
2309+
22552310
it("keeps an integration operation's own activity argument under the gateway's grouping", async () => {
22562311
isSimExecuted.mockReturnValue(false)
22572312
executeTool.mockResolvedValueOnce({ success: true, output: {} })
@@ -2295,7 +2350,7 @@ describe('sse-handlers tool lifecycle', () => {
22952350
{ interactive: false, timeout: 1000 }
22962351
)
22972352

2298-
await sleep(0)
2353+
await flushMacrotask()
22992354

23002355
expect(executeTool).toHaveBeenCalledWith(
23012356
'azure_devops_create_work_item',

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
MothershipStreamV1ToolOutcome,
2121
type MothershipStreamV1ToolResultPayload,
2222
} from '@/lib/mothership/generated/mothership-stream-v1'
23+
import type { ToolActivity } from '@/lib/mothership/generated/protocol'
2324
import { TraceAttr } from '@/lib/mothership/generated/trace-attributes-v1'
2425
import { TraceSpan } from '@/lib/mothership/generated/trace-spans-v1'
2526
import { withCopilotSpan } from '@/lib/mothership/request/otel'
@@ -145,6 +146,11 @@ function applyToolDisplay(toolCall: ToolCallState | undefined): void {
145146

146147
const INTEGRATION_GATEWAY_TOOL = 'call_integration_tool'
147148

149+
/** A call's display grouping: carried in its own arguments, or inherited and held beside them. */
150+
function toolCallActivity(toolCall: ToolCallState | undefined): ToolActivity | undefined {
151+
return toolCall?.activity ?? readToolActivity(toolCall?.params)
152+
}
153+
148154
function handleToolArgsDelta(
149155
data: { argumentsDelta: string; toolCallId: string; toolName: string },
150156
context: StreamingContext
@@ -580,17 +586,18 @@ async function handleCallPhase(
580586
}
581587
/** Persist visible labels even when the introducing discovery call is omitted from the transcript. */
582588
const argsActivity = readToolActivity(args)
583-
const suppliedActivity = argsActivity ?? readToolActivity(existing?.params)
589+
const suppliedActivity = argsActivity ?? toolCallActivity(existing)
584590
const activity = suppliedActivity?.completedTitle
585591
? suppliedActivity
586592
: suppliedActivity &&
587593
Array.from(context.toolCalls.values())
588594
.filter((tool) => tool.agentId === agentId && tool.parentToolCallId === parentToolCallId)
589-
.map((tool) => readToolActivity(tool.params))
595+
.map(toolCallActivity)
590596
.find((candidate) => candidate?.id === suppliedActivity.id && candidate.completedTitle)
591-
// A tool's own `activity` argument (any value that is not a display label) is never replaced.
592-
const ownsActivity = args?.activity !== undefined && !argsActivity
593-
const displayArgs = activity && !ownsActivity ? { ...args, activity } : args
597+
// Only a label the frame's own arguments carry is completed in place. An inherited one stays
598+
// off the arguments, which execute as given (an MCP schema refuses keys it never declared).
599+
const displayArgs = activity && argsActivity ? { ...args, activity } : args
600+
if (existing && activity && !argsActivity) existing.activity = activity
594601
const isSubagent = scope === 'subagent'
595602
const ui = getToolCallUI(data)
596603

‎apps/sim/lib/mothership/request/types.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
type MothershipStreamV1CompletionStatus,
44
MothershipStreamV1ToolOutcome,
55
} from '@/lib/mothership/generated/mothership-stream-v1'
6-
import type { StreamActivityReceipt } from '@/lib/mothership/generated/protocol'
6+
import type { StreamActivityReceipt, ToolActivity } from '@/lib/mothership/generated/protocol'
77
import type { RequestTraceV1Span } from '@/lib/mothership/generated/request-trace-v1'
88
import type { ProviderToolCallIdentity } from '@/lib/mothership/request/go/tool-call-identity'
99
import type { StreamEvent } from '@/lib/mothership/request/session'
@@ -42,6 +42,12 @@ export interface ToolCallState {
4242
activityDescription?: string
4343
/** Model-authored activity text for a gateway-resolved integration call. */
4444
integrationDescription?: string
45+
/**
46+
* Display grouping the call inherited rather than carried in its own arguments (a gateway's
47+
* grouping on its resolved operation). Kept out of `params`, which execute as given; it is
48+
* merged back only into the persisted arguments, where history reads it.
49+
*/
50+
activity?: ToolActivity
4551
/** Accumulated partial JSON of the arguments while the model streams them. */
4652
streamingArgs?: string
4753
params?: Record<string, unknown>

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

Lines changed: 1 addition & 35 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, getToolParams } from '@/tools/metadata'
13+
import { getToolMetadata } from '@/tools/metadata'
1414
import { slackGetUserTool } from '@/tools/slack/get_user'
1515

1616
const { getToolEntry, isKnownTool, isSimExecuted, isClientExecuted } = vi.hoisted(() => ({
@@ -306,40 +306,6 @@ describe('copilot tool executor fallback', () => {
306306
)
307307
})
308308

309-
it('keeps display activity out of integration and MCP execution parameters', async () => {
310-
isKnownTool.mockReturnValue(false)
311-
isClientExecuted.mockReturnValue(false)
312-
executeAppTool.mockResolvedValue({ success: true })
313-
await executeTool(
314-
'mcp-server-get_overview',
315-
{ activity: { id: 'gauge-read' }, category_id: null },
316-
{ userId: 'person', workflowId: '', workspaceId: 'workspace', requestMode: 'agent' }
317-
)
318-
const appParams = executeAppTool.mock.calls[0]?.[1]
319-
expect(appParams).not.toHaveProperty('activity')
320-
expect(appParams).toMatchObject({ category_id: null })
321-
})
322-
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-
343309
it('projects resolved secrets before logging registered handler failures', async () => {
344310
const secret = 'mounted-secret-value'
345311
const registry = new ResolvedSecretTraceRegistry([

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

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { createLogger } from '@sim/logger'
22
import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace'
33
import { toError } from '@sim/utils/errors'
4-
import { omit } from '@sim/utils/object'
54
import { openResourceInputSchema } from '@/lib/api/contracts/mothership-resource-tools'
65
import { observeContentDelivery } from '@/lib/core/application'
76
import { withWorkspaceInvocationScope } from '@/lib/core/application/workspace-invocation-scope'
@@ -13,10 +12,9 @@ import { prepareCopilotEnvironmentContext } from '@/lib/mothership/environment-c
1312
import { isSearchIntegrationToolsEnabled } from '@/lib/mothership/feature-flags'
1413
import { isOwnerScopedFile } from '@/lib/mothership/file-owners'
1514
import { projectToolErrorMessageForCopilot } from '@/lib/mothership/request/tools/resolved-secret-result'
16-
import { readToolActivity } from '@/lib/mothership/tools/activity'
1715
import { recordSecretUsage } from '@/lib/secrets/usage/record'
1816
import { executeTool as executeAppTool } from '@/tools'
19-
import { getToolMetadata, getToolParams } from '@/tools/metadata'
17+
import { getToolMetadata } from '@/tools/metadata'
2018
import { getToolEntry, isClientExecuted, isKnownTool, isSimExecuted } from './router'
2119
import type { ToolExecutionContext, ToolExecutionResult, ToolHandler } from './types'
2220

@@ -241,7 +239,7 @@ async function executeGatedTool(
241239
hasHandler(toolId) &&
242240
(!isKnownTool(toolId) || isSimExecuted(toolId) || usesHeadlessClientFallback)
243241
if (!canUseRegisteredHandler) {
244-
const appParams = buildAppToolParams(withoutDisplayActivity(toolId, params), context)
242+
const appParams = buildAppToolParams(params, context)
245243
const options = {
246244
...(context.resolvedSecretTraceRegistry
247245
? { resolvedSecretTraceRegistry: context.resolvedSecretTraceRegistry }
@@ -295,7 +293,8 @@ async function executeGatedTool(
295293
}
296294

297295
try {
298-
return await handler(omit(params, ['activity']), context)
296+
const { activity: _activity, ...executionParams } = params
297+
return await handler(executionParams, context)
299298
} catch (error) {
300299
const message = toError(error).message
301300
logger.error('Tool execution failed', {
@@ -331,21 +330,6 @@ function recordAppToolSecretUsage(context: ToolExecutionContext): void {
331330
})
332331
}
333332

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-
return readToolActivity(params) && !getToolParams(toolId)?.activity
345-
? omit(params, ['activity'])
346-
: params
347-
}
348-
349333
function buildAppToolParams(
350334
params: Record<string, unknown>,
351335
context: ToolExecutionContext

0 commit comments

Comments
 (0)