From 4d2a6666a48fe878cf5634a53b5c7ff2c73f50c1 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 27 Jun 2026 20:58:36 -0400 Subject: [PATCH] fix(rag): convert RAGChunk to dict in _rag_search_result to match type contract The RAG engine's search() returns List[RAGChunk] (dataclass instances), but _rag_search_result's return type is Result[list[Metadata]] (a list of dicts). The previous code returned the RAGChunks as-is, then the caller in _handle_request_event did chunk["metadata"] (dict access on a dataclass) which raised TypeError. The exception was silently swallowed by the submit_io worker, leaving ai_status stuck at sending... for the full 50-second test poll before failing. Two surgical changes: 1. _rag_search_result: convert RAGChunk to dict via to_dict() (with a hasattr guard for tests that return dicts directly). Matches the function's documented return type. 2. _handle_request_event: use isinstance guards + dict.get() on the chunk fields. Defensive against the type mismatch and matches the dict contract. The test fix (unique collection name + workspace-targeted cleanup) is the test-side complement that prevents the dim-mismatch path from being hit in batched runs. Verified: 4 consecutive PASS runs of test_rag_phase4_final_verify in isolation (7-8s each). 25/26 RAG tests pass; the one remaining failure (test_rag_collection_dim_mismatch_recreates_collection) is a pre-existing regression from commit 24e93a75 which changed the dim check from delete_collection to shutil.rmtree without updating the test mock setup. Out of scope for this fix. --- src/app_controller.py | 19 +++++++------ tests/test_rag_phase4_final_verify.py | 39 +++++++++++++++------------ 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/app_controller.py b/src/app_controller.py index 04bc7891..c40a4aaa 100644 --- a/src/app_controller.py +++ b/src/app_controller.py @@ -3501,14 +3501,17 @@ class AppController: def _rag_search_result(self, user_msg: str) -> "Result[list[Metadata]]": """Per-event handler (Phase 6 Group 6.6): RAG search via the engine. - Returns Result[List[RAGChunk]]. On failure: any engine/SDK exception - -> ErrorInfo(original=e). Caller (`_handle_request_event`) appends + Returns Result[List[Metadata]] (list of dicts per the function's + type annotation; the engine's search() returns List[RAGChunk] + dataclasses which are converted via to_dict() here so the caller + in `_handle_request_event` can use dict-style access). On failure: + any engine/SDK exception -> ErrorInfo(original=e). Caller appends to `self._last_request_errors` for sub-track 4 GUI display.""" if not (self.rag_engine and self.rag_config and self.rag_config.enabled): return Result(data=[]) try: chunks = self.rag_engine.search(user_msg) - return Result(data=list(chunks) if chunks else []) + return Result(data=[c.to_dict() if hasattr(c, "to_dict") and not isinstance(c, dict) else (c if isinstance(c, dict) else Metadata()) for c in chunks]) except (OSError, IOError, ValueError, TypeError, KeyError, AttributeError, RuntimeError) as e: return Result(data=[], errors=[ErrorInfo( kind=ErrorKind.NETWORK, @@ -4171,18 +4174,18 @@ class AppController: [C: tests/test_live_gui_integration_v2.py:test_user_request_error_handling, tests/test_live_gui_integration_v2.py:test_user_request_integration_flow, tests/test_rag_integration.py:test_rag_integration] """ self.ai_status = 'sending...' - + user_msg = event.prompt - + # 1. RAG Retrieval (Enrich prompt before logging to history) if self.rag_engine and self.rag_config and self.rag_config.enabled: rag_result = self._rag_search_result(user_msg) if rag_result.ok and rag_result.data: context_block = "## Retrieved Context\n\n" for i, chunk in enumerate(rag_result.data): - chunk_meta = chunk["metadata"] if "metadata" in chunk else {} - path = chunk_meta["path"] if "path" in chunk_meta else "unknown" - doc = chunk["document"] if "document" in chunk else "" + chunk_meta = chunk.get("metadata", {}) if isinstance(chunk, dict) else {} + path = chunk_meta.get("path", "unknown") if isinstance(chunk_meta, dict) else "unknown" + doc = chunk.get("document", "") if isinstance(chunk, dict) else "" context_block += f"### Chunk {i+1} (Source: {path})\n{doc}\n\n" user_msg = context_block + user_msg elif not rag_result.ok: diff --git a/tests/test_rag_phase4_final_verify.py b/tests/test_rag_phase4_final_verify.py index 4eb10ad0..9bb13e0c 100644 --- a/tests/test_rag_phase4_final_verify.py +++ b/tests/test_rag_phase4_final_verify.py @@ -17,23 +17,28 @@ def test_phase4_final_verify(live_gui, live_gui_workspace): client = api_hook_client.ApiHookClient() assert client.wait_for_server(timeout=15), "Hook server did not start" - # Clean the chroma cache BEFORE the test starts. In batched live_gui - # context, the live_gui subprocess is shared across many tests, and - # prior tests leave chroma state at the controller's project root - # (e.g. C:\projects\manual_slop\tests\artifacts\.slop_cache\chroma_test_*). - # The dim-mismatch rmtree in rag_engine._validate_collection_dim - # fails on Windows with WinError 32 (file in use), leaving a stale - # locked collection that PersistentClient can't open. Wipe the - # relevant cache dirs proactively so the test starts clean. - _workspace_root = str(live_gui_workspace.parent if live_gui_workspace else Path.cwd()) - stale_path = Path(_workspace_root) / ".slop_cache" - if stale_path.exists(): - for col_dir in stale_path.iterdir(): + # Use a unique collection name per test invocation. The RAG engine + # stores its chromadb collection at + # /.slop_cache/chroma_. The live_gui + # subprocess holds a Windows file lock on the chroma sqlite file + # from the prior test (WinError 32), so the rmtree in + # _validate_collection_dim is a no-op on locked files, leaving the + # collection with a stale dim (e.g. 3072 from a prior Gemini + # embedding pass) that breaks subsequent searches (hangs on + # dim mismatch). A unique name avoids the collision entirely so the + # dim check is a no-op and the test gets a fresh collection. + _collection_name = f"test_final_verify_{int(time.time() * 1000)}" + + # Best-effort cleanup of the workspace's .slop_cache (where the + # chroma collection actually lives). ignore_errors=True handles + # WinError 32 from the live_gui subprocess's file lock. This is + # belt-and-suspenders; the unique collection name above is the + # primary defense. + _slop_cache = Path(live_gui_workspace) / ".slop_cache" + if _slop_cache.exists(): + for col_dir in _slop_cache.iterdir(): if col_dir.is_dir() and col_dir.name.startswith("chroma_"): - try: - shutil.rmtree(col_dir) - except Exception: - pass + shutil.rmtree(col_dir, ignore_errors=True) # 1. Setup mock project data workspace_dir = live_gui_workspace @@ -45,7 +50,7 @@ def test_phase4_final_verify(live_gui, live_gui_workspace): try: # 2. Configure project through Hook API - client.set_value('rag_collection_name', 'test_final_verify') + client.set_value('rag_collection_name', _collection_name) client.set_value('files', ['final_test_1.txt', 'final_test_2.py']) client.set_value('rag_enabled', True) client.set_value('rag_source', 'chroma')