From 87e64192cc488f0c06b916bd4c23c1795760abd4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 19 Sep 2026 17:11:58 -0700 Subject: [PATCH 1/2] improvement(copilot): refuse approval-gated tools on the in-band lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../api/copilot/tools/execute/route.test.ts | 75 ++++++++++++++++++- .../app/api/copilot/tools/execute/route.ts | 24 ++++++ .../copilot/request/tools/permission.test.ts | 36 ++++++++- .../lib/copilot/request/tools/permission.ts | 21 ++++++ apps/sim/lib/core/config/env-flags.ts | 8 ++ 5 files changed, 161 insertions(+), 3 deletions(-) diff --git a/apps/sim/app/api/copilot/tools/execute/route.test.ts b/apps/sim/app/api/copilot/tools/execute/route.test.ts index 79efd345ce4..c8a314108dc 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.test.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.test.ts @@ -1,13 +1,20 @@ /** * @vitest-environment node */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' -const { mockCheckInternalApiKey, mockPrepareEnvironmentContext, mockHandler } = vi.hoisted(() => ({ +const { + mockCheckInternalApiKey, + mockPrepareEnvironmentContext, + mockHandler, + mockToolRequiresApproval, +} = vi.hoisted(() => ({ mockCheckInternalApiKey: vi.fn(), mockPrepareEnvironmentContext: vi.fn(), mockHandler: vi.fn(), + mockToolRequiresApproval: vi.fn().mockReturnValue(false), })) vi.mock('@/lib/copilot/request/http', () => ({ @@ -20,6 +27,7 @@ vi.mock('@/lib/copilot/environment-context', () => ({ vi.mock('@/lib/copilot/tool-executor', () => ({ ensureHandlersRegistered: vi.fn(), + toolRequiresApproval: mockToolRequiresApproval, })) vi.mock('@/lib/copilot/tool-executor/executor', () => ({ @@ -67,6 +75,7 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { beforeEach(() => { vi.clearAllMocks() mockCheckInternalApiKey.mockReturnValue({ success: true }) + mockToolRequiresApproval.mockReturnValue(false) // A fresh, complete registry per test: the module-level turn cache is keyed // by messageId, so each test uses a distinct messageId to avoid cross-test // cache hits. @@ -161,6 +170,68 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { }) }) + describe('approval-gated tools', () => { + afterEach(resetEnvFlagsMock) + + /** + * This lane cannot hold an approval prompt: the dispatch handler owns the gate and + * declines to dispatch in-band calls, so a gated tool arriving here has no waiter behind + * it. Refuse before running anything rather than execute on consent nobody gave. + */ + it('refuses an approval-gated tool without executing it when permissions are enabled', async () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + mockToolRequiresApproval.mockReturnValue(true) + mockHandler.mockResolvedValue({ success: true, output: { ran: true } }) + + const res = await POST( + makeRequest({ + ...BASE_BODY, + toolName: 'run_function', + params: { code: 'return 1' }, + messageId: 'msg-gated', + }) as never + ) + const body = await res.json() + + expect(mockHandler).not.toHaveBeenCalled() + expect(body.success).toBe(false) + expect(body.output).toEqual({ resultWithheld: true, effect: 'not_attempted' }) + expect(body.error).toContain('requires user approval') + expect(body.error).toContain('checkpoint lane') + }) + + it('still runs a tool the catalog does not gate when permissions are enabled', async () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + mockHandler.mockResolvedValue({ success: true, output: { content: 'hello' } }) + + const res = await POST(makeRequest({ ...BASE_BODY, messageId: 'msg-ungated' }) as never) + + expect(mockHandler).toHaveBeenCalledTimes(1) + await expect(res.json()).resolves.toEqual({ success: true, output: { content: 'hello' } }) + }) + + /** + * The guard is inert while the feature is off, which is the state this ships in — enabling + * the flag is what makes it bite, so it cannot change in-band behavior today. + */ + it('runs an approval-gated tool unchanged while permissions are disabled', async () => { + mockToolRequiresApproval.mockReturnValue(true) + mockHandler.mockResolvedValue({ success: true, output: { ran: true } }) + + const res = await POST( + makeRequest({ + ...BASE_BODY, + toolName: 'run_function', + params: { code: 'return 1' }, + messageId: 'msg-gated-flag-off', + }) as never + ) + + expect(mockHandler).toHaveBeenCalledTimes(1) + await expect(res.json()).resolves.toEqual({ success: true, output: { ran: true } }) + }) + }) + it('passes a failed generate_api_key call through with its error', async () => { mockHandler.mockResolvedValue({ success: false, error: 'name is required' }) const res = await POST( diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index 284a2e5286c..f1f3d67bcc2 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -10,6 +10,7 @@ import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1' import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1' import { checkInternalApiKey } from '@/lib/copilot/request/http' import { withIncomingGoSpan } from '@/lib/copilot/request/otel' +import { toolRequiresApprovalLane } from '@/lib/copilot/request/tools/permission' import { describeWithholdingCause, inspectToolResultForCopilot, @@ -126,6 +127,29 @@ export const POST = withRouteHandler((request: NextRequest) => [TraceAttr.UserId]: userId, }) + /** + * Cheap admission, before any work: this lane cannot hold an approval prompt. The + * dispatch handler gates `requiresApproval` tools against a streaming context and a + * decision row, then deliberately declines to dispatch anything the mothership marks + * in-band — so a gated tool arriving here has no waiter behind it and would run on + * consent nobody gave. Refuse instead, and let the mothership take the checkpoint lane + * 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)) { + logger.warn('Refusing an approval-gated tool on the in-band lane', { + toolName, + toolCallId, + userId, + }) + rootSpan.setAttributes({ [TraceAttr.ToolOutcome]: MothershipStreamV1ToolOutcome.error }) + return NextResponse.json({ + success: false, + error: `${toolName} was not run: it requires user approval, and this lane cannot hold an approval prompt. Dispatch it on the checkpoint lane instead.`, + output: { resultWithheld: true, effect: TOOL_EFFECT_PHASE.notAttempted }, + }) + } + let toolRegistry: ResolvedSecretTraceRegistry let turnRegistry: ResolvedSecretTraceRegistry try { diff --git a/apps/sim/lib/copilot/request/tools/permission.test.ts b/apps/sim/lib/copilot/request/tools/permission.test.ts index 07765132b18..3bea2f1a097 100644 --- a/apps/sim/lib/copilot/request/tools/permission.test.ts +++ b/apps/sim/lib/copilot/request/tools/permission.test.ts @@ -2,7 +2,8 @@ * @vitest-environment node */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { TraceCollector } from '@/lib/copilot/request/trace' const { toolRequiresApproval, waitForToolPermissionDecision } = vi.hoisted(() => ({ @@ -28,6 +29,7 @@ import { createStreamingContext } from '@/lib/copilot/request/context/request-co import { runGatedToolExecution, toolCallNeedsApproval, + toolRequiresApprovalLane, } from '@/lib/copilot/request/tools/permission' import type { StreamEvent, ToolCallState } from '@/lib/copilot/request/types' @@ -73,6 +75,38 @@ function gate( ) } +describe('toolRequiresApprovalLane', () => { + // vi.clearAllMocks() clears calls but not implementations, so a mockReturnValue + // set here would otherwise outlive this block and silently flip the suites below. + afterEach(() => { + resetEnvFlagsMock() + toolRequiresApproval.mockReturnValue(true) + }) + + /** + * Asked by lanes that cannot hold a prompt, so it answers from the catalog and the + * feature flag alone — there is no streaming context to consult, and the stored + * auto-allow list is deliberately not read (an auto-allowed tool is admitted on the + * checkpoint lane without prompting anyone). + */ + it('is false while the feature is off, whatever the catalog says', () => { + toolRequiresApproval.mockReturnValue(true) + expect(toolRequiresApprovalLane('run_function')).toBe(false) + }) + + it('is true for a catalog-gated tool once the feature is on', () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + toolRequiresApproval.mockReturnValue(true) + expect(toolRequiresApprovalLane('run_function')).toBe(true) + }) + + it('is false for an ungated tool once the feature is on', () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + toolRequiresApproval.mockReturnValue(false) + expect(toolRequiresApprovalLane('read')).toBe(false) + }) +}) + describe('toolCallNeedsApproval', () => { const runCall = { operation: 'run', args: { command: 'ls' } } diff --git a/apps/sim/lib/copilot/request/tools/permission.ts b/apps/sim/lib/copilot/request/tools/permission.ts index 1e050b67e45..7060319258f 100644 --- a/apps/sim/lib/copilot/request/tools/permission.ts +++ b/apps/sim/lib/copilot/request/tools/permission.ts @@ -27,6 +27,7 @@ import type { ToolCallState, } from '@/lib/copilot/request/types' import { getToolEntry, toolRequiresApproval } from '@/lib/copilot/tool-executor' +import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags' const logger = createLogger('CopilotToolPermissionGate') @@ -51,6 +52,26 @@ const PERMISSION_WAIT_TIMEOUT_MS = ORCHESTRATION_TIMEOUT_MS export const TOOL_AWAITING_APPROVAL_STATUS = MothershipStreamV1ToolStatus.awaiting_approval +/** + * Whether a tool may only run on a lane that is able to hold an approval prompt. + * + * `toolCallNeedsApproval` answers for the dispatch lane, where a streaming + * context exists to gate against. The in-band route has neither a context nor a + * waiter — the mothership executes those calls itself — so it asks this instead, + * before running anything, and refuses rather than blocks: a background lane + * must never hang on a prompt with no row behind it. + * + * 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 + * route: an auto-allowed tool sent to the checkpoint lane is admitted there + * without prompting anyone. Refusing unconditionally keeps this fail-closed and + * leaves the one implementation of "has the user allowed this" on the lane that + * already owns it. + */ +export function toolRequiresApprovalLane(toolName: string): boolean { + return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolName) +} + /** * Whether this call must be held for an explicit user decision. * diff --git a/apps/sim/lib/core/config/env-flags.ts b/apps/sim/lib/core/config/env-flags.ts index dd16bc9ab0c..241803c114c 100644 --- a/apps/sim/lib/core/config/env-flags.ts +++ b/apps/sim/lib/core/config/env-flags.ts @@ -92,6 +92,14 @@ export const isStatusNoticePreviewEnabled = isTruthy(getEnv('NEXT_PUBLIC_STATUS_ * used tools, so it is an opt-in change in how the product feels, not just a * safety toggle. With it off nothing is stamped, gated, or persisted, and an * approval stamp arriving from Go is cleared on the way to the client. + * + * The gate is a property of the lane, not only of the tool. Sim can hold a call + * for a decision on the dispatch lane, where a streaming context and a decision + * row exist. It cannot on the in-band route (`POST /api/copilot/tools/execute`), + * which the mothership drives for background lanes, so that route refuses a + * `requiresApproval` tool outright rather than running it ungated — see + * `toolRequiresApprovalLane`. Any new execution lane has to answer the same + * question before this flag is turned on. */ export const isCopilotToolPermissionsEnabled = isTruthy(env.COPILOT_TOOL_PERMISSIONS_ENABLED) From 2986e56365bcc6d1d6b1c3fb216aa8e9f7fb40cc Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 19 Sep 2026 17:20:30 -0700 Subject: [PATCH 2/2] improvement(copilot): move the approval-lane predicate beside the tool router MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing the dispatch gate module for a one-line predicate pulled the permission persistence layer in with it, whose module body opens a pub/sub channel — two Redis clients and a channel subscription — in every process that loads the in-band route. Move toolRequiresApprovalLane to tool-executor/router.ts, which imports only the catalog. The route already imported @/lib/copilot/tool-executor for ensureHandlersRegistered, so the guard now costs no new import edge at all. The dispatch gate keeps a pointer to it. Its flag-and-catalog behavior is covered in the router tests against the real flag and the real catalog; the route tests keep to what the route does with the answer. --- .../api/copilot/tools/execute/route.test.ts | 48 ++++++------------- .../app/api/copilot/tools/execute/route.ts | 3 +- .../copilot/request/tools/permission.test.ts | 36 +------------- .../lib/copilot/request/tools/permission.ts | 26 ++-------- apps/sim/lib/copilot/tool-executor/index.ts | 7 ++- .../lib/copilot/tool-executor/router.test.ts | 35 +++++++++++++- apps/sim/lib/copilot/tool-executor/router.ts | 25 ++++++++++ 7 files changed, 86 insertions(+), 94 deletions(-) diff --git a/apps/sim/app/api/copilot/tools/execute/route.test.ts b/apps/sim/app/api/copilot/tools/execute/route.test.ts index c8a314108dc..faacd5934fb 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.test.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.test.ts @@ -1,20 +1,19 @@ /** * @vitest-environment node */ -import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' const { mockCheckInternalApiKey, mockPrepareEnvironmentContext, mockHandler, - mockToolRequiresApproval, + mockToolRequiresApprovalLane, } = vi.hoisted(() => ({ mockCheckInternalApiKey: vi.fn(), mockPrepareEnvironmentContext: vi.fn(), mockHandler: vi.fn(), - mockToolRequiresApproval: vi.fn().mockReturnValue(false), + mockToolRequiresApprovalLane: vi.fn().mockReturnValue(false), })) vi.mock('@/lib/copilot/request/http', () => ({ @@ -27,7 +26,7 @@ vi.mock('@/lib/copilot/environment-context', () => ({ vi.mock('@/lib/copilot/tool-executor', () => ({ ensureHandlersRegistered: vi.fn(), - toolRequiresApproval: mockToolRequiresApproval, + toolRequiresApprovalLane: mockToolRequiresApprovalLane, })) vi.mock('@/lib/copilot/tool-executor/executor', () => ({ @@ -75,7 +74,7 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { beforeEach(() => { vi.clearAllMocks() mockCheckInternalApiKey.mockReturnValue({ success: true }) - mockToolRequiresApproval.mockReturnValue(false) + mockToolRequiresApprovalLane.mockReturnValue(false) // A fresh, complete registry per test: the module-level turn cache is keyed // by messageId, so each test uses a distinct messageId to avoid cross-test // cache hits. @@ -170,17 +169,20 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { }) }) + /** + * Whether a tool needs an approval-capable lane is decided by + * `toolRequiresApprovalLane` (covered against the real flag and catalog in + * the tool-executor router tests). What matters here is what the route does + * with that answer. + */ describe('approval-gated tools', () => { - afterEach(resetEnvFlagsMock) - /** * This lane cannot hold an approval prompt: the dispatch handler owns the gate and * declines to dispatch in-band calls, so a gated tool arriving here has no waiter behind * it. Refuse before running anything rather than execute on consent nobody gave. */ - it('refuses an approval-gated tool without executing it when permissions are enabled', async () => { - setEnvFlags({ isCopilotToolPermissionsEnabled: true }) - mockToolRequiresApproval.mockReturnValue(true) + it('refuses a tool that needs an approval-capable lane, without executing it', async () => { + mockToolRequiresApprovalLane.mockReturnValue(true) mockHandler.mockResolvedValue({ success: true, output: { ran: true } }) const res = await POST( @@ -200,8 +202,7 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { expect(body.error).toContain('checkpoint lane') }) - it('still runs a tool the catalog does not gate when permissions are enabled', async () => { - setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + it('still runs a tool that does not need an approval-capable lane', async () => { mockHandler.mockResolvedValue({ success: true, output: { content: 'hello' } }) const res = await POST(makeRequest({ ...BASE_BODY, messageId: 'msg-ungated' }) as never) @@ -209,27 +210,6 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { expect(mockHandler).toHaveBeenCalledTimes(1) await expect(res.json()).resolves.toEqual({ success: true, output: { content: 'hello' } }) }) - - /** - * The guard is inert while the feature is off, which is the state this ships in — enabling - * the flag is what makes it bite, so it cannot change in-band behavior today. - */ - it('runs an approval-gated tool unchanged while permissions are disabled', async () => { - mockToolRequiresApproval.mockReturnValue(true) - mockHandler.mockResolvedValue({ success: true, output: { ran: true } }) - - const res = await POST( - makeRequest({ - ...BASE_BODY, - toolName: 'run_function', - params: { code: 'return 1' }, - messageId: 'msg-gated-flag-off', - }) as never - ) - - expect(mockHandler).toHaveBeenCalledTimes(1) - await expect(res.json()).resolves.toEqual({ success: true, output: { ran: true } }) - }) }) it('passes a failed generate_api_key call through with its error', async () => { diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index f1f3d67bcc2..d68ac58ba59 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -10,7 +10,6 @@ import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1' import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1' import { checkInternalApiKey } from '@/lib/copilot/request/http' import { withIncomingGoSpan } from '@/lib/copilot/request/otel' -import { toolRequiresApprovalLane } from '@/lib/copilot/request/tools/permission' import { describeWithholdingCause, inspectToolResultForCopilot, @@ -18,7 +17,7 @@ import { } from '@/lib/copilot/request/tools/resolved-secret-result' import { handleResourceSideEffects } from '@/lib/copilot/request/tools/resources' import type { ToolCallResult } from '@/lib/copilot/request/types' -import { ensureHandlersRegistered } from '@/lib/copilot/tool-executor' +import { ensureHandlersRegistered, toolRequiresApprovalLane } from '@/lib/copilot/tool-executor' import { executeTool } from '@/lib/copilot/tool-executor/executor' import { TOOL_EFFECT_PHASE } from '@/lib/copilot/tool-executor/types' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' diff --git a/apps/sim/lib/copilot/request/tools/permission.test.ts b/apps/sim/lib/copilot/request/tools/permission.test.ts index 3bea2f1a097..07765132b18 100644 --- a/apps/sim/lib/copilot/request/tools/permission.test.ts +++ b/apps/sim/lib/copilot/request/tools/permission.test.ts @@ -2,8 +2,7 @@ * @vitest-environment node */ -import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { TraceCollector } from '@/lib/copilot/request/trace' const { toolRequiresApproval, waitForToolPermissionDecision } = vi.hoisted(() => ({ @@ -29,7 +28,6 @@ import { createStreamingContext } from '@/lib/copilot/request/context/request-co import { runGatedToolExecution, toolCallNeedsApproval, - toolRequiresApprovalLane, } from '@/lib/copilot/request/tools/permission' import type { StreamEvent, ToolCallState } from '@/lib/copilot/request/types' @@ -75,38 +73,6 @@ function gate( ) } -describe('toolRequiresApprovalLane', () => { - // vi.clearAllMocks() clears calls but not implementations, so a mockReturnValue - // set here would otherwise outlive this block and silently flip the suites below. - afterEach(() => { - resetEnvFlagsMock() - toolRequiresApproval.mockReturnValue(true) - }) - - /** - * Asked by lanes that cannot hold a prompt, so it answers from the catalog and the - * feature flag alone — there is no streaming context to consult, and the stored - * auto-allow list is deliberately not read (an auto-allowed tool is admitted on the - * checkpoint lane without prompting anyone). - */ - it('is false while the feature is off, whatever the catalog says', () => { - toolRequiresApproval.mockReturnValue(true) - expect(toolRequiresApprovalLane('run_function')).toBe(false) - }) - - it('is true for a catalog-gated tool once the feature is on', () => { - setEnvFlags({ isCopilotToolPermissionsEnabled: true }) - toolRequiresApproval.mockReturnValue(true) - expect(toolRequiresApprovalLane('run_function')).toBe(true) - }) - - it('is false for an ungated tool once the feature is on', () => { - setEnvFlags({ isCopilotToolPermissionsEnabled: true }) - toolRequiresApproval.mockReturnValue(false) - expect(toolRequiresApprovalLane('read')).toBe(false) - }) -}) - describe('toolCallNeedsApproval', () => { const runCall = { operation: 'run', args: { command: 'ls' } } diff --git a/apps/sim/lib/copilot/request/tools/permission.ts b/apps/sim/lib/copilot/request/tools/permission.ts index 7060319258f..aae9c714b5a 100644 --- a/apps/sim/lib/copilot/request/tools/permission.ts +++ b/apps/sim/lib/copilot/request/tools/permission.ts @@ -27,7 +27,6 @@ import type { ToolCallState, } from '@/lib/copilot/request/types' import { getToolEntry, toolRequiresApproval } from '@/lib/copilot/tool-executor' -import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags' const logger = createLogger('CopilotToolPermissionGate') @@ -52,31 +51,16 @@ const PERMISSION_WAIT_TIMEOUT_MS = ORCHESTRATION_TIMEOUT_MS export const TOOL_AWAITING_APPROVAL_STATUS = MothershipStreamV1ToolStatus.awaiting_approval -/** - * Whether a tool may only run on a lane that is able to hold an approval prompt. - * - * `toolCallNeedsApproval` answers for the dispatch lane, where a streaming - * context exists to gate against. The in-band route has neither a context nor a - * waiter — the mothership executes those calls itself — so it asks this instead, - * before running anything, and refuses rather than blocks: a background lane - * must never hang on a prompt with no row behind it. - * - * 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 - * route: an auto-allowed tool sent to the checkpoint lane is admitted there - * without prompting anyone. Refusing unconditionally keeps this fail-closed and - * leaves the one implementation of "has the user allowed this" on the lane that - * already owns it. - */ -export function toolRequiresApprovalLane(toolName: string): boolean { - return isCopilotToolPermissionsEnabled && toolRequiresApproval(toolName) -} - /** * Whether this call must be held for an explicit user decision. * * Headless one-shot executions are never gated: nobody is there to answer, and * blocking them would hang the run until the orchestration timeout. + * + * This is the dispatch lane's answer, and it needs a streaming context. A lane + * that has none — the in-band route — asks `toolRequiresApprovalLane` instead, + * which lives beside the tool router so a caller needing only the predicate does + * not pull this module's permission pub/sub in with it. */ export function toolCallNeedsApproval( toolName: string, diff --git a/apps/sim/lib/copilot/tool-executor/index.ts b/apps/sim/lib/copilot/tool-executor/index.ts index a287c8835af..aa4e27d50dc 100644 --- a/apps/sim/lib/copilot/tool-executor/index.ts +++ b/apps/sim/lib/copilot/tool-executor/index.ts @@ -1,3 +1,8 @@ export { executeTool } from './executor' export { ensureHandlersRegistered } from './register-handlers' -export { getToolEntry, isSimExecuted, toolRequiresApproval } from './router' +export { + getToolEntry, + isSimExecuted, + toolRequiresApproval, + toolRequiresApprovalLane, +} from './router' diff --git a/apps/sim/lib/copilot/tool-executor/router.test.ts b/apps/sim/lib/copilot/tool-executor/router.test.ts index a0a980fbad3..83c5bf14b6c 100644 --- a/apps/sim/lib/copilot/tool-executor/router.test.ts +++ b/apps/sim/lib/copilot/tool-executor/router.test.ts @@ -2,7 +2,8 @@ * @vitest-environment node */ -import { describe, expect, it, vi } from 'vitest' +import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock' +import { afterEach, describe, expect, it, vi } from 'vitest' /** * The handler map is a wiring table from tool id to implementation. Only its @@ -54,6 +55,7 @@ import { getToolEntry, isSimExecuted, toolRequiresApproval, + toolRequiresApprovalLane, } from '@/lib/copilot/tool-executor/router' import { executeCancelWorkflowRun } from '@/lib/copilot/tools/handlers/workflow/mutations' @@ -75,3 +77,34 @@ describe('workflow-run cancellation tool routing', () => { expect(buildHandlerMap().cancel_workflow_run).toBe(executeCancelWorkflowRun) }) }) + +describe('toolRequiresApprovalLane', () => { + afterEach(resetEnvFlagsMock) + + /** + * Asked by lanes that cannot hold a prompt, so it answers from the catalog and the + * feature flag alone: there is no streaming context to consult, and the stored + * auto-allow list is deliberately not read (an auto-allowed tool is admitted on the + * checkpoint lane without prompting anyone). + */ + it('is false while copilot tool permissions are off, whatever the catalog says', () => { + expect(toolRequiresApproval('run_function')).toBe(true) + expect(toolRequiresApprovalLane('run_function')).toBe(false) + }) + + it('is true for a catalog-gated tool once the feature is on', () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + expect(toolRequiresApprovalLane('run_function')).toBe(true) + }) + + it('is false for a tool the catalog does not gate, feature on', () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + expect(toolRequiresApproval('read')).toBe(false) + expect(toolRequiresApprovalLane('read')).toBe(false) + }) + + it('is false for a tool that is not in the catalog at all', () => { + setEnvFlags({ isCopilotToolPermissionsEnabled: true }) + expect(toolRequiresApprovalLane('not_a_real_tool')).toBe(false) + }) +}) diff --git a/apps/sim/lib/copilot/tool-executor/router.ts b/apps/sim/lib/copilot/tool-executor/router.ts index eee305eefee..257f4ab60a3 100644 --- a/apps/sim/lib/copilot/tool-executor/router.ts +++ b/apps/sim/lib/copilot/tool-executor/router.ts @@ -1,4 +1,5 @@ import { TOOL_CATALOG, type ToolCatalogEntry } from '@/lib/copilot/generated/tool-catalog-v1' +import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags' export function isToolInCatalog(toolId: string): boolean { return toolId in TOOL_CATALOG @@ -24,3 +25,27 @@ export function isKnownTool(toolId: string): boolean { export function toolRequiresApproval(toolId: string): boolean { return getToolEntry(toolId)?.requiresApproval === true } + +/** + * Whether a tool may only run on a lane that is able to hold an approval prompt. + * + * `toolCallNeedsApproval` answers for the dispatch lane, where a streaming + * context exists to gate against. The in-band route has neither a context nor a + * waiter — the mothership executes those calls itself — so it asks this instead, + * before running anything, and refuses rather than blocks: a background lane + * 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. + * + * 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 + * route: an auto-allowed tool sent to the checkpoint lane is admitted there + * without prompting anyone. Refusing unconditionally keeps this fail-closed and + * 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) +}