Fix main-branch context compression baseline regressions - #211
Conversation
Restore the sync context-compression API expected by the current test suite and model assembly path. This reintroduces dare_framework.compression.core, restores Context.compress() semantics, preserves retrieval-fusion metadata assembly, and keeps the moving-compressor path available through assemble_for_model(). Also fix the concurrent transport-loop contract test to use canonical Message inputs so the background task reaches the task-local event barrier instead of failing early.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb8dd5518f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key = (msg.role, msg.text) | ||
| digest = hash(key) | ||
| if digest in seen: |
There was a problem hiding this comment.
This dedup key only uses role and text, so distinct messages that share text are collapsed even when their semantic payload differs; in practice, multiple assistant tool_call messages often have empty text, which means later tool calls are dropped and tool_pair_safe will also remove their matching tool results. That loses valid execution history even when no truncation is needed (max_messages can still be larger than the current history), which can materially degrade subsequent model behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 56f6e57.
_dedup_messages() now keys on the full public message shape (role, kind, text, name, attachments, data) instead of just (role, text), so distinct assistant tool-call messages with empty text no longer collapse into one entry. Added a regression in tests/unit/test_context_compression.py that keeps both tool-call/tool-result pairs intact under dedup_then_truncate with tool_pair_safe=True.
| if self._maybe_auto_compress(messages): | ||
| assembled = await self._context.assemble_for_model() | ||
| messages = list(assembled.messages) |
There was a problem hiding this comment.
In the SmartContext path, the transient reflection prompt is appended and then cleared before this branch runs; when auto-compression triggers, you rebuild messages from assemble_for_model() here but never re-append the saved injected_reflection_prompt. As a result, the per-round manage-context guidance silently disappears whenever compression runs, changing agent control flow (e.g., wasting rounds or failing to enforce intended first-step guidance under tighter max_tool_rounds).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 56f6e57.
When SmartContext auto-compression rebuilds the assembled messages, the saved one-shot reflection prompt is now appended again before model invocation. Added a regression in tests/unit/test_react_agent_gateway_injection.py that forces compression and asserts the manage-context guidance is still present in the model input.
Preserve structured tool-call payloads during deduplication by incorporating message kind, name, attachments, and data into the compression identity key, so auto-compression no longer drops distinct assistant tool-call history with empty text. Also re-append the transient SmartContext reflection prompt after re-assembling messages for auto-compression, and add regression coverage for both review findings.
|
Addressed the Codex review findings in Handled:
Verification:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56f6e572ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if callable(compress_impl) and not has_advanced_options: | ||
| compress_impl(max_messages=max_messages) | ||
| return |
There was a problem hiding this comment.
This fast path returns after calling backend compress_impl(max_messages=max_messages), which breaks Context.compress(max_messages=0) for the default InMemorySTM: its compress implementation slices with self._messages[-max_messages:], and when max_messages is 0 that keeps the entire list instead of clearing it. Because this branch is now the canonical basic-compression route, callers who explicitly request full STM eviction will silently retain prior history and leak stale context into later model calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in a6aff31.
The default InMemorySTM.compress() now treats max_messages=0 as full eviction instead of relying on [-0:], and tests/unit/test_context_implementation.py now covers Context.compress(max_messages=0) against the default STM so the canonical basic-compression path cannot silently retain stale history.
Make the default in-memory STM treat max_messages=0 as full eviction instead of relying on Python's -0 slice behavior, which previously retained the entire history. Add a Context-level regression test so the canonical basic compression path guarantees explicit zero-message requests clear the default STM.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6aff312ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value.filename, | ||
| _freeze_value(getattr(value, "metadata", {})), | ||
| ) | ||
| return value |
There was a problem hiding this comment.
_dedup_messages builds a set key from _freeze_value(msg.data), but _freeze_value falls back to returning unknown objects unchanged. If a tool result includes non-JSON nested values (for example a set or custom object inside Message.data), if key in seen raises TypeError: unhashable type, which aborts compression and can fail agent execution when auto-compression is enabled. Please make _freeze_value always return a hashable representation (or safely bypass dedup for unfreezable payloads).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 73b04b2.
_freeze_value() now normalizes sets/frozensets, dataclass instances, and other unhashable objects before they participate in the dedup key, so dedup_then_truncate cannot raise TypeError on non-JSON-native payload values. Added a regression in tests/unit/test_context_compression.py that deduplicates messages containing set payloads.
| remaining_tokens = context.budget_remaining("tokens") | ||
| stm_token_estimate = self._estimate_tokens(messages) | ||
| retrieval_budget: float = float("inf") | ||
| if remaining_tokens != float("inf"): | ||
| retrieval_budget = max(0.0, float(remaining_tokens) - float(stm_token_estimate) - float(reserve_tokens)) |
There was a problem hiding this comment.
In DefaultAssembledContext.assemble, retrieval budgeting is only applied when remaining_tokens != float("inf"). With a zero token cap (Budget(max_tokens=0)), budget_remaining("tokens") currently evaluates to inf, so this branch is skipped and retrieval_budget stays infinite, allowing LTM/knowledge fetches despite an exhausted token budget. This weakens the new budget guardrails and can inject unexpected retrieval messages under strict-budget runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 73b04b2.
The root cause was budget_remaining() treating zero-valued limits as falsy and returning inf. It now treats 0 as a finite cap for all tracked resources, and tests/unit/test_context_implementation.py now covers Budget(max_tokens=0) to verify retrieval degrades with token_budget_low and skips LTM/knowledge fetches.
Make budget_remaining treat zero-valued limits as finite instead of falling back to infinity, so strict zero-token runs correctly degrade retrieval instead of fetching LTM/knowledge under an exhausted budget. Also make compression dedup keys robust for unhashable payload values, and add regressions for both the zero-budget retrieval path and non-JSON-native payload deduplication.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b04b28ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self._maybe_auto_compress(messages): | ||
| assembled = await self._context.assemble_for_model() |
There was a problem hiding this comment.
When auto_compress is enabled and the token trigger fires, this branch calls assemble_for_model() a second time in the same round; because Context.assemble_for_model() always runs moving_compressor.prune(...) when a moving compressor is attached, the same request can be pruned twice before one model call. In configurations that enable both moving compression and auto compression, this can over-compress STM (or trigger duplicate expensive summary work) and change model inputs unexpectedly.
Useful? React with 👍 / 👎.
| if max_messages == 0: | ||
| removed_total += len(messages) | ||
| messages = [] |
There was a problem hiding this comment.
The max_messages == 0 branch unconditionally clears all messages, which drops IMMUTABLE/PERSISTENT anchors before backend retention semantics can run. In the advanced path (strategy/tool_pair_safe/target_tokens present), callers using Smart STM semantics can lose protected context such as task/core guardrails by passing max_messages=0, even though backend compression would normally retain those protected entries.
Useful? React with 👍 / 👎.
Drop the production-side compression and auto-compress changes from PR 211 and keep the branch focused on correcting outdated expectations in tests and examples. Key changes: - remove the framework-level compression.core contract from unit coverage - rewrite Context tests around the current default assemble path and moving-compressor behavior - update ReactAgent tests to use the existing Message-level execute contract and remove auto_compress expectations - refresh the AgentScope compat example docs/comments to describe moving compression as the current framework behavior Rationale: The module owner confirmed the failing assumptions were in UT/example coverage rather than in mainline runtime behavior, so this PR should only realign tests and example documentation with the current implementation.
|
Scope of This update removes the production-side
Verification on commit
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
dare_framework.compression.coreand preserve retrieval-fusion metadata assemblyMessageinputsVerification
.venv/bin/pytest -q.venv/bin/pytest -q tests/unit/test_context_compression.py tests/unit/test_context_implementation.py.venv/bin/pytest -q tests/unit/test_base_agent_transport_contract.py::test_transport_loop_flag_is_task_local_for_concurrent_execute_calls