feat(mcp): add memory.store and memory.note write tools

[Liohtml]

Summary

• Add memory.store MCP tool: creates memory documents from content with configurable namespace and tags, routing to the existing doc_put RPC
• Add memory.note MCP tool: creates annotation documents linked to existing chunks via metadata.annotates_chunk_id, also routing through doc_put
• Add enforce_write_policy() for write-specific security gating (distinct from read/act for audit clarity)
• Add dispatch_write_tool() with structured tracing::info! audit logging (tool, chunk_id, client fields)
• Both tools set source_type = "mcp" for provenance tracking
• 11 new unit tests covering param validation, defaults, slug generation, and error cases

Closes #2269

Test plan

• cargo check -p openhuman --lib passes
• cargo test -p openhuman --lib mcp_server::tools — all 40 tests pass (29 existing + 11 new)
• cargo fmt --all applied
• CI checks pass on PR

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added memory.store to save memories (title + content) with automatic namespace, deterministic key generation and tagging support
  • Added memory.note to attach notes to specific memory chunks with automatic key generation and linked metadata and content structure

• Tests

  • Expanded unit tests covering the new memory tools, argument validation (required fields and unknown-field rejection), defaults, and metadata/content construction

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds two MCP write tools—memory.store and memory.note—registered to openhuman.memory_doc_put. Both enforce a write gate, validate inputs against JSON schemas, build RPC parameters with deterministic keys/slugs, dispatch via a dedicated write path with audit logs, and include unit tests.

Changes

Memory Write Tools

Layer / File(s)	Summary
Tool declarations and schemas src/openhuman/mcp_server/tools.rs	Defines allowed-argument keys and registers memory.store / memory.note in the base tool catalog mapped to openhuman.memory_doc_put; adds JSON schemas that require specific fields and disallow extra properties.
Tool dispatch routing src/openhuman/mcp_server/tools.rs	call_tool now recognizes the new tools, enforces write-policy (Act), validates controller params, and routes to a dedicated write dispatch helper.
RPC parameter construction src/openhuman/mcp_server/tools.rs	build_rpc_params adds branches for both tools: required-field validation, namespace defaulting for memory.store, deterministic key generation, tags forwarding, note document composition, and metadata.annotates_chunk_id for notes.
Write enforcement and dispatch src/openhuman/mcp_server/tools.rs	Adds enforce_write_policy to gate write operations and dispatch_write_tool to invoke openhuman.memory_doc_put, map outcomes, and emit structured audit logs.
Slug generation helper src/openhuman/mcp_server/tools.rs	slug_from normalizes titles into deterministic, URL-safe slugs with lowercasing, non-alphanumeric→hyphen conversion, hyphen collapsing, truncation, and hash-based fallback for non-ASCII/empty inputs.
Tests / protocol expectations src/openhuman/mcp_server/protocol.rs, src/openhuman/mcp_server/tools.rs	Updates tools/list expectation to include the new tools; adds tests for RPC-param construction/validation, namespace/tag handling, unknown-argument rejection, constructed document fields, and slug_from behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MCP_Server
  participant WritePolicy
  participant RPC_Builder
  participant MemoryService
  Client->>MCP_Server: tools.call(memory.store | memory.note, args)
  MCP_Server->>WritePolicy: enforce_write_policy(controller, ToolOperation::Write)
  WritePolicy-->>MCP_Server: allow/deny
  MCP_Server->>RPC_Builder: build_rpc_params(tool, args)
  RPC_Builder-->>MCP_Server: rpc_params
  MCP_Server->>MemoryService: openhuman.memory_doc_put(rpc_params)
  MemoryService-->>MCP_Server: success/error
  MCP_Server-->>Client: result (including audit/log entry)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• MCP write tools: propagate client_info into chunk provenance (source_type = "mcp:<client>") #2317: This PR sets source_type = "mcp" while the issue proposes mcp:<client> provenance, so the change is closely related to threading clientInfo into write-tool RPC construction.

