feat(mcp): add tree.tag write tool (completes Phase 3 #2269)

[justinhsu1477]

Summary

Adds the third MCP write tool from the Phase 3 RFC (#2269) on top of @Liohtml's memory.store + memory.note scaffolding in #2306, so the RFC closes out fully.

• New tree.tag MCP tool: attach one or more category labels to an existing memory chunk.
• Routes through the existing openhuman.memory_doc_put RPC.
• Reuses enforce_write_policy + dispatch_write_tool from feat(mcp): add memory.store and memory.note write tools #2306 unchanged — inherits the same policy gate and audit-log tracing.
• Same source_type = "mcp" provenance convention as memory.store / memory.note.

Depends on #2306

This PR is layered directly on top of #2306. The diff here only adds the new tool surface; if #2306 changes during review (e.g. CodeRabbit's open CHANGES_REQUESTED items), I'll rebase this branch.

What tree.tag does

{
  "name": "tree.tag",
  "arguments": {
    "chunk_id": "chunk-42",
    "tags": ["todo", "q3-planning"]
  }
}

→ Creates / upserts a tag-record document keyed on mcp-tag-<chunk_id> in the mcp namespace, with:

• tags = ["todo", "q3-planning"] on the top-level document tags index (queryable downstream via doc_list / search filters).
• metadata.tags_for_chunk_id = "chunk-42" — back-reference so consumers reading the metadata view know which chunk the tag-record points at.
• metadata.applied_tags = ["todo", "q3-planning"] — mirror of the tag list for callers that read the metadata blob without joining against the top-level field.

How it differs from memory.note

	memory.note	tree.tag
Payload shape	Free-form text (note_text)	Typed categorical list (tags[])
Re-call semantic	Upsert by chunk_id (silently overwrites — see #2306 for the open contract discussion)	Upsert: re-tagging replaces the prior tag set
Queryable index	Opaque content	Top-level tags index (filterable)
Key derivation	mcp-note-<chunk_id>	mcp-tag-<chunk_id>

Both upsert by chunk_id. As graycyrus noted in #2306, memory.note overwrites on re-call — whether that should change to a content-hash key (true append) or stay as set-semantic is the open contract discussion happening on the parent PR. tree.tag is unambiguously replace-on-re-call: supply the complete desired set each time.

Implementation footprint

src/openhuman/mcp_server/protocol.rs    |   1 +
src/openhuman/mcp_server/tools.rs       | 242 +++++++++++++++++++++++++-
2 files changed, 242 insertions(+), 1 deletion(-)

Plus a small reusable helper required_non_empty_string_array added next to its sibling optional_string_array — used to reject tags: [] up-front at the MCP layer rather than letting an empty tag-set propagate to the document RPC where the failure mode would be silent.

Submission Checklist

• Tests added or updated — 8 new unit tests cover: missing chunk_id, missing tags, empty tags array, all-blank tags, non-string tag entries, happy path, blank-trim with non-empty residue, empty chunk_id rejection, unknown-argument rejection.
• Diff coverage ≥ 80% — new code is overwhelmingly build_rpc_params validation + schema, fully exercised by the new unit tests; helper function paths covered by the same tests.
• N/A: Coverage matrix updated — extends the same MCP write tool surface row added in feat(mcp): add memory.store and memory.note write tools #2306; no new matrix feature row.
• N/A: All affected feature IDs from the matrix are listed — extends an existing capability.
• No new external network dependencies introduced — pure RPC routing through existing memory_doc_put.
• N/A: Manual smoke checklist updated — no release-cut surface changes.
• Linked issue closed via Closes #2269 in ## Related (transitively — feat(mcp): add memory.store and memory.note write tools #2306 already declares Closes #2269; this PR completes the RFC's three-tool surface so it's logically the closing piece).

Impact

• Runtime/platform impact: desktop core only (Rust). No Tauri shell, no frontend.
• Compatibility impact: strictly additive. Schema for the existing tools unchanged. memory.store / memory.note behavior unchanged.
• Performance impact: zero on existing flows; new path is one extra dispatch arm and one extra match branch in build_rpc_params.
• Security impact: inherits enforce_write_policy gate from feat(mcp): add memory.store and memory.note write tools #2306 — no new attack surface relative to that PR.

Related

• Builds on feat(mcp): add memory.store and memory.note write tools #2306 (memory.store + memory.note, by @Liohtml).
• RFC: Add write tools to the MCP server (memory.store, memory.note, tree.tag) #2269 (authored this issue describing the three-tool Phase 3 surface).
• Follow-up: client_info propagation — capturing the calling MCP client name on each written chunk (e.g. source_type = "mcp:Claude Desktop") — separate issue per feat(mcp): add memory.store and memory.note write tools #2306 review thread, will file once this lands.

---

AI Authored PR Metadata

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: feat/mcp-tree-tag-write-tool
• Commit SHA: 19a8694
• Base: 79026df (head of feat(mcp): add memory.store and memory.note write tools #2306 at time of writing)

Validation Run

• N/A: pnpm --filter openhuman-app format:check — Rust-only change.
• N/A: pnpm typecheck — Rust-only change.
• Focused tests: cargo test --lib tree_tag (8/8 pass); cargo test --lib mcp_server (59/59 pass, no regression on memory.store / memory.note from feat(mcp): add memory.store and memory.note write tools #2306).
• Rust fmt/check: cargo fmt --check clean; cargo check --lib clean (pre-existing warnings only); cargo clippy --lib --no-deps no new warnings in mcp_server/tools.rs.
• N/A: Tauri fmt/check — no app/src-tauri/src/** changes.

Validation Blocked

• N/A

Behavior Changes

• Intended behavior change: MCP clients gain a third write tool — tree.tag — that attaches typed category labels to existing memory chunks.
• User-visible effect: an LLM conversation in (e.g.) Claude Desktop can now categorize chunks of OpenHuman's memory tree with structured tags that are queryable downstream, completing the read-and-write loop the RFC proposed.

Parity Contract

• Legacy behavior preserved: existing core.* / agent.* / memory.* / tree.* (read_chunk / browse / top_entities / list_sources) tool schemas and dispatch are unchanged. memory.store and memory.note from feat(mcp): add memory.store and memory.note write tools #2306 are unchanged.
• Guard/fallback/dispatch parity checks: enforce_write_policy gate inherited from feat(mcp): add memory.store and memory.note write tools #2306; dispatch_write_tool inherited from feat(mcp): add memory.store and memory.note write tools #2306; both reused without modification.

Duplicate / Superseded PR Handling

• Duplicate PR(s): none — confirmed no concurrent PR for tree.tag via search before opening. @Liohtml explicitly invited this follow-up in the feat(mcp): add memory.store and memory.note write tools #2306 review thread.
• Canonical PR: this PR.
• Resolution: N/A.

Summary by CodeRabbit

• New Features

  • Added three tools for memory storage, note creation, and content tagging. They include enforced write security, deterministic keying and slugging for stored items, sensible defaults (e.g., default namespace), strict tag limits/trimming, and robust parameter validation to ensure consistent persistence and tagging behavior.

• Tests

  • Expanded coverage for tool inclusion, parameter validation/rejection, key/content generation, tag semantics and limits, slug generation, and write-path auditing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ca8944b5-00e7-4300-b715-f7f86ea8fc74

📥 Commits

Reviewing files that changed from the base of the PR and between 3c50c97 and dbcd678.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

---

📝 Walkthrough

Walkthrough

Adds three MCP write tools—memory.store, memory.note, and tree.tag—with JSON input schemas, write-gated call dispatch to openhuman.memory_doc_put, parameter-building/validation (including slug_from), and unit tests; also updates the tools/list test expectations.

Changes

MCP Write Tools: memory.store, memory.note, tree.tag

Layer / File(s)	Summary
Tool catalog, constants, and input schemas src/openhuman/mcp_server/tools.rs (lines 29–407)	Adds MCP argument-name constants, TREE_TAG_* caps, registers memory.store, memory.note, tree.tag in base_tool_specs, and provides memory_store_schema, memory_note_schema, and tree_tag_schema.
call_tool write dispatch & security src/openhuman/mcp_server/tools.rs (lines 504–1148)	Routes the three tools through a write path in call_tool, enforces write policy via enforce_write_policy, validates controller params, and centralizes write RPC invocation with dispatch_write_tool.
Parameter building, validation, slug helper src/openhuman/mcp_server/tools.rs (lines 690–1357)	Adds build_rpc_params branches for the new tools with argument validation, namespace defaulting, deterministic key generation (slug_from), content/metadata formatting, tag trimming/limits, and required_non_empty_string_array.
Tests and updated tool-list expectations src/openhuman/mcp_server/protocol.rs (lines 350–360), src/openhuman/mcp_server/tools.rs (lines 1397–2136)	Updates tools/list test expectations and adds unit tests covering required-field validation, namespace defaults, tag behavior and limits, slug generation/truncation/hash fallback, and document construction semantics.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• tinyhumansai/openhuman#2268: Also modifies MCP tool surface emission and tests in tools.rs/protocol.rs, with overlapping tool-list changes.
• tinyhumansai/openhuman#1760: Introduced initial MCP tools and call_tool dispatch that this PR extends with write handling and new tools.
• tinyhumansai/openhuman#1974: Related tool-list additions and tests for tree.* tools affecting protocol expectations.

Suggested reviewers

• senamakel

Poem

🐰 I hop through the code with a carrot and tag,

memory.store nests facts in a soft little bag,
memory.note whispers where annotations belong,
tree.tag strings labels to keep order strong,
Happy hops — the memory garden grows along!

🚥 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	Title accurately describes the main change: adding the tree.tag write tool. References the phase completion and issue number for context.
Linked Issues check	✅ Passed	PR successfully implements tree.tag write tool from RFC #2269 with required parameter validation, tag caps, security policy enforcement, and comprehensive tests meeting stated objectives.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to tree.tag tool implementation and related test updates. No unrelated modifications or feature creep detected.
Docstring Coverage	✅ Passed	Docstring coverage is 92.86% 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.}

[graycyrus]

Review — tree.tag write tool (Phase 3 completion)

Solid, well-documented PR. The tree.tag tool is cleanly implemented, validation is thorough (especially required_non_empty_string_array), and the 8 new unit tests are comprehensive. Good use of upsert semantics keyed on chunk_id and the tag-index design for queryability.

One semantic issue in memory.note that should be addressed before merge.

Area	Files	Verdict
MCP tool specs	tools.rs (schemas + dispatch)	1 major, 2 minor
Protocol test	protocol.rs	✅ Clean

Nice work on the slug fallback for non-ASCII titles — the SHA-256 hash approach is a good call.

[graycyrus]

[major] memory.note has an upsert-vs-append semantic mismatch.

The key is mcp-note-{chunk_id} and it routes through memory_doc_put — so calling memory.note twice on the same chunk_id silently replaces the first note. But the tool description above says "Append a note" and the PR description table lists the re-call semantic as "Append-style annotation per call."

This is confusing for MCP clients: an LLM that calls memory.note multiple times on the same chunk expecting accumulated annotations will instead get only the last one.

Options:

1. Change the key to include a nonce/timestamp (e.g. mcp-note-{chunk_id}-{unix_millis}) so each call creates a distinct document — actual append semantics.
2. Keep upsert but fix the description to say "Upsert a note" and document that re-noting replaces the prior annotation (matching tree.tag's honesty about this).

Either way, the description and behavior need to agree.

[graycyrus]

[minor] rpc_method is hardcoded to "openhuman.memory_doc_put" here, but each tool spec already declares this via rpc_method: Some("openhuman.memory_doc_put"). If a future write tool uses a different RPC method, this function will silently send it to the wrong endpoint.

Consider passing the McpToolSpec (or at least spec.rpc_method) into this function instead of hardcoding.

[graycyrus]

[minor] slug_from maps non-alphanumeric chars to hyphens, so titles that differ only in punctuation collide silently. E.g. "Hello World!" and "Hello World?" both produce key mcp-store-hello-world, causing one to overwrite the other.

This may be acceptable if title-based dedup is intentional, but it's worth a comment acknowledging the trade-off — or mixing in a short content hash to make collisions less likely.

[justinhsu1477]

@graycyrus — thanks for the careful read. All three inline notes are valid concerns, and all three are in code that landed in #2306 (dispatch_write_tool, slug_from, and the mcp-note-<chunk_id> key shape), not in this PR's new surface. To avoid forking the fix into two PRs:

1. [major] line 705 — memory.note upsert-vs-append mismatch — Real. The key derivation lives in feat(mcp): add memory.store and memory.note write tools #2306's build_rpc_params("memory.note") arm. Posting a separate technical note on feat(mcp): add memory.store and memory.note write tools #2306 so the fix lands at the source. In parallel I'll update this PR's description (the comparison table calls memory.note's re-call semantic "Append-style") to say "upsert" instead, so the docs match the actual behaviour until feat(mcp): add memory.store and memory.note write tools #2306 settles the contract.

2. [minor] line 1072 — dispatch_write_tool hardcoded rpc_method — Agreed. Passing spec.rpc_method through would future-proof for any write tool that doesn't route through memory_doc_put. Same function (feat(mcp): add memory.store and memory.note write tools #2306's), best refactored at the source.

3. [minor] line 686 — slug_from punctuation collisions — Also in feat(mcp): add memory.store and memory.note write tools #2306. A short content-hash mix-in, or an explicit "intentional dedup-on-title" comment, resolves it either way.

tree.tag itself doesn't introduce upsert/append ambiguity — re-tag replaces, stated explicitly in the schema description ("Re-tagging the same chunk replaces the prior tag set; supply the complete desired set on each call") — and doesn't introduce its own slug path. Once these land in #2306 this PR rebases cleanly.

[graycyrus]

Security & Robustness Review — Final Verdict

Clean, well-scoped PR. The tree.tag implementation is tight — required_non_empty_string_array catches empty/blank tag arrays at the MCP layer, the upsert-replace semantic is explicitly documented in the schema description, and the 8 unit tests cover every validation path. Good call reusing enforce_write_policy + dispatch_write_tool from #2306 without modification — keeps the security surface consistent.

Overall: this is good to merge (once #2306 lands and you rebase).

Two items worth noting, both minor:

Tag-specific concerns

1. Tag value sanitization — Tags are trimmed and blank entries are filtered, which is good. But there's no max-length or max-count cap on the tags array. A client could send 1000 tags or a single tag that's 10KB of text. A lightweight cap (e.g. max 50 tags, max 128 chars per tag) would match the "explicit rejection" pattern used elsewhere in the MCP layer and prevent index bloat.

2. Silent tag replacement — The replace-on-re-call semantic is the right design and it's clearly documented. Just flagging that from a user's perspective, if an LLM re-tags a chunk, the prior tag set is gone with no undo. This ties into the broader "UI notification for MCP writes" recommendation on feat(mcp): add memory.store and memory.note write tools #2306 — users should ideally see when their tags get modified by an external client.

Already covered by #2306's foundation

The content size limit, ToolOperation::Write gate, per-write notification, and multi-process concerns I raised on #2306 apply equally here but are inherited from that PR's scaffolding — no action needed on this side.

Nice work closing out the Phase 3 RFC. The three-tool write surface (memory.store, memory.note, tree.tag) is a well-structured, incremental addition. 👍

[justinhsu1477]

@graycyrus thanks for the thorough review.

Minor #1 (tag-array sanitization) — addressed in 3c50c975. Added TREE_TAG_MAX_TAGS = 50 and TREE_TAG_MAX_TAG_LENGTH = 128 caps in build_rpc_params("tree.tag"), both rejecting up-front via ToolCallError::InvalidParams (same explicit-rejection style as required_non_empty_string_array). Three new tests cover:

• Oversize array (MAX_TAGS + 1 entries) → rejected.
• Oversize single entry (MAX_TAG_LENGTH + 1 chars) → rejected.
• Boundary acceptance (exactly MAX_TAGS entries × MAX_TAG_LENGTH chars) → succeeds, locking in inclusive bounds against a future off-by-one refactor.

11 tree_tag unit tests pass (was 8; +3 new), all 62 mcp_server tests still pass — no regression.

Minor #2 (silent tag replacement → UI notification) — agreed this lives at the broader "MCP write surface UI" follow-up theme you raised on #2306 rather than being tree.tag-specific. Will file a separate issue once the foundation PRs (#2306, #2316) merge so the UX scope can span all three write tools at once. Not blocking this PR.

Ready for re-review.

[coderabbitai]

🧹 Nitpick comments (1)

src/openhuman/mcp_server/tools.rs (1)

752-756: 💤 Low value

Byte length vs character count terminology.

String::len() returns byte length, but the error message says "characters". For ASCII tags (typical for categorical labels) this is equivalent, but for Unicode input (e.g., "会议" is 2 characters but 6 bytes) the reported count would be misleading.

Consider either changing the message to say "bytes" or using .chars().count() if true character-count semantics are intended. Given tags are categorical labels (expected ASCII), this is a minor documentation nit.

Option A: Update terminology (simpler)

             if let Some(oversize) = tags.iter().find(|t| t.len() > TREE_TAG_MAX_TAG_LENGTH) {
                 return Err(ToolCallError::InvalidParams(format!(
-                    "argument `tags` entry exceeds {TREE_TAG_MAX_TAG_LENGTH} characters (got {} chars)",
+                    "argument `tags` entry exceeds {TREE_TAG_MAX_TAG_LENGTH} bytes (got {} bytes)",
                     oversize.len()
                 )));
             }

🤖 Prompt for 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.

In `@src/openhuman/mcp_server/tools.rs` around lines 752 - 756, The error message
reports "characters" but uses String::len() (byte length); update the code that
constructs the ToolCallError in the oversize branch (the tags.iter().find(...)
check that returns ToolCallError::InvalidParams) to either report "bytes"
instead of "characters" to match oversize.len(), or replace oversize.len() with
oversize.chars().count() if you want true character counts; keep the referenced
constant TREE_TAG_MAX_TAG_LENGTH unchanged and ensure the formatted value
reflects the chosen unit (bytes or characters).

🤖 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.

Nitpick comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 752-756: The error message reports "characters" but uses
String::len() (byte length); update the code that constructs the ToolCallError
in the oversize branch (the tags.iter().find(...) check that returns
ToolCallError::InvalidParams) to either report "bytes" instead of "characters"
to match oversize.len(), or replace oversize.len() with oversize.chars().count()
if you want true character counts; keep the referenced constant
TREE_TAG_MAX_TAG_LENGTH unchanged and ensure the formatted value reflects the
chosen unit (bytes or characters).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 444a0ba3-d376-4b30-b4d9-06c337374de9

📥 Commits

Reviewing files that changed from the base of the PR and between 19a8694 and 3c50c97.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

[justinhsu1477]

Quick CI status note for anyone scanning this surface:

• cargo fmt failure on the earlier push fixed in dbcd6783.
• Remaining Rust Quality (clippy) failure is a stale-base issue: McpToolSpec gained an annotations field in feat(mcp): advertise tool annotations on tools/list #2268 (merged), but this branch was forked from feat(mcp): add memory.store and memory.note write tools #2306's pre-rebase head and inherits the older struct definition without it. The merge against current main surfaces E0063: missing field 'annotations' on the new memory.store / memory.note / tree.tag initializers.
• Frontend / i18n failures are the German-locale parity gap on main that @YOMXXX is fixing in fix(i18n): complete German MCP translations #2470 — same root cause as the CI red on feat(mcp): add memory.store and memory.note write tools #2306, per their note there.

Plan: once #2470 lands and @Liohtml rebases #2306 onto new main, I'll do the matching rebase here and add annotations: … to the tree.tag initializer. No action needed in the meantime.

[YOMXXX]

Status link for the dependency chain: I prepared and validated a #2306 sync branch at YOMXXX:fix/2306-review-sync, but cannot push directly to Liohtml:feat/mcp-write-tools from the current credentials. Details are on #2306.

This PR still has three unresolved review threads pointing at src/openhuman/mcp_server/tools.rs from the #2306 foundation layer (memory.note, dispatch_write_tool, slug_from). I would keep #2316 unchanged until #2306 advances, then rebase #2316 so those inherited threads become stale/resolved and CI can rerun against current main.

[senamakel]

Status update (PR-finisher pass on 2026-05-22T20:02Z)

Pushed 560a4c2 to address what was actionable without forking the #2306 fixes:

1. CodeRabbit nit (tools.rs:803, characters → bytes) — applied. String::len() returns bytes, so the error message now reads "exceeds {N} bytes (got {} bytes)" for consistency on Unicode tag input.
2. CI: McpToolSpec.annotations missing on the three new specs — applied. Added write_local_annotations() (readOnlyHint: false, destructiveHint: true, idempotentHint: true, openWorldHint: false) for memory.store / memory.note / tree.tag, and extended act_tool_names in read_only_tools_are_marked_read_only_and_closed_world so the readOnly contract test still passes. cargo check --lib, cargo clippy --lib --no-deps -D warnings, and cargo test --lib mcp_server (78/78) all green locally. Should clear the Rust Quality red.
3. Frontend / i18n / Coverage / E2E reds — left untouched; per @justinhsu1477's earlier note these are the stale-base German-locale gap @YOMXXX is unblocking via fix(i18n): complete German MCP translations #2470 + feat(mcp): add memory.store and memory.note write tools #2306 rebase. Not in scope for feat(mcp): add tree.tag write tool (completes Phase 3 #2269) #2316.

Deferred per @justinhsu1477's review reply (not addressed here):

• @graycyrus tools.rs:705 — memory.note upsert-vs-append semantic mismatch.
• @graycyrus tools.rs:1072 — dispatch_write_tool hardcoded rpc_method.
• @graycyrus tools.rs:686 — slug_from punctuation-collision risk.

All three live in #2306's foundation code (build_rpc_params("memory.note"), dispatch_write_tool, slug_from) and the author has explicitly chosen to fix them at the source so this branch inherits cleanly on rebase. Flagging here so the threads aren't lost.