fix(optimal): serialize singleton init, normalize kb_search index_types, surface mentor errors

The OptimalService singleton published the instance before initialize()
finished, so a concurrent caller could observe _initialized=False and hit
"OptimalService not initialized" during RAG indexing. Build the instance,
initialize it, then publish under a lazily-bound asyncio lock so all callers
share a fully-initialized singleton.

roboco_kb_search forwarded the legacy alias index_types=['docs'], which is
not a valid IndexType value (the enum value is 'documentation'), producing a
400 at the route. Normalize the alias in the client before the request is
sent and fix the misleading tool docstring.

The mentor route let exceptions from mentor.ask escape as a bare 500 that
masked the real cause. Catch, log the true upstream error with stack, and
surface it in the response detail so failures are diagnosable.

roboco/api/routes/optimal.py

@@ -9,6 +9,7 @@ from datetime import UTC, datetime
 from typing import Any
 from uuid import uuid4
 
+import structlog
 from fastapi import APIRouter, HTTPException, status
 
 from roboco.api.deps import (
@@ -79,6 +80,8 @@ from roboco.services.optimal_brain import (
     get_validator_service,
 )
 
+logger = structlog.get_logger()
+
 router = APIRouter()
 
 
@@ -768,12 +771,28 @@ async def mentor_ask(
             detail=f"Mentor service initialization failed: {e}",
         ) from e
 
-    response = await mentor.ask(
-        question=request.question,
-        agent_id=str(agent.agent_id),
-        conversation_id=request.conversation_id,
-        domain=request.domain,
-    )
+    try:
+        response = await mentor.ask(
+            question=request.question,
+            agent_id=str(agent.agent_id),
+            conversation_id=request.conversation_id,
+            domain=request.domain,
+        )
+    except Exception as e:
+        # Do NOT let this escape as a bare 500 that masks the real cause.
+        # Log the true upstream error (with stack) and surface it in the
+        # detail so the failure is diagnosable end-to-end.
+        logger.error(
+            "Mentor ask failed",
+            agent_id=str(agent.agent_id),
+            error=str(e),
+            error_type=type(e).__name__,
+            exc_info=True,
+        )
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail=f"Mentor ask failed: {e}",
+        ) from e
 
     return MentorAskResponse(
         answer=response.answer,

roboco/mcp/optimal_server.py

@@ -44,6 +44,25 @@ class RecordDecisionInput(BaseModel):
     tags: list[str] | None = Field(None, description="Tags for categorization")
 
 
+# Legacy aliases agents commonly pass that are not valid IndexType values.
+# The only valid value for documentation is 'documentation' — 'docs' raises
+# ValueError at the route's IndexType(...) conversion, yielding a 400.
+_INDEX_TYPE_ALIASES = {"docs": "documentation"}
+
+
+def normalize_index_types(index_types: list[str] | None) -> list[str] | None:
+    """Map legacy index-type aliases to valid IndexType values.
+
+    Agents (and our own docstrings) historically used 'docs', which is not a
+    member of the ``IndexType`` enum. Translate it to 'documentation' before
+    the request leaves the client so the route's ``IndexType(...)`` conversion
+    succeeds instead of 400-ing.
+    """
+    if index_types is None:
+        return None
+    return [_INDEX_TYPE_ALIASES.get(t, t) for t in index_types]
+
+
 def _register_search_tools(mcp: FastMCP, client: ApiClient) -> None:
     """Register search tools available to all agents."""
 
@@ -66,7 +85,8 @@ def _register_search_tools(mcp: FastMCP, client: ApiClient) -> None:
             top_k: Number of results to return (1-20, default 5)
             project: Optional project filter
             task_id: Optional task filter
-            index_types: Index types to search (code, docs, decisions, learnings)
+            index_types: Index types to search (documentation, decisions,
+                learnings, standards, errors, reviews, journals, conversations)
 
         Returns:
             Search results with relevance scores and source info
@@ -79,8 +99,9 @@ def _register_search_tools(mcp: FastMCP, client: ApiClient) -> None:
             payload["project"] = project
         if task_id:
             payload["task_id"] = task_id
-        if index_types:
-            payload["index_types"] = index_types
+        normalized_index_types = normalize_index_types(index_types)
+        if normalized_index_types:
+            payload["index_types"] = normalized_index_types
 
         resp = await client.post("/optimal/kb/search", json=payload)
         if not resp.ok:

roboco/services/optimal.py

@@ -11,6 +11,7 @@ The service uses a plugin-based architecture where each index type is handled
 by a specialized plugin that implements the BaseIndexPlugin interface.
 """
 
+import asyncio
 import re
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -189,8 +190,6 @@ class OptimalService:
         initialized_count = 0
         failed_plugins: list[tuple[IndexType, str]] = []
 
-        import asyncio
-
         # Per-plugin initialization timeout (embedding validation can be slow)
         plugin_init_timeout = 30.0
 
@@ -439,8 +438,6 @@ class OptimalService:
 
     async def _start_periodic_update(self) -> None:
         """Start periodic update task if enabled in config."""
-        import asyncio
-
         from roboco.config import get_settings
 
         settings = get_settings()
@@ -459,8 +456,6 @@ class OptimalService:
 
     async def _periodic_update_loop(self, interval: int) -> None:
         """Background loop that checks for file changes periodically."""
-        import asyncio
-
         while True:
             try:
                 await asyncio.sleep(interval)
@@ -560,7 +555,6 @@ class OptimalService:
 
     async def close(self) -> None:
         """Cleanup resources."""
-        import asyncio
         import contextlib
 
         # Cancel periodic update task
@@ -1249,8 +1243,6 @@ class OptimalService:
         (e.g., due to timeouts or errors). Includes retry logic for transient
         failures.
         """
-        import asyncio
-
         import httpx
 
         from roboco.config import settings
@@ -1734,8 +1726,6 @@ class OptimalService:
         self, details: dict[str, Any], timeout: float
     ) -> bool:
         """Test embedding model connectivity; record result in `details`."""
