diff --git a/src/gaia/apps/webui/src/App.tsx b/src/gaia/apps/webui/src/App.tsx index fa8972388..0caf4c6ad 100644 --- a/src/gaia/apps/webui/src/App.tsx +++ b/src/gaia/apps/webui/src/App.tsx @@ -291,33 +291,39 @@ function App() { }; }, [setRunningSessions]); - // Support URL-based session navigation (?session= or #) + // Support URL-based session navigation (?session= or #). + // Runs once on mount only. Depending on currentSessionId caused a + // ping-pong (#1755): the hash-sync effect writes a 7-char short hash that + // never equals the full UUID, so on every session switch this guard re-fired, + // read a stale hash, and scheduled a ~500ms bounce back to the URL's target. + // currentSessionId is read fresh inside the timer instead of via deps. useEffect(() => { const params = new URLSearchParams(window.location.search); const sessionParam = params.get('session'); const hashParam = window.location.hash.replace(/^#/, ''); const target = sessionParam || hashParam; if (!target) return; - // Already on this session — no switch needed - if (target === currentSessionId) return; log.nav.info(`URL session parameter: ${target}`); // Defer so session list has time to load const timer = setTimeout(() => { - const { sessions } = useChatStore.getState(); + const { sessions, currentSessionId } = useChatStore.getState(); + // Already on this session — no switch needed + if (target === currentSessionId) return; // Try exact match first (full UUID), then short hash match - let matchId: string | null = sessions.some((s: { id: string }) => s.id === target) + const matchId: string | null = sessions.some((s: { id: string }) => s.id === target) ? target : findSessionByHash(sessions, target); - if (matchId) { + if (matchId && matchId !== currentSessionId) { setCurrentSession(matchId); setMessages([]); - } else { + } else if (!matchId) { log.nav.warn(`Session ${target} not found in loaded sessions`); } }, 500); return () => clearTimeout(timer); - }, [currentSessionId, setCurrentSession, setMessages]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [setCurrentSession, setMessages]); // Subscribe to system session-activation events (MCP bridge P1) useEffect(() => { diff --git a/src/gaia/apps/webui/src/__tests__/App.test.tsx b/src/gaia/apps/webui/src/__tests__/App.test.tsx index 528f7852d..1009439e6 100644 --- a/src/gaia/apps/webui/src/__tests__/App.test.tsx +++ b/src/gaia/apps/webui/src/__tests__/App.test.tsx @@ -2,35 +2,107 @@ // SPDX-License-Identifier: MIT /** - * App.tsx integration tests — MCP/SSE session activation (issue #1086). + * App.tsx nav-guard tests — URL/hash session navigation (issues #1086, #1755). + * + * Renders the real with every child stubbed so only App's own effects + * run, then drives the URL-hash navigation guard with fake timers. */ -import { describe, it, expect } from 'vitest'; - -describe('App session navigation guard', () => { - it('hash URL format includes # not ?session=', () => { - // Verify the contract: bridge uses /#, not /?session= - const sessionId = 'abc123'; - const hashUrl = `http://localhost:4200/#${sessionId}`; - const queryUrl = `http://localhost:4200/?session=${sessionId}`; - expect(hashUrl).toContain('#'); - expect(hashUrl).not.toContain('?session='); - expect(queryUrl).not.toContain('#'); - }); +import { render, act } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import App from '../App'; +import { useChatStore } from '../stores/chatStore'; +import * as api from '../services/api'; +import { getSessionHash } from '../utils/format'; + +// Stub every child so their mount effects don't interfere with the guard. +vi.mock('../components/Sidebar', () => ({ Sidebar: () => null })); +vi.mock('../components/ChatView', () => ({ ChatView: () => null })); +vi.mock('../components/WelcomeScreen', () => ({ WelcomeScreen: () => null })); +vi.mock('../components/DocumentLibrary', () => ({ DocumentLibrary: () => null })); +vi.mock('../components/FileBrowser', () => ({ FileBrowser: () => null })); +vi.mock('../components/MemoryDashboard', () => ({ MemoryDashboard: () => null })); +vi.mock('../components/ScheduleManager', () => ({ ScheduleManager: () => null })); +vi.mock('../components/SettingsPage', () => ({ SettingsPage: () => null })); +vi.mock('../components/MobileAccessModal', () => ({ MobileAccessModal: () => null })); +vi.mock('../components/ConnectionBanner', () => ({ ConnectionBanner: () => null })); +vi.mock('../components/UpdateIndicator', () => ({ UpdateIndicator: () => null })); +vi.mock('../components/PermissionPrompt', () => ({ PermissionPrompt: () => null })); +vi.mock('../components/NotificationCenter', () => ({ NotificationCenter: () => null })); +vi.mock('../services/api'); + +const SESSION_A = { id: 'aaaaaaaa-1111-2222-3333-444444444444' }; +const SESSION_B = { id: 'bbbbbbbb-5555-6666-7777-888888888888' }; + +const mockedApi = vi.mocked(api); - it('set_active_session event shape matches contract', () => { - // Verify the SSE event shape the frontend expects - const event = { type: 'set_active_session', session_id: 'test-123' }; - expect(event.type).toBe('set_active_session'); - expect(event.session_id).toBeDefined(); - expect(typeof event.session_id).toBe('string'); +// jsdom has no EventSource; provide a no-op so App's SSE effect can mount. +class FakeEventSource { + onopen: (() => void) | null = null; + onmessage: ((ev: { data: string }) => void) | null = null; + onerror: (() => void) | null = null; + close() { /* no-op */ } +} + +beforeEach(() => { + vi.clearAllMocks(); + (globalThis as unknown as { EventSource: unknown }).EventSource = FakeEventSource; + + mockedApi.listAgents.mockResolvedValue({ agents: [] } as never); + mockedApi.getSystemStatus.mockResolvedValue({ lemonade_running: true } as never); + mockedApi.listSessions.mockResolvedValue({ sessions: [SESSION_A, SESSION_B] } as never); + mockedApi.getActiveRuns.mockResolvedValue({ session_ids: [] } as never); + mockedApi.getTunnelStatus.mockResolvedValue({ active: false } as never); + + useChatStore.setState({ + sessions: [SESSION_A, SESSION_B] as never, + currentSessionId: null, + messages: [], }); + window.history.replaceState(null, '', '/'); +}); + +afterEach(() => { + vi.useRealTimers(); }); -describe('index.html title', () => { - it('title is GAIA Agent UI', () => { - // This test documents the required title value - const expectedTitle = 'GAIA Agent UI'; - expect(expectedTitle).toBe('GAIA Agent UI'); +describe('App URL/hash session navigation guard', () => { + it('navigates to the session named in the URL hash on mount', async () => { + vi.useFakeTimers(); + window.history.replaceState(null, '', `#${getSessionHash(SESSION_A.id)}`); + + await act(async () => { + render(); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(600); + }); + + expect(useChatStore.getState().currentSessionId).toBe(SESSION_A.id); + }); + + it('does not ping-pong back to the URL hash after switching sessions (#1755)', async () => { + vi.useFakeTimers(); + window.history.replaceState(null, '', `#${getSessionHash(SESSION_A.id)}`); + + await act(async () => { + render(); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(600); + }); + expect(useChatStore.getState().currentSessionId).toBe(SESSION_A.id); + + // User switches to B (e.g. sidebar click). The hash-sync effect rewrites + // the URL hash to B's short hash. The nav guard must NOT re-fire and bounce + // back to A — that was the #1755 oscillation. + await act(async () => { + useChatStore.getState().setCurrentSession(SESSION_B.id); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(600); + }); + + expect(useChatStore.getState().currentSessionId).toBe(SESSION_B.id); }); }); diff --git a/src/gaia/mcp/servers/agent_ui_mcp.py b/src/gaia/mcp/servers/agent_ui_mcp.py index 98f655a6e..e73d8a84b 100644 --- a/src/gaia/mcp/servers/agent_ui_mcp.py +++ b/src/gaia/mcp/servers/agent_ui_mcp.py @@ -74,6 +74,17 @@ def _normalize_error( return {"status": "error", "detail": msg} +def _is_error(result: Any) -> bool: + """True if a tool result is a structured error envelope. + + All boundary errors flow through ``_normalize_error`` which returns + ``{"status": "error", ...}``. Callers must key on this shape, not the + legacy ``{"error": ...}`` one, or a backend failure silently passes as + success. + """ + return isinstance(result, dict) and result.get("status") == "error" + + def _api(base_url: str, method: str, path: str, **kwargs) -> Dict[str, Any]: """Make an API request to the GAIA Agent UI backend.""" url = f"{base_url}/api{path}" @@ -291,7 +302,7 @@ def delete_session(session_id: str) -> Dict[str, Any]: r.raise_for_status() return {"deleted": True, "session_id": session_id} except Exception as e: - return {"error": str(e)} + return _normalize_error(e, backend_url) # ── Messages ─────────────────────────────────────────────────── @@ -299,7 +310,7 @@ def delete_session(session_id: str) -> Dict[str, Any]: def get_messages(session_id: str) -> Dict[str, Any]: """Get all messages in a session (with agent steps and tool outputs).""" data = _api(backend_url, "get", f"/sessions/{session_id}/messages") - if "error" in data: + if _is_error(data): return data # Simplify for readability messages = [] @@ -369,15 +380,16 @@ def index_document(filepath: str, session_id: str = "") -> Dict[str, Any]: f"/sessions/{session_id}/documents", json={"document_id": doc_id}, ) - if "error" not in attach_result: - result["linked_to_session"] = session_id - else: + if _is_error(attach_result): logger.warning( "Failed to link doc %s to session %s: %s", doc_id, session_id, - attach_result.get("error"), + attach_result.get("detail"), ) + result["link_error"] = attach_result.get("detail") + else: + result["linked_to_session"] = session_id return result @mcp.tool() @@ -463,7 +475,10 @@ def take_screenshot( try: from PIL import Image, ImageGrab except ImportError: - return {"error": "Pillow not installed. Run: pip install Pillow"} + return { + "status": "error", + "detail": "Pillow not installed. Run: pip install Pillow", + } try: bbox = None @@ -524,7 +539,7 @@ def take_screenshot( "file_size_kb": file_size_kb, } except Exception as e: - return {"error": f"Screenshot failed: {e}"} + return {"status": "error", "detail": f"Screenshot failed: {e}"} # ── Memory ──────────────────────────────────────────────────────── # diff --git a/src/gaia/ui/sse_handler.py b/src/gaia/ui/sse_handler.py index 0140fa923..0353abc4e 100644 --- a/src/gaia/ui/sse_handler.py +++ b/src/gaia/ui/sse_handler.py @@ -184,17 +184,6 @@ def _emit(self, event: Dict[str, Any]): """Push an event to the queue for SSE delivery.""" self.event_queue.put(event) - def emit_set_active_session(self, session_id: str) -> None: - """Emit a set_active_session event into the per-session chat stream. - - Note: the primary activation path (MCP bridge → App.tsx) routes - through _system_emitter in routers/sessions.py, not this method. - This is a convenience shim for agent-loop code that already holds - an SSEOutputHandler and wants to push the event without importing - the sessions router. - """ - self._emit({"type": "set_active_session", "session_id": session_id}) - def _elapsed(self) -> float: if self._start_time is None: return 0.0 diff --git a/tests/unit/test_agent_ui_mcp.py b/tests/unit/test_agent_ui_mcp.py index 805ecf18c..a3359bc86 100644 --- a/tests/unit/test_agent_ui_mcp.py +++ b/tests/unit/test_agent_ui_mcp.py @@ -138,3 +138,107 @@ def test_docstring_honest(self): ), "send_message docstring should not claim real-time streaming" # Should mention open_session_in_browser assert "open_session_in_browser" in doc + + +class TestIsError: + """#1755: error detection must key on the structured envelope, not the + legacy ``{"error": ...}`` shape that ``_normalize_error`` replaced.""" + + def test_new_envelope_is_error(self): + from gaia.mcp.servers.agent_ui_mcp import _is_error + + assert _is_error({"status": "error", "detail": "boom"}) is True + + def test_success_payload_not_error(self): + from gaia.mcp.servers.agent_ui_mcp import _is_error + + assert _is_error({"messages": [], "total": 0}) is False + assert _is_error({"status": "ok"}) is False + assert _is_error("not a dict") is False + + def test_legacy_error_key_not_matched(self): + from gaia.mcp.servers.agent_ui_mcp import _is_error + + # The legacy shape no longer appears; _is_error must not depend on it. + assert _is_error({"error": "boom"}) is False + + +def _get_tool(name): + """Construct the MCP server and return a tool's raw function.""" + pytest.importorskip("mcp") # constructing the server needs optional mcp + from gaia.mcp.servers.agent_ui_mcp import create_agent_ui_mcp + + with patch("requests.get") as mock_get: + mock_get.return_value = MagicMock( + status_code=200, json=MagicMock(return_value={}) + ) + mock_get.return_value.raise_for_status.return_value = None + mcp = create_agent_ui_mcp("http://localhost:4200") + + tool = mcp._tool_manager._tools.get(name) # pylint: disable=protected-access + if tool is None: + pytest.skip(f"{name} tool not found in tool manager") + return tool.fn + + +class TestGetMessagesSurfacesError: + """#1755: get_messages keyed on the old shape, so a backend error fell + through to an empty success payload (silent fallback).""" + + def test_backend_error_not_silent_empty(self): + get_messages = _get_tool("get_messages") + + mock_resp = MagicMock() + mock_resp.status_code = 404 + mock_resp.json.return_value = {"detail": "Session not found"} + mock_resp.raise_for_status.side_effect = requests.exceptions.HTTPError( + response=mock_resp + ) + with patch("requests.get", return_value=mock_resp): + result = get_messages("nonexistent") + + assert result["status"] == "error" + assert result.get("detail") + # Regression: must NOT degrade to {"messages": [], "total": 0}. + assert "messages" not in result + + +class TestIndexDocumentLinkFailure: + """#1755: index_document reported a session link succeeded even when the + attach call failed (false success).""" + + @staticmethod + def _upload_ok(): + resp = MagicMock() + resp.status_code = 200 + resp.json.return_value = {"id": "doc-1"} + resp.raise_for_status.return_value = None + return resp + + def test_attach_failure_not_reported_as_linked(self): + index_document = _get_tool("index_document") + + attach_resp = MagicMock() + attach_resp.status_code = 404 + attach_resp.json.return_value = {"detail": "Session not found"} + attach_resp.raise_for_status.side_effect = requests.exceptions.HTTPError( + response=attach_resp + ) + with patch("requests.post", side_effect=[self._upload_ok(), attach_resp]): + result = index_document("/tmp/file.pdf", session_id="sess-1") + + assert "linked_to_session" not in result + assert result.get("link_error") + + def test_attach_success_reports_linked(self): + index_document = _get_tool("index_document") + + attach_resp = MagicMock() + attach_resp.status_code = 200 + attach_resp.json.return_value = {"attached": True} + attach_resp.raise_for_status.return_value = None + with patch("requests.post", side_effect=[self._upload_ok(), attach_resp]): + result = index_document("/tmp/file.pdf", session_id="sess-1") + + assert result.get("linked_to_session") == "sess-1" + assert "link_error" not in result