Skip to content

fix(agent): pass memory-provider on_pre_compress() insights to compression summary - #43567

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/pre-compress-insights-discarded
Closed

fix(agent): pass memory-provider on_pre_compress() insights to compression summary#43567
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/pre-compress-insights-discarded

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes MemoryProvider.on_pre_compress() return value being silently discarded in conversation_compression.py. Memory providers' pre-compression insights are now passed to the summarizer prompt so they survive context compaction.

Related Issue

Fixes #43558

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/conversation_compression.py: Capture return value of on_pre_compress() and store on compressor instance instead of discarding it
  • agent/context_compressor.py: Add provider_insights parameter to _generate_summary(), inject as "MEMORY PROVIDER INSIGHTS" section in the summarizer prompt; capture and clear insights at the start of compress() to prevent stale data on re-entry; propagate insights through retry paths
  • tests/agent/test_context_compressor.py: 7 new tests covering insights injection, empty/whitespace handling, iterative update, cleanup after compress, parameter forwarding, and retry survival

How to Test

  1. Configure a memory provider that returns non-empty text from on_pre_compress()
  2. Trigger context compression on a long conversation
  3. Verify the compression summary includes content from the provider insights section
  4. Run pytest tests/agent/test_context_compressor.py::TestProviderInsightsInCompression -v — all 7 tests should pass
  5. Run pytest tests/agent/test_context_compressor.py -v — all 101 tests should pass (no regressions)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/agent/test_context_compressor.py -v and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: agent/conversation_compression.py:compress_context() (caller), agent/context_compressor.py:_generate_summary() (receiver), agent/memory_manager.py:on_pre_compress() (interface)
  • Blast radius: LOW — only affects the compression summary prompt construction; no behavioral change when no memory provider is configured
  • Related patterns: focus_topic parameter follows the same injection pattern into the summarizer prompt

…ssion summary

The MemoryManager.on_pre_compress() method returns provider-extracted
insights that should survive context compaction, but the caller in
conversation_compression.py discarded the return value. This meant
memory providers' pre-compression observations were silently lost when
the compressor summarized old turns.

Now the return value is captured, stored on the compressor instance,
and injected into the summarizer prompt as a "MEMORY PROVIDER INSIGHTS"
section so the compressor preserves them in the summary.

Fixes NousResearch#43558
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/memory Memory tool and memory providers duplicate This issue or pull request already exists labels Jun 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #7195 (open) — same fix, same files (agent/context_compressor.py, agent/conversation_compression.py): capture the discarded MemoryProvider.on_pre_compress() return value and inject it into the summarizer prompt. #7195 is the earliest open PR for this. Targets the same underlying bug as #43558. Other competing open fixes: #11236, #22576, #29598.

@sam7894604

Copy link
Copy Markdown
Contributor

We hit this independently while building an out-of-tree MemoryProvider, and can corroborate the core claim: on the conversation_compression.py path, the value returned by on_pre_compress() is not threaded into the summary — a provider that returns non-empty text from the hook sees that text absent from the compaction summary.

One thing that may help unblock this: there appear to be two compression paths that behave differently for the same hook.

If both descriptions are accurate, the hook's return value is honored on one path and dropped on the other. That inconsistency — rather than a single missing wire — may be the underlying issue, and reconciling the two paths so the hook behaves the same regardless of which compression entry point runs seems like the thing to settle. Happy to share a minimal repro if it helps.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

This PR (#43567) is indeed addressing the same issue as #7195, but the implementations are different in technical approach. Here's a detailed comparison:

File Coverage

Key Implementation Difference

#7195 (Tranquil-Flow):

  • Uses a function parameter (memory_context) passed through the call chain
  • compress(memory_context="")_generate_summary(memory_context="")
  • Simpler, more direct parameter passing

#43567 (this PR):

  • Uses an instance attribute (_pre_compress_insights) with clear-on-use pattern
  • Captures return value → stores on compressor → cleared after use
  • Prevents stale data on re-entry (edge case in retry paths)
  • More explicit lifecycle management

Test Coverage Comparison

#7195: 3 static assertions in a new test file (checks presence in source)

#43567: 7 comprehensive unit tests covering:

  • Insights injection into prompt
  • Empty insights handling
  • Iterative update prompts
  • Clear-after-use behavior
  • Parameter passing verification
  • Whitespace-only edge case
  • Retry path survival

Recommendation

#7195 is preferred for:

  1. Earlier submission (2026-04-10 vs 2026-06-10)
  2. Simpler function-parameter approach
  3. External corroboration (user comment confirms the bug independently)

#43567 has merit for:

  1. More comprehensive test coverage
  2. Explicit clear-on-use pattern (prevents stale data)
  3. Retry path handling

Action Suggested

Since #7195 is already open and addressing the same issue with a simpler approach, I recommend closing #43567 as a duplicate. The comprehensive tests from this PR could potentially be added to #7195 if the maintainer agrees.

Reference: alt-glitch comment already flagged this overlap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MemoryProvider.on_pre_compress() return value discarded

3 participants