Skip to content

Commit 045b07c

Browse files
authored
fix(mothership): stop duplicating chat resource tabs in workspace chats (#8537)
* fix(mothership): stop duplicating chat resource tabs in workspace chats * fix(mothership): scope tool side-effect resources inside handleResourceSideEffects * fix(mothership): assert emitted resource events instead of mock calls
1 parent 064cd41 commit 045b07c

9 files changed

Lines changed: 45 additions & 37 deletions

File tree

‎apps/sim/lib/api/contracts/mothership-resource-tools.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ export const openResourceOutputSchema = z.object({
2929
type: z.enum(['workflow', 'table', 'knowledgebase', 'file', 'dashboard', 'log']),
3030
id: z.string(),
3131
title: z.string(),
32-
workspaceId: z.string(),
3332
viewId: z.string().optional(),
3433
executionId: z.string().optional(),
3534
})

‎apps/sim/lib/mothership/agent-cli/index.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,8 @@ async function executeBoundAgentCliRequest(
177177
} else throw new Error('Service invocation must use the service bridge')
178178
if (resources.length)
179179
result = { ...result, resources: [...resources, ...(result.resources ?? [])] }
180-
if (result.resources?.length)
180+
// Only organization chats address a workspace; a workspace chat's resources leave it implicit.
181+
if (context.chatOrganizationId && result.resources?.length)
181182
result = {
182183
...result,
183184
resources: result.resources.map((effect) => {

‎apps/sim/lib/mothership/agent-cli/resource-owner.test.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ beforeEach(() => {
8585
})
8686

8787
describe('scoped CLI resource ownership', () => {
88-
it('gives a workspace upload completion and explicit open the same resource identity', async () => {
88+
it('gives a workspace upload completion, explicit open, and an open tab one identity', async () => {
8989
const upload = await executeAgentCliRequest(
9090
{ invocation: { kind: 'cli', argv: ['files', 'upload', '@/tmp/upload-acceptance.txt'] } },
9191
context
@@ -102,9 +102,10 @@ describe('scoped CLI resource ownership', () => {
102102
id: fileId,
103103
title: file.name,
104104
path: 'files/upload-acceptance.txt',
105-
workspaceId: first,
106105
})
107-
expect(getChatResourceKey(resource)).toBe(getChatResourceKey(opened.resources[0]))
106+
const openTab = getChatResourceKey({ type: 'file', id: fileId })
107+
expect(getChatResourceKey(resource)).toBe(openTab)
108+
expect(getChatResourceKey(opened.resources[0])).toBe(openTab)
108109
expect(mocks.transport).toHaveBeenCalledTimes(1)
109110
})
110111

‎apps/sim/lib/mothership/request/tools/executor.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1040,7 +1040,10 @@ async function executeToolAndReportInner(
10401040
execContext.chatId,
10411041
options?.onEvent,
10421042
() => abortRequested(context, execContext, options),
1043-
toolCall.targetWorkspaceId ?? execContext.workspaceId,
1043+
{
1044+
organizationId: execContext.organizationId,
1045+
workspaceId: toolCall.targetWorkspaceId ?? execContext.workspaceId,
1046+
},
10441047
execContext.userId
10451048
)
10461049
}

‎apps/sim/lib/mothership/request/tools/resources.test.ts‎

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ vi.mock('@/lib/mothership/resources/persistence', () => ({
2424

2525
import { MothershipStreamV1EventType } from '@/lib/mothership/generated/mothership-stream-v1'
2626
import { handleResourceSideEffects } from '@/lib/mothership/request/tools/resources'
27+
import type { StreamEvent } from '@/lib/mothership/request/types'
2728
import type { MothershipResource } from '@/lib/mothership/resources/types'
2829

2930
describe('handleResourceSideEffects', () => {
@@ -97,31 +98,35 @@ describe('handleResourceSideEffects', () => {
9798
})
9899
}
99100
)
100-
it.each(['workspace-a'])('addresses extracted exports to admitted %s', async (workspaceId) => {
101-
const resource = { type: 'file' as const, id: 'export', title: 'decisions.csv' }
102-
mocks.extractResourcesFromToolResult.mockReturnValue([resource])
103-
const onEvent = vi.fn()
104-
await handleResourceSideEffects(
105-
'run_function',
106-
undefined,
107-
{ success: true, output: {} },
108-
{ success: true, output: {} },
109-
'org-chat',
110-
onEvent,
111-
() => false,
112-
workspaceId
113-
)
114-
expect(mocks.persistChatResources).toHaveBeenCalledWith('org-chat', [
115-
{ ...resource, workspaceId },
116-
])
117-
expect(onEvent).toHaveBeenCalledWith({
118-
type: 'resource',
119-
payload: {
120-
op: 'upsert',
121-
resource: { ...resource, workspaceId },
122-
},
123-
})
124-
})
101+
it.each([
102+
{ chat: 'organization', organizationId: 'org', expected: { workspaceId: 'workspace-a' } },
103+
{ chat: 'workspace', organizationId: undefined, expected: {} },
104+
])(
105+
'addresses extracted exports in a $chat chat like its open tabs',
106+
async ({ organizationId, expected }) => {
107+
const resource = { type: 'file' as const, id: 'export', title: 'decisions.csv' }
108+
mocks.extractResourcesFromToolResult.mockReturnValue([resource])
109+
const events: StreamEvent[] = []
110+
await handleResourceSideEffects(
111+
'run_function',
112+
undefined,
113+
{ success: true, output: {} },
114+
{ success: true, output: {} },
115+
'chat',
116+
(event) => {
117+
events.push(event)
118+
},
119+
() => false,
120+
{ organizationId, workspaceId: 'workspace-a' }
121+
)
122+
expect(events).toEqual([
123+
{
124+
type: 'resource',
125+
payload: { op: 'upsert', resource: { ...resource, ...expected } },
126+
},
127+
])
128+
}
129+
)
125130
})
126131

127132
it('emits authorized Search results beside the persisted address, never inside it', async () => {

‎apps/sim/lib/mothership/request/tools/resources.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@ export async function handleResourceSideEffects(
3636
chatId: string,
3737
onEvent: ((event: StreamEvent) => void | Promise<void>) | undefined,
3838
isAborted: () => boolean,
39-
workspaceId?: string,
39+
owner?: { organizationId?: string; workspaceId?: string },
4040
actorUserId?: string
4141
): Promise<void> {
42+
// Only organization chats address a workspace; a workspace chat's resources leave it implicit.
43+
const workspaceId = owner?.organizationId ? owner.workspaceId : undefined
4244
// Cheap early exit so we don't emit a span for tools that can never
4345
// produce resources (most of them). The span only shows up for tools
4446
// that might actually do resource work.

‎apps/sim/lib/mothership/tools/registry/server-tool-adapter.test.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,7 @@ describe('server tool adapter authority boundary', () => {
120120
})
121121

122122
it('forwards canonical open-resource effects without replacing target assertions or injecting a workflow', async () => {
123-
const resources = [
124-
{ type: 'workflow', id: 'flow', title: 'Canonical', workspaceId: 'workspace-1' },
125-
]
123+
const resources = [{ type: 'workflow', id: 'flow', title: 'Canonical' }]
126124
mocks.routeExecution.mockResolvedValue({ resources })
127125
const params = {
128126
workspaceId: 'asserted-workspace',

‎apps/sim/lib/mothership/tools/server/open-resource.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ describe('resource opening authorization boundary', () => {
8787
'Canonical knowledge',
8888
'Workflow run',
8989
])
90-
expect(result.resources.every((resource) => resource.workspaceId === 'ws-a')).toBe(true)
9190
for (const read of Object.values(mocks))
9291
expect(read).toHaveBeenCalledWith(
9392
expect.objectContaining({

‎apps/sim/lib/mothership/tools/server/open-resource.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export const openResourceServerTool: BaseServerTool<OpenResourceInput, OpenResou
5656
assertServerToolNotAborted(context)
5757
if (resource.viewId && resource.type !== 'table')
5858
throw new OrchestrationError('validation', 'Saved views apply only to tables')
59-
const base = { type: resource.type, id: resource.id, workspaceId }
59+
const base = { type: resource.type, id: resource.id }
6060
switch (resource.type) {
6161
case 'workflow': {
6262
const { workflow } = await executeCopilotWorkflowUseCase(context, readWorkflowMetadata, {

0 commit comments

Comments
 (0)