Possibly related PRs

• tinyhumansai/openhuman#1760: Extends the same MCP stdio tool-discovery/call plumbing and is a likely predecessor to these write-tool changes.

Suggested reviewers

• senamakel

Poem

A rabbit hops through code and logs,
Two tools to store and note the thoughts.
Slugs march tidy, hashes keep the pace,
Memory finds a stable place. 🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and specifically summarizes the main change: adding two new MCP write tools (memory.store and memory.note) to the codebase.
Linked Issues check	✅ Passed	The PR implements the primary objectives from issue #2269 Phase 1: adds memory.store and memory.note write tools with provenance (source_type=mcp), security gating (enforce_write_policy), and audit logging (dispatch_write_tool). Defers tree.tag and destructive operations as planned.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issue objectives: memory.store and memory.note tools, write-path scaffolding, slug generation, parameter validation, and unit tests. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1177-1216: slug_from currently can return an empty string for
titles with only non-alphanumeric characters, causing key collisions; update
slug_from so that after collapsing/trimming and truncating you check if
result.is_empty() and if so produce a deterministic fallback (e.g., "untitled"
or "doc-" plus a short hex/hash of the original title) that is URL-safe and <=64
chars, ensure any trailing hyphens are removed, and return that non-empty
fallback so keys like "mcp-store-" are avoided.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 26b66ae5-a3a2-4d7d-880b-1741465d4827

📥 Commits

Reviewing files that changed from the base of the PR and between 65d92bf and bbe7ab7.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

[Liohtml]

