Skip to content

Commit 87e6419

Browse files
committed
improvement(copilot): refuse approval-gated tools on the in-band lane
Copilot's approval gate is scaffolding today: COPILOT_TOOL_PERMISSIONS_ENABLED is off by default, so nothing is gated on any lane. It is built only on the dispatch lane, which holds a call against a streaming context and a decision row and then declines to dispatch anything the mothership marks in-band. Those calls run via POST /api/copilot/tools/execute, which has no context and no waiter, so turning the flag on would gate the foreground and leave background lanes ungated — a gate that looks enforced but is not. Add toolRequiresApprovalLane next to toolCallNeedsApproval so the covered tool set is defined once, and refuse a gated tool at the in-band route before it runs. Refuse rather than block: a background lane must never hang on a prompt with no row behind it. The check deliberately ignores the stored auto-allow list — an auto-allowed tool sent to the checkpoint lane is admitted there without prompting anyone, so reading it here would only add a database read to reach the same place. Inert while the flag is off, which is the state this ships in; a test pins that. Also record on the flag itself that the gate is a property of the lane, since that is what the next person reads before enabling it.
1 parent 0d9e256 commit 87e6419

5 files changed

Lines changed: 161 additions & 3 deletions

File tree

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

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,20 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { beforeEach, describe, expect, it, vi } from 'vitest'
4+
import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock'
5+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
56
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
67

