From e7dad08696f1b720ce9414ca5976ea12e4300525 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 7 Oct 2026 00:47:01 -0700 Subject: [PATCH 1/2] fix(mothership): clear errors and a working direct path for CLI version, function and grep commands --- .../versions/[version]/route.test.ts | 16 +++++ apps/sim/lib/api/contracts/primitives.ts | 11 ++-- .../sim/lib/internal/function/execute.test.ts | 60 ++++++++++++++++++- apps/sim/lib/internal/function/execute.ts | 17 +++++- .../agent-cli/engines/universal-grep.test.ts | 11 ++++ .../agent-cli/engines/universal-grep.ts | 14 +++-- .../application/execute-tool.ts | 7 ++- .../tool-execution/application/operations.ts | 3 + 8 files changed, 125 insertions(+), 14 deletions(-) diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/versions/[version]/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/versions/[version]/route.test.ts index 9c86e7c6b61..863e2f4f495 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/versions/[version]/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/versions/[version]/route.test.ts @@ -161,6 +161,22 @@ describe('GET /api/v2/workflows/[workflowId]/versions/[version]', () => { ) }) + it.each(['latest', 'v2'])( + 'names the version path segment when %s is not a version number', + async (version) => { + const request = createMockRequest({ + url: `http://localhost/api/v2/workflows/workflow-1/versions/${version}`, + }) + const response = await GET(request, createRouteContext({ workflowId: 'workflow-1', version })) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe('version must be a positive integer') + expect( + workflowsPersistenceUtilsMockFns.mockGetWorkflowDeploymentVersion + ).not.toHaveBeenCalled() + } + ) + it('never serves credential values in the pinned graph', async () => { const response = await get() diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 039699e799b..1931a0c6c38 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -363,11 +363,14 @@ export const workspaceFileIdSchema = requiredFieldSchema('File ID is required') */ export const INT4_MAX = 2147483647 +/** Also replaces zod's `expected number, received NaN` for a non-numeric path segment. */ +const VERSION_NUMBER_MESSAGE = 'version must be a positive integer' + /** A version number in a body or cursor, bounded to the range its column can hold. */ export const versionNumberSchema = z .number() .int('version must be an integer') - .min(1, 'version must be a positive integer') + .min(1, VERSION_NUMBER_MESSAGE) .max(INT4_MAX, 'version is out of range') /** @@ -377,9 +380,9 @@ export const versionNumberSchema = z * holds. */ export const versionNumberPathSchema = z.coerce - .number() - .int() - .positive() + .number({ error: VERSION_NUMBER_MESSAGE }) + .int(VERSION_NUMBER_MESSAGE) + .positive(VERSION_NUMBER_MESSAGE) .max(INT4_MAX, 'version is out of range') /** diff --git a/apps/sim/lib/internal/function/execute.test.ts b/apps/sim/lib/internal/function/execute.test.ts index fc39083acbb..6acb713e418 100644 --- a/apps/sim/lib/internal/function/execute.test.ts +++ b/apps/sim/lib/internal/function/execute.test.ts @@ -1,3 +1,5 @@ +import type { Principal } from '@sim/auth/principal' +import { createPersonalApiKeyPrincipal } from '@sim/testing/factories/principal.factory' import { executorPrincipalMock, executorPrincipalMockFns, @@ -12,15 +14,18 @@ const mocks = vi.hoisted(() => ({ vi.mock('@/lib/internal/principals/executor', () => executorPrincipalMock) vi.mock('@/lib/function-execution/application/execute-function', () => ({ - executeFunction: { execute: mocks.execute }, + executeFunction: { execute: mocks.execute, delegationAudience: 'sim:function-executions' }, })) vi.mock('@/lib/function-execution/application/execute-chat-function', () => ({ executeChatFunction: { execute: mocks.executeChat }, })) +import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' import { FUNCTION_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/function-execution/application/authorization' import { executeFunctionTool } from '@/lib/internal/function/execute' +import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' +import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' const { mockCreateExecutorPrincipalFromExecutionContext } = executorPrincipalMockFns @@ -169,4 +174,57 @@ describe('executeFunctionTool', () => { expect(mocks.executeChat).not.toHaveBeenCalled() expect(mocks.execute).not.toHaveBeenCalled() }) + + describe('direct tool calls (POST /api/v2/tools/{id}/execute)', () => { + function directContext(callerPrincipal: Principal) { + return { + workflowId: '', + workspaceId: 'workspace-1', + userId: 'user-1', + callerPrincipal, + } + } + + it('rebinds an admitted Mothership caller to the function-execution audience', async () => { + const caller = createCopilotChatPrincipal( + { userId: 'user-1', workspaceId: 'workspace-1', chatId: 'chat-1' }, + TOOL_EXECUTION_DELEGATION_AUDIENCE + ) + markCopilotWorkspaceInvocation(caller) + + await executeFunctionTool({ + body: { code: 'return 1' }, + headers: new Headers(), + requestId: 'request-1', + context: directContext(caller), + }) + + expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled() + expect(mocks.execute).toHaveBeenCalledWith( + expect.objectContaining({ + principal: expect.objectContaining({ + kind: 'delegated', + serviceId: 'copilot', + subjectUserId: 'user-1', + workspaceId: 'workspace-1', + audience: FUNCTION_EXECUTION_DELEGATION_AUDIENCE, + }), + }) + ) + }) + + it('hands any other caller to the function-execution policy unchanged', async () => { + const caller = createPersonalApiKeyPrincipal({ keyId: 'personal-key-1' }) + + await executeFunctionTool({ + body: { code: 'return 1' }, + headers: new Headers(), + requestId: 'request-1', + context: directContext(caller), + }) + + expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled() + expect(mocks.execute).toHaveBeenCalledWith(expect.objectContaining({ principal: caller })) + }) + }) }) diff --git a/apps/sim/lib/internal/function/execute.ts b/apps/sim/lib/internal/function/execute.ts index 5383a810faa..6a92e885d6e 100644 --- a/apps/sim/lib/internal/function/execute.ts +++ b/apps/sim/lib/internal/function/execute.ts @@ -1,6 +1,7 @@ -import type { DelegatedPrincipal } from '@sim/auth/principal' +import type { Principal } from '@sim/auth/principal' import type { FunctionExecuteBody } from '@/lib/api/contracts' import type { InternalSandboxProfile } from '@/lib/auth/internal' +import { bindCopilotWorkspaceOperation } from '@/lib/core/application/copilot-workspace-invocation' import { DEFAULT_EXECUTION_TIMEOUT_MS } from '@/lib/core/execution-limits' import { serializeExecutionDeadlineHeader } from '@/lib/execution/execution-deadline-header' import { FUNCTION_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/function-execution/application/authorization' @@ -9,6 +10,7 @@ import { executeFunction } from '@/lib/function-execution/application/execute-fu import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' import type { InternalToolOperationContext } from '@/lib/internal/tool-operations/types' import { createTrustedOrganizationCopilotPrincipal } from '@/lib/mothership/auth/application-delegation' +import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations' export type TrustedFunctionToolExecutionContext = InternalToolOperationContext @@ -78,8 +80,17 @@ export async function executeFunctionTool(input: ExecuteFunctionToolInput): Prom }, }) } - let principal: DelegatedPrincipal - if (context.copilotToolExecution === true) { + let principal: Principal + if (context.callerPrincipal && !context.executorDelegationOrigin) { + // A direct tool call has no workflow run to bind, so the authenticated caller is the + // authority; the function-execution policy admits or refuses whoever that is. + principal = bindCopilotWorkspaceOperation( + context.callerPrincipal, + context.workspaceId, + [TOOL_EXECUTION_DELEGATION_AUDIENCE], + executeFunction + ) + } else if (context.copilotToolExecution === true) { if (!context.userId) throw new Error('Copilot Function execution requires a user') principal = { kind: 'delegated', diff --git a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.test.ts b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.test.ts index d29e49c6878..f8c4208cb1f 100644 --- a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.test.ts +++ b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.test.ts @@ -348,6 +348,17 @@ describe('universal grep', () => { } ) + it.each(['knowledge', 'blocks,KB'])( + 'redirects --scope %s to semantic knowledge search before materializing any world', + async (scope) => { + /** An empty runtime throws on any request, so a clean refusal proves nothing was fetched. */ + const result = await runEngine('grep', ['invoice'], runtimeWith({}), { scope }) + expect(result.exitCode).toBe(1) + expect(result.stderr).toContain('Unknown scope') + expect(result.stderr).toContain('use knowledge search --kb ') + } + ) + it('refuses an --in selector no resource in the searched worlds answers to', async () => { const bare = await runEngine('grep', ['id'], runtimeWith(CATALOG), { scope: 'blocks', diff --git a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts index d4496a4f677..b77400c6cee 100644 --- a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts +++ b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts @@ -445,14 +445,17 @@ function unknownWithin(selector: string): string { } /** - * Heads a model reaches for when it wants to grep a knowledge base. Knowledge is not a - * grep world — chunks are retrieved semantically — so the refusal has to redirect rather - * than just list the worlds, or the next attempt is the same selector spelled differently. + * Names a model reaches for when it wants to grep a knowledge base, as a `--scope` or an + * `--in` head. Knowledge is not a grep world — chunks are retrieved semantically — so the + * refusal has to redirect rather than just list the worlds, or the next attempt is the + * same selector spelled differently. */ const KNOWLEDGE_SELECTOR_HEADS = new Set(['knowledge', 'kb']) +const KNOWLEDGE_REDIRECT = `Knowledge bases are searched semantically — use knowledge search --kb --query "…"; grep covers ${SCOPES.join(', ')}.` + function knowledgeWithin(selector: string): string { - return `${unknownWithin(selector)} Knowledge bases are searched semantically — use knowledge search --kb --query "…"; grep covers ${SCOPES.join(', ')}.` + return `${unknownWithin(selector)} ${KNOWLEDGE_REDIRECT}` } function parseScopes(flags: AgentCliFlags): Scope[] | string { @@ -463,6 +466,9 @@ function parseScopes(flags: AgentCliFlags): Scope[] | string { .split(',') .map((s) => s.trim()) .filter(Boolean)) { + if (KNOWLEDGE_SELECTOR_HEADS.has(part.toLowerCase())) { + return `Unknown scope "${part}". ${KNOWLEDGE_REDIRECT}` + } if (!(SCOPES as readonly string[]).includes(part)) { return `Unknown scope "${part}".${didYouMean(part)} Scopes: ${SCOPES.join(', ')}.` } diff --git a/apps/sim/lib/tool-execution/application/execute-tool.ts b/apps/sim/lib/tool-execution/application/execute-tool.ts index 9612a06e772..51c04479ea6 100644 --- a/apps/sim/lib/tool-execution/application/execute-tool.ts +++ b/apps/sim/lib/tool-execution/application/execute-tool.ts @@ -19,7 +19,10 @@ import { isHosted } from '@/lib/core/config/env-flags' import { OrchestrationError } from '@/lib/core/orchestration/types' import { getEffectiveDecryptedEnv } from '@/lib/environment/utils' import { principalUserId } from '@/lib/integrations/principal-scope.server' -import { toolExecutionOperations } from '@/lib/tool-execution/application/operations' +import { + TOOL_EXECUTION_DELEGATION_AUDIENCE, + toolExecutionOperations, +} from '@/lib/tool-execution/application/operations' import { isEnvVarReference } from '@/executor/constants' import { resolveEnvVarReferences } from '@/executor/utils/reference-validation' import { executeTool as executeRegistryTool } from '@/tools' @@ -286,7 +289,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({ resolveContext: ({ input }: { input: ExecuteToolInput }) => loadCatalogWorkspaceContext(input.workspaceId), authorizationOptions: { - delegation: { audience: 'sim:tool-execution', isWithinScope: () => true }, + delegation: { audience: TOOL_EXECUTION_DELEGATION_AUDIENCE, isWithinScope: () => true }, }, execute: async ({ principal, input, context }): Promise => { const gate = await resolveCatalogGate(principal, context) diff --git a/apps/sim/lib/tool-execution/application/operations.ts b/apps/sim/lib/tool-execution/application/operations.ts index 3dbe325c164..57deb1e284d 100644 --- a/apps/sim/lib/tool-execution/application/operations.ts +++ b/apps/sim/lib/tool-execution/application/operations.ts @@ -1,5 +1,8 @@ import { defineWorkspaceOperation } from '@/lib/core/application/workspace-operation' +/** Audience of the Copilot delegation a direct tool call is admitted under. */ +export const TOOL_EXECUTION_DELEGATION_AUDIENCE = 'sim:tool-execution' + /** * Semantic operation for running one code-defined tool directly. * From 738e955ac3a3d145343505f657fe86f1e13f8c24 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 7 Oct 2026 01:05:21 -0700 Subject: [PATCH 2/2] test(function): prove direct tool calls against the real function-execution policy --- .../function/execute-direct-call.test.ts | 69 +++++++++++++++++++ .../sim/lib/internal/function/execute.test.ts | 60 +--------------- 2 files changed, 70 insertions(+), 59 deletions(-) create mode 100644 apps/sim/lib/internal/function/execute-direct-call.test.ts diff --git a/apps/sim/lib/internal/function/execute-direct-call.test.ts b/apps/sim/lib/internal/function/execute-direct-call.test.ts new file mode 100644 index 00000000000..a584555f182 --- /dev/null +++ b/apps/sim/lib/internal/function/execute-direct-call.test.ts @@ -0,0 +1,69 @@ +import { createPersonalApiKeyPrincipal } from '@sim/testing/factories/principal.factory' +import { workspaceAuthzMock, workspaceAuthzMockFns } from '@sim/testing/mocks/workspace-authz.mock' +import { + workspaceContextMock, + workspaceContextMockFns, +} from '@sim/testing/mocks/workspace-context.mock' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ runSandbox: vi.fn() })) + +vi.mock('@/lib/function-execution/execute-request', () => ({ + executeFunctionRequest: mocks.runSandbox, +})) +vi.mock('@/lib/workspaces/application/workspace-context', () => workspaceContextMock) +vi.mock('@sim/platform-authz/workspace', () => workspaceAuthzMock) + +import type { Principal } from '@sim/auth/principal' +import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' +import { PrincipalKindAuthorizationError } from '@/lib/core/application/workspace-authorization' +import { executeFunctionTool } from '@/lib/internal/function/execute' +import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' +import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations' + +/** + * `POST /api/v2/tools/{id}/execute` (the embedded CLI's `tools execute`) runs `function_execute` + * with no workflow behind it, so no executor origin exists. The real function-execution policy + * decides from the authenticated caller; only the sandbox is stubbed. + */ +function runDirect(callerPrincipal: Principal) { + return executeFunctionTool({ + body: { code: 'return 1' }, + headers: new Headers(), + requestId: 'request-1', + context: { workflowId: '', workspaceId: 'workspace-1', userId: 'user-1', callerPrincipal }, + }) +} + +describe('executeFunctionTool direct tool calls', () => { + beforeEach(() => { + workspaceContextMockFns.mockResolveActiveWorkspaceApplicationContext.mockResolvedValue({ + workspaceId: 'workspace-1', + workspaceOrganizationId: null, + billedAccountUserId: 'user-1', + allowPersonalApiKeys: true, + }) + workspaceAuthzMockFns.mockResolveEffectiveWorkspacePermission.mockResolvedValue('write') + mocks.runSandbox.mockResolvedValue(Response.json({ success: true, output: { result: 1 } })) + }) + + it('runs the code for an admitted Mothership caller', async () => { + const caller = createCopilotChatPrincipal( + { userId: 'user-1', workspaceId: 'workspace-1', chatId: 'chat-1' }, + TOOL_EXECUTION_DELEGATION_AUDIENCE + ) + markCopilotWorkspaceInvocation(caller) + + const response = await runDirect(caller) + + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ success: true, output: { result: 1 } }) + }) + + it('still refuses a personal API key before any code runs', async () => { + await expect(runDirect(createPersonalApiKeyPrincipal({ keyId: 'key-1' }))).rejects.toThrow( + PrincipalKindAuthorizationError + ) + expect(mocks.runSandbox).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/internal/function/execute.test.ts b/apps/sim/lib/internal/function/execute.test.ts index 6acb713e418..fc39083acbb 100644 --- a/apps/sim/lib/internal/function/execute.test.ts +++ b/apps/sim/lib/internal/function/execute.test.ts @@ -1,5 +1,3 @@ -import type { Principal } from '@sim/auth/principal' -import { createPersonalApiKeyPrincipal } from '@sim/testing/factories/principal.factory' import { executorPrincipalMock, executorPrincipalMockFns, @@ -14,18 +12,15 @@ const mocks = vi.hoisted(() => ({ vi.mock('@/lib/internal/principals/executor', () => executorPrincipalMock) vi.mock('@/lib/function-execution/application/execute-function', () => ({ - executeFunction: { execute: mocks.execute, delegationAudience: 'sim:function-executions' }, + executeFunction: { execute: mocks.execute }, })) vi.mock('@/lib/function-execution/application/execute-chat-function', () => ({ executeChatFunction: { execute: mocks.executeChat }, })) -import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' import { FUNCTION_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/function-execution/application/authorization' import { executeFunctionTool } from '@/lib/internal/function/execute' -import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' -import { TOOL_EXECUTION_DELEGATION_AUDIENCE } from '@/lib/tool-execution/application/operations' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' const { mockCreateExecutorPrincipalFromExecutionContext } = executorPrincipalMockFns @@ -174,57 +169,4 @@ describe('executeFunctionTool', () => { expect(mocks.executeChat).not.toHaveBeenCalled() expect(mocks.execute).not.toHaveBeenCalled() }) - - describe('direct tool calls (POST /api/v2/tools/{id}/execute)', () => { - function directContext(callerPrincipal: Principal) { - return { - workflowId: '', - workspaceId: 'workspace-1', - userId: 'user-1', - callerPrincipal, - } - } - - it('rebinds an admitted Mothership caller to the function-execution audience', async () => { - const caller = createCopilotChatPrincipal( - { userId: 'user-1', workspaceId: 'workspace-1', chatId: 'chat-1' }, - TOOL_EXECUTION_DELEGATION_AUDIENCE - ) - markCopilotWorkspaceInvocation(caller) - - await executeFunctionTool({ - body: { code: 'return 1' }, - headers: new Headers(), - requestId: 'request-1', - context: directContext(caller), - }) - - expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled() - expect(mocks.execute).toHaveBeenCalledWith( - expect.objectContaining({ - principal: expect.objectContaining({ - kind: 'delegated', - serviceId: 'copilot', - subjectUserId: 'user-1', - workspaceId: 'workspace-1', - audience: FUNCTION_EXECUTION_DELEGATION_AUDIENCE, - }), - }) - ) - }) - - it('hands any other caller to the function-execution policy unchanged', async () => { - const caller = createPersonalApiKeyPrincipal({ keyId: 'personal-key-1' }) - - await executeFunctionTool({ - body: { code: 'return 1' }, - headers: new Headers(), - requestId: 'request-1', - context: directContext(caller), - }) - - expect(mockCreateExecutorPrincipalFromExecutionContext).not.toHaveBeenCalled() - expect(mocks.execute).toHaveBeenCalledWith(expect.objectContaining({ principal: caller })) - }) - }) })