fix: memory dashboard delete + model picker empty list (#23, #24)#25
Conversation
- Fix memory delete: read file content instead of path to match IDs - Fix model picker: load from all connected providers, not just OAuth - Add /connected_providers API endpoint for API-key-based providers - Move model picker from top bar to bottom actions bar Closes #23 Closes #24 Made-with: Cursor
When the Cognee data item file cannot be read (e.g. path is not a real file), fall back to returning raw_data_location as the searchable string. This preserves backward compatibility with tests and older data while still reading file content when available (fixing #23). Made-with: Cursor
Nafania
left a comment
Nafania
left a comment
There was a problem hiding this comment.
Code Review: PR #25 — Fix Memory Delete + Model Picker (#23, #24)
Strengths
-
Correct root cause fix for #23.
_read_data_item_content()properly reads the file atraw_data_locationto find the[META:{"id":"..."}]header, fixing the mismatch between where IDs live (file content) and where the old code searched (file path). -
Good DRY refactor. Duplicate file-reading logic extracted into a single shared function in
memory.py— all three call sites (delete_documents_by_ids,_delete_data_by_id, dashboard listing) are now consistent. -
Graceful fallback chain.
_read_data_item_contenttries: file read →raw_data_locationas string →item.name. Handles missing/unreadable files without breaking. -
Clean model-listing architecture. OAuth-first with API-key fallback is logical. Parallel model fetching with
Promise.allin the frontend is efficient. -
Good UI implementation. Upward-opening dropdown for bottom-bar placement, loading state, empty state messaging, context length display, vision tags, and "Reset to default".
Issues Summary
| Severity | ID | Issue |
|---|---|---|
| 🟥 Critical | C1 | aiohttp is an undeclared dependency — runtime crash risk |
| 🟧 Important | I1 | No tests for _read_data_item_content (the core #23 fix) |
| 🟧 Important | I2 | No tests for _list_models_via_api |
| 🟧 Important | I3 | No tests for ProviderPool.list_models API-key path |
| 🟧 Important | I4 | No tests for ConnectedProviders API handler |
| 🟧 Important | I5 | Underscore-prefixed function exported as public API |
| 🟧 Important | I6 | Vision detection only works for OpenRouter |
| 🟨 Minor | M1 | Synchronous file I/O in async context |
| 🟨 Minor | M2 | _API_BASES dict recreated on every call |
| 🟨 Minor | M3 | Model list only loaded once on init() — no refresh without page reload |
| 🟨 Minor | M4 | Inline styles mixed with CSS classes in bottom-actions.html |
Recommendations
- Before merge: fix C1 — replace
aiohttpwithhttpx.AsyncClient(already a declared dependency). - Before or immediately after merge: add tests for I1–I4. At minimum:
- 5 tests for
_read_data_item_content(file found,file://URI, file missing, noraw_data_location, exception) - 4 tests for
_list_models_via_api(success, non-200, network error, empty data) - 2 tests for
list_modelsAPI-key path - 2 tests for
ConnectedProvidershandler
- 5 tests for
- Consider extending provider config (
providers.yaml) for API base URLs instead of the hardcoded_API_BASESdict.
Assessment
Ready to merge? ❌ No — fix C1 first.
Core logic for both bug fixes is correct and well-reasoned. The one blocker is aiohttp being an undeclared dependency — it's a simple swap to httpx which is already available. After that fix, the PR is functionally ready. Test gaps (I1–I4) are significant but shouldn't block merge given active user impact.
| async def _list_models_via_api(provider_id: str, api_key: str) -> list[ModelInfo]: | ||
| """List models from a provider using its OpenAI-compatible /models endpoint.""" | ||
| from python.helpers.providers import get_provider_config | ||
| import aiohttp |
There was a problem hiding this comment.
🟥 C1 — Critical: aiohttp is not in requirements.txt and is not used anywhere else in the codebase. In a clean install without Cognee's transitive deps, this will raise ModuleNotFoundError the first time any API-key provider tries to list models.
The project already declares httpx>=0.27.0. Replace with:
import httpx
async with httpx.AsyncClient(timeout=15.0) as client:
resp = await client.get(models_url, headers={"Authorization": f"Bearer {api_key}"})
if resp.status_code != 200:
logger.warning("Models API returned %d for %s", resp.status_code, provider_id)
return []
data = resp.json()|
|
||
| from python.helpers.api import ApiHandler, Request, Response | ||
| from python.helpers.memory import Memory, get_existing_memory_subdirs, get_context_memory_subdir | ||
| from python.helpers.memory import Memory, get_existing_memory_subdirs, get_context_memory_subdir, _read_data_item_content |
There was a problem hiding this comment.
🟧 I5 — Important: _read_data_item_content uses the underscore convention indicating it's module-private, but it's imported here as a cross-module dependency. Either:
- Rename to
read_data_item_content(no underscore) to acknowledge its public status, or - Route the call through a public method on
Memory(e.g.Memory.read_data_item_content(item))
| continue | ||
| name = m.get("name") or mid | ||
| ctx = m.get("context_length") or m.get("context_window") or 0 | ||
| vision = False |
There was a problem hiding this comment.
🟧 I6 — Important: The architecture.modality field is OpenRouter-specific. OpenAI, Mistral, Groq, xAI, DeepSeek don't return this in their /models response. supports_vision will always be False for non-OpenRouter providers.
At minimum document this limitation. Ideally, add provider-specific parsing (e.g. checking model ID patterns like gpt-4o, gemini-*-pro-vision) or just skip the vision flag for non-OpenRouter providers.
| kwargs = config.get("kwargs", {}) or {} | ||
| api_base = kwargs.get("api_base", "") | ||
|
|
||
| _API_BASES = { |
There was a problem hiding this comment.
🟨 M2 — Minor: This constant dict is recreated on every function call. Move to module level. Also consider pulling API base URLs from providers.yaml / get_provider_config instead of maintaining a parallel hardcoded list that can drift.
| return text, {"area": Memory.Area.MAIN.value} | ||
|
|
||
|
|
||
| def _read_data_item_content(item) -> str: |
There was a problem hiding this comment.
🟧 I1 — Important: This function is the core fix for #23 and has zero test coverage. Needs at least 4–5 unit tests:
- Valid file at
raw_data_location→ returns file content file://URI scheme → correctly resolves and reads- File doesn't exist → falls back to
raw_data_locationstring raw_data_locationisNone→ falls back toitem.name- File exists but unreadable → falls back gracefully
🟨 M1 — Minor: Synchronous open() + read() in an async call path. For large datasets with many items, this could block the event loop. Consider asyncio.to_thread() wrapping if performance becomes an issue.
| open: false, | ||
| loading: false, | ||
|
|
||
| init() { |
There was a problem hiding this comment.
🟨 M3 — Minor: Models are loaded once on init() and never refreshed. If the user configures a new provider's API key in Settings, they'd need to reload the page to see new models. A refresh button in the dropdown or periodic re-fetch (e.g. on each toggle when data is stale) would improve UX.
- Replace aiohttp with httpx in _list_models_via_api (C1) - Rename _read_data_item_content → read_data_item_content for public API (I5) - Move _API_BASES to module-level _KNOWN_API_BASES, extract _resolve_api_base helper (M2) - Add 5 unit tests for read_data_item_content (I1) - Refresh models on toggle when data is stale (>5 min) (M3) Made-with: Cursor
- Add tests for _list_models_via_api (success, non-200, network error, empty data, no api base, OpenRouter vision detection) - Add tests for ProviderPool.list_models API-key fallback path (no credential, happy path, caching, stale cache fallback) - Add tests for ConnectedProviders API handler - Document vision detection as OpenRouter-specific - Add read_data_item_content_async to avoid blocking the event loop - Move inline styles to CSS classes in bottom-actions.html Made-with: Cursor
Review Update — All Issues AddressedCommits
All 32 tests pass. Ready to merge. |
Pre-existing tests still referenced _read_data_item_content (with underscore). Updated to read_data_item_content and fixed assertion for missing-file fallback (returns raw_data_location, not name). Made-with: Cursor
- Add max_agent_depth to AgentConfig (default 5) - Refuse delegation when depth limit reached - Detect same-profile delegation loops (streak >= 2) - Add depth awareness to agent info prompt - Show subordinate status in agent info Made-with: Cursor
Mock agents need explicit max_agent_depth=5 on config to avoid MagicMock vs int comparison errors in depth/loop checks. Update agent info test assertion to include new prompt parameters. Made-with: Cursor
feat(#25): Subordinate delegation loop prevention and depth limits
1 participant
Summary
delete_documents_by_idssearched for the ID in the file path (raw_data_location) instead of the file content where the[META:{"id":"..."}]header is stored./modelsendpoints (OpenRouter, OpenAI, DeepSeek, Groq, Mistral, xAI, etc.).Changes
python/helpers/memory.py_read_data_item_content(), fixeddelete_documents_by_idsand_delete_data_by_idto read file contentpython/api/memory_dashboard.py_read_data_item_contentfrom memory modulepython/helpers/connected_providers.pylist_models()with API-key fallback via_list_models_via_api()python/api/connected_providers.py/connected_providersendpoint returning all providers with valid credentialswebui/js/model-picker.jsinit(),toggle()webui/components/chat/top-section/chat-top.htmlwebui/components/chat/input/bottom-actions.htmlTest plan
tests/helpers/test_connected_providers.py— 11 passedtests/api/test_provider_models.py— 3 passedCloses #23
Closes #24
Made with Cursor