Address code review: tighter guards, default ResultType, session cleanup

- Node: validate resultType against allowed values in isToolResultObject
- Go: default empty ResultType to 'success' (or 'failure' when error set)
- Python: use _from_exception attribute instead of sentinel string match
- Python/Go: disconnect sessions in e2e tests to avoid leaking state

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Morabbin

go/internal/e2e/tool_results_test.go

@@ -53,6 +53,10 @@ func TestToolResults(t *testing.T) {
 		if !strings.Contains(strings.ToLower(content), "sunny") && !strings.Contains(content, "72") {
 			t.Errorf("Expected answer to mention sunny or 72, got %q", content)
 		}
+
+		if err := session.Disconnect(); err != nil {
+			t.Errorf("Failed to disconnect session: %v", err)
+		}
 	})
 
 	t.Run("should handle tool result with failure resulttype", func(t *testing.T) {
@@ -97,6 +101,10 @@ func TestToolResults(t *testing.T) {
 		if !strings.Contains(strings.ToLower(content), "service is down") {
 			t.Errorf("Expected 'service is down', got %q", content)
 		}
+
+		if err := session.Disconnect(); err != nil {
+			t.Errorf("Failed to disconnect session: %v", err)
+		}
 	})
 
 	t.Run("should preserve tooltelemetry and not stringify structured results for llm", func(t *testing.T) {
@@ -167,5 +175,9 @@ func TestToolResults(t *testing.T) {
 		if strings.Contains(toolResults[0].Content, "resultType") {
 			t.Error("Tool result content should not contain 'resultType'")
 		}
+
+		if err := session.Disconnect(); err != nil {
+			t.Errorf("Failed to disconnect session: %v", err)
+		}
 	})
 }

go/session.go

@@ -625,15 +625,23 @@ func (s *Session) executeToolAndRespond(requestID, toolName, toolCallID string,
 		textResultForLLM = fmt.Sprintf("%v", result)
 	}
 
+	// Default ResultType to "success" when unset, or "failure" when there's an error.
+	effectiveResultType := result.ResultType
+	if effectiveResultType == "" {
+		if result.Error != "" {
+			effectiveResultType = "failure"
+		} else {
+			effectiveResultType = "success"
+		}
+	}
+
 	rpcResult := rpc.ResultUnion{
 		ResultResult: &rpc.ResultResult{
 			TextResultForLlm: textResultForLLM,
 			ToolTelemetry:    result.ToolTelemetry,
+			ResultType:       &effectiveResultType,
 		},
 	}
-	if result.ResultType != "" {
-		rpcResult.ResultResult.ResultType = &result.ResultType
-	}
 	if result.Error != "" {
 		rpcResult.ResultResult.Error = &result.Error
 	}

nodejs/src/session.ts

@@ -1050,15 +1050,27 @@ export class CopilotSession {
 
 /**
  * Type guard that checks whether a value is a {@link ToolResultObject}.
- * A valid object must have a string `textResultForLlm` and a string `resultType`.
+ * A valid object must have a string `textResultForLlm` and a recognized `resultType`.
  */
 function isToolResultObject(value: unknown): value is ToolResultObject {
-    return (
-        typeof value === "object" &&
-        value !== null &&
-        "textResultForLlm" in value &&
-        typeof (value as ToolResultObject).textResultForLlm === "string" &&
-        "resultType" in value &&
-        typeof (value as ToolResultObject).resultType === "string"
-    );
+    if (typeof value !== "object" || value === null) {
+        return false;
+    }
+
+    if (!("textResultForLlm" in value) || typeof (value as ToolResultObject).textResultForLlm !== "string") {
+        return false;
+    }
+
+    if (!("resultType" in value) || typeof (value as ToolResultObject).resultType !== "string") {
+        return false;
+    }
+
+    const allowedResultTypes: Array<ToolResultObject["resultType"]> = [
+        "success",
+        "failure",
+        "rejected",
+        "denied",
+    ];
+
+    return allowedResultTypes.includes((value as ToolResultObject).resultType);
 }

python/copilot/session.py

@@ -38,7 +38,7 @@
     SessionEventType,
     session_event_from_dict,
 )
-from .tools import TOOL_EXCEPTION_TEXT, Tool, ToolHandler, ToolInvocation, ToolResult
+from .tools import Tool, ToolHandler, ToolInvocation, ToolResult
 
 # Re-export SessionEvent under an alias used internally
 SessionEventTypeAlias = SessionEvent
@@ -948,11 +948,7 @@ async def _execute_tool_and_respond(
             # sent via the top-level error param so the CLI formats them with its
             # standard "Failed to execute..." message. Deliberate user-returned
             # failures send the full structured result to preserve metadata.
-            if (
-                tool_result.result_type == "failure"
-                and tool_result.error
-                and tool_result.text_result_for_llm == TOOL_EXCEPTION_TEXT
-            ):
+            if getattr(tool_result, "_from_exception", False):
                 await self.rpc.tools.handle_pending_tool_call(
                     SessionToolsHandlePendingToolCallParams(
                         request_id=request_id,

python/copilot/tools.py

@@ -17,11 +17,6 @@
 
 ToolResultType = Literal["success", "failure", "rejected", "denied"]
 
-# Constant used by define_tool's exception handler so that
-# _execute_tool_and_respond can detect exception-originated failures
-# and send them via the top-level error param (matching CLI formatting).
-TOOL_EXCEPTION_TEXT = "Invoking this tool produced an error. Detailed information is not available."
-
 
 @dataclass
 class ToolBinaryResult:
@@ -199,12 +194,20 @@ async def wrapped_handler(invocation: ToolInvocation) -> ToolResult:
             except Exception as exc:
                 # Don't expose detailed error information to the LLM for security reasons.
                 # The actual error is stored in the 'error' field for debugging.
-                return ToolResult(
-                    text_result_for_llm=TOOL_EXCEPTION_TEXT,
+                tr = ToolResult(
+                    text_result_for_llm=(
+                        "Invoking this tool produced an error. "
+                        "Detailed information is not available."
+                    ),
                     result_type="failure",
                     error=str(exc),
                     tool_telemetry={},
                 )
+                # Mark as exception-originated so _execute_tool_and_respond
+                # sends it via the top-level error param (matching CLI formatting)
+                # rather than as a structured result.
+                tr._from_exception = True  # type: ignore[attr-defined]
+                return tr
 
         return Tool(
             name=tool_name,

python/e2e/test_tool_results.py

@@ -30,12 +30,15 @@ def get_weather(params: WeatherParams, invocation: ToolInvocation) -> ToolResult
             on_permission_request=PermissionHandler.approve_all, tools=[get_weather]
         )
 
-        await session.send("What's the weather in Paris?")
-        assistant_message = await get_final_assistant_message(session)
-        assert (
-            "sunny" in assistant_message.data.content.lower()
-            or "72" in assistant_message.data.content
-        )
+        try:
+            await session.send("What's the weather in Paris?")
+            assistant_message = await get_final_assistant_message(session)
+            assert (
+                "sunny" in assistant_message.data.content.lower()
+                or "72" in assistant_message.data.content
+            )
+        finally:
+            await session.disconnect()
 
     async def test_should_handle_tool_result_with_failure_resulttype(self, ctx: E2ETestContext):
         @define_tool("check_status", description="Checks the status of a service")
@@ -50,12 +53,15 @@ def check_status(invocation: ToolInvocation) -> ToolResult:
             on_permission_request=PermissionHandler.approve_all, tools=[check_status]
         )
 
-        answer = await session.send_and_wait(
-            "Check the status of the service using check_status."
-            " If it fails, say 'service is down'."
-        )
-        assert answer is not None
-        assert "service is down" in answer.data.content.lower()
+        try:
+            answer = await session.send_and_wait(
+                "Check the status of the service using check_status."
+                " If it fails, say 'service is down'."
+            )
+            assert answer is not None
+            assert "service is down" in answer.data.content.lower()
+        finally:
+            await session.disconnect()
 
     async def test_should_preserve_tooltelemetry_and_not_stringify_structured_results_for_llm(
         self, ctx: E2ETestContext
@@ -78,14 +84,19 @@ def analyze_code(params: AnalyzeParams, invocation: ToolInvocation) -> ToolResul
             on_permission_request=PermissionHandler.approve_all, tools=[analyze_code]
         )
 
-        await session.send("Analyze the file main.ts for issues.")
-        assistant_message = await get_final_assistant_message(session)
-        assert "no issues" in assistant_message.data.content.lower()
-
-        # Verify the LLM received just textResultForLlm, not stringified JSON
-        traffic = await ctx.get_exchanges()
-        last_conversation = traffic[-1]
-        tool_results = [m for m in last_conversation["request"]["messages"] if m["role"] == "tool"]
-        assert len(tool_results) == 1
-        assert "toolTelemetry" not in tool_results[0]["content"]
-        assert "resultType" not in tool_results[0]["content"]
+        try:
+            await session.send("Analyze the file main.ts for issues.")
+            assistant_message = await get_final_assistant_message(session)
+            assert "no issues" in assistant_message.data.content.lower()
+
+            # Verify the LLM received just textResultForLlm, not stringified JSON
+            traffic = await ctx.get_exchanges()
+            last_conversation = traffic[-1]
+            tool_results = [
+                m for m in last_conversation["request"]["messages"] if m["role"] == "tool"
+            ]
+            assert len(tool_results) == 1
+            assert "toolTelemetry" not in tool_results[0]["content"]
+            assert "resultType" not in tool_results[0]["content"]
+        finally:
+            await session.disconnect()