Skip to content

Commit e7dad08

Browse files
committed
fix(mothership): clear errors and a working direct path for CLI version, function and grep commands
1 parent 1741ff9 commit e7dad08

8 files changed

Lines changed: 125 additions & 14 deletions

File tree

‎apps/sim/app/api/v2/workflows/[workflowId]/versions/[version]/route.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,22 @@ describe('GET /api/v2/workflows/[workflowId]/versions/[version]', () => {
161161
)
162162
})
163163

164+
it.each(['latest', 'v2'])(
165+
'names the version path segment when %s is not a version number',
166+
async (version) => {
167+
const request = createMockRequest({
168+
url: `http://localhost/api/v2/workflows/workflow-1/versions/${version}`,
169+
})
170+
const response = await GET(request, createRouteContext({ workflowId: 'workflow-1', version }))
171+
172+
expect(response.status).toBe(400)
173+
expect((await response.json()).error.message).toBe('version must be a positive integer')
174+
expect(
175+
workflowsPersistenceUtilsMockFns.mockGetWorkflowDeploymentVersion
176+
).not.toHaveBeenCalled()
177+
}
178+
)
179+
164180
it('never serves credential values in the pinned graph', async () => {
165181
const response = await get()
166182

‎apps/sim/lib/api/contracts/primitives.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -363,11 +363,14 @@ export const workspaceFileIdSchema = requiredFieldSchema('File ID is required')
363363
*/
364364
export const INT4_MAX = 2147483647
365365

366+
/** Also replaces zod's `expected number, received NaN` for a non-numeric path segment. */
367+
const VERSION_NUMBER_MESSAGE = 'version must be a positive integer'
368+
366369
/** A version number in a body or cursor, bounded to the range its column can hold. */
367370
export const versionNumberSchema = z
368371
.number()
369372
.int('version must be an integer')
370-
.min(1, 'version must be a positive integer')
373+
.min(1, VERSION_NUMBER_MESSAGE)
371374
.max(INT4_MAX, 'version is out of range')
372375

373376
/**
@@ -377,9 +380,9 @@ export const versionNumberSchema = z
377380
* holds.
378381
*/
379382
export const versionNumberPathSchema = z.coerce
380-
.number()
381-
.int()
382-
.positive()
383+
.number({ error: VERSION_NUMBER_MESSAGE })
384+
.int(VERSION_NUMBER_MESSAGE)
385+
.positive(VERSION_NUMBER_MESSAGE)
383386
.max(INT4_MAX, 'version is out of range')
384387

385388
/**

‎apps/sim/lib/internal/function/execute.test.ts‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import type { Principal } from '@sim/auth/principal'
2+
import { createPersonalApiKeyPrincipal } from '@sim/testing/factories/principal.factory'
13
import {
24
executorPrincipalMock,
35
executorPrincipalMockFns,
@@ -12,15 +14,18 @@ const mocks = vi.hoisted(() => ({
1214
vi.mock('@/lib/internal/principals/executor', () => executorPrincipalMock)
1315

1416
vi.mock('@/lib/function-execution/application/execute-function', () => ({
15-
executeFunction: { execute: mocks.execute },
17+
executeFunction: { execute: mocks.execute, delegationAudience: 'sim:function-executions' },
1618
}))
1719

1820
vi.mock('@/lib/function-execution/application/execute-chat-function', () => ({
1921
executeChatFunction: { execute: mocks.executeChat },
2022
}))
2123

24+
import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation'
2225
import { FUNCTION_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/function-execution/application/authorization'
2326
import { executeFunctionTool } from '@/lib/internal/function/execute'
27+
import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation'
28+
import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations'
2429
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
2530

2631
const { mockCreateExecutorPrincipalFromExecutionContext } = executorPrincipalMockFns
@@ -169,4 +174,57 @@ describe('executeFunctionTool', () => {
169174
expect(mocks.executeChat).not.toHaveBeenCalled()
170175
expect(mocks.execute).not.toHaveBeenCalled()
171176
})
177+
178+
describe('direct tool calls (POST /api/v2/tools/{id}/execute)', () => {
179+
function directContext(callerPrincipal: Principal) {
180+
return {
181+
workflowId: '',
182+
workspaceId: 'workspace-1',
183+
userId: 'user-1',
184+
callerPrincipal,
185+
}
186+
}
187+
188+
it('rebinds an admitted Mothership caller to the function-execution audience', async () => {
189+
const caller = createCopilotChatPrincipal(
190+
{ userId: 'user-1', workspaceId: 'workspace-1', chatId: 'chat-1' },
191+
TOOL_EXECUTION_DELEGATION_AUDIENCE
192+
)
193+
markCopilotWorkspaceInvocation(caller)
194+
195+
await executeFunctionTool({
196+
body: { code: 'return 1' },
197+
headers: new Headers(),
198+
requestId: 'request-1',
199+
context: directContext(caller),
200+
})
201+
202+
expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled()
203+
expect(mocks.execute).toHaveBeenCalledWith(
204+
expect.objectContaining({
205+
principal: expect.objectContaining({
206+
kind: 'delegated',
207+
serviceId: 'copilot',
208+
subjectUserId: 'user-1',
209+
workspaceId: 'workspace-1',
210+
audience: FUNCTION_EXECUTION_DELEGATION_AUDIENCE,
211+
}),
212+
})
213+
)
214+
})
215+
216+
it('hands any other caller to the function-execution policy unchanged', async () => {
217+
const caller = createPersonalApiKeyPrincipal({ keyId: 'personal-key-1' })
218+
219+
await executeFunctionTool({
220+
body: { code: 'return 1' },
221+
headers: new Headers(),
222+
requestId: 'request-1',
223+
context: directContext(caller),
224+
})
225+
226+
expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled()
227+
expect(mocks.execute).toHaveBeenCalledWith(expect.objectContaining({ principal: caller }))
228+
})
229+
})
172230
})

‎apps/sim/lib/internal/function/execute.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import type { DelegatedPrincipal } from '@sim/auth/principal'
1+
import type { Principal } from '@sim/auth/principal'
22
import type { FunctionExecuteBody } from '@/lib/api/contracts'
33
import type { InternalSandboxProfile } from '@/lib/auth/internal'
4+
import { bindCopilotWorkspaceOperation } from '@/lib/core/application/copilot-workspace-invocation'
45
import { DEFAULT_EXECUTION_TIMEOUT_MS } from '@/lib/core/execution-limits'
56
import { serializeExecutionDeadlineHeader } from '@/lib/execution/execution-deadline-header'
67
import { FUNCTION_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/function-execution/application/authorization'
@@ -9,6 +10,7 @@ import { executeFunction } from '@/lib/function-execution/application/execute-fu
910
import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor'
1011
import type { InternalToolOperationContext } from '@/lib/internal/tool-operations/types'
1112
import { createTrustedOrganizationCopilotPrincipal } from '@/lib/mothership/auth/application-delegation'
13+
import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations'
1214

1315
export type TrustedFunctionToolExecutionContext = InternalToolOperationContext
1416

@@ -78,8 +80,17 @@ export async function executeFunctionTool(input: ExecuteFunctionToolInput): Prom
7880
},
7981
})
8082
}
81-
let principal: DelegatedPrincipal
82-
if (context.copilotToolExecution === true) {
83+
let principal: Principal
84+
if (context.callerPrincipal && !context.executorDelegationOrigin) {
85+
// A direct tool call has no workflow run to bind, so the authenticated caller is the
86+
// authority; the function-execution policy admits or refuses whoever that is.
87+
principal = bindCopilotWorkspaceOperation(
88+
context.callerPrincipal,
89+
context.workspaceId,
90+
[TOOL_EXECUTION_DELEGATION_AUDIENCE],
91+
executeFunction
92+
)
93+
} else if (context.copilotToolExecution === true) {
8394
if (!context.userId) throw new Error('Copilot Function execution requires a user')
8495
principal = {
8596
kind: 'delegated',

‎apps/sim/lib/mothership/agent-cli/engines/universal-grep.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,17 @@ describe('universal grep', () => {
348348
}
349349
)
350350

351+
it.each(['knowledge', 'blocks,KB'])(
352+
'redirects --scope %s to semantic knowledge search before materializing any world',
353+
async (scope) => {
354+
/** An empty runtime throws on any request, so a clean refusal proves nothing was fetched. */
355+
const result = await runEngine('grep', ['invoice'], runtimeWith({}), { scope })
356+
expect(result.exitCode).toBe(1)
357+
expect(result.stderr).toContain('Unknown scope')
358+
expect(result.stderr).toContain('use knowledge search --kb <id>')
359+
}
360+
)
361+
351362
it('refuses an --in selector no resource in the searched worlds answers to', async () => {
352363
const bare = await runEngine('grep', ['id'], runtimeWith(CATALOG), {
353364
scope: 'blocks',

‎apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -445,14 +445,17 @@ function unknownWithin(selector: string): string {
445445
}
446446

447447
/**
448-
* Heads a model reaches for when it wants to grep a knowledge base. Knowledge is not a
449-
* grep world — chunks are retrieved semantically — so the refusal has to redirect rather
450-
* than just list the worlds, or the next attempt is the same selector spelled differently.
448+
* Names a model reaches for when it wants to grep a knowledge base, as a `--scope` or an
449+
* `--in` head. Knowledge is not a grep world — chunks are retrieved semantically — so the
450+
* refusal has to redirect rather than just list the worlds, or the next attempt is the
451+
* same selector spelled differently.
451452
*/
452453
const KNOWLEDGE_SELECTOR_HEADS = new Set(['knowledge', 'kb'])
453454

455+
const KNOWLEDGE_REDIRECT = `Knowledge bases are searched semantically — use knowledge search --kb <id> --query "…"; grep covers ${SCOPES.join(', ')}.`
456+
454457
function knowledgeWithin(selector: string): string {
455-
return `${unknownWithin(selector)} Knowledge bases are searched semantically — use knowledge search --kb <id> --query "…"; grep covers ${SCOPES.join(', ')}.`
458+
return `${unknownWithin(selector)} ${KNOWLEDGE_REDIRECT}`
456459
}
457460

458461
function parseScopes(flags: AgentCliFlags): Scope[] | string {
@@ -463,6 +466,9 @@ function parseScopes(flags: AgentCliFlags): Scope[] | string {
463466
.split(',')
464467
.map((s) => s.trim())
465468
.filter(Boolean)) {
469+
if (KNOWLEDGE_SELECTOR_HEADS.has(part.toLowerCase())) {
470+
return `Unknown scope "${part}". ${KNOWLEDGE_REDIRECT}`
471+
}
466472
if (!(SCOPES as readonly string[]).includes(part)) {
467473
return `Unknown scope "${part}".${didYouMean(part)} Scopes: ${SCOPES.join(', ')}.`
468474
}

‎apps/sim/lib/tool-execution/application/execute-tool.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ import { isHosted } from '@/lib/core/config/env-flags'
1919
import { OrchestrationError } from '@/lib/core/orchestration/types'
2020
import { getEffectiveDecryptedEnv } from '@/lib/environment/utils'
2121
import { principalUserId } from '@/lib/integrations/principal-scope.server'
22-
import { toolExecutionOperations } from '@/lib/tool-execution/application/operations'
22+
import {
23+
TOOL_EXECUTION_DELEGATION_AUDIENCE,
24+
toolExecutionOperations,
25+
} from '@/lib/tool-execution/application/operations'
2326
import { isEnvVarReference } from '@/executor/constants'
2427
import { resolveEnvVarReferences } from '@/executor/utils/reference-validation'
2528
import { executeTool as executeRegistryTool } from '@/tools'
@@ -286,7 +289,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
286289
resolveContext: ({ input }: { input: ExecuteToolInput }) =>
287290
loadCatalogWorkspaceContext(input.workspaceId),
288291
authorizationOptions: {
289-
delegation: { audience: 'sim:tool-execution', isWithinScope: () => true },
292+
delegation: { audience: TOOL_EXECUTION_DELEGATION_AUDIENCE, isWithinScope: () => true },
290293
},
291294
execute: async ({ principal, input, context }): Promise<ExecuteToolResult> => {
292295
const gate = await resolveCatalogGate(principal, context)

‎apps/sim/lib/tool-execution/application/operations.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { defineWorkspaceOperation } from '@/lib/core/application/workspace-operation'
22

3+
/** Audience of the Copilot delegation a direct tool call is admitted under. */
4+
export const TOOL_EXECUTION_DELEGATION_AUDIENCE = 'sim:tool-execution'
5+
36
/**
47
* Semantic operation for running one code-defined tool directly.
58
*

0 commit comments

Comments
 (0)