scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-05 08:10:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: address review comments for individual tool controls

Browse source 
- Filter tools at the very end of getToolDescriptionsForMode()
- Use settings.disabledTools instead of separate parameter
- Use translations for tool display names
- Allow disabling all tools (not restricted to non-always-available)
- Make tool list dynamic based on packages/types/src/tool.ts
- Add tests to validate all tools are represented
This commit is contained in:
Roo Code 2025-07-20 04:13:02 +00:00
parent 18fe0d7972
commit a18cf17894
6 changed files with 134 additions and 159 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/core/prompts/system.ts
View file

@ -98,7 +98,6 @@ ${getToolDescriptionsForMode(
experiments,
partialReadsEnabled,
settings,
settings?.disabledTools,
)}
${getToolUseGuidelinesSection(codeIndexManager)}

105
src/core/prompts/tools/__tests__/index.spec.ts
View file

@ -3,6 +3,8 @@
import { describe, it, expect } from "vitest"
import { getToolDescriptionsForMode } from "../index"
import { defaultModeSlug } from "../../../../shared/modes"
import { toolNames } from "@roo-code/types"
import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS } from "../../../../shared/tools"
describe("getToolDescriptionsForMode", () => {
const mockCwd = "/test/path"
@ -21,7 +23,6 @@ describe("getToolDescriptionsForMode", () => {
undefined, // experiments
undefined, // partialReadsEnabled
undefined, // settings
undefined, // disabledTools
)
expect(result).toBeTruthy()
@ -47,8 +48,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with disabledTools
)
// Check that disabled tools are not included
@ -60,7 +60,7 @@ describe("getToolDescriptionsForMode", () => {
expect(result).toContain("## search_files")
})
it("should not filter out always-available tools even if disabled", () => {
it("should filter out all tools including always-available tools when disabled", () => {
const disabledTools = ["ask_followup_question", "attempt_completion"]
const result = getToolDescriptionsForMode(
defaultModeSlug,
@ -73,13 +73,12 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with disabledTools
)
// These tools should always be available
expect(result).toContain("## ask_followup_question")
expect(result).toContain("## attempt_completion")
// These tools should be filtered out since we now allow disabling all tools
expect(result).not.toContain("## ask_followup_question")
expect(result).not.toContain("## attempt_completion")
})
it("should handle empty disabled tools array", () => {
@ -95,8 +94,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with empty disabledTools
)
const resultWithoutDisabled = getToolDescriptionsForMode(
defaultModeSlug,
@ -109,8 +107,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
undefined,
undefined, // no settings
)
// Should return the same tools
@ -129,8 +126,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
undefined,
{ disabledTools: undefined }, // settings with undefined disabledTools
)
const resultWithoutDisabled = getToolDescriptionsForMode(
defaultModeSlug,
@ -143,8 +139,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
undefined,
undefined, // no settings
)
// Should return the same tools
@ -164,8 +159,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with disabledTools
)
// Check that all disabled tools are filtered out
@ -175,8 +169,8 @@ describe("getToolDescriptionsForMode", () => {
// Check that some other tools are still included
expect(result).toContain("# Tools")
// Always available tools should still be there
expect(result).toContain("## ask_followup_question")
// Other tools that weren't disabled should still be there
expect(result).toContain("## list_code_definition_names")
})
it("should handle invalid tool names in disabled list", () => {
@ -192,8 +186,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with disabledTools
)
// Should still filter out valid disabled tools
@ -217,8 +210,7 @@ describe("getToolDescriptionsForMode", () => {
undefined,
undefined,
undefined,
undefined,
disabledTools,
{ disabledTools }, // settings object with disabledTools
)
// execute_command should be filtered out
@ -228,4 +220,67 @@ describe("getToolDescriptionsForMode", () => {
expect(result).toContain("## read_file")
expect(result).toContain("## write_to_file")
})
it("should have all tools from packages/types/src/tool.ts represented in TOOL_GROUPS or ALWAYS_AVAILABLE_TOOLS", () => {
// Get all tools from TOOL_GROUPS
const toolsInGroups = new Set<string>()
Object.values(TOOL_GROUPS).forEach((group) => {
group.tools.forEach((tool) => toolsInGroups.add(tool))
})
// Get all tools from ALWAYS_AVAILABLE_TOOLS
const alwaysAvailableSet = new Set(ALWAYS_AVAILABLE_TOOLS)
// Combine both sets
const allRepresentedTools = new Set([...toolsInGroups, ...alwaysAvailableSet])
// Check that every tool from toolNames is represented
const missingTools: string[] = []
toolNames.forEach((toolName) => {
if (!allRepresentedTools.has(toolName)) {
missingTools.push(toolName)
}
})
// Assert that there are no missing tools
expect(missingTools).toEqual([])
})
it("should not have tools in TOOL_GROUPS that are not in packages/types/src/tool.ts", () => {
// Get all tools from TOOL_GROUPS
const toolsInGroups = new Set<string>()
Object.values(TOOL_GROUPS).forEach((group) => {
group.tools.forEach((tool) => toolsInGroups.add(tool))
})
// Convert toolNames to a Set for easier lookup
const validToolNames = new Set(toolNames)
// Check that every tool in TOOL_GROUPS exists in toolNames
const invalidTools: string[] = []
toolsInGroups.forEach((tool) => {
if (!validToolNames.has(tool as any)) {
invalidTools.push(tool)
}
})
// Assert that there are no invalid tools
expect(invalidTools).toEqual([])
})
it("should not have tools in ALWAYS_AVAILABLE_TOOLS that are not in packages/types/src/tool.ts", () => {
// Convert toolNames to a Set for easier lookup
const validToolNames = new Set(toolNames)
// Check that every tool in ALWAYS_AVAILABLE_TOOLS exists in toolNames
const invalidTools: string[] = []
ALWAYS_AVAILABLE_TOOLS.forEach((tool) => {
if (!validToolNames.has(tool)) {
invalidTools.push(tool)
}
})
// Assert that there are no invalid tools
expect(invalidTools).toEqual([])
})
})

42
src/core/prompts/tools/index.ts
View file

@ -61,7 +61,6 @@ export function getToolDescriptionsForMode(
experiments?: Record<string, boolean>,
partialReadsEnabled?: boolean,
settings?: Record<string, any>,
disabledTools?: string[],
): string {
const config = getModeConfig(mode, customModes)
const args: ToolArgs = {
@ -110,30 +109,31 @@ export function getToolDescriptionsForMode(
tools.delete("codebase_search")
}
// Filter out disabled tools (except always-available tools)
if (disabledTools && disabledTools.length > 0) {
disabledTools.forEach((tool) => {
// Don't filter out always-available tools
if (!ALWAYS_AVAILABLE_TOOLS.includes(tool as any)) {
tools.delete(tool)
// Map tool descriptions for allowed tools
const toolsArray = Array.from(tools)
const descriptions = toolsArray
.map((toolName) => {
const descriptionFn = toolDescriptionMap[toolName]
if (!descriptionFn) {
return undefined
}
return {
name: toolName,
description: descriptionFn({
...args,
toolOptions: undefined, // No tool options in group-based approach
}),
}
})
}
.filter((item) => item && item.description)
// Map tool descriptions for allowed tools
const descriptions = Array.from(tools).map((toolName) => {
const descriptionFn = toolDescriptionMap[toolName]
if (!descriptionFn) {
return undefined
}
// Filter out disabled tools at the very end using the simplest possible implementation
const enabledDescriptions = descriptions
.filter((item) => !settings?.disabledTools?.includes(item!.name))
.map((item) => item!.description)
return descriptionFn({
...args,
toolOptions: undefined, // No tool options in group-based approach
})
})
return `# Tools\n\n${descriptions.filter(Boolean).join("\n\n")}`
return `# Tools\n\n${enabledDescriptions.join("\n\n")}`
}
// Export individual description functions for backward compatibility

3
src/core/webview/generateSystemPrompt.ts
View file

@ -24,7 +24,6 @@ export const generateSystemPrompt = async (provider: ClineProvider, message: Web
language,
maxReadFileLine,
maxConcurrentFileReads,
disabledTools,
} = await provider.getState()
// Check experiment to determine which diff strategy to use
@ -83,7 +82,7 @@ export const generateSystemPrompt = async (provider: ClineProvider, message: Web
maxReadFileLine !== -1,
{
maxConcurrentFileReads,
disabledTools,
disabledTools: (await provider.getState()).disabledTools,
},
)

119
webview-ui/src/components/settings/ToolSettings.tsx
View file

@ -3,6 +3,7 @@ import { Wrench } from "lucide-react"
import { HTMLAttributes, useMemo } from "react"
import { useAppTranslation } from "@/i18n/TranslationContext"
import { toolNames } from "@roo-code/types"
import { Section } from "./Section"
import { SectionHeader } from "./SectionHeader"
@ -13,65 +14,6 @@ type ToolSettingsProps = HTMLAttributes<HTMLDivElement> & {
setCachedStateField: SetCachedStateField<"disabledTools">
}
// Import the constants from shared/tools.ts
// Tool display names mapping
const TOOL_DISPLAY_NAMES: Record<string, string> = {
execute_command: "Run commands",
read_file: "Read files",
fetch_instructions: "Fetch instructions",
write_to_file: "Write files",
apply_diff: "Apply changes",
search_files: "Search files",
list_files: "List files",
list_code_definition_names: "List definitions",
browser_action: "Use a browser",
use_mcp_tool: "Use MCP tools",
access_mcp_resource: "Access MCP resources",
ask_followup_question: "Ask questions",
attempt_completion: "Complete tasks",
switch_mode: "Switch modes",
new_task: "Create new task",
insert_content: "Insert content",
search_and_replace: "Search and replace",
codebase_search: "Codebase search",
update_todo_list: "Update todo list",
}
// Tools that are always available and cannot be disabled
const ALWAYS_AVAILABLE_TOOLS = [
"ask_followup_question",
"attempt_completion",
"switch_mode",
"new_task",
"update_todo_list",
]
// Tool groups configuration
const TOOL_GROUPS: Record<string, { tools: readonly string[] }> = {
read: {
tools: [
"read_file",
"fetch_instructions",
"search_files",
"list_files",
"list_code_definition_names",
"codebase_search",
],
},
edit: {
tools: ["apply_diff", "write_to_file", "insert_content", "search_and_replace"],
},
browser: {
tools: ["browser_action"],
},
command: {
tools: ["execute_command"],
},
mcp: {
tools: ["use_mcp_tool", "access_mcp_resource"],
},
}
export const ToolSettings = ({ disabledTools = [], setCachedStateField, ...props }: ToolSettingsProps) => {
const { t } = useAppTranslation()
@ -90,34 +32,15 @@ export const ToolSettings = ({ disabledTools = [], setCachedStateField, ...props
const isToolEnabled = (toolName: string) => !disabledTools.includes(toolName)
// Get all available tools dynamically from the global tools configuration
// Get all available tools dynamically from packages/types/src/tool.ts
const allTools = useMemo(() => {
const tools = new Set<string>()
// Add all tools from tool groups
Object.values(TOOL_GROUPS).forEach((group) => {
group.tools.forEach((tool) => tools.add(tool))
})
// Add always available tools
ALWAYS_AVAILABLE_TOOLS.forEach((tool) => tools.add(tool))
// Convert to array and sort alphabetically
return Array.from(tools).sort((a, b) => {
const nameA = TOOL_DISPLAY_NAMES[a as keyof typeof TOOL_DISPLAY_NAMES] || a
const nameB = TOOL_DISPLAY_NAMES[b as keyof typeof TOOL_DISPLAY_NAMES] || b
// Use the toolNames array from @roo-code/types and sort alphabetically by translated names
return [...toolNames].sort((a, b) => {
const nameA = t(`settings:tools.names.${a}`) || a
const nameB = t(`settings:tools.names.${b}`) || b
return nameA.localeCompare(nameB)
})
}, [])
// Separate tools into disableable and always-available
const disableableTools = useMemo(() => {
return allTools.filter((tool) => !ALWAYS_AVAILABLE_TOOLS.includes(tool as any))
}, [allTools])
const alwaysAvailableTools = useMemo(() => {
return allTools.filter((tool) => ALWAYS_AVAILABLE_TOOLS.includes(tool as any))
}, [allTools])
}, [t])
return (
<div {...props}>
@ -132,38 +55,16 @@ export const ToolSettings = ({ disabledTools = [], setCachedStateField, ...props
<div className="text-vscode-descriptionForeground text-sm mb-3">{t("settings:tools.description")}</div>
<div className="space-y-2">
{/* Disableable tools */}
{disableableTools.map((tool) => (
{/* All tools can be toggled */}
{allTools.map((tool) => (
<VSCodeCheckbox
key={tool}
checked={isToolEnabled(tool)}
onChange={(e: any) => handleToolToggle(tool, e.target.checked)}>
<span className="text-sm">
{TOOL_DISPLAY_NAMES[tool as keyof typeof TOOL_DISPLAY_NAMES] || tool}
</span>
</VSCodeCheckbox>
))}
{/* Separator */}
{alwaysAvailableTools.length > 0 && (
<div className="border-t border-vscode-panel-border my-3 pt-3">
<div className="text-vscode-descriptionForeground text-xs mb-2">
{t("settings:tools.alwaysAvailable")}
</div>
</div>
)}
{/* Always available tools (disabled checkboxes) */}
{alwaysAvailableTools.map((tool) => (
<VSCodeCheckbox key={tool} checked={true} disabled={true}>
<span className="text-sm opacity-75">
{TOOL_DISPLAY_NAMES[tool as keyof typeof TOOL_DISPLAY_NAMES] || tool}
</span>
<span className="text-sm">{t(`settings:tools.names.${tool}`) || tool}</span>
</VSCodeCheckbox>
))}
</div>
<div className="text-vscode-descriptionForeground text-xs mt-3">{t("settings:tools.note")}</div>
</Section>
</div>
)

23
webview-ui/src/i18n/locales/en/settings.json
View file

@ -723,6 +723,27 @@
"tools": {
"description": "Disable specific tools to reduce token usage. Disabled tools won't be included in the system prompt.",
"note": "Note: Some tools are always available and cannot be disabled (ask questions, complete tasks, switch modes, create new task, update todo list).",
"alwaysAvailable": "Always available tools"
"alwaysAvailable": "Always available tools",
"names": {
"execute_command": "Run commands",
"read_file": "Read files",
"fetch_instructions": "Fetch instructions",
"write_to_file": "Write files",
"apply_diff": "Apply changes",
"search_files": "Search files",
"list_files": "List files",
"list_code_definition_names": "List definitions",
"browser_action": "Use a browser",
"use_mcp_tool": "Use MCP tools",
"access_mcp_resource": "Access MCP resources",
"ask_followup_question": "Ask questions",
"attempt_completion": "Complete tasks",
"switch_mode": "Switch modes",
"new_task": "Create new task",
"insert_content": "Insert content",
"search_and_replace": "Search and replace",
"codebase_search": "Codebase search",
"update_todo_list": "Update todo list"
}
}
}