fix(compaction): keep prior summary in chained summarization input (#2359)#2373
Open
gxgeek-n wants to merge 2 commits into
Open
fix(compaction): keep prior summary in chained summarization input (#2359)#2373gxgeek-n wants to merge 2 commits into
gxgeek-n wants to merge 2 commits into
Conversation
…skill
SkillRegistration.agentTool() was a single-slot field — calling it twice on
one registration overwrote the first tool, so only the last AgentTool was
bound into the skill's gated tool group ({skillId}_skill_tools). Any skill
declaring multiple tools silently lost all but the last one: those tools
were neither gated (visible without loading the skill) nor unlockable.
Since Toolkit.ToolRegistration enforces exactly-one-tool-per-registration,
SkillRegistration now accumulates agentTools into a list and apply() binds
each one with its own Toolkit registration. Other tool kinds (toolObject /
mcpClient / subAgent) also get their own registrations instead of sharing
one chain that violated the exactly-one rule when combined.
Regression test (SkillBoxTest#testMultipleAgentToolsAllBoundToSkillGroup):
- fails on the v2.0.0 tag with "first tool must also be bound into the
skill group (regression: was overwritten)"
- passes with this fix; full SkillBoxTest suite green (16/16)
…gentscope-ai#2359) ConversationCompactor filtered previous summary messages out of the prefix used for BOTH flush/offload dedup AND the LLM summarization input. The filter is correct for dedup (its documented purpose), but feeding the filtered prefix to summarizePrefix broke the chained-summarization intent chain: - round N's summary was built only from messages since round N-1's summary — the prior summary (the only remaining record of the original intent) was dropped from the source material, decaying user intent round over round; - when the prefix consisted only of a prior summary, the round degenerated to "No previous conversation history." Fix: summarize over the UNFILTERED prefix (old summary + new raw messages → cumulative summary), and use the filtered list only for flushMemories, which is what the filter was documented for. Offload behavior unchanged. Regression tests (ConversationCompactorChainedSummaryTest, 2 cases): - without the fix both fail (prior intent absent from summarization prompt / degenerate output); with the fix both pass; full harness suite green (666).
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR primarily fixes chained summarization in ConversationCompactor so that each compaction round incorporates the prior compaction summary into the next summarization input (preventing intent decay and avoiding the "No previous conversation history." degeneration). It also includes an additional, separate change in SkillBox to correctly bind multiple agentTool() registrations to the same skill tool group.
Changes:
- Fix compaction to summarize over the unfiltered prefix while using the filtered prefix only for memory flush de-duplication.
- Add regression tests covering chained summarization behavior across multiple compaction rounds.
- Update
SkillBoxregistration to support multipleagentTool()calls (and add a regression test).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| agentscope-harness/src/main/java/io/agentscope/harness/agent/memory/compaction/ConversationCompactor.java | Keeps prior summary in summarization input; uses filtered prefix only for flushing. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/memory/compaction/ConversationCompactorChainedSummaryTest.java | New regression tests ensuring chained summarization preserves prior intent. |
| agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java | Registers multiple AgentTools for a skill by iterating registrations (but needs a guard to prevent mixing registration types). |
| agentscope-core/src/test/java/io/agentscope/core/skill/SkillBoxTest.java | Adds regression test for binding multiple agent tools to a single skill’s gated group. |
Comments suppressed due to low confidence (1)
agentscope-core/src/main/java/io/agentscope/core/skill/SkillBox.java:600
- apply() now registers agentTools, toolObject, mcpClient, and subAgent in separate Toolkit registrations. This can unintentionally allow multiple registration types to be configured on the same SkillRegistration (e.g., calling both tool() and agentTool()), which previously would fail Toolkit's exactly-one validation. Add an explicit guard to enforce that only one registration type is set (while still allowing multiple agentTool() calls).
if (toolObject != null
|| !agentTools.isEmpty()
|| mcpClientWrapper != null
|| subAgentProvider != null) {
Comment on lines
+118
to
+122
| // 摘要输入必须保留上轮摘要（链式摘要的意图链不能断，#2359）； | ||
| // 过滤版仅用于 flush/offload 去重（filterSummaryMessages 的 javadoc 原意）， | ||
| // 此前过滤版同时喂了摘要输入 → 连续压缩丢原始意图。 | ||
| List<Msg> prefix = new ArrayList<>(messages.subList(0, cutoff)); | ||
| List<Msg> prefixForFlush = filterSummaryMessages(prefix); |
Comment on lines
+609
to
+610
| // Toolkit.ToolRegistration 每次只能注册一个工具（exactly-one 校验）， | ||
| // 多 tool 的 skill 必须逐个独立注册，否则只有最后一个生效 |
Comment on lines
+51
to
+56
| return Flux.just( | ||
| ChatResponse.builder() | ||
| .content( | ||
| List.<io.agentscope.core.message.ContentBlock>of( | ||
| TextBlock.builder().text("压缩后的新摘要").build())) | ||
| .build()); |
Comment on lines
+199
to
+200
| // 连续两次 agentTool()：此前第二次会覆盖第一次，导致只有 second 被绑进门控组 | ||
| skillBox.registration().skill(skill).agentTool(first).agentTool(second).apply(); |
Comment on lines
383
to
387
| private Toolkit toolkit; | ||
| private AgentSkill skill; | ||
| private Object toolObject; | ||
| private AgentTool agentTool; | ||
| private final List<AgentTool> agentTools = new ArrayList<>(); | ||
| private McpClientWrapper mcpClientWrapper; |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2359 —
ConversationCompactorfiltered previous summary messages out of the prefix used for both flush/offload dedup and the LLM summarization input. The filter is correct for dedup (its documented purpose), but feeding the filtered prefix tosummarizePrefixbroke the chained-summarization intent chain.Bug
Each compaction round replaced the prefix with a single new summary built from the filtered list:
summarizePrefixhit its empty branch and the round degenerated to "No previous conversation history."(Note: the offload step already uses the unfiltered
messages, so the filter never actually performed its dedup role there either — its only real effect was stripping intent from the summary input.)Fix
flushMemories, which is whatfilterSummaryMessageswas documented for.Tests
New
ConversationCompactorChainedSummaryTest(2 cases):chainedCompaction_keepsPriorSummaryInSummarizationInput— prior intent text must appear in the prompt sent to the summary modelprefixOfOnlyPriorSummary_doesNotDegenerate— must not emit "No previous conversation history."Both directions verified: without the fix both tests fail (prior intent absent / degenerate output); with the fix both pass. Full harness suite green (666 tests, 0 failures).