Skip to content

Commit 49f0123

Browse files
jdneojdneo
authored
fix: enhance error handling in appendToolCallStatus method (#145)
1 parent 39565bc commit 49f0123

4 files changed

Lines changed: 331 additions & 7 deletions

File tree

‎com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidgetToolCallStatusTest.java‎

Lines changed: 284 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,284 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT license.
3+
4+
package com.microsoft.copilot.eclipse.ui.chat;
5+
6+
import static org.junit.jupiter.api.Assertions.assertFalse;
7+
import static org.junit.jupiter.api.Assertions.assertNotNull;
8+
import static org.junit.jupiter.api.Assertions.assertNull;
9+
import static org.junit.jupiter.api.Assertions.assertTrue;
10+
import static org.mockito.Mockito.lenient;
11+
import static org.mockito.Mockito.mock;
12+
import static org.mockito.Mockito.mockStatic;
13+
14+
import java.lang.reflect.Field;
15+
import java.util.HashMap;
16+
import java.util.Map;
17+
18+
import org.eclipse.swt.SWT;
19+
import org.eclipse.swt.custom.StyledText;
20+
import org.eclipse.swt.widgets.Display;
21+
import org.eclipse.swt.widgets.Shell;
22+
import org.junit.jupiter.api.AfterEach;
23+
import org.junit.jupiter.api.BeforeEach;
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.api.extension.ExtendWith;
26+
import org.mockito.Mock;
27+
import org.mockito.MockedStatic;
28+
import org.mockito.junit.jupiter.MockitoExtension;
29+
30+
import com.google.gson.Gson;
31+
import com.microsoft.copilot.eclipse.core.lsp.protocol.AgentToolCall;
32+
import com.microsoft.copilot.eclipse.ui.CopilotUi;
33+
import com.microsoft.copilot.eclipse.ui.chat.services.AvatarService;
34+
import com.microsoft.copilot.eclipse.ui.chat.services.ChatFontService;
35+
import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager;
36+
import com.microsoft.copilot.eclipse.ui.utils.SwtUtils;
37+
38+
/**
39+
* Verifies that {@link BaseTurnWidget#appendToolCallStatus} renders text for
40+
* tool calls that finish with status {@code error}.
41+
*/
42+
@ExtendWith(MockitoExtension.class)
43+
class BaseTurnWidgetToolCallStatusTest {
44+
45+
private static final String TURN_ID = "turn-1";
46+
private static final String TOOL_CALL_ID = "tool-call-1";
47+
private static final String TOOL_NAME = "edit_file";
48+
private static final String SUBAGENT_TOOL_NAME = "run_subagent";
49+
private static final String ERROR_MESSAGE = "file not found";
50+
private static final String PROGRESS_MESSAGE = "Editing file.txt";
51+
52+
private Shell shell;
53+
private MockedStatic<CopilotUi> copilotUiMock;
54+
private CopilotUi mockPlugin;
55+
56+
@Mock
57+
private ChatServiceManager mockChatServiceManager;
58+
@Mock
59+
private AvatarService mockAvatarService;
60+
@Mock
61+
private ChatFontService mockChatFontService;
62+
63+
@BeforeEach
64+
void setUp() {
65+
lenient().when(mockChatServiceManager.getAvatarService()).thenReturn(mockAvatarService);
66+
lenient().when(mockChatServiceManager.getChatFontService()).thenReturn(mockChatFontService);
67+
lenient().when(mockAvatarService.getAvatarForCopilot()).thenReturn(null);
68+
69+
// Mockito's static mocks are scoped to the registering thread. The widgets under
70+
// test execute on the SWT Display thread (via SwtUtils.invokeOnDisplayThread), so
71+
// the mock must be registered there as well; otherwise CopilotUi.getPlugin() calls
72+
// from the display thread would not see it.
73+
SwtUtils.invokeOnDisplayThread(() -> {
74+
shell = new Shell(Display.getDefault());
75+
copilotUiMock = mockStatic(CopilotUi.class);
76+
mockPlugin = mock(CopilotUi.class);
77+
copilotUiMock.when(CopilotUi::getPlugin).thenReturn(mockPlugin);
78+
lenient().when(mockPlugin.getChatServiceManager()).thenReturn(mockChatServiceManager);
79+
});
80+
}
81+
82+
@AfterEach
83+
void tearDown() {
84+
SwtUtils.invokeOnDisplayThread(() -> {
85+
if (copilotUiMock != null) {
86+
copilotUiMock.close();
87+
copilotUiMock = null;
88+
}
89+
if (shell != null && !shell.isDisposed()) {
90+
shell.dispose();
91+
}
92+
});
93+
}
94+
95+
@Test
96+
void appendToolCallStatus_errorWithMessage_rendersErrorText() {
97+
SwtUtils.invokeOnDisplayThread(() -> {
98+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
99+
100+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, ERROR_MESSAGE));
101+
102+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
103+
assertNotNull(text, "Status label text must be created when an error event is delivered");
104+
assertTrue(text.getText().contains(ERROR_MESSAGE),
105+
"Expected error message to be rendered next to the error icon, got: '" + text.getText() + "'");
106+
assertTrue(text.getText().contains(TOOL_NAME),
107+
"Expected error text to be prefixed with the tool name, got: '" + text.getText() + "'");
108+
});
109+
}
110+
111+
@Test
112+
void appendToolCallStatus_errorWithoutProgressMessage_isStillProcessed() {
113+
SwtUtils.invokeOnDisplayThread(() -> {
114+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
115+
116+
// No progressMessage (e.g., tool failed before the running event was dispatched server-side).
117+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", null, ERROR_MESSAGE));
118+
119+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
120+
assertNotNull(text, "Error event without progressMessage must still be processed");
121+
assertTrue(text.getText().contains(ERROR_MESSAGE),
122+
"Expected error message to be rendered, got: '" + text.getText() + "'");
123+
});
124+
}
125+
126+
@Test
127+
void appendToolCallStatus_runningThenError_replacesProgressTextWithErrorText() {
128+
SwtUtils.invokeOnDisplayThread(() -> {
129+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
130+
131+
// First the running event sets the progress text.
132+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "running", PROGRESS_MESSAGE, null));
133+
StyledText runningText = getStatusLabelText(widget, TOOL_CALL_ID);
134+
assertNotNull(runningText, "Running event should have populated text");
135+
assertTrue(runningText.getText().contains(PROGRESS_MESSAGE),
136+
"Running text should be visible before failure, got: '" + runningText.getText() + "'");
137+
138+
// Then the same tool call fails: the error text must overwrite the stale running text.
139+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, ERROR_MESSAGE));
140+
141+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
142+
assertNotNull(text);
143+
assertTrue(text.getText().contains(ERROR_MESSAGE),
144+
"Error text should replace stale running text, got: '" + text.getText() + "'");
145+
});
146+
}
147+
148+
@Test
149+
void appendToolCallStatus_errorFallsBackToProgressMessage_whenErrorIsBlank() {
150+
SwtUtils.invokeOnDisplayThread(() -> {
151+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
152+
153+
// Whitespace-only error must fall back to progressMessage rather than rendering empty text.
154+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, " "));
155+
156+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
157+
assertNotNull(text, "Status label text must be created on error even when only progressMessage is present");
158+
assertTrue(text.getText().contains(PROGRESS_MESSAGE),
159+
"Expected progressMessage to be used as fallback, got: '" + text.getText() + "'");
160+
});
161+
}
162+
163+
@Test
164+
void appendToolCallStatus_errorFallsBackToProgressMessage_whenErrorIsNull() {
165+
SwtUtils.invokeOnDisplayThread(() -> {
166+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
167+
168+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, null));
169+
170+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
171+
assertNotNull(text);
172+
assertTrue(text.getText().contains(PROGRESS_MESSAGE),
173+
"Expected progressMessage to be used as fallback, got: '" + text.getText() + "'");
174+
});
175+
}
176+
177+
@Test
178+
void appendToolCallStatus_errorWithBothFieldsBlank_rendersGenericFallbackWithToolName() {
179+
SwtUtils.invokeOnDisplayThread(() -> {
180+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
181+
182+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", null, null));
183+
184+
StyledText text = getStatusLabelText(widget, TOOL_CALL_ID);
185+
assertNotNull(text, "Error event with no fields must still surface a generic message");
186+
assertTrue(text.getText().contains(TOOL_NAME),
187+
"Generic fallback should still tell the user which tool failed, got: '" + text.getText() + "'");
188+
});
189+
}
190+
191+
@Test
192+
void appendToolCallStatus_nonErrorStatusWithOnlyError_isIgnored() {
193+
SwtUtils.invokeOnDisplayThread(() -> {
194+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
195+
196+
// Non-error events with only `error` populated must be dropped; otherwise
197+
// setRunningStatus/setCompletedStatus/setText would call setMarkup(null).
198+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "running", null, ERROR_MESSAGE));
199+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "completed", " ", ERROR_MESSAGE));
200+
widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "cancelled", null, ERROR_MESSAGE));
201+
202+
@SuppressWarnings("unchecked")
203+
Map<String, AgentStatusLabel> labels = (Map<String, AgentStatusLabel>) getField(widget, "statusLabels");
204+
assertTrue(labels.isEmpty(),
205+
"Non-error events without progressMessage should be ignored, got: " + labels.keySet());
206+
});
207+
}
208+
209+
/**
210+
* Regression: a {@code run_subagent} terminal event with no progressMessage must still
211+
* clear the subagent block state. Otherwise subsequent messages get routed to a ghost
212+
* {@code currentSubagentBlock} (see review comment on PR #145).
213+
*/
214+
@Test
215+
void appendToolCallStatus_subagentTerminalEventWithBlankMessage_clearsSubagentState() {
216+
SwtUtils.invokeOnDisplayThread(() -> {
217+
CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID);
218+
219+
// Open a subagent block.
220+
widget.appendToolCallStatus(buildToolCall(SUBAGENT_TOOL_NAME, "running", null, null));
221+
assertTrue((boolean) getField(widget, "inSubagentBlock"),
222+
"Subagent block should be active after a 'running' event");
223+
assertNotNull(getField(widget, "currentSubagentBlock"),
224+
"currentSubagentBlock should be populated after a 'running' event");
225+
226+
// Terminal event with no display message — the early-return guard for blank
227+
// progressMessage must NOT short-circuit subagent state cleanup.
228+
widget.appendToolCallStatus(buildToolCall(SUBAGENT_TOOL_NAME, "completed", null, null));
229+
230+
assertFalse((boolean) getField(widget, "inSubagentBlock"),
231+
"inSubagentBlock should be cleared after a terminal subagent event");
232+
assertNull(getField(widget, "currentSubagentBlock"),
233+
"currentSubagentBlock should be cleared after a terminal subagent event");
234+
});
235+
}
236+
237+
private static AgentToolCall buildToolCall(String name, String status, String progressMessage, String error) {
238+
Gson gson = new Gson();
239+
Map<String, Object> fields = new HashMap<>();
240+
fields.put("id", TOOL_CALL_ID);
241+
fields.put("name", name);
242+
fields.put("status", status);
243+
if (progressMessage != null) {
244+
fields.put("progressMessage", progressMessage);
245+
}
246+
if (error != null) {
247+
fields.put("error", error);
248+
}
249+
return gson.fromJson(gson.toJson(fields), AgentToolCall.class);
250+
}
251+
252+
private static StyledText getStatusLabelText(BaseTurnWidget widget, String toolCallId) {
253+
@SuppressWarnings("unchecked")
254+
Map<String, AgentStatusLabel> labels = (Map<String, AgentStatusLabel>) getField(widget, "statusLabels");
255+
AgentStatusLabel label = labels.get(toolCallId);
256+
assertNotNull(label, "AgentStatusLabel for tool call '" + toolCallId + "' should have been created");
257+
Object markupViewer = getField(label, "textLabel");
258+
if (markupViewer == null) {
259+
return null;
260+
}
261+
try {
262+
return (StyledText) markupViewer.getClass().getMethod("getTextWidget").invoke(markupViewer);
263+
} catch (ReflectiveOperationException e) {
264+
throw new RuntimeException("Failed to obtain StyledText from textLabel", e);
265+
}
266+
}
267+
268+
private static Object getField(Object target, String name) {
269+
Class<?> cls = target.getClass();
270+
while (cls != null) {
271+
try {
272+
Field f = cls.getDeclaredField(name);
273+
f.setAccessible(true);
274+
return f.get(target);
275+
} catch (NoSuchFieldException e) {
276+
cls = cls.getSuperclass();
277+
} catch (IllegalAccessException e) {
278+
throw new RuntimeException(e);
279+
}
280+
}
281+
throw new RuntimeException("Field '" + name + "' not found on " + target.getClass());
282+
}
283+
}
284+

