diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index 101c487d5a..1fae674145 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 96698fe8bf..a7e1daebc6 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 0b1711c548..1b794632f3 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 01fc72becb..f3c66b6c2b 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 3faa2e5e87..f9154e1b21 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 010c9371b2..d8459c9d01 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)) {