fix(optimal): raise instead of falling back to 'unknown' doc source

Silent fallback hid an upstream bug where journal_entry.id wasn't
flushed before indexing. Raise so we see the regression.

roboco/services/optimal.py

@@ -758,8 +758,15 @@ class OptimalService:
                 message_type=params.message_type,
             )
 
-        # Track in database
-        source = f"roboco://conversations/{params.session_id or 'unknown'}"
+        # Track in database. Refuse to fall back to 'unknown' — that masked
+        # upstream bugs where session_id was None despite the type contract.
+        if params.session_id is None:
+            raise ValueError(
+                "index_conversation: session_id is required; refusing to "
+                "build doc-source with placeholder. Caller must pass a "
+                "flushed session UUID."
+            )
+        source = f"roboco://conversations/{params.session_id}"
         await self._track_indexed_document(
             IndexType.CONVERSATIONS,
             source=source,
@@ -787,8 +794,18 @@ class OptimalService:
                 tags=params.tags,
             )
 
-        # Track in database
-        source = f"roboco://journals/{params.entry_id or 'unknown'}"
+        # Track in database. Refuse to fall back to 'unknown' — that masked
+        # the upstream bug where the journal entry row hadn't been flushed
+        # before indexing, producing roboco://journals/None doc-sources.
+        if params.entry_id is None:
+            raise ValueError(
+                "index_journal_entry: entry_id is required; refusing to "
+                "build doc-source with placeholder. Caller must flush the "
+                "entry row first (use require_uuid(entry_row.id)). System "
+                "events without a journal entry must use a different "
+                "indexing path."
+            )
+        source = f"roboco://journals/{params.entry_id}"
         await self._track_indexed_document(
             IndexType.JOURNALS,
             source=source,
@@ -896,12 +913,20 @@ class OptimalService:
             result = await plugin.record_review(review_params)
             doc_id = result.doc_id
 
-        # Track in database
-        source = f"roboco://reviews/{params.file_path or 'unknown'}"
+        # Track in database. Refuse to fall back to 'unknown' — file_path
+        # is typed `str` (required) and an empty value means the caller is
+        # broken; an unidentifiable review is worse than no review.
+        if not params.file_path:
+            raise ValueError(
+                "record_review: file_path is required; refusing to build "
+                "doc-source with placeholder. Caller must pass a non-empty "
+                "path or task identifier."
+            )
+        source = f"roboco://reviews/{params.file_path}"
         await self._track_indexed_document(
             IndexType.REVIEWS,
             source=source,
-            title=f"Review: {params.file_path or 'Code'}",
+            title=f"Review: {params.file_path}",
             preview=params.summary[:500] if params.summary else None,
             metadata={
                 "file_path": params.file_path,

roboco/services/task.py

@@ -1658,6 +1658,8 @@ class TaskService(BaseService):
             task_team: Team for categorization
             details: Additional event details
         """
+        from uuid import NAMESPACE_URL, uuid5
+
         from roboco.models.optimal import IndexJournalEntryParams
         from roboco.services.optimal import get_optimal_service
 
@@ -1670,11 +1672,22 @@ class TaskService(BaseService):
             if details:
                 content += f"\nDetails: {details}"
 
+            # Lifecycle events have no journal-entry row of their own. Derive
+            # a deterministic synthetic UUID from (task, event, timestamp)
+            # so the index_journal_entry source is meaningful and unique
+            # per event — never the literal "None" that the old fallback
+            # produced.
+            now_iso = datetime.now(UTC).isoformat()
+            synthetic_entry_id = uuid5(
+                NAMESPACE_URL,
+                f"roboco-lifecycle/{task_id}/{event_type}/{now_iso}",
+            )
+
             # Index to journals for lifecycle tracking
             await optimal.index_journal_entry(
                 IndexJournalEntryParams(
                     content=content,
-                    entry_id=None,  # Will be auto-generated
+                    entry_id=synthetic_entry_id,
                     agent_id=None,  # System event, no specific agent
                     entry_type=f"lifecycle_{event_type}",
                     task_id=task_id,

tests/unit/scripts/test_bash_guard.py

@@ -29,6 +29,7 @@ def _run(cmd: str) -> int:
         input=payload,
         capture_output=True,
         text=True,
+        check=False,
     )
     return result.returncode
 
@@ -67,13 +68,17 @@ def test_allows_external_curl_to_documentation() -> None:
 def test_github_url_still_uses_github_specific_deny() -> None:
     """Existing GitHub deny rule must fire BEFORE the new gateway deny."""
     payload = json.dumps(
-        {"tool_name": "Bash", "tool_input": {"command": "curl https://api.github.com/user"}}
+        {
+            "tool_name": "Bash",
+            "tool_input": {"command": "curl https://api.github.com/user"},
+        }
     )
     result = subprocess.run(
         [str(GUARD)],
         input=payload,
         capture_output=True,
         text=True,
+        check=False,
     )
     assert result.returncode != _ALLOWED
     # The GitHub-specific message should appear, not the gateway message.

tests/unit/services/test_optimal_doc_source.py

@@ -0,0 +1,145 @@
+"""Unit tests for OptimalService doc-source identifier validation.
+
+These guard against the silent ``or 'unknown'`` / ``or None`` fallbacks that
+previously masked upstream bugs (e.g. a journal entry being indexed before
+its ID was flushed to the database produced ``roboco://journals/None`` rows
+in indexed_documents).
+
+Each indexer that builds a ``source`` URI from a caller-supplied identifier
+must raise ``ValueError`` instead of stitching a placeholder when that
+identifier is missing.
+"""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+from typing import cast
+from unittest.mock import AsyncMock, MagicMock
+from uuid import uuid4
+
+import pytest
+from roboco.models.optimal import (
+    IndexConversationParams,
+    IndexJournalEntryParams,
+    IndexReviewParams,
+    IndexType,
+)
+from roboco.services.optimal import OptimalService
+
+
+class _StubOptimalService(OptimalService):
+    """Test-only subclass that bypasses DB tracking.
+
+    The indexer methods we exercise call ``_track_indexed_document`` after
+    building the doc-source; we don't want a DB round-trip in unit tests,
+    and we want the raise to happen *before* this stub is ever called.
+    """
+
+    async def _track_indexed_document(
+        self,
+        index_type: IndexType,
+        source: str,
+        title: str | None = None,
+        preview: str | None = None,
+        metadata: dict | None = None,
+    ) -> None:
+        # Arguments are deliberately ignored — this stub exists solely to
+        # neutralize the DB write in the success-path test.
+        del index_type, source, title, preview, metadata
+
+
+def _service_with_stub_plugin() -> _StubOptimalService:
+    """Build a _StubOptimalService with MagicMock plugins.
+
+    The indexer methods short-circuit through ``_get_plugin`` -> plugin
+    coroutine; the source-construction step we want to exercise sits
+    *after* the plugin call, so any AsyncMock plugin coroutine will do.
+    """
+    svc = _StubOptimalService()
+    plugin = MagicMock()
+    plugin.index_entry = AsyncMock()
+    plugin.index_message = AsyncMock()
+    plugin.record_review = AsyncMock(return_value=MagicMock(doc_id=""))
+    plugin.ingest = AsyncMock()
+    svc._plugins = {
+        IndexType.JOURNALS: plugin,
+        IndexType.CONVERSATIONS: plugin,
+        IndexType.REVIEWS: plugin,
+    }
+    svc._initialized = True
+    return svc
+
+
+@pytest.mark.asyncio
+async def test_index_journal_entry_raises_when_entry_id_is_none() -> None:
+    """``entry_id=None`` must raise — the silent fallback hid an upstream
+    bug where the entry row hadn't been flushed before indexing, producing
+    ``roboco://journals/None`` doc-sources in the RAG store.
+    """
+    svc = _service_with_stub_plugin()
+    params = IndexJournalEntryParams(
+        content="some reflection",
+        entry_type="reflect",
+        entry_id=None,  # the bug we now reject
+        agent_id=uuid4(),
+        task_id=uuid4(),
+    )
+
+    with pytest.raises(ValueError, match="entry_id is required"):
+        await svc.index_journal_entry(params)
+
+
+@pytest.mark.asyncio
+async def test_index_journal_entry_succeeds_with_real_entry_id() -> None:
+    """Sanity check: a flushed entry_id flows through without raising."""
+    svc = _service_with_stub_plugin()
+    params = IndexJournalEntryParams(
+        content="some reflection",
+        entry_type="reflect",
+        entry_id=uuid4(),
+        agent_id=uuid4(),
+        task_id=uuid4(),
+    )
+    await svc.index_journal_entry(params)
+
+
+@pytest.mark.asyncio
+async def test_index_conversation_raises_when_session_id_missing() -> None:
+    """``session_id`` is typed UUID (required) but the old code coerced
+    falsy values to ``'unknown'``. Reject explicitly so the caller fixes
+    the missing flush instead of writing junk doc-sources.
+
+    A real caller would have to bypass the dataclass type contract for
+    this to fire (e.g. ``cast(UUID, None)``); we simulate that with a
+    ``SimpleNamespace`` so we don't have to reach inside a frozen-style
+    dataclass to clobber a field.
+    """
+    svc = _service_with_stub_plugin()
+    fake_params = SimpleNamespace(
+        content="hello",
+        channel_id=uuid4(),
+        session_id=None,  # the runtime bug we now reject
+        agent_id=uuid4(),
+        task_id=None,
+        message_type=None,
+    )
+
+    with pytest.raises(ValueError, match="session_id is required"):
+        await svc.index_conversation(cast("IndexConversationParams", fake_params))
+
+
+@pytest.mark.asyncio
+async def test_record_review_raises_when_file_path_empty() -> None:
+    """``file_path`` is typed ``str`` (required); empty strings produced
+    ``roboco://reviews/unknown`` doc-sources. Reject empty-or-missing.
+    """
+    svc = _service_with_stub_plugin()
+    params = IndexReviewParams(
+        file_path="",  # empty string -> would hit `or 'unknown'`
+        comments=[],
+        approved=True,
+        summary="ok",
+    )
+
+    with pytest.raises(ValueError, match="file_path is required"):
+        await svc.record_review(params)