-        import asyncio
-
         from roboco.config import settings
         from roboco.services.optimal_brain.shared_embedder import get_shared_embedder
 
@@ -1791,8 +1781,6 @@ class OptimalService:
         self, details: dict[str, Any], timeout: float
     ) -> bool:
         """Test vector store connectivity + per-index search; record details."""
-        import asyncio
-
         try:
             async with asyncio.timeout(timeout):
                 stats = await self.get_stats()
@@ -1837,13 +1825,39 @@ class _OptimalServiceHolder:
     """Holder for singleton OptimalService instance."""
 
     instance: OptimalService | None = None
+    lock: asyncio.Lock | None = None
+
+
+def _get_init_lock() -> asyncio.Lock:
+    """Return the init lock, lazily bound to the running event loop.
+
+    The lock is created on first use (not at import time) so it binds to the
+    loop that is actually running; a lock created at import time would bind to
+    the wrong loop and raise "bound to a different event loop".
+    """
+    if _OptimalServiceHolder.lock is None:
+        _OptimalServiceHolder.lock = asyncio.Lock()
+    return _OptimalServiceHolder.lock
 
 
 async def get_optimal_service() -> OptimalService:
-    """Get or create the OptimalService instance."""
-    if _OptimalServiceHolder.instance is None:
-        _OptimalServiceHolder.instance = OptimalService()
-        await _OptimalServiceHolder.instance.initialize()
+    """Get or create the OptimalService instance.
+
+    The instance is only published *after* ``initialize()`` completes. A lock
+    serializes concurrent first-callers so a second coroutine can never observe
+    a half-built, ``_initialized == False`` singleton mid-initialization (which
+    previously surfaced as "OptimalService not initialized" during indexing).
+    """
+    if _OptimalServiceHolder.instance is not None:
+        return _OptimalServiceHolder.instance
+
+    async with _get_init_lock():
+        # Re-check under the lock: another coroutine may have built it while
+        # we waited to acquire.
+        if _OptimalServiceHolder.instance is None:
+            service = OptimalService()
+            await service.initialize()
+            _OptimalServiceHolder.instance = service
     return _OptimalServiceHolder.instance
 
 
@@ -1852,3 +1866,5 @@ async def close_optimal_service() -> None:
     if _OptimalServiceHolder.instance is not None:
         await _OptimalServiceHolder.instance.close()
         _OptimalServiceHolder.instance = None
