From ceb026efd92ba558d1b05aea44c7956ca98c4e3d Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Tue, 30 Dec 2025 15:06:31 -0700 Subject: [PATCH] feat(modes): support fileRegex for read_file and allow orchestrator skill reads --- packages/types/src/mode.ts | 10 ++- .../presentAssistantMessage.ts | 8 +- .../tools/__tests__/validateToolUse.spec.ts | 21 +++++ src/core/tools/validateToolUse.ts | 56 +++++++++++++ src/shared/__tests__/modes.spec.ts | 81 +++++++++++++++++++ src/shared/modes.ts | 12 ++- 6 files changed, 184 insertions(+), 4 deletions(-) diff --git a/packages/types/src/mode.ts b/packages/types/src/mode.ts index c02c47c134..748c89b5bc 100644 --- a/packages/types/src/mode.ts +++ b/packages/types/src/mode.ts @@ -188,7 +188,15 @@ 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", + { + 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.", }, diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 9b1a5fcffb..0367e71ce3 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/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, ) diff --git a/src/core/tools/__tests__/validateToolUse.spec.ts b/src/core/tools/__tests__/validateToolUse.spec.ts index 87aa159420..815db31a37 100644 --- a/src/core/tools/__tests__/validateToolUse.spec.ts +++ b/src/core/tools/__tests__/validateToolUse.spec.ts @@ -185,6 +185,27 @@ 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("throws error when tool requirement is not met", () => { const requirements = { apply_diff: false } expect(() => validateToolUse("apply_diff", codeMode, [], requirements)).toThrow( diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 751d164fd2..995ffc91f1 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -78,6 +78,52 @@ function doesFileMatchRegex(filePath: string, pattern: string): boolean { } } +function extractReadFilePaths(toolParams: Record | undefined): string[] { + if (!toolParams) { + return [] + } + + const paths: string[] = [] + + // Native protocol read_file: { files: [{ path: string }] } + const files = (toolParams as { files?: unknown }).files + if (Array.isArray(files)) { + for (const entry of files) { + 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()) + } + } + } + } + + // 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: "......" } + const args = (toolParams as { args?: unknown }).args + if (typeof args === "string") { + const filePathMatches = args.match(/([^<]+)<\/path>/g) + if (filePathMatches) { + for (const match of filePathMatches) { + const pathMatch = match.match(/([^<]+)<\/path>/) + if (pathMatch && pathMatch[1]) { + const extractedPath = pathMatch[1].trim() + if (extractedPath && !extractedPath.includes("<") && !extractedPath.includes(">")) { + paths.push(extractedPath) + } + } + } + } + } + + return Array.from(new Set(paths)) +} + export function isToolAllowedForMode( tool: string, modeSlug: string, @@ -201,6 +247,16 @@ export function isToolAllowedForMode( } } + // 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") + } + } + } + return true } diff --git a/src/shared/__tests__/modes.spec.ts b/src/shared/__tests__/modes.spec.ts index a00abde787..c32815b0e0 100644 --- a/src/shared/__tests__/modes.spec.ts +++ b/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,34 @@ 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: "src/index.ts" }], + }), + ).toThrow(FileRestrictionError) + }) + }) + describe("getFullModeDetails", () => { beforeEach(() => { vi.clearAllMocks() diff --git a/src/shared/modes.ts b/src/shared/modes.ts index a94aa47ed0..daea67fbe2 100644 --- a/src/shared/modes.ts +++ b/src/shared/modes.ts @@ -133,10 +133,18 @@ export function getModeSelection(mode: string, promptComponent?: PromptComponent // Custom error class for file restrictions export class FileRestrictionError extends Error { - constructor(mode: string, pattern: string, description: string | undefined, filePath: string, tool?: string) { + constructor( + mode: string, + pattern: string, + description: string | undefined, + filePath: string, + tool?: string, + operation: "read" | "edit" = "edit", + ) { const toolInfo = tool ? `Tool '${tool}' in mode '${mode}'` : `This mode (${mode})` + const verb = operation === "read" ? "read" : "edit" super( - `${toolInfo} can only edit files matching pattern: ${pattern}${description ? ` (${description})` : ""}. Got: ${filePath}`, + `${toolInfo} can only ${verb} files matching pattern: ${pattern}${description ? ` (${description})` : ""}. Got: ${filePath}`, ) this.name = "FileRestrictionError" }