Skip to content

Fix/openclaw addmsg#1391

Merged
Mijamind719 merged 14 commits intomainfrom
fix/openclaw_addmsg
Apr 14, 2026
Merged

Fix/openclaw addmsg#1391
Mijamind719 merged 14 commits intomainfrom
fix/openclaw_addmsg

Conversation

@chenjw
Copy link
Copy Markdown
Collaborator

@chenjw chenjw commented Apr 12, 2026

Description

  1. 提交到session的内容,user和assistant分开,去掉了记忆召回部分,工具part修复input的问题。
  2. 修复agfs session文件中写入文件,会变成追加写的问题。
  3. 去掉了get_content返回的历史abstract 列表。

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

chenjw and others added 3 commits April 12, 2026 20:22
The localfs write implementation was ignoring the WriteFlag parameter and
not truncating the file when WriteFlag::Create or WriteFlag::Truncate was
specified. This caused files to be appended instead of overwritten when
writing empty content.

Root cause: The _flags parameter was declared but never used. When opening
an existing file, .truncate(true) was not called.

Fix: Use OpenOptions::new().truncate(should_truncate) based on the flags.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…traction

- Add workaround to delete messages.jsonl before writing to work around
  RAGFS localfs not truncating files (now fixed in ragfs)
- Add _get_latest_archive_last_msg_time to filter messages for memory
  extraction by timestamp
- Fix memory extraction to only process new messages after last archive

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 55
🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Fix localfs write truncation logic

Relevant files:

  • crates/ragfs/src/plugins/localfs/mod.rs

Sub-PR theme: OpenClaw plugin addmsg improvements

Relevant files:

  • examples/openclaw-plugin/client.ts
  • examples/openclaw-plugin/context-engine.ts
  • examples/openclaw-plugin/index.ts
  • examples/openclaw-plugin/text-utils.ts
  • bot/scripts/install_local_openclaw_plugin.sh

Sub-PR theme: Session message filtering and extraction fixes

Relevant files:

  • openviking/session/session.py

⚡ Recommended focus areas for review

API Breaking Change

Removed 'pre_archive_abstracts' from get_session_context response, breaking existing clients relying on this field.

# 不再返回 pre_archive_abstracts(只保留 latest_archive_overview)
included_pre_archive_abstracts: List[Dict[str, str]] = []
pre_archive_tokens = 0

archive_tokens = latest_archive_tokens + pre_archive_tokens
included_archives = len(included_pre_archive_abstracts)
dropped_archives = max(
    0, context["total_archives"] - context["failed_archives"] - included_archives
)

