fix: resolve P1 bugs (streaming UX + api key persistence) and update tests/docs

docs/bugs/P1_MAGENTIC_STREAMING_AND_KEY_PERSISTENCE.md

@@ -133,178 +133,25 @@ Gradio's `ChatInterface` with `additional_inputs` has known issues:
 
 ### Fix Applied
 **Files Modified:**
-1. `src/app.py:111` - Added `api_key_state: str = ""` parameter to `research_agent()`
-2. `src/app.py:133` - Logic: Use `api_key` if present, else fallback to `api_key_state`
-3. `src/app.py:219` - Created `api_key_state = gr.State("")` component
-4. `src/app.py:234-252` - Added empty `api_key_state` values to examples
-5. `src/app.py:268` - Added `api_key_state` to `additional_inputs` list
+1. `src/app.py`
+2. `src/utils/llm_factory.py`
 
-The `gr.State` component persists across example clicks, providing a fallback when the textbox is reset.
-
-### Proposed Fix Options
-
-**Option A: Use `gr.State` for persistence**
-```python
-api_key_state = gr.State("")
-
-def research_agent(message, history, mode, api_key, api_key_state):
-    # Use api_key_state if api_key is empty
-    effective_key = api_key or api_key_state
-    ...
-    return response, effective_key  # Return to update state
-```
-
-**Option B: Use browser localStorage via JavaScript**
-```python
-demo.load(js="""
-    () => {
-        const saved = localStorage.getItem('deepboner_api_key');
-        if (saved) document.querySelector('input[type=password]').value = saved;
-    }
-""")
-```
-
-**Option C: Environment variable only (remove BYOK textbox)**
-Remove the API key input entirely. Require users to set `OPENAI_API_KEY` in HuggingFace Secrets. This is more secure but less user-friendly.
-
-**Option D: Use Gradio LoginButton or HuggingFace OAuth**
-Leverage HF's built-in auth and secrets management.
-
----
-
-## Bug 3: Deprecated `OpenAIModel` Import
-
-### Symptoms
-HuggingFace Spaces logs show deprecation warning:
-```
-DeprecationWarning: OpenAIModel is deprecated, use OpenAIChatModel instead
-```
-
-### Root Cause
-**Files using deprecated API:**
-- `src/app.py:9` - `from pydantic_ai.models.openai import OpenAIModel`
-- `src/utils/llm_factory.py:59` - `from pydantic_ai.models.openai import OpenAIModel`
-
-**File already using correct API:**
-- `src/agent_factory/judges.py:12` - `from pydantic_ai.models.openai import OpenAIChatModel`
-
-### Fix
-Replace all `OpenAIModel` imports with `OpenAIChatModel`:
-
-```python
-# Before (deprecated)
-from pydantic_ai.models.openai import OpenAIModel
-model = OpenAIModel(settings.openai_model, provider=provider)
-
-# After (correct)
-from pydantic_ai.models.openai import OpenAIChatModel
-model = OpenAIChatModel(settings.openai_model, provider=provider)
-```
-
-**Files to update:**
-1. `src/app.py` - lines 9, 64, 73
-2. `src/utils/llm_factory.py` - lines 59, 67
-
----
-
-## Bug 4: Asyncio Event Loop Garbage Collection Error
-
-### Symptoms
-HuggingFace Spaces logs show intermittent errors:
-```
-ValueError: Invalid file descriptor: -1
-Exception ignored in: <function BaseSelector.__del__ at 0x...>
-```
-
-### Root Cause
-This occurs during garbage collection of asyncio event loops. Likely causes:
-1. Event loop cleanup timing issues in Gradio's threaded model
-2. Selector objects being garbage-collected before proper cleanup
-3. Concurrent access to event loop resources during shutdown
-
-### Analysis
-The codebase uses `asyncio.get_running_loop()` correctly (not the deprecated `get_event_loop()`).
-This error appears to be a Gradio/HuggingFace Spaces environment issue rather than a code bug.
-
-### Potential Mitigations
-1. **Add explicit cleanup**: Use `asyncio.get_event_loop().close()` in appropriate places
-2. **Ignore in logs**: This is a known Python issue and can be safely ignored if it doesn't affect functionality
-3. **File issue with Gradio**: If reproducible, report to Gradio GitHub
-
-### Impact
-- **Severity**: Low - appears to be a cosmetic log issue
-- **User-visible**: No - errors occur during garbage collection, not during request handling
-
----
-
-## Recommended Priority
-
-1. **Bug 1 (Streaming Spam)**: HIGH - makes Advanced mode unusable for reading output
-2. **Bug 3 (OpenAIModel deprecation)**: MEDIUM - fix to avoid future breakage
-3. **Bug 2 (Key Persistence)**: LOW - annoying but users can re-paste
-4. **Bug 4 (Asyncio GC)**: LOW - cosmetic log noise, monitor but likely no action needed
-
-## Testing Plan
-
-1. Run Advanced mode query, verify no token-by-token spam
-2. Verify no deprecation warnings in logs after OpenAIChatModel fix
-3. Paste API key, click example, verify key persists
-4. Refresh page, verify key persists (if using localStorage)
-5. Run `make check` - all tests pass
-
----
-
-## Fix Summary (2025-11-29)
-
-### ✅ Bug 1: Token-by-Token Streaming Spam - FIXED
-
-**Root Cause Analysis:**
-- Validated the exact data flow from `orchestrator_magentic.py` → `models.py` → `app.py`
-- Confirmed O(N²) complexity: For N tokens, yielding N times with full history each time
-- Each `MagenticAgentDeltaEvent` created individual `AgentEvent(type="streaming")`
-
-**Fix Implementation:**
-- **File:** `/Users/ray/Desktop/CLARITY-DIGITAL-TWIN/DeepBoner/src/app.py`
-- **Lines Modified:** 158, 171-197
-- **Strategy:** Streaming token buffering (Option A from proposals)
-  1. Added `streaming_buffer = ""` variable
-  2. When `event.type == "streaming"`: accumulate in buffer, skip yield
-  3. On non-streaming events: flush buffer, reset
-  4. At completion: flush any remaining buffer
-- **Result:** One consolidated streaming message instead of hundreds of individual tokens
-
-**Validation:**
-- Created unit test: `tests/unit/test_streaming_fix.py::test_streaming_events_are_buffered_not_spammed`
-- Test verifies max 1 buffered streaming message (not N individual ones)
-- All 138 tests pass
-
-### ✅ Bug 2: API Key Persistence - FIXED
-
-**Root Cause Analysis:**
-- Validated Gradio `ChatInterface.additional_inputs` limitation
-- Clicking examples resets textbox values to defaults
-- No state persistence mechanism existed
-
-**Fix Implementation:**
-- **File:** `/Users/ray/Desktop/CLARITY-DIGITAL-TWIN/DeepBoner/src/app.py`
-- **Lines Modified:** 111, 133, 219, 234-252, 268
-- **Strategy:** `gr.State` for persistence (Option A from proposals)
-  1. Added `api_key_state: str = ""` parameter to `research_agent()`
-  2. Logic: Use `api_key` if present, else fallback to `api_key_state`
-  3. Created `api_key_state = gr.State("")` component
-  4. Added to `additional_inputs` list
-  5. Updated examples with empty state placeholders
-- **Result:** API key persists across example clicks via state component
-
-**Validation:**
-- Created unit test: `tests/unit/test_streaming_fix.py::test_api_key_state_parameter_exists`
-- Test verifies parameter exists and signature is correct
-- All 138 tests pass
-
-### Files Modified
-1. `/Users/ray/Desktop/CLARITY-DIGITAL-TWIN/DeepBoner/src/app.py` - Streaming buffering + API key state
-2. `/Users/ray/Desktop/CLARITY-DIGITAL-TWIN/DeepBoner/docs/bugs/P1_MAGENTIC_STREAMING_AND_KEY_PERSISTENCE.md` - Documentation
-3. `/Users/ray/Desktop/CLARITY-DIGITAL-TWIN/DeepBoner/tests/unit/test_streaming_fix.py` - New validation tests
+**Bug 1 (Streaming Spam):**
+- Implemented "smart streaming":
+  - Accumulate tokens in `streaming_buffer`
+  - Yield updates immediately to show progress (UX improvement)
+  - **Crucially**: Do NOT append to the persistent `response_parts` list until the stream segment is complete.
+  - This prevents the O(N²) list growth and "new line spam" while keeping the UI responsive.
+
+**Bug 2 (API Key Persistence):**
+- **Strategy:** Cleaner Fix (Example List Modification)
+  - Instead of complex `gr.State` wiring (which caused context issues), we simply **removed the empty string columns** for `api_key` and `api_key_state` from the `examples` list in `create_demo`.
+  - Gradio's behavior is that if an example row has fewer columns than inputs, the remaining inputs (like the API key textbox) are **left unchanged** when the example is clicked.
+  - This naturally preserves the user's input without requiring extra state management.
+  - The `api_key_state` parameter remains in `research_agent` as a fallback but is largely redundant with this cleaner fix.
+
+**Bug 3 (OpenAIModel Deprecation):** ✅ FIXED
+- Replaced all `OpenAIModel` imports with `OpenAIChatModel` in `src/app.py` and `src/utils/llm_factory.py`.
 
 ### Test Results
 ```
@@ -312,22 +159,20 @@ uv run pytest tests/ -q
 ============================= 138 passed in 20.60s =============================
 ```
 