7-
const { mockCheckInternalApiKey, mockPrepareEnvironmentContext, mockHandler } = vi.hoisted(() => ({
8+
const {
9+
mockCheckInternalApiKey,
10+
mockPrepareEnvironmentContext,
11+
mockHandler,
12+
mockToolRequiresApproval,
13+
} = vi.hoisted(() => ({
814
mockCheckInternalApiKey: vi.fn(),
915
mockPrepareEnvironmentContext: vi.fn(),
1016
mockHandler: vi.fn(),
17+
mockToolRequiresApproval: vi.fn().mockReturnValue(false),
1118
}))
1219

1320
vi.mock('@/lib/copilot/request/http', () => ({
@@ -20,6 +27,7 @@ vi.mock('@/lib/copilot/environment-context', () => ({
2027

2128
vi.mock('@/lib/copilot/tool-executor', () => ({
2229
ensureHandlersRegistered: vi.fn(),
30+
toolRequiresApproval: mockToolRequiresApproval,
2331
}))
2432

2533
vi.mock('@/lib/copilot/tool-executor/executor', () => ({
@@ -67,6 +75,7 @@ describe('POST /api/copilot/tools/execute (in-band)', () => {
6775
beforeEach(() => {
6876
vi.clearAllMocks()
6977
mockCheckInternalApiKey.mockReturnValue({ success: true })
78+
mockToolRequiresApproval.mockReturnValue(false)
7079
// A fresh, complete registry per test: the module-level turn cache is keyed
7180
// by messageId, so each test uses a distinct messageId to avoid cross-test
7281
// cache hits.
@@ -161,6 +170,68 @@ describe('POST /api/copilot/tools/execute (in-band)', () => {
161170
})
162171
})
163172

173+
describe('approval-gated tools', () => {
174+
afterEach(resetEnvFlagsMock)
175+
176+
/**
177+
* This lane cannot hold an approval prompt: the dispatch handler owns the gate and
178+
* declines to dispatch in-band calls, so a gated tool arriving here has no waiter behind
179+
* it. Refuse before running anything rather than execute on consent nobody gave.
180+
*/
181+
it('refuses an approval-gated tool without executing it when permissions are enabled', async () => {
182+
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
183+
mockToolRequiresApproval.mockReturnValue(true)
184+
mockHandler.mockResolvedValue({ success: true, output: { ran: true } })
185+
186+
const res = await POST(
187+
makeRequest({
188+
...BASE_BODY,
189+
toolName: 'run_function',
190+
params: { code: 'return 1' },
191+
messageId: 'msg-gated',
192+
}) as never
193+
)
194+
const body = await res.json()
195+
196+
expect(mockHandler).not.toHaveBeenCalled()
197+
expect(body.success).toBe(false)
198+
expect(body.output).toEqual({ resultWithheld: true, effect: 'not_attempted' })
199+
expect(body.error).toContain('requires user approval')
200+
expect(body.error).toContain('checkpoint lane')
201+
})
202+
203+
it('still runs a tool the catalog does not gate when permissions are enabled', async () => {
204+
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
205+
mockHandler.mockResolvedValue({ success: true, output: { content: 'hello' } })
206+
207+
const res = await POST(makeRequest({ ...BASE_BODY, messageId: 'msg-ungated' }) as never)
208+
209+
expect(mockHandler).toHaveBeenCalledTimes(1)
210+
await expect(res.json()).resolves.toEqual({ success: true, output: { content: 'hello' } })
211+
})
212+
213+
/**
214+
* The guard is inert while the feature is off, which is the state this ships in — enabling
215+
* the flag is what makes it bite, so it cannot change in-band behavior today.
216+
*/
217+
it('runs an approval-gated tool unchanged while permissions are disabled', async () => {
218+
mockToolRequiresApproval.mockReturnValue(true)
219+
mockHandler.mockResolvedValue({ success: true, output: { ran: true } })
220+
221+
const res = await POST(
222+
makeRequest({
223+
...BASE_BODY,
224+
toolName: 'run_function',
225+
params: { code: 'return 1' },
226+
messageId: 'msg-gated-flag-off',
227+
}) as never
228+
)
229+
230+
expect(mockHandler).toHaveBeenCalledTimes(1)
231+
await expect(res.json()).resolves.toEqual({ success: true, output: { ran: true } })
232+
})
233+
})
234+
164235
it('passes a failed generate_api_key call through with its error', async () => {
165236
mockHandler.mockResolvedValue({ success: false, error: 'name is required' })
166237
const res = await POST(

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1'
1010
import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1'
1111
import { checkInternalApiKey } from '@/lib/copilot/request/http'
1212
import { withIncomingGoSpan } from '@/lib/copilot/request/otel'
13+
import { toolRequiresApprovalLane } from '@/lib/copilot/request/tools/permission'
1314
import {
1415
describeWithholdingCause,
1516
inspectToolResultForCopilot,
@@ -126,6 +127,29 @@ export const POST = withRouteHandler((request: NextRequest) =>
126127
[TraceAttr.UserId]: userId,
127128
})
128129

130+
/**
131+
* Cheap admission, before any work: this lane cannot hold an approval prompt. The
132+
* dispatch handler gates `requiresApproval` tools against a streaming context and a
133+
* decision row, then deliberately declines to dispatch anything the mothership marks
134+
* in-band — so a gated tool arriving here has no waiter behind it and would run on
135+
* consent nobody gave. Refuse instead, and let the mothership take the checkpoint lane
136+
* where the gate lives. Inert while copilot tool permissions are disabled, which keeps
137+
* enabling the flag from silently leaving background lanes ungated.
138+
*/
139+
if (toolRequiresApprovalLane(toolName)) {
140+
logger.warn('Refusing an approval-gated tool on the in-band lane', {
141+
toolName,
142+
toolCallId,
143+
userId,
144+
})
145+
rootSpan.setAttributes({ [TraceAttr.ToolOutcome]: MothershipStreamV1ToolOutcome.error })
146+
return NextResponse.json({
147+
success: false,
148+
error: `${toolName} was not run: it requires user approval, and this lane cannot hold an approval prompt. Dispatch it on the checkpoint lane instead.`,
149+
output: { resultWithheld: true, effect: TOOL_EFFECT_PHASE.notAttempted },
150+
})
151+
}
152+
129153
let toolRegistry: ResolvedSecretTraceRegistry
130154
let turnRegistry: ResolvedSecretTraceRegistry
131155
try {

‎apps/sim/lib/copilot/request/tools/permission.test.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@
22
* @vitest-environment node
33
*/
44

5-
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
67
import { TraceCollector } from '@/lib/copilot/request/trace'
78

89
const { toolRequiresApproval, waitForToolPermissionDecision } = vi.hoisted(() => ({
@@ -28,6 +29,7 @@ import { createStreamingContext } from '@/lib/copilot/request/context/request-co
2829
import {
2930
runGatedToolExecution,
3031
toolCallNeedsApproval,
32+
toolRequiresApprovalLane,
3133
} from '@/lib/copilot/request/tools/permission'
3234
import type { StreamEvent, ToolCallState } from '@/lib/copilot/request/types'
3335

@@ -73,6 +75,38 @@ function gate(
7375
)
7476
}
7577

78+
describe('toolRequiresApprovalLane', () => {
79+
// vi.clearAllMocks() clears calls but not implementations, so a mockReturnValue
80+
// set here would otherwise outlive this block and silently flip the suites below.
81+
afterEach(() => {
82+
resetEnvFlagsMock()
83+
toolRequiresApproval.mockReturnValue(true)
84+
})
85+
86+
/**
87+
* Asked by lanes that cannot hold a prompt, so it answers from the catalog and the
88+
* feature flag alone — there is no streaming context to consult, and the stored
89+
* auto-allow list is deliberately not read (an auto-allowed tool is admitted on the
90+
* checkpoint lane without prompting anyone).
91+
*/
92+
it('is false while the feature is off, whatever the catalog says', () => {
93+
toolRequiresApproval.mockReturnValue(true)
94+
expect(toolRequiresApprovalLane('run_function')).toBe(false)
95+
})
96+
97+
it('is true for a catalog-gated tool once the feature is on', () => {
98+
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
99+
toolRequiresApproval.mockReturnValue(true)
100+
expect(toolRequiresApprovalLane('run_function')).toBe(true)
101+
})
102+
103+
it('is false for an ungated tool once the feature is on', () => {
104+
setEnvFlags({ isCopilotToolPermissionsEnabled: true })
105+
toolRequiresApproval.mockReturnValue(false)
106+
expect(toolRequiresApprovalLane('read')).toBe(false)
107+
})
108+
})
109+
76110
describe('toolCallNeedsApproval', () => {
77111
const runCall = { operation: 'run', args: { command: 'ls' } }
78112

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import type {
2727
ToolCallState,
2828
} from '@/lib/copilot/request/types'
2929
import { getToolEntry, toolRequiresApproval } from '@/lib/copilot/tool-executor'
30+
import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags'
3031

3132
const logger = createLogger('CopilotToolPermissionGate')
3233

@@ -51,6 +52,26 @@ const PERMISSION_WAIT_TIMEOUT_MS = ORCHESTRATION_TIMEOUT_MS
5152

5253
export const TOOL_AWAITING_APPROVAL_STATUS = MothershipStreamV1ToolStatus.awaiting_approval
5354

55+
/**
56+
* Whether a tool may only run on a lane that is able to hold an approval prompt.
57+
*
58+
* `toolCallNeedsApproval` answers for the dispatch lane, where a streaming
59+
* context exists to gate against. The in-band route has neither a context nor a
60+
* waiter — the mothership executes those calls itself — so it asks this instead,
61+
* before running anything, and refuses rather than blocks: a background lane
62+
* must never hang on a prompt with no row behind it.
63+
*
64+
* Deliberately blind to the stored auto-allow list. Consulting it here would add
65+
* a database read to every in-band call to reach the same place by a longer
66+
* route: an auto-allowed tool sent to the checkpoint lane is admitted there
67+
* without prompting anyone. Refusing unconditionally keeps this fail-closed and
68+
* leaves the one implementation of "has the user allowed this" on the lane that
69+
* already owns it.
70+
*/
71+
export function toolRequiresApprovalLane(toolName: string): boolean {
72+
return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolName)
73+
}
74+
5475
/**
5576
* Whether this call must be held for an explicit user decision.
5677
*

‎apps/sim/lib/core/config/env-flags.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,14 @@ export const isStatusNoticePreviewEnabled = isTruthy(getEnv('NEXT_PUBLIC_STATUS_
9292
* used tools, so it is an opt-in change in how the product feels, not just a
9393
* safety toggle. With it off nothing is stamped, gated, or persisted, and an
9494
* approval stamp arriving from Go is cleared on the way to the client.
95+
*
96+
* The gate is a property of the lane, not only of the tool. Sim can hold a call
97+
* for a decision on the dispatch lane, where a streaming context and a decision
98+
* row exist. It cannot on the in-band route (`POST /api/copilot/tools/execute`),
99+
* which the mothership drives for background lanes, so that route refuses a
100+
* `requiresApproval` tool outright rather than running it ungated — see
101+
* `toolRequiresApprovalLane`. Any new execution lane has to answer the same
102+
* question before this flag is turned on.
95103
*/
96104
export const isCopilotToolPermissionsEnabled = isTruthy(env.COPILOT_TOOL_PERMISSIONS_ENABLED)
97105

0 commit comments

Comments
 (0)