refactor(mcp_server): extract write-dispatch and audit pipeline into write_dispatch.rs

[M3gA-Mind]

Summary

• Extracts dispatch_write_tool, audit_write, audit_write_rejection, audit_write_rejection_without_config, is_write_tool, summarize_write_args, and 6 private helpers out of the 2,758-line tools.rs into a new focused sibling module mcp_server/write_dispatch.rs
• tools.rs delegates to write_dispatch:: at all call sites; public behavior (tool responses, audit records, logging, correlation fields) is unchanged
• Adds 8 new targeted tests covering the dispatch error path, is_write_tool, now_ms, first_chars, and summarize_write_args edge cases; 7 existing tests moved into the new module

Result

File	Lines before	Lines after
tools.rs	2,758	2,330
write_dispatch.rs	—	598 (new)

All 9,490 Rust lib tests pass. TypeScript, ESLint, Prettier, and cargo fmt all clean.

Test plan

• cargo test -p openhuman --lib -- openhuman::mcp_server — 94 passed, 0 failed
• cargo fmt --all -- --check — clean
• cargo clippy -p openhuman — no errors
• pnpm --filter openhuman-app compile — clean
• pnpm --filter openhuman-app lint — 0 errors (pre-existing warnings only)
• pnpm --filter openhuman-app format:check — clean

Closes #2604

Summary by CodeRabbit

• Refactor
  • Reorganized write handling into a dedicated module for clearer structure and maintenance.
• Behavior & Reliability
  • Write operations now produce more consistent audit records and clearer write error responses.
  • Improved detection and handling of write-type tool calls to reduce unexpected failures.
• Tests
  • Added comprehensive tests covering write argument summarization, audit recording, and dispatch outcomes.

[coderabbitai]

📝 Walkthrough

Walkthrough

Extracts the write-tool dispatch, policy enforcement, and audit pipeline from tools.rs into a new write_dispatch.rs module and updates tools.rs to delegate write-tool calls to the new module while preserving MCP responses and audit semantics.

Changes

Write dispatch module extraction and integration

Layer / File(s)	Summary
Module structure and public interface setup src/openhuman/mcp_server/mod.rs, src/openhuman/mcp_server/write_dispatch.rs	Adds write_dispatch module declaration and establishes module documentation and imports for JSON, configuration, security policy, and audit handling.
Configuration loading and policy enforcement src/openhuman/mcp_server/write_dispatch.rs	load_write_config loads RPC config with timeout/error wrapping; enforce_write_policy_for_config enforces per-tool ToolOperation::Act; is_write_tool identifies write tool names.
Write dispatch and audit recording src/openhuman/mcp_server/write_dispatch.rs	dispatch_write_tool invokes the mapped RPC method, extracts resulting document/chunk ids, records success/failure audit rows asynchronously, and returns MCP-style payloads via tool_success/tool_error.
Audit rejection paths src/openhuman/mcp_server/write_dispatch.rs	audit_write_rejection records immediate rejections when config is available; audit_write_rejection_without_config attempts deferred async auditing when called before config load with fallback logging.
Supporting utilities and helpers src/openhuman/mcp_server/write_dispatch.rs	Helpers: summarize_write_args (omission/truncation per tool), summarize_rejected_write_args (adds sorted param_keys), extract_document_id, and now_ms.
Test coverage for write dispatch module src/openhuman/mcp_server/write_dispatch.rs	Comprehensive tests for summarization rules, rejected param-key handling, policy denial, envelope document-id extraction, timestamp recency, write-tool recognition, and audit insertion behaviors.
Tools.rs integration and visibility changes src/openhuman/mcp_server/tools.rs, src/openhuman/mcp_server/mod.rs	tools.rs now delegates write-tool flows to super::write_dispatch; removed the prior in-file write helpers and related tests; changed tool_success and tool_error visibility to pub(super).

Sequence Diagram

