fix(native_compaction): preserve compression summary messages during pre-checkpoint pruning - #90976
Conversation
…pre-checkpoint pruning
…rns to is_summary
…nd sequence ordering (NousResearch#90976) - Add dedicated RETAINED_SUMMARY_TOKEN_BUDGET (32,000 tokens) with head-truncation fallback - Implement robust multi-pattern _is_summary_item helper inspecting top-level flags, nested metadata dicts, and string headers - Implement safe _extract_item_text for string, multipart lists, output_text, and nested text - Preserve original relative chronological order between retained user messages and summaries - Add logger audit metrics and enable_summary_retention configuration toggle - Add comprehensive test matrix covering multipart inputs, budget limits, malformed payloads, and toggle controls
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review — do not merge current head d85dffa0db84aabba76b6daa69e04b6674547eeb.
The bug is real, but this patch replaces missing retention with an untyped persistence channel across the native compaction boundary.
Hermes already has an authoritative summary producer contract in agent.context_compressor: COMPRESSED_SUMMARY_METADATA_KEY, ContextCompressor.classify_summary_content(...), and is_compaction_summary_message(...), with live-emission agreement tests. _is_summary_item() ignores that contract and independently classifies content by broad heuristics:
- any top-level underscore-prefixed key containing
summary, regardless of its value; - any metadata key containing
summaryorcompression, also regardless of value; - arbitrary message text containing phrases such as
conversation summary,[summary], or## summary.
That means a normal user/assistant message, a false-valued marker such as {"_summary_requested": false}, or untrusted content containing a summary heading can be promoted to durable retained history. A user message matching the heuristic is also charged to the separate summary budget instead of the user budget. This is authority drift and can preserve stale or adversarial instructions across a checkpoint.
There is a second structural problem at the budget boundary: generic head truncation of a compressed-summary message is not shape-safe. Hermes summaries can be standalone or merged into a preserved tail with explicit delimiters. Slicing content[:remaining * 4] can retain the old tail while cutting off the actual summary, or break the framing/closing marker that keeps historical text non-active.
Required before merge:
- Reuse the canonical summary predicate/metadata constant rather than adding a second heuristic detector. Exact metadata provenance should win; content fallback should be only the canonical persisted-row classifier already maintained by the compressor.
- Require truthy exact markers—never infer authority from a key name alone, and never classify arbitrary headings in ordinary content.
- Preserve canonical summary shapes whole, or add a structure-aware extraction/truncation path that understands standalone versus merged emissions. Do not byte/character-slice the opaque message envelope.
- Add negative witnesses for
## Summaryin ordinary user text, false-valued summary/compression metadata, arbitrary underscore keys, and non-Hermes assistant content. - Add live-compressor emission tests for both standalone and merged summaries at the retention boundary, plus repeated-checkpoint/idempotency coverage. The PR body claims deduplication, but the implementation currently retains every matching pre-checkpoint item.
- Either wire
enable_summary_retentionto a real configuration surface or remove the claim that this adds a configuration toggle; at present it is only a function parameter.
Exact-head CI/Nix/Docker are green, but the tests encode the permissive detector as expected behavior and therefore do not prove summary provenance or safe retention.
Delegates summary detection to the canonical agent.context_compressor.is_compaction_summary_message provenance check instead of an ad-hoc key/content heuristic, adds negative-witness coverage so lookalike content is never promoted to retained history, switches oversized summaries to whole-or-drop instead of byte-slicing (which could corrupt structural framing), makes retention idempotent across repeated checkpoints, adds tests against live ContextCompressor emissions, and corrects the enable_summary_retention docstring to stop overclaiming it as a wired user-facing config toggle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all items from the blocking review:
Validated: |
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking re-review — do not merge exact head bf8703c262fa6c945e9ab6d40ff893c4f327715d.
The canonical predicate, truthy provenance, whole-or-drop handling, negative witnesses, and dedup/idempotency blockers from the first review are resolved. Exact-head CI, Nix, and Docker are green.
One blocking runtime path remains: summary provenance is checked only after _chat_messages_to_responses_input() has already erased or displaced it.
ContextCompressor.compress() explicitly allows merge-into-tail to use the first protected tail row when that row is template-exempt; its own comment names a bare assistant tool-call row or a tool result as the ideal carrier. It rewrites that chat message's content and stamps COMPRESSED_SUMMARY_METADATA_KEY.
Production then converts the chat transcript before prune_pre_checkpoint_items() runs:
- A merged tool-result carrier becomes
{"type": "function_call_output", "output": ...}. The private summary marker is not forwarded,_extract_item_text()does not readoutput, and the pruner skips every typed non-messageitem before calling_is_summary_item(). That canonical summary is still dropped at the checkpoint. - A merged assistant carrier with
codex_message_itemscan also lose the rewrite. The adapter replays the exact stored message item and, oncereplayed_message_items > 0, deliberately does not emit the assistant row's rewrittencontent.drop_stale_api_content()clears onlyapi_content, and the compressor's stale-replay pass strips onlycodex_reasoning_items, so the pre-merge exact-message sidecar can shadow the summary before the pruner sees it.
The new tests do not cross this boundary. test_standalone_live_marker_is_retained calls only _render_micro_marker_content; test_merged_tail_summary_is_retained_and_classified_merged manually assembles private delimiter constants and passes a synthetic role="assistant" dict directly to the pruner. The linked reply's claim that both shapes are live compressor emissions is therefore incorrect, and neither test exercises the Responses adapter where the loss occurs.
Required before merge:
- Add end-to-end witnesses using a real
ContextCompressor.compress()emission, then feed its output through_chat_messages_to_responses_input(..., native_compaction_eligible=True)with a replayed checkpoint. Cover standalone plus merge-into-tail on at least a tool-result carrier and an assistant carrier carryingcodex_message_items. - Preserve canonical summary provenance/content before the lossy role/tool conversion, or map a canonical carrier into a dedicated valid Responses message before pruning. Do not fix this by heuristically treating arbitrary
function_call_output.outputas summaries. - Assert the fix emits neither an orphaned
function_call_outputnor stalecodex_message_items, and that the canonical summary appears after the newest checkpoint exactly once. - Update the PR body and the adapter's user-only pruning comment. The body still documents the removed broad detector, summary head-slicing, and a user-facing toggle that does not exist.
No need to revisit the first review's items 1–4 or the text-dedup work; those are fixed. This remaining blocker is the actual producer → adapter → pruner contract.
…ousResearch#90976) The pruner only ever saw whatever survived _chat_messages_to_responses_input's conversion. Two merge-into-tail carrier shapes lose the summary before pruning ever runs: a tool-result carrier becomes a typed function_call_output (no content/role survives), and an assistant carrier can be shadowed by a stale codex_message_items exact-replay captured before the merge rewrote its content. Thread the original chat message alongside each converted item (item_sources) so prune_pre_checkpoint_items can read a canonical summary carrier's up-to-date content straight from its source instead of trying to recover it from whatever shape conversion produced.
|
Addressed the remaining blocking item (runtime path: summary provenance checked after
Backward compatible: Validated: |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review at exact head 99937f26c7c8f923c8acf620e19731b58e2e5de0.
The runtime repair itself now closes the conversion-order defect from my prior review: item_sources is carried in parallel with every emitted Responses item; canonical provenance is checked on the raw source before lossy tool/replay conversion can erase it; a qualifying source is synthesized back as one assistant message; the pre-checkpoint typed call/output material is not retained, so this does not leave an orphaned function_call_output; and the stale codex_message_items replay is displaced by the current source content. The PR body/commentary now describes that actual path. Exact-head CI 32430608964, Docker 32430608304, and Nix 32430608294 are all green.
One proof blocker from the prior review is still not satisfied, though. I asked for end-to-end witnesses that use a real ContextCompressor.compress() emission and then feed that mutated transcript through _chat_messages_to_responses_input(..., native_compaction_eligible=True). The two new adapter tests still manufacture the producer state themselves with _merged_summary_content(...) plus COMPRESSED_SUMMARY_METADATA_KEY = True. Their own class docstring says they are merely “shaped exactly the way” the compressor produces them. That proves the adapter fix for the assumed shape, but it does not prove the producer→adapter contract or catch future/current drift in the compressor's actual carrier selection/mutation semantics.
Please replace or supplement those fixtures with producer-backed witnesses: drive ContextCompressor.compress() far enough to produce (a) the tool-result merge carrier and (b) the assistant carrier with the replay sidecar, then pass those actual resulting messages through the adapter and retain the existing assertions: newest checkpoint first, summary exactly once, no stale replay text, and no orphaned typed output. A standalone real-compressor case is useful too, but the two lossy merge carriers are the load-bearing proof.
Disposition: implementation looks correct; merge remains blocked only on the missing producer-backed witness. No need to revisit the already-closed provenance, budget, dedup, or documentation items.
|
I came at #90975 independently, reproduced it on Worth saying up front: 1. A
|
Of course, you can do this. When I get home, I'll save your commit to my PR. And if I can't do it in time before the maintainers do, they will. I appreciate this feedback again, which I wasn't able to see. |
…pre-checkpoint pruning prune_pre_checkpoint_items() had a hardcoded role=='user' filter that discarded all non-user messages before a checkpoint — including Hermes' own compression summaries (role='assistant'), causing total context amnesia about past conversation summaries. The fix: - _is_summary_item delegates to the canonical agent.context_compressor.is_compaction_summary_message provenance check (not an ad-hoc heuristic) - Summaries are retained whole (never byte-sliced) within a 32k token budget - Idempotent across repeated checkpoints (dedup by identical text) - _chat_messages_to_responses_input threads item_sources (raw chat messages) through to the pruner, so it can read summary content directly from the source when the Responses conversion shape is lossy (tool-result carrier becomes function_call_output, or stale codex_message_items replay shadows merged content) Fixes NousResearch#90975. Salvage of NousResearch#90976 by @JoaoMarcos44.
|
Merged via #91477 — your commits applied with authorship preserved (rebase-merge). Thank you for this fix! The approach is exactly right: canonical provenance check via Follow-up cleanups we applied on top during /simplify-code review:
|
…pre-checkpoint pruning prune_pre_checkpoint_items() had a hardcoded role=='user' filter that discarded all non-user messages before a checkpoint — including Hermes' own compression summaries (role='assistant'), causing total context amnesia about past conversation summaries. The fix: - _is_summary_item delegates to the canonical agent.context_compressor.is_compaction_summary_message provenance check (not an ad-hoc heuristic) - Summaries are retained whole (never byte-sliced) within a 32k token budget - Idempotent across repeated checkpoints (dedup by identical text) - _chat_messages_to_responses_input threads item_sources (raw chat messages) through to the pruner, so it can read summary content directly from the source when the Responses conversion shape is lossy (tool-result carrier becomes function_call_output, or stale codex_message_items replay shadows merged content) Fixes NousResearch#90975. Salvage of NousResearch#90976 by @JoaoMarcos44.
…pre-checkpoint pruning prune_pre_checkpoint_items() had a hardcoded role=='user' filter that discarded all non-user messages before a checkpoint — including Hermes' own compression summaries (role='assistant'), causing total context amnesia about past conversation summaries. The fix: - _is_summary_item delegates to the canonical agent.context_compressor.is_compaction_summary_message provenance check (not an ad-hoc heuristic) - Summaries are retained whole (never byte-sliced) within a 32k token budget - Idempotent across repeated checkpoints (dedup by identical text) - _chat_messages_to_responses_input threads item_sources (raw chat messages) through to the pruner, so it can read summary content directly from the source when the Responses conversion shape is lossy (tool-result carrier becomes function_call_output, or stale codex_message_items replay shadows merged content) Fixes #90975. Salvage of #90976 by @JoaoMarcos44.
Preserve valid normalized input_image user messages across native-compaction checkpoints at bounded one-token retention cost. Keep text extraction text-only, reject malformed or unknown multipart placeholders, and prove the production adapter path without claiming unsupported input_file behavior. Refs NousResearch#90976 and NousResearch#91477.
Preserve valid normalized input_image user messages across native-compaction checkpoints at bounded one-token retention cost. Keep text extraction text-only, reject malformed or unknown multipart placeholders, and prove the production adapter path without claiming unsupported input_file behavior. Refs NousResearch#90976 and NousResearch#91477.
Preserve valid normalized input_image user messages across native-compaction checkpoints at bounded one-token retention cost. Keep text extraction text-only, reject malformed or unknown multipart placeholders, and prove the production adapter path without claiming unsupported input_file behavior. Republish the identical source tree after an unrelated nondeterministic focus-redraw test failure; this commit contains no source delta from the previously verified object. Refs NousResearch#90976 and NousResearch#91477.
What does this PR do?
Preserves local-compression summary messages across native OpenAI Responses compaction's pre-checkpoint pruning (
agent/native_compaction.py), so a compression handoff never silently vanishes from the model's view once a server-side checkpoint has been replayed.Fixes #90975.
Technical details
1. Canonical summary detection, not a heuristic
_is_summary_itemdelegates entirely toagent.context_compressor.is_compaction_summary_message— the same provenance check every other summary consumer (memory providers, frontends, the compactor itself) already uses. It prefers the exactCOMPRESSED_SUMMARY_METADATA_KEYflag and falls back to the canonical prefix classifier (handles the merge-into-tail shape too) for the case where the underscore-prefixed key was stripped by a wire sanitizer. No ad-hoc key scanning, no matching on content headings like"## Summary"in ordinary text.2. Whole-or-drop retention, never a byte slice
Retained summaries carry structural framing (handoff prefix, end marker, merge-into-tail delimiters) that a blind character slice can corrupt. A summary that doesn't fit
RETAINED_SUMMARY_TOKEN_BUDGET(32k tokens) is dropped whole instead of truncated.RETAINED_USER_MESSAGE_TOKEN_BUDGET(64k tokens, Codex CLI parity) keeps its existing head-truncation behavior for retained user messages only.3. Idempotent across repeated checkpoints
A summary already retained once (identical text) is never duplicated across repeated checkpoint sequences.
4. Source-based recovery for lossy carrier shapes
A canonical summary can be merged into an existing tail message rather than inserted standalone (
ContextCompressormerge-into-tail). By the time that message becomes a Responses input item, two real shapes lose or shadow the summary before pruning ever runs:function_call_output— nocontent/rolesurvives the conversion at all, and the pruner's type filter skips every non-messageitem;codex_message_itemsexact-replay captured before the merge rewrote its content — the replay path wins over the rewrittencontentfor prefix-cache continuity._chat_messages_to_responses_inputnow threadsitem_sources— the raw chat message each converted item came from — through toprune_pre_checkpoint_items. When a source is itself a canonical summary carrier, its content is read directly from the source (never from the lossy converted item) and retained as a synthesizedrole="assistant"message. This never orphans afunction_call_output/function_callpair: the original typed item is fully replaced, not retained alongside a dropped partner.5.
enable_summary_retentionFunction-level override for tests and callers that need pre-#90975 behavior back. Not wired to a user-facing config surface — there is no
agentreference at the actual call site (codex_responses_adapter.py) to wire real config without much broader plumbing.Test plan
tests/run_agent/test_native_compaction_summary_retention.py— canonical detection, negative witnesses (a"## Summary"heading in ordinary text, aFalse-valued flag, an arbitrary underscore key never misclassify), whole-or-drop truncation, idempotency, liveContextCompressormarker emissions, and two new adapter-level witnesses feeding real merge-into-tail carrier shapes (tool-result, assistant-with-stale-codex_message_items) through the real_chat_messages_to_responses_input(..., native_compaction_eligible=True)with a replayed checkpoint.tests/run_agent/test_native_compaction.py,tests/agent/test_compressed_summary_metadata.py,tests/agent/test_codex_responses_adapter.py,tests/run_agent/test_run_agent_codex_responses.py,tests/run_agent/test_provider_parity.py,tests/run_agent/test_codex_multimodal_tool_result.py— no regressions (117 passed).ruff checkclean on all changed files.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[Compressor Merge-Into-Tail] -->|Stamps COMPRESSED_SUMMARY_METADATA_KEY| B[Chat Message Source] B -->|role=tool| C[Lossy: function_call_output] B -->|role=assistant + stale codex_message_items| D[Lossy: Stale Exact Replay] B -->|item_sources mapping| E[Pruner Reads Source Directly] C -.->|old bug: type filter skips it| F[Summary Lost] D -.->|old bug: replay shadows rewrite| F E -->|Canonical Content, Whole-or-Drop| G[Synthesized Assistant Message] G --> H[Checkpoint Run + Retained + Post] H --> I[Wire: Summary Survives Exactly Once]Infographic: