fix: 🐛 Resolve comments from review

2025-09-09 23:38:44 -07:00
parent 3ca3897463
commit 0660687653
5 changed files with 373 additions and 23 deletions
--- a/core/core.ts
+++ b/core/core.ts
@@ -29,6 +29,7 @@ import { editConfigFile, migrateV1DevDataFiles } from "./util/paths";
 import { Telemetry } from "./util/posthog";
 import {
  isProcessBackgrounded,
+  killTerminalProcess,
  markProcessAsBackgrounded,
 } from "./util/processTerminalStates";
 import { getSymbolsForManyFiles } from "./util/treeSitter";
@@ -1057,9 +1058,6 @@ export class Core {
    );

    on("process/killTerminalProcess", async ({ data: { toolCallId } }) => {
-      const { killTerminalProcess } = await import(
-        "./util/processTerminalStates.js"
-      );
      await killTerminalProcess(toolCallId);
    });

--- a/core/util/messageContent.ts
+++ b/core/util/messageContent.ts
@@ -35,6 +35,21 @@ export function renderContextItems(contextItems: ContextItem[]): string {
  return contextItems.map((item) => item.content).join("\n\n");
 }

+export function renderContextItemsWithStatus(contextItems: any[]): string {
+  return contextItems
+    .map((item) => {
+      let result = item.content;
+
+      // If this item has a status, append it directly after the content
+      if (item.status) {
+        result += `\n[Status: ${item.status}]`;
+      }
+
+      return result;
+    })
+    .join("\n\n");
+}
+
 export function normalizeToMessageParts(message: ChatMessage): MessagePart[] {
  switch (message.role) {
    case "user":
--- a/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx
+++ b/gui/src/components/mainInput/Lump/LumpToolbar/StreamingToolbar.tsx
@@ -12,15 +12,11 @@ export function StreamingToolbar({
 }: StreamingToolbarProps) {
  const jetbrains = isJetBrains();

-  const handleStop = () => {
-    onStop();
-  };
-
  return (
    <div className="flex w-full items-center justify-between">
      <GeneratingIndicator />
      <div
-        onClick={handleStop}
+        onClick={onStop}
        className="text-2xs cursor-pointer px-1.5 py-0.5 hover:brightness-125"
      >
        <span className="text-description">{displayText}</span>
--- a/gui/src/redux/util/constructMessages.test.ts
+++ b/gui/src/redux/util/constructMessages.test.ts
@@ -8,6 +8,7 @@ import {
  ToolResultChatMessage,
  UserChatMessage,
 } from "core";
+import { BuiltInToolNames } from "core/tools/builtIn";
 import {
  CANCELLED_TOOL_CALL_MESSAGE,
  NO_TOOL_CALL_OUTPUT_MESSAGE,
@@ -712,4 +713,346 @@ describe("constructMessages", () => {
    expect(messages[0].content).toContain("Base System Message");
    expect(messages[0].content).not.toContain(LAST_MESSAGE_RULE.rule);
  });
+
+  // Tests for the specific block in lines 135-142 handling toolCallState.output
+  describe("toolCallState.output handling (lines 135-142)", () => {
+    test("should use CANCELLED_TOOL_CALL_MESSAGE for cancelled tool calls", () => {
+      const assistantWithToolCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll run a command",
+        toolCalls: [
+          {
+            id: "tool-call-1",
+            type: "function",
+            function: {
+              name: "some_tool",
+              arguments: '{"command": "ls"}',
+            },
+          },
+        ],
+      };
+
+      mockHistory = [
+        {
+          message: assistantWithToolCall,
+          contextItems: [],
+          toolCallStates: [
+            {
+              toolCallId: "tool-call-1",
+              toolCall: {
+                id: "tool-call-1",
+                type: "function",
+                function: {
+                  name: "some_tool",
+                  arguments: '{"command": "ls"}',
+                },
+              },
+              status: "canceled",
+              parsedArgs: { command: "ls" },
+              output: [
+                createContextItem(
+                  "result",
+                  "This should be ignored due to cancelled status",
+                ),
+              ],
+            },
+          ],
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.content).toBe(CANCELLED_TOOL_CALL_MESSAGE);
+      expect(toolMessage.content).not.toContain("This should be ignored");
+    });
+
+    test("should use renderContextItemsWithStatus for RunTerminalCommand with output", () => {
+      const assistantWithTerminalCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll run a terminal command",
+        toolCalls: [
+          {
+            id: "terminal-call-1",
+            type: "function",
+            function: {
+              name: BuiltInToolNames.RunTerminalCommand,
+              arguments: '{"command": "ls -la"}',
+            },
+          },
+        ],
+      };
+
+      const terminalOutputWithStatus = [
+        {
+          ...createContextItem("terminal-output", "file1.txt\nfile2.txt"),
+          status: "completed",
+        },
+        {
+          ...createContextItem("terminal-error", "Warning: deprecated flag"),
+          status: "warning",
+        },
+      ];
+
+      mockHistory = [
+        {
+          message: assistantWithTerminalCall,
+          contextItems: [],
+          toolCallStates: [
+            {
+              toolCallId: "terminal-call-1",
+              toolCall: {
+                id: "terminal-call-1",
+                type: "function",
+                function: {
+                  name: BuiltInToolNames.RunTerminalCommand,
+                  arguments: '{"command": "ls -la"}',
+                },
+              },
+              status: "done",
+              parsedArgs: { command: "ls -la" },
+              output: terminalOutputWithStatus,
+            },
+          ],
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.toolCallId).toBe("terminal-call-1");
+      // Should contain content with status appended
+      expect(toolMessage.content).toContain("file1.txt\nfile2.txt");
+      expect(toolMessage.content).toContain("[Status: completed]");
+      expect(toolMessage.content).toContain("Warning: deprecated flag");
+      expect(toolMessage.content).toContain("[Status: warning]");
+      // Should use renderContextItemsWithStatus format with double newlines between items
+      expect(toolMessage.content).toMatch(
+        /file1\.txt[\s\S]*\[Status: completed\][\s\S]*\n\n[\s\S]*Warning.*\[Status: warning\]/,
+      );
+    });
+
+    test("should use renderContextItems for non-RunTerminalCommand tools with output", () => {
+      const assistantWithSearchCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll search for that",
+        toolCalls: [
+          {
+            id: "search-call-1",
+            type: "function",
+            function: {
+              name: "search", // Not RunTerminalCommand
+              arguments: '{"query": "test"}',
+            },
+          },
+        ],
+      };
+
+      const searchOutput = [
+        createContextItem("result1", "First search result"),
+        createContextItem("result2", "Second search result"),
+      ];
+
+      mockHistory = [
+        {
+          message: assistantWithSearchCall,
+          contextItems: [],
+          toolCallStates: [
+            {
+              toolCallId: "search-call-1",
+              toolCall: {
+                id: "search-call-1",
+                type: "function",
+                function: {
+                  name: "search",
+                  arguments: '{"query": "test"}',
+                },
+              },
+              status: "done",
+              parsedArgs: { query: "test" },
+              output: searchOutput,
+            },
+          ],
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.toolCallId).toBe("search-call-1");
+      // Should use renderContextItems format (no status, double newlines between items)
+      expect(toolMessage.content).toBe(
+        "First search result\n\nSecond search result",
+      );
+      expect(toolMessage.content).not.toContain("[Status:");
+    });
+
+    test("should use NO_TOOL_CALL_OUTPUT_MESSAGE when no output exists", () => {
+      const assistantWithToolCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll search for that",
+        toolCalls: [
+          {
+            id: "search-call-1",
+            type: "function",
+            function: {
+              name: "search",
+              arguments: '{"query": "test"}',
+            },
+          },
+        ],
+      };
+
+      mockHistory = [
+        {
+          message: assistantWithToolCall,
+          contextItems: [],
+          toolCallStates: [
+            {
+              toolCallId: "search-call-1",
+              toolCall: {
+                id: "search-call-1",
+                type: "function",
+                function: {
+                  name: "search",
+                  arguments: '{"query": "test"}',
+                },
+              },
+              status: "generating",
+              parsedArgs: { query: "test" },
+              // No output field
+            },
+          ],
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.toolCallId).toBe("search-call-1");
+      expect(toolMessage.content).toBe(NO_TOOL_CALL_OUTPUT_MESSAGE);
+    });
+
+    test("should use NO_TOOL_CALL_OUTPUT_MESSAGE when toolCallState is undefined", () => {
+      const assistantWithToolCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll search for that",
+        toolCalls: [
+          {
+            id: "search-call-1",
+            type: "function",
+            function: {
+              name: "search",
+              arguments: '{"query": "test"}',
+            },
+          },
+        ],
+      };
+
+      mockHistory = [
+        {
+          message: assistantWithToolCall,
+          contextItems: [],
+          // No toolCallStates array
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.toolCallId).toBe("search-call-1");
+      expect(toolMessage.content).toBe(NO_TOOL_CALL_OUTPUT_MESSAGE);
+    });
+
+    test("should prioritize cancelled status over output content", () => {
+      const assistantWithTerminalCall: AssistantChatMessage = {
+        role: "assistant",
+        content: "I'll run a command",
+        toolCalls: [
+          {
+            id: "terminal-call-1",
+            type: "function",
+            function: {
+              name: BuiltInToolNames.RunTerminalCommand,
+              arguments: '{"command": "ls"}',
+            },
+          },
+        ],
+      };
+
+      mockHistory = [
+        {
+          message: assistantWithTerminalCall,
+          contextItems: [],
+          toolCallStates: [
+            {
+              toolCallId: "terminal-call-1",
+              toolCall: {
+                id: "terminal-call-1",
+                type: "function",
+                function: {
+                  name: BuiltInToolNames.RunTerminalCommand,
+                  arguments: '{"command": "ls"}',
+                },
+              },
+              status: "canceled", // Cancelled status
+              parsedArgs: { command: "ls" },
+              output: [
+                createContextItem(
+                  "terminal-output",
+                  "This output should be ignored",
+                ),
+              ], // Has output but is cancelled
+            },
+          ],
+        },
+      ];
+
+      const { messages } = constructMessages(
+        mockHistory,
+        "Base System Message",
+        mockRules,
+        {},
+      );
+
+      const toolMessage = messages[2] as ToolResultChatMessage;
+      expect(toolMessage.role).toBe("tool");
+      expect(toolMessage.toolCallId).toBe("terminal-call-1");
+      // Should use cancelled message, not the output content
+      expect(toolMessage.content).toBe(CANCELLED_TOOL_CALL_MESSAGE);
+      expect(toolMessage.content).not.toContain(
+        "This output should be ignored",
+      );
+    });
+  });
 });
--- a/gui/src/redux/util/constructMessages.ts
+++ b/gui/src/redux/util/constructMessages.ts
@@ -10,6 +10,7 @@ import {
 import { chatMessageIsEmpty } from "core/llm/messages";
 import { getSystemMessageWithRules } from "core/llm/rules/getSystemMessageWithRules";
 import { RulePolicies } from "core/llm/rules/types";
+import { BuiltInToolNames } from "core/tools/builtIn";
 import {
  CANCELLED_TOOL_CALL_MESSAGE,
  NO_TOOL_CALL_OUTPUT_MESSAGE,
@@ -17,25 +18,16 @@ import {
 import { convertToolCallStatesToSystemCallsAndOutput } from "core/tools/systemMessageTools/convertSystemTools";
 import { SystemMessageToolsFramework } from "core/tools/systemMessageTools/types";
 import { findLast, findLastIndex } from "core/util/findLast";
-import { normalizeToMessageParts } from "core/util/messageContent";
+import {
+  normalizeToMessageParts,
+  renderContextItems,
+  renderContextItemsWithStatus,
+} 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 {
-  return contextItems
-    .map((item) => {
-      let result = item.content;

-      // If this item has a status, append it directly after the content
-      if (item.status) {
-        result += `\n[Status: ${item.status}]`;
-      }
-
-      return result;
-    })
-    .join("\n\n");
-}
 interface MessageWithContextItems {
  ctxItems: ContextItemWithId[];
  message: ChatMessage;
@@ -146,8 +138,14 @@ export function constructMessages(

          if (toolCallState?.status === "canceled") {
            content = CANCELLED_TOOL_CALL_MESSAGE;
-          } else if (toolCallState?.output) {
+          } else if (
+            toolCallState?.output &&
+            toolCall.function?.name == BuiltInToolNames.RunTerminalCommand
+          ) {
+            // Add status for tools containing detailed status outcomes per context item
            content = renderContextItemsWithStatus(toolCallState.output);
+          } else if (toolCallState?.output) {
+            content = renderContextItems(toolCallState?.output);
          }

          msgs.push({