sequenceDiagram
  participant call_tool
  participant write_dispatch
  participant RPC
  participant AuditStore
  call_tool->>write_dispatch: is_write_tool(name)
  write_dispatch-->>call_tool: true/false
  call_tool->>write_dispatch: load_write_config(tool_name)
  write_dispatch->>RPC: load config (with timeout)
  RPC-->>write_dispatch: Config or error
  write_dispatch-->>call_tool: Result<Config>
  call_tool->>write_dispatch: enforce_write_policy_for_config(tool_name, config)
  write_dispatch-->>call_tool: Ok() or InvalidParams
  call_tool->>write_dispatch: dispatch_write_tool(tool_name, params, args, client_info, config)
  write_dispatch->>RPC: invoke mapped RPC method
  RPC-->>write_dispatch: result or handler error
  write_dispatch->>AuditStore: record audit row (success/failure)
  AuditStore-->>write_dispatch: row inserted
  write_dispatch-->>call_tool: Ok(tool response)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• tinyhumansai/openhuman#2566: Implements the MCP write-tool audit pipeline that this PR modularizes into write_dispatch.
• tinyhumansai/openhuman#2306: Overlapping refactor of write-tool routing and dispatch logic that this PR extracts from tools.rs.
• tinyhumansai/openhuman#2316: Changes to tree.tag and related write-tool flows that rely on the dispatch path extracted here.

Suggested reviewers

• senamakel

Poem

🐰 I hopped the code and found a nest,
Split the write-path so it can rest.
Audit logs now march in line,
Clean modules hum, tests all fine.
A carrot for CI — refactor blessed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.05% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main refactoring: extracting write-dispatch and audit pipeline into a new module.
Linked Issues check	✅ Passed	All primary objectives from issue #2604 are met: write-dispatch code extracted to new module, public behavior preserved, regression tests included, and diff coverage requirements satisfied.
Out of Scope Changes check	✅ Passed	Changes are tightly scoped to the refactoring objective with no unrelated modifications; visibility changes to tool_success and tool_error are necessary for module delegation.

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

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.

---

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

[coderabbitai]

🧹 Nitpick comments (1)

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

508-546: ⚡ Quick win

Make this test prove the handler-error branch.

This still passes if openhuman.memory_doc_put is unregistered and dispatch_write_tool takes the None path, so it doesn't actually pin the Some(Err(_)) branch named in the test. Assert on the exact returned/audited error or set up a deterministic failing handler here.

🤖 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/write_dispatch.rs` around lines 508 - 546, The test
currently can pass via the None-path; make it exercise the Some(Err(_)) branch
by explicitly registering a deterministic failing write handler for the
"memory.store" tool before calling dispatch_write_tool (or otherwise ensure
openhuman.memory_doc_put is registered to return Err). Specifically, install a
handler that returns Some(Err("deterministic failure")) for "memory.store", then
call dispatch_write_tool and assert the returned tool_error content and the
audit row from list_writes (via McpWriteListQuery) contains the same
deterministic error message and success=false to prove the handler-error branch
executed.

🤖 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/write_dispatch.rs`:
- Around line 508-546: The test currently can pass via the None-path; make it
exercise the Some(Err(_)) branch by explicitly registering a deterministic
failing write handler for the "memory.store" tool before calling
dispatch_write_tool (or otherwise ensure openhuman.memory_doc_put is registered
to return Err). Specifically, install a handler that returns
Some(Err("deterministic failure")) for "memory.store", then call
dispatch_write_tool and assert the returned tool_error content and the audit row
from list_writes (via McpWriteListQuery) contains the same deterministic error
message and success=false to prove the handler-error branch executed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bf02c5ce-1c3f-4e9c-8c1b-523f50586620

📥 Commits

Reviewing files that changed from the base of the PR and between 0d62e84 and 1c1a2ef.

📒 Files selected for processing (3)

• src/openhuman/mcp_server/mod.rs
• src/openhuman/mcp_server/tools.rs
• src/openhuman/mcp_server/write_dispatch.rs

[coderabbitai]

🧹 Nitpick comments (2)

src/openhuman/mcp_server/write_dispatch.rs (2)

203-218: 💤 Low value

Timestamp captured inside async task may drift from rejection time.

