Skip to content

Commit 23fed10

Browse files
committed
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.
1 parent 1a14c87 commit 23fed10

9 files changed

Lines changed: 287 additions & 23 deletions

File tree

‎apps/sim/app/api/copilot/tools/execute/route.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ export const POST = withRouteHandler((request: NextRequest) =>
134134
* where the gate lives. Inert while copilot tool permissions are disabled, which keeps
135135
* enabling the flag from silently leaving background lanes ungated.
136136
*/
137-
if (toolRequiresApprovalLane(toolName)) {
137+
if (toolRequiresApprovalLane(toolName, params)) {
138138
logger.warn('Refusing an approval-gated tool on the in-band lane', {
139139
toolName,
140140
toolCallId,
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { resolveEmbeddedCliCommand, type V2OperationName } from 'sim/embed'
2+
import { AgentCliRequest } from '@/lib/mothership/generated/agent-cli'
3+
import {
4+
CallIntegrationTool,
5+
CancelWorkflowRun,
6+
DeleteWorkspaceMcpServer,
7+
DeployAsApi,
8+
DeployAsChat,
9+
DeployAsMcp,
10+
PromoteToLive,
11+
RunWorkflow,
12+
TOOL_CATALOG,
13+
type ToolCatalogEntry,
14+
} from '@/lib/mothership/generated/tool-catalog-v1'
15+
16+
/**
17+
* The catalog tool whose capability each v2 operation is. The worker retired these
18+
* tools in favour of `sim_cli`, so a command that calls one of these operations answers
19+
* to that tool's approval flag: one policy, whichever name the capability arrives under.
20+
* The deploy tools gate undeploy as well, so both directions map to them; rollback runs
21+
* the same use case as activating a version.
22+
*/
23+
const CATALOG_TOOL_BY_OPERATION: Partial<Record<V2OperationName, ToolCatalogEntry>> = {
24+
activateWorkflowVersion: PromoteToLive,
25+
rollbackWorkflow: PromoteToLive,
26+
deployWorkflow: DeployAsApi,
27+
undeployWorkflow: DeployAsApi,
28+
replaceWorkflowChatDeployment: DeployAsChat,
29+
deleteWorkflowChatDeployment: DeployAsChat,
30+
deployWorkflowMcpTool: DeployAsMcp,
31+
undeployWorkflowMcpTool: DeployAsMcp,
32+
deleteWorkflowMcpServer: DeleteWorkspaceMcpServer,
33+
executeWorkflow: RunWorkflow,
34+
resumeWorkflow: RunWorkflow,
35+
cancelWorkflowRun: CancelWorkflowRun,
36+
executeTool: CallIntegrationTool,
37+
}
38+
39+
/**
40+
* Whether a `sim_cli` call must be held for the user's decision. Its argv is resolved by
41+
* the CLI's own parser, so the answer is about the command that would run, and a `--yes`
42+
* in that argv answers only the CLI's prompt, never this one. Argv the parser cannot
43+
* follow is held; a malformed request is not, because `executeSimCli` refuses it.
44+
*/
45+
export function cliRequestRequiresApproval(request: unknown): boolean {
46+
const parsed = AgentCliRequest.safeParse(request)
47+
if (!parsed.success) return false
48+
const { invocation } = parsed.data
49+
if (invocation.kind === 'service') return TOOL_CATALOG[invocation.name]?.requiresApproval === true
50+
if (invocation.kind !== 'cli') return false
51+
const command = resolveEmbeddedCliCommand(invocation.argv)
52+
if (command === undefined) return true
53+
return (
54+
command?.operations.some(
55+
(operation) => CATALOG_TOOL_BY_OPERATION[operation]?.requiresApproval === true
56+
) === true
57+
)
58+
}
59+
60+
/**
61+
* The tool name (`cli_workflows_deploy`) of the command a `sim_cli` call would run, as the
62+
* CLI's parser resolves it. The row's own name is the worker's reading of the same argv;
63+
* an always-allow saved under that name covers the call only when the two agree, so a
64+
* row named for one command, or for no command at all, never carries another's consent.
65+
*/
66+
export function cliCommandToolName(request: unknown): string | undefined {
67+
const parsed = AgentCliRequest.safeParse(request)
68+
if (!parsed.success || parsed.data.invocation.kind !== 'cli') return undefined
69+
const command = resolveEmbeddedCliCommand(parsed.data.invocation.argv)
70+
return command ? `cli_${command.path.join('_').replace(/-/g, '_')}` : undefined
71+
}

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

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
mothershipClientToolWaiterMockFns,
1010
} from '@sim/testing/mocks/mothership-client-tool-waiter.mock'
1111
import { sleep } from '@sim/utils/helpers'
12-
import { beforeEach, describe, expect, it, vi } from 'vitest'
12+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
1313
import { AsyncToolCallOwnershipError } from '@/lib/mothership/async-runs/errors'
1414
import { restoreStreamingContext } from '@/lib/mothership/request/context/restore'
1515
import { TraceCollector } from '@/lib/mothership/request/trace'
@@ -431,6 +431,79 @@ describe('sse-handlers tool lifecycle', () => {
431431
expect(event.payload.status).toBe('awaiting_approval')
432432
})
433433

434+
describe('sim_cli approval', () => {
435+
const activate = ['workflows', 'activate', 'create', 'wf-1', '1', '--yes']
436+
437+
function cliFrame(toolCallId: string, toolName: string, argv: string[]): StreamEvent {
438+
return {
439+
type: MothershipStreamV1EventType.tool,
440+
payload: {
441+
toolCallId,
442+
toolName,
443+
execName: 'sim_cli',
444+
arguments: { args: argv, request: { invocation: { kind: 'cli', argv } } },
445+
executor: MothershipStreamV1ToolExecutor.sim,
446+
mode: MothershipStreamV1ToolMode.async,
447+
phase: MothershipStreamV1ToolPhase.call,
448+
},
449+
}
450+
}
451+
452+
beforeEach(async () => {
453+
const router = await vi.importActual<typeof import('@/lib/mothership/tool-executor/router')>(
454+
'@/lib/mothership/tool-executor/router'
455+
)
456+
toolRequiresApproval.mockImplementation(router.toolRequiresApproval)
457+
context.runId = 'run-1'
458+
context.toolPermissions = {
459+
enabled: true,
460+
autoAllowed: new Set(),
461+
autoAllowPermitted: true,
462+
}
463+
})
464+
465+
afterEach(() => {
466+
toolRequiresApproval.mockReset()
467+
toolRequiresApproval.mockReturnValue(false)
468+
})
469+
470+
it('holds a command that performs a gated capability, though its argv says --yes', async () => {
471+
const event = cliFrame('cli-activate', 'cli_workflows_activate_create', activate)
472+
473+
await prePersistClientExecutableToolCall(event, context, {})
474+
475+
expect(event.payload.status).toBe('awaiting_approval')
476+
expect(upsertAsyncToolCall).toHaveBeenCalledWith(
477+
expect.objectContaining({
478+
toolName: 'cli_workflows_activate_create',
479+
executableName: 'sim_cli',
480+
permissionRequested: true,
481+
})
482+
)
483+
})
484+
485+
it('lets always-allow cover only the command it was granted for', async () => {
486+
context.toolPermissions.autoAllowed.add('cli_workflows_list')
487+
context.toolPermissions.autoAllowed.add('sim_cli')
488+
const held = cliFrame('cli-held', 'cli_workflows_activate_create', activate)
489+
await prePersistClientExecutableToolCall(held, context, {})
490+
expect(held.payload.status).toBe('awaiting_approval')
491+
492+
// A row named for an allowed command, or for none, does not carry its consent
493+
// to the command the CLI would actually run.
494+
for (const name of ['cli_workflows_list', 'sim_cli']) {
495+
const mislabeled = cliFrame(`cli-${name}`, name, activate)
496+
await prePersistClientExecutableToolCall(mislabeled, context, {})
497+
expect(mislabeled.payload.status).toBe('awaiting_approval')
498+
}
499+
500+
context.toolPermissions.autoAllowed.add('cli_workflows_activate_create')
501+
const allowed = cliFrame('cli-allowed', 'cli_workflows_activate_create', activate)
502+
await prePersistClientExecutableToolCall(allowed, context, {})
503+
expect(allowed.payload.status).toBeUndefined()
504+
})
505+
})
506+
434507
it('clears a Go-stamped approval frame when the gate is off', async () => {
435508
// Go stamps integration calls regardless of Sim's feature flag. Forwarding
436509
// that stamp with nothing gating behind it would draw a card whose buttons

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,8 @@ export async function prePersistClientExecutableToolCall(
330330
context,
331331
options ?? {},
332332
frameRequestsApproval,
333-
data.arguments
333+
data.arguments,
334+
data.execName
334335
)
335336
if (gated) {
336337
data.status = TOOL_AWAITING_APPROVAL_STATUS
@@ -1029,7 +1030,14 @@ async function dispatchToolExecution(
10291030
isAssistantIntegrationTool(getToolMetadata(toolCall.execName ?? toolName))
10301031
const gated = assistantAction
10311032
? frameRequestsApproval
1032-
: toolCallNeedsApproval(toolName, context, options, frameRequestsApproval, args)
1033+
: toolCallNeedsApproval(
1034+
toolName,
1035+
context,
1036+
options,
1037+
frameRequestsApproval,
1038+
args,
1039+
toolCall.execName
1040+
)
10331041
if (gated || clientExecutable) await adoptRecordedToolDeadline(toolCall)
10341042
if (gated) {
10351043
registerPendingToolPromise(

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { createLogger } from '@sim/logger'
22
import { TERMINAL_TOOL_NAME, type TerminalOperation } from '@sim/terminal-protocol'
33
import { getErrorMessage } from '@sim/utils/errors'
4+
import { cliCommandToolName } from '@/lib/mothership/agent-cli/approval'
45
import type { AsyncCompletionSignal } from '@/lib/mothership/async-runs/lifecycle'
56
import { PERMISSION_WAIT_TIMEOUT_MS } from '@/lib/mothership/constants'
67
import {
@@ -91,13 +92,19 @@ export function toolCallNeedsApproval(
9192
*/
9293
frameRequestsApproval = false,
9394
/** The call's arguments, for a tool whose gate depends on what it is doing. */
94-
args?: Record<string, unknown>
95+
args?: Record<string, unknown>,
96+
/**
97+
* The tool that executes the call, when the frame names it apart from its display
98+
* name: a `sim_cli` call is shown as the command it runs (`cli_workflows_deploy`),
99+
* and always-allow is saved under that name.
100+
*/
101+
executableName = toolName
95102
): boolean {
96103
if (!context.toolPermissions.enabled) return false
97104
if (options.interactive === false) return false
98105

99106
if (!frameRequestsApproval) {
100-
if (!toolRequiresApproval(toolName)) return false
107+
if (!toolRequiresApproval(executableName, args)) return false
101108
if (toolName === TERMINAL_TOOL_NAME && !terminalOperationNeedsApproval(args)) return false
102109
// A go-routed tool executes inside mothership and never reaches Sim's
103110
// dispatch, so there is nothing here to hold. Stamping the frame anyway
@@ -117,8 +124,9 @@ export function toolCallNeedsApproval(
117124
* leave every entry saved before the policy changed still silencing prompts.
118125
*/
119126
if (!context.toolPermissions.autoAllowPermitted) return true
127+
if (!context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName))) return true
120128

121-
return !context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName))
129+
return executableName === 'sim_cli' && cliCommandToolName(args?.request) !== toolName
122130
}
123131

124132
function skipOutput(toolName: string) {

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,4 +132,15 @@ describe('toolRequiresApprovalLane', () => {
132132
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
133133
expect(toolRequiresApprovalLane('not_a_real_tool')).toBe(false)
134134
})
135+
136+
it.each([
137+
[['workflows', 'activate', 'create', 'wf-1', '1', '--yes'], true],
138+
[['workflows', 'rollback', 'wf-1', '--to-version', '1', '--yes'], true],
139+
[['workflows', 'list'], false],
140+
])('answers sim_cli %j by the command it runs: %s', (argv, gated) => {
141+
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
142+
expect(
143+
toolRequiresApprovalLane('sim_cli', { request: { invocation: { kind: 'cli', argv } } })
144+
).toBe(gated)
145+
})
135146
})

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { BROWSER_TOOL_OBSERVES_ONLY, isBrowserToolName } from '@sim/browser-protocol'
22
import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags'
3+
import { cliRequestRequiresApproval } from '@/lib/mothership/agent-cli/approval'
34
import { TOOL_CATALOG, type ToolCatalogEntry } from '@/lib/mothership/generated/tool-catalog-v1'
45

56
export function isToolInCatalog(toolId: string): boolean {
@@ -30,9 +31,11 @@ export function isKnownTool(toolId: string): boolean {
3031
* only observing it. One that acts does so as the user, in a browser signed in
3132
* to their accounts, so it is gated like a terminal command. Reading the split
3233
* from `BROWSER_TOOL_OBSERVES_ONLY` gates a browser tool added later by default.
34+
* `sim_cli` has no entry: each call answers for the command its arguments would run.
3335
*/
34-
export function toolRequiresApproval(toolId: string): boolean {
36+
export function toolRequiresApproval(toolId: string, args?: Record<string, unknown>): boolean {
3537
if (isBrowserToolName(toolId)) return !BROWSER_TOOL_OBSERVES_ONLY[toolId]
38+
if (toolId === 'sim_cli') return cliRequestRequiresApproval(args?.request)
3639
return getToolEntry(toolId)?.requiresApproval === true
3740
}
3841

@@ -46,8 +49,8 @@ export function toolRequiresApproval(toolId: string): boolean {
4649
* must never hang on a prompt with no row behind it.
4750
*
4851
* Lives here rather than beside the dispatch gate so that asking the question
49-
* costs only the catalog. The gate module reaches the permission persistence
50-
* layer, which opens a pub/sub channel when it loads.
52+
* costs only the catalog and the CLI's parser. The gate module reaches the
53+
* permission persistence layer, which opens a pub/sub channel when it loads.
5154
*
5255
* Deliberately blind to the stored auto-allow list. Consulting it here would add
5356
* 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 {
5659
* leaves the one implementation of "has the user allowed this" on the lane that
5760
* already owns it.
5861
*/
59-
export function toolRequiresApprovalLane(toolId: string): boolean {
60-
return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolId)
62+
export function toolRequiresApprovalLane(toolId: string, args?: Record<string, unknown>): boolean {
63+
return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolId, args)
6164
}

‎packages/sim-cli/src/embed.test.ts‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { sleep } from '@sim/utils/helpers'
22
import { describe, expect, it, vi } from 'vitest'
3-
import { runEmbeddedCli } from './embed'
3+
import { resolveEmbeddedCliCommand, runEmbeddedCli } from './embed'
44

55
const IDENTITY = {
66
endpoint: 'https://sim.internal.test',
@@ -189,3 +189,44 @@ describe('embedded request defaults', () => {
189189
)
190190
})
191191
})
192+
193+
describe('resolveEmbeddedCliCommand', () => {
194+
it('names the command argv would run and what it may call, however it is spelled', () => {
195+
expect(
196+
resolveEmbeddedCliCommand([
197+
'-w',
198+
'ws-1',
199+
'workflows',
200+
'activate',
201+
'create',
202+
'wf-1',
203+
'1',
204+
'--yes',
205+
])
206+
).toEqual({
207+
path: ['workflows', 'activate', 'create'],
208+
operations: ['activateWorkflowVersion'],
209+
})
210+
expect(resolveEmbeddedCliCommand(['workflows', '-P', 'default', 'undeploy', 'wf-1'])).toEqual({
211+
path: ['workflows', 'undeploy'],
212+
operations: ['undeployWorkflow'],
213+
})
214+
expect(resolveEmbeddedCliCommand(['kb', 'list'])).toEqual({
215+
path: ['knowledge', 'list'],
216+
operations: ['listKnowledgeBases'],
217+
})
218+
})
219+
220+
it.each([
221+
[['--help']],
222+
[['workflows', 'no-such-command']],
223+
[['workflows', 'activate', 'create']],
224+
[['workflows', 'activate', 'create', 'wf-1', '1', '--dry-run']],
225+
[['workflows', 'run', 'wf-1', '--help']],
226+
])('reports no command for %j, and running it calls nothing', async (argv) => {
227+
expect(resolveEmbeddedCliCommand(argv)).toBeNull()
228+
const transport = vi.fn(async () => jsonResponse({ data: {} }))
229+
await runEmbeddedCli(argv, { ...IDENTITY, transport })
230+
expect(transport).not.toHaveBeenCalled()
231+
})
232+
})

0 commit comments

Comments
 (0)