+    # Drop the lock so the next initialization rebinds to the running loop.
+    _OptimalServiceHolder.lock = None

tests/integration/test_optimal_mentor_error_surfacing.py

@@ -0,0 +1,85 @@
+"""The mentor route must surface (log) the real upstream error, not mask it.
+
+Previously an exception raised inside ``mentor.ask`` escaped the route as a
+bare 500 whose body carried no diagnosable cause — agents saw only a generic
+mask. The route must now log the true error and return a clean envelope whose
+``detail`` names the real cause.
+"""
+
+from __future__ import annotations
+
+from http import HTTPStatus
+from typing import TYPE_CHECKING
+from unittest.mock import AsyncMock, patch
+from uuid import uuid4
+
+import pytest
+import pytest_asyncio
+from fastapi import FastAPI
+from httpx import ASGITransport, AsyncClient
+from roboco.api.deps import get_agent_context
+from roboco.api.routes.optimal import router as optimal_router
+from roboco.models import AgentRole
+from roboco.models.permissions import AgentContext
+
+if TYPE_CHECKING:
+    from collections.abc import AsyncIterator
+
+
+_HDR = {"X-Agent-ID": str(uuid4()), "X-Agent-Role": "ceo"}
+
+
+@pytest_asyncio.fixture
+async def optimal_client() -> AsyncIterator[AsyncClient]:
+    app = FastAPI()
+    app.include_router(optimal_router, prefix="/api/optimal")
+
+    async def _override_agent() -> AgentContext:
+        return AgentContext(agent_id=uuid4(), role=AgentRole.CEO, team=None)
+
+    app.dependency_overrides[get_agent_context] = _override_agent
+    transport = ASGITransport(app=app)
+    async with AsyncClient(transport=transport, base_url="http://test") as client:
+        yield client
+    app.dependency_overrides.clear()
+
+
+@pytest.mark.asyncio
+async def test_mentor_ask_logs_and_surfaces_real_upstream_error(
+    optimal_client: AsyncClient,
+) -> None:
+    mentor = AsyncMock()
+    mentor._optimal_service = object()  # already initialized
+    mentor.ask = AsyncMock(side_effect=RuntimeError("ollama connection refused"))
+
+    with (
+        patch("roboco.api.routes.optimal.get_mentor_service") as mock_mentor,
+        patch("roboco.api.routes.optimal.get_optimal_service") as mock_get,
+        patch("roboco.api.routes.optimal.logger") as mock_logger,
+    ):
+        mock_mentor.return_value = mentor
+        mock_get.return_value = AsyncMock()
+        response = await optimal_client.post(
+            "/api/optimal/mentor/ask",
+            json={"question": "What"},
+            headers=_HDR,
+        )
+
+    # The route must not 200 on a failed ask.
+    assert response.status_code in (
+        HTTPStatus.INTERNAL_SERVER_ERROR,
+        HTTPStatus.SERVICE_UNAVAILABLE,
+    )
+    # The real cause must be diagnosable in the response body, not masked.
+    assert "ollama connection refused" in response.text
+
+    # The real error must be logged (so it is diagnosable server-side even if
+    # the envelope is later sanitized).
+    logged_text = "".join(
+        str(call.args) + str(call.kwargs)
+        for call in (
+            *mock_logger.error.call_args_list,
+            *mock_logger.exception.call_args_list,
+        )
+    )
+    assert "ollama connection refused" in logged_text

tests/unit/services/test_optimal_grounding.py

