Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
| r"""Update memory only after summarization succeeds.""" | ||
| summary = summary_result.get("summary", "") | ||
| if ( | ||
| summary_result.get("status") != "success" |
There was a problem hiding this comment.
I ran this against ddd7c82 in a clean container (python:3.11-slim, pip install -e ., mcp<2.0.0).
The status conjunct here is doing more than #4321 asks for. Parsing summarize and asummarize, result["summary"] is assigned in exactly one place in each: the result.update(result_dict) at chat_agent.py:2044 and :2495, on the success path. Every other return leaves it at the initial "" and sets only status. So a non-empty summary already means generation succeeded, and this conjunct rejects one further case: the summary is good and only save_markdown_file failed. That is summary_result1 in the parametrize list.
In that case memory is unchanged, so the threshold is still exceeded and the summarizer runs again on the next context build. Same fixture on both arms, 10 turns, token_limit=1000, summarize_threshold=50, summarize() stubbed to return {"summary": "a perfectly good summary", "status": "Error: [Errno 30] Read-only file system"}:
master 8c791b7 summarize() calls over 5 context builds: 1
memory: 21 messages / 630 tokens -> 3 messages / 90 tokens
head ddd7c82 summarize() calls over 5 context builds: 5
memory: 21 messages / 630 tokens -> 21 messages / 630 tokens
summarize() is a model call, so while the directory stays unwritable that is one extra call per step, with a log warning as the only signal.
Dropping the status conjunct and keeping the text check gives 1 call and the compression back, and 3 of the 4 new tests still pass. It flips summary_result1, which is why I am raising it rather than suggesting a diff: the summary lives in memory, not in the file, so a failed save need not also cost the rewrite. If you would rather keep a save failure fatal, capping the retries would cover the same ground.
Not checked: the async path beyond reading it, and the repo's full test suite.
There was a problem hiding this comment.
Addressed in e0ddfbc. You are right: the returned status also reflects Markdown persistence, so it should not gate an in-memory rewrite when valid summary text is available. I removed the status conjunct and retained the non-empty string check.
The save-error case now verifies that compression succeeds. Added coverage for both sync and async context construction, with progressive and full compression: five consecutive context builds retain the summary and latest user message, and invoke the summarizer exactly once. Generation failure and empty-summary cases still preserve the original memory.
Local validation: 7 targeted regression cases passed; the offline ChatAgent selection passed (17 passed, 28 deselected). Ruff check, Ruff format check, and git diff --check passed. The full model-backed suite was not run.
There was a problem hiding this comment.
Read e0ddfbc, and that is exactly the change I measured. I have not run the new tests.
2 participants
Related Issue
Closes #4321
Description
Automatic context compression previously rewrote chat memory even when the summarizer returned an error or an empty summary. This could clear the conversation history after a transient summarization failure.
This change validates the summarization result before committing the memory rewrite. Both synchronous and asynchronous compression paths now preserve and return the original context unless the result reports success and contains a non-empty summary.
Regression tests cover failed model generation, failed summary persistence, empty summary content, and the asynchronous failure path.
What is the purpose of this pull request?
Testing
uv run pytest test/agents/test_chat_agent.py -k "automatic_summarization_preserves_memory" -q— 4 passeduv run pytest test/agents/test_chat_agent.py -m "not model_backend and not heavy_dependency and not very_slow" -q— 14 passeduv run ruff check camel/agents/chat_agent.py test/agents/test_chat_agent.py— passeduv run ruff format --check camel/agents/chat_agent.py test/agents/test_chat_agent.py— passedChecklist