fix: change rag&longtermmemory hook from prereasoning to pre call#156
Conversation
此提交为从长期记忆检索到的内容添加了一个包装方法，并相应地调整了相关的测试和依赖项。此外，还改进了`PreCallEvent`处理逻辑，使其更通用且适用于更多场景。 Change-Id: I7cfedf2df9c37e09899e94636d551ca48f53f6e2 Co-developed-by: Aone Copilot <noreply@alibaba-inc.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the agent hook system to enable sequential message modification through PreCallEvent instead of PreReasoningEvent, standardizes formatting for long-term memory and RAG knowledge retrieval, and improves file writing robustness.
- Consolidated pre-processing hooks to use
PreCallEvent, allowing hooks to modify input messages sequentially with each hook receiving the event modified by previous hooks - Added standardized
wrap()method inLongTermMemoryToolsto format retrieved memory content with XML tags and descriptive text - Enhanced RAG knowledge injection to wrap content in
<retrieved_knowledge>tags and append as system message at the end of input messages
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/hook/PreCallEvent.java | Added inputMessages field with getter/setter to enable message modification by hooks |
| agentscope-core/src/main/java/io/agentscope/core/agent/AgentBase.java | Refactored notifyPreCall() to execute hooks sequentially, allowing each hook to modify messages |
| agentscope-core/src/main/java/io/agentscope/core/memory/LongTermMemoryTools.java | Added wrap() method to standardize formatting of retrieved memory content |
| agentscope-core/src/main/java/io/agentscope/core/memory/StaticLongTermMemoryHook.java | Updated to use PreCallEvent and append memory messages at end of input list |
| agentscope-core/src/main/java/io/agentscope/core/rag/GenericRAGHook.java | Updated to use PreCallEvent, wrap knowledge in XML tags, and append at end of messages |
| agentscope-core/src/main/java/io/agentscope/core/tool/file/WriteFileTool.java | Enhanced to create parent directories automatically when writing new files |
| agentscope-core/pom.xml | Updated maven-source-plugin from 3.3.0 to 3.3.1 |
| agentscope-core/src/test/java/io/agentscope/core/hook/HookEventTest.java | Updated tests to use new PreCallEvent constructor with inputMessages parameter |
| agentscope-core/src/test/java/io/agentscope/core/memory/StaticLongTermMemoryHookTest.java | Updated to use PreCallEvent and verify memory messages at end of list |
| agentscope-core/src/test/java/io/agentscope/core/memory/LongTermMemoryToolsTest.java | Updated expectations to verify wrapped memory content format |
| agentscope-extensions/agentscope-extensions-rag-simple/src/test/java/io/agentscope/core/rag/hook/GenericRAGHookTest.java | Updated to use PreCallEvent and verify knowledge messages at end of list |
| agentscope-extensions/agentscope-extensions-studio/src/test/java/io/agentscope/core/studio/StudioMessageHookTest.java | Updated to use new PreCallEvent constructor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| * Wraps the given text in a special format to indicate that it is retrieved from long-term memory. |
There was a problem hiding this comment.
This method is using block comment syntax (/* /) but should use Javadoc syntax (/* */) since it's a public static method that should be documented. Additionally, it should include @param and @return tags to properly document the method signature.
| /* | |
| * Wraps the given text in a special format to indicate that it is retrieved from long-term memory. | |
| /** | |
| * Wraps the given text in a special format to indicate that it is retrieved from long-term memory. | |
| * | |
| * @param text The text content retrieved from long-term memory to be wrapped. | |
| * @return The input text wrapped in a long-term memory annotation block. |
| */ | ||
| private List<Msg> createEnhancedMessages( | ||
| List<Msg> originalMessages, List<Document> retrievedDocs) { | ||
| private Msg createEnhancedMessages(List<Document> retrievedDocs) { |
There was a problem hiding this comment.
The method name createEnhancedMessages is misleading since it now returns a single Msg object instead of a List<Msg>. Consider renaming to createKnowledgeMessage or buildKnowledgeMessage to better reflect its current purpose.
|
|
||
| public List<Msg> getInputMessages() { | ||
| return inputMessages; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing javadoc for the getInputMessages() and setInputMessages() methods. These public methods should have proper documentation explaining their purpose, parameters, and return values.
| public List<Msg> getInputMessages() { | |
| return inputMessages; | |
| } | |
| /** | |
| * Returns the list of input messages that will be processed by the agent. | |
| * | |
| * @return the list of input {@link Msg} objects | |
| */ | |
| public List<Msg> getInputMessages() { | |
| return inputMessages; | |
| } | |
| /** | |
| * Sets the list of input messages to be processed by the agent. | |
| * | |
| * @param inputMessages the new list of input {@link Msg} objects | |
| */ |
| /** | ||
| * Constructor for PreCallEvent. | ||
| * | ||
| * @param agent The agent instance (must not be null) |
There was a problem hiding this comment.
The constructor javadoc should document the new inputMessages parameter, including its purpose and any constraints (e.g., can it be null?).
| * @param agent The agent instance (must not be null) | |
| * @param agent The agent instance (must not be null) | |
| * @param inputMessages The list of input messages for the agent. May be {@code null} if there are no input messages. |
| @@ -156,8 +156,8 @@ private Mono<PreReasoningEvent> handlePreReasoning(PreReasoningEvent event) { | |||
|
|
|||
| // Inject memory message at the beginning | |||
There was a problem hiding this comment.
The comment on line 157 says "Inject memory message at the beginning" but the code on line 160 uses enhancedMessages.add(memoryMsg) after adding all input messages, which places it at the end, not the beginning. The comment should be updated to say "Inject memory message at the end" or similar.
| // Inject memory message at the beginning | |
| // Inject memory message at the end |
| */ | ||
| public static String wrap(String text) { | ||
| return "Below is content retrieved from the long-term memory associated with the current" | ||
| + " user, Please extract useful information from it in the context of the" |
There was a problem hiding this comment.
Punctuation issue: The comma before "Please" should be a period since "Please extract useful information..." is a separate sentence from "Below is content retrieved...".
| + " user, Please extract useful information from it in the context of the" | |
| + " user. Please extract useful information from it in the context of the" |
| @@ -198,26 +199,17 @@ private String extractQueryFromMessages(List<Msg> messages) { | |||
| * | |||
| * <p>The knowledge is injected as a system message at the beginning of the message list. | |||
There was a problem hiding this comment.
The javadoc comment is outdated. It says "The knowledge is injected as a system message at the beginning of the message list" but the new implementation adds it at the end (line 166 uses enhancedMessages.add(enhancedMessage) after adding all input messages). The documentation should be updated to reflect that the knowledge message is appended at the end.
| * <p>The knowledge is injected as a system message at the beginning of the message list. | |
| * <p>The knowledge is injected as a system message at the end of the message list. |
| * @param retrievedDocs the retrieved documents | ||
| * @return the enhanced message list with knowledge context |
There was a problem hiding this comment.
The javadoc return description says "the enhanced message list with knowledge context" but the method now returns a single Msg object, not a list. The documentation should be updated to accurately describe the return value as a single message containing the knowledge context.
| /** | ||
| * Constructor for PreCallEvent. | ||
| * | ||
| * @param agent The agent instance (must not be null) | ||
| * @throws NullPointerException if agent is null | ||
| */ | ||
| public PreCallEvent(Agent agent) { | ||
| public PreCallEvent(Agent agent, List<Msg> inputMessages) { |
There was a problem hiding this comment.
The javadoc at line 25 states "Modifiable: No (notification-only)" but the class now has setInputMessages() method (line 59-61) that allows modification of input messages. The documentation should be updated to indicate that this event is modifiable via setInputMessages(), similar to PreReasoningEvent's documentation. The Context section should also be updated to include getInputMessages() and note that messages can be modified.
| @DisplayName("Should ignore non-PreReasoningEvent events") | ||
| void testIgnoreOtherEvents() { | ||
| PreCallEvent preCallEvent = new PreCallEvent(mockAgent); | ||
| PreCallEvent preCallEvent = new PreCallEvent(mockAgent, List.of()); |
There was a problem hiding this comment.
This test is checking that the hook ignores non-PreCallEvent events, but it's actually testing with a PreCallEvent, which the hook should handle (not ignore). The test should use a different event type (e.g., PostCallEvent or PreReasoningEvent) to properly validate that other event types are ignored. Additionally, the display name should be updated to say "non-PreCallEvent" instead of "non-PreReasoningEvent".
AlbumenJ
changed the title
2 participants
This pull request refactors the agent hook system to streamline how input messages are modified by hooks before agent processing, and improves the handling and formatting of retrieved long-term memory and RAG knowledge. The most impactful changes are the consolidation of pre-processing hooks to use
PreCallEvent, enhanced message wrapping for memory and knowledge retrieval, and improved file writing robustness.Agent Hook System Refactor
PreCallEventinstead ofPreReasoningEvent, allowing hooks to modify input messages sequentially and simplifying hook implementations (AgentBase.java,PreCallEvent.java,StaticLongTermMemoryHook.java,GenericRAGHook.java). [1] [2] [3] [4]PreCallEventconstructor and usage. [1] [2] [3]Long-Term Memory and RAG Knowledge Formatting
wrapmethod inLongTermMemoryToolsto clearly mark retrieved memory content in messages, and updated all relevant usages and tests to utilize this format. [1] [2] [3] [4] [5] [6] [7]<retrieved_knowledge>tags for clarity, and injected as a system message at the end of the input message list. [1] [2] [3]File Tool Robustness
Dependency Update
maven-source-plugin.versionfrom3.3.0to3.3.1inpom.xml.此提交为从长期记忆检索到的内容添加了一个包装方法，并相应地调整了相关的测试和依赖项。此外，还改进了PreCallEvent处理逻辑，使其更通用且适用于更多场景。Change-Id: I7cfedf2df9c37e09899e94636d551ca48f53f6e2
Co-developed-by: Aone Copilot noreply@alibaba-inc.com
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.1, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)