return {
    "latest_archive_overview": (
        latest_archive["overview"] if include_latest_overview else ""
    ),
    # 不再返回 pre_archive_abstracts
Incorrect Truncation Logic

Truncation flag check uses exact match instead of bitwise check, failing to truncate when only Truncate flag is set or other flags are combined.

// Determine if we should truncate based on flags
let should_truncate = matches!(flags, WriteFlag::Create | WriteFlag::Truncate);

// Open or create file with truncate support
let mut file = fs::OpenOptions::new()
    .write(true)
    .create(true)
    .truncate(should_truncate)
    .open(&local_path)
    .map_err(|e| Error::plugin(format!("failed to open file: {}", e)))?;
Private Attribute Access

Accessing private attributes _uri_to_path and agfs of viking_fs, breaking encapsulation and risking future compatibility issues.

# Workaround: RAGFSBindingClient.write is not truncating, so delete file first
# Use AGFS directly to avoid lock issues with viking_fs.rm
try:
    path = viking_fs._uri_to_path(f"{self._session_uri}/messages.jsonl", ctx=self.ctx)
    viking_fs.agfs.rm(path)
except Exception as e:
    logger.warning(f"[_write_to_agfs_async] Failed to delete messages.jsonl: {e}")

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling for archive timestamp retrieval

Add error handling around archive reading and timestamp parsing to avoid breaking
commit or context retrieval when archives are missing/corrupted. Return None on
failure, which will skip filtering and use all messages.

openviking/session/session.py [185-201]

 async def _get_latest_archive_last_msg_time(self) -> Optional[float]:
     """获取上一次归档的最后一条消息的时间戳(毫秒),用于过滤需要提取记忆的消息。"""
     # 使用 compression_index - 1 获取上一次归档的索引
     if self._compression.compression_index <= 1:
         return None
     # 获取上一次归档的目录
     archive_uri = (
         f"{self._session_uri}/history/archive_{self._compression.compression_index - 1:03d}"
     )
-    messages = await self._read_archive_messages(archive_uri)
-    if messages and messages[-1].created_at:
-        # 解析 ISO 时间戳为毫秒
-        from dateutil import parser
+    try:
+        messages = await self._read_archive_messages(archive_uri)
+        if messages and messages[-1].created_at:
+            # 解析 ISO 时间戳为毫秒
+            from dateutil import parser
 
-        dt = parser.parse(messages[-1].created_at)
-        return dt.timestamp() * 1000
+            dt = parser.parse(messages[-1].created_at)
+            return dt.timestamp() * 1000
+    except Exception as e:
+        logger.warning(f"[_get_latest_archive_last_msg_time] Failed to read archive timestamp: {e}")
     return None
Suggestion importance[1-10]: 7

__

Why: Adds error handling around archive reading and timestamp parsing to prevent breaking commit/context retrieval when archives are missing/corrupted, which improves robustness.

Medium

chenjw and others added 10 commits April 12, 2026 22:15
过滤 HEARTBEAT 健康检查消息,避免 HEARTBEAT.md 和 HEARTBEAT_OK
进入 session 存档。

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reverting to use logger.warn directly. The warnOrInfo wrapper was
unnecessary as logger always has warn method.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ragfs localfs write now respects WriteFlag to truncate files,
so the workaround to delete messages.jsonl before writing is no
longer needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove old addSessionMessage (string content)
- Keep addSessionMessage but accept parts array
- Update index.ts and context-engine.ts to use new format
- Update tests to check parts structure

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TypeScript strict mode requires handling possibly undefined warn method.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add trace_id to CommitSessionResult type and log it for all
commitSession calls (afterTurn, commitOVSession, compact).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add toolInput field to ExtractedMessage type
- Extract tool_input from msg.toolInput in extractNewTurnMessages
- Pass tool_input when constructing tool parts for addSessionMessage

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When extracting toolResult messages, look up the corresponding toolInput
from the preceding assistant messages' toolUse blocks using toolCallId/
toolUseId as the key.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Support additional field names: arguments, toolInput, tool_call_id

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Scan all messages to find toolCall/toolUse/tool_call blocks
- Extract tool input from arguments/input/toolInput fields
- Match toolResult with toolCall using toolCallId/toolUseId

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@chenjw chenjw requested review from MaojiaSheng and qin-ctx and removed request for MaojiaSheng April 13, 2026 08:42
@Mijamind719 Mijamind719 merged commit c430b97 into main Apr 14, 2026
15 of 18 checks passed
@Mijamind719 Mijamind719 deleted the fix/openclaw_addmsg branch April 14, 2026 06:03
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 14, 2026
Copy link
Copy Markdown
Collaborator

@qin-ctx qin-ctx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I left 4 blocking comments.

The main issues are: tool-call history no longer round-trips through afterTurn -> commit -> assemble; get_session_context() silently removes pre_archive_abstracts even though the public contract and consumers still require it; archive summaries are now generated from the filtered extraction subset instead of the actual archived message set; and compacted system-message extraction currently captures from the first ] instead of the last one.

CI is also failing on this branch: lint reports unused locals in session.py, and API integration tests already fail on the missing pre_archive_abstracts contract.

: undefined);
if (output) {
result.push({
role: "user",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking) This persists tool results as a user message containing a tool part, but the assemble path only reconstructs tool calls/results from tool parts on assistant messages and only when tool_id is present. extractNewTurnMessages() also does not persist the original tool call id, so a tool turn cannot round-trip through afterTurn -> commit -> assemble: the next assemble loses the toolUse/toolResult structure entirely. Please persist a stable tool identifier and store tool-call/tool-result data in a shape that the reader can actually reconstruct.

task_id=task.task_id,
archive_uri=archive_uri,
messages=messages_to_archive,
messages=messages_for_extraction,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking) messages_for_extraction is passed straight into _run_memory_extraction(), but that method also generates the archive summary and writes .abstract.md / .overview.md. That means the timestamp filter now changes the archive metadata as well, not just the memory-extraction input. If any archived messages are filtered out here, the summary no longer describes the actual messages.jsonl that was written for this archive. The filter should apply only to the extraction input, not to archive rendering.

latest_archive["overview"] if include_latest_overview else ""
),
"pre_archive_abstracts": included_pre_archive_abstracts,
# 不再返回 pre_archive_abstracts
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking) Removing pre_archive_abstracts here is a breaking API change, not an internal cleanup. The OpenClaw plugin type still requires it, the CLI renderer still expects it, the docs still publish it, and API/session tests still assert that it exists. CI is already failing in scenarios/sessions/test_session_commit.py for this exact reason. If the intent is to retire archive-index support, that needs a coordinated migration; otherwise this field needs to remain in the response.

// 格式: "System: [时间] Compacted ... Context ... [时间] 实际内容"
if (COMPACTED_SYSTEM_MSG_RE.test(text)) {
// 提取最后一个 ] 之后的内容(即实际用户输入)
const match = text.match(/\]\s*(.+)$/);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking) The implementation does not match the comment here. text.match(/\]\s*(.+)$/) matches from the first ], not the last one, so an input like System: [t1] Compacted ... [t2] real content becomes Compacted ... [t2] real content instead of just real content. That writes system noise back into the session and pollutes downstream extraction.

@chenjw chenjw mentioned this pull request Apr 14, 2026
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants