fix: address review comments for individual tool controls

- 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

Author: roomote

src/core/prompts/system.ts

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

src/core/prompts/tools/__tests__/index.spec.ts

@@ -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([])
+	})
 })

src/core/prompts/tools/index.ts

@@ -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 descriptions = Array.from(tools).map((toolName) => {
-		const descriptionFn = toolDescriptionMap[toolName]
-		if (!descriptionFn) {
-			return undefined
-		}
+	const toolsArray = Array.from(tools)
+	const descriptions = toolsArray
+		.map((toolName) => {
+			const descriptionFn = toolDescriptionMap[toolName]
+			if (!descriptionFn) {
+				return undefined
+			}
 
-		return descriptionFn({
-			...args,
-			toolOptions: undefined, // No tool options in group-based approach
+			return {
+				name: toolName,
+				description: descriptionFn({
+					...args,
+					toolOptions: undefined, // No tool options in group-based approach
+				}),
+			}
 		})
-	})
+		.filter((item) => item && item.description)
+
+	// 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 `# Tools\n\n${descriptions.filter(Boolean).join("\n\n")}`
+	return `# Tools\n\n${enabledDescriptions.join("\n\n")}`
 }
 
 // Export individual description functions for backward compatibility

src/core/webview/generateSystemPrompt.ts

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