fix(chat): surface unavailable memory at startup - #3153
Conversation
|
Verdict: Request changes This makes a chat session say out loud when memory failed to come up instead of only whispering it to a log file — the right fix for a real p1. But as written the message never reaches the two places the linked issue actually complains about. The warning is printed before anyone is listening. In the Agent UI and in the flagship's TUI sidecar, the agent is built in "quiet" mode and only later handed the channel that carries text to the user. The new warning fires during construction, into the quiet channel, and is discarded. Only the interactive terminal No evidence that a user sees the warning. The PR shows a passing unit test on the helper function, which proves the helper calls the console — not that a person with a missing embedding model gets told. A short paste of a real Real-world evidenceN/A — no evidence bundle was produced for this run, and the PR description lists only unit tests plus formatting checks. The verdict rests on static review of the startup ordering; nothing here demonstrates the warning reaching a user on any surface. 🔍 Technical details🔴 The warning is swallowed on every non-terminal surface (
Net effect: only Suggested shape: record the pending message in self._pending_memory_warning = (
self.memory_unavailable_message()
if getattr(self, "_memory_unavailable_reason", None) in (...)
else None
)then in 🟡 No real-surface evidence for a user-visible change Per the rubric, a change to what a chat session prints wants the real 🟢 Duplicates the email agent's block instead of sharing it (
🟢 Test covers only the reporting branch ( The skip path is the part with the actual policy in it ( Strengths
|
|
Addressed the review feedback in 56edcd0. MemoryMixin now owns one-shot reporting, ChatAgent defers the warning until the first user-visible turn (after TUI/SSE console injection), and EmailAgent reuses the shared path. Tests now cover one-shot behavior plus explicit GAIA_MEMORY_DISABLED/healthy quiet paths. |
itomek
left a comment
There was a problem hiding this comment.
Approved this earlier, then went back and traced the turn hook — the email agent, which is the agent #2831 was actually filed against, still never shows this warning to anyone.
Moving the message off construction and onto the first real turn is the right call, and it works for chat and the flagship. But the email agent's own process_query hands off to a base implementation that stops there, so the new turn-start report is unreachable code for it. Its only remaining report is the one at construction time — the exact path the earlier review asked to move away from. On the email surface the behaviour is unchanged from before this PR: warning in the log, nothing on screen.
Second one: the report marks itself "already shown" even when it printed into a console that discards output. A session whose first turn goes through the non-streaming path spends the single shot on nothing, and every later turn in that session stays quiet.
Both are small — roughly fifteen lines. The test matters as much as the fix: the current one exercises the helper against a hand-written stub, so it passes unchanged with both of these present. There are already 28 email tests that build a real agent, so driving one through a turn and asserting the warning fires is cheap, and it's what would have caught this.
Last thing — I'd drop Fixes #2831 to Refs #2831 for now. As it stands a merge auto-closes a p1 whose "covers every MemoryMixin consumer" criterion this doesn't yet meet, plus two it doesn't touch (whether running without memory is permitted at all, and whether to accept an available embedding model rather than failing on an exact-id miss). Happy for those to be follow-ups, just not silently closed.
I'll get you a real run against a Lemonade with the embedding model unpulled — that evidence is easier to produce on my side, and it's the check that would have surfaced this.
🔍 Technical details
Email agent: the deferred report is unreachable
process_query defined in, in MRO order: ['EmailTriageAgent', 'Agent', 'MemoryMixin']
EmailTriageAgent -> calls super().process_query: True
Agent -> calls super().process_query: False <- chain stops here
MemoryMixin -> calls super().process_query: True <- never reached
EmailTriageAgent(Agent, MemoryMixin, ...) puts Agent ahead of MemoryMixin, and Agent.process_query (src/gaia/agents/base/agent.py:4426) calls _process_query_impl without delegating further. Chat and the flagship both resolve process_query to MemoryMixin.process_query, so they're fine.
No need to reorder the bases — just call it directly in hub/agents/email/python/gaia_agent_email/agent.py:1221:
def process_query(self, user_input: str, *args, **kwargs):
# UI/sidecar callers swap in the real console after __init__ —
# report through whatever handler is attached now.
self.report_memory_unavailable()
self._reset_organize_counter()One-shot flag spent on a console that renders nothing
SilentConsole.print_warningis a no-op (src/gaia/agents/base/console.py:2701)- agents are built with
silent_mode = not streaming(src/gaia/ui/_chat_helpers.py:494) - the non-streaming
process_queryat:1607has noagent.console =assignment before it (injections are at:1852,:1935,:2081) - both paths share one
_agent_cache(lookups at:1427non-streaming,:1847streaming)
So: first turn non-streaming → flag set against a no-op console → every later streaming turn on that cached agent is silent. In report_memory_unavailable:
target = console if console is not None else getattr(self, "console", None)
if target is None or isinstance(target, SilentConsole):
return Falseconsole.py doesn't import memory, so that import is cycle-free.
Minor — report_memory_unavailable(console=None): no caller passes it. Drop the parameter or use it from the email agent.
Follow-up, not a blocker — in the Agent UI the warning arrives as a status event, and ChatView.tsx:995 consolidates consecutive status steps by overwriting the previous one's label and detail. So it flickers in the steps panel rather than persisting as the degraded-state indicator #2831 asks for. Read from the code, not observed live.
Worth keeping
- Chat and flagship path is correct and verified —
SSEOutputHandler.print_warningemits a realstatus/warningevent (src/gaia/ui/sse_handler.py:502), so the injected handler does carry it. - Excluding
GAIA_MEMORY_DISABLEDfrom the warning is right, and easy to get wrong. - Consolidating the policy into
MemoryMixininstead of a second copy in the email agent is the correct home.
…eal console EmailTriageAgent's MRO puts Agent ahead of MemoryMixin, so Agent.process_query never delegates into MemoryMixin.process_query the way ChatAgent does — the turn-start notice added for the degraded-memory warning was unreachable code for the email agent. It now reports directly from EmailTriageAgent.process_query, so a UI/sidecar caller that swaps in the real console after construction (agent.console = ...) still gets the notice on the next turn. Separately, report_memory_unavailable() was spending its one-shot flag on a SilentConsole, whose print_warning is a no-op — the UI's non-streaming path builds agents silent, and the same cached agent later serves streaming turns with a real console that would never see the notice. It now skips (and does not consume) the one-shot report when the current console is silent. Also dropped the unused console= parameter, which no caller ever passed.
|
Pushed the two fixes from my last review straight to your branch rather than bounce it back — you'd already turned this around once, and it seemed silly to hold it another cycle over fifteen lines. Nothing needed from you. Shout if you'd rather have done it yourself and I'll know for next time. The email agent now reports the degraded-memory notice from its own turn entry point instead of relying on the shared one it never actually reaches, and reporting into a console that renders nothing no longer burns the one-shot flag. I also added two tests that build a real email agent and drive it through a turn — both fail on your previous head and pass now, which is the coverage the earlier stub-based test couldn't give. Also dropped Still on me: the real run against a Lemonade with the embedding model unpulled. Once I post that, this is good to go. 🔍 Technical detailsPushed as 1. Email agent — the deferred report was unreachable
2. One-shot flag spent on a no-op console
3. Dropped the unused Verification
I deliberately left Still open, not blocking this PR — in the Agent UI the warning arrives as a status event and |
ChatAgent currently logs embedding initialization failures but does not show them in the chat UI, so every MemoryMixin-based chat session can silently lose memory. This change reports model-not-pulled and service-unreachable states through the existing console warning channel after the console is initialized, while preserving the explicit GAIA_MEMORY_DISABLED opt-out. Added a deterministic unit test for the warning boundary.
Tested: PYTHONPATH=src;hub/agents/chat/python python -m pytest hub/agents/chat/python/tests/test_chat_agent.py -q (8 passed); python -m black --check on changed files; git diff --check.
Refs #2831