Memory: per-assistant toggle for the automatic memory system #23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/disable-memory"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Assistants whose memory summaries have gone stale or contradictory can now have the automatic memory system switched off entirely. Disabling it does two things:
runMemoryUpdate(triggerReason == 'auto'). The explicit Update Now button in the Memory tab keeps working — a deliberate choice: a manual user action should win over the toggle.The switch sits at the top of the Memory tab, right next to the content it controls, with a subtitle explaining the current state. Clearing/editing stored memory stays available while disabled.
How
Rides the
disabled_toolsmechanism from #22 with a newToolGroups.memorykey — no migration needed, the JSON column accepts new keys as designed. The key is documented as "not a tool" and deliberately excluded fromToolGroups.all, so the Tools tab doesn't duplicate the switch. Gating ismemoryRepo: enabled ? repos.memory : nullat the fiveSystemPromptBuilderconstruction sites (the builder already handled a null memory repo) plus the early return inrunMemoryUpdate.Verification
Memorysection for a normal assistant and is absent for one with["memory"]disabled;runMemoryUpdate(..., 'auto')returns null for the disabled assistant without touching the LLM. Exercised through the prompt-preview dry-run (the real composition path).updateAssistantround-trip) are each covered by the preview-based test or were verified live in #22 with the identical mechanism.🤖 Generated with Claude Code
Coverage: apps/angela_server
Total: 51.1% (137 of 268)
Coverage: packages/angela_api
Total: 5.6% (27 of 479)
Coverage: packages/angela_core
Total: 23.6% (1482 of 6271)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ A memory toggle that rides the existing
disabled_toolsmechanism instead of bolting on a new column? Knowledge sparkle. This is exactly the kind of reuse that makes a Flugel's heart sing — no migration, one new key, and the JSON column accepts it as designed. Clever girl~ ♡Verdict: ✅ Looks good to me~
I poked at every single thing that could go wrong here, and it all holds together beautifully. Let me show you what I checked~
🔍 What I verified (the obsessive part~)
All five
SystemPromptBuildersites are gated. I grepped every construction site in the codebase — there are exactly five (chat at L190, timer at L580, sub-agent at L902, Über-Ich at L1139, peer-consultation at L1418), and every single one now passesmemoryRepo: _toolEnabled(…) ? repos.memory : null. None missed. fufu~ if even one had been forgotten, memory would've leaked into a disabled assistant's prompt. But you got them all. ♡The "manual wins over toggle" semantics are correct. I traced both call paths:
chat_executor.dart:292→triggerReason: 'auto'→ blocked by the new guard ✓memory_handler.dart:61→triggerReason: 'manual'→ guard'striggerReason == 'auto'is false → runs anyway ✓The guard at
agent_runner.dart:1360is exactly right:if (triggerReason == 'auto' && !_toolEnabled(…)). A deliberate user action overriding the toggle — that's the correct hierarchy.SystemPromptBuilderalready handled null memory. Confirmed all four prompt profiles (buildChatPromptSections,buildTimerPrompt,buildUberIchPrompt,buildPeerPrompt) gate memory behindif (_memoryRepo != null). So passing null is genuinely a no-op, not a crash waiting to happen.No memory reads bypass the gate. I searched for any
memoryRepo.formatForPrompt/repos.memory.usage outsideSystemPromptBuilder— there are none in prompt composition. The gate is airtight.Excluding
memoryfromToolGroups.allis safe.disabled_toolsis a free-formSet<String>with no validation againstall, so the Tools tab won't reject or duplicate the key. Documenting it as "not a tool" in the doc comment is the right call.The UI round-trip refreshes state correctly.
updateAssistantposts the full recomputeddisabledToolslist, gets back the updated assistant, and replaces the state entry — soref.watch(assistantListProvider)in_buildEnableSwitchrebuilds with the new toggle value. No stale-state trap.The test is real, not theater. It uses an actual
AgentRunner(db)(no mock), seeds memory for both assistants, and asserts both the prompt-composition path (Memorysection present vs absent) and the update gate (runMemoryUpdate('a3')→isNull). It exercises the real composition viabuildChatPreview. That's the good stuff~💡 Little ideas (non-blocking)~
agent_runner.dart:1361—_loadAssistant(assistantId)throwsStateErrorif the assistant was deleted between the threshold check and this call. For the auto path that's probably fine (the assistant existed moments ago inchat_executor), but if you ever wanted belt-and-suspenders, atry/catchreturning null onStateErrorwould make the auto-skip bulletproof against a race. Truly optional — the current behavior (loud failure in a background task that's already wrapped in try/catch atchat_executor.dart:295) is acceptable.memory_tab.dart_setMemoryEnabled— the toggle has no optimistic update, so there's a brief moment where the switch visually snaps back until the Riverpod state refreshes after the API round-trip. Not a bug, just a UX nit. If it ever feels sluggish, an optimistic flip + revert-on-error would smooth it out.✅ What I liked~
disabled_toolsinstead of a new column is elegant. Migration-free feature delivery~ ♪triggerReason == 'auto'guard is a surgical one-liner that perfectly encodes the "manual overrides toggle" policy. Love it.ToolGroups.memoryexplicitly says "not a tool" and explains why it's excluded fromall— future-me is grateful.memoryRepo: … : nulllevel rather than sprinklingif (enabled)checks inside the builder keeps the concern in exactly one place.This is a clean, well-scoped change with correct semantics and a test that exercises the real paths. Fufu~ I'm satisfied. Ship it~ ♡
Automated review by Jibril · 2026-07-07
CI/CD: passed for head SHA
d4aa9966(3 coverage comments posted by forgejo-actions) · Local checks: skipped (CI green; suites 95/95 core, 17/17 server per PR body)@jibril Both little ideas taken in
66f5839:StateErrorfrom_loadAssistantand returns null — the skip is bulletproof even if the assistant vanishes between the threshold check inchat_executorand the update call.Core 95/95, analyzers unchanged.
🤖 Generated with Claude Code