-**Before:** 136 tests
-**After:** 138 tests (added 2 validation tests)
 **Status:** ✅ All tests passing
 
 ### Why This Fix Works
 
 **Bug 1 (Streaming Spam):**
-- **Before:** Every token → `append()` → `yield "\n\n".join(all_parts)` → O(N²) spam
-- **After:** Every token → `buffer += token` → Skip yield → O(1) per token, O(N) total
-- **Impact:** Reduced from hundreds of UI updates to ~1-2 consolidated messages
+- **Before:** Every token → `append()` to list → `yield` → List grew to size N → O(N²) complexity.
+- **After:** Every token → `yield` dynamically constructed string (buffer + history) → List stays size K (number of *events*).
+- **Impact:** Smooth streaming, no visual spam, no browser freeze.
 
 **Bug 2 (API Key):**
-- **Before:** Textbox value lost on example click (Gradio limitation)
-- **After:** `gr.State` survives example clicks, fallback logic ensures key persists
-- **Impact:** User doesn't need to re-paste key after clicking examples
+- **Before:** Example click → Overwrote API Key textbox with `""`.
+- **After:** Example click → Updates only `message` and `mode` → API Key textbox untouched.
+- **Impact:** User input persists naturally.
 
 ### Remaining Work
-- **Bug 3 (OpenAIModel deprecation):** Not addressed in this fix - separate issue
 - **Bug 4 (Asyncio GC errors):** Monitoring only - likely Gradio/HF Spaces issue
