From acbba9b7d084e83dced1dbdb28ee6f4b90ed8814 Mon Sep 17 00:00:00 2001 From: chezsmithy Date: Wed, 27 Aug 2025 23:48:05 -0700 Subject: [PATCH] fix: :bug: Fixes based on reviewer comments --- core/core.ts | 6 +-- core/protocol/core.ts | 2 +- core/protocol/passThrough.ts | 2 +- .../runTerminalCommand.vitest.ts | 8 +--- core/util/processTerminalStates.test.ts | 39 +++++++++++++------ core/util/processTerminalStates.ts | 15 ++++--- .../constants/MessageTypes.kt | 3 +- .../Lump/LumpToolbar/LumpToolbar.tsx | 9 ++--- .../Lump/LumpToolbar/StreamingToolbar.tsx | 13 ++----- gui/src/redux/util/constructMessages.ts | 26 ++++++------- 10 files changed, 64 insertions(+), 59 deletions(-) diff --git a/core/core.ts b/core/core.ts index f26dc1b6a..2357d8687 100644 --- a/core/core.ts +++ b/core/core.ts @@ -1056,11 +1056,11 @@ export class Core { }, ); - on("process/cancelTerminalCommand", async ({ data: { toolCallId } }) => { - const { cancelTerminalCommand } = await import( + on("process/killTerminalProcess", async ({ data: { toolCallId } }) => { + const { killTerminalProcess } = await import( "./util/processTerminalStates.js" ); - await cancelTerminalCommand(toolCallId); + await killTerminalProcess(toolCallId); }); on("mdm/setLicenseKey", ({ data: { licenseKey } }) => { diff --git a/core/protocol/core.ts b/core/protocol/core.ts index 2bd22c830..74243f195 100644 --- a/core/protocol/core.ts +++ b/core/protocol/core.ts @@ -313,6 +313,6 @@ export type ToCoreFromIdeOrWebviewProtocol = { ]; "process/markAsBackgrounded": [{ toolCallId: string }, void]; "process/isBackgrounded": [{ toolCallId: string }, boolean]; - "process/cancelTerminalCommand": [{ toolCallId: string }, void]; + "process/killTerminalProcess": [{ toolCallId: string }, void]; "mdm/setLicenseKey": [{ licenseKey: string }, boolean]; }; diff --git a/core/protocol/passThrough.ts b/core/protocol/passThrough.ts index ff0717244..8f109f46f 100644 --- a/core/protocol/passThrough.ts +++ b/core/protocol/passThrough.ts @@ -86,7 +86,7 @@ export const WEBVIEW_TO_CORE_PASS_THROUGH: (keyof ToCoreFromWebviewProtocol)[] = "isItemTooBig", "process/markAsBackgrounded", "process/isBackgrounded", - "process/cancelTerminalCommand", + "process/killTerminalProcess", ]; // Message types to pass through from core to webview diff --git a/core/tools/implementations/runTerminalCommand.vitest.ts b/core/tools/implementations/runTerminalCommand.vitest.ts index c26e5b64d..230d482f7 100644 --- a/core/tools/implementations/runTerminalCommand.vitest.ts +++ b/core/tools/implementations/runTerminalCommand.vitest.ts @@ -39,12 +39,8 @@ describe("runTerminalCommandImpl", () => { vi.resetAllMocks(); // Setup backgrounded processes handling - don't mock, just make sure it's empty - // Get any processes that might be already tracked and clear them - const processMap = (processTerminalStates as any) - .processTerminalBackgroundStates; - if (processMap && typeof processMap.clear === "function") { - processMap.clear(); - } + // Clear any processes that might be already tracked + processTerminalStates.clearAllBackgroundProcesses(); // Setup IDE mocks mockGetIdeInfo.mockReturnValue(Promise.resolve({ remoteName: "local" })); diff --git a/core/util/processTerminalStates.test.ts b/core/util/processTerminalStates.test.ts index 00f64ab4e..6b9e39f00 100644 --- a/core/util/processTerminalStates.test.ts +++ b/core/util/processTerminalStates.test.ts @@ -1,13 +1,14 @@ import { ChildProcess } from "child_process"; import { - cancelAllRunningTerminalCommands, - cancelMultipleTerminalCommands, - cancelTerminalCommand, + clearAllBackgroundProcesses, getAllBackgroundedProcessIds, getAllRunningProcessIds, getRunningProcess, isProcessBackgrounded, isProcessRunning, + killAllRunningTerminalProcesses, + killMultipleTerminalProcesses, + killTerminalProcess, markProcessAsBackgrounded, markProcessAsRunning, removeBackgroundedProcess, @@ -205,7 +206,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId, mockProcess); expect(isProcessRunning(toolCallId)).toBe(true); - await cancelTerminalCommand(toolCallId); + await killTerminalProcess(toolCallId); expect(mockProcess.kill).toHaveBeenCalledWith("SIGTERM"); expect(isProcessRunning(toolCallId)).toBe(false); @@ -214,7 +215,7 @@ describe("processTerminalStates", () => { test("should handle cancelling non-existent process", async () => { const toolCallId = "non-existent"; - await expect(cancelTerminalCommand(toolCallId)).resolves.not.toThrow(); + await expect(killTerminalProcess(toolCallId)).resolves.not.toThrow(); }); test("should handle cancelling already killed process", async () => { @@ -223,7 +224,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId, mockProcess); - await cancelTerminalCommand(toolCallId); + await killTerminalProcess(toolCallId); expect(mockProcess.kill).not.toHaveBeenCalled(); }); @@ -242,7 +243,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId, mockProcess); - const cancelPromise = cancelTerminalCommand(toolCallId); + const cancelPromise = killTerminalProcess(toolCallId); // Fast-forward time to trigger the timeout jest.advanceTimersByTime(5000); @@ -259,7 +260,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId, mockProcess); - const cancelPromise = cancelTerminalCommand(toolCallId); + const cancelPromise = killTerminalProcess(toolCallId); // Fast-forward time to trigger the timeout jest.advanceTimersByTime(5000); @@ -314,7 +315,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId1, mockProcess1); markProcessAsRunning(toolCallId2, mockProcess2); - await cancelMultipleTerminalCommands([toolCallId1, toolCallId2]); + await killMultipleTerminalProcesses([toolCallId1, toolCallId2]); expect(mockProcess1.kill).toHaveBeenCalledWith("SIGTERM"); expect(mockProcess2.kill).toHaveBeenCalledWith("SIGTERM"); @@ -331,7 +332,7 @@ describe("processTerminalStates", () => { markProcessAsRunning(toolCallId1, mockProcess1); markProcessAsRunning(toolCallId2, mockProcess2); - const cancelledIds = await cancelAllRunningTerminalCommands(); + const cancelledIds = await killAllRunningTerminalProcesses(); expect(cancelledIds).toContain(toolCallId1); expect(cancelledIds).toContain(toolCallId2); @@ -343,8 +344,24 @@ describe("processTerminalStates", () => { }); test("should return empty array when no running commands to cancel", async () => { - const cancelledIds = await cancelAllRunningTerminalCommands(); + const cancelledIds = await killAllRunningTerminalProcesses(); expect(cancelledIds).toEqual([]); }); + + test("should clear all background processes", () => { + const toolCallId1 = "test-123"; + const toolCallId2 = "test-456"; + + markProcessAsBackgrounded(toolCallId1); + markProcessAsBackgrounded(toolCallId2); + + expect(getAllBackgroundedProcessIds()).toHaveLength(2); + + clearAllBackgroundProcesses(); + + expect(getAllBackgroundedProcessIds()).toEqual([]); + expect(isProcessBackgrounded(toolCallId1)).toBe(false); + expect(isProcessBackgrounded(toolCallId2)).toBe(false); + }); }); }); diff --git a/core/util/processTerminalStates.ts b/core/util/processTerminalStates.ts index 922b9d7dc..cad243a82 100644 --- a/core/util/processTerminalStates.ts +++ b/core/util/processTerminalStates.ts @@ -67,7 +67,7 @@ export function removeRunningProcess(toolCallId: string): void { processTerminalForegroundStates.delete(toolCallId); } -export async function cancelTerminalCommand(toolCallId: string): Promise { +export async function killTerminalProcess(toolCallId: string): Promise { const processInfo = processTerminalForegroundStates.get(toolCallId); if (processInfo && !processInfo.process.killed) { const { process } = processInfo; @@ -86,20 +86,20 @@ export async function cancelTerminalCommand(toolCallId: string): Promise { } // Function to cancel multiple terminal commands at once -export async function cancelMultipleTerminalCommands( +export async function killMultipleTerminalProcesses( toolCallIds: string[], ): Promise { const cancelPromises = toolCallIds.map((toolCallId) => - cancelTerminalCommand(toolCallId), + killTerminalProcess(toolCallId), ); await Promise.all(cancelPromises); } // Function to cancel ALL currently running terminal commands -export async function cancelAllRunningTerminalCommands(): Promise { +export async function killAllRunningTerminalProcesses(): Promise { const runningIds = getAllRunningProcessIds(); if (runningIds.length > 0) { - await cancelMultipleTerminalCommands(runningIds); + await killMultipleTerminalProcesses(runningIds); } return runningIds; // Return the IDs that were cancelled } @@ -112,3 +112,8 @@ export function getAllRunningProcessIds(): string[] { export function getAllBackgroundedProcessIds(): string[] { return Array.from(processTerminalBackgroundStates.keys()); } + +// Utility function for testing - clears all background process states +export function clearAllBackgroundProcesses(): void { + processTerminalBackgroundStates.clear(); +} diff --git a/extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/constants/MessageTypes.kt b/extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/constants/MessageTypes.kt index a400adb0a..63484d9eb 100644 --- a/extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/constants/MessageTypes.kt +++ b/extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/constants/MessageTypes.kt @@ -144,10 +144,9 @@ class MessageTypes { "tools/evaluatePolicy", "controlPlane/openUrl", "isItemTooBig", - "controlPlane/openUrl", "process/markAsBackgrounded", "process/isBackgrounded", - "process/cancelTerminalCommand", + "process/killTerminalProcess" ) } } diff --git a/gui/src/components/mainInput/Lump/LumpToolbar/LumpToolbar.tsx b/gui/src/components/mainInput/Lump/LumpToolbar/LumpToolbar.tsx index 245076d74..3127dd0a8 100644 --- a/gui/src/components/mainInput/Lump/LumpToolbar/LumpToolbar.tsx +++ b/gui/src/components/mainInput/Lump/LumpToolbar/LumpToolbar.tsx @@ -19,6 +19,7 @@ import { PendingApplyStatesToolbar } from "./PendingApplyStatesToolbar"; import { PendingToolCallToolbar } from "./PendingToolCallToolbar"; import { StreamingToolbar } from "./StreamingToolbar"; import { TtsActiveToolbar } from "./TtsActiveToolbar"; +import { BuiltInToolNames } from "core/tools/builtIn"; // Keyboard shortcut detection utilities const isExecuteToolCallShortcut = (event: KeyboardEvent) => { @@ -38,11 +39,7 @@ const isCancelToolCallShortcut = ( // Check if a tool call is a terminal command const isTerminalCommand = (toolCallState: any) => { - return ( - toolCallState?.toolCall?.function?.name === "run_terminal_command" || - toolCallState?.toolCall?.function?.name === "Bash" || - toolCallState?.toolCall?.function?.name === "runTerminalCommand" - ); + return BuiltInToolNames.RunTerminalCommand; }; export function LumpToolbar() { @@ -82,7 +79,7 @@ export function LumpToolbar() { const stopPromises = runningTerminalCalls.map(async (terminalCall) => { try { // Cancel the process on the backend - await ideMessenger.request("process/cancelTerminalCommand", { + await ideMessenger.request("process/killTerminalProcess", { toolCallId: terminalCall.toolCallId, }); diff --git a/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx b/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx index 15e7334ab..66d35f3cb 100644 --- a/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx +++ b/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx @@ -1,26 +1,19 @@ -import { useAppDispatch } from "../../../../redux/hooks"; -import { cancelStream } from "../../../../redux/thunks/cancelStream"; import { getAltKeyLabel, getMetaKeyLabel, isJetBrains } from "../../../../util"; import { GeneratingIndicator } from "./GeneratingIndicator"; interface StreamingToolbarProps { - onStop?: () => void; + onStop: () => void; displayText?: string; } export function StreamingToolbar({ onStop, displayText = "Stop", -}: StreamingToolbarProps = {}) { +}: StreamingToolbarProps) { const jetbrains = isJetBrains(); - const dispatch = useAppDispatch(); const handleStop = () => { - if (onStop) { - onStop(); - } else { - void dispatch(cancelStream()); - } + onStop(); }; return ( diff --git a/gui/src/redux/util/constructMessages.ts b/gui/src/redux/util/constructMessages.ts index c36e1ccad..666a1bfd9 100644 --- a/gui/src/redux/util/constructMessages.ts +++ b/gui/src/redux/util/constructMessages.ts @@ -17,26 +17,24 @@ import { import { convertToolCallStatesToSystemCallsAndOutput } from "core/tools/systemMessageTools/convertSystemTools"; import { SystemMessageToolsFramework } from "core/tools/systemMessageTools/types"; import { findLast, findLastIndex } from "core/util/findLast"; -import { - normalizeToMessageParts, - renderContextItems, -} from "core/util/messageContent"; +import { normalizeToMessageParts } from "core/util/messageContent"; import { toolCallStateToContextItems } from "../../pages/gui/ToolCallDiv/utils"; +// Helper function to render context items and append status information // Helper function to render context items and append status information function renderContextItemsWithStatus(contextItems: any[]): string { - const content = renderContextItems(contextItems); + return contextItems + .map((item) => { + let result = item.content; - // Check if any context items have status information - const statusItems = contextItems - .filter((item) => item.status) - .map((item) => `Status: ${item.status}`); + // If this item has a status, append it directly after the content + if (item.status) { + result += `\n[Status: ${item.status}]`; + } - if (statusItems.length > 0) { - return `${content}\n\n${statusItems.join("\n")}`; - } - - return content; + return result; + }) + .join("\n\n"); } interface MessageWithContextItems { ctxItems: ContextItemWithId[];