@@ -0,0 +1,137 @@
+"""Grounding-layer regression tests for the Optimal/RAG stack.
+
+Covers three failure modes that surfaced at runtime:
+
+1. ``get_optimal_service`` published a not-yet-initialized singleton while
+   ``initialize()`` was still awaiting, so a concurrent caller hit
+   "OptimalService not initialized. Call initialize() first." during indexing.
+2. ``roboco_kb_search`` forwarded the legacy alias ``index_types=['docs']``,
+   which is not a valid ``IndexType`` value ('documentation' is), producing a
+   400 at the route.
+3. The mentor route let exceptions from ``mentor.ask`` escape as a bare 500
+   with no log of the true upstream cause.
+"""
+
+from __future__ import annotations
+
+import asyncio
+
+import pytest
+from roboco.mcp.optimal_server import normalize_index_types
+from roboco.models.optimal import IndexType
+from roboco.services import optimal as optimal_module
+from roboco.services.optimal import (
+    OptimalService,
+    close_optimal_service,
+    get_optimal_service,
+)
+
+# ---------------------------------------------------------------------------
+# Sub-issue 2: kb_search must not forward the invalid 'docs' alias
+# ---------------------------------------------------------------------------
+
+
+def test_normalize_index_types_maps_docs_alias_to_documentation() -> None:
+    """The legacy 'docs' alias must become the valid 'documentation' value.
+
+    ``IndexType('docs')`` raises ``ValueError`` — the only valid value is
+    ``IndexType.DOCUMENTATION`` whose string value is 'documentation'.
+    """
+    assert normalize_index_types(["docs"]) == ["documentation"]
+    # Every normalized value must be a constructible IndexType.
+    for value in normalize_index_types(["docs"]):
+        IndexType(value)
+
+
+def test_normalize_index_types_passes_valid_values_through() -> None:
+    assert normalize_index_types(["documentation", "decisions"]) == [
+        "documentation",
+        "decisions",
+    ]
+
+
+def test_normalize_index_types_none_returns_none() -> None:
+    assert normalize_index_types(None) is None
+
+
+# ---------------------------------------------------------------------------
+# Sub-issue 1: the init entrypoint must never expose an uninitialized singleton
+# ---------------------------------------------------------------------------
+
+
+class _SlowInitService(OptimalService):
+    """OptimalService whose initialize() yields control mid-flight.
+
+    This reproduces the publish-before-initialize race: while one coroutine
+    is awaiting inside ``initialize()``, a second coroutine calls
+    ``get_optimal_service()``. With the old code the second caller received
+    the instance with ``_initialized == False``.
+    """
+
+    init_calls = 0
+
+    async def initialize(self) -> None:
+        type(self).init_calls += 1
+        # Cooperatively yield so a concurrent get_optimal_service() can run
+        # during the window the old code left the instance unpublished/uninit.
+        await asyncio.sleep(0)
+        self._initialized = True
+
+
+@pytest.mark.asyncio
+async def test_get_optimal_service_never_returns_uninitialized(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Concurrent callers must all receive a fully-initialized singleton.
+
+    The indexing entrypoint calls ``_get_plugin`` which raises
+    "OptimalService not initialized" when ``_initialized`` is False. This test
+    asserts no concurrent caller can observe that state.
+    """
+    await close_optimal_service()
+    _SlowInitService.init_calls = 0
+    monkeypatch.setattr(optimal_module, "OptimalService", _SlowInitService)
+
+    try:
+        results: list[OptimalService] = await asyncio.gather(
+            get_optimal_service(),
+            get_optimal_service(),
+            get_optimal_service(),
+        )
+        for svc in results:
+            assert svc._initialized is True
+        # All callers share the one singleton, initialized exactly once.
+        assert len({id(s) for s in results}) == 1
+        assert _SlowInitService.init_calls == 1
+    finally:
+        await close_optimal_service()
+
+
+@pytest.mark.asyncio
+async def test_indexing_entrypoint_does_not_raise_not_initialized(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A caller obtaining the service mid-init must not hit _get_plugin's guard."""
+    await close_optimal_service()
+    _SlowInitService.init_calls = 0
+    monkeypatch.setattr(optimal_module, "OptimalService", _SlowInitService)
+
+    async def _use_service() -> None:
+        svc = await get_optimal_service()
+        # Mirror what index_documentation does first: resolve the plugin,
+        # which raises RuntimeError("OptimalService not initialized...") if
+        # the singleton was published before initialize() completed.
+        svc._plugins[IndexType.DOCUMENTATION] = _FakePlugin()
+        svc._get_plugin(IndexType.DOCUMENTATION)
+
+    try:
+        await asyncio.gather(_use_service(), _use_service())
+    finally:
+        await close_optimal_service()
+
+
+class _FakePlugin:
+    """Minimal stand-in so _get_plugin returns without a real plugin."""
+
+    async def close(self) -> None:  # pragma: no cover - never awaited here
+        return None