From 22086b5b866646a75b0562222b6969b3472a98ce Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 05:29:54 +0000 Subject: [PATCH 1/6] feat: add model-driven permission control for subtasks (Phase 3b) Adds an optional `permissions` parameter to the `new_task` tool, allowing the Orchestrator (or any parent task) to dynamically set permission boundaries for subtasks: - New `TaskPermissions` type with filePatterns, commandPatterns, allowedTools, and deniedTools - Permission merging with most-restrictive-wins semantics for nested subtask delegation - Runtime enforcement in validateToolUse() for all permission types - Full test coverage for merging logic and enforcement Addresses Issue #12330 (Phase 3b) --- .../src/__tests__/task-permissions.spec.ts | 152 ++++++++++++ packages/types/src/index.ts | 1 + packages/types/src/task-permissions.ts | 124 ++++++++++ packages/types/src/task.ts | 4 + .../assistant-message/NativeToolCallParser.ts | 4 + .../presentAssistantMessage.ts | 1 + .../prompts/tools/native-tools/new_task.ts | 6 + src/core/task/Task.ts | 7 + src/core/tools/NewTaskTool.ts | 33 ++- .../taskPermissionsEnforcement.spec.ts | 219 ++++++++++++++++++ src/core/tools/validateToolUse.ts | 83 ++++++- src/core/webview/ClineProvider.ts | 6 +- src/shared/tools.ts | 5 +- 13 files changed, 638 insertions(+), 7 deletions(-) create mode 100644 packages/types/src/__tests__/task-permissions.spec.ts create mode 100644 packages/types/src/task-permissions.ts create mode 100644 src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts new file mode 100644 index 00000000000..101c487d5a4 --- /dev/null +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -0,0 +1,152 @@ +import { describe, it, expect } from "vitest" +import { mergeTaskPermissions, matchesAnyPattern, taskPermissionsSchema } from "../task-permissions.js" +import type { TaskPermissions } from "../task-permissions.js" + +describe("TaskPermissions", () => { + describe("taskPermissionsSchema", () => { + it("validates a valid permissions object", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["src/components/.*"], + commandPatterns: ["npm test.*"], + allowedTools: ["read_file", "write_to_file"], + deniedTools: ["execute_command"], + }) + expect(result.success).toBe(true) + }) + + it("validates an empty object", () => { + const result = taskPermissionsSchema.safeParse({}) + expect(result.success).toBe(true) + }) + + it("validates partial permissions", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["src/.*"], + }) + expect(result.success).toBe(true) + }) + + it("rejects non-string array values", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: [123], + }) + expect(result.success).toBe(false) + }) + }) + + describe("mergeTaskPermissions", () => { + it("returns undefined when both are undefined", () => { + expect(mergeTaskPermissions(undefined, undefined)).toBeUndefined() + }) + + it("returns child when parent is undefined", () => { + const child: TaskPermissions = { filePatterns: ["src/.*"] } + expect(mergeTaskPermissions(undefined, child)).toEqual(child) + }) + + it("returns parent when child is undefined", () => { + const parent: TaskPermissions = { filePatterns: ["src/.*"] } + expect(mergeTaskPermissions(parent, undefined)).toEqual(parent) + }) + + it("intersects filePatterns when both defined", () => { + const parent: TaskPermissions = { filePatterns: ["src/.*", "tests/.*"] } + const child: TaskPermissions = { filePatterns: ["src/.*", "docs/.*"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.filePatterns).toEqual(["src/.*"]) + }) + + it("intersects commandPatterns when both defined", () => { + const parent: TaskPermissions = { commandPatterns: ["npm test.*", "npm run lint"] } + const child: TaskPermissions = { commandPatterns: ["npm test.*", "npm run build"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.commandPatterns).toEqual(["npm test.*"]) + }) + + it("intersects allowedTools when both defined", () => { + const parent: TaskPermissions = { allowedTools: ["read_file", "write_to_file", "search_files"] } + const child: TaskPermissions = { allowedTools: ["read_file", "execute_command"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.allowedTools).toEqual(["read_file"]) + }) + + it("unions deniedTools when both defined", () => { + const parent: TaskPermissions = { deniedTools: ["execute_command"] } + const child: TaskPermissions = { deniedTools: ["write_to_file"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.deniedTools).toEqual(["execute_command", "write_to_file"]) + }) + + it("deduplicates deniedTools in union", () => { + const parent: TaskPermissions = { deniedTools: ["execute_command", "write_to_file"] } + const child: TaskPermissions = { deniedTools: ["execute_command", "search_files"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.deniedTools).toEqual(["execute_command", "write_to_file", "search_files"]) + }) + + it("uses parent filePatterns when child has none", () => { + const parent: TaskPermissions = { filePatterns: ["src/.*"] } + const child: TaskPermissions = { deniedTools: ["execute_command"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.filePatterns).toEqual(["src/.*"]) + expect(merged?.deniedTools).toEqual(["execute_command"]) + }) + + it("returns empty array when intersection is empty", () => { + const parent: TaskPermissions = { allowedTools: ["read_file"] } + const child: TaskPermissions = { allowedTools: ["write_to_file"] } + const merged = mergeTaskPermissions(parent, child) + expect(merged?.allowedTools).toEqual([]) + }) + + it("handles complex nested merge scenario with exact string matching", () => { + const grandparent: TaskPermissions = { + filePatterns: ["src/.*"], + commandPatterns: ["npm.*"], + allowedTools: ["read_file", "write_to_file", "search_files"], + deniedTools: ["execute_command"], + } + const parent: TaskPermissions = { + filePatterns: ["src/components/.*"], + allowedTools: ["read_file", "write_to_file"], + } + + // Intersection uses exact string matching, so "src/components/.*" (child) + // is not equal to "src/.*" (parent) -- intersection is empty + const merged1 = mergeTaskPermissions(grandparent, parent) + expect(merged1?.filePatterns).toEqual([]) + // allowedTools intersection: read_file and write_to_file are in both + expect(merged1?.allowedTools).toEqual(["read_file", "write_to_file"]) + // commandPatterns: only grandparent has them, so they pass through + expect(merged1?.commandPatterns).toEqual(["npm.*"]) + // deniedTools: only grandparent has them, so they pass through + expect(merged1?.deniedTools).toEqual(["execute_command"]) + }) + }) + + describe("matchesAnyPattern", () => { + it("matches a simple regex pattern", () => { + expect(matchesAnyPattern("src/components/Button.tsx", ["src/components/.*"])).toBe(true) + }) + + it("does not match when no patterns match", () => { + expect(matchesAnyPattern("tests/unit/test.ts", ["src/components/.*"])).toBe(false) + }) + + it("matches when at least one pattern matches", () => { + expect(matchesAnyPattern("tests/unit/test.ts", ["src/.*", "tests/.*"])).toBe(true) + }) + + it("handles invalid regex gracefully", () => { + expect(matchesAnyPattern("test.ts", ["[invalid"])).toBe(false) + }) + + it("matches command patterns", () => { + expect(matchesAnyPattern("npm test -- --coverage", ["npm test.*"])).toBe(true) + }) + + it("does not match restricted commands", () => { + expect(matchesAnyPattern("rm -rf /", ["npm.*", "yarn.*"])).toBe(false) + }) + }) +}) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 64e2a3ded79..65fa66b08dc 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -20,6 +20,7 @@ export * from "./mode.js" export * from "./model.js" export * from "./provider-settings.js" export * from "./task.js" +export * from "./task-permissions.js" export * from "./todo.js" export * from "./skills.js" export * from "./terminal.js" diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts new file mode 100644 index 00000000000..96698fe8bf4 --- /dev/null +++ b/packages/types/src/task-permissions.ts @@ -0,0 +1,124 @@ +import { z } from "zod" + +/** + * TaskPermissions defines permission boundaries that a parent task can impose + * on a subtask created via the `new_task` tool. + * + * When nested subtasks are created, permissions are merged using + * "most-restrictive-wins" semantics: a child can never grant itself + * more access than its parent. + */ + +export const taskPermissionsSchema = z.object({ + /** + * Regex patterns for allowed file paths. + * When set, file operations (read/write) are restricted to paths matching + * at least one of these patterns. + */ + filePatterns: z.array(z.string()).optional(), + + /** + * Regex patterns for allowed shell commands. + * When set, command execution is restricted to commands matching + * at least one of these patterns. + */ + commandPatterns: z.array(z.string()).optional(), + + /** + * Explicit tool allowlist. When set, only these tools may be used + * by the subtask (in addition to always-available tools like + * attempt_completion and ask_followup_question). + */ + allowedTools: z.array(z.string()).optional(), + + /** + * Explicit tool blocklist. These tools are denied regardless of + * mode configuration. + */ + deniedTools: z.array(z.string()).optional(), +}) + +export type TaskPermissions = z.infer + +/** + * Merge two TaskPermissions using most-restrictive-wins semantics. + * + * - filePatterns / commandPatterns: if both define patterns, keep only patterns + * present in both (intersection). If only one side defines patterns, use that. + * - allowedTools: intersection of both lists (if both defined). + * - deniedTools: union of both lists (most restrictive). + * + * @returns merged permissions, or undefined if both inputs are undefined. + */ +export function mergeTaskPermissions( + parent: TaskPermissions | undefined, + child: TaskPermissions | undefined, +): TaskPermissions | undefined { + if (!parent && !child) { + return undefined + } + if (!parent) { + return child + } + if (!child) { + return parent + } + + return { + filePatterns: intersectOptionalArrays(parent.filePatterns, child.filePatterns), + commandPatterns: intersectOptionalArrays(parent.commandPatterns, child.commandPatterns), + allowedTools: intersectOptionalArrays(parent.allowedTools, child.allowedTools), + deniedTools: unionOptionalArrays(parent.deniedTools, child.deniedTools), + } +} + +/** + * Check if a value matches at least one pattern in a list of regex patterns. + */ +export function matchesAnyPattern(value: string, patterns: string[]): boolean { + return patterns.some((pattern) => { + try { + return new RegExp(pattern).test(value) + } catch { + // Invalid regex -- treat as non-match + return false + } + }) +} + +/** + * Intersect two optional arrays. If both are defined, return elements present + * in both. If only one is defined, return that one. If neither, return undefined. + */ +function intersectOptionalArrays(a: string[] | undefined, b: string[] | undefined): string[] | undefined { + if (!a && !b) { + return undefined + } + if (!a) { + return b + } + if (!b) { + return a + } + + const setB = new Set(b) + const result = a.filter((item) => setB.has(item)) + return result.length > 0 ? result : [] +} + +/** + * Union two optional arrays, deduplicating entries. + */ +function unionOptionalArrays(a: string[] | undefined, b: string[] | undefined): string[] | undefined { + if (!a && !b) { + return undefined + } + if (!a) { + return b + } + if (!b) { + return a + } + + return [...new Set([...a, ...b])] +} diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index 7447dc772e7..5c447ddbf2f 100644 --- a/packages/types/src/task.ts +++ b/packages/types/src/task.ts @@ -4,6 +4,7 @@ import { RooCodeEventName } from "./events.js" import type { RooCodeSettings } from "./global-settings.js" import type { ClineMessage, QueuedMessage, TokenUsage } from "./message.js" import type { ToolUsage, ToolName } from "./tool.js" +import type { TaskPermissions } from "./task-permissions.js" import type { TodoItem } from "./todo.js" /** @@ -94,6 +95,9 @@ export interface CreateTaskOptions { /** Whether to start the task loop immediately (default: true). * When false, the caller must invoke `task.start()` manually. */ startTask?: boolean + /** Permission boundaries for the task, set by the parent via new_task tool. + * When set, restricts what file paths, commands, and tools the task may use. */ + taskPermissions?: TaskPermissions } export enum TaskStatus { diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index c391f9852cf..0b1711c5483 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -551,6 +551,7 @@ export class NativeToolCallParser { if (partialArgs.todos !== undefined) { nativeArgs = { todos: partialArgs.todos, + permissions: partialArgs.permissions, } } break @@ -633,6 +634,7 @@ export class NativeToolCallParser { mode: partialArgs.mode, message: partialArgs.message, todos: partialArgs.todos, + permissions: partialArgs.permissions, } } break @@ -887,6 +889,7 @@ export class NativeToolCallParser { if (args.todos !== undefined) { nativeArgs = { todos: args.todos, + permissions: args.permissions, } as NativeArgsFor } break @@ -982,6 +985,7 @@ export class NativeToolCallParser { mode: args.mode, message: args.message, todos: args.todos, + permissions: args.permissions, } as NativeArgsFor } break diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 49ce56a305d..dc0c98c7d11 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -589,6 +589,7 @@ export async function presentAssistantMessage(cline: Task) { block.params, stateExperiments, includedTools, + cline.taskPermissions, ) } catch (error) { cline.consecutiveMistakeCount++ diff --git a/src/core/prompts/tools/native-tools/new_task.ts b/src/core/prompts/tools/native-tools/new_task.ts index f8e29e549d9..c8e778f1122 100644 --- a/src/core/prompts/tools/native-tools/new_task.ts +++ b/src/core/prompts/tools/native-tools/new_task.ts @@ -10,6 +10,8 @@ const MESSAGE_PARAMETER_DESCRIPTION = `Initial user instructions or context for const TODOS_PARAMETER_DESCRIPTION = `Optional initial todo list written as a markdown checklist; required when the workspace mandates todos` +const PERMISSIONS_PARAMETER_DESCRIPTION = `Optional JSON object defining permission boundaries for the subtask. Allows the parent to restrict the subtask's access. Supports: filePatterns (array of regex patterns for allowed file paths), commandPatterns (array of regex patterns for allowed commands), allowedTools (array of tool names the subtask may use), deniedTools (array of tool names the subtask may NOT use). Example: {"filePatterns":["src/components/.*"],"commandPatterns":["npm test.*"],"deniedTools":["execute_command"]}` + export default { type: "function", function: { @@ -31,6 +33,10 @@ export default { type: ["string", "null"], description: TODOS_PARAMETER_DESCRIPTION, }, + permissions: { + type: ["string", "null"], + description: PERMISSIONS_PARAMETER_DESCRIPTION, + }, }, required: ["mode", "message", "todos"], additionalProperties: false, diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 97f07fcc7aa..80c70841c0c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -39,6 +39,8 @@ import { TaskStatus, TodoItem, getApiProtocol, + type TaskPermissions, + mergeTaskPermissions, getModelId, isRetiredProvider, isIdleAsk, @@ -159,6 +161,7 @@ export class Task extends EventEmitter implements TaskLike { readonly taskId: string readonly rootTaskId?: string readonly parentTaskId?: string + readonly taskPermissions?: TaskPermissions childTaskId?: string pendingNewTaskToolCallId?: string @@ -430,6 +433,7 @@ export class Task extends EventEmitter implements TaskLike { initialTodos, workspacePath, initialStatus, + taskPermissions, }: TaskOptions) { super() @@ -456,6 +460,9 @@ export class Task extends EventEmitter implements TaskLike { this.parentTaskId = historyItem ? historyItem.parentTaskId : parentTask?.taskId this.childTaskId = undefined + // Merge task permissions with parent (most-restrictive-wins) + this.taskPermissions = mergeTaskPermissions(parentTask?.taskPermissions, taskPermissions) + this.metadata = { task: historyItem ? historyItem.task : task, images: historyItem ? [] : images, diff --git a/src/core/tools/NewTaskTool.ts b/src/core/tools/NewTaskTool.ts index f36d8e1e379..01fc72becba 100644 --- a/src/core/tools/NewTaskTool.ts +++ b/src/core/tools/NewTaskTool.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode" import { TodoItem } from "@roo-code/types" +import { type TaskPermissions, taskPermissionsSchema } from "@roo-code/types" import { Task } from "../task/Task" import { getModeBySlug } from "../../shared/modes" @@ -15,13 +16,14 @@ interface NewTaskParams { mode: string message: string todos?: string + permissions?: string } export class NewTaskTool extends BaseTool<"new_task"> { readonly name = "new_task" as const async execute(params: NewTaskParams, task: Task, callbacks: ToolCallbacks): Promise { - const { mode, message, todos } = params + const { mode, message, todos, permissions: permissionsJson } = params const { askApproval, handleError, pushToolResult } = callbacks try { @@ -82,6 +84,33 @@ export class NewTaskTool extends BaseTool<"new_task"> { } } + // Parse and validate permissions if provided + let parsedPermissions: TaskPermissions | undefined + if (permissionsJson) { + try { + const raw = JSON.parse(permissionsJson) + const result = taskPermissionsSchema.safeParse(raw) + if (!result.success) { + task.consecutiveMistakeCount++ + task.recordToolError("new_task") + task.didToolFailInCurrentTurn = true + pushToolResult( + formatResponse.toolError( + `Invalid permissions format: ${result.error.issues.map((i) => i.message).join(", ")}`, + ), + ) + return + } + parsedPermissions = result.data + } catch (error) { + task.consecutiveMistakeCount++ + task.recordToolError("new_task") + task.didToolFailInCurrentTurn = true + pushToolResult(formatResponse.toolError("Invalid permissions: must be a valid JSON string")) + return + } + } + task.consecutiveMistakeCount = 0 // Un-escape one level of backslashes before '@' for hierarchical subtasks @@ -101,6 +130,7 @@ export class NewTaskTool extends BaseTool<"new_task"> { mode: targetMode.name, content: message, todos: todoItems, + ...(parsedPermissions ? { permissions: parsedPermissions } : {}), }) const didApprove = await askApproval("tool", toolMessage) @@ -115,6 +145,7 @@ export class NewTaskTool extends BaseTool<"new_task"> { message: unescapedMessage, initialTodos: todoItems, mode, + permissions: parsedPermissions, }) // Reflect delegation in tool result (no pause/unpause, no wait) diff --git a/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts b/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts new file mode 100644 index 00000000000..3faa2e5e876 --- /dev/null +++ b/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts @@ -0,0 +1,219 @@ +import { describe, it, expect } from "vitest" +import { isToolAllowedForMode, TaskPermissionError } from "../validateToolUse" +import type { TaskPermissions } from "@roo-code/types" +import type { ModeConfig } from "@roo-code/types" + +const codeMode: ModeConfig = { + slug: "code", + name: "Code", + roleDefinition: "You are a coder", + groups: ["read", "edit", "command", "mcp"], +} + +describe("TaskPermissions enforcement in isToolAllowedForMode", () => { + describe("deniedTools", () => { + it("throws TaskPermissionError when tool is in deniedTools", () => { + const permissions: TaskPermissions = { + deniedTools: ["execute_command"], + } + expect(() => + isToolAllowedForMode( + "execute_command", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toThrow(TaskPermissionError) + }) + + it("allows tools not in deniedTools", () => { + const permissions: TaskPermissions = { + deniedTools: ["execute_command"], + } + expect( + isToolAllowedForMode( + "read_file", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + }) + + describe("allowedTools", () => { + it("throws TaskPermissionError when tool is not in allowedTools", () => { + const permissions: TaskPermissions = { + allowedTools: ["read_file", "search_files"], + } + expect(() => + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toThrow(TaskPermissionError) + }) + + it("allows tools in allowedTools", () => { + const permissions: TaskPermissions = { + allowedTools: ["read_file", "write_to_file"], + } + expect( + isToolAllowedForMode( + "read_file", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + + it("always allows ALWAYS_AVAILABLE_TOOLS even when allowedTools is set", () => { + const permissions: TaskPermissions = { + allowedTools: ["read_file"], + } + // attempt_completion and ask_followup_question should always be allowed + expect( + isToolAllowedForMode( + "attempt_completion", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + }) + + describe("filePatterns", () => { + it("throws TaskPermissionError when file path doesn't match any pattern", () => { + const permissions: TaskPermissions = { + filePatterns: ["src/components/.*"], + } + expect(() => + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + { path: "src/utils/helper.ts" }, + undefined, + undefined, + permissions, + ), + ).toThrow(TaskPermissionError) + }) + + it("allows file paths matching a pattern", () => { + const permissions: TaskPermissions = { + filePatterns: ["src/components/.*"], + } + expect( + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + { path: "src/components/Button.tsx" }, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + + it("does not restrict tools without file paths", () => { + const permissions: TaskPermissions = { + filePatterns: ["src/components/.*"], + } + expect( + isToolAllowedForMode( + "search_files", + "code", + [codeMode], + undefined, + { regex: "TODO" }, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + }) + + describe("commandPatterns", () => { + it("throws TaskPermissionError when command doesn't match any pattern", () => { + const permissions: TaskPermissions = { + commandPatterns: ["npm test.*", "npm run lint"], + } + expect(() => + isToolAllowedForMode( + "execute_command", + "code", + [codeMode], + undefined, + { command: "rm -rf /" }, + undefined, + undefined, + permissions, + ), + ).toThrow(TaskPermissionError) + }) + + it("allows commands matching a pattern", () => { + const permissions: TaskPermissions = { + commandPatterns: ["npm test.*", "npm run lint"], + } + expect( + isToolAllowedForMode( + "execute_command", + "code", + [codeMode], + undefined, + { command: "npm test -- --coverage" }, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) + }) + + describe("no permissions", () => { + it("allows all tools when taskPermissions is undefined", () => { + expect( + isToolAllowedForMode( + "execute_command", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + undefined, + ), + ).toBe(true) + }) + }) +}) diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 243a170ed90..010c9371b20 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -1,5 +1,5 @@ -import type { ToolName, ModeConfig, ExperimentId, GroupOptions, GroupEntry } from "@roo-code/types" -import { toolNames as validToolNames } from "@roo-code/types" +import type { ToolName, ModeConfig, ExperimentId, GroupOptions, GroupEntry, TaskPermissions } from "@roo-code/types" +import { toolNames as validToolNames, matchesAnyPattern } from "@roo-code/types" import { customToolRegistry } from "@roo-code/core" import { type Mode, FileRestrictionError, getModeBySlug, getGroupName } from "../../shared/modes" @@ -37,6 +37,7 @@ export function validateToolUse( toolParams?: Record, experiments?: Record, includedTools?: string[], + taskPermissions?: TaskPermissions, ): void { // First, check if the tool name is actually a valid/known tool // This catches completely invalid tool names like "edit_file" that don't exist @@ -56,6 +57,7 @@ export function validateToolUse( toolParams, experiments, includedTools, + taskPermissions, ) ) { throw new Error(`Tool "${toolName}" is not allowed in ${mode} mode.`) @@ -117,6 +119,19 @@ function doesFileMatchRegex(filePath: string, pattern: string): boolean { } } +/** + * Error thrown when a tool is denied by TaskPermissions. + */ +export class TaskPermissionError extends Error { + constructor( + public readonly toolName: string, + public readonly reason: string, + ) { + super(`Tool "${toolName}" is not allowed: ${reason}`) + this.name = "TaskPermissionError" + } +} + export function isToolAllowedForMode( tool: string, modeSlug: string, @@ -125,11 +140,75 @@ export function isToolAllowedForMode( toolParams?: Record, // All tool parameters experiments?: Record, includedTools?: string[], // Opt-in tools explicitly included (e.g., from modelInfo) + taskPermissions?: TaskPermissions, ): boolean { // Resolve alias to canonical name (e.g., "search_and_replace" → "edit") const resolvedTool = TOOL_ALIASES[tool] ?? tool const resolvedIncludedTools = includedTools?.map((t) => TOOL_ALIASES[t] ?? t) + // Check TaskPermissions first -- these are set by the parent task via new_task + if (taskPermissions) { + // Check deniedTools + if (taskPermissions.deniedTools?.includes(resolvedTool) || taskPermissions.deniedTools?.includes(tool)) { + throw new TaskPermissionError(tool, "This tool is denied by the parent task's permission boundaries.") + } + + // Check allowedTools (if set, only these tools are permitted) + if (taskPermissions.allowedTools) { + const isAllowed = + taskPermissions.allowedTools.includes(resolvedTool) || + taskPermissions.allowedTools.includes(tool) || + // Always allow certain critical tools regardless of allowlist + ALWAYS_AVAILABLE_TOOLS.includes(tool as any) + if (!isAllowed) { + throw new TaskPermissionError(tool, "This tool is not in the parent task's allowed tools list.") + } + } + + // Check filePatterns for file-related operations + if (taskPermissions.filePatterns && taskPermissions.filePatterns.length > 0) { + const filePath = toolParams?.path || toolParams?.file_path + if (filePath && typeof filePath === "string") { + if (!matchesAnyPattern(filePath, taskPermissions.filePatterns)) { + throw new TaskPermissionError( + tool, + `File "${filePath}" is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, + ) + } + } + + // Check apply_patch file paths + if (tool === "apply_patch" && typeof toolParams?.patch === "string") { + const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) + for (const patchFilePath of patchFilePaths) { + if (!matchesAnyPattern(patchFilePath, taskPermissions.filePatterns)) { + throw new TaskPermissionError( + tool, + `File "${patchFilePath}" in patch is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, + ) + } + } + } + } + + // Check commandPatterns for execute_command + if ( + taskPermissions.commandPatterns && + taskPermissions.commandPatterns.length > 0 && + resolvedTool === "execute_command" + ) { + const command = toolParams?.command + if (command && typeof command === "string") { + if (!matchesAnyPattern(command, taskPermissions.commandPatterns)) { + throw new TaskPermissionError( + tool, + `Command "${command}" is outside the allowed command patterns: ${taskPermissions.commandPatterns.join(", ")}`, + ) + } + } + } + } + // Check tool requirements first — explicit disabling takes priority over everything, // including ALWAYS_AVAILABLE_TOOLS. This ensures disabledTools works consistently // at both the filtering layer and the execution-time validation layer. diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index aecdb17f316..6fd4ed20cff 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -82,7 +82,7 @@ import { CustomModesManager } from "../config/CustomModesManager" import { Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" -import type { ClineMessage, TodoItem } from "@roo-code/types" +import type { ClineMessage, TodoItem, TaskPermissions } from "@roo-code/types" import { readApiMessages, saveApiMessages, saveTaskMessages, TaskHistoryStore } from "../task-persistence" import { readTaskMessages } from "../task-persistence/taskMessages" import { getNonce } from "./getNonce" @@ -2785,8 +2785,9 @@ export class ClineProvider message: string initialTodos: TodoItem[] mode: string + permissions?: TaskPermissions }): Promise { - const { parentTaskId, message, initialTodos, mode } = params + const { parentTaskId, message, initialTodos, mode, permissions } = params // Metadata-driven delegation is always enabled @@ -2876,6 +2877,7 @@ export class ClineProvider const child = await this.createTask(message, undefined, parent as any, { initialTodos, initialStatus: "active", + taskPermissions: permissions, startTask: false, }) diff --git a/src/shared/tools.ts b/src/shared/tools.ts index 4cac8335ea7..a5d5383fd22 100644 --- a/src/shared/tools.ts +++ b/src/shared/tools.ts @@ -56,6 +56,7 @@ export const toolParamNames = [ "start_line", "end_line", "todos", + "permissions", // new_task parameter for subtask permission boundaries "prompt", "image", // read_file parameters (native protocol) @@ -102,7 +103,7 @@ export type NativeToolArgs = { edit_file: { file_path: string; old_string: string; new_string: string; expected_replacements?: number } apply_patch: { patch: string } list_files: { path: string; recursive?: boolean } - new_task: { mode: string; message: string; todos?: string } + new_task: { mode: string; message: string; todos?: string; permissions?: string } ask_followup_question: { question: string follow_up: Array<{ text: string; mode?: string }> @@ -240,7 +241,7 @@ export interface SwitchModeToolUse extends ToolUse<"switch_mode"> { export interface NewTaskToolUse extends ToolUse<"new_task"> { name: "new_task" - params: Partial, "mode" | "message" | "todos">> + params: Partial, "mode" | "message" | "todos" | "permissions">> } export interface RunSlashCommandToolUse extends ToolUse<"run_slash_command"> { From 6c51a5d52b150198262dc50b46cf74a5753a55c8 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 05:51:37 +0000 Subject: [PATCH 2/6] fix: three bugs in task permissions - parser, deniedTools exemption, pattern merging 1. NativeToolCallParser: Remove permissions from update_todo_list cases (was erroneously added to wrong tool case, should only be on new_task) 2. deniedTools: Exempt ALWAYS_AVAILABLE_TOOLS (attempt_completion, etc.) from deniedTools check, matching the existing allowedTools behavior. Prevents parent from trapping subtask by denying completion tools. 3. Pattern merging: Replace broken exact-string intersection with layered enforcement. filePatterns/commandPatterns from parent and child are kept as separate layers (AND between layers, OR within each layer). This correctly handles narrowing: parent ["src/.*"] + child ["src/components/.*"] now allows only files matching BOTH patterns, instead of producing an empty intersection. --- .../src/__tests__/task-permissions.spec.ts | 118 ++++++++++++++---- packages/types/src/task-permissions.ts | 117 ++++++++++++++++- .../assistant-message/NativeToolCallParser.ts | 2 - src/core/tools/NewTaskTool.ts | 4 +- .../taskPermissionsEnforcement.spec.ts | 103 +++++++++++++-- src/core/tools/validateToolUse.ts | 54 ++++++-- 6 files changed, 346 insertions(+), 52 deletions(-) diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index 101c487d5a4..1fae6741452 100644 --- a/packages/types/src/__tests__/task-permissions.spec.ts +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from "vitest" -import { mergeTaskPermissions, matchesAnyPattern, taskPermissionsSchema } from "../task-permissions.js" +import { + mergeTaskPermissions, + matchesAnyPattern, + matchesAllPatternLayers, + taskPermissionsSchema, + toTaskPermissions, +} from "../task-permissions.js" import type { TaskPermissions } from "../task-permissions.js" describe("TaskPermissions", () => { @@ -34,6 +40,28 @@ describe("TaskPermissions", () => { }) }) + describe("toTaskPermissions", () => { + it("wraps flat filePatterns into a single layer", () => { + const input = { filePatterns: ["src/.*"] } + const result = toTaskPermissions(input) + expect(result._filePatternLayers).toEqual([["src/.*"]]) + expect(result.filePatterns).toEqual(["src/.*"]) + }) + + it("wraps flat commandPatterns into a single layer", () => { + const input = { commandPatterns: ["npm test.*"] } + const result = toTaskPermissions(input) + expect(result._commandPatternLayers).toEqual([["npm test.*"]]) + }) + + it("leaves layers undefined when patterns are not set", () => { + const input = { allowedTools: ["read_file"] } + const result = toTaskPermissions(input) + expect(result._filePatternLayers).toBeUndefined() + expect(result._commandPatternLayers).toBeUndefined() + }) + }) + describe("mergeTaskPermissions", () => { it("returns undefined when both are undefined", () => { expect(mergeTaskPermissions(undefined, undefined)).toBeUndefined() @@ -49,18 +77,25 @@ describe("TaskPermissions", () => { expect(mergeTaskPermissions(parent, undefined)).toEqual(parent) }) - it("intersects filePatterns when both defined", () => { - const parent: TaskPermissions = { filePatterns: ["src/.*", "tests/.*"] } - const child: TaskPermissions = { filePatterns: ["src/.*", "docs/.*"] } + it("accumulates filePatterns as separate layers when both defined", () => { + const parent = toTaskPermissions({ filePatterns: ["src/.*", "tests/.*"] }) + const child = toTaskPermissions({ filePatterns: ["src/.*", "docs/.*"] }) const merged = mergeTaskPermissions(parent, child) - expect(merged?.filePatterns).toEqual(["src/.*"]) + // Both layers are kept (AND semantics between layers) + expect(merged?._filePatternLayers).toEqual([ + ["src/.*", "tests/.*"], + ["src/.*", "docs/.*"], + ]) }) - it("intersects commandPatterns when both defined", () => { - const parent: TaskPermissions = { commandPatterns: ["npm test.*", "npm run lint"] } - const child: TaskPermissions = { commandPatterns: ["npm test.*", "npm run build"] } + it("accumulates commandPatterns as separate layers when both defined", () => { + const parent = toTaskPermissions({ commandPatterns: ["npm test.*", "npm run lint"] }) + const child = toTaskPermissions({ commandPatterns: ["npm test.*", "npm run build"] }) const merged = mergeTaskPermissions(parent, child) - expect(merged?.commandPatterns).toEqual(["npm test.*"]) + expect(merged?._commandPatternLayers).toEqual([ + ["npm test.*", "npm run lint"], + ["npm test.*", "npm run build"], + ]) }) it("intersects allowedTools when both defined", () => { @@ -85,42 +120,50 @@ describe("TaskPermissions", () => { }) it("uses parent filePatterns when child has none", () => { - const parent: TaskPermissions = { filePatterns: ["src/.*"] } + const parent = toTaskPermissions({ filePatterns: ["src/.*"] }) const child: TaskPermissions = { deniedTools: ["execute_command"] } const merged = mergeTaskPermissions(parent, child) - expect(merged?.filePatterns).toEqual(["src/.*"]) + expect(merged?._filePatternLayers).toEqual([["src/.*"]]) expect(merged?.deniedTools).toEqual(["execute_command"]) }) - it("returns empty array when intersection is empty", () => { + it("returns empty array when allowedTools intersection is empty", () => { const parent: TaskPermissions = { allowedTools: ["read_file"] } const child: TaskPermissions = { allowedTools: ["write_to_file"] } const merged = mergeTaskPermissions(parent, child) expect(merged?.allowedTools).toEqual([]) }) - it("handles complex nested merge scenario with exact string matching", () => { - const grandparent: TaskPermissions = { + it("handles nested delegation where child narrows scope", () => { + const grandparent = toTaskPermissions({ filePatterns: ["src/.*"], commandPatterns: ["npm.*"], allowedTools: ["read_file", "write_to_file", "search_files"], deniedTools: ["execute_command"], - } - const parent: TaskPermissions = { + }) + const parent = toTaskPermissions({ filePatterns: ["src/components/.*"], allowedTools: ["read_file", "write_to_file"], - } + }) - // Intersection uses exact string matching, so "src/components/.*" (child) - // is not equal to "src/.*" (parent) -- intersection is empty - const merged1 = mergeTaskPermissions(grandparent, parent) - expect(merged1?.filePatterns).toEqual([]) + const merged = mergeTaskPermissions(grandparent, parent) + + // Both layers are kept -- runtime enforces AND between them + expect(merged?._filePatternLayers).toEqual([["src/.*"], ["src/components/.*"]]) // allowedTools intersection: read_file and write_to_file are in both - expect(merged1?.allowedTools).toEqual(["read_file", "write_to_file"]) + expect(merged?.allowedTools).toEqual(["read_file", "write_to_file"]) // commandPatterns: only grandparent has them, so they pass through - expect(merged1?.commandPatterns).toEqual(["npm.*"]) + expect(merged?._commandPatternLayers).toEqual([["npm.*"]]) // deniedTools: only grandparent has them, so they pass through - expect(merged1?.deniedTools).toEqual(["execute_command"]) + expect(merged?.deniedTools).toEqual(["execute_command"]) + }) + + it("deduplicates identical pattern layers", () => { + const parent = toTaskPermissions({ filePatterns: ["src/.*"] }) + const child = toTaskPermissions({ filePatterns: ["src/.*"] }) + const merged = mergeTaskPermissions(parent, child) + // Identical layers are deduplicated + expect(merged?._filePatternLayers).toEqual([["src/.*"]]) }) }) @@ -149,4 +192,31 @@ describe("TaskPermissions", () => { expect(matchesAnyPattern("rm -rf /", ["npm.*", "yarn.*"])).toBe(false) }) }) + + describe("matchesAllPatternLayers", () => { + it("returns true when layers is undefined", () => { + expect(matchesAllPatternLayers("anything", undefined)).toBe(true) + }) + + it("returns true when layers is empty", () => { + expect(matchesAllPatternLayers("anything", [])).toBe(true) + }) + + it("returns true when value matches all layers", () => { + const layers = [["src/.*"], ["src/components/.*"]] + expect(matchesAllPatternLayers("src/components/Button.tsx", layers)).toBe(true) + }) + + it("returns false when value fails to match one layer", () => { + const layers = [["src/.*"], ["src/components/.*"]] + // Matches src/.* but not src/components/.* + expect(matchesAllPatternLayers("src/utils/helper.ts", layers)).toBe(false) + }) + + it("handles single layer like matchesAnyPattern", () => { + const layers = [["src/.*", "tests/.*"]] + expect(matchesAllPatternLayers("tests/unit/test.ts", layers)).toBe(true) + expect(matchesAllPatternLayers("docs/readme.md", layers)).toBe(false) + }) + }) }) diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts index 96698fe8bf4..a7e1daebc63 100644 --- a/packages/types/src/task-permissions.ts +++ b/packages/types/src/task-permissions.ts @@ -38,13 +38,46 @@ export const taskPermissionsSchema = z.object({ deniedTools: z.array(z.string()).optional(), }) -export type TaskPermissions = z.infer +/** The shape accepted as input from the model via the new_task tool. */ +export type TaskPermissionsInput = z.infer + +/** + * Internal representation of task permissions. Extends the input shape with + * layered pattern fields that accumulate across nested delegation so that + * each ancestor's constraints are enforced independently (AND semantics + * between layers, OR semantics within a layer). + */ +export interface TaskPermissions extends TaskPermissionsInput { + /** + * Accumulated file-pattern layers from ancestor tasks. + * Each inner array is an OR-group; all layers must match (AND between layers). + * Populated only by `mergeTaskPermissions` -- never set from model input. + */ + _filePatternLayers?: string[][] + /** + * Accumulated command-pattern layers from ancestor tasks. + * Same semantics as `_filePatternLayers`. + */ + _commandPatternLayers?: string[][] +} + +/** + * Convert a validated input object (flat arrays) into the internal + * `TaskPermissions` representation, wrapping patterns into single layers. + */ +export function toTaskPermissions(input: TaskPermissionsInput): TaskPermissions { + return { + ...input, + _filePatternLayers: input.filePatterns ? [input.filePatterns] : undefined, + _commandPatternLayers: input.commandPatterns ? [input.commandPatterns] : undefined, + } +} /** * Merge two TaskPermissions using most-restrictive-wins semantics. * - * - filePatterns / commandPatterns: if both define patterns, keep only patterns - * present in both (intersection). If only one side defines patterns, use that. + * - filePatterns / commandPatterns: accumulated as independent layers so that + * a value must match at least one pattern from EACH ancestor's layer. * - allowedTools: intersection of both lists (if both defined). * - deniedTools: union of both lists (most restrictive). * @@ -64,14 +97,77 @@ export function mergeTaskPermissions( return parent } + // Collect pattern layers from both sides. Each side may already carry + // accumulated layers from earlier merges (_*PatternLayers) as well as + // its own top-level patterns (filePatterns / commandPatterns). + const filePatternLayers = collectPatternLayers( + parent._filePatternLayers, + parent.filePatterns, + child._filePatternLayers, + child.filePatterns, + ) + + const commandPatternLayers = collectPatternLayers( + parent._commandPatternLayers, + parent.commandPatterns, + child._commandPatternLayers, + child.commandPatterns, + ) + return { - filePatterns: intersectOptionalArrays(parent.filePatterns, child.filePatterns), - commandPatterns: intersectOptionalArrays(parent.commandPatterns, child.commandPatterns), + // The top-level field stores the child's own patterns (used for display / + // serialization); runtime enforcement uses the layers. + filePatterns: child.filePatterns ?? parent.filePatterns, + commandPatterns: child.commandPatterns ?? parent.commandPatterns, + _filePatternLayers: filePatternLayers.length > 0 ? filePatternLayers : undefined, + _commandPatternLayers: commandPatternLayers.length > 0 ? commandPatternLayers : undefined, allowedTools: intersectOptionalArrays(parent.allowedTools, child.allowedTools), deniedTools: unionOptionalArrays(parent.deniedTools, child.deniedTools), } } +/** + * Collect pattern layers from parent and child, deduplicating identical layers. + */ +function collectPatternLayers( + parentLayers: string[][] | undefined, + parentPatterns: string[] | undefined, + childLayers: string[][] | undefined, + childPatterns: string[] | undefined, +): string[][] { + const layers: string[][] = [] + const seen = new Set() + + const addLayer = (layer: string[]) => { + if (layer.length === 0) return + const key = JSON.stringify(layer) + if (!seen.has(key)) { + seen.add(key) + layers.push(layer) + } + } + + // Add accumulated parent layers + if (parentLayers) { + for (const layer of parentLayers) { + addLayer(layer) + } + } else if (parentPatterns && parentPatterns.length > 0) { + addLayer(parentPatterns) + } + + // Add accumulated child layers + if (childLayers) { + for (const layer of childLayers) { + addLayer(layer) + } + } else if (childPatterns && childPatterns.length > 0) { + addLayer(childPatterns) + } + + return layers +} + /** * Check if a value matches at least one pattern in a list of regex patterns. */ @@ -86,6 +182,17 @@ export function matchesAnyPattern(value: string, patterns: string[]): boolean { }) } +/** + * Check if a value matches ALL pattern layers (AND between layers, OR within each layer). + * Returns true if there are no layers. + */ +export function matchesAllPatternLayers(value: string, layers: string[][] | undefined): boolean { + if (!layers || layers.length === 0) { + return true + } + return layers.every((layer) => matchesAnyPattern(value, layer)) +} + /** * Intersect two optional arrays. If both are defined, return elements present * in both. If only one is defined, return that one. If neither, return undefined. diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index 0b1711c5483..1b794632f3b 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -551,7 +551,6 @@ export class NativeToolCallParser { if (partialArgs.todos !== undefined) { nativeArgs = { todos: partialArgs.todos, - permissions: partialArgs.permissions, } } break @@ -889,7 +888,6 @@ export class NativeToolCallParser { if (args.todos !== undefined) { nativeArgs = { todos: args.todos, - permissions: args.permissions, } as NativeArgsFor } break diff --git a/src/core/tools/NewTaskTool.ts b/src/core/tools/NewTaskTool.ts index 01fc72becba..f3c66b6c2b0 100644 --- a/src/core/tools/NewTaskTool.ts +++ b/src/core/tools/NewTaskTool.ts @@ -1,7 +1,7 @@ import * as vscode from "vscode" import { TodoItem } from "@roo-code/types" -import { type TaskPermissions, taskPermissionsSchema } from "@roo-code/types" +import { type TaskPermissions, taskPermissionsSchema, toTaskPermissions } from "@roo-code/types" import { Task } from "../task/Task" import { getModeBySlug } from "../../shared/modes" @@ -101,7 +101,7 @@ export class NewTaskTool extends BaseTool<"new_task"> { ) return } - parsedPermissions = result.data + parsedPermissions = toTaskPermissions(result.data) } catch (error) { task.consecutiveMistakeCount++ task.recordToolError("new_task") diff --git a/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts b/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts index 3faa2e5e876..f9154e1b214 100644 --- a/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts +++ b/src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest" import { isToolAllowedForMode, TaskPermissionError } from "../validateToolUse" import type { TaskPermissions } from "@roo-code/types" +import { toTaskPermissions, mergeTaskPermissions } from "@roo-code/types" import type { ModeConfig } from "@roo-code/types" const codeMode: ModeConfig = { @@ -47,6 +48,38 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { ), ).toBe(true) }) + + it("never denies ALWAYS_AVAILABLE_TOOLS even when in deniedTools", () => { + const permissions: TaskPermissions = { + deniedTools: ["attempt_completion", "ask_followup_question"], + } + // attempt_completion should always be allowed + expect( + isToolAllowedForMode( + "attempt_completion", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toBe(true) + // ask_followup_question should always be allowed + expect( + isToolAllowedForMode( + "ask_followup_question", + "code", + [codeMode], + undefined, + undefined, + undefined, + undefined, + permissions, + ), + ).toBe(true) + }) }) describe("allowedTools", () => { @@ -108,9 +141,9 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { describe("filePatterns", () => { it("throws TaskPermissionError when file path doesn't match any pattern", () => { - const permissions: TaskPermissions = { + const permissions = toTaskPermissions({ filePatterns: ["src/components/.*"], - } + }) expect(() => isToolAllowedForMode( "write_to_file", @@ -126,9 +159,9 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { }) it("allows file paths matching a pattern", () => { - const permissions: TaskPermissions = { + const permissions = toTaskPermissions({ filePatterns: ["src/components/.*"], - } + }) expect( isToolAllowedForMode( "write_to_file", @@ -144,9 +177,9 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { }) it("does not restrict tools without file paths", () => { - const permissions: TaskPermissions = { + const permissions = toTaskPermissions({ filePatterns: ["src/components/.*"], - } + }) expect( isToolAllowedForMode( "search_files", @@ -162,11 +195,61 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { }) }) + describe("filePatterns layered enforcement", () => { + it("enforces all pattern layers (AND between layers)", () => { + const parent = toTaskPermissions({ filePatterns: ["src/.*"] }) + const child = toTaskPermissions({ filePatterns: ["src/components/.*"] }) + const merged = mergeTaskPermissions(parent, child)! + + // src/components/Button.tsx matches both layers + expect( + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + { path: "src/components/Button.tsx" }, + undefined, + undefined, + merged, + ), + ).toBe(true) + + // src/utils/helper.ts matches parent layer but not child layer + expect(() => + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + { path: "src/utils/helper.ts" }, + undefined, + undefined, + merged, + ), + ).toThrow(TaskPermissionError) + + // tests/test.ts matches neither layer + expect(() => + isToolAllowedForMode( + "write_to_file", + "code", + [codeMode], + undefined, + { path: "tests/test.ts" }, + undefined, + undefined, + merged, + ), + ).toThrow(TaskPermissionError) + }) + }) + describe("commandPatterns", () => { it("throws TaskPermissionError when command doesn't match any pattern", () => { - const permissions: TaskPermissions = { + const permissions = toTaskPermissions({ commandPatterns: ["npm test.*", "npm run lint"], - } + }) expect(() => isToolAllowedForMode( "execute_command", @@ -182,9 +265,9 @@ describe("TaskPermissions enforcement in isToolAllowedForMode", () => { }) it("allows commands matching a pattern", () => { - const permissions: TaskPermissions = { + const permissions = toTaskPermissions({ commandPatterns: ["npm test.*", "npm run lint"], - } + }) expect( isToolAllowedForMode( "execute_command", diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 010c9371b20..d8459c9d01f 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -1,5 +1,5 @@ import type { ToolName, ModeConfig, ExperimentId, GroupOptions, GroupEntry, TaskPermissions } from "@roo-code/types" -import { toolNames as validToolNames, matchesAnyPattern } from "@roo-code/types" +import { toolNames as validToolNames, matchesAnyPattern, matchesAllPatternLayers } from "@roo-code/types" import { customToolRegistry } from "@roo-code/core" import { type Mode, FileRestrictionError, getModeBySlug, getGroupName } from "../../shared/modes" @@ -148,8 +148,13 @@ export function isToolAllowedForMode( // Check TaskPermissions first -- these are set by the parent task via new_task if (taskPermissions) { - // Check deniedTools - if (taskPermissions.deniedTools?.includes(resolvedTool) || taskPermissions.deniedTools?.includes(tool)) { + const isAlwaysAvailable = ALWAYS_AVAILABLE_TOOLS.includes(tool as any) + + // Check deniedTools (but never deny always-available tools like attempt_completion) + if ( + !isAlwaysAvailable && + (taskPermissions.deniedTools?.includes(resolvedTool) || taskPermissions.deniedTools?.includes(tool)) + ) { throw new TaskPermissionError(tool, "This tool is denied by the parent task's permission boundaries.") } @@ -159,14 +164,37 @@ export function isToolAllowedForMode( taskPermissions.allowedTools.includes(resolvedTool) || taskPermissions.allowedTools.includes(tool) || // Always allow certain critical tools regardless of allowlist - ALWAYS_AVAILABLE_TOOLS.includes(tool as any) + isAlwaysAvailable if (!isAllowed) { throw new TaskPermissionError(tool, "This tool is not in the parent task's allowed tools list.") } } - // Check filePatterns for file-related operations - if (taskPermissions.filePatterns && taskPermissions.filePatterns.length > 0) { + // Check filePatterns using layered enforcement (AND between layers, OR within each layer). + // Falls back to flat filePatterns if no layers are present. + const filePatternLayers = taskPermissions._filePatternLayers + if (filePatternLayers && filePatternLayers.length > 0) { + const filePath = toolParams?.path || toolParams?.file_path + if (filePath && typeof filePath === "string") { + if (!matchesAllPatternLayers(filePath, filePatternLayers)) { + throw new TaskPermissionError(tool, `File "${filePath}" is outside the allowed file patterns.`) + } + } + + // Check apply_patch file paths + if (tool === "apply_patch" && typeof toolParams?.patch === "string") { + const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) + for (const patchFilePath of patchFilePaths) { + if (!matchesAllPatternLayers(patchFilePath, filePatternLayers)) { + throw new TaskPermissionError( + tool, + `File "${patchFilePath}" in patch is outside the allowed file patterns.`, + ) + } + } + } + } else if (taskPermissions.filePatterns && taskPermissions.filePatterns.length > 0) { + // Fallback for non-merged permissions (single layer) const filePath = toolParams?.path || toolParams?.file_path if (filePath && typeof filePath === "string") { if (!matchesAnyPattern(filePath, taskPermissions.filePatterns)) { @@ -177,7 +205,6 @@ export function isToolAllowedForMode( } } - // Check apply_patch file paths if (tool === "apply_patch" && typeof toolParams?.patch === "string") { const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) for (const patchFilePath of patchFilePaths) { @@ -191,12 +218,21 @@ export function isToolAllowedForMode( } } - // Check commandPatterns for execute_command - if ( + // Check commandPatterns using layered enforcement + const commandPatternLayers = taskPermissions._commandPatternLayers + if (commandPatternLayers && commandPatternLayers.length > 0 && resolvedTool === "execute_command") { + const command = toolParams?.command + if (command && typeof command === "string") { + if (!matchesAllPatternLayers(command, commandPatternLayers)) { + throw new TaskPermissionError(tool, `Command "${command}" is outside the allowed command patterns.`) + } + } + } else if ( taskPermissions.commandPatterns && taskPermissions.commandPatterns.length > 0 && resolvedTool === "execute_command" ) { + // Fallback for non-merged permissions (single layer) const command = toolParams?.command if (command && typeof command === "string") { if (!matchesAnyPattern(command, taskPermissions.commandPatterns)) { From d9a172d2ede8fdaa909a7fd1e7108e771d979171 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 06:04:41 +0000 Subject: [PATCH 3/6] fix: harden task permissions - anchor regex patterns, validate at schema level, simplify validation code 1. Anchor regex patterns in matchesAnyPattern with ^(?:...)$ wrapping so patterns like "src/.*" require full-path matching instead of substring matching. Prevents "evil/src/foo" from matching a "src/.*" permission. 2. Add regex validation at schema level (regexString refinement) so invalid patterns are rejected at parse time rather than silently failing at runtime. 3. Simplify duplicate file/command pattern validation in validateToolUse by unifying layered and flat code paths into a single branch that falls back to wrapping flat patterns as a single layer. 4. Remove unused matchesAnyPattern import from validateToolUse.ts. 5. Add tests for anchoring behavior, pre-anchored patterns, and invalid regex rejection at schema level. --- .../src/__tests__/task-permissions.spec.ts | 27 +++++++++ packages/types/src/task-permissions.ts | 27 +++++++-- src/core/tools/validateToolUse.ts | 56 ++++--------------- 3 files changed, 61 insertions(+), 49 deletions(-) diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index 1fae6741452..cea98cff38e 100644 --- a/packages/types/src/__tests__/task-permissions.spec.ts +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -38,6 +38,20 @@ describe("TaskPermissions", () => { }) expect(result.success).toBe(false) }) + + it("rejects invalid regex patterns in filePatterns", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["[invalid"], + }) + expect(result.success).toBe(false) + }) + + it("rejects invalid regex patterns in commandPatterns", () => { + const result = taskPermissionsSchema.safeParse({ + commandPatterns: ["(unclosed"], + }) + expect(result.success).toBe(false) + }) }) describe("toTaskPermissions", () => { @@ -191,6 +205,19 @@ describe("TaskPermissions", () => { it("does not match restricted commands", () => { expect(matchesAnyPattern("rm -rf /", ["npm.*", "yarn.*"])).toBe(false) }) + + it("anchors patterns so substrings do not match", () => { + // "src/.*" should NOT match a path that merely contains "src/" as a substring + expect(matchesAnyPattern("evil/src/components/foo.ts", ["src/.*"])).toBe(false) + // But should still match paths that start with src/ + expect(matchesAnyPattern("src/components/foo.ts", ["src/.*"])).toBe(true) + }) + + it("respects pre-anchored patterns (starting with ^)", () => { + // A pattern already starting with ^ should not be double-wrapped + expect(matchesAnyPattern("src/foo.ts", ["^src/.*$"])).toBe(true) + expect(matchesAnyPattern("evil/src/foo.ts", ["^src/.*$"])).toBe(false) + }) }) describe("matchesAllPatternLayers", () => { diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts index a7e1daebc63..61f5001ffe9 100644 --- a/packages/types/src/task-permissions.ts +++ b/packages/types/src/task-permissions.ts @@ -9,20 +9,34 @@ import { z } from "zod" * more access than its parent. */ +/** Zod refinement that rejects strings which are not valid regular expressions. */ +const regexString = z.string().refine( + (val) => { + try { + new RegExp(val) + return true + } catch { + return false + } + }, + { message: "Invalid regular expression" }, +) + export const taskPermissionsSchema = z.object({ /** * Regex patterns for allowed file paths. * When set, file operations (read/write) are restricted to paths matching - * at least one of these patterns. + * at least one of these patterns. Patterns are automatically anchored + * (wrapped in `^(?:...)$`) at runtime so they match the full path. */ - filePatterns: z.array(z.string()).optional(), + filePatterns: z.array(regexString).optional(), /** * Regex patterns for allowed shell commands. * When set, command execution is restricted to commands matching - * at least one of these patterns. + * at least one of these patterns. Patterns are automatically anchored. */ - commandPatterns: z.array(z.string()).optional(), + commandPatterns: z.array(regexString).optional(), /** * Explicit tool allowlist. When set, only these tools may be used @@ -174,7 +188,10 @@ function collectPatternLayers( export function matchesAnyPattern(value: string, patterns: string[]): boolean { return patterns.some((pattern) => { try { - return new RegExp(pattern).test(value) + // Anchor patterns so they must match the entire value, not a substring. + // This prevents "src/.*" from matching "evil/src/foo". + const anchored = pattern.startsWith("^") ? pattern : `^(?:${pattern})$` + return new RegExp(anchored).test(value) } catch { // Invalid regex -- treat as non-match return false diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index d8459c9d01f..6dd9597f0a6 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -1,5 +1,5 @@ import type { ToolName, ModeConfig, ExperimentId, GroupOptions, GroupEntry, TaskPermissions } from "@roo-code/types" -import { toolNames as validToolNames, matchesAnyPattern, matchesAllPatternLayers } from "@roo-code/types" +import { toolNames as validToolNames, matchesAllPatternLayers } from "@roo-code/types" import { customToolRegistry } from "@roo-code/core" import { type Mode, FileRestrictionError, getModeBySlug, getGroupName } from "../../shared/modes" @@ -170,9 +170,12 @@ export function isToolAllowedForMode( } } - // Check filePatterns using layered enforcement (AND between layers, OR within each layer). - // Falls back to flat filePatterns if no layers are present. - const filePatternLayers = taskPermissions._filePatternLayers + // Check filePatterns -- use layered enforcement when available (AND between + // layers, OR within each layer), fall back to flat filePatterns as a single layer. + const filePatternLayers = + taskPermissions._filePatternLayers ?? + (taskPermissions.filePatterns?.length ? [taskPermissions.filePatterns] : undefined) + if (filePatternLayers && filePatternLayers.length > 0) { const filePath = toolParams?.path || toolParams?.file_path if (filePath && typeof filePath === "string") { @@ -193,33 +196,13 @@ export function isToolAllowedForMode( } } } - } else if (taskPermissions.filePatterns && taskPermissions.filePatterns.length > 0) { - // Fallback for non-merged permissions (single layer) - const filePath = toolParams?.path || toolParams?.file_path - if (filePath && typeof filePath === "string") { - if (!matchesAnyPattern(filePath, taskPermissions.filePatterns)) { - throw new TaskPermissionError( - tool, - `File "${filePath}" is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, - ) - } - } - - if (tool === "apply_patch" && typeof toolParams?.patch === "string") { - const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) - for (const patchFilePath of patchFilePaths) { - if (!matchesAnyPattern(patchFilePath, taskPermissions.filePatterns)) { - throw new TaskPermissionError( - tool, - `File "${patchFilePath}" in patch is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, - ) - } - } - } } - // Check commandPatterns using layered enforcement - const commandPatternLayers = taskPermissions._commandPatternLayers + // Check commandPatterns -- same layered approach as filePatterns. + const commandPatternLayers = + taskPermissions._commandPatternLayers ?? + (taskPermissions.commandPatterns?.length ? [taskPermissions.commandPatterns] : undefined) + if (commandPatternLayers && commandPatternLayers.length > 0 && resolvedTool === "execute_command") { const command = toolParams?.command if (command && typeof command === "string") { @@ -227,21 +210,6 @@ export function isToolAllowedForMode( throw new TaskPermissionError(tool, `Command "${command}" is outside the allowed command patterns.`) } } - } else if ( - taskPermissions.commandPatterns && - taskPermissions.commandPatterns.length > 0 && - resolvedTool === "execute_command" - ) { - // Fallback for non-merged permissions (single layer) - const command = toolParams?.command - if (command && typeof command === "string") { - if (!matchesAnyPattern(command, taskPermissions.commandPatterns)) { - throw new TaskPermissionError( - tool, - `Command "${command}" is outside the allowed command patterns: ${taskPermissions.commandPatterns.join(", ")}`, - ) - } - } } } From eb256220e7c3d2ef8d3caa45faff6afbb688dc8f Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 06:17:03 +0000 Subject: [PATCH 4/6] fix: persist permissions in HistoryItem and add ReDoS mitigation 1. Persist taskPermissions in HistoryItem so permissions survive task restarts. Added taskPermissions field to historyItemSchema, included it in taskMetadata output, and restored it in the Task constructor when loading from history. 2. Add ReDoS mitigation for model-provided regex patterns: - isSafeRegex() heuristic rejects nested quantifiers like (a+)+ and overlapping alternations in repeated groups like (a|a)+ - Max pattern length capped at 200 characters - Both checks enforced at schema validation time via Zod refinements - 11 new tests covering ReDoS detection and persistence round-trips --- .../src/__tests__/task-permissions.spec.ts | 76 +++++++++++++++++++ packages/types/src/history.ts | 3 + packages/types/src/task-permissions.ts | 68 ++++++++++++++--- src/core/task-persistence/taskMetadata.ts | 6 +- src/core/task/Task.ts | 27 ++++++- 5 files changed, 165 insertions(+), 15 deletions(-) diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index cea98cff38e..48990c98752 100644 --- a/packages/types/src/__tests__/task-permissions.spec.ts +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -5,6 +5,7 @@ import { matchesAllPatternLayers, taskPermissionsSchema, toTaskPermissions, + isSafeRegex, } from "../task-permissions.js" import type { TaskPermissions } from "../task-permissions.js" @@ -246,4 +247,79 @@ describe("TaskPermissions", () => { expect(matchesAllPatternLayers("docs/readme.md", layers)).toBe(false) }) }) + + describe("isSafeRegex", () => { + it("accepts simple file path patterns", () => { + expect(isSafeRegex("src/.*")).toBe(true) + expect(isSafeRegex("src/components/.*\\.tsx")).toBe(true) + expect(isSafeRegex("npm test.*")).toBe(true) + }) + + it("rejects nested quantifiers (classic ReDoS)", () => { + expect(isSafeRegex("(a+)+")).toBe(false) + expect(isSafeRegex("(a*)+")).toBe(false) + expect(isSafeRegex("(a+)*")).toBe(false) + expect(isSafeRegex("(a+){2,}")).toBe(false) + }) + + it("rejects overlapping alternations in repeated groups", () => { + expect(isSafeRegex("(a|a)+")).toBe(false) + expect(isSafeRegex("(.|a)*")).toBe(false) + }) + + it("rejects patterns exceeding maximum length", () => { + const longPattern = "a".repeat(201) + expect(isSafeRegex(longPattern)).toBe(false) + }) + + it("accepts patterns at maximum length", () => { + const maxPattern = "a".repeat(200) + expect(isSafeRegex(maxPattern)).toBe(true) + }) + }) + + describe("schema ReDoS rejection", () => { + it("rejects ReDoS-vulnerable patterns in filePatterns", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["(a+)+"], + }) + expect(result.success).toBe(false) + }) + + it("rejects ReDoS-vulnerable patterns in commandPatterns", () => { + const result = taskPermissionsSchema.safeParse({ + commandPatterns: ["(cmd|cmd)*"], + }) + expect(result.success).toBe(false) + }) + + it("rejects overly long patterns at schema level", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["a".repeat(201)], + }) + expect(result.success).toBe(false) + }) + }) + + describe("persistence round-trip", () => { + it("taskPermissionsSchema can parse persisted permissions (without internal fields)", () => { + // Simulate what gets persisted: only the input-level fields + const persisted = { + filePatterns: ["src/.*"], + commandPatterns: ["npm test.*"], + allowedTools: ["read_file"], + deniedTools: ["execute_command"], + } + const result = taskPermissionsSchema.safeParse(persisted) + expect(result.success).toBe(true) + if (result.success) { + // Can be converted back to internal representation + const restored = toTaskPermissions(result.data) + expect(restored._filePatternLayers).toEqual([["src/.*"]]) + expect(restored._commandPatternLayers).toEqual([["npm test.*"]]) + expect(restored.allowedTools).toEqual(["read_file"]) + expect(restored.deniedTools).toEqual(["execute_command"]) + } + }) + }) }) diff --git a/packages/types/src/history.ts b/packages/types/src/history.ts index a60d1a75b65..8bf1d3f36b8 100644 --- a/packages/types/src/history.ts +++ b/packages/types/src/history.ts @@ -1,5 +1,7 @@ import { z } from "zod" +import { taskPermissionsSchema } from "./task-permissions.js" + /** * HistoryItem */ @@ -26,6 +28,7 @@ export const historyItemSchema = z.object({ awaitingChildId: z.string().optional(), // Child currently awaited (set when delegated) completedByChildId: z.string().optional(), // Child that completed and resumed this parent completionResultSummary: z.string().optional(), // Summary from completed child + taskPermissions: taskPermissionsSchema.optional(), // Permission boundaries set by parent task }) export type HistoryItem = z.infer diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts index 61f5001ffe9..938b321ec77 100644 --- a/packages/types/src/task-permissions.ts +++ b/packages/types/src/task-permissions.ts @@ -9,18 +9,62 @@ import { z } from "zod" * more access than its parent. */ -/** Zod refinement that rejects strings which are not valid regular expressions. */ -const regexString = z.string().refine( - (val) => { - try { - new RegExp(val) - return true - } catch { - return false - } - }, - { message: "Invalid regular expression" }, -) +/** Maximum allowed length for a regex pattern to limit complexity. */ +const MAX_REGEX_PATTERN_LENGTH = 200 + +/** + * Heuristic check for ReDoS-vulnerable patterns. + * Detects common dangerous constructs like nested quantifiers: + * (a+)+, (a*)+, (a+)*, (a*){2,}, etc. + * These can cause catastrophic backtracking on crafted input. + */ +export function isSafeRegex(pattern: string): boolean { + if (pattern.length > MAX_REGEX_PATTERN_LENGTH) { + return false + } + + // Detect nested quantifiers: a group with a quantifier inside, followed by an outer quantifier. + // Examples: (a+)+, (a+)*, (a*){2,}, (?:a+)+ + // This regex looks for: group containing a quantifier, followed by another quantifier + const nestedQuantifierPattern = /\([^)]*[+*][^)]*\)[+*{]/ + if (nestedQuantifierPattern.test(pattern)) { + return false + } + + // Detect overlapping alternations inside repeated groups: (a|a)+, (.|a)+ + // where both alternatives can match the same input + const overlappingAlternationInGroup = /\([^)]*\|[^)]*\)[+*{]/ + if (overlappingAlternationInGroup.test(pattern)) { + return false + } + + return true +} + +/** + * Zod refinement that rejects strings which are not valid regular expressions, + * and also rejects patterns that are vulnerable to ReDoS (catastrophic backtracking). + */ +const regexString = z + .string() + .max(MAX_REGEX_PATTERN_LENGTH, { + message: `Regex pattern must be at most ${MAX_REGEX_PATTERN_LENGTH} characters`, + }) + .refine( + (val) => { + try { + new RegExp(val) + return true + } catch { + return false + } + }, + { message: "Invalid regular expression" }, + ) + .refine((val) => isSafeRegex(val), { + message: + "Regex pattern rejected: potentially vulnerable to ReDoS (catastrophic backtracking). Avoid nested quantifiers like (a+)+ or overlapping alternations in repeated groups.", + }) export const taskPermissionsSchema = z.object({ /** diff --git a/src/core/task-persistence/taskMetadata.ts b/src/core/task-persistence/taskMetadata.ts index 4b771269713..83dee5fb478 100644 --- a/src/core/task-persistence/taskMetadata.ts +++ b/src/core/task-persistence/taskMetadata.ts @@ -1,7 +1,7 @@ import NodeCache from "node-cache" import getFolderSize from "get-folder-size" -import type { ClineMessage, HistoryItem } from "@roo-code/types" +import type { ClineMessage, HistoryItem, TaskPermissionsInput } from "@roo-code/types" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" @@ -25,6 +25,8 @@ export type TaskMetadataOptions = { apiConfigName?: string /** Initial status for the task (e.g., "active" for child tasks) */ initialStatus?: "active" | "delegated" | "completed" + /** Permission boundaries for the task, set by the parent via new_task tool */ + taskPermissions?: TaskPermissionsInput } export async function taskMetadata({ @@ -38,6 +40,7 @@ export async function taskMetadata({ mode, apiConfigName, initialStatus, + taskPermissions, }: TaskMetadataOptions) { const taskDir = await getTaskDirectoryPath(globalStoragePath, id) @@ -112,6 +115,7 @@ export async function taskMetadata({ mode, ...(typeof apiConfigName === "string" && apiConfigName.length > 0 ? { apiConfigName } : {}), ...(initialStatus && { status: initialStatus }), + ...(taskPermissions && { taskPermissions }), } return { historyItem, tokenUsage } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 80c70841c0c..0b38bcb322f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -41,6 +41,7 @@ import { getApiProtocol, type TaskPermissions, mergeTaskPermissions, + toTaskPermissions, getModelId, isRetiredProvider, isIdleAsk, @@ -460,8 +461,13 @@ export class Task extends EventEmitter implements TaskLike { this.parentTaskId = historyItem ? historyItem.parentTaskId : parentTask?.taskId this.childTaskId = undefined - // Merge task permissions with parent (most-restrictive-wins) - this.taskPermissions = mergeTaskPermissions(parentTask?.taskPermissions, taskPermissions) + // Merge task permissions with parent (most-restrictive-wins). + // When restoring from history, use the persisted permissions as the base; + // when creating fresh, use the permissions passed via new_task tool. + const effectivePermissions = historyItem?.taskPermissions + ? toTaskPermissions(historyItem.taskPermissions) + : taskPermissions + this.taskPermissions = mergeTaskPermissions(parentTask?.taskPermissions, effectivePermissions) this.metadata = { task: historyItem ? historyItem.task : task, @@ -1182,6 +1188,19 @@ export class Task extends EventEmitter implements TaskLike { await this.taskApiConfigReady } + // Serialize only the input-level permission fields for persistence + // (exclude internal _*PatternLayers fields which are runtime-only) + const persistablePermissions = this.taskPermissions + ? { + ...(this.taskPermissions.filePatterns && { filePatterns: this.taskPermissions.filePatterns }), + ...(this.taskPermissions.commandPatterns && { + commandPatterns: this.taskPermissions.commandPatterns, + }), + ...(this.taskPermissions.allowedTools && { allowedTools: this.taskPermissions.allowedTools }), + ...(this.taskPermissions.deniedTools && { deniedTools: this.taskPermissions.deniedTools }), + } + : undefined + const { historyItem, tokenUsage } = await taskMetadata({ taskId: this.taskId, rootTaskId: this.rootTaskId, @@ -1193,6 +1212,10 @@ export class Task extends EventEmitter implements TaskLike { mode: this._taskMode || defaultModeSlug, // Use the task's own mode, not the current provider mode. apiConfigName: this._taskApiConfigName, // Use the task's own provider profile, not the current provider profile. initialStatus: this.initialStatus, + taskPermissions: + persistablePermissions && Object.keys(persistablePermissions).length > 0 + ? persistablePermissions + : undefined, }) // Emit token/tool usage updates using debounced function From c0a84c0239cc2d1d1481408a71e4a3f6d2d1f8f4 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 06:58:45 +0000 Subject: [PATCH 5/6] feat: add Orchestrator prompt guidance for permissions and UI visibility - Enhance Orchestrator customInstructions with guidance on using the permissions parameter (filePatterns, commandPatterns, allowedTools, deniedTools) including example use cases and most-restrictive-wins semantics explanation - Add permission boundaries display in the ChatRow newTask approval message so users can see what restrictions are being set before approving subtask creation - Add i18n translation keys for permission display - Add 8 new tests across packages/types and webview-ui --- .../orchestrator-permissions-prompt.spec.ts | 32 +++ packages/types/src/mode.ts | 2 +- packages/types/src/vscode-extension-host.ts | 7 + webview-ui/src/components/chat/ChatRow.tsx | 33 ++++ .../__tests__/ChatRow.permissions.spec.tsx | 182 ++++++++++++++++++ webview-ui/src/i18n/locales/en/chat.json | 7 +- 6 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 packages/types/src/__tests__/orchestrator-permissions-prompt.spec.ts create mode 100644 webview-ui/src/components/chat/__tests__/ChatRow.permissions.spec.tsx diff --git a/packages/types/src/__tests__/orchestrator-permissions-prompt.spec.ts b/packages/types/src/__tests__/orchestrator-permissions-prompt.spec.ts new file mode 100644 index 00000000000..1a6a9bcc3ae --- /dev/null +++ b/packages/types/src/__tests__/orchestrator-permissions-prompt.spec.ts @@ -0,0 +1,32 @@ +import { describe, it, expect } from "vitest" +import { DEFAULT_MODES } from "../mode.js" +import type { ModeConfig } from "../mode.js" + +describe("Orchestrator mode - permissions prompt guidance", () => { + const orchestratorMode = DEFAULT_MODES.find((m: ModeConfig) => m.slug === "orchestrator") + + it("should have the orchestrator mode defined", () => { + expect(orchestratorMode).toBeDefined() + }) + + it("should include permissions guidance in customInstructions", () => { + expect(orchestratorMode!.customInstructions).toContain("permissions") + expect(orchestratorMode!.customInstructions).toContain("filePatterns") + expect(orchestratorMode!.customInstructions).toContain("commandPatterns") + expect(orchestratorMode!.customInstructions).toContain("allowedTools") + expect(orchestratorMode!.customInstructions).toContain("deniedTools") + }) + + it("should mention most-restrictive-wins semantics", () => { + expect(orchestratorMode!.customInstructions).toContain("most-restrictive-wins") + }) + + it("should provide example use cases for permissions", () => { + // Guidance about restricting file access + expect(orchestratorMode!.customInstructions).toContain("specific directory") + // Guidance about read-only research tasks + expect(orchestratorMode!.customInstructions).toContain("read-only research") + // Guidance about blocking shell access + expect(orchestratorMode!.customInstructions).toContain("shell access") + }) +}) diff --git a/packages/types/src/mode.ts b/packages/types/src/mode.ts index 249e9820af0..b482c0d680b 100644 --- a/packages/types/src/mode.ts +++ b/packages/types/src/mode.ts @@ -222,6 +222,6 @@ export const DEFAULT_MODES: readonly ModeConfig[] = [ description: "Coordinate tasks across multiple modes", groups: [], customInstructions: - "Your role is to coordinate complex workflows by delegating tasks to specialized modes. As an orchestrator, you should:\n\n1. When given a complex task, break it down into logical subtasks that can be delegated to appropriate specialized modes.\n\n2. For each subtask, use the `new_task` tool to delegate. Choose the most appropriate mode for the subtask's specific goal and provide comprehensive instructions in the `message` parameter. These instructions must include:\n * All necessary context from the parent task or previous subtasks required to complete the work.\n * A clearly defined scope, specifying exactly what the subtask should accomplish.\n * An explicit statement that the subtask should *only* perform the work outlined in these instructions and not deviate.\n * An instruction for the subtask to signal completion by using the `attempt_completion` tool, providing a concise yet thorough summary of the outcome in the `result` parameter, keeping in mind that this summary will be the source of truth used to keep track of what was completed on this project.\n * A statement that these specific instructions supersede any conflicting general instructions the subtask's mode might have.\n\n3. Track and manage the progress of all subtasks. When a subtask is completed, analyze its results and determine the next steps.\n\n4. Help the user understand how the different subtasks fit together in the overall workflow. Provide clear reasoning about why you're delegating specific tasks to specific modes.\n\n5. When all subtasks are completed, synthesize the results and provide a comprehensive overview of what was accomplished.\n\n6. Ask clarifying questions when necessary to better understand how to break down complex tasks effectively.\n\n7. Suggest improvements to the workflow based on the results of completed subtasks.\n\nUse subtasks to maintain clarity. If a request significantly shifts focus or requires a different expertise (mode), consider creating a subtask rather than overloading the current one.", + 'Your role is to coordinate complex workflows by delegating tasks to specialized modes. As an orchestrator, you should:\n\n1. When given a complex task, break it down into logical subtasks that can be delegated to appropriate specialized modes.\n\n2. For each subtask, use the `new_task` tool to delegate. Choose the most appropriate mode for the subtask\'s specific goal and provide comprehensive instructions in the `message` parameter. These instructions must include:\n * All necessary context from the parent task or previous subtasks required to complete the work.\n * A clearly defined scope, specifying exactly what the subtask should accomplish.\n * An explicit statement that the subtask should *only* perform the work outlined in these instructions and not deviate.\n * An instruction for the subtask to signal completion by using the `attempt_completion` tool, providing a concise yet thorough summary of the outcome in the `result` parameter, keeping in mind that this summary will be the source of truth used to keep track of what was completed on this project.\n * A statement that these specific instructions supersede any conflicting general instructions the subtask\'s mode might have.\n\n3. Track and manage the progress of all subtasks. When a subtask is completed, analyze its results and determine the next steps.\n\n4. Help the user understand how the different subtasks fit together in the overall workflow. Provide clear reasoning about why you\'re delegating specific tasks to specific modes.\n\n5. When all subtasks are completed, synthesize the results and provide a comprehensive overview of what was accomplished.\n\n6. Ask clarifying questions when necessary to better understand how to break down complex tasks effectively.\n\n7. Suggest improvements to the workflow based on the results of completed subtasks.\n\n8. When delegating subtasks, consider using the optional `permissions` parameter on `new_task` to restrict what the subtask can do. This is especially useful when:\n * The subtask should only modify files in a specific directory (use `filePatterns`, e.g. `["src/components/.*"]`).\n * The subtask should only run certain commands (use `commandPatterns`, e.g. `["npm test.*", "npm run lint"]`).\n * The subtask should be limited to specific tools (use `allowedTools`, e.g. `["read_file", "search_files"]` for read-only research tasks).\n * Certain tools should be explicitly blocked (use `deniedTools`, e.g. `["execute_command"]` to prevent shell access).\n Permissions are enforced at runtime and follow most-restrictive-wins semantics when subtasks are nested. Use them to keep subtasks focused and safe.\n\nUse subtasks to maintain clarity. If a request significantly shifts focus or requires a different expertise (mode), consider creating a subtask rather than overloading the current one.', }, ] as const diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index a4ef802efbc..b4f02003ba9 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -766,6 +766,13 @@ export interface ClineSayTool { description?: string // Properties for skill tool skill?: string + // Properties for newTask tool - permission boundaries set by parent + permissions?: { + filePatterns?: string[] + commandPatterns?: string[] + allowedTools?: string[] + deniedTools?: string[] + } } export interface ClineAskUseMcpServer { diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 33c9acb2df2..68bad8f7dca 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -869,6 +869,39 @@ export const ChatRowContent = ({
+ {tool.permissions && ( +
+
{t("chat:subtasks.permissionBoundaries")}
+ {tool.permissions.filePatterns && ( +
+ {t("chat:subtasks.permissionFilePatterns", { + patterns: tool.permissions.filePatterns.join(", "), + })} +
+ )} + {tool.permissions.commandPatterns && ( +
+ {t("chat:subtasks.permissionCommandPatterns", { + patterns: tool.permissions.commandPatterns.join(", "), + })} +
+ )} + {tool.permissions.allowedTools && ( +
+ {t("chat:subtasks.permissionAllowedTools", { + tools: tool.permissions.allowedTools.join(", "), + })} +
+ )} + {tool.permissions.deniedTools && ( +
+ {t("chat:subtasks.permissionDeniedTools", { + tools: tool.permissions.deniedTools.join(", "), + })} +
+ )} +
+ )}
{childTaskId && !isFollowedBySubtaskResult && (