fix(compression): skip session split on plugin no-op - #58495
Conversation
fb415de to
c23ab9b
Compare
c23ab9b to
e39cd29
Compare
e39cd29 to
49f0d6b
Compare
|
Rebased onto current Validation: Current head: |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The current-main premise is real: after compress() returns, agent/conversation_compression.py:769-781 only recognizes an identity no-op, while a newly allocated unchanged list can proceed into session-boundary handling at agent/conversation_compression.py:819 and the context-engine boundary hook at :987-1001.
Problems
tests/run_agent/test_compression_boundary_hook.py:183returns the exactmessagesobject. Current main already skips that path atagent/conversation_compression.py:772, so this test passes without the newlast_compression_status == "noop"branch.- The compatibility fallback to
_last_compression_statusin the PR diff is not covered.
Suggested changes
- Return
list(messages)from the mock and assert it is a distinct object; this directly proves the new status branch prevents rotation. - Add a private-field fallback case, and document the optional status contract alongside the ContextEngine interface in
agent/context_engine.pyand the context-engine plugin guide.
Automated hermes-sweeper review.
|
Strengthened the no-op regression test so it no longer passes through current Validation: |
6668e68 to
874b378
Compare
|
Rebased onto current Note on the rebase conflict: |
874b378 to
8237ceb
Compare
|
Review accepted — both points applied (commit @teknium1 the hermes-sweeper review was sound, and the AGENTS.md caching rubric (no session split / no cache-invalidating rewrite when a plugin reports a no-op) backs it. Applied:
All 31 related tests pass ( Upstream check: the Rebase note: |
DEEP upstream-already-fixed analysis — verdict: STILL_OPEN (not fixed elsewhere)I read current What
|
| # | Gate (upstream/main) | Line | Catches a plugin no-op that returns list(messages)? |
|---|---|---|---|
| 1 | if getattr(agent.context_compressor, "_last_compress_aborted", False) |
938 | ❌ Only set on aux-LLM summary failure, not a clean plugin no-op |
| 2 | if compressed is messages: (identity check) |
958 | ❌ A plugin returning a new equal list (list(messages)) is not the same object → fails |
| 3 | if not compressed: |
969 | ❌ No-op returns a non-empty equal list, not an empty one |
If a plugin's compress() reports "nothing changed" by returning a freshly-built equal list (as hermes-lcm-style engines plausibly do, and exactly what this PR's regression test mocks at test_compression_boundary_hook.py:189 via compressor.compress.return_value = list(messages)), none of the three gates fire, so execution falls through to the rotation block (lines 1066+) and agent.session_id is rotated / session split — the churn this PR targets.
Signals that do NOT already cover the gap
_last_compression_made_progressis captured (lines 926-928) but is used only to arm the effectiveness verdict/deferral (line 1297) — it does not gate rotation. So "made no progress" does not by itself skip the split.- No
len(compressed) == len(messages)equality gate exists anywhere in the rotation path (only referenced in docstrings about the caller's retry loop, lines 623/710/937). - No
last_compression_status/_last_compression_status/did_compress/compression_result/no_changesignal exists anywhere onupstream/main(git grep: 0 occurrences). - The plugin contract (
agent/context_engine.py) has no no-op sentinel incompress()'s return — it just returnsList[Dict]. Built-inContextCompressordodges the bug only because it returns the samemessagesobject on a no-window case (context_compressor.py:3338etc.), which the identity gate at line 958 catches. A third-party plugin that builds a new list is not afforded that protection.
Conclusion
This is not already fixed by another mechanism. The PR correctly adds the missing last_compression_status == "noop" gate (public attribute preferred, _last_compression_status private fallback) ahead of the identity gate, plus documents the contract on ContextEngine. Leaving open for the maintainer — the change is in-scope and the gap is real.
(Filed from the delegated deep-analysis pass; no code touched on main, inspected in an isolated worktree.)
8237ceb to
e6c4e17
Compare
Thanks for the focused fix — I traced it against Two small notes, neither blocking:
One actionable item: |
e6c4e17 to
725b102
Compare
d1df7ba to
6492730
Compare
|
Closing as absorbed by upstream. The no-op compression guard this PR added is now present directly in |
Port the explicit plugin no-op session-boundary guard onto current upstream lifecycle seams and add a regression for the residual case not covered by semantic equality: cleanup-changed active context reported as noop must be adopted without rotating the session. Prior art: NousResearch#58495. Equal-copy no-progress handling remains upstream-owned by NousResearch#67938.
Summary
last_compression_status == "noop"as a non-boundary result in_compress_context(falling back to the legacy private field for compatibility)Why
LCM can be above the host token threshold while having no eligible leaf backlog outside its protected fresh tail. In that case it returns a no-op. Previously the host still proceeded through the successful-compression session split path, creating continuation sessions without summary/DAG state.
Tests
python -m py_compile agent/conversation_compression.py tests/run_agent/test_compression_boundary_hook.py scripts/release.pypython -m pytest tests/run_agent/test_compression_boundary_hook.py -qpython -m pytest tests/run_agent/test_infinite_compaction_loop.py tests/run_agent/test_compression_persistence.py -q