Revert "refactor: remove approval flow from skill tool"

This reverts commit 40dd577.

Author: hannesrudolph

src/core/auto-approval/index.ts

@@ -151,6 +151,10 @@ export async function checkAutoApproval({
 			return { decision: "approve" }
 		}
 
+		if (tool.tool === "skill") {
+			return { decision: "approve" }
+		}
+
 		if (tool?.tool === "switchMode") {
 			return state.alwaysAllowModeSwitch === true ? { decision: "approve" } : { decision: "ask" }
 		}

src/core/tools/SkillTool.ts

@@ -1,6 +1,7 @@
 import { Task } from "../task/Task"
 import { formatResponse } from "../prompts/responses"
 import { BaseTool, ToolCallbacks } from "./BaseTool"
+import type { ToolUse } from "../../shared/tools"
 
 interface SkillParams {
 	skill: string
@@ -12,7 +13,7 @@ export class SkillTool extends BaseTool<"skill"> {
 
 	async execute(params: SkillParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
 		const { skill: skillName, args } = params
-		const { handleError, pushToolResult } = callbacks
+		const { askApproval, handleError, pushToolResult } = callbacks
 
 		try {
 			// Validate skill name parameter
@@ -59,7 +60,22 @@ export class SkillTool extends BaseTool<"skill"> {
 				return
 			}
 
-			// Build the result message - no approval needed, skills just execute
+			// Build approval message
+			const toolMessage = JSON.stringify({
+				tool: "skill",
+				skill: skillName,
+				args: args,
+				source: skillContent.source,
+				description: skillContent.description,
+			})
+
+			const didApprove = await askApproval("tool", toolMessage)
+
+			if (!didApprove) {
+				return
+			}
+
+			// Build the result message
 			let result = `Skill: ${skillName}`
 
 			if (skillContent.description) {
@@ -79,7 +95,18 @@ export class SkillTool extends BaseTool<"skill"> {
 		}
 	}
 
-	// No handlePartial - skills execute silently without streaming UI
+	override async handlePartial(task: Task, block: ToolUse<"skill">): Promise<void> {
+		const skillName: string | undefined = block.params.skill
+		const args: string | undefined = block.params.args
+
+		const partialMessage = JSON.stringify({
+			tool: "skill",
+			skill: skillName,
+			args: args,
+		})
+
+		await task.ask("tool", partialMessage, block.partial).catch(() => {})
+	}
 }
 
 export const skillTool = new SkillTool()

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

@@ -22,6 +22,7 @@ describe("skillTool", () => {
 			recordToolError: vi.fn(),
 			didToolFailInCurrentTurn: false,
 			sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
+			ask: vi.fn().mockResolvedValue({}),
 			providerRef: {
 				deref: vi.fn().mockReturnValue({
 					getState: vi.fn().mockResolvedValue({ mode: "code" }),
@@ -31,6 +32,7 @@ describe("skillTool", () => {
 		}
 
 		mockCallbacks = {
+			askApproval: vi.fn().mockResolvedValue(true),
 			handleError: vi.fn(),
 			pushToolResult: vi.fn(),
 		}
@@ -97,7 +99,7 @@ describe("skillTool", () => {
 		)
 	})
 
-	it("should successfully load built-in skill without approval", async () => {
+	it("should successfully load built-in skill", async () => {
 		const block: ToolUse<"skill"> = {
 			type: "tool_use" as const,
 			name: "skill" as const,
@@ -119,7 +121,17 @@ describe("skillTool", () => {
 
 		await skillTool.handle(mockTask as Task, block, mockCallbacks)
 
-		// Skills execute directly without approval
+		expect(mockCallbacks.askApproval).toHaveBeenCalledWith(
+			"tool",
+			JSON.stringify({
+				tool: "skill",
+				skill: "create-mcp-server",
+				args: undefined,
+				source: "built-in",
+				description: "Instructions for creating MCP servers",
+			}),
+		)
+
 		expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
 			`Skill: create-mcp-server
 Description: Instructions for creating MCP servers
@@ -166,6 +178,57 @@ Step 1: Create the server...`,
 		)
 	})
 
+	it("should handle user rejection", async () => {
+		const block: ToolUse<"skill"> = {
+			type: "tool_use" as const,
+			name: "skill" as const,
+			params: {},
+			partial: false,
+			nativeArgs: {
+				skill: "create-mcp-server",
+			},
+		}
+
+		mockSkillsManager.getSkillContent.mockResolvedValue({
+			name: "create-mcp-server",
+			description: "Test",
+			source: "built-in",
+			instructions: "Test instructions",
+		})
+
+		mockCallbacks.askApproval.mockResolvedValue(false)
+
+		await skillTool.handle(mockTask as Task, block, mockCallbacks)
+
+		expect(mockCallbacks.pushToolResult).not.toHaveBeenCalled()
+	})
+
+	it("should handle partial block", async () => {
+		const block: ToolUse<"skill"> = {
+			type: "tool_use" as const,
+			name: "skill" as const,
+			params: {
+				skill: "create-mcp-server",
+				args: "",
+			},
+			partial: true,
+		}
+
+		await skillTool.handle(mockTask as Task, block, mockCallbacks)
+
+		expect(mockTask.ask).toHaveBeenCalledWith(
+			"tool",
+			JSON.stringify({
+				tool: "skill",
+				skill: "create-mcp-server",
+				args: "",
+			}),
+			true,
+		)
+
+		expect(mockCallbacks.pushToolResult).not.toHaveBeenCalled()
+	})
+
 	it("should handle errors during execution", async () => {
 		const block: ToolUse<"skill"> = {
 			type: "tool_use" as const,
@@ -236,7 +299,7 @@ Step 1: Create the server...`,
 		)
 	})
 
-	it("should load project skill without approval", async () => {
+	it("should load project skill", async () => {
 		const block: ToolUse<"skill"> = {
 			type: "tool_use" as const,
 			name: "skill" as const,
@@ -258,7 +321,17 @@ Step 1: Create the server...`,
 
 		await skillTool.handle(mockTask as Task, block, mockCallbacks)
 
-		// Skills execute directly without approval
+		expect(mockCallbacks.askApproval).toHaveBeenCalledWith(
+			"tool",
+			JSON.stringify({
+				tool: "skill",
+				skill: "my-project-skill",
+				args: undefined,
+				source: "project",
+				description: "A custom project skill",
+			}),
+		)
+
 		expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
 			`Skill: my-project-skill
 Description: A custom project skill

webview-ui/src/components/chat/ChatRow.tsx

@@ -666,6 +666,75 @@ export const ChatRowContent = ({
 						</div>
 					</>
 				)
+			case "skill": {
+				const skillInfo = tool
+				return (
+					<>
+						<div style={headerStyle}>
+							{toolIcon("book")}
+							<span style={{ fontWeight: "bold" }}>
+								{message.type === "ask" ? t("chat:skill.wantsToLoad") : t("chat:skill.didLoad")}
+							</span>
+						</div>
+						<div
+							style={{
+								marginTop: "4px",
+								backgroundColor: "var(--vscode-editor-background)",
+								border: "1px solid var(--vscode-editorGroup-border)",
+								borderRadius: "4px",
+								overflow: "hidden",
+								cursor: "pointer",
+							}}
+							onClick={handleToggleExpand}>
+							<ToolUseBlockHeader
+								className="group"
+								style={{
+									display: "flex",
+									alignItems: "center",
+									justifyContent: "space-between",
+									padding: "10px 12px",
+								}}>
+								<div style={{ display: "flex", alignItems: "center", gap: "8px" }}>
+									<span style={{ fontWeight: "500", fontSize: "var(--vscode-font-size)" }}>
+										{skillInfo.skill}
+									</span>
+									{skillInfo.source && (
+										<VSCodeBadge style={{ fontSize: "calc(var(--vscode-font-size) - 2px)" }}>
+											{skillInfo.source}
+										</VSCodeBadge>
+									)}
+								</div>
+								<span
+									className={`codicon codicon-chevron-${isExpanded ? "up" : "down"} opacity-0 group-hover:opacity-100 transition-opacity duration-200`}></span>
+							</ToolUseBlockHeader>
+							{isExpanded && (skillInfo.args || skillInfo.description) && (
+								<div
+									style={{
+										padding: "12px 16px",
+										borderTop: "1px solid var(--vscode-editorGroup-border)",
+										display: "flex",
+										flexDirection: "column",
+										gap: "8px",
+									}}>
+									{skillInfo.description && (
+										<div style={{ color: "var(--vscode-descriptionForeground)" }}>
+											{skillInfo.description}
+										</div>
+									)}
+									{skillInfo.args && (
+										<div>
+											<span style={{ fontWeight: "500" }}>Arguments: </span>
+											<span style={{ color: "var(--vscode-descriptionForeground)" }}>
+												{skillInfo.args}
+											</span>
+										</div>
+									)}
+								</div>
+							)}
+						</div>
+					</>
+				)
+			}
 			case "listFilesTopLevel":
 				return (
 					<>