Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
590 changes: 295 additions & 295 deletions src/core/prompts/__tests__/__snapshots__/system.test.ts.snap

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions src/exports/roo-code.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,12 @@ type GlobalSettings = {
roleDefinition: string
customInstructions?: string | undefined
groups: (
| ("read" | "edit" | "browser" | "command" | "mcp" | "modes")
| ("read" | "edit" | "browser" | "command" | "mcp" | "subtask" | "switch" | "followup")
| [
"read" | "edit" | "browser" | "command" | "mcp" | "modes",
"read" | "edit" | "browser" | "command" | "mcp" | "subtask" | "switch" | "followup",
{
fileRegex?: string | undefined
slugRegex?: string | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if I change my mind after reading the whole diff, but I think an array of slugs might make more sense than a regex here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, still think an array of slugs would be easier to manage.

description?: string | undefined
},
]
Expand Down
5 changes: 3 additions & 2 deletions src/exports/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,11 +264,12 @@ type GlobalSettings = {
roleDefinition: string
customInstructions?: string | undefined
groups: (
| ("read" | "edit" | "browser" | "command" | "mcp" | "modes")
| ("read" | "edit" | "browser" | "command" | "mcp" | "subtask" | "switch" | "followup")
| [
"read" | "edit" | "browser" | "command" | "mcp" | "modes",
"read" | "edit" | "browser" | "command" | "mcp" | "subtask" | "switch" | "followup",
{
fileRegex?: string | undefined
slugRegex?: string | undefined
description?: string | undefined
},
]
Expand Down
19 changes: 18 additions & 1 deletion src/schemas/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export type ProviderName = z.infer<typeof providerNamesSchema>
* ToolGroup
*/

export const toolGroups = ["read", "edit", "browser", "command", "mcp", "modes"] as const
export const toolGroups = ["read", "edit", "browser", "command", "mcp", "subtask", "switch", "followup"] as const

export const toolGroupsSchema = z.enum(toolGroups)

Expand Down Expand Up @@ -186,6 +186,23 @@ export const groupOptionsSchema = z.object({
},
{ message: "Invalid regular expression pattern" },
),
slugRegex: z
.string()
.optional()
.refine(
(pattern) => {
if (!pattern) return true // Optional, valid if not provided
try {
new RegExp(pattern)
return true
} catch {
return false
}
},
// Use i18n for the message
// Using plain string due to persistent TS errors with t() here.
{ message: "Invalid regular expression pattern for slugRegex" },
),
description: z.string().optional(),
})

Expand Down
216 changes: 214 additions & 2 deletions src/shared/__tests__/modes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ describe("isToolAllowedForMode", () => {
]

it("allows always available tools", () => {
expect(isToolAllowedForMode("ask_followup_question", "markdown-editor", customModes)).toBe(true)
// Only attempt_completion is always available now
expect(isToolAllowedForMode("attempt_completion", "markdown-editor", customModes)).toBe(true)
// ask_followup_question is now in the 'followup' group and needs to be explicitly added to a mode
expect(isToolAllowedForMode("ask_followup_question", "markdown-editor", customModes)).toBe(false)
})

it("allows unrestricted tools", () => {
Expand Down Expand Up @@ -242,6 +244,118 @@ describe("isToolAllowedForMode", () => {
expect(isToolAllowedForMode("use_mcp_tool", "architect", [])).toBe(true)
})
})
describe("slugRegex restrictions", () => {
const modesWithSlugRegex: ModeConfig[] = [
{
slug: "subtask-runner",
name: "Subtask Runner",
roleDefinition: "Runs subtasks",
groups: [["subtask", { slugRegex: "^subtask-target-.*" }]],
},
{
slug: "mode-switcher",
name: "Mode Switcher",
roleDefinition: "Switches modes",
groups: [["switch", { slugRegex: "^switch-target-.*" }]],
},
{
slug: "subtask-no-regex",
name: "Subtask No Regex",
roleDefinition: "Runs any subtask",
groups: ["subtask"], // No slugRegex defined
},
{
slug: "switch-no-regex",
name: "Switch No Regex",
roleDefinition: "Switches to any mode",
groups: ["switch"], // No slugRegex defined
},
]

// --- subtask tool tests ---
it("subtask: allows when slugRegex matches toolParams.mode", () => {
expect(
isToolAllowedForMode("new_task", "subtask-runner", modesWithSlugRegex, undefined, {
mode: "subtask-target-allowed",
message: "test",
}),
).toBe(true) // Should FAIL until implemented
})

it("subtask: rejects when slugRegex does not match toolParams.mode", () => {
expect(
isToolAllowedForMode("new_task", "subtask-runner", modesWithSlugRegex, undefined, {
mode: "other-mode",
message: "test",
}),
).toBe(false) // Should PASS (as false is default), but confirms logic path
})

it("subtask: allows when slugRegex matches toolParams.mode_slug (legacy)", () => {
// Test legacy parameter name if needed
expect(
isToolAllowedForMode("new_task", "subtask-runner", modesWithSlugRegex, undefined, {
mode_slug: "subtask-target-allowed",
message: "test",
}),
).toBe(true) // Should FAIL until implemented
})

it("subtask: rejects when slugRegex does not match toolParams.mode_slug (legacy)", () => {
expect(
isToolAllowedForMode("new_task", "subtask-runner", modesWithSlugRegex, undefined, {
mode_slug: "other-mode",
message: "test",
}),
).toBe(false) // Should PASS (as false is default), but confirms logic path
})

it("subtask: allows when group exists but no slugRegex is defined", () => {
expect(
isToolAllowedForMode("new_task", "subtask-no-regex", modesWithSlugRegex, undefined, {
mode: "any-mode",
message: "test",
}),
).toBe(true) // Should PASS (current behavior)
})

it("subtask: allows when slugRegex exists but toolParams are missing", () => {
// If toolParams are missing, the regex check shouldn't run/fail
expect(isToolAllowedForMode("new_task", "subtask-runner", modesWithSlugRegex)).toBe(true) // Should PASS (current behavior)
})

// --- switch_mode tool tests ---
it("switch: allows when slugRegex matches toolParams.mode_slug", () => {
expect(
isToolAllowedForMode("switch_mode", "mode-switcher", modesWithSlugRegex, undefined, {
mode_slug: "switch-target-allowed",
}),
).toBe(true) // Should FAIL until implemented
})

it("switch: rejects when slugRegex does not match toolParams.mode_slug", () => {
expect(
isToolAllowedForMode("switch_mode", "mode-switcher", modesWithSlugRegex, undefined, {
mode_slug: "other-mode",
}),
).toBe(false) // Should PASS (as false is default), but confirms logic path
})

// Note: switch_mode only uses mode_slug, no need to test 'mode' param

it("switch: allows when group exists but no slugRegex is defined", () => {
expect(
isToolAllowedForMode("switch_mode", "switch-no-regex", modesWithSlugRegex, undefined, {
mode_slug: "any-mode",
}),
).toBe(true) // Should PASS (current behavior)
})

it("switch: allows when slugRegex exists but toolParams are missing", () => {
// If toolParams are missing, the regex check shouldn't run/fail
expect(isToolAllowedForMode("switch_mode", "mode-switcher", modesWithSlugRegex)).toBe(true) // Should PASS (current behavior)
})
})

it("handles non-existent modes", () => {
expect(isToolAllowedForMode("write_to_file", "non-existent", customModes)).toBe(false)
Expand All @@ -256,6 +370,104 @@ describe("isToolAllowedForMode", () => {
})
})

// New tests for default built-in mode permissions
describe("default permissions for built-in modes", () => {
const builtInModes: ModeConfig[] = [] // Use empty array as customModes to test against built-in definitions
const modesToTest = ["architect", "ask", "debug", "orchestrator"]
const toolsToCheck = ["new_task", "switch_mode", "ask_followup_question"]

modesToTest.forEach((modeSlug) => {
describe(`mode: ${modeSlug}`, () => {
toolsToCheck.forEach((toolName) => {
/**
* @test {isToolAllowedForMode} Verifies default tool permissions for built-in modes.
* These tests are expected to FAIL until the corresponding groups ('subtask', 'switch', 'followup')
* are added to the built-in mode definitions in src/shared/modes.ts.
*/
it(`should allow tool: ${toolName}`, () => {
// We expect this to be true eventually, but it should fail initially
expect(isToolAllowedForMode(toolName as any, modeSlug, builtInModes)).toBe(true)
})
})
})
})
})
// End of new tests
describe('when tool group is "subtask"', () => {
const toolName = "new_task"
const requiredGroup = "subtask"

it('should return true if mode has the "subtask" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode", // roleDefinition is required by ModeConfig
groups: [requiredGroup], // Use groups array
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(true)
})

it('should return false if mode does not have the "subtask" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: [], // Do not include the required group
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(false)
})
})

describe('when tool group is "switch"', () => {
const toolName = "switch_mode"
const requiredGroup = "switch"

it('should return true if mode has the "switch" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: [requiredGroup],
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(true)
})

it('should return false if mode does not have the "switch" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: [],
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(false)
})
})

describe('when tool group is "followup"', () => {
const toolName = "ask_followup_question"
const requiredGroup = "followup"

it('should return true if mode has the "followup" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: [requiredGroup],
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(true)
})

it('should return false if mode does not have the "followup" group', () => {
const mode: ModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: [],
}
expect(isToolAllowedForMode(toolName, mode.slug, [mode])).toBe(false)
})
})

describe("FileRestrictionError", () => {
it("formats error message with pattern when no description provided", () => {
const error = new FileRestrictionError("Markdown Editor", "\\.md$", undefined, "test.js")
Expand All @@ -274,7 +486,7 @@ describe("FileRestrictionError", () => {
name: "🪲 Debug",
roleDefinition:
"You are Roo, an expert software debugger specializing in systematic problem diagnosis and resolution.",
groups: ["read", "edit", "browser", "command", "mcp"],
groups: ["read", "edit", "browser", "command", "mcp", "subtask", "switch", "followup"],
})
expect(debugMode?.customInstructions).toContain(
"Reflect on 5-7 different possible sources of the problem, distill those down to 1-2 most likely sources, and then add logs to validate your assumptions. Explicitly ask the user to confirm the diagnosis before fixing the problem.",
Expand Down
Loading