diff --git a/extensions/cli/src/stream/handleToolCalls.ts b/extensions/cli/src/stream/handleToolCalls.ts index c58206b56f8..0a160f0f5a6 100644 --- a/extensions/cli/src/stream/handleToolCalls.ts +++ b/extensions/cli/src/stream/handleToolCalls.ts @@ -3,6 +3,7 @@ import { stripImages } from "core/util/messageContent.js"; import { createHistoryItem } from "core/util/messageConversion.js"; import { checkToolPermission } from "src/permissions/permissionChecker.js"; +import type { ToolPermissions } from "src/permissions/types.js"; import { SERVICE_NAMES, @@ -169,7 +170,10 @@ export async function handleToolCalls( return false; } -export async function getRequestTools(isHeadless: boolean) { +export async function getRequestTools( + isHeadless: boolean, + permissionSnapshot?: Readonly, +) { const availableTools = await getAllAvailableTools(isHeadless); const permissionsState = @@ -181,7 +185,7 @@ export async function getRequestTools(isHeadless: boolean) { for (const tool of availableTools) { const result = checkToolPermission( { name: tool.name, arguments: {} }, - permissionsState.permissions, + permissionSnapshot ?? permissionsState.permissions, ); if ( diff --git a/extensions/cli/src/stream/streamChatResponse.helpers.ts b/extensions/cli/src/stream/streamChatResponse.helpers.ts index 776af67f554..d436b4c3ed2 100644 --- a/extensions/cli/src/stream/streamChatResponse.helpers.ts +++ b/extensions/cli/src/stream/streamChatResponse.helpers.ts @@ -125,6 +125,10 @@ export async function checkToolPermissionApproval( // "ask" tools are excluded in headless so can only get here by policy evaluation return { approved: false, denialReason: "policy" }; } + // Fail closed: with no way to ask the user, "ask" is a denial + if (!callbacks?.onToolPermissionRequest) { + return { approved: false, denialReason: "policy" }; + } const userApproved = await requestUserPermission(toolCall, callbacks); return userApproved ? { approved: true } @@ -510,8 +514,9 @@ export async function executeStreamedToolCalls( await serviceContainer.get( SERVICE_NAMES.TOOL_PERMISSIONS, ); + // A run with its own snapshot (subagents) ignores the live state const permissionResult = await checkToolPermissionApproval( - permissionState.permissions, + callbacks?.permissionSnapshot ?? permissionState.permissions, call, callbacks, isHeadless, @@ -566,6 +571,8 @@ export async function executeStreamedToolCalls( const toolResult = await executeToolCall(call, { parallelToolCallCount, + onToolPermissionRequest: callbacks?.onToolPermissionRequest, + permissionSnapshot: callbacks?.permissionSnapshot, }); const entry: ToolResultWithStatus = { role: "tool", diff --git a/extensions/cli/src/stream/streamChatResponse.permissions.test.ts b/extensions/cli/src/stream/streamChatResponse.permissions.test.ts new file mode 100644 index 00000000000..5450e466146 --- /dev/null +++ b/extensions/cli/src/stream/streamChatResponse.permissions.test.ts @@ -0,0 +1,103 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { serviceContainer } from "../services/index.js"; + +import { + checkToolPermissionApproval, + executeStreamedToolCalls, +} from "./streamChatResponse.helpers.js"; + +vi.mock("../services/index.js", () => ({ + services: { + chatHistory: { + addToolResult: vi.fn(), + updateToolStatus: vi.fn(), + }, + }, + serviceContainer: { get: vi.fn() }, + SERVICE_NAMES: { TOOL_PERMISSIONS: "toolPermissions" }, +})); + +vi.mock("../tools/index.js", async (importOriginal) => ({ + ...(await importOriginal()), + executeToolCall: vi.fn().mockResolvedValue("ran"), +})); + +const askBash = { policies: [{ tool: "Bash", permission: "ask" as const }] }; +const allowAll = { policies: [{ tool: "*", permission: "allow" as const }] }; + +function bashCall() { + return { + id: "call-1", + name: "Bash", + arguments: { command: "echo hi" }, + argumentsStr: '{"command":"echo hi"}', + tool: { name: "Bash", run: vi.fn() }, + } as any; +} + +describe("tool permission fail-closed behavior", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("denies an ask-policy tool when no approval callback is registered", async () => { + const result = await checkToolPermissionApproval( + askBash, + bashCall(), + undefined, + false, + ); + + expect(result).toEqual({ approved: false, denialReason: "policy" }); + }); + + it("denies an ask-policy tool when callbacks exist without onToolPermissionRequest", async () => { + const result = await checkToolPermissionApproval( + askBash, + bashCall(), + { onContent: vi.fn() }, + false, + ); + + expect(result).toEqual({ approved: false, denialReason: "policy" }); + }); + + it("does not run an ask-policy Bash call in a headless-style context with no callback", async () => { + // Live state is permissive; the run's own snapshot says ask. + vi.mocked(serviceContainer.get).mockResolvedValue({ + permissions: allowAll, + } as any); + const { executeToolCall } = await import("../tools/index.js"); + + const { hasRejection, chatHistoryEntries } = await executeStreamedToolCalls( + [bashCall()], + { permissionSnapshot: askBash }, + false, + ); + + expect(hasRejection).toBe(true); + expect(chatHistoryEntries[0].status).toBe("canceled"); + expect(executeToolCall).not.toHaveBeenCalled(); + }); + + it("checks tool calls against the run's snapshot, not the live state", async () => { + vi.mocked(serviceContainer.get).mockResolvedValue({ + permissions: allowAll, + } as any); + const { executeToolCall } = await import("../tools/index.js"); + + // Without a snapshot the live allow-all policy applies + await executeStreamedToolCalls([bashCall()], {}, false); + expect(executeToolCall).toHaveBeenCalledTimes(1); + + // With a snapshot the ask policy applies and the call is not run + vi.mocked(executeToolCall).mockClear(); + await executeStreamedToolCalls( + [bashCall()], + { permissionSnapshot: askBash }, + false, + ); + expect(executeToolCall).not.toHaveBeenCalled(); + }); +}); diff --git a/extensions/cli/src/stream/streamChatResponse.ts b/extensions/cli/src/stream/streamChatResponse.ts index 20156663381..bd662b54b92 100644 --- a/extensions/cli/src/stream/streamChatResponse.ts +++ b/extensions/cli/src/stream/streamChatResponse.ts @@ -451,7 +451,10 @@ export async function streamChatResponse( ); // Recompute tools on each iteration to handle mode changes during streaming - const rawTools = await getRequestTools(isHeadless); + const rawTools = await getRequestTools( + isHeadless, + callbacks?.permissionSnapshot, + ); const tools = applyChatCompletionToolOverrides( rawTools, model.chatOptions?.toolOverrides, diff --git a/extensions/cli/src/stream/streamChatResponse.types.ts b/extensions/cli/src/stream/streamChatResponse.types.ts index 405505eee69..ba8edc82b31 100644 --- a/extensions/cli/src/stream/streamChatResponse.types.ts +++ b/extensions/cli/src/stream/streamChatResponse.types.ts @@ -2,6 +2,7 @@ import { CompletionOptions } from "@continuedev/config-yaml"; import type { ToolStatus } from "core/index.js"; import type { ChatCompletionCreateParamsStreaming } from "openai/resources.mjs"; +import type { ToolPermissions } from "../permissions/types.js"; import { ToolCallPreview } from "../tools/types.js"; export interface StreamCallbacks { @@ -17,6 +18,12 @@ export interface StreamCallbacks { preview?: ToolCallPreview[], ) => void; onSystemMessage?: (message: string) => void; + /** + * Immutable permission policy for this stream. When set, tool calls are + * checked against it instead of the live session state. Subagent runs set + * this so a run cannot be affected by later changes to the shared state. + */ + permissionSnapshot?: Readonly; } export function getDefaultCompletionOptions( diff --git a/extensions/cli/src/subagent/executor.test.ts b/extensions/cli/src/subagent/executor.test.ts new file mode 100644 index 00000000000..8f19059dc36 --- /dev/null +++ b/extensions/cli/src/subagent/executor.test.ts @@ -0,0 +1,95 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { serviceContainer } from "../services/ServiceContainer.js"; +import { SERVICE_NAMES } from "../services/types.js"; +import { streamChatResponse } from "../stream/streamChatResponse.js"; + +import { executeSubAgent } from "./executor.js"; + +vi.mock("../stream/streamChatResponse.js", () => ({ + streamChatResponse: vi.fn(), +})); + +vi.mock("../services/index.js", () => ({ + services: {}, +})); + +describe("executeSubAgent", () => { + const agent = { + model: { name: "test-model" }, + llmApi: {}, + } as any; + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(streamChatResponse).mockResolvedValue(undefined as any); + }); + + it("keeps the parent session's tool permissions during and after execution", async () => { + const parentPermissionsState = { + permissions: { + policies: [{ tool: "Bash", permission: "ask" as const }], + }, + currentMode: "normal" as const, + isHeadless: false, + }; + serviceContainer.registerValue( + SERVICE_NAMES.TOOL_PERMISSIONS, + parentPermissionsState, + ); + + let stateDuringExecution: unknown; + vi.mocked(streamChatResponse).mockImplementation(async () => { + stateDuringExecution = await serviceContainer.get( + SERVICE_NAMES.TOOL_PERMISSIONS, + ); + return ""; + }); + + const result = await executeSubAgent({ + agent, + prompt: "do a thing", + parentSessionId: "parent-session", + abortController: new AbortController(), + }); + + expect(result.success).toBe(true); + expect(stateDuringExecution).toEqual(parentPermissionsState); + const stateAfter = await serviceContainer.get( + SERVICE_NAMES.TOOL_PERMISSIONS, + ); + expect(stateAfter).toEqual(parentPermissionsState); + }); + + it("forwards onToolPermissionRequest to the subagent stream callbacks", async () => { + const onToolPermissionRequest = vi.fn(); + + await executeSubAgent({ + agent, + prompt: "do a thing", + parentSessionId: "parent-session", + abortController: new AbortController(), + onToolPermissionRequest, + }); + + const callbacks = vi.mocked(streamChatResponse).mock.calls[0][4]; + expect(callbacks?.onToolPermissionRequest).toBe(onToolPermissionRequest); + }); + + it("passes the permission snapshot to the subagent stream callbacks", async () => { + const permissionSnapshot = Object.freeze({ + policies: [{ tool: "Bash", permission: "ask" as const }], + }); + + await executeSubAgent({ + agent, + prompt: "do a thing", + parentSessionId: "parent-session", + abortController: new AbortController(), + permissionSnapshot, + }); + + const callbacks = vi.mocked(streamChatResponse).mock.calls[0][4]; + expect(callbacks?.permissionSnapshot).toBe(permissionSnapshot); + }); +}); diff --git a/extensions/cli/src/subagent/executor.ts b/extensions/cli/src/subagent/executor.ts index 1580e7f7f9d..50247b0238b 100644 --- a/extensions/cli/src/subagent/executor.ts +++ b/extensions/cli/src/subagent/executor.ts @@ -1,10 +1,10 @@ import type { ChatHistoryItem } from "core"; +import type { ToolPermissions } from "../permissions/types.js"; import { services } from "../services/index.js"; -import { serviceContainer } from "../services/ServiceContainer.js"; -import type { ToolPermissionServiceState } from "../services/ToolPermissionService.js"; -import { ModelServiceState, SERVICE_NAMES } from "../services/types.js"; +import { ModelServiceState } from "../services/types.js"; import { streamChatResponse } from "../stream/streamChatResponse.js"; +import type { ToolCallPreview } from "../tools/types.js"; import { escapeEvents } from "../util/cli.js"; import { logger } from "../util/logger.js"; @@ -17,6 +17,17 @@ export interface SubAgentExecutionOptions { parentSessionId: string; abortController: AbortController; onOutputUpdate?: (output: string) => void; + onToolPermissionRequest?: ( + toolName: string, + toolArgs: any, + requestId: string, + preview?: ToolCallPreview[], + ) => void; + /** + * Immutable policy the subagent runs under. Computed when the subagent is + * spawned, so later changes to the shared session state do not affect it. + */ + permissionSnapshot?: Readonly; } /** @@ -54,16 +65,17 @@ async function buildAgentSystemMessage( /** * Execute a subagent in a child session */ -// eslint-disable-next-line complexity export async function executeSubAgent( options: SubAgentExecutionOptions, ): Promise { - const { agent: subAgent, prompt, abortController, onOutputUpdate } = options; - - const mainAgentPermissionsState = - await serviceContainer.get( - SERVICE_NAMES.TOOL_PERMISSIONS, - ); + const { + agent: subAgent, + prompt, + abortController, + onOutputUpdate, + onToolPermissionRequest, + permissionSnapshot, + } = options; try { logger.debug("Starting subagent execution", { @@ -75,18 +87,9 @@ export async function executeSubAgent( throw new Error("Model or LLM API not available"); } - // allow all tools for now - // todo: eventually we want to show the same prompt in a dialog whether asking whether that tool call is allowed or not - - serviceContainer.set( - SERVICE_NAMES.TOOL_PERMISSIONS, - { - ...mainAgentPermissionsState, - permissions: { - policies: [{ tool: "*", permission: "allow" }], - }, - }, - ); + // The subagent runs under the permission snapshot taken at spawn time. + // "ask"-policy tool calls surface the approval dialog through + // onToolPermissionRequest; with no callback they are denied. // Build agent system message const systemMessage = await buildAgentSystemMessage(subAgent, services); @@ -157,6 +160,8 @@ export async function executeSubAgent( onOutputUpdate(accumulatedOutput); } }, + onToolPermissionRequest, + permissionSnapshot, }, false, // Not compacting ); @@ -191,12 +196,6 @@ export async function executeSubAgent( if (chatHistorySvc && originalIsReady) { chatHistorySvc.isReady = originalIsReady; } - - // Restore original main agent tool permissions - serviceContainer.set( - SERVICE_NAMES.TOOL_PERMISSIONS, - mainAgentPermissionsState, - ); } } catch (error: any) { logger.error("Subagent execution failed", { diff --git a/extensions/cli/src/tools/index.tsx b/extensions/cli/src/tools/index.tsx index ff87aae193f..b91602426c9 100644 --- a/extensions/cli/src/tools/index.tsx +++ b/extensions/cli/src/tools/index.tsx @@ -213,7 +213,11 @@ export function convertMcpToolToContinueTool(mcpTool: MCPTool): Tool { export async function executeToolCall( toolCall: PreprocessedToolCall, - options: { parallelToolCallCount: number } = { parallelToolCallCount: 1 }, + options: { + parallelToolCallCount: number; + onToolPermissionRequest?: ToolRunContext["onToolPermissionRequest"]; + permissionSnapshot?: ToolRunContext["permissionSnapshot"]; + } = { parallelToolCallCount: 1 }, ): Promise { const startTime = Date.now(); @@ -230,6 +234,8 @@ export async function executeToolCall( const context: ToolRunContext = { toolCallId: toolCall.id, parallelToolCallCount: options.parallelToolCallCount, + onToolPermissionRequest: options.onToolPermissionRequest, + permissionSnapshot: options.permissionSnapshot, }; // IMPORTANT: if preprocessed args are present, uses preprocessed args instead of original args diff --git a/extensions/cli/src/tools/subagent.test.ts b/extensions/cli/src/tools/subagent.test.ts index 3af2ff4bfe8..6a52af9b73e 100644 --- a/extensions/cli/src/tools/subagent.test.ts +++ b/extensions/cli/src/tools/subagent.test.ts @@ -118,4 +118,104 @@ describe("subagentTool", () => { "subagent-output\n\nstatus: completed\n", ); }); + + it("run forwards the permission request callback to the executor", async () => { + vi.mocked(getAgentNames).mockReturnValue(["code-agent"]); + vi.mocked(getSubagent).mockReturnValue({ + model: { name: "test-model" }, + } as any); + vi.mocked(executeSubAgent).mockResolvedValue({ + success: true, + response: "subagent-output", + } as any); + + const tool = await subagentTool(); + const onToolPermissionRequest = vi.fn(); + + await tool.run( + { + prompt: "Subagent prompt", + subagent_name: "code-agent", + }, + { + toolCallId: "tool-call-id", + parallelToolCallCount: 1, + onToolPermissionRequest, + }, + ); + + const [options] = vi.mocked(executeSubAgent).mock.calls[0]; + expect(options.onToolPermissionRequest).toBe(onToolPermissionRequest); + }); + + describe("permission snapshot", () => { + const askBash = { + policies: [{ tool: "Bash", permission: "ask" as const }], + }; + const allowAll = { + policies: [{ tool: "*", permission: "allow" as const }], + }; + + function mockLivePermissions(permissions: unknown) { + vi.mocked(serviceContainer.get).mockImplementation(async (name: any) => + name === "toolPermissions" + ? ({ permissions } as any) + : modelServiceState, + ); + } + + beforeEach(() => { + vi.mocked(getAgentNames).mockReturnValue(["code-agent"]); + vi.mocked(getSubagent).mockReturnValue({ + model: { name: "test-model" }, + } as any); + vi.mocked(executeSubAgent).mockResolvedValue({ + success: true, + response: "ok", + } as any); + }); + + it("gives the subagent a frozen copy of the policy taken at spawn", async () => { + const live = structuredClone(askBash); + mockLivePermissions(live); + const tool = await subagentTool(); + + await tool.run( + { prompt: "p", subagent_name: "code-agent" }, + { toolCallId: "id", parallelToolCallCount: 1 }, + ); + + const [options] = vi.mocked(executeSubAgent).mock.calls[0]; + const snapshot = options.permissionSnapshot!; + expect(snapshot).toEqual(askBash); + expect(snapshot).not.toBe(live); + expect(Object.isFrozen(snapshot)).toBe(true); + expect(Object.isFrozen(snapshot.policies[0])).toBe(true); + + // Later changes to the shared state do not reach the snapshot + live.policies = [{ tool: "*", permission: "allow" }] as any; + expect(snapshot).toEqual(askBash); + }); + + it("keeps the original policy for a nested spawn", async () => { + // The live state now allows everything, but the spawning run was + // started under the ask policy. + mockLivePermissions(allowAll); + const tool = await subagentTool(); + const parentSnapshot = Object.freeze(structuredClone(askBash)); + + await tool.run( + { prompt: "grandchild task", subagent_name: "code-agent" }, + { + toolCallId: "id", + parallelToolCallCount: 1, + permissionSnapshot: parentSnapshot, + }, + ); + + const [options] = vi.mocked(executeSubAgent).mock.calls[0]; + expect(options.permissionSnapshot).toBe(parentSnapshot); + expect(options.permissionSnapshot).toEqual(askBash); + }); + }); }); diff --git a/extensions/cli/src/tools/subagent.ts b/extensions/cli/src/tools/subagent.ts index 43d2d29eb4f..08a630d9316 100644 --- a/extensions/cli/src/tools/subagent.ts +++ b/extensions/cli/src/tools/subagent.ts @@ -1,5 +1,7 @@ +import type { ToolPermissions } from "../permissions/types.js"; import { services } from "../services/index.js"; import { serviceContainer } from "../services/ServiceContainer.js"; +import type { ToolPermissionServiceState } from "../services/ToolPermissionService.js"; import { ModelServiceState, SERVICE_NAMES } from "../services/types.js"; import { executeSubAgent } from "../subagent/executor.js"; import { @@ -10,7 +12,33 @@ import { import { SUBAGENT_TOOL_META } from "../subagent/index.js"; import { logger } from "../util/logger.js"; -import { Tool } from "./types.js"; +import { Tool, ToolRunContext } from "./types.js"; + +function deepFreeze(value: T): Readonly { + if (value && typeof value === "object" && !Object.isFrozen(value)) { + Object.freeze(value); + for (const key of Object.keys(value as object)) { + deepFreeze((value as Record)[key]); + } + } + return value; +} + +/** + * The policy a subagent runs under. A nested spawn reuses the snapshot of the + * run that spawned it; a top-level spawn copies the live session policy once. + */ +async function resolvePermissionSnapshot( + context?: ToolRunContext, +): Promise> { + if (context?.permissionSnapshot) { + return context.permissionSnapshot; + } + const state = await serviceContainer.get( + SERVICE_NAMES.TOOL_PERMISSIONS, + ); + return deepFreeze(structuredClone(state.permissions)); +} export const subagentTool = async (): Promise => { const modelServiceState = await serviceContainer.get( @@ -60,7 +88,7 @@ export const subagentTool = async (): Promise => { }; }, - run: async (args: any, context?: { toolCallId: string }) => { + run: async (args: any, context?: ToolRunContext) => { const { prompt, subagent_name } = args; logger.debug("subagent args", { args, context }); @@ -77,12 +105,16 @@ export const subagentTool = async (): Promise => { throw new Error("No active session found"); } + const permissionSnapshot = await resolvePermissionSnapshot(context); + // Execute subagent with output streaming const result = await executeSubAgent({ agent, prompt, parentSessionId, abortController: new AbortController(), + onToolPermissionRequest: context?.onToolPermissionRequest, + permissionSnapshot, onOutputUpdate: context?.toolCallId ? (output: string) => { try { diff --git a/extensions/cli/src/tools/types.ts b/extensions/cli/src/tools/types.ts index 612bd22ff84..421ca21939a 100644 --- a/extensions/cli/src/tools/types.ts +++ b/extensions/cli/src/tools/types.ts @@ -1,5 +1,7 @@ import type { ToolPolicy } from "@continuedev/terminal-security"; +import type { ToolPermissions } from "../permissions/types.js"; + // JSON Schema compatible parameter definition export interface ParameterSchema { type: string; @@ -39,6 +41,23 @@ export interface ToolRunContext { * Tools should divide their output limits by this number to avoid context overflow. */ parallelToolCallCount: number; + /** + * Surfaces a tool permission prompt to the user. Forwarded to nested + * execution contexts (e.g. subagents) so their "ask"-policy tool calls + * show the same approval dialog as top-level tool calls. + */ + onToolPermissionRequest?: ( + toolName: string, + toolArgs: any, + requestId: string, + preview?: ToolCallPreview[], + ) => void; + /** + * Immutable permission policy the current run is checked against. A subagent + * spawned from this run inherits it, so nested spawns stay under the + * original policy. + */ + permissionSnapshot?: Readonly; } export interface Tool {