From 23fed10a6e60c110174b2122e975b2a5a1dd2d87 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 2 Nov 2026 12:37:47 -0800 Subject: [PATCH 1/2] fix(copilot): hold sim_cli commands that perform approval-gated capabilities The permission gate decided by catalog lookup alone, and sim_cli has no catalog entry, so no CLI command was ever held. The worker exposes deploy, promote, run and cancel only through the CLI, so those catalog approval flags guarded nothing, and the CLI's own confirmation is answered by --yes in model argv. Resolve the argv with the CLI's own parser to the command it would run and the v2 operations it declares, and hold the call when one is the capability of an approval-gated catalog tool. Both lanes ask the same predicate. Always-allow covers a CLI call only when its row names the command the parser resolves. --- .../app/api/copilot/tools/execute/route.ts | 2 +- apps/sim/lib/mothership/agent-cli/approval.ts | 71 ++++++++++++++++++ .../request/handlers/handlers.test.ts | 75 ++++++++++++++++++- .../lib/mothership/request/handlers/tool.ts | 12 ++- .../mothership/request/tools/permission.ts | 14 +++- .../mothership/tool-executor/router.test.ts | 11 +++ .../lib/mothership/tool-executor/router.ts | 13 ++-- packages/sim-cli/src/embed.test.ts | 43 ++++++++++- packages/sim-cli/src/embed.ts | 69 ++++++++++++++--- 9 files changed, 287 insertions(+), 23 deletions(-) create mode 100644 apps/sim/lib/mothership/agent-cli/approval.ts diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index 5575a25a746..703708a1a09 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -134,7 +134,7 @@ export const POST = withRouteHandler((request: NextRequest) => * where the gate lives. Inert while copilot tool permissions are disabled, which keeps * enabling the flag from silently leaving background lanes ungated. */ - if (toolRequiresApprovalLane(toolName)) { + if (toolRequiresApprovalLane(toolName, params)) { logger.warn('Refusing an approval-gated tool on the in-band lane', { toolName, toolCallId, diff --git a/apps/sim/lib/mothership/agent-cli/approval.ts b/apps/sim/lib/mothership/agent-cli/approval.ts new file mode 100644 index 00000000000..53885df7bed --- /dev/null +++ b/apps/sim/lib/mothership/agent-cli/approval.ts @@ -0,0 +1,71 @@ +import { resolveEmbeddedCliCommand, type V2OperationName } from 'sim/embed' +import { AgentCliRequest } from '@/lib/mothership/generated/agent-cli' +import { + CallIntegrationTool, + CancelWorkflowRun, + DeleteWorkspaceMcpServer, + DeployAsApi, + DeployAsChat, + DeployAsMcp, + PromoteToLive, + RunWorkflow, + TOOL_CATALOG, + type ToolCatalogEntry, +} from '@/lib/mothership/generated/tool-catalog-v1' + +/** + * The catalog tool whose capability each v2 operation is. The worker retired these + * tools in favour of `sim_cli`, so a command that calls one of these operations answers + * to that tool's approval flag: one policy, whichever name the capability arrives under. + * The deploy tools gate undeploy as well, so both directions map to them; rollback runs + * the same use case as activating a version. + */ +const CATALOG_TOOL_BY_OPERATION: Partial> = { + activateWorkflowVersion: PromoteToLive, + rollbackWorkflow: PromoteToLive, + deployWorkflow: DeployAsApi, + undeployWorkflow: DeployAsApi, + replaceWorkflowChatDeployment: DeployAsChat, + deleteWorkflowChatDeployment: DeployAsChat, + deployWorkflowMcpTool: DeployAsMcp, + undeployWorkflowMcpTool: DeployAsMcp, + deleteWorkflowMcpServer: DeleteWorkspaceMcpServer, + executeWorkflow: RunWorkflow, + resumeWorkflow: RunWorkflow, + cancelWorkflowRun: CancelWorkflowRun, + executeTool: CallIntegrationTool, +} + +/** + * Whether a `sim_cli` call must be held for the user's decision. Its argv is resolved by + * the CLI's own parser, so the answer is about the command that would run, and a `--yes` + * in that argv answers only the CLI's prompt, never this one. Argv the parser cannot + * follow is held; a malformed request is not, because `executeSimCli` refuses it. + */ +export function cliRequestRequiresApproval(request: unknown): boolean { + const parsed = AgentCliRequest.safeParse(request) + if (!parsed.success) return false + const { invocation } = parsed.data + if (invocation.kind === 'service') return TOOL_CATALOG[invocation.name]?.requiresApproval === true + if (invocation.kind !== 'cli') return false + const command = resolveEmbeddedCliCommand(invocation.argv) + if (command === undefined) return true + return ( + command?.operations.some( + (operation) => CATALOG_TOOL_BY_OPERATION[operation]?.requiresApproval === true + ) === true + ) +} + +/** + * The tool name (`cli_workflows_deploy`) of the command a `sim_cli` call would run, as the + * CLI's parser resolves it. The row's own name is the worker's reading of the same argv; + * an always-allow saved under that name covers the call only when the two agree, so a + * row named for one command, or for no command at all, never carries another's consent. + */ +export function cliCommandToolName(request: unknown): string | undefined { + const parsed = AgentCliRequest.safeParse(request) + if (!parsed.success || parsed.data.invocation.kind !== 'cli') return undefined + const command = resolveEmbeddedCliCommand(parsed.data.invocation.argv) + return command ? `cli_${command.path.join('_').replace(/-/g, '_')}` : undefined +} diff --git a/apps/sim/lib/mothership/request/handlers/handlers.test.ts b/apps/sim/lib/mothership/request/handlers/handlers.test.ts index 3d1ab77e7d2..e3dde17b745 100644 --- a/apps/sim/lib/mothership/request/handlers/handlers.test.ts +++ b/apps/sim/lib/mothership/request/handlers/handlers.test.ts @@ -9,7 +9,7 @@ import { mothershipClientToolWaiterMockFns, } from '@sim/testing/mocks/mothership-client-tool-waiter.mock' import { sleep } from '@sim/utils/helpers' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { AsyncToolCallOwnershipError } from '@/lib/mothership/async-runs/errors' import { restoreStreamingContext } from '@/lib/mothership/request/context/restore' import { TraceCollector } from '@/lib/mothership/request/trace' @@ -431,6 +431,79 @@ describe('sse-handlers tool lifecycle', () => { expect(event.payload.status).toBe('awaiting_approval') }) + describe('sim_cli approval', () => { + const activate = ['workflows', 'activate', 'create', 'wf-1', '1', '--yes'] + + function cliFrame(toolCallId: string, toolName: string, argv: string[]): StreamEvent { + return { + type: MothershipStreamV1EventType.tool, + payload: { + toolCallId, + toolName, + execName: 'sim_cli', + arguments: { args: argv, request: { invocation: { kind: 'cli', argv } } }, + executor: MothershipStreamV1ToolExecutor.sim, + mode: MothershipStreamV1ToolMode.async, + phase: MothershipStreamV1ToolPhase.call, + }, + } + } + + beforeEach(async () => { + const router = await vi.importActual( + '@/lib/mothership/tool-executor/router' + ) + toolRequiresApproval.mockImplementation(router.toolRequiresApproval) + context.runId = 'run-1' + context.toolPermissions = { + enabled: true, + autoAllowed: new Set(), + autoAllowPermitted: true, + } + }) + + afterEach(() => { + toolRequiresApproval.mockReset() + toolRequiresApproval.mockReturnValue(false) + }) + + it('holds a command that performs a gated capability, though its argv says --yes', async () => { + const event = cliFrame('cli-activate', 'cli_workflows_activate_create', activate) + + await prePersistClientExecutableToolCall(event, context, {}) + + expect(event.payload.status).toBe('awaiting_approval') + expect(upsertAsyncToolCall).toHaveBeenCalledWith( + expect.objectContaining({ + toolName: 'cli_workflows_activate_create', + executableName: 'sim_cli', + permissionRequested: true, + }) + ) + }) + + it('lets always-allow cover only the command it was granted for', async () => { + context.toolPermissions.autoAllowed.add('cli_workflows_list') + context.toolPermissions.autoAllowed.add('sim_cli') + const held = cliFrame('cli-held', 'cli_workflows_activate_create', activate) + await prePersistClientExecutableToolCall(held, context, {}) + expect(held.payload.status).toBe('awaiting_approval') + + // A row named for an allowed command, or for none, does not carry its consent + // to the command the CLI would actually run. + for (const name of ['cli_workflows_list', 'sim_cli']) { + const mislabeled = cliFrame(`cli-${name}`, name, activate) + await prePersistClientExecutableToolCall(mislabeled, context, {}) + expect(mislabeled.payload.status).toBe('awaiting_approval') + } + + context.toolPermissions.autoAllowed.add('cli_workflows_activate_create') + const allowed = cliFrame('cli-allowed', 'cli_workflows_activate_create', activate) + await prePersistClientExecutableToolCall(allowed, context, {}) + expect(allowed.payload.status).toBeUndefined() + }) + }) + it('clears a Go-stamped approval frame when the gate is off', async () => { // Go stamps integration calls regardless of Sim's feature flag. Forwarding // that stamp with nothing gating behind it would draw a card whose buttons diff --git a/apps/sim/lib/mothership/request/handlers/tool.ts b/apps/sim/lib/mothership/request/handlers/tool.ts index 3ec10080dae..33183671aef 100644 --- a/apps/sim/lib/mothership/request/handlers/tool.ts +++ b/apps/sim/lib/mothership/request/handlers/tool.ts @@ -330,7 +330,8 @@ export async function prePersistClientExecutableToolCall( context, options ?? {}, frameRequestsApproval, - data.arguments + data.arguments, + data.execName ) if (gated) { data.status = TOOL_AWAITING_APPROVAL_STATUS @@ -1029,7 +1030,14 @@ async function dispatchToolExecution( isAssistantIntegrationTool(getToolMetadata(toolCall.execName ?? toolName)) const gated = assistantAction ? frameRequestsApproval - : toolCallNeedsApproval(toolName, context, options, frameRequestsApproval, args) + : toolCallNeedsApproval( + toolName, + context, + options, + frameRequestsApproval, + args, + toolCall.execName + ) if (gated || clientExecutable) await adoptRecordedToolDeadline(toolCall) if (gated) { registerPendingToolPromise( diff --git a/apps/sim/lib/mothership/request/tools/permission.ts b/apps/sim/lib/mothership/request/tools/permission.ts index 44e4b231f65..2c179366551 100644 --- a/apps/sim/lib/mothership/request/tools/permission.ts +++ b/apps/sim/lib/mothership/request/tools/permission.ts @@ -1,6 +1,7 @@ import { createLogger } from '@sim/logger' import { TERMINAL_TOOL_NAME, type TerminalOperation } from '@sim/terminal-protocol' import { getErrorMessage } from '@sim/utils/errors' +import { cliCommandToolName } from '@/lib/mothership/agent-cli/approval' import type { AsyncCompletionSignal } from '@/lib/mothership/async-runs/lifecycle' import { PERMISSION_WAIT_TIMEOUT_MS } from '@/lib/mothership/constants' import { @@ -91,13 +92,19 @@ export function toolCallNeedsApproval( */ frameRequestsApproval = false, /** The call's arguments, for a tool whose gate depends on what it is doing. */ - args?: Record + args?: Record, + /** + * The tool that executes the call, when the frame names it apart from its display + * name: a `sim_cli` call is shown as the command it runs (`cli_workflows_deploy`), + * and always-allow is saved under that name. + */ + executableName = toolName ): boolean { if (!context.toolPermissions.enabled) return false if (options.interactive === false) return false if (!frameRequestsApproval) { - if (!toolRequiresApproval(toolName)) return false + if (!toolRequiresApproval(executableName, args)) return false if (toolName === TERMINAL_TOOL_NAME && !terminalOperationNeedsApproval(args)) return false // A go-routed tool executes inside mothership and never reaches Sim's // dispatch, so there is nothing here to hold. Stamping the frame anyway @@ -117,8 +124,9 @@ export function toolCallNeedsApproval( * leave every entry saved before the policy changed still silencing prompts. */ if (!context.toolPermissions.autoAllowPermitted) return true + if (!context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName))) return true - return !context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName)) + return executableName === 'sim_cli' && cliCommandToolName(args?.request) !== toolName } function skipOutput(toolName: string) { diff --git a/apps/sim/lib/mothership/tool-executor/router.test.ts b/apps/sim/lib/mothership/tool-executor/router.test.ts index da36cddacab..6d1afbae683 100644 --- a/apps/sim/lib/mothership/tool-executor/router.test.ts +++ b/apps/sim/lib/mothership/tool-executor/router.test.ts @@ -132,4 +132,15 @@ describe('toolRequiresApprovalLane', () => { setEnvFlags({ isCopilotToolPermissionsEnabled: true }) expect(toolRequiresApprovalLane('not_a_real_tool')).toBe(false) }) + + it.each([ + [['workflows', 'activate', 'create', 'wf-1', '1', '--yes'], true], + [['workflows', 'rollback', 'wf-1', '--to-version', '1', '--yes'], true], + [['workflows', 'list'], false], + ])('answers sim_cli %j by the command it runs: %s', (argv, gated) => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + expect( + toolRequiresApprovalLane('sim_cli', { request: { invocation: { kind: 'cli', argv } } }) + ).toBe(gated) + }) }) diff --git a/apps/sim/lib/mothership/tool-executor/router.ts b/apps/sim/lib/mothership/tool-executor/router.ts index b90e4f3ee75..e62c41028a7 100644 --- a/apps/sim/lib/mothership/tool-executor/router.ts +++ b/apps/sim/lib/mothership/tool-executor/router.ts @@ -1,5 +1,6 @@ import { BROWSER_TOOL_OBSERVES_ONLY, isBrowserToolName } from '@sim/browser-protocol' import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags' +import { cliRequestRequiresApproval } from '@/lib/mothership/agent-cli/approval' import { TOOL_CATALOG, type ToolCatalogEntry } from '@/lib/mothership/generated/tool-catalog-v1' export function isToolInCatalog(toolId: string): boolean { @@ -30,9 +31,11 @@ export function isKnownTool(toolId: string): boolean { * only observing it. One that acts does so as the user, in a browser signed in * to their accounts, so it is gated like a terminal command. Reading the split * from `BROWSER_TOOL_OBSERVES_ONLY` gates a browser tool added later by default. + * `sim_cli` has no entry: each call answers for the command its arguments would run. */ -export function toolRequiresApproval(toolId: string): boolean { +export function toolRequiresApproval(toolId: string, args?: Record): boolean { if (isBrowserToolName(toolId)) return !BROWSER_TOOL_OBSERVES_ONLY[toolId] + if (toolId === 'sim_cli') return cliRequestRequiresApproval(args?.request) return getToolEntry(toolId)?.requiresApproval === true } @@ -46,8 +49,8 @@ export function toolRequiresApproval(toolId: string): boolean { * must never hang on a prompt with no row behind it. * * Lives here rather than beside the dispatch gate so that asking the question - * costs only the catalog. The gate module reaches the permission persistence - * layer, which opens a pub/sub channel when it loads. + * costs only the catalog and the CLI's parser. The gate module reaches the + * permission persistence layer, which opens a pub/sub channel when it loads. * * Deliberately blind to the stored auto-allow list. Consulting it here would add * a database read to every in-band call to reach the same place by a longer @@ -56,6 +59,6 @@ export function toolRequiresApproval(toolId: string): boolean { * leaves the one implementation of "has the user allowed this" on the lane that * already owns it. */ -export function toolRequiresApprovalLane(toolId: string): boolean { - return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolId) +export function toolRequiresApprovalLane(toolId: string, args?: Record): boolean { + return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolId, args) } diff --git a/packages/sim-cli/src/embed.test.ts b/packages/sim-cli/src/embed.test.ts index 9f6f72bbe80..f47df6ce2d5 100644 --- a/packages/sim-cli/src/embed.test.ts +++ b/packages/sim-cli/src/embed.test.ts @@ -1,6 +1,6 @@ import { sleep } from '@sim/utils/helpers' import { describe, expect, it, vi } from 'vitest' -import { runEmbeddedCli } from './embed' +import { resolveEmbeddedCliCommand, runEmbeddedCli } from './embed' const IDENTITY = { endpoint: 'https://sim.internal.test', @@ -189,3 +189,44 @@ describe('embedded request defaults', () => { ) }) }) + +describe('resolveEmbeddedCliCommand', () => { + it('names the command argv would run and what it may call, however it is spelled', () => { + expect( + resolveEmbeddedCliCommand([ + '-w', + 'ws-1', + 'workflows', + 'activate', + 'create', + 'wf-1', + '1', + '--yes', + ]) + ).toEqual({ + path: ['workflows', 'activate', 'create'], + operations: ['activateWorkflowVersion'], + }) + expect(resolveEmbeddedCliCommand(['workflows', '-P', 'default', 'undeploy', 'wf-1'])).toEqual({ + path: ['workflows', 'undeploy'], + operations: ['undeployWorkflow'], + }) + expect(resolveEmbeddedCliCommand(['kb', 'list'])).toEqual({ + path: ['knowledge', 'list'], + operations: ['listKnowledgeBases'], + }) + }) + + it.each([ + [['--help']], + [['workflows', 'no-such-command']], + [['workflows', 'activate', 'create']], + [['workflows', 'activate', 'create', 'wf-1', '1', '--dry-run']], + [['workflows', 'run', 'wf-1', '--help']], + ])('reports no command for %j, and running it calls nothing', async (argv) => { + expect(resolveEmbeddedCliCommand(argv)).toBeNull() + const transport = vi.fn(async () => jsonResponse({ data: {} })) + await runEmbeddedCli(argv, { ...IDENTITY, transport }) + expect(transport).not.toHaveBeenCalled() + }) +}) diff --git a/packages/sim-cli/src/embed.ts b/packages/sim-cli/src/embed.ts index f3c52b3a0f0..a13f36747eb 100644 --- a/packages/sim-cli/src/embed.ts +++ b/packages/sim-cli/src/embed.ts @@ -1,5 +1,5 @@ import { getErrorMessage } from '@sim/utils/errors' -import { type Command, CommanderError } from 'commander' +import { type Command, CommanderError, type OutputConfiguration } from 'commander' import { ProfileConfigError } from './config/index' import { type EmbedContext, @@ -9,6 +9,7 @@ import { embedStore, } from './embed-context' import { EmbeddedOutput, EmbeddedOutputLimitError } from './embed-output' +import type { V2OperationName } from './generated/v2-api' import { formatApiErrorDetails, isRequestTimeout, @@ -19,6 +20,8 @@ import { import { writeStderr, writeStdout } from './output/io' import { sanitize } from './output/render' import { buildProgram } from './program' +import { calledOperations } from './runtime/called-operations' +import { commandPath } from './runtime/derive' /** * In-process execution of one CLI invocation, for a server that already knows @@ -40,6 +43,7 @@ export type { ListFilesResponse, ListWorkflowsResponse, ReadFileTextResponse, + V2OperationName, } from './generated/v2-api' export { SimApiError, SimClient } from './http/client' @@ -133,12 +137,7 @@ export async function runEmbeddedCli( let exitCode = 0 try { identity.signal?.throwIfAborted() - const program = buildProgram({ - hostCommands: false, - helpText: - 'The embedding application supplies authentication, endpoint and workspace scope.', - }) - configureEmbeddedOutput(program) + const program = buildEmbeddedProgram({ writeOut: writeStdout, writeErr: writeStderr }) await program.parseAsync(argv, { from: 'user' }) // Commands that soft-fail (a failed run outcome, wait timeout) report through the // context via setSoftExitCode — never process.exitCode, which is shared and raced @@ -203,9 +202,59 @@ function renderEmbeddedError(ctx: EmbedContext, error: unknown): number { return 1 } +function buildEmbeddedProgram(output: OutputConfiguration): Command { + const program = buildProgram({ + hostCommands: false, + helpText: 'The embedding application supplies authentication, endpoint and workspace scope.', + }) + configureEmbeddedOutput(program, output) + return program +} + /** Independently constructed subcommands do not inherit Commander output or exit overrides. */ -function configureEmbeddedOutput(command: Command): void { - command.configureOutput({ writeOut: writeStdout, writeErr: writeStderr }) +function configureEmbeddedOutput(command: Command, output: OutputConfiguration): void { + command.configureOutput(output) command.exitOverride() - for (const child of command.commands) configureEmbeddedOutput(child) + for (const child of command.commands) configureEmbeddedOutput(child, output) +} + +/** Carries the command a parse dispatched to out of the parse, before its action runs. */ +class DispatchedCommand { + constructor(readonly command: Command) {} +} + +const DISCARD_OUTPUT: OutputConfiguration = { writeOut: () => {}, writeErr: () => {} } + +interface EmbeddedCliCommand { + /** The command's canonical path (`['workflows', 'deploy']`), aliases resolved. */ + path: string[] + /** Every v2 operation it may call: a command reaches only those it declares (`connect`). */ + operations: readonly V2OperationName[] +} + +/** + * The command `argv` runs under {@link runEmbeddedCli}, resolved by the same command + * tree and parser and stopped before any action runs. + * + * Null when the parse ends without dispatching (help, a usage error), which calls + * nothing. Undefined when the parse could not be followed to an answer, so a caller + * deciding whether to stop for consent can fail closed. + */ +export function resolveEmbeddedCliCommand(argv: string[]): EmbeddedCliCommand | null | undefined { + try { + const program = buildEmbeddedProgram(DISCARD_OUTPUT) + program.hook('preAction', (_program, action) => { + throw new DispatchedCommand(action) + }) + program.parse(argv, { from: 'user' }) + } catch (error) { + if (error instanceof DispatchedCommand) { + return { + path: commandPath(error.command), + operations: calledOperations(error.command) ?? [], + } + } + if (error instanceof CommanderError) return null + } + return undefined } From 1f0cab170696047571deecaaa3a385e0e1391f86 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 2 Nov 2026 12:50:14 -0800 Subject: [PATCH 2/2] fix(copilot): type the sim_cli test frame as a tool call --- apps/sim/lib/mothership/request/handlers/handlers.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/mothership/request/handlers/handlers.test.ts b/apps/sim/lib/mothership/request/handlers/handlers.test.ts index e3dde17b745..1aadd9f8aa1 100644 --- a/apps/sim/lib/mothership/request/handlers/handlers.test.ts +++ b/apps/sim/lib/mothership/request/handlers/handlers.test.ts @@ -62,6 +62,7 @@ import { MothershipStreamV1ResourceOp, MothershipStreamV1RunKind, MothershipStreamV1TextChannel, + type MothershipStreamV1ToolCallDescriptor, MothershipStreamV1ToolExecutor, MothershipStreamV1ToolMode, MothershipStreamV1ToolOutcome, @@ -434,7 +435,11 @@ describe('sse-handlers tool lifecycle', () => { describe('sim_cli approval', () => { const activate = ['workflows', 'activate', 'create', 'wf-1', '1', '--yes'] - function cliFrame(toolCallId: string, toolName: string, argv: string[]): StreamEvent { + function cliFrame( + toolCallId: string, + toolName: string, + argv: string[] + ): StreamEvent & { payload: MothershipStreamV1ToolCallDescriptor } { return { type: MothershipStreamV1EventType.tool, payload: {