From 4be85b27ad4ee7aaaff6586151a0d1701feead00 Mon Sep 17 00:00:00 2001 From: Zexin Yuan Date: Thu, 2 Jul 2026 13:05:26 +0800 Subject: [PATCH] Show a friendly error when the ImageJ plugin is not connected When no ImageJ plugin is connected to the bridge, plugin calls used to fail with an opaque "Plugin request failed: Client not found" and the chat stream ended silently, leaving the user with a generic error and no idea what to do. A dedicated PluginNotConnectedError now carries a curated, actionable message (start Fiji with the CopilotJ plugin, wait for it to connect, then resend) and is surfaced through the existing new:error channel as a red error post. The leader stops retrying the failing tool instead of burning all its retries, and prompt optimization degrades gracefully since ImageJ window info is optional context. Surrounding hardening so failures are never silent or leaky: - Plugin request failures (no client, timeouts, plugin-side errors) are normalized into a PluginRequestError hierarchy. Raw plugin/bridge error text is logged server-side only and kept out of exception messages, so it cannot flow into the model's retry observation or the UI. - _Thread._run_agent surfaces every agent crash to the user: the curated message for the not-connected case, a generic sanitized message otherwise. - The frontend new:error handler finalizes any in-progress agent post so a mid-stream crash cannot leave a hanging spinner. Tests cover the error mapping (bridge and HTTP paths, timeouts), the leader tool-exec short-circuit, executor propagation, crash surfacing/sanitization, and graceful prompt optimization. Assisted-by: Claude-Code:GLM-5.2 --- copilotj/core/ui.py | 1 + copilotj/multiagent/Executor.py | 10 + copilotj/multiagent/leader_multiagent.py | 43 +++-- copilotj/plugin/api.py | 74 ++++++- copilotj/server/bridge.py | 10 +- copilotj/server/threads.py | 16 +- .../test_executor_plugin_not_connected.py | 65 +++++++ .../test_leader_plugin_not_connected.py | 152 +++++++++++++++ copilotj/test/plugin/__init__.py | 3 + .../plugin/test_api_plugin_not_connected.py | 182 ++++++++++++++++++ .../server/test_threads_run_agent_errors.py | 87 +++++++++ web/src/store/thread.ts | 6 + 12 files changed, 624 insertions(+), 25 deletions(-) create mode 100644 copilotj/test/multiagent/test_executor_plugin_not_connected.py create mode 100644 copilotj/test/multiagent/test_leader_plugin_not_connected.py create mode 100644 copilotj/test/plugin/__init__.py create mode 100644 copilotj/test/plugin/test_api_plugin_not_connected.py create mode 100644 copilotj/test/server/test_threads_run_agent_errors.py diff --git a/copilotj/core/ui.py b/copilotj/core/ui.py index c2a8e161..f1478ba9 100644 --- a/copilotj/core/ui.py +++ b/copilotj/core/ui.py @@ -18,6 +18,7 @@ "CLI", "UIEventState", "UIEventPost", + "UIEventError", "UIEventPostReasoningChunk", "UIEventPostContentChunk", "UIEventToolCall", diff --git a/copilotj/multiagent/Executor.py b/copilotj/multiagent/Executor.py index 4e873db3..95a55862 100644 --- a/copilotj/multiagent/Executor.py +++ b/copilotj/multiagent/Executor.py @@ -7,6 +7,7 @@ from copilotj.core import ChatAgent, ModelClient, ModelSyntaxError, TextMessage, Tool from copilotj.multiagent.leader_prompts import build_tool_prompt +from copilotj.plugin.api import PluginNotConnectedError __all__ = ["Executor"] @@ -140,6 +141,12 @@ async def run(self, task: str) -> str: } ) + except PluginNotConnectedError: + # No ImageJ plugin connected: propagate immediately so the leader's + # tool-exec short-circuit surfaces the curated message instead of + # burning retries here. + raise + except Exception as e: error_msg = f"❌ Error executing action: {str(e)}" self.log_error(error_msg) @@ -160,6 +167,9 @@ async def run(self, task: str) -> str: return self._generate_final_summary(conversation_context) + except PluginNotConnectedError: + # Let it propagate to the leader's tool-exec short-circuit (do not stringify). + raise except Exception as e: self.log_error(f"Executor error: {e}") return f"❌ {self.name} encountered an error: {str(e)}" diff --git a/copilotj/multiagent/leader_multiagent.py b/copilotj/multiagent/leader_multiagent.py index 95d35f1d..6d2f7df7 100644 --- a/copilotj/multiagent/leader_multiagent.py +++ b/copilotj/multiagent/leader_multiagent.py @@ -65,6 +65,7 @@ ) from copilotj.multiagent.tools import system_info from copilotj.plugin import ClientPluginAPI +from copilotj.plugin.api import PluginNotConnectedError, PluginRequestError from copilotj.util import ReActChatCompletionClient __all__ = ["LeaderDriven"] @@ -194,6 +195,21 @@ async def _build_system_prompt(self) -> str: default_image_path=str(py_tools.get_project_temp_dir()), ) + async def _safe_window_info(self) -> str: + """Best-effort ImageJ window info for prompt context. + + Window info is optional context; any plugin failure (no client connected, + timeout, plugin-side error) is tolerated as an empty string so dialog entry + and prompt optimization degrade gracefully instead of crashing the run. + Returns ``""`` when ``plugin_tools`` is absent (e.g. a stripped-down leader). + """ + if not hasattr(self, "plugin_tools"): + return "" + try: + return await self.plugin_tools.imagej_windowInfo() + except PluginRequestError: + return "" + async def begin_dialog(self, main_task: str, *, trace_ctx: Langfuse | None = None) -> ModelResponse: """Start a new dialog, seeding the conversation with system + initial user turn. @@ -202,7 +218,7 @@ async def begin_dialog(self, main_task: str, *, trace_ctx: Langfuse | None = Non here and then retained in ``self._dialog_messages`` so subsequent steps only append new turns rather than resending the whole prompt. """ - self.imagej_windowInfo_text = await self.plugin_tools.imagej_windowInfo() + self.imagej_windowInfo_text = await self._safe_window_info() system_prompt = await self._build_system_prompt() initial_user = build_initial_user_message( @@ -235,7 +251,7 @@ async def continue_dialog( if assistant_text: self._dialog_messages.append(TextMessage(role="assistant", text=assistant_text)) - self.imagej_windowInfo_text = await self.plugin_tools.imagej_windowInfo() + self.imagej_windowInfo_text = await self._safe_window_info() user_text = build_observation_message( tool_response=observation, imagej_window_info=self.imagej_windowInfo_text, @@ -303,7 +319,7 @@ async def delegate_task(self, agent: str, task: str) -> str: agent_instance = self.agents[agent] self.log_info(f"[CALL] Calling Agent: {agent} | Params/Task: {task}") - self.imagej_windowInfo_text = await self.plugin_tools.imagej_windowInfo() + self.imagej_windowInfo_text = await self._safe_window_info() context = f"{task}{self.imagej_windowInfo_text}Previous Chat History: \n{self._prompt_chat_history_json()}" return await agent_instance.run(context) @@ -328,12 +344,7 @@ async def optimize_prompt(self, user_prompt: str) -> str: """ # Get ImageJ window info for context (only if available) # This is read-only, no side effects - window_info = "" - try: - if hasattr(self, "plugin_tools"): - window_info = await self.plugin_tools.imagej_windowInfo() - except Exception: - window_info = "" + window_info = await self._safe_window_info() # Build context section context_parts = [] @@ -511,11 +522,7 @@ async def optimize_prompt(self, user_prompt: str) -> str: """ # Get ImageJ window info for context (only if available) - window_info = "" - try: - window_info = await self.leader_agent.plugin_tools.imagej_windowInfo() - except Exception: - window_info = "" + window_info = await self.leader_agent._safe_window_info() # Build context section context_parts = [] @@ -850,6 +857,14 @@ async def run(self, task: str, trace_ctx: Langfuse | None = None) -> None: resp = await self.leader_agent._call_tool(tool_call) tool_retry_counter = 0 # Reset retry counter on success + except PluginNotConnectedError as e: + # No ImageJ plugin connected: don't burn retries on a tool that can't + # succeed. Surface the curated message to the user and end the dialog. + await self.leader_agent.print_error(str(e)) + dialog_context["status"] = "failed" + dialog_context["steps"].append({"name": tool_call.tool.name, "error": str(e)}) + break + except Exception as e: self.log_error(f"[ERROR] Error executing '{tool_call.tool.name}': {e}") resp = f""" diff --git a/copilotj/plugin/api.py b/copilotj/plugin/api.py index 9a0a5e4b..e82d0ad8 100644 --- a/copilotj/plugin/api.py +++ b/copilotj/plugin/api.py @@ -3,6 +3,7 @@ # SPDX-License-Identifier: Apache-2.0 import abc +import logging import uuid import warnings from datetime import datetime @@ -31,7 +32,47 @@ if TYPE_CHECKING: from copilotj.server import Bridge -__all__ = ["PluginAPI", "HTTPPluginAPI", "BridgePluginAPI", "ClientPluginAPI"] +_log = logging.getLogger(__name__) + +__all__ = [ + "PluginAPI", + "HTTPPluginAPI", + "BridgePluginAPI", + "ClientPluginAPI", + "PluginRequestError", + "PluginNotConnectedError", + "CLIENT_NOT_FOUND_ERR", +] + +# Bridge error string returned when no ImageJ plugin client is connected. Shared with +# ``copilotj.server.bridge`` so the producer and the consumer agree on the literal. +CLIENT_NOT_FOUND_ERR = "Client not found" + + +class PluginRequestError(RuntimeError): + """Any failure of a plugin request (no client connected, timeout, plugin-side error). + + Catch this base class where plugin failures should be tolerated (e.g. optional + ImageJ window-info lookups). Catch :class:`PluginNotConnectedError` where the + "plugin not connected" case specifically should short-circuit. + """ + + +class PluginNotConnectedError(PluginRequestError): + """No ImageJ plugin client is connected to the bridge. + + Carries a curated, user-facing message (``DEFAULT_MESSAGE``) so the agent/UI layer + can surface it directly without leaking internal detail. + """ + + DEFAULT_MESSAGE = ( + "ImageJ is not connected. Please start Fiji with the CopilotJ plugin " + "installed and wait for it to connect to the server, then resend your request." + ) + + def __init__(self, message: str | None = None) -> None: + super().__init__(message or self.DEFAULT_MESSAGE) + ########################################################## @@ -185,11 +226,29 @@ async def _request[R: Response](self, client_id: uuid.UUID, data: Request[R], *, "/api/plugins/events", data=bytes(request, "utf-8"), headers={"Content-Type": "application/json"} ) as resp: respStr = await resp.text() + # The bridge returns HTTP 500 + a BridgeResponse body (err=...) for plugin-side + # failures such as "Client not found". Parse the body even on >=400 so those map + # to the typed plugin errors instead of a generic Exception (the status check + # used to fire first, hiding the not-connected mapping). + result: BridgeResponse | None + try: + result = BridgeResponse.model_validate_json(respStr) + except Exception: + result = None + err = result.err if result is not None else None + if err == CLIENT_NOT_FOUND_ERR: + raise PluginNotConnectedError() if resp.status >= 400: - raise Exception(f"Plugin API request failed with {resp.status}: {respStr}") - result = BridgeResponse.model_validate_json(respStr) - if result.err: - raise Exception(result.err) + # Body may be plugin/proxy-controlled (echoed paths, script text) — log it + # server-side only, keep the exception message free of raw response text so + # it can't flow into the LLM retry observation or the UI. + _log.warning("Plugin API request failed: status=%s body=%s", resp.status, respStr) + raise PluginRequestError(f"Plugin API request failed with HTTP {resp.status}") + if err: + _log.warning("Plugin request returned error: %s", err) + raise PluginRequestError("Plugin request failed") + if result is None: + raise PluginRequestError("Plugin API returned a malformed response") return cast(R, data.response_type.model_validate(result.data)) @override @@ -212,7 +271,10 @@ async def _request[R: Response](self, client_id: uuid.UUID, data: Request[R], *, BridgeRequest(client_id=client_id, event=data.event, data=data, timeout=timeout) ) if response.err: - raise RuntimeError(f"Plugin request failed: {response.err}") + if response.err == CLIENT_NOT_FOUND_ERR: + raise PluginNotConnectedError() + _log.warning("Plugin request failed: %s", response.err) + raise PluginRequestError("Plugin request failed") adapter = TypeAdapter(data.response_type) return cast(R, adapter.validate_python(response.data)) diff --git a/copilotj/server/bridge.py b/copilotj/server/bridge.py index 6f990ae1..6892315f 100644 --- a/copilotj/server/bridge.py +++ b/copilotj/server/bridge.py @@ -13,6 +13,7 @@ import pydantic from copilotj.core.config import SINGLE_CLIENT_ID, is_single_client +from copilotj.plugin.api import CLIENT_NOT_FOUND_ERR, PluginRequestError from copilotj.util import Base64ImageTruncator, IndentedRawJson __all__ = ["BridgeRequest", "BridgeResponse", "Bridge"] @@ -141,7 +142,10 @@ async def send_event(self, event: str, data: Any, *, timeout: float = DEFAULT_TI return await asyncio.wait_for(future, timeout=timeout) except asyncio.TimeoutError: _log.warning("Timeout waiting for response to event: %s (id: %s)", event, event_id) - raise Exception(f"Timeout waiting for response to event: {event} (id: {event_id})") + # Normalize into the plugin-error hierarchy so callers that tolerate + # PluginRequestError (e.g. optional window-info lookups) also tolerate + # timeouts, instead of getting a bare Exception. + raise PluginRequestError(f"Timeout waiting for response to event: {event} (id: {event_id})") from None finally: # Clean up the registered event @@ -256,8 +260,8 @@ async def send_event(self, req: BridgeRequest) -> BridgeResponse: client = self._clients.get(client_id) if client is None: - _log.error(f"Client not found: {req.client_id}") - return BridgeResponse(err="Client not found") + _log.error(f"{CLIENT_NOT_FOUND_ERR}: {req.client_id}") + return BridgeResponse(err=CLIENT_NOT_FOUND_ERR) # Forward the event timeout = req.timeout or DEFAULT_TIMEOUT diff --git a/copilotj/server/threads.py b/copilotj/server/threads.py index acaefedc..af5fd801 100644 --- a/copilotj/server/threads.py +++ b/copilotj/server/threads.py @@ -17,11 +17,11 @@ import pydantic from langfuse import propagate_attributes -from copilotj.core import UI, UIEvent, UIEventPost, UIEventState +from copilotj.core import UI, UIEvent, UIEventError, UIEventPost, UIEventState from copilotj.core.config import Config, resolve_vision_config from copilotj.core.ui import UIEventContentMarkdown from copilotj.multiagent.leader_multiagent import LeaderDriven -from copilotj.plugin.api import BridgePluginAPI, PluginAPI +from copilotj.plugin.api import BridgePluginAPI, PluginAPI, PluginNotConnectedError if TYPE_CHECKING: from copilotj.server.bridge import Bridge @@ -31,6 +31,11 @@ # Timeout for thread locks in seconds (300ms) THREAD_LOCK_TIMEOUT = 0.3 +# Generic, leakage-safe message shown to the user when the agent crashes on an +# unexpected error. Exception text is kept in server logs only (it may contain +# provider payloads, file paths, or script content). +GENERIC_AGENT_ERROR_MSG = "The agent stopped unexpectedly. Please check the server logs or try again." + _log = logging.getLogger(__name__) # --------------------------------------------------------------------------- @@ -338,9 +343,16 @@ async def _run_agent(self, prompt: str, done_event: asyncio.Event) -> None: ): await self._agent.run(prompt, trace_ctx=self._trace_ctx) + except PluginNotConnectedError as e: + # Curated, actionable message — safe to show verbatim. + _log.warning("Plugin not connected for thread %s", self.thread_id) + await self.send(UIEventError(role="system", data=str(e))) except Exception: + # Unknown crash: log full detail, show only a generic message so provider + # payloads / file paths / script text never leak to the frontend. _log.exception("Agent run failed for thread %s", self.thread_id) self._agent.log_error(f"Agent run failed for thread {self.thread_id}:\n{traceback.format_exc()}") + await self.send(UIEventError(role="system", data=GENERIC_AGENT_ERROR_MSG)) finally: done_event.set() # Signal that the chat is done diff --git a/copilotj/test/multiagent/test_executor_plugin_not_connected.py b/copilotj/test/multiagent/test_executor_plugin_not_connected.py new file mode 100644 index 00000000..2f153614 --- /dev/null +++ b/copilotj/test/multiagent/test_executor_plugin_not_connected.py @@ -0,0 +1,65 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for Executor propagation of PluginNotConnectedError. + +When a delegated agent's tool fails because no plugin is connected, the Executor +must re-raise PluginNotConnectedError out of ``run()`` (skipping its retry loop) +so the leader's tool-exec short-circuit can surface the curated message. A generic +tool error still goes through the existing retry path and returns a string. +""" + +import asyncio +from types import SimpleNamespace + +import pytest + +from copilotj.multiagent.Executor import Executor +from copilotj.plugin.api import PluginNotConnectedError + + +def _make_executor(*, call_tool_exc: BaseException) -> Executor: + """An Executor bypassing __init__, with _create/_call_tool stubbed.""" + exe = Executor.__new__(Executor) + exe.name = "test_executor" + exe.tools = [] + exe.system_prompt = "" + exe.max_iterations = 15 + exe.tool_retry_counter = 0 + exe.max_tool_retry = 3 + + fake_tool_call = SimpleNamespace(tool=SimpleNamespace(name="take_snapshot"), args=SimpleNamespace()) + fake_response = SimpleNamespace(content=None, reasoning_content=None, tool_calls=[fake_tool_call]) + + async def _create(*_a: object, **_k: object) -> SimpleNamespace: + return fake_response + + async def _call_tool(_tool_call: object) -> str: + raise call_tool_exc + + exe._create = _create # type: ignore[method-assign] + exe._call_tool = _call_tool # type: ignore[method-assign] + exe._build_execution_context = lambda _ctx, _i: "" # type: ignore[method-assign] + exe._is_task_complete = lambda _text: False # type: ignore[method-assign] + exe._suggest_tool_based_on_context = lambda _thought, _task: "" # type: ignore[method-assign] + exe._generate_final_summary = lambda _ctx: "summary" # type: ignore[method-assign] + exe.log_info = lambda *_a, **_k: None # type: ignore[method-assign] + exe.log_error = lambda *_a, **_k: None # type: ignore[method-assign] + return exe + + +def test_plugin_not_connected_propagates_out_of_run(): + exe = _make_executor(call_tool_exc=PluginNotConnectedError()) + + with pytest.raises(PluginNotConnectedError): + asyncio.run(exe.run("do something")) + + +def test_generic_tool_error_does_not_propagate(): + # A non-plugin error stays on the retry path and returns a string (existing behaviour). + exe = _make_executor(call_tool_exc=ValueError("not a plugin error")) + + result = asyncio.run(exe.run("do something")) + + assert isinstance(result, str) diff --git a/copilotj/test/multiagent/test_leader_plugin_not_connected.py b/copilotj/test/multiagent/test_leader_plugin_not_connected.py new file mode 100644 index 00000000..34f077a3 --- /dev/null +++ b/copilotj/test/multiagent/test_leader_plugin_not_connected.py @@ -0,0 +1,152 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for the leader's plugin-not-connected handling. + +C. LeaderDriven.run() short-circuits on PluginNotConnectedError at the tool-exec + boundary: it calls leader_agent.print_error with the curated message and breaks + (no retries). A generic PluginRequestError (e.g. timeout) does NOT short-circuit. +F. LeaderAgent._safe_window_info() tolerates any PluginRequestError (incl. the + PluginNotConnectedError subclass and timeouts) and returns "". +""" + +import asyncio +from types import SimpleNamespace + +from copilotj.multiagent.leader_multiagent import LeaderAgent, LeaderDriven +from copilotj.plugin.api import PluginNotConnectedError, PluginRequestError + + +# -------------------------------------------------------------------------------------------- +# F. _safe_window_info +# -------------------------------------------------------------------------------------------- + + +class _FakePluginTools: + def __init__(self, *, window_info: str = "info", exc: BaseException | None = None) -> None: + self._window_info = window_info + self._exc = exc + + async def imagej_windowInfo(self) -> str: + if self._exc is not None: + raise self._exc + return self._window_info + + +def _leader_with_plugin_tools(exc: BaseException | None) -> LeaderAgent: + agent = LeaderAgent.__new__(LeaderAgent) + agent.plugin_tools = _FakePluginTools(exc=exc) + return agent + + +def test_safe_window_info_tolerates_not_connected(): + agent = _leader_with_plugin_tools(PluginNotConnectedError()) + assert asyncio.run(agent._safe_window_info()) == "" + + +def test_safe_window_info_tolerates_timeout(): + agent = _leader_with_plugin_tools(PluginRequestError("Timeout waiting for response")) + assert asyncio.run(agent._safe_window_info()) == "" + + +def test_safe_window_info_returns_info_when_ok(): + agent = _leader_with_plugin_tools(None) + assert asyncio.run(agent._safe_window_info()) == "info" + + +def test_safe_window_info_returns_empty_when_no_plugin_tools(): + # A stripped-down leader without plugin_tools degrades to "" (no AttributeError). + agent = LeaderAgent.__new__(LeaderAgent) + assert asyncio.run(agent._safe_window_info()) == "" + + +# -------------------------------------------------------------------------------------------- +# C. run() tool-exec short-circuit +# -------------------------------------------------------------------------------------------- + + +class _FakeLeaderAgent: + """Leader stand-in: begin_dialog yields one tool call; _call_tool raises on demand.""" + + def __init__(self, *, call_tool_exc: BaseException | None) -> None: + self._call_tool_exc = call_tool_exc + self.print_errors: list[str] = [] + self.name = "leader" + + tool_call = SimpleNamespace( + tool=SimpleNamespace(name="take_snapshot"), args=SimpleNamespace(model_dump=lambda: {}) + ) + self._first_response = SimpleNamespace(content=None, reasoning_content="thinking", tool_calls=[tool_call]) + self._final_response = SimpleNamespace(content="done", reasoning_content=None, tool_calls=[]) + + async def begin_dialog(self, _task: str, *, trace_ctx: object = None) -> SimpleNamespace: # noqa: ARG002 + return self._first_response + + async def _call_tool(self, _tool_call: object) -> str: # noqa: ARG002 + if self._call_tool_exc is not None: + raise self._call_tool_exc + return "ok" + + async def continue_dialog( + self, + prior_response: object, + observation: str, + *, + trace_ctx: object = None, # noqa: ARG002 + ) -> SimpleNamespace: + # On the generic-retry path, end the dialog with a final answer so the loop exits. + return self._final_response + + async def print_error(self, message: str) -> None: + self.print_errors.append(message) + + +def _make_pattern(leader: _FakeLeaderAgent) -> tuple[LeaderDriven, list[dict]]: + """Build a LeaderDriven bypassing __init__, wired for run().""" + pattern = LeaderDriven.__new__(LeaderDriven) + pattern.leader_agent = leader + pattern._summarize_task = None + pattern.dialog_counter = 0 + + captured: list[dict] = [] + + async def _bg_summarize(_dialog_id: int, _task: str, dialog_context: dict) -> None: + captured.append(dialog_context) + + async def _dialog_changed(_dialog_id: int, _state: str) -> None: # noqa: ARG001 + pass + + pattern._background_summarize_and_store = _bg_summarize # type: ignore[method-assign] + pattern.dialog_changed = _dialog_changed # type: ignore[method-assign] + pattern.log_info = lambda *_a, **_k: None # type: ignore[method-assign] + pattern.log_error = lambda *_a, **_k: None # type: ignore[method-assign] + return pattern, captured + + +def test_run_short_circuits_on_plugin_not_connected(): + leader = _FakeLeaderAgent(call_tool_exc=PluginNotConnectedError()) + pattern, captured = _make_pattern(leader) + + asyncio.run(pattern.run("analyze this image")) + + # The curated message was surfaced exactly once via print_error (no retries). + assert len(leader.print_errors) == 1 + assert leader.print_errors[0] == PluginNotConnectedError.DEFAULT_MESSAGE + # Background summarize captured the failed status (best-effort: the task may not + # have run before loop close, so only assert when it did). + if captured: + assert captured[0]["status"] == "failed" + + +def test_run_does_not_short_circuit_on_generic_plugin_request_error(): + # A timeout (PluginRequestError, not the subclass) must NOT short-circuit — it + # falls through to the generic retry path, so print_error is never called. + leader = _FakeLeaderAgent(call_tool_exc=PluginRequestError("Timeout waiting for response")) + pattern, _captured = _make_pattern(leader) + + # The generic retry path eventually exhausts max_tool_retry and breaks with a + # "Too many failed tool calls" status — but no curated print_error. + asyncio.run(pattern.run("analyze this image")) + + assert leader.print_errors == [] diff --git a/copilotj/test/plugin/__init__.py b/copilotj/test/plugin/__init__.py new file mode 100644 index 00000000..d8d52ffa --- /dev/null +++ b/copilotj/test/plugin/__init__.py @@ -0,0 +1,3 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 diff --git a/copilotj/test/plugin/test_api_plugin_not_connected.py b/copilotj/test/plugin/test_api_plugin_not_connected.py new file mode 100644 index 00000000..39c1b27d --- /dev/null +++ b/copilotj/test/plugin/test_api_plugin_not_connected.py @@ -0,0 +1,182 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for the plugin-error hierarchy and the not-connected mapping. + +Covers: + A. BridgePluginAPI._request maps ``err="Client not found"`` -> PluginNotConnectedError + (with the curated DEFAULT_MESSAGE); any other err -> PluginRequestError. + A2. HTTPPluginAPI._request parses the body even on HTTP >=400 (the bridge returns + 500 + body for not-found), so the mapping is reachable. + A3. A plugin timeout (raised by the bridge as PluginRequestError) propagates as + PluginRequestError through BridgePluginAPI. + B. Bridge.send_event returns ``err == CLIENT_NOT_FOUND_ERR`` when no client matches. + +Drives async code with ``asyncio.run`` (no pytest-asyncio), matching the suite. +""" + +import asyncio +import uuid + +import pytest + +from copilotj.plugin.api import ( + CLIENT_NOT_FOUND_ERR, + BridgePluginAPI, + BridgeRequest, + BridgeResponse, + HTTPPluginAPI, + PluginNotConnectedError, + PluginRequestError, +) +from copilotj.plugin.awt import TakeSnapshotRequest +from copilotj.server.bridge import Bridge + + +class _StubBridge: + """Bridge stand-in whose send_event returns a canned response or raises.""" + + def __init__(self, response: BridgeResponse | None = None, raise_exc: BaseException | None = None) -> None: + self._response = response + self._raise_exc = raise_exc + + async def send_event(self, req: BridgeRequest) -> BridgeResponse: # noqa: ARG002 + if self._raise_exc is not None: + raise self._raise_exc + assert self._response is not None + return self._response + + +class _FakeResp: + """aiohttp response stand-in usable as an async context manager.""" + + def __init__(self, status: int, body: str) -> None: + self.status = status + self._body = body + + async def text(self) -> str: + return self._body + + async def __aenter__(self) -> "_FakeResp": + return self + + async def __aexit__(self, *_a: object) -> bool: + return False + + +class _FakeSession: + """aiohttp ClientSession stand-in returning a canned response for .post().""" + + def __init__(self, status: int, body: str) -> None: + self._resp = _FakeResp(status, body) + + def post(self, url: str, **_kwargs: object) -> _FakeResp: # noqa: ARG002 + return self._resp + + async def close(self) -> None: + pass + + +def _http_api(status: int, body: str) -> HTTPPluginAPI: + api = HTTPPluginAPI.__new__(HTTPPluginAPI) # bypass __init__ (avoids a real aiohttp session) + api.session = _FakeSession(status, body) # type: ignore[attr-defined] + return api + + +# --- A. BridgePluginAPI mapping ------------------------------------------------=========== + + +def test_bridge_plugin_api_maps_client_not_found(): + api = BridgePluginAPI(_StubBridge(response=BridgeResponse(err=CLIENT_NOT_FOUND_ERR))) + + with pytest.raises(PluginNotConnectedError) as exc_info: + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + assert str(exc_info.value) == PluginNotConnectedError.DEFAULT_MESSAGE + + +def test_bridge_plugin_api_other_err_is_request_error_not_subclass(): + api = BridgePluginAPI(_StubBridge(response=BridgeResponse(err="boom"))) + + with pytest.raises(PluginRequestError) as exc_info: + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + assert not isinstance(exc_info.value, PluginNotConnectedError) + # Raw bridge text must NOT be echoed into the message (it flows into the LLM retry + # observation and the UI) — kept in server logs only. + assert "boom" not in str(exc_info.value) + + +# --- A2. HTTPPluginAPI mapping (>=400 body parsing) ========================================= + + +def test_http_plugin_api_maps_not_found_on_500(): + body = BridgeResponse(err=CLIENT_NOT_FOUND_ERR).model_dump_json() + api = _http_api(500, body) + + with pytest.raises(PluginNotConnectedError): + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + +def test_http_plugin_api_other_err_on_500(): + body = BridgeResponse(err="boom").model_dump_json() + api = _http_api(500, body) + + with pytest.raises(PluginRequestError) as exc_info: + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + assert not isinstance(exc_info.value, PluginNotConnectedError) + assert "boom" not in str(exc_info.value) + + +def test_http_plugin_api_malformed_2xx_is_request_error(): + # A 2xx response with a non-JSON body must raise PluginRequestError, not crash with + # AssertionError (the old `assert result is not None` was stripped under `python -O`). + api = _http_api(200, "not json") + + with pytest.raises(PluginRequestError): + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + +def test_http_plugin_api_non_json_500_is_request_error(): + api = _http_api(500, "gateway error") + + with pytest.raises(PluginRequestError): + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + +# --- A3. Timeout normalization ============================================================== + + +def test_bridge_plugin_api_propagates_timeout_as_request_error(): + # After bridge normalization, a plugin timeout surfaces as PluginRequestError. + api = BridgePluginAPI(_StubBridge(raise_exc=PluginRequestError("Timeout waiting for response"))) + + with pytest.raises(PluginRequestError): + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + +# --- B. Bridge constant ==================================================================== + + +def test_bridge_send_event_returns_client_not_found_constant(): + bridge = Bridge() + + resp = asyncio.run(bridge.send_event(BridgeRequest(client_id=uuid.uuid4(), event="x"))) + + assert resp.err == CLIENT_NOT_FOUND_ERR + + +# --- G (lean). Real Bridge (no clients) + real BridgePluginAPI end-to-end ================== + + +def test_real_bridge_with_real_bridge_plugin_api_raises_not_connected(): + # Integration guard: the real Bridge produces the constant that the real + # BridgePluginAPI maps — the whole source-to-error chain with no stubs between them. + api = BridgePluginAPI(Bridge()) + + with pytest.raises(PluginNotConnectedError) as exc_info: + asyncio.run(api._request(uuid.uuid4(), TakeSnapshotRequest())) + + assert str(exc_info.value) == PluginNotConnectedError.DEFAULT_MESSAGE diff --git a/copilotj/test/server/test_threads_run_agent_errors.py b/copilotj/test/server/test_threads_run_agent_errors.py new file mode 100644 index 00000000..b85d513e --- /dev/null +++ b/copilotj/test/server/test_threads_run_agent_errors.py @@ -0,0 +1,87 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for _Thread._run_agent crash surfacing (decision B + Codex Q3 sanitization). + +- PluginNotConnectedError -> a UIEventError carrying the curated DEFAULT_MESSAGE. +- Any other exception -> a UIEventError with the generic message only; the exception + text (which may contain provider payloads / file paths / script content) must NOT + leak to the UI. Full detail still goes to logs via log_error. +""" + +import asyncio + +from copilotj.core.ui import UIEventError +from copilotj.plugin.api import PluginNotConnectedError +from copilotj.server.threads import GENERIC_AGENT_ERROR_MSG, _Thread + + +class _FakeAgent: + def __init__(self, exc: BaseException | None) -> None: + self._exc = exc + self.log_errors: list[str] = [] + + async def run(self, _prompt: str, *, trace_ctx: object = None) -> None: # noqa: ARG002 + if self._exc is not None: + raise self._exc + + def log_error(self, message: str) -> None: + self.log_errors.append(message) + + +def _make_thread(agent: _FakeAgent) -> tuple[_Thread, list[UIEventError]]: + thread = _Thread.__new__(_Thread) + thread.thread_id = "t-1" + thread._trace_ctx = None # type: ignore[attr-defined] + thread._agent = agent # type: ignore[attr-defined] + + sent: list[UIEventError] = [] + + async def _send(event: UIEventError) -> None: + sent.append(event) + + thread.send = _send # type: ignore[method-assign] + return thread, sent + + +def test_run_agent_surfaces_plugin_not_connected_with_curated_message(): + thread, sent = _make_thread(_FakeAgent(PluginNotConnectedError())) + done = asyncio.Event() + + asyncio.run(thread._run_agent("hi", done)) + + assert done.is_set() + assert len(sent) == 1 + assert sent[0].data == PluginNotConnectedError.DEFAULT_MESSAGE + assert sent[0].role == "system" + + +def test_run_agent_sanitizes_unknown_crash(): + secret = ValueError("boom at /tmp/secret with token xyz and ValueError") + thread, sent = _make_thread(_FakeAgent(secret)) + agent = thread._agent # type: ignore[attr-defined] + done = asyncio.Event() + + asyncio.run(thread._run_agent("hi", done)) + + assert done.is_set() + assert len(sent) == 1 + # Generic message only — no leakage. + assert sent[0].data == GENERIC_AGENT_ERROR_MSG + assert "secret" not in sent[0].data + assert "ValueError" not in sent[0].data + assert "token" not in sent[0].data + # Full detail preserved in logs. + assert len(agent.log_errors) == 1 + assert "secret" in agent.log_errors[0] + + +def test_run_agent_no_event_when_run_succeeds(): + thread, sent = _make_thread(_FakeAgent(None)) + done = asyncio.Event() + + asyncio.run(thread._run_agent("hi", done)) + + assert done.is_set() + assert sent == [] diff --git a/web/src/store/thread.ts b/web/src/store/thread.ts index fe46e98a..086b32a0 100644 --- a/web/src/store/thread.ts +++ b/web/src/store/thread.ts @@ -251,6 +251,12 @@ export function useThread(): { break; case "new:error": { + // An error mid-stream can leave a prior agent post with streaming=true. + // The EOF fallback only inspects the last post (now this error post), so + // finalize any streaming agent posts here to avoid a hanging spinner. + for (const p of posts.value) { + if (p.type === "agent" && p.streaming) p.streaming = false; + } const createAt = dayjs(); posts.value.push({ id: generatePostId(),