Skip to content

feat: support extra metadata in batch write - #223

Merged
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
loveRhythm1990:lr90/interface
Aug 3, 2026
Merged

feat: support extra metadata in batch write#223
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
loveRhythm1990:lr90/interface

Conversation

@loveRhythm1990

@loveRhythm1990 loveRhythm1990 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR improves MCP argument validation and adds end-to-end support for arbitrary memory metadata, including batch writes.

Related Issue

Fixes #224

Changes

  • Validate required MCP arguments for both embedded and remote modes:
    • memory_store.content
    • memory_retrieve.query
    • memory_search.query
    • memory_correct.new_content
  • Reject blank required values and invalid extra_metadata types.
  • Keep validation failures as MCP tool results instead of JSON-RPC internal errors.
  • Add extra_metadata support to:
    • REST single-memory writes
    • REST batch writes
    • MCP memory_store
    • memory responses
    • lightweight list queries
    • embedded and remote retrieve/search output
  • Normalize empty metadata objects to None for consistent write/read behavior.
  • Preserve metadata through the service and storage layers.
  • Refresh metadata when a same-content duplicate is detected.
  • Return the actual persisted record after deduplication instead of a non-existent generated ID.
  • Add an audit-log entry for deduplicated metadata updates.
  • Handle the race where a deduplication target becomes inactive before it can be fetched.
  • Add regression coverage for missing and blank MCP arguments.
  • Ignore nested .DS_Store files.

Motivation

MCP calls previously allowed missing or whitespace-only required arguments, which could create invalid memories or issue meaningless retrieval requests.

The API also did not expose a complete path for business metadata such as scene or agent. This metadata needs to survive single and batch writes and remain available when
memories are read.

Testing

The following checks were run successfully:

  • cargo test -p memoria-mcp --lib --test tools_unit
  • cargo test -p memoria-api --test api_e2e test_mcp_memory_ -- --nocapture
  • cargo test --no-run -p memoria-api -p memoria-service -p memoria-storage -p memoria-mcp
  • RUSTC_WRAPPER= cargo clippy -p memoria-api -p memoria-service -p memoria-storage -p memoria-mcp --lib -- -D warnings
  • git diff --check

Notes

  • No database migration is required because memories.extra_metadata already exists.
  • Empty metadata objects are normalized to absent metadata to match the existing MatrixOne JSON workaround.
  • cargo fmt --all -- --check currently reports repository-wide formatting drift.

Follow-ups

  • Expose extra_metadata through the Python SDK and OpenClaw client.
  • Include metadata in remote memory_list output.
  • Ensure the deduplication race-insert path runs the standard metrics and entity-extraction hooks.
  • Clarify or implement scene-specific retrieval decay if scoring is expected to consume this metadata directly.
  • Add dedicated metadata round-trip and deduplication regression tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the memory write/read pipeline to support arbitrary per-memory extra_metadata (including batch writes) and tightens MCP argument validation so missing/blank required fields return consistent “soft” tool results in both embedded and remote modes.

