feat(modes): add regex scoped read permissions per mode + orchestrator skills read allowlist

packages/types/src/mode.ts

@@ -188,7 +188,20 @@ export const DEFAULT_MODES: readonly ModeConfig[] = [
 		whenToUse:
 			"Use this mode for complex, multi-step projects that require coordination across different specialties. Ideal when you need to break down large tasks into subtasks, manage workflows, or coordinate work that spans multiple domains or expertise areas.",
 		description: "Coordinate tasks across multiple modes",
-		groups: [],
+		groups: [
+			[
+				"read",
+				{
+					// Allow both workspace-relative and absolute skill paths (cross-platform).
+					// Examples:
+					// - .roo/skills/example-skill/SKILL.md
+					// - /Users/alice/.roo/skills/example-skill/SKILL.md
+					// - C:\\Users\\alice\\.roo\\skills\\example-skill\\SKILL.md
+					fileRegex: "(^|.*[\\\\/])\\.roo[\\\\/]skills(-[a-zA-Z0-9-]+)?[\\\\/][^\\\\/]+[\\\\/]SKILL\\.md$",
+					description: "Skill definition files only",
+				},
+			],
+		],
 		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.",
 	},

src/core/assistant-message/presentAssistantMessage.ts

@@ -714,13 +714,19 @@ export async function presentAssistantMessage(cline: Task) {
 				const { resolveToolAlias } = await import("../prompts/tools/filter-tools-for-mode")
 				const includedTools = rawIncludedTools?.map((tool) => resolveToolAlias(tool))
 
+				// Prefer nativeArgs for validation when available (e.g., read_file uses nativeArgs.files).
+				// nativeArgs should win on key collisions.
+				const toolParamsForValidation = block.nativeArgs
+					? { ...block.params, ...block.nativeArgs }
+					: block.params
+
 				try {
 					validateToolUse(
 						block.name as ToolName,
 						mode ?? defaultModeSlug,
 						customModes ?? [],
 						{ apply_diff: cline.diffEnabled },
-						block.params,
+						toolParamsForValidation,
 						stateExperiments,
 						includedTools,
 					)

src/core/tools/__tests__/validateToolUse.spec.ts

@@ -185,6 +185,50 @@ describe("mode-validator", () => {
 			expect(() => validateToolUse("read_file", "architect", [])).not.toThrow()
 		})
 
+		it("enforces read fileRegex restrictions for read_file", () => {
+			const customModes: ModeConfig[] = [
+				{
+					slug: "md-reader",
+					name: "Markdown Reader",
+					roleDefinition: "Read markdown only",
+					groups: [["read", { fileRegex: "\\.md$" }]] as const,
+				},
+			]
+
+			expect(() =>
+				validateToolUse("read_file", "md-reader", customModes, undefined, {
+					files: [{ path: "src/index.ts" }],
+				}),
+			).toThrow(/can only read files matching pattern/)
+
+			expect(() =>
+				validateToolUse("read_file", "md-reader", customModes, undefined, { files: [{ path: "README.md" }] }),
+			).not.toThrow()
+		})
+
+		it("enforces read fileRegex restrictions when toolParams.files is a JSON string", () => {
+			const customModes: ModeConfig[] = [
+				{
+					slug: "md-reader",
+					name: "Markdown Reader",
+					roleDefinition: "Read markdown only",
+					groups: [["read", { fileRegex: "\\.md$" }]] as const,
+				},
+			]
+
+			expect(() =>
+				validateToolUse("read_file", "md-reader", customModes, undefined, {
+					files: '[{"path":"src/index.ts"}]',
+				}),
+			).toThrow(/can only read files matching pattern/)
+
+			expect(() =>
+				validateToolUse("read_file", "md-reader", customModes, undefined, {
+					files: '[{"path":"README.md"}]',
+				}),
+			).not.toThrow()
+		})
+
 		it("throws error when tool requirement is not met", () => {
 			const requirements = { apply_diff: false }
 			expect(() => validateToolUse("apply_diff", codeMode, [], requirements)).toThrow(

src/core/tools/validateToolUse.ts

@@ -78,6 +78,123 @@ function doesFileMatchRegex(filePath: string, pattern: string): boolean {
 	}
 }
 
+/**
+ * Extracts file paths from XML args format: <path>...</path>
+ * Used by both edit and read file regex validation.
+ */
+function extractPathsFromXmlArgs(args: string): string[] {
+	const paths: string[] = []
+	const filePathMatches = args.match(/<path>([^<]+)<\/path>/g)
+	if (filePathMatches) {
+		for (const match of filePathMatches) {
+			const pathMatch = match.match(/<path>([^<]+)<\/path>/)
+			if (pathMatch && pathMatch[1]) {
+				const extractedPath = pathMatch[1].trim()
+				// Validate that the path is not empty and doesn't contain invalid characters
+				if (extractedPath && !extractedPath.includes("<") && !extractedPath.includes(">")) {
+					paths.push(extractedPath)
+				}
+			}
+		}
+	}
+	return paths
+}
+
+function tryParseJson(value: string): unknown | undefined {
+	const trimmed = value.trim()
+	if (!trimmed) {
+		return undefined
+	}
+
+	// Avoid attempting to parse arbitrary strings.
+	if (!(trimmed.startsWith("{") || trimmed.startsWith("[") || trimmed.startsWith('"'))) {
+		return undefined
+	}
+
+	try {
+		return JSON.parse(trimmed) as unknown
+	} catch {
+		return undefined
+	}
+}
+
+function parseJsonStringDeep(value: string, maxDepth = 2): unknown {
+	let current: unknown = value
+	for (let i = 0; i < maxDepth; i++) {
+		if (typeof current !== "string") {
+			break
+		}
+		const parsed = tryParseJson(current)
+		if (parsed === undefined) {
+			break
+		}
+		current = parsed
+	}
+	return current
+}
+
+function extractReadPathsFromFilesValue(value: unknown, paths: string[]): void {
+	if (Array.isArray(value)) {
+		for (const entry of value) {
+			if (typeof entry === "string" && entry.trim().length > 0) {
+				paths.push(entry.trim())
+				continue
+			}
+
+			if (typeof entry === "object" && entry !== null) {
+				const p = (entry as { path?: unknown }).path
+				if (typeof p === "string" && p.trim().length > 0) {
+					paths.push(p.trim())
+				}
+			}
+		}
+		return
+	}
+
+	// Support mis-typed payloads like: files: "[{ \"path\": \"README.md\" }]".
+	if (typeof value === "string") {
+		const parsed = parseJsonStringDeep(value)
+		if (parsed !== value) {
+			extractReadPathsFromFilesValue(parsed, paths)
+		}
+		return
+	}
+
+	// Support nested payloads like: files: "{ \"files\": [...] }".
+	if (typeof value === "object" && value !== null) {
+		const nested = (value as { files?: unknown }).files
+		if (nested !== undefined) {
+			extractReadPathsFromFilesValue(nested, paths)
+		}
+	}
+}
+
+function extractReadFilePaths(toolParams: Record<string, unknown> | undefined): string[] {
+	if (!toolParams) {
+		return []
+	}
+
+	const paths: string[] = []
+
+	// Native protocol read_file: { files: [{ path: string }] }
+	const files = (toolParams as { files?: unknown }).files
+	extractReadPathsFromFilesValue(files, paths)
+
+	// Legacy single-path read_file: { path: string }
+	const legacyPath = (toolParams as { path?: unknown }).path
+	if (typeof legacyPath === "string" && legacyPath.trim().length > 0) {
+		paths.push(legacyPath.trim())
+	}
+
+	// Legacy XML args read_file: { args: "<args><file><path>...</path>...</file></args>" }
+	const args = (toolParams as { args?: unknown }).args
+	if (typeof args === "string") {
+		paths.push(...extractPathsFromXmlArgs(args))
+	}
+
+	return Array.from(new Set(paths))
+}
+
 export function isToolAllowedForMode(
 	tool: string,
 	modeSlug: string,
@@ -165,38 +282,28 @@ export function isToolAllowedForMode(
 			}
 
 			// Handle XML args parameter (used by MULTI_FILE_APPLY_DIFF experiment)
-			if (toolParams?.args && typeof toolParams.args === "string") {
-				// Extract file paths from XML args with improved validation
-				try {
-					const filePathMatches = toolParams.args.match(/<path>([^<]+)<\/path>/g)
-					if (filePathMatches) {
-						for (const match of filePathMatches) {
-							// More robust path extraction with validation
-							const pathMatch = match.match(/<path>([^<]+)<\/path>/)
-							if (pathMatch && pathMatch[1]) {
-								const extractedPath = pathMatch[1].trim()
-								// Validate that the path is not empty and doesn't contain invalid characters
-								if (extractedPath && !extractedPath.includes("<") && !extractedPath.includes(">")) {
-									if (!doesFileMatchRegex(extractedPath, options.fileRegex)) {
-										throw new FileRestrictionError(
-											mode.name,
-											options.fileRegex,
-											options.description,
-											extractedPath,
-											tool,
-										)
-									}
-								}
-							}
+				if (toolParams?.args && typeof toolParams.args === "string") {
+					const xmlPaths = extractPathsFromXmlArgs(toolParams.args)
+					for (const extractedPath of xmlPaths) {
+						if (!doesFileMatchRegex(extractedPath, options.fileRegex)) {
+							throw new FileRestrictionError(
+								mode.name,
+								options.fileRegex,
+								options.description,
+								extractedPath,
+								tool,
+							)
 						}
 					}
-				} catch (error) {
-					// Re-throw FileRestrictionError as it's an expected validation error
-					if (error instanceof FileRestrictionError) {
-						throw error
-					}
-					// If XML parsing fails, log the error but don't block the operation
-					console.warn(`Failed to parse XML args for file restriction validation: ${error}`)
+				}
+		}
+
+		// For the read group, optionally restrict read_file paths if specified
+		if (groupName === "read" && options.fileRegex && tool === "read_file") {
+			const readPaths = extractReadFilePaths(toolParams)
+			for (const p of readPaths) {
+				if (!doesFileMatchRegex(p, options.fileRegex)) {
+					throw new FileRestrictionError(mode.name, options.fileRegex, options.description, p, tool, "read")
 				}
 			}
 		}

src/shared/__tests__/modes.spec.ts

@@ -45,6 +45,52 @@ describe("isToolAllowedForMode", () => {
 		expect(isToolAllowedForMode("browser_action", "markdown-editor", customModes)).toBe(true)
 	})
 
+	describe("read file restrictions", () => {
+		it("allows reading matching files", () => {
+			const customModesWithReadRestriction: ModeConfig[] = [
+				{
+					slug: "md-reader",
+					name: "Markdown Reader",
+					roleDefinition: "You can only read markdown",
+					groups: ["browser", ["read", { fileRegex: "\\.md$" }]],
+				},
+			]
+
+			expect(
+				isToolAllowedForMode("read_file", "md-reader", customModesWithReadRestriction, undefined, {
+					files: [{ path: "README.md" }],
+				}),
+			).toBe(true)
+		})
+
+		it("rejects reading non-matching files", () => {
+			const customModesWithReadRestriction: ModeConfig[] = [
+				{
+					slug: "md-reader",
+					name: "Markdown Reader",
+					roleDefinition: "You can only read markdown",
+					groups: [["read", { fileRegex: "\\.md$", description: "Markdown files only" }]],
+				},
+			]
+
+			expect(() =>
+				isToolAllowedForMode("read_file", "md-reader", customModesWithReadRestriction, undefined, {
+					files: [{ path: "src/index.ts" }],
+				}),
+			).toThrow(FileRestrictionError)
+			expect(() =>
+				isToolAllowedForMode("read_file", "md-reader", customModesWithReadRestriction, undefined, {
+					files: [{ path: "src/index.ts" }],
+				}),
+			).toThrow(/can only read files matching pattern/)
+			expect(() =>
+				isToolAllowedForMode("read_file", "md-reader", customModesWithReadRestriction, undefined, {
+					files: [{ path: "src/index.ts" }],
+				}),
+			).toThrow(/Markdown files only/)
+		})
+	})
+
 	describe("file restrictions", () => {
 		it("allows editing matching files", () => {
 			// Test markdown editor mode
@@ -395,6 +441,13 @@ describe("FileRestrictionError", () => {
 		expect(error.name).toBe("FileRestrictionError")
 	})
 
+	it("formats error message for read operations", () => {
+		const error = new FileRestrictionError("Markdown Reader", "\\.md$", undefined, "test.js", "read_file", "read")
+		expect(error.message).toBe(
+			"Tool 'read_file' in mode 'Markdown Reader' can only read files matching pattern: \\.md$. Got: test.js",
+		)
+	})
+
 	describe("debug mode", () => {
 		it("is configured correctly", () => {
 			const debugMode = modes.find((mode) => mode.slug === "debug")
@@ -412,6 +465,41 @@ describe("FileRestrictionError", () => {
 		})
 	})
 
+	describe("orchestrator mode", () => {
+		it("is configured to only read skill definition files", () => {
+			const orchestratorMode = modes.find((mode) => mode.slug === "orchestrator")
+			expect(orchestratorMode).toBeDefined()
+			expect(orchestratorMode?.groups).toEqual([
+				[
+					"read",
+					{
+						fileRegex:
+							"(^|.*[\\\\/])\\.roo[\\\\/]skills(-[a-zA-Z0-9-]+)?[\\\\/][^\\\\/]+[\\\\/]SKILL\\.md$",
+						description: "Skill definition files only",
+					},
+				],
+			])
+
+			expect(
+				isToolAllowedForMode("read_file", "orchestrator", [], undefined, {
+					files: [{ path: ".roo/skills/example-skill/SKILL.md" }],
+				}),
+			).toBe(true)
+
+			expect(
+				isToolAllowedForMode("read_file", "orchestrator", [], undefined, {
+					files: [{ path: "/Users/test/.roo/skills/example-skill/SKILL.md" }],
+				}),
+			).toBe(true)
+
+			expect(() =>
+				isToolAllowedForMode("read_file", "orchestrator", [], undefined, {
+					files: [{ path: "src/index.ts" }],
+				}),
+			).toThrow(FileRestrictionError)
+		})
+	})
+
 	describe("getFullModeDetails", () => {
 		beforeEach(() => {
 			vi.clearAllMocks()