‎com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.eclipse.e4.core.services.events.IEventBroker;
1212
import org.eclipse.jface.text.Document;
1313
import org.eclipse.jface.text.source.SourceViewer;
14+
import org.eclipse.osgi.util.NLS;
1415
import org.eclipse.swt.SWT;
1516
import org.eclipse.swt.graphics.Font;
1617
import org.eclipse.swt.graphics.Image;
@@ -226,16 +227,26 @@ public void appendMessage(String message) {
226227
* @param toolCall the tool call of the agent turn
227228
*/
228229
public void appendToolCallStatus(AgentToolCall toolCall) {
229-
if (toolCall == null || StringUtils.isEmpty(toolCall.getProgressMessage())) {
230+
if (toolCall == null || toolCall.getStatus() == null) {
230231
return;
231232
}
232233

233-
// Check if this is a run_subagent tool call
234+
// Subagent tool calls drive routing state for `currentSubagentBlock`/`inSubagentBlock`,
235+
// so they must always be dispatched, even when the terminal event has no display message.
234236
if ("run_subagent".equalsIgnoreCase(toolCall.getName())) {
235237
handleSubagentToolCall(toolCall);
236238
return;
237239
}
238240

241+
String status = toolCall.getStatus().toLowerCase();
242+
// Non-error events require a non-blank progressMessage to render (otherwise we'd
243+
// call ChatMarkupViewer#setMarkup(null) and NPE). Error events always pass through:
244+
// getErrorDisplayText(...) provides a non-blank fallback.
245+
boolean isError = "error".equals(status);
246+
if (!isError && StringUtils.isBlank(toolCall.getProgressMessage())) {
247+
return;
248+
}
249+
239250
// If we're in a subagent block, route tool calls there
240251
if (inSubagentBlock && currentSubagentBlock != null) {
241252
currentSubagentBlock.appendToolCallStatus(toolCall);
@@ -254,7 +265,6 @@ public void appendToolCallStatus(AgentToolCall toolCall) {
254265
AgentStatusLabel statusLabel = statusLabels.computeIfAbsent(toolCall.getId(),
255266
id -> new AgentStatusLabel(this, SWT.LEFT));
256267

257-
String status = toolCall.getStatus().toLowerCase();
258268
switch (status) {
259269
case "running":
260270
statusLabel.setRunningStatus(toolCall.getProgressMessage());
@@ -268,6 +278,7 @@ public void appendToolCallStatus(AgentToolCall toolCall) {
268278
break;
269279
case "error":
270280
statusLabel.setErrorStatus();
281+
statusLabel.setText(getErrorDisplayText(toolCall));
271282
break;
272283
default:
273284
statusLabel.setErrorStatus();
@@ -317,12 +328,12 @@ private void handleSubagentToolCall(AgentToolCall toolCall) {
317328
id -> new AgentStatusLabel(this, SWT.LEFT));
318329
if ("cancelled".equals(status)) {
319330
statusLabel.setCancelledStatus();
320-
statusLabel.setText(toolCall.getProgressMessage());
331+
if (StringUtils.isNotBlank(toolCall.getProgressMessage())) {
332+
statusLabel.setText(toolCall.getProgressMessage());
333+
}
321334
} else {
322335
statusLabel.setErrorStatus();
323-
String errorText = StringUtils.isNotEmpty(toolCall.getError()) ? toolCall.getError()
324-
: toolCall.getProgressMessage();
325-
statusLabel.setText(errorText);
336+
statusLabel.setText(getErrorDisplayText(toolCall));
326337
}
327338
requestLayout();
328339
break;
@@ -335,6 +346,31 @@ private void handleSubagentToolCall(AgentToolCall toolCall) {
335346
}
336347
}
337348

349+
/**
350+
* Resolve the user-facing error text for a tool call.
351+
*
352+
* <p>Picks the first non-blank of {@code toolCall.getError()} or {@code toolCall.getProgressMessage()},
353+
* falls back to a generic message when both are blank, and prefixes the result with the tool name
354+
* so the user knows which tool failed.
355+
*
356+
* @param toolCall the failing tool call
357+
* @return a non-blank, prefixed display string suitable for {@link AgentStatusLabel#setText(String)}
358+
*/
359+
private static String getErrorDisplayText(AgentToolCall toolCall) {
360+
String detail = toolCall.getError();
361+
if (StringUtils.isBlank(detail)) {
362+
detail = toolCall.getProgressMessage();
363+
}
364+
if (StringUtils.isBlank(detail)) {
365+
detail = Messages.chat_toolCall_genericError;
366+
}
367+
String name = toolCall.getName();
368+
if (StringUtils.isBlank(name)) {
369+
return detail;
370+
}
371+
return NLS.bind(Messages.chat_toolCall_errorTemplate, name, detail);
372+
}
373+
338374
private void processMessageLine(String line) {
339375
SwtUtils.invokeOnDisplayThread(() -> {
340376
if (line.trim().startsWith(CODE_BLOCK_ANNOTATION)) {

‎com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ public final class Messages extends NLS {
1212
private static final String BUNDLE_NAME = "com.microsoft.copilot.eclipse.ui.chat.messages"; //$NON-NLS-1$
1313

1414
public static String chat_chatContentView_errorTemplate;
15+
public static String chat_toolCall_genericError;
16+
public static String chat_toolCall_errorTemplate;
1517
public static String endChat_confirmationTitle;
1618
public static String endChat_confirmationMessage;
1719
public static String confirmDialog_keepChangesButton;

0 commit comments

Comments
 (0)