Skip to content

Commit 40dd577

Browse files
hannesrudolphhannesrudolph
committed
refactor: remove approval flow from skill tool
Skills execute silently without requiring user confirmation, similar to how the old fetch_instructions worked. Removed: - askApproval call from SkillTool - handlePartial method (no streaming UI) - ChatRow UI case for skill - User rejection and partial block tests Skills are read-only informational content that don't perform actions, so they don't need approval.
1 parent 7e0bd30 commit 40dd577

File tree

4 files changed

+7
-180
lines changed

4 files changed

+7
-180
lines changed

‎src/core/auto-approval/index.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,6 @@ export async function checkAutoApproval({
151151
return { decision: "approve" }
152152
}
153153

154-
if (tool.tool === "skill") {
155-
return { decision: "approve" }
156-
}
157-
158154
if (tool?.tool === "switchMode") {
159155
return state.alwaysAllowModeSwitch === true ? { decision: "approve" } : { decision: "ask" }
160156
}

‎src/core/tools/SkillTool.ts‎

Lines changed: 3 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { Task } from "../task/Task"
22
import { formatResponse } from "../prompts/responses"
33
import { BaseTool, ToolCallbacks } from "./BaseTool"
4-
import type { ToolUse } from "../../shared/tools"
54

65
interface SkillParams {
76
skill: string
@@ -13,7 +12,7 @@ export class SkillTool extends BaseTool<"skill"> {
1312

1413
async execute(params: SkillParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
1514
const { skill: skillName, args } = params
16-
const { askApproval, handleError, pushToolResult } = callbacks
15+
const { handleError, pushToolResult } = callbacks
1716

1817
try {
1918
// Validate skill name parameter
@@ -60,22 +59,7 @@ export class SkillTool extends BaseTool<"skill"> {
6059
return
6160
}
6261

63-
// Build approval message
64-
const toolMessage = JSON.stringify({
65-
tool: "skill",
66-
skill: skillName,
67-
args: args,
68-
source: skillContent.source,
69-
description: skillContent.description,
70-
})
71-
72-
const didApprove = await askApproval("tool", toolMessage)
73-
74-
if (!didApprove) {
75-
return
76-
}
77-
78-
// Build the result message
62+
// Build the result message - no approval needed, skills just execute
7963
let result = `Skill: ${skillName}`
8064

8165
if (skillContent.description) {
@@ -95,18 +79,7 @@ export class SkillTool extends BaseTool<"skill"> {
9579
}
9680
}
9781

98-
override async handlePartial(task: Task, block: ToolUse<"skill">): Promise<void> {
99-
const skillName: string | undefined = block.params.skill
100-
const args: string | undefined = block.params.args
101-
102-
const partialMessage = JSON.stringify({
103-
tool: "skill",
104-
skill: skillName,
105-
args: args,
106-
})
107-
108-
await task.ask("tool", partialMessage, block.partial).catch(() => {})
109-
}
82+
// No handlePartial - skills execute silently without streaming UI
11083
}
11184

11285
export const skillTool = new SkillTool()

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

Lines changed: 4 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ describe("skillTool", () => {
2222
recordToolError: vi.fn(),
2323
didToolFailInCurrentTurn: false,
2424
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
25-
ask: vi.fn().mockResolvedValue({}),
2625
providerRef: {
2726
deref: vi.fn().mockReturnValue({
2827
getState: vi.fn().mockResolvedValue({ mode: "code" }),
@@ -32,7 +31,6 @@ describe("skillTool", () => {
3231
}
3332

3433
mockCallbacks = {
35-
askApproval: vi.fn().mockResolvedValue(true),
3634
handleError: vi.fn(),
3735
pushToolResult: vi.fn(),
3836
}
@@ -99,7 +97,7 @@ describe("skillTool", () => {
9997
)
10098
})
10199

102-
it("should successfully load built-in skill", async () => {
100+
it("should successfully load built-in skill without approval", async () => {
103101
const block: ToolUse<"skill"> = {
104102
type: "tool_use" as const,
105103
name: "skill" as const,
@@ -121,17 +119,7 @@ describe("skillTool", () => {
121119

122120
await skillTool.handle(mockTask as Task, block, mockCallbacks)
123121

124-
expect(mockCallbacks.askApproval).toHaveBeenCalledWith(
125-
"tool",
126-
JSON.stringify({
127-
tool: "skill",
128-
skill: "create-mcp-server",
129-
args: undefined,
130-
source: "built-in",
131-
description: "Instructions for creating MCP servers",
132-
}),
133-
)
134-
122+
// Skills execute directly without approval
135123
expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
136124
`Skill: create-mcp-server
137125
Description: Instructions for creating MCP servers
@@ -178,57 +166,6 @@ Step 1: Create the server...`,
178166
)
179167
})
180168

181-
it("should handle user rejection", async () => {
182-
const block: ToolUse<"skill"> = {
183-
type: "tool_use" as const,
184-
name: "skill" as const,
185-
params: {},
186-
partial: false,
187-
nativeArgs: {
188-
skill: "create-mcp-server",
189-
},
190-
}
191-
192-
mockSkillsManager.getSkillContent.mockResolvedValue({
193-
name: "create-mcp-server",
194-
description: "Test",
195-
source: "built-in",
196-
instructions: "Test instructions",
197-
})
198-
199-
mockCallbacks.askApproval.mockResolvedValue(false)
200-
201-
await skillTool.handle(mockTask as Task, block, mockCallbacks)
202-
203-
expect(mockCallbacks.pushToolResult).not.toHaveBeenCalled()
204-
})
205-
206-
it("should handle partial block", async () => {
207-
const block: ToolUse<"skill"> = {
208-
type: "tool_use" as const,
209-
name: "skill" as const,
210-
params: {
211-
skill: "create-mcp-server",
212-
args: "",
213-
},
214-
partial: true,
215-
}
216-
217-
await skillTool.handle(mockTask as Task, block, mockCallbacks)
218-
219-
expect(mockTask.ask).toHaveBeenCalledWith(
220-
"tool",
221-
JSON.stringify({
222-
tool: "skill",
223-
skill: "create-mcp-server",
224-
args: "",
225-
}),
226-
true,
227-
)
228-
229-
expect(mockCallbacks.pushToolResult).not.toHaveBeenCalled()
230-
})
231-
232169
it("should handle errors during execution", async () => {
233170
const block: ToolUse<"skill"> = {
234171
type: "tool_use" as const,
@@ -299,7 +236,7 @@ Step 1: Create the server...`,
299236
)
300237
})
301238

302-
it("should load project skill", async () => {
239+
it("should load project skill without approval", async () => {
303240
const block: ToolUse<"skill"> = {
304241
type: "tool_use" as const,
305242
name: "skill" as const,
@@ -321,17 +258,7 @@ Step 1: Create the server...`,
321258

322259
await skillTool.handle(mockTask as Task, block, mockCallbacks)
323260

324-
expect(mockCallbacks.askApproval).toHaveBeenCalledWith(
325-
"tool",
326-
JSON.stringify({
327-
tool: "skill",
328-
skill: "my-project-skill",
329-
args: undefined,
330-
source: "project",
331-
description: "A custom project skill",
332-
}),
333-
)
334-
261+
// Skills execute directly without approval
335262
expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
336263
`Skill: my-project-skill
337264
Description: A custom project skill

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

Lines changed: 0 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -661,75 +661,6 @@ export const ChatRowContent = ({
661661
</div>
662662
</>
663663
)
664-
case "skill": {
665-
const skillInfo = tool
666-
return (
667-
<>
668-
<div style={headerStyle}>
669-
{toolIcon("book")}
670-
<span style={{ fontWeight: "bold" }}>
671-
{message.type === "ask" ? t("chat:skill.wantsToLoad") : t("chat:skill.didLoad")}
672-
</span>
673-
</div>
674-
<div
675-
style={{
676-
marginTop: "4px",
677-
backgroundColor: "var(--vscode-editor-background)",
678-
border: "1px solid var(--vscode-editorGroup-border)",
679-
borderRadius: "4px",
680-
overflow: "hidden",
681-
cursor: "pointer",
682-
}}
683-
onClick={handleToggleExpand}>
684-
<ToolUseBlockHeader
685-
className="group"
686-
style={{
687-
display: "flex",
688-
alignItems: "center",
689-
justifyContent: "space-between",
690-
padding: "10px 12px",
691-
}}>
692-
<div style={{ display: "flex", alignItems: "center", gap: "8px" }}>
693-
<span style={{ fontWeight: "500", fontSize: "var(--vscode-font-size)" }}>
694-
{skillInfo.skill}
695-
</span>
696-
{skillInfo.source && (
697-
<VSCodeBadge style={{ fontSize: "calc(var(--vscode-font-size) - 2px)" }}>
698-
{skillInfo.source}
699-
</VSCodeBadge>
700-
)}
701-
</div>
702-
<span
703-
className={`codicon codicon-chevron-${isExpanded ? "up" : "down"} opacity-0 group-hover:opacity-100 transition-opacity duration-200`}></span>
704-
</ToolUseBlockHeader>
705-
{isExpanded && (skillInfo.args || skillInfo.description) && (
706-
<div
707-
style={{
708-
padding: "12px 16px",
709-
borderTop: "1px solid var(--vscode-editorGroup-border)",
710-
display: "flex",
711-
flexDirection: "column",
712-
gap: "8px",
713-
}}>
714-
{skillInfo.description && (
715-
<div style={{ color: "var(--vscode-descriptionForeground)" }}>
716-
{skillInfo.description}
717-
</div>
718-
)}
719-
{skillInfo.args && (
720-
<div>
721-
<span style={{ fontWeight: "500" }}>Arguments: </span>
722-
<span style={{ color: "var(--vscode-descriptionForeground)" }}>
723-
{skillInfo.args}
724-
</span>
725-
</div>
726-
)}
727-
</div>
728-
)}
729-
</div>
730-
</>
731-
)
732-
}
733664
case "listFilesTopLevel":
734665
return (
735666
<>

0 commit comments

Comments
 (0)