Changes:

  • Add end-to-end passthrough for extra_metadata across API → service → storage, including lightweight list queries and MCP tool output.
  • Improve MCP tool argument validation for required string fields (reject missing/blank) and reject invalid extra_metadata types while keeping failures as tool results (not JSON-RPC internal errors).
  • Improve dedup behavior for same-content stores by refreshing survivor metadata and returning the actually persisted record (with a race fallback).

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
memoria/crates/memoria-storage/src/store.rs Adds metadata refresh update, includes extra_metadata in lite listings, and adjusts SQL selects/mappers for metadata passthrough.
memoria/crates/memoria-service/tests/subject_id_mo_e2e.rs Updates batch-store test tuple shape to include extra_metadata.
memoria/crates/memoria-service/src/service.rs Extends service batch/single store APIs with extra_metadata, normalizes empty maps, and updates dedup behavior and side effects.
memoria/crates/memoria-mcp/tests/tools_unit.rs Adds unit coverage for rejecting missing/blank required MCP arguments.
memoria/crates/memoria-mcp/tests/edit_log_e2e.rs Updates batch-store test tuple shape to include extra_metadata.
memoria/crates/memoria-mcp/tests/core_tools_e2e.rs Adds E2E coverage for MCP validation of missing/blank required arguments.
memoria/crates/memoria-mcp/src/tools.rs Implements shared tool-arg validation and adds extra_metadata support to MCP store + output formatting.
memoria/crates/memoria-mcp/src/remote.rs Applies shared validation in remote mode and forwards/prints extra_metadata.
memoria/crates/memoria-api/tests/api_e2e.rs Adds E2E coverage ensuring MCP validation returns tool results (not RPC errors) and doesn’t create memories.
memoria/crates/memoria-api/src/routes/memory.rs Threads extra_metadata through REST single and batch store endpoints into the service layer.
memoria/crates/memoria-api/src/models.rs Adds extra_metadata to REST request/response models.
.gitignore Adds an ignore entry for a nested .DS_Store path.
Suppressed comments (1)

memoria/crates/memoria-api/src/models.rs:291

  • This response-field doc comment also claims Memoria uses extra_metadata for retrieval-time decay/scoring, but there’s no implementation evidence for that. Align this comment with the current behavior: stored and returned verbatim, interpretation left to clients.
    /// 业务元数据(如 scene)从 memories.extra_metadata 透传回给调用方,
    /// 供检索期打分(decay 按 scene 取半衰期/抗遗忘地板)使用。
    #[serde(skip_serializing_if = "Option::is_none")]
    pub extra_metadata: Option<std::collections::HashMap<String, serde_json::Value>>,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread memoria/crates/memoria-storage/src/store.rs
Comment thread memoria/crates/memoria-api/src/models.rs Outdated
Comment thread memoria/crates/memoria-storage/src/store.rs
Comment thread memoria/crates/memoria-service/src/service.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

memoria/crates/memoria-storage/src/store.rs:5305

  • The doc comment still says this “lite” list skips extra_metadata, but the query now selects it and row_to_memory_lite populates it. Please update/remove the outdated wording so callers and tests don’t assume metadata is omitted.
    /// Lightweight list for API responses — skips embedding, source_event_ids,
    /// extra_metadata to reduce I/O and deserialization cost.
    #[allow(clippy::too_many_arguments)]

memoria/crates/memoria-service/src/service.rs:1360

  • New behavior updates the survivor’s extra_metadata on same-content dedup and returns the persisted record (plus a race-insert path when the survivor becomes inactive). There’s no regression test covering this path (metadata refresh + “no phantom record” guarantee), so it’s easy to break without noticing.
                    // Same content — a near-duplicate already exists. Do NOT create/return a
                    // phantom, never-inserted record. Refresh the survivor's extra_metadata with
                    // the caller's new metadata (so an updated scene/agent isn't silently dropped),
                    // write an audit edit-log entry, then return the ACTUAL persisted record
                    // (real id + real author/session/trust/timestamps), not the new in-memory object.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (4)

memoria/crates/memoria-mcp/src/tools.rs:569

  • Same as above: this metadata suffix formatting is currently a long nested expression, which hurts readability and is duplicated across branches. Extracting it into a small block makes it easier to change consistently.
                let text = results
                    .iter()
                    .map(|m| format!("[{}] ({}) {}{}", m.memory_id, m.memory_type, m.content, m.extra_metadata.as_ref().filter(|md| !md.is_empty()).map(|md| format!(" | metadata: {}", serde_json::to_string(md).unwrap_or_default())).unwrap_or_default()))
                    .collect::<Vec<_>>()
                    .join("\n");

memoria/crates/memoria-mcp/src/tools.rs:718

  • Same readability/duplication concern for list output: the metadata suffix formatting is currently an inline nested expression. Expanding it into a small block (or a shared helper) will make future changes safer.
            let text = memories
                .iter()
                .map(|m| format!("[{}] ({}) {}{}", m.memory_id, m.memory_type, m.content, m.extra_metadata.as_ref().filter(|md| !md.is_empty()).map(|md| format!(" | metadata: {}", serde_json::to_string(md).unwrap_or_default())).unwrap_or_default()))
                .collect::<Vec<_>>()
                .join("\n");