+

src/app.py

@@ -177,7 +177,10 @@ async def research_agent(
             if event.type == "streaming":
                 # Accumulate streaming tokens without emitting individual events
                 streaming_buffer += event.message
-                # Don't append to response_parts or yield - just buffer
+                # Yield the current buffer combined with previous parts to show progress
+                # But DO NOT append to response_parts list yet (to avoid O(N^2) list growth)
+                current_parts = [*response_parts, f"📡 **STREAMING**: {streaming_buffer}"]
+                yield "\n\n".join(current_parts)
                 continue
 
             # For non-streaming events, flush any buffered streaming content first
@@ -235,20 +238,15 @@ def create_demo() -> tuple[gr.ChatInterface, gr.Accordion]:
             [
                 "What drugs improve female libido post-menopause?",
                 "simple",
-                "",  # api_key placeholder for examples
-                "",  # api_key_state placeholder for examples
+                # Removed empty strings for api_key and api_key_state to prevent overwriting
             ],
             [
                 "Clinical trials for erectile dysfunction alternatives to PDE5 inhibitors?",
                 "advanced",
-                "",  # api_key placeholder
-                "",  # api_key_state placeholder
             ],
             [
                 "Evidence for testosterone therapy in women with HSDD?",
                 "simple",
-                "",  # api_key placeholder
-                "",  # api_key_state placeholder
             ],
         ],
         additional_inputs_accordion=additional_inputs_accordion,
@@ -269,6 +267,15 @@ def create_demo() -> tuple[gr.ChatInterface, gr.Accordion]:
         ],
     )
 
+    # Wire up API key change to update state
+    # This ensures that when user types, state is updated.
+    # When examples are clicked (and only modify first 2 args), state remains.
+    # Note: This requires a Blocks context, which ChatInterface doesn't expose easily here.
+    # However, by removing the empty strings from the examples list above,
+    # we prevent the API key from being overwritten in the first place,
+    # so the api_key textbox retains its value, and research_agent receives it directly.
+    # api_key_input.change(lambda x: x, inputs=api_key_input, outputs=api_key_state)
+
     return demo, additional_inputs_accordion
 
 

tests/unit/test_streaming_fix.py

@@ -51,18 +51,38 @@ async def test_streaming_events_are_buffered_not_spammed():
         async for result in research_agent("test query", [], mode="simple", api_key=""):
             results.append(result)
 
-        # Verify that we don't have individual streaming events in the output
-        # Before fix: Would see "📡 **STREAMING**: This", "📡 **STREAMING**: is", etc.
-        # After fix: Should see buffered content only
-
-        # Count how many times we see streaming markers
-        streaming_count = sum(1 for r in results if "📡 **STREAMING**:" in r)
-
-        # Should be at most 1 streaming message (buffered), not 4 (one per token)
-        assert streaming_count <= 1, (
-            f"Expected at most 1 buffered streaming message, got {streaming_count}. "
-            f"This indicates token-by-token spam is still happening!"
-        )
+        # Verify that we DO see streaming updates (for UX responsiveness)
+        # But we don't want O(N^2) growth of the persisted list.
+
+        # We expect results to contain the streaming updates
+        assert len(results) > 0, "Should have yielded results"
+
+        # Check that we see the accumulated message
+        assert any(
+            "📡 **STREAMING**: This is a test" in r for r in results
+        ), "Buffer didn't accumulate correctly"
+
+        # The critical check for the "Spam" bug:
+        # In the spam bug, the output grew like:
+        # "Stream: T"
+        # "Stream: T\nStream: h"
+        # "Stream: T\nStream: h\nStream: i"
+        #
+        # In the fixed version, it should look like:
+        # "Stream: T"
+        # "Stream: Th"
+        # "Stream: Thi"
+        # (Replacing the last line, not adding new lines)
+
+        for res in results:
+            # Count occurrences of "📡 **STREAMING**:": in a single result string
+            # It should appear AT MOST once
+            # (unless we have multiple distinct streaming blocks)
+            streaming_markers = res.count("📡 **STREAMING**:")
+            assert streaming_markers <= 1, (
+                f"Found multiple streaming markers in single response: {res}\n"
+                "This indicates we are appending new lines instead of updating in place."
+            )
 
         # The final result should be the complete message
         assert any("Final answer" in r for r in results), "Missing final complete message"