The now_ms() call on line 210 occurs inside the spawned async task after load_config_with_timeout() completes. In contrast, audit_write_rejection (line 175) captures the timestamp immediately. If config loading is slow, the audit timestamp will reflect when the config loaded rather than when the rejection occurred.

Consider capturing the timestamp before spawning:

⏱️ Suggested fix

     let tool_name = tool_name.to_string();
     let client_info = client_info.to_string();
     let error_message = error_message.to_string();
     let args_summary = summarize_write_args(&tool_name, audit_arguments);
+    let timestamp_ms = now_ms();
     match tokio::runtime::Handle::try_current() {
         Ok(handle) => {
             let _ = handle.spawn(async move {
                 match config_rpc::load_config_with_timeout().await {
                     Ok(config) => audit_write(
                         &config,
                         NewMcpWriteRecord {
-                            timestamp_ms: now_ms(),
+                            timestamp_ms,
                             client_info,

🤖 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/write_dispatch.rs` around lines 203 - 218, The
timestamp for the rejection should be captured before spawning the async task so
it reflects the actual rejection time rather than when config loading completes;
modify the block around tokio::runtime::Handle::try_current() to call now_ms()
into a local variable (e.g., captured_ts) before creating the closure, then move
that captured_ts into the async move and use it when constructing
NewMcpWriteRecord in audit_write (similar to how audit_write_rejection captures
time); update references to now_ms() inside the spawned task to use the captured
variable and keep use of load_config_with_timeout() and audit_write unchanged
otherwise.

---

58-58: Confirm write tool → RPC routing is consistent

memory.store, memory.note, and tree.tag are all registered in src/openhuman/mcp_server/tools.rs with rpc_method: Some("openhuman.memory_doc_put"), so the hardcoded value in src/openhuman/mcp_server/write_dispatch.rs matches the existing routing.

🤖 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/write_dispatch.rs` at line 58, The code hardcodes
let rpc_method = "openhuman.memory_doc_put" in write_dispatch.rs which must
match the rpc_method used when registering tools (memory.store, memory.note,
tree.tag) in tools.rs; update the code to remove the literal by introducing and
using a single shared identifier (e.g., a pub const OPENHUMAN_MEMORY_DOC_PUT)
and replace the literal in write_dispatch.rs with that constant (and use it in
tools.rs registrations), or if a constant already exists, reference that
constant from write_dispatch.rs instead so routing is guaranteed consistent
across memory.store, memory.note, tree.tag and write_dispatch::rpc_method.

🤖 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/write_dispatch.rs`:
- Around line 203-218: The timestamp for the rejection should be captured before
spawning the async task so it reflects the actual rejection time rather than
when config loading completes; modify the block around
tokio::runtime::Handle::try_current() to call now_ms() into a local variable
(e.g., captured_ts) before creating the closure, then move that captured_ts into
the async move and use it when constructing NewMcpWriteRecord in audit_write
(similar to how audit_write_rejection captures time); update references to
now_ms() inside the spawned task to use the captured variable and keep use of
load_config_with_timeout() and audit_write unchanged otherwise.
- Line 58: The code hardcodes let rpc_method = "openhuman.memory_doc_put" in
write_dispatch.rs which must match the rpc_method used when registering tools
(memory.store, memory.note, tree.tag) in tools.rs; update the code to remove the
literal by introducing and using a single shared identifier (e.g., a pub const
OPENHUMAN_MEMORY_DOC_PUT) and replace the literal in write_dispatch.rs with that
constant (and use it in tools.rs registrations), or if a constant already
exists, reference that constant from write_dispatch.rs instead so routing is
guaranteed consistent across memory.store, memory.note, tree.tag and
write_dispatch::rpc_method.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dc9df06a-24af-4086-b084-90e1507a7432

📥 Commits

Reviewing files that changed from the base of the PR and between 1c1a2ef and 949df25.

📒 Files selected for processing (3)

• src/openhuman/mcp_server/mod.rs
• src/openhuman/mcp_server/tools.rs
• src/openhuman/mcp_server/write_dispatch.rs

✅ Files skipped from review due to trivial changes (1)

• src/openhuman/mcp_server/mod.rs