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(),