From f8e07d47e33710ebbe9d2d732d3026b686d81866 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 3 May 2026 10:38:59 +0200 Subject: [PATCH] fix(optimal): make IndexJournalEntryParams.entry_id required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 22 added a runtime ValueError when entry_id is None, but the dataclass still typed it Optional. Contract-vs-runtime split — callers get no IDE/mypy hint about the required field. Tighten the type to UUID (required) and remove the misleading "can be None for system events" docstring note. Lifecycle events already use their synthetic uuid5 path, so no real caller is broken. --- roboco/models/optimal.py | 7 ++----- tests/unit/services/test_optimal_doc_source.py | 11 +++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/roboco/models/optimal.py b/roboco/models/optimal.py index 4a3eafb1..2bc34d5d 100644 --- a/roboco/models/optimal.py +++ b/roboco/models/optimal.py @@ -90,14 +90,11 @@ class IndexConversationParams: @dataclass class IndexJournalEntryParams: - """Parameters for indexing a journal entry. - - Note: entry_id and agent_id can be None for system events (e.g., lifecycle events). - """ + """Parameters for indexing a journal entry.""" content: str entry_type: str - entry_id: UUID | None = None + entry_id: UUID # required; lifecycle events use a different indexing path agent_id: UUID | None = None task_id: UUID | None = None tags: list[str] | None = None diff --git a/tests/unit/services/test_optimal_doc_source.py b/tests/unit/services/test_optimal_doc_source.py index 02e3c7ea..b9e6390c 100644 --- a/tests/unit/services/test_optimal_doc_source.py +++ b/tests/unit/services/test_optimal_doc_source.py @@ -75,18 +75,25 @@ 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. + + ``entry_id`` is typed ``UUID`` (required); a real caller would have to + bypass the dataclass type contract for this to fire (e.g. via + ``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 — same pattern used by the conversation test below. """ svc = _service_with_stub_plugin() - params = IndexJournalEntryParams( + fake_params = SimpleNamespace( content="some reflection", entry_type="reflect", entry_id=None, # the bug we now reject agent_id=uuid4(), task_id=uuid4(), + tags=None, ) with pytest.raises(ValueError, match="entry_id is required"): - await svc.index_journal_entry(params) + await svc.index_journal_entry(cast("IndexJournalEntryParams", fake_params)) @pytest.mark.asyncio