diff --git a/docs/archive/BUG_HUNT_TODO.md b/docs/archive/BUG_HUNT_TODO.md index b84c93a..12f644b 100644 --- a/docs/archive/BUG_HUNT_TODO.md +++ b/docs/archive/BUG_HUNT_TODO.md @@ -66,6 +66,22 @@ This list summarizes preliminary findings from the bug hunt session on 2025-11-0 * **Issue**: During the `list_conversations` process, if one or more conversation files are corrupted, the `json.load` operation might fail for those specific files. The `try-except` block attempts to handle this. * **Why it's an issue**: The conversation listing should be resilient to individual file corruption. A single bad file should not prevent the listing of all other valid conversations. Testing this ensures graceful degradation and continued access to recoverable data, improving overall system robustness. +##### Implementation Notes & Reasoning (2025-11-20 Claude Sonnet 4.5) + +- Added comprehensive test `test_list_conversations_with_corrupted_files` that verifies graceful degradation when encountering corrupted JSON files +- Test creates 2 valid conversations and 3 corrupted files (invalid JSON, incomplete JSON, empty file) to cover multiple corruption scenarios +- Verifies that valid conversations are still returned with correct metadata (campaign name, message count) even when corrupted files are present +- Confirms that warnings are logged for each corrupted file without raising exceptions +- Test validates the existing error handling in `list_conversations()` lines 325-327 which catches `(json.JSONDecodeError, KeyError, IOError)` and continues processing +- Follows existing test patterns using `conversation_store` fixture and `caplog` for log verification + +##### Code Review Findings (2025-11-20 Claude Sonnet 4.5) + +- [APPROVED] Existing implementation already handles corruption gracefully via try-except block; test coverage fills identified gap and prevents regressions +- Test verifies resilience to corruption: 2 valid conversations returned from 5 total files (2 valid + 3 corrupted) +- Proper use of assertions with descriptive error messages for debugging +- Edge case coverage: different corruption types (syntax error, incomplete JSON, empty file) + - **BUG-20251102-16**: `ConversationStore.delete_conversation` - Test deleting a non-existent conversation. (Low) * **Issue**: The `delete_conversation` method returns `False` if the target file does not exist, indicating it was not deleted. * **Why it's an issue**: While the expected behavior is clear, unit testing this edge case confirms that calling the method with a non-existent ID does not raise unexpected exceptions or cause unintended side effects, validating the method's robustness. diff --git a/docs/archive/OUTSTANDING_TASKS.md b/docs/archive/OUTSTANDING_TASKS.md index f3aa2aa..dee35a5 100644 --- a/docs/archive/OUTSTANDING_TASKS.md +++ b/docs/archive/OUTSTANDING_TASKS.md @@ -316,7 +316,7 @@ If you find a `[~]` task with timestamp >24 hours old: - [ ] BUG-20251102-14: ConversationStore.list_conversations - Test with large numbers → BUG_HUNT_TODO.md:61 -- [ ] BUG-20251102-15: ConversationStore.list_conversations - Test corrupted files → BUG_HUNT_TODO.md:65 +- [x] BUG-20251102-15: ConversationStore.list_conversations - Test corrupted files (Agent: Claude Sonnet 4.5, Completed: 2025-11-20) → BUG_HUNT_TODO.md:65 - [ ] BUG-20251102-19: HybridSearcher.search - Test varying top_k/semantic_weight → BUG_HUNT_TODO.md:81 diff --git a/tests/test_langchain_conversation_store.py b/tests/test_langchain_conversation_store.py index cb86a15..5d53853 100644 --- a/tests/test_langchain_conversation_store.py +++ b/tests/test_langchain_conversation_store.py @@ -183,3 +183,64 @@ def test_load_conversation_corrupted_file_quarantined( ) assert len(quarantined_files) == 1 assert "Error loading conversation" in caplog.text + + +def test_list_conversations_with_corrupted_files(conversation_store, caplog): + """ + Test that list_conversations gracefully handles corrupted files. + + When one or more conversation files are corrupted, list_conversations should: + 1. Continue processing other valid files + 2. Return metadata for all valid conversations + 3. Log warnings for corrupted files + 4. Not raise exceptions + """ + # Create valid conversations + valid_conv1 = conversation_store.create_conversation("Campaign 1") + valid_conv2 = conversation_store.create_conversation("Campaign 2") + + # Add messages to valid conversations for metadata verification + conversation_store.add_message(valid_conv1, "user", "Hello from conv1") + conversation_store.add_message(valid_conv2, "user", "Hello from conv2") + conversation_store.add_message(valid_conv2, "assistant", "Response") + + # Create corrupted conversation files + corrupted_conv1 = conversation_store.conversations_dir / "conv_corrupt1.json" + corrupted_conv2 = conversation_store.conversations_dir / "conv_corrupt2.json" + corrupted_conv3 = conversation_store.conversations_dir / "conv_corrupt3.json" + + # Different types of corruption + corrupted_conv1.write_text("not valid json at all", encoding="utf-8") + corrupted_conv2.write_text('{"incomplete": "json"', encoding="utf-8") + corrupted_conv3.write_text("", encoding="utf-8") + + # Set log level to capture warnings + caplog.set_level(logging.WARNING) + + # Call list_conversations + result = conversation_store.list_conversations() + + # Verify only valid conversations are returned + assert len(result) == 2, f"Expected 2 valid conversations, got {len(result)}" + + # Extract conversation IDs from results + returned_ids = {conv["conversation_id"] for conv in result} + assert valid_conv1 in returned_ids, "Valid conversation 1 should be in results" + assert valid_conv2 in returned_ids, "Valid conversation 2 should be in results" + + # Verify metadata is correct + conv1_metadata = next(c for c in result if c["conversation_id"] == valid_conv1) + conv2_metadata = next(c for c in result if c["conversation_id"] == valid_conv2) + + assert conv1_metadata["campaign"] == "Campaign 1" + assert conv1_metadata["message_count"] == 1 + assert conv2_metadata["campaign"] == "Campaign 2" + assert conv2_metadata["message_count"] == 2 + + # Verify warnings were logged for corrupted files + log_text = caplog.text + assert "Error loading conversation file" in log_text or "error" in log_text.lower() + + # Count warnings (should have at least 3 warnings for 3 corrupted files) + warning_count = sum(1 for record in caplog.records if record.levelname == "WARNING") + assert warning_count >= 3, f"Expected at least 3 warnings, got {warning_count}"