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.
This commit is contained in:
Roo Code 2026-05-12 05:51:37 +00:00
parent 22086b5b86
commit 6c51a5d52b
6 changed files with 346 additions and 52 deletions

View file

@ -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)
})
})
})

View file

@ -38,13 +38,46 @@ export const taskPermissionsSchema = z.object({
deniedTools: z.array(z.string()).optional(),
})
export type TaskPermissions = z.infer<typeof taskPermissionsSchema>
/** The shape accepted as input from the model via the new_task tool. */
export type TaskPermissionsInput = z.infer<typeof taskPermissionsSchema>
/**
* 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<string>()
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.

View file

@ -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<TName>
}
break

View file

@ -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")

View file

@ -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",

View file

@ -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)) {