Credit to @justinhsu1477 for the thorough RFC-style issue (#2269) — the architecture questions, proposed tool surface, and phased rollout plan made scoping this first wave (memory.store + memory.note) much cleaner. The provenance and audit logging patterns follow directly from the issue's Q2/Q4 proposals.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1215-1218: The current slug_from fallback returns a constant
"untitled" which collapses all Unicode-only titles into the same key; update
slug_from so that when the ASCII-alphanumeric result is empty it returns
"untitled-<stable_hash>" where <stable_hash> is a short deterministic hash
(e.g., hex of SHA256 or SipHash truncated to a few chars) computed from the
original title string, ensuring unique stable slugs for titles like "会议记录" and
"Протокол"; apply the same change to the identical fallback in the other
occurrence noted (around the second block at lines similar to 1763-1769), and
add a regression test that creates two documents with different Unicode-only
titles and asserts their generated keys (e.g., mcp-store-<slug>) are distinct
and stable across runs.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a18feea9-77d8-4b2b-8e58-d087d7249f91

📥 Commits

Reviewing files that changed from the base of the PR and between bbe7ab7 and b1d584a.

📒 Files selected for processing (2)

• src/openhuman/mcp_server/protocol.rs
• src/openhuman/mcp_server/tools.rs

[justinhsu1477]

@Liohtml — thanks for picking this up so fast, and for the explicit RFC credit.

Quick mapping of your implementation against the four open questions in #2269, mostly so I can verify I'm reading the diff right and so anyone landing here later sees the trade-off space:

• Q1 (multi-process write path) — you route through the existing doc_put RPC, which is effectively Option A from the RFC (daemon is the sole writer). Cleanest path for v1 and side-steps Option C's WAL-coordination work.
• Q2 (provenance) — metadata.source_type = "mcp" on each chunk. Schema-light, no migration.
• Q3 (SecurityPolicy) — separating enforce_write_policy() from the read enforcer is a nicer split than tunnelling both through the same gate; matches what the RFC was hoping for under "default-allow + audit".
• Q4 (audit log) — structured tracing::info! instead of a new mcp_writes table. Reasonable for v1; if maintainers later want queryable history this is the easiest place to swap a SQLite-backed sink in.

Two follow-ups I'd happily own so the RFC closes out cleanly:

1. tree.tag — the third tool the RFC proposed. Want me to spin it up as a follow-up PR using your enforce_write_policy + dispatch_write_tool scaffolding once this one lands, so it merges cleanly on top?
2. client_info propagation — RFC Q2 also suggested capturing the calling MCP client name (e.g. "mcp:Claude Desktop" from the initialize payload) on each chunk, so a write from Claude Desktop reads differently from one written by Cursor. Doesn't need to block this PR — happy to file as a separate issue once this lands if maintainers want it.

Happy to do a deeper line-level review if it's useful.

[Liohtml]

@justinhsu1477 Great mapping — you're reading the implementation exactly right on all four questions.

Re your follow-ups:

1. tree.tag — yes, please! The enforce_write_policy + dispatch_write_tool scaffolding is designed to be additive, so a follow-up PR on top should be clean. Happy to review it.
2. client_info propagation — agreed this should be a separate issue. The initialize payload's clientInfo is the right source; the current source_type = "mcp" is a placeholder that your follow-up would refine to "mcp:Claude Desktop" etc.

A deeper line-level review would absolutely be useful if you have the time — especially around the dispatch_write_tool → doc_put RPC parameter assembly, since you know the memory schema better than anyone.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1215-1221: The slug-generation currently only appends a hash when
result.is_empty(), which lets mixed-script titles that normalize to a numeric
ASCII slug (e.g. "会议记录 2026" and "Протокол 2026") collide; change the logic in
the slugifier so that whenever any non-ASCII alphanumeric characters were
dropped during normalization you append a short stable hash (use the existing
Sha256 + hex::encode(...)[..8] pattern) to the ASCII slug instead of only on
empty slugs, update the return formatting to include that hash (the code around
result/is_empty() and the Sha256 usage), and add a regression test that stores
two mixed-script titles (e.g. the two examples) and verifies their memory.store
keys (mcp-store-...) differ.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cb0ed376-5657-4afb-82f1-5e3970d7022d

📥 Commits

Reviewing files that changed from the base of the PR and between b1d584a and 79026df.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1215-1238: The slug truncation must reserve space for the
mixed-script hash so the hash isn't dropped; in the block handling had_non_ascii
(where you compute hash from title.as_bytes()), compute hash_len =
hex::encode(...).len(), then truncate the base result to max_base = 64 - 1 -
hash_len (if max_base < 0 treat as 0), trim trailing '-' from the truncated
base, and only then set result = format!("{result}-{hash}") (and if result was
empty keep returning "untitled-{hash}" as before). Also ensure the later global
truncation (the final if result.len() > 64) only runs when no hash was appended
(or adjust it to use the same reserved-size logic), and add a regression test
for a long mixed-script pair to verify two long titles differing only in
non-ASCII suffix produce distinct slugs.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 17b71a6f-132d-4d9f-b92b-a7131dfd332f

📥 Commits

Reviewing files that changed from the base of the PR and between 79026df and cc8507b.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

[justinhsu1477]

@Liohtml — green light on tree.tag ack'd, both follow-ups are now up:

• feat(mcp): add tree.tag write tool (completes Phase 3 #2269) #2316 — tree.tag PR layered on top of this branch. Reuses enforce_write_policy + dispatch_write_tool unchanged; +242 LoC net (the new dispatch arm, the tree_tag_schema, the build_rpc_params branch, a small required_non_empty_string_array helper next to optional_string_array, and 8 unit tests covering each failure path). Marked Depends on #2306 and will rebase cleanly when this lands.
• MCP write tools: propagate client_info into chunk provenance (source_type = "mcp:<client>") #2317 — client_info propagation filed as a separate issue. Includes the proposed mcp:<normalized-name> shape on source_type plus the back-compat "mcp" fallback when clientInfo.name is absent / empty, so any client predating this stays on the current behaviour.

For the deeper line-level look at dispatch_write_tool → doc_put parameter assembly: I'd rather sit with the second CodeRabbit round here first so the review lands against the version heading for merge, instead of churning on in-flight code. Will circle back after that.

[YOMXXX]

@senamakel #2306 当前 checks 24 success / 2 skipped / 0 pending / 0 failure，CodeRabbit approved，review threads 0 unresolved。这个是 MCP write tools 的 Phase 1（memory.store / memory.note），麻烦再看一下。

[graycyrus]

Walkthrough

Adds two write tools (memory.store, memory.note) to the MCP surface, both routing through openhuman.memory_doc_put. Includes enforce_write_policy() for security gating, dispatch_write_tool() for audit-logged dispatch, a slug_from() utility for deterministic key generation, and 11 new unit tests. All prior CodeRabbit findings on slug edge cases are addressed — nice iterating on that.

File	Changes
tools.rs	+2 tool specs, +2 schemas, +call_tool routing, +build_rpc_params arms, +enforce_write_policy, +dispatch_write_tool, +slug_from, +11 tests
protocol.rs	+2 entries in tool-name list test

1 critical issue blocks this PR — see inline comment. The build_rpc_params logic (key generation, provenance, metadata) is unreachable because dispatch_write_tool receives raw client args and passes them straight to the RPC.

Minor notes (not blocking):

• enforce_write_policy is functionally identical to enforce_act_policy (both use ToolOperation::Act). The "distinct log line for auditors" rationale from the doc comment doesn't hold since both produce the same log pattern — the only difference is the function name. Consider either adding a ToolOperation::Write variant or just reusing enforce_act_policy until the distinction matters.
• dispatch_write_tool mixes tracing::info! (success path) with log::warn!/log::error! (error paths). Pick one facade for consistency.

[justinhsu1477]

@graycyrus — quick technical note on the [critical] flag against dispatch_write_tool:

I think the read may have skipped the line right above the match arm. build_rpc_params runs before the match in call_tool, not inside any of the arms:

// src/openhuman/mcp_server/tools.rs (current PR HEAD)
pub async fn call_tool(name: &str, arguments: Value) -> Result<Value, ToolCallError> {
    let spec = ...;
    let params = build_rpc_params(spec.name, arguments)?;   // ← runs first
    match spec.name {
        ...
        "memory.store" | "memory.note" => {
            enforce_write_policy(spec.name).await?;
            validate_controller_params(&spec, &params)?;
            return dispatch_write_tool(spec.name, &params).await;
            //                                    ↑ &params, not raw `arguments`
        }
        ...
    }
}

So by the time dispatch_write_tool is reached, params already carries the slug-based key, source_type = "mcp", the default namespace = "mcp", and (for memory.note) the metadata.annotates_chunk_id linkage. Those transformations aren't dead code — they're exactly what memory_doc_put ends up receiving via the params.clone() inside dispatch_write_tool.

The existing tests in this PR pin that behaviour:

• memory_store_defaults_namespace_to_mcp asserts the built params have namespace == "mcp", source_type == "mcp", and a mcp-store-<slug> key.
• memory_note_builds_annotation_document asserts the built params include the metadata.annotates_chunk_id linkage.

Both pass at the current head. If the dispatch were sending raw client args, neither would.

—

That said, the [major] memory.note upsert-vs-append note (line 680) is correct and worth addressing before merge: keying solely on chunk_id does mean re-annotating the same chunk silently overwrites the prior note. Two clean ways through:

1. Mix a short content/timestamp hash into the key (mcp-note-{chunk_id}-{hash8(note_text)}) so the tool is genuinely append-style.
2. Update the description from "Append a note" to "Set a note" if single-note-per-chunk is the intent.

Either resolves the contract — happy to take a hint on the preferred direction so the fix lands consistently across memory.note (here) and tree.tag (#2316).

[Liohtml]

@graycyrus Thanks for the thorough security review — really appreciate the depth.

Fixed before merge (cdd5818):

• Added 64KB content size limit on both memory.store and memory.note in build_rpc_params. Rejects with a clear error message including actual vs limit sizes. Test added.

Acknowledged as follow-ups (not blocking):

• ToolOperation::Write as distinct gate — agreed, will file when the surface grows beyond store/note
• Per-write user notification — UX concern, separate issue territory
• Write-specific rate limiting — good idea for a tighter budget
• Multi-process write path documentation — SQLite WAL handles persistence; cache staleness is acceptable and documented in Add write tools to the MCP server (memory.store, memory.note, tree.tag) #2269's Q1 resolution

All 48 MCP tool tests pass.

[YOMXXX]

Status update: the current red frontend/i18n checks on this PR are from a main-side German locale parity gap, not from this PR's MCP Rust changes. I reproduced the failure on latest upstream/main: German was missing 20 English keys, which caused pnpm i18n:check, I18nContext.test.tsx, and coverage.test.ts to fail.\n\nOpened #2470 to fix the German locale gap. Once #2470 lands, rerun #2306 checks. The remaining non-CI blocker is the stale CHANGES_REQUESTED review decision from earlier reviews; all review threads are resolved, so after checks are green this should just need a fresh review/dismissal from the reviewer.

[YOMXXX]

@graycyrus Heads up: #2470 is now green and ready for review/merge. Once it lands on main, #2306 should just need a CI rerun plus a fresh review/dismissal of the stale CHANGES_REQUESTED state. I am not rerunning #2306 yet because the German i18n fix is not on main, so the same frontend/i18n checks would likely fail again.

[justinhsu1477]

@Liohtml — nice job on the content-size cap (cdd5818). And kudos for the patience with this review cycle — landing the foundation here makes #2316 a trivial rebase on top.

Looking forward to merge once @YOMXXX's #2470 clears the German-locale CI noise.

[YOMXXX]

I prepared and validated a sync branch for this PR, but cannot push directly to Liohtml:feat/mcp-write-tools from the current YOMXXX credentials (Permission to Liohtml/openhuman.git denied).

Branch available for fast-forward / cherry-pick:

• YOMXXX:fix/2306-review-sync
• Head commit: 196ca6dc
• Includes: merge from current upstream/main (7fe3dd06) plus a small German i18n fix for missing MCP settings strings.

Validation run locally on that branch:

• GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml mcp_server::tools -- --nocapture -> 48 passed, 0 failed
• pnpm i18n:check -> passed, including de missing keys = 0
• pre-push with GGML_NATIVE=OFF completed: format check, lint (0 errors / existing warnings), TypeScript compile, Tauri rust:check, and command-token lint passed

The previous reviewer findings for the MCP write-tool logic appear resolved by the current branch history. Once Liohtml:feat/mcp-write-tools is advanced to this branch, this PR should be ready for CI rerun and final reviewer approval. cc @graycyrus

[Liohtml]

@graycyrus All your review findings are resolved (confirmed in your own comments above). Could you dismiss your CHANGES_REQUESTED review so the PR can move forward? Thanks for the thorough security review — it made the code significantly better.

[senamakel]

hey @Liohtml same thing as the last pr. just resolve the tests failing in the CI and then all good to go

[YOMXXX]

Status refresh after today's PR babysit pass:

• Latest head: ce2f3056.
• CI is green: all required checks are SUCCESS; only platform E2E skips remain skipped as expected.
• Review threads check via GraphQL shows every code review thread is isResolved=true and the prior inline threads are now outdated/resolved.
• The remaining blocker appears to be the stale CHANGES_REQUESTED review state from the earlier graycyrus review, after graycyrus later confirmed the findings were resolved/false alarm.

@graycyrus could you dismiss or update the stale changes-requested review when you have a moment? @senamakel the CI issue you flagged appears resolved now.