fix(compression): prune stale codex_reasoning_items during compaction (#71058) - #71077
fix(compression): prune stale codex_reasoning_items during compaction (#71058)#71077webtecnica wants to merge 1 commit into
Conversation
|
Nice diagnosis and a genuinely additive shape — the 1. The prune boundary is the last assistant message, but a Codex turn is a multi-message chain — this can strip reasoning mid-turn.
This strips reasoning A and B while keeping Worth stressing: the two precedents the PR cites as making this safe both differ in the load-bearing way. Suggested boundary: prune older than the last turn / tool-call chain, not the last message — i.e. walk back from the end over the contiguous assistant/tool run to the last 2. No regression test is committed. The diff touches only Happy to be wrong on (1) if there's provider behavior I'm missing — if the Responses endpoint tolerates a function_call with a missing preceding reasoning item under |
8509051 to
2968a4c
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the replay-payload floor; current main still accounts for these fields without pruning them during compaction (agent/context_compressor.py:722-767, 4806-4854).
Problems
- The proposed
for i in range(last_asst_idx)boundary removes replay state from earlier assistant messages in a retained active tool-call chain. The Responses converter replays each assistant message's encrypted reasoning before that message's tool calls (agent/codex_responses_adapter.py:410-570), and Codex interim state is explicitly required to be replayed verbatim (agent/agent_runtime_helpers.py:555-561). - The PR changes only
agent/context_compressor.py; no regression coverage accompanies it. Existing tests cover replay-field budgeting (tests/agent/test_context_compressor.py:267-346) and single-message replay (tests/run_agent/test_provider_parity.py:884-902), not this chain boundary.
Suggested changes
- Preserve all assistant messages in the latest active assistant/tool chain; prune only before that chain.
- Add helper, compression, and adapter-output tests for that boundary.
Automated hermes-sweeper review.
| # No assistant message, or only one (nothing to prune). | ||
| return 0 | ||
|
|
||
| pruned = 0 |
There was a problem hiding this comment.
This treats every earlier assistant message as stale, but a retained Codex tool turn can contain assistant → tool → assistant rounds. The Responses converter replays encrypted reasoning before each assistant message's tool calls (agent/codex_responses_adapter.py:410-570), so retain the whole latest active chain rather than only the final assistant message.
SummaryOne open PR addresses Issue #71058 by pruning stale Related pull requests
Suggested consolidationkeep open with a salvage path: retain #71077’s post-compaction pruning approach, but preserve the entire latest active assistant/tool chain rather than only the final assistant message, and add helper, compression, and adapter-output regression tests. This follows the [PR review:COMMENTED] keep_open verdict and the blocking [contributor:7624 commits] review; do not merge or close #71077 until that replay-boundary correctness issue is explicitly addressed. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I71058(["issue #71058 (open)"])
P71077["PR #71077 (open)"]
P71077 -->|best fix| I71058
class I71058 open
class P71077 open
class P71077 best
class P71077 target
click I71058 "https://github.com/NousResearch/hermes-agent/issues/71058"
click P71077 "https://github.com/NousResearch/hermes-agent/pull/71077"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 6 kB of PR diffs, 5 kB of issue/PR text, 5 kB of discussion (3 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…heckpoints Two corrections on top of the #71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR #71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR #81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
…heckpoints Two corrections on top of the #71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR #71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR #81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
|
Landed on main via PR #81944 with your base commit cherry-picked intact — thank you @webtecnica, the diagnosis and the prune shape were exactly right. Two corrections rode on top: (1) the prune boundary is now the last USER message rather than the last assistant message, so a multi-message Codex tool-call chain in the active turn keeps its reasoning items (the gap @PRATHAMESH75 flagged in review); (2) type="compaction" items are exempt — those are native server-side compaction checkpoints (PR #81747, merged after this PR was opened) that carry already-pruned history and must survive on every retained message. A sibling site in the continuation-dedup path got the same checkpoint-preserving treatment. Closing this PR as superseded by the merged salvage. |
|
Thanks @teknium1 — thrilled to see it land! The last-USER-message boundary (keeping the active turn's multi-message Codex chain intact) and the compaction-checkpoint exemption are exactly the right corrections, and the sibling continuation-dedup treatment closes the class. Appreciate the cherry-picked authorship and the kind words on the diagnosis. |
…heckpoints Two corrections on top of the NousResearch#71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR NousResearch#71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR NousResearch#81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
…heckpoints Two corrections on top of the NousResearch#71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR NousResearch#71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR NousResearch#81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
…heckpoints Two corrections on top of the NousResearch#71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR NousResearch#71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR NousResearch#81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
…heckpoints Two corrections on top of the NousResearch#71077 base (the whole bug class): 1. Turn boundary = last USER message, not last assistant message. A Codex turn spans several assistant messages (assistant+tool_calls -> tool -> ... -> final assistant) whose reasoning items must replay together; the last-assistant boundary would strip reasoning mid-chain from the active turn (the gap flagged in PR NousResearch#71077 review). 2. type="compaction" checkpoints (native server-side compaction, PR NousResearch#81747) are exempt: they carry already-pruned history, not per-turn reasoning. Pruning filters items instead of popping the sidecar key. Sibling site fixed in the same class: the Codex incomplete-continuation dedup path blind-overwrote codex_reasoning_items on visually-duplicate interim messages, which would drop the only copy of a checkpoint captured on the earlier response. Extracted merge_interim_reasoning_items() into agent/native_compaction.py; newer reasoning wins, prior checkpoints are preserved unless the newer payload carries its own.
Summary
Fixes #71058. Compaction never pruned
codex_reasoning_items— encrypted reasoning blobs from Codex/Responses that ride on every retained assistant message. On long-lived sessions these blobs become the largest incompressible component (~36% of the payload), causing compaction to bottom out at ~2× the configuredtarget_ratioand re-fire every 30–60 minutes.Changes
agent/context_compressor.py— 3 additive changes, no behavioral change to non-Codex sessions:_prune_stale_reasoning_replay(messages)— New module-level function that scans the fully-assembled compacted message list right-to-left, locates the last assistant turn, and stripscodex_reasoning_items(and any future keys in_STALE_REPLAY_PRUNE_KEYS) from all assistant messages before it. The final assistant message retains its items because its replay chain may still be active._STALE_REPLAY_PRUNE_KEYS— New tuple alongside_REPLAY_BUDGET_KEYS, listing fields safe to strip from stale turns. Currently:codex_reasoning_itemsonly.Wired into
compress()— Called after_strip_persistence_markers(compressed), just before the return. Logs pruned count at INFO when non-zero and not in quiet mode.Safety
conversation_loop.pyalready drops these wholesale whenapi_mode != "codex_responses"— this is a scoped strip consistent with existing semantics.Testing
_prune_stale_reasoning_replaypass (empty list, single asst, multi-asst, no asst, three-asst cases).test_context_compressor.pytests pass (includingTestCompressfull integration tests).from agent.context_compressor import _prune_stale_reasoning_replay, _STALE_REPLAY_PRUNE_KEYSsucceeds.Closes #71058.