Skip to content

Commit 93d37a5

Browse files
roomoteroomote
committed
Fixes #5058 
1 parent 609df58 commit 93d37a5

File tree

3 files changed

+233
-2
lines changed

3 files changed

+233
-2
lines changed

‎src/core/assistant-message/__tests__/askApproval.test.ts‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest"
2+
import { Anthropic } from "@anthropic-ai/sdk"
3+
4+
// Create a simplified test that focuses on the askApproval function behavior
5+
describe("askApproval - User Feedback Preservation", () => {
6+
let mockTask: any
7+
let askApproval: any
8+
9+
beforeEach(() => {
10+
vi.clearAllMocks()
11+
12+
mockTask = {
13+
ask: vi.fn(),
14+
say: vi.fn(),
15+
addToApiConversationHistory: vi.fn(),
16+
didRejectTool: false,
17+
}
18+
19+
// Extract the askApproval function logic for testing
20+
askApproval = async (type: string, partialMessage?: string, progressStatus?: any, isProtected?: boolean) => {
21+
const { response, text, images } = await mockTask.ask(
22+
type,
23+
partialMessage,
24+
false,
25+
progressStatus,
26+
isProtected || false,
27+
)
28+
29+
if (response !== "yesButtonClicked") {
30+
// Handle both messageResponse and noButtonClicked with text.
31+
if (text) {
32+
await mockTask.say("user_feedback", text, images)
33+
34+
// Add user feedback to API conversation history to preserve context
35+
const userContent: Anthropic.Messages.ContentBlockParam[] = [
36+
{ type: "text", text: `[User feedback during tool validation]: ${text}` },
37+
]
38+
if (images && images.length > 0) {
39+
// Mock formatResponse.imageBlocks
40+
userContent.push(
41+
...images.map((img: string) => ({
42+
type: "image",
43+
source: {
44+
type: "base64",
45+
media_type: "image/jpeg",
46+
data: img,
47+
},
48+
})),
49+
)
50+
}
51+
await mockTask.addToApiConversationHistory({ role: "user", content: userContent })
52+
}
53+
mockTask.didRejectTool = true
54+
return false
55+
}
56+
57+
// Handle yesButtonClicked with text.
58+
if (text) {
59+
await mockTask.say("user_feedback", text, images)
60+
61+
// Add user feedback to API conversation history to preserve context
62+
const userContent: Anthropic.Messages.ContentBlockParam[] = [
63+
{ type: "text", text: `[User feedback during tool validation]: ${text}` },
64+
]
65+
if (images && images.length > 0) {
66+
// Mock formatResponse.imageBlocks
67+
userContent.push(
68+
...images.map((img: string) => ({
69+
type: "image",
70+
source: {
71+
type: "base64",
72+
media_type: "image/jpeg",
73+
data: img,
74+
},
75+
})),
76+
)
77+
}
78+
await mockTask.addToApiConversationHistory({ role: "user", content: userContent })
79+
}
80+
81+
return true
82+
}
83+
})
84+
85+
it("should preserve user feedback in API conversation history when tool is approved with feedback", async () => {
86+
// Mock user providing feedback when approving tool
87+
const mockAskResponse = {
88+
response: "yesButtonClicked" as const,
89+
text: "Please read the entire file carefully",
90+
images: ["image1.jpg"],
91+
}
92+
mockTask.ask.mockResolvedValue(mockAskResponse)
93+
94+
// Execute
95+
const result = await askApproval("tool", "test message")
96+
97+
// Verify result
98+
expect(result).toBe(true)
99+
100+
// Verify that user feedback was added to API conversation history
101+
expect(mockTask.addToApiConversationHistory).toHaveBeenCalledWith({
102+
role: "user",
103+
content: [
104+
{
105+
type: "text",
106+
text: "[User feedback during tool validation]: Please read the entire file carefully",
107+
},
108+
{
109+
type: "image",
110+
source: {
111+
type: "base64",
112+
media_type: "image/jpeg",
113+
data: "image1.jpg",
114+
},
115+
},
116+
],
117+
})
118+
119+
// Verify that user feedback was also displayed in UI
120+
expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Please read the entire file carefully", [
121+
"image1.jpg",
122+
])
123+
})
124+
125+
it("should preserve user feedback in API conversation history when tool is rejected with feedback", async () => {
126+
// Mock user providing feedback when rejecting tool
127+
const mockAskResponse = {
128+
response: "noButtonClicked" as const,
129+
text: "Don't write to that file, use a different path",
130+
images: undefined,
131+
}
132+
mockTask.ask.mockResolvedValue(mockAskResponse)
133+
134+
// Execute
135+
const result = await askApproval("tool", "test message")
136+
137+
// Verify result
138+
expect(result).toBe(false)
139+
expect(mockTask.didRejectTool).toBe(true)
140+
141+
// Verify that user feedback was added to API conversation history
142+
expect(mockTask.addToApiConversationHistory).toHaveBeenCalledWith({
143+
role: "user",
144+
content: [
145+
{
146+
type: "text",
147+
text: "[User feedback during tool validation]: Don't write to that file, use a different path",
148+
},
149+
],
150+
})
151+
152+
// Verify that user feedback was also displayed in UI
153+
expect(mockTask.say).toHaveBeenCalledWith(
154+
"user_feedback",
155+
"Don't write to that file, use a different path",
156+
undefined,
157+
)
158+
})
159+
160+
it("should not add to API conversation history when user approves without feedback", async () => {
161+
// Mock user approving without feedback
162+
const mockAskResponse = {
163+
response: "yesButtonClicked" as const,
164+
text: undefined,
165+
images: undefined,
166+
}
167+
mockTask.ask.mockResolvedValue(mockAskResponse)
168+
169+
// Execute
170+
const result = await askApproval("tool", "test message")
171+
172+
// Verify result
173+
expect(result).toBe(true)
174+
175+
// Verify that no additional API conversation history was added
176+
expect(mockTask.addToApiConversationHistory).not.toHaveBeenCalled()
177+
178+
// Verify that no user feedback was displayed in UI
179+
expect(mockTask.say).not.toHaveBeenCalledWith("user_feedback", expect.anything(), expect.anything())
180+
})
181+
182+
it("should handle messageResponse type with feedback", async () => {
183+
// Mock user providing messageResponse with feedback
184+
const mockAskResponse = {
185+
response: "messageResponse" as const,
186+
text: "Use ls -la --color=always instead",
187+
images: undefined,
188+
}
189+
mockTask.ask.mockResolvedValue(mockAskResponse)
190+
191+
// Execute
192+
const result = await askApproval("tool", "test message")
193+
194+
// Verify result
195+
expect(result).toBe(false)
196+
expect(mockTask.didRejectTool).toBe(true)
197+
198+
// Verify that user feedback was added to API conversation history
199+
expect(mockTask.addToApiConversationHistory).toHaveBeenCalledWith({
200+
role: "user",
201+
content: [
202+
{
203+
type: "text",
204+
text: "[User feedback during tool validation]: Use ls -la --color=always instead",
205+
},
206+
],
207+
})
208+
209+
// Verify that user feedback was also displayed in UI
210+
expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Use ls -la --color=always instead", undefined)
211+
})
212+
})

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import cloneDeep from "clone-deep"
22
import { serializeError } from "serialize-error"
3+
import { Anthropic } from "@anthropic-ai/sdk"
34

45
import type { ToolName, ClineAsk, ToolProgressStatus } from "@roo-code/types"
56
import { TelemetryService } from "@roo-code/telemetry"
@@ -275,6 +276,16 @@ export async function presentAssistantMessage(cline: Task) {
275276
// Handle both messageResponse and noButtonClicked with text.
276277
if (text) {
277278
await cline.say("user_feedback", text, images)
279+
280+
// Add user feedback to API conversation history to preserve context
281+
const userContent: Anthropic.Messages.ContentBlockParam[] = [
282+
{ type: "text", text: `[User feedback during tool validation]: ${text}` },
283+
]
284+
if (images && images.length > 0) {
285+
userContent.push(...formatResponse.imageBlocks(images))
286+
}
287+
await cline.addToApiConversationHistory({ role: "user", content: userContent })
288+
278289
pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images))
279290
} else {
280291
pushToolResult(formatResponse.toolDenied())
@@ -286,7 +297,15 @@ export async function presentAssistantMessage(cline: Task) {
286297
// Handle yesButtonClicked with text.
287298
if (text) {
288299
await cline.say("user_feedback", text, images)
289-
pushToolResult(formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text), images))
300+
301+
// Add user feedback to API conversation history to preserve context
302+
const userContent: Anthropic.Messages.ContentBlockParam[] = [
303+
{ type: "text", text: `[User feedback during tool validation]: ${text}` },
304+
]
305+
if (images && images.length > 0) {
306+
userContent.push(...formatResponse.imageBlocks(images))
307+
}
308+
await cline.addToApiConversationHistory({ role: "user", content: userContent })
290309
}
291310

292311
return true

‎src/core/task/Task.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ export class Task extends EventEmitter<ClineEvents> {
318318
return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath })
319319
}
320320

321-
private async addToApiConversationHistory(message: Anthropic.MessageParam) {
321+
public async addToApiConversationHistory(message: Anthropic.MessageParam) {
322322
const messageWithTs = { ...message, ts: Date.now() }
323323
this.apiConversationHistory.push(messageWithTs)
324324
await this.saveApiConversationHistory()

0 commit comments

Comments
 (0)