feat(memory): system-wide memory model; drop per-agent model (#580)#584
Conversation
…ent model (#580) The memory/Librarian model is now a single system-wide setting (taOSmd's get_memory_model/set_memory_model), managed in the Memory app. Remove the per-agent model dropdown from agent Memory settings (enable/tasks/fanout stay per-agent) and stop forwarding the dropped per-agent model to taOSmd. - GET/PUT /api/memory/model (guarded for older taosmd without the API). - Memory app gains a single Memory-model picker + 'use built-in default'. - MemoryTab loses the per-agent model select; LibrarianPatch drops model/clear_model.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds system-wide memory model support: new GET/PUT /api/memory/model endpoints and schemas, backend tests, a frontend client module and MemoryModelSection UI placed in MemorySettings, removal of the per-agent librarian model selector, and matching frontend tests. ChangesMemory Model System-Wide Configuration
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as MemoryModelSection
participant API as /api/memory/model
participant taosmd
User->>Frontend: Mounts / interacts (Change / Clear)
Frontend->>API: GET /api/memory/model
API->>taosmd: getattr(get_memory_model)
taosmd-->>API: current model or missing
API-->>Frontend: {model, supported}
alt User selects model
Frontend->>API: PUT {model}
API->>taosmd: set_memory_model(model, clear=false)
taosmd-->>API: success / raises
API-->>Frontend: 200 {model} or 400 error
else User clears
Frontend->>API: PUT {clear:true}
API->>taosmd: set_memory_model(None, clear=true)
taosmd-->>API: success / raises
API-->>Frontend: 200 {model:null} or 400 error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The system-wide memory model is install-level config; gate the write with _require_admin (mirrors other admin-only settings) so a non-admin session can't change which model powers memory for everyone. GET stays open — it returns only a model-id string, and gating it would break the read-only display for non-admin users. Addresses the commit security review finding.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
desktop/src/components/memory/MemorySettings.tsx (1)
30-43: 💤 Low valueConsider logging fetch errors for easier debugging.
The empty catch block on line 41 silently ignores errors when fetching available models. This makes it harder to diagnose issues if the models endpoint fails. Consider adding
console.error(err)or a subtle user-facing indicator.🔍 Optional enhancement
} catch { /* leave empty */ } + } catch (err) { + console.error("Failed to load models for picker:", err); } finally { setModelsLoaded(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/components/memory/MemorySettings.tsx` around lines 30 - 43, The openPicker function swallows fetch errors in an empty catch which hinders debugging; update the catch to log the caught error (e.g., console.error(err) or use a logger) and optionally set a small user-facing state/flag (or call setModelsLoaded with an error state) so the UI can indicate the fetch failed, keeping the existing finally that calls setModelsLoaded(true) or adjusting it to reflect the error state if you add an error flag; refer to openPicker, setModels, setModelsLoaded and modelsLoaded when making these changes.tinyagentos/routes/librarian.py (1)
63-64: 💤 Low valueConsider logging or narrowing the exception handler.
The broad
Exceptioncatch ensures robustness when calling the externaltaosmd.set_memory_modelAPI, but it may obscure unexpected failures. Consider logging the exception before returning the 400 response, or catching more specific exception types if known.📋 Optional: Add exception logging
+import logging + +logger = logging.getLogger(__name__) + ... try: setter(body.model or "", clear=body.clear) except Exception as exc: + logger.exception("taosmd.set_memory_model failed") return JSONResponse(status_code=400, content={"detail": str(exc)})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/librarian.py` around lines 63 - 64, The except block catching Exception when calling taosmd.set_memory_model should either log the error or narrow the exception types; update the handler that contains the taosmd.set_memory_model call to call logger.exception(...) (or use the existing module logger) before returning the JSONResponse so the stack/trace is recorded, and if you know specific errors from taosmd (e.g., TaosMDError, HTTPError, ValueError) replace the broad except Exception with individual except clauses to return 400 for expected client errors and let unexpected exceptions propagate or be handled by a global error handler; ensure you reference the existing taosmd.set_memory_model call and the JSONResponse return when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@desktop/src/components/memory/MemorySettings.tsx`:
- Around line 30-43: The openPicker function swallows fetch errors in an empty
catch which hinders debugging; update the catch to log the caught error (e.g.,
console.error(err) or use a logger) and optionally set a small user-facing
state/flag (or call setModelsLoaded with an error state) so the UI can indicate
the fetch failed, keeping the existing finally that calls setModelsLoaded(true)
or adjusting it to reflect the error state if you add an error flag; refer to
openPicker, setModels, setModelsLoaded and modelsLoaded when making these
changes.
In `@tinyagentos/routes/librarian.py`:
- Around line 63-64: The except block catching Exception when calling
taosmd.set_memory_model should either log the error or narrow the exception
types; update the handler that contains the taosmd.set_memory_model call to call
logger.exception(...) (or use the existing module logger) before returning the
JSONResponse so the stack/trace is recorded, and if you know specific errors
from taosmd (e.g., TaosMDError, HTTPError, ValueError) replace the broad except
Exception with individual except clauses to return 400 for expected client
errors and let unexpected exceptions propagate or be handled by a global error
handler; ensure you reference the existing taosmd.set_memory_model call and the
JSONResponse return when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56c11e1b-7a13-4178-b049-4c1f57481b5b
📒 Files selected for processing (7)
desktop/src/apps/__tests__/MemoryApp.test.tsxdesktop/src/components/agent-settings/MemoryTab.tsxdesktop/src/components/memory/MemorySettings.tsxdesktop/src/lib/memory-api.tstests/test_librarian_api.pytests/test_memory_model.pytinyagentos/routes/librarian.py
💤 Files with no reviewable changes (1)
- desktop/src/components/agent-settings/MemoryTab.tsx
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if not body.clear and not (body.model and body.model.strip()): | ||
| return JSONResponse(status_code=400, content={"detail": "model is required unless clear=true"}) | ||
| try: | ||
| setter(body.model or "", clear=body.clear) |
There was a problem hiding this comment.
WARNING: Potential issue with None to empty string conversion
When body.model is None (which happens when clearing without specifying a model), the code converts it to empty string via body.model or "". This may not be correct if the taosmd.set_memory_model function expects None to have a special meaning.
Looking at the frontend code:
handleClear()callssetMemoryModel({ clear: true })(no model field)- This results in
body.model = Nonein the endpoint - The validation correctly allows this when
body.clear = true
However, we then pass "" (empty string) to the setter instead of None.
Since the MemoryModelUpdate schema allows model: str | None = None, and the getter returns None when no model is set, it seems more appropriate to pass body.model directly to preserve the None value when no model is specified.
Suggested fix: Pass body.model directly instead of converting None to empty string.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
When Looking at the frontend code:
However, we then pass Since the Suggested fix: Pass Files Reviewed (7 files)
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 506,526 tokens |
1 participant
Completes the memory-model move (taOS#580). Pairs with taOSmd#82 (shipped):
taosmd.get_memory_model()/set_memory_model(model, clear=False).What
The memory/Librarian model is now a single system-wide setting (managed in the Memory app), not per-agent. Memory is shared infrastructure, so its model is a quality/infra choice — distinct from each agent's per-agent chat model.
routes/librarian.py):GET/PUT /api/memory/modelcalling taOSmd's global getter/setter, guarded withgetattrso an older taosmd degrades gracefully (supported:false/ 501) instead of crashing. Removedmodel/clear_modelfromLibrarianPatchand stopped forwarding them (taOSmd dropped per-agent model).<select>from the agent's Memory tab (Enable Librarian / tasks / fanout stay per-agent). Added a single Memory model control to the Memory app settings (MemorySettings.tsx) — shows the current model or "Built-in default", with a picker (reusingModelPickerModal) and a "use built-in default" action. Newmemory-api.tshelpers.Model id format is
provider:model(e.g.ollama:qwen3:4b);null= built-in default.Tests
tests/test_memory_model.py(8: GET model/null/unsupported, PUT set/clear/400/501),test_librarian_api.pyupdated (model no longer forwarded),MemoryApp.test.tsx(4). Full vitest 973 pass;npm run buildclean.Summary by CodeRabbit
New Features
Refactor
Tests