diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidgetToolCallStatusTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidgetToolCallStatusTest.java new file mode 100644 index 00000000..af37dce6 --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidgetToolCallStatusTest.java @@ -0,0 +1,284 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; + +import org.eclipse.swt.SWT; +import org.eclipse.swt.custom.StyledText; +import org.eclipse.swt.widgets.Display; +import org.eclipse.swt.widgets.Shell; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.junit.jupiter.MockitoExtension; + +import com.google.gson.Gson; +import com.microsoft.copilot.eclipse.core.lsp.protocol.AgentToolCall; +import com.microsoft.copilot.eclipse.ui.CopilotUi; +import com.microsoft.copilot.eclipse.ui.chat.services.AvatarService; +import com.microsoft.copilot.eclipse.ui.chat.services.ChatFontService; +import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager; +import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; + +/** + * Verifies that {@link BaseTurnWidget#appendToolCallStatus} renders text for + * tool calls that finish with status {@code error}. + */ +@ExtendWith(MockitoExtension.class) +class BaseTurnWidgetToolCallStatusTest { + + private static final String TURN_ID = "turn-1"; + private static final String TOOL_CALL_ID = "tool-call-1"; + private static final String TOOL_NAME = "edit_file"; + private static final String SUBAGENT_TOOL_NAME = "run_subagent"; + private static final String ERROR_MESSAGE = "file not found"; + private static final String PROGRESS_MESSAGE = "Editing file.txt"; + + private Shell shell; + private MockedStatic copilotUiMock; + private CopilotUi mockPlugin; + + @Mock + private ChatServiceManager mockChatServiceManager; + @Mock + private AvatarService mockAvatarService; + @Mock + private ChatFontService mockChatFontService; + + @BeforeEach + void setUp() { + lenient().when(mockChatServiceManager.getAvatarService()).thenReturn(mockAvatarService); + lenient().when(mockChatServiceManager.getChatFontService()).thenReturn(mockChatFontService); + lenient().when(mockAvatarService.getAvatarForCopilot()).thenReturn(null); + + // Mockito's static mocks are scoped to the registering thread. The widgets under + // test execute on the SWT Display thread (via SwtUtils.invokeOnDisplayThread), so + // the mock must be registered there as well; otherwise CopilotUi.getPlugin() calls + // from the display thread would not see it. + SwtUtils.invokeOnDisplayThread(() -> { + shell = new Shell(Display.getDefault()); + copilotUiMock = mockStatic(CopilotUi.class); + mockPlugin = mock(CopilotUi.class); + copilotUiMock.when(CopilotUi::getPlugin).thenReturn(mockPlugin); + lenient().when(mockPlugin.getChatServiceManager()).thenReturn(mockChatServiceManager); + }); + } + + @AfterEach + void tearDown() { + SwtUtils.invokeOnDisplayThread(() -> { + if (copilotUiMock != null) { + copilotUiMock.close(); + copilotUiMock = null; + } + if (shell != null && !shell.isDisposed()) { + shell.dispose(); + } + }); + } + + @Test + void appendToolCallStatus_errorWithMessage_rendersErrorText() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, ERROR_MESSAGE)); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text, "Status label text must be created when an error event is delivered"); + assertTrue(text.getText().contains(ERROR_MESSAGE), + "Expected error message to be rendered next to the error icon, got: '" + text.getText() + "'"); + assertTrue(text.getText().contains(TOOL_NAME), + "Expected error text to be prefixed with the tool name, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_errorWithoutProgressMessage_isStillProcessed() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + // No progressMessage (e.g., tool failed before the running event was dispatched server-side). + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", null, ERROR_MESSAGE)); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text, "Error event without progressMessage must still be processed"); + assertTrue(text.getText().contains(ERROR_MESSAGE), + "Expected error message to be rendered, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_runningThenError_replacesProgressTextWithErrorText() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + // First the running event sets the progress text. + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "running", PROGRESS_MESSAGE, null)); + StyledText runningText = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(runningText, "Running event should have populated text"); + assertTrue(runningText.getText().contains(PROGRESS_MESSAGE), + "Running text should be visible before failure, got: '" + runningText.getText() + "'"); + + // Then the same tool call fails: the error text must overwrite the stale running text. + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, ERROR_MESSAGE)); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text); + assertTrue(text.getText().contains(ERROR_MESSAGE), + "Error text should replace stale running text, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_errorFallsBackToProgressMessage_whenErrorIsBlank() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + // Whitespace-only error must fall back to progressMessage rather than rendering empty text. + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, " ")); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text, "Status label text must be created on error even when only progressMessage is present"); + assertTrue(text.getText().contains(PROGRESS_MESSAGE), + "Expected progressMessage to be used as fallback, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_errorFallsBackToProgressMessage_whenErrorIsNull() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", PROGRESS_MESSAGE, null)); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text); + assertTrue(text.getText().contains(PROGRESS_MESSAGE), + "Expected progressMessage to be used as fallback, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_errorWithBothFieldsBlank_rendersGenericFallbackWithToolName() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "error", null, null)); + + StyledText text = getStatusLabelText(widget, TOOL_CALL_ID); + assertNotNull(text, "Error event with no fields must still surface a generic message"); + assertTrue(text.getText().contains(TOOL_NAME), + "Generic fallback should still tell the user which tool failed, got: '" + text.getText() + "'"); + }); + } + + @Test + void appendToolCallStatus_nonErrorStatusWithOnlyError_isIgnored() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + // Non-error events with only `error` populated must be dropped; otherwise + // setRunningStatus/setCompletedStatus/setText would call setMarkup(null). + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "running", null, ERROR_MESSAGE)); + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "completed", " ", ERROR_MESSAGE)); + widget.appendToolCallStatus(buildToolCall(TOOL_NAME, "cancelled", null, ERROR_MESSAGE)); + + @SuppressWarnings("unchecked") + Map labels = (Map) getField(widget, "statusLabels"); + assertTrue(labels.isEmpty(), + "Non-error events without progressMessage should be ignored, got: " + labels.keySet()); + }); + } + + /** + * Regression: a {@code run_subagent} terminal event with no progressMessage must still + * clear the subagent block state. Otherwise subsequent messages get routed to a ghost + * {@code currentSubagentBlock} (see review comment on PR #145). + */ + @Test + void appendToolCallStatus_subagentTerminalEventWithBlankMessage_clearsSubagentState() { + SwtUtils.invokeOnDisplayThread(() -> { + CopilotTurnWidget widget = new CopilotTurnWidget(shell, SWT.NONE, mockChatServiceManager, TURN_ID); + + // Open a subagent block. + widget.appendToolCallStatus(buildToolCall(SUBAGENT_TOOL_NAME, "running", null, null)); + assertTrue((boolean) getField(widget, "inSubagentBlock"), + "Subagent block should be active after a 'running' event"); + assertNotNull(getField(widget, "currentSubagentBlock"), + "currentSubagentBlock should be populated after a 'running' event"); + + // Terminal event with no display message — the early-return guard for blank + // progressMessage must NOT short-circuit subagent state cleanup. + widget.appendToolCallStatus(buildToolCall(SUBAGENT_TOOL_NAME, "completed", null, null)); + + assertFalse((boolean) getField(widget, "inSubagentBlock"), + "inSubagentBlock should be cleared after a terminal subagent event"); + assertNull(getField(widget, "currentSubagentBlock"), + "currentSubagentBlock should be cleared after a terminal subagent event"); + }); + } + + private static AgentToolCall buildToolCall(String name, String status, String progressMessage, String error) { + Gson gson = new Gson(); + Map fields = new HashMap<>(); + fields.put("id", TOOL_CALL_ID); + fields.put("name", name); + fields.put("status", status); + if (progressMessage != null) { + fields.put("progressMessage", progressMessage); + } + if (error != null) { + fields.put("error", error); + } + return gson.fromJson(gson.toJson(fields), AgentToolCall.class); + } + + private static StyledText getStatusLabelText(BaseTurnWidget widget, String toolCallId) { + @SuppressWarnings("unchecked") + Map labels = (Map) getField(widget, "statusLabels"); + AgentStatusLabel label = labels.get(toolCallId); + assertNotNull(label, "AgentStatusLabel for tool call '" + toolCallId + "' should have been created"); + Object markupViewer = getField(label, "textLabel"); + if (markupViewer == null) { + return null; + } + try { + return (StyledText) markupViewer.getClass().getMethod("getTextWidget").invoke(markupViewer); + } catch (ReflectiveOperationException e) { + throw new RuntimeException("Failed to obtain StyledText from textLabel", e); + } + } + + private static Object getField(Object target, String name) { + Class cls = target.getClass(); + while (cls != null) { + try { + Field f = cls.getDeclaredField(name); + f.setAccessible(true); + return f.get(target); + } catch (NoSuchFieldException e) { + cls = cls.getSuperclass(); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + } + throw new RuntimeException("Field '" + name + "' not found on " + target.getClass()); + } +} + diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java index 66e64742..5ddc9898 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java @@ -11,6 +11,7 @@ import org.eclipse.e4.core.services.events.IEventBroker; import org.eclipse.jface.text.Document; import org.eclipse.jface.text.source.SourceViewer; +import org.eclipse.osgi.util.NLS; import org.eclipse.swt.SWT; import org.eclipse.swt.graphics.Font; import org.eclipse.swt.graphics.Image; @@ -226,16 +227,26 @@ public void appendMessage(String message) { * @param toolCall the tool call of the agent turn */ public void appendToolCallStatus(AgentToolCall toolCall) { - if (toolCall == null || StringUtils.isEmpty(toolCall.getProgressMessage())) { + if (toolCall == null || toolCall.getStatus() == null) { return; } - // Check if this is a run_subagent tool call + // Subagent tool calls drive routing state for `currentSubagentBlock`/`inSubagentBlock`, + // so they must always be dispatched, even when the terminal event has no display message. if ("run_subagent".equalsIgnoreCase(toolCall.getName())) { handleSubagentToolCall(toolCall); return; } + String status = toolCall.getStatus().toLowerCase(); + // Non-error events require a non-blank progressMessage to render (otherwise we'd + // call ChatMarkupViewer#setMarkup(null) and NPE). Error events always pass through: + // getErrorDisplayText(...) provides a non-blank fallback. + boolean isError = "error".equals(status); + if (!isError && StringUtils.isBlank(toolCall.getProgressMessage())) { + return; + } + // If we're in a subagent block, route tool calls there if (inSubagentBlock && currentSubagentBlock != null) { currentSubagentBlock.appendToolCallStatus(toolCall); @@ -254,7 +265,6 @@ public void appendToolCallStatus(AgentToolCall toolCall) { AgentStatusLabel statusLabel = statusLabels.computeIfAbsent(toolCall.getId(), id -> new AgentStatusLabel(this, SWT.LEFT)); - String status = toolCall.getStatus().toLowerCase(); switch (status) { case "running": statusLabel.setRunningStatus(toolCall.getProgressMessage()); @@ -268,6 +278,7 @@ public void appendToolCallStatus(AgentToolCall toolCall) { break; case "error": statusLabel.setErrorStatus(); + statusLabel.setText(getErrorDisplayText(toolCall)); break; default: statusLabel.setErrorStatus(); @@ -317,12 +328,12 @@ private void handleSubagentToolCall(AgentToolCall toolCall) { id -> new AgentStatusLabel(this, SWT.LEFT)); if ("cancelled".equals(status)) { statusLabel.setCancelledStatus(); - statusLabel.setText(toolCall.getProgressMessage()); + if (StringUtils.isNotBlank(toolCall.getProgressMessage())) { + statusLabel.setText(toolCall.getProgressMessage()); + } } else { statusLabel.setErrorStatus(); - String errorText = StringUtils.isNotEmpty(toolCall.getError()) ? toolCall.getError() - : toolCall.getProgressMessage(); - statusLabel.setText(errorText); + statusLabel.setText(getErrorDisplayText(toolCall)); } requestLayout(); break; @@ -335,6 +346,31 @@ private void handleSubagentToolCall(AgentToolCall toolCall) { } } + /** + * Resolve the user-facing error text for a tool call. + * + *

Picks the first non-blank of {@code toolCall.getError()} or {@code toolCall.getProgressMessage()}, + * falls back to a generic message when both are blank, and prefixes the result with the tool name + * so the user knows which tool failed. + * + * @param toolCall the failing tool call + * @return a non-blank, prefixed display string suitable for {@link AgentStatusLabel#setText(String)} + */ + private static String getErrorDisplayText(AgentToolCall toolCall) { + String detail = toolCall.getError(); + if (StringUtils.isBlank(detail)) { + detail = toolCall.getProgressMessage(); + } + if (StringUtils.isBlank(detail)) { + detail = Messages.chat_toolCall_genericError; + } + String name = toolCall.getName(); + if (StringUtils.isBlank(name)) { + return detail; + } + return NLS.bind(Messages.chat_toolCall_errorTemplate, name, detail); + } + private void processMessageLine(String line) { SwtUtils.invokeOnDisplayThread(() -> { if (line.trim().startsWith(CODE_BLOCK_ANNOTATION)) { diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java index 0f17edd2..20117a07 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java @@ -12,6 +12,8 @@ public final class Messages extends NLS { private static final String BUNDLE_NAME = "com.microsoft.copilot.eclipse.ui.chat.messages"; //$NON-NLS-1$ public static String chat_chatContentView_errorTemplate; + public static String chat_toolCall_genericError; + public static String chat_toolCall_errorTemplate; public static String endChat_confirmationTitle; public static String endChat_confirmationMessage; public static String confirmDialog_keepChangesButton; diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties index 30864676..acfc43c0 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties @@ -1,5 +1,7 @@ chat_chatContentView_errorTemplate=Sorry, your request failed: %s. Request id: %s. chat_warnWidget_defaultErrorMsg=An error occurred. Please try again. +chat_toolCall_genericError=Tool call failed. +chat_toolCall_errorTemplate={0}: {1} confirmDialog_keepChangesButton=Keep and Continue confirmDialog_undoChangesButton=Undo and Continue