memoria/crates/memoria-mcp/src/tools.rs:547

  • The metadata formatting in the retrieve/search output is a single very long expression, which makes this section hard to read/maintain and duplicates the same logic in multiple branches. Consider expanding it into a small block that computes the optional metadata suffix separately.

This issue also appears in the following locations of the same file:

  • line 565
  • line 714
                let text = results
                    .iter()
                    .map(|m| format!("[{}] ({}) {}{}", m.memory_id, m.memory_type, m.content, m.extra_metadata.as_ref().filter(|md| !md.is_empty()).map(|md| format!(" | metadata: {}", serde_json::to_string(md).unwrap_or_default())).unwrap_or_default()))
                    .collect::<Vec<_>>()
                    .join("\n");

memoria/crates/memoria-mcp/src/tools.rs:119

  • New behavior rejects non-object extra_metadata, but there are no tests covering the invalid-type cases (e.g. string/array/number). Adding regression tests would help ensure both embedded and remote MCP modes keep rejecting these inputs without creating memories.
pub fn validate_tool_args(name: &str, args: &Value) -> Result<(), &'static str> {
    // extra_metadata 若存在必须是 object(与 REST StoreRequest 一致);非 object 明确拒绝,
    // 不能静默丢弃当作缺失。
    if let Some(v) = args.get("extra_metadata") {
        if !v.is_null() && !v.is_object() {
            return Err("extra_metadata must be an object");
        }
    }

@aptend aptend left a comment

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.

有两点需要在合并前处理:

  1. BatchStoreItem 从五元组改成六元组,并给公开的 store_memory_on_branch 新增必填参数,会让现有 Rust 调用方直接编译失败,与 #224 的 backward-compatible 要求冲突。请保留原有公开签名并由旧入口传 None,另加 metadata-aware 方法/options struct,或采用其他兼容方案。

  2. 当前新增测试主要覆盖 MCP 空参数校验,没有覆盖 #224 要求的 extra_metadata round trip。请至少补齐 REST 单条/批量写入及 list/get/retrieve/search 读取、空对象语义、非 object 拒绝、同内容 dedup metadata 更新/返回真实 ID,以及 embedded/remote MCP metadata 透传测试。这里包含 MatrixOne JSON 与 lite mapper 行为,仅编译检查不足以验证。

当前 DB Tests 的 9 个 snapshot 失败看起来与本 PR 无关,已由 #226 修复;建议 #226 合并后更新本分支并重新跑完整 DB Tests。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

memoria/crates/memoria-storage/src/store.rs:5227

  • The doc comment above list_active_lite still says the lite list skips extra_metadata, but the query now explicitly selects and maps it (via extra_meta/row_to_memory_lite). This is contradictory and can mislead callers and future maintainers.
    /// Lightweight list for API responses — skips embedding, source_event_ids,
    /// extra_metadata to reduce I/O and deserialization cost.

@loveRhythm1990
loveRhythm1990 requested a review from aptend August 3, 2026 06:38

@aptend aptend left a comment

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.

上次两项 blocking feedback 已解决:

  • 保留了 BatchStoreItemstore_memory_on_branchstore_batch_on_branch 的原公开签名,并新增 metadata-aware API,现有 Rust 调用方保持兼容。
  • 已补充 MatrixOne-backed 的 REST 单条/批量、list/get/retrieve/search、空对象、非法类型、同内容 dedup,以及 embedded/remote MCP metadata 覆盖。

完整 DB Tests、Unit Tests、Check & Clippy 均通过。本轮未发现新的阻塞问题。

@XuPeng-SH
XuPeng-SH merged commit 54c9114 into matrixorigin:main Aug 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Support user-defined extra